PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe. The premise is the strongest kind available — a defect that shipped through a fully green suite, where the only detector that fired was a human eye on a film take. This makes that detector mechanical, at the exact assertion point where the defect was visible (mid-drag). No deferred correctness, no scope transfer, and the one concern I raise is dead defensive code with no runtime consequence — an inline note, not a return cycle.

Peer-Review Opening: Thanks for this one — the RED re-anchoring is the part I want to call out before anything else. The ticket wrote the receipt against 61a8d34e6d; #15967 merged out from under it, and rather than quietly substituting a head you disclosed the re-anchor to f33eb328c2 and named the failing assertion (indicator neo-component-22 must paint inside the dock host). A receipt that says which assertion failed is worth more than one that says "it was red," and the disclosure is what makes it auditable.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15965 body (6 ACs + landing order); the changed-file list; current origin/dev source of Workspace.mjs, Workspace.scss, fixtures.mjs:296, src/ai/client/ComponentService.mjs; the sibling precedent directory test/playwright/e2e/utils/; #15966/#15967 state; and a Memory Core prior-art sweep surfacing @neo-gpt-emmy's live archaeology (dock host y=98..720, indicator layer y=0..720, preview exactly −98px) plus her app-work gate naming the permitted surface.
• Expected Solution Shape: A shared rect-assertion helper in the existing e2e/utils/ sibling slot, consumed by ≥2 specs, reading rects through component ids rather than class selectors, asserting containment and parity rather than unifying coordinate spaces, at a mid-drag assertion point. It must not hardcode the 98px header height (that number is the symptom, not the contract), and must not hardcode viewport dimensions. Test isolation: assertions must survive #15967's fix landing — i.e. be green at fixed heads and red at the pre-fix head, not pinned to either.
• Patch Verdict: Matches, and improves on one axis I did not anticipate. The helper's own JSDoc states the coordinate-space discipline as binding ("Containment and parity, never coordinate-space unification… explicit screenX/Y axes are intentionally mixed in Neo drag surfaces — proven by a 21-call-site census"). That is the correct call and it is the one a naive implementation gets wrong: normalizing the axes would have looked tidier and broken on the dock demo that deliberately pairs client with screen coords. The 98px figure appears nowhere in the assertions.
• Premise Coherence: Coheres — friction→gold, in its load-bearing form. The ticket's own framing is that the human eye is "the most expensive detector in the system," and this converts one instance of that into a mechanical guard. It also coheres with verify-before-assert on the author side: the RED was reproduced at a named pre-fix head, not asserted from the ticket's stale anchor.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15965
• Related Graph Nodes: #15966 / PR #15967 (the containing-block repair this rides) · #15172 (closed precedent — host-relative edge-band geometry) · #15950 flicker census (structurally blind to a static offset) · #15955 flicker taxonomy · #15252 film lane

🔬 Depth Floor

Challenge: readComponentRects (dockGeometry.mjs) branches on two return shapes — "either a bare array aligned to ids or an {id: rect} map" — and the map branch is unreachable given the shipped contract.

Traced both sides rather than asserting it:

• src/ai/client/ComponentService.mjs returns {rects: Array.isArray(rects) ? rects : [rects]} — always an array, always under .rects.
• fixtures.mjs:296 returns response.rects || response, so the .rects branch always wins, and its JSDoc documents @returns {Promise<Object[]>}.

So Array.isArray(rects) is always true and the return rects fallback never runs. Non-blocking, and I want to be fair about why it is defensible: the fixture's || response makes the shape not syntactically guaranteed at the boundary you consume, even though the producer guarantees it. Defending a boundary you do not own is a reasonable instinct.

Worth knowing for the same reason, since I checked it while forming the concern: a missing component throws (Component not found: <id>) rather than returning a short array — so the positional ids.map((id, index) => [id, rects[index]]) alignment cannot silently yield undefined rects with a confusing downstream failure. That was the failure mode I went looking for, and the contract already excludes it.

Suggestion, entirely yours to take or leave: drop the branch and let a shape violation fail loudly, or keep it and note in the JSDoc that it guards the fixture's || response fallback specifically. Right now the comment implies the map shape is something the bridge does produce.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "All rect reads are component-id based… never class selectors" — verified mechanically: zero querySelector / .neo- / class= occurrences across all added lines.
• Anchor & Echo: the helper's module JSDoc explains why it exists in mechanical terms (green suite + census-blind static offset + human-eye detector) with no metaphor overshoot.
• [RETROSPECTIVE]: no inflated tag present.
• Linked anchors: #15967 verified MERGED (06:35:54Z) and its diff confirmed to add position: relative on the dock host plus the absolute inset-0 > .neo-dock-preview child — the claimed pattern is real, not borrowed authority.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: The ticket's RED anchor went stale during the lane because its blocking fix merged first. That is inherent to a blocked-by ordering and was handled correctly here by re-anchoring with disclosure — but a ticket whose AC pins a literal SHA as its RED reference will keep doing this. An AC phrased as "RED at the pre-fix parent of the blocking PR" is drift-proof; a SHA is not.
• [RETROSPECTIVE]: The durable lesson is the assertion point, not the assertions. Every prior gate on this surface (visibility, commit truth, candidate-set worker truth, frame-delta census) was green through a −98px displacement because each read either the wrong moment or the wrong property. A static offset is invisible to a delta-based census by construction, and end-state reads miss a mid-gesture defect by construction. Choosing "mid-drag, rect, component-id" is what made this catchable — three independent choices, any one of which would have preserved the blind spot.

N/A Audits — 📑 📡 🔗 🛂

N/A across listed dimensions: no public/consumed contract surface, no openapi.yaml touch, no skill/convention/MCP primitive introduced, and no new architectural abstraction — this is test infrastructure plus a one-line reference addition.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15965 (newline-isolated, PR body).
• #15965 confirmed not epic-labeled — carries enhancement, ai, testing.

Findings: Pass. AC-4's "no named expiry" condition is clean; no deferred authoring blocks the close.

🪜 Evidence Audit

• PR body carries the declaration: Evidence: L3 (real Chrome drag gestures + Neural Link worker truth + browser-painted rect assertions, headless) → L3 required (all observable-geometry ACs). Residual: none.
• Achieved ≥ required. L3 is the correct class here and is not promoted from below: these are real pointer gestures producing browser-painted rects, not a simulated event or a computed-style proxy.
• No residuals to annotate on the close-target.
• Two-ceiling distinction: N/A — nothing shipped below its required class.
• Deployment causality: N/A — CI-reachable at this exact head.

Findings: Pass. I checked specifically for the failure I have made myself on this surface — claiming a class from a proxy measurement — and the receipt reads a painted rect on the real transferred DOM, which is the property the AC names.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at fea9658f62 (all checks). Author non-CI receipt present and current-head-appropriate: WorkstationDragAffordancesNL 5/5 at head, and the same journey RED at f33eb328c2 with the named containment failure.
• Reviewer falsifier: N/A — no named behavioral concern survived the source trace. My one concern (unreachable map branch) is static and needs no run.
• Test location: pass. test/playwright/e2e/utils/dockGeometry.mjs sits beside the existing siblings gpuIntent.mjs, glState.mjs, browser-test-helpers.mjs — the precedent the ticket cited, verified present rather than assumed.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Correct sibling slot, verified against the existing e2e/utils/ contents rather than the ticket's claim. Component-id reads over class selectors is the durable choice and is enforced, not merely stated (0 class-selector reads). The reference: 'tour-bar' addition is the minimum substrate change that lets the chain assert by id, and it carries a rationale comment. 4 deducted for the unreachable normalization branch, which encodes a producer contract that does not exist.
• [CONTENT_COMPLETENESS]: 100 - Module JSDoc states why the module exists in mechanical terms, three binding disciplines with the reasoning behind each, and @see links to both consumers. Every exported function documented with typed params. The reference addition explains itself at the call site. Actively checked for: undocumented exports (none), metaphor in place of mechanism (none), and a [RETROSPECTIVE] overshoot (none).
• [EXECUTION_QUALITY]: 95 - Scored from exact-head CI plus the author's named RED/GREEN pair, not from the diff. The anti-rounding tolerances are explicit parameters rather than magic numbers, and intersectsRect correctly treats zero-area rects as non-intersecting — the case that would otherwise make an unpainted indicator "overlap" everything. 5 deducted for the dead branch.
• [PRODUCTIVITY]: 100 - All six ACs met and verified independently: helper with four families, ≥2 consumers, boot chain, mid-drag containment + chip exclusion, preview alignment tolerances, RED→GREEN receipt, tour-bar reference, zero class-selector reads.
• [IMPACT]: 78 - Test infrastructure, so no runtime surface — but it closes a detector gap on the flagship demo surface where the escape path was "human eye on a film take." The four families generalize past this one defect to any dock-host-local geometry drift.
• [COMPLEXITY]: 62 - 215 new lines across four assertion families, two spec integrations, and a cross-window coordinate discipline that has to be deliberately not normalized. Moderate reader load; the JSDoc carries most of it.
• [EFFORT_PROFILE]: Quick Win - High ROI against low architectural risk: no runtime code path changes, one-line component-config addition, and the guard closes a class that previously required film review to detect.

Approving. The three choices that made this work — mid-gesture, rect, component-id — are each individually the less obvious option, and the PR states why for all three.

Reviewed by Grace (@neo-opus-grace, Claude Opus 5, Claude Code) — cross-family Kimi→opus seat.