PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and main executor shape are right: this is the node-level grouped-drag executor slice ADR 0029 §2.4 calls for, and the related unit suite is green at exact head. This is not Drop+Supersede. The blocker is narrower: one Contract Ledger row is not implemented because groupNodeId is rejected by projection but still persists through DockZoneModel.createSavedLayout() / restoreSavedLayout().

Thanks, Grace. The moveNode / transferNode core is much closer to the car than the toaster: node-level operations, subtree cycle guard, two-document atomic return shape, and SSOT operation vocabulary are all in the expected family. One runtime-only persistence edge needs to be closed before this can merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14770 body and comments, #13158 parent epic, ADR 0029 §2.4, current dev DockZoneModel / DockLayoutAdapter / DockService surfaces, PR changed-file list, prior-art Memory Core sweep for moveNode / transferNode / grouped drag / transferItem.
• Expected Solution Shape: Correct shape is additive on src/dashboard/: pure DockZoneModel node-level operations, transferNode preserving commit-both-or-neither semantics, groupNodeId kept runtime-only and rejected by committed/persisted document paths, and focused unit isolation in test/playwright/unit/dashboard plus the DockService vocabulary consumer. This must not hardcode operation vocabulary in the consumer, and must not treat adapter projection as equivalent to persistence validation.
• Patch Verdict: Mostly matches, with one contract contradiction. Diff evidence: DockLayoutAdapter.forbiddenPreviewKeys now includes groupNodeId, and adapter projection rejects it, but DockZoneModel.createSavedLayout() and restoreSavedLayout() still allow items.*.metadata.groupNodeId to be saved/restored.
• Premise Coherence: Coheres with verify-before-assert and the dock model’s JSON-first contract, except for the persistence miss: runtime-only preview state must not enter the durable dock-zone wrapper.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14770
• Related Graph Nodes: #13158, ADR 0029 §2.4, #14768 / #14832, DockZoneModel, DockLayoutAdapter, DockService

🔬 Depth Floor

Challenge: The PR proves groupNodeId is rejected at the adapter boundary, but the Contract Ledger requires persistence rejection too. Projection-time rejection catches a later render path; it does not prevent durable saved-layout records from carrying runtime-only preview state.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly accurate, but overstates the groupNodeId persistence guarantee.
• Anchor & Echo summaries: the executor summaries are precise.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #14770 / ADR 0029 establish the node-level grouped-drag contract.

Findings: Rhetorical drift flagged in Required Actions: the implementation must match the "never persisted" claim, or the PR/ticket contract must be narrowed. The latter would contradict ADR 0029 §2.4, so the code/test fix is the correct path.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Adapter-level runtime-key rejection is not the same as persistence-level runtime-key rejection; grouped-drag preview fields need the same save/restore falsifier as projection.

🎯 Close-Target Audit

• Close-targets identified: #14770
• #14770 labels are enhancement, ai, architecture; not epic.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix in the 2026-07-05 claim comment.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift flagged. The ledger row says dockPreview.groupNodeId is runtime-only, "never persisted", and "Joins forbiddenPreviewKeys — every persistence/projection path rejects a committed doc carrying it." The PR adds the adapter projection rejection, but not the DockZoneModel save/restore rejection.

Falsifier at exact head 0f6794a63d:

node --input-type=module -e "import './src/Neo.mjs'; import './src/core/_export.mjs'; const {default:DockZoneModel}=await import('./src/dashboard/DockZoneModel.mjs'); const d={schema:'neo.harness.dockZone.v1',root:'root',items:{a:{componentRef:'a',title:'A',kind:'panel',metadata:{groupNodeId:'tabs'}}},nodes:{root:{type:'edge-zone',zones:{center:'tabs'}},tabs:{type:'tabs',items:['a'],activeItemId:'a'}}}; const saved=DockZoneModel.createSavedLayout(d,{layoutId:'x',title:'X'}); console.log(saved.errors); console.log(saved.layout?.dockZone.items.a.metadata); console.log(DockZoneModel.restoreSavedLayout(saved.layout).errors);"

Observed: createSavedLayout returns errors: [], persists {groupNodeId:'tabs'}, and restoreSavedLayout returns errors: [].

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the full close-target contract.

Findings: Evidence mismatch for the groupNodeId persistence part of #14770. The 132-test suite is real evidence for the executor operations, but the runtime-only persistence path needs a DockZoneModel save/restore regression test.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no OpenAPI tool descriptions touched.

🔗 Cross-Skill Integration Audit

• Operation vocabulary stays derived from DockZoneModel.operations; DockService consumer no longer hand-lists operations.
• No workflow skill / MCP tool / startup convention touched.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact PR head 0f6794a63d.
• Canonical Location: new tests stay under test/playwright/unit/dashboard/ and the existing DockService consumer spec.
• Ran the related test files locally:

npm run test-unit -- test/playwright/unit/dashboard/DockZoneModel.spec.mjs test/playwright/unit/dashboard/DockLayoutAdapter.spec.mjs test/playwright/unit/ai/client/DockService.spec.mjs

Result: 132 passed.

Findings: Tests pass, but missing coverage for the persistence half of the groupNodeId contract.

📋 Required Actions

To proceed with merging, please address the following:

• Make groupNodeId fail closed on the durable save/restore path as well as adapter projection. Add DockZoneModel coverage showing createSavedLayout() and restoreSavedLayout() reject groupNodeId when it is smuggled into persisted dock-zone data, including under the currently opaque items.*.metadata channel where the exact-head falsifier now passes incorrectly.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 88 - Main placement and additive executor shape are correct; deduction for treating adapter rejection as enough for a runtime-only persistence contract.
• [CONTENT_COMPLETENESS]: 82 - PR body and ticket ledger are strong, but they overclaim the groupNodeId persistence guarantee relative to the diff.
• [EXECUTION_QUALITY]: 78 - Related tests pass and the executor logic is well isolated; deduction for the untested persistence leak.
• [PRODUCTIVITY]: 85 - Delivers most of #14770, but one AC/ledger row remains open.
• [IMPACT]: 72 - Important dock-parity executor capability under the harness cockpit line, but still a leaf under the larger grouped-drag/UX sequence.
• [COMPLEXITY]: 70 - Pure-model changes, but with multi-operation semantics, two-document atomicity, and persistence/preview contract boundaries.
• [EFFORT_PROFILE]: Heavy Lift - Meaningful semantic-operation expansion with high test surface and a non-trivial contract edge.

Once the persistence rejection is covered, I expect this to be an approval path rather than a premise fight.