golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2526): remove gsd-ui-auditor's uncallable Playwright-MCP block (#2594)

Browse Source 
* 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
`<playwright_mcp_approach>` 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__<server>__*` 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__<SERVER>__*` 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 <trekkie@nomorestars.com>
This commit is contained in:
0xdhx
2026-07-28 17:02:01 -05:00
committed by GitHub
parent 1c1af70a4b
commit 3274db2757
5 changed files with 451 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/2526-ui-auditor-dead-mcp-block.md Normal file
View File

@@ -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 `<playwright_mcp_approach>` 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__<server>__*` 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)

40
agents/gsd-ui-auditor.md
View File

@@ -104,46 +104,6 @@ This gate runs unconditionally on every audit. The .gitignore ensures screenshot
</gitignore_gate>
<playwright_mcp_approach>
## 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.
</playwright_mcp_approach>
<screenshot_approach>
## Screenshot Capture (CLI only — no MCP, no persistent browser)

52
docs/AGENTS.md
View File

@@ -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 |

85
tests/agent-classification-parity.test.cjs
View File

@@ -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'),
);
});
});

335
tests/mcp-tool-inheritance.test.cjs
View File

@@ -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
// <playwright_mcp_approach> 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__<SERVER>__*` — 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__<server>__<tool>`; `<server>` 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__<SERVER>__*`, 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__<SERVER>__*` 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()'])),
[]);
});
});
});