Repository navigation
fix(ci): resolve macOS/bash 3.2 vitest failures on main - #6140
Merged
Merged
Conversation
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>
The job-control noise emitted by macOS bash 3.2 is specifically "Terminated: 15 <cmd>" — signal name, colon, signal number. Tighten the replace() regex from a broad any-line-containing-the-word match to /^(?:Terminated|Killed): \d+/ so unrelated stderr that happens to contain those words still fails the assertion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8 tasks done
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates Hermes shell compatibility for restart and managed-gateway checks, switches gateway nonce validation to explicit lowercase hex character sets, adds a Tavily token prefix to redaction fixtures, and adjusts supervisor control tests for macOS bash behavior. ChangesHermes shell portability
Gateway nonce validation
Redaction and supervisor test updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
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
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 4 items to resolve/justify, 2 in-scope improvements
|
apurvvkumaria
approved these changes
Jul 1, 2026
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/lib/gateway-supervisor.sh (1)
9-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winScope
LC_ALL=Cto the nonce check, not the whole sourced library.
export LC_ALL=Cat file scope mutates the caller’s shell and every child process. Since this library is sourced byscripts/nemoclaw-start.shandagents/hermes/start.sh, the locale override should be limited to the nonce validation itself, e.g. by prefixing just that check or running it in a subshell.🤖 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 `@scripts/lib/gateway-supervisor.sh` around lines 9 - 11, The locale override in gateway-supervisor.sh is too broad because export LC_ALL=C changes the caller shell and all child processes. Limit the C locale to the nonce validation logic in the nonce check path instead of setting it at file scope, using the nonce-related comparison code in this sourced library so only the byte-exact match is affected.
🤖 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.
Nitpick comments:
In `@scripts/lib/gateway-supervisor.sh`:
- Around line 9-11: The locale override in gateway-supervisor.sh is too broad
because export LC_ALL=C changes the caller shell and all child processes. Limit
the C locale to the nonce validation logic in the nonce check path instead of
setting it at file scope, using the nonce-related comparison code in this
sourced library so only the byte-exact match is affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6d78bdab-83b1-4274-9b59-51152cef1556
📒 Files selected for processing (5)
agents/hermes/start.shscripts/gateway-control.shscripts/lib/gateway-supervisor.shtest/e2e/fixtures/redaction.tstest/gateway-supervisor-control.test.ts
…ng LC_ALL Addresses PR review advisor PRA-1/PRA-2 on #6140. The sourced supervisor library set a top-level `export LC_ALL=C` so the `[!0-9a-f]` nonce glob would match byte-exact (macOS UTF-8 folds [a-f] case-insensitively) — but exporting LC_ALL from a sourced library leaks the C locale into the Hermes runtime. Replace the locale-dependent range `[!0-9a-f]` with an explicit locale-independent class `[!0123456789abcdef]` in both nonce checks (scripts/lib/gateway-supervisor.sh and scripts/gateway-control.sh) and drop the LC_ALL exports. Lowercase-only rejection is preserved (uppercase A-F are not in the class) with no global locale side-effect. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed the PR review advisor's two resolve/justify items in
Lowercase-only rejection is preserved (uppercase Also dispatched the E2E advisor's Required jobs against this branch (they don't run on PRs automatically): |
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28546233046
|
This was referenced Jul 1, 2026
prekshivyas
added a commit
that referenced
this pull request
Jul 6, 2026
…se64 Address the required PR-advisor findings on #6210: - PRA-3: the corporate-CA bundle merged into a predictable /tmp path (/tmp/nemoclaw-ca-bundle.pem). Before the rename, drop any pre-planted symlink at the target so we rename into a fresh regular file we own rather than through an attacker-controlled link. Applied to both scripts/nemoclaw-start.sh and agents/hermes/start.sh. - PRA-4: the Dockerfile decoded NEMOCLAW_CORPORATE_CA_B64 with `base64 --decode` without checking the tool exists; add a build-time `command -v base64` guard that fails the build with a clear message. - PRA-5 (justify, no change): GIT_SSL_CAINFO is already propagated to connect sessions by the pre-existing #2270 block whenever set (the merge exports it to the same bundle), so the corporate-CA propagation loop intentionally omits it to avoid a duplicate export; documented inline. corporate-ca-runtime-merge + corporate-ca-tls-e2e suites pass; shell syntax verified. SKIP=test-cli: shell/Dockerfile-only change; the full vitest hook can trip on pre-existing macOS bash 3.2 noise if this branch predates #6140. CI runs bash 5.x green. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
added a commit
that referenced
this pull request
Jul 6, 2026
…x test titles Follow-up to the #6293 SSRF hardening — two CI breakages: - The isPrivateHostname block in probeOpenAiLikeEndpoint over-blocked host.docker.internal, a trusted sandbox->host bridge already gated by the allowHostDockerInternal check at the top of the function (Windows-host Ollama validation). Exempt the already-permitted hijacked-docker-internal alias. - Three new test titles put the issue ref mid-parens ("(SSRF, #6293)") which fails test-title-style; move to a clean final "(#6293)". cli project (635 tests) + npm run checks pass locally. SKIP=test-cli: full hook may trip on pre-existing macOS bash 3.2 noise if this branch predates #6140; CI runs bash 5.x green. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.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>
wscurran
added
area: ci
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.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; 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; tighten macOS bash 3.2 SIGTERM filter from broad word-match to exactTerminated: <digits>/Killed: <digits>format so unrelated stderr still fails the 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
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)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