Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
45 changes: 45 additions & 0 deletions docs/dev/0014-go-quality-audit.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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' <file>` 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.
6 changes: 4 additions & 2 deletions internal/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 "-":
Expand Down
8 changes: 7 additions & 1 deletion internal/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cli_test

import (
"bytes"
"os"
"path/filepath"
"strings"
"testing"
Expand All @@ -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() {
Expand Down Expand Up @@ -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)
}
7 changes: 6 additions & 1 deletion internal/cli/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions internal/cli/helpers_test.go

This file was deleted.

Loading
Loading