fix(security): redact secret patterns from CLI log and error output#672
Closed
WuKongAI-CMU wants to merge 6 commits intoNVIDIA:mainfrom
Closed
fix(security): redact secret patterns from CLI log and error output#672WuKongAI-CMU wants to merge 6 commits intoNVIDIA:mainfrom
WuKongAI-CMU wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds secret-redaction utilities ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Runner as bin/lib/runner.js
participant Child as child_process
participant Redactor as redact()
User->>Runner: invoke run / runCapture / runInteractive(command)
Runner->>Child: spawn / exec command
Child-->>Runner: stdout/stderr or throws error
alt command failed
Runner->>Redactor: redact(command) and redactError(err)
Redactor-->>Runner: redacted strings / redacted error
Runner-->>User: log and/or throw redacted error
else command succeeded
Runner-->>User: return stdout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/runner.js (1)
46-58:⚠️ Potential issue | 🟡 MinorApply
redact()torunCaptureerror paths for consistency with other run functions.
runCapturethrows unredacted exceptions (line 57), whilerun()andrunInteractive()both applyredact()to command failures. This inconsistency creates a defensive programming gap: although current callers catch errors without logging, future modifications could expose the unredacted command string containing secrets. Align with the existing pattern by wrapping the thrown error message withredact().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 46 - 58, The catch block in runCapture currently rethrows the raw error; update runCapture so that in the catch, after handling opts.ignoreError, it rethrows the error wrapped with redact() (i.e., throw redact(err)) to match the behavior of run() and runInteractive(); keep the existing ignoreError return "" behavior and ensure you reference the runCapture function and redact() wrapper when making the change.
🧹 Nitpick comments (2)
test/runner.test.js (1)
144-182: Consider adding test cases for edge patterns and potential bypasses.The test suite covers the happy paths well. Consider adding tests for:
- Case sensitivity: Does
BEARER(uppercase) orbearer(lowercase) get masked? (The regex usesgiflag, so it should)- Quoted values:
API_KEY="secret123abc"— does the quote handling work?- Multiple secrets in one string:
nvapi-key1 nvapi-key2Optional: Additional test cases
it("masks bearer tokens case-insensitively", () => { const { redact } = require(runnerPath); expect(redact("authorization: bearer someToken1234")).toContain("****"); }); it("masks multiple secrets in one string", () => { const { redact } = require(runnerPath); const input = "nvapi-firstkey12345 and nvapi-secondkey6789"; const output = redact(input); expect(output).not.toContain("firstkey"); expect(output).not.toContain("secondkey"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 144 - 182, Add unit tests to cover edge patterns and potential bypasses for the redact function: include case-insensitive bearer variations (e.g., "BEARER"/"bearer") to ensure redact (from runnerPath) handles regex flags correctly, add quoted assignment cases (e.g., API_KEY="secret123abc") to verify quotes are masked, and add a test with multiple secrets in one string (e.g., "nvapi-first nvapi-second") to confirm redact masks all occurrences; place these new it() cases alongside the existing describe("redact") block referencing redact from runnerPath.bin/lib/runner.js (1)
66-71: Add coverage for_KEY=andghp_patterns to match shell script.The JS patterns miss two variants that are covered in
scripts/debug.sh(lines 107-113):
ghp_prefix (GitHub personal access tokens)_KEY=suffix (catchesNVIDIA_API_KEY,OTHER_API_KEY, etc.)The shell script redacts both, but the JS version only covers
API_KEY,SECRET,TOKEN,PASSWORD,CREDENTIAL.Suggested update
const SECRET_PATTERNS = [ /nvapi-[A-Za-z0-9_-]{10,}/g, /nvcf-[A-Za-z0-9_-]{10,}/g, + /ghp_[A-Za-z0-9]{30,}/g, /(?<=Bearer\s)[A-Za-z0-9_.+/=-]{10,}/gi, - /(?<=(?:API_KEY|SECRET|TOKEN|PASSWORD|CREDENTIAL)[=: ]['"]?)[A-Za-z0-9_.+/=-]{10,}/gi, + /(?<=(?:_KEY|API_KEY|SECRET|TOKEN|PASSWORD|CREDENTIAL)[=: ]['"]?)[A-Za-z0-9_.+/=-]{10,}/gi, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 66 - 71, SECRET_PATTERNS in runner.js is missing coverage for GitHub PATs and env vars that end with _KEY; update the SECRET_PATTERNS array (the constant named SECRET_PATTERNS) to add a regex for the ghp_ prefix (e.g. match ghp_[A-Za-z0-9_-]{10,}) and extend the existing lookbehind group that matches API_KEY/SECRET/TOKEN/PASSWORD/CREDENTIAL to also accept variable names that end with _KEY (so include _KEY in that lookbehind), keeping the same overall length threshold and flags as the other patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/runner.js`:
- Around line 46-58: The catch block in runCapture currently rethrows the raw
error; update runCapture so that in the catch, after handling opts.ignoreError,
it rethrows the error wrapped with redact() (i.e., throw redact(err)) to match
the behavior of run() and runInteractive(); keep the existing ignoreError return
"" behavior and ensure you reference the runCapture function and redact()
wrapper when making the change.
---
Nitpick comments:
In `@bin/lib/runner.js`:
- Around line 66-71: SECRET_PATTERNS in runner.js is missing coverage for GitHub
PATs and env vars that end with _KEY; update the SECRET_PATTERNS array (the
constant named SECRET_PATTERNS) to add a regex for the ghp_ prefix (e.g. match
ghp_[A-Za-z0-9_-]{10,}) and extend the existing lookbehind group that matches
API_KEY/SECRET/TOKEN/PASSWORD/CREDENTIAL to also accept variable names that end
with _KEY (so include _KEY in that lookbehind), keeping the same overall length
threshold and flags as the other patterns.
In `@test/runner.test.js`:
- Around line 144-182: Add unit tests to cover edge patterns and potential
bypasses for the redact function: include case-insensitive bearer variations
(e.g., "BEARER"/"bearer") to ensure redact (from runnerPath) handles regex flags
correctly, add quoted assignment cases (e.g., API_KEY="secret123abc") to verify
quotes are masked, and add a test with multiple secrets in one string (e.g.,
"nvapi-first nvapi-second") to confirm redact masks all occurrences; place these
new it() cases alongside the existing describe("redact") block referencing
redact from runnerPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29d3d3d1-7a52-4a7e-a4ec-c0af1bf967dd
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
Contributor
Author
|
Pushed a follow-up in |
Contributor
|
Thanks for submitting this PR, redacting secret patterns from CLI log and error output is a great way to improve security and prevent unauthorized access. |
wscurran
added
the
priority: high
WuKongAI-CMU
force-pushed
the
security/redact-cli-secrets
branch
2 times, most recently
from
75b56d5 to
9b4e083
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/runner.js (1)
57-57: Add redaction forcmdandoutputproperties in error objects fromexecSync.The
redactErrorfunction currently redactsmessage,stdout, andstderr, but Node.jsexecSyncerror objects also containcmd(the executed command) andoutput(an array of captured stdio). If callers log full error objects, these unredacted properties may leak secrets.Suggested patch
function redactError(err) { if (!err || typeof err !== "object") return err; if (typeof err.message === "string") err.message = redact(err.message); + if (typeof err.cmd === "string") err.cmd = redact(err.cmd); if (typeof err.stdout === "string") err.stdout = redact(err.stdout); if (typeof err.stderr === "string") err.stderr = redact(err.stderr); + if (Array.isArray(err.output)) { + err.output = err.output.map((v) => (typeof v === "string" ? redact(v) : v)); + } return err; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` at line 57, The redactError function currently only strips message, stdout, and stderr but execSync errors also include cmd and output which can contain secrets; update the redactError implementation to also redact/omit the cmd property and the output array (and redact any stdout/stderr values inside it), and ensure the thrown value (throw redactError(err)) returns the sanitized object — locate the redactError function and the call site that throws its result (symbols: redactError, execSync, cmd, output, stdout, stderr) and modify redactError to remove or replace cmd and output entries with redacted placeholders before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Line 25: The log currently truncates the raw command before redacting
(console.error uses redact(cmd.slice(0, 80))), which can leak partial secrets;
update both occurrences to redact the full cmd first and then truncate the
redacted string (i.e., call redact(cmd) and then .slice(0,80) on its result) so
console.error prints redact(cmd).slice(0,80) along with the existing context
(result.status) for both the line shown and the similar line around 40.
---
Nitpick comments:
In `@bin/lib/runner.js`:
- Line 57: The redactError function currently only strips message, stdout, and
stderr but execSync errors also include cmd and output which can contain
secrets; update the redactError implementation to also redact/omit the cmd
property and the output array (and redact any stdout/stderr values inside it),
and ensure the thrown value (throw redactError(err)) returns the sanitized
object — locate the redactError function and the call site that throws its
result (symbols: redactError, execSync, cmd, output, stdout, stderr) and modify
redactError to remove or replace cmd and output entries with redacted
placeholders before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c9251bb-2e39-4f52-a757-d1f24cec9147
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/runner.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/runner.js (2)
83-93: In-place mutation may affect callers needing raw error fields.
redactErrormutates the error object directly. This is efficient and works well for logging/display purposes, but callers that catch errors fromrunCaptureand need to programmatically inspectstdout/stderr(e.g., for substring matching to determine state) will receive redacted data.Currently, code like the pattern in
bin/nemoclaw.js:134-140usesexecFileSyncdirectly and is unaffected. However, this is worth noting in the JSDoc or a comment to prevent future refactoring surprises.📝 Optional: Add a note about mutation semantics
+/** + * Redact sensitive fields in an error object. NOTE: Mutates the error in-place. + * Callers that need to programmatically inspect raw stdout/stderr should use + * execSync directly rather than runCapture. + */ function redactError(err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 83 - 93, redactError currently mutates the passed error object in-place which can surprise callers (e.g., consumers of runCapture) that rely on raw error.stdout/stderr; change redactError to return a new error-like object (shallow copy) with redacted copies of message, cmd, stdout, stderr and output instead of modifying the original err, and update callers to use the returned value; refer to the function name redactError and fields err.message, err.cmd, err.stdout, err.stderr and err.output to locate where to implement the shallow clone-and-redact behavior and add a brief JSDoc/comment noting the non-mutating contract for future maintainers.
66-72: Pattern coverage is solid; minor gap with multi-space Bearer headers.The regex patterns provide good coverage for the stated secret types. One minor observation: the Bearer token pattern
(?<=Bearer\s)matches exactly one whitespace character. Malformed input like"Bearer token"(double space) would not be caught.This is unlikely in practice since well-formed Authorization headers use a single space, but if you want to be defensive:
🛡️ Optional: Match one or more spaces after Bearer
- /(?<=Bearer\s)[A-Za-z0-9_.+/=-]{10,}/gi, + /(?<=Bearer\s+)[A-Za-z0-9_.+/=-]{10,}/gi,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 66 - 72, Update the Bearer token lookbehind in SECRET_PATTERNS to allow one or more spaces so tokens like "Bearer token" are matched; specifically, replace the pattern /(?<=Bearer\s)[A-Za-z0-9_.+/=-]{10,}/gi in the SECRET_PATTERNS array with a variant that uses \s+ (e.g., (?<=Bearer\s+)... ) and keep the same character class and flags so behavior for token characters and case/ global matching is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/runner.js`:
- Around line 83-93: redactError currently mutates the passed error object
in-place which can surprise callers (e.g., consumers of runCapture) that rely on
raw error.stdout/stderr; change redactError to return a new error-like object
(shallow copy) with redacted copies of message, cmd, stdout, stderr and output
instead of modifying the original err, and update callers to use the returned
value; refer to the function name redactError and fields err.message, err.cmd,
err.stdout, err.stderr and err.output to locate where to implement the shallow
clone-and-redact behavior and add a brief JSDoc/comment noting the non-mutating
contract for future maintainers.
- Around line 66-72: Update the Bearer token lookbehind in SECRET_PATTERNS to
allow one or more spaces so tokens like "Bearer token" are matched;
specifically, replace the pattern /(?<=Bearer\s)[A-Za-z0-9_.+/=-]{10,}/gi in the
SECRET_PATTERNS array with a variant that uses \s+ (e.g., (?<=Bearer\s+)... )
and keep the same character class and flags so behavior for token characters and
case/ global matching is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb1e9f26-e26e-4c37-a14b-605fe68bc79a
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/runner.test.js
Add a redact() helper to bin/lib/runner.js that masks known secret patterns before they reach CLI error messages. Covers: - NVIDIA API keys (nvapi-*) - NVCF keys (nvcf-*) - Bearer tokens - Generic key/token/password assignments Applied to all run() and runInteractive() error output where the failing command string is logged. Non-secret strings pass through unchanged. Fixes NVIDIA#664 Signed-off-by: peteryuqin <peter.yuqin@gmail.com>
Contributor
Author
|
Rebased this branch onto the current |
WuKongAI-CMU
force-pushed
the
security/redact-cli-secrets
branch
from
131452b to
ea87861
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 83-93: redactError currently updates err.message but leaves
err.stack containing the original unredacted message; capture the original
message at the start (e.g., const origMsg = typeof err.message === "string" ?
err.message : null), perform your redactions on
message/cmd/stdout/stderr/output, then if origMsg and typeof err.stack ===
"string" replace occurrences of origMsg in err.stack with the redacted
err.message (or run a full redact(err.stack)) so the stack trace no longer
contains sensitive text; ensure this logic is applied inside the redactError
function and preserves stack format when present.
In `@test/runner.test.js`:
- Around line 276-309: The failing test uses "stderr: secret123456" which
doesn't match the module's redaction patterns, so update the mocked execSync
error output in the test (the thrown err.output array inside the it block for
runCapture) to include a string that matches the redaction rules (for example
prepend a recognized keyword or prefix such as "PASSWORD=secret123456" or use a
recognized token prefix like "ghp_..."), so that the module's redact() logic
(and constants like SECRET_PATTERNS used by runCapture) will actually replace
the secret with "****" and the existing assertions
(expect(error.output[1]).not.toContain(...) and
expect(error.output[1]).toContain("****")) will pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f505141-e9cf-4106-a19a-b2c17e4737ba
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
8 tasks
2 tasks
Contributor
Author
|
Addressed the latest review nits in |
Contributor
Author
|
Pushed a small follow-up on top of 544c71b: the bearer-token pattern now handles repeated spacing after |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
Review follow-up: the current branch tip already includes the requested redaction hardening on |
Contributor
Author
|
Friendly ping — this PR carries the |
5 tasks
cv
added a commit
that referenced
this pull request
Apr 1, 2026
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 2, 2026
Contributor
3 tasks
cv
added a commit
that referenced
this pull request
Apr 2, 2026
…1246) Finalizes the approach from #672 on top of current `main`. Summary: - add runner-level secret redaction for command/error output - cover `run()`, `runInteractive()`, and `runCapture()` paths - include runner tests for keyed secrets, provider tokens, bearer tokens, stack traces, and multi-secret strings Tested: - npm test -- --run test/runner.test.js <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automatic secret redaction for command outputs and a new public redaction utility. * **Bug Fixes** * Non-interactive command runs now suppress stdin and pipe outputs; interactive runs preserve stdin while captured stdout/stderr are redacted before logging or rethrowing. * Error messages and thrown errors now have secrets redacted. * **Tests** * Added unit and regression tests covering many token formats and redaction in error fields and output arrays. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: KJ <kejones@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…ript modules (NVIDIA#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any NVIDIA#924 blocker PR** (NVIDIA#781, NVIDIA#782, NVIDIA#819, NVIDIA#672, NVIDIA#634, NVIDIA#890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes NVIDIA#1237. Relates to NVIDIA#924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…VIDIA#1246) Finalizes the approach from NVIDIA#672 on top of current `main`. Summary: - add runner-level secret redaction for command/error output - cover `run()`, `runInteractive()`, and `runCapture()` paths - include runner tests for keyed secrets, provider tokens, bearer tokens, stack traces, and multi-secret strings Tested: - npm test -- --run test/runner.test.js <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automatic secret redaction for command outputs and a new public redaction utility. * **Bug Fixes** * Non-interactive command runs now suppress stdin and pipe outputs; interactive runs preserve stdin while captured stdout/stderr are redacted before logging or rethrowing. * Error messages and thrown errors now have secrets redacted. * **Tests** * Added unit and regression tests covering many token formats and redaction in error fields and output arrays. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: KJ <kejones@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…ript modules (NVIDIA#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any NVIDIA#924 blocker PR** (NVIDIA#781, NVIDIA#782, NVIDIA#819, NVIDIA#672, NVIDIA#634, NVIDIA#890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes NVIDIA#1237. Relates to NVIDIA#924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…VIDIA#1246) Finalizes the approach from NVIDIA#672 on top of current `main`. Summary: - add runner-level secret redaction for command/error output - cover `run()`, `runInteractive()`, and `runCapture()` paths - include runner tests for keyed secrets, provider tokens, bearer tokens, stack traces, and multi-secret strings Tested: - npm test -- --run test/runner.test.js <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automatic secret redaction for command outputs and a new public redaction utility. * **Bug Fixes** * Non-interactive command runs now suppress stdin and pipe outputs; interactive runs preserve stdin while captured stdout/stderr are redacted before logging or rethrowing. * Error messages and thrown errors now have secrets redacted. * **Tests** * Added unit and regression tests covering many token formats and redaction in error fields and output arrays. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: KJ <kejones@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary
Adds a
redact()helper tobin/lib/runner.jsthat masks known secret patterns before they reach CLI error messages.Patterns covered:
nvapi-*)nvcf-*)Applied to all
run()andrunInteractive()error output where the failing command string is logged. Non-secret strings pass through unchanged.Test plan
test/runner.test.jscovering all pattern types + non-secret passthrough + non-string inputFixes #664
Summary by CodeRabbit
New Features
Bug Fixes
Tests