From 3ad5acfe55fc16a75bb05fd4b7d8b808cd9f3536 Mon Sep 17 00:00:00 2001 From: Ruturaj-Browserstack Date: Mon, 31 Aug 2026 22:43:54 +0530 Subject: [PATCH] =?UTF-8?q?fix!:=20drop=20the=20coordinator's=20tools=20al?= =?UTF-8?q?lowlist=20=E2=80=94=20it=20reduced=20it=20to=20Bash=20+=20Read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live run's coordinators could not work: dispatched as `tfa-rca:ai-tfa-coordinator`, they reported having only `Bash` and `Read` — no `ToolSearch`, no `tfaRcaTurn`, no `getTfaTurnResult` — and terminated honestly rather than fabricating a result. Two retries did the same. I caused it. The frontmatter carried tools: [Bash, Read, Grep, Glob, Task, mcp__*__tfaRcaTurn, mcp__*__getTfaTurnResult, mcp__github__*] and that line had been INERT for months because the YAML block failed to parse. Fixing the parse so `claude plugin validate` would pass activated an allowlist that had therefore never once been exercised — and it is wrong three ways: 1. `mcp__*__tfaRcaTurn` does not match the real `mcp__plugin_tfa-rca_bstack__tfaRcaTurn`; 2. `Task`, `Grep` and `Glob` were not granted either; 3. `ToolSearch` is absent — and MCP tools are DEFERRED, so without it no schema can be loaded and `tfaRcaTurn` is unreachable even when its name is permitted. That is the lesson I should have drawn at the time: I noted in the validate fix that `tools:` and `model:` "were being discarded at runtime" and treated restoring them as pure upside, without asking whether a list nothing had ever enforced was correct. Restoring an unexercised constraint is a behaviour change, not a repair. **Removed rather than corrected**, because an allowlist cannot express what this agent needs. The coordinator routes evidence to whatever the CUSTOMER has — a log store, a cluster, a metrics surface, a forge — and those are unknowable when this file is written. Inheriting the session's tools is not laxness here; it is the only thing consistent with "generic over product and infra", and it is what worked for months. `model: sonnet` is kept, but flagging it: it was dropped by the same parse failure, so it is newly in force. Coordinators now run on Sonnet where they previously inherited the session model. That is what the file asks for and it is a deliberate cost choice, but it shipped unexamined alongside the tools change and is worth a conscious decision. Guard added, mutation-proven against the exact old line, against a plausible "better" allowlist, and against the frontmatter being un-indented back into unparseability — the failure mode that hid all of this. The guard states why a list cannot work, so the next person adding one has to argue with the reason rather than rediscover it. 340 tests, up from 339. Co-Authored-By: Claude Opus 5 (cherry picked from commit a3df342a4a8c42bfa4f9980ff02b3f56689751f8) --- agents/ai-tfa-coordinator.md | 1 - tests/wiring.test.mjs | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/agents/ai-tfa-coordinator.md b/agents/ai-tfa-coordinator.md index 8650b64..5b196ac 100644 --- a/agents/ai-tfa-coordinator.md +++ b/agents/ai-tfa-coordinator.md @@ -4,7 +4,6 @@ description: 'Per-test collaborative-RCA coordinator (autonomous — never promp - orchestrator: Agent(subagent_type="tfa-rca:ai-tfa-coordinator", prompt="RCA testRunId=39 — error: empty buildName rejected on POST /builds") → drives the loop, returns RCA_OUTPUT - sibling confirm: Agent(subagent_type="tfa-rca:ai-tfa-coordinator", prompt="RCA testRunId=40 — pre-seed: cause=, suspect PR=#7421") → one-turn confirm against this test logs - user: "run collaborative RCA on test run 39" → single-test loop to RESOLVED/PENDING' -tools: [Bash, Read, Grep, Glob, Task, mcp__*__tfaRcaTurn, mcp__*__getTfaTurnResult, mcp__github__*] model: sonnet --- diff --git a/tests/wiring.test.mjs b/tests/wiring.test.mjs index df34e6a..8091b2f 100644 --- a/tests/wiring.test.mjs +++ b/tests/wiring.test.mjs @@ -1153,3 +1153,46 @@ test("SETUP.md gates its remaining steps on the sign-in", () => { assert.match(flat, /wiring rather than sign-in/u, "the two failure modes must be distinguished — guessing wastes a round trip"); }); + +// ---- the coordinator must not carry a tools allowlist ------------------------ +// +// It had one, and it was inert for months because the YAML frontmatter failed to parse. +// Fixing the parse ACTIVATED it, and a live run then died: the coordinator was granted +// only `Bash` and `Read`. Three separate faults in one line — +// +// tools: [Bash, Read, Grep, Glob, Task, mcp__*__tfaRcaTurn, mcp__*__getTfaTurnResult, mcp__github__*] +// +// 1. `mcp__*__tfaRcaTurn` did not match the real `mcp__plugin_tfa-rca_bstack__tfaRcaTurn`; +// 2. `Task`/`Grep`/`Glob` were not granted either; +// 3. `ToolSearch` was absent — and MCP tools are DEFERRED, so without it no schema can +// be loaded and `tfaRcaTurn` is unreachable even when its name is allowed. +// +// It had never been exercised, so none of that had ever been caught. +// +// The deeper reason not to fix the list: this plugin routes evidence to whatever tools +// the CUSTOMER has — a log store, a cluster, a metrics surface, a forge. Those are +// unknowable when the file is written, so an allowlist cannot express what a coordinator +// legitimately needs. Inheriting the session's tools is not laxness here, it is the only +// thing consistent with "generic over product and infra". +test("the coordinator agent declares no tools allowlist", () => { + // MUTATION: add any `tools:` line to the frontmatter -> fails. + const src = readFileSync(join(ROOT, "agents/ai-tfa-coordinator.md"), "utf8"); + const fm = src.split("---")[1] ?? ""; + assert.doesNotMatch( + fm, /^tools:/mu, + "a coordinator restricted to a fixed tool list cannot reach a customer's connectors, " + + "and the last such list silently reduced it to Bash + Read", + ); + + // The frontmatter must still PARSE — an unparseable block drops every field silently, + // which is how the broken allowlist stayed hidden. Continuation lines of a multi-line + // scalar must be indented; at column 0 a strict parser sees a new document-level + // sequence and discards the lot. + for (const line of fm.split("\n")) { + assert.doesNotMatch( + line, /^- /u, + "a frontmatter continuation line at column 0 makes the whole block unparseable, " + + "and the runtime then drops every field but the filename-derived name", + ); + } +});