Skip to content

fix(computer-use): scope HarmonyOS cleanup to owned temporary files #5895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hmbown merged 4 commits into from
Sep 5, 2026
+49 −6
Merged

fix(computer-use): scope HarmonyOS cleanup to owned temporary files #5895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7643c0a
fix(computer-use): contain HarmonyOS temporary-file cleanup
Sep 5, 2026
4f18c59
Merge remote-tracking branch 'origin/main' into fix/hdc-owned-temp-09…
Sep 5, 2026
29bdcf1
fix(computer-use): preserve HDC results when temporary cleanup fails
Sep 5, 2026
030ff76
Merge remote-tracking branch 'origin/main' into fix/hdc-owned-temp-09…
Sep 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions crates/tui/plugins/computer-use/src/transport.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,15 @@ export function hdcExec(computer) {
return localPath;
},
async readFile(remotePath, opts = {}) {
const tmp = path.join(os.tmpdir(), `cu-hdc-${crypto.randomBytes(4).toString("hex")}`);
await this.pullFile(remotePath, tmp, opts);
const data = await fs.promises.readFile(tmp);
await fs.promises.rm(path.dirname(tmp), { recursive: true, force: true }).catch(() => {});
return data;
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 {
// Cleanup must not replace downloaded bytes or the original I/O error.
await fs.promises.rm(dir, { recursive: true, force: true }).catch(() => {});
}
},
};
}
Expand Down
41 changes: 40 additions & 1 deletion crates/tui/plugins/computer-use/tests/exec-transport.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// exec + transport safety tests.
import { test } from "node:test";
import assert from "node:assert/strict";
import fs from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { run, runOk, ExecError, have, trim } from "../src/exec.mjs";
import { safeRemotePath, b64, localExec } from "../src/transport.mjs";
import { safeRemotePath, b64, localExec, hdcExec } from "../src/transport.mjs";

test("run captures stdout/stderr and exit codes without a shell", async () => {
const r = await run("node", ["-e", "console.log('hello'); console.error('boo')"]);
Expand Down Expand Up @@ -55,3 +58,39 @@ test("localExec provides run/runOk/tmpFile", async () => {
const f = ex.tmpFile("cu-test-");
assert.ok(typeof f === "string");
});

for (const cleanupFails of [false, true]) {
for (const outcome of ["success", "transfer failure", "read failure"]) {
test(`hdc readFile preserves ${outcome} when cleanup ${cleanupFails ? "fails" : "succeeds"}`, async (t) => {
// Contain even the old recursive-deletion bug inside this fixture.
const root = await fs.mkdtemp(path.join(os.tmpdir(), "cu-hdc-test-"));
const realRm = fs.rm.bind(fs);
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");

@codewhale-agent codewhale-agent Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

assert.ok(path.basename(dir).startsWith("cu-hdc-"));

@codewhale-agent codewhale-agent Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw Object.assign(new Error("cleanup denied"), { code: "EACCES" });

@codewhale-agent codewhale-agent Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

});
}

@codewhale-agent codewhale-agent Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t.mock.method(os, "tmpdir", () => root);
await fs.writeFile(path.join(root, "unrelated"), "keep me");
const ex = hdcExec({});
const transferError = new Error("transfer interrupted");
ex.pullFile = async (remote, local) => {
assert.equal(remote, "fixture.txt");
if (outcome === "read failure") return;
await fs.writeFile(local, "downloaded bytes");
if (outcome === "transfer failure") throw transferError;
};
if (outcome === "success") {
assert.equal((await ex.readFile("fixture.txt")).toString(), "downloaded bytes");
} else {
await assert.rejects(ex.readFile("fixture.txt"), (error) =>
outcome === "transfer failure" ? error === transferError : error.code === "ENOENT");
}
assert.equal(await fs.readFile(path.join(root, "unrelated"), "utf8"), "keep me");
if (!cleanupFails) assert.deepEqual(await fs.readdir(root), ["unrelated"]);
});
}
}
Loading