fix(ci): resolve macOS/bash 3.2 vitest failures on main#6137
Closed
prekshivyas wants to merge 8 commits into
Closed
fix(ci): resolve macOS/bash 3.2 vitest failures on main#6137prekshivyas wants to merge 8 commits into
prekshivyas wants to merge 8 commits into
Conversation
… user presets Fixes two SSRF gaps identified in issues #6072 and #6073. 1. src/lib/onboard/inference-providers/hermes.ts: call isPrivateHostname() on the user-supplied endpointUrl before passing it downstream as OPENAI_BASE_URL. Previously the URL bypassed all SSRF checks until plugin-side validateEndpointUrl() fired at inference time — too late to prevent credential dispatch to an internal host. 2. src/lib/policy/index.ts: reject user-supplied preset files that declare allowed_ips in any network_policies endpoint. The merge was previously a blind structural pass-through that let callers expand the private-IP allowlist OpenShell enforces. Fixes #6072 Fixes #6073 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codebase growth guardrail bans if statements in test files. Replace requireValue mock body with a single expression. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drop the vi.mock factory — it contained if statements which the codebase-growth guardrail forbids in test files. The real implementation reads from nemoclaw-blueprint/private-networks.yaml and works correctly in test context. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace 'in' operator with Object.hasOwn to avoid matching inherited prototype properties (prototype pollution guard, per PRA-2). Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tent bypass - hermes.ts: reject non-http/https schemes and URLs with embedded credentials; redact raw URL from parse-error messages - policy/index.ts: extract networkPoliciesHasAllowedIps helper and enforce it in applyPresetContent() when options.custom is set, closing the snapshot-replay bypass path (PRA-3 partial, PRA-4) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Five pre-existing failures on macos-vitest traced to bash 3.x
incompatibilities and a missing locale guard:
- agents/hermes/start.sh: replace bash 4.1+ named-FD exec ({var}<file)
with a { } < file grouped redirect (bash 3.2 compatible); replace
mapfile -d '' -t with while/read -r -d "" loop; guard
${_HERMES_GUARD_TIMEOUT[@]} with ${arr[@]+"${arr[@]}"} so set -u
with an empty array does not abort the script
- scripts/gateway-control.sh, scripts/lib/gateway-supervisor.sh: export
LC_ALL=C so [a-f] character-class ranges in case patterns are
byte-exact — macOS en_US.UTF-8 treats [a-f] as case-insensitive,
allowing uppercase hex nonces to bypass validation
- test/gateway-supervisor-control.test.ts: pin
NEMOCLAW_TEST_GATEWAY_CONTROL_CALLER_UID=0 so the nonce-rejection test
does not depend on the CI runner being root; suppress macOS bash 3.2
SIGTERM job-notification lines in the stderr assertion
- test/e2e/fixtures/redaction.ts: add tvly- Tavily pattern missing since
#6134, fixing e2e-redaction-parity Array(16) vs Array(17) mismatch
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughChanges include Hermes shell script bash robustness fixes (array expansion, cmdline parsing, marker reads), locale-pinned nonce validation in gateway control scripts, a new SSRF endpoint-validation guard for the Hermes inference provider, a new policy guard rejecting ChangesShell Script Reliability Fixes
SSRF Guard for Hermes Provider Endpoint
Policy allowed_ips Rejection Guard
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard/inference-providers/hermes.test.ts`:
- Around line 127-141: The malformed URL test in setupHermesProviderInference
only checks for the expected “valid URL” text and can still pass if the raw
endpointUrl leaks in the error message. Update the hermes.test.ts case for
setupHermesProviderInference to capture the thrown error message and assert both
that it matches the expected validation text and that it does not contain the
input value “not-a-url”. Keep the assertion at the public boundary by inspecting
the rejected error message rather than internals.
In `@src/lib/onboard/inference-providers/hermes.ts`:
- Around line 49-52: The Hermes endpoint validation in
ensureHermesProvider*Credentials() only checks isPrivateHostname() on
parsedEndpoint.hostname, so a public hostname that later resolves to private
space can still be accepted. Add the same DNS-pinning or isPrivateIp()
resolution check used in src/lib/sandbox/config.ts before persisting endpointUrl
as baseUrl, and keep the existing private-hostname rejection in Hermes as a
first-pass guard.
In `@test/e2e/live/onboard-negative-paths.test.ts`:
- Around line 185-187: The E2E assertion in the negative-path test is too broad
because `resultText(result)` is allowed to match a generic permission failure,
which can mask an unrelated error. Tighten the check in this test so the failure
message must include the `allowed_ips` guard wording (or the exact rejection
text from the preset validation path) while still asserting a non-zero exit
code. Update the assertion near `resultText`/`expect(...)` to verify the
specific denial path this scenario is meant to exercise.
In `@test/gateway-supervisor-control.test.ts`:
- Around line 176-178: The stderr cleanup in the test is too broad and can hide
real failures; tighten the filter used in the `result.stderr.replace(...)`
assertion so it only removes the specific macOS bash 3.2 job-control notice
shape. Update the regex in `test/gateway-supervisor-control.test.ts` to match
the leading signal-name plus signal-number form emitted by that noise, and keep
the `expect(...).toBe("")` check strict so unrelated stderr like genuine
`Killed` errors still fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b8c1ca6-0385-4954-ad91-160ce31c70ff
📒 Files selected for processing (10)
agents/hermes/start.shscripts/gateway-control.shscripts/lib/gateway-supervisor.shsrc/lib/onboard/inference-providers/hermes.test.tssrc/lib/onboard/inference-providers/hermes.tssrc/lib/policy/index.tssrc/lib/policy/preset-allowed-ips.test.tstest/e2e/fixtures/redaction.tstest/e2e/live/onboard-negative-paths.test.tstest/gateway-supervisor-control.test.ts
Comment on lines
+127
to
+141
| it("throws on malformed URL without leaking the raw value", async () => { | ||
| await expect( | ||
| setupHermesProviderInference( | ||
| { | ||
| sandboxName: "alpha", | ||
| model: "m", | ||
| provider: "p", | ||
| endpointUrl: "not-a-url", | ||
| credentialEnv: null, | ||
| hermesAuthMethod: null, | ||
| hermesToolGateways: [], | ||
| }, | ||
| makeDeps() as never, | ||
| ), | ||
| ).rejects.toThrow(/valid URL/); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the “no raw value leak” claim directly.
The test name says not-a-url must not leak, but Line 141 would still pass if the thrown message included the raw endpoint. Capture the error message and assert both the expected text and absence of the input value.
Suggested test tightening
it("throws on malformed URL without leaking the raw value", async () => {
- await expect(
- setupHermesProviderInference(
+ let thrown: unknown;
+ try {
+ await setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
@@
hermesToolGateways: [],
},
makeDeps() as never,
- ),
- ).rejects.toThrow(/valid URL/);
+ );
+ } catch (err) {
+ thrown = err;
+ }
+ expect(thrown).toBeInstanceOf(Error);
+ expect((thrown as Error).message).toMatch(/valid URL/);
+ expect((thrown as Error).message).not.toContain("not-a-url");
});As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("throws on malformed URL without leaking the raw value", async () => { | |
| await expect( | |
| setupHermesProviderInference( | |
| { | |
| sandboxName: "alpha", | |
| model: "m", | |
| provider: "p", | |
| endpointUrl: "not-a-url", | |
| credentialEnv: null, | |
| hermesAuthMethod: null, | |
| hermesToolGateways: [], | |
| }, | |
| makeDeps() as never, | |
| ), | |
| ).rejects.toThrow(/valid URL/); | |
| it("throws on malformed URL without leaking the raw value", async () => { | |
| let thrown: unknown; | |
| try { | |
| await setupHermesProviderInference( | |
| { | |
| sandboxName: "alpha", | |
| model: "m", | |
| provider: "p", | |
| endpointUrl: "not-a-url", | |
| credentialEnv: null, | |
| hermesAuthMethod: null, | |
| hermesToolGateways: [], | |
| }, | |
| makeDeps() as never, | |
| ); | |
| } catch (err) { | |
| thrown = err; | |
| } | |
| expect(thrown).toBeInstanceOf(Error); | |
| expect((thrown as Error).message).toMatch(/valid URL/); | |
| expect((thrown as Error).message).not.toContain("not-a-url"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/inference-providers/hermes.test.ts` around lines 127 - 141,
The malformed URL test in setupHermesProviderInference only checks for the
expected “valid URL” text and can still pass if the raw endpointUrl leaks in the
error message. Update the hermes.test.ts case for setupHermesProviderInference
to capture the thrown error message and assert both that it matches the expected
validation text and that it does not contain the input value “not-a-url”. Keep
the assertion at the public boundary by inspecting the rejected error message
rather than internals.
Source: Path instructions
Comment on lines
+49
to
+52
| if (isPrivateHostname(parsedEndpoint.hostname)) { | ||
| throw new Error( | ||
| `Inference endpoint URL points to a private or internal address "${parsedEndpoint.hostname}". Use a public endpoint.`, | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the downstream baseUrl consumers and private-network helpers to place
# resolved-address validation at the actual network boundary.
rg -n -C4 '\bbaseUrl\b|ensureHermesProvider(ApiKey|OAuth)Credentials|isPrivateHostname|isPrivateIp' src agentsRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## hermes.ts\n'
sed -n '1,180p' src/lib/onboard/inference-providers/hermes.ts
printf '\n## hermes-provider-auth.ts\n'
sed -n '1,260p' src/lib/hermes-provider-auth.ts
printf '\n## private-networks.ts\n'
sed -n '1,240p' src/lib/private-networks.ts
printf '\n## sandbox/config.ts relevant section\n'
sed -n '620,710p' src/lib/sandbox/config.ts
printf '\n## look for DNS resolution / endpoint validation call sites\n'
rg -n -C3 'validateEndpointUrl|lookup|resolve.*hostname|dns\.|isPrivateHostname|isPrivateIp|baseUrl: endpointUrl|endpointUrl' src/lib/onboard src/lib/sandbox src/lib/hermes-provider-auth.tsRepository: NVIDIA/NemoClaw
Length of output: 50372
Block DNS-rebinding on Hermes endpoints. isPrivateHostname() only rejects literal private/internal names here. endpointUrl is forwarded unchanged into ensureHermesProvider*Credentials(...) and persisted as baseUrl, so a public hostname that resolves to private space can still slip through. Reuse the DNS-pinning/isPrivateIp() check from src/lib/sandbox/config.ts before storing the URL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/inference-providers/hermes.ts` around lines 49 - 52, The
Hermes endpoint validation in ensureHermesProvider*Credentials() only checks
isPrivateHostname() on parsedEndpoint.hostname, so a public hostname that later
resolves to private space can still be accepted. Add the same DNS-pinning or
isPrivateIp() resolution check used in src/lib/sandbox/config.ts before
persisting endpointUrl as baseUrl, and keep the existing private-hostname
rejection in Hermes as a first-pass guard.
Comment on lines
+185
to
+187
| const text = resultText(result); | ||
| expect(result.exitCode, text).not.toBe(0); | ||
| expect(text).toMatch(/allowed_ips|not permitted/i); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the guard-specific rejection text.
Line 187 accepts any generic “not permitted” failure, so this can pass on an unrelated permission error before preset validation runs. Please require the allowed_ips wording (or the full rejection message) so the E2E proves this new denial path is what fired. As per path instructions, tests under **/*.test.{ts,js,mts,mjs,cts,cjs} should provide “behavioral confidence rather than implementation lock-in,” and test/e2e/** should preserve the real boundary under test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/live/onboard-negative-paths.test.ts` around lines 185 - 187, The E2E
assertion in the negative-path test is too broad because `resultText(result)` is
allowed to match a generic permission failure, which can mask an unrelated
error. Tighten the check in this test so the failure message must include the
`allowed_ips` guard wording (or the exact rejection text from the preset
validation path) while still asserting a non-zero exit code. Update the
assertion near `resultText`/`expect(...)` to verify the specific denial path
this scenario is meant to exercise.
Source: Path instructions
Comment on lines
+176
to
+178
| // macOS bash 3.2 reports SIGTERM job-control notifications to stderr | ||
| // (e.g. "Terminated: 15 sleep 30") despite set +m; filter them out. | ||
| expect(result.stderr.replace(/^[^\n]*(?:Terminated|Killed)[^\n]*\n?/gm, "")).toBe(""); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the stderr filter so it can't mask unrelated failures.
The current pattern strips any stderr line containing Terminated or Killed, which would also swallow a genuine error unrelated to bash job-control noise (e.g., an OOM Killed from a different failure), weakening the "stderr is empty" assertion. Anchor the filter to the specific job-control notice shape (leading signal-name + signal number) so only the macOS 3.2 noise is removed.
💚 Suggested tighter filter
- expect(result.stderr.replace(/^[^\n]*(?:Terminated|Killed)[^\n]*\n?/gm, "")).toBe("");
+ // Only strip bash job-control notices of the form "Terminated: 15 <cmd>".
+ expect(result.stderr.replace(/^(?:Terminated|Killed):\s*\d+\b[^\n]*\n?/gm, "")).toBe("");As per path instructions: "Flag ... conditionals that make a test pass without exercising its claim."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // macOS bash 3.2 reports SIGTERM job-control notifications to stderr | |
| // (e.g. "Terminated: 15 sleep 30") despite set +m; filter them out. | |
| expect(result.stderr.replace(/^[^\n]*(?:Terminated|Killed)[^\n]*\n?/gm, "")).toBe(""); | |
| // macOS bash 3.2 reports SIGTERM job-control notifications to stderr | |
| // (e.g. "Terminated: 15 sleep 30") despite set +m; filter them out. | |
| // Only strip bash job-control notices of the form "Terminated: 15 <cmd>". | |
| expect(result.stderr.replace(/^(?:Terminated|Killed):\s*\d+\b[^\n]*\n?/gm, "")).toBe(""); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gateway-supervisor-control.test.ts` around lines 176 - 178, The stderr
cleanup in the test is too broad and can hide real failures; tighten the filter
used in the `result.stderr.replace(...)` assertion so it only removes the
specific macOS bash 3.2 job-control notice shape. Update the regex in
`test/gateway-supervisor-control.test.ts` to match the leading signal-name plus
signal-number form emitted by that noise, and keep the `expect(...).toBe("")`
check strict so unrelated stderr like genuine `Killed` errors still fail.
Source: Path instructions
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 68%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
apurvvkumaria
approved these changes
Jul 1, 2026
8 tasks
Contributor
Author
apurvvkumaria
pushed a commit
that referenced
this pull request
Jul 1, 2026
## Summary Five pre-existing test failures on the `macos-vitest` CI workflow (confirmed present before #6060) traced to bash 3.x incompatibilities, a macOS UTF-8 locale issue, and a missing fixture sync. This PR fixes all five root causes. Supersedes #6137 (that PR's branch was accidentally cut from a security feature branch, pulling in unrelated files into the diff; this is a clean rebase onto main with the same two commits plus a CodeRabbit fix). ## Related Issue Investigation of CI run [28539883104](https://github.com/NVIDIA/NemoClaw/actions/runs/28539883104); failures also present in run [28534214317](https://github.com/NVIDIA/NemoClaw/actions/runs/28534214317) (before #6060), confirming #6060 is not the cause. ## Changes - **`agents/hermes/start.sh`** — three bash 3.2 compatibility fixes: - Replace bash 4.1+ named-FD `exec {var}<file` with a `{ } < file` grouped redirect; variables assigned inside `{}` remain in function scope - Replace `mapfile -d '' -t` (bash 4.x only) with `while IFS= read -r -d "" elem; do arr+=("$elem"); done` - Guard `${_HERMES_GUARD_TIMEOUT[@]}` with `${arr[@]+"${arr[@]}"}` so `set -u` does not abort the script when the array is empty (bash 3.2 treats empty `[@]` as unbound) - **`scripts/gateway-control.sh`** — add `export LC_ALL=C` so `[a-f]` character-class ranges in `case` patterns are byte-exact; macOS `en_US.UTF-8` makes `[a-f]` case-insensitive, allowing uppercase hex nonces to pass the `*[!0-9a-f]*` check - **`scripts/lib/gateway-supervisor.sh`** — same `LC_ALL=C` fix for the sourced library's nonce validation path - **`test/gateway-supervisor-control.test.ts`** — pin `NEMOCLAW_TEST_GATEWAY_CONTROL_CALLER_UID=0` in the nonce-rejection test so it does not depend on the CI runner's UID; tighten macOS bash 3.2 SIGTERM filter from broad word-match to exact `Terminated: <digits>` / `Killed: <digits>` format so unrelated stderr still fails the assertion - **`test/e2e/fixtures/redaction.ts`** — add `tvly-` Tavily token pattern missing since #6134, fixing the `e2e-redaction-parity` `Array(16)` vs `Array(17)` mismatch ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — justification: shell compatibility and test fixes, no user-facing behavior change - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) — `gateway-control.sh` and `gateway-supervisor.sh` handle nonce validation - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the `LC_ALL=C` fix tightens nonce validation (rejects uppercase hex on macOS that was previously accepted); `gateway_control_stop_tracked_pid` behavior is unchanged ## Verification - [x] Git hooks passed during commit and push - [x] Targeted tests pass for changed behavior - `test/gateway-supervisor-control.test.ts`: 22/22 ✓ - `test/hermes-managed-exit-authorization.test.ts`: all ✓ (was 8 failures before) - `test/e2e/support/e2e-redaction-parity.test.ts`: 3/3 ✓ - `test/hermes-gateway-supervisor-recovery.test.ts`: 41/42 (1 local flake — PID 4242 alive on dev machine; unrelated to these changes, passes on CI fresh runners) - [x] No secrets, API keys, or credentials committed **Remaining failures not addressed in this PR** (different root class, need separate investigation): - `install-preflight.test.ts` — environment-specific - `deepagents-code-tui-startup-check.test.ts` — needs investigation - `platform-parity-cloud-experimental.test.ts` — needs investigation - WSL `runtime-recovery-preload.test.ts`, `rebuild-config-hash.test.ts` — different class of failure --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway nonce validation to reliably accept only lowercase hex characters, independent of locale and macOS environments. * Fixed managed gateway startup, recovery, and live-status monitoring to be compatible with older Bash versions. * Tightened controller/marker parsing to reduce incorrect authorization or status detection. * Expanded secret redaction to cover additional Tavily-shaped tokens, improving protection in logs and text output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Five pre-existing test failures on the `macos-vitest` CI workflow (confirmed present before NVIDIA#6060) traced to bash 3.x incompatibilities, a macOS UTF-8 locale issue, and a missing fixture sync. This PR fixes all five root causes. Supersedes NVIDIA#6137 (that PR's branch was accidentally cut from a security feature branch, pulling in unrelated files into the diff; this is a clean rebase onto main with the same two commits plus a CodeRabbit fix). ## Related Issue Investigation of CI run [28539883104](https://github.com/NVIDIA/NemoClaw/actions/runs/28539883104); failures also present in run [28534214317](https://github.com/NVIDIA/NemoClaw/actions/runs/28534214317) (before NVIDIA#6060), confirming NVIDIA#6060 is not the cause. ## Changes - **`agents/hermes/start.sh`** — three bash 3.2 compatibility fixes: - Replace bash 4.1+ named-FD `exec {var}<file` with a `{ } < file` grouped redirect; variables assigned inside `{}` remain in function scope - Replace `mapfile -d '' -t` (bash 4.x only) with `while IFS= read -r -d "" elem; do arr+=("$elem"); done` - Guard `${_HERMES_GUARD_TIMEOUT[@]}` with `${arr[@]+"${arr[@]}"}` so `set -u` does not abort the script when the array is empty (bash 3.2 treats empty `[@]` as unbound) - **`scripts/gateway-control.sh`** — add `export LC_ALL=C` so `[a-f]` character-class ranges in `case` patterns are byte-exact; macOS `en_US.UTF-8` makes `[a-f]` case-insensitive, allowing uppercase hex nonces to pass the `*[!0-9a-f]*` check - **`scripts/lib/gateway-supervisor.sh`** — same `LC_ALL=C` fix for the sourced library's nonce validation path - **`test/gateway-supervisor-control.test.ts`** — pin `NEMOCLAW_TEST_GATEWAY_CONTROL_CALLER_UID=0` in the nonce-rejection test so it does not depend on the CI runner's UID; tighten macOS bash 3.2 SIGTERM filter from broad word-match to exact `Terminated: <digits>` / `Killed: <digits>` format so unrelated stderr still fails the assertion - **`test/e2e/fixtures/redaction.ts`** — add `tvly-` Tavily token pattern missing since NVIDIA#6134, fixing the `e2e-redaction-parity` `Array(16)` vs `Array(17)` mismatch ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — justification: shell compatibility and test fixes, no user-facing behavior change - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) — `gateway-control.sh` and `gateway-supervisor.sh` handle nonce validation - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the `LC_ALL=C` fix tightens nonce validation (rejects uppercase hex on macOS that was previously accepted); `gateway_control_stop_tracked_pid` behavior is unchanged ## Verification - [x] Git hooks passed during commit and push - [x] Targeted tests pass for changed behavior - `test/gateway-supervisor-control.test.ts`: 22/22 ✓ - `test/hermes-managed-exit-authorization.test.ts`: all ✓ (was 8 failures before) - `test/e2e/support/e2e-redaction-parity.test.ts`: 3/3 ✓ - `test/hermes-gateway-supervisor-recovery.test.ts`: 41/42 (1 local flake — PID 4242 alive on dev machine; unrelated to these changes, passes on CI fresh runners) - [x] No secrets, API keys, or credentials committed **Remaining failures not addressed in this PR** (different root class, need separate investigation): - `install-preflight.test.ts` — environment-specific - `deepagents-code-tui-startup-check.test.ts` — needs investigation - `platform-parity-cloud-experimental.test.ts` — needs investigation - WSL `runtime-recovery-preload.test.ts`, `rebuild-config-hash.test.ts` — different class of failure --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway nonce validation to reliably accept only lowercase hex characters, independent of locale and macOS environments. * Fixed managed gateway startup, recovery, and live-status monitoring to be compatible with older Bash versions. * Tightened controller/marker parsing to reduce incorrect authorization or status detection. * Expanded secret redaction to cover additional Tavily-shaped tokens, improving protection in logs and text output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.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
3 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
Five pre-existing test failures on the
macos-vitestCI workflow (confirmed present before #6060) traced to bash 3.x incompatibilities, a macOS UTF-8 locale issue, and a missing fixture sync. This PR fixes all five root causes.Related Issue
Investigation of CI run 28539883104; failures also present in run 28534214317 (before #6060), confirming #6060 is not the cause.
Changes
agents/hermes/start.sh— three bash 3.2 compatibility fixes:exec {var}<filewith a{ } < filegrouped redirect; variables assigned inside{}remain in function scopemapfile -d '' -t(bash 4.x only) withwhile IFS= read -r -d "" elem; do arr+=("$elem"); done${_HERMES_GUARD_TIMEOUT[@]}with${arr[@]+"${arr[@]}"}soset -udoes not abort the script when the array is empty (bash 3.2 treats empty[@]as unbound)scripts/gateway-control.sh— addexport LC_ALL=Cso[a-f]character-class ranges incasepatterns are byte-exact; macOSen_US.UTF-8makes[a-f]case-insensitive, allowing uppercase hex nonces to pass the*[!0-9a-f]*checkscripts/lib/gateway-supervisor.sh— sameLC_ALL=Cfix for the sourced library's nonce validation pathtest/gateway-supervisor-control.test.ts— pinNEMOCLAW_TEST_GATEWAY_CONTROL_CALLER_UID=0in the nonce-rejection test so it does not depend on the CI runner's UID; filter macOS bash 3.2 SIGTERM job-notification lines from the stderr assertiontest/e2e/fixtures/redaction.ts— addtvly-Tavily token pattern missing since fix(policy): restore Tavily egress for managed Python #6134, fixing thee2e-redaction-parityArray(16)vsArray(17)mismatchType of Change
Quality Gates
gateway-control.shandgateway-supervisor.shhandle nonce validationLC_ALL=Cfix tightens nonce validation (rejects uppercase hex on macOS that was previously accepted);gateway_control_stop_tracked_pidbehavior is unchangedVerification
npx prek run --from-ref main --to-ref HEADpassestest/gateway-supervisor-control.test.ts: 22/22 ✓test/hermes-managed-exit-authorization.test.ts: all ✓ (was 8 failures before)test/e2e/support/e2e-redaction-parity.test.ts: 3/3 ✓test/hermes-gateway-supervisor-recovery.test.ts: 41/42 (1 local flake — PID 4242 alive on dev machine; unrelated to these changes, passes on CI fresh runners)Remaining failures not addressed in this PR (different root class, need separate investigation):
install-preflight.test.ts— environment-specificdeepagents-code-tui-startup-check.test.ts— needs investigationplatform-parity-cloud-experimental.test.ts— needs investigationruntime-recovery-preload.test.ts,rebuild-config-hash.test.ts— different class of failureSigned-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
allowed_ips, with clearer failures when invalid presets are used.