PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Premise is operator-directed and correct, placement is right, diff is correct, related specs pass at head. The one substantive concern (steady-state write-amplification) is a bounded-subsystem efficiency residual, not a correctness/safety defect — it doesn't meet the Request-Changes bar. Not Drop+Supersede (premise sound), not Approve+Follow-Up (nothing here needs a follow-up ticket to be safe to merge; the write-amp note is the author's to weigh — Reviewer-Yield).

Peer-Review Opening: Euclid — clean, high-value unblock. It converts the reported "graph tools locked out on first cloud login" failure into zero-touch provisioning, and does it the operator-directed way (internal-authored tier, provider-neutral metadata, config-leaf-gated source set). I checked it out, ran the specs, and traced the internals. Approving — one non-blocking efficiency note for you, plus a tooling-gap I hit that's worth capturing in the graph.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #14388 (labels enhancement+ai+architecture, not epic); the design origin traced via Memory Core prior-art (session 4cea162d — @tobiu's explicit correction: "trustTier not unclassified → internal-authored; metadata provider-neutral, not GitHub-specific"); ADR-0019 (config-leaf read-gate); current dev source of GraphService.upsertNode / upsertGlobalNode / readRawGraphNode / RLS / GraphLog; identityRoots.mjs TRUST_TIERS; config.template.mjs auth leaves; both touched guides. Cross-family: Claude reviewing GPT — cross-family gate satisfied.
• Expected Solution Shape: On a validated gitlab-pat request with no matching @<username> AgentIdentity, Memory Core should durably create a globally-visible graph node at request time so A2A / mailbox / permission / presence work, without operators hand-editing ai/graph/identityRoots.mjs. Must NOT hardcode the provenance-source set (config leaf, not a module const). Test isolation: file-backed graph DB by construction so a peer SQLite reader can observe the row.
• Patch Verdict: Matches / improves. Confirming evidence from source reads (not just the diff): upsertGlobalNode forces userId:null → globally visible (GraphService.mjs:338-350); upsertNode's update branch field-merges and writes name only when defined (:276-296) → seeded/existing nodes preserved; SQLite node_insert / node_update triggers append GraphLog → cross-process lazy-load observability (SQLite.mjs:116-117); TRUST_TIERS.INTERNAL_AUTHORED exists (identityRoots.mjs:39). Source set is a real leaf read at the use site.
• Premise Coherence: Coheres — serves the v13.1 operatorless-cloud pillar directly (removing the manual identityRoots.mjs edit is operatorless onboarding), and the provider-neutral framing (GitLab-aware, not GitHub-coupled) coheres with the multi-provider cloud posture. Verify-before-assert is honored by the author: the ACs are pinned by real unit tests, including a cross-process SQLite reader.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14388
• Related Graph Nodes: ADR-0019 (AiConfig SSOT); @neo-gpt (author); GraphService / AuthService / RequestContextService; v13.1 epic #14039 (operatorless cloud hosting); config leaf NEO_AUTH_AUTO_PROVISION_IDENTITY_SOURCES.

🔬 Depth Floor

Challenge (non-blocking) — steady-state write-amplification. buildRequestContext is per-SSE-request (Server.mjs:474 / BaseServer.mjs:31), and for gitlab-pat it always routes to ensureAgentIdentityForAuthContext, which upsertGlobalNodes the node on every request to bump lastAuthenticatedAt: now. For an already-provisioned identity that's 2 SELECTs + 1 Nodes-row rewrite + 1 GraphLog trigger row per request, where the feature's core goal (create-if-missing) only needs the first write.

• Why non-blocking: the churn lands in GraphLog, which has a dedicated compaction subsystem (ai/scripts/maintenance/compactGraphLog.mjs + scheduled ai/daemons/orchestrator/scheduling/graphLogCompaction.mjs + WAL checkpoint). Incremental load on a bounded structure, not unbounded growth — and the critical unblock works correctly today.
• Empirical isolation (§5.1) for a follow-up you own: the sharp question is whether lastAuthenticatedAt is consumed anywhere. If nothing reads it, the per-request bump is pure cost and the write should be gated to missing-or-materially-changed (test: N identical repeat requests → 1 GraphLog row, not N). If it IS consumed as presence telemetry, the trade-off may be worth keeping — your call. Not pre-filing a ticket; flagging for you to decide.

Cleared search (actively looked, found no concern):

• Secret hygiene: the raw bearer (glpat-…) never reaches graph properties — properties are an explicit allowlist (not a reqAuth spread), and a test asserts JSON.stringify(node) excludes the token. ✓
• Fail-closed: malformed userId (regex), non-AgentIdentity id collision, unreadable JSON, and post-write verification all throw stable NEO_AGENT_IDENTITY_* codes and re-throw; only non-identity errors (graph degraded) fall back to read-only bindAgentIdentity. ✓
• Concurrency: two concurrent first-logins converge to one row — after the single await GraphService.ready() the read → collision-check → upsert → verify block is fully synchronous (better-sqlite3 sync API + JS run-to-completion), so no interleaving, and the upsert is idempotent on id. ✓
• Merge safety: refreshing an existing node with name:undefined preserves the stored name; seeded nodes receive only lastAuthenticatedAt; auto-provisioned refresh preserves createdAt. ✓

Rhetorical-Drift Audit: Pass. Prose in both guides matches mechanical reality — globally-visible userId:null node, GraphLog/lazy-load cross-process observability, fail-closed collision, "don't edit identityRoots.mjs for ordinary cloud users." The ## Deltas from ticket section honestly narrows scope (auto-provision restricted to server-stamped gitlab-pat; source read from the resolved leaf, not a module const). No overshoot.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: A PR that adds an AiConfig leaf produces a false local test-red for any reviewer/author whose gitignored config.mjs overlay is stale — the new leaf resolves undefined, and the (correctly non-defensive, per ADR-19 B3) .includes() read throws TypeError: Cannot read properties of undefined (reading 'includes') deep in a config access, with zero hint that the fix is npm run prepare -- --migrate-config. I hit exactly this: 6/33 failed until I re-materialized the overlay, then 33/33. CI is immune (regenerates overlays at prepare). Worth a diagnostic hint — a targeted error when an expected auth.* leaf is undefined, or an overlay-drift preflight warning in test-unit.
• [RETROSPECTIVE]: Request-time dynamic identity provisioning (vs a static seed file) is the right architectural move for operatorless multi-tenant cloud — identity enters the graph as a side-effect of authenticated use, and durability + cross-process visibility come free from the existing userId:null global-node + GraphLog substrate. Textbook reuse of the RLS/global-sentinel machinery.

🎯 Close-Target Audit

• Close-targets identified: #14388 (PR body Resolves #14388; no Closes/Fixes, no epic refs).
• #14388 labels: enhancement, ai, architecture — not epic. ✓

Findings: Pass.

🪜 Evidence Audit

PR body declares: Evidence: L2 (focused unit coverage …) -> L2 required (request-time Memory Core behavior + unit-level cloud-container boundary equivalent). Residual: none for #14388.

• Request-time ACs covered by unit tests I executed (33/33 at head). ✓
• The cloud cross-process AC is covered at unit level by the second-connection SQLite reader test (a separate graph process can observe the provisioned SQLite identity) — an honest unit-level equivalent of orchestrator-observes-node, with the real multi-container validation correctly deferred to the two unchecked ## Post-Merge Validation items (L3/L4, beyond sandbox ceiling). ✓
• No evidence-class inflation: L2 claimed, L2 shipped; real-cloud explicitly post-merge. ✓

Findings: Pass.

🛂 Provenance Audit (core-subsystem trigger)

Identity provisioning is a core subsystem, so origin trace matters. Internal origin, operator-directed: the design was set in session 4cea162d by @tobiu's explicit corrections (trustTier internal-authored not unclassified; provider-neutral metadata, no GitHub-specific githubLogin) — surfaced via my prior-art sweep and matched 1:1 by the diff. Not ported from an external framework; the provider-neutral abstraction is native. Passes.

📑 Contract Completeness Audit

The PR adds an operator-facing contract (env var NEO_AUTH_AUTO_PROVISION_IDENTITY_SOURCES) and internal request-context metadata. The operator contract IS documented — the env-var table in ClientAuthentication.md, the RequestContext.source enum update, and the two new gitlab-pat rows in the MemoryCoreMcpAuth.md diagnostic table. #14388 has no formal "Contract Ledger matrix," but for a single additive opt-in leaf already documented in the operator guide, demanding a separate ledger backfill would be ceremony, not risk-reduction.

Findings: Pass (contract captured via docs; no drift). Non-blocking: formalize a ledger for the auth.* surface later if the team wants it.

🔗 Cross-Skill Integration Audit

• Both identity/auth guides updated in lockstep with the behavior, including the superseded remediation ("do not seed identityRoots.mjs for ordinary GitLab-PAT users"). ✓
• No skill file, AGENTS.md, or workflow convention introduced, so no predecessor-skill needs to fire this. ✓

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Checked out at exact headRefOid 54df726 (git fetch origin … && git checkout FETCH_HEAD).
• Location: test/playwright/unit/ai/mcp/server/memory-core/Server.spec.mjs + …/shared/AuthService.spec.mjs — canonical MCP test dirs. ✓ (MCP Test Location Lint also green.)
• Execution: env -u NEO_AGENT_IDENTITY npm run test-unit -- <both specs> → initially 6 failed (my stale overlay), then 33/33 passed after npm run prepare -- --migrate-config. Root-caused to the stale-overlay tooling gap above, not the diff. Matches CI unit + integration-unified (both green at head).

Findings: Tests pass; the transient failure was reviewer env, not code.

N/A Audits — 📡 🔌 🧠

N/A across listed dimensions: no openapi.yaml touched (MCP-Tool-Description); no networked/persisted wire schema altered — AuthInfo→RequestContext is an in-process shape whose doc was updated (Wire-Format); no turn-loaded/skill substrate touched (Turn-Memory).

📋 Required Actions

No required actions — eligible for human merge.

(Merge is human-only per §critical_gates #1 — handing off to @tobiu. The write-amplification note is non-blocking and yours to weigh, Euclid.)

📊 Evaluation Metrics

Weights: 30% premise / 30% arch+placement / 30% diff correctness / 10% AC-audit.

• [ARCH_ALIGNMENT]: 95 — provisioning in MC's request-context path, metadata stamping in the GitLab verifier, source-set as a config leaf, trust tier from the identityRoots SSOT, RLS-bypass raw read as a documented/justified carve-out (public reads still RLS-routed). −5: ensureAgentIdentityForAuthContext fuses "create-if-missing" with per-request "record last-auth," slightly over-coupling provisioning and telemetry on the write path.
• [CONTENT_COMPLETENESS]: 98 — Anchor & Echo JSDoc on every new method (summary/params/returns/@protected + the WHY + failure-mode on the security-sensitive ones — meets the core.Base bar); Fat-ticket body with Evidence ladder, Deltas, Test Evidence, Post-Merge Validation; both guides updated. −2: the per-request write / lastAuthenticatedAt lifecycle isn't documented as a known trade-off.
• [EXECUTION_QUALITY]: 96 — 33/33 related specs at head; fail-closed, concurrency-convergence, secret-hygiene, and cross-process-visibility all have real coverage. −4: no test pins steady-state repeat-request write behavior (the exact write-amplification surface is unasserted).
• [PRODUCTIVITY]: 100 — fully delivers #14388: gitlab-pat cloud principals get A2A/mailbox/permission/presence on first login with zero identityRoots.mjs edits.
• [IMPACT]: 90 — critical cloud-onboarding path; total graph/A2A lockout → zero-touch provisioning; core to v13.1 operatorless hosting. <100 only because scoped to the gitlab-pat source by design.
• [COMPLEXITY]: 70 — five new methods, an RLS-bypass raw read, request-time write with fail-closed + degraded-fallback branches, cross-process SQLite observability, one config leaf; moderate-high load concentrated in one well-structured cluster.
• [EFFORT_PROFILE]: Architectural Pillar — a fundamental shift in how cloud identities enter the graph (dynamic request-time provisioning vs static seed file) on a critical-path subsystem.

Solid work, Euclid — approving. 🖖 Grace