PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The producer implementation matches the AC1 compute slice and the focused unit evidence is green, so this is not Drop+Supersede. The merge blocker is scope truth: the PR currently closes #14866 while its own body still lists renderer wiring, drop routing, and E2E coverage as remaining WIP for the same ticket.

Grace, the compute half is in good shape. The required change is to make the PR/ticket closure shape honest before merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #14866 body and ACs; changed-file list (src/dashboard/DockPreviewProducer.mjs, test/playwright/unit/dashboard/DockPreviewProducer.spec.mjs); current dev docking sources (AgentOS.view.DockPreview, Neo.dashboard.DockZoneModel, Neo.dashboard.CrossWindowDragTarget); learn/agentos/HarnessDockZoneModel.md Preview State Contract; ADR 0029 §2.3 cross-window drag contract; prior-art Memory Core sweep for dock preview / preview-to-operation decisions.
• Expected Solution Shape: A correct producer-only slice belongs in src/dashboard/, stays pure and layer-blind, emits the existing runtime-only neo.harness.dockPreview.v1 payload, fails closed outside valid geometry, and proves the producer/consumer contract with a focused unit test. It must not own pointer lifecycle, rendering, drop commit, or persistence, and it must not close the full #14866 ticket unless the renderer/drop/E2E ACs also land.
• Patch Verdict: Matches the expected AC1 producer shape, but contradicts the close-target scope. The diff adds a pure DockPreviewProducer with config-tunable edge bands and no app-layer import, plus unit tests pinning produced payloads through DockPreview.isValidPreview; however the PR body says Resolves #14866 while also listing AC2/AC3 as remaining WIP.
• Premise Coherence: Mixed. The code honors verify-before-assert by pinning the payload against the consumer validator; the PR metadata violates the same value if merged unchanged because it would claim a full ticket resolution that the evidence and body explicitly say is not delivered.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Current PR body says Resolves #14866; that close-target is not valid for the current diff.
• Related Graph Nodes: #13158, #14769, ADR 0029 §2.3, dockPreview.v1, previewToOperation, CrossWindowDragTarget.previewFor.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Non-blocking implementation watch item: previewId is currently deterministic by itemId/nodeId/kind. That is fine for the current unit scope, but the wiring leaf should ensure either node ids or preview ids are workspace/container scoped once multiple live workspaces can share generic ids like main-tabs.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift found. It simultaneously presents the PR as ready/closing (Resolves #14866) and as WIP with AC2/AC3 still pending.
• Anchor & Echo summaries: pass for the new class JSDoc; it accurately describes a producer-only, runtime-only compute slice.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: ADR 0029 / HarnessDockZoneModel references match the producer boundary.

Findings: Rhetorical drift flagged as a Required Action.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A. The exact-head unit spec ran locally after an ignored node_modules worktree link.
• [RETROSPECTIVE]: The producer→consumer pin is the right guard for the dashboard/app-layer boundary: src/dashboard stays layer-blind while the unit test proves emitted payloads remain consumable by AgentOS.view.DockPreview.

🎯 Close-Target Audit

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

• Close-targets identified: #14866.
• #14866 confirmed not epic-labeled.

Findings: Fails close-target honesty. #14866 AC2 requires live DockPreview rendering, and AC3 requires drop routing through previewToOperation → applyDockZoneOperation plus E2E coverage. This PR only delivers AC1 and explicitly lists the other ACs as residual WIP.

📑 Contract Completeness Audit

• Existing contract authority located: learn/agentos/HarnessDockZoneModel.md defines neo.harness.dockPreview.v1.
• Implemented diff matches that existing runtime payload contract: split previews carry placement.orientation, rejected/no-candidate hovers fail closed to null, and produced payloads pass DockPreview.isValidPreview in the unit pin.

Findings: Pass. No new durable wire format or persisted schema was introduced.

🪜 Evidence Audit

The PR body declares L2 unit evidence and names renderer wiring/E2E as residual. That is accurate for the code slice, but not compatible with closing #14866 as written.

• PR body contains an Evidence: declaration line.
• Achieved evidence covers close-target ACs.
• Residuals are reconciled with the close-target issue/PR closure shape.
• Review language does not promote L2 unit evidence to L3 rendered/e2e proof.

Findings: Evidence/AC mismatch flagged. The body can keep the L2 claim for a producer-only leaf, but the close target must be narrowed or the remaining ACs must be implemented before this PR merges.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml changes.

🔗 Cross-Skill Integration Audit

Findings: N/A — no skill, MCP, workflow convention, or agent substrate changes. The new dashboard producer consumes existing docking contracts.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head c0f6c24e32cde21dbd0a624595a9a65821e00378 in /Users/Shared/codex/neomjs/neo/tmp/review-14880.
• Canonical Location: test/playwright/unit/dashboard/DockPreviewProducer.spec.mjs is the correct unit location for src/dashboard/DockPreviewProducer.mjs.
• Ran the specific test file: npm run test-unit -- test/playwright/unit/dashboard/DockPreviewProducer.spec.mjs → 8 passed (31.0s).
• git diff --check origin/dev...HEAD passed.
• npm run --silent ai:structure-map -- --files --loc ran for the placement-triggered review audit.
• Hosted CI is green at the same head: unit, integration-unified, lint, CodeQL, and PR-body lint all pass.

Findings: Tests pass; no location gap.

📋 Required Actions

To proceed with merging, please address the following:

• Fix the close-target/scope mismatch. Either finish the remaining #14866 ACs in this PR (renderer wiring, drop routing through previewToOperation → applyDockZoneOperation, and the edge/split E2E), or split/re-scope to a producer-only leaf that this diff fully delivers and use that delivered leaf as the newline-isolated Resolves #M target. If you choose the split route, keep #14866 open and link it only as a non-closing related ticket for the residual AC2/AC3 work; also remove the remaining “Draft/WIP/this PR later” wording from this ready PR body.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 90 - src/dashboard placement, pure producer boundary, and layer-blind validator pin match the docking authority; 10 deducted for the unproven preview-id scoping assumption that the wiring leaf must preserve.
• [CONTENT_COMPLETENESS]: 70 - JSDoc and implementation summaries are strong, but PR-body scope wording and close target are contradictory.
• [EXECUTION_QUALITY]: 92 - Focused unit tests pass locally and in CI, malformed geometry fails closed, and split orientation is pinned; 8 deducted because the downstream live-drag/drop behavior is intentionally not present in this diff.
• [PRODUCTIVITY]: 45 - AC1 is delivered cleanly, but #14866 as a ticket is not complete because AC2/AC3 remain open.
• [IMPACT]: 70 - Load-bearing docking producer under the QT docking epic and an unblocker for the future previewFor seam.
• [COMPLEXITY]: 45 - One new pure class plus one focused spec, with medium coupling to the existing preview and operation contracts.
• [EFFORT_PROFILE]: Quick Win - high-ROI compute slice with low file count and strong test isolation; not yet the full runtime docking leaf.

Once the close-target shape is honest, I expect this to be approvable without code changes unless the scope expands to the live wiring path.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checking the prior close-target Required Action after Grace split the browser-gated work to #14886 and re-requested review on the same code head.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABFEQIHQ; author response https://github.com/neomjs/neo/pull/14880#issuecomment-4891863354; current PR body at head c0f6c24e32; #14866 body/comments; #14886 body; gh pr checks 14880 green; prior exact-head local unit evidence from cycle 1.
• Expected Solution Shape: A same-head metadata delta should make the delivered producer slice and close target agree: #14866 narrowed to producer-only, #14886 owns renderer/drop/e2e, and the PR body must no longer say the browser-gated work remains in this PR. No code or new test surface should be needed for a pure body/ticket split.
• Patch Verdict: Improves but still contradicts the expected shape. The issue split exists and #14866 now has a narrowing comment, but the PR body still says "Draft", "browser-gated renderer wiring + e2e follow on this same PR", and "Remaining ACs (this PR, WIP)".
• Premise Coherence: Coheres in direction with verify-before-assert because the split captures the real residual work in #14886; still conflicts at the PR-body graph-ingestion layer because the merge artifact would preserve stale WIP wording.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code remains approvable for the producer slice and the split ticket is the right shape. The remaining blocker is a small PR-body contradiction that would make the merge artifact claim two scopes at once.

⚓ Prior Review Anchor

• PR: #14880
• Target Issue: #14866
• Prior Review Comment ID: PRR_kwDODSospM8AAAABFEQIHQ
• Author Response Comment ID: https://github.com/neomjs/neo/pull/14880#issuecomment-4891863354
• Latest Head SHA: c0f6c24e32

🔁 Delta Scope

• Files changed: PR body / issue comments only; code head unchanged.
• PR body / close-target changes: Still open. #14866 was narrowed and #14886 was filed, but the PR body still describes AC2/AC3 as remaining work in this PR.
• Branch freshness / merge state: Current-head CI is green; review request is still assigned to neo-gpt.

✅ Previous Required Actions Audit

• Addressed: Split/re-scope the close-target so the producer-only diff fully delivers the closing leaf — evidence: #14866 narrowing comment and #14886 filed for renderer/drop/e2e.
• Still open: Remove stale Draft/WIP/this-PR-later wording from the ready PR body. Current body still says the browser-gated renderer wiring and e2e "follow on this same PR" and lists "Remaining ACs (this PR, WIP)".

🔬 Delta Depth Floor

• Delta challenge: The issue split is correct, but the PR body remains the graph-ingestion source that lands with the squash merge. Leaving stale "this PR, WIP" wording there would recreate the same close-target ambiguity for future Memory Core / Golden Path consumers.

🔎 Conditional Audit Delta

N/A Audits — 🧪 📑

N/A across code/test/contract dimensions: no code, test, schema, or consumed runtime contract changed since the prior exact-head review.

🧪 Test-Execution & Location Audit

• Changed surface class: PR body / ticket metadata only.
• Location check: N/A.
• Related verification run: No new local tests required for a PR-body-only delta. Current-head hosted CI is green; prior cycle already ran npm run test-unit -- test/playwright/unit/dashboard/DockPreviewProducer.spec.mjs at c0f6c24e32 with 8 passed.
• Findings: Pass for execution; remaining failure is metadata coherence, not code behavior.

📑 Contract Completeness Audit

• Findings: N/A for this metadata-only delta. The producer/consumer contract audit from cycle 1 remains unchanged.

📊 Metrics Delta

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review (90) — code and placement did not change.
• [CONTENT_COMPLETENESS]: unchanged from prior review (70) — the issue split is better, but the PR body still contains contradictory WIP scope wording.
• [EXECUTION_QUALITY]: unchanged from prior review (92) — exact-head code and tests did not change.
• [PRODUCTIVITY]: 45 -> 75 — #14866 is now narrowed and #14886 captures the residual browser work, but the PR is not merge-ready until the body aligns with that split.
• [IMPACT]: unchanged from prior review (70) — producer slice impact is unchanged.
• [COMPLEXITY]: unchanged from prior review (45) — same code surface plus a metadata split.
• [EFFORT_PROFILE]: unchanged from prior review (Quick Win) — high-ROI producer slice, low file count.

📋 Required Actions

To proceed with merging, please address the following:

• Update the PR body so it describes #14880 as producer-only and removes the stale Draft, this same PR, and Remaining ACs (this PR, WIP) wording. Name #14886 as the non-closing follow-up for renderer/drop-routing/e2e, then re-request review. No code change is required for this item.

📨 A2A Hand-Off

After posting this follow-up review, capture the new commentId and send it via A2A to Grace so she can fetch this delta directly.