fix: skip Homebrew dependency for skill install on Linux (#14593)#55940
fix: skip Homebrew dependency for skill install on Linux (#14593)#55940lupuletic wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a best-effort apt-get fallback for Confidence Score: 5/5Safe to merge — the fallback is correctly gated and fails gracefully; all identified issues are minor style concerns. No P0 or P1 issues found. Formula validation is already enforced upstream via No files require special attention beyond the P2 style notes in
|
| Filename | Overview |
|---|---|
| src/agents/skills-install.ts | Adds installBrewFormulaViaApt to fall back to apt-get on Linux when Homebrew is absent. Correctly reuses the root/sudo pattern from installGoViaApt. Minor issues: formula should be trimmed inside the helper, the new function duplicates ~50 lines from installGoViaApt, and the && spec.formula guard is redundant. |
| src/agents/skills-install-fallback.test.ts | Adds 6 focused tests covering the new brew-to-apt fallback: root install, sudo install, apt failure, no apt-get, sudo password required, and macOS no-op. Tests correctly override process.platform with a proper afterEach restore. Assertions match actual error messages produced by installBrewFormulaViaApt. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/skills-install.ts
Line: 302
Comment:
**Formula not trimmed before passing to apt-get**
`buildInstallCommand` uses `spec.formula.trim()` for the brew invocation (line 149), but `installBrewFormulaViaApt` receives and uses the formula as-is. `assertSafeInstallerValue` validates the *trimmed* value, so a formula stored with leading/trailing whitespace (e.g. `" openai-whisper "`) would pass validation and then be forwarded to apt-get untrimmed — causing a package-not-found failure.
```suggestion
const aptInstallArgv = ["apt-get", "install", "-y", formula.trim()];
const aptUpdateArgv = ["apt-get", "update", "-qq"];
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/skills-install.ts
Line: 298-347
Comment:
**Significant duplication with `installGoViaApt`**
`installBrewFormulaViaApt` (50 lines) is structurally identical to `installGoViaApt` (51 lines) — same root-check, same sudo probe, same `runBestEffortCommand` for `apt-get update`, same error shape. The only differences are the package name and the failure message strings.
Consider extracting a shared `installViaApt(pkg, failureMessage, timeoutMs)` helper and calling it from both functions to avoid the duplication and make future maintenance easier.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/skills-install.ts
Line: 547
Comment:
**Redundant `spec.formula` guard**
`buildInstallCommand` (called a few lines above on line 530) already returns an error and exits early if `spec.formula` is missing or invalid. By the time execution reaches this line, `spec.formula` is guaranteed to be a non-empty, validated string. The `&& spec.formula` check is harmless but unnecessary clutter.
```suggestion
if (process.platform === "linux" && hasBinary("apt-get")) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: fall back to apt-get for brew skill..." | Re-trigger Greptile
007f851 to
abf5539
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf5539586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
abf5539 to
a2cf27b
Compare
a2cf27b to
dd9f6b8
Compare
Summary
kind: "brew"skill install runs on Linux without Homebrew, fall back toapt-get install <formula>(root or passwordless sudo) before giving upinstallGoViaAptpattern already used forgokind installsapt-get), behavior is unchanged -- the existing "brew not installed" error is returnedbrew not installedon Linux container #14593Test plan
skills-install-fallback.test.tscovering: root apt install, sudo apt install, apt failure, no apt-get available, sudo password required, macOS no-fallbackpnpm checkpasses (lint, typecheck, format)pnpm test -- src/agents/skills-install-fallback.test.tspasses (9/9)pnpm test -- src/agents/skills-install.test.tspasses (2/2)AI-Assisted PR Checklist:
🤖 Generated with Claude Code