Await specialist sub-agents so the review panel posts a verdict - #459
Conversation
The parallel dispatch added in #458 launches the six specialists with `run_in_background: true`. A background agent returns immediately, so the dispatching turn ends with every review still running. Interactively that is fine — the notifications arrive and work resumes. Headlessly it is not: when the turn ends, the CI job ends. Observed on PR #451 (run 30216174198). The job exited `success` after 3m22s having posted nothing. Its final message was "I'll synthesize the verdict and post it as a single PR comment once all six report back" — the six had been dispatched moments earlier and none had returned. From outside the workflow this is indistinguishable from a clean review: a green check and no findings. Dispatch synchronously instead — `run_in_background: false`, still all in one message. The single message is what makes them concurrent; the synchronous flag is what makes the call block until all six return. The parallelism #458 wanted is preserved; only the fire-and-forget goes. Also state in the Completeness Gate that the run must never end there. A panel that stops after dispatching produces nothing actionable and looks identical to a clean pass, so a partial verdict naming the failed specialists beats silence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 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. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
✨ 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 |
PR Summary by QodoAwait parallel specialist agents so review panel always posts a verdict
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
136 rules✅ Skills:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c7250922c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Conflicts with #460 on exactly one line, in all three skill copies. #460 adds a seventh specialist and changes the count; this changes the flag. Both edit: Resolution either way is Worth landing this one first regardless: without it the seventh specialist would be dispatched and abandoned like the other six. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #459 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 145 145
Lines 11418 11418
=======================================
Hits 10681 10681
Misses 737 737 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolves the conflict with stbenjam#459 as flagged on both PRs — `all 7` from here, `run_in_background: false` from there. Addresses the review findings: - **The BLOCKING criterion was unsatisfiable.** Palimpsest was told to escalate a factually false comment only when Architecture or the Technical Writer corroborated it — but specialists run isolated from one another, so it could never see that and would always self-downgrade, while synthesis never upgraded it back. A corroborated correctness defect could ship as a SUGGESTION under an APPROVE. It now reports a `BLOCKING CANDIDATE` and the arbiter assigns the final severity in a new synthesis step, which may not leave one unsettled. - **Regression coverage** for both this reviewer and the stbenjam#459 dispatch fix, in a new `tests/test_review_panel_skill.py`. The panel is a prompt, so its failure modes are textual and invisible to the rest of the suite — which is exactly how the headless silent-exit shipped green. The guards cover: dispatch is synchronous and background dispatch appears only in its own prohibition; the run may not end before the verdict; every specialist is rostered with a scope file that exists and a quality gate; Palimpsest reports a candidate rather than deciding; the three skill copies are byte-identical; and the verdict template has a section per specialist. Adding a seventh specialist pushed SKILL.md past the 3,000-token warn limit its own linter enforces — main was already at ~2,930. Serial mode moved to `references/serial-mode.md`: it is the non-default path, so an always-read copy costs every invocation for something most never use. That is the progressive-disclosure pattern the skill already applies to specialist scopes, and stbenjam#458 inlined dispatch specifically because it *is* always read. The file is now smaller than on main despite the addition, and a test pins the budget so the next specialist has to face the same question. No README change: the review panel is not documented there, so there is nothing for a new specialist to fall out of sync with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
The bug
The parallel dispatch added in #458 launches the six specialists with
run_in_background: true. A background agent returns immediately, so the dispatching turn ends with every review still in flight. Interactively that's fine — the completion notifications arrive and work resumes. Headlessly it isn't: when the turn ends, the CI job ends.The panel has posted no verdict since #458 landed.
Evidence
Run 30216174198 on #451 — exited
successafter 3m22s, 22 turns, having posted nothing. From its own execution log, the six dispatches and then the final message:They had been launched moments earlier. None had returned. The turn ended; the job ended.
The failure mode is the bad kind:
is_error: false, a green check, and no findings — indistinguishable from a clean review unless you go read the execution artifact. Compare run 30214646754 (pre-#458, serial) which took ~14 minutes and posted a full verdict.The fix
Dispatch synchronously —
run_in_background: false, still all six in a single message.Both halves matter, and the skill now says so explicitly:
run_in_background: falseis what makes the call block until all six returnSo #458's parallelism is fully preserved; only the fire-and-forget goes. Six concurrent reviews, awaited.
I also added a line to the Completeness Gate: the run must never end there. If specialists are missing and can't be recovered, post the verdict anyway naming which ones failed — a partial verdict beats silence that reads as approval.
Verification
make updateregenerates cleanly; all three copies (.apm/,.agents/,.claude/) verified byte-identical.venv/bin/skillsaw lint .apm/skills/skillsaw-review-panel/— 0 errors, 0 warnings, A+make lintclean,make test2965 passedNote on overlap
There's a second PR in flight adding a seventh specialist to this same skill. It touches the roster table and frontmatter; this touches Step 3's dispatch mechanics and Step 4. Different regions, so they should merge cleanly in either order — but worth landing this one first, since without it the new specialist would never report either.
🤖 Generated with Claude Code