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:
WalkthroughAdds a VersionMismatchError type and CLI exit code 2 path, enforces post-install version verification, tightens architecture-aware asset/install logic, introduces cross-platform dynamic-library diagnostics, adds pipeline logging helpers, updates defaults/registry loading, and includes tests and small refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Main
participant Installer as Installer
participant VerCmd as Version Command
participant Comparator as CompareVersions
participant Diagnoser as DiagnoseLibraryIssues
CLI->>Installer: request install
Installer->>Installer: extract/copy binary and post-processing
Installer->>VerCmd: run package.VersionCommand
alt version command succeeds
VerCmd-->>Installer: version output
Installer->>Comparator: Compare installed vs expected
alt OK or Newer
Comparator-->>Installer: OK
Installer-->>CLI: success
else mismatch
Comparator-->>Installer: MISMATCH
Installer->>Diagnoser: DiagnoseLibraryIssues(binary)
Diagnoser-->>Installer: diagnostics
Installer-->>CLI: return VersionMismatchError
CLI->>CLI: detect VersionMismatchError -> os.Exit(2)
end
else version command fails
VerCmd-->>Installer: error
Installer-->>CLI: propagate error -> os.Exit(1)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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: 2
🧹 Nitpick comments (4)
pkg/manager/asset_filter_test.go (1)
308-323: Inconsistent indentation in test cases.The new test cases have inconsistent indentation compared to the surrounding code. The struct fields should be indented with tabs to match the other test cases in this file.
🔧 Proposed fix for indentation
{ name: "should not return wrong-arch asset as universal (aarch64 when amd64 requested)", - assets: []string{ - "postgrest-v14.6-macos-aarch64.tar.xz", - }, - arch: "amd64", - wantErr: true, - }, - { - name: "should not return wrong-arch asset as universal (x86_64 when arm64 requested)", - assets: []string{ - "tool-macos-x86_64.tar.gz", - }, - arch: "arm64", - wantErr: true, - }, - { - name: "should prefer arm64 over arm when both exist", + assets: []string{ + "postgrest-v14.6-macos-aarch64.tar.xz", + }, + arch: "amd64", + wantErr: true, + }, + { + name: "should not return wrong-arch asset as universal (x86_64 when arm64 requested)", + assets: []string{ + "tool-macos-x86_64.tar.gz", + }, + arch: "arm64", + wantErr: true, + }, + { + name: "should prefer arm64 over arm when both exist", assets: []string{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/manager/asset_filter_test.go` around lines 308 - 323, The two new test cases named "should not return wrong-arch asset as universal (aarch64 when amd64 requested)" and "should not return wrong-arch asset as universal (x86_64 when arm64 requested)" have inconsistent spacing for struct fields; update those test case entries so the struct fields (name, assets, arch, wantErr) use the same tab indentation style as the surrounding tests (align fields with tabs, not spaces) in the test table (look for the table of test cases and the assets/arch/wantErr fields in the same block).pkg/version/clickhouse_test.go (1)
11-11: Extract duplicated version expression into a shared constant.The same long expression is duplicated in both tests; centralizing it will reduce drift risk when the expression changes.
♻️ Suggested refactor
+const clickhouseVersionExpr = `(tag.contains("-stable") || tag.contains("-lts")) ? (tag.substring(1).split("-")[0].split(".").size() >= 4 ? tag.substring(1).split("-")[0].split(".")[0] + "." + tag.substring(1).split("-")[0].split(".")[1] + "." + tag.substring(1).split("-")[0].split(".")[2] + "+" + tag.substring(1).split("-")[0].split(".")[3] : tag.substring(1).split("-")[0]) : ""` + func TestClickHouseVersionExpr(t *testing.T) { - expr := `(tag.contains("-stable") || tag.contains("-lts")) ? (tag.substring(1).split("-")[0].split(".").size() >= 4 ? tag.substring(1).split("-")[0].split(".")[0] + "." + tag.substring(1).split("-")[0].split(".")[1] + "." + tag.substring(1).split("-")[0].split(".")[2] + "+" + tag.substring(1).split("-")[0].split(".")[3] : tag.substring(1).split("-")[0]) : ""` + expr := clickhouseVersionExpr ... func TestClickHousePartialVersionConstraint(t *testing.T) { - expr := `(tag.contains("-stable") || tag.contains("-lts")) ? (tag.substring(1).split("-")[0].split(".").size() >= 4 ? tag.substring(1).split("-")[0].split(".")[0] + "." + tag.substring(1).split("-")[0].split(".")[1] + "." + tag.substring(1).split("-")[0].split(".")[2] + "+" + tag.substring(1).split("-")[0].split(".")[3] : tag.substring(1).split("-")[0]) : ""` + expr := clickhouseVersionExprAlso applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/version/clickhouse_test.go` at line 11, The long ClickHouse tag parsing expression assigned to expr in clickhouse_test.go is duplicated across tests; extract it into a shared constant (e.g., const clickhouseTagExpr) and replace both occurrences with that constant to avoid drift—declare the constant at package scope in the same test file (or a new test_helpers_test.go in the pkg/version package) and update the tests that reference expr to use clickhouseTagExpr instead.pkg/pipeline/fix_dylibs_darwin.go (1)
55-68: Universal binaries may report multiple architectures.The
filecommand on a universal (fat) binary returns both architectures (e.g., "Mach-O universal binary with 2 architectures: [x86_64:...] [arm64:...]"). The current loop returns the first match (arm64), which may not reflect the architecture actually used at runtime.Consider parsing more carefully or using
lipo -archsfor accurate detection on universal binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/pipeline/fix_dylibs_darwin.go` around lines 55 - 68, The detectArch function currently inspects `file` output and returns the first matching architecture, which is incorrect for universal binaries; change detectArch to accurately detect the runtime/desired arch by invoking `lipo -archs <path>` (or parsing its output) instead of or in addition to `file`, and return a single, explicit architecture string (e.g., "arm64" or "x86_64"); update any callers that expect detectArch to possibly return multiple entries to handle a single definitive arch.pkg/installer/installer.go (1)
839-841: Consider usingruntime.GOARCHnormalization for consistency.The cross-platform check compares
ArchOverridedirectly againstruntime.GOARCH. However,ArchOverridemight use aliases likex86_64whileruntime.GOARCHreturnsamd64. Consider usingarchMatchesfor consistency.♻️ Proposed fix
isCrossPlatform := (i.options.OSOverride != "" && i.options.OSOverride != runtime.GOOS) || - (i.options.ArchOverride != "" && i.options.ArchOverride != runtime.GOARCH) + (i.options.ArchOverride != "" && !archMatches(i.options.ArchOverride, runtime.GOARCH))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/installer/installer.go` around lines 839 - 841, The isCrossPlatform check currently compares i.options.ArchOverride directly to runtime.GOARCH which can mismatch due to aliasing; update the logic to use the existing archMatches helper (or equivalent) when comparing i.options.ArchOverride to runtime.GOARCH so aliases like "x86_64" map to "amd64" correctly—i.e., replace the direct ArchOverride == runtime.GOARCH comparison in the isCrossPlatform expression with a call to archMatches(i.options.ArchOverride, runtime.GOARCH) (keeping the OSOverride comparison as-is) so isCrossPlatform reliably detects cross-platform requests.
🤖 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/manager/asset_filter.go`:
- Around line 208-230: The hasOtherArchMarkers function uses an incomplete amd64
alias set and compares requestedArch without normalizing, causing false
negatives and positives; update hasOtherArchMarkers to reuse/sync with
getArchAliases (or the canonical alias map) adding missing aliases like "386",
"64bit", and "64-bit" for amd64, and normalize the incoming requestedArch to its
canonical key (e.g., map "x86_64"/"x64" -> "amd64") before the archKey ==
requestedArch check so the function skips only the requested architecture's
aliases when scanning asset.Name.
In `@pkg/pipeline/fix_dylibs_linux.go`:
- Around line 11-15: In detectBrokenDylibs, stop invoking ldd on possibly
untrusted binaries; replace the exec.Command("ldd", binaryPath) call with a
static ELF metadata inspection (e.g., run readelf -d or objdump -p on binaryPath
and parse lines with "NEEDED") to extract shared-library dependencies, then feed
those NEEDED entries into the existing DylibRef parsing/validation logic; ensure
errors from readelf/objdump are returned with context and that parsing handles
trimmed tokens and optional versioned sonames.
---
Nitpick comments:
In `@pkg/installer/installer.go`:
- Around line 839-841: The isCrossPlatform check currently compares
i.options.ArchOverride directly to runtime.GOARCH which can mismatch due to
aliasing; update the logic to use the existing archMatches helper (or
equivalent) when comparing i.options.ArchOverride to runtime.GOARCH so aliases
like "x86_64" map to "amd64" correctly—i.e., replace the direct ArchOverride ==
runtime.GOARCH comparison in the isCrossPlatform expression with a call to
archMatches(i.options.ArchOverride, runtime.GOARCH) (keeping the OSOverride
comparison as-is) so isCrossPlatform reliably detects cross-platform requests.
In `@pkg/manager/asset_filter_test.go`:
- Around line 308-323: The two new test cases named "should not return
wrong-arch asset as universal (aarch64 when amd64 requested)" and "should not
return wrong-arch asset as universal (x86_64 when arm64 requested)" have
inconsistent spacing for struct fields; update those test case entries so the
struct fields (name, assets, arch, wantErr) use the same tab indentation style
as the surrounding tests (align fields with tabs, not spaces) in the test table
(look for the table of test cases and the assets/arch/wantErr fields in the same
block).
In `@pkg/pipeline/fix_dylibs_darwin.go`:
- Around line 55-68: The detectArch function currently inspects `file` output
and returns the first matching architecture, which is incorrect for universal
binaries; change detectArch to accurately detect the runtime/desired arch by
invoking `lipo -archs <path>` (or parsing its output) instead of or in addition
to `file`, and return a single, explicit architecture string (e.g., "arm64" or
"x86_64"); update any callers that expect detectArch to possibly return multiple
entries to handle a single definitive arch.
In `@pkg/version/clickhouse_test.go`:
- Line 11: The long ClickHouse tag parsing expression assigned to expr in
clickhouse_test.go is duplicated across tests; extract it into a shared constant
(e.g., const clickhouseTagExpr) and replace both occurrences with that constant
to avoid drift—declare the constant at package scope in the same test file (or a
new test_helpers_test.go in the pkg/version package) and update the tests that
reference expr to use clickhouseTagExpr instead.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 550bc371-7af8-457c-9f2e-423cf3c3e6c2
📒 Files selected for processing (20)
cmd/deps/main.godeps.gopkg/config/defaults.yamlpkg/installer/errors.gopkg/installer/installer.gopkg/manager/asset_filter.gopkg/manager/asset_filter_test.gopkg/manager/github/github_tags.gopkg/manager/recommendations.gopkg/pipeline/cel.gopkg/pipeline/context.gopkg/pipeline/fix_dylibs.gopkg/pipeline/fix_dylibs_darwin.gopkg/pipeline/fix_dylibs_linux.gopkg/pipeline/fix_dylibs_other.gopkg/pipeline/fix_dylibs_test.gopkg/version/check.gopkg/version/clickhouse_test.gopkg/version/helpers.gopkg/version/resolver.go
| // hasOtherArchMarkers returns true if any asset contains an arch alias | ||
| // belonging to a different architecture than the requested one. | ||
| func hasOtherArchMarkers(assets []AssetInfo, requestedArch string) bool { | ||
| allArchAliases := map[string][]string{ | ||
| "amd64": {"amd64", "x86_64", "x64", "x86-64", "x86", "i386", "i686"}, | ||
| "arm64": {"arm64", "aarch64"}, | ||
| "arm": {"armv7", "armv7l"}, | ||
| } | ||
| for _, asset := range assets { | ||
| nameLower := strings.ToLower(asset.Name) | ||
| for archKey, aliases := range allArchAliases { | ||
| if archKey == requestedArch { | ||
| continue | ||
| } | ||
| for _, alias := range aliases { | ||
| if strings.Contains(nameLower, alias) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check call sites of filterByArch to see if arch is already normalized
rg -n -B5 -A2 'filterByArch\(' --type=goRepository: flanksource/deps
Length of output: 1459
🏁 Script executed:
# First, let's read the filterByArch function and see how it calls hasOtherArchMarkers
sed -n '168,210p' pkg/manager/asset_filter.goRepository: flanksource/deps
Length of output: 1346
🏁 Script executed:
# Get the getArchAliases function to compare alias definitions
sed -n '144,165p' pkg/manager/asset_filter.goRepository: flanksource/deps
Length of output: 560
🏁 Script executed:
# Check if there are test cases for hasOtherArchMarkers
rg -n 'hasOtherArchMarkers' --type=go pkg/manager/Repository: flanksource/deps
Length of output: 342
🏁 Script executed:
# Read the actual hasOtherArchMarkers function at lines 208-230
sed -n '208,230p' pkg/manager/asset_filter.goRepository: flanksource/deps
Length of output: 755
🏁 Script executed:
# Check test coverage for hasOtherArchMarkers with different arch forms
rg -n -A10 'hasOtherArchMarkers' pkg/manager/asset_filter_test.goRepository: flanksource/deps
Length of output: 42
Alias list inconsistency and missing arch normalization in hasOtherArchMarkers.
The function has two issues:
-
Incomplete amd64 alias list: Missing
"386","64bit", and"64-bit"compared togetArchAliases. This causes false negatives — assets with these markers won't be flagged as wrong architecture. -
Non-canonical arch handling: The
requestedArchparameter is not normalized (e.g.,"x86_64"vs"amd64"). If a non-canonical form is passed,archKey == requestedArchwon't match the canonical key, causing all aliases (including the requested architecture's own) to be checked and flagged as "other" markers — producing false positives.
Synchronize the alias lists and normalize requestedArch to the canonical form before comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/manager/asset_filter.go` around lines 208 - 230, The hasOtherArchMarkers
function uses an incomplete amd64 alias set and compares requestedArch without
normalizing, causing false negatives and positives; update hasOtherArchMarkers
to reuse/sync with getArchAliases (or the canonical alias map) adding missing
aliases like "386", "64bit", and "64-bit" for amd64, and normalize the incoming
requestedArch to its canonical key (e.g., map "x86_64"/"x64" -> "amd64") before
the archKey == requestedArch check so the function skips only the requested
architecture's aliases when scanning asset.Name.
| func detectBrokenDylibs(binaryPath, _ string) ([]DylibRef, error) { | ||
| out, err := exec.Command("ldd", binaryPath).Output() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("ldd failed: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is running ldd on untrusted executables considered unsafe on Linux, and what safer alternatives do glibc/binutils documentation recommend (e.g., readelf/objdump)?
💡 Result:
Yes, running ldd on untrusted executables is considered unsafe on Linux. The official ldd(1) man page warns: "Be aware that in some circumstances (e.g., where the program specifies an ELF interpreter other than ld-linux.so), some versions of ldd may attempt to obtain the dependency information by directly executing the program... Thus, you should never employ ldd on an untrusted executable." This can lead to arbitrary code execution if the binary has a malicious ELF interpreter. Safer alternatives recommended in the ldd man page and various technical sources include static analysis tools from binutils: readelf -d or objdump -p, which parse the ELF dynamic section without executing anything. For example: - readelf -d binary | grep 'NEEDED' - objdump -p binary | grep NEEDED These display the shared library dependencies (NEEDED entries) directly from the file structure. Binutils documentation for readelf and objdump confirms they inspect ELF files without execution, independent of BFD for readelf. Glibc's SECURITY.md notes: "The ldd tool is not expected to be used with untrusted executables," reinforcing the advice without specifying alternatives, but aligning with man page recommendations.
Citations:
- 1: https://www.man7.org/linux/man-pages/man1/ldd.1.html
- 2: https://jmmv.dev/2023/07/ldd-untrusted-binaries.html
- 3: https://medium.com/@anandaoo.it/why-running-ldd-on-untrusted-executables-is-a-security-risk-5e5cef653b3a
- 4: http://klamp.works/2016/04/15/code-exec-ldd.html
- 5: https://github.com/foss-for-synopsys-dwc-arc-processors/glibc/blob/arc-2025.09/SECURITY.md
- 6: https://sourceware.org/binutils/docs/binutils/readelf.html
- 7: https://sourceware.org/binutils/docs-2.35/binutils/objdump.html
- 8: https://ruvi-d.medium.com/linux-binary-compatibility-explained-at-5-levels-of-difficulty-ffeab6235fc8
- 9: https://duetorun.com/blog/20230620/gnu-binutils/
🏁 Script executed:
# First, let's read the entire fix_dylibs_linux.go file to understand the full context
cat -n pkg/pipeline/fix_dylibs_linux.goRepository: flanksource/deps
Length of output: 1331
🏁 Script executed:
# Check if there's any binary validation or trust checks before ldd is called
rg -n "detectBrokenDylibs" --type goRepository: flanksource/deps
Length of output: 467
🏁 Script executed:
# Look for any context about how binaries arrive in this pipeline
rg -n "fix_dylibs\|binaryPath" --type go -B 3 -A 3Repository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Let's examine fix_dylibs.go to understand the pipeline context
head -100 pkg/pipeline/fix_dylibs.go | cat -nRepository: flanksource/deps
Length of output: 3651
🏁 Script executed:
# Get context around line 79 where detectBrokenDylibs is called
sed -n '60,100p' pkg/pipeline/fix_dylibs.go | cat -nRepository: flanksource/deps
Length of output: 1665
🏁 Script executed:
# Look for how binaryPath is sourced (downloaded, local, etc.)
rg -n "binaryPath\s*=" pkg/pipeline/fix_dylibs.go -B 2 -A 2Repository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Find where DiagnoseLibraryIssues is called from
rg -n "DiagnoseLibraryIssues" --type go -B 2 -A 2Repository: flanksource/deps
Length of output: 1197
🏁 Script executed:
# Check if there's any download/validation logic in the codebase
rg -n "download\|untrusted\|verify\|signature" --type go -i | head -20Repository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Look for the main entry points and how binaries arrive in this pipeline
fd "main\.go|cmd" --type f | xargs rg -l "DiagnoseLibraryIssues\|fix_dylibs" 2>/dev/null || echo "No results"Repository: flanksource/deps
Length of output: 70
🏁 Script executed:
# Get more context from installer.go around the DiagnoseLibraryIssues call
sed -n '820,880p' pkg/installer/installer.go | cat -nRepository: flanksource/deps
Length of output: 3284
🏁 Script executed:
# Check if there's any signature verification or trust checks before binaries are executed
rg -n "verify\|signature\|checksum\|hash" pkg/installer/installer.go -i -B 2 -A 2 | head -40Repository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Check for any TODOs, FIXMEs, or security-related comments about ldd
rg -n "ldd\|TODO\|FIXME\|security\|untrust" --type go -iRepository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Check if there's any binary signature verification before installation
rg -n "signature\|gpg\|pgp\|verify.*binary" --type go -iRepository: flanksource/deps
Length of output: 42
🏁 Script executed:
# Look at the full installer flow to understand if binaries are verified before reaching this point
head -50 pkg/installer/installer.go | cat -nRepository: flanksource/deps
Length of output: 2109
Replace ldd with static ELF inspection to avoid executing untrusted binaries.
The ldd man page explicitly warns: "you should never employ ldd on an untrusted executable," as it may execute code from the target binary's interpreter on certain systems. Since this tool processes downloaded binaries, use readelf -d or objdump -p instead to extract NEEDED entries without execution.
At Line 12, replace:
out, err := exec.Command("ldd", binaryPath).Output()With static inspection via readelf -d binary | grep NEEDED or equivalent parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/pipeline/fix_dylibs_linux.go` around lines 11 - 15, In
detectBrokenDylibs, stop invoking ldd on possibly untrusted binaries; replace
the exec.Command("ldd", binaryPath) call with a static ELF metadata inspection
(e.g., run readelf -d or objdump -p on binaryPath and parse lines with "NEEDED")
to extract shared-library dependencies, then feed those NEEDED entries into the
existing DylibRef parsing/validation logic; ensure errors from readelf/objdump
are returned with context and that parsing handles trimmed tokens and optional
versioned sonames.
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/config/registry.go (1)
64-68:⚠️ Potential issue | 🟡 MinorStale comment contradicts new lazy re-initialization behavior.
The comment states the registry "will not reload until the program is restarted," but with the lazy re-initialization added to
GetGlobalRegistry(), the registry will now be reloaded on the next call afterResetGlobalRegistry().📝 Proposed fix
// ResetGlobalRegistry resets the global registry (useful for testing) -// Note: This will not reload the registry until the program is restarted -// since init functions only run once per program execution +// Note: The registry will be re-initialized on the next call to GetGlobalRegistry() func ResetGlobalRegistry() { globalRegistry = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/registry.go` around lines 64 - 68, Update the stale comment on ResetGlobalRegistry to reflect the new lazy re-initialization behavior: explain that ResetGlobalRegistry sets globalRegistry to nil and that GetGlobalRegistry() will recreate/reload the registry on the next call (i.e., lazy re-initialization), while preserving the note about init functions only running once per execution where relevant; reference ResetGlobalRegistry and GetGlobalRegistry by name so the maintainer can locate and update the comment accordingly.
🧹 Nitpick comments (1)
pkg/config/registry.go (1)
35-37: Potential data race in lazy re-initialization.If multiple goroutines call
GetGlobalRegistry()concurrently whenglobalRegistryis nil, both may read nil and callinitGlobalRegistry(), causing a data race on theglobalRegistrypointer. Since this primarily affects testing scenarios (afterResetGlobalRegistry()), consider usingsync.Oncefor thread-safe initialization if concurrent access is expected.♻️ Thread-safe lazy initialization using sync.Once
package config import ( + "sync" + "github.com/flanksource/deps/pkg/types" ) var globalRegistry *types.DepsConfig +var registryOnce sync.Once func init() { - initGlobalRegistry() + registryOnce.Do(initGlobalRegistry) }func GetGlobalRegistry() *types.DepsConfig { if globalRegistry == nil { - initGlobalRegistry() + registryOnce.Do(initGlobalRegistry) } return globalRegistry }Note: If
ResetGlobalRegistry()needs to allow re-initialization, you'd need to reset thesync.Onceas well (which requires a different pattern, such as usingsync.Mutexinstead).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/registry.go` around lines 35 - 37, GetGlobalRegistry currently checks if globalRegistry == nil and calls initGlobalRegistry(), which can race when multiple goroutines observe nil; change the lazy initialization to be thread-safe by protecting initialization with a sync.Once (call once.Do(initGlobalRegistry) inside GetGlobalRegistry) or, if ResetGlobalRegistry must allow re-initialization, replace the pattern with a sync.Mutex to serialize access in GetGlobalRegistry, ResetGlobalRegistry and initGlobalRegistry so only one goroutine can initialize/reset the globalRegistry at a time; update references to globalRegistry, GetGlobalRegistry, initGlobalRegistry and ResetGlobalRegistry accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/config/registry.go`:
- Around line 64-68: Update the stale comment on ResetGlobalRegistry to reflect
the new lazy re-initialization behavior: explain that ResetGlobalRegistry sets
globalRegistry to nil and that GetGlobalRegistry() will recreate/reload the
registry on the next call (i.e., lazy re-initialization), while preserving the
note about init functions only running once per execution where relevant;
reference ResetGlobalRegistry and GetGlobalRegistry by name so the maintainer
can locate and update the comment accordingly.
---
Nitpick comments:
In `@pkg/config/registry.go`:
- Around line 35-37: GetGlobalRegistry currently checks if globalRegistry == nil
and calls initGlobalRegistry(), which can race when multiple goroutines observe
nil; change the lazy initialization to be thread-safe by protecting
initialization with a sync.Once (call once.Do(initGlobalRegistry) inside
GetGlobalRegistry) or, if ResetGlobalRegistry must allow re-initialization,
replace the pattern with a sync.Mutex to serialize access in GetGlobalRegistry,
ResetGlobalRegistry and initGlobalRegistry so only one goroutine can
initialize/reset the globalRegistry at a time; update references to
globalRegistry, GetGlobalRegistry, initGlobalRegistry and ResetGlobalRegistry
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a960d7-0346-4306-94a6-5f580ec9c8ef
📒 Files selected for processing (3)
pkg/config/registry.gopkg/runtime/detector.gopkg/runtime/runtime_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/runtime/runtime_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/registry.go (1)
34-34: Gate success diagnostics behind debug/verbose mode.Line 34 unconditionally writes a success message to stderr on initialization. This can create unnecessary noise in CLI pipelines and brittle log expectations. The codebase uses verbose/debug flags to gate diagnostic output (see
cmd/update.go,cmd/lock.go); this informational message should follow the same pattern, either usinglogger.Debugf()or conditionally gated output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/registry.go` at line 34, The unconditional stderr diagnostic in initGlobalRegistry (the fmt.Fprintf writing "deps: initGlobalRegistry completed...") should be gated by the existing verbose/debug logging system: replace the direct os.Stderr write with a call to the project's debug logger (e.g., logger.Debugf) or wrap it behind the same verbose flag check used by cmd/update.go and cmd/lock.go, so the message about len(globalRegistry.Registry) only emits when verbose/debug is enabled.
🤖 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.go`:
- Around line 37-39: Comments around GetGlobalRegistry and ResetGlobalRegistry
are inconsistent: update the reset-related comments to reflect the new lazy
re-initialization behavior (GetGlobalRegistry now re-initializes the global
registry when it's nil) so they no longer claim a process restart is required;
specifically edit the comment block for GetGlobalRegistry and any comments on
ResetGlobalRegistry to state that ResetGlobalRegistry sets the global registry
to nil and that GetGlobalRegistry will reload defaults + user config on next
access, referencing the functions GetGlobalRegistry and ResetGlobalRegistry to
make the behavior explicit.
- Around line 40-42: globalRegistry is accessed concurrently without
synchronization (seen in GetGlobalRegistry and other functions like
ResetGlobalRegistry/SetGlobalRegistry), causing races and duplicate
initialization; fix by adding a package-level synchronization primitive (e.g., a
sync.RWMutex named registryMu or a sync.Once for one-time init) and guard all
reads/writes to globalRegistry: use registryMu.RLock/RUnlock for reads in
GetGlobalRegistry, registryMu.Lock/Unlock for writes in SetGlobalRegistry and
ResetGlobalRegistry, and ensure initGlobalRegistry is called under the write
lock or protected by sync.Once to prevent double initialization.
---
Nitpick comments:
In `@pkg/config/registry.go`:
- Line 34: The unconditional stderr diagnostic in initGlobalRegistry (the
fmt.Fprintf writing "deps: initGlobalRegistry completed...") should be gated by
the existing verbose/debug logging system: replace the direct os.Stderr write
with a call to the project's debug logger (e.g., logger.Debugf) or wrap it
behind the same verbose flag check used by cmd/update.go and cmd/lock.go, so the
message about len(globalRegistry.Registry) only emits when verbose/debug is
enabled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 390eb5bf-9e01-419c-997a-c4ec5cc8158e
📒 Files selected for processing (2)
pkg/config/registry.gopkg/runtime/detector.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/runtime/detector.go
| if globalRegistry == nil { | ||
| initGlobalRegistry() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unsynchronized shared-state access patterns in this file.
rg -n -C2 '\bglobalRegistry\b|sync\.(Mutex|RWMutex|Once)|atomic\.' pkg/config/registry.goRepository: flanksource/deps
Length of output: 1021
🏁 Script executed:
cat -n pkg/config/registry.go | head -100Repository: flanksource/deps
Length of output: 2734
Add synchronization to prevent concurrent access race conditions on globalRegistry.
Lines 40-42 (and other accesses in ResetGlobalRegistry/SetGlobalRegistry) read and write globalRegistry without any locking. Concurrent callers can race—for example, multiple threads calling GetGlobalRegistry() simultaneously with nil value will both invoke initGlobalRegistry(), triggering duplicate initialization and potential data races on the global state.
Suggested fix (mutex-guarded access)
import (
"fmt"
"os"
+ "sync"
"github.com/flanksource/deps/pkg/types"
)
var globalRegistry *types.DepsConfig
+var globalRegistryMu sync.Mutex
func init() {
+ globalRegistryMu.Lock()
+ defer globalRegistryMu.Unlock()
initGlobalRegistry()
}
@@
func GetGlobalRegistry() *types.DepsConfig {
+ globalRegistryMu.Lock()
+ defer globalRegistryMu.Unlock()
if globalRegistry == nil {
initGlobalRegistry()
}
return globalRegistry
}
@@
func ResetGlobalRegistry() {
+ globalRegistryMu.Lock()
+ defer globalRegistryMu.Unlock()
globalRegistry = nil
}
@@
func SetGlobalRegistry(config *types.DepsConfig) {
+ globalRegistryMu.Lock()
+ defer globalRegistryMu.Unlock()
globalRegistry = config
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/registry.go` around lines 40 - 42, globalRegistry is accessed
concurrently without synchronization (seen in GetGlobalRegistry and other
functions like ResetGlobalRegistry/SetGlobalRegistry), causing races and
duplicate initialization; fix by adding a package-level synchronization
primitive (e.g., a sync.RWMutex named registryMu or a sync.Once for one-time
init) and guard all reads/writes to globalRegistry: use registryMu.RLock/RUnlock
for reads in GetGlobalRegistry, registryMu.Lock/Unlock for writes in
SetGlobalRegistry and ResetGlobalRegistry, and ensure initGlobalRegistry is
called under the write lock or protected by sync.Once to prevent double
initialization.
moshloop
force-pushed
the
feat/arch-mismatch-detection
branch
from
6aa7f88 to
c76c3e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/config/config_ginkgo_test.go (1)
15-15: Prefer intent-based assertion over a hardcoded registry count.Line 15 (
> 50) is brittle for legitimate defaults cleanup. A non-empty assertion plus required key checks (Lines 16-17) is more stable.Proposed test tweak
- Expect(len(config.Registry)).To(BeNumerically(">", 50)) + Expect(config.Registry).ToNot(BeEmpty())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config_ginkgo_test.go` at line 15, Replace the brittle numeric assertion on the registry length with an intent-based non-empty check: instead of Expect(len(config.Registry)).To(BeNumerically(">", 50)) assert that config.Registry is not empty (e.g., Expect(config.Registry).ToNot(BeEmpty()) or Expect(len(config.Registry)).To(BeNumerically(">", 0))) and keep the subsequent required key checks (the assertions referencing specific keys on config.Registry) intact; update the test in config_ginkgo_test.go around the Expect on config.Registry accordingly.
🤖 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/config_ginkgo_test.go`:
- Around line 13-17: The embedded defaults.yaml contains duplicate mapping keys
for the "step" registry entry which causes YAML unmarshal to fail before
LoadDefaultConfig() can be asserted in pkg/config/config_ginkgo_test.go; open
pkg/config/defaults.yaml, locate the two "step" mapping entries (around the
reported duplicate lines) and merge them into a single "step" registry entry
preserving all unique fields (or remove the redundant one), ensuring there are
no duplicate keys so LoadDefaultConfig() can parse the defaults and the test
assertions on config.Registry (including
Expect(config.Registry).To(HaveKey("step"))) succeed.
---
Nitpick comments:
In `@pkg/config/config_ginkgo_test.go`:
- Line 15: Replace the brittle numeric assertion on the registry length with an
intent-based non-empty check: instead of
Expect(len(config.Registry)).To(BeNumerically(">", 50)) assert that
config.Registry is not empty (e.g., Expect(config.Registry).ToNot(BeEmpty()) or
Expect(len(config.Registry)).To(BeNumerically(">", 0))) and keep the subsequent
required key checks (the assertions referencing specific keys on
config.Registry) intact; update the test in config_ginkgo_test.go around the
Expect on config.Registry accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12235cdd-c78b-477d-9825-03407fffb4f6
📒 Files selected for processing (5)
.github/workflows/test.ymlpkg/config/config_ginkgo_test.gopkg/config/registry.gopkg/runtime/detector.gopkg/runtime/runtime_test.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yml
- pkg/runtime/runtime_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/runtime/detector.go
- pkg/config/registry.go
…stall Exit with code 2 on VersionMismatchError to distinguish from other failures. Detects when downloaded binary architecture doesn't match requested --arch override and skips cached installation. Validates installed version matches expected and provides diagnostic suggestions for library/architecture issues. BREAKING CHANGE: detect and reject architecture mismatches during install
…t param in deleteFiles
- Add //go:build darwin constraint to fix_dylibs_test.go - Use fmt.Fprintf instead of WriteString(fmt.Sprintf) (staticcheck QF1012) - Skip version verification for cross-platform and alias installs - Use full bin-dir path for version checks - Fix jq version_regex to capture patch version - Simplify clickhouse version_expr and capture 4-part version - Use context param in deleteFiles error message
For directory-mode packages with symlinks, binPath resolves to a symlink to a binary file. Passing directory mode to version check causes it to treat the symlink as a working directory, which fails.
- Extract init() body into initGlobalRegistry() for reuse - Make GetGlobalRegistry() lazily re-initialize when nil - Add nil guard and registry-size diagnostic in installRuntime - Add LoadDefaultConfig test to catch yaml parsing errors early - Increase PowerShell test timeout to 120s for CI install
moshloop
force-pushed
the
feat/arch-mismatch-detection
branch
from
731d2e1 to
8003a8f
Compare
- Add prefix matching to CompareVersions so "26.2.5.45" matches "26.2.5.45-stable" (clickhouse-style versions) - Use CheckBinaryVersion in finalizeInstallation to properly resolve binary paths for directory-mode packages with symlinks - Fix resolveBinaryPath to skip shell-wrapped commands (bash -c) which must run in directory mode - Fix svu version_regex to match GitVersion output format - Fix powershell/uv version_command: remove binary name prefix since the binary path is already resolved - Fix double-backslash regexes in single-quoted YAML for activemq, activemq-artemis, and tomee
… version transformation Add version_fallback field to Package that transforms the resolved version before URL templating. This allows kube-apiserver on darwin/windows to use envtest minor-only versions (e.g., 1.35.0) while linux uses the exact patch version (e.g., 1.35.3) from kubernetes/kubernetes tags.
Refactor GetInstalledVersionWithMode to return raw command output alongside the parsed version. This enables verify_expr CEL expressions to match against the full output when standard semver comparison fails. Adds verify_expr to openjdk8 config to accept 1.8.0/8.0.x outputs that don't match the jdk8u482-b08 tag format.
… files The build job already uploads binaries via svenstaro/upload-release-action. The release job just needs to update the release with generated notes instead of trying to create a new release with files from a non-existent release/ directory.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests