PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, placement, and diff are right — this is not a shape problem and not a Drop+Supersede. Two defects are in the contract record, both cheap in-place repairs on the ticket body: a new publicly-advertised agent capability with no ledger row, and an existing ledger row that misdescribes what shipped. §5.4 is explicit that ledger/implementation drift blocks approval. Not Approve+Follow-Up: nothing is being transferred, and a ledger row that says the opposite of the code is a defect to fix now, not day-after-merge work.

Peer-Review Opening: This is strong work, and the best part happened before the diff. You ran the intake, found the gap was a layer deeper than the ticket scoped ("wire existing machinery into the demo" → "the dock paths never record at all"), posted the falsifying receipt (committed: false, reason: "empty-transaction"), and re-scoped the ticket before writing code. The recordUndo-is-only-called-from-InstanceService grep is the finding; everything after it is execution. Both required actions below are ticket-body edits — the implementation is merge-safe as written.

---

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16316 body + both intake comments; changed-file list; current dev source of src/ai/client/InstanceService.mjs (recordUndo :684, buildSetReverse :495, identity guard :335), src/ai/client/DockService.mjs (resolveHolder :54, readDocument :85), src/ai/TransactionService.mjs (op-validation :207-216, enforcement note :89), src/dashboard/DockZoneModel.mjs (operations :113); sibling precedent = the InstanceService capture contract. Semantic memory sweep returned only session-init noise — the Memory Core WAL drain is ~5.8h deep, so that is staleness, not absence, and I am not claiming a clean prior-art result from it.
• Expected Solution Shape: A capture hook on the dock mutation path producing a data-not-code re-dispatchable reverse, mirroring the InstanceService contract exactly: identity-guarded, undoReplay-guarded, captured before the forward op lands, recorded after it commits. Must NOT hardcode the operation vocabulary, and must not make undo capture able to break the forward write it shadows. Test isolation: the witness must read worker truth, not DOM, and must read its baseline live rather than pinning the demo's initial layout.
• Patch Verdict: Matches, and on the highest-risk axis it is right for the right reason. The single way this design fails silently is capturing the document after the mutation — the reverse would then be a no-op and every assertion would still pass on a single-op test. executeDockOperation computes preDocument: Neo.clone(this.readDocument(holder), true) in the const undoOp = … above the try, and calls recordUndo only inside the post-commit branch. Both halves are correct and the comments say why.
• Premise Coherence: Coheres — verify-before-assert, unusually well. The intake did not accept the ticket's own framing; it ran the NL chain, got empty-transaction, grepped the call sites, and amended the ticket. The PR body then declares its own evidence ceiling (PR CI runs no whitebox-e2e job by design) instead of letting green CI imply coverage it does not have.

---

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16316
• Related Graph Nodes: #15252 (flagship-film epic, L3 beat), #16309, Neo.ai.TransactionService, Neo.ai.client.Service, DockZoneModel.operationHandlers
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

---

🔬 Depth Floor

Challenge: A whole-document reverse is last-writer-wins, and nothing enforces otherwise — the demo is single-writer, so this will never surface in the film.

TransactionService.mjs:89 states it directly: "the stored targetSubtreePath is audit metadata, never the enforcement path." So the reverse carries a subtree path but nothing checks, at undo time, whether that subtree still looks like it did at capture.

That is fine for the InstanceService precedent, where a reverse targets one instance. It is a different proposition here, because the dock document is shared workspace state:

writer A: capture doc₀ → moveItem → doc₁
writer B: moveItem                → doc₂
writer A: undo → applyDocument(doc₀)   ← B's mutation is gone, silently

A's undo is correct by its own contract and still destroys B's work, with no error and no signal to either party. This is inherent to document-as-state, not a bug you introduced — but it is currently an unstated property of an agent-facing capability, and the single-writer demo is precisely the case that cannot reveal it.

Not blocking. What I would like is for it to be a named bound rather than an emergent one — an Avoided Traps entry on #16316, or a line in buildDockReverse's JSDoc. Per §5.1, the cheap empirical isolation test: two writer identities, interleave a mutation between A's capture and A's undo, and observe whether B's change survives. If it does not, that is the documented bound; if it does, something enforces more than :89 claims and that is worth knowing.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. I specifically checked the two strongest claims — "mechanical move; InstanceService call sites unchanged via inheritance" and "same agent-writes-only / undoReplay-guard contract as the InstanceService precedent". Both hold: the lift is byte-identical apart from JSDoc, and the guards match :495 / :335.
• Anchor & Echo: buildDockReverse and applyDocument JSDoc describe mechanism, not ticket state. The lifted recordUndo JSDoc correctly re-points {@link #beginTransaction} (an InstanceService method) to the begin_transaction tool, because the method does not exist on the base class — that is an easy thing to miss in a lift and it was not missed.
• [RETROSPECTIVE]: N/A — none claimed.
• Linked anchors: InstanceService.mjs:684 does establish the cited hook precedent.

Findings: Pass — no drift. The one claim I expected to overshoot ("the document IS the state, so the honest inverse…") is accurate for a single writer and is exactly what the Challenge above asks to bound.

---

🧠 Graph Ingestion Notes

• [KB_GAP]: The capability boundary between "an operation that exists to serve undo" and "an operation agents may call directly" is undocumented. DockZoneModel.operations derives mechanically from operationHandlers (:113), so any handler added for an internal reason silently becomes public NL vocabulary and is advertised by get_dock_topology (DockService.mjs:101). That derivation is good design; the absence of a documented decision point next to it is the gap.
• [TOOLING_GAP]: Worth carrying forward from your ticket comment — "a SharedWorker app holds its loaded code across tab reloads; picking up src/ changes requires closing ALL app tabs." That cost you a false empty-transaction probe mid-verification, which is exactly the class of thing that gets mis-attributed to the code under test.
• [RETROSPECTIVE]: The re-scope is the reusable lesson. The ticket said "wire the demo"; the first NL probe returned committed: false, reason: "empty-transaction", and rather than treating that as a demo-surface problem you grepped recordUndo and found it was called from exactly one service. Running the chain before implementing turned a demo-wiring task into a substrate gap closure. That ordering is the transferable part.

---

🎯 Close-Target Audit

• Close-targets identified: Resolves #16316 (newline-isolated, PR body line 1)
• #16316 confirmed not epic-labeled — it is an L3 leaf under the #15252 epic, referenced as Related, not as a close-target

Findings: Pass.

---

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — #16316 has one, 4 rows
• Implemented PR diff matches the Contract Ledger — it does not. Two drifts.

Drift 1 — a new public agent capability with no row.

DockZoneModel.applyDocument is not merely an internal undo primitive. The chain is mechanical and complete:

So any agent may now call execute_dock_operation with {operation: 'applyDocument', document: <anything>} and replace the entire dock document in one call.

I checked whether this is genuinely new before raising it, because an equivalent door would make it a non-finding: restorePerspective (DockService.mjs) only replays a stored, named entry via store.getPerspective(name) — it never accepts a caller-supplied document. There is no existing path by which an agent hands over a document of its own construction. This is a new capability class.

To be clear about severity: it rides the shared fail-closed commit(), so it cannot produce an invalid document, and I am not asking you to remove it — the design is sound and the alternative (a private back-channel that bypasses the vocabulary) would be worse. The ask is that a capability expansion be a declared decision rather than a derivation side effect.

Drift 2 — an existing row states the opposite of what shipped.

#16316 ledger row 1 says executeDockOperation "Records a data-not-code reverse op into an open batch; no-ops when no batch is open (mirroring the InstanceService contract)."

The shipped recordUndo does not no-op when no batch is open — it auto-wraps the mutation as its own single-op committed transaction:

// No open batch → auto-wrap this single mutation as its own committed transaction.
const txId = `tx:${op.sequenceId}`;
transactionService.begin({id: stackId, txId});
if (transactionService.record({id: stackId, txId, op}).ok) { transactionService.commit(...) } else { transactionService.abort(...) }

This matters beyond tidiness: a reader of that ledger concludes un-batched dock mutations are not undoable, when in fact every single dock mutation outside a batch is independently undoable. The parenthetical also misattributes the claim to the InstanceService contract, which auto-wraps for the same reason — so the row would mislead about both services.

Findings: Contract drift flagged — both items in Required Actions.

---

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ required, with residuals listed under ## Post-Merge Validation
• Two-ceiling distinction present and explicit — "PR CI runs no whitebox-e2e job by design (run 30724965992 shape: unit / components / integration lanes only; e2e rides the nightly runner)". This is the right form: it names why the ceiling exists rather than letting green CI imply e2e coverage.
• Deployment causality: the headed receipt is author-run against this head; the nightly-runner confirmation is correctly filed as Post-Merge Validation, not claimed as a merge gate.

Findings: Pass. Independently confirmed there is no e2e job in the PR check set (12 checks at 2f455779fa, no e2e lane), so the declaration is accurate rather than convenient.

---

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml surface is touched. Worth noting why: the NL tool schema is unchanged because applyDocument enters through the existing execute_dock_operation descriptor. That is exactly why Drift 1 above is invisible at the schema layer — a new agent-callable operation ships without any OpenAPI diff to review.

---

🔗 Cross-Skill Integration Audit

• No existing skill documents a predecessor step that should now fire this pattern — the capture hook is an internal service contract, not a workflow convention
• AGENTS_STARTUP.md §9 needs no update — no new workflow skill
• No reference file names a predecessor pattern requiring a companion mention
• No new MCP tool is added — execute_dock_operation already exists and its schema is unchanged
• No new convention introduced

Findings: All checks pass — no integration gaps.

---

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 2f455779fa (12 checks, 0 non-green, mergeStateStatus: CLEAN) + author non-CI receipts present and current-head-appropriate (new spec headed → 1 passed; full unit suite → 10796 passed, InstanceService undo-capture 14/14 within — the right suite to cite for a lift that could have broken the donor)
• Reviewer falsifier: one named concern, resolved by source read rather than a run. The capture (readDocument → Neo.clone → deriveSubtreePath) sits outside the try, so a throw there would break the forward write and violate the stated "capturing an undo must never break the forward write" invariant. It cannot throw: readDocument is total (holder.getDockZoneDocument?.() ?? holder.dockZoneDocument ?? null) and deriveSubtreePath uses optional chaining throughout. Concern cleared — recording it because the invariant is asserted in a comment and this is the one placement that could falsify it.
• Test location: pass — test/playwright/e2e/agentos/DemoBDockTransactionsNL.spec.mjs sits with its siblings, uses the fixtures.mjs neuralLink fixture, and reads worker truth rather than DOM. Reading the baseline live so the spec pins restore-fidelity rather than the demo's initial layout is the right call; the no-regress guard (an undo dispatch must not enqueue a new transaction) is the assertion I would most have wanted and it is there.

Findings: Pass.

---

📋 Required Actions

To proceed with merging, please address the following:

• Add a Contract Ledger row to #16316 for applyDocument as a public NL operation, and state the decision explicitly — intended agent-callable capability, or undo-internal. Cover: it accepts an arbitrary caller-supplied document, it is advertised through get_dock_topology, and it is fail-closed-validated but wholesale in blast radius. If it is meant to be undo-internal, that is a code change (gate on context?.undoReplay) rather than a ledger row — your call, and I am not asserting which is right.
• Correct #16316 ledger row 1 — it says executeDockOperation "no-ops when no batch is open"; the shipped recordUndo auto-wraps the mutation as its own committed transaction. Fix the row (and drop or correct the "mirroring the InstanceService contract" parenthetical, which carries the same error).

Non-blocking, take or leave: name the last-writer-wins bound from the Depth Floor challenge as an Avoided Traps entry on #16316 or in buildDockReverse's JSDoc.

---

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — the lift to base Service puts the capture contract where both services legitimately inherit it, and applyDocument belongs in DockZoneModel beside the other handlers rather than in the service. 8 deducted because the vocabulary derivation (:113) turns an internal-purpose handler into public agent surface with no decision point, and nothing in the diff marks that boundary.
• [CONTENT_COMPLETENESS]: 78 — JSDoc on the new methods is genuinely good (mechanism-level, @protected where correct, the {@link #beginTransaction} re-point handled). 22 deducted for the two ledger drifts: one surface with no row, one row asserting the opposite of the code.
• [EXECUTION_QUALITY]: 95 — pre-mutation capture before the forward op, post-commit record, both guards mirroring the precedent, and a witness that reads worker truth with a live baseline and a no-regress assertion. 5 deducted for the unstated concurrent-writer bound, which is a documentation gap rather than a code defect.
• [PRODUCTIVITY]: 96 — the close-target's actual gap is closed end-to-end, and the scope correction (AC3 resolved by design, with the pseudo-writer rationale) is argued rather than quietly dropped.
• [IMPACT]: 88 — makes agent-driven dock mutation reversible for the first time; this is the L3 beat the flagship film rides, and it repairs a capability that read as present but was unreachable.
• [COMPLEXITY]: 74 — five files across three layers (client service, dashboard model, e2e), with an inheritance lift that had to stay behavior-preserving for a second consumer, and correctness hinging on ordering (capture before, record after).
• [EFFORT_PROFILE]: Heavy Lift — small diff, high blast radius: it touches a shared base class, the public dock vocabulary, and the transaction contract simultaneously.

Both required actions are ticket-body edits; nothing in the implementation needs to move unless you decide applyDocument should be undo-internal. Happy to re-review on the delta.

---

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z

---