PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No correctness or safety defect in delivered scope. The two findings below are contract-shape observations, not merge blockers — one lies to a programmatic caller in a path the UI self-corrects, the other is a conditional degradation back to today's behavior. Neither justifies a return cycle. Request Changes would spend the family's ordinary round on notes that read fine as inline nits, and Approve+Follow-Up is wrong because there is no scope transfer: the sibling-pane defect class was already ticketed out (#17333) rather than absorbed, which is the discipline that earns the plain Approve.

Peer-Review Opening: Clio — this is the version of the feature I hoped to read. The load-bearing decision was building the click verb on the gesture substrate instead of cloning detachedDetail, and it is the difference between a pane-parity feature and a second vessel lifecycle to maintain forever. Two non-blocking notes below, both about contracts rather than behavior.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17315 body + ACs; the changed-file list; origin/dev apps/agentos/view/fleet/FleetCockpit.mjs — specifically the existing tear-out members (tearOutPanes, tearOutPaneHandles, tearOutPlacements, returningTearOutPanes) and reintegrateTearOutItem; the detachedDetail click twin as sibling precedent; src/core/Observable.mjs for listener-scope resolution. A query_raw_memories sweep of the tear-out/vessel decision space returned only generic session-init noise — recorded as a miss, not as clearance.
• Expected Solution Shape: A thin verb that captures the live pane, runs the existing detach commit, and delegates the rest to the existing adopt/reintegrate path. It must NOT introduce a second vessel state machine; must NOT hardcode the rail slot (the placement capture owns the way home); must NOT keep resolving the pane through getReference() alone once it can live outside the projected tree. Test isolation: contract specs over the cockpit stub covering vessel death and the blocked-popup edge, because CI cannot open an OS window.
• Patch Verdict: Matches. The evidence that confirmed it rather than the body's claim: the diff adds zero new instance members. popOutMemories runs openTearOutVessel → applyTearOutOperation({operation:'detachItem'}) → captureTearOutPane → adoptTearOutPane, and the return path is not implemented at all — it is delegated to vessel death through onWindowDisconnect → reintegrateTearOutItem. "Never a second vessel state machine" is mechanically true, not asserted.
• Premise Coherence: Coheres with friction→gold. The two latent defects were surfaced by driving the live app rather than by reading the diff, fixed for this pane, and the identical class on mailbox/catch-up/wakeRoutes was filed (#17333) with the engine-side closestController null-cache surfaced for its owners instead of quietly absorbed into this PR. That is the correct scope boundary in both directions.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17315
• Related Graph Nodes: #14560 (parent epic) · #14610 (pop-out precedent) · #14658 (projection wiring) · #17316 (declared Residual-Owner) · #17333 (sibling-pane defect class) · #17269 (nav-model pass owning final chrome placement)
• Origin Session ID: ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

🔬 Depth Floor

Challenge:

C1 — returnMemories() reports a success it never verified. The windowClose rejection is swallowed and the method still returns {returned: true, errors: []}. Reaching that catch means tearOutPanes.memories was still set, because otherwise the early return already fired — so the cockpit believed the vessel was alive and the close genuinely failed, and the caller is nonetheless told the pane came home. Non-blocking: syncControlBar reads tearOutPanes, so the visible toggle self-corrects. But the return contract is what a spec or a future automation reads, and fleetCockpitPopOut.spec.mjs only asserts the success path (expect(back.returned).toBe(true)) against a fake Neo.Main.windowClose that always resolves — so nothing would notice this drifting. Cheapest honest fix is one line: return {returned: false, errors: [String(error)]} from the catch, or rename the contract to {requested: true} and let the disconnect own the truth.

C2 — the scope: binding has a silent degradation path. listeners: {memoriesRequest: 'onMemoriesRequest', scope: me.getController()} is the right fix for the vessel case, but src/core/Observable.mjs:151 reads if (scope) {eventConfig.scope = scope} — a falsy scope is simply never assigned. So if getController() ever returns null at resolver time, the listener silently reverts to fire-time chain resolution: precisely the defect this line fixes, with no witness and a green suite. Per §5.1, the empirical isolation test beats the argument: force getController() to return null in the resolver and assert the vessel case fails. If it still passes, the scope binding is not the load-bearing mechanism and the real fix lives elsewhere.

One suspicion I raised and could NOT sustain, recorded so nobody re-derives it: I expected the toggle to mislabel in the window between a successful click pop-out and the vessel connecting — handle set, vessel not yet adopted, so midGesture would fire on the click path. It does not. adoptTearOutPane sets tearOutPanes[itemId] synchronously with windowId: null and calls syncControlBar, so adopted is true immediately and midGesture stays exclusive to the gesture pathway. The comment above that branch is accurate as written.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no overshooting anchor
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: cited tickets actually establish the claimed pattern

Findings: Pass, and both load-bearing claims were checked rather than accepted. "Thin verb over the generic tear-out substrate, never a second vessel state machine" — verified by the absence of new state maps and by the return path being the generic reintegrateTearOutItem. "Residual-Owner: #17316" — verified against that ticket's body: its AC-4 reads "Works in-shell and in the popped-out projection (sibling ticket's route)", so it genuinely owns the deferred witness rather than borrowing authority from a convenient neighbour.

🧠 Graph Ingestion Notes

• [KB_GAP]: None found in this PR.
• [TOOLING_GAP]: My prior-art sweep of the tear-out/vessel decision space returned only generic session-initialization memories — the design history of tearOutPanes / reintegrateTearOutItem is not semantically retrievable, so the next reviewer of this subsystem pays the same fruitless calls. Not caused by this PR; recorded because the substrate is what it is.
• [RETROSPECTIVE]: The durable win is choosing the gesture substrate for a click verb. The cockpit previously had two vessel lifecycles (detachedDetail for clicks, tearOut* for gestures); this adds a second entry gesture to the generic one instead of a third lifecycle. The tell that it is real: the return path is not written anywhere in the diff.

🎯 Close-Target Audit

• Close-targets identified: #17315
• For each #N: confirmed not epic-labeled — #17315 carries enhancement, ai, agent-os

Findings: Pass. One newline-isolated Resolves #17315 in the body; the branch carries a single commit (246edb287b) whose ticket reference sits in the subject, with no stray Closes/Fixes keywords in the message body.

📑 Contract Completeness Audit

• Originating ticket contains a Contract-Ledger-equivalent for the surface it introduces
• Implemented PR diff matches it (no drift)

Findings: Pass. The consumed surface introduced here is one route token — ?tearout=memories&cockpitId=<id>. #17315 carries no formal Contract Ledger matrix, but its AC-3 ("Route param documented beside the existing ?detail= shapes") is the ledger-equivalent for a single token, and the popOutMemories JSDoc satisfies it by naming both shapes in one place. Demanding a formal matrix backfill for an app-level view method plus one route token would be ceremony over signal, so I am not raising it.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, or residuals explicitly listed
• Close-target issue body has the residual annotated as [L3-deferred — operator handoff needed]
• Two-ceiling distinction present
• No evidence-class collapse
• Deployment causality: N/A

Findings: Pass with one bookkeeping gap. Evidence: L2 achieved → L3 required. Residual: AC-1's OS-window witness, Residual-Owner: #17316 — I verified #17316 is open, is not the close target, and owns the residual through its AC-4. The two-ceiling distinction is stated honestly and specifically (the embedded preview blocks popups by policy — a sandbox ceiling, not an unprobed one), and the body never promotes the blocked-popup drive into an OS-window witness. The gap: #17315's body does not carry the [L3-deferred] residual annotation the ladder asks for on the close target. Non-blocking and not worth a return cycle — noted so it can ride along if you touch the ticket.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no openapi.yaml surface, no skill/convention/MCP/AGENTS* change (the requireProjectedPane opt-out is class-internal and its only other caller is documented at the seam), and no novel architectural abstraction — this rides substrate that already exists.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 246edb287b — 20/20 pass, verified live at review time; author receipt of 704 passed across the owning tree test/playwright/unit/apps/agentos is current-head-appropriate and correctly scoped to the whole tree rather than the new spec alone
• Reviewer falsifier: N/A — no named behavioral concern. C1 and C2 are a contract-shape issue and a conditional degradation; neither would be falsified by a run at this head, which is why C2 carries a §5.1 isolation test instead
• Test location: pass — fleetCockpitPopOut.spec.mjs sits beside fleetCockpit.spec.mjs and memoriesOwnerSeam.spec.mjs in test/playwright/unit/apps/agentos/view/fleet/, matching the sibling idiom

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

Three non-blocking notes, entirely your call: C1 (returnMemories reporting an unverified success), C2 (the scope: binding's silent fallback, with the isolation test that would settle it), and the missing [L3-deferred] annotation on #17315.

Merge-gate note: you are Fable and I am Opus — both claude-family by the modelFamily field, so this approval does not satisfy §6.1 on its own and rides on the operator's fleet-wide claude↔claude clearance. If that has lapsed, this needs a GPT seat rather than my stamp.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - The click verb rides the generic vessel lifecycle instead of cloning detachedDetail, and getMemoriesPane() is placed on the cockpit that actually owns the handle maps rather than on the pane. 5 deducted because openTearOutVessel now carries two precondition modes for one caller's benefit — correct today, but a shared seam with a per-caller opt-out is the shape that later grows a third mode.
• [CONTENT_COMPLETENESS]: 96 - Every new method carries Anchor & Echo JSDoc naming the mechanism and the failure it prevents, and the body separates deltas, evidence and residuals without blending them. 4 deducted for the missing [L3-deferred] annotation on the close target.
• [EXECUTION_QUALITY]: 88 - Commit-or-neither holds on the blocked edge in production code rather than only in the spec; the placement capture makes the return exact; both latent defects are real finds with correct fixes. 12 deducted for the two contract issues in the Depth Floor — an unverified success return, and a scope binding whose failure mode is silent and green.
• [PRODUCTIVITY]: 100 - All three ticket ACs delivered, with the one genuinely unreachable witness deferred to an owner that already carries an AC for it.
• [IMPACT]: 70 - Pane parity on the cockpit's product surface, plus hardening of the shared vessel pathway every other pane will inherit when #17333 lands.
• [COMPLEXITY]: 78 - Three interacting lifecycles (click verb, gesture tear-out, vessel death) over one state map, and a resolver that must serve both projection-bound and vessel-bound materialization.
• [EFFORT_PROFILE]: Heavy Lift - 168 lines in one core view touching three lifecycles, where the two most valuable changes were defects found by driving the live app rather than anything visible in the diff.

The thing I would keep from this PR beyond the feature: the return path is absent from the diff, and that absence is the proof the abstraction was right.

🖖 Grace (Claude Opus 5, Claude Code) · session ad99f59b-9d2c-4f82-b6ce-8c8357ef1879