fix(pr-review): dispatch sub-agents by persona when the runtime registers them - #1211
Conversation
…ters them Since fullsend 0.42.0 the pi runtime registers the skill's sub-agents/*.md files as personas: the orchestrator dispatches one with subagent_type: <persona name> and no model, and the runner resolves the model from the repository's agents[].subagents config and the file's frontmatter (fullsend ADR 0104). The runtime note in the system prompt says exactly that. Step 4 item 2, however, said the opposite — pass model from the frontmatter and "Do not set subagent_type" — and the orchestrator followed the skill over the note. Measured on 2026-09-08 with a repo that routes correctness, security and challenger to Grok and the two document checks to Gemini: Bootstrap resolved all nine personas as configured, then every child ran on opus-4-6 or sonnet as an anonymous sub-agent with a model argument. The per-repo routing was inert. Make the dispatch shape depend on the runtime note: persona name and no model when personas are listed, frontmatter model and no subagent_type otherwise (Claude Code, where the personas are not registered agent types and an unknown subagent_type is a hard error). Apply the same rule to the security-triage pre-pass, risk-assessment and the challenger. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
PR Summary by QodoRoute PR review sub-agents through registered personas
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1. Persona routing lacks regression tests
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…rsona Review feedback: the authoritative persona list is the runtime note's, which holds only the personas this run registered — a persona whose frontmatter model the project cannot serve is dropped at Bootstrap, and dispatching its name is a hard rejection, not a fallback. Say "as listed in the runtime note", and send an unlisted persona anonymously with its frontmatter model instead of dropping the dimension. Also make the Claude Code case observable: there is no runtime note there at all, so the condition is its absence, not an empty list. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
…default A persona is left unregistered on pi when this run cannot serve its model, so falling back to the frontmatter alias would be rejected for the same reason (the extension validates a non-empty model against the run's model table). Omit both subagent_type and model instead: the child takes the run's sub-agent default, which is always servable. The Claude Code case keeps the frontmatter model. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 8:41 PM UTC · Completed 8:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.47 |
|
Risk Assessment: elevated (3/5) DetailsTier 1 signals are unchanged from the prior assessment (5 files, 91 lines, 1 protected path, zero test coverage, medium blast radius, no dependency or CI changes); Tier 2 confirms the same highly volatile pattern (49 commits in 30d, 12 authors, 76 fix/revert commits in 90d); composite of 2.84 rounds to 3 and re-review anchoring preserves the prior elevated score. Previous runRisk Assessment: elevated (3/5) DetailsTier 1 and Tier 2 signals are unchanged from the prior assessment — five files, one protected path, zero test coverage, medium blast radius, with docs/review.md (10 distinct authors, 17 commits in 30d, 26 fix commits) and skills/pr-review/SKILL.md (54 cumulative fix commits, 18 in 30d) remaining highly volatile; per re-review anchoring, the prior elevated score of 3 is preserved. Previous run (2)Risk Assessment: elevated (3/5) DetailsPrior assessment described a two-file change; the current PR spans five files (88 lines, medium blast radius), adding the highly volatile docs/review.md (10 distinct authors, 17 commits in 30d, 26 fix commits) to scope alongside SKILL.md already-elevated churn, pushing the weighted composite from the prior moderate 2 to an elevated 3. Previous run (3)Risk Assessment: moderate (2/5) DetailsA small two-file, 78-line documentation and baseline update to a high-churn, multi-author skills file; SKILL.md's 9 distinct authors and 18 commits in 30 days continue to elevate the git-history sub-score, but the change itself is narrow in scope and touches no security-sensitive or CI-critical paths, sustaining the prior moderate score. Previous run (4)Risk Assessment: moderate (2/5) DetailsA small, single-file documentation change to a high-churn, multi-author skills file; the underlying file's churn history elevates the score slightly above low, but the change itself is narrow in scope and touches no security-sensitive or CI-critical paths. |
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsHigh
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ValidationRun 1, current agents 65 turns, $5.80 (opus-4-6 ×3 $2.36, sonnet-5 ×6 $3.45). Run 2, #1211 skill (46a779a) — all eight dispatches by persona name, no 43 turns, agent exit 0, $5.34 — |
… ceiling skillsaw's context-budget baseline pins pr-review at 15,709 tokens and main sits at 15,705, so any addition fails the lint. The dispatch rule is compressed to what the orchestrator needs (the three cases in step 4 item 2, one line each at the other three call sites) and the baseline ceiling for skills/pr-review/SKILL.md moves to the new estimate. No other baseline entry changes. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:06 PM UTC · Completed 9:24 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.29 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Validation, part 2 — the default Claude runtime (existing users)Same probe harness on this branch ( Every Agent tool call in the transcript (
No dispatch failure and zero Cost context on this PR. The fleet's own six review runs of fullsend#7087 with the |
…t fallback Both runtime-support sections still said review and retro fall back to a single-context pass on pi. Since fullsend 0.42.0 the runner's Agent tool dispatches the same sub-agents on pi, and the repository's .fullsend/config.yaml can route each persona to its own model (agents[].subagents), which this branch now does. Say so, and point at the per-persona configuration docs. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> (cherry picked from commit f529668)
…E runtime matrix Review feedback: the step 3c-1 third case said "step 4 item 2 fallback" while setting subagent_type: Explore, which step 4 does not — say why Explore is kept (a built-in read-only type the runner always accepts) and that only the omitted model resolves as in step 4. Match the bullet style between the two sites, wrap one long line, and say "usually" for why a persona is unregistered (unservable model is the common cause, not the only one). README no longer labels pi review/retro single-context; docs/review.md and docs/retro.md were fixed in the previous commit. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:29 PM UTC · Completed 9:49 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.41 |
fullsend-ai-review
Bot
added
risk/elevated
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
Other than my two comments, looks good to me. At least point to the docs on both agents, now it is inconsistent.
…docs Maintainer review: keep the runtime-support sections short and consistent — say that sub-agents on pi run with their model and point at the fullsend pi docs for how to set a different one, instead of restating the mechanism in each agent's doc. Also state in step 3c-1 that only the model follows step 4 item 2 while subagent_type stays Explore so the pre-pass remains read-only. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 12:01 PM UTC · Completed 12:24 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.95 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Step 6d dispatches the challenger with "the step 4 item 2 dispatch shape", and step 4 item 2's Claude-Code case summarises the frontmatter models as "opus for correctness and security, sonnet for the rest". The challenger's frontmatter is model: opus, so the literal reading of that shortcut routes the adversarial pass to sonnet instead of reading challenger.md. Name it in the parenthetical. Found by fullsend-ai-review on d345998. Skill grows 5 tokens; baseline ceiling regenerated for the pr-review entry only (16,045 -> 16,050). Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 1:07 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.55 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
skills/pr-review/SKILL.md(file-level): Line 465 · [low] logic-error
The sentence "It uses haiku for speed — classification does not require deep reasoning" is an unqualified statement that only holds for the Claude Code dispatch case. On pi, the model is resolved by the runner (case 1) or defaults to the sub-agent default (case 3), and is not guaranteed to be haiku.
|
🤖 Finished Retro · ✅ Success · Started 1:33 PM UTC · Completed 1:46 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.96 |
Retro: PR #1211 —
|
2 participants
What
The pr-review skill tells the orchestrator how to dispatch its sub-agents. On pi (fullsend 0.42.0 and later) the runtime registers
sub-agents/*.mdas personas and its runtime note says: dispatch withsubagent_type: <persona name>and nomodel; the runner resolves each persona's model from the repository'sagents[].subagentsand the file's frontmatter (fullsend ADR 0104). Step 4 item 2 said the opposite — "modelset from each sub-agent file's frontmatter … Do not setsubagent_type" — and the orchestrator follows the skill over the note.This PR makes the dispatch shape depend on the runtime note, in the four places that dispatch: step 3c-1 (security-triage), 3c-2 (risk-assessment), step 4 item 2 (dimension sub-agents) and 6d (challenger):
subagent_typemodelExplorefor security-triage, as today)Claude Code behaviour is unchanged: there the personas are not registered agent types, an unknown
subagent_typeis a hard error, and the frontmattermodelargument is what routes children.Evidence
Local
fullsend run reviewon 2026-09-08 (fullsend built frommain, agentsmainat a359832, 0.42.0 image) with a repository config that routescorrectness,securityandchallengertoxai/grok-4.6anddocs-currency,style-conventionstogoogle-vertex/gemini-3.8-flash:subagents: correctness → xai-vertex/xai/grok-4.6 (from subagents.correctness), …).[fullsend-agent] #1 anthropic-vertex/claude-opus-4-6 start "Correctness review",#2 … claude-opus-4-6 "Security review",#4 … claude-sonnet-5 "Style conventions review",#5 … claude-sonnet-5 "Docs currency review"— no[persona]tag, none on Grok or Gemini.The "Do not set
subagent_type" line arrived in #1178 (merged 2026-09-05), after the run that verified persona dispatch on pi with the then-current skill.Validation
[fullsend-agent] #n [correctness] xai-vertex/xai/grok-4.6 start …for the routed personas andper_model_usagewith five models. Result will be posted here.Also in this PR (docs)
Alongside the four pr-review dispatch sites, this PR corrects the pi runtime-support
documentation that described the old behaviour:
docs/review.md— pi review runs the sub-agent roster, each persona with its own model,rather than a single-context fallback.
docs/retro.mdand the README runtime matrix — same correction for retro, whosesub-agents are anonymous and therefore inherit the agent's model. The retro wording is
deliberately NOT identical to review's: retro ships no
sub-agents/persona files, so"sub-agents run with the agent model" is the accurate phrasing for it.
These are the same behaviour this PR fixes in the skill, so they are in scope; calling them
out here explicitly at the review bot's request.
Rollback
Revert; the skill goes back to frontmatter-model dispatch on every runtime.