diff --git a/.changeset/gentle-jays-wave.md b/.changeset/gentle-jays-wave.md new file mode 100644 index 000000000..33cd90539 --- /dev/null +++ b/.changeset/gentle-jays-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4581 +--- +**`state update` no longer reports a same-value write as a missing field** — updating `Last Activity` (or any other body-sourced frontmatter key) to the value it already holds reported `updated: false` with a "not found in STATE.md" message telling the caller to add a line that was already there at the correct value. Any day `gsd-ship` runs before `gsd-extract-learnings`, both write today's date to the same field, so the second call always hit this. (#4488) diff --git a/.gitignore b/.gitignore index 96d003adb..7b768feea 100644 --- a/.gitignore +++ b/.gitignore @@ -314,6 +314,7 @@ build/ /gsd-core/bin/lib/git-base-branch.cjs /gsd-core/bin/lib/host-runtime-detection.cjs /gsd-core/bin/lib/task-content-resolution.cjs +/gsd-core/bin/lib/tdd-red-evidence.cjs __pycache__/ *.pyc .venv/ diff --git a/gsd-core/bin/lib/tdd-red-evidence.cjs b/gsd-core/bin/lib/tdd-red-evidence.cjs deleted file mode 100644 index b3644010e..000000000 --- a/gsd-core/bin/lib/tdd-red-evidence.cjs +++ /dev/null @@ -1,133 +0,0 @@ -"use strict"; -/** - * TDD RED-evidence classification (#3770). - * - * The `type: tdd` executor gate previously accepted ANY nonzero test command as - * RED: syntax errors, zero-test discovery, fixture crashes, parser errors, and - * unrelated assertions all authorized production edits (GREEN). This module - * defines the compact RED evidence the gate now requires — the TARGET test's - * identity plus a matching assertion failure — and classifies a persisted test - * run into exactly one verdict: - * - * RED_EVIDENCE_OK — nonzero exit AND the target test failed as a REAL test - * (distinctly named, TAP-reported failure). The ONLY - * verdict that may advance to GREEN. - * INVALID_RED — everything else, with a machine-readable reason: - * unexpected_green | zero_tests_discovered | - * nonzero_exit_without_test_failure | - * fixture_or_load_failure | no_target_test_failure | - * invalid_record | unreadable_record (router arm). - * - * Everything here is PURE — no fs, no spawn, no clock — so the executor can - * persist the record (command, exit code, failing test, expected, actual) and - * validate it via `gsd_run check tdd-red-evidence `. - * - * TAP parsing reuses the proven primitives from prohibition-enforcement - * (`parseNodeTestSummary`, `tapFailedTestNames`) — the same contract the - * prohibition probe's fail-first prover already relies on (#1259). - */ -Object.defineProperty(exports, "__esModule", { value: true }); -exports.classifyRedEvidence = classifyRedEvidence; -exports.buildRedEvidenceRecord = buildRedEvidenceRecord; -const prohibition_enforcement_cjs_1 = require("./prohibition-enforcement.cjs"); -/** Basename of a path-like string ('' for non-strings) — separators `/` and `\`. */ -function baseOf(p) { - return typeof p === 'string' ? (p.split(/[\\/]/).pop() ?? p) : ''; -} -/** Coerce and validate the raw record's scalar fields. Returns null exit_code only when absent/non-numeric. */ -function readInput(input) { - const command = typeof input?.command === 'string' ? input.command : ''; - const output = typeof input?.output === 'string' ? input.output : ''; - const targetTest = typeof input?.targetTest === 'string' ? input.targetTest.trim() : ''; - const exitCode = typeof input?.exitCode === 'number' && Number.isFinite(input.exitCode) ? input.exitCode : null; - if (!command || !targetTest || exitCode === null) - return null; - return { command, exitCode, output, targetTest }; -} -/** - * Classify a persisted RED-phase test run. Fail-closed: malformed input, an - * unparseable/empty TAP summary, a file-named (load/crash) failure, or a - * failure that is not the target test's are all INVALID_RED — only a nonzero - * exit WITH the distinctly-named target test failing is RED_EVIDENCE_OK. - * Never throws. - */ -function classifyRedEvidence(input) { - const parsed = readInput(input); - if (!parsed) { - return { - verdict: 'INVALID_RED', - reason: 'invalid_record', - evidence: { - command: typeof input?.command === 'string' ? input.command : '', - exit_code: null, - target_test: '', - tests: 0, - pass: 0, - fail: 0, - failing_tests: [], - }, - }; - } - const { command, exitCode, output, targetTest } = parsed; - const summary = (0, prohibition_enforcement_cjs_1.parseNodeTestSummary)(output); - const failing = (0, prohibition_enforcement_cjs_1.tapFailedTestNames)(output); - const evidence = { - command, - exit_code: exitCode, - target_test: targetTest, - tests: summary.tests, - pass: summary.pass, - fail: summary.fail, - failing_tests: failing, - }; - // Existing fail-fast rule, now machine-checked: exit 0 during RED is an - // unexpected GREEN — the feature may already exist or the test is wrong. - if (exitCode === 0) { - return { verdict: 'INVALID_RED', reason: 'unexpected_green', evidence }; - } - // Zero-test discovery: the discovery pattern / fixture matched no tests. - // A run that executed nothing cannot prove anything about the behavior. - if (summary.tests === 0) { - return { verdict: 'INVALID_RED', reason: 'zero_tests_discovered', evidence }; - } - // Nonzero exit but TAP reports no failing test: harness/setup/parser crash - // whose failure never reached a test assertion (or unparseable output). - if (summary.fail === 0 || failing.length === 0) { - return { verdict: 'INVALID_RED', reason: 'nonzero_exit_without_test_failure', evidence }; - } - // Fixture/load failure: every failing entry is named like the target FILE — - // node reports a load-time crash (throw-on-require, syntax error, ENOENT - // fixture) as a file-named `not ok 1 - `, never the target test. - const targetBase = baseOf(input?.targetFile ?? ''); - const distinctlyNamed = failing.filter((n) => (targetBase ? baseOf(n) !== targetBase : true)); - if (distinctlyNamed.length === 0) { - return { verdict: 'INVALID_RED', reason: 'fixture_or_load_failure', evidence }; - } - // Unrelated failure: real tests ran and failed, but none is the target test - // the plan named — an unrelated assertion must not authorize GREEN. - if (!distinctlyNamed.includes(targetTest)) { - return { verdict: 'INVALID_RED', reason: 'no_target_test_failure', evidence }; - } - return { verdict: 'RED_EVIDENCE_OK', reason: 'target_test_failed', evidence }; -} -/** - * Project a classification into the persisted record shape — command, exit - * code, failing test, expected, actual, verdict, reason — so the evidence - * survives past the terminal and the gate can re-verify it deterministically. - * Pure: JSON-serializable, no timestamps (the record's mtime/commit carries time). - */ -function buildRedEvidenceRecord(input, result) { - const failingTest = result.evidence.failing_tests.find((n) => n === result.evidence.target_test) ?? - result.evidence.failing_tests[0] ?? - null; - return { - command: result.evidence.command, - exit_code: result.evidence.exit_code, - failing_test: failingTest, - target_test: result.evidence.target_test, - expected: typeof input?.expected === 'string' ? input.expected : null, - actual: typeof input?.actual === 'string' ? input.actual : null, - verdict: result.verdict, - reason: result.reason, - }; -} diff --git a/src/state.cts b/src/state.cts index 906b469fe..521cd1461 100644 --- a/src/state.cts +++ b/src/state.cts @@ -808,7 +808,31 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | // (#3345's direction) — reported separately from `updated` because this // command's contract is a single-field boolean, not a per-field array. const reconciled = reconcileReportedFields(statePath, preWriteState, updated ? [field as string] : [], divergedFields); - updated = reconciled.includes(field as string); + // #4488: `updateCore` itself already told us whether it matched the field + // (`updated`, captured above `readModifyWriteStateMd` runs it) — that is a + // real signal, not a guess. `reconcileReportedFields` answers a DIFFERENT + // question ("what changed on disk") and, per its own docstring, reports + // `[]` whenever `preWriteState.fm` is `undefined`. That happens in two + // known cases, both of which mean "no snapshot was ever captured", not + // "nothing happened": (a) `readModifyWriteStateMd`'s #948 no-op guard + // fires because the transform's output was byte-identical to the input — + // the requested value already equals what's on disk, so the field WAS + // found and there was simply nothing left to change; (b) + // `applyPostSyncPreservation`'s `isUnparseableFrontmatter` early return — + // the ORIGINAL frontmatter block was malformed, so preservation never + // runs, yet `readModifyWriteStateMd` still persists the transform's raw + // output via `platformWriteSync`. In neither case did preservation + // discard or rewrite what the transform wrote, so trusting the + // transform's own `updated` signal here is never a false positive. + // Collapsing either case into the same `false` as "field not found" is + // the #4488 bug — `explainUpdateFailure` then reports a message that is + // actively false (it tells the caller to add a line that is already + // there). Every other `false` origin (case-D fallback did not apply, or + // the transform genuinely found nothing) is unaffected: there + // `preWriteState.fm` is defined (a normal sync ran) or `updated` was + // already false before this line. + const noopBecauseAlreadyCorrect = updated && preWriteState.fm === undefined; + updated = reconciled.includes(field as string) || noopBecauseAlreadyCorrect; const preserved = reconciled.filter((f) => f !== field); if (updated) { diff --git a/tests/lint-compiled-artifact-sync.test.cjs b/tests/lint-compiled-artifact-sync.test.cjs index 0cee4336d..2a0c8d115 100644 --- a/tests/lint-compiled-artifact-sync.test.cjs +++ b/tests/lint-compiled-artifact-sync.test.cjs @@ -186,17 +186,50 @@ describe('fix-2657: compiled .cjs artifacts are gitignored, not tracked (ADR-457 // #4093 CI: this spawn is NOT the PROBE class the shared default below // describes. The script's "nothing left to check" path still runs a FULL // `tsc -p tsconfig.build.json` compile to a throwaway outDir whenever any - // compiled artifact remains tracked (ten are, deliberately — ADR-457's - // staged end state), and under CI shard load that compile can exceed the - // 15s probe budget, dying to a SIGTERM with empty piped stdout (observed - // twice on ubuntu shard 1/3). Per helpers/timeouts.cjs's own rule, a call - // site that genuinely differs from its class — "a real `tsc` compile" — - // keeps its own local constant with its own justifying comment; see - // tests/ensure-runtime-build.test.cjs's BUILD_TIMEOUT_MS for the other - // instance. 60s is that same class, sized for the cold-cache CI case. + // compiled artifact remains tracked, and under CI shard load that compile + // can exceed even a generous budget, dying to a SIGTERM with empty piped + // stdout (observed on ubuntu shard 1/3 both pre- and post-#4488). Per + // helpers/timeouts.cjs's own rule, a call site that genuinely differs from + // its class — "a real `tsc` compile" — keeps its own local constant with + // its own justifying comment; see tests/ensure-runtime-build.test.cjs's + // BUILD_TIMEOUT_MS for the other instance. 60s is that same class, sized + // for the cold-cache CI case. + // + // #4488: as of this fix the tracked set is fully empty (see the + // "fully-empty ADR-457 end state" test above) — trackedCompiledArtifacts() + // returning [] means main() short-circuits at its own line ~98 BEFORE ever + // invoking `compileToTemp()`, so this spawn no longer runs a real tsc + // compile at all. The 60s budget above stays as a guard for whenever a + // future module reintroduces a tracked artifact (re-widening the migration + // gap this whole file exists to catch), not because this run needs it now. const TSC_COMPILE_TIMEOUT_MS = 60000; const args = [path.join(REPO_ROOT, 'scripts', 'lint-compiled-artifact-sync.cjs')]; const result = run(process.execPath, args, { timeoutMs: TSC_COMPILE_TIMEOUT_MS }); assert.equal(result.status, 0, describeFailure(process.execPath, args, result)); }); + + test('trackedCompiledArtifacts() reports the fully-empty ADR-457 end state (#4488)', () => { + // #4488 CI: tdd-red-evidence.cjs (introduced by #3770/PR #4279, AFTER the + // original "nine" this file's other tests pin) was never gitignored — + // a tenth, later instance of the exact #2657/#2653 migration-gap defect + // class, incidentally surfaced by a #4488 CI run timing out on the real + // tsc compile this test's sibling above must run while ANY artifact stays + // tracked. Generic (not tied to the closed NINE_ARTIFACTS list above) so + // it also catches any future module that lands compiled-but-tracked. + let pairs; + try { + pairs = trackedCompiledArtifacts(); + } catch (err) { + assert.fail( + `trackedCompiledArtifacts() threw instead of returning a result:\n` + + `${err && err.stack ? err.stack : err}`, + ); + } + assert.deepEqual( + pairs, + [], + `expected zero tracked compiled artifacts (ADR-457 end state); still tracked: ` + + `${pairs.map((p) => p.artifact).join(', ') || '(none)'}`, + ); + }); }); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 1bd07d8f8..9a9ff3790 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -13797,6 +13797,155 @@ describe('#905: syncStateFrontmatter preserves scalars when body annotations are }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Bug #4488: state update reports a same-value write as "field not found" +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #4488: state update reports updated:true, not a false "not found", when the new value equals the current value', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('same-value update on a template-shaped STATE.md reports updated:true, not "not found"', () => { + // Exact shape from the issue: frontmatter last_activity AND body + // "Last activity:" line already hold the value being "written". The + // transform's own stateReplaceField match succeeds and produces + // byte-identical output, which trips readModifyWriteStateMd's #948 + // no-op guard before reconcileReportedFields' preWriteState snapshot is + // ever populated -- reconciliation then (correctly, for the general + // case) reports "[]", and cmdStateUpdate must not read that as "the + // field could not be found". + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, [ + '---', + 'gsd_state_version: "1.0"', + 'current_phase: 1', + 'current_phase_name: Test Phase', + 'status: planning', + 'last_activity: 2026-09-07', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 1 of 1 (Test Phase)', + 'Status: In progress', + 'Last activity: 2026-09-07', + '', + ].join('\n'), 'utf-8'); + + const result = runGsdTools('state update "Last Activity" "2026-09-07"', tmpDir); + assert.ok(result.success, `state update failed: ${result.error}`); + + const json = JSON.parse(result.output); + assert.strictEqual( + json.updated, + true, + `same-value update must report updated:true, not a false negative (got: ${JSON.stringify(json)})`, + ); + assert.ok( + !('reason' in json), + `same-value update must not carry a "field not found" reason (got: ${JSON.stringify(json)})`, + ); + + // The file itself is untouched byte-for-byte (there was nothing to change). + const after = fs.readFileSync(statePath, 'utf-8'); + assert.match(after, /^last_activity: 2026-09-07$/m); + assert.match(after, /^Last activity: 2026-09-07$/m); + }); + + test('changed-value update (control) still reports updated:true and actually rewrites the line', () => { + // Same fixture, different target date -- the pre-existing, always-worked + // path. Pins that the #4488 fix does not turn INTO a false positive for + // a real change. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, [ + '---', + 'gsd_state_version: "1.0"', + 'last_activity: 2026-09-07', + '---', + '', + '## Current Position', + '', + 'Last activity: 2026-09-07', + '', + ].join('\n'), 'utf-8'); + + const result = runGsdTools('state update "Last Activity" "2026-09-08"', tmpDir); + assert.ok(result.success, `state update failed: ${result.error}`); + const json = JSON.parse(result.output); + assert.strictEqual(json.updated, true); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.match(after, /^Last activity: 2026-09-08$/m); + }); + + test('a genuinely absent field still reports updated:false with the case-D diagnostic', () => { + // Control for the other direction: this must NOT become a blanket + // "always true" -- a field with no body source line and no frontmatter + // key at all is still a real failure. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, [ + '---', + 'gsd_state_version: "1.0"', + '---', + '', + '## Current Position', + '', + 'Status: In progress', + '', + ].join('\n'), 'utf-8'); + + const result = runGsdTools('state update "Last Activity" "2026-09-08"', tmpDir); + assert.ok(result.success, `state update failed: ${result.error}`); + const json = JSON.parse(result.output); + assert.strictEqual(json.updated, false); + assert.match(json.reason, /not found in STATE\.md/); + }); + + test('#3699 case-D repair with a same-value frontmatter target still reports updated:true', () => { + // Case D fires when the CALLER addresses the FRONTMATTER key directly + // (e.g. "last_activity", per explainUpdateFailure's own "update + // \"last_activity\" directly to repair" instruction) on a document whose + // body has no source line at all. That repair write can ALSO be + // byte-identical to the original (the frontmatter already holds the + // target value) and trip the same #948 no-op guard as the body-label + // path above -- a second origin for the #4488 collapse, covered here so + // it doesn't regress silently. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, [ + '---', + 'gsd_state_version: "1.0"', + 'current_phase: 1', + 'current_phase_name: Test Phase', + 'status: planning', + 'last_activity: 2026-09-07', + '---', + '', + '## Current Position', + '', + 'Status: In progress', + '', + ].join('\n'), 'utf-8'); + + const result = runGsdTools('state update last_activity 2026-09-07', tmpDir); + assert.ok(result.success, `state update failed: ${result.error}`); + const json = JSON.parse(result.output); + assert.strictEqual( + json.updated, + true, + `same-value case-D repair must report updated:true (got: ${JSON.stringify(json)})`, + ); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // Bug #1230 regression suite // ─────────────────────────────────────────────────────────────────────────────