fix: move session dirs to ~/.browseros/sessions and update skill paths#494
Conversation
Session directories now live under ~/.browseros/sessions/{conversationId}/
instead of executionDir/sessions/. Adds 30-day cleanup for stale sessions
at server startup. Updates 6 default skills to reference the working
directory instead of hardcoding ~/Downloads/.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Greptile (@greptileai) review |
Greptile SummaryThis PR moves session directories from Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Server Startup] --> B[ensureBrowserosDir\ncreates ~/.browseros/sessions/]
B --> C[cleanOldSessions\nremove dirs with mtime < 30 days]
D[Chat Request] --> E{userWorkingDir set?}
E -- Yes --> F[Use user workspace path]
E -- No --> G[~/.browseros/sessions/conversationId/]
F --> H[mkdir recursive]
G --> H
H --> I{userWorkingDir?}
I -- No --> J[utimes touch dir\nupdates mtime to now]
I -- Yes --> K[Skip touch]
J --> L[workingDir passed to Agent]
K --> L
M[MCP Server] --> N[workingDir = executionDir\napp-managed directory]
L --> O[Tools: save_pdf\nsave_screenshot\ndownload_file\nresolve paths against workingDir]
N --> O
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/server/src/tools/page-actions.ts
Line: 21
Comment:
**Stale LLM-facing `cwd` description still says "execution directory"**
The `cwd` parameter descriptions for `save_pdf`, `save_screenshot`, and `download_file` still say "defaults to the execution directory" after the rename to `workingDir`. The LLM reads these descriptions to reason about tool behaviour, so using the old term is misleading. All three occurrences should be updated:
- `save_pdf` cwd description (line 21)
- `save_screenshot` cwd description (line 54)
- `download_file` cwd description (line 111)
```suggestion
'Working directory to resolve relative paths against; defaults to the working directory',
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/api/services/chat-service.ts
Line: 25
Comment:
**`executionDir` in `ChatServiceDeps` is now dead code**
After this PR, `resolveSessionDir` calls `getSessionsDir()` directly and never reads `this.deps.executionDir`. The field is still declared in the interface and populated in `chat.ts` (line 34), but it is never consumed. Consider removing it to avoid confusion about what directory the service actually uses.
```suggestion
export interface ChatServiceDeps {
sessionStore: SessionStore
klavisClient: KlavisClient
browser: Browser
registry: ToolRegistry
browserosId?: string
}
```
The corresponding `executionDir` assignment in `apps/server/src/api/routes/chat.ts:34` and the `executionDir?: string` field in `ChatRouteDeps` (line 20) would also need to be removed.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/skills/defaults/read-later/SKILL.md
Line: 32
Comment:
**Notification format missing emoji — inconsistent with the folder name**
The description (line 3), the workflow table (lines 23–24), and the Tips section (line 48) all use `"📚 Read Later"` as the folder name. The Notification Format template at line 32 still shows `"Saved to Read Later"` (without the emoji), which will cause the agent-reported confirmation message to differ from the actual bookmark folder name.
```suggestion
Saved to 📚 Read Later
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: aa87c4e |
| | Check for folder | `get_bookmarks` | Look for an existing folder named "Read Later" in the bookmark bar | | ||
| | Create folder (if needed) | `create_bookmark` | If the folder doesn't exist, create "Read Later" in the bookmark bar | | ||
| | Add bookmark | `create_bookmark` | Save the current page URL and title into the "Read Later" folder | | ||
| | Save PDF | `save_pdf` | Download the full page as a PDF to the working directory | |
There was a problem hiding this comment.
Emoji removal may create duplicate bookmark folders for existing users
The bookmark folder name is being changed from "📚 Read Later" to "Read Later". Existing users who already have a "📚 Read Later" folder from the old skill would see the agent fail to find it (the check at line 23 looks for "Read Later"), and subsequently create a new, duplicate folder named "Read Later" alongside the existing one.
Since get_bookmarks + create_bookmark is a purely name-based lookup, there is no automatic migration. Users with the old emoji folder will end up with two separate Read Later folders until they manually reconcile them. Consider whether this rename is intentional and, if so, whether the workflow should also check for the legacy "📚 Read Later" folder name and rename or merge it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/skills/defaults/read-later/SKILL.md
Line: 23-26
Comment:
**Emoji removal may create duplicate bookmark folders for existing users**
The bookmark folder name is being changed from `"📚 Read Later"` to `"Read Later"`. Existing users who already have a `"📚 Read Later"` folder from the old skill would see the agent fail to find it (the check at line 23 looks for `"Read Later"`), and subsequently create a **new, duplicate** folder named `"Read Later"` alongside the existing one.
Since `get_bookmarks` + `create_bookmark` is a purely name-based lookup, there is no automatic migration. Users with the old emoji folder will end up with two separate Read Later folders until they manually reconcile them. Consider whether this rename is intentional and, if so, whether the workflow should also check for the legacy `"📚 Read Later"` folder name and rename or merge it.
How can I resolve this? If you propose a fix, please make it concise.| const entryPath = join(sessionsDir, entry) | ||
| try { | ||
| const info = await stat(entryPath) | ||
| if (info.isDirectory() && info.mtimeMs < cutoff) { |
There was a problem hiding this comment.
mtime-based staleness may not reflect recent file-write activity
The cleanup uses info.mtimeMs (directory modification time) to decide whether to remove a session directory. On Linux and macOS, a directory's mtime is updated only when entries are directly added to or removed from that directory — it is not updated when files inside subdirectories are modified.
Consider this sequence:
- Day 0 — session dir
~/.browseros/sessions/<id>/is created; agent writesresearch-topic/sources/source.md.
The session dir'smtimeis updated whenresearch-topic/is created inside it. - Day 1-30 — the agent only modifies content inside
research-topic/(no new entries in the session dir itself).
The session dir'smtimestays at Day 0. - Day 31 — server restarts; the session dir is removed because
mtime < cutoff.
Additionally, mkdir(dir, { recursive: true }) in resolveSessionDir is a no-op when the directory already exists, so repeated messages in the same conversation do not refresh the mtime.
If the intent is truly "30 days since last write activity", consider using ctimeMs (last metadata change, including deep writes on some systems) or updating a sentinel file inside the session dir on each message to keep mtime fresh. If the intent is "30 days since the session dir was last structurally changed", the current behaviour is fine — just worth documenting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/lib/browseros-dir.ts
Line: 53
Comment:
**`mtime`-based staleness may not reflect recent file-write activity**
The cleanup uses `info.mtimeMs` (directory modification time) to decide whether to remove a session directory. On Linux and macOS, a directory's `mtime` is updated only when entries are directly **added to or removed from** that directory — it is **not** updated when files inside subdirectories are modified.
Consider this sequence:
1. Day 0 — session dir `~/.browseros/sessions/<id>/` is created; agent writes `research-topic/sources/source.md`.
The session dir's `mtime` is updated when `research-topic/` is created inside it.
2. Day 1-30 — the agent only modifies content inside `research-topic/` (no new entries in the session dir itself).
The session dir's `mtime` stays at Day 0.
3. Day 31 — server restarts; the session dir is removed because `mtime < cutoff`.
Additionally, `mkdir(dir, { recursive: true })` in `resolveSessionDir` is a no-op when the directory already exists, so repeated messages in the same conversation do not refresh the `mtime`.
If the intent is truly "30 days since last write activity", consider using `ctimeMs` (last metadata change, including deep writes on some systems) or updating a sentinel file inside the session dir on each message to keep `mtime` fresh. If the intent is "30 days since the session dir was last structurally changed", the current behaviour is fine — just worth documenting.
How can I resolve this? If you propose a fix, please make it concise.Consistent naming for the per-conversation working directory: - ResolvedAgentConfig.sessionExecutionDir → workingDir - ToolDirectories.executionDir → workingDir - resolveExecutionPath() → resolveWorkingPath() - buildBrowserToolSet param: executionDir → workingDir Server-level executionDir (DB, logs) unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…time - Revert "Read Later" back to "📚 Read Later" to avoid creating duplicate bookmark folders for existing users - Touch session dir mtime on each message via utimes() so cleanup correctly reflects last activity, not just directory creation time Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Greptile (@greptileai) review |
- Remove executionDir from ChatServiceDeps and ChatRouteDeps since resolveSessionDir now uses getSessionsDir() directly - Fix missed emoji in notification format template Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
executionDir/sessions/to~/.browseros/sessions/— co-locates session data with other user data (soul, memory, skills)~/Downloads/Design
Session directories conceptually belong with user data, not server infrastructure. In production,
executionDiris~/Library/Application Support/BrowserOS/.browseros(app-managed), while~/.browseros/is the user-level BrowserOS home. When no workspace is selected, sessions now default to~/.browseros/sessions/{conversationId}/. User-selected workspaces still override this.executionDirandresourcesDirare unchanged.Test plan
~/.browseros/sessions/directory~/.browseros/sessions/bun run typecheckpassesbun run lintpasses🤖 Generated with Claude Code