feat: setup/teardown commands and configurable symlink dirs in project settings#23
feat: setup/teardown commands and configurable symlink dirs in project settings#23linniksa wants to merge 4 commits intojohannesjo:mainfrom
Conversation
There was a problem hiding this comment.
Thank you very much for this and sorry for the late reply. Somehow I wasn't receiving notifications for PRs opened here....
PR #23 Review: Setup/Teardown Commands & Configurable Symlink Dirs
Strengths
- Clean
CommandListEditorextraction — reusable for bookmarks, setup, and teardown PathSelectorwith directory autocomplete is well-designed (cancellation, keyboard nav, Tab-to-drill)SetupBannerwith auto-scroll, retry/skip is solid UX- Prompt stashing to prevent agent from acting during setup is clever
requestAnimationFrame-buffered log flushing reduces store churn- Path traversal protection in
listProjectEntriesandcreateWorktreelooks correct - Types are properly optional — backward compatible with persisted state
CRITICAL (must fix before merge)
1. Unintended deletions from rebase — notifications and plan watcher cleanup broken
The diff removes StopPlanWatcher, ShowNotification, and NotificationClicked from channels.ts, register.ts, and preload.cjs. These are still actively used:
src/store/tasks.tscallsinvoke(IPC.StopPlanWatcher, ...)on task close/collapsesrc/store/desktopNotifications.tsusesShowNotificationandNotificationClicked
This will leak plan file watchers and silently break all desktop notifications. Appears to be a rebase artifact — these have nothing to do with the PR's intent.
2. deleteTask signature change drops taskId — plan watcher leak on backend
The PR changes deleteTask in electron/ipc/tasks.ts from an options object to positional params and removes the stopPlanWatcher(opts.taskId) call. Combined with #1, plan watchers are never cleaned up on task deletion.
3. STDERR_CAP removed from pushTask — reverts recent bug fix
Commit c0d312b on main ("cap unbounded buffers and stop leaked plan watchers") added a 4096-byte cap to stderr in pushTask. The PR removes it, re-introducing unbounded buffer growth from verbose git push output.
IMPORTANT (should fix)
4. .claude shallow-symlink special handling removed
The old code shallow-symlinked .claude (excluding plans and settings.local.json which must be per-worktree). Now it's a plain symlink. If users add .claude to their symlink dirs, plans and local settings will collide across worktrees.
5. Race condition: task closed during setup
If closeTask runs while setup is in progress, spawned processes aren't killed, and .finally() will try to write to a deleted task in the store.
6. Teardown output silently discarded
Teardown creates a Channel<string>() but never reads from it. Makes debugging teardown failures impossible.
7. IPC.RunSetupCommands used for teardown too
Semantically misleading. Consider renaming to RunProjectCommands or adding a separate channel.
Minor / Nice-to-Have
8. subpath arg in register.ts should be validated (assertOptionalString)
9. Branch name typo: feature/setup-and-terdown (missing "a")
10. PathSelector renders input even when projectRoot is undefined (no-project state in NewTaskDialog)
11. logBuffer may not flush if the final rAF fires after the promise resolves — force-flush in .then()/.catch()
Verdict
The core feature (setup/teardown, PathSelector, CommandListEditor) is well-implemented. However, the three critical issues are regressions from what appears to be a bad rebase — they break notifications, leak file watchers, and revert a buffer cap fix. These must be resolved before merge.
|
Security: shell metacharacter injection via
// setup.ts – the substituted string is passed directly to a shell
const cmd = expandVars(raw); // e.g. "npm install --prefix /home/user/my project; echo pwned"
spawn(cmd, { shell: true, ... });Fix options:
The simplest safe fix is to quote the values: const shellQuote = (s: string) => `'${s.replace(/'/g, "'\\''")}'`;
const expandVars = (cmd: string): string =>
cmd
.replace(/\$\{PROJECT_ROOT\}|\$PROJECT_ROOT\b/g, () => shellQuote(projectRoot))
.replace(/\$\{WORKTREE\}|\$WORKTREE\b/g, () => shellQuote(worktreePath));In practice, project roots set by the user on their own machine make this low-severity, but it's worth closing if paths can ever contain spaces (common on macOS).
|
|
Thank you very much! <3 |
johannesjo
left a comment
There was a problem hiding this comment.
New finding: In electron/ipc/setup.ts, $PROJECT_ROOT and $WORKTREE are substituted into the command string and then executed via spawn(cmd, { shell: true }). If either path contains shell metacharacters (spaces, semicolons, backticks), the shell will interpret them — potential shell injection. Fix: shell-quote the substituted values before interpolation.
In addition, the three critical rebase regressions from the existing review must be fixed before merge: removal of StopPlanWatcher/ShowNotification/NotificationClicked IPC channels (breaking notifications and leaking plan watchers), the deleteTask signature drop of stopPlanWatcher, and loss of the STDERR_CAP buffer guard. Details in the review comment.
Temporary instrumentation to isolate which boundary drops data when steps.json "does not always seem to be read properly". Logs: - [steps.watch] fs.watch event + filename - [steps.send] backend read result length - [steps.recv] renderer IPC receipt length
Supersedes johannesjo#23 by @linniksa. Re-implemented on current main to avoid a bad rebase in the original PR that would have reverted recently-landed fixes (StopPlanWatcher / ShowNotification / STDERR_CAP / deleteTask options-object) and shipped a shell-injection in setup.ts. **Feature** - Per-project setup/teardown commands run on worktree create/remove, configured in Edit Project. Output streams into a SetupBanner above the terminal; Retry / Skip on failure. Initial prompt is stashed while setup runs so the agent can't start working before it finishes. - Manual-input guard in sendPrompt drops user Enter while setupStatus is 'running' (banner makes the wait visible). - Teardown runs before worktree removal; hung teardown auto-aborts after 30s so closeTask never blocks. - New reusable CommandListEditor (setup/teardown inputs) and PathSelector (flat autocomplete over project dirs) components. - New per-project defaultSymlinkDirs: pre-fills symlink list on new task creation; falls back to auto-detected gitignored dirs when unset, so fresh projects still work with zero config. - createWorktree now accepts nested symlink paths (e.g. packages/app/node_modules) with mkdir -p on parents and post-normalization escape check. **Security** - setup.ts spawns sh -c <cmd> with $PROJECT_ROOT / $WORKTREE as env vars, not interpolated into the command string. Paths with spaces, semicolons, backticks etc. cannot break out of quoting. - Electron-internal env is filtered before spawn: NODE_OPTIONS, ELECTRON_*, LD_PRELOAD. **Internals** - Cancellation via AbortController: one controller per channel, spawn({ signal }) kills children on abort, typed AbortError in the close handler — no string-matching on error messages. - Setup log is capped at 64 KB with head-trim, preventing O(n^2) concat on noisy commands like npm install. - New IPC: ListProjectEntries, RunSetupCommands, RunTeardownCommands, CancelProjectCommands. All existing channels preserved. **Removed** - SymlinkDirPicker.tsx (replaced by PathSelector). Co-authored-by: Sergey Linnik <linniksa@gmail.com>
* origin/main: feat: add MiniMax M2.7 as configurable backend for Ask about Code (johannesjo#53) # Conflicts: # electron/ipc/register.ts # electron/preload.cjs
b4f3ed7 to
d5b5119
Compare
* origin/main: Feat/focus mode (johannesjo#76) Add existing worktree import flow (johannesjo#16) # Conflicts: # electron/ipc/channels.ts # electron/ipc/register.ts # electron/preload.cjs # src/components/EditProjectDialog.tsx # src/store/tasks.ts
|
@linniksa Thank you very much for this! Sorry for the delay. I took the liberty to make some adjustments. Could you maybe have a look if the updated version fits your vision? |
Add per-project setup and teardown commands that run automatically when worktrees are created or removed. This lets users configure things
like
npm install, environment setup, or cleanup scripts that execute in the context of each task's worktree.Setup/teardown commands:
CommandListEditorcomponent$PROJECT_ROOTand$WORKTREEvariable substitution in commandsSetupBannerabove the terminal with retry/skip on failureConfigurable symlink directories:
defaultSymlinkDirssettingPathSelectorcomponent with autocomplete backed byListProjectEntriesIPC (Tab to navigate into subdirectories, supports nestedpaths)