fix: moduleroot() produces a corrupt root in Go workspace mode

[StevenACoffman]

fix: moduleroot() produces a corrupt root in Go workspace mode

Summary

gta.go's moduleroot() function runs go list -m -f '{{.Dir}}' to find the
module root directory. In Go workspace mode (when a go.work file is present),
this command outputs one directory per workspace module rather than one. The
function then calls strings.TrimSpace() on the combined output and returns the
result as a single string, producing a multi-line value that is not a valid
filesystem path.

The bug is semi-benign in that it does not always prevent gta from producing
correct output, but leaves gta.roots in a corrupt state, removing
the workspace root from the set of known roots and causing the "is this the module
root?" early-exit in isIgnoredByGo to never fire.

---

Affected code

File: gta.go

// toplevel returns the root directories of the current Go workspace.
func toplevel() ([]string, error) {
    if os.Getenv("GO111MODULE") == "off" {
        return gopaths()
    }

    root, err := moduleroot()         // <-- calls moduleroot()
    if err != nil {
        return nil, err
    }
    return []string{root}, nil
}

func moduleroot() (string, error) {
    cmd := exec.Command("go", "list", "-m", "-f", "{{.Dir}}")
    b, err := cmd.CombinedOutput()
    if err != nil {
        return "", fmt.Errorf("could get not get module root: %w", err)
    }

    return strings.TrimSpace(string(b)), nil  // <-- broken in workspace mode
}

---

Root cause

go list -m -f '{{.Dir}}' behaves differently depending on context:

Context	Output
Single-module repo	One line: the module's root directory
Go workspace	One line per module listed in go.work

strings.TrimSpace strips leading and trailing whitespace but leaves internal
newlines intact. In a workspace with 19 modules, the returned string is
1,000+ characters containing 18 embedded newlines, none of which is a valid
filesystem path.

Concrete example with workspace-mono (19 modules):

/path/to/workspace-mono/modone\n
/path/to/workspace-mono/modtwo\n
/path/to/workspace-mono/modthree\n
... (16 more lines)

This gets stored as gta.roots[0]. Each later call to
isIgnoredByGo(path, gta.roots) compares the corrupt string against real
filesystem paths. The comparison always fails, and the early-exit path (which
short-circuits the recursion when the path is the module root itself) never fires.

---

Why the Bug Is Semi-Benign (Latent) Today

isIgnoredByGo has two layers:

1. Root check (early exit): if the path exactly equals a root, return false
   (not ignored). This is the layer that the corrupt root breaks.

2. Base-name check: if filepath.Base(path) starts with . or _, or equals
   "testdata", return true (ignored). The corrupt root does not affect this
   layer. It handles all real cases correctly.

Because the base-name checks do the substantive work, the corrupt root only removes
an optimization. The function produces correct results regardless.

The bug would become harmful if gta gained use of roots beyond the early-exit
in isIgnoredByGo — for example, to bound the upward walk in
deepestUnignoredDir, or to restrict which modules the dependency graph loads.
Today, neither applies.

---

Fix: Separate the Pure Computation from the I/O

workspaceroot() does two distinct things: it spawns a child process (I/O) and
parses the output into a directory path (pure computation). Keeping these separate
makes both independently testable without running a real go toolchain.

The pure core is parseGOWORK(output string) (string, bool). The I/O shell,
workspaceroot(), runs go env GOWORK, passes the output to parseGOWORK, and
returns the result. toplevel() calls workspaceroot() and falls back to
moduleroot() in the non-workspace case.

gta.go — changes

func toplevel() ([]string, error) {
	if os.Getenv("GO111MODULE") == "off" {
		return gopaths()
	}

	// In workspace mode, go list -m -f '{{.Dir}}' outputs one line per workspace
	// module, producing a corrupt multi-line string when TrimSpace'd. Use
	// go env GOWORK instead, which always returns a single path.
	root, ok, err := workspaceroot()
	if err != nil {
		return nil, err
	}
	if ok {
		return []string{root}, nil
	}

	root, err = moduleroot()
	if err != nil {
		return nil, err
	}
	return []string{root}, nil
}

// workspaceroot returns the directory containing the active go.work file and
// true when Go workspace mode is active.
func workspaceroot() (string, bool, error) {
	cmd := exec.Command("go", "env", "GOWORK")
	b, err := cmd.CombinedOutput()
	if err != nil {
		return "", false, fmt.Errorf("go env GOWORK: %w", err)
	}
	dir, ok := parseGOWORK(string(b))
	return dir, ok, nil
}

// parseGOWORK interprets the raw output of `go env GOWORK`.
// "off" indicates workspace mode was explicitly disabled via GOWORK=off.
func parseGOWORK(output string) (string, bool) {
	gowork := strings.TrimSpace(output)
	if gowork == "" || gowork == "off" {
		return "", false
	}
	return filepath.Dir(gowork), true
}

No changes to moduleroot() are needed.

options.go — add SetRoots

The existing test suite uses SetDiffer, SetPackager, and SetPrefixes to
inject behavior via Option functions. Adding SetRoots follows the same pattern
and lets tests (and callers) inject a root directly without going through
toplevel().

// SetRoots sets the root directories for the GTA. When provided, toplevel() is
// not called and the supplied roots are used directly.
func SetRoots(roots ...string) Option {
	return func(g *GTA) error {
		g.roots = roots
		return nil
	}
}

This requires a small guard in New() where toplevel() is called.
We should only call it when g.roots is still nil after applying all options.

Test plan

Unit tests for parseGOWORK (pure function, no I/O)

Because parseGOWORK has no I/O, table-driven tests cover it exhaustively in
parallel:

func TestParseGOWORK(t *testing.T) {
	t.Parallel()

	cases := map[string]struct {
		input   string
		wantDir string
		wantOK  bool
	}{
		"active workspace": {
			input:   "/home/user/project/go.work\n",
			wantDir: "/home/user/project",
			wantOK:  true,
		},
		"explicitly disabled": {
			input:   "off\n",
			wantDir: "",
			wantOK:  false,
		},
		"empty output": {
			input:   "",
			wantDir: "",
			wantOK:  false,
		},
		"whitespace only": {
			input:   "   \n",
			wantDir: "",
			wantOK:  false,
		},
		"workspace at filesystem root": {
			input:   "/go.work\n",
			wantDir: "/",
			wantOK:  true,
		},
	}

	for name, tc := range cases {
		t.Run(name, func(t *testing.T) {
			t.Parallel()
			gotDir, gotOK := parseGOWORK(tc.input)
			if gotDir != tc.wantDir || gotOK != tc.wantOK {
				t.Errorf("parseGOWORK(%q) = (%q, %v), want (%q, %v)",
					tc.input, gotDir, gotOK, tc.wantDir, tc.wantOK)
			}
		})
	}
}

Integration tests for toplevel() via SetRoots

toplevel() calls exec.Command("go", "env", "GOWORK"), which requires a real Go toolchain and reads the actual environment. Do not use t.SetEnv here — it turns off t.Parallel(). Instead, use t.Setenv, which is safe for parallel tests in Go 1.17+, or bypass toplevel() altogether by testing through New()
with SetRoots.

The SetRoots option provides a clean seam for testing the downstream effects of
root injection without spawning child processes:

func TestSetRootsOption(t *testing.T) {
	t.Parallel()

	root := "/fake/workspace/root"
	gta, err := New(
		SetRoots(root),
		SetDiffer(noopDiffer{}),
		SetPackager(noopPackager{}),
	)
	if err != nil {
		t.Fatalf("New() error: %v", err)
	}
	if len(gta.roots) != 1 || gta.roots[0] != root {
		t.Errorf("roots = %v, want [%q]", gta.roots, root)
	}
}

For workspaceroot(), combine all cases into one table-driven test. The three
cases (active workspace, GOWORK=off, no workspace) map cleanly to a table.
t.TempDir() at the parent level provides a real path for the active case
without requiring a real file on disk:

func TestWorkspaceroot(t *testing.T) {
	tmpDir := t.TempDir()

	cases := map[string]struct {
		gowork  string
		wantDir string
		wantOK  bool
	}{
		"active workspace": {
			gowork:  filepath.Join(tmpDir, "go.work"),
			wantDir: tmpDir,
			wantOK:  true,
		},
		"disabled":     {gowork: "off"},
		"no workspace": {gowork: ""},
	}

	for name, tc := range cases {
		t.Run(name, func(t *testing.T) {
			t.Setenv("GOWORK", tc.gowork)
			gotDir, gotOK, err := workspaceroot()
			if err != nil {
				t.Fatalf("workspaceroot() error: %v", err)
			}
			if gotDir != tc.wantDir || gotOK != tc.wantOK {
				t.Errorf("workspaceroot() = (%q, %v), want (%q, %v)",
					gotDir, gotOK, tc.wantDir, tc.wantOK)
			}
		})
	}
}

Note: t.Setenv (not t.SetEnv) is used here. It restores the original value after the test completes and is safe in parallel test cases because it does not turn off t.Parallel() at the parent level.

Test helper discipline

Any helper function shared by more than one test case must call t.Helper() as its first statement so that failure output points to the call site, not the helper:

func assertRoots(t *testing.T, g *GTA, want []string) {
	t.Helper()
	if !reflect.DeepEqual(g.roots, want) {
		t.Errorf("roots = %v, want %v", g.roots, want)
	}
}

[StevenACoffman]

@anitgandhi if you get a chance, I would appreciate it if you could take a look

[anitgandhi]

approach makes sense, thanks for submitting the fix!