Skip to content

test(e2e): wire sandbox operations into Vitest - #5926

Merged
cv merged 17 commits into
mainfrom
codex/e2e-sandbox-operations-parity
Jun 28, 2026
Merged

test(e2e): wire sandbox operations into Vitest#5926
cv merged 17 commits into
mainfrom
codex/e2e-sandbox-operations-parity

Conversation

@cv

@cv cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Makes the existing Vitest sandbox-operations replacement reachable from the cutover workflow. The new default-enabled, selectively dispatchable job preserves the legacy Ubuntu/Docker/OpenShell boundary while using Vitest's structured artifacts and cleanup reporting instead of the legacy shell log streamer; it also fixes the previously unexercised replacement to configure hosted-compatible inference rather than treating that credential as a Build API key.

Related Issue

Refs #5919

Changes

  • add sandbox-operations-vitest to the Vitest E2E workflow and PR result aggregation
  • run the existing live replacement with isolated Docker auth, the compiled CLI, OpenShell, and hosted inference
  • configure both sandbox onboard operations through the shared hosted-inference abstraction
  • add a focused workflow-boundary test for inventory, default dispatch, and both selective-dispatch inputs

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: CI-only E2E reachability; no user-facing behavior or interface changes
  • 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:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Targeted evidence: npx vitest run --project e2e-vitest-support (54 files, 386 tests); npm run typecheck:cli; source-shape and test-size ratchets; normal commit and push hooks.


Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • Tests

    • Added a new live Vitest E2E run for sandbox operations and included it in PR E2E scenario status reporting.
    • Expanded workflow-boundary validation to block unsafe Docker auth and environment/config scoping, enforce required secret placement, and verify consistent artifact upload and Docker cleanup behavior.
    • Refined live sandbox assertions (including agent exit behavior), improved gateway recovery reporting, and updated scenario outputs.
  • Chores

    • Updated sandbox onboarding to use a hosted inference configuration with consistent per-sandbox environment overrides and improved API-key redaction handling; streamlined cleanup/onboarding flow for reliability.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Jun 28, 2026
@cv cv added chore Build, CI, dependency, or tooling maintenance area: e2e End-to-end tests, nightly failures, or validation infrastructure area: ci CI workflows, checks, release automation, or GitHub Actions v0.0.70 labels Jun 28, 2026
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a new sandbox-operations-vitest workflow job, validates its workflow boundary, updates the live sandbox scenario, and expands host client cleanup handling.

Changes

Sandbox-operations Vitest job and test updates

Layer / File(s) Summary
Vitest job and reporting
.github/workflows/e2e-vitest-scenarios.yaml
Defines the new sandbox-operations-vitest job with checkout, Node setup, dependency install, OpenShell setup, isolated Docker auth, live Vitest execution, artifact upload, and cleanup; adds the job to report-to-pr needs.
Workflow boundary validation
tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts
Defines workflow loading and validation for sandbox-operations-vitest, including job shape, step ordering, action pinning, Docker auth handling, live test command checks, artifact upload checks, and cleanup checks.
Boundary wiring and tests
tools/e2e-scenarios/workflow-boundary.mts, test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
Imports the sandbox-operations workflow validator into the shared workflow boundary check and adds Vitest coverage for selector behavior and validation failures.
Live sandbox scenario
test/e2e-scenario/live/sandbox-operations.test.ts
Threads hosted inference config through onboarding, refactors gateway cleanup and recovery handling, changes the TC-SBX-02b assertions, and records the gateway recovery outcome in scenario-result.json.
Host client cleanup support
test/e2e-scenario/fixtures/clients/host.ts, test/e2e-scenario/support-tests/e2e-clients.test.ts
Adds gateway-registration cleanup fallback handling in the host client and expands cleanup-oriented command runner and host client tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5607: Also extends the Vitest workflow boundary validation wiring in tools/e2e-scenarios/workflow-boundary.mts for a new free-standing job.
  • NVIDIA/NemoClaw#5610: Also adds a free-standing Vitest job and wires it into the shared workflow boundary validation flow.
  • NVIDIA/NemoClaw#5891: Also derives hosted inference config with requireHostedInferenceConfig(secrets) and threads hosted.env into live onboarding.

Suggested labels

integration: openclaw, bug-fix

Suggested reviewers

  • ericksoa
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: wiring sandbox operations into the Vitest E2E workflow.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/e2e-sandbox-operations-parity

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/e2e-sandbox-op... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main codex/e2e-sandbox-op... ca68bbd +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the codex/e2e-sandbox-op... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main codex/e2e-sandbox-op... ca68bbd +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 78%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 28, 2026 21:51 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: sandbox-operations-vitest
Optional E2E: sandbox-survival-vitest, network-policy-vitest

Dispatch hint: sandbox-operations-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • sandbox-operations-vitest (high): This is the directly affected live E2E job being added and modified. It validates the real sandbox lifecycle, hosted inference, OpenShell gateway cleanup/recovery, multi-sandbox metadata, logs/status/list/destroy commands, PTY/tmux behavior, and sandbox network isolation through the same workflow path changed by the PR.

Optional E2E

  • sandbox-survival-vitest (medium): Adjacent confidence for gateway restart/state survival and live inference after restart. Useful because this PR changes sandbox operations gateway cleanup/recovery behavior, but it does not change product runtime code for survival directly.
  • network-policy-vitest (medium): Optional adjacent coverage for network/security isolation because the modified sandbox-operations scenario asserts sandbox-to-sandbox isolation and runs under an open policy tier. Product network policy assets are not changed, so this is not merge-blocking.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: sandbox-operations-vitest

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: sandbox-operations-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-operations-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • sandbox-operations-vitest: Focused free-standing Vitest job wired for changed live test test/e2e-scenario/live/sandbox-operations.test.ts.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-operations-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/fixtures/clients/host.ts
  • test/e2e-scenario/live/sandbox-operations.test.ts
  • test/e2e-scenario/support-tests/e2e-clients.test.ts
  • test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts
  • tools/e2e-scenarios/workflow-boundary.mts

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28313799712
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ❌ failure

Failed jobs: sandbox-operations-vitest. Check run artifacts for logs.

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Default-enabled sandbox-operations job still relies on trusted manual dispatch refs.
Open items: 0 required · 1 warning · 0 suggestions · 6 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Default-enabled sandbox-operations job still relies on trusted manual dispatch refs in .github/workflows/e2e-vitest-scenarios.yaml:4153
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Acceptance clause
  • PRA-T6 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify security .github/workflows/e2e-vitest-scenarios.yaml:4153 Add an explicit invariant near the job and in the sandbox-operations workflow-boundary validator documentation/tests that this default-enabled lane is intended only for trusted refs/manual dispatch. If that is not the intended policy, make `sandbox-operations-vitest` explicit-only or restructure the lane so checked-out test code does not overlap with both the inference secret and authenticated Docker config.
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Default-enabled sandbox-operations job still relies on trusted manual dispatch refs

  • Location: .github/workflows/e2e-vitest-scenarios.yaml:4153
  • Category: security
  • Problem: `sandbox-operations-vitest` runs on default workflow dispatch when both selectors are empty, checks out and builds the selected ref, authenticates Docker Hub into a persistent `DOCKER_CONFIG`, and then runs checked-out Vitest code with `NVIDIA_INFERENCE_API_KEY`. The PR adds useful guardrails for action pinning, checkout credentials, credential scoping, sanitized OpenShell install, and cleanup, but it still does not encode the invariant that this default-enabled, secret-bearing lane is only safe on trusted refs.
  • Impact: If a maintainer manually dispatches this workflow on attacker-controlled or otherwise unreviewed code, that code can execute in the live test step with the hosted inference credential and can read or use the authenticated Docker config before the cleanup step runs.
  • Recommended action: Add an explicit invariant near the job and in the sandbox-operations workflow-boundary validator documentation/tests that this default-enabled lane is intended only for trusted refs/manual dispatch. If that is not the intended policy, make `sandbox-operations-vitest` explicit-only or restructure the lane so checked-out test code does not overlap with both the inference secret and authenticated Docker config.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `.github/workflows/e2e-vitest-scenarios.yaml` around `sandbox-operations-vitest`: confirm the default-selector branch in the job `if`, `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations`, Docker Hub authentication before `Run sandbox operations live test`, and the step-scoped `NVIDIA_INFERENCE_API_KEY`. Then read `test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts` and `tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts` to confirm the existing mutation coverage does not require a trusted-ref/manual-dispatch policy marker.
  • Missing regression test: Add a static workflow-boundary assertion that fails if the trusted-ref/manual-dispatch invariant text or policy marker for `sandbox-operations-vitest` is removed while the job remains default-enabled and secret-bearing. Existing mutation tests reject `pull_request`, write-scoped contents permissions, workspace Docker auth, broad Docker credentials, and broad inference secrets, but they do not cover this invariant.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `.github/workflows/e2e-vitest-scenarios.yaml` around `sandbox-operations-vitest`: confirm the default-selector branch in the job `if`, `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations`, Docker Hub authentication before `Run sandbox operations live test`, and the step-scoped `NVIDIA_INFERENCE_API_KEY`. Then read `test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts` and `tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts` to confirm the existing mutation coverage does not require a trusted-ref/manual-dispatch policy marker.
  • Evidence: The job condition includes `(inputs.jobs == '' && inputs.scenarios == '')`, the workflow is manually dispatchable, the job checks out repository code and runs `npm run build:cli`, writes `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations` to `$GITHUB_ENV`, authenticates Docker Hub, and runs `test/e2e-scenario/live/sandbox-operations.test.ts` with `NVIDIA_INFERENCE_API_KEY`. The new boundary tests cover trigger/permission and credential-scoping drift but not a trusted-ref invariant.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Run or identify a targeted `sandbox-operations-vitest` workflow dispatch that reaches `test/e2e-scenario/live/sandbox-operations.test.ts` and writes `e2e-artifacts/vitest/sandbox-operations/scenario-result.json` containing a `gatewayRecovery` value.. Static support tests cover workflow shape, selector behavior, credential scoping, OpenShell cleanup compatibility, and mutation guards. The changed behavior still crosses GitHub Actions dispatch, Docker authentication, OpenShell installation, the compiled CLI launcher, hosted inference, two real sandbox onboards, gateway/process recovery, network isolation, artifact publication, and cleanup.
  • PRA-T2 Runtime validation — Run or identify selector-path validation for both `scenarios=sandbox-operations` and `jobs=sandbox-operations-vitest` showing the free-standing job is selected without falling back to the matrix `live-scenarios` lane.. Static support tests cover workflow shape, selector behavior, credential scoping, OpenShell cleanup compatibility, and mutation guards. The changed behavior still crosses GitHub Actions dispatch, Docker authentication, OpenShell installation, the compiled CLI launcher, hosted inference, two real sandbox onboards, gateway/process recovery, network isolation, artifact publication, and cleanup.
  • PRA-T3 Runtime validation — Exercise the live lane through Docker Hub authentication plus `if: always()` cleanup and confirm `${RUNNER_TEMP}/docker-config-sandbox-operations` is removed after artifact upload, including a failing-live-test path.. Static support tests cover workflow shape, selector behavior, credential scoping, OpenShell cleanup compatibility, and mutation guards. The changed behavior still crosses GitHub Actions dispatch, Docker authentication, OpenShell installation, the compiled CLI launcher, hosted inference, two real sandbox onboards, gateway/process recovery, network isolation, artifact publication, and cleanup.
  • PRA-T4 Runtime validation — When issue Complete the Vitest E2E cutover with job-variant parity evidence #5919 text/comments are available, map each literal acceptance clause to the new workflow artifacts/contracts or add targeted coverage for any clause not already represented.. Static support tests cover workflow shape, selector behavior, credential scoping, OpenShell cleanup compatibility, and mutation guards. The changed behavior still crosses GitHub Actions dispatch, Docker authentication, OpenShell installation, the compiled CLI launcher, hosted inference, two real sandbox onboards, gateway/process recovery, network isolation, artifact publication, and cleanup.
  • PRA-T5 Acceptance clause — Refs Complete the Vitest E2E cutover with job-variant parity evidence #5919 — add test evidence or identify existing coverage. The deterministic review context did not include linked issue Complete the Vitest E2E cutover with job-variant parity evidence #5919 text or comments (`linkedIssues` was empty), so literal issue clauses could not be extracted or mapped. PR-local behavior claims are represented in the diff by the new workflow job, report aggregation, hosted inference configuration, and workflow-boundary tests.
  • PRA-T6 Acceptance clause — run the existing live replacement with isolated Docker auth, the compiled CLI, OpenShell, and hosted inference — add test evidence or identify existing coverage. The workflow builds and verifies `${{ github.workspace }}/bin/nemoclaw.js`, runs `scripts/install-openshell.sh`, sets Docker auth under `${RUNNER_TEMP}/docker-config-sandbox-operations`, and scopes `NVIDIA_INFERENCE_API_KEY` to the live test step. Full confidence still needs runtime validation because this crosses GitHub Actions dispatch, Docker auth, OpenShell installation, hosted inference, sandbox onboarding, recovery, and cleanup.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Default-enabled sandbox-operations job still relies on trusted manual dispatch refs

  • Location: .github/workflows/e2e-vitest-scenarios.yaml:4153
  • Category: security
  • Problem: `sandbox-operations-vitest` runs on default workflow dispatch when both selectors are empty, checks out and builds the selected ref, authenticates Docker Hub into a persistent `DOCKER_CONFIG`, and then runs checked-out Vitest code with `NVIDIA_INFERENCE_API_KEY`. The PR adds useful guardrails for action pinning, checkout credentials, credential scoping, sanitized OpenShell install, and cleanup, but it still does not encode the invariant that this default-enabled, secret-bearing lane is only safe on trusted refs.
  • Impact: If a maintainer manually dispatches this workflow on attacker-controlled or otherwise unreviewed code, that code can execute in the live test step with the hosted inference credential and can read or use the authenticated Docker config before the cleanup step runs.
  • Recommended action: Add an explicit invariant near the job and in the sandbox-operations workflow-boundary validator documentation/tests that this default-enabled lane is intended only for trusted refs/manual dispatch. If that is not the intended policy, make `sandbox-operations-vitest` explicit-only or restructure the lane so checked-out test code does not overlap with both the inference secret and authenticated Docker config.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `.github/workflows/e2e-vitest-scenarios.yaml` around `sandbox-operations-vitest`: confirm the default-selector branch in the job `if`, `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations`, Docker Hub authentication before `Run sandbox operations live test`, and the step-scoped `NVIDIA_INFERENCE_API_KEY`. Then read `test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts` and `tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts` to confirm the existing mutation coverage does not require a trusted-ref/manual-dispatch policy marker.
  • Missing regression test: Add a static workflow-boundary assertion that fails if the trusted-ref/manual-dispatch invariant text or policy marker for `sandbox-operations-vitest` is removed while the job remains default-enabled and secret-bearing. Existing mutation tests reject `pull_request`, write-scoped contents permissions, workspace Docker auth, broad Docker credentials, and broad inference secrets, but they do not cover this invariant.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `.github/workflows/e2e-vitest-scenarios.yaml` around `sandbox-operations-vitest`: confirm the default-selector branch in the job `if`, `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations`, Docker Hub authentication before `Run sandbox operations live test`, and the step-scoped `NVIDIA_INFERENCE_API_KEY`. Then read `test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts` and `tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts` to confirm the existing mutation coverage does not require a trusted-ref/manual-dispatch policy marker.
  • Evidence: The job condition includes `(inputs.jobs == '' && inputs.scenarios == '')`, the workflow is manually dispatchable, the job checks out repository code and runs `npm run build:cli`, writes `DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-operations` to `$GITHUB_ENV`, authenticates Docker Hub, and runs `test/e2e-scenario/live/sandbox-operations.test.ts` with `NVIDIA_INFERENCE_API_KEY`. The new boundary tests cover trigger/permission and credential-scoping drift but not a trusted-ref invariant.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: test/e2e-scenario/fixtures/clients/host.ts:144.
Open items: 0 required · 6 warnings · 4 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 0 still apply · 7 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: test/e2e-scenario/fixtures/clients/host.ts:144
  • PRA-2 Resolve or justify: Source-of-truth review needed: test/e2e-scenario/live/sandbox-operations.test.ts:250
  • PRA-3 Resolve or justify: Legacy gateway destroy fallback lacks source-of-truth justification in test/e2e-scenario/fixtures/clients/host.ts:144
  • PRA-4 Resolve or justify: Stdin transport boundary for agent --json untested after live test removal in test/e2e-scenario/live/sandbox-operations.test.ts:250
  • PRA-5 Resolve or justify: Gateway recovery outcome not asserted in CI — skipped-gateway-absent may hide untested contract in test/e2e-scenario/live/sandbox-operations.test.ts:580
  • PRA-6 Resolve or justify: Double-already-absent cleanup path untested (both remove and destroy return 'already absent') in test/e2e-scenario/support-tests/e2e-clients.test.ts:130
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Missing positive validation test for current workflow
  • PRA-T7 Add or justify test follow-up: GATEWAY_ALREADY_ABSENT regex not tested against actual OpenShell output
  • PRA-T8 Add or justify test follow-up: test/e2e-scenario/fixtures/clients/host.ts:144
  • PRA-7 In-scope improvement: Missing positive validation test for current workflow in test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts:1
  • PRA-8 In-scope improvement: Unnecessary DEFAULT_WORKFLOW_PATH constant (single use) in tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts:14
  • PRA-9 In-scope improvement: workflow-boundary.mts monolith growth (8000+ lines, 40+ validate*Job functions) in tools/e2e-scenarios/workflow-boundary.mts:7830
  • PRA-10 In-scope improvement: GATEWAY_ALREADY_ABSENT regex not tested against actual OpenShell output in test/e2e-scenario/support-tests/e2e-clients.test.ts:60

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify acceptance test/e2e-scenario/fixtures/clients/host.ts:144 Add a minimum OpenShell version check (e.g., via `openshell --version` parse) that gates the fallback, or document a target OpenShell version floor with a linked follow-up issue and removal date. At minimum, add a test that validates the fallback is only triggered for versions below the documented floor.
PRA-4 Resolve/justify security test/e2e-scenario/live/sandbox-operations.test.ts:250 Add a unit test in test/openclaw-agent-json.test.ts for stdin transport parsing (piping input to the parser helper), or add a comment in sandbox-operations.test.ts linking to the unit test that covers stdin parsing. If stdin is intentionally unsupported, document that in the CLI help or protocol spec.
PRA-5 Resolve/justify correctness test/e2e-scenario/live/sandbox-operations.test.ts:580 Add an assertion in the live test: when running in GitHub Actions (process.env.GITHUB_ACTIONS === 'true'), expect(gatewayRecovery).not.toBe('skipped-gateway-absent'). Alternatively, add a pre-check that the gateway container exists before the recovery probe.
PRA-6 Resolve/justify correctness test/e2e-scenario/support-tests/e2e-clients.test.ts:130 Add a test case in e2e-clients.test.ts: remove returns exitCode=1 with already-absent stderr, destroy returns exitCode=1 with already-absent stderr → should resolve without error.
PRA-7 Improvement tests test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts:1 Add a test: expect(validateSandboxOperationsWorkflow(readSandboxOperationsWorkflow())).toEqual([]).
PRA-8 Improvement architecture tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts:14 Inline the path join at the call site. Remove the constant.
PRA-9 Improvement architecture tools/e2e-scenarios/workflow-boundary.mts:7830 Continue extracting per-job validation modules (pattern started correctly). Create a registry that imports all validators and a single validateAll function. This is a follow-up refactor, not a blocker.
PRA-10 Improvement tests test/e2e-scenario/support-tests/e2e-clients.test.ts:60 Add a test case in e2e-clients.test.ts with destroy output variant, or test the regex against actual `openshell gateway remove nonexistent` and `openshell gateway destroy -g nonexistent` output.
Review findings by urgency: 0 required fixes, 6 items to resolve/justify, 4 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: test/e2e-scenario/fixtures/clients/host.ts:144

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — no test pins minimum OpenShell version; no version detection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: host.ts:144 comment and fallback logic at lines 140-150

PRA-2 Resolve/justify — Source-of-truth review needed: test/e2e-scenario/live/sandbox-operations.test.ts:250

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial — unit test covers failed-tool provenance but NOT stdin parsing
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: sandbox-operations.test.ts:250-258 comment about retiring stdin assertion

PRA-3 Resolve/justify — Legacy gateway destroy fallback lacks source-of-truth justification

  • Location: test/e2e-scenario/fixtures/clients/host.ts:144
  • Category: acceptance
  • Problem: cleanupGatewayRegistration falls back to `openshell gateway destroy -g` when `gateway remove` is unsupported (older OpenShell builds). The comment says 'Remove this fallback once the supported OpenShell floor no longer includes builds whose local-registration verb was `gateway destroy`' but does not specify: what invalid state is handled, where it's created, why the source cannot be fixed here, what regression test proves the source cannot regress, or when the workaround can be removed.
  • Impact: Technical debt accumulates without a removal path; fallback may mask compatibility issues with newer OpenShell versions; no automated guard ensures fallback is still necessary. If OpenShell changes the destroy verb output, the fallback could silently fail.
  • Recommended action: Add a minimum OpenShell version check (e.g., via `openshell --version` parse) that gates the fallback, or document a target OpenShell version floor with a linked follow-up issue and removal date. At minimum, add a test that validates the fallback is only triggered for versions below the documented floor.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read host.ts lines 60-95. Check if any OpenShell version detection or floor enforcement exists. Search for 'openshell --version' usage in the codebase.
  • Missing regression test: Add a test that runs cleanupGatewayRegistration against a mocked OpenShell version below the documented minimum (fallback triggers) and at/above the minimum (fallback not needed). Alternatively, add an integration test that runs against the minimum supported OpenShell version to prove the fallback is unnecessary.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read host.ts lines 60-95. Check if any OpenShell version detection or floor enforcement exists. Search for 'openshell --version' usage in the codebase.
  • Evidence: host.ts:144 comment: 'Remove this fallback once the supported OpenShell floor no longer includes builds whose local-registration verb was `gateway destroy`'; no version check, no floor constant, no removal timeline. Fallback logic at lines 140-150.

PRA-4 Resolve/justify — Stdin transport boundary for agent --json untested after live test removal

  • Location: test/e2e-scenario/live/sandbox-operations.test.ts:250
  • Category: security
  • Problem: Removed assertAgentJsonTransportBoundaries which tested piped stdin transport for `agent --json` and failed-tool provenance. Failed-tool provenance is covered by unit test in openclaw-agent-json.test.ts. However, stdin transport parsing is now untested in both live and unit tests. Comment claims 'v0.0.69 legacy job did not exercise piped stdin' but does not confirm stdin is unsupported.
  • Impact: If stdin is a supported transport for agent --json, a regression in stdin parsing would go undetected. If stdin is unsupported, this should be documented. Attackers could exploit untested stdin paths for injection if the parser is ever exposed.
  • Recommended action: Add a unit test in test/openclaw-agent-json.test.ts for stdin transport parsing (piping input to the parser helper), or add a comment in sandbox-operations.test.ts linking to the unit test that covers stdin parsing. If stdin is intentionally unsupported, document that in the CLI help or protocol spec.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare removed assertAgentJsonTransportBoundaries (git show HEAD~1:test/e2e-scenario/live/sandbox-operations.test.ts) with test/openclaw-agent-json.test.ts coverage. Search for 'stdin' or 'pipe' in openclaw-agent-json.test.ts.
  • Missing regression test: Add unit test in openclaw-agent-json.test.ts: pipe JSON input to the parser helper and verify correct extraction. If stdin is unsupported, add a test that documents the expected behavior (e.g., parser reads from argv only).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare removed assertAgentJsonTransportBoundaries (git show HEAD~1:test/e2e-scenario/live/sandbox-operations.test.ts) with test/openclaw-agent-json.test.ts coverage. Search for 'stdin' or 'pipe' in openclaw-agent-json.test.ts.
  • Evidence: sandbox-operations.test.ts:250-258 comment: 'The v0.0.69 legacy job did not exercise piped stdin. That experimental migration-only assertion was retired... Failed-tool provenance remains covered deterministically by test/openclaw-agent-json.test.ts; a live prompt cannot require upstream OpenClaw to emit failed tool-result metadata.'

PRA-5 Resolve/justify — Gateway recovery outcome not asserted in CI — skipped-gateway-absent may hide untested contract

  • Location: test/e2e-scenario/live/sandbox-operations.test.ts:580
  • Category: correctness
  • Problem: assertGatewayRecovery now returns GatewayRecoveryOutcome enum (recovered-before-status, recovered-by-status, skipped-gateway-absent) but the test only writes it to scenario-result.json without asserting. In CI, if the shared gateway container is absent, TC-SBX-06 (gateway recovery) is effectively not tested.
  • Impact: Gateway recovery contract (TC-SBX-06) may silently become a no-op in CI if the gateway container is not running, reducing confidence in the recovery path. A broken recovery would not fail the build.
  • Recommended action: Add an assertion in the live test: when running in GitHub Actions (process.env.GITHUB_ACTIONS === 'true'), expect(gatewayRecovery).not.toBe('skipped-gateway-absent'). Alternatively, add a pre-check that the gateway container exists before the recovery probe.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read sandbox-operations.test.ts lines 570-590. The gatewayRecovery variable is only used in artifacts.writeJson('scenario-result.json', { gatewayRecovery }). No expect() call on it.
  • Missing regression test: Add: if (process.env.GITHUB_ACTIONS === 'true') { expect(gatewayRecovery).not.toBe('skipped-gateway-absent'); } in the live test body after assertGatewayRecovery call.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read sandbox-operations.test.ts lines 570-590. The gatewayRecovery variable is only used in artifacts.writeJson('scenario-result.json', { gatewayRecovery }). No expect() call on it.
  • Evidence: sandbox-operations.test.ts:580 const gatewayRecovery = await assertGatewayRecovery(host, SANDBOX_A); line 590 only writes to scenario-result.json.

PRA-6 Resolve/justify — Double-already-absent cleanup path untested (both remove and destroy return 'already absent')

  • Location: test/e2e-scenario/support-tests/e2e-clients.test.ts:130
  • Category: correctness
  • Problem: cleanupGatewayRegistration tries `remove` then legacy `destroy`. If both return exitCode=1 with 'already absent' stderr, the current logic calls assertExitZero(destroy) which throws. Tests cover: remove succeeds, remove fails with already-absent, remove unsupported+destroy succeeds, remove unsupported+destroy already-absent. Missing: remove already-absent + destroy already-absent.
  • Impact: Edge case where gateway registration is already fully cleaned up could cause cleanup to fail and mask the primary test result. This could cause false negatives in CI when running tests in parallel or after partial cleanup.
  • Recommended action: Add a test case in e2e-clients.test.ts: remove returns exitCode=1 with already-absent stderr, destroy returns exitCode=1 with already-absent stderr → should resolve without error.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read e2e-clients.test.ts lines 130-195. Verify the five existing test cases and confirm the double-already-absent case is missing.
  • Missing regression test: Add test: 'host client accepts double already-absent gateway cleanup' with runner.enqueue({ exitCode: 1, stderr: 'No gateway metadata found' }) for both remove and destroy calls; expect cleanupGatewayRegistration to resolve.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read e2e-clients.test.ts lines 130-195. Verify the five existing test cases and confirm the double-already-absent case is missing.
  • Evidence: e2e-clients.test.ts has 5 cleanupGatewayRegistration tests but none with both verbs returning already-absent. Host.ts logic at lines 135-150 shows destroy is only called if remove fails with unsupported verb, not if remove returns already-absent.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-7 Improvement — Missing positive validation test for current workflow

  • Location: test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts:1
  • Category: tests
  • Problem: Mutation tests verify boundary rejects bad configs, but no explicit test validates that the current sandbox-operations-vitest job passes validation (i.e., validateSandboxOperationsWorkflow returns empty errors).
  • Impact: If the validator has a bug that always returns errors, the mutation tests would still pass (they expect errors). A positive test ensures the current config is valid and catches regressions where the validator becomes too strict.
  • Suggested action: Add a test: expect(validateSandboxOperationsWorkflow(readSandboxOperationsWorkflow())).toEqual([]).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run the test file and check if any test asserts empty errors for the current workflow.
  • Missing regression test: Add positive validation test case in sandbox-operations-workflow-boundary.test.ts.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: sandbox-operations-workflow-boundary.test.ts has 4 tests: inventory/dispatch, mutation rejections (3), secret-bearing job checks. No positive validation test.

PRA-8 Improvement — Unnecessary DEFAULT_WORKFLOW_PATH constant (single use)

  • Location: tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts:14
  • Category: architecture
  • Problem: DEFAULT_WORKFLOW_PATH constant defined at line 14 and used only once at line 40 as default parameter. No other consumer exists.
  • Impact: Minor code clarity debt; the constant adds indirection without reuse benefit. Increases cognitive load for readers who must trace the constant to its definition.
  • Suggested action: Inline the path join at the call site. Remove the constant.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-operations-workflow-boundary.mts for any other references to DEFAULT_WORKFLOW_PATH. Only one usage found at line 40.
  • Missing regression test: Not applicable — structural simplification.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 14: const DEFAULT_WORKFLOW_PATH = join(REPO_ROOT, '.github', 'workflows', 'e2e-vitest-scenarios.yaml'); Line 40: workflowPath = DEFAULT_WORKFLOW_PATH. No other usages.

PRA-9 Improvement — workflow-boundary.mts monolith growth (8000+ lines, 40+ validate*Job functions)

  • Location: tools/e2e-scenarios/workflow-boundary.mts:7830
  • Category: architecture
  • Problem: The workflow-boundary.mts file has grown to 8000+ lines with 40+ validate*Job functions. Adding validateSandboxOperationsWorkflow continues this pattern. The new per-job validator (sandbox-operations-workflow-boundary.mts) is the right direction but a registry pattern would improve maintainability.
  • Impact: Monolith growth makes it harder to review changes, increases merge conflict risk, and slows CI (full file re-linted). Future validators will keep adding to the same file unless a registry pattern is adopted.
  • Suggested action: Continue extracting per-job validation modules (pattern started correctly). Create a registry that imports all validators and a single validateAll function. This is a follow-up refactor, not a blocker.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Count lines and validate*Job functions in workflow-boundary.mts. Observe the import of validateSandboxOperationsWorkflow at line ~80.
  • Missing regression test: Not applicable — architectural observation.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: workflow-boundary.mts:8000+ lines, imports validateSandboxOperationsWorkflow at line 7830, 40+ validate*Job functions called sequentially in validateE2eVitestScenariosWorkflowBoundary.

PRA-10 Improvement — GATEWAY_ALREADY_ABSENT regex not tested against actual OpenShell output

  • Location: test/e2e-scenario/support-tests/e2e-clients.test.ts:60
  • Category: tests
  • Problem: The regex matches 'gateway.*does not exist', 'gateway.*not found', 'No (active )?gateway', 'No gateway metadata found'. The legacy destroy verb may produce different output. Should verify regex covers both verbs' 'not found' outputs.
  • Impact: If the legacy destroy verb produces stderr that doesn't match the regex, the fallback path would incorrectly treat it as a real error and call assertExitZero, causing test failures on older OpenShell versions.
  • Suggested action: Add a test case in e2e-clients.test.ts with destroy output variant, or test the regex against actual `openshell gateway remove nonexistent` and `openshell gateway destroy -g nonexistent` output.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run openshell gateway remove nonexistent and openshell gateway destroy -g nonexistent locally (or in CI) and check stderr. Compare against GATEWAY_ALREADY_ABSENT regex at host.ts:60-61.
  • Missing regression test: Add test case in e2e-clients.test.ts: destroy returns exitCode=1 with stderr matching legacy destroy 'not found' output; expect cleanupGatewayRegistration to succeed.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: host.ts:60-61: const GATEWAY_ALREADY_ABSENT = /gateway[^\n]*(?:does not exist|not found)|No (?:active )?gateway|No gateway metadata found/i; const GATEWAY_REMOVE_UNSUPPORTED = /unrecognized subcommand ['"]remove['"]|unknown command ['"]remove['"]/i;
Simplification opportunities: 1 possible cut, net -2 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-8 shrink (tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts:14): const DEFAULT_WORKFLOW_PATH = join(REPO_ROOT, '.github', 'workflows', 'e2e-vitest-scenarios.yaml'); and default parameter workflowPath = DEFAULT_WORKFLOW_PATH
    • Replacement: workflowPath = join(REPO_ROOT, '.github', 'workflows', 'e2e-vitest-scenarios.yaml')
    • Net: -2 lines
    • Safety boundary: No behavior change; only removes indirection. The path is a fixed repo constant.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Assert gatewayRecovery !== 'skipped-gateway-absent' in CI (GITHUB_ACTIONS=true). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow boundary validation adds static enforcement but live test requires NEMOCLAW_RUN_E2E_SCENARIOS=1. CI should run validation tests on every PR (already via e2e-vitest-support project).
  • PRA-T2 Runtime validation — Add double-already-absent cleanup test case in e2e-clients.test.ts. Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow boundary validation adds static enforcement but live test requires NEMOCLAW_RUN_E2E_SCENARIOS=1. CI should run validation tests on every PR (already via e2e-vitest-support project).
  • PRA-T3 Runtime validation — Add unit test for stdin transport parsing in openclaw-agent-json.test.ts (if stdin supported) or document as unsupported. Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow boundary validation adds static enforcement but live test requires NEMOCLAW_RUN_E2E_SCENARIOS=1. CI should run validation tests on every PR (already via e2e-vitest-support project).
  • PRA-T4 Runtime validation — Add positive workflow validation test: expect(validateSandboxOperationsWorkflow(readSandboxOperationsWorkflow())).toEqual([]). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow boundary validation adds static enforcement but live test requires NEMOCLAW_RUN_E2E_SCENARIOS=1. CI should run validation tests on every PR (already via e2e-vitest-support project).
  • PRA-T5 Runtime validation — Test GATEWAY_ALREADY_ABSENT regex against actual OpenShell gateway destroy output. Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow boundary validation adds static enforcement but live test requires NEMOCLAW_RUN_E2E_SCENARIOS=1. CI should run validation tests on every PR (already via e2e-vitest-support project).
  • PRA-T6 Missing positive validation test for current workflow — Add a test: expect(validateSandboxOperationsWorkflow(readSandboxOperationsWorkflow())).toEqual([]).
  • PRA-T7 GATEWAY_ALREADY_ABSENT regex not tested against actual OpenShell output — Add a test case in e2e-clients.test.ts with destroy output variant, or test the regex against actual `openshell gateway remove nonexistent` and `openshell gateway destroy -g nonexistent` output.
  • PRA-T8 test/e2e-scenario/fixtures/clients/host.ts:144 — Missing — no test pins minimum OpenShell version; no version detection. host.ts:144 comment and fallback logic at lines 140-150
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: test/e2e-scenario/fixtures/clients/host.ts:144

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — no test pins minimum OpenShell version; no version detection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: host.ts:144 comment and fallback logic at lines 140-150

PRA-2 Resolve/justify — Source-of-truth review needed: test/e2e-scenario/live/sandbox-operations.test.ts:250

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial — unit test covers failed-tool provenance but NOT stdin parsing
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: sandbox-operations.test.ts:250-258 comment about retiring stdin assertion

PRA-3 Resolve/justify — Legacy gateway destroy fallback lacks source-of-truth justification

  • Location: test/e2e-scenario/fixtures/clients/host.ts:144
  • Category: acceptance
  • Problem: cleanupGatewayRegistration falls back to `openshell gateway destroy -g` when `gateway remove` is unsupported (older OpenShell builds). The comment says 'Remove this fallback once the supported OpenShell floor no longer includes builds whose local-registration verb was `gateway destroy`' but does not specify: what invalid state is handled, where it's created, why the source cannot be fixed here, what regression test proves the source cannot regress, or when the workaround can be removed.
  • Impact: Technical debt accumulates without a removal path; fallback may mask compatibility issues with newer OpenShell versions; no automated guard ensures fallback is still necessary. If OpenShell changes the destroy verb output, the fallback could silently fail.
  • Recommended action: Add a minimum OpenShell version check (e.g., via `openshell --version` parse) that gates the fallback, or document a target OpenShell version floor with a linked follow-up issue and removal date. At minimum, add a test that validates the fallback is only triggered for versions below the documented floor.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read host.ts lines 60-95. Check if any OpenShell version detection or floor enforcement exists. Search for 'openshell --version' usage in the codebase.
  • Missing regression test: Add a test that runs cleanupGatewayRegistration against a mocked OpenShell version below the documented minimum (fallback triggers) and at/above the minimum (fallback not needed). Alternatively, add an integration test that runs against the minimum supported OpenShell version to prove the fallback is unnecessary.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read host.ts lines 60-95. Check if any OpenShell version detection or floor enforcement exists. Search for 'openshell --version' usage in the codebase.
  • Evidence: host.ts:144 comment: 'Remove this fallback once the supported OpenShell floor no longer includes builds whose local-registration verb was `gateway destroy`'; no version check, no floor constant, no removal timeline. Fallback logic at lines 140-150.

PRA-4 Resolve/justify — Stdin transport boundary for agent --json untested after live test removal

  • Location: test/e2e-scenario/live/sandbox-operations.test.ts:250
  • Category: security
  • Problem: Removed assertAgentJsonTransportBoundaries which tested piped stdin transport for `agent --json` and failed-tool provenance. Failed-tool provenance is covered by unit test in openclaw-agent-json.test.ts. However, stdin transport parsing is now untested in both live and unit tests. Comment claims 'v0.0.69 legacy job did not exercise piped stdin' but does not confirm stdin is unsupported.
  • Impact: If stdin is a supported transport for agent --json, a regression in stdin parsing would go undetected. If stdin is unsupported, this should be documented. Attackers could exploit untested stdin paths for injection if the parser is ever exposed.
  • Recommended action: Add a unit test in test/openclaw-agent-json.test.ts for stdin transport parsing (piping input to the parser helper), or add a comment in sandbox-operations.test.ts linking to the unit test that covers stdin parsing. If stdin is intentionally unsupported, document that in the CLI help or protocol spec.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare removed assertAgentJsonTransportBoundaries (git show HEAD~1:test/e2e-scenario/live/sandbox-operations.test.ts) with test/openclaw-agent-json.test.ts coverage. Search for 'stdin' or 'pipe' in openclaw-agent-json.test.ts.
  • Missing regression test: Add unit test in openclaw-agent-json.test.ts: pipe JSON input to the parser helper and verify correct extraction. If stdin is unsupported, add a test that documents the expected behavior (e.g., parser reads from argv only).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare removed assertAgentJsonTransportBoundaries (git show HEAD~1:test/e2e-scenario/live/sandbox-operations.test.ts) with test/openclaw-agent-json.test.ts coverage. Search for 'stdin' or 'pipe' in openclaw-agent-json.test.ts.
  • Evidence: sandbox-operations.test.ts:250-258 comment: 'The v0.0.69 legacy job did not exercise piped stdin. That experimental migration-only assertion was retired... Failed-tool provenance remains covered deterministically by test/openclaw-agent-json.test.ts; a live prompt cannot require upstream OpenClaw to emit failed tool-result metadata.'

PRA-5 Resolve/justify — Gateway recovery outcome not asserted in CI — skipped-gateway-absent may hide untested contract

  • Location: test/e2e-scenario/live/sandbox-operations.test.ts:580
  • Category: correctness
  • Problem: assertGatewayRecovery now returns GatewayRecoveryOutcome enum (recovered-before-status, recovered-by-status, skipped-gateway-absent) but the test only writes it to scenario-result.json without asserting. In CI, if the shared gateway container is absent, TC-SBX-06 (gateway recovery) is effectively not tested.
  • Impact: Gateway recovery contract (TC-SBX-06) may silently become a no-op in CI if the gateway container is not running, reducing confidence in the recovery path. A broken recovery would not fail the build.
  • Recommended action: Add an assertion in the live test: when running in GitHub Actions (process.env.GITHUB_ACTIONS === 'true'), expect(gatewayRecovery).not.toBe('skipped-gateway-absent'). Alternatively, add a pre-check that the gateway container exists before the recovery probe.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read sandbox-operations.test.ts lines 570-590. The gatewayRecovery variable is only used in artifacts.writeJson('scenario-result.json', { gatewayRecovery }). No expect() call on it.
  • Missing regression test: Add: if (process.env.GITHUB_ACTIONS === 'true') { expect(gatewayRecovery).not.toBe('skipped-gateway-absent'); } in the live test body after assertGatewayRecovery call.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read sandbox-operations.test.ts lines 570-590. The gatewayRecovery variable is only used in artifacts.writeJson('scenario-result.json', { gatewayRecovery }). No expect() call on it.
  • Evidence: sandbox-operations.test.ts:580 const gatewayRecovery = await assertGatewayRecovery(host, SANDBOX_A); line 590 only writes to scenario-result.json.

PRA-6 Resolve/justify — Double-already-absent cleanup path untested (both remove and destroy return 'already absent')

  • Location: test/e2e-scenario/support-tests/e2e-clients.test.ts:130
  • Category: correctness
  • Problem: cleanupGatewayRegistration tries `remove` then legacy `destroy`. If both return exitCode=1 with 'already absent' stderr, the current logic calls assertExitZero(destroy) which throws. Tests cover: remove succeeds, remove fails with already-absent, remove unsupported+destroy succeeds, remove unsupported+destroy already-absent. Missing: remove already-absent + destroy already-absent.
  • Impact: Edge case where gateway registration is already fully cleaned up could cause cleanup to fail and mask the primary test result. This could cause false negatives in CI when running tests in parallel or after partial cleanup.
  • Recommended action: Add a test case in e2e-clients.test.ts: remove returns exitCode=1 with already-absent stderr, destroy returns exitCode=1 with already-absent stderr → should resolve without error.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read e2e-clients.test.ts lines 130-195. Verify the five existing test cases and confirm the double-already-absent case is missing.
  • Missing regression test: Add test: 'host client accepts double already-absent gateway cleanup' with runner.enqueue({ exitCode: 1, stderr: 'No gateway metadata found' }) for both remove and destroy calls; expect cleanupGatewayRegistration to resolve.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read e2e-clients.test.ts lines 130-195. Verify the five existing test cases and confirm the double-already-absent case is missing.
  • Evidence: e2e-clients.test.ts has 5 cleanupGatewayRegistration tests but none with both verbs returning already-absent. Host.ts logic at lines 135-150 shows destroy is only called if remove fails with unsupported verb, not if remove returns already-absent.

PRA-7 Improvement — Missing positive validation test for current workflow

  • Location: test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts:1
  • Category: tests
  • Problem: Mutation tests verify boundary rejects bad configs, but no explicit test validates that the current sandbox-operations-vitest job passes validation (i.e., validateSandboxOperationsWorkflow returns empty errors).
  • Impact: If the validator has a bug that always returns errors, the mutation tests would still pass (they expect errors). A positive test ensures the current config is valid and catches regressions where the validator becomes too strict.
  • Suggested action: Add a test: expect(validateSandboxOperationsWorkflow(readSandboxOperationsWorkflow())).toEqual([]).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run the test file and check if any test asserts empty errors for the current workflow.
  • Missing regression test: Add positive validation test case in sandbox-operations-workflow-boundary.test.ts.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: sandbox-operations-workflow-boundary.test.ts has 4 tests: inventory/dispatch, mutation rejections (3), secret-bearing job checks. No positive validation test.

PRA-8 Improvement — Unnecessary DEFAULT_WORKFLOW_PATH constant (single use)

  • Location: tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts:14
  • Category: architecture
  • Problem: DEFAULT_WORKFLOW_PATH constant defined at line 14 and used only once at line 40 as default parameter. No other consumer exists.
  • Impact: Minor code clarity debt; the constant adds indirection without reuse benefit. Increases cognitive load for readers who must trace the constant to its definition.
  • Suggested action: Inline the path join at the call site. Remove the constant.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-operations-workflow-boundary.mts for any other references to DEFAULT_WORKFLOW_PATH. Only one usage found at line 40.
  • Missing regression test: Not applicable — structural simplification.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 14: const DEFAULT_WORKFLOW_PATH = join(REPO_ROOT, '.github', 'workflows', 'e2e-vitest-scenarios.yaml'); Line 40: workflowPath = DEFAULT_WORKFLOW_PATH. No other usages.

PRA-9 Improvement — workflow-boundary.mts monolith growth (8000+ lines, 40+ validate*Job functions)

  • Location: tools/e2e-scenarios/workflow-boundary.mts:7830
  • Category: architecture
  • Problem: The workflow-boundary.mts file has grown to 8000+ lines with 40+ validate*Job functions. Adding validateSandboxOperationsWorkflow continues this pattern. The new per-job validator (sandbox-operations-workflow-boundary.mts) is the right direction but a registry pattern would improve maintainability.
  • Impact: Monolith growth makes it harder to review changes, increases merge conflict risk, and slows CI (full file re-linted). Future validators will keep adding to the same file unless a registry pattern is adopted.
  • Suggested action: Continue extracting per-job validation modules (pattern started correctly). Create a registry that imports all validators and a single validateAll function. This is a follow-up refactor, not a blocker.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Count lines and validate*Job functions in workflow-boundary.mts. Observe the import of validateSandboxOperationsWorkflow at line ~80.
  • Missing regression test: Not applicable — architectural observation.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: workflow-boundary.mts:8000+ lines, imports validateSandboxOperationsWorkflow at line 7830, 40+ validate*Job functions called sequentially in validateE2eVitestScenariosWorkflowBoundary.

PRA-10 Improvement — GATEWAY_ALREADY_ABSENT regex not tested against actual OpenShell output

  • Location: test/e2e-scenario/support-tests/e2e-clients.test.ts:60
  • Category: tests
  • Problem: The regex matches 'gateway.*does not exist', 'gateway.*not found', 'No (active )?gateway', 'No gateway metadata found'. The legacy destroy verb may produce different output. Should verify regex covers both verbs' 'not found' outputs.
  • Impact: If the legacy destroy verb produces stderr that doesn't match the regex, the fallback path would incorrectly treat it as a real error and call assertExitZero, causing test failures on older OpenShell versions.
  • Suggested action: Add a test case in e2e-clients.test.ts with destroy output variant, or test the regex against actual `openshell gateway remove nonexistent` and `openshell gateway destroy -g nonexistent` output.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run openshell gateway remove nonexistent and openshell gateway destroy -g nonexistent locally (or in CI) and check stderr. Compare against GATEWAY_ALREADY_ABSENT regex at host.ts:60-61.
  • Missing regression test: Add test case in e2e-clients.test.ts: destroy returns exitCode=1 with stderr matching legacy destroy 'not found' output; expect cleanupGatewayRegistration to succeed.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: host.ts:60-61: const GATEWAY_ALREADY_ABSENT = /gateway[^\n]*(?:does not exist|not found)|No (?:active )?gateway|No gateway metadata found/i; const GATEWAY_REMOVE_UNSUPPORTED = /unrecognized subcommand ['"]remove['"]|unknown command ['"]remove['"]/i;

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts (1)

33-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the full default-dispatch result for {}.

The empty-selector branch only checks that sandbox-operations-vitest is present, so this test still passes if the default path starts returning valid: false or flips liveScenariosRuns. Match the same observable metadata here as you do for the selective-dispatch cases. As per path instructions, "Review tests for behavioral confidence rather than implementation lock-in."

🤖 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-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts`
around lines 33 - 35, The empty-selector assertion in the e2e workflow dispatch
test only checks selectedFreeStandingJobs, so it can miss regressions in the
default path metadata. Update the {} case in
sandbox-operations-workflow-boundary.test to assert the full result from
evaluateE2eVitestWorkflowDispatchSelectors, matching the same observable fields
used in the selective-dispatch tests such as valid and liveScenariosRuns, while
still verifying sandbox-operations-vitest is included.

Source: Path instructions

🤖 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 `@tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts`:
- Around line 146-149: validateSandboxOperationsWorkflowBoundary() only runs the
job-local validator today, so the public boundary helper misses the shared
selector/gate and inventory coverage checks. Update this function to also invoke
the shared e2e-vitest-scenarios boundary validation after
readSandboxOperationsWorkflow(workflowPath), using the existing
validateSandboxOperationsWorkflow and the shared boundary helper so callers get
the full contract centrally from this entrypoint.

---

Outside diff comments:
In
`@test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts`:
- Around line 33-35: The empty-selector assertion in the e2e workflow dispatch
test only checks selectedFreeStandingJobs, so it can miss regressions in the
default path metadata. Update the {} case in
sandbox-operations-workflow-boundary.test to assert the full result from
evaluateE2eVitestWorkflowDispatchSelectors, matching the same observable fields
used in the selective-dispatch tests such as valid and liveScenariosRuns, while
still verifying sandbox-operations-vitest is included.
🪄 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: 1856ff6b-b8c1-46c1-b157-2805ba9cd107

📥 Commits

Reviewing files that changed from the base of the PR and between 9aec335 and 7a9a099.

📒 Files selected for processing (3)
  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/e2e-vitest-scenarios.yaml

Comment thread tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts Outdated
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28313913603
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ❌ failure

Failed jobs: sandbox-operations-vitest. Check run artifacts for logs.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28314187409
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28314394255
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-scenario/live/sandbox-operations.test.ts (1)

100-119: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Pass redaction inputs through the shared hosted config instead of hardcoding env names.

onboardSandbox() now accepts a generic providerEnv, but the artifact redaction still assumes the hosted credentials will always live in NVIDIA_INFERENCE_API_KEY and COMPATIBLE_API_KEY. That duplicates the contract already exposed by requireHostedInferenceConfig(), so a future change to the shared mapping can leave onboarding working while CI artifacts stop masking the real secret.

Suggested direction
 async function onboardSandbox(
   host: HostCliClient,
   cleanup: CleanupRegistry,
   sandboxName: string,
   artifactName: string,
   providerEnv: NodeJS.ProcessEnv,
+  providerRedactions: string[] = [],
   extraEnv: NodeJS.ProcessEnv = {},
 ): Promise<ShellProbeResult> {
   cleanup.add(`destroy sandbox ${sandboxName}`, () => cleanupSandbox(host, sandboxName));
   const result = await host.nemoclaw(
@@
-      redactionValues: [
-        providerEnv.NVIDIA_INFERENCE_API_KEY ?? "",
-        providerEnv.COMPATIBLE_API_KEY ?? "",
-      ],
+      redactionValues: providerRedactions,
       timeoutMs: 20 * 60_000,
     },
   );

Then have the callers pass the hosted redaction values from the shared config instead of re-encoding the env-key names here.

🤖 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-scenario/live/sandbox-operations.test.ts` around lines 100 - 119,
The redaction setup in onboardSandbox() still hardcodes hosted secret env names,
which can drift from the shared hosted config contract. Update onboardSandbox()
to derive its redaction values from the same shared mapping used by
requireHostedInferenceConfig(), and have the callers pass those hosted redaction
inputs through providerEnv/extraEnv instead of directly reading
NVIDIA_INFERENCE_API_KEY and COMPATIBLE_API_KEY here.
🤖 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-scenario/live/sandbox-operations.test.ts`:
- Line 595: The live E2E coverage for agent --json no longer verifies that piped
stdin still reaches the CLI entrypoint. Update the existing sandbox-operations
scenario (the TC-SBX-02b case in the live test suite) to keep an assertion for
stdin passthrough, or add a separate live scenario that exercises printf ... |
nemoclaw ... agent --json while still preserving the current exit-status and
failed-tool provenance checks.

---

Outside diff comments:
In `@test/e2e-scenario/live/sandbox-operations.test.ts`:
- Around line 100-119: The redaction setup in onboardSandbox() still hardcodes
hosted secret env names, which can drift from the shared hosted config contract.
Update onboardSandbox() to derive its redaction values from the same shared
mapping used by requireHostedInferenceConfig(), and have the callers pass those
hosted redaction inputs through providerEnv/extraEnv instead of directly reading
NVIDIA_INFERENCE_API_KEY and COMPATIBLE_API_KEY here.
🪄 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: 039a9a14-8283-4242-87f0-d0a46f7649d1

📥 Commits

Reviewing files that changed from the base of the PR and between ac07ef9 and 238224a.

📒 Files selected for processing (1)
  • test/e2e-scenario/live/sandbox-operations.test.ts

Comment thread test/e2e-scenario/live/sandbox-operations.test.ts Outdated
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28314537135
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ❌ failure

Failed jobs: sandbox-operations-vitest. Check run artifacts for logs.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

Live run 28314537135 reached the agent transport checks and exposed another unreliable assertion introduced with #5683, not a legacy sandbox-operations-e2e contract. The prompt did execute exit 7, but OpenClaw returned toolSummary.failures: 0 and no failed tool-result metadata, so NemoClaw correctly emitted no [openclaw provenance] line. #5683 explicitly bounds the feature to provenance metadata present in upstream JSON; NemoClaw cannot reconstruct metadata OpenClaw omits.

The deterministic source/package tests already cover provenance extraction and forwarding. I have removed only this prompt-based live assertion locally while retaining the deterministic non-zero transport check. Artifact upload and cleanup both passed. I will refresh/push #5926 after bootstrap #5927 lands, avoiding another known static-checks failure from current main, then rerun the exact live job.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28335063162
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts (1)

33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid coupling this helper to the next workflow job.

Line 34 makes these boundary tests fail if an unrelated job is inserted between sandbox-operations-vitest and sandbox-survival-vitest. Find the next two-space job key instead so the mutation remains scoped to the sandbox-operations job without encoding sibling order. As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”

Suggested helper adjustment
 function mutateSandboxOperationsJob(source: string, mutate: (jobSource: string) => string): string {
   const startMarker = "  sandbox-operations-vitest:\n";
-  const endMarker = "  sandbox-survival-vitest:\n";
   const start = source.indexOf(startMarker);
-  const end = source.indexOf(endMarker, start + startMarker.length);
   expect(start).toBeGreaterThanOrEqual(0);
-  expect(end).toBeGreaterThan(start);
+  const rest = source.slice(start + startMarker.length);
+  const nextJob = /^  [A-Za-z0-9_-]+:\n/m.exec(rest);
+  const end = nextJob ? start + startMarker.length + nextJob.index : source.length;
+  expect(end).toBeGreaterThan(start + startMarker.length);
   const jobSource = source.slice(start, end);
🤖 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-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts`
around lines 33 - 38, The boundary helper in
sandbox-operations-workflow-boundary.test.ts is too tightly coupled to the next
sibling job marker. Update the logic around the sandbox-operations-vitest check
so it finds the next two-space-indented job key after the start marker, rather
than hardcoding sandbox-survival-vitest as the end boundary. Keep the assertions
in the boundary test focused on the sandbox-operations-vitest block and use the
helper’s existing source/index scan approach to locate the next job entry
without depending on workflow job order.

Source: Path instructions

🤖 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-scenario/live/sandbox-operations.test.ts`:
- Around line 539-540: The recovery check in the sandbox operations test is
soft-passing a real failure by returning a skipped result when the gateway did
not restart. Update the failure path in the recovery logic around the
afterStatus check so that a failed restart or unhealthy Docker state causes the
test to fail rather than emit a passing or skipped scenario result. Keep the
existing gateway recovery flow in the sandbox-operations test, but make the
conditionals in that recovery branch strict so the test only passes when the
gateway actually recovers.

---

Nitpick comments:
In
`@test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts`:
- Around line 33-38: The boundary helper in
sandbox-operations-workflow-boundary.test.ts is too tightly coupled to the next
sibling job marker. Update the logic around the sandbox-operations-vitest check
so it finds the next two-space-indented job key after the start marker, rather
than hardcoding sandbox-survival-vitest as the end boundary. Keep the assertions
in the boundary test focused on the sandbox-operations-vitest block and use the
helper’s existing source/index scan approach to locate the next job entry
without depending on workflow job order.
🪄 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: 471fedb7-199c-4f03-99ff-126fde3df4c7

📥 Commits

Reviewing files that changed from the base of the PR and between f72ad79 and f3fc8f1.

📒 Files selected for processing (4)
  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/live/sandbox-operations.test.ts
  • test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/e2e-vitest-scenarios.yaml
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts

Comment thread test/e2e-scenario/live/sandbox-operations.test.ts Outdated
cv added 2 commits June 28, 2026 13:50
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28335373856
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ❌ failure

Failed jobs: sandbox-operations-vitest. Check run artifacts for logs.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28335835809
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28335992056
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28336125029
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28336265130
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28336361990
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success

@cv

cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

Exact-head sandbox-operations evidence

Verified head ca68bbdcb1e05ed4f97a3840b0f5a32bd54f13e6 passed the exact sandbox-operations-vitest dispatch in run 28337053896. The live scenario, artifact upload, isolated Docker-auth cleanup, and all post steps passed.

Downloaded artifact e2e-vitest-scenarios-sandbox-operations shows:

  • scenario-result.json: status: passed, gatewayRecovery: skipped-gateway-absent.
  • TC-SBX-01/02/02b/03/04/05/07/08/09/10/11 evidence is present; both TC-SBX-11 directions returned STATUS_403.
  • cleanup.json: sandbox B, sandbox A, and the shared gateway registration all passed; failures: [].
  • Current OpenShell cleanup used gateway remove nemoclaw successfully.
  • Artifact scan found no NVIDIA_INFERENCE_API_KEY, COMPATIBLE_API_KEY, nvapi-, or bearer-token-shaped values.

TC-SBX-06 is deliberately not claimed as proven: this runner had no legacy openshell-cluster-nemoclaw Docker container, matching the v0.0.69 script's soft-skip condition. The new scenario records that outcome instead of silently counting it as recovery parity; if a container is present, kill/status/final-running assertions are fail-closed.

Review dispositions on this frozen head:

  • Fixed: job/step Docker-auth scoping, workspace-auth rejection, hosted-secret redaction, strict cleanup, canonical HostCliClient sandbox cleanup, current/legacy gateway-registration cleanup with five support cases, quick-Docker-restart recovery classification, and structural workflow-job mutation.
  • bin/nemoclaw.js is the intentional stable executable launcher. The workflow now proves it with test -x plus --version, and the boundary validator rejects substituting dist/nemoclaw.js; the exact live run passed that preflight.
  • Manual-dispatch-only and permissions.contents: read are already ratcheted by the central validateE2EVitestScenariosWorkflowBoundary checks, including rejection of push, pull_request, pull_request_target, and schedule triggers.
  • The named default workflow path is retained as the validator's injectable test seam. The 60-minute job budget is explicitly split between the 45-minute live test and 15 minutes of setup/artifact/cleanup overhead.

Local evidence on the final changes: 48 focused E2E support tests, CLI typecheck, repository architecture checks, YAML/format/lint/security hooks, source-shape budget, and test-file budget passed. The broad test-cli pre-commit hook was skipped only on the last test-only commits after unrelated shared-machine integration flakes under several concurrent coverage runs; the preceding full hook passed, and PR CI reran the broad suite independently with all five CLI shards and aggregate cli-tests/checks green.

@cv

cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-scenario/live/sandbox-operations.test.ts (1)

462-472: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fail the gateway probe when Docker introspection itself fails.

These branches currently interpret docker ps / docker inspect from stdout alone. A Docker error here can be misreported as "skipped-gateway-absent" or "recovered-by-status", so TC-SBX-06 can pass without actually proving the gateway transition. Only treat the gateway as absent when docker ps exits successfully with empty output, and assert success on the inspect probes before classifying the outcome.

Suggested fix
   const running = await host.command(
     "docker",
     ["ps", "-q", "--filter", `name=${GATEWAY_CONTAINER}`],
@@
       timeoutMs: 15_000,
     },
   );
+  expectExitZero(running, "probe shared NemoClaw gateway container");
   if (!running.stdout.trim()) {
     return "skipped-gateway-absent";
   }
@@
   const afterKill = await host.command(
     "docker",
     ["inspect", "-f", "{{.State.Running}}", GATEWAY_CONTAINER],
@@
       timeoutMs: 15_000,
     },
   );
+  expectExitZero(afterKill, "inspect shared NemoClaw gateway after kill");
   const recoveryOutcome =
     afterKill.stdout.trim() === "true" ? "recovered-before-status" : "recovered-by-status";
@@
   const afterStatus = await host.command(
     "docker",
     ["inspect", "-f", "{{.State.Running}}", GATEWAY_CONTAINER],
@@
       timeoutMs: 15_000,
     },
   );
   expectExitZero(status, `nemoclaw ${sandboxName} status after gateway kill`);
+  expectExitZero(afterStatus, "inspect shared NemoClaw gateway after status");
   expect(afterStatus.stdout.trim(), resultText(afterStatus)).toBe("true");

As per path instructions, “Flag ... conditionals that make a test pass without exercising its claim.”

Also applies to: 483-493, 500-510

🤖 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-scenario/live/sandbox-operations.test.ts` around lines 462 - 472,
The TC-SBX-06 gateway probe logic in the sandbox-operations test is treating
Docker command output as if it were success, which can hide failures. Update the
gateway checks around host.command for the docker ps and docker inspect probes
so they first verify the command succeeded before using stdout to classify the
outcome. In the gateway-absent, recovered-by-status, and related branches, only
return those states when the corresponding Docker introspection call completes
successfully; otherwise fail the probe so the test cannot pass without actually
validating the gateway transition.

Source: Path instructions

🤖 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-scenario/fixtures/clients/host.ts`:
- Around line 128-146: In cleanupGatewayRegistration, the legacy gateway destroy
fallback is being used for any non-zero gateway remove result, which can mask
real failures in the supported path. Update the logic around
command/resultText/GATEWAY_ALREADY_ABSENT so the fallback only runs when
openshell gateway remove clearly reports an unknown or unrecognized subcommand,
and otherwise fail fast by asserting the remove result. Keep the existing
destroy fallback and cleanup gateway registration flow, but narrow the condition
using the remove command result before invoking the legacy path.

---

Outside diff comments:
In `@test/e2e-scenario/live/sandbox-operations.test.ts`:
- Around line 462-472: The TC-SBX-06 gateway probe logic in the
sandbox-operations test is treating Docker command output as if it were success,
which can hide failures. Update the gateway checks around host.command for the
docker ps and docker inspect probes so they first verify the command succeeded
before using stdout to classify the outcome. In the gateway-absent,
recovered-by-status, and related branches, only return those states when the
corresponding Docker introspection call completes successfully; otherwise fail
the probe so the test cannot pass without actually validating the gateway
transition.
🪄 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: fdfaadb2-d208-4023-a1f2-8c6bc418beb9

📥 Commits

Reviewing files that changed from the base of the PR and between 5f790e5 and ac1e912.

📒 Files selected for processing (6)
  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/fixtures/clients/host.ts
  • test/e2e-scenario/live/sandbox-operations.test.ts
  • test/e2e-scenario/support-tests/e2e-clients.test.ts
  • test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/e2e-scenario/support-tests/sandbox-operations-workflow-boundary.test.ts
  • .github/workflows/e2e-vitest-scenarios.yaml
  • tools/e2e-scenarios/sandbox-operations-workflow-boundary.mts

Comment thread test/e2e-scenario/fixtures/clients/host.ts
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28336717058
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28337053896
Workflow ref: codex/e2e-sandbox-operations-parity
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success

@cv

cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

Final automated-review dispositions

Reviewed the final-head GPT and Nemotron advisor output against ca68bbdcb1e05ed4f97a3840b0f5a32bd54f13e6 and the exact live run 28337053896.

  • Trusted dispatch ref: this is deliberately a maintainer-operated, workflow_dispatch-only lane. The central boundary validator rejects automatic/untrusted event triggers and write-scoped contents permissions; new mutation tests cover pull_request and contents: write. A prose marker would not enforce ref trust, while restricting refs would prevent the pre-merge exact-head evidence this lane exists to collect. Maintainer selection of a trusted ref remains the explicit trust boundary.
  • Runtime validation: the final-head dispatch reached the dedicated job, proved the stable CLI launcher, Docker authentication and cleanup, live scenario, artifact upload, and post steps. The downloaded artifact passed, recorded both isolation directions as STATUS_403, removed both sandboxes and the shared gateway registration with zero cleanup failures, and had no credential-shape matches.
  • Gateway recovery: TC-SBX-06 remains an explicit gap rather than being forced green. This runner had no legacy gateway container and recorded skipped-gateway-absent, matching the frozen v0.0.69 soft-skip. The Complete the Vitest E2E cutover with job-variant parity evidence #5919 row remains unchecked until an applicable run proves recovery or reviewers disposition the legacy contract.
  • Legacy cleanup compatibility: feature detection is safer than version parsing here. Fallback is allowed only for an explicit unsupported gateway remove; all other current-command failures fail closed. Tests cover current success/absence/failure and legacy success/absence/failure. The compatibility comment states the removal condition.
  • Piped stdin: it was not part of the signed v0.0.69 sandbox-operations contract, so this parity PR does not add it as a new requirement. Frozen failed-tool provenance behavior remains covered deterministically.
  • Other suggestions: remove reporting already absent returns before legacy destroy, so the proposed double-absent sequence is unreachable by design. The default workflow constant is the validator's injection seam. Splitting the central workflow validator is worthwhile architecture work, but is outside this isolated parity slice.

No remaining advisor item changes the scoped implementation. Human review is still required, and the unresolved TC-SBX-06 evidence decision remains visible in #5919.

@cv
cv merged commit 7b9267d into main Jun 28, 2026
114 checks passed
@cv
cv deleted the codex/e2e-sandbox-operations-parity branch June 28, 2026 22:26
@miyoungc miyoungc mentioned this pull request Jun 30, 2026
21 tasks
jyaunches pushed a commit that referenced this pull request Jun 30, 2026
## Summary
Refreshes the v0.0.70 release docs from the release announcement and the
`v0.0.69..v0.0.70` commit range.
It also documents the `channels start` policy restoration behavior that
was missing from the shared OpenClaw and Hermes command references, and
bumps the Fern CLI version used for docs validation.

## Changes
- Replaced the stale `v0.0.70` release-notes entry with the actual
release themes, including CLI, onboarding, inference, messaging,
Windows, documentation, and release-validation changes.
- Documented that `channels start` reapplies the matching built-in
network policy preset before rebuild and rolls back to disabled if
policy restoration fails.
- Bumped `fern/fern.config.json` from `5.55.0` to `5.59.0` for the docs
refresh.
- Source summary:
- #5754 -> `docs/about/release-notes.mdx`: Notes Docker Desktop gateway
bridge retry behavior during onboarding.
- #5930 -> `docs/about/release-notes.mdx`: Links `nemoclaw use` default
sandbox selection to the command reference.
- #5948 -> `docs/about/release-notes.mdx`: Links reasoning-compatible
endpoint validation to inference documentation.
- #5950 -> `docs/about/release-notes.mdx`: Links Windows bootstrap WSL
recovery behavior to Windows preparation and troubleshooting docs.
- #5856 -> `docs/about/release-notes.mdx`: Notes rebuilt policy preset
registry repair.
- #5882 and #5949 -> `docs/about/release-notes.mdx`: Notes Hermes stale
base-image state repair.
- #6016 -> `docs/reference/commands.mdx`,
`docs/reference/commands-nemohermes.mdx`, and
`docs/manage-sandboxes/messaging-channels.mdx`: Documents channel policy
restoration and rollback on `channels start`.
- #5859 -> `docs/about/release-notes.mdx`: Links quickstart network
approval guidance.
- #5863 -> `docs/about/release-notes.mdx`: Links Teams allowlist
guidance in the messaging page.
- #5756, #5926, #6010, and #6011 -> `docs/about/release-notes.mdx`:
Summarizes the Vitest E2E validation cutover.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only prose refresh with
no runtime behavior change.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [x] 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)

`npm run docs` exited 0 and Fern reported one existing light-mode accent
contrast warning.
`fern check --warnings` confirmed the warning is the site theme contrast
ratio, not content introduced by this PR.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary

Makes the existing Vitest sandbox-operations replacement reachable from
the cutover workflow. The new default-enabled, selectively dispatchable
job preserves the legacy Ubuntu/Docker/OpenShell boundary while using
Vitest's structured artifacts and cleanup reporting instead of the
legacy shell log streamer; it also fixes the previously unexercised
replacement to configure hosted-compatible inference rather than
treating that credential as a Build API key.

## Related Issue

Refs NVIDIA#5919

## Changes

- add `sandbox-operations-vitest` to the Vitest E2E workflow and PR
result aggregation
- run the existing live replacement with isolated Docker auth, the
compiled CLI, OpenShell, and hosted inference
- configure both sandbox onboard operations through the shared
hosted-inference abstraction
- add a focused workflow-boundary test for inventory, default dispatch,
and both selective-dispatch inputs

## 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: CI-only E2E reachability; no
user-facing behavior or interface 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:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)

Targeted evidence: `npx vitest run --project e2e-vitest-support` (54
files, 386 tests); `npm run typecheck:cli`; source-shape and test-size
ratchets; normal commit and push hooks.

---

Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Tests**
* Added a new live Vitest E2E run for sandbox operations and included it
in PR E2E scenario status reporting.
* Expanded workflow-boundary validation to block unsafe Docker auth and
environment/config scoping, enforce required secret placement, and
verify consistent artifact upload and Docker cleanup behavior.
* Refined live sandbox assertions (including agent exit behavior),
improved gateway recovery reporting, and updated scenario outputs.

* **Chores**
* Updated sandbox onboarding to use a hosted inference configuration
with consistent per-sandbox environment overrides and improved API-key
redaction handling; streamlined cleanup/onboarding flow for reliability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
Refreshes the v0.0.70 release docs from the release announcement and the
`v0.0.69..v0.0.70` commit range.
It also documents the `channels start` policy restoration behavior that
was missing from the shared OpenClaw and Hermes command references, and
bumps the Fern CLI version used for docs validation.

## Changes
- Replaced the stale `v0.0.70` release-notes entry with the actual
release themes, including CLI, onboarding, inference, messaging,
Windows, documentation, and release-validation changes.
- Documented that `channels start` reapplies the matching built-in
network policy preset before rebuild and rolls back to disabled if
policy restoration fails.
- Bumped `fern/fern.config.json` from `5.55.0` to `5.59.0` for the docs
refresh.
- Source summary:
- NVIDIA#5754 -> `docs/about/release-notes.mdx`: Notes Docker Desktop gateway
bridge retry behavior during onboarding.
- NVIDIA#5930 -> `docs/about/release-notes.mdx`: Links `nemoclaw use` default
sandbox selection to the command reference.
- NVIDIA#5948 -> `docs/about/release-notes.mdx`: Links reasoning-compatible
endpoint validation to inference documentation.
- NVIDIA#5950 -> `docs/about/release-notes.mdx`: Links Windows bootstrap WSL
recovery behavior to Windows preparation and troubleshooting docs.
- NVIDIA#5856 -> `docs/about/release-notes.mdx`: Notes rebuilt policy preset
registry repair.
- NVIDIA#5882 and NVIDIA#5949 -> `docs/about/release-notes.mdx`: Notes Hermes stale
base-image state repair.
- NVIDIA#6016 -> `docs/reference/commands.mdx`,
`docs/reference/commands-nemohermes.mdx`, and
`docs/manage-sandboxes/messaging-channels.mdx`: Documents channel policy
restoration and rollback on `channels start`.
- NVIDIA#5859 -> `docs/about/release-notes.mdx`: Links quickstart network
approval guidance.
- NVIDIA#5863 -> `docs/about/release-notes.mdx`: Links Teams allowlist
guidance in the messaging page.
- NVIDIA#5756, NVIDIA#5926, NVIDIA#6010, and NVIDIA#6011 -> `docs/about/release-notes.mdx`:
Summarizes the Vitest E2E validation cutover.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only prose refresh with
no runtime behavior change.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [x] 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)

`npm run docs` exited 0 and Fern reported one existing light-mode accent
contrast warning.
`fern check --warnings` confirmed the warning is the site theme contrast
ratio, not content introduced by this PR.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 16, 2026
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure chore Build, CI, dependency, or tooling maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants