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)', () => {