test(e2e): skip cleanly under VPN, cover Discord token rotation - #2257
Merged
Merged
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:
📝 WalkthroughWalkthroughAdded Discord coverage and provider-isolation checks to the token-rotation E2E: workflow env for Discord tokens, Discord prereq guards, SKIP handling for external API/network install failures, provider-isolated rotation phases for Telegram and Discord, and updated summary reporting including skip counts. Changes
Sequence DiagramsequenceDiagram
participant Test as Token Rotation Test
participant Install as install.sh
participant Telegram as Telegram API
participant Discord as Discord API
participant Sandbox as Sandbox Registry
participant Rotation as Rotation Detection Logic
Test->>Install: Phase 0: run with TELEGRAM_BOT_TOKEN_A, DISCORD_BOT_TOKEN_A
alt External API unreachable
Install-->>Test: exit non-zero (network/API reachability failure)
Test->>Test: scan install log for reachability failure
Test->>Test: record SKIP, set INSTALL_OK=0
else Installation succeeds
Install->>Telegram: validate Telegram connectivity
Install->>Discord: validate Discord connectivity
Install->>Sandbox: register providers and store credential hashes
Install-->>Test: success (INSTALL_OK=1)
end
Note over Test: Phase 1 — verify install state
Test->>Sandbox: assert `telegram-bridge` exists
Test->>Sandbox: assert `discord-bridge` exists
Test->>Sandbox: assert both credential hashes present
Note over Test: Phase 2 — rotate Telegram only
Test->>Rotation: re-onboard with TELEGRAM_BOT_TOKEN_B, DISCORD_BOT_TOKEN_A
Rotation->>Sandbox: detect Telegram changed, Discord unchanged
Rotation-->>Test: log rotation outcome
Test->>Test: assert log contains 'telegram-bridge'
Test->>Test: assert log does NOT contain 'discord-bridge'
Note over Test: Phase 4 — rotate Discord only
Test->>Rotation: re-onboard with TELEGRAM_BOT_TOKEN_B, DISCORD_BOT_TOKEN_B
Rotation->>Sandbox: detect Discord changed, Telegram unchanged
Rotation-->>Test: log rotation outcome
Test->>Test: assert log contains 'discord-bridge'
Test->>Test: assert log does NOT contain 'telegram-bridge'
Test->>Test: print final summary: Passed / Failed / Skip counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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.
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-token-rotation.sh (1)
150-180:⚠️ Potential issue | 🟠 MajorGate tool availability checks behind
INSTALL_OKto prevent hard exit before skip phases.The unconditional
command -v openshellandnemoclawchecks (lines 162–170) will hard-exit if the tools are not on PATH, preventing theINSTALL_OKconditional (line 172) from running and skipping the verification phases. Ifinstall.shfails before tool installation completes, these checks will override the intended skip behavior. Move the availability checks inside theif [ "$INSTALL_OK" = "1" ]block or gate them with a conditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-token-rotation.sh` around lines 150 - 180, The openshell and nemoclaw PATH checks currently run unconditionally and can abort the script before the INSTALL_OK check and skip phases; move the command -v checks for openshell and nemoclaw (and the subsequent pass/fail handling and exit) inside the INSTALL_OK guard (the if [ "$INSTALL_OK" = "1" ] block) or wrap them with a conditional that first tests INSTALL_OK, so the verification phases are only gated when INSTALL_OK is 1 and missing tools do not prevent the intended skip behavior.
🧹 Nitpick comments (1)
test/e2e/test-token-rotation.sh (1)
85-98: Route prereq skips through the shared summary path.These early
echo ...; exit 0branches still bypassskip(),TOTAL, and the final Summary, so a missing or duplicated Discord token ends the script without the accounting this PR adds. Prefer recording the skip and falling through to the Summary instead of exiting here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-token-rotation.sh` around lines 85 - 98, Replace the early echo+exit branches that check DISCORD_BOT_TOKEN_A/B and TELEGRAM_BOT_TOKEN_A/B with calls to the test harness skip() so the script records the skip and continues to the final Summary/TOTAL accounting instead of exiting; specifically, in the blocks referencing the DISCORD_BOT_TOKEN_A, DISCORD_BOT_TOKEN_B, TELEGRAM_BOT_TOKEN_A, and TELEGRAM_BOT_TOKEN_B checks, call skip("reason about missing or duplicate token") (or the existing skip helper signature used elsewhere) and do not call exit, allowing the script to fall through to TOTAL and the final Summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/e2e/test-token-rotation.sh`:
- Around line 150-180: The openshell and nemoclaw PATH checks currently run
unconditionally and can abort the script before the INSTALL_OK check and skip
phases; move the command -v checks for openshell and nemoclaw (and the
subsequent pass/fail handling and exit) inside the INSTALL_OK guard (the if [
"$INSTALL_OK" = "1" ] block) or wrap them with a conditional that first tests
INSTALL_OK, so the verification phases are only gated when INSTALL_OK is 1 and
missing tools do not prevent the intended skip behavior.
---
Nitpick comments:
In `@test/e2e/test-token-rotation.sh`:
- Around line 85-98: Replace the early echo+exit branches that check
DISCORD_BOT_TOKEN_A/B and TELEGRAM_BOT_TOKEN_A/B with calls to the test harness
skip() so the script records the skip and continues to the final Summary/TOTAL
accounting instead of exiting; specifically, in the blocks referencing the
DISCORD_BOT_TOKEN_A, DISCORD_BOT_TOKEN_B, TELEGRAM_BOT_TOKEN_A, and
TELEGRAM_BOT_TOKEN_B checks, call skip("reason about missing or duplicate
token") (or the existing skip helper signature used elsewhere) and do not call
exit, allowing the script to fall through to TOTAL and the final Summary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e3f6238-e719-426a-922f-82bbd7da896b
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamltest/e2e/test-token-rotation.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-token-rotation.sh`:
- Around line 115-117: Test inherits ambient messaging tokens causing
nondeterministic providers; before setting TELEGRAM_BOT_TOKEN and
DISCORD_BOT_TOKEN in the test script, explicitly clear unrelated messaging env
vars so src/lib/onboard.ts (the code that scans env for messaging tokens around
lines 3395-3415) only sees the tokens you intend; unset common Slack vars (e.g.
SLACK_BOT_TOKEN, SLACK_APP_TOKEN and any SLACK_*), plus any other
provider-specific messaging tokens your repo uses, then export
TELEGRAM_BOT_TOKEN/DISCORD_BOT_TOKEN and NEMOCLAW_SANDBOX_NAME; apply the same
clearing/unsetting at the other mentioned spots (around lines 231-233 and
308-309) to keep tests deterministic.
- Around line 85-98: Replace the immediate exit 0 in the prereq guards with
routing into the script's common Summary path: instead of calling exit 0 inside
the DISCORD_BOT_TOKEN_A/ DISCORD_BOT_TOKEN_B and
TELEGRAM_BOT_TOKEN_A/TELEGRAM_BOT_TOKEN_B checks, set a SKIP_REASON or SKIP_FLAG
(e.g., SKIP_REASON="DISCORD tokens missing" or "tokens identical") and then jump
to or call the existing final Summary routine (the script’s "final Summary"
path) so the per-phase skip accounting and final summary are still printed
before the script exits.
🪄 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: bba7c8a4-1eae-4448-bc91-a88252dbfb56
📒 Files selected for processing (1)
test/e2e/test-token-rotation.sh
Contributor
✅ Actions performedReview triggered.
|
…IA/NemoClaw into fix/token-rotation-telegram-discord
Contributor
ericksoa
approved these changes
Apr 23, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Good extension — cross-talk assertions between Telegram and Discord rotation are the right test. VPN/proxy resilience with SKIP instead of hard-exit is a welcome improvement. LGTM.
prekshivyas
added a commit
that referenced
this pull request
Apr 23, 2026
`nightly-e2e.yaml:232-233` sets `DISCORD_BOT_TOKEN_A`/`_B` to the fake values `test-fake-discord-A-rotation-e2e` and `test-fake-discord-B- rotation-e2e` as part of the token-rotation E2E (added in #2257). The `discord-client-secret` gitleaks rule false-positives on these strings because the entropy and shape match the pattern. These values are not secrets and never leave CI. Unblocks pre-commit for any branch that rebases on current main. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
brandonpelfrey
added a commit
that referenced
this pull request
Apr 23, 2026
…2313) ## Summary Addresses the recent `network-policy-e2e` flake in nightly-e2e and the broader sandbox-leak pattern across the e2e suite. ## Root cause (immediate flake) In [run 24806434211](https://github.com/NVIDIA/NemoClaw/actions/runs/24806434211) the `network-policy-e2e` job failed at onboard with: ``` Sandbox 'e2e-net-policy' already exists but is not ready. Pass --recreate-sandbox or set NEMOCLAW_RECREATE_SANDBOX=1 to overwrite. FATAL: Onboard failed ``` A prior run on the shared Brev launchable left `e2e-net-policy` in a not-ready state. Neither the workflow nor `test-network-policy.sh` set `NEMOCLAW_RECREATE_SANDBOX=1`, and the script's preflight cleanup (`test-network-policy.sh:217`) was guarded by `nemoclaw list | grep`, which does not surface not-ready sandboxes — so the preflight destroy never fired. ## Broader issue (sandbox leaks) 15 of 27 `test/e2e/test-*.sh` scripts had no `trap teardown EXIT`. A mid-run failure leaves the sandbox alive on the launchable until the next run's `NEMOCLAW_RECREATE_SANDBOX=1` clobbers it (if set). On the shared launchable this accumulates as dangling k8s pods, netns, volumes — eventually producing the exact not-ready state this PR is closing. ## Changes - `.github/workflows/nightly-e2e.yaml` — add `NEMOCLAW_RECREATE_SANDBOX: "1"` to the 3 jobs still missing it: `network-policy-e2e`, `inference-routing-e2e`, `deployment-services-e2e`. - `test/e2e/test-network-policy.sh`, `test/e2e/test-inference-routing.sh` — replace `list | grep`-gated preflight destroys with unconditional `nemoclaw "$SANDBOX_NAME" destroy --yes 2>/dev/null || true`. Post-failure assertions (e.g. TC-INF-06/07 checking that no sandbox was created after a bad key) kept unchanged — those are checks, not cleanup. - `test/e2e/lib/sandbox-teardown.sh` — new shared helper (executable, `#!/usr/bin/env bash`). Registers each sandbox name and installs a single `trap … EXIT` that destroys every registered sandbox and clears `onboard.lock`. Keeps the per-script diff to two lines so reviewers can audit the full wiring quickly. - 15 test scripts wired up: credential-sanitization, full-e2e, hermes-e2e, messaging-providers, onboard-repair (two sandboxes), onboard-resume, rebuild-hermes, rebuild-openclaw, sandbox-rebuild, sandbox-survival, shields-config, skip-permissions-policy, snapshot-commands, telegram-injection, upgrade-stale-sandbox. - `test-onboard-repair.sh`, `test-onboard-resume.sh` — extra conditional shim: `if ! command -v nemoclaw; then nemoclaw() { node "$REPO/bin/nemoclaw.js" "$@"; }; fi`. These two scripts drive the CLI via `run_nemoclaw` (bare `node bin/nemoclaw.js`) and don't assume a global CLI on PATH; without the shim, the trap's `nemoclaw destroy` would silently no-op in repo-local dev runs. CI (with `install-openshell.sh` + `npm link` on PATH) skips the shim. - `NEMOCLAW_E2E_KEEP_SANDBOX=1` escape hatch so a local dev reproducing a flake can inspect the sandbox after the test — CI never sets this variable. - `.gitleaksignore` — allowlist two pre-existing false-positives (`discord-client-secret` rule matching the fake `DISCORD_BOT_TOKEN_A/_B` strings at `nightly-e2e.yaml:232-233` from #2257). These are test fakes, not secrets. Unblocks pre-commit for any branch rebased onto current main. ### Intentionally skipped - `test-runtime-overrides.sh` — pure `docker run --rm`, no sandbox. - `test-spark-install.sh` — install smoke, no sandbox. - 10 scripts that already set a `trap teardown EXIT` themselves were **not** changed, because sourcing the helper would silently replace their existing trap: deployment-services, diagnostics, double-onboard, gpu-e2e, inference-routing, network-policy, ollama-auth-proxy, sandbox-operations, token-rotation, e2e-cloud-experimental. ## Type of Change - [x] Code change for a bug fix / infra robustness improvement. ## Testing - [x] `bash -n` syntax check on all 18 modified scripts + new helper — passes - [x] Helper smoke test with mock `nemoclaw`: `register_sandbox_for_teardown` registered two sandboxes, EXIT trap dispatched `nemoclaw destroy --yes` for both; with `NEMOCLAW_E2E_KEEP_SANDBOX=1` set, teardown correctly returned early without destroying - [x] Double-destroy audit: grepped all 15 modified scripts — every one already has an end-of-main `destroy --yes || true`. My trap adds a second destroy on EXIT that's idempotent (`|| true`) and silent (`>/dev/null 2>&1`). Functionally zero impact on success paths; benefit is protection on failure paths where mid-script exit skips the end-of-main destroy. - [x] No self-conflicting traps: grepped all 15 modified scripts — zero `trap`/`exec` calls. My helper's trap is the sole EXIT handler in each. - [x] `$REPO` ordering verified — defined at `test-onboard-repair.sh:51-53` / `test-onboard-resume.sh:67-69`, well before the shim at line 70 / 85. - [x] pre-commit hooks (prek) clean: shellcheck, shfmt, commitlint, YAML validator, CLI test suite, gitleaks - [ ] **Nightly E2E on this branch** — [run 24810292642](https://github.com/NVIDIA/NemoClaw/actions/runs/24810292642) exercises the preflight + helper + wiring changes. A follow-up run will be triggered on the latest HEAD to cover the CodeRabbit fixes (shebang + shim). ## Known gaps (not in this PR) **CI coverage:** 5 of 15 modified scripts aren't invoked by any GH Actions workflow — `test-credential-sanitization`, `test-onboard-repair`, `test-onboard-resume`, `test-sandbox-rebuild`, `test-telegram-injection`. Changes are defensive (they benefit when run locally or from ad-hoc workflows) but aren't exercised in the automated suite. **Signal handling:** the helper traps `EXIT` only (matching the existing convention in the 10 skipped scripts). SIGINT / SIGTERM / SIGKILL are not handled — consistent with the rest of the e2e suite. **`.gitleaksignore` fingerprints are positional** (line 232/233 of `nightly-e2e.yaml`). Inherent limitation of gitleaks' allowlist format. **Out of scope (filed for a follow-up):** - Workflow-level `if: always()` post-job cleanup (belt-and-suspenders against script crashes that never reach the trap) - Pre-run `e2e-*` sweeper at the launchable level Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Centralized sandbox teardown added to E2E tests; most scripts now register sandboxes for automatic cleanup. * Some preflight cleanup changed to attempt unconditional sandbox destroy to reduce flaky preflight checks. * Added support so teardown works even when the test CLI isn’t globally installed. * **Chores** * CI E2E runs now request sandbox recreation for nightly jobs. * Updated secret-scan ignore entries for the nightly E2E workflow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Brandon Pelfrey <bpelfrey@nvidia.com>
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
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
Extends
test/e2e/test-token-rotation.shto cover Discord rotation alongside Telegram (with provider-isolation checks), and makes the test resilient to environmental install failures so corporate-VPN runs SKIP cleanly instead of exiting 1 mid-Phase 0.Related Issue
Fixes #2247
Closes #2255
Changes
Test script (
test/e2e/test-token-rotation.sh)TELEGRAM_BOT_TOKENandDISCORD_BOT_TOKEN; Phase 1 verifies both providers and both credential hashes are stored.SKIPcounter andskip()helper. Wheninstall.shfails with(Telegram|Discord) network reachability failurein the install log (typical of VPN/proxy blockingapi.telegram.org), record a SKIP, mark Phases 1–5 as skipped, and still print the Summary instead of exiting 1.DISCORD_BOT_TOKEN_A/_Bset, A ≠ B); SKIP cleanly if either is missing.CI (
.github/workflows/nightly-e2e.yaml)DISCORD_BOT_TOKEN_A/DISCORD_BOT_TOKEN_Binto thetoken-rotation-e2ejob so the new prereq guards do not skip the test under nightly cron.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesSigned-off-by: Hung Le hple@nvidia.com
Summary by CodeRabbit
Documentation
Tests