fix(computer-use): scope HarmonyOS cleanup to owned temporary files - #5895
fix(computer-use): scope HarmonyOS cleanup to owned temporary files#5895Hmbown wants to merge 4 commits into
Conversation
Fixes #5894. Download into an owned mkdtemp directory and clean it in finally so successful reads cannot delete unrelated temporary files and failed transfers do not leak partial downloads. Validation: computer-use suite 34/34 passed. Isolated regression fixture reproduces unrelated-file deletion and leaked partial transfer on the previous implementation. Local file-read smoke passed. No live HarmonyOS device acceptance claimed. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_48738b28-92ff-4688-8a48-c109e70ab2d1) |
There was a problem hiding this comment.
🟡 Changes recommended
The new finally cleanup can throw and mask the original transfer/read error (or fail an otherwise-successful read) unless cleanup errors are made best-effort again.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes unsafe temporary-file cleanup in the HarmonyOS (hdc) transport readFile() implementation by ensuring downloads occur in a uniquely owned temp directory and only that directory is removed, preventing accidental deletion of unrelated files and cleaning up partial downloads on failure.
Changes:
- Update
hdcExec().readFile()to download into anmkdtemp-created owned directory and remove it in afinallyblock. - Add parameterized regression tests covering successful reads, transfer failures, and read failures, ensuring unrelated temp files remain intact and cleanup is complete.
File summaries
| File | Description |
|---|---|
| crates/tui/plugins/computer-use/src/transport.mjs | Scopes HarmonyOS readFile downloads/cleanup to an owned temp directory to prevent cross-file deletion and ensure cleanup on all outcomes. |
| crates/tui/plugins/computer-use/tests/exec-transport.test.mjs | Adds regression tests asserting unrelated temp files are preserved and owned temp artifacts are cleaned up for success/failure paths. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "cu-hdc-")); | ||
| try { | ||
| const tmp = path.join(dir, "out"); | ||
| await this.pullFile(remotePath, tmp, opts); | ||
| return await fs.promises.readFile(tmp); | ||
| } finally { | ||
| await fs.promises.rm(dir, { recursive: true, force: true }); | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7643c0a1c4
ℹ️ 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".
| await this.pullFile(remotePath, tmp, opts); | ||
| return await fs.promises.readFile(tmp); | ||
| } finally { | ||
| await fs.promises.rm(dir, { recursive: true, force: true }); |
There was a problem hiding this comment.
If removing the temporary directory fails—for example because of a transient Windows file lock—this awaited rm replaces a successful read result and also masks any transfer or read error from the try block. The previous cleanup explicitly ignored removal failures, so keep cleanup best-effort or otherwise preserve the primary result/error while reporting the cleanup failure separately.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…13-20260905 Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a3a6043d-1e70-4eb4-8440-08501c9f70c7) |
|
Claude finished @Hmbown's task in 1m 16s —— View job Review:
|
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Fixes the HarmonyOS readFile cleanup bug where the previous code called fs.promises.rm(path.dirname(tmp), { recursive: true, force: true }) and could delete the entire os.tmpdir(). The new implementation downloads into a unique mkdtemp directory and removes only that directory in a finally block. It also relocates the computer-use plugin bundle to crates/tui/plugins/computer-use and updates CI, .gitignore, Rust include_str paths, manifest validation, and UI source references. Tests cover success, transfer failure, and read failure.
Findings
- [WARNING] Cleanup failure in finally can mask the original error or reject a successful read (
crates/tui/plugins/computer-use/src/transport.mjs:127)
In readFile, the finally block awaits fs.promises.rm without a catch. If rm rejects, it overrides any transfer/read error, and it can also turn an otherwise successful read into a rejected promise. The previous implementation swallowed cleanup errors with .catch(() => {}). Consider catching cleanup failures in the finally block so they do not mask the primary outcome.
Suggestions
-
crates/tui/plugins/computer-use/src/transport.mjs:127— Add a catch to the cleanup call in the finally block so a failed rm does not mask the original transfer/read error or reject an already successful read.await fs.promises.rm(dir, { recursive: true, force: true }).catch(() => {});
Assessment
The core bug fix is correct and well tested: it scopes cleanup to an owned temporary directory and exercises success, transfer failure, and read failure. The path relocation updates are consistent with the new bundle location. The main remaining concern is cleanup error handling in the finally block, which should be adjusted to preserve the original operation result.
Advisory review by Codewhale (codewhale review --pr 5895 --post, head 4f18c59b1a2a1cee37758a13fc7401d40ba8df73). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| await this.pullFile(remotePath, tmp, opts); | ||
| return await fs.promises.readFile(tmp); | ||
| } finally { | ||
| await fs.promises.rm(dir, { recursive: true, force: true }); |
There was a problem hiding this comment.
[WARNING] Cleanup failure in finally can mask the original error or reject a successful read
In readFile, the finally block awaits fs.promises.rm without a catch. If rm rejects, it overrides any transfer/read error, and it can also turn an otherwise successful read into a rejected promise. The previous implementation swallowed cleanup errors with .catch(() => {}). Consider catching cleanup failures in the finally block so they do not mask the primary outcome.
| await this.pullFile(remotePath, tmp, opts); | ||
| return await fs.promises.readFile(tmp); | ||
| } finally { | ||
| await fs.promises.rm(dir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Add a catch to the cleanup call in the finally block so a failed rm does not mask the original transfer/read error or reject an already successful read.
| await fs.promises.rm(dir, { recursive: true, force: true }); | |
| await fs.promises.rm(dir, { recursive: true, force: true }).catch(() => {}); |
Keep recursive cleanup confined to the owned directory and best effort, so cleanup failure cannot mask a transfer/read error or reject a successful download. Validation: 37/37 plugin tests pass, including six success/failure/cleanup combinations; git diff --check passed. Runtime root has no npm test/check:web scripts. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a4630fb9-4154-4a51-b7f6-ece87b1ea16e) |
|
Claude finished @Hmbown's task in 1m 7s —— View job Review:
|
|
Review follow-up is pushed at 29bdcf1. Cleanup remains confined to its owned temporary directory and is best effort, preserving both successful bytes and the original transfer/read error when directory removal fails. 37/37 plugin tests pass, including success, transfer failure, and read failure with both successful and failed cleanup. Hosted CI is running on this new head. |
There was a problem hiding this comment.
Codewhale review
The PR fixes unsafe HarmonyOS temp cleanup by creating a unique per-read directory with mkdtemp and removing only that directory in a finally block. It also relocates the computer-use bundle to crates/tui/plugins/computer-use and updates references. The production change is correct and the new filesystem tests cover success, transfer failure, read failure, and cleanup failure paths.
Findings
- [WARNING] Cleanup-failure test assertions can be swallowed by production catch (
crates/tui/plugins/computer-use/tests/exec-transport.test.mjs:73)
In exec-transport.test.mjs, the cleanupFails branch asserts inside the mocked fs.rm that the removed path is the owned temp directory. However, readFile intentionally catches all cleanup errors with.catch(() => {}), so an AssertionError thrown by those assertions would be swallowed and the test could pass even if readFile attempted to remove the wrong directory. The non-mocked cleanup-success cases still catch root deletion via the sentinel file, so the suite is not currently blind to the original bug, but this branch provides weaker protection than it appears.
Suggestions
crates/tui/plugins/computer-use/tests/exec-transport.test.mjs:73— Record the directory passed to the mocked fs.rm and assert the recorded path after readFile settles, so the assertion is outside the production catch and cannot be swallowed.
Assessment
The production change is correct and limits cleanup blast radius to the owned temporary directory. The path relocation updates are consistent. No production correctness issues found; the only concern is test assertion visibility in the cleanup-failure branch.
Advisory review by Codewhale (codewhale review --pr 5895 --post, head 29bdcf1dcaaa76ec1cf5713fe28826e65f536558). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| t.mock.method(fs, "rm", async (dir) => { | ||
| assert.equal(path.dirname(dir), root, "cleanup stays inside its fixture"); | ||
| assert.ok(path.basename(dir).startsWith("cu-hdc-")); | ||
| throw Object.assign(new Error("cleanup denied"), { code: "EACCES" }); |
There was a problem hiding this comment.
[WARNING] Cleanup-failure test assertions can be swallowed by production catch
In exec-transport.test.mjs, the cleanupFails branch asserts inside the mocked fs.rm that the removed path is the owned temp directory. However, readFile intentionally catches all cleanup errors with .catch(() => {}), so an AssertionError thrown by those assertions would be swallowed and the test could pass even if readFile attempted to remove the wrong directory. The non-mocked cleanup-success cases still catch root deletion via the sentinel file, so the suite is not currently blind to the original bug, but this branch provides weaker protection than it appears.
| assert.ok(path.basename(dir).startsWith("cu-hdc-")); | ||
| throw Object.assign(new Error("cleanup denied"), { code: "EACCES" }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Record the directory passed to the mocked fs.rm and assert the recorded path after readFile settles, so the assertion is outside the production catch and cannot be swallowed.
…13-20260905 Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_279f7683-ac6e-436b-b26a-c90f933ae540) |
|
Claude finished @Hmbown's task in 1m 50s —— View job Review:
|
There was a problem hiding this comment.
Codewhale review
The production change correctly scopes HarmonyOS hdc readFile temp cleanup to a dedicated mkdtemp directory and uses finally to clean up failed transfers/reads without masking original errors. Tests cover the main outcome/cleanup matrix, but one cleanup-path assertion is unobservable because production's cleanup catch swallows the mock rejection.
Findings
- [WARNING] Cleanup path assertions in fs.rm mock are swallowed by production .catch (
crates/tui/plugins/computer-use/tests/exec-transport.test.mjs:71)
In cleanupFails=true cases, the installed fs.rm mock performs assert.equal/assert.ok inside the async function. If cleanup targets the wrong directory, these assertions reject the mock promise, but transport.mjs's finally uses.catch(() => {}), so the AssertionError is swallowed and the test still passes. The path safety check should run outside the mocked call, for example by recording the cleanup path and asserting after readFile settles.
Suggestions
crates/tui/plugins/computer-use/tests/exec-transport.test.mjs:71— Record the cleanup path in a variable inside the mock and assert path.dirname and basename after ex.readFile settles. This makes a wrong cleanup target fail the test despite production's cleanup-error catch.
Assessment
Production fix is correct and scoped. The tests cover the important lifecycle paths, but the cleanup-failure branch should be strengthened so its cleanup-path assertions cannot be swallowed.
Advisory review by Codewhale (codewhale review --pr 5895 --post, head 030ff761a6fc2a1cda14fb9223e524a16abc645c). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| t.after(() => realRm(root, { recursive: true, force: true })); | ||
| if (cleanupFails) { | ||
| t.mock.method(fs, "rm", async (dir) => { | ||
| assert.equal(path.dirname(dir), root, "cleanup stays inside its fixture"); |
There was a problem hiding this comment.
[WARNING] Cleanup path assertions in fs.rm mock are swallowed by production .catch
In cleanupFails=true cases, the installed fs.rm mock performs assert.equal/assert.ok inside the async function. If cleanup targets the wrong directory, these assertions reject the mock promise, but transport.mjs's finally uses .catch(() => {}), so the AssertionError is swallowed and the test still passes. The path safety check should run outside the mocked call, for example by recording the cleanup path and asserting after readFile settles.
| if (cleanupFails) { | ||
| t.mock.method(fs, "rm", async (dir) => { | ||
| assert.equal(path.dirname(dir), root, "cleanup stays inside its fixture"); | ||
| assert.ok(path.basename(dir).startsWith("cu-hdc-")); |
There was a problem hiding this comment.
Record the cleanup path in a variable inside the mock and assert path.dirname and basename after ex.readFile settles. This makes a wrong cleanup target fail the test despite production's cleanup-error catch.
A successful HarmonyOS file read downloaded directly under the system temporary directory and recursively removed its parent, potentially deleting unrelated temporary files. Failed transfers also left partial downloads behind.
Download into a unique owned directory and remove only that directory in a finally block. This also cleans up failed transfers and failed reads.
Fixes #5894
Validation: computer-use suite 34/34 passed; local file-read smoke returned the downloaded bytes and removed the owned directory. Three isolated filesystem cases preserve an unrelated sentinel and check cleanup on success, transfer failure, and read failure. Running those tests against the prior code reproduces unrelated-file deletion and leaked partial downloads entirely within the fixture. No real HarmonyOS device acceptance is claimed.
Depends on the bundle relocation in #5890; the defect predates that move. This is v0.9.13 work and does not alter the published v0.9.12 tag or assets.
Note
Low Risk
Localized temp-file lifecycle fix in the computer-use HDC transport with regression tests; no auth, data model, or API surface changes.
Overview
HarmonyOS
hdcremote reads no longer download into the system temp root and recursively delete the parent folder—a pattern that could wipe unrelated temp files and leave partial files on failed transfers.hdcExec.readFilenow pulls into a dedicatedmkdtempdirectory undercu-hdc-, readsout, and removes only that directory in afinallyblock (errors from cleanup are swallowed so they cannot mask transfer or read failures).Tests add six parametrized cases (success, transfer failure, read failure × cleanup success/failure) that mock
pullFileand optionally failingfs.rm, asserting sentinel files survive and the correct error or bytes are returned—including reproducing the old parent-delete behavior in isolation.Reviewed by Cursor Bugbot for commit 030ff76. Bugbot is set up for automated code reviews on this repo. Configure here.