worker: allowlist spec.tools, and admit the four launcher network tools - #61
Merged
Conversation
`spec.tools` becomes the `--tools` CSV handed to the unattended `vinci -p` agent (run.mjs:1210), and task.mjs validated it for SHAPE ONLY: any non-empty string passed. The launcher registers ~30 extension tools (web_search, web_fetch, web_answer, library_docs, advisor, convene_council, orchestrate, spawn_helper, ...), so any name a spec carried would have been forwarded verbatim. Add SUPPORTED_TOOLS, frozen, containing exactly the seven tools run.mjs already falls back to (read, grep, find, ls, bash, edit, write), and refuse anything else as `tool_unsupported` — the same fail-closed posture the adjacent requiredCapabilities field has against SUPPORTED_CAPABILITIES. This only NARROWS what a work order may request down to what the default already grants; no tool is enabled anywhere, and run.mjs's default string is unchanged. SCOPE: this is hardening of a path not yet in service, not the closure of a live hole. `spec.tools` is populated only by the digest handoff form; the prose envelope form has no `tools` header (HEADER_KEYS), so on the path actually in use `envelope.tools` is undefined and run.mjs falls back to the seven-tool default regardless. The digest path requires a contract registry that production does not configure. The guard becomes load-bearing when that registry is enabled. `tool_unsupported` is deliberately distinct from `tool_not_granted`: containment (within-order.mjs, vendored from vinci-gpu-control's check_within_order) asks whether THIS ORDER granted tool:<name> and runs earlier at step 3.5; this asks whether THIS WORKER supports it at all. Both must pass. Tests: vinci/test/worker-tools-allowlist.mjs (auto-discovered by run.sh's worker-*.mjs glob). Because containment refuses first, every negative fixture uses an order that GRANTS the tool being refused — otherwise the test would pin `execution_exceeds_contract` and never reach this guard. Covers the negative, an ordering control asserting exactly which guard answered, positive reachability for the subset and the full seven, mixed lists in both orders, and the edge inputs, each pinned at the layer that actually decides it (malformed and duplicate entries are refused a layer earlier by digest.mjs). Duplicates take no new rule (digest.mjs already refuses `duplicate_entry`); case matching is exact, so "READ" is refused. Mutation control: with the allowlist loop removed, the negative test fails with "expected a refusal (tool_unsupported), got an envelope" — the spec is ADMITTED, which is the intended assertion failing for the intended reason. Restored from an out-of-repo copy (sha256 verified); 46/46 worker tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid
Step 5 stated the seven-tool list as if it were fixed; it is the fallback run.mjs uses when a spec names no tools, and with SUPPORTED_TOOLS in place a spec may narrow it but not widen it. Say that. The Network Access bullet claimed "no network tools" under a heading about network access, while the clean-room gaps section two hundred lines up says "No network allowlist. The child can reach anything the box can reach." Both are true and together they read as a containment claim that does not exist: `bash` is in the allowlist, so withholding web_fetch removes the attributable path to the network, not access to it. Mark it as a tool boundary explicitly and point at the gap rather than restating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid
Add web_search, web_fetch, web_answer and library_docs to BOTH lists that define the worker's --tools set, as one change, so they stay in lockstep: - run.mjs:1210 default CSV -> 11 entries - task.mjs SUPPORTED_TOOLS -> the same 11, same order The launcher (vinci/bin/vinci) has always registered these unconditionally via vinci/extensions/vinci-search.ts; only the --tools allowlist was hiding them. This is an allowlist edit, not an integration, and it is authorized by the repo owner. The guard has to stay discriminating, so the test moved with it rather than merely going green. Every negative fixture in worker-tools-allowlist.mjs was spelled `web_fetch`; each one would now assert a refusal for an ADMITTED tool and prove nothing. They are re-pointed at `orchestrate` and `spawn_helper` — registered by the launcher, still outside this allowlist — and a new loop asserts each refused example is genuinely absent from SUPPORTED_TOOLS, so a future widening cannot hollow them out the same way. The ordering control is preserved: the work order still GRANTS tool:<name> so containment's `tool_not_granted` (step 3.5) cannot answer first, the refusal code is pinned to exactly `tool_unsupported`, and the granted/ungranted pair is now run on a second name so it is not a property of one string. New positive-reachability cases prove each of the four is now ADMITTED through the same entry point (materializeEnvelope) and carried into the envelope. Their names are LITERALS, not `[...SUPPORTED_TOOLS]`: a case that iterates the list under test can only agree with it and would stay green if the four were removed again. Mutation control: reverting the four from SUPPORTED_TOOLS only, leaving run.mjs at 11, fails the cross-file lockstep pin, and (driven past it) fails the new positive cases with materializeEnvelope refusing web_search as `tool_unsupported`. Restored and verified byte-identical by sha256 against an out-of-repo copy. README.md's claim that none of the network tools are in the allowlist is now false and is corrected; the adjacent point is kept and sharpened — this is a TOOL boundary and never was a network boundary, since `bash` is in the set and there is no egress allowlist, so admitting these adds an attributable path rather than a capability the child lacked. lease.mjs's declaration comment listing the fixed spawn tool set is updated for the same reason. Worker test group: 46/46 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid
The lockstep pin between task.mjs's SUPPORTED_TOOLS and run.mjs's `--tools`
fallback was carried by `runSource.includes(`"${DEFAULT_CSV}"`)`. `includes`
is satisfied by the literal appearing anywhere in the file, a comment
included, so it pinned a string in a file rather than the value the worker
hands to the agent.
Executed mutation: reverting the live fallback at run.mjs:1210 to the old
seven-tool CSV while leaving the correct eleven-tool literal in a comment
above `runVinci` left the suite printing PASS. That is the value production
actually uses — the prose-envelope form carries no `tools` header, so the
fallback branch is taken on every prose task.
Replaced with a behavioural pin. `resolveBin("vinci")` is a bare PATH scan at
spawn time, so a stub `vinci` first on PATH is the executable `runVinci`
really launches; the stub records its argv and the assertions read that.
A. fallback — no `tools` on the envelope: --tools is the eleven-tool CSV,
and its split deep-equals SUPPORTED_TOOLS.
B. narrowing — `tools: ["read","bash"]`: --tools is exactly "read,bash".
Its value differs from A's, which is the instrument's
positive control: the recording tracks the envelope rather
than echoing a constant.
C/D. one tool, the explicit full set, and five degenerate `tools` fields
([], null, undefined, string, object) that must all fall back rather
than launch an empty or malformed CSV.
This also gives `runVinci` its first test of any kind — it was imported by
zero tests repo-wide, so the parameterisation path had no coverage at all.
The source-text check survives only as a clearly-labelled secondary smoke,
now anchored on `const tools =` so a comment cannot satisfy it.
Controls: mutation 1 (7-tool live CSV + 11-tool comment) fails case A on the
recorded argv while the old assertion still evaluates true; mutation 2 (drop
the narrowing branch) fails case B and leaves A passing, so the two cases
discriminate different mechanisms. run.mjs restored from an out-of-repo copy,
sha256 56788c6d3a52ffec1d67699af27c197608f0e4dd9db752221c7d900ffee71402
before and after. Worker group 46/46, unchanged from baseline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid
Independent review flagged that "on the repo owner's authorization" appeared three times inside this diff as prose the author typed, with nothing outside the diff corroborating it — while being the sole justification for the one change that alters live fleet behaviour. Citing the artifact that needs the authorization is not evidence. Both in-code claims now cite the decision recorded on the bus (msg_de1a219d, corrected by msg_02bb0a87), which is external, timestamped, addressed to the room, and can be repudiated by the person who made it. The commit message for the widening itself (d1563bb) carries the same wording and is left alone rather than rewritten, since rewriting history to improve my own citation would defeat the point. No behavioural change: comments and documentation only. Worker group still 46/46. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Read this as a widening with a dormant guard attached — not as hardening
The commit subjects say "refuse ... outside the allowlist", which is true but is not the part that takes effect. Stating the net effect plainly, because independent review found the subjects mislead a skimming reviewer:
On merge and deploy, the fleet gets strictly more child-agent capability and no new restriction in force.
HEADER_KEYS(vinci/worker/task.mjs) has notoolsentry, so the prose-envelope path — the only path in service, 769 of 769 real dispatches — always falls through torun.mjs's default CSV. This PR moves that default from 7 tools to 11.SUPPORTED_TOOLSrefusal is reachable only viamaterializeEnvelope, i.e. the digest/pinned-contract path, which requires a contract registry production does not configure (VGC_CONTRACT_REGISTRY_DIRunset; the live server returns the "no contract registry" 404 branch).So the guard is correct and pre-positions for the registry being enabled, but it restricts nothing today. Merge on the widening's merits.
What changed
SUPPORTED_TOOLS— a frozen allowlist intask.mjs, refusing anyspec.toolsentry outside it astool_unsupported. Before this,spec.toolswas validated for shape only: any non-empty string passed, so a spec could name any tool the launcher registers. The adjacentrequiredCapabilitiesfield was already fail-closed against a frozen list; this givestoolsthe same posture.web_search,web_fetch,web_answer,library_docs) admitted to both lists together —run.mjs's default andSUPPORTED_TOOLS— which must stay in lockstep.README.mdline claiming no network tool was in the allowlist and alease.mjscomment carrying a stale copy of the seven-tool CSV.tool_unsupportedis deliberately nottool_not_granted. Containment (checkValidatedExecutionSpecWithinOrder) asks whether this work order granted the tool; this list asks whether this worker supports it at all. Different questions, different codes, both must pass.Authorization
Recorded on the bus as
msg_de1a219d(corrected bymsg_02bb0a87) — external, timestamped, repudiable. An earlier revision asserted the authorization in code comments; review correctly called that self-attestation, and56f79f9dreplaces it with the citation.The stated rationale is worth reading in full, because the premise it was first offered on does not hold: only
web_searchandweb_answerare Brave-backed,library_docsis Context7, andweb_fetchis a direct arbitrary-URL fetch. The change rests on a different argument — the unattended child already holdsbashwith no egress allowlist, so admittingweb_fetchadds an attributable, SSRF-guarded path to reach it already had. It does not add reach.Evidence
Every guard assertion was mutation-tested; the ones worth naming:
runSource.includes(csv)— a whole-file substring check that a comment satisfies. Confirmed by executing the decoy: revertingrun.mjs's live fallback to the old 7 tools while leaving the correct 11-tool literal in a comment left the suite printingPASS.b903c0eereplaces it with a test that puts a stubvincionPATHand asserts the argvrunVinciactually spawns. The identical decoy now fails on the intended assertion, printing the real spawned--toolsvalue. This also givesrunVinciits first test of any kind — it was imported by zero tests repo-wide.tool_not_grantedat step 3.5, before this guard. Every negative fixture therefore uses a work order that grants the tool it then refuses, and asserts the code is exactlytool_unsupportedand explicitly not any earlier guard's — otherwise the test would never reach the mechanism it claims to cover.web_fetch(now admitted) ontoorchestrate/spawn_helper, plus a loop asserting each refused example is absent fromSUPPORTED_TOOLS, so a future widening cannot silently hollow them out.Review
Two independent lanes reviewed this and disagreed. One returned
PASS / FINDINGS: NONEand specifically certified the lockstep pin as sound without ever running a divergence. The other returned GO-WITH-FIXES and ran ten mutations, one of which left the suite green. I settled it by executing the mutation myself: the second lane was right. The fixes inb903c0eeand56f79f9dare the result.Not in scope
Issue #60 — two confirmed pre-existing SSRF bypasses in
web_fetch's guard (IPv4-mapped IPv6 branch is dead code after WHATWG URL normalization; redirects followed with no per-hop revalidation). Both reproduced against the real predicate. They are not introduced here and do not gate this PR: low marginal risk while the child holdsbash, but genuinely exploitable on any surface shippingweb_fetchwithout it.🤖 Generated with Claude Code
https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid