feat(gate): escalate verdict to CONDITIONAL on breaking API change#17
Conversation
Introduce a `[gate]` section in prview.toml carrying the `breaking_escalation` flag, surfaced on Config as `breaking_escalation: bool`. Absent section/key defaults to true so breaking-change verdict escalation is on out-of-the-box; setting it false keeps breaking findings as an informational caveat only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the critic-1 truth inversion where a real breaking-API-change detection never escalated the merge verdict while a weaker semantic signal did. `apply_breaking_escalation` ratchets the merge axis Approve -> ReviewRequired (PASS -> CONDITIONAL) when genuine breaking findings are present, never forces BLOCK, and never downgrades an axis already raised for another reason. Wired identically into build_dashboard_context and generate_merge_gate so the verdict is identical across the console, report.json.gate and MERGE_GATE.json.decision surfaces, each carrying the same explicit 'breaking API change detected: <n> finding(s)' reason caveat. Gated by the [gate] breaking_escalation knob; when off, breaking findings stay visible as an informational caveat only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drive the real binary against a fixture whose feature branch removes a public Rust function and assert `gate --strict` exits 2, proving the breaking-change escalation holds at the process boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a breaking-change escalation feature to the merge-gate verdict system. A new [gate] configuration section with a breaking_escalation option (defaulting to true) has been added to prview.toml and documented. When enabled, genuine breaking API changes (such as removed public symbols, changed public signatures, or new required environment variables) escalate the merge verdict to at least CONDITIONAL. This escalation logic is consistently applied across both the dashboard context and the merge gate generation, and is backed by comprehensive unit and integration tests. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4769582175
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if *worst_merge == MergeRecommendation::Approve { | ||
| *worst_merge = MergeRecommendation::ReviewRequired; | ||
| } |
There was a problem hiding this comment.
This ratchets only the merge axis; for the documented prview --ci automation path, the process exit still comes from output::compute_exit_code, which returns non-zero for Block or !quality_pass only. A breaking-only diff leaves quality_pass = true, so --ci runs will now emit/read a CONDITIONAL verdict for the breaking API change but still exit 0; either make CI exit handling reject conditional verdicts or make the new escalation apply only to prview gate --strict.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional — this matches the pre-existing --ci contract rather than introducing a new inconsistency. compute_exit_code (src/output/mod.rs) has always returned non-zero only for Block || !quality_pass; a CONDITIONAL verdict from any cause (semgrep skip, policy warnings, …) already exits 0 under --ci, so breaking-driven CONDITIONAL behaves identically to every other CONDITIONAL there. The enforcement lane is the gate contract: prview gate exits 1 on CONDITIONAL and --strict rejects it with exit 2 — covered by the new e2e test (strict + breaking → exit 2). Making --ci reject CONDITIONAL would be a behavioral break for all existing --ci users and all conditional causes, well beyond this PR's scope. Added a docs note clarifying the two exit-code lanes so this is spelled out explicitly.
There was a problem hiding this comment.
Docs note added in 41e5fb6 — docs/gate-playbook.md now spells out the two exit-code lanes explicitly (--ci legacy advisory vs prview gate / --strict enforcement).
Document the two distinct exit contracts: `prview --ci` is the legacy advisory review exit (CONDITIONAL -> 0, including breaking-only CONDITIONAL, as for any other CONDITIONAL cause), while `prview gate` is the contractual enforcement path (CONDITIONAL -> 1, --strict -> 2). To fail CI on a breaking change, use `prview gate --strict`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Closes the last confirmed finding from the 2026-07-07 deep-dig audit (critic-1, P2): genuine breaking-API-change detections never escalated the merge verdict, while weaker signals did — a truth inversion in the gate contract. Operator decision: escalate with an opt-out knob.
9f2993afeat(config):[gate] breaking_escalationknob inprview.toml, default ON (absent file/section → ON)c73cc9ffeat(gate): sharedapply_breaking_escalationratchet inverdict.rs— breaking findings raise PASS→CONDITIONAL / Approve→ReviewRequired, never BLOCK, never lower an already-higher verdict; wired identically intomerge_gate.rs(MERGE_GATE.json) andcontext.rs(report.json + console) so all three truth surfaces stay in sync64bb525test(gate): e2e — strict mode + breaking change → exit 2 (exit-code contract structurally unchanged)4769582docs(gate): knob + semantics in configuration.md and gate-playbook.mdKnob OFF keeps breaking findings visible as an informational caveat with zero verdict impact.
Test plan
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test→ 1290 passed / 0 failed (no flake)pub fn→ CONDITIONAL identical on console,MERGE_GATE.json.decision.verdictandreport.json.gate.verdict, each carryingbreaking API change detected: 1 finding; same repo with knob OFF → zero escalation caveats, informational finding still present🤖 Generated with Claude Code