Skip to content

feat(ship-check): pr-monitor sweeps the finding's class before each push #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aliasunder merged 1 commit into from
Sep 18, 2026
Merged

feat(ship-check): pr-monitor sweeps the finding's class before each push #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions plugins/ship-check/skills/pr-monitor/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,17 @@ For each unresolved bot thread, do ALL of these in order:
Fix anything that's reasonable effort; only escalate to the user when the fix is
genuinely high lift.

4. **Reply to the comment** -- do this BEFORE resolving, for EVERY bot comment.
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.
Comment on lines +235 to +243

@umm-actually umm-actually Bot Sep 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


5. **Reply to the comment** -- do this BEFORE resolving, for EVERY bot comment.

Since `gh` posts as the user's account, every reply MUST include an attribution
footer. Claude runs use the family ID from system context, such as
Expand All @@ -258,7 +268,7 @@ For each unresolved bot thread, do ALL of these in order:
- **False positive** -- reply explaining why:
*"This is intentional -- [reasoning].\n\n---\n🔍 ship-check · pr-monitor · MODEL_ID"*

5. **Resolve the thread** (AFTER replying):
6. **Resolve the thread** (AFTER replying):
```
gh api graphql -f query='mutation {
resolveReviewThread(input: {threadId: "THREAD_ID"}) {
Expand Down Expand Up @@ -309,6 +319,12 @@ If the user provides a response, reply on their behalf and resolve.

### After handling all threads

**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.
Comment on lines +322 to +326

@umm-actually umm-actually Bot Sep 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


If you fixed any code: stage, commit, push. Then **go to Step 4** -- this is mandatory.

If no fixes were needed and no code was pushed: **go to Step 5.**
Expand Down
Loading