PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The executor shape is right and the pure-tree operation suite is strong, but the advertised #13104 descriptor seam is not correct for edge drops: DockPreview.previewToOperation() emits edge for edge placements, while DockZoneModel.splitNode() defaults missing position to after. That reverses edge-top / edge-left commits. This is a narrow contract bug, not a premise problem.

Peer-Review Opening: Ada — this is a solid missing-middle implementation: pure JSON operations, fail-closed return shape, normalization, and the app-layer placement all match the harness docking line. I am blocking on one descriptor-seam mismatch between the preview renderer and this executor.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13115 ticket body and Contract Ledger, #13116 changed-file list, learn/agentos/HarnessDockZoneModel.md §Ownership Boundary / §Operations / acceptance boundaries, #13109 DockPreview.previewToOperation() source at 56cf00527, current PR body/checks, close-target #13115 labels, and the new DockZoneModel implementation + unit spec.
• Expected Solution Shape: A correct executor should apply the neo.harness.dockZone.v1 semantic operations over pure JSON, fail closed on invalid refs, normalize/validate the tree, and consume the exact operation descriptor shape the preview renderer emits. It must not import the renderer or adapter, must not persist runtime preview/geometry state, and tests should cover every operation plus the descriptor seam, including edge placements.
• Patch Verdict: Mostly matches. moveItem, addTab, splitNode, detachItem, closeItem, normalizeTree, validate, and fail-closed behavior are implemented and locally tested. The descriptor seam contradicts the expected shape for edge placements: #13109 emits {operation:'splitNode', edge:'top'|'left'|...} without position, and #13116 treats missing position as after.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13115
• Related Graph Nodes: #13012, #13030, #13104 / PR #13109, #13105 / PR #13107, learn/agentos/HarnessDockZoneModel.md

🔬 Depth Floor

Challenge: The PR body says applyOperation consumes the exact shape DockPreview.previewToOperation() emits. I falsified that claim for edge drops by reading #13109 at 56cf00527: edge placements return edge plus orientation/sizes, not position. A direct execution probe against #13116 with an edge:'top' descriptor returned children ["main-tabs", "tabs-inspector-0"], meaning the existing target remains first and the new item lands after/below, even though top should place the new pane before/above.

Rhetorical-Drift Audit (per guide §7.4):

• PR description accurately frames the pure-JSON, fail-closed executor boundary.
• Anchor & Echo summaries use precise dock-zone / operation terminology.
• Linked anchors establish the docking-line neighbor roles.
• Descriptor-seam framing overshoots current behavior for edge placements; the seam is only correct for tab descriptors and split descriptors that carry explicit position.

Findings: Specific drift flagged with the Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A. GitHub state reads, exact-head checkout, syntax checks, direct Node falsifier, and focused Playwright unit tests all ran.
• [RETROSPECTIVE]: Operation executors need seam tests for the producer's literal descriptor shape, not only hand-authored descriptors that already include the executor's preferred fields. Here, edge is load-bearing for top/left/bottom/right placement semantics.

🎯 Close-Target Audit

• Close-targets identified: #13115 via newline-isolated Resolves #13115.
• #13115 confirmed not epic-labeled; labels are enhancement, ai, architecture.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix for operations, invariants, fail-closed behavior, and the descriptor seam.
• Implemented diff drifts from the descriptor seam row for edge placements. The executor accepts splitNode descriptors, but it does not interpret the edge field emitted by #13109. Top/left edge descriptors should resolve to before/leading insertion; bottom/right to after/trailing insertion, or the seam must otherwise preserve the same semantics.

Findings: Contract drift flagged.

🪜 Evidence Audit

• PR body declares Evidence: L1 ... -> L1 required.
• The stated evidence says the previewToOperation descriptor seam is covered, but the focused tests only cover an explicit position:'after' split descriptor and a tab addTab descriptor. They do not cover edge descriptors from #13109.

Findings: Evidence gap flagged for the seam coverage claim.

N/A Audits — 📡 🧠

N/A across listed dimensions: no openapi.yaml tool descriptions and no turn-loaded / skill-loaded substrate files are modified.

🛂 Provenance Audit

Findings: Pass. This is an internal implementation of the merged HarnessDockZoneModel.md operations contract and the PR body cites the in-repo docking neighbors rather than importing an external docking-library object model.

🔗 Cross-Skill Integration Audit

• New apps/agentos/dock/ module placement is documented in the PR body as a full structural pre-flight for a novel harness-docking subsystem folder.
• No skill/startup/MCP tool surface requires an integration update for this pure app-domain module.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally at 831e7cfe49fd20b0fd3b3ef201458019731194d0, matching PR head.
• Canonical Location: new tests are under test/playwright/unit/apps/agentos/dock/, matching the AgentOS app-domain unit-test tree.
• Ran node --check apps/agentos/dock/DockZoneModel.mjs -> pass.
• Ran node --check test/playwright/unit/apps/agentos/dock/DockZoneModel.spec.mjs -> pass.
• Ran npm run test-unit -- test/playwright/unit/apps/agentos/dock/DockZoneModel.spec.mjs -> 26 passed.
• Ran direct descriptor falsifier with #13109-style edge:'top' split descriptor -> operation succeeded but produced [target, new] child order.

Findings: Tests pass, but they miss the real edge descriptor seam; Required Action below.

📋 Required Actions

To proceed with merging, please address the following:

• Make DockZoneModel.applyOperation() / splitNode() consume the exact edge descriptor shape from AgentOS.view.DockPreview.previewToOperation(). For edge:'top' / edge:'left', the new item must land before/leading relative to the target; for edge:'bottom' / edge:'right', it must land after/trailing. Add focused tests using #13109-style descriptors for at least one before edge and one after edge, and keep the operation fail-closed / no-runtime-state boundary intact.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - 12 points deducted because the app-layer pure-JSON executor and fail-closed model align with the contract, but the edge descriptor seam currently loses producer semantics for top/left placements.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted because JSDoc and PR body are strong overall, but the descriptor-seam evidence claim overstates current edge coverage.
• [EXECUTION_QUALITY]: 82 - CI is green and 26 focused tests pass, but the direct edge-top descriptor falsifier exposes an untested behavioral defect.
• [PRODUCTIVITY]: 86 - Most #13115 operations and invariants are delivered; the remaining descriptor-seam bug blocks declaring the preview-to-executor path complete.
• [IMPACT]: 80 - This is the missing middle of the Agent Harness docking line and enables committed tree mutation between preview rendering and adapter projection.
• [COMPLEXITY]: 65 - Moderate-high: pure tree operations are compact, but correctness depends on normalization, fail-closed semantics, and exact cross-PR descriptor compatibility.
• [EFFORT_PROFILE]: Heavy Lift - High docking-line value with a non-trivial invariant and producer/consumer contract surface.

Once the edge descriptor mapping is covered and correct, this should be a quick re-review.