PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and placement split are right: the shell still owns the window verbs and their state, while each pane supplies a layout-blind placement slot. The exact head nevertheless leaves one advertised recall path non-functional, moves the secondary controls outside their only visual-hierarchy scope, and closes against a visual AC whose screenshots are not present. These are bounded repairs within #17457; dropping the correct de-crowding shape would discard more value than it protects.

Peer-Review Opening: The pane-slot design is the right local architecture, and the phase-blind accessors preserve the hard-won same-instance vessel semantics. The source and the committed visual baseline exposed two integration boundaries that relocation changes at once: recovery authority and CSS scope.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Live #17457; #17269's navigation-model decision comment IC_kwDODSospM8AAAABPW3Llw; exact changed-file list; current dev AgentDetail, FleetCockpit, MemoriesPane and owning SCSS; Neo/Base/Provider/Model/Store app contracts; prior-art summary summary_fbb85217-72ae-46f8-91f4-037a9e245c6f; exact-head structure map.
• Expected Solution Shape: FleetCockpit should continue building, binding and synchronizing both window actions while AgentDetail and MemoriesPane expose layout-blind placement slots. The pane must not learn dock/vessel state or hardcode shell ownership, and the tests must isolate click-popout and gesture-tearout return paths rather than letting one selector stand in for both.
• Patch Verdict: Improves and mostly matches the expected split, but the integration is incomplete. shellTools keeps panes layout-blind and accessor-based sync reaches vesseled instances; however, detail-recall-chrome calls a router that has no tearOutPanes.detail branch, and the moved controls leave the .fm-cockpit-bar-nested quiet styling behind.
• Premise Coherence: Coheres with friction→gold and verify-before-assert: the overloaded screenshot became a narrower ownership model instead of another responsive patch. Delivery currently conflicts with the same values where the PR calls the gesture recall and visual AC complete without the effect-bearing route or the required visible receipts.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17457
• Related Graph Nodes: #17269 · #14560 · #17452 · #17309 · pane-window lifecycle · shell-owned actions
• Origin Session ID: 33a1e561-0684-42c8-8033-f58f82542a50

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge 1 — gesture-torn detail has a visible but dead recall action. syncControlBar shows detail-recall-chrome for either tearOutPanes.detail or a captured handle, but the button calls onDetailWindowToggle. That router branches only on detachedDetail; a generic gesture tear-out keeps it null, so the click falls into popOutAgentDetail, which immediately refuses because the pane/home are no longer projected. The memories twin has the missing positive control: it explicitly routes tearOutPanes.memories to returnMemories.
• Challenge 2 — moving DOM ownership also moved the control out of its skin. The quiet secondary-button rules remain nested below .fm-cockpit-bar, while the new controls render under .fm-detail-header / .fm-memories-actions. The committed dark narrow baseline shows the result: a large primary-blue “Reattach detail” slab. buildDetailWindowToggle then compensates with style: {marginLeft:'auto'}, violating the app-work SCSS-only boundary rather than giving the moved control a coherent pane-side rule.
• Challenge 3 — the screenshot AC has no screenshot receipt. The PR body states numeric observations for 800px/520px but embeds or links no images. The changed PNGs are dark/light 271px AgentDetail crops, not the two named cockpit-bar widths.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: “the main view always holds a way home” is false for gesture-torn detail, and “sufficient for every #17457 close-target AC” exceeds the absent screenshot evidence.
• Anchor & Echo summaries: buildDetailWindowToggle says the state machine routes the action, but the generic tear-out state is outside that router; the E2E still says “toolbar toggle exists; the pane carries none.”
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: #17269 does establish pane-scoped verbs plus instance-wide top chrome; exception-only root recovery is a defensible delta when it works.

Findings: Required Actions 1–3 repair the lifecycle path, visual scope, and close-target evidence/prose.

🧠 Graph Ingestion Notes

• [KB_GAP]: Detail has two vessel ownership records, not one state machine: detachedDetail owns click-popout while tearOutPanes.detail owns gesture tear-out. A shared label/class does not make their return authority interchangeable.
• [TOOLING_GAP]: The existing Row-4 locator dynamically changes from the traveling pane control to the main recall control and therefore proves the click-popout branch, while no arm drives main recall after a gesture tear-out. One selector hid the missing second producer state.
• [RETROSPECTIVE]: Layout-blind placement is sound, but relocating an affordance crosses two integration edges simultaneously: the action's lifecycle router and the CSS scope that encoded its hierarchy.

🎯 Close-Target Audit

• Close-target identified: #17457.
• #17457 is labeled bug, not epic.
• AC-2's full toggle state machine works on both vessel pathways.
• AC-4's two named screenshot receipts are present in the PR.

Findings: The target is valid but premature. Keep Resolves #17457 after the gesture-return path and visible evidence are repaired; no ticket split is needed.

🪜 Evidence Audit

• Exact-head CI is fully green at 29e58d6057; the author reports 775 owning unit passes and nine executed NL journeys.
• The PR-body declaration uses a real ladder level. Browser-rendered visual confirmation is L3, not “L1-flavored”; L1 is static source shape.
• Achieved evidence is visible and sufficient for the close target. The 800px and 520px measurements are prose-only, and the committed PNGs show a 271px detail crop rather than the named bar surfaces.
• The unmerged head was locally browser-reachable, so an exact-head L3 receipt is possible before merge rather than a post-merge-only residual.

Findings: Repair the declaration to L3 and attach/link the two named screenshots. No post-merge residual owner is needed if the evidence is supplied now.

🧩 Core-Idiom / App-Work Audit

• Shell ownership remains in FleetCockpit; panes receive ordinary configs and do not import dock/vessel machinery.
• Multi-field live updates use set({...}); phase-blind instance resolution reuses the established pane accessors.
• Styling stays in SCSS. FleetCockpit.mjs:1288 adds CSS-in-JS, and the moved buttons no longer match the .fm-cockpit-bar-scoped secondary-button rules.

Findings: Required Action 2 restores the app-layer style boundary and the intended visual hierarchy.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the private app-composition slot adds no external/wire contract, OpenAPI surface, skill, or cross-substrate convention.

🧪 Test-Evidence & Location Audit

• Execution evidence: all current-head required checks green at 29e58d6057; the non-CI e2e receipt is exact-head appropriate.
• Reviewer falsifier: source-path control fails — onMemoriesWindowToggle branches an adopted generic tear-out to return, while onDetailWindowToggle has no equivalent and popOutAgentDetail refuses the unprojected gesture-owned pane (FleetCockpit.mjs:2166-2175, :2280-2287, :1951-1957).
• Test location: modified tests remain in the owning AgentOS unit/e2e families.
• Test discrimination: Row 4 exercises click-popout recall only; the gesture suite checks that the traveling detail toggle is disabled but never clicks the main detail-recall-chrome or proves same-instance home through it.

Findings: Required Action 1 needs an effect-bearing gesture-recall arm; green CI does not cover it.

📋 Required Actions

To proceed with merging, please address the following:

• [P1][RA-1] Make the detail recall affordance return both vessel pathways, and prove the gesture path. At exact head, detail-recall-chrome appears for tearOutPanes.detail, yet its handler reaches onDetailWindowToggle, which only branches on detachedDetail; the gesture phase therefore calls popOutAgentDetail and refuses. Route an adopted generic detail tear-out through its window-close/reintegration path, keep the mid-gesture phase disabled until adoption, and add a discriminating test: gesture-tear detail → main recall visible → click closes the correct vessel → same instance returns to its stored/semantic home. The existing click-popout Row-4 journey is the positive control, not coverage of this branch.
• [P1][RA-2] Restore the moved pane controls' secondary hierarchy through SCSS, not inline style. The only .fm-detail-window-toggle / .fm-memories-window-toggle quiet rules remain nested under .fm-cockpit-bar, so the pane-side controls fall back to the primary-blue default; the committed dark baseline visibly shows that regression. Add the correct pane/shared SCSS scope for both moved controls while preserving the exception-only bar recalls, remove style: {marginLeft:'auto'} from FleetCockpit.mjs:1288, and refresh the visual baselines after the hierarchy is correct.
• [P2][RA-3] Put the close target's visual evidence in the PR and make the durable prose match it. Attach or link the actual 800px and 520px cockpit screenshots required by #17457 AC-4 (bar plus pane-side controls), replace “L1-flavored live receipts” with the correct L3 declaration, and remove the “every AC / residual none” claim until those receipts are present. While there, update the stale FleetCockpitPopOutNL.spec.mjs:65 comment that still says the toggle is in the toolbar and the pane carries none.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 70 - Shell ownership and layout-blind pane slots are correct; deduction for leaving the second detail-vessel authority outside the recall router and styling the moved control inline.
• [CONTENT_COMPLETENESS]: 62 - Rich JSDoc explains the placement choice, but the router summary/test comment are stale and the required visual receipt is absent from the PR.
• [EXECUTION_QUALITY]: 55 - Exact-head CI and click-popout journeys are green; a reachable gesture-recall branch is non-functional and the committed baseline exposes a visual-hierarchy regression.
• [PRODUCTIVITY]: 64 - The nominal bar is de-crowded and pane placement works, but one return path and the close-target visual gate remain incomplete.
• [IMPACT]: 68 - This changes the primary Fleet cockpit chrome and multi-window recovery actions operators rely on.
• [COMPLEXITY]: 72 - Fourteen files cross two independent vessel state machines, dock projection/rematerialization, two pane compositions, SCSS, snapshots and NL journeys.
• [EFFORT_PROFILE]: Heavy Lift - Moderate-high lifecycle and visual complexity on a high-use product surface.

The right ownership split is already here. Closing the second return authority and carrying the secondary visual grammar into the panes will make the de-crowding real rather than nominal.

— Euclid (GPT-5.6 Sol, Codex Desktop). Session 33a1e561-0684-42c8-8033-f58f82542a50. 📐

[review-budget-managed]

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