refactor(e2e): complete cleanup resource tracking - #6724
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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:
📝 WalkthroughWalkthroughThis PR adds typed E2E cleanup resource tracking, command availability probing, idempotent sandbox deletion, stricter cleanup result handling, and migrates live tests from monolithic cleanup callbacks to tracked gateway, forward, sandbox, and disposable resources. ChangesE2E cleanup lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/live/rebuild-hermes.test.ts (1)
147-173: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExport the sandbox name for the pre-clean shell.
Lines 158-159 expand
$SANDBOX_NAME, but the environment only providesNEMOCLAW_SANDBOX_NAME. Pre-clean therefore skips the intended sandbox, allowing stale resources to contaminate the run.Proposed fix
env: testEnv(apiKey, { + SANDBOX_NAME, DISCORD_PROVIDER: `${SANDBOX_NAME}-discord-bridge`, OLD_BASE_TAG, }),🤖 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/live/rebuild-hermes.test.ts` around lines 147 - 173, Update bestEffortPrecleanHermesResources so the pre-clean shell receives SANDBOX_NAME mapped from the existing NEMOCLAW_SANDBOX_NAME value in testEnv. Preserve the command references to SANDBOX_NAME and ensure the exported environment variable targets the intended sandbox.test/e2e/live/double-onboard.test.ts (1)
220-247: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNew
exitCodeassertion doesn't exercise a real success/failure claim.The script uses
set +eand every fallible step is guarded with|| true, ending in an unconditionalexit 0.stop.exitCodewill always be0regardless of whether any internal step actually succeeded, soexpect(stop.exitCode, ...).toBe(0)only verifies thatbashitself could be spawned — it never fails on a real teardown problem. This reads as a meaningful assertion but doesn't test what it implies.Either drop the assertion (it adds no signal), or make the script propagate a real terminal status so the check is meaningful.
As per path instructions for test files, "Flag ... conditionals that make a test pass without exercising its claim."
🤖 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/live/double-onboard.test.ts` around lines 220 - 247, Make the teardown result assertion in stopGatewayRuntime meaningful: either remove the stop.exitCode expectation, or change the script to preserve and return a real failure status for teardown operations instead of using unconditional exit 0 with broadly suppressed errors. Keep intentionally best-effort cleanup behavior only where appropriate, while ensuring the chosen outcome matches what the test claims to verify.Source: Path instructions
🧹 Nitpick comments (3)
test/e2e/live/jetson-nvmap-gpu.test.ts (1)
119-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated "probe availability, then cleanup" boilerplate across gateway/sandbox/nemoclaw guards.
The same three-line pattern (probe with
isCommandAvailable, return early if false, else delegate to the real cleanup) is duplicated three times with only the probed command and delegate call varying. Extracting a small helper (e.g.,guardedCleanup(probeCommand, probeOptions, action)) would remove ~30 lines of repetition and reduce the chance of the drift seen with theOPENSHELL_BINinconsistency above.🤖 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/live/jetson-nvmap-gpu.test.ts` around lines 119 - 181, Extract the repeated availability-check and early-return logic into a small guarded cleanup helper near the cleanup registrations, accepting the probe command, probe options, and cleanup action. Update the gateway, OpenShell sandbox, and nemoclaw sandbox registrations to use this helper while preserving each existing command source and cleanup delegate, including the current OPENSHELL_BIN behavior.test/e2e/live/credential-sanitization.test.ts (1)
81-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated sandbox-absence detection logic; regex has already drifted.
This re-implements the same absence-pattern check as
HostCliClient.cleanupSandbox(test/e2e/fixtures/clients/host.ts), but uses/iuhere versus/ithere. Since this test needs anode [CLI_ENTRYPOINT, ...]invocation instead ofhost.nemoclaw(...), it can't callhost.cleanupSandboxdirectly, but the duplicated regex is exactly the kind of pattern the PR's "distinguishing permitted absence from real failures" objective is meant to centralize.Consider extracting a shared
isSandboxAbsentError(text: string): booleanhelper (e.g., alongsideresultText/assertExitZeroincommand.ts) and reusing it from bothHostCliClient.cleanupSandboxand this function.♻️ Proposed shared helper
+// test/e2e/fixtures/clients/command.ts +const SANDBOX_ABSENT_PATTERN = + /Sandbox '.+' does not exist|Run 'nemoclaw onboard' to create one|sandbox .* not found|no such sandbox/i; + +export function isSandboxAbsentError(text: string): boolean { + return SANDBOX_ABSENT_PATTERN.test(text); +}async function cleanupCredentialSanitizationNemoClawSandbox( host: HostCliClient, home: string, ): Promise<void> { const result = await host.command("node", [CLI_ENTRYPOINT, SANDBOX_NAME, "destroy", "--yes"], { artifactName: "cleanup-nemoclaw-destroy-credential-sanitization", env: testEnv(home), timeoutMs: 120_000, }); - if ( - result.exitCode === 0 || - /Sandbox '.+' does not exist|Run 'nemoclaw onboard' to create one|sandbox .* not found|no such sandbox/iu.test( - resultText(result), - ) - ) { + if (result.exitCode === 0 || isSandboxAbsentError(resultText(result))) { return; } assertExitZero(result, `cleanup credential sanitization sandbox ${SANDBOX_NAME}`); }🤖 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/live/credential-sanitization.test.ts` around lines 81 - 99, Centralize sandbox-absence detection in a shared isSandboxAbsentError(text: string) helper alongside resultText/assertExitZero in command.ts, preserving the existing absence patterns and case-insensitive behavior. Update HostCliClient.cleanupSandbox and cleanupCredentialSanitizationNemoClawSandbox to call this helper instead of maintaining separate regular expressions, while keeping their existing command invocation and failure handling unchanged.test/e2e/live/gpu-e2e-helpers.ts (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider consolidating the repeated
preCleanBestEfforthelper.This file's exported
preCleanBestEffort(label, run)duplicates near-identical, differently-shaped local helpers reintroduced in several other test/helper files (some take onlyrun, nolabel). Since the PR's stated goal is reducing duplicatedbestEfforthelpers, extracting one shared implementation (e.g., in a common fixtures module) would prevent further signature drift.Also applies to: 118-140
🤖 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/live/gpu-e2e-helpers.ts` around lines 57 - 60, Consolidate the exported preCleanBestEffort helper with the duplicated best-effort helpers used across the test and fixture files by moving one shared implementation into the common fixtures module. Standardize callers on a single signature, adapting label handling as needed, and remove the local duplicate definitions while preserving their existing cleanup behavior.
🤖 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/live/bedrock-runtime-compatible-anthropic.test.ts`:
- Around line 1344-1347: Reverse the cleanup registration order around
stopBedrockAdapter and the fake endpoint callback so cleanup LIFO executes
stopBedrockAdapter first and mock.close() afterward. Keep both existing cleanup
actions and their conditions unchanged.
In `@test/e2e/live/common-egress-agent.test.ts`:
- Around line 241-252: Update the cleanup callback around cleanupSandbox to
capture its validated ShellProbeResult instead of discarding the return value.
Populate summary.openshellDelete from that result, preserving the actual
exitCode, missingSandboxTolerated flag, and outputTail so already-absent
sandboxes are recorded as tolerated rather than as successful deletions.
In `@test/e2e/live/concurrent-gateway-ports.test.ts`:
- Around line 332-346: The cleanup trackers around gateway destruction and port
forwarding currently use HostCliClient, which invokes literal openshell instead
of the configured executable. Update these trackers to use a
SandboxClient-backed adapter, or update HostCliClient to resolve OPENSHELL_BIN
consistently with the surrounding setup and pre-clean flow, while preserving the
existing cleanup behavior.
In `@test/e2e/live/diagnostics.test.ts`:
- Around line 152-173: Update the teardown registrations around
cleanup.trackGateway and the OpenShell sandbox-delete cleanup.trackDisposable to
pass redactionValues: [apiKey], matching cleanup.trackSandbox. Ensure all three
cleanup trackers using cleanupEnv consistently redact the API key.
In `@test/e2e/live/gpu-double-onboard.test.ts`:
- Around line 219-235: Update the gateway cleanup flow around
cleanupRegistry.trackGateway and HostCliClient.cleanupGatewayRegistration to use
the configured OPENSHELL_BIN executable for both isCommandAvailable probing and
cleanup invocation, rather than the literal “openshell” command. Thread the
configured executable through gatewayCleanupOptions or the cleanup client so
probing and execution use the same path and cleanup still runs when the binary
is outside PATH.
In `@test/e2e/live/hermes-shields-config.test.ts`:
- Around line 179-182: Update the cleanup callback registered with
cleanupRegistry.trackDisposable to wrap artifacts.writeJson in a try/finally
block, ensuring fake.close() is always awaited even when writing
fake-openai-compatible-requests.json fails.
In `@test/e2e/live/hermes-slack-e2e-helpers.ts`:
- Around line 243-248: Update the pre-install gateway registrations using
cleanup.trackGateway so cleanupGatewayRegistration treats unavailable OpenShell
as an allowed absence rather than a failure. Apply the same availability-aware
cleanup configuration to the Hermes Slack flow and the matching Hermes Discord
and onboard-repair registration paths, while preserving cleanup failures when an
existing gateway cannot be removed.
In `@test/e2e/live/jetson-nvmap-gpu.test.ts`:
- Around line 119-141: Update the gateway cleanup probe inside
cleanupGatewayRegistration to use the same process.env.OPENSHELL_BIN ??
"openshell" command selection as the sandbox-delete guard, while preserving the
existing cleanup flow and options.
In `@test/e2e/live/messaging-compatible-endpoint-helpers.ts`:
- Around line 41-55: Update the PID cleanup logic in the messaging-compatible
endpoint helper to validate that the PID belongs to the OpenShell gateway using
/proc/$pid/cmdline or an equivalent gateway-specific marker before any kill or
kill -9 operation. Perform this ownership check for both termination attempts
and preserve the existing wait and exit behavior for an owned process.
In `@test/e2e/live/messaging-providers-helpers.ts`:
- Around line 618-628: Update the cleanup callback around the Docker removal in
cleanup to wrap the removal attempt and exit-code validation in try/finally, and
move fs.rmSync(dir, { recursive: true, force: true }) into the finally block so
the fake API scratch directory is always removed even when Docker cleanup
validation throws.
In `@test/e2e/live/openshell-gateway-upgrade.test.ts`:
- Around line 392-394: Update preCleanUpgradeGateway to capture and assert the
result returned by upgradeGatewayCleanupScript, matching the validation already
used in the teardown path at Lines 743-747. Ensure a non-zero cleanup result
fails before the upgrade retry proceeds.
In `@test/e2e/live/sandbox-survival.test.ts`:
- Around line 188-210: Update the cleanupSandbox call in the trackSandbox setup
to pass env: buildAvailabilityProbeEnv() alongside sandboxCleanupOptions. Keep
the availability probe and existing cleanup options unchanged so sandbox
deletion uses the same fixture PATH.
In `@test/e2e/live/snapshot-commands.test.ts`:
- Around line 62-68: Update the pre-clean callback in the snapshot command test
to invoke the configured NemoClaw executable through the existing HostCliClient
configuration rather than calling host.command directly. Preserve the current
destroy arguments, environment, artifactName, timeoutMs, and bestEffortPreclean
behavior.
In `@test/e2e/live/upgrade-stale-sandbox.test.ts`:
- Around line 61-74: Register the “remove stale OpenClaw test image” disposable
before the sandbox cleanup registrations so LIFO cleanup runs cleanupOldImage
only after the OpenShell deletion and sandbox destruction complete. Preserve the
existing artifact, environment, timeout, and precleanStaleSandbox behavior.
---
Outside diff comments:
In `@test/e2e/live/double-onboard.test.ts`:
- Around line 220-247: Make the teardown result assertion in stopGatewayRuntime
meaningful: either remove the stop.exitCode expectation, or change the script to
preserve and return a real failure status for teardown operations instead of
using unconditional exit 0 with broadly suppressed errors. Keep intentionally
best-effort cleanup behavior only where appropriate, while ensuring the chosen
outcome matches what the test claims to verify.
In `@test/e2e/live/rebuild-hermes.test.ts`:
- Around line 147-173: Update bestEffortPrecleanHermesResources so the pre-clean
shell receives SANDBOX_NAME mapped from the existing NEMOCLAW_SANDBOX_NAME value
in testEnv. Preserve the command references to SANDBOX_NAME and ensure the
exported environment variable targets the intended sandbox.
---
Nitpick comments:
In `@test/e2e/live/credential-sanitization.test.ts`:
- Around line 81-99: Centralize sandbox-absence detection in a shared
isSandboxAbsentError(text: string) helper alongside resultText/assertExitZero in
command.ts, preserving the existing absence patterns and case-insensitive
behavior. Update HostCliClient.cleanupSandbox and
cleanupCredentialSanitizationNemoClawSandbox to call this helper instead of
maintaining separate regular expressions, while keeping their existing command
invocation and failure handling unchanged.
In `@test/e2e/live/gpu-e2e-helpers.ts`:
- Around line 57-60: Consolidate the exported preCleanBestEffort helper with the
duplicated best-effort helpers used across the test and fixture files by moving
one shared implementation into the common fixtures module. Standardize callers
on a single signature, adapting label handling as needed, and remove the local
duplicate definitions while preserving their existing cleanup behavior.
In `@test/e2e/live/jetson-nvmap-gpu.test.ts`:
- Around line 119-181: Extract the repeated availability-check and early-return
logic into a small guarded cleanup helper near the cleanup registrations,
accepting the probe command, probe options, and cleanup action. Update the
gateway, OpenShell sandbox, and nemoclaw sandbox registrations to use this
helper while preserving each existing command source and cleanup delegate,
including the current OPENSHELL_BIN behavior.
🪄 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: 08b13b04-8445-4162-9f21-2053c6dc05fc
📒 Files selected for processing (76)
test/e2e/fixtures/cleanup.tstest/e2e/fixtures/clients/host.tstest/e2e/fixtures/clients/sandbox.tstest/e2e/live/agent-turn-latency-helpers.tstest/e2e/live/agent-turn-latency.test.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/brave-search-helpers.tstest/e2e/live/brave-search.test.tstest/e2e/live/channels-add-remove.test.tstest/e2e/live/channels-stop-start-helpers.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/cloud-onboard.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/concurrent-gateway-ports.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/cron-preflight-inference-local.test.tstest/e2e/live/device-auth-health-helpers.tstest/e2e/live/device-auth-health.test.tstest/e2e/live/diagnostics.test.tstest/e2e/live/double-onboard.test.tstest/e2e/live/full-e2e.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/gpu-e2e-helpers.tstest/e2e/live/gpu-e2e.test.tstest/e2e/live/hermes-discord.test.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-inference-switch-helpers.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-shields-config.test.tstest/e2e/live/hermes-slack-e2e-helpers.tstest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-4462-scope-upgrade-approval.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/kimi-inference-compat-helpers.tstest/e2e/live/kimi-inference-compat.test.tstest/e2e/live/mcp-bridge.test.tstest/e2e/live/messaging-compatible-endpoint-helpers.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/messaging-providers-helpers.tstest/e2e/live/messaging-providers.test.tstest/e2e/live/model-router-provider-routed-inference.test.tstest/e2e/live/network-policy.test.tstest/e2e/live/ollama-auth-proxy.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/onboard-resume.test.tstest/e2e/live/openclaw-discord-pairing.test.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/live/openclaw-pairing-helpers.tstest/e2e/live/openclaw-plugin-runtime-exdev.test.tstest/e2e/live/openclaw-skill-cli.test.tstest/e2e/live/openclaw-slack-pairing.test.tstest/e2e/live/openshell-gateway-upgrade-helpers.tstest/e2e/live/openshell-gateway-upgrade.test.tstest/e2e/live/overlayfs-autofix.test.tstest/e2e/live/phase6-messaging-helpers.tstest/e2e/live/rebuild-hermes.test.tstest/e2e/live/rebuild-openclaw.test.tstest/e2e/live/sandbox-rebuild.test.tstest/e2e/live/sandbox-rlimits-connect.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/skill-agent.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/spark-install.test.tstest/e2e/live/state-backup-restore.test.tstest/e2e/live/telegram-injection.test.tstest/e2e/live/token-rotation.test.tstest/e2e/live/tunnel-lifecycle-helpers.tstest/e2e/live/upgrade-stale-sandbox-helpers.tstest/e2e/live/upgrade-stale-sandbox.test.tstest/e2e/support/e2e-cleanup-resources.test.tstest/e2e/support/e2e-clients.test.ts
15 tasks
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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/live/overlayfs-autofix-cleanup.ts`:
- Around line 25-71: Split cleanupOverlayArtifacts into independently tracked
disposable steps for gateway container removal, patched-image listing/removal,
and onboard.lock deletion. Update the surrounding trackDisposable registration
so CleanupRegistry.runAll() isolates failures and continues executing later
cleanup steps; preserve the existing absence handling, assertions, command
options, and lock-file removal behavior.
In `@test/e2e/support/overlayfs-autofix-outcome.test.ts`:
- Around line 108-136: Restore the fs.rmSync spy created in the test “tears down
sandbox resources before gateway artifacts in LIFO order (`#6352`)” after
assertions complete, using the repository’s established mock-restoration pattern
so the stub cannot affect later tests.
🪄 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: ed80f5d9-02a9-4e62-ab59-a60d0476ddb1
📒 Files selected for processing (25)
test/e2e/fixtures/cleanup-resources.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-shields-config.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/mcp-bridge.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/onboard-resume.test.tstest/e2e/live/overlayfs-autofix-cleanup.tstest/e2e/live/overlayfs-autofix.test.tstest/e2e/live/rebuild-hermes.test.tstest/e2e/live/rebuild-openclaw.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/token-rotation.test.tstest/e2e/mock-parity.jsontest/e2e/support/e2e-cleanup-resources.test.tstest/e2e/support/overlayfs-autofix-outcome.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- test/e2e/live/onboard-repair.test.ts
- test/e2e/live/hermes-shields-config.test.ts
- test/e2e/live/sessions-agents-cli.test.ts
- test/e2e/live/jetson-nvmap-gpu.test.ts
- test/e2e/live/token-rotation.test.ts
- test/e2e/live/common-egress-agent.test.ts
- test/e2e/live/gpu-double-onboard.test.ts
- test/e2e/live/cloud-inference.test.ts
- test/e2e/live/shields-config.test.ts
- test/e2e/live/rebuild-hermes.test.ts
- test/e2e/live/rebuild-openclaw.test.ts
- test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
- test/e2e/live/onboard-resume.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
cv
pushed a commit
that referenced
this pull request
Jul 14, 2026
<!-- markdownlint-disable MD041 --> ## Summary Addresses the five release/nightly E2E blockers according to the evidence each failure left behind. The OpenShell version-pin fixture now supports the installer's archive validation calls, the Shields target restores lockdown after its intentional failure assertion, MCP patch failures preserve their real sandbox phase, and the Hermes rebuild target emits secret-safe phase and host-resource heartbeats. The Hermes failure was not the fixture's 45-minute rebuild timeout. GitHub ended the job after about 50 minutes because the hosted runner lost communication; the rebuild command began later in the scenario and could not yet have reached its own timeout. No final logs or artifacts survived, so the underlying memory, disk, CPU, network, or runner-termination trigger cannot be recovered from that run. Follow-up to #6744 and #6724. The source nightly had seven red jobs. A same-SHA rerun cleared the two accepted flakes, `hermes-shields-config` and `gateway-guard-recovery`, leaving five release blockers: `openshell-version-pin`, `shields-config`, `mcp-bridge`, `rebuild-hermes`, and `channels-stop-start (hermes)`. This PR fixes the two reproducible fixture defects, repairs the MCP phase evidence, and instruments the Hermes rebuild runner loss. `mcp-bridge` failed when Docker `stop` exceeded its 30-second client timeout after a successful image build, while `channels-stop-start (hermes)` was externally canceled before any assertion or artifact; neither has evidence for a causal retry or timeout change, so both still require a fresh green candidate run or a reproducible product defect. ## Changes - Model `tar -tzf`, `tar -tvzf`, and extraction in the hermetic OpenShell version-pin fixture so the installer can validate fake release archives before extracting them. - Restore Shields lockdown after the duplicate-down rejection assertion and register a strict relock cleanup that reports a failed command or missing lockdown confirmation before continuing destruction. - Parse modern `NAME CREATED PHASE` OpenShell rows with the shared sandbox-list parser so MCP Docker-patch diagnostics report `Provisioning` instead of the creation date, while retaining the canonical terminal `Evicted` phase. - Emit a one-minute Hermes rebuild heartbeat from setup through cleanup with the active phase, child-output age, memory, process RSS, workspace disk, and load average. The output observer records timestamps only and never forwards command output or credentials. - Map the changed OpenShell and Hermes live targets to their fast PR coverage in the mock/live parity manifest. - Keep installer recovery behavior, the 30-second destructive Docker-operation timeout, Shields behavior, sandbox destruction, and Hermes rebuild behavior unchanged; the 90-minute workflow limit and 45-minute rebuild-command limit are unchanged. - Keep the underlying `mcp-bridge` Docker timeout and `channels-stop-start (hermes)` cancellation as explicit green-evidence blockers instead of masking them with retries. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: all changes are internal E2E fixture, diagnostic, and parity corrections; no user-facing CLI, policy, lifecycle, installer, or Hermes contract changes. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: the Shields security E2E target and secret-safe Hermes diagnostics changed; review is pending on the current revision and no waiver is requested. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: no waiver is requested; required PR CI is pending. ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — OpenShell version-pin live fixture (3/3); focused archive-safety installer tests (9/9); MCP Docker-patch and shared phase parsing (29/29); Hermes progress and cleanup tests (15/15); mock/live parity guard; Vitest project membership; source-shape check; CLI typecheck and build; Shields and Hermes live target collection. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable to narrow live-target fixture and diagnostic changes. An exploratory local E2E-support run passed 979 tests and hit 15 environment-specific failures (macOS Bash behavior, Node 26 warning output, and nested-run timeouts); required Linux CI remains authoritative. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run 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) --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved sandbox status detection across modern list output formats, including evicted sandboxes. - Improved cleanup behavior so shield restoration failures are reported while subsequent cleanup continues. - Strengthened shield-state handling during test teardown. - **User Experience** - Added clearer progress and activity reporting during lengthy Hermes rebuild operations. - **Tests** - Expanded coverage for installer version pinning, sandbox lifecycle states, cleanup ordering, and rebuild progress reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
area: ci
This was referenced Jul 31, 2026
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
2 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
Complete the remaining live-E2E cleanup resource migration so ordinary teardown is strict, idempotent, ordered, and visible in
cleanup.json. Standard sandbox, gateway, forward, server, process, image, and temporary-state cleanup now uses typed trackers or strict disposables; destructive pre-clean and negative-path verification remain explicit. This changes test infrastructure only, not NemoClaw product behavior.Related Issue
Closes #6352
Closes #6346
Changes
SandboxClientwith strict already-absent-aware OpenShell sandbox cleanup, add a tested command-availability probe for partial installer setup, and narrow each tracker to the client method it consumes.trackSandbox,trackGateway,trackForward, andtrackDisposablewhile preserving LIFO dependencies, artifact names, redactions, timeouts, keep policies, and owned gateway-runtime cleanup.cleanup.addcalls123 -> 53; exact genericbestEfforthelpers28 -> 0; current typed adoption is 41 sandbox, 39 gateway, 8 forward, and 124 disposable registrations. The remaining raw callbacks are bespoke lifecycle, recovery, diagnostic, or negative-proof work rather than repeated standard sandbox/gateway/forward teardown.36 -> 0, local exit-zero helpers10 -> 0, manualtarget.jsonwriters61 -> 0, manualtarget-result.jsonwriters34 -> 0, and redundant per-file live gates66 -> 0.The new command-availability client method is required by installer-fidelity live tests that register cleanup before
install.shmay acquire the CLI. Registering only after a successful install would miss partially-created resources;test/e2e/support/e2e-clients.test.tsprotects available, missing, failure, and exact-option behavior.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project e2e-support(115 files, 978 tests); exact mock/live parity for all 56 changed live specs; conditional count375 -> 373with zero per-file increases;npm run typecheck:cli; exact 8-project membership; all live specs transform/listnpm test(1,450 files; 16,396 tests; 3 files / 40 tests skipped)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests
Refactor