diff --git a/internal/agents/codex/adapter.go b/internal/agents/codex/adapter.go index 8f636465c..1c3b77fd6 100644 --- a/internal/agents/codex/adapter.go +++ b/internal/agents/codex/adapter.go @@ -13,6 +13,7 @@ package codex import ( + "context" "fmt" "io" "os" @@ -21,10 +22,12 @@ import ( "reflect" "slices" "strings" + "time" "github.com/zzet/gortex/internal/agents" "github.com/zzet/gortex/internal/agents/internalutil" "github.com/zzet/gortex/internal/mcp" + "github.com/zzet/gortex/internal/platform" "github.com/zzet/gortex/internal/version" ) @@ -400,13 +403,27 @@ func codexStartupTimeoutAtLeast(value any, minimum int) bool { } } +// codexVersionProbeTimeout bounds the `codex --version` probe. A version +// banner is instant; anything slower is a wedged binary, and without a +// deadline the probe blocks the install indefinitely and leaks a child +// process that never exits. Failing the probe is cheap — the caller reads +// an error as "unknown install" and keeps direct tool exposure on. +// Overridable so the timeout test does not have to sleep for the real one. +var codexVersionProbeTimeout = 5 * time.Second + // codexVersionOutput is a seam for hermetic adapter tests. var codexVersionOutput = func() ([]byte, error) { path, err := exec.LookPath("codex") if err != nil { return nil, err } - return exec.Command(path, "--version").Output() + ctx, cancel := context.WithTimeout(context.Background(), codexVersionProbeTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, path, "--version") + // The probe also runs from surfaces with no console of their own; without + // this, Windows hands the child a fresh console window. + platform.ConfigureBackgroundCommand(cmd) + return cmd.Output() } func codexSupportsDirectToolNamespaces() (supported bool, detectedVersion string) { diff --git a/internal/agents/codex/adapter_version_probe_test.go b/internal/agents/codex/adapter_version_probe_test.go new file mode 100644 index 000000000..a5257a403 --- /dev/null +++ b/internal/agents/codex/adapter_version_probe_test.go @@ -0,0 +1,63 @@ +package codex + +import ( + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +// TestCodexVersionOutput_TimesOut pins the deadline on the `codex --version` +// probe. A wedged binary previously blocked the caller forever and left a +// child process behind that never exited; the probe must give up and report +// an error instead. +func TestCodexVersionOutput_TimesOut(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake binary is a POSIX shell script") + } + + dir := t.TempDir() + fake := filepath.Join(dir, "codex") + if err := os.WriteFile(fake, []byte("#!/bin/sh\nsleep 60\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + + original := codexVersionProbeTimeout + codexVersionProbeTimeout = 100 * time.Millisecond + t.Cleanup(func() { codexVersionProbeTimeout = original }) + + done := make(chan error, 1) + go func() { + _, err := codexVersionOutput() + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("hung probe returned success; want a timeout error") + } + case <-time.After(10 * time.Second): + t.Fatal("probe did not honour its deadline") + } +} + +// TestCodexSupportsDirectToolNamespaces_TreatsProbeFailureAsCurrent documents +// the posture the timeout relies on: a probe that fails for any reason — +// including the new deadline — leaves direct tool exposure enabled rather +// than silently downgrading the install. +func TestCodexSupportsDirectToolNamespaces_TreatsProbeFailureAsCurrent(t *testing.T) { + original := codexVersionOutput + codexVersionOutput = func() ([]byte, error) { return nil, os.ErrDeadlineExceeded } + t.Cleanup(func() { codexVersionOutput = original }) + + supported, detected := codexSupportsDirectToolNamespaces() + if !supported { + t.Error("probe failure disabled direct tool namespaces") + } + if detected != "" { + t.Errorf("detected version = %q, want empty on probe failure", detected) + } +} diff --git a/internal/churn/churn.go b/internal/churn/churn.go index 2abf56e11..a0b74bd19 100644 --- a/internal/churn/churn.go +++ b/internal/churn/churn.go @@ -31,7 +31,7 @@ import ( "bytes" "context" "fmt" - "os/exec" + "os" "path/filepath" "strconv" "strings" @@ -378,9 +378,6 @@ func stripRepoPrefix(filePath, repoRoot string) string { if !strings.Contains(filePath, "/") { return filePath } - if _, err := exec.LookPath("git"); err != nil { - return filePath - } abs := filepath.Join(repoRoot, filePath) if fileExists(abs) { return filePath @@ -394,9 +391,16 @@ func stripRepoPrefix(filePath, repoRoot string) string { return filePath } +// fileExists is split out so tests can stub it. os.Stat follows symlinks, +// and Mode().IsRegular matches the previous `test -f` semantics without +// spawning a process for every lookup. Shelling out here cost one child +// process per indexed file on every enrichment pass, and was outright +// wrong on Windows, where there is no `test` executable: every lookup +// failed, so multi-repo paths were never stripped and the enricher +// silently produced no churn data. var fileExists = func(path string) bool { - cmd := exec.Command("test", "-f", path) - return cmd.Run() == nil + info, err := os.Stat(path) + return err == nil && info.Mode().IsRegular() } // runGit shells out and returns trimmed stdout, or "" on error. Used diff --git a/internal/churn/churn_test.go b/internal/churn/churn_test.go index 6accacbf9..21b6c1619 100644 --- a/internal/churn/churn_test.go +++ b/internal/churn/churn_test.go @@ -203,3 +203,61 @@ func currentBranch(t *testing.T, dir string) string { } return strings.TrimSpace(string(out)) } + +// TestStripRepoPrefix_NeedsNoExternalBinaries pins the fix for the +// subprocess fan-out: resolving whether a path exists is a filesystem +// question, and answering it must not depend on anything being on PATH. +// The previous implementation gated on `exec.LookPath("git")` and then +// shelled out to `test -f` once per indexed file, so on a host where +// neither resolves — every Windows box, since there is no `test` +// executable — the prefix was never stripped and the enricher silently +// produced no churn data. +func TestStripRepoPrefix_NeedsNoExternalBinaries(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "internal"), 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(root, "internal", "foo.go") + if err := os.WriteFile(target, []byte("package internal\n"), 0o600); err != nil { + t.Fatal(err) + } + + // Nothing is resolvable on PATH — the old implementation bailed here. + t.Setenv("PATH", "") + + if got := stripRepoPrefix("myrepo/internal/foo.go", root); got != "internal/foo.go" { + t.Errorf("prefixed path: got %q, want %q", got, "internal/foo.go") + } + if got := stripRepoPrefix("internal/foo.go", root); got != "internal/foo.go" { + t.Errorf("repo-relative path: got %q, want %q", got, "internal/foo.go") + } + // A path that resolves under neither spelling is returned untouched. + if got := stripRepoPrefix("myrepo/internal/absent.go", root); got != "myrepo/internal/absent.go" { + t.Errorf("unresolvable path: got %q, want it unchanged", got) + } +} + +// TestFileExists_MatchesTestF keeps the os.Stat replacement honest about +// the `test -f` semantics it replaced: regular files only, symlinks +// followed, directories and missing paths rejected. +func TestFileExists_MatchesTestF(t *testing.T) { + dir := t.TempDir() + regular := filepath.Join(dir, "regular") + if err := os.WriteFile(regular, []byte("data"), 0o600); err != nil { + t.Fatal(err) + } + if !fileExists(regular) { + t.Error("regular file reported missing") + } + if fileExists(dir) { + t.Error("directory reported as a regular file") + } + if fileExists(filepath.Join(dir, "missing")) { + t.Error("missing path reported as a regular file") + } + + link := filepath.Join(dir, "regular-link") + if err := os.Symlink(regular, link); err == nil && !fileExists(link) { + t.Error("symlink to a regular file reported missing") + } +}