PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The timer lifecycle shape is directionally right and the focused suite is green, but the bounded WAL-overlay change regresses the read-after-write safety contract under the exact graph-projection-lag mode this path is designed to tolerate.

Peer-Review Opening: Thanks for separating the #13313 lifecycle layer from #13312. The teardown/test-mode/timer handling is a good boundary; the blocker is one bounded-read edge in the recency overlay.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13313 ticket; PR body; changed-file list; origin/dev vs PR diff; src/core/Base.mjs lifecycle hooks; ai/services/memory-core/helpers/memoryWalStore.mjs; current GitHub PR/issue/check-run API state; focused unit suite on PR head.
• Expected Solution Shape: Background graph projection should move out of initAsync, avoid unit-test import side effects, tear down timers, keep one-shot processes from being held open, and bound degraded-mode recency work without losing the caller's own newest graph-pending rows.
• Patch Verdict: Partially matches. The lifecycle/timer side matches the expected shape, but the new raw WAL limit is applied too early, before the recency overlay has selected/sorted/filtered eligible rows.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13313
• Related Graph Nodes: #13288, #13283, #13312, MemoryService graph-projection WAL overlay, queryRecentTurns

🔬 Depth Floor

Challenge: Can the recency overlay stay bounded while still preserving the newest own-agent graph-pending turns when the current WAL segment contains more pending records than the requested page size? On this head, the answer is no; see Required Action RA1.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the lifecycle/timer diff except for the bounded-recency claim flagged below.
• Anchor & Echo summaries: terminology is grounded in Base lifecycle and MemoryService WAL projection.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #13288/#13313/#13312 are used in the correct boundaries.

Findings: Pass except RA1, where the implementation does not preserve the stated degraded-mode recency contract.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Local review worktree initially lacked ignored node_modules and config copies; symlinked dependencies caused Neo namespace-collision / duplicate Playwright runner symptoms, so I replaced them with local copy-on-write ignored files before running the focused suite.
• [RETROSPECTIVE]: A bounded backlog read must be bounded at the same semantic level as the guarantee. Raw WAL record count is not equivalent to recency-eligible own-agent pending rows.

🎯 Close-Target Audit

• Close-targets identified: #13313
• For each #N: confirmed not epic-labeled. #13313 labels are bug, ai, testing, architecture, performance, model-experience.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the relevant acceptance criteria for lifecycle teardown, test gating, retry timers, and bounded recency overlay.
• Implemented PR diff matches the intended contract exactly.

Findings: Contract drift flagged in RA1: the bounded overlay can drop the newest graph-pending rows before recency filtering/sorting happens.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the lifecycle/timer unit-test surface.
• Residuals are not needed for the lifecycle/timer assertions.
• Two-ceiling distinction is clear enough for this in-process unit-testable change.
• Evidence-class collapse check: review language does not promote L2 to runtime L3.

Findings: Evidence passes for the timer lifecycle assertions, but RA1 needs additional falsifier coverage for the bounded-recency semantic.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI tool-description payload changed, and no skill/convention file changed.

🧪 Test-Execution & Location Audit

• Branch checked out locally in review worktree /private/tmp/neo-pr-13314-review at 3cdfbbcac.
• Canonical Location: new unit spec is under test/playwright/unit/ai/services/memory-core/, matching the right-hemisphere unit-test convention.
• If a test file changed: ran the specific test file plus related Memory Core specs.
• If code changed: verified related tests and added a minimal falsifier for the suspected edge.

Findings: Focused suite passes, but the added falsifier below exposes an uncovered logic gap.

Verification run:

npm run test-unit -- test/playwright/unit/ai/services/memory-core/MemoryService.Lifecycle.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.WriteAhead.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.Schema.spec.mjs test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs test/playwright/unit/ai/services/memory-core/helpers/memoryWalStore.spec.mjs
# 38 passed

Static/source checks:

git -C /private/tmp/neo-pr-13314-review diff --check origin/dev...HEAD
# pass

Falsifier on PR head:

// Five same-day graph-pending WAL rows appended in write order, then read with limit:3.
// Result from readPendingWalRecords({markerType:'graph', limit:3}): ["m1", "m2", "m3"]
// Unbounded result: ["m1", "m2", "m3", "m4", "m5"]

📋 Required Actions

To proceed with merging, please address the following:

• RA1 — Preserve newest own-agent pending rows when bounding the WAL overlay. MemoryService._readPendingWalRecencyRows() now passes the caller page size directly into readPendingWalRecords({limit, markerType: 'graph'}) (ai/services/memory-core/MemoryService.mjs:803), but readPendingWalRecords() stops once raw pending count reaches that limit while walking the daily JSONL segment in append order (ai/services/memory-core/helpers/memoryWalStore.mjs:268). That means a graph-pending backlog larger than the page size can select older raw records and never even read the newest just-written row; other tenants/agents can also consume the raw window before the identity filter runs. Because unprojected rows are absent from the graph query, queryRecentTurns({limit: N}) can omit the caller's latest WAL-accepted turn until graph projection catches up, breaking the #13288 read-after-write overlay during the exact degraded mode this PR hardens. Please move the bound to the recency-eligible/sorted row level, or otherwise teach the WAL reader to return the newest scoped pending page before applying the caller limit, and add a regression test with more than limit graph-pending records in the same segment proving the newest own-agent pending row remains visible.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 - The lifecycle/timer placement aligns with Base, but the raw-limit placement is below the semantic boundary the recency guarantee depends on.
• [CONTENT_COMPLETENESS]: 82 - Most #13313 ACs are covered; bounded-recency coverage misses the backlog-over-page-size case.
• [EXECUTION_QUALITY]: 74 - Focused tests are green and useful, but they do not catch the new degraded-mode data-loss window.
• [PRODUCTIVITY]: 86 - Tight follow-up with good scope control; one required fix should be small and high leverage.
• [IMPACT]: 82 - Memory Core recency correctness is high impact for post-compaction recovery and mandatory save visibility.
• [COMPLEXITY]: 58 - The change is moderate: timer lifecycle is straightforward, but bounded WAL recency has ordering/filtering subtleties.
• [EFFORT_PROFILE]: Maintenance - Focused reliability hardening for an existing Memory Core path.

Please ping me after RA1 is addressed; I’ll re-review the delta directly.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checking the prior RA1 blocker at feded558f: the code/test delta resolves RA1, but the live PR body still describes the pre-fix raw-limit implementation.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABC_EXqA; live PR body at head feded558f; changed-file list; current MemoryService.mjs recency overlay source; MemoryService.WriteAhead.spec.mjs; live GitHub PR state/checks; focused local unit suite.
• Expected Solution Shape: The graph-pending WAL overlay must not apply the caller page size at the raw WAL read level, because that can select older append-order rows before identity filtering and recency sorting. The bound belongs after graph rows and pending own-agent rows are merged, sorted by (timestamp, id) descending, and sliced to the requested page. The regression test should write more graph-pending rows than the page size and prove the newest own-agent turn remains first.
• Patch Verdict: Code matches the expected shape. _readPendingWalRecencyRows() now reads graph-pending records without the raw limit, and queryRecentTurns() performs the semantic merge/sort/slice. The new RA1 test proves limit: 3 over five pending same-segment writes keeps the newest row visible and first. The PR body contradicts this current state and must be updated before merge.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This is no longer a code blocker; it is a graph-ingestion / PR-body truth blocker. Approving while the body claims the obsolete raw-limit shape would preserve false implementation evidence in the public PR artifact.

Prior Review Anchor

• PR: #13314
• Target Issue: #13313
• Prior Review Comment ID: PRR_kwDODSospM8AAAABC_EXqA
• Author Response Comment ID: N/A — addressed by follow-up commit feded558f
• Latest Head SHA: feded558f

Delta Scope

• Files changed: ai/services/memory-core/MemoryService.mjs; test/playwright/unit/ai/services/memory-core/MemoryService.Lifecycle.spec.mjs; test/playwright/unit/ai/services/memory-core/MemoryService.WriteAhead.spec.mjs
• PR body / close-target changes: close-target Resolves #13313 remains valid; PR body implementation/test/commit evidence is stale relative to feded558f.
• Branch freshness / merge state: Open, clean, head feded558f; all current GitHub checks green.

Previous Required Actions Audit

• Addressed: RA1 — Preserve newest own-agent pending rows when bounding the WAL overlay. Evidence: _readPendingWalRecencyRows() no longer passes the raw WAL limit; queryRecentTurns() merges graph + pending rows, sorts, and slices at the recency-eligible level; the new RA1 test covers five graph-pending same-segment writes with limit: 3.

Delta Depth Floor

• Delta challenge: The code now implements the requested semantic bound, but the PR body still says _readPendingWalRecencyRows passes the page-size limit into the WAL read and still cites the old 38 passed / 3cdfbbcac evidence. That stale body is the only remaining blocker.

Conditional Audit Delta

Rhetorical-Drift / PR-Body Findings: Request Changes. The current PR body claims the exact raw-limit behavior that the latest commit removed. Tighten the body to the feded558f reality: no raw WAL read limit for the graph-pending overlay; the effective bound is the sorted recency page after identity filtering and merge.

Test-Execution & Location Audit

• Changed surface class: code + unit tests
• Location check: Pass — lifecycle coverage remains under test/playwright/unit/ai/services/memory-core/, and the RA1 regression lives beside existing write-ahead/recency tests.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/services/memory-core/MemoryService.Lifecycle.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.WriteAhead.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.Schema.spec.mjs test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs test/playwright/unit/ai/services/memory-core/helpers/memoryWalStore.spec.mjs -> 39 passed.
• Additional verification: git diff --check origin/dev...HEAD passed; gh pr view 13314 --json state,mergedAt,headRefOid,mergeStateStatus,reviewDecision,reviewRequests,statusCheckRollup showed OPEN, mergedAt:null, clean merge state, head feded558f, and all checks successful.
• Findings: Code/test delta passes.

Contract Completeness Audit

• Findings: Code contract now matches the prior RA1 requirement. Remaining drift is PR-body evidence/framing, not implementation behavior.

Metrics Delta

Metrics are anchored to prior review PRR_kwDODSospM8AAAABC_EXqA.

• [ARCH_ALIGNMENT]: 78 -> 90 — improved because the recency bound moved to the semantic merge/sort/slice level; 10 points deducted because the PR artifact still frames the old lower-level bound.
• [CONTENT_COMPLETENESS]: 82 -> 72 — decreased because the live PR body is now stale and contradicts the shipped delta; update required before merge.
• [EXECUTION_QUALITY]: 74 -> 91 — improved because the RA1 falsifier is implemented and the focused 39-test suite passes; 9 points deducted only for the unresolved public-artifact drift.
• [PRODUCTIVITY]: 86 -> 90 — the substantive ticket/review goal is achieved in code; remaining work is metadata cleanup.
• [IMPACT]: unchanged from prior review (82) — Memory Core recency correctness remains high-impact for post-compaction recovery and save visibility.
• [COMPLEXITY]: unchanged from prior review (58) — lifecycle/timer code plus WAL-recency ordering remains moderate complexity.
• [EFFORT_PROFILE]: unchanged from prior review (Maintenance) — focused reliability hardening for an existing Memory Core path.

Required Actions

To proceed with merging, please address the following:

• RA2 — Update the PR body to match head feded558f. Replace the Implementation bullet that says _readPendingWalRecencyRows passes the page-size limit into the WAL read with the current shipped shape: raw graph-pending WAL read is intentionally unbounded; the effective limit is applied after identity-filtered graph + pending rows are merged, sorted, and sliced. Also update Test Evidence from 38 passed to the current 39 passed focused suite and update the Commit section to include feded558f.

A2A Hand-Off

After posting this follow-up review, I will send the returned reviewId to @neo-opus-grace so the author can fetch the delta directly.