feat: Layer 3 transcript storage, TITANS observer, and build fixes#1661
feat: Layer 3 transcript storage, TITANS observer, and build fixes#1661thedotmack wants to merge 12 commits intomainfrom
Conversation
…onfidence detection 6-phase plan covering: - MCP instruction fix for progressive search skill loading - Type system prerequisites (dynamic TypeFilter, UNIQUE constraint) - Transcript storage in separate Chroma collection via TranscriptWatcher - TITANS conversation observer with 7 observation types including overconfidence - Real-time overconfidence challenge injection (follow-up phase) - Integration and verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 1: Add Skill(mem-search) references to MCP tool descriptions and Layer 3 transcript placeholder to SKILL.md. Phase 2: Make observation types dynamic via ModeManager instead of hardcoded unions, add migration 25 for user_prompts UNIQUE constraint, and add 7 conversation observation types (insight, commitment, correction, frustration, pattern_recognition, emotional_signal, overconfidence). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…observer Phase 3 (Transcript Storage): - Second ChromaSync instance (cm__claude-mem-transcripts) for transcript chunks - TranscriptEventProcessor accumulates exchanges, flushes on prompt boundary - POST /api/transcript/store and /api/transcript/segment endpoints - get_transcript_segment MCP tool for Layer 3 traversal - Privacy tag stripping and text chunking utilities - SessionStore.getContentSessionIdByMemoryId() for observation→transcript bridge Phase 4 (TITANS Observer): - buildConversationObservationPrompt() with 7 signal types including overconfidence - OneShotQuery utility supporting Gemini/OpenRouter/Anthropic providers - POST /api/sessions/conversation-observe fire-and-forget endpoint - Processor fires conversation observation on prompt boundary - SKILL.md updated with all 13 observation types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Updated progressive-disclosure.mdx with 3-layer model - Updated CLAUDE.md with transcript storage and TITANS observer architecture - Version bump 11.0.0 → 11.1.0 across package.json and plugin manifests - Added missing Component types to logger (TRANSCRIPT, ONE_SHOT, etc.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-titans # Conflicts: # .claude-plugin/marketplace.json # package.json # plugin/.claude-plugin/plugin.json # plugin/package.json # plugin/skills/mem-search/SKILL.md # src/services/sqlite/SessionStore.ts # src/services/transcripts/processor.ts
… import - Replace `glob` npm package with `Bun.Glob` in transcript watcher to fix lru-cache top-level await incompatibility with CJS esbuild output - Add 'bun' to esbuild externals in build-hooks.js - Fix OneShotQuery.ts: import loadClaudeMemEnv instead of nonexistent EnvManager class Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a 3‑layer retrieval flow (search → get_observations → get_transcript_segment), a separate Chroma collection for transcript segments, TITANS-style conversation-observer extraction and new observation types, DB migration/indexing for prompt dedupe, new worker HTTP endpoints/MCP tool, and supporting worker/processor/sync changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Agent
participant MCP as MCP Server
participant Search as Search Service
participant Chroma as Chroma VectorDB
participant SessionStore as SessionStore
participant Worker as Worker API
rect rgba(100,150,200,0.5)
note over Client,MCP: Layer 1 — Search
Client->>MCP: search(query, obs_type)
MCP->>Search: execute search
Search->>Chroma: vector search observations
Chroma-->>Search: observation IDs
Search-->>MCP: results (observation_ids)
MCP-->>Client: return results
end
rect rgba(150,100,200,0.5)
note over Client,MCP: Layer 2 — Get Observations
Client->>MCP: get_observations(observation_ids)
MCP->>SessionStore: fetch observation records
SessionStore-->>MCP: observation data
MCP-->>Client: return observations
end
rect rgba(200,150,100,0.5)
note over Client,Worker: Layer 3 — Get Transcript Segment
Client->>MCP: get_transcript_segment(observation_id, query?)
MCP->>Worker: POST /api/transcript/segment
Worker->>SessionStore: resolve observation → contentSessionId, promptNumber
SessionStore-->>Worker: contentSessionId + promptNumber
Worker->>Chroma: query transcript collection (contentSessionId,promptNumber, query?)
Chroma-->>Worker: transcript chunks
Worker-->>MCP: return chunks, prompt_number
MCP-->>Client: return transcript segment
end
sequenceDiagram
participant Processor as Transcript Processor
participant Buffer as Exchange Buffer
participant Store as Transcript Store (Chroma)
participant ConvObserver as Conversation Observer
participant LLM as One-Shot LLM
participant DB as SessionStore
Processor->>Buffer: session_init → flush & increment promptNumber
loop prompt turns
Processor->>Buffer: user_message → append userText
Processor->>Buffer: assistant_message → populate assistantText
end
Processor->>Store: POST /api/transcript/store (chunks)
Store-->>Processor: ack (chunk count)
Processor->>ConvObserver: fireConversationObservation(exchanges) (async)
ConvObserver->>LLM: oneShotQuery(observationPrompt)
LLM-->>ConvObserver: observation XML
ConvObserver->>DB: create/attach observations to memory_session_id
DB-->>ConvObserver: confirm storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review: feat: Layer 3 transcript storage, TITANS observer, and build fixesThis is a substantial feature addition — the 3-layer progressive search architecture is well-designed and the TITANS observer concept is compelling. The code is generally clean and follows existing patterns. A few issues worth addressing before merge. Bugs1. The prompt introduces two flavors — Either add 2. const sessionDbId = store.createSDKSession(contentSessionId, project || 'unknown', '', undefined);
const session = store.getSessionById(sessionDbId);This is a get-or-create. If the session doesn't exist yet (race between processor flush and session init completing), this creates an orphaned session record with an empty model string. Should be a read-only lookup — the session must already exist for there to be exchanges to observe: const session = store.getSessionByContentId(contentSessionId);
if (!session?.memory_session_id) { /* log and return */ }3. Migration version skip [
4. Misleading fallback for missing const promptNumber = obs.prompt_number ?? 1;If an observation has no if (obs.prompt_number == null) {
res.status(404).json({ error: 'Observation has no prompt_number — transcript segment unavailable' });
return;
}Security5. Gemini API key in URL query parameter [ const url = `${GEMINI_API_URL}/${model}:generateContent?key=${apiKey}`;API keys in URLs appear in web server logs, proxy logs, and error message strings. Gemini supports const url = `${GEMINI_API_URL}/${model}:generateContent`;
headers: { 'Content-Type': 'application/json', 'x-goog-api-key': apiKey }Performance6. Each of 7. No All three provider calls have no timeout. A hung LLM request will block the background goroutine indefinitely. Add 8. Hardcoded At 2000 chars/chunk, 100 chunks ≈ 200KB. Transcripts beyond that are silently truncated. At minimum, log a warning when the limit is hit: if (documents.length === 100) {
logger.warn('CHROMA', 'Transcript segment hit chunk limit — result may be truncated', { contentSessionId, promptNumber });
}Code Quality9. Exported from both 10. Async processing after The handler sends the response, then continues async work in the same res.json({ status: 'accepted', exchangeCount: exchanges.length });
void this.processConversationObservation(contentSessionId, exchanges, project);Where 11. The doc has unchecked 12. Version number discrepancy PR description says worker reports SummaryThe Layer 3 architecture is sound and TITANS observer is a compelling addition. Main issues to address before merge: the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/transcripts/watcher.ts (1)
67-78:⚠️ Potential issue | 🔴 CriticalPersist the read offset only after the line has been durably handled.
readNewData()saves the file size beforeawait this.onLine(trimmed). With Layer 3 storage now hanging offonLine, a crash or mid-processing failure will advance the saved offset past data that never made it to storage, so those transcript lines are skipped forever on restart. This needs a post-commit watermark, not a pre-processing one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/transcripts/watcher.ts` around lines 67 - 78, The code advances this.tailState.offset and calls this.onOffset(size) before processing lines in readNewData, which can lose data if await this.onLine(trimmed) fails; change the logic so the offset is only updated after lines have been durably handled: process the combined lines (using this.tailState.partial for the trailing fragment) and await this.onLine(trimmed) for each non-empty trimmed line first, and only when all awaits succeed update this.tailState.offset and call this.onOffset(...) (or alternatively update the offset incrementally after each successful onLine to provide a post-commit watermark). Ensure this.tailState.partial is left as the trailing fragment until after successful processing so crashes do not advance the persisted offset past uncommitted lines.
🧹 Nitpick comments (3)
src/services/sqlite/SessionStore.ts (1)
66-68: Migration ordering note: version 25 called before version 24.The
addUserPromptsUniqueIndex()(migration 25) is called beforeaddSessionPlatformSourceColumn()(migration 24). While both migrations are idempotent and this doesn't cause functional issues, it creates a non-sequential version history inschema_versions. Consider swapping lines 66 and 67 to maintain ascending order.♻️ Optional: Reorder migration calls
this.addSessionCustomTitleColumn(); - this.addUserPromptsUniqueIndex(); this.addSessionPlatformSourceColumn(); + this.addUserPromptsUniqueIndex(); this.addObservationModelColumns();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/sqlite/SessionStore.ts` around lines 66 - 68, The migration calls are executed out of numeric order: addUserPromptsUniqueIndex() (v25) is invoked before addSessionPlatformSourceColumn() (v24); swap the two calls so addSessionPlatformSourceColumn() runs immediately before addUserPromptsUniqueIndex() to preserve ascending migration version history in schema_versions, leaving addObservationModelColumns() unchanged.plugin/modes/code.json (1)
48-96: Keepprompts.type_guidancein sync with these new IDs.This mode now declares seven additional observation types, but the prompt copy later in the file still says only six values are valid. That contradiction will bias prompt-driven generators away from the new types you just added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/modes/code.json` around lines 48 - 96, The prompt copy in prompts.type_guidance is out of sync: update the guidance text and any validation/count mention to include the seven new observation IDs (insight, commitment, correction, frustration, pattern_recognition, emotional_signal, overconfidence) so it no longer claims only six values are valid; ensure prompts.type_guidance enumerates or accepts these IDs and revise any numeric count or example list to match the new set.src/services/worker/http/routes/SessionRoutes.ts (1)
979-1038: Fire-and-forget observation processing may silently lose data on session lookup failure.The handler returns 202 immediately but the background processing silently returns without logging when:
- Line 1013:
session?.memory_session_idis null (new sessions without SDK response yet)For new sessions that haven't received their first SDK response, observations will be silently dropped. Consider:
- Adding INFO-level logging for this case (not just DEBUG)
- Or queueing failed observations for retry once memory_session_id is available
💡 Suggested improvement for visibility
if (!session?.memory_session_id) { - logger.debug('SESSION', 'No memory session ID for conversation observation', { contentSessionId }); + logger.info('SESSION', 'Conversation observation skipped - memory session ID not yet captured', { + contentSessionId, + sessionDbId, + exchangeCount: exchanges.length + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/worker/http/routes/SessionRoutes.ts` around lines 979 - 1038, The background worker in handleConversationObserve currently drops observations when session?.memory_session_id is missing; update the logic in handleConversationObserve (around the createSDKSession/getSessionById check) to (1) log an INFO-level message when memory_session_id is absent including contentSessionId and project so drops are visible, and (2) implement a simple retry/queue fallback: push the observation payload (observations, contentSessionId, project, promptNumber) into a retry queue or persistent store and schedule a retry that re-checks getSessionById/createSDKSession for memory_session_id and calls store.storeObservation when available; ensure storeObservation remains the final call and that retry entries expire or backoff to avoid infinite retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plan-mem-layer3-titans.md`:
- Around line 1-5: The plan file docs/plan-mem-layer3-titans.md is a
non-user-facing design doc and should be moved into the repository's contextual
docs folder; move the file from docs/ to docs/context/, update any internal
references (README, docs index, sidebar or mkdocs/nav entries) that point to
docs/plan-mem-layer3-titans.md to the new path, and ensure any links inside the
file reflecting its location are adjusted accordingly so navigation and builds
continue to work.
In `@plugin/skills/mem-search/SKILL.md`:
- Around line 133-140: The fenced code examples for get_transcript_segment are
missing blank lines before/after the triple-backtick fences and lack a language
tag, causing markdownlint MD031/MD040 failures; update both examples (the simple
call get_transcript_segment(observation_id=11131) and the scoped search
get_transcript_segment(observation_id=11131, query="why did we choose JWT")) by
adding a blank line above and below each fenced block and add a language tag
(e.g., ```text) after the opening backticks so both fences are properly
separated and annotated.
In `@src/sdk/prompts.ts`:
- Around line 261-265: The code builds exchangeXml by embedding ex.userText and
ex.assistantText raw, which can break or inject tags; add an XML-escaping helper
(e.g., escapeXml or encodeXml) that replaces &, <, >, " and ' with their entity
equivalents and use it when constructing exchangeXml (apply to ex.userText and
ex.assistantText inside the template) and any other places where conversation
text is interpolated into XML/HTML templates.
In `@src/services/transcripts/processor.ts`:
- Around line 369-390: The code calls workerHttpRequest('/api/transcript/store')
but ignores the returned Response, so non-2xx responses are treated like
successes and session.exchanges is cleared; change the try block to capture the
response (from workerHttpRequest) and check response.ok (and optionally
response.status/text) before considering the store successful; if response.ok is
false, log a warning with the status/body and do not clear session.exchanges (so
the buffered segment is retained for retry), while still calling
this.fireConversationObservation(session, exchangesToFlush) as needed.
In `@src/services/worker/OneShotQuery.ts`:
- Around line 23-128: The selected-provider branches currently return
immediately and the provider helpers can hang or reject; change oneShotQuery so
it only returns when a provider helper yields a non-null string (i.e., call
queryGemini, queryOpenRouter, queryAnthropic and if the result is null proceed
to the next provider instead of returning early), and harden each helper
(queryGemini, queryOpenRouter, queryAnthropic) to never throw by wrapping
fetch()/response.json() in try/catch, use an AbortController-based timeout
(e.g., 10s) to cancel slow requests, and on any error or timeout log a warning
and return null so failures fall through to the next provider.
---
Outside diff comments:
In `@src/services/transcripts/watcher.ts`:
- Around line 67-78: The code advances this.tailState.offset and calls
this.onOffset(size) before processing lines in readNewData, which can lose data
if await this.onLine(trimmed) fails; change the logic so the offset is only
updated after lines have been durably handled: process the combined lines (using
this.tailState.partial for the trailing fragment) and await this.onLine(trimmed)
for each non-empty trimmed line first, and only when all awaits succeed update
this.tailState.offset and call this.onOffset(...) (or alternatively update the
offset incrementally after each successful onLine to provide a post-commit
watermark). Ensure this.tailState.partial is left as the trailing fragment until
after successful processing so crashes do not advance the persisted offset past
uncommitted lines.
---
Nitpick comments:
In `@plugin/modes/code.json`:
- Around line 48-96: The prompt copy in prompts.type_guidance is out of sync:
update the guidance text and any validation/count mention to include the seven
new observation IDs (insight, commitment, correction, frustration,
pattern_recognition, emotional_signal, overconfidence) so it no longer claims
only six values are valid; ensure prompts.type_guidance enumerates or accepts
these IDs and revise any numeric count or example list to match the new set.
In `@src/services/sqlite/SessionStore.ts`:
- Around line 66-68: The migration calls are executed out of numeric order:
addUserPromptsUniqueIndex() (v25) is invoked before
addSessionPlatformSourceColumn() (v24); swap the two calls so
addSessionPlatformSourceColumn() runs immediately before
addUserPromptsUniqueIndex() to preserve ascending migration version history in
schema_versions, leaving addObservationModelColumns() unchanged.
In `@src/services/worker/http/routes/SessionRoutes.ts`:
- Around line 979-1038: The background worker in handleConversationObserve
currently drops observations when session?.memory_session_id is missing; update
the logic in handleConversationObserve (around the
createSDKSession/getSessionById check) to (1) log an INFO-level message when
memory_session_id is absent including contentSessionId and project so drops are
visible, and (2) implement a simple retry/queue fallback: push the observation
payload (observations, contentSessionId, project, promptNumber) into a retry
queue or persistent store and schedule a retry that re-checks
getSessionById/createSDKSession for memory_session_id and calls
store.storeObservation when available; ensure storeObservation remains the final
call and that retry entries expire or backoff to avoid infinite retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44b66dfa-120b-4006-a8cb-19d6acb30b84
📒 Files selected for processing (24)
CLAUDE.mddocs/plan-mem-layer3-titans.mddocs/public/progressive-disclosure.mdxplugin/modes/code.jsonplugin/scripts/context-generator.cjsplugin/scripts/mcp-server.cjsplugin/scripts/worker-service.cjsplugin/skills/mem-search/SKILL.mdplugin/ui/viewer-bundle.jsscripts/build-hooks.jssrc/sdk/prompts.tssrc/servers/mcp-server.tssrc/services/sqlite/SessionStore.tssrc/services/sqlite/types.tssrc/services/sync/ChromaSync.tssrc/services/transcripts/processor.tssrc/services/transcripts/watcher.tssrc/services/worker/DatabaseManager.tssrc/services/worker/OneShotQuery.tssrc/services/worker/http/routes/SessionRoutes.tssrc/services/worker/search/filters/TypeFilter.tssrc/types/database.tssrc/utils/logger.tssrc/utils/tag-stripping.ts
| # Implementation Plan: Memory Layer 3, TITANS Observer, and Progressive Search Fix | ||
|
|
||
| **Created:** 2026-04-05 | **Revised:** 2026-04-06 (post eng review) | ||
| **Branch:** thedotmack/mem-layer3-titans | ||
| **Scope:** 4 enhancements to claude-mem's memory system |
There was a problem hiding this comment.
Move this plan into docs/context/.
This is a planning/reference document, so keeping it at the top level of docs/ makes the docs tree inconsistent with the repo convention for non-user-facing design material.
Based on learnings, planning documents, design docs, and reference materials must be placed in /docs/context/ instead of /docs/public/.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...26-04-05 | Revised: 2026-04-06 (post eng review) Branch: thedotmack/mem-l...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plan-mem-layer3-titans.md` around lines 1 - 5, The plan file
docs/plan-mem-layer3-titans.md is a non-user-facing design doc and should be
moved into the repository's contextual docs folder; move the file from docs/ to
docs/context/, update any internal references (README, docs index, sidebar or
mkdocs/nav entries) that point to docs/plan-mem-layer3-titans.md to the new
path, and ensure any links inside the file reflecting its location are adjusted
accordingly so navigation and builds continue to work.
… watermark - Escape user/assistant text in XML conversation wrapper (prompts.ts) - Check response.ok before clearing exchange buffer (processor.ts) - Add 30s timeout + try/catch fallthrough to OneShotQuery providers - Move offset persistence after line processing in watcher (post-commit watermark) - Move plan doc to docs/context/ per repo convention - Fix markdown fences in SKILL.md (blank lines + language tags) - Reorder migration calls to ascending version order (SessionStore.ts) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: Layer 3 Transcript Storage, TITANS Observer, and Build FixesGood architectural work here — the 3-layer progressive search model is clean and the separation of transcript storage (Chroma) from searchable observations (SQLite) makes sense. A few issues need attention before this lands. Critical: Offset watermark race in
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/scripts/context-generator.cjs (1)
290-294:⚠️ Potential issue | 🟠 MajorMake
saveUserPrompt()idempotent now that the index is unique.
addUserPromptsUniqueIndex()makes(content_session_id, prompt_number)unique, butsaveUserPrompt()still does a plainINSERT. A retriedsession_initnow throwsSQLITE_CONSTRAINT_UNIQUEinstead of deduping, which is the exact path this migration is supposed to harden.Suggested fix
saveUserPrompt(e,t,s){ let n=new Date,o=n.getTime(); - return this.db.prepare(` - INSERT INTO user_prompts - (content_session_id, prompt_number, prompt_text, created_at, created_at_epoch) - VALUES (?, ?, ?, ?, ?) - `).run(e,t,s,n.toISOString(),o).lastInsertRowid + let i=this.db.prepare(` + INSERT OR IGNORE INTO user_prompts + (content_session_id, prompt_number, prompt_text, created_at, created_at_epoch) + VALUES (?, ?, ?, ?, ?) + `).run(e,t,s,n.toISOString(),o); + if(i.changes===0){ + return this.db.prepare(` + SELECT id + FROM user_prompts + WHERE content_session_id = ? AND prompt_number = ? + LIMIT 1 + `).get(e,t).id + } + return i.lastInsertRowid }Based on learnings,
plugin/scripts/context-generator.cjscontains its own embedded DB class with its own migration methods and must be reviewed independently from the TypeScript source path.Also applies to: 480-484
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/context/plan-mem-layer3-titans.md`:
- Around line 162-175: The heading "Add 6 conversation observation types to code
mode" is inconsistent with the JSON payload which adds 7 entries; update the
heading to "Add 7 conversation observation types to code mode" (or remove the
extra `overconfidence` entry if intentional) and ensure the `observation_types`
change references the seven ids (insight, commitment, correction, frustration,
pattern_recognition, emotional_signal, overconfidence) so the section text and
payload match.
- Around line 113-125: Update the rollout plan to use the same schema version as
the implementation: change the migration label from "Migration 24" to "Migration
25" (or otherwise make the plan's migration number match the one passed to
applyMigration in src/services/sqlite/SessionStore.ts); ensure the UNIQUE index
description and the index name idx_user_prompts_unique_session_prompt remain
unchanged so verification steps align with the actual migration applied.
In `@src/services/transcripts/processor.ts`:
- Around line 147-149: The code increments session.promptNumber immediately
after calling flushTranscriptSegment(session), which means if the POST to
/api/transcript/store fails the retained session.exchanges will be associated
with the new promptNumber on the next successful flush; modify the logic in the
session handling (where flushTranscriptSegment and session.promptNumber are
used, and similarly in the blocks around lines 354-395) to ensure failed flushes
are not re-associated with a new promptNumber by either 1) moving the
session.promptNumber++ to occur only after a successful flush (i.e., increment
inside the success callback of flushTranscriptSegment) or 2) implement a retry
queue keyed by the original promptNumber (store a copy of session.exchanges and
its promptNumber when a flush fails and retry posting that exact pair until
success); update references to session.exchanges, flushTranscriptSegment, and
session_init accordingly so each buffered segment remains tied to its original
promptNumber.
- Around line 404-418: The background observe call treats any fulfilled
workerHttpRequest as success; update the ensureWorkerRunning() handler that
calls workerHttpRequest('/api/sessions/conversation-observe', ...) to inspect
the resolved response and log non-2xx results: after the promise resolves, check
response.ok (or response.status) and if false call logger.error (or logger.debug
as appropriate) with the endpoint, status, statusText and include the response
body/text for diagnostics; keep the existing .catch for transport failures but
ensure non-2xx responses are not ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6a825b8-053c-43eb-b7b4-eb3723768677
📒 Files selected for processing (9)
docs/context/plan-mem-layer3-titans.mdplugin/scripts/context-generator.cjsplugin/scripts/worker-service.cjsplugin/skills/mem-search/SKILL.mdsrc/sdk/prompts.tssrc/services/sqlite/SessionStore.tssrc/services/transcripts/processor.tssrc/services/transcripts/watcher.tssrc/services/worker/OneShotQuery.ts
✅ Files skipped from review due to trivial changes (2)
- src/sdk/prompts.ts
- plugin/skills/mem-search/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/transcripts/watcher.ts
- src/services/worker/OneShotQuery.ts
- src/services/sqlite/SessionStore.ts
…serve logging - Make saveUserPrompt() use INSERT OR IGNORE for unique index compatibility - Return boolean from flushTranscriptSegment; only increment promptNumber on success - Log non-2xx responses from background conversation-observe call - Fix plan doc: Migration 24→25, 6→7 observation types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review: Layer 3 Transcript Storage, TITANS Observer, Build FixesOverall this is a well-structured addition. The 3-layer progressive search model is clearly architected, the separation of the transcript Chroma collection from the main search index is the right call, and the fire-and-forget TITANS pattern is clean. A few issues worth addressing before merge: 🐛 Bugs1. First conversation segment is silently dropped (
The first segment is lost every time. Fix: initialize 2. const sessionDbId = store.createSDKSession(contentSessionId, project || 'unknown', '', undefined);If the session doesn't already exist in the DB, this inserts a new row with an empty string
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/scripts/context-generator.cjs (1)
290-294:⚠️ Potential issue | 🔴 CriticalThe bundled DB class still has the unsafe UNIQUE-index migration.
This embedded
SessionStorealso does a straightCREATE UNIQUE INDEXonuser_prompts(content_session_id, prompt_number), so installs with pre-existing duplicates can still fail through the context-generator/plugin path. Fix the source migration to reconcile duplicates first, then regenerate this bundle rather than patching the minified artifact by hand.Based on learnings,
plugin/scripts/context-generator.cjscontains its own embedded DB class with its own migration methods, so this code path must be checked independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/scripts/context-generator.cjs` around lines 290 - 294, The embedded migration addUserPromptsUniqueIndex still runs a straight CREATE UNIQUE INDEX which fails on pre-existing duplicates; update the addUserPromptsUniqueIndex method in the bundled DB class to first reconcile duplicates (e.g. identify duplicate rows on (content_session_id, prompt_number), keep the canonical row per group and delete other duplicates using their rowids or primary key), then run CREATE UNIQUE INDEX IF NOT EXISTS idx_user_prompts_unique_session_prompt and only after that insert the schema_versions row; regenerate the bundle instead of hand-patching the minified artifact.src/services/transcripts/processor.ts (1)
337-349:⚠️ Potential issue | 🟠 MajorDon't tear down the session after a failed final flush.
handleSessionEnd()ignores the boolean fromflushTranscriptSegment()and then deletes the in-memory session. Any 503/network failure here permanently drops the last transcript segment and its TITANS input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/transcripts/processor.ts` around lines 337 - 349, handleSessionEnd currently ignores the boolean result from flushTranscriptSegment and always tears down the session; change it to check the return value of flushTranscriptSegment(session) and if it returns false (flush failed) abort teardown—do not call queueSummary, sessionCompleteHandler.execute, updateContext, session.pendingTools.clear, or sessions.delete; instead log the failure and return (or throw) so the session remains in memory for retry. Ensure references to flushTranscriptSegment, queueSummary, sessionCompleteHandler.execute, updateContext, session.pendingTools.clear, getSessionKey, and sessions.delete are used so you modify the correct control flow.
♻️ Duplicate comments (3)
docs/context/plan-mem-layer3-titans.md (2)
735-738:⚠️ Potential issue | 🟡 MinorThis summary still says 6 conversation observation types.
The plan and implementation use seven types now, including
overconfidence. Keeping6here makes the final checklist internally inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/context/plan-mem-layer3-titans.md` around lines 735 - 738, The Architecture section currently states "Conversation observation modality (TITANS-inspired, 6 types)" but the implementation and plan use seven types including the new `overconfidence`; update that sentence to "7 types" and ensure the conversation observation types list explicitly includes `overconfidence` (and any other seven names used in the plan) so the Architecture text and final checklist are consistent with the implementation.
113-125:⚠️ Potential issue | 🟡 MinorThe plan still names the
user_promptsindex as migration 24.This change landed as schema version
25, but this section and Appendix B still say24. That makes rollout and verification notes easy to follow incorrectly.Also applies to: 798-799
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/context/plan-mem-layer3-titans.md` around lines 113 - 125, The docs incorrectly label the UNIQUE index migration for user_prompts as migration 24; update every mention in docs/context/plan-mem-layer3-titans.md (section "2.1: Add UNIQUE constraint to user_prompts" and Appendix B references around lines noted) to reflect the actual schema version 25, and ensure the migration description references the same applyMigration(version, fn) pattern used for Migration 22 and the file `src/services/sqlite/SessionStore.ts` so rollout and verification steps point to migration 25 consistently.src/services/transcripts/processor.ts (1)
146-149:⚠️ Potential issue | 🟠 MajorA failed flush still collapses two prompts into one buffered segment.
When
flushTranscriptSegment()returnsfalse,session_initkeeps going and the nextuser_messageis appended to the samesession.exchangesarray. That merges two prompt boundaries into one stored segment, so observation → transcript traversal no longer matches the original prompt. A failed segment needs its own retry slot keyed by the original prompt number, or the buffer needs to rotate before the next prompt is accepted.Also applies to: 156-165, 355-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/transcripts/processor.ts` around lines 146 - 149, The flush failure currently lets the next prompt get appended to the same session.exchanges and collapses prompt boundaries; modify the session_init handling (and the other similar blocks around flushTranscriptSegment usage) so that when flushTranscriptSegment(session) returns false you still rotate the buffer / reserve a retry slot keyed by the current prompt number before accepting the next prompt—e.g., if flushed === false then increment session.promptNumber (or push a retry placeholder keyed by the original session.promptNumber) so the next user_message is recorded under a new prompt boundary; apply the same change to the other flush usages mentioned (the blocks around lines 156-165 and 355-397) and ensure any retry logic references the preserved original promptNumber.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/sqlite/SessionStore.ts`:
- Around line 894-905: The migration method addUserPromptsUniqueIndex attempts
to create a UNIQUE index but will fail if duplicate (content_session_id,
prompt_number) rows already exist; modify addUserPromptsUniqueIndex to first
deduplicate existing rows inside a transaction (e.g., delete duplicates keeping
one row per (content_session_id, prompt_number) using ROWID or a CTE that
preserves the min ROWID), then create the UNIQUE index and finally insert the
schema_versions row; target the addUserPromptsUniqueIndex function and ensure
the dedupe+CREATE INDEX+INSERT are atomic to avoid startup failures on older
databases.
In `@src/services/transcripts/processor.ts`:
- Around line 64-65: The bug is that promptNumber is initialized to 0 so the
first prompt (prompt 1) is dropped because session_init doesn't increment before
the first flush and flushTranscriptSegment refuses to store prompt 0; fix this
by initializing promptNumber to 1 (or alternatively adjust
flushTranscriptSegment to accept/promote 0 to 1) so the first prompt is
preserved. Update the initial value where promptNumber is set and ensure any
logic in session_init and flushTranscriptSegment (referenced by the symbols
promptNumber, session_init, and flushTranscriptSegment) remains consistent with
the new starting value.
---
Outside diff comments:
In `@plugin/scripts/context-generator.cjs`:
- Around line 290-294: The embedded migration addUserPromptsUniqueIndex still
runs a straight CREATE UNIQUE INDEX which fails on pre-existing duplicates;
update the addUserPromptsUniqueIndex method in the bundled DB class to first
reconcile duplicates (e.g. identify duplicate rows on (content_session_id,
prompt_number), keep the canonical row per group and delete other duplicates
using their rowids or primary key), then run CREATE UNIQUE INDEX IF NOT EXISTS
idx_user_prompts_unique_session_prompt and only after that insert the
schema_versions row; regenerate the bundle instead of hand-patching the minified
artifact.
In `@src/services/transcripts/processor.ts`:
- Around line 337-349: handleSessionEnd currently ignores the boolean result
from flushTranscriptSegment and always tears down the session; change it to
check the return value of flushTranscriptSegment(session) and if it returns
false (flush failed) abort teardown—do not call queueSummary,
sessionCompleteHandler.execute, updateContext, session.pendingTools.clear, or
sessions.delete; instead log the failure and return (or throw) so the session
remains in memory for retry. Ensure references to flushTranscriptSegment,
queueSummary, sessionCompleteHandler.execute, updateContext,
session.pendingTools.clear, getSessionKey, and sessions.delete are used so you
modify the correct control flow.
---
Duplicate comments:
In `@docs/context/plan-mem-layer3-titans.md`:
- Around line 735-738: The Architecture section currently states "Conversation
observation modality (TITANS-inspired, 6 types)" but the implementation and plan
use seven types including the new `overconfidence`; update that sentence to "7
types" and ensure the conversation observation types list explicitly includes
`overconfidence` (and any other seven names used in the plan) so the
Architecture text and final checklist are consistent with the implementation.
- Around line 113-125: The docs incorrectly label the UNIQUE index migration for
user_prompts as migration 24; update every mention in
docs/context/plan-mem-layer3-titans.md (section "2.1: Add UNIQUE constraint to
user_prompts" and Appendix B references around lines noted) to reflect the
actual schema version 25, and ensure the migration description references the
same applyMigration(version, fn) pattern used for Migration 22 and the file
`src/services/sqlite/SessionStore.ts` so rollout and verification steps point to
migration 25 consistently.
In `@src/services/transcripts/processor.ts`:
- Around line 146-149: The flush failure currently lets the next prompt get
appended to the same session.exchanges and collapses prompt boundaries; modify
the session_init handling (and the other similar blocks around
flushTranscriptSegment usage) so that when flushTranscriptSegment(session)
returns false you still rotate the buffer / reserve a retry slot keyed by the
current prompt number before accepting the next prompt—e.g., if flushed ===
false then increment session.promptNumber (or push a retry placeholder keyed by
the original session.promptNumber) so the next user_message is recorded under a
new prompt boundary; apply the same change to the other flush usages mentioned
(the blocks around lines 156-165 and 355-397) and ensure any retry logic
references the preserved original promptNumber.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 366b381b-bd37-4a7d-9417-22c95ff72efb
📒 Files selected for processing (5)
docs/context/plan-mem-layer3-titans.mdplugin/scripts/context-generator.cjsplugin/scripts/worker-service.cjssrc/services/sqlite/SessionStore.tssrc/services/transcripts/processor.ts
…ssion end safety - Reconcile pre-existing duplicate user_prompts rows before CREATE UNIQUE INDEX - Log warning on failed final flush in handleSessionEnd instead of silently dropping - Rebuild bundles with migration fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/transcripts/processor.ts (1)
337-353:⚠️ Potential issue | 🟠 MajorA failed final flush is still discarded during teardown.
Lines 339-343 log the failure, but Lines 351-353 immediately clear state and delete the session anyway. Any transient 503/timeout on the last prompt permanently loses that segment, so Layer 3 retrieval can miss the conversation tail. This needs a retry path keyed to the original
promptNumber, or the session state has to survive until the store succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/transcripts/processor.ts` around lines 337 - 353, handleSessionEnd currently logs a failed final flush from flushTranscriptSegment but then proceeds to clear state and delete the session (calling queueSummary, sessionCompleteHandler.execute, updateContext, session.pendingTools.clear and sessions.delete via getSessionKey), which can permanently lose the last segment; change handleSessionEnd so that when flushTranscriptSegment returns false it does not purge the session immediately but instead retries the flush (exponential backoff or limited retry loop) keyed by the session's promptNumber or otherwise preserves session.exchanges and session state until flushTranscriptSegment succeeds or a configurable maxRetry is reached; only after a successful flush (or exhausting retries with a safe persisted fallback) should you call queueSummary, sessionCompleteHandler.execute, updateContext, clear pendingTools and sessions.delete.
♻️ Duplicate comments (1)
src/services/transcripts/processor.ts (1)
64-65:⚠️ Potential issue | 🔴 CriticalPrompt 1 is still unflushable.
Line 64 starts
promptNumberat0, and Line 361 refuses to flush prompt0. Becausesession_initonly increments after a successful flush, the counter never advances and the first turn never reaches/api/transcript/store.🩹 Minimal fix
session = { sessionId, - promptNumber: 0, + promptNumber: 1, exchanges: [], platformSource: normalizePlatformSource(watch.name), pendingTools: new Map() }; ... - if (session.exchanges.length === 0 || session.promptNumber === 0) return false; + if (session.exchanges.length === 0) return false;Also applies to: 360-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/transcripts/processor.ts` around lines 64 - 65, The initial promptNumber is set to 0 so session_init never advances past the first prompt and prompt 0 is never flushed; change the initialization of promptNumber from 0 to 1 (where promptNumber and exchanges are initialized) so the first turn can be flushed to /api/transcript/store, or alternatively adjust the session_init flush logic to allow flushing promptNumber 0 by incrementing the counter before the flush; reference the promptNumber variable and the session_init flush path to locate and apply the minimal fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/sqlite/SessionStore.ts`:
- Around line 1699-1710: The code currently handles an INSERT OR IGNORE into
user_prompts and returns 0 if result.changes === 0 and the subsequent SELECT for
the existing row (by contentSessionId and promptNumber) returns nothing;
instead, update the branch in SessionStore.ts so that when result.changes === 0
you attempt to load the existing row id (SELECT id FROM user_prompts WHERE
content_session_id = ? AND prompt_number = ?), and if that query returns
undefined throw a descriptive error (including contentSessionId and
promptNumber) indicating a DB inconsistency rather than returning 0; keep
returning the found id when present and only throw when the existing variable is
missing.
---
Outside diff comments:
In `@src/services/transcripts/processor.ts`:
- Around line 337-353: handleSessionEnd currently logs a failed final flush from
flushTranscriptSegment but then proceeds to clear state and delete the session
(calling queueSummary, sessionCompleteHandler.execute, updateContext,
session.pendingTools.clear and sessions.delete via getSessionKey), which can
permanently lose the last segment; change handleSessionEnd so that when
flushTranscriptSegment returns false it does not purge the session immediately
but instead retries the flush (exponential backoff or limited retry loop) keyed
by the session's promptNumber or otherwise preserves session.exchanges and
session state until flushTranscriptSegment succeeds or a configurable maxRetry
is reached; only after a successful flush (or exhausting retries with a safe
persisted fallback) should you call queueSummary,
sessionCompleteHandler.execute, updateContext, clear pendingTools and
sessions.delete.
---
Duplicate comments:
In `@src/services/transcripts/processor.ts`:
- Around line 64-65: The initial promptNumber is set to 0 so session_init never
advances past the first prompt and prompt 0 is never flushed; change the
initialization of promptNumber from 0 to 1 (where promptNumber and exchanges are
initialized) so the first turn can be flushed to /api/transcript/store, or
alternatively adjust the session_init flush logic to allow flushing promptNumber
0 by incrementing the counter before the flush; reference the promptNumber
variable and the session_init flush path to locate and apply the minimal fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bedb1e8-5639-43ac-8b49-be14c9c8dc39
📒 Files selected for processing (4)
plugin/scripts/context-generator.cjsplugin/scripts/worker-service.cjssrc/services/sqlite/SessionStore.tssrc/services/transcripts/processor.ts
| const result = this.db.prepare(` | ||
| INSERT OR IGNORE INTO user_prompts | ||
| (content_session_id, prompt_number, prompt_text, created_at, created_at_epoch) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| `); | ||
| `).run(contentSessionId, promptNumber, promptText, now.toISOString(), nowEpoch); | ||
|
|
||
| const result = stmt.run(contentSessionId, promptNumber, promptText, now.toISOString(), nowEpoch); | ||
| if (result.changes === 0) { | ||
| const existing = this.db.prepare( | ||
| 'SELECT id FROM user_prompts WHERE content_session_id = ? AND prompt_number = ? LIMIT 1' | ||
| ).get(contentSessionId, promptNumber) as { id: number } | undefined; | ||
| return existing?.id ?? 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the SessionStore.ts file
fd -t f "SessionStore.ts" --path-separator / | head -20Repository: thedotmack/claude-mem
Length of output: 100
🏁 Script executed:
# Check the file size and read the relevant lines
wc -l src/services/sqlite/SessionStore.tsRepository: thedotmack/claude-mem
Length of output: 105
🏁 Script executed:
# Read the actual implementation around lines 1699-1710
sed -n '1690,1720p' src/services/sqlite/SessionStore.ts | cat -nRepository: thedotmack/claude-mem
Length of output: 1346
🏁 Script executed:
# Search for the user_prompts table schema definition
rg "CREATE TABLE.*user_prompts|user_prompts.*CREATE TABLE" -A 15Repository: thedotmack/claude-mem
Length of output: 6594
🏁 Script executed:
# Look for migration files that might define the schema
fd -t f -e "sql" -e "ts" "migration" | head -20Repository: thedotmack/claude-mem
Length of output: 145
🏁 Script executed:
# Search for saveUserPrompt usage to understand how the return value is used
rg "saveUserPrompt" -B 2 -A 2Repository: thedotmack/claude-mem
Length of output: 36646
🏁 Script executed:
# Check for unique constraints on user_prompts table
rg "unique.*content_session_id|prompt_number|UNIQUE" -B 2 -A 2Repository: thedotmack/claude-mem
Length of output: 50378
🏁 Script executed:
# Search for how saveUserPrompt's return value is used
rg "saveUserPrompt.*\(" -A 2 | head -40Repository: thedotmack/claude-mem
Length of output: 2519
🏁 Script executed:
# Look at the unique index creation more carefully
rg "addUserPromptsUniqueIndex|idx_user_prompts_unique" -B 2 -A 5Repository: thedotmack/claude-mem
Length of output: 27232
🏁 Script executed:
# Check if there are any other constraints on user_prompts table
rg "CREATE TABLE.*user_prompts|FOREIGN KEY.*user_prompts|UNIQUE.*user_prompts" -B 1 -A 10 | head -60Repository: thedotmack/claude-mem
Length of output: 5226
🏁 Script executed:
# Search for tests that verify duplicate prompt handling
rg "saveUserPrompt.*duplicate|duplicate.*prompt|saveUserPrompt.*existing" -B 3 -A 3Repository: thedotmack/claude-mem
Length of output: 28392
🏁 Script executed:
# Check if the return value 0 is handled anywhere
rg "saveUserPrompt" -A 3 | rg "=== 0|== 0|!== 0|!= 0" -B 2 -A 1Repository: thedotmack/claude-mem
Length of output: 47
Return a valid row ID or throw; do not silently return 0.
When INSERT OR IGNORE succeeds (duplicate is skipped), the follow-up lookup should always find the existing row. If it misses, that signals a database inconsistency and should raise an error rather than return 0. Throwing makes failures visible instead of masked by a sentinel value.
🩹 Fail explicitly when the existing row is not found after insert conflict
if (result.changes === 0) {
const existing = this.db.prepare(
'SELECT id FROM user_prompts WHERE content_session_id = ? AND prompt_number = ? LIMIT 1'
).get(contentSessionId, promptNumber) as { id: number } | undefined;
- return existing?.id ?? 0;
+ if (!existing) {
+ throw new Error(`Failed to save user prompt for ${contentSessionId}:${promptNumber}`);
+ }
+ return existing.id;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/sqlite/SessionStore.ts` around lines 1699 - 1710, The code
currently handles an INSERT OR IGNORE into user_prompts and returns 0 if
result.changes === 0 and the subsequent SELECT for the existing row (by
contentSessionId and promptNumber) returns nothing; instead, update the branch
in SessionStore.ts so that when result.changes === 0 you attempt to load the
existing row id (SELECT id FROM user_prompts WHERE content_session_id = ? AND
prompt_number = ?), and if that query returns undefined throw a descriptive
error (including contentSessionId and promptNumber) indicating a DB
inconsistency rather than returning 0; keep returning the found id when present
and only throw when the existing variable is missing.
Code Review: Layer 3 Transcript Storage, TITANS Observer, Build FixesGood architectural scope — the 3-layer progressive search is a solid design. The build fixes (glob → Bun.Glob, esbuild externals) are clean. Here are my findings, prioritized by severity. 🔴 Critical1.
Prompt 1 exchanges are never stored. Fix: unconditionally increment on session_init boundary, flush if there are exchanges: case 'session_init': {
await this.flushTranscriptSegment(session); // returns bool but always runs
session.promptNumber++; // always increment
...And remove the 2. const sessionDbId = store.createSDKSession(contentSessionId, project || 'unknown', '', undefined);
const session = store.getSessionById(sessionDbId);
const contentId = store.getContentSessionIdByMemoryId(...); // already added
// or add: store.getSDKSessionByContentId(contentSessionId)This is a data integrity issue on a critical path — the observations stored will be linked to a phantom session with no real context. 🟠 High3. No AbortController on fetch — timeout leaks (
const withTimeout = <T>(p: Promise<T>, ms: number): Promise<T | null> => {
const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), ms);
return p.finally(() => clearTimeout(timer)).catch(() => null);
};
// Pass signal to each fetch({ ..., signal: controller.signal })4. Settings file read on every one-shot query (
5. limit: 100,A 200k-char conversation produces 100 chunks at 2000 chars each. This silently truncates any transcript over 200k chars. Either document this as a known limit, make it configurable, or increase it substantially. Silent truncation on 6. No concurrency control on conversation observation ( Every session boundary fires a one-shot LLM call in the background. There's no queuing, rate limiting, or max-concurrency guard. A user with many active windows could trigger dozens of simultaneous API calls. Consider a simple semaphore or queue (e.g. 🟡 Medium7. Defined identically in both 8. chunks.push(text.substring(i, i + chunkSize));This cuts at byte boundary, not word boundary. For vector embeddings this reduces chunk quality — a chunk beginning with 9. Migration version gap: 24 → 25 skipped ( The new migration registers as version 25, but the previous migration in the constructor ( 10. If ⚪ Minor11.
12. In ✅ Things that look good
TestsThe test plan in the PR description has no checked items. Are tests expected in this PR or a follow-up? The behaviors most worth testing given the issues above:
🤖 Generated with Claude Code |
…n session end - Start promptNumber at 1 (not 0) so first prompt is flushable - Remove dead promptNumber === 0 guard from flushTranscriptSegment - Add 3-attempt retry with exponential backoff for final flush on session end Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review: feat: Layer 3 transcript storage, TITANS observer, and build fixesThe architecture concept is solid — a 3-layer progressive search with TITANS-inspired conversation observation is a meaningful improvement to the memory pipeline. The build fix (Bun.Glob, esbuild externals) is clean and necessary. Below are issues worth addressing before merge, organized by severity. CRITICAL
const url = `${GEMINI_API_URL}/${model}:generateContent?key=${apiKey}`;API keys in URLs are logged by servers, proxies, and error reporting tools. Use headers: { 'Content-Type': 'application/json', 'x-goog-api-key': apiKey }
All three provider fetch functions call let data: any;
try { data = await response.json(); } catch { return null; }HIGH
const withTimeout = <T>(p: Promise<T>): Promise<T | null> =>
Promise.race([p, new Promise<null>(resolve => setTimeout(() => resolve(null), ONE_SHOT_TIMEOUT_MS))]);After timeout resolves, the original fetch continues running in the background. Each timed-out TITANS query leaks a connection. Use const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), ONE_SHOT_TIMEOUT_MS);
// pass signal: controller.signal to fetch
return existing?.id ?? 0;If the constraint fires but the fallback SELECT also finds nothing (shouldn't happen, but edge cases exist), this returns
Long sessions will be cut off without any warning. At minimum, log a warning when
If Chroma returns mismatched arrays, the
ensureWorkerRunning().then(ready => { ... }).catch(...);The top-level
If
if (!contentSessionId || !exchanges || !Array.isArray(exchanges)) { ... }This passes for an empty array and for arrays containing malformed objects. Downstream code at line ~3597 accesses MEDIUM
for (let i = 0; i < text.length; i += chunkSize)
if (chunkSize <= 0) throw new Error('chunkSize must be positive');
The move from Minor
Summary
The API key URL exposure and session race condition are the most urgent to address before merge. The rest are solid improvements but could be follow-up PRs if needed. 🤖 Reviewed with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/transcripts/processor.ts`:
- Around line 343-345: The retry wait is using a linear backoff (1000 * (attempt
+ 1)) producing 1s,2s,3s; change it to exponential backoff by computing the
delay as 1000 * (2 ** attempt) so retries wait 1s,2s,4s etc.; update the
setTimeout call in the block that references attempt (the Promise wrapping
setTimeout) to use the new exponential formula and keep the existing await/new
Promise structure intact.
- Around line 419-438: The outer fire-and-forget promise starting with
ensureWorkerRunning() can reject and cause an unhandled rejection; wrap the
whole chain in a terminal catch (or convert to an async IIFE with try/catch) so
any rejection from ensureWorkerRunning() is handled. Specifically, after the
ensureWorkerRunning().then(... ) chain attach a .catch(error => {
logger.debug('TRANSCRIPT', 'Conversation observation failed', { error: error
instanceof Error ? error.message : String(error) }); }) or wrap the logic in
(async () => { const ready = await ensureWorkerRunning(); if (!ready) return;
... })().catch(...), ensuring you reference ensureWorkerRunning and
workerHttpRequest/Conversation observation logging in the handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d9dc503-4594-491b-b55a-13b131e6c448
📒 Files selected for processing (2)
plugin/scripts/worker-service.cjssrc/services/transcripts/processor.ts
…ction guard - Change retry backoff from linear to exponential (1s, 2s, 4s) - Wrap fire-and-forget observe chain in async IIFE with terminal catch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewSolid architecture overall — the 3-layer progressive search design is clean, the fire-and-forget pattern for TITANS is correct, and the FileTailer crash-safety fix (offset after processing) is a meaningful correctness improvement. A few issues worth addressing: Bugs / Correctness1. In const sessionDbId = store.createSDKSession(contentSessionId, project || 'unknown', '', undefined);This is semantically wrong — the goal is to find an existing session, but 2. The TITANS prompt template mentions: ...but only 3. In if (!contentSessionId || !promptNumber || !text) {
Code Quality4. It's defined identically in both 5. chunks.push(text.substring(i, i + chunkSize));This cuts mid-word and mid-sentence. For semantic vector search, this noticeably degrades recall — a key phrase like 6. It just calls 7. import { basename, join, resolve } from 'path';
Architecture / Performance8. The background processing runs inside the route handler after 9. This is intentional per the requirements (Bun is mandatory), but the migration removes Node.js compatibility for Positives worth keeping
🤖 Generated with Claude Code |
thedotmack
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
All requested changes addressed — CodeRabbit CI passed clean on final push
The metadata column was stored correctly in the database but never delivered to the viewer through either the SSE streaming path or the REST pagination path. This adds metadata to ObservationSSEPayload, includes it in the broadcastObservation call in ResponseProcessor, and adds o.metadata to the PaginationHelper SELECT query. Also includes bug fixes from adversarial review: - GeminiAgent/OpenRouterAgent: push to processingMessageMeta (was missing) - SessionSearch: guard metadata filters to observations table only - SessionStore: add missing indexes to migration 27 - MigrationRunner: restart-safe migration via PRAGMA table_info check - ObservationCard: HTTP/HTTPS protocol allowlist for source URLs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: Layer 3 Transcript Storage, TITANS Observer, and Build FixesGood architectural direction overall — the 3-layer progressive search model is clean, and reusing existing 🔴 Critical1. Missing
|
| # | Severity | Issue |
|---|---|---|
| 1 | 🔴 Critical | getSessionByContentId() missing — runtime crash |
| 2 | 🟠 High | Fire-and-forget silently loses conversation observations |
| 3 | 🟠 High | OneShotQuery provider fallthrough hides real errors |
| 4 | 🟡 Medium | No validation on exchange structure/length |
| 5 | 🟡 Medium | Unvalidated Chroma query parameter |
| 6 | 🟡 Medium | Null safety gap after session lookup |
| 7 | 🟡 Medium | Retry loop doesn't skip permanent failures |
| 8 | 🟡 Medium | Migration not transactional, silent deduplication |
| 9 | 🟢 Low | Incomplete XML escaping in prompt builder |
| 10 | 🟢 Low | chunkText() splits without word boundaries |
| 11 | 🟢 Low | Built artifacts in diff make review hard |
Issue #1 is a blocker — the conversation observe endpoint will crash on every call. Issues #2-3 mean failures in the TITANS observer will be invisible in production. Recommend fixing those before merging.
🤖 Generated with Claude Code
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Closing during backlog consolidation. The work from this PR is re-filed as issue #2025 for proper prioritization. |
2 participants
Summary
get_transcript_segmentMCP tool for retrieving original conversation context from ChromaDB, traversal-only via observation IDsglobwithBun.Globto fix lru-cache/CJS incompatibility, addedbunto esbuild externals, fixedOneShotQueryEnvManager importFiles changed
src/services/sync/ChromaSync.ts— transcript Chroma methodssrc/services/transcripts/processor.ts— conversation exchange tracking, flush to Chromasrc/services/worker/http/routes/SessionRoutes.ts— Layer 3 HTTP handlerssrc/services/worker/OneShotQuery.ts— TITANS one-shot LLM queriessrc/servers/mcp-server.ts— get_transcript_segment tool registrationsrc/services/sqlite/SessionStore.ts— getContentSessionIdByMemoryId bridgesrc/utils/tag-stripping.ts— stripTranscriptPrivacyTags, chunkTextscripts/build-hooks.js— addedbunto externalssrc/services/transcripts/watcher.ts— glob → Bun.Glob migrationTest plan
GET /api/search?query=...returns resultsPOST /api/observations/batchreturns full observationsPOST /api/transcript/segmentreturns{chunks, prompt_number, mode}POST /api/transcript/storestores transcript chunksPOST /api/sessions/conversation-observereturns 202 acceptednpm run build-and-sync🤖 Generated with Claude Code