fix(agents): There are no subagent "types"; they don't compose#2098
Conversation
ben-alkov
force-pushed
the
feat/review-agent-parallel-specialized-subagents
branch
from
61695e7 to
878d86c
Compare
Site previewPreview: https://8bc59259-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 8:30 PM UTC · Completed 8:38 PM UTC |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Low
Labels: PR modifies harness schema (readonly_repo field), sandbox policy (include_workdir), and sub-agent frontmatter definitions Previous run (2)ReviewFindingsLow
Labels: PR modifies skill definitions and sub-agent frontmatter under skills/pr-review/, warranting the component/skills label. Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Previous run (5)ReviewFindingsMedium
Labels: PR modifies harness schema and sandbox policy for review agent write restrictions. Previous run (6)ReviewFindingsMedium
Low
Info
Previous run (7)ReviewFindingsMedium
Low
Info
Previous run (8)ReviewFindingsHigh
Medium
Low
Info
Previous run (9)ReviewFindingsMedium
Low
Info
Previous run (10)ReviewFindingsMedium
Low
Info
Previous run (11)ReviewFindingsMedium
Low
Previous run (12)ReviewFindingsMedium
Low
Previous run (13)ReviewFindingsMedium
Low
Previous run (14)ReviewFindingsMedium
Low
Previous run (15)ReviewFindingsMedium
Low
Previous runReviewFindingsHigh
Low
Previous run (16)ReviewFindingsMedium
Low
Previous run (17)ReviewFindingsHigh
Low
|
![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.
ben-alkov
changed the title
ben-alkov
force-pushed
the
feat/review-agent-parallel-specialized-subagents
branch
2 times, most recently
from
a6bfcd5 to
b4d01d4
Compare
|
🤖 Finished Review · ✅ Success · Started 8:59 PM UTC · Completed 9:11 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Good catch! |
|
@rh-hemartin, @rbean - FWIW, FYI; The In my mind, the likelihood of Claude Code discovering this randomly, during a code review, is miniscule, so, by Occam's Chisel (not as nice/precise as the Razor 😁) it must be part of the system prompt[2][3]. TL;DR - it's not an hallucination. Having said all of that: the security implications are real, and severe. If I can get an answer in PR 192, and the answer is "diffs are the way", then we can stick with Explore type and moot this PR. [1] Summarizing, not quoting verbatim. |
|
Ok, thinking. Explore has the right permission profile (no write tools) but the wrong behavioral disposition (reads excerpts, not whole files). Switching to general-purpose fixes the behavioral problem but opens a privilege escalation surface because Can we solve this at the sandbox layer instead of the agent type layer? Right now filesystem_policy:
include_workdir: true
read_only: [/usr, /lib, /proc, /dev/urandom, /app, /etc, /var/log, /sandbox/workspace/target-repo]
read_write: [/sandbox, /tmp, /dev/null]With that, it wouldn't matter what Then, we could make sub-agents full "general-purpose" agents. Does usage of read_only like that actually work? |
|
EDIT: Experiment at fullsend-ai/experiments/pull/31 tl;dr - No.
🤷 @maruiz93? 🤣 |
|
Try it! :D |
ben-alkov
force-pushed
the
feat/review-agent-parallel-specialized-subagents
branch
from
b4d01d4 to
bb7b589
Compare
|
🤖 Review · ❌ Terminated · Started 7:36 PM UTC · Ended 7:49 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 7:36 PM UTC · Completed 7:49 PM UTC |
ben-alkov
force-pushed
the
feat/review-agent-parallel-specialized-subagents
branch
from
bb7b589 to
617f74d
Compare
|
🤖 Finished Review · ✅ Success · Started 1:22 PM UTC · Completed 1:39 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
ben-alkov
force-pushed
the
feat/review-agent-parallel-specialized-subagents
branch
from
c121a62 to
550a6f4
Compare
|
🤖 Finished Review · ✅ Success · Started 8:16 PM UTC · Completed 8:41 PM UTC |
fullsend-ai-review
Bot
dismissed stale reviews from themself
Superseded by updated review
fullsend-ai-review
Bot
removed
the
requires-manual-review
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
This is an automated comment. We are moving agent content from internal/scaffold/fullsend-repo/ to https://github.com/fullsend-ai/agents -- changes should be made to agent definitions there going forwards.
In working this PR, I had to *carefully* review the Claude Code docs. *This* commit is an outcome of that review. - There is no such thing as a "subagent_type" - it's an hallucination. The only existing fields are listed in https://code.claude.com/docs/en/sub-agents#supported-frontmatter-fields - Agent definitions *are not composable* - The builtin agents (e.g. Explore) are defined in *exactly* the same way as custom agents - There is no mechanism for "subclassing" agents - The recommended way to restrict subagents is using one or both of `tools`/`disallowedTools` - `permissionMode`: `dontAsk` (unintuitively) forbids Claude Code from executing any action *which would normally require user approval* - There is no such thing as "run_in_background" - another hallucination - `prompt` is under-specified: it *does not* exist in the list of frontmatter fields mentioned above, but it *can* be used when invoking the `--agents` command line flag. `initialPrompt` exists, but its description is a little fuzzy. Going conservative here. Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Review agents should not modify the target repository - add harness field `readonly_repo` (boolean) which enforces `chmod -R a-w` on the cloned repo, after copy - tighten the review agent's sandbox policy to exclude workdir writes Assisted-by: Claude Code (Opus 4.6) Signed-off-by: Ben Alkov <ben.alkov@redhat.com> fixup! ffe4628bbe3caed1c6231ae5b662f5d9dc0ef86e
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 6 agents (Claude, Gemini, Codex)
4 findings posted inline (1 HIGH, 3 MEDIUM). 4 false positives removed during verification. 3 additional LOW/INFO findings omitted from inline comments (missing runtime test coverage, trailing period on intent-coherence.md description, inaccurate comment about root-owned extraction).
Assisted-by: Claude (review), Gemini (review), Codex (review)
| - `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 |
There was a problem hiding this comment.
HIGH — Dispatch instructions no longer specify how to invoke sub-agents
The old step 4 explicitly said "Spawn via Agent tool with: model, subagent_type, run_in_background, prompt". The hallucinated params were correctly removed, but the replacement ("Spawn the subagents with their prompt argument composed from parts 1–5 above") doesn't tell the orchestrating LLM:
- To use the Agent tool specifically
- That the sub-agent
.mdfile path should be passed as the agent reference - How
model,tools,permissionMode,backgroundfrom frontmatter get consumed
Without this, the orchestrator may fabricate its own invocation approach.
Suggestion: Change to something like:
Invoke the Agent tool for each sub-agent, referencing
sub-agents/{name}.md. Themodel,tools,permissionMode, andbackgroundfields from frontmatter are consumed automatically by the Agent tool — do not pass them as arguments.
Same fix needed for the challenger dispatch in step 6d.
Flagged by 3/6 review agents (consensus)
There was a problem hiding this comment.
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).
| // -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( | ||
| "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", |
There was a problem hiding this comment.
MEDIUM — && chaining means .git fixup won't run if find fails
If find fails partway (e.g., permission error on a file), the &&-chained chown and chmod on .git will not execute, leaving .git both non-writable and potentially incorrectly owned. Git operations would be completely broken in the sandbox.
Suggestion: Use ; instead of && between the find and the .git fixup commands so .git is always restored:
"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"Alternatively, split into separate sandbox.Exec calls so each step is independently error-checked.
Flagged by 1/6 review agents
| ) | ||
| if _, stderr, exitCode, err := sandbox.Exec(sandboxName, chmodCmd, 30*time.Second); err != nil { | ||
| printer.StepFail("Could not make repo read-only: " + err.Error()) | ||
| return fmt.Errorf("Read-only repo enforcement failed: %w", err) |
There was a problem hiding this comment.
MEDIUM — Error message casing inconsistency
This line uses "Read-only repo enforcement failed: %w" (capital R), while line 1029 uses "read-only repo enforcement failed: exit code %d" (lowercase). Go convention (per go vet and the Go wiki) is that error strings should not be capitalized. The rest of run.go consistently uses lowercase.
| return fmt.Errorf("Read-only repo enforcement failed: %w", err) | |
| return fmt.Errorf("read-only repo enforcement failed: %w", err) |
Flagged by 3/6 review agents (consensus)
| "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", | ||
| remoteRepositoryDir, remoteRepositoryDir, remoteRepositoryDir, | ||
| ) | ||
| if _, stderr, exitCode, err := sandbox.Exec(sandboxName, chmodCmd, 30*time.Second); err != nil { |
There was a problem hiding this comment.
MEDIUM — 30-second timeout may be insufficient for large repos
This compound command performs find traversal + chmod on every non-symlink entry + chown -R + chmod -R on .git/. For large monorepos with deep directory trees (especially on overlay/network filesystems), 30 seconds could be tight. This is a mandatory step — timeout failure terminates the entire agent run. Other comparable filesystem-traversal operations in run.go use 60 seconds.
Suggestion: Consider increasing to 60s to match, or document the 30s rationale if intentional.
Flagged by 2/6 review agents (consensus)
|
🤖 Finished Retro · ✅ Success · Started 7:33 PM UTC · Completed 7:52 PM UTC |
|
PR #2098 was a human-authored fix to remove hallucinated Claude Code sub-agent fields ( Proposals filed
|
4 participants
2nd EDIT:
In working this PR, I had to carefully review the Claude Code docs. This commit is an outcome of that review.
tools/disallowedToolspermissionMode:dontAsk(unintuitively) forbids Claude Code from executing any action which would normally require user approvalpromptis under-specified: it does not exist in the list of frontmatter fields mentioned above, but it can be used when invoking the--agentscommand line flag.initialPromptexists, but its description is a little fuzzy. Going conservative here.EDIT: This change also requires a supporting mechanism for blocking writes to "/sandbox/workspace/target-repo".
After much discussion, we've settled on a harness step which runs
chmod -R a-wimmediately after the copy takes place (obviously an agent could chmod it back, but at least we'd be able to analyze the session trace(s) to know why).The review subagents are being invoked incorrectly;
subagent_typedoesn't exist. It's a complete hallucination