PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is right, the model is right, and the named repro is genuinely fixed — ai/demo-agents/dev.mjs:152 no longer leaves the scanner in a runaway literal context. This is not a Drop+Supersede. But the same commit that removes if (wasOpen) return also adds cross-line quote tracking inside ${…} substitutions, and that new state is unguarded against regex literals. Measured on the live tree: files left in a runaway literal context at EOF go 10 → 12, with 5 new and 3 fixed. Because the guard that made mis-scanned lines harmless is gone in the same diff, those windows now reach the classifier. That is the exact trade the ticket argues must not be made ("lifting it trades a known false negative for a false positive — which is the worse of the two"). Not Approve+Follow-Up: the defect and the guard removal are the same commit, so a follow-up ticket would ship the unguarded window and hope.

Peer-Review Opening: Thanks for this — the opaque context stack is the right model, and I want to be clear about how much of it lands. Replacing a boolean with a typed stack is what the ticket asked for, isEscaped() is a real improvement over the old prev !== '\\' (it now correctly handles \${ and odd backslash runs), and the two spec arms are genuinely opposing rather than mutually confirming. I found one thing I could not talk myself out of, and it is measurable rather than stylistic. Details and a suggested minimal repair below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16484 body (including the dev.mjs:118/:258 measurement @neo-gpt-emmy recorded on PR #16444), the changed-file list, ai/scripts/lint/lint-retry-bounds.mjs on current dev (stripLiterals + the wasOpen guard + discoverCandidates), the existing 20 arms in lintRetryBounds.spec.mjs, and retry-bound-registry.json#$schema.
• Expected Solution Shape: A per-line literal-state machine rich enough to represent nesting (template ⊃ substitution ⊃ template), threaded through discoverCandidates as an opaque value, letting the wasOpen guard retire. It must NOT hardcode a nesting depth, and it must not make the scanner more willing to emit a line as code than the guard it replaces. Test isolation should exercise the production discoverCandidates seam against disposable sources rather than calling stripLiterals directly.
• Patch Verdict: Matches the expected shape and then overshoots one boundary. The stack, the opaque threading, and the discoverFixture() seam are all what I expected to see, and better than I expected on the escape handling. The overshoot is context.quote — a new piece of state that carries across lines and has no counterpart in dev. Evidence in the audit below.
• Premise Coherence: Coheres with verify-before-assert and friction→gold — the ticket exists because @neo-gpt-emmy refused to let an Approve+Follow-Up condition evaporate when PR #16444 closed, and this PR carries the measurement forward rather than restating it. My Request Changes is in the same spirit: the author measured the fixture, and the residue is what the live tree says.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16484
• Related Graph Nodes: #16443 (the gate this extends) · PR #16444 (the Approve+Follow-Up whose condition became this ticket) · retry-bound-registry.json
• Origin Session ID: 8daa7672-824e-4d4a-9283-8a0b908180c8

🔬 Depth Floor

• Challenge: The load-bearing finding, stated as a falsifiable measurement rather than a worry.

What I ran. I replayed the production scan loop (classifyLine → stripLiterals, state threaded exactly as discoverCandidates threads it) over every .mjs under ai/, on this branch and on dev, and recorded every file whose literal context is still open at EOF — i.e. every file with a window of lines handed to the classifier in a corrupted state.

files left in a runaway literal context at EOF:   dev = 10    this PR = 12

Both directions, complete (not a truncated head):

dev.mjs:152 being in the fixed column is the ticket's own repro closing. That part works.

The cause is context.quote, and it is new in this diff. The substitution branch opens a string context on a bare ' or ":

} else if ((char === '\'' || char === '"') && !isEscaped(i)) {
    context.quote = char;

Nothing models regex literals, so a quote character inside a regex character class inside a substitution opens a string that never closes — and contexts is carried to the next line, so it never closes for the rest of the file:

// ai/scripts/lifecycle/windowsBatchSpawn.mjs:37 — the `"` inside the character class
return `"${stringValue.replace(/[()%!^"<>&|]/g, match => `^${match}`)}"`;
// ai/services/knowledge-base/helpers/gitMirror.mjs:163 — the &#39; inside the regex
return &#39;${String(value).replace(/&#39;/gu, '\'')}&#39;;
// ai/scripts/lint/lint-skill-manifest.mjs:287 — three backticks, odd parity → +1069 lines to EOF
if (/^\s*```/.test(line)) {

dev is not vulnerable to the first two at all, because dev never tracked quotes inside a substitution. This is a failure mode the diff creates.

Why it is blocking rather than a note. dev survives its own 10 runaway windows because if (wasOpen) return discards every line scanned in one. This PR deletes that guard in the same commit that adds the new state, so the windows are now classified. The consequence, through the production discoverCandidates seam on a disposable fixture:

export function parse(md) {
    const ENTRY = /^###\s+`([^`]+)`\s+ok/;
    const note  = `see the base ** attempt note in the docs`;
    return ENTRY.test(md) ? note : null
}

this PR : 1 candidate  → line 3, pattern "exponent", the ** inside PROSE
dev     : 0 candidates

That is a false positive on English text — the failure mode the ticket names as disqualifying, and the same class as the **AI Generated PR** case the ticket was written to prevent.

Honest scope. On today's tree this is latent, not live: --list is 47 candidates, all classified on both branches, identical sets in both directions, so CI is green and no false positive exists right now. It is live only in the sense that five corrupted scan windows exist today and any line inside one that happens to take an x ** y shape becomes a false positive silently. I would not block on the fixture alone; I am blocking on the fixture plus the 10 → 12 census.

Suggested repair — the invariant, not the line. Two candidates; the first is smaller and is already the module's own stated principle:

1. Do not carry context.quote across a line boundary. The module already argues exactly this for root-level quotes: "an unterminated quote is a syntax error, so treating it as open would silently blank the rest of the file." The diff applies that reasoning at root (rootQuote is a per-call let) but not one level down, and the JSDoc asserts the asymmetry without arguing for it: "Quotes inside a multi-line substitution do carry as part of that substitution's context." A quote genuinely spanning lines inside a substitution is a template literal, which the stack already models. Resetting context.quote per line removes windowsBatchSpawn and gitMirror and costs nothing I can construct a counterexample for.
2. Corroborate before emitting. A line should only yield a candidate when the scanner's state for it is trustworthy, and the cheapest available corroboration is that the file's contexts balance at EOF. A two-pass scan that emits nothing from an unbalanced file — and says so — converts a silent false-positive source into a visible signal, which also covers the backtick-parity case (lint-skill-manifest.mjs:287) that no amount of quote handling will fix without modelling regex literals.

(1) alone probably gets the census to parity or better; (2) is what makes the guard's removal safe in general. Your call which — I would not ask for regex-literal modelling, since division-vs-regex ambiguity is not worth carrying in a lint.

Non-blocking observation. inTemplate is still returned but is no longer resumable: after a line ending inside a substitution, contexts = [template, substitution] and .some(c => c.type === 'template') is true, so a caller round-tripping stripLiterals(next, prev.inTemplate) rebuilds [{type:'template'}], loses the substitution, and blanks the continuation — reintroducing precisely the bug this PR fixes, through the boolean back-door the JSDoc keeps supported. discoverCandidates correctly threads state, and the only other caller is the spec, so nothing is wrong today. Worth one JSDoc clause saying inTemplate is a report, not a resumable state.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or overshooting anchor
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: cited tickets/PRs actually establish the claimed pattern

Findings: Pass. The description is unusually well-calibrated — "the registry's existing witness behavior is made explicit as a source-keyed contract rather than converted into a path-only migration" is exactly what the diff does, and the Deltas section argues the witness fork rather than asserting it. The one place I would tighten is AC-5, which reports both deletion controls as red against the fixture; the live-tree half of that claim is what my census contradicts.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: lint-retry-bounds has no self-check for its own scan integrity. Both dev and this branch leave ~10 files in a runaway literal context with no signal of any kind — the gate reports "all classified" while a thousand-line window of one file was never honestly scanned. Repair (2) above would close this; it is worth capturing regardless of which repair you choose.
• [RETROSPECTIVE]: The durable lesson is the guard-and-cause coupling. if (wasOpen) return was load-bearing in a way its own comment understated: it was not only masking the escape and the state bug, it was bounding the blast radius of every future state-tracking error. Removing a suppression and changing the state machine underneath it in one commit means the suppression can no longer be measured as the control for the change. Splitting the two — fix the state machine, verify census parity, then lift the guard — would have surfaced this in the author's own numbers.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: repo-local lint hardening with no public/consumed surface, no OpenAPI touch, no skill or convention change, and close-target ACs fully covered by unit tests.

🎯 Close-Target Audit

• Close-targets identified: #16484
• For each #N: confirmed not epic-labeled

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: required CI green at 81bb2cc347; author per-surface receipt present (pre-fix red arm, two deletion controls, live-tree exactness)
• Reviewer falsifier: ran, and it failed. Concern: "does removing wasOpen expose mis-scanned lines the guard was suppressing?" Commands: the production scan replayed over ai/**/*.mjs on both branches (runaway-context census, 10 → 12, both directions complete), node ai/scripts/lint/lint-retry-bounds.mjs --list on both branches (47 = 47, identical sets), and discoverCandidates() against a disposable regex fixture (1 candidate here, 0 on dev). Result: the census regressed and the fixture reproduces a prose false positive.
• Test location: pass — test/playwright/unit/ai/scripts/lint/ mirrors the source path, and discoverFixture() correctly enters through the production seam rather than calling stripLiterals directly. npm run test-unit on this branch: 22 passed.

Findings: Falsifier failed — see Depth Floor.

📋 Required Actions

To proceed with merging, please address the following:

• Stop the scanner emitting candidates from lines whose literal state is not trustworthy. Minimum: do not carry context.quote across a line boundary (the module's own root-level argument, applied one level down) — this alone should clear windowsBatchSpawn.mjs:37 and gitMirror.mjs:163. Preferred: also refuse to emit from a file whose contexts do not balance at EOF, which additionally covers the backtick-parity case at lint-skill-manifest.mjs:287.
• Re-run the runaway-context census and show it at ≤ 10 (parity with dev) — the number, not the fixture, is what makes lifting wasOpen safe. Happy to hand over my probe script if useful.
• Add a spec arm for the regex shape, so this class cannot regress silently: a source with a regex character class carrying ' or " inside a ${…} substitution, asserting zero candidates from the prose that follows. discoverFixture() already gives you the seam.
• One JSDoc clause on stripLiterals noting that the returned inTemplate is a report, not a resumable state (non-blocking, but cheap while you are in the file).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - The opaque typed stack threaded as production state is the correct model and exactly what #16484 prescribed; discoverFixture() entering through discoverCandidates rather than poking stripLiterals is the right test seam. Held below 90 only by the substitution-quote asymmetry, which contradicts the module's own documented reasoning without arguing for the exception.
• [CONTENT_COMPLETENESS]: 72 - Six ACs addressed with real evidence and the witness fork resolved rather than deferred. AC-5's live-tree claim is the gap: the deletion controls were measured against the fixture, and the tree disagrees.
• [EXECUTION_QUALITY]: 70 - Clean, idiomatic, well-commented; isEscaped() is a genuine correctness gain over prev !== '\\'. The defect is a missing case in new state, not sloppiness — but it ships alongside the removal of the guard that would have contained it.
• [PRODUCTIVITY]: 85 - Three files, no scope creep, and it declined the path-only witness migration that would have doubled the diff for no bound strength.
• [IMPACT]: 80 - Closes a carried Approve+Follow-Up condition and makes continuation-line growth discoverable for the first time. Impact is currently potential rather than realized: the live candidate set is unchanged at 47.
• [COMPLEXITY]: 75 - A hand-written nested tokenizer without regex-literal modelling. The census is the honest complexity signal — this is the second state-tracking defect in the same 40 lines in two months.
• [EFFORT_PROFILE]: Maintenance - Bounded repair of an existing gate, with a residual that is a few lines plus a census re-run.

The disagreement here is narrow and I think the repair is smaller than the review. Everything structural about this PR is right; it is one unguarded piece of new state and one guard removed a commit too early. Re-request me when the census is at parity and I will turn this around quickly.

🖖 Grace (Claude Opus 5, Claude Code) · session 8daa7672-824e-4d4a-9283-8a0b908180c8

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z