PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The two things most likely to make an instrumentation PR harmful — changing what reaches the provider, and changing admission order — are both provably not done, and I checked them at source rather than accepting the claim. The one concern I found is a growth property this PR inherits from the existing telemetry family rather than introduces, which makes it a follow-up, not a return cycle. Request Changes on an inherited family property would be nitpick-gating; Approve+Follow-Up would be miscategorising it, since there is no deferred correctness in the delivered scope.

Peer-Review Opening: This is the cleanest instrumentation diff I have reviewed here. The part I want to name specifically is #notify swallowing observer throws inside the queue: it means a broken telemetry sink degrades to no telemetry rather than to a stalled provider lane, which is the exact failure mode that makes teams distrust instrumentation forever after. One follow-up concern below, and one assumption I want on the record — neither blocks.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16770 in full (including its Contract Ledger and Avoided Traps); the changed-file list; current dev source of InteractiveBatchQueue.mjs, buildChatModel.mjs, MemoryCoreRecorderService.mjs, and chromaClientPrimitives.mjs; ADR-0019 §3 antipattern catalog (mandatory before any ai/ config judgement); npm run ai:structure-map for placement. Memory-Core prior-art sweep returned no relevant prior art — recording that as "nothing surfaced", not as clearance.
• Expected Solution Shape: One frozen, low-cardinality stage allowlist with unknown as the only fallback; instrumentation at the two real provider boundaries that is observation-only. It must NOT hardcode the queue's interactive-before-batch semantics into the telemetry path, must NOT let stage/priority reach the provider request, and must emit null — never 0 — for queueWaitMs where Neo owns no queue. Test isolation should be a deterministic injected clock plus real-SQLite lifecycle coverage, with negative fixtures proving secrets and identity cannot reach storage.
• Patch Verdict: Matches, and improves in one place. Evidence that confirmed it: #nextIndex() is untouched, so admission order is byte-identical; buildChatModel destructures operationStage/priority into named bindings and dispatches only the ...providerOptions rest, so neither reaches model.generateContent; beginProviderActivity computes queueWaitMs only under queueDisposition === 'neo-queued' and leaves it null otherwise, and startProviderActivity re-asserts that with a CASE rather than trusting the insert. The improvement over my expected shape is normalizeModel: I expected an allowlist plus a length cap, and it additionally rejects credential-prefixed, endpoint-shaped, path-shaped, and @/://-bearing values before they can reach a column. That is defence against a leak vector I had not enumerated.
• Premise Coherence: Coheres with verify-before-assert at the substrate level, and that is the actual point of the ticket. The existing surfaces let an operator infer which subsystem is loading the provider from temporal overlap; this replaces inference with a source-owned recorded fact, and the observer is explicitly forbidden from reconstructing a stage from timing or model name. It builds the instrument that makes a class of confident-and-wrong diagnosis unavailable.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16770
• Related Graph Nodes: #13923 / #13927 (provider role attribution, preserved as the adjacent authority) · #16723 / #16724 (the tool-boundary observer this extends) · #12068 / #12088 (REM phase timing) · #16766 (container-health control — separate lane, mine)
• Origin Session ID: 0473b65d-090b-412a-a926-b90e5851f58a

🔬 Depth Floor

Challenge — a follow-up concern, non-blocking:

provider_activity_log has no retention. I searched the full diff for DELETE FROM provider_activity, prune, retention, and any row cap and found none; the only bounding is LIMIT on the read projections, which bounds the answer, not the table.

I then checked whether that is a regression before raising it, and it is not: mc_tool_call_log on dev is unbounded in exactly the same way. So this PR is consistent with the family's existing contract, which is precisely why it should not block here.

What makes it worth writing down anyway is the row rate, which is not inherited. mc_tool_call_log gets one row per MCP tool call. provider_activity_log gets one row per provider call — every chunk of every ingestion batch, every WAL-drain embedding, every mini-summary. On a plane doing bulk ingestion that is orders of magnitude more rows into the same shared SQLite artifact, and it is the artifact whose health we are currently trying to stabilise. The honest framing is that this PR makes an existing unbounded-growth property matter much more than it did, without being the thing that created it. Worth a ticket for the telemetry family as a whole rather than a change here.

Second item — an unverified assumption, for the record:

In createDynamicTextEmbeddingFunction, the AsyncLocalStorage context overrides the configured stage (activity?.operationStage || operationStage). Today that precedence is correct, and I verified it rather than assuming: runWithProviderActivityContext has exactly one producer (drainCycle.mjs) and getProviderActivityContext exactly one consumer, so the only scope that exists is the WAL drain, and the dynamic value genuinely is the more specific one. The assumption being relied on is that this stays true. A second producer wrapping a broader region would silently re-label any embedding function executing inside it — and the failure would look like correct data, which is the shape this ticket exists to eliminate. If a second runWithProviderActivityContext call site ever appears, that precedence line is where to look first.

I also actively looked for, and did not find, three things I expected to find: an ADR-0019 violation (no config leaf is re-derived, and PROVIDER_ACTIVITY_BUSY_TIMEOUT_MS is an internal constant with no env binding, so it is not an A1); a fabricated 0 queue wait on the Gemini path; and stage or priority leaking into the provider request.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise, mechanical, no overshoot
• [RETROSPECTIVE]-class prose: no inflation
• Linked anchors: #13923 / #16723 / #12088 are cited as adjacent authorities the ticket does not replace, and the diff honours that — ollamaEvalAttribution and the REM phase telemetry are untouched

Findings: Pass. The body claims a bounded, redacted, side-effect-free ledger and the diff substantiates each of those three words separately.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered in this review.
• [RETROSPECTIVE]: The design decision worth remembering is where the timing is taken. Queue wait and execution are separated at InteractiveBatchQueue, the only component that actually knows admission order — not reconstructed by a caller wrapping generateContent, which is the obvious cheaper implementation and would have silently folded queue wait into execution for every local chat call. That distinction is the whole diagnostic value of the ticket, and it survives only because the instrumentation went to the component that owns the fact. The second is #notify's catch: telemetry that cannot throw into the lane it observes.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16770 (standalone, newline-isolated, single target)
• #16770 labels are enhancement, ai, architecture, performance, agent-os — not epic

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches it

Findings: Pass — spot-checked five rows against source rather than against the PR body. buildChatModel strips stage/priority before dispatch (row 1); #nextIndex() untouched so FIFO semantics are byte-equivalent and a throwing task still drains (row 2); missing stage normalizes to unknown via the closed STAGES set (row 3); the new table lives in the existing shared telemetry artifact rather than a new one (row 4); the observer extends get_memory_core_tool_metrics with a providerActivity field and adds no new tool path or operationId, which is the ticket's explicit Out-of-Scope line (row 5). No drift found.

🪜 Evidence Audit

• PR body contains a greppable Evidence: line
• Residual is explicit: L2 → L3 required. Residual: AC12 [#16770], and AC12 is written in the ticket as an L3 post-merge item, so the ceiling and the residual agree rather than the PR inventing a caveat
• Two-ceiling distinction: the L2 ceiling is a genuine sandbox limit — "two concurrent known workloads on the deployed shared plane" is not reachable from an unmerged head — not an unprobed author stop
• No evidence-class collapse: this review does not promote the deterministic fixtures to a claim about live concurrent behaviour

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

• Single-line descriptions throughout; no block literals added
• No internal cross-refs — no ticket numbers, phases, or session IDs in any description payload
• Call-site usage language, not architectural narrative
• Longest added description is 113 characters, far inside the 1024 cap
• No new tool path or operationId — this extends the existing observer's response schema, per the ticket's Out of Scope

Findings: Pass. Worth noting the shortest one carries real information: "Payloads, raw labels, and caller identity are absent" tells a calling agent the redaction contract at the point of use.

🔗 Cross-Skill Integration Audit

• No skill file, workflow convention, or AGENTS*.md surface touched
• No new MCP tool added, so no skill reference payload needs updating
• The one new convention — "a provider caller declares a stage from the frozen allowlist" — is documented at its definition site in PROVIDER_ACTIVITY_STAGES and enforced by normalizeEnum falling to unknown, so a caller that ignores it degrades visibly rather than silently

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 2fd5c36210a21484d9e7c19a4276578e6afcf1ac — 16/16 checks pass. 21 spec files against 21 source files.
• Reviewer falsifier: N/A — my two concerns are a growth property and a future-precedence risk; neither is falsifiable by running a test at this head, and I did not run one to manufacture the appearance of a probe.
• Test location: pass — the two new specs sit beside their modules under test/playwright/unit/ai/services/shared/, mirroring the source tree.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — instrumentation went to the components that own the facts (InteractiveBatchQueue for admission order, TextEmbeddingService for the embedding boundary) rather than to a convenient outer wrapper, and the shared ledger sits in ai/services/shared/ where both the KB and MC recorders legitimately reach it. 5 deducted as a placement nit, not a defect: ai/services/shared/ already uses subfolders (contentTrust/, vector/), so two new sibling telemetry modules flat at that level is a mild drift from the local convention.
• [CONTENT_COMPLETENESS]: 100 — every new exported function carries Anchor & Echo JSDoc; actively checked that the non-obvious decisions are documented where a reader will hit them, and they are: the #notify catch explains why observability cannot alter admission, and the batch-vs-fan-out comment in chromaClientPrimitives explains why the whole batch goes through embedTexts.
• [EXECUTION_QUALITY]: 96 — scored from exact-head green CI plus source verification of the four failure modes that matter here (order preservation, stage stripping, null-not-0 queue wait, redaction). 4 deducted for the AsyncLocalStorage precedence relying on there being exactly one context producer, which is true today and untested as an invariant.
• [PRODUCTIVITY]: 100 — all twelve ACs are addressed, and AC12 is honestly declared as an L3 post-merge residual rather than quietly claimed.
• [IMPACT]: 80 — this is the missing axis for answering "which Neo operation is loading the endpoint?", the question a saturated shared provider makes urgent. Not 90+: it is a diagnostic surface, so it changes what we can see rather than what the system does.
• [COMPLEXITY]: 85 — 42 files across four subsystems, an async-context seam, a new SQLite table with a lifecycle spread over four call sites, and a redaction contract that has to hold at every one of them.
• [EFFORT_PROFILE]: Heavy Lift — high complexity and broad reach, with the difficulty concentrated in keeping a cross-cutting concern from perturbing the paths it instruments.

The thing I would have gotten wrong writing this myself is the queue-wait boundary — I would very likely have wrapped generateContent and reported one duration, and the resulting metric would have looked correct while being unable to answer the question the ticket was filed for.

Reviewed by 🖖 Grace (Claude Opus 5, Claude Code). Session a641ddac-565a-4fc8-adc1-6c25629bddb7.