* fix(#4488): report state update as successful when the value is already correct `cmdStateUpdate` unconditionally overwrote `updateCore`'s own `updated:true` signal with `reconcileReportedFields`'s disk-diff result. That diff reports `[]` -- by design -- whenever `readModifyWriteStateMd`'s #948 no-op guard fires because the transform's output was byte-identical to the input, which happens precisely when the requested value already equals what's on disk. The field genuinely was found and matched; there was simply nothing left to change. Collapsing that into the same `false`/"not found" response as a genuine miss produced an actively wrong diagnostic message and a silent same-day no-op in gsd-ship + gsd-extract-learnings, which both write `Last Activity` to today's date. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4488): backfill changeset pr number to 4581 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4488): untrack tdd-red-evidence.cjs, completing its ADR-457 gitignore migration Bundled discovery from this PR's own CI run: tests/lint-compiled-artifact- sync.test.cjs's full tsc compile (which runs whenever ANY compiled artifact remains tracked) SIGTERM'd under shard contention. gsd-core/bin/lib/tdd-red- evidence.cjs (introduced by #3770/PR #4279) was the sole remaining tracked artifact -- a tenth, later, separate instance of the #2657/#2653 migration-gap defect class this test file's closed nine-item list doesn't cover. Untracked it and added the .gitignore entry, same fix shape as the original nine. This eliminates the slow tsc-compile path entirely (verified: 0.1s vs ~7s locally) rather than papering over a timeout. Added a generic regression test asserting the tracked set is fully empty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/gentle-jays-wave.md
Normal file
5
.changeset/gentle-jays-wave.md
Normal file
@@ -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)
|
||||
1
.gitignore
vendored
1
.gitignore
vendored
@@ -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/
|
||||
|
||||
@@ -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 <record.json>`.
|
||||
*
|
||||
* 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 - <file>`, 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,
|
||||
};
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
@@ -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)'}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user