test(e2e): channels add/remove lifecycle - #3673
Closed
hunglp6d wants to merge 15 commits into
Closed
Conversation
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new end-to-end test script exercising the Telegram channel add/remove lifecycle in an empty sandbox, with supporting parity-tracking YAML and JSON files documenting 44 new assertions covering prerequisites, baseline absence, add+rebuild, egress validation via L7 proxy, removal, and cleanup. ChangesTelegram Channel Add/Remove E2E Test
Sequence Diagram(s)N/A — This is a test script verifying existing functionality; no new API flows or multi-component interactions are being introduced. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
hunglp6d
marked this pull request as ready for review
May 18, 2026 08:15
![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: 3
🤖 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 `@test/e2e/test-channels-add-remove.sh`:
- Around line 174-179: The grep pattern in the test (the if condition that
checks "$body" in test/e2e/test-channels-add-remove.sh) wrongly treats the
generic string "fetch failed" as a policy denial; remove "fetch failed" from the
regex (keep explicit tokens like "policy_denied", "engine:ssrf", "forbidden by
policy", and "CONNECT.*40[0-9]") so transient DNS/TCP errors are not classified
as policy rejections and the script only returns 1 for concrete policy-related
signatures.
- Around line 193-198: Add an early validation for the
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE env var similar to the existing
NEMOCLAW_NON_INTERACTIVE check: if [ "${NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE:-}"
!= "1" ]; then call fail "C?: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is
required" and print_summary, else call pass "C?:
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is set"; place this check near the top of
test/e2e/test-channels-add-remove.sh before any long install or setup steps so
the script fails fast (use the same fail, print_summary, and pass helpers as in
the existing block).
- Around line 121-124: The case branch is too permissive because the pattern
"*no*" can match substrings like "not known"; update the parsing of $out (from
openclaw_has_telegram) to normalize and compare exact tokens: trim
leading/trailing whitespace and convert to lowercase, then test explicitly (e.g.
if [ "$out" = "yes" ] return 0; elif [ "$out" = "no" ] return 1; else return 2)
so only exact "yes" or "no" succeed and all other outputs are treated as
unreadable.
🪄 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: 209faf8d-58d7-46db-b834-47a723225612
📒 Files selected for processing (3)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yamltest/e2e/test-channels-add-remove.sh
Comment on lines
+193
to
+198
| if [ "${NEMOCLAW_NON_INTERACTIVE:-}" != "1" ]; then | ||
| fail "C0: NEMOCLAW_NON_INTERACTIVE=1 is required" | ||
| print_summary | ||
| fi | ||
| pass "C0: NEMOCLAW_NON_INTERACTIVE=1 is set" | ||
|
|
Contributor
There was a problem hiding this comment.
Validate NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 up front.
The script documents this prerequisite but doesn’t enforce it early, which can defer failure into the long install phase.
Suggested fix
if [ "${NEMOCLAW_NON_INTERACTIVE:-}" != "1" ]; then
fail "C0: NEMOCLAW_NON_INTERACTIVE=1 is required"
print_summary
fi
pass "C0: NEMOCLAW_NON_INTERACTIVE=1 is set"
+
+if [ "${NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE:-}" != "1" ]; then
+ fail "C0: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is required"
+ print_summary
+fi
+pass "C0: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is set"📝 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
| if [ "${NEMOCLAW_NON_INTERACTIVE:-}" != "1" ]; then | |
| fail "C0: NEMOCLAW_NON_INTERACTIVE=1 is required" | |
| print_summary | |
| fi | |
| pass "C0: NEMOCLAW_NON_INTERACTIVE=1 is set" | |
| if [ "${NEMOCLAW_NON_INTERACTIVE:-}" != "1" ]; then | |
| fail "C0: NEMOCLAW_NON_INTERACTIVE=1 is required" | |
| print_summary | |
| fi | |
| pass "C0: NEMOCLAW_NON_INTERACTIVE=1 is set" | |
| if [ "${NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE:-}" != "1" ]; then | |
| fail "C0: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is required" | |
| print_summary | |
| fi | |
| pass "C0: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is set" |
🤖 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/test-channels-add-remove.sh` around lines 193 - 198, Add an early
validation for the NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE env var similar to the
existing NEMOCLAW_NON_INTERACTIVE check: if [
"${NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE:-}" != "1" ]; then call fail "C?:
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is required" and print_summary, else call
pass "C?: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 is set"; place this check near
the top of test/e2e/test-channels-add-remove.sh before any long install or setup
steps so the script fails fast (use the same fail, print_summary, and pass
helpers as in the existing block).
## Summary Harden the high-reward/low-risk code scanning findings by redacting logged values, replacing predictable temp/random patterns, and tightening URL/search validation paths. This targets the first remediation bucket from the code scanning risk/reward tracker. ## Related Issue Refs #3654 ## Changes - Add recursive log redaction for JSON command output and remove credential/env names from user-facing diagnostics. - Replace `Math.random()` session/temp suffixes with `crypto.randomUUID()`. - Move temp config/SSH files into private `mkdtemp` directories and write service PID files via secured file descriptors. - Parse Slack and Cloudflare hosts via URL/anchored host validation instead of broad substring URL checks. - Sanitize docs search terms before passing user input into Lunr query strategies. - Update affected tests for redacted messages and OpenShell error wording. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [ ] `npx prek run --all-files` passes - Ran twice; the full hook suite reached the CLI test phase but failed on existing 5s timeout/flaky host-environment tests unrelated to this patch. Plugin tests passed in the hook output. - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional checks run: - [x] `npm run typecheck:cli` passes - [x] `cd nemoclaw && npm run build` passes - [x] Targeted Vitest suite passes: `src/lib/security/redact.test.ts`, `src/lib/adapters/openshell/client.test.ts`, `src/lib/onboard/summary.test.ts`, `src/lib/onboard/preflight.test.ts`, `src/lib/tunnel/services.test.ts`, `src/lib/state/onboard-session.test.ts`, `src/lib/diagnostics/debug.test.ts`, `src/lib/actions/dev/npm-link-or-shim.test.ts`, `test/rebuild-credential-preflight.test.ts` - [x] `npx vitest run test/nemoclaw-start.test.ts -t "Slack channel guard"` passes - [x] `cd nemoclaw && npm test -- src/onboard/config.test.ts` passes --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Enhanced security redaction for sensitive data in logs and JSON output * **Bug Fixes** * Improved detection of Slack-related connection errors * Better DNS probing for container connectivity validation * **Improvements** * Cleaner onboarding messages that avoid exposing environment variable names and technical paths * More secure temporary file and directory handling with restrictive permissions * Generic error messages that provide clarity without revealing sensitive system information * Hardened sandbox rebuild logging with automatic redaction <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3657?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3657?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
## Summary Fixes #3578. This makes GPU-enabled onboarding recover from a healthy reusable gateway that was previously started without GPU passthrough, instead of aborting with a full `nemoclaw uninstall && nemoclaw onboard --gpu` recovery path. The recovery is intentionally conservative: - Reuse is unchanged when GPU is not requested, when the reusable gateway is already a confirmed Docker-driver gateway, or when a legacy gateway already has Docker GPU DeviceRequests. - A CPU-only legacy gateway is automatically retired only when no local sandboxes are registered, or when `NEMOCLAW_RECREATE_SANDBOX=1` is recreating the one registered sandbox with the same name. - If other registered sandboxes depend on the gateway, onboarding still aborts non-destructively and prints targeted `destroy --cleanup-gateway` guidance. - Unknown Docker/container state remains non-destructive. - `--no-gpu` / `NEMOCLAW_SANDBOX_GPU=0` remain explicit opt-outs; this does not silently downshift GPU-selected onboarding to CPU. I also tightened the manual recovery message so the empty-registry fallback no longer recommends full uninstall first. ## Testing for reviewers The focused unit coverage exercises the issue decision matrix: - GPU requested + healthy CPU-only legacy gateway + empty registry => restart/recreate path. - GPU requested + healthy CPU-only legacy gateway + the one registered sandbox being recreated => restart/recreate path. - GPU requested + healthy CPU-only legacy gateway + shared/different registered sandboxes => abort with targeted recovery. - GPU not requested => reuse path unchanged. - Confirmed Docker-driver gateway => reuse path unchanged. - Unknown legacy Docker inspection state => non-destructive abort. - Empty-registry recovery wording recommends targeted gateway cleanup, not full uninstall. Local validation run: ```text npm run build:cli npm test -- src/lib/onboard/gateway-gpu-passthrough.test.ts src/lib/onboard/gpu-recovery.test.ts src/lib/onboard/sandbox-gpu-create.test.ts src/lib/onboard/docker-gpu-patch.test.ts npm run typecheck npm run source-shape:check git diff --check npx prek run --all-files ``` All of the above passed locally. I did not run live GPU UAT on Ubuntu GPU, DGX Spark, or DGX Station hardware. That is still the end-to-end validation needed for the exact reporter scenario: first create a CPU/no-GPU gateway, leave the stale gateway behind, then rerun GPU-default onboarding with `NEMOCLAW_RECREATE_SANDBOX=1` and verify the new sandbox gets GPU access. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved GPU passthrough detection and reuse logic to avoid unsafe reuse and to perform safer restart or abort-with-recovery when needed * Updated recovery guidance for GPU passthrough issues with clearer cleanup and re-onboarding instructions * **New Features** * Added explicit GPU inspection and decision flow to better handle legacy gateway GPU states and restart safety checks * **Tests** * Expanded and hardened tests around GPU inspection, reuse decisions, recovery messaging, and CLI timeouts/robustness * **Chores** * Hardened pre-commit test hook execution (serialized runs and cleanup before build) <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3670?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3670?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
…2e-channels-add-remove
…alse baseline/remove passes
wscurran
added
area: e2e
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
4 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
Add Test 2 from #3462 — end-to-end coverage for the
onboard empty → channels add → channels removelifecycle. The test asserts against the baked sandbox image (openclaw.json) and the gateway (provider attachment, policy presets) so regressions cannot hide behind a correct host-side registry. Companion to Test 1 (test-channels-stop-start.sh) shipped via #3532.Related Issue
Closes #3462
Changes
test/e2e/test-channels-add-remove.shcovering Test 2 spec from test(messaging): add E2E coverage for channels stop/start and channels add/remove rebuild flows #3462:telegraminopenclaw.json, no telegram preset applied).channels add telegram+ rebuild; asserts the matching preset auto-applied (bug(messaging): channels add <mesaging> doesn't apply the network policy #3437 regression gate), bridge provider exists,openclaw.jsoncontains the channel, and the L7 proxy lets bridge-style traffic through toapi.telegram.org.channels remove telegram+ rebuild; asserts the bridge is gone fromopenclaw.json, provider deleted from the gateway, and the matching preset un-applied ([Channels]channels remove <ch>fails on a live sandbox and leaves the channel's policy preset applied #3671 regression gate for the detach+delete + symmetric cleanup fix shipped in fix(channels): detach provider + un-apply preset on channels remove #3672).telegram_egress_open) usesnode -e fetchagainst a/bot*/...path so it matches the telegram preset's binary + path scoping. A naïvecurl /probe would trip both guards and produce a misleading 403, hiding real regressions.print_policy_listsnapshots the gateway's policy-list output before everypolicy_list_has_presetassertion so the test transcript records the actual gateway state alongside each pass/fail line.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Hung Le hple@nvidia.com
Summary by CodeRabbit
Tests
Documentation