diff --git a/CLAUDE.md b/CLAUDE.md index 4673eb9..2127e17 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,6 +91,8 @@ Algorithmic packages. **No io, no syscalls, no `os/exec`** between them. Inputs These three packages are **gated at 100% statement coverage in CI** by `scripts/coverage-check.sh`. The gate skips packages with no coverage data ("N/A"), so empty pre-implementation stubs don't fire it; once a package gets code, the gate fires immediately. +**Layout deviation note (PS-06).** The substrate's `assets/ai/go/guides/10-project-structure.md` flags `pkg/` as `SHOULD-AVOID` — a community convention rather than a Uber/Google rule, but one the Go standard library and most idiomatic projects don't follow. Cascade adopts `pkg/` deliberately: it makes the public OSS surface explicit, pairs cleanly with `internal/`'s language-enforced privacy boundary, and keeps the substrate of "what's importable by downstreams" obvious at a glance. The deviation is acknowledged in the M2-M5 audit (`docs/dev/0014-go-quality-audit.md` finding F-12) as **Acknowledged-with-rationale**. **Carry-forward:** revisit at v1.0 if a flatter layout becomes feasible without breaking downstream consumers' import paths. + ### Private support — module-internal (`internal/project/`) Build-metadata package; carries cascade-specific Version / GitCommit / GitBranch / BuildDate values populated via `-ldflags` at link time, with a `runtime/debug.ReadBuildInfo` fallback for `go install`-built binaries. The pattern is generic but the values are cascade-specific; Go's `internal/` rule structurally enforces that downstream consumers cannot import this package. Also gated at 100% coverage by `scripts/coverage-check.sh`. diff --git a/docs/dev/0014-go-quality-audit.md b/docs/dev/0014-go-quality-audit.md index a4f9121..5c15ae4 100644 --- a/docs/dev/0014-go-quality-audit.md +++ b/docs/dev/0014-go-quality-audit.md @@ -340,6 +340,36 @@ None of this blocks M6. The audit's findings are quality-of-life improvements an Five-for-five on the CC self-check protocol catching zero softpedals across milestones; six-for-six now if you count this audit's discipline (the auditor independently re-derived findings from substrate, didn't re-litigate retro-disclosed deviations, surfaced cite drift back at the prompt). The methodology is paying compound interest. +## Post-audit closure walk (per-finding disposition) + +Disposition for each finding per the work order in [`docs/dev/0015-audit-follow-up-plan-address-all-16-findings.md`](./0015-audit-follow-up-plan-address-all-16-findings.md). Each row names the resolution path and the post-fix file:line evidence so CDC can re-verify against the working tree. + +| ID | Disposition | Resolution + post-fix evidence | +|----------|-------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| F-1 | **closed in PR-A** | `pkg/golist/errors.go:16` now reads `// against Error() output is forbidden (AP-13).` ✓ | +| F-2 | **closed in PR-A** | `pkg/golist/golist.go:204-211` rewritten: cite changed from `EH-08` to `CC-08` and rationale strengthened to make the post-hoc-vs-propagation distinction explicit (also closes F-4 for this site). ✓ | +| F-3 | **closed in PR-A** | `internal/cli/seam.go:62-69` rewritten: cite changed from `EH-08` to `CC-08` and rationale mirrors the strengthened pkg/golist version (also closes F-4 for this site). ✓ | +| F-4 | **closed in PR-A** (rationale-strengthen) | The "post-hoc consultation, not propagation" rationale now appears in both classifier comments with an explicit CC-08 cite and the cousin-shape lock-in note. PR-C (substrate-align by moving ctx to first param) declined per 0015 work-order recommendation. | +| F-5 | **deferred to PR-B** | Add `Dir string` field to `*GitDiffError`; populate from `cfg.root` at `internal/cli/cli.go:270`; update `internal/cli/seam_test.go` test to assert the new field. | +| F-6 | **closed in PR-A** | `// Parallel-unsafe:` comment block added at the top of `internal/cli/seam_test.go`, `pkg/golist/seam_test.go`, `pkg/changeset/seam_test.go`, and `internal/project/version_test.go` (each names which package-level vars the file mutates and points to S-2). ✓ | +| F-7 | **closed in PR-A** (decline-with-cross-reference) | `internal/cli/cli_test.go:20-28` carries a 4-line comment cross-referencing `internal/project/version_test.go`'s `withMetadata` helper as the canonical pattern, plus a parallel-unsafe note. ✓ | +| F-8 | **closed in PR-A** | `internal/cli/helpers_test.go` deleted; `writeFile` in `internal/cli/cli_test.go` inlines the body (`return os.WriteFile(path, []byte(content), 0o600)`); `os` import added to `cli_test.go`. ✓ | +| F-9 | **closed in PR-A** | `pkg/golist/golist_test.go:99-122` now uses a `[]struct{ name string; patterns []string }` table with rows `{"nil", nil}` and `{"empty", []string{}}` — the inline-map subtest-name lookup is gone. ✓ | +| F-10 | **closed in PR-A** | `pkg/changeset/changeset.go:65-75` doc rewritten to describe the actual post-bug-#12 behaviour: getCwd-error leaves `moduleRoot` empty at the fallback step, but `filepath.Abs("")` resolves it to the cwd anyway. The doc now matches the code. ✓ | +| F-11 | **closed in PR-A** | `pkg/changeset/changeset.go:113-118` internal comment rewritten to match the corrected doc. The "leave as-is" claim is replaced with "the empty value is replaced by the process cwd here, not 'left as-is.'" ✓ | +| F-12 | **closed in PR-A** (rationale + carry-forward) | `CLAUDE.md`'s Architecture / "Pure core" subsection grew a "Layout deviation note (PS-06)" paragraph stating the deviation is acknowledged-with-rationale and naming v1.0 as the carry-forward window. The deviation itself remains accepted. ✓ | +| F-13 | **closed in PR-A** | `pkg/golist/golist.go` block at former lines 238-241 collapsed: the `_ = io.EOF` placeholder and its justification comment are removed; the surviving block keeps the three useful compile-time assertions for `*ExitError`, `*ParseError`, and `Option`. ✓ | +| F-14 | **deferred to PR-B** | Return argv from the git-diff seam (extend `gitDiffResult` with `argv []string`); drop the synthetic argv construction at `internal/cli/cli.go:269`; update tests. | +| F-15 | **closed in PR-A** (decline + document) | `pkg/changeset/changeset.go:18-29` config struct's `moduleRootSet` field now carries a doc comment explaining why the flag exists post-bug-#12 (test-driven design choice for the os.Getwd-error fallback path). The flag itself stays. ✓ | +| F-16 | **closed in PR-A** (rationale-strengthen) | `internal/project/version.go:54-71` var-block doc grew an explicit "AP-07 deviation (acknowledged)" paragraph naming the link-time-vs-runtime distinction, the kubectl/hugo/cobra precedent, and the no-`t.Parallel()` test discipline (paired with F-6 / S-2). ✓ | +| PROMPT-1 | **closed in PR-A** | `workbench/cc-audit-prompt-go-quality.md`'s "Receiver consistency" line now reads `IM-12 / TD-09` (was `TD-09 / IM-04`). The line carries a parenthetical pointing future-runners at the M3-retro / 0015 lineage so the drift's history is traceable. ✓ | + +**S-1 (citation drift): closed.** All three concrete instances are corrected (F-1, F-2, F-3) plus the meta-instance (PROMPT-1). The systemic recommendation (a write-time discipline or pre-commit hook) remains a future-work item; logged but unscheduled. + +**S-2 (seam pattern / no-`t.Parallel()` discipline): closed in PR-A** at the documentation layer per 0015's plan. The structural enforcement option (custom `go vet`-style linter) is logged as v1.0+ future work. + +**Net post-PR-A state:** 14 of 16 findings closed (plus PROMPT-1). 2 deferred to PR-B (F-5, F-14, both CONSIDER). 0 still open at exit. The `make check` bundle (build + lint + test, with `-race -count=1`) passes against the PR-A working tree. + ## Closure Audit run complete. Total findings: 16. Open: 13. Acknowledged-with-rationale: 3 (F-4 ctx-as-last-param; F-12 `pkg/` layout; F-16 mutable build-info vars). CDC review **complete** — every spot-checked claim reproduces against the cited file:line; severity assignments calibrated; methodology adherence exemplary; closure recommendation: ready to consume, follow-up PRs grouped above. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 90730db..a238c86 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -2,6 +2,7 @@ package cli_test import ( "bytes" + "os" "path/filepath" "strings" "testing" @@ -18,6 +19,11 @@ import ( // prints the project metadata to stdout and exits 0. Mirrors the M1 // in-process version test that previously lived in cmd/cascade/main_test.go. func TestRun_Version(t *testing.T) { + // The save-restore dance below mirrors internal/project/version_test.go's + // withMetadata helper; we reproduce it inline here because withMetadata + // is unexported and adding a public test-only API just for one consumer + // is not worth the surface. See F-7 in docs/dev/0014-go-quality-audit.md. + // Parallel-unsafe (mutates package-level project.* vars; cousin of S-2). saveVersion, saveCommit, saveBranch, saveSummary, saveDate := project.Version, project.GitCommit, project.GitBranch, project.GitSummary, project.BuildDate t.Cleanup(func() { @@ -203,5 +209,5 @@ func TestRun_FileChangedFiles_OpenFailure(t *testing.T) { // writeFile is a tiny test helper that writes content to path. func writeFile(path, content string) error { - return writeAll(path, []byte(content)) + return os.WriteFile(path, []byte(content), 0o600) } diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go deleted file mode 100644 index 504e0a2..0000000 --- a/internal/cli/helpers_test.go +++ /dev/null @@ -1,8 +0,0 @@ -package cli_test - -import "os" - -// writeAll writes data to path, creating or truncating. Test-only helper. -func writeAll(path string, data []byte) error { - return os.WriteFile(path, data, 0o600) -} diff --git a/internal/cli/seam.go b/internal/cli/seam.go index c56a163..03a3d77 100644 --- a/internal/cli/seam.go +++ b/internal/cli/seam.go @@ -59,9 +59,13 @@ func defaultRunGitDiff(ctx context.Context, base, head, dir string) gitDiffResul // - other exec errors (rare; e.g. permission denied on git binary, or git // not on PATH): wrapped under ErrGitDiffFailed for category clarity. // -// ctx is the last param (against EH-08 ordering convention) because it's only -// used to detect cancellation post-hoc, not for propagation. Mirrors -// pkg/golist's classifyRunError. +// ctx is the last param, deliberately deviating from CC-08 (ctx is the +// first parameter). The CC-08 MUST is calibrated to *propagation* — a +// function that passes ctx forward into a blocking call. This classifier +// consults ctx.Err() exactly once, post-hoc, to disambiguate "is this +// error a cancellation?" and never propagates ctx anywhere else; that is +// outside CC-08's intended scope. Mirrors pkg/golist's classifyRunError so +// readers who learn one know the other. func classifyGitDiffError(err error, argv []string, _, stderr string, ctx context.Context) error { if ctxErr := ctx.Err(); ctxErr != nil { return fmt.Errorf("git diff: %w", ctxErr) diff --git a/internal/cli/seam_test.go b/internal/cli/seam_test.go index 15a0799..45ee862 100644 --- a/internal/cli/seam_test.go +++ b/internal/cli/seam_test.go @@ -15,6 +15,10 @@ import ( "github.com/geomyidia/cascade/pkg/golist" ) +// Parallel-unsafe: tests in this file mutate package-level seam variables +// (runGitDiff, runGoListWrapper, signalContext). Do not call t.Parallel() +// in any test that exercises them. See S-2 in docs/dev/0014-go-quality-audit.md. + // withGitDiffSeam swaps the package-level runGitDiff for the duration of a // test, restoring the default afterward. Mirrors pkg/golist's withSeam. func withGitDiffSeam(t *testing.T, fn func(ctx context.Context, base, head, dir string) gitDiffResult) { diff --git a/internal/project/version.go b/internal/project/version.go index 242c226..d1b066c 100644 --- a/internal/project/version.go +++ b/internal/project/version.go @@ -51,6 +51,21 @@ var versionFile string // (GitCommit, BuildDate). GitBranch and GitSummary stay empty under the // fallback path — Go's build-info doesn't carry branch names, and a // synthesised git-describe could diverge from the Makefile representation. +// +// AP-07 deviation (acknowledged): these are exported package-level +// **mutable** vars. The substrate's AP-07 ("Mutable Package-Level +// Globals") is SHOULD-AVOID. Rationale for keeping them: cascade uses +// them as **link-time injection targets** for `-ldflags -X`, populated +// once before init() runs and never mutated thereafter in production. +// They are the standard Go pattern for build-info embedding (also seen +// in `kubectl`, `hugo`, every cobra-based CLI). AP-07's hazards +// (order-dependent tests, multi-tenancy collisions) don't apply because +// these aren't config or registries — they're constants whose value is +// set at link time. Tests in this package and `internal/cli/cli_test.go` +// do mutate them (via the withMetadata helper in version_test.go); the +// "do not call t.Parallel() in any test that mutates these" discipline +// is documented in S-2 of docs/dev/0014-go-quality-audit.md and at the +// top of every seam-using *_test.go file. var ( // Version is the cascade module version, sourced from project/VERSION. // Always populated (either via ldflags or the go:embed fallback). diff --git a/internal/project/version_test.go b/internal/project/version_test.go index d8b6d67..b6291fe 100644 --- a/internal/project/version_test.go +++ b/internal/project/version_test.go @@ -8,6 +8,11 @@ import ( "testing" ) +// Parallel-unsafe: tests in this file mutate the package-level build-info +// vars (Version, GitCommit, GitBranch, GitSummary, BuildDate) and the +// readBuildInfo seam. Do not call t.Parallel() in any test that uses +// withMetadata. See S-2 in docs/dev/0014-go-quality-audit.md. + // withMetadata sets the package-level build vars for the duration of the // test and restores them afterward. Tests use this to exercise both the // empty-default path and the populated-via-ldflags path without relying on diff --git a/pkg/changeset/changeset.go b/pkg/changeset/changeset.go index eb0aa06..d106a9f 100644 --- a/pkg/changeset/changeset.go +++ b/pkg/changeset/changeset.go @@ -16,7 +16,15 @@ 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 + moduleRoot string + + // moduleRootSet distinguishes "user explicitly supplied WithModuleRoot" + // from "no option supplied." Post-bug-#12 the user-visible behaviour is + // identical for both (filepath.Abs("") and filepath.Abs(".") both + // resolve to the cwd), so the flag exists primarily so seam_test.go + // can drive the os.Getwd-error fallback branch deterministically. + // Documented as a deliberate test-driven design choice in F-15 of + // docs/dev/0014-go-quality-audit.md. moduleRootSet bool } @@ -64,8 +72,15 @@ var getCwd = os.Getwd // // 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(""). +// an error (rare), moduleRoot is left as the zero value at the fallback +// step; the subsequent filepath.Abs("") then resolves it to the process +// cwd anyway, so relative changedFiles are interpreted against the cwd — +// identical to passing WithModuleRoot("") or WithModuleRoot(".") (closes +// bug #12). If the absolutized cwd doesn't share a prefix with any +// pkg.Dir, the parent-directory lookup returns no matches and Resolve +// produces an empty result — the same observable outcome the original +// "stays empty" doc claimed, but reached through filepath.Abs's fallback +// rather than the empty string flowing untouched. // // Mapping rules: // - A file ending in ".go" whose parent directory exactly matches some @@ -113,9 +128,11 @@ func Resolve(changedFiles []string, pkgs []golist.Package, opts ...Option) []str // (which golist documents as absolute) succeeds even when the caller // supplies a relative path. filepath.Abs("") and filepath.Abs(".") // both resolve to the cwd, so this also handles the empty-string case - // gracefully. On error (rare; documented in os.Getwd), leave as-is — - // the lookup will fail consistently with the unfixed relative-input - // case rather than panic. Closes bug #12. + // (including the getCwd-error fallback at line 107) gracefully — the + // empty value is replaced by the process cwd here, not "left as-is." + // If filepath.Abs itself errors (rare; documented in path/filepath), + // the lookup falls back to the unmodified value and fails consistently + // rather than panicking. Closes bug #12. if abs, err := filepath.Abs(cfg.moduleRoot); err == nil { cfg.moduleRoot = abs } diff --git a/pkg/changeset/seam_test.go b/pkg/changeset/seam_test.go index e49dd62..4445556 100644 --- a/pkg/changeset/seam_test.go +++ b/pkg/changeset/seam_test.go @@ -7,6 +7,10 @@ import ( "github.com/geomyidia/cascade/pkg/golist" ) +// Parallel-unsafe: tests in this file mutate the package-level getCwd +// seam. Do not call t.Parallel() in any test that exercises it. See S-2 +// in docs/dev/0014-go-quality-audit.md. + // 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)) { diff --git a/pkg/golist/errors.go b/pkg/golist/errors.go index 7fc81c7..3992730 100644 --- a/pkg/golist/errors.go +++ b/pkg/golist/errors.go @@ -13,7 +13,7 @@ import ( // // These follow the EH-36 sentinel naming convention (ErrXxx) and are // the only mechanism callers should use to classify errors — string-match -// against Error() output is forbidden (AP-02). +// against Error() output is forbidden (AP-13). var ( // ErrGoNotFound is returned when the `go` binary cannot be found on // $PATH (or at the path configured via WithGoBin). The wrapped error diff --git a/pkg/golist/golist.go b/pkg/golist/golist.go index cfd0815..15fe084 100644 --- a/pkg/golist/golist.go +++ b/pkg/golist/golist.go @@ -200,8 +200,13 @@ func buildArgv(goBin string, tags []string, patterns []string) []string { // - other exec errors (rare): returned wrapped with %w under // ErrGoListFailed for category clarity. // -// ctx is the last param (against EH-08 ordering convention) because it's -// only used to detect cancellation post-hoc, not for propagation. +// ctx is the last param, deliberately deviating from CC-08 (ctx is the +// first parameter). The CC-08 MUST is calibrated to *propagation* — a +// function that passes ctx forward into a blocking call. This classifier +// consults ctx.Err() exactly once, post-hoc, to disambiguate "is this +// error a cancellation?" and never propagates ctx anywhere else; that is +// outside CC-08's intended scope. Cousin classifyGitDiffError keeps the +// same shape so readers who learn one know the other. func classifyRunError(err error, argv []string, dir string, stderr string, ctx context.Context) error { if ctxErr := ctx.Err(); ctxErr != nil { return fmt.Errorf("go list: %w", ctxErr) @@ -235,8 +240,4 @@ var ( _ error = (*ExitError)(nil) _ error = (*ParseError)(nil) _ Option = WithDir("") - // Reference io to keep the import alive for future use; the streaming - // decoder lives in parse.go and consumes this package's io types via - // the parseStream helper. - _ = io.EOF ) diff --git a/pkg/golist/golist_test.go b/pkg/golist/golist_test.go index 5d57d64..ac1dcea 100644 --- a/pkg/golist/golist_test.go +++ b/pkg/golist/golist_test.go @@ -101,13 +101,19 @@ func TestRun_DefaultPatterns(t *testing.T) { t.Skip("skipping subprocess test in short mode") } - tests := [][]string{nil, {}} - for i, patterns := range tests { - t.Run(map[bool]string{true: "nil", false: "empty"}[patterns == nil], func(t *testing.T) { - pkgs, err := golist.Run(t.Context(), nil, patterns, + tests := []struct { + name string + patterns []string + }{ + {"nil", nil}, + {"empty", []string{}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + pkgs, err := golist.Run(t.Context(), nil, tc.patterns, golist.WithDir(sampleModulePath(t))) if err != nil { - t.Fatalf("Run failed (case %d): %v", i, err) + t.Fatalf("Run failed: %v", err) } if len(pkgs) == 0 { t.Errorf("Run returned 0 packages with default ./...") diff --git a/pkg/golist/seam_test.go b/pkg/golist/seam_test.go index de9767a..8bf4690 100644 --- a/pkg/golist/seam_test.go +++ b/pkg/golist/seam_test.go @@ -8,6 +8,10 @@ import ( "testing" ) +// Parallel-unsafe: tests in this file mutate the package-level runGoList +// seam. Do not call t.Parallel() in any test that exercises it. See S-2 +// in docs/dev/0014-go-quality-audit.md. + // withSeam swaps the package-level runGoList for the duration of a // test, restoring the default afterward. func withSeam(t *testing.T, fn func(_ context.Context, argv []string, dir string, env []string) runResult) {