fix(deps): require patched js-yaml - #11264
Merged
Merged
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
dependencies
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change updates the ChangesYAML parser validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This updates the YAML parser dependency to reject oversized merge sequences and adds coverage at the agent-manifest parsing boundary. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 6e35d18 in the TypeScript / code-coverage/cliThe overall line coverage in commit 6e35d18 in the Show a line coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-11264.docs.buildwithfern.com/nemoclaw |
Contributor
|
PR Review Advisor finished for commit |
prekshivyas
approved these changes
Sep 9, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Approved commit 6e35d18e09df7ef1c49d91937a46b61f68e10d05. I found no blocking or advisory issue.
Security review:
- Secrets and credentials: PASS — the change adds no credential flow.
- Input validation: PASS — the test sends 101 empty merge sources through
parseManifestRecordand requires rejection. - Authentication and authorization: PASS — the change does not alter either boundary.
- Dependencies: PASS — GitHub identifies 4.3.2 as the first fixed 4.x release for GHSA-2883-xcg3-v3hh. The npm archive integrity matches the lockfile, and its loader matches upstream tag
4.3.2. - Error handling and logging: PASS — the parser propagates the rejection without exposing sensitive data.
- Cryptography and data protection: PASS — the change adds no cryptographic or protected-data path.
- Configuration and security headers: PASS — the package range and lockfile both require 4.3.2 or later within major version 4.
- Security testing: PASS — the focused source test passed and covers the vulnerable input at the production parser boundary.
- System security: PASS — the change replaces the vulnerable parser without adding a second parser path or fallback.
I reviewed package.json, package-lock.json, and src/lib/agent/manifest-readers.test.ts. Local npm ci --ignore-scripts --no-audit --no-fund, the focused Vitest test, and npm run build:cli passed. Required CI, CodeRabbit, and all nine Advisor specialists also completed without findings.
This was referenced Sep 9, 2026
This was referenced Sep 9, 2026
prekshivyas
added a commit
that referenced
this pull request
Sep 9, 2026
<!-- markdownlint-disable MD041 --> ## Outcome Publish base and managed images after the root package manifest or lockfile changes. Downstream PR gates can then use publication evidence built from the dependency graph on `main`. ## Reason PR #11264 updated the root dependency graph, but the base-image workflow did not run because its `push.paths` list omitted the root package files. As a result, PR #11098 still sees the earlier failed publication instead of a new audited publication from `main`. ### Related issues Unblocks #11098. Relates to #11264. ## Changes - Add `package.json` and `package-lock.json` to the base-image workflow's exact push paths. - Add a regression test that reads the checked-in workflow and proves that either root package file selects publication. - Keep the existing push-only publication evidence boundary unchanged. A manual dispatch is insufficient because the downstream verifier accepts trusted `push` runs from `main`. ## Verification - `npx vitest run --project e2e-support test/e2e/support/base-image-publication.test.ts --reporter=dot` — passed, 62 tests. - `npm run format:check` — passed. - `npm run test:titles:check` — passed. - `npm run build:cli` — passed. - `npm --prefix nemoclaw run build` — passed. - `npm run validate:pr` — passed. - `git diff --check` and manual diff review — passed; the diff contains no secrets, API keys, or credentials. ## Review notes This changes the sensitive workflow path `.github/workflows/base-image.yaml` in `NVIDIA/NemoClaw` at commit `efd8d768676feeac545b6ab4140590e16cfacf6e`. Author self-review and the focused workflow contract test found no change to permissions, credentials, job code, or publication trust boundaries. No independent pre-publication review is verified; this draft awaits independent review. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Base image publication workflows now trigger when the root package manifest or lockfile changes. * **Tests** * Added coverage to verify that changes to these package files correctly trigger base image publication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
19 tasks
prekshivyas
added a commit
that referenced
this pull request
Sep 9, 2026
…10379) ## Summary NemoClaw probes Docker reachability by running `docker version` in a four-name environment (`HOME`, `USER`, `LOGNAME`, `PATH`), but every Docker command it runs afterwards gets the full subprocess allowlist. On a host whose daemon answers only through one of the dropped names, the probe reports the host's own default authority unreachable, detection falls through to the socket candidates, and the CLI pins `DOCKER_HOST` to Podman's rootless socket. Preflight then reports `Docker is not reachable` and points at the docker group, so onboarding stops at its first step on a host whose Docker is healthy. After this change the probe asks the same question the later commands answer, and detection never redirects the CLI on no evidence. ## Related Issue Closes #10367 This removes the mechanisms that produce the reported outcome: a probe environment narrower than the one the predicted commands run in, and a probe that reaches no verdict counting as a refusal. Either can send `DOCKER_HOST` to Podman's socket on a host whose Docker daemon is live. One honest caveat for whoever merges this. The reporter runs a DGX Spark with Docker and Podman installed; I have no such host and they have not yet answered the two diagnostic commands I asked for on the issue, so the cure is reasoned from the code path, not observed on their machine. If their `docker version` under the old four-name environment turns out to exit `0` quickly, neither fix explains their failure and the issue should be reopened rather than left closed. Two details from the report stay out of scope either way: the docker-group remediation text that names the wrong cause, and the `docker info` versus `docker version` disagreement on an unhealthy daemon. ## Changes - `buildDockerProbeEnv` now selects names with `isSubprocessEnvNameAllowed`, the same allowlist `buildSubprocessEnv` gives real Docker commands, and drops an ambient `DOCKER_HOST` so the probe still pins the authority under test. The probe predicts whether those commands reach a daemon, so it must not ask under a narrower environment: `SSH_AUTH_SOCK` authenticates an `ssh://` Docker context and the proxy names decide how a `tcp://` one is routed. (An earlier revision of this description claimed `XDG_RUNTIME_DIR` selects a rootless daemon socket for the Docker CLI. I tested that and it is false — the CLI ignores a listening `docker.sock` in the runtime directory — so the justification is corrected here and in the code comment.) - `probeDockerHost` reports `inconclusive` when the Docker CLI cannot be spawned or the 3-second probe timeout kills it, and `detectDockerHost` holds the host default in that case. A probe that never answered is not an observed refusal, so it must not move the whole CLI to a fallback socket. - Linux socket candidates are now ordered `/run/docker.sock`, `/var/run/docker.sock`, `/run/user/<uid>/docker.sock`, then Podman's. Rootless Docker's socket sits beside Podman's in the same runtime directory and was never a candidate. - `buildDockerProbeEnv` also applies `withLocalNoProxy`, which `buildSubprocessEnv` gives every real Docker command. Without it, forwarding the proxy names could route a probe of a local `tcp://` authority through a host proxy that the real commands bypass — the same defect class, reintroduced by the fix. - `ci/source-architecture-budget.json`: reading the shared allowlist raises the recorded fan-in of `src/lib/subprocess-env.ts` from 24 to 25. - Onboarding now bounds the existing `docker info` and `docker version` preflight calls at 15 seconds, so preserving an inconclusive default authority cannot leave onboarding waiting without a limit. ## Risk family `src/lib/platform.ts` puts this PR in the tier-3 `platform-install` family, whose required job is `cloud-onboard`. That workflow has no `pull_request` trigger, so it selects on the post-merge push to `main` rather than here. Say the word if you want a `cloud-onboard` run before merge and I will arrange it. ## Not in this PR CodeRabbit's merge-risk note and the PR Review Advisor both point at the mixed-identity bail: when the default authority is dead and both a Docker socket and a Podman socket answer, `detectDockerHost` returns `null` and the CLI keeps its dead default. That path is pre-existing and unchanged here, and removing it reverses a decision recorded in #8823 and #10253, whose security review cited it as a pass criterion. It is a maintainer call, so it is a separate stacked PR — #10387 — with the reversal argued. This PR leaves the guard exactly as it was. ## 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: - [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: pending review on this PR - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: The probe environment stays an allowlist. `test/e2e-runtime/platform.test.ts` fails the probe binary when `NVIDIA_INFERENCE_API_KEY` crosses the boundary, in the new test and in the existing `#8816` one. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: `npx vitest run test/e2e-runtime/platform.test.ts` gives 38 passed, and a focused sweep over the Docker-authority files (`platform`, `runner`, `preflight-docker-host`, `domain/docker-host`, `subprocess-env`, `readiness/host`, `container-engine`, `docker-authority-profile`) gives 187 passed. `npm run typecheck:cli` and `npm run lint` pass. The focused platform and Docker-preflight timeout suites cover 2 files and 40 tests, and the codebase growth guardrails cover 33 tests. All three original probe changes were confirmed red first: without the probe-environment change the default-authority test returns `unix:///run/user/1000/podman/podman.sock` where `null` is expected; without `withLocalNoProxy` that same test fails on the proxy-exclusion guard; and without the no-verdict branch, the test whose Docker CLI dies without an exit status selects the Podman socket. - [ ] Applicable broad gate passed — command/result: not run. The change set is two source functions and their tests. - [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 (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Docker environment detection across Linux setups, including rootless Docker and Podman installations. * Prioritized native Docker sockets for more accurate runtime detection. * Prevented incorrect Docker or Podman classification when the Docker CLI is unavailable or unresponsive. * Preserved relevant runtime and proxy settings while excluding ambient configuration that could cause misleading results. * **Tests** * Expanded coverage for socket prioritization, environment handling, proxy behavior, and inconclusive Docker probes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Merge with `main` (`7d1476a7a1`, 2026-09-09) `reviewed-npm-audit` failed on `b20e7fcd22` with `Tencent WeChat plugin 2.4.3 locked runtime graph lock SHA-256 mismatch`: the branch carried the pre-#11023/#11253 expected hash in `ci/reviewed-npm-audit.json` while the trusted action computes the current one. `main` already records the current hash, so this is a clean merge of `main` (104 commits, no conflicts) with no change to the fix itself. It also picks up the patched `js-yaml` pin from #11264. --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
This was referenced Sep 9, 2026
This was referenced Sep 9, 2026
prekshivyas
added a commit
that referenced
this pull request
Sep 9, 2026
## Outcome Explicit Docker sandbox recreation can publish its registry entry when a later owner-scoped observation is `not_ready`, but only after OpenShell acknowledged the exact replacement's final handoff and its durable identity still matches. A two-phase pending-create checkpoint makes that handoff crash consistent: intent is persisted before the irreversible commit, and completion is persisted only after exact Ready revalidation. Missing, malformed, or changed identities still block publication and retain fail-closed recovery. ## Reason Current OpenClaw recreation can complete the exact stop, start, `Ready`, container, and workspace-restore sequence before OpenShell's next observation returns `not_ready`. The prior exception covered managed bootstrap by implementation mode, so the same proven handoff in the ordinary restart-safe recreation path failed before registry publication. ### Related issues Fixes #10560 ## Changes - Persist a final-handoff commit fence before the irreversible cutover and an acknowledgement after it completes. - On restart, reconcile a fenced handoff by starting the owner-scoped sandbox, proving Ready with the same durable identity, and then acknowledging it. - Let post-create and registry revalidation accept a matching `not_ready` identity only when that receipt exists. - Preserve the receipt when the verified create boundary is persisted again, and require it on native, compatibility, and managed-bootstrap routes. - Add regression coverage for successful and interrupted recreation, failed persistence and commit, absent receipts, and changed identities. ## Verification - `npm run build:cli` - passed. - `npm run typecheck:cli` - passed. - Focused final-handoff and registration suite - 97 tests passed. - Composed registry persistence-to-publication regression plus related lifecycle and registry suites - 163 tests passed. - Isolated registry normalization and route-reservation suite - 70 tests passed. Four cases first timed out under concurrent load, then passed unchanged with one worker. - `npm run validate:pr` - passed against `ece0afbb81f8d7673f478dc6f564d8e4a5436f40`. - Secret scan in the commit and PR validation hooks - passed. - Latest CI repair: npm run test:changed passed 79 changed tests plus 45 growth-guardrail tests; the exact nine-test recreation suite and four-test Hermes provider-publication suite passed; CLI typecheck, Oxlint, Oxfmt, pre-commit, commit-msg, and pre-push checks passed. - Current-main refresh dad5795: 86 focused CLI tests and 4 Hermes publication integration tests passed; plugin build, CLI typecheck, npm run validate:pr, pre-commit, commit-msg, and pre-push checks passed. - Latest current-main refresh 3b4e2a3: 86 focused CLI tests and 4 Hermes publication integration tests passed; CLI typecheck, npm run validate:pr, pre-commit, commit-msg, and pre-push checks passed. - CodeRabbit repair 3213a8e: the recovery message assertion now checks the literal create-attempt label and nonce; its 29-test suite, CLI typecheck, npm run validate:pr, and all hooks passed. - Final CodeRabbit risk repair 08079d7: resumed compatibility recovery now proves that unavailable or changed post-restart identity cannot persist acknowledgement; 31 focused tests, 45 growth-guardrail tests, CLI typecheck, npm run validate:pr, and all hooks passed. - Exact-head Advisor repair `69b930fef1`: legacy schema-1 compatibility checkpoints now pass journal, gateway, generation, and live-identity selection before their commit fence is written; the production resume flow then performs the exact handoff and persists acknowledgement before publication. - Deterministic evidence: 155 focused CLI tests passed; the v0.0.55 installer restore integration suite passed all 4 cases, including production journal selection, pending-checkpoint consumption, exact generation and identity preservation, and workspace-marker access after restoration. - `npm run validate:pr` passed on `69b930fef1`. The local Advisor was retried after the material repair but its temporary OpenShell gateway refused connections; the hosted exact-head Advisor completed all nine specialist reviews with no required change. - Exact-head Advisor repair `db8da2ed27`: nonce-less legacy compatibility checkpoints now resume only when they carry a durable final-handoff commit fence and exact runtime ID; both managed and unmanaged resumes prove that runtime before restart and before acknowledgement. - Added direct positive and negative coverage for the real sole-running-runtime predicate, and corrected the command reference so a verified post-handoff replacement may report not-ready. - Validation on `db8da2ed27`: 166 focused CLI tests, 4 legacy restore integration tests, 45 growth-guardrail tests, CLI type-check, docs validation, `npm run validate:pr`, and all commit and push hooks passed. - `npm run review:local` reached its temporary OpenShell setup but could not start the local gateway; hosted exact-head Advisor evidence is used instead. ## Review notes - The Advisor pass for `4abfb52369` found three valid gaps: nonce-less legacy checkpoints were rejected before handoff reconciliation, managed-bootstrap resume skipped exact Docker runtime proof, and direct running-state evidence plus one documentation correction were missing. Commit `db8da2ed27` closes all three. - The first PR Review Advisor pass found two valid gaps: the handoff acknowledgement was process-local, and the producer-to-publication transfer needed stronger deterministic evidence. Commit `193bb29852` persists the receipt and covers the post-commit transfer, resume, and fail-closed cases. - The second Advisor pass found that managed bootstrap could still bypass the compatibility receipt. Commit `1227ef101c` separates the native managed-bootstrap contract from compatibility recreation and requires the receipt for the latter. - The third Advisor pass found a crash window before receipt persistence, an external managed cutover that did not acknowledge completion, and a boundary rewrite that could discard the receipt. Commit `280e9c1c3d` adds the two-phase fence, restart reconciliation, managed-cutover acknowledgement, and receipt-preserving boundary persistence. - The fourth Advisor pass found that the persistence and publication pieces lacked one composed deterministic proof. Commit `fd1a17486b` adds a real-registry test that proves the commit fence alone cannot publish, persists acknowledgement through compare-and-set, then consumes that exact checkpoint into the final registry entry. - CodeRabbit then found that resumed unmanaged compatibility recovery could call `ensureApplied()` and repeat the already-crossed container recreation. Commit `6b3f9f4842` skips that mutation after a durable commit fence and tests that resume performs no create or runtime-patch replay before exact start and Ready reconciliation. - The latest CodeRabbit pass also identified missing post-restart identity-drift coverage in its full risk writeup. Commit 08079d7 adds unavailable-identity and changed-identity cases and proves neither can acknowledge the handoff. - The latest CodeRabbit pass found a loose interpolated regex in recovery-message evidence. Commit 3213a8e replaces it with literal assertions for the label, nonce, and safety guidance. - The first full CI pass after merging current main exposed an eager final-registration policy lookup before the verified create boundary existed. Commit ad0e8b7 defers that lookup until registration; the exact failing integration suites now pass. - `npm run review:local` did not reach analysis after its dependency retry because its temporary OpenShell gateway refused connections. Cleanup also reported a temporary-file permissions error. - `npm run test:changed` passed 6,383 tests and failed 35 unrelated environment-sensitive tests on this macOS `/private/tmp` worktree. Failures included missing Linux `ip`, trusted-directory rejection of `/private/tmp`, and timeouts. All five changed lifecycle files passed focused tests and `npm run validate:pr`. --- 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** - Added durable tracking for sandbox final-handoff progress and replacement runtime identity. - Resumed handoffs verify the exact replacement runtime, restart when needed, and acknowledge completion before publication. - Same-name replacements are accepted only after live identity and handoff verification. - **Bug Fixes** - Registration is blocked until final-handoff acknowledgement is recorded. - Lifecycle checks remain disabled until commit completion, including failed handoffs. - Recovery guidance preserves diagnostic evidence and directs users through the sandbox destroy workflow. - **Documentation** - Clarified sandbox recovery and agent-specific logging behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Required main-fix refresh Head `7ca6ccb1cddc413b9ae206cd84229f1624cf060a` merges current `main` only because main contains the required js-yaml security fix from #11264. The merge was conflict-free. Focused recovery validation passed 99 tests, `npm run validate:pr` passed, and CLI plus JavaScript-config type checks passed in push hooks. `npm run review:local` was retried, but its temporary OpenShell gateway refused connections during specialist setup. Hosted exact-head Advisor run `34309713228` completed all nine specialist reviews with no required change. --------- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
rsliter
added a commit
that referenced
this pull request
Sep 9, 2026
<!-- markdownlint-disable MD041 --> ## Outcome Buffered sandbox execution now goes through the typed OpenShell command adapter. Command construction, environment handling, input, output capture, deadlines, and transport failures are owned by the CLI adapter while callers consume transport-neutral results. ## Reason Buffered probes and setup flows still depended on action-owned OpenShell argv builders and synchronous process-shaped results after the streamed command boundary landed. That bypassed the adapter boundary and made timeout, fallback, environment, and currentness behavior inconsistent across consumers. ### Related issues Fixes #10991 ## Changes - Add a typed asynchronous buffered request and completion contract for command bytes, sandbox and host environment, working directory, input, output limits, signals, deadlines, and transport failures. - Centralize asynchronous OpenShell child-process execution in one CLI adapter lifecycle. Legacy status capture and typed buffered execution retain separate public results while sharing process creation, capture, deadlines, cancellation, and cleanup. - Migrate the complete #10991-owned buffered call graph across onboarding, health probes, recovery, messaging, agent setup, binary checks, compatible-endpoint checks, and the finite-output OpenClaw sessions-list path. - Preserve named-gateway authority, sanitized subprocess environments, repository working directories, Hermes command authority, process-group timeout cleanup, and the existing immediate-kill deadline sites. - Preserve registered local runtime-provider fallback through explicit operation policies: unavailable-only by default, never for authority-sensitive or mutating proofs, read-only for probes and diagnostics, and reconciled for idempotent state cleanup. Valid marked results and cancellation remain final for the registered-provider retry. - Replace remaining buffered argv and process-result consumers with typed requests, remove the unused inference-route argv builder, and reject new buffered compatibility-helper imports and value re-exports through the architecture check. - Re-establish and verify mutable OpenClaw config permissions after asynchronous MCP restoration settles and before sealing the rebuilt config hash. - Keep this as one semantic slice. Splitting the interface propagation would leave production buffered bypasses or require temporary duplicate machinery without an independently useful capability. ## Verification - `npm run docs` and `npm run validate:pr` passed on exact commit `24d3aaba943004707ce09b07243be3657b73c69e` against canonical base `7e4bdf27f17bd760a02e36467d7ea79a9ae5a37e`. The docs build rendered the OpenClaw and Hermes variants and validated published routes. Two independent reviews found no remaining accuracy, security, lifecycle, variant, or ownership issue. The main composition preserves #11234.s adapter-backed provider inspection, #11089.s typed buffered sandbox execution, and the awaited policy-handler seam. Focused validation passed 96/96 tests; CLI build/typecheck, repository architecture checks, commit hooks, and push hooks passed. - The final architecture repair passed all 63 focused guard tests, including named, namespace, star, `export =`, `export default`, direct `require`, comment-separated legacy helper re-exports, interpolated-template predecessors, type-only exclusions, lexer bait, and a 10,000-comment candidate-path case. The fixed-string prefilter plus memoized TypeScript AST removes the reported regular-expression backtracking risk. CLI type-checking, repository hooks, and two independent read-only reviews also passed. - The latest main refresh composed #10991 with #11087's structured MCP tool-discovery failures. The resolved call keeps the structured precondition result and the awaited execution boundary. Targeted validation passed 112/112 MCP tests, plus 8/8 cross-process MCP tests outside the filesystem sandbox. - The supervisor and onboarding refresh composition passed 160/160 focused tests, CLI type-checking, and independent review. The test-only Hermes port isolation repair prevents host config and suite order from influencing supervisor-relaunch coverage. - The post-#11205 repair awaits asynchronous OpenClaw config sync and updates the resumed identity-gate test for the async probe contract. The composed regression sets passed 46/46 and 33/33, and all 12 hosted CLI shards are green. - Canonical base `de7f565` includes #11264's patched `js-yaml`, resolving the inherited `GHSA-2883-xcg3-v3hh` audit failure without adding dependency changes to this PR's semantic diff. - Earlier migration validation passed adapter, integration, onboarding, process-recovery, architecture, managed-image fixture, E2E-support, project-membership, growth, and secret-scanning checks. Exact-head CI remains authoritative. - The exact-head Advisor review for `0bdd4af0c89224684f0a600213b869b8bd4e012d` completed all nine specialists and requested one documentation correction. Commit `24d3aaba943004707ce09b07243be3657b73c69e` addresses it. Exact-head review for the latest PR commit is pending. - All PR commits through the exact head are signed, DCO-compliant, and verified by GitHub. ## Review notes All valid CodeRabbit findings have been addressed. Apurv’s code feedback is implemented: safe fallback eligibility is explicit and cancellation is distinct, while the architecture assertion rejects static computed compatibility-helper access. His latest documentation correction is also implemented in both owning channel-removal pages. The earlier repair also converts synchronous process-creation throws into the typed structured failure and makes the shell-quoting security test use the production argv builder. Rejected channel probes are already caught in the shared sandbox status adapter and covered through the public Telegram status flow. The onboarding FSM test intentionally verifies root composition wiring; direct agent tests own exact buffered request and completion behavior. The accepted #10991 architecture requirement authorizes the modified repository validator surface. The guard is part of this PR because it prevents the migrated buffered execution boundary from silently regressing through imports or value re-exports. The suggestion to inject the sessions executor from a higher layer was not applied. Action modules in this repository construct adapters at their boundary, and the sessions tests replace that factory with a typed fake. A broader four-file dependency-wiring refactor would not improve #10991's contract. At commit `0bdd4af0c89224684f0a600213b869b8bd4e012d`, eight Advisor specialists found no change-required issue. The documentation specialist requested the transport-order correction included in the latest PR commit. The #10994 deferral is narrow: it retains agent captured-dispatch callers that combine bounded output capture with piped stdin and signal handoff, plus the streamed Hermes sessions path. The finite-output OpenClaw sessions-list path is included here and covered by the typed executor tests. The code migration preserves public command behavior. This PR updates the two owning channel-removal pages to document the existing destructive cleanup transport order and recovery boundary. No new live E2E target was added because the adapter, process, fallback, and consumer contracts are deterministic local boundaries. The existing managed-image workflow owns the relevant composed OpenClaw and Hermes boundary. --- 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** * Added structured sandbox command execution with gateway targeting, timeouts, output limits, environment controls, and cancellation handling. * Onboarding, health checks, inference probes, messaging diagnostics, and deployment verification now support asynchronous execution. * Added clearer recovery, readiness, gateway, and command-failure diagnostics. * **Bug Fixes** * Improved handling of timeouts, execution failures, gateway restarts, route recovery, and rollback sequencing. * Strengthened authority, identity, environment, and output validation. * **Tests** * Expanded coverage for asynchronous execution, recovery, security boundaries, diagnostics, and failure scenarios. <!-- 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> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
Agent manifest parsing now requires js-yaml 4.3.2, the first release patched against GHSA-2883-xcg3-v3hh. Oversized merge sequences of empty mappings are rejected instead of consuming unbounded parser work.
Reason
The repository-wide reviewed npm audit currently fails on js-yaml 4.3.1. This isolated update removes that shared security gate without mixing dependency work into the guarded PR autofix implementation.
Related issues
Changes
Verification
npm ci --ignore-scripts— passed.npm run build:cli— passed.npm --prefix nemoclaw run build— passed.npx vitest run src/lib/agent/manifest-readers.test.ts --reporter=dot— 1 test passed.npm run check:diff— all applicable pre-commit, commit-message, and pre-push checks passed.Review notes
The 4.3.2 upstream tag is lightweight and the upstream commit is unsigned; the published npm package carries registry signatures, its SHA-512 matches the lockfile SRI, and its loader source matches the audited tag. Local
npm auditwas not sent because that would disclose the full production dependency graph to the registry advisory service; the repository reviewed-npm-audit CI remains the authoritative gate.Documentation review found no user-facing change: existing manifest documentation already states malformed manifests fail validation.
Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests