PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and placement are right, but the implementation fails a same-topology restore case the close target owns: swapping items between two existing tabs nodes keeps the fingerprint shape equal, yet the generated move plan can normalize away a source node before the next move targets it. This is a correctness blocker in the planner, not follow-up material.

Thanks for taking the restore planner slice. The model-tier boundary is the right one: pure planning over DockTopologyDiff plus sequential application through DockZoneModel.applyOperation, with no document replacement.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14653 body and comments, including Clio's spec-transfer and Euclid's earlier dependency/Contract Ledger intake; #14871 live PR state, close target, green current-head CI, commit log, and changed-file list; ADR 0029 §2.2 and §2.6; KB result for DockZoneModel / DockTopologyDiff / ADR 0029; Memory Core prior-art sweep for #14653 and ADR 0029 terms; current exact-head source for DockRestorePlanner.mjs, DockTopologyDiff.mjs, DockZoneModel.mjs, and the new unit spec.
• Expected Solution Shape: Same-topology restore should plan from current to captured through semantic executor operations only, and should not equate "matching shape fingerprint" with "only intra-node tab reorders can occur." Test isolation should include same-shape cross-node moves/swaps, because equal per-node tab counts can still hide item exchanges between tabs nodes.
• Patch Verdict: Contradicts the expected shape for cross-node moves. The planner emits diff.moves directly before tab reorders, but applying the first move can empty and normalize away the source tabs node, so the next move can fail even though the current/captured fingerprints match.
• Premise Coherence: The premise coheres with verify-before-assert and ADR 0029's reparent-never-recreate model. The current implementation misses the falsifier Clio explicitly warned about: structural collapse during operation sequencing.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14653
• Related Graph Nodes: Parent #13158, blocked primitive #14650 / PR #14824, ADR 0029 §2.2 / §2.6.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The PR treats matching shape fingerprints as if cross-node moves cannot occur. They can: two tabs nodes with the same item counts can exchange items while preserving the exact fingerprint shape, and that is still an unchanged-topology restore.

Rhetorical-Drift Audit (per guide §7.4):

• PR description and class JSDoc overshoot when they say matching fingerprints mean adds / removes / moves cannot occur. The implementation itself maps diff.moves, and the falsifier below proves moves are possible under equal shape.
• Contract Ledger is present, but the shipped behavior does not satisfy the ledger's "restore reaches captured layout" row for same-shape cross-node moves.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: ADR 0029 and #14653 establish the operation-only restore boundary.

Findings: Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Local npm run test-unit -- test/playwright/unit/dashboard/DockRestorePlanner.spec.mjs produced no test output and exited 143 in both sandboxed and escalated runs while starting the Chroma-backed unit webServer. CI unit is green, but local Playwright execution did not produce usable evidence in this clone.
• [RETROSPECTIVE]: For dock restore planning, computeShapeFingerprint().shape proves same structural topology, not same item-to-container assignment. Same-topology planners must still handle cross-node item moves without allowing normalizeTree() to collapse a needed target/source node mid-plan.

🎯 Close-Target Audit

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

• Close-targets identified: #14653 in the PR body and commit subject.
• #14653 labels are enhancement and ai; it is not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating PR body contains a Contract Ledger matrix for planner/application surfaces.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift: the ledger says unchanged-topology restore reaches the captured layout through the planner/application shell, but the current operation ordering fails a valid same-shape cross-node swap.

🪜 Evidence Audit

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

Findings: Evidence mismatch: the added round-trip test covers resize + intra-node reorder + auto-hide, but not same-fingerprint cross-node movement. The direct falsifier below shows the core restore AC is not yet covered.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI tool descriptions, workflow substrate, skill files, or cross-skill conventions are changed.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 37f005b91f2afc04835faac56215facbaef243a2 in tmp/pr-14871-review.
• Canonical Location: new unit spec is under test/playwright/unit/dashboard/.
• Static parse checks passed: node --check src/dashboard/DockRestorePlanner.mjs and node --check test/playwright/unit/dashboard/DockRestorePlanner.spec.mjs.
• Local Playwright focused unit run did not produce usable evidence; both sandboxed and escalated npm run test-unit -- test/playwright/unit/dashboard/DockRestorePlanner.spec.mjs exited 143 before test output.
• Current-head CI is green, including unit.
• Direct Node falsifier against the exact PR head fails the same-topology cross-node swap case:

// current/captured have the same shape: main-tabs has 2 items, side-tabs has 1.
current.nodes['main-tabs'].items = ['terminal', 'swarm'];
current.nodes['side-tabs'].items = ['strategy'];
const result = DockRestorePlanner.restoreToward(current, captured);
// result.applied === 1
// result.errors === ['"side-tabs" is not a tabs node']

Findings: Test placement and CI pass; execution-quality blocker remains because the missing same-shape cross-node case fails.

📋 Required Actions

To proceed with merging, please address the following:

• Fix DockRestorePlanner.planRestore() / application sequencing so same-fingerprint cross-node item swaps restore successfully without document replacement. Add a unit test where two tabs nodes keep the same tab counts but exchange items, e.g. main-tabs: [terminal, swarm] and side-tabs: [strategy] restoring to main-tabs: [strategy, swarm] and side-tabs: [terminal]. The test should assert no errors, full plan application, empty diff vs captured, and itemId continuity.

📊 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]: 82 - Placement and operation-only boundary are correct; 18 deducted because the same-topology premise is narrowed incorrectly by treating matching shape as no cross-node moves.
• [CONTENT_COMPLETENESS]: 78 - Strong JSDoc and PR body with a Contract Ledger; deduction for prose overclaiming that moves cannot happen under a matching fingerprint.
• [EXECUTION_QUALITY]: 55 - CI is green and simple round trips pass, but the direct same-shape cross-node restore falsifier fails with a real executor error.
• [PRODUCTIVITY]: 65 - Delivers the intended planner surface and useful tests, but does not yet satisfy the core restore-reaches-captured-topology AC for all unchanged-topology cases.
• [IMPACT]: 70 - Important dashboard model leaf that future live perspective restore and Neural Link exposure will consume.
• [COMPLEXITY]: 65 - Small file count, but semantic operation ordering under normalization makes the state space non-trivial.
• [EFFORT_PROFILE]: Heavy Lift - The diff is compact, but the correctness burden is high because operation sequencing must preserve topology while the executor normalizes after every step.

This should be fixable in the same PR: the contract and placement are sound; the missing case is in the planner's move sequencing and test matrix.