feat(desktop): add nonblocking File Workspace and editor search - #62616
feat(desktop): add nonblocking File Workspace and editor search#62616seagpt wants to merge 3 commits into
Conversation
Add independent inline location navigation and hardened local/remote file mutations without changing the active agent working directory.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused workspace-browser contribution. The current desktop browser is still directly bound to the active session cwd (apps/desktop/src/app/right-sidebar/index.tsx:31,50), so the premise is valid.
Problems
hermes_cli/web_server.py:2243permanently deletes remote files withshutil.rmtree/unlink, whileapps/desktop/src/app/right-sidebar/file-actions.tsx:137retains the existing “moved to the Trash” confirmation. Remote users receive a materially incorrect destructive-operation warning.apps/desktop/electron/fs-workspace-ops.ts:17keeps intermediate symlinks lexical beforeguardSensitive; existing hardened reads canonicalize real paths inapps/desktop/electron/hardening.ts:251,284. Mutation guards need the equivalent canonical-parent treatment while retaining leaflstatbehavior.apps/desktop/electron/fs-workspace-ops.ts:141uses onlyfs.promises.rename, unlike remoteshutil.moveathermes_cli/web_server.py:2276; local moves across filesystems have no fallback.
Suggested changes
- Make the remote-delete dialog explicitly permanent or implement recoverable remote deletion.
- Canonicalize mutation parents and add intermediate-symlink coverage.
- Handle or explicitly constrain
EXDEVlocal moves, with regression coverage.
Automated hermes-sweeper review.
| <> | ||
| <ConfirmDialog | ||
| confirmLabel={t.fileMenu.delete} | ||
| description={t.fileMenu.deleteBody} |
There was a problem hiding this comment.
For remote mode this confirmation is incorrect: /api/fs/delete permanently uses shutil.rmtree or unlink, while deleteBody promises the item will be moved to Trash. Use remote-specific permanent-delete copy or provide a recoverable remote trash implementation.
There was a problem hiding this comment.
Resolved in 4002d3336d4ee44372c00907bd5ff12b1a6d233a. Remote delete confirmation now explicitly states permanent deletion; only local Electron delete uses Trash wording. Covered by apps/desktop/src/app/right-sidebar/file-actions.test.tsx.
| } | ||
|
|
||
| function resolved(raw: unknown, purpose: string): string { | ||
| return resolveRequestedPathForIpc(String(raw || '').trim(), { purpose }) |
There was a problem hiding this comment.
This only normalizes syntax; it does not canonicalize intermediate symlink components before guardSensitive. A path through a symlinked directory can evade path-based sensitive-file guards. Resolve the parent via realpath while preserving the final leaf for lstat, then guard the canonical target.
There was a problem hiding this comment.
Resolved in 4002d3336d4ee44372c00907bd5ff12b1a6d233a. Mutation parents are canonicalized through intermediate symlinks before sensitive-path/root guards, while the final leaf retains lstat semantics. Regression coverage exercises a non-sensitive alias into a sensitive ancestor in both Electron and Webapp mutation suites.
| if (await collision(target)) { | ||
| throw new Error(`"${path.basename(source)}" already exists`) | ||
| } | ||
| await fs.promises.rename(source, target) |
There was a problem hiding this comment.
A raw rename fails with EXDEV when the user selects a destination on another filesystem. The remote path uses shutil.move; please either add a hardened cross-device fallback locally or constrain/document the operation and test the chosen behavior.
There was a problem hiding this comment.
Resolved in 4002d3336d4ee44372c00907bd5ff12b1a6d233a. Local moves catch EXDEV and use a guarded copy/remove fallback with cleanup and no-overwrite checks. The Electron workspace-op suite passes 8/8 under its correct Node runner, including cross-device behavior coverage.
Resolve permanent-delete messaging, canonical-parent symlink guards, cross-device move fallback, and retryable item-scoped mutations. Add native CodeMirror Find/Replace with localized controls and regression coverage.
|
Reviewer follow-up pushed in Resolved all three inline blockers with regression coverage:
The update also keeps mutation failures retryable and adds native CodeMirror Find/Replace. Current-main broad comparison has the same 24 assertion failures as |
Summary
Reviewer findings resolved
lstatbehaviorEXDEVthrough a guarded copy/remove fallback with cleanup and regression coverageBehavior and safety
setCurrentCwd; later agent cwd updates do not redirect the browser/api/*middlewareValidation
git diff --check: passedassert-dist-built: passedCurrent-main broad baseline
Source-wide Desktop Vitest under the same Node 24 runtime:
origin/main: 1,173 passed / 24 failedVitest reports one additional failed file because
electron/fs-workspace-ops.test.tsis anode:testfile and appears empty under jsdom Vitest; it passes 8/8 undernode --import tsx --test. Unhandled errors came from the existingstreaming.test.tsxtimer/window teardown and varied across broad runs, not from File Workspace or CodeMirror tests.Visual QA
Dogfooded real styled File Workspace/editor components at normal and narrow widths:
Related / overlap checked
mainNotes
@codemirror/searchis the only added runtime dependency for native editor search