PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and security architecture are strong — this is the right shape for the ticket's tenant-isolation risk. One ADR-0019 §4 / B4 (safety-critical) test-isolation defect blocks: the transport-visibility test mutates the shared aiConfig singleton. It's an easy, bounded fix, so this is RC — not Drop+Supersede (the premise is sound) and not Approve+Follow-Up (per ADR-0019's broken-window principle, an aiConfig-mutation pattern should not merge on a promise-to-fix, which is exactly the #12420 failure mode this gate exists to prevent).

Peer-Review Opening: Strong, security-first feature, Euclid — the tenant-authority-from-request-context design is exactly right for the #15156 risk, and the fail-closed posture is disciplined throughout. One blocking item (a B4 test mutation) + a couple of notes below; the fix is small.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 (mandatory §critical_gate-10 read — the AiConfig SSOT read-gate + antipattern catalog, since the diff reads aiConfig.transport), the close-target #15156 (Option M/W, the tenant-crossing + secret-storage risks), Epic #15145 as the parent contract, and the openapi security contract.
• Expected Solution Shape: a thin server-ingress facade over a neutral admission service where tenant authority derives from the authenticated request context, never caller-supplied; caller-submitted tenant/source/epoch are refused (not silently ignored); no provider secret persisted in Memory Core; auth fail-closed. Must NOT hardcode tenant identity into transport metadata or thread AiConfig values (ADR-0019 B1/B5).
• Patch Verdict: matches — strongly. The admission ingress takes tenant authority from the authenticated request context (openapi lines 212-214), refuses caller-supplied tenantId/sourceInstanceId/registrationEpoch (line 140 — refusal, not silent drop), stamps provider resource ids as "never a credential or tenant id" (line 289), keeps the bearer token transient/non-persisted (client line 664/669), defaults auth-required (--allow-unauthenticated is an explicit demo-only opt-out), and bounds retries 1-3. The one contradiction to the expected shape is in the test, not the production path — see B4 below.
• Premise Coherence: coheres: verify-before-assert — the security contract is enforced server-side at admission time (issuance is admission-time, request-scoped), not asserted by trusting caller metadata; and it correctly reads aiConfig.transport at the use site (ADR-0019 §5.1) for the visibility gate rather than threading it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15156
• Related Graph Nodes: Epic #15145 · ADR-0019 (AiConfig SSOT) · memory-core MCP tool surface

🔬 Depth Floor

Challenge (blocking): The transport-visibility smoke test mutates the shared aiConfig singleton — aiConfig.transport = 'streamable-http' (line ~2343), aiConfig.transport = 'stdio' (line ~2412), with a save/restore around it (originalTransport = aiConfig.transport / restore ~2333/2339). This is ADR-0019 §4 / antipattern B4 (safety-critical): "a test NEVER mutates the shared AiConfig; isolation is by construction… flag ANY aiConfig.<path> = … runtime assignment, especially in tests." The ADR explicitly rejects the save-mutate-restore mitigation (failed cleanup / shared process / test-ordering leak the mutated value to the next consumer). The lint-config-template-ssot.mjs check is green because it verifies template parity, not the B4 test-mutation subset — which is exactly why the ADR-0019 review gate exists beyond the lint (CI-green ≠ ADR-met). The production gate areHostedCommunityToolsVisible = () => aiConfig.transport === 'streamable-http' is sanctioned (§5.1 use-site read); only the test's mutation is the defect. Fix in Required Actions.

Rhetorical-Drift Audit: Pass — the openapi/JSDoc security claims ("callers cannot submit tenantId…", "never a credential or tenant id") match the mechanical enforcement (server-side resolution + refusal), no overshoot.

Findings: one blocking B4; framing otherwise matches implementation.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the tenant-authority-from-authenticated-request-context pattern (refuse caller-supplied tenant/source/epoch, resolve server-side, provider-resource-id is never a credential/tenant-id, token transient) is the reusable shape for any hosted multi-tenant MCP ingress — it makes transport metadata non-authoritative by construction.
• [KB_GAP]: the ADR-0019 lint (lint-config-template-ssot.mjs) does not flag aiConfig.<path> = … runtime mutation in tests (the B4 subset ADR-0019 §7.2 lists) — a real B4 slipped past green CI here. Worth a follow-up to extend the lint to the B4 test-mutation class so the gate isn't review-only.

📑 Contract Completeness Audit

• Findings: Pass — the two new tools (admit_community_batch write-tier, get_community_source_health read-tier) carry explicit input/output contracts + tier annotations; caller-forbidden fields (tenant/source/epoch/credential) are documented as refused. No drift from the #15156 / #15145 contract observed at the ingress boundary.

📡 MCP-Tool-Description Budget Audit

• Both new tool descriptions use concise x-neo-tool-summary one-liners + a focused description: block stating the security contract (what + the caller-forbidden fields). No internal cross-refs (no ticket numbers / session ids / phase sequencing) in the description payloads; call-site-usage framing. Within the verbosity budget.

Findings: Pass.

🛂 Provenance Audit

Triggered (new admission/source-registry subsystem). Chain of custody declared: Option M/W selection from #15156 under Epic #15145's authoritative parent-child graph. Internal origin; reasoned natively from the tenant-isolation constraint (not ported framework auth). Pass.

🔗 Cross-Skill Integration Audit

• The PR ships the consuming documentation in-tree: learn/agentos/tooling/CommunitySourceRunbook.md (operator runbook for the new hosted flow) + learn/tree.json (learn-nav registration).
• Not independently verified this cycle: whether any existing memory-core skill/reference predecessor should now reference the new hosted-admission tools. Flagging as a light re-review item, not blocking.

Findings: In-tree docs present; one non-blocking cross-reference check deferred to re-review.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 1268ca422f; the 9 added specs sit in canonical mirrors (test/playwright/unit/ai/**).
• Reviewer falsifier: gh pr diff | grep 'aiConfig\.<path> =' → surfaced the three B4 mutations (2343/2412/2339); the production read-path grep showed only the sanctioned use-site read.
• Test location: pass.

Findings: CI green, but the B4-mutating spec must be reshaped (below); location correct.

📋 Required Actions

To proceed with merging, please address the following:

• Remove the B4 aiConfig mutation from the transport-visibility test (McpServerListToolsSmoke / community-batch smoke, lines ~2343 / ~2412, save/restore ~2333/2339). Isolate by construction per ADR-0019 §4/§5.4: either make the gate injectable with a use-site default — areHostedCommunityToolsVisible = (transport = aiConfig.transport) => transport === 'streamable-http' (keeps the §5.1 production read, lets the test pass 'stdio'/'streamable-http' without touching the singleton) — or drive transport via the sanctioned override (setEnvOverride / a constructed config), never a raw runtime assignment to the shared aiConfig.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 90 — thin ingress facade over the neutral admission service, request-scoped authority, AiConfig read at the use site (not threaded); -10 for the one B4 test-isolation seam that reaches into the shared singleton.
• [CONTENT_COMPLETENESS]: 88 — thorough Fat Ticket + in-tree runbook + explicit tool contracts; small deduction for the deferred cross-reference check.
• [EXECUTION_QUALITY]: 72 — production security path is correct and fail-closed; the B4 test mutation is a real safety-critical-class defect (bounded, but the pattern is exactly what ADR-0019 forbids), and CI green did not catch it.
• [PRODUCTIVITY]: 85 — delivers the #15156 hosted-admission leaf end-to-end (server ingress + client + operator script + runbook).
• [IMPACT]: 80 — a security-sensitive multi-tenant ingress into Memory Core; correct here, load-bearing for the hosted-community epic.
• [COMPLEXITY]: 78 — ~2315 lines across MCP client/server, admission + source-registry services, a contract, scripts, and 9 specs; high reader load with a genuine auth-boundary to reason about.
• [EFFORT_PROFILE]: Heavy Lift — a security-critical multi-subsystem feature with a real tenant-isolation boundary.

Coverage note (honesty): this cross-family pass is premise-first + the mandatory gates (ADR-0019/B4, tool-budget, provenance) + the tenant/auth security core, which is where a security PR's risk concentrates. The admission logic internals (idempotency / backpressure / conflict / epoch-resolution correctness) I assessed at the design/contract level; I'll go line-deep on those on the re-review once the B4 is resolved. Strong feature — fix the one test seam and this is close.

Authored by Vega (Claude Opus 4.8, Claude Code). Session ec14fd1b-28a0-4157-aef3-dbe8a5003eca.

[review-budget-managed]

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