refactor(ci): share sandbox base-image resolver primitives - #6968
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:
📝 WalkthroughWalkthroughShared base-image resolution logic is moved into a reusable shell helper. Sandbox and Hermes actions use it for image validation, candidate selection, local builds, and environment output, with Vitest coverage for the helper contracts. ChangesBase-image resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CompositeAction
participant ResolverHelper
participant Docker
participant GITHUB_ENV
CompositeAction->>ResolverHelper: Try candidate image
ResolverHelper->>Docker: Pull and inspect image
Docker-->>ResolverHelper: Digest and GLIBC version
ResolverHelper-->>CompositeAction: Accept compatible candidate
CompositeAction->>ResolverHelper: Write validated image reference
ResolverHelper->>GITHUB_ENV: Append environment assignment
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.github/actions/resolve-hermes-base-image/action.yaml:
- Line 65: Propagate resolver_write_env failures so candidate-selection
callbacks do not report success without exporting the required variable: in
.github/actions/resolve-hermes-base-image/action.yaml lines 65-65, return
failure when writing HERMES_BASE_IMAGE fails; apply the equivalent change in
.github/actions/resolve-sandbox-base-image/action.yaml lines 40-40 for
BASE_IMAGE.
In `@test/base-image-resolver-helper.test.ts`:
- Around line 102-117: Add assertions in the “writes one validated GitHub
environment assignment” test for empty and multiline BASE_IMAGE values,
verifying each invocation fails and the existing GITHUB_ENV contents remain
unchanged. Exercise these cases through resolver_write_env and assert only the
observable command status and file output.
🪄 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: 7303e8af-e0ce-47ae-96e3-47f70203649d
📒 Files selected for processing (4)
.github/actions/base-image-resolver.sh.github/actions/resolve-hermes-base-image/action.yaml.github/actions/resolve-sandbox-base-image/action.yamltest/base-image-resolver-helper.test.ts
2d2ff5c to
9b07ee2
Compare
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/base-image-resolver-helper.test.ts`:
- Around line 88-99: Update the fakeDocker command and assertion around run to
preserve argument boundaries: log each positional argument from "$@" using a
delimiter rather than joining "$*". Assert the resulting delimited argument
vector, including the Dockerfile path, tag, and build context as separate
arguments.
🪄 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: f52b119d-c178-47da-af2c-573a75b511d2
📒 Files selected for processing (4)
.github/actions/base-image-resolver.sh.github/actions/resolve-hermes-base-image/action.yaml.github/actions/resolve-sandbox-base-image/action.yamltest/base-image-resolver-helper.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/actions/resolve-sandbox-base-image/action.yaml
- .github/actions/base-image-resolver.sh
- .github/actions/resolve-hermes-base-image/action.yaml
9b07ee2 to
d4997c6
Compare
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/base-image-resolver-helper.test.ts`:
- Line 34: Update the suite title in the “base image resolver helper” describe
block to append the local issue reference as the final “(`#6957`)” suffix, without
changing child test titles.
🪄 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: 7e225953-7178-41e9-84c5-3ef51509e8d4
📒 Files selected for processing (4)
.github/actions/base-image-resolver.sh.github/actions/resolve-hermes-base-image/action.yaml.github/actions/resolve-sandbox-base-image/action.yamltest/base-image-resolver-helper.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/actions/resolve-sandbox-base-image/action.yaml
- .github/actions/resolve-hermes-base-image/action.yaml
- .github/actions/base-image-resolver.sh
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
d4997c6 to
e2596ad
Compare
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the shared shell helper extraction against both resolver actions. Agent-specific validation remains local, shared behavior is preserved, environment writes are validated, and helper tests cover the extracted contracts. No blocking findings.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after syncing main. The workflow contract now supplies the standard GITHUB_ACTION_PATH when executing the composite step directly; helper coverage passes and the Linux CI path retains the existing Bash behavior.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed at ae51f85: the Linux CI timeout is fixed by replacing the nested Node fake-Docker process with a deterministic Bash harness; the composite action is now Bash 3-compatible as well. Focused workflow-contract test, shell syntax, CLI build and typecheck pass.
Signed-off-by: Ho Lim <subhoya@gmail.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed at d552e19: the follow-up only simplifies parsing of the NUL-delimited fake-Docker log while preserving the verified Bash harness and current-operation behavior.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed at df150e3: the sandbox composite action now runs end-to-end under a fake Docker contract with its real GITHUB_ACTION_PATH and verifies the exact BASE_IMAGE export. The focused seven-test resolver suite, CLI build/typecheck, and test-size gate pass.
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…olver' into codex/pr-6968-sync # Conflicts: # test/base-image-resolver-helper.test.ts # test/pr-workflow-contract.test.ts
wscurran
added
area: ci
Summary
Closes #6957.
Validation
bash -n .github/actions/base-image-resolver.shnpx vitest run --project integration test/base-image-resolver-helper.test.ts(6 passed)npm run build:clinpm run typechecknpm run test-size:checknpm run check:diffDCO
I certify that all commits in this pull request are signed off in accordance with the Developer Certificate of Origin.
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
Signed-off-by: Ho Lim subhoya@gmail.com