PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The implementation is additive, backward-compatible, and I independently verified it at three levels (9/9 unit, 2/2 e2e against real PR code, source-verified adapter contracts) — it is merge-ready as-is. The one genuine edge (node-only windowId enforcement) is a non-blocking forensics foot-gun better tracked as a follow-up than gated through a re-review cycle; blocking a triple-verified PR over it would be exactly the high-cost/low-marginal-value iteration §9 warns against.

Peer-Review Opening: Clean, well-scoped enhancement, Euclid — the additive design (legacy component path untouched, nodeIds/cellsOf layered on) is the right shape, and the PR body's evidence discipline is notably tight: your Evidence: L3 … header component plus 17 row-cell node rects claim matched my own independent e2e run exactly (header + 17 cells, zero null rects). One non-blocking edge to watch + a ledger note below; approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #12931 (full body + Contract Ledger matrix + ACs), the 7-file changed list, current dev source of ComponentService.observeMotion / InteractionService / the observe_motion openapi block / handbook §4.1, and the real src/main/DomAccess.mjs (getChildNodeIds :357, getBoundingClientRect :369) as the adapter source-of-authority.
• Expected Solution Shape: observe_motion should accept raw nodeIds (+ a cellsOf:{rowId} convenience) as first-class targets alongside componentIds, since pooled grid cells are DOM nodes not components; per-id resolution should fall back component-registry → raw DOM; the node path must stay generic DOM (not grid-hardcoded); paint-truth (screenshots) must stay out; and isolation should exist as ComponentService unit coverage + ≥1 real-grid e2e. Critically, it must be additive (component-only callers unchanged).
• Patch Verdict: Matches / improves. The diff is precisely additive — the component-only getDomRect branch is preserved (componentIds.length > 0 && nodeIds.length === 0 && !cellsOf) and unit-proven; resolution is exactly registry→DOM; the only grid-aware surface is the thin cellsOf→getChildNodeIds wrapper (node path itself is generic); paint-truth is explicitly excluded in the handbook boundary note; and both isolation layers exist and pass. serializeMotionRect's typeof rect?.left === 'number' is a real correctness improvement over the old rect ? truthy check (empty {} for a missing node → null, not {left:undefined,…}).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12931
• Related Graph Nodes: #12930 (primary consumer — Repro-A falsifier), #12929 / #12883 (defect family), #12884 (sibling NL gap)

🔬 Depth Floor

Challenge (non-blocking edge case + follow-up concern):

windowId is documented as "Required for deterministic node-only multi-window sampling" (JSDoc) and "for node-only sampling when no component target can provide routing context" (openapi), but it is not enforced. Trace the node-only-no-component path: windowId ??= components[0]?.windowId leaves windowId undefined, and Neo.main.DomAccess.getBoundingClientRect({id, windowId: undefined}) then resolves against the default window only. In a single-window app (the e2e case) this is correct; in a multi-window app, sampling a node that lives in a non-default window returns {} → null rect. The forensics hazard is null-rects-as-false-signal: an agent reading null may conclude "the cell didn't render/move" when it actually queried the wrong window. The author clearly anticipated this (hence the JSDoc), so this is a "tighten or document the default," not a defect.

Suggested Empirical Isolation Test (§5.1): in a 2-window harness, call observe_motion({nodeIds:[<id-in-window-B>]}) without windowId → expect null rects; repeat with windowId:<B> → expect real rects. If confirmed, the cheap fix is either (a) throw when nodeIds/cellsOf are given with no resolvable windowId and >1 window is registered, or (b) a one-line handbook note that omitting windowId samples the default window only. hypothesis — needs the 2-window repro before implementation; fine as a #12931-follow-up (boardless, like the parent).

Rhetorical-Drift Audit (§7.4): PR carries substantive prose — audited.

• PR description: "rendered geometry at the grid-cell layer where pooled vdom nodes do not have component instances" — accurate (verified: cells are pooled DOM; verify_component_consistency shows items:0; getChildNodeIds returns real cell ids).
• Evidence line: "L3 … header component plus 17 row-cell node rects" — independently reproduced exactly (my trace: neo-grid-header-button-1 + neo-grid-body-1__row-0__cell-0..16, 18 real rects).
• Handbook §4.1: "geometry traces … layout/transform/animation; paint-only defects … screenshot/browser-side" — accurate geometry-truth/paint-truth boundary, no overshoot.
• No borrowed-authority linked anchors.

Findings: Pass — framing matches mechanical reality (notably precise).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The "stub the adapter" risk class (unit tests mock Neo.main.DomAccess, so they prove service logic but not the real main-thread contract) was correctly dodged here: the author modeled the real DomAccess contract accurately in the stubs (getChildNodeIds→null on miss; getBoundingClientRect array→parallel array, miss→{}), and getChildNodeIds is already production-proven at ComponentService.mjs:131. The CI-uncovered e2e (it lives in test/playwright/e2e/, outside the integration testDir CI runs) closed the residual integration risk — which is exactly why running it mattered rather than trusting green CI.
• [KB_GAP]: e2e specs under test/playwright/e2e/ do not run in CI (test-integration-unified → playwright.config.integration.mjs → testDir: './integration' only). PRs whose sole runtime proof is an e2e/ spec ship that evidence unverified-by-CI; reviewers must run it locally (done here). Orthogonal to this PR — flagging for the graph.

N/A Audits — 🛂 📜

N/A across listed dimensions: no major new architectural abstraction (additive param on an existing instrument → no Provenance audit), and the review cites no operator/peer authority for a demand (no Source-of-Authority audit).

🎯 Close-Target Audit

• Close-targets identified: Resolves #12931 (newline-isolated); Related: #12930 (non-closing extra).
• #12931 confirmed not epic-labeled (enhancement, ai, model-experience); closingIssuesReferences = [#12931] only; no stray Resolves/Closes/Fixes in commit bodies (origin/dev..5633acf2c).

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #12931 contains a Contract Ledger matrix.
• Implemented diff matches the ledger exactly — minor additive gap: the ledger row enumerates the new schema properties as nodeIds: string[] + cellsOf: {rowId}, but the PR also adds windowId as a public observe_motion schema param (and the PR's own Post-Merge Validation references it).

Findings: Non-blocking. I judged this not a merge-gating §5.4 drift because the live agent-consumed contract — the openapi schema + JSDoc — documents windowId correctly and completely; only the ticket's planning ledger omits it, and that ticket closes on this merge (the ledger becomes historical). The live contract is not out of sync. Recommend (optional) backfilling the windowId row into #12931's ledger for graph-completeness; not worth a re-review cycle.

🪜 Evidence Audit

• PR body contains an Evidence: line: L3 (live lockedColumns E2E …) -> L3 required (runtime node-granularity observe_motion ACs).
• Achieved (L3) ≥ required (L3); no residual close-target ACs (exact drag-defect assertions correctly stay with #12930).
• No evidence-class inflation — the L3 claim is a real served-app e2e, which I reproduced.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

• The observe_motion description was shortened from a multi-line block-literal to a single ~185-char line; no ticket refs / phase sequencing / session ids; describes call-site usage (what + when); well under the 1024 cap.

Findings: Pass — net budget improvement.

🔌 Wire-Format Compatibility Audit

• observe_motion signature gains nodeIds / cellsOf / windowId; the return trace gains nodeIds + targetIds; required:[componentIds] is relaxed to "at least one of". All three are backward-compatible: relaxing a requirement keeps old calls valid; added return fields are ignorable by existing consumers; the component-only path returns identical rect behavior (unit-proven + verified in my component-only trace). No breaking change.

🔗 Cross-Skill Integration Audit

• Handbook §4.1 (the canonical observe_motion discovery doc) is updated for node granularity + the boundary note.
• Fixture convenience wrapper (fixtures.mjs) updated with an array-or-options overload — backward-compatible (Array.isArray branch preserves the old positional signature).
• Turn-Memory Pre-Flight: the PR body documents the decision tree for the handbook edit (0-byte always-loaded delta, rewrite disposition, mechanical checks run) — §7.8 satisfied by the author proactively.
• The motion-assertion DSL for /whitebox-e2e is correctly scoped out (ticket follow-up #4), so no latent integration gap from this PR.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally (main checkout, then restored to dev).
• Canonical location: unit at test/playwright/unit/ai/client/ComponentService.spec.mjs ✓; e2e at test/playwright/e2e/GridLockedRegionRowRender.spec.mjs ✓.
• Unit — ran UNIT_TEST_MODE config: 9/9 passed (incl. all 5 new observeMotion cases: nodeIds routing, mixed component-first order, component-only backward-compat, cellsOf expansion, no-target throw).
• E2e — ran against a dedicated :8091 server confirmed serving PR code: 2/2 passed (region-render + the new observeMotion trace; worker response captured 18 real rects, zero nulls).

Findings: Tests pass (independently executed, not diff-read).

📋 Required Actions

No required actions — eligible for human merge.

(Two non-blocking follow-ups noted above for the author's discretion: the node-only windowId enforcement/doc edge, and the optional windowId ledger backfill on #12931. Neither gates merge.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - 5 deducted: node-only windowId routing is documented-but-not-enforced (a soft idiom gap vs the framework's usual component-anchored window resolution). Otherwise exemplary: additive, preserves the worker getDomRect path, reuses the established DomAccess remote, clean geometry-truth/paint-truth boundary.
• [CONTENT_COMPLETENESS]: 90 - 10 deducted: the ticket's Contract Ledger enumerates nodeIds/cellsOf but omits the windowId schema param the PR ships (live openapi/JSDoc document it, so it's a ledger-completeness gap, not a live-contract gap). Anchor&Echo JSDoc, handbook rewrite, and Turn-Memory Pre-Flight are all present.
• [EXECUTION_QUALITY]: 95 - 5 deducted: the node-only-no-windowId path can silently sample the default window only (null-rects-as-false-signal) without a guard. No other defects across any path — I executed 9/9 unit + 2/2 e2e green; the live trace shows 18 real rects; serializeMotionRect hardens the missing-node case.
• [PRODUCTIVITY]: 100 - I actively confirmed each ticket AC maps to shipped code + a passing test: AC1 nodeIds traces (unit + e2e), AC2 mixed calls (unit + e2e), AC3 handbook §4.1 + pixels boundary (diff), AC4 Repro-A header+cells-in-one-call (the live 17-cell trace). None deferred or implied.
• [IMPACT]: 70 - Solid agent-tooling subsystem enhancement: closes a real motion-forensics blind spot (pooled cells were structurally invisible to NL) that cost a live debugging session. A targeted instrument extension, not foundational framework architecture — hence Solid, not Major-subsystem.
• [COMPLEXITY]: 55 - Moderate: one-method refactor + 2 small static helpers + a fixture-wrapper overload + schema/handbook deltas; the dual resolution branch (getDomRect vs DomAccess.getBoundingClientRect, registry→DOM fallback) introduces traceable branching, but it stays localized to one service.
• [EFFORT_PROFILE]: Quick Win - High ROI (unblocks a whole defect class for motion forensics) at contained complexity (single service, additive, unit+e2e covered).

[RETROSPECTIVE] Strong instrument extension with unusually disciplined evidence — the kind of node-granularity reach that turns "infer the defect + rely on the operator's eyes" into "one observe_motion call sees it." Approved; merge gate is @tobiu's.

Authored by Ada (@neo-opus-ada), Opus 4.8 (Claude Code). Session 04c3b09e-b951-4ddb-b835-08edadc18939.