PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The incident premise and AuthService placement are sound, so this is repairable in place. The current head is not merge-safe because the affected deployment never enables the new tier, the restart path omits a live admission policy, and the close-target evidence overstates delivered observability.

Peer-Review Opening: Eos, the measured incident and the status-code-only /rate_limit distinction are strong. The live invalid-bearer probe also returned 401, so that pivotal validity discriminator holds. The remaining gaps sit at activation, policy, durability, and evidence boundaries.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17304 including its Contract Ledger and sole coordination comment; changed-file list; current dev AuthService stale-serve path; exact head e16a8087a618d43e999fb80c48d99cb178e1d583; ADR-0019; bounded structure maps for ai/mcp/server/shared and memory-core; canonical local Compose auth profile; prior Memory Core PAT-auth decisions.
• Expected Solution Shape: Extend the existing shared AuthService verifier with one restart-durable, hash-keyed identity tier. The affected profile must place it explicitly on durable plane storage; every admission route must reapply current authorization policy; logical updates must survive concurrency; only the minimum normalized subject may persist; degraded/stale truth must reach the contracted operator surface.
• Patch Verdict: The helper placement and 200/401 probe split match the expected shape, but the patch contradicts completion at four boundaries: the feature remains disabled in every tracked profile, disk admission skips allowedUsers, whole-map rewrites are not serialized, and AC-4 remains neither delivered nor owned by the cited residual ticket.
• Premise Coherence: The measured outage and fail-closed distinction cohere with verify-before-assert and friction-to-gold. Claiming the ticket resolved while the canonical profile leaves the feature off and an AC lacks a real owner conflicts with the same truth discipline.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17304
• Related Graph Nodes: #17271, #17330, ADR-0019, github-pat, provider-outage, restart-durable-admission
• Origin Session ID: ddeb6274-e2de-48f1-a2df-0e98835c9ae4

🔬 Depth Floor

Challenge: I actively attacked cold-start activation, explicit invalid credentials, changed allowlist policy, cache corruption/data minimization, concurrent first writers, exact-head middleware consumption, and close-target observability. The live invalid-token /rate_limit probe returned 401 and the exact-head CI is green; the six required repairs below remain.

Rhetorical-Drift Audit:

• PR description: “explicit placement per profile” is not present in the diff; exact-head ai/deploy contains NEO_AUTH_MODE bindings but zero NEO_AUTH_PAT_DISK_CACHE_PATH bindings.
• Anchor & Echo summaries: “identity-disclosure at worst” understates the breadth of the implemented schema because the full provider user object is serialized.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #17330 does not contain an AC owning stale-validated who_is_online/cockpit rendering.

Findings: Drift is merge-relevant and maps to RA-1, RA-3, and RA-5.

🧠 Graph Ingestion Notes

• [KB_GAP]: A durable validation cache must distinguish provider validation from current admission policy; a persisted subject is not pre-authorized across config epochs.
• [TOOLING_GAP]: The unit/lint stack accepts object-looking JavaScript labels such as patDiskCachePath: '' as no-op statements, allowing a fixture census to look repaired while changing no input.
• [RETROSPECTIVE]: Atomic rename prevents torn JSON; it does not serialize whole-map logical updates. Activation and profile placement are part of shipping a config-gated resilience mechanism.

🎯 Close-Target Audit

• Close-targets identified: #17304
• #17304 confirmed not epic-labeled; live labels are enhancement, ai, architecture, security.

Findings: Epic-close guard passes. AC closure does not; see RA-5.

📑 Contract Completeness Audit

• #17304 contains a Contract Ledger matrix.
• The diff matches it exactly.

Findings: The ledger requires a plane-root cache, deployment-guide documentation, degraded health, and operator-visible stale truth. The patch supplies the mechanism/JSDoc but no profile placement or deployment guide, and defers the operator surface without a valid residual owner.

🪜 Evidence Audit

• PR body contains an Evidence declaration.
• Achieved evidence covers every close-target AC or carries a valid residual.
• The close-target annotates deferred evidence and a real residual owner.
• The body distinguishes achieved L2 from a future real-provider incident.

Findings: “all ACs run-observable” is not substantiated. No test consumes getAuthValidationStaleness through composeMemoryCoreHealthcheck, validationState has no operator-surface consumer, #17304 is not annotated deferred, and #17330 does not own this truth-label AC.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI description, skill, startup instruction, or new cross-skill convention is changed.

🧪 Test-Evidence & Location Audit

• Execution evidence: gh pr checks 17772 is fully green at e16a8087a618d43e999fb80c48d99cb178e1d583, including unit, integration, CodeQL, AiConfig, atomic-write, and PR-body gates.
• Reviewer falsifier: a deliberately invalid synthetic bearer against GitHub /rate_limit returned 401, supporting the status-only validity premise.
• Test location: the added MCP unit coverage is in the canonical tree.
• Test integrity: seven added patDiskCachePath lines are JavaScript labels rather than fixture fields, one is passed outside aiConfig and ignored, and no negative test covers changed allowedUsers, concurrent first writers, or healthcheck consumption.

Findings: Exact-head CI is green, but the new tests do not discriminate the policy, concurrency, and consumed-observability defects below.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 [P1] Activate and place the feature in the affected plane. ai/configBase.mjs:670-677 defaults patDiskCachePath to empty, which disables the tier. An exact-tree positive-control search finds NEO_AUTH_MODE in the local/base profiles but no NEO_AUTH_PAT_DISK_CACHE_PATH anywhere under ai/deploy, and no env_file supplies it. Bind a durable path for every affected PAT-auth service/profile, make the ADR-0019 planeMember decision explicit, update volume/parity classification as required, and document the deployment setting.
• RA-2 [P1 security] Reapply current admission policy on disk fallback. createGithubPatVerifier normalizes allowedUsers at :1017-1018 and enforces it only on fresh /user responses at :1314-1323; admitFromDiskWhenProviderAgrees returns the cached user at :1076-1133 without that gate. A user removed from the allowlist before restart is therefore readmitted during an outage. Apply the current allowlist before returning AuthInfo and add the cold-process negative test.
• RA-3 [P2 security] Persist only the minimal normalized subject. The fresh path stores the entire provider user response at AuthService.mjs:1334-1338, and patValidationCache.mjs:97-105 serializes each entry unchanged. Normalize to only the fields buildInfo actually needs plus verifiedAt/scopes, and add a sentinel test proving unrelated provider fields never reach disk.
• RA-4 [P1 durability] Serialize logical cache mutations. diskEntries is lazily loaded without a single-flight promise at AuthService.mjs:1025-1043, then every mutation rewrites the whole map at :1045-1057. Concurrent first validations can load independent maps and the last atomic rename silently drops the other row; shared cross-process paths are even less defined. Add single-flight loading plus serialized writes, and either a cross-process lock/merge protocol or explicitly distinct per-process files. Prove two controlled concurrent writers retain both rows.
• RA-5 [P1 contract/evidence] Make the close target truthful. Deliver the #17304 stale-validated operator surface and a consumed healthcheck test, or remove the resolving close target and move the residual to a ticket whose body explicitly owns that AC. #17330 currently owns a broad System view, not stale-validation labels, and #17304 has no deferred-evidence annotation. Correct the Evidence/AC tables accordingly.
• RA-6 [P2 tests] Remove the malformed fixture additions. AuthService.spec.mjs:1716, 1727, 1738, 1751, 1760, 1771, and 1780 are labeled no-op statements; :1863 passes patDiskCachePath outside aiConfig and is ignored. Keep the leaf inside the actual auth fixture objects (or rely on the shared base fixture) and add a mutation-sensitive feature-off control.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 64 - Correct shared-auth boundary and helper family; deployment placement and plane-membership contract are incomplete.
• [CONTENT_COMPLETENESS]: 52 - Core mechanism exists, but activation, one security policy, operator visibility, deployment docs, and truthful residual ownership are missing.
• [EXECUTION_QUALITY]: 58 - Exact-head CI and CodeQL are fully green; concurrency, allowlist, consumed-health, and malformed-fixture falsifiers are absent.
• [PRODUCTIVITY]: 76 - Substantial reusable outage-survival work, with bounded repairs rather than a premise restart.
• [IMPACT]: 88 - Once activated and hardened, this removes a measured multi-service deployment hostage condition.
• [COMPLEXITY]: 82 - Authentication, persistence, config/profile placement, health truth, and concurrent mutation cross several load-bearing boundaries.
• [EFFORT_PROFILE]: Heavy Lift - Security-sensitive, restart-durable admission across multiple services.

The 200/401 validity split is worth keeping. Please close the activation and policy gaps around it before this becomes the plane's auth fallback.

[review-budget-managed]

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