diff --git a/.changeset/brave-geese-bark.md b/.changeset/brave-geese-bark.md new file mode 100644 index 000000000..a6c881245 --- /dev/null +++ b/.changeset/brave-geese-bark.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3237 +--- +**`gsd-plan-checker` now flags same-wave plans that are coupled but don't say so** — two plans in the same wave that share mutable state (a config key, table, migration, env var, singleton) or depend on each other's execution order, with no `depends_on` edge between them, are reported as an advisory Dimension 3 finding. The coupling gets settled at plan time instead of surfacing as an intermittent failure during parallel execution. `docs/AGENTS.md`'s plan-checker entry, which claimed eight verification dimensions and listed eight names matching none of the agent's actual fifteen, is corrected to the real list in the same change. (#1954) diff --git a/agents/gsd-plan-checker.md b/agents/gsd-plan-checker.md index f15b76904..1f18ea591 100644 --- a/agents/gsd-plan-checker.md +++ b/agents/gsd-plan-checker.md @@ -210,6 +210,42 @@ issue: fix_hint: "Plan 02 depends on 03, but 03 depends on 02" ``` +## Dimension 3b: Undeclared / Temporal Coupling + +**Question:** Do two same-wave plans depend on each other through shared mutable state or +execution order without declaring it? Dimension 3 checks *declared* edges and the wave guard +checks `files_modified` overlap; neither sees an undeclared edge, which under parallel +execution becomes an intermittent failure nobody can attribute. + +**Scope: PLAN pairs, not tasks.** Tasks inside one plan run sequentially and cannot race. +Compare same-wave plan pairs over the union of their tasks' `` and ``. + +**FLAG only when ALL THREE hold** (coupling that is strong *and* non-local — Connascence of +Execution; strong-but-local coupling inside one plan is fine): +1. both plans sit in the same wave, and +2. neither declares `depends_on` on the other, and +3. their actions name a *specific* shared mutable resource (config key, table/row, migration, + env var, singleton, cache) with at least one WRITER, or one names a prerequisite the other + produces. + +**Do NOT flag:** both sides only READ it, or it is immutable; the pair already overlaps in +`files_modified` (report that once, on the file axis); the plans sit in a different wave, which +already orders them; two tasks inside one plan; a vague same-subsystem claim naming no +resource; incompatible *transformations* of one entity — that is Dimension 9. + +**Severity: ALWAYS WARNING, never a blocker.** Coupling is sometimes intentional; the finding +lets the planner declare the edge, move a plan to a later wave, or justify the pair. + +```yaml +issue: + dimension: dependency_correctness + severity: warning + description: "Plans 02 and 03 are both Wave 1 with no depends_on, but 02 writes config key + auth.session_ttl and 03 reads it" + plans: ["02", "03"] + fix_hint: "Declare depends_on, move 03 to a later wave, or justify either order" +``` + ## Dimension 4: Key Links Planned **Question:** Are artifacts wired together, not just created in isolation? @@ -1036,6 +1072,7 @@ Plan verification complete when: - [ ] Requirement coverage checked (all requirements have tasks) - [ ] Task completeness validated (all required fields present) - [ ] Dependency graph verified (no cycles, valid references) +- [ ] Undeclared/temporal coupling checked (same-wave plan pairs, advisory) - [ ] Key links checked (wiring planned, not just artifacts) - [ ] Scope assessed (within context budget) - [ ] must_haves derivation verified (user-observable truths) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index e093f5f62..62b30ee17 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -237,15 +237,28 @@ GSD uses a multi-agent architecture where thin orchestrators (workflow files) sp | **Color** | Green | | **Produces** | PASS/FAIL verdict with specific feedback | -**8 Verification Dimensions:** -1. Requirement coverage -2. Task atomicity -3. Dependency ordering -4. File scope -5. Verification commands -6. Context fit -7. Gap detection -8. Nyquist compliance (when enabled) +**Verification Dimensions** — labels match the agent's own `## Dimension ` headings: + +| # | Dimension | +|---|---| +| 1 | Requirement coverage | +| 2 | Task completeness | +| 3 | Dependency correctness | +| 3b | Undeclared / temporal coupling — advisory; flags same-wave plan pairs coupled through shared mutable state or execution order with no `depends_on` between them | +| 4 | Key links planned | +| 5 | Scope sanity | +| 6 | Verification derivation | +| 7 | Context compliance (when CONTEXT.md exists) | +| 7b | Scope reduction detection | +| 7c | Architectural tier compliance (when RESEARCH.md defines a responsibility map) | +| 8 | Nyquist compliance (when enabled) | +| 9 | Cross-plan data contracts | +| 10 | CLAUDE.md compliance | +| 11 | Research resolution | +| 12 | Pattern compliance | + +Two further dimensions carry no number: **Verify Command Format Sanity** and +**Numeric/Factual Claim Authority**. --- diff --git a/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json b/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json new file mode 100644 index 000000000..52f878a8c --- /dev/null +++ b/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "gsd-plan-checker.md": "#1954: Dimension 3 gains sub-dimension 3b (undeclared / temporal coupling), plus one success-criteria line. Growth is the new sub-dimension only — no existing text was rewritten. The addition is deliberately dense rather than extracted: ADR-1610 Decision 4 names eager `@`-import relocation as proxy-gaming (it shrinks the measured file while leaving loaded context unchanged or larger), legitimate extraction is Read-at-step lazy, and this agent has no lazy-read seam; issue #1954's approved scope is also explicitly 'No new files'. After the change the file sits at 48810 bytes against the LARGE tier hard cap of 49152 (tests/agent-size-budget.test.cjs), i.e. 342 bytes of headroom. That is deliberate and disclosed: the cap is not crossed and is not raised, but the next contributor who needs room in this agent must do a lazy extraction rather than add prose. Content justification: Dimension 3 proves declared dependency edges resolve and are acyclic, and execute-phase's intra-wave guard proves same-wave plans do not overlap in files_modified — neither axis sees an undeclared edge, so two same-wave plans coupled through a shared config key, table, migration, env var, singleton or cache (or through one plan's produced state) pass plan-check and fail intermittently under parallel execution. 3b is advisory only (WARNING, never blocker, per the issue's rejected-alternatives list) and reuses the existing dependency_correctness finding key so no consumer sees a new dimension." + } +} diff --git a/tests/emitted-drift-acks/2962-zsh-nomatch-for-glob-portability.json b/tests/emitted-drift-acks/2962-zsh-nomatch-for-glob-portability.json index cda74a468..919db6e7d 100644 --- a/tests/emitted-drift-acks/2962-zsh-nomatch-for-glob-portability.json +++ b/tests/emitted-drift-acks/2962-zsh-nomatch-for-glob-portability.json @@ -2,7 +2,6 @@ "version": 1, "paths": { "gsd-integration-checker.md": "#2962: 1 bash block (line ~98 SUMMARY iteration) gained the nullglob shim for zsh portability of the for-glob loop.", - "gsd-plan-checker.md": "#2962: 3 bash blocks (lines ~716, ~737, ~822) gained the nullglob shim for zsh portability of the for-glob loops.", "resume-project.md": "#2962: 1 bash block (line ~66 plans-without-summaries scan) gained the nullglob shim for zsh portability of the for-glob loop.", "complete-milestone.md": "#2962: 1 bash block (line ~221 summary one-liner extraction) gained the nullglob shim for zsh portability of the for-glob loop.", "audit-milestone.md": "#2962: 1 bash block (line ~126 requirements_completed extraction) gained the nullglob shim for zsh portability of the for-glob loop." diff --git a/tests/plan-checker-coupling.test.cjs b/tests/plan-checker-coupling.test.cjs new file mode 100644 index 000000000..1778d0bbd --- /dev/null +++ b/tests/plan-checker-coupling.test.cjs @@ -0,0 +1,289 @@ +// allow-test-rule: source-text-is-the-product — the plan-checker is a prompt; its .md text IS what the runtime loads (#1954) + +/** + * Dimension 3b — undeclared / temporal coupling between same-wave plans (#1954). + * + * `gsd-plan-checker` proves plan dependencies resolve and are acyclic (Dimension 3), + * and `/gsd:execute-phase` separately proves same-wave plans do not overlap in + * `files_modified`. Neither axis sees coupling that is real but undeclared — plan A + * writes a config key / table / migration / global module that plan B reads, or B + * only works if A ran first. In parallel execution that surfaces as an intermittent + * failure the executor cannot attribute. + * + * ## What this suite locks + * + * The deployed contract AND the wiring between the three regions of the agent doc that + * must agree: the sub-check body, its severity rule, and the `` + * checklist. A sub-check present in the body but absent from `success_criteria` is a + * check the agent is never told to run; the reverse is a checklist item with no rubric. + * Neither half is observable from the other, which is why both are asserted here. + * + * ## What it cannot prove + * + * That the model acts on the text. The subject is an LLM prompt — no test in this repo + * can prove behavior for any of the agent's twelve existing dimensions either. Stated + * so the coverage claim is honest rather than implied. + * + * Patterns are CRLF-tolerant (`\r?\n`): the runtime loads the file whole, including on + * a checkout that produced CRLF, the same case `scripts/workflow-size.cjs` defends. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const ROOT = path.join(__dirname, '..'); +const AGENT_PATH = path.join(ROOT, 'agents', 'gsd-plan-checker.md'); +const DOCS_AGENTS_PATH = path.join(ROOT, 'docs', 'AGENTS.md'); + +const agentDoc = fs.readFileSync(AGENT_PATH, 'utf-8'); +const docsAgents = fs.readFileSync(DOCS_AGENTS_PATH, 'utf-8'); + +// ── Span helpers ─────────────────────────────────────────────────── +// Offsets of the headings that bound each region. `indexOfHeading` returns -1 when +// absent so a missing heading fails as a named assertion rather than an off-by-one. +function indexOfHeading(content, pattern) { + const m = content.match(pattern); + return m && typeof m.index === 'number' ? m.index : -1; +} + +const D3_HEADING = /^## Dimension 3: /m; +const D3B_HEADING = /^## Dimension 3b: /m; +const D4_HEADING = /^## Dimension 4: /m; + +function sliceBetween(content, startPattern, endPattern) { + const start = indexOfHeading(content, startPattern); + const end = indexOfHeading(content, endPattern); + assert.ok(start >= 0, `start heading not found: ${startPattern}`); + assert.ok(end >= 0, `end heading not found: ${endPattern}`); + assert.ok(end > start, `end heading precedes start heading: ${startPattern} .. ${endPattern}`); + return content.slice(start, end); +} + +/** + * Count top-level ordered-list items in a span. This is the trigger gate's arity — + * the "flag only when ALL N hold" conjunction. Widening it from 3 to 2 is what turns + * a precise heuristic into a noise generator, so the count is asserted, not the prose. + */ +function countOrderedItems(span) { + const matches = span.match(/^\d+\. /gm); + return matches ? matches.length : 0; +} + +/** + * Remove fenced code blocks before scanning for headings. The agent's + * `### Dimension 8 Output` section embeds a literal `## Dimension 8: ...` line inside a + * fence as its output template, so a fence-blind scan reports Dimension 8 twice and any + * uniqueness or completeness check built on it is wrong before it starts. + */ +function stripFences(content) { + return content.replace(/^```[\s\S]*?^```/gm, ''); +} + +describe('gsd-plan-checker Dimension 3b — undeclared/temporal coupling (#1954)', () => { + describe('the sub-check exists and is scoped to Dimension 3', () => { + test('Dimension 3 carries an undeclared-coupling sub-check', () => { + const heading = agentDoc.match(/^## Dimension 3b: (.+)$/m); + assert.ok(heading, 'agents/gsd-plan-checker.md must define a "## Dimension 3b:" heading'); + assert.match( + heading[1], + /coupling/i, + `Dimension 3b must name coupling as its subject, got: ${heading[1]}` + ); + }); + + test('the sub-check sits inside Dimension 3, not after it', () => { + const d3 = indexOfHeading(agentDoc, D3_HEADING); + const d3b = indexOfHeading(agentDoc, D3B_HEADING); + const d4 = indexOfHeading(agentDoc, D4_HEADING); + assert.ok(d3 >= 0 && d3b >= 0 && d4 >= 0, 'Dimensions 3, 3b and 4 must all be present'); + assert.ok(d3 < d3b, 'Dimension 3b must follow Dimension 3'); + assert.ok(d3b < d4, 'Dimension 3b must precede Dimension 4 — it extends Dimension 3'); + }); + + test('the sub-check scopes comparison to plan pairs', () => { + // Parallelism is per PLAN (execute-phase spawns one executor per plan per wave), + // so two tasks inside one plan run sequentially and cannot race. Scoping the + // comparison to task pairs would flag orderings that are guaranteed by construction. + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match( + span, + /Scope:\s*PLAN pairs, not tasks/, + 'Dimension 3b must state that the comparison is plan-pair scoped' + ); + }); + }); + + describe('the trigger gate is a three-way conjunction', () => { + test('the trigger gate enumerates exactly three conditions', () => { + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.strictEqual( + countOrderedItems(span), + 3, + 'Dimension 3b must gate on exactly three AND-ed conditions ' + + '(same wave, no declared edge, named shared mutable resource or produced-state ' + + 'prerequisite). Dropping one widens the heuristic into noise; adding one ' + + 'silently narrows what it can catch.' + ); + }); + + test('condition counter fires at 2 / 3 / 4', () => { + // The assertion above can only ever observe the real doc's arity, so its + // inequality branch never executes. Exercise the counter at limit-1 / limit / + // limit+1 (RULESET.TESTS.boundary-coverage) through the SAME function the guard + // uses, in both LF and CRLF form, so a future edit cannot neuter it. + const item = (n) => `${n}. condition ${n}`; + for (const eol of ['\n', '\r\n']) { + const spanOf = (count) => + ['## Dimension 3b: heading', ...Array.from({ length: count }, (_, i) => item(i + 1))] + .join(eol); + assert.strictEqual(countOrderedItems(spanOf(2)), 2, `2 items must count as 2 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countOrderedItems(spanOf(3)), 3, `3 items must count as 3 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countOrderedItems(spanOf(4)), 4, `4 items must count as 4 (eol=${JSON.stringify(eol)})`); + } + }); + }); + + describe('severity is advisory, and stays advisory', () => { + test('the sub-check finding is a warning', () => { + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match( + span, + /severity:\s*warning/, + 'Dimension 3b\'s example issue must carry severity: warning' + ); + }); + + test('the sub-check forbids escalating to blocker', () => { + // The agent's own penalises "issuing warnings for what are + // actually blockers", which biases the model to escalate. Issue #1954 rejected the + // hard-block alternative outright, so the prohibition has to be explicit in the span. + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match( + span, + /never\s+(a\s+)?blocker/i, + 'Dimension 3b must state that the finding is never a blocker' + ); + assert.doesNotMatch( + span, + /severity:\s*blocker/, + 'Dimension 3b must not contain a blocker-severity example — it is advisory only' + ); + }); + + test('existing Dimension 3 blocker severities are unchanged', () => { + // Independence: 3b is additive. The circular-dependency finding above it must + // still block, or this change quietly downgraded a real gate. + const d3Body = sliceBetween(agentDoc, D3_HEADING, D3B_HEADING); + assert.match( + d3Body, + /severity:\s*blocker/, + 'Dimension 3\'s own example issue must still be severity: blocker' + ); + assert.match( + d3Body, + /Circular dependency/, + 'Dimension 3 must still carry its circular-dependency example' + ); + }); + + test('the finding reuses the dependency_correctness dimension key', () => { + // Hyrum: anything consuming the checker's structured issues keys on `dimension`. + // A new key would be a new observable contract; the issue asked for a finding + // "under Dimension 3", not a new dimension. + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match( + span, + /dimension:\s*dependency_correctness/, + 'Dimension 3b\'s example issue must reuse the dependency_correctness dimension key' + ); + }); + }); + + describe('negative space is enumerated', () => { + test('the sub-check enumerates its non-triggering cases', () => { + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match(span, /Do NOT flag/, 'Dimension 3b must carry an explicit non-triggering list'); + // Each token below is a distinct exclusion class from the design's negative space. + // Their absence is what produced false positives in the alternatives considered. + for (const [token, why] of [ + [/files_modified/, 'the file-overlap axis is already checked elsewhere — report it once'], + [/depends_on/, 'a pair whose edge is already declared is not a finding'], + [/different wave/i, 'the wave itself already orders the pair'], + [/READ/, 'two readers of a shared resource are not coupled'], + ]) { + assert.match(span, token, `Dimension 3b non-triggering list must cover: ${why}`); + } + }); + + test('the sub-check defers transform conflicts to Dimension 9', () => { + // Dimension 9 (Cross-Plan Data Contracts) owns incompatible transformations of a + // shared entity. Without this boundary the same plan pair is reported twice. + const span = sliceBetween(agentDoc, D3B_HEADING, D4_HEADING); + assert.match( + span, + /Dimension 9/, + 'Dimension 3b must defer incompatible-transform findings to Dimension 9' + ); + }); + }); + + describe('the sub-check is wired into the agent\'s completion checklist', () => { + test('success_criteria includes the coupling check', () => { + const span = sliceBetween(agentDoc, //, /<\/success_criteria>/); + assert.match( + span, + /^- \[ \] .*coupling.*$/im, + 'the checklist must carry a line for the coupling check — ' + + 'a rubric the agent is never told to run is not a check' + ); + }); + }); + + describe('docs parity', () => { + // Parity against the agent's own headings is self-maintaining; a hand-typed count is + // not. The section previously claimed "8 Verification Dimensions" while enumerating + // names that matched no dimension in the agent at all — a stale count reads as + // authoritative, which is worse than no count. + function agentDimensionLabels() { + return [...stripFences(agentDoc).matchAll(/^## Dimension ([0-9]+[a-z]?): /gm)].map((m) => m[1]); + } + + test('the agent defines a discoverable set of numbered dimensions', () => { + const labels = agentDimensionLabels(); + assert.ok( + labels.length >= 12, + `expected the agent to define at least 12 numbered dimensions, found ${labels.length}` + ); + assert.ok(labels.includes('3b'), 'Dimension 3b must be among the agent\'s numbered dimensions'); + assert.strictEqual( + new Set(labels).size, + labels.length, + `duplicate dimension labels in the agent: ${labels.join(', ')}` + ); + }); + + test('docs/AGENTS.md enumerates every dimension the agent defines', () => { + const section = sliceBetween(docsAgents, /^### gsd-plan-checker$/m, /^### gsd-integration-checker$/m); + const documented = new Set( + [...section.matchAll(/^\| ([0-9]+[a-z]?) \| /gm)].map((m) => m[1]) + ); + const missing = agentDimensionLabels().filter((label) => !documented.has(label)); + assert.deepStrictEqual( + missing, + [], + `docs/AGENTS.md omits dimension(s): ${missing.join(', ')}` + ); + }); + + test('docs/AGENTS.md documents the coupling check', () => { + const section = sliceBetween(docsAgents, /^### gsd-plan-checker$/m, /^### gsd-integration-checker$/m); + assert.match( + section, + /coupling/i, + 'docs/AGENTS.md\'s gsd-plan-checker section must document the coupling check' + ); + }); + }); +});