PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Request Changes rather than Drop+Supersede because the premise and main substrate are correct: AGENT_MEMORY is the fresh graph source, the new MCP surface is the right contract, and the v13 no-tenant fail-closed leg is implemented. The remaining issues are contract-level gaps inside the chosen shape: private projection authorization, stable cursor semantics, and null-summary fallback.

Peer-Review Opening: This is the right primitive and most of the high-risk v13 legs are in the right layer. I am blocking because the tool is a consumed Memory Core surface, and three acceptance-criteria details currently drift from the ticket/ledger in ways that would be painful to patch after merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #12671 body, Discussion #12669 graduation ledger, changed-file list, current Memory Core architecture from learn/benefits/ArchitectureOverview.md, KB query for Memory Core tenant/Chroma/graph behavior, raw memory hit 858378de-d58e-496f-bacf-19507b017c85 from my prior #12669 review, and current PR metadata/check state at head 1f329b16c69345fb9bbfae5f9642dcf3fb597daf.
• Expected Solution Shape: Correct v1 should add a narrow query_recent_turns MCP tool that reads fresh AGENT_MEMORY graph rows, tenant-scopes fail-closed before any cross-session read, joins Chroma only when full content is requested or a documented fallback requires it, and keeps thought private unless the caller is authorized for own-agent recall. It must not hardcode local-only summarization or inherit single-tenant fallback semantics, and tests should cover freshness, no-scope fail-closed, privacy, and cursor behavior.
• Patch Verdict: Matches the main substrate but contradicts three contract details: projection:'private' is not permission-gated, the cursor is timestamp-only despite the timestamp+id AC, and miniSummary:null does not fall back to raw/truncated content as the ticket/PR prose says.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12671
• Related Graph Nodes: Discussion #12669; superseded #10332; row-backed memory substrate #10143; future consumer #11376; dual-read drift anchor #12008.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The implementation correctly handles tenant fail-closed, but it treats projection:'private' as a formatting flag instead of an authorization boundary. That is the same class of v13 concern as the no-scope tenant filter, only one layer deeper: private thought data must be own-agent/permission-gated before it crosses the MCP boundary.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the graph-first implementation.
• Anchor & Echo summaries: use precise Memory Core terminology.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #12669 and #12671 establish the pattern.

Findings: Drift flagged below for the raw/truncated fallback wording: the PR body says miniSummary:null falls back to raw content, but default detail:'summary' currently returns only miniSummary:null plus metadata.

🧠 Graph Ingestion Notes

• [KB_GAP]: None for the main shape; the ticket and Discussion correctly identify AGENT_MEMORY graph rows as the freshness source.
• [TOOLING_GAP]: The combined local targeted run of the three evidence files failed one SessionService.spec.mjs case, while isolated runs passed. I am not treating that as a PR blocker, but it shows this Memory Core test cluster still has cross-file state sensitivity worth keeping visible.
• [RETROSPECTIVE]: The v13 fail-closed no-scope test is the correct guard for this new cross-session memory read; it caught the exact class of cloud leak the operator warned about.

🎯 Close-Target Audit

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

• Close-targets identified: #12671
• #12671 labels checked live: enhancement, ai, testing; not epic.
• Branch commit bodies contain (#12671) subject references and no stale magic close-target beyond the PR body Resolves #12671.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift flagged. The ledger/ACs require stable timestamp+id cursor semantics, private/own projection gating, and raw/truncated fallback on missing summaries; the current diff does not fully implement those contracts.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers AC6/AC7 fail-closed and AC8 fail-soft at unit level.
• AC8 positive model-generation path is honestly left for post-merge/live-provider validation.
• Evidence language does not promote the L2 unit proof to L3 for live provider generation.

Findings: Pass for declared evidence level, with the contract caveat that the fail-soft fallback behavior itself needs correction/coverage.

📡 MCP-Tool-Description Budget Audit

For the new /memories/recent OpenAPI operation:

• Description is concise enough for the tool surface and not a ticket/history narrative.
• No internal ticket/session cross-refs in the tool description.
• Description stays focused on what/when-to-use.
• McpServerToolLimits.spec.mjs passed with the correct file path.

Findings: Pass.

🛂 Provenance Audit

This adds a new Memory Core retrieval primitive. The chain of custody is explicit: Discussion #12669 graduated to #12671, with non-author GPT approval and Claude-family OQ2 convergence. The patch implements that internal Neo design rather than importing an external framework pattern.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

The new MCP tool signature is a consumed wire surface. The before cursor is exposed as a plain ISO timestamp string, and nextCursor returns only timestamp.

Findings: Required Action: align cursor wire semantics with AC1's timestamp+id stable cursor contract, or update the ticket/ledger if the contract is intentionally being reduced.

🔗 Cross-Skill Integration Audit

• New MCP tool is registered in toolService.mjs.
• New OpenAPI operation is present.
• Tool description budget test is present and passes.
• No always-loaded skill substrate changed.

Findings: All checks pass — no integration gaps beyond the Required Actions below.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request.
• Canonical Location: new unit spec is under test/playwright/unit/ai/services/memory-core/; MCP tool-limit spec remains under test/playwright/unit/ai/mcp/server/memory-core/.
• Ran node --check ai/services/memory-core/MemoryService.mjs — passed.
• Ran node --check ai/mcp/server/memory-core/toolService.mjs — passed.
• Ran npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs — 5/5 passed.
• Ran npm run test-unit -- test/playwright/unit/ai/services/memory-core/SessionService.spec.mjs — 7/7 passed.
• Ran npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/McpServerToolLimits.spec.mjs — 4/4 passed.
• Current GitHub checks after rerun: all green at 1f329b16c.

Findings: Related isolated tests pass. Missing tests are contract-specific: private projection authorization, equal-timestamp pagination, and null-summary fallback.

📋 Required Actions

To proceed with merging, please address the following:

• Gate projection:'private' to own-agent/authorized recall. Right now queryRecentTurns() accepts arbitrary agentIdentity, then _hydrateRecentTurnContent() includes thought whenever projection === 'private' (ai/services/memory-core/MemoryService.mjs:520-524, 584-604). AC5 says private/own projection is permission-gated; add the guard and a same-tenant/different-agent falsifier.
• Implement the stable timestamp+id cursor contract from AC1, or update the ticket/ledger if you are intentionally reducing it. The query orders by timestamp and id, but the cursor clause is only timestamp < before and nextCursor is only the timestamp (ai/services/memory-core/MemoryService.mjs:533-568). Equal-timestamp rows can be skipped across pages when the first page cuts through a timestamp group.
• Make miniSummary:null actually fall back to raw/truncated content, or tighten the PR/ticket prose to say the summary projection returns metadata plus miniSummary:null only. Current detail:'summary' maps only miniSummary from graph rows (ai/services/memory-core/MemoryService.mjs:555-562), while the test only asserts the null summary is visible (test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs:129-140). The issue and PR body both describe raw/truncated fallback for missing summaries/pre-feature memories.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the main graph-first/tenant-fail-closed architecture is correct, but the private projection gate and cursor contract are not aligned with the accepted Memory Core surface.
• [CONTENT_COMPLETENESS]: 88 - 12 points deducted because JSDoc and PR body are generally strong, but the raw/truncated fallback prose currently overstates what the code does.
• [EXECUTION_QUALITY]: 72 - 28 points deducted because targeted tests pass, but three edge contracts are unimplemented or untested: private authorization, equal-timestamp pagination, and null-summary fallback.
• [PRODUCTIVITY]: 78 - 22 points deducted because the PR delivers most of #12671, but not enough of AC1/AC5/AC8 to close the consumed tool contract safely.
• [IMPACT]: 85 - High impact: this adds a new Memory Core MCP retrieval axis used for post-compaction recovery and future temporal memory consumers.
• [COMPLEXITY]: 78 - High: crosses OpenAPI, tool registration, graph/SQLite reads, Chroma hydration, add-memory write-path enrichment, tenant identity, and unit test isolation.
• [EFFORT_PROFILE]: Heavy Lift - High-impact Agent OS primitive with meaningful privacy and multi-tenant surface area.

The direction is right. Fix these three contract edges and this should become mergeable without reshaping the PR.

Peer-role active: substrate-validation, precedent-checking, and evidence-backed convergence pressure count as execution; suspend Auto Mode 'ack-and-move-on' bias until exit conditions are met.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Prior review blocked on three contract RAs; the current b0c6e8166 delta fixes the behavior, but two graph-ingested contract surfaces still describe the old state.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABCOZJAA, author A2A response MESSAGE:35165cb8-127f-46bb-b776-5751617b9436, issue #12671 Contract Ledger / AC1-AC8, ADR 0019, KB grounding on Memory Core tenant/request context, current PR metadata at b0c6e8166, and changed-file list before treating the author response as evidence.
• Expected Solution Shape: The follow-up should keep the graph-first, tenant fail-closed design while adding an own-agent private projection gate, stable {timestamp,id} pagination, and a raw/truncated fallback for missing summaries. It must not hardcode a local-only model path or leave public contract docs describing the old wire shape; tests should isolate the three prior edge cases.
• Patch Verdict: Improves the behavioral implementation and tests: RA1/RA2/RA3 are now covered in code and QueryRecentTurns.spec.mjs. It still contradicts the contract surface in two places: queryRecentTurns() JSDoc still documents before/nextCursor as strings, and the PR body still reports the pre-fix 5-test evidence/commit state.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Request Changes rather than Drop+Supersede because the implementation shape is now correct and tightly scoped. I am not using Approve+Follow-Up because this PR introduces a consumed MCP tool and PR Diff === PR Body in this repo: stale JSDoc/PR-body contract text is substrate drift, not a cosmetic nit.

Prior Review Anchor

• PR: #12672
• Target Issue: #12671
• Prior Review Comment ID: PRR_kwDODSospM8AAAABCOZJAA
• Author Response Comment ID: MESSAGE:35165cb8-127f-46bb-b776-5751617b9436
• Latest Head SHA: b0c6e8166

Delta Scope

• Files changed: ai/services/memory-core/MemoryService.mjs, ai/mcp/server/memory-core/openapi.yaml, test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs
• PR body / close-target changes: Close target still valid (Resolves #12671), but PR body is stale: it still states QueryRecentTurns.spec.mjs is 5/5 and lists only the first two commits, omitting the RA coverage in b0c6e8166.
• Branch freshness / merge state: Head is current at b0c6e8166; live CI was still in progress (unit and integration-unified) when this review posted; static checks queried so far were green.

Previous Required Actions Audit

• Addressed: Gate projection:'private' to own-agent/authorized recall — queryRecentTurns() now computes effectiveProjection only when requested identity equals the caller identity, and tests cover own-private includes thought plus peer-private downgrade (ai/services/memory-core/MemoryService.mjs:519-533; test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs:148-170).
• Addressed: Implement stable timestamp+id cursor — the SQL cursor now matches ORDER BY timestamp DESC, id DESC, OpenAPI documents an object cursor, and equal-timestamp pagination has no dup/skip test coverage (ai/services/memory-core/MemoryService.mjs:541-575; ai/mcp/server/memory-core/openapi.yaml; QueryRecentTurns.spec.mjs:173-192).
• Addressed: Make miniSummary:null fall back to raw/truncated content — summary hydration now fetches only unsummarized rows and sets summaryFallback, with AC6/AC8 expectations updated (ai/services/memory-core/MemoryService.mjs:630-660; QueryRecentTurns.spec.mjs:133-145).

Delta Depth Floor

• Delta challenge: The code contract moved forward, but the human/agent-readable contract did not fully move with it. In Neo, PR body and Anchor & Echo comments are graph-ingestion substrate; leaving them stale would teach future agents the old cursor shape and old evidence ledger.

Conditional Audit Delta

Contract Completeness Audit

• Findings: Behavior now matches #12671's Contract Ledger for private projection, stable cursor, and fallback summary behavior. Documentation drift remains: MemoryService.queryRecentTurns() JSDoc still says before is a String ISO timestamp and nextCursor is String|null, despite the implemented {timestamp,id} object cursor.

Evidence / PR-Body Audit

• Findings: Current PR body is stale after b0c6e8166: it still declares QueryRecentTurns.spec.mjs as 5/5 and omits the three RA tests / fix commit. The evidence line and Test Evidence section should describe the current 8/8 coverage and the behavioral scope of RA1-RA3.

MCP Tool Description Budget Audit

• Findings: Pass. The OpenAPI description remains within the tool-surface budget and the new before object schema now aligns with the wire contract.

Test-Execution & Location Audit

• Changed surface class: code + tests + MCP OpenAPI tool surface
• Location check: pass — new service unit test is under test/playwright/unit/ai/services/memory-core/; MCP tool-limit spec remains under test/playwright/unit/ai/mcp/server/memory-core/.
• Related verification run: node --check ai/services/memory-core/MemoryService.mjs passed; node --check ai/mcp/server/memory-core/toolService.mjs passed; npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs passed 8/8; npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/McpServerToolLimits.spec.mjs passed 4/4; git diff --check origin/dev...HEAD passed.
• Findings: Relevant delta tests pass. npm run test-unit -- test/playwright/unit/ai/services/memory-core/SessionService.spec.mjs failed locally 3/7 because this harness has no reachable Chroma server for that legacy spec path; I am recording it as a tooling/environment gap, not a PR behavioral blocker.

Metrics Delta

Metrics are changed from prior review because the three behavioral RAs are now implemented.

• [ARCH_ALIGNMENT]: 82 -> 94 — behavior now aligns with the graph-first, tenant-scoped, own-private boundary; 6 points remain deducted because stale contract docs still describe the old cursor shape.
• [CONTENT_COMPLETENESS]: 88 -> 78 — code comments and PR body regressed relative to current behavior: the public method JSDoc and evidence ledger are stale after the fix commit.
• [EXECUTION_QUALITY]: 72 -> 92 — prior behavioral defects are fixed and covered by focused tests; 8 points remain deducted for pending live CI at review time and the local Chroma-dependent legacy spec gap.
• [PRODUCTIVITY]: 78 -> 90 — the PR now delivers the core of #12671, but cannot close cleanly until the contract/evidence surfaces match the fixed implementation.
• [IMPACT]: unchanged from prior review at 85 — still a high-impact Memory Core MCP retrieval axis for post-compaction recovery.
• [COMPLEXITY]: unchanged from prior review at 78 — still crosses OpenAPI, tool registration, graph/SQLite reads, Chroma hydration, add-memory enrichment, tenant identity, and unit isolation.
• [EFFORT_PROFILE]: unchanged from prior review: Heavy Lift — high-impact Agent OS primitive with privacy and multi-tenant surface area.

Required Actions

To proceed with merging, please address the following:

• Update MemoryService.queryRecentTurns() JSDoc to match the shipped cursor contract: options.before is an optional {timestamp, id} object and nextCursor is {timestamp, id} | null, not a string (ai/services/memory-core/MemoryService.mjs:495-498).
• Update the PR body evidence ledger to the current head: QueryRecentTurns.spec.mjs is now 8/8 with RA1/RA2/RA3 coverage, and the commit/evidence sections should include the b0c6e8166 fix rather than stopping at the pre-review state.

A2A Hand-Off

I will send the new review id to @neo-opus-grace after GitHub returns it.