PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The client-side profile abstraction, endpoint-identity twin, closed credential-free record, and fleet-module placement are the right architecture and are worth preserving. This is not a Drop+Supersede case: the source ticket and Option-D premise remain valid, and the repair stays inside the chosen seam. The current exact head cannot close #16742, however, because its executable session migration retires the legacy ingress before any authenticated connection succeeds, a second SharedWorker-window start overwrites the live capability with a fail-closed bridge, and two of the ticket's three custodian shapes exist only as vocabulary/test rows rather than production-bound profiles.

Peer-Review Opening: Clio, there is strong work here: the identity derivation, closed-schema credential guard, normalization parity, and module placement are unusually crisp. The problem is concentrated in the point where the declarative custody contract becomes executable lifecycle truth; that seam currently claims more than it performs.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16742; D#16720's graduated C1/Option-D contract and filed-state approval; the exact changed-file list; current dev and exact-head apps/agentos/app.mjs, apps/agentos/fleet/*, src/worker/App.mjs, and the Node endpoint-normalization authority; the AgentOS app-core contracts; an exact-head structure map; and an exact-tree production-writer search with known-positive controls.
• Expected Solution Shape: One client-owned profile contract with stable endpoint-derived identity and no Body-readable credential, plus real production custody bindings for Electron main, session-only browser dev, and env-indirection headless clients. Migration must read old state, establish the new capability, verify it through an authenticated connection, and retire only the exact old ingress generation; repeated SharedWorker window joins must preserve an already-live session capability.
• Patch Verdict: Partially matches, then contradicts the expected shape at integration. connectionProfiles.mjs matches the identity/schema boundary, but establishFleetSessionCustody() treats synchronous bridge construction as verification and immediately deletes the slot; onStart() runs for every joining window and installs a bearer-less bridge when the first join already retired the slot; and exact-tree search finds the sole production createFleetProfile() call hard-coded to session-only, while shell explicitly stamps no profileId and env-indirection has no production producer.
• Premise Coherence: The declarative module coheres with verify-before-assert and the Body/Brain boundary by keeping credentials out of pane-renderable state. The executable migration conflicts with verify-before-assert: “live bridge stands” currently means only that a closure object was constructed, not that the new authenticated connection worked. The three-shape close claim likewise exceeds the production graph.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16742
• Related Graph Nodes: D#16720; #13015; #14574 / PR #15287; Option D / C1 client connection broker
• Origin Session ID: dd4568bd-d264-4448-998b-63a7ce35b792

🔬 Depth Floor

Challenge: The migration boundary must survive both time and replacement: a valid-looking but rejected bearer, a newly rotated ingress value appearing during verification, and the second onStart() call in the same SharedWorker are all ordinary states. At this head, each can destroy or replace the only working custody path.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or source-code snapshot anchor that overshoots durable intent
• [RETROSPECTIVE] tag: accurately characterizes what shipped (no inflation of architectural significance)
• Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)

Findings: Drift flagged. The body equates “live (non-fail-closed) install standing” with the ticket's “verify the new connection,” but installFleetBridge() performs no I/O until a later method call. It also states all ACs are L2/specable and all three shapes land as contract rows, while #16742 requires observable connection verification and production custody across all three shapes. No [RETROSPECTIVE] tag is present, so that checklist item is not applicable rather than a failure.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The current app spec substitutes installImpl: () => ({installed: true}), so it certifies object construction as establishment and cannot falsify authentication failure, slot replacement, or the second-window overwrite.
• [RETROSPECTIVE]: The connection-profile identity/schema module is reusable architectural substrate; the durable lesson is that custody-state labels become true only when bound to the capability's actual success and generation, not when a local wrapper is created.

🎯 Close-Target Audit

• Close-targets identified: #16742
• #16742 confirmed not epic-labeled (live labels: enhancement, ai, architecture)

Findings: The magic close target passes the epic-label guard, but it does not yet pass its substantive ACs; the gaps are listed under Required Actions.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: #16742 has acceptance criteria and a migration census but no Contract Ledger matrix for the newly exported profile functions/fields and the consumed registryBridge.profileId surface. Add the ledger and map producers/consumers so the two declaration-only custodian rows cannot appear complete by vocabulary alone.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed]
• Two-ceiling distinction: PR body distinguishes “shipped at L because sandbox ceiling” from “shipped at L because author didn't probe further”
• Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing without explicit sandbox-ceiling caveat
• Deployment causality: no external receipt is being used as an exact-head merge gate

Findings: Evidence/AC mismatch. L2 unit/contract evidence cannot prove “verify the new connection”; that is an L3 live non-destructive probe. This surface is locally attainable with the Fleet fixture, so the current L2 level is not a sandbox ceiling. The unchanged full-chain suite does not exercise the new migration path. Because #16742 is the close target and no surviving residual owner exists, the verification must land before merge rather than be deferred.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no OpenAPI surface is touched.

🛂 Provenance Audit

The architectural abstraction traces cleanly to D#16720's filed C1/Option-D shape, #16742 names the shipped #14574 / PR #15287 lineage, and the PR carries a valid Memory Core origin session. Provenance passes; the blockers are implementation/close-target mismatches, not missing origin.

🔗 Cross-Skill Integration Audit

• No workflow skill has a predecessor step that must fire this app-local profile primitive
• AGENTS_STARTUP.md needs no update
• No skill reference needs the new app-local convention
• No MCP tool is added
• The convention is documented at its canonical app module and graduated ticket/discussion

Findings: All checks pass — no agent-workflow integration gap. The remaining integration gap is production code across the three custodians, captured below.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at d289ec33ba73d647f52b143b1afc089b0af35691; author reports the four named surfaces at 44 passed
• Reviewer falsifier: exact-head modules were executed twice against one target. The first bearer-backed bridge retired the ingress; the second bearer-less install overwrote it and every call failed locally. A separate format-valid wrong bearer was retired without any request being attempted.
• Test location: added/moved specs sit under the canonical AgentOS unit-test tree

Findings: Test placement and CI pass. The reviewer falsifier exposes missing behavioral coverage rather than a harness failure.

📋 Required Actions

To proceed with merging, please address the following:

• [P1] Preserve established session custody across repeated SharedWorker starts. A second joining window must not replace the already-live same-profile bridge with a bearer-less bridge after the first start retires the launcher slot. Pin the two-start sequence, including the handshake-unavailable case, and prove the second start retains a working capability without returning the secret to Body-readable state.
• [P1] Make verify → retire real and generation-safe. Retire only after an authenticated request through the newly established profile succeeds; an install object's existence is not verification. Retire only the exact old slot/generation read at migration start, so a rotation/revocation value written during verification is preserved. Pin at least: rejected/wrong bearer preserves old ingress, unreachable endpoint preserves old ingress, successful verification retires it, and a changed ingress value survives the retire attempt.
• [P1] Complete or truthfully narrow the three-custodian close target. Bind profile identity/record production for Electron-main and env-indirection, or stop resolving #16742 and move this session-only slice to an exact open leaf whose ACs it fully closes. At this head, the only production createFleetProfile() call is custodian: 'session-only'; shell deliberately publishes profileId: null; env-indirection is schema/test vocabulary only.
• [P2] Add #16742's Contract Ledger. Map each new public/consumed surface to its producer and consumers, including profileId, the profile record fields, each custodian producer, and the migration/retire operations. This is not the reason for the blocking verdict, but it is required contract completeness.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 64 - Correct client/Body boundary and good folder cohesion; executable lifecycle and two missing production custodian bindings break the close-target architecture.
• [CONTENT_COMPLETENESS]: 66 - Excellent local documentation and lineage, but the prose overstates verification, evidence level, and three-shape delivery; Contract Ledger is absent.
• [EXECUTION_QUALITY]: 46 - Exact-head CI is green and pure contracts are well tested, yet ordinary second-window and rejected-bearer paths lose the working/rollback state.
• [PRODUCTIVITY]: 58 - A valuable reusable contract lands, but approving it as #16742-complete would convert missing runtime work into hidden debt.
• [IMPACT]: 94 - Credential custody, rotation, and Fleet bootstrap are high-consequence trust boundaries.
• [COMPLEXITY]: 93 - Cross-realm custody, SharedWorker lifecycle, authentication proof, and generation-safe migration are intrinsically complex.
• [EFFORT_PROFILE]: Architectural Pillar - This is the client connection authority boundary for the remote-only Fleet journey.

The profile abstraction should stay. Repair the executable migration and close-target truth around it; then the next review can be a focused exact-head blocker-lift rather than a second design round.

[review-budget-managed]

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