diff --git a/PLAN.md b/PLAN.md index 9c54bcd71..b3acee008 100644 --- a/PLAN.md +++ b/PLAN.md @@ -259,4 +259,6 @@ footer: | | 2607191918 | ✅ | haiku | [Deduplicate isClaimed between internal/schema and requiredstructure](plan/2607191918_arch-fix-isclaimed-dedup.md) | | 2607242010 | 🔲 | sonnet | [MDS072 external-link-check: SSRF and egress hardening](plan/2607242010_mds072-ssrf-network-hardening.md) | | 2607242011 | 🔲 | haiku | [Security hardening batch — 2026-07-24](plan/2607242011_security-hardening-batch-2026-07-24.md) | +| 2608021915 | 🔲 | haiku | [Move list-query subcommand logic out of cmd/mdsmith/main.go into query.go](plan/2608021915_arch-fix-query-subcommand-placement.md) | +| 2608021916 | 🔲 | sonnet | [Split internal/githooks by responsibility](plan/2608021916_arch-fix-githooks-package-split.md) | diff --git a/cmd/mdsmith/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index c202ed429..2aeb65549 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -811,6 +811,157 @@ func TestFixDiscovered_BadMaxInputSize_ExitsTwo(t *testing.T) { assert.Contains(t, stderr, "max-input-size") } +// gitBoundary marks dir as a repo root for config.Discover (see +// internal/config/load.go), so a test with no .mdsmith.yml of its own +// stops its upward config search at dir instead of walking to the +// filesystem root and, in principle, picking up an unrelated ancestor +// config. +func gitBoundary(t *testing.T, dir string) { + t.Helper() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) +} + +// --- runCheck --- +// +// runCheck is the "check" subcommand entry dispatched from dispatch() in +// main.go; go.md/audit-checklist.md name a CLI subcommand entry as a +// public surface, so a missing dedicated test here is a blocker, not tax. +// These exercise the three branches it routes to (flag error, stdin, +// explicit files, config-discovered files) in-process, mirroring the +// TestRunInit_* pattern in init_unit_test.go rather than relying solely +// on the binary-spawn e2e tests in e2e_coverage_test.go. + +func TestRunCheck_UnknownFlag_ExitsTwo(t *testing.T) { + var code int + stderr := captureStderr(func() { + code = runCheck([]string{"--definitely-not-a-flag"}) + }) + assert.Equal(t, 2, code) + assert.Contains(t, stderr, "unknown flag") +} + +func TestRunCheck_Stdin_ChecksSource(t *testing.T) { + dir := t.TempDir() + gitBoundary(t, dir) + t.Chdir(dir) + + oldStdin := os.Stdin + r, w, err := os.Pipe() + require.NoError(t, err) + defer r.Close() //nolint:errcheck // best-effort close on read-only pipe end + os.Stdin = r + defer func() { os.Stdin = oldStdin }() + go func() { + // Trailing spaces trigger a diagnostic, so a passing exit code + // alone can't mask a routing bug that skips reading stdin. + _, _ = w.WriteString("# Title\n\nHello \n") + _ = w.Close() + }() + + var code int + stderr := captureStderr(func() { + code = runCheck([]string{"-"}) + }) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "") +} + +func TestRunCheck_Files_ExitsOneOnDiagnostics(t *testing.T) { + dir := t.TempDir() + gitBoundary(t, dir) + t.Chdir(dir) + require.NoError(t, os.WriteFile(filepath.Join(dir, "dirty.md"), + []byte("# Title\n\nHello \n"), 0o644)) + + var code int + stderr := captureStderr(func() { + code = runCheck([]string{"dirty.md"}) + }) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "dirty.md") +} + +func TestRunCheck_Discovered_ChecksConfiguredFiles(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, ".mdsmith.yml"), + []byte("files: [\"**/*.md\"]\n"), 0o644)) + // Trailing spaces trigger a diagnostic, so a passing exit code alone + // can't mask a discovery bug that silently finds zero files. + require.NoError(t, os.WriteFile(filepath.Join(dir, "test.md"), + []byte("# Title\n\nHello \n"), 0o644)) + t.Chdir(dir) + + var code int + stderr := captureStderr(func() { + code = runCheck(nil) + }) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "test.md") +} + +// --- runFix --- +// +// runFix is the "fix" subcommand entry, the same public-surface tier as +// runCheck above. These cover the stdin-rejection branch and the two +// file-resolution branches (explicit files, config-discovered files) that +// runCheck's tests exercise, plus the disk write that runCheck has no +// equivalent of. + +func TestRunFix_UnknownFlag_ExitsTwo(t *testing.T) { + var code int + stderr := captureStderr(func() { + code = runFix([]string{"--definitely-not-a-flag"}) + }) + assert.Equal(t, 2, code) + assert.Contains(t, stderr, "unknown flag") +} + +func TestRunFix_StdinArg_ExitsTwo(t *testing.T) { + var code int + stderr := captureStderr(func() { + code = runFix([]string{"-"}) + }) + assert.Equal(t, 2, code) + assert.Contains(t, stderr, "cannot fix stdin in place") +} + +func TestRunFix_Files_FixesGivenFile(t *testing.T) { + dir := t.TempDir() + gitBoundary(t, dir) + t.Chdir(dir) + path := filepath.Join(dir, "fixme.md") + require.NoError(t, os.WriteFile(path, []byte("# Title\n\nHello \n"), 0o644)) + + var code int + captureStderr(func() { + code = runFix([]string{"fixme.md"}) + }) + assert.Equal(t, 0, code) + + fixed, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "# Title\n\nHello\n", string(fixed)) +} + +func TestRunFix_Discovered_FixesConfiguredFiles(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, ".mdsmith.yml"), + []byte("files: [\"**/*.md\"]\n"), 0o644)) + path := filepath.Join(dir, "fixme.md") + require.NoError(t, os.WriteFile(path, []byte("# Title\n\nHello \n"), 0o644)) + t.Chdir(dir) + + var code int + captureStderr(func() { + code = runFix(nil) + }) + assert.Equal(t, 0, code) + + fixed, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "# Title\n\nHello\n", string(fixed)) +} + // --- printErrors --- func TestPrintErrors_Empty_NoOutput(t *testing.T) { diff --git a/docs/development/architecture-audit.md b/docs/development/architecture-audit.md index 9053b8433..add92fd05 100644 --- a/docs/development/architecture-audit.md +++ b/docs/development/architecture-audit.md @@ -6,7 +6,7 @@ summary: >- solid-architecture skill (audit mode) appends here; blockers are also filed as plans. -audit-from: 6680ff53d440337d780e62d19d6eb65a4e1b6e2c +audit-from: 2ab4b2949e9420b2eb73f4239091fd647c4b5e80 --- # Architecture audit log @@ -17,6 +17,108 @@ to [the archive](architecture-audit-archive.md) this cycle to stay under the file-length budget; every finding there is resolved. +## Audit 2026-08-02 (range: 6680ff5..2ab4b29) + +148 touched files. Notable new surfaces: MDS073 +slide-structure and MDS060 occurrence. MDS073 was +renamed from MDS072; no leftover duplication was +found. Also new: foreign-managed regions and +user-extensible wordlists. + +No rule-to-rule imports. No reverse-layer imports. No +Liskov breaks. The prior cycle's `isClaimed` dedup +(plan 2607191918) is confirmed resolved — `schema.IsClaimed` +is now exported and `requiredstructure` calls it directly. + +### blockers (2026-08-02) + +- `cmd/mdsmith/check.go`'s `runCheck` and + `cmd/mdsmith/fix.go`'s `runFix` — the `check` and `fix` + CLI subcommand entry points — had no dedicated in-process + unit test; only binary-spawn e2e tests + (`internal/integration`, `cmd/mdsmith/e2e_*_test.go`) + exercised them. + [audit-checklist.md][audit-checklist]: "blocker if the + function is on a public surface ... a CLI subcommand + entry." Fixed: added `TestRunCheck_UnknownFlag_ExitsTwo`, + `TestRunCheck_Stdin_ChecksSource`, + `TestRunCheck_Files_ExitsOneOnDiagnostics`, + `TestRunCheck_Discovered_ChecksConfiguredFiles`, + `TestRunFix_UnknownFlag_ExitsTwo`, + `TestRunFix_StdinArg_ExitsTwo`, + `TestRunFix_Files_FixesGivenFile`, and + `TestRunFix_Discovered_FixesConfiguredFiles` to + `cmd/mdsmith/main_unit_test.go`, matching the + `TestRunInit_*` precedent from the 2026-07-19 cycle. + `go test ./cmd/mdsmith/...` and + `go tool golangci-lint run` are green. + +### tax (2026-08-02) + +- `cmd/mdsmith/main.go` still carries the `list query` + subcommand's full domain logic (`parseQueryFlags`, + `runQuery`, `queryFiles`, `readFrontMatterRaw`) instead + of a dedicated file, even though `list.go` dispatches to + it the same way it dispatches to `backlinks.go`. + [go.md][go] "Clean wiring in `cmd/mdsmith`" — + [plan/2608021915][2608021915]. +- `internal/githooks`'s package doc comment joins three + responsibilities with "and" (hook-script generation, + `.gitattributes` I/O, directive-file discovery) — + [go.md][go] refactor-moves: "Split a package by + question" — [plan/2608021916][2608021916]. +- `internal/lint/files.go`'s CLI-path/glob resolution + overlaps the question `internal/discovery` already + answers for config-glob-driven discovery. No plan filed + this cycle; flagged for the next package-boundary pass. +- A broad set of unexported, branching helper functions + across the touched rule packages + (`requiredstructure`, `crossfilereferenceintegrity`, + `noreferencestyle`, `include`, `linkstyle`, + `slidevstructure`, `occurrence`, `tablefmt`, + `tocdirective`, `githooksync`, `build`) and + `cmd/mdsmith` (`checkFiles`, `checkBatchOptions`, + `batchMaxBytes`, `parseFixFlags`, `setFixUsage`, + `fixFiles`, `readStdinLimited`, `regenDirectiveNames`) + lack a dedicated `TestFuncName` symbol per + [tests.md][tests], though each is exercised + transitively through its caller's scenario tests. None + sit on a public surface by themselves, so tax rather + than blocker. +- Trivial one-line accessors (`FixTitle` across several + rule packages, plus a few `rule.Rule` capability + predicates) carry only an "implements rule.X" comment, + not the exemption statement [tests.md][tests] requires + to distinguish "no test by design" from "no test, + forgotten." +- Similar untested-but-covered helper clusters exist in + `internal/lsp/server_codeaction.go`, + `internal/config/merge.go`, and `internal/fix/fix.go`. + +[tests]: architecture/tests.md +[go]: architecture/go.md +[audit-checklist]: architecture/audit-checklist.md +[2608021915]: ../../plan/2608021915_arch-fix-query-subcommand-placement.md +[2608021916]: ../../plan/2608021916_arch-fix-githooks-package-split.md + +### nice-to-have (2026-08-02) + +- Several tests covering the flagged helper clusters use + scenario names (e.g. `TestDriftParts_ResolvesHooksDirOnce`) + rather than the literal `TestReceiver_Foo` binding — + coverage exists, only the naming convention drifts. +- `stagingHelperShellFunc` (`internal/githooks/githooks.go`) + contains "Helper" in its name — go.md flags this as a + smell, though the constant is well-scoped. Rename on + next touch. +- `Override.Patterns()` / `KindAssignmentEntry.Patterns()` + (`internal/config/config.go`) are two-line branching + methods exercised only inside larger config tests, not by + a dedicated `Test*`. Low regression risk. +- `parseCheckFlags` / `parseFixFlags` cross go.md's ~50-line + guidance for `cmd/mdsmith` handlers, but the body is pure + flag-registration boilerplate, not domain logic. + ## Audit 2026-07-19 (range: 834b560..6680ff5) 89 touched files. Notable new surfaces: SARIF output, @@ -86,8 +188,6 @@ None. to an interface... once two rules needed the same shape" — [plan/2607191918][2607191918]. -[tests]: architecture/tests.md -[go]: architecture/go.md [2607191917]: ../../plan/2607191917_arch-fix-printinitcatalog-unit-test.md [2607191918]: ../../plan/2607191918_arch-fix-isclaimed-dedup.md diff --git a/plan/2608021915_arch-fix-query-subcommand-placement.md b/plan/2608021915_arch-fix-query-subcommand-placement.md new file mode 100644 index 000000000..6469a839d --- /dev/null +++ b/plan/2608021915_arch-fix-query-subcommand-placement.md @@ -0,0 +1,74 @@ +--- +id: 2608021915 +title: >- + Move list-query subcommand logic out of + cmd/mdsmith/main.go into query.go +status: "🔲" +model: haiku +summary: >- + parseQueryFlags, runQuery, queryFiles, and + readFrontMatterRaw implement the `mdsmith list + query` subcommand but live in main.go instead of a + dedicated per-subcommand file. Flagged by the + 2026-08-02 audit. +--- +# Move list-query subcommand logic out of cmd/mdsmith/main.go into query.go + +## Goal + +Relocate the `list query` subcommand's implementation +into its own file. That keeps `main.go` as +dispatch-and-glue, matching the pattern already used +for every sibling subcommand. + +## Background + +The 2026-08-02 audit (see +[the audit log](../docs/development/architecture-audit.md)) +found this placement gap: + +- [main.go](../cmd/mdsmith/main.go) defines + `parseQueryFlags`, `runQuery`, `queryFiles`, and + `readFrontMatterRaw` — the full domain logic for the + `list query` subcommand (CUE-expression matching + against front matter, its own file walk). +- [list.go](../cmd/mdsmith/list.go)'s `runList` + dispatches `case "query": return runQuery(...)`, the + same way it dispatches `case "backlinks":` to + `runBacklinks`, which lives in its own + [backlinks.go](../cmd/mdsmith/backlinks.go). +- [go.md](../docs/development/architecture/go.md)'s + "Clean wiring in `cmd/mdsmith`" section: domain logic + belongs in `pkg/mdsmith`, `internal/engine`, or a + subcommand's own file — not in `main.go`. +- The 2026-07-19 audit already applied this exact move + once, relocating the `init` subcommand's logic out of + `main.go` into + [init.go](../cmd/mdsmith/init.go); this plan repeats + that move for `query`. + +## Tasks + +1. Create `cmd/mdsmith/query.go` and move + `parseQueryFlags`, `runQuery`, `queryFiles`, and + `readFrontMatterRaw` into it, unchanged. +2. Move the query-only tests covering those functions + out of `main_unit_test.go` into a new + `cmd/mdsmith/query_unit_test.go`, following the + `init_unit_test.go` precedent. +3. Confirm `list.go`'s `runList` still compiles against + the relocated `runQuery` with no signature change. +4. `go build ./...` passes. +5. `go test ./cmd/mdsmith/...` passes. +6. `go tool -modfile=tools/go.mod golangci-lint run` + reports no issues. + +## Acceptance Criteria + +- [ ] `parseQueryFlags`, `runQuery`, `queryFiles`, and + `readFrontMatterRaw` no longer appear in + `cmd/mdsmith/main.go`. +- [ ] `cmd/mdsmith/query.go` holds the relocated code + with no behavior change. +- [ ] `go test ./...` is green. +- [ ] `mdsmith check .` is green. diff --git a/plan/2608021916_arch-fix-githooks-package-split.md b/plan/2608021916_arch-fix-githooks-package-split.md new file mode 100644 index 000000000..d3e871b59 --- /dev/null +++ b/plan/2608021916_arch-fix-githooks-package-split.md @@ -0,0 +1,86 @@ +--- +id: 2608021916 +title: >- + Split internal/githooks by responsibility +status: "🔲" +model: sonnet +summary: >- + internal/githooks's package doc names three joined + responsibilities — hook-script generation, + .gitattributes glob/managed-block I/O, and + directive-file discovery — a package-by-question SRP + smell per go.md. Flagged by the 2026-08-02 audit. +--- +# Split internal/githooks by responsibility + +## Goal + +Split `internal/githooks` so each resulting package +answers one question, per go.md's "Split a package by +question" refactor move. + +## Background + +The 2026-08-02 audit (see +[the audit log](../docs/development/architecture-audit.md)) +found this SRP smell: + +- [githooks.go](../internal/githooks/githooks.go)'s + package doc comment names three joined + responsibilities: "managing the pre-merge-commit + hook, merge-driver assignments in `.gitattributes`, + and discovery of files that contain generated-section + directives." +- [go.md](../docs/development/architecture/go.md)'s + refactor-moves section: "Split a package by question. + If the package doc comment requires 'and' to describe + ... the package wants to be two." +- The file is 1,346 lines and cleanly separates into: + - hook-script generation/validation + (`BuildHookScript`, `HookMatchesCanonical`, + the staging shell-function builder); + - `.gitattributes` glob/managed-block read-write + (`GlobsFromConfig`, `WriteGitattributes`, + `ExtractGlobs`, `StageGitattributes`); + - directive-file discovery (`DiscoverFiles`, + the directive-marker scanner). +- The project has precedent for this exact move: `internal/gitignore`, + `internal/bytelimit`, and `internal/piparser` were all + split out of `internal/lint` once their question + diverged from "model a parsed Markdown file" (see + go.md's package list). + +## Tasks + +1. Read [githooks.go](../internal/githooks/githooks.go) + and its test file in full; group every exported and + unexported symbol by the three questions above. +2. Create `internal/gitattributes` for the + `.gitattributes` glob/managed-block read-write group; + move `GlobsFromConfig`, `WriteGitattributes`, + `ExtractGlobs`, `StageGitattributes`, and their + dedicated tests into it. +3. Decide which package keeps `DiscoverFiles` — the + merge-driver install path is the deciding consumer; + read its call sites in `cmd/mdsmith/mergedriver.go` + before choosing. +4. Update every import of the moved symbols across + `cmd/mdsmith` and `internal/...`. +5. Keep `internal/githooks` scoped to hook-script + generation/validation only. +6. `go build ./...` passes. +7. `go test ./...` passes. +8. `go tool -modfile=tools/go.mod golangci-lint run` + reports no issues. + +## Acceptance Criteria + +- [ ] `internal/githooks`'s package doc no longer needs + "and" to describe its responsibility. +- [ ] `.gitattributes` glob/managed-block logic lives in + its own package with its own tests. +- [ ] No behavior change: `mdsmith merge-driver install` + and `mdsmith pre-merge-commit install` produce + identical output before and after the split. +- [ ] `go test ./...` is green. +- [ ] `mdsmith check .` is green.