test(e2e): channels add/remove lifecycle#3696
Merged
Merged
Conversation
|
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. |
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:
📝 WalkthroughWalkthroughAdds a new E2E test entrypoint, parity inventory/mapping, and a Bash script that onboards a sandbox, runs ChangesTelegram Channel Add/Remove E2E Test
Sequence DiagramsequenceDiagram
participant Sandbox
participant NemoclawCLI
participant Gateway
participant api_telegram_org as api.telegram.org
Sandbox->>NemoclawCLI: run `nemoclaw channels add/remove` (via sandbox_exec)
NemoclawCLI->>Gateway: register/unregister telegram-bridge provider
NemoclawCLI->>Sandbox: trigger `nemoclaw rebuild` (bakes openclaw.json)
Gateway->>api_telegram_org: L7 egress probe (connectivity via proxy/preset)
api_telegram_org-->>Gateway: HTTP response (2xx/4xx) or network error
Gateway-->>Sandbox: policy/preset visible via `nemoclaw policy list`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 198-203: The script currently checks NEMOCLAW_NON_INTERACTIVE but
lacks an upfront check for the documented prerequisite
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE; add a similar explicit validation before
tests run that verifies the environment variable
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE is set to "1" and call the existing fail
(and print_summary) functions on mismatch, otherwise call pass with a
descriptive message so the test fails fast; reference the same check pattern
used for NEMOCLAW_NON_INTERACTIVE and reuse the fail, print_summary, and pass
helpers.
- Around line 114-127: The helper openclaw_has_telegram() is checking the
runtime sandbox path /sandbox/.openclaw/openclaw.json but the test should assert
the baked-image file at /opt/nemoclaw/openclaw.json; update the sandbox_exec
Python command in openclaw_has_telegram() to load /opt/nemoclaw/openclaw.json
instead, keeping the same output logic (print "yes" if "telegram" in
d.get("channels",{}) else "no") and preserving the same return codes (0 for yes,
1 for no, 2 for unreadable/other).
🪄 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: 67187ac8-e2d2-4361-b7cf-bc6e651c9695
📒 Files selected for processing (3)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yamltest/e2e/test-channels-add-remove.sh
…CEPT_THIRD_PARTY_SOFTWARE=1
Contributor
|
✨
Related open issues: |
cjagwani
approved these changes
May 19, 2026
cjagwani
left a comment
Contributor
There was a problem hiding this comment.
Approving after a rebase to clear DIRTY.
Strong test PR — three files, all test artifacts, every assertion ties to either the #3462 spec or a named shipped regression (#3437 at C4a, #3671 at C6c). CI is fully green incl. ShellCheck + the full self-hosted e2e suite.
Notes for the author:
-
CodeRabbit's
/opt/nemoclaw/openclaw.jsonsuggestion (line 114-127) is incorrect — the canonical runtime path is/sandbox/.openclaw/openclaw.json, matching sibling Test 1 on main (test-channels-stop-start.sh:247). You correctly kept the existing path. No change needed. -
Minor gap vs. #3462 Test 2 clause 7: the spec asks to assert "registry shows telegram removed from
messagingChannels". C6a (baked openclaw.json) + C6b (gateway provider gone) cover this implicitly via the rebuild chain, but a direct registry check would close the loop. Optional follow-up — not blocking. -
The
node -e fetchegress probe (line 166) is well-targeted: it matches the telegram preset's binary+path scoping and accepts STATUS_2xx OR 4xx so a fake token producing 401 still proves CONNECT passed. Nice.
Please rebase on main and we can merge.
Contributor
Author
|
Thanks @cjagwani , resolved the conflicts and ready to merge |
12 tasks
ericksoa
pushed a commit
that referenced
this pull request
May 20, 2026
<!-- markdownlint-disable MD041 --> ## Summary Wire `test-channels-add-remove.sh` into the nightly E2E workflow. The test landed via PR #3696 but didn't get a nightly job slot at the time — adding one now so regressions for #3437 (channels add preset auto-apply) and #3671 (channels remove detach + un-apply + survive rebuild) get scheduled coverage. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> ## Changes - New job `channels-add-remove-e2e` runs `bash test/e2e/test-channels-add-remove.sh` on `ubuntu-latest`. - Env: `TELEGRAM_BOT_TOKEN=test-fake-telegram-token-add-remove-e2e`, `NEMOCLAW_NON_INTERACTIVE=1`, `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1`, `NEMOCLAW_SANDBOX_NAME=e2e-channels-add-remove`. Telegram-only — Discord/Slack/WeChat walk the same `KNOWN_CHANNELS` + preset lookup code path (already covered by `channels-stop-start-e2e`). - On failure, uploads `/tmp/nemoclaw-e2e-install.log`, `/tmp/nc-{add,remove}.log`, `/tmp/nc-rebuild-{add,remove}.log`. - Registered in: - `inputs.jobs` description (for `workflow_dispatch` job selection). - Three downstream aggregation `needs:` lists alongside `channels-stop-start-e2e`. ## 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 - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [ ] 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) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Hung Le <hple@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added new end-to-end test for channels add/remove functionality with improved failure diagnostics and automated artifact collection during nightly test runs. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3932?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 -->
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
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