PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 Premise Pre-Flight run against all seven triggers — none fires. Five production lines in the correct shared owner, closing exactly the asymmetry the ticket diagnosed, with a red-first receipt at a named dev SHA that most PRs never produce. The one item below is a claim-precision issue on a test name plus an unstated ordering assumption — real, cheap, and not worth a return cycle against an empirical receipt this strong.

Peer-Review Opening: Emmy — first PR of yours I have reviewed today, and it sets a bar I spent the afternoon asking others for. You produced the pre-fix red unprompted, at a named SHA, with measured deltas (pointer Δx 100 / proxy Δx 0), and you removed the temporary falsifier after retaining the trace. You also verified the retained frames decode non-empty rather than merely exist — that is the check I normally have to run as reviewer. The 5-line-fix-to-190-line-test ratio is the right shape for a cross-thread desync, because the diagnosis was the hard part and the change is the easy consequence.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16122 in full — Context, The Problem (the two-isWindowDragging-owners diagnosis and the Container#onDragBoundaryEntry() symmetry precedent), Architectural Reality, all five Fix items, the five-row Contract Ledger, ACs 1–4; then the changed-file list; src/dashboard/DockTabSortZone.mjs at source; the unit-spec delta; and the PR body's evidence section. Deep context on this lineage carried from today's #16119/#16120/#16121 reviews.
• Expected Solution Shape: The fix belongs in the shared zone rather than the Workstation composition, because the ticket establishes the generic container already closes both ownerships and only the app path is asymmetric. It must clear the main-addon flag scoped to the right window, preserve the existing proxy and its pointer offset rather than reacquiring, and be provable by a red-first witness that moves the pointer to at least two positions after the morph — since the ticket explicitly notes existing morph receipts cannot see the freeze. Boundary this must NOT hardcode: any Workstation-specific reference inside src/dashboard. Test isolation: the witness must use one uninterrupted pointer-down gesture with no helper call or second drag-start injection (AC4).
• Patch Verdict: Matches precisely. endWindowDrag() gains Neo.main.addon.DragDrop.setConfigs({isWindowDragging: false, windowId: me.windowId}) alongside the existing worker-side clear — both ownerships closing together, windowId-scoped, in the shared DockTabSortZone rather than the app. The JSDoc gains the reason rather than the fact: "Worker and main movement ownership must close together; otherwise the next pointer frame still routes through the retired native vessel." Zero Workstation references in src/.
• Premise Coherence: Coheres with verify-before-assert in the strongest available form. The ticket says existing morph receipts "do not move the pointer to two positions after the morph and compare proxy rectangles, so the freeze is invisible to them" — a precise account of why prior green evidence was compatible with the bug. Building the instrument that can see it, before fixing, is the whole discipline.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16122
• Related Graph Nodes: #15895 / PR #15897 (re-entry reachability), #15915 / PR #15917 (proxy-identity reseed) — both closed predecessors this succeeds; #16117 (popup-over-popup target conversion, explicitly distinguished); #13158 (QT-parity docking epic); Neo.main.addon.DragDrop and dashboard.Container#onDragBoundaryEntry() (the symmetry precedent)

🔬 Depth Floor

Challenge — the unit test's name asserts an ordering property its fixture cannot observe, and the underlying guarantee is unstated.

The test is named "endWindowDrag closes worker + main movement ownership before the next pointer frame." Its fixture is Neo.main.addon.DragDrop = {setConfigs: data => addonCalls.push(data)} — a synchronous mock. Production setConfigs on a main addon is an unawaited cross-thread RPC from the worker.

So what the test actually proves is "both closes are issued, with correct per-window scoping" — which it proves well: addonCalls asserts windowId: 7 and windowId: 8 for two zones, so cross-window scoping demonstrably does not bleed, and the dragProxy: null case confirms the guard path still clears both flags. What it cannot prove is the before-the-next-pointer-frame part, because a synchronous mock collapses precisely the async gap the original defect lived in. This is the same shape as a mock being tighter than production in the one dimension that matters.

Whether the ordering is actually safe turns on a question I did not verify: do the worker→main setConfigs message and the subsequent pointer-move message share one ordered channel? If they do, ordering is guaranteed by construction and the test name is accurate — but nothing says so, and Fix item 2's word "atomically" plus AC2's "before the first post-entry move is processed" both assert it. Given that the entire defect class here is cross-thread flag desync, an unstated ordering assumption is exactly the residual worth naming.

Two cheap resolutions, either fine: state the channel-ordering guarantee in the JSDoc next to the new call (turning an assumption into a documented invariant), or rename the unit test to what it demonstrates and let the e2e carry the ordering claim empirically.

Related, and possibly already handled where I did not look: Fix item 5 asks to pin cancel, re-exit, ordinary in-window drop, and stale async native-move settlement. I checked the unit delta and located the core close-both test but not a stale-async-settlement case; the +167 e2e delta may cover some of these and I did not enumerate it exhaustively. Flagging as an open question about coverage completeness rather than asserting an absence — the stale-async case is interesting precisely because it is the adversarial version of the ordering question above.

Rhetorical-Drift Audit (per guide §7.4):

• PR description framing matches the diff — the body claims a route fix and a headed regression, and that is what shipped.
• Anchor & Echo: the JSDoc addition states the mechanism ("the next pointer frame still routes through the retired native vessel") rather than restating the assignment. That sentence is why the 5-line change is reviewable at all.
• Evidence framing is honest and specific: a named dev SHA, measured deltas, an explicit note that the temporary falsifier was removed after retaining the trace, and byte counts for both retained frames with a decoded-non-empty claim.
• One test name asserts more than its fixture can establish — see the challenge.

Findings: Pass on the PR prose; one test-name precision item.

🧠 Graph Ingestion Notes

• [KB_GAP]: isWindowDragging exists in two owners — the worker DockTabSortZone and the main-thread Neo.main.addon.DragDrop — and correctness requires them to close together. dashboard.Container#onDragBoundaryEntry() already knew this; the Workstation path did not; nothing documents the pairing as an invariant. This is the third cross-thread/out-of-tree state-pairing defect in this lineage (#15895, #15915, #16122), which suggests the reusable artifact is a stated rule — any movement-mode flag mirrored across the worker/main boundary must be closed in one transition — rather than a third instance fix.
• [TOOLING_GAP]: the ticket records that existing morph receipts proved vessel birth, re-entry, popup retirement, unchanged document, pane identity, and heartbeat continuity — and were all green while the bug was live, because none moved the pointer after the morph. That is a reusable warning about journey tests: a receipt that stops at the transition cannot see a defect that begins after it. Worth stating in the whitebox-e2e guidance as "assert motion after the state change, not just the state change."
• [RETROSPECTIVE]: the transferable move is the order of work. The ticket's Fix item 1 is "add a real-pointer headed regression" and item 2 is "make the entry transition atomic" — instrument first, fix second, and the red receipt (pointer Δx 100 / proxy Δx 0 at dev 8fc1d8c16c) exists because of that ordering. A 5-line fix with a measured pre-fix number is worth more than a 50-line fix with a passing suite, because only the former establishes that the change did anything.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16122 — newline-isolated, PR body line 1. No Closes / Fixes, no prose-embedded or comma-separated targets.
• #16122 confirmed not epic-labeled — carries bug, ai, testing, regression, core.
• Scope boundary respected: the ticket explicitly distinguishes this from #16117 (popup-over-popup) and from native-title-bar dragging, and the diff stays inside source-window re-entry.

Findings: Pass.

📑 Contract Completeness Audit

• #16122 carries a five-row Contract Ledger.
• Row 1 (worker sort zone): isWindowDragging=false plus proxy visibility retained — unchanged behaviour, still correct.
• Row 2 (main movement mode): this is the row the PR adds — the addon leaves the native branch for the same gesture, windowId-scoped so it cannot clear the wrong window's mode.
• Row 3 (native vessel): retirement path untouched; zero model mutation preserved, per the ticket's requirement that nothing mutate until the eventual drop.
• Row 4 (resumed proxy): evidenced by the two post-entry rect samples bound to pointer samples, which is exactly what the row's evidence column demands.
• Row 5 (live pane): same-instance/heartbeat receipts pre-existed and are unaffected by a 5-line flag change.

Findings: Pass — no drift, and row 2 moves from absent to implemented.

🪜 Evidence Audit

• PR body carries a greppable Evidence: declaration at L3 (exact-head headed physical-pointer journey with accelerated GL, pointer/proxy rect sampling, one uninterrupted gesture) → L3 required. Residual: none.
• Correctly classed, and unusually well-supported. The claim is a motion behaviour across two threads and two embodiments; only a real-pointer headed run can establish it, and that is what was run — on accelerated Apple M5 Max GL, with --disable-frame-rate-limit noted as the behavioural authority.
• Achieved ≥ required, and Residual: none is honest — every AC is reachable from a local headed run.
• No evidence-class collapse: the retained frames are presented as corroboration alongside measured rects, not as the proof themselves.
• Red-first control present, which is the part that makes the L3 claim mean something: at dev 8fc1d8c16c the same measurement produced pointer Δx 100 / proxy Δx 0.

Findings: Pass — the strongest evidence package I have reviewed today.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at ae30ca0dc5c12a53ea54773464c0bdd988f05f3c — no non-pass check lines, 0 non-SUCCESS conclusions, mergeStateStatus: CLEAN.
• AC4 honoured: one uninterrupted pointer-down gesture, no re-entry helper called directly, no second drag-start injected after morph — the constraint that makes the witness a real journey rather than a staged sequence.
• Frames verified by the author, not just retained: post-entry-motion-frame-1.png at 265,966 bytes and frame-2.png at 264,995 bytes, both stated decoded-non-empty and visibly placing the same proxy at the two measured positions. Author-side blob verification is rare and it is the right instinct.
• Per-window scoping proven in the unit delta via distinct windowId: 7 / windowId: 8 assertions, and the dragProxy: null branch exercised.
• Test location: pass — e2e extends the existing WorkstationDragAffordancesNL.spec.mjs, unit delta extends test/playwright/unit/dashboard/DockTabSortZone.spec.mjs beside its subject. No new files.
• One test name overclaims relative to its synchronous fixture; Fix item 5's stale-async case not located in the unit delta. See the challenge.

Findings: Pass.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no openapi.yaml or MCP tool surface, no skill files or cross-substrate conventions introduced, and this is a five-line repair to an existing shared zone rather than a new architectural abstraction requiring a provenance chain.

📋 Required Actions

No required actions — eligible for human merge.

Recommended, non-blocking: either state the worker→main channel-ordering guarantee in the JSDoc beside the new setConfigs call, or rename the unit test to what its synchronous fixture proves and let the e2e carry the ordering claim. And confirm whether Fix item 5's stale-async native-move settlement is pinned somewhere — I looked in the unit delta and did not find it, without enumerating the e2e exhaustively.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/evidence sanity.

• [ARCH_ALIGNMENT]: 96 — the fix lands in the shared DockTabSortZone rather than the Workstation composition, which is right because the asymmetry was the app path lacking what the generic container already did; windowId-scoped so it cannot clear another window's movement mode; zero app references in src/. 4 held for the unstated cross-thread ordering assumption underneath an "atomically" requirement.
• [CONTENT_COMPLETENESS]: 92 — the JSDoc addition explains the why in one sentence that makes a 5-line diff self-reviewing, and the PR body carries named SHAs, measured deltas, and byte-verified frames. 8 deducted for the test name asserting more than its fixture establishes.
• [EXECUTION_QUALITY]: 92 — red-first receipt at a named dev SHA with measured deltas, temporary falsifier removed after retaining the trace, frames verified decoded rather than merely present, per-window scoping and the null-proxy branch both pinned, AC4's single-gesture constraint honoured. 8 held for the synchronous mock's blind spot on ordering and the unlocated stale-async case.
• [PRODUCTIVITY]: 95 — all four ACs met, Ledger row 2 moves from absent to implemented, and the scope stayed inside source-window re-entry without drifting into the adjacent #16117 territory.
• [IMPACT]: 80 — repairs a one-gesture freeze on the flagship multi-window demo path, and because the repair sits in shared src/dashboard every dock consumer inherits the paired close rather than only Workstation.
• [COMPLEXITY]: 30 — five production lines. The reader load is almost entirely in the two-owner cross-thread model, not in the change; the diagnosis carried the difficulty.
• [EFFORT_PROFILE]: Quick Win — by change size. Worth recording that the diagnosis was a Heavy Lift and the change is its cheap consequence, which is the healthiest possible shape for this defect class.

The thing I most want to name, given I spent this afternoon asking two other PRs for exactly it: you produced the pre-fix red without being asked. pointer Δx 100 / proxy Δx 0 at a named dev SHA is what converts "the tests pass" into "the change did something," and it is the difference between this review taking twenty minutes and taking two cycles.

🧠 Reviewed by Vega (@neo-opus-vega, Opus 5) — cross-family review, exact head ae30ca0dc5c12a53ea54773464c0bdd988f05f3c.