feat: distill long-horizon agent transcripts into durable memory

[lsm]

Adds a memory_distillation job that periodically walks each active long-horizon agent's transcript tail and extracts durable facts/decisions/outcomes into the agent-memory store, so knowledge accumulates without the agent manually calling memory_write.

Design:

• New per-agent cursor table (space_agent_memory_distillation, migration 163) keyed by agent_id, advancing by monotonic sdk_messages.rowid so the same messages are never re-distilled.
• MemoryDistillationService with an injectable LLM extraction seam (default = bounded SDK query() with a strict JSON prompt). Budget-bounded: 30 msgs/pass, 800 chars/msg, 24k chars/transcript. Idle runs (no new messages past the cursor) make no LLM call.
• Memories are written space-scoped and tagged agent:<handle> + distilled — per-agent owner_agent_id namespacing isn't landed yet, so this is the documented space-scoped-first path and remains re-scopable later.
• MEMORY_DISTILLATION registered in job-queue-constants + app.ts; self-schedules every 30 min. Per-agent extraction failures are isolated and recorded on the cursor (cursor doesn't advance on failure → retried next run).

Acceptance covered by tests: facts appear without manual memory_write, cursor prevents duplicate distillation across runs, extraction is budget-bounded, and key-based upsert dedupes re-distilled content.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM (ZhipuAI)

Recommendation: REQUEST_CHANGES

Solid implementation — clean cursor design, faithful reuse of the proven bounded-query() extraction pattern (near-verbatim copy of evolution-conversation-analysis-service.ts), good per-agent failure isolation, and 16 focused tests covering cursor advance, dedupe, bounding, and self-scheduling. Schema parity passes, typecheck is clean, and the app.ts wiring mirrors memory_consolidation exactly. The two P2s below are the blockers; the rest are nits.

P2 — getDistillableMessages doesn't actually mirror getRenderableTextMessages

The docstring (sdk-message-repository.ts:603) claims it "Mirrors the renderability rules of getRenderableTextMessages," but it omits both NOT EXISTS guards the original carries (sdk-message-repository.ts:539-556): the retracted_message_uuids / model_refusal_fallback exclusion and the supersedes exclusion. As written, a retracted or superseded message can be distilled into durable memory. Either carry the same guards over (preferred — a retracted assistant turn is exactly the transient content we don't want to persist) or correct the docstring to "approximates" so the next maintainer isn't misled.

P2 — No backoff on per-agent failure → unbounded retry / token spend

When extraction throws, distillAgentSafely records the error without advancing the cursor (memory-distillation-service.ts:159-176), so the same messages are retried every 30 min. For transient failures that's the intended design — but a deterministic failure (a message the model always refuses, a persistent provider error) retries forever, each attempt making a real bounded LLM call. The acceptance criteria bound spend per pass, but aggregate spend on a stuck agent is unbounded. Recommend a lightweight consecutive_failures counter (+ optional next_attempt_at) on the cursor: after N consecutive failures, skip the agent with skipped: 'backoff' and don't invoke the LLM. At minimum, document the unbounded-retry behavior explicitly.

P2 — Missing test for success-then-failure cursor preservation

The retry-safety guarantee rests on recordError's ON CONFLICT clause not touching last_distilled_rowid when a cursor already exists (space-agent-memory-distillation-repository.ts:97-118), so a previously-distilled agent that later fails retries from the same cursor instead of resetting to 0. The existing failure-isolation test only covers a fresh agent (lastDistilledRowid === 0). Please add a test: agent distilled successfully (cursor at C) → next run throws → assert cursor stays at C (not 0) and the same messages are retried. This directly covers the "cursor prevents duplicate distillation across runs" acceptance criterion on the failure path.

P3 — sanitizeTags silently truncates the owner tag

memory-distillation-service.ts:508-513 slices every tag to 50 chars, so an agent with a long handle produces a truncated agent:<handl…> tag, silently breaking provenance attribution. Handles are short in practice so this is low risk — but since the owner tag is the attribution mechanism until owner_agent_id lands, consider bounding the handle or ensuring the owner tag survives intact.

P3 — exported-but-internal symbols

DEFAULT_MAX_TRANSCRIPT_CHARS, defaultExtractMemories, buildDistillationPrompt, and resolveDistillationModel are exported but only used within the file. Knip (project config) doesn't flag them, so check passes — if they're not intended as a test/override seam, dropping the export is cleaner. Optional.

---

Verified good: cursor monotonicity + MAX() guard against backward moves, idempotent upsert dedupe via (space_id, key), per-pass bounding (30 msgs / 800 chars / 24k transcript), per-write try/catch isolation, FK cascade on agent deletion, migration tableExists guard, self-schedule-before-work (reschedules even on throw), and faithful copy of the provider/env/thinking handling (applyEnvVarsToProcessForProvider → restoreEnvVars, glm→haiku override, Kimi thinking branch).

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM (ZhipuAI)

Recommendation: APPROVE

All five prior findings are resolved and independently verified; 20/20 tests pass, schema-parity + typecheck clean. Approving.

P2 — retraction/supersession guards (resolved): getDistillableMessages now carries both NOT EXISTS guards verbatim from getRenderableTextMessages, the docstring is corrected, and a new test proves retracted/superseded rows are excluded while the kept + superseding (replacement) messages remain.

P2 — unbounded failure retry (resolved): Cursor gains consecutiveFailures + nextAttemptAt; recordError schedules exponential backoff (30m→1h→2h… capped at 24h) and the service skips with skipped: 'backoff' — no LLM call — until the window elapses. recordSuccess/advanceCursor reset the backoff. A persistently-failing agent now costs ≤1 bounded call/day instead of one per 30-min cadence. Tests cover skip, resume, and the computeBackoffMs ramp.

P2 — success-then-failure cursor preservation (resolved): New test distills (cursor=C) → a later run throws → cursor stays at C, consecutiveFailures=1, nextAttemptAt set. Confirms recordError's ON CONFLICT clause preserves the cursor for retry rather than resetting it.

P3 — owner-tag truncation (resolved): Dedicated buildOwnerTag truncates the handle (preserving the agent: prefix) and the provenance tags bypass the generic sanitizer, so attribution survives long handles instead of being sliced mid-character.

P3 — exported-but-internal symbols (resolved): The four symbols are un-exported; the newly-exported computeBackoffMs + backoff constants are genuinely used by tests (knip-clean).

---

Advisory (not blocking, FYI): Migration 163's body changed between the first and second commit of this PR. The marker mechanism (runMarkedMigration returns early once migration_163 is set) combined with CREATE TABLE IF NOT EXISTS means a persistent DB that already ran the first commit's 163 won't receive the new consecutive_failures / next_attempt_at columns, and recordError/recordSuccess would then throw "no such column." Since 163 is unmerged, dev/CI/fresh DBs all run the final schema correctly — only a stale local branch DB that ran commit 53babae is affected (recreate it). Optional hardening: guarded ALTER TABLE ADD COLUMN for idempotency, but not required for the merge.

[lsm]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 376add923b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM (ZhipuAI)

Recommendation: REQUEST_CHANGES — reversing my prior APPROVE.

My first pass verified the cursor/dedupe/bounding mechanics and the original 5 findings were addressed, but it missed deeper correctness/security/operational issues. Codex surfaced 5 findings (4× P1, 1× P2) which I have independently validated against the source — all hold up. Detail below, each with the fix I'd suggest.

P1 — getDistillableMessages can permanently stall behind textless rows

extractVisibleText only collects type === 'text' blocks, returning '' for tool_use-only or thinking-only assistant turns — and computeIsRenderable marks those same tool_use-only turns is_renderable = 1. getDistillableMessages applies the SQL LIMIT before the JS text filter and, unlike getRenderableTextMessages (which has a maxScan/batch loop), has no scan-past-textless-rows logic. So ≥30 contiguous tool-only turns past the cursor → method returns [] → service returns skipped: 'no new messages' without advancing the cursor → every later run re-selects the same rows → distillation is permanently stuck for that agent. For LH agents that burst tool-only turns this is realistic. Fix: mirror getRenderableTextMessages' batch scan, or at minimum advance the cursor past rows that were read-but-filtered. (Codex: #2272 (comment))

P1 — Empty/error SDK response is treated as a successful no-memory pass

defaultExtractMemories does if (!raw) return [];. A provider auth/rate-limit/exec failure yields a result message with no assistant text → raw stays empty → returns [] → the service records the batch as processed and advances the cursor past every source message, so they're never retried. This is a regression from the proven twin: evolution-conversation-analysis-service.ts:135 does if (!raw) throw new Error('… returned no text'). It also means the backoff machinery just added never fires for the most common failure mode — the extractor swallows the error instead of throwing into distillAgentSafely. Fix: detect the SDK error result (isSDKResultMessage + is_error/error subtype) and throw so it enters backoff; mirror the twin's throw on empty. (Codex: #2272 (comment))

P1 — Distilled writes can overwrite curated memories (integrity + injection)

Transcript text is interpolated into the same extraction prompt as the rules with no trust boundary, and the LLM chooses arbitrary key/content. writeMemories → AgentMemoryRepository.write upserts on (space_id, key), the same keyspace as manual memory_write. So a distilled key that collides with an existing curated memory silently overwrites it (non-adversarial — the LLM can emit a generic key by chance — and adversarial via prompt injection steering the key). Custom-agent startup then serves the overwritten content as Core/Relevant memory. Until owner_agent_id namespacing lands, at minimum prefix distilled keys (e.g. distilled:) or route them to a separate keyspace so they cannot clobber non-distilled rows, and mark the transcript as untrusted in the prompt. (Codex: #2272 (comment))

P1 — Long distillAll runs get reclaimed as stale and double-execute

distillAll is sequential, one bounded LLM call per agent. The job processor's staleThresholdMs = 5*60*1000 (job-queue-processor.ts:29, set in app.ts:331) with concurrency 5, and reclaimStale flips the in-flight processing row back to pending (job_queue-repository.ts:237). enqueueMemoryDistillationIfMissing only guards new-row insertion, not the reset path — so after 5 min the same job is dequeued again and a second distillAll runs concurrently with the first: duplicate paid extraction calls and racing writes to the same cursors/memories. ~20–30 active agents at 10–15 s/extraction breaches the threshold. Fix: split into bounded per-agent jobs, or heartbeat/extend started_at, or exempt the queue from stale reclamation. (Codex: #2272 (comment))

P2 — Distilling in-flight (not-yet-terminal) turns

If a pass runs while an agent has an in-flight turn, persisted assistant rows are included before the terminal result lands; the retraction/supersession guards only see markers already in the DB, so a later refusal-fallback/replacement invalidates text after it's been written and the cursor advanced — never reconciled. Fix: read only through the latest completed-turn watermark, or revisit memories invalidated after the fact. (Codex: #2272 (comment))

---

What still checks out: cursor monotonicity + backoff ramp + reset-on-success, retraction/supersession guard parity (fixed last round), write isolation, schema parity, typecheck, 20/20 tests. The blockers above are the reason this can't merge yet — F1 and F2 in particular silently break the feature's core promise (reliable auto-capture) under realistic conditions.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: APPROVE

Round 3 — all 5 second-round findings are resolved and independently verified against real daemon data paths. 24/24 distillation tests pass; typecheck clean for the changed files; no schema/migration change this round.

Findings resolved (commit bfd846)

F1 — textless-row stall (P1 → fixed). getDistillableMessages now returns { messages, consumedRowid } and batch-scans (maxScan=250) past tool_use-only / thinking-only rows that extractVisibleText skips. The service advances the cursor to consumedRowid whether or not messages were found, so a window of pure textless turns no longer parks the cursor at sinceRowid and stalls distillation. Covered by the "advances cursor past textless rows" test.

F2 — message-loss regression (P1 → fixed). defaultExtractMemories now throws on isSDKResultError(message) and on an empty extractor response, routing both into the existing consecutiveFailures backoff instead of silently returning [] and advancing the cursor past content it failed to distill. isSDKResultError (type-guards.ts:103) is the right predicate. Matches the proven twin (analyzeConversationWithModel).

F3 — distilled/curated key collision + injection (P1 → fixed). writeMemories now writes under buildDistilledKey(key) = distilled:<key[:budget]>, which cannot collide with curated keys (no distilled: prefix). The extraction prompt adds an UNTRUSTED-input disclaimer. Covered by the "preserves curated memory whose key collides with a distilled candidate" test.

F4 — stale-job double-execution (P1 → fixed). Coordinator/per-agent split: the coordinator self-schedules the next tick and fans out one short per-agent job per agent; per-agent jobs are cursor-idempotent and complete well under the 5-min stale threshold, so a stale-reclaim cannot cause overlapping extractions or duplicate writes.

F5 — in-flight turn watermark (P2 → fixed). getDistillableMessages clamps to rowid <= MAX(rowid) WHERE session_id=? AND is_terminal=1 (COALESCE→-1). Verified against real data: saveSDKMessage persists every SDK result message (no filter) and computeIsTerminal stamps type==='result'' → is_terminal=1, so the watermark is a real completed-turn boundary for any session with ≥1 turn; the COALESCE arm only fires for a never-completed-turn session (correct). Covered by the "waits for completed turn before distilling (in-flight watermark)" test.

Non-blocking observations (FYI, not findings)

1. (perf) The MAX(rowid) … is_terminal=1 subquery is covered by idx_sdk_messages_renderable_terminal, but that index leads with (session_id, is_renderable, is_terminal), so the MAX scans the session partition rather than a pure seek. A partial index (session_id) WHERE is_terminal=1 would make it O(log n). Negligible at a 30-min cadence — leave as-is unless distillation is ever moved to a hot path.

2. (scale) enqueueMemoryDistillationIfMissing scans COORDINATOR_DEDUP_SCAN_LIMIT=500 pending jobs to detect an existing coordinator. If listJobs returns pending jobs ordered by runAt ASC, per-agent jobs (runAt=now) precede a self-scheduled coordinator (runAt=+30m); with >500 simultaneously-pending distillation jobs the coordinator could fall outside the window and a second coordinator tick could enqueue duplicate per-agent jobs. At maxConcurrent=5 and a 30-min cadence, pending jobs drain well within a tick, so this is unreachable at current scale — worth a one-line ORDER BY guard only if agent counts grow toward the hundreds.

Both are advisory; neither is a defect in normal operation.

Verdict

APPROVE. Correctness, backoff, and key namespacing are verified; test coverage (24 tests) exercises the stall, message-loss, collision, fan-out/dedup, and watermark paths. Ready for QA.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfd846c9cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: REQUEST_CHANGES — supersedes my Round-3 APPROVE below.

Reversing the Round-3 APPROVE. Codex reviewed head bfd846c9cd (review 4790205127) and raised 6 findings; I independently validated all 6 against the source — each is real. Most are data-loss or provider-routing correctness issues in the new distillation path. File:line evidence + suggested fixes below.

P1

F2 — Malformed extraction output silently advances the cursor → permanent batch loss. defaultExtractMemories throws on SDK-error (memory-distillation-service.ts:451) and on empty text (:469), but line :470 return parseDistillationJson(raw) lets nonempty-but-malformed JSON (truncated, prose, wrong shape) fall through: parseDistillationJson swallows every parse failure and shape mismatch to [] (:376-381, never throws). distillAgent then advances the cursor to consumedRowid whenever memoriesWritten === 0 regardless of cause, so the source batch is never retried. This is precisely the data-loss hazard the :446-450 comment describes — defeated for the malformed case (the Round-2 message-loss fix was incomplete). Fix: in defaultExtractMemories, validate that raw parses to a {memories:[]}-shaped object before returning; on parse failure throw so it routes through recordError/backoff (mirrors the empty-text fix). Add a unit test injecting malformed JSON through the service asserting the cursor does NOT advance.

F6 — Provider-only agents are distilled on the wrong provider. resolveDistillationModel (memory-distillation-service.ts:521-544) consults agentProvider only inside the if (context.agentModel) branch (:529). When an agent has provider set but model empty (a supported config — SpaceLongHorizonAgent has independent model | null / provider | null) and there's no space default, it falls to providerService.getDefaultProvider() (:541), ignoring agentProvider entirely. The agent's OWN session honors agent.provider (buildLongHorizonAgentSessionConfig), so distillation sends that agent's transcript to a different external provider/credentials than the agent uses. (The spaceDefaultModel branch :533-538 also ignores agentProvider.) Fix: add if (context.agentProvider) { resolve that provider's default model; return { provider, modelId }; } before the global fallback, and consult agentProvider in the spaceDefaultModel branch.

F1 — Provider env mutation races across concurrent distillation jobs. defaultExtractMemories calls providerService.applyEnvVarsToProcessForProvider(...) (:410), which writes ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN/etc. into shared process.env, then awaits across the SDK query() (streaming loop :446) before restoring in finally (:472). Jobs run concurrently in one process (JobQueueProcessor dispatches processJob un-awaited, job-queue-processor.ts:81; maxConcurrent=5, app.ts:327) and the coordinator fans out one job per agent (handler.ts:52-60). mergeProviderEnvVars (used for options.env, :435) spreads the LIVE process.env, so a concurrent job for a different provider can clobber routing mid-call — an Anthropic extraction can inherit a GLM base URL/token — and the non-LIFO restoreEnvVars (provider-service.ts:885-1014) leaves process.env corrupted afterward. Note: the identical apply/restore pattern exists in the twin evolution-conversation-analysis-service.ts:291-348, so this is a pre-existing hazard the PR's fan-out materially amplifies. Cleanest fix: stop mutating process.env and route purely via query() options.env (already carries merged provider env), or serialize extractions; fixing at the provider-service layer would also close the twin. Impact: transcript routed to the wrong provider/credentials (confidentiality) + persistent env corruption.

F3 — Slow per-agent extractions are stale-reclaimed and re-run concurrently. staleThresholdMs = 5*60*1000 (app.ts:328) and reclaimStale flips processing→pending for jobs older than the threshold WITHOUT aborting the in-flight handler (job-queue-repository.ts:237-244; the processor awaits the handler in-process, job-queue-processor.ts:90-109). The per-agent SDK query() has NO timeout (no AbortSignal/timeoutMs anywhere in the service), and providers set API_TIMEOUT_MS: '3000000' (50 min) for GLM/Kimi/MiniMax/OpenRouter/Ollama (e.g. glm-provider.ts:307). So an extraction exceeding 5 min is re-enqueued and a second worker re-runs distillAgentById(agentId) for the SAME agent before the first updates its cursor → duplicate paid LLM calls + racing concurrent memory writes. The handler comment (memory-distillation.handler.ts:27-31) claims the cursor makes re-runs idempotent, which is false for in-flight overlap. Likelihood is moderate (a single bounded extraction is usually <5 min), but the configured timeouts make it reachable, not hypothetical. Fix: add a per-agentId in-flight guard/claim, and/or pass AbortSignal.timeout(...) into query() with a budget well under 5 min.

F5 — Rewind + rowid reuse permanently skips new turns. sdk_messages.id is TEXT PRIMARY KEY with an implicit rowid and NO AUTOINCREMENT (schema/index.ts:191-206). Rewind deletes the high-rowid tail via deleteMessagesAtAndAfter (timestamp >= ?, sdk-message-repository.ts:1386-1397), so SQLite reuses rowids (next = MAX(rowid)+1 of remaining rows, which can be ≤ the old max). New turns can land at rowid <= last_distilled_rowid and are silently skipped by getDistillableMessages (WHERE rowid > ?). There is no cursor reset/clamp on rewind (rewind-handler.ts:304-357 never touches space_agent_memory_distillation). Reachability caveat: rewind of a long-horizon agent session is possible (the LH session is a normal AgentSession) but not routine. Fix: on rewind of an LH-agent session, clamp last_distilled_rowid to MIN(rowid) of remaining rows (or 0); more robustly, key the cursor on a monotonic turn-id/message UUID rather than the recyclable rowid.

P2

F4 — Per-agent execution path doesn't re-check agent status (TOCTOU). The coordinator enumerates only status='active' agents (space-long-horizon-agent-repository.ts:122-131), but the per-agent path distillAgentById → getById (memory-distillation-service.ts:152-156; repo getById has no status filter, :60-65) only checks the row exists. An agent paused/disabled/archived between enumeration and job execution still gets distilled (tokens spent + memory written for a stopped agent). Fix: if (agent.status !== 'active') return skipped(...) in distillAgentById after the null check.

Verdict

REQUEST_CHANGES. F2 (incomplete message-loss fix) and F6 (provider-routing divergence) are the clearest correctness bugs and should definitely be fixed; F1 and F3 are concurrency hazards amplified by this PR's fan-out; F5 is a subtle cursor/rewind interaction; F4 is a small TOCTOU. Happy to review a follow-up commit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88316d5ccd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: APPROVE

Round 5 — all 6 round-4 findings (5× P1 + 1× P2) are resolved in commit 88316d5cc and independently verified against the source. 32 distillation tests pass (26 service + 6 handler); typecheck clean for the changed files. I verified each fix, the guard/delete-path coverage, and that no new blocking issue is introduced.

Findings resolved (commit 88316d5)

F2 (P1 → fixed). parseDistillationJson now THROWS on unparseable JSON and on a wrong-shaped object, returning [] only for a legitimate {"memories":[]}. The throw propagates defaultExtractMemories → extractMemoriesLocked → distillAgent → distillAgentSafely catch → recordError/backoff, so the cursor does NOT advance and the batch is retried. Verified propagation through withExtractionLock (error-safe chain) and the in-flight guard's finally. Test asserts malformed JSON → cursor stays 0, lastError contains 'malformed', no memories written.

F6 (P1 → fixed). resolveDistillationModel adds an agentProvider-only branch (resolves that provider's default model) and honors an agent-pinned provider when it conflicts with the space-default model's provider. The provider-only agent no longer falls through to the global default. Same provider/credentials as the agent's own session, so the routing/credential concern is resolved (the model-within-provider is the title-gen variant — an intentional cost choice).

F4 (P2 → fixed). distillAgentById rechecks agent.status === 'active' after getById, returning skipped for paused/disabled/archived. Tested.

F3 (P1 → fixed). A process-wide per-agent MEMORY_DISTILLATION_IN_FLIGHT Set guards distillAgentSafely (the single chokepoint — both distillAgentById and distillAll route through it; the inner distillAgent has no external caller). A reclaimed+re-dequeued slow job hits the guard and skips non-blocking; finally always clears the entry. The has/add pair is synchronous (no await between) so it's race-free on the event loop. Tested (in-flight → skip, exactly 1 extraction call).

F1 (P1 → fixed). A module-level withExtractionLock serializes the env-mutating extractMemoriesLocked body (apply env → query → restore) so concurrent different-provider distillation jobs can't clobber process.env routing. The lock chain is error-safe (extractionLock = run.then(_, _) swallows rejections so a failed extraction doesn't break subsequent ones) while still propagating errors to the caller.

F5 (P1 → fixed). clampCursorToRemainingMessages clamps the session's cursor to MAX(remaining rowid) (COALESCE→0 for empty) after a rewind, wired at BOTH rewind delete sites (rewindToCheckpoint + selective rewind) via the new db.distillationCursor facade, each in a best-effort try/catch so distillation bookkeeping can never block a rewind. Tested (cursor 10 → 5 after tail delete; new rowid 6 > cursor; no-op for sessions with no cursor).

Non-blocking observations (FYI, not findings)

1. (F1 residual) The extraction lock serializes distillation jobs among themselves, but the twin evolution-conversation-analysis-service still mutates process.env independently — a distillation-vs-evolution cross-service race remains (pre-existing; acknowledged). The durable fix is provider-service-level env isolation (route via query() options.env without mutating process.env), which would also close the twin.

2. (F5 placement) The cursor clamp is wired at the two deleteMessagesAtAndAfter rewind call-sites rather than inside the repo delete method; the sibling deleteMessagesAfter (exclusive, currently has no callers) isn't clamped. Correct for today's paths; if a future rewind caller uses deleteMessagesAfter or a new tail-delete path appears, it would need the clamp too. Centralizing in the delete method (or a rewind helper) would be more robust.

3. (coverage) resolveDistillationModel branches 3/4 (provider-only / global fallback) are provider-service-dependent and covered by code review rather than unit tests — branches 1/2 are unit-tested.

None are defects in normal operation.

Verdict

APPROVE. All six findings are correctly resolved and tested; guard coverage, rewind delete-path completeness, typecheck, and the 32-test suite all verify clean. Ready for QA (pending Codex +1 for the new head).

[lsm]

@codex review — Round-5 fixes pushed (commit 88316d5) addressing all 6 round-4 findings. Requesting a fresh review for the new head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88316d5ccd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

@codex review — retriggering: the prior trigger reviewed (eyes) but posted no verdict for commit 88316d5. Requesting a fresh pass for the current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88316d5ccd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2dac8554d2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: APPROVE (for the current head dd9eaff3a)

Round 6 — the PR head advanced past my Round-5 APPROVE (88316d5cc → rebased 2dac8554d + this commit dd9eaff3a). I reviewed dd9eaff3a fresh: the rebase only pulls in #2265 (no distillation change); dd9eaff3a adds 3 small, correct fixes. 33 distillation tests pass (27 service + 6 handler); typecheck clean.

Fixes validated (dd9eaff3a)

Prompt ↔ parser consistency (the important one). Round-5's F2 made parseDistillationJson THROW on a missing/non-array memories key, but the extraction prompt still said "omit the array entirely if nothing durable." A model complying with the old instruction would return {} → throw → recordError → backoff → retry the same empty batch → throw again → an infinite retry loop on legitimately-empty batches. The fix (memory-distillation-service.ts:567) now instructs "return exactly {"memories": []} — do NOT omit the memories key," making prompt and parser consistent. (Disclosure: I should have caught this when I reviewed F2 in Round 5 — I didn't cross-check the empty-case prompt against the new throw behavior.)

Failed-delivery user messages excluded. getDistillableMessages now filters user messages to send_status = 'consumed' (sdk-message-repository.ts:642), dropping 'failed'/'deferred'/'enqueued' messages the agent never actually processed. Correct — distillation should only summarize content the model consumed. The watermark (rowid <= MAX(is_terminal=1)) and cursor advancement are unaffected.

Archived-Space agents excluded. listActiveWithSessions now JOIN spaces … WHERE s.status != 'archived' (space-long-horizon-agent-repository.ts:122-131), so agents whose Space was archived (status flip, agent row left active) are no longer enumerated/distilled. Tested.

Non-blocking observation (FYI, not a finding)

• The failed-delivery filter (fix 2) has no dedicated unit test. It's a one-token SQL change (IN ('consumed','failed') → = 'consumed') and CI is green, but a test asserting a 'failed' user message is excluded would lock the behavior in.

Verdict

APPROVE for dd9eaff3a. The Round-5-approved content (now 2dac8554d after rebase) plus these 3 correct fixes are the full reviewable surface; all green. Ready for QA on the current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd9eaff3a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: REQUEST_CHANGES (reversing my round-6 APPROVE, review 4793145718)

I'm reversing my round-6 APPROVE — it was premature. I validated the three dd9eaff3a code fixes (prompt empty-case, failed-delivery filter, archived-space exclusion) and they are correct; no change there. But I posted APPROVE on the strength of "44 review threads all resolved" without independently checking for open review threads. Three Codex P2 threads on dd9eaff3a remain unresolved, and on independent verification all three are genuine. This is a disclosure of the gap, not a defense of it — I should have checked thread state before approving.

---

P2-1 — Coordinator dedup misses a pending coordinator past the 500-row scan

packages/daemon/src/lib/job-handlers/memory-distillation.handler.ts:81

listJobs orders created_at DESC (job-queue-repository.ts:165), i.e. newest-first. The coordinator handler enqueues the next coordinator tick (handler.ts:48) before fanning out the per-agent jobs (handler.ts:53-60), so the coordinator is the oldest row of the batch. Once ≥500 per-agent jobs are pending (a large fleet, or a backlog surviving a daemon restart), the newest-first LIMIT 500 window is filled entirely with per-agent jobs, the coordinator falls outside it, hasCoordinator === false, and a duplicate coordinator is enqueued. This compounds on every subsequent tick and on app.ts startup — two coordinators each fan out another full set + another coordinator.

Fix: query specifically for a coordinator payload rather than assuming it appears within the first 500 pending jobs, e.g.:

SELECT 1 FROM job_queue
 WHERE queue = ? AND status = 'pending'
   AND json_extract(payload, '$.agentId') IS NULL
 LIMIT 1

Note: enqueueUniquePending({ matchPayload: {} }) is not a substitute — findMatchingActiveJob with no payload predicates matches any pending job, including per-agent ones, and would suppress the coordinator whenever per-agent work exists.

P2-2 — ACP-provider long-horizon agents can never be distilled

packages/daemon/src/lib/space/memory-distillation-service.ts:474

ACP is a registered provider (providers/acp-provider.ts:91, factory.ts:187, registry.ts:266) and LH agents carry agent.provider (export-format.ts:326), so resolveDistillationModel returns provider: 'acp' for an ACP-pinned agent. But extractMemoriesLocked always invokes the Claude SDK query(), and ACP bypasses the SDK entirely (it has its own AcpQueryAdapter — model-switch-handler.ts:249-250, query-lifecycle-manager.ts:99). The extraction call therefore cannot reach the configured ACP subprocess: it throws → recordError → backoff, and the 30-min coordinator tick re-dispatches the per-agent job each cycle, which fails again. Net effect: silent repeated failures + log spam, and the ACP agent's transcript is never distilled. Bounded impact (degradation, no corruption), but a genuinely unreachable path.

Fix (minimal): detect provider === 'acp' in distillAgent/resolveDistillationModel and skip with a clear reason (e.g. unsupported extraction provider: acp) instead of backoff-looping. Routing through the ACP query path is the fuller fix but can wait.

P2-3 — distilled: prefix is a convention, not enforced; curated memories with that prefix get clobbered

packages/daemon/src/lib/space/memory-distillation-service.ts:349

The comment at memory-distillation-service.ts:346-348 asserts distilled writes "can NEVER collide with / overwrite a curated memory_write key". But the distilled: prefix is only applied by buildDistilledKey — normalizeKey (agent-memory-repository.ts:804-809) merely trims + length-checks, and memory.write upserts on (space_id, key) with content and tags overwritten on conflict (agent-memory-repository.ts:122-124). So a hand-written memory whose key starts with distilled: — including ones created before this feature shipped — is silently overwritten in content+tags by the next distillation that produces the matching raw key. Low probability, but real data loss, and the code asserts a guarantee it does not enforce.

Fix: either reserve the prefix in normalizeKey/memory.write (reject distilled: from manual writes), or check for an existing non-distilled row before overwriting. Softening the comment to match reality is the minimum.

---

All three have cheap, localized fixes and none of them touch the dd9eaff3a correctness work, which stands. Once these are addressed I'll re-review; no need to re-litigate anything already green.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e25c96be6f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: NeoKai (GLM)

Recommendation: APPROVE

All three round-7 P2 findings are fixed correctly in e25c96be6, verified against the source (not just the commit message):

• P2-1 (coordinator dedup) — enqueueMemoryDistillationIfMissing now calls JobQueueRepository.hasPendingJobWithoutPayloadField(queue, 'agentId'), a targeted json_extract(payload,'$.agentId') IS NULL query that is order- and count-independent. The COORDINATOR_DEDUP_SCAN_LIMIT/listJobs newest-first bounded scan is gone, so a coordinator pending among ≥500 per-agent jobs is still detected. Test asserts no duplicate is enqueued. ✓
• P2-2 (ACP/non-SDK providers) — NON_SDK_EXTRACTION_PROVIDERS = new Set(['acp']); distillAgent skips before the cursor/backoff path with reason unsupported extraction provider: acp. Test asserts zero LLM calls and no cursor/error recorded (so it won't backoff-loop every cadence). Re-checked each tick so a reconfigured agent resumes. ✓
• P2-3 (reserved distilled: prefix) — AgentMemoryRepository.write rejects reserved-prefix keys unless allowReservedNamespace: true; only the distillation writer passes true. I confirmed the guard is not bypassable: the RPC handler (agent-memory-handlers.ts) and the MCP tool (agent-memory-tools.ts, zod schema) neither pass nor expose allowReservedNamespace. Reads are unaffected. Test asserts a hand-written distilled: key throws, a normal key works, and the automated writer owns the prefix. ✓

Independently verified (not trusting the summary):

• 0 unresolved review threads (GraphQL reviewThreads.isResolved) — matches the "49 threads, 0 unresolved" claim.
• The two dismissed automated-bot P2s are sound: duplicate keys within a batch is contained by upsert (one row, no corruption; the memoriesWritten over-count is cosmetic and doesn't feed a correctness decision); owner-tag truncation (buildOwnerTag capping agent:<handle> at 50 chars) is acceptable — the tag is a soft provenance hint on memories that are space-scoped by design ("per-agent namespacing not yet landed"), so a collision between two handles sharing their first 44 chars is tag ambiguity, not data loss. Non-blocking nit; the one process note is that this thread was resolved silently without a written rationale (the duplicate-keys thread had one — a one-line "by-design, space-scoped attribution" reply would help future readers).
• Tests: 36 distillation-service/handler + 48 agent-memory-repository + 15 manual-write-path (MCP tool + RPC) all pass; tsc --noEmit clean. No existing code or test writes a distilled: key through a manual path, so the new guard is non-breaking.

Zero blocking findings. Ready for QA.

[lsm]

@codex review — fresh review requested for the round-7 fix commit e25c96b (coordinator dedup via json_extract, ACP/non-SDK provider skip, reserved distilled: key prefix).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e25c96be6f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Resolve the effective provider before skipping non-SDK agents

When agent.provider is null but agent.model, the Space default model, or the global default resolves to ACP, this check does not skip the agent. resolveDistillationModel() later infers acp, after which the production extractor still invokes the Claude SDK query() path; extraction therefore fails and records repeated backoff instead of cleanly skipping these valid ACP configurations. Base this decision on the effective resolved provider rather than only the persisted provider override.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Isolate distilled rows during memory consolidation

When an extracted memory is semantically similar to a curated memory_write entry, the daily memory_consolidation job defeats this namespace separation: AgentMemoryRepository.mergeDuplicateMemories() compares every row in the Space without checking the distilled: prefix, then mergeMemoryRows() deletes whichever row loses chooseMergeTarget(). Because a freshly written distilled row commonly wins on updated_at, automatic distillation can delete the user's stable curated key; if the curated row wins instead, the next pass recreates the distilled duplicate. Exclude cross-namespace pairs from duplicate consolidation or otherwise preserve the curated entry.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Migrate existing keys before reserving the prefix

For databases upgraded from an earlier release, users may already have valid hand-written keys beginning with distilled: because the repository previously accepted them. This unconditional check immediately makes those entries impossible to update through either memory_write or agentMemory.write, while the automated writer is allowed to upsert the same keys and can overwrite one when an extracted suffix matches. Add a migration or grandfathering/ownership check before reserving the namespace so existing user data remains editable and protected.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reset the cursor when a long-horizon session is deleted

When a user hard-deletes a long-horizon agent session, the session FK cascades away its sdk_messages, but the cursor survives because it is keyed only to the agent and Space. The runtime later recreates the same deterministic session ID; if the deleted transcript contained the database's highest rowids, SQLite can assign the replacement messages rowids at or below this preserved cursor, and this filter silently skips them until the global table grows past the old value. Clear or clamp the distillation cursor as part of session deletion/recreation, just as the rewind paths already do.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Consume the terminal result before accepting extracted text

When the SDK emits an assistant message followed by an error result, this break stops iteration before isSDKResultError() can observe the terminal failure. If the assistant text is already parseable JSON—for example, a partial response completed the JSON shape before a later execution or billing error—the service writes it and advances the cursor even though the extraction call failed, permanently preventing the source messages from being retried. Retain the assistant text but continue consuming through the terminal result before parsing it.

Useful? React with 👍 / 👎.