PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The contract shape, placement, and focused unit coverage are mostly correct, and current-head CI is green after the body fix. One correctness defect remains: the drop path does not cleanup in a thrown commit path, contradicting the PR's fail-closed cleanup claim and leaving stale hover state.

Peer-Review Opening: I reviewed exact head 1a2fe038dad321faf289f19be777c9d489bcd66c. This is close, but the cleanup guarantee needs to survive a failing owner commit seam before the receiving-window target is mergeable.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14670 issue body; #14757 body and changed-file list; ADR-0029 §2.3; KB query for CrossWindowDragTarget/receiving-window contract; src/manager/DragCoordinator.mjs; existing dashboard SortZone remote-drop precedent; exact-head diff; focused unit spec; current PR checks.
• Expected Solution Shape: A dashboard-layer receiving target should register with DragCoordinator using the §2.3 sortGroup/windowId identity, expose the mandatory target hooks, keep the coordinator dock-blind, ride owner-provided preview→operation seams, and fail closed without leaking hover state across rejected or failed drops. It must not import app-layer preview renderers or mutate dock documents directly.
• Patch Verdict: Mostly matches: the new class is a thin src/dashboard/ target, imports only Base and DragCoordinator, exposes the field-for-field target hooks, and the spec covers registration, hit-test, hover, commit, and no-preview/rejected-drop paths. It contradicts the cleanup shape when commitOperation() throws: onRemoteDragLeave() is after the commit and not finally-protected.
• Premise Coherence: Coheres with the Body/Brain boundary and ADR-0029's dock-blind coordinator invariant. The remaining issue is a V-B-A cleanup defect, not an architectural placement failure.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14670
• Related Graph Nodes: #13158, ADR-0029 §2.3, Discussion #13370 Option-4 seam, #13028, Neo.manager.DragCoordinator, dockZone.v1

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The drop cleanup invariant is only true on successful or converter-rejected paths. A throwing commitOperation() leaves currentPreview set and skips the owner's clearPreview seam, so the receiving window can retain stale hover state after a failed drop.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the contract/placement framing matches the diff, except the fail-closed "hover state never leaks past the gesture" claim overstates the throwing commit path.
• Anchor & Echo summaries: the class summary accurately names ADR-0029 and the no-parallel-drag/coordinator-dock-blind boundaries.
• [RETROSPECTIVE] tag: N/A, none present.
• Linked anchors: #14670 and ADR-0029 §2.3 support the target-side hook shape.

Findings: Rhetorical drift detected on the fail-closed cleanup claim; same Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Local unit test initially hit a shared 18180 Chroma port; reran the focused spec with NEO_CHROMA_PORT_TEST=18189 to isolate the review run.
• [RETROSPECTIVE]: For receiving-window drag targets, cleanup invariants must be tested on thrown owner seams, not only rejected/null conversion paths.

N/A Audits — 📡

N/A across listed dimensions: no MCP/OpenAPI tool-description surface changed.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #14670
• #14670 is labeled enhancement, ai, architecture; not epic.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket / source authority contains the contract: #14670 points to ADR-0029 §2.3, whose reviewer table defines sortGroup, windowId, acceptsRemoteDrag, onRemoteDragMove, onRemoteDragLeave, and onRemoteDrop.
• Implemented PR diff matches the contract: class exposes those identities/hooks and avoids dock-specific imports into DragCoordinator.

Findings: Pass on contract shape; cleanup failure is a behavioral edge within that contract, handled under Required Actions.

🪜 Evidence Audit

Reference: learn/agentos/process/evidence-ladder.md.

• PR body contains an Evidence: declaration line.
• L2 unit evidence is appropriate for this contract leaf; live multi-window gesture is correctly deferred to consuming leaves.
• Evidence-class collapse check: review language does not promote this leaf to live gesture proof.

Findings: Pass on evidence class.

Conditional Audit Triggers

📜 Source-of-Authority Audit: ADR-0029 §2.3 is the authority for the target-side hooks and the dock-blind coordinator invariant. The diff stays in src/dashboard/ beside the docking model/adapter substrate and does not import app-layer preview renderers.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? N/A; this is Body dashboard code, not an agent workflow convention.
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating? N/A.
• Does any reference file mention a predecessor pattern that should now also mention the new one? No; ADR-0029 already names this contract.
• If a new MCP tool is added, is it documented in the relevant skill's reference payload? N/A.
• If a new convention is introduced, is there documentation somewhere explaining when the convention applies and how it fires? Yes, ADR-0029 §2.3; this PR implements the leaf.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally in tmp/review-14757-gpt-1a2f at exact head 1a2fe038dad321faf289f19be777c9d489bcd66c.
• Canonical Location: new unit spec is in test/playwright/unit/dashboard/CrossWindowDragTarget.spec.mjs, matching the touched src/dashboard/ class.
• If a test file changed: Ran NEO_CHROMA_PORT_TEST=18189 npm run test-unit -- test/playwright/unit/dashboard/CrossWindowDragTarget.spec.mjs → 6 passed.
• If code changed: Ran git diff --check origin/dev...HEAD, node --check src/dashboard/CrossWindowDragTarget.mjs, and npm run --silent ai:structure-map -- --root src/dashboard --files --loc.

Findings: Tests pass. Additional falsifier probe shows a missing thrown-commit cleanup regression.

📋 Required Actions

To proceed with merging, please address the following:

• Make onRemoteDrop() cleanup robust when the owner commitOperation() seam throws. The expected shape is try/finally or equivalent so onRemoteDragLeave() always clears currentPreview and calls clearPreview, plus a unit regression proving a thrown commitOperation leaves no stale preview.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 94 - Correct dashboard-layer target and dock-blind coordinator boundary; deduction for an unprotected cleanup invariant in the target lifecycle.
• [CONTENT_COMPLETENESS]: 90 - Strong JSDoc and body mapping; deduction for overclaiming fail-closed cleanup on a throwing commit path.
• [EXECUTION_QUALITY]: 76 - Focused tests pass, but the thrown commit falsifier leaves stale runtime state.
• [PRODUCTIVITY]: 84 - Delivers most of #14670's contract surface; blocked on one lifecycle edge before it can be a reliable receiving target.
• [IMPACT]: 82 - Cross-window docking is a high-risk Body interaction seam with downstream C2-C4 consumers.
• [COMPLEXITY]: 55 - Small two-file diff, but it sits on a multi-window lifecycle boundary.
• [EFFORT_PROFILE]: Heavy Lift - Compact implementation with high correctness bar because stale drag state crosses windows.

Falsifying probe, exact head:

node --input-type=module -e 'import {setup} from "./test/playwright/setup.mjs"; setup({appConfig:{name:"Probe"}}); import Neo from "./src/Neo.mjs"; import "./src/core/_export.mjs"; const {default: CrossWindowDragTarget}=await import("./src/dashboard/CrossWindowDragTarget.mjs"); const target=Neo.create(CrossWindowDragTarget,{sortGroup:"dock-main",windowId:99,previewToOperation:()=>({operation:"addTab"}),commitOperation:()=>{throw new Error("commit failed")},clearPreview:()=>console.log("CLEARED")}); target.currentPreview={itemId:"pane-a"}; try { target.onRemoteDrop({id:"pane-a"}); } catch (error) { console.log(JSON.stringify({threw:error.message,currentPreview:target.currentPreview})); } target.destroy();'

Output:

{"threw":"commit failed","currentPreview":{"itemId":"pane-a"}}