Repository navigation
feat(ship-check): pr-monitor sweeps the finding's class before each push - #27
Conversation
Monitoring loops converged one instance per bot cycle: fix the flagged site, push, wait for the re-review, get the sibling flagged — 121 comments on one PR. The skill handled findings as instances and never asked for the pattern. - Step 3 bot-thread procedure gains step 4: generalize each valid finding to its class, grep the changed files (doc claims: every sibling surface), fix all instances in the same commit, and state the sibling count in the reply - "After handling all threads" now requires one full-diff re-scan against every class flagged on the PR so far before pushing — each push buys a complete re-review, so a missed sibling is next cycle's comments Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
| 4. **Sweep the class, not just the instance.** A bot flags instances; the next pass | ||
| flags the sibling you left, and the point-fix loop is how a PR ends up with 100+ | ||
| comments. Generalize each valid finding to the pattern it represents — the same | ||
| claim on another doc surface, the same code shape in another function, the same | ||
| missing guard on another entry point — then grep the PR's changed files (and for | ||
| doc claims, every sibling surface) for the other instances and fix them all in | ||
| the same commit. State the count in the reply: *"Fixed here and at N sibling | ||
| sites."* A class-wide fix converges the review; a point fix buys another full | ||
| bot cycle for each sibling. |
There was a problem hiding this comment.
Add a boundary clause to the class-sweep rule
Medium severity · conventions · high confidence
AGENTS.md requires every procedural skill rule to carry action, condition, and boundary — the line between apply and skip. The new class-sweep rule states the action (grep for sibling instances and fix them all in the same commit) and the condition (each valid finding), but no boundary: nothing tells the agent when NOT to generalize a finding to a site that shares a code shape without sharing the invariant the finding depends on. Over-swept edits ship in the same commit as the valid fix and become next cycle's bot comments.
Failure scenario: A bot flags a missing null guard on getUser(id); the agent generalizes to 'the same missing guard on another entry point' and adds a guard to getOptionalUser(name), whose callers intentionally rely on it returning null — the sweep-derived edit changes behavior and is pushed alongside the valid fix, producing a new review finding the next cycle.
Suggested fix
Append a boundary sentence, e.g.: 'Sweep only sites that share the exact invariant the finding describes; if equivalence is uncertain, or the fix would need site-specific judgment (fixtures, legacy files, docs the PR didn't touch), keep the site a separate finding instead of folding it into the class.'umm-actually · deepseek/deepseek-v4-flash-0731
| **Before pushing, re-scan the full diff once against every class the bot has | ||
| flagged on this PR so far** — this cycle's and earlier cycles'. Each push | ||
| triggers a complete re-review, so any instance the per-finding sweeps missed | ||
| becomes next cycle's comments. The target is convergence in one or two | ||
| cycles, not a comment-per-instance conversation. |
There was a problem hiding this comment.
Specify what to do when the pre-push re-scan finds a missed instance
Low severity · correctness · medium confidence
The pre-push gate directs a full-diff re-scan against every class the bot has flagged but never states what to do when the re-scan finds an instance the per-finding sweeps missed. The text that follows only offers 'stage, commit, push' or 'go to Step 5', so a literal execution pushes with the missed instance still present and the stated convergence target fails.
Failure scenario: Per-finding sweeps miss one sibling of a flagged class; the agent re-scans the diff as instructed, spots the sibling, and — with no directive to fix it — stages only the per-finding fixes and pushes. The bot's re-review produces exactly the comment the gate exists to prevent, buying another full cycle.
Suggested fix
Add a sentence after the re-scan directive: 'If the re-scan finds an instance of a flagged class, evaluate and fix it under the same valid/false-positive rules before pushing, then re-scan the affected files once more.'umm-actually · deepseek/deepseek-v4-flash-0731
|
umm-actually reviewed at 2 new finding(s) posted (2 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4-flash-0731 |
Monitoring loops converge one instance per bot cycle: fix the flagged site, push, wait ~3 minutes for the re-review, get the sibling flagged, repeat — recent PRs closed with 121, 72, 65, and 62 comments. The skill's Step 3 evaluates each finding thoroughly (valid vs false positive, lift, scope) but treats every finding as an instance; nothing asks for the pattern behind it, and Step 4's push-and-wait structure turns each missed sibling into another full cycle.
Changes
Companion to the existing anticipate-the-reviewer directive — this puts the rule inside the procedure that runs at the moment it matters instead of relying on session-start memory.
🤖 Generated with Claude Code