Skip to content

fix(#4998): read the decoded milestone scalar, so a quoted milestone stops gaining an escape layer - #5234

Open
behruznassre wants to merge 56 commits into
open-gsd:nextfrom
behruznassre:fix/4998-state-milestone-escaping-session-continuity
Open

behruznassre wants to merge 56 commits into
open-gsd:nextfrom
behruznassre:fix/4998-state-milestone-escaping-session-continuity

Conversation

@behruznassre

@behruznassre behruznassre commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Fix PR

Linked Issue

Fixes #4998

Both halves, per the maintainer's direction on this PR. #5246 (the milestone half, split out in the previous round) is closed as superseded by this PR.

What was broken

Milestone escaping. A milestone: value YAML has to double-quote (one containing a literal ") was read back from STATE.md as its on-disk YAML text, quotes and backslashes included, and re-serialized one escape layer deeper. On current next I reproduced it in state.json: with milestone: "\"v1.1 — Example milestone\"" in STATE.md, state begin-phase wrote "milestone": "\"\\\"v1.1 — Example milestone\\\"\"". I could not reproduce STATE.md's own frontmatter growing on next: begin-phase, add-decision, record-metric, record-session and state sync, three rounds each, left it byte-identical. So this PR claims the state.json symptom it can demonstrate, not the unbounded growth reported on 1.14.0.

record-session damaging prose, each reproduced on next:

  • A field whose existing value wraps onto the next line was rewritten on its first line only, leaving the rest as an orphan (the issue's Last session: <new text> / to .planning/milestones/, … example).
  • A ## Session Continuity section written in the template's plain form (Last session: / Resume file:) with no Stopped at: line got **Stopped at:** … inserted in bold, directly under the heading and above Last session:. That's a line the template does not define.
  • A stopped_at note kept only in frontmatter (no body line) was replaced with no report. A note on the body line has already been reported in replacedRecord since bug(state): record-session silently overwrites a concurrent session's Stopped-at record, and add-decision/advance-plan resolve the phase from the global pointer instead of the caller (1.14.0) #4763. I could not reproduce a timestamp landing in stopped_at: every shipped caller passes descriptive --stopped-at text, and record-session writes its timestamp only to Last session / Last Date.

What this fix does

Milestone. readStateMilestoneScalar (src/roadmap-parser.cts) reads the value through rawFrontmatterField, the frontmatter parser's own decoded value (under FAILSAFE_SCHEMA, so 1.0 stays the text 1.0). It falls back to the old line read only when there is no parseable frontmatter carrying a string milestone key. getMilestoneInfo and both extractCurrentMilestone readers use it, and classifyMilestoneScalar (#5038) still classifies the result.

record-session, policy (b) from the #4998 thread: a wrapped field is left whole, not partly or wholly replaced.

  • A line that continues a value and a separate note written directly below it look the same, so deleting a "tail" can delete a note. When the line a write would change has a continuation, record-session leaves that field untouched and lists it under skipped ({ field, reason: "wrapped_value", continuation }). It also writes a stderr warning saying how to fix the document: fold the value onto one line, or put a blank line before a separate note. The other fields are still written. If every field it would write wraps, it writes nothing and returns recorded: false with skipped. The check (editedLineContinuation, src/state-document.cts) is located from the edit itself, at the first byte where the old and new content differ, so it reads the occurrence the writer actually touched rather than the first label-regex match. stateFieldContinuation's line scan is shared, not copied (continuationLineCount).
  • A missing field is inserted beside its sibling in template order (Last session → Stopped at → Resume file), below that sibling's own wrapped lines, never inside a fenced example (fences come from scanFencedBlocks, via markdownFencedBlocks). It goes in plain form beside a plain sibling, otherwise in bold **Label:**: the spellings stateExtractField reads back. With no sibling present it goes right after the heading in bold, as before. A canonical ## Session now gets the same in-place insert. It used to be replaced wholesale with three canonical lines, which deleted any note or wrapped value in it and reset an authored Resume file to None. With no session heading at all, the appended section carries the current value of each field the call is not writing (a skipped field included), so a skipped field never comes back carrying the new value.
  • With no body line, the frontmatter stopped_at is the only copy of the record. The write re-derives it, so it's now captured as the prior record and reported in replacedRecord (unless it is the None placeholder).

Single-line values, for every writer. A line break is CR, LF, U+2028 or U+2029 — every character a m-flag ^ starts a line at (FIELD_LINE_BREAK_RE). Every stateReplaceField rung refuses one, and record-session, state update and state patch reject such a value by name before writing. The ADR-4910 seam (setFieldValue, replaceProse in planning-document.cts) owns the constant and refuses the same set; setFieldValue keeps a separator a field already carries only when everything from it on is unchanged, so phase remove still renumbers such a Depends on. state add-roadmap-evolution flattens all four in a note. A value already wrapped by a CR, U+2028 or U+2029 inside its line counts as wrapped (skipped), like an LF continuation. The frontmatter writer quotes and escapes U+2028/U+2029.

Fenced examples are never session fields. Through one scanner (scanFencedBlocks, scoped by FenceScope: only a whole document has frontmatter, whose content is masked as YAML data; in a section a leading --- is a horizontal rule): record-session's writes and stateReplaceFieldInSession (used by state update, state patch and phase complete for every session field: Stopped At, Paused At, Resume File, Last session, Last Date) write outside fences (writeOutsideFences); the frontmatter sync, the preservation pass, state-snapshot and the post-write check that reports what changed read session fields with fences masked (the check reads the session section for update/patch, which write there, and the whole document for record-session, as it writes); a fenced ## Session example is not a section. A document that ends inside an unclosed fence gets no appended section (it would land inside the fence); the call reports unclosed_fence.

Root cause

Milestone: three readers matched ^milestone:\s*(.+) against the raw file text instead of the parsed frontmatter, so the value they handed on was YAML source, not a value. record-session: its writers are single-line by design (stateReplaceField, #4010/#4243), but nothing checked whether the line they replaced had a continuation. The #1101 insert path hard-coded the bold spelling and the position right after the heading. The #944 ## Session path replaced the whole section.

Testing

How I verified the fix

Fixtures with the issue's shapes, against a build of this branch. state.json carries the decoded milestone. In the wrapped and prose-note shapes, the session section is byte-for-byte as before apart from the fields that were written.

Fail-first, against the compiled module:

  • Restoring the raw milestone line read reds the ten-writes end-to-end test and six of the eight #4998 roadmap-parser tests. The other two pin the unchanged fallback.

  • Disabling the wrap check reds seven record-session tests.

  • Forcing the bold spelling reds the two insertion-shape tests.

  • Dropping the frontmatter fallback reds the replacedRecord test.

  • Swapping the edit-located check for a label-regex lookup reds the fenced look-alike test.

  • Disabling the fence mask reds the three fenced-insert tests (plain, nested and indented fences).

  • The five insert-path tests below reproduce defects that the previous head showed in a probe run: the canonical rewrite deleting a note, the appended section re-adding a skipped field, a fenced anchor, a heading glued at EOF, and an unreadable **Label**: insert.

  • npm run lint:ci — exit 0

  • state*, state-document, continuation-grammar-parity and the session-management files (1,554 tests) — all green

  • Full suite on 9140d3e18: 46,480 tests, 46,377 pass, 3 fail, none in this PR's paths. runtime-launcher-parity K2 and host-integration (orchestrator-worktree argv shape) fail the same way on a clean next checkout on this Mac; verification-status feat(completion): thread convention through the completion-path membership seam (epic #612) #4142 fails only because this machine's ~/.gsd/defaults.json prints a stderr warning (303/303 with HOME sandboxed). The branch includes a merge of next at 7a4b76f68.

  • Scoped Stryker (state-document.cjs editedLineStart..insertMissingSessionFields, planning-document.cjs setFieldValue + replaceProse; tests state-document + planning-document; --force): 85.94% overall, state-document.cjs 86.24% (0 no coverage), planning-document.cjs 85.12%

  • This round (review 2026-10-10 and two Codex whole-PR rounds): every new row fails on the commit before its fix; the maintainer's reproduction table re-run on the built head (U+2028 / U+2029 probes refused with STATE.md byte-identical; fenced example untouched; wrapped Last session carried; EOF heading written; update/patch refused by name)

  • Review 5563202317 (Major 2) and two Codex whole-PR rounds: state update/patch of Resume File, Last session and Last Date write the live session line (rows × update/patch, plus an archive decoy); the post-write check reads session fields with fences blanked, in the session section (for record-session too since review 5608265301); a same-value write of a session field reports it updated. Every new row fails on the commit before its fix. The round-2 fix did not get a third Codex pass.

  • Review 5593733001 and two Codex whole-PR rounds (round 2 clean): rows for update/patch of Resume File, Last session and Last Date with no session section ("not found", document unchanged), under ## Session Continuity in bold form, with both headings (## Session written), in a CRLF document, a same-value state patch alone and beside a changed field, and writer/state-snapshot parity per layout; a record-session field written in place before an unclosed fence is not also skipped. The same-value and mixed patch rows and the unclosed-fence row fail on the commit before their fix.

  • Review 5608265301: record-session writes Stopped At, Resume File and Last session in the session section (inserting a field it lacks), reads the displaced record there and reconciles there. Rows: a **Stopped at**: line above an empty ## Session (frontmatter stopped_at and state-snapshot read the new value), the same above a ## Session with a table row and an unclosed fence, and the two archive-above-## Session rows moved to the new contract. Also: the echoed continuation is bounded at 200 characters (rows at 199/200/201); unclosed_fence names only fields the call writes and that were not already written or skipped (two rows); state-document and planning-document are mutation-matrix shards: planning-document floor 74 from CI run 39736529871 (75.14%); state-document measured 50.47% in a 94-minute job (CI run 39742269013), so it keeps floor 50 (floor(50.47) - 1 = 49 is below the range minimum) and has a 140-minute budget; the add-roadmap-evolution note flattening is linear (80k spaces: 3982 ms → 0.1 ms, identical output on 20,000 random strings). Every new row fails on the commit before its fix. Two Codex whole-PR rounds: round 1 found a same-value --stopped-at listed as unclosed_fence (fixed, red before) and a docs over-claim (fixed); round 2 clean.

Regression test added?

  • Yes — added a test that would have caught this bug

  • No — explain why:

  • tests/state.test.cjs:

    • Ten consecutive state.* writes leave the decoded milestone unchanged in STATE.md and in state.json.
    • record-session:
      • The issue's wrapped Last session is left whole and named, and Stopped at is still written.
      • An unseparated prose note and a two-space-indented bullet directly below Stopped at survive byte-for-byte.
      • A wrapped look-alike inside a fence doesn't block the real field.
      • When every field to write wraps, STATE.md is untouched and the result is recorded: false.
      • A missing Stopped at goes in plain form between its siblings.
      • An insert lands below a wrapped sibling's lines, not between them.
      • A canonical ## Session with a wrapped field gets an insert, not the rewrite.
      • A frontmatter-only stopped_at is reported in replacedRecord.
      • A canonical ## Session with a note keeps the note when a field is missing.
      • The appended section never re-adds a skipped field.
      • A fenced sibling label is never the anchor, including inside a nested ```` fence.
      • An insert never lands inside an indented fence.
      • A heading at EOF with no newline is left alone (recorded: false).
      • **Label**: siblings anchor a readable **Label:** insert.
  • tests/roadmap-parser.test.cjs — readStateMilestoneScalar decodes, keeps 1.0 as text, strips a trailing # comment, and falls back to the legacy line read. A fast-check property checks that any value written through the writer's escapeDoubleQuotedScalar reads back byte-identical. getMilestoneInfo, extractCurrentMilestoneScoped and currentMilestoneRawRanges are each asserted directly.

  • tests/state-document.test.cjs — editedLineContinuation: a wrapped tail, a single-line field, an unchanged write, a table row, a value that only extends the old one, CRLF, and a fast-check property over arbitrary surroundings and value pairs. FIELD_LINE_BREAK_RE is checked against the engine over every BMP code point. writeOutsideFences/maskFencedLines/stateReplaceFieldInSession fence rows (including edits touching a fenced line, whitespace-only too). readableEditedBoldLabel boundary rows and a property. insertMissingSessionFields: a property asserting order, count, no-insert-inside-a-fence, template placement and CRLF preservation. Sibling-label rows.

  • tests/state.test.cjs (this round): U+2028/U+2029 × --stopped-at/--resume-file; update/patch with LF, CR, U+2028, U+2029; fenced examples under record-session, state update, state patch and state-snapshot; a fenced ## Session heading; skipped wrapped Last session/Last Date and **Label**: values carried; an unclosed fence; EOF headings; None placeholders; S009 parity for every grammar change.

  • tests/frontmatter.test.cjs — a value holding U+2028/U+2029 is written quoted and escaped.

Platforms tested

  • macOS
  • Windows (including backslash path handling) — CI lane
  • Linux — CI lane
  • N/A (not platform-specific)

Runtimes tested

  • Claude Code
  • Antigravity
  • OpenCode
  • Other: ___
  • N/A (not runtime-specific)

Self-test report

The full suite is reported as it ran: three failures, all environment-only on this machine (named in output_excerpt; each also fails on a clean upstream/next checkout). CI on the queue commit is authoritative.

{
  "schema": 1,
  "head_sha": "9140d3e1858608761cb64abdb838cc504a405038",
  "tool": "npm-test",
  "failing_first": {
    "test": "tests/state.test.cjs",
    "fails_on_base": true,
    "passes_on_head": true
  },
  "commands": [
    "node --test --test-name-pattern=4998 tests/state.test.cjs",
    "npm run lint:ci",
    "npm test"
  ],
  "full_suite": {
    "command": "npm test",
    "result": "failed",
    "passed": 46377,
    "failed": 3
  },
  "environment": {
    "os": "macOS 27.0.1",
    "node": "v24.19.0"
  },
  "output_excerpt": "tests 46480, pass 46377, fail 3, skipped 99, todo 1 (head 9140d3e18). The 3 failures are environment-only on this machine and fail the same way on a clean upstream/next checkout: runtime-launcher-parity (K2, #4834), host-integration (#2627 orchestrator-worktree argv shape), verification-status (#4142; 303/303 with HOME sandboxed). tests/state.test.cjs '#4998' rows on upstream/next 26387bc83: 78 of 98 fail (the quoted milestone gains a layer: expected '\"v1.1 — Example milestone\"', actual '\"\\\\\"v1.1 ...\\\\\"\"'); on the head they pass."
}

Checklist

  • Issue linked above with Fixes #NNN — PR will be auto-closed if missing
  • Linked issue has the confirmed-bug label
  • Fix is scoped to the reported bug — no unrelated changes included
  • Regression test added (or explained why not)
  • All existing tests pass (npm test)
  • .changeset/ fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — or no-changelog label applied
  • No unnecessary dependencies added

Breaking changes

One payload change in state record-session. A wrapped Stopped At record used to be rewritten on its first line and reported in replacedRecord (the #4763 test "a wrapped multi-line Stopped At record is surfaced whole"). It's now left whole, so replacedRecord no longer reports it, and it's listed under the new skipped key. That test now asserts the new contract. A canonical ## Session missing a field is no longer normalized to exactly three bold lines. The missing field is inserted and every other line kept. No existing test pinned the old shape. #4945's Current focus writer is untouched.

Three further behavior changes come from fixing the four items the previous body had deferred (all in this PR now):

  • stateReplaceField returns null for a value containing a line break on every rung — the plain and pipe-table rungs used to splice it in as extra lines; only the bold rung refused. state record-session now rejects a multi-line --stopped-at / --resume-file with an error before writing.
  • record-session respells the line it writes from **Stopped at**: / **Resume file**: to **Label:**, the spelling stateExtractField reads, so the value it just wrote reaches the frontmatter instead of leaving the old one there. stateReplaceField itself still keeps the author's spelling (chore(#4906): drain the grandfather allowlist to zero, add the field-shaped .replace() detector and positive-control lint, ratify ADR-4910 — Phase 6 #5007 contract), and the reader is not widened, because a widened reader would also read a fenced **Status**: example over the live field.
  • With no session heading at all, the appended ## Session section carries the values the sync reads today for any field the call is not setting: a body line, a wrapped field's first line, or a table row. Previously it wrote None or left the field out. That had dropped the frontmatter stopped_at (reproduced) and reset an authored Resume file. Nothing is ever inserted outside a session section, so frontmatter and archive sections are untouched.

This round adds: state update and state patch reject a multi-line value (CR, LF, U+2028, U+2029) with an error naming the field, where they used to report "not found" / failed or, on a plain line, splice it in; state update and state patch write every session field (Resume File, Last session and Last Date included, not only Stopped At / Paused At) inside the session section, so neither a fenced example nor an archive section's copy is the line written, and a same-value write of one reports it updated; and fenced example lines in the session section are no longer read or written as session fields by record-session, state update, state patch, phase complete, the frontmatter sync or state-snapshot. A Resume file: line outside any session section is no longer written by update/patch ("not found"), matching state-snapshot, which reads it only from the session section.

Review 5608265301 adds: when a session section exists, state record-session writes its fields there instead of on the first matching line anywhere. A copy in an archive section above ## Session is left as is, and replacedRecord reports the live record the write replaced (the #4763 row that pinned the archive line being overwritten now pins this). A skipped[].continuation longer than 200 characters is cut there and ends in …, and unclosed_fence is no longer listed for a field the caller did not pass.

This round adds: with no ## Session or ## Session Continuity section, state update/state patch of Resume File, Last session or Last Date report the field not found and leave the document unchanged (previously the first match anywhere was rewritten; now stated in the changeset); state patch reports a field that already holds the requested value as updated, as state update has since #4488 (previously failed, for every field, also on next); and record-session no longer lists a field it wrote in place under skipped as unclosed_fence.

The continuation scan also treats a fence or sibling field indented one to three spaces as the end of a value (four spaces is indented code, already an end), and a sibling label may carry ., (, ) and /. An indented bullet stays a continuation on purpose, so the scan errs toward skipping the field rather than separating a note from the field it annotates.

🤖 Generated with Claude Code

…lestone stops gaining an escape layer

getMilestoneInfo and the two extractCurrentMilestone readers took
STATE.md's `milestone:` from a raw line regex, so a value YAML must quote
(one containing a literal `"`) came back as its on-disk text, quotes and
backslashes included, and was written to state.json one escape layer
deep. readStateMilestoneScalar reads the value through
rawFrontmatterField (FAILSAFE, so `1.0` stays text), falling back to the
line read only when no parseable frontmatter key exists.

The issue's second half (record-session leaving a wrapped value's tail
behind) is not changed here: whether unseparated text below a field is
part of its value is a policy question for the maintainer.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Full local suite on the PR head (Node 24.19.0, macOS, TMPDIR unset, GSD_STRICT_LIVE_CONFIG_GUARD=1): 44,356 tests — 44,320 pass, 2 fail, the rest skipped.

Both failures reproduce identically on a clean upstream/next checkout (13d37238b) on the same machine, with this branch absent, so they are environmental here and not from this change:

  • runtime-launcher-parity — (K2) end-to-end: the resolved local tool honors git.allow_default_branch_commits (#4834)
  • effort-local-install — #4988: installed local CLI syncs and reads its own Claude agents (/tmp vs /private/tmp path comparison)

npm run lint:ci exits 0.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — fix(#4998): read the decoded milestone scalar, so a quoted milestone stops gaining an escape layer

Summary

Routes the three ^milestone: raw-line readers in src/roadmap-parser.cts through the frontmatter parser's decoded value (rawFrontmatterField, FAILSAFE) via new readStateMilestoneScalar. Reviewed head f5ae1591f9e4613840cada490f04b9926fec879e.

Classification & gate compliance

  • Issue #4998 is OPEN, confirmed-bug (bug-fix track). Title conventional, branch fix/4998-... ok, changeset present (Fixed, pr: 5234, body format ok), CHANGELOG untouched, not a draft.
  • Gate failure: body says Fixes #4998 but the PR states it fixes the first half only. See Blocker 1.

Functional checklist / root-cause verdict

Half 1 (milestone gains an escape layer per write): root-cause fix. The readers captured YAML source instead of the decoded value; they now use the existing frontmatter owner, with no parallel parser. The regression tests would fail on base (raw read returns the quoted/escaped text; readStateMilestoneScalar does not exist on base).
Issue acceptance checklist:

  • milestone not re-escaped per write
  • record-session orphaned tail line of a wrapped value (issue section 2)
  • stopped_at "derive this" note replaced by a timestamp
  • spurious **Stopped at:** line

Blast radius

codegraph_impact: getMilestoneInfo 29 affected symbols (cmdCommit, cmdProgressRender, cmdStats, six init.cts commands, milestone.cts archive/phases clear, health-diagnostic repair); extractCurrentMilestoneScoped 34 affected (extractCurrentMilestone, getRoadmapPhaseInternal, getMilestonePhaseFilter, routeRoadmapCommand). New export readStateMilestoneScalar. Risk: medium (wide fan-in, narrow semantic change).

Co-change completeness

No docs, CONTEXT.md or INVENTORY change is required for a Fixed change. No parallel surface found beyond the warn-only read in src/milestone.cts.

CodeGraph Evidence

  • codegraph_impact target getMilestoneInfo: 29 affected symbols (above).
  • codegraph_impact target extractCurrentMilestoneScoped: 34 affected symbols.
  • codegraph_node target rawFrontmatterField (src/frontmatter.cts:868): returns {value} from the decoded FAILSAFE parse, or null when unterminated/unparseable, so the line-read fallback is reachable.
  • codegraph_callers target readStateMilestoneScalar: absent from the base index (new in PR); its call sites are the three edited readers.
    Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)

CI at head f5ae1591f9e4613840cada490f04b9926fec879e: 42 check runs, none failing or pending, 0 action_required runs, behind next by 0 (clean rebase). mergeStateStatus BLOCKED on required review only.

Findings

Blocker

  1. (judgment, gate) PR body Fixes #4998 closes an issue whose second half (record-session damage: orphaned tail line, stopped_at replacement, added **Stopped at:** line) is unfixed and acknowledged as such. Unmet acceptance criteria are release blockers. Implement the second half here, or split it into its own tracked issue and stop closing #4998 with this PR. The policy question in the body is a maintainer decision to be answered in chat, not in the PR.

Minor
2. (judgment) src/roadmap-parser.cts readStateMilestoneScalar (~line 1016) is a parser-adjacent reader with three hand-picked examples (tests/roadmap-parser.test.cjs:4090-4101); add one discriminating fast-check property (decoded value round-trips through YAML quoting).
3. (judgment) Untested new behavior: trailing # comment stripping (v1.0 # note -> v1.0) and the non-string-frontmatter-key fallback. The three changed call sites (extractCurrentMilestoneScoped, getMilestoneInfo, currentMilestoneRawRanges) are exercised only end to end via state.json; add direct assertions.

Nit
4. Policy question embedded in the PR body; local full-suite failures dismissed as environmental (CI is green on head, no action).
5. Head contains a merge-of-next commit (moot under squash).

Verdict

Request changes (Blocker 1; Minors 2-3). No rebase needed (behind_by 0).

@trek-e trek-e added the review: changes requested PR reviewed — changes required before merge label Oct 7, 2026
…ree call sites

Review findings 2-3:
- fast-check property: any value written through escapeDoubleQuotedScalar
  (the writer's escaper) reads back byte-identical
- trailing `# comment` stripping and the non-string-value fallback to the
  legacy line read are asserted directly
- getMilestoneInfo, extractCurrentMilestoneScoped and
  currentMilestoneRawRanges each select the right milestone for a quoted
  or commented `milestone:` spelling

Six of the eight open-gsd#4998 tests fail when the reader is mutated back to the
legacy `^milestone:` line read; the two fallback tests pin unchanged
behavior by design.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
behruznassre and others added 3 commits October 6, 2026 21:49
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…stone-escaping-session-continuity

# Conflicts:
#	tests/state.test.cjs
@behruznassre

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed:

Merge. The branch had gone DIRTY against next because #4935 (the C2 StateWriteIntent executor) and this PR both appended a top-level describe block at the end of tests/state.test.cjs. I merged next and kept both blocks. Each runs and passes on its own (--test-name-pattern, 25/25).

Codex whole-PR review. No Blocker or Major. One Minor is noted, not acted on: classifyMilestoneScalar strips # … before its null check. A decoded value such as "null # label" (quoted, so a literal string in YAML) is therefore classified as an explicit null. That limitation already applies to every existing decoded-frontmatter consumer of the classifier. This PR just routes three more readers through it, and only for a value that's literally null/~ followed by # …. Fixing the classifier itself belongs in a separate change.

A second whole-PR Codex pass after the next merge found nothing at any severity. It reviewed all six newly merged commits (none touches roadmap-parser.cts, frontmatter.cts, the classifier or state.json publication) and checked that the conflict resolution kept both test blocks intact as separate top-level describes.

Verified on the new head (macOS, Node 24.19.0, TMPDIR unset): npm run lint:ci exit 0; full npm test 44,382 pass, 2 fail. The two failures are runtime-launcher-parity K2 and effort-local-install #4988; both fail the same way on a clean next checkout on this Mac (/tmp vs /private/tmp), and CI has them green.

@behruznassre
behruznassre requested a review from trek-e October 7, 2026 06:52
@trek-e

trek-e commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Maintainer decision on the question in the PR body: implement both halves in this PR. The second half of #4998 (record-session orphaned tail line, the stopped_at replacement, and the spurious Stopped-at line) is acceptance scope, so the PR must fix it before merge. The closing keyword stays. Blocker 1 and the two Minors from the latest review remain open until then.

behruznassre and others added 4 commits October 7, 2026 06:32
…nserts in the template's shape

The second half of open-gsd#4998, on the maintainer's direction to fix both halves
in open-gsd#5234:

- A single-line write to a field whose value wraps rewrote only the first
  line and orphaned the rest. Which lines continue a value and which are a
  separate note directly below it cannot be told apart, so record-session
  now leaves such a field untouched, names it under `skipped`
  (reason `wrapped_value`, with the continuation), and writes a stderr
  warning saying how to fix the document. The check is located from the
  edit itself (editedLineContinuation), so it reads the occurrence the
  writer actually touched, not the first label-regex match.
- A missing field is inserted in the section's own spelling (plain vs bold)
  and template order, beside its sibling and below that sibling's own
  wrapped lines, instead of a bold `**Stopped at:**` line right under the
  heading. A canonical `## Session` that holds a skipped wrapped field gets
  the same insert instead of the whole-section rewrite, which would delete it.
- With no body line, the frontmatter `stopped_at` is the only copy of the
  record; the write re-derives it, so it is now reported in replacedRecord.

The open-gsd#4763 wrapped-record test pinned the old first-line rewrite through its
replacedRecord payload; it now asserts the record is left whole and reported
under `skipped`.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…lands outside examples

Codex whole-PR review of the record-session half (all reproduced against the
previous build before changing anything):

- A canonical `## Session` missing a field was still replaced wholesale with
  three canonical lines, deleting any note or wrapped value in it (and
  resetting an authored Resume file to None). It now gets the same in-place
  insert as `## Session Continuity`; the whole-section rewrite is gone.
- With no session heading, the appended section carried a field already
  skipped as wrapped, with the new value — so it was both `skipped` and, via
  the sync, effectively replaced. The scaffold now omits skipped fields.
- A fenced example of a sibling label could be the insertion anchor, putting
  the real field inside the code block. Fenced lines are never anchors.
- A `## Session Continuity` heading at EOF with no newline had the field
  glued onto it while reporting success. It is now left alone (recorded:false).
- `**Label**:` siblings were not recognized as anchors. They are; the
  inserted line still uses `**Label:**`, the spelling stateExtractField reads.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…er and never crosses a fence

Second Codex whole-PR pass: the hand-rolled fence toggle treated an inner
``` inside a ```` example as its close, so a sibling label in the example
could still anchor the insert. insertMissingFields now builds its mask from
scanFencedBlocks (CommonMark delimiter char, run length and indent rules),
and the move below an anchor's wrapped lines stops at the first fenced line,
which the continuation classifier does not recognize when indented.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Following your direction, both halves of #4998 are now in this PR. The body is back to Fixes #4998, the changeset covers both halves, and I've closed #5246 as superseded.

Policy for the wrapped-field case. You didn't pick between the options I listed on #4998, so I chose (b): a field whose value wraps onto the next line is left whole. A line that continues the value and a separate note written directly below it look identical, and (a) would delete the note in the second case. record-session lists such a field under a new skipped key ({ field, reason: "wrapped_value", continuation }), still writes the other fields, and prints a stderr warning saying how to fix the document. If you'd rather have (a), tell me and I'll switch.

The three record-session observations:

  1. Orphaned tail — fixed as above. The check reads the line the write actually changes (editedLineContinuation), not the first label-regex match.
  2. stopped_at "derive this" note replaced by a timestamp — I couldn't reproduce a timestamp in stopped_at. Every shipped caller passes descriptive --stopped-at text, and the command writes its timestamp only to Last session / Last Date. A displaced body note has been reported in replacedRecord since bug(state): record-session silently overwrites a concurrent session's Stopped-at record, and add-decision/advance-plan resolve the phase from the global pointer instead of the caller (1.14.0) #4763. The remaining silent case, a note kept only in frontmatter, is now reported too.
  3. Spurious **Stopped at:** line — a missing field is now inserted beside its sibling in template order, plain beside plain, bold otherwise.

One existing test changed. #4763's "a wrapped multi-line Stopped At record is surfaced whole" pinned the old first-line rewrite through its replacedRecord payload. It now asserts the record is left whole and listed under skipped. That's the payload change listed under Breaking changes.

One behavior removed. A canonical ## Session missing a field was replaced wholesale with three canonical lines. That deleted any note or wrapped value in it and reset an authored Resume file to None. It now gets the same in-place insert as ## Session Continuity. No existing test pinned the old shape.

Codex whole-PR review, two rounds (my cap for one update):

  • Round 1 found five Majors and a Minor, all in my first draft of this half. Each one reproduced against that build, and each is fixed in c741fb4d2 with a test:

    • the canonical rewrite deleting a note
    • the appended section re-adding a skipped field with the new value
    • a fenced example used as the insertion anchor
    • a heading at EOF with no newline glued to the inserted field
    • **Label**: siblings not recognized as anchors
  • Round 2 found the fence handling still wrong for nested fences. It now uses scanFencedBlocks and never inserts across a fence (2dd5015ed).

  • Four more findings are noted in the body and not acted on. Each needs an unusual STATE.md shape, and each comes from an existing writer/reader/classifier mismatch rather than this PR:

    • multi-line caller values through the plain writer
    • an updated **Stopped at**: line that the reader can't read
    • indented fences or fields read as continuations, which skip rather than lose data
    • a headingless session with a frontmatter stopped_at

    I can file any of them separately.

Verified on the new head (macOS, Node 24.19.0, TMPDIR unset): npm run lint:ci exit 0; full npm test 44,402 pass, 2 fail. The two failures are runtime-launcher-parity K2 and effort-local-install #4988; both fail the same way on a clean next checkout on this Mac (/tmp vs /private/tmp), and CI has them green. The body lists the fail-first evidence for each new test: a mutation of the compiled module, or a probe of the previous build that showed the defect.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated maintainer re-review of head 1a5ecb8e06. Zero-tolerance policy: every finding at any severity must be resolved before approval.

Re-review PR #5234 — fix(#4998) milestone decode + record-session (head 1a5ecb8)

Summary

Head 1a5ecb8 (fresh gh pr view this turn): mergeable=MERGEABLE, mergeStateStatus=BLOCKED, reviewDecision=CHANGES_REQUESTED, not draft, label "review: changes requested". Behind origin/next by 0 (merge-base ed0f3fd = next tip), ahead 12. All 42 check runs SUCCESS/SKIPPED (Stryker skipped, Required tests SUCCESS). BLOCKED = stale CHANGES_REQUESTED review (on f5ae159) only; no conflict, not behind.
Delta: both halves of #4998 now implemented (milestone decode via readStateMilestoneScalar; record-session: wrapped-field skip with skipped payload, sibling-aware insert, canonical ## Session in-place insert, frontmatter stopped_at in replacedRecord).

Prior findings status (review of 2026-10-07, f5ae159)

  • Blocker 1 (second half of #4998 unfixed while Fixes #4998): RESOLVED in scope (src/state.cts:2111-2175 insertMissingFields, :1974-1985 skipWrapped, state-document.cts:393 editedLineContinuation). Caveats under Findings 1-2.
  • Minor 2 (fast-check property for readStateMilestoneScalar): RESOLVED (tests/roadmap-parser.test.cjs, fc.property over grapheme strings through escapeDoubleQuotedScalar).
  • Minor 3 (comment stripping, non-string fallback, 3 call sites direct asserts): RESOLVED (3629188 tests).
  • Nit 4 (policy question in body): RESOLVED. Nit 5 (merge commit): moot under squash.

Acceptance checklist (issue #4998, verbatim)

  • "milestone is re-escaped on every state.* write" — fixed at root (decoded value through frontmatter owner); ten-writes test.
  • [~] "STATE.md's front matter grows the same way" — author could not reproduce on next; claims only state.json. Unverified-by-repro; judgment.
  • [~] record-session "replaces a stopped_at 'derive this' note with a literal timestamp" — not reproduced; body-line displacement already reported (#4763); frontmatter-only now reported in replacedRecord. Value is still replaced (reported, not preserved).
  • orphaned tail of wrapped Session Continuity field — fixed by leaving field whole + skipped. Note: Expected clause "A write replaces the parsed value/node, never a text span" is met only by refusing to write, not by replacing the whole value (policy (b), maintainer did not object).
  • spurious **Stopped at:** line — insert now plain beside plain sibling.

Findings

  1. MAJOR (deterministic, rule: CLAUDE.md "No Deferrals"; RULESET.PR-SCOPE one-concern is overridden by inline-fix rule) — PR body "Noted, not fixed here" lists four defects in the touched path and offers to file separately: (a) plain-rung stateReplaceField writes multi-line caller value; (b) **Stopped at**: updated but unreadable by stateExtractField, sync keeps old frontmatter; (c) stateFieldContinuation treats indented fence / indented **Field:** as continuation so field is skipped; (d) headingless session with wrapped field + frontmatter stopped_at: appended ## Session omits the skipped field and the sync can drop frontmatter stopped_at (data loss, new path at src/state.cts ~2188-2205 scaffoldFields). Fix inline or ask the blocking question in chat; recording in PR body is a silent defer. (d) and (b) are the most serious (silent state loss).
  2. MAJOR (judgment) src/state-document.cts continuationLineCount / STATE_SIBLING_FIELD_LINE_RE (col-0 only): indented sibling fields read as continuations, so record-session silently skips legitimate fields (warns on stderr but recorded can be false). Same root as 1(c); add recognizing indented fields/fences or document and test the exact boundary.
  3. MINOR (judgment, RULESET.TESTS property-based) — insertMissingFields (src/state.cts:2117-2170) and editedLineContinuation are parser-adjacent with only example tests (tests/state-document.test.cjs +35 lines, none fast-check). Add a discriminating property: for any generated section, insert keeps all pre-existing lines (multiset-subsequence) and never lands inside a fence; boundary for continuation-run length 0/1/2.
  4. MINOR (deterministic) — user-visible payload contract change (skipped key, recorded:false with skipped, stderr warning) is undocumented: docs/CLI-TOOLS.md:117 documents only invocation. Fixed-type changeset is docs-exempt, but a payload change in a CLI contract warrants a CLI-TOOLS.md note (and Breaking-changes section in PR body admits payload change).
  5. MINOR (judgment) src/state-document.cts:393 editedLineContinuation: before.lastIndexOf('\n', i - 1) with i=0 evaluates from index 0 (edit at byte 0 misattributes the line); unreachable for real docs (frontmatter) but untested at limit-1/limit/limit+1 boundary of first-diff index.
  6. NIT — Emitted-Drift-Ack-Growth trailer: not applicable (no gsd-core/workflows/* touched); no INVENTORY/CONTEXT.md change needed (no new modules/vocabulary).

Verified fine: branch fix/4998-... ok; title conventional; Fixes #4998 (issue open, confirmed-bug); changeset type Fixed, pr:5234, bold lead + symptom body, one fragment, CHANGELOG untouched; no source-grep, no wall-clock; no new subprocess/argv/path surfaces; fence handling reuses scanFencedBlocks; continuation scan shared (continuationLineCount) not copied.
Fail-first: author states mutation evidence per test; not independently executed (node --test forbidden).

Blast radius

codegraph_impact cmdStateRecordSession: 1 symbol (itself; CLI entry). Prior review: getMilestoneInfo 29, extractCurrentMilestoneScoped 34 affected (medium risk, narrow semantic change). New exports editedLineContinuation, continuationLineCount, readStateMilestoneScalar; stateFieldContinuation refactored (shared with state validate S009) — covered by continuation-grammar-parity per PR body. Memtrace quota exhausted; codegraph used (main-checkout index, branch-new symbols absent).

Co-change gaps

docs/CLI-TOOLS.md payload note (Finding 4). No parity gap found: stateFieldContinuation and editedLineContinuation share continuationLineCount.

Behind-by / rebase

0 behind origin/next (ed0f3fd), no conflicts. BLOCKED solely by CHANGES_REQUESTED review decision; CI all green at 1a5ecb8.

Verdict

Changes requested. Substance largely addresses the prior blocker, but the PR body records four known defects as "noted, not fixed" (zero-deferral violation), including a silent state-loss path, plus missing property test and undocumented payload change.

Memtrace Evidence

Memtrace quota was exhausted for this review run, so the graph-backed checks were NOT executed; this is stated rather than reported as passed:

  • get_impact: not run (quota). Blast radius above comes from codegraph where present.
  • get_symbol_context: not run (quota). Callers/callees were read via codegraph where present.
  • recall_decision: not run (quota). The recorded-decision gate was NOT evaluated; this is not a finding that no decision governs the change.
  • find_code_review_issues: not run (quota). The deterministic AST/YAML/cross-module pass did not execute; findings above are judgment-sourced unless marked deterministic.

Findings above Nit are open, so no further CI is being spent on this head. Address them; the fix push re-triggers CI once.

behruznassre and others added 3 commits October 9, 2026 10:47
Review round on PR open-gsd#5234 (no deferrals):
- (a) every stateReplaceField rung refuses a line-break value, not just
  the bold one; record-session rejects a multi-line --stopped-at /
  --resume-file before writing
- (b) stateExtractField / stateFieldContinuation read `**Label**:` as
  well as `**Label:**`, the two spellings the writer updates
- (c) an indented (1-3 space) fence or sibling field below a field ends
  its continuation instead of making record-session skip the field;
  indented bullets stay continuations on purpose
- (d) with no session heading, missing fields are inserted beside the
  existing ones instead of appending a `## Session` that shadows them
- extract insertMissingSessionFields (pure) with a fast-check property;
  editedLineContinuation's index-0 and blank-line boundaries; document
  the record-session payload in docs/CLI-TOOLS.md

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex round 1 on the previous commit found three regressions in it:
- widening stateExtractField to `**Label**:` let a fenced example
  override the live field (Status, stopped_at; false S009)
- with no session heading, inserting into the whole document anchored
  in frontmatter (corrupting a block scalar) or an archive section

Both reverted. Instead:
- (b) record-session respells the one line it wrote from `**Label**:`
  to `**Label:**` (readableEditedBoldLabel); stateReplaceField keeps the
  author's spelling per its open-gsd#5007 contract
- (d) the appended `## Session` carries the value the sync reads today
  for a field the call is not setting (body line, a wrapped field's
  first line, table row) instead of None or omitting it; this keeps the
  frontmatter stopped_at (dropped at the PR head, reproduced) and an
  authored or table-only Resume file

insertMissingSessionFields loses its requireSibling parameter.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ontmatter

Codex round 2: the headingless carry read the whole file, so a
field-like line inside a frontmatter block scalar could become the
carried Stopped at (and be reported as replaced), while the sync reads
the stripped body. Read the carried value from the body, as the sync
does.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Addressed the 2026-10-09 review. Head is now d839b2e09. All four items the previous body had deferred are fixed in this PR, and that section of the body is replaced by the behavior changes they bring.

Finding 1 (Major), the four deferred defects:

  • (a) Multi-line values: every stateReplaceField rung now returns null for a value containing \r or \n. The plain and pipe-table rungs used to splice such a value in as extra lines; only the bold rung refused. state record-session rejects a multi-line --stopped-at / --resume-file with an error before anything is written.
  • (b) **Stopped at**: unreadable after update: record-session respells the one line it writes from **Label**: to **Label:** (readableEditedBoldLabel), so the value it wrote reaches the frontmatter. stateReplaceField still keeps the author's spelling, as tests/state-document.test.cjs pins for chore(#4906): drain the grandfather allowlist to zero, add the field-shaped .replace() detector and positive-control lint, ratify ADR-4910 — Phase 6 #5007. The reader is not widened: in a first attempt, Codex showed that a widened stateExtractField reads a fenced **Status**: Complete example over a live Status: Executing.
  • (c) and Finding 2, indented fence or sibling read as a continuation: a fence or a sibling field indented 1–3 spaces now ends a value (4 spaces is indented code, which already did). An indented bullet stays a continuation on purpose: the scan errs toward skipping the field, never toward separating a note from the field it annotates. Boundary rows cover 0/1/3/4 spaces and run lengths 0/1/2.
  • (d) No session heading, wrapped field, frontmatter stopped_at: I reproduced the loss at the previous head: the frontmatter stopped_at was dropped, and Stopped At was reported as updated. The appended ## Session now carries, for any field the call is not setting, the value the sync reads today. That's the body line, a wrapped field's first line, or a table row, read from the body and never from frontmatter, instead of None or omitting the field. It also stops an authored or table-only Resume file from being reset to None (same class, pre-existing). My first attempt inserted into the whole document instead; Codex showed it could anchor inside a frontmatter block scalar or an archive section, so it was dropped. Those cases now have regression rows.

Finding 3 (Minor), property test: the session-section insert is now the pure exported insertMissingSessionFields. A fast-check property checks that every pre-existing line survives in order, exactly the needed fields are added, and none lands inside a fenced block.

Finding 4 (Minor), payload contract: docs/CLI-TOOLS.md now documents record-session's single-line values, updated, replacedRecord, skipped ({field, reason: "wrapped_value", continuation}) and recorded: false.

Finding 5 (Minor), editedLineContinuation boundary: first-diff index 0, and the indices around a line start (limit-1/limit/limit+1), are pinned. The i = 0 case and the table-row check now look at the edited line only. Neither change alters a return value on any reachable input, which matches your "unreachable" note. The rows pin the boundary.

Codex reviewed the whole PR twice.

  • Round 1: found the regressions in my first attempts at (b) and (d) described above.
  • Round 2: found the carry reading frontmatter, fixed and pinned.

It also measured one input I'm not changing. A table cell containing a pasted U+2028/U+2029 separator, carried into the appended bold line, reads back up to the separator. That truncation already exists for every single-line field on next: stateExtractField('**Stopped at:** Phase 2
handoff') and its plain form both return Phase 2, because JS . excludes those separators. Only the table rung keeps the whole value.

Gates on this head: lint:ci green; full npm test green apart from runtime-launcher-parity K2 and effort-local-install, which reproduce on clean next on this machine (environmental; CI does not see them).

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — fix(#4998): read the decoded milestone scalar, so a quoted milestone stops gaining an escape layer

Summary

Re-review at head 299f4053d57ecf4472c89d6b2ae17a08c72c1220 (gh pr view this turn: mergeable MERGEABLE, mergeStateStatus BEHIND, reviewDecision CHANGES_REQUESTED). The last trek-e review (2026-10-17T15:07Z) was at d4d661a61be0d370b2772a191f31a37a3e3d618c and asked for Minor 1 (mutation score below 80% on the touched modules) and Nit 1 (stale Testing section). Delta since that review: a2d80b919 (in-process tests, tests/state-document.test.cjs +113), 0633ec56c (changeset + docs/CLI-TOOLS.md wording), 299f4053d (merge of next). Both prior items are addressed in the diff. Two things block approval: the matrix on this head is RED, and the fence-protection claim is narrower in the code than in the PR body (Major 1 below).

Classification & gate compliance

  • #4998 labels: bug, confirmed-bug (bug-fix track). Title fix(#4998): ... conventional; branch fix/4998-state-milestone-escaping-session-continuity; not a draft; PR body carries Fixes #4998 and follows the Fix PR template (all sections present; Platforms row checks macOS, Windows, Linux).
  • Scope: trek-e's 2026-10-07 comment directs both halves of #4998 into this PR, so the milestone half and the record-session half are in scope. The diff also carries the seam line-break set (planning-document.cts), add-roadmap-evolution flattening and a frontmatter-writer escape; all were rulings from the 2026-10-14 review (Major 1), not drift.
  • Changeset .changeset/sharp-pandas-squeak.md: type Fixed, pr: 5234, bold lead. CHANGELOG.md untouched. Fixed is docs-exempt, but docs/CLI-TOOLS.md and CONTEXT.md are updated anyway.
  • No references/ or workflows/ files added: no INVENTORY drift.
  • Not behind-clean: 8 commits behind origin/next (see Finding Minor 3).

Must-Have Acceptance Checklist (issue #4998; the triage Agent Brief states the criteria verbatim)

  • [PASS] "Given a milestone name containing a double quote, ten consecutive state.* writes leave the stored milestone value unchanged (no escape-depth growth)": readStateMilestoneScalar (decoded value via rawFrontmatterField) feeds getMilestoneInfo, extractCurrentMilestoneScoped, currentMilestoneRawRanges; ten-writes end-to-end test and fast-check round-trip property present in the diff.
  • [PASS] "Given a ## Session Continuity field whose existing value spans two or more lines, state.record-session leaves no orphaned continuation-line fragment": a wrapped field is left whole and listed under skipped (wrapped_value), with a stderr warning (skipWrapped, editedLineContinuation, src/state-document.cts:428).
  • [PASS, as narrowed by the author] issue body: spurious **Stopped at:** line (inserted beside its sibling in the section's own spelling, insertMissingSessionFields, src/state-document.cts:650) and stopped_at note replaced (a frontmatter-only stopped_at is now reported in replacedRecord; the author could not reproduce a timestamp landing in stopped_at).
  • [PASS] "Expected: a write replaces the parsed value/node, never a text span": satisfied for the milestone reader and the record-session writers; see Major 1 for the one remaining text-span writer on a session field.

Prior findings (trek-e 2026-10-17, head d4d661a) status at this head

Prior finding Status Evidence
Minor 1: mutation score below 80% on touched modules ADDRESSED, author-reported only a2d80b919 adds in-process rows for the in-line CR/U+2028/U+2029 branch of editedLineContinuation, the document-scope branch of maskFencedLines/markdownFencedBlocks, an unclosed frontmatter block, CRLF, and fieldContinuationEnd. I walked each new assertion against the code and each holds. The 85.94% / 86.24% Stryker figures are the author's local run; I did not re-run it and the modules are still outside scripts/mutation-matrix.cjs COVERED, so CI does not measure them (Nit 2).
Nit 1: stale Testing section ADDRESSED partly Body now reports the 0633ec56c suite; head is the later merge 299f4053d (Nit 1 below).

Root-cause / functional verdict

Root cause, one sentence: three milestone readers matched ^milestone: against raw file text and so passed YAML source on as a value, and the record-session writers were single-line with no continuation awareness, a hard-coded bold spelling and position, and a whole-section rewrite. The fix is at the reader and writer, through one owner each (rawFrontmatterField, scanFencedBlocks/FenceScope, FIELD_LINE_BREAK_RE), with no parallel parser. Root-cause fix, not symptom handling, for everything the PR claims except the gap in Major 1.

Verified on disk at this head:

  • editedLineStart is the first differing byte; i === 0 is handled explicitly (the lastIndexOf clamp comment is accurate).
  • writeOutsideFences returns null when the edit touches a fenced line or cannot be carried back through a re-mask, so there is no partial write.
  • fieldContinuationEnd (#5446/#4945) is shared, not copied.
  • Security lens: no new file path, argv or shell surface in the delta. The inputs are --stopped-at/--resume-file/state update|patch values; all four line-break characters (CR, LF, U+2028, U+2029) are refused by name before any write, and add-roadmap-evolution flattens them. No injection or traversal finding.

Blast radius

CodeGraph (index built in a detached worktree of refs/prrev/5234): editedLineContinuation has 3 callers in src/state.cts; stateFieldContinuation 3 callers in src/state.cts; stateReplaceFieldInSession 6 callers in src/state-transition.cts; cmdStatePatch/cmdStateUpdate each 1 caller (src/state-command-router.cts). New exports are additive (readStateMilestoneScalar, editedLineContinuation, readableEditedBoldLabel, insertMissingSessionFields, maskFencedLines, markdownFencedBlocks, writeOutsideFences, readableBoldLabels, FIELD_LINE_BREAK_RE, FenceScope); none removed. Behavior changes: stateReplaceField returns null on any line break on every rung; state update/patch/record-session refuse a multi-line value; record-session payload gains skipped.

Co-change completeness

Present: the six src/ files, a test file for each, CONTEXT.md, docs/CLI-TOOLS.md, the changeset. Absent and not required: references/ or workflows/ additions. Not present and relevant: the updateCore/patchCore session-field routing for the Resume File (Major 1). next has since changed src/state.cts, docs/CLI-TOOLS.md and tests/state.test.cjs (#5314); this PR touches the same three files.

Test quality

  • Persisted artifact asserted (STATE.md bytes and state.json), not only the command echo.
  • Properties present: milestone round-trip, editedLineContinuation, readableEditedBoldLabel, insertMissingSessionFields (order/count/no-insert-in-fence/CRLF), engine-derived line-break set over every BMP code point.
  • Boundaries: first-diff index 0 and around a line start, each of the four break characters at each position, 0/1/3/4-space indentation.
  • No Date.now, performance.now or elapsed assertion added; no readFileSync of a src/ path in the added tests.
  • Fail-first is the author's statement per row (compiled-module revert); my reasoning from the diff agrees for the rows I walked (document-scope mask, section-scope contrast, unclosed-frontmatter, fieldContinuationEnd x4, in-line break x5).
  • Gap: no test pins state update "Resume File" against a fenced example (Major 1).

CI (head 299f405, latest-per-name, collapsed)

RED. Non-success runs, verbatim:

  • failure conformance test (macos-latest, 24) (job 117502207510, run 39068881278)
  • failure Required tests (job 117507911026)

Failing assertion from the macOS job log, verbatim: not ok 3 - property: every non-boolean CLI value is rejected without modifying config, suite enhancement #4570 config contract — planner stall detection has a typed default-on opt-out, location /Users/runner/work/gsd-core/gsd-core/tests/plan-phase-stall-detection.test.cjs:518:3, Property failed after 4 tests, { seed: -1201433058, path: "3", endOnFailure: true }, Counterexample: ["-h"], # fail 0 for every other file shown.
Attribution: tests/plan-phase-stall-detection.test.cjs is not in this PR's changed-file list (15 files, all state/planning/roadmap/frontmatter plus docs), and the test drives a config-set CLI value, not a STATE.md path. That points to base drift or a seed-dependent property, but I do not classify it benign: the maintainer decides. Latest-per-name filter shows no other red, and no action_required runs for this head sha. next Tests on 274f8cae1 is still in progress at review time.

Memtrace Evidence

  • get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). Substitute: CodeGraph caller counts above plus a manual read of git diff origin/next...refs/prrev/5234 (15 files, +1960/-146).
  • get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). Substitute: CodeGraph codegraph_explore over a detached worktree of the PR head for cmdStateRecordSession, editedLineContinuation, insertMissingSessionFields, writeOutsideFences, stateReplaceFieldInSession, updateCore, SESSION_SCOPED_KEYS (verbatim source and callers).
  • recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). The recorded-decision gate did not execute. Substitute: ADR-4910 (planning-document seam) and ADR-3180 section 7.7 as cited in CONTEXT.md; maintainer rulings read from the PR thread.
  • find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). The deterministic review pass did not run; findings below are manual-diff judgment unless marked deterministic.

Findings

Blocker

  1. [deterministic fact] CI is red on the head: conformance test (macos-latest, 24) and Required tests failed (details and verbatim assertion above). A PR whose required matrix has not passed is not approvable. Action: re-run on a rebased head (Minor 3) and report the result; if the property fails again, the maintainer decides ownership (the test is outside this PR's files).

Major
2. [judgment, confirmed by the author's own admission in docs/CLI-TOOLS.md:132 and by the code] state update / state patch of Resume File (and any session field other than Stopped At / Paused At) still use a whole-body stateReplaceField, which can rewrite a fenced example line above the live field. SESSION_SCOPED_KEYS = new Set(['stopped_at','paused_at']) at src/state-transition.cts:309 is the only gate to stateReplaceFieldInSession; updateCore falls through to stateReplaceField(body, ...) at src/state-transition.cts:2952. The author states in the 2026-10-17 comment that state update "Resume File" and state patch --"Resume File" "both rewrite a fenced Resume file: example above the live line." That is the same defect class this PR fixes for record-session and for Stopped At / Paused At (headline claim: "Fenced examples are never session fields"), and CLAUDE.md "No Deferrals" bars documenting a defect in the changed area instead of fixing it. The 0633ec56c change narrowed the docs and changeset wording to match the code; it did not close the gap. Fix: route session-section labels (Resume File, Last session, Last Date) in updateCore and patchCore through stateReplaceFieldInSession/writeOutsideFences (a session label set, not only the frontmatter-keyed SESSION_SCOPED_KEYS), add a CLI row asserting the fenced example is byte-identical and the live line changes, and drop the sentence at docs/CLI-TOOLS.md:132 that excuses the gap.

Minor
3. [deterministic fact] Branch is BEHIND origin/next by 8 commits (git rev-list --count refs/prrev/5234..origin/next). git merge-tree --write-tree is clean, but #5314 (274f8cae13, fix #4926) changed src/state.cts (including cmdStateAddRoadmapEvolution, which this PR also edits), docs/CLI-TOOLS.md and tests/state.test.cjs. A clean textual merge does not show the state suites still pass together. Fix: git fetch origin && git rebase origin/next (or merge), then re-run state*, state-document and continuation-grammar-parity.
4. [deterministic fact] The PR body's Behavior section over-claims against the shipped code: it says fenced example lines "are no longer read or written as session fields by any of the paths above" and that state patch "writes a session field inside the session section, as state update already did". Per Major 2, only Stopped At / Paused At are covered for update/patch. Update the body once Major 2 is resolved (or correct it if Major 2 is resolved by narrowing).

Nit
5. [deterministic fact] The PR body's Testing section cites the full-suite result on 0633ec56c; the head is now 299f4053d and will move again on rebase. Refresh the counts and the pre-existing failure list on the final head.
6. [judgment] The scoped Stryker figures (85.94% overall, 86.24% state-document.cjs) are an author-local run I did not reproduce, and state-document.cjs / planning-document.cjs remain outside scripts/mutation-matrix.cjs COVERED, so CI never measures them. The author left the set unchanged as a repo-level decision; the maintainer should confirm that is accepted.

Verdict

Changes requested (zero tolerance: Blocker 1, Major 2, Minor 3-4, Nit 5-6). Substance is otherwise clean: Minor 1 and Nit 1 from the previous review are addressed and no security finding was raised. Re-review after the rebase, the Resume File routing fix and a green matrix.

behruznassre and others added 4 commits October 18, 2026 16:44
…de the session section

Resume File, Last session and Last Date have no frontmatter key, so
state update / state patch wrote them with the whole-body field writer: a
fenced example above the live line, or an archive section's copy, was the
line rewritten. They now go through the session writer as Stopped At and
Paused At do, matching where state-snapshot reads them.

The post-write check read the field with a whole-body extract too, so even a
correct write reported "not found" while the unchanged fenced example sat
above it; it now reads a session field from the session section with fences
blanked.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e writers that write it there

The previous commit made the post-write check read every session field from
the session section, but record-session's writer is document-wide (open-gsd#4763): with
an archive above ## Session it rewrote the archive and then reported
"no bytes changed". The session-scoped read is now an explicit opt-in that
state update and state patch pass; every other caller reads as before.

state update / state patch of a session field that already holds the value
reported it "not found": the session writer returns the content unchanged on a
miss and on a same-value match alike. A present field is now told apart with
the existing sessionSourceExists probe (open-gsd#4488).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nced examples blanked

record-session writes outside fences, but its post-write check read the field
document-wide with fences included, so with an example above the live line it
read the unchanged example: the live fields were written and the call reported
"no bytes changed", dropping the displaced value from replacedRecord. No
session writer changes a fenced line, so a session field is now read with
fences blanked for every caller (still document-wide unless the caller writes
in the session section).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Thanks for review 5563202317. Here is how each finding is handled at f97afe827.

Blocker 1 (red macOS matrix). The failing assertion is tests/plan-phase-stall-detection.test.cjs:518, the property "every non-boolean CLI value is rejected without modifying config", with counterexample ["-h"]. That file is outside this PR. I filed it as #5507 (a seed-dependent property: the generator can produce -h, which the CLI reads as a flag). It is still open. This push re-runs the matrix on the merged head, and I will report the result here.

Major 2 (Resume File and the other session fields). state update / state patch now write every session field inside the session section: Stopped At, Paused At, Resume File, Last session and Last Date. A fenced example above the live line is never the line written. Nor is a copy in another section such as ## Session Continuity Archive.

  • SESSION_BODY_FIELDS (src/state-transition.cts) adds the three labels with no frontmatter key. They go through stateReplaceFieldInSession, as Stopped At / Paused At already did, which matches where state-snapshot reads them.
  • The post-write check also had to change. reconcileReportedFields read the field with a whole-body stateExtractField, so it read the unchanged fenced example first, and a correct write reported "not found". With the new opt-in (sessionScope, passed only by update and patch), it reads a session field from the session section with fences blanked.
  • The sentence in docs/CLI-TOOLS.md that excused the gap is gone. The paragraph now lists the five fields. The changeset says the same.

Rows in tests/state.test.cjs:

  • update and patch × Resume File, Last session, Last Date: the fenced example is byte-identical, the live line changes, and the call reports the update;
  • an archive copy above ## Session is left alone.

All seven fail on the previous head.

One behavior change follows from this. A STATE.md with a Resume file: line but no session section is no longer written by update/patch; it reports "not found". That matches state-snapshot, which reads the field only from the session section, and it is how Stopped At already behaved. No shipped workflow calls update/patch on these fields. record-session still appends a session section in that case.

Codex whole-PR review, two rounds.

  • Round 1 found two defects in my first cut, both fixed and pinned by rows that fail on the previous head:
  • Round 2 found one more case, from an earlier round of this PR rather than this one. record-session writes outside fences, but its post-write check read the field document-wide with fences included. With an example above the live line, the live fields were written while the call said "no bytes changed" and dropped the displaced value from replacedRecord. No session writer changes a fenced line, so the check now reads session fields with fences blanked for every caller. It stays document-wide unless the caller writes in the session section. A new row fails on the previous head. This fix did not get a third Codex pass; I keep cross-AI review to two rounds.

Two behaviors are unchanged because they are existing contracts for every field:

Minor 3 (behind next). Merged upstream/next at 8940d2a13, which includes #5314. The state suites ran together in the full suite below.

Minor 4 (PR body over-claimed). The Behavior section now names the five fields, and the closing paragraph no longer says "any of the paths above" for fields that weren't covered.

Nit 5 (stale Testing counts). Refreshed on this head.

Nit 6 (COVERED set). state-document.cjs / planning-document.cjs are still outside scripts/mutation-matrix.cjs COVERED. Adding them is your call. If you want them in, I'll add the entries with floors from the first CI measurement, as on #5232.

Validation.

@behruznassre

Copy link
Copy Markdown
Contributor Author

CI result for f97afe827, as promised. Blocker 1: the macOS lane's plan-phase-stall-detection property (#5507) passed this run (ok 3 - property: every non-boolean CLI value is rejected without modifying config). The red lanes (macOS, Windows shard 3/3, ubuntu shard 3/3) each fail on one test outside this PR, install — changeset CLI lands at scripts/changeset/cli.cjs (#935) ("installed cli.cjs can run extract subcommand end-to-end"). next's own Tests run at 168318367 (run 39179559870, the 1.18.0 back-merge) fails the same test on the same lanes, which is what "Base branch health" reports. The branch is merged with next at 8940d2a13, whose Tests run passed. The lanes need a re-run once next is green again.

@behruznassre
behruznassre requested a review from trek-e October 19, 2026 15:50

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — fix(#4998): read the decoded milestone scalar, so a quoted milestone stays correct

Head reviewed: 9303947dc45c7902f8baa5642e2b7a104f0a7c89 (mergeable=MERGEABLE, mergeStateStatus=BEHIND by 5). Prior review was on 299f4053d5; the author replied twice on 2026-10-19 with commits 8b397965b8, 16fc2a30bf, f97afe8270 and merges up to this head. CI at this head: 36 distinct checks all success or skipped; no failing, pending or action_required.

Prior findings

# Finding Status
Blocker 1 CI red (macOS stall-detection property; Required tests) FIXED on this head
Major 2 update/patch of Resume File etc. rewrote the fenced example FIXED (src/state-transition.cts:316 SESSION_BODY_FIELDS, :340-343, :2883-2886 patchCore and :2961-2967 updateCore route through stateReplaceFieldInSession; read side in src/state.cts readStateTarget; docs/CLI-TOOLS.md:142 excuse sentence removed; six "(review Major 2)" rows)
Minor 3 behind next PARTIAL: merged at 9303947dc4, BEHIND by 5 again
Minor 4 PR body over-claim FIXED
Nit 5 stale Testing counts NOT FIXED (body cites f97afe827)
Nit 6 state-document/planning-document outside the mutation-matrix COVERED set OPEN, maintainer decision (the author offered to add entries)

Acceptance criteria of #4998 are satisfied. No new path, argv or shell surface in the delta.

Summary

No Blocker or Major. Three Minors and three Nits remain; changes requested.

Findings

Minor

  1. Behavior change missing from the changeset: state update/state patch of Resume File / Last session / Last Date on a STATE.md with no session section (previously a whole-body replace) now reports "not found". It is stated only in docs/CLI-TOOLS.md:142 and a PR comment; .changeset/sharp-pandas-squeak.md (type Fixed) does not say it. State it in the changeset, or use type Changed with justification.
  2. Nothing in tests/state.test.cjs pins that behavior change: no row for a no-session-section layout, a ## Session Continuity heading (versus the ## Session preference), the bold **Resume file:** label form, or CRLF for update/patch of the three new labels. The documented contract at docs/CLI-TOOLS.md:142 is currently unpinned.
  3. No same-value row for state patch: the patchCore change at src/state-transition.cts:2886 is exercised only through update.

Nit
4. Reader/writer scope asymmetry: sessionFieldScope (src/state.cts:2425-2427) falls back to the whole body when no session section exists, while the writer replaces nothing without one. Harmless today, but it is a second place that must stay in step; add a parity row.
5. PR body Testing section cites f97afe827, not the head; refresh it after the rebase onto next, which is needed because the branch is BEHIND by 5.
6. The changeset body is a single ~2000-character paragraph; the CONTRIBUTING.md format is a bold lead plus a symptom-led explanation.

Memtrace Evidence

The Memtrace monthly quota is exhausted for this run, so none of the graph tools below were executed. These are not-run, not passed; findings come from the isolated manual review only.

  • get_impact: not run (quota exhausted). Blast radius and risk tier were not computed from the graph.
  • get_symbol_context: not run (quota exhausted).
  • recall_decision: not run (quota exhausted). The recorded-decision gate did not execute.
  • find_code_review_issues: not run (quota exhausted). The deterministic review pass did not execute.

Verdict

Changes requested. Every finding above must be resolved before approval; a rebase clears base drift only and retires none of them. This review does not release or re-run CI.

behruznassre and others added 3 commits October 20, 2026 23:08
…in the session-scope contract

trek-e review 5593733001:
- Minor 1: the changeset states the behavior change — with no session
  section, `state update`/`patch` of Resume File, Last session or Last Date
  now report the field not found and leave the document unchanged.
- Minor 2: rows for no session section, `## Session Continuity`, both
  headings, the bold label form and a CRLF document, for update and patch.
- Minor 3: the same-value `state patch` row found that patch reported a
  field already holding the value as failed (as on next, for every field):
  the reconcile drops a field whose bytes did not change. cmdStatePatch now
  counts a matched field whose persisted value is the requested one, as
  cmdStateUpdate's open-gsd#4488 rule does — alone or beside a field that changed.
- Nit 4: writer/reader parity rows against state-snapshot's session read.
- Nit 6: changeset is a bold lead plus a symptom-led paragraph.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… skipped as unclosed_fence

Codex review of this round: with no session heading and a document that
ends inside an unclosed fence, record-session listed every scaffold field
under `skipped` with reason unclosed_fence and warned "did not write" —
including a field it had already replaced in place above (Stopped at: old
-> new), which then appeared under both `updated` and `skipped`. Only
fields not already written are skipped now. Row added; docs and the
changeset's line-break wording qualified (the planning-document writers
refuse a value that would add a line break; a separator-bearing tail kept
unchanged is accepted).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review (5593733001). All findings are addressed on 92a65a162, which also merges next at 7a4b76f68.

# Finding Resolution
Minor 1 Behavior change missing from the changeset 28e335c0d: the changeset now states it. With no ## Session or ## Session Continuity section, state update/state patch of Resume File, Last session or Last Date reports the field not found and leaves the document unchanged. The type stays Fixed. docs/CLI-TOOLS.md says the same.
Minor 2 Contract unpinned 28e335c0d, rows in tests/state.test.cjs tagged (review 5593733001): no session section (3 labels, update and patch, exact payloads, document byte-identical); ## Session Continuity with the bold **Label:** form (update and patch); both headings (## Session written, ## Session Continuity keeps its copy); a CRLF document.
Minor 3 No same-value state patch row The row failed. A same-value state patch reported the field under failed. This is also true on next, for every field: state patch --Status Executing on an already-Executing document reports failed, while state update reports updated: true since #4488. The reconcile only counts fields whose bytes changed. Fix in cmdStatePatch (src/state.cts): a field patchCore matched is reported updated when its persisted value is the requested one. This applies alone or beside a field that did change. A value the write pipeline discarded still differs, so it stays failed. Rows for both cases fail on the commit before the fix.
Nit 4 Reader/writer scope asymmetry Parity rows per layout (## Session, ## Session Continuity, none): the write lands where state-snapshot reads, and with no section neither side takes a line from elsewhere. The whole-body fallback in sessionFieldScope is only reached for a field the writer reported written (reconcileReportedFields(..., updated ? [field] : []), src/state.cts:861 / :728). So with no session section that read never runs for update/patch. The fallback stays for record-session's document-wide writer and the frontmatter sync (#4763).
Nit 5 Stale Testing counts Body refreshed: full suite on 92a65a162, 46,370 tests, 3 failures, all environment-only on this Mac (same three as before; #4142 passes 303/303 with HOME sandboxed).
Nit 6 Changeset format Rewritten as a bold lead plus a paragraph that starts from the symptom.

Codex reviewed the whole PR twice this round.

  • Round 1 found one Minor: record-session on a document with no session heading that ends inside an unclosed fence listed every scaffold field under skipped as unclosed_fence. That included a field it had already replaced in place above the fence, which then appeared under both updated and skipped and got a false "did not write" warning. Fixed in 92a65a162, with a row.
  • Round 1 also found a Nit: the changeset over-stated the planning-document writers' line-break refusal. setFieldValue keeps an unchanged separator-bearing tail, so the wording is now qualified.
  • Round 2 was clean.

Gate on 92a65a162: lint:ci exits 0, and the full suite is as above.

Nit 6 from 5563202317 (whether to add state-document/planning-document to the mutation-matrix COVERED set) is still your call. I can add the entries in this PR or as a follow-up.

@behruznassre
behruznassre requested a review from trek-e October 21, 2026 19:07
@trek-e

trek-e commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Heads-up on a change to how fixes land in next, and what it means for this PR. Nothing here blocks you today; it is how to be ready.

1. No need to keep merging next into your branch. next now uses a GitHub merge queue. Once a PR is approved and green, it is queued, and the queue re-tests it on top of next plus whatever is ahead of it, then merges. "Update branch" is no longer a prerequisite to merging, so please skip it unless you have a real conflict or want a fresh signal.

2. Add a Self-test report (fix PRs). Fix PRs now carry a short ## Self-test report section with a JSON block, in the fix template. It records the failing-first regression test, the commands you ran, your OS/Node, and a passing full-suite run, bound to your final commit sha. It is a head start for the reviewer, not proof; CI on the queue commit is authoritative. The advisory Self-test report check on this PR shows what is missing. Fill it in after your last push (a new push makes the sha stale).

3. Review. The advisory Reviewed or maintainer check is red until someone with write access approves. That is expected and resolves with a review, not with a change from you.

4. CI cost. Windows/macOS lanes now run on the queue, not on every push. If your change is platform-specific, add the full-matrix label to get them on the PR.

Questions are welcome here. Details: docs/branch-protection.md.

Posted at the maintainer's request.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — fix(#4998): read the decoded milestone scalar

Head reviewed: e8cb221eb2 (MERGEABLE, BEHIND next by 10; the merge queue re-tests, a rebase is not requested). CI at this head: all 36 distinct checks success or skipped, none pending or action_required. That statement is about CI only; the findings below are separate and stand.

Prior findings (review 5593733001)

Minor 1 (changeset behavior change), Minor 2 (contract rows), Minor 3 (same-value state patch), Nit 4 (parity rows), Nit 5 (Testing counts), Nit 6 changeset format: FIXED at 28e335c0d2 / 92a65a1627. Nit 6 of 5563202317 (state-document/planning-document outside the mutation-matrix COVERED set) is still open; see Minor 4.

Findings

Major

  1. (judgment, isolated reviewer, reproduced) src/state.cts:~2079-2161 record-session writes Stopped At / Resume File / Last session document-wide, but the frontmatter sync and state snapshot read them session-scoped. When the only live line is outside ## Session and that section lacks the field, the insert-only path no longer rewrites the section; the call reports recorded:true, updated:["Stopped At"] while frontmatter stopped_at keeps the stale value. reconcileReportedFields reads whole-document for these fields, so it confirms the false success. Regression versus base (old code wrote **Stopped at:** NEWVAL into ## Session). Repro STATE.md: # S\n\nprose\n**Stopped at**: old\n## Session\n## Session Continuity, then state record-session --stopped-at NEWVAL. A second repro: **Stopped at:** old above ## Session containing a | Stopped at | old | row and an unclosed ~~~. Fix: write session fields inside the session section as state update does, or insert into the section when the in-place match lies outside it; add a row.

Minor
2. src/state.cts:~2331 (skippedDisclosure / editedLineContinuation): the wrapped continuation (untrusted STATE.md text) is echoed unbounded to stderr and to skipped[].continuation; a 100,000-char continuation emits ~200 KB per call. Truncate it (for example 200 chars) in both outputs; add a boundary row (limit-1, limit, limit+1).
3. src/state.cts:~2290 (unclosed_fence loop): pushes unclosed_fence rows for fields the caller did not ask to write (--stopped-at only yields "did not write Resume file") and duplicates a field already listed as wrapped_value. Filter on the fields actually requested and not already skipped.
4. Nit 6 of 5563202317 is unresolved: state-document and planning-document are not in the mutation-matrix COVERED set while this PR adds most of its logic there. The author offered the entries; add them in this PR.

Nit
5. src/state.cts:~1810 flatNote regex \s*[\r\n

]+\s* is quadratic on a long whitespace run with no break (80k spaces + "x" took 2.6 s). The old regex had the same cost, but this PR rewrote the line; use a linear form.

Blast radius and evidence

Changed symbols: cmdStateRecordSession, cmdStatePatch, cmdStateUpdate, editedLineContinuation, insertMissingSessionFields, setFieldValue, replaceProse, readStateMilestoneScalar, scalarNeedsDoubleQuoting. Co-change files docs/CLI-TOOLS.md, CONTEXT.md and the changeset are present in the diff. Graph-derived blast radius was not computed: see below.

Memtrace Evidence

The indexed graph is stale (branch fix/5165-milestone-progress-rewind, commit 0ce32128; no episodes on next), so graph-derived results are not a clean verdict.

  • get_impact on cmdStateRecordSession: ambiguous across branches (found:false, risk UNKNOWN); no traversal performed. Blast radius not computed from the graph.
  • get_symbol_context: same ambiguity, 0 callers/callees returned; not usable.
  • recall_decision ("record-session session field write STATE.md single-line values"): returned ADR-3408 (STATE.md write path), ADR-4629 (bounded verified writes), ADR-3212 (lexical seam); no verify_intent was run, so nothing is proven held or violated.
  • find_code_review_issues (review_mode: online, graph_mode: off, max_candidates: 100) and find_yaml_rule_matches: 0 issues each. They ran on the changed hunk ranges of src/state.cts, src/state-document.cts, src/state-transition.cts (hunk headers only, not full diff text), so a zero is weak evidence. detect_changes returned empty symbols for all six files (stale graph). The findings above come from the isolated adversarial pass (maintainer-reviewer, reproduced against a built head).

Verdict

Changes requested: 1 Major, 3 Minor, 1 Nit. CI release was not needed (nothing pending). A rebase clears no finding.

behruznassre and others added 2 commits October 21, 2026 22:31
…ession section

trek-e review 5608265301:

- Major 1: record-session replaced the first label match anywhere in the
  document, but the frontmatter sync and state-snapshot read Stopped At,
  Resume File and Last session from the session section. A line outside it
  took the write, the call reported success, and `stopped_at` stayed stale.
  The in-place writes, the replacedRecord capture and the reconcile now use
  the session section when there is one, as `state update` does; a field the
  section lacks is inserted there. Two rows that pinned the old
  document-wide write over an archive section now pin the new contract.
- Minor 2: a wrapped value's continuation is echoed up to 200 characters.
- Minor 3: `unclosed_fence` names only fields the call writes that were not
  already written or skipped; `line_separator` rows are built only when the
  section is appended.
- Minor 4: state-document and planning-document enter the mutation matrix
  with a provisional floor of 50, to be set from this PR's CI shard.
- Nit 5: the note flattening splits and trims instead of a backtracking
  `\s*<break>+\s*` replace (80k spaces: 3982 ms -> 0.1 ms, same output).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…orted blocked by a fence

Codex review of this round: with no session heading and an unclosed fence, a
same-value `--stopped-at` was listed as `unclosed_fence` although it needed no
write. The docs and changeset now say the appended section carries the body's
recorded values.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

All five items from review 5608265301 are addressed in 3c292b31d, plus one Codex finding in 43f1bfa93.

  • Major 1: reproduced both of your layouts on the built head; stopped_at stayed stale while the call reported Stopped At updated. record-session now does all of its in-place work inside the session section (## Session, else ## Session Continuity, located on the fence-masked text), the way state update does: the writes, the replacedRecord capture, and the post-write reconcile. If the section lacks the field, the field is inserted there. The whole document is used only when there is no session section, which is also when the readers read it. Both repros now give frontmatter stopped_at: NEWVAL, and state-snapshot reads it in the first layout. Two existing rows (the bug(state): record-session silently overwrites a concurrent session's Stopped-at record, and add-decision/advance-plan resolve the phase from the global pointer instead of the caller (1.14.0) #4763 archive row and an earlier "review Major 2" row) pinned the old overwrite of an archive line above ## Session. They now pin the new contract: the archive is left as is, and replacedRecord names the live record. The breaking-changes section says so.
    • In your second layout state-snapshot still returns null. Its own session regexes have never read a table-row field (| Stopped at | x |), while the frontmatter sync does. That is the same on next; Codex confirmed it is pre-existing. I left it out of this PR.
  • Minor 2: skipped[].continuation, and so the stderr echo, is cut at 200 characters with a trailing …. Rows cover 199, 200 and 201. The file itself is untouched.
  • Minor 3: unclosed_fence now lists only fields the call writes (Last session always; Stopped at and Resume file when passed) that were not already written in place or skipped for another reason. line_separator rows are built only when the section is actually appended. Codex round 1 found a same-value --stopped-at still listed; fixed with a row.
  • Minor 4: state-document and planning-document are COVERED. The floor is the range minimum (50) for now, because the matrix requires a floor from a CI shard. I'll set floor(measured) - 1 from this push's shards in a follow-up commit. planning-document.test.cjs and frontmatter-fence.test.cjs are extraTests: the first requires the module through a constant the derivation can't follow.
  • Nit 5: the note flattening now splits on line breaks, trims, and joins. On 80k spaces plus x it went from 3982 ms to 0.1 ms, with identical output on 20,000 random strings. There's no timing row, because the no-elapsed-assertion lint forbids one.

Gate on 43f1bfa93: lint:ci exits 0. Full suite: 46,377 pass, 3 fail. These are the same three environment-only failures, and each also fails on a clean next checkout. Codex reviewed the whole PR twice; round 2 found nothing. The Self-test report in the body records the suite as it ran, so its check will show fail_suite_not_passed. The #4998 rows fail on upstream/next 26387bc and pass on the head.

…document shard budget

CI run 39736529871: planning-document scored 75.14%, so its floor is 74
(floor(measured) - 1). The state-document shard was cancelled at the
15-minute default with ~66 minutes left by Stryker's estimate; it gets 120
minutes, and its floor stays provisional until that run measures it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

Mutation floors, follow-up to Minor 4 (b0e97da8c):

  • planning-document scored 75.14% in CI run 39736529871. The floor is now 74 (floor(measured) - 1, as the ratchet step itself asked), and RATCHET_BASELINE is updated to match.
  • The state-document shard was cancelled at the 15-minute default after 506 mutants, with about 66 minutes left by Stryker's own estimate. It now has a 120-minute budget. Its floor stays at the provisional 50 until a run completes; I'll set it from that run. Codex reviewed the PR at this head and found nothing. The gate was unchanged: lint:ci exits 0, and the suite showed the same three environment-only failures. The Self-test report now points at b0e97da8c.

… run

CI run 39742269013 measured state-document at 50.47% in a 94-minute job.
floor(50.47) - 1 = 49 is below the matrix's range minimum, so minScore
stays at 50; timeoutMinutes goes from 120 to 140 (about 1.5x the job).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@behruznassre

Copy link
Copy Markdown
Contributor Author

state-document mutation floor, closing out Minor 4 (9140d3e18):

  • CI run 39742269013 completed the shard: 50.47% in a 94-minute job, passing the floor of 50.
  • floor(50.47) - 1 = 49 is below the matrix's 50 minimum, so minScore stays at 50. The comment and RATCHET_BASELINE now cite the run instead of "provisional".
  • timeoutMinutes is now 140, about 1.5x the measured job (it was 120 before the shard had ever finished).

Lint passed. The full suite and the Self-test report show the same 3 environment-only failures as before. A whole-PR Codex review found nothing blocking.

@behruznassre
behruznassre requested a review from trek-e October 22, 2026 14:31

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review: changes requested PR reviewed — changes required before merge

Projects

None yet

2 participants