Repository navigation
ci(#5104): the raw STATE.md write guard sees wrapped calls and same-function variable targets - #5119
davesienkowski wants to merge 26 commits into
Conversation
trek-e
left a comment
There was a problem hiding this comment.
Summary
Changes requested. Axis 2 of the raw STATE.md write guard now sees wrapped calls and resolves variable targets, but the promised one-line shape is still missed and the new taint rule can flag writes that are not STATE.md.
Classification & gate compliance
- Enhancement track. Linked issue #5104 carries
approved-enhancement(Layer 1 approved in the issue thread).Closes #5104present, conventional titleci(#5104):, not a draft. Governing ADR: ADR-4629 Decision 5 (no allowlist) — honored. - Labels
area: workflow+no-changelog; changeset lint reports no user-facing change, so no changeset is required. Branch is BEHINDnext(MERGEABLE). Required checks (Tests, lint-tests, Required tests, changeset-lint, docs-lint) are SUCCESS at this head; earlier-run Changeset/Docs entries are CANCELLED and superseded by later SUCCESS runs.
Must-Have Acceptance Checklist (issue #5104, Layer 1)
- First argument captured across line breaks, over comment-stripped text.
- Resolve a variable target through same-function assignments with bounded hops — partly met: a same-line assignment-plus-write is missed (Major 1).
-
for…of/ destructuring bindings are unresolved (two binding rows). - Reuses the
nearestPrecedingAssignmentlogic (assignmentLineReshared). - Axis 2 only, no
src/change, no allowlist, no new dependency. - Tests: wrapped, variable, sibling-block binding, negatives, real-tree control.
- Miss 3 is out of scope per the ruling; the PR body says a separate #4629 child covers it but cites no issue number (Nit 2).
Findings
Major
- (judgment + static trace)
scripts/lint-state-write-path-drift.cjsfindRawStateWritescallsresolvesToStatePath(stripped, lineIdx - 1, targetArg); the backward scan starts ABOVE the call line, soconst p = path.join(dir, 'STATE.md'); fs.writeFileSync(p, c);on one line resolves to nothing and is not flagged. The PR body and issue fixture 3 list exactly this one-hop shape as caught. Every F4 positive row puts the assignment on its own line, so the gap is untested. Fix: scan fromlineIdxinclusive, limited to text before the call's column, and add a one-line positive row. - (judgment)
identifiersIn+resolvesToStatePathtreat any identifier on the right-hand side as tainting.const dir = path.dirname(statePath); const p = path.join(dir, 'ROADMAP.md'); fs.writeFileSync(p, c)is flaggedRAW_STATE_WRITEalthough the file written is ROADMAP.md, and ADR-4629 Decision 5 forbids an allowlist, so the contributor has no way to clear it. No negative control covers astatePath-derived directory with a non-STATE.mdliteral tail. Fix: stop descent when apath.join/resolvetail is a literal that does not name STATE.md, or add the negative control and document the posture.
Minor
FUNCTION_DECL_LINE_RE(scripts/lint-state-write-path-drift.cjs:280) matches onlyfunction name(. Arrow functions and class/object methods are not scope boundaries, so resolution can cross into a sibling body with a same-named variable (false positive); the new resolver is broader (up to 4 hops, every identifier) and the limit is untested for arrows/methods.- Boundary coverage: the hop-bound test asserts
MAXandMAX+1only; addMAX-1(limit-1, limit, limit+1). - No
fast-checkproperty for what is now a text parser/resolver (captureFirstArg,identifiersIn, binding regexes), and no CRLF fixture. identifiersIndrops template interpolation with\$\{[^}]*\}, so nested braces (${f({a:1})}) truncate and identifiers after the inner}are lost (false negative). Untested.
Nit
- Docs: "Updated docs/" is unchecked in the body and the PR relies on the exemption; consider a one-line note in the ADR-4629 guard table that Axis 2 now resolves same-function variables and still misses data-flow targets.
- The
no-changeloglabel description says "docs-only", but the diff is a CI lint guard (valid per changeset lint). The Miss-3 follow-up child is not linked. - Branch is BEHIND
next; rebase before merge. A rebase clears base drift only; it does not retire findings 1-6.
Security
No ReDoS found: the assignment/loop/destructuring regexes are lazy and bounded by line length, with cost O(lines × identifiers × hops). Output still passes sanitizeForReport. No traversal or argv surface added.
Blast radius & risk tier
findRawStateWrites is the Axis 2 scanner inside one lint script, with a test-side alias in tests/state-write-path-drift-guard.test.cjs; it runs in CI only and has no runtime callers. Not a central or bridge symbol.
Co-change completeness
The test file and script are both in the diff. No missing co-change file identified.
Memtrace Evidence
get_impact(findRawStateWrites, upstream, depth 2, branchnext): ambiguous — two declarations (the script function atscripts/lint-state-write-path-drift.cjs:627-652and a test-side constant), so no traversal was run on a single anchor. Impact is bounded by reading: a CI lint function with no runtime callers.get_symbol_context(findRawStateWrites): same ambiguity (2 candidates), no caller/callee rows returned. The documented contract on the script declaration ("unratcheted, unexempted: any occurrence is a violation") matches ADR-4629 Decision 5.recall_decision("raw STATE.md write guard no allowlist lint-state-write-path-drift"): returned only unrelated/indirect decisions (e.g. #3824 machine-readable state contract at step boundaries). No recorded decision contradicts this change. The result is statistically ranked, not a proof of absence.find_code_review_issues: not run for this PR — the diff text could not be supplied to the tool in this session. Findings above come from the isolated reviewer's static analysis of the diff and from direct reads, not from this tool.
Verdict
Changes requested (2 Major, 4 Minor, 3 Nit). CI is green on the required checks at this head, but that does not retire any finding above.
82b5777 to
9d7950e
Compare
|
@trek-e thanks. I pushed
The guard now carries five hand-written scanners. If more misses turn up, I'd propose an AST-based Axis 2 as its own issue. Re-requested your review. Verification
|
9d7950e to
250b5a4
Compare
|
Rebased onto Local on the new head: My 10-03 reply above still covers your findings.
|
trek-e
left a comment
There was a problem hiding this comment.
Review — ci(#5104): the raw STATE.md write guard sees wrapped calls and same-function variable targets
Head reviewed: 250b5a4f58714dc1ebe7be161678a6236edeefca. Full diff read (scripts/lint-state-write-path-drift.cjs and tests/state-write-path-drift-guard.test.cjs).
Summary
Axis 2 now captures wrapped calls and resolves variable targets with a bounded hop count. Review items 1, 2 (inline literal tail), 4, 5 and 6 from the 2026-09-30 review are fixed and verified by executing the code. Item 3 is declined and pinned, and a related false-positive class (m2) remains. Changes requested on the three Minor items below.
Classification & gate compliance
- Enhancement track. #5104 is OPEN with
approved-enhancement;Closes #5104present; conventional title; not draft; no competing open PR. - Scope: scripts/ + tests/ only, no src/ change, no allowlist, no dependency. Governing ADR-4629 Decision 5 honored.
- No changeset (changeset-lint
ok_no_user_facing_changes,no-changeloglabel); no docs (docs-lint green). - The issue said to reuse
nearestPrecedingAssignment; only its regex is shared (assignmentLineRe), the resolver is a newnearestTargetBinding. Axis 3 unchanged. - CI at this head: 28 success, 5 skipped, 0 failed, 0 pending, 0 action_required runs. Branch is BEHIND by 1 (
82f7acdfa7, CI budget history, does not touch the guard); clean rebase. A rebase fixes behind-ness only, not the findings below:git fetch origin next && git rebase origin/next.
Functional checklist / root-cause verdict
- Wrapped call captured over comment-stripped joined text (probe line numbers correct, CRLF equal to LF).
- Variable target resolved, bounded at MAX_TARGET_RESOLUTION_HOPS=4; MAX-1/MAX/MAX+1 tested.
- for...of / destructuring stop resolution (two rows).
- One-line
const p = path.join(dir,'STATE.md'); fs.writeFileSync(p,c)flagged at line 2 (executed). -
path.join(path.dirname(statePath),'ROADMAP.md')not flagged (executed). - Real tree via
--root: rawWriteViolations 0. - Root cause: the scan was per-line and matched only the literal
statePath/STATE.mdtext in the captured argument. The fix addresses that at the scanner, not by special-casing. Miss 3 (data-flow) stays open by ruling (#5120).
Blast radius
findRawStateWrites runs only in lint:ci (guard script plus its test and the --json CLI). New export MAX_TARGET_RESOLUTION_HOPS only. Risk: low. Perf probes: 3000-hop chain 7 ms; 3000 writes 140 ms; 200k-char line 4 ms.
Co-change completeness
Guard and test are both in the diff. No INVENTORY, CONTEXT.md, changeset or docs surface applies. ADR-4629 guard-table line refs remain stale (Nit n2, contributor declines ADR edits).
CodeGraph Evidence
codegraph_impacttargetfindRawStateWrites(base index, run by orchestrator): 5 affected symbols —findRawStateWritesand its callerscollect/maininscripts/lint-state-write-path-drift.cjs, the test filetests/state-write-path-drift-guard.test.cjs, andscanMarkdownLetterAxisConsumersinscripts/lint-phase-id-drift.cjs(likely a same-name symbol; not a runtime dependency). No runtime (non-lint) dependents. Low risk.codegraph_callerstargetnearestPrecedingAssignment(run by orchestrator): 1 caller,findUnstrippedContentWrites(scripts/lint-state-write-path-drift.cjs:531). Its behavior is unchanged byassignmentLineRe(same regex string, verified by reading the diff).codegraph_callees/codegraph_nodewere not run for this review; the callee list offindRawStateWrites(stripComments,captureFirstArg,resolvesToStatePath,sanitizeForReport) and theliteralJoinTailcharacterization are from reading the diff.
Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)
Test quality
Fail-first: the F4 rows target shapes that returned 0 on base (wrapped call, variable, one-line); PR body reports 8 failing against the unchanged guard. Property test discriminates STATE.md vs other files across shapes and CRLF. Boundaries covered. No wall-clock, no source-grep. Concern: limit: rows assert known-wrong verdicts (m3).
Prior review resolution (trek-e, 82b5777)
1 fixed; 2 fixed for inline literal tails; 3 not fixed (declined, pinned); 4 fixed; 5 fixed; 6 fixed; 7 declined; 8 #5120 linked; 9 behind by 1.
Findings
Minor
- m1 (judgment, executed probes)
scripts/lint-state-write-path-drift.cjsnearestTargetBinding: scope is line-based; arrows/methods are not boundaries and a one-line block/function is not read. Probes: sibling-arrowpresolves to another body'sSTATE.mdvariable (flagged); parameter shadowing on a prior line flagged;if (c) { const p = ...STATE.md; fs.writeFileSync(p,c); }not flagged. With no allowlist (ADR-4629 Decision 5) a false positive cannot be cleared. Fix: a brace-depth boundary tracker, or a maintainer ruling accepting the limit. - m2 (executed probe)
literalJoinTaildecides only an inline literal tail.const d = path.dirname(statePath); const n = 'ROADMAP.md'; fs.writeFileSync(path.join(d, n), c),path.join(d, name)andpath.join(d, `${name}.json`)are reported RAW_STATE_WRITE. Same unclearable class as prior Major 2. Fix: resolve an identifier tail throughnearestTargetBindingand decide by its literal value. - m3 (judgment)
tests/state-write-path-drift-guard.test.cjslimit:rows pin the false-positive verdicts above as passing assertions. Fix: assert the correct verdict after m1, or mark themtodo.
Nit
- n1 PR body claims
npm testpassed and cites a gsd-test run on the pre-rebase head; the Node 22 failure claim is not independently verified. - n2 ADR-4629 guard-table note declined (review item 7).
- n3
no-changeloglabel description says docs-only (review item 8).
Verdict
Request changes (0 Blocker, 0 Major, 3 Minor, 3 Nit). Substance is close and CI is green at this head; the branch also needs git fetch origin next && git rebase origin/next before merge.
250b5a4 to
081d4df
Compare
|
@trek-e thanks. I pushed
Re-requested your review. Verification (local, head 081d4df)
|
There was a problem hiding this comment.
Automated maintainer re-review of head 081d4df038. Zero-tolerance policy: every finding at any severity must be resolved before approval.
Re-review PR #5119 (head 081d4df, fetched this turn)
Summary
Axis-2 now handles wrapped calls, same-function variable targets, one-line assign+write, CRLF, join-tail literal (inline and via resolved identifier). Executed probes show remaining false negatives introduced by the new join-tail pre-check (flow-insensitive literal), plus the documented scope limit (m1). Limitation: the test file could not be read (Memtrace-first guard denies Read/Bash on indexed source; codegraph does not index the test file in the worktree). Test-quality items are unverified except via PR-body claims and executed guard behavior.
State (gh pr view this turn)
mergeable=MERGEABLE, mergeStateStatus=BEHIND, reviewDecision=CHANGES_REQUESTED, isDraft=false. Labels: area: workflow, no-changelog, review: changes requested. No non-success/non-skipped checks in statusCheckRollup. Behind origin/next (ed0f3fd) by 5 commits (merge-base f2077e7); none touch the 2 PR files (three-dot diff = only those 2 files; commits: #5253, #5247, #4935, #5093, #5092). Rebase: git fetch origin && git rebase origin/next.
Prior findings status (2026-10-07 01:24 review)
- m1 sibling arrow/method/shadow scope: UNRESOLVED (declined, pinned). Executed:
const p=...STATE.md; fn((p)=>{ fs.writeFileSync(p,c) })reports 1 (false positive; no allowlist per ADR-4629 D5 so uncleared). - m2 join tail via identifier: PARTIAL.
const n='ROADMAP.md'cleared (scripts/lint-state-write-path-drift.cjs resolvedJoinFileTail).path.join(d,name)param and${name}.jsonremain reported (pinned as known gap). But new false negatives, see Findings M1. - m3 limit rows assert wrong verdicts: author says converted to known-gap table asserting today's output AND that it differs from correct verdict (unverified: test file unreadable). Still pins wrong verdicts as passing tests.
- n1 PR body claims: author says corrected. Body now says gsd-test run predates review rounds; CI on head is authority. RESOLVED.
- n2 ADR guard-table note: declined (ADR edits excluded). UNRESOLVED nit.
- n3 no-changelog label description: not author's to change. Nit.
Earlier 09-30 findings (one-line, dirname tail, MAX-1, fast-check, CRLF, nested braces): verified fixed by execution (onelinevar, join-dirname ROADMAP negative, crlf, 4hop flagged/5hop not).
Acceptance checklist (issue #5104 Layer 1, trek-e approval 2026-09-29)
- First arg across line breaks over comment-stripped text (wrapped, wrapped-with-comment, CRLF executed -> 1 each).
- [~] Resolve non-statePath non-literal target via same-function assignments "reusing the guard's existing nearestPrecedingAssignment", bounded hops: resolver is a new nearestTargetBinding; only the regex (assignmentLineRe) is shared. Behavior met; "reuse" wording not literally. Bounded: 4-hop flagged, 5-hop not (executed).
- for...of / destructuring = unresolved (per PR; code lastBindingInText 'unresolved').
- Axis 2 only, no src/ change, no allowlist, no new dependency (diff = 2 files).
- [~] Tests: failing-first, sibling-block, negatives, real-tree control claimed; not verified by reading.
- Miss 3 out of scope (#5120 linked).
Findings
Major
M1 scripts/lint-state-write-path-drift.cjs resolvedJoinFileTail/nearestTargetBinding (flow-insensitive literal decides): REGRESSION vs prior head 250b5a4 (by code trace: dir hop to statePath flagged before). Executed on head, all report 0 where STATE.md is written on a path:
function f(statePath){const d=path.dirname(statePath); let n='STATE.md'; if (x) n='ROADMAP.md'; fs.writeFileSync(path.join(d,n),c);}-> 0- same with
if (x) {\n n='ROADMAP.md';\n}-> 0
Nearest textual assignment wins; a conditional reassignment hides a real STATE.md write. Contradicts the guard's fail-safe posture (unknown must not resolve to safe) and the "0 newly reported / no regressions" claim, which only measures newly reported, not newly cleared beyond the 5 "intended". Fix: only let the pre-check clear when the identifier has exactly one binding in the function and that binding is a const (or no other assignment to the name exists in the function); otherwise fall back to whole-expression rule. Add a negative-control... positive row for the conditional reassign.
Minor
m1 Scope limit (arrows/methods/params) unresolved; false positives uncleared with no allowlist. Author proposes AST Axis 2 as separate #4629 child: maintainer decision, but until filed the limit is a standing FP source. Request the child issue be filed/linked.
m2 Pre-existing, adjacent, executed: wrapped call on a line after a string containing // (const u='http://x'; fs.writeFileSync(\n statePath,c);) -> 0 (stripComments is string-unaware); regex literal /\// same -> 0. Wrapped detection (this PR's purpose) is blind there. Fix inside stripComments (string/regex aware) or document.
m3 Pre-existing, unchanged scope but unlisted in guard docs: only literal fs.writeFileSync( matches. fs.writeFile, fs.promises.writeFile, fsp.writeFile, fs.appendFileSync, destructured writeFileSync, fs?.writeFileSync, fs['writeFileSync'], node:fs alias all report 0 for statePath targets (executed). Not in #5104 scope; mention so the guard header does not overclaim (ADR-3408 D5 zero-is-not-proof).
m4 False positive (executed): "fs.writeFileSync(statePath, c)" inside a string literal or template text reports 1.
m5 Test-quality not verifiable here (file unreadable under guard): confirm no source-grep, no wall-clock, property is discriminating, limit rows not vacuous.
Nit
n1 ADR-4629 guard-table stale line refs (author declines).
n2 no-changelog label description says docs-only.
n3 PR body checkboxes: platforms Linux only; body says docs not updated (exempt, internal lint).
Gates
Enhancement track OK (approved-enhancement on #5104, Closes #5104, ci(#5104): title, not draft). ADR-4629 Decision 5 (no allowlist) honored. no-changelog justified: guard runs in lint:ci only, no user-facing surface (author reports changeset lint ok_no_user_facing_changes; not re-run here). No docs/INVENTORY/CONTEXT.md/hook registration needed (scripts only, no new file). Not run: recall_decision, find_code_review_issues (Memtrace quota exhausted); not run: lint/tests (forbidden).
Blast radius (codegraph_impact, main-checkout index, 5 symbols)
findRawStateWrites:627, collect:881, main:961 in the guard; tests file; scanMarkdownLetterAxisConsumers (lint-phase-id-drift.cjs:646, name-similar only). CI-only, no runtime dependents. Low. New export MAX_TARGET_RESOLUTION_HOPS only.
Behind-by / rebase
BEHIND by 5; no overlap with PR files. Rebase clears behind-ness only.
Verdict
Changes requested: 1 Major (M1 conditional-reassign false negative / regression), minors above. Probe script: scratchpad/probe.cjs (40 shapes).
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.
Open findings remain, so no further CI is being spent on this head unless the remaining findings are Nit-only (CI already on record for this head is noted above). Address them; the fix push re-triggers CI once.
081d4df to
fad8de3
Compare
|
@trek-e thanks. I pushed
Re-requested your review. Verification (local, head fad8de3)
|
trek-e
left a comment
There was a problem hiding this comment.
Review of head fad8de39927a — changes requested
Summary. M1 is fixed: both of your probes are reported again on this head, and the "can only add reports" claim holds for this commit. One new Major remains in the same family, plus Minors and Nits.
Your four asks, verified
- Both M1 probes are reported [executed with plain
nodeagainstfindRawStateWrites].let n='STATE.md'; if (x) n='ROADMAP.md'; write(path.join(d,n))reports line 5; the braced variant, line 7; thevarvariant, line 5; and aconst m = nalias of a reassignedlet, line 6. The code path isresolvedJoinFileTailreturningnullwhen!binding.isConst(~L771), soresolvesToStatePathfalls through totargetsStatePath(expr). The one-line whole-function form still reads 0 (the cut-line rule), as you disclosed. I could not check the "also 0 on250b5a4f5" claim; it is consistent with the cut-line rule and with the pinned one-line-block rows. - "Only removes clears, so it can only add reports" is true for this commit, relative to
081d4df03and not tonext: the single behavioral edit is!binding.isConston the sole clear path, and returningnullmore often can only send more expressions to the existing report path. The 56-shape harness is therefore not needed for this claim. It does not make the remaining clears safe (Major 1). - m1-m4 and #5279. m1 is filed as #5279. For m2 and m3 your instruction in the earlier review was "fix inside
stripCommentsor document" and "mention so the guard header does not overclaim", and both are documented in thefindRawStateWritesheader and pinned in theKNOWN_*_GAPStables, consistent withCONTRIBUTING.md:766. They are not findings against this head. Separately, #5279 is stillneeds-triage; that is yours to decide. - Fresh review: below.
Classification and gates. #5104 carries enhancement + approved-enhancement + area: workflow. Closes #5104; title ci(#5104):; not draft; ADR-4629 Decision 5 (no allowlist) honored. No changeset, with the no-changelog label and changeset-lint SUCCESS, which is consistent for scripts-and-tests-only CI lint. Two files, no src/. The script is already wired in lint:ci (package.json:142).
Tests. Assertions read findRawStateWrites output; no source-grep, no wall-clock; a CRLF row is present; the CLI --root row asserts findings lines [5, 12]. The main resolution chain has MAX-1 / MAX / MAX+1 rows. The fast-check property is not vacuous: it builds three source shapes × CRLF/LF × filler × three directory expressions, and asserts the STATE.md variant yields exactly one finding at the exact line and the other-file variant yields zero. Its reach is const and path.join(<dir>, '<literal>') only.
Findings
Major 1 — false negative from the const-follow rule ignoring block scope [verified, executed]
The rule takes the nearest textual const and ignores block and arrow scope. With n a parameter:
function f(statePath, n){
const d=path.dirname(statePath);
if (x) {
const n = 'ROADMAP.md';
use(n);
}
fs.writeFileSync(path.join(d, n), c); // n is the parameter, may be 'STATE.md'
}reports 0; the same happens with the const n inside a sibling arrow body. nearestTargetBinding stops only at named function declarations, and the const here is block-scoped to a closed sibling block. This is the same fail-safe contradiction as your M1 (a literal clears a write another binding could aim at STATE.md), reached through scope instead of reassignment. It predates this commit, has no pinned row, and is not among the acceptance rows listed for #5279.
Fix: clear only when the const's line is at the same brace depth as the write, or when no {, } or => lies between the const line and the write; otherwise fall back to the whole-expression rule. At minimum pin it as a known-gap row and add it to #5279's acceptance. Severity is judgment, under the "unknown must not resolve to safe" posture applied to M1; the precondition (a same-named const in a closed sibling scope plus a parameter tail on a statePath-derived directory) is narrower than M1's.
Minor 2 — no limit-1/limit/limit+1 rows for the join-tail const chain [verified by reading]. resolvedJoinFileTail spends hopsLeft and returns null on exhaustion, which reports. Only the target chain (chainFixture) has MAX-1 / MAX / MAX+1. Add a tail chain n0=n1=…='ROADMAP.md' at those three lengths.
Minor 3 — the exact one-line header-line shape is not its own pinned row [verified]. function f(statePath){ … } on one line reads 0. The class is pinned as a one-line block; pin this exact shape too.
Nit 4. The PR body is stale: "37 rows", "52/52 pass", and the gsd-test run "predates the review rounds"; the body states Linux only for platforms, which is accurate for what was run.
Nit 5. The notDeepStrictEqual(gap.currentBuggyOutput, gap.expected) assertion compares two hard-coded literals from the table row, a table-integrity check. The row going loud when the guard is fixed comes from the next line, deepStrictEqual(lines(gap.text), gap.currentBuggyOutput). The PR's wording attributes the loud break to the wrong assertion; behavior is correct and matches CONTRIBUTING.md:766.
Nit 6. The property's RESERVED list omits function, a legal 8-character draw from ^[a-z][a-zA-Z0-9]{0,7}$; const function = … would trip CUT_LINE_BODY_RE. Astronomically unlikely, but add it. Also a cosmetic mid-sentence reflow in the resolvedJoinFileTail docblock.
Nit 7. BEHIND next by one commit; a merge or rebase clears that and is a CI condition.
Security. No new file-reading, argv, subprocess or path-handling surface; report output still passes through sanitizeForReport; the new regexes are anchored per line and the hop bound is 4. The only relevant direction is the fail-open one in Major 1; the guard is advisory CI, not a runtime boundary.
Not verified. I did not execute any test (local node --test is not permitted); fail-first rests on the author's RED 2 / GREEN 89 report, and CI is green on this head.
CI. Head fad8de39927a: Required tests, lint-tests, the Ubuntu shards 1-3, Coverage gate (merged shards), changeset-lint, docs-lint, QA loop walk, Base branch health and the policy checks are SUCCESS; validate-target, auto-merge, conformance test (matrix), test (inert CI) and the emitted-baseline job are SKIPPED. No failed, pending or action_required rows.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run. The recorded-decision check was not evaluated beyond ADR-4629. The index is of the main checkout, so branch-new symbols are absent.
codegraph_impact findRawStateWrites: 5 symbols (findRawStateWritesatscripts/lint-state-write-path-drift.cjs:627,collect:881,main:961,scanMarkdownLetterAxisConsumersinscripts/lint-phase-id-drift.cjs:646, and the guard's test file).codegraph_callers findRawStateWrites:collect(scripts/lint-state-write-path-drift.cjs:881) andtests/state-write-path-drift-guard.test.cjs.- Finding: CI-only, no runtime dependents; blast radius is low.
Verdict
Changes requested. Blocker 0, Major 1, Minor 2, Nit 4.
trek-e
left a comment
There was a problem hiding this comment.
Review of head fad8de39927a — changes requested
Summary. This is the full pass on the same head; the earlier review (02:25Z) is not edited. The M1 fix holds and the guard is clean on the real tree. Running findRawStateWrites on the head again confirms the block-scope Major and adds a second one in the same family: the guard misses the headline shape (a variable holding a STATE.md path) when the declaration carries a TypeScript type annotation, and the scanned sources are TypeScript. Blocker 0, Major 2, Minor 8, Nit 7.
Classification and gates. #5104 carries enhancement + approved-enhancement + area: workflow. Closes #5104, title ci(#5104):, not draft. ADR gate: ADR-4629 Decision 5 (no allowlist, terminal guard) is honored; the guard-table line references in the ADR are stale (the author declined to update them earlier). No changeset, with the no-changelog label and changeset-lint SUCCESS, which is consistent for scripts-and-tests-only CI lint; no docs/ surface. Two files, no src/; the script is wired in lint:ci.
Acceptance, #5104 Layer 1 (as approved)
- Capture
fs.writeFileSync's first argument across line breaks over comment-stripped text — Met. - Resolve a non-
statePath, non-literal target through same-function assignments with a bounded hop count — Partial: Met for the chain with a 4-hop bound; the block-scope and typed-declaration holes (Majors 1 and 2) mean a real write reads 0. - Treat a
for...ofor destructuring binding as unresolved — Met. - Reuse
nearestPrecedingAssignment— Partial: only the regex is shared (assignmentLineRe); the resolver is new (nearestTargetBinding), and the diff is about 1,060 lines against the issue's "about 20 lines on top of the existing helpers". - Axis 2 only, no
src/change, no allowlist, no dependency — Met. - Failing-first fixtures per shape, a sibling-block fixture, negatives, a real-tree control — Met in shape; I did not verify a committed real-tree test (the delegated pass ran the guard against the real tree: exit 0, 0 raw writes).
- Miss 3 out of scope — Met; documented in the header.
Reproduction (plain node, findRawStateWrites(file, text) on a copy of the head script; each row is the number of reported writes)
| Source | Reports |
|---|---|
function f(d,c){ const p = path.join(d,'STATE.md'); fs.writeFileSync(p,c); } (multi-line, untyped control) |
1 |
the same with const p: string = … |
0 |
the same with let p: string = … |
0 |
module-level const STATE_FILENAME = 'STATE.md'; and fs.writeFileSync(path.join(d, STATE_FILENAME), c) |
0 |
function f(statePath, n){ const d = path.dirname(statePath); if (x) { const n = 'ROADMAP.md'; use(n); } fs.writeFileSync(path.join(d, n), c); } |
0 |
Findings
Major 1 — the const-follow rule ignores block and arrow scope: a literal in a closed sibling scope clears a real write [verified: last row; carried] nearestTargetBinding stops only at named function declarations, so a const n = 'ROADMAP.md' inside a closed sibling block or arrow body hides a parameter n that may be STATE.md. A worse variant needs no parameter: an outer const n = 'STATE.md' followed by a closed-block const n = 'ROADMAP.md' and write(join(d, n)) reads 0 although the write provably lands on STATE.md (the delegated pass's probes; the one-line-block and arrow-body variants also read 0). This is the same fail-safe contradiction as the earlier M1 (a literal must never clear a write another binding could aim at STATE.md), reached through scope instead of reassignment; it has no pinned row and is not in #5279's acceptance. Fix: clear only when the const's line is at the write's brace depth, or no {, } or => lies between the const line and the write; otherwise fall to the whole-expression rule. At minimum pin it as a known-gap row and add it to #5279.
Major 2 — a TypeScript type annotation hides the binding: the guard misses const p: string = path.join(d, 'STATE.md') [verified: rows 2-3; isolated pass rated this Major] assignmentLineRe (scripts/lint-state-write-path-drift.cjs ~491-492) requires name\s*=, so name: Type = never matches and resolution returns unresolved, which reads as "no STATE.md write". The scanned sources (src/*.cts) are TypeScript, so the issue's headline shape is missed in the language the guard protects. The regex is shared with Axis 3, so allow an optional :\s*<type> before =, re-run Axis 3's rows, and add fixtures for typed const/let and a typed join tail.
Minor 1 — no limit-1/limit/limit+1 rows for the join-tail chain [verified] chainFixture covers MAX_TARGET_RESOLUTION_HOPS for the target chain only; resolvedJoinFileTail has its own hopsLeft budget (the delegated probe: d = dirname(statePath), n0 = 'ROADMAP.md', chain n_k = n_{k-1}, write(join(d, n_h)): h = 2 and 3 read 0, h = 4, 5, 6 read 1). Add a tail chain at the three lengths.
Minor 2 — the exact one-line function and arrow shapes are not pinned [verified] function w(){ const p = join(dir,'STATE.md'); fs.writeFileSync(p,c); } and the one-line arrow read 0 (the cut-line rule); only the one-line if block (~:1166) is its own row.
Minor 3 — a module-level constant file name is a false negative that the header does not list [verified: row 4; isolated pass] const STATE_FILENAME = 'STATE.md' outside the function, used inside it, reads 0; this is a more common real shape than the Map-key case the header lists.
Minor 4 — the header and the gap tables do not record the three verified gaps [Standards pass, rated Major there] findRawStateWrites's header lists only the Map-key miss plus the string/comment and write-API gaps, and says the tests pin each known gap; the typed declaration, the module constant and block shadowing have no characterization row (CONTRIBUTING.md:766: record known-broken behavior as expected* plus currentBuggyOutput), although the PR does this for 14 other shapes. With no allowlist (ADR-4629 Decision 5) an unrecorded false negative has no other record. Some KNOWN_TAIL_GAPS rows ("parameter", "interpolated template") are deliberate fallback behavior, not defects, and are filed under "currently wrong".
Minor 5 — property-test breadth [Standards pass; verified for RESERVED] One fc property covers const plus path.join(<dir>,'<literal>') over three source shapes and four file names; it generates no wrapped calls, typed declarations, hop-chain depth, let/var or shadowing, which is why Majors 1 and 2 survive it, and it is built from the author's own resolver shapes. RESERVED omits function, a legal draw from ^[a-z][a-zA-Z0-9]{0,7}$ (const function = … would trip CUT_LINE_BODY_RE); joinLastSegment, splitStatements and templateInterpolations have no direct property.
Minor 6 — the path-tail pre-check changes the existing statePath verdict [Spec pass, judgment] A tail that resolves to a non-STATE literal now returns false before targetsStatePath runs (const n = 'ROADMAP.md'; path.join(statePathDir, n) no longer reports), arguably correct; with no allowlist to recover from either error, please state the narrowed rule in the PR body and confirm it is intended.
Minor 7 — scope and the reuse requirement [Spec pass, judgment] About 300 added script lines (a second resolver, += handling, same-line statement splitting, template-interpolation parsing, a CUT_LINE_BODY_RE heuristic, CRLF normalization, a regex cache) against "about 20 lines on top of the existing helpers", and nearestPrecedingAssignment is not reused; bindingRegexCache is module-level mutable state; resolvedJoinFileTail and resolvesToStatePath each walk the binding chain with their own hop budget and seen handling.
Minor 8 — false-positive classes with no way to clear them [isolated pass, verified] A shadowing parameter with the same name in an arrow or method (const p = join(d,'STATE.md'); const w = (p) => { fs.writeFileSync(p,c); } reports 1), targets derived from the state file's content or name (const p = readIt(path.join(d,'STATE.md')); fs.writeFileSync(p,c) reports 1), STATE.md.bak literals, and a string that spells the call; the current tree contains none of these shapes (scan 0), and ADR-4629 Decision 5 forbids an allowlist.
Nit 1 [verified] The documented-gap call shapes can be written by a contributor: require('fs').writeFileSync(statePath,c), an alias (nfs.writeFileSync), destructured writeFileSync, fs.promises.writeFile, fs.appendFileSync, fs . writeFileSync (, and fs.renameSync(tmp, statePath) all read 0; a // inside a string on the same line hides the call (const u='http://x'; fs.writeFileSync(statePath,c); reads 0).
Nit 2 [verified] Per-call backward resolution is quadratic in the number of unresolved calls: 500 calls in one file take 84 ms, 2,000 take 713 ms, 5,000 take 5.2 s; not reachable on the current tree.
Nit 3 [verified] The PR body is stale ("37 rows", "52/52 pass", a gsd-test run that predates the review rounds).
Nit 4 [verified] notDeepStrictEqual(gap.currentBuggyOutput, gap.expected) compares two table literals; the "loud when fixed" break comes from the next deepStrictEqual, so the body attributes it to the wrong assertion.
Nit 5 [judgment] The shared assignmentLineRe couples Axis 3 to Axis 2, so a regex change for one silently alters the other; add a parity assertion (Generative Fix Divergence).
Nit 6 [judgment] Test comments still say "review round 3/4" (process noise in committed tests); ADR-4629's guard-table line references are stale and the no-changelog label description says docs-only.
Nit 7 [verified] The branch is BEHIND next by one commit (a rebase is a CI condition; next's commit touches neither file).
Security. No new file-reading, argv, subprocess or path surface (applied inline over this diff); output still passes through sanitizeForReport; the new regexes are per line and the hop bound is 4. The only relevant direction is the fail-open one in Majors 1 and 2; the guard is advisory CI, not a runtime boundary.
Passes run
| Pass | Result |
|---|---|
| ADR gate | read ADR-4629 Decision 5, ADR-3473 §8.6, ADR-3408 D5: consistent |
/writing-documentation-with-diataxis |
not applicable: scripts and tests only, no docs/ surface (applied inline) |
| Functional checklist | acceptance table above |
/qa-test-architect |
run; Minors 1, 2, 5, Nits 4 and 6 |
/code-review |
run as two parallel sub-agents: Standards (Minors 4 and 5, Nit 5) and Spec (Minors 6 and 7, acceptance 4) |
/security-review |
applied inline: no finding (CI-only guard) |
/rubber-duck |
run; "a literal must never clear a write another binding could aim at STATE.md" fails for scope and for typed declarations |
/codebase-design |
run; two partial parsers (nearestTargetBinding, nearestPrecedingAssignment) share one regex and neither owns "which binding does this name refer to" |
/skills-from-the-artificer (consolidation-review) |
run; block below |
| Isolated adversarial pass | run by a fresh agent with no prior context (about 140 crafted inputs); found Major 2 and Minors 3 and 8 |
Not run (Memtrace is unavailable): the Deterministic Review Pass (detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr), preflight_check, risk tiering (find_central_symbols, find_bridge_symbols), the recorded-decision check (recall_decision, governing_rules) and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted); Stryker is not among the rollup rows, so the 80% threshold is not evaluated.
Laws that apply
Subject: PR #5119 makes the raw STATE.md write guard follow wrapped calls and same-function variable targets with text-based binding resolution.
- greenspuns-tenth-rule — fired: the guard now reimplements scoping, hop-bounded binding resolution, statement splitting and template parsing with regexes over text (
nearestTargetBinding,lastBindingInText,CUT_LINE_BODY_RE), which is an ad-hoc, partial JavaScript/TypeScript parser. Action: Majors 1 and 2; consider an existing parser over text heuristics. - goodharts-law — fired: a green guard and a green property test do not mean "no raw STATE.md write exists"; the generators never produce typed, shadowed or module-constant shapes. Action: Minor 5.
- postels-law — fired: unresolved input is treated as "not a write" (lenient) where the guard's purpose requires "unknown must not resolve to safe"; the earlier M1 applied that to
let/var, but scope and type annotations still resolve to safe. Action: Majors 1 and 2. - hyrums-law — fired: contributors will learn which shapes the guard passes and write that way, so the known-gap list becomes the interface; unrecorded gaps (Minor 4) are worse. Action: Minors 3 and 4.
- leaky-abstractions — fired: the guard's text model leaks the language (TypeScript annotations, block scope, arrow bodies), which it cannot see. Action: Majors 1 and 2.
Considered and cleared: kerckhoffs-principle (advisory CI guard, not a runtime boundary), kernighans-law (covered by Minor 7), knuths-optimization-principle (the quadratic cost is Nit 2).
CI. Head fad8de39927a: every row is SUCCESS or SKIPPED (Required tests, lint-tests, the Ubuntu shards, Coverage gate, QA loop walk, changeset-lint, docs-lint, security, and the policy checks); no failure, pending or action_required run. The branch is BEHIND next by one commit. I am not approving: the Majors need a push.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted and the server disconnected, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run, and the recorded-decision check was not evaluated beyond ADR-4629.
codegraph_impact findRawStateWrites: 5 symbols (findRawStateWritesatscripts/lint-state-write-path-drift.cjs:627,collect:881,main:961,scanMarkdownLetterAxisConsumersinscripts/lint-phase-id-drift.cjs:646, and the guard's test file);codegraph_callers:collectandtests/state-write-path-drift-guard.test.cjs. The index is the main checkout, so branch-new symbols are absent and the head behavior was exercised through a copy of the head script.- Finding: CI-only with no runtime dependents, so the blast radius is low; the risk is a guard that reads green on shapes it cannot see.
Verdict
Changes requested. Blocker 0, Major 2, Minor 8, Nit 7.
fad8de3 to
29f3b6e
Compare
|
@trek-e thanks. I pushed
I'm adding the shapes still pinned as gaps to #5279's acceptance list. Verification (local, head 29f3b6e, empty HOME/GSD_HOME)
|
….md file name is not a raw STATE.md write
Review round 3 (5436531471) m2: `const n = 'ROADMAP.md'; fs.writeFileSync(path.join(d, n), c)`
with d derived from statePath was reported RAW_STATE_WRITE, and with no allowlist
(ADR-4629 Decision 5) nobody could clear it. A path.join/path.resolve now resolves its
last argument through the same-function resolver (spending hops), and when that
reaches a string literal that names a file, the literal alone decides. Every other
tail (a parameter, member, call, spread, interpolated template, conditional, '' or
'.', a module-level constant, an exhausted hop budget) falls through to the previous
rule unchanged, so the guard reports nothing less than before: across 56 probe shapes
the only changes are the five intended clears.
Still reported, and pinned as known gaps: an unresolved tail on a statePath-derived
directory (`path.join(d, name)`, `${name}.json`), where the written file is not visible
in the function.
m3: the limit rows assert today's output beside the correct verdict and fail loudly
when a fix changes either, instead of passing assertions on known-wrong verdicts
(CONTRIBUTING.md forbids `todo` tests).
…d pin the axis's call-match limits Review round 4, M1: the join-tail pre-check let the nearest textual assignment decide, so `let n = 'STATE.md'; if (x) n = 'ROADMAP.md';` cleared a write that can land on STATE.md. A tail identifier is now followed only through `const` declarations; a `let`, `var` or bare assignment leaves the whole expression to decide, as before the pre-check. The two reviewer probes are reported again, and a `let` that is never reassigned is pinned as a known false positive. m2-m4: the axis matches only a literal `fs.writeFileSync(` over `stripComments` output, which does not track strings or regex literals. The function header now says so, and the tests pin the string/regex `//` misses, the string false positive and two other write APIs as known gaps.
…ite and frontmatter-write scans The shared assignment regex required `name =`, so `const p: string = path.join(dir, 'STATE.md')` never bound `p` and the raw-write axis read 0 on the headline shape in the TypeScript it scans. A `const`/`let`/`var` declaration may now carry an annotation before `=` (generics, unions, record types). The annotation is read only after a declaration keyword and holds no `=` or `;`, so object keys, `==`/`===` comparisons and `let p: T; q = x` still bind nothing. The regex is shared with Axis 3, so a parity row now pins that both axes read the same binding grammar. Real tree: rawWriteViolations 0; Axis 3 findings and its resolver output at all 47 stateReplaceField( sites in src/ are unchanged.
…no longer clears a raw STATE.md write
The raw-write axis took the nearest textual binding and ignored block and
arrow scope, so `const n = 'ROADMAP.md'` inside a closed `if` block or
arrow body cleared `fs.writeFileSync(path.join(d, n), c)` where `n` is a
parameter or an outer `const n = 'STATE.md'`.
Fail-safe minimum fix, not a scope tracker: a binding decides only when no
`{`, `}` or `=>` lies between the end of its right-hand side and the write
in the comment-stripped text. Past such a boundary, a const join tail no
longer clears (the whole-expression rule decides, as before the tail
pre-check), and the target chain keeps searching for an earlier binding of
the same name instead of stopping at the first one that misses STATE.md.
A binding that reaches STATE.md still reports at once, so closure writes
are unchanged. The rule only removes clears.
The const check is now anchored at the match start, so a later `const` in
the same statement span cannot mark a `let` as `const`.
New rows: the sibling-block probe (multi-line, one-line block, arrow
body), the outer-STATE/inner-ROADMAP shadow (directory from statePath and
from a plain parameter, and through the write target), controls for a
binding in the write's own body, a by-design row for the new fallback
(a const before a block that holds the write), and the whole-function
one-line probe as a known scope gap. Real tree: rawWriteViolations 0.
…esign fallback from known defects
Characterization only; no guard behavior changes.
- The findRawStateWrites header now lists every verified false negative
(data built in another function, a module-level constant, a body on the
write's own line, a chain past the hop bound, the unseen call shapes)
and every verified false positive, and names the by-design fallback.
- Known-gap rows (expected plus currentBuggyOutput) for the one-line
function and one-line arrow, a module-level STATE_FILENAME constant, a
file name returned by another function, fs.appendFileSync,
fs.renameSync(tmp, statePath), fs.writeFile, nfs.writeFileSync,
require('fs').writeFileSync, `fs . writeFileSync`, fs?.writeFileSync and
fs['writeFileSync'].
- A known false-positive table: same-name arrow parameter shadowing (two
forms), a target derived from the state file, and a STATE.md.bak
literal.
- The parameter and interpolated-template join tails move from
KNOWN_TAIL_GAPS to BY_DESIGN_FALLBACKS, which asserts current output
only: there the correct verdict is unknown, not wrong.
- MAX - 1 / MAX / MAX + 1 rows for the join-tail chain, built from the
exported MAX_TARGET_RESOLUTION_HOPS.
- Rows for joinLastSegment, splitStatements and templateInterpolations,
through findRawStateWrites (they are not exported).
- The gap-loop comment now says the following deepStrictEqual, not
notDeepStrictEqual, is what fails loudly when a fix lands; review-round
process comments are removed.
…lon still binds its name
`const p: { dir: string; file: string } | string = path.join(dir,
'STATE.md')` read 0: the annotation excluded `;`, which an object type
holds between its members. The annotation may now hold one level of
braces whose body is read whole; `;` and `=` outside braces still end
it, so `let p: T; q = x` binds nothing. Rows added to the typed
declaration test and the Axis 2 / Axis 3 parity table. Real tree:
rawWriteViolations 0; Axis 3 output unchanged at all 47 sites.
…ained and shadowed declarations The existing property drew only `const` plus path.join(<dir>, '<literal>') over shapes the resolver was built for, so it could not have caught the typed-declaration or sibling-block misses. A second property states its oracle from the spec: a write whose target can be STATE.md is reported once at its own line, and a write whose every possible target is another known file is not. It draws const/let/var, five type annotations, a chain of 1..MAX_TARGET_RESOLUTION_HOPS declarations, a closed sibling block that redeclares the target as ROADMAP.md, a wrapped call, filler and CRLF. Against the previous head it fails (shrunk counterexample: const, depth 1, dir, shadow); here it passes on seeds 42, 1, 2, 3, 7, 11, 99, 1234 and 2026. The RESERVED set shared by both properties now holds the JS/TS reserved and keyword-like words (function, class, return, typeof, await, ...) and every name the fixtures already use.
…ot in module state The per-name assignment regexes lived in a module-level Map that grew across every scanned file. They now live in the context that one findRawStateWrites call already threads through the resolver, and nearestTargetBinding looks its pair up once per call instead of once per line. No verdict changes. Measured on N unresolved writes in one function (best of 3): 500 writes 138 ms before, 128 ms after; 2000 writes 2210 ms before, 2085 ms after; 5000 writes 14134 ms before, 11068 ms after. The quadratic cost of per-call backward resolution is unchanged; the AST follow-up (open-gsd#5279) replaces it.
…sses as known gaps The no-worse harness re-ran two misses reported in earlier rounds and found neither pinned nor listed: a `;` inside a template nested in another template's interpolation ends a right-hand side early, and `p = fs.writeFileSync(p, c)` reads its target's binding as empty. Both read 0 on the previous head and on this one. They are now known-gap rows (expected plus currentBuggyOutput) and listed among the verified false negatives in the findRawStateWrites header. No behavior change.
…e header and as rows
The header's false-positive list now names the arrow parameter read as an
assignment and the sibling arrow body, both already pinned. It states that
the body-boundary test is textual, so an unrelated closed block, an object
literal, a callback or a `${}` interpolation also stops a const tail from
clearing a write; four by-design rows pin that. Four known-gap rows pin
missed writes: a declaration whose value starts on the next line (typed and
untyped), an annotation with nested braces, and a `;` inside a
string-literal type.
… 1 and reuse nearestPrecedingAssignment Review 5512635344 (Majors 1 and 2, Minor 1): - Axis 2 now resolves a target through nearestPrecedingAssignment itself, extended with opt-in options (typed, stopAtPatternBinding, withLine). Axis 3 passes no options, so its regex and verdicts are unchanged. nearestTargetBinding, lastBindingInText and the binding regex cache are gone. - Removed, moved to open-gsd#5279: the join-tail pre-check (joinLastSegment, fileNameLiteral, resolvedJoinFileTail), CRLF normalization, the template-interpolation parser, += handling, and same-line statement splitting (splitStatements, statementSpans, CUT_LINE_BODY_RE). With the join tail gone there is one resolver, one hop budget. - Kept, as requested in earlier rounds: multi-line capture, the bounded hop count, the for...of / destructuring stop, the write's own line read up to the call, the body-boundary rule, and typed declarations. - Each shape the cut gives up is a known-gap row with its correct verdict.
…zero is not proof Review 5512635344, Minor 3: the file header's Axis 2 paragraph now states that a zero from this line scan does not prove no raw STATE.md writer remains, and points at findRawStateWrites' list of verified misses (ADR-3408 Decision 5) and the syntax-tree follow-up, open-gsd#5279.
…he misses it closed - Known-gap rows (false positives) for three shapes whose verdict moved with the cut: a one-line top-level function's parameter resolved into an earlier function, a bare `x =>` arrow parameter shadowing an outer variable, and a `for` header's loop-scoped `let` earlier on the write's line. The findRawStateWrites header lists them. - One positive row for three shapes that were known misses before the cut and now report: an assignment and write in a one-line block, in a one-line arrow body, and a reassignment in a closed block on the write's line.
…ound is not exported
Without the export, fc.integer({ max: undefined }) draws unbounded chain
lengths and the run dies out of memory instead of failing. Asserting the
export first makes the fail-first run against next's guard report its
failures (24 of 82) with no shim.
… write stay bounded
Every hop of resolvesToStatePath branched over each identifier of a
right-hand side, with no record of what it had already resolved. A file
with 60 `let vN = v0 + ... + v59;` lines, each followed by `{ }`, and
`fs.writeFileSync(v0 + ... + v59, c)` took 112 s to scan locally (20.4 s
in review 5558390402).
Each name's lookup is now memoized per write on (name, line, hops left),
in a context findRawStateWrites makes fresh per write, never in module
state. The verdict depends only on those and the scanned lines, so no
verdict moves: the old and new script agree on all 650 tracked files
that call fs.writeFileSync (964 findings, 0 differences). The 60-name
file now scans in about 1.1 s locally.
findRawStateWrites takes an optional stats object and reports
assignmentLookups, the nearestPrecedingAssignment calls it made (the
count is overwritten per call, not accumulated). A new row bounds that
count on the crafted shape at 20 names without a clock: 33215 calls
without the memo (the row fails), 1694 with it, against a bound of
2 * N * (N + 1) * MAX_TARGET_RESOLUTION_HOPS = 3360 (at
MAX_TARGET_RESOLUTION_HOPS = 4).
The bound is per write only. Residuals, measured locally:
- Cost scales with the number of writes, because the memo is rebuilt per
write: the 60-name file with the write line repeated 40 times took
48.6 s (634960 lookups).
- The assignment regex prefix backtracks on a long whitespace run: one
40000-space line holding the name, plus one write, took 0.9 s (80000
spaces: 3.1 s); with 20 writes, 17.6 s. origin/next's Axis 2 took 2 to
8 ms on the same inputs.
…ne and shared across a file's writes
A line holding a variable's name and a long whitespace run stalled the
raw-write scan, and the cost of a file grew with its number of writes.
One `const p<40000 spaces>x;` line followed by 20 writes took 10.8 s
locally, 22.6 s in review.
- The assignment regex matched `(?:^|[^.\w$])(?:const|let|var)?\s*NAME`,
so every position of a whitespace run could start a match and
backtrack over the rest of the run. Each match now starts at the name
and a lookbehind reads the text before it; the language and the
captured right-hand side are unchanged, and Axis 3 reads through the
same regex. The typed form `[^=;]+?\s*=` is written `[^=;]+=` (same
language). The for...of binding regex had the same overlap
(`\s+(.+?)\s+of\b`); it now reads `(.*?\S)\s+of\b` after taking the
keyword's whitespace whole, which reads the same pattern.
- nearestPrecedingAssignment takes an opt-in `cache`, one per file for
Axis 2: each line is read at most once per name across every write,
and the write's own line, cut at the call, is read per write
(`firstLine`, never cached).
- A name's verdict below a write's line depends on the write only
through whether a body boundary lies after that line, so those
verdicts are shared by every write in the file, keyed on that flag.
The boundary rule reads a per-file count of boundary lines instead of
joining the lines between per write.
- findRawStateWrites no longer throws on a null `stats`, and reports
`linesRead` beside `assignmentLookups`.
Probe, before -> after (local, ms): the whitespace line with N = 1, 5,
20 writes: 508, 2590, 10791 -> 5, 3, 6. Same with a type annotation:
1023 -> 4 (N = 1); with a for...of header: 985 -> 4 (N = 1); the
for...of header with the run also after its keyword did not finish in
300 s before, 5 ms now. The 60-name shape with 40 writes: 24334 -> 106.
One assignment, `{ }` and 8000 writes: 6633 -> 32.
Old and new script compared: every tracked .cts/.cjs/.ts/.js/.mjs file,
each line read for each identifier on it with no options and with the
Axis 2 options, plus findRawStateWrites and findUnstrippedContentWrites
per file (1715 files, 12.2M comparisons); 4.5M comparisons over 150000
fuzzed lines; 2.4M fuzzed binding lines; 150000 fuzzed files (44512
with findings). 0 differences. Mutants of the assignment regex, the loop regex
and the shared verdict key each show differences in the same harness.
New rows: lines read are bounded on a 200-write file (the row fails
without the cache); lookups are bounded on a 50-write branching file
(19950 lookups before, 1379 now, bound 1880); five whitespace shapes
keep their verdicts (time is not asserted); a null or undefined `stats`
changes no finding. The work-bound comment states the rule, not a
review.
Not bounded: a line holding many writes is read once per write, and a
call left open takes the rest of the file as its target. 8000 writes on
one 200 KB line: 2209 -> 627 ms; 8000 open calls on one line: 6341 ->
5292 ms.
…ne terminator after a whitespace run stays linear The assignment tail `\s*=\s*([^=].*)$` kept the quadratic shape the prefix had: `.` does not match `\r`, U+2028 or U+2029, so when one follows a long whitespace run after `=`, each shorter run is tried and rescanned up to it. `const p =<40000 spaces>x;\r` cost 1613 ms per read on the previous head (32255 ms with 20 writes), and 531 ms per file after the per-file cache. origin/next's Axis 3 reads through the same tail (2268 ms for one such line there). - assignmentHeadRegex matches up to the `=` and its whole whitespace run (a lookahead and backreference, as no possessive quantifier exists). assignedValue then decides the old tail from that one position: the capture could only start at the end of the run, or one character before it when that end is `=` or the end of the line, and `.*$` holds only when no line terminator follows. A failed head resumes one character on. The result is unchanged. - findRawStateWrites reports `assignmentMatches`, the heads decided. Probe, before (472cfb2) -> after (ms): `\r` ending, N = 1 and 20 writes: 1613, 32255 -> 1, 1; U+2028 ending: 1822, 36356 -> 1, 1. Old and new script compared again, with `\r`, U+2028, U+2029 and `=` runs added to the fuzz alphabet: 1715 files (12.2M comparisons), 4.5M comparisons over 150000 fuzzed lines, 150000 fuzzed files, 2.4M fuzzed binding lines: 0 differences. Four mutants (a stricter keyword form, a greedy loop pattern, the shared verdict key without its boundary flag, and an off-by-one in the tail decision) show 7156, 38836, 23 and 186 differences. Rows: a `\r` or U+2028 run decides at most one head per occurrence of the name, with verdicts pinned; two writes across a `}` keep different verdicts (pins the boundary flag in the shared key). Two test comments that cited a review now state the rule. The header now also discloses a line that repeats a declaration or `for` header of the name many times as quadratic in that line (180 KB of `const p: `: 4485 -> 1659 ms; 260 KB of `for (const p `: 3331 -> 3573 ms).
…write past the cap as unresolved A call left open took the rest of the file as its first argument, once per call, and each of many writes on one line re-read the line before it, so both shapes cost time quadratic in the file or the line. - MAX_RAW_WRITE_TEXT_LENGTH (4096, exported) bounds both reads. The longest first argument of a write over the 1715 tracked script and source files is 115 characters and the longest line before a call 129, so the cap is over 30 times either. - A write past the cap is not resolved. It is reported as raw_state_write_unresolved on the raw-write axis, at most once per line, so the cap adds reports and never drops one. - captureFirstArg takes an optional limit and reads at most limit + 1 characters. The line before the call is measured before it is sliced. - A finding's source is built once per line. - The stats seam gains argumentChars and ownLineChars. New rows pin the cap at limit - 1, limit and limit + 1 for both reads, count the work for calls left open and for many writes on one line, and run the CLI on a call left open. - The header now describes the cap and lists what it does not bound.
… raw character The row that ends a whitespace run with a line terminator held a raw U+2028 in its string literal. It is valid JavaScript but invisible in review and trips invisible-character scanners. The escape is the same string. A code-point scan of the guard and its test file finds no other invisible character.
…report the writes past the budget as unresolved
Shapes of the resolver this change adds still cost time superlinear in a
file: many writes that each name a different variable, where each name's
first lookup read back to the enclosing named function (distinct names x
lines); a line that repeats a typed head, a for header or a destructuring
head, where each repeat scanned to the end of the line; many writes that
re-read one wide right-hand side; and writes that walk every earlier
assignment of their name past body boundaries.
- MAX_RAW_WRITE_RESOLVER_WORK (250000000, exported) bounds the work of
resolving one file's writes. Over the 1728 tracked script and source
files the most any file takes is 8186334 (tests/state.test.cjs, 1 MB),
and in src/ 160872 (src/commands.cts), so the budget is over 30 times
the first.
- Charged: each write's argument and line before the call, by length; each
nearestPrecedingAssignment call, LOOKUP_WORK (64) plus the name; each line
it reads, by lineReadWork: its length plus LINE_READ_WORK (32), times one
more than its count of ':', '(', '[' and '{' when it holds the name,
since each of those can start a scan to the end of the line; and each
assignment right-hand side it resolves, by length.
- assignmentVerdict keeps an assignment's verdict below the write's line
for later writes, under the same boundary flag as the other shared
verdicts, so a wide right-hand side is read once. The assignment head
regex is built once per name.
- A charge that would pass the budget is not made and its read is not
done. The write that ran out and every later write in the file are
reported as raw_state_write_unresolved, at most once per line.
- The budget is opt-in through the cache, so Axis 3's reads are unchanged.
- The stats seam gains resolverWork. New rows pin the budget at
limit - 1, limit and limit + 1, count the work for distinct names, each
repeated head, a shared right-hand side and boundary walks, and check
that a write after the budget is spent is still reported.
- The header describes the budget in place of the shapes it left
unbounded.
… and find the name in linear time
The resolver budget charged a line read by the line's length, but the
assignment head regex starts with the name, so where the name nearly
matches at every offset (a run of 'a' read for the name 'aaa...') one read
cost line length x name length. Two lines of 100000 'a's and 100 writes of
4000-character names took 22 s and spent a quarter of the budget.
String#includes has the same cost for a name with a repeated half.
- A line read under the budget finds the name's offsets with
Knuth-Morris-Pratt (nameOccurrences), linear in the line, in place of
includes. The head regex and the pattern-binding regex run sticky at
those offsets only, which finds the same matches as the global scan.
- lineReadWork charges (line length + LINE_READ_WORK + occurrences x name
length) x (1 + count of ':', '(', '[' and '{') when the name occurs, and
line length + LINE_READ_WORK when it does not.
- Each name's regexes and failure table are built once (nameReader).
- Over the 1728 tracked script and source files the most any file now
takes is 8267472 (tests/state.test.cjs) and in src/ 163185
(src/commands.cts); MAX_RAW_WRITE_RESOLVER_WORK stays 250000000, 30.2
and 1532 times those.
- A counted row pins the per-occurrence charge and the review's shape; the
budget limit rows follow the new charge.
- Axis 3 does not pass a cache, so its reads are unchanged.
- The text cap's comment now cites the measurement after the rebase: over
the 1728 tracked files the longest first argument and the longest line
before a call are each 129 characters. The cap commit's message keeps
the figures measured before the rebase, over 1715 files (115 and 129).
… a resolved name, and search for names with indexOf Each new name compiles its own regexes (about 70 us and 7 KB for a short name, 350 us and 30 KB for a 256-character one), and that was not charged: one distinct short name per write spent the budget in 44 s and 3.8 GB, and 4000-character distinct names ran out of heap inside findRawStateWrites, which is a crash, not a reported write. - Building a name's reader is charged READER_WORK (10000) plus READER_WORK_PER_CHAR (128) per character. The charge also bounds the memory the readers hold for the rest of the file. - MAX_RESOLVED_NAME_LENGTH (256): a longer name is charged as the whole budget, so its write and every later write in the file are reported unresolved. Over the 1728 tracked script and source files the longest name the resolver looks up is 32 characters (27 in src/), and the longest identifier in src/ is 52. - With names that short, the native indexOf search is cheap on the adversarial patterns probed (self-overlapping, near-miss, periodic and Fibonacci names up to 256 characters on 1 MB lines), so the Knuth-Morris-Pratt search is removed and indexOf is used again. The search for a name on a line stops once its occurrences could no longer be charged, so the read that runs out of budget does not first list every occurrence. - The stats seam gains readersBuilt. New rows: one reader per distinct name, charged by length, for short and long names; and the name cap at limit - 1, limit and limit + 1. The text-cap row pads its name with spaces, so it no longer depends on the name cap. - Over the 1728 tracked files the most any file now takes is 9315920 (tests/state.test.cjs) and in src/ 308033 (src/commands.cts), so the budget of 250000000 is 26.8 and 812 times those. The most readers any file builds is 110 (18 in src/).
… one pass identifiersIn dropped quoted strings with /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"/g, which, from every quote with no closer, scans to the end of the text and fails, then tries the next quote. A run of escaped quotes therefore cost time quadratic in its length, but was charged by length. A template literal of escaped quotes reaches it as one argument (the argument capture treats a backtick as a string; this regex does not). Nested writes sharing one such literal took 26.6 s for a 999 KB file, and one 400 KB right-hand side took 37.8 s. - stripQuotedStrings gives the same result in one pass. A quote with no closer records where its failed scan stopped, and quotes of the same kind before that point are not tried: the failed scan passed each of them as the escaped half of a backslash pair, so a scan from one stops at the same point. A backslash before a line terminator also stops a scan, as `.` does not match one. - Same output as the regex on 2500000 fuzzed strings (quotes of both kinds, backslash runs, \n, \r, U+2028, U+2029, backticks) and on every line and whole file of the 1728 tracked files. - The backtick handling is unchanged in both places. - The stats seam gains stringScanChars. A counted row puts a run of 1000 escaped quotes in a target and in a right-hand side; each character is examined at most three times.
8cea5f8 to
1830f52
Compare
|
Review 5608091717 is addressed at
Evidence: bounds, probes, rows, mutants, commandsScope. The timings below hold for the shapes probed, listed in the tables. They are not a proof for all inputs. Probing the resolver this round found slower shapes than the one in your review; they are fixed here. The bounds
Your probe and related shapes (ms, one run each, measured at
Resolver shapes found while probing (ms, measured at A file that spends the budget reports its remaining writes as unresolved.
Rows (
Fail-first and mutants
Differential,
Commands (at
|
Withdrawn: review produced outside the required /review-open-prs procedure; run is being restarted.
…unresolved, even one naming statePath Comment-only. The budget header and the MAX_RAW_WRITE_RESOLVER_WORK comment said only writes "not yet found to name the state path" are downgraded; once the budget is spent no argument is read, so a later fs.writeFileSync(statePath, c) is reported as raw_state_write_unresolved too (pinned by the budget test). The nearestPrecedingAssignment cache doc now lists stringScanChars, which findRawStateWrites builds and stripQuotedStrings increments.
|
Review 5622671659 (head
Self-test (head f2aedc2)
|
Enhancement PR
Linked Issue
Closes #5104
Child of epic #4629. Approved as Layer 1 by @trek-e (issuecomment-5891253965).
What this enhancement improves
Axis 2 of
scripts/lint-state-write-path-drift.cjs(findRawStateWrites), the terminal guard that ADR-4629 Decision 5 names for "No raw STATE.md write outside the seam". A rawfs.writeFileSyncagainst STATE.md skips the lock, so it can lose a concurrent update (the #500/#905/#1230 family).Before / After
Before (
next@ a1c7bc7):findRawStateWritesreported 0 for each of these shapes:After: each shape is reported at the line of the
fs.writeFileSync(call, asRAW_STATE_WRITE. The real tree still reportsrawWriteViolations: 0: the prototype measured 0 new findings onsrc/, and this branch confirms it.How it was implemented
All changes are in
scripts/lint-state-write-path-drift.cjs, Axis 2. It stays a string match, with no AST.captureFirstArgcaptures a first argument that follows a line break.statePathnor aSTATE.mdliteral, one resolver,resolvesToStatePath, follows its identifiers back through same-function assignments, at mostMAX_TARGET_RESOLUTION_HOPS(4) deep. It stops at the enclosing namedfunction, the boundarynearestPrecedingAssignmentalready uses. String contents and property names after a.are not resolved. Each hop resumes above the assignment it followed, so the scan always ends.nearestPrecedingAssignment. Axis 2 calls it directly, with a 4thoptionsargument:typed(a TypeScript annotation between the name and=, read only afterconst/let/varand holding no=or;),stopAtPatternBinding(afor...ofheader or a destructuring declaration that binds the name ends the search as unresolved, which is not flagged) andwithLine(return the line with the text, so the next hop can resume above it). Axis 3 passes no options, so its regex and behavior are unchanged. Thefor...ofstop matters live: without it the rollback loop atsrc/roadmap-upgrade.cts:2395resolved to theconst filePathat:2329(lines onnext@ 62549bf).{,}or=>lies between an assignment and the write, a binding that misses STATE.md does not end the search: it moves on to the next earlier binding of that name. So a literal in a closed sibling block or an arrow body never clears a write. A binding that reaches STATE.md still reports across the boundary. This can only add reports. There is no brace-depth tracker; full scoping is enhance(#4629): the Axis-2 raw STATE.md write guard parses the file instead of matching lines, so scope, strings and other write APIs stop fooling it #5279.findRawStateWritesheader say this axis's zero does not prove no raw writer remains (ADR-3408 Decision 5), and list every verified miss and false positive.+=handling and same-line statement splitting are not in this PR. Each shape the cut gives up is pinned as a known-gap row (expectedbesidecurrentBuggyOutput) and tracked in enhance(#4629): the Axis-2 raw STATE.md write guard parses the file instead of matching lines, so scope, strings and other write APIs stop fooling it #5279.Not closed here (by design): the raw writer at
src/roadmap-upgrade.cts:2345. Its file name is aMapkey filled in another function (miss 3), and no resolver scoped to one function reaches it. Per the ruling on #5104, that half is #5120 (approved; sequenced after #5056 Phase 13): a test-time check on the written path's basename. Until then, this axis's zero does not mean no raw writer exists (ADR-3408 Decision 5).Testing
How I verified the enhancement works
tests/state-write-path-drift-guard.test.cjssection F4 holds:MAX_TARGET_RESOLUTION_HOPS;KNOWN_TAIL_GAPS,KNOWN_SCOPE_GAPS,KNOWN_MATCH_GAPS), each row withexpectedbesidecurrentBuggyOutput;--rootCLI row.next'sscripts/lint-state-write-path-drift.cjsswapped in (no shim needed; the property asserts the hop-bound export first), 25 of the file's 83 tests fail. Against this head all pass.472cfb257onnext@ 755bf26, with emptyHOME/GSD_HOME: the two test files that load this script ran 84 tests, all passing.npm run lint:ciexit 0.rawWriteViolations: 0, and the behavioral identity test,tests/state-transition.test.cjs"enhance(#3408): applyStatePreservation dispatches on the declared policy, not the field — Phase 1 #3468 matrix C1/C2", passes 6/6. Neither proves no raw STATE.md writer exists. This axis is a line scan with the misses listed infindRawStateWrites's header; the syntax-tree pass is enhance(#4629): the Axis-2 raw STATE.md write guard parses the file instead of matching lines, so scope, strings and other write APIs stop fooling it #5279 and the run-time check is enhance(#4629): the test suite fails when product code raw-writes a file named STATE.md outside the write seam #5120.next's guard vs this head, both axes, every source file undersrc/,scripts/,gsd-core/bin/,hooks/,bin/, 792 files): 0 verdict changes. Axis 3 verdicts are part of that comparison (0 changes).Platforms tested
Runtimes tested
Scope confirmation
Scope held: Axis 2, opt-in options on
nearestPrecedingAssignment(Axis 3 passes none, output unchanged), and the test file. Nosrc/change, no allowlist, no new dependency.for...inis not treated as a binding, because the approval namedfor...ofand destructuring.Documentation
docs/to reflect this changedocs/content added in this PR is written in Englishapply the
no-docslabel or add<!-- docs-exempt: <reason> -->inside eachtriggering changeset fragment and leave a comment explaining why.
Internal lint guard only. No changeset fragment exists, so
lint:docshas nothing to trigger on. ADR-4629 cites the guard at:606,564. Those line numbers were already stale onnext, and I left the ADR untouched.Checklist
Closes #NNN— PR will be auto-closed if missingapproved-enhancementlabel — PR will be closed if missingnpm test), on the head commit per CI.changeset/fragment added (npm run changeset -- --type Changed --pr <NNN> --body "...") — orno-changeloglabel applied if not user-facingBreaking changes
None. The guard runs only in
lint:ci, it stays green onnext, and its--jsonshape is unchanged.Round 3 (2026-10-21)
Addresses review 5563197184 at
8cea5f843, rebased onnext812ddb5fe. Detail, probes and commands are in the round-3 PR comment.scripts/lint-state-write-path-drift.cjsis now linear in each line and shared across a file's writes: the assignment read is split intoassignmentHeadRegexandassignedValue,LOOP_BINDING_REno longer overlaps, andnearestPrecedingAssignmentgains opt-incacheandfirstLineoptions used by Axis 2 only. The review's probe went from 490 / 9894 ms to 5 / 3 ms (N = 1, 20). The old and new scripts differ on 0 inputs over the tracked tree and the fuzz corpora. Lines that repeat a write, an open call or a declaration many times stayed quadratic in one line's length; round 4 bounds them (below).nullorundefinedstatsis ignored; row added.Round 4 (2026-10-22)
Addresses review 5608091717 at
1830f5210, rebased onnext26387bc83. Detail, probes and commands are in the round-4 PR comment.MAX_RAW_WRITE_TEXT_LENGTH = 4096caps each write's first argument and its line before the call.MAX_RAW_WRITE_RESOLVER_WORK = 250000000caps the resolver's counted work per file: argument and line lengths, each lookup, each name's compiled reader, each line read (charged per occurrence of the name, and per:,(,[or{when it occurs), and each right-hand side read.MAX_RESOLVED_NAME_LENGTH = 256caps the length of a name the resolver follows.raw_state_write_unresolvedon the raw-write axis, at most once per line, so the bounds add reports and never drop one.argumentChars,ownLineChars,resolverWork,readersBuiltandstringScanChars.