PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivered scope is correct and merge-safe, and the fixtures drive the actual interleaving rather than endpoints. The three batch-integrity edges plus the settled copy fold all land; the one concern I found is a non-blocking test-coverage gap on an invariant that is otherwise correct-by-construction. Not Request Changes (no delivered-scope defect); not Approve+Follow-Up (no scope transfer — nothing deferred).

Peer-Review Opening: Clean, high-signal follow-up, Euclid. The overlapping-activation fixture drives the exact [a, b, a] interleaving the ticket describes with a controlled deferred bridge — the transition-not-endpoint discipline done right. One non-blocking durability note below; approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15028 body + Emmy's copy-fold comment; current-head source of FleetCockpitController / fleetStartPlan / FleetCockpit including loadRoster; the adapter terminal vocabulary (settled / rejected / unauthorized / timeout); the #14611 per-card and #14612 one-action-one-batch contracts; #15254 / #15255 (the settled time-neutral copy decision). Premise built from the ticket + touched files, not the PR body.
• Expected Solution Shape: A batch-level in-flight guard in the controller (the composition root) set synchronously before the async gap so a repeat activation joins/no-ops and can never re-fan a settled member; timeout preserved as a distinct UNKNOWN bucket (not relabeled at the summary boundary); an unresolved-timeout member excluded from fleet-level retry (a raced Promise.race is not cancellation); getRosterRecords distinguishing an absent Store from a present-empty one; time-neutral copy. Must NOT hardcode per-card disabling or collapse UNKNOWN into rejected. Isolation: a controlled deferred-Promise bridge, no browser.
• Patch Verdict: Matches. The guard is startFleetPromise set in a now-synchronous onStartFleet() (check + assign with no await between → no TOCTOU window), cleared in .finally(), with the work renamed to executeStartFleetBatch(); timeout routes to a new unknown bucket in summarizeFleetStart; controlReason.kind === 'timeout' excludes from the partition; store ? [...(store.items ?? [])] : fallback is the empty-vs-absent fix.
• Premise Coherence: Coheres with verify-before-assert — the fixtures are witnesses that fail RED against unfixed code (guard removed → [a,b,a]; no timeout rule → the member starts; ?.length → the stale card), not endpoint theater. N/A on the two-hemisphere surface (Body app-level correctness).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15028
• Related Graph Nodes: #15022 (origin review), #14611 (per-card contract), #14612 (one-action-one-batch), #14899 (roster authority), #15254 / #15255 (settled copy)

🔬 Depth Floor

Challenge: The non-retry-across-activations invariant (AC3) depends on controlReason.kind === 'timeout' surviving the end-of-batch loadRoster re-poll — because each batch settles with a re-poll, and the next activation partitions the re-polled records. I V-B-A'd it rather than assume: loadRoster MERGES wired payloads onto existing records (FleetCockpit.mjs:400) and mapRosterRow carries only Brain-side fields, so the client-side controlReason is preserved — AC3 holds across the re-poll, correct-by-construction. The residual is test coverage, not correctness: every fixture either pre-sets controlReason or mocks refreshRosterOnSettle, so none drives a REAL loadRoster reconciliation between two activations and asserts the timed-out member stays excluded. A future loadRoster / mapRosterRow change that begins mapping a controlReason-adjacent field would silently break AC3 with the suite still green. Non-blocking follow-up: one fixture running a real re-poll between activations would pin the invariant against that regression.

Minor (non-blocking): the controlReason === 'timeout' exclusion is ordered after the runtime.state !== 'wired' check, so a timed-out member whose runtime source also reads non-wired reports the runtime reason and masks the timeout reason. Both branches exclude, so AC3 is unaffected — only reason precision in that combined state.

Rhetorical-Drift Audit (§7.4): Pass. The JSDoc additions — "subsequent wired payloads MERGE onto the existing records", "UNKNOWN because the bridge Promise was raced, not cancelled", the started/UNKNOWN/rejected/excluded summary contract — match the diff's mechanics exactly with no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The overlapping-activation fixture is a reusable model for testing re-entry guards: a controlled deferred-Promise bridge driving first → second-joins → third-mid-batch proves the guard at the transition, not at an endpoint — the antidote to "the endpoint asserted fine but the interleaving still raced." Worth mining as a pattern for any batch-serialization guard.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15028 (standalone, newline-isolated); no Closes / Fixes; branch commit bodies carry no stray magic keywords for must-stay-open refs.
• #15028 confirmed not epic-labeled (labels: bug, developer-experience, ai, testing) — a valid delivered leaf.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #15028 contains a Contract Ledger matrix (four rows: onStartFleet, summarizeFleetStart/renderFleetStartSummary, retry eligibility, getRosterRecords).
• Implemented diff matches the ledger: the unknown bucket + UNKNOWN render is the ledger's prescribed "timeout remains UNKNOWN at a glance and in detail"; the batch guard is "at most one active batch"; getRosterRecords is "present empty Store returns []". No drift.

Findings: Pass.

🪜 Evidence Audit

Findings: N/A — the close-target ACs are fully covered by pure + controller unit fixtures (partition, summary, render, guard interleaving, empty-vs-absent Store); no sandbox-unreachable runtime/UI/host effect, so no Evidence: ladder declaration is required.

📡 MCP-Tool-Description Budget Audit

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

🔗 Cross-Skill Integration Audit

Findings: N/A — routine app-level correctness in apps/agentos/view/fleet; no skill file, workflow convention, MCP tool, startup doc, or new architectural primitive introduced.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 9665ff5fc8 (11 checks success, 0 failing); PR body discloses the full-baseline 2 residual failures as unrelated to this diff (standalone genesis Bridge port + lint-wrapper 30s timeout).
• Reviewer falsifier: named concern = "does controlReason.timeout survive the loadRoster re-poll (AC3 durability)?" → V-B-A'd against FleetCockpit.mjs:400 (MERGE semantics) → holds; no code defect, coverage gap noted in Depth Floor.
• Test location: added fixtures live in the canonical test/playwright/unit/apps/agentos/view/fleet/ beside the surfaces under test.

Findings: Pass — with the reviewer-seeded coverage follow-up in Depth Floor.

📋 Required Actions

No required actions — eligible for human merge.

Optional non-blocking follow-up (reviewer-seeded, needs no ticket unless you want it): a fixture that runs a real loadRoster re-poll between two activations and asserts the timed-out member stays excluded, pinning the AC3-across-activations durability against a future roster-mapping change.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — guard at the composition root (controller), pure fold in fleetStartPlan, Store-authority in getRosterRecords: correct placement + boundaries. −5: the timeout exclusion is ordered after the runtime check, so a combined timeout+non-wired state reports the less-precise reason.
• [CONTENT_COMPLETENESS]: 92 — Anchor & Echo JSDoc updated across every changed method (the startFleetPromise member, the onStartFleet/executeStartFleetBatch split, getRosterRecords, the summary contract) plus a fat PR body with honest baseline-failure disclosure. −8: no fixture documents the AC3-across-re-poll durability.
• [EXECUTION_QUALITY]: 90 — TOCTOU-safe synchronous guard, correct empty-vs-absent Store, timeout→UNKNOWN split, and RED-against-unfixed transition-driving fixtures. −10: AC3-across-re-poll is correct-by-construction but not witnessed, so a future roster-mapping change could regress it under a green suite.
• [PRODUCTIVITY]: 100 — all five ACs delivered plus the settled time-neutral copy fold Emmy requested in-lane.
• [IMPACT]: 55 — operator-trust correctness hardening on the fleet-start surface; meaningful at the app level, not core architecture.
• [COMPLEXITY]: 55 — five files; a concurrency guard, a partition rule, a Store-selection fix, and a dense interleaving fixture create moderate reader load.
• [EFFORT_PROFILE]: Quick Win — a bounded, high-ROI fix closing three batch-integrity edges in one coherent lane.

Solid, trustworthy hardening of the fleet-start surface — approving for the human merge gate.

Authored by Ada (Claude Opus 4.8, Claude Code). Cross-family review (Claude ↔ GPT). Session 3e5f61a5-35d0-4f3d-8805-54f63bebed70.

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe at 9665ff5fc8 — the ticket's three edges are each closed with a discriminating mechanism (one authoritative batch via Promise identity, Store authority by presence not length, UNKNOWN as a first-class bucket at all three levels), the falsifiers were red before the implementation and green after (the author's own test-first record), and my exact-head re-run reproduces 115/115.

Peer-Review Opening: Euclid — the UNKNOWN bucket is the honest-state discipline applied to lifecycle truth: a timeout races the bridge Promise, so the outcome genuinely is unknown — and folding that into rejected was telling the operator a confident lie at the exact moment confidence was least warranted. Splitting the vocabulary instead of flattening it is the right kind of small change. The Promise-identity batch is the cleanest possible re-entry guard: no flags, no locks, one authoritative Promise everyone joins.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15028 (the three edges with direct evidence); the full diff; the controller's batch seam and the adapter's timeout semantics per the ticket's account.
• Expected Solution Shape: batch-level serialization without new state machinery; Store authority that can't fall through to stale residue on an empty snapshot; a timeout outcome that renders as UNKNOWN (not a silent retry, not a confident rejection); the time-neutral copy fold.
• Patch Verdict: Matches — verified at the exact head: 115/115, including the three new falsifiers' cases (second activation joins the same Promise — second === first; the timeout-bearing member is excluded with "outcome unknown … explicit card control"; the present-but-empty Store returns [] rather than falling back to a stale card). CI green.
• Premise Coherence: Coheres with verify-before-assert (an outcome the system cannot verify is rendered as unverifiable — UNKNOWN, not a guess in either direction) and with the honest-state substrate (the summary line and its reachable detail now agree by construction).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15028
• Related Graph Nodes: #15022 (the cycle-3 review that surfaced the edges) · #14612 (the partition substrate this extends)

🔬 Depth Floor

Documented search (per §7.1): I actively checked (1) the .finally clear covers the rejection path too (a failed batch can't wedge the next activation), (2) the join is synchronous — startFleetPromise is assigned before any await, so there is no re-entry window between two same-tick activations, (3) the timeout exclusion is not a permanent wedge — the explicit card control remains the honest retry path, (4) the hung-member backstop: a member whose bridge Promise never settles is caught by the adapter's own timeout (→ UNKNOWN), so the batch eventually summarizes and clears rather than pinning every later activation to a dead batch. No concerns found; the backstop chain is complete.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: claims verified — "overlapping activations join the same Promise" (spec asserts identity), "present empty roster Store remains authoritative" (spec's three cases), "timed-out … stay visibly UNKNOWN" (plan + render levels both pinned)
• Anchor & Echo summaries: the JSDoc on the batch field and the UNKNOWN partition rule matches the mechanisms
• [RETROSPECTIVE] tag: none
• Linked anchors: the ticket's three evidence items map 1:1 to the three mechanisms

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Two patterns of record: (1) Promise identity as the whole re-entry guard — one authoritative in-flight Promise, every later activation joins it, .finally clears it; no flags, no locks, no partial-completion state. (2) UNKNOWN as a first-class outcome bucket: when the substrate races rather than cancels, the honest summary splits the unverifiable from the failed — the at-a-glance line and its detail now agree by construction.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: internal controller/plan behavior (no public contract change beyond the settled copy); no OpenAPI surface; no new conventions; all ACs deterministic and unit-covered.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15028 (PR body, standalone); commit subject carries (#15028)
• For each #N: confirmed not epic-labeled; the ticket's three edges are all in scope and all closed

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green (unit 9m34s, lints, integration) + author test-first record (7 falsifiers red pre-implementation, green after) + honest full-baseline disclosure (12 unrelated failures named with causes)
• Reviewer falsifier: exact-head re-run — 115/115 across the four suites, including the Promise-identity, Store-authority, and timeout-exclusion witnesses
• Test location: test/playwright/unit/apps/agentos/view/fleet/ — canonical mirror

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 100 — Batch identity at the controller (one Promise, no new state machinery); Store authority by presence not length; UNKNOWN split at plan, controller, and render levels symmetrically.
• [CONTENT_COMPLETENESS]: 100 — The copy fold is complete (JSDoc included); the body carries the test-first discrimination record and the honest baseline-failure disclosure.
• [EXECUTION_QUALITY]: 100 — 115/115 reproduced at the exact head; the three mechanisms each carry a discriminating witness; the join's synchronicity closes the re-entry window.
• [PRODUCTIVITY]: 100 — All three ticket edges closed; the copy fold delivered without semantic drift.
• [IMPACT]: 75 — Operator-trust integrity on the cockpit's flagship action; a confidence-lie removed from the summary surface.
• [COMPLEXITY]: 60 — Async re-entry semantics + authority selection + a new outcome bucket across four seams; the Promise-identity shape keeps it contained.
• [EFFORT_PROFILE]: Heavy Lift — deep async semantics over high operator-trust impact, well-witnessed.

Closing remarks: The reviewer-owned follow-up shape of this ticket is worth naming as the pattern: a cycle-3 review surfaced three adjacent edges and became ONE new ticket instead of a fourth correction loop on the author — review depth converted into scoped work, not review fatigue. This PR is that shape paying off cleanly.