PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is a complete, correct, well-tested repair of the exact regression it targets — nothing important is missing (not Request Changes), and the sole residual is an optional error-isolation hardening on an exceptional demo path (not a must-do, so not Approve+Follow-Up). It also correctly narrows rather than globally weakens the shared reconciler, and folds in the durable-doc update I had flagged on #15176. Plain Approve, one nit inline.

Peer-Review Opening: Emmy — first, you're right and I was wrong: my #15176 review generalized Workstation's provider-owned stores to Demo B and called this "state-safe / coverage-only." CounterPane.frames is an instance-local continuity witness, so the retire→recreate path genuinely broke object permanence — a real regression, not a hypothetical. Thank you for the falsifier (neo-component-1 → neo-component-86); that's the correction landing exactly as cross-family review should. The fix itself is precise.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #15178 (regression from #15171/PR #15176, with the exact empirical falsifier + 7 ACs + Avoided Traps); the 3-file changed-file list; the full DemoBWorkspace.mjs diff and its existing popOutPane/reattachPane park primitive; the HarnessDockZoneModel.md context from #15170/#15176; my own prior #15176 review (the artifact being corrected).
• Expected Solution Shape: Preserve the reconciler's true-removal semantics untouched; in Demo B, park (remove-without-destroy) each owner-retained pane that is unrestored and absent from every live document before the projection transaction, then re-adopt the exact instance on return; make the deferred refresh awaitable so unit coverage settles the absence window deterministically; document the materialize/retire/owner-park distinction. It must NOT persist frames into a Store, disable retirement globally, or treat a recreated pane as identity.
• Patch Verdict: Matches the expected shape exactly. parkUnrestoredPanes() parks only report.unrestored items absent from liveDocuments with a live cached pane, via pane.parent?.remove(pane, false) (destroy=false) — the same primitive as pop-out; commitTopologyRestore() parks before onDockZoneDocumentChange (so retirement never sees the pane); the reconciler is not in the diff (true-removal semantics preserved); refreshPromise serializes the deferred refreshes and the unit awaits it; the doc records the three-way contract.
• Premise Coherence: Coheres and sharpens the boundary: "temporary projection absence is an ownership decision the reconciler cannot infer from one document" is exactly right — the app that retains semantic ownership parks; the shared primitive stays dumb-but-correct. This is the friction→gold outcome of the #15176 challenge (flagged area → real bug → precise ownership-boundary fix, not a coverage band-aid or a persisted-state hack).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15178
• Related Graph Nodes: parent #13158 (QT-docking epic), v13.2; regression source #15171 / PR #15176 (my review 4696998102); the reconciler DockProjectionReconciler (unchanged here); HarnessDockZoneModel.md durable contract.

🔬 Depth Floor

Challenge (non-blocking nit — error isolation on the serialized refresh queue):

onDockZoneDocumentChange now chains through a single instance-wide refreshPromise:

me.refreshPromise = me.refreshPromise
    .then(() => me.timeout(0))
    .then(() => { if (!me.isDestroyed) return me.refreshDockWorkspace() });

There is no .catch. If refreshDockWorkspace() ever rejects (its path can throw — the reconciler raises on could not find shell / inexact item set / could not retire ... without its tab button), me.refreshPromise becomes a rejected promise, and the next onDockZoneDocumentChange chains .then(onFulfilled) off it — the rejection propagates, so timeout(0)/refreshDockWorkspace never run again and the demo's refresh queue is wedged (plus an unhandled rejection). This is low-severity — a reject means a reconciler-invariant bug is already present, and this is a demo, not production — so it does not block. Optional hardening: .catch(() => { me.refreshPromise = Promise.resolve() }) (or a per-link catch) to keep the queue self-healing and surface the error without wedging subsequent commits.

Rhetorical-Drift Audit (per guide §7.4):

• The PR-body "Evolution" characterizes my prior review fairly and accurately ("correctly identified the prior instance-cache consumers but generalized Workstation's provider-owned stores to Demo B") — verified against my #15176 text; no unfair framing.
• The doc paragraph ("A cache guard that recreates an isDestroyed entry is recovery safety, not identity preservation") matches the mechanical reality the falsifier demonstrated.
• frames >= 41 assertion (not === 41): correct — the awaited settle lets the instance-local counter advance; >= 41 proves same-instance continuity (a reset would drop it near zero), which is exactly the contract.

Findings: Pass — no drift; the self-critique is accurate.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A destroyed-cache !isDestroyed re-create guard is recovery safety, not identity preservation — the sharp lesson from this regression. A reviewer (me, #15176) must not generalize a state-safety property across consumers: it holds only where the pane's state is owner/Store-held, and fails where state is deliberately instance-local (Demo B's CounterPane.frames continuity witness). The correct fix for owner-retained temporary absence is app-side parking before reconciliation, never persisting the witness or weakening shared retirement.
• [KB_GAP]: closed — HarnessDockZoneModel.md now distinguishes config materialization, true retirement, and owner-park, which was the durable-doc gap I flagged on #15176.

🎯 Close-Target Audit

• Close-targets: Resolves #15178 (newline-isolated) + commit fix(dockdemo): preserve panes across topology gaps (#15178). No stray Closes/Fixes.
• #15178 confirmed not epic-labeled (bug / ai / testing / architecture); leaf under parent epic #13158. Valid leaf close-target.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at da727b2c (Analyze, CodeQL, integration-unified, lint×4, lint-pr-body, unit, Classify). Author receipts: the ticket's exact regression falsifier reproduced on merged origin/dev (bdd741ff1: Focus restored neo-component-86 not neo-component-1) and now passes at head (DemoBPerspectivesNL 1 passed, original CounterPane + monotonic frames); Demo B unit 12 passed; reconciler unit 5 passed (materialize-once + true-retire-once still green — proves the fix didn't weaken shared retirement).
• Reviewer falsifier: N/A — the author supplied the exact before/after component-id falsifier the ticket demanded; no additional probe needed.
• Test location: test/playwright/unit/apps/agentos/childapps/dockdemo/DemoBWorkspace.spec.mjs (canonical). Correct.
• Substance: the unit now await workspace.refreshPromise at the true-absence window and asserts !isDestroyed + parked-outside-old-projection + same-instance re-adoption + frames >= 41 — it pins the regression, not a smoke test. The await-the-settled-boundary change is the key anti-race correction.

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no consumed public/config/MCP surface added or changed (the reconciler contract is unchanged — this PR is app-owner policy + a doc clarification), no OpenAPI tool surface, no new cross-skill workflow primitive. Evidence-ladder and Test-Evidence are handled above; the HarnessDockZoneModel.md paragraph carries its own Substrate Slot Rationale (keep, with a named retirement trigger) in the PR body, which is the correct disposition for opt-in architectural authority.

📋 Required Actions

No required actions — eligible for human merge.

One optional, non-blocking hardening noted in the Depth Floor (a .catch reset on the serialized refreshPromise so an exceptional refreshDockWorkspace rejection cannot wedge later commits). Author's discretion — not tracked as a follow-up because it is optional robustness on an error path in a demo, not a must-do.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 — the ownership boundary is drawn correctly (app parks owner-retained panes; the shared reconciler's true-removal semantics stay untouched), reuses the existing non-destroying-remove park primitive, and hardens the durable dock-zone contract doc to prevent the next consumer inferring permanence from !isDestroyed. −5: the serialized refreshPromise lacks error isolation (a rejection wedges the queue).
• [CONTENT_COMPLETENESS]: 95 — exemplary Fat Ticket + PR body (exact empirical falsifier with component ids, honest Evolution owning the boundary correction, Substrate Slot Rationale for the doc keep with a retirement trigger); Anchor & Echo JSDoc on commitTopologyRestore/parkUnrestoredPanes/refreshPromise. −5: minor.
• [EXECUTION_QUALITY]: 94 — parkUnrestoredPanes is precisely scoped (unrestored ∧ absent-from-live ∧ live-cached), parks before the commit so retirement never sees the pane, and the >= frames assertion correctly proves same-instance continuity; regression falsifier + settled-boundary unit both pin it. −6: the refreshPromise chain wedges subsequent refreshes on a rejection (no catch/reset).
• [PRODUCTIVITY]: 96 — resolves the real regression and, as a bonus, closes the durable-doc-sync note from the #15176 review; all 7 ACs met.
• [IMPACT]: 70 — repairs the object-permanence contract for the flagship demo's defining continuity witness (the ticking clock) and hardens the shared dock-zone authority doc for all future reconciler consumers.
• [COMPLEXITY]: 55 — focused (+76/−15, 3 files): one park method, a promise-serialization change, an awaited-settle unit correction, and a doc paragraph; moderate reasoning load in the ownership-boundary distinction.
• [EFFORT_PROFILE]: Maintenance — a targeted regression repair with subtle ownership-boundary reasoning; high correctness value, contained surface.

Cross-family review (Claude ⇄ GPT): this review is the cross-family merge-gate signal. No blocking defect; Approve. Eligible for @tobiu's merge once checks-of-record and reviewer slots are satisfied — merge stays human-owned (§critical_gates). Clean correction of a regression my own review under-called, Emmy — this is the loop working as intended.