feat(gate): choose provider, model, effort and self-consistency runs — per repo, and escalated on guarded paths - #9821
Merged
Merged
Conversation
…— per repo, and escalated on guarded paths
Effort DID already exist (CLAUDE_AI_EFFORT -> `claude --effort`, CODEX_AI_EFFORT ->
model_reasoning_effort) but only as a GLOBAL env var, while provider and model were
already per-repo manifest fields. Self-consistency runs had the same asymmetry.
So the two knobs that most directly control review depth were the two a repo could
not set.
Close that, and use it where it matters most. Adds:
gate.aiReview.effort / .selfConsistencyRuns — per-repo parity with the env vars
gate.guardrailEscalation.{provider,model,effort,selfConsistencyRuns}
— used INSTEAD when a PR touches a
hardGuardrailGlobs path
Precedence resolves per field independently: escalation > per-repo > global. An
escalation block that sets only `effort` still inherits the repo's model and the
global provider -- the common ask is "same model, think harder", not "swap the
model out".
WHY: a guardrail hit used to change exactly one thing -- it suppressed auto-merge
and queued a human. It bought no extra analysis at all: a PR touching
.github/workflows/** got the same single-pass, same-model, same-effort review as a
README typo. Measured on the production ORB that was 74 distinct PRs held in 14
days, on repos whose guardrail lists had already been narrowed twice. A guarded
path can now buy MORE SCRUTINY instead of maintainer toil; manual review stays the
fallback when the escalated review is not clean.
Every layer unset ⇒ null throughout ⇒ byte-identical to today for every existing
repo and self-hoster. An escalation is logged (ai_review_guardrail_escalated) and
describeReviewEscalation renders the one-line "why" for the panel/decision record.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | e98bbb2 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 02:26 PM |
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 15:08:06 UTC
Review summary Blockers
Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Bundle ReportChanges will increase total bundle size by 1.83kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9821 +/- ##
==========================================
+ Coverage 91.65% 91.74% +0.09%
==========================================
Files 916 917 +1
Lines 112763 112868 +105
Branches 27087 27143 +56
==========================================
+ Hits 103348 103550 +202
+ Misses 8126 8034 -92
+ Partials 1289 1284 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…nvocation Review blocker on #9821, and correct: resolveReviewKnobs was computed, logged, and then DROPPED. Only selfConsistencyRuns was consumed, so "choose provider, model, effort" was unimplemented at the one place it takes effect -- and the doc comment claiming the fields "ride through to the provider invocation" was false. Apply them where review.ai_model's overrides already land: claudeModel/codexModel/ollamaModel/openaiModel/openaiCompatibleModel/ anthropicModel <- reviewKnobs.model ?? review.ai_model ?? env claudeEffort/codexEffort <- reviewKnobs.effort ?? ... ?? env reviewKnobs wins over review.ai_model because it is strictly more specific: it is the only layer that can differ PER PR, and an escalation existing at all means it should win for the PR that triggered it. Unset ⇒ `??` falls straight through, so a repo using only review.ai_model is untouched. Provider also now governs the BYOK key. The providerKey gate runs before the knobs are resolved and only knew settings.aiReviewProvider, so an ESCALATED provider would have kept using a stored key belonging to a different one. Same rule, re-applied with the resolved value: a mismatch drops the key. Also re-syncs config/examples/loopover.full.yml with .loopover.yml.example -- caught by running the FULL suite this time, which is the same break class that took main down after #9813.
…ard the coverage exposed Codecov flagged 39 uncovered lines on #9821, and closing them found a REAL bug: gateConfigToJson's outer aiReview guard was never taught the two new fields, so a manifest whose aiReview block set ONLY effort/selfConsistencyRuns serialized to NOTHING -- the setting silently vanished on the next snapshot reload. The new round-trip assertion failed on exactly that before touching coverage at all. (An earlier edit had aimed at this guard and hit the presence expression instead; the round-trip test is what caught the difference.) Coverage added: - engine parse: full/partial/invalid/non-mapping guardrailEscalation, invalid effort + runs values warn and stay null, presence flips on EACH of the six fields alone (the #9813 presence-gap class, per field), serialize round-trips full AND partial blocks - resolution: every field lands on effective settings; absent leaves the stored value untouched - END-TO-END escalation: env self-consistency OFF, manifest hardGuardrailGlobs + guardrailEscalation.selfConsistencyRuns=3, a guarded changed file -- the pipeline delivers 1 primary + 2 rotated-exemplar extra calls, proving the resolved knobs reach the invocation (the #9821 review blocker, now pinned by an integration test, not just the precedence mirror) - INERT invariant: same escalation block, no guarded file -- zero selfConsistency usage rows (counted via the usage marker, not raw AI.run calls, matching the flag-off baseline's discipline)
# Conflicts: # packages/loopover-contract/src/api-schemas.ts
…where the reviewer looked Re-review blocker: the diff still showed only selfConsistencyRuns consumed in src/services/ai-review.ts. The orchestration DID pre-apply model/effort to the claude*/codex*/HTTP-model fields, so behavior was correct -- but this module, which owns the doc comment claiming the fields "ride through to the provider invocation", never read them, and any OTHER caller passing reviewKnobs would have been silently ignored. Fold reviewKnobs.model/effort into the AiRunCorrelation, which IS what becomes the provider's per-call options at this module's dispatch. Same ?? precedence as the orchestration, so applying it in both places is idempotent, and this module is now self-sufficient. Correct the doc comment to state exactly what is consumed WHERE, including that `provider` is deliberately NOT consumed here -- it governs BYOK key selection at the orchestration, before this runs, and rides along for logging/replay only. Test captures the options the provider actually receives (claudeEffort/ claudeModel/codexEffort/anthropicModel) for a guarded-path PR with an escalation block, rather than asserting on the precedence mirror.
Codecov kept reporting focus-manifest.ts at ~65% despite the root vitest suite covering every added line. The reports disagree because they measure differently: the root v8 run instruments only the 11 lines it actually executes from that source, while the engine's own c8 run uses --all over dist/ and instruments all 66 -- and the engine's node:test suite never exercised the new branches. Codecov merges both, so 21 lines read as uncovered no matter how thorough the vitest tests were. Add the engine-side tests: parse, per-field presence, serialize round-trip (full AND partial -- the case whose guard was genuinely broken), invalid-value rejection, non-mapping escalation, and absent-means-null including that the serializer emits no escalation block at all. Engine c8 now reports 0 of 66 added lines uncovered, down from 21.
Owner
Author
The blocker is stale — it reviewed a commit from 93 minutes before the fixThe original flag was correct and useful — the first implementation put the wiring in the orchestration only, so On the current head, claudeModel: input.reviewKnobs?.model ?? input.claudeModel ?? undefined,
claudeEffort: input.reviewKnobs?.effort ?? input.claudeEffort ?? undefined,
codexModel: input.reviewKnobs?.model ?? input.codexModel ?? undefined,
codexEffort: input.reviewKnobs?.effort ?? input.codexEffort ?? undefined,
// + ollamaModel / openaiModel / openaiCompatibleModel / anthropicModel, same precedenceSame On
|
This was referenced Jul 29, 2026
This was referenced Jul 29, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
… hold when the escalated review is clean The second half of #9808, as an explicit MODE rather than a policy flip. #9821 shipped the escalation knobs, so a guarded PR now gets a high-effort review -- but the disposition still held it unconditionally: guardrailHit sat in heldForManualReview regardless of what the escalated review found. The 74 held PRs / 14 days kept landing on the maintainer, just with better reviews attached. gate.guardrailEscalation.onCleanReview: hold (default) today's behavior exactly -- human-in-the-loop proceed a clean escalated review releases the guardrail hold and the normal approve/merge path continues -- full-autonomy mode Because it is a manifest field it layers global -> per-repo like everything else, so repos can be flipped one at a time and the mode is reversible without a deploy. Fail-closed on every axis: - `proceed` is inert unless at least one escalation knob is actually SET: the release is justified by extra scrutiny, so absent scrutiny nothing vouches. - the release requires reviewGood -- gate success (which folds in the AI verdict's blockers) AND green CI -- and the disposition re-checks that independently of the caller's flag. - only the guardrail term is released: migration collisions, unlinked-issue holds, advisory-check holds, and unstable merge states all still hold. - the manual-hold reason no longer claims "guarded path -> manual review" for a cleared PR.
JSONbored
added a commit
that referenced
this pull request
Jul 30, 2026
…stry, write down the coverage rule (#9949) * chore(guards): pin the focus-manifest field registry, document the coverage asymmetry Refs #9860 (items 4 and 5). ITEM 4. FOCUS_MANIFEST_TOP_LEVEL_FIELDS is a hand-kept list of 28 keys whose doc comment claims it is every top-level key parseFocusManifest actually reads. Nothing checked that claim, and it is load-bearing both ways: a key the parser reads but the list omits is reported to operators as an unknown field, while a key in the list the parser never reads is worse -- the validator blesses it, the runtime ignores it, and the operator's setting silently does nothing while every surface says it is fine. #9813 and #9821 each shipped a bug from missing one of this registry's touchpoints. The relation is now computed from the parser's own source rather than trusted. Currently in sync at 28/28, so this pins today's state rather than fixing a live bug. Source-scanned deliberately: the failure is a key the parser reads and the list forgot, which produces no behavioural difference to assert on. Includes an anti-vacuity check, because a refactor renaming the local would otherwise empty both sets and make the assertions pass while checking nothing. Not guarded: the parse/serialize pairing. Four parsers (AutoReview, MaxFindings, SelfHostAiModel, Visual) have no *ConfigToJson of their own because they are nested under the review config and serialized through it, so a naive 1:1 check would fire four false positives -- and a checker that cries wolf gets muted. ITEM 5. The engine coverage asymmetry is now a contributor-facing rule rather than folklore. packages/loopover-engine/src/** is credited by two uploads whose hits Codecov unions, and the two runs disagree about which lines exist (c8 --all instruments files no test imports; v8 does not), so a genuinely 100%-tested engine change can land at ~65% on codecov/patch. The rule: change engine source, add the test to packages/loopover-engine/test/** as well. Also corrects AGENTS.md, which stated that only src/** is measured -- untrue since the engine flag, and misleading in exactly the direction that costs a PR. And records the issue's own closing note: ui:typecheck is in test:ci but not in the root typecheck script, so a locally-clean type change can still break apps/** (#9815). * chore(mcp): derive the help banner's --login command list instead of typing it Refs #9860 (item 1). The LOOPOVER_LOGIN line in `loopover-mcp --help` named its commands as hand-typed prose, and it had already drifted: it omitted contributor-profile, explain-review-risk and watch, all three of which accept --login and resolve it through the same resolveLogin fallback. A user reading --help was told those commands would not pick up their configured login. They do. Derived from CLI_COMMAND_SPEC now -- every command declaring --login in its usage resolves it that way, and resolveLogin is the single helper that does it, so the usage table is the fact to read rather than a second list to keep in step. Also drops a no-op .replace() on the usage line beside it. Items 1 and 2 of #9860 are otherwise already closed by the MCP contract epic: STDIO_TOOL_DESCRIPTORS derives from STDIO_TOOL_NAMES, printHelp's command list derives from CLI_COMMAND_SPEC, the README block is generated by mcp:tool-reference, and the stdio bin now declares no zod shapes of its own -- all 119 z. uses are z.infer over contract-imported schemas. This line was the one hand-maintained list left in that surface.
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
FOCUS_MANIFEST_TOP_LEVEL_FIELDS' own header says "Every top-level key parseFocusManifest below actually reads." That is a statement about code twenty lines further down, and nothing enforced it -- the same shape as the turbo.json inputs comment #9860 calls out as the tell: a snapshot written as a guarantee. The list is the single source of truth for unknown-field detection (#9065), so drift is silent in both directions and asymmetrically bad: DECLARED BUT NEVER READ -- the parser accepts the field, warns about nothing, and ignores it. An operator sets it in .loopover.yml, sees no error, and the setting does nothing. This is the worse direction: it looks configured. READ BUT NOT DECLARED -- the parser consumes the field while warning "unknown top-level field" about it, and config-lint's recognizedFieldsFor leaves it out of the recognized count. A working field reported as a mistake. Adding a field means editing two places twenty lines apart, and #9813 and #9821 each shipped a bug from missing one touchpoint of a multi-touchpoint field. Computed, not restated: the declared names come out of the array literal and the used names out of the function body, so this file holds no second list and cannot itself go stale. It is a regex over source rather than the TS AST, which is a real tradeoff. The access pattern is uniform and mechanical, and every extractor THROWS when the shape it depends on is missing -- a renamed constant, a moved signature, or a refactor to destructuring all fail loudly instead of matching nothing and reporting "no drift". A guard that passes vacuously is worse than none, because its presence reads as coverage. Holds today at 28 declared and 28 read, empty both ways, so this lands green and guards forward. Mutation-tested: declaring an unread field, reading an undeclared one, and removing a still-read declaration each fail with the field named. Closes #9979
loopover-orb Bot
pushed a commit
that referenced
this pull request
Jul 31, 2026
…ds (#9980) FOCUS_MANIFEST_TOP_LEVEL_FIELDS' own header says "Every top-level key parseFocusManifest below actually reads." That is a statement about code twenty lines further down, and nothing enforced it -- the same shape as the turbo.json inputs comment #9860 calls out as the tell: a snapshot written as a guarantee. The list is the single source of truth for unknown-field detection (#9065), so drift is silent in both directions and asymmetrically bad: DECLARED BUT NEVER READ -- the parser accepts the field, warns about nothing, and ignores it. An operator sets it in .loopover.yml, sees no error, and the setting does nothing. This is the worse direction: it looks configured. READ BUT NOT DECLARED -- the parser consumes the field while warning "unknown top-level field" about it, and config-lint's recognizedFieldsFor leaves it out of the recognized count. A working field reported as a mistake. Adding a field means editing two places twenty lines apart, and #9813 and #9821 each shipped a bug from missing one touchpoint of a multi-touchpoint field. Computed, not restated: the declared names come out of the array literal and the used names out of the function body, so this file holds no second list and cannot itself go stale. It is a regex over source rather than the TS AST, which is a real tradeoff. The access pattern is uniform and mechanical, and every extractor THROWS when the shape it depends on is missing -- a renamed constant, a moved signature, or a refactor to destructuring all fail loudly instead of matching nothing and reporting "no drift". A guard that passes vacuously is worse than none, because its presence reads as coverage. Holds today at 28 declared and 28 read, empty both ways, so this lands green and guards forward. Mutation-tested: declaring an unread field, reading an undeclared one, and removing a still-read declaration each fail with the field named. Closes #9979
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The asymmetry this fixes
Effort already existed —
CLAUDE_AI_EFFORT→claude --effort,CODEX_AI_EFFORT→model_reasoning_effort, levelslow|medium|high|xhigh|max. But only as a global env var, whileproviderandmodelwere already per-repo manifest fields. Self-consistency runs had exactly the same gap.So the two knobs that most directly control review depth were the two a repo couldn't set. (I'd earlier told the maintainer no effort knob existed at all — that was wrong; it exists, it just wasn't reachable from config.)
What's added
Precedence resolves per field independently: escalation > per-repo > global. An escalation setting only
effortstill inherits the repo's model and the global provider — the common request is "same model, think harder", not "swap the model out".Why escalation, not just per-repo knobs
A
hardGuardrailGlobshit currently changes exactly one thing: it suppresses auto-merge and queues a human. It buys no extra analysis — a PR touching.github/workflows/**gets the same single-pass, same-model, same-effort review as a README typo. On the production ORB that was 74 distinct PRs held in 14 days, on repos whose guardrail lists had already been narrowed twice. Automation replaced by a maintainer queue, with scrutiny unchanged.Now a guarded path can buy more scrutiny instead. Manual review remains the fallback: if the escalated review isn't clean, the PR still holds.
Safety
nullthroughout ⇒ byte-identical to today for every existing repo and self-hoster.selfConsistencyRuns: 0is a real value, not "unset" — a truthiness check there would silently re-enable multi-run reviews on a repo that deliberately turned them off. Pinned by a test.isGuardrailHitis the shared matcher (src/signals/change-guardrail.ts), never a second copy free to drift.ai_review_guardrail_escalated, anddescribeReviewEscalationrenders the one-line "why" for the panel and decision record — an auto-merge on a guarded path should be able to say what extra scrutiny it got.Tests
9 resolver cases (escalation applies / is inert off-path / per-field precedence / per-repo parity / runs=0 / all-absent / the three description shapes) plus the manifest parse, round-trip and resolve coverage. 1118 pass across the touched files.
tsc,dead-source-files,docs:drift,manifest:drift,engine-parity:drift,ui:openapi:check,import-specifiersall green — including the drift guards that forced the.loopover.yml.exampledocumentation and the alias rows for the nested keys.Follow-up
effort/model/providercurrently reach the provider invocation throughoptions.claudeEffort, which already outranks the env var; the remaining wiring for BYOK provider swaps on escalation is small and can follow. The ORB's own configs stay untouched here — flippingguardrailEscalationon for the three repos is a server-side config change once this ships in an image.