fix(cli): detect tar failure in debug --output and set non-zero exit code by kagura-agent · Pull Request #1770 · NVIDIA/NemoClaw

kagura-agent · 2026-04-11T00:39:56Z

Problem

nemoclaw debug --output /nonexistent/path/debug.tar.gz prints "Tarball written to ..." and exits with code 0 even when tar fails. Users see a success message despite no tarball being created.

Fixes #1765

Root Cause

createTarball() called spawnSync("tar", ...) without checking the return status. The success message was unconditionally printed.

Fix

Check spawnSync result .status and .signal
On failure: print a red error message and set process.exitCode = 1
On signal kill (e.g. timeout): report the signal name

Testing

Added 2 regression tests for createTarball:
- Invalid output path → process.exitCode === 1
- Valid output path → tarball created, no error
All existing tests pass (npx vitest run src/lib/debug.test.ts — 10/10)
tsc clean, eslint clean, prettier clean

Signed-off-by: kagura-agent kagura.chen28@gmail.com

Summary by CodeRabbit

Bug Fixes
- Tarball creation now detects failures, emits a clear error message, sets the process exit code, and avoids showing misleading “done” instructions on failure.
Tests
- Added tests covering tarball creation success and failure, including setup/teardown and exit-code behavior.

coderabbitai · 2026-04-11T00:40:12Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d9018a16-a2b4-4431-a78d-918c05c4a2d6

📥 Commits

Reviewing files that changed from the base of the PR and between cbb0fa2 and de1911a.

📒 Files selected for processing (2)

src/lib/debug.test.ts
src/lib/debug.ts

🚧 Files skipped from review as they are similar to previous changes (1)

src/lib/debug.test.ts

📝 Walkthrough

Walkthrough

createTarball was exported and updated to capture the tar child process result, detect failures, log a red error, set process.exitCode = 1, and return false on failure; runDebug now only prints success instructions when tar succeeds. Tests for createTarball were added/updated.

Changes

Cohort / File(s)	Summary
Tarball logic & error reporting `src/lib/debug.ts`	Added `RED` color constant and `error()` helper. Exported `createTarball(collectDir, output): boolean`. capture `spawnSync` result, detect non-zero exit or signal, log error, set `process.exitCode = 1`, and return `false`. `runDebug` now prints final instructions only when tar creation succeeds.
Tests for createTarball `src/lib/debug.test.ts`	Added `describe("createTarball")` tests. Reset `process.exitCode` in `beforeEach`, cleanup temp dir in `afterEach`. Tests assert invalid output sets `process.exitCode = 1` and returns `false`; valid output returns `true` and produces the tarball file.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI (runDebug)
    participant Debug as createTarball
    participant Tar as tar (child)
    participant Console as Console

    CLI->>Debug: createTarball(collectDir, output)
    Debug->>Tar: spawnSync tar -czf output -C collectDir .
    Tar-->>Debug: exitCode / signal
    alt exitCode == 0
        Debug-->>CLI: return true
        CLI->>Console: print "Tarball written..." and instructions
    else failure (non-zero or signal)
        Debug->>Console: error("[debug] Tarball creation failed: ...")
        Debug-->>CLI: return false (set process.exitCode = 1)
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I dug through code with carrot zest,
Found tar that failed and would not rest.
Now errors glow in crimson thread,
Exit codes hop where they should tread. 📦🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main fix: detecting tar failure and setting a non-zero exit code when the debug --output command fails.
Linked Issues check	✅ Passed	The PR fully addresses all coding requirements from issue `#1765`: checks spawnSync result status/signal, prints error on failure, sets process.exitCode=1, and includes regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the tar failure detection issue: error helper function, createTarball export and failure handling, conditional success message, and related tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

src/lib/debug.ts (1)

435-447: Return a boolean from createTarball so callers can suppress success-style footer text on failure.

Right now createTarball only mutates process.exitCode; runDebug still prints completion guidance afterward. Returning an explicit success flag makes the control flow clearer and avoids mixed messaging.

Proposed refactor

-export function createTarball(collectDir: string, output: string): void {
+export function createTarball(collectDir: string, output: string): boolean {
   const result = spawnSync("tar", ["czf", output, "-C", dirname(collectDir), basename(collectDir)], {
     stdio: "inherit",
     timeout: 60_000,
   });
   if (result.status !== 0 || result.signal) {
     const reason = result.signal
       ? `killed by signal ${result.signal}`
       : `exited with code ${result.status ?? "unknown"}`;
     error(`Failed to create tarball at ${output} (tar ${reason})`);
     process.exitCode = 1;
-    return;
+    return false;
   }
   info(`Tarball written to ${output}`);
   warn(
     "Known secrets are auto-redacted, but please review for any remaining sensitive data before sharing.",
   );
   info("Attach this file to your GitHub issue.");
+  return true;
 }

-    if (output) {
-      createTarball(collectDir, output);
-    }
+    const tarOk = output ? createTarball(collectDir, output) : true;
 
     console.log("");
-    info("Done. If filing a bug, run with --output and attach the tarball to your issue:");
-    info("  nemoclaw debug --output /tmp/nemoclaw-debug.tar.gz");
+    if (tarOk) {
+      info("Done. If filing a bug, run with --output and attach the tarball to your issue:");
+      info("  nemoclaw debug --output /tmp/nemoclaw-debug.tar.gz");
+    } else {
+      warn("Diagnostics were collected, but tarball creation failed.");
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/debug.ts` around lines 435 - 447, The createTarball function
currently only sets process.exitCode on failure; change its signature to return
a boolean (true on success, false on failure), keep the existing spawnSync logic
and error logging but return false where you set process.exitCode = 1 (and
return true at the end on success). Update callers (e.g., runDebug) to capture
the boolean result from createTarball and use it to suppress any post-run
success/footer messages when false. Ensure the function name createTarball
remains and only the return type/returns plus caller checks are modified.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/debug.ts`:
- Around line 435-447: The createTarball function currently only sets
process.exitCode on failure; change its signature to return a boolean (true on
success, false on failure), keep the existing spawnSync logic and error logging
but return false where you set process.exitCode = 1 (and return true at the end
on success). Update callers (e.g., runDebug) to capture the boolean result from
createTarball and use it to suppress any post-run success/footer messages when
false. Ensure the function name createTarball remains and only the return
type/returns plus caller checks are modified.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9a26f6c6-98b9-4013-b666-3e6e1918e76d

📥 Commits

Reviewing files that changed from the base of the PR and between b409821 and cbb0fa2.

📒 Files selected for processing (2)

src/lib/debug.test.ts
src/lib/debug.ts

…code (NVIDIA#1765) `nemoclaw debug --output <invalid-path>` would print 'Tarball written to ...' and exit 0 even when tar failed. Now checks spawnSync return status and signal, reports the error, and sets process.exitCode = 1. Changes: - Check spawnSync result.status and result.signal in createTarball - Report error with red-colored message on failure - Set process.exitCode = 1 so the command exits non-zero - Export createTarball for testability - Add regression tests for both success and failure paths Signed-off-by: kagura-agent <kagura.chen28@gmail.com>

kagura-agent · 2026-04-11T00:45:04Z

@coderabbitai Good suggestion — updated createTarball to return a boolean. runDebug now uses the return value to suppress the success footer when tarball creation fails. Force-pushed with the change.

coderabbitai · 2026-04-11T00:47:09Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check current state of createTarball and runDebug in debug.ts
rg -n "createTarball|runDebug|exitCode|return" --type=ts -A2 -B2

Length of output: 297756

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate files =="
fd -a --strip-cwd-prefix "^debug\.ts$" src || true
fd -a --strip-cwd-prefix "^debug-command\.ts$" src || true

echo "== show src/lib/debug.ts =="
sed -n '1,240p' src/lib/debug.ts || true

echo "== show src/lib/debug-command.ts =="
sed -n '1,260p' src/lib/debug-command.ts || true

echo "== confirm tests reference boolean return =="
sed -n '55,100p' src/lib/debug.test.ts || true

Length of output: 11156

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file length =="
wc -l src/lib/debug.ts || true

echo "== grep key functions =="
rg -nP '^\s*(export\s+)?function\s+(createTarball|runDebug)\b|Tarball written|footer|createTarball\(' src/lib/debug.ts -n -C3 || true

echo "== show rest of src/lib/debug.ts from line 240 =="
sed -n '240,999p' src/lib/debug.ts || true

Length of output: 10021

@kagura-agent Awesome—thanks for the quick follow-up. I pulled the force-push and verified:

createTarball now returns boolean and sets process.exitCode = 1 on failure (Line 435; failure path Lines 445–447). It logs a clear reason, and only on success prints “Tarball written…” plus the redaction reminder (Lines 448–452).
runDebug uses tarballOk to gate the success footer, so the footer is suppressed on failure (Lines 497–502; runDebug defined at Line 460).
Tests exercise the boolean + exitCode behavior (src/lib/debug.test.ts, Lines 69–85 in the shown slice).

Looks good to me.

Optional nit for later: consider mapping signal termination to a distinct exit code (128 + signal) for parity with uninstall-command, but keeping 1 is perfectly fine since you already include the signal detail in the error.

wscurran · 2026-04-13T15:18:12Z

✨ Thanks for submitting this PR, which proposes a fix for a bug where tarball creation failure is not detected and a non-zero exit code is not set.

Possibly related open issues:

#1765 [All Platforms][Nemoclaw] nemoclaw debug --output ` reports "Tarball written to ..." even when tar fails; exit code remains 0

coderabbitai bot reviewed Apr 11, 2026

View reviewed changes

kagura-agent force-pushed the fix/debug-tarball-exit-code branch from cbb0fa2 to de1911a Compare April 11, 2026 00:44

wscurran added bug Something isn't working NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). labels Apr 13, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): detect tar failure in debug --output and set non-zero exit code#1770

fix(cli): detect tar failure in debug --output and set non-zero exit code#1770
kagura-agent wants to merge 1 commit intoNVIDIA:mainfrom
kagura-agent:fix/debug-tarball-exit-code

kagura-agent commented Apr 11, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Apr 11, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

kagura-agent commented Apr 11, 2026

Uh oh!

coderabbitai bot commented Apr 11, 2026

Uh oh!

wscurran commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kagura-agent commented Apr 11, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root Cause

Fix

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

kagura-agent commented Apr 11, 2026

Uh oh!

coderabbitai bot commented Apr 11, 2026

Uh oh!

wscurran commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kagura-agent commented Apr 11, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Apr 11, 2026 •

edited

Loading