feat(workflows): cascade PR retargeting + bounded review/resolve loop - #63
Conversation
The composite ship cascade inserted every child workflow_run with the parent's target verbatim, so review/resolve (registered with `context: "pr"`) inherited the originating issue and immediately failed their `target.type !== "pr"` guard. Issue #16's ship cascade halted at step 3 with `review failed: review requires PR target`, leaving PR #62 with no bot review or resolve coverage. `onStepComplete` now derives the child target via `deriveChildTarget`: when the next step's registry context is "pr" but the parent target is an issue, it discovers the PR number from the just-completed child's state (typical implement → review hand-off) or the parent's state (preserved across subsequent transitions). Missing pr_number fails the parent with a clear reason instead of silently inheriting issue → review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A single review/resolve pass leaves the ship pipeline under-validated:
resolve can introduce new issues while fixing old ones, and the cascade
finishes without ever confirming the fixes hold up under a fresh read. This
adds a bounded loop so review runs at least twice before the parent ship
flips to succeeded.
Semantics (cap = config.reviewResolveMaxIterations, default 2, range 1–5):
- After review-N: if N ≥ 2 AND total findings == 0, the parent succeeds
early with "review found no issues after N iterations".
- After resolve-N: if N < cap, insert another review at
ship.steps.indexOf("review") (the cascade picks resolve up naturally
again at index 4). Else the parent succeeds; if last_review_findings > 0
the terminal message recommends manual re-review since resolve-N's
fixes were never re-validated.
Findings come from `review.ts`'s new `countFindings()` parser, which counts
the severity tags (`[blocker]`, `[major]`, `[minor]`, `[nit]`) the agent
prompt already mandates. `total` excludes `nit` so taste-level comments
never keep the loop spinning.
Loop-back also relies on the cascade-retargeting fix from the prior commit:
the second-iteration review must run against the PR opened by implement,
not the originating issue.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…late Aligns the PR template surface with the `/pr-description` skill format and gives the implement workflow its own machine-fillable template so bot PRs stop drifting from the human shape. - `.github/PULL_REQUEST_TEMPLATE.md` (human) — Summary / Diagram (optional, with GitHub-compat mermaid hints) / Changes / Related Issues / Test plan. Drops Screenshots (this is a server, not a UI). Keeps the testing checkboxes as a self-review prompt. markdownlint-disable MD041 + cspell ignore for WCAG inline so the template doesn't fight tooling. - `.github/PULL_REQUEST_TEMPLATE/bot-implement.md` — bot template. Mirrors the human shape but adds Files changed / Commits / Tests run / Verification sections the agent already populates during its run. Test-plan boxes are pre-checked since the bot only opens PRs after running the gates. - `src/workflows/handlers/implement.ts` — prompt now instructs the agent to read `.github/PULL_REQUEST_TEMPLATE/bot-implement.md`, fill each section, write to a temp file, and pass `gh pr create --body-file …` so `gh` cannot silently fall back to the human template. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR restructures PR templates for human and bot contributions, adds a review iteration limit config parameter, extracts severity-tagged findings from review reports into structured format, updates the implement workflow to use a bot-specific template, and refactors the orchestrator to support bounded review/resolve loops with PR targeting logic and state persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Review Handler
participant Config
participant State
Orchestrator->>Config: Read reviewResolveMaxIterations
loop while review_iterations < max AND findings.total > 0
Orchestrator->>Review Handler: Execute review step
Review Handler->>State: Parse REVIEW.md & extract findings
Review Handler->>State: Update parent.state.last_review_findings
State->>Orchestrator: Increment parent.state.review_iterations
Orchestrator->>Orchestrator: Check termination condition
alt findings.total == 0 OR iterations >= 2
Orchestrator->>State: Mark parent succeeded
else iterations < max
Orchestrator->>Orchestrator: Enqueue resolve step
Orchestrator->>Orchestrator: Loop back to review
end
end
Orchestrator->>State: Finalize with optional humanMessage warning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Required by FR-019 (docs-sync guard, scripts/check-docs-sync.ts) — the prior three commits touched src/workflows/handlers/implement.ts, src/workflows/handlers/review.ts, and src/workflows/orchestrator.ts without updating docs/BOT-WORKFLOWS.md, which trips the CI gate. - Documents the new state.findings shape on `review`'s outputs. - New "Cascade target retargeting" subsection on `ship` explaining deriveChildTarget and pr_number persistence. - New "Bounded review/resolve loop" subsection covering the iteration semantics, REVIEW_RESOLVE_MAX_ITERATIONS knob, and the manual-re-review warning when the cap is reached with non-zero findings. - New "PR body" subsection for `ship` pointing implement at .github/PULL_REQUEST_TEMPLATE/bot-implement.md. - Cost note updated: minimum 2 review + 1 resolve per ship; worst case 2 review + 2 resolve at the default cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/workflows/handlers/review.ts (1)
277-284: Tag-counting is regex-anywhere — fragile if the agent quotes example tags in REVIEW.md.Because the regex matches the literal tag anywhere in the report, any severity tag that appears inside a code fence, inline-code span, or quoted example in REVIEW.md will bump the count and skew the loop short-circuit decision (e.g., a quoted "see prior
[major]example" in## Reasoningwould over-count by 1).The doc-comment already calls out this is a "sufficient — and stable — proxy", so this is acceptable for the current scope, but if you start seeing the loop fail to short-circuit on otherwise-clean reviews it's the first place to look. A line-anchored variant (
/^\s*[*-]?\s*\[(blocker|major|minor|nit)\]/gim) would tighten this without changing the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/review.ts` around lines 277 - 284, countFindings currently counts severity tags anywhere in the text which can pick up examples or inline code; update countFindings to only match tags anchored to list/line starts by using a line-anchored regex (e.g. /^\s*[*-]?\s*\[(blocker|major|minor|nit)\]/gim) inside the count logic (function countFindings) and then tally matches per captured group (blocker, major, minor, nit) to preserve the same returned shape and total semantics..github/PULL_REQUEST_TEMPLATE/bot-implement.md (1)
9-12: Doc nit: template comment says--body, but the implement handler uses--body-file.The implement handler prompt (
src/workflows/handlers/implement.tslines 76–78) tells the agent to write the rendered body to a temp file and pass it viagh pr create --body-file /tmp/pr-body.md. The template's self-description here says--body, which is a differentghflag with different semantics (string vs. file path). Worth tightening so a maintainer scanning just the template doesn't get a misleading impression.📝 Proposed wording fix
The agent reads this file, fills each section from its actual work, and -passes the result as `--body` to `gh pr create`. Sections marked optional +passes the rendered result via `gh pr create --body-file <tempfile>` (so +`gh` does not auto-pick the human PR template). Sections marked optional should be omitted if they would only contain placeholder text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/PULL_REQUEST_TEMPLATE/bot-implement.md around lines 9 - 12, Update the PR template text to match the actual flag used by the implement handler: replace the mention of `--body` with `--body-file` so maintainers aren't misled; specifically, change the sentence mentioning `--body` in the template to reference `--body-file` (the same flag written by the implement handler), and ensure wording clarifies that a temp file path is passed rather than an inline string.src/workflows/orchestrator.ts (2)
400-407:extractFindingssilently returns 0 whenfindingsis missing/malformed — could mask a regression as a clean review.If
reviewever succeeds without writingstate.findings(e.g. a future regression in the handler, or a partial state write), this returns 0. Combined withreviewClean = … && lastReviewFindings === 0, that turns a missing signal into an early ship-succeed, which is exactly the silent-bail class of bug this PR is otherwise tightening.Since the review handler is now contractually required to write
findings, consider logging a warning (or even falling back toNumber.MAX_SAFE_INTEGERso the loop refuses to short-circuit) when the review child has succeeded butfindingsis missing/malformed. At minimum alogger.warnhere would surface the regression instead of silencing it.♻️ Proposed defensive fix
-function extractFindings(state: Record<string, unknown>): number { - const raw = state["findings"]; - if (raw !== null && typeof raw === "object") { - const total = (raw as Record<string, unknown>)["total"]; - if (typeof total === "number" && Number.isFinite(total) && total >= 0) return total; - } - return 0; -} +function extractFindings( + state: Record<string, unknown>, + logger?: pino.Logger, +): number { + const raw = state["findings"]; + if (raw !== null && typeof raw === "object") { + const total = (raw as Record<string, unknown>)["total"]; + if (typeof total === "number" && Number.isFinite(total) && total >= 0) return total; + } + // Treat a missing/malformed findings as "non-clean" so a regressed review + // handler cannot trip the early short-circuit. + logger?.warn( + { findings: raw }, + "orchestrator: review child has no usable findings.total — refusing to short-circuit", + ); + return Number.MAX_SAFE_INTEGER; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/orchestrator.ts` around lines 400 - 407, The extractFindings function currently silences missing/malformed state["findings"] by returning 0; change it to detect when findings is absent or malformed and surface a warning and a safe non-short-circuit value: inside extractFindings, when raw is null/undefined or total is not a valid non-negative finite number, call the existing logger.warn (the logger in scope) with context that the review child succeeded but state.findings is missing/malformed (include the state payload or identifying ids), and return Number.MAX_SAFE_INTEGER (or another large sentinel) instead of 0 so the orchestrator loop won't treat a missing finding as a clean 0; keep the normal return path when total is a valid number.
131-134: Note on cap=1 boundary: early-exit can never fire whencap === 1.
reviewCleanrequiresreviewIterations >= 2(hardcoded), but the schema permitscap=1. Withcap=1, the loop runs review-1 → resolve-1 and always falls into the terminal branch on resolve-1 — the early-exit never has a chance to fire because no second review can run. Functionally fine (cap=1 effectively means "never loop"), but the comment block above could note this so an operator settingcap=1doesn't expect early-exit-on-clean-review-1 behaviour. Alternatively, replacing the hardcoded2withMath.min(2, cap)would make cap=1 behave as "first clean review wins".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/orchestrator.ts` around lines 131 - 134, The current logic uses a hardcoded 2 in computing reviewClean which prevents early-exit when config.reviewResolveMaxIterations (cap) is 1; update reviewClean to use Math.min(2, cap) so the condition becomes: const reviewClean = isShipParent && isReviewChild && reviewIterations >= Math.min(2, cap) && lastReviewFindings === 0; also update the adjacent comment to note that cap=1 will now allow a first-clean-review to terminate (or alternatively document that cap=1 means "never loop") so operators aren’t surprised; reference variables: config.reviewResolveMaxIterations (cap), reviewIterations, lastReviewFindings, reviewClean, shouldLoopBackToReview.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.ts`:
- Around line 346-354: Add documentation for the new
REVIEW_RESOLVE_MAX_ITERATIONS environment variable to docs/CONFIGURATION.md:
describe the env var name and the corresponding schema key
reviewResolveMaxIterations, state Default: 2, Range: 1–5, and include the
semantics that it caps composite ship review/resolve iterations, that each
iteration is one review run, and that a clean review after at least 2 iterations
short-circuits the loop (otherwise when the cap is reached the ship marks
succeeded but recommends manual re-review). Place this entry under the
"Orchestrator and daemon" or the composite ship settings subsection consistent
with adjacent entries.
---
Nitpick comments:
In @.github/PULL_REQUEST_TEMPLATE/bot-implement.md:
- Around line 9-12: Update the PR template text to match the actual flag used by
the implement handler: replace the mention of `--body` with `--body-file` so
maintainers aren't misled; specifically, change the sentence mentioning `--body`
in the template to reference `--body-file` (the same flag written by the
implement handler), and ensure wording clarifies that a temp file path is passed
rather than an inline string.
In `@src/workflows/handlers/review.ts`:
- Around line 277-284: countFindings currently counts severity tags anywhere in
the text which can pick up examples or inline code; update countFindings to only
match tags anchored to list/line starts by using a line-anchored regex (e.g.
/^\s*[*-]?\s*\[(blocker|major|minor|nit)\]/gim) inside the count logic (function
countFindings) and then tally matches per captured group (blocker, major, minor,
nit) to preserve the same returned shape and total semantics.
In `@src/workflows/orchestrator.ts`:
- Around line 400-407: The extractFindings function currently silences
missing/malformed state["findings"] by returning 0; change it to detect when
findings is absent or malformed and surface a warning and a safe
non-short-circuit value: inside extractFindings, when raw is null/undefined or
total is not a valid non-negative finite number, call the existing logger.warn
(the logger in scope) with context that the review child succeeded but
state.findings is missing/malformed (include the state payload or identifying
ids), and return Number.MAX_SAFE_INTEGER (or another large sentinel) instead of
0 so the orchestrator loop won't treat a missing finding as a clean 0; keep the
normal return path when total is a valid number.
- Around line 131-134: The current logic uses a hardcoded 2 in computing
reviewClean which prevents early-exit when config.reviewResolveMaxIterations
(cap) is 1; update reviewClean to use Math.min(2, cap) so the condition becomes:
const reviewClean = isShipParent && isReviewChild && reviewIterations >=
Math.min(2, cap) && lastReviewFindings === 0; also update the adjacent comment
to note that cap=1 will now allow a first-clean-review to terminate (or
alternatively document that cap=1 means "never loop") so operators aren’t
surprised; reference variables: config.reviewResolveMaxIterations (cap),
reviewIterations, lastReviewFindings, reviewClean, shouldLoopBackToReview.
🪄 Autofix (Beta)
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: ebdaa2da-8c97-413c-8eda-3bbdee6acffb
📒 Files selected for processing (8)
.github/PULL_REQUEST_TEMPLATE.md.github/PULL_REQUEST_TEMPLATE/bot-implement.mdsrc/config.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/review.tssrc/workflows/orchestrator.tstest/workflows/handlers/review.test.tstest/workflows/orchestrator.test.ts
- docs(config): document REVIEW_RESOLVE_MAX_ITERATIONS in
docs/CONFIGURATION.md (CLAUDE.md docs-sync requirement for src/config.ts
env additions). New "Composite ship workflow" section.
- docs(template): bot-implement.md self-description now references
--body-file (the flag the implement handler actually uses), not --body.
- review.ts countFindings: line-anchored regex
/^\s*(?:[*\-+>]\s+)?\[(blocker|major|minor|nit)\]/gim so a quoted
example tag in REVIEW.md prose can't over-count and mis-trip the loop's
early-exit. Fixture updated to match the new contract.
- orchestrator.ts extractFindings: missing or malformed state.findings
now warns via the in-scope logger and returns Number.MAX_SAFE_INTEGER
so a regressed review handler can't accidentally short-circuit ship to
succeed without confirmation.
- orchestrator.ts reviewClean: floor is Math.min(2, cap) so a cap=1
deployment ("never loop") still terminates on its single review while
cap≥2 preserves the original "at least two passes" guarantee.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@coderabbitai — addressed the 4 nitpicks from your review in
Typecheck + lint + format clean. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7977
Bonus appreciated: the new All four original concerns are fully addressed. 🎉 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
# [1.5.0](v1.4.0...v1.5.0) (2026-04-26) ### Features * **workflows:** cascade PR retargeting + bounded review/resolve loop ([#63](#63)) ([3079014](3079014))
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Two cascade-correctness fixes for ship plus aligned PR templates. Issue #16's
ship pipeline halted at step 3 (
review failed: review requires PR target)because every cascade child inherited the parent ship's issue target — review
and resolve are PR-scoped and immediately bailed. Even after the cascade can
reach review/resolve, a single pass leaves the pipeline under-validated:
resolve can introduce new issues while fixing old ones, and the cascade ends
without re-checking. This PR fixes both, then aligns the PR template surface
so bot- and human-authored PRs use predictable shapes.
Diagram
Before
After
flowchart LR triage["triage<br/>issue"]:::ok plan["plan<br/>issue"]:::ok implement["implement<br/>issue<br/>writes pr_number"]:::ok retarget["retarget<br/>issue → PR<br/>persist pr_number"]:::fix review1["review-1<br/>target=PR<br/>findings tagged"]:::ok resolve1["resolve-1<br/>fix CI + comments"]:::ok loop{"iterations < cap?"}:::fix review2["review-2<br/>target=PR"]:::ok resolve2["resolve-2<br/>optional"]:::ok done["parent succeeded<br/>clean OR manual-re-review hint"]:::ok triage --> plan --> implement --> retarget --> review1 --> resolve1 --> loop loop -- yes --> review2 review2 -- findings==0 --> done review2 -- findings > 0 --> resolve2 --> done loop -- no --> done classDef ok fill:#196f3d,color:#ffffff,stroke:#ffffff classDef fix fill:#6c3483,color:#ffffff,stroke:#ffffffChanges
src/workflows/orchestrator.ts—deriveChildTargetretargets the next child when its registry context isprbut the parent target is anissue. Source ofpr_number: just-completed child's state (typicalimplementhand-off) or parent state (preserved across loop-back). Missingpr_numberfails the parent with a clear reason instead of silently inheriting the issue target.src/workflows/orchestrator.ts— bespoke ship review/resolve loop. After review-N: if N≥2 AND total findings == 0 → parent succeeds early. After resolve-N: if N < cap → insert another review atsteps.indexOf(\"review\"). Else → succeeded; iflast_review_findings>0the terminal message recommends manual re-review.src/config.ts—REVIEW_RESOLVE_MAX_ITERATIONS(int, 1–5, default 2).src/workflows/handlers/review.ts—countFindings()parses the severity tags ([blocker]/[major]/[minor]/[nit]) the agent prompt already mandates.totalexcludes nits so taste-level comments never keep the loop spinning. Findings written tostate.findings..github/PULL_REQUEST_TEMPLATE.md— restructured to match the/pr-descriptionskill format (Summary / Diagram / Changes / Related Issues / Test plan). markdownlint-disable MD041 + cspell-ignore inline..github/PULL_REQUEST_TEMPLATE/bot-implement.md— new bot-specific template. Mirrors the human shape; adds Files changed / Commits / Tests run / Verification sections the agent already populates.src/workflows/handlers/implement.ts— prompt now instructs the agent to read the bot template, fill it, and pass viagh pr create --body-filesoghcannot fall back to the human template.Related Issues
Test plan
Summary by CodeRabbit
New Features
Documentation