Skip to content

ci(typecheck): give CLI type checking a 5 GiB heap - #11356

Merged
cv merged 2 commits into
mainfrom
codex/ci-cli-typecheck-heap
Sep 10, 2026
Merged

ci(typecheck): give CLI type checking a 5 GiB heap#11356
cv merged 2 commits into
mainfrom
codex/ci-cli-typecheck-heap

Conversation

@cv

@cv cv commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Outcome

The shared CLI and test type-check step receives a 5 GiB JavaScript heap. This gives the current TypeScript program enough memory to finish instead of aborting near the default 4 GiB limit.

Reason

The build-typecheck job on #11353 passed all 1,299 package-contract tests, then aborted during npm run typecheck:cli with JavaScript heap out of memory and exit 134.

Changes

Set NODE_OPTIONS: --max-old-space-size=5120 on Typecheck CLI + tests in .github/actions/ci-build-typecheck/action.yaml. Both PR and main CI use this shared action. The value matches the existing CLI heap remediation in scripts/dev-setup.sh.

Verification

  • Node.js 22.23.1 reproduction on the original failing compiler inputs: --max-old-space-size=4096 reproduced exit 134; --max-old-space-size=5120 passed and reported Memory used: 4356061K.
  • Executed the changed YAML step's command and environment under Node.js 22.23.1 on this branch — npm run typecheck:cli passed.
  • npx vitest run --project integration test/automation/pull-requests/pr-workflow-contract.test.ts — all 44 tests passed.
  • npm run validate:pr — passed against canonical base a4265abfc2e46922100baff0ba8f5985b681cf69, with unchanged local validator sources and executables.
  • git diff --check and secret scans — passed. The diff contains no secrets, API keys, or credentials.

Review notes

Self-reviewed NVIDIA/NemoClaw commit 35bc9934af317cff2cdc0ea7da0310f64986676f and the changed .github/actions/ci-build-typecheck/action.yaml against both callers, the existing setup remediation, and the executed type-check result. No actionable finding. Independent review of this sensitive path is pending; this PR is a draft.

PR CI loads this action from its base commit. This PR's CI can still encounter the old heap limit until the shared action change lands on main. The local execution above exercises the proposed setting directly.


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

Summary by CodeRabbit

  • Chores
    • Increased the memory available to CLI and test type-checking workflows, improving reliability for larger builds.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Sep 10, 2026
@copy-pr-bot

copy-pr-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 860be754-fb65-41e0-9bc7-5e84e0bc377e

📥 Commits

Reviewing files that changed from the base of the PR and between c8cbebc and 9504a78.

📒 Files selected for processing (1)
  • .github/actions/ci-build-typecheck/action.yaml

📝 Walkthrough

Walkthrough

The CI typecheck action sets NODE_OPTIONS to allow a 5120 MB Node.js heap for CLI and test typechecks.

Changes

Typecheck configuration

Layer / File(s) Summary
Increase typecheck heap limit
.github/actions/ci-build-typecheck/action.yaml
The CLI and test typecheck step sets NODE_OPTIONS=--max-old-space-size=5120.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: apurvvkumaria, cjagwani

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/ci-cli-typecheck-heap

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

@github-code-quality

github-code-quality Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 9504a78 in the codex/ci-cli-typeche... branch remains at 96%, unchanged from commit c8cbebc in the main branch.


Updated September 10, 2026 02:00 UTC

@cv
cv marked this pull request as ready for review September 10, 2026 02:03
@cv
cv merged commit b7198be into main Sep 10, 2026
42 of 44 checks passed
@cv
cv deleted the codex/ci-cli-typecheck-heap branch September 10, 2026 02:03
cv pushed a commit that referenced this pull request Sep 10, 2026
## Outcome

Supply verified mcporter audit evidence to the offline protected image
build. This prerequisite lets trusted-main E2E exercise #11156 without
executing its candidate workflow as trusted code.

## Reason

The protected rebuild uses `--network none` and `--no-cache`, but its
controller does not supply audit evidence. OpenClaw therefore attempts a
live npm audit without network access.

The [failed protected
job](https://github.com/NVIDIA/NemoClaw/actions/runs/34375097855/job/102555650142)
returned exit 1, `EAI_AGAIN`, 206 bytes, and `metadata:missing` on both
attempts. Both responses had SHA-256
`2ca8dc3553861955190f44adaa5ed386851b1c217d7e5084d6bc150ddb8623b4`.
Runtime qualification did not run. This is separate from the
receipt-identity failure fixed by #11069.

### Related issues

Refs #11088. Prerequisite for #11156, which owns the complete fix and
protected-runtime acceptance.

## Changes

- Run the existing trusted audit action before the offline build. Reuse
matching cached evidence or refresh it through that action's existing
networked path.
- Verify candidate manifests, lockfile, raw report, receipt, npm
identity, and exception policy with the existing trusted verifier. Pass
the verified evidence and hashes through BuildKit secrets. The derived
policy result supports #11156's consumer.
- Transfer the existing controller/workflow assertions from #11156. No
new test scenario, audit subsystem, workflow job, Dockerfile, registry
fallback, or retry is introduced.

The seven-file diff is +234/-7. The build path changes only the workflow
handoff and its controller. The remaining changes protect that boundary.
Main's merged CI memory fix (#11356) is consumed without duplication.

## Verification

- Existing affected audit, controller, and workflow tests: 313 passed
across 12 files.
- `npm run build:cli` and `npm --prefix nemoclaw run build`: passed.
- `npm exec -- vitest run --project package-contract
test/package-contract/managed-image-registry-transport.test.ts`: one
test passed.
- Normal pre-commit and commit-message hooks: passed, including
repository checks, ShellCheck, secret scanning, and source-shape/growth
checks.
- `NODE_OPTIONS=--max-old-space-size=5120 npm run validate:pr`: passed
at `d2c864d4accfacdfe6825ded68d12e9b3fc7b462` against canonical main
`270275f2a2b31a70fa72692d2ec304b5dffe8ee3`. No tracked file changed.

<details>
<summary>Focused test command</summary>

```sh
npm exec -- vitest run --project integration \
  test/platform/images/protected-managed-image-build-script.test.ts \
  test/automation/releases/reviewed-npm-audit.test.ts \
  test/automation/releases/npm-audit-receipt.test.ts \
  test/automation/releases/reviewed-npm-audit-handoff.test.ts \
  test/automation/releases/reviewed-npm-audit-workflow.test.ts \
  test/automation/releases/reviewed-npm-audit-cache-key.test.ts \
  --project e2e-support \
  test/e2e/support/e2e-operations-workflow-boundary.test.ts \
  test/e2e/support/managed-image-protected-runtime-workflow.test.ts \
  test/e2e/support/hermes-workflow-boundary.test.ts \
  test/e2e/support/jetson-workflow-boundary.test.ts \
  test/e2e/support/staging-brev-launchable-identity-workflow-boundary.test.ts \
  test/e2e/support/shared-e2e-workflow-boundary.test.ts --reporter=default
```

</details>

Validation used pinned Node.js 22.23.2 and verified npm 10.9.4 in an
unprivileged Linux container without host mounts, credentials, or a
Docker socket. Signed candidate
`d2c864d4accfacdfe6825ded68d12e9b3fc7b462` has the same source tree as
the hook-tested isolated commit. The CI memory setting comes from merged
#11356.

No secrets, API keys, or credentials are in the diff. Incomplete,
invalid, or unaccepted high-severity audit results still fail closed.
Completeness checks, exception policy, raw-report and receipt integrity,
registry selection, and retry bounds are unchanged. No broad-suite or
protected-runtime pass is claimed.

## Review notes

Self-review covered all seven paths at
`d2c864d4accfacdfe6825ded68d12e9b3fc7b462` in NVIDIA/NemoClaw. Sensitive
paths are `.github/workflows/e2e.yaml`,
`scripts/checks/build-protected-managed-images.sh`, and the two changed
`tools/e2e/` validators. Their independent review remains pending.
Changed validators ran only in the approved credential-free environment;
this is not independent security approval.

Protected GPU/local-inference acceptance remains pending. This
prerequisite must enter trusted main through normal review and merge
gates before dispatching `jobs=managed-image-protected-runtime` for
#11156. MCP or image-startup results do not substitute for that job. No
CI waiver is claimed.

The controller's private policy result follows its existing exit/signal
cleanup. No protected registry, builder, container, image, or credential
file was created by the local tests; Docker commands in controller tests
are stubbed.

---

Signed-off-by: San Dang <sdang@nvidia.com>


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

## Summary by CodeRabbit

- **Security Improvements**
- Protected managed-image builds now require verified, reviewed npm
audit evidence before importing caches or building images.
- Builds reject incomplete or unverifiable audit evidence before Docker
execution.
- Audit results are bound to the candidate runtime and policy
configuration, with integrity hashes passed into the protected build.
- Workflow validation ensures audits and builds run through trusted
controls rather than candidate-provided scripts.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: San Dang <sdang@nvidia.com>
cv added a commit that referenced this pull request Sep 10, 2026
## Outcome

CI selects cache restore/save actions that declare Node.js 24. All five
pins move from v4.2.4 to v6.1.0, and the existing managed-image workflow
test expects the new pins.

## Reason

GitHub warns that the pinned v4.2.4 actions target deprecated Node.js 20
and are being forced to run on Node.js 24.

## Changes

- Pin three `actions/cache/restore` calls and two `actions/cache/save`
calls to v6.1.0 (`55cc8345863c7cc4c66a329aec7e433d2d1c52a9`) in
`ci-compile-artifacts` and `ci-reviewed-npm-audit`.
- Update the three exact-pin assertions in the reviewed audit workflow
test, retaining its cache identity and trusted-write checks.
- Audit all external actions under `.github`, including transitive
composite actions and the reusable workflow. Across 34 unique
references, cache restore/save were the only actions declaring Node.js
20.
- Consume the merged CLI type-check heap fix from #11356. The PR diff
remains the two cache actions and their test expectations.

## Verification

- `npx vitest run --project integration
test/automation/pull-requests/compiled-artifact.test.ts
test/automation/pull-requests/pr-workflow-contract.test.ts
test/automation/releases/reviewed-npm-audit-cache-key.test.ts
test/automation/releases/reviewed-npm-audit-workflow.test.ts
test/inference/managed/managed-image-publication-workflow.test.ts` — 133
tests passed across five files. The managed-image workflow file
reproduced the stale-pin failure before repair and passes afterward.
- `npm run validate:pr` — passed against canonical base
`270275f2a2b31a70fa72692d2ec304b5dffe8ee3` after verifying the
validation surface and resolved validator identities.
- Authenticated upstream audit — verified the v6.1.0 tag and distributed
restore/save entry points, both declaring `node24`. Upstream build,
distribution, and Linux/macOS/Windows restore/save checks passed for the
pinned commit.
- Compatibility review — v5 requires runner 2.327.1 or newer; affected
callers already use Node.js 24 actions. v6 includes ESM module metadata,
and v6.1 keeps denied cache saves nonfatal. Cache keys, paths, hit/miss
handling, and audit trust boundaries remain compatible.
- Candidate action audit — no reachable action manifest declares Node.js
20. No old cache SHA remains in `.github` or `test`.
- `git diff --check` and hook secret scans — passed. The diff contains
no secrets, API keys, or credentials.

## Review notes

Self-reviewed NVIDIA/NemoClaw commit
`dfd169efa1d95103879d2fc2be33a41d922ee117` and its three-file diff
against canonical base `270275f2a2b31a70fa72692d2ec304b5dffe8ee3`. The
sensitive paths are `.github/actions/ci-compile-artifacts/action.yaml`
and `.github/actions/ci-reviewed-npm-audit/action.yaml`; review checked
the upstream manifests, cache consumers, and trust assertions. No
actionable finding remains in the local repair.

CodeRabbit completed review through
`dfd169efa1d95103879d2fc2be33a41d922ee117` with no actionable comments.
All nine [Advisor
specialists](https://github.com/NVIDIA/NemoClaw/actions/runs/34429878549)
completed successfully, and their full Markdown reports were read. Eight
reported no findings. The verification specialist suggested adding
exact-SHA assertions for the compiled-artifact cache actions; this is
advisory and was not adopted because it would duplicate the dependency
pin without testing behavior. The existing cache hit/miss and
artifact-integrity tests plus the authenticated upstream action audit
remain the evidence for that change.

PR CI loads these composite actions from its base commit, so it can
still emit the old cache warning until this change merges. Execution of
the new pins in NemoClaw remains a post-merge validation step.

### Remaining CI failure: missing shared-module build

The [required PR CI
run](https://github.com/NVIDIA/NemoClaw/actions/runs/34428843302) passed
for `dfd169efa1d95103879d2fc2be33a41d922ee117`, including
build/type-check and all CLI shards.

The remaining [managed-image-openclaw-security
failure](https://github.com/NVIDIA/NemoClaw/actions/runs/34428843860/job/102720614753)
occurs during test collection: `Cannot find module
'../../nemoclaw/dist/shared/sandbox-name.cjs'`. The job installs root
dependencies with `--ignore-scripts` and does not build the shared
modules imported by its fixture chain. It does not invoke either cache
composite action changed by this PR.

Reproduced the same failure on a clean archive of the PR base
`270275f2a2b31a70fa72692d2ec304b5dffe8ee3` using Node.js 22.23.1 and the
locked root dependencies. `vitest list --project integration
test/e2e-runtime/managed-image-openclaw-security.test.ts` exited 1
before `npm run build:policy-boundary` and exited 0 afterward. With
`NEMOCLAW_TEST_IMAGE` set, it registers the security test. This verifies
the collection prerequisite; the Docker assertions were not run locally.

The minimal separate repair is to run `npm run build:policy-boundary`
after dependency installation in the `managed-image-openclaw-security`
jobs in `.github/workflows/pr-self-hosted.yaml` and
`.github/workflows/sandbox-images.yaml`. Both jobs and the affected
source/configuration are unchanged from the recorded base and remain
unchanged on current canonical main. This inherited failure has no
waiver.

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


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

## Summary by CodeRabbit

- **Chores**
- Updated automated build and security workflows to use the latest cache
action version.
- Existing cache keys, paths, conditions, and workflow behavior remain
unchanged.
- Updated related workflow test references to remain aligned with the
current cache action configuration.
  - No changes were made to product functionality or public interfaces.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
ericksoa added a commit that referenced this pull request Sep 10, 2026
## Outcome

Verified same-user OpenClaw sandboxes retain native `0700/0600` state
permissions through startup, native commands, config editing and
recovery. Separate-user and unverified layouts retain `2770/0660`.

## Reason

Duplicate permission writers reversed native OpenClaw permissions. The
existing guard should own the stateless mode decision, while native
setup initializes agent state. Image-security tests also need their
existing shared module compiled on the host before they can load.

### Related issues

Closes #11257. Part of #11255. Independently based on main after #11268.

## Changes

- Reuse the guard's process/readiness proof to select private or shared
modes. The normalizer imports the root-owned helper; its CLI actions
still require root. Marker inspection requests metadata access while
retaining path, inode and owner checks.
- Remove duplicate shell permission walkers, fixed host-mode verdicts
and onboarding overrides. Repair keeps the existing lifecycle lock and
pinned container identity. Only conclusive mode mismatches are
repairable; ownership, flags, JSON, transaction and recovery refusals
remain.
- Validate existing configuration before native `openclaw setup
--baseline`, refresh its hash, and propagate normalizer failures.
Preserve managed Docker/Podman behavior and upstream-secret custody.
- Consolidate duplicated test fixtures and keep classification coverage
with its existing owner. Retain journal replay, ownership, native doctor
and first-start gateway identity coverage; correct the existing
documentation.
- Prepare image-test host prerequisites once: the shared install step
calls the existing `build:policy-boundary`, and OpenClaw security reuses
that step. This fixes the missing `sandbox-name.cjs` import in OpenClaw,
Hermes and runtime-override suites without changing their test bodies or
cleanup.

Production remains **nine lines smaller** than main. Conflict resolution
introduces no additional runtime behavior.

## Verification

Current candidate: `9f6dd6b79e8ee4c5d1113fd1dc71d7d833ef0712`. It merges
main through `915f34957c6b190603562541f8a6c33dc238837c` to resolve the
full-E2E formatting conflict. Independent syntax comparisons confirm the
original PR test logic is preserved, including the automatically merged
overlaps and three tests formatted under main's new rule. No runtime
behavior was added by the resolution.

Conflict-resolution checks: CLI and plugin builds passed; 185 focused
tests passed with four existing skips; all 1,800 live assertions remain;
formatting and independent source-preservation review passed. Canonical
`npm run validate:pr` passed in an isolated validation of this candidate
merged with main `ea68ea444a9f7e27455961ca0412258d47dca489`, using
unchanged trusted validators and fresh dependencies. Core CI passed. Two
managed-image MCP checks remain failed during public-tunnel setup; the
remaining review and merge gates still apply.


One-off local compatibility validation passed for `9f6dd6b` on Linux
ARM64/Colima Docker 29.2.1, OpenShell 0.0.106 and Node 22.23.2. The
baseline CLI and retained managed image came from `915f349` ([qualified
prior
publication](https://github.com/NVIDIA/NemoClaw/actions/runs/34471439130)).
All 23 comparison commands had their expected outcomes: doctor/fix,
native doctor fix, exec exit statuses, config editing, gateway
restart/recovery and completed gateway-backed agent turns. The
container, ARM64 image
`sha256:9838c60b465aa9647b5072b15ddf268d91263ffc0b26836aa18f173dc2c542bf`,
and packaged helpers remained unchanged. The old image retained
2770/0660 modes; workspace/session data and routing/auth settings were
preserved. Native doctor made its own plugin/skills/wizard metadata
updates. Synthetic upstream credentials were absent from the checked
sandbox environment and managed configuration files. Cleanup removed all
test resources and private credentials while preserving the user's
existing container. No permanent test or source change was added. This
is local Docker/ARM64 evidence, not Podman or full GitHub E2E
qualification.

The following qualification evidence applies to the preceding candidate
`123ed3c80efd13d29b3dde53863f60fdf56c2fb7`; it is not presented as a run
of the merge commit.

- [Native Docker/Podman
E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/34441172212),
attempt 1: both runtimes passed all seven phases and four cleanup
actions each. All six selected state-integrity doctor reports had zero
findings, including first startup before repair. Native doctor fix,
native config 119, host edit to 120, restart/recovery, identity checks,
inference, interactive launches, personal profiles and retained security
controls passed. The authenticated receipt binds the exact candidate,
base and trusted controller. Native tests explicitly reused qualified
c0c9c46 images; all runtime/image inputs are unchanged between `c0c9c46`
and `123ed3c`.
- [Packaged image
contracts](https://github.com/NVIDIA/NemoClaw/actions/runs/34441197506),
attempt 1: all seven active jobs passed; optional arm64 was not
selected. OpenClaw security and glibc tests passed, including
shared-user gateway writes, root recovery, protected-file refusals,
volume removal and final labeled-resource absence checks. Hermes's
secret-boundary and six root-entrypoint tests passed, as did runtime
overrides. The same-run image archive and test-image identity were
verified.
- [Fresh managed-image
qualification](https://github.com/NVIDIA/NemoClaw/actions/runs/34440929489),
attempt 1: all-agent activation passed all 12 phases and nine cleanup
checks. OpenClaw, Hermes and Deep Agents Code each completed agent turns
before and after gateway restart while preserving durable state. Both
MCP discovery passes passed all five phases and ten cleanup checks,
including denied-auth cases. These newly built images form cohort
`ghrun-34440929489-1`; contracts and retained evidence archives were
authenticated. This is separate from the earlier image cohort reused by
native E2E.
- [Core
CI](https://github.com/NVIDIA/NemoClaw/actions/runs/34440929557),
attempt 1: all twelve test shards, typechecking, coverage aggregation
and the final gate passed. The existing main #11356 typecheck heap fix
remains in use.
- Canonical `npm run validate:pr` passed against main
`f1a5bc1031babb1d7ed15baa8fa2a6a53c76b6df` in isolated validation merge
`0fd9045bfc211cecf06b69ba84ae0095b698ad3c`, with a clean tree. The
reviewed image workflow was YAML data for unchanged validators.
- The missing host-module failure was reproduced before compilation on
both candidate and exact base. The existing compiler fixed collection of
all nine affected test definitions; the remote results above confirm the
actual tests now execute successfully.
- `npm run docs`, generated agent variants and all 69 guarded routes
passed. Five existing Fern warnings outside the corrected passage
remain.

Broad native doctor remains separately recorded: exit 1 with 34 warnings
per runtime, comprising 30 skill-dependency warnings and four generic
security messages concerning the inference placeholder and sandbox-local
gateway token. The selected state-integrity checks are clean. No
exhaustive live secret scan is claimed.

The diff contains no secrets, API keys or credentials. Prior failed CI,
image and image-contract attempts remain preserved. The earlier c0
activation failure was followed by one targeted activation-only rerun
using unchanged images; its underlying pre-readiness cause remains
unclassified. The native, shared-image and fresh image qualification
runs listed above passed on their first attempt for the preceding
candidate.

## Review notes

NVIDIA/NemoClaw sensitive paths include `scripts/`, `src/lib/sandbox/`,
`src/lib/onboard/` and the image workflow. All nine c0c9c46 Advisor
artifacts and paginated PR feedback were collected. The stale
documentation finding is fixed here; the root-marker and nested-warning
claims were rechecked against their existing source owners and excluded.
The unbounded config-sync request remains an advisory observation: no
hang or connection to the observed image-test failure was demonstrated,
so this repair adds no timeout policy.

The setup repair has independent before/after collection evidence and
root review; the one-line documentation deletion has documentation
validation and root review. CodeRabbit completed an actual review of
`123ed3c` with no new inline findings. All nine applicable Advisor
artifacts were reverified, and all six review threads remain resolved. A
later self-hosted security job collected zero tests because of an
inherited missing host-module prerequisite; that separate workflow
defect is preserved and is not part of this conflict repair. CI and
review for `9f6dd6b` must settle before merge. The PR remains ready for
review and unmerged; no admin bypass is requested.

---
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>

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

- **New Features**
- Verified same-user launches now use private OpenClaw configuration
permissions (`0700/0600`); separate or unverified identities retain
shared access (`2770/0660`).
- Startup, recovery, synchronization, and permission repair consistently
apply the selected security layout.
- Added stronger validation for ownership, directory permissions,
immutable flags, and configuration integrity.

- **Bug Fixes**
- Permission cleanup now fails safely when posture cannot be verified or
repaired.
- Configuration synchronization rejects symlinked state paths and
surfaces permission errors.
- Doctor reports clearer warnings for unsupported or unverifiable
sandbox permission checks.

- **Documentation**
- Updated security, troubleshooting, command, and filesystem guidance to
describe permission behavior and recovery requirements.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
cv pushed a commit that referenced this pull request Sep 10, 2026
<!-- markdownlint-disable MD041 -->
## Outcome

An intentionally stopped sandbox reports `Stopped` with exit code 0 when
provider observation confirms the stop. Status preserves the stopped
sandbox and suppresses recovery and inference probes.

## Reason

The stop command stopped the container but did not preserve the
operator's intent. Status therefore treated the expected stop as an
outage and suggested recovery.

### Related issues

Fixes #11025.
Supersedes #11091 because required trusted package CI does not run for
fork PRs.
Consumes the merged dependency repairs #11264 and #11356 (5 GiB CLI
type-check heap).

## Changes

- Persist stop intent only after a successful managed stop, through the
sandbox registry.
- Clear that record after a confirmed start or replacement, before
downstream recovery can fail. Report persistence failures instead of
silently continuing.
- Require both recorded intent and provider confirmation before
reporting a clean stop. Preserve other preflight and gateway errors.
- Cover lifecycle persistence, stale records, recovery suppression, and
public text and JSON status. The registry record is necessary because a
stopped container alone cannot establish operator intent.
- Integrate canonical main to consume the patched js-yaml dependency.
Preserve both stop-intent and Hermes Portable forward-repair assertions
in the overlapping start test.

## Verification

### Latest dependency validation

Current candidate: `4082dfdece3dead650e0b70ac66bcc835024185c`. Preserves
Prekshi’s stop-state classification fixes and documentation, and
integrates merged fixture-lifetime repair #11465. The failing test used
a mock cloudflared process with a two-second lifetime; this was not an
upstream Cloudflare outage.

CLI and plugin builds passed. All 13 MCP fixture tests and 56 focused
status tests passed. All three `validate:pr` stages passed with a 5 GiB
heap against the integrated canonical bug-fix baseline
`95e578095c6cf99d400f25768e7f862c7b6ccd49`, using that pinned SHA for
`--from-ref` and `--from` as authorized. Fresh GitHub CI is pending.
Prior Advisor service-budget failure disposition remains unchanged.


Candidate: `393fac7bd7e659d90ec89c68b97b844c99113ba1`.

- All three `validate:pr` stages passed with
`NODE_OPTIONS=--max-old-space-size=5120`: `npx prek run --from-ref
d8fb882 --to-ref HEAD --stage
pre-commit`, `npx commitlint --from
d8fb882 --to HEAD`, and the same Prek
command with `--stage pre-push`. This uses the explicitly
user-authorized pinned baseline instead of the moving `origin/main` ref.
- Latest refreshed lifecycle/status tests: 138 of 139 passed. The
remaining active-Hermes stop test reads the real local config directory
and fails because it is not writable in the sandbox. The identical error
reproduces on recorded canonical base
`ea68ea444a9f7e27455961ca0412258d47dca489`; it is
inherited/environmental, not introduced by this candidate. No runtime or
test timeout changes were committed.
- Latest real CLI intentional-stop cases: both passed after the
dependency refresh. CLI and plugin builds passed.
- `npx vitest run --project integration
test/cli/sandbox-status-text.test.ts -t 'clean Stopped state'`: both
cases passed. They run stop and verify text/JSON status with either a
missing live lookup or a present lookup reporting `Provisioning`. The
first run hit a stop deadline in the existing missing case; adding
failure-output diagnostics and repeating the two cases passed without
timeout or runtime changes.

- Eight focused CLI suites: 191 of 192 tests passed together. The
remaining rebuild test timed out during shared host-fence contention;
its full 22-test suite passed when run alone with `--maxWorkers=1`.
- `npx vitest run --project integration
test/cli/sandbox-status-text.test.ts`: eight of nine passed, including
intentional-stop text and JSON behavior. The existing Docker-unreachable
test exceeded the subprocess deadline. It passed unchanged with
`NEMOCLAW_EXEC_TIMEOUT=30000 NEMOCLAW_TEST_TIMEOUT=60000` and
`--testTimeout=60000`.
- `npm run build:cli`, `npm --prefix nemoclaw run build`, and `npm run
typecheck:cli`: passed.
- `npm run docs`: passed again after the lifecycle-guide repair and
canonical integration, with zero errors and five warnings. Generated
OpenClaw and Hermes variants show the correct `start` command.
- Five affected lifecycle/status suites after integration passed 116 of
117 tests; the known slow rebuild fixture exceeded 5 seconds. The full
start and rebuild suites then passed all 65 tests with `--maxWorkers=1
--testTimeout=15000`. No runtime or checked-in timeout changes.
- Canonical startup recovery now returns promises. The merge preserves
stop-intent persistence and updates its failure-test mock to the async
contract; final PR validation includes the corrected types.
- `npm run review:local`: unavailable. Trusted preparation completed
after a sandbox DNS failure, but OpenShell gateway connection was
refused before any specialist ran. Temporary review cleanup also
reported EACCES. This is not successful review evidence.
- Validator sources, manifests, lockfiles, and configuration match
pinned canonical baseline `d8fb8822833b6c60d3a1d984b9d0c6b5d7f6d56f`.
Independently installed dependency inventories matched across 31,807
entries before generated build/cache output.
- The diff contains no secrets, API keys, or credentials.

## Review notes

All nine complete Advisor specialist writeups from [run
34418099043](https://github.com/NVIDIA/NemoClaw/actions/runs/34418099043)
were read for commit `f2e81ff99c92d4eebf09f635e372f06feba7fbf7`. The
earlier `Provisioning` regression-test finding is cleared. The
documentation specialist identified two lifecycle-guide instructions
that did not distinguish intentional stops from failures. The repair now
routes clean `Phase: Stopped` to `start` and stopped-container failures,
missing sandboxes, or unhealthy gateways to `recover` for OpenClaw and
Hermes. Fresh review is required before readiness.

CodeRabbit reports minimal risk and no current-head blocker on the
reviewed commit. All six review threads are resolved. Its generic
docstring-coverage warning is advisory; the specialist reviews
identified no missing code behavior explanation.

All checks passed on the reviewed commit, including [full
CI](https://github.com/NVIDIA/NemoClaw/actions/runs/34416907284) and
[managed image
validation](https://github.com/NVIDIA/NemoClaw/actions/runs/34416907199).
The inherited js-yaml audit failure and prior Pi Perl build failure no
longer reproduce. Fresh checks and reviews are required for this
candidate. No GitHub check or human approval is waived.

The previous published head `595d0f979a498fcc09360dd3a0cdb9ecc61835ed`
failed only the CLI type-check memory gate and its dependent aggregate:
Node exhausted its default 4 GiB heap without TypeScript diagnostics.
The merged shared CI repair #11356 raises that step to 5 GiB, now
integrated and validated locally. CodeRabbit had no actionable finding
on that head; its warning about channel/migration documentation concerns
inherited canonical text outside this diff. Advisor did not run because
CI was not green.

Ho Lim remains the original contributor; the maintainer repair preserves
both DCO declarations.

---
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>


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

* **New Features**
* Sandbox status now distinguishes intentional stops from unexpected
container failures.
* Intentionally stopped sandboxes report a clean `Stopped` state without
unnecessary health checks or recovery attempts.
* JSON status accepts confirmed stopped sandboxes without a gateway as
valid.
* Starting, connecting to, rebuilding, or recovering a sandbox clears
its stopped state after successful recovery.
* Portable lifecycle diagnostics now include bounded startup and
recovery timing receipts.

* **Bug Fixes**
* Improved handling of failed stop-state updates with actionable retry
guidance.
* Prevented inappropriate recovery guidance for schema mismatches and
confirmed stops.

* **Documentation**
* Updated permission, troubleshooting, and recovery guidance for sandbox
layouts and stopped states.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Ho Lim <subhoya@gmail.com>
Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas pushed a commit that referenced this pull request Sep 10, 2026
## Outcome

The focused EXDEV lifecycle now uses and verifies the canonical
OpenShell executable when restarting the gateway, then terminates that
verified listener before sandbox recreation. It fails at that boundary
if the dashboard port does not release. The trusted-image wrapper
remains limited to sandbox creation and recreation, where its image
rewrite is required.

## Reason

The automatic main E2E selected after #10808 first failed because the
restart path treated a listener owned by canonical OpenShell as foreign
when the test wrapper remained configured. Two focused runs then exposed
the remaining lifecycle defect: cleanup stopped the port without the
sandbox and gateway identity, OpenShell reported no active forward, the
valid restart listener remained bound to port 18789, and recreation
moved to 18790.

### Related issues

Relates to #10808

## Changes

- Override `NEMOCLAW_OPENSHELL_BIN` with the resolved canonical
OpenShell executable for gateway restart.
- Verify the restarted dashboard listener is owned by canonical
OpenShell, using the production ForwardTcp argument builder as the
command authority.
- Terminate only the listener whose executable and complete command line
prove canonical OpenShell ownership, then require bounded dashboard-port
release before recreation.
- Carry the configured dashboard port through the same child environment
used by onboarding, listener inspection, and cleanup.
- Cover wrapper ownership rejection and targeted cleanup in the existing
fast E2E client tests.

## Verification

- `npx vitest run --project e2e-support
test/e2e/support/e2e-clients.test.ts`: 82 tests passed.
- `NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli`:
passed.
- `npm run checks:repository`: passed, including the 1,798-assertion
ratchet.
- `npm run test:e2e-phases:check`: 134 tests across 88 files passed.
- `NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr`: passed
for commit under review `29f98461c60a029010bdab608775cb4290daa3b6`
against canonical main `194324d6d07e38fadf9a64d875b09e3c7eb02f8d`.
- Focused manual E2E [run
34536058612](https://github.com/NVIDIA/NemoClaw/actions/runs/34536058612)
passed `openclaw-plugin-runtime-exdev` for exact candidate
`29f98461c60a029010bdab608775cb4290daa3b6` on recorded base
`3bfc4a83e413b7f56a327130815fa91bcbf0745f`; its immutable dispatch
receipt, risk signal, target result, first-attempt retry evidence, and
complete cleanup record were verified.
- Pre-commit, commit-message, and pre-push hooks passed.
- GitHub marks commit under review `29f98461c6` Verified.
- The diff contains no secrets, API keys, or credentials.

## Review notes

- Before-fix evidence: [automatic main E2E run
34383659825](https://github.com/NVIDIA/NemoClaw/actions/runs/34383659825),
failed job 102575200782.
- Focused run
[34411638365](https://github.com/NVIDIA/NemoClaw/actions/runs/34411638365)
proved recreation moved to port 18790 while the restart forward retained
18789.
- Focused run
[34417902543](https://github.com/NVIDIA/NemoClaw/actions/runs/34417902543)
proved the unscoped stop returned success with `No active forward found
for port 18789`; the candidate now uses `forward stop 18789 e2e-oc-exdev
--gateway nemoclaw`.
- The complete hosted Advisor artifact set for `c9eab2af59` was
inspected. Its two valid findings were addressed in `e669a9edfa`: the
fixture reuses the production ForwardTcp argument builder, and
onboarding receives the configured dashboard port used by inspection and
cleanup.
- Exact-head hosted Advisor follow-up for `e669a9edfa` was unavailable
because all nine specialists exhausted provider retries with the same
HTTP 429 `budget_exceeded` response and produced no artifacts.
CodeRabbit completed with no actionable finding.
- Focused run
[34505225563](https://github.com/NVIDIA/NemoClaw/actions/runs/34505225563)
reached sandbox recreation and failed restoring workspace state. Its
artifacts were inspected; the sandbox, gateway, images, fixtures,
wrapper, and mock were cleaned. Exact head `9d8759fca0` integrates the
recovery-permission fix from #11309 and the CI heap fix from #11356, so
a fresh exact-head focused run is required.
- Focused run
[34524049458](https://github.com/NVIDIA/NemoClaw/actions/runs/34524049458)
proved the externally discovered listener PID was not its process-group
leader. Commit `72cbe661d8` therefore targets only the listener after
exact ownership proof.
- CodeRabbit found that commit `72cbe661d8` ignored the bounded
port-release result. Commit under review `29f98461c6` fails before
recreation when the port remains reachable, while keeping the live
assertion budget unchanged.
- `npm run review:local` reached the trusted implementation but its
temporary OpenShell gateway never became reachable. It produced no
findings; retained temporary review data was removed after restoring its
owner permissions. No local Advisor finding is claimed.
- No documentation change is needed because this corrects test fixture
lifecycle handling without changing product or E2E contract behavior.

---
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>


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

* **Tests**
  * Expanded end-to-end coverage for cross-device plugin installation.
* Verified dashboard ports become unreachable after forwarding listeners
are terminated.
  * Added listener process identification checks to validate ownership.
* Confirmed wrapper processes are rejected when they do not own the
canonical listener.
* Improved validation of gateway- and sandbox-specific forwarding
targets.
* Added checks to ensure forwarding listeners are cleaned up before
sandbox recreation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant