feat(review): submit REQUEST_CHANGES on substantive FAIL so reviewDecision is authoritative (INV-52, closes #193)#197
Merged
Conversation
12 tasks
![kane-review-agent[bot]](https://avatars.githubusercontent.com/in/3020954?s=60&v=4){kind=small}
…ent posts verdicts only (INV-52, closes #193) The review WRAPPER now owns the GitHub-native PR review/merge action on both sides: it submits `gh pr review --approve` (+ merge) on PASS and, NEW, `gh pr review --request-changes` on a substantive FAIL so the PR's `reviewDecision` becomes CHANGES_REQUESTED — authoritative for humans, branch protection, the dispatcher, and the dev-resume agent (root cause behind #188). The review AGENT posts a verdict comment only and must never approve/merge itself (the PR #191 / issue #190 incident, where a self-approve raced the INV-44 mergeable gate + no-auto-close skip-merge). - NEW lib-review-request-changes.sh::submit_request_changes — best-effort (always returns 0; a 403/transient gh failure is logged, never aborts the FAIL route under set -e). Wired onto EXACTLY the two substantive FAIL routes (agent-posted findings + CONFLICTING mergeable block); NOT on the non-substantive routes (mergeable-UNKNOWN re-queue, agent crash w/ no verdict). - A subsequent PASS re-approves the new HEAD, superseding the standing CHANGES_REQUESTED (no permanently-stuck blocking state). - Agent-side framing fixed: SKILL.md + decision-gate.md re-scope "approve+merge" to the wrapper and explicitly forbid the agent running gh pr review/merge. - Pipeline docs: new INV-52; review-agent-flow.md FAIL/PASS paths + state-machine.md transition rows updated in the same PR. - ci.yml: lib-review-request-changes.sh added to the shellcheck list. - Tests: tests/unit/test-autonomous-review-request-changes.sh (24 assertions, executable helper harness + source-of-truth greps + agent-doc greps).
kane-coding-agent
Bot
force-pushed
the
fix/request-changes-on-fail-193
branch
from
ba1c439 to
fc2dabd
Compare
…oo (INV-52, #193) Addresses the #197 codex review finding: the INV-52 acceptance criterion is route-agnostic ("a blocking FAIL verdict must result in reviewDecision == CHANGES_REQUESTED"), but the original diff wired submit_request_changes only into the agent-findings FAIL branch and the CONFLICTING mergeable block. The E2E hard-gate failure (INV-46, E2E_GATE == fail, lane rc≠0, produced before the review fan-out) is ALSO a dev-actionable `failed-substantive` blocking FAIL and must assert CHANGES_REQUESTED on the PR. - autonomous-review.sh: add submit_request_changes to the E2E `fail` branch (best-effort `|| log`, same shape as the other two substantive sites). The E2E `block-nonsubstantive` (evidence-missing) re-queue route deliberately does NOT request changes (transient, not a code defect). - test: TC-RC-SRC-02/04 now require EXACTLY 3 substantive call sites; new TC-RC-SRC-04b (E2E-fail requests changes) + TC-RC-SRC-04c (E2E-evidence-missing does NOT). 26 assertions, all green. - docs: INV-52 substantive-route table + sub-rule 2 + producer + cross-ref to INV-46; review-agent-flow.md E2E gate block + FAIL-path prose; state-machine.md E2E-fail transition row; design + test-case docs. The substantive set is now explicitly defined by "the verdict is a real dev-actionable blocking FAIL", NOT by which gate produced it.
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.
Summary
gh pr review --approve(+ merge) on PASS, and NEWgh pr review --request-changeson a substantive FAIL — so the PR'sreviewDecisionbecomesCHANGES_REQUESTED, authoritative for humans browsing the PR, branch protection, the dispatcher, and the dev-resume agent. This closes the false-green-PR root cause behind fix(autonomous-dev): resume skips post-approval "Review findings:" — approved-PR short-circuit + brittle marker match #188.gh pr review/gh pr mergeitself. This fixes the PR feat(agent): agy backend honors --model via agy-models validation (INV-50, closes #190) #191 / issue feat(autonomous): agy backend honors --model (Antigravity 2.0 CLI no longer warn-and-ignore) #190 incident, where the review agent self-approved+merged ~18 min before the wrapper's gates ran, bypassing the INV-44 mergeable hard gate (PR wasUNKNOWN-mergeable) and theno-auto-closeskip-merge.Both halves land together because they share
decision-gate.md+ the approve/merge flow.What changed
skills/autonomous-dispatcher/scripts/lib-review-request-changes.sh—submit_request_changes <pr> <body>, a best-effort helper (always returns0; a 403/transientghfailure is logged and swallowed so it can never abort the FAIL route underset -eand strand the issue inreviewing).autonomous-review.sh— wires the helper onto exactly the two substantive FAIL routes: thefailed-substantivebranch (agent postedReview findings:) and theblock-substantivemerge-conflict (CONFLICTING) route. It is deliberately NOT wired onto the non-substantive routes (mergeable-UNKNOWNre-queue, agent crash with no verdict) — a transient hold is not a dev-actionable blocking finding, and a standingCHANGES_REQUESTEDthere would falsely accuse the dev. The existing--approveon PASS is retained; PASS and REQUEST_CHANGES are mutually exclusive branches.CHANGES_REQUESTED(no permanently-stuck blocking state).SKILL.md+references/decision-gate.mdre-scope "approve + merge" to the wrapper and explicitly forbid the agent runninggh pr review/gh pr merge(the rejected mechanical tool-deny backstop is recorded in INV-52 — the review agent runs headless under--dangerously-skip-permissions, so prompt+wrapper enforcement is the robust path).INV-52ininvariants.md;review-agent-flow.md(FAIL/PASS paths + sequence diagram) andstate-machine.md(transition rows +reviewDecisionnote) updated in the same PR..github/workflows/ci.yml— the new lib added to the shellcheck list.Invariant numbering
INV-51 was taken by #194 (codex review resume loop, on
main); highest committed is INV-51 → this PR is INV-52, per the owner's numbering heads-up on the issue.Post-install / upgrade
This PR adds
scripts/lib-review-request-changes.sh. After merge +npx skills update -g, re-runinstall-project-hooks.shon every onboarded project (see CLAUDE.local.md → Post-merge Step 2) or their review wrappers will crash on the missingsource(the per-projectscripts/holds per-file symlinks into the user-scope skill; a newly-added file has no symlink until the installer re-runs).Test Plan
docs/designs/request-changes-on-fail.md)docs/test-cases/request-changes-on-fail.md)tests/unit/test-autonomous-review-request-changes.sh— 24 assertions: executablesubmit_request_changesharness (requests-changes-not-approve; non-zerogh→ returns 0 + warns; continues past helper underset -e), source-of-truth greps (helper sourced + called on EXACTLY the 2 substantive routes, every call best-effort, PASS still--approve, mutual exclusion,bash -n), and agent-doc greps (no "PASS (approve + merge)"; agent prohibited fromgh pr review/merge; INV-52 exists + referenced). Fails before the fix, passes after.tests/unit/test-*.shsuite green (97 suites); regression pins (auto-merge-failure,mergeable-gate,multi-agent,verdict-trailer,prompt) hold.shellcheck -S errorclean on the new lib + wrapper.Acceptance Criteria
reviewDecision == CHANGES_REQUESTEDon the PR.--request-changescall is best-effort (non-fatal underset -e).gh pr review --approve/gh pr merge.review-agent-flow.md,decision-gate.md,state-machine.md,invariants.mdupdated in the same PR.Closes #193