fix(#2847): require gap_closure value in plan-gap-closure schema and bind validate_plan to it (#3018)
* 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 "<downstream_consumer>" 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 -- <path> 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 <sim@local>
This commit is contained in:
Tom Boucher
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user