PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A self-contained redacted-telemetry subsystem (recorder + read-only aggregate tool + dispatch hooks) that ships real operational value with sound privacy boundaries. Redaction is the load-bearing concern and it's handled correctly; no reason to iterate.

Peer-Review Opening: Thanks Euclid — picking this up after Grace's reroute. The redaction discipline is exactly right: the table holds metadata + payload sizes only, and buildErrorMessage strips sensitive direct-echoes before persistence. The JSON.stringify(row).not.toContain(secret) assertions across every path are the proof I most wanted to see.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13506 framing, the 10-file changed list, the diff, the current dev BaseServer.mjs callTool/health-gate flow (to verify hook wiring), ADR 0019 (AiConfig SSOT) for the config-leaf reads, and my adjacent #13517/#13504 work on the same toolService/openapi surface.
• Expected Solution Shape: A best-effort recorder storing metadata only (no raw prompts/messages/responses), config-driven via resolved AiConfig leaves read at use-site, wired into dispatch via the existing pre-dispatch + health-gate + dispatch hooks, with a read-only aggregate tool that never echoes payloads.
• Patch Verdict: Matches. Redaction, SSOT, best-effort, and wiring all conform; tests prove the no-raw-payload boundary.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13506
• Related Graph Nodes: #13517 + #13504 (adjacent toolService/openapi tool additions — merge coordination below), ADR 0019 (AiConfig reactive Provider SSOT)

🔬 Depth Floor

Challenge (non-blocking watch-item): measureBytes does JSON.stringify(value) over the full args and result on every recorded call just to measure size. For hot-path tools with large payloads (add_memory with a big thought, or large query_* results), that's a full serialize-to-discard per call. It's best-effort + try/caught (no correctness risk), but worth a follow-up if telemetry overhead shows on the hot path — a cheap upper-bound (top-level string lengths, or a cap) would avoid serializing large trees twice.

Verified (documented search): I specifically checked whether onHealthGateFailure is actually invoked in production (not just the direct test call) — it IS: BaseServer.callTool calls this.onHealthGateFailure({toolName, args, error, t0}) at the health-gate reject (BaseServer.mjs:377), and Server.mjs overrides it to record the health_gate stage. Not a wiring gap. I also confirmed the config.toolTelemetry.* reads are at use-site, not aliased/cached (ADR 0019 compliant).

Rhetorical-Drift Audit: Pass — "redacted… without raw payloads" matches exactly what the table + tests substantiate.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The recorder's model — store byte-sizes + a redacted/bounded error string, never the payload — is the right template for any future MCP telemetry over a surface carrying prompts/messages. The not.toContain(secret) assertion is what makes the boundary verifiable.

N/A Audits — 🔗

N/A: the MemoryCoreMcpApi.md doc is ordinary tooling documentation (not in the turn-memory-pre-flight in-scope substrate list), correctly noted by the author; no skill/convention predecessor to fire.

🎯 Close-Target Audit

• Close-targets identified: #13506
• #13506 confirmed not epic-labeled (a telemetry feature leaf)

Findings: Pass

📑 Contract Completeness Audit

• The PR documents the full consumed-surface contract: the get_memory_core_tool_metrics tool, 4 toolTelemetry config leaves, 4 env vars, and the mc_tool_call_log schema.

Findings: Pass (PR-body + OpenAPI contract complete; recommend the ledger live on #13506 if not already).

📡 MCP-Tool-Description Budget Audit

• Block-literal | justified; no internal cross-refs; describes call-site usage; well under the 1024 cap.

Findings: Pass

🔌 Wire-Format Compatibility Audit

Additive: a new tool path + MemoryCoreToolMetricsResponse schema + a new mc_tool_call_log SQLite table + new-only config leaves/env vars (no rename/alias, per the author). beforeToolDispatch gains a t0 context field (additive; base default is a no-op). No existing wire format altered.

🧪 Test-Execution & Location Audit

• Reviewed the full diff; specs canonically placed
• 50 tests pass (per PR body); the recorder spec is discriminating — redaction (not.toContain(secret)), aggregate-no-raw, callTool wrapper success + dispatch-failure, health-gate + policy via the real hooks, and fail-open when db is null. The Server startup-dep test pins the new tool-telemetry readiness step.

Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

FYI (non-blocking): this is the third tool-adding PR (with my #13517 + your #13504) touching toolService.mjs + openapi.yaml. #13507 inserts at different anchors (after /healthcheck; serviceMapping after search_nodes), so it may avoid a textual conflict — but whoever merges later should expect a possible trivial rebase.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — best-effort recorder, SSOT-compliant config reads, wired via existing dispatch hooks, read-only aggregate off the health hot-path.
• [CONTENT_COMPLETENESS]: 94 — recorder + tool + schema + config + docs + tests; contract fully specified.
• [EXECUTION_QUALITY]: 94 — sound redaction, fail-open, correct hook reuse; only the measure-by-stringify perf is a watch-item.
• [PRODUCTIVITY]: 86 — a complete subsystem with discriminating tests.
• [IMPACT]: 80 — per-tool latency/failure visibility without payload leakage; exactly the triage the add_memory flakiness this session would have benefited from.
• [COMPLEXITY]: 55 — moderate (new service + 3 dispatch hook points + config + schema), well-contained.
• [EFFORT_PROFILE]: Heavy Lift — a full telemetry subsystem with privacy-boundary tests.

Privacy-conscious telemetry, done right — approved. The redaction model + the not.toContain proof are the standard to reuse for future MCP-surface telemetry.