fix(process): Program.Find used a SECOND, unfixed copy of lookPath - #10
Conversation
v0.16.2 fixed executable resolution on Windows and did not fix executable resolution on Windows, because this module carried TWO copies of it: exec/exec.go lookPath <- fixed in v0.16.2 os_exec_link.go lookPath <- byte-identical, untouched Program.Find calls the second. os_exec_link.go:45 still read `return info.Mode()&0111 != 0` — the exact defect v0.16.2's own commit message spends three paragraphs on — so every consumer resolving through Program.Find kept failing with `Program.Find: "git": not found in PATH`, on a release that claimed the opposite. go-inference's windows lane measured it: 41 occurrences before v0.16.2, 41 after. A fix is not landed until you have grepped for its siblings. Byte-identical duplicates are how a fixed defect stays live. So the fix is not a second copy of the fix. internal/lookpath now holds ONE implementation and both consumers call it — because the duplication IS the defect's cause, and patching in place would guarantee a third divergence. The three Windows defects it carries are unchanged from v0.16.2: no %PATHEXT% expansion, a mode&0111 test the platform can never satisfy, and a path-vs-name check that missed '/'. commandContext no longer discards the resolution failure either. It records it on Cmd.Err exactly as exec.Command does with its own LookPath error — Start returns it without running anything. Deferring to Start rather than returning early is deliberate: an early return skipped Service.start's exited-event broadcast, which TestService_Actions/broadcasts_exited_event_on_start_failure caught. The callers' failure handling is untouched. Receipts — macOS, GOWORK=off (what CI runs): go test -count=1 ./... ok process 13.478s · exec 0.733s · internal/lookpath 0.499s · pkg/api 2.800s golangci-lint run ./... 0 issues gofmt -l · go vet: clean TestProgram_Find_UsesSharedResolution pins the miss itself: Program.Find must land on the same path the shared resolver reports. It fails the moment a third copy appears or the two drift apart again — which a version bump could not detect and did not. The resolution tests move with the code into internal/lookpath, keeping the fixture-PATH + fake-%PATHEXT% receipts that prove the Windows rules on a POSIX runner. Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughThe PR adds a shared ChangesExecutable resolution
Sequence Diagram(s)sequenceDiagram
participant commandContext
participant lookpath.Look
participant PATHAndFilesystem
commandContext->>lookpath.Look: resolve executable
lookpath.Look->>PATHAndFilesystem: inspect PATH and candidates
PATHAndFilesystem-->>lookpath.Look: return path or lookup error
lookpath.Look-->>commandContext: return core.Result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (5)
go/internal/lookpath/lookpath.go (3)
149-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment to
IsExecutable.Every other exported symbol in this file is documented.
IsExecutableis not. One line stating that it applies the platform extension list keeps the package surface consistent.🤖 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 `@go/internal/lookpath/lookpath.go` around lines 149 - 151, Add a concise Go doc comment immediately before the exported IsExecutable function, stating that it checks executability using the platform extension list returned by Extensions().
92-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalise the supplied extensions, not only
base.
HasExtensionlowersbasebut compares againstextensionsas given.ParsePathExtreturns lower-cased entries, so the internal call paths are correct. An exported caller that passes".EXE"gets no match, andCandidatesthen producesgit.EXE.EXE. Lower each extension before the comparison to make the exported contract case-insensitive on both sides.♻️ Proposed refactor
func HasExtension(base string, extensions []string) bool { lowered := core.Lower(base) for _, extension := range extensions { - if core.HasSuffix(lowered, extension) { + if core.HasSuffix(lowered, core.Lower(extension)) { return true } } return false }🤖 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 `@go/internal/lookpath/lookpath.go` around lines 92 - 100, Update HasExtension to normalize each supplied extension with core.Lower before comparing it to the already-lowered base, ensuring exported callers get case-insensitive matching on both sides and preventing duplicate extensions such as git.EXE.EXE.
104-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the operating system instead of
core.PathSeparator.
core.PathSeparatoris only available here because it is used internally bypath.Join/Join; it does not expose\\as a stable Windows-OS constant. Useruntime.GOOS == "windows"for the Windows extension policy switch, then stop.🤖 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 `@go/internal/lookpath/lookpath.go` around lines 104 - 109, Update Extensions to select the Windows extension policy using runtime.GOOS == "windows" instead of comparing core.PathSeparator, preserving nil for non-Windows systems and ParsePathExt(core.Getenv("PATHEXT")) for Windows.Source: Coding guidelines
go/program.go (1)
51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the resolver error into the
Program.Findfailure.The failure branch discards the message that
lookpath.Lookproduced.Lookdistinguishes a path-qualified target ("executable file %q not found") from a PATH search ("executable file %q not found in PATH").Program.Findreports "not found in PATH" for both, so aPaththat exists but is not runnable is described incorrectly. KeepErrProgramNotFoundas the sentinel and add the resolver detail to the message.🤖 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 `@go/program.go` around lines 51 - 54, Update the failure branch in Program.Find to include result’s resolver error/message in the core.E call while retaining ErrProgramNotFound as the sentinel. Preserve lookpath.Look’s distinction between path-qualified and PATH-search failures instead of always reporting “not found in PATH.”go/internal/lookpath/lookpath_test.go (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the tests after the symbols they now exercise.
The names carry the
TestExecInternal_prefix and the old unexported spellings, such aslookPath,isExecutable, andparsePathExt. Those symbols no longer exist. The package now exportsLook,IsExecutable,ParsePathExt,Candidates,HasExtension, andContainsSeparator. Rename toTestLook_Good,TestIsExecutable_Bad,TestParsePathExt_Ugly, and so on, so a reader can map each test to its symbol.Also applies to: 31-31, 43-43, 57-57, 63-63, 70-70, 87-87, 98-98, 106-106, 115-115, 122-122, 129-129, 148-148, 157-157, 166-166, 177-177
🤖 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 `@go/internal/lookpath/lookpath_test.go` at line 11, Rename all tests in the lookpath test file to match the exported symbols they exercise, removing the TestExecInternal_ prefix and old unexported spellings. Use names such as TestLook_Good, TestIsExecutable_Bad, TestParsePathExt_Ugly, and corresponding names for Candidates, HasExtension, and ContainsSeparator while preserving each test’s existing scenario suffix.
🤖 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 `@go/internal/lookpath/lookpath_test.go`:
- Around line 297-320: The new tests must use testify assertions consistently:
in go/internal/lookpath/lookpath_test.go lines 297-320, remove assertExtensions,
replace its call sites with require.Equal(t, want, got), convert writeFixture
setup checks to require.True, and update the remaining raw assertions throughout
the file; in go/program_test.go lines 320-343, convert
TestProgram_Find_UsesSharedResolution’s four checks to require.False,
assert.Contains, require.True, and assert.Equal respectively.
- Line 1: Correct the SPDX header spelling at the top of lookpath_test.go from
“SPDX-Licence-Identifier” to the exact registered “SPDX-License-Identifier” tag,
preserving the existing license identifier.
In `@go/os_exec_link.go`:
- Around line 24-25: Rename commandContext to reflect that it does not propagate
ctx, and update all call sites accordingly; alternatively, use an existing
context-aware *core.Cmd constructor if one is available. Do not imply
cancellation support that the current core.Cmd literal cannot provide.
---
Nitpick comments:
In `@go/internal/lookpath/lookpath_test.go`:
- Line 11: Rename all tests in the lookpath test file to match the exported
symbols they exercise, removing the TestExecInternal_ prefix and old unexported
spellings. Use names such as TestLook_Good, TestIsExecutable_Bad,
TestParsePathExt_Ugly, and corresponding names for Candidates, HasExtension, and
ContainsSeparator while preserving each test’s existing scenario suffix.
In `@go/internal/lookpath/lookpath.go`:
- Around line 149-151: Add a concise Go doc comment immediately before the
exported IsExecutable function, stating that it checks executability using the
platform extension list returned by Extensions().
- Around line 92-100: Update HasExtension to normalize each supplied extension
with core.Lower before comparing it to the already-lowered base, ensuring
exported callers get case-insensitive matching on both sides and preventing
duplicate extensions such as git.EXE.EXE.
- Around line 104-109: Update Extensions to select the Windows extension policy
using runtime.GOOS == "windows" instead of comparing core.PathSeparator,
preserving nil for non-Windows systems and ParsePathExt(core.Getenv("PATHEXT"))
for Windows.
In `@go/program.go`:
- Around line 51-54: Update the failure branch in Program.Find to include
result’s resolver error/message in the core.E call while retaining
ErrProgramNotFound as the sentinel. Preserve lookpath.Look’s distinction between
path-qualified and PATH-search failures instead of always reporting “not found
in PATH.”
🪄 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: 8b74f4bc-ec6f-4ca3-95df-e4878639d962
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go/exec/exec.gogo/exec/exec_internal_test.gogo/internal/lookpath/lookpath.gogo/internal/lookpath/lookpath_test.gogo/os_exec_link.gogo/program.gogo/program_test.go
💤 Files with no reviewable changes (1)
- go/exec/exec_internal_test.go
| @@ -0,0 +1,320 @@ | |||
| // SPDX-Licence-Identifier: EUPL-1.2 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the SPDX tag spelling.
The registered tag is SPDX-License-Identifier, with the American spelling fixed by the specification. Licence scanners match that exact string, so SPDX-Licence-Identifier leaves the file unattributed.
🐛 Proposed fix
-// SPDX-Licence-Identifier: EUPL-1.2
+// SPDX-License-Identifier: EUPL-1.2📝 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.
| // SPDX-Licence-Identifier: EUPL-1.2 | |
| // SPDX-License-Identifier: EUPL-1.2 |
🤖 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 `@go/internal/lookpath/lookpath_test.go` at line 1, Correct the SPDX header
spelling at the top of lookpath_test.go from “SPDX-Licence-Identifier” to the
exact registered “SPDX-License-Identifier” tag, preserving the existing license
identifier.
| func assertExtensions(t *testing.T, got, want []string) { | ||
| t.Helper() | ||
| if len(got) != len(want) { | ||
| t.Fatalf("got %v (%d entries), want %v (%d entries)", got, len(got), want, len(want)) | ||
| } | ||
| for i := range want { | ||
| if got[i] != want[i] { | ||
| t.Fatalf("entry %d: got %q, want %q (full: %v vs %v)", i, got[i], want[i], got, want) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // writeFixture creates a file with the given mode, failing the test if it | ||
| // cannot. WriteFile does not apply the mode to an existing file, so each | ||
| // fixture name is written once per test. | ||
| func writeFixture(t *testing.T, path string, mode core.FileMode) { | ||
| t.Helper() | ||
| if w := core.WriteFile(path, []byte("fixture"), mode); !w.OK { | ||
| t.Fatalf("write %s: %v", path, w.Error()) | ||
| } | ||
| if c := core.Chmod(path, mode); !c.OK { | ||
| t.Fatalf("chmod %s: %v", path, c.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New tests do not use testify. Both new test files assert with raw if statements and t.Fatal/t.Fatalf, and one adds a hand-written slice comparator. The coding guidelines require testify require for setup assertions and assert for checks in go/**/*_test.go.
go/internal/lookpath/lookpath_test.go#L297-L320: deleteassertExtensionsin favour ofrequire.Equal(t, want, got), and convert thewriteFixtureresult checks torequire.True. Apply the same conversion to the assertions throughout the file.go/program_test.go#L320-L343: convert the four checks inTestProgram_Find_UsesSharedResolutiontorequire.False,assert.Contains,require.True, andassert.Equal.
As per coding guidelines: "Use testifyrequirefor test setup assertions andassertfor checks in test files".
📍 Affects 2 files
go/internal/lookpath/lookpath_test.go#L297-L320(this comment)go/program_test.go#L320-L343
🤖 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 `@go/internal/lookpath/lookpath_test.go` around lines 297 - 320, The new tests
must use testify assertions consistently: in
go/internal/lookpath/lookpath_test.go lines 297-320, remove assertExtensions,
replace its call sites with require.Equal(t, want, got), convert writeFixture
setup checks to require.True, and update the remaining raw assertions throughout
the file; in go/program_test.go lines 320-343, convert
TestProgram_Find_UsesSharedResolution’s four checks to require.False,
assert.Contains, require.True, and assert.Equal respectively.
Source: Coding guidelines
| func commandContext(ctx context.Context, name string, arg ...string) *core.Cmd { | ||
| path := name | ||
| if result := lookPath(name); result.OK { | ||
| path = result.Value.(string) | ||
| } | ||
|
|
||
| cmd := &core.Cmd{ | ||
| Path: path, | ||
| Args: append([]string{name}, arg...), | ||
| } | ||
| return cmd | ||
| } | ||
|
|
||
| func lookPath(file string) core.Result { | ||
| if file == "" { | ||
| return core.Fail(core.E("lookPath", "executable file not found in PATH", nil)) | ||
| } | ||
| if core.Contains(file, string(core.PathSeparator)) { | ||
| if isExecutable(file) { | ||
| return core.Ok(file) | ||
| } | ||
| return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found", file), nil)) | ||
| } | ||
|
|
||
| for _, dir := range core.Split(core.Getenv("PATH"), string(core.PathListSeparator)) { | ||
| if dir == "" { | ||
| dir = "." | ||
| cmd := &core.Cmd{Args: append([]string{name}, arg...)} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does core expose a context-aware command constructor or cancellation fields?
rg -n --no-heading -C4 'CommandContext|type Cmd struct|Cancel\s|WaitDelay' \
"$(go env GOMODCACHE 2>/dev/null || echo ~/go/pkg/mod)" 2>/dev/null | head -40
# How do callers use commandContext and the returned handle?
rg -n --no-heading -C6 '\bcommandContext\(' goRepository: dAppCore/go-process
Length of output: 11472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== os_exec_link.go =="
cat -n go/os_exec_link.go
echo
echo "== exec.Cmd fields =="
rg -n --no-heading -C8 'type Cmd struct|ctx\s+context\.' go/exec go 2>/dev/null | sed -n '1,220p'
echo
echo "== commandContext/use sites with core.Cmd fields =="
rg -n --no-heading -C12 '\bctx\b|\.Start\(|\.Wait\(|\.Kill\(|\.Process\b|cancel\(' go os_exec_link.go go/service.go go/program.go go/exec 2>/dev/null | sed -n '1,260p'Repository: dAppCore/go-process
Length of output: 22763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod =="
cat -n go.mod
echo
echo "== dappco.re/go Cmd/Context APIs from module cache =="
MODCACHE="$(cd go && go mod download -json dappco.re/go 2>/dev/null | jq -r '.Dir // empty' || true)"
if [ -n "${MODCACHE:-}" ]; then
rg -n --no-heading -C8 'type Cmd struct|context\.(\w+|Context)\s+ctx|func .*CommandContext|func .*Command\(ctx|Cancel\s*func|WaitDelay|Run\(ctx|Start\(ctx|Wait\(ctx' "$MODCACHE" 2>/dev/null | sed -n '1,260p'
else
echo "dappco.re/go module directory not available"
fd -t f '.*\.go$' "$(go env GOMODCACHE)/dappco.re/go@*" 2>/dev/null | head -20 || true
fi
echo
echo "== local process.Cmd and Process cancellation wiring =="
cat -n go/process.go | sed -n '1,260p'
rg -n --no-heading -C6 'func \(p \*Process\) (Run|RunDir|Start|Kill|killTree|Done|IsRunning|cancel\(|KillGroup)' go/process.go go/service.go go/os_exec_link.go 2>/dev/null | sed -n '1,260p'Repository: dAppCore/go-process
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching go.mod/go.sum =="
find . -maxdepth 3 \( -name 'go.mod' -o -name 'go.sum' \) -print
echo
echo "== package files =="
find go -type f \( -path 'go/*.go' -o -path 'go/*/*.go' \) -print | sort | sed -n '1,120p'Repository: dAppCore/go-process
Length of output: 2088
Wire the context through *core.Cmd or rename commandContext.
*core.Cmd has no exported Canceller/WaitDelay/Context fields that this built literal can set, and the returned handle is not wrapped by exec.CommandContext. A dropped context leaves Service.StartWithOptions unable to cancel an already-running process via that handle; callers must terminate the process through *Process teardown instead. Rename this helper unless a context-aware command constructor exists.
🤖 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 `@go/os_exec_link.go` around lines 24 - 25, Rename commandContext to reflect
that it does not propagate ctx, and update all call sites accordingly;
alternatively, use an existing context-aware *core.Cmd constructor if one is
available. Do not imply cancellation support that the current core.Cmd literal
cannot provide.
1 participant
v0.16.2fixed executable resolution on Windows and did not fix executable resolution on Windows, because this module carried two copies of it:exec/exec.golookPathos_exec_link.golookPathProgram.Findcalls the second.os_exec_link.go:45still readreturn info.Mode()&0111 != 0— the exact defect v0.16.2's own commit message spends three paragraphs on. Every consumer resolving throughProgram.Findkept failing withProgram.Find: "git": not found in PATH, on a release that claimed the opposite.go-inference's windows lane measured it: 41 occurrences before v0.16.2, 41 after.
The fix is not a second copy of the fix
internal/lookpathnow holds one implementation and both consumers call it — because the duplication is the defect's cause, and patching in place would guarantee a third divergence.The three Windows defects it carries are unchanged from v0.16.2: no
%PATHEXT%expansion, amode&0111test the platform can never satisfy, and a path-vs-name check that missed/.commandContextno longer discards the failure eitherIt records it on
Cmd.Err, exactly asexec.Commanddoes with its own LookPath error —Startreturns it without running anything.Deferring to
Startrather than returning early is deliberate. An early return skippedService.start's exited-event broadcast, whichTestService_Actions/broadcasts_exited_event_on_start_failurecaught. The callers' failure handling is untouched.Receipts — macOS,
GOWORK=off(what CI runs)The regression pin
TestProgram_Find_UsesSharedResolutionpins the miss itself:Program.Findmust land on the same path the shared resolver reports. It fails the moment a third copy appears or the two drift apart — which a version bump could not detect, and did not.The resolution tests move with the code into
internal/lookpath, keeping the fixture-PATH + fake-%PATHEXT%receipts that prove the Windows rules on a POSIX runner.Summary by CodeRabbit
Bug Fixes
Tests