fix(e2e): replace hard exits with skip-and-continue in test-token-rotation.sh - #2256
Closed
kagura-agent wants to merge 2 commits into
Closed
kagura-agent wants to merge 2 commits into
kagura-agent wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughReworked Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as "Test Runner\n(test-token-rotation.sh)"
participant Installer as "Installer\n(install.sh)"
participant Tools as "Local Tools\n(openshell/nemoclaw)"
participant External as "External APIs\n(e.g., Telegram)"
Runner->>Installer: run install.sh
alt install exits 0
Installer-->>Runner: success
Runner->>Tools: verify PATH/tools
Tools-->>Runner: tools present
Runner->>External: run Phase 1..5 as gated by flags
External-->>Runner: responses
else install exits non-zero
Installer-->>Runner: failure (logs)
Runner->>Runner: is_environmental_failure()?
alt environmental failure
Runner-->>Runner: increment SKIP, set PHASE0_OK=false
Runner->>Runner: mark dependent phases SKIPPED
else infrastructure failure
Runner-->>Runner: record FAIL / hard-exit if critical
end
end
Runner->>Runner: summarize results (Pass / Fail / Skip, include Skip count)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
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-token-rotation.sh (1)
131-137:⚠️ Potential issue | 🟠 MajorTreat Phase 0 preflight/onboard failures as
SKIP, not unconditionalFAIL.
install.sh --non-interactiveincludes the first onboard, so a non-zeroinstall_exitcan still be the environmental preflight case from#2247. Recording every non-zero exit as FAIL will keep failing CI for exactly the scenario this PR is meant to downgrade to skip.Suggested direction
+is_environmental_preflight_failure() { + grep -Eq 'preflight|unreachable|Telegram|Discord' "$1" +} + if [ $install_exit -eq 0 ]; then pass "install.sh completed (exit 0)" else - fail "install.sh failed (exit $install_exit)" + if is_environmental_preflight_failure "$INSTALL_LOG"; then + skip "Phase 0 skipped due to environmental preflight failure (exit $install_exit)" + else + fail "install.sh failed (exit $install_exit)" + fi info "Last 30 lines of install log:" tail -30 "$INSTALL_LOG" 2>/dev/null || true PHASE0_OK=false fi🤖 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 131 - 137, The current block treats any non-zero install_exit as a hard FAIL; change it so a non-zero exit from running install.sh (which may be an environment/preflight onboarding case) is recorded as a SKIP rather than unconditional FAIL: when install_exit is 0 keep pass "install.sh completed (exit 0)"; otherwise call skip "install.sh skipped (exit $install_exit)" (or use the existing skip helper if present), print the last 30 lines of INSTALL_LOG as you already do, and set PHASE0_OK=false or a PHASE0_SKIPPED flag as appropriate—update references to install_exit, install.sh, and PHASE0_OK in this block to reflect SKIP semantics instead of fail.
🤖 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 192-250: Phase 3 must be gated on Phase 2 success and Phase 2
preflight/environmental failures should be downgraded to SKIP: introduce a
PHASE2_OK boolean (set to true only when the Phase 2 onboarding completes with
exit 0 and the output confirms rotation and sandbox rebuild—i.e., when the
checks around "credential(s) rotated" and "Rebuilding sandbox" both pass),
change the Phase 3 guard to if [ "$PHASE0_OK" != true ] || [ "$PHASE2_OK" !=
true ] to skip when Phase 2 did not establish the rotated-token baseline, and
update Phase 2 failure branches that currently call fail for known
environmental/preflight errors (detect via onboard_exit non-zero combined with
output matching preflight/env error strings) to call skip instead of fail so
preflight failures are recorded as SKIP rather than failing downstream phases.
---
Outside diff comments:
In `@test/e2e/test-token-rotation.sh`:
- Around line 131-137: The current block treats any non-zero install_exit as a
hard FAIL; change it so a non-zero exit from running install.sh (which may be an
environment/preflight onboarding case) is recorded as a SKIP rather than
unconditional FAIL: when install_exit is 0 keep pass "install.sh completed (exit
0)"; otherwise call skip "install.sh skipped (exit $install_exit)" (or use the
existing skip helper if present), print the last 30 lines of INSTALL_LOG as you
already do, and set PHASE0_OK=false or a PHASE0_SKIPPED flag as
appropriate—update references to install_exit, install.sh, and PHASE0_OK in this
block to reflect SKIP semantics instead of fail.
🪄 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: 438333c5-f8e0-4aa4-b3fe-c1c1ab63e9e7
📒 Files selected for processing (1)
test/e2e/test-token-rotation.sh
Contributor
Author
|
Thanks for the review! Both suggestions addressed in 497febb:
|
Contributor
|
✨ Thanks for submitting this PR that proposes a way to improve the E2E testing process. Related open issues: |
…ation.sh (NVIDIA#2247) Replace `exit 1` after install/onboard failures with a skip-and-continue pattern so CI always prints the Summary section and marks dependent phases as skipped instead of aborting silently. Changes: - Add SKIP counter and skip() helper (yellow output) - Track Phase 0 success with PHASE0_OK flag - When Phase 0 fails (e.g. Telegram API unreachable), Phases 1-3 are marked SKIP instead of never running - When Phase 2/3 onboard fails, record FAIL but continue to Summary - Summary line now includes Skip count Fixes NVIDIA#2247
… 3 on Phase 2 Address CodeRabbit feedback: - Add is_environmental_failure() to detect network/preflight issues in install log and record them as SKIP instead of FAIL - Track Phase 2 success with PHASE2_OK flag - Gate Phase 3 on both Phase 0 and Phase 2 success, since Phase 3 (same-token reuse) depends on Phase 2 (token rotation) completing
kagura-agent
force-pushed
the
fix/e2e-token-rotation-hard-exit
branch
from
April 23, 2026 12:57
497febb to
550502c
Compare
Contributor
Author
|
Rebased onto latest main to resolve conflicts. All changes preserved. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/e2e/test-token-rotation.sh (2)
282-303:⚠️ Potential issue | 🟠 MajorOnly mark a rotation phase “OK” after its baseline checks pass, and mirror that for Phase 5.
PHASE2_OKflips totruebefore the rotation/rebuild assertions run, and Phase 5 has no Phase 4 success gate at all. That lets the “same tokens” phases run against an unverified baseline and can turn the later result into noise.Suggested direction
PHASE2_OK=false +PHASE4_OK=false ... - else - PHASE2_OK=true + else + phase2_checks_ok=true fi ... if echo "$ONBOARD_OUTPUT" | grep -q "credential(s) rotated"; then pass "Credential rotation detected" else fail "Credential rotation not detected in onboard output" + phase2_checks_ok=false fi ... if echo "$ONBOARD_OUTPUT" | grep -q "Rebuilding sandbox"; then pass "Sandbox rebuild triggered by rotation" else fail "Sandbox rebuild not triggered" + phase2_checks_ok=false fi ... + if [ $onboard_exit -eq 0 ] && [ "$phase2_checks_ok" = true ]; then + PHASE2_OK=true + fi ... -if [ "$PHASE0_OK" != true ]; then - skip "Phase 5 — skipped (Phase 0 failed)" +if [ "$PHASE0_OK" != true ] || [ "$PHASE4_OK" != true ]; then + skip "Phase 5 — skipped (Phase 0 or Phase 4 did not succeed)"Also applies to: 306-346, 352-353, 372-380, 385-431, 437-439
🤖 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 282 - 303, The test currently sets PHASE2_OK (and similarly other PHASEx flags) to true before performing the rotation/rebuild assertions; change the flow so each PHASEx_OK flag (e.g., PHASE2_OK, PHASE5_OK) is only set to true after the baseline onboarding/check assertions succeed (i.e., after verifying onboard_exit == 0 and any baseline checks/expectations); additionally add a prerequisite gate for Phase 5 so it only runs when PHASE4_OK is true; apply the same ordering/gating fix to the other phase blocks referenced (lines for Phase 3/4/5/6 placeholders) so no phase flips true until its baseline verification completes and later phases check the previous PHASE*_OK before running.
299-301:⚠️ Potential issue | 🟠 MajorKeep environmental/preflight onboard failures as
SKIPin the later phases too.These branches still unconditionally
failon non-zeronemoclaw onboard, so the same blocked-network/preflight condition you now classify asSKIPin Phase 0 is still reported asFAILin Phases 2–5. That leaves the#2247behavior only half-fixed.Suggested direction
+is_environmental_failure_text() { + printf '%s' "$1" | grep -Eqi 'not reachable|unreachable|preflight|network reachability failure' +} + if [ $onboard_exit -ne 0 ]; then - fail "Phase 2 onboard failed (exit $onboard_exit)" + if is_environmental_failure_text "$ONBOARD_OUTPUT"; then + skip "Phase 2 skipped — environmental preflight failure (exit $onboard_exit)" + else + fail "Phase 2 onboard failed (exit $onboard_exit)" + fi echo "$ONBOARD_OUTPUT" | tail -30 fiAlso applies to: 358-361, 388-391, 443-446
🤖 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 299 - 301, The script currently unconditionally calls fail when onboard_exit is non-zero (using variables onboard_exit, ONBOARD_OUTPUT, and the fail function); update this to detect the same preflight/blocked-network SKIP condition used in Phase 0 (reuse the Phase 0 check or helper that inspects ONBOARD_OUTPUT / exit code for the preflight/blocked-network marker) and, if that condition matches, call the test skip path instead of fail (e.g., emit SKIP or call the existing skip helper with a descriptive message); otherwise keep the existing fail behavior. Apply this conditional replacement at the shown block and the analogous blocks referenced (around lines 358-361, 388-391, 443-446).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/test-token-rotation.sh`:
- Around line 282-303: The test currently sets PHASE2_OK (and similarly other
PHASEx flags) to true before performing the rotation/rebuild assertions; change
the flow so each PHASEx_OK flag (e.g., PHASE2_OK, PHASE5_OK) is only set to true
after the baseline onboarding/check assertions succeed (i.e., after verifying
onboard_exit == 0 and any baseline checks/expectations); additionally add a
prerequisite gate for Phase 5 so it only runs when PHASE4_OK is true; apply the
same ordering/gating fix to the other phase blocks referenced (lines for Phase
3/4/5/6 placeholders) so no phase flips true until its baseline verification
completes and later phases check the previous PHASE*_OK before running.
- Around line 299-301: The script currently unconditionally calls fail when
onboard_exit is non-zero (using variables onboard_exit, ONBOARD_OUTPUT, and the
fail function); update this to detect the same preflight/blocked-network SKIP
condition used in Phase 0 (reuse the Phase 0 check or helper that inspects
ONBOARD_OUTPUT / exit code for the preflight/blocked-network marker) and, if
that condition matches, call the test skip path instead of fail (e.g., emit SKIP
or call the existing skip helper with a descriptive message); otherwise keep the
existing fail behavior. Apply this conditional replacement at the shown block
and the analogous blocks referenced (around lines 358-361, 388-391, 443-446).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8101284a-92bd-43f0-b87f-897adafafcb2
📒 Files selected for processing (1)
test/e2e/test-token-rotation.sh
jyaunches
self-requested a review
April 23, 2026 18:17
Contributor
|
Thanks for putting this together, @kagura-agent! The skip-and-continue approach and per-phase gate flags ( Heads-up: PR #2257 landed on I'd recommend closing this one since #2247 is resolved. Thanks again for the contribution! 🙏 |
Contributor
Author
|
Thanks for the kind words, @jyaunches! Glad the skip-and-continue pattern was useful. I see #2257 also added Discord rotation coverage — nice extension. Happy to contribute again on future test improvements! |
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
Replace
exit 1after install/onboard failures intest-token-rotation.shwith a skip-and-continue pattern so CI always prints the Summary section.Fixes #2247
Problem
When
install.sh --non-interactivefails due to environmental issues (e.g.api.telegram.orgunreachable from the CI network), the test script callsexit 1immediately:The same hard-exit pattern repeats in Phases 2 and 3.
Changes
SKIPcounter andskip()helper (yellow output) alongside existingpass()/fail()PHASE0_OKflagTotal: N Pass: N Fail: N Skip: NTesting
bash -n test/e2e/test-token-rotation.sh— syntax check passesexit 1calls within phase logic are replaced; only the prerequisite checks and repo-root detection retain early exits (correct behavior)Summary by CodeRabbit