fix #12: silent-empty output when --root is relative#13
Merged
Conversation
…ken)
Bug: cascade v0.1.0's documented invocation 'cascade --base=origin/main
--head=HEAD' (no --root, taking default '.') silently emitted empty
output and exited 0 against any real Go monorepo. The exact silent-
zero-packages failure mode that drove the gta → cascade pivot.
Root cause: changeset.Resolve does
abs = filepath.Clean(filepath.Join(cfg.moduleRoot, file))
With cfg.moduleRoot="." and file="rel/path", filepath.Join returns a
relative result. filepath.Dir of that yields a relative parent, which
can't match the absolute keys in dirMap (built from golist.Package.Dir
values, which are documented absolute). Lookup miss → no seeds → empty
RevDepClosure → empty stdout, exit 0.
Fix at three layers (defense in depth, per plan-mode user decisions):
1. Library (pkg/changeset/changeset.go): absolutize cfg.moduleRoot via
filepath.Abs before the dirMap lookup. Protects ANY current/future
library consumer (the deferred pkg/cascade.Run; third-party imports
of pkg/changeset) from the bug class. filepath.Abs("") and
filepath.Abs(".") both resolve to cwd.
2. CLI (internal/cli/cli.go): absolutize cfg.root once after parseFlags
succeeds, before validateConfig. The four downstream consumers
(runGitDiff, classifyGitDiffError, golist.WithDir,
changeset.WithModuleRoot) all receive a path absolutized exactly
once.
3. Diagnostic (internal/cli/cli.go): when len(affected) == 0 and
changedFiles contained .go-suffix entries, emit a stderr breadcrumb
cascade: N changed Go file(s) did not resolve to any package; check --root
Doesn't change exit code. Filtered on .go-suffix so docs-only PRs
stay silent (legitimate empty case). Bug #12 would have surfaced in
seconds during the original Banyan integration with this in place.
Regression coverage at three layers:
- pkg/changeset/changeset_test.go: TestResolve_RelativeModuleRoot_
AbsolutizedInternally (subtests for "." and ""). Pre-fix this test
fails with empty result; post-fix it passes.
- internal/cli/seam_test.go: TestRun_DefaultRootResolvesCWD. End-to-
end CLI invocation with no --root flag (default ".") + relative
changedFile + cwd-rooted absolute pkg.Dir values.
- internal/cli/seam_test.go: TestRun_DiagnosticOnSuspiciousEmpty +
TestRun_NoDiagnosticOnDocsOnlyEmpty. Pin the diagnostic's .go-suffix
filter behaviour; the second test is the explicit anti-regression
for docs-only PRs.
All 5 cascade packages at 100% coverage post-fix. make check-all green.
Smoke test against the cascade repo itself (cascade-against-cascade
with default --root) yields the expected 5-package affected-set, where
v0.1.0 yielded empty.
Verification target post-tag: getBanyan/api PR #3474 (parked as the
cascade-fix verification PR). Once v0.1.1 ships, that PR's CI re-trigger
provides the real-codebase closure (M2 F-18 / M5 F-19 lineage).
Closes #12.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
README: --root flag row gains "Absolute or relative; cascade absolutizes via filepath.Abs before use, so the default '.' resolves to the process cwd." Documents the fix without committing to brittle prose about the old bug. Retrospective at docs/dev/0013-bug12-fix-relative-root.md walks the three-layer fix shape, the four-test regression coverage, and the methodology lesson the bug surfaced: 100% line coverage doesn't imply behavioral-space coverage. The bug lived in a parameter-space cell (relative moduleRoot × absolute pkg.Dir) that no test happened to hit. Carry-forward into M7+: enumerate the parameter-space matrix explicitly when designing tests for new APIs, especially path-handling code. Verification post-merge: - v0.1.1 tag + GH release (standard M6 flow; manual tag+push to bypass the Makefile's release-dry-run go.sum bug). - getBanyan/api PR #3474 CI re-trigger against v0.1.1 (load-bearing real-codebase closure, M2 F-18 / M5 F-19 lineage). Note: the retro mentions the WithModuleRoot godoc update was bundled into commit 7f8d183 alongside the library fix; that's where the doc- comment lives, so the doc change naturally rode with the impl change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Closes #12.
What
cascade v0.1.0's documented invocation
cascade --base=origin/main --head=HEAD(no--root, taking default.) silently emitted empty output and exited 0 against any real Go monorepo. The exact silent-zero-packages failure mode that drove the gta → cascade pivot.Three-layer holistic fix (per plan-mode user decisions for defense-in-depth + diagnostic loudness):
pkg/changeset/changeset.go: absolutizecfg.moduleRootviafilepath.Absbefore the dirMap lookup. Defensive at the library level — protects ANY current/future caller (deferredpkg/cascade.Run; third-party imports ofpkg/changeset).internal/cli/cli.go: absolutizecfg.rootonce afterparseFlagssucceeds. Surgical fix; the four downstream consumers all receive a path absolutized exactly once.internal/cli/cli.go: stderr breadcrumb whenlen(affected) == 0andchangedFilescontains.go-suffix entries. Filtered on.goso docs-only PRs (legitimate empty case) stay silent. Doesn't change exit code.Where
pkg/changeset/changeset.gofilepath.Abs(cfg.moduleRoot)after the moduleRootSet check; updatedWithModuleRootgodoc to document the new behaviour.pkg/changeset/changeset_test.goTestResolve_RelativeModuleRoot_AbsolutizedInternally(subtests for.and"").internal/cli/cli.gofilepath.Abs(cfg.root)afterparseFlags; diagnostic-warning block inRunafterrunPipeline.internal/cli/seam_test.goTestRun_DefaultRootResolvesCWD,TestRun_DiagnosticOnSuspiciousEmpty,TestRun_NoDiagnosticOnDocsOnlyEmpty.README.md--rootflag row clarifies absolutization.docs/dev/0013-bug12-fix-relative-root.mdRoot cause (verified)
internal/cli/cli.go:189".".internal/cli/cli.go:279changeset.WithModuleRoot(cfg.root)hands the relative"."to the library.pkg/changeset/changeset.go:120dirMap[p.Dir] = p.ImportPath— keyed on absolute paths (pergolist.Package.Dir's contract).pkg/changeset/changeset.go:136filepath.Join(".", "rel/path")returns relative — mismatch.pkg/changeset/changeset.go:139RevDepClosure([])→ empty stdout, exit 0.The bug lived in a parameter-space cell that no test happened to hit (
relative moduleRoot × absolute pkg.Dir). Both M4 and M5 had 100% statement coverage but the pairing was untested in either layer.How to verify
Tests added
TestResolve_RelativeModuleRoot_AbsolutizedInternallypkg/changeset.and"". Pre-fix fails (empty); post-fix passes.TestRun_DefaultRootResolvesCWDinternal/clicli.Runwith no--rootflag against absolute pkg.Dir values. Pre-fix produced bug; post-fix produces affected-set.TestRun_DiagnosticOnSuspiciousEmptyinternal/cli.gofiles in changedFiles + empty affected-set. Pins the count format too.TestRun_NoDiagnosticOnDocsOnlyEmptyinternal/cli.go-suffix filter.Notable findings
runGitDiff,runGoListWrapper,signalContext); no real subprocess invocations.Post-merge follow-ups
v0.1.1(manual tag + push to bypass the Makefile'srelease-dry-rungo.sum bug).gh release create v0.1.1with notes calling out the fix + reproduction case + verification PR.Checklist
make checkpasses locallyWithModuleRoot's doc-comment changed; updated to document the absolutize behaviour)WithModuleRootdocumented contract is refined, not changed: relative paths now resolve correctly instead of silently failing)Breaking change?
No. The library-layer change refines
WithModuleRoot's documented behaviour (relative paths now resolve correctly viafilepath.Abs) without changing any contractual guarantee. Tests passing absolute paths see no observable change (absolutize-of-absolute is identity). The CLI-layer change makes--root=.(the default) work as documented; users who had been working around the bug by passing absolute--rootsee no observable change either.🤖 Generated with Claude Code