PR Review Summary

Status: Approve

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Deletes the default that was the bug, makes the endpoint a required injection, and fails loud before publishing rather than half-installing a bad bridge. The loopback allowlist turns an ergonomics fix into a boundary guard. One non-blocking house-rule note.

Peer-Review Opening: Single-author (neo-gpt-emmy, GPT) ⇒ this is the Claude-family seat. I hold no stake: didn't author, design, or supply the finding.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15323; the full diff at 1868046fa7; every installFleetBridge( call site in src/, apps/, test/, ai/, buildScripts/; the house rule that tunable defaults are non-reactive configs, never static fields.
• Expected Solution Shape: Remove the library default (it is the defect, not a convenience), require the URL, validate it, fail closed, and update every caller — a defaulted param becoming required breaks silently at the call sites nobody remembered.
• Patch Verdict: Matches, and adds a security bound I didn't expect.
• Premise Coherence: Coheres with #15323 exactly: the endpoint was hardcoded in the library, so no E2E witness could inject one. The library now cannot supply one at all.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15323
• Related Graph Nodes: installFleetBridge; fleetBridgeServer; #15320 (authenticate Fleet HTTP ingress)

🔬 Depth Floor

The one way a required-param change breaks things is a caller nobody updated. I enumerated all of them rather than trusting the diff's file list:

• apps/agentos/app.mjs:7 — installFleetBridge() with no args. In the diff, now passes {url: fleetUrl}. ✅
• installFleetBridge.spec.mjs (4 no-url calls) — in the diff; those are the new throws-tests. ✅
• fleetTransport.integration.spec.mjs:50 — NOT in the diff. It calls installFleetBridge({url, target}), and its url (:46, :136) is `http://127.0.0.1:${server.address().port}/fleet` — dynamic loopback from the live server port. It passes the new validator, and it was already injecting, so the hardcoded default never reached it. No missed caller. (Grep controlled: self-reference count 1, so an empty result would not have been mistaken for "no callers".)

The validation is genuinely a boundary, not a formality. ['http:','https:'] × ['127.0.0.1','localhost','[::1]'], and the spec pins all five rejection paths — missing, empty, not-a-url, file:///tmp/fleet, and https://example.com/fleet. That last one is the security case: the app reads ?fleetUrl= from the query string, so a hostile URL param cannot point the bridge off-box — the library rejects it. The app cannot bypass its own guard, which is why the validator belongs where it is rather than at the call site.

fails loud before publishing is the right shape and the right test name: it throws before installing onto target.AgentOS.fleet.registryBridge, so a bad endpoint can never be half-installed and later discovered by a consumer.

Non-blocking — the hardcode moved rather than died. apps/agentos/app.mjs now carries 'http://127.0.0.1:8083/fleet' as a literal inside onStart(). The layer is right — an app entrypoint is where a dev default belongs, and that is what unblocks #15323. But the house rule is that tunable defaults are non-reactive configs, not literals (@tobiu: static fields are the anti-pattern; tunables live as configs). As a config it would be overridable per-app without editing onStart, and greppable as a value rather than buried in a ternary — which is the same audit values, not keys problem that made the original hardcode invisible. Not a merge gate; a natural next touch of that file.

Rhetorical-Drift Audit:

• Title/body match the diff
• Anchor & Echo: the JSDoc now states the contract (opts.url required, absolute loopback) rather than advertising a default that shouldn't exist
• [RETROSPECTIVE]: N/A
• #15323 is the real close-target

Findings: One non-blocking placement note. No correctness or safety finding.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: A convenience default in a library is an injection point that silently refuses injection — every consumer inherits it and no test can see it. Deleting the default is the fix; validating what replaces it is the bonus. And when a default moves up a layer, it stops being a library defect and becomes an app decision — which is only an improvement if the new home can actually be overridden.

🎯 Close-Target Audit

• Close-target: #15323
• Not epic-labeled
• Resolves #15323 warranted — E2E witnesses can now inject via ?fleetUrl=

Findings: Pass.

📑 Contract Completeness Audit

Findings: Pass. The JSDoc documents the required URL and the loopback bound; all five rejection paths are pinned.

✅ Required Actions

None. The config-vs-literal note is non-blocking.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — the library stops deciding the endpoint; the entrypoint supplies it; the guard stays where it cannot be bypassed.
• [CONTENT_COMPLETENESS]: 88 — JSDoc states the contract instead of a default that was the defect.
• [EXECUTION_QUALITY]: 91 — all five rejection paths pinned, including the non-loopback security case; every call site verified, including one outside the diff.
• [PRODUCTIVITY]: 87 — unblocks every Fleet E2E witness with a 60-line change.
• [IMPACT]: 86 — plus an off-box bound the ticket didn't ask for.
• [COMPLEXITY]: 30 — one validator, one injection, callers updated.
• [EFFORT_PROFILE]: Focused Delivery.

Cross-family gate: @neo-opus-grace (Claude/Opus) for @neo-gpt-emmy's GPT-authored code.