fix(subprocess-env): strip empty segments when building NO_PROXY#4086
Merged
Conversation
withLocalNoProxy splits an existing NO_PROXY value on commas to check which local hostnames still need to be appended. If the value has trailing or doubled commas (e.g. "corp.internal,,other.com,"), the split produces empty-string entries that pass through into the joined result, producing malformed output like "corp.internal,,other.com,,localhost,...". Switching from the conditional split to split+trim+filter(Boolean) removes the empty entries before the loop runs, so the final joined value is always well-formed regardless of what the incoming NO_PROXY looks like. Both the CLI file (src/lib/subprocess-env.ts) and its plugin mirror (nemoclaw/src/lib/subprocess-env.ts) carry the same function — fix both to keep them in sync as the header comments require. Tests added: - https_proxy (lowercase) as proxy trigger — previously untested - doubled/trailing commas in NO_PROXY produce no empty segments Signed-off-by: Atulya Singh <atulyarajsingh@gmail.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 112 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates ChangesNO_PROXY String Normalization
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🤖 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/subprocess-env.test.ts`:
- Around line 44-56: The test only asserts normalization for env.NO_PROXY;
update the same test ("strips empty segments from a NO_PROXY with doubled or
trailing commas") to also assert env.no_proxy is normalized the same way—after
calling withLocalNoProxy(env) add checks that env.no_proxy does not contain
",,", does not start or end with a comma (/^,|,$/), and contains
"corp.internal", "other.com", and "localhost", mirroring the existing
expectations for env.NO_PROXY and using the same symbols (withLocalNoProxy,
env.NO_PROXY, env.no_proxy).
🪄 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: 706d8982-7f1f-4e29-8ffc-4ec838332279
📒 Files selected for processing (3)
nemoclaw/src/lib/subprocess-env.tssrc/lib/subprocess-env.test.tssrc/lib/subprocess-env.ts
Comment on lines
+44
to
+56
| it("strips empty segments from a NO_PROXY with doubled or trailing commas", () => { | ||
| const env: Record<string, string> = { | ||
| HTTP_PROXY: "http://proxy:8888", | ||
| NO_PROXY: "corp.internal,,other.com,", | ||
| no_proxy: "corp.internal,,other.com,", | ||
| }; | ||
| withLocalNoProxy(env); | ||
| expect(env.NO_PROXY).not.toContain(",,"); | ||
| expect(env.NO_PROXY).not.toMatch(/^,|,$/); | ||
| expect(env.NO_PROXY).toContain("corp.internal"); | ||
| expect(env.NO_PROXY).toContain("other.com"); | ||
| expect(env.NO_PROXY).toContain("localhost"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Verify no_proxy normalization for consistency.
The test sets both NO_PROXY and no_proxy with malformed input but only verifies NO_PROXY is normalized. For consistency with other tests in this file (lines 58-67, 80-89), also verify no_proxy is normalized correctly.
✅ Add assertions for no_proxy
expect(env.NO_PROXY).not.toContain(",,");
expect(env.NO_PROXY).not.toMatch(/^,|,$/);
expect(env.NO_PROXY).toContain("corp.internal");
expect(env.NO_PROXY).toContain("other.com");
expect(env.NO_PROXY).toContain("localhost");
+ expect(env.no_proxy).not.toContain(",,");
+ expect(env.no_proxy).not.toMatch(/^,|,$/);
+ expect(env.no_proxy).toContain("corp.internal");
+ expect(env.no_proxy).toContain("other.com");
+ expect(env.no_proxy).toContain("localhost");
});📝 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("strips empty segments from a NO_PROXY with doubled or trailing commas", () => { | |
| const env: Record<string, string> = { | |
| HTTP_PROXY: "http://proxy:8888", | |
| NO_PROXY: "corp.internal,,other.com,", | |
| no_proxy: "corp.internal,,other.com,", | |
| }; | |
| withLocalNoProxy(env); | |
| expect(env.NO_PROXY).not.toContain(",,"); | |
| expect(env.NO_PROXY).not.toMatch(/^,|,$/); | |
| expect(env.NO_PROXY).toContain("corp.internal"); | |
| expect(env.NO_PROXY).toContain("other.com"); | |
| expect(env.NO_PROXY).toContain("localhost"); | |
| }); | |
| it("strips empty segments from a NO_PROXY with doubled or trailing commas", () => { | |
| const env: Record<string, string> = { | |
| HTTP_PROXY: "http://proxy:8888", | |
| NO_PROXY: "corp.internal,,other.com,", | |
| no_proxy: "corp.internal,,other.com,", | |
| }; | |
| withLocalNoProxy(env); | |
| expect(env.NO_PROXY).not.toContain(",,"); | |
| expect(env.NO_PROXY).not.toMatch(/^,|,$/); | |
| expect(env.NO_PROXY).toContain("corp.internal"); | |
| expect(env.NO_PROXY).toContain("other.com"); | |
| expect(env.NO_PROXY).toContain("localhost"); | |
| expect(env.no_proxy).not.toContain(",,"); | |
| expect(env.no_proxy).not.toMatch(/^,|,$/); | |
| expect(env.no_proxy).toContain("corp.internal"); | |
| expect(env.no_proxy).toContain("other.com"); | |
| expect(env.no_proxy).toContain("localhost"); | |
| }); |
🤖 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/subprocess-env.test.ts` around lines 44 - 56, The test only asserts
normalization for env.NO_PROXY; update the same test ("strips empty segments
from a NO_PROXY with doubled or trailing commas") to also assert env.no_proxy is
normalized the same way—after calling withLocalNoProxy(env) add checks that
env.no_proxy does not contain ",,", does not start or end with a comma
(/^,|,$/), and contains "corp.internal", "other.com", and "localhost", mirroring
the existing expectations for env.NO_PROXY and using the same symbols
(withLocalNoProxy, env.NO_PROXY, env.no_proxy).
Contributor
|
✨ Thanks for submitting this detailed PR about fixing the subprocess-env issue with empty segments in the NO_PROXY value. This proposes a way to strip empty segments when building NO_PROXY by adding a filter to remove them after the comma split and trim. |
cv
approved these changes
Jun 5, 2026
miyoungc
added a commit
that referenced
this pull request
Jun 6, 2026
## Summary - Adds the `v0.0.60` section to `docs/about/release-notes.mdx` using the dev announcement from discussion #4877. - Fills the source-doc gaps found during release-prep review across inference, policy tiers, command behavior, security boundaries, Hermes dashboard/tooling, runtime context, and troubleshooting. - Refreshes generated agent skills under `.agents/skills/` from the current Fern docs output and upgrades Fern from `5.44.3` to `5.45.0`. ## Source summary - #4037 -> `docs/reference/architecture.mdx`, `docs/about/how-it-works.mdx`, `docs/about/release-notes.mdx`: Documents system-only runtime context that stays out of visible chat. - #4875 -> `docs/reference/architecture.mdx`, `docs/about/how-it-works.mdx`, `docs/about/release-notes.mdx`: Documents try-first sandbox network/filesystem guidance and clearer failure classification. - #4788 -> `docs/security/best-practices.mdx`, `docs/about/release-notes.mdx`: Documents shared OpenClaw device-approval policy for startup and connect. - #4768 -> `docs/reference/network-policies.mdx`, `docs/network-policy/integration-policy-examples.mdx`, `docs/get-started/quickstart.mdx`, `docs/get-started/quickstart-hermes.mdx`, `docs/reference/commands.mdx`: Documents `weather`, `public-reference`, and Hermes managed-tool gateway preset behavior. - #3788 and #4864 -> `docs/reference/network-policies.mdx`, `docs/reference/commands.mdx`: Documents non-interactive policy-tier fail-fast behavior and interactive prompt fallback. - #4756 and #4866 -> `docs/reference/commands.mdx`: Documents env-aware default sandbox resolution for `list`, `status`, and `tunnel` commands. - #4320 -> `docs/reference/commands.mdx`: Documents `$$nemoclaw tunnel status` behavior. - #4328 -> `docs/reference/commands.mdx`: Documents line-scoped policy preset descriptions in `policy-list`. - #4580 and #4748 -> `docs/reference/architecture.mdx`: Documents package-managed OpenShell gateway service and Docker-driver gateway-marker behavior. - #4598 -> `docs/manage-sandboxes/lifecycle.mdx`: Documents concurrent gateway/dashboard cleanup isolation by sandbox name and port. - #4777 -> `docs/reference/troubleshooting.mdx`: Documents Docker GPU patch rollback behavior. - #4610 -> `docs/reference/troubleshooting.mdx`, `docs/reference/commands.mdx`: Keeps mutable OpenClaw config permission guidance aligned and removes skipped experimental wording. - #4868 -> `docs/reference/commands.mdx`: Keeps `.dockerignore` handling for custom `onboard --from <Dockerfile>` contexts in generated skills. - #4870 -> `docs/reference/commands.mdx`, `docs/manage-sandboxes/runtime-controls.mdx`: Documents `NEMOCLAW_MINIMAL_BOOTSTRAP` and generated skill coverage. - #4641 -> `docs/inference/inference-options.mdx`, `docs/reference/troubleshooting.mdx`: Documents local NVIDIA NIM platform-digest pulls and served-model id adoption. - #4810 and #4867 -> `docs/inference/inference-options.mdx`: Documents stable NGC managed-vLLM image lineage and DGX Station DeepSeek V4 Flash coverage. - #4852 -> `docs/inference/use-local-inference.mdx`, `docs/reference/troubleshooting.mdx`: Documents Ollama model fit filtering, 16K context floor, cold-load retry, and failed-model exclusion. - #4847 -> `docs/inference/switch-inference-providers.mdx`: Documents API-family sync, Hermes `api_mode`, and Bedrock Runtime exception. - #4800 -> `docs/inference/tool-calling-reliability.mdx`: Documents Nemotron managed-inference native tool-search fallback. - #4333 -> `docs/inference/switch-inference-providers.mdx`: Documents interactive multimodal input prompting. - #4086 -> `docs/reference/troubleshooting.mdx`: Keeps proxy bypass normalization in generated troubleshooting coverage. - #4811 and #4855 -> `docs/get-started/quickstart-hermes.mdx`: Documents prebuilt Hermes dashboard assets and TUI recovery without runtime rebuilds. - #4854 -> `docs/inference/switch-inference-providers.mdx`, `docs/reference/commands.mdx`: Documents Hermes proxy API-key placeholder preservation during inference switches. - #4248 -> `docs/manage-sandboxes/messaging-channels.mdx`, `.agents/skills/`: Keeps messaging enrollment behavior aligned with manifest-hook implementation. - #4771 -> `docs/security/best-practices.mdx`, `docs/security/credential-storage.mdx`: Documents Hermes placeholder-only secret boundary for sandbox-visible runtime files. - #4787 -> `docs/security/best-practices.mdx`, `docs/about/release-notes.mdx`: Documents expanded memory scanner examples for OpenAI project keys and Slack app-level tokens. - #4848 -> `docs/reference/commands.mdx`: Documents OpenClaw skill install mirroring into the agent home directory. - #4790 -> `docs/about/release-notes.mdx`: Uses the prior release-prep structure and generated `.agents/skills/` refresh as the template for this release. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ skills/ --prefix nemoclaw-user --doc-platform fern-mdx --dry-run` - `npm run docs` - `git diff --check` - skip-term scan across `docs/`, `.agents/skills/`, and `skills/` - `npm run build:cli` - `npm run typecheck:cli` - Commit and pre-push hook suites, including markdownlint, gitleaks, env-var docs gate, docs-to-skills verification, and skills YAML tests <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * DeepSeek-V4-Flash now available as default inference model for DGX Station. * Hermes dashboard improved with dedicated port and OAuth-authenticated tool gateway selection. * Added weather and public-reference policy presets for expanded agent capabilities. * Enhanced Ollama model selection with GPU memory filtering and automatic retry for timeouts. * **Bug Fixes** * Improved policy tier validation to prevent invalid configurations. * Better sandbox cleanup scoping by port to prevent conflicts across deployments. * Added GPU patch failure recovery with automatic rollback. * **Documentation** * Expanded troubleshooting guides for inference, security, and sandbox lifecycle. * Added .dockerignore best practices for custom deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
12 tasks
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
withLocalNoProxysplits the existingNO_PROXYvalue on commas before appending local hostnames. If that value has trailing or doubled commas — e.g. from a misconfigured environment or a tool that concatenates proxy exclusions — the split produces empty-string elements. Those empties survive the loop unchanged and end up joined back into the result, giving malformed output likecorp.internal,,other.com,,localhost,....The fix adds
.filter(Boolean)after the split+trim so empty segments are removed before the loop runs, keeping the final value well-formed. Both the CLI copy (src/lib/subprocess-env.ts) and its plugin mirror (nemoclaw/src/lib/subprocess-env.ts) carry the same function, so both are updated.Changes
withLocalNoProxyby adding.filter(Boolean)after the comma split+trim in both fileshttps_proxy(lowercase) as a proxy trigger — the implementation checked it but there was no test covering this branchNO_PROXYvalue produce no empty segments in the outputType of Change
Verification
npx prek run --all-filespassesnpm testpassesSigned-off-by: Atulya Singh atulyarajsingh@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests