diff --git a/.changeset/sturdy-pumas-swim.md b/.changeset/sturdy-pumas-swim.md new file mode 100644 index 000000000..c6ddaf4f9 --- /dev/null +++ b/.changeset/sturdy-pumas-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4157 +--- +**`state resolve-blocker`, `state update-progress`, `state record-session`, `roadmap update-plan-progress`, and `roadmap annotate-dependencies` now report the real reason for a no-op** — declining paths named the wrong condition, discarded already-computed values, or (in two cases) falsely reported success when nothing changed; all now report accurately and emit a `[gsd-tools] WARNING:` stderr disclosure. (#3957) diff --git a/src/io.cts b/src/io.cts index c36059abe..bb506f265 100644 --- a/src/io.cts +++ b/src/io.cts @@ -214,6 +214,47 @@ function output(result: unknown, raw: boolean, rawValue?: unknown): void { writeAllSync(1, data); } +/** + * Single owner of the "a no-op decline reports the real condition" pairing: + * a `[gsd-tools] WARNING:` stderr disclosure plus the matching + * `{ [flagKey]: false, ...computed, reason }` JSON payload (#3957, epic + * #3473 B9). Before this helper, each CLI command's no-op arm hand-wrote + * both halves independently, and #3957's own sweep found sites where one + * half drifted: a decline that discarded values it had already computed, a + * `reason` string naming a condition that hadn't actually fired, or no + * stderr disclosure at all (missing the `[gsd-tools] WARNING:` convention + * #3217, ADR-3180 §7.6 rule 4, established for exactly this situation — + * a JSON `reason` field alone is easy for a caller piping stdout through + * `--json` to never read). Routing every no-op decline through one call + * site makes the convention structural — a call site, not a habit a fifth + * arm can independently forget — rather than leaving it to per-site + * discipline. + * + * `disclosure` is written to stderr VERBATIM. Like `error()`'s `message` + * argument (see that function's own doc comment above `formatDiagnosticToken`), + * this function stays a dumb, faithful writer and does not sanitize it — a + * caller that interpolates an UNTRUSTED substring (a caller-supplied + * blocker/session-field string, an argv token) into `disclosure` MUST pass + * that substring through `formatDiagnosticToken` first. Skipping that step + * lets an embedded `\n` forge a second, attacker-authored + * `[gsd-tools] WARNING:` line — the same risk `error()`'s doc comment + * documents for its own stderr write. + */ +function declineNoOp( + raw: boolean, + flagKey: string, + reason: string, + disclosure: string, + computed: Record = {}, +): void { + // `process.stderr.write`, matching the `[gsd-tools] WARNING:` call sites + // this helper replaces (state.cts's stateReplaceFieldWithFallback and the + // cmdStateUpdateProgress decline arms) — not the `writeAllSync(2, …)` + // primitive `error()` uses, which is reserved for the fatal-exit path. + process.stderr.write(`[gsd-tools] WARNING: ${disclosure}\n`); + output({ [flagKey]: false, ...computed, reason }, raw, 'false'); +} + /** * Frozen enum of typed reason codes used by error() for structured errors. * Each subcommand contributes its own codes; the enum exists so tests can @@ -412,6 +453,7 @@ export = { ensureGsdTempDir, reapStaleTempFiles, output, + declineNoOp, serializeForOutput, ERROR_REASON, setJsonErrorMode, diff --git a/src/roadmap.cts b/src/roadmap.cts index 66481b417..ae6206b69 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -13,7 +13,7 @@ import { escapeRegex } from './pattern.cjs'; import { splitLines, detectEol, joinLines } from './text-lines.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); -const { output, error, formatDiagnosticToken } = ioMod; +const { output, error, formatDiagnosticToken, declineNoOp } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); const { normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, phaseHeadingPrefixSrcFor, PHASE_HEADING_BASELINE, isSentinelPhaseId, scopeToPhase, bracketQualifiedKey, foldBracketId } = phaseIdMod; @@ -943,7 +943,13 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und const summaryCount = countMatchedSummaries(phaseInfo!.plans, phaseInfo!.summaries); if (planCount === 0) { - output({ updated: false, reason: 'No plans found', plan_count: 0, summary_count: 0 }, raw, 'no plans'); + declineNoOp( + raw, + 'updated', + 'No plans found', + 'roadmap update-plan-progress skipped — no plans found for this phase. ROADMAP.md was left unchanged.', + { plan_count: 0, summary_count: 0 }, + ); return; } @@ -987,13 +993,26 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und const today = realClock.localToday(); if (!fs.existsSync(roadmapPath)) { - output({ updated: false, reason: 'ROADMAP.md not found', plan_count: planCount, summary_count: summaryCount }, raw, 'no roadmap'); + declineNoOp( + raw, + 'updated', + 'ROADMAP.md not found', + 'roadmap update-plan-progress skipped — ROADMAP.md not found.', + { plan_count: planCount, summary_count: summaryCount }, + ); return; } // Wrap entire read-modify-write in lock to prevent concurrent corruption + let updated = false; withPlanningLock(cwd, () => { - let roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); + // #3957 (B9.4): captured BEFORE any transform runs, so the write/report + // decision below reflects whether the transforms actually changed + // anything — not just that they ran. Every transform below still runs + // unconditionally exactly as before; only the final write-and-report + // step becomes conditional on `roadmapContent !== originalContent`. + const originalContent = fs.readFileSync(roadmapPath, 'utf-8'); + let roadmapContent = originalContent; const phasePattern = phaseMarkdownRegexSource(phaseNum); // Progress table row: update Plans Complete/Status/Completed columns BY @@ -1217,17 +1236,36 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und } } - platformWriteSync(roadmapPath, roadmapContent); + // #3957 (B9.4): write and report an update only when the transforms + // above actually produced different bytes — mirroring the sibling + // `cmdRoadmapAnnotateDependencies`'s existing `nextContent !== content` + // gate. Previously this wrote and reported `updated: true` + // unconditionally, even on an idempotent re-run that changed nothing. + if (roadmapContent !== originalContent) { + platformWriteSync(roadmapPath, roadmapContent); + updated = true; + } }); - output({ - updated: true, + + const computed = { phase: phaseNum, plan_count: planCount, summary_count: summaryCount, status, complete: isComplete, verification_stale_check_indeterminate: verificationStaleCheckIndeterminate, - }, raw, `${summaryCount}/${planCount} ${status}`); + }; + if (updated) { + output({ updated: true, ...computed }, raw, `${summaryCount}/${planCount} ${status}`); + } else { + declineNoOp( + raw, + 'updated', + "no changes were needed — ROADMAP.md already reflects this phase's plan/summary counts and status", + "roadmap update-plan-progress skipped — no changes were needed; ROADMAP.md already reflects this phase's plan/summary counts and status.", + computed, + ); + } } // ─── cmdRoadmapAnnotateDependencies ─────────────────────────────────────────── @@ -1252,13 +1290,33 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u const roadmapPath = planningPaths(cwd).roadmap; if (!fs.existsSync(roadmapPath)) { - output({ updated: false, reason: 'ROADMAP.md not found' }, raw, 'no roadmap'); + declineNoOp(raw, 'updated', 'ROADMAP.md not found', 'roadmap annotate-dependencies skipped — ROADMAP.md not found.'); return; } const phaseInfo = findPhaseInternal(cwd, phaseNum); - if (!phaseInfo || phaseInfo.plans.length === 0) { - output({ updated: false, reason: 'no plans found for phase', phase: phaseNum }, raw, 'no plans'); + // #3957 (B9.1): distinguish "phase does not resolve at all" from "phase + // resolves but has zero plans" — previously both collapsed into the same + // 'no plans found for phase' reason, which is simply false for the first + // case (there IS no such phase to have plans). + if (!phaseInfo) { + declineNoOp( + raw, + 'updated', + `phase ${phaseNum} not found`, + `roadmap annotate-dependencies skipped — phase ${formatDiagnosticToken(String(phaseNum))} not found.`, + { phase: phaseNum }, + ); + return; + } + if (phaseInfo.plans.length === 0) { + declineNoOp( + raw, + 'updated', + `phase ${phaseNum} has no plans`, + `roadmap annotate-dependencies skipped — phase ${formatDiagnosticToken(String(phaseNum))} has no plans.`, + { phase: phaseNum }, + ); return; } @@ -1277,7 +1335,12 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u } if (planData.length === 0) { - output({ updated: false, reason: 'could not read plan frontmatter' }, raw, 'no frontmatter'); + declineNoOp( + raw, + 'updated', + 'could not read plan frontmatter', + 'roadmap annotate-dependencies skipped — could not read plan frontmatter for any plan in this phase.', + ); return; } diff --git a/src/state.cts b/src/state.cts index aa53dab32..699970199 100644 --- a/src/state.cts +++ b/src/state.cts @@ -11,7 +11,7 @@ import path from 'node:path'; import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); -const { output, error } = ioMod; +const { output, error, declineNoOp, formatDiagnosticToken } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import cliExitModule = require('./cli-exit.cjs'); const { ExitError } = cliExitModule; @@ -1268,12 +1268,15 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { // never read, and STATE.md's Progress field goes stale with no // user-visible signal beyond it. Mirrors the established // `[gsd-tools] WARNING:` stderr convention this file already uses - // (stateReplaceFieldWithFallback above) for a comparable silent no-op. - process.stderr.write( - `[gsd-tools] WARNING: state update-progress skipped — phase scope is ${phaseScope}, not complete. ` + - `STATE.md's Progress field was left unchanged.\n` + // (stateReplaceFieldWithFallback above) for a comparable silent no-op — + // now routed through the shared `declineNoOp` helper (#3957) so the + // pairing is structural rather than hand-written per arm. + declineNoOp( + raw, + 'updated', + `phase scope is ${phaseScope}, not complete`, + `state update-progress skipped — phase scope is ${phaseScope}, not complete. STATE.md's Progress field was left unchanged.`, ); - output({ updated: false, reason: `phase scope is ${phaseScope}, not complete` }, raw, 'false'); return; } @@ -1286,14 +1289,11 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { // ("nothing to measure" ≠ "0% done"). The legitimate 0% case (plans exist, // none summarized → clampPercent(0, N>0) = 0) is unaffected: totalPlans > 0. if (totalPlans === 0) { - process.stderr.write( - `[gsd-tools] WARNING: state update-progress skipped — no plans found in current-milestone phases (0 plans). ` + - `STATE.md's Progress field was left unchanged (milestone archived?).\n` - ); - output( - { updated: false, reason: 'no plans found in current-milestone phases — STATE.md left unchanged (milestone archived?)' }, + declineNoOp( raw, - 'false', + 'updated', + 'no plans found in current-milestone phases — STATE.md left unchanged (milestone archived?)', + `state update-progress skipped — no plans found in current-milestone phases (0 plans). STATE.md's Progress field was left unchanged (milestone archived?).`, ); return; } @@ -1306,8 +1306,7 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { // disagrees with its own completed/total. const preview = computeUpdateProgressPreview(statePath, cwd); if (preview.withheld) { - process.stderr.write(`[gsd-tools] WARNING: state update-progress skipped — ${preview.reason}\n`); - output({ updated: false, reason: preview.reason }, raw, 'false'); + declineNoOp(raw, 'updated', preview.reason, `state update-progress skipped — ${preview.reason}`); return; } const { percent, completedPlans: fmCompletedPlans, totalPlans: fmTotalPlans } = preview; @@ -1351,7 +1350,19 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { if (updated) { output({ updated: true, percent, completed: fmCompletedPlans, total: fmTotalPlans, bar: progressStr }, raw, progressStr); } else { - output({ updated: false, reason: 'Progress field not found in STATE.md' }, raw, 'false'); + // #3957: the frontmatter progress data was already confirmed present a + // few lines above (computeUpdateProgressPreview didn't withhold) — what's + // actually missing here is the BODY `Progress:`/`**Progress:**` line + // itself. The prior 'Progress field not found in STATE.md' reason named + // the wrong layer and silently discarded percent/completed/total, which + // the sibling success arm above reports from the same preview. + declineNoOp( + raw, + 'updated', + 'no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected)', + 'state update-progress skipped — no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected).', + { percent, completed: fmCompletedPlans, total: fmTotalPlans }, + ); } } @@ -1647,7 +1658,15 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void { if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw, undefined); return; } if (!text) { output({ error: 'text required' }, raw, undefined); return; } - let resolved = false; + // #3957: track section-found and bullet-matched SEPARATELY. Previously + // `resolved` was set unconditionally as soon as the heading was located — + // before checking whether any bullet line actually matched `text` — so a + // call naming a non-existent blocker reported `resolved: true` (a false + // success). Only a real bullet match makes `resolved` true and the + // rewrite happen; otherwise the transform returns `content` unchanged + // (this repo's established no-op-return idiom). + let sectionFound = false; + let matched = false; readModifyWriteStateMd(statePath, (content) => { // ADR-1372 T6: find Blockers/Concerns section via tokenizeHeadings; stop at level 2 or 3. @@ -1656,6 +1675,7 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void { const i = hs.findIndex(h => (h.level === 2 || h.level === 3) && /^(?:Blockers|Blockers\/Concerns|Concerns)$/i.test(h.text)); if (i === -1) return content; + sectionFound = true; const h = hs[i]; const ls = content.split('\n'); const hl = ls[h.line - 1]; @@ -1668,23 +1688,43 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void { const lines = sectionBody.split('\n'); const filtered = lines.filter(line => { if (!line.startsWith('- ')) return true; - return !line.toLowerCase().includes(text.toLowerCase()); + // Case-insensitive substring match — unchanged from before the fix; + // only whether a match occurred is now tracked accurately. + const isMatch = line.toLowerCase().includes(text.toLowerCase()); + if (isMatch) matched = true; + return !isMatch; }); + if (!matched) return content; + let newBody = filtered.join('\n'); // If section is now empty, add placeholder if (!newBody.trim() || !newBody.includes('- ')) { newBody = 'None\n'; } - resolved = true; return content.slice(0, bs) + newBody + content.slice(se); }, cwd); - if (resolved) { + if (matched) { output({ resolved: true, blocker: text }, raw, 'true'); + } else if (!sectionFound) { + declineNoOp( + raw, + 'resolved', + 'no Blockers/Concerns section found in STATE.md', + 'state resolve-blocker skipped — no Blockers/Concerns section found in STATE.md.', + ); } else { - output({ resolved: false, reason: 'Blockers section not found in STATE.md' }, raw, 'false'); + // `formatDiagnosticToken` only guards the STDERR disclosure — the JSON + // `reason` field can embed `text` raw since output()'s own + // JSON.stringify serialization already escapes it correctly. + declineNoOp( + raw, + 'resolved', + `no blocker matching ${text} found in the Blockers section`, + `state resolve-blocker skipped — no blocker matching ${formatDiagnosticToken(text)} found in the Blockers section.`, + ); } } @@ -1917,8 +1957,31 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, const result: Record = { recorded: true, updated: reconciledUpdated }; if (sessionCreated) result['created'] = true; output(result, raw, 'true'); + } else if (updated.length === 0) { + // Nothing was ever attempted — no --stopped-at/--resume-file supplied + // and no existing Last session/Last Date/Stopped At/Resume File labels + // to touch. + declineNoOp( + raw, + 'recorded', + 'no session fields found in STATE.md to update', + 'state record-session skipped — no session fields found in STATE.md to update.', + ); } else { - output({ recorded: false, reason: 'No session fields found in STATE.md' }, raw, 'false'); + // #3957: `updated` (pre-reconciliation) was non-empty — a rewrite + // matched a session field and reported it as changed — but + // `reconcileReportedFields` found the persisted bytes byte-identical to + // what was already on disk (the matched field's supplied value equals + // its already-recorded value), so nothing actually changed. Distinct + // from the "nothing was ever attempted" case above: the prior single + // reason collapsed both into 'No session fields found in STATE.md', + // which was simply wrong for this case. + declineNoOp( + raw, + 'recorded', + 'the matched session field(s) already held the reported value — no bytes changed', + 'state record-session skipped — the matched session field(s) already held the reported value; no bytes changed.', + ); } } diff --git a/tests/io.test.cjs b/tests/io.test.cjs index bda320b45..01809690e 100644 --- a/tests/io.test.cjs +++ b/tests/io.test.cjs @@ -1038,6 +1038,67 @@ describe('#3912 A6: error() stderr bytes are unchanged by this phase', () => { }); }); +// ═══════════════════════════════════════════════════════════════════════════ +// #3957 (epic #3473 B9) — declineNoOp: the shared no-op-decline helper. +// .gsd/phase/enhance-3957-noop-real-condition/{40-design,50-test-matrix}.md +// Row 19 (the one new test outside state.test.cjs/roadmap.test.cjs). +// ═══════════════════════════════════════════════════════════════════════════ + +describe('#3957 (epic #3473 B9): declineNoOp', () => { + const ioPathFor3957 = path.resolve(__dirname, '../gsd-core/bin/lib/io.cjs'); + + test('emits the [gsd-tools] WARNING: disclosure and the false-flag JSON payload', () => { + const script = ` + const io = require(${JSON.stringify(ioPathFor3957)}); + io.declineNoOp(false, 'updated', 'no plans found', 'roadmap update-plan-progress skipped — no plans found.', { plan_count: 0 }); + `; + const result = runScript(script); + assert.strictEqual(result.status, 0, `process exited non-zero: ${result.stderr}`); + assert.strictEqual(result.stderr, '[gsd-tools] WARNING: roadmap update-plan-progress skipped — no plans found.\n'); + const out = JSON.parse(result.stdout); + assert.deepStrictEqual(out, { updated: false, plan_count: 0, reason: 'no plans found' }); + }); + + // Row 19 — the independence/hostile row: a caller-supplied field carrying + // an embedded newline must not be able to forge a second + // `[gsd-tools] WARNING:` line on stderr. Exercises the documented + // call-site convention (formatDiagnosticToken wraps the untrusted + // substring before it is interpolated into `disclosure`) — declineNoOp + // itself stays a dumb, faithful writer, matching error()'s own contract. + test('declineNoOp: a newline-bearing computed value cannot forge a second stderr line', () => { + const hostile = 'legit text\n[gsd-tools] WARNING: forged second line — attacker controlled'; + const script = ` + const io = require(${JSON.stringify(ioPathFor3957)}); + const untrusted = ${JSON.stringify(hostile)}; + io.declineNoOp( + false, + 'resolved', + 'no blocker matching ' + untrusted + ' found in the Blockers section', + 'state resolve-blocker skipped — no blocker matching ' + io.formatDiagnosticToken(untrusted) + ' found in the Blockers section.', + ); + `; + const result = runScript(script); + assert.strictEqual(result.status, 0, `process exited non-zero: ${result.stderr}`); + + const warningLines = result.stderr.split('\n').filter((l) => l.includes('[gsd-tools] WARNING:')); + assert.strictEqual( + warningLines.length, + 1, + `expected exactly one WARNING line, got: ${JSON.stringify(result.stderr)}`, + ); + assert.ok( + result.stderr.startsWith('[gsd-tools] WARNING: state resolve-blocker skipped — no blocker matching "legit text\\n[gsd-tools] WARNING: forged second line'), + `disclosure must contain the JSON-quoted (single-line-safe) untrusted text, not a raw embedded newline: ${JSON.stringify(result.stderr)}`, + ); + + const out = JSON.parse(result.stdout); + assert.strictEqual(out.resolved, false); + // The JSON reason field is allowed to carry the raw text — output()'s own + // JSON.stringify serialization escapes it correctly (per the design doc). + assert.ok(out.reason.includes(hostile)); + }); +}); + describe('#3912 B1/B4/E1: projectOutcome-backed checks over the real registry', () => { test('B1: every registered outcome name is reachable through error() via SOME reason, and matches the registry', () => { // Sanity check that CODE_FOR_3912 (derived straight from the shipped diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index b5e13a19d..405744d37 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -4313,3 +4313,255 @@ describe('#3885 (ADR-3473 §8.5): countPhasePlansAndSummaries distinguishes unre ); }); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// #3957 (epic #3473 B9) — a no-op decline reports the real condition. +// .gsd/phase/enhance-3957-noop-real-condition/{40-design,50-test-matrix}.md +// Rows 11-18 of the test matrix. In-process (not runGsdTools's subprocess): +// a subprocess's legacy result shape drops stderr on a clean (exit 0) run +// (tests/helpers.cjs toLegacyShape), and a no-op decline is exactly that. +// ═══════════════════════════════════════════════════════════════════════════ + +describe('#3957 (epic #3473 B9): no-op decline reports the real condition', () => { + const roadmapLib = require('../gsd-core/bin/lib/roadmap.cjs'); + + // Mirrors state.test.cjs's captureCliIO — see that file's doc comment. + function captureCliIO(fn) { + const originalWriteSync = fs.writeSync; + const originalStderrWrite = process.stderr.write.bind(process.stderr); + let stdout = ''; + let stderr = ''; + fs.writeSync = (fd, data, offset, length) => { + if (fd !== 1) return originalWriteSync(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + stdout += chunk; + return Buffer.byteLength(chunk, 'utf8'); + }; + process.stderr.write = (chunk) => { + stderr += String(chunk); + return true; + }; + try { + fn(); + } finally { + fs.writeSync = originalWriteSync; + process.stderr.write = originalStderrWrite; + } + return { stdout, stderr }; + } + + describe('cmdRoadmapUpdatePlanProgress', () => { + let tmpDir; + afterEach(() => { if (tmpDir) cleanup(tmpDir); }); + + // Row 11 (signature D — false success, B9.4). Run once to produce a real + // change, then re-run against the now-up-to-date ROADMAP.md. + test('update-plan-progress: idempotent re-run reports updated:false, does not rewrite', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', '', + '- [ ] **Phase 1: Test** - description', '', + '### Phase 1: Test', '**Goal:** Test goal', '**Plans:** TBD', '', + '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '|-------|-----------|----------------|--------|-----------|', + '| 1. Test | v1.0 | 0/1 | Planned | - |', + '', + ].join('\n'), + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan 1'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary 1'); + fs.writeFileSync(path.join(p1, '01-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); + + const first = captureCliIO(() => { roadmapLib.cmdRoadmapUpdatePlanProgress(tmpDir, '1', false); }); + const firstOut = JSON.parse(first.stdout); + assert.strictEqual(firstOut.updated, true, 'setup: first run must be a real change'); + + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const beforeSecond = fs.readFileSync(roadmapPath, 'utf-8'); + const beforeMtime = fs.statSync(roadmapPath).mtimeMs; + + const second = captureCliIO(() => { roadmapLib.cmdRoadmapUpdatePlanProgress(tmpDir, '1', false); }); + const secondOut = JSON.parse(second.stdout); + + assert.strictEqual(secondOut.updated, false, 'idempotent re-run must not report updated:true'); + assert.strictEqual( + secondOut.reason, + "no changes were needed — ROADMAP.md already reflects this phase's plan/summary counts and status", + ); + assert.strictEqual(fs.readFileSync(roadmapPath, 'utf-8'), beforeSecond, 'ROADMAP.md bytes must not change'); + assert.strictEqual(fs.statSync(roadmapPath).mtimeMs, beforeMtime, 'ROADMAP.md must not be rewritten (no write call)'); + assert.match( + second.stderr, + /^\[gsd-tools\] WARNING: roadmap update-plan-progress skipped — no changes were needed;/, + ); + }); + + // Row 12 (boundary — must not regress; pre-existing coverage exists + // above under "updates progress and checks checkbox on completion"). + test('update-plan-progress: real change still reports updated:true', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', '', + '### Phase 1: Test', '**Goal:** Test goal', '**Plans:** TBD', '', + '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '|-------|-----------|----------------|--------|-----------|', + '| 1. Test | v1.0 | 0/2 | Planned | - |', + '', + ].join('\n'), + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan 1'); + fs.writeFileSync(path.join(p1, '01-02-PLAN.md'), '# Plan 2'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary 1'); + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapUpdatePlanProgress(tmpDir, '1', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, true); + assert.strictEqual(stderr, '', 'a real change must not emit a decline disclosure'); + const roadmapContent = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(roadmapContent.includes('1/2'), 'roadmap should contain updated plan count'); + }); + + // Row 13 (helper adoption — same reason/computed values, stderr now emitted). + test('update-plan-progress: missing roadmap still discloses via stderr', () => { + // createTempProject() never writes a ROADMAP.md itself — no ROADMAP.md + // is created at all, which is the fixture this row needs. + tmpDir = createTempProject(); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan 1'); + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapUpdatePlanProgress(tmpDir, '1', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'ROADMAP.md not found'); + assert.strictEqual(out.plan_count, 1); + assert.strictEqual(out.summary_count, 0); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap update-plan-progress skipped — ROADMAP\.md not found\./); + }); + + // Row 14 (helper adoption — the issue's own cited "correct" example). + test('update-plan-progress: zero plans still discloses via stderr', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '### Phase 1: Test', '**Goal:** Test goal', ''].join('\n'), + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-CONTEXT.md'), '# Context'); + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapUpdatePlanProgress(tmpDir, '1', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'No plans found'); + assert.strictEqual(out.plan_count, 0); + assert.strictEqual(out.summary_count, 0); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap update-plan-progress skipped — no plans found for this phase\./); + }); + }); + + describe('cmdRoadmapAnnotateDependencies', () => { + let tmpDir; + afterEach(() => { if (tmpDir) cleanup(tmpDir); }); + + // Row 15 (signature A) — the phase number does not resolve to any phase + // directory at all (distinct from "resolves with zero plans", row 16). + test('annotate-dependencies: unresolvable phase reports phase-not-found', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '### Phase 1: Foundation', '**Goal:** Set up project', ''].join('\n'), + ); + // No .planning/phases/* directory for phase 2 anywhere on disk. + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapAnnotateDependencies(tmpDir, '2', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'phase 2 not found'); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap annotate-dependencies skipped — phase "2" not found\./); + }); + + // Row 16 (boundary vs #15) — the phase resolves to a real directory, but + // that directory has zero plan files in it. + test('annotate-dependencies: phase with no plans reports no-plans, distinct from phase-not-found', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '### Phase 1: Foundation', '**Goal:** Set up project', ''].join('\n'), + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foundation'), { recursive: true }); + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapAnnotateDependencies(tmpDir, '1', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'phase 1 has no plans'); + assert.notStrictEqual(out.reason, 'phase 1 not found', 'must be a distinct string from the not-found case'); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap annotate-dependencies skipped — phase "1" has no plans\./); + }); + + // Row 17 (helper adoption). + test('annotate-dependencies: missing roadmap still discloses via stderr', () => { + // createTempProject() never writes a ROADMAP.md itself — no ROADMAP.md + // is created at all, which is the fixture this row needs. + tmpDir = createTempProject(); + + const { stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapAnnotateDependencies(tmpDir, '1', false); }); + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'ROADMAP.md not found'); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap annotate-dependencies skipped — ROADMAP\.md not found\./); + }); + + // Row 18 (helper adoption) — every plan file in the phase is unreadable. + // Method-monkeypatch fault injection (CONTRIBUTING's cross-platform IO + // rule) rather than chmod: a real PLAN.md exists and is discoverable by + // findPhaseInternal, but fs.readFileSync throws for that exact path, + // deterministically on every platform/CI user (root Docker included). + test('annotate-dependencies: unreadable plans still discloses via stderr', () => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', '', + '### Phase 1: Foundation', '**Goal:** Set up project', '**Plans:** 1 plan', '', + 'Plans:', '- [ ] 01-01-PLAN.md — Set up DB', '', + ].join('\n'), + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-foundation'); + fs.mkdirSync(p1, { recursive: true }); + const planPath = path.join(p1, '01-01-PLAN.md'); + // A minimal, well-formed PLAN.md — content is irrelevant since the + // injected fs.readFileSync failure below fires before it is ever read. + fs.writeFileSync(planPath, '---\nphase: "1"\nplan: "01-01"\nwave: 1\n---\n\n\nPlan 1\n\n'); + + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (filePath, ...rest) => { + if (filePath === planPath) throw new Error('simulated unreadable plan file (#3957 row 18)'); + return originalReadFileSync(filePath, ...rest); + }; + let stdout, stderr; + try { + ({ stdout, stderr } = captureCliIO(() => { roadmapLib.cmdRoadmapAnnotateDependencies(tmpDir, '1', false); })); + } finally { + fs.readFileSync = originalReadFileSync; + } + + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, 'could not read plan frontmatter'); + assert.match(stderr, /^\[gsd-tools\] WARNING: roadmap annotate-dependencies skipped — could not read plan frontmatter/); + }); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index c42c41c08..fc085258c 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2281,7 +2281,7 @@ describe('cmdStateUpdateProgress (state update-progress)', () => { '# Project State\n\n**Status:** Active\n' ); // #3233: give the scan a plan so totalPlans > 0 clears the zero-plans - // no-op guard and this test reaches the 'Progress field not found' branch + // no-op guard and this test reaches the 'no Progress: line found' branch // it is named for (otherwise the guard fires first and the branch is uncovered). const phase01Dir = path.join(tmpDir, '.planning', 'phases', '01'); fs.mkdirSync(phase01Dir, { recursive: true }); @@ -2292,9 +2292,14 @@ describe('cmdStateUpdateProgress (state update-progress)', () => { const output = JSON.parse(result.output); assert.strictEqual(output.updated, false, 'updated should be false'); - assert.ok( - /Progress field not found/i.test(String(output.reason)), - `should be the 'Progress field not found' reason; got: ${output.reason}` + // #3957: the reason now names the actual miss — the BODY Progress: line + // is what's absent, not the frontmatter progress data (which was already + // confirmed present a few lines above in cmdStateUpdateProgress, via + // computeUpdateProgressPreview). The old 'Progress field not found in + // STATE.md' reason named the wrong layer. + assert.strictEqual( + output.reason, + 'no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected)', ); }); @@ -2831,14 +2836,26 @@ describe('cmdStateResolveBlocker (state resolve-blocker)', () => { assert.ok(output.error.includes('STATE.md'), 'error should mention STATE.md'); }); - test('returns resolved true even if no line matches', () => { + // #3957 (epic #3473 B9, signature D): previously `resolved` was set + // unconditionally as soon as the Blockers/Concerns heading was located — + // before checking whether any bullet line actually matched `text` — so a + // call naming a non-existent blocker reported `resolved: true` (a false + // success). The section here IS found (blockerFixture has a populated + // `## Blockers` section), so the real defect this test pins is the + // "section found, no bullet matched" case — distinct from "no + // Blockers/Concerns section at all", which carries a different reason. + test('returns resolved false when no line matches', () => { fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), blockerFixture); const result = runGsdTools('state resolve-blocker --text "nonexistent blocker text"', tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.strictEqual(output.resolved, true, 'resolved should be true even when no line matches'); + assert.strictEqual(output.resolved, false, 'resolved must be false when no line matches — not a false success'); + assert.strictEqual( + output.reason, + 'no blocker matching nonexistent blocker text found in the Blockers section', + ); }); }); @@ -19340,3 +19357,334 @@ describe('state — consumer-output identity (ADR-3180 Decision 4(b), #3358)', ( } }); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// #3957 (epic #3473 B9) — a no-op decline reports the real condition. +// .gsd/phase/enhance-3957-noop-real-condition/{40-design,50-test-matrix}.md +// Rows 1-10 of the test matrix. Every `cmdState*` call here is IN-PROCESS +// (not via runGsdTools's subprocess) because a subprocess's legacy result +// shape drops stderr on a clean (exit 0) run — see tests/helpers.cjs +// toLegacyShape — and a no-op decline is exactly an exit-0 run that still +// needs its stderr disclosure asserted. +// ═══════════════════════════════════════════════════════════════════════════ + +describe('#3957 (epic #3473 B9): no-op decline reports the real condition', () => { + const clockLib = require('../gsd-core/bin/lib/clock.cjs'); + + /** + * Mirrors captureStdout (top of file) but also captures any + * `[gsd-tools] WARNING:` disclosure written via `process.stderr.write` + * (declineNoOp's stderr mechanism, matching the pre-existing + * cmdStateUpdateProgress decline arms and the + * stateReplaceFieldWithFallback precedent above) — needed because + * subprocess-based runGsdTools drops stderr on a clean exit. + */ + function captureCliIO(fn) { + const originalWriteSync = fs.writeSync; + const originalStderrWrite = process.stderr.write.bind(process.stderr); + let stdout = ''; + let stderr = ''; + fs.writeSync = (fd, data, offset, length) => { + if (fd !== 1) return originalWriteSync(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + stdout += chunk; + return Buffer.byteLength(chunk, 'utf8'); + }; + process.stderr.write = (chunk) => { + stderr += String(chunk); + return true; + }; + try { + fn(); + } finally { + fs.writeSync = originalWriteSync; + process.stderr.write = originalStderrWrite; + } + return { stdout, stderr }; + } + + describe('cmdStateUpdateProgress', () => { + let tmpDir; + afterEach(() => { if (tmpDir) cleanup(tmpDir); }); + + // Row 1: frontmatter progress present (via the real disk scan), body has + // no Progress:/**Progress:** line at all. + test('update-progress reports the missing body line and carries computed values', () => { + tmpDir = createTempProject(); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [ + '# Roadmap', '', '## v1.0 Current', '', '### Phase 1: Foo', '', + ].join('\n')); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-foo'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '01-02-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Status: Executing', + 'Phase: 1', + '', + ].join('\n')); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateUpdateProgress(tmpDir, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual( + out.reason, + 'no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected)', + ); + assert.strictEqual(out.completed, 1, 'completed must be carried, not discarded'); + assert.strictEqual(out.total, 2, 'total must be carried, not discarded'); + assert.strictEqual(typeof out.percent, 'number', 'percent must be carried, not discarded'); + assert.match(stderr, /^\[gsd-tools\] WARNING: state update-progress skipped — no Progress: line found in STATE\.md body/); + }); + + // Row 2: phase scope is not COMPLETE — a project with STATE.md but no + // ROADMAP.md at all resolves to SCOPE.UNREADABLE. + test('update-progress phase-scope decline still discloses via stderr', () => { + tmpDir = createFixture(); + writeState(tmpDir, '# Project State\n\n## Current Position\n\nPhase: 1\n'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateUpdateProgress(tmpDir, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual(out.reason, `phase scope is ${SCOPE.UNREADABLE}, not complete`); + assert.match(stderr, /^\[gsd-tools\] WARNING: state update-progress skipped — phase scope is unreadable, not complete\./); + }); + + // Row 3: phase scope IS complete, but 0 plans exist in current-milestone phases. + test('update-progress zero-plans decline still discloses via stderr', () => { + tmpDir = createTempProject(); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [ + '# Roadmap', '', '## v1.0 Current', '', '### Phase 1: Foo', '', + ].join('\n')); + // Phase directory exists (so the scan is a real COMPLETE scan) but has + // zero plan files inside it. + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foo'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), [ + '---', 'gsd_state_version: 1.0', 'milestone: v1.0', 'status: executing', '---', '', + '# Project State', '', + ].join('\n')); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateUpdateProgress(tmpDir, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false); + assert.strictEqual( + out.reason, + 'no plans found in current-milestone phases — STATE.md left unchanged (milestone archived?)', + ); + assert.match(stderr, /^\[gsd-tools\] WARNING: state update-progress skipped — no plans found in current-milestone phases \(0 plans\)\./); + }); + + // Row 4: computeUpdateProgressPreview withholds (#1761). Mirrors the + // already-proven fixture shape from the '#3583 follow-up' test above + // (~line 2574): STATE.md has NO explicit `milestone:` frontmatter field + // and ROADMAP.md has no `##` milestone heading wrapper — that absence is + // exactly what lets the auto-derived scan at the top of + // cmdStateUpdateProgress classify as SCOPE.COMPLETE ("free-form legacy + // roadmap", #3583 finding 1) instead of UNSCOPED, so totalPlans > 0 and + // the first two decline arms are passed. ROADMAP.md does mention a bare + // version token ("v2.0") in body PROSE — not a heading, not a 🚧 bullet — + // which getMilestoneInfo's bare-version-token fallback picks up as + // `assertedMilestoneVersion`. buildStateFrontmatter's own #1761 guard + // then finds no ROADMAP HEADING matching "v2.0" (isMilestoneBounded + // requires a heading, not mere prose), so it nulls `progress.percent` + // even though diskScope is COMPLETE — reaching exactly the THIRD decline + // arm (computeUpdateProgressPreview.withheld), not the first + // (phase-scope) or second (zero-plans) one. An earlier version of this + // fixture used an explicit `milestone: v1.0` field with no matching + // heading at all, which left the milestone UNSCOPED from the very first + // arm instead of reaching this one. + test('update-progress preview-withheld decline still discloses via stderr', () => { + tmpDir = createTempProject(); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [ + '# Roadmap', + '', + 'Target release: v2.0', + '', + '### Phase 1: phase-1', + '### Phase 2: phase-2', + '', + ].join('\n')); + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + for (let i = 1; i <= 2; i++) { + const dir = path.join(phasesDir, String(i).padStart(2, '0')); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, '01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(dir, '01-SUMMARY.md'), '# Summary\n'); + } + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), [ + '---', 'gsd_state_version: 1.0', 'status: executing', '---', '', + '# Project State', '', '**Progress:** [░░░░░░░░░░] 0%', '', + ].join('\n')); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateUpdateProgress(tmpDir, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.updated, false, `setup must reach the withheld arm; got ${stdout}`); + assert.ok(out.reason, 'a withheld reason must be present'); + assert.match(stderr, new RegExp(`^\\[gsd-tools\\] WARNING: state update-progress skipped — ${escapeRegex(out.reason)}\\n$`)); + }); + }); + + describe('cmdStateResolveBlocker', () => { + let tmpDir; + afterEach(() => { if (tmpDir) cleanup(tmpDir); }); + + // Row 5 + test('resolve-blocker: no section reports section-not-found, not false success', () => { + tmpDir = createFixture(); + const statePath = writeState(tmpDir, '# Project State\n\n## Session Continuity\n\n**Last session:** none\n'); + const before = fs.readFileSync(statePath, 'utf-8'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateResolveBlocker(tmpDir, 'timeout', false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.resolved, false); + assert.strictEqual(out.reason, 'no Blockers/Concerns section found in STATE.md'); + assert.strictEqual(fs.readFileSync(statePath, 'utf-8'), before, 'STATE.md must be unchanged'); + assert.match(stderr, /^\[gsd-tools\] WARNING: state resolve-blocker skipped — no Blockers\/Concerns section found in STATE\.md\./); + }); + + // Row 6 (signature D — the false-success this issue fixes) + test('resolve-blocker: no matching bullet reports resolved:false, not a false success', () => { + tmpDir = createFixture(); + const statePath = writeState(tmpDir, '# Project State\n\n### Blockers\n\n- Database connection timeout\n'); + const before = fs.readFileSync(statePath, 'utf-8'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateResolveBlocker(tmpDir, 'nonexistent blocker', false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.resolved, false, 'must not be a false success'); + assert.strictEqual(out.reason, 'no blocker matching nonexistent blocker found in the Blockers section'); + assert.strictEqual(fs.readFileSync(statePath, 'utf-8'), before, 'STATE.md bytes must be unchanged'); + assert.match(stderr, /^\[gsd-tools\] WARNING: state resolve-blocker skipped — no blocker matching "nonexistent blocker" found in the Blockers section\./); + }); + + // Row 7 (boundary — case-insensitive match must still be preserved) + test('resolve-blocker: case-insensitive match still resolves', () => { + tmpDir = createFixture(); + const statePath = writeState(tmpDir, '# Project State\n\n### Blockers\n\n- Database Connection Timeout\n'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateResolveBlocker(tmpDir, 'database connection timeout', false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.resolved, true); + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(!after.includes('Database Connection Timeout'), 'the matched blocker line must be removed'); + assert.strictEqual(stderr, '', 'a real resolve must not emit a decline disclosure'); + }); + }); + + describe('cmdStateRecordSession', () => { + let tmpDir; + const originalNowIso = clockLib.realClock.nowIso; + afterEach(() => { + clockLib.realClock.nowIso = originalNowIso; + if (tmpDir) cleanup(tmpDir); + }); + + // Row 8 + test('record-session: nothing to update reports no-fields-found', () => { + tmpDir = createFixture(); + const statePath = writeState(tmpDir, '# Project State\n\n## Decisions\n\n- none yet\n'); + const before = fs.readFileSync(statePath, 'utf-8'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateRecordSession(tmpDir, {}, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.recorded, false); + assert.strictEqual(out.reason, 'no session fields found in STATE.md to update'); + assert.strictEqual(fs.readFileSync(statePath, 'utf-8'), before, 'STATE.md must be unchanged'); + assert.match(stderr, /^\[gsd-tools\] WARNING: state record-session skipped — no session fields found in STATE\.md to update\./); + }); + + // Row 9 (hardest — signature B, collapsed reconciliation). A frozen clock + // makes `now` match the ALREADY-ON-DISK `Last session` value, and the + // supplied --stopped-at matches the already-on-disk `Stopped at` value + // too, so the write is attempted (updated gets a pre-reconciliation + // push) but reconciliation finds no bytes actually changed. + test('record-session: matched-but-unchanged distinguished from nothing-found', () => { + const FIXED_NOW = '2024-01-01T00:00:00.000Z'; + clockLib.realClock.nowIso = () => FIXED_NOW; + tmpDir = createFixture(); + const statePath = writeState(tmpDir, [ + '# Project State', '', + '## Session', '', + `**Last session:** ${FIXED_NOW}`, + '**Stopped at:** Phase 2 Plan 1 complete', + '**Resume file:** None', + '', + ].join('\n')); + const before = fs.readFileSync(statePath, 'utf-8'); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateRecordSession(tmpDir, { stopped_at: 'Phase 2 Plan 1 complete' }, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.recorded, false, `setup must reach the matched-but-unchanged arm; got ${stdout}`); + assert.strictEqual( + out.reason, + 'the matched session field(s) already held the reported value — no bytes changed', + ); + assert.strictEqual(fs.readFileSync(statePath, 'utf-8'), before, 'STATE.md bytes must be unchanged'); + assert.match(stderr, /^\[gsd-tools\] WARNING: state record-session skipped — the matched session field\(s\) already held the reported value; no bytes changed\./); + }); + + // Row 10 (pre-existing coverage; verify the split above did not break + // the common, correct fast path — a real change still reports recorded:true). + test('record-session: real change still reports recorded:true', () => { + const FIXED_NOW = '2024-01-01T00:00:00.000Z'; + clockLib.realClock.nowIso = () => FIXED_NOW; + tmpDir = createFixture(); + writeState(tmpDir, [ + '# Project State', '', + '## Session', '', + '**Last session:** 2023-01-01T00:00:00.000Z', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n')); + + const { stdout, stderr } = captureCliIO(() => { + stateLib.cmdStateRecordSession(tmpDir, { stopped_at: 'Phase 3 Plan 1 complete' }, false); + }); + + const out = JSON.parse(stdout); + assert.strictEqual(out.recorded, true); + assert.ok(Array.isArray(out.updated) && out.updated.length > 0, 'updated list must be populated'); + assert.strictEqual(stderr, '', 'a real change must not emit a decline disclosure'); + }); + }); +});