admin-bypass-sweep (3) a truncated diff read is unchecked, not reviewed - #498
Open
EdbertChan wants to merge 4 commits into
Open
Conversation
… a full diff read per PR before an admin merge, and mark a narrowed read unchecked. Review claim: A PR whose diff was read through a truncating filter is unchecked, never reviewed. Review lane: behavior Safety invariant: A truncated or filtered diff read marks that PR unchecked, never reviewed; no PR that is reviewable today becomes unmergeable, because the gate reports rather than blocks the merge command. Effectiveness measurement: A fixture review command piped through `head` marks its PR unchecked, and the same command without the pipe marks it reviewed. Both asserted in the skill's test directory. Slice rationale: One step of the sweep skill's review procedure; stacked behind the tally step because both edit the same SKILL.md. Architectural effect: Step 4 gains an explicit read-completeness precondition expressed as the same three-outcome contract the rest of the stack uses. Goal: Change Step 4 of product/skills/admin-bypass-sweep/SKILL.md so each PR's diff is written to a file, fully read, and asserted line-complete before `gh pr merge --admin`, with a narrowed read reported as unchecked. Motivation: A sweep reported 'Reviewed all 19' after two commands that between them truncated one PR entirely and filtered every test file out of five others. The largest diff in the sweep, touching six hook detectors, was cleared for merge on about 21% of its non-test changes. Alternative considerations: Telling the operator not to pipe through head was rejected as unenforceable prose. Blocking the merge outright on a narrowed read was rejected for this slice: the skill's own consent model puts the merge decision with the human, so the gate reports unchecked and the operator decides. Implementation details: In Step 4, replace the ad hoc `gh pr diff` skim with: write each PR's diff to a file, record `wc -l`, read the whole file, and assert lines_read equals the recorded total before merging that PR. Any read through head, tail, grep, awk, or sed marks that PR unchecked. Use the same three outcomes as the rest of the stack: reviewed, flagged, unchecked. Add fixtures under the skill's tests directory for a full read and a truncated read. Non-goals: Does not change what counts as a review finding, does not add an automated code review, does not alter the consent requirements in the STOP section, and does not touch any other skill. Layer: domain Feature state: active Files: product/skills/admin-bypass-sweep/SKILL.md, product/skills/admin-bypass-sweep/tests/ Change types: - product/skills/admin-bypass-sweep/SKILL.md: docs-only - product/skills/admin-bypass-sweep/tests/fires_truncated_diff_read.md: create - product/skills/admin-bypass-sweep/tests/stays_silent_full_diff_read.md: create Acceptance criteria: - `python3 -m unittest discover -s product/skills/admin-bypass-sweep/tests -v` exits 0. - Step 4 requires a recorded line count and a full read before `gh pr merge --admin`. - A read through head, tail, grep, awk, or sed is documented as unchecked. Solution: Require a full diff read per PR before an admin merge, and mark a narrowed read unchecked. Review claim: A PR whose diff was read through a truncating filter is unchecked, never reviewed. Review lane: behavior Safety invariant: A truncated or filtered diff read marks that PR unchecked, never reviewed; no PR that is reviewable today becomes unmergeable, because the gate reports rather than blocks the merge command. Effectiveness measurement: A fixture review command piped through `head` marks its PR unchecked, and the same command without the pipe marks it reviewed. Both asserted in the skill's test directory. Slice rationale: One step of the sweep skill's review procedure; stacked behind the tally step because both edit the same SKILL.md. Architectural effect: Step 4 gains an explicit read-completeness precondition expressed as the same three-outcome contract the rest of the stack uses. Goal: Change Step 4 of product/skills/admin-bypass-sweep/SKILL.md so each PR's diff is written to a file, fully read, and asserted line-complete before `gh pr merge --admin`, with a narrowed read reported as unchecked. Motivation: A sweep reported 'Reviewed all 19' after two commands that between them truncated one PR entirely and filtered every test file out of five others. The largest diff in the sweep, touching six hook detectors, was cleared for merge on about 21% of its non-test changes. Alternative considerations: Telling the operator not to pipe through head was rejected as unenforceable prose. Blocking the merge outright on a narrowed read was rejected for this slice: the skill's own consent model puts the merge decision with the human, so the gate reports unchecked and the operator decides. Implementation details: In Step 4, replace the ad hoc `gh pr diff` skim with: write each PR's diff to a file, record `wc -l`, read the whole file, and assert lines_read equals the recorded total before merging that PR. Any read through head, tail, grep, awk, or sed marks that PR unchecked. Use the same three outcomes as the rest of the stack: reviewed, flagged, unchecked. Add fixtures under the skill's tests directory for a full read and a truncated read. Non-goals: Does not change what counts as a review finding, does not add an automated code review, does not alter the consent requirements in the STOP section, and does not touch any other skill. Layer: domain Feature state: active Files: product/skills/admin-bypass-sweep/SKILL.md, product/skills/admin-bypass-sweep/tests/ Change types: - product/skills/admin-bypass-sweep/SKILL.md: docs-only - product/skills/admin-bypass-sweep/tests/fires_truncated_diff_read.md: create - product/skills/admin-bypass-sweep/tests/stays_silent_full_diff_read.md: create Acceptance criteria: - `python3 -m unittest discover -s product/skills/admin-bypass-sweep/tests -v` exits 0. - Step 4 requires a recorded line count and a full read before `gh pr merge --admin`. - A read through head, tail, grep, awk, or sed is documented as unchecked. Invoker-Finalize-Id: 86232251-f80f-4a4b-a379-36417182163d
…terministic proof for the full-diff-read gate. Review claim: The skill's fixtures assert a truncated diff read is reported unchecked. Review lane: proof Safety invariant: Proof-only; adds no product behavior. Effectiveness measurement: The suite fails if the truncated-read fixture is reported as reviewed. Slice rationale: One proof slice for this step. Architectural effect: None; verification only. Goal: Prove the full-diff-read gate deterministically. Motivation: The defect is an over-claimed review, so the proof asserts the narrowed case is named unchecked. 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: 4cde49f2-d3fe-41d7-ad5f-4c24fb36f491
… 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: 401825e5-c8a7-488b-9472-5b8212a9ca68
…aff8b535d-05d30b31 — 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_7f851215-dd01-455a-9706-eb1c7abd8154) |
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
Step 4 now requires each admin-merge diff to be saved, counted, fully read, and checked for line completeness.
Narrowed or filtered reads are reported as
unchecked, preventing unsupported review claims.Review Claim
A truncated or filtered diff is
unchecked, neverreviewed; a complete diff may bereviewedorflagged.Review Lane
policy
Review Unit
validation-policy
Safety Invariant
The gate reports review state without blocking the human’s existing decision to proceed with an admin merge.
Slice Rationale
This isolates the Step 4 read-completeness contract and focused fixtures from other sweep changes.
Non-goals
Architecture
Before
graph TD A["PR diff skim"] --> B["reviewed or flagged"] B --> C["gh pr merge --admin"]After
graph TD A["PR diff saved and line-counted"] --> B["full read asserted"] B --> C["reviewed or flagged"] A --> D["narrowed or filtered read"] D --> E["unchecked"] C --> F["gh pr merge --admin"] E --> FTest Plan
Test Plan
python3 -m unittest discover -s product/skills/admin-bypass-sweep/tests -v— 4 tests passed (OK).bash scripts/scrub-handoff-artifacts.sh—scrub-handoff-artifacts-ok.Revert Plan
Revert Plan
git revert <sha>.Note
Low Risk
Documentation and policy tests for a manual skill; no runtime merge automation or auth changes.
Overview
Step 4 of
admin-bypass-sweepno longer allows a casual skim ofgh pr diff. Operators must dump each PR diff to a temp file, recordwc -l, read the full file (e.g. vianl -ba), and assertlines_readequalsdiff_linesbefore anygh pr merge --admin.The skill now classifies each PR as
reviewed,flagged, orunchecked. Reads throughhead,tail,grep,awk, orsedcount as narrowed and must beunchecked—never reported as reviewed; mergeduncheckedPRs stayuncheckedin the final report. The gate documents state only; it does not block the human from proceeding with admin merge.Adds fixture scenarios (complete vs truncated read) and
test_diff_read_gate.pyunittest checks that Step 4 and the fixtures encode this contract.Reviewed by Cursor Bugbot for commit 9eecf4a. Bugbot is set up for automated code reviews on this repo. Configure here.