Harden send-to-R temp cleanup and restrict config scopes#175
Conversation
- Create per-call temp directories for sent R scripts - Remove script directories after execution or fallback cleanup - Restrict executable-path settings in untrusted workspaces
📝 WalkthroughWalkthroughVSCode extension manifest enhanced with untrusted workspace capabilities and machine-overridable configuration scopes. Temporary file creation strategy changed from timestamped files in shared OS temp directory to per-call temporary directories with matching recursive cleanup behavior. ChangesWorkspace Trust & Temp File Security
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editors/vscode/src/send-to-r/temp-file.ts`:
- Line 24: The cleanup currently uses fs.rmdirSync(path.dirname(file_path))
which only removes empty directories and leaks non-empty temp dirs; update the
cleanup in temp-file.ts to call fs.rmSync(path.dirname(file_path), { recursive:
true, force: true }) inside the existing try/catch (or equivalent best-effort
block) so the temp directory is removed recursively and safely; reference the
existing file_path variable and the surrounding cleanup logic to locate where to
replace rmdirSync with rmSync and keep the try/catch behavior.
🪄 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: b616149f-77a7-4390-948e-b2c72995507e
📒 Files selected for processing (3)
editors/vscode/package.jsoneditors/vscode/src/send-to-r/commands.tseditors/vscode/src/send-to-r/temp-file.ts
| export function schedule_temp_file_cleanup(file_path: string): void { | ||
| setTimeout(() => { | ||
| try { fs.unlinkSync(file_path); } catch {} | ||
| try { fs.rmdirSync(path.dirname(file_path)); } catch {} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is fs.rmdirSync deprecated in current Node.js, and is fs.rmSync(path, { recursive: true, force: true }) the recommended replacement for recursive temp-directory cleanup?
💡 Result:
Yes. In current Node.js, fs.rmdirSync with the recursive option is deprecated; Node.js documentation specifically says to use fs.rm(path, { recursive: true, force: true }) (or fs.rmSync / fs.promises.rm) for rm -rf-like recursive deletion and notes that the recursive option for rmdir has been deprecated/removed in favor of rm.[1][2][3] For recursive temp-directory cleanup, the recommended replacement is fs.rmSync(path, { recursive: true, force: true }). Node’s fs docs state: “To get a behavior similar to the rm -rf Unix command, use fs.rm with options { recursive: true, force: true }.”[1]
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: nodejs/node@0ddd75b
- 3: fs: move rmdir recursive option to end-of-life nodejs/node#58616
Use recursive/force dir removal for the JS fallback cleanup.
rmdirSync(dirname) only removes empty directories, so non-empty temp dirs leak. Use fs.rmSync(path.dirname(file_path), { recursive: true, force: true }) for robust best-effort cleanup. This is the recommended Node.js replacement for recursive directory removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editors/vscode/src/send-to-r/temp-file.ts` at line 24, The cleanup currently
uses fs.rmdirSync(path.dirname(file_path)) which only removes empty directories
and leaks non-empty temp dirs; update the cleanup in temp-file.ts to call
fs.rmSync(path.dirname(file_path), { recursive: true, force: true }) inside the
existing try/catch (or equivalent best-effort block) so the temp directory is
removed recursively and safely; reference the existing file_path variable and
the surrounding cleanup logic to locate where to replace rmdirSync with rmSync
and keep the try/catch behavior.
There was a problem hiding this comment.
temp-file.ts · L23 first runs fs.unlinkSync(file_path), so if R never executes, the fallback deletes src.R before rmdirSync(dirname). The claimed leak from “src.R still present” is not valid for the current code.
There was a problem hiding this comment.
@jbearak, you're right — my concern was invalid. The sequence is:
fs.unlinkSync(file_path)removessrc.R(the only file ever written into the per-call dir).fs.rmdirSync(path.dirname(file_path))is then called on an already-empty directory, which works correctly.
There is no leak scenario here. I also re-checked the Node.js deprecation note: it is specifically the recursive option on rmdirSync that is deprecated — using rmdirSync to remove a plain empty directory is still valid. My suggestion to switch to rmSync({ recursive: true, force: true }) was unnecessary for this code. I'll withdraw the concern.
🧠 Learnings used
Learnt from: jbearak
Repo: jbearak/raven PR: 170
File: editors/vscode/src/send-to-r/commands.ts:80-87
Timestamp: 2026-05-06T10:43:15.222Z
Learning: In the Raven VSCode extension, do not rely on vscode.window.onDidWriteTerminalData because it is a proposed API requiring --enable-proposed-api and cannot be distributed via the marketplace. For R terminal readiness, use terminal.sendText() to send commands (this buffers behind the prompt). For multi-line commands, route via a tempfile and source() to execute. These approaches avoid terminal startup races. If a race ever surfaces, apply a simple mitigation: an unconditional small delay on first terminal creation. This guidance applies to all TypeScript files under editors/vscode/src/send-to-r/ and should be followed to ensure reliable terminal interaction without depending on experimental APIs.
Benchmark ComparisonNo A baseline will be created when this branch (or a future PR) is merged to |
1 participant
Summary
send to Rscripts usingmkdtempSync, reducing symlink-race risk and keeping each script isolated.machine-overridableand restrict executable-path settings in untrusted workspaces to user/global scope only.Testing
temp-file.tsandcommands.tsconsistently.untrustedWorkspacesandscopeentries are added inpackage.jsonfor the affected settings.Summary by CodeRabbit
New Features
Bug Fixes