From ca03e01cb903eccd6082fd13e99a387f47cc4ae8 Mon Sep 17 00:00:00 2001 From: Dongni-Yang Date: Wed, 22 Jul 2026 14:52:11 +0800 Subject: [PATCH 1/3] fix(sandbox): verify session-export download wrote a file before recording a bundle (#7367) `openshell sandbox download` has an upstream process-exit race (NVIDIA/OpenShell) that can report success (exit 0) even when the transfer was rejected or failed and no file was written. NemoClaw's session export/backup trusted that exit code alone, so a dropped code could record a rejected or partial download as a valid session bundle. Add assertDownloadedFile(), which re-checks the outcome against the file system after each trusted `sandbox download`: the destination must exist as a regular file (and, for bundles that are never legitimately empty, be non-empty). Wire it into the three export download sites (OpenClaw tar, OpenClaw per-file dir, hermes). The hermes site verifies existence only, since a zero-session hermes export can legitimately be empty. This is a NemoClaw-side defensive mitigation; the root-cause exit-code race remains an upstream OpenShell fix, so this refs rather than closes the issue. Refs #7367 Signed-off-by: Dongni-Yang Co-Authored-By: Claude Fable 5 --- .../sandbox/sessions/download-verify.test.ts | 90 +++++++++++++++++ .../sandbox/sessions/download-verify.ts | 69 +++++++++++++ .../actions/sandbox/sessions/export.test.ts | 99 ++++++++++++++++++- src/lib/actions/sandbox/sessions/export.ts | 35 ++++--- 4 files changed, 272 insertions(+), 21 deletions(-) create mode 100644 src/lib/actions/sandbox/sessions/download-verify.test.ts create mode 100644 src/lib/actions/sandbox/sessions/download-verify.ts diff --git a/src/lib/actions/sandbox/sessions/download-verify.test.ts b/src/lib/actions/sandbox/sessions/download-verify.test.ts new file mode 100644 index 0000000000..cba2c6058e --- /dev/null +++ b/src/lib/actions/sandbox/sessions/download-verify.test.ts @@ -0,0 +1,90 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { assertDownloadedFile } from "./download-verify"; + +describe("assertDownloadedFile", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "nc-7367-verify-")); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it("passes when the download reported success and wrote a non-empty file", () => { + const target = path.join(dir, "bundle.tgz"); + fs.writeFileSync(target, "payload"); + expect(() => + assertDownloadedFile({ status: 0 }, target, { + remoteLabel: "/sandbox/x.tgz", + sandboxName: "alpha", + requireNonEmpty: true, + }), + ).not.toThrow(); + }); + + it("rejects a non-zero exit status with the exit code in the message", () => { + const target = path.join(dir, "bundle.tgz"); + fs.writeFileSync(target, "payload"); + expect(() => + assertDownloadedFile({ status: 1 }, target, { + remoteLabel: "/sandbox/x.tgz", + sandboxName: "alpha", + }), + ).toThrow(/Failed to download '\/sandbox\/x\.tgz' from sandbox 'alpha' \(exit 1\)\./); + }); + + // The #7367 core: openshell can exit 0 while writing nothing. Trusting the + // exit code alone would record the rejected download as a valid bundle. + it("rejects exit 0 when no file was written", () => { + const target = path.join(dir, "missing.tgz"); + expect(() => + assertDownloadedFile({ status: 0 }, target, { + remoteLabel: "/sandbox/x.tgz", + sandboxName: "alpha", + requireNonEmpty: true, + }), + ).toThrow(/reported success \(exit 0\) but no file was written to/); + }); + + it("rejects exit 0 when the destination is a directory, not a regular file", () => { + const target = path.join(dir, "adir"); + fs.mkdirSync(target); + expect(() => + assertDownloadedFile({ status: 0 }, target, { + remoteLabel: "/sandbox/x.tgz", + sandboxName: "alpha", + }), + ).toThrow(/reported success \(exit 0\) but '.*' is not a regular file/); + }); + + it("rejects exit 0 with an empty file when requireNonEmpty is set", () => { + const target = path.join(dir, "empty.tgz"); + fs.writeFileSync(target, ""); + expect(() => + assertDownloadedFile({ status: 0 }, target, { + remoteLabel: "/sandbox/x.tgz", + sandboxName: "alpha", + requireNonEmpty: true, + }), + ).toThrow(/reported success \(exit 0\) but wrote an empty file/); + }); + + it("allows an empty file when requireNonEmpty is not set (per-session files)", () => { + const target = path.join(dir, "session.jsonl"); + fs.writeFileSync(target, ""); + expect(() => + assertDownloadedFile({ status: 0 }, target, { + remoteLabel: "/sandbox/.openclaw/agents/main/sessions/session.jsonl", + sandboxName: "alpha", + }), + ).not.toThrow(); + }); +}); diff --git a/src/lib/actions/sandbox/sessions/download-verify.ts b/src/lib/actions/sandbox/sessions/download-verify.ts new file mode 100644 index 0000000000..694565cf0b --- /dev/null +++ b/src/lib/actions/sandbox/sessions/download-verify.ts @@ -0,0 +1,69 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; + +export interface DownloadOutcome { + status: number | null; +} + +export interface VerifyDownloadedFileOptions { + /** Sandbox-side source label used in error messages (e.g. the remote path). */ + remoteLabel: string; + /** Sandbox name used in error messages. */ + sandboxName: string; + /** + * Require the artifact to be non-empty. Set for bundles that are never + * legitimately empty (a gzip tarball, a hermes export); leave off for + * individual session files whose size we do not want to constrain. + */ + requireNonEmpty?: boolean; +} + +/** + * Confirm that an `openshell sandbox download` of a single file both reported + * success AND actually produced the artifact on the host. + * + * The exit status alone cannot be trusted: `openshell sandbox download` has a + * process-exit race that can report success (exit 0) even when the transfer + * was rejected or failed and no file was written (NVIDIA/OpenShell; NemoClaw + * #7367). Trusting exit 0 alone would let a rejected or partial download be + * recorded as a valid session bundle, so re-check the outcome against the file + * system before treating the download as complete. + * + * @throws if the download reported a non-zero status, wrote no file, wrote a + * non-regular file, or (when `requireNonEmpty`) wrote an empty file. + */ +export function assertDownloadedFile( + download: DownloadOutcome, + hostPath: string, + options: VerifyDownloadedFileOptions, +): void { + const { remoteLabel, sandboxName, requireNonEmpty = false } = options; + const prefix = `Failed to download '${remoteLabel}' from sandbox '${sandboxName}'`; + + if (download.status !== 0) { + throw new Error(`${prefix} (exit ${download.status}).`); + } + + let stat: fs.Stats; + try { + stat = fs.statSync(hostPath); + } catch { + throw new Error( + `${prefix}: openshell reported success (exit 0) but no file was written to '${hostPath}'.`, + ); + } + + if (!stat.isFile()) { + throw new Error( + `${prefix}: openshell reported success (exit 0) but '${hostPath}' is not a regular file.`, + ); + } + + if (requireNonEmpty && stat.size === 0) { + throw new Error( + `${prefix}: openshell reported success (exit 0) but wrote an empty file to '${hostPath}'.`, + ); + } +} diff --git a/src/lib/actions/sandbox/sessions/export.test.ts b/src/lib/actions/sandbox/sessions/export.test.ts index b157d78723..0911977c12 100644 --- a/src/lib/actions/sandbox/sessions/export.test.ts +++ b/src/lib/actions/sandbox/sessions/export.test.ts @@ -30,6 +30,7 @@ const getSandboxMock = registry.getSandbox as unknown as ReturnType; let consoleLogSpy: ReturnType; +let statSyncSpy: ReturnType; beforeEach(() => { captureMock.mockReset(); @@ -39,11 +40,18 @@ beforeEach(() => { getSandboxMock.mockReturnValue(null); consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + // Default to a present, non-empty regular file so the post-download artifact + // verification (assertDownloadedFile) treats the mocked download as complete. + // Individual tests override this to simulate a missing/empty artifact. + statSyncSpy = vi + .spyOn(fs, "statSync") + .mockReturnValue({ size: 42, isFile: () => true } as unknown as ReturnType); }); afterEach(() => { consoleErrorSpy.mockRestore(); consoleLogSpy.mockRestore(); + statSyncSpy.mockRestore(); }); function makeCapture(output: string, status = 0) { @@ -201,7 +209,7 @@ describe("exportSandboxSessions", () => { expect(result.resolvedSessionIds).toEqual(["sid-a", "sid-b"]); expect(result.resolvedFiles).toEqual(["sid-a.jsonl", "sid-b.jsonl"]); expect(result.hostDest).toBe(expectedHostDest); - expect(result.bundleBytes).toBeNull(); + expect(result.bundleBytes).toBe(42); }); it("writes a browsable directory of session files by default (dir format, no tar staging)", async () => { @@ -216,9 +224,6 @@ describe("exportSandboxSessions", () => { const mkdirSpy = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); const chmodSpy = vi.spyOn(fs, "chmodSync").mockImplementation(() => {}); - const statSpy = vi - .spyOn(fs, "statSync") - .mockReturnValue({ size: 42 } as unknown as ReturnType); const result = await exportSandboxSessions({ sandboxName: "alpha", @@ -277,7 +282,6 @@ describe("exportSandboxSessions", () => { mkdirSpy.mockRestore(); chmodSpy.mockRestore(); - statSpy.mockRestore(); }); it("dedupes resolved session ids when the same session is referenced by both alias and canonical key", async () => { @@ -459,6 +463,73 @@ describe("exportSandboxSessions", () => { expect(cleanupCall?.[1]).toMatchObject({ ignoreError: true }); }); + // #7367: `openshell sandbox download` can report success (exit 0) while + // writing nothing (an upstream process-exit race). The export must not treat + // that as a valid bundle, and must still clean up the in-sandbox staging file. + it("aborts and cleans up when the host download reports success but writes no file (#7367)", async () => { + captureMock.mockReturnValueOnce( + makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), + ); + // tar exec + download both report success... + runMock.mockReturnValueOnce(makeRun(0)).mockReturnValueOnce(makeRun(0)); + // ...but the host artifact never materialised. + statSyncSpy.mockImplementation(() => { + throw new Error("ENOENT"); + }); + await expect( + exportSandboxSessions({ + sandboxName: "alpha", + out: "./out.tgz", + format: "tar", + }), + ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); + const cleanupCall = runMock.mock.calls.at(-1); + expect(cleanupCall?.[0]).toContain("rm"); + expect(cleanupCall?.[0]).toContain("-f"); + expect(cleanupCall?.[1]).toMatchObject({ ignoreError: true }); + }); + + // #7367: a tar bundle is never legitimately empty (the zero-session case is + // refused earlier), so an exit-0 download that produced an empty file is the + // race, not a valid export — and cleanup must still run. + it("aborts and cleans up when the host download reports success but writes an empty tar bundle (#7367)", async () => { + captureMock.mockReturnValueOnce( + makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), + ); + runMock.mockReturnValueOnce(makeRun(0)).mockReturnValueOnce(makeRun(0)); + statSyncSpy.mockReturnValue({ + size: 0, + isFile: () => true, + } as unknown as ReturnType); + await expect( + exportSandboxSessions({ + sandboxName: "alpha", + out: "./out.tgz", + format: "tar", + }), + ).rejects.toThrow(/reported success \(exit 0\) but wrote an empty file/); + const cleanupCall = runMock.mock.calls.at(-1); + expect(cleanupCall?.[0]).toContain("rm"); + expect(cleanupCall?.[0]).toContain("-f"); + expect(cleanupCall?.[1]).toMatchObject({ ignoreError: true }); + }); + + // #7367: the per-file dir path must abort the export when a session download + // reports success but wrote no file, rather than returning a partial export. + it("aborts a dir export when a per-file download reports success but writes no file (#7367)", async () => { + captureMock.mockReturnValueOnce( + makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), + ); + const mkdirSpy = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); + statSyncSpy.mockImplementation(() => { + throw new Error("ENOENT"); + }); + await expect( + exportSandboxSessions({ sandboxName: "alpha", out: "./sessions-alpha" }), + ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); + mkdirSpy.mockRestore(); + }); + it("emits a JSON manifest with resolved session ids, files, host path, and bundle size when --json is set", async () => { captureMock.mockReturnValueOnce( makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), @@ -596,6 +667,24 @@ describe("exportSandboxSessions (hermes sandbox)", () => { expect(renameSpy).not.toHaveBeenCalled(); }); + // #7367: an exit-0 download that wrote no file must abort before the staging + // file is renamed into place, and must still clean up the in-sandbox staging. + it("aborts before rename and cleans up when the host download reports success but writes no file (#7367)", async () => { + getSandboxMock.mockReturnValue({ name: "alpha", agent: "hermes" }); + runMock.mockReturnValueOnce(makeRun(0)).mockReturnValueOnce(makeRun(0)); + statSyncSpy.mockImplementation(() => { + throw new Error("ENOENT"); + }); + + await expect(exportSandboxSessions({ sandboxName: "alpha" })).rejects.toThrow( + /reported success \(exit 0\) but no file was written/, + ); + expect(renameSpy).not.toHaveBeenCalled(); + const cleanupCall = runMock.mock.calls.at(-1); + expect(cleanupCall?.[0]).toContain("rm"); + expect(cleanupCall?.[0]).toContain("-f"); + }); + it("fails closed when chmod on the staging file errors so a permissive host cannot end up with a world-readable session bundle", async () => { getSandboxMock.mockReturnValue({ name: "alpha", agent: "hermes" }); chmodSpy.mockImplementation(() => { diff --git a/src/lib/actions/sandbox/sessions/export.ts b/src/lib/actions/sandbox/sessions/export.ts index 6b45c5bbaf..b8d8edddff 100644 --- a/src/lib/actions/sandbox/sessions/export.ts +++ b/src/lib/actions/sandbox/sessions/export.ts @@ -47,13 +47,14 @@ import * as registry from "../../../state/registry"; import { ensureLiveSandboxOrExit } from "../gateway-state"; import { resolveHostPathFromCwd } from "../host-path"; import { isWarmupSessionId } from "../warmup-session"; -import { type SessionIndexEntry, parseSessionIndex } from "./session-index"; +import { assertDownloadedFile } from "./download-verify"; import { DEFAULT_AGENT_ID, parseAgentIdFromSessionKey, validateAgentId, validateSessionKey, } from "./paths"; +import { parseSessionIndex, type SessionIndexEntry } from "./session-index"; export type SessionsExportFormat = "dir" | "tar" | "jsonl"; @@ -168,11 +169,11 @@ export async function exportSandboxSessions( ["sandbox", "download", opts.sandboxName, tarballRemote, hostDest], { ignoreError: true, stdio: "inherit" }, ); - if (downloadResult.status !== 0) { - throw new Error( - `Failed to download '${tarballRemote}' from sandbox '${opts.sandboxName}' (exit ${downloadResult.status}).`, - ); - } + assertDownloadedFile(downloadResult, hostDest, { + remoteLabel: tarballRemote, + sandboxName: opts.sandboxName, + requireNonEmpty: true, + }); } finally { // Best-effort cleanup of the in-sandbox staging tarball. Runs even when // tar/download fail so a partial export cannot leave a bundle of session @@ -214,11 +215,10 @@ export async function exportSandboxSessions( ["sandbox", "download", opts.sandboxName, `${sourceDir}/${file}`, localPath], { ignoreError: true, stdio: "inherit" }, ); - if (downloadResult.status !== 0) { - throw new Error( - `Failed to download '${file}' from sandbox '${opts.sandboxName}' (exit ${downloadResult.status}).`, - ); - } + assertDownloadedFile(downloadResult, localPath, { + remoteLabel: file, + sandboxName: opts.sandboxName, + }); // Session JSONL can contain pasted secrets — restrict each file to owner-only. hardenPermissions(localPath); } @@ -332,11 +332,14 @@ async function exportHermesSessions(opts: SessionsExportOptions): Promise Date: Thu, 23 Jul 2026 09:22:44 +0800 Subject: [PATCH 2/3] fix(sandbox): stage session-export downloads before publishing (#7367) Address review on #7371: verify each download against a fresh mkdtemp staging path and rename into place only after it passes, so a stale file from an earlier export cannot satisfy the exit-0/no-write check. Covers both the tar bundle and the per-file dir path (the tar path had the same exposure). Document the fresh-path precondition on assertDownloadedFile; correct the requireNonEmpty comment re: hermes; restore leaked fs spies in finally. Refs #7367 Signed-off-by: Dongni-Yang Co-Authored-By: Claude Fable 5 --- .../sandbox/sessions/download-verify.ts | 14 +- .../actions/sandbox/sessions/export.test.ts | 303 ++++++++++++------ src/lib/actions/sandbox/sessions/export.ts | 88 +++-- test/sandbox-sessions-export-cli.test.ts | 15 +- 4 files changed, 281 insertions(+), 139 deletions(-) diff --git a/src/lib/actions/sandbox/sessions/download-verify.ts b/src/lib/actions/sandbox/sessions/download-verify.ts index 694565cf0b..67d61cae65 100644 --- a/src/lib/actions/sandbox/sessions/download-verify.ts +++ b/src/lib/actions/sandbox/sessions/download-verify.ts @@ -13,9 +13,10 @@ export interface VerifyDownloadedFileOptions { /** Sandbox name used in error messages. */ sandboxName: string; /** - * Require the artifact to be non-empty. Set for bundles that are never - * legitimately empty (a gzip tarball, a hermes export); leave off for - * individual session files whose size we do not want to constrain. + * Require the artifact to be non-empty. Set for a bundle that is never + * legitimately empty (a gzip tarball of at least one file); leave off for + * individual session files and for the hermes export, whose size we do not + * want to constrain (a zero-session hermes export can be legitimately empty). */ requireNonEmpty?: boolean; } @@ -31,6 +32,13 @@ export interface VerifyDownloadedFileOptions { * recorded as a valid session bundle, so re-check the outcome against the file * system before treating the download as complete. * + * `hostPath` must be a path that did not exist before the download — a fresh + * per-export staging path, published to its real destination only after this + * check passes. The check can only establish that SOMETHING exists at + * `hostPath`; run against a reused destination it would accept a stale + * artifact left by an earlier export and mask the exit-0/no-write race it + * exists to catch. + * * @throws if the download reported a non-zero status, wrote no file, wrote a * non-regular file, or (when `requireNonEmpty`) wrote an empty file. */ diff --git a/src/lib/actions/sandbox/sessions/export.test.ts b/src/lib/actions/sandbox/sessions/export.test.ts index 0911977c12..8241a5245d 100644 --- a/src/lib/actions/sandbox/sessions/export.test.ts +++ b/src/lib/actions/sandbox/sessions/export.test.ts @@ -31,6 +31,9 @@ const getSandboxMock = registry.getSandbox as unknown as ReturnType; let consoleLogSpy: ReturnType; let statSyncSpy: ReturnType; +let stagingMkdtempSpy: ReturnType; +let stagingRenameSpy: ReturnType; +let stagingRmSpy: ReturnType; beforeEach(() => { captureMock.mockReset(); @@ -46,12 +49,23 @@ beforeEach(() => { statSyncSpy = vi .spyOn(fs, "statSync") .mockReturnValue({ size: 42, isFile: () => true } as unknown as ReturnType); + // Downloads are mocked and never write, so the host-side staging pipeline + // (mkdtemp -> verify -> rename -> rm) must not touch the real filesystem. + // The deterministic mkdtemp suffix lets tests compute staging paths. + stagingMkdtempSpy = vi + .spyOn(fs, "mkdtempSync") + .mockImplementation((prefix) => `${prefix as string}stub`); + stagingRenameSpy = vi.spyOn(fs, "renameSync").mockImplementation(() => {}); + stagingRmSpy = vi.spyOn(fs, "rmSync").mockImplementation(() => {}); }); afterEach(() => { consoleErrorSpy.mockRestore(); consoleLogSpy.mockRestore(); statSyncSpy.mockRestore(); + stagingMkdtempSpy.mockRestore(); + stagingRenameSpy.mockRestore(); + stagingRmSpy.mockRestore(); }); function makeCapture(output: string, status = 0) { @@ -162,54 +176,69 @@ describe("exportSandboxSessions", () => { ); const chmodSpy = vi.spyOn(fs, "chmodSync").mockImplementation(() => {}); + try { + const result = await exportSandboxSessions({ + sandboxName: "alpha", + out: "./out.tgz", + format: "tar", + }); + + // Session JSONL can contain pasted secrets, so the downloaded bundle must + // be locked down to owner-only while it is still staged — the publish + // rename preserves the mode, so the bundle is never world-readable at the + // final path. The download lands in a fresh staging directory next to the + // destination (#7367: a stale pre-existing bundle must never satisfy the + // post-download verification), and is renamed into place after it passes. + const expectedHostDest = path.resolve(process.cwd(), "out.tgz"); + const expectedStagingPath = path.join( + path.dirname(expectedHostDest), + ".sessions-export-stub", + "out.tgz", + ); + expect(chmodSpy).toHaveBeenCalledWith(expectedStagingPath, 0o600); + expect(stagingRenameSpy).toHaveBeenCalledWith(expectedStagingPath, expectedHostDest); + expect(stagingRmSpy).toHaveBeenCalledWith(path.dirname(expectedStagingPath), { + recursive: true, + force: true, + }); + + expect(captureMock).toHaveBeenCalledTimes(1); + const captureCall = captureMock.mock.calls[0]?.[0] as string[]; + expect(captureCall).toContain("openclaw"); + expect(captureCall).toContain("sessions"); + expect(captureCall).toContain("list"); + expect(captureCall).toContain("--agent"); + expect(captureCall).toContain("main"); + + const tarCall = runMock.mock.calls[0]?.[0] as string[]; + expect(tarCall.slice(0, 7)).toEqual(["sandbox", "exec", "--name", "alpha", "--", "sh", "-c"]); + const shellCommand = tarCall[7] as string; + // Staging directory inside /sandbox keeps openshell's workspace check happy + // and the umask + chmod chain seals the staging tarball to owner-only. + expect(shellCommand).toMatch( + /^umask 077 && mkdir -p \/sandbox\/\.nemoclaw-staging && chmod 700 \/sandbox\/\.nemoclaw-staging && tar -czf \/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz/, + ); + expect(shellCommand).toMatch(/-- \.\/sid-a\.jsonl \.\/sid-b\.jsonl/); + expect(shellCommand).toMatch( + /&& chmod 600 \/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz$/, + ); + expect(shellCommand).not.toMatch(/sid-a\.trajectory\.jsonl/); - const result = await exportSandboxSessions({ - sandboxName: "alpha", - out: "./out.tgz", - format: "tar", - }); - - // Session JSONL can contain pasted secrets, so the downloaded host bundle - // must be locked down to owner-only at the resolved host path, not at a - // path that drifts with the caller's cwd. - const expectedHostDest = path.resolve(process.cwd(), "out.tgz"); - expect(chmodSpy).toHaveBeenCalledWith(expectedHostDest, 0o600); - chmodSpy.mockRestore(); - - expect(captureMock).toHaveBeenCalledTimes(1); - const captureCall = captureMock.mock.calls[0]?.[0] as string[]; - expect(captureCall).toContain("openclaw"); - expect(captureCall).toContain("sessions"); - expect(captureCall).toContain("list"); - expect(captureCall).toContain("--agent"); - expect(captureCall).toContain("main"); - - const tarCall = runMock.mock.calls[0]?.[0] as string[]; - expect(tarCall.slice(0, 7)).toEqual(["sandbox", "exec", "--name", "alpha", "--", "sh", "-c"]); - const shellCommand = tarCall[7] as string; - // Staging directory inside /sandbox keeps openshell's workspace check happy - // and the umask + chmod chain seals the staging tarball to owner-only. - expect(shellCommand).toMatch( - /^umask 077 && mkdir -p \/sandbox\/\.nemoclaw-staging && chmod 700 \/sandbox\/\.nemoclaw-staging && tar -czf \/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz/, - ); - expect(shellCommand).toMatch(/-- \.\/sid-a\.jsonl \.\/sid-b\.jsonl/); - expect(shellCommand).toMatch( - /&& chmod 600 \/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz$/, - ); - expect(shellCommand).not.toMatch(/sid-a\.trajectory\.jsonl/); - - const downloadCall = runMock.mock.calls[1]?.[0] as string[]; - expect(downloadCall.slice(0, 3)).toEqual(["sandbox", "download", "alpha"]); - expect(downloadCall[3]).toMatch( - /^\/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz$/, - ); - expect(downloadCall.at(-1)).toBe(expectedHostDest); + const downloadCall = runMock.mock.calls[1]?.[0] as string[]; + expect(downloadCall.slice(0, 3)).toEqual(["sandbox", "download", "alpha"]); + expect(downloadCall[3]).toMatch( + /^\/sandbox\/\.nemoclaw-staging\/sessions-export-main-[0-9a-f]+\.tgz$/, + ); + expect(downloadCall.at(-1)).toBe(expectedStagingPath); - expect(result.selectedKeys).toBe("all"); - expect(result.resolvedSessionIds).toEqual(["sid-a", "sid-b"]); - expect(result.resolvedFiles).toEqual(["sid-a.jsonl", "sid-b.jsonl"]); - expect(result.hostDest).toBe(expectedHostDest); - expect(result.bundleBytes).toBe(42); + expect(result.selectedKeys).toBe("all"); + expect(result.resolvedSessionIds).toEqual(["sid-a", "sid-b"]); + expect(result.resolvedFiles).toEqual(["sid-a.jsonl", "sid-b.jsonl"]); + expect(result.hostDest).toBe(expectedHostDest); + expect(result.bundleBytes).toBe(42); + } finally { + chmodSpy.mockRestore(); + } }); it("writes a browsable directory of session files by default (dir format, no tar staging)", async () => { @@ -224,64 +253,78 @@ describe("exportSandboxSessions", () => { const mkdirSpy = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); const chmodSpy = vi.spyOn(fs, "chmodSync").mockImplementation(() => {}); - - const result = await exportSandboxSessions({ - sandboxName: "alpha", - out: "./sessions-alpha", - }); - - // dir is the default: no in-sandbox tar/staging, just a per-file download - // straight into the host directory. The host directory is resolved - // against process.cwd() so the chmod hardening hits the real file even - // though openshell runs with a different cwd. - const expectedDir = path.resolve(process.cwd(), "sessions-alpha"); - const expectedSidA = path.join(expectedDir, "sid-a.jsonl"); - const expectedSidB = path.join(expectedDir, "sid-b.jsonl"); - expect(mkdirSpy).toHaveBeenCalledWith(expectedDir, { recursive: true }); - const shellCalls = runMock.mock.calls.filter((c) => (c[0] as string[]).includes("sh")); - expect(shellCalls).toHaveLength(0); - const downloadCalls = runMock.mock.calls.filter((c) => (c[0] as string[])[1] === "download"); - expect(downloadCalls).toHaveLength(2); - expect(downloadCalls[0]?.[0]).toEqual([ - "sandbox", - "download", - "alpha", - "/sandbox/.openclaw/agents/main/sessions/sid-a.jsonl", - expectedSidA, - ]); - expect(downloadCalls[1]?.[0]).toEqual([ - "sandbox", - "download", - "alpha", - "/sandbox/.openclaw/agents/main/sessions/sid-b.jsonl", - expectedSidB, - ]); - // Every downloaded session file is locked to owner-only — the bug observed - // in production was inconsistent perms across files in the same export - // run, so both files in this two-session fixture must see the chmod. - expect(chmodSpy).toHaveBeenCalledWith(expectedSidA, 0o600); - expect(chmodSpy).toHaveBeenCalledWith(expectedSidB, 0o600); - - expect(result.format).toBe("dir"); - expect(result.hostDest).toBe(expectedDir); - expect(result.bundleBytes).toBeNull(); - expect(result.sessions).toEqual([ - { - key: "agent:main:main", - sessionId: "sid-a", - path: expectedSidA, - sizeBytes: 42, - }, - { - key: "agent:main:telegram:t-1", - sessionId: "sid-b", - path: expectedSidB, - sizeBytes: 42, - }, - ]); - - mkdirSpy.mockRestore(); - chmodSpy.mockRestore(); + try { + const result = await exportSandboxSessions({ + sandboxName: "alpha", + out: "./sessions-alpha", + }); + + // dir is the default: no in-sandbox tar/staging, but each file downloads + // into a fresh host-side staging directory inside the export directory and + // is renamed into place only after verification (#7367: a stale file from + // an earlier export must never satisfy the post-download check). The host + // directory is resolved against process.cwd() so the chmod hardening hits + // the real file even though openshell runs with a different cwd. + const expectedDir = path.resolve(process.cwd(), "sessions-alpha"); + const expectedSidA = path.join(expectedDir, "sid-a.jsonl"); + const expectedSidB = path.join(expectedDir, "sid-b.jsonl"); + const expectedStagingDir = path.join(expectedDir, ".sessions-export-stub"); + const expectedStagingSidA = path.join(expectedStagingDir, "sid-a.jsonl"); + const expectedStagingSidB = path.join(expectedStagingDir, "sid-b.jsonl"); + expect(mkdirSpy).toHaveBeenCalledWith(expectedDir, { recursive: true }); + const shellCalls = runMock.mock.calls.filter((c) => (c[0] as string[]).includes("sh")); + expect(shellCalls).toHaveLength(0); + const downloadCalls = runMock.mock.calls.filter((c) => (c[0] as string[])[1] === "download"); + expect(downloadCalls).toHaveLength(2); + expect(downloadCalls[0]?.[0]).toEqual([ + "sandbox", + "download", + "alpha", + "/sandbox/.openclaw/agents/main/sessions/sid-a.jsonl", + expectedStagingSidA, + ]); + expect(downloadCalls[1]?.[0]).toEqual([ + "sandbox", + "download", + "alpha", + "/sandbox/.openclaw/agents/main/sessions/sid-b.jsonl", + expectedStagingSidB, + ]); + // Every downloaded session file is locked to owner-only while still staged + // (the publish rename preserves the mode) — the bug observed in production + // was inconsistent perms across files in the same export run, so both + // files in this two-session fixture must see the chmod. + expect(chmodSpy).toHaveBeenCalledWith(expectedStagingSidA, 0o600); + expect(chmodSpy).toHaveBeenCalledWith(expectedStagingSidB, 0o600); + // ...and both are published to their final paths after verification. + expect(stagingRenameSpy).toHaveBeenCalledWith(expectedStagingSidA, expectedSidA); + expect(stagingRenameSpy).toHaveBeenCalledWith(expectedStagingSidB, expectedSidB); + expect(stagingRmSpy).toHaveBeenCalledWith(expectedStagingDir, { + recursive: true, + force: true, + }); + + expect(result.format).toBe("dir"); + expect(result.hostDest).toBe(expectedDir); + expect(result.bundleBytes).toBeNull(); + expect(result.sessions).toEqual([ + { + key: "agent:main:main", + sessionId: "sid-a", + path: expectedSidA, + sizeBytes: 42, + }, + { + key: "agent:main:telegram:t-1", + sessionId: "sid-b", + path: expectedSidB, + sizeBytes: 42, + }, + ]); + } finally { + mkdirSpy.mockRestore(); + chmodSpy.mockRestore(); + } }); it("dedupes resolved session ids when the same session is referenced by both alias and canonical key", async () => { @@ -524,10 +567,58 @@ describe("exportSandboxSessions", () => { statSyncSpy.mockImplementation(() => { throw new Error("ENOENT"); }); + try { + await expect( + exportSandboxSessions({ sandboxName: "alpha", out: "./sessions-alpha" }), + ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); + } finally { + mkdirSpy.mockRestore(); + } + }); + + // #7367 regression: the default destination is deterministic, so re-running + // an export into the same directory is the normal workflow and stale files + // from the previous run sit at the published paths. Verification must run + // against the fresh staging path — a stale destination file must not turn an + // exit-0/no-write download into a phantom success. + it("rejects a dir export whose download wrote nothing even when a stale file from an earlier export sits at the destination (#7367)", async () => { + captureMock.mockReturnValueOnce( + makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), + ); + const mkdirSpy = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); + // Stale published paths stat fine; the fresh staging path does not exist + // because the exit-0 download wrote nothing. + statSyncSpy.mockImplementation((target) => { + if (String(target).includes(".sessions-export-")) { + throw new Error("ENOENT"); + } + return { size: 42, isFile: () => true } as unknown as ReturnType; + }); + try { + await expect( + exportSandboxSessions({ sandboxName: "alpha", out: "./sessions-alpha" }), + ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); + // Nothing may be published when verification fails. + expect(stagingRenameSpy).not.toHaveBeenCalled(); + } finally { + mkdirSpy.mockRestore(); + } + }); + + it("rejects a tar export whose download wrote nothing even when a stale bundle from an earlier export sits at the destination (#7367)", async () => { + captureMock.mockReturnValueOnce( + makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), + ); + statSyncSpy.mockImplementation((target) => { + if (String(target).includes(".sessions-export-")) { + throw new Error("ENOENT"); + } + return { size: 42, isFile: () => true } as unknown as ReturnType; + }); await expect( - exportSandboxSessions({ sandboxName: "alpha", out: "./sessions-alpha" }), + exportSandboxSessions({ sandboxName: "alpha", out: "./out.tgz", format: "tar" }), ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); - mkdirSpy.mockRestore(); + expect(stagingRenameSpy).not.toHaveBeenCalled(); }); it("emits a JSON manifest with resolved session ids, files, host path, and bundle size when --json is set", async () => { diff --git a/src/lib/actions/sandbox/sessions/export.ts b/src/lib/actions/sandbox/sessions/export.ts index b8d8edddff..fdd1633946 100644 --- a/src/lib/actions/sandbox/sessions/export.ts +++ b/src/lib/actions/sandbox/sessions/export.ts @@ -141,6 +141,16 @@ export async function exportSandboxSessions( if (format === "tar") { const tarballRemote = stagingTarballPath(agent); const tarArgv = buildSandboxTarArgv({ sourceDir, tarballRemote, resolvedFiles }); + // Download into a fresh host-side staging path and publish to hostDest + // only after verification. hostDest can already hold a bundle from an + // earlier export, and a pre-existing file must never satisfy the #7367 + // exit-0/no-write check — assertDownloadedFile can only prove THIS + // download wrote something when the path it inspects started out absent. + const absoluteHostDest = path.resolve(hostDest); + const hostStagingDir = fs.mkdtempSync( + path.join(path.dirname(absoluteHostDest), ".sessions-export-"), + ); + const hostStagingPath = path.join(hostStagingDir, path.basename(absoluteHostDest)); try { // Use ignoreError so the underlying spawn helper does not call // process.exit on a non-zero tar/download status. Without that, the @@ -166,14 +176,21 @@ export async function exportSandboxSessions( } const downloadResult = runOpenshell( - ["sandbox", "download", opts.sandboxName, tarballRemote, hostDest], + ["sandbox", "download", opts.sandboxName, tarballRemote, hostStagingPath], { ignoreError: true, stdio: "inherit" }, ); - assertDownloadedFile(downloadResult, hostDest, { + assertDownloadedFile(downloadResult, hostStagingPath, { remoteLabel: tarballRemote, sandboxName: opts.sandboxName, requireNonEmpty: true, }); + // Session JSONL captures user prompts and tool I/O, which routinely + // contain pasted secrets. The staged tarball lands with the caller's + // umask, so restrict it to owner-only before it is published — the + // rename preserves the mode (the in-sandbox staging copy is already + // 0600). + hardenPermissions(hostStagingPath); + fs.renameSync(hostStagingPath, hostDest); } finally { // Best-effort cleanup of the in-sandbox staging tarball. Runs even when // tar/download fail so a partial export cannot leave a bundle of session @@ -182,12 +199,9 @@ export async function exportSandboxSessions( ["sandbox", "exec", "--name", opts.sandboxName, "--", "rm", "-f", tarballRemote], { ignoreError: true, stdio: "ignore" }, ); + removeHostStagingDir(hostStagingDir); } - // Session JSONL captures user prompts and tool I/O, which routinely contain - // pasted secrets. The host tarball lands with the caller's umask, so - // restrict it to owner-only (the in-sandbox staging copy is already 0600). - hardenPermissions(hostDest); try { bundleBytes = fs.statSync(hostDest).size; } catch { @@ -201,26 +215,36 @@ export async function exportSandboxSessions( sizeBytes: null, })); } else { - // dir format default: copy each resolved session file straight onto the - // host into a browsable directory. No in-sandbox staging tarball, so - // there is no staging window to clean up. + // dir format default: copy each resolved session file onto the host into + // a browsable directory. No in-sandbox staging tarball, but + // `mkdirSync(recursive)` preserves whatever an earlier export already + // left in hostDest, and a stale file must never satisfy the #7367 + // exit-0/no-write check — so each file is downloaded into a fresh + // host-side staging directory and published only after verification. try { fs.mkdirSync(hostDest, { recursive: true }); } catch (err) { throw new Error(`Failed to create export directory '${hostDest}': ${(err as Error).message}`); } - for (const file of resolvedFiles) { - const localPath = path.join(hostDest, file); - const downloadResult = runOpenshell( - ["sandbox", "download", opts.sandboxName, `${sourceDir}/${file}`, localPath], - { ignoreError: true, stdio: "inherit" }, - ); - assertDownloadedFile(downloadResult, localPath, { - remoteLabel: file, - sandboxName: opts.sandboxName, - }); - // Session JSONL can contain pasted secrets — restrict each file to owner-only. - hardenPermissions(localPath); + const hostStagingDir = fs.mkdtempSync(path.join(hostDest, ".sessions-export-")); + try { + for (const file of resolvedFiles) { + const stagingPath = path.join(hostStagingDir, file); + const downloadResult = runOpenshell( + ["sandbox", "download", opts.sandboxName, `${sourceDir}/${file}`, stagingPath], + { ignoreError: true, stdio: "inherit" }, + ); + assertDownloadedFile(downloadResult, stagingPath, { + remoteLabel: file, + sandboxName: opts.sandboxName, + }); + // Session JSONL can contain pasted secrets — restrict each file to + // owner-only before it is published (the rename preserves the mode). + hardenPermissions(stagingPath); + fs.renameSync(stagingPath, path.join(hostDest, file)); + } + } finally { + removeHostStagingDir(hostStagingDir); } exported = sessions.map((entry) => { const localPath = path.join(hostDest, `${entry.sessionId}.jsonl`); @@ -356,13 +380,7 @@ async function exportHermesSessions(opts: SessionsExportOptions): Promise { expect(tarLine).not.toContain("trajectory.jsonl"); expect(tarLine).toContain("chmod 600"); expect(downloadLine).toContain("alpha"); - expect(downloadLine).toContain(out); + // #7367: the download lands in a fresh staging dir next to the + // destination and is renamed into place only after verification, so the + // download target is a staging path — but the bundle ends up at `out`. + expect(downloadLine).toContain(".sessions-export-"); + expect(downloadLine).toContain("bundle.tgz"); + expect(fs.existsSync(out)).toBe(true); expect(cleanupLine).toBeDefined(); expect(cleanupLine).toContain("/sandbox/.nemoclaw-staging/sessions-export-main-"); @@ -166,7 +171,13 @@ describe("sandbox sessions export CLI", () => { const downloadLines = calls.filter((line) => line.startsWith("sandbox download")); expect(downloadLines).toHaveLength(2); expect(downloadLines[0]).toContain("/sandbox/.openclaw/agents/main/sessions/sid-a.jsonl"); - expect(downloadLines[0]).toContain(path.join(outDir, "sid-a.jsonl")); + // #7367: each file downloads into a fresh staging dir under outDir and is + // renamed to its final path only after verification, so the download + // target is a staging path — the published file lands at outDir/. + expect(downloadLines[0]).toContain(path.join(outDir, ".sessions-export-")); + expect(downloadLines[0]).toContain("sid-a.jsonl"); + expect(fs.existsSync(path.join(outDir, "sid-a.jsonl"))).toBe(true); + expect(fs.existsSync(path.join(outDir, "sid-b.jsonl"))).toBe(true); const manifest = JSON.parse(result.out.trim().split("\n").at(-1) as string); expect(manifest).toMatchObject({ From 8507e7cab72ba6a24c57365d21635ab8c3fe77eb Mon Sep 17 00:00:00 2001 From: Dongni-Yang Date: Thu, 23 Jul 2026 10:05:59 +0800 Subject: [PATCH 3/3] test(sandbox): keep #7367 export tests typed and branchless Fix the build-typecheck and codebase-growth-guardrails failures on the prior commit: type the statSync mock parameter (TS7006) and replace the if-based conditional mocks with a ternary + throwEnoent helper so the changed test files add no if statements. Also assert staging-path targets for both directory downloads (CodeRabbit), not just the first. Refs #7367 Signed-off-by: Dongni-Yang Co-Authored-By: Claude Fable 5 --- .../actions/sandbox/sessions/export.test.ts | 28 +++++++++++-------- test/sandbox-sessions-export-cli.test.ts | 8 ++++-- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/lib/actions/sandbox/sessions/export.test.ts b/src/lib/actions/sandbox/sessions/export.test.ts index 8241a5245d..dccb70f442 100644 --- a/src/lib/actions/sandbox/sessions/export.test.ts +++ b/src/lib/actions/sandbox/sessions/export.test.ts @@ -76,6 +76,12 @@ function makeRun(status: number) { return { status, stdout: "", stderr: "" }; } +// A statSync stand-in for "this path does not exist". Kept branchless so the +// mocks that use it stay linear (the changed-test if-statement guardrail). +function throwEnoent(): never { + throw new Error("ENOENT"); +} + describe("buildSandboxTarArgv", () => { it("isolates resolved files behind `--` and prefixes each with './' so a leading-dash filename cannot be reinterpreted as a tar option", () => { expect( @@ -588,12 +594,11 @@ describe("exportSandboxSessions", () => { const mkdirSpy = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); // Stale published paths stat fine; the fresh staging path does not exist // because the exit-0 download wrote nothing. - statSyncSpy.mockImplementation((target) => { - if (String(target).includes(".sessions-export-")) { - throw new Error("ENOENT"); - } - return { size: 42, isFile: () => true } as unknown as ReturnType; - }); + statSyncSpy.mockImplementation((target: fs.PathLike) => + String(target).includes(".sessions-export-") + ? throwEnoent() + : ({ size: 42, isFile: () => true } as unknown as ReturnType), + ); try { await expect( exportSandboxSessions({ sandboxName: "alpha", out: "./sessions-alpha" }), @@ -609,12 +614,11 @@ describe("exportSandboxSessions", () => { captureMock.mockReturnValueOnce( makeCapture(JSON.stringify([{ key: "agent:main:main", sessionId: "sid-a" }])), ); - statSyncSpy.mockImplementation((target) => { - if (String(target).includes(".sessions-export-")) { - throw new Error("ENOENT"); - } - return { size: 42, isFile: () => true } as unknown as ReturnType; - }); + statSyncSpy.mockImplementation((target: fs.PathLike) => + String(target).includes(".sessions-export-") + ? throwEnoent() + : ({ size: 42, isFile: () => true } as unknown as ReturnType), + ); await expect( exportSandboxSessions({ sandboxName: "alpha", out: "./out.tgz", format: "tar" }), ).rejects.toThrow(/reported success \(exit 0\) but no file was written/); diff --git a/test/sandbox-sessions-export-cli.test.ts b/test/sandbox-sessions-export-cli.test.ts index cc5305b891..f6f1edf47e 100644 --- a/test/sandbox-sessions-export-cli.test.ts +++ b/test/sandbox-sessions-export-cli.test.ts @@ -172,10 +172,14 @@ describe("sandbox sessions export CLI", () => { expect(downloadLines).toHaveLength(2); expect(downloadLines[0]).toContain("/sandbox/.openclaw/agents/main/sessions/sid-a.jsonl"); // #7367: each file downloads into a fresh staging dir under outDir and is - // renamed to its final path only after verification, so the download - // target is a staging path — the published file lands at outDir/. + // renamed to its final path only after verification, so every download + // targets a staging path — the published files land at outDir/. + // Assert staging for BOTH downloads so a regression on only the second + // file cannot pass (the stub still creates the final file either way). expect(downloadLines[0]).toContain(path.join(outDir, ".sessions-export-")); expect(downloadLines[0]).toContain("sid-a.jsonl"); + expect(downloadLines[1]).toContain(path.join(outDir, ".sessions-export-")); + expect(downloadLines[1]).toContain("sid-b.jsonl"); expect(fs.existsSync(path.join(outDir, "sid-a.jsonl"))).toBe(true); expect(fs.existsSync(path.join(outDir, "sid-b.jsonl"))).toBe(true);