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..8314892 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. @@ -349,3 +379,18 @@ Total Go files audited: **27** (excluding `pkg/golist/testdata/sample-module/*.g Honest engagement note: per the methodology's flag-dissonance principle, the systemic citation-cleanup recommendation in S-1 includes the audit prompt's own `TD-09 / IM-04` mis-pair as evidence that the project has a recurring drift on substrate IDs. This is meta — the audit prompt recommended citing pattern IDs concretely, and I cited the prompt's own ID-drift back at it because the trend is the finding. CDC may rule that the prompt's drift is out of scope; I've surfaced it because the trending discipline asked me to. When CDC reviews: every per-row Verify is a `grep -n 'quoted-text' ` against the cited file:line, plus a re-read of the named substrate pattern ID. The protocol holds when CC's findings reproduce against an independent reading; my self-assessment of the report's verifiability is "high — every finding is anchored at file:line + verbatim quote." + +--- + +## Post-audit closure — PR-B (cousin-shape symmetry) + +Closes the two CONSIDER findings the 0015 work-order grouped as PR-B. Branch: `audit/follow-up-pr-b-cousin-shape`. Companion to PR-A (`audit/follow-up-pr-a-doc-pass`), which carries the closure walk for the other 14 findings + PROMPT-1. + +| ID | Disposition | Resolution + post-fix evidence | +|-------|---------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| F-5 | **closed in PR-B** | `internal/cli/errors.go`'s `*GitDiffError` now carries a `Dir string` field next to `Cmd []string`, mirroring `*golist.ExitError`'s shape exactly. `classifyGitDiffError`'s third parameter is `dir string` (was `_`); the caller at `internal/cli/cli.go` passes `cfg.root` which is now stored on `*GitDiffError.Dir` instead of being discarded. New assertion in `internal/cli/seam_test.go`'s `TestClassifyGitDiffError/exec_exit_error` verifies `ge.Dir == "/m"`. ✓ | +| F-14 | **closed in PR-B** | `gitDiffResult` (`internal/cli/seam.go`) grew an `argv []string` field; `defaultRunGitDiff` constructs argv once and uses it both for the actual `exec.CommandContext` invocation and as the returned diagnostic argv — they cannot drift. The synthetic argv reconstruction at `internal/cli/cli.go`'s `loadChangeSet` is removed; the call site now reads `classifyGitDiffError(res.err, res.argv, cfg.root, res.stderr, ctx)` and the comment names F-14. Tests that synthesize `gitDiffResult` without setting `argv` produce nil Cmd in the diagnostic — acceptable for the existing tests (none assert on Cmd); a future test that needs the argv sets it on the synthetic result. ✓ | + +`make check` (build + lint via `golangci-lint` + `go test -race -count=1 ./...`) passes against PR-B's working tree. + +When PR-A and PR-B both land on `main`, every audit finding is closed: 14 + PROMPT-1 via PR-A, F-5 + F-14 via PR-B. Net: 16 of 16 (plus the meta-finding) closed. No deferrals at exit. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index f1c83c1..acb0019 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -266,8 +266,10 @@ func loadChangeSet(ctx context.Context, cfg *config, stdin io.Reader) ([]string, case "": res := runGitDiff(ctx, cfg.base, cfg.head, cfg.root) if res.err != nil { - argv := []string{"git", "diff", "--name-only", cfg.base + ".." + cfg.head} - return nil, classifyGitDiffError(res.err, argv, cfg.root, res.stderr, ctx) + // res.argv comes back from the seam itself, so the diagnostic + // chain reflects the actual exec invocation rather than a + // reconstruction (closes F-14 in 0014-go-quality-audit.md). + return nil, classifyGitDiffError(res.err, res.argv, cfg.root, res.stderr, ctx) } return scanLines(res.stdout) case "-": 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/errors.go b/internal/cli/errors.go index b683e05..b0a7524 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -20,11 +20,16 @@ var ErrGitDiffFailed = errors.New("git diff failed") // Unwrap directly. // // The shape mirrors pkg/golist.ExitError so contributors who already know one -// know the other (EH-08 + EH-16). +// know the other (EH-08 + EH-16). The Dir field was added to close F-5 in +// docs/dev/0014-go-quality-audit.md, restoring full cousin-shape parity. type GitDiffError struct { // Cmd is the full argv as passed to exec, in order, for reproduction. Cmd []string + // Dir is the working directory the command was run in (typically the + // absolutized cfg.root from the cli layer). Mirrors *golist.ExitError.Dir. + Dir string + // ExitCode is the subprocess exit code (typically 1 or 128 for `git diff` // errors; may be other values). ExitCode int 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..162e34d 100644 --- a/internal/cli/seam.go +++ b/internal/cli/seam.go @@ -12,12 +12,14 @@ import ( // gitDiffResult is the io seam's return shape: stdout (a Reader so tests can // supply bytes.Buffer / strings.Reader / etc.), the raw cmd.Run error (nil on // success; *exec.ExitError on non-zero; other errors for fork failures / -// cancellation / lookup failures), and the captured stderr text. Mirrors -// pkg/golist's runResult. +// cancellation / lookup failures), the captured stderr text, and the actual +// argv the seam used (so the cli layer's diagnostic doesn't drift from the +// real exec invocation; closes F-14). Mirrors pkg/golist's runResult. type gitDiffResult struct { stdout io.Reader err error stderr string + argv []string } // runGitDiff is the function-variable seam over the os/exec call. Production @@ -30,14 +32,18 @@ type gitDiffResult struct { var runGitDiff = defaultRunGitDiff // defaultRunGitDiff shells out to `git diff --name-only ..` in -// the configured working directory and captures stdout / stderr. +// the configured working directory and captures stdout / stderr. The argv +// it constructs is returned verbatim in the gitDiffResult so the cli layer's +// diagnostic chain (in *GitDiffError.Cmd) reflects the actual exec, not a +// reconstruction that could drift. // // gosec G204 (subprocess from variable) is suppressed: argv[0] is the literal // string "git" (a constant); argv[1:] is constructed from caller-supplied // refs that are documented as caller-controlled. The package contract makes // this explicit. func defaultRunGitDiff(ctx context.Context, base, head, dir string) gitDiffResult { - cmd := exec.CommandContext(ctx, "git", "diff", "--name-only", base+".."+head) //nolint:gosec + argv := []string{"git", "diff", "--name-only", base + ".." + head} + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) //nolint:gosec if dir != "" { cmd.Dir = dir } @@ -45,7 +51,7 @@ func defaultRunGitDiff(ctx context.Context, base, head, dir string) gitDiffResul cmd.Stdout = &stdout cmd.Stderr = &stderr err := cmd.Run() - return gitDiffResult{stdout: &stdout, err: err, stderr: stderr.String()} + return gitDiffResult{stdout: &stdout, err: err, stderr: stderr.String(), argv: argv} } // classifyGitDiffError converts an exec.Cmd.Run error into the appropriate @@ -59,10 +65,18 @@ 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. -func classifyGitDiffError(err error, argv []string, _, stderr string, ctx context.Context) error { +// 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. +// +// dir is the working directory the seam ran the subprocess in (typically the +// absolutized cfg.root); stored on *GitDiffError.Dir for cousin-shape parity +// with *golist.ExitError (closes F-5 in docs/dev/0014-go-quality-audit.md). +func classifyGitDiffError(err error, argv []string, dir, stderr string, ctx context.Context) error { if ctxErr := ctx.Err(); ctxErr != nil { return fmt.Errorf("git diff: %w", ctxErr) } @@ -70,6 +84,7 @@ func classifyGitDiffError(err error, argv []string, _, stderr string, ctx contex if errors.As(err, &exitErr) { return &GitDiffError{ Cmd: append([]string(nil), argv...), + Dir: dir, ExitCode: exitErr.ExitCode(), Stderr: stderr, underlying: exitErr, diff --git a/internal/cli/seam_test.go b/internal/cli/seam_test.go index 15a0799..2c87264 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) { @@ -260,6 +264,11 @@ func TestClassifyGitDiffError(t *testing.T) { if ge.Stderr != "fatal: bad ref\n" { t.Errorf("ge.Stderr = %q, want %q", ge.Stderr, "fatal: bad ref\n") } + // F-5: Dir is propagated onto *GitDiffError for cousin-shape parity + // with *golist.ExitError. + if ge.Dir != "/m" { + t.Errorf("ge.Dir = %q, want %q", ge.Dir, "/m") + } }) t.Run("other_exec_error", func(t *testing.T) { diff --git a/internal/project/VERSION b/internal/project/VERSION index 8294c18..7693c96 100644 --- a/internal/project/VERSION +++ b/internal/project/VERSION @@ -1 +1 @@ -0.1.2 \ No newline at end of file +0.1.3 \ No newline at end of file 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) {