feat(agent): add session tape memory#1669
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an append-only session tape: DB schema and table, tape service, fact writers/readers and effective view, integration into message/session/compaction flows, presenter/tool APIs and MCP tools, fork/handoff management, tests, and README acknowledgments. ChangesTape Core Implementation
Tape Integration with Session State and Compaction
Tape Public APIs and Tool Integration
Tape Tests and Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
src/main/presenter/agentRuntimePresenter/messageStore.ts (2)
166-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord compaction message updates in tape as well.
This only captures the initial compaction message state. If the row is created as
'compacting'and later updated to'compacted'inupdateCompactionMessage(), tape-backed history stays on the stale version and reconstruction can surface the wrong compaction status. Add a replacement/live tape append after the update path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/messageStore.ts` around lines 166 - 183, createCompactionMessage currently appends the new compaction row to the live tape but updateCompactionMessage does not, leaving tape with a stale 'compacting' record; modify updateCompactionMessage (the function that updates the deepchatMessagesTable row to 'compacted') to also call appendLiveTapeFacts with the updated message id (or append a replacement event) after the DB update so the tape reflects the new content/metadata built via buildCompactionBlocks and buildCompactionMetadata; ensure you use the same id and include updated JSON content and metadata in the replacement entry so reconstruction reads the latest compaction status.
371-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake tape retractions and row deletes atomic.
Both delete paths append the retraction fact before deleting the message rows and related secondary tables. If any later delete fails, tape says the message was removed while SQLite still contains it, which breaks the tape-as-source-of-truth model introduced by this PR. Wrap the tape append and all related deletes in one SQLite transaction.
Also applies to: 390-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/messageStore.ts` around lines 371 - 387, The deleteMessage path currently calls appendMessageRetractionToTape(...) and then performs multiple deletes on sqlitePresenter.deepchat* tables (deepchatSearchDocumentsTable, deepchatAssistantBlocksTable, deepchatUserMessageLinksTable, deepchatUserMessageFilesTable, deepchatUserMessagesTable, deepchatMessageTracesTable, deepchatMessageSearchResultsTable, deepchatMessagesTable) separately, which can leave tape inconsistent if a later delete fails; modify deleteMessage to perform the appendMessageRetractionToTape call and all subsequent deletes inside a single SQLite transaction (use the sqlitePresenter's existing transaction/connection API—e.g. begin/commit or a runInTransaction helper) so that either all operations commit or all roll back; apply the same transactional change to the other deletion path referenced around lines 390-412.src/main/presenter/agentRuntimePresenter/sessionStore.ts (1)
203-240:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve compare-and-set semantics when no tape anchor is passed.
The optional
tapeAnchorcreates a broken success path here: without it, this does a non-atomic read/compare/write and only updatesdeepchatSessionsTable, whilegetSummaryState()still prefers the previous tape anchor. That meansapplied: truecan be returned even though later reads keep surfacing the old summary state, and concurrent callers can both win. Either require an anchor for every successful update, or restore the DB-side CAS and keep tape state in sync in the same transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/sessionStore.ts` around lines 203 - 240, The compareAndSetSummaryState implementation currently only runs the DB transaction when a tapeAnchor is provided, causing non-atomic read/compare/write when tapeAnchor is absent; fix by always running applyUpdate inside a DB transaction when a database is available (use this.sqlitePresenter.getDatabase and db.transaction(applyUpdate)()), and only fall back to non-transactional applyUpdate when no DB is present; keep conditional appendAnchor logic (this.sqlitePresenter.deepchatTapeEntriesTable.appendAnchor) inside applyUpdate so tape updates happen in the same transaction when present, and ensure the returned applied/currentState value is based on the transaction result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/presenter/agentRuntimePresenter/tapeFacts.ts`:
- Around line 287-307: The appended "message/retracted" event is including a
source object which auto-generates a provenance key and can cause
unique-provenance collisions for rapid non-idempotent retractions; in the call
to table.appendEvent (the event with name 'message/retracted' and idempotent:
false), remove the source property (do not supply source) so no provenance is
auto-generated for this non-idempotent retraction. Ensure the rest of the
payload (sessionId, data, meta, idempotent: false) remains unchanged.
In `@src/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.ts`:
- Around line 388-403: The code sets task.tapeFinalized = true before calling
the runtime APIs (this.runtimePort.mergeSubagentTape / discardSubagentTape),
which will suppress retry if those calls fail; move the assignment so it only
happens after the awaited mergeSubagentTape or discardSubagentTape resolves
successfully (i.e., set task.tapeFinalized = true inside the try block after the
await completes for either the completed or non-completed branch) so failed
merges/discards leave tapeFinalized false and can be retried; update references
in subagentOrchestratorTool (task, runtimePort.mergeSubagentTape,
runtimePort.discardSubagentTape, parentSessionId, task.sessionId, meta)
accordingly.
In `@src/main/presenter/toolPresenter/index.ts`:
- Around line 638-650: The buildTapePrompt function currently unconditionally
documents `tape_info`, `tape_search`, and `tape_handoff`; change it to inspect
the provided tools array (MCPToolDefinition[] tools) and only include
descriptions for the tools that are present (use tool.function.name to detect
each). Build the `names` list from tools as you do now, then conditionally
append the specific descriptive lines for `tape_info`, `tape_search`, and
`tape_handoff` only when a matching tool name exists in tools; keep the header
("## Tape Tools") and join the resulting lines into the final string returned by
buildTapePrompt.
In `@test/main/presenter/agentRuntimePresenter/tapeService.test.ts`:
- Around line 129-133: The mock search implementation (vi.fn named search) trims
the incoming query into normalizedQuery but still proceeds to filter entries
even when normalizedQuery is empty; update the mock so that after computing
normalizedQuery it immediately returns an empty array when normalizedQuery ===
'' to match the real table contract, preserving the existing options.limit
handling and the subsequent entries.filter logic for non-empty queries
(references: search, normalizedQuery, entries).
---
Outside diff comments:
In `@src/main/presenter/agentRuntimePresenter/messageStore.ts`:
- Around line 166-183: createCompactionMessage currently appends the new
compaction row to the live tape but updateCompactionMessage does not, leaving
tape with a stale 'compacting' record; modify updateCompactionMessage (the
function that updates the deepchatMessagesTable row to 'compacted') to also call
appendLiveTapeFacts with the updated message id (or append a replacement event)
after the DB update so the tape reflects the new content/metadata built via
buildCompactionBlocks and buildCompactionMetadata; ensure you use the same id
and include updated JSON content and metadata in the replacement entry so
reconstruction reads the latest compaction status.
- Around line 371-387: The deleteMessage path currently calls
appendMessageRetractionToTape(...) and then performs multiple deletes on
sqlitePresenter.deepchat* tables (deepchatSearchDocumentsTable,
deepchatAssistantBlocksTable, deepchatUserMessageLinksTable,
deepchatUserMessageFilesTable, deepchatUserMessagesTable,
deepchatMessageTracesTable, deepchatMessageSearchResultsTable,
deepchatMessagesTable) separately, which can leave tape inconsistent if a later
delete fails; modify deleteMessage to perform the appendMessageRetractionToTape
call and all subsequent deletes inside a single SQLite transaction (use the
sqlitePresenter's existing transaction/connection API—e.g. begin/commit or a
runInTransaction helper) so that either all operations commit or all roll back;
apply the same transactional change to the other deletion path referenced around
lines 390-412.
In `@src/main/presenter/agentRuntimePresenter/sessionStore.ts`:
- Around line 203-240: The compareAndSetSummaryState implementation currently
only runs the DB transaction when a tapeAnchor is provided, causing non-atomic
read/compare/write when tapeAnchor is absent; fix by always running applyUpdate
inside a DB transaction when a database is available (use
this.sqlitePresenter.getDatabase and db.transaction(applyUpdate)()), and only
fall back to non-transactional applyUpdate when no DB is present; keep
conditional appendAnchor logic
(this.sqlitePresenter.deepchatTapeEntriesTable.appendAnchor) inside applyUpdate
so tape updates happen in the same transaction when present, and ensure the
returned applied/currentState value is based on the transaction result.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 204ee85e-048e-41ff-bdc2-f6e9a6da5fb0
📒 Files selected for processing (33)
README.jp.mdREADME.mdREADME.zh.mdsrc/main/presenter/agentRuntimePresenter/compactionService.tssrc/main/presenter/agentRuntimePresenter/contextBuilder.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/sessionStore.tssrc/main/presenter/agentRuntimePresenter/tapeEffectiveView.tssrc/main/presenter/agentRuntimePresenter/tapeFacts.tssrc/main/presenter/agentRuntimePresenter/tapeService.tssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/databaseSecurityPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/schemaCatalog.tssrc/main/presenter/sqlitePresenter/tables/deepchatTapeEntries.tssrc/main/presenter/toolPresenter/agentTools/agentTapeTools.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/agentTools/index.tssrc/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.tssrc/main/presenter/toolPresenter/index.tssrc/main/presenter/toolPresenter/runtimePorts.tssrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/agent-session.presenter.d.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/compactionService.test.tstest/main/presenter/agentRuntimePresenter/sessionStoreTape.test.tstest/main/presenter/agentRuntimePresenter/tapeService.test.tstest/main/presenter/sqlitePresenter.migrationSqlSplit.test.tstest/main/presenter/sqlitePresenter/deepchatTapeEntriesTable.test.tstest/main/presenter/toolPresenter/agentTools/agentTapeTools.test.tstest/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/agentRuntimePresenter/index.ts (1)
261-261: 💤 Low valuePotential stale entry in
activeSteerPendingInputIdsafter cancellation.The
activeSteerPendingInputIdsmap is cleaned up indestroySessionand after successful drain indrainPendingQueueIfPossible, but not incancelGeneration. If a user cancels generation while a steer input is queued but not yet consumed, the map entry will remain stale until the session is destroyed.Consider adding cleanup in
cancelGeneration:♻️ Proposed fix
async cancelGeneration(sessionId: string): Promise<void> { const activeGeneration = this.activeGenerations.get(sessionId) if (activeGeneration) { // ... existing code ... } else { const controller = this.abortControllers.get(sessionId) if (controller) { controller.abort() this.abortControllers.delete(sessionId) } } this.abortDeferredToolAbortControllers(sessionId) this.clearActiveProviderPermissionsForSession(sessionId) + this.activeSteerPendingInputIds.delete(sessionId) this.setSessionStatus(sessionId, 'idle') }Also applies to: 421-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` at line 261, The map activeSteerPendingInputIds can retain stale entries when a generation is cancelled; update cancelGeneration to remove any associated steer pending id(s) from activeSteerPendingInputIds (the same cleanup that exists in destroySession and drainPendingQueueIfPossible). Locate cancelGeneration and ensure it deletes the entry keyed by the generation/input id (or any mapping used there) from activeSteerPendingInputIds and/or runs the same cleanup helper used elsewhere so queued steer inputs are removed immediately on cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Line 261: The map activeSteerPendingInputIds can retain stale entries when a
generation is cancelled; update cancelGeneration to remove any associated steer
pending id(s) from activeSteerPendingInputIds (the same cleanup that exists in
destroySession and drainPendingQueueIfPossible). Locate cancelGeneration and
ensure it deletes the entry keyed by the generation/input id (or any mapping
used there) from activeSteerPendingInputIds and/or runs the same cleanup helper
used elsewhere so queued steer inputs are removed immediately on cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a32d4b-028e-477f-979e-09d616ca02b8
📒 Files selected for processing (4)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/presenter/agentRuntimePresenter/compactionService.ts (1)
173-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCap serialized handoff state before appending it to the system prompt.
Allowlisting fixed field leakage, but a very large
summaryorreasoncan still bloat prompt tokens and destabilize context assembly. Add a hard size cap (and truncation marker) beforebuildUntrustedPromptBlock.💡 Proposed fix
+const MAX_RECONSTRUCTION_STATE_CHARS = 4000 + export function appendReconstructionAnchorStateSection( systemPrompt: string, anchor: ReconstructionAnchorPromptState | null | undefined ): string { @@ - const stateJson = JSON.stringify( + const rawStateJson = JSON.stringify( { anchor: anchor.name, state: visibleState }, null, 2 ) + const stateJson = + rawStateJson.length > MAX_RECONSTRUCTION_STATE_CHARS + ? `${rawStateJson.slice(0, MAX_RECONSTRUCTION_STATE_CHARS)}\n... [TRUNCATED]` + : rawStateJson🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/compactionService.ts` around lines 173 - 185, The serialized handoff state (stateJson) can be arbitrarily large; before calling buildUntrustedPromptBlock include a hard size cap and truncation marker: serialize visibleState as stateJson, check its length, and if it exceeds the cap (e.g., N chars) truncate it and append an explicit marker like "…[TRUNCATED]" so the anchorSection uses the capped string; update the code around stateJson / buildUntrustedPromptBlock (and any helper that consumes it) to ensure composeSections still receives the truncated payload while preserving anchor.name and the rest of the structure.
🧹 Nitpick comments (1)
test/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.ts (1)
309-340: ⚡ Quick winRestore
console.warnspies via shared cleanup to avoid leak on failed assertions.If an assertion fails before
warnSpy.mockRestore(), the spy can bleed into later tests. PreferafterEach(() => vi.restoreAllMocks())for deterministic cleanup.♻️ Proposed cleanup
-import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' @@ describe('SubagentOrchestratorTool', () => { + afterEach(() => { + vi.restoreAllMocks() + }) @@ - warnSpy.mockRestore() @@ - warnSpy.mockRestore() @@ - warnSpy.mockRestore()Also applies to: 380-459, 473-562
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.ts` around lines 309 - 340, The test creates a console.warn spy (warnSpy) and restores it inline, which can leak if assertions fail; replace per-test inline restores with deterministic shared cleanup by adding an afterEach hook that calls vi.restoreAllMocks() (or vi.resetAllMocks()) so all spies including warnSpy are restored automatically, and ensure the tests referencing SubagentOrchestratorTool.finalizeTaskTape and the mergeSubagentTape mock remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/main/presenter/agentRuntimePresenter/compactionService.ts`:
- Around line 173-185: The serialized handoff state (stateJson) can be
arbitrarily large; before calling buildUntrustedPromptBlock include a hard size
cap and truncation marker: serialize visibleState as stateJson, check its
length, and if it exceeds the cap (e.g., N chars) truncate it and append an
explicit marker like "…[TRUNCATED]" so the anchorSection uses the capped string;
update the code around stateJson / buildUntrustedPromptBlock (and any helper
that consumes it) to ensure composeSections still receives the truncated payload
while preserving anchor.name and the rest of the structure.
---
Nitpick comments:
In
`@test/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.ts`:
- Around line 309-340: The test creates a console.warn spy (warnSpy) and
restores it inline, which can leak if assertions fail; replace per-test inline
restores with deterministic shared cleanup by adding an afterEach hook that
calls vi.restoreAllMocks() (or vi.resetAllMocks()) so all spies including
warnSpy are restored automatically, and ensure the tests referencing
SubagentOrchestratorTool.finalizeTaskTape and the mergeSubagentTape mock remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a14df6b8-de16-422f-9c72-4e51479bfb53
📒 Files selected for processing (14)
src/main/presenter/agentRuntimePresenter/compactionService.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/sessionStore.tssrc/main/presenter/agentRuntimePresenter/tapeFacts.tssrc/main/presenter/toolPresenter/agentTools/agentTapeTools.tssrc/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.tssrc/main/presenter/toolPresenter/index.tstest/main/presenter/agentRuntimePresenter/compactionService.test.tstest/main/presenter/agentRuntimePresenter/messageStore.test.tstest/main/presenter/agentRuntimePresenter/sessionStoreTape.test.tstest/main/presenter/agentRuntimePresenter/tapeService.test.tstest/main/presenter/toolPresenter/agentTools/agentTapeTools.test.tstest/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.tstest/main/presenter/toolPresenter/toolPresenter.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds SQLite-backed, session-scoped tape memory for DeepChat agents, inspired by Bub's tape model; for more on the underlying tape architecture, see tape.systems.
Summary by CodeRabbit