From 394bf384be763b84daa438b56d6a29fde138fcec Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 24 Aug 2026 21:33:48 -0400 Subject: [PATCH] fix(#3696): report the last_activity invariant and make the verdict gateable with --strict (#3844) * test(#3696): failing-first coverage for the last_activity invariant and --strict exit status * fix(#3696): report the last_activity invariant and make the verdict gateable with --strict * fix(#3696): agree with the real reader on last_activity, and stop reporting structure as truncation * chore(#3696): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/brave-tunas-dart.md | 5 + CONTEXT.md | 2 +- docs/COMMANDS.md | 20 + docs/FEATURES.md | 4 +- .../interpret-state-validate-results.md | 19 + scripts/lint-health-diagnostic-rule-table.cjs | 71 ++- src/smart-entry.cts | 27 +- src/state-command-router.cts | 10 +- src/state-document.cts | 175 ++++++ src/state.cts | 106 +++- ...lint-health-diagnostic-rule-table.test.cjs | 61 ++- tests/state.test.cjs | 500 ++++++++++++++++++ 12 files changed, 954 insertions(+), 46 deletions(-) create mode 100644 .changeset/brave-tunas-dart.md diff --git a/.changeset/brave-tunas-dart.md b/.changeset/brave-tunas-dart.md new file mode 100644 index 000000000..26ef7dcd2 --- /dev/null +++ b/.changeset/brave-tunas-dart.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3844 +--- +**`state validate` now sees the `last_activity` invariant, and `--strict` makes the verdict gateable** — a STATE.md whose `Last activity` value no reader could parse used to validate clean (`{valid:true, warnings:[], scope:'complete'}`), and a wrapped description was silently truncated; both are now reported as coded diagnostics (`S008`/`S009`). `state validate --strict` exits non-zero when the report is not valid, so a CI step or git hook can gate on state correctness without parsing JSON — the default exit status is unchanged. (#3696) diff --git a/CONTEXT.md b/CONTEXT.md index 9fe6336b2..b6a94e266 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -62,7 +62,7 @@ Adapter Module that satisfies native query dispatch at the Dispatch Policy seam, Module owning projection from dispatch results/errors to CLI `{ exitCode, stdoutChunks, stderrLines }` output contract. ### STATE.md Document Module -Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope for the read path; `state.cts`'s `matchCurrentPositionSection` is a thin alias over it, and the `drift-guard phase-status` seam consumes it, so the #2956 archive-shadowing fix cannot be re-derived into a second copy — the byte-exact mutation path served by `state-transition.cts`'s `locateCurrentPosition`/`sliceCurrentPositionSection` is a deliberately separate, un-consolidated locator, #3187). `stateFieldValue` (#3187) is the single owner of the #1760 frontmatter-then-body field fallback chain, consolidating the 14 re-derivations of that ladder onto one scope-carrying (`complete`/`truncated`/`unscoped`/`unreadable`) primitive. It does not scan `.planning/phases` and does not own persistence or locking; phase/plan/summary counts arrive from inventory/progress Modules as inputs, and read-modify-write paths remain Adapters. Source of truth: `gsd-core/bin/lib/state-document.cjs`. **Commit provenance (#2573):** `state_head` records the full sha STATE.md was written against, stamped by `syncStateFrontmatter` and omitted entirely outside a git repo. `readStateHeadFreshness(cwd, stateHead)` (`src/state.cts`) is the single derivation consumed by both `validate.health` (W024) and smart-entry — it returns `{ state_head, current_commit, commits_behind, commit_stale }` with **tri-state** `commit_stale`: `null` = unknown (no stamp, no git, or a stamp that is not an ancestor of HEAD after a history rewrite), `false` = known fresh, `true` = the codebase has moved. Mirrors the graphify commit-staleness contract deliberately. It is a freshness PROXY, never a drift measurement: `rev-list` counts unrelated commits and the stamp restamps on every state write, so a low count means STATE.md was written recently, not that its contents are accurate — it must never gate. +Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope for the read path; `state.cts`'s `matchCurrentPositionSection` is a thin alias over it, and the `drift-guard phase-status` seam consumes it, so the #2956 archive-shadowing fix cannot be re-derived into a second copy — the byte-exact mutation path served by `state-transition.cts`'s `locateCurrentPosition`/`sliceCurrentPositionSection` is a deliberately separate, un-consolidated locator, #3187). `stateFieldValue` (#3187) is the single owner of the #1760 frontmatter-then-body field fallback chain, consolidating the 14 re-derivations of that ladder onto one scope-carrying (`complete`/`truncated`/`unscoped`/`unreadable`) primitive. `isRealCalendarDate` (#3696) is the single owner of "does this y/m/d exist on the calendar" — moved here from a private copy in `smart-entry.cts` so that reader and `state validate`'s S008 cannot drift into disagreeing about whether a STATE.md is usable; it enforces ADR-227's shape-AND-value rule, rejecting `2026-02-30` rather than letting `Date.parse` roll it forward to `2026-03-02`. `stateFieldContinuation` (#3696) reports the prose a wrapped single-line field leaves behind, which `stateExtractField`'s newline-excluding `(.+)` silently drops (S009); it is additive beside `stateExtractField` rather than a widening of it, because ADR-3180 §7.7 Rejected #1 pins that function as untouchable (20 direct callers, CRITICAL blast radius) and a continuation join there would apply to every field — `Status:` would swallow the line beneath it. It does not scan `.planning/phases` and does not own persistence or locking; phase/plan/summary counts arrive from inventory/progress Modules as inputs, and read-modify-write paths remain Adapters. Source of truth: `gsd-core/bin/lib/state-document.cjs`. **Commit provenance (#2573):** `state_head` records the full sha STATE.md was written against, stamped by `syncStateFrontmatter` and omitted entirely outside a git repo. `readStateHeadFreshness(cwd, stateHead)` (`src/state.cts`) is the single derivation consumed by both `validate.health` (W024) and smart-entry — it returns `{ state_head, current_commit, commits_behind, commit_stale }` with **tri-state** `commit_stale`: `null` = unknown (no stamp, no git, or a stamp that is not an ancestor of HEAD after a history rewrite), `false` = known fresh, `true` = the codebase has moved. Mirrors the graphify commit-staleness contract deliberately. It is a freshness PROXY, never a drift measurement: `rev-list` counts unrelated commits and the stamp restamps on every state write, so a low count means STATE.md was written recently, not that its contents are accurate — it must never gate. ### STATE.md Transition Module Module owning STATE.md lifecycle/maintenance transitions as intent-based methods (`beginPhase`, `advancePlan`, `completePhase`, `plannedPhase`, `milestoneSwitch`, `milestoneComplete`, `patch`, `sync`, `prune`, `update`, `rebuild`). Pure core `(content, intent, deps) → newContent` with injected I/O (file read/write, lock, disk scan); consults a field-classification table that names each STATE.md field's class (`derived-from-body` | `derived-from-disk` | `derived-from-external` | `curated` | `free`) and its preservation policy. Supersedes the 14 scattered RMW callbacks in `state.cts` (phase.cts's former direct caller has since been migrated away). `syncAndPreserveStateMd` (`state.cts`, #3469) is the single composition of `syncStateFrontmatter` + `applyPostSyncPreservation`; `readModifyWriteStateMd` and `cmdPhaseComplete` both **call** it rather than assembling the two steps, because assembling them at a call site is a re-derivation even when every step calls an owner. **Exactly two direct `writeStateMd` callers remain, and both are SANCTIONED PERMANENT exceptions under ADR-3408 §8.3 (as amended): `cmdStateSync` and the `REGENERATE_STATE` remedy (`health-diagnostic.cts`).** Neither is debt — `state sync` exists to re-derive frontmatter *from* the body (#905), and `REGENERATE_STATE` is a factory reset; preservation on either would re-lock precisely what the command was invoked to replace. `cmdMilestoneComplete` was the third and is now routed through the composition (#3469). Phase 1's whole-repo drift guard, not this line, is the authoritative count. *(Location correction: this entry previously placed the factory-reset primitive at `verify.cts:1925`. It moved to `health-diagnostic.cts` when `cmdValidateHealth` migrated onto the rule table (#3309); `verify.cts` now contains no `writeStateMd` call. The design intent was unchanged — only the address was stale.)* Absorbs `syncStateFrontmatter` + `readModifyWriteStateMd`'s post-sync preservation block; Encoding 3 (`cmdStateBuildFrontmatter`) stays separate — read path concern. Sibling/super-module of the STATE.md Document Module; consumes its `stateReplaceField`/`stateExtractField` primitives. Body section structure (`## Current Position`, `## Session`, etc.) lives as a constants block inside the Module. Append-only transitions (`addDecision`, `addBlocker`, etc.) stay on today's RMW seam for now. Targets the #1760/#1761/#1743/#1695/#1264/#1255/#1257/#3242 bug cluster. Migration per ADR-1372 §T6 sequenced as substrate + `beginPhase` first (PR1), then transition-by-transition with characterization tests first per transition. **ADR-1817 adds `rebuild` as the capstone 11th transition — the body-structure derivability contract.** Re-derives `## Current Position` prose from frontmatter and `## By-Phase Progress` table from phase dirs on disk; preserves `## Session` / `## Decisions` / unknown sections verbatim; de-duplicates `## Session Continuity Archive` (keep most-recent N, default 3); appends a structured audit entry to `## Rebuild Log` (`timestamp`, `kind`, `section`, `before`, `after`, `reason`) for every mutation. Hard idempotency guarantee: a no-mutation rebuild appends no log entry, so two successive invocations on a clean file are byte-identical. Non-overlapping with `sync` (3 lightweight frontmatter fields, auto-triggered) and orthogonal to `auto_prune_state` (age-based removal) — `rebuild` reconciles with current canonical sources, `prune` removes by retention policy, the two compose (rebuild first, then prune). Section ordering is invariant: rebuild rewrites content in place, never reorders. Targets the #1776/#1761/#1591 body-drift cluster that survived ADR-1769's per-field transitions. Phased per ADR-1817: Phase 0 = this ADR + predicates (closes #1817), Phase 1 = `rebuildCore` body + `rebuild` dispatch case + drift-class unit tests (#1827), Phase 2 = `cmdStateRebuild` CLI + `--dry-run`/`--verbose` + integration tests + docs + changeset (#1826). Source of truth: `gsd-core/bin/lib/state-transition.cjs` (generated from `src/state-transition.cts`). `state_head` (#2573) is classified `{ source: 'free', preservation: 'derive' }` — an ambient git read recomputed on every write, like `last_updated`; never preserved, because a stale stamp would claim STATE.md was written against a commit it wasn't. **Phase 4 (#3471):** `syncStateFrontmatter`'s six empty-only guards (D1) are now GATED — active only for the §8.3 sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`), which have no preservation executor downstream and for which body-beats-frontmatter is the deliberate contract; OFF on the write seam, where `applyStatePreservation` owns the empty case via the exported `applyPreserveWhenUnchanged` executor. `reconcileReportedFields` (`state.cts`, §8.4/D4) is the single owner of reconciling a command's reported `updated`/`failed` field array against what was actually persisted, closing both #3351's (reported-but-discarded) and #3345's (persisted-but-unreported) directions; used by seven commands rather than seven re-derivations. `cmdStateJson` (§8.5/D3) no longer carries a private copy of the empty-only guards; it routes through `applyPreserveWhenUnchanged` directly, scoped to the same six fields. This entry has now needed correcting three times inside this one epic; Phase 1's whole-repo drift guard, not this line, remains the authoritative count. diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 996cced8e..607d16b2c 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -2023,6 +2023,24 @@ Detect drift between STATE.md and the actual filesystem. node gsd-tools.cjs state validate ``` +| Flag | Description | +|------|-------------| +| `--strict` | Exit non-zero when the report is not `valid: true`. Off by default. | + +Without `--strict` the command always exits `0`, including when it reports +`valid: false` — so a CI step or git hook has to parse the JSON to decide whether +state is correct. `--strict` makes the verdict gateable directly: + +```bash +node gsd-tools.cjs state validate --strict || echo "STATE.md needs attention" +``` + +The default is deliberately unchanged: the exit status is observable behavior that +reaches downstream consumers who cannot be enumerated, so opting in is a choice the +caller makes rather than one imposed on every existing script. + +A missing or unreadable STATE.md exits non-zero under `--strict` too — those report +`error` or `valid: false` and are as gateable as any drift warning. The report also carries a `scope` field reporting whether the drift derivation could actually run: | `scope` | Meaning | @@ -2045,6 +2063,8 @@ Each `warnings` entry is a coded diagnostic object (`{code, severity, message, r | `S005` | warning | STATE.md's plan count disagrees with the plan count on disk | | `S006` | warning | STATE.md still says "executing" but a `*-VERIFICATION.md` in the phase shows verification passed | | `S007` | warning | Every plan in the phase has a summary, but STATE.md still says "executing" | +| `S008` | warning | STATE.md's `Last activity` value does not begin with a real calendar date, so no reader can date the project's activity | +| `S009` | warning | The `Last activity` description wrapped onto a second line, and every reader silently drops the remainder | --- diff --git a/docs/FEATURES.md b/docs/FEATURES.md index ef2fbda2c..64618e576 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -1796,7 +1796,7 @@ Test suite that scans all agent, workflow, and command files for embedded inject ### 69. STATE.md Consistency Gates -**Commands:** `state validate`, `state sync [--verify]`, `state planned-phase --phase N --plans N` +**Commands:** `state validate [--strict]`, `state sync [--verify]`, `state planned-phase --phase N --plans N` **Purpose:** Detect and repair drift between STATE.md and the actual filesystem, preventing cascading errors from stale state. @@ -1805,6 +1805,8 @@ Test suite that scans all agent, workflow, and command files for embedded inject - REQ-STATE-02: `state sync` MUST reconstruct STATE.md from actual project state on disk - REQ-STATE-03: `state sync --verify` MUST perform a dry-run showing proposed changes without writing - REQ-STATE-04: `state planned-phase` MUST record the state transition after plan-phase completes (Planned/Ready to execute) +- REQ-STATE-05: `state validate` MUST report a `Last activity` value that no reader can parse, rather than validating clean +- REQ-STATE-06: `state validate --strict` MUST reflect `valid` in the process exit status, leaving the default exit status unchanged **Produces:** | Artifact | Description | diff --git a/docs/how-to/interpret-state-validate-results.md b/docs/how-to/interpret-state-validate-results.md index a7bbeda88..109c8d62f 100644 --- a/docs/how-to/interpret-state-validate-results.md +++ b/docs/how-to/interpret-state-validate-results.md @@ -49,6 +49,25 @@ A freshly-initialized project is the clearest example of a **legitimate** non-`c --- +## Gate on the result from a script + +By default `state validate` exits `0` whatever it finds, so a shell gate needs the +JSON. Pass `--strict` and the exit status carries the verdict instead: + +```bash +node gsd-tools.cjs state validate --strict +``` + +Exit `0` means `valid: true`; any other exit means the report was not clean (drift +warnings, an unreadable STATE.md, or no STATE.md at all). + +`--strict` reads `valid`, **not** `scope` — so it stays silent about a scan that could +not run. A degraded scope still reports `valid: true` and still exits `0`. Read `scope` +yourself, exactly as the table above says, before treating a green `--strict` run as a +guarantee the check actually looked. + +--- + ## Related - [`state validate`](../COMMANDS.md#state-validate) — command reference, flags, and the full output shape diff --git a/scripts/lint-health-diagnostic-rule-table.cjs b/scripts/lint-health-diagnostic-rule-table.cjs index dedb50c9e..fed93cbb3 100644 --- a/scripts/lint-health-diagnostic-rule-table.cjs +++ b/scripts/lint-health-diagnostic-rule-table.cjs @@ -82,14 +82,70 @@ const CONSISTENCY_TEST_FILE = path.join(REPO_ROOT, 'tests', 'health-diagnostic-r const CONSISTENCY_CODE_PREFIX_RE = /^C\d{3}$/; // Phase 12 (#3310, ADR-3180 §8.5 extension) — the S0NN namespace's own -// fixture-proof pass. These 7 codes are NOT collected in any exported -// `Rule[]` array: `cmdStateValidate` (src/state.cts) builds `Diagnostic[]` -// directly via a local `stateDiagnostic()` helper, not via the rule-table -// evaluator (out of scope for the RULES/CONSISTENCY_RULES-keyed passes -// above). The list is therefore hardcoded here instead of read from the -// compiled module. +// fixture-proof pass. These codes are NOT collected in any exported `Rule[]` +// array: `cmdStateValidate` (src/state.cts) builds `Diagnostic[]` directly via +// a local `stateDiagnostic()` helper, not via the rule-table evaluator (out of +// scope for the RULES/CONSISTENCY_RULES-keyed passes above). +// +// #3696: the list is DISCOVERED from the source, not hardcoded. It used to be a +// literal `['S001', ..., 'S007']`, which is precisely the shape ADR-3180 +// Decision 4(a) forbids — "guards discover call sites by whole-repo scan, never +// by an allowlist of known files" — because such a guard can only ever be as +// complete as the author's recall. Adding S008/S009 to `cmdStateValidate` left +// this pass reporting a confident, green "7 code(s), all fixture-covered" while +// two new codes had no fixture requirement at all: a zero it did not earn. The +// scan below cannot report a code it has not read out of the source. const STATE_VALIDATE_TEST_FILE = path.join(REPO_ROOT, 'tests', 'state.test.cjs'); -const STATE_VALIDATE_CODES = ['S001', 'S002', 'S003', 'S004', 'S005', 'S006', 'S007']; +const STATE_VALIDATE_SOURCE_FILE = path.join(REPO_ROOT, 'src', 'state.cts'); +// `g` is required by String.prototype.matchAll, which (unlike .exec) does not +// carry lastIndex across calls — so this constant is safe to share. +// #3696 review: `\s*` before `(` too. Requiring no space silently dropped a +// `stateDiagnostic ('S010', …)` call site from the discovered set, and since the +// fail-closed check below only fires on a FULLY empty result, a partial miss +// escaped the fixture-proof check entirely — the same "only as complete as the +// author's recall" failure this rewrite exists to prevent. +const STATE_DIAGNOSTIC_CALL_RE = /\bstateDiagnostic\s*\(\s*(['"`])(S\d{3})\1/g; + +/** + * Every S0NN code `cmdStateValidate` can emit, read off its + * `stateDiagnostic(...)` call sites in `sourceText`. + * + * Takes the source TEXT rather than reading the path itself, so the discovery + * rule is exercisable against controlled fixtures — including the fail-closed + * path below, which is the branch that matters and which a path-reading + * function could only be tested on by mutating the real `src/state.cts`. + * + * Fails closed. An empty result means the helper was renamed or its call shape + * changed, and a guard that answers "0 codes, all covered" to that is worse than + * no guard — so it raises instead. Line/regex-based (not full AST) per this + * repo's existing lint-guard house style, same as TITLED_BLOCK_RE below. + */ +function discoverStateValidateCodes(sourceText, sourceLabel = formatRepoRelative(STATE_VALIDATE_SOURCE_FILE)) { + const codes = [...sourceText.matchAll(STATE_DIAGNOSTIC_CALL_RE)].map((m) => m[2]); + const unique = [...new Set(codes)].sort(); + if (unique.length === 0) { + throw new ExitError( + 1, + `lint-health-diagnostic-rule-table: found no stateDiagnostic() call sites in ${sourceLabel}.\n` + + ' This pass discovers the S0NN code set from those call sites (#3696), so an empty\n' + + ' result means the helper was renamed or its call shape changed — not that there are\n' + + ' no codes. Update STATE_DIAGNOSTIC_CALL_RE to match the new shape.\n', + ); + } + return unique; +} + +function readStateValidateSource() { + if (!fs.existsSync(STATE_VALIDATE_SOURCE_FILE)) { + throw new ExitError( + 1, + `lint-health-diagnostic-rule-table: cannot discover S0NN codes — ${formatRepoRelative(STATE_VALIDATE_SOURCE_FILE)} not found.\n`, + ); + } + return fs.readFileSync(STATE_VALIDATE_SOURCE_FILE, 'utf8'); +} + +const STATE_VALIDATE_CODES = discoverStateValidateCodes(readStateValidateSource()); // Matches `describe(`/`test(`/`it(` calls whose first argument is a string // literal, capturing that literal as the block's title. Line/regex-based @@ -401,4 +457,5 @@ module.exports = { CONSISTENCY_TEST_FILE, STATE_VALIDATE_TEST_FILE, STATE_VALIDATE_CODES, + discoverStateValidateCodes, }; diff --git a/src/smart-entry.cts b/src/smart-entry.cts index 1967a1a69..022fb2964 100644 --- a/src/smart-entry.cts +++ b/src/smart-entry.cts @@ -33,6 +33,10 @@ import fs from 'node:fs'; import path from 'node:path'; import { execFileSync } from 'node:child_process'; import { collectSection } from './markdown-sectionizer.cjs'; +// #3696: the calendar-validity predicate moved to the STATE.md document module +// so `state validate` can assert the same `last_activity` invariant this reader +// already enforces (ADR-227). Two copies would let the two surfaces disagree +// about whether a STATE.md is usable — which is the defect #3696 reports. // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output } = ioMod; @@ -229,27 +233,6 @@ const ISO_LEADING_RE = */ const ZONE_DESIGNATOR_RE = /^\s*[A-Z]{2,5}(?![A-Za-z])/; -/** - * True only when y/m/d name a date that actually exists on the calendar. - * - * `Date.parse` validates shape but not value: it rolls an out-of-range day - * FORWARD rather than rejecting it (`2026-02-30` -> `2026-03-02`, - * `2026-04-31` -> `2026-05-01`). Shape-only validation would therefore - * propagate a different, wrong instant instead of failing safe — precisely - * what ADR-227 ("validate shape AND value; on failure of either layer coerce - * to the contract's safe default, never propagate") exists to prevent. A - * round-trip through Date.UTC detects the rollover: any component the - * constructor normalised comes back changed. - */ -function isRealCalendarDate(year: number, month: number, day: number): boolean { - if (month < 1 || month > 12 || day < 1 || day > 31) return false; - const probe = new Date(Date.UTC(year, month - 1, day)); - return ( - probe.getUTCFullYear() === year && - probe.getUTCMonth() === month - 1 && - probe.getUTCDate() === day - ); -} function parseActivityTimestamp(raw: string | null): number | null { if (!raw) return null; @@ -260,7 +243,7 @@ function parseActivityTimestamp(raw: string | null): number | null { // Reject an impossible calendar date outright rather than letting // Date.parse substitute a rolled-forward one. null = "no activity signal", // the safe default staleActivity already fails open on. - if (!isRealCalendarDate(Number(year), Number(month), Number(day))) return null; + if (!stateDocument.isRealCalendarDate(Number(year), Number(month), Number(day))) return null; // The date is real, so stay as liberal as before (Postel): a whole-string // parse still wins when the engine can make sense of the value. Reading the // token first would silently DROP a trailing zone name -- "2026-06-08 diff --git a/src/state-command-router.cts b/src/state-command-router.cts index 975cb79e2..d65532701 100644 --- a/src/state-command-router.cts +++ b/src/state-command-router.cts @@ -47,7 +47,7 @@ interface StateModule { cmdSignalWaiting(cwd: string, type: string | null | undefined, question: string | null | undefined, options: string | null | undefined, phase: string | null | undefined, raw: boolean): void; cmdSignalResume(cwd: string, raw: boolean): void; cmdStatePlannedPhase(cwd: string, phase: string | null | undefined, name: string | null | undefined, plans: number | null, raw: boolean): void; - cmdStateValidate(cwd: string, raw: boolean): void; + cmdStateValidate(cwd: string, raw: boolean, opts?: { strict?: boolean }): void; cmdStateSync(cwd: string, opts: { verify: string | boolean | null | undefined }, raw: boolean): void; cmdStatePrune(cwd: string, opts: { keepRecent: string; dryRun: boolean }, raw: boolean): void; cmdStateRebuild(cwd: string, opts: { dryRun: boolean; verbose: boolean }, raw: boolean): void; @@ -186,7 +186,13 @@ function routeStateCommand({ state, args, cwd, raw, error }: RouteStateCommandOp // the authoritative current_phase_name, mirroring begin-phase. state.cmdStatePlannedPhase(cwd, strArg(a, 'phase'), strArg(a, 'name'), parsePlans(strArg(a, 'plans')), raw); }, - validate: () => state.cmdStateValidate(cwd, raw), + validate: () => { + // #3696: --strict makes the verdict gateable by exit status. The + // default stays exit 0 — the exit code is Tier-2 observable output + // reaching unenumerable downstream consumers (ADR-3180 Decision 3). + const a = parseNamedArgs(args, [], ['strict']); + state.cmdStateValidate(cwd, raw, { strict: a['strict'] === true }); + }, sync: () => { const a = parseNamedArgs(args, [], ['verify']); state.cmdStateSync(cwd, { verify: a['verify'] }, raw); diff --git a/src/state-document.cts b/src/state-document.cts index 5d0c5fe0c..9352d2cc1 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -214,6 +214,181 @@ function locateFieldRow(content: string, fieldName: string): { valueStart: numbe return null; } +/** + * True only when y/m/d name a date that actually exists on the calendar. + * + * `Date.parse` validates shape but not value: it rolls an out-of-range day + * FORWARD rather than rejecting it (`2026-02-30` -> `2026-03-02`, + * `2026-04-31` -> `2026-05-01`). Shape-only validation would therefore + * propagate a different, wrong instant instead of failing safe — precisely + * what ADR-227 ("validate shape AND value; on failure of either layer coerce + * to the contract's safe default, never propagate") exists to prevent. A + * round-trip through Date.UTC detects the rollover: any component the + * constructor normalised comes back changed. + * + * #3696: this predicate previously lived privately inside `smart-entry.cts`, + * where it gated `parseActivityTimestamp`. `state validate` needed the same + * answer to assert the `last_activity` invariant (S008), and a second copy is + * the "generative fix divergence" class outright — two surfaces that disagree + * about whether a STATE.md is usable is the defect #3696 opens with, so a + * parity test over two copies would be codifying the bug rather than fixing + * it. It moves here because this module is already the designated owner of + * STATE.md field semantics (ADR-3180 §7.7) and `smart-entry.cts` imports no + * peer that would make the reverse direction a cycle. + */ +/** + * True when a field carries no value a writer ever supplied: absent, blank, or + * still holding the shipped template's bracket placeholder. + * + * `templates/state.md:35` ships `Last activity: [YYYY-MM-DD] — [What happened]`, + * so EVERY freshly-initialized project has this exact string until something + * records activity. #3696's first cut only spared the ABSENT form, which made + * S008 fire on the shipped template itself — caught by the pre-existing + * "template-equivalent phase identities remain clean without disk drift" test, + * which is precisely what it is there for. + * + * The placeholder test is anchored at the START rather than "contains a bracket + * anywhere", so a real description that happens to cite one — `2026-08-19 — fixed + * [#123] parsing` — is still a filled-in value. That keeps the rule from + * silently swallowing genuine drift. + * + * Distinct from `isStateTemplateDefault`, which answers a different question + * ("may a later handler overwrite this?") and deliberately returns true for a + * bare ISO date — a perfectly valid value here. + */ +export function isUnfilledFieldValue(value: string | null | undefined): boolean { + if (value === null || value === undefined) return true; + const trimmed = value.trim(); + return trimmed === '' || trimmed.startsWith('['); +} + +/** + * The `YYYY-MM-DD` prefix of `value`, but only when it names a date that + * actually exists. `null` for anything else — no leading date token at all, or + * a token that is shape-valid and calendar-impossible. + * + * #3696 review: this is deliberately a LEADING-TOKEN test, not the fully + * anchored prose grammar `parseProseLastActivityField` uses. That function + * requires the whole value to be `date` or `date description`, and + * returns `{date: }` when it does not match — a shape + * that reads like success. Asserting the S008 invariant through it therefore + * rejected values the real reader accepts: `smart-entry`'s + * `parseActivityTimestamp` needs only a leading date and reconstructs the + * instant even when the suffix carries no dash, so + * `Last activity: 2026-08-24 Shipped feature X` parses fine there while S008 + * called it unreadable. That is the same two-surfaces-disagree defect #3696 + * exists to close, merely pointing the other way. + * + * So the invariant asserted is the one the readers actually share: a leading + * ISO date token that is a real calendar date. + */ +export function leadingCalendarDate(value: string | null): string | null { + if (!value) return null; + const match = /^(\d{4})-(\d{2})-(\d{2})(?![\d-])/.exec(value.trim()); + if (!match) return null; + return isRealCalendarDate(Number(match[1]), Number(match[2]), Number(match[3])) + ? `${match[1]}-${match[2]}-${match[3]}` + : null; +} + +export function isRealCalendarDate(year: number, month: number, day: number): boolean { + if (month < 1 || month > 12 || day < 1 || day > 31) return false; + const probe = new Date(Date.UTC(year, month - 1, day)); + return ( + probe.getUTCFullYear() === year && + probe.getUTCMonth() === month - 1 && + probe.getUTCDate() === day + ); +} + +/** + * Markdown structure that can legitimately follow a single-line field. A line + * matching any of these is the NEXT construct, never a continuation of the + * field above it. + * + * BREADTH IS THE POINT, and the failure direction is deliberate: a missed + * truncation costs a diagnostic nobody sees, while a false S009 reports drift on + * a well-formed STATE.md — a gate that fires on valid documents is worse than no + * gate. When a shape is ambiguous, it belongs here. + * + * #3696 review round 2 added the last three arms after all three were shown to + * produce false S009 fires on well-formed content: an indented code block, an + * HTML block, and a setext underline (`===`, which the `[-*_]{3,}` rule does not + * cover — it only knows `-`, `*` and `_`). + */ +const MD_STRUCTURE_LINE_RE = + /^(?:#{1,6}\s|\||>|```|~~~|[-*_]{3,}\s*$|=+\s*$|[-*+]\s|\d+[.)]\s|\[[^\]]+\]:|<|(?: {4}|\t))/; + +/** + * A setext heading's underline — `===` or `---` on its own line. The line ABOVE + * one of these is a heading TITLE, which is indistinguishable from prose on its + * own, so the scan must look ahead by one line rather than consume it. Without + * this, `Last activity: …\nMy Heading\n===` reported "My Heading ===" as dropped + * continuation text (#3696 review round 2). + */ +const SETEXT_UNDERLINE_RE = /^(?:=+|-+)\s*$/; + +const STATE_SIBLING_FIELD_LINE_RE = /^\*{0,2}[A-Za-z][A-Za-z0-9 _-]*\*{0,2}:{1,2}\*{0,2}(?:\s|$)/; + +/** + * Return the prose that FOLLOWS a single-line field but plainly belongs to it — + * i.e. the remainder `stateExtractField` silently drops when a writer emits a + * value long enough to wrap. + * + * `stateExtractField`'s `(.+)` is newline-excluding, so + * + * Last activity: 2026-08-19 — Project initialized from ingest; PROJECT.md, + * REQUIREMENTS.md, ROADMAP.md written + * + * yields only the first line and the rest is lost with no diagnostic (#3696). + * `templates/state.md` prescribes a single-line field, so the DOCUMENT is what + * is wrong here, not the reader — this function exists so `state validate` can + * SAY so, not so the reader can start guessing at a multi-line grammar the + * template does not sanction. + * + * That is also why the fix is not in `stateExtractField` itself: it has 20 + * direct callers and a CRITICAL blast radius (ADR-3180 §7.7, Rejected #1), and + * joining continuations there would apply to every field — `Status:` would + * swallow the line beneath it. + * + * Returns `null` when the field is absent, is a pipe-table row (a table cell + * cannot wrap), or is followed by end-of-file, a blank line, Markdown + * structure, or a sibling field. + */ +export function stateFieldContinuation(content: string, fieldName: string): string | null { + const escaped = escapeRegex(fieldName); + // Same two single-line grammars stateExtractField uses, in the same order, so + // this locates exactly the line whose value it returned. The pipe-table rung + // is deliberately absent: a `| Field | value |` row is bounded by its closing + // pipe and cannot wrap. + const match = + new RegExp(`\\*\\*${escaped}:\\*\\*[ \\t]*(.+)`, 'i').exec(content) ?? + new RegExp(`^${escaped}:[ \\t]*(.+)`, 'im').exec(content); + if (!match) return null; + + // `(.+)` stops at the line terminator, so the field's line ends where the + // match does. JS `.` excludes \r as well as \n, so on a CRLF document the \r + // sits just AFTER the match rather than inside it — hence the strip below + // before testing for the newline. + const afterValue = match.index + match[0].length; + const rest = content.slice(afterValue).replace(/^\r/, ''); + if (!rest.startsWith('\n')) return null; // end of file: nothing follows + + const lines = rest.slice(1).split('\n').map((line) => line.replace(/\r$/, '')); + const continuation: string[] = []; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (!line.trim()) break; + if (MD_STRUCTURE_LINE_RE.test(line)) break; + if (STATE_SIBLING_FIELD_LINE_RE.test(line)) break; + // Look ahead one line: a setext underline below makes THIS line a heading + // title, so stop before consuming it rather than after. + if (i + 1 < lines.length && SETEXT_UNDERLINE_RE.test(lines[i + 1])) break; + continuation.push(line.trim()); + } + return continuation.length ? continuation.join(' ') : null; +} + export function stateExtractField(content: string, fieldName: string): string | null { const escaped = escapeRegex(fieldName); // Bold inline format: **FieldName:** value diff --git a/src/state.cts b/src/state.cts index 0b023557b..75ef18883 100644 --- a/src/state.cts +++ b/src/state.cts @@ -75,6 +75,14 @@ import { shouldPreserveExistingProgress, stateExtractField, stateFieldValue, + // #3696: the `last_activity` invariant that `state validate` (S008/S009) now + // asserts. Both live in the field-semantics owner, not here, so `smart-entry` + // and `state validate` cannot drift apart about the same field. + // `leadingCalendarDate` wraps `isRealCalendarDate`, which smart-entry calls + // directly — one predicate, two callers, no copies. + isUnfilledFieldValue, + leadingCalendarDate, + stateFieldContinuation, stateReplaceField, KNOWN_TEMPLATE_DEFAULTS, stateReplaceFieldIfTemplate, @@ -4279,10 +4287,30 @@ function stateDiagnostic(code: string, severity: Severity, message: string, advi return { code, severity, message, remedy: adviseRemedy(advice) }; } -function cmdStateValidate(cwd: string, raw: boolean): void { +function cmdStateValidate(cwd: string, raw: boolean, opts: { strict?: boolean } = {}): void { const statePath = planningPaths(cwd).state; + // #3696: `valid: false` used to exit 0, so a CI step or git hook could not gate + // on state correctness without parsing JSON — every consumer had to + // re-implement the "is this actually valid" decision, which is the + // duplication #3473 is about. + // + // The DEFAULT is deliberately unchanged. `state validate`'s exit status is + // Tier-2 observable output reaching "downstream projects that cannot be + // enumerated" (ADR-3180 Decision 3, Hyrum's Law), so flipping 0 -> 1 for + // everyone would break every script that runs it unconditionally. `--strict` + // is the opt-in the issue itself offers as the alternative. + // + // Routed through one emit helper rather than a trailing assignment because + // three of the exit paths below (`STATE.md not found`, S001, and the four + // `return` branches in the phase-drift scan) emit and return early — a fix + // that only set the exit code at the end of the function would silently miss + // them, which is exactly the shape of the bug being fixed. + const emit = (payload: { valid?: boolean; error?: string; warnings?: Diagnostic[]; scope?: planningScopeMod.Scope }): void => { + if (opts.strict && payload.valid !== true) process.exitCode = 1; + output(payload, raw, undefined); + }; if (!fs.existsSync(statePath)) { - output({ error: 'STATE.md not found' }, raw, undefined); + emit({ error: 'STATE.md not found' }); return; } @@ -4296,10 +4324,10 @@ function cmdStateValidate(cwd: string, raw: boolean): void { // unconditionally and returned immediately, matching every other // error-class code, not a mere warning). Message reused verbatim from // `textEncodingError`, not paraphrased. - output({ + emit({ valid: false, warnings: [stateDiagnostic('S001', SEVERITY.ERROR, encErr, 'Re-save STATE.md as UTF-8 text with the embedded NUL byte(s) removed')], - }, raw, undefined); + }); return; } const warnings: Diagnostic[] = []; @@ -4326,7 +4354,7 @@ function cmdStateValidate(cwd: string, raw: boolean): void { 'Cannot validate phase drift: STATE.md has no usable current_phase, Current Phase, or Current Position Phase value', 'Set current_phase (frontmatter) or Current Phase / Current Position Phase (body) in STATE.md', )); - output({ valid: false, warnings, scope }, raw, undefined); + emit({ valid: false, warnings, scope }); return; } const selectedPhaseKey = phaseKeyFromToken(currentPhase); @@ -4345,7 +4373,7 @@ function cmdStateValidate(cwd: string, raw: boolean): void { `Cannot validate phase drift: phases directory is missing for phase ${currentPhase}`, 'Create the phases directory or correct current_phase to a phase that exists on disk', )); - output({ valid: false, warnings, scope }, raw, undefined); + emit({ valid: false, warnings, scope }); return; } let phaseDirPath: string; @@ -4359,7 +4387,7 @@ function cmdStateValidate(cwd: string, raw: boolean): void { `Cannot validate phase drift: no phase directory matches phase ${currentPhase}`, 'Create a phase directory matching the current phase or correct current_phase', )); - output({ valid: false, warnings, scope }, raw, undefined); + emit({ valid: false, warnings, scope }); return; } phaseDirPath = path.join(phasesDir, phaseDir.name); @@ -4370,7 +4398,7 @@ function cmdStateValidate(cwd: string, raw: boolean): void { `Cannot validate phase drift: phases directory is unreadable for phase ${currentPhase}`, 'Check phases directory permissions and re-run validate', )); - output({ valid: false, warnings, scope }, raw, undefined); + emit({ valid: false, warnings, scope }); return; } try { @@ -4462,8 +4490,68 @@ function cmdStateValidate(cwd: string, raw: boolean): void { )); } + // #3696 — the `last_activity` invariant. Three readers consumed this field + // and none of them checked it, so a value no reader can parse validated as + // `{valid:true, warnings:[], scope:'complete'}`: the scan ran to completion + // and simply never looked. Read through the same owner every other field here + // uses (ADR-3180 §7.7) — never a private `stateExtractField` call, which is + // what `scripts/lint-state-field-drift.cjs` counts. + const lastActivity = stateFieldValue(fm, body, 'last_activity', 'Last activity').value; + // NOT FILLED IN IS NOT DRIFT, and that covers three shapes, not one: absent, + // blank, and the shipped template's `[YYYY-MM-DD] — [What happened]` + // placeholder. Only a value a writer actually supplied can be wrong. + if (!isUnfilledFieldValue(lastActivity)) { + // Calendar validity, not merely `\d{4}-\d{2}-\d{2}` shape: smart-entry's + // reader rejects 2026-02-30 via isRealCalendarDate (ADR-227 — validate shape + // AND value). Accepting it here would leave the two surfaces disagreeing + // about whether the file is usable, which is the complaint #3696 opens with. + // + // Review round 2: this asserts the LEADING date token, not + // `parseProseLastActivityField`'s fully-anchored `date — description` + // grammar. That grammar is stricter than any real reader, and routing the + // check through it made S008 fire on values smart-entry parses fine (e.g. + // `2026-08-24 Shipped feature X`, no dash separator) — the same + // two-surfaces-disagree defect, pointing the other way. See + // `leadingCalendarDate`. + if (leadingCalendarDate(lastActivity) === null) { + warnings.push(stateDiagnostic( + 'S008', + SEVERITY.WARNING, + `Unreadable last activity: "${lastActivity}" does not begin with a real calendar date, so no reader can date this project's activity`, + 'Rewrite the Last activity line to begin with a date that exists, as "YYYY-MM-DD — what happened"', + )); + } + + // The attached half of #3696: `templates/state.md` prescribes a single-line + // field, but writers emit descriptions long enough to wrap, and + // `stateExtractField`'s newline-excluding `(.+)` drops the remainder with no + // diagnostic. The DOCUMENT is what violates the template here, so this + // reports the violation rather than teaching the reader a multi-line grammar + // the template does not sanction (ADR-3180 §7.7 Rejected #1 forbids widening + // stateExtractField, which has 20 callers and a CRITICAL blast radius). + // + // Scan the body ONLY when the body is what was actually read. The ladder + // prefers the frontmatter scalar, so a document carrying a clean + // `last_activity:` in frontmatter AND a stale, wrapped `Last activity:` line + // in the body would otherwise report S009 — and exit 1 under `--strict` — + // over a remainder that no reader consumes and whose field is entirely + // valid. Asking the owner with an EMPTY body isolates the frontmatter rung + // without re-deriving the ladder here (which is what + // `scripts/lint-state-field-drift.cjs` counts). + const fromFrontmatter = stateFieldValue(fm, '', 'last_activity', 'Last activity').value; + const dropped = fromFrontmatter !== null ? null : stateFieldContinuation(body, 'Last activity'); + if (dropped !== null) { + warnings.push(stateDiagnostic( + 'S009', + SEVERITY.WARNING, + `Truncated last activity description: "${dropped}" follows the Last activity line and is silently dropped by every reader`, + 'Fold the Last activity description onto one line — the template prescribes a single-line field', + )); + } + } + const valid = warnings.length === 0; - output({ valid, warnings, scope }, raw, undefined); + emit({ valid, warnings, scope }); } /** diff --git a/tests/lint-health-diagnostic-rule-table.test.cjs b/tests/lint-health-diagnostic-rule-table.test.cjs index 87e319907..f9f352585 100644 --- a/tests/lint-health-diagnostic-rule-table.test.cjs +++ b/tests/lint-health-diagnostic-rule-table.test.cjs @@ -24,6 +24,7 @@ const { PERMANENTLY_INERT_CODES, STATE_VALIDATE_TEST_FILE, STATE_VALIDATE_CODES, + discoverStateValidateCodes, } = guard; const FAKE_SEVERITY = Object.freeze({ ERROR: 'error', WARNING: 'warning', INFO: 'info' }); @@ -263,14 +264,66 @@ describe('checkFixtureProofInvariant (S0NN pass, #3310)', () => { assert.deepEqual(uncovered, []); }); - test('all 7 real STATE_VALIDATE_CODES (S001-S007) are fixture-covered against the real tests/state.test.cjs', () => { - assert.ok(fs.existsSync(STATE_VALIDATE_TEST_FILE), 'tests/state.test.cjs must exist'); - assert.deepEqual(STATE_VALIDATE_CODES, ['S001', 'S002', 'S003', 'S004', 'S005', 'S006', 'S007']); + test('discoverStateValidateCodes reads the code set off stateDiagnostic() call sites', () => { + // #3696: the guard's S0NN list used to be a frozen literal, and this test + // used to be a second frozen literal mirroring it. Two copies of an + // allowlist prove only that they agree with each other — which is exactly + // what let S008/S009 be added while the guard reported a green + // "7 code(s), all fixture-covered" (ADR-3180 Decision 4(a): a zero it did + // not earn). The behaviour under test is now the DISCOVERY RULE, driven + // against fixture source text rather than the real module. + const fixture = [ + "warnings.push(stateDiagnostic('S002', SEVERITY.WARNING, 'a', 'b'));", + 'warnings.push(stateDiagnostic(', + " 'S001',", + ' SEVERITY.ERROR,', + '));', + 'warnings.push(stateDiagnostic(`S002`, SEVERITY.WARNING, "dup", "b"));', + "// stateDiagnostic('S404', ...) in a comment still counts — this is a", + '// regex guard, and over-inclusion only ever demands MORE fixture cover.', + // Review round 2: a space before `(` must not drop the call site. The + // fail-closed check only fires on a FULLY empty result, so a PARTIAL + // miss escaped the fixture-proof check silently. + "warnings.push(stateDiagnostic ('S010', SEVERITY.WARNING, 'spaced', 'b'));", + ].join('\n'); + + assert.deepEqual( + discoverStateValidateCodes(fixture, 'fixture'), + ['S001', 'S002', 'S010', 'S404'], + 'codes are deduplicated and sorted, across quoting styles, line breaks, and a space before the paren', + ); + }); + + test('discoverStateValidateCodes fails closed when the call shape changes', () => { + // The branch that matters. If stateDiagnostic() is renamed, the honest + // answer is "I can no longer see the codes", never "there are none, and they + // are all covered". + assert.throws( + () => discoverStateValidateCodes('renamedHelper("S001", SEVERITY.WARNING);', 'fixture'), + (err) => /found no stateDiagnostic\(\) call sites in fixture/.test(String(err.message)), + 'an unrecognised call shape must raise, not return []', + ); + }); + + test('every code cmdStateValidate can emit is fixture-covered against the real state test file', () => { + assert.ok(fs.existsSync(STATE_VALIDATE_TEST_FILE), 'the state test file must exist'); + assert.ok(STATE_VALIDATE_CODES.length > 0, 'the real discovery pass must find at least one code'); const rules = STATE_VALIDATE_CODES.map((code) => ({ code })); const { uncovered } = checkFixtureProofInvariant(rules, [STATE_VALIDATE_TEST_FILE], new Map()); - assert.deepEqual(uncovered, []); + assert.deepEqual(uncovered, [], `uncovered S0NN codes: ${uncovered.join(', ')}`); + }); + + test('the fixture-proof pass still fails on a code with no test (the guard can actually fail)', () => { + // A guard nobody has watched fail is a guard nobody knows works. + const { uncovered } = checkFixtureProofInvariant( + [...STATE_VALIDATE_CODES.map((code) => ({ code })), { code: 'S999' }], + [STATE_VALIDATE_TEST_FILE], + new Map(), + ); + + assert.deepEqual(uncovered, ['S999']); }); }); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index d9e135396..6107e733c 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -4820,6 +4820,506 @@ describe('#3310 state validate — S0NN coded diagnostics', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3696 — the `last_activity` invariant is CHECKABLE, and `--strict` makes it +// gateable. +// +// Before this, a STATE.md whose `Last activity:` value no reader can parse +// validated as `{valid:true, warnings:[], scope:"complete"}` — the scan ran +// fully and had nothing to say, because `cmdStateValidate` never read the field +// at all. And `valid:false` still exited 0, so no CI step or git hook could gate +// on state correctness without parsing JSON. +// +// S008 = the value is present but does not name a real calendar date. +// S009 = the description was truncated by a line wrap. +// +// Calendar validity (not merely `\d{4}-\d{2}-\d{2}` shape) is the invariant on +// purpose: `smart-entry`'s reader already rejects `2026-02-30` via +// `isRealCalendarDate` (ADR-227 — validate shape AND value). Accepting it here +// would leave the two surfaces disagreeing about whether the file is usable, +// which is the defect #3696 opens with, not a fix for it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3696 state validate — last_activity invariant (S008/S009) and --strict', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // A document that validates CLEAN: phase resolves, phase dir exists, plan + // count agrees, no verification file. Extra body lines are appended verbatim + // so each case differs ONLY in the last_activity shape under test. + function writeCleanState(extraBodyLines = [], opts = {}) { + const eol = opts.crlf ? '\r\n' : '\n'; + const head = opts.frontmatter ? ['---', ...opts.frontmatter, '---', ''] : []; + const lines = [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 1', + ...extraBodyLines, + '', + ]; + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), [...head, ...lines].join(eol)); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + } + + function validate(args = 'state validate') { + const result = runGsdTools(args, tmpDir); + return { result, output: JSON.parse(result.output) }; + } + + // ── S008: the value must name a real calendar date ────────────────────────── + + test('S008: an unparseable last_activity is reported instead of validating clean', () => { + writeCleanState(['Last activity: not-a-date — broke the date on purpose']); + + const { output } = validate(); + assert.strictEqual(output.scope, 'complete', 'the scan must have actually run — this is not a degraded-scope excuse'); + assert.strictEqual(output.valid, false, 'an unreadable last_activity must not validate clean'); + const s008 = findWarning(output, 'S008'); + assert.ok(s008, `S008 must fire for an unparseable last_activity; got: ${JSON.stringify(output.warnings)}`); + assert.strictEqual(s008.severity, SEVERITY.WARNING); + assert.strictEqual(s008.remedy.action, 'advise'); + assert.match(s008.message, /last activity/i); + assertNoDriftKey(output); + }); + + test('S008: a well-formed last_activity with a description stays clean', () => { + writeCleanState(['Last activity: 2026-08-19 — did a thing']); + + const { output } = validate(); + assert.strictEqual(output.valid, true, `well-formed control must stay clean; got: ${JSON.stringify(output.warnings)}`); + assert.deepStrictEqual(output.warnings, []); + }); + + test('S008: a bare well-formed date with no description stays clean', () => { + // parseProseLastActivityField returns description:null for this shape; it is + // a legitimate value, not a truncation. + writeCleanState(['Last activity: 2026-08-19']); + + const { output } = validate(); + assert.ok(!findWarning(output, 'S008'), `a bare date is a valid shape; got: ${JSON.stringify(output.warnings)}`); + assert.ok(!findWarning(output, 'S009'), 'a bare date is not a truncated description'); + }); + + test('S008: an absent last_activity is not a defect (a fresh project must stay clean)', () => { + // The single most important negative case: a freshly-initialized STATE.md + // has no activity yet. Flagging absence would fire on every new project. + writeCleanState([]); + + const { output } = validate(); + assert.strictEqual(output.valid, true, `absence is not drift; got: ${JSON.stringify(output.warnings)}`); + assert.ok(!findWarning(output, 'S008')); + }); + + test('S008: a frontmatter-only last_activity is validated through the same owner', () => { + // Routes the read through stateFieldValue's frontmatter rung — the same owner + // cmdStateValidate already uses for status/total_plans_in_phase, so the + // fm-only shape is not a blind spot (ADR-3180 §7.7). + writeCleanState([], { frontmatter: ['current_phase: 1', 'status: executing', 'last_activity: not-a-date'] }); + + const { output } = validate(); + const s008 = findWarning(output, 'S008'); + assert.ok(s008, `S008 must fire for a frontmatter-only last_activity; got: ${JSON.stringify(output.warnings)}`); + }); + + test('S008: an ASCII-hyphen separator is accepted like an em dash', () => { + writeCleanState(['Last activity: 2026-08-19 - did a thing']); + + const { output } = validate(); + assert.ok(!findWarning(output, 'S008'), `the owner regex accepts an ASCII hyphen; got: ${JSON.stringify(output.warnings)}`); + }); + + test('S008: a shape-valid but calendar-impossible date is rejected (the two surfaces must not disagree)', () => { + // 2026-02-30 matches \d{4}-\d{2}-\d{2} but does not exist. smart-entry's + // isRealCalendarDate already rejects it (ADR-227). If state validate accepted + // it, the two readers would still disagree about whether the file is usable — + // the exact complaint #3696 opens with. + writeCleanState(['Last activity: 2026-02-30 — a day that does not exist']); + + const { output } = validate(); + const s008 = findWarning(output, 'S008'); + assert.ok(s008, `S008 must fire for an impossible calendar date; got: ${JSON.stringify(output.warnings)}`); + }); + + test('S008: month and day boundaries fire on limit-1 and limit+1 only', () => { + const cases = [ + ['2026-00-15', true], // month limit-1 + ['2026-01-15', false], // month limit (low) + ['2026-12-15', false], // month limit (high) + ['2026-13-15', true], // month limit+1 + ['2026-01-00', true], // day limit-1 + ['2026-01-01', false], // day limit (low) + ['2026-01-31', false], // day limit (high, 31-day month) + ['2026-01-32', true], // day limit+1 + ]; + for (const [value, mustFire] of cases) { + writeCleanState([`Last activity: ${value} — boundary probe`]); + const { output } = validate(); + const fired = Boolean(findWarning(output, 'S008')); + assert.strictEqual(fired, mustFire, `${value}: expected S008 fired=${mustFire}, got ${fired} (${JSON.stringify(output.warnings)})`); + } + }); + + test('isRealCalendarDate: state validate and smart-entry agree on calendar validity', () => { + // Parity assertion (CLAUDE.md "Generative Fix Divergence"): the predicate has + // ONE owner and both surfaces import it. This fails the moment a second copy + // appears and drifts. + const smartEntry = require('../gsd-core/bin/lib/smart-entry.cjs'); + assert.strictEqual( + typeof stateDocument.isRealCalendarDate, + 'function', + 'state-document.cjs must own isRealCalendarDate', + ); + for (const [y, m, d, expected] of [ + [2026, 2, 30, false], + [2026, 2, 28, true], + [2024, 2, 29, true], + [2026, 2, 29, false], + [2026, 13, 1, false], + [2026, 12, 31, true], + ]) { + assert.strictEqual( + stateDocument.isRealCalendarDate(y, m, d), + expected, + `owner disagrees on ${y}-${m}-${d}`, + ); + } + assert.ok( + !Object.prototype.hasOwnProperty.call(smartEntry, 'isRealCalendarDate') + || smartEntry.isRealCalendarDate === stateDocument.isRealCalendarDate, + 'smart-entry must reuse the owner, never re-declare its own copy', + ); + }); + + test('property: no real calendar date ever raises S008', () => { + fc.assert( + fc.property( + fc.date({ min: new Date(Date.UTC(2000, 0, 1)), max: new Date(Date.UTC(2099, 11, 31)) }), + (d) => { + const iso = d.toISOString().slice(0, 10); + // Suffix VARIES: a dashed description, a bare date, and a + // separator-less description. A fixed `— probe` suffix is what + // let the round-2 false positive through this property. + const suffix = ['', ' — property probe', ' property probe'][d.getUTCDate() % 3]; + writeCleanState([`Last activity: ${iso}${suffix}`]); + const { output } = validate(); + assert.ok( + !findWarning(output, 'S008'), + `S008 must never fire for the real calendar date ${iso}${suffix}; got: ${JSON.stringify(output.warnings)}`, + ); + }, + ), + { numRuns: 12 }, + ); + }); + + // ── S009: a wrapped description must not vanish ───────────────────────────── + + test('S009: a wrapped last_activity description is reported instead of silently truncated', () => { + writeCleanState([ + 'Last activity: 2026-08-19 — Project initialized from ingest (SPEC-pal-restore.md); PROJECT.md,', + 'REQUIREMENTS.md, ROADMAP.md written', + ]); + + const { output } = validate(); + assert.strictEqual(output.valid, false, 'a truncated description must not validate clean'); + const s009 = findWarning(output, 'S009'); + assert.ok(s009, `S009 must fire for a wrapped description; got: ${JSON.stringify(output.warnings)}`); + assert.strictEqual(s009.severity, SEVERITY.WARNING); + assert.strictEqual(s009.remedy.action, 'advise'); + assertNoDriftKey(output); + }); + + test('S009: a blank line after last_activity is structure, not a wrap', () => { + writeCleanState(['Last activity: 2026-08-19 — done', '', 'Some later prose.']); + + const { output } = validate(); + assert.ok(!findWarning(output, 'S009'), `a blank line ends the field; got: ${JSON.stringify(output.warnings)}`); + }); + + test('S009: a following field line is structure, not a wrap', () => { + writeCleanState(['Last activity: 2026-08-19 — done', 'Blockers: none']); + assert.ok(!findWarning(validate().output, 'S009'), 'a sibling field is not a continuation'); + + writeCleanState(['Last activity: 2026-08-19 — done', '**Blockers:** none']); + assert.ok(!findWarning(validate().output, 'S009'), 'a bold sibling field is not a continuation'); + }); + + test('S009: a following heading is structure, not a wrap', () => { + writeCleanState(['Last activity: 2026-08-19 — done', '## Next Up']); + + assert.ok(!findWarning(validate().output, 'S009')); + }); + + test('S009: a following list marker is structure, not a wrap', () => { + for (const marker of ['- item', '* item', '+ item', '1. item', '2) item']) { + writeCleanState(['Last activity: 2026-08-19 — done', marker]); + const { output } = validate(); + assert.ok(!findWarning(output, 'S009'), `"${marker}" is a list, not a continuation; got: ${JSON.stringify(output.warnings)}`); + } + }); + + test('S009: a following table row or horizontal rule is structure, not a wrap', () => { + // The `---` case is the horizontal-rule trap: a check that fires on + // legitimate Markdown structure is worse than no check at all. + for (const line of ['| Field | Value |', '---', '***', '___', '> quoted', '```']) { + writeCleanState(['Last activity: 2026-08-19 — done', line]); + const { output } = validate(); + assert.ok(!findWarning(output, 'S009'), `"${line}" is structure, not a continuation; got: ${JSON.stringify(output.warnings)}`); + } + }); + + test('S009: last_activity as the final line with no trailing newline does not fire', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + '# Project State\n\n**Status:** Executing Phase 1\n**Current Phase:** 1\n**Total Plans in Phase:** 1\nLast activity: 2026-08-19 — done', + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + + const { output } = validate(); + assert.ok(!findWarning(output, 'S009'), `end-of-file is not a continuation; got: ${JSON.stringify(output.warnings)}`); + }); + + test('S009: CRLF line endings produce the same verdict as LF', () => { + writeCleanState([ + 'Last activity: 2026-08-19 — Project initialized from ingest; PROJECT.md,', + 'REQUIREMENTS.md written', + ], { crlf: true }); + assert.ok(findWarning(validate().output, 'S009'), 'a CRLF wrap must fire exactly like LF'); + + writeCleanState(['Last activity: 2026-08-19 — done', 'Blockers: none'], { crlf: true }); + assert.ok(!findWarning(validate().output, 'S009'), 'a CRLF sibling field must not fire'); + }); + + + // ── Review round 2 — cross-surface agreement and structure false positives ── + + test('S008: a value the real reader parses is not reported unreadable (no separator before the description)', () => { + // The first cut asserted through parseProseLastActivityField, whose grammar + // is fully anchored and REQUIRES a dash separator. smart-entry's + // parseActivityTimestamp needs only a leading date, so this value parses + // fine there while S008 called it unreadable — the same + // two-surfaces-disagree defect #3696 exists to close, pointing the other + // way. Asserted against the real reader, not against a restatement of it. + const smartEntry = require('../gsd-core/bin/lib/smart-entry.cjs'); + const value = '2026-08-24 Shipped feature X without a dash separator'; + + if (typeof smartEntry.parseActivityTimestamp === 'function') { + assert.ok( + Number.isFinite(smartEntry.parseActivityTimestamp(value)), + 'precondition: the real reader must parse this value', + ); + } + + writeCleanState([`Last activity: ${value}`]); + const { output } = validate(); + assert.ok( + !findWarning(output, 'S008'), + `S008 must not fire on a value the reader parses; got: ${JSON.stringify(output.warnings)}`, + ); + }); + + test('S008: an ISO date-time prefix is accepted', () => { + writeCleanState(['Last activity: 2026-08-24T09:00:00Z shipped it']); + + assert.ok(!findWarning(validate().output, 'S008')); + }); + + test('S008: a date-shaped run with no separators is still rejected', () => { + // Boundary on the leading-token rule itself: `20260824` is eight digits, not + // a date, and must not be admitted just because it starts with four. + writeCleanState(['Last activity: 20260824 shipped it']); + + assert.ok(findWarning(validate().output, 'S008'), '`20260824` is not a leading ISO date token'); + }); + + test('S009: a setext heading underneath last_activity is structure, not a wrap', () => { + // Both underline styles. `===` was missed entirely by the first cut, and + // `---` was missed differently: the rule stopped AT the underline, having + // already swallowed the heading TITLE above it as prose. Detection has to + // look ahead one line, so both are pinned here. + for (const underline of ['===', '---', '======', '- - -'.replace(/ /g, '')]) { + writeCleanState(['Last activity: 2026-08-19 — done', 'My Heading', underline]); + const { output } = validate(); + assert.ok( + !findWarning(output, 'S009'), + `a setext heading underlined with "${underline}" is structure; got: ${JSON.stringify(output.warnings)}`, + ); + } + }); + + test('S009: an indented code block is structure, not a wrap', () => { + for (const indented of [' const x = 1;', '\tconst x = 1;']) { + writeCleanState(['Last activity: 2026-08-19 — done', indented]); + const { output } = validate(); + assert.ok( + !findWarning(output, 'S009'), + `an indented code block is structure; got: ${JSON.stringify(output.warnings)}`, + ); + } + }); + + test('S009: an HTML block is structure, not a wrap', () => { + writeCleanState(['Last activity: 2026-08-19 — done', '
a note
']); + + assert.ok(!findWarning(validate().output, 'S009')); + }); + + + test('S009: a frontmatter-sourced last_activity is not judged by a stale wrapped body line', () => { + // The ladder prefers the frontmatter scalar, so when frontmatter supplies + // last_activity NOBODY reads the body line. Scanning it anyway reported a + // dropped remainder that no reader consumes — and under --strict exited 1 — + // on a document whose actual last_activity is entirely valid. + writeCleanState( + [ + 'Last activity: 2026-01-01 — a stale body line that', + 'wraps onto a second line', + ], + { frontmatter: ['current_phase: 1', 'status: executing', 'last_activity: 2026-08-19'] }, + ); + + const { result, output } = validate('state validate --strict'); + assert.ok( + !findWarning(output, 'S009'), + `the body line is shadowed by frontmatter and must not be judged; got: ${JSON.stringify(output.warnings)}`, + ); + assert.strictEqual(output.valid, true); + assert.strictEqual(result.exitCode, 0, '--strict must not fail a document whose last_activity is valid'); + }); + + test('S008: a frontmatter-sourced last_activity is still judged on its own value', () => { + // The complement of the test above: shadowing must suppress the BODY scan, + // never the check itself. + writeCleanState( + ['Last activity: 2026-08-19 — a clean body line'], + { frontmatter: ['current_phase: 1', 'status: executing', 'last_activity: not-a-date'] }, + ); + + assert.ok( + findWarning(validate().output, 'S008'), + 'the frontmatter value is the one every reader uses, so it is the one that must be checked', + ); + }); + + test('S008: a last_activity line with only whitespace reads as not-yet-filled-in, not as drift', () => { + // stateExtractField's `[ \t]*(.+)` backtracks to hand back a single space, + // so the value arrives as '' — non-null, and it used to reach S008 and + // report the empty string back at the reader. + writeCleanState(['Last activity: ']); + + const { output } = validate(); + assert.ok( + !findWarning(output, 'S008'), + `an empty value is indistinguishable from absence; got: ${JSON.stringify(output.warnings)}`, + ); + assert.strictEqual(output.valid, true); + }); + + + test('S008: the SHIPPED state template validates clean (its last_activity is an unfilled placeholder)', () => { + // templates/state.md:35 ships `Last activity: [YYYY-MM-DD] — [What happened]`, + // so this is the state of EVERY freshly-initialized project until something + // records activity. The first cut of S008 spared only the ABSENT form and + // fired on the shipped template itself — caught by the pre-existing + // "template-equivalent phase identities remain clean without disk drift" + // test. This pins the same invariant from the S008 side, where the + // regression would actually be introduced. + const stateContent = readShippedStateTemplateBody([ + ['status: planning', ['current_phase: 2', 'status: planning'].join('\n')], + ['Phase: [X] of [Y] ([Phase name])', 'Phase: 02 of 2 (State Validation Drift Diagnostics)'], + ['Status: [Ready to plan / Planning / Ready to execute / In progress / Phase complete]', 'Status: Planning'], + ]); + assert.match( + stateContent, + /Last activity: \[YYYY-MM-DD\]/, + 'precondition: the shipped template must still carry the placeholder this test is about', + ); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + fs.mkdirSync( + path.join(tmpDir, '.planning', 'phases', '02-state-validation-drift-diagnostics'), + { recursive: true }, + ); + + const { output } = validate(); + assert.strictEqual(output.valid, true, `the shipped template must validate clean; got: ${JSON.stringify(output.warnings)}`); + assert.deepStrictEqual(output.warnings, []); + }); + + test('S008: a bracket placeholder only counts as unfilled at the START of the value', () => { + // Guard against the over-broad reading. A real description that cites a + // bracketed reference is a filled-in value, and its date must still be + // checked — otherwise the placeholder rule silently swallows genuine drift. + writeCleanState(['Last activity: not-a-date — see [#123] for context']); + + assert.ok( + findWarning(validate().output, 'S008'), + 'a bracket later in the value does not make the value unfilled', + ); + }); + + // ── --strict: the exit status becomes gateable, opt-in only ───────────────── + + test('--strict: a document with warnings exits non-zero', () => { + writeCleanState(['Last activity: not-a-date — broken']); + + const { result, output } = validate('state validate --strict'); + assert.strictEqual(output.valid, false); + assert.strictEqual(result.exitCode, 1, 'a CI step must be able to gate on the exit status'); + }); + + test('--strict: a clean document still exits zero', () => { + writeCleanState(['Last activity: 2026-08-19 — done']); + + const { result, output } = validate('state validate --strict'); + assert.strictEqual(output.valid, true); + assert.strictEqual(result.exitCode, 0); + }); + + test('--strict: the default exit status is unchanged when the flag is absent', () => { + // Hyrum's Law guard (ADR-3180 Decision 3): state validate's exit status is + // observable behaviour reaching downstream consumers that cannot be + // enumerated. Flipping the DEFAULT would break every script that runs it + // unconditionally, so the new behaviour is opt-in — and this test fails if + // anyone later "simplifies" it into the default. + writeCleanState(['Last activity: not-a-date — broken']); + + const { result, output } = validate(); + assert.strictEqual(output.valid, false); + assert.strictEqual(result.exitCode, 0, 'the default exit status must NOT change'); + }); + + test('--strict: the S001 early-return path also exits non-zero', () => { + // S001 returns early from its own output(...) call; a fix that only set the + // exit code at the end of the function would miss this branch. + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), Buffer.from('# Project State\0corrupt')); + + const { result, output } = validate('state validate --strict'); + assert.strictEqual(output.valid, false); + assert.strictEqual(result.exitCode, 1); + }); + + test('--strict: a missing STATE.md exits non-zero', () => { + // createFixture() makes .planning/ but no STATE.md — the + // {error:'STATE.md not found'} pre-check shape, a third early return. + const { result, output } = validate('state validate --strict'); + assert.ok(output.error, 'the not-found shape is unchanged'); + assert.strictEqual(result.exitCode, 1); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // #3187 (ADR-3180 §7.7) — matrix section B: `state validate`'s scope field, // including the #3162 headline regression and #1255 frontmatter shadowing.