feat(tape): add durable execution journal - #2104
Conversation
|
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:
📝 WalkthroughWalkthroughThe pull request adds a durable Execution Journal for DeepChat runs and side-effecting tools. It introduces strict event persistence, UUID run identities, recovery classification, fail-closed dispatch ordering, terminal-state validation, tape reconciliation, and mutation-boundary callbacks across tools and services. ChangesExecution Journal and runtime lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/memory/services/rowMutations.ts (1)
296-301: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThread
beforeMutationthrough the manual edit provenance path.
updateMemoryContentrunsapplyManualContentEditinside the memory transaction atsrc/main/memory/services/managementService.ts:634, butapplyManualContentEditcallsresolveProvenancewithout that callback. Provenance hits can runrekeyProvenancebefore the caller’s new commit boundary runs. AddbeforeMutation?: () => voidtoapplyManualContentEdit, pass it intoresolveProvenance, and update the manual-edit call sites viamanagement.ts/ports.🤖 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/memory/services/rowMutations.ts` around lines 296 - 301, Thread the optional beforeMutation callback through applyManualContentEdit and its manual-edit ports/call sites, including the managementService updateMemoryContent path. Pass it to resolveProvenance in the newKey branch before resolveManualEditFold, preserving the callback through the management.ts layers so provenance mutations wait for the caller’s commit boundary.src/main/orchestration/liveDelegationService.ts (1)
719-742: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
interruptcan return success without invokingbeforeMutation.If no active turn exists and the latest persisted turn is already terminal, the method returns the inspected detail at line 741.
beforeMutationnever runs, so no dispatch fact is committed for a tool call that reports success. This matches the early-return gap inBackgroundExecSessionManager.kill. I consolidate the required change in a separate comment.🤖 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/orchestration/liveDelegationService.ts` around lines 719 - 742, Update the inactive branch of interrupt to invoke beforeMutation even when no active persisted turn requires finishing, including when the latest turn is terminal or absent. Preserve the existing finishTurn path and return inspection result, ensuring successful tool calls still commit the dispatch fact before returning.
🧹 Nitpick comments (18)
src/main/agent/deepchat/runtime/deferredToolExecutor.ts (1)
556-576: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRecording
completedfor a failed post-dispatch tool call reads as a false positive.When the tool throws after dispatch, the code commits an error outcome and then a
completed/tool_resultterminal. The run terminal therefore states success while the outcome fact states failure. The error state is recoverable from the outcome fact, so this is not a data loss, but a distinctstopReasonkeeps the terminal fact self-describing.♻️ Proposed adjustment
if (runStartedCommitted && !terminalCommitAttempted) { commitRunTerminal( dispatchCommitted - ? { outcome: 'completed', stopReason: 'tool_result' } + ? { outcome: 'completed', stopReason: 'tool_error', errorMessage: errorText } : { outcome: 'error', stopReason: 'pre_dispatch_error', errorMessage: errorText } ) }🤖 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/agent/deepchat/runtime/deferredToolExecutor.ts` around lines 556 - 576, Update the run-terminal commit in the error-handling path around commitRunTerminal so post-dispatch tool failures no longer use outcome 'completed' with stopReason 'tool_result'. Preserve the existing pre-dispatch error branch, and assign the appropriate distinct error stop reason for failures after dispatch while retaining the committed error outcome.test/main/session/runtimeIntegration.test.ts (1)
82-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the runtimeIntegration tape append mock idempotent.
DeepChatTapeEntriesTable.appendInternaldedupes idempotent appends usingprovenance_key, while this fixture appends every input and derivesentry_idfromtapeEntries.length + 1. If production retries a journal event with the same provenance key, this mock can create duplicate rows with non-matching entry IDs unlessappendalso returns the existing provenance-key match foridempotentinputs.🤖 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/session/runtimeIntegration.test.ts` around lines 82 - 124, Update the runtimeIntegration tape append mock’s append implementation to handle idempotent inputs by checking tapeEntries for an existing row with the same provenance_key and returning that existing entry without appending. Preserve the current append-and-entry_id generation behavior for non-idempotent inputs and new provenance keys.test/main/agent/deepchat/runtime/dispatch.test.ts (1)
573-578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the
ExecutionJournalErrorargument list to the 100-column limit.Line 575 is about 107 columns. Oxfmt formats this file at 100 columns.
♻️ Proposed fix
const commitDispatch = vi.fn((input) => { if (input.toolName === 'invalid') { - throw new ExecutionJournalError('normalizedArguments must be JSON serializable.', 'invalid_fact') + throw new ExecutionJournalError( + 'normalizedArguments must be JSON serializable.', + 'invalid_fact' + ) }As per coding guidelines: "Follow Oxfmt formatting: single quotes, no semicolons, and a 100-column width."
🤖 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/agent/deepchat/runtime/dispatch.test.ts` around lines 573 - 578, Reformat the ExecutionJournalError constructor call inside commitDispatch so its arguments wrap within the 100-column limit, while preserving the existing single-quote style and behavior.Source: Coding guidelines
test/main/agent/deepchat/runtime/compactionService.test.ts (1)
1071-1075: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the contribution exists before checking the absent property.
findreturnsundefinedwhen nosummary_checkpointcontribution exists.expect(undefined).not.toHaveProperty('sourceEntryIds')then passes. The test would also pass if the checkpoint stops emitting the contribution.♻️ Proposed fix
- expect( - unrelatedCheckpoint.contributions.find( - (contribution) => contribution.reason === 'summary_checkpoint' - ) - ).not.toHaveProperty('sourceEntryIds') + const unrelatedContribution = unrelatedCheckpoint.contributions.find( + (contribution) => contribution.reason === 'summary_checkpoint' + ) + expect(unrelatedContribution).toBeDefined() + expect(unrelatedContribution).not.toHaveProperty('sourceEntryIds')🤖 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/agent/deepchat/runtime/compactionService.test.ts` around lines 1071 - 1075, Update the assertion for the summary_checkpoint contribution to first require that the find result is defined, then assert that the contribution does not have sourceEntryIds. Preserve the existing lookup on unrelatedCheckpoint.contributions and ensure the test fails when the contribution is missing.test/main/tape/executionJournal.test.ts (1)
612-661: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the database in a
finallyblock.Line 660 calls
db.close()only on the success path. If any assertion between Line 618 and Line 659 throws, the connection stays open. The three siblingitIfSqlitetests at Lines 663, 691, and 719 already usetry/finally. Apply the same pattern here.♻️ Proposed change
itIfSqlite('queries only journal events through the dedicated SQLite index', () => { const db = new DatabaseCtor(':memory:') - const table = new DeepChatExecutionJournalStore(db) - table.createTable() - const service = new ExecutionJournalService(() => table) - - table.appendEvent({ sessionId: 'session-1', name: 'context/example', data: { value: 1 } }) - // ... remaining assertions ... - db.close() + try { + const table = new DeepChatExecutionJournalStore(db) + table.createTable() + const service = new ExecutionJournalService(() => table) + + table.appendEvent({ sessionId: 'session-1', name: 'context/example', data: { value: 1 } }) + // ... remaining assertions unchanged ... + } finally { + db.close() + } })🤖 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/tape/executionJournal.test.ts` around lines 612 - 661, Wrap the database setup and assertions in the itIfSqlite test in a try/finally block, moving db.close() into finally so it runs on both success and assertion failure. Preserve the existing test behavior and use the db resource created at the start of the test.test/main/skill/skillExecutionService.test.ts (1)
267-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet the
fsmock state explicitly in the negative cwd test.This test depends on
fs.existsSyncreturning a falsy value. It does not set that value. The sibling test at Line 294 setsexistsSynctotrue. If mock state leaks between tests, this test stops exercising the working-directory guard and can pass or fail for the wrong reason. Set the mock explicitly so the test is self-contained.♻️ Proposed change
it('does not commit dispatch when the resolved spawn cwd is unusable', async () => { + vi.mocked(fs.existsSync).mockReturnValue(false) const plan = { command: 'python', args: ['/skills/ocr/scripts/run.py'],🤖 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/skill/skillExecutionService.test.ts` around lines 267 - 279, Update the test “does not commit dispatch when the resolved spawn cwd is unusable” to explicitly mock fs.existsSync as falsy before execution, ensuring it independently exercises the working-directory guard regardless of state from sibling tests.test/main/tool/agentTools/agentMemoryTools.test.ts (1)
127-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the argument assertion out of the
beforeMutationcallback.Lines 129-133 call
expectinsidebeforeMutation. The handler under test treats a throw frombeforeMutationas a commit failure and rejects the call. The second half of this test proves that behavior. If the argument assertion fails, the resulting error propagates as a rejected handler call, not as a clear assertion failure, and Line 140 never runs.Capture the arguments and assert after the call.
💚 Proposed fix to keep assertion failures visible
- const beforeMutation = vi.fn((args) => { - order.push('commit') - expect(args).toEqual({ - content: 'repo uses pnpm', - kind: 'semantic', - importance: 0.7 - }) - }) + const beforeMutation = vi.fn(() => { + order.push('commit') + }) await handler.call(MEMORY_TOOL_NAMES.remember, { content: ' repo uses pnpm ' }, 'conv-1', { beforeMutation }) + expect(beforeMutation).toHaveBeenCalledWith({ + content: 'repo uses pnpm', + kind: 'semantic', + importance: 0.7 + }) expect(order).toEqual(['commit', 'target'])🤖 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/tool/agentTools/agentMemoryTools.test.ts` around lines 127 - 140, Update the beforeMutation callback in the remember handler test to capture its args and record the commit order without asserting there. After handler.call completes, assert the captured arguments alongside the existing order expectation, preserving the separate test behavior for callback-thrown commit failures.src/main/tape/application/factPersistence.ts (1)
451-453: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getBySessionloads the whole session tape on each record replacement.When a record replacement carries tool facts,
buildTapeToolRevisionIndexreads every entry of the session. Long sessions accumulate thousands of entries, so each user message edit performs a full-session read and a full fingerprint pass.Consider scoping the read to the message. The revision index is keyed by
tool_call:{messageId}:{toolCallId}, so only entries belonging torecord.idare ever consulted.♻️ Sketch of a narrower read
const toolInputs = options.revisionKind === 'record' ? buildTapeToolFactInputs(record) : [] const toolRevisionIndex = - toolInputs.length > 0 ? buildTapeToolRevisionIndex(table.getBySession(record.sessionId)) : null + toolInputs.length > 0 + ? buildTapeToolRevisionIndex(table.getToolEntriesByMessageId(record.sessionId, record.id)) + : nullThis requires a new store method that filters on
json_extract(payload_json, '$.messageId').🤖 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/tape/application/factPersistence.ts` around lines 451 - 453, Replace the full-session lookup in the tool-fact branch around buildTapeToolRevisionIndex with a message-scoped store query keyed by record.id. Add and use a store method that filters entries via json_extract(payload_json, '$.messageId'), preserving the existing toolInputs and revision-index behavior while avoiding getBySession for record replacements.src/main/tape/ports/storage.ts (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
listEventsByNamesout ofTapeEntryStore.The only production caller passes
ExecutionJournalPersistenceStore, and the method is declared on bothTapeEntryStoreandExecutionJournalPersistenceStoredespite this store port’s note that strict journal persistence is intentionally absent fromTapeEntryStore. Keep the method only on the journal port, or on a small local interface, so generic entry-store fakes do not need journal-only behavior.🤖 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/tape/ports/storage.ts` at line 23, Remove listEventsByNames from the TapeEntryStore interface and retain it on ExecutionJournalPersistenceStore or a focused local interface used by its production caller. Update affected typing and fakes so generic TapeEntryStore implementations no longer require this journal-specific method.src/main/tape/infrastructure/sqlite/tapeEntryStore.ts (1)
1080-1091: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer the tool-order projection until after search filtering.
projectedTapePayloadSqlruns two nestedNOT EXISTSscans for eachtool_call/tool_resultcandidate, and those select-list expressions are not guarded by theORDER BY ... LIMIT 20in this query. On large matches, materialize the limited rows first in an inner query, then apply thepayload_jsonproject in an outer query.🤖 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/tape/infrastructure/sqlite/tapeEntryStore.ts` around lines 1080 - 1091, The query should defer the expensive projectedTapePayloadSql calculation until after search filtering and the ORDER BY ... LIMIT 20 selection. Restructure the relevant tape-entry query so an inner query materializes the limited candidate rows and an outer query computes payload_json via projectedTapePayloadSql, preserving all returned columns and ordering behavior.src/main/desktop/browser/YoBrowserToolHandler.ts (1)
43-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant call branches.
Both branches call the same presenter method.
runIdandbeforeDispatchare optional parameters, so passingundefinedis equivalent to omitting them. The conditionrunId || beforeDispatchadds no behavior.♻️ Suggested simplification
const beforeDispatch = beforeInvoke ? () => beforeInvoke({ url }) : undefined return JSON.stringify( - runId || beforeDispatch - ? await this.presenter.loadUrl( - sessionId, - url, - undefined, - undefined, - 'agent', - runId, - beforeDispatch - ) - : await this.presenter.loadUrl(sessionId, url, undefined, undefined, 'agent') + await this.presenter.loadUrl( + sessionId, + url, + undefined, + undefined, + 'agent', + runId, + beforeDispatch + ) )Apply the same change to the
cdp_sendbranch at Lines 73-83.Also applies to: 72-83
🤖 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/desktop/browser/YoBrowserToolHandler.ts` around lines 43 - 56, Remove the conditional loadUrl call branches in the browser navigation handler, including both the shown branch and the cdp_send branch. Call presenter.loadUrl once in each case, passing the optional runId and beforeDispatch arguments directly; preserve the existing sessionId, URL, and agent arguments and JSON serialization.src/main/agent/deepchat/runtime/dispatch.ts (2)
1882-1908: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
commitOutcomefor the final staged result.Lines 1882-1908 repeat the logic of
commitOutcomeat Lines 1621-1635: commit the outcome, recordcommittedOutcome, then release projections. Two copies can diverge when the commit rules change.♻️ Suggested consolidation
- const stagedResult = commitDispatchedToolOutcome( - { + const outcome = commitOutcome({ + kind: 'staged', + stagedResult: { toolCallId: completedToolCall.id,Then build the remaining fields inside the same object and use
outcome.stagedResultfor the later block updates and the return value.🤖 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/agent/deepchat/runtime/dispatch.ts` around lines 1882 - 1908, Replace the duplicated final staged-result commit block with the existing commitOutcome helper used earlier in the dispatch flow. Pass the complete outcome fields through that helper, then use its returned stagedResult for committedOutcome assignment, projection release, subsequent block updates, and the function return value.
243-249: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMove the outcome-committed invariant before
toolResults.fitBatch.The check runs after the
awaitat Lines 230-242.fitBatchcan offload content and perform I/O for results that the invariant then rejects. Running the check first avoids that work and fails faster on corruption.♻️ Suggested reordering
if (stagedResults.length > 0) { + for (const stagedResult of stagedResults) { + if (stagedResult.operation && !stagedResult.outcomeCommitted) { + throw new ExecutionJournalCorruptionError( + `Dispatched tool result was not committed for ${stagedResult.toolCallId}.` + ) + } + } const fittedResults = await toolResults.fitBatch({🤖 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/agent/deepchat/runtime/dispatch.ts` around lines 243 - 249, Move the outcome-committed validation loop over stagedResults before the toolResults.fitBatch call and its await in the dispatch flow. Keep the existing ExecutionJournalCorruptionError condition and message unchanged, so invalid staged results fail before fitBatch performs offloading or I/O.src/main/desktop/browser/YoBrowserPresenter.ts (1)
161-178: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUI events fire before the dispatch commit runs.
beforeDispatchruns insidenavigateUntilDomReadyat Line 169.ensureSessionBrowserState,updateOwner,ensurePreviewHost, andemitOpenRequestedrun earlier. If the journal commit throws, the session browser state and the preview host already exist and the renderer already receivedbrowser.open.requestedfor a navigation that never starts. The same pattern applies tosendCdpCommandat Lines 476-495.The target action still stays behind the commit, so the journal contract holds. If you want the observable state to match the journal, invoke
beforeDispatchbeforeemitOpenRequested.🤖 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/desktop/browser/YoBrowserPresenter.ts` around lines 161 - 178, Move the beforeDispatch invocation in the navigation flow so it completes before emitOpenRequested and other observable setup in the surrounding method, while preserving the target action’s existing post-commit execution. Apply the same ordering correction to sendCdpCommand: run its beforeDispatch step before emitting UI events or creating observable state, without moving the actual command dispatch ahead of the journal commit.src/main/agent/shared/process/backgroundExecSessionManager.ts (1)
961-966: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse of the reconciliation constant changes the cleanup meaning.
COMPLETED_SESSION_RECONCILIATION_MSnow controls two unrelated policies: the proxy reconciliation period and the local auto-removal age for finished sessions. A later tuning of the reconciliation period silently changes how long finished sessions stay readable. Consider a separate named constant for the local retention age.♻️ Proposed separate constant
+const COMPLETED_SESSION_RETENTION_MS = 5 * 60 * 1000} else if ( session.status !== 'running' && - now - session.lastAccessedAt > COMPLETED_SESSION_RECONCILIATION_MS + now - session.lastAccessedAt > COMPLETED_SESSION_RETENTION_MS ) {🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 961 - 966, Introduce a separate named constant for local retention of finished sessions and use it in the expired-session condition within the background session cleanup flow, replacing COMPLETED_SESSION_RECONCILIATION_MS while leaving the proxy reconciliation policy unchanged.src/main/mcp/toolManager.ts (1)
823-823: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState the commit flag directly.
access?.commitDispatch !== undefinedre-derives a condition that is already known at this point. Assigntrueand keep the flag meaning "a dispatch fact was committed".♻️ Proposed change
- try { - access?.commitDispatch?.({ + const commitDispatch = access?.commitDispatch + try { + commitDispatch?.({ toolName: finalName, toolSource: 'mcp', normalizedArguments: preparedArgs.args, target: { serverName: toolServerName, originalName, ...(ownerPluginId ? { ownerPluginId } : {}) } }) - dispatchCommitted = access?.commitDispatch !== undefined + dispatchCommitted = commitDispatch !== undefined🤖 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/mcp/toolManager.ts` at line 823, Update the assignment to dispatchCommitted in the surrounding dispatch handling logic to set it directly to true, preserving its meaning as indicating that a dispatch fact was committed.src/main/skill/skillTools.ts (1)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
optionsunconditionally tomanageDraftSkill.
handleSkillManage()initializesoptionsas{}, andSkillService.manageDraftSkill()also defaults the third parameter to{}, so the default-no-op branch and the explicit-optionsbranch both call the same default. Remove the conditional forwarding unless a test requires the two-argument call form.🤖 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/skill/skillTools.ts` around lines 98 - 100, Update handleSkillManage() to pass the initialized options object unconditionally as the third argument to SkillService.manageDraftSkill(). Remove the beforeMutation conditional and preserve the existing manageDraftSkill invocation behavior.src/main/memory/services/writeCoordinator.ts (1)
1146-1157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated tombstone-check block into a helper.
The tombstone check (build the scoped provenance key, then call
hasTombstoneForClaim) repeats near-identically at Line 1146 (coordinateWriteAttempt), Line 1675 (tryExplicitRelearn, negated form), and Line 1749 (directAddMemory). Extract a private helper, for exampleisClaimTombstoned(agentId, kind, content, scope), and call it from all three sites. This keeps the tombstone check consistent if the identity fields change later.♻️ Proposed helper extraction
+ private isClaimTombstoned( + agentId: string, + kind: NormalizedMemoryCandidate['kind'], + content: string, + scope: MemoryScope + ): boolean { + return this.ports.repository.hasTombstoneForClaim({ + agentId, + kind, + content, + provenanceKey: buildScopedMemoryProvenanceKey(agentId, kind, content, scope), + scope + }) + }Then replace each inline check, for example at Line 1146:
- if ( - !duplicate && - this.ports.repository.hasTombstoneForClaim({ - agentId, - kind: normalized.kind, - content, - provenanceKey: buildScopedMemoryProvenanceKey(agentId, normalized.kind, content, scope), - scope - }) - ) { + if (!duplicate && this.isClaimTombstoned(agentId, normalized.kind, content, scope)) { return { action: 'noop', reason: 'forgotten' } }🤖 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/memory/services/writeCoordinator.ts` around lines 1146 - 1157, Extract the repeated tombstone lookup into a private isClaimTombstoned helper that builds the scoped provenance key and calls repository.hasTombstoneForClaim. Replace the inline checks in coordinateWriteAttempt, tryExplicitRelearn, and directAddMemory with this helper, preserving each caller’s existing negation and outcome behavior.
🤖 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 `@docs/architecture/tape-system.md`:
- Line 113: Update the execution/tool_outcome row in the architecture
documentation to remove cancel from the listed outcome categories, leaving only
success/error so it matches the isError-based domain model and T1/T2 abort
contract.
In `@src/main/agent/deepchat/runtime/dispatch.ts`:
- Around line 1636-1658: The failPostDispatchPermission recovery path must
reconcile the committed stagedResult before rethrowing invalid_fact. Update the
surrounding settleToolBatch/deferred-interaction handling to project and insert
committedOutcome into the assistant content/transcript when
execution.journalFailure.outcomeCommitted is set, using the committed staged
result and avoiding duplicate insertion.
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 1025-1047: The list method’s host-unavailable fallback omits
completed sessions, causing finished records to disappear. Update the fallback
projection in list to include matching entries from completedSessions alongside
active session metadata, while preserving host responses and reconciliation
behavior when hostListSucceeded is true.
In `@src/main/mcp/toolManager.ts`:
- Around line 907-909: Update the abort-handling branch around isAbortError and
dispatchCommitted so post-T1 aborted calls are classified as
indeterminate/parked rather than rethrown. Ensure dispatchCommitFailed
suppresses committed aborts consistently with McpService.callTool, while
preserving rethrow behavior for pre-dispatch failures.
In `@src/main/tape/application/executionJournalService.ts`:
- Around line 163-167: Bound the recovery scan in classifyRecoveryCandidates by
passing an explicit row limit or recent session/time-window constraint to
listEventsByNames, rather than retrieving every matching execution-journal row.
Preserve classification through classifyExecutionJournalRows while ensuring
recovery processing remains limited to the intended recent scope.
In `@src/main/tape/domain/canonicalJson.ts`:
- Around line 3-25: Update normalizeForStableJson and its callers to eliminate
the unused preservePrototypeKeys branch, or make stableJsonStringify
consistently pass and honor it when prototype-preserving behavior is required.
Ensure the chosen behavior is explicit for the accumulator object, including how
parsed keys such as __proto__ are handled, and remove the parameter threading if
the legacy {} behavior remains.
In `@src/main/tape/infrastructure/sqlite/tapeEntryStore.ts`:
- Around line 61-63: Bound recovery journal scans by applying a maximum row or
run-count limit at the recovery reader boundary, preferably in
listEventsByNames() or its call from classifyRecoveryCandidates(), before rows
reach classifyExecutionJournalRows(). Preserve classification behavior for rows
within the bound and prevent unbounded streaming when execution names span
multiple sessions.
In `@src/main/tool/agentTools/agentFileSystemHandler.ts`:
- Around line 845-858: Update writeFile and the corresponding mutation methods
to avoid reusing a validated pathname after beforeMutation. Open the validated
parent directory via a descriptor, perform a no-follow file operation relative
to that descriptor, and ensure the write cannot follow a replaced file or parent
symlink outside the allowed directory.
In `@test/main/agent/deepchat/runtime/process.test.ts`:
- Line 1859: Rename the test case describing the provider AbortError to state
that the abort signal is inactive, not active. Update only the `it` description
in the test around `records a provider AbortError`; preserve its existing
assertions and behavior.
In `@test/main/agent/shared/process/backgroundExecSessionManager.test.ts`:
- Around line 300-313: Update the test covering rejected local session preflight
so its third kill call targets the same missing session, or remove it from this
case. If retaining a successful kill assertion, move it to a separate test using
a running session and assert the intended commit behavior there.
In `@test/main/session/data/tapeFork.test.ts`:
- Around line 401-427: In the test around service.mergeFork, add a positive
precondition after appendExecutionJournalEvent asserting that the
execution/run_started entry exists for fork.forkSessionId via the existing table
query. Keep the subsequent parent-session assertion unchanged so the test
verifies both fork journal creation and non-copying during merge.
---
Outside diff comments:
In `@src/main/memory/services/rowMutations.ts`:
- Around line 296-301: Thread the optional beforeMutation callback through
applyManualContentEdit and its manual-edit ports/call sites, including the
managementService updateMemoryContent path. Pass it to resolveProvenance in the
newKey branch before resolveManualEditFold, preserving the callback through the
management.ts layers so provenance mutations wait for the caller’s commit
boundary.
In `@src/main/orchestration/liveDelegationService.ts`:
- Around line 719-742: Update the inactive branch of interrupt to invoke
beforeMutation even when no active persisted turn requires finishing, including
when the latest turn is terminal or absent. Preserve the existing finishTurn
path and return inspection result, ensuring successful tool calls still commit
the dispatch fact before returning.
---
Nitpick comments:
In `@src/main/agent/deepchat/runtime/deferredToolExecutor.ts`:
- Around line 556-576: Update the run-terminal commit in the error-handling path
around commitRunTerminal so post-dispatch tool failures no longer use outcome
'completed' with stopReason 'tool_result'. Preserve the existing pre-dispatch
error branch, and assign the appropriate distinct error stop reason for failures
after dispatch while retaining the committed error outcome.
In `@src/main/agent/deepchat/runtime/dispatch.ts`:
- Around line 1882-1908: Replace the duplicated final staged-result commit block
with the existing commitOutcome helper used earlier in the dispatch flow. Pass
the complete outcome fields through that helper, then use its returned
stagedResult for committedOutcome assignment, projection release, subsequent
block updates, and the function return value.
- Around line 243-249: Move the outcome-committed validation loop over
stagedResults before the toolResults.fitBatch call and its await in the dispatch
flow. Keep the existing ExecutionJournalCorruptionError condition and message
unchanged, so invalid staged results fail before fitBatch performs offloading or
I/O.
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 961-966: Introduce a separate named constant for local retention
of finished sessions and use it in the expired-session condition within the
background session cleanup flow, replacing COMPLETED_SESSION_RECONCILIATION_MS
while leaving the proxy reconciliation policy unchanged.
In `@src/main/desktop/browser/YoBrowserPresenter.ts`:
- Around line 161-178: Move the beforeDispatch invocation in the navigation flow
so it completes before emitOpenRequested and other observable setup in the
surrounding method, while preserving the target action’s existing post-commit
execution. Apply the same ordering correction to sendCdpCommand: run its
beforeDispatch step before emitting UI events or creating observable state,
without moving the actual command dispatch ahead of the journal commit.
In `@src/main/desktop/browser/YoBrowserToolHandler.ts`:
- Around line 43-56: Remove the conditional loadUrl call branches in the browser
navigation handler, including both the shown branch and the cdp_send branch.
Call presenter.loadUrl once in each case, passing the optional runId and
beforeDispatch arguments directly; preserve the existing sessionId, URL, and
agent arguments and JSON serialization.
In `@src/main/mcp/toolManager.ts`:
- Line 823: Update the assignment to dispatchCommitted in the surrounding
dispatch handling logic to set it directly to true, preserving its meaning as
indicating that a dispatch fact was committed.
In `@src/main/memory/services/writeCoordinator.ts`:
- Around line 1146-1157: Extract the repeated tombstone lookup into a private
isClaimTombstoned helper that builds the scoped provenance key and calls
repository.hasTombstoneForClaim. Replace the inline checks in
coordinateWriteAttempt, tryExplicitRelearn, and directAddMemory with this
helper, preserving each caller’s existing negation and outcome behavior.
In `@src/main/skill/skillTools.ts`:
- Around line 98-100: Update handleSkillManage() to pass the initialized options
object unconditionally as the third argument to SkillService.manageDraftSkill().
Remove the beforeMutation conditional and preserve the existing manageDraftSkill
invocation behavior.
In `@src/main/tape/application/factPersistence.ts`:
- Around line 451-453: Replace the full-session lookup in the tool-fact branch
around buildTapeToolRevisionIndex with a message-scoped store query keyed by
record.id. Add and use a store method that filters entries via
json_extract(payload_json, '$.messageId'), preserving the existing toolInputs
and revision-index behavior while avoiding getBySession for record replacements.
In `@src/main/tape/infrastructure/sqlite/tapeEntryStore.ts`:
- Around line 1080-1091: The query should defer the expensive
projectedTapePayloadSql calculation until after search filtering and the ORDER
BY ... LIMIT 20 selection. Restructure the relevant tape-entry query so an inner
query materializes the limited candidate rows and an outer query computes
payload_json via projectedTapePayloadSql, preserving all returned columns and
ordering behavior.
In `@src/main/tape/ports/storage.ts`:
- Line 23: Remove listEventsByNames from the TapeEntryStore interface and retain
it on ExecutionJournalPersistenceStore or a focused local interface used by its
production caller. Update affected typing and fakes so generic TapeEntryStore
implementations no longer require this journal-specific method.
In `@test/main/agent/deepchat/runtime/compactionService.test.ts`:
- Around line 1071-1075: Update the assertion for the summary_checkpoint
contribution to first require that the find result is defined, then assert that
the contribution does not have sourceEntryIds. Preserve the existing lookup on
unrelatedCheckpoint.contributions and ensure the test fails when the
contribution is missing.
In `@test/main/agent/deepchat/runtime/dispatch.test.ts`:
- Around line 573-578: Reformat the ExecutionJournalError constructor call
inside commitDispatch so its arguments wrap within the 100-column limit, while
preserving the existing single-quote style and behavior.
In `@test/main/session/runtimeIntegration.test.ts`:
- Around line 82-124: Update the runtimeIntegration tape append mock’s append
implementation to handle idempotent inputs by checking tapeEntries for an
existing row with the same provenance_key and returning that existing entry
without appending. Preserve the current append-and-entry_id generation behavior
for non-idempotent inputs and new provenance keys.
In `@test/main/skill/skillExecutionService.test.ts`:
- Around line 267-279: Update the test “does not commit dispatch when the
resolved spawn cwd is unusable” to explicitly mock fs.existsSync as falsy before
execution, ensuring it independently exercises the working-directory guard
regardless of state from sibling tests.
In `@test/main/tape/executionJournal.test.ts`:
- Around line 612-661: Wrap the database setup and assertions in the itIfSqlite
test in a try/finally block, moving db.close() into finally so it runs on both
success and assertion failure. Preserve the existing test behavior and use the
db resource created at the start of the test.
In `@test/main/tool/agentTools/agentMemoryTools.test.ts`:
- Around line 127-140: Update the beforeMutation callback in the remember
handler test to capture its args and record the commit order without asserting
there. After handler.call completes, assert the captured arguments alongside the
existing order expectation, preserving the separate test behavior for
callback-thrown commit failures.
🪄 Autofix
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 Plus
Run ID: 9c5feb4f-c8ed-4a38-b00a-a8792de45001
📒 Files selected for processing (109)
docs/architecture/agent-system.mddocs/architecture/durable-execution-journal/plan.mddocs/architecture/durable-execution-journal/spec.mddocs/architecture/durable-execution-journal/tasks.mddocs/architecture/session-management.mddocs/architecture/tape-system.mdsrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/runtime/contextContributions.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/interactionParkingRegistry.tssrc/main/agent/deepchat/runtime/process.tssrc/main/agent/deepchat/runtime/runTerminalProjectionError.tssrc/main/agent/deepchat/runtime/sessionLifecycleCoordinator.tssrc/main/agent/deepchat/runtime/toolAdapters.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/types.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/app/composition.tssrc/main/desktop/browser/BrowserTab.tssrc/main/desktop/browser/YoBrowserPresenter.tssrc/main/desktop/browser/YoBrowserToolHandler.tssrc/main/mcp/index.tssrc/main/mcp/toolManager.tssrc/main/memory/data/tables/agentMemory.tssrc/main/memory/index.tssrc/main/memory/ports.tssrc/main/memory/services/managementService.tssrc/main/memory/services/rowMutations.tssrc/main/memory/services/writeCoordinator.tssrc/main/orchestration/liveDelegationRepository.tssrc/main/orchestration/liveDelegationService.tssrc/main/scheduler/index.tssrc/main/session/data/database.tssrc/main/session/data/tables/deepchatMessages.tssrc/main/session/data/transcript.tssrc/main/skill/index.tssrc/main/skill/skillExecutionService.tssrc/main/skill/skillTools.tssrc/main/tape/application/executionJournalService.tssrc/main/tape/application/factPersistence.tssrc/main/tape/application/factService.tssrc/main/tape/application/forkService.tssrc/main/tape/application/reconcilerService.tssrc/main/tape/application/sessionTape.tssrc/main/tape/domain/canonicalJson.tssrc/main/tape/domain/effectiveView.tssrc/main/tape/domain/executionJournal.tssrc/main/tape/domain/facts.tssrc/main/tape/domain/viewManifest.tssrc/main/tape/infrastructure/sqlite/tapeEntryStore.tssrc/main/tape/infrastructure/sqlite/tapeSearchProjectionStore.tssrc/main/tape/ports/application.tssrc/main/tape/ports/capabilities.tssrc/main/tape/ports/storage.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentFileSystemHandler.tssrc/main/tool/agentTools/agentImageGenerationTool.tssrc/main/tool/agentTools/agentMemoryTools.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/agentTools/chatSettingsTools.tssrc/main/tool/agentTools/cronJobTool.tssrc/main/tool/agentTools/liveDelegationTool.tssrc/main/tool/index.tssrc/main/tool/runtimePorts.tssrc/shared/types/core/mcp.tssrc/shared/types/mcp.tssrc/shared/types/skill.tssrc/shared/types/tool.d.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/compactionService.test.tstest/main/agent/deepchat/runtime/deferredToolExecutor.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/deepchat/runtime/sessionLifecycleCoordinator.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/desktop/browser/BrowserTab.test.tstest/main/desktop/browser/YoBrowserPresenter.test.tstest/main/desktop/browser/YoBrowserToolHandler.test.tstest/main/mcp/mcpService.test.tstest/main/mcp/toolManager.test.tstest/main/memory/support/memoryFakes.tstest/main/memory/writeCoordinator.test.tstest/main/orchestration/liveDelegationRepository.test.tstest/main/orchestration/liveDelegationService.test.tstest/main/session/data/settings.test.tstest/main/session/data/tapeFacts.test.tstest/main/session/data/tapeFork.test.tstest/main/session/data/tapeRecall.test.tstest/main/session/data/tapeReconciler.test.tstest/main/session/data/tapeTestHarness.tstest/main/session/data/tapeViewManifest.test.tstest/main/session/data/transcript.test.tstest/main/session/runtimeIntegration.test.tstest/main/skill/skillExecutionService.test.tstest/main/skill/skillService.test.tstest/main/tape/canonicalJson.test.tstest/main/tape/executionJournal.test.tstest/main/tape/executionJournalCrash.test.tstest/main/tape/fixtures/executionJournalCrashWorker.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/agentTools/agentImageGenerationTool.test.tstest/main/tool/agentTools/agentMemoryTools.test.tstest/main/tool/agentTools/agentToolManagerRead.test.tstest/main/tool/agentTools/chatSettingsTools.test.tstest/main/tool/toolService.test.ts
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 (1)
src/main/agent/shared/process/backgroundExecSessionManager.ts (1)
1186-1196: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate completed-session metadata after a successful clear.
clearresets the host output length and offload state.completedSessionskeeps the old values. If a laterlistcall cannot reach the host, it reports cleared output as still available.Proposed fix
try { await this.request('clear', [conversationId, sessionId]) + if (completed) { + this.completedSessions.set(sessionId, { + ...completed, + outputLength: 0, + offloaded: false, + lastAccessedAt: Date.now() + }) + } } catch (error) {🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 1186 - 1196, Update the successful clear path in the method containing request('clear') to refresh the matching completedSessions metadata after the request completes. Reset the completed session’s host output length and offload state to the values representing a cleared session, while preserving the existing missing-utility-session cleanup and error propagation behavior.
🧹 Nitpick comments (2)
test/main/desktop/browser/YoBrowserPresenter.test.ts (1)
459-460: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the CDP suppression assertion.
Line 460 only excludes the exact argument pair
('Page.reload', {}). If the implementation dispatchesPage.reloadwithundefinedparams, the assertion still passes. Assert on the method name only, or reusecdpCommandObserveras the other CDP test does.♻️ Proposed change
expect(sendToAllWindowsMock).not.toHaveBeenCalled() - expect(webContents?.debugger.sendCommand).not.toHaveBeenCalledWith('Page.reload', {}) + expect(webContents?.debugger.sendCommand).not.toHaveBeenCalledWith( + 'Page.reload', + expect.anything() + ) + expect(webContents?.debugger.sendCommand).not.toHaveBeenCalledWith('Page.reload')🤖 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/desktop/browser/YoBrowserPresenter.test.ts` around lines 459 - 460, Strengthen the CDP suppression assertion in the relevant YoBrowserPresenter test by verifying that debugger.sendCommand was never called with the method name Page.reload, regardless of its parameters. Replace the exact argument-pair check or reuse cdpCommandObserver consistently with the other CDP test.test/main/evals/nativeAgent/harness.ts (1)
256-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the argument parse in the scripted
callTool.Line 261 parses
request.function.argumentswithout a guard. If a scenario produces a truncated or empty argument string,JSON.parsethrows insidecallTool. The harness then reports a tool error instead of the scripted behavior, and the failure reason is hard to read. The productionToolServiceuses a tolerant parser for the same input.♻️ Proposed change
+const parseToolArguments = (raw: string): Record<string, unknown> => { + try { + const parsed = JSON.parse(raw) as unknown + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) + ? (parsed as Record<string, unknown>) + : {} + } catch { + return {} + } +}options?.commitDispatch?.({ toolName: request.function.name, toolSource: 'agent', - normalizedArguments: JSON.parse(request.function.arguments) as Record<string, unknown>, + normalizedArguments: parseToolArguments(request.function.arguments), target: {🤖 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/evals/nativeAgent/harness.ts` around lines 256 - 266, Update the scripted callTool implementation to parse request.function.arguments through the same tolerant parsing behavior used by production ToolService, so empty or truncated JSON does not throw and replace the scripted tool behavior. Preserve the existing normalizedArguments dispatch contract and fallback behavior for invalid arguments.
🤖 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/desktop/browser/YoBrowserPresenter.ts`:
- Around line 155-183: Mark the session as dispatched at the start of the inner
project function in the projectDispatch flow, before emitWindowCreated can
throw. Preserve createdEventPublished as the guard, and ensure emitWindowUpdated
can use the dispatched state to lazily emit the creation event for sessions
whose initial projection failed.
In `@test/main/desktop/browser/YoBrowserPresenter.test.ts`:
- Around line 379-385: Reset sendToAllWindowsMock between tests so the custom
implementation installed in the affected tests does not leak into later tests.
Update the relevant beforeEach setup to use mockReset() for this mock, or
explicitly restore its default implementation after each test that calls
mockImplementation; preserve the existing event-order assertions.
---
Outside diff comments:
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 1186-1196: Update the successful clear path in the method
containing request('clear') to refresh the matching completedSessions metadata
after the request completes. Reset the completed session’s host output length
and offload state to the values representing a cleared session, while preserving
the existing missing-utility-session cleanup and error propagation behavior.
---
Nitpick comments:
In `@test/main/desktop/browser/YoBrowserPresenter.test.ts`:
- Around line 459-460: Strengthen the CDP suppression assertion in the relevant
YoBrowserPresenter test by verifying that debugger.sendCommand was never called
with the method name Page.reload, regardless of its parameters. Replace the
exact argument-pair check or reuse cdpCommandObserver consistently with the
other CDP test.
In `@test/main/evals/nativeAgent/harness.ts`:
- Around line 256-266: Update the scripted callTool implementation to parse
request.function.arguments through the same tolerant parsing behavior used by
production ToolService, so empty or truncated JSON does not throw and replace
the scripted tool behavior. Preserve the existing normalizedArguments dispatch
contract and fallback behavior for invalid arguments.
🪄 Autofix
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 Plus
Run ID: 248f2d94-0ab1-42da-bb47-69ede285ddcc
📒 Files selected for processing (19)
docs/architecture/tape-system.mdsrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/desktop/browser/BrowserTab.tssrc/main/desktop/browser/YoBrowserPresenter.tssrc/main/tape/domain/canonicalJson.tssrc/main/tape/ports/storage.tstest/main/agent/deepchat/runtime/compactionService.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/desktop/browser/YoBrowserPresenter.test.tstest/main/evals/nativeAgent/harness.tstest/main/evals/nativeAgent/nativeAgentBehavior.eval.test.tstest/main/session/data/tapeFork.test.tstest/main/session/runtimeIntegration.test.tstest/main/skill/skillExecutionService.test.tstest/main/tape/executionJournal.test.tstest/main/tool/agentTools/agentMemoryTools.test.ts
💤 Files with no reviewable changes (1)
- src/main/tape/ports/storage.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- test/main/session/data/tapeFork.test.ts
- test/main/tool/agentTools/agentMemoryTools.test.ts
- src/main/desktop/browser/BrowserTab.ts
- src/main/tape/domain/canonicalJson.ts
- test/main/skill/skillExecutionService.test.ts
- test/main/session/runtimeIntegration.test.ts
- test/main/agent/deepchat/runtime/compactionService.test.ts
- test/main/agent/shared/process/backgroundExecSessionManager.test.ts
- docs/architecture/tape-system.md
- test/main/tape/executionJournal.test.ts
- test/main/agent/deepchat/runtime/process.test.ts
- src/main/agent/deepchat/runtime/dispatch.ts
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 (3)
src/main/agent/shared/process/backgroundExecSessionManager.ts (3)
1410-1411: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve completed metadata across an unexpected utility-process exit.
handleHostExitclearscompletedSessionsafter moving active sessions tocrashedSessions. Sessions that completed before the exit then disappear fromlistwhile the host is unavailable. The local completed-session fallback cannot help after this clear.Keep completed records across an unexpected exit. Let successful reconciliation or explicit cleanup remove them.
shutdownalready clears them for intentional shutdown.Proposed fix
this.hostReady = null this.activeSessions.clear() - this.completedSessions.clear() this.stopCompletedSessionReconciliation() this.rejectPendingRequests(error)🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 1410 - 1411, Update handleHostExit to stop clearing completedSessions when moving active sessions to crashedSessions, preserving completed records while the utility process is unavailable. Keep their removal under successful reconciliation or explicit cleanup, and leave shutdown’s existing completedSessions.clear() behavior unchanged.
1169-1177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn before dispatching
killfor completed sessions.
completedis terminal metadata, but this path still invokesbeforeMutationand sends akillrequest. This leavesdone,error, andkilledsessions on the termination path. Return whencompletedis present, before the callback. Useremovefor cleanup. Otherwise, the callback can persist a dispatch for an operation that cannot terminate the session.🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 1169 - 1177, Update the session termination flow around getCompletedSession to return immediately when completed metadata exists, before invoking beforeMutation or dispatching the kill request. Use the completed-session cleanup remove operation rather than delete, while preserving the existing crashed-session early return and active-session kill behavior.
1051-1057: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProtect new completions from stale reconciliation.
At Lines [1051]-[1057], the loop iterates the live
completedSessionsmap after the host-list request resolves. A concurrentpoll,log,waitForCompletionOrYield, orgetCompletionResultcall can add a completed session after the host snapshot was taken. This loop can then delete the new terminal metadata.Snapshot
completedSessionsbefore the await. Delete an entry only when its current value is the same object that was present in the snapshot.Proposed fix
async list(conversationId: string): Promise<SessionMeta[]> { + const completedAtListStart = new Map(this.completedSessions) const active = Array.from(this.activeSessions.values()) .filter((session) => session.conversationId === conversationId) .map((session) => this.toActiveSessionMeta(session)) @@ - for (const [sessionId, session] of this.completedSessions) { + for (const [sessionId, session] of completedAtListStart) { if (session.conversationId === conversationId && !hostSessionIds.has(sessionId)) { - this.completedSessions.delete(sessionId) + if (this.completedSessions.get(sessionId) === session) { + this.completedSessions.delete(sessionId) + } } }🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 1051 - 1057, Update the host reconciliation flow around the host-list await and the completedSessions cleanup loop to snapshot completedSessions before awaiting the host request. Iterate that snapshot, and delete an entry only if it still exists with the same session object as the snapshot value, preserving concurrent completions added or replaced after the snapshot.
🤖 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 `@test/main/evals/nativeAgent/harness.ts`:
- Around line 240-253: Update parseToolArguments to validate both direct
JSON.parse and jsonrepair fallback results, returning {} unless the parsed value
is a non-null plain object; only return validated object values so
normalizedArguments remains compatible with commitDispatch.
---
Outside diff comments:
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 1410-1411: Update handleHostExit to stop clearing
completedSessions when moving active sessions to crashedSessions, preserving
completed records while the utility process is unavailable. Keep their removal
under successful reconciliation or explicit cleanup, and leave shutdown’s
existing completedSessions.clear() behavior unchanged.
- Around line 1169-1177: Update the session termination flow around
getCompletedSession to return immediately when completed metadata exists, before
invoking beforeMutation or dispatching the kill request. Use the
completed-session cleanup remove operation rather than delete, while preserving
the existing crashed-session early return and active-session kill behavior.
- Around line 1051-1057: Update the host reconciliation flow around the
host-list await and the completedSessions cleanup loop to snapshot
completedSessions before awaiting the host request. Iterate that snapshot, and
delete an entry only if it still exists with the same session object as the
snapshot value, preserving concurrent completions added or replaced after the
snapshot.
🪄 Autofix
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 Plus
Run ID: 6519ee02-2afe-4388-9e88-959461cb4f53
📒 Files selected for processing (5)
src/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/desktop/browser/YoBrowserPresenter.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/desktop/browser/YoBrowserPresenter.test.tstest/main/evals/nativeAgent/harness.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/main/agent/shared/process/backgroundExecSessionManager.test.ts
- src/main/desktop/browser/YoBrowserPresenter.ts
zerob13
left a comment
There was a problem hiding this comment.
Complexity and test-value review
I would not merge this PR in its current shape. The crash-boundary direction is useful and the
boundary/real-process tests are valuable, but the implementation is not yet the smallest coherent
change: the PR is 111 files and +11,382/-827, mixes a durable journal with unrelated Context Tape
repairs, and puts an unbounded amount of historical work on application startup.
Findings
-
[P1] Recovery performs an unbounded full-history scan on the synchronous startup path.
ExecutionJournalService.classifyRecoveryCandidates()reads every journal event through
listEventsByNames()(src/main/tape/application/executionJournalService.ts:163), whose SQL has
no run-state predicate, checkpoint, or limit
(src/main/tape/infrastructure/sqlite/tapeEntryStore.ts:642). The classifier then materializes
every Run and reparses every payload; outcome parsing also hashes the persisted response again.
MAX_STARTUP_RECOVERY_DETAILSonly bounds logging, not scanning. With response payloads allowed
up to 256,000 characters, launch cost grows with total lifetime journal bytes rather than unresolved
work. This also contradicts the PR description's “bounded recovery scanning” claim.The minimal V1 is to query only Runs without a terminal fact and load their related facts. If a
permanent global corruption audit is required, run it outside synchronous startup or add a durable
acknowledgement/retention design before landing this path. -
[P2] V1 persists outcome data that no production recovery path consumes.
ExecutionToolOutcomeFactstoresresponseTextandoffloadPath
(src/main/tape/domain/executionJournal.ts:112), but the classifier reduces an outcome to
{ entryId }(:708,:762) and startup recovery only marks the pending message interrupted.
There is no production reader that replays or projects either field. Normal cleanup can also delete
the referenced offload file, leaving a durable stale path. This duplicates sensitive tool output,
increases database size and startup hashing cost, and implements a future recovery format without
implementing that recovery.For V1, persist only the operation identity,
isError, and a response hash if idempotency needs it.
Add durable response recovery in a separate change when there is an actual consumer and retention
contract. -
[P2] Split the Context Tape ordering fixes out of the journal PR.
Acceptance criteria 18-20 in
docs/architecture/durable-execution-journal/spec.md:254cover compaction provenance, message
replacement semantics, tool ordering, and backfill revisions. Commitsf0e045ed3,58c0189a1,
ande718512bdaccount for+989/-149lines and are independently reviewable Context Tape fixes;
they are not required to establish T0/T1/T2/T3 journal boundaries. Combining them makes failures
harder to attribute and forces journal reviewers to validate a second storage model. Move those
commits and their tests to a separate PR. -
[P3] The missing-dispatch gate should be a type contract, not a runtime branch and test.
ToolExecutionPort.execute()still accepts optional options, so
createToolExecutionPort()checksoptions?.commitDispatchat runtime
(src/main/agent/deepchat/runtime/toolAdapters.ts:61). The only production callers of this port are
normal dispatch and deferred dispatch, and both always provide the callback. Define the internal
execution options asToolCallOptions & { commitDispatch: ToolDispatchCommit }, require them on
ToolExecutionPort.execute, and delete this guard plus its dedicated test. Lower-level/direct tool
APIs may keep optional callbacks where non-journal execution is a real use case.
Gate assessment
The T1/T2 prerequisite checks, duplicate-operation checks, local preflight-before-T1 checks, and the
host-transaction prohibition protect different failure windows; they are not mutually redundant.
The redundant gate is the runtime capability-presence check above because the internal port can make
the invalid state unrepresentable. The broader callback plumbing is costly (new beforeMutation /
beforeInvoke variants span 23 source files), but most of it exists because side-effect preflight is
owned deep in each subsystem. Replacing it safely would require one uniform prepare/commit/invoke
contract, not another layer of booleans.
Test assessment
The test total (+6,065/-138) exceeds the production-source addition (+4,455/-676), but the ratio
alone is not the problem. Keep the native crash tests and the dispatch/outcome/terminal ordering tests;
they protect observable durability contracts. The avoidable bulk is mainly implementation proof:
deepChatAgentHarness.test.ts:2317-2460spends about 140 lines on log caps, priority, and string
sanitization through full harness construction. Build classifier reports directly and collapse this
to one table-driven diagnostic test.deepChatAgentHarness.test.ts:2928-3006repeats nearly the same fallback-error scenario for typed,
corruption, and untyped errors. One table-driven contract test can preserve subtype/cause assertions.- The dedicated missing-
commitDispatchadapter test disappears when the port type is corrected. - Keep the background-process ownership and Context Tape regression tests with the separate PRs that
own those behaviors; they are useful tests, but they inflate and couple this journal review.
Minimal-cut summary
src/main/tape/application/executionJournalService.ts:L163: scale: replace the lifetime scan with an unresolved-Run recovery query.
src/main/tape/domain/executionJournal.ts:L112: yagni: remove raw response/offload persistence until recovery consumes it.
src/main/agent/deepchat/runtime/toolAdapters.ts:L61: type: require commitDispatch on the internal execution port and delete the runtime gate.
docs/architecture/durable-execution-journal/spec.md:L254: scope: move Context Tape compaction/order repair to its own PR.
docs/architecture/durable-execution-journal/plan.md:L1: artifact: keep the final spec; drop the 375-line plan and 111-line task ledger, moving open follow-ups to an issue.
net(PR scope): at least -1,300 lines possible before parser and test deduplication.
Validation at f9098f6513a317dcd265709d0959935f0c1ec0ff: required GitHub checks are green; focused
Vitest coverage passed 36 tests, with 11 environment-gated native crash cases skipped locally.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/architecture/durable-execution-journal/tasks.md`:
- Line 120: Update the checklist item at the diff’s task entry to replace the
instruction to “push normally” with “do not push,” while preserving the required
validation, diff review, and commit steps.
In `@src/main/tape/infrastructure/sqlite/tapeEntryStore.ts`:
- Around line 67-69: Update the migration around the
idx_deepchat_tape_entries_session_provenance definition in
src/main/tape/infrastructure/sqlite/tapeEntryStore.ts so legacy duplicate Tape
rows cannot cause SQLite startup failure: scope the uniqueness constraint to the
applicable Journal events, or add and verify a migration that resolves duplicate
provenance keys before enabling it. In
docs/architecture/durable-execution-journal/spec.md lines 211-212, revise the
compatibility claim if existing Tape rows are no longer guaranteed to satisfy
this constraint.
🪄 Autofix
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 Plus
Run ID: 76167ca8-53f6-4a05-9553-c06c2af5699b
📒 Files selected for processing (18)
docs/architecture/durable-execution-journal/plan.mddocs/architecture/durable-execution-journal/spec.mddocs/architecture/durable-execution-journal/tasks.mdsrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/tape/application/executionJournalService.tssrc/main/tape/domain/executionJournal.tssrc/main/tape/infrastructure/sqlite/tapeEntryStore.tssrc/main/tape/ports/storage.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/deferredToolExecutor.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/session/data/tapeTestHarness.tstest/main/session/runtimeIntegration.test.tstest/main/tape/executionJournal.test.tstest/main/tape/executionJournalCrash.test.ts
💤 Files with no reviewable changes (1)
- test/main/agent/deepchat/runtime/toolAdapters.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- test/main/session/runtimeIntegration.test.ts
- test/main/session/data/tapeTestHarness.ts
- test/main/tape/executionJournal.test.ts
- test/main/agent/deepchat/runtime/deferredToolExecutor.test.ts
- test/main/agent/deepchat/runtime/dispatch.test.ts
- src/main/tape/application/executionJournalService.ts
- src/main/tape/domain/executionJournal.ts
- docs/architecture/durable-execution-journal/plan.md
- src/main/agent/deepchat/runtime/deferredToolExecutor.ts
- test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts
- src/main/tape/ports/storage.ts
Summary
Add a durable Execution Journal to the existing Tape store so DeepChat can reliably classify tool execution and Run state after process crashes.
Changes
not_dispatched,completed,indeterminate, orcorruption.Failure Semantics
This does not claim exactly-once execution for arbitrary external systems. A crash after remote acceptance but before outcome persistence can still be indeterminate.
The journal instead guarantees that uncertainty has a durable, classifiable boundary and is never silently converted into success or automatic replay.
Summary by CodeRabbit