refactor(ship): review stage 1 with a subagent, never a review CLI - #12
Conversation
Stage 1 resolved a `review` slot that in practice pointed at a vendor review CLI - the same vendor whose GitHub App runs the stage 3 review that ship treats as the final bar. Both draw on one quota, so the local run spent credit on a judgment stage 3 reaches anyway, and the rate limit it earned resurfaced half an hour later as a PR review that would not settle. Stage 1 now always dispatches a Code Reviewer subagent. That keeps the pre-push gate (findings caught before a push cost one edit, not a commit-push-poll cycle) while making the two passes independent: a subagent reading this run's intent, then the bot reading the pushed diff cold. - Drop the `review` slot from stage 0's table, the executable-slots list, the `none`-is-illegal rule, and the config example. A `review:` line in an existing .ship/config.md is ignored, reported, and dropped on rewrite, so an older config degrades rather than failing. - Close the side doors: a review command must not be adopted as `verify`, and a review skill offering to handle stage 1 does not override it. - Ask the subagent for severity on every finding. Stage 1's blocking bar keys on critical/major, which a CLI emitted structurally and a subagent emits only when asked. - Delete stage 3's same-vendor carry-forward block, unreachable now that the two reviewers always differ. Findings are triaged fresh, with stage 1's recorded dispositions as context. Renumbering the remaining steps also fixes the `post-merge` row's stale pointer to "step 5". Ship plugin 1.0.0 -> 1.1.0.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe ship plugin is updated to version 1.1.0. Its pipeline now uses mandatory Code Reviewer subagent review, deterministic verification gates, deliberate staging, independent automated finding triage, and revised post-merge sequencing. ChangesShip pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ShipPipeline
participant CodeReviewerSubagent
participant PullRequestBot
participant ReleasePipeline
ShipPipeline->>CodeReviewerSubagent: Run mandatory Stage 1 review
CodeReviewerSubagent-->>ShipPipeline: Return findings and severity
ShipPipeline->>PullRequestBot: Request mandatory automated review
PullRequestBot-->>ShipPipeline: Return findings and review status
ShipPipeline->>ReleasePipeline: Watch release pipeline after merge
ReleasePipeline-->>ShipPipeline: Report release completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
skills/ship/SKILL.md (1)
256-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not assume independent reviewers cannot report the same finding.
Different reviewers can identify the same root cause. Keep the fresh-triage rule, but state that Stage 3 must not treat a Stage 1 disposition as resolution or assume a duplicate without reviewing the current finding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ship/SKILL.md` around lines 256 - 258, Revise the Stage 3 triage guidance to retain fresh review of every finding while explicitly stating that independent reviewers may identify the same root cause. Require Stage 3 to inspect the current finding and not treat a Stage 1 disposition as resolution or assume duplication without verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/ship/SKILL.md`:
- Around line 187-189: Update the Code Reviewer subagent contract in the review
dispatch instructions to require a structured severity value on every finding,
limited to the valid severity values used by the Stage 1 gate. Validate every
returned finding and fail the review round when any severity is missing or
invalid, rather than allowing it to pass to the blocking check.
- Around line 261-263: Update the termination rule in the review-pass loop to
stop only when the current settled review has zero actionable findings after
triage, regardless of whether those findings were reported in an earlier pass.
Preserve the five-pass cap and its final-pass push restriction, and continue
reporting unresolved findings with the pull request unmerged when the cap is
reached.
---
Nitpick comments:
In `@skills/ship/SKILL.md`:
- Around line 256-258: Revise the Stage 3 triage guidance to retain fresh review
of every finding while explicitly stating that independent reviewers may
identify the same root cause. Require Stage 3 to inspect the current finding and
not treat a Stage 1 disposition as resolution or assume duplication without
verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 245efd72-cfbf-4d8d-9d90-94521e1d6f73
📒 Files selected for processing (3)
.claude-plugin/marketplace.jsonREADME.mdskills/ship/SKILL.md
All three trace to the reviewer swap, and one of them was introduced by removing the stage 3 dedupe block. - Stage 1 asked for severity but nothing depended on getting it. Step 3 buckets "everything else" as non-blocking, so a genuinely major finding that came back unlabelled fell through the gate. Severity is now one of an enumerated set, and a missing or unrecognized one is assigned during triage from what the finding describes, treated as major when triage cannot place it. Assigning beats failing the round: the round cap is five, and spending one on a formatting miss costs a real pass. - Stage 3 terminated on "zero NEW actionable findings". The word new lost its antecedent when the same-vendor carry-forward block went away, and read literally it closes the loop on a finding the bot repeats because the last fix missed it - not new, not resolved. Termination now keys on every actionable finding being fixed or dispositioned after triage. - Stage 3 claimed none of the bot's findings could be duplicates of stage 1's. Overstated: two independent reviewers can reach the same root cause. The operative rule was already right, so only the justification changes - a familiar-looking finding is read and judged rather than assumed to be a repeat.
|
Disposition of the review-body nitpick ( The sentence I wrote overclaimed: "none of its findings are duplicates of stage 1's" is simply false, since two independent reviewers can reach the same root cause. The operative instruction around it was already correct (triage fresh, never set a finding aside unread), so only the justification changed:
Taking it despite being trivial-severity because it is a one-line claim that is wrong as written, and a false justification in a skill file is the kind of thing a later edit reasons from. All three findings from this pass are fixed in Note on the three SkillSpector |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Ran the stage 1 loop this change introduces against the change itself: five rounds of a Code Reviewer subagent, 33 findings, 28 applied. Round 5 came back with no blocking findings. In rounds 2, 3 and 4 every blocking finding was a defect in the previous round's fixes, which is the loop doing the thing it exists to do. Blocking defects fixed: - Stage 3 could terminate on a pass that PUSHED fixes and then merge a SHA no pass had reviewed, the same hole the merge step's SHA pin exists to close. Exit now requires a pass that applied no fixes, mirroring stage 1. - Stage 1 claimed `git add -N` leaves files staged for stage 2. It does not. Git reports such a file as added, it stops appearing as untracked, and a plain `git commit` writes none of its content, so a new file could merge missing with no gate noticing. Verified in a scratch repo on git 2.50.0; stage 2 now lists the entries with `git diff --name-only --diff-filter=A` and stages them by name. - Stripping a stale `review:` line contradicted the rule that .ship/config.md is written only when the user was asked. Reconciled as a second write trigger on the same preflight gate and commit, and an in-place line deletion rather than a regeneration, since these files carry hand-written rationale. - The resume-to-stage-3 route was exempted from the strip but not from the write, though the same reasoning applied: that route never reaches the commit either write rides. - A red flag fired on the config write the same change had just mandated. Non-blocking fixes: severity placement separated from claim confirmation; the non-blocking bucket widened from "minor" to "below the blocking bar"; a failed review gate given a stated consequence and barred from ending the stage; a prohibited verify command given a fall-through path and an ask trigger; intent-to-add entries reset on any stop before stage 2; stage 3 mapping the bot's labels onto this skill's severities; a dangling comparison removed; reporting extended. Dispositioned, not fixed: stage 3 step 1's conditional on a bot being configured, and stage 3's "Otherwise:" scoping (both pre-existing and out of scope), SKILL.md frontmatter staying 1.0.0 (repo convention - every skill here is 1.0.0 while fathom's plugin is 2.2.0), reporting a deferred strip, and letting a strip-only commit message describe itself.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/ship/SKILL.md`:
- Around line 234-241: Update the staging instructions to capture tracked files
modified or deleted during this run and explicitly stage those paths by name
before committing. Preserve the existing intent-to-add handling for new files,
and require asking about any changed path with unclear ownership rather than
staging it automatically.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The PR review caught what five local rounds did not: stage 2 step 1 had grown into six lines of intent-to-add mechanics and said nothing about staging tracked files. An agent could read the `--diff-filter=A` list as the list of what to stage, commit only the new files, and ship a change with every stage 1 fix missing. The step now opens with all three cases - tracked modifications, tracked deletions, new files - each staged by explicit path, and says plainly that the intent-to-add material below covers the new-file case alone. The imbalance came from this branch: earlier rounds kept adding new-file detail without rebalancing the step around it.
Merging, and what did and did not get reviewedRecording this because this PR tightens the very rule being bent. Reviewed:
Not reviewed: Why merge rather than wait: CodeRabbit applied adaptive rate limiting during this PR — the window grew 31 → 44 → 50 minutes, and the last attempt was cut off mid-review ( Gates: SkillSpector, semgrep, and both repo scripts ( Known follow-ups, dispositioned not fixed — pre-existing, out of this change's scope:
Both are worth a follow-up PR; neither is introduced or worsened here. |
1 participant
Problem
Ship's stage 0 resolved a
reviewslot, and in this repo it resolved to a CodeRabbit CLI invocation. Stage 3 then waits on the CodeRabbit GitHub App as its final bar. Same vendor, one quota.So every ship run spent local review credit on a judgment stage 3 reaches on its own, and the rate limit it earned surfaced 20-30 minutes later as a PR review that would not settle. Stage 3 already treated the local run as non-authoritative (
this bot is the final bar and is never skipped, not even when stage 1 already ran the same vendor's CLI locally), so the spend bought nothing.Change
Stage 1 always dispatches a Code Reviewer subagent. The pre-push gate stays - a finding caught before the push costs one edit to a dirty tree, while the same finding caught by the bot costs commit + push + poll + re-settle, and every fix push restarts the bot pass. What changes is that the two passes are now independent reviewers rather than one reviewer twice: a subagent reading this run's intent, then the bot reading the pushed diff cold.
skills/ship/SKILL.md:reviewslot row deleted from the resolution table, and from the executable-slots list and thenone-is-illegal rule. A new paragraph states why there is no slot, so a future edit does not reintroduce it as an obvious convenience.verify(verify is for deterministic gates, review is for judgment), and a review skill offering to handle stage 1 does not override it. The second matters here: the globally installedcoderabbit:code-reviewskill self-describes as triggering "autonomously when the agent thinks a review is needed", which is precisely stage 1.review:line in an existing.ship/config.mdis ignored, reported in the final summary, and dropped when the file is rewritten. Nothing errors, so an older config degrades rather than failing.post-mergetable row's pre-existing stale pointer to "step 5".verify, or via a skill that wraps one.README.md- the "The bot is the final bar, asked once" bullet claimed the local CLI review still runs. Replaced with the two-reviewers rationale..claude-plugin/marketplace.json- ship 1.0.0 -> 1.1.0, README version synced bybin/sync-versions.sh.Not in scope
.claude/hooks/coderabbit-on-pr.shis untouched. It drives the PR-side bot loop (stage 3, via thepr-hookslot) and never invokes the CLI - that is the review being kept.Verification
Both repo gates pass on the branch:
Docs-only change to a skill; there is no runtime to exercise beyond the next ship run, which will be the real test of stage 1 severity reporting.
Summary by CodeRabbit
shipplugin to version 1.1.0.