fix(e2e): fix 3 stale assertions in newly-wired nightly E2E tests - #2656
Merged
Conversation
The test grepped for 'Reusing existing NemoClaw gateway' but the product now prints '✓ Reusing existing gateway' (the 'NemoClaw' prefix was dropped). Update both assertions to match the current output. Fixes double-onboard-e2e nightly failures.
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)
📝 WalkthroughWalkthroughThe test script updates gateway reuse validation by replacing log message substring matching with Docker container identity checks. The script now captures the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25088874456
|
Replace the fragile log-text grep with a Docker container ID comparison. Capture the gateway container ID before and after each onboard, then verify it's the same container. This asserts on observable state rather than log messages that drift as the product evolves.
jyaunches
changed the title
![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
🤖 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-double-onboard.sh`:
- Line 277: The docker container name filter uses substring matching which can
return the wrong container; update the docker ps calls that use docker ps -qf
"name=openshell-cluster-nemoclaw" (e.g., the assignment to GATEWAY_ID_BEFORE and
the other three occurrences) to use an exact-name regex anchor: docker ps -qf
"name=^/openshell-cluster-nemoclaw$" so the filter matches the exact container
name only.
🪄 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: feac6e1e-8a58-4be5-b14e-e2190836b69e
📒 Files selected for processing (1)
test/e2e/test-double-onboard.sh
| section "Phase 3: Second onboard ($SANDBOX_A — same name, recreate)" | ||
| info "Running nemoclaw onboard with NEMOCLAW_RECREATE_SANDBOX=1..." | ||
|
|
||
| GATEWAY_ID_BEFORE=$(docker ps -qf "name=openshell-cluster-nemoclaw" | head -1) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Broad filter (may include partial matches):"
docker ps --format '{{.ID}}\t{{.Names}}' --filter 'name=openshell-cluster-nemoclaw'
echo
echo "Exact filter (only exact container name):"
docker ps --format '{{.ID}}\t{{.Names}}' --filter 'name=^/openshell-cluster-nemoclaw$'Repository: NVIDIA/NemoClaw
Length of output: 168
🏁 Script executed:
# First, check if the file exists and read the relevant sections
wc -l test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 95
🏁 Script executed:
# Read lines around 277 (the first occurrence mentioned)
sed -n '270,285p' test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 631
🏁 Script executed:
# Read lines around 320 and 332 (other occurrences)
sed -n '315,340p' test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 1057
🏁 Script executed:
# Check for SPDX header and shebang at the start of the file
head -5 test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 248
🏁 Script executed:
# Check if file is executable
ls -la test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 141
🌐 Web query:
Docker ps filter name exact match vs partial match documentation
💡 Result:
The docker ps (or docker container ls) command with --filter name=VALUE (or -f name=VALUE) performs a partial substring match by default, not an exact match. This is explicitly stated in the official Docker documentation. For partial match: - docker ps --filter "name=nostalgic" matches containers like i_am_nostalgic, nostalgic_stallman, and nostalgic_shockley. For exact match: - Use regex anchors: docker ps --filter "name=^exactname$" to match only containers named precisely "exactname". The ^ anchors to the start and $ to the end of the name string. This behavior is consistent across Docker CLI reference pages and manpages. Note that container names are unique, but the filter matches substrings unless regex is used. There is no built-in exact-match flag without regex; workarounds like piping to grep exist but are not native to the filter.
Citations:
- 1: https://docs.docker.com/reference/cli/docker/container/ps/
- 2: https://docs.docker.com/engine/reference/commandline/ps
- 3: https://docs.docker.com/reference/cli/docker/container/ls
- 4: https://stackoverflow.com/questions/48767760/how-to-make-docker-container-ls-f-name-filter-by-exact-name
- 5: https://manpages.ubuntu.com/manpages/jammy/man1/docker-container-ls.1.html
🏁 Script executed:
# Check lines 289-294
sed -n '285,300p' test/e2e/test-double-onboard.shRepository: NVIDIA/NemoClaw
Length of output: 646
🏁 Script executed:
# Find all instances of the problematic docker ps command in the file
rg 'docker ps.*openshell-cluster-nemoclaw' test/e2e/test-double-onboard.sh -nRepository: NVIDIA/NemoClaw
Length of output: 391
Use exact Docker name filtering to prevent nondeterministic container ID selection.
The docker ps -qf "name=openshell-cluster-nemoclaw" filter on lines 277, 289, 320, and 332 performs partial substring matching by default. Combined with | head -1, this can select the wrong container if similarly named containers exist (e.g., "openshell-cluster-nemoclaw-backup"), causing flaky assertions.
Use regex anchors to match the exact container name:
Proposed fix
+gateway_container_id() {
+ docker ps --filter "name=^openshell-cluster-nemoclaw$" --format '{{.ID}}'
+}
+
-GATEWAY_ID_BEFORE=$(docker ps -qf "name=openshell-cluster-nemoclaw" | head -1)
+GATEWAY_ID_BEFORE="$(gateway_container_id)"
@@
-GATEWAY_ID_AFTER=$(docker ps -qf "name=openshell-cluster-nemoclaw" | head -1)
+GATEWAY_ID_AFTER="$(gateway_container_id)"
@@
-GATEWAY_ID_BEFORE3=$(docker ps -qf "name=openshell-cluster-nemoclaw" | head -1)
+GATEWAY_ID_BEFORE3="$(gateway_container_id)"
@@
-GATEWAY_ID_AFTER3=$(docker ps -qf "name=openshell-cluster-nemoclaw" | head -1)
+GATEWAY_ID_AFTER3="$(gateway_container_id)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/test-double-onboard.sh` at line 277, The docker container name
filter uses substring matching which can return the wrong container; update the
docker ps calls that use docker ps -qf "name=openshell-cluster-nemoclaw" (e.g.,
the assignment to GATEWAY_ID_BEFORE and the other three occurrences) to use an
exact-name regex anchor: docker ps -qf "name=^/openshell-cluster-nemoclaw$" so
the filter matches the exact container name only.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25089704017
|
…e exit code The onboard-repair test relied on NEMOCLAW_POLICY_MODE=invalid causing exit 1 to create resumable state. The product now gracefully falls back to suggested presets instead of failing, so the test never got its interrupted state. Fix: run a successful onboard with NEMOCLAW_POLICY_MODE=skip, then patch the session file to look like it failed at the policies step. This tests the same resume-repair logic without depending on a specific product error behavior.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25090094139
|
Same approach as the onboard-repair fix: run a successful onboard with NEMOCLAW_POLICY_MODE=skip, then patch the session file to simulate a failure at the policies step.
jyaunches
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/test-onboard-resume.sh (1)
189-194: Align the synthetic failed session with production failure fieldsThe patched object at Line 191 is missing
recordedAt, and Lines 192-194 don’t set the failed step’serror/completedAtshape. Adding these fields will better match real failed-session state and reduce fragility.Proposed refactor
const data = JSON.parse(fs.readFileSync(file, "utf8")); +const now = new Date().toISOString(); data.status = "failed"; data.lastCompletedStep = "openclaw"; -data.failure = { step: "policies", message: "simulated policy failure for E2E" }; +data.updatedAt = now; +data.failure = { + step: "policies", + message: "simulated policy failure for E2E", + recordedAt: now, +}; if (data.steps && data.steps.policies) { data.steps.policies.status = "failed"; + data.steps.policies.completedAt = null; + data.steps.policies.error = "simulated policy failure for E2E"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-onboard-resume.sh` around lines 189 - 194, The synthetic failure object is missing production fields: when setting data.status = "failed" and data.failure = { step: "policies", ... } add a recordedAt timestamp (e.g. ISO string) to data.failure, and for the per-step object referenced by data.steps.policies set the full failure shape by adding an error object (with the message and any code/metadata) and a completedAt timestamp in addition to status = "failed" so the synthetic session matches real failed-session structure used by functions that read data.failure and data.steps.policies.test/e2e/test-onboard-repair.sh (1)
169-174: Patch the simulated failure state to match production shapeAt Line 171, the synthetic
failureobject omitsrecordedAt, and Lines 172-174 only flipsteps.policies.status. Mirroring production failure writes more closely will make this test less brittle to session validation/normalization changes.Proposed refactor
-const data = JSON.parse(fs.readFileSync(file, "utf8")); +const data = JSON.parse(fs.readFileSync(file, "utf8")); +const now = new Date().toISOString(); data.status = "failed"; data.lastCompletedStep = "openclaw"; -data.failure = { step: "policies", message: "simulated policy failure for E2E" }; +data.updatedAt = now; +data.failure = { + step: "policies", + message: "simulated policy failure for E2E", + recordedAt: now, +}; if (data.steps && data.steps.policies) { data.steps.policies.status = "failed"; + data.steps.policies.completedAt = null; + data.steps.policies.error = "simulated policy failure for E2E"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-onboard-repair.sh` around lines 169 - 174, The synthetic failure state in the test mutates data.status, data.lastCompletedStep and data.failure but omits production fields like recordedAt and may not mirror how steps are written; update the test so data.failure includes recordedAt (e.g., a timestamp string) alongside step and message, and ensure the steps mutation mirrors production shape by either setting data.steps.policies = { status: "failed", recordedAt: <timestamp> } or adding recordedAt to data.steps.policies if it exists; keep mutations on data.status and data.lastCompletedStep as-is so the failure shape matches production validation/normalization.
🤖 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-onboard-repair.sh`:
- Around line 164-180: The current session-patching block runs the inline node
command but always calls pass "Session file patched..." even if the node command
fails, which hides failures; update the block that uses the inline node -e
script and SESSION_FILE so you check the node command's exit status (or use set
-e) and call fail "Session file patching failed" on error instead of pass, and
also verify the session file exists and is writable after the patch before
calling pass; reference the node -e invocation, SESSION_FILE variable, and the
pass/fail helper calls to locate and update the logic.
In `@test/e2e/test-onboard-resume.sh`:
- Around line 177-197: The test currently continues after a missing session file
and always prints "Session file patched" even if the node patch step fails;
update the script so the SESSION_FILE existence check halts the test on failure
(call fail/exit when the file is missing instead of merely logging), and run the
node patch step guarded—detect the node command exit status and call fail/exit
if it fails before emitting the pass "Session file patched to simulate
interrupted state"; reference the SESSION_FILE variable and the node -e patch
invocation to locate and update the checks and pass/fail calls.
---
Nitpick comments:
In `@test/e2e/test-onboard-repair.sh`:
- Around line 169-174: The synthetic failure state in the test mutates
data.status, data.lastCompletedStep and data.failure but omits production fields
like recordedAt and may not mirror how steps are written; update the test so
data.failure includes recordedAt (e.g., a timestamp string) alongside step and
message, and ensure the steps mutation mirrors production shape by either
setting data.steps.policies = { status: "failed", recordedAt: <timestamp> } or
adding recordedAt to data.steps.policies if it exists; keep mutations on
data.status and data.lastCompletedStep as-is so the failure shape matches
production validation/normalization.
In `@test/e2e/test-onboard-resume.sh`:
- Around line 189-194: The synthetic failure object is missing production
fields: when setting data.status = "failed" and data.failure = { step:
"policies", ... } add a recordedAt timestamp (e.g. ISO string) to data.failure,
and for the per-step object referenced by data.steps.policies set the full
failure shape by adding an error object (with the message and any code/metadata)
and a completedAt timestamp in addition to status = "failed" so the synthetic
session matches real failed-session structure used by functions that read
data.failure and data.steps.policies.
🪄 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: a8281d28-ebd4-4fb3-98d0-054da1d81d0a
📒 Files selected for processing (2)
test/e2e/test-onboard-repair.shtest/e2e/test-onboard-resume.sh
cv
approved these changes
Apr 29, 2026
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
…IDIA#2656) ## Problem Three newly-wired nightly E2E tests fail due to stale assertions: 1. **`double-onboard-e2e`** — grepped for `"Reusing existing NemoClaw gateway"` log message that no longer exists 2. **`onboard-repair-e2e`** — relied on `NEMOCLAW_POLICY_MODE=invalid` causing exit 1; product now gracefully falls back 3. **`onboard-resume-e2e`** — same as onboard-repair ## Fixes ### double-onboard: container ID instead of log grep Replace the fragile log-text grep with a Docker container ID comparison. Capture the gateway container ID before and after each onboard, verify it's the same container. ### onboard-repair & onboard-resume: simulate interrupted state Instead of relying on an invalid policy mode to create a failure, run a successful onboard with `NEMOCLAW_POLICY_MODE=skip`, then patch the session file to simulate a failure at the policies step. This creates the exact resumable state the test needs without depending on specific product error behavior. ## Philosophy All three fixes follow the same principle: **assert on observable state, not log text or exit codes that drift as the product evolves.** <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Enhanced end-to-end testing with improved gateway reuse validation that verifies container identity, replacing log message parsing for more reliable and accurate test results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
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.
Problem
Three newly-wired nightly E2E tests fail due to stale assertions:
double-onboard-e2e— grepped for"Reusing existing NemoClaw gateway"log message that no longer existsonboard-repair-e2e— relied onNEMOCLAW_POLICY_MODE=invalidcausing exit 1; product now gracefully falls backonboard-resume-e2e— same as onboard-repairFixes
double-onboard: container ID instead of log grep
Replace the fragile log-text grep with a Docker container ID comparison. Capture the gateway container ID before and after each onboard, verify it's the same container.
onboard-repair & onboard-resume: simulate interrupted state
Instead of relying on an invalid policy mode to create a failure, run a successful onboard with
NEMOCLAW_POLICY_MODE=skip, then patch the session file to simulate a failure at the policies step. This creates the exact resumable state the test needs without depending on specific product error behavior.Philosophy
All three fixes follow the same principle: assert on observable state, not log text or exit codes that drift as the product evolves.
Summary by CodeRabbit