diff --git a/.changeset/kind-sloths-climb.md b/.changeset/kind-sloths-climb.md new file mode 100644 index 000000000..5269c7130 --- /dev/null +++ b/.changeset/kind-sloths-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3093 +--- +**`docs/json-errors.md` now documents the ExitError plain-text carve-out** — the page previously claimed every CLI error emits a structured JSON envelope on stderr, but usage errors (ExitError) intentionally emit plain text with their own exit code. The structured-envelope guidance is now scoped to non-usage failures, with the carve-out stated explicitly and a characterization test pinning both paths. (#2979) diff --git a/.changeset/lucky-tunas-leap.md b/.changeset/lucky-tunas-leap.md new file mode 100644 index 000000000..c548f2d6d --- /dev/null +++ b/.changeset/lucky-tunas-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3091 +--- +**`progress.completed_plans` no longer stays pinned after a gap-closure cycle** — when plan-phase re-planned a phase and added gap-closure plans, `total_plans` corrected upward but `completed_plans` was restored to its pre-growth value, so STATE.md showed `completed_plans < total_plans` permanently even after every plan (including the gap-closure ones) was summarized. `completed_plans` and `completed_phases` now ratchet up to the disk-derived count under the plan-phase progress opt-in (never deriving downward, preserving the curated-progress ratchet for unrelated edits). (#2969) diff --git a/.changeset/proud-eagles-chatter.md b/.changeset/proud-eagles-chatter.md new file mode 100644 index 000000000..49f9b3016 --- /dev/null +++ b/.changeset/proud-eagles-chatter.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3097 +--- +**`/gsd` commands in Pi now display their output** — the command handler returned output as a bare string, which Pi's ExtensionAPI silently dropped. It now returns Pi's structured `{ content: [{ type: 'text', text }] }` display shape (matching the `gsd_invoke` tool's proven contract), so success output and error messages are visible. (#2991) diff --git a/.changeset/proud-pumas-bark.md b/.changeset/proud-pumas-bark.md new file mode 100644 index 000000000..1cbbe9665 --- /dev/null +++ b/.changeset/proud-pumas-bark.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3096 +--- +**`/gsd-code-review` no longer picks a wrong diff base from unanchored commit-message grep** — the diff-base fallback searched all commit messages for the bare phase number as a substring, matching version strings, dates, and issue refs, then took the oldest match. The grep is now anchored to the phase-mention convention (`Phase N` with a word boundary), so the fail-closed branch is reachable when no commit genuinely references the phase. (#2989) diff --git a/.changeset/silly-moles-rally.md b/.changeset/silly-moles-rally.md new file mode 100644 index 000000000..f1a88560b --- /dev/null +++ b/.changeset/silly-moles-rally.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3092 +--- +**`roadmap validate` now performs real structural validation** — it previously returned `{"warnings":[]}` (exit 0) for every input including empty files, garbage text, and missing files, providing false assurance. It now checks file existence/readability, emptiness, frontmatter well-formedness, and the presence of at least one phase entry, exiting non-zero on any warning (per its documented contract). The existing opt-in milestone-prefix consistency check is preserved. (#2978) diff --git a/.changeset/witty-yaks-munch.md b/.changeset/witty-yaks-munch.md new file mode 100644 index 000000000..458bf18eb --- /dev/null +++ b/.changeset/witty-yaks-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3095 +--- +**Local `lint:changeset` and `lint:docs-required` now diff against `next` instead of `main`** — the local fallback was the release branch (`main`), which lags far behind the integration branch (`next`), so the lint always passed by finding fragments from other already-merged PRs in the oversized diff range. The local invocation now matches the base CI uses. (#2988) diff --git a/docs/json-errors.md b/docs/json-errors.md index c12c058c6..2da5a9869 100644 --- a/docs/json-errors.md +++ b/docs/json-errors.md @@ -2,11 +2,12 @@ ## Overview -`gsd-tools` supports a **JSON error mode** that emits all errors as structured +`gsd-tools` supports a **JSON error mode** that emits most errors as structured JSON objects on stderr instead of free-form text. This is the recommended surface for tests and tooling that need to assert on error types without grepping raw text (see `CONTRIBUTING.md` — "Prohibited: Raw Text Matching on -Test Outputs"). +Test Outputs"). Usage errors are an intentional exception — see the +`ExitError` carve-out below. ## Activating @@ -37,6 +38,20 @@ Fields: | `reason` | string | Typed reason code from the taxonomy below. | | `message` | string | Human-readable description (may change; do not assert on it). | +### `ExitError` carve-out (plain text, not JSON) + +Usage errors and explicit exit-code signals take a **different path**: they +throw `ExitError` (`src/cli-exit.cts`), which `runMain` catches *before* the +JSON-envelope branch. An `ExitError` writes its `message` as **plain text** +to stderr (not a JSON object) and exits with the error's own `code` (which +may differ from 1). This is intentional — usage messages are operator-facing +prose, not structured failures. + +If you are testing a usage/flag error, do **not** parse stderr as JSON; +assert on the exit code and (if needed) the plain-text message. The +"parse stderr as JSON" guidance below applies only to the structured-envelope +branch (non-`ExitError` failures). + ## Error code taxonomy Codes are frozen constants in `gsd-core/bin/lib/core.cjs` under @@ -99,8 +114,9 @@ text (unstable). ## Writing tests -Always parse stderr with `JSON.parse` and assert on typed fields. Never use -`.includes()`, `.match()`, or regex on the raw error string. +For **non-usage** errors (the structured-envelope branch), parse stderr with +`JSON.parse` and assert on typed fields. Never use `.includes()`, `.match()`, +or regex on the raw error string. ```js // CORRECT: parse then assert on typed field diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index b64264738..9adc0f639 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -236,8 +236,13 @@ Additionally, whenever a reliable diff base is available, cross-check the SUMMAR against the diff and warn about (then add) any changed files the SUMMARY extractor did not surface — so a partial SUMMARY result can no longer silently mask the rest of the phase. ```bash -# Compute diff base from phase commits — fail closed if no reliable base found -PHASE_COMMITS=$(git log --oneline --all --grep="${PADDED_PHASE}" --format="%H" 2>/dev/null) +# Compute diff base from phase commits — fail closed if no reliable base found. +# #2989: anchor the grep to the phase-mention convention ("Phase N" / "phase N" +# with a word boundary) so a bare digit substring doesn't match version strings, +# dates, issue refs, or other phases' numbers. With --extended-regexp, \b is +# a word boundary. When no commit genuinely references the phase, this yields +# empty and the fail-closed warning below actually fires. +PHASE_COMMITS=$(git log --oneline --all --grep="[Pp]hase ${PADDED_PHASE}\b" --extended-regexp --format="%H" 2>/dev/null) DIFF_BASE="" if [ -n "$PHASE_COMMITS" ]; then DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1)^ diff --git a/pi/gsd.cjs b/pi/gsd.cjs index c30bd738a..9a3fdc702 100644 --- a/pi/gsd.cjs +++ b/pi/gsd.cjs @@ -287,11 +287,16 @@ module.exports = function gsdPiExtension(pi) { try { ({ dispatchGsdCommand } = require(path.join(GSD_CORE, 'bin', 'lib', 'shell-command-projection.cjs'))); } catch (e) { - return `GSD engine unavailable: ${e && e.message ? e.message : String(e)}`; + // #2991: return the structured { content } shape Pi's ExtensionAPI + // displays, not a bare string (which Pi silently drops). + return { content: [{ type: 'text', text: `GSD engine unavailable: ${e && e.message ? e.message : String(e)}` }] }; } const result = dispatchGsdCommand({ family, subcommand, args: rest, cwd }); - if (result.ok) return result.stdout; - return `GSD error: ${result.stderr || result.stdout || `dispatch failed (exit ${result.code})`}`; + // #2991: match gsd_invoke's proven output shape so Pi actually displays it. + const text = result.ok + ? result.stdout + : `GSD error: ${result.stderr || result.stdout || `dispatch failed (exit ${result.code})`}`; + return { content: [{ type: 'text', text }] }; }, }); diff --git a/scripts/changeset/lint.cjs b/scripts/changeset/lint.cjs index 9fe975093..933d5fe55 100755 --- a/scripts/changeset/lint.cjs +++ b/scripts/changeset/lint.cjs @@ -12,6 +12,10 @@ * Tests assert on the typed verdict, never on free text. */ +// #2988: the repo's integration/default branch — the base every PR targets. +// Used as the local fallback when GITHUB_BASE_REF is unset (CI sets it). +const DEFAULT_BASE = 'next'; + const LINT_REASON = Object.freeze({ OK_FRAGMENT_PRESENT: 'ok_fragment_present', OK_OPT_OUT_LABEL: 'ok_opt_out_label', @@ -80,7 +84,10 @@ function main() { labels = (event.pull_request?.labels || []).map((l) => l.name); } catch { /* fall through */ } } - const base = process.env.GITHUB_BASE_REF || 'main'; + // #2988: local fallback must match the repo's integration branch (`next`), + // not the release branch (`main`). CI sets GITHUB_BASE_REF explicitly; the + // fallback only fires locally, where `next` is the base every PR targets. + const base = process.env.GITHUB_BASE_REF || DEFAULT_BASE; let changedFiles = []; try { // Use execFileSync with an argv array — the base ref is interpolated @@ -145,4 +152,4 @@ function main() { if (require.main === module) runMain(main); -module.exports = { evaluateLint, LINT_REASON, OPT_OUT_LABEL, isUserFacing, isFragment }; +module.exports = { evaluateLint, LINT_REASON, OPT_OUT_LABEL, isUserFacing, isFragment, DEFAULT_BASE }; diff --git a/scripts/lint-docs-required.cjs b/scripts/lint-docs-required.cjs index a6580cb4a..3bcce28ca 100755 --- a/scripts/lint-docs-required.cjs +++ b/scripts/lint-docs-required.cjs @@ -16,6 +16,10 @@ const { parseFragment, FRAGMENT_ERROR } = require('./changeset/parse.cjs'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +// #2988: the repo's integration/default branch — the base every PR targets. +// Used as the local fallback when GITHUB_BASE_REF is unset (CI sets it). +const DEFAULT_BASE = 'next'; + const LINT_REASON = Object.freeze({ OK_NO_TRIGGERING_FRAGMENTS: 'ok_no_triggering_fragments', OK_DOCS_UPDATED: 'ok_docs_updated', @@ -149,7 +153,10 @@ function main() { } catch { /* fall through */ } } - const base = process.env.GITHUB_BASE_REF || 'main'; + // #2988: local fallback must match the repo's integration branch (`next`), + // not the release branch (`main`). CI sets GITHUB_BASE_REF explicitly; the + // fallback only fires locally, where `next` is the base every PR targets. + const base = process.env.GITHUB_BASE_REF || DEFAULT_BASE; let changedFiles = []; try { // execFileSync with argv — no shell, so a malicious GITHUB_BASE_REF @@ -216,6 +223,7 @@ module.exports = { OPT_OUT_LABEL, TRIGGERING_TYPES, FRAGMENT_ERROR, + DEFAULT_BASE, isFragmentPath, isDocsFile, isExemptFragment, diff --git a/src/roadmap-command-router.cts b/src/roadmap-command-router.cts index 7ef26d5a1..7b54d8cf4 100644 --- a/src/roadmap-command-router.cts +++ b/src/roadmap-command-router.cts @@ -21,6 +21,9 @@ const { planningDir } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import configLoaderMod = require('./config-loader.cjs'); const { loadConfig } = configLoaderMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import cliExitMod = require('./cli-exit.cjs'); +const { ExitError } = cliExitMod; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -143,11 +146,43 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom 'annotate-dependencies': () => roadmap.cmdRoadmapAnnotateDependencies(cwd, args[2], raw), 'validate': () => { const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); - let roadmapContent = ''; + const warnings: Array<{ code: string; message: string }> = []; + + // #2978: structural validation. A verb named "validate" that cannot + // produce a negative result provides false assurance. Before the + // opt-in milestone-prefix check, verify the file is structurally a + // roadmap at all. + let roadmapContent: string; try { roadmapContent = fs.readFileSync(roadmapPath, 'utf8'); } catch { - // ROADMAP.md missing — return empty warnings + // ROADMAP.md missing — not silent success. + warnings.push({ code: 'V001', message: 'ROADMAP.md not found or unreadable' }); + const result = { warnings }; + process.stdout.write(raw ? JSON.stringify(result) : JSON.stringify(result, null, 2)); + throw new ExitError(1); + } + + // Empty or whitespace-only. + if (roadmapContent.trim() === '') { + warnings.push({ code: 'V002', message: 'ROADMAP.md is empty' }); + } + + // Malformed frontmatter — a `---` opener with no matching closer. + // Tolerate a leading BOM (#3057) before the fence. + const contentAfterBom = roadmapContent.replace(/^\uFEFF/, ''); + if (contentAfterBom.startsWith('---')) { + const closeMatch = contentAfterBom.slice(3).match(/\r?\n---\s*(\r?\n|$)/); + if (!closeMatch) { + warnings.push({ code: 'V003', message: 'ROADMAP.md frontmatter is malformed (unterminated --- fence)' }); + } + } + + // No recognizable phase structure — at least one `### Phase N:` heading. + // Mirrors the phase-heading pattern used across roadmap-parser.cts. + const hasPhaseEntry = /^#{2,4}\s*Phase\s+\S/im.test(roadmapContent); + if (!hasPhaseEntry && !warnings.some((w) => w.code === 'V002')) { + warnings.push({ code: 'V004', message: 'ROADMAP.md contains no recognizable phase entries (no "### Phase N:" headings)' }); } // W021 only fires when phase_id_convention is explicitly 'milestone-prefixed'. @@ -173,13 +208,17 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom } } } - const warnings = (convention === 'milestone-prefixed') - ? checkW021(roadmapContent) - : []; + if (convention === 'milestone-prefixed') { + warnings.push(...checkW021(roadmapContent)); + } const result = { warnings }; - if (raw) process.stdout.write(JSON.stringify(result)); - else process.stdout.write(JSON.stringify(result, null, 2)); + process.stdout.write(raw ? JSON.stringify(result) : JSON.stringify(result, null, 2)); + // #2978: exit non-zero on any warning, per the documented contract + // ("exits non-zero on any error or warning"). + if (warnings.length > 0) { + throw new ExitError(1); + } }, 'upgrade': () => { const dryRun = !args.includes('--apply'); diff --git a/src/state-transition.cts b/src/state-transition.cts index 9db1f96f3..362180719 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -195,8 +195,25 @@ export function applyStatePreservation(input: StatePreservationInput): StatePres const derived = (postFm['progress'] ?? {}) as Record; const merged: Record = { ...derived }; if (curated) { + // #2440: total_plans and total_phases always take the derived value. + // #2969: completed_plans and completed_phases take the derived value + // when it is GREATER than the curated value (gap-closure plans that + // completed after the plan count grew) — ratcheting UP only, never + // deriving downward (preserves the #3242 curated-progress protection + // for cases unrelated to plan-count growth, e.g. a deleted SUMMARY). + // percent also takes the derived value — the resync recomputed it from + // disk counts, and a stale curated percent would be incoherent against + // the ratcheted-up completed counts (e.g. 54/54 at 93%). + const ratchetUpKeys = new Set(['completed_plans', 'completed_phases']); for (const [key, value] of Object.entries(curated)) { - if (key !== 'total_plans' && key !== 'total_phases') { + if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; + if (ratchetUpKeys.has(key)) { + const derivedNum = typeof derived[key] === 'number' ? derived[key] : -Infinity; + const curatedNum = typeof value === 'number' ? value : -Infinity; + // Take the derived value only when it ratchets up; else keep curated. + if (derivedNum > curatedNum) continue; + merged[key] = value; + } else { merged[key] = value; } } diff --git a/tests/changeset-lint.test.cjs b/tests/changeset-lint.test.cjs index dff7ab16a..6775d142d 100644 --- a/tests/changeset-lint.test.cjs +++ b/tests/changeset-lint.test.cjs @@ -8,7 +8,8 @@ const fs = require('node:fs'); const os = require('node:os'); const cp = require('node:child_process'); -const { evaluateLint, LINT_REASON } = require(path.join(__dirname, '..', 'scripts', 'changeset', 'lint.cjs')); +const { evaluateLint, LINT_REASON, DEFAULT_BASE: CHANGESET_DEFAULT_BASE } = require(path.join(__dirname, '..', 'scripts', 'changeset', 'lint.cjs')); +const { DEFAULT_BASE: DOCS_DEFAULT_BASE } = require(path.join(__dirname, '..', 'scripts', 'lint-docs-required.cjs')); const ROOT = path.join(__dirname, '..'); const LINT_SCRIPT = path.join(ROOT, 'scripts', 'changeset', 'lint.cjs'); @@ -297,3 +298,19 @@ describe('changeset lint: main() end-to-end wiring (#1006)', () => { assert.ok(!deletedEntry, `deleted fragment must not appear in failures, got: ${JSON.stringify(failures)}`); }); }); + +// ─── #2988: local base fallback parity ────────────────────────────────────── + +describe('#2988: changeset + docs lints resolve the same local base fallback', () => { + test('both lints default to `next` (the integration branch), not `main`', () => { + assert.strictEqual(CHANGESET_DEFAULT_BASE, 'next', + `changeset lint DEFAULT_BASE must be 'next', got '${CHANGESET_DEFAULT_BASE}'`); + assert.strictEqual(DOCS_DEFAULT_BASE, 'next', + `docs lint DEFAULT_BASE must be 'next', got '${DOCS_DEFAULT_BASE}'`); + }); + + test('both lints resolve the same base given the same environment (parity)', () => { + assert.strictEqual(CHANGESET_DEFAULT_BASE, DOCS_DEFAULT_BASE, + `the two lints must not diverge on base resolution: changeset='${CHANGESET_DEFAULT_BASE}' docs='${DOCS_DEFAULT_BASE}'`); + }); +}); diff --git a/tests/cli-exit.test.cjs b/tests/cli-exit.test.cjs index 03a1de939..21823bd14 100644 --- a/tests/cli-exit.test.cjs +++ b/tests/cli-exit.test.cjs @@ -183,11 +183,18 @@ describe('runMain', () => { describe('regressions', () => { /** Spawn a one-shot script that sets json-error mode and calls runMain with a throwing handler. */ function spawnJsonErrorRun({ jsonMode, errorType = 'TypeError', message = 'unexpected boom' } = {}) { + // ExitError lives in the same module as runMain; import it when the test + // wants to exercise the ExitError carve-out path. ExitError takes (code, message). + const isExitError = errorType === 'ExitError'; + const destructure = isExitError ? '{ runMain, ExitError }' : '{ runMain }'; + const throwExpr = isExitError + ? `new ExitError(1, ${JSON.stringify(message)})` + : `new ${errorType}(${JSON.stringify(message)})`; const script = ` const io = require(${JSON.stringify(IO_PATH)}); - const { runMain } = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)}); + const ${destructure} = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)}); io.setJsonErrorMode(${jsonMode ? 'true' : 'false'}); - runMain(() => { throw new ${errorType}(${JSON.stringify(message)}); }); + runMain(() => { throw ${throwExpr}; }); setImmediate(() => {}); `; return spawnSync(process.execPath, ['-e', script], { encoding: 'utf-8' }); @@ -245,5 +252,35 @@ describe('regressions', () => { `expected "unexpected boom" in stderr, got: ${stderrTrimmed.slice(0, 200)}` ); }); + + // #2979: characterization test pinning the two error paths under json-errors + // mode. The structured envelope covers non-ExitError failures; ExitError + // (usage errors) intentionally emits plain text with its own exit code. + // Both halves asserted together so the code cannot drift toward the doc's + // prior overstated claim that EVERY error emits JSON. + test('#2979: ExitError emits plain text (not JSON) even under --json-errors; non-ExitError emits the envelope', () => { + // ExitError path: plain text, own exit code, NOT a JSON object. + const exitResult = spawnJsonErrorRun({ + jsonMode: true, + errorType: 'ExitError', + message: 'Usage: gsd-tools [args]', + }); + assert.strictEqual(exitResult.status, 1, 'ExitError exits with its code'); + const exitStderr = exitResult.stderr.trim(); + let exitParsed = null; + try { exitParsed = JSON.parse(exitStderr); } catch { /* expected — plain text */ } + assert.strictEqual(exitParsed, null, + `ExitError must emit plain text, not JSON; got: ${exitStderr.slice(0, 200)}`); + assert.ok(exitStderr.includes('Usage'), + `ExitError plain-text message must reach stderr; got: ${exitStderr.slice(0, 200)}`); + + // Non-ExitError path: structured JSON envelope. + const envResult = spawnJsonErrorRun({ jsonMode: true }); + assert.strictEqual(envResult.status, 1); + const envParsed = JSON.parse(envResult.stderr.trim()); + assert.strictEqual(envParsed.ok, false); + assert.strictEqual(envParsed.reason, 'sdk_fail_fast'); + assert.ok(envParsed.message, 'envelope must carry a message'); + }); }); }); diff --git a/tests/emitted-drift-acks/2989-code-review-anchored-diff-base.json b/tests/emitted-drift-acks/2989-code-review-anchored-diff-base.json new file mode 100644 index 000000000..3452f97c2 --- /dev/null +++ b/tests/emitted-drift-acks/2989-code-review-anchored-diff-base.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "code-review.md": "#2989: the diff-base fallback's git log --grep was changed from an unanchored bare phase number (matching version strings, dates, issue refs) to an anchored '[Pp]hase N\\b' with --extended-regexp, plus a 5-line comment explaining the anchor. Makes the fail-closed branch reachable when no commit genuinely references the phase." + } +} diff --git a/tests/milestone-prefixed-convention.test.cjs b/tests/milestone-prefixed-convention.test.cjs index f54e42a1a..455b42632 100644 --- a/tests/milestone-prefixed-convention.test.cjs +++ b/tests/milestone-prefixed-convention.test.cjs @@ -89,7 +89,8 @@ describe('W021 — milestone-prefixed phase ID convention', () => { ]); const result = runGsdTools(['roadmap', 'validate'], tmpDir); - assert.ok(result.success, `roadmap validate should exit 0 even with warnings: ${result.error}`); + // #2978: validate now exits non-zero on any warning (per its documented contract). + assert.strictEqual(result.success, false, `roadmap validate must exit non-zero on W021 warnings: ${result.error}`); const out = JSON.parse(result.output); assert.ok(Array.isArray(out.warnings), 'output.warnings should be an array'); @@ -219,7 +220,8 @@ describe('W021 — milestone-prefixed phase ID convention', () => { ]); const result = runGsdTools(['roadmap', 'validate'], tmpDir); - assert.ok(result.success, `roadmap validate failed: ${result.error}`); + // #2978: validate now exits non-zero on any warning (per its documented contract). + assert.strictEqual(result.success, false, `roadmap validate must exit non-zero on W021 warnings: ${result.error}`); const out = JSON.parse(result.output); const w021 = (out.warnings || []).filter(w => w.code === 'W021'); diff --git a/tests/pi-extension-reachability.test.cjs b/tests/pi-extension-reachability.test.cjs index 8d51730cb..c2208f6b5 100644 --- a/tests/pi-extension-reachability.test.cjs +++ b/tests/pi-extension-reachability.test.cjs @@ -63,23 +63,28 @@ test('REACHABILITY: the /gsd handler dispatches a real family through gsd-tools. const dir = createTempDir(); try { const result = await pi._recorded.commands['gsd'].handler('progress json', { cwd: dir }); - assert.equal(typeof result, 'string', '/gsd handler returns a string result'); - const parsed = JSON.parse(result); + // #2991: handler returns { content: [{ type: 'text', text }] } (Pi's display shape), not a bare string. + assert.ok(result && Array.isArray(result.content) && result.content[0].type === 'text', + `/gsd handler must return Pi's display shape { content: [{ type: 'text', text }] }; got: ${JSON.stringify(result).slice(0, 200)}`); + const parsed = JSON.parse(result.content[0].text); assert.equal(typeof parsed.percent, 'number', '/gsd dispatch reached gsd-tools.cjs for real (the engine was reached)'); } finally { cleanup(dir); } }); -test('REACHABILITY: an unknown family surfaces a clear GSD error string, not a throw', async () => { +test('REACHABILITY: an unknown family surfaces a clear GSD error, not a throw', async () => { const pi = mockPi(); gsdPiExtension(pi); const dir = createTempDir(); try { const result = await pi._recorded.commands['gsd'].handler('no-such-family-8675309', { cwd: dir }); - assert.equal(typeof result, 'string'); - assert.match(result, /GSD error:/); - assert.match(result, /no-such-family-8675309|Unknown command/); + // #2991: handler returns { content: [{ type: 'text', text }] } (Pi's display shape). + assert.ok(result && Array.isArray(result.content) && result.content[0].type === 'text', + `error result must carry Pi's display shape; got: ${JSON.stringify(result).slice(0, 200)}`); + const text = result.content[0].text; + assert.match(text, /GSD error:/); + assert.match(text, /no-such-family-8675309|Unknown command/); } finally { cleanup(dir); } diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 8b9e60d6f..9b518f39b 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -3612,3 +3612,79 @@ describe('bug #1103 — annotate-dependencies preserves newline before Plans: he }); }); } + +// ───────────────────────────────────────────────────────────────────────────── +// bug #2978: roadmap validate returns {"warnings":[]} for every input +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2978: roadmap validate performs structural validation', () => { + test('empty (zero-byte) ROADMAP.md → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-empty-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), ''); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'empty file must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(Array.isArray(payload.warnings) && payload.warnings.length > 0, + `empty file must produce non-empty warnings; got: ${JSON.stringify(payload)}`); + } finally { cleanup(tmpDir); } + }); + + test('garbage/non-roadmap text → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-garbage-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'not a roadmap at all'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'garbage must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'garbage must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('missing ROADMAP.md → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-missing-'); + try { + // createTempProject creates .planning/phases but no ROADMAP.md — don't write one + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'missing file must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'missing file must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('truncated frontmatter (unterminated ---) → non-empty warnings', () => { + const tmpDir = createTempProject('gsd-2978-trunc-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '---\nmilestone: v1.0\n### Phase 1: Setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'truncated frontmatter must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'truncated frontmatter must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('well-formed roadmap → warnings: [], exit 0 (no false positive)', () => { + const tmpDir = createTempProject('gsd-2978-good-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## v1.0\n\n### Phase 1: Foundation\n**Goal:** setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.ok(result.success, `well-formed roadmap must exit 0; got: ${result.error}`); + const payload = JSON.parse(result.output); + assert.deepStrictEqual(payload.warnings, [], 'well-formed roadmap must have no warnings'); + } finally { cleanup(tmpDir); } + }); + + test('BOM-prefixed well-formed roadmap → warnings: [], exit 0 (not corruption)', () => { + const tmpDir = createTempProject('gsd-2978-bom-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '\uFEFF# Roadmap\n\n### Phase 1: Foundation\n**Goal:** setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.ok(result.success, `BOM-prefixed roadmap must exit 0; got: ${result.error}`); + const payload = JSON.parse(result.output); + assert.deepStrictEqual(payload.warnings, [], 'BOM is not corruption'); + } finally { cleanup(tmpDir); } + }); +}); diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 6b7409a2d..89b95e3b4 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -1340,6 +1340,61 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons 'total_plans equality → derived value (identity)'); }); + test('#2969: deriveProgressKeys=true — completed_plans ratchets UP when disk count exceeds curated (gap-closure plans completed)', () => { + // Gap-closure scenario: a phase had 50 plans all summarized (completed_plans: 50), + // then 4 gap-closure plans were added (total_plans -> 54) and all 4 got SUMMARYs. + // Disk scan now counts 54 summaries. The curated completed_plans (50) must + // ratchet UP to the derived value (54), not stay pinned at 50 — otherwise + // completed_plans < total_plans forever even though every plan is summarized. + const curated = { progress: { total_plans: 54, completed_plans: 50, total_phases: 2, completed_phases: 1, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 54, total_phases: 2, completed_phases: 1, percent: 100 } }, + resync: false, + deriveProgressKeys: true, + ...untouched, + }); + assert.equal(r.postFm.progress.total_plans, 54, 'total_plans takes derived value'); + assert.equal(r.postFm.progress.completed_plans, 54, + 'completed_plans must ratchet UP to derived (54 > curated 50 — gap-closure plans completed) (#2969)'); + assert.equal(r.postFm.progress.percent, 100, + 'percent must reflect the true completion fraction (54/54) (#2969)'); + }); + + test('#2969 ratchet-down protection: deriveProgressKeys=true keeps curated when disk count < curated', () => { + // The ratchet must only go UP. If the disk count is somehow LOWER than + // curated (e.g. a SUMMARY was deleted), keep the curated value — do not + // derive downward. (#3242 curated-progress protection, scoped to deriveProgressKeys.) + const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 47, percent: 87 } }, + resync: false, + deriveProgressKeys: true, + ...untouched, + }); + assert.equal(r.postFm.progress.completed_plans, 50, + 'completed_plans must NOT derive downward (47 < curated 50) — ratchet-up only (#2969/#3242)'); + }); + + test('#2969 body-only write protection: deriveProgressKeys absent keeps wholesale restore', () => { + // state.update/patch (no deriveProgressKeys flag) must keep the full #3242 + // wholesale curated restore — completed_plans never moves for a body-only edit. + const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 54, percent: 100 } }, + resync: false, + // deriveProgressKeys NOT set — body-only write path + ...untouched, + }); + assert.equal(r.postFm.progress.completed_plans, 50, + 'body-only write must keep curated completed_plans (no deriveProgressKeys) (#2969/#3242)'); + }); + test('progress: NOT restored when transition re-derives from disk (resync=true) — sync/advancePlan/completePhase path', () => { const recomputed = { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }; const r = applyStatePreservation({