From 28cc827ad7ec2e6f38440d74fe462b21ee8c1ffa Mon Sep 17 00:00:00 2001 From: George Pu <19347973+thegeorgepu@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:02:26 -0400 Subject: [PATCH 1/5] worker: refuse execution-spec tools outside the worker's allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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: 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) Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid --- vinci/test/worker-tools-allowlist.mjs | 278 ++++++++++++++++++++++++++ vinci/worker/task.mjs | 38 ++++ 2 files changed, 316 insertions(+) create mode 100644 vinci/test/worker-tools-allowlist.mjs diff --git a/vinci/test/worker-tools-allowlist.mjs b/vinci/test/worker-tools-allowlist.mjs new file mode 100644 index 000000000..6afef67e3 --- /dev/null +++ b/vinci/test/worker-tools-allowlist.mjs @@ -0,0 +1,278 @@ +// TOOL ALLOWLIST: an execution spec may name only tools this worker advertises. +// +// `spec.tools` becomes the `--tools` CSV handed to the unattended `vinci -p` agent +// (run.mjs:1210). It was validated for SHAPE ONLY — a list of non-empty strings — so any string +// passed, and the launcher (vinci/bin/vinci) unconditionally registers ~30 extension tools +// (web_search, web_fetch, web_answer, library_docs, advisor, convene_council, orchestrate, +// spawn_helper, …) that a spec could therefore have named. task.mjs now holds SUPPORTED_TOOLS — +// exactly the seven tools run.mjs already falls back to — and refuses anything else as +// `tool_unsupported`, the same fail-closed posture the adjacent `requiredCapabilities` field has +// had against SUPPORTED_CAPABILITIES. +// +// 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), and the digest path needs a contract registry that production does not set. The +// suite is written so it keeps its meaning when that registry IS enabled. +// +// `tool_unsupported` is deliberately NOT `tool_not_granted`. The latter belongs to containment +// (within-order.mjs, vendored from vinci-gpu-control's `check_within_order`) and means "this +// work order did not grant it"; this one means "this worker does not support it, whatever was +// granted". The ordering control below turns that distinction into an executed assertion. +// +// ORDERING IS THE WHOLE DIFFICULTY HERE. `spec.tools` is validated LAST, in step 4 of +// materializeEnvelope. Four separate guards answer before it, and a fixture that trips any of +// them tests that guard instead of this one: +// 1. digest.mjs validateExecutionSpec — unknown fields, malformed/duplicate tool entries +// 2. binding (step 3) — workOrderId / workOrderDigest +// 3. containment (step 3.5) — `tool_not_granted`: the ORDER must grant tool: +// 4. step 4's earlier fields — repository, modelClass, branches, bounds, output, +// promotion, evidence +// Guard 3 is the sharp one and is NOT optional to clear: to reach this allowlist at all, the +// work order must GRANT the very tool we then refuse. That is the real threat model — a work +// order that grants `tool:web_fetch` must still not get web_fetch out of this worker. Every +// assertion below pins the EXACT reason code, and the ordering control proves which guard +// answered. +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { executionSpecDigest, workOrderDigest } from "../worker/contracts/digest.mjs"; +import { materializeEnvelope, SUPPORTED_TOOLS } from "../worker/task.mjs"; + +const VECTORS = join(dirname(fileURLToPath(import.meta.url)), "fixtures/contract-vectors"); +const workOrder = JSON.parse(readFileSync(join(VECTORS, "work-order-1-minimal", "input.json"), "utf8")); +const baseSpec = JSON.parse(readFileSync(join(VECTORS, "execution-spec-1-minimal", "input.json"), "utf8")); + +// The non-tool grants the golden order carries. Every order below keeps these and varies ONLY +// the tool: grants, so containment can never refuse for a reason this suite is not about. +const NON_TOOL_GRANTS = workOrder.grantedAuthority.filter((g) => !g.startsWith("tool:")); + +// An order identical to the golden one except that it grants exactly `tools`. Granting the tool +// is what clears containment (guard 3) and lets execution reach the allowlist. +const orderGranting = (tools, overrides = {}) => ({ + ...JSON.parse(JSON.stringify(workOrder)), + grantedAuthority: [...NON_TOOL_GRANTS, ...tools.map((t) => `tool:${t}`)], + ...overrides, +}); +// A spec bound to `order`. `requiredCapabilities: []` because the golden vector asks for two the +// worker does not advertise, and `capability_unsupported` is the guard immediately AFTER this +// one — leaving the vector's value in place would let it mask a missing refusal here. +const specFor = (order, overrides = {}) => ({ + ...JSON.parse(JSON.stringify(baseSpec)), + workOrderId: order.id, + workOrderDigest: workOrderDigest(order), + requiredCapabilities: [], + ...overrides, +}); +const materialize = (order, spec) => + materializeEnvelope( + { work_order_id: order.id, contract_digest: workOrderDigest(order), execution_spec_digest: executionSpecDigest(spec) }, + { work_order: order, execution_spec: spec }, + { modelClasses: { forte: { provider: "vinci", model: "forte" } }, modelClassesConfigured: true }, + ); +// Materializing must throw a HandoffRefusal whose `.code` is EXACTLY `code` and whose message +// contains every string in `names`. Asserting the code (not merely "it threw") is what makes +// these tests reach the mechanism instead of an earlier guard. +function refuses(order, spec, code, names, label) { + let thrown = null; + try { + materialize(order, spec); + } catch (error) { + thrown = error; + } + assert.ok(thrown, `${label}: expected a refusal (${code}), got an envelope`); + assert.equal(thrown.code, code, `${label}: refusal code (message was: ${thrown.message})`); + for (const name of names) { + assert.ok(thrown.message.includes(name), `${label}: the reason names ${JSON.stringify(name)}: ${thrown.message}`); + } + return thrown; +} + +// --- the allowlist IS the run.mjs default, neither more nor less ---------------------------- +// This change restricts what a spec may REQUEST down to what run.mjs already grants by default; +// it must not enable anything. Pinned against the literal fallback string in run.mjs:1210 so the +// two cannot drift apart silently in either direction. +{ + const DEFAULT_CSV = "read,grep,find,ls,bash,edit,write"; + assert.deepEqual([...SUPPORTED_TOOLS], DEFAULT_CSV.split(","), "SUPPORTED_TOOLS is exactly run.mjs's default --tools set"); + assert.equal(SUPPORTED_TOOLS.length, 7, "seven tools, nothing more"); + assert.ok(Object.isFrozen(SUPPORTED_TOOLS), "the allowlist is frozen"); + const runSource = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "../worker/run.mjs"), "utf8"); + assert.ok(runSource.includes(`"${DEFAULT_CSV}"`), "run.mjs still carries the default this list mirrors, unchanged"); +} + +// --- precondition: the fixture materializes when the tool IS advertised --------------------- +// Every refusal below must be caused by the one thing that case changes, not by a fixture that +// never passed in the first place. +{ + const order = orderGranting(["read", "edit", "bash"]); + const materialized = materialize(order, specFor(order)); + assert.equal(materialized.envelope.branch, "feat/vector-1", "the golden pair still materializes"); + assert.deepEqual(materialized.envelope.tools, ["read", "edit", "bash"], "and carries its tools through"); +} + +// --- 1. NEGATIVE: an extension tool is refused ---------------------------------------------- +// Valid in every other field. The order GRANTS tool:web_fetch, so containment is satisfied and +// the worker's own allowlist is the only thing standing between this spec and an unattended +// agent with network fetch. materializeEnvelope is the pre-clone boundary: worker.mjs binds its +// result at :791 and only then reaches prepareRepository (:1208) and runVinci (:1293), so a +// throw here means nothing was cloned and nothing was spawned. +{ + const order = orderGranting(["web_fetch"]); + const spec = specFor(order, { tools: ["web_fetch"] }); + const thrown = refuses(order, spec, "tool_unsupported", ['"web_fetch"', "read, grep, find, ls, bash, edit, write"], "web_fetch alone"); + assert.match(thrown.message, /^tool_unsupported: tool "web_fetch" is not supported by this worker \(supported: /); +} + +// Every extension tool the launcher registers, not just the one in the headline case. +for (const tool of ["web_search", "web_fetch", "web_answer", "library_docs", "advisor", "convene_council", "orchestrate", "spawn_helper"]) { + const order = orderGranting([tool]); + refuses(order, specFor(order, { tools: [tool] }), "tool_unsupported", [JSON.stringify(tool)], `extension tool ${tool}`); +} + +// --- 2. ORDERING CONTROL: prove WHICH guard answered ---------------------------------------- +// (a) Without the tool: grant, containment (guard 3) answers FIRST and the allowlist is never +// reached. This is the hazard made visible — a fixture written without the grant would have +// gone green on `execution_exceeds_contract` and proved nothing about this change. +{ + const order = orderGranting(["read"]); // deliberately does NOT grant tool:web_fetch + refuses(order, specFor(order, { tools: ["web_fetch"] }), "execution_exceeds_contract", ["tool_not_granted", "/tools/0"], "ungranted web_fetch"); +} +// (b) With the grant, the SAME spec gets a different code. The pair is the discriminator: the +// only thing that changed is the order's grant, and the answering guard moved from containment +// to the allowlist. Nothing but the new check can produce `tool_unsupported`. +{ + const granted = orderGranting(["web_fetch"]); + const thrown = refuses(granted, specFor(granted, { tools: ["web_fetch"] }), "tool_unsupported", ["web_fetch"], "granted web_fetch"); + for (const earlier of ["unknown_field", "invalid_spec_field", "no_tools", "capability_unsupported", "execution_exceeds_contract", "binding_mismatch"]) { + assert.notEqual(thrown.code, earlier, `an earlier guard must not be what answered (${earlier})`); + } +} + +// --- 3. POSITIVE REACHABILITY: the guarded operation still works ----------------------------- +// Same entry point, same fixture shape. If these fail, the guard is refusing work it must admit +// and the negative cases above prove nothing. +{ + const order = orderGranting(["read", "bash"]); + const materialized = materialize(order, specFor(order, { tools: ["read", "bash"] })); + assert.deepEqual(materialized.envelope.tools, ["read", "bash"], "a narrowed subset materializes"); +} +{ + // The full default set — the exact seven run.mjs would have used anyway. Behaviour for a spec + // using the default MUST be unchanged by this commit. + const order = orderGranting([...SUPPORTED_TOOLS]); + const materialized = materialize(order, specFor(order, { tools: [...SUPPORTED_TOOLS] })); + assert.deepEqual(materialized.envelope.tools, ["read", "grep", "find", "ls", "bash", "edit", "write"], "the whole default set materializes"); +} +// …and each of the seven on its own, so a typo in the list cannot hide behind its neighbours. +for (const tool of SUPPORTED_TOOLS) { + const order = orderGranting([tool]); + const materialized = materialize(order, specFor(order, { tools: [tool] })); + assert.deepEqual(materialized.envelope.tools, [tool], `${tool} alone is admitted`); +} + +// --- 4. MIXED: one disallowed entry poisons the list ---------------------------------------- +// The refusal must name the OFFENDING entry, not the first entry, and must not quietly drop it +// and run with the rest. +{ + const order = orderGranting(["read", "web_fetch"]); + const thrown = refuses(order, specFor(order, { tools: ["read", "web_fetch"] }), "tool_unsupported", ['"web_fetch"'], "read + web_fetch"); + assert.ok(!thrown.message.includes('tool "read"'), `the reason names the offending entry, not the admitted one: ${thrown.message}`); +} +{ + // …and in the other order, so the check is a scan and not a look at tools[tools.length - 1]. + const order = orderGranting(["web_fetch", "read"]); + refuses(order, specFor(order, { tools: ["web_fetch", "read"] }), "tool_unsupported", ['"web_fetch"'], "web_fetch + read"); +} + +// --- 5. EDGE INPUTS --------------------------------------------------------------------------- +// Each case records WHICH layer answers. Several are answered by digest.mjs's +// validateExecutionSpec before materializeEnvelope ever selects the spec — that is correct +// fail-closed behaviour, but it means those inputs do NOT exercise the new allowlist, and this +// suite says so rather than letting a green tick imply otherwise. + +// [] — unchanged: the empty list keeps its own reason code. The allowlist has nothing to reject +// (the loop body never runs), so `no_tools` must still be what answers. +{ + const order = orderGranting(["read"]); + refuses(order, specFor(order, { tools: [] }), "no_tools", ["tools is empty"], "empty tools"); +} + +// Non-array, empty-string entry, numeric entry, null entry — refused by digest.mjs one layer +// earlier, so a spec carrying them can never be selected and never reaches step 4. Asserted at +// that layer, where they actually happen. +for (const [label, tools, pattern] of [ + ["non-array", "read", /\/tools invalid_type/], + ["empty-string entry", [""], /\/tools\/0 invalid_tool/], + ["numeric entry", [42], /\/tools\/0 invalid_tool/], + ["null entry", [null], /\/tools\/0 invalid_tool/], + ["boolean entry", [true], /\/tools\/0 invalid_tool/], + ["nested-array entry", [["read"]], /\/tools\/0 invalid_tool/], + ["whitespace-only entry", [" "], /\/tools\/0 invalid_tool/], +]) { + const order = orderGranting(["read"]); + const spec = { ...specFor(order), tools }; + assert.throws(() => executionSpecDigest(spec), pattern, `${label}: refused by validateExecutionSpec, before the allowlist`); +} +// …and the same malformed spec is refused as `invalid_execution_spec` through the real entry +// point too, so this is not merely a property of the digest helper called in isolation. +{ + const order = orderGranting(["read"]); + const good = specFor(order); + const bad = { ...good, tools: [42] }; + let thrown = null; + try { + materializeEnvelope( + { work_order_id: order.id, contract_digest: workOrderDigest(order), execution_spec_digest: executionSpecDigest(good) }, + { work_order: order, execution_spec: bad }, + { modelClasses: { forte: { provider: "vinci", model: "forte" } }, modelClassesConfigured: true }, + ); + } catch (error) { + thrown = error; + } + assert.ok(thrown, "a malformed tools entry refuses through materializeEnvelope"); + assert.equal(thrown.code, "invalid_execution_spec", `malformed entries are refused at spec validation: ${thrown.message}`); +} + +// DUPLICATES — ["read","read"]. DELIBERATE CHOICE: the allowlist does NOT add a uniqueness rule. +// digest.mjs's validateStringList already refuses a repeated entry as `duplicate_entry` before a +// spec is ever selected, so a uniqueness check in task.mjs could never fire through the handoff +// path — it would be an unreachable guard, and unreachable guards read as coverage while +// protecting nothing. The allowlist is a pure membership test: it answers "may this worker run +// this tool", and asking it twice has the same answer. Pinned at the layer that does decide. +{ + const order = orderGranting(["read"]); + const spec = { ...specFor(order), tools: ["read", "read"] }; + assert.throws(() => executionSpecDigest(spec), /\/tools\/1 duplicate_entry/, "duplicates are refused one layer earlier, as duplicate_entry"); +} +// The membership test itself is duplicate-blind, which is the property the choice above rests +// on: if that upstream rule were ever relaxed, a repeated ADMITTED tool would still be admitted +// and a repeated REFUSED tool would still be refused. +{ + assert.equal(SUPPORTED_TOOLS.includes("read"), true); + assert.equal(SUPPORTED_TOOLS.includes("web_fetch"), false); +} + +// CASE — ["READ"]. DELIBERATE CHOICE: matching is EXACT and case-sensitive, so "READ" is +// refused. Two reasons. (1) The `tool:` grant grammar in within-order.mjs is documented +// case-sensitive, and a lenient allowlist beside a strict grant check would mean the two layers +// disagree about what a tool name is. (2) `--tools` receives the spec's spelling verbatim +// (run.mjs joins the list unchanged); admitting "READ" here would forward a name that the agent +// resolves by its own rules — the allowlist would have approved a string it did not check. +// Refusing is the fail-closed reading, and it can only ever reject work, never enable any. +{ + const order = orderGranting(["READ"]); // granted, so containment is not what answers + refuses(order, specFor(order, { tools: ["READ"] }), "tool_unsupported", ['"READ"'], "uppercase READ"); +} +for (const variant of ["Read", "rEaD", "WRITE", "Bash"]) { + const order = orderGranting([variant]); + refuses(order, specFor(order, { tools: [variant] }), "tool_unsupported", [JSON.stringify(variant)], `case variant ${variant}`); +} +// Near-misses that are not case: padding and separators are not normalised away either. +for (const variant of [" read", "read ", "read,bash", "read/../bash"]) { + const order = orderGranting([variant]); + refuses(order, specFor(order, { tools: [variant] }), "tool_unsupported", [JSON.stringify(variant)], `near-miss ${variant}`); +} + +console.log("PASS worker-tools-allowlist"); diff --git a/vinci/worker/task.mjs b/vinci/worker/task.mjs index b224db5ff..38990a5ec 100644 --- a/vinci/worker/task.mjs +++ b/vinci/worker/task.mjs @@ -188,6 +188,33 @@ export const MODEL_CLASSES = DEFAULT_MODEL_CLASSES; // rather than silently unfulfilled. Grow this list only when the worker actually provides one. export const SUPPORTED_CAPABILITIES = Object.freeze([]); +// The closed set of tools this worker will hand an unattended `vinci -p` agent. It is EXACTLY the +// default allowlist run.mjs falls back to when a spec names none ("read,grep,find,ls,bash,edit, +// write"), so a work order may NARROW what the agent may call and never widen it. Same posture as +// SUPPORTED_CAPABILITIES above: anything this worker does not advertise is BLOCKED +// (`tool_unsupported`), never silently dropped. Grow this list only together with the run.mjs +// default it mirrors. +// +// SCOPE — this is hardening, not the closure of a live hole. `spec.tools` reaches run.mjs only +// through the DIGEST handoff form (materializeEnvelope populates `envelope.tools`); the prose +// envelope form has no `tools` header at all (see HEADER_KEYS above), so on the prose path +// `envelope.tools` is undefined and run.mjs falls back to the seven-tool default regardless. The +// digest path needs a contract registry that is not configured in production, so nothing has been +// dispatched through this field. It becomes load-bearing the moment that registry is enabled — +// which is exactly when it is too late to add. Before this list, `tools` was validated for shape +// only, and the launcher (vinci/bin/vinci) unconditionally registers ~30 extension tools +// (web_search, web_fetch, web_answer, library_docs, advisor, convene_council, orchestrate, +// spawn_helper), so any string a spec named would have been forwarded verbatim to `--tools`. +// +// NOT the same check as `tool_not_granted`, and it does not supersede it. Containment +// (contracts/within-order.mjs, the vendored port of vinci-gpu-control's +// `authority/vinci_gpu_authority/contracts.py::check_within_order`) asks whether THIS WORK ORDER +// granted `tool:`; it runs earlier, at step 3.5, and refuses as `tool_not_granted`. This +// list asks whether THIS WORKER supports the tool at all — a question no work order can answer, +// and one that must still refuse when a registry grants something the worker should not run. Two +// different questions, two different reason codes, deliberately. Both must pass. +export const SUPPORTED_TOOLS = Object.freeze(["read", "grep", "find", "ls", "bash", "edit", "write"]); + // B2: read the operator model-class table from VINCI_WORKER_MODEL_CLASSES. The value is a JSON // object `{ : { provider, model } }`, or `@` naming a JSON file (parsed // identically; `@/foo.json` reads `/foo.json`). Unset => `{ configured: false, table: @@ -398,6 +425,17 @@ export function materializeEnvelope(triple, registry, opts = {}) { if (!Array.isArray(spec.tools)) refuse("invalid_spec_field", "tools is a list of tool names"); if (spec.tools.length === 0) refuse("no_tools", "tools is empty; at least one tool is required"); if (!spec.tools.every((t) => typeof t === "string" && t.length > 0)) refuse("invalid_spec_field", "tools entries are non-empty tool names"); + // …and every entry must be a tool this worker advertises (SUPPORTED_TOOLS) — one unsupported + // entry BLOCKs the task rather than being dropped from the list. Comparison is EXACT and + // case-sensitive, matching both the case-sensitive `tool:` grant grammar in + // within-order.mjs and the CSV run.mjs hands to `--tools`; "READ" is not "read". Duplicates are + // deliberately NOT this check's business: digest.mjs already refuses a repeated entry as + // `duplicate_entry` before a spec is ever selected, so a uniqueness rule here could never fire. + for (const tool of spec.tools) { + if (!SUPPORTED_TOOLS.includes(tool)) { + refuse("tool_unsupported", `tool ${JSON.stringify(tool)} is not supported by this worker (supported: ${SUPPORTED_TOOLS.join(", ") || "none"})`); + } + } const tools = [...spec.tools]; // inputArtifacts (list): recorded as-is (no fetch in Wave 1B scope). if (spec.inputArtifacts !== undefined && !Array.isArray(spec.inputArtifacts)) refuse("invalid_spec_field", "inputArtifacts is a list"); From 6b7df7b55127685c2a5d5fc861c1d54f684302e5 Mon Sep 17 00:00:00 2001 From: George Pu <19347973+thegeorgepu@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:04:28 -0400 Subject: [PATCH 2/5] docs(worker): correct two claims the tools allowlist changes 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) Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid --- vinci/worker/README.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/vinci/worker/README.md b/vinci/worker/README.md index 25610cfe9..8806bbc02 100644 --- a/vinci/worker/README.md +++ b/vinci/worker/README.md @@ -59,7 +59,11 @@ WantedBy=multi-user.target 2. **Parse**: Task envelope with headers (repo, evidence, provider, model, budget, timeout, deadline, ref) 3. **Claim**: POST status "claimed attempt N" to bus 4. **Setup**: Clone/fetch the repo and reuse or create `worker/` from `origin/main` -5. **Run**: Spawn `vinci -p --session-id --tools read,grep,find,ls,bash,edit,write ""` +5. **Run**: Spawn `vinci -p --session-id --tools ""`, where `` is + the execution spec's `tools` when the digest form supplies one and + `read,grep,find,ls,bash,edit,write` otherwise. A spec may NARROW that set; it cannot widen + it, because `SUPPORTED_TOOLS` in `task.mjs` refuses anything outside it as + `tool_unsupported`. 6. **Limits**: - `max_runtime_s`: SIGTERM then SIGKILL after 30s - `budget_usd`: Poll session JSONL every 15s; kill if cost exceeds budget @@ -766,7 +770,14 @@ existing worker suite with the flag off. - Outbound HTTPS to bus (`--server`) - Outbound HTTPS to GitHub (clone, fetch, push, PR operations) - NO inbound network required -- Runs with `--tools read,grep,find,ls,bash,edit,write` only (no network tools) +- Runs with at most `--tools read,grep,find,ls,bash,edit,write` (enforced by `SUPPORTED_TOOLS` + in `task.mjs`; a spec may ask for fewer, never more). None of the launcher's network tools + — `web_search`, `web_fetch`, `web_answer`, `library_docs` — are in that set, though the + launcher does register them. +- 🔴 That is a TOOL boundary, not a network boundary. `bash` is in the set and there is no + egress allowlist (see "No network allowlist" under the clean-room gaps above), so the child + can still reach anything the box can reach. Withholding the network tools removes the + attributable path, not the capability. ## See Also From d1563bb6c610a5f8d424e89752ae8bb1970e11f2 Mon Sep 17 00:00:00 2001 From: George Pu <19347973+thegeorgepu@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:59:05 -0400 Subject: [PATCH 3/5] worker: admit the four launcher network tools to the tools allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 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) Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid --- vinci/test/worker-tools-allowlist.mjs | 136 +++++++++++++++++++------- vinci/worker/README.md | 23 +++-- vinci/worker/lease.mjs | 3 +- vinci/worker/run.mjs | 2 +- vinci/worker/task.mjs | 34 +++++-- 5 files changed, 145 insertions(+), 53 deletions(-) diff --git a/vinci/test/worker-tools-allowlist.mjs b/vinci/test/worker-tools-allowlist.mjs index 6afef67e3..a5ae46490 100644 --- a/vinci/test/worker-tools-allowlist.mjs +++ b/vinci/test/worker-tools-allowlist.mjs @@ -3,11 +3,19 @@ // `spec.tools` becomes the `--tools` CSV handed to the unattended `vinci -p` agent // (run.mjs:1210). It was validated for SHAPE ONLY — a list of non-empty strings — so any string // passed, and the launcher (vinci/bin/vinci) unconditionally registers ~30 extension tools -// (web_search, web_fetch, web_answer, library_docs, advisor, convene_council, orchestrate, -// spawn_helper, …) that a spec could therefore have named. task.mjs now holds SUPPORTED_TOOLS — -// exactly the seven tools run.mjs already falls back to — and refuses anything else as -// `tool_unsupported`, the same fail-closed posture the adjacent `requiredCapabilities` field has -// had against SUPPORTED_CAPABILITIES. +// (advisor, convene_council, orchestrate, spawn_helper, …) that a spec could therefore have +// named. task.mjs now holds SUPPORTED_TOOLS — exactly the eleven tools run.mjs already falls +// back to — and refuses anything else as `tool_unsupported`, the same fail-closed posture the +// adjacent `requiredCapabilities` field has had against SUPPORTED_CAPABILITIES. +// +// 🔴 THE ALLOWLIST GREW, SO THE NEGATIVE FIXTURES MOVED. The four network tools (`web_search`, +// `web_fetch`, `web_answer`, `library_docs`) were added to BOTH lists together, on the repo +// owner's authorization. Every negative case in this file used to be spelled `web_fetch`; each +// one would now be VACUOUS — asserting a refusal for a tool that is admitted — so they are all +// re-pointed at `orchestrate` and `spawn_helper`, which the launcher registers and which this +// worker still does NOT advertise. The membership probes at the bottom of §5 assert both +// directions (a newly-admitted tool is in, the refused example is out) so that a future +// widening cannot quietly hollow these cases out the same way again. // // 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 @@ -29,7 +37,7 @@ // promotion, evidence // Guard 3 is the sharp one and is NOT optional to clear: to reach this allowlist at all, the // work order must GRANT the very tool we then refuse. That is the real threat model — a work -// order that grants `tool:web_fetch` must still not get web_fetch out of this worker. Every +// order that grants `tool:orchestrate` must still not get orchestrate out of this worker. Every // assertion below pins the EXACT reason code, and the ordering control proves which guard // answered. import assert from "node:assert/strict"; @@ -90,13 +98,14 @@ function refuses(order, spec, code, names, label) { } // --- the allowlist IS the run.mjs default, neither more nor less ---------------------------- -// This change restricts what a spec may REQUEST down to what run.mjs already grants by default; -// it must not enable anything. Pinned against the literal fallback string in run.mjs:1210 so the -// two cannot drift apart silently in either direction. +// This restricts what a spec may REQUEST down to what run.mjs already grants by default; it must +// not enable anything beyond that default. Pinned against the literal fallback string in +// run.mjs:1210 so the two cannot drift apart silently in either direction — this is the LOCKSTEP +// pin, and it is what fails if the four network tools are added to one list and not the other. { - const DEFAULT_CSV = "read,grep,find,ls,bash,edit,write"; + const DEFAULT_CSV = "read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs"; assert.deepEqual([...SUPPORTED_TOOLS], DEFAULT_CSV.split(","), "SUPPORTED_TOOLS is exactly run.mjs's default --tools set"); - assert.equal(SUPPORTED_TOOLS.length, 7, "seven tools, nothing more"); + assert.equal(SUPPORTED_TOOLS.length, 11, "eleven tools, nothing more"); assert.ok(Object.isFrozen(SUPPORTED_TOOLS), "the allowlist is frozen"); const runSource = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "../worker/run.mjs"), "utf8"); assert.ok(runSource.includes(`"${DEFAULT_CSV}"`), "run.mjs still carries the default this list mirrors, unchanged"); @@ -113,42 +122,62 @@ function refuses(order, spec, code, names, label) { } // --- 1. NEGATIVE: an extension tool is refused ---------------------------------------------- -// Valid in every other field. The order GRANTS tool:web_fetch, so containment is satisfied and +// Valid in every other field. The order GRANTS tool:orchestrate, so containment is satisfied and // the worker's own allowlist is the only thing standing between this spec and an unattended -// agent with network fetch. materializeEnvelope is the pre-clone boundary: worker.mjs binds its -// result at :791 and only then reaches prepareRepository (:1208) and runVinci (:1293), so a -// throw here means nothing was cloned and nothing was spawned. +// agent that can spawn further agents. materializeEnvelope is the pre-clone boundary: worker.mjs +// binds its result at :791 and only then reaches prepareRepository (:1208) and runVinci (:1293), +// so a throw here means nothing was cloned and nothing was spawned. +// +// `orchestrate` replaced `web_fetch` here when the four network tools were admitted. The +// expected-substring list below spells the FULL eleven-name supported set that the refusal +// message prints, so this case also fails if the message stops reflecting the real list. { - const order = orderGranting(["web_fetch"]); - const spec = specFor(order, { tools: ["web_fetch"] }); - const thrown = refuses(order, spec, "tool_unsupported", ['"web_fetch"', "read, grep, find, ls, bash, edit, write"], "web_fetch alone"); - assert.match(thrown.message, /^tool_unsupported: tool "web_fetch" is not supported by this worker \(supported: /); + const order = orderGranting(["orchestrate"]); + const spec = specFor(order, { tools: ["orchestrate"] }); + const supportedNames = "read, grep, find, ls, bash, edit, write, web_search, web_fetch, web_answer, library_docs"; + const thrown = refuses(order, spec, "tool_unsupported", ['"orchestrate"', supportedNames], "orchestrate alone"); + assert.match(thrown.message, /^tool_unsupported: tool "orchestrate" is not supported by this worker \(supported: /); } -// Every extension tool the launcher registers, not just the one in the headline case. -for (const tool of ["web_search", "web_fetch", "web_answer", "library_docs", "advisor", "convene_council", "orchestrate", "spawn_helper"]) { +// Every extension tool the launcher registers that this worker still does NOT advertise. The +// four network tools that used to head this list were moved to §3's positive cases when they +// were admitted; leaving them here would have asserted a refusal that no longer happens. +for (const tool of ["advisor", "convene_council", "orchestrate", "spawn_helper"]) { const order = orderGranting([tool]); refuses(order, specFor(order, { tools: [tool] }), "tool_unsupported", [JSON.stringify(tool)], `extension tool ${tool}`); } +// …and no tool named in that loop may be one the allowlist now admits. Without this, a future +// widening would turn each case above into a green assertion about nothing — exactly the way +// the `web_fetch` fixtures went vacuous. +for (const tool of ["advisor", "convene_council", "orchestrate", "spawn_helper"]) { + assert.equal(SUPPORTED_TOOLS.includes(tool), false, `${tool} is still outside the allowlist, so the negative case above is not vacuous`); +} // --- 2. ORDERING CONTROL: prove WHICH guard answered ---------------------------------------- // (a) Without the tool: grant, containment (guard 3) answers FIRST and the allowlist is never // reached. This is the hazard made visible — a fixture written without the grant would have // gone green on `execution_exceeds_contract` and proved nothing about this change. { - const order = orderGranting(["read"]); // deliberately does NOT grant tool:web_fetch - refuses(order, specFor(order, { tools: ["web_fetch"] }), "execution_exceeds_contract", ["tool_not_granted", "/tools/0"], "ungranted web_fetch"); + const order = orderGranting(["read"]); // deliberately does NOT grant tool:orchestrate + refuses(order, specFor(order, { tools: ["orchestrate"] }), "execution_exceeds_contract", ["tool_not_granted", "/tools/0"], "ungranted orchestrate"); } // (b) With the grant, the SAME spec gets a different code. The pair is the discriminator: the // only thing that changed is the order's grant, and the answering guard moved from containment // to the allowlist. Nothing but the new check can produce `tool_unsupported`. { - const granted = orderGranting(["web_fetch"]); - const thrown = refuses(granted, specFor(granted, { tools: ["web_fetch"] }), "tool_unsupported", ["web_fetch"], "granted web_fetch"); + const granted = orderGranting(["orchestrate"]); + const thrown = refuses(granted, specFor(granted, { tools: ["orchestrate"] }), "tool_unsupported", ["orchestrate"], "granted orchestrate"); for (const earlier of ["unknown_field", "invalid_spec_field", "no_tools", "capability_unsupported", "execution_exceeds_contract", "binding_mismatch"]) { assert.notEqual(thrown.code, earlier, `an earlier guard must not be what answered (${earlier})`); } } +// (c) The same discriminator run on `spawn_helper`, so the pair is not a property of one name. +{ + const ungranted = orderGranting(["read"]); + refuses(ungranted, specFor(ungranted, { tools: ["spawn_helper"] }), "execution_exceeds_contract", ["tool_not_granted"], "ungranted spawn_helper"); + const granted = orderGranting(["spawn_helper"]); + refuses(granted, specFor(granted, { tools: ["spawn_helper"] }), "tool_unsupported", ['"spawn_helper"'], "granted spawn_helper"); +} // --- 3. POSITIVE REACHABILITY: the guarded operation still works ----------------------------- // Same entry point, same fixture shape. If these fail, the guard is refusing work it must admit @@ -159,31 +188,69 @@ for (const tool of ["web_search", "web_fetch", "web_answer", "library_docs", "ad assert.deepEqual(materialized.envelope.tools, ["read", "bash"], "a narrowed subset materializes"); } { - // The full default set — the exact seven run.mjs would have used anyway. Behaviour for a spec - // using the default MUST be unchanged by this commit. + // The full default set — the exact eleven run.mjs would have used anyway. Behaviour for a spec + // using the default MUST match run.mjs's fallback exactly. The expected value is a LITERAL, + // not `[...SUPPORTED_TOOLS]`: comparing the list against itself would pass under any edit. const order = orderGranting([...SUPPORTED_TOOLS]); const materialized = materialize(order, specFor(order, { tools: [...SUPPORTED_TOOLS] })); - assert.deepEqual(materialized.envelope.tools, ["read", "grep", "find", "ls", "bash", "edit", "write"], "the whole default set materializes"); + assert.deepEqual( + materialized.envelope.tools, + ["read", "grep", "find", "ls", "bash", "edit", "write", "web_search", "web_fetch", "web_answer", "library_docs"], + "the whole default set materializes", + ); } -// …and each of the seven on its own, so a typo in the list cannot hide behind its neighbours. +// …and each of the eleven on its own, so a typo in the list cannot hide behind its neighbours. for (const tool of SUPPORTED_TOOLS) { const order = orderGranting([tool]); const materialized = materialize(order, specFor(order, { tools: [tool] })); assert.deepEqual(materialized.envelope.tools, [tool], `${tool} alone is admitted`); } +// --- 3b. POSITIVE REACHABILITY FOR THE FOUR NEWLY-ADMITTED NETWORK TOOLS --------------------- +// The names are LITERALS, deliberately not derived from SUPPORTED_TOOLS: a case that iterates +// the list under test can only ever agree with it, and would stay green if the four were +// removed again. Spelled out, these fail the moment the allowlist stops carrying them — which +// is what makes them the positive control for this change rather than decoration. +const NETWORK_TOOLS = ["web_search", "web_fetch", "web_answer", "library_docs"]; +for (const tool of NETWORK_TOOLS) { + assert.equal(SUPPORTED_TOOLS.includes(tool), true, `${tool} is advertised by this worker`); + const order = orderGranting([tool]); + const materialized = materialize(order, specFor(order, { tools: [tool] })); + assert.deepEqual(materialized.envelope.tools, [tool], `${tool} alone is ADMITTED and carried through to the envelope`); +} +// All four together, and mixed with the original seven's members, through the same entry point. +{ + const order = orderGranting(NETWORK_TOOLS); + const materialized = materialize(order, specFor(order, { tools: [...NETWORK_TOOLS] })); + assert.deepEqual(materialized.envelope.tools, ["web_search", "web_fetch", "web_answer", "library_docs"], "all four network tools materialize together"); +} +{ + const mixed = ["read", "web_fetch", "bash", "library_docs"]; + const order = orderGranting(mixed); + const materialized = materialize(order, specFor(order, { tools: [...mixed] })); + assert.deepEqual(materialized.envelope.tools, ["read", "web_fetch", "bash", "library_docs"], "network tools mix with the original set"); +} + // --- 4. MIXED: one disallowed entry poisons the list ---------------------------------------- // The refusal must name the OFFENDING entry, not the first entry, and must not quietly drop it // and run with the rest. { - const order = orderGranting(["read", "web_fetch"]); - const thrown = refuses(order, specFor(order, { tools: ["read", "web_fetch"] }), "tool_unsupported", ['"web_fetch"'], "read + web_fetch"); + const order = orderGranting(["read", "orchestrate"]); + const thrown = refuses(order, specFor(order, { tools: ["read", "orchestrate"] }), "tool_unsupported", ['"orchestrate"'], "read + orchestrate"); assert.ok(!thrown.message.includes('tool "read"'), `the reason names the offending entry, not the admitted one: ${thrown.message}`); } { // …and in the other order, so the check is a scan and not a look at tools[tools.length - 1]. - const order = orderGranting(["web_fetch", "read"]); - refuses(order, specFor(order, { tools: ["web_fetch", "read"] }), "tool_unsupported", ['"web_fetch"'], "web_fetch + read"); + const order = orderGranting(["orchestrate", "read"]); + refuses(order, specFor(order, { tools: ["orchestrate", "read"] }), "tool_unsupported", ['"orchestrate"'], "orchestrate + read"); +} +{ + // …and a NEWLY-ADMITTED tool beside a refused one: the widening must not have turned the scan + // into "the list contains something allowed, ship it". web_fetch is admitted, orchestrate is + // not, and the pair must still refuse and still name orchestrate. + const order = orderGranting(["web_fetch", "orchestrate"]); + const thrown = refuses(order, specFor(order, { tools: ["web_fetch", "orchestrate"] }), "tool_unsupported", ['"orchestrate"'], "web_fetch + orchestrate"); + assert.ok(!thrown.message.includes('tool "web_fetch"'), `the admitted network tool is not the one named: ${thrown.message}`); } // --- 5. EDGE INPUTS --------------------------------------------------------------------------- @@ -251,7 +318,8 @@ for (const [label, tools, pattern] of [ // and a repeated REFUSED tool would still be refused. { assert.equal(SUPPORTED_TOOLS.includes("read"), true); - assert.equal(SUPPORTED_TOOLS.includes("web_fetch"), false); + assert.equal(SUPPORTED_TOOLS.includes("web_fetch"), true); // admitted by this change + assert.equal(SUPPORTED_TOOLS.includes("orchestrate"), false); // and this is the refused example } // CASE — ["READ"]. DELIBERATE CHOICE: matching is EXACT and case-sensitive, so "READ" is diff --git a/vinci/worker/README.md b/vinci/worker/README.md index 8806bbc02..87fedd336 100644 --- a/vinci/worker/README.md +++ b/vinci/worker/README.md @@ -61,7 +61,8 @@ WantedBy=multi-user.target 4. **Setup**: Clone/fetch the repo and reuse or create `worker/` from `origin/main` 5. **Run**: Spawn `vinci -p --session-id --tools ""`, where `` is the execution spec's `tools` when the digest form supplies one and - `read,grep,find,ls,bash,edit,write` otherwise. A spec may NARROW that set; it cannot widen + `read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs` otherwise. + A spec may NARROW that set; it cannot widen it, because `SUPPORTED_TOOLS` in `task.mjs` refuses anything outside it as `tool_unsupported`. 6. **Limits**: @@ -770,14 +771,18 @@ existing worker suite with the flag off. - Outbound HTTPS to bus (`--server`) - Outbound HTTPS to GitHub (clone, fetch, push, PR operations) - NO inbound network required -- Runs with at most `--tools read,grep,find,ls,bash,edit,write` (enforced by `SUPPORTED_TOOLS` - in `task.mjs`; a spec may ask for fewer, never more). None of the launcher's network tools - — `web_search`, `web_fetch`, `web_answer`, `library_docs` — are in that set, though the - launcher does register them. -- 🔴 That is a TOOL boundary, not a network boundary. `bash` is in the set and there is no - egress allowlist (see "No network allowlist" under the clean-room gaps above), so the child - can still reach anything the box can reach. Withholding the network tools removes the - attributable path, not the capability. +- Runs with at most + `--tools read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs` + (enforced by `SUPPORTED_TOOLS` in `task.mjs`; a spec may ask for fewer, never more). The + launcher's four network tools — `web_search`, `web_fetch`, `web_answer`, `library_docs` — + ARE in that set: they were added to both lists together on the repo owner's authorization, + the launcher having registered them unconditionally all along. Other launcher extension + tools (`orchestrate`, `spawn_helper`, `advisor`, `convene_council`, …) remain outside it and + are refused as `tool_unsupported`. +- 🔴 That is a TOOL boundary, not a network boundary — and it never was one. `bash` is in the + set and there is no egress allowlist (see "No network allowlist" under the clean-room gaps + above), so the child can still reach anything the box can reach. Admitting the network tools + adds an attributable path; it does not add a capability the child lacked. ## See Also diff --git a/vinci/worker/lease.mjs b/vinci/worker/lease.mjs index dee27647d..05a042c54 100644 --- a/vinci/worker/lease.mjs +++ b/vinci/worker/lease.mjs @@ -366,7 +366,8 @@ export const DECLARATION_REFRESH_DEFAULT_S = 21600; // steering false no command can redirect a running task // approvals "none" nothing in the run waits for a person // pause false the only brake is termination -// restrictToReadOnly false the tool set is fixed at spawn (read,grep,find,ls,bash,edit,write) +// restrictToReadOnly false the tool set is fixed at spawn (read,grep,find,ls,bash,edit, +// write,web_search,web_fetch,web_answer,library_docs) // abort false no bus command aborts a run: the daemon consumes only kind // "handoff" and has no abort handler. Limits, lease loss and the // daemon's own SIGTERM end a run, but none of those is a caller- diff --git a/vinci/worker/run.mjs b/vinci/worker/run.mjs index c2f7ed7fb..f689bf0b6 100644 --- a/vinci/worker/run.mjs +++ b/vinci/worker/run.mjs @@ -1207,7 +1207,7 @@ export function runVinci({ envelope, repoDir, stateDir, taskId, sessionId, env, const pollMs = Number(process.env.VINCI_WORKER_LIMIT_POLL_MS) || 15_000; const killGraceMs = Number(process.env.VINCI_WORKER_KILL_GRACE_MS) || 30_000; const abortKillGraceMs = Number(process.env.VINCI_WORKER_LEASE_KILL_GRACE_MS) || 10_000; - const tools = Array.isArray(envelope.tools) && envelope.tools.length > 0 ? envelope.tools.join(",") : "read,grep,find,ls,bash,edit,write"; + const tools = Array.isArray(envelope.tools) && envelope.tools.length > 0 ? envelope.tools.join(",") : "read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs"; const taskEnvironment = applyEnvDelta(env ?? process.env, envDelta); taskEnvironment.VINCI_UPDATE_DISABLED = "1"; for (const name of [ diff --git a/vinci/worker/task.mjs b/vinci/worker/task.mjs index 38990a5ec..f234f2ba9 100644 --- a/vinci/worker/task.mjs +++ b/vinci/worker/task.mjs @@ -190,21 +190,27 @@ export const SUPPORTED_CAPABILITIES = Object.freeze([]); // The closed set of tools this worker will hand an unattended `vinci -p` agent. It is EXACTLY the // default allowlist run.mjs falls back to when a spec names none ("read,grep,find,ls,bash,edit, -// write"), so a work order may NARROW what the agent may call and never widen it. Same posture as -// SUPPORTED_CAPABILITIES above: anything this worker does not advertise is BLOCKED -// (`tool_unsupported`), never silently dropped. Grow this list only together with the run.mjs -// default it mirrors. +// write,web_search,web_fetch,web_answer,library_docs"), so a work order may NARROW what the agent +// may call and never widen it. Same posture as SUPPORTED_CAPABILITIES above: anything this worker +// does not advertise is BLOCKED (`tool_unsupported`), never silently dropped. Grow this list only +// together with the run.mjs default it mirrors — worker-tools-allowlist.mjs pins the two to each +// other and fails if either moves alone. +// +// The four network tools (web_search, web_fetch, web_answer, library_docs) were added to BOTH +// lists together, on the repo owner's authorization. They are registered unconditionally by the +// launcher (vinci/bin/vinci loads vinci/extensions/vinci-search.ts), so this was the `--tools` +// allowlist catching up with what the child already had access to, not a new integration. // // SCOPE — this is hardening, not the closure of a live hole. `spec.tools` reaches run.mjs only // through the DIGEST handoff form (materializeEnvelope populates `envelope.tools`); the prose // envelope form has no `tools` header at all (see HEADER_KEYS above), so on the prose path -// `envelope.tools` is undefined and run.mjs falls back to the seven-tool default regardless. The +// `envelope.tools` is undefined and run.mjs falls back to the eleven-tool default regardless. The // digest path needs a contract registry that is not configured in production, so nothing has been // dispatched through this field. It becomes load-bearing the moment that registry is enabled — // which is exactly when it is too late to add. Before this list, `tools` was validated for shape // only, and the launcher (vinci/bin/vinci) unconditionally registers ~30 extension tools -// (web_search, web_fetch, web_answer, library_docs, advisor, convene_council, orchestrate, -// spawn_helper), so any string a spec named would have been forwarded verbatim to `--tools`. +// (advisor, convene_council, orchestrate, spawn_helper, … alongside the four network tools this +// list now admits), so any string a spec named would have been forwarded verbatim to `--tools`. // // NOT the same check as `tool_not_granted`, and it does not supersede it. Containment // (contracts/within-order.mjs, the vendored port of vinci-gpu-control's @@ -213,7 +219,19 @@ export const SUPPORTED_CAPABILITIES = Object.freeze([]); // list asks whether THIS WORKER supports the tool at all — a question no work order can answer, // and one that must still refuse when a registry grants something the worker should not run. Two // different questions, two different reason codes, deliberately. Both must pass. -export const SUPPORTED_TOOLS = Object.freeze(["read", "grep", "find", "ls", "bash", "edit", "write"]); +export const SUPPORTED_TOOLS = Object.freeze([ + "read", + "grep", + "find", + "ls", + "bash", + "edit", + "write", + "web_search", + "web_fetch", + "web_answer", + "library_docs", +]); // B2: read the operator model-class table from VINCI_WORKER_MODEL_CLASSES. The value is a JSON // object `{ : { provider, model } }`, or `@` naming a JSON file (parsed From b903c0ee9b5b56f048ee66d25366924ee897ea75 Mon Sep 17 00:00:00 2001 From: George Pu <19347973+thegeorgepu@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:26:50 -0400 Subject: [PATCH 4/5] test(worker): pin the --tools default behaviourally, not by source text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid --- vinci/test/worker-tools-allowlist.mjs | 156 ++++++++++++++++++++++++-- 1 file changed, 148 insertions(+), 8 deletions(-) diff --git a/vinci/test/worker-tools-allowlist.mjs b/vinci/test/worker-tools-allowlist.mjs index a5ae46490..28be7cb7f 100644 --- a/vinci/test/worker-tools-allowlist.mjs +++ b/vinci/test/worker-tools-allowlist.mjs @@ -41,11 +41,13 @@ // assertion below pins the EXACT reason code, and the ordering control proves which guard // answered. import assert from "node:assert/strict"; -import { readFileSync } from "node:fs"; -import { dirname, join } from "node:path"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { delimiter, dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { executionSpecDigest, workOrderDigest } from "../worker/contracts/digest.mjs"; +import { runVinci } from "../worker/run.mjs"; import { materializeEnvelope, SUPPORTED_TOOLS } from "../worker/task.mjs"; const VECTORS = join(dirname(fileURLToPath(import.meta.url)), "fixtures/contract-vectors"); @@ -97,18 +99,156 @@ function refuses(order, spec, code, names, label) { return thrown; } -// --- the allowlist IS the run.mjs default, neither more nor less ---------------------------- +// --- 0. the allowlist IS the run.mjs default, neither more nor less ------------------------- // This restricts what a spec may REQUEST down to what run.mjs already grants by default; it must -// not enable anything beyond that default. Pinned against the literal fallback string in -// run.mjs:1210 so the two cannot drift apart silently in either direction — this is the LOCKSTEP -// pin, and it is what fails if the four network tools are added to one list and not the other. +// not enable anything beyond that default. This is the LOCKSTEP pin, and it is what fails if the +// four network tools are added to one list and not the other. +// +// 🔴 HOW THIS PIN USED TO BE VACUOUS. It was carried by a source-TEXT check: +// assert.ok(runSource.includes(`"${DEFAULT_CSV}"`), …) +// `includes` is satisfied by the literal appearing ANYWHERE in run.mjs — a comment included. +// Executed mutation, 2026-09-04: reverting the LIVE fallback expression at run.mjs:1210 to the +// old seven-tool CSV while leaving the correct eleven-tool literal in a comment above `runVinci` +// left this suite printing PASS. The assertion pinned a string in a file, not the value the +// worker hands to the agent. It is replaced below by a BEHAVIOURAL pin. +// +// Why the value matters at all: run.mjs's fallback is what the deployed fleet actually passes to +// `--tools`, because the prose-envelope form carries no `tools` field (HEADER_KEYS has none), so +// the fallback branch is the one production takes on every prose task. +const DEFAULT_CSV = "read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs"; { - const DEFAULT_CSV = "read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs"; assert.deepEqual([...SUPPORTED_TOOLS], DEFAULT_CSV.split(","), "SUPPORTED_TOOLS is exactly run.mjs's default --tools set"); assert.equal(SUPPORTED_TOOLS.length, 11, "eleven tools, nothing more"); assert.ok(Object.isFrozen(SUPPORTED_TOOLS), "the allowlist is frozen"); +} + +// --- 0b. BEHAVIOURAL PIN: what `runVinci` actually puts on the command line ------------------- +// `resolveBin("vinci")` (worker/build.mjs:16) is a bare PATH scan performed at spawn time, so a +// stub `vinci` placed first on PATH IS the executable the real `runVinci` launches. The stub +// records its own argv; every assertion below reads that recording. Nothing here inspects source +// text, so a comment cannot satisfy it and only the expression that survives to the spawn can. +// +// This block also gives `runVinci` its first test of any kind: it is imported by zero other tests +// repo-wide, so the `envelope.tools` parameterisation path had no coverage at all. +{ + const RUN_BOUND_MS = 20_000; + const savedPath = process.env.PATH; + const root = mkdtempSync(join(tmpdir(), "worker-tools-argv-")); + try { + const binDir = join(root, "bin"); + const repoDir = join(root, "repo"); + const stateDir = join(root, "state"); + const argvFile = join(root, "argv.json"); + for (const directory of [binDir, repoDir, stateDir]) mkdirSync(directory, { recursive: true }); + + // Shebanged with this interpreter's absolute path, so the stub does not itself depend on what + // PATH resolves — the only PATH lookup under test is the one `resolveBin` performs for + // "vinci". No extension ⇒ CommonJS, hence `require`. + const stub = join(binDir, "vinci"); + writeFileSync( + stub, + `#!${process.execPath}\n` + + `require("node:fs").writeFileSync(${JSON.stringify(argvFile)}, JSON.stringify(process.argv.slice(2)));\n` + + `process.exit(0);\n`, + ); + chmodSync(stub, 0o755); + process.env.PATH = binDir + delimiter + savedPath; + + // Bounded: `runVinci` polls on a timer and resolves on the child's `close`. If the stub were + // never spawned the promise would sit forever, and a hung suite reads as neither pass nor + // fail — so the wait is capped and the cap is itself an assertion failure. + const launch = async (envelope, label) => { + rmSync(argvFile, { force: true }); + const bound = new Promise((_, reject) => + setTimeout(() => reject(new Error(`${label}: runVinci did not settle within ${RUN_BOUND_MS}ms`)), RUN_BOUND_MS).unref(), + ); + const run = await Promise.race([ + runVinci({ + envelope: { provider: "vinci", model: "forte", spec: "stub task", max_runtime_s: 300, budget_usd: 100, ...envelope }, + repoDir, + stateDir, + taskId: `argv-${label}`, + sessionId: `argv-${label}`, + }), + bound, + ]); + assert.equal(run.exit_code, 0, `${label}: the stub ran and exited cleanly`); + assert.equal(run.limit_tripped, null, `${label}: no limit tripped, so the argv below is a real launch`); + const argv = JSON.parse(readFileSync(argvFile, "utf8")); + const flags = argv.filter((a) => a === "--tools"); + assert.equal(flags.length, 1, `${label}: exactly one --tools flag on the command line: ${JSON.stringify(argv)}`); + const at = argv.indexOf("--tools"); + assert.ok(at + 1 < argv.length, `${label}: --tools is followed by a value: ${JSON.stringify(argv)}`); + return { argv, tools: argv[at + 1] }; + }; + + // (A) FALLBACK — the production path. No `tools` on the envelope, exactly as a prose handoff + // arrives, so run.mjs's default branch is the one taken. The expected value is the LITERAL + // eleven-tool CSV; it is then split and compared to SUPPORTED_TOOLS, which is what makes the + // two lists lockstep through the value the worker really uses. + { + const { tools, argv } = await launch({}, "fallback"); + assert.equal( + tools, + "read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs", + `an envelope with no tools launches the eleven-tool default: ${JSON.stringify(argv)}`, + ); + assert.deepEqual(tools.split(","), [...SUPPORTED_TOOLS], "the CSV the worker actually launches with IS the allowlist, element for element"); + assert.equal(tools, DEFAULT_CSV, "…and is the literal this file pins"); + } + + // (B) NARROWING — the parameterised path, previously untested. Its value is DIFFERENT from + // (A)'s, which is also this instrument's positive control: it proves the recording tracks the + // envelope rather than echoing a constant, so (A)'s match is evidence and not an artefact. + { + const { tools, argv } = await launch({ tools: ["read", "bash"] }, "narrowed"); + assert.equal(tools, "read,bash", `an envelope naming two tools launches exactly those two: ${JSON.stringify(argv)}`); + assert.notEqual(tools, DEFAULT_CSV, "the narrowed launch is not the default, so the recording is envelope-sensitive"); + } + + // (C) A single tool, and the full set spelled out — a one-element join must not gain a + // separator, and an envelope that names the whole default must reach the agent unchanged. + { + const { tools } = await launch({ tools: ["read"] }, "single"); + assert.equal(tools, "read", "one tool joins to itself, with no trailing separator"); + } + { + const { tools } = await launch({ tools: [...SUPPORTED_TOOLS] }, "explicit-full"); + assert.equal(tools, DEFAULT_CSV, "an envelope naming the whole allowlist launches the same CSV as the fallback"); + } + + // (D) EDGE INPUTS on the fallback condition itself. run.mjs takes the default unless `tools` + // is a NON-EMPTY ARRAY, so each of these must land on the eleven-tool CSV rather than on "" + // or "undefined" — an agent launched with `--tools ""` would be a silent capability change. + for (const [label, tools] of [ + ["empty-array", []], + ["null", null], + ["undefined", undefined], + ["string", "read,bash"], + ["object", { read: true }], + ]) { + const launched = await launch({ tools }, `degenerate-${label}`); + assert.equal(launched.tools, DEFAULT_CSV, `a ${label} tools field falls back to the eleven-tool default, not to an empty or malformed CSV`); + } + } finally { + process.env.PATH = savedPath; + rmSync(root, { recursive: true, force: true }); + } +} + +// --- 0c. SECONDARY SMOKE (not the guarantee) -------------------------------------------------- +// Anchored on `const tools =` so a comment cannot satisfy it, unlike the `includes` check this +// replaced. It is kept only to name run.mjs:1210 as the site under test and to fail loudly if +// that expression is restructured; §0b is what actually pins the value, and this check would be +// removable without weakening the guard. +{ const runSource = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "../worker/run.mjs"), "utf8"); - assert.ok(runSource.includes(`"${DEFAULT_CSV}"`), "run.mjs still carries the default this list mirrors, unchanged"); + const live = runSource.match(/^\s*const tools = .*$/m); + assert.ok(live, "run.mjs still computes the --tools CSV in a `const tools =` expression"); + assert.ok( + live[0].includes(`"${DEFAULT_CSV}"`), + `the LIVE fallback expression — not a comment — carries the eleven-tool default: ${live[0]}`, + ); } // --- precondition: the fixture materializes when the tool IS advertised --------------------- From e6ee15d5d78ed45a28c91e6fd6223eb3c75adba6 Mon Sep 17 00:00:00 2001 From: George Pu <19347973+thegeorgepu@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:33:20 -0400 Subject: [PATCH 5/5] docs(worker): cite the authorizing decision instead of asserting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (d1563bb6) 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) Claude-Session: https://claude.ai/code/session_01UNuX6G1j9mieQ1jcwpuAid --- vinci/worker/README.md | 5 +++-- vinci/worker/task.mjs | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/vinci/worker/README.md b/vinci/worker/README.md index 87fedd336..d7973887c 100644 --- a/vinci/worker/README.md +++ b/vinci/worker/README.md @@ -775,8 +775,9 @@ existing worker suite with the flag off. `--tools read,grep,find,ls,bash,edit,write,web_search,web_fetch,web_answer,library_docs` (enforced by `SUPPORTED_TOOLS` in `task.mjs`; a spec may ask for fewer, never more). The launcher's four network tools — `web_search`, `web_fetch`, `web_answer`, `library_docs` — - ARE in that set: they were added to both lists together on the repo owner's authorization, - the launcher having registered them unconditionally all along. Other launcher extension + ARE in that set: they were added to both lists together under the decision recorded on the bus + as `msg_de1a219d` (corrected by `msg_02bb0a87`), the launcher having registered them + unconditionally all along. Other launcher extension tools (`orchestrate`, `spawn_helper`, `advisor`, `convene_council`, …) remain outside it and are refused as `tool_unsupported`. - 🔴 That is a TOOL boundary, not a network boundary — and it never was one. `bash` is in the diff --git a/vinci/worker/task.mjs b/vinci/worker/task.mjs index f234f2ba9..2782b5748 100644 --- a/vinci/worker/task.mjs +++ b/vinci/worker/task.mjs @@ -197,9 +197,12 @@ export const SUPPORTED_CAPABILITIES = Object.freeze([]); // other and fails if either moves alone. // // The four network tools (web_search, web_fetch, web_answer, library_docs) were added to BOTH -// lists together, on the repo owner's authorization. They are registered unconditionally by the -// launcher (vinci/bin/vinci loads vinci/extensions/vinci-search.ts), so this was the `--tools` -// allowlist catching up with what the child already had access to, not a new integration. +// lists together. The authorizing decision is recorded on the bus as msg_de1a219d (corrected by +// msg_02bb0a87) — cite that, not this comment: a source file asserting its own authorization is +// self-attestation, and a reader has no way to check it from inside the diff. They are registered +// unconditionally by the launcher (vinci/bin/vinci loads vinci/extensions/vinci-search.ts), so +// this was the `--tools` allowlist catching up with what the child already had, not a new +// integration. // // SCOPE — this is hardening, not the closure of a live hole. `spec.tools` reaches run.mjs only // through the DIGEST handoff form (materializeEnvelope populates `envelope.tools`); the prose