gh-write-verification: the landing check's result is read, not just its command - #494
Open
EdbertChan wants to merge 4 commits into
Conversation
… the landing-proof detector require the check's OK result, not merely its invocation. Review claim: A landing check that printed FAIL or UNCHECKED does not satisfy the landing-proof hook. Review lane: behavior Safety invariant: No merge shape that is silent today becomes noisy: a merge followed by a landing check printing OK stays silent, and only FAIL, UNCHECKED, or a missing result newly flags. Effectiveness measurement: Three fixtures: a landing check printing OK stays silent, one printing FAIL flags, one printing UNCHECKED flags. The current detector passes only the first, so the suite fails before the change and passes after. Slice rationale: One detector and its tests in one hook directory; shares no file with the sweep-skill chain, so it runs independently rather than waiting on it. Architectural effect: The detector gains access to each command's paired tool result, so the hook judges outcomes rather than invocations. Goal: Change engine/hooks/gh-write-verification/detect.py so _proves_landing requires the paired tool result to report OK, and add tests for the FAIL and UNCHECKED cases. Motivation: The hook's own purpose is to stop a merge claim that outran its landing check. Because it reads only the command text, a check that ran and reported failure clears it exactly as well as one that reported success, so the failure mode it exists to catch is the one it cannot see. Alternative considerations: Parsing the exit code alone was rejected because the script prints its verdict on stdout and an exit code is not available for every recorded command shape. Leaving the detector as-is and adding a prose rule was rejected: the detector is the mechanism, so prose would be a second copy of the same rule with no enforcement. Implementation details: Extend bash_commands_this_turn (detect.py lines 369-391) to carry each command's paired tool result alongside its text rather than discarding non-assistant records. Change _proves_landing (detect.py lines 291-302) to require an OK marker in that result, treating a missing result as not proving landing. Update merges_missing_landing_proof (detect.py line 306) to pass the results through. Add tests asserting a FAIL result and an UNCHECKED result both still flag, and that an OK result stays silent. Non-goals: Does not change the verifier script's exit contract, does not change which commands count as merges, does not widen the hook to other tools, and does not touch any skill prose. Layer: domain Feature state: active Files: engine/hooks/gh-write-verification/detect.py, engine/hooks/gh-write-verification/tests/test_hooks.py Change types: - engine/hooks/gh-write-verification/detect.py: modify - engine/hooks/gh-write-verification/tests/test_hooks.py: modify Acceptance criteria: - `python3 -m unittest discover -s engine/hooks/gh-write-verification/tests -v` exits 0. - A landing check whose result printed FAIL still flags the merge. - A landing check whose result printed UNCHECKED still flags the merge. - A landing check whose result printed OK stays silent. Solution: Make the landing-proof detector require the check's OK result, not merely its invocation. Review claim: A landing check that printed FAIL or UNCHECKED does not satisfy the landing-proof hook. Review lane: behavior Safety invariant: No merge shape that is silent today becomes noisy: a merge followed by a landing check printing OK stays silent, and only FAIL, UNCHECKED, or a missing result newly flags. Effectiveness measurement: Three fixtures: a landing check printing OK stays silent, one printing FAIL flags, one printing UNCHECKED flags. The current detector passes only the first, so the suite fails before the change and passes after. Slice rationale: One detector and its tests in one hook directory; shares no file with the sweep-skill chain, so it runs independently rather than waiting on it. Architectural effect: The detector gains access to each command's paired tool result, so the hook judges outcomes rather than invocations. Goal: Change engine/hooks/gh-write-verification/detect.py so _proves_landing requires the paired tool result to report OK, and add tests for the FAIL and UNCHECKED cases. Motivation: The hook's own purpose is to stop a merge claim that outran its landing check. Because it reads only the command text, a check that ran and reported failure clears it exactly as well as one that reported success, so the failure mode it exists to catch is the one it cannot see. Alternative considerations: Parsing the exit code alone was rejected because the script prints its verdict on stdout and an exit code is not available for every recorded command shape. Leaving the detector as-is and adding a prose rule was rejected: the detector is the mechanism, so prose would be a second copy of the same rule with no enforcement. Implementation details: Extend bash_commands_this_turn (detect.py lines 369-391) to carry each command's paired tool result alongside its text rather than discarding non-assistant records. Change _proves_landing (detect.py lines 291-302) to require an OK marker in that result, treating a missing result as not proving landing. Update merges_missing_landing_proof (detect.py line 306) to pass the results through. Add tests asserting a FAIL result and an UNCHECKED result both still flag, and that an OK result stays silent. Non-goals: Does not change the verifier script's exit contract, does not change which commands count as merges, does not widen the hook to other tools, and does not touch any skill prose. Layer: domain Feature state: active Files: engine/hooks/gh-write-verification/detect.py, engine/hooks/gh-write-verification/tests/test_hooks.py Change types: - engine/hooks/gh-write-verification/detect.py: modify - engine/hooks/gh-write-verification/tests/test_hooks.py: modify Acceptance criteria: - `python3 -m unittest discover -s engine/hooks/gh-write-verification/tests -v` exits 0. - A landing check whose result printed FAIL still flags the merge. - A landing check whose result printed UNCHECKED still flags the merge. - A landing check whose result printed OK stays silent. Invoker-Finalize-Id: 275f6067-457a-4dde-8dbf-8542a8fff971
… deterministic proof for the landing detector's result reading. Review claim: The hook's tests assert a failed landing check still flags the merge. Review lane: proof Safety invariant: Proof-only; adds no product behavior. Effectiveness measurement: The suite fails if a FAIL or UNCHECKED landing result clears the hook. Slice rationale: One proof slice for this step. Architectural effect: None; verification only. Goal: Prove the landing detector's result reading deterministically. Motivation: The defect is a detector that reads the wrong input, so the proof drives it with results rather than commands. Alternative considerations: Manual verification was rejected as non-deterministic. Implementation details: Execute the command below as the terminal proof. Non-goals: No product edits here; proof only. Layer: app_regression Feature state: active Exit code: 0 Invoker-Finalize-Id: 1e117326-8ae6-4b5c-9862-bf9319098010
… that no ephemeral inter-task handoff files remain. Review claim: The workflow leaves no scratch handoff artifacts behind. Review lane: cleanup Safety invariant: Read-only; never deletes files, alters the index, or commits caller work. Effectiveness measurement: The gate fails when a plans/invoker-handoff.* or lens-*.json file is still present after the leaf tasks complete. Slice rationale: One terminal hygiene gate for the workflow. Architectural effect: None; check only. Goal: Confirm no ephemeral handoff files survive the run. Motivation: Inter-task scratch files leak into diffs and read as part of the work. Alternative considerations: Deleting them automatically was rejected; the gate reports, it does not mutate. Implementation details: Run scripts/scrub-handoff-artifacts.sh without --apply. Non-goals: No deletion, no index changes, no commits. Layer: app_regression Feature state: active Exit code: 0 Invoker-Finalize-Id: 657b1e05-5809-43b8-a2ea-e4ef235c90a4
…aa2588d36-c24f6041 — Terminal check that no ephemeral inter-task handoff files remain. Review claim: The workflow leaves no scratch handoff artifacts behind. Review lane: cleanup Safety invariant: Read-only; never deletes files, alters the index, or commits caller work. Effectiveness measurement: The gate fails when a plans/invoker-handoff.* or lens-*.json file is still present after the leaf tasks complete. Slice rationale: One terminal hygiene gate for the workflow. Architectural effect: None; check only. Goal: Confirm no ephemeral handoff files survive the run. Motivation: Inter-task scratch files leak into diffs and read as part of the work. Alternative considerations: Deleting them automatically was rejected; the gate reports, it does not mutate. Implementation details: Run scripts/scrub-handoff-artifacts.sh without --apply. Non-goals: No deletion, no index changes, no commits. Layer: app_regression Feature state: active
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_909fe4b7-e4be-4459-957b-ec155048c9a7) |
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Owner
Author
|
Mergify repair stopped: missing required check validate |
Owner
Author
|
Invoker repair dispatch was not acknowledged. The request was recorded as pending, then closed without consuming the code-repair retry budget; automatic retry remains enabled for current head 533f8ff. |
Owner
Author
|
Mergify repair stopped: required check failed: test. The retry cap was reached for current head 533f8ff. |
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
A merge safeguard checks that a merged change reached the main branch before the workflow falls quiet.
It previously treated running the landing check as proof, even when the check reported failure or could not check.
The safeguard read the command that launched the check but not the result it returned.
It now requires the paired result to say OK, while failed, uncertain, or missing results continue to flag the merge.
Review Claim
A failed or uncertain landing check still flags the merge, while a successful landing check keeps the workflow quiet.
Review Lane
behavior
Review Unit
engine-runtime
Safety Invariant
No merge shape that is silent today becomes noisy: a merge followed by a landing check printing OK stays silent, and only FAIL, UNCHECKED, or a missing result newly flags.
Slice Rationale
This slice changes one landing detector and its tests in one hook directory. It shares no file with the rest of the sweep stack, so it runs independently.
Non-goals
Architecture
Before
graph TD A["landing command"] --> B["detector reads command text"] B --> C["merge treated as proven"]After
graph TD A["landing command"] --> B["detector pairs command with tool result"] B --> C["detector requires OK result"] C --> D["merge is proven or remains flagged"]Test Plan
Test Plan
python3 -m unittest discover -s engine/hooks/gh-write-verification/tests -v— 52 tests passed.python3 engine/skills/make-pr/scripts/preflight.py --base origin/main— preflight passed; hook coverage passed.python3 scripts/check_hook_test_coverage.py engine/hooks/gh-write-verification— 1 hook checked.bash scripts/scrub-handoff-artifacts.sh— no handoff artifacts remain.Revert Plan
Revert Plan
git revert <sha>.Note
Medium Risk
Tightens post-merge stop behavior so failed or absent verifier output blocks the turn; successful OK paths stay silent per the stated invariant.
Overview
The unverified landing stop-hook no longer treats “ran a landing check” as proof—it now pairs each Bash command with its transcript tool result and only clears a merge when that output contains a line matching
OK:.bash_commands_this_turnwalks the JSONL transcript, indexestool_resultblocks bytool_use_id, and returns(command, result)tuples._proves_landingstill recognizes the same proof commands (verify_pr_landed_on_trunk,git merge-base --is-ancestor, etc.) but requires a non-empty result with an OK verdict; FAIL, UNCHECKED, or a missing result leaves the merge flagged. Tests and synthetic transcripts were extended for failed/unchecked/missing-result cases.Reviewed by Cursor Bugbot for commit 533f8ff. Bugbot is set up for automated code reviews on this repo. Configure here.