fix(hermes): trust corporate CA in final-stage downloads - #8228
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHermes configures corporate CA trust after the base image and before npm operations. Messaging installation uses conditional CA trust variables. Shared Dockerfile parsing helpers, contract tests, documentation, and boundary-build checks validate command structure, ordering, and certificate installation. ChangesHermes corporate CA trust
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 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 24dd82d in the TypeScript / code-coverage/cliThe overall coverage in commit 24dd82d in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8228.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
jyaunches
approved these changes
Aug 4, 2026
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…age-ca-ordering Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helpers/dockerfile-run-commands.ts`:
- Around line 85-103: Update the scanner around the command parsing logic in
test/helpers/dockerfile-run-commands.ts:85-103 to recognize executable commands
inside $() and backtick substitutions, and avoid treating ${`#PATH`} as a comment;
alternatively reject these unsupported shell constructs before accepting a
reviewed command. Add executable-behavior fixtures in
test/dockerfile-run-commands.test.ts:13-24 containing one valid direct
invocation plus substitution and ${`#PATH`}; variants, and assert that each
produces an unreviewed RUN error.
🪄 Autofix
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: a0166195-29a5-4f6e-8647-411063c5fea1
📒 Files selected for processing (10)
agents/hermes/Dockerfileci/source-shape-test-budget.jsondocs/security/configure-corporate-ca-trust.mdxtest/bundled-npm-brace-expansion-dockerfile-contract.test.tstest/bundled-npm-ip-address-dockerfile-contract.test.tstest/corporate-ca-build-tls-anchor.test.tstest/dockerfile-run-commands.test.tstest/helpers/dockerfile-run-commands.tstest/hermes-final-image-layout.test.tstest/node-tar-dockerfile-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/security/configure-corporate-ca-trust.mdx
- agents/hermes/Dockerfile
- ci/source-shape-test-budget.json
- test/corporate-ca-build-tls-anchor.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/support/sandbox-images-workflow-boundary.test.ts`:
- Around line 431-444: The sandbox image workflow boundary test currently
removes CA-related fragments without validating command ordering. Extend the
mutation setup around hermes.run to move CA preparation after docker build and
place OpenSSL verification before CA preparation, then invoke the validator for
each mutated workflow and assert both are rejected. Keep the assertions focused
on invalid behavioral ordering rather than implementation details.
In `@tools/e2e/sandbox-images-workflow-boundary.mts`:
- Line 600: Make the CA boundary contract order-aware: in
tools/e2e/sandbox-images-workflow-boundary.mts at lines 600-600, define
pre-build and post-build fragment groups; at lines 618-618, validate each
fragment’s position relative to docker build; and at lines 682-686, classify the
Hermes CA fragments into the appropriate phase. In
test/e2e/support/sandbox-images-workflow-boundary.test.ts at lines 431-444,
mutate command ordering, and at lines 453-455, assert rejection for each invalid
placement so tests verify boundary ordering rather than presence alone.
🪄 Autofix
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: 80d14d78-ae2e-46b5-8755-ecea359d556c
📒 Files selected for processing (3)
.github/workflows/sandbox-images-and-e2e.yamltest/e2e/support/sandbox-images-workflow-boundary.test.tstools/e2e/sandbox-images-workflow-boundary.mts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
wscurran
added
area: sandbox
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
enabled auto-merge (squash)
August 5, 2026 08:11
cv
disabled auto-merge
August 5, 2026 08:20
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hermes custom images that receive a corporate CA now validate and install it in the final build stage before any registry or Python package download. Builds without a corporate CA keep the default uv and Python trust behavior.
Changes
FROMinstruction and install it as a root-owned, read-only trust anchor.NODE_EXTRA_CA_CERTSbefore final-stage Node.js and npm operations.CURL_CA_BUNDLE,SSL_CERT_FILE, andREQUESTS_CA_BUNDLEonly when the decoded corporate CA exists.Type of Change
Quality Gates
24dd82d3a90a2affb564382b1f8153951dcd9665, against base commit89e17bfe9a22ffc6b16c8407ce83a2c0a493ba74passed all nine security categories with no findings. The merge from current main preserved the 15-file contribution with stable patch IDeee48b893c2158892e7ceb81925edc0067c5828a. The final Hermes stage validates the supplied CA before TLS operations and preserves default trust when it is absent. The CI fixture uses fixed endpoints, hostname and chain verification, bounded CA roots, disabled ambient CA stores, and installed-bundle SHA-256 equality. Output handling opens without following symlinks or truncating, verifies descriptor and path identity, requires a single-link regular file, and writes through the descriptor.Documentation Writer Review
docs-updated24dd82d3a90a2affb564382b1f8153951dcd9665, against base commit89e17bfe9a22ffc6b16c8407ce83a2c0a493ba74. The merge from current main preserved the contribution with stable patch IDeee48b893c2158892e7ceb81925edc0067c5828a.docs/security/configure-corporate-ca-trust.mdxaccurately documents the Hermes discovery installer, final-stage CA ordering, conditional registry and Python trust, and default-trust behavior. The page is published in the OpenClaw, Hermes, and Deep Agents guide variants. The review found no changed text whose wording alters behavior, security, data safety, or test meaning. GitHub Fern Preview passed for this commit.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHub — all 37 commits areVerified.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable —pre-commitandcommit-msgpassed. The linked worktree lacks generateddist/artifacts required bypre-push; GitHub Actions is the validation authority.24dd82d3a90a2affb564382b1f8153951dcd9665.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Core GitHub checks passed for commit24dd82d3a90a2affb564382b1f8153951dcd9665; the required E2E gate is still running.npm run docsbuilds without warnings (doc changes only) — GitHub Docs / Fern Preview will validate the published commit.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests