Repository navigation
[codex] Fix verifier auto-install and cosign bundles - #2481
Merged
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2481 +/- ##
==========================================
+ Coverage 78.20% 78.24% +0.04%
==========================================
Files 1119 1119
Lines 106283 106335 +52
==========================================
+ Hits 83115 83202 +87
+ Misses 18529 18490 -39
- Partials 4639 4643 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
May 22, 2026 23:43
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 22, 2026 23:43
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
📝 WalkthroughWalkthroughAsset URL construction is refactored to accept explicit goos/goarch parameters throughout the builder chain. Verifier bootstrap version resolution is extracted into a dedicated method with a new error type. Platform override application is generalized to support cross-platform testing. Cosign signature verification now properly combines user options with downloaded bundle sidecars. Windows .exe handling is centralized through an OS-aware helper. Tests cover Windows asset URL scenarios and verifier bootstrap edge cases. ChangesPlatform-aware asset URL construction and verifier bootstrap refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
pkg/toolchain/installer/override.go (1)
21-23: ⚡ Quick winAdd a doc comment for the new exported function.
ApplyPlatformOverridesForPlatformis exported and should have a Go-style doc comment to keep docs/lint clean.Suggested patch.
+// ApplyPlatformOverridesForPlatform applies platform-specific overrides using the provided GOOS/GOARCH values. func ApplyPlatformOverridesForPlatform(tool *registry.Tool, goos, goarch string) { defer perf.Track(nil, "installer.ApplyPlatformOverridesForPlatform")()As per coding guidelines, "Document all exported functions, types, and methods following Go's documentation conventions."
🤖 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 `@pkg/toolchain/installer/override.go` around lines 21 - 23, Add a Go-style doc comment for the exported function ApplyPlatformOverridesForPlatform: start the comment with the function name and write one concise sentence describing what the function does, and include brief notes about the parameters (tool *registry.Tool, goos, goarch) and any important behavior or side-effects; place the comment immediately above the ApplyPlatformOverridesForPlatform declaration in override.go to satisfy Go documentation/lint rules.
🤖 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 `@pkg/toolchain/installer/installer.go`:
- Around line 401-415: resolveVerifierInstallVersion currently ignores errors
returned by configuredReg.GetLatestVersion and reg.GetLatestVersion and only
returns a generic ErrVerifierVersionUnavailable; capture those lookup errors,
aggregate them with errors.Join, and return a wrapped error that includes
ErrVerifierVersionUnavailable (e.g., fmt.Errorf("%w: %s/%s",
errors.Join(collectedErrs...), owner, repo) or wrap Join result) so callers see
root causes. Specifically, in resolveVerifierInstallVersion collect non-nil
errors from i.configuredReg.GetLatestVersion and from reg.GetLatestVersion
(referencing i.useConfiguredReg, i.configuredReg,
i.registryFactory.NewAquaRegistry, and GetLatestVersion), join them with
errors.Join, and return the joined error wrapped with
ErrVerifierVersionUnavailable when no version is found.
---
Nitpick comments:
In `@pkg/toolchain/installer/override.go`:
- Around line 21-23: Add a Go-style doc comment for the exported function
ApplyPlatformOverridesForPlatform: start the comment with the function name and
write one concise sentence describing what the function does, and include brief
notes about the parameters (tool *registry.Tool, goos, goarch) and any important
behavior or side-effects; place the comment immediately above the
ApplyPlatformOverridesForPlatform declaration in override.go to satisfy Go
documentation/lint rules.
🪄 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: Pro
Run ID: b40cf68a-33e9-4143-8467-bdcaeb932ba2
📒 Files selected for processing (8)
pkg/toolchain/installer/asset.gopkg/toolchain/installer/asset_test.gopkg/toolchain/installer/errors.gopkg/toolchain/installer/installer.gopkg/toolchain/installer/override.gopkg/toolchain/installer/verifier_command_test.gopkg/toolchain/verification/checksum_test.gopkg/toolchain/verification/signature.go
Contributor
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)
pkg/toolchain/installer/verifier_command_test.go (1)
125-155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack install attempts explicitly in this regression test.
assert.Empty(t, reg.requestedVersion, ...)cannot tell “Install was never called” from “Install was called with an empty version”. A buggy bootstrap path that reachesGetToolWithVersion(..., "")would still pass here.Suggested fix.
type verifierBootstrapRegistry struct { mu sync.Mutex latest string latestErr error tool *registry.Tool requestedVersion string + getToolCalls int } func (r *verifierBootstrapRegistry) GetToolWithVersion(_, _, version string) (*registry.Tool, error) { r.mu.Lock() defer r.mu.Unlock() + r.getToolCalls++ r.requestedVersion = version return r.tool, nil }- assert.Empty(t, reg.requestedVersion, "bootstrap install must not be called with literal latest") + assert.Zero(t, reg.getToolCalls, "bootstrap install must not be attempted")Also applies to: 205-221
🤖 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 `@pkg/toolchain/installer/verifier_command_test.go` around lines 125 - 155, The test currently uses assert.Empty(t, reg.requestedVersion) which can’t distinguish “no install attempt” from “install attempted with empty string”; update verifierBootstrapRegistry to track attempts explicitly (e.g., add a bool field like installCalled or attemptCount) and set it inside its Install/GetToolWithVersion path, then in TestVerifierCommandRunnerAutoInstallFailsBeforeInstallingLatest (and the other case at 205-221) assert that reg.installCalled is false (or attemptCount == 0) instead of checking requestedVersion; reference verifierBootstrapRegistry, requestedVersion, and TestVerifierCommandRunnerAutoInstallFailsBeforeInstallingLatest when making the changes.
🤖 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 `@pkg/toolchain/installer/verifier_command_test.go`:
- Around line 125-155: The test currently uses assert.Empty(t,
reg.requestedVersion) which can’t distinguish “no install attempt” from “install
attempted with empty string”; update verifierBootstrapRegistry to track attempts
explicitly (e.g., add a bool field like installCalled or attemptCount) and set
it inside its Install/GetToolWithVersion path, then in
TestVerifierCommandRunnerAutoInstallFailsBeforeInstallingLatest (and the other
case at 205-221) assert that reg.installCalled is false (or attemptCount == 0)
instead of checking requestedVersion; reference verifierBootstrapRegistry,
requestedVersion, and
TestVerifierCommandRunnerAutoInstallFailsBeforeInstallingLatest when making the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 219c944b-06ec-4af4-9c4b-5a8bec2c47a3
📒 Files selected for processing (2)
pkg/toolchain/installer/installer.gopkg/toolchain/installer/verifier_command_test.go
Andriy Knysh (aknysh)
approved these changes
May 23, 2026
Contributor
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
May 23, 2026
Pre-commit was failing on the merge ref for PR #2482 because three commits recently merged to main (#2348, #2478, #2481) contain files that were not gofumpt-formatted and a couple of native-ci test fixtures with trailing whitespace from captured terraform output. These files are not touched by this PR, but they appear in the PR's GitHub-generated merge ref diff and so pre-commit checks them. Apply the auto-fixes that pre-commit produces: - cmd/terraform/output.go: group two consecutive var decls. - internal/exec/terraform_output_getter_test.go: line-break in assert.PanicsWithValue calls (2x). - pkg/terraform/output/executor_test.go: line-break in NewExecutor call. - pkg/terraform/output/get_test.go: line-break in assert.PanicsWithValue calls (3x). - tests/fixtures/scenarios/native-ci/github-output.txt: trim trailing whitespace. - tests/fixtures/scenarios/native-ci/github-step-summary.txt: trim trailing whitespace. The fixture files are runtime outputs set via GITHUB_OUTPUT and GITHUB_STEP_SUMMARY env vars during the native-ci test scenarios and are only checked via file_contains substring assertions, so trimming trailing whitespace is safe. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Andriy Knysh (aknysh)
added a commit
that referenced
this pull request
May 25, 2026
…nd skip controls (#2482) * feat(hooks): kind system + scanner kinds + auto-install + --skip-hooks Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(hooks): test coverage + error-builder polish + helper extraction Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: apply gofumpt and trim trailing whitespace after main merge Pre-commit was failing on the merge ref for PR #2482 because three commits recently merged to main (#2348, #2478, #2481) contain files that were not gofumpt-formatted and a couple of native-ci test fixtures with trailing whitespace from captured terraform output. These files are not touched by this PR, but they appear in the PR's GitHub-generated merge ref diff and so pre-commit checks them. Apply the auto-fixes that pre-commit produces: - cmd/terraform/output.go: group two consecutive var decls. - internal/exec/terraform_output_getter_test.go: line-break in assert.PanicsWithValue calls (2x). - pkg/terraform/output/executor_test.go: line-break in NewExecutor call. - pkg/terraform/output/get_test.go: line-break in assert.PanicsWithValue calls (3x). - tests/fixtures/scenarios/native-ci/github-output.txt: trim trailing whitespace. - tests/fixtures/scenarios/native-ci/github-step-summary.txt: trim trailing whitespace. The fixture files are runtime outputs set via GITHUB_OUTPUT and GITHUB_STEP_SUMMARY env vars during the native-ci test scenarios and are only checked via file_contains substring assertions, so trimming trailing whitespace is safe. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(snapshots): regenerate help snapshots for --skip-hooks global flag The new --skip-hooks global flag added in pkg/flags/global_builder.go appears in every command's --help output. Regenerated the 39 affected golden snapshots so TestCLICommands passes on Linux, macOS, and Windows. Also includes: - pkg/hooks/hooks_test.go: save and restore prior viper "skip-hooks" value in TestRunAll_SkipHooksBypassesPreflightBinaryCheck to avoid cross-test viper leakage. - website/src/data/roadmap.js: minor copy edit to custom-hooks milestone. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(ci): update test workflow checkout actions * test(hooks): assert normalized legacy hook kind * test: increase hooks coverage * ci: disable checkout credential persistence * ci: pin workflow actions * docs: clarify custom hook outputs * docs: rewrite tool-deps PRD as requirements; split scanner kinds; document hook env var lifetime and --all Rewrite docs/prd/tool-dependencies-integration.md to state requirements and per-feature implementation status instead of narrating its own history. The PRD now leads with a Requirements & Implementation Status table that flags each requirement as Implemented or Not implemented with a code reference. Implementation Plan phases carry Status annotations. Success Criteria are plain numbered items, not a completion tracker. Split the lumped trivy/checkov/kics section in website/docs/stacks/hooks.mdx into three separate sections, one per kind, each with a definition list documenting that scanner's command, args, output handling, on_failure default, runtime requirements, and kind-specific quirks (Checkov's SSL_CERT_FILE workaround, KICS's KICS_QUERIES_PATH and curated registry override). Expand the ATMOS_OUTPUT_DIR / ATMOS_OUTPUT_FILE env-var entries to spell out the per-hook-invocation contract: fresh os.MkdirTemp("atmos-hook-*") directory created before the subprocess runs, deleted automatically when the hook returns, never shared between sibling hooks. Add a new "Hooks with --all" subsection covering per-component firing in dependency order, per-component tool install, shared --skip-hooks, and on_failure:fail aborting the downstream traversal. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(hooks): link examples to website routes; document override semantics; drop stream paragraph from changelog Switch the Examples list in website/docs/stacks/hooks.mdx from raw GitHub URLs to the website's /examples/<name> routes (the file-browser plugin already publishes them). Add an "Overriding Kind Defaults" section to hooks.mdx documenting that any field set on a hook (command, args, env, on_failure) overrides the named kind's default for that field. Includes a caution callout that args and env are full replacement — not merge — so overriding args requires restating the full default arg list. Three examples cover the common cases: bumping a scanner's severity threshold via args, making a finding block the run via on_failure, and injecting a tool config path via env. Code path: pkg/hooks/kind.go::ResolveDefaults. Drop the "Tool output streams naturally" subsection from the custom hooks changelog entry per review feedback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(hooks): signal overridable defaults up front in Built-in Kinds preamble The per-kind sections list "(default)" values without indicating they're customizable. Add a one-paragraph preamble under "Built-in Kinds" that tells readers every default is overridable and points to the override rules section before they read any individual kind. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(blog): link custom-hooks examples to /examples routes; add docs ActionCard Swap the four GitHub example URLs in the custom-hooks changelog post to the website's /examples/<name> routes (already published by the file-browser plugin), matching the change in website/docs/stacks/hooks.mdx. Append an ActionCard at the bottom pointing readers from the changelog narrative to /stacks/hooks for the full reference (kinds, override semantics, lifecycle events, tool auto-install, --all / --skip-hooks). The remaining github.com URL in the "Try it" section stays — git clone needs the GitHub URL. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(blog): add override-defaults chapter; wire ActionCard CTAs correctly Add an "Override any default" h3 between the built-in-kinds and bring-your-own-command sections, matching the chapter style of "Workdir compatible". The new chapter shows that command, args, env, and on_failure are all overridable on built-in kinds, with a worked example (Trivy with HIGH,CRITICAL severity and on_failure: fail) and a call-out that args/env are full replacement, not merge — linking to the override docs at /stacks/hooks#overriding-kind-defaults. Fix the closing ActionCard so the CTA buttons actually render. The component reads ctaText/ctaLink and secondaryCtaText/secondaryCtaLink, not href. The card now renders a primary "Read the docs" CTA to /stacks/hooks and a secondary "Browse examples" CTA to /examples/hooks-trivy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
|
These changes were released in v1.220.0-rc.2. |
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.
what
latest.optswith downloaded sidecars like--bundleso Trivy checksum signature verification works with Aqua metadata.why
v...tags and the bootstrap path could construct invalid release URLs.cosign verify-blobcommand.references
go test ./pkg/toolchain/installer ./pkg/toolchain/registry/aqua ./pkg/toolchain/verification,go test ./pkg/toolchain, pre-commit hooks, and a livego run . toolchain install aquasecurity/trivy@v0.70.0.Summary by CodeRabbit
New Features
Bug Fixes
Tests