fix(e2e): adapt network policy tests to non-interactive policy-add CLI#2149
Merged
ericksoa merged 2 commits intoNVIDIA:mainfrom Apr 21, 2026
Merged
fix(e2e): adapt network policy tests to non-interactive policy-add CLI#2149ericksoa merged 2 commits intoNVIDIA:mainfrom
ericksoa merged 2 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:
📝 WalkthroughWalkthroughUpdated Changes
Sequence DiagramsequenceDiagram
participant Test as Test Script
participant CLI as "nemoclaw CLI"
participant Expect as Expect
participant Endpoint as System Endpoint
Test->>Test: preflight() - check for expect
alt expect present
Test->>CLI: run policy-add (interactive mode env)
CLI->>Expect: present interactive preset list
Expect->>Test: select preset number & confirm
CLI->>Endpoint: apply policy
Endpoint-->>CLI: response (STATUS_/ERROR_)
CLI-->>Test: output + exit code
else expect missing
Test->>CLI: run policy-add <preset> --yes or --dry-run
CLI->>Endpoint: apply or simulate policy
Endpoint-->>CLI: response
CLI-->>Test: output + exit code
end
Test->>Test: classify result (success / proxy-blocked / unexpected)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/test-network-policy.sh (1)
127-132:⚠️ Potential issue | 🟠 MajorMake the optional
expectinstall truly non-fatal.With
set -euo pipefailenabled, a failure insudo apt-get install -y -qq expectexits the script before Line 131 can evaluate the warning condition. The subshell pattern below prevents the install failure from terminating the script.Suggested fix
if ! command -v expect >/dev/null 2>&1; then log "Installing expect..." - sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1 - if ! command -v expect >/dev/null 2>&1; then + if ! (sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1); then + log "WARNING: failed to install expect — interactive tests will skip" + elif ! command -v expect >/dev/null 2>&1; then log "WARNING: expect not available — interactive tests will skip" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-network-policy.sh` around lines 127 - 132, The apt-get install of expect is currently fatal under set -euo pipefail; change the install to be non-fatal so the subsequent "if ! command -v expect" check can run. Wrap the update+install in a subshell or append a safe-fail fallback (e.g. (sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1) || true) for the existing install line that installs expect so the script continues and the later check for the command can emit the warning as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 156-158: The interactive-mode check currently causes interactive
failures to be treated like a skip; change the logic so that the script only
skips when expect is actually missing: keep the if ! command -v expect check but
return 0 (or otherwise explicitly skip) when expect is unavailable and log that
fact; when expect is present, run the expect-driven interactive test (the block
that exercises policy-add telegram --yes / the expect script) and do NOT swallow
its exit status—propagate any non-zero exit so failures fail the test. Apply the
same change to the duplicate check at the other occurrence (the block around
lines 310-316).
---
Outside diff comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 127-132: The apt-get install of expect is currently fatal under
set -euo pipefail; change the install to be non-fatal so the subsequent "if !
command -v expect" check can run. Wrap the update+install in a subshell or
append a safe-fail fallback (e.g. (sudo apt-get update -qq && sudo apt-get
install -y -qq expect >/dev/null 2>&1) || true) for the existing install line
that installs expect so the script continues and the later check for the command
can emit the warning as intended.
🪄 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: Pro Plus
Run ID: 46018c33-9e46-402a-8a7a-2386169313f8
📒 Files selected for processing (1)
test/e2e/test-network-policy.sh
TruongNguyenG
force-pushed
the
feat/e2e-network-policy
branch
from
fea48a8 to
63c2cf5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/test-network-policy.sh (2)
305-307:⚠️ Potential issue | 🟡 MinorKeep the precondition aligned with the new
4xx == reachablerule.After this change,
STATUS_403is treated as a successful reachability signal forapi.telegram.org, but the pre-check still skips only on2xx/3xx. If telegram is already reachable and returns403,TC-NET-03now misses the intended early skip.Suggested fix
- if echo "$before" | grep -qE "STATUS_[23]"; then + if echo "$before" | grep -qE "STATUS_[2-4][0-9][0-9]"; then skip "TC-NET-03" "api.telegram.org already reachable before policy-add (preset may be pre-applied)" return fiAlso applies to: 330-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-network-policy.sh` around lines 305 - 307, The precondition grep only looks for "STATUS_[23]" so it misses 4xx codes now treated as reachable; update the check to include 4 (e.g., change the pattern to "STATUS_[234]") wherever the pre-check appears (the block using the variable before and the skip "TC-NET-03" and the similar block around lines 330-335) so a prior STATUS_4 will cause the early skip as intended.
127-131:⚠️ Potential issue | 🟠 MajorDon't let optional
expectinstallation abort preflight.With
set -e, the linesudo apt-get update -qq && sudo apt-get install -y -qq expectwill exit the script ifapt-get installfails, preventing the subsequentexpectavailability check from running. Interactive tests are not actually optional in that case.Suggested fix
if ! command -v expect >/dev/null 2>&1; then log "Installing expect..." - sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1 + if ! (sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1); then + log "WARNING: failed to install expect — interactive tests will skip" + fi if ! command -v expect >/dev/null 2>&1; then log "WARNING: expect not available — interactive tests will skip" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-network-policy.sh` around lines 127 - 131, The install step for expect can abort the script due to set -e; wrap the apt-get install so failures don't exit (e.g., run sudo apt-get update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1 || true or temporarily disable errexit around that command), then keep the subsequent command -v expect check and the warning log; update the block around the log function and the install invocation so the availability check for expect (command -v expect) always runs regardless of apt-get install exit status.
♻️ Duplicate comments (1)
test/e2e/test-network-policy.sh (1)
156-158:⚠️ Potential issue | 🟠 MajorOnly fall back when
expectis unavailable.
apply_preset_interactive()returns the same status for “expect missing” and for a real interactive failure, andTC-NET-03falls back on any non-zero exit. That still lets a broken prompt flow go green as long aspolicy-add telegram --yesworks.Suggested fix
apply_preset_interactive() { local preset_name="$1" if ! command -v expect >/dev/null 2>&1; then log " expect not available — cannot test interactive mode" - return 1 + return 2 fi @@ log " Step 2: Adding telegram preset (interactive mode)..." if ! apply_preset_interactive "telegram"; then - log " Interactive mode failed — falling back to non-interactive..." - if ! apply_preset "telegram"; then - fail "TC-NET-03: Setup" "Could not apply telegram preset" - return - fi + local interactive_rc=$? + if [[ $interactive_rc -eq 2 ]]; then + skip "TC-NET-03 interactive" "expect not available" + log " Interactive mode unavailable — falling back to non-interactive..." + if ! apply_preset "telegram"; then + fail "TC-NET-03: Setup" "Could not apply telegram preset" + return + fi + else + fail "TC-NET-03: Interactive policy-add" "interactive flow failed" + return + fi fiAlso applies to: 310-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-network-policy.sh` around lines 156 - 158, The fallback currently treats any non-zero return from apply_preset_interactive() the same as "expect" being missing, hiding real interactive failures; change the logic so you first check for the presence of the expect binary (command -v expect) and only log "expect not available — cannot test interactive mode" and return 1 when that check fails; otherwise invoke apply_preset_interactive() and if it returns non-zero propagate that non-zero (fail the test) instead of falling back to the non-interactive path. Apply the same change to the other occurrence (lines around the second block, referenced in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 358-364: The test swallows the exit status of the nemoclaw call
and uses an overly-broad grep pattern which can mask failures; run the
policy-add command capturing both output and exit code (keep the dry_output
variable and also capture the command's exit status), assert the exit code is
zero before checking output, and tighten the output check to look for explicit
success indicators from the dry-run (e.g., "slack\.com" or the literal phrase
"would be opened" or another known success string) when validating TC-NET-04 for
the command `nemoclaw ... policy-add slack --dry-run`.
---
Outside diff comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 305-307: The precondition grep only looks for "STATUS_[23]" so it
misses 4xx codes now treated as reachable; update the check to include 4 (e.g.,
change the pattern to "STATUS_[234]") wherever the pre-check appears (the block
using the variable before and the skip "TC-NET-03" and the similar block around
lines 330-335) so a prior STATUS_4 will cause the early skip as intended.
- Around line 127-131: The install step for expect can abort the script due to
set -e; wrap the apt-get install so failures don't exit (e.g., run sudo apt-get
update -qq && sudo apt-get install -y -qq expect >/dev/null 2>&1 || true or
temporarily disable errexit around that command), then keep the subsequent
command -v expect check and the warning log; update the block around the log
function and the install invocation so the availability check for expect
(command -v expect) always runs regardless of apt-get install exit status.
---
Duplicate comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 156-158: The fallback currently treats any non-zero return from
apply_preset_interactive() the same as "expect" being missing, hiding real
interactive failures; change the logic so you first check for the presence of
the expect binary (command -v expect) and only log "expect not available —
cannot test interactive mode" and return 1 when that check fails; otherwise
invoke apply_preset_interactive() and if it returns non-zero propagate that
non-zero (fail the test) instead of falling back to the non-interactive path.
Apply the same change to the other occurrence (lines around the second block,
referenced in the comment).
🪄 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: Pro Plus
Run ID: 835c5a33-d484-47d9-83dd-1324e6a30155
📒 Files selected for processing (1)
test/e2e/test-network-policy.sh
TruongNguyenG
force-pushed
the
feat/e2e-network-policy
branch
from
63c2cf5 to
7477557
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/test-network-policy.sh (1)
337-343: Consider including 5xx responses as "reachable" for consistency.The pattern
STATUS_[2-4][0-9][0-9]treats 2xx–4xx as success. However,test/e2e/test-skip-permissions-policy.shuses[2-5][0-9][0-9]since any HTTP response (including 5xx) proves egress works—the request reached the server. While 5xx is unlikely forapi.telegram.org, aligning the patterns improves consistency.Optional fix for consistency
- if echo "$after" | grep -qE "STATUS_[2-4][0-9][0-9]"; then + if echo "$after" | grep -qE "STATUS_[2-5][0-9][0-9]"; thenApply the same change to line 307 if desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-network-policy.sh` around lines 337 - 343, Update the grep pattern that checks the variable after to treat 5xx as reachable by changing the regex STATUS_[2-4][0-9][0-9] to STATUS_[2-5][0-9][0-9] in the conditional that inspects "$after" (the block that calls pass "TC-NET-03: Endpoint reachable after live policy-add"); also apply the same regex change to the other similar check earlier in the script that uses the same STATUS_[2-4][0-9][0-9] pattern so both places treat 2xx–5xx as evidence the request reached the server.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 337-343: Update the grep pattern that checks the variable after to
treat 5xx as reachable by changing the regex STATUS_[2-4][0-9][0-9] to
STATUS_[2-5][0-9][0-9] in the conditional that inspects "$after" (the block that
calls pass "TC-NET-03: Endpoint reachable after live policy-add"); also apply
the same regex change to the other similar check earlier in the script that uses
the same STATUS_[2-4][0-9][0-9] pattern so both places treat 2xx–5xx as evidence
the request reached the server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 685e9b17-ccf7-485e-9e7c-a772b73535a2
📒 Files selected for processing (1)
test/e2e/test-network-policy.sh
TruongNguyenG
force-pushed
the
feat/e2e-network-policy
branch
2 times, most recently
from
53625c7 to
45e0dbe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 307-308: The grep pattern treating 5xx as “reachable” is too broad
in TC-NET-03; update the regex checks that currently use
"STATUS_[2-5][0-9][0-9]" to only match 2xx–4xx by changing them to
"STATUS_[2-4][0-9][0-9]" (apply this change to the occurrences around the checks
that skip with skip "TC-NET-03" and the similar check later in the same script).
Ensure both instances are updated so the test only treats 2xx–4xx responses as
reachable.
🪄 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: Pro Plus
Run ID: 5c7af532-9787-4e20-805d-bb74e3932b0a
📒 Files selected for processing (1)
test/e2e/test-network-policy.sh
TruongNguyenG
force-pushed
the
feat/e2e-network-policy
branch
from
45e0dbe to
0900236
Compare
PR NVIDIA#2070 changed nemoclaw policy-add to require a preset name argument in non-interactive mode. Replace the expect-based interactive approach with direct CLI calls: nemoclaw <sandbox> policy-add <preset> --yes. Add apply_preset_interactive() for TC-NET-03 to also cover the interactive code path (unsets NEMOCLAW_NON_INTERACTIVE, uses expect). Fix TC-NET-03 assertion: accept any HTTP status (2xx-4xx) as reachable since Telegram returns 403 to unauthenticated requests — the endpoint is reachable through the proxy, Telegram itself rejects the bare request. Fixes NVIDIA#2148 Verified on CI: 10/10 PASS on ubuntu-latest. Signed-off-by: Truong Nguyen <tgnguyen@nvidia.com> Made-with: Cursor
TruongNguyenG
force-pushed
the
feat/e2e-network-policy
branch
from
0900236 to
abcaac6
Compare
cv
approved these changes
Apr 21, 2026
hunglp6d
pushed a commit
that referenced
this pull request
Apr 21, 2026
#2149) ## Summary Fix 4 nightly failures in `test/e2e/test-network-policy.sh` caused by PR #2070 changing `nemoclaw policy-add` to require a preset name in non-interactive mode. ## Related Issue Fixes #2148 ## Changes - **Replace expect-based preset application with direct CLI calls:** `apply_preset` now calls `nemoclaw <sandbox> policy-add <preset> --yes` directly instead of capturing the interactive list, grepping for a number, and automating prompts with `expect`. This works with the new non-interactive mode from #2070. - **Add interactive mode coverage (TC-NET-03):** New `apply_preset_interactive` function unsets `NEMOCLAW_NON_INTERACTIVE` and uses `expect` to exercise the interactive prompt path. TC-NET-03 tries interactive first, falls back to non-interactive. Both CLI code paths are now tested. - **Fix TC-NET-03 reachability assertion:** After applying the telegram preset, `api.telegram.org` returns HTTP 403 (unauthenticated — no bot token). This is correct: the proxy lets the request through, Telegram rejects the bare request. Changed assertion to accept any HTTP status (2xx-4xx) as "reachable" — only `ERROR_` responses (proxy blocks) count as failure. - **Remove expect as hard dependency:** `expect` is installed with a warning if missing (only needed for TC-NET-03 interactive test). Non-interactive tests work without it. ## Type of Change - Code change (feature, bug fix, or refactor) ## Verification - `npx prek run --all-files` passes - Tests added or updated for new or changed behavior - No secrets, API keys, or credentials committed - Verified on CI: 10/10 PASS on ubuntu-latest ## AI Disclosure - AI-assisted — tool: Cursor --- Signed-off-by: Truong Nguyen <tgnguyen@nvidia.com> Made with [Cursor](https://cursor.com) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Tests now tolerate missing interactive tooling and log a warning instead of aborting. * Added non-interactive and interactive preset-application flows with automatic fallback when interactive tooling is unavailable. * Simplified one preset workflow to a direct dry-run path that checks for "would be opened" output. * Improved result classification to better distinguish success, proxy-blocked, and unexpected responses. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Truong Nguyen <tgnguyen@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
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
Fix 4 nightly failures in
test/e2e/test-network-policy.shcaused by PR #2070 changingnemoclaw policy-addto require a preset name in non-interactive mode.Related Issue
Fixes #2148
Changes
Replace expect-based preset application with direct CLI calls:
apply_presetnow callsnemoclaw <sandbox> policy-add <preset> --yesdirectly instead of capturing the interactive list, grepping for a number, and automating prompts withexpect. This works with the new non-interactive mode from fix(cli): support non-interactive policy preset updates (Fixes #2067) #2070.Add interactive mode coverage (TC-NET-03): New
apply_preset_interactivefunction unsetsNEMOCLAW_NON_INTERACTIVEand usesexpectto exercise the interactive prompt path. TC-NET-03 tries interactive first, falls back to non-interactive. Both CLI code paths are now tested.Fix TC-NET-03 reachability assertion: After applying the telegram preset,
api.telegram.orgreturns HTTP 403 (unauthenticated — no bot token). This is correct: the proxy lets the request through, Telegram rejects the bare request. Changed assertion to accept any HTTP status (2xx-4xx) as "reachable" — onlyERROR_responses (proxy blocks) count as failure.Remove expect as hard dependency:
expectis installed with a warning if missing (only needed for TC-NET-03 interactive test). Non-interactive tests work without it.Type of Change
Verification
npx prek run --all-filespassesAI Disclosure
Signed-off-by: Truong Nguyen tgnguyen@nvidia.com
Made with Cursor
Summary by CodeRabbit