fix(agents): There are no subagent "types"; they don't compose#2098
fix(agents): There are no subagent "types"; they don't compose#2098ben-alkov wants to merge 2 commits into
Conversation
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
|
a6bfcd5 to
b4d01d4
Compare
|
🤖 Finished Review · ✅ Success · Started 8:59 PM UTC · Completed 9:11 PM UTC |
|
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 |
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! |
|
🤖 Finished Review · ✅ Success · Started 7:36 PM UTC · Completed 7:49 PM UTC |
bb7b589 to
617f74d
Compare
|
🤖 Finished Review · ✅ Success · Started 1:22 PM UTC · Completed 1:39 PM UTC |
|
🤖 Finished Review · ❌ Failure · Started 7:55 PM UTC · Completed 7:56 PM UTC |
(Getting myself out of the way here. I rescind my "block" and defer to Ben and Hector on this one.)
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:55 PM UTC · Completed 7:10 PM UTC |
902966c to
c121a62
Compare
|
🤖 Finished Review · ✅ Success · Started 7:57 PM UTC · Completed 8:11 PM UTC |
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
c121a62 to
550a6f4
Compare
|
🤖 Finished Review · ✅ Success · Started 8:16 PM UTC · Completed 8:41 PM UTC |
Superseded by updated review
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