refactor(cli): move dns proxy setup behind internal command - #3075
Merged
Conversation
This reverts commit 4ebeae4.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds DNS proxy setup: new domain helpers, a core runSetupDnsProxy action, a hidden oclif internal command, tests, and converts the shell script into a compatibility wrapper that delegates to the Node.js CLI or falls back to the CLI's internal command. ChangesDNS Proxy Setup Infrastructure
Sequence DiagramsequenceDiagram
actor User
participant CLI as oclif CLI
participant Action as runSetupDnsProxy
participant Docker as Docker/K8s
participant Pod as Sandbox Pod
participant Kernel as Host Network
User->>CLI: internal dns setup-proxy gateway sandbox
CLI->>Action: invoke(gatewayName, sandboxName)
Action->>Docker: discover openshell cluster container
Docker-->>Action: cluster container info
Action->>Docker: fetch Kubernetes DNS endpoints
Docker-->>Action: upstream IP (or default)
Action->>Docker: list pods and select sandbox pod
Docker-->>Action: pod name
Action->>Pod: write /tmp/dns-proxy.py and readiness probe
Action->>Pod: start proxy (nohup) and clear old PID
Pod-->>Action: process started
Action->>Pod: run readiness probe (DNS query to veth gateway)
Pod-->>Action: probe result (ok/timeout)
Action->>Kernel: configure iptables rule (if available)
Kernel-->>Action: rule applied / unavailable
Action->>Pod: write resolv.conf in sandbox namespace (if possible)
Pod-->>Action: config applied
Action-->>CLI: return SetupDnsProxyResult (exitCode, verification metrics)
CLI-->>User: exit code and messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
cv
marked this pull request as draft
May 6, 2026 06:15
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
cv
added
NemoClaw CLI
refactor
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/domain/dns/setup-proxy.ts (1)
57-64: 💤 Low valueConsider documenting the substring matching behavior.
selectSandboxPodusesincludes()for substring matching, which meanssandboxName="box"would matchpod/toolbox-xyz. This appears intentional for prefix-style naming (e.g.,box[1]matchingbox[1]-abc), but a brief comment would clarify the design intent.🤖 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/domain/dns/setup-proxy.ts` around lines 57 - 64, The function selectSandboxPod currently uses substring matching via includes() which can match partial names (e.g., sandboxName "box" matching "pod/toolbox-xyz"); add a concise comment above selectSandboxPod documenting this intentional substring matching behavior and the expected naming pattern (e.g., prefix-style names like "box[1]" matching "box[1]-abc"), and note that it deliberately strips a leading "pod/" via replace so callers rely on that output format; this clarifies intent without changing logic.scripts/setup-dns-proxy.sh (1)
7-14: 💤 Low valueUsage message inconsistency with argument validation.
The usage string uses
[gateway-name]which conventionally indicates an optional argument, but line 11 requires at least 2 arguments. The oclif command also marks both arguments as required. Consider updating the usage to<gateway-name> <sandbox-name>for consistency.📝 Suggested fix
-# Usage: ./scripts/setup-dns-proxy.sh [gateway-name] <sandbox-name> +# Usage: ./scripts/setup-dns-proxy.sh <gateway-name> <sandbox-name>And similarly in line 12:
- echo "Usage: $0 [gateway-name] <sandbox-name>" + echo "Usage: $0 <gateway-name> <sandbox-name>"🤖 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/setup-dns-proxy.sh` around lines 7 - 14, The usage message is inconsistent: the echo currently shows "[gateway-name] <sandbox-name>" but the script's argument check in the same file (the if [ "$#" -lt 2 ] block) and the oclif command treat both arguments as required; update the usage string to "<gateway-name> <sandbox-name>" so it matches the validation, i.e., change the echo/usage text in the setup-dns-proxy.sh script to use angle brackets for both arguments and ensure the Usage comment at the top is likewise updated to "Usage: ./scripts/setup-dns-proxy.sh <gateway-name> <sandbox-name>" to keep all references consistent.src/lib/actions/dns.ts (1)
340-376: 💤 Low valueConsider adding a small delay after launching the proxy before probing.
The proxy is launched with
nohup(line 357) and immediately probed in the loop (lines 363-375). Since the proxy needs time to bind to port 53 and start listening, the first few probe attempts will likely fail. This works correctly but a brief initial delay could reduce unnecessary probe iterations.🤖 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/actions/dns.ts` around lines 340 - 376, After launching the DNS proxy with the kctl(...) call that runs nohup python3 /tmp/dns-proxy.py (the block using shellSingleQuote(dnsUpstream) and vethGateway), add a short sleep (e.g. sleep(500)–sleep(1000)) immediately before the probe loop that calls buildDnsReadyProbePython so the proxy has time to bind to port 53; keep the existing probe loop (dnsReady, attempt loop and probe via kctl) unchanged—just insert the brief delay after the kctl that starts the proxy to reduce needless early probe failures.
🤖 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/setup-dns-proxy.sh`:
- Around line 7-14: The usage message is inconsistent: the echo currently shows
"[gateway-name] <sandbox-name>" but the script's argument check in the same file
(the if [ "$#" -lt 2 ] block) and the oclif command treat both arguments as
required; update the usage string to "<gateway-name> <sandbox-name>" so it
matches the validation, i.e., change the echo/usage text in the
setup-dns-proxy.sh script to use angle brackets for both arguments and ensure
the Usage comment at the top is likewise updated to "Usage:
./scripts/setup-dns-proxy.sh <gateway-name> <sandbox-name>" to keep all
references consistent.
In `@src/lib/actions/dns.ts`:
- Around line 340-376: After launching the DNS proxy with the kctl(...) call
that runs nohup python3 /tmp/dns-proxy.py (the block using
shellSingleQuote(dnsUpstream) and vethGateway), add a short sleep (e.g.
sleep(500)–sleep(1000)) immediately before the probe loop that calls
buildDnsReadyProbePython so the proxy has time to bind to port 53; keep the
existing probe loop (dnsReady, attempt loop and probe via kctl) unchanged—just
insert the brief delay after the kctl that starts the proxy to reduce needless
early probe failures.
In `@src/lib/domain/dns/setup-proxy.ts`:
- Around line 57-64: The function selectSandboxPod currently uses substring
matching via includes() which can match partial names (e.g., sandboxName "box"
matching "pod/toolbox-xyz"); add a concise comment above selectSandboxPod
documenting this intentional substring matching behavior and the expected naming
pattern (e.g., prefix-style names like "box[1]" matching "box[1]-abc"), and note
that it deliberately strips a leading "pod/" via replace so callers rely on that
output format; this clarifies intent without changing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ad1fcb4-52ad-4900-b0bf-287f8b4cc40d
📒 Files selected for processing (7)
scripts/setup-dns-proxy.shsrc/commands/internal/dns/setup-proxy.tssrc/lib/actions/dns.test.tssrc/lib/actions/dns.tssrc/lib/domain/dns/setup-proxy.test.tssrc/lib/domain/dns/setup-proxy.tstest/internal-cli.test.ts
Collaborator
Author
Automated PR review summaryReviewed PR #3075: refactor(cli): move dns proxy setup behind internal command Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Wrapper and hidden command still perform real DNS proxy setup
Passing test 2: Adversarial sandbox-name input does not trigger shell injection
Passing test 3: Adversarial gateway-name quoting does not escape wrapper/internal command path
Bottom line
|
…nto refactor/internal-dns-setup-proxy
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
marked this pull request as ready for review
May 6, 2026 19:14
cv
enabled auto-merge (squash)
May 6, 2026 19:14
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
approved these changes
May 6, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Same #3074 Bash → TypeScript port pattern — 7 files / +583 / -315.
314 lines of scripts/setup-dns-proxy.sh logic moved into:
src/lib/actions/dns.ts(+339) — proxy setup orchestration, extends the same action module from #3074.src/lib/domain/dns/setup-proxy.ts(+89) — pure domain helpers (proxy payload, pod/namespace selection, gateway parsing, safety validation).src/commands/internal/dns/setup-proxy.ts(+31) — hidden oclif commandnemoclaw internal dns setup-proxy <gateway-name> <sandbox-name>.
scripts/setup-dns-proxy.sh becomes a thin 8-line wrapper that delegates — preserves the public entrypoint.
+115 new test lines (+64 action, +42 domain, +10 internal-command routing). CI: pr.yaml mostly green (lint/dco/check-hash/legacy-path-guard/changes PASS); macos-e2e/wsl-e2e/checks still in flight at review time. No failures.
12 tasks
wscurran
added
area: cli
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
Moves
scripts/setup-dns-proxy.shbehind a hidden oclif-nativenemoclaw internal dns setup-proxycommand. The shell script is now a compatibility wrapper while DNS proxy orchestration, payload construction, sandbox pod selection, and verification live in typed TypeScript.Changes
internal dns setup-proxy <gateway-name> <sandbox-name>.scripts/setup-dns-proxy.shwith a thin wrapper and update tests for wrapper/internal-command behavior.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Tests