PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR is the right first Fleet Manager leaf and the broader shape matches #13031/#13015, but the credential resolver has a small fail-closed bug on a security-boundary method. This should be fixed in this PR because the service contract and PR body both make resolveCredential() the only raw-PAT path.

Peer-review opening: I reviewed #13037 at exact head 624b8f62eb1d066d8cd8bf40cb1905d6edcd28d9.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13031 close target, #13015 parent epic, PR changed-file list, current sibling ai/services/ConceptService.mjs pattern, unit-test workflow, KB lookup for ai/services singleton/dataDir patterns, live CI, and exact branch checkout.
• Expected Solution Shape: A correct first Fleet Manager registry leaf should add a Brain-side singleton service under ai/services/fleet/, keep PATs out of public read APIs, store credentials encrypted at rest, isolate storage by dataDir, and include focused unit coverage for CRUD, security boundary, durable reload, validation, and fail-closed resolver behavior. It must not hardcode Body-side/browser credential reads or depend on future lifecycle/spawner leaves.
• Patch Verdict: Mostly matches. Placement, singleton shape, encrypted store, public read boundary, parent/sub-issue relationship, and focused tests line up with the expected shape. The exception is the resolver fail-closed branch: prototype-inherited keys are not handled safely.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13031
• Related Graph Nodes: #13015, #13012, Discussion #10119, ai/services/fleet/FleetRegistryService.mjs

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: resolveCredential() promises String|null and the PR claims fail-closed behavior, but it currently indexes a normal object directly. I reproduced that resolveCredential('toString') and resolveCredential('constructor') return inherited functions from Object.prototype instead of null on an empty credential store.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: security-boundary framing is accurate except for the fail-closed resolver edge case flagged below.
• Anchor & Echo summaries: service JSDoc accurately states the intended two-hemisphere boundary; the implementation needs the lookup fix to satisfy it fully.
• [RETROSPECTIVE] tag: N/A; none present.
• Linked anchors: #13031/#13015 establish the registry + credential boundary and native sub-issue link is present.

Findings: Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None. Sandbox networking required escalated read-only GitHub checks, but the PR tooling and local test path worked.
• [RETROSPECTIVE]: Security-boundary services that decrypt secrets should treat dictionary keys as untrusted and use own-property/null-prototype lookup even when the persisted data is locally generated.

🎯 Close-Target Audit

• Close-targets identified: #13031 via PR body Resolves #13031 and commit subject (#13031).
• #13031 labels are enhancement, ai, architecture; confirmed not epic-labeled.
• #13031 has parent #13015 via native issue relationship; #13015/#13012 are Related, not close-targets.

Findings: Pass.

📑 Contract Completeness Audit

• #13031 contains a Contract Ledger for FleetRegistryService with define/list/get/remove, Brain-internal credential resolve, encrypted credential store, and fail-closed fallback.
• Implemented PR diff does not yet match the fail-closed resolver portion for inherited object keys.

Findings: Contract drift flagged in Required Actions.

🪜 Evidence Audit

• PR body declares Evidence: L1 (unit-tested service contract; node-side fs/crypto, no runtime surface beyond CI reach).
• Achieved evidence is appropriate for this code-only service leaf; later spawner consumption is correctly listed as post-merge/future leaf validation.
• Evidence-class collapse check: review language does not promote local unit evidence to end-to-end Fleet Manager behavior.

Findings: Pass, except the unit set needs the missing resolver edge case from Required Actions.

🛂 Provenance Audit

The architectural source chain is coherent: #13012 establishes the Agent Harness / two-hemisphere boundary, #13015 scopes the Fleet Manager MVP, and #13031 scopes this registry/credential leaf. The implementation does not import external framework architecture; it uses Node fs/crypto and existing Neo.core.Base singleton service shape.

🔗 Cross-Skill Integration Audit

• New ai/services/fleet/ domain follows the existing ai/services/<domain>/ Brain-side pattern.
• No MCP tool surface or Body-side settings pane is introduced, so no MCP/OpenAPI or app-surface docs are required in this leaf.
• Not registering in ai/services.mjs is consistent with the PR's stated out-of-scope consumption leaf.
• Native sub-issue relationship to #13015 is present.

Findings: All checks pass — no integration gaps beyond the resolver fix.

🧪 Test-Execution & Location Audit

• Branch checked out locally at 624b8f62eb1d066d8cd8bf40cb1905d6edcd28d9.
• Canonical Location: test/playwright/unit/ai/FleetRegistryService.spec.mjs is correct for a right-hemisphere AI service test.
• Ran npm run test-unit -- test/playwright/unit/ai/FleetRegistryService.spec.mjs: 8/8 passed.
• Ran git diff --check origin/dev...HEAD: clean.
• Ran gh pr checks 13037: Analyze, Classify, CodeQL, Retired Primitives Check, integration-unified, lint-pr-body, and unit all pass.
• One-off probe exposed missing coverage: resolveCredential('toString') and resolveCredential('constructor') return functions instead of null on an empty store.

Findings: Focused tests pass but coverage misses the failing resolver edge case.

N/A Audits — 📡 🔌 🧠

N/A across listed dimensions: no OpenAPI/MCP description changes, no wire-format changes, and no turn-loaded memory substrate files were modified.

📋 Required Actions

To proceed with merging, please address the following:

• Fix FleetRegistryService.resolveCredential() / credential map handling so unknown ids always return null for inherited/prototype names such as toString and constructor, then add a regression test. The local falsifier at this head is:

resolveCredential('nobody')      -> null
resolveCredential('toString')    -> function toString() { [native code] }
resolveCredential('constructor') -> function Function() { [native code] }

A narrow fix is enough: either guard the lookup with Object.hasOwn(credentials, id) ? credentials[id] : null, normalize decrypted credential maps to a null-prototype object, or both.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - 5 points deducted because the Brain-side service shape and dataDir seam are correct, but the secret resolver currently violates fail-closed lookup semantics for inherited object keys.
• [CONTENT_COMPLETENESS]: 95 - 5 points deducted because the service JSDoc and PR body accurately describe the intended boundary, but the fail-closed claim needs the implementation/test correction above.
• [EXECUTION_QUALITY]: 80 - 20 points deducted because CI and focused tests are green, but a concrete security-boundary edge case returns prototype functions instead of null.
• [PRODUCTIVITY]: 90 - 10 points deducted because the registry leaf is otherwise delivered, but merge should wait for the resolver fail-closed gap.
• [IMPACT]: 80 - 20 points deducted because this is a foundational Fleet Manager leaf, not the whole lifecycle/spawner surface.
• [COMPLEXITY]: 55 - Medium: small file count, but non-trivial persistence, encryption, singleton state, and security-boundary semantics.
• [EFFORT_PROFILE]: Heavy Lift - High-value first Fleet Manager substrate with moderate implementation complexity and security-sensitive behavior.

Requesting changes on the resolver only; the rest of the PR shape looks sound.