[codex] Persist core-owned interaction state#739
Open
rahulkarajgikar wants to merge 13 commits intotruffle-ai:mainfrom
Open
[codex] Persist core-owned interaction state#739rahulkarajgikar wants to merge 13 commits intotruffle-ai:mainfrom
rahulkarajgikar wants to merge 13 commits intotruffle-ai:mainfrom
Conversation
|
@rahulkarajgikar is attempting to deploy a commit to the Shaunak's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughAdds durable per-session interaction state (message queues, tool preferences, approvals, LLM overrides) with cache-backed stores, wires them into MessageQueueService, ApprovalManager, ToolManager, SessionManager, and agent/TUI paths, and converts many session preference/queue APIs to async to await persistence and restoration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DextoAgent
participant ChatSession
participant MessageQueueService
participant MessageQueueStore
participant ToolManager
participant SessionToolPreferencesStore
participant ApprovalManager
participant SessionApprovalStore
Client->>DextoAgent: start session / queue message
DextoAgent->>ChatSession: create/get session
ChatSession->>MessageQueueService: initialize()
MessageQueueService->>MessageQueueStore: load(sessionId)
MessageQueueStore-->>MessageQueueService: persisted queue
Client->>ChatSession: queueMessage(...)
ChatSession->>MessageQueueService: enqueue(message)
MessageQueueService->>MessageQueueStore: save(sessionId, queue)
MessageQueueStore-->>MessageQueueService: ack
DextoAgent->>ToolManager: setSessionUserAutoApproveTools(sessionId,...)
ToolManager->>SessionToolPreferencesStore: save(sessionId, prefs)
SessionToolPreferencesStore-->>ToolManager: ack
DextoAgent->>ApprovalManager: addPattern(..., sessionId)
ApprovalManager->>SessionApprovalStore: save(sessionId, approvals)
SessionApprovalStore-->>ApprovalManager: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
packages/core/src/llm/executor/turn-executor.integration.test.ts (1)
415-424:⚠️ Potential issue | 🟡 MinorMake the queued follow-up setup deterministic.
void messageQueue.enqueue(...)can race the executor’sawait messageQueue.dequeueAll()termination check. If enqueue now serializes through async persistence, this test can intermittently miss the follow-up message.💡 Suggested fix
if (callCount === 1) { - void messageQueue.enqueue({ - content: [{ type: 'text', text: 'Follow-up question' }], - }); - return createMockStream({ - text: 'First response', - finishReason: 'stop', - }) as unknown as ReturnType<typeof streamText>; + return { + fullStream: (async function* () { + await messageQueue.enqueue({ + content: [{ type: 'text', text: 'Follow-up question' }], + }); + yield* createMockStream({ + text: 'First response', + finishReason: 'stop', + }).fullStream; + })(), + } as unknown as ReturnType<typeof streamText>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/llm/executor/turn-executor.integration.test.ts` around lines 415 - 424, The test's mock for streamText enqueues a follow-up message asynchronously with "void messageQueue.enqueue(...)" which can race with the executor's await messageQueue.dequeueAll(); change the mock to perform the enqueue deterministically by awaiting the enqueue call (i.e., remove "void" and await messageQueue.enqueue(...)) or use a synchronous enqueue helper so the enqueue completes before returning the mock stream in the streamText mock implementation (referencing streamText, messageQueue.enqueue, messageQueue.dequeueAll, callCount, and createMockStream).packages/core/src/utils/service-initializer.ts (1)
248-270:⚠️ Potential issue | 🟠 Major
toolManagerFactorynow bypasses session tool persistence.The new
sessionToolPreferencesStoreis only wired through the defaultnew ToolManager(...)branch. Any advanced/test setup usingoverrides.toolManagerFactorystill builds a manager with the old inputs, so per-session disabled/auto-approve state stops persisting on that supported override path.As per coding guidelines, "Use the
InitializeServicesOptionstype to override only top-level services (e.g.,ContextManager,LLMService,McpManager) for advanced, programmatic, or test scenarios. Do not expose every internal dependency as a code-level override." and "Use top-level service overrides for code/test/advanced scenarios rather than adding every internal dependency toInitializeServicesOptions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/service-initializer.ts` around lines 248 - 270, The overrides.toolManagerFactory branch currently doesn't receive session tool persistence so per-session preferences aren't persisted; update the factory call to pass sessionToolPreferencesStore (same value used in the default new ToolManager(...) call) in its argument object so any custom factory builds a ToolManager wired to session persistence; specifically, add sessionToolPreferencesStore to the object passed to overrides.toolManagerFactory and ensure InitializeServicesOptions documentation/type for toolManagerFactory expects/accepts that property so tests/advanced overrides don't bypass session persistence.packages/tui/src/components/overlays/ToolBrowser.tsx (1)
265-335:⚠️ Potential issue | 🟠 MajorRoll back optimistic tool toggles on persistence failure.
applyToolToggle()now awaits async backend writes after mutating local state. If any of those writes reject, the overlay stays in the optimistic state while the backend keeps the old one, and thevoid applyToolToggle()call site can surface an unhandled rejection.💡 Suggested fix
const applyToolToggle = async (overrideTarget?: 'session' | 'global') => { const targetToolName = scopeToolNameRef.current ?? scopeToolName ?? selectedToolRef.current?.name; if (!targetToolName) return; const effectiveTarget = overrideTarget ?? scopeTargetRef.current; const nextEnabled = scopeNextEnabledRef.current; const previousTools = toolsRef.current; const updatedTools = previousTools.map((tool) => { if (tool.name !== targetToolName) { return tool; } const updatedTool = { ...tool, isEnabled: nextEnabled }; if (!nextEnabled && tool.isAutoApproved) { updatedTool.isAutoApproved = false; } return updatedTool; }); setTools(updatedTools); const updatedSelected = updatedTools.find((tool) => tool.name === targetToolName) ?? null; setSelectedTool(updatedSelected); const autoApprovedTools = updatedTools .filter((tool) => tool.isAutoApproved) .map((tool) => tool.name); - if (sessionId) { - await agent.setSessionAutoApproveTools(sessionId, autoApprovedTools); - } - - const disabledTools = updatedTools - .filter((tool) => !tool.isEnabled) - .map((tool) => tool.name); - - if (effectiveTarget === 'session' && sessionId) { - await agent.setSessionDisabledTools(sessionId, disabledTools); - } else if (effectiveTarget === 'global') { - try { + try { + if (sessionId) { + await agent.setSessionAutoApproveTools(sessionId, autoApprovedTools); + } + + const disabledTools = updatedTools + .filter((tool) => !tool.isEnabled) + .map((tool) => tool.name); + + if (effectiveTarget === 'session' && sessionId) { + await agent.setSessionDisabledTools(sessionId, disabledTools); + } else if (effectiveTarget === 'global') { const { updateAgentPreferences, saveAgentPreferences, agentPreferencesExist } = await import('@dexto/agent-management'); if (agentPreferencesExist(agent.config.agentId)) { await updateAgentPreferences(agent.config.agentId, { @@ } - - agent.setGlobalDisabledTools(disabledTools); - } catch (_error) { - // If we can't persist, still keep session state so user sees effect - if (sessionId) { - await agent.setSessionDisabledTools(sessionId, disabledTools); - } else { - setTools(previousTools); - setSelectedTool( - previousTools.find((tool) => tool.name === targetToolName) ?? null - ); - } + agent.setGlobalDisabledTools(disabledTools); } + + closeScopePrompt(); + } catch (_error) { + setTools(previousTools); + setSelectedTool(previousTools.find((tool) => tool.name === targetToolName) ?? null); } - - closeScopePrompt(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tui/src/components/overlays/ToolBrowser.tsx` around lines 265 - 335, applyToolToggle mutates local state optimistically but doesn’t reliably roll back if any awaited backend write (agent.setSessionAutoApproveTools, setSessionDisabledTools, updateAgentPreferences/saveAgentPreferences) rejects, which can leave UI out-of-sync and surface unhandled rejections; wrap all backend persistence calls in try/catch blocks and on any catch restore previousTools and selectedTool (using previousTools and the previous selectedTool lookup), then rethrow or return the error so callers (void applyToolToggle()) won’t produce unhandled rejections; specifically, keep the optimistic setTools/setSelectedTool as-is, but surround the sequence that computes autoApprovedTools/disabledTools and calls agent.setSessionAutoApproveTools, agent.setSessionDisabledTools, and the `@dexto/agent-management` import/update/save logic with a single try/catch that reverts state via setTools(previousTools) and setSelectedTool(previousSelected) and then throws the caught error.
🧹 Nitpick comments (2)
packages/core/src/approval/session-approval-store.ts (1)
62-67: Consider parallelizing database and cache writes insave().Same observation as
SessionToolPreferencesStore:delete()usesPromise.all()butsave()writes sequentially.♻️ Optional: Parallelize writes for consistency
async save(sessionId: string | undefined, state: SessionApprovalState): Promise<void> { const key = this.buildKey(sessionId); const normalized = SessionApprovalStateSchema.parse(state); - await this.storageManager.getDatabase().set(key, normalized); - await this.storageManager.getCache().set(key, normalized, this.cacheTtlSeconds); + await Promise.all([ + this.storageManager.getDatabase().set(key, normalized), + this.storageManager.getCache().set(key, normalized, this.cacheTtlSeconds), + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/approval/session-approval-store.ts` around lines 62 - 67, The save() method currently writes to the database then the cache sequentially; change it to run both writes in parallel by calling storageManager.getDatabase().set(key, normalized) and storageManager.getCache().set(key, normalized, this.cacheTtlSeconds) inside a Promise.all so both execute concurrently. Keep the existing key construction via buildKey and validation via SessionApprovalStateSchema.parse(state), and await Promise.all to preserve error propagation consistent with delete().packages/core/src/tools/session-tool-preferences-store.ts (1)
52-57: Consider parallelizing database and cache writes insave().The
delete()method usesPromise.all()for concurrent operations (lines 61-64), butsave()writes sequentially. If cache write fails after database succeeds, the data is persisted but the cache remains stale until the next load populates it.♻️ Optional: Parallelize writes for consistency with delete()
async save(sessionId: string, preferences: SessionToolPreferences): Promise<void> { const key = this.buildKey(sessionId); const normalized = SessionToolPreferencesSchema.parse(preferences); - await this.storageManager.getDatabase().set(key, normalized); - await this.storageManager.getCache().set(key, normalized, this.cacheTtlSeconds); + await Promise.all([ + this.storageManager.getDatabase().set(key, normalized), + this.storageManager.getCache().set(key, normalized, this.cacheTtlSeconds), + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/tools/session-tool-preferences-store.ts` around lines 52 - 57, The save() method currently performs sequential writes (database then cache); change it to run both writes concurrently by calling storageManager.getDatabase().set(key, normalized) and storageManager.getCache().set(key, normalized, this.cacheTtlSeconds) inside Promise.all so both operations run in parallel. Update the save() function in SessionToolPreferencesStore (references: save, buildKey, SessionToolPreferencesSchema.parse, storageManager.getDatabase().set, storageManager.getCache().set, cacheTtlSeconds) to await Promise.all([...]) and preserve the normalized value and error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/session/session-manager.ts`:
- Around line 543-544: The code is restoring tool and approval state even when
existingMetadata is absent, causing new sessions to inherit orphaned
session-scoped preferences; modify the fresh-session path in session-manager.ts
so that this.services.toolManager.restoreSessionState(id) and
this.services.approvalManager.restoreSessionState(id) are only called when
existingMetadata indicates a resumed session (e.g., guard the calls with a check
like if (existingMetadata) or an equivalent isResumingSession flag) so new
sessions do not hydrate old session-tool-preferences:* or session-approvals:*
data.
---
Outside diff comments:
In `@packages/core/src/llm/executor/turn-executor.integration.test.ts`:
- Around line 415-424: The test's mock for streamText enqueues a follow-up
message asynchronously with "void messageQueue.enqueue(...)" which can race with
the executor's await messageQueue.dequeueAll(); change the mock to perform the
enqueue deterministically by awaiting the enqueue call (i.e., remove "void" and
await messageQueue.enqueue(...)) or use a synchronous enqueue helper so the
enqueue completes before returning the mock stream in the streamText mock
implementation (referencing streamText, messageQueue.enqueue,
messageQueue.dequeueAll, callCount, and createMockStream).
In `@packages/core/src/utils/service-initializer.ts`:
- Around line 248-270: The overrides.toolManagerFactory branch currently doesn't
receive session tool persistence so per-session preferences aren't persisted;
update the factory call to pass sessionToolPreferencesStore (same value used in
the default new ToolManager(...) call) in its argument object so any custom
factory builds a ToolManager wired to session persistence; specifically, add
sessionToolPreferencesStore to the object passed to overrides.toolManagerFactory
and ensure InitializeServicesOptions documentation/type for toolManagerFactory
expects/accepts that property so tests/advanced overrides don't bypass session
persistence.
In `@packages/tui/src/components/overlays/ToolBrowser.tsx`:
- Around line 265-335: applyToolToggle mutates local state optimistically but
doesn’t reliably roll back if any awaited backend write
(agent.setSessionAutoApproveTools, setSessionDisabledTools,
updateAgentPreferences/saveAgentPreferences) rejects, which can leave UI
out-of-sync and surface unhandled rejections; wrap all backend persistence calls
in try/catch blocks and on any catch restore previousTools and selectedTool
(using previousTools and the previous selectedTool lookup), then rethrow or
return the error so callers (void applyToolToggle()) won’t produce unhandled
rejections; specifically, keep the optimistic setTools/setSelectedTool as-is,
but surround the sequence that computes autoApprovedTools/disabledTools and
calls agent.setSessionAutoApproveTools, agent.setSessionDisabledTools, and the
`@dexto/agent-management` import/update/save logic with a single try/catch that
reverts state via setTools(previousTools) and setSelectedTool(previousSelected)
and then throws the caught error.
---
Nitpick comments:
In `@packages/core/src/approval/session-approval-store.ts`:
- Around line 62-67: The save() method currently writes to the database then the
cache sequentially; change it to run both writes in parallel by calling
storageManager.getDatabase().set(key, normalized) and
storageManager.getCache().set(key, normalized, this.cacheTtlSeconds) inside a
Promise.all so both execute concurrently. Keep the existing key construction via
buildKey and validation via SessionApprovalStateSchema.parse(state), and await
Promise.all to preserve error propagation consistent with delete().
In `@packages/core/src/tools/session-tool-preferences-store.ts`:
- Around line 52-57: The save() method currently performs sequential writes
(database then cache); change it to run both writes concurrently by calling
storageManager.getDatabase().set(key, normalized) and
storageManager.getCache().set(key, normalized, this.cacheTtlSeconds) inside
Promise.all so both operations run in parallel. Update the save() function in
SessionToolPreferencesStore (references: save, buildKey,
SessionToolPreferencesSchema.parse, storageManager.getDatabase().set,
storageManager.getCache().set, cacheTtlSeconds) to await Promise.all([...]) and
preserve the normalized value and error propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d224d042-2e23-4646-9a30-160c7e4c9154
📒 Files selected for processing (23)
.changeset/four-beers-march.mdpackages/cli/src/cli/cloud-chat.tspackages/core/src/agent/DextoAgent.tspackages/core/src/approval/manager.test.tspackages/core/src/approval/manager.tspackages/core/src/approval/session-approval-store.tspackages/core/src/llm/executor/turn-executor.integration.test.tspackages/core/src/llm/executor/turn-executor.tspackages/core/src/llm/services/factory.tspackages/core/src/llm/services/types.tspackages/core/src/llm/services/vercel.tspackages/core/src/session/chat-session.test.tspackages/core/src/session/chat-session.tspackages/core/src/session/message-queue-store.tspackages/core/src/session/message-queue.test.tspackages/core/src/session/message-queue.tspackages/core/src/session/session-manager.integration.test.tspackages/core/src/session/session-manager.test.tspackages/core/src/session/session-manager.tspackages/core/src/tools/session-tool-preferences-store.tspackages/core/src/tools/tool-manager.tspackages/core/src/utils/service-initializer.tspackages/tui/src/components/overlays/ToolBrowser.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/llm/executor/turn-executor.integration.test.ts (1)
419-443:⚠️ Potential issue | 🟡 MinorConsider clarifying the fire-and-forget enqueue pattern.
The
void messageQueue.enqueue(...)pattern on line 424 discards the returned Promise without awaiting it. While this works correctly due to the mutation lock serialization (enqueue operations are guaranteed to complete before dequeueAll runs), the void cast makes it ambiguous whether the intent is to fire-and-forget or if awaiting was overlooked.The mutation lock design ensures this.queue is updated synchronously within the locked function before persistQueue() awaits, so dequeueAll() will always see the queued message. For code clarity, consider:
- Adding a comment explaining why fire-and-forget is safe here, or
- Explicitly awaiting the enqueue and documenting that it must complete before the first response is processed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/llm/executor/turn-executor.integration.test.ts` around lines 419 - 443, The void cast on messageQueue.enqueue in the test makes the fire-and-forget intent ambiguous; update the test near the messageQueue.enqueue call to either (1) replace the void cast with awaiting the Promise (await messageQueue.enqueue(...)) to make the ordering explicit, or (2) keep the void but add a short comment referencing the mutation lock behavior and dequeueAll guarantee (e.g., "safe to fire-and-forget: mutation lock updates queue synchronously before persistQueue/dequeueAll runs") and mention messageQueue.enqueue, the mutation lock, and dequeueAll so future readers understand why not awaiting is safe.
🧹 Nitpick comments (2)
packages/core/src/session/message-queue.ts (2)
68-81: Consider adding an initialization guard to mutating methods.Methods like
enqueue,dequeueAll,clear, andremovedon't verify thatinitialize()was called. If these are invoked before initialization, persisted messages won't be loaded and could be lost.🛡️ Proposed guard pattern
+ private assertInitialized(): void { + if (!this.initialized) { + throw new DextoRuntimeError('MessageQueueService must be initialized before use'); + } + } + async enqueue( message: UserMessageInput ): Promise<{ queued: true; position: number; id: string }> { return await this.runWithMutationLock(async () => { + this.assertInitialized(); const queuedMsg: QueuedMessage = {Apply similarly to
dequeueAll,clear, andremove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/message-queue.ts` around lines 68 - 81, The mutating methods (enqueue, dequeueAll, clear, remove) can run before persisted messages are loaded because they don't ensure initialize() ran; add an initialization guard by calling await this.initialize() at the start of each of those methods (enqueue, dequeueAll, clear, remove) so the in-memory queue is populated from this.store.load(this.sessionId) before any mutation; rely on the existing this.initialized flag inside initialize() to keep it idempotent and avoid duplicate work.
198-201: Replace genericErrorwith typed errors per coding guidelines.Lines 200 and 286 use
throw new Error(). Per the coding guidelines forpackages/core/src/**/*.ts, prefer typed errors likeDextoRuntimeError.♻️ Proposed fix
+import { DextoRuntimeError } from '../errors/index.js'; + // Line 200 - throw new Error('Unexpected empty messages array'); + throw new DextoRuntimeError('Unexpected empty messages array'); // Line 286 - throw new Error('Unexpected undefined message in non-empty array'); + throw new DextoRuntimeError('Unexpected undefined message in non-empty array');As per coding guidelines: "Avoid throw new Error() in packages/core; prefer typed errors (DextoRuntimeError, DextoValidationError)"
Also applies to: 284-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/message-queue.ts` around lines 198 - 201, Replace the generic throw new Error usages around the checks for firstMsg and the other throw at lines ~284-287 with a typed runtime error: instantiate and throw DextoRuntimeError (rather than Error) so the code follows packages/core error conventions; update the two locations where firstMsg is validated (the block referencing firstMsg and the later throw handling the messages array) to throw new DextoRuntimeError with a clear message like "Unexpected empty messages array" to preserve intent and stack clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/llm/executor/turn-executor.integration.test.ts`:
- Around line 419-443: The void cast on messageQueue.enqueue in the test makes
the fire-and-forget intent ambiguous; update the test near the
messageQueue.enqueue call to either (1) replace the void cast with awaiting the
Promise (await messageQueue.enqueue(...)) to make the ordering explicit, or (2)
keep the void but add a short comment referencing the mutation lock behavior and
dequeueAll guarantee (e.g., "safe to fire-and-forget: mutation lock updates
queue synchronously before persistQueue/dequeueAll runs") and mention
messageQueue.enqueue, the mutation lock, and dequeueAll so future readers
understand why not awaiting is safe.
---
Nitpick comments:
In `@packages/core/src/session/message-queue.ts`:
- Around line 68-81: The mutating methods (enqueue, dequeueAll, clear, remove)
can run before persisted messages are loaded because they don't ensure
initialize() ran; add an initialization guard by calling await this.initialize()
at the start of each of those methods (enqueue, dequeueAll, clear, remove) so
the in-memory queue is populated from this.store.load(this.sessionId) before any
mutation; rely on the existing this.initialized flag inside initialize() to keep
it idempotent and avoid duplicate work.
- Around line 198-201: Replace the generic throw new Error usages around the
checks for firstMsg and the other throw at lines ~284-287 with a typed runtime
error: instantiate and throw DextoRuntimeError (rather than Error) so the code
follows packages/core error conventions; update the two locations where firstMsg
is validated (the block referencing firstMsg and the later throw handling the
messages array) to throw new DextoRuntimeError with a clear message like
"Unexpected empty messages array" to preserve intent and stack clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df743986-2730-4af1-88ce-cf9f6e3aa8bf
📒 Files selected for processing (6)
packages/core/src/llm/executor/turn-executor.integration.test.tspackages/core/src/llm/services/factory.tspackages/core/src/llm/services/vercel.tspackages/core/src/session/message-queue-store.tspackages/core/src/session/message-queue.test.tspackages/core/src/session/message-queue.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/llm/services/vercel.ts
- packages/core/src/session/message-queue-store.ts
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/core/src/session/session-manager.ts (2)
543-544:⚠️ Potential issue | 🟠 MajorDon't hydrate persisted tool/approval state on the fresh-session path.
Lines 543-544 run only after
existingMetadatawas absent, so a reused session ID can inherit orphanedsession-tool-preferences:*/session-approvals:*state from an older session.Suggested fix
session = new ChatSession(this.getChatSessionServices(), id, sessionLogger); await session.init(); - await this.services.toolManager.restoreSessionState(id); - await this.services.approvalManager.restoreSessionState(id); this.sessions.set(id, session);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/session-manager.ts` around lines 543 - 544, The code calls this.services.toolManager.restoreSessionState and this.services.approvalManager.restoreSessionState even when existingMetadata is absent, which hydrates stale tool/approval state into a fresh session; change the logic so these restoreSessionState calls only run when existingMetadata is present (i.e., on the resume path), or otherwise skip/clear persisted session-tool-preferences:* and session-approvals:* for a fresh session. Locate the conditional around existingMetadata in session-manager (the method containing those calls) and move or wrap the two restoreSessionState calls behind a check for existingMetadata (or add an explicit "isFreshSession" guard) so fresh-session creation does not inherit orphaned state.
493-494:⚠️ Potential issue | 🟠 MajorEvict restored tool/approval state when TTL cleanup drops a session.
These restore calls memoize the session as already hydrated, but
cleanupExpiredSessions()still only disposes theChatSession. After expiry, a later reload can skip the store read and keep stale restored prefs/approvals alive in memory.Suggested follow-up in
cleanupExpiredSessions()for (const sessionId of expiredSessions) { const session = this.sessions.get(sessionId); if (session) { // Only dispose memory resources, don't delete chat history session.dispose(); this.sessions.delete(sessionId); + this.services.toolManager.evictSessionState(sessionId); + this.services.approvalManager.evictSessionState(sessionId); this.logger.debug( `Removed expired session from memory: ${sessionId} (chat history preserved)` ); } }Also applies to: 632-633
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/session-manager.ts` around lines 493 - 494, When cleaning up expired sessions in cleanupExpiredSessions(), also evict any memoized/hydrated tool and approval state so restored prefs don't survive after the ChatSession is disposed; after disposing the ChatSession call the counterpart eviction methods on services.toolManager and services.approvalManager (e.g. toolManager.evictSessionState(id) and approvalManager.evictSessionState(id) or the equivalent clearHydration/removeMemoizedState APIs) to remove cached/restored state for that session id; apply the same eviction where restoreSessionState(...) is called (the other occurrence around the 632-633 section).
🧹 Nitpick comments (3)
.agents/skills/tdd/deep-modules.md (2)
31-33: Optional readability tweak for repeated “Can I …” phrasing.The checklist is clear, but the repeated sentence openings read a bit mechanical. Consider varying one line for flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/tdd/deep-modules.md around lines 31 - 33, The three checklist lines all start with "Can I ..." and read repetitive; edit one line in the checklist (the three bullet lines shown) to vary phrasing for better flow—e.g., change one line to "Is it possible to reduce the number of methods?" or reword to a single combined sentence like "Reduce methods, simplify parameters, or hide more complexity?"—so the content stays the same but avoids the repeated opening.
1-33: Add one concrete agent-layer example to ground the concept in this repo.A short example (e.g., “session persistence facade as a deep module”) would make this guidance more actionable for contributors.
Based on learnings: Applies to **/*.md : Document agent architecture and design patterns in markdown files (e.g., AGENTS.md).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/tdd/deep-modules.md around lines 1 - 33, Add a short concrete agent-layer example called "SessionPersistenceFacade" to the Deep Modules doc that shows a small public interface (e.g., methods saveSession(session), restoreSession(sessionId)) and a deep implementation that hides serialization, encryption, retry/backoff, and multiple storageBackends; describe how callers only use the two facade methods while complex logic (serialize/deserialize, encrypt/decrypt, storage selection, retry) lives inside the implementation to illustrate the deep-module pattern and make the guidance actionable for contributors..agents/skills/simplify/SKILL.md (1)
52-52: Keep a short rationale when skipping findings on Line 52.“Skip and move on” is fine, but add a one-line reason per skipped item to preserve review traceability.
Proposed edit
-Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it. +Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, record a brief reason and move on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/simplify/SKILL.md at line 52, Update the instruction that begins "Wait for all three agents to complete. Aggregate their findings and fix each issue directly." so that when a finding is skipped you require a one-line rationale; change the phrase "If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it." to explicitly state "if you skip a finding, add a single-line reason for the skip (one brief rationale per skipped item) to preserve review traceability." Ensure the SKILL markdown text containing that sentence is edited to mandate and illustrate the one-line rationale format for skipped findings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/simplify/SKILL.md:
- Line 12: Update the staged-diff advice on the line containing "Run `git diff`
(or `git diff HEAD` if there are staged changes)..." to recommend the precise
staged-only command: replace the suggestion with "Run `git diff --staged` (or
`git diff --staged HEAD` if you need explicit HEAD comparison)" so the guidance
uses the exact staged-diff syntax; edit the SKILL.md sentence that currently
references `git diff`/`git diff HEAD` to use `git diff --staged` instead.
In @.agents/skills/tdd/deep-modules.md:
- Around line 7-27: The two fenced ASCII diagram blocks in deep-modules.md are
missing language identifiers (triggering MD040); update both triple-backtick
fences that surround the "Small Interface / Deep Implementation" and "Large
Interface / Thin Implementation" diagrams to use ```text so the code blocks are
explicitly marked as plain text; ensure both opening fences are changed and the
closing fences remain unchanged.
In @.agents/skills/tdd/SKILL.md:
- Around line 31-41: Add a language tag (e.g., text) to each fenced code block
that contains the TDD examples (the blocks showing "WRONG (horizontal):" and
"RIGHT (vertical):" and the other similar example blocks later in the document)
so the fences become ```text instead of ```; update all such fences in the file
(including the other example blocks referenced in the comment) to satisfy MD040
and keep the doc lint-clean.
In `@packages/core/src/approval/manager.ts`:
- Around line 436-440: The code calls addApprovedDirectory(directory, type,
sessionId) without awaiting it, causing a race with subsequent checks now that
addApprovedDirectory uses an async lock; update the caller to await
addApprovedDirectory(...) so the approval write completes before any immediate
follow-up checks, making the caller function async if necessary, and wrap the
await in a try/catch to handle/log errors rather than fire-and-forget.
- Around line 253-262: deleteSessionState currently evicts local state and
deletes storage without acquiring the per-scope lock, allowing concurrent
mutating helpers like addPattern or addApprovedDirectory to re-save the scope
after deletion; wrap the eviction and storage deletion inside the per-scope lock
by calling runWithScopeLock(getScopeKey(sessionId) or
runWithScopeLock(sessionId) (use the same signature as
addPattern/addApprovedDirectory) and perform evictSessionState(sessionId) and
await sessionApprovalStore.delete(sessionId) from within that locked callback so
the delete is serialized with other scope mutations.
In `@packages/core/src/llm/executor/turn-executor.integration.test.ts`:
- Around line 427-432: The mocked stream implementation for streamText currently
fire-and-forgets messageQueue.enqueue, causing a race; update the
vi.mocked(streamText).mockImplementation callback (the branch where callCount
=== 1) to await the asynchronous messageQueue.enqueue call before
returning/finishing the mock so the queued follow-up is persisted
deterministically for the executor’s subsequent pass; ensure the mock
implementation returns a Promise (or is async) so the test waits for enqueue to
complete.
In `@packages/core/src/session/message-queue.ts`:
- Around line 99-125: initialize() currently assigns this.queue from store.load
outside the mutation lock, allowing a late load to overwrite concurrent
mutations; wrap the restore logic in runWithMutationLock so load, assignment of
this.queue and this.initialized are executed under the same lock as
enqueue()/dequeueAll()/clear()/remove(). Concretely, replace the body that calls
this.store.load(...) and sets this.queue/this.initialized with a call to
this.runWithMutationLock(async () => { this.queue = await
this.store.load(this.sessionId); if (this.queue.length > 0)
this.logger.debug(...); this.initialized = true; }); and return that promise so
initialization is serialized with persistQueue and other mutating methods.
In `@packages/core/src/tools/tool-manager.ts`:
- Around line 203-213: applySessionToolPreferences currently always sets
sessionUserAutoApproveTools.set(sessionId,
[...preferences.userAutoApproveTools]) even when the array is empty, causing
hasSessionUserAutoApproveTools(sessionId) to return true for default/empty
states; change the logic so you only set sessionUserAutoApproveTools for a
session when preferences.userAutoApproveTools.length > 0 and otherwise delete
the key (mirroring the existing sessionDisabledTools behavior) so empty lists
are not treated as explicit user-configured entries.
- Around line 244-254: deleteSessionState currently evicts in-memory maps then
deletes storage without acquiring the writer lock, allowing concurrent setters
(e.g. setSessionDisabledTools or setSessionUserAutoApproveTools) to race and
rewrite preferences; fix this by acquiring the sessionPreferenceLocks writer for
the sessionId inside deleteSessionState, perform evictSessionState(sessionId)
and await sessionToolPreferencesStore.delete(sessionId) while holding the lock,
then release the lock so the deletion is serialized with the existing setters
that also use sessionPreferenceLocks.
---
Duplicate comments:
In `@packages/core/src/session/session-manager.ts`:
- Around line 543-544: The code calls
this.services.toolManager.restoreSessionState and
this.services.approvalManager.restoreSessionState even when existingMetadata is
absent, which hydrates stale tool/approval state into a fresh session; change
the logic so these restoreSessionState calls only run when existingMetadata is
present (i.e., on the resume path), or otherwise skip/clear persisted
session-tool-preferences:* and session-approvals:* for a fresh session. Locate
the conditional around existingMetadata in session-manager (the method
containing those calls) and move or wrap the two restoreSessionState calls
behind a check for existingMetadata (or add an explicit "isFreshSession" guard)
so fresh-session creation does not inherit orphaned state.
- Around line 493-494: When cleaning up expired sessions in
cleanupExpiredSessions(), also evict any memoized/hydrated tool and approval
state so restored prefs don't survive after the ChatSession is disposed; after
disposing the ChatSession call the counterpart eviction methods on
services.toolManager and services.approvalManager (e.g.
toolManager.evictSessionState(id) and approvalManager.evictSessionState(id) or
the equivalent clearHydration/removeMemoizedState APIs) to remove
cached/restored state for that session id; apply the same eviction where
restoreSessionState(...) is called (the other occurrence around the 632-633
section).
---
Nitpick comments:
In @.agents/skills/simplify/SKILL.md:
- Line 52: Update the instruction that begins "Wait for all three agents to
complete. Aggregate their findings and fix each issue directly." so that when a
finding is skipped you require a one-line rationale; change the phrase "If a
finding is a false positive or not worth addressing, note it and move on — do
not argue with the finding, just skip it." to explicitly state "if you skip a
finding, add a single-line reason for the skip (one brief rationale per skipped
item) to preserve review traceability." Ensure the SKILL markdown text
containing that sentence is edited to mandate and illustrate the one-line
rationale format for skipped findings.
In @.agents/skills/tdd/deep-modules.md:
- Around line 31-33: The three checklist lines all start with "Can I ..." and
read repetitive; edit one line in the checklist (the three bullet lines shown)
to vary phrasing for better flow—e.g., change one line to "Is it possible to
reduce the number of methods?" or reword to a single combined sentence like
"Reduce methods, simplify parameters, or hide more complexity?"—so the content
stays the same but avoids the repeated opening.
- Around line 1-33: Add a short concrete agent-layer example called
"SessionPersistenceFacade" to the Deep Modules doc that shows a small public
interface (e.g., methods saveSession(session), restoreSession(sessionId)) and a
deep implementation that hides serialization, encryption, retry/backoff, and
multiple storageBackends; describe how callers only use the two facade methods
while complex logic (serialize/deserialize, encrypt/decrypt, storage selection,
retry) lives inside the implementation to illustrate the deep-module pattern and
make the guidance actionable for contributors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d533fba0-631d-4ba3-85c7-c1acf0e39e75
📒 Files selected for processing (27)
.agents/skills/grill-me/SKILL.md.agents/skills/simplify/SKILL.md.agents/skills/simplify/agents/openai.yaml.agents/skills/tdd/SKILL.md.agents/skills/tdd/deep-modules.md.agents/skills/tdd/interface-design.md.agents/skills/tdd/mocking.md.agents/skills/tdd/refactoring.md.agents/skills/tdd/tests.mdpackages/core/src/approval/manager.test.tspackages/core/src/approval/manager.tspackages/core/src/llm/executor/turn-executor.integration.test.tspackages/core/src/llm/services/factory.tspackages/core/src/llm/services/types.tspackages/core/src/session/chat-session.tspackages/core/src/session/message-queue-store.tspackages/core/src/session/message-queue.test.tspackages/core/src/session/message-queue.tspackages/core/src/session/session-manager.test.tspackages/core/src/session/session-manager.tspackages/core/src/session/title-generator.test.tspackages/core/src/session/title-generator.tspackages/core/src/test-utils/session-state-stores.tspackages/core/src/tools/tool-manager.integration.test.tspackages/core/src/tools/tool-manager.test.tspackages/core/src/tools/tool-manager.tspackages/tools-filesystem/src/directory-approval.integration.test.ts
✅ Files skipped from review due to trivial changes (6)
- .agents/skills/tdd/refactoring.md
- .agents/skills/simplify/agents/openai.yaml
- .agents/skills/grill-me/SKILL.md
- .agents/skills/tdd/interface-design.md
- .agents/skills/tdd/mocking.md
- .agents/skills/tdd/tests.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/llm/services/types.ts
- packages/core/src/session/session-manager.test.ts
- packages/core/src/approval/manager.test.ts
- packages/core/src/session/chat-session.ts
- packages/core/src/session/message-queue-store.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/session/session-manager.ts (1)
543-544:⚠️ Potential issue | 🟠 MajorSkip persisted tool/approval restore on the brand-new session path.
This branch only runs after the
existingMetadatalookup missed, so these restores can only load orphanedsession-tool-preferences:*/session-approvals:*data from a reused ID. That leaks old disabled tools / auto-approvals into a new session.Suggested fix
session = new ChatSession(this.getChatSessionServices(), id, sessionLogger); await session.init(); - await this.services.toolManager.restoreSessionState(id); - await this.services.approvalManager.restoreSessionState(id); this.sessions.set(id, session);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/session-manager.ts` around lines 543 - 544, The restore calls for tool/approval state (this.services.toolManager.restoreSessionState(id) and this.services.approvalManager.restoreSessionState(id)) are being invoked on the brand-new session path where existingMetadata was not found, causing old session preferences to be reloaded; guard these restore calls so they only run when existingMetadata is present (i.e., only restore when the session ID corresponds to existingMetadata/reuse) — add a simple conditional around the two restore calls checking existingMetadata (or the same "isNew" flag used earlier) to skip restores for new sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/agent/DextoAgent.ts`:
- Around line 2394-2397: The code branches on sessionId truthiness which treats
empty string as undefined and allows silent global reconfig—change the
conditional in the currentLLMConfig assignment to explicitly check sessionId !==
undefined (i.e., use sessionId !== undefined ?
this.stateManager.getRuntimeConfig(sessionId).llm :
this.stateManager.getRuntimeConfig().llm) to reject empty strings, and make the
identical explicit !== undefined check in performLLMSwitch() so both switchLLM
and performLLMSwitch use the same null/undefined-safe logic referencing
sessionId.
In `@packages/core/src/session/session-manager.ts`:
- Around line 1190-1192: The session currently applies new LLM config in memory
via this.services.stateManager.updateLLM(...) and await session.switchLLM(...)
before calling await this.persistSessionLLMOverride(...), which risks an
inconsistent runtime if persistence fails; change the flow to persist the
newLLMConfig first (call persistSessionLLMOverride(sId, newLLMConfig) before
updating state or switching the live session), or alternatively capture the
previous LLM config (read from this.services.stateManager or
session.getLLMConfig()), attempt persistence, and if persistSessionLLMOverride
throws revert both this.services.stateManager.updateLLM(previousConfig, sId) and
await session.switchLLM(previousConfig) to roll back the live session; make the
change in the code paths that call updateLLM/switchLLM and
persistSessionLLMOverride (including the similar block at the other location
noted around lines 1237-1238).
---
Duplicate comments:
In `@packages/core/src/session/session-manager.ts`:
- Around line 543-544: The restore calls for tool/approval state
(this.services.toolManager.restoreSessionState(id) and
this.services.approvalManager.restoreSessionState(id)) are being invoked on the
brand-new session path where existingMetadata was not found, causing old session
preferences to be reloaded; guard these restore calls so they only run when
existingMetadata is present (i.e., only restore when the session ID corresponds
to existingMetadata/reuse) — add a simple conditional around the two restore
calls checking existingMetadata (or the same "isNew" flag used earlier) to skip
restores for new sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 361ff143-7003-4fef-8dca-b947e83803da
📒 Files selected for processing (3)
packages/core/src/agent/DextoAgent.tspackages/core/src/session/session-manager.test.tspackages/core/src/session/session-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/approval/manager.ts (1)
253-261:⚠️ Potential issue | 🟠 MajorSerialize session eviction/deletion with the scope lock.
deleteSessionState()still clears memory and storage outsiderunWithScopeLock(). A concurrentaddPattern()oraddApprovedDirectory()that already entered the mutation path can finish afterwards and persist the scope again, recreating approvals that were just deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/approval/manager.ts` around lines 253 - 261, The eviction and persistent deletion must be performed under the same scope lock to avoid races: modify deleteSessionState to call runWithScopeLock(this.getScopeKey(sessionId), async () => { evictSessionState(sessionId); await this.sessionApprovalStore.delete(sessionId); }) so that memory clears (scopes, loadedScopes via evictSessionState) and storage deletion (sessionApprovalStore.delete) are serialized with other mutations such as addPattern() and addApprovedDirectory(); ensure evictSessionState remains idempotent and remove any calls that delete state outside runWithScopeLock.
🧹 Nitpick comments (1)
packages/core/src/approval/manager.ts (1)
373-379: Encode the non-empty key invariant in the type instead of using!.
keys[0]!is safe today, but it leaves a production non-null assertion in core code. Returning a tuple fromgetPathApprovalKeys()lets this method destructure the result without the assertion and documents the invariant at the API boundary.♻️ Suggested change
- private getPathApprovalKeys(targetPath: string): string[] { + private getPathApprovalKeys(targetPath: string): [string] | [string, string] { const resolved = path.resolve(targetPath); const real = tryRealpathSyncWithExistingParent(resolved); if (real && real !== resolved) { return [resolved, real]; } return [resolved]; } ... - const resolvedKey = keys[0]!; + const [resolvedKey, realKey] = keys; if (effectiveType === 'session' && type === 'once' && hasSessionApproval) { this.logger.debug( `Directory "${resolvedKey}" already approved as 'session', not downgrading to 'once'` ); return; } - const realKey = keys.length > 1 ? keys[1] : null; this.logger.debug( `Added approved directory in '${this.getScopeLabel(sessionId)}': "${resolvedKey}" (type: ${effectiveType})${ realKey ? `, realpath: "${realKey}"` : '' }` );As per coding guidelines,
packages/**/src/**/*.ts: Avoid non-null assertions (!) in production code; acceptable only in tests when it improves clarity and value is provably present.Also applies to: 467-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/approval/manager.ts` around lines 373 - 379, Change getPathApprovalKeys to return a typed tuple so callers don't need non-null assertions: update its signature to return [string, string?] (or [string, string | undefined]) and keep the same return values ([resolved] or [resolved, real]); then update call sites that currently do keys[0]! to destructure like const [resolved, real] = getPathApprovalKeys(...) or use keys[0] without !. Also apply the same tuple-return refactor to the other similar helper referenced around the approval logic (the second occurrence noted at lines 467-475) so no production code uses non-null assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tools-filesystem/src/tool-factory.ts`:
- Around line 49-65: configureFileSystemService mutates a shared
fileSystemService (calling setDirectoryApprovalChecker and applyWorkspace)
causing session-specific state to leak between concurrent ToolExecutionContext
executions; instead, avoid mutating the singleton: change
configureFileSystemService (and the similar block at 82-99) to return a
per-execution FileSystemService view by either cloning/instantiating a new
FileSystemService for the context or by returning a small context-bound
wrapper/decorator that delegates to the shared service but implements directory
approval checks and working-directory resolution using the provided
ToolExecutionContext (use context.sessionId in the wrapper and call
approvalManager.isDirectoryApproved there, and use applyWorkspace logic only on
the wrapper), and remove calls that call
setDirectoryApprovalChecker/setWorkingDirectory on the shared fileSystemService.
---
Duplicate comments:
In `@packages/core/src/approval/manager.ts`:
- Around line 253-261: The eviction and persistent deletion must be performed
under the same scope lock to avoid races: modify deleteSessionState to call
runWithScopeLock(this.getScopeKey(sessionId), async () => {
evictSessionState(sessionId); await this.sessionApprovalStore.delete(sessionId);
}) so that memory clears (scopes, loadedScopes via evictSessionState) and
storage deletion (sessionApprovalStore.delete) are serialized with other
mutations such as addPattern() and addApprovedDirectory(); ensure
evictSessionState remains idempotent and remove any calls that delete state
outside runWithScopeLock.
---
Nitpick comments:
In `@packages/core/src/approval/manager.ts`:
- Around line 373-379: Change getPathApprovalKeys to return a typed tuple so
callers don't need non-null assertions: update its signature to return [string,
string?] (or [string, string | undefined]) and keep the same return values
([resolved] or [resolved, real]); then update call sites that currently do
keys[0]! to destructure like const [resolved, real] = getPathApprovalKeys(...)
or use keys[0] without !. Also apply the same tuple-return refactor to the other
similar helper referenced around the approval logic (the second occurrence noted
at lines 467-475) so no production code uses non-null assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc7ad476-66ea-4655-b093-c3d2abd09477
📒 Files selected for processing (8)
packages/core/src/approval/manager.test.tspackages/core/src/approval/manager.tspackages/core/src/session/session-manager.integration.test.tspackages/core/src/session/session-manager.test.tspackages/core/src/session/session-manager.tspackages/tools-filesystem/src/directory-approval.integration.test.tspackages/tools-filesystem/src/directory-approval.tspackages/tools-filesystem/src/tool-factory.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/session/session-manager.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/session/session-manager.ts
- packages/core/src/session/session-manager.integration.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
1 participant
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.
Summary
SessionManager,ToolManager,ApprovalManager, andChatSessionwithout introducing new top-level storage configWhy
Core-owned interaction state was still being kept only in memory in several places. That breaks hosted/runtime restart scenarios even though core already has database/cache/blob storage available.
Validation
./scripts/quality-checks.shpnpm exec vitest packages/core/src/session/message-queue.test.ts packages/core/src/session/chat-session.test.ts packages/core/src/session/session-manager.test.ts packages/core/src/session/session-manager.integration.test.ts packages/core/src/approval/manager.test.ts packages/core/src/tools/tool-manager.test.ts --run --reporter=dotNotes
Summary by CodeRabbit
New Features
Improvements
Documentation