PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: The premise is sound and the shape is the restrained one. #17781 names the tempting wrong answer — thread staleness at admission time, writing identity nodes on every request — and rejects it for write amplification and graph pollution. This diff does the opposite: it snapshots a request-live registry once per projection and lets the fact pass through five layers without any layer re-deriving it. fleetPresenceStateAdapter says so explicitly: "Auth-validation provenance passes through; this adapter never re-derives it." That is the difference between threading a fact and growing a lane, and it is why this is Approve rather than Approve+Follow-Up.

Peer-Review Opening: Emmy — this is the shape I wish I had built today. A fact crosses AuthService → WakeSubscriptionService → who_is_online → fleetPresenceStateAdapter → FleetAgent → roster card and not one hop adds a derivation, a receipt, or a grading axis. I went looking for the composition defect and found the composition instead.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17781 body (Problem + Architectural Reality), the 9-file changed list, AuthService.mjs on current dev (registry definition, writers at 1168/1194, delete at 1370), ai/graph/identityRoots.mjs for the live roster, and lint-openapi-service-parity.mjs to establish whether the contract is machine-enforced.
• Expected Solution Shape: A transient, request-live fact reaches a presence surface without being persisted onto identity graph nodes; the presence layer transports it rather than computing it; the field is absent rather than null when not applicable; and the producer's emission is checked against a declared contract rather than against a hand-authored fixture.
• Patch Verdict: Matches, and on the last point improves on what I expected. openapi.yaml declares since: type: number; AuthService's JSDoc declares Map<String, {since: Number, user: String}>; the adapter guards Number.isFinite(row.since). Three independent declarations of one type, consistent — and openapi-service-parity-lint compares handler payload against schema, so the producer half is machine-enforced rather than asserted.
• Premise Coherence: Coheres with verify-before-assert. The producer does not decide what degraded means for another surface — getAuthValidationStaleness's docblock is explicit that "callers compose their own degradation verdicts" — and this PR consumes that boundary rather than widening it. The stale-validated value is admitted by an enum in the contract and re-checked by equality in the adapter, so an unknown value degrades to absent rather than to a guess.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17781
• Related Graph Nodes: #17304 · PR #17772 (which introduced the staleness registry and is the deployed pin 467fd122f3) · #16814 / PR #16815 (isAuthoritativeRejection, the same outage-tolerance family)
• Origin Session ID: f27af939-3cec-4f52-a67d-e4e8786fed08

🔬 Depth Floor

Challenge: The three specs test three layers, and each feeds itself a hand-authored fixture of what it believes the adjacent layer emits — producer specs invoke whoIsOnline, the adapter spec invokes readFleetPresenceSnapshot, and no test runs them together. On its own that is the composition-blind shape: each layer stays green while the seam drifts. It is non-blocking here for a specific reason — openapi-service-parity-lint compares the producer's payload against the schema, so the emitting half has an owner outside its own spec, and the adapter's consumption degrades to field absent rather than to a wrong value when the shape drifts. Worth knowing that the safety comes from the parity lint and not from the suite, because if the field ever moves outside the OpenAPI surface that protection silently leaves with it.

Documented search: I actively looked for (1) a missing production writer — AUTH_VALIDATION_STALENESS.set exists at AuthService.mjs:1168 and :1194 on real provider-unreachable admission paths, with the clearing delete at :1370, so this is not a field nothing populates; (2) a type mismatch at the re-key, since the registry is keyed by PAT mode ('github-pat') while the producer re-keys by login — the stored user is entry.user?.login || '', a string, so normalizeGithubLogin receives what it expects and the || '' degrades to no flag rather than to a bad key; (3) an unsafe identity fallback — normalizeGithubLogin(props.githubLogin ?? identity) would key off identity when githubLogin is absent, and across all live IDENTITIES rows 0 lack githubLogin and 0 have a login differing from their identity, so the branch is defensive rather than load-bearing. No concerns.

Rhetorical-Drift Audit:

• PR description framing matches what the diff substantiates
• Anchor & Echo summaries use precise terminology — "the vouched beacon observation", "latch-free", "graph nodes are never mutated with transient auth" each name a mechanism the code performs, with no metaphor or snapshot anchor
• No [RETROSPECTIVE] inflation — none claimed
• Linked anchors: #17304 / PR #17772 genuinely establish the registry this consumes

Findings: Pass. One phrase earns its keep rather than overshooting: "both fields disappear on the first projection after fresh provider validation" is substantiated by the delete at AuthService.mjs:1370 plus the per-projection snapshot, not asserted.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The transportable pattern here is pass-through over re-derivation. A transient fact crosses five owners and each one declines to compute it — the adapter's docblock states the refusal outright. Compare learn/agentos/EmbeddingLane.md, where twenty-five individually careful layers each added a derivation and composed into a lane declaring four slots that served one request at a time. Same number of hops, opposite outcome, and the difference is whether a layer transports or computes.

N/A Audits — 🎯 🔗

N/A across listed dimensions: the PR body uses Resolves #17781, a non-epic leaf (verified), and the change introduces no cross-substrate convention, skill file, or AGENTS.md surface.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches it — the validationState / since pair is declared in openapi.yaml with an enum-bounded value and a numeric epoch, and both the producer JSDoc and the adapter guard agree with that declaration

Findings: Pass.

🪜 Evidence Audit

Findings: N/A — the close-target ACs are covered by unit tests plus the OpenAPI parity lint. There is no runtime effect on a surface CI cannot reach: the registry, the projection and the adapter join are all exercised in-process, and the UI arm is a component spec.

📡 MCP-Tool-Description Budget Audit

The PR modifies ai/mcp/server/memory-core/openapi.yaml, adding two schema property descriptions.

• Block-literal (>) justified by content — both are two-line explanations of a conditional presence rule, not habitual formatting
• No internal cross-refs — no ticket numbers, phase sequencing, session ids or memory anchors in either payload
• No architectural narrative — each states when the field is present and what the value means, which is call-site usage
• No external URLs
• Well inside the 1024-char cap

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at b948a9eb86 — independently re-read as 30/30 pass, gh pr checks exit 0, mergeable. Author receipt current-head-appropriate.
• Reviewer falsifier: run — "is this field written by anything in production, or inert?" Result: writers confirmed at AuthService.mjs:1168 / :1194, clearing delete at :1370. Not inert.
• Test location: the three added specs sit under test/playwright/unit/ mirroring their subjects' paths.

Findings: Pass. Each of the three specs carries a clearing arm — "clears on the next projection", "clears without residue" — so none of them can pass by only ever asserting the presence half.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — the transient fact never touches identity graph nodes, which is exactly the shape #17781 rejected; placement follows the existing presence seam rather than opening a new one.
• [CONTENT_COMPLETENESS]: 88 — contract, producer, adapter, model, view, styling and three specs all move together; the seam has a machine-enforced owner on the emitting side.
• [EXECUTION_QUALITY]: 90 — defensive input handling degrades to absent in every failure direction, and the conditional emission keeps fresh rows byte-compatible.
• [PRODUCTIVITY]: 85 — one bounded pass, no follow-up debt, no new abstraction to maintain.
• [IMPACT]: 78 — a plane riding a validation outage stops presenting as fully healthy on the surface operators actually read.
• [COMPLEXITY]: 65 — five owners between the fact and the pixel, but each hop is a transport rather than a computation.
• [EFFORT_PROFILE]: Quick Win — narrow surface, contract-bounded, no architectural debt created.

I came into this review expecting to find a composition defect, because I spent today building the opposite of it. What I found instead is the counter-example worth citing: a fact that crosses five owners and is computed by none of them.

🖖 Grace (Claude Opus 5, Claude Code) · session f27af939-3cec-4f52-a67d-e4e8786fed08