From 9640968f8eee9c692800145544c81a0a2a6b1ddc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 3 Aug 2026 09:16:29 -0400 Subject: [PATCH] fix(#2847): require gap_closure value in plan-gap-closure schema and bind validate_plan to it (#3018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2847): add failing-first regression tests for gap-closure frontmatter schema gap --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, and plan-phase.md's downstream_consumer contract never mentioned it either, so gap-closure plans could pass validation while missing the field that /gsd:execute-phase --gaps-only filters on. These tests are RED against current production code: no plan-gap-closure schema exists yet, and neither agents/gsd-planner.md's validate_plan step nor plan-phase.md's downstream_consumer block references gap_closure conditionally. * fix(#2847): enforce gap_closure via plan-gap-closure schema --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, so a gap-closure plan could pass validation while missing the field /gsd:execute-phase --gaps-only filters on, silently spawning zero executors. Add a plan-gap-closure schema (every plan-required field plus gap_closure) and make the planner's validate_plan step select it when gap_closure mode is active, plan otherwise. Standard/reviews-mode plans are unaffected: plan's required fields are unchanged. plan-phase.md's downstream_consumer block was investigated for a symmetric mention but deliberately left untouched: it sits 36 bytes under the frozen ADR-857 PRE_PHASE6 ceiling and the validate_plan step in gsd-planner.md is the actual call site, needing no help from plan-phase.md's prose. * fix(#2847): compact validate_plan edit under gsd-planner.md size caps Merging origin/next (7 commits, including #2775's gsd-planner.md STRIDE-row edit) left only 22 chars of headroom under four separate hard-coded 49152-char caps on gsd-planner.md (planner-decomposition, precondition-element, reversibility-tagging, security.test.cjs). The verbose validate_plan prose from the previous commit overran all four. Compact the edit to a single line (net +17 chars vs origin/next) while keeping the functional content: schema name, mode condition, and the unchanged base required-fields list. Also: - Fix a real bug in the fix-2847 negative-assertion test: plan-phase.md mentions the literal string "" twice in backtick-quoted prose before the actual opening tag, so a plain indexOf() grabbed the wrong start position and swallowed ~10KB of unrelated content (including a "gap_closure" hit in a Mode: enum line), producing a false failure. Anchor on the tag starting its own line instead. - Merge the emitted-drift-ack fragment for gsd-planner.md with the #2775 fragment brought in by the merge (both named the same path; two ack sources may never name the same path) and correct its byte delta to the actual final number. * fix(#2847): drop stale merge-inherited emitted-drift-ack fragments Merging origin/next brought in three new emitted-drift-ack fragments (1700, 2658, 2775) relative to this branch's fork point. #2775 collided with my own gsd-planner.md key and was already consolidated. #1700 and #2658 don't collide, but none of their entries name a path this branch's actual diff touches (git diff --name-only origin/next...HEAD) — the ripples they explain are already baked into the current next baseline, so they explain nothing here and the emitted-attribution gate correctly reports them as stale (verified live: spike-wrap-up.md from #1700). Delete both fragment files. Neither is referenced by any test beyond a stray comment pointing at an unrelated diagnosis artifact path, not the ack fragment itself. * fix(#2847): restore merge-inherited ack fragments deleted in error 1700-spike-manifest-idea-scoping.json and 2658-trae-instruction-file-path.json exist on origin/next (landed via other, already-merged PRs) and arrived on this branch unchanged via the origin/next merge. The previous commit deleted them to satisfy a stale-acknowledgment finding, but the finding was about the acks being MODIFIED in this diff, not about needing to stop existing — deleting them would have silently reverted two other PRs' already-merged, already-justified byte growth. Restored byte-identical to origin/next (git diff origin/next -- empty for both). 2775-planner-package-legitimacy-gate.json stays consolidated into 2847-gap-closure-validate-plan-step.json: that one was a genuine hard key-collision (two fragments naming the same gsd-planner.md path, which lint-emitted-drift-ack hard-blocks), not a pass-through case. * fix(#2847): bind --schema to gap_closure mode, not hardcode it Prior revision left the validate_plan bash invocation unconditional (--schema plan)) while only the prose sentence above it described the gap_closure-mode branch. An agent executing the shown line literally always validated with the plan schema, so a gap-closure plan missing gap_closure: true still reported valid:true — #2847 reproducing unchanged. Existing tests didn't catch it: they checked for substring presence anywhere in the step, which the prose alone satisfied. Change the bash line to --schema "$SCHEMA" — a real shell-variable reference in the same placeholder convention this file already uses for "$PLAN_PATH" (never literally assigned; the agent resolves it from context, same as PLAN_PATH). A genuine if/then bash conditional already exists elsewhere in this file (load_project_state's INIT @file: check), confirming executed conditionals, not merely descriptive prose, are the established pattern here. Rewrite the regression test to assert on the bash block's literal --schema argument: reject a hardcoded plan) or plan-gap-closure) literal, require a variable reference, and require the step's prose to bind that same variable name. Verified RED against the prior revision and GREEN against this one before committing either state. * fix(#2847): CRLF-safe tests, drop unexplained ack, require gap_closure=true Four items from independent review, all landing together per request: 1. The #2847 regression test file had two CRLF-fragile regexes (local/no-crlf-fragile-split): a bare \n on readFileSync content means a real \r\n checkout returns invocationLine === null and all four executable-content assertions stop asserting anything while still reporting green. Both now use \r?\n. Prior lint report of exit 0 was a false green from a stale eslint cache. 2. The 2847 drift-ack fragment explained nothing: a direct edit to agents/gsd-planner.md is self-explaining, drift-acks exist for emitted-artifact ripple that cannot be traced to a changed source path. Deleted. Restored the 2775 fragment byte-identical to next (git diff --name-status next...HEAD -- tests/emitted-drift-acks/ now prints nothing) — it only conflicted with the now-deleted 2847 fragment, never needed touching itself. 3. plan-gap-closure validated gap_closure by PRESENCE only (unchanged since the original #2847 fix), so gap_closure: false satisfied it — --gaps-only filters strictly on gap_closure === true, so a false-valued plan still validates green and still spawns zero executors: #2847's exact reported symptom, one value away. Added an optional requiredValues map to FRONTMATTER_SCHEMAS; plan-gap-closure now requires gap_closure to equal the string "true" (extractFrontmatter parses every scalar as a string) in addition to being present. Every other schema/field keeps the original presence-only contract. The row that had documented the hole instead of closing it now asserts the fix; a matching unit test locks requiredValues on FRONTMATTER_SCHEMAS. 4. The "names the plain plan schema" assertion matched the bare substring "plan" anywhere in the step, which verify.plan-structure satisfies incidentally a few lines below — the assertion could not fail even if the plain-plan branch were deleted from the prose. Changed to match the standalone backtick-quoted plan token. * fix(#2847): remove contradictory leftover assertion in Row 6 test The gap_closure:false test asserted !present.includes('gap_closure') (correct — matches the implementation's fold-wrong-value-into-missing semantics) immediately followed by a stale, unedited leftover from an earlier draft of the same test asserting the opposite: present.includes('gap_closure'). The second could never pass once the first did; both were in the same diff. Verified before committing: searched every consumer of frontmatter.validate output (agents/gsd-planner.md, docs/CLI-TOOLS.md, all other test files) for any read of the present field — none exist. Nothing depends on "present" meaning "physically exists regardless of value correctness", so the implementation's fold (present/missing stay a full partition of required) is the right call; the test needed to agree with it, not the other way around. Manually replayed all six rows in the plan-gap-closure describe block against the built CLI to confirm each now passes. * fix(#2847): prototype-key guard, wrong-value diagnostic, doc fixes, vacuous tests Six items from an independent SHIP_VERDICT:no review, landing together per request: 1. Prototype-key crash (src/frontmatter.cts): FRONTMATTER_SCHEMAS[schemaName] was an unguarded lookup, so --schema __proto__ (also constructor, toString, hasOwnProperty, valueOf) resolved to an Object.prototype member instead of undefined, the `!schema` check never fired, and the command crashed with an uncaught TypeError and a stack trace instead of "Unknown schema". Now reachable from prompt state (--schema is an agent-bound $SCHEMA), not just an unreachable literal. Guarded with Object.prototype.hasOwnProperty.call before the lookup, checked and rejected before assignment so `schema`'s type stays non-optional. Added a test for all five prototype keys. 2. Wrong-value diagnostic (src/frontmatter.cts, agents/gsd-planner.md): the strict gap_closure === "true" check from the previous fix was correct (fail-closed) but silent about WHY — a plan with gap_closure: True got "missing", indistinguishable from genuinely absent, even though the field is plainly in the file. Added an `invalidValue` field to the validate JSON (present but wrong-valued, disjoint from missing/present) and updated validate_plan's prose to state the exact required literal and explain invalidValue, within the remaining byte budget (49130/49152). 3. docs/reference/plan-md.md: fixed three inaccuracies in the gap_closure row — "this field plus every field above" implied `requirements` (documented Required: Yes) is schema-enforced, it is not; "Type: boolean" implied YAML True/TRUE/yes/1 are accepted, they are rejected (exact string match on literal lowercase true); "must never carry it" stated an unenforced rule as fact. Also switched /gsd:plan-phase and /gsd:execute-phase to the house-style hyphen form for docs/. 4. Vacuous negative assertions (tests/fix-2847-gap-closure-frontmatter.test.cjs): RegExp#test coerces a null invocationLine to the string "null", so both hardcoded-literal checks passed vacuously even if the step or its bash block were deleted entirely. Added a truthy precondition check first. 5. Deleted vacuous/pass-always tests: four in tests/frontmatter.unit.test.cjs strictly subsumed by (or, for the "superset" test, tautologically guaranteed by the same spread as) the deepEqual exact-list test; two describe blocks in the #2847 regression file that were already GREEN at the RED commit (5e5897cd2f17ebf2fc55757bae651bbbeb236289) and pinned untouched files rather than covering anything this change altered — one of them additionally forbade any future legitimate gap_closure mention in plan-phase.md, a trap for whoever frees up that file's byte budget later. 6. .changeset/clever-newts-wake.md: switched /gsd:plan-phase and /gsd:execute-phase to /gsd-plan-phase and /gsd-execute-phase — changesets render verbatim into CHANGELOG.md with no converter in the path, so the colon form would have reached readers naming a command no runtime registers. * chore(#2847): backfill changeset pr number (#3018) --------- Co-authored-by: sim --- .changeset/clever-newts-wake.md | 5 + agents/gsd-planner.md | 11 +- docs/reference/plan-md.md | 1 + src/frontmatter.cts | 68 +++++- .../fix-2847-gap-closure-frontmatter.test.cjs | 220 ++++++++++++++++++ tests/frontmatter-cli.test.cjs | 201 ++++++++++++++++ tests/frontmatter.unit.test.cjs | 56 ++++- 7 files changed, 547 insertions(+), 15 deletions(-) create mode 100644 .changeset/clever-newts-wake.md create mode 100644 tests/fix-2847-gap-closure-frontmatter.test.cjs diff --git a/.changeset/clever-newts-wake.md b/.changeset/clever-newts-wake.md new file mode 100644 index 000000000..45136b65b --- /dev/null +++ b/.changeset/clever-newts-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3018 +--- +**Gap-closure plans generated by `/gsd-plan-phase --gaps` now deterministically carry `gap_closure: true`** — the planner's frontmatter validator previously only checked plans against a schema that never required this field, so a gap-closure plan could silently omit it and `/gsd-execute-phase --gaps-only` would then match zero plans with no error. (#2847) diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index 6595d9386..4769df146 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -869,18 +869,15 @@ Include all frontmatter fields. -Validate each created PLAN.md using `gsd-tools query`: +`$SCHEMA`: `plan-gap-closure` in gap_closure mode, else `plan`. `gap_closure` must be literal lowercase `true`. ```bash -VALID=$(gsd_run query frontmatter.validate "$PLAN_PATH" --schema plan) +VALID=$(gsd_run query frontmatter.validate "$PLAN_PATH" --schema "$SCHEMA") ``` -Returns JSON: `{ valid, missing, present, schema }` +Returns JSON: `{ valid, missing, present, invalidValue, schema }` -**If `valid=false`:** Fix missing required fields before proceeding. - -Required plan frontmatter fields: -- `phase`, `plan`, `type`, `wave`, `depends_on`, `files_modified`, `autonomous`, `must_haves` +**If `valid=false`:** `missing` = absent fields, `invalidValue` = present but wrong-valued. Fix either before proceeding. Also validate plan structure: diff --git a/docs/reference/plan-md.md b/docs/reference/plan-md.md index 73a628d0a..2989b406e 100644 --- a/docs/reference/plan-md.md +++ b/docs/reference/plan-md.md @@ -75,6 +75,7 @@ must_haves: | `status` | No | `superseded` | Marks a plan that was deliberately reassigned or abandoned mid-phase and will never be executed. A `status: superseded` plan is excluded from the phase's plan and summary counts, so it never holds the phase below 100%. See [Superseded plans](#superseded-plans). Any other value (or the field's absence) has no effect on counting. | | `estimate` | No | object | Projected execution cost: `{tokens, raw_tokens, tasks, confidence}` (#2631, [ADR-2629](../adr/2629-phase-effort-estimation-calibration.md)). `tokens` is an `estimateTokens`-scale projection with the project's calibration factor **already applied** (which is why the plan-checker passes `--calibrated` to `estimate-check` — re-applying it would square the correction); `confidence` (`low`/`med`/`high`) is **derived from the calibration sample count, never self-rated**. Additive and optional — a plan without it behaves exactly as before. A plan estimated above `workflow.smart_zone_tokens` is flagged with a split recommendation at plan time; the flag is advisory and never blocks. | | `must_haves` | Yes | object | Goal-backward verification criteria. See below. | +| `gap_closure` | Only in gap-closure mode | string, exact match | Must be exactly the literal lowercase `true` — validated as a string comparison, not a YAML boolean, so `True`, `TRUE`, `yes`, and `1` are all rejected. Required on every plan generated by `/gsd-plan-phase --gaps`, checked by the `plan-gap-closure` schema (`src/frontmatter.cts`) rather than `plan`. `/gsd-execute-phase --gaps-only` filters strictly on this field, so an omitted or wrong-valued `gap_closure` on a gap-closure plan means it is silently skipped — zero executors spawned, no error (#2847). Standard and reviews-mode plans validate against the unmodified `plan` schema, which neither requires nor checks this field (nothing rejects it as an extra field either, if present). | ### Superseded plans diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 4bf0b7abd..ebbd69414 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -607,8 +607,36 @@ function parseMustHavesBlock(content: string, blockName: string): unknown[] { // ─── Frontmatter CRUD commands ──────────────────────────────────────────────── -const FRONTMATTER_SCHEMAS: Record = { - plan: { required: ['phase', 'plan', 'type', 'wave', 'depends_on', 'files_modified', 'autonomous', 'must_haves'] }, +// Shared base for 'plan' and 'plan-gap-closure' below — a plain array reference (not +// FRONTMATTER_SCHEMAS.plan.required) because the object literal that defines +// FRONTMATTER_SCHEMAS cannot refer to itself mid-initialization (TDZ). +const PLAN_REQUIRED_FIELDS = ['phase', 'plan', 'type', 'wave', 'depends_on', 'files_modified', 'autonomous', 'must_haves']; + +// `requiredValues` is optional per schema: when a field name is a key here, the +// field must be PRESENT AND strictly equal (===) to the given value to satisfy +// the schema — presence alone is not enough. Every other required field (no +// entry in requiredValues) keeps the original presence-only contract. +const FRONTMATTER_SCHEMAS: Record }> = { + plan: { required: PLAN_REQUIRED_FIELDS }, + // #2847: gap-closure plans carry every 'plan' field PLUS gap_closure — the flag + // execute-phase --gaps-only filters on. A separate schema (not a change to + // 'plan') so standard/reviews-mode plans stay unaffected: they validate against + // 'plan' and are never required to declare or be checked for gap_closure. + // Derived from PLAN_REQUIRED_FIELDS (never hand-duplicated) so the two can't drift. + // + // requiredValues.gap_closure = true (not just presence): --gaps-only filters + // strictly on gap_closure === true (execute-phase.md, partial-wave.md), so a + // plan carrying `gap_closure: false` would pass a presence-only check and + // still be silently skipped at execute time — the exact symptom #2847 + // reports, one value away. Presence-only was flagged in review as a live + // reproduction of the bug this schema exists to close. + 'plan-gap-closure': { + required: [...PLAN_REQUIRED_FIELDS, 'gap_closure'], + // extractFrontmatter parses every scalar as a string (FrontmatterValue has + // no boolean member — `gap_closure: true` in YAML becomes the JS string + // "true", not the boolean true), so the required value is the string here. + requiredValues: { gap_closure: 'true' }, + }, summary: { required: ['phase', 'plan', 'subsystem', 'tags', 'duration', 'completed'] }, verification: { required: ['phase', 'verified', 'status', 'score'] }, }; @@ -715,8 +743,19 @@ function cmdFrontmatterMerge(cwd: string, filePath: string, data: string | undef function cmdFrontmatterValidate(cwd: string, filePath: string, schemaName: string | undefined, raw: boolean): void { if (!filePath || !schemaName) { error('file and schema required'); } if (filePath.includes('\0')) { error('file path contains null bytes'); } + // Guard against prototype-chain keys (__proto__, constructor, toString, hasOwnProperty, + // valueOf, ...): a bare FRONTMATTER_SCHEMAS[schemaName] lookup resolves those to + // Object.prototype members instead of undefined, so a `!schema` check on the raw + // lookup never fires and the code crashes later on `schema.required.filter` with an + // uncaught TypeError instead of the intended "Unknown schema" message. Confirmed live + // with --schema __proto__. Now that --schema is agent-bound (agents/gsd-planner.md's + // $SCHEMA), this is reachable from prompt state, not just an unreachable literal. + // Checked and rejected BEFORE the lookup (rather than `?? undefined`-ing the lookup + // itself) so `schema`'s inferred type stays non-optional and needs no assertion below. + if (!Object.prototype.hasOwnProperty.call(FRONTMATTER_SCHEMAS, schemaName as string)) { + error(`Unknown schema: ${schemaName}. Available: ${Object.keys(FRONTMATTER_SCHEMAS).join(', ')}`); + } const schema = FRONTMATTER_SCHEMAS[schemaName as string]; - if (!schema) { error(`Unknown schema: ${schemaName}. Available: ${Object.keys(FRONTMATTER_SCHEMAS).join(', ')}`); } const fullPath = path.isAbsolute(filePath) ? filePath : path.join(cwd, filePath); const content = safeReadFile(fullPath); if (!content) { output({ error: 'File not found', path: filePath }, raw, undefined); return; } @@ -728,9 +767,26 @@ function cmdFrontmatterValidate(cwd: string, filePath: string, schemaName: strin // Pass the resolved path so a truncated file is named in the diagnostic and deduplicated // per file rather than per content digest (#1882, ADR-1411 wiring clause). const fm = extractFrontmatter(content, fullPath); - const missing = schema.required.filter(f => fm[f] === undefined); - const present = schema.required.filter(f => fm[f] !== undefined); - output({ valid: missing.length === 0, missing, present, schema: schemaName }, raw, missing.length === 0 ? 'valid' : 'invalid'); + const requiredValues = schema.requiredValues || {}; + // A field satisfies the schema when it is present AND — for fields with a + // requiredValues entry — strictly equal to that value. Absent and + // wrong-value both surface as `missing` (existing `missing`/`present` + // partition of `required` is unchanged — no consumer reads `present` to + // mean "physically exists regardless of value," confirmed by searching + // every caller before choosing this). But #2847 review: folding silently + // made "missing" misleading for a WRONG-valued field the plan author can + // plainly see in the file (e.g. `gap_closure: True`) — nothing told them + // the field is present but the VALUE is wrong, so they could loop trying + // to add a field that is already there. `invalidValue` names exactly that + // subset (present, but not the required value) so the message stays + // actionable without changing what `missing`/`present` mean. + const wrongValue = (f: string): boolean => + fm[f] !== undefined && Object.prototype.hasOwnProperty.call(requiredValues, f) && fm[f] !== requiredValues[f]; + const satisfies = (f: string): boolean => fm[f] !== undefined && !wrongValue(f); + const missing = schema.required.filter(f => !satisfies(f)); + const present = schema.required.filter(f => satisfies(f)); + const invalidValue = schema.required.filter(wrongValue); + output({ valid: missing.length === 0, missing, present, invalidValue, schema: schemaName }, raw, missing.length === 0 ? 'valid' : 'invalid'); } export = { diff --git a/tests/fix-2847-gap-closure-frontmatter.test.cjs b/tests/fix-2847-gap-closure-frontmatter.test.cjs new file mode 100644 index 000000000..1db5fe83a --- /dev/null +++ b/tests/fix-2847-gap-closure-frontmatter.test.cjs @@ -0,0 +1,220 @@ +'use strict'; + +// allow-test-rule: source-text-is-the-product (see #2847) +// agents/gsd-planner.md is the deployed runtime prompt contract — the planner +// agent literally executes this markdown. Testing its text content tests the +// deployed contract, per the CONTRIBUTING.md exception matrix and the existing +// precedent in tests/plan-phase-drift-guard.test.cjs and +// tests/edge-probe-planner-contract.test.cjs. + +/** + * Regression tests for #2847 + * + * "--gaps does not load planner-gap-closure.md, so generated gap plans may + * miss gap_closure metadata" + * + * Root cause: the planner's only machine-checked validation gate + * (`gsd_run query frontmatter.validate "$PLAN_PATH" --schema plan`) never + * required `gap_closure`. The only place `gap_closure: true` was actually + * documented as required was prose in a conditionally-loaded reference file + * (gsd-core/references/planner-gap-closure.md) plus an unvalidated checklist + * item — neither backed by a deterministic gate. + * + * Fix: + * - src/frontmatter.cts: new `plan-gap-closure` FRONTMATTER_SCHEMAS entry + * (covered behaviorally in tests/frontmatter-cli.test.cjs and + * tests/frontmatter.unit.test.cjs — this file covers the prompt-level wiring + * that selects it). + * - agents/gsd-planner.md ``: the bash invocation + * now reads `--schema "$SCHEMA"` — a real shell-variable reference, bound in + * the same style as the file's existing `"$PLAN_PATH"` convention — instead + * of a hardcoded literal. An earlier revision left the bash line unconditional + * (`--schema plan)`, a copy-executable no-op) while only the prose sentence + * above it mentioned the conditional; that revision satisfied every + * substring-presence check but never actually selected plan-gap-closure at + * runtime. Caught by review, not by tests — see the describe block below for + * the executable-content assertions written specifically to catch it. + * + * Deliberately NOT touched: gsd-core/workflows/plan-phase.md's + * `` block. An earlier draft of this fix added a + * gap_closure mention there too (mirroring plan-phase.md's existing + * `` mode-scoped-block pattern for reviews + * mode), but plan-phase.md sits only 36 bytes under the hard ADR-857 + * PRE_PHASE6 ceiling (tests/phase6-capstone-conformance.test.cjs, + * `PRE_PHASE6['plan-phase.md'] = 94519`) and cannot absorb the ~330-byte + * addition. The `` fix in gsd-planner.md is the + * actual call site and is sufficient on its own: the planner already tracks + * gap_closure mode internally (its own `` switches + * to gap_closure_mode on `--gaps`), so the schema selection does not depend on + * plan-phase.md's prose at all. See .gsd/bug/fix-2847-gap-closure-frontmatter/10-diagnosis.md. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const PLANNER_AGENT_PATH = path.join(__dirname, '..', 'agents', 'gsd-planner.md'); + +function readFile(p) { + return fs.readFileSync(p, 'utf-8'); +} + +function extractStep(content, stepName) { + const marker = ``; + const start = content.indexOf(marker); + if (start === -1) return null; + const end = content.indexOf('', start); + if (end === -1) return null; + return content.slice(start, end + ''.length); +} + +/** + * Extract the FIRST ```bash ... ``` fenced block from a step's text. Returns null + * if no fenced bash block is found. + */ +function extractFirstBashBlock(stepText) { + const m = /```bash\r?\n([\s\S]*?)```/.exec(stepText); + return m ? m[1] : null; +} + +/** + * Find the literal line, within a bash block, that invokes `frontmatter.validate`. + * Returns null if not found. + */ +function findValidateInvocationLine(bashBlock) { + if (!bashBlock) return null; + return bashBlock.split('\n').find((l) => l.includes('frontmatter.validate')) || null; +} + +// ─── agents/gsd-planner.md: validate_plan step BINDS schema to mode (#2847) ── +// +// This describe block asserts on the EXECUTABLE content of the step — the literal +// argument passed to `--schema` in the fenced bash block the agent actually runs — +// not on whether explanatory words appear anywhere in the step's prose. A prose +// sentence like "use plan-gap-closure in gap_closure mode, else plan" sitting next +// to an UNCONDITIONAL `--schema plan)` line satisfies every substring-presence +// check imaginable while the agent still only ever executes `--schema plan`. That +// exact shape shipped in an earlier revision of this fix and was caught by review, +// not by tests — these tests are written specifically to catch it mechanically: +// verified RED against that revision (`--schema plan)` hardcoded in the bash +// block, `--schema plan-gap-closure` only in the prose sentence above it) before +// the bash block was changed to `--schema "$SCHEMA"`. + +describe('#2847: gsd-planner.md validate_plan step BINDS --schema to gap_closure mode (executable content, not prose)', () => { + const plannerContent = readFile(PLANNER_AGENT_PATH); + const validateStep = extractStep(plannerContent, 'validate_plan'); + const bashBlock = extractFirstBashBlock(validateStep || ''); + const invocationLine = findValidateInvocationLine(bashBlock); + + test('validate_plan step exists and has a fenced bash block invoking frontmatter.validate', () => { + assert.ok(validateStep, ' must exist in agents/gsd-planner.md'); + assert.ok(bashBlock, 'validate_plan step must have a ```bash fenced block'); + assert.ok(invocationLine, 'validate_plan step bash block must invoke frontmatter.validate'); + }); + + test('the --schema argument in the bash invocation is NOT a hardcoded literal', () => { + // Precondition: both regex checks below use `.test(invocationLine)`, and + // RegExp#test coerces a null/undefined argument to the STRING "null"/ + // "undefined" rather than throwing — neither hardcoded-literal pattern + // matches that string, so both negative assertions would pass vacuously + // (reporting "not hardcoded") even if the step or its bash block were + // deleted entirely. Fail loudly on that precondition first so a deleted + // step is reported as exactly that, not as a false "fix confirmed". + assert.ok(invocationLine, 'precondition: invocationLine must be found (see the first test in this block)'); + + // This is the exact regression: a prior revision had this line read + // `--schema plan)` verbatim — a plain, hardcoded, always-the-same-value + // literal that an agent executes as-is regardless of mode. Reject BOTH + // possible hardcoded literals explicitly, not just one, so a fix that + // flips the hardcoded default to plan-gap-closure (breaking standard mode + // instead of gap_closure mode) is caught too. + assert.ok( + !/--schema\s+plan\)/.test(invocationLine), + `bash invocation must not hardcode --schema plan — found: ${invocationLine}` + ); + assert.ok( + !/--schema\s+plan-gap-closure\)/.test(invocationLine), + `bash invocation must not hardcode --schema plan-gap-closure — found: ${invocationLine}` + ); + }); + + test('the --schema argument in the bash invocation IS a shell variable reference', () => { + // A variable reference means the value is resolved at execution time from + // whatever the agent has bound it to, not printed once in the template and + // copy-executed unchanged. Matches --schema "$SCHEMA", --schema $SCHEMA, + // or --schema "${SCHEMA}". + const varMatch = /--schema\s+"?\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?"?\)/.exec(invocationLine); + assert.ok( + varMatch, + `bash invocation's --schema argument must be a shell variable (e.g. --schema "$SCHEMA"), not a literal — found: ${invocationLine}` + ); + }); + + test('the bound variable is actually conditioned on gap_closure mode in the step prose, and both target schema names are named', () => { + const varMatch = /--schema\s+"?\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?"?\)/.exec(invocationLine); + assert.ok(varMatch, 'precondition: --schema must reference a variable (see previous test)'); + const varName = varMatch[1]; + + // The SAME variable name the bash block reads must appear in the step's prose + // (outside the bash block) — otherwise the "binding" is a variable nothing + // ever explains how to set, which is not meaningfully better than a literal. + const proseOutsideBash = validateStep.replace(/```bash\r?\n[\s\S]*?```/, ''); + assert.ok( + proseOutsideBash.includes(`$${varName}`) || proseOutsideBash.includes(`\`$${varName}\``), + `step prose must explain how $${varName} is set — the bash block references it but nothing binds it` + ); + + // Both concrete schema names this variable can resolve to must be named + // somewhere in the step, and gap_closure mode must be the stated condition + // for choosing between them. Match the plain `plan` schema as a standalone + // backtick-quoted token (`` `plan` ``), not the bare substring "plan" — + // a bare-substring check is satisfied incidentally by "verify.plan-structure" + // a few lines below even if the plain-plan branch were deleted entirely from + // the prose, which would make this assertion unable to ever fail. + assert.ok(validateStep.includes('plan-gap-closure'), 'step must name the plan-gap-closure schema'); + assert.ok( + /`plan`/.test(validateStep), + 'step must name the plain plan schema, as a standalone `plan` token, as the other branch' + ); + assert.ok(/gap_closure mode/i.test(validateStep), 'step must condition the choice on gap_closure mode by name'); + }); + + test('the plan-structure validation call below (unrelated step) is unaffected', () => { + // Regression guard for the fix itself: confirm the edit did not touch the + // sibling verify.plan-structure invocation in the same step. + assert.ok( + validateStep.includes('verify.plan-structure "$PLAN_PATH"'), + 'validate_plan step must still invoke verify.plan-structure unchanged' + ); + }); +}); + +// ─── Cross-file consistency: schema name used by both files matches (#2847) ── + +describe('#2847: schema name consistency between gsd-planner.md and src/frontmatter.cts', () => { + test('gsd-planner.md references the exact schema name "plan-gap-closure"', () => { + const plannerContent = readFile(PLANNER_AGENT_PATH); + assert.ok( + plannerContent.includes('plan-gap-closure'), + 'agents/gsd-planner.md must reference the literal schema name "plan-gap-closure" ' + + '(the exact key registered in FRONTMATTER_SCHEMAS in src/frontmatter.cts) — a ' + + 'mismatched name would fail at runtime with "Unknown schema"' + ); + }); +}); + +// #2847 review: two describe blocks previously lived here — +// "planner-gap-closure.md reference is untouched" and "plan-phase.md is +// deliberately unmodified by this fix" — both deleted. Neither file is +// touched by this fix, so both assertions were already GREEN at the RED +// commit (5e5897cd2f17ebf2fc55757bae651bbbeb236289): they pinned untouched +// files rather than providing regression coverage for anything this change +// altered. The plan-phase.md one was worse than merely unhelpful — it +// permanently forbade any FUTURE legitimate `gap_closure` mention in +// plan-phase.md, a trap for whoever eventually frees up that file's byte +// budget and has a real reason to add one. The design decision itself (why +// plan-phase.md is untouched — the ADR-857 PRE_PHASE6 byte ceiling) remains +// documented in the file-level comment above and in +// .gsd/bug/fix-2847-gap-closure-frontmatter/10-diagnosis.md; it just isn't +// asserted as a permanent negative here. diff --git a/tests/frontmatter-cli.test.cjs b/tests/frontmatter-cli.test.cjs index ebb4a0705..ad19e3341 100644 --- a/tests/frontmatter-cli.test.cjs +++ b/tests/frontmatter-cli.test.cjs @@ -268,6 +268,29 @@ body`; assert.ok(result.error.includes('Unknown schema'), 'Error should mention unknown schema'); }); + // #2847 review finding: a bare FRONTMATTER_SCHEMAS[schemaName] lookup resolves + // prototype-chain keys to Object.prototype members instead of undefined, so the + // `!schema` guard never fires and the command crashes with an uncaught TypeError + // ("Cannot read properties of undefined (reading 'filter')") and a stack trace + // instead of reporting "Unknown schema". Now that --schema is an agent-bound + // variable ($SCHEMA in agents/gsd-planner.md's validate_plan step) rather than a + // fixed literal, this is reachable from prompt state. + for (const schemaName of ['__proto__', 'constructor', 'toString', 'hasOwnProperty', 'valueOf']) { + test(`--schema ${schemaName} reports Unknown schema, not a crash`, () => { + const file = writeTempFile('---\nphase: 01\n---\n'); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', schemaName]); + assert.ok(!result.success, `--schema ${schemaName} should fail with a non-zero exit code, not crash`); + assert.ok( + result.error.includes('Unknown schema'), + `--schema ${schemaName} error should be "Unknown schema...", not a TypeError stack trace; got: ${result.error}` + ); + assert.ok( + !result.error.includes('TypeError') && !result.error.includes('Cannot read properties'), + `--schema ${schemaName} must not surface a raw TypeError; got: ${result.error}` + ); + }); + } + test('returns error for missing file', () => { const result = runGsdTools('frontmatter validate /nonexistent/file.md --schema plan'); assert.ok(result.success, 'Command should exit 0 with error JSON'); @@ -276,6 +299,184 @@ body`; }); }); +// ─── frontmatter validate: plan-gap-closure schema (#2847) ─────────────────── +// +// Regression coverage for #2847: "--gaps does not load planner-gap-closure.md, +// so generated gap plans may miss gap_closure metadata". A gap-closure plan +// with every other required field but no `gap_closure` used to report +// `valid: true` against the only schema the planner validated against +// (`plan`). Row 1 below is the failing-first regression test: it fails on +// pre-fix `FRONTMATTER_SCHEMAS` (no `plan-gap-closure` key exists — the CLI +// exits 1 with "Unknown schema: plan-gap-closure") and passes after the fix. + +describe('frontmatter validate: plan-gap-closure schema (#2847)', () => { + const PLAN_BODY_NO_GAP_CLOSURE = `--- +phase: 01 +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: [src/auth.ts] +autonomous: true +must_haves: + truths: + - "All tests pass" +--- +body`; + + // Row 1 — failing-first regression test. + test('rejects plan-gap-closure frontmatter missing gap_closure (#2847)', () => { + const file = writeTempFile(PLAN_BODY_NO_GAP_CLOSURE); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan-gap-closure']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, false, 'Should be invalid: gap_closure is missing'); + assert.ok(parsed.missing.includes('gap_closure'), 'gap_closure should be reported missing'); + assert.strictEqual(parsed.missing.length, 1, 'Only gap_closure should be missing; all other fields are present'); + assert.deepStrictEqual(parsed.invalidValue, [], 'gap_closure is ABSENT here, not wrong-valued — invalidValue must stay empty'); + assert.strictEqual(parsed.schema, 'plan-gap-closure'); + }); + + // Row 2 — happy path. + test('accepts complete plan-gap-closure frontmatter', () => { + const content = `--- +phase: 01 +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: [src/auth.ts] +autonomous: true +must_haves: + truths: + - "All tests pass" +gap_closure: true +--- +body`; + const file = writeTempFile(content); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan-gap-closure']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, true, 'Should be valid: gap_closure is present'); + assert.deepStrictEqual(parsed.missing, []); + assert.ok(parsed.present.includes('gap_closure')); + assert.deepStrictEqual(parsed.invalidValue, [], 'gap_closure has the correct value here — invalidValue must be empty'); + assert.strictEqual(parsed.schema, 'plan-gap-closure'); + }); + + // Row 3 — empty/near-empty input boundary. + test('reports all plan-gap-closure fields missing except phase for near-empty frontmatter', () => { + const file = writeTempFile('---\nphase: 01\n---\nbody'); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan-gap-closure']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, false); + // plan-gap-closure requires 9 fields; phase is present, so 8 should be missing. + assert.strictEqual(parsed.missing.length, 8, 'Should have 8 missing required fields'); + assert.ok(parsed.missing.includes('gap_closure'), 'gap_closure should be among the missing fields'); + }); + + // Row 4 — negative space: standard-mode ('plan' schema) plans are unaffected by #2847's fix. + test('plan schema (standard/reviews mode) still reports valid without gap_closure — unaffected by #2847 fix', () => { + const file = writeTempFile(PLAN_BODY_NO_GAP_CLOSURE); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, true, 'plan schema must not require gap_closure (AC(3): standard mode unaffected)'); + assert.deepStrictEqual(parsed.missing, []); + assert.strictEqual(parsed.schema, 'plan'); + }); + + // Row 5 — CRLF cross-platform newline handling. + test('parses plan-gap-closure frontmatter with CRLF line endings', () => { + const content = [ + '---', + 'phase: 01', + 'plan: 01', + 'type: execute', + 'wave: 1', + 'depends_on: []', + 'files_modified: [src/auth.ts]', + 'autonomous: true', + 'must_haves:', + ' truths:', + ' - "All tests pass"', + 'gap_closure: true', + '---', + 'body', + ].join('\r\n'); + const file = writeTempFile(content); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan-gap-closure']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, true, 'CRLF frontmatter must parse identically to LF for plan-gap-closure'); + assert.ok(parsed.present.includes('gap_closure')); + }); + + // Row 6 — gap_closure: false must be REJECTED, not merely present. + // + // #2847 review finding: --gaps-only filters strictly on gap_closure === true + // (execute-phase.md, partial-wave.md). A presence-only check (matching every + // other required field) lets `gap_closure: false` validate as valid:true, + // which is #2847's exact reported symptom — --gaps-only still spawns zero + // executors — one value away. plan-gap-closure's requiredValues entry closes + // this: gap_closure must be present AND equal "true" (extractFrontmatter + // parses every scalar as a string; FrontmatterValue has no boolean member). + test('gap_closure: false is rejected — plan-gap-closure requires the value true, not mere presence', () => { + const content = `--- +phase: 01 +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: [src/auth.ts] +autonomous: true +must_haves: + truths: + - "All tests pass" +gap_closure: false +--- +body`; + const file = writeTempFile(content); + const result = runGsdTools(['frontmatter', 'validate', file, '--schema', 'plan-gap-closure']); + assert.ok(result.success, `Command failed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.valid, false, 'gap_closure: false must NOT satisfy plan-gap-closure'); + assert.ok(parsed.missing.includes('gap_closure'), 'gap_closure must be reported missing when its value is false'); + assert.ok(!parsed.present.includes('gap_closure'), 'gap_closure must not be reported present when its value is false'); + // #2847 review: presence alone is not the whole story here — the field IS in the + // file, just wrong-valued. invalidValue distinguishes that from a genuinely absent + // field (Row 1) so a caller (or a human) gets an actionable "the value is wrong", + // not "this field is missing" for a field they can plainly see in the plan. + assert.ok( + parsed.invalidValue.includes('gap_closure'), + 'gap_closure must be reported in invalidValue — present but wrong-valued, distinct from genuinely absent' + ); + }); + + // Row 7 — invalidValue vs missing distinction, spelled out directly (not just + // implied by Rows 1/2/6 individually). + test('invalidValue distinguishes "present but wrong value" from "absent" for the same missing-reporting field', () => { + const absentResult = JSON.parse( + runGsdTools(['frontmatter', 'validate', writeTempFile(PLAN_BODY_NO_GAP_CLOSURE), '--schema', 'plan-gap-closure']).output + ); + const wrongValueContent = PLAN_BODY_NO_GAP_CLOSURE.replace('---\nbody', 'gap_closure: TRUE\n---\nbody'); + const wrongValueResult = JSON.parse( + runGsdTools(['frontmatter', 'validate', writeTempFile(wrongValueContent), '--schema', 'plan-gap-closure']).output + ); + + // Both report gap_closure as missing (the field does not satisfy the schema either way)... + assert.ok(absentResult.missing.includes('gap_closure')); + assert.ok(wrongValueResult.missing.includes('gap_closure')); + // ...but only the wrong-VALUE case appears in invalidValue. + assert.deepStrictEqual(absentResult.invalidValue, [], 'a genuinely absent field must not appear in invalidValue'); + assert.ok( + wrongValueResult.invalidValue.includes('gap_closure'), + 'gap_closure: TRUE (capitalized YAML boolean, rejected — the validator requires the exact literal lowercase true) must appear in invalidValue' + ); + }); +}); + // ─── frontmatter set/merge: must_haves object-list preservation (#1572) ────── // `frontmatter set`/`merge` round-tripped the WHOLE frontmatter through the lossy // extractFrontmatter → reconstructFrontmatter pair, which flattens must_haves diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 6ada479e6..a88aeadc2 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -832,8 +832,11 @@ describe('FRONTMATTER_SCHEMAS', () => { ]); }); - test('three schemas exist: plan, summary, verification', () => { - assert.deepEqual(Object.keys(FRONTMATTER_SCHEMAS).sort(), ['plan', 'summary', 'verification']); + test('four schemas exist: plan, plan-gap-closure, summary, verification (#2847)', () => { + assert.deepEqual( + Object.keys(FRONTMATTER_SCHEMAS).sort(), + ['plan', 'plan-gap-closure', 'summary', 'verification'] + ); }); test('plan includes phase field', () => { @@ -861,6 +864,55 @@ describe('FRONTMATTER_SCHEMAS', () => { }); }); +// ─── FRONTMATTER_SCHEMAS['plan-gap-closure'] (#2847) ────────────────────────── +// +// #2847: --gaps does not load planner-gap-closure.md's requirement into the +// only machine-checked gate — gap-closure plans could pass validation +// without `gap_closure: true`, so a subsequent `--gaps-only` execute run +// silently matched zero plans. Fix: a dedicated schema that requires every +// 'plan' field plus `gap_closure`, kept separate so standard/reviews-mode +// plans (validated against 'plan', asserted unchanged above) are unaffected. + +describe("FRONTMATTER_SCHEMAS['plan-gap-closure'] (#2847)", () => { + // #2847 review: "has required field", "has exactly 9 required fields", + // "includes gap_closure field", and "is a superset of every plan-required + // field" were deleted here. Each was strictly subsumed by the deepEqual + // exact-list test below (a 9-element array that deepEquals a literal + // containing 'gap_closure' trivially has 9 elements, a 'required' key, + // and includes 'gap_closure' — none of those checks could ever fail + // independently of the deepEqual one). The "superset" test was additionally + // tautological on its own: plan-gap-closure.required is LITERALLY + // `[...PLAN_REQUIRED_FIELDS, 'gap_closure']` (src/frontmatter.cts), so it + // cannot fail while that spread exists, regardless of what the deepEqual + // test above catches. + test('plan-gap-closure schema required fields are exact', () => { + assert.deepEqual(FRONTMATTER_SCHEMAS['plan-gap-closure'].required, [ + 'phase', 'plan', 'type', 'wave', 'depends_on', 'files_modified', 'autonomous', 'must_haves', 'gap_closure', + ]); + }); + + test('plan schema (standard/reviews mode) does NOT include gap_closure — unaffected by #2847 fix', () => { + assert.ok(!FRONTMATTER_SCHEMAS.plan.required.includes('gap_closure')); + }); + + // requiredValues (#2847 review finding): presence alone let `gap_closure: false` + // validate as valid:true — --gaps-only filters strictly on gap_closure === true, + // so that was a live reproduction of #2847's reported symptom, one value away. + test('plan-gap-closure requires the value "true" for gap_closure, not mere presence', () => { + assert.ok('requiredValues' in FRONTMATTER_SCHEMAS['plan-gap-closure']); + assert.equal(FRONTMATTER_SCHEMAS['plan-gap-closure'].requiredValues.gap_closure, 'true'); + }); + + test('plan schema has no requiredValues — every field is presence-only, unaffected by #2847 fix', () => { + assert.equal(FRONTMATTER_SCHEMAS.plan.requiredValues, undefined); + }); + + test('summary and verification schemas have no requiredValues — presence-only, unaffected', () => { + assert.equal(FRONTMATTER_SCHEMAS.summary.requiredValues, undefined); + assert.equal(FRONTMATTER_SCHEMAS.verification.requiredValues, undefined); + }); +}); + // ─── Tight branch / boundary tests ─────────────────────────────────────────── describe('reconstructFrontmatter: array length boundary (<=3 vs >3)', () => {