fix(ci): reuse Hermes base image in boundary test - #8221
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 Hermes sandbox-image workflow resolves its base image before E2E tests. Validation requires one canonical, unconditional, fail-closed resolver before the secret-boundary probe. Tests cover invalid resolver configurations. ChangesHermes image validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8f59563 in the TypeScript / code-coverage/cliThe overall coverage in commit 8f59563 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/support/sandbox-images-workflow-boundary.test.ts (1)
184-193: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover each resolver invariant in the regression tests.
This case only covers a missing named step. Add behavior cases for:
- a non-canonical
usesvalue;- a resolver placed after the secret-boundary step;
- more than one resolver invocation, including a duplicate with another step name.
As per path instructions, targeted checks should validate required action identity, ordering, and exact invocation count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/sandbox-images-workflow-boundary.test.ts` around lines 184 - 193, Extend the regression tests around validateSandboxImagesWorkflow to cover every Hermes base-image resolver invariant: reject a resolver with a non-canonical uses value, reject one placed after the secret-boundary probe, and reject multiple invocations including a duplicate under another step name. Ensure each case asserts the expected validation error for canonical action identity, ordering, and exactly one invocation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/e2e/sandbox-images-workflow-boundary.mts`:
- Around line 907-920: Update the validation around baseImageResolvers to count
every workflow step whose uses value equals HERMES_BASE_IMAGE_RESOLVER_ACTION,
regardless of step name. Require exactly one such action step, and ensure that
step is named "Resolve Hermes base image" and runs before secretBoundary;
preserve the existing validation error behavior.
---
Nitpick comments:
In `@test/e2e/support/sandbox-images-workflow-boundary.test.ts`:
- Around line 184-193: Extend the regression tests around
validateSandboxImagesWorkflow to cover every Hermes base-image resolver
invariant: reject a resolver with a non-canonical uses value, reject one placed
after the secret-boundary probe, and reject multiple invocations including a
duplicate under another step name. Ensure each case asserts the expected
validation error for canonical action identity, ordering, and exactly one
invocation.
🪄 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: d2b5ed72-4a4e-46ce-8e77-80f395798d2b
📒 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
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
6 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 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. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
2 participants
Summary
The Hermes secret-boundary consumer rebuilt the full Hermes base image when it prepared the managed variant. The repeated build exceeded the probe's 10-minute process limit on
main, so the consumer now resolves the verified base image before the live probe.Changes
Type of Change
Quality Gates
8f595637ec0ae9ba437eb825b39172af2a598fabagainst base SHA962f1c3bf3a1354a8cfdc2056e04a27828be0dc0; no findings. The three changed files match the previously reviewed patch (SHA-256d5c45bb3da1403b5f4a4ac5453d92a907f869dbd). Workflow permissions remain unchanged, checkout credentials remain disabled, and the existing canonical local resolver receives no secrets. It runs once, unconditionally, before the secret-boundary probe; failure stops the job. The resolver retains immutable-digest preference and validates image layout, glibc, imports, and environment writes. Path normalization and negative tests cover omission, substitution, conditions, failure tolerance, duplicates, aliases, and ordering. No injection, dependency, endpoint-authentication, cryptography, permission, logging, secret-handling, network-policy, or resource-control regression was introduced. All nine security categories pass.Documentation Writer Review
no-docs-needed8f595637ec0ae9ba437eb825b39172af2a598fabagainst base SHA962f1c3bf3a1354a8cfdc2056e04a27828be0dc0confirmed that the change affects only internal Hermes image and E2E workflow control. It resolves the Hermes base image before the secret-boundary probe and enforces that invariant through boundary validation and behavior tests. It changes no supported product or user-facing documentation surface. The changed workflow labels, diagnostics, and test titles comply with the repository writing rules.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable —npm run validate:prand the normal push hooks passed for PR commit SHA8f595637ec0ae9ba437eb825b39172af2a598fab.test/e2e/support/sandbox-images-workflow-boundary.test.tspassed 37/37 for PR commit SHA8f595637ec0ae9ba437eb825b39172af2a598fab;git diff --checkpassed.8f595637ec0ae9ba437eb825b39172af2a598fab; no non-success check is accepted or waived.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit