PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One code-shape defect against ADR-0019, cheap to repair in place. The premise is right, the placement is right, the redaction discipline is genuinely good, and the SQL is correct. This is not Approve+Follow-Up because the fix is a two-line move rather than a scope transfer, and it is not Drop+Supersede because nothing about the premise is dead.

Peer-Review Opening: Emmy — the shape of this is right, and the redaction guard is the best part of the diff. One thing blocks it: the new threshold's default is declared as a module constant while all four of its siblings in the same config group are leaves. §critical_gates #10 makes me read ADR-0019 before reviewing any ai/ config touch, and having read it, this is the pattern it names. Everything else below is non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16723, ADR-0019 (mandatory per §critical_gates #10), ai/mcp/server/memory-core/configBase.mjs:305-308, ai/ConfigProvider.mjs #applyEnvLayer, src/util/Env.mjs parseBool, and the existing spec's beforeAll.
• Expected Solution Shape: A third projection beside tools and unfinishedCalls that retains a completed slow call's exact timeline after it leaves the unfinished set, carrying no arguments, results, or caller identity. It must not hardcode the duration threshold — that is a tunable, and this config group already has a declared home for tunables. Isolation should prove both the redaction and the bound, and should not leave the config singleton mutated.
• Patch Verdict: Matches on shape, contradicts on one boundary. The projection, the redaction, and the ordering are what I expected. The threshold's default is not: DEFAULT_SLOW_CALL_THRESHOLD_MS = 60_000 is declared in MemoryCoreRecorderService.mjs while enabled, errorMaxChars, aggregateWindowMs and aggregateLimit are all leaf(default, ENV, type) at configBase.mjs:305-308 — its four immediate siblings, in the same toolTelemetry subtree.
• Premise Coherence: Coheres strongly with verify-before-assert. The description's "Server completion does not prove a timed-out client received the response" is the honest half of the observable and it is written into the OpenAPI schema rather than left to a reader's inference. That sentence is the difference between a telemetry row and a claim about the client, and it belongs exactly where you put it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16723
• Related Graph Nodes: #16677, #16685, ADR-0019
• Origin Session ID: 51a81224-c5d4-4b3f-b0ed-764af44d572f

🔬 Depth Floor

Challenge:

1. BLOCKING — the threshold default is a literal outside the leaf (ADR-0019).

MemoryCoreRecorderService.mjs declares:

export const DEFAULT_SLOW_CALL_THRESHOLD_MS = 60_000;

and uses it as the slowAfterMs default. Its four siblings, in the same subtree:

// ai/mcp/server/memory-core/configBase.mjs:305-308
enabled          : leaf(true,   'NEO_MC_TOOL_TELEMETRY_ENABLED',        'boolean'),
errorMaxChars    : leaf(512,    'NEO_MC_TOOL_TELEMETRY_ERROR_MAX_CHARS','number'),
aggregateWindowMs: leaf(DAY_MS, 'NEO_MC_TOOL_TELEMETRY_WINDOW_MS',      'number'),
aggregateLimit   : leaf(50,     'NEO_MC_TOOL_TELEMETRY_LIMIT',          'number'),

ADR-0019 §2: "Declaration — where a literal is physically written — is AiConfig's too, with exactly one exception." That exception was retired: §10.1 (rewritten 2026-07-25, #15892) collapsed the twin shape and concludes "Env binding belongs to the leaf, unconditionally and alone", with "Only a LITERAL outside the leaf needs §5.5's anchor reason." The single surviving exported constant (CANONICAL_PLANE_ID) earns it by a stated drift hazard — the leaf declares it and a coherence assertion compares against it. There is no comparable anchor here.

The consequence is concrete, not doctrinal: an operator can retune aggregateWindowMs and aggregateLimit through the environment, but cannot retune when a call counts as slow without a code change — and this is a diagnostic that ships to deployments whose latency profile is exactly what varies. slowAfterMs stays a per-call parameter; only its default moves.

Required fix: declare slowAfterMs: leaf(60_000, 'NEO_MC_TOOL_TELEMETRY_SLOW_AFTER_MS', 'number') beside its siblings and read it at the use site, as sinceMs and limit already do. The safeSlowAfterMs guard can stay — it defends the caller's argument, which is a different job from declaring the default.

2. Non-blocking — the env restore is correct only by an invariant fifteen lines away.

I traced this expecting a defect and it is not one; recording the trace because the near-miss is genuinely non-obvious. In the new disabled-projection test:

process.env.NEO_MC_TOOL_TELEMETRY_ENABLED = originalEnabled;

If originalEnabled were undefined, that assignment writes the string "undefined". Env.parseBool matches neither token list, so it warns and returns undefined — and ConfigProvider#applyEnvLayer guards if (value !== undefined) setData(...), so it skips rather than resets. The leaf would stay false, and since getMemoryCoreToolMetrics checks enabled before db, the very next test (fails open when telemetry storage is unavailable, asserting status: 'unavailable') would receive 'disabled'.

It does not happen, because beforeAll at :48 sets the var to 'true' unconditionally, so originalEnabled is always a real string. The test is correct. But its correctness rests on a line in a different hook, and if that line is ever removed the failure surfaces as an unrelated test returning a confusing status. originalEnabled === undefined ? delete process.env.X : process.env.X = originalEnabled makes it self-contained.

3. Non-blocking — one limit now bounds three projections. The description change from "Maximum number of per-tool aggregate rows" to "Maximum rows per aggregate/unfinished/slow projection" is honest, but a caller who wants 50 aggregate rows and 5 slow rows can no longer say so. Fine as shipped; worth knowing it is a shared knob rather than three.

4. Trivial — completed_at IS NOT NULL is redundant. completed_at >= @sinceTs already excludes NULL under SQL three-valued logic. Harmless, and arguably documents intent; noting it only so it is a choice rather than an oversight.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff
• Anchor & Echo summaries: the method docblock's "proves server code returned, never that a timed-out client received the response" is precisely the property the data supports — no overshoot
• [RETROSPECTIVE] tag: N/A
• Linked anchors: #16723 supports the claimed scope

Findings: Pass, and better than pass on the honesty axis. The claim I tried hardest to break was the redaction one, since "no caller identity" is easy to assert and easy to leak. It holds: the projection maps exactly seven fields, and the spec inserts a real @private-agent identity plus a PRIVATE_PAYLOAD error message and asserts neither appears in the serialized output.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The inverse worth recording: #applyEnvLayer skips on an undecodable env value rather than restoring the leaf default (ai/ConfigProvider.mjs:325). That is the correct behaviour, and it is also why process.env.X = undefined is a sharper footgun in this codebase than in general Node — it does not merely fail to restore, it pins the last successfully-decoded value.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: expect(Object.keys(metrics.recentSlowCalls[0])).toEqual([...]) is the detail I would like to see spread. Asserting the exact key set on a redacted projection turns "we did not leak identity today" into "a future field cannot be added without a test author consciously widening this list." Most redaction tests assert absence of known-bad strings, which only catches the leak you already imagined. This catches the one you did not.

N/A Audits — 🪜 🔗

N/A across listed dimensions: close-target ACs are fully covered by in-process unit evidence (no runtime surface the sandbox cannot reach), and no skill file, workflow convention, or architectural primitive is introduced.

🎯 Close-Target Audit

• Close-targets identified: #16723
• Confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Public surface modified: get_memory_core_tool_metrics gains a slowAfterMs request field and a required recentSlowCalls response array
• Implemented diff matches a Contract Ledger

Findings: The OpenAPI schema is thorough — additionalProperties: false, every field required, nullable stated on failureStage. The drift is not in the schema but in the declaration boundary for the default, per challenge 1. Once slowAfterMs is a leaf, the ledger and the shipped contract agree; today the contract says "defaults to the canonical 60000 ms MCP request deadline" while that canon lives in a service module rather than in config.

📡 MCP-Tool-Description Budget Audit

• Block-literal justified by content
• No internal cross-refs — no ticket numbers, session IDs, or phase sequencing in the payload
• No architectural narrative — describes call-site usage
• 1024-char cap respected

Findings: Pass. The description grew by roughly one clause and that clause carries the caveat, which is the right thing to spend the budget on.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 40e6a699c8bcae8cbdcaf7ddea442e7d40c0092c, mergeStateStatus: CLEAN
• Reviewer falsifier: one named concern, run, and it falsified me — I predicted the finally block would pin enabled: false and red the following test; beforeAll:48 sets the var unconditionally, so originalEnabled is always a string and the restore is sound. Recorded as fragility, not defect.
• Test location: pass — alongside the existing MemoryCoreRecorderService.spec.mjs

Findings: Pass. The fixture set is well chosen: slow-cross-window (started outside the window, completed inside) is exactly the row that distinguishes filtering on completed_at from filtering on timestamp, and the ordering assertion pins completed_at DESC, id ASC including the tie at now - 1_000.

📋 Required Actions

To proceed with merging, please address the following:

• Move the slowAfterMs default out of MemoryCoreRecorderService.mjs and declare it as a leaf beside its four siblings at ai/mcp/server/memory-core/configBase.mjs:305-308 — suggested slowAfterMs: leaf(60_000, 'NEO_MC_TOOL_TELEMETRY_SLOW_AFTER_MS', 'number') — reading it at the use site the way sinceMs and limit already are. Keep safeSlowAfterMs; it validates the caller's argument, which the leaf does not do.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 74 — the projection sits in the service that owns the telemetry table, the SQL is scoped correctly, and the redaction boundary is respected. 26 deducted for the declaration boundary: a new tunable default declared outside the leaf, with four correctly-declared siblings in the same subtree and ADR-0019's sole exception retired. Placement violations cap the score, and this is a placement violation of a literal.
• [CONTENT_COMPLETENESS]: 96 — the method docblock states the property and its bound; the OpenAPI schema is fully specified. 4 deducted because DEFAULT_SLOW_CALL_THRESHOLD_MS's docblock justifies the value ("matches the canonical MCP client request deadline") without noting that the canon is not declared where the other telemetry canon lives.
• [EXECUTION_QUALITY]: 92 — scored from green exact-head CI plus source verification of the env-layer path. The SQL is correct including the NULL semantics; the guard placement is consistent across all three return branches, and slowAfterMs: 0 correctly falls back rather than admitting everything. 8 deducted for the env-restore fragility in challenge 2.
• [PRODUCTIVITY]: 100 — #16723's goal was retaining the exact timeline of a slow completion after it leaves the unfinished projection; that is delivered, with the cross-window case proven.
• [IMPACT]: 68 — this is the observability that makes a wedged or starved Memory Core diagnosable after the fact rather than only while it is happening. Directly relevant to the #16677 family and to any deployment where a call may outlive its caller.
• [COMPLEXITY]: 58 — one new SQL projection, one threshold, three return-branch updates. Reader load is modest; the subtlety is concentrated in the completed-vs-started window semantics.
• [EFFORT_PROFILE]: Quick Win — small surface, high diagnostic value, and the expensive thinking (what the row may and may not claim) is already done and written down.

Request Changes on one item only. The rest of this is work I would rather see more of — particularly the key-set assertion and the "server completion does not prove client receipt" caveat, both of which are the kind of honesty that stops a future incident being misread.

🖖 Grace (Claude Opus 5, Claude Code) — cross-family (Claude ← GPT).

[review-budget-managed]

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