feat: add variants override mechanism with libc detection#4733
Conversation
Add a generic `variants` field to registry overrides for selecting artifacts that share the same goos/goarch but differ along other axes. The first supported axis is `libc` (musl / gnu) detected via `ldd --version` on Linux. Refs #4514 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdd variant-based override matching: new Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller (cmd/runner/exec)
participant Init as Runtime.New(ctx)
participant Detect as detectLibC(ctx)
participant Runtime as Runtime{GOOS, GOARCH, LibC}
participant Override as Override.Match
participant VariantEval as Variant evaluator
Caller->>Init: New(ctx) / NewR(ctx)
Init->>Detect: call detectLibC(ctx) or read AQUA_LIBC
Detect-->>Init: return "musl"/"gnu"/""
Init-->>Runtime: populate LibC field
Override->>VariantEval: for each variant key/value
VariantEval->>Runtime: read key (e.g., libc)
VariantEval-->>Override: equal? (yes/no)
Override->>Override: include variant results in final match decision
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/registry/override.go`:
- Around line 48-55: The loop over ov.Variants in override matching must guard
against nil entries to avoid panics; update the loop that iterates ov.Variants
to check if v == nil and return false (or otherwise treat the override as
non-matching) before accessing v.Key or v.Value so you don't dereference a nil
pointer when evaluating supportedVariantKeys[v.Key] or calling
runtimeVariantValue(rt, v.Key).
In `@pkg/config/registry/package_info.go`:
- Around line 188-189: The struct fields Variant.Key and Variant.Value are
marked with `omitempty`, allowing missing keys/values to silently pass
deserialization; remove `omitempty` from the YAML/JSON struct tags on `Key` and
`Value` in the Variant struct so those fields are required at parse time (e.g.,
change `yaml:",omitempty" json:"key,omitempty"` to `yaml:"" json:"key"` and
similarly for `Value`), ensuring parsing fails for malformed entries and
preventing silent misses in the override matching logic used by the matching
code (see the override matching that checks Variant.Key).
In `@pkg/runtime/libc_linux.go`:
- Around line 21-25: The ldd invocation in detectLibC() currently runs via
exec.Command("ldd","--version") without a timeout and can hang; change it to use
a context with timeout (e.g., context.WithTimeout) and exec.CommandContext so
the process is killed if it exceeds the timeout, preserve capturing
stdout/stderr (out buffer) and handle ctx.Err() / process exit to return the
same safe default value on timeout or error; update detectLibC() to cancel the
context and treat a timeout as a non-detection rather than blocking startup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a6eae9b-2589-4cc9-b02a-3772a7d11c64
📒 Files selected for processing (8)
json-schema/registry.jsonpkg/config/registry/override.gopkg/config/registry/override_test.gopkg/config/registry/package_info.gopkg/runtime/libc_linux.gopkg/runtime/libc_linux_test.gopkg/runtime/libc_other.gopkg/runtime/runtime.go
| for _, v := range ov.Variants { | ||
| if _, ok := supportedVariantKeys[v.Key]; !ok { | ||
| return false | ||
| } | ||
| if runtimeVariantValue(rt, v.Key) != v.Value { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard nil Variants entries to prevent panic in matcher.
At Line 49, v.Key is dereferenced without checking v != nil. A malformed config containing a null variant element can crash override matching.
Suggested fix
for _, v := range ov.Variants {
+ if v == nil {
+ return false
+ }
if _, ok := supportedVariantKeys[v.Key]; !ok {
return false
}
if runtimeVariantValue(rt, v.Key) != v.Value {
return false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, v := range ov.Variants { | |
| if _, ok := supportedVariantKeys[v.Key]; !ok { | |
| return false | |
| } | |
| if runtimeVariantValue(rt, v.Key) != v.Value { | |
| return false | |
| } | |
| } | |
| for _, v := range ov.Variants { | |
| if v == nil { | |
| return false | |
| } | |
| if _, ok := supportedVariantKeys[v.Key]; !ok { | |
| return false | |
| } | |
| if runtimeVariantValue(rt, v.Key) != v.Value { | |
| return false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/registry/override.go` around lines 48 - 55, The loop over
ov.Variants in override matching must guard against nil entries to avoid panics;
update the loop that iterates ov.Variants to check if v == nil and return false
(or otherwise treat the override as non-matching) before accessing v.Key or
v.Value so you don't dereference a nil pointer when evaluating
supportedVariantKeys[v.Key] or calling runtimeVariantValue(rt, v.Key).
| Key string `yaml:",omitempty" json:"key,omitempty" jsonschema:"enum=libc"` | ||
| Value string `yaml:",omitempty" json:"value,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
with open("json-schema/registry.json", "r", encoding="utf-8") as f:
schema = json.load(f)
variant = schema["$defs"]["Variant"]
print("Variant.required =", variant.get("required"))
print("Variant.properties keys =", sorted(variant.get("properties", {}).keys()))
PYRepository: aquaproj/aqua
Length of output: 124
🏁 Script executed:
# First, let's examine the Variant struct definition
sed -n '180,200p' pkg/config/registry/package_info.go | cat -nRepository: aquaproj/aqua
Length of output: 1418
🏁 Script executed:
# Check how Variant is used in the codebase
rg -n "Variant" pkg/config/registry/package_info.go | head -20Repository: aquaproj/aqua
Length of output: 376
🏁 Script executed:
# Check for any validation logic around Variant fields
rg -n "\.Key|\.Value" pkg/config/ -A 2 -B 2 | head -40Repository: aquaproj/aqua
Length of output: 1049
🏁 Script executed:
# Check the schema generation to understand how omitempty affects it
find . -name "*.go" -type f | xargs rg -l "jsonschema" | grep -E "(cmd|pkg)" | head -5Repository: aquaproj/aqua
Length of output: 229
🏁 Script executed:
# Check the Override matching logic more carefully
cat -n pkg/config/registry/override.go | sed -n '45,60p'Repository: aquaproj/aqua
Length of output: 469
🏁 Script executed:
# Check if there are any tests that might rely on empty Variant fields
rg -n "Variant" pkg/config/registry/ -A 2 | grep -E "test|Test"Repository: aquaproj/aqua
Length of output: 1073
🏁 Script executed:
# Look for the schema generation code to understand jsonschema tag handling
rg -l "jsonschema" pkg/ | head -10Repository: aquaproj/aqua
Length of output: 365
🏁 Script executed:
# Check the package_info.go for complete context around Override definition
sed -n '210,230p' pkg/config/registry/package_info.go | cat -nRepository: aquaproj/aqua
Length of output: 1799
🏁 Script executed:
# Check if there are any cases where Variant is created with empty fields
rg -n "Variant\s*{" pkg/ -A 3 | grep -v "Key:" | grep -v "test"Repository: aquaproj/aqua
Length of output: 143
🏁 Script executed:
# Check the actual schema generation logic to understand how omitempty affects required fields
rg -n "required" json-schema/registry.json | head -20Repository: aquaproj/aqua
Length of output: 250
🏁 Script executed:
# Verify the full Override schema definition
python3 - <<'PY'
import json
with open("json-schema/registry.json", "r", encoding="utf-8") as f:
schema = json.load(f)
override = schema["$defs"]["Override"]
print("Override.required =", override.get("required", []))
print("\nVariant definition:")
variant = schema["$defs"]["Variant"]
print(" properties:", list(variant.get("properties", {}).keys()))
print(" required:", variant.get("required", []))
print("\nOverride.properties.variants:")
if "variants" in override.get("properties", {}):
variants_prop = override["properties"]["variants"]
print(" ", variants_prop)
PYRepository: aquaproj/aqua
Length of output: 210
🏁 Script executed:
# Let's check how the runtime matching works with empty values
sed -n '45,75p' pkg/config/registry/override.go | cat -nRepository: aquaproj/aqua
Length of output: 530
🏁 Script executed:
# Verify if there's any defensive code that handles empty Key or Value
rg -n "Key == |Value == " pkg/config/registry/Repository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Check Variants definition to understand the list behavior
sed -n '192,196p' pkg/config/registry/package_info.go | cat -nRepository: aquaproj/aqua
Length of output: 299
🏁 Script executed:
# Search for any YAML unmarshaling or config loading that might handle empty variants
rg -n "Variants" pkg/config/ -B 2 -A 2 | head -30Repository: aquaproj/aqua
Length of output: 1679
🏁 Script executed:
# Check if there's any code that might intentionally create Variant with empty fields
rg -n "Key.*:|Value.*:" pkg/config/registry/ | grep -v "test\|json\|yaml\|jsonschema"Repository: aquaproj/aqua
Length of output: 343
🏁 Script executed:
# Verify the supportedVariantKeys implementation to understand empty key behavior
rg -n "supportedVariantKeys" pkg/config/registry/ -B 2 -A 5Repository: aquaproj/aqua
Length of output: 1536
🏁 Script executed:
# Double-check if there are any integration tests or config files that might have empty variants
find . -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs rg -l "variants" 2>/dev/null | head -5Repository: aquaproj/aqua
Length of output: 113
🏁 Script executed:
# Verify the specific lines mentioned in the review
sed -n '186,191p' pkg/config/registry/package_info.go | cat -nRepository: aquaproj/aqua
Length of output: 329
🏁 Script executed:
# Check if there's any validation or config loading that checks for empty Variant fields
rg -n "Variant" pkg/ --type go -B 3 -A 3 | grep -E "empty|nil|validate" | head -20Repository: aquaproj/aqua
Length of output: 1158
🏁 Script executed:
# Let's understand the exact behavior when Key is empty
python3 - <<'PY'
# Simulate the matching logic
supportedVariantKeys = {"libc": None}
# Test case 1: empty Key
v_key = ""
v_value = "musl"
print("Test 1: Key='', Value='musl'")
if v_key in supportedVariantKeys:
print(f" v.Key '{v_key}' is supported")
else:
print(f" v.Key '{v_key}' is NOT supported - override would NOT match")
# Test case 2: valid Key
v_key = "libc"
v_value = "musl"
print("\nTest 2: Key='libc', Value='musl'")
if v_key in supportedVariantKeys:
print(f" v.Key '{v_key}' is supported - would check value")
else:
print(f" v.Key '{v_key}' is NOT supported")
PYRepository: aquaproj/aqua
Length of output: 223
🏁 Script executed:
# Check if jsonschema library handles omitempty tag to generate required fields
find . -name "*.go" -type f | xargs rg -l "jsonschema\|json.Marshal" | xargs rg -l "omitempty" | head -3Repository: aquaproj/aqua
Length of output: 149
🏁 Script executed:
# Check if there are any tests that verify behavior with empty Key or Value
rg -n "Key.*\"\"" pkg/config/registry/override_test.goRepository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Verify the Variant matching logic doesn't have defensive code for empty fields
cat -n pkg/config/registry/override.go | sed -n '40,60p'Repository: aquaproj/aqua
Length of output: 675
🏁 Script executed:
# Check if removing omitempty could break backward compatibility by searching for examples
find . -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) -exec rg -l "variants" {} \; 2>/dev/nullRepository: aquaproj/aqua
Length of output: 113
🏁 Script executed:
# Verify the full context of how omitempty affects schema generation in this project
rg -n "omitempty" json-schema/registry.json | head -10Repository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Let's verify if removing omitempty from the struct tags would actually affect the generated schema
# by checking how schema generation works in this project
rg -n "schema\|jsonschema" pkg/config/ --type go -A 2 | head -20Repository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Check the test case mentioned in the first search result
sed -n '195,210p' pkg/config/registry/override_test.go | cat -nRepository: aquaproj/aqua
Length of output: 520
🏁 Script executed:
# Verify the overall pattern: does the project use omitempty consistently with optional fields?
python3 - <<'PY'
import json
with open("json-schema/registry.json", "r", encoding="utf-8") as f:
schema = json.load(f)
# Check a few other types to see if they have required fields
for type_name in ["Override", "PackageInfo", "Checksum"]:
if type_name in schema["$defs"]:
obj = schema["$defs"][type_name]
required = obj.get("required", [])
print(f"{type_name}.required = {required}")
PYRepository: aquaproj/aqua
Length of output: 135
🏁 Script executed:
# Check what happens with actual config parsing - does omitempty prevent validation errors?
# Look for any validation code that checks for required Variant fields
rg -n "validate|Validate" pkg/config/registry/ --type go | grep -i variantRepository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Final verification: check if there's any code that deserializes Variant and expects optional fields
rg -n "unmarshal\|Unmarshal\|decode\|Decode" pkg/config/registry/ --type go | head -10Repository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Verify line numbers are correct
wc -l pkg/config/registry/package_info.goRepository: aquaproj/aqua
Length of output: 97
Make Variant.Key and Variant.Value required in config shape.
At lines 188–189, omitempty allows malformed entries (e.g., missing key) to pass deserialization and silently fail during override matching at runtime. The matching logic at override.go:49 checks if Key is in the supported keys map; an empty Key returns false without validation feedback. Removing omitempty enforces these fields at parse time, catching config errors early rather than during runtime matching.
Suggested fix
type Variant struct {
- Key string `yaml:",omitempty" json:"key,omitempty" jsonschema:"enum=libc"`
- Value string `yaml:",omitempty" json:"value,omitempty"`
+ Key string `yaml:"key" json:"key" jsonschema:"enum=libc"`
+ Value string `yaml:"value" json:"value"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Key string `yaml:",omitempty" json:"key,omitempty" jsonschema:"enum=libc"` | |
| Value string `yaml:",omitempty" json:"value,omitempty"` | |
| Key string `yaml:"key" json:"key" jsonschema:"enum=libc"` | |
| Value string `yaml:"value" json:"value"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/registry/package_info.go` around lines 188 - 189, The struct
fields Variant.Key and Variant.Value are marked with `omitempty`, allowing
missing keys/values to silently pass deserialization; remove `omitempty` from
the YAML/JSON struct tags on `Key` and `Value` in the Variant struct so those
fields are required at parse time (e.g., change `yaml:",omitempty"
json:"key,omitempty"` to `yaml:"" json:"key"` and similarly for `Value`),
ensuring parsing fails for malformed entries and preventing silent misses in the
override matching logic used by the matching code (see the override matching
that checks Variant.Key).
Make libc detection more robust on minimal images. Stat the well-known musl dynamic linker / libc alias paths first so distroless or scratch musl containers (where ldd may not be on PATH) are still detected as musl. Fall back to `ldd --version` when none of the file checks hit. Mirrors the multi-stage approach in the official Claude Code install script (libc.musl-*.so.1 file checks combined with an ldd grep). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/runtime/libc_linux.go (1)
46-50:⚠️ Potential issue | 🟠 MajorBound
lddexecution with context timeout (CommandContext).At Line 46,
exec.Command("ldd", "--version")is unbounded and can hang runtime initialization. Usecontext.WithTimeout+exec.CommandContextand preserve current safe fallback behavior.Suggested patch
import ( "bytes" + "context" "os" "os/exec" + "time" ) @@ - cmd := exec.Command("ldd", "--version") + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "ldd", "--version") var out bytes.Buffer cmd.Stdout = &out cmd.Stderr = &out _ = cmd.Run() + if ctx.Err() != nil { + return "" + }#!/bin/bash set -euo pipefail echo "Inspect libc detection implementation:" cat -n pkg/runtime/libc_linux.go | sed -n '1,120p' echo echo "Check for unbounded exec.Command usage in Go files:" rg -n --type=go 'exec\.Command\(' -C2 echo echo "Check timeout/context usage for command execution:" rg -n --type=go 'context\.WithTimeout\(' -C2 rg -n --type=go 'exec\.CommandContext\(' -C2As per coding guidelines: "Use context for cancellation and timeouts in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/libc_linux.go` around lines 46 - 50, The unbounded exec.Command("ldd", "--version") can hang; replace it with a context-backed call: create a context.WithTimeout (e.g., 2s), use exec.CommandContext with that context to build the command (keeping the same bytes.Buffer for Stdout/Stderr), defer the cancel(), then call Run() and preserve the existing fallback behavior by ignoring or handling the error as before; update the code around the cmd variable creation and execution to use CommandContext and ensure the timeout/cancel prevents indefinite blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/runtime/libc_linux.go`:
- Around line 46-50: The unbounded exec.Command("ldd", "--version") can hang;
replace it with a context-backed call: create a context.WithTimeout (e.g., 2s),
use exec.CommandContext with that context to build the command (keeping the same
bytes.Buffer for Stdout/Stderr), defer the cancel(), then call Run() and
preserve the existing fallback behavior by ignoring or handling the error as
before; update the code around the cmd variable creation and execution to use
CommandContext and ensure the timeout/cancel prevents indefinite blocking.
Replace exec.Command with exec.CommandContext (with a defensive 5s timeout so a hung ldd cannot stall aqua's startup) to satisfy noctx. Rename the test file to *_internal_test.go to follow the existing parse_internal_test.go convention so testpackage permits the same-package test of the unexported detectLibC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/runtime/libc_linux_internal_test.go (1)
9-17: Consider table-driven tests for extensibility.This assertion works, but if supported libc values grow, a table-driven pattern with subtests will scale better and keep expectations explicit.
As per coding guidelines,
**/*_test.go: For test files, automatically suggest table-driven test patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/libc_linux_internal_test.go` around lines 9 - 17, Convert TestDetectLibC to a table-driven subtest pattern: define a slice of test cases listing expected valid libc values (e.g., "musl", "gnu", "") and iterate with t.Run for each case, asserting detectLibC() equals one of the allowed values per case (or that the returned value is contained in the allowed set); update the test function name TestDetectLibC and use t.Parallel inside each subtest as appropriate; reference the detectLibC function and the original TestDetectLibC test harness when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runtime/libc_linux.go`:
- Around line 57-63: The current detection treats any non-empty, non-musl ldd
output as libcGNU; change the logic in the libc detection (the block using
out.Bytes(), libcMusl and libcGNU in pkg/runtime/libc_linux.go) to explicitly
detect GNU/glibc tokens (e.g., "GNU libc", "GLIBC", or "gnu") using
bytes.Contains, return libcMusl if musl is found, return libcGNU only when one
of those GNU/glibc tokens is present, and otherwise return "" for
unknown/unexpected non-empty output (preserving the empty-output check).
---
Nitpick comments:
In `@pkg/runtime/libc_linux_internal_test.go`:
- Around line 9-17: Convert TestDetectLibC to a table-driven subtest pattern:
define a slice of test cases listing expected valid libc values (e.g., "musl",
"gnu", "") and iterate with t.Run for each case, asserting detectLibC() equals
one of the allowed values per case (or that the returned value is contained in
the allowed set); update the test function name TestDetectLibC and use
t.Parallel inside each subtest as appropriate; reference the detectLibC function
and the original TestDetectLibC test harness when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d5a19dd-749d-4639-83e7-ade4a5383378
📒 Files selected for processing (2)
pkg/runtime/libc_linux.gopkg/runtime/libc_linux_internal_test.go
| if bytes.Contains(out.Bytes(), []byte(libcMusl)) { | ||
| return libcMusl | ||
| } | ||
| if out.Len() == 0 { | ||
| return "" | ||
| } | ||
| return libcGNU |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What stable tokens appear in ldd --version output for glibc across distros/locales, and how do BusyBox/Toybox/uClibc outputs differ?
💡 Result:
Stable tokens in ldd --version output for glibc across distros/locales are: - First line: "ldd (VARIANT GLIBC MAJOR.MINOR[-PATCH]) MAJOR.MINOR" where VARIANT is "Ubuntu GLIBC", "Debian GLIBC", "Debian EGLIBC", "GNU libc", etc., MAJOR.MINOR is the glibc version (e.g. 2.17, 2.21, 2.23, 2.31, 2.36, 2.40). - Second line: "Copyright (C) YEAR Free Software Foundation, Inc." - Third line: "This is free software; see the source for copying conditions. There is NO" - Fourth line: "warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE." - Fifth line: "Written by Roland McGrath and Ulrich Drepper." The VARIANT and exact version/patch vary by distro (Ubuntu adds "Ubuntu GLIBC X.Y-Z", Fedora/Alma "GNU libc", Debian "Debian GLIBC/EGLIBC"), year, and glibc version, but the structure, copyright notice, warranty disclaimer, and authors are stable. Locale does not affect this output as it's not translated. BusyBox: Does not provide ldd (BusyBox implements many commands but ldd is not standard in it; systems using BusyBox often use uClibc/musl static binaries or minimal dynamic setups without ldd). Running ldd if symlinked would show BusyBox version via --help: "BusyBox vX.Y.Z (DATE) multi-call binary.", but no ldd-specific --version output. Toybox: Does not implement ldd (listed as "not started" in status; toybox provides ~200 commands but ldd not included). Commands support --version showing toybox version (e.g. "toybox version 0.8.13"), but no ldd. uClibc: Provides a standalone ldd utility (in utils/ldd.c), but no specific --version output documented; shows library dependencies like glibc ldd when run (e.g. "libc.so.6 => /lib/tls/libc.so.6"), without standard version banner. Can be built/run on glibc hosts.
Citations:
- 1: https://stackoverflow.com/questions/9705660/check-glibc-version-for-a-particular-gcc-compiler
- 2: https://stackoverflow.com/questions/30491690/extracting-ldd-version-with-bash-script
- 3: glibc check: alternatives to ldd needed axodotdev/cargo-dist#1439
- 4: https://manpages.ubuntu.com/manpages/noble/man1/ldd.1.html
- 5: https://unix.stackexchange.com/questions/120380/what-c-library-version-does-my-system-use
- 6: https://askubuntu.com/questions/163138/how-to-find-which-libc-version-app-links-against
- 7: https://lists.uclibc.org/pipermail/uclibc/2004-January/028911.html
- 8: https://lists.uclibc.org/pipermail/uclibc/2003-December/028693.html
- 9: https://unix.stackexchange.com/questions/15895/how-do-i-check-busybox-version-from-busybox
- 10: https://lists.busybox.net/pipermail/busybox/2018-June/086531.html
- 11: http://landley.net/toybox/quick.html
- 12: https://landley.net/toybox/status.html
- 13: http://lists.landley.net/htdig.cgi/toybox-landley.net/2022-January/028857.html
- 14: https://stackoverflow.com/questions/62732447/how-to-check-libc-version
🏁 Script executed:
# First, let's look at the actual file and understand the constants
cat -n pkg/runtime/libc_linux.go | head -70Repository: aquaproj/aqua
Length of output: 2462
🏁 Script executed:
# Also check for the constants definitions
rg -n "libcMusl|libcGNU" pkg/runtime/Repository: aquaproj/aqua
Length of output: 373
Avoid treating any non-musl output as GNU libc.
At line 63, non-empty output that does not contain "musl" is always classified as gnu. This violates the function's documented contract to return "" when detection is not possible, and can misclassify systems with missing or unexpected ldd --version output (e.g., systems with alternative libc implementations or no ldd available). Glibc's ldd --version reliably outputs "GNU libc" or "GLIBC" version strings; explicitly match these tokens rather than assuming any unrecognized output is GNU.
Proposed fix
_ = cmd.Run()
- if bytes.Contains(out.Bytes(), []byte(libcMusl)) {
+ if ctx.Err() != nil {
+ return ""
+ }
+ b := bytes.ToLower(out.Bytes())
+ if bytes.Contains(b, []byte(libcMusl)) {
return libcMusl
}
- if out.Len() == 0 {
- return ""
- }
- return libcGNU
+ if bytes.Contains(b, []byte("gnu libc")) || bytes.Contains(b, []byte("glibc")) {
+ return libcGNU
+ }
+ return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/libc_linux.go` around lines 57 - 63, The current detection treats
any non-empty, non-musl ldd output as libcGNU; change the logic in the libc
detection (the block using out.Bytes(), libcMusl and libcGNU in
pkg/runtime/libc_linux.go) to explicitly detect GNU/glibc tokens (e.g., "GNU
libc", "GLIBC", or "gnu") using bytes.Contains, return libcMusl if musl is
found, return libcGNU only when one of those GNU/glibc tokens is present, and
otherwise return "" for unknown/unexpected non-empty output (preserving the
empty-output check).
Extract the ldd timeout into a named const (lddTimeout) so mnd no longer flags the literal. detectLibC runs once at startup and is not part of any cancellable user operation, so it intentionally creates a fresh context rather than threading one through runtime.New's many callers; document this and silence contextcheck at the source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/runtime/libc_linux.go (1)
62-68:⚠️ Potential issue | 🟠 MajorDo not default unknown
lddoutput to GNU libc.At Line 68, the current fallback classifies any non-empty output as
gnu. This can mis-detect cases like missinglddor unexpected stderr and cause incorrect variant matches downstream.Suggested fix
_ = cmd.Run() - if bytes.Contains(out.Bytes(), []byte(libcMusl)) { + if ctx.Err() != nil { + return "" + } + b := bytes.ToLower(out.Bytes()) + if bytes.Contains(b, []byte(libcMusl)) { return libcMusl } - if out.Len() == 0 { - return "" - } - return libcGNU + if bytes.Contains(b, []byte("gnu libc")) || bytes.Contains(b, []byte("glibc")) { + return libcGNU + } + return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/libc_linux.go` around lines 62 - 68, The code currently defaults any non-empty ldd output to libcGNU (the final "return libcGNU"), which misclassifies unknown or error outputs; update the fallback so that only an explicit match of libcMusl (bytes.Contains(out.Bytes(), []byte(libcMusl))) returns libcMusl and all other cases return the unknown/empty value (keep the existing empty-string behavior used when out.Len() == 0) instead of returning libcGNU; change the final return from libcGNU to "" (or the function's explicit unknown constant) so unexpected ldd output doesn't get mis-detected as GNU libc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/runtime/libc_linux.go`:
- Around line 62-68: The code currently defaults any non-empty ldd output to
libcGNU (the final "return libcGNU"), which misclassifies unknown or error
outputs; update the fallback so that only an explicit match of libcMusl
(bytes.Contains(out.Bytes(), []byte(libcMusl))) returns libcMusl and all other
cases return the unknown/empty value (keep the existing empty-string behavior
used when out.Len() == 0) instead of returning libcGNU; change the final return
from libcGNU to "" (or the function's explicit unknown constant) so unexpected
ldd output doesn't get mis-detected as GNU libc.
…ction Make runtime.New and runtime.NewR take a context so detectLibC can use exec.CommandContext with the caller's context. This satisfies both the noctx (use CommandContext) and contextcheck (propagate the parent context) lint rules without per-call //nolint directives. Update all callers: - pkg/cli/runner.go and pkg/controller/exec/exec.go (updateTimestamp now takes a ctx) thread the request-scoped context through. - Constructors and factories that have no ctx in scope (cmd/aqua/main.go, installpackage.New, minisign/ghattestation/slsa/ cosign NewExecutor/NewVerifier) use context.Background(). - Tests use t.Context(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lers The New constructor previously called runtime.NewR() five times — once per dedicated installer (cosign, slsa, minisign, gh) plus once inside newInstaller for the realRuntime field. With libc detection now part of NewR, each call performed file stats and could invoke ldd --version, which was wasteful for what is the same host runtime value. Compute the real runtime once at the top of New and thread it through both newInstaller (as a new realRT parameter) and the four dedicated installer wirings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/runtime/libc_linux.go (1)
52-58:⚠️ Potential issue | 🟠 MajorAvoid defaulting unknown
lddoutput to GNU libc.At Line 58, any non-empty output that doesn’t contain
"musl"is treated as GNU. That can misclassify unexpectedlddoutput and incorrectly matchlibc=gnuoverrides.Proposed fix
_ = cmd.Run() - if bytes.Contains(out.Bytes(), []byte(libcMusl)) { + b := bytes.ToLower(out.Bytes()) + if bytes.Contains(b, []byte(libcMusl)) { return libcMusl } - if out.Len() == 0 { + if out.Len() == 0 { return "" } - return libcGNU + if bytes.Contains(b, []byte("gnu libc")) || bytes.Contains(b, []byte("glibc")) { + return libcGNU + } + return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/libc_linux.go` around lines 52 - 58, The current detection code checks bytes.Contains(out.Bytes(), []byte(libcMusl)) and otherwise returns libcGNU for any non-empty ldd output, which wrongly defaults unknown output to GNU; change the logic so you only return libcGNU when the output explicitly indicates GNU (e.g., match a known GNU marker or explicit "glibc"/"gnu" token), otherwise return the empty/unknown value (keep returning "" when out.Len() == 0 or when no known marker is found). Update the branch that now returns libcGNU to instead perform an explicit check for a GNU indicator before returning libcGNU, and return "" for all other unexpected ldd outputs to avoid misclassification (referencing the symbols libcMusl, libcGNU, out and the bytes.Contains call to locate the code).
🧹 Nitpick comments (1)
pkg/runtime/runtime.go (1)
24-33: Add doc comments for exported constructorsNewandNewR.These signatures changed and now carry distinct libc behavior (
NewhonorsAQUA_LIBC,NewRuses detection only), so comments are important for callers.As per coding guidelines "Add comments for all exported functions and types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/runtime.go` around lines 24 - 33, Add Go doc comments for the exported constructors New and NewR on type Runtime that explain their differing libc semantics: document that New(ctx context.Context) constructs a Runtime using libc behavior that honors the AQUA_LIBC environment/configuration, while NewR(ctx context.Context) constructs a Runtime that uses automatic libc detection only; place the comments immediately above the respective New and NewR function declarations and follow the standard Go comment style starting with the function name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/minisign/exec.go`:
- Around line 32-33: NewExecutor currently constructs the minisign runtime with
a background context (runtime.NewR(context.Background())), which ignores caller
cancellation; change the NewExecutor signature to accept a context.Context
parameter (e.g., func NewExecutor(ctx context.Context, logger *slog.Logger,
executor CommandExecutor, param *config.Param) (*ExecutorImpl, error)) and pass
that ctx into runtime.NewR(ctx), and propagate this signature change to the
wire/init call sites that create ExecutorImpl so callers can supply their
context; update any tests or callers of NewExecutor accordingly and ensure
ExecutorImpl construction still returns the same types.
In `@pkg/slsa/exec.go`:
- Around line 31-33: Change NewExecutor to accept a caller context and propagate
it into runtime.NewR instead of using context.Background(): update the
NewExecutor signature to NewExecutor(ctx context.Context, executor
CommandExecutor, param *config.Param) and call runtime.NewR(ctx) when
initializing the runtime; update all callers (the DI entry in
pkg/controller/wire.go to pass the injected ctx into slsa.NewExecutor) so the
injected context flows through (wire_gen.go will be regenerated/updated for
other call sites).
---
Duplicate comments:
In `@pkg/runtime/libc_linux.go`:
- Around line 52-58: The current detection code checks
bytes.Contains(out.Bytes(), []byte(libcMusl)) and otherwise returns libcGNU for
any non-empty ldd output, which wrongly defaults unknown output to GNU; change
the logic so you only return libcGNU when the output explicitly indicates GNU
(e.g., match a known GNU marker or explicit "glibc"/"gnu" token), otherwise
return the empty/unknown value (keep returning "" when out.Len() == 0 or when no
known marker is found). Update the branch that now returns libcGNU to instead
perform an explicit check for a GNU indicator before returning libcGNU, and
return "" for all other unexpected ldd outputs to avoid misclassification
(referencing the symbols libcMusl, libcGNU, out and the bytes.Contains call to
locate the code).
---
Nitpick comments:
In `@pkg/runtime/runtime.go`:
- Around line 24-33: Add Go doc comments for the exported constructors New and
NewR on type Runtime that explain their differing libc semantics: document that
New(ctx context.Context) constructs a Runtime using libc behavior that honors
the AQUA_LIBC environment/configuration, while NewR(ctx context.Context)
constructs a Runtime that uses automatic libc detection only; place the comments
immediately above the respective New and NewR function declarations and follow
the standard Go comment style starting with the function name.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5417ee01-3c6d-461f-bbae-a70d519a1925
📒 Files selected for processing (15)
cmd/aqua/main.gopkg/cli/runner.gopkg/config/package_internal_test.gopkg/config/package_test.gopkg/controller/exec/exec.gopkg/cosign/verify.gopkg/ghattestation/exec.gopkg/installpackage/installer.gopkg/minisign/exec.gopkg/runtime/libc_linux.gopkg/runtime/libc_linux_internal_test.gopkg/runtime/libc_other.gopkg/runtime/runtime.gopkg/runtime/runtime_test.gopkg/slsa/exec.go
✅ Files skipped from review due to trivial changes (3)
- pkg/ghattestation/exec.go
- cmd/aqua/main.go
- pkg/cosign/verify.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/runtime/libc_linux_internal_test.go
- pkg/runtime/libc_other.go
| func NewExecutor(logger *slog.Logger, executor CommandExecutor, param *config.Param) (*ExecutorImpl, error) { | ||
| rt := runtime.NewR() | ||
| rt := runtime.NewR(context.Background()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify minisign.NewExecutor call sites for context propagation.
rg -nP --type=go '\bminisign\.NewExecutor\s*\(' -C 2Repository: aquaproj/aqua
Length of output: 1773
🏁 Script executed:
# Try listing the repository structure and accessing the file directly
git ls-files | head -20Repository: aquaproj/aqua
Length of output: 866
🏁 Script executed:
# Search for minisign.NewExecutor in Go files
rg 'minisign\.NewExecutor' --type=go -C 2Repository: aquaproj/aqua
Length of output: 2961
🏁 Script executed:
# Look for the NewExecutor function definition in the minisign package
rg 'func NewExecutor' --type=go -C 5Repository: aquaproj/aqua
Length of output: 1797
🏁 Script executed:
# Check the wire_gen.go functions that call minisign.NewExecutor
rg -B10 'minisign\.NewExecutor\(' pkg/controller/wire_gen.go | head -50Repository: aquaproj/aqua
Length of output: 2589
🏁 Script executed:
# Check if there's a context parameter in wire_gen.go functions
rg 'func.*wire_gen\.go' --type=go -A5 pkg/controller/wire_gen.go | head -40Repository: aquaproj/aqua
Length of output: 39
🏁 Script executed:
# Search for context usage in the wire setup or controller initialization
rg 'context\.' pkg/controller/wire.go -C 2Repository: aquaproj/aqua
Length of output: 3500
Thread caller context into minisign runtime initialization.
At line 33, runtime.NewR(context.Background()) ignores caller cancellation and timeouts. The NewExecutor function should accept context.Context as a parameter and pass it to runtime.NewR. Context is already available at all call sites through the wire initialization functions.
This violates the coding guideline: "Use context for cancellation and timeouts in Go code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/minisign/exec.go` around lines 32 - 33, NewExecutor currently constructs
the minisign runtime with a background context
(runtime.NewR(context.Background())), which ignores caller cancellation; change
the NewExecutor signature to accept a context.Context parameter (e.g., func
NewExecutor(ctx context.Context, logger *slog.Logger, executor CommandExecutor,
param *config.Param) (*ExecutorImpl, error)) and pass that ctx into
runtime.NewR(ctx), and propagate this signature change to the wire/init call
sites that create ExecutorImpl so callers can supply their context; update any
tests or callers of NewExecutor accordingly and ensure ExecutorImpl construction
still returns the same types.
| func NewExecutor(executor CommandExecutor, param *config.Param) *ExecutorImpl { | ||
| rt := runtime.NewR() | ||
| rt := runtime.NewR(context.Background()) | ||
| return &ExecutorImpl{ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call sites that would need context threading if NewExecutor signature changes.
rg -nP --type=go '\bslsa\.NewExecutor\s*\(' -C 2Repository: aquaproj/aqua
Length of output: 1769
🏁 Script executed:
#!/bin/bash
# Verify call sites that would need context threading if NewExecutor signature changes.
rg -nP --type=go '\bslsa\.NewExecutor\s*\(' -C 2Repository: aquaproj/aqua
Length of output: 5396
🏁 Script executed:
#!/bin/bash
# Check the full NewExecutor implementation and surrounding context
cat -n pkg/slsa/exec.go | head -50Repository: aquaproj/aqua
Length of output: 1705
🏁 Script executed:
#!/bin/bash
# Check if wire_gen.go is auto-generated and look for wire build file
head -20 pkg/controller/wire_gen.goRepository: aquaproj/aqua
Length of output: 560
🏁 Script executed:
#!/bin/bash
# Search for wire.go or go:generate directives
find . -name "wire.go" -o -name "*wire*.go" | grep -v wire_gen | head -20Repository: aquaproj/aqua
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check wire.go to see how providers are defined
cat pkg/controller/wire.goRepository: aquaproj/aqua
Length of output: 39130
🏁 Script executed:
#!/bin/bash
# Search for runtime.NewR implementation
rg -nA 10 "func NewR" pkg/runtime/ --type=goRepository: aquaproj/aqua
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Search for other usages of runtime.NewR to ensure consistency
rg -n "runtime\.NewR\(" --type=goRepository: aquaproj/aqua
Length of output: 403
Propagate caller context into runtime.NewR in NewExecutor.
At line 32, context.Background() drops caller cancellation/deadline for runtime libc detection. Pass a caller context into NewExecutor and use it here.
💡 Suggested fix
-func NewExecutor(executor CommandExecutor, param *config.Param) *ExecutorImpl {
- rt := runtime.NewR(context.Background())
+func NewExecutor(ctx context.Context, executor CommandExecutor, param *config.Param) *ExecutorImpl {
+ rt := runtime.NewR(ctx)
return &ExecutorImpl{Update pkg/controller/wire.go to pass the injected ctx to slsa.NewExecutor. The auto-generated wire_gen.go will update all 12 call sites.
As per coding guidelines: "Use context for cancellation and timeouts in Go code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/slsa/exec.go` around lines 31 - 33, Change NewExecutor to accept a caller
context and propagate it into runtime.NewR instead of using
context.Background(): update the NewExecutor signature to NewExecutor(ctx
context.Context, executor CommandExecutor, param *config.Param) and call
runtime.NewR(ctx) when initializing the runtime; update all callers (the DI
entry in pkg/controller/wire.go to pass the injected ctx into slsa.NewExecutor)
so the injected context flows through (wire_gen.go will be regenerated/updated
for other call sites).
1 participant
Context
Refs #4514. Resolves the request to detect musl Linux at runtime and select different release assets accordingly.
The original issue proposed adding a dedicated
musl: truefield to registry overrides. @jdx commented that the underlying need is broader: samegoos/goarchartifacts diverge along multiple axes (libc, CPU baseline, static vs dynamic linkage, ...), and a generic mechanism would handle them all without one-off schema fields. This PR implements that generic mechanism, withlibcas the first concrete axis.Design
Schema
A new
variantsfield onOverride, modeled as a list of structs to mirror existing aqua conventions (format_overrides,version_overrides) and to leave room for future extension fields (values,operator,not, ...) without reshaping the schema.Matching rules
In addition to existing goos/goarch/envs matching, every variant must match for the override to apply:
keyis not in aqua's allowlist of supported keys, the override is treated as non-matching (skipped).valueexactly.variantskeep their existing behavior — fully backwards compatible.Forward compatibility (skip on unknown key)
Skipping on unknown keys lets registries stack a more specific variant alongside a fallback override that older aqua versions can still match:
This was preferred over silently ignoring unknown keys because silent-ignore can install the wrong artifact (e.g. a glibc binary on a musl host). Failing to match is the safer default. A registry-level
min_aqua_versionknob was also considered, but does not currently exist in aqua and was deemed heavier than needed for this change.Changes
pkg/config/registry/package_info.goVariantstruct (Key,Value) andVariantsslice type.Keycarriesjsonschema:"enum=libc"so the generated schema reflects the current allowlist.Variants Variantsfield toOverride. (Not added toVersionOverride: that type is matched by version constraints only — the platform-level variant matching belongs on the nestedOverrideentries it already contains.)pkg/config/registry/override.gosupportedVariantKeysallowlist (currently justlibc).Override.Matchwith a final loop overVariantsthat returns false on any unknown key or any value mismatch.runtimeVariantValuehelper that maps a variant key to the correspondingRuntimefield (currentlylibc→rt.LibC).pkg/runtime/runtime.goLibC stringto theRuntimestruct.New()populates it via a newlibc()helper that honorsAQUA_LIBCenv var (mirrors the existingAQUA_GOOS/AQUA_GOARCHoverride pattern, useful for tests and manual overrides) and falls back todetectLibC().NewR()populates it viadetectLibC()directly (no env override, matching its existing semantics).pkg/runtime/libc_linux.go/pkg/runtime/libc_other.go(new)ldd --versionand inspects the combined stdout/stderr for the substringmusl. Returns"musl","gnu", or""(whenlddis missing and the buffer is empty). Run errors are intentionally ignored: musl'sldd --versionexits non-zero but still prints its libc name to stderr, and a missinglddlegitimately means we can't detect — we fall through to""and let the override loop skip libc-constrained entries in favor of fallback overrides."". Selected by build tag, so non-Linux binaries never invokeos/exec.pkg/config/registry/override_test.goTestOverride_Matchcases:libc: muslmatches a musl runtime.libc: musldoes not match a gnu runtime.libc: musldoes not match an emptyLibC(non-Linux / detection failed).cpu) skips the override.libc: muslANDlibc: gnu) cannot all match.pkg/runtime/libc_linux_test.go(new)detectLibC()returns one of"musl","gnu",""on Linux. The CI runner is glibc-based so a real musl test would need an Alpine container; skipped here to keep this PR scoped.json-schema/registry.jsongo run ./cmd/gen-jsonschema. AddsVariantandVariantsdefinitions and avariantsproperty onOverride.Test plan
cmdx v(go vet)cmdx l(golangci-lint, 0 issues)cmdx t(full test suite, all packages pass)variants: [{key: libc, value: musl}]— recommended before merging if reviewers want end-to-end confidence; the unit tests cover the matching logic with a mockedLibCfield.variants) regress nothing — covered by the existingTestOverride_MatchandTestPackageInfo_Overridecases continuing to pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests