Repository navigation
Description
Found during orchestrate run prd352-20260803-015333 (PRD #352, 12 slices, all passed).
This is a security-model finding, not a convenience one. The plugin's stated safety invariant rests on the subagent tools: restriction. In this run that restriction did not hold.
Symptom
The reviewer-deep spawned for slice #362 reported, unprompted:
"I did have Bash, contrary to the brief, so every claim is a command I ran rather than a re-assertion of the implementer's inventory."
It then produced a git diff -U0 hunk map — fifteen hunks, with the gaps used to prove byte-identical regions — and cited line ranges of uncommitted changes against HEAD. Read/Grep cannot produce that output; it requires shell access to git.
What the definition actually grants
Verified in both the shipped and the installed copies of reviewer-deep.md (plugins/orchestrate/agents/reviewer-deep.md:4 and ~/.claude/plugins/marketplaces/agent-engineering-toolkit/plugins/orchestrate/agents/reviewer-deep.md:4, byte-identical):
tools: Read, Edit, Write, Grep, Glob, mcp__plugin_orchestrate_orchestrate__run_tests,
mcp__plugin_orchestrate_orchestrate__run_typecheck,
mcp__plugin_orchestrate_orchestrate__run_build,
mcp__plugin_orchestrate_orchestrate__run_lint,
mcp__plugin_orchestrate_orchestrate__search_structural
No Bash. No git.
Why this is not cosmetic
SKILL.md:52-54, under Roles and the safety boundary, states it as an invariant:
All four subagents have no Bash and no git access — they are sandboxed to one worktree (the investigator is read-only). Only the orchestrator touches branches, remotes, and the tracker.
That sentence is load-bearing for:
- the single-writer tracker guarantee (orchestrate: orchestrator spine restructure — delegate the slice, map the failure class #362's AC5),
- the claim that a subagent's blast radius is one directory, and
- ADR-0017's whole delegation argument.
If the tools: restriction is not reliably enforced, the invariant is enforced only by prose in the subagent's own instructions — which is precisely the weakness #361's read guard exists to remove for a different boundary.
Evidence the restriction usually DOES hold — and the exact scope of the claim
In the same session, three probe subagents whose definitions declared tools: Write came up with Write plus only the harness's own coordination tools (SendMessage, Task*, advisor) and reported no Bash. So this is intermittent, not a blanket no-op — which makes it harder to notice and worse to rely on.
To keep the claim precise: harness coordination tools appear to be injected regardless of tools:, and that is not what this issue is about. Bash is not a coordination tool. The finding is specifically that a shell — and through it, arbitrary git — reached a subagent whose definition does not list it.
Assessment (stated honestly)
In this instance the leak made the review better: every claim became a command the reviewer ran rather than a re-assertion of the implementer's inventory, and the hunk-map audit is stronger evidence than any grep set.
That is a reason to consider granting the reviewer read-only git deliberately — it is not a reason to leave the enforcement gap unexamined. The two questions are independent:
- Why was the declared
tools:restriction not enforced? — this issue. The security question. - Should the reviewer be granted read-only git on purpose? — a separate design question that should be filed and decided on its own merits, and whose answer does not resolve (1) for the other three roles.
Suggested fix
- Establish reproducibility upstream first. Does a plugin subagent with an explicit
tools:list ever receive a tool it did not list, and under what condition — foreground vs background, fork mode, nesting depth? - Then, plugin-side, decide whether the safety boundary should depend on tool restriction at all, or whether the invariant needs a mechanism with the same properties as the read guard (orchestrate: read guard hook and hook-matcher consistency test #361) — enforced outside the agent's own cooperation.
- Until either lands, the spine must not describe the no-Bash sandbox as a guarantee.
SKILL.md:52-54currently asserts it flatly.
Relationship to existing issues (read before deduping)
- Complements orchestrate: read guard hook and hook-matcher consistency test #361 (read guard hook and hook-matcher consistency test): orchestrate: read guard hook and hook-matcher consistency test #361 hardens a different boundary with a mechanism rather than prose. This finding is the argument that the no-Bash boundary needs the same treatment — it is currently prose plus an unenforced frontmatter field.
- Related to orchestrate: the slice-executor's skills: preload is unverified, and a miss leaves it with no procedure and no error #375 (
skills:preload unverified) and to the registry-refresh issue filed from this run: all three are instances of declared subagent configuration not demonstrably taking effect, with no error. They differ in which field is ignored (tools:,skills:, the body), and this one is the only one with a security consequence. - Not orchestrate: implementer/reviewer subagents assert a non-existent "advisor pass" in result envelopes (false provenance, ADR-0009) #285. orchestrate: implementer/reviewer subagents assert a non-existent "advisor pass" in result envelopes (false provenance, ADR-0009) #285 confirmed the advisor tool is correctly absent and the problem was fabricated prose. This is the inverse: a tool that is genuinely absent from the definition was genuinely present at runtime.
Environment
orchestrate 1.6.0 (plugins/orchestrate/.claude-plugin/plugin.json:3 on development) plus the PRD #352 umbrella branch (slice-executor delegation layer). Run prd352-20260803-015333; slice #362, orchestrate:reviewer-deep. Claude Code on WSL2.
Activity