Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSession-level project directory management is introduced through a new Changes
Sequence DiagramsequenceDiagram
actor User
participant SessionPresenter as AgentSessionPresenter
participant RuntimePresenter as AgentRuntimePresenter
participant Agent as Agent Implementation
participant DB as SQLite
User->>SessionPresenter: setSessionProjectDir(sessionId, path)
SessionPresenter->>RuntimePresenter: setSessionProjectDir(sessionId, path)
RuntimePresenter->>RuntimePresenter: Normalize & validate projectDir
RuntimePresenter->>RuntimePresenter: Invalidate system prompt cache
RuntimePresenter->>DB: Store projectDir in session
RuntimePresenter->>Agent: setSessionProjectDir(sessionId, path)
Agent-->>RuntimePresenter: OK
User->>SessionPresenter: queuePendingInput(input, options)
SessionPresenter->>SessionPresenter: Resolve projectDir from session
SessionPresenter->>RuntimePresenter: processMessage(..., projectDir)
RuntimePresenter->>RuntimePresenter: Resolve projectDir (with DB fallback)
RuntimePresenter->>Agent: processMessage(..., projectDir)
Agent-->>RuntimePresenter: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/main/presenter/agentSessionPresenter/agentSessionPresenter.test.ts (1)
924-951: Good end-to-end sync test; consider adding clear-project-dir coverage.This test validates persistence + runtime forwarding + environment sync for a non-null path. Adding a companion case for
projectDir = nullwould help guard against stale workspace state when users clear the directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentSessionPresenter/agentSessionPresenter.test.ts` around lines 924 - 951, Add a complementary test in the same describe('setSessionProjectDir') block that covers clearing the project dir (project_dir = null): simulate an existing session row with a non-null project_dir, mock sqlitePresenter.newSessionsTable.get and update to reflect clearing the field, call presenter.setSessionProjectDir('s1', null), and assert that sqlitePresenter.newSessionsTable.update was called with project_dir: null, deepChatAgent.setSessionProjectDir was called with ('s1', null), and sqlitePresenter.newEnvironmentsTable.syncPath was called with null (or cleared indicator) to ensure workspace state is cleared in runtime and environment sync.test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts (2)
1322-1365: Nice coverage for project-dir update/rehydration; add the null-reset case too.These tests validate the happy path well. Since the API allows
projectDir: string | null, please also coversetSessionProjectDir('s1', null)to ensure prompt cache invalidation and workspace/tool context reset do not leave stale directory state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts` around lines 1322 - 1365, Add a new test that after initializing a session and setting a project dir then resetting it to null via setSessionProjectDir('s1', null) verifies the prompt cache is invalidated and workspace/tool context is cleared: call agent.initSession('s1', ...), agent.setSessionProjectDir('s1','/tmp/workspace'), agent.processMessage('s1', ...), then agent.setSessionProjectDir('s1', null) and agent.processMessage('s1', ...); assert buildSystemEnvPrompt (or buildSystemEnvPrompt mock) was invoked again, inspect the later processStream call (processStream.mock.calls) to ensure the latest messages do NOT contain 'WORKDIR:' (or do not contain the previous path), and assert toolPresenter.getAllToolDefinitions was called with an objectContaining where agentWorkspacePath is null or undefined to confirm tools were reset. Ensure you reference setSessionProjectDir, processMessage, buildSystemEnvPrompt, processStream, and toolPresenter.getAllToolDefinitions in the test.
60-75: Align thebuildSystemEnvPrompttest double with production output semantics.At Line 72 the mock emits
WORKDIR:and uses empty-string fallback, while the real builder emitsWorking directory: ...and resolves a default workdir (src/main/lib/agentRuntime/systemEnvPromptBuilder.tsLine 137-152). This mismatch can hide prompt-shape regressions.♻️ Suggested test-double alignment
- return [ - 'ENV_BLOCK', - `MODEL:${providerId}/${modelId}`, - `WORKDIR:${options?.workdir ?? ''}`, - `DATE:${dateText}` - ].join('\n') + const resolvedWorkdir = options?.workdir ?? process.cwd() + return [ + 'ENV_BLOCK', + `MODEL:${providerId}/${modelId}`, + `Working directory: ${resolvedWorkdir}`, + `DATE:${dateText}` + ].join('\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts` around lines 60 - 75, The test double for buildSystemEnvPrompt currently emits "WORKDIR:" with an empty-string fallback; update it to match production semantics by emitting "Working directory: <value>" (exact phrase/casing) and when options.workdir is omitted or null resolve the same default workdir used by the real builder (the logic in systemEnvPromptBuilder that supplies the default), so the mock mirrors buildSystemEnvPrompt's output format and defaulting behavior exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/agentSessionPresenter/index.ts`:
- Around line 1718-1728: The code currently updates persistent session state via
sessionManager.update(sessionId, { projectDir: normalizedProjectDir }) before
calling syncAcpSessionWorkdir(...), which can leave the DB in a
partially-applied state if the ACP/runtime sync fails; fix this by performing
the external syncs first (call
sqlitePresenter.newEnvironmentsTable.syncPath(normalizedProjectDir), await
agent.setSessionProjectDir(sessionId, normalizedProjectDir) if present, and
await syncAcpSessionWorkdir(providerId, sessionId, session.agentId,
normalizedProjectDir)) and only call sessionManager.update(...) after those
awaitable operations succeed; if you cannot reorder, implement a try/catch
around the sync calls and on error rollback the previous projectDir via
sessionManager.update(sessionId, { projectDir: previousProjectDir }) and rethrow
the error.
- Around line 1718-1723: When updating the session's projectDir you must also
resync the old path so it doesn't remain stale: before calling
this.sessionManager.update(sessionId, { projectDir: normalizedProjectDir }) read
the existing session (e.g., const prev = this.sessionManager.get(sessionId) or
equivalent) and if prev?.projectDir and prev.projectDir !== normalizedProjectDir
call this.sqlitePresenter.newEnvironmentsTable.syncPath(prev.projectDir); then
proceed to update and still call syncPath(normalizedProjectDir) for the new
value (or call syncPath(null) when clearing) so both old and new aggregate rows
are refreshed.
---
Nitpick comments:
In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts`:
- Around line 1322-1365: Add a new test that after initializing a session and
setting a project dir then resetting it to null via setSessionProjectDir('s1',
null) verifies the prompt cache is invalidated and workspace/tool context is
cleared: call agent.initSession('s1', ...),
agent.setSessionProjectDir('s1','/tmp/workspace'), agent.processMessage('s1',
...), then agent.setSessionProjectDir('s1', null) and agent.processMessage('s1',
...); assert buildSystemEnvPrompt (or buildSystemEnvPrompt mock) was invoked
again, inspect the later processStream call (processStream.mock.calls) to ensure
the latest messages do NOT contain 'WORKDIR:' (or do not contain the previous
path), and assert toolPresenter.getAllToolDefinitions was called with an
objectContaining where agentWorkspacePath is null or undefined to confirm tools
were reset. Ensure you reference setSessionProjectDir, processMessage,
buildSystemEnvPrompt, processStream, and toolPresenter.getAllToolDefinitions in
the test.
- Around line 60-75: The test double for buildSystemEnvPrompt currently emits
"WORKDIR:" with an empty-string fallback; update it to match production
semantics by emitting "Working directory: <value>" (exact phrase/casing) and
when options.workdir is omitted or null resolve the same default workdir used by
the real builder (the logic in systemEnvPromptBuilder that supplies the
default), so the mock mirrors buildSystemEnvPrompt's output format and
defaulting behavior exactly.
In `@test/main/presenter/agentSessionPresenter/agentSessionPresenter.test.ts`:
- Around line 924-951: Add a complementary test in the same
describe('setSessionProjectDir') block that covers clearing the project dir
(project_dir = null): simulate an existing session row with a non-null
project_dir, mock sqlitePresenter.newSessionsTable.get and update to reflect
clearing the field, call presenter.setSessionProjectDir('s1', null), and assert
that sqlitePresenter.newSessionsTable.update was called with project_dir: null,
deepChatAgent.setSessionProjectDir was called with ('s1', null), and
sqlitePresenter.newEnvironmentsTable.syncPath was called with null (or cleared
indicator) to ensure workspace state is cleared in runtime and environment sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8635461-72ba-4a34-99ac-8a1b3a67d353
📒 Files selected for processing (5)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentSessionPresenter/index.tssrc/shared/types/agent-interface.d.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentSessionPresenter/agentSessionPresenter.test.ts
Comment on lines
+1718
to
+1728
| this.sessionManager.update(sessionId, { projectDir: normalizedProjectDir }) | ||
|
|
||
| // Sync environment for new project dir | ||
| if (projectDir) { | ||
| this.sqlitePresenter.newEnvironmentsTable.syncPath(projectDir) | ||
| if (normalizedProjectDir) { | ||
| this.sqlitePresenter.newEnvironmentsTable.syncPath(normalizedProjectDir) | ||
| } | ||
|
|
||
| if (agent.setSessionProjectDir) { | ||
| await agent.setSessionProjectDir(sessionId, normalizedProjectDir) | ||
| } | ||
| await this.syncAcpSessionWorkdir(providerId, sessionId, session.agentId, normalizedProjectDir) |
Contributor
There was a problem hiding this comment.
Avoid committing the new workdir before downstream sync can fail.
If syncAcpSessionWorkdir() rejects, this method still leaves new_sessions.project_dir updated
from Line 1718, so callers see a failure even though subsequent reads return the new directory.
Either perform the ACP/runtime sync before persisting, or rollback the previous projectDir on
error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 1718 - 1728,
The code currently updates persistent session state via
sessionManager.update(sessionId, { projectDir: normalizedProjectDir }) before
calling syncAcpSessionWorkdir(...), which can leave the DB in a
partially-applied state if the ACP/runtime sync fails; fix this by performing
the external syncs first (call
sqlitePresenter.newEnvironmentsTable.syncPath(normalizedProjectDir), await
agent.setSessionProjectDir(sessionId, normalizedProjectDir) if present, and
await syncAcpSessionWorkdir(providerId, sessionId, session.agentId,
normalizedProjectDir)) and only call sessionManager.update(...) after those
awaitable operations succeed; if you cannot reorder, implement a try/catch
around the sync calls and on error rollback the previous projectDir via
sessionManager.update(sessionId, { projectDir: previousProjectDir }) and rethrow
the error.
Comment on lines
+1718
to
+1723
| this.sessionManager.update(sessionId, { projectDir: normalizedProjectDir }) | ||
|
|
||
| // Sync environment for new project dir | ||
| if (projectDir) { | ||
| this.sqlitePresenter.newEnvironmentsTable.syncPath(projectDir) | ||
| if (normalizedProjectDir) { | ||
| this.sqlitePresenter.newEnvironmentsTable.syncPath(normalizedProjectDir) | ||
| } |
Contributor
There was a problem hiding this comment.
Resync the previous environment path too.
syncPath() only recomputes the path you pass in. After Line 1718 moves a session from /old to
/new or clears the directory, the aggregate row for /old is left stale because only the new path
is refreshed here.
♻️ Suggested fix
+ const previousProjectDir = session.projectDir ?? null
this.sessionManager.update(sessionId, { projectDir: normalizedProjectDir })
- if (normalizedProjectDir) {
+ if (previousProjectDir !== normalizedProjectDir) {
+ this.sqlitePresenter.newEnvironmentsTable.syncPath(previousProjectDir)
+ }
+ if (normalizedProjectDir) {
this.sqlitePresenter.newEnvironmentsTable.syncPath(normalizedProjectDir)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 1718 - 1723,
When updating the session's projectDir you must also resync the old path so it
doesn't remain stale: before calling this.sessionManager.update(sessionId, {
projectDir: normalizedProjectDir }) read the existing session (e.g., const prev
= this.sessionManager.get(sessionId) or equivalent) and if prev?.projectDir and
prev.projectDir !== normalizedProjectDir call
this.sqlitePresenter.newEnvironmentsTable.syncPath(prev.projectDir); then
proceed to update and still call syncPath(normalizedProjectDir) for the new
value (or call syncPath(null) when clearing) so both old and new aggregate rows
are refreshed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit