fix(app): Find resolves programs on Windows — PATHEXT candidates, no mode bit - #22
Merged
Merged
Conversation
…mode bit Three defects, one function, all Windows-fatal and all invisible from a POSIX runner: 1. isExecutable asked mode&0111. Windows has no execute bit — Stat synthesises 0666, or 0444 for read-only — so the test rejected EVERY file on the platform, git.exe included, and Find could never succeed there. This is the 'Program.Find: "git": not found in PATH' arm of the go-inference Windows failures (3 packages), and the same defect go-process fixed in v0.16.2. 2. No extension candidates: PathJoin(dir, "git") never tries git.exe, so fixing the mode test alone changes nothing. 3. The path-vs-name test matched only the platform separator, so "bin/tool" — which Go accepts on Windows — was hunted on PATH instead of checked directly. Shape mirrors go-process's proven fix: findWith/isExecutableWith take PATH and PATHEXT as arguments, and the extension list IS the platform switch (empty = POSIX mode-bit semantics, non-empty = Windows PATHEXT semantics), so every Windows rule is exercised by fixture from any host. Find passes the real environment, defaulting PATHEXT to Go's own .COM;.EXE;.BAT;.CMD when unset on Windows. Receipt: TestApp_isExecutableWith_Good is a 0644 git.exe accepted under a listed extension — the exact file the old logic rejected. Full module: ok, vet + gofmt clean. POSIX behaviour byte-identical (existing Find/isExecutable triplets pass unmodified). Co-Authored-By: Virgil <virgil@lethean.io>
Contributor
📝 WalkthroughWalkthroughChangesThe executable lookup now supports Windows Executable resolution
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app_internal_test.go`:
- Around line 76-103: Add the missing splitPathExt _Ugly test and
executableCandidates _Bad and _Ugly tests alongside the existing coverage.
Exercise boundary and invalid inputs, including whitespace/empty extension
entries and unusual or empty executable names, while preserving the current
_Good and splitPathExt _Bad expectations.
In `@app.go`:
- Around line 50-55: The PATHEXT lookup currently affects POSIX behavior; update
both path-resolution sites in app.go at lines 50-55 and 157-161 to read or
initialize pathExt only when OS() == "windows", leaving it empty on POSIX so
execute-bit checks remain unchanged. Apply the change consistently in both
methods using the existing findWith flow.
- Line 67: Update the path classification condition in the relevant
filename-resolution function to recognize drive-prefixed Windows paths such as
C:git as direct paths, including when extension-based Windows mode is active,
rather than searching PATH. Add a regression test covering this drive-relative
input and asserting direct-path resolution.
🪄 Autofix
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 Plus
Run ID: b44d23e0-5894-4567-84db-110ee5ca7dfb
📒 Files selected for processing (2)
app.goapp_internal_test.go
Comment on lines
+76
to
+103
| func TestApp_splitPathExt_Good(t *T) { | ||
| exts := splitPathExt(".COM;.EXE;bat; .Cmd ;") | ||
| AssertEqual(t, 4, len(exts)) | ||
| AssertEqual(t, ".com", exts[0]) | ||
| AssertEqual(t, ".exe", exts[1]) | ||
| AssertEqual(t, ".bat", exts[2]) | ||
| AssertEqual(t, ".cmd", exts[3]) | ||
| } | ||
| func TestApp_splitPathExt_Bad(t *T) { | ||
| AssertEqual(t, 0, len(splitPathExt(""))) | ||
| } | ||
| func TestApp_executableCandidates_Good(t *T) { | ||
| // A name already carrying a listed extension is tried as itself | ||
| // first, then with each extension appended; a bare name only with | ||
| // the extensions; POSIX (empty list) is the path alone. | ||
| withExt := executableCandidates("dir/git.exe", ".COM;.EXE") | ||
| AssertEqual(t, 3, len(withExt)) | ||
| AssertEqual(t, "dir/git.exe", withExt[0]) | ||
|
|
||
| bare := executableCandidates("dir/git", ".COM;.EXE") | ||
| AssertEqual(t, 2, len(bare)) | ||
| AssertEqual(t, "dir/git.com", bare[0]) | ||
| AssertEqual(t, "dir/git.exe", bare[1]) | ||
|
|
||
| posix := executableCandidates("dir/git", "") | ||
| AssertEqual(t, 1, len(posix)) | ||
| AssertEqual(t, "dir/git", posix[0]) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the mandatory coverage states.
splitPathExt has no _Ugly test. executableCandidates has no _Bad or _Ugly test. Add the required test functions with boundary and invalid-input cases.
As per coding guidelines, “All three coverage states are mandatory: _Good for happy path, _Bad for expected failures, and _Ugly for edge cases”.
🤖 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 `@app_internal_test.go` around lines 76 - 103, Add the missing splitPathExt
_Ugly test and executableCandidates _Bad and _Ugly tests alongside the existing
coverage. Exercise boundary and invalid inputs, including whitespace/empty
extension entries and unusual or empty executable names, while preserving the
current _Good and splitPathExt _Bad expectations.
Source: Coding guidelines
Comment on lines
+50
to
+55
| pathExt := Env("PATHEXT") | ||
| if pathExt == "" && OS() == "windows" { | ||
| // Go's own default when the variable is unset. | ||
| pathExt = ".COM;.EXE;.BAT;.CMD" | ||
| } | ||
| return a.findWith(filename, name, Env("PATH"), pathExt) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict PATHEXT processing to Windows.
A populated PATHEXT on POSIX changes these methods from execute-bit checks to extension checks. This breaks the stated unchanged POSIX behaviour.
app.go#L50-L55: initialisepathExtonly whenOS() == "windows".app.go#L157-L161: initialisepathExtonly whenOS() == "windows".
📍 Affects 1 file
app.go#L50-L55(this comment)app.go#L157-L161
🤖 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 `@app.go` around lines 50 - 55, The PATHEXT lookup currently affects POSIX
behavior; update both path-resolution sites in app.go at lines 50-55 and 157-161
to read or initialize pathExt only when OS() == "windows", leaving it empty on
POSIX so execute-bit checks remain unchanged. Apply the change consistently in
both methods using the existing findWith flow.
| // as a separator on Windows too, so both spellings mean "path" — | ||
| // matching only the platform separator sent "bin/tool" on a PATH | ||
| // hunt there instead of probing it. | ||
| if Contains(filename, string(PathSeparator)) || Contains(filename, "/") { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat drive-relative Windows paths as direct paths.
A valid Windows path such as C:git contains neither \ nor /. Line 67 therefore searches PATH instead of resolving the path relative to drive C:. When extension-based Windows mode applies, detect drive-prefixed paths and use the direct-path branch. Add a regression test for this case.
Proposed fix
- if Contains(filename, string(PathSeparator)) || Contains(filename, "/") {
+ if Contains(filename, string(PathSeparator)) || Contains(filename, "/") ||
+ (len(splitPathExt(pathExt)) != 0 && Contains(filename, ":")) {🤖 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 `@app.go` at line 67, Update the path classification condition in the relevant
filename-resolution function to recognize drive-prefixed Windows paths such as
C:git as direct paths, including when extension-based Windows mode is active,
rather than searching PATH. Add a regression test covering this drive-relative
input and asserting direct-path resolution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
Program.Find: "git": not found in PATHarm of the go-inference Windows suite (3 packages: agent/orchestrator, agent/provider, engine/driver — see the classified 34 in go-inference's ledger).Three defects, one function, all invisible from POSIX runners:
PathJoin(dir, "git")never triesgit.exe, so the mode fix alone changes nothing.bin/tool(valid on Windows) was hunted on PATH instead of checked directly.Shape mirrors go-process's proven fix:
findWith/isExecutableWithtake PATH + PATHEXT as arguments and the extension list is the platform switch — empty = POSIX semantics, non-empty = Windows semantics — so the Windows rules are fixture-tested from any host.Findpasses the real environment, defaulting PATHEXT to Go's own.COM;.EXE;.BAT;.CMDwhen unset on Windows.Receipt:
TestApp_isExecutableWith_Goodis a 0644git.exeaccepted under a listed extension — the exact file the old logic rejected, failing against the old code for the right reason. Full module green, vet + gofmt clean, existing Find/isExecutable triplets pass unmodified (POSIX behaviour byte-identical).core/go is review-only: this awaits Snider's review alongside #21 — together they clear 33 of go-inference's 34 remaining Windows failures.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Bug Fixes
PATHEXTand standard executable extensions.Tests