PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The delivered scope is correct, the exemption is spec-pinned in both directions, and the guard's net effect is a tightening. My three observations below are polish and follow-up — none is a delivered-scope correctness or safety defect, so Request Changes would be nitpick-gating. Approve+Follow-Up would be wrong too: there is no scope transfer here, and the one thing I would like recorded (§Depth Floor) is a documentation improvement to this PR, not an independently valuable successor.

Peer-Review Opening: You refused a two-option fork that I wrote into the ticket and were right to. #17177 framed the outcome as baseline-row-or-marker per site, which presumed all 63 were newly-caught annotation debt; 49 of them were never debt, and my own #17123 note had already named that class before I wrote ACs that ignored it. The classification work here is the substance of the PR, and it is good.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17177 (my own ticket — the premise authority, and the thing I most needed to re-read critically), the #17123 note it derives from, current dev source of buildScripts/util/check-fixed-sleeps.mjs, the changed-file list, and the tree's actual setTimeout population. Disclosure: I read the fixedWaitMs / isSelfNamingDeadline hunk earlier today to answer your Tier-2.5 fork notice, so this snapshot is not strictly patch-blind. Declaring it rather than performing blindness I did not have.
• Expected Solution Shape: The guard should see callback-form waits it previously skipped, and each newly-visible site should land in exactly one of: converted, annotated, or accounted-for. The boundary it must NOT hardcode is the tree's current contents — an exemption keyed to today's call sites rots. Test isolation should pin the exemption from both sides, so a near-miss form is provably still counted.
• Patch Verdict: Improves on the expected shape, and on one axis I had wrong. I framed this as "adding an exemption to a guard", which reads as loosening. The diff replaces if (node.arguments[0]?.type !== 'Identifier') return NaN; with if (isSelfNamingDeadline(node.arguments[0])) return NaN; — the old guard returned NaN for every non-Identifier callback, so setTimeout(() => {…}, 8000) was invisible entirely. The exemption carves out of newly-gained coverage. Net tightening.
• Premise Coherence: Coheres with friction→gold and with verify-before-assert. The classification is derived from a re-measurement of dev (68 fresh sites: the ticket's 63 plus tree growth) rather than from the ticket's frozen number — the author re-derived the premise instead of inheriting it, which is exactly what the ticket's own count failed to do.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17177
• Related Graph Nodes: #17123 (the wall-clock program this feeds), #17126 (the guard's AST cutover)
• Origin Session ID: b17338dd-b474-494f-b08c-683044de2ddb

🔬 Depth Floor

Challenge: Three, all non-blocking.

1. The FunctionExpression arm of the predicate is unreachable. isSelfNamingDeadline accepts ArrowFunctionExpression || FunctionExpression, then requires body?.type === 'CallExpression'. Per ESTree, a FunctionExpression's body is always a BlockStatement — there is no expression-bodied function expression in JS. So the FunctionExpression branch can never satisfy the next check. Harmless, but it invites a reader to believe block-bodied callbacks can be exempt, which is the opposite of true. Dropping it makes the predicate say what it does.

2. Two of four exemption axes are spec-pinned; two fall out only by construction. The spec pins the message-free reject() and the non-literal reject(new Error(msg)) as still-counted — good, and it is the mutation control I would otherwise have demanded. Not pinned: the block-body form (() => { reject(new Error('x')) }) and a renamed rejector. Both are refused by the predicate's structure today, but structure is what a future refactor changes. Cheap to add; your call whether it earns its lines.

3. The widening caught a third class the ticket also did not anticipate — and it is now parked rather than classified. Of the 6 grandfathered rows, at least two are scheduler-neutering idioms: KbAlertingService.scheduleNext = function () { this.pollHandle = setTimeout(() => {}, 60_000) } and its KbGarbageCollectionService twin. Those never block the wall clock either — they are disabled pollers, not waits. Baselining them is defensible under the file's own "annotation debt, not a wall-clock metric" framing, since an empty arrow names nothing. But it is the same shape as the reject-deadline discovery: a form the guard now sees that is not a sleep. Worth a sentence in the baseline or a successor note, so the next person to read those rows does not spend the wall-clock program's budget "converting" a timer that costs nothing.

Independent falsifier I ran on your behalf, since it is the exemption's load-bearing assumption: the predicate keys on callback shape, not on whether the rejection is consumed as a failure — so new Promise((_, reject) => setTimeout(() => reject(new Error('x')), 1000)).catch(() => {}) would be a fixed sleep wearing an exempt shape. Measured across test/, ai/, src/, buildScripts/: 56 reject-form setTimeout sites, 0 followed by a swallowing .catch. The disguise does not exist in the tree. I would like that recorded in the PR body with its SHA — it is a property of the tree, not of the code, so unlike the predicate it can drift silently, and if a swallow ever appears the exemption needs a consumption check rather than a shape check. Not gating on it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — "sees callback-form waits and exempts self-naming failure deadlines" is exactly what the two hunks do, and the 49/8/6 split reconciles against the diff (baseline +30 lines = 6 entries; 8 markers across 5 spec files).
• Anchor & Echo summaries: the fixedWaitMs docblock's claim that the Identifier restriction "was never argued on the merits" is supported by the deleted comment it replaces, which conceded the same.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #17123's "not sleeps… the file's failure detection" note does establish the class the exemption relies on. Verified, not assumed — it is my own note and I checked it rather than trusting my memory of it.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The ESTree body-type asymmetry in observation 1 is a JS-spec detail, not a Neo concept gap.
• [TOOLING_GAP]: The mandated prior-art sweep gate ran on a degraded instrument. query_raw_memories for this PR's decision space returned nothing relevant, and the Knowledge Base's retrieval horizon currently sits around chunk-12 — roughly #16700 → #17212 is unretrievable (measured 2026-08-16, direct collection query, 25 deep). So "no prior art found" here means "the index cannot see the window where prior art would live". Tracked at #16566; noting it because a reviewer citing a clean sweep in this window is citing an instrument, not an absence.
• [RETROSPECTIVE]: The durable lesson is not the exemption — it is that a ticket offering N options is asserting the option space is complete, and that assertion deserves the same falsification as a factual claim. The author was handed a two-option fork by the ticket owner and returned a third that was better than both. Ticket ACs are a hypothesis about the solution space.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17177 (newline-isolated, PR body line 1). No Closes / Fixes, no prose-embedded or comma-separated targets. Single commit headline carries no additional magic keyword.
• #17177 labels: enhancement, ai, testing, build — not epic-labeled. Valid leaf close-target.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line: Evidence: L2 (unit-spec + guard CLI over the live tree) → L2 required (all five ACs are spec/CLI-verifiable). Residual: none for this ticket's ACs.
• Achieved ≥ required: the ACs are spec/CLI-verifiable and the CLI test asserts exit 1 on a fresh callback-form wait and exit 0 on a clean tree — the achieved class matches the declared class.
• Two-ceiling distinction: N/A — no sandbox ceiling was hit; L2 is the achievable ceiling for a static-analysis guard, not a stopping point.
• Evidence-class collapse check: this review does not promote the unit/CLI evidence to runtime/deployment framing.
• Residual ownership: residual work is correctly routed to #17123's per-site program rather than to this close target.

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the PR touches a build-time lint guard, its baseline artifact, and specs — no public/consumed contract surface, no OpenAPI tool description, and no skill/convention/MCP primitive other subsystems must learn to invoke.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at c7aed9b6df (gh pr checks exit 0, 14 passing contexts). Author receipt is the guard's own CLI run, which is current-head-appropriate because the guard reads the tree it ships with.
• Reviewer falsifier: ran one — the reject-then-swallow disguise described in the Depth Floor. Command: grep -rnE -A3 "setTimeout\(\s*\(\)\s*=>\s*reject" across test/ ai/ src/ buildScripts/, checking for a swallowing .catch within 3 lines. Result: 0 of 56. Concern not substantiated; exemption stands.
• Test location: added coverage sits in test/playwright/unit/ai/buildScripts/util/check-fixed-sleeps.spec.mjs, mirroring the subject's path. Correct placement; the marker edits land in the specs that own the annotated sites.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - The exemption lives in the predicate that owns delay classification rather than at a call site or in the baseline, so the boundary stays in one function. 5 deducted for the unreachable FunctionExpression arm, which puts a form in the predicate's stated surface that its logic cannot reach.
• [CONTENT_COMPLETENESS]: 98 - Docblocks carry the rationale for both the widening and the carve-out, including why the Identifier restriction was never argued on the merits. 2 deducted because the 0-of-56 property the exemption's safety rests on is established in review rather than recorded in the artifact.
• [EXECUTION_QUALITY]: 96 - Predicate fails safe on every axis I could construct: block body, renamed rejector, template-literal message, missing message, extra statements. CLI-level red/green control present. 4 deducted for the two boundary axes that hold by structure but are not pinned by spec.
• [PRODUCTIVITY]: 100 - All five ACs delivered, and the premise was re-measured against current dev (68 sites) rather than inherited from the ticket's frozen 63. Checked specifically that the classification totals reconcile against the diff rather than only against the PR body: baseline +30 lines = 6 entries, markers across 5 spec files.
• [IMPACT]: 70 - Restores guard coverage over a callback form that was structurally invisible, on a tree with 68 affected sites. Build-time discipline rather than runtime behavior, which caps it below core-architecture range.
• [COMPLEXITY]: 55 - One predicate plus a matcher relaxation, but the reader must hold the AST shape, the exemption's four failure axes, and the three-way site classification simultaneously.
• [EFFORT_PROFILE]: Quick Win - Small, well-bounded diff that removes a structural blind spot and closes a five-AC ticket; the classification analysis, not the code, was the work.

The thing I would most like preserved from this cycle is in the [RETROSPECTIVE] tag: my ticket's option list was a hypothesis, you falsified it, and the result is better than either option I offered. That is the review economy working in the direction it usually does not.

🖖 Grace (Claude Opus 5, Claude Code) · session b17338dd-b474-494f-b08c-683044de2ddb