diff --git a/README.md b/README.md index 2015843..809f24b 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ git diff --name-only origin/main..HEAD | cascade --changed-files=- | `--base` | (required unless `--changed-files`) | Base git ref (e.g. `origin/main`); cascade runs `git diff --name-only ..` to derive the change-set. | | `--head` | `HEAD` | Head git ref. | | `--changed-files` | (none) | Path to a file with one change-set entry per line. `-` reads from stdin. When set, `--base` is not required and `git diff` is not invoked. | -| `--root` | `.` | Working directory for `go list` and module-root for `changeset.Resolve`. | +| `--root` | `.` | Working directory for `go list` and module-root for `changeset.Resolve`. Absolute or relative; cascade absolutizes via `filepath.Abs` before use, so the default `.` resolves to the process cwd. | | `--version` | false | Print `cascade (build @, )` and exit. (Branch is empty for `go install` builds — the module proxy doesn't carry branch metadata.) | | `--help` | false | Print usage and exit. Routes to stdout per GNU convention; flag-parse errors route help to stderr per stdlib `flag` default. | diff --git a/docs/dev/0013-bug12-fix-relative-root.md b/docs/dev/0013-bug12-fix-relative-root.md new file mode 100644 index 0000000..4f4ba14 --- /dev/null +++ b/docs/dev/0013-bug12-fix-relative-root.md @@ -0,0 +1,132 @@ +# Bug Fix Retrospective: #12 — silent-empty output when `--root` is relative + +**Status:** closing report; awaiting CDC verification. +**Closing commit:** _TBD_ (head of `fix/bug12-relative-root` at PR open). +**Adjacent commits:** the bug-fix work lands in two commits — implementation+tests, and docs+retro. +**CDC verification:** pending. +**Source:** GitHub issue [#12](https://github.com/geomyidia/cascade/issues/12) (filed by maintainer 2026-05-07). +**Source plan:** [`docs/dev/0011-implementation-plan-m5-cli-main-wiring.md`](./0011-implementation-plan-m5-cli-main-wiring.md) — wait, no — plan-mode plan at `~/.claude-banyan/plans/please-brush-up-on-joyful-pearl.md` (the plan was approved via ExitPlanMode prior to implementation; not committed to the repo per the convention for transient bug-fix planning). +**Methodology:** [`assets/ai/LEDGER_DISCIPLINE.md`](../../assets/ai/LEDGER_DISCIPLINE.md), [`assets/ai/AI-ENGINEERING-METHODOLOGY.md`](../../assets/ai/AI-ENGINEERING-METHODOLOGY.md). +**Target version:** v0.1.1 (patch release post-merge). +**Verification target:** [getBanyan/api PR #3474](https://github.com/getBanyan/api/pull/3474) — the cascade-fix verification PR that should green once v0.1.1 ships. + +## Bug summary + +cascade v0.1.0's documented default invocation — `cascade --base=origin/main --head=HEAD` (no `--root`, taking the documented default of `.`) — silently emitted an empty affected-package list and exited 0 against any real Go monorepo. **The exact "silent-zero-packages" failure mode that drove the gta → cascade pivot.** + +Root cause: the CLI passed the literal string `"."` through `changeset.WithModuleRoot`. Inside `changeset.Resolve`, `filepath.Join(".", "rel/path")` returned a relative path; `filepath.Dir` of that yielded a relative parent; the `dirMap` (keyed on absolute `golist.Package.Dir` values) missed every lookup; `RevDepClosure([])` returned nil; cascade exited 0 with empty stdout. + +The bug lived in a parameter-space cell that no test happened to hit: relative moduleRoot × absolute pkg.Dir keys. Both M4 and M5 had 100% statement coverage but the pairing was untested in either layer. **Coverage didn't catch it because coverage measures statement reachability, not behavioral coverage across the parameter space.** + +## What changed + +**Five deliverables, all in this PR.** + +### 1. Library fix (`pkg/changeset/changeset.go`) + +```go +// After the moduleRootSet check, before dirMap-build: +if abs, err := filepath.Abs(cfg.moduleRoot); err == nil { + cfg.moduleRoot = abs +} +``` + +Defensive at the library level — protects ANY current/future caller (including the deferred `pkg/cascade.Run`) from the bug class. `filepath.Abs("")` and `filepath.Abs(".")` both resolve to the cwd, so this also handles the empty-string case gracefully. + +### 2. CLI fix (`internal/cli/cli.go`) + +```go +// After parseFlags returns successfully, before validateConfig: +if abs, err := filepath.Abs(cfg.root); err == nil { + cfg.root = abs +} +``` + +Surgical fix at the CLI layer: the four downstream consumers (`runGitDiff`, `classifyGitDiffError`, `golist.WithDir`, `changeset.WithModuleRoot`) all receive a path absolutized exactly once. Defense in depth alongside the library fix. + +### 3. Diagnostic warning (`internal/cli/cli.go`) + +```go +// After runPipeline returns: +if len(affected) == 0 { + nGoFiles := 0 + for _, f := range changedFiles { + if strings.HasSuffix(f, ".go") { + nGoFiles++ + } + } + if nGoFiles > 0 { + fmt.Fprintf(stderr, "cascade: %d changed Go file(s) did not resolve to any package; check --root\n", nGoFiles) + } +} +``` + +Surfaces the suspicious-zero-result case loudly. Filters on `.go`-suffix so docs-only PRs (legitimate empty case) stay silent. Doesn't change exit code; just adds a stderr breadcrumb. The issue author's lean: *"would have caught this in seconds during the Banyan integration."* + +### 4. Regression tests (4 new tests, three layers) + +- **`pkg/changeset/changeset_test.go`** → `TestResolve_RelativeModuleRoot_AbsolutizedInternally` (2 subtests: `"."` and `""`). Pre-fix this test fails with empty result; post-fix it passes. Library-layer regression coverage. +- **`internal/cli/seam_test.go`** → `TestRun_DefaultRootResolvesCWD`. End-to-end CLI invocation with no `--root` flag (default `"."`) + relative changedFiles + cwd-rooted absolute pkg.Dir values. Pre-fix produced the empty-output bug; post-fix produces the expected affected-set on stdout with no diagnostic. +- **`internal/cli/seam_test.go`** → `TestRun_DiagnosticOnSuspiciousEmpty`. Drives a pipeline where the changedFile path doesn't match any pkg.Dir; asserts the stderr diagnostic line fires with the correct count. +- **`internal/cli/seam_test.go`** → `TestRun_NoDiagnosticOnDocsOnlyEmpty`. Asserts the diagnostic does NOT fire on docs-only changesets (no `.go` files). Pins the `.go`-suffix filter behaviour. + +All five cascade packages remain at 100% coverage post-fix; no test contortions needed. + +### 5. Documentation updates + +- **`pkg/changeset/changeset.go`** — `WithModuleRoot` godoc: now explicitly documents that the supplied value is absolutized via `filepath.Abs`, with explicit reference to bug #12. Test-purity story preserved (passing an absolute path is still a no-op for absolutize). +- **`README.md`** — `--root` flag row gains: *"Absolute or relative; cascade absolutizes via `filepath.Abs` before use, so the default `.` resolves to the process cwd."* + +## Verification + +**Pre-merge** (in this PR): +- `make check-all` green: build + lint (cold cache via OUT-1) + race tests + per-package coverage gate at 100% on all 5 cascade packages (golist, depgraph, changeset, internal/project, internal/cli). +- Local smoke against the cascade repo itself reproducing the issue's exact failure case: + ``` + $ ./bin/cascade-fixed (built from this branch) + $ echo "pkg/golist/golist.go" | ./bin/cascade-fixed --changed-files=- + github.com/geomyidia/cascade/cmd/cascade + github.com/geomyidia/cascade/internal/cli + github.com/geomyidia/cascade/pkg/changeset + github.com/geomyidia/cascade/pkg/depgraph + github.com/geomyidia/cascade/pkg/golist + ``` + Pre-fix (v0.1.0): empty output. Post-fix: 5-package affected-set, sorted lexicographically. **The bug is closed.** + +**Post-merge** (closes after v0.1.1 tag + GH release): +- `go install github.com/geomyidia/cascade/cmd/cascade@v0.1.1` against `proxy.golang.org` — confirm tag indexed cleanly (M5 F-18 lineage). +- **getBanyan/api PR #3474** CI greens up. This is the load-bearing real-codebase closure for the bug — same shape as M2 F-18 and M5 F-19 (real-codebase verification proves the fix works at production scale). + +## What Worked + +**The five-seam pattern's testability paid off again.** All four new tests use existing seams (`runGitDiff`, `runGoListWrapper`, `signalContext`) to drive the bug + diagnostic scenarios in-process, no real subprocess invocations. The bug's cell-in-the-test-matrix (relative moduleRoot × absolute pkg.Dir) is now explicitly exercised at both the library layer (where the bug literally lived) and the CLI integration layer (where the user-facing surface is). M5's seam-pattern claim — *"established convention; future packages with io edges should reach for it by default"* — generalised cleanly to bug-regression coverage too: if a future change re-introduces the bug class at either layer, the layer-specific test fails first. + +**Defense-in-depth at both layers (Q1 user decision) was the right call.** The CLI fix is surgical and immediate; the library fix protects future consumers (the deferred `pkg/cascade.Run`, third-party code importing `pkg/changeset` directly). If only the CLI was fixed, a future library consumer would re-discover the bug. If only the library was fixed, the CLI's `cfg.root` flowing through `runGitDiff` etc. would still be relative — not buggy in those callers, but inconsistent with the absolutization-once-at-the-edge story. + +**The diagnostic stderr line (Q2 user decision) is the right kind of "loudness."** Doesn't change exit code; doesn't fire on legitimate-empty cases (docs-only PRs); fires loudly on suspicious-empty cases. **The bug was discovered exactly the way the diagnostic would have surfaced it: a real-world user noticed the empty output didn't match expectations.** With this diagnostic in place, future cascade users catch this bug class in their first real-world run, not after digging through CI logs. + +**Plan-mode `AskUserQuestion` for the two material decisions was load-bearing.** Both Q1 (defense-in-depth scope) and Q2 (diagnostic shape) had multiple defensible answers. Asking surfaced the user's preference (both at the comprehensive end) rather than me applying a default and getting it slightly wrong. Same pattern that worked for M4's Q1+Q2 and M5's plan-mode session. + +**100% coverage maintained without contortion.** The new `if abs, err := filepath.Abs(...); err == nil` branches in both layers don't introduce a separately-coverable error path because `filepath.Abs` only errors when `os.Getwd` fails — a system-level event that's fundamentally unreliable to test in-process. The `err == nil` branch is the happy path; the implicit "leave as-is on error" branch is structurally dead in any test environment where `os.Getwd` works (which is all of them). Coverage still reads 100% because the error branch's "do nothing" body has no statements to count. + +## What didn't work (worth carrying forward) + +**Coverage discipline didn't catch a behavioral-space gap.** M5's retro claimed *"100% coverage on first try, with no test contortions"* and that's true for line coverage. But coverage measured statement reachability, not behavioral coverage across the parameter space. The bug lived in a cell of the parameter space (relative moduleRoot × absolute pkg.Dir) that no test happened to hit, even though every line in the production code was reached. + +**Carry-forward into M7+ test design:** when designing tests for new features (especially library APIs that take typed-string arguments like paths, refs, or import patterns), explicitly enumerate the parameter-space matrix rather than assuming line coverage implies behavioral coverage. For path-handling code specifically: relative-vs-absolute × empty-vs-non-empty × symlink-vs-not × OS-native-separator-vs-other. For each cell, document either a test row OR a deliberate "out of scope" decision. The point isn't to write 16x as many tests; it's to think the matrix through explicitly so the unexamined cell becomes a documented decision rather than a silent gap. + +**The CC self-check protocol's track record (5-for-5 zero softpedals from M3/refactor/M4/M5/M6) does NOT extend to behavioral-space gaps.** The audit catches textual softpedals (evidence-vs-criterion mismatches in the row walk). It doesn't catch "this row is structurally `done` but the assertion space is incomplete." Worth recording as a known limitation: the audit is necessary, not sufficient. The mature-field discipline this approximates (aviation 14 CFR 121.563, NRC inspector pattern) catches the kind of drift the audit is good at; behavioral-space gaps are the M5-retro-style "first stress test against integration-shaped scope" failure mode that needs different protection. + +**The Makefile's `release-dry-run` target's go.sum bug surfaced again** when M6 tagged v0.1.0 — wasn't fixed in M6 because release work didn't include Makefile changes. That bug is still open; same workaround applies for v0.1.1 (manual `git tag` + `git push origin v0.1.1` skipping the Makefile target). + +## Open items / carry-forward + +- **v0.1.1 release** — tag + push + GH release after this PR merges. Standard M6 flow (manual tag + push to bypass the Makefile's go.sum bug). Release notes should call out: (a) bug #12 fix, (b) reproduction case, (c) verification PR (getBanyan/api #3474), (d) recommendation that v0.1.0 users upgrade. +- **Real-world verification** — re-trigger getBanyan/api PR #3474's CI against the v0.1.1 binary. **This is the load-bearing closure for bug #12.** Synthetic tests pass; production-scale evidence is the structural proof. +- **Should v0.1.0 be `retract`-ed?** Plan deferred to data-driven decision. If real-world consumers report the bug beyond #12, add `retract v0.1.0` to go.mod in v0.2. If only the maintainer hit it, leave v0.1.0 available with the v0.1.1 release-notes recommending upgrade. +- **Methodology note on coverage-vs-behavior gap.** Documented above; worth surfacing in any future M-spec's "test strategy" section so milestone leads explicitly enumerate the parameter-space matrix. +- **The Makefile's `release-dry-run` go.sum bug.** Not blocking v0.1.1 (manual workaround works) but should be fixed before the next release flow. One-line fix: `git diff --quiet -- go.mod && { ! test -f go.sum || git diff --quiet -- go.sum; }`. Could ride alongside any v0.2 release-prep work. + +## Closure + +All five deliverables landed; bug #12's reproduction case verified-fixed locally; all five cascade packages at 100% coverage; `make check-all` green. Awaiting CDC verification. Real-world-codebase closure pending v0.1.1 release + getBanyan/api PR #3474 re-trigger. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 6a7393c..f1c83c1 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -9,6 +9,7 @@ import ( "io" "os" "os/signal" + "path/filepath" "strings" "syscall" @@ -142,6 +143,18 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return exitSuccess } + // Absolutize cfg.root once after flag parse so the four downstream + // consumers (runGitDiff, classifyGitDiffError, golist.WithDir, + // changeset.WithModuleRoot) all receive a path resolved against the + // process cwd exactly once. filepath.Abs(".") and filepath.Abs("") + // both resolve to the cwd. On error, leave as-is and let the + // library-layer absolutization in changeset.Resolve catch it. + // Closes bug #12 at the CLI layer (defense in depth alongside the + // library-layer fix). + if abs, err := filepath.Abs(cfg.root); err == nil { + cfg.root = abs + } + if err := validateConfig(cfg); err != nil { fmt.Fprintln(stderr, "cascade:", err) return mapError(err) @@ -159,6 +172,27 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return mapError(err) } + // Surface a diagnostic when changedFiles contained Go files but the + // affected-set is empty — that's a suspicious zero-result (any .go + // file change should produce at least the seed package itself). + // Filter on .go suffix so docs-only PRs (legitimately empty) stay + // silent. Doesn't change exit code; just adds a stderr breadcrumb. + // Bug #12's silent-empty case would have surfaced in seconds with + // this check. + if len(affected) == 0 { + nGoFiles := 0 + for _, f := range changedFiles { + if strings.HasSuffix(f, ".go") { + nGoFiles++ + } + } + if nGoFiles > 0 { + fmt.Fprintf(stderr, + "cascade: %d changed Go file(s) did not resolve to any package; check --root\n", + nGoFiles) + } + } + for _, path := range affected { fmt.Fprintln(stdout, path) } diff --git a/internal/cli/seam_test.go b/internal/cli/seam_test.go index a8aad90..15a0799 100644 --- a/internal/cli/seam_test.go +++ b/internal/cli/seam_test.go @@ -7,6 +7,7 @@ import ( "io" "os" "os/exec" + "path/filepath" "sort" "strings" "testing" @@ -397,3 +398,130 @@ func (r *errReader) Read(_ []byte) (int, error) { return 0, r.err } // Compile-time assertion that errReader implements io.Reader. var _ io.Reader = (*errReader)(nil) + +// TestRun_DefaultRootResolvesCWD is the bug-#12 regression test at the CLI +// integration layer. Pre-fix, invoking cli.Run with no --root flag (default +// "."), a relative changedFile entry, and synthetic golist packages with +// absolute Dir values produced empty stdout (with a stderr diagnostic post- +// fix; pre-fix it was silent-empty). Post-fix, cfg.root is absolutized via +// filepath.Abs(".") to the test process's actual cwd; the synthetic Dir +// values constructed from os.Getwd() then match in changeset.Resolve's +// dirMap, and the affected packages land on stdout. +// +// This test pairs with TestResolve_RelativeModuleRoot_AbsolutizedInternally +// in pkg/changeset (library-layer); both layers are exercised so a +// regression in either fix surfaces in the failing test for that layer. +func TestRun_DefaultRootResolvesCWD(t *testing.T) { + installQuietSignalContext(t) + + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd: %v", err) + } + + withGitDiffSeam(t, func(_ context.Context, _, _, _ string) gitDiffResult { + return gitDiffResult{stdout: strings.NewReader("pkga/a.go\n"), err: nil} + }) + withGoListSeam(t, func(_ context.Context, _, _ []string, _ ...golist.Option) ([]golist.Package, error) { + // Dir values rooted at the test process's cwd so absolutize-of-"." + // resolves to the same prefix and the parent-dir lookup succeeds. + return []golist.Package{ + {ImportPath: "ex/pkga", Dir: filepath.Join(cwd, "pkga")}, + }, nil + }) + + var stdout, stderr bytes.Buffer + // No --root flag: takes the default ".". + exit := Run( + []string{"--base=origin/main", "--head=HEAD"}, + strings.NewReader(""), &stdout, &stderr, + ) + if exit != 0 { + t.Fatalf("exit = %d, want 0; stderr: %q", exit, stderr.String()) + } + wantStdout := "ex/pkga\n" + if stdout.String() != wantStdout { + t.Errorf("stdout = %q, want %q (bug #12 regression — empty output indicates relative-root absolutize broken)", + stdout.String(), wantStdout) + } + // No diagnostic stderr should fire (affected is non-empty). + if strings.Contains(stderr.String(), "did not resolve") { + t.Errorf("unexpected diagnostic stderr on successful run: %q", stderr.String()) + } +} + +// TestRun_DiagnosticOnSuspiciousEmpty verifies the bug-#12 mitigation: when +// changedFiles contains .go files but the affected-set is empty, cli.Run +// emits a stderr breadcrumb to surface the suspicious zero-result. Drives a +// pipeline where the seams return packages but the changedFile path doesn't +// match any package's Dir, so changeset.Resolve yields empty seeds → empty +// affected set → diagnostic fires. +func TestRun_DiagnosticOnSuspiciousEmpty(t *testing.T) { + installQuietSignalContext(t) + withGitDiffSeam(t, func(_ context.Context, _, _, _ string) gitDiffResult { + // One .go file that won't resolve to any pkg.Dir (orphan path). + return gitDiffResult{stdout: strings.NewReader("orphan/path/file.go\n"), err: nil} + }) + withGoListSeam(t, func(_ context.Context, _, _ []string, _ ...golist.Option) ([]golist.Package, error) { + // Pkg's Dir doesn't match the changed file's parent → no seeds. + return []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/m/pkga"}, + }, nil + }) + + var stdout, stderr bytes.Buffer + exit := Run( + []string{"--base=origin/main", "--head=HEAD", "--root=/m"}, + strings.NewReader(""), &stdout, &stderr, + ) + if exit != 0 { + t.Errorf("exit = %d, want 0 (suspicious-empty doesn't change exit code); stderr: %q", + exit, stderr.String()) + } + if stdout.Len() != 0 { + t.Errorf("stdout should be empty; got %q", stdout.String()) + } + wantStderr := "did not resolve to any package" + if !strings.Contains(stderr.String(), wantStderr) { + t.Errorf("stderr should contain %q (diagnostic warning); got %q", + wantStderr, stderr.String()) + } + // Verify the count is correct (1 Go file in changedFiles). + if !strings.Contains(stderr.String(), "1 changed Go file") { + t.Errorf("stderr should mention count=1; got %q", stderr.String()) + } +} + +// TestRun_NoDiagnosticOnDocsOnlyEmpty verifies the diagnostic does NOT fire +// when changedFiles has no .go files — that's a legitimate empty case (a +// docs-only PR), not the suspicious-zero-result case bug #12 represents. +// The .go-suffix filter is the one piece of state that distinguishes the +// two; this test pins that distinction. +func TestRun_NoDiagnosticOnDocsOnlyEmpty(t *testing.T) { + installQuietSignalContext(t) + withGitDiffSeam(t, func(_ context.Context, _, _, _ string) gitDiffResult { + // Only non-.go files — legitimate "no Go files changed" case. + return gitDiffResult{stdout: strings.NewReader("README.md\nCHANGELOG\n"), err: nil} + }) + withGoListSeam(t, func(_ context.Context, _, _ []string, _ ...golist.Option) ([]golist.Package, error) { + return []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/m/pkga"}, + }, nil + }) + + var stdout, stderr bytes.Buffer + exit := Run( + []string{"--base=origin/main", "--head=HEAD", "--root=/m"}, + strings.NewReader(""), &stdout, &stderr, + ) + if exit != 0 { + t.Errorf("exit = %d, want 0; stderr: %q", exit, stderr.String()) + } + if stdout.Len() != 0 { + t.Errorf("stdout should be empty (no Go files changed); got %q", stdout.String()) + } + if strings.Contains(stderr.String(), "did not resolve") { + t.Errorf("diagnostic should NOT fire for docs-only empty case; stderr: %q", + stderr.String()) + } +} diff --git a/pkg/changeset/changeset.go b/pkg/changeset/changeset.go index 9e93045..eb0aa06 100644 --- a/pkg/changeset/changeset.go +++ b/pkg/changeset/changeset.go @@ -21,17 +21,22 @@ type config struct { } // WithModuleRoot sets the module root used to resolve relative entries in -// changedFiles. When supplied, Resolve uses dir directly without consulting -// the filesystem. +// changedFiles. Both absolute and relative paths are accepted; Resolve calls +// filepath.Abs on the supplied value before use, so a relative path is +// resolved against the process cwd at call time. (Closes bug #12: a literal +// "." or any other relative path now resolves correctly rather than silently +// failing the absolute-keyed dirMap lookup.) // // If WithModuleRoot is not supplied, Resolve falls back to os.Getwd at call -// time. Tests should pass WithModuleRoot explicitly so test outcomes don't -// depend on the working directory and the io is bypassed. +// time. Tests should pass WithModuleRoot with an absolute path so test +// outcomes don't depend on the working directory and the io is bypassed. // -// An empty argument is treated as "explicitly set to empty," distinct from -// "not set" (the latter triggers the os.Getwd fallback). With an -// explicitly-empty moduleRoot, relative entries in changedFiles cannot be -// resolved to absolute paths and silently fail to match any package. +// An empty argument and "." both resolve to the cwd at call time (because +// filepath.Abs("") and filepath.Abs(".") are equivalent). The distinction +// between "explicitly empty" and "not set" remains in the implementation +// (moduleRootSet flag) so tests that need to drive the os.Getwd fallback +// branch can still do so by omitting WithModuleRoot, but the user-visible +// behaviour of "." vs "" is now identical and correct. func WithModuleRoot(dir string) Option { return func(c *config) { c.moduleRoot = dir @@ -104,6 +109,17 @@ func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []str } } + // Absolutize moduleRoot so the parent-dir comparison against pkg.Dir + // (which golist documents as absolute) succeeds even when the caller + // supplies a relative path. filepath.Abs("") and filepath.Abs(".") + // both resolve to the cwd, so this also handles the empty-string case + // gracefully. On error (rare; documented in os.Getwd), leave as-is — + // the lookup will fail consistently with the unfixed relative-input + // case rather than panic. Closes bug #12. + if abs, err := filepath.Abs(cfg.moduleRoot); err == nil { + cfg.moduleRoot = abs + } + if len(pkgs) == 0 || len(changedFiles) == 0 { return nil } diff --git a/pkg/changeset/changeset_test.go b/pkg/changeset/changeset_test.go index 8b2c422..13c13bf 100644 --- a/pkg/changeset/changeset_test.go +++ b/pkg/changeset/changeset_test.go @@ -1,6 +1,8 @@ package changeset_test import ( + "os" + "path/filepath" "sort" "testing" @@ -316,3 +318,55 @@ func TestResolve_EmptyFilePathSkipped(t *testing.T) { t.Errorf("Resolve = %v, want %v (empty file paths must be skipped)", got, want) } } + +// TestResolve_RelativeModuleRoot_AbsolutizedInternally is the regression test +// for bug #12. Pre-fix, passing a relative moduleRoot (notably ".") with +// relative changedFiles produced an empty result silently because filepath. +// Join(".", "rel/path") returns relative output, which can't match the +// absolute keys built from golist.Package.Dir. Post-fix, Resolve absolutizes +// moduleRoot via filepath.Abs before the dirMap lookup, so relative +// moduleRoots resolve correctly against the process cwd. +// +// The test sets up packages with Dir values rooted at the test's actual cwd +// (via os.Getwd) so the post-absolutize lookup succeeds with predictable +// values. Pre-fix this test fails (empty result); post-fix it passes. +func TestResolve_RelativeModuleRoot_AbsolutizedInternally(t *testing.T) { + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd: %v", err) + } + + pkgs := []golist.Package{ + // Dir values must be absolute (golist contract); construct them + // rooted at the test cwd so absolutize-of-"." resolves to the same + // prefix and the parent-dir lookup succeeds. + {ImportPath: "ex/pkga", Dir: filepath.Join(cwd, "pkga")}, + {ImportPath: "ex/pkgb", Dir: filepath.Join(cwd, "pkgb")}, + } + + tests := []struct { + name string + moduleRoot string + }{ + // Bug #12's headline case: literal "." (the CLI flag default). + {name: "dot", moduleRoot: "."}, + // Empty string: filepath.Abs("") also resolves to cwd; same fix. + {name: "empty", moduleRoot: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := changeset.Resolve( + []string{"pkga/a.go", "pkgb/b.go"}, + pkgs, + changeset.WithModuleRoot(tt.moduleRoot), + ) + want := []string{"ex/pkga", "ex/pkgb"} + if !stringSlicesEqual(got, want) { + t.Errorf("Resolve(...WithModuleRoot(%q)) = %v, want %v\n"+ + "(pre-fix empty output would indicate bug #12 regressed)", + tt.moduleRoot, got, want) + } + }) + } +}