You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
feat(gate): choose provider, model, effort and self-consistency runs — per repo, and escalated on guarded paths (#9821)
* feat(gate): choose provider, model, effort and self-consistency runs — 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.
* fix(gate): actually apply the resolved effort/model/provider at the invocation
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.
* test(gate): cover every new manifest branch, and fix the serialize guard 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)
* fix(gate): consume the resolved model/effort in ai-review.ts itself, 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.
* test(engine): cover the new manifest branches in the ENGINE's own suite
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.
0 commit comments