PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The core direction is right: keep the re-ranker non-throwing while making a corrupt query path observable. The current patch is not merge-ready because the PR changes public MCP tool-response behavior without updating the OpenAPI contract, the regression coverage stops at the marker layer rather than the tool/healthcheck surfaces named by #12628, and current CI is non-green (unit failed at f71dd65f8; integration-unified was still pending when reviewed).

Peer-Review Opening: I reviewed this as the requested cross-family reviewer. The StorageRouter marker itself is a good local shape; the required changes are around consumed contract completeness and AC-level proof, not the premise of surfacing the swallowed query failure.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12628 body and Contract Ledger; PR #12629 changed-file list and commit log; current dev/PR branch OpenAPI schemas for query_raw_memories, query_summaries, and healthcheck; StorageRouter, SummaryService, MemoryService, HealthService; local OpenApiValidatorCompliance.spec; focused unit-test skill guidance.
• Expected Solution Shape: A correct fix should preserve the non-throwing Pass-1 fallback but stamp a degraded marker, then surface that marker through query_raw_memories, query_summaries, and healthcheck as a documented, test-covered public tool contract. It must not collapse into a silent {count:0} and must not leave agents/tool clients learning the old response shape from OpenAPI. Test isolation should cover both the marker source and the consumed service/tool envelope without requiring a live corrupt Chroma collection.
• Patch Verdict: Partial match. The marker source matches the expected shape and the focused StorageRouter unit spec passes locally. The patch contradicts the expected consumed-surface shape because openapi.yaml still omits the new degraded envelope and queryCanary fields, while tests do not cover MemoryService/SummaryService degraded envelopes or the healthcheck canary result.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12628
• Related Graph Nodes: #12450 (parent corruption/observability), #9959 (origin Pass-1 swallow context), Memory Core query/healthcheck MCP tool contract

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The PR treats AC2/AC3 as thin consumers of the marker, but those consumers are exactly the public surfaces agents will observe. If the tool contract and tests stay marker-only, a future agent can still learn from openapi.yaml that the only successful query shape is {query,count,results}, and the PR can close #12628 without proving the actual QUERY_PATH_DEGRADED / healthcheck behavior the ticket asked for.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the mechanical implementation, but the "No blocking residual" / "full substrate-level integration is bucket-C-gated" framing overstates the proof. AC2 and AC3 are code paths in this PR, not only live-corrupt-DB validation.
• Anchor & Echo summaries: source comments and JSDoc are precise and explain the degraded-marker contract.
• [RETROSPECTIVE] tag: N/A, none present.
• Linked anchors: #12628 and #12450 are legitimate anchors.

Findings: Drift flagged above; captured as Required Actions because it affects close-target completeness.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: CI is non-green at the reviewed head (unit failed; integration-unified still pending). Also, OpenApiValidatorCompliance passes because output schemas are lenient for extra fields, but it does not catch that the new degraded response contract is undocumented.
• [RETROSPECTIVE]: For Memory Core MCP surfaces, making runtime degradation observable is only half the fix; the OpenAPI/tool contract must teach agents the degraded envelope, or future consumers will continue reasoning from the old clean-empty shape.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #12628
• #12628 confirmed not epic-labeled (bug, ai, architecture, model-experience)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly (no drift).

Findings: Contract drift flagged. #12628 explicitly names ai/mcp/server/memory-core/openapi.yaml as Docs for the query_summaries / query_raw_memories degraded envelope and the healthcheck collection report. This PR changes MemoryService.queryMemories() and SummaryService.querySummaries() to return degraded, code, collection, signature, and message, but QueryMemoriesResponse at ai/mcp/server/memory-core/openapi.yaml:1972 and QuerySummariesResponse at ai/mcp/server/memory-core/openapi.yaml:2131 still only declare _channelSeparation, query, count, and results. HealthService now injects database.connection.collections.queryCanary, but the HealthCheckResponse collections schema still declares only memories and summaries.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section.
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed].
• Two-ceiling distinction: PR body does name post-merge validation for the live corrupt Memory Core.
• Evidence-class collapse check: review language does not promote marker-only L2 evidence into full AC2/AC3 coverage.

Findings: Evidence mismatch flagged. Local marker coverage is valuable, but #12628 AC4 asks that query_summaries / query_raw_memories surface degraded instead of silent {count:0}, and that healthcheck reflects degraded for that collection. Current tests cover StorageRouter.injectQueryReRanker() only; they do not assert the MemoryService/SummaryService envelope or the HealthService queryCanary behavior.

📡 MCP-Tool-Description Budget Audit

Findings: N/A for budget: the PR does not touch OpenAPI descriptions. The absence of OpenAPI edits is handled under Contract Completeness, not as a description-bloat issue.

🔗 Cross-Skill Integration Audit

• Existing skills do not need a predecessor trigger for this runtime observability bug.
• AGENTS_STARTUP.md does not need an update.
• No new MCP tool is added.
• Existing MCP tool response contracts must be updated for the new degraded envelope / healthcheck field.

Findings: Integration gap is the same public MCP contract drift captured in Required Actions.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request.
• Canonical Location: new test file is under test/playwright/unit/ai/services/memory-core/, appropriate for Memory Core service logic.
• Ran the specific new test file: npm run test-unit -- test/playwright/unit/ai/services/memory-core/StorageRouterDegraded.spec.mjs → 3/3 passed locally.
• Ran related OpenAPI validator suite: npm run test-unit -- test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs → 26/26 passed locally, but it is leniency/shape compliance, not documentation of the new fields.
• Current GitHub CI: unit check failed at f71dd65f8; integration-unified was still in progress when reviewed.

Findings: Local focused tests pass; CI is not green and the tests do not yet cover the consumed degraded surfaces.

📋 Required Actions

To proceed with merging, please address the following:

• Update ai/mcp/server/memory-core/openapi.yaml so query_raw_memories, query_summaries, and healthcheck document the new degraded response contract: the query degraded envelope fields and database.connection.collections.queryCanary shape. If you intentionally want these fields to remain undocumented extra properties, update #12628's Contract Ledger and PR rationale to say that explicitly; otherwise the contract is currently drifted.
• Add regression coverage for the consumed surfaces, not only the marker source: a _degraded collection query should produce the QUERY_PATH_DEGRADED envelope from MemoryService.queryMemories() / SummaryService.querySummaries(), and the healthcheck query-canary path should mark the collection/status degraded. If any part is truly bucket-C/live-corrupt-DB-only, move that residual into the issue/PR explicitly instead of closing #12628 as fully delivered.
• Resolve the failing GitHub unit check at head f71dd65f83a376734702c1e8e6143df505808e0f and re-request review once CI is green. The run was still in progress when I tried to fetch logs, so the failed job log was not yet available.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the non-throwing degraded marker aligns with the StorageRouter resilience boundary, but the public MCP response contract was not updated alongside the consumed behavior.
• [CONTENT_COMPLETENESS]: 65 - 35 points deducted because the code comments/JSDoc are strong, but OpenAPI contract docs and PR evidence framing do not yet match the shipped tool-response fields.
• [EXECUTION_QUALITY]: 58 - 42 points deducted because focused marker tests pass locally, but GitHub unit is failing and AC2/AC3 consumer-path tests are missing.
• [PRODUCTIVITY]: 70 - 30 points deducted because AC1 is delivered and AC2/AC3 are implemented in code, but #12628 cannot honestly close until the contract and evidence gaps are repaired.
• [IMPACT]: 80 - 20 points deducted because this is major Memory Core observability for a real silent-failure class, but it is not a new subsystem or foundational architecture change.
• [COMPLEXITY]: 68 - Medium-high: four service files plus MCP response-contract implications and healthcheck caching; the implementation is localized but crosses runtime, tooling, and observability surfaces.
• [EFFORT_PROFILE]: Heavy Lift - High-impact Memory Core observability with cross-surface contract/test requirements, even though the code delta is compact.

The marker contract is a solid start. The PR needs the public contract and AC-level test proof before it can safely close #12628.

the concept to detect failures is GOOD. however, it does not rectify BLOATING the already bloated healthcheck even more. increasing tool shapes, increasing token use, corrupting agent focus window.

the goal of a healthcheck is pretty much "is it healthy?" yes or no. adding messages already questionable. returning a book => NO. your context and focus windows are not unlimited. a SCRIPT to optionally trigger and inspect => fine.