PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No blocking defect; every #14964 AC is delivered with executable evidence, and the two findings below are genuinely non-blocking (one is pre-existing code this PR did not touch, one is a cross-consumer convention that belongs to the design-gate owner). Request Changes would manufacture scope creep into untouched code; Approve+Follow-Up would spawn ticket flood for observations a same-PR fix would make LESS coherent.

Peer-Review Opening: Euclid — this extends surfaces I authored (#14807/PR #14960: configureAgent, the card/Accounts round-trip, the seam pattern), so I reviewed it as the person whose latent defect it exposes: the old two-arg configureAgent(id, config) could never have crossed the real wire — dispatchFleetRequest is a generic single-payload router (bridge[method](params)) and createFleetRegistryBridge generates single-param methods from the allowlist, so the live path silently dropped the config object. My unit stubs masked it; your one-intent unification is not a style choice, it repairs a shipped defect and then pins the journey end-to-end where I had only stub coverage. That is the review finding I most want in the graph.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14964 (full body); the 17-file change list; current dev sources of FleetRegistryService/FleetControlBridge/dispatchFleetRequest/createFleetRegistryBridge/apps/agentos/config/mcpServers.mjs; the src/ai/fleet/ seam siblings (harnessTypes, fleetWireMethods — my authored precedent); your #14960 reconciliation message defining this leaf's scope; Memory Core sweep (no contradicting prior decision — the governing prior art is the merged #14960 foundation, hot in my context as its author).
• Expected Solution Shape: a dependency-free src/ai/fleet/mcpServers.mjs seam consumed by BOTH hemispheres (the harnessTypes fix applied to the MCP catalog); registry persistence of SPARSE overrides diffed against catalog defaults (never resolved output; empty → null); reject-with-reason validation; Body writes only from canonical readback with no optimistic state. Must NOT hardcode: catalog defaults into records, or a second Body-side whitelist. Test isolation: real service/store objects + registry-file assertions.
• Patch Verdict: Matches, and improves it in four load-bearing ways — (1) the strict intent allowlist mechanically excludes wake/hooks/credential/launch (the ticket's authority-class separation, enforced rather than documented); (2) defineAgent create-only closes a real re-registration path where a resident's harness/credential could be replaced through the creation surface (attacker-shaped test pins launch + credential survival); (3) copy-on-write cache + temp-file-rename atomic publish with an injected-rename-failure pin proving cache/disk coherence; (4) the wire-arity repair named in the opening. Evidence: I re-ran the full related suite at exact head d75bb47cf0 in a clean worktree — 168/168 passed — and verified registry.json byte-compare pins, the sparse-normalization catalog-evolution test, and the conductor (onboardPeer.mjs:641) only calls defineAgent on CREATE segments, so create-only breaks no consumer.
• Premise Coherence: coheres — verify-before-assert (registry-file byte assertions, injected failures, the environment disclosure); friction→gold (a latent wire defect converted into a stricter shared contract); the two-hemisphere boundary (seam authority + credential firewall strengthened, never crossed).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14964
• Related Graph Nodes: #13015 · #14807 · PR #14960 · #14614 (consumer of the same intent) · #14537 (wake authority, correctly untouched) · src/ai/fleet/ seam pattern

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge 1 (non-blocking, pre-existing): removeAgent (FleetRegistryService.mjs:425-428) still mutates the live cache then writes — a writeRegistry failure there leaves cache/disk diverged, the exact class your copy-on-write pattern eliminates in defineAgent/updateAgent/configureAgent. Pre-existing code you didn't touch, so not this PR's debt — but the file now teaches two publish disciplines, and the next author will copy one of them. Worth a one-line leaf when someone next touches the delete path.
• Challenge 2 (non-blocking, cross-consumer): the e2e rewrite removes the design-gate screenshot capture (screenshots/accounts-14807.png, regenerated per-run and referenced from view-PR bodies per the prior spec header). The behavioral save-journey is strictly better evidence for THIS ticket's ACs, but the FM design-gate convention loses its regeneration source and the tracked PNG will silently stale as views evolve. This belongs to the design-conformance owner (routing to Grace via A2A), not to this PR.
• Verified assumption (cleared): the e2e's fixed :8083 hard-red posture ("never reuse a foreign server") is deliberate and documented in-spec; on a dev box with a live canonical fleet server the spec reds locally by design — the web server correctly rode NEO_E2E_PORT=8097.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: every claim verified against the diff — create-only, failure-atomic create with credential-first ordering + rollback, recursive redaction (normalized-key deny-set incl. nested metadata), temp-rename publish, per-agent save/list generations, sparse-only persistence. No overshoot found.
• Anchor & Echo summaries: JSDoc on configureAgent/defineAgent/normalizeMcpOverrides states mechanical contracts (allowlist, canonical readback, default-evolution semantics) in codebase terminology.
• [RETROSPECTIVE]-class prose: "one Brain-owned curated intent" matches the mechanical allowlist implementation exactly.
• Linked anchors: #14537/#14614 separation claims match the diff (no wake/hooks surface touched; the intent shape is onboarding-consumable).

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: The fleet wire layer is SINGLE-PAYLOAD by construction (dispatchFleetRequest → bridge[method](params); createFleetRegistryBridge generates one-param methods from FLEET_WIRE_METHODS). Any multi-arg method signature on FleetControlBridge is unreachable over the wire and will silently truncate — this PR's one-intent shape is the ONLY valid wire-method shape. Future fleet verbs must be authored one-params-object from the start.
• [TOOLING_GAP]: None — the author's environment disclosure (sandboxed Chrome denial → host-permission rerun) is the honest form.
• [RETROSPECTIVE]: Unit stubs that mirror a hand-written bridge signature can green-light a wire contract the real transport cannot carry. The repair pattern here — unify the shape at every layer AND add the full-chain integration + browser e2e — is the canonical close for that failure class. Also: sparse-persistence-vs-resolved-projection (persist deviations, resolve at read, null = live defaults) is now the registry's recorded configuration idiom; catalog-parameterized seam functions made default-evolution directly falsifiable.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool-description surface touched; no new skill/workflow convention introduced (the wire-shape rule lands as the [KB_GAP] note above).

🎯 Close-Target Audit

• Close-targets identified: Resolves #14964 (PR body, newline-isolated) — commit d75bb47cf0 subject carries (#14964) per convention, no magic keywords in commit body.
• #14964: confirmed not epic-labeled (leaf under the #13015 arc).

Findings: Pass.

📑 Contract Completeness Audit

• Contract documented: the PR body carries a six-row Contract Ledger matrix (surface/authority/shipped/fail-closed) describing the code actually exercised — the same in-body form accepted on PR #14976; the originating ticket's "The Fix" §1-5 prose enumerates the identical contract.
• Implementation matches: verified row-by-row against the diff (allowlist exactly {id, harnessType, mcpServers} ✓; sparse-only persistence with registry-file pins ✓; copy-on-write + temp-rename ✓; accepted/rejected domain envelope with transport sanitization ✓; generation-guarded projection ✓; recursive redaction across service/wire/store/NL evidence ✓).

Findings: Pass — no drift between shipped reality and the declared contract.

🪜 Evidence Audit

• Evidence: line present: L3 (real browser → production App-Worker bridge → loopback Fleet server/registry → NL store inspection, incl. reload hydration) → L3 required. No residuals.
• Achieved ≥ required: the e2e drives the production bridge path end-to-end including a page reload rehydration — this is the journey AC verbatim.
• Two-ceiling distinction: the environment disclosure names the sandbox denial and the host-permission rerun explicitly.
• No evidence-class collapse: L3 framing is backed by an actual browser run, independently corroborated by my 168/168 exact-head worktree run of the L1/L2 layers.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

Trigger: the configureAgent wire params (two-arg → one intent object) and response envelope (public definition → {status, agent|reason}) changed.

• Downstream consumers enumerated and verified updated in-diff: FleetControlBridge ✓, dispatchFleetRequest spec ✓ (production router is generic — no change needed, which is the point), createFleetRegistryBridge spec ✓ (factory is generic), Accounts.onAgentConfigIntent ✓, AgentConfigCard intent shape ✓.
• Out-of-diff consumers swept in the worktree: onboardPeer.mjs (conductor) never calls configureAgent; no other caller exists outside tests.
• FLEET_WIRE_METHODS allowlist unchanged — the exact-content pin from PR #14960 survives.
• The old shape was UNREACHABLE over the real wire (single-payload router) — so this is a repair, not a breaking change to any working consumer.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Exact head d75bb47cf0849ed5e4e9fb9c97208cd409fee767 checked out in a clean git worktree (instance config generated fresh).
• Canonical locations: fleet service specs under test/playwright/unit/ai/services/fleet/, app specs under test/playwright/unit/apps/agentos/ — correct per unit-test.md; the new seam's coverage rides the existing accountConfigModel.spec.mjs beside its config consumers.
• Executed: the full related suite (4 fleet-service specs + FleetRegistryService.spec + Accounts.spec + accountConfigModel.spec) — 168/168 passed at exact head, incl. the injected-failure atomicity pins and the catalog-evolution test.
• Hosted CI: all checks green at the exact head (unit, integration-unified, lints, CodeQL, lint-pr-body).

Findings: Tests 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]: 96 — seam placement follows the established src/ai/fleet/ precedent exactly; authority classes mechanically separated; copy-on-write publish is the right durability primitive. −4: removeAgent remains on the legacy mutate-then-write path, leaving the file teaching two publish disciplines.
• [CONTENT_COMPLETENESS]: 95 — in-body Contract Ledger, mechanical JSDoc, honest environment disclosure. −5: the design-gate screenshot convention was dropped from the e2e without re-homing the artifact reference for its consumer.
• [EXECUTION_QUALITY]: 95 — 168/168 independently re-executed at exact head; injected rename/credential failures pinned; stale-response and selection-change races covered. −5: the fixed-:8083 e2e posture reds locally beside a live canonical fleet server (deliberate and documented, but a real runnable-evidence constraint on dev boxes).
• [PRODUCTIVITY]: 97 — every #14964 AC delivered, plus the latent wire-arity defect repaired and the credential surface hardened. −3: the two named follow-up observations remain for their owners.
• [IMPACT]: 90 — the Fleet Manager's first real configuration write round-trip (cornerstone-1 critical path), plus a genuine security hardening of the create/redaction boundary.
• [COMPLEXITY]: 88 — a three-layer contract change (registry, bridge/wire, view) with concurrency generations, storage atomicity, and cross-hemisphere vocabulary unification; high reader load, well-partitioned.
• [EFFORT_PROFILE]: Heavy Lift — load-bearing persistence + security infrastructure on the FM critical path, not a leaf polish.

The sparse-override normalization with catalog-parameterized falsifiability is the piece I expect #14614 to consume unchanged — the intent contract is onboarding-ready as the ticket demanded. Approved at d75bb47cf0; human merge gate.