From 3274db275758907705dc95953a90b8e8f2521ccb Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Tue, 28 Jul 2026 17:02:01 -0500 Subject: [PATCH] fix(#2526): remove gsd-ui-auditor's uncallable Playwright-MCP block (#2594) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2526): drop gsd-ui-auditor's uncallable Playwright-MCP block The agent declares `tools: Read, Write, Bash, Grep, Glob, Skill` — no `mcp__*` grant of any kind — while its body presented a `` block as the *preferred* capture path. That branch was unreachable by construction: the availability check had a fixed answer, the three `mcp__playwright__*` calls could never dispatch, and the "when Playwright-MCP is NOT available" fallback was the only branch that ever ran — 39 lines of instruction loaded on every /gsd-ui-review spawn that also invited the model to claim a capture path it could not take. Remove the dead block, leaving the CLI screenshot path as the sole documented approach. Guard the class in tests/mcp-tool-inheritance.test.cjs, which already owns agent MCP-grant parity: the new block generalizes the #1284 researcher check from two agents and one dispatch table to every agents/*.md and its whole body — no agent may document an `mcp____*` namespace absent from its own `tools:` declaration. Frontmatter is read through the canonical parser (gsd-core/bin/lib/frontmatter.cjs) rather than a hand-rolled scan, so inline CSV, block sequences, flow arrays, quoted scalars and full-line comments are handled by construction; inline comments inside a scalar survive that parser, so they are stripped explicitly. The check is server-level by design, ignores prose metavariables like `mcp__X__*`, matches hyphenated server ids, and carries a discovery guard plus negative controls for every documented boundary so it cannot decay into a vacuous pass. The session-level Playwright-MCP pass in gsd-core/workflows/ui-review.md is deliberately untouched — workflow files carry no fixed allowlist, so their availability check is genuinely runtime-detected and honest. Fixes #2526 * chore(#2526): set changeset fragment pr to 2594 The fragment shipped with the documented `pr: 0` placeholder because the PR number does not exist until the PR is opened, and scripts/changeset/parse.cjs rejects `pr <= 0`. Now that the PR is open, set the real number so changeset-lint passes. * test(#2526): cover the two-char server-id boundary of the metavariable exclusion The length-1 "prose metavariable" exclusion was tested at length=1 and at real ids (>=3 chars), but never at length=2 — the limit+1 boundary where a server id starts being recognized. Review finding on #2594: `mcp__ab__foo` in a body with no grant must flag `['ab']`. * fix(#2526): treat a bare mcp__* grant as covering every server `grantedServers()` stripped `mcp__*` to the empty string and dropped it via `if (server)`, so an allowlist that grants every MCP server read as granting none — and the guard then fired against a body the grant plainly covered. That is the one input shape that inverts the check, turning it against a correct agent rather than merely missing a bad one. A `/^mcp__\*+$/` token now sets a GRANT_ALL sentinel that short-circuits `ungrantedServers()`. The sentinel `*` is outside REFERENCE_RE's character class, so no body reference can collide with it. A bare `mcp__` with no wildcard stays a typo rather than a grant and keeps failing closed. No agent uses the `mcp__*` spelling today, so this was latent rather than live. Two negative controls pin both halves. * fix(#2526): scan the frontmatter description for MCP references too `ungrantedServers()` scanned `stripFrontmatter(content)` only, so an `mcp__foo__bar` reference in the `description` field escaped the check. That field ships with the agent and the dispatcher reads it, which makes a dead reference there exactly as dead as one in the body. Only `description` is added to the scanned surface, never the whole frontmatter: `tools:` is the grant list itself, so scanning it would let every allowlist satisfy itself and turn the guard vacuous. A negative control pins that boundary alongside the new positive case. All 34 per-agent tests still pass with the wider surface, so no live agent verdict changes — this was latent. * test(#2526): give multi-character placeholders a convention the checker knows The metavariable exclusion is `length === 1`, so the natural placeholders `mcp__SRV__*` and `mcp__SERVER__*` were flagged as real references — and the failure message then offered an author two remedies ("grant the namespace or drop the block") that both misread what they wrote. Adopts the angle-bracket half of the suggested fix: `mcp____*` is the sanctioned multi-character placeholder, exempt by construction because `<` is outside the reference pattern's character class. This pins an existing property rather than adding a special case. Declines the all-caps half. An all-caps exemption would be a false NEGATIVE for any real server spelled in caps, and a guard that misses a dead reference fails in exactly the direction this check exists to prevent. The bare-caps form keeps firing; the message now names the convention as a third remedy. Also corrects "grants neither" in that message, which was wrong for any count other than two. * test(#2526): pin the zero-length server id, completing the boundary triple `mcp____foo` yields `[]`, but for a different reason than the length-1 case: it is unrepresentable by `/mcp__([A-Za-z0-9_-]+?)__/g` since `+?` requires at least one character, so the pattern skips it before the metavariable exclusion is ever consulted. Pinning limit-1 completes the 0/1/2 boundary rule on its own terms and records which mechanism owns the case. * docs(#2526): correct every drifted AGENTS.md Tools row, not just the one The review asked for the one-line `gsd-ui-auditor` correction (missing `Skill`). Sweeping the defect class first — every `**Tools**` row in docs/AGENTS.md against its agent's `tools:` frontmatter — found it was 26 of 34 rows, so the one-line framing was the reviewer's premise rather than the population. Breakdown of the 26: * 21 omitted `Skill`, 6 omitted `Edit` (overlapping) — under-promises, the same drift class as #2526 but in the harmless direction. * 8 wrote `mcp (context7)` as shorthand while frontmatter granted up to 8 servers (firecrawl, exa, tavily, ref, jina, perplexity, both context7s). * 1 was actively wrong: gsd-debug-session-manager documented `Task`, a tool name that no longer exists — the #2526 shape at the doc layer, naming a capability that cannot dispatch. Every row is now the frontmatter `tools:` value verbatim, which is also what makes the parity guard in the following commit non-brittle. The diff is 26 insertions / 26 deletions, all Tools rows. * test(#2526): guard AGENTS.md Tools rows against agent frontmatter The 26 corrected rows in the previous commit were free to drift because nothing asserted the role card and the frontmatter agreed — the same reason the #2526 block itself survived. Correcting them without an invariant just resets the clock. Lands in agent-classification-parity.test.cjs rather than a new file: that suite already owns docs/AGENTS.md as a contract surface, already carries the `allow-test-rule` exemption for treating the doc as the product, and file count is the unit of CI overhead (docs/TESTING-SUITES.md). Compares the row to the frontmatter value VERBATIM, not as a set — a set comparison would keep accepting the "mcp (context7)" shorthand that hid eight grants behind one, which is the under-documentation half of the drift. Carries the same discovery guard #2526's own check uses: a section with a granted `tools:` but no **Tools** row fails loudly, so deleting a row cannot silently retire its assertion. Both halves are negative-controlled — against the pre-fix doc it fails naming 26 rows (gsd-ui-auditor:339 among them), and with a row deleted it fails on the missing-row assertion. * chore(#2526): note the AGENTS.md drift correction in the changeset The role cards are user-visible, and 26 of them documented a tool set the agent did not have. Type, `pr: 2594`, and the trailing `(#2526)` are unchanged. * fix(#2526): use a CRLF-safe split in the AGENTS.md Tools-row guard `lint-tests` (npm run lint:ci) rejected `rawAgentsMd.split('\n')` under the repo's local/no-crlf-fragile-split rule: Windows autocrlf yields CRLF, so a trailing \r rides into the parsed line. Switched to `/\r?\n/`. Caught by CI on the round-3 push before the response comment went out. * docs(#2526): correct the drift tallies stated in c5a9607e Re-derived the census programmatically from the pre-fix doc instead of by eye. The 26-of-34 headline was right; the breakdown was not. Skill omitted 21 -> 22 Edit omitted 6 -> 7 "mcp (context7)" shorthand 8 rows -> 7 rows The 8 was conflating two things: 8 rows omitted MCP grants entirely, but only 7 of them used the "mcp (context7)" shorthand — gsd-executor listed no MCP at all. Also names the one `Agent` omission (gsd-debug-session-manager, the row that still read `Task`). c5a9607e's message keeps the wrong numbers rather than rewriting a pushed branch mid-review; the test comment and changeset are the durable statements and both are corrected here. --------- Co-authored-by: Tom Boucher --- .changeset/2526-ui-auditor-dead-mcp-block.md | 5 + agents/gsd-ui-auditor.md | 40 --- docs/AGENTS.md | 52 +-- tests/agent-classification-parity.test.cjs | 85 +++++ tests/mcp-tool-inheritance.test.cjs | 335 +++++++++++++++++++ 5 files changed, 451 insertions(+), 66 deletions(-) create mode 100644 .changeset/2526-ui-auditor-dead-mcp-block.md diff --git a/.changeset/2526-ui-auditor-dead-mcp-block.md b/.changeset/2526-ui-auditor-dead-mcp-block.md new file mode 100644 index 000000000..ff7e2ebe3 --- /dev/null +++ b/.changeset/2526-ui-auditor-dead-mcp-block.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2594 +--- +**`gsd-ui-auditor` no longer documents an uncallable Playwright-MCP capture path** — the agent's `tools:` allowlist grants no MCP namespace, so the `` block it presented as "preferred" could never dispatch: the availability check had a fixed answer, the three `mcp__playwright__*` calls were unreachable, and the CLI fallback was the only branch that ever ran. The dead block is removed, leaving the CLI screenshot path as the sole documented approach, and a new consistency test fails any `agents/*.md` that documents an `mcp____*` namespace its own `tools:` line withholds. Session-level Playwright-MCP capture in `/gsd-ui-review` is unaffected — that path is genuinely runtime-detected. The same documented-vs-granted drift is corrected one layer out in `docs/AGENTS.md`, where 26 of 34 per-agent **Tools** rows disagreed with the agent's frontmatter — 22 omitting `Skill`, 7 omitting `Edit`, 8 omitting MCP grants entirely (7 of them abbreviating up to eight distinct servers as "mcp (context7)"), and one still naming `Task`, a tool that no longer exists — with a parity guard added so the role cards and the frontmatter cannot drift apart again. (#2526) diff --git a/agents/gsd-ui-auditor.md b/agents/gsd-ui-auditor.md index 9111177f2..f502d99c3 100644 --- a/agents/gsd-ui-auditor.md +++ b/agents/gsd-ui-auditor.md @@ -104,46 +104,6 @@ This gate runs unconditionally on every audit. The .gitignore ensures screenshot - - -## Automated Screenshot Capture via Playwright-MCP (preferred when available) - -Before attempting the CLI screenshot approach, check whether `mcp__playwright__*` -tools are available in this session. If they are, use them instead of the CLI approach: - -``` -# Preferred: Playwright-MCP automated verification -# 1. Navigate to the component URL -mcp__playwright__navigate(url="http://localhost:3000") - -# 2. Take desktop screenshot -mcp__playwright__screenshot(name="desktop", width=1440, height=900) - -# 3. Take mobile screenshot -mcp__playwright__screenshot(name="mobile", width=375, height=812) - -# 4. For specific components listed in UI-SPEC.md, navigate to each -# component route and capture targeted screenshots for comparison -# against the spec's stated dimensions, colors, and layout. - -# 5. Compare screenshots against UI-SPEC.md requirements: -# - Dimensions: Is component X width 70vw as specified? -# - Color: Is the accent color applied only on declared elements? -# - Layout: Are spacing values within the declared spacing scale? -# Report any visual discrepancies as automated findings. -``` - -**When Playwright-MCP is available:** -- Use it for all screenshot capture (skip the CLI approach below) -- Each UI checkpoint from UI-SPEC.md can be verified automatically -- Discrepancies are reported as pillar findings with screenshot evidence -- Items requiring subjective judgment are flagged as `needs_human_review: true` - -**When Playwright-MCP is NOT available:** fall back to the CLI screenshot approach -below. Behavior is unchanged from the standard code-only audit path. - - - ## Screenshot Capture (CLI only — no MCP, no persistent browser) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 462375240..8d5c9c990 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -40,7 +40,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-new-project`, `/gsd-new-milestone` | | **Parallelism** | 4 instances (stack, features, architecture, pitfalls) | -| **Tools** | Read, Write, Bash, Grep, Glob, WebSearch, WebFetch, mcp (context7) | +| **Tools** | Read, Write, Bash, Grep, Glob, Skill, WebSearch, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__*, mcp__firecrawl__*, mcp__exa__*, mcp__tavily__*, mcp__ref__*, mcp__jina__*, mcp__perplexity__* | | **Model (balanced)** | Sonnet | | **Color** | Cyan | | **Produces** | `.planning/research/STACK.md`, `FEATURES.md`, `ARCHITECTURE.md`, `PITFALLS.md` | @@ -60,7 +60,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-plan-phase` | | **Parallelism** | 4 instances (same focus areas as project researcher) | -| **Tools** | Read, Write, Bash, Grep, Glob, WebSearch, WebFetch, mcp (context7) | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, Skill, WebSearch, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__*, mcp__firecrawl__*, mcp__exa__*, mcp__tavily__*, mcp__ref__*, mcp__jina__*, mcp__perplexity__* | | **Model (balanced)** | Sonnet | | **Color** | Cyan | | **Produces** | `{phase}-RESEARCH.md` | @@ -80,7 +80,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-ui-phase` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Grep, Glob, WebSearch, WebFetch, mcp (context7) | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, Skill, WebSearch, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__*, mcp__firecrawl__*, mcp__exa__*, mcp__tavily__*, mcp__ref__*, mcp__jina__* | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | `{phase}-UI-SPEC.md` | @@ -101,7 +101,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `discuss-phase-assumptions` workflow (when `workflow.discuss_mode = 'assumptions'`) | | **Parallelism** | Single instance | -| **Tools** | Read, Bash, Grep, Glob | +| **Tools** | Read, Bash, Grep, Glob, Skill | | **Model (balanced)** | Sonnet | | **Color** | Cyan | | **Produces** | Structured assumptions with decision statements, evidence file paths, confidence levels | @@ -124,7 +124,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `discuss-phase` workflow (when ADVISOR_MODE = true) | | **Parallelism** | Multiple instances (one per gray area) | -| **Tools** | Read, Bash, Grep, Glob, WebSearch, WebFetch, mcp (context7) | +| **Tools** | Read, Bash, Grep, Glob, Skill, WebSearch, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__* | | **Model (balanced)** | Sonnet | | **Color** | Cyan | | **Produces** | 5-column comparison table (Option / Pros / Cons / Complexity / Recommendation) with rationale paragraph | @@ -146,7 +146,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-new-project` (after 4 researchers complete) | | **Parallelism** | Single instance (sequential after researchers) | -| **Tools** | Read, Write, Bash | +| **Tools** | Read, Write, Bash, Skill | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | `.planning/research/SUMMARY.md` | @@ -161,7 +161,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-plan-phase`, `/gsd-quick` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Edit, Bash, Glob, Grep, WebFetch, mcp (context7) | +| **Tools** | Read, Write, Edit, Bash, Glob, Grep, Skill, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__* | | **Model (balanced)** | Opus | | **Color** | Green | | **Produces** | `{phase}-{N}-PLAN.md` files | @@ -185,7 +185,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-new-project` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Glob, Grep | +| **Tools** | Read, Write, Bash, Glob, Grep, Skill | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | `ROADMAP.md` | @@ -206,7 +206,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-execute-phase`, `/gsd-quick` | | **Parallelism** | Multiple (parallel within waves, sequential across waves) | -| **Tools** | Read, Write, Edit, Bash, Grep, Glob | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, Skill, mcp__context7__*, mcp__plugin_context7_context7__* | | **Model (balanced)** | Sonnet | | **Color** | Yellow | | **Produces** | Code changes, git commits, `{phase}-{N}-SUMMARY.md` | @@ -230,7 +230,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-plan-phase` (verification loop, max 3 iterations) | | **Parallelism** | Single instance (iterative) | -| **Tools** | Read, Bash, Glob, Grep | +| **Tools** | Read, Bash, Glob, Grep, Skill | | **Disallowed Tools** | Write, Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Green | @@ -256,7 +256,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-audit-milestone` | | **Parallelism** | Single instance | -| **Tools** | Read, Bash, Grep, Glob | +| **Tools** | Read, Bash, Grep, Glob, Skill | | **Disallowed Tools** | Write, Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Blue | @@ -272,7 +272,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-ui-phase` (validation loop, max 2 iterations) | | **Parallelism** | Single instance | -| **Tools** | Read, Bash, Glob, Grep | +| **Tools** | Read, Bash, Glob, Grep, Skill | | **Disallowed Tools** | Write, Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Cyan | @@ -291,7 +291,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-execute-phase` (after all executors complete) | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Grep, Glob | +| **Tools** | Read, Write, Bash, Grep, Glob, Skill | | **Disallowed Tools** | Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Green | @@ -316,7 +316,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-validate-phase` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Edit, Bash, Grep, Glob | +| **Tools** | Read, Write, Edit, Bash, Glob, Grep, Skill | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | Test files, updated `VALIDATION.md` | @@ -336,7 +336,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-ui-review` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Grep, Glob | +| **Tools** | Read, Write, Bash, Grep, Glob, Skill | | **Disallowed Tools** | Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Pink | @@ -360,7 +360,7 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp |----------|-------| | **Spawned by** | `/gsd-map-codebase`, post-execute drift gate in `/gsd-execute-phase` | | **Parallelism** | 4 instances (tech, architecture, quality, concerns) | -| **Tools** | Read, Bash, Grep, Glob, Write | +| **Tools** | Read, Bash, Grep, Glob, Write, Skill | | **Model (balanced)** | Haiku | | **Color** | Cyan | | **Produces** | `.planning/codebase/*.md` (7 documents, with `last_mapped_commit` frontmatter) | @@ -388,7 +388,7 @@ runs its default whole-repo scan. |----------|-------| | **Spawned by** | `/gsd-debug`, `/gsd-verify-work` (for failures) | | **Parallelism** | Single instance (interactive) | -| **Tools** | Read, Write, Edit, Bash, Grep, Glob, WebSearch | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, Skill, WebSearch | | **Model (balanced)** | Sonnet | | **Color** | Orange | | **Produces** | `.planning/debug/*.md`, knowledge-base updates | @@ -443,7 +443,7 @@ Communication style, decision patterns, debugging approach, UX preferences, vend |----------|-------| | **Spawned by** | `/gsd-docs-update` | | **Parallelism** | Multiple instances (one per doc type) | -| **Tools** | Read, Write, Bash, Grep, Glob | +| **Tools** | Read, Bash, Grep, Glob, Write, Edit, Skill | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | Project documentation files (README, architecture, API docs, etc.) | @@ -487,7 +487,7 @@ Communication style, decision patterns, debugging approach, UX preferences, vend |----------|-------| | **Spawned by** | `/gsd-secure-phase` | | **Parallelism** | Single instance | -| **Tools** | Read, Bash, Glob, Grep | +| **Tools** | Read, Bash, Glob, Grep, Skill | | **Model (balanced)** | Sonnet | | **Color** | Red | | **Produces** | Structured verdict (SECURED / OPEN_THREATS / ESCALATE) — orchestrator writes `{phase}-SECURITY.md` (#2119) | @@ -533,7 +533,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-debug` | | **Parallelism** | Single instance (interactive, stateful) | -| **Tools** | Read, Write, Bash, Grep, Glob, Task, AskUserQuestion | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, Agent, AskUserQuestion | | **Model (balanced)** | Sonnet | | **Color** | Orange | | **Produces** | Compact summary returned to main context; evolves the `.planning/debug/{slug}.md` session file | @@ -553,7 +553,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-code-review` | | **Parallelism** | Typically single instance per review scope | -| **Tools** | Read, Write, Bash, Grep, Glob | +| **Tools** | Read, Write, Bash, Grep, Glob, Skill | | **Model (balanced)** | Sonnet | | **Color** | Orange | | **Produces** | `REVIEW.md` in the phase directory | @@ -573,7 +573,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-code-review --fix` | | **Parallelism** | Single instance | -| **Tools** | Read, Edit, Write, Bash, Grep, Glob | +| **Tools** | Read, Edit, Write, Bash, Grep, Glob, Skill | | **Model (balanced)** | Sonnet | | **Color** | Green | | **Produces** | `REVIEW-FIX.md`; one atomic git commit per applied fix | @@ -593,7 +593,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-ai-integration-phase` | | **Parallelism** | Single instance (sequential with domain-researcher / eval-planner) | -| **Tools** | Read, Write, Bash, Grep, Glob, WebFetch, WebSearch, mcp (context7) | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, WebFetch, WebSearch, mcp__context7__*, mcp__plugin_context7_context7__* | | **Model (balanced)** | Sonnet | | **Color** | Green | | **Produces** | Sections 3–4b of `AI-SPEC.md` (framework quick reference + implementation guidance) | @@ -612,7 +612,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-ai-integration-phase` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Grep, Glob, WebSearch, WebFetch, mcp (context7) | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, WebSearch, WebFetch, mcp__context7__*, mcp__plugin_context7_context7__* | | **Model (balanced)** | Sonnet | | **Color** | Purple | | **Produces** | Section 1b of `AI-SPEC.md` | @@ -631,7 +631,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-ai-integration-phase` | | **Parallelism** | Single instance (sequential after domain-researcher) | -| **Tools** | Read, Write, Bash, Grep, Glob, AskUserQuestion | +| **Tools** | Read, Write, Edit, Bash, Grep, Glob, AskUserQuestion | | **Model (balanced)** | Sonnet | | **Color** | Orange | | **Produces** | Sections 5–7 of `AI-SPEC.md` (Evaluation Strategy, Guardrails, Production Monitoring) | @@ -652,7 +652,7 @@ Twelve additional agents ship under `agents/gsd-*.md` and are used by specialty |----------|-------| | **Spawned by** | `/gsd-eval-review` | | **Parallelism** | Single instance | -| **Tools** | Read, Write, Bash, Grep, Glob | +| **Tools** | Read, Write, Bash, Grep, Glob, Skill | | **Disallowed Tools** | Edit, MultiEdit | | **Model (balanced)** | Sonnet | | **Color** | Red | diff --git a/tests/agent-classification-parity.test.cjs b/tests/agent-classification-parity.test.cjs index 2d4877f5b..8264091e8 100644 --- a/tests/agent-classification-parity.test.cjs +++ b/tests/agent-classification-parity.test.cjs @@ -366,4 +366,89 @@ describe('agent-classification-parity: AGENTS.md section structure is the single ); }); + /** + * Test 4 — AGENTS.md **Tools** rows match agent frontmatter verbatim (#2526). + * + * Same defect class as #2526 itself, one layer out: there, an agent's body + * documented a capability its `tools:` allowlist withheld; here, the role + * card documents a tool set its frontmatter disagrees with. When the review + * that found it was written, 26 of 34 rows had drifted: 22 omitted `Skill` + * and 7 omitted `Edit`; 8 omitted MCP grants entirely (7 of them writing + * "mcp (context7)" for what was up to eight distinct servers, and + * gsd-executor naming none at all); and one still read `Task`, a tool that + * no longer exists. Nothing asserted the two agreed, so the drift was free. + * + * The row must equal the frontmatter value verbatim rather than as a set: + * a set comparison would accept the "mcp (context7)" shorthand class of + * under-documentation this test exists to stop, and an exact string is what + * makes the check cheap to satisfy — copy the line. + * + * The row-count assertion is the discovery guard (mirroring #2526's own): + * without it, deleting a **Tools** row would silently retire its assertion + * while the remaining rows kept the test green. + */ + test('AGENTS.md **Tools** rows match each agent\'s tools: frontmatter (#2526)', () => { + const { parseFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); + const TOOLS_ROW = /^\|\s*\*\*Tools\*\*\s*\|\s*(.*?)\s*\|\s*$/; + const H3 = /^###\s+(gsd-[\w-]+)\s*$/; + + // /\r?\n/, not '\n': Windows autocrlf yields CRLF, and a trailing \r would + // survive into the row's last cell and fail every comparison (local/no-crlf-fragile-split). + const lines = rawAgentsMd.split(/\r?\n/); + const sections = []; + lines.forEach((line, i) => { + const m = H3.exec(line); + if (m) sections.push({ slug: m[1], start: i }); + }); + sections.forEach((s, i) => { + s.end = i + 1 < sections.length ? sections[i + 1].start : lines.length; + }); + + const mismatches = []; + const missingRow = []; + + for (const section of sections) { + const agentPath = path.join(AGENTS_DIR, `${section.slug}.md`); + if (!fs.existsSync(agentPath)) continue; // phantom headings are test 3's job + + const fm = parseFrontmatter(fs.readFileSync(agentPath, 'utf8')) || {}; + // No `tools:` key at all means "inherits everything" — there is no + // declared set for the row to agree with, so nothing to assert. + if (fm.tools === undefined || fm.tools === null) continue; + const declared = Array.isArray(fm.tools) ? fm.tools.join(', ') : String(fm.tools); + + let row = null; + for (let i = section.start; i < section.end; i += 1) { + const m = TOOLS_ROW.exec(lines[i]); + if (m) { row = { value: m[1], line: i + 1 }; break; } + } + + if (row === null) { missingRow.push(section.slug); continue; } + if (row.value !== declared) { + mismatches.push( + ` ${section.slug} (docs/AGENTS.md:${row.line})\n` + + ` doc: ${row.value}\n` + + ` frontmatter: ${declared}`, + ); + } + } + + assert.deepStrictEqual( + missingRow, + [], + 'AGENTS.md sections with a granted tools: frontmatter but no "| **Tools** |" row — ' + + 'the row cannot be allowed to vanish, or its parity assertion vanishes with it: ' + + JSON.stringify(missingRow), + ); + + assert.strictEqual( + mismatches.length, + 0, + 'docs/AGENTS.md **Tools** rows disagree with the agents\' tools: frontmatter.\n' + + 'The role card documents a tool set the agent does not have (or omits one it does) — ' + + 'the #2526 drift class at the doc layer. Copy the frontmatter value verbatim:\n' + + mismatches.join('\n'), + ); + }); + }); diff --git a/tests/mcp-tool-inheritance.test.cjs b/tests/mcp-tool-inheritance.test.cjs index b5d32192a..6fe148957 100644 --- a/tests/mcp-tool-inheritance.test.cjs +++ b/tests/mcp-tool-inheritance.test.cjs @@ -169,3 +169,338 @@ describe('researcher Step-C dispatch ↔ tools frontmatter parity (#1284)', () = }); } }); + +// --- Regression (#2526): the generalization of the #1284 check above, applied +// to EVERY agent and its WHOLE body rather than two researchers and one table. +// +// gsd-ui-auditor declared `tools: Read, Write, Bash, Grep, Glob, Skill` — no +// mcp__* grant of any kind — while its body presented a +// block as the *preferred* capture path. That branch +// was unreachable by construction: the availability check had a fixed answer, +// the three mcp__playwright__* calls could never dispatch, and the "when +// Playwright-MCP is NOT available" fallback was the only branch that ever ran. +// +// Frontmatter is read through the canonical parser (gsd-core/bin/lib/ +// frontmatter.cjs), not a hand-rolled scan, so every valid YAML shape — +// inline CSV, block sequence, flow array, quoted scalar, commented-out key — +// is handled by construction rather than by accumulating regex special cases. +// +// Deliberate scope boundaries (each keeps the check honest rather than merely +// broad; every one is exercised by the negative controls below): +// * The scanned surface is the BODY plus the frontmatter `description`, which +// ships and is read by the dispatcher. The rest of the frontmatter is not +// scanned: `tools:` is the grant list itself and would self-reference. +// * SERVER-level, not exact-tool. A `mcp__playwright__navigate` grant counts +// as granting the `playwright` server. The bug class here is a server with +// ZERO grants; asserting exact tool names is a stricter, separate invariant. +// * A body reference must carry the trailing `__` of a real tool name +// (`mcp__playwright__navigate`). Bare prose naming a server is not an +// invocation and is not flagged. +// * A single-character server id is a prose metavariable, not a reference: +// gsd-phase-researcher legitimately writes "for any other provider id `X` +// ... use `mcp__X__*`". Real server ids are longer. (Same false-positive +// hazard #1284 avoids by scoping to table rows.) +// A MULTI-character placeholder is spelled `mcp____*` — the +// angle-bracket form is the sanctioned convention, and it is exempt by +// construction because `<` lies outside REFERENCE_RE's character class. +// `mcp__SERVER__*` is deliberately NOT exempt: an all-caps escape hatch +// would be a false NEGATIVE for any real server that happens to be spelled +// in caps, and a guard that misses a dead reference fails in the direction +// this whole check exists to prevent. Failing loudly on the bare-caps form +// costs one author one message, which names the convention. +// * MCP namespaces only. Built-in tool names (Read, Bash, Skill) are ordinary +// English words that appear throughout agent prose and would be pure noise. +// --- +describe('agent tools: allowlist covers every documented MCP namespace (#2526)', () => { + const { parseFrontmatter, stripFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); + const AGENTS_DIR = path.join(__dirname, '..', 'agents'); + + // A tool name is `mcp____`; `` may contain underscores + // and hyphens (mcp__plugin_context7_context7__, mcp__chrome-devtools__). + const REFERENCE_RE = /mcp__([A-Za-z0-9_-]+?)__/g; + + // Sentinel for "this allowlist grants every MCP server". Safe as a Set member + // alongside real server ids: `*` is outside REFERENCE_RE's character class, so + // no body reference can ever produce it and collide. + const GRANT_ALL = '*'; + + // The frontmatter parser preserves an INLINE comment inside a scalar value + // (`tools: Read # mcp__playwright__*` parses as the literal string + // `Read # mcp__playwright__*`), so a commented-out grant would otherwise read + // as a real one. Full-line comments are already dropped by the parser. + const stripInlineComment = (s) => String(s).replace(/\s+#.*$/, ''); + + /** `tools:` as a token list. null = no tools: key at all (inherits everything). */ + function toolTokens(tools) { + if (tools === undefined || tools === null) return null; + const items = Array.isArray(tools) ? tools : [tools]; + return items + .flatMap((t) => stripInlineComment(t).split(/[,\s]+/)) + .map((t) => t.trim()) + .filter(Boolean); + } + + /** Server ids granted, from any accepted grant spelling. `GRANT_ALL` = every server. */ + function grantedServers(tokens) { + const servers = new Set(); + for (const token of tokens) { + if (!token.startsWith('mcp__')) continue; + // A bare `mcp__*` is a wildcard over EVERY server, not a grant of the + // empty-string server id. Without this branch it strips to '' and is + // dropped by the `if (server)` guard below, so the one grant spelling + // that plainly covers any body would flag every reference in it — + // inverting the guard against a correct agent. A bare `mcp__` (no star) + // is a typo rather than a wildcard and keeps failing closed. + if (/^mcp__\*+$/.test(token)) { servers.add(GRANT_ALL); continue; } + const rest = token.slice('mcp__'.length).replace(/\*+$/, ''); + // `mcp__srv__*` and `mcp__srv__tool` both grant `srv`; so does bare `mcp__srv`. + const server = rest.includes('__') ? rest.slice(0, rest.indexOf('__')) : rest; + if (server) servers.add(server.toLowerCase()); + } + return servers; + } + + /** Server ids a body references as tool namespaces. */ + function referencedServers(body) { + const servers = new Set(); + for (const m of body.matchAll(REFERENCE_RE)) { + if (m[1].length === 1) continue; // prose metavariable, e.g. mcp__X__* + servers.add(m[1].toLowerCase()); + } + return servers; + } + + /** MCP servers an agent documents but does not grant. Empty = consistent. */ + function ungrantedServers(content) { + const fm = parseFrontmatter(content) || {}; + const tokens = toolTokens(fm.tools); + if (tokens === null) return []; + const granted = grantedServers(tokens); + if (granted.has(GRANT_ALL)) return []; + // `description` ships with the agent and the dispatcher reads it, so an + // mcp__ reference there is exactly as dead as one in the body. Only that + // one field is scanned, never the whole frontmatter: `tools:` legitimately + // contains the grants themselves and would self-reference into a + // guaranteed pass. + const documented = `${String(fm.description ?? '')}\n${stripFrontmatter(content)}`; + return [...referencedServers(documented)] + .filter((s) => !granted.has(s)) + .sort(); + } + + const agentFiles = fs.readdirSync(AGENTS_DIR).filter((f) => f.endsWith('.md')).sort(); + + // Discovery guard: without this, a reorganisation that empties agentFiles + // would silently delete every real assertion below while the synthetic + // controls kept the suite green. Mirrors #1284's `referenced.size > 0`. + test('agent discovery finds the agent definitions to check', () => { + assert.ok(agentFiles.length >= 20, + `expected agents/ to hold the agent definitions, found ${agentFiles.length}`); + const anyGrant = agentFiles.some((f) => { + const tokens = toolTokens((parseFrontmatter( + fs.readFileSync(path.join(AGENTS_DIR, f), 'utf8')) || {}).tools); + return tokens !== null && grantedServers(tokens).size > 0; + }); + assert.ok(anyGrant, 'no agent grants any mcp__ namespace — the grant parser is not matching'); + }); + + for (const file of agentFiles) { + test(`${file}: documents no MCP namespace its tools: line withholds`, () => { + const ungranted = ungrantedServers(fs.readFileSync(path.join(AGENTS_DIR, file), 'utf8')); + assert.deepStrictEqual(ungranted, [], + `${file} documents mcp__${ungranted.join('__*, mcp__')}__* but its tools: allowlist ` + + 'grants none of them — those calls can never dispatch, so the instruction is dead ' + + 'and invites the agent to claim a path it cannot take (#2526). Either grant the ' + + 'namespace, drop the block, or — if this is a prose placeholder rather than a real ' + + 'server — spell it `mcp____*`, the angle-bracket form this check ignores.'); + }); + } + + // Negative controls — these keep the property check from decaying into a + // vacuous pass by proving the checker still FIRES, and still stays quiet, on + // synthetic inputs independent of whatever agents/ happens to contain. + describe('checker fires on known-bad input', () => { + const agent = (fm, body) => ['---', ...fm, '---', '', ...body].join('\n'); + + test('flags the #2526 shape: MCP block under an MCP-less allowlist', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: gsd-ui-auditor', 'tools: Read, Write, Bash, Grep, Glob, Skill'], + ['Check whether `mcp__playwright__*` tools are available in this session.', + 'mcp__playwright__navigate(url="http://localhost:3000")'])), + ['playwright']); + }); + + test('accepts the same body once the namespace is granted', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: a', 'tools: Read, mcp__playwright__*'], + ['mcp__playwright__navigate(url="http://localhost:3000")'])), + []); + }); + + test('an exact-tool grant covers its server (documented server-level scope)', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: mcp__playwright__navigate'], + ['mcp__playwright__screenshot(name="desktop")'])), + []); + }); + + test('a bare server-wide grant is recognised', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read, mcp__playwright'], + ['mcp__playwright__navigate()'])), + []); + }); + + // `mcp__*` strips to the empty string; without the wildcard branch it is + // dropped as a grant of nothing, and the guard fires against an allowlist + // that plainly covers the body — the one input shape that inverts it. + test('a bare mcp__* wildcard grants every server', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read, mcp__*'], + ['mcp__playwright__navigate()', 'mcp__chrome-devtools__take_screenshot()'])), + []); + }); + + test('a bare mcp__ without a wildcard is a typo, not a grant', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read, mcp__'], + ['mcp__playwright__navigate()'])), + ['playwright']); + }); + + test('reads block-sequence tools:, not just the inline CSV form', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: a', 'tools:', ' - Read', ' - mcp__context7__*', 'color: pink'], + ['Use mcp__context7__resolve-library-id, never mcp__tavily__search.'])), + ['tavily']); + }); + + test('reads a YAML flow array', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: [Read, mcp__context7__*]'], + ['mcp__context7__get-library-docs()'])), + []); + }); + + test('reads a quoted scalar tools: value', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: "Read, mcp__playwright__*"'], + ['mcp__playwright__navigate()'])), + []); + }); + + test('server ids with hyphens are matched, not silently skipped', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['mcp__chrome-devtools__take_screenshot()'])), + ['chrome-devtools']); + }); + + test('a commented-out grant does not count as granted (full-line form)', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: a', 'tools: Read', '# tools: mcp__playwright__*'], + ['mcp__playwright__navigate()'])), + ['playwright']); + }); + + test('a commented-out grant does not count as granted (inline form)', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: a', 'tools: Read # mcp__playwright__* withheld'], + ['mcp__playwright__navigate()'])), + ['playwright']); + }); + + // Boundary 2: a bare prose mention names a server without invoking it. + test('bare prose naming a server is not treated as a reference', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['Screenshots come from the mcp__playwright server when the operator configures it.'])), + []); + }); + + // Boundary 4: built-in tool names are ordinary English and must stay silent. + test('built-in tool names in prose are never flagged', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['Use Write and Bash to Edit the file, then Grep and Glob for the results.'])), + []); + }); + + test('ignores prose metavariables like mcp__X__*', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read, mcp__exa__*'], + ['For any other provider id `X`: use `mcp__X__*` if available, else WebSearch.'])), + []); + }); + + // The sanctioned spelling for a MULTI-character placeholder. Exempt by + // construction — `<` is outside REFERENCE_RE's character class — so this + // pins an existing property rather than adding a special case. + test('the angle-bracket placeholder form is not a reference', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['For any provider: use `mcp____*` when it is configured.'])), + []); + }); + + // The deliberate other half: a bare all-caps id still fires. Exempting it + // would be a false negative for any real server spelled in caps, and the + // failure message names the angle-bracket form instead. + test('a bare all-caps placeholder is still flagged, and fails closed', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['For any provider `SERVER`: use `mcp__SERVER__*` when configured.'])), + ['server']); + }); + + // The metavariable exclusion is length===1 exactly: two characters is the + // shortest server id that must still be recognized as a real reference. + test('a two-character server id is a reference, not a metavariable', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['mcp__ab__foo()'])), + ['ab']); + }); + + // Limit-1, completing the boundary triple (0 / 1 / 2). A zero-length id is + // unrepresentable by REFERENCE_RE — `+?` requires at least one character — + // so it is skipped by the pattern, never by the length===1 exclusion. + test('a zero-length server id is not representable, and not a reference', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read'], + ['mcp____foo()'])), + []); + }); + + // The description ships and is read by the dispatcher, so it is part of + // what the agent "documents" — scanning only the body left it exempt. + test('an ungranted namespace in the description is flagged', () => { + assert.deepStrictEqual( + ungrantedServers(agent( + ['name: a', 'description: Captures screens via mcp__playwright__navigate.', + 'tools: Read'], + ['The body names no MCP tool at all.'])), + ['playwright']); + }); + + // The `tools:` line is a grant list, not documentation of a call — scanning + // the whole frontmatter would let every allowlist satisfy itself. + test('the tools: line itself is never read as a body reference', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a', 'tools: Read, mcp__context7__*'], + ['No MCP call appears in this body.'])), + []); + }); + + test('an agent with no tools: key inherits everything', () => { + assert.deepStrictEqual( + ungrantedServers(agent(['name: a'], ['mcp__playwright__navigate()'])), + []); + }); + }); +});