-
Notifications
You must be signed in to change notification settings - Fork 73
fix(agents): There are no subagent "types"; they don't compose #2098
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1011,7 +1011,27 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep | |||||
| printer.StepDone(fmt.Sprintf("Agent-input files copied (%.1fs)", time.Since(inputStart).Seconds())) | ||||||
| } | ||||||
|
|
||||||
| // 8c. Host-side scan (Path A): scan the target repo's context files | ||||||
| // 8c. Make the target repo read-only if the harness opts in. | ||||||
| // Runs after all repo-directory writes (8a, 8a-2) are complete. | ||||||
| // Excludes .git/ so git operations (index.lock, etc.) still work. | ||||||
| if h.ReadonlyRepo { | ||||||
| // -prune skips .git traversal entirely; -not -type l prevents symlink traversal. | ||||||
| // chown ensures the sandbox user (run_as_user) owns .git/ after root-owned extraction. | ||||||
| chmodCmd := fmt.Sprintf( | ||||||
|
ben-alkov marked this conversation as resolved.
|
||||||
| "find %s -path '*/.git' -prune -o -not -type l -exec chmod a-w {} + && chown -R sandbox:sandbox %s/.git && chmod -R u+w %s/.git", | ||||||
|
ben-alkov marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM —
|
||||||
| remoteRepositoryDir, remoteRepositoryDir, remoteRepositoryDir, | ||||||
| ) | ||||||
| if _, stderr, exitCode, err := sandbox.Exec(sandboxName, chmodCmd, 30*time.Second); err != nil { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — 30-second timeout may be insufficient for large reposThis compound command performs Suggestion: Consider increasing to 60s to match, or document the 30s rationale if intentional. Flagged by 2/6 review agents (consensus) |
||||||
| printer.StepFail("Could not make repo read-only: " + err.Error()) | ||||||
| return fmt.Errorf("Read-only repo enforcement failed: %w", err) | ||||||
|
ben-alkov marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — Error message casing inconsistencyThis line uses
Suggested change
Flagged by 3/6 review agents (consensus) |
||||||
| } else if exitCode != 0 { | ||||||
| printer.StepFail("Could not make repo read-only (exit " + fmt.Sprintf("%d", exitCode) + "): " + stderr) | ||||||
| return fmt.Errorf("read-only repo enforcement failed: exit code %d", exitCode) | ||||||
| } | ||||||
| printer.StepDone("Target repo set to read-only") | ||||||
| } | ||||||
|
|
||||||
| // 8d. Host-side scan (Path A): scan the target repo's context files | ||||||
| // (CLAUDE.md, AGENTS.md, SKILL.md, etc.) before the agent processes them. | ||||||
| // The target branch may contain attacker-controlled files from a PR. | ||||||
| if h.SecurityEnabled() { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,23 +33,18 @@ post-script to post. In interactive mode, it posts directly via | |
|
|
||
| ## Sub-agent roster | ||
|
|
||
| Sub-agent definitions live in `sub-agents/` relative to this file. | ||
| Each is a markdown file with frontmatter specifying `name`, `model`, | ||
| and `description`. | ||
|
|
||
| | Sub-agent | Model | Dispatch | Dimension | | ||
| |------------------------|--------|------------|--------------------------------------------------------------------------------| | ||
| | `correctness` | opus | parallel | Logic errors, edge cases, nil handling, API contracts, test adequacy/integrity | | ||
| | `security` | opus | parallel | Auth, data exposure, privilege escalation, injection defense, content security | | ||
| | `intent-coherence` | sonnet | parallel | Authorization, scope, intent authorization tier matching, architectural fit, design coherence | | ||
| | `style-conventions` | sonnet | parallel | Naming, error handling idioms, API shape, code organization | | ||
| | `docs-currency` | sonnet | parallel | Documentation staleness (follows docs-review skill inline) | | ||
| | `cross-repo-contracts` | sonnet | parallel | API contract breakage affecting other repos (conditional) | | ||
| | `challenger` | opus | sequential | Adversarial challenge of findings, false-positive removal, deduplication | | ||
|
|
||
| The Model column reflects each sub-agent's current frontmatter. Any | ||
| value accepted by the Agent tool's `model` parameter is valid in | ||
| sub-agent frontmatter. | ||
| Sub-agent discovery: The sub-agents' definitions are in `sub-agents/` | ||
| relative to this file. | ||
|
|
||
| | Sub-agent | Dispatch | Dimensions | | ||
| |------------------------|------------|-------------------------------------------------------------------------------------------------------------------------| | ||
| | `correctness` | parallel | Logic errors, edge cases, nil handling, API contracts, test adequacy/integrity | | ||
| | `security` | parallel | Security vulnerabilities, auth/access control, data exposure, injection defense, privilege escalation, content security | | ||
| | `intent-coherence` | parallel | Architectural coherence & fit, design coherence, intent alignment, PR scope, scope authorization, tier matching | | ||
| | `style-conventions` | parallel | Repo-specific naming, error-handling idioms, API shape, code organization | | ||
| | `docs-currency` | parallel | Documentation staleness (follows docs-review skill inline) | | ||
| | `cross-repo-contracts` | parallel | API contract breakage affecting other repos (conditional) | | ||
| | `challenger` | sequential | Adversarial challenge of findings, false-positive removal, deduplication | | ||
|
|
||
| ## Findings vs inline comments | ||
|
|
||
|
|
@@ -255,7 +250,8 @@ dimensions are relevant: | |
| #### 3c. Select sub-agents | ||
|
|
||
| Based on the domain classification, select sub-agents for dispatch. | ||
| All selected sub-agents run in parallel. | ||
| All selected sub-agents run in parallel (with the exception of the | ||
| challenger, which runs by itself after all other sub-agents have finished). | ||
|
|
||
| **Dispatch sub-agents based on the classification — typically 3-6.** | ||
| The orchestrator should auto-select which sub-agents are relevant for | ||
|
|
@@ -336,9 +332,7 @@ the constraint first. | |
|
|
||
| For each selected sub-agent: | ||
|
|
||
| 1. Read the sub-agent definition from `sub-agents/{name}.md` | ||
| 2. Extract the `model` from frontmatter | ||
| 3. Compose the spawn prompt from these parts: | ||
| 1. Compose the spawn prompt from: | ||
|
|
||
| **Part 0 — Scope constraint (conditional):** If `scope_constraint` | ||
| from step 3e is not `"none"`, prepend: | ||
|
|
@@ -399,12 +393,8 @@ For each selected sub-agent: | |
| REVIEW_SUB_AGENT_TRUE | ||
| ``` | ||
|
|
||
| 4. Spawn via Agent tool with: | ||
| - `model`: from the sub-agent frontmatter (any value accepted by | ||
| the Agent tool's `model` parameter) | ||
| - `subagent_type`: `Explore` (read-only — sub-agents do not write) | ||
| - `run_in_background`: `true` | ||
| - `prompt`: composed from parts 1–5 | ||
| 2. Spawn the subagents with their `prompt` argument composed from parts | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HIGH — Dispatch instructions no longer specify how to invoke sub-agentsThe old step 4 explicitly said "Spawn via Agent tool with:
Without this, the orchestrator may fabricate its own invocation approach. Suggestion: Change to something like:
Same fix needed for the challenger dispatch in step 6d. Flagged by 3/6 review agents (consensus)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude Code can only spawn subagents using the Agent tool, to which it knows all of the necessary parameters. The YAML frontmatter is consumed by the harness itself (the TypeScript backend). |
||
| 1–5 above | ||
|
ben-alkov marked this conversation as resolved.
|
||
|
|
||
| **All sub-agents MUST be dispatched simultaneously** — include all | ||
| Agent calls in a single message so they run concurrently. This is the | ||
|
|
@@ -520,8 +510,7 @@ fresh context. The challenger has not seen the orchestrator's synthesis | |
| — it receives only the raw findings and the diff, preserving context | ||
| isolation. | ||
|
|
||
| 1. Read `sub-agents/challenger.md` for the sub-agent definition | ||
| 2. Compose the spawn prompt from: | ||
| 1. Compose the spawn prompt from: | ||
|
|
||
| **Part 1 — Sub-agent definition:** the full markdown body of the | ||
| challenger sub-agent file (everything after the frontmatter) | ||
|
|
@@ -555,10 +544,8 @@ isolation. | |
| REVIEW_SUB_AGENT_TRUE | ||
| ``` | ||
|
|
||
| 3. Spawn via Agent tool with: | ||
| - `model`: from the challenger sub-agent frontmatter (`opus`) | ||
| - `subagent_type`: `Explore` (read-only) | ||
| - `prompt`: composed from parts 1–4 | ||
| 2. Spawn the subagents with their `prompt` argument composed from parts | ||
| 1–4 above | ||
|
|
||
| **Prompt size guard:** If the combined context package (findings | ||
| JSON + diff + file list + PR metadata) exceeds 80 000 tokens, | ||
|
|
@@ -571,7 +558,7 @@ isolation. | |
| needs their findings as input), so it is dispatched sequentially, | ||
| not in the parallel batch from step 4. | ||
|
|
||
| 4. Consume the challenger's output. The challenger returns a **different | ||
| 3. Consume the challenger's output. The challenger returns a **different | ||
| format** from dimension sub-agents: an object with | ||
| `adjudicated_findings` and `removed_findings` arrays (not a flat | ||
| finding array). Parse accordingly: | ||
|
|
@@ -583,15 +570,15 @@ isolation. | |
| part of the standard finding schema. | ||
| - If `adjudicated_findings` is empty but the pre-challenger finding | ||
| set was non-empty, treat this as a challenger failure (fall back | ||
| per step 5 below). A legitimate challenger pass that removes all | ||
| findings is unlikely — an empty result more likely indicates a | ||
| parsing error or context truncation. | ||
| per the immediate next step below). A legitimate challenger pass | ||
| that removes all findings is unlikely — an empty result more likely | ||
| indicates a parsing error or context truncation. | ||
| - Otherwise, replace the merged finding set with the challenger's | ||
| `adjudicated_findings`. | ||
| - Log any `removed_findings` for transparency but do not include | ||
| them in the final review. | ||
|
|
||
| 5. If the challenger sub-agent fails (timeout, error, empty | ||
| 4. If the challenger sub-agent fails (timeout, error, empty | ||
| response), fall back to using the pre-challenger merged finding | ||
| set from steps 6a–6c. Record an **info**-level finding: | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.