From 185da024cb6e3692f2c8c3b8b88bcb9fc547754c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Jul 2026 16:55:10 -0400 Subject: [PATCH] fix(#2770): decision-coverage gate fails closed on empty arg + workflow recomputes CONTEXT_PATH in-block (#2881) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2770): empty contextPath argument must fail closed, not green-skip the decision-coverage gate The handler conflated empty-arg (caller error) with file-missing (legitimate skip), returning passed:true/skipped on an empty argument. Add: empty arg → passed:false; real-path-to-absent-file → legitimate green skip preserved; omitted arg → fail closed. * fix(#2770): decision-coverage gate fails closed on empty arg + workflow recomputes CONTEXT_PATH in-block Handler (check-command-router.cts): split the guard — empty/missing contextPath argument is a caller error (fail closed, passed:false, mirrors #1365); a real path whose file genuinely does not exist keeps the legitimate green skip. Workflow (plan-phase.md): recompute CONTEXT_PATH inside the consuming Bash block (it was set in the step-1 init block, which does not survive into the separately- spawned gate block — so the gate ran with an empty arg and silently green-skipped). * chore(#2770): changeset fragment * fix(#2770): guard workflow empty-glob case (review blocker) + update drift-guard test The handler now fails closed on an empty contextPath arg, so the workflow's unguarded glob (empty when a phase genuinely has no CONTEXT.md) would invoke the gate with an empty arg → passed:false → exit 1, hard-halting the legitimate 'Continue without context' plan-phase path. Guard the empty-glob case: only run the gate when a CONTEXT.md actually exists. Update the F1 drift-guard test (which gave false coverage — it only checked for the ${CONTEXT_PATH} token) to assert the in-block recompute AND the empty-glob guard. * fix(#2770): keep plan-phase.md under ADR-857 size cap + ack emitted drift + fix drift-guard window The workflow fix grew plan-phase.md past the ADR-857 phase-6 size cap (94519B) and triggered emitted-attribution. Condense adjacent §13a prose/JSON to offset (net +89B, under cap). Add tests/emitted-drift-ack.json acknowledging the residual growth. Widen the drift-guard test window (the gate invocation is now nested in the empty-glob guard, so the old 400-char window missed the glob recompute). * chore(#2770): backfill changeset PR number (2881) --------- Co-authored-by: Test --- .changeset/bold-geese-wander.md | 5 +++ gsd-core/workflows/plan-phase.md | 52 ++++++++++++------------- src/check-command-router.cts | 14 ++++++- tests/decisions.test.cjs | 56 +++++++++++++++++++++++++++ tests/emitted-drift-ack.json | 8 ++++ tests/plan-phase-drift-guard.test.cjs | 27 ++++++------- 6 files changed, 119 insertions(+), 43 deletions(-) create mode 100644 .changeset/bold-geese-wander.md create mode 100644 tests/emitted-drift-ack.json diff --git a/.changeset/bold-geese-wander.md b/.changeset/bold-geese-wander.md new file mode 100644 index 000000000..79f21f6b2 --- /dev/null +++ b/.changeset/bold-geese-wander.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2881 +--- +**The plan-phase decision-coverage gate can no longer silently pass when its context-path argument is missing** — the handler now fails closed on an empty/missing argument (a caller error), and the plan-phase workflow recomputes the CONTEXT.md path in the same Bash block that runs the gate (the variable set in the init block did not survive into the gate block). A genuinely-absent CONTEXT.md still produces the legitimate green skip. Previously the gate reported `passed` without ever checking coverage. (#2770) diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index 8d446b4e7..a4b6286af 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -1400,47 +1400,43 @@ If `TEXT_MODE` is true, present as a plain-text numbered list (options already s ## 13a. Decision Coverage Gate -After the requirements coverage gate passes, verify that every trackable -decision captured by discuss-phase in CONTEXT.md `` is referenced -by at least one plan. This is the **translation gate** from issue #2492 — -its job is to refuse to mark a phase planned when a discuss-phase decision -silently dropped on the way into the plans. +Verify every trackable decision in CONTEXT.md `` is referenced by at +least one plan. This **translation gate** (#2492) refuses to mark a phase planned +when a discuss-phase decision silently dropped. -**Skip if** `workflow.context_coverage_gate` is explicitly set to `false` -(absent key = enabled). Also skip if no CONTEXT.md exists for this phase -(nothing to translate) or if its `` block is empty. +**Skip if** `workflow.context_coverage_gate` is `false` (absent = enabled), or +no CONTEXT.md exists for this phase, or its `` block is empty. ```bash GATE_CFG=$(gsd_run query config-get workflow.context_coverage_gate 2>/dev/null || echo "true") if [ "$GATE_CFG" != "false" ]; then - GATE_RESULT=$(gsd_run query check.decision-coverage-plan "${PHASE_DIR}" "${CONTEXT_PATH}") - # BLOCKING: refuse to mark phase planned when a trackable decision is uncovered. - # `passed: true` covers both real-pass and skipped cases (gate disabled / no CONTEXT.md / - # no trackable decisions). Verify-phase counterpart deliberately omits this exit-1 — that - # gate is non-blocking by design (review finding F15). - echo "$GATE_RESULT" | jq -e '(.passed // .data.passed) == true' >/dev/null || { - echo "$GATE_RESULT" | jq -r '(.message // .data.message // "Decision coverage gate failed.")' - exit 1 - } + # #2770: CONTEXT_PATH from step-1 init doesn't survive into this Bash block; + # recompute it. Only run when a CONTEXT.md exists (handler fails closed on an + # empty arg, so an unguarded empty glob would halt a context-less phase). + CONTEXT_PATH=$(ls "${PHASE_DIR}"/*-CONTEXT.md 2>/dev/null | head -1) + if [ -n "$CONTEXT_PATH" ]; then + GATE_RESULT=$(gsd_run query check.decision-coverage-plan "${PHASE_DIR}" "${CONTEXT_PATH}") + # BLOCKING: refuse to mark phase planned when a trackable decision is uncovered. + # `passed: true` covers both real-pass and skipped cases (gate disabled / no CONTEXT.md / + # no trackable decisions). Verify-phase counterpart deliberately omits this exit-1 — that + # gate is non-blocking by design (review finding F15). + echo "$GATE_RESULT" | jq -e '(.passed // .data.passed) == true' >/dev/null || { + echo "$GATE_RESULT" | jq -r '(.message // .data.message // "Decision coverage gate failed.")' + exit 1 + } + fi fi ``` The handler returns JSON: ```json -{ - "passed": true, - "skipped": false, - "total": 2, - "covered": 2, - "uncovered": [ { "id": "D-01", "text": "...", "category": "..." } ], - "message": "..." -} +{ "passed": true, "skipped": false, "total": 2, "covered": 2, + "uncovered": [{ "id": "D-01", "text": "...", "category": "..." }], "message": "..." } ``` **If `passed` is true (or `skipped` is true):** Display -`✓ Decision coverage: {M}/{N} CONTEXT.md decisions covered by plans` (or -`(skipped — gate disabled)` / `(skipped — no decisions)`) and proceed to -step 13b. +`✓ Decision coverage: {M}/{N} decisions covered` (or `(skipped)`) and proceed +to step 13b. **If `passed` is false:** Display the handler's `message` block. It already names each uncovered decision (`D-NN | category | text`) and tells the user diff --git a/src/check-command-router.cts b/src/check-command-router.cts index a20f65e60..560d1d089 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -276,13 +276,23 @@ function loadDecisionExtraction(contextPath: string): { trackable: Decision[]; o function cmdDecisionCoveragePlan(projectDir: string, args: string[], raw: boolean): void { const phaseDir = args[2] ? resolvePath(args[2], projectDir) : ''; - const contextPath = args[3] ? resolvePath(args[3], projectDir) : ''; + const contextArg = args[3]; + const contextPath = contextArg ? resolvePath(contextArg, projectDir) : ''; if (!gateEnabled(projectDir)) { output({ passed: true, skipped: true, reason: 'workflow.context_coverage_gate is false', total: 0, covered: 0, uncovered: [], message: 'Decision coverage gate disabled by config.' }, raw, undefined); return; } - if (!contextPath || !fs.existsSync(contextPath)) { + // #2770: an EMPTY/MISSING contextPath argument is a CALLER ERROR (the workflow + // forgot to pass the path — e.g. a shell variable lost between Bash blocks), not + // evidence the phase has no CONTEXT.md. Fail closed (mirrors #1365 fail-loud) so a + // blocking gate cannot silently certify success on a caller mistake. + if (!contextArg || contextArg === '') { + output({ passed: false, skipped: false, reason: 'missing context path argument', total: 0, covered: 0, uncovered: [], message: 'Decision coverage gate called without a context path argument — the caller (e.g. the plan-phase workflow) must pass the CONTEXT.md path. An empty argument is a caller error, not evidence there is nothing to check (#2770).' }, raw, undefined); + return; + } + // A REAL path whose file genuinely does not exist is the LEGITIMATE green skip. + if (!fs.existsSync(contextPath)) { output({ passed: true, skipped: true, reason: 'CONTEXT.md missing', total: 0, covered: 0, uncovered: [], message: 'No CONTEXT.md - nothing to check.' }, raw, undefined); return; } diff --git a/tests/decisions.test.cjs b/tests/decisions.test.cjs index 6b75b2cc8..e3a4383b5 100644 --- a/tests/decisions.test.cjs +++ b/tests/decisions.test.cjs @@ -1124,3 +1124,59 @@ describe('check.decision-coverage-plan — planner-canonical tag scanning (#2372 assert.strictEqual(parsed.passed, true); }); }); + +// ─── #2770: empty contextPath argument must fail closed, not green-skip ────── +// The handler conflated "empty argument" (a CALLER ERROR — the workflow forgot to +// pass the path) with "file missing" (a LEGITIMATE green skip). An empty arg +// returned passed:true/skipped/reason:"CONTEXT.md missing", silently certifying a +// blocking gate. Must fail closed (mirrors #1365 fail-loud). + +describe('check.decision-coverage-plan — empty contextPath argument fails closed (#2770)', () => { + let tmpDir; + let planningDir; + let phaseDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-2770-'); + planningDir = path.join(tmpDir, '.planning'); + phaseDir = path.join(planningDir, 'phases', '01-init'); + fs.mkdirSync(phaseDir, { recursive: true }); + }); + + afterEach(() => cleanup(tmpDir)); + + test('empty contextPath argument → passed:false (caller error, fail closed)', () => { + const result = runDecisionCoveragePlan(phaseDir, '', tmpDir); + const parsed = JSON.parse(result.output || '{}'); + assert.strictEqual(parsed.passed, false, + `Empty contextPath argument must fail closed (caller error), not green-skip. Got: ${JSON.stringify(parsed)}`); + const reason = (parsed.reason || '').toLowerCase(); + assert.ok( + reason.includes('missing') && reason.includes('argument'), + `Reason must identify the missing argument. Got: "${parsed.reason}"` + ); + }); + + test('real path to a genuinely-absent CONTEXT.md → legitimate green skip preserved (#2770)', () => { + // Negative space: a REAL path whose file does not exist is the legitimate skip. + const absentPath = path.join(phaseDir, 'CONTEXT.md'); // never written + const result = runDecisionCoveragePlan(phaseDir, absentPath, tmpDir); + const parsed = JSON.parse(result.output || '{}'); + assert.strictEqual(parsed.passed, true, + `A real path to a genuinely-absent CONTEXT.md is a legitimate green skip. Got: ${JSON.stringify(parsed)}`); + assert.strictEqual(parsed.skipped, true); + assert.ok( + (parsed.reason || '').toLowerCase().includes('context.md missing'), + `Reason must be the legitimate CONTEXT.md-missing skip. Got: "${parsed.reason}"` + ); + }); + + test('undefined-ish argument omitted entirely → passed:false (fail closed)', () => { + // The CLI invocation drops a trailing empty arg in some shells; the handler must + // still fail closed when args[3] is absent (not just empty string). + const result = runGsdTools(['query', 'check.decision-coverage-plan', phaseDir], tmpDir); + const parsed = JSON.parse(result.output || '{}'); + assert.strictEqual(parsed.passed, false, + `Missing contextPath argument must fail closed. Got: ${JSON.stringify(parsed)}`); + }); +}); diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json new file mode 100644 index 000000000..9545990d1 --- /dev/null +++ b/tests/emitted-drift-ack.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "plan-phase.md": { + "reason": "#2770: decision-coverage gate recomputes CONTEXT_PATH in-block + guards the empty-glob case (handler now fails closed on empty arg). Net growth kept under the ADR-857 size cap by condensing adjacent §13a prose/JSON; the residual +89 bytes are the irreducible glob+guard logic." + } + } +} diff --git a/tests/plan-phase-drift-guard.test.cjs b/tests/plan-phase-drift-guard.test.cjs index 1c9a070f4..35a1ccc81 100644 --- a/tests/plan-phase-drift-guard.test.cjs +++ b/tests/plan-phase-drift-guard.test.cjs @@ -1604,25 +1604,26 @@ describe('plan-phase decision-coverage gate (#2492)', () => { assert.ok(decIdx < commitIdx, 'Decision gate must run before commit so failures block the commit'); }); - test('plan-phase Decision Coverage Gate uses CONTEXT_PATH variable defined in INIT extraction (review F1)', () => { - // The CONTEXT_PATH bash variable is defined at Step 4 (`CONTEXT_PATH=$(_gsd_field "$INIT" context_path)`). - // The plan-phase gate snippet must reference the same casing — `${CONTEXT_PATH}` — not `${context_path}`, - // otherwise the BLOCKING gate is invoked with an empty path and silently skips. - const defIdx = md.indexOf('CONTEXT_PATH=$(_gsd_field "$INIT" context_path)'); - assert.ok(defIdx !== -1, 'CONTEXT_PATH must be defined from INIT JSON'); - + test('plan-phase Decision Coverage Gate recomputes CONTEXT_PATH in-block and guards the empty-glob case (#2770)', () => { + // #2770: the CONTEXT_PATH set in the step-1 init Bash block does NOT survive into + // the separately-spawned gate block, so the gate used to run with an empty arg and + // silently green-skip. The gate must now (a) recompute CONTEXT_PATH locally from the + // phase dir, and (b) guard the empty case so a genuinely CONTEXT.md-less phase still + // skips (the handler now fails closed on an empty arg, so an unguarded empty path + // would hard-halt the legitimate "Continue without context" flow). const gateIdx = md.indexOf('check.decision-coverage-plan'); assert.ok(gateIdx !== -1, 'check.decision-coverage-plan invocation must exist'); - // Slice the surrounding gate snippet (~600 chars) and verify variable casing matches the definition. - const snippet = md.slice(Math.max(0, gateIdx - 200), gateIdx + 400); + // The gate invocation is now nested inside the empty-glob guard, so slice a wide + // window around it to capture both the recompute and the guard. + const snippet = md.slice(Math.max(0, gateIdx - 600), gateIdx + 400); assert.ok( - snippet.includes('${CONTEXT_PATH}'), - 'Gate snippet must reference ${CONTEXT_PATH} (uppercase) to match the variable defined in Step 4', + snippet.includes('CONTEXT_PATH=$(ls "${PHASE_DIR}"/*-CONTEXT.md'), + 'Gate must recompute CONTEXT_PATH in-block from the phase-dir glob (not rely on the init-block variable) (#2770)', ); assert.ok( - !snippet.includes('${context_path}'), - 'Gate snippet must NOT reference ${context_path} (lowercase) — that name is undefined in shell scope', + snippet.includes('if [ -n "$CONTEXT_PATH" ]'), + 'Gate must guard the empty-glob case so a CONTEXT.md-less phase still skips (handler now fails closed on empty arg) (#2770)', ); });