fix(hermes): retire stale OpenClaw base guard - #5949
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR removes the stale OpenClaw verifier path, updates Hermes base-image retired-layout checks, and adds final-image layout tests for mutable base rejection, retired state rejection, and legacy-data migration. ChangesHermes base image layout validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 0 in-scope improvements
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sandbox-provisioning.test.ts (1)
1291-1306: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a Hermes negative-path test for legacy
.hermes-datacleanup.test/sandbox-provisioning.test.tsonly covers the successful migration path today, so the Dockerfile’s symlink-rejection branches can still regress without a failing test. Add one case that starts with a legacy.hermes-datasymlink and asserts the build exits non-zero.🤖 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/sandbox-provisioning.test.ts` around lines 1291 - 1306, Add a negative-path test in sandbox-provisioning to cover legacy .hermes-data cleanup failure: extend the existing docker-based helper around runDockerShell/dockerRunCommandBetween so one case starts with a preexisting .hermes-data symlink and verifies the build fails with a non-zero exit. Reuse the current sandbox setup in test/sandbox-provisioning.test.ts and the migration setup around precreateConfig, but assert the symlink-rejection path instead of the successful config migration.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.
Outside diff comments:
In `@test/sandbox-provisioning.test.ts`:
- Around line 1291-1306: Add a negative-path test in sandbox-provisioning to
cover legacy .hermes-data cleanup failure: extend the existing docker-based
helper around runDockerShell/dockerRunCommandBetween so one case starts with a
preexisting .hermes-data symlink and verifies the build fails with a non-zero
exit. Reuse the current sandbox setup in test/sandbox-provisioning.test.ts and
the migration setup around precreateConfig, but assert the symlink-rejection
path instead of the successful config migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 962818d7-70c1-40cc-8d68-32a0a743eaa6
📒 Files selected for processing (7)
.github/workflows/pr-self-hosted.yamlagents/hermes/Dockerfilescripts/verify-hermes-stale-openclaw-image.shtest/helpers/hermes-dockerfile-run.tstest/hermes-stale-openclaw-guard.test.tstest/pr-workflow-contract.test.tstest/sandbox-provisioning.test.ts
💤 Files with no reviewable changes (4)
- scripts/verify-hermes-stale-openclaw-image.sh
- test/hermes-stale-openclaw-guard.test.ts
- test/pr-workflow-contract.test.ts
- .github/workflows/pr-self-hosted.yaml
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28339310217
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/actions/resolve-hermes-base-image/action.yaml:
- Around line 29-39: The layout_ok check is too strict because it rejects any
/sandbox/.hermes-data entry, but the Hermes Dockerfile still allows a real
directory there and migrates it. Update layout_ok in resolve-hermes-base-image
action to only fail on retired sandbox state that is actually invalid for the
final image contract (for example, symlink forms) while allowing a real
.hermes-data directory so try_image() does not incorrectly skip a valid
published digest.
In `@test/hermes-final-image-layout.test.ts`:
- Around line 42-70: The new hermes final image layout test is tripping the
conditional guardrail because it contains too many inline branches in the
fixture setup and OpenClaw assertions. Refactor the setup logic in the affected
test to use table-driven cases or small helper functions instead of multiple
if/else blocks, and move any OpenClaw-specific assertion into a separate helper
or dedicated test. Keep the unique fixtures grouped by scenario using the
existing hermes fixture builder and related test symbols so the behavior stays
the same while reducing conditional count.
- Line 117: The `.hermes-data` migration assertion in the test is too weak
because `fs.existsSync()` can return false for a dangling symlink, so it may not
prove the old path was actually removed. Update the check in
`hermes-final-image-layout.test.ts` to use a stronger existence/removal
assertion against `run.sandboxRoot` and `.hermes-data`, so the test verifies the
superseded path is truly gone or unreachable rather than merely non-resolvable.
🪄 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: 98bd0e74-ce9e-4fcb-aee2-689cbb8df787
📒 Files selected for processing (5)
.github/actions/resolve-hermes-base-image/action.yamlagents/hermes/Dockerfiletest/e2e/test-hermes-sandbox-secret-boundary.shtest/hermes-final-image-layout.test.tstest/pr-workflow-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- agents/hermes/Dockerfile
- test/pr-workflow-contract.test.ts
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28339588125
|
Selective E2E Results — ✅ All requested jobs passedRun: 28339640498
|
Vitest E2E Scenario Results —
|
| Job | Result |
|---|---|
| hermes-e2e-vitest | |
| hermes-root-entrypoint-smoke-vitest | |
| hermes-sandbox-secret-boundary-vitest |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Advisor disposition for final head
Runtime evidence for |
Selective E2E Results — ✅ All requested jobs passedRun: 28339907806
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 28339846140
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 28340700087
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28340627350
|
|
Final-head evidence for
Advisor dispositions on the final head:
The remaining gate is the PR template's independent sensitive-path human review; I have intentionally left that checkbox unchecked. |
## 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>
<!-- markdownlint-disable MD041 --> ## Summary Advance the default Hermes final-image base to the newly published post-stale-layout digest and retire the temporary digest-coupled repair that intentionally rejected newer published bases. This restores Hermes onboarding and live E2E builds after the `latest` base moved, while keeping current-state fail-closed layout checks and the older `.hermes-data` compatibility migration. ## Related Issue Unblocks NVIDIA#5947. Follow-up to NVIDIA#5882. ## Changes - Pin the default Hermes base to `sha256:8dad3b989a9ed1e601743310b97be21be5f59f89f7913a47d04f3ec3c40b8ce6`, whose published ARM64 image has neither `/sandbox/.openclaw` nor `/sandbox/.hermes-data`. - Remove the fired `NEMOCLAW_STALE_OPENCLAW_BASE_DIGEST` guard and temporary `/sandbox/.openclaw` repair; replace it with a small fail-closed invariant that rejects retired OpenClaw state rather than maintaining it. - Keep published-image provenance in the resolver: export immutable official GHCR digests and reject candidates containing `.openclaw` or `.hermes-data` state, while preserving local rebuild and caller-selected base refs in the final Dockerfile. - Delete the dedicated stale-layout verifier job, script, helpers, and tests now that their documented removal trigger has fired. - Add a focused final-image layout suite covering OpenClaw-state refusal, the retained `.hermes-data` migration, and symlink refusal; keep immutable published-ref coverage at the resolver boundary and extend live secret-boundary inspection to both retired paths. - Reject nested symlinks before copying retained `.hermes-data` compatibility state so migration cannot preserve links outside the legacy tree. ## 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 <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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: internal published-image lifecycle cleanup; CLI behavior, configuration, supported workflows, and the documented Hermes state location are unchanged. - [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 independent review of the Hermes sandbox image boundary. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [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) - [ ] 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) Verification evidence: - Pulled and inspected the published ARM64 `8dad…` image; `/sandbox/.openclaw` and `/sandbox/.hermes-data` are absent. - Built `agents/hermes/Dockerfile` against the exact `8dad…` digest and verified sandbox-user readability, runtime permissions, runtime symlinks, and absence of both stale paths. - Focused integration run passed 61 tests across `hermes-final-image-layout`, `hermes-doctor-config-hash`, `pr-workflow-contract`, and `sandbox-provisioning`. - `npm run build:cli`, `npm run typecheck:cli`, `npm run checks`, and `npm run test:projects:check` passed. - Normal commit and push hooks passed for all commits, including the full CLI test lane, shellcheck, hadolint, repository checks, source-shape and test-size budgets, gitleaks, commitlint, and CLI TypeScript checks. - [Vitest E2E run 28339846140](https://github.com/NVIDIA/NemoClaw/actions/runs/28339846140) passed Hermes install/inference, root entrypoint, and secret-boundary jobs, then exposed that Dockerfile-level registry validation rejected the forced local `:latest` base-cache rebuild. Commit `ce85e762a` moves provenance back to the resolver boundary; [exact-head rerun 28340627350](https://github.com/NVIDIA/NemoClaw/actions/runs/28340627350) passed all four jobs, including stale-base rebuild. - Downloaded exact-head artifacts have no credential-shaped matches; every command result in the rebuild artifact is zero, post-rebuild inference returned `PONG`, backup scanning found no leaks, and all scenario cleanups report no failures. - Required documentation writer assessment, rerun after the fail-closed review fixes, reported no user-facing docs changes needed. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## 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>
Summary
Advance the default Hermes final-image base to the newly published post-stale-layout digest and retire the temporary digest-coupled repair that intentionally rejected newer published bases. This restores Hermes onboarding and live E2E builds after the
latestbase moved, while keeping current-state fail-closed layout checks and the older.hermes-datacompatibility migration.Related Issue
Unblocks #5947. Follow-up to #5882.
Changes
sha256:8dad3b989a9ed1e601743310b97be21be5f59f89f7913a47d04f3ec3c40b8ce6, whose published ARM64 image has neither/sandbox/.openclawnor/sandbox/.hermes-data.NEMOCLAW_STALE_OPENCLAW_BASE_DIGESTguard and temporary/sandbox/.openclawrepair; replace it with a small fail-closed invariant that rejects retired OpenClaw state rather than maintaining it..openclawor.hermes-datastate, while preserving local rebuild and caller-selected base refs in the final Dockerfile..hermes-datamigration, and symlink refusal; keep immutable published-ref coverage at the resolver boundary and extend live secret-boundary inspection to both retired paths..hermes-datacompatibility state so migration cannot preserve links outside the legacy tree.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification evidence:
8dad…image;/sandbox/.openclawand/sandbox/.hermes-dataare absent.agents/hermes/Dockerfileagainst the exact8dad…digest and verified sandbox-user readability, runtime permissions, runtime symlinks, and absence of both stale paths.hermes-final-image-layout,hermes-doctor-config-hash,pr-workflow-contract, andsandbox-provisioning.npm run build:cli,npm run typecheck:cli,npm run checks, andnpm run test:projects:checkpassed.:latestbase-cache rebuild. Commitce85e762amoves provenance back to the resolver boundary; exact-head rerun 28340627350 passed all four jobs, including stale-base rebuild.PONG, backup scanning found no leaks, and all scenario cleanups report no failures.Signed-off-by: Carlos Villela cvillela@nvidia.com