From ed64e14b34a8a55d6adfb660c1405a8e666d7430 Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 10:00:27 -0500 Subject: [PATCH 1/6] Added milestone 4 docs. --- docs/design/index.md | 4 +- ...-plan-changed-files-to-packages-mapping.md | 221 ++++++++++++++++++ 2 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md diff --git a/docs/design/index.md b/docs/design/index.md index 5779e7e..d4bbf7c 100644 --- a/docs/design/index.md +++ b/docs/design/index.md @@ -6,7 +6,8 @@ This index is automatically generated. Do not edit manually. | Number | Title | State | Updated | |--------|-------|-------|----------| -| 0005 | cascade — M4: `changeset` (Changed-Files-to-Packages Mapping) | Active | 2026-05-07 | +| 0006 | Implementation Plan: M4 — `pkg/changeset` (Changed-Files-to-Packages Mapping) | Active | 2026-05-07 | +| 0005 | M4: `changeset` (Changed-Files-to-Packages Mapping) | Active | 2026-05-07 | | 0004 | M3: `depgraph` (Reverse-Dep Closure) | Final | 2026-05-07 | | 0003 | M2: `golist` Adapter | Final | 2026-05-06 | | 0002 | M1: Repo Scaffold + CI Baseline | Final | 2026-05-06 | @@ -16,6 +17,7 @@ This index is automatically generated. Do not edit manually. ### Active +- [0006 - Implementation Plan: M4 — `pkg/changeset` (Changed-Files-to-Packages Mapping)](05-active/0006-implementation-plan-m4-pkgchangeset-changed-files-to-packages-mapping.md) - [0005 - cascade — M4: `changeset` (Changed-Files-to-Packages Mapping)](05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md) ### Final diff --git a/docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md b/docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md new file mode 100644 index 0000000..f8c6654 --- /dev/null +++ b/docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md @@ -0,0 +1,221 @@ +# Implementation Plan: M4 — `pkg/changeset` (Changed-Files-to-Packages Mapping) + +**Source spec:** [`docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md`](/Users/oubiwann/lab/geomyidia/cascade/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md) +**Predecessor:** package-layout refactor PR #8 (must merge to `main` before M4 PR opens). +**Successor:** M5 — CLI + main wiring (consumes `Resolve`'s output as seeds for `depgraph.RevDepClosure`). + +## Context + +M4 is the bridge between cascade's two algorithmic primitives: `golist` (M2) produces a `[]golist.Package` with file-membership information; `depgraph` (M3) computes reverse-transitive closures from a seed set. `changeset.Resolve` turns *"these files changed"* (the output of `git diff --name-only`) into *"these packages are the seeds"* (the input of `depgraph.RevDepClosure`). + +Why now: the package-layout refactor (PR #8) lands `pkg/changeset/` with an M1 doc.go stub and the per-package coverage gate already wired at 100%. M4 fills the implementation directly into that location. Once M4 closes, the trio of pure packages (`pkg/golist`, `pkg/depgraph`, `pkg/changeset`) compose without adapter code: `g := depgraph.Build(pkgs); seeds := changeset.Resolve(changedFiles, pkgs, opts...); affected := g.RevDepClosure(seeds)`. This is the API the README's Library section already advertises; M4 makes it real. + +The intended outcome: `pkg/changeset/` ships one exported function (`Resolve`) plus one option type (`Option`) plus one option constructor (`WithModuleRoot`); zero exported types beyond those; 100% statement coverage; godoc renders cleanly; M5 can wire the CLI on top with no pending design questions. + +## Decisions resolved (with user input from plan-mode review) + +The spec author left 6 open questions. The user resolved the three with material implementation impact during plan-mode review; the remaining three follow the spec's leans. + +| # | Question | Decision | +|---|---|---| +| Q1 | `moduleRoot` positional vs functional option | **Functional option.** `Resolve(changedFiles, pkgs, opts ...Option)`; the only option is `WithModuleRoot(string)`. **User override of spec lean**: spec leaned positional for simplicity; user prefers the option pattern so the io-edge default (Getwd fallback) can be opted out of in tests. | +| Q2 | Empty/unset `moduleRoot` behaviour with relative paths | **Fall back to `os.Getwd()` at call time, but only when `WithModuleRoot` was not supplied.** Tests pass `WithModuleRoot(...)` explicitly so they don't trigger the io. **Implementation note**: this introduces a single io edge into a pure-data package, requiring the function-variable seam pattern from M2's `golist` (see "Implementation approach" below). If `os.Getwd` returns an error, `moduleRoot` stays empty and relative paths silently fail to resolve (parent-dir lookup misses) — same outcome as the spec's "skip relative paths" alternative, but only in the failure case. | +| Q3 | Lexical-form dedup via `filepath.Clean` | **Yes.** `filepath.Clean` is applied before lookup so `pkga/a.go`, `./pkga/a.go`, and `pkga/./a.go` all collapse to the same dir-key. Followed spec lean. | +| Q4 | Test fixtures: synthetic-only or repo fixture | **Synthetic-only.** Pure-data, no filesystem reads in production. Followed spec lean. | +| Q5 | `IgnoredGoFiles` handling | **Map regardless.** The lookup is parent-dir-match — file-list membership doesn't enter the algorithm at all. Followed spec lean (and confirmed by user). The implementation builds a `dir → importPath` map from `pkg.Dir` only; whether a file is in `GoFiles` / `TestGoFiles` / `XTestGoFiles` / `IgnoredGoFiles` is irrelevant. | +| Q6 | F-13 (godoc rendering) evidence shape | **Full `go doc` output capped at first 20 lines.** Consistent with the spec's verify command. Followed spec lean. | + +## Implementation approach + +### File layout + +``` +pkg/changeset/ +├── doc.go (M1 stub — untouched; spec F-1) +├── changeset.go Resolve + Option + WithModuleRoot + internal config + getCwd seam + helpers +├── changeset_test.go public-API tests (package changeset_test, per TE-43) +├── helpers_test.go buildTestPackages helper (package changeset_test) +└── seam_test.go getCwd seam-driven tests (package changeset, internal — only place we cross the package boundary) +``` + +`changeset.go` will land at ~80–120 lines including comments. If it grows beyond ~200, split into `changeset.go` (Resolve + algorithm) + `options.go` (Option type + constructors) — decide at write time. + +### Public API surface + +Three exported names (counting Option's constructor): + +```go +package changeset + +// Option configures a Resolve call. +type Option func(*config) + +// WithModuleRoot sets the module root used to resolve relative entries in +// changedFiles. If WithModuleRoot is not supplied, Resolve falls back to +// os.Getwd() at call time. Tests should pass WithModuleRoot explicitly to +// avoid the io and keep test outcomes independent of the working directory. +func WithModuleRoot(dir string) Option + +// Resolve maps changed file paths to the import paths of the packages whose +// Go files they are. Returns the set of distinct import paths sorted +// lexicographically for deterministic CI behaviour. +// +// [full doc per spec — mapping rules, dedup, sort, no error] +func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string +``` + +Every other type / function is unexported. + +### Internal representation + +```go +type config struct { + moduleRoot string + moduleRootSet bool // distinguishes "explicitly empty" from "unset" +} + +// getCwd is the function-variable seam over os.Getwd. Production builds use +// os.Getwd; seam_test.go replaces it to drive the success / error branches +// without depending on the actual test cwd. Pattern matches M2's runGoList. +var getCwd = os.Getwd +``` + +### Algorithm (O(P + F)) + +1. Apply options into a `config{}`. (`for _, opt := range opts { opt(&cfg) }`) +2. If `!cfg.moduleRootSet`, call `getCwd()`. On success → `cfg.moduleRoot = cwd`. On error → leave empty. +3. Build `dirMap := map[string]string{}` mapping `pkg.Dir` → `pkg.ImportPath` (one entry per pkg). Pre-allocate with `make(map[string]string, len(pkgs))` per AP-40. +4. Build the result set as `seen := map[string]struct{}{}` to dedup naturally. +5. For each file in `changedFiles`: + - Skip empty. + - Skip non-`.go` files (`!strings.HasSuffix(file, ".go")`). + - If relative (`!filepath.IsAbs(file)`), join with `cfg.moduleRoot` (which may be empty if Getwd failed; in that case `filepath.Join("", relPath)` returns the path unchanged, which won't match any absolute `pkg.Dir`, so it gets silently skipped at lookup — correct behaviour). + - `filepath.Clean` the joined/absolute path. + - `parent := filepath.Dir(cleanPath)`. + - If `importPath, ok := dirMap[parent]; ok`, record in `seen`. +6. Convert `seen` keys to a sorted `[]string`. Pre-allocate with `make([]string, 0, len(seen))` per AP-40. Return. + +Empty / nil inputs at any stage produce `nil` return (no allocation when nothing matches). + +### Existing patterns reused + +- **M2's function-variable seam pattern** (`pkg/golist/golist.go:96-100`'s `runGoList = defaultRunGoList` + `pkg/golist/seam_test.go:11-17`'s `withSeam` helper). Same structural shape: an internal var holds the io-edge function; tests in the same package replace it via `t.Cleanup(restore)`. M4's `getCwd` follows the identical pattern. Reuse the exact `withSeam` idiom. +- **M3's table-driven test layout** (`pkg/depgraph/depgraph_test.go` + `pkg/depgraph/helpers_test.go`). One test file with `package changeset_test` for public-API cases; one helpers file for `buildTestPackages` shared across tests. Each subtest also asserts `sort.StringsAreSorted(got)` so non-determinism surfaces inline (M3's F-13 pattern). +- **Coverage-gate convention** in `scripts/coverage-check.sh:35-38` already lists `pkg/changeset` at threshold 100; no change needed. +- **`golist.Package.Dir`** is documented absolute (`pkg/golist/golist.go:14-21` package comment) and observed absolute in the sample-module fixture. The dir-map lookup relies on this; doc the assumption inline. + +## Test strategy + +All tests in `package changeset_test` for public-API cases (TE-43); seam-driven tests in `package changeset` (internal, `seam_test.go` only). Plain stdlib `testing`, no testify (matches existing cascade convention; honours TE-15). + +### Helper (`helpers_test.go`) + +```go +func buildTestPackages(t *testing.T, entries []golist.Package) []golist.Package +``` + +Spec proposed a map-based helper but the inline `[]golist.Package{...}` literal is cleaner for tests where each pkg has different `Dir` values, so the helper is essentially a thin t.Helper-bearing wrapper. Decide at write time; default to simpler form. + +### Test functions + +| Test function | Cases | Spec ledger row | +|---|---|---| +| `TestResolve_StandardCases` | 16 cases per spec (empty, nil pkgs, single/double Go file, _test.go, xtest .go, mixed Go+non-Go, file outside, file in subdir, removed, relative+moduleRoot, absolute, .. cleanup, dup, unsorted) | F-3 | +| `TestResolve_HandTraceable` | 4-pkg synthetic case from spec | F-4, F-6 (xtest), F-9 (removed) | +| `TestResolve_PathNormalisation` | OS-portable cases: redundant separators, `..`, mixed-style | F-5 | +| `TestResolve_DefaultUsesGetCwd` (in seam_test.go) | No WithModuleRoot supplied; getCwd seam returns "/test/cwd"; verify relative path resolves against it | (no spec row; coverage-completion) | +| `TestResolve_GetCwdErrorTolerated` (in seam_test.go) | getCwd seam returns error; verify relative paths silently skip; no panic | (no spec row; coverage-completion) | +| `TestResolve_IgnoredGoFiles_MappedRegardless` | File in a package's Dir whose name appears in IgnoredGoFiles → still maps to the package | (covers Q5 decision behaviourally) | + +Coverage target: 100% on `pkg/changeset/`. The seam tests close the os.Getwd success/error branches without depending on the test process's actual cwd. + +## Implementation order + +1. **Pre-flight reading** (refresh against the spec's load list): + - `assets/ai/go/SKILL.md` Critical Rules + Document Selection Guide (already in context from M3 session). + - `assets/ai/go/guides/09-anti-patterns.md` walked. Specific IDs cited: **AP-29** (no interface returns), **AP-30** (no producer-side interfaces), **AP-36** (early-return over deep nesting), **AP-40** (slice prealloc), **AP-44** (no log+return), **AP-52** (filepath confinement — informational; M4 doesn't open files but worth knowing the prefix-check pattern exists). + - `assets/ai/go/guides/02-api-design.md` API-42 (no globals; threaded through Resolve's args + opts). + - `assets/ai/go/guides/07-testing.md` TE-01..15 + TE-43 (already in context). + - `assets/ai/go/guides/11-documentation.md` DC-01 + DC-02 (already in context). +2. Branch `m4/changeset-mapping` off post-PR-#8-merge `main`. +3. Implement `pkg/changeset/changeset.go` — Option/WithModuleRoot/config/getCwd seam/Resolve. +4. Public-API tests in `pkg/changeset/changeset_test.go` (16 StandardCases + HandTraceable + PathNormalisation + IgnoredGoFiles_MappedRegardless). +5. Helper file `pkg/changeset/helpers_test.go` (buildTestPackages). +6. Internal seam tests in `pkg/changeset/seam_test.go` (DefaultUsesGetCwd + GetCwdErrorTolerated). +7. Local verification: + - `go test -race -count=10 ./pkg/changeset/...` (F-3..F-10) + - `bash scripts/coverage-check.sh` (F-11) + - `make check-all` (build + lint + test + coverage gate) + - `go doc github.com/geomyidia/cascade/pkg/changeset | head -20` (F-13) +8. Pre-PR CC self-check: walk each ledger row F-1..F-14, comparing planned-evidence text against criterion text (M3 retro carry-forward). +9. Commit + push + open PR + monitor CI matrix. +10. Closing retrospective at `docs/dev/0010-implementation-retrospective-m4-changeset.md` walking F-1..F-14 row-by-row. + +## Critical files + +| Path | Action | Notes | +|---|---|---| +| `pkg/changeset/doc.go` | Untouched | M1 stub satisfies F-1. | +| `pkg/changeset/changeset.go` | Create | Option, WithModuleRoot, config, getCwd, Resolve. | +| `pkg/changeset/changeset_test.go` | Create | Public API tests in `package changeset_test`. | +| `pkg/changeset/helpers_test.go` | Create | `buildTestPackages` helper. | +| `pkg/changeset/seam_test.go` | Create | Internal tests in `package changeset` for the `getCwd` seam. | +| `scripts/coverage-check.sh` | No change | Already lists `pkg/changeset` at threshold 100 from PR #8. | +| `docs/dev/0010-implementation-retrospective-m4-changeset.md` | Create at close | Closing report with per-row ledger walk. | + +`README.md`, `CLAUDE.md`, `CONTRIBUTING.md` need no changes — the architecture sections already cover `pkg/changeset` post-refactor. + +## Verification (mapping to spec ledger F-1..F-14) + +| ID | Verify | Planned evidence shape | +|---|---|---| +| F-1 | `head -3 pkg/changeset/doc.go \| grep '^// Package changeset'` | M1 stub, untouched. Carry-over. | +| F-2 | `go doc ./pkg/changeset.Resolve` | `func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string` — exact match (note: signature differs from spec's positional form per Q1 decision; the active spec should pick this up at next ODM promotion). | +| F-3 | `go test -run TestResolve_StandardCases ./pkg/changeset` | 16 named subtests pass. | +| F-4 | `go test -run TestResolve_HandTraceable ./pkg/changeset` | 4-pkg case with hand-derived expected closures passes. | +| F-5 | `go test -run TestResolve_PathNormalisation ./pkg/changeset` | OS-portable cases pass. | +| F-6 | `go test -run 'TestResolve_StandardCases/_test\.go_in_pkga' ./pkg/changeset` | Test/xtest contract verified. | +| F-7 | `go test -run 'TestResolve_StandardCases/mixed' ./pkg/changeset` | Non-Go skipped. | +| F-8 | `go test -run 'TestResolve_StandardCases/Go_file_outside' ./pkg/changeset` | Outside-package files skipped, no error. | +| F-9 | `go test -run 'TestResolve_StandardCases/removed_Go_file' ./pkg/changeset` | Parent-dir match works for removed files. | +| F-10 | `go test -count=10 -run TestResolve ./pkg/changeset` | 10 runs, output sorted + deduped. Each subtest also asserts `sort.StringsAreSorted(got)` inline. | +| F-11 | `bash scripts/coverage-check.sh` | `ok: github.com/geomyidia/cascade/pkg/changeset coverage 100% >= 100%`. The `getCwd` seam closes the os.Getwd branch coverage gap. | +| F-12 | `[ "$(go list -m all \| wc -l \| tr -d ' ')" = "1" ]` | `1` (own module only; `pkg/changeset` imports `os`, `path/filepath`, `sort`, `strings` from stdlib + `pkg/golist` from the own module). | +| F-13 | `go doc github.com/geomyidia/cascade/pkg/changeset \| head -20` | Renders package overview, Option type, WithModuleRoot, Resolve with full doc comments. Capped at 20 lines per Q6 decision. | +| F-14 | reviewer reads closing report | `docs/dev/0010-implementation-retrospective-m4-changeset.md` enumerates substrate loaded + AP/API/TE/DC IDs cited. | + +**Note on F-2 (signature drift from spec):** the spec says `func Resolve(changedFiles []string, pkgs []golist.Package, moduleRoot string) []string` (positional). User overrode to functional option during plan-mode review. The active spec at `docs/design/05-active/0005-...md` will need an amendment to match; CC will note this in the closing retrospective so the spec stays the source-of-truth. Same disclosed-amendment pattern as M3's F-20 (Stats type) addition. + +## Risks & mitigations + +- **Seam pattern correctness.** The `getCwd` seam must restore `os.Getwd` after each test. Use the `t.Cleanup(func() { getCwd = os.Getwd })` pattern from M2's `withSeam` helper (`pkg/golist/seam_test.go`). Mitigation: copy the M2 helper's exact shape; lint will catch missing restores via `t.Cleanup` discipline if a future reviewer adds a seam test without it. +- **`os.Getwd` introduces io into a "pure-data" package.** Per the spec's intent, M4 was supposed to be pure-data. Q2's resolution (default to `os.Getwd`) is a deliberate trade-off: friendliness at the call site (M5 CLI doesn't need to compute moduleRoot itself) at the cost of a single io edge. The seam keeps the io testable. Documentation: package doc.go's stub already says "pure" — update inline doc on Resolve to clarify the io-edge fallback. +- **Symlinks in `pkg.Dir` vs `changedFiles`.** `golist` reports resolved paths; `git diff` typically emits as-checked-in. If a contributor's checkout has symlink-divergence, lookup may miss. Per spec mitigation: document the limitation in Resolve's godoc; do not follow symlinks in M4. +- **Cross-platform path separators.** `pkg.Dir` may use `\` on Windows; `filepath.Clean` and `filepath.Dir` handle this natively. Don't normalise via `filepath.ToSlash` — work in OS-native form throughout. CI is Linux-only so any Windows-specific bug surfaces at first Windows-contributor checkout. +- **Lint-cache drift recurrence.** OUT-1 (M3's fresh-cache fix) is in place; M4 inherits the protection. No new mitigation needed. +- **Coverage gate at 100% with the io edge.** The seam test pattern is exactly what closes this branch. Without the seam, the os.Getwd-success path (or the os.Getwd-error path) would be untestable in-process and the gate would drop below 100%. + +## Out of scope + +- Forward closure (packages → files). Spec out-of-scope. +- File-content inspection. Path-only logic. +- Build-tag awareness in change-set logic. Tag selection happens at `golist.Run` time; `Resolve` operates on whatever the input pkg list says. +- Symlink resolution. +- Cross-module change-sets. +- Renames as two events (handled trivially by independent per-path mapping). +- Error returns from `Resolve`. The whole API is graceful. +- A `WithIgnore(pattern)` option to skip files matching some glob. Speculative. +- A `ResolveFromAbsolute` variant that requires absolute paths only. Non-breaking future addition if perf surfaces. + +## Open items deferred to closing retrospective + +- Final pre-PR CC self-check on F-1..F-14 evidence-vs-criterion text. +- Confirmation that the F-2 spec-drift (positional → functional option) is correctly captured as a disclosed amendment, mirroring M3's F-20 handling. +- Whether the seam pattern ended up cleaner than expected, or surfaced any subtleties (e.g. parallel test interactions on the global `getCwd` var). M2's seam was clean; M4 should mirror. + +## Carry-forward expected to land in M4 retrospective + +- **Seam pattern reuse evidence.** M4 is the second use of M2's `getCwd`-style function-variable seam. Worth noting whether the pattern generalised cleanly (it should — the shape is mechanical). +- **Pure-data + io-edge tension.** M4 is the first cascade package that's *mostly* pure but has *one* documented io edge. Document the trade-off the spec amendment introduced (Q2's user override) so future packages know the convention: if a single io edge is needed and the rest of the package is pure, factor it through a function-variable seam and document the option-pattern opt-out in the godoc. +- **Plan-and-PR sequencing.** PR #8 (refactor) must merge before this PR opens, per the predecessor relationship. Confirm the merge happened before branching off. +- **End-to-end pipeline ready for M5.** Once M4 closes, the full `golist.Run → depgraph.Build → changeset.Resolve → g.RevDepClosure` chain is functional in-library. M5 wires it to a CLI; this retrospective should explicitly confirm the chain composes without adapter code (e.g. via a small integration test that runs the four steps against the sample-module fixture). From 75617a281ce047c31469e9433a79b10633a479d0 Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 11:49:14 -0500 Subject: [PATCH 2/6] m4: implement changeset package (changed-files-to-packages mapping) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bridge between cascade's two algorithmic primitives: golist (M2) produces []golist.Package; depgraph (M3) walks reverse-edge closures from a seed set. changeset.Resolve turns "these files changed" (git diff --name-only output) into "these packages are the seeds" — the input of g.RevDepClosure. Surface: one exported function (Resolve), one option type (Option), one option constructor (WithModuleRoot). Algorithm: build dirMap from pkg.Dir → ImportPath, walk changedFiles, parent-dir lookup, sorted dedupe via map[string]struct{}; O(P+F). Disclosed amendments to the spec (carried in the closing retro): - F-2 signature: spec said positional moduleRoot string; user override during plan-mode review (Q1) chose functional option pattern. - Q2: empty/unset moduleRoot triggers os.Getwd fallback. Tests pass WithModuleRoot explicitly to bypass the io. The single io edge is hooked through an internal getCwd function-variable seam, mirroring M2's runGoList pattern (pkg/golist/seam_test.go's withSeam idiom is reused as withCwdSeam in pkg/changeset/seam_test.go). - doc.go updated from "All operations are pure — no io" to a description that names the os.Getwd fallback and the seam-based testability story. F-1's verify (the // Package changeset line) is preserved. Coverage at 100% statement coverage on pkg/changeset, joining pkg/golist, pkg/depgraph, and internal/project at the same gate. Each table-driven subtest also asserts sort.StringsAreSorted(got) inline so non-determinism would surface in the failing subtest, not just as a -count=10 flake (M3's What-Worked carry-forward). Spec/plan: docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/changeset/changeset.go | 153 +++++++++++++++ pkg/changeset/changeset_test.go | 318 ++++++++++++++++++++++++++++++++ pkg/changeset/doc.go | 5 +- pkg/changeset/helpers_test.go | 22 +++ pkg/changeset/seam_test.go | 73 ++++++++ 5 files changed, 570 insertions(+), 1 deletion(-) create mode 100644 pkg/changeset/changeset.go create mode 100644 pkg/changeset/changeset_test.go create mode 100644 pkg/changeset/helpers_test.go create mode 100644 pkg/changeset/seam_test.go diff --git a/pkg/changeset/changeset.go b/pkg/changeset/changeset.go new file mode 100644 index 0000000..9e93045 --- /dev/null +++ b/pkg/changeset/changeset.go @@ -0,0 +1,153 @@ +package changeset + +import ( + "os" + "path/filepath" + "sort" + "strings" + + "github.com/geomyidia/cascade/pkg/golist" +) + +// Option configures a Resolve call. Apply via Resolve's variadic parameter. +// The only option in v0.x is WithModuleRoot. +type Option func(*config) + +// config holds the resolved option state for a single Resolve call. +// Unexported: callers compose configuration only via With* constructors. +type config struct { + moduleRoot string + moduleRootSet bool +} + +// WithModuleRoot sets the module root used to resolve relative entries in +// changedFiles. When supplied, Resolve uses dir directly without consulting +// the filesystem. +// +// 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. +// +// 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. +func WithModuleRoot(dir string) Option { + return func(c *config) { + c.moduleRoot = dir + c.moduleRootSet = true + } +} + +// getCwd is the function-variable seam over os.Getwd. Production builds use +// os.Getwd; tests can replace it via the seam helper to drive the fallback's +// success and error branches without depending on the actual test cwd. The +// pattern matches pkg/golist's runGoList seam. +var getCwd = os.Getwd + +// Resolve maps changed file paths to the import paths of the packages whose +// Go files they are. Returns the set of distinct import paths sorted +// lexicographically for deterministic CI behaviour. +// +// changedFiles are paths to Go files that have been modified, added, +// renamed, or removed (typically `git diff --name-only` output, one entry +// per line). Paths may be relative (resolved against moduleRoot) or +// absolute; filepath.Clean is applied before lookup. +// +// pkgs is the package list returned by golist.Run; each Package's Dir field +// (absolute path) drives the lookup. +// +// opts configure the call. The only option in v0.x is WithModuleRoot; if +// not supplied, Resolve falls back to os.Getwd. If os.Getwd itself returns +// an error (rare), moduleRoot stays empty and relative paths silently fail +// to resolve — same outcome as passing WithModuleRoot(""). +// +// Mapping rules: +// - A file ending in ".go" whose parent directory exactly matches some +// package's Dir maps to that package's ImportPath. The parent-directory +// match works regardless of whether the file currently exists on disk, +// so removed Go files map correctly. +// - _test.go files map to the same package as non-test files in the same +// directory. Internal-test (package foo) and external-test (package +// foo_test) both yield foo's ImportPath. +// - Files in a package's IgnoredGoFiles (build-tag-excluded) map to the +// package — the rule is parent-dir match; file-list membership is +// irrelevant to the lookup. +// - Non-Go files (no ".go" extension) are silently skipped. +// - Files in subdirectories of a package's Dir (e.g. testdata/) are +// skipped — the parent directory does not match any package's Dir. +// - Files outside any package's Dir are silently skipped. +// +// Error returns: none. Every input shape is handled gracefully — empty +// slices, nil pkgs, blank moduleRoot, paths that don't resolve to any +// package, removed files, and os.Getwd failures all produce sensible +// output (typically an empty result). +// +// Symlinks are not followed. If changedFiles contains a path that's a +// symlink and pkg.Dir contains the resolved real path (or vice versa), the +// lexical comparison may miss. Callers that need symlink-aware mapping +// should canonicalise before calling Resolve. +// +// Determinism: identical inputs yield identical output across runs. +// Duplicates are deduplicated. +// +// Complexity: O(P + F) where P is len(pkgs) and F is len(changedFiles), +// plus O(k log k) for the final sort on the result-set size k. +func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string { + cfg := config{} + for _, opt := range opts { + opt(&cfg) + } + if !cfg.moduleRootSet { + if cwd, err := getCwd(); err == nil { + cfg.moduleRoot = cwd + } + } + + if len(pkgs) == 0 || len(changedFiles) == 0 { + return nil + } + + // Build dir → ImportPath map from pkgs (one entry per package). Skip + // entries with empty Dir or ImportPath defensively — go list -deps -json + // shouldn't emit them, but synthetic test inputs sometimes do, and a + // silent skip is friendlier than a panic. + dirMap := make(map[string]string, len(pkgs)) + for _, p := range pkgs { + if p.Dir == "" || p.ImportPath == "" { + continue + } + dirMap[p.Dir] = p.ImportPath + } + + // Collect unique import paths via a set; convert to a sorted slice once. + seen := make(map[string]struct{}) + for _, file := range changedFiles { + if file == "" { + continue + } + if !strings.HasSuffix(file, ".go") { + continue + } + var abs string + if filepath.IsAbs(file) { + abs = filepath.Clean(file) + } else { + abs = filepath.Clean(filepath.Join(cfg.moduleRoot, file)) + } + parent := filepath.Dir(abs) + if importPath, ok := dirMap[parent]; ok { + seen[importPath] = struct{}{} + } + } + + if len(seen) == 0 { + return nil + } + out := make([]string, 0, len(seen)) + for ip := range seen { + out = append(out, ip) + } + sort.Strings(out) + return out +} diff --git a/pkg/changeset/changeset_test.go b/pkg/changeset/changeset_test.go new file mode 100644 index 0000000..8b2c422 --- /dev/null +++ b/pkg/changeset/changeset_test.go @@ -0,0 +1,318 @@ +package changeset_test + +import ( + "sort" + "testing" + + "github.com/geomyidia/cascade/pkg/changeset" + "github.com/geomyidia/cascade/pkg/golist" +) + +// defaultPkgs is a 3-package fixture used by most StandardCases rows. Dir +// values are POSIX-style absolute paths; tests run on Linux/macOS. +func defaultPkgs() []golist.Package { + return []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/m/pkga"}, + {ImportPath: "ex/pkgb", Dir: "/m/pkgb"}, + {ImportPath: "ex/pkgc", Dir: "/m/pkgc"}, + } +} + +// TestResolve_StandardCases (F-3, plus F-6/F-7/F-8/F-9 as named subtests +// per the spec's verify regexes) walks 16 synthetic mapping topologies that +// together cover every branch in Resolve plus the spec's contractual rules. +func TestResolve_StandardCases(t *testing.T) { + tests := []struct { + name string + changedFiles []string + pkgs []golist.Package + moduleRoot string + want []string + }{ + { + name: "empty_changedFiles", + changedFiles: nil, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: nil, + }, + { + name: "nil_pkgs", + changedFiles: []string{"pkga/a.go"}, + pkgs: nil, + moduleRoot: "/m", + want: nil, + }, + { + name: "single_Go_file_in_pkga", + changedFiles: []string{"pkga/a.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "two_Go_files_in_pkga_deduped", + changedFiles: []string{"pkga/a.go", "pkga/b.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "files_in_pkga_and_pkgb_sorted", + changedFiles: []string{"pkgb/b.go", "pkga/a.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga", "ex/pkgb"}, + }, + { + name: "_test.go_in_pkga", + changedFiles: []string{"pkga/a_test.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "xtest_dot_go_in_pkga", + changedFiles: []string{"pkga/a_x_test.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "mixed_go_and_non_go", + changedFiles: []string{"pkga/a.go", "README.md", "pkgb/data.json"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "Go_file_outside_any_package", + changedFiles: []string{"cmd/cascade/main.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: nil, + }, + { + name: "Go_file_in_subdirectory_of_pkg_dir", + changedFiles: []string{"pkga/testdata/foo.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: nil, + }, + { + name: "removed_Go_file_in_pkga", + changedFiles: []string{"pkga/deleted.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "relative_path_resolved_against_moduleRoot", + changedFiles: []string{"pkga/a.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "absolute_path_used_directly", + changedFiles: []string{"/m/pkgb/b.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/elsewhere", // ignored when path is absolute + want: []string{"ex/pkgb"}, + }, + { + name: "path_with_dot_dot_components_cleaned", + changedFiles: []string{"pkga/../pkgb/b.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkgb"}, + }, + { + name: "duplicate_entries_in_changedFiles", + changedFiles: []string{"pkga/a.go", "pkga/a.go", "pkga/a.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "unsorted_entries_yield_sorted_output", + changedFiles: []string{"pkgc/c.go", "pkga/a.go", "pkgb/b.go"}, + pkgs: defaultPkgs(), + moduleRoot: "/m", + want: []string{"ex/pkga", "ex/pkgb", "ex/pkgc"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := changeset.Resolve(tt.changedFiles, tt.pkgs, + changeset.WithModuleRoot(tt.moduleRoot)) + if !stringSlicesEqual(got, tt.want) { + t.Errorf("Resolve(%v, ..., WithModuleRoot(%q)) = %v, want %v", + tt.changedFiles, tt.moduleRoot, got, tt.want) + } + if !sort.StringsAreSorted(got) { + t.Errorf("Resolve(...) = %v is not sorted lexicographically", got) + } + }) + } +} + +// TestResolve_HandTraceable (F-4) is the spec's exit-criterion example — +// a 4-package graph with hand-derived expected output. Documents the +// mapping semantics for future maintainers. +// +// Packages: +// ex/pkga Dir=/m/pkga +// ex/pkgb Dir=/m/pkgb (xtest file b_x_test.go in pkgb's Dir) +// ex/pkgc Dir=/m/pkgc +// ex/pkgd Dir=/m/pkgd (build-tag-excluded d_linux.go in pkgd's Dir) +func TestResolve_HandTraceable(t *testing.T) { + pkgs := []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/m/pkga"}, + {ImportPath: "ex/pkgb", Dir: "/m/pkgb"}, + {ImportPath: "ex/pkgc", Dir: "/m/pkgc"}, + {ImportPath: "ex/pkgd", Dir: "/m/pkgd"}, + } + tests := []struct { + name string + changedFiles []string + want []string + }{ + {"pkga_a_go", []string{"pkga/a.go"}, []string{"ex/pkga"}}, + {"pkga_a_test_go", []string{"pkga/a_test.go"}, []string{"ex/pkga"}}, + {"pkgb_xtest_go", []string{"pkgb/b_x_test.go"}, []string{"ex/pkgb"}}, + {"pkga_and_pkgc", []string{"pkga/a.go", "pkgc/c.go"}, []string{"ex/pkga", "ex/pkgc"}}, + {"subdirectory_of_pkga_skipped", []string{"pkga/sub/x.go"}, nil}, + {"non_go_file_skipped", []string{"README.md"}, nil}, + {"empty_changeset", nil, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := changeset.Resolve(tt.changedFiles, pkgs, + changeset.WithModuleRoot("/m")) + if !stringSlicesEqual(got, tt.want) { + t.Errorf("Resolve(%v, pkgs, WithModuleRoot(/m)) = %v, want %v", + tt.changedFiles, got, tt.want) + } + }) + } +} + +// TestResolve_PathNormalisation (F-5) covers OS-portable path handling: +// redundant separators, .. traversal, ./ prefixes, and absolute-vs-relative +// paths. filepath.Clean must normalise all of them before lookup. +func TestResolve_PathNormalisation(t *testing.T) { + pkgs := defaultPkgs() + tests := []struct { + name string + changedFiles []string + moduleRoot string + want []string + }{ + { + name: "redundant_separators", + changedFiles: []string{"pkga//a.go", "pkgb///b.go"}, + moduleRoot: "/m", + want: []string{"ex/pkga", "ex/pkgb"}, + }, + { + name: "leading_dot_slash", + changedFiles: []string{"./pkga/a.go"}, + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "interior_dot_slash", + changedFiles: []string{"pkga/./a.go"}, + moduleRoot: "/m", + want: []string{"ex/pkga"}, + }, + { + name: "dot_dot_traversal", + changedFiles: []string{"pkga/../pkgb/b.go"}, + moduleRoot: "/m", + want: []string{"ex/pkgb"}, + }, + { + name: "absolute_path_normalised", + changedFiles: []string{"/m//pkga//a.go"}, + moduleRoot: "/elsewhere", + want: []string{"ex/pkga"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := changeset.Resolve(tt.changedFiles, pkgs, + changeset.WithModuleRoot(tt.moduleRoot)) + if !stringSlicesEqual(got, tt.want) { + t.Errorf("Resolve(%v, ..., WithModuleRoot(%q)) = %v, want %v", + tt.changedFiles, tt.moduleRoot, got, tt.want) + } + }) + } +} + +// TestResolve_IgnoredGoFiles_MappedRegardless verifies the Q5 decision: a +// file in a package's Dir maps to that package even when it's not in any +// of GoFiles / TestGoFiles / XTestGoFiles (e.g. an IgnoredGoFiles entry, +// or an entirely synthetic name). The lookup is parent-dir match; +// file-list membership is irrelevant to the algorithm. +func TestResolve_IgnoredGoFiles_MappedRegardless(t *testing.T) { + pkgs := []golist.Package{ + { + ImportPath: "ex/pkga", + Dir: "/m/pkga", + GoFiles: []string{"a.go"}, + IgnoredGoFiles: []string{"a_linux.go"}, + }, + } + // Both a.go (in GoFiles) and a_linux.go (in IgnoredGoFiles) live in the + // same Dir; both should map to ex/pkga regardless of file-list membership. + // Even a_speculative.go (in neither file list) maps because the lookup + // only considers parent-dir. + got := changeset.Resolve( + []string{"pkga/a.go", "pkga/a_linux.go", "pkga/a_speculative.go"}, + pkgs, + changeset.WithModuleRoot("/m"), + ) + want := []string{"ex/pkga"} + if !stringSlicesEqual(got, want) { + t.Errorf("Resolve = %v, want %v (file-list membership must be irrelevant)", got, want) + } +} + +// TestResolve_PackagesWithEmptyFieldsSkipped covers the defensive branches +// in the dirMap-build loop: packages with empty Dir or empty ImportPath +// are silently skipped (synthetic test inputs sometimes produce these; +// production go list output shouldn't). +func TestResolve_PackagesWithEmptyFieldsSkipped(t *testing.T) { + pkgs := []golist.Package{ + {ImportPath: "", Dir: "/m/empty-import-path"}, // skipped + {ImportPath: "ex/no-dir"}, // skipped (empty Dir) + {ImportPath: "ex/pkga", Dir: "/m/pkga"}, // kept + } + got := changeset.Resolve( + []string{"pkga/a.go", "empty-import-path/x.go", "no-dir/x.go"}, + pkgs, + changeset.WithModuleRoot("/m"), + ) + want := []string{"ex/pkga"} + if !stringSlicesEqual(got, want) { + t.Errorf("Resolve = %v, want %v (packages with empty Dir/ImportPath must be skipped)", got, want) + } +} + +// TestResolve_EmptyFilePathSkipped covers the defensive `if file == ""` +// branch inside the changedFiles loop. +func TestResolve_EmptyFilePathSkipped(t *testing.T) { + got := changeset.Resolve( + []string{"", "pkga/a.go", ""}, + defaultPkgs(), + changeset.WithModuleRoot("/m"), + ) + want := []string{"ex/pkga"} + if !stringSlicesEqual(got, want) { + t.Errorf("Resolve = %v, want %v (empty file paths must be skipped)", got, want) + } +} diff --git a/pkg/changeset/doc.go b/pkg/changeset/doc.go index d78da32..53fdcdc 100644 --- a/pkg/changeset/doc.go +++ b/pkg/changeset/doc.go @@ -1,7 +1,10 @@ // Package changeset maps a list of changed file paths into the set of // import paths whose packages those files belong to. // -// All operations are pure — no io. Tests are table-driven. +// Operations are deterministic and table-test-friendly. The single io edge +// — a default os.Getwd fallback when WithModuleRoot is not supplied — is +// hooked through an internal function-variable seam so tests can be made +// fully pure by passing WithModuleRoot explicitly. // // API stability: pre-v1.0, package surface may change. See repo README. package changeset diff --git a/pkg/changeset/helpers_test.go b/pkg/changeset/helpers_test.go new file mode 100644 index 0000000..47d4ac7 --- /dev/null +++ b/pkg/changeset/helpers_test.go @@ -0,0 +1,22 @@ +package changeset_test + +// Test fixtures use POSIX-style absolute paths (e.g. "/m/pkga"). CI is +// Linux-only; Windows-portability is documented as out-of-scope in the +// M4 spec's §"Risks & mitigations." + +// stringSlicesEqual compares two []string treating nil and []string{} as +// equivalent. Avoids the reflect.DeepEqual nil-vs-empty asymmetry that +// would otherwise force every "empty result" case to write a literal nil +// even when production code returns nil naturally. Mirrors pkg/depgraph's +// helper of the same name. +func stringSlicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/pkg/changeset/seam_test.go b/pkg/changeset/seam_test.go new file mode 100644 index 0000000..e49dd62 --- /dev/null +++ b/pkg/changeset/seam_test.go @@ -0,0 +1,73 @@ +package changeset + +import ( + "errors" + "testing" + + "github.com/geomyidia/cascade/pkg/golist" +) + +// withCwdSeam swaps the package-level getCwd for the duration of a test, +// restoring the default afterward. Mirrors pkg/golist's withSeam helper. +func withCwdSeam(t *testing.T, fn func() (string, error)) { + t.Helper() + saved := getCwd + t.Cleanup(func() { getCwd = saved }) + getCwd = fn +} + +// TestResolve_DefaultUsesGetCwd covers the os.Getwd-success branch of the +// fallback: when WithModuleRoot is not supplied, Resolve calls getCwd and +// uses its return value as the module root. The seam returns a stub cwd +// so the test outcome doesn't depend on the actual working directory. +func TestResolve_DefaultUsesGetCwd(t *testing.T) { + withCwdSeam(t, func() (string, error) { + return "/stub/cwd", nil + }) + + // Pkg's Dir matches the stub cwd's child; relative file should resolve. + pkgs := []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/stub/cwd/pkga"}, + } + got := Resolve( + []string{"pkga/a.go"}, // relative + pkgs, + // no WithModuleRoot — triggers getCwd fallback + ) + want := []string{"ex/pkga"} + if len(got) != len(want) || got[0] != want[0] { + t.Errorf("Resolve(...) = %v, want %v (Getwd fallback should resolve relative paths)", got, want) + } +} + +// TestResolve_GetCwdErrorTolerated covers the os.Getwd-error branch of the +// fallback: when getCwd returns an error, moduleRoot stays empty and +// relative paths silently fail to resolve (no panic, no error from +// Resolve). Same observable outcome as passing WithModuleRoot(""). +func TestResolve_GetCwdErrorTolerated(t *testing.T) { + withCwdSeam(t, func() (string, error) { + return "", errors.New("getwd failed (synthetic)") + }) + + pkgs := []golist.Package{ + {ImportPath: "ex/pkga", Dir: "/anywhere/pkga"}, + } + got := Resolve( + []string{"pkga/a.go"}, // relative; can't resolve without moduleRoot + pkgs, + ) + if got != nil { + t.Errorf("Resolve(...) = %v, want nil (Getwd error must silently fail relative resolution)", got) + } + + // Absolute paths still work even when getCwd fails — moduleRoot is + // only consulted for relative paths. + got = Resolve( + []string{"/anywhere/pkga/a.go"}, + pkgs, + ) + want := []string{"ex/pkga"} + if len(got) != len(want) || got[0] != want[0] { + t.Errorf("Resolve(...) = %v, want %v (absolute paths should resolve regardless of Getwd)", got, want) + } +} From f78dcb9b2e118dc1dda0d79c5d98632c0cc4de1c Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 11:49:39 -0500 Subject: [PATCH 3/6] m4 retro: closing retrospective with F-1..F-14 row walk Walks F-1..F-14 row-by-row with grep-verifiable evidence. 14 done, 0 deferred, 0 no-op. Documents three disclosed amendments to the spec (Q1 functional-option signature, Q2 os.Getwd fallback with seam, doc.go content update) so the spec's next ODM promotion picks them up. Substrate section enumerates pattern IDs cited during implementation (AP-29/30/36/40/44/52, API-42, IM-01, TD-09/17, TE-01..08/15/43, DC-01/02). Co-Authored-By: Claude Opus 4.7 (1M context) --- ...plementation-retrospective-m4-changeset.md | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 docs/dev/0010-implementation-retrospective-m4-changeset.md diff --git a/docs/dev/0010-implementation-retrospective-m4-changeset.md b/docs/dev/0010-implementation-retrospective-m4-changeset.md new file mode 100644 index 0000000..93d0e73 --- /dev/null +++ b/docs/dev/0010-implementation-retrospective-m4-changeset.md @@ -0,0 +1,128 @@ +# M4 Implementation Retrospective: `pkg/changeset` (Changed-Files-to-Packages Mapping) + +**Status:** closing report; awaiting CDC verification. +**Closing commit:** `75617a2` (head of `m4/changeset-mapping` at close; M4 PR head). Merge OID lands here when rebase-merge to `main` completes. +**Adjacent commit:** `ed64e14` (the implementation plan + design-doc-state transitions, committed to main by the user before the M4 branch was cut). +**CDC verification:** pending. +**Source spec:** [`docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md`](../design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md). Anticipated to transition 05-active → 06-final post-merge. +**Source impl plan:** [`docs/dev/0009-implementation-plan-changed-files-to-packages-mapping.md`](./0009-implementation-plan-changed-files-to-packages-mapping.md). +**Methodology:** [`assets/ai/LEDGER_DISCIPLINE.md`](../../assets/ai/LEDGER_DISCIPLINE.md), [`assets/ai/AI-ENGINEERING-METHODOLOGY.md`](../../assets/ai/AI-ENGINEERING-METHODOLOGY.md). + +## Closure summary + +The spec's acceptance ledger declared **fourteen** rows (F-1..F-14) with a stated **deferral budget of zero**. All fourteen reach a final status of `done`. The exit signal — *"`changeset.Resolve(changedFiles, pkgs, opts...)` returns the import paths of packages whose Go files appear in `changedFiles`, sorted lexicographically; per-package coverage gate at 100% holds; a change-set with a file outside any package returns gracefully (empty or skipped, not an error)"* — is satisfied across all three clauses. + +| Status | Count | +|--------|-------| +| Done | 14 | +| Deferred | 0 | +| No-op | 0 | +| Open at close | 0 | + +The package's exit signal — *"the trio of pure packages (`pkg/golist`, `pkg/depgraph`, `pkg/changeset`) compose without adapter code"* — is observed structurally: `Resolve`'s return type (`[]string` of import paths) is exactly the input shape `depgraph.Graph.RevDepClosure` accepts as seeds. M5 wires the CLI on top with no pending design questions. + +## Disclosed amendments to the spec + +Three deviations from the spec text, all caught at plan-time and recorded for the spec's next ODM promotion: + +1. **F-2 signature drift (positional → functional option).** The spec specified `func Resolve(changedFiles []string, pkgs []golist.Package, moduleRoot string) []string`. User overrode to functional option during plan-mode review (decision Q1): `func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string` with `WithModuleRoot(string) Option`. Same disclosed-amendment pattern as M3's F-20 (Stats type) addition. +2. **`os.Getwd` fallback (Q2 decision).** The spec leaned "skip relative paths when moduleRoot is empty" (b). User chose: fall back to `os.Getwd()` when `WithModuleRoot` is not supplied; tests pass `WithModuleRoot` explicitly to bypass the io. This introduces a single io edge into the package, hooked through an internal function-variable seam (`getCwd`) following M2's `runGoList` pattern. +3. **`doc.go` updated.** The M1 stub said "All operations are pure — no io." That is no longer accurate after Q2. Updated inline to "Operations are deterministic and table-test-friendly. The single io edge — a default os.Getwd fallback when WithModuleRoot is not supplied — is hooked through an internal function-variable seam so tests can be made fully pure by passing WithModuleRoot explicitly." F-1's verify still passes (the `// Package changeset` comment line is preserved). + +These amendments are all forward-looking surface decisions; no behavioural drift from the spec's contractual rules (mapping rules, dedup, sort, no-error contract are all preserved). + +## Substrate loaded at session start + +Per [`assets/ai/AI-ENGINEERING-METHODOLOGY.md`](../../assets/ai/AI-ENGINEERING-METHODOLOGY.md) §"Substrate" and the spec's §"Required reading": + +- [`AI-CONSTITUTION-SUPPLEMENT.md`](../../assets/ai/AI-CONSTITUTION-SUPPLEMENT.md), [`AI-ENGINEERING-METHODOLOGY.md`](../../assets/ai/AI-ENGINEERING-METHODOLOGY.md), [`LEDGER_DISCIPLINE.md`](../../assets/ai/LEDGER_DISCIPLINE.md) — already loaded from the M3/refactor session continuing into this one. +- [`assets/ai/go/SKILL.md`](../../assets/ai/go/SKILL.md) — Critical Rules section and Document Selection Guide refreshed. +- [`assets/ai/go/guides/09-anti-patterns.md`](../../assets/ai/go/guides/09-anti-patterns.md) — walked. M4-relevant pattern IDs cited: **AP-29** (no interface returns — Resolve returns `[]string`, Option is a function-type alias not an interface), **AP-30** (no producer-side interfaces — none exposed), **AP-36** (early-return over deep nesting — `if !ok { return }` style throughout), **AP-40** (slice prealloc — `make([]string, 0, len(seen))` for the result, `make(map[string]string, len(pkgs))` for the dirMap), **AP-44** (no log+return — Resolve has no error return at all), **AP-52** (filepath confinement — informational; M4 doesn't open files but the prefix-check pattern is documented for any future file-content variant). +- [`assets/ai/go/guides/02-api-design.md`](../../assets/ai/go/guides/02-api-design.md) — **API-42** honoured (no globals; everything threaded through `Resolve`'s args + opts; the `getCwd` seam variable is unexported and tests-only). +- [`assets/ai/go/guides/04-type-design.md`](../../assets/ai/go/guides/04-type-design.md) — already loaded; **TD-09** (uniform receivers; not applicable here as `Resolve` is a free function and `Option` carries no receiver), **TD-17** (nil slices treated as empty; `Resolve` returns nil from the no-match path, callers may `range` over the result either way). +- [`assets/ai/go/guides/05-interfaces-methods.md`](../../assets/ai/go/guides/05-interfaces-methods.md) — already loaded; **IM-01** (return concrete; `Resolve` returns `[]string`; `Option` is a concrete function-type alias). +- [`assets/ai/go/guides/07-testing.md`](../../assets/ai/go/guides/07-testing.md) — already loaded; **TE-01..TE-08** (table-driven idiom — every test in M4 is a table-driven loop; `name` field; `t.Run` per case; field-named struct literals; no `t.Fatal` from secondary goroutines), **TE-15** (no assertion library — plain stdlib `testing`, matches existing cascade convention), **TE-43** (`package changeset_test` for public-API tests; `package changeset` only inside `seam_test.go` to access the unexported `getCwd` variable). +- [`assets/ai/go/guides/11-documentation.md`](../../assets/ai/go/guides/11-documentation.md) — already loaded; **DC-01** (every exported name documented: `Resolve`, `Option`, `WithModuleRoot`); **DC-02** (every doc comment starts with the identifier name). + +## Ledger + +| ID | Criterion | Verify (reproducible) | Status | Evidence | +|----|-----------|----------------------|--------|----------| +| F-1 | `pkg/changeset/doc.go` exists with package comment | `test -f pkg/changeset/doc.go && head -3 pkg/changeset/doc.go \| grep -q '^// Package changeset'` | done | M1 stub preserved at the structural level (the `// Package changeset` comment line is intact); content body updated per disclosed amendment #3 above to reflect the `os.Getwd` io edge introduced by Q2. Verify command still exits 0. | +| F-2 | `Resolve` signature matches spec | `go doc github.com/geomyidia/cascade/pkg/changeset.Resolve \| grep -F '...'` | done | Output: `func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string`. Diverges from the spec's positional `moduleRoot string` form — the disclosed amendment #1 above documents the user override (Q1) to functional option. The active spec's ledger F-2 verify text needs updating at next ODM promotion to match. | +| F-3 | All `TestResolve_StandardCases` rows pass | `go test -run 'TestResolve_StandardCases' ./pkg/changeset` | done | `ok github.com/geomyidia/cascade/pkg/changeset 0.263s`. 16 named subtests pass: `empty_changedFiles`, `nil_pkgs`, `single_Go_file_in_pkga`, `two_Go_files_in_pkga_deduped`, `files_in_pkga_and_pkgb_sorted`, `_test.go_in_pkga`, `xtest_dot_go_in_pkga`, `mixed_go_and_non_go`, `Go_file_outside_any_package`, `Go_file_in_subdirectory_of_pkg_dir`, `removed_Go_file_in_pkga`, `relative_path_resolved_against_moduleRoot`, `absolute_path_used_directly`, `path_with_dot_dot_components_cleaned`, `duplicate_entries_in_changedFiles`, `unsorted_entries_yield_sorted_output`. Each subtest also asserts `sort.StringsAreSorted(got)` inline — the determinism criterion enforced per-subtest, not just under -count. | +| F-4 | Hand-traceable 4-package case passes | `go test -run 'TestResolve_HandTraceable' ./pkg/changeset` | done | `ok github.com/geomyidia/cascade/pkg/changeset 0.159s`. Seven sub-cases assert the exact mappings from the spec (pkga→ex/pkga, _test.go→ex/pkga, xtest→ex/pkgb, multi-pkg→sorted, subdir→nil, non-go→nil, empty→nil). | +| F-5 | Path-normalisation cases pass | `go test -run 'TestResolve_PathNormalisation' ./pkg/changeset` | done | `ok github.com/geomyidia/cascade/pkg/changeset 0.149s`. Five OS-portable cases pass: redundant separators, `./` prefix, interior `./`, `..` traversal, absolute-with-extra-separators. `filepath.Clean` normalises all of them. | +| F-6 | `_test.go` files map to the package, not to `_test` ImportPath | `go test -run 'TestResolve_StandardCases/_test\.go_in_pkga' ./pkg/changeset` | done | `--- PASS: TestResolve_StandardCases/_test.go_in_pkga (0.00s)`. Confirms `_test.go` files yield `ex/pkga`, not `ex/pkga_test`. | +| F-7 | Non-Go files silently skipped | `go test -run 'TestResolve_StandardCases/mixed' ./pkg/changeset` | done | `--- PASS: TestResolve_StandardCases/mixed_go_and_non_go (0.00s)`. The case includes `pkga/a.go`, `README.md`, `pkgb/data.json`; only the Go file maps. | +| F-8 | Files outside any package skipped, no error | `go test -run 'TestResolve_StandardCases/Go_file_outside' ./pkg/changeset` | done | `--- PASS: TestResolve_StandardCases/Go_file_outside_any_package (0.00s)`. `cmd/cascade/main.go` (parent dir not in dirMap) yields `nil`. | +| F-9 | Removed files mapped via parent-dir match | `go test -run 'TestResolve_StandardCases/removed_Go_file' ./pkg/changeset` | done | `--- PASS: TestResolve_StandardCases/removed_Go_file_in_pkga (0.00s)`. `pkga/deleted.go` (no `os.Stat`, no existence check) yields `[ex/pkga]` because the parent-dir lookup is purely lexical. | +| F-10 | Output sorted lexicographically and deduplicated | `go test -count=10 -run 'TestResolve' ./pkg/changeset` | done | 10 runs, all pass; `ok github.com/geomyidia/cascade/pkg/changeset 0.154s`. Each subtest also asserts `sort.StringsAreSorted(got)` inline so non-determinism would fail in the failing subtest, not just as a flake under -count=10. | +| F-11 | Per-package coverage gate at 100% on `pkg/changeset` | `bash scripts/coverage-check.sh` | done | `ok: github.com/geomyidia/cascade/pkg/changeset coverage 100% >= 100%`. The `getCwd` seam closes the os.Getwd success/error branch coverage gap; the `TestResolve_PackagesWithEmptyFieldsSkipped` and `TestResolve_EmptyFilePathSkipped` tests close the defensive-skip branches in the dirMap-build and changedFiles loops. All four cascade pure/internal packages now at 100%. | +| F-12 | `pkg/changeset` has no non-stdlib imports beyond `pkg/golist` | `[ "$(go list -m all \| wc -l \| tr -d ' ')" = "1" ]` | done | Output: `1` (the cascade module itself; zero external deps). Imports: `os`, `path/filepath`, `sort`, `strings` from stdlib, plus `github.com/geomyidia/cascade/pkg/golist` from own module. | +| F-13 | `go doc github.com/geomyidia/cascade/pkg/changeset` renders cleanly | `go doc github.com/geomyidia/cascade/pkg/changeset \| head -20` | done | Output (capped at 20 lines per Q6 decision): package overview (3 paragraphs covering the mapping role, the io-edge note, and the API-stability note), then `func Resolve(...)`, `type Option func(*config)`, `func WithModuleRoot(dir string) Option`. Every exported name documented per DC-01; every doc comment starts with the identifier per DC-02. | +| F-14 | Closing report names guides loaded + pattern IDs cited | reviewer reads closing report | done | This document, §"Substrate loaded at session start" enumerates every guide loaded with the specific pattern IDs cited (AP-29/30/36/40/44/52, API-42, TD-09/17, IM-01, TE-01..08/15/43, DC-01/02). | + +## Deferrals + +**Zero deferrals.** The spec's stated deferral budget was zero, and the milestone closes with that property held. + +## What Worked + +Patterns that made M4 close cleanly. Per `LEDGER_DISCIPLINE.md` CDC protocol step 8. + +**The function-variable seam pattern generalised cleanly from M2 to M4.** M2 introduced `runGoList = defaultRunGoList` for the `os/exec` boundary; M4 reuses the exact shape for `getCwd = os.Getwd`. The `withCwdSeam` helper in `pkg/changeset/seam_test.go` is structurally identical to `pkg/golist/seam_test.go`'s `withSeam` — just substitute the type. **This confirms the pattern's reusability:** when a single io edge needs in-process testing without subprocess overhead, the function-variable seam is the textbook structural answer. Future packages with similar "mostly pure plus one io call" shape should reach for the same idiom by default. + +**Plan-mode resolution of open questions surfaced design-affecting answers BEFORE implementation.** The spec author had left six open questions. Three (Q1 API shape, Q2 empty-moduleRoot behaviour, Q5 IgnoredGoFiles) were materially impactful; the other three were minor. Asking the user via `AskUserQuestion` with three bundled multiple-choice questions surfaced two override decisions (Q1 and Q2) that would have been wrong if I'd applied the spec's leans blindly. The user's Q1+Q2 combined answer ("functional option, with `os.Getwd` as default but use option pattern so tests can opt out") was clearly worked-out — the kind of design decision that benefits from the explicit ask, not from CC second-guessing. Pattern: **always ask about open questions in the spec, even when leans are present.** The plan-mode workflow's Phase 3 (`AskUserQuestion`) is the right place for this; the cost is one round-trip and the benefit is a plan that matches user intent. + +**The test-side `_` package convention (TE-43) plus the internal `seam_test.go` carve-out works.** The public-API tests live in `package changeset_test` (external) so they exercise the contract; the seam-driven tests live in `package changeset` (internal, in `seam_test.go`) so they can replace the unexported `getCwd` variable. Both files coexist in the same directory; `go test` runs them together. **The boundary is clean and well-precedented**: M2's `pkg/golist/seam_test.go` follows the same shape. Carry-forward: any future package with a function-variable seam should follow this two-file layout (external `*_test` for contract, internal `seam_test.go` for seam-driven coverage). + +**100% coverage on first try, with no test contortions.** The `pkg/changeset` package hit 100% statement coverage on the first `go test -coverprofile` run. The seam tests close the os.Getwd success/error branches; two small dedicated tests close the defensive-skip branches (`TestResolve_PackagesWithEmptyFieldsSkipped`, `TestResolve_EmptyFilePathSkipped`); the StandardCases table covers everything else. **No "increase coverage by adding speculative branches" anti-pattern was triggered**; every branch in the production code is reachable from natural test inputs. This is what the M3 retro called out as the right shape for pure-data packages with the 100% gate: the gate is honest, not gamed. + +**OUT-1 (M3's lint-cache fix) caught the gofmt issue immediately.** First `make check-all` after writing the tests failed at the gofmt step on `pkg/changeset/changeset_test.go` (struct-literal trailing-comment alignment). `make format` fixed it. **OUT-1 again paid for itself**: the lint surfaced the issue locally rather than after a CI round-trip. Pattern is now battle-tested across three milestones (M3, refactor, M4). + +**Spec-confirmed: `golist.Package.Dir` is absolute.** The M2 inventory (per the explore agent's report) confirmed `pkg/golist` documents `Dir` as absolute, and live `go list -json` output on the sample-module fixture produces `Dir: "/Users/.../sample-module/pkga"` (fully-qualified). M4's algorithm relies on this — comparing `filepath.Dir(absPath)` against `pkg.Dir` works only if both are absolute. **The dependency on M2's contract is explicit, documented, and verified at the substrate level**, not assumed. + +## Pre-PR self-check (CC) + +Per the M2/M3/refactor retro carry-forward: read each row's criterion text against the planned evidence, flag any "pending" / "deferred" / "next round" framings as drift candidates *before* PR open. + +Walked F-1..F-14: +- All fourteen rows have `done` status with reproducible Verify command output captured in the Evidence column above. +- No rows framed as "pending," "deferred," or "next round." +- F-1 has a noted spec-vs-evidence shape difference (the `doc.go` body changed to reflect the io edge), explicitly documented as disclosed amendment #3 above. The Verify command's letter (the `// Package changeset` comment line) is satisfied; the spirit (the M1 stub's content) is honestly amended, not silently drifted. +- F-2 has a noted signature drift (positional → functional option), explicitly documented as disclosed amendment #1. The active spec's ledger F-2 will need updating at next ODM promotion; CC will mention this in the PR description so CDC notices. +- All other rows: criterion text matches evidence text exactly. + +No softpedals identified at self-check. + +## CDC review notes + +_Pending. To be filled in by CDC after independent verification per `LEDGER_DISCIPLINE.md` CDC protocol._ + +## Carry-forward into M5 + +- **Three pure packages compose without adapter code.** The trio (`pkg/golist` + `pkg/depgraph` + `pkg/changeset`) is now functionally complete in-library. M5's CLI wires: + ```go + pkgs, err := golist.Run(ctx, tags, patterns) // M2 + g := depgraph.Build(pkgs) // M3 + seeds := changeset.Resolve(changedFiles, pkgs, opts...) // M4 + affected := g.RevDepClosure(seeds) // M3 + ``` + Each call's output type matches the next call's input shape; no adapter functions are needed. M5 should explicitly verify this at the integration-test layer. + +- **`os.Getwd` fallback semantics in M5 CLI.** When M5 invokes `Resolve`, it should pass `WithModuleRoot(rootDir)` explicitly with the value of `git rev-parse --show-toplevel` — not rely on the `os.Getwd` fallback, since the CLI may be invoked from any cwd. The fallback exists for library consumers who *want* the "use the current directory" convenience; M5 specifically does not. + +- **The seam pattern is now established convention.** Two milestones (M2, M4) have used the function-variable seam for an io edge. The pattern is mature; M5+ packages with io edges should reach for it by default. No further documentation work needed — the existing two-package precedent is sufficient. + +- **Spec amendments need to flow back into 05-active.** Three disclosed amendments in M4 (F-2 signature, Q2 io edge, doc.go content). The active M4 spec at `docs/design/05-active/0005-...md` should be updated post-merge to match — the CC retrospective is the carry-forward record, but the spec is the source-of-truth. ODM promotion (05-active → 06-final) should happen after that update. + +- **CC self-check protocol track record: 4-for-4.** M3, refactor, and now M4 have all applied the pre-PR row audit; zero softpedals caught at audit time across the three. The discipline is settled. M5 will be the first milestone where the audit exists pre-implementation (CLI scope is larger; the chance of a softpedal is non-trivial). + +- **Test-fixture-vs-real-path convention.** M4 tests use POSIX-style absolute paths (`/m/pkga`) explicitly. CI is Linux-only. If/when Windows-portability becomes relevant, M4's tests would need updating. Documented inline in `pkg/changeset/helpers_test.go`. + +## Closure + +Closing report submitted with the M4 PR; CDC verification pending. All fourteen criteria reach a final status of `done`. Zero deferrals. Zero no-ops. Zero open at close. + +Total rows: 14. Done: 14. Deferred: 0. No-op: 0. From 9e92a95859d8e6a6546192554bbda9da9fe8add3 Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 11:56:14 -0500 Subject: [PATCH 4/6] m4 spec: fold plan-mode resolutions into the active spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captures the three disclosed amendments from the M4 retrospective (0010) into the active spec at docs/design/05-active/0005-... so the spec stays the source-of-truth for downstream consumers and CDC review: - Public API surface: Resolve signature reflects the functional- option pattern (Q1 user override). Adds Option type definition and WithModuleRoot constructor as a sibling subsection. Updates Resolve godoc to describe opts, the os.Getwd fallback (Q2), and the IgnoredGoFiles mapping rule (Q5 confirmed). - Decided: bullets updated. moduleRoot via WithModuleRoot, not positional; the seam pattern (getCwd) for the io edge is named. - F-2 ledger row's verify command updated to grep for the functional-option signature. - Open questions section retitled "Open questions (resolved)" with each Q1-Q6 marked **Resolved:** plus the resolution chosen (or spec lean confirmed). Q1 and Q2 record the user override explicitly. - Risks & mitigations: adds an entry for the single io edge from os.Getwd fallback, naming the seam pattern as the mitigation and M2's runGoList as the precedent. - Frontmatter version 1.0 → 1.1. The implementation already matches this amended spec; this commit just brings the spec text in line with what shipped. Nothing in the ledger's done-ness shifts: F-1..F-14 closed in 75617a2 and f78dcb9 remain done. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...geset-changed-files-to-packages-mapping.md | 67 +++++++++++++------ 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md b/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md index 6072e86..d961390 100644 --- a/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md +++ b/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md @@ -9,7 +9,7 @@ updated: 2026-05-07 state: Active supersedes: null superseded-by: null -version: 1.0 +version: 1.1 --- # cascade — M4: `changeset` (Changed-Files-to-Packages Mapping) @@ -54,7 +54,25 @@ Closing report names guides loaded + pattern IDs cited. ## Public API surface -One exported function. Zero exported types. The package is the leanest in the public surface. +One exported function plus an `Option` type and one option constructor. Three exported names total — still the leanest in the public surface. + +### `Option` and `WithModuleRoot` + +```go +package changeset + +// Option configures a Resolve call. Apply via Resolve's variadic parameter. +type Option func(*config) + +// WithModuleRoot sets the module root used to resolve relative entries in +// changedFiles. When supplied, Resolve uses dir directly without consulting +// the filesystem. +// +// 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. +func WithModuleRoot(dir string) Option +``` ### `Resolve` @@ -67,15 +85,16 @@ package changeset // // changedFiles are paths to Go files that have been modified, added, renamed, // or removed (typically `git diff --name-only` output, one per line). Paths -// may be relative (interpreted relative to moduleRoot) or absolute; the -// function normalises before lookup. +// may be relative (resolved against moduleRoot) or absolute; filepath.Clean +// is applied before lookup. // // pkgs is the package list returned by golist.Run; each Package's Dir field -// (absolute path) plus its file lists drive the lookup. +// (absolute path) drives the lookup. // -// moduleRoot is the absolute path of the module's repo root, used to resolve -// relative entries in changedFiles. Typically `git rev-parse --show-toplevel` -// in the caller (M5 CLI). +// opts configure the call. The only option in v0.x is WithModuleRoot; if not +// supplied, Resolve falls back to os.Getwd at call time. Typically M5's CLI +// passes WithModuleRoot(rootDir) with the value of `git rev-parse +// --show-toplevel`. // // Mapping rules: // - A file ending in ".go" whose parent directory exactly matches some @@ -86,6 +105,9 @@ package changeset // directory. Internal-test (package foo) and external-test (package // foo_test) both yield foo's ImportPath, because affecting either re-runs // foo's tests. +// - Files in a package's IgnoredGoFiles (build-tag-excluded) map to the +// package — the rule is parent-dir match; file-list membership is +// irrelevant to the lookup. // - Non-Go files (no .go extension) are silently skipped. // - Files in subdirectories of a package's Dir (e.g., testdata/) are // skipped, because the parent directory does not match any package's Dir. @@ -94,14 +116,15 @@ package changeset // // Determinism: identical inputs yield identical output across runs. The // returned slice is sorted; duplicates are deduplicated. -func Resolve(changedFiles []string, pkgs []golist.Package, moduleRoot string) []string +func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string ``` **Decided:** -- Single exported function. No `Resolver` type, no functional options, no error return. The contract fits in a function signature plus a doc comment. -- `moduleRoot` is positional rather than via a functional option. Adding `Option`/`WithModuleRoot` for one option is over-engineering at this surface size; a positional param keeps the call site one line. +- Three exported names: `Resolve`, `Option`, and `WithModuleRoot`. The contract fits in two function signatures plus an option-pattern config. +- `moduleRoot` is supplied via `WithModuleRoot(dir string) Option`, not as a positional parameter. The functional-option pattern future-proofs for additional knobs (e.g. `WithIgnorePattern`) and lets tests opt out of the `os.Getwd` fallback so they remain pure (no io). Plan-mode resolution of Q1. +- The single io edge (`os.Getwd` fallback when `WithModuleRoot` is not supplied) is hooked through an internal function-variable seam (`getCwd`) following M2's `runGoList` pattern. Tests in `seam_test.go` (`package changeset` internal) replace it to drive the success/error branches without depending on the actual test cwd. Plan-mode resolution of Q2. - Return type is `[]string`, not a named `[]ImportPath` type. Consistent with M3's decision to use `string` for import paths throughout. -- No error return. Every input shape is handled gracefully — out-of-package files, non-Go files, removed files, nil/empty inputs all produce sensible output. +- No error return. Every input shape is handled gracefully — out-of-package files, non-Go files, removed files, nil/empty inputs, and `os.Getwd` failures all produce sensible output. **Not exposed in M4** (deliberately deferred): - A `WithIgnore(pattern)` option to skip files matching some glob. Speculative; no current need. @@ -196,7 +219,7 @@ Per [`assets/ai/LEDGER_DISCIPLINE.md`](../../../assets/ai/LEDGER_DISCIPLINE.md). | ID | Criterion | Verify (reproducible) | Significance | Status | Evidence | |----|-----------|----------------------|--------------|--------|----------| | F-1 | `pkg/changeset/doc.go` exists with package comment | `test -f pkg/changeset/doc.go && head -3 pkg/changeset/doc.go \| grep -q '^// Package changeset'` | serious | open | preserved from M1 stub | -| F-2 | `Resolve` signature matches spec | `go doc github.com/geomyidia/cascade/pkg/changeset.Resolve \| grep -F 'func Resolve(changedFiles []string, pkgs []golist.Package, moduleRoot string) []string'` | serious | open | | +| F-2 | `Resolve` signature matches spec | `go doc github.com/geomyidia/cascade/pkg/changeset.Resolve \| grep -F 'func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string'` | serious | open | | | F-3 | All `TestResolve_StandardCases` rows pass | `go test -run 'TestResolve_StandardCases' ./pkg/changeset` | serious | open | enumerated cases | | F-4 | Hand-traceable 4-package case passes | `go test -run 'TestResolve_HandTraceable' ./pkg/changeset` | serious | open | spec exit criterion | | F-5 | Path-normalisation cases pass | `go test -run 'TestResolve_PathNormalisation' ./pkg/changeset` | serious | open | OS-portable | @@ -231,21 +254,23 @@ Per [`assets/ai/LEDGER_DISCIPLINE.md`](../../../assets/ai/LEDGER_DISCIPLINE.md). **`changedFiles` contains paths that have been *renamed*.** `git diff --name-only` may emit both the old and new path as separate entries (or a single rename indicator depending on flags). `Resolve` handles each entry independently — no special-casing needed. Document. -## Open questions +**Single io edge from `os.Getwd` fallback.** Per the Q2 resolution, the package introduces one io call when `WithModuleRoot` is not supplied. Mitigation: hook through an internal function-variable seam (`getCwd = os.Getwd`) so seam_test.go can replace it; structurally identical to M2's `runGoList` seam. Tests bypass the io entirely by passing `WithModuleRoot` explicitly; production callers (M5 CLI) should also pass `WithModuleRoot` with the result of `git rev-parse --show-toplevel` rather than relying on the fallback. + +## Open questions (resolved) -These are the calibrations the maintainer should weigh in on before CC starts implementation. +All six questions resolved during plan-mode review prior to M4 implementation. Recorded here for historical context; the decisions are now reflected in the API surface and ledger above. -1. **`moduleRoot` as positional parameter, or via functional option?** Specced as positional for API simplicity (one exported function, one config knob, no `Option` type to define). Functional options would future-proof for additional knobs (`WithIgnorePattern`, `WithFollowSymlinks`, etc.) but introduce surface area for hypothetical needs. Lean: positional. Confirm or override. +1. **`moduleRoot` as positional parameter, or via functional option?** **Resolved:** functional option. The maintainer overrode the spec's "positional" lean: the option-pattern future-proofs for additional knobs (e.g. `WithIgnorePattern`) and, more importantly here, lets tests opt out of the `os.Getwd` fallback (Q2) by passing `WithModuleRoot` explicitly so they remain pure (no io). The cost — one extra `Option` type plus one constructor — is small relative to the testability gain. -2. **Behaviour when `moduleRoot == ""` and `changedFiles` contains relative paths.** Two options: (a) treat empty `moduleRoot` as "use `os.Getwd()`" — convenience, mild magic; (b) require a non-empty `moduleRoot` when any path is relative — strict, predictable. Lean: (b), with a clear godoc note. Empty `moduleRoot` works fine when all `changedFiles` are absolute. Confirm. +2. **Behaviour when `moduleRoot == ""` and `changedFiles` contains relative paths.** **Resolved:** option (a) modified — when `WithModuleRoot` is *not supplied*, fall back to `os.Getwd` at call time; when `WithModuleRoot` is supplied (even with an empty string), use that value as-is. Tests bypass the `os.Getwd` io by always passing `WithModuleRoot`. The fallback is hooked through an internal function-variable seam (`getCwd`) so the success/error branches of `os.Getwd` are testable in-process without depending on the actual cwd. Pattern matches M2's `runGoList` seam. -3. **Should `Resolve` deduplicate across distinct lexical forms of the same path?** E.g., `pkga/a.go` and `./pkga/a.go` and `pkga/./a.go` after `filepath.Clean` all become `pkga/a.go`; deduplication happens automatically in the result-set. But what about `pkga/a.go` (relative, resolved against moduleRoot=`/m`) and `/m/pkga/a.go` (already absolute)? Both resolve to the same package and the result-set dedupes. Confirm this is the desired behaviour (not "treat each lexical form as a distinct change"). +3. **Should `Resolve` deduplicate across distinct lexical forms of the same path?** **Resolved:** yes — followed the spec's lean. `filepath.Clean` is applied before lookup; the result-set deduplicates via `map[string]struct{}{}`. Distinct lexical forms collapse naturally. -4. **Test fixtures: synthetic-only, or include a small repo fixture under `pkg/changeset/testdata/`?** Specced as synthetic-only because the function is pure-data and doesn't touch the filesystem. A repo fixture would only matter if the function followed symlinks or read file metadata, which it doesn't. Lean: synthetic-only. Confirm. +4. **Test fixtures: synthetic-only, or include a small repo fixture under `pkg/changeset/testdata/`?** **Resolved:** synthetic-only — followed the spec's lean. The function is path-only; no filesystem reads. Tests use POSIX-style absolute paths (`/m/pkga`) directly; CI is Linux-only and Windows portability is documented as out-of-scope. -5. **`IgnoredGoFiles` handling.** Currently the spec maps all `.go`-extension files in a package's Dir to that package, regardless of whether they're in `GoFiles` or `IgnoredGoFiles`. Argument for: tag-excluded files still belong to the package; if their tag becomes active in the build-tag union, they affect the package. Argument against: under the current tag union, an `IgnoredGoFiles` entry is irrelevant. Lean: map regardless (simpler semantics; edges erring on the side of "more re-runs, not fewer" is the safer side for cascade's affected-set-for-CI use case). Confirm. +5. **`IgnoredGoFiles` handling.** **Resolved:** map regardless — followed the spec's lean and confirmed by the maintainer. The mapping rule is "parent-dir match"; file-list membership is irrelevant to the lookup. A `.go` file in a package's `Dir` maps to that package whether it's in `GoFiles`, `TestGoFiles`, `XTestGoFiles`, `IgnoredGoFiles`, or none of them. -6. **Closing-report evidence for F-13 (godoc rendering).** Should the closing report include the full `go doc` output for the package, or a summary line plus a one-paragraph judgment that "all exported names are documented per DC-01/DC-02"? The latter is lighter-weight for reviewers; the former is stronger evidence. Lean: full output, capped at first 20 lines (consistent with the verify command). Confirm. +6. **Closing-report evidence for F-13 (godoc rendering).** **Resolved:** full `go doc` output capped at first 20 lines — followed the spec's lean. Consistent with the verify command. ## Cross-references From e17a7301a141646b81eae625d5c590e6a62a1aa0 Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 11:57:44 -0500 Subject: [PATCH 5/6] =?UTF-8?q?m4=20spec:=20add=20Version=20History=20sect?= =?UTF-8?q?ion=20for=201.0=20=E2=86=92=201.1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous spec-amendment commit (9e92a95) bumped the frontmatter version from 1.0 to 1.1 but didn't document what changed in-doc. Adding a Version History table at the bottom of the spec capturing: - 1.0 baseline (positional Resolve, no exported types, no io) - 1.1 amendments (Option + WithModuleRoot, os.Getwd fallback with getCwd seam, IgnoredGoFiles mapping rule, F-2 verify command, resolved open questions, risks-section addition) Per spec-keeping discipline (assets/ai/AI-ENGINEERING-METHODOLOGY.md §"Anti-degradation practices"): a version bump without a documented change is silent drift; the table makes the diff visible to future readers and to CDC. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...scade-m4-changeset-changed-files-to-packages-mapping.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md b/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md index d961390..0640491 100644 --- a/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md +++ b/docs/design/05-active/0005-cascade-m4-changeset-changed-files-to-packages-mapping.md @@ -272,6 +272,13 @@ All six questions resolved during plan-mode review prior to M4 implementation. R 6. **Closing-report evidence for F-13 (godoc rendering).** **Resolved:** full `go doc` output capped at first 20 lines — followed the spec's lean. Consistent with the verify command. +## Version history + +| Version | Date | Author | Change | +|---------|------|--------|--------| +| 1.0 | 2026-05-07 | Duncan McGreggor (with CC) | Initial active spec. Specced `Resolve` as a single positional-parameter function with no exported types and no io. Six open questions left for plan-mode review. | +| 1.1 | 2026-05-07 | Duncan McGreggor (with CC) | Folded the M4 plan-mode resolutions into the spec body. Public API surface gains an `Option` type and a `WithModuleRoot` constructor (Q1 user override of the positional lean). `Resolve` signature changes to `(changedFiles []string, pkgs []golist.Package, opts ...Option) []string`. Q2 resolution introduces a single `os.Getwd` fallback when `WithModuleRoot` is not supplied, hooked through an internal `getCwd` function-variable seam (mirrors M2's `runGoList` pattern). Q5 mapping rule for `IgnoredGoFiles` clarified as "map regardless" (followed spec lean, confirmed by maintainer). F-2 ledger row's verify command updated to grep for the functional-option signature. Open questions section retitled "Open questions (resolved)" with each Q1-Q6 marked resolved. Risks section gains an entry for the `os.Getwd` io edge with the seam pattern as the named mitigation. No behavioural drift from 1.0's contractual rules — same mapping rules, same dedup, same sort, same no-error contract. Implementation in commits `75617a2` (M4 implementation) and `f78dcb9` (M4 closing retrospective); spec amendment in commit `9e92a95`. | + ## Cross-references - Parent plan: [`0001-cascade-high-level-project-plan.md`](./0001-cascade-high-level-project-plan.md), §"M4 — Changed-files-to-packages mapping". From 207c5489f404e39c256d4dd8c81659b7aed0cdeb Mon Sep 17 00:00:00 2001 From: Duncan McGreggor Date: Thu, 7 May 2026 12:08:54 -0500 Subject: [PATCH 6/6] m4 retro: fold in CDC review notes (M4 verified mergeable) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CDC pass run against m4/changeset-mapping head 75617a2 + retro fix-up f78dcb9. Replaces the "Pending..." placeholder in the §"CDC review notes" section with the full independent-verification report: - Direct re-reads on F-1 (doc.go), F-2 (Resolve signature), F-9 (removed-file lookup is purely lexical), F-12 (imports list + zero go.mod requires), F-14 (substrate enumeration). - CC-attested-via-CI rows (F-3..F-8, F-10, F-11, F-13) accepted on the strength of CI green + local-run evidence in the row walk. - Specifically verified the options pattern (config struct, WithModuleRoot setting both moduleRoot + moduleRootSet, the meaningful "not set vs explicitly empty" distinction the flag enables) and the seam-based pure-testing path (every test call site in changeset_test.go passes WithModuleRoot; TestResolve_DefaultUsesGetCwd + TestResolve_GetCwdErrorTolerated exercise both getCwd branches via withCwdSeam). CDC findings: zero softpedals, zero silent drops, row count 14-declared / 14-closed. Closure recommendation: M4 is mergeable. Three engineering observations carried forward into the retros' What-Worked corpus: (a) the moduleRootSet flag as an API-design pattern (option zero value semantically meaningful); (b) the plan-mode AskUserQuestion round-trip surfacing design-affecting decisions before code is written; (c) the external+internal two-file test convention now established across pkg/golist and pkg/changeset. Forward-looking note: the active M4 spec's F-2 verify text references the original positional moduleRoot signature; the in-PR amendment commit (9e92a95) already updates this to the functional-option form, so the spec is in-sync at v1.1. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...plementation-retrospective-m4-changeset.md | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/docs/dev/0010-implementation-retrospective-m4-changeset.md b/docs/dev/0010-implementation-retrospective-m4-changeset.md index 93d0e73..d91d57a 100644 --- a/docs/dev/0010-implementation-retrospective-m4-changeset.md +++ b/docs/dev/0010-implementation-retrospective-m4-changeset.md @@ -98,7 +98,46 @@ No softpedals identified at self-check. ## CDC review notes -_Pending. To be filled in by CDC after independent verification per `LEDGER_DISCIPLINE.md` CDC protocol._ +CDC pass run against `m4/changeset-mapping` head `75617a2` plus the retro fix-up `f78dcb9`. Sandbox has no `go` toolchain so the toolchain rows (F-3..F-13's test-passes, F-13's `go doc` rendering) stay CC-attested via local + CI green; structural rows reproducible by direct read are re-checked. + +**Specifically verified per the user's pre-CDC asks:** + +The options pattern is correctly implemented. `Option` is a function-type alias `func(*config)` (changeset.go:14), `config` is unexported with `moduleRoot` + `moduleRootSet` fields (lines 18–21), and `WithModuleRoot` is a closure-returning constructor that sets *both* fields (lines 35–40). The `moduleRootSet` flag is a meaningful design touch — it distinguishes "user passed `WithModuleRoot("")`" (treat as explicitly empty; relative paths silently fail) from "user passed nothing" (fall back to `os.Getwd`). This addresses the open question I raised in the M4 spec's Q2 ("behaviour when `moduleRoot == ""` and `changedFiles` contains relative paths") more precisely than my own design lean did, and the doc comment at lines 31–34 names the distinction explicitly. Above-spec design quality. + +Pure functional use in testing is achievable through two independent mechanisms, both verified directly: + +- **Pass `WithModuleRoot(...)` explicitly** — every test in `changeset_test.go` does this, so the os.Getwd path is never reached and tests are fully deterministic on path inputs only. Verified at every `Resolve(...)` call site in the table-driven cases. +- **Replace `getCwd` via the function-variable seam** — `var getCwd = os.Getwd` (changeset.go:46) is replaceable by tests through the `withCwdSeam(t, fn)` helper (seam_test.go:12–17). `TestResolve_DefaultUsesGetCwd` exercises the success branch with a stub returning `/stub/cwd`; `TestResolve_GetCwdErrorTolerated` exercises the error branch with a synthetic error AND verifies that absolute paths still resolve when the seam returns an error. Both branches give pure, deterministic test outcomes that don't depend on the actual cwd at test-run time. + +The seam pattern is structurally identical to M2's `runGoList` (both are `var seam = realImpl` package-level function variables, replaceable by tests, restored via `t.Cleanup`). The reusability claim from the M4 retro's "What Worked" section holds. + +**Verified directly by CDC (per-row reproduction):** + +- **F-1** (doc.go updated, package comment preserved): direct read of `pkg/changeset/doc.go` confirms `// Package changeset` line at top; body content updated per disclosed amendment #3 to describe the io edge + seam. +- **F-2** (Resolve signature): direct read of changeset.go:96 confirms `func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []string` — matches the *amended* spec (functional option), diverges from the *original* spec text (positional). The disclosed amendment is clean and the active spec's F-2 verify text needs updating at next ODM promotion, as the retro flags. +- **F-9** (removed-file mapping): direct read of changeset.go:138–141 confirms the parent-dir lookup is purely lexical (`filepath.Dir(abs)` looked up in `dirMap`) — no file-existence check anywhere, so removed Go files map correctly. +- **F-12** (no non-stdlib imports beyond own module): direct read of changeset.go:3–10 confirms imports are `os`, `path/filepath`, `sort`, `strings` from stdlib + `pkg/golist` from own module. `go.mod` has zero `require` entries, so `go list -m all | wc -l` yields 1. +- **F-14** (closing report attestation): the substrate-loaded section above enumerates seven guides with pattern IDs cited (AP-29/30/36/40/44/52, API-42, TD-09/17, IM-01, TE-01..08/15/43, DC-01/02). That's the load-bearing artifact for this row. + +**CC-attested via CI green / local run (not re-runnable in sandbox):** + +F-3 (16-case StandardCases pass), F-4 (HandTraceable 7 cases), F-5 (PathNormalisation 5 cases), F-6 (_test.go mapping subtest), F-7 (mixed go/non-go subtest), F-8 (Go-file-outside subtest), F-10 (-count=10 determinism), F-11 (per-package coverage gate at 100%), F-13 (`go doc` rendering). All called out with `ok ...0.Xs` evidence in the retro's row walk. The "100% coverage on first try, with no test contortions" claim in §"What Worked" is consistent with the structural read: every branch in the production code (the four early-skip branches + the two map-build defensive skips + the two getCwd branches) has a named test. + +**No softpedals identified.** The pre-PR self-check walked all fourteen rows and named the two evidence-vs-criterion shape differences (F-1 doc.go body change, F-2 signature drift) as disclosed amendments rather than soft-passing. CDC's independent re-read finds the same conclusion: both differences are honest amendments traced to plan-mode user decisions, both flagged for the spec's next ODM promotion. + +**No silent drops.** Spec ledger declared 14 rows, all 14 present in the retro's ledger walk, all 14 closed. Row count check: 14 declared / 14 closed. + +**Substrate enumeration is consistent with the M3 retro's pattern.** The cited Go-guide pattern IDs (AP-* + API-42 + TD-09/17 + IM-01 + TE-* + DC-*) are concrete and traceable. The retro names AP-52 (filepath confinement) as "informational; M4 doesn't open files but the prefix-check pattern is documented for any future file-content variant" — that's the right kind of forward-looking citation: load-bearing-now-or-relevant-later, named not silent. + +**Engineering observations worth carrying forward to retros' What-Worked corpus (in addition to CC's retro entries):** + +- The `moduleRootSet` flag — making "not set" distinguishable from "explicitly empty" — is a worth-recording API-design pattern. Functional options in Go often miss this distinction; M4's implementation captures it cleanly. Reusable when an option's zero value is semantically meaningful. +- The plan-mode `AskUserQuestion` round-trip on the open spec questions (Q1 + Q2) directly produced this design. The retro's §"What Worked" already names this pattern; CDC affirms it as a method that surfaces design-affecting decisions before code is written, when the cost of a design wrong-turn is highest. +- The `package changeset_test` (external) + `package changeset` in `seam_test.go` (internal) two-file convention is now established across two packages (`pkg/golist`, `pkg/changeset`). Future packages with internal seams should follow this layout by default. + +**Closure recommendation: M4 is mergeable.** All 14 ledger rows have evidence of the right shape. The three disclosed amendments are clean. The substrate enumeration meets F-14. The options pattern is correctly implemented and the seam-based pure-testing path is verified directly. The trio of pure packages (`pkg/golist`, `pkg/depgraph`, `pkg/changeset`) compose without adapter code, as the retro's §"Closure summary" claims — `Resolve`'s `[]string` return matches `RevDepClosure`'s `seeds []string` parameter exactly. + +**Forward-looking note (carry-forward, not a finding for M4):** the active M4 spec's F-2 verify text references the original positional `moduleRoot string` signature; should be updated at next ODM promotion to match the functional-option form actually shipped. The retro flags this; surfacing it again here so it doesn't get lost in the M5 ramp-up. ## Carry-forward into M5