fix(update-bulletin-job): shell-quote workspace path in cleanup hint#26496
Conversation
| function updateBulletinHint(): string { | ||
| const workspace = getWorkspaceDirDisplay(); | ||
| return `Check ${workspace}/UPDATES.md — new release notes are present. Apply any assistant-facing behavior changes (new tools, deprecations, memory updates). If the user would benefit from knowing about a user-facing change, surface it only when the next topic makes it relevant — do not interrupt them with a proactive message. When you're done processing, delete the file by running \`cd ${workspace} && rm UPDATES.md\` (the bare-filename \`rm UPDATES.md\` is auto-allowed; path-qualified deletes are not). A silent no-op is preferable to low-signal chatter.`; | ||
| return `Check ${workspace}/UPDATES.md — new release notes are present. Apply any assistant-facing behavior changes (new tools, deprecations, memory updates). If the user would benefit from knowing about a user-facing change, surface it only when the next topic makes it relevant — do not interrupt them with a proactive message. When you're done processing, delete the file by running \`cd "${workspace}" && rm UPDATES.md\` (the bare-filename \`rm UPDATES.md\` is auto-allowed; path-qualified deletes are not). A silent no-op is preferable to low-signal chatter.`; |
There was a problem hiding this comment.
🔴 Quoting workspace path prevents tilde expansion in shell command hint
The PR wraps ${workspace} in double quotes in the shell command hint (cd "${workspace}" && rm UPDATES.md). However, getWorkspaceDirDisplay() at assistant/src/util/platform.ts:330-337 replaces the home directory prefix with ~ (e.g. ~/.vellum/workspace). In bash, tilde expansion does not occur inside double quotes — cd "~/.vellum/workspace" fails with "No such file or directory" because bash treats the literal character ~ as part of the path rather than expanding it to the home directory. This is the common case for all users whose workspace is under their home directory. The agent receiving this hint would attempt to execute the failing cd command and be unable to delete UPDATES.md, causing the bulletin job to retry on every startup without ever completing.
Prompt for agents
The fix needs to ensure the workspace path is both properly quoted (for spaces) and tilde-expandable. Options:
1. Use getWorkspaceDir() (the absolute path) instead of getWorkspaceDirDisplay() (the display path with ~) for the shell command portion of the hint string. The display path with ~ can still be used in the prose portion (Check ~/...UPDATES.md). This is the cleanest fix.
2. Use single-quote + unquoted tilde: cd ~/'rest/of/path' — this allows tilde expansion while quoting the rest. But this is fragile if the path doesn't start with ~.
3. Use $HOME instead of ~: cd "$HOME/.vellum/workspace" — but this requires changing getWorkspaceDirDisplay() or doing a string replacement.
Recommended: In updateBulletinHint() at assistant/src/prompts/update-bulletin-job.ts:23-26, import getWorkspaceDir (the absolute path variant) from platform.ts and use it for the cd command. The absolute path (e.g. /Users/sidd/.vellum/workspace) works correctly inside double quotes. Keep using getWorkspaceDirDisplay() for the human-readable reference at the start of the hint.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 880f9ba218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function updateBulletinHint(): string { | ||
| const workspace = getWorkspaceDirDisplay(); | ||
| return `Check ${workspace}/UPDATES.md — new release notes are present. Apply any assistant-facing behavior changes (new tools, deprecations, memory updates). If the user would benefit from knowing about a user-facing change, surface it only when the next topic makes it relevant — do not interrupt them with a proactive message. When you're done processing, delete the file by running \`cd ${workspace} && rm UPDATES.md\` (the bare-filename \`rm UPDATES.md\` is auto-allowed; path-qualified deletes are not). A silent no-op is preferable to low-signal chatter.`; | ||
| return `Check ${workspace}/UPDATES.md — new release notes are present. Apply any assistant-facing behavior changes (new tools, deprecations, memory updates). If the user would benefit from knowing about a user-facing change, surface it only when the next topic makes it relevant — do not interrupt them with a proactive message. When you're done processing, delete the file by running \`cd "${workspace}" && rm UPDATES.md\` (the bare-filename \`rm UPDATES.md\` is auto-allowed; path-qualified deletes are not). A silent no-op is preferable to low-signal chatter.`; |
There was a problem hiding this comment.
Use shell-valid workspace path in cleanup hint
getWorkspaceDirDisplay() normalizes home-relative paths to ~ (assistant/src/util/platform.ts), but this change now emits cd "${workspace}". In the default local setup that becomes cd "~/.vellum/workspace", and shells do not expand ~ inside quotes, so cd fails and rm UPDATES.md never executes. That breaks the intended cleanup flow for common installations; use an absolute path for the command (or escape a non-~ path) instead of quoting the display-form path.
Useful? React with 👍 / 👎.
Quote the interpolated workspace path so the agent-facing cleanup command works when VELLUM_WORKSPACE_DIR contains spaces.
Addresses feedback on #26460.