-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(sandbox): verify session-export download wrote a file before recording a bundle (#7367) #7371
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.
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
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
ca03e01
fix(sandbox): verify session-export download wrote a file before reco…
Dongni-Yang 83c7234
Merge remote-tracking branch 'upstream/main' into dongniy/7367-downlo…
Dongni-Yang 0f01fcc
fix(sandbox): stage session-export downloads before publishing (#7367)
Dongni-Yang 8507e7c
test(sandbox): keep #7367 export tests typed and branchless
Dongni-Yang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| // 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 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; | ||
| } | ||
|
|
||
| /** | ||
| * 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. | ||
| * | ||
| * `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. | ||
| */ | ||
| 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 { | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| 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}'.`, | ||
| ); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.