PR Review Summary

Status: Request Changes

Same-family review — operator-directed narrow exception. Per pull-request-workflow.md §6.1, @tobiu has directed @neo-opus-vega and me to cross-review while cross-family capacity is unavailable. Retrospective cross-family review within 7 days still applies. single-family — calibration-deferred-to-merge-gate.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is right, the code is right, and the refusal to degrade health.status is the best judgement call in the diff. But the delivered behaviour has no witness: logCollectionStats appears nowhere in test/, and neither does the rescue-provenance field. This PR exists because an instrument rendered a six-day corpus outage as success — shipping its replacement with nothing proving it fires repeats the shape one layer up. One narrow, cheap in-place repair, which is Request Changes rather than Approve+Follow-Up: a deferred witness for this PR's headline behaviour is deferred correctness, not scope transfer.

Peer-Review Opening: The reasoning in this diff is better than the coverage. Your call to fix the render and explicitly NOT health.status is the kind of decision I want cited later — you traced it to mcpHealthcheck accepting only healthy and to ingress + orchestrator gating on service_healthy, and concluded that degrading an empty corpus stops a fresh plane from booting at all. That is a real interlock, found by looking rather than assumed. The ask below is small and it is about proof, not design.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16512; the changed-file list; current dev source of Server.mjs logCollectionStats, HealthService.mjs, QueryService.mjs; the sibling ai/mcp/server/memory-core/Server.mjs:770 implementation of the same method; test/playwright/unit/ai/services/knowledge-base/QueryService.queryDocuments.spec.mjs; and the live incident this cites, which I verified independently last night at 61,206 documents.
• Expected Solution Shape: An empty corpus must stop rendering as success, and a rescue-only answer must announce itself rather than presenting in a shape identical to a grounded one. The boundary this must NOT hardcode is the container liveness contract — health.status is consumed by orchestration, so an empty corpus must not become "unhealthy". Test isolation should exist for the render decision specifically, since the whole defect class is an instrument that reported the wrong thing.
• Patch Verdict: Matches on design, incomplete on proof. The three-way render (unreadable / 0 / populated) is correct and the typeof count !== 'number' branch is the right third state — an unreadable count is not a zero. The rescue-provenance capture is placed correctly, before the rescue runs, and your comment names exactly why: after the fact, added and boosted sources are indistinguishable. What is missing is any assertion that any of it happens.
• Premise Coherence: Coheres strongly with verify-before-assert. The ticket's framing — "the count was never missing, it was framed as success" — is the same lesson three of us hit from different directions this session. This is that lesson applied to a startup banner.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16512
• Related Graph Nodes: Refs #16510 (per-collection verdict), #16516 (export-side completeness), #16491
• Origin Session ID: c724a85f-2d37-44ac-9a33-12dcce415aa2

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: the instrument that replaces a lying instrument has no proof it fires.

Measured, with a positive control so the absence is not my grep failing:

grep -rn "logCollectionStats" ai/ test/
  ai/mcp/server/memory-core/Server.mjs:770       <- the sibling implementation
  ai/mcp/server/knowledge-base/Server.mjs:168    <- this one
  (zero hits under test/)

The search finds the method twice in ai/, so it works; test/ genuinely has none. Same for the new provenance field — vectorSourceCount, rescueOnly and provenance appear nowhere in QueryService.queryDocuments.spec.mjs.

Why this one matters more than a typical coverage gap: the failure mode is silent and directional. If someone later "tidies" logger.warn back to logger.info, or the health payload's count shape changes so typeof count !== 'number' starts swallowing a real number, nothing goes red — and the symptom is once again a corpus outage that reads as success. That is precisely the six-day invisibility this PR is written to end.

It is also load-bearing beyond this PR: this render is the instrument an operator will read to answer "did the corpus survive the container update". An unproven instrument is a poor thing to answer that question with.

Second, non-blocking: HealthService.mjs is +4/-4 pure alignment and a large share of QueryService.mjs's +59/-29 is block-alignment churn. It is legitimate, but it buries a real behavioural change in cosmetic diff. Worth splitting next time so a reviewer's eye lands on the four lines that matter.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, and the Evidence: line is first-hand incident measurement rather than inference.
• Anchor & Echo summaries: the in-source comment naming mcpHealthcheck and the service_healthy gate is specific and checkable, not gestural. The cwd spacing note is unusually good — it explains a non-obvious constraint a future formatter would otherwise "fix".
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: the incident cited (a recreated collection with ~61,206 documents left in the prior data root) matches what I verified independently on this plane.

Findings: Pass — no drift. The prose is accurate; the gap is that some of it is unwitnessed.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The cwd alignment constraint is a real one — AI_CONFIG_MODULE_SCOPE_BASELINE in lint-config-template-ssot.mjs fingerprints exact spacing, so widening that declaration reads to the lint as both a removed baseline capture and a new capture, failing twice for whitespace. Documenting it in-source is the right call; a lint whose failure mode is "you aligned a const" is worth its own friction ticket eventually.
• [RETROSPECTIVE]: Refusing to route a new signal through an existing consumed field is the reusable move here. health.status was the obvious place to express "the corpus is empty", and it was the wrong one because orchestration reads it as liveness. Same shape as restorable on #16510 — a field whose dominant consumer asks a different question than the one you want to answer. Two independent instances in one session suggests the general rule: before expressing a new fact through an existing field, name that field's dominant consumer and the question it is actually asking.

🎯 Close-Target Audit

• Close-targets identified: #16512 only, newline-isolated Resolves #16512 on line 1.
• For each #N: confirmed not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• The response shape gains an aggregate provenance field alongside the existing per-row lexicalRescueReasons, and the per-row surface is unchanged.
• health.status is deliberately unchanged — the consumed contract orchestration depends on is preserved, and the reason is documented at the site.

Findings: Pass on contract. The addition is additive and the field that had consumers was left alone.

🪜 Evidence Audit

• PR body contains an Evidence: line.
• Achieved evidence does not cover the delivered behaviour. The evidence describes the incident that motivated the change, not that the change behaves as claimed. Those are different objects, and the first cannot stand in for the second.

Findings: Evidence-AC mismatch — see Required Actions.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

Findings: N/A — no skill file, convention, or AGENTS.md change.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 8381a64590508e75fe7fd139c1b7f6d9c1877ba2.
• Author per-surface coverage: obvious omission flagged. Both changed behaviours are unwitnessed, verified with a positive control above.
• Test location: N/A — no tests added or moved.

Findings: Author evidence gap. Green CI here means "nothing else broke", which is true and not the question.

📋 Required Actions

To proceed with merging, please address the following:

• Witness the three-state render decision: an unreadable count warns as unverified, 0 warns and does not render under the success framing, and a positive count still renders as info. The positive case is the control — without it, "always warn" passes the other two. If Server is awkward to instantiate in a unit spec, the idiomatic route is the one you already used on #16491: lift the decision into a small pure helper (describeCollectionStats(knowledgeBase) -> {level, lines}) and test that directly, leaving the class a thin caller. unit-test.md §4 prescribes exactly this for connect-on-init singletons.
• Witness that a rescue-only answer is distinguishable from a grounded one — one assertion that a response assembled with zero vector sources carries the new provenance, and one that a vector-backed response does not falsely claim it. vectorSourceCount is captured before the rescue precisely so this is expressible; the assertion is what makes it a guarantee rather than an intention.

Both are narrow. I am not asking for the surrounding surfaces to be back-filled — only the behaviour this PR delivers.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - The correction is placed at the render rather than in the health contract, with the consumer interlock traced at source. 5 deducted because the behavioural change is interleaved with alignment churn across two files, which costs reviewability.
• [CONTENT_COMPLETENESS]: 90 - In-source comments are specific and checkable, including the non-obvious lint-spacing constraint. 10 deducted: the PR body's Evidence: line documents the motivating incident rather than the delivered behaviour, which is the same gap the Required Actions name.
• [EXECUTION_QUALITY]: 70 - Code correct on inspection, including the third state for an unreadable count. Deducted for the absent witness on both delivered behaviours: the regression path (warn silently reverting to info) is invisible to CI, and this is the PR where that specific invisibility is the subject.
• [PRODUCTIVITY]: 85 - The ticket's defect is resolved and the harder judgement call (not degrading health.status) is made correctly and documented.
• [IMPACT]: 75 - Turns a silent corpus outage into a loud one on every startup. Rated below the top band only because it changes reporting rather than recovery — but it is the instrument an operator will read to confirm a corpus survived a container update, which is why the witness matters.
• [COMPLEXITY]: 35 - Small behavioural surface; most of the line count is alignment.
• [EFFORT_PROFILE]: Quick Win - Few substantive lines against a defect that hid an outage for six days.

To be explicit about the bar: I am not holding this for style, and the design needs no changes. The two assertions are the difference between "this instrument reports correctly" and "this instrument reported correctly once, when a human looked."

[review-budget-managed]

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