fix(upgrade): verify tool is brew-managed before routing to brew upgrade#904
fix(upgrade): verify tool is brew-managed before routing to brew upgrade#904decode2 wants to merge 8 commits into
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:
📝 WalkthroughWalkthrough
ChangesBrew-managed detection for upgrade strategy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/upgrade/strategy.go`:
- Around line 305-318: The isBrewManaged function uses brew list without
explicit filtering flags, which has unreliable exit code semantics across
Homebrew versions. Since the function's contract states it should detect tools
installed "either as a formula or a cask," modify the function to make two
separate execCommandContext calls to brew list, one with the --formula flag and
one with the --cask flag, and return true if either call succeeds (returns nil).
This explicit probing approach ensures correct detection across different
Homebrew versions.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a180db3c-f0b9-47c9-bf78-815fba8e3bfa
📒 Files selected for processing (5)
internal/cli/run.gointernal/cli/run_component_paths_test.gointernal/update/upgrade/executor.gointernal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
decode2
force-pushed
the
fix/brew-upgrade-non-brew-tools
branch
from
601dd6f to
5c412c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/upgrade/executor.go`:
- Around line 500-501: The effectiveMethod function is being called multiple
times for the same tool (at the code around lines 500-501, 513-514, 521-525, and
within runStrategy at lines 571-577) instead of once per tool, causing repeated
external shell calls via isBrewManaged and creating TOCTOU risks. Compute
effectiveMethod(ctx, r.Tool, profile) once at the start of the tool processing
flow, store the result in a variable, and pass this precomputed method value
through all execution paths instead of recomputing it. Update all the affected
sites to use the stored method value: replace the effectiveMethod calls at lines
500-501, 513-514, 521-525 with references to the precomputed variable, and
modify runStrategy to accept the precomputed method as a parameter instead of
computing it internally at lines 571-577.
In `@internal/update/upgrade/strategy_test.go`:
- Around line 359-365: The test table in the struct definition is missing a test
case that exercises the mixed formula/cask outcome behavior. Currently all test
cases use the same brewListExit value for both probes, so the code path where
one probe fails (like --formula returning non-zero exit) while the other
succeeds (--cask returning zero) is never verified. Add at least one test case
to the tests slice where brewListExit differs from another field or add multiple
entries with different exit codes to cover the scenario where --formula fails
but --cask succeeds, ensuring the fallback/dual-probe logic is fully exercised
in the test coverage.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4704b492-64b6-4a49-892b-81c614bf7826
📒 Files selected for processing (3)
internal/update/upgrade/executor.gointernal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
This is the lower-risk direction because Homebrew ownership should come from Homebrew, not from path heuristics. Please cache the resolved method once per tool so we do not run repeated brew list probes during the same upgrade path. Also add one mixed probe test where --formula fails and --cask succeeds. After that, this is the PR I would keep for #186.
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)
internal/update/upgrade/strategy.go (1)
77-83: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRefresh the strategy-routing comment to match current behavior.
Line 77 still says brew profile always routes to brew, but routing is now conditional on
isBrewManaged(ctx, toolName). Keeping this comment stale can mislead future changes.Suggested doc-only fix
-// - brew profile → brewUpgrade (regardless of tool's declared method) +// - brew profile + brew-managed tool → brewUpgrade +// - brew profile + non-brew-managed tool → fall through to go-install/declared method🤖 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 `@internal/update/upgrade/strategy.go` around lines 77 - 83, The upgrade strategy routing comment describes the brew profile routing as unconditional with "regardless of tool's declared method", but the actual implementation now conditionally routes based on the isBrewManaged function check. Update the comment block describing the upgrade strategy routing to accurately reflect that brew profile routing is conditional on isBrewManaged(ctx, toolName) rather than being an unconditional path, ensuring the documentation matches the current implementation behavior.
🤖 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 `@internal/update/upgrade/strategy.go`:
- Around line 77-83: The upgrade strategy routing comment describes the brew
profile routing as unconditional with "regardless of tool's declared method",
but the actual implementation now conditionally routes based on the
isBrewManaged function check. Update the comment block describing the upgrade
strategy routing to accurately reflect that brew profile routing is conditional
on isBrewManaged(ctx, toolName) rather than being an unconditional path,
ensuring the documentation matches the current implementation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d5ddb85-81bc-4c28-ad15-83ba7e09b260
📒 Files selected for processing (2)
internal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
|
Changes requested have been applied: ✅ Cache �ffectiveMethod result per tool to avoid repeated �rew list probes The cache is implemented at the isBrewManaged() level with a package-level map protected by sync.RWMutex. This ensures we don't spawn multiple �rew list processes for the same tool during a single upgrade run. Ready for re-review. |
|
@Alan-TheGentleman Changes requested have been applied and are ready for re-review. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Approved. The routing change is conceptually correct: a brew profile should not force every tool through Homebrew unless the specific tool is actually brew-managed.
The cache addresses the repeated-probe concern well enough for this CLI process model. This does not solve the Windows PATH shadowing issue in #943, so that should stay separate.
Head branch was pushed to by a user without write access
|
Hi @Alan-TheGentleman, I've updated this PR:
Ready for re-review! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/upgrade/strategy_test.go`:
- Around line 1860-1862: The runStrategyTest helper function invokes
effectiveMethod which routes to execCommandContext for brew package managers,
but execCommandContext is not stubbed in tests, causing real subprocess calls to
the host's Homebrew and making tests non-deterministic. Additionally,
clearBrewManagedCache is never called, allowing cached results to leak between
tests. Stub execCommandContext (similar to how execCommand is stubbed elsewhere
in the test file) and call clearBrewManagedCache inside runStrategyTest to
ensure deterministic behavior and isolation for all tests that use this helper,
including TestRunStrategy_BrewUpgrade, TestRunStrategy_BrewUpgradeFailure,
TestRunStrategy_ExecErrorWrapped, and other brew-related test cases.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0e7fd39-292b-467e-bf4d-f8570d7ccf04
📒 Files selected for processing (4)
internal/update/upgrade/executor.gointernal/update/upgrade/executor_test.gointernal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
internal/update/upgrade/executor.go (1)
495-517: 🚀 Performance & Scalability | 🔵 TrivialAvoid
brew listsubprocess probes when labeling skipped tools.For dev-build and version-unknown tools (which are reported as
UpgradeSkippedand never executed),effectiveMethod(ctx, ...)invokesisBrewManaged, which shells out tobrew list --formula/--cask --versionssolely to populate the informationalMethodfield. On a brew profile this probes brew for each distinct skipped tool, adding latency to the upgrade command for tools that won't be upgraded. Consider computing the display label without the brew probe for these non-executed paths (e.g. fall back to the declared method, since a source/dev build is never brew-managed).Also applies to line 513.
🤖 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 `@internal/update/upgrade/executor.go` around lines 495 - 517, The effectiveMethod function is being called for devBuilds and versionUnknowns which are both marked as UpgradeSkipped and never executed, causing unnecessary brew subprocess probes on brew profiles. In both loops (the devBuilds loop starting at line 495 and the versionUnknowns loop at line 513), replace the call to effectiveMethod(ctx, r.Tool, profile) with the declared method from the tool directly (such as r.Tool.Method) to avoid the expensive brew list subprocess probe since source/dev builds are never brew-managed anyway and these tools won't be upgraded regardless.
🤖 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 `@internal/update/upgrade/executor.go`:
- Around line 495-517: The effectiveMethod function is being called for
devBuilds and versionUnknowns which are both marked as UpgradeSkipped and never
executed, causing unnecessary brew subprocess probes on brew profiles. In both
loops (the devBuilds loop starting at line 495 and the versionUnknowns loop at
line 513), replace the call to effectiveMethod(ctx, r.Tool, profile) with the
declared method from the tool directly (such as r.Tool.Method) to avoid the
expensive brew list subprocess probe since source/dev builds are never
brew-managed anyway and these tools won't be upgraded regardless.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 61b8e84e-335a-4371-8661-e1d9815cd879
📒 Files selected for processing (4)
internal/update/upgrade/executor.gointernal/update/upgrade/executor_test.gointernal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Good direction, but this cannot merge while the PR does not compile. The current Unit Tests job fails in internal/update/upgrade/strategy_test.go because two test call sites still call runStrategy(ctx, result, profile) after the function was changed to require the resolved install method as a fourth argument. Please update those remaining calls, rerun the unit tests, and ping for re-review.
On Linux systems with Linuxbrew installed for unrelated packages, profile.PackageManager is set to 'brew' even when tools were installed via go install or binary download. This caused 'brew upgrade <tool>' to fail with 'No available formula' errors. Added isBrewManaged() helper that probes 'brew list --versions <tool>' to verify the tool is actually managed by Homebrew. effectiveMethod() now checks this before routing to InstallBrew, falling through to go-install or the declared method when the tool is not brew-managed. Closes Gentleman-Programming#186
brew list --versions without explicit type flags has inconsistent exit-code semantics across Homebrew versions. Probe --formula and --cask separately to ensure correct detection regardless of Homebrew version.
effectiveMethod is called multiple times per tool during planning, execution, and verification. Each call previously spawned 2 brew processes (one for --formula, one for --cask). Cache the result per tool name to avoid redundant brew list calls during a single upgrade session. Also adds tests for mixed probe scenarios (formula fails + cask succeeds) and cache behavior verification. Addresses review feedback from @Alan-TheGentleman
… brew routing Update the runStrategy documentation comment to accurately reflect that brew routing is now conditional on isBrewManaged(ctx, toolName), not unconditional for brew profiles. Addresses CodeRabbit review feedback.
Head branch was pushed to by a user without write access
decode2
force-pushed
the
fix/brew-upgrade-non-brew-tools
branch
from
52c03b8 to
c2c51d2
Compare
|
@Alan-TheGentleman ready for re-review! I've rebased the branch on main and resolved the compilation failures in strategy_test.go by updating the outdated runStrategy call sites. |
|
@Alan-TheGentleman all your review points are addressed on the current head (
CI is green and CodeRabbit's latest pass has no actionable comments. Ready for re-review whenever you have a moment 🙏 |
2 participants
Summary
Fixes upgrade failures on Linux systems where Linuxbrew is installed for unrelated packages. Previously,
effectiveMethod()would route ALL tools tobrew upgradewhenprofile.PackageManager == "brew", even if the tool was installed viago installor binary download.Root Cause
On Linux, platform detection sets
PackageManager = "brew"when Linuxbrew is present on PATH — regardless of how individual tools were actually installed. The upgrade logic then blindly routed every tool tobrew upgrade, which failed with "No available formula" for tools not managed by Homebrew.Fix
Added
isBrewManaged(ctx, toolName)helper instrategy.gothat probesbrew list --versions <toolName>to verify the tool is actually installed via Homebrew.Updated
effectiveMethod()to checkisBrewManaged()before routing toInstallBrew. If the tool is not brew-managed, it falls through to the next priority (go-install or declared method).Added
execCommandContextpackage var for testability (same pattern as existingexecCommand).Tests
TestEffectiveMethodwith new cases for issue fix(upgrade): brew upgrade fails for non-brew tools on Linux with Linuxbrew #186:brew profile + tool NOT brew-managed + go-install → go-installbrew profile + tool NOT brew-managed + binary → binarymockExitCommandhelper for simulatingbrew listexit codesCloses #186
Summary by CodeRabbit
Bug Fixes
Improvements
Tests