diff --git a/internal/services/covgate/covgate.go b/internal/services/covgate/covgate.go index dfc2d9c..84cfb2d 100644 --- a/internal/services/covgate/covgate.go +++ b/internal/services/covgate/covgate.go @@ -94,7 +94,7 @@ func (r *runner) run(opts Opts) error { return err } - pkgs, err = r.applyExclude(pkgs, opts.Exclude, w) + pkgs, excluded, err := r.applyExclude(pkgs, opts.Exclude, w) if err != nil { return err } @@ -113,51 +113,54 @@ func (r *runner) run(opts Opts) error { start := time.Now() results := r.runPackages(pkgs, ctx, parallelism) wallTime := time.Since(start) - return r.printResults(w, results, wallTime) + return r.printResults(w, results, excluded, module, wallTime) } // applyExclude removes packages matched by the comma-separated // exclude patterns from pkgs. It preserves the original order of -// pkgs and prints a one-line notice to w when any package is -// actually removed. An empty (or whitespace-only) exclude string -// returns pkgs unchanged with no output. +// pkgs in both returned slices and prints a one-line notice to w +// when any package is actually removed. An empty (or +// whitespace-only) exclude string returns pkgs unchanged, a nil +// excluded slice, and no output. func (r *runner) applyExclude( pkgs []string, exclude string, w io.Writer, -) ([]string, error) { +) (kept []string, excluded []string, err error) { if strings.TrimSpace(exclude) == "" { - return pkgs, nil + return pkgs, nil, nil } - excluded := make(map[string]struct{}) + excludedSet := make(map[string]struct{}) for _, raw := range strings.Split(exclude, ",") { entry := strings.TrimSpace(raw) if entry == "" { continue } - matched, err := r.goListPackages(entry) - if err != nil { - return nil, fmt.Errorf("resolve exclude %q: %w", entry, err) + matched, listErr := r.goListPackages(entry) + if listErr != nil { + return nil, nil, fmt.Errorf("resolve exclude %q: %w", entry, listErr) } for _, p := range matched { - excluded[p] = struct{}{} + excludedSet[p] = struct{}{} } } - kept := make([]string, 0, len(pkgs)) + kept = make([]string, 0, len(pkgs)) + excluded = make([]string, 0, len(excludedSet)) for _, p := range pkgs { - if _, drop := excluded[p]; drop { + if _, drop := excludedSet[p]; drop { + excluded = append(excluded, p) continue } kept = append(kept, p) } - if removed := len(pkgs) - len(kept); removed > 0 { + if len(excluded) > 0 { _, _ = fmt.Fprintf( w, "Excluded %d package(s) from coverage measurement\n", - removed, + len(excluded), ) } - return kept, nil + return kept, excluded, nil } func (r *runner) runPackages( @@ -181,7 +184,11 @@ func (r *runner) runPackages( } func (r *runner) printResults( - w io.Writer, results []checkResult, totalTime time.Duration, + w io.Writer, + results []checkResult, + excluded []string, + module string, + totalTime time.Duration, ) error { hasFailures := false for _, res := range results { @@ -191,6 +198,10 @@ func (r *runner) printResults( } } + for _, pkg := range excluded { + _, _ = fmt.Fprint(w, skippedRow(gocover.RelPkg(pkg, module))) + } + _, _ = fmt.Fprintf(w, "\nTotal time: %s\n", fmtDuration(totalTime)) if hasFailures { _, _ = fmt.Fprintln( @@ -207,12 +218,12 @@ func (r *runner) printResults( func printHeader(w io.Writer) { _, _ = fmt.Fprintf( - w, "%-6s %8s %8s %8s %s\n", + w, "%-7s %8s %8s %8s %s\n", "STATUS", "COVERAGE", "REQUIRED", "TIME", "PACKAGE", ) _, _ = fmt.Fprintf( - w, "%-6s %8s %8s %8s %s\n", - "------", "--------", "--------", "--------", "-------", + w, "%-7s %8s %8s %8s %s\n", + "-------", "--------", "--------", "--------", "-------", ) } @@ -251,7 +262,7 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { var b strings.Builder if testErr != nil { _, _ = fmt.Fprintf( - &b, "%-6s %8s %8s %8s %s\n", + &b, "%-7s %8s %8s %8s %s\n", "FAIL", "---", "---", fmtDuration(elapsed), relPkg+" (tests failed)", ) @@ -263,7 +274,7 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { if coverage < threshold { _, _ = fmt.Fprintf( - &b, "%-6s %7.1f%% %7.1f%% %8s %s\n", + &b, "%-7s %7.1f%% %7.1f%% %8s %s\n", "FAIL", coverage, threshold, fmtDuration(elapsed), relPkg, ) return checkResult{b.String(), false, elapsed} @@ -274,7 +285,7 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { if gap > ctx.tightnessTolerance { recommended := coverage - ctx.tightnessTolerance _, _ = fmt.Fprintf( - &b, "%-6s %7.1f%% %7.1f%% %8s "+ + &b, "%-7s %7.1f%% %7.1f%% %8s "+ "%s (required lags actual by %.1fpp; "+ "update .covgate to >= %.1f)\n", "LOOSE", coverage, threshold, fmtDuration(elapsed), @@ -285,12 +296,23 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { } _, _ = fmt.Fprintf( - &b, "%-6s %7.1f%% %7.1f%% %8s %s\n", + &b, "%-7s %7.1f%% %7.1f%% %8s %s\n", "PASS", coverage, threshold, fmtDuration(elapsed), relPkg, ) return checkResult{b.String(), true, elapsed} } +// skippedRow formats a single SKIPPED row using the same column +// widths as PASS/FAIL/LOOSE. relPkg is the module-relative import +// path. Used for packages removed by --exclude so the user can see +// which ones were skipped. +func skippedRow(relPkg string) string { + return fmt.Sprintf( + "%-7s %8s %8s %8s %s\n", + "SKIPPED", "---", "---", "---", relPkg, + ) +} + func fmtDuration(d time.Duration) string { d = d.Round(100 * time.Millisecond) if d < time.Minute { diff --git a/internal/services/covgate/covgate_test.go b/internal/services/covgate/covgate_test.go index ad08bfd..1d82706 100644 --- a/internal/services/covgate/covgate_test.go +++ b/internal/services/covgate/covgate_test.go @@ -178,13 +178,15 @@ func TestRun_Exclude(t *testing.T) { lookup map[string][]string wantContains []string wantNotContain []string + extra func(t *testing.T, out string) }{ { name: "NoExclude", exclude: "", lookup: map[string][]string{"./...": allThree}, wantContains: []string{"pkg/a", "pkg/b", "pkg/c"}, - wantNotContain: []string{"Excluded"}, + wantNotContain: []string{"Excluded", "SKIPPED"}, + extra: nil, }, { name: "Subset", @@ -192,10 +194,14 @@ func TestRun_Exclude(t *testing.T) { lookup: map[string][]string{"./...": allThree, "./pkg/b": {pkgB}}, wantContains: []string{ "pkg/a", + "pkg/b", "pkg/c", + "SKIPPED", "Excluded 1 package(s) from coverage measurement", + "All packages meet minimum coverage requirement", }, - wantNotContain: []string{"pkg/b"}, + wantNotContain: []string{"FAIL"}, + extra: nil, }, { name: "NoOpPattern", @@ -205,7 +211,8 @@ func TestRun_Exclude(t *testing.T) { "./does-not-exist/...": {}, }, wantContains: []string{"pkg/a", "pkg/b", "pkg/c"}, - wantNotContain: []string{"Excluded"}, + wantNotContain: []string{"Excluded", "SKIPPED"}, + extra: nil, }, { // Includes empty entries before, between, and after @@ -219,21 +226,80 @@ func TestRun_Exclude(t *testing.T) { "./pkg/c": {pkgC}, }, wantContains: []string{ + "pkg/a", "pkg/b", + "pkg/c", + "SKIPPED", "Excluded 2 package(s) from coverage measurement", }, - wantNotContain: []string{"pkg/a", "pkg/c"}, + wantNotContain: []string{"FAIL"}, + extra: nil, }, { name: "AllPackages", exclude: "./...", lookup: map[string][]string{"./...": allThree}, wantContains: []string{ + "pkg/a", + "pkg/b", + "pkg/c", + "SKIPPED", "Excluded 3 package(s) from coverage measurement", "All packages meet minimum coverage requirement", "Total time:", }, - wantNotContain: []string{"pkg/a", "pkg/b", "pkg/c"}, + wantNotContain: []string{"FAIL"}, + extra: nil, + }, + { + name: "OrderingAndCount", + exclude: "./pkg/b", + lookup: map[string][]string{"./...": allThree, "./pkg/b": {pkgB}}, + wantContains: []string{"pkg/a", "pkg/b", "pkg/c", "SKIPPED"}, + wantNotContain: []string{"FAIL"}, + extra: func(t *testing.T, out string) { + t.Helper() + idxSkipped := strings.Index(out, "SKIPPED") + idxA := strings.Index(out, "pkg/a") + idxC := strings.Index(out, "pkg/c") + if idxA < 0 || idxC < 0 || idxSkipped < 0 { + t.Fatalf("expected pkg/a, pkg/c, and SKIPPED in output:\n%s", out) + } + if idxA >= idxSkipped { + t.Errorf( + "expected measured pkg/a row (%d) before SKIPPED block (%d):\n%s", + idxA, idxSkipped, out, + ) + } + if idxC >= idxSkipped { + t.Errorf( + "expected measured pkg/c row (%d) before SKIPPED block (%d):\n%s", + idxC, idxSkipped, out, + ) + } + if got := strings.Count(out, "SKIPPED"); got != 1 { + t.Errorf("expected exactly 1 SKIPPED, got %d:\n%s", got, out) + } + var skippedLine string + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "pkg/b") { + skippedLine = line + break + } + } + if skippedLine == "" { + t.Fatalf("no line contained pkg/b:\n%s", out) + } + if !strings.Contains(skippedLine, "SKIPPED") { + t.Errorf("pkg/b line missing SKIPPED: %q", skippedLine) + } + if strings.Count(skippedLine, "---") != 3 { + t.Errorf( + "pkg/b SKIPPED line expected 3 '---' placeholders, got %d: %q", + strings.Count(skippedLine, "---"), skippedLine, + ) + } + }, }, } @@ -276,6 +342,9 @@ func TestRun_Exclude(t *testing.T) { t.Errorf("output unexpectedly contains %q:\n%s", unwanted, out) } } + if tc.extra != nil { + tc.extra(t, out) + } }) } } @@ -655,7 +724,7 @@ func TestPrintResults_UsesWallTime(t *testing.T) { var buf bytes.Buffer //nolint:exhaustruct // test uses partial initialization r := runner{} - err := r.printResults(&buf, results, 3*time.Second) + err := r.printResults(&buf, results, nil, modName, 3*time.Second) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/plans/completed/20260515-covgate-skipped-rows.md b/plans/completed/20260515-covgate-skipped-rows.md new file mode 100644 index 0000000..16b7e66 --- /dev/null +++ b/plans/completed/20260515-covgate-skipped-rows.md @@ -0,0 +1,328 @@ +# Render excluded packages as SKIPPED rows in `miru covgate` output + +This ExecPlan is a living document. The sections Progress, Surprises & Discoveries, Decision Log, and Outcomes & Retrospective must be kept up to date as work proceeds. + +## Scope + +| Repository | Access | Description | +|-----------|--------|-------------| +| `gotools/` | read-write | Extend covgate service to emit `SKIPPED` rows for `--exclude`d packages; update tests. | + +This plan lives in `gotools/plans/backlog/` because all edits happen in `internal/services/covgate/` inside the `gotools` repo. No other repos are read or written. + +## Purpose / Big Picture + +`miru covgate --exclude ` already removes matched packages from the measurement set and prints a single summary line `Excluded N package(s) from coverage measurement`. Today the excluded packages never appear by name in the per-package table, so a user who mistypes a pattern only sees the count drop — not which packages are missing. This change keeps the summary line and additionally renders one `SKIPPED` row per excluded package in the same table, so users can confirm at a glance exactly which packages were skipped. + +After this change, the user running: + + $ go tool miru covgate --packages ./... --exclude ./pkg/b + +against a module that enumerates `pkg/a`, `pkg/b`, `pkg/c` will see: + + Checking per-package coverage (default minimum: 80.0%)... + + Excluded 1 package(s) from coverage measurement + STATUS COVERAGE REQUIRED TIME PACKAGE + ------ -------- -------- -------- ------- + PASS 90.0% 80.0% 0.5s pkg/a + PASS 90.0% 80.0% 0.5s pkg/c + SKIPPED --- --- --- pkg/b + + Total time: 1.0s + All packages meet minimum coverage requirement + +Key user-visible properties: + +- Each excluded package gets one `SKIPPED` row. +- `COVERAGE`, `REQUIRED`, and `TIME` columns are `---` (literal dashes; not `0.0s`). +- `SKIPPED` rows appear grouped at the end of the table, after all measured rows. +- `SKIPPED` packages do not count toward the failure tally — if every measured package passes, the run succeeds even when packages are skipped. +- When `--exclude` is unset or empty, output is byte-identical to today. + +## Progress + +- [x] Milestone 1 — Service: thread excluded list through the run and emit `SKIPPED` rows; ignore skipped packages in the failure tally. (2026-05-15) +- [x] Milestone 2 — Tests: update `TestRun_Exclude` cases to assert `SKIPPED` rows and tally behavior; add an all-excluded case; verify summary line still prints. (2026-05-15) +- [x] Milestone 3 — Validation: run `./scripts/preflight.sh`; iterate until it reports `=== All checks passed ===`. (2026-05-15) + +Use timestamps as steps are completed. Split partially completed work into "done" and "remaining" as needed. + +## Surprises & Discoveries + +- 2026-05-15: `TestPrintResults_UsesWallTime` in `covgate_test.go` directly calls `r.printResults(...)` with the old 3-arg signature. The plan focused on the table-driven `TestRun_Exclude` tests, but the signature change required updating this test too (added `nil` for `excluded` and `modName` for `module`). No behavior change. +- 2026-05-15: Preflight surfaced two lint issues on the new test code that the plan didn't anticipate: + - `exhaustruct` linter required the new `extra` field to be explicitly set on every case (added `extra: nil` on the five non-`OrderingAndCount` cases). + - The custom `collapse` linter flagged a 4-element string slice spread across multiple lines; collapsed to one line. + Both are stylistic fixes — landed in a follow-up `chore(covgate): preflight fixes` commit per the plan's idempotence note. + +## Decision Log + +- Decision: Place `SKIPPED` rows at the **end** of the per-package table, after all measured rows. + Rationale: Skipped packages have no coverage data and would visually intrude on the scannable PASS/FAIL/LOOSE block if interleaved with measured rows. Grouping them at the end keeps the measured results dense and the skipped block easy to scan as a separate "what got dropped" section. The summary line at the top already signals upfront *how many* were skipped; the table tail answers *which ones*. + Date/Author: 2026-05-15, planning subagent. + +- Decision: Return the excluded list from `applyExclude` alongside the kept list, rather than marking-and-skipping inside iteration. + Rationale: Keeps the existing `runPackages` concurrency loop untouched (no synthetic "skip" branch competing with `measure`). Building a separate slice of pre-formatted `SKIPPED` rows and appending them after the measured results in `printResults` is the smallest, most local change. The alternative (a `skipped bool` on `checkResult` plus a branch in `checkPackage`) would mean either feeding fake packages into the goroutine pool or adding a fork in `runPackages`, both of which expand the diff for no observable benefit. + Date/Author: 2026-05-15, planning subagent. + +- Decision: Render the SKIPPED row's coverage, required, and time columns as the literal string `---`. + Rationale: `0.0s` for time is actively misleading (no test ran), and a blank column would mis-align under the existing fixed-width format. `---` is already used elsewhere in this file for the FAIL-with-test-error case (see `covgate.go` line 254) so the convention is established. + Date/Author: 2026-05-15, planning subagent. + +- Decision: Widen the `STATUS` column from 6 to 7 characters so `SKIPPED` fits without breaking alignment. + Rationale: `SKIPPED` is 7 characters; the current format width is 6 (`%-6s`). Keeping the width at 6 would force `SKIPPE` truncation or unaligned output. Update the header, separator, and every status `Fprintf` together so all rows stay aligned. + Date/Author: 2026-05-15, planning subagent. + +## Outcomes & Retrospective + +- 2026-05-15: All three milestones complete. `./scripts/preflight.sh` reports `=== All checks passed ===`. Commits on `feat/covgate-skipped-rows`: + - `70ee0ee` feat(covgate): render --exclude packages as SKIPPED rows + - `6d185db` test(covgate): assert SKIPPED rows render for --exclude and don't fail the run + - `a029d2e` chore(covgate): preflight fixes + The behavior change is purely visual: excluded packages now appear as `SKIPPED` rows at the end of the per-package table with `---` placeholders for COVERAGE/REQUIRED/TIME; STATUS column widened from 6→7 chars for `SKIPPED` alignment; failure tally unchanged (SKIPPED packages do not flip it). + +## Context and Orientation + +Covgate is a Go test-coverage gate exposed as the `covgate` subcommand of the `miru` CLI (built from `cmd/miru/main.go`). All edits in this plan happen in two files inside `/home/ben/miru/workbench2/repos/gotools/`: + +- `internal/services/covgate/covgate.go` — service logic. +- `internal/services/covgate/covgate_test.go` — tests. + +Relevant types and functions in `covgate.go`: + +- `Opts` (lines 15–29) carries options including `Exclude string` (added previously in `20260515-covgate-exclude-flag.md`). +- `runner` (lines 31–36) injects `goModule`, `goListPackages`, `measure` for testability. +- `(*runner).run` (lines 70–117) is the orchestration entry: resolves packages, calls `applyExclude`, prints the header, calls `runPackages`, then `printResults`. +- `applyExclude` (lines 124–161) currently: + - Returns the kept slice and `nil` for empty/whitespace `exclude`. + - For each comma-separated entry, calls `r.goListPackages(entry)` and unions the results into a `map[string]struct{}`. + - Walks `pkgs` keeping the non-excluded ones in original order. + - Prints `Excluded N package(s) from coverage measurement` to `w` when `N > 0`. + - Returns the kept slice and any error. +- `runPackages` (lines 163–181) walks `pkgs` concurrently bounded by `parallelism`, returning a `[]checkResult` indexed by the original slice order. +- `printResults` (lines 183–206) prints each `checkResult.output`, sets `hasFailures` from `!res.passed`, prints `Total time`, and emits the final pass/fail line. +- `printHeader` (lines 208–217) uses the format string `"%-6s %8s %8s %8s %s\n"` with columns `STATUS COVERAGE REQUIRED TIME PACKAGE` and a matching dashed separator. +- `checkResult` (lines 219–224) is `{output string; passed bool; duration time.Duration}`. +- `checkPackage` (lines 236–292) is the per-package measure + formatter. The FAIL-with-test-error branch (lines 252–262) already uses `"---"` for coverage and required when no measurement could be taken; SKIPPED reuses that convention plus `"---"` for time. + +Test fixtures in `covgate_test.go`: + +- `TestRun_Exclude` (lines 167–281) is the existing table-driven test for `--exclude`. The five subtests are `NoExclude`, `Subset`, `NoOpPattern`, `MultiplePatternsWithWhitespace`, and `AllPackages`. Each case asserts `wantContains` (substrings that must appear) and `wantNotContain` (substrings that must NOT appear). +- `TestRun_Exclude_GoListError` (lines 283–317) covers the error path when an exclude pattern fails to resolve. + +Validation: + +- `scripts/preflight.sh` runs `lint.sh`, `covgate.sh`, and `lint-surface.sh` in parallel; the final success line is `=== All checks passed ===`. This is the canonical "preflight" command. + +Glossary: + +- **SKIPPED row**: a single line in the per-package table representing a package that was excluded from measurement by `--exclude`. Format: `SKIPPED --- --- --- `. +- **Failure tally**: the `hasFailures` boolean in `printResults` (line 186). Set when any `checkResult.passed` is false. SKIPPED rows have `passed: true` so they do not flip it. +- **Preflight clean**: `./scripts/preflight.sh` exits 0 and prints `=== All checks passed ===`. +- **Relative package path**: the import path with the module prefix stripped, e.g., `pkg/b` for `example.com/mod/pkg/b`. Computed by `gocover.RelPkg(pkg, module)` in the existing code. + +## Plan of Work + +### Milestone 1 — Service changes (`internal/services/covgate/covgate.go`) + +1. Change `applyExclude` to return both the kept slice and the excluded slice: + + - New signature: `func (r *runner) applyExclude(pkgs []string, exclude string, w io.Writer) (kept []string, excluded []string, err error)`. + - When `exclude` is empty/whitespace, return `pkgs, nil, nil`. + - Otherwise, build the same `excluded` map as today, then in one pass over `pkgs` produce both the `kept` slice (paths not in the map, preserving original order) and the `excluded` slice (paths that are in the map, also in original `pkgs` order so output stays deterministic). + - Keep the existing `Excluded N package(s) from coverage measurement` notice; it fires when `len(excluded) > 0`. + - Update the doc comment to reflect the new return shape. + +2. Update the call site in `(*runner).run` (line 97): + + pkgs, excluded, err := r.applyExclude(pkgs, opts.Exclude, w) + + Pass `excluded` through to `printResults`. + +3. Widen the `STATUS` column from 6 to 7 characters. Edit `printHeader` (lines 208–217) so the format string becomes: + + "%-7s %8s %8s %8s %s\n" + + and update the separator literal `"------"` to `"-------"` (seven dashes). Apply the same `%-7s` width to every status `Fprintf` inside `checkPackage` (the FAIL-tests-failed branch at line 254, the FAIL-below-threshold branch at line 266, the LOOSE branch at line 277, and the PASS branch at line 288). After this edit, every row's first field is left-padded to 7 columns so `SKIPPED` and the existing statuses all align. + +4. Add a small helper that produces a SKIPPED row for a given package import path. Inside `covgate.go`, below `checkPackage`, add: + + // skippedRow formats a single SKIPPED row using the same + // column widths as PASS/FAIL/LOOSE. relPkg is the + // module-relative import path. Used for packages removed + // by --exclude so the user can see which ones were skipped. + func skippedRow(relPkg string) string { + return fmt.Sprintf( + "%-7s %8s %8s %8s %s\n", + "SKIPPED", "---", "---", "---", relPkg, + ) + } + +5. Update `printResults` to accept the module name and the excluded slice, and to emit SKIPPED rows after the measured rows: + + - New signature: `func (r *runner) printResults(w io.Writer, results []checkResult, excluded []string, module string, totalTime time.Duration) error`. + - Existing behavior: print each `res.output`, set `hasFailures` if any `!res.passed`. + - New behavior: after the measured-results loop, iterate `excluded` in order and print `skippedRow(gocover.RelPkg(pkg, module))` for each. Do **not** touch `hasFailures`. + - The remaining tail (`Total time:`, pass/fail line) is unchanged. + +6. Update the caller in `(*runner).run` (line 116): + + return r.printResults(w, results, excluded, module, wallTime) + +7. The `module` value is already in scope at the call site (resolved earlier in `run` from `r.goModule()`), so no additional plumbing is required. + +After these edits, the failure tally only considers `results` (measured packages), and skipped packages affect only the visible table. + +### Milestone 2 — Test changes (`internal/services/covgate/covgate_test.go`) + +Goal: assert `SKIPPED` rows now appear and that the failure tally ignores them. + +1. Update `TestRun_Exclude` cases (lines 167–281). For each subtest, expand `wantContains` / `wantNotContain` so the assertions match the new behavior: + + - **NoExclude**: unchanged. Still expects `pkg/a`, `pkg/b`, `pkg/c` to appear; still expects no `Excluded` notice; still expects no `SKIPPED` row. + - **Subset** (`exclude: "./pkg/b"`): + - `wantContains` adds: `"SKIPPED"`, the substring `"pkg/b"` (it now reappears as a SKIPPED row). + - `wantContains` keeps: `"pkg/a"`, `"pkg/c"`, `"Excluded 1 package(s) from coverage measurement"`. + - `wantNotContain` removes `"pkg/b"` (it is now expected to appear) and **adds** `"FAIL"` (no failures: SKIPPED must not count toward the tally; the `All packages meet minimum coverage requirement` line is the success indicator and is already an implicit assertion via the absence of an error). + - **NoOpPattern**: unchanged. Pattern matches no packages, so no SKIPPED row and no `Excluded` notice. + - **MultiplePatternsWithWhitespace** (`exclude: ", ./pkg/a, , ./pkg/c,"`): + - `wantContains` adds: `"SKIPPED"`, `"pkg/a"`, `"pkg/c"` (both reappear as SKIPPED rows). + - `wantContains` keeps: `"pkg/b"`, `"Excluded 2 package(s) from coverage measurement"`. + - `wantNotContain` removes `"pkg/a"` and `"pkg/c"` (now expected to appear). + - **AllPackages** (`exclude: "./..."`, all three packages excluded): + - `wantContains` adds: `"SKIPPED"`, `"pkg/a"`, `"pkg/b"`, `"pkg/c"` (all three appear as SKIPPED rows). + - `wantContains` keeps: `"Excluded 3 package(s) from coverage measurement"`, `"All packages meet minimum coverage requirement"`, `"Total time:"`. + - `wantNotContain` removes `"pkg/a"`, `"pkg/b"`, `"pkg/c"` (now expected to appear) and **adds** `"FAIL"` (run must succeed — when *all* packages are excluded the tally is empty and pass/fail logic must report success). + +2. Add a new subtest **OrderingAndCount** to `TestRun_Exclude`'s cases table that asserts the SKIPPED rows appear *after* the measured rows. Use `strings.Index` for ordering: + + { + name: "OrderingAndCount", + exclude: "./pkg/b", + lookup: map[string][]string{"./...": allThree, "./pkg/b": {pkgB}}, + // assertions handled below in the test loop, see note + }, + + Because the existing table-driven helper does substring `Contains` only, add a small `extra func(t *testing.T, out string)` hook on the case struct (default `nil`) and call it from the loop after the contains/not-contains assertions. For this case, the hook asserts: + + - `strings.Index(out, "pkg/a") < strings.Index(out, "SKIPPED")` — measured pkg/a row precedes the SKIPPED block. + - `strings.Index(out, "pkg/c") < strings.Index(out, "SKIPPED")` — measured pkg/c row precedes the SKIPPED block. + - The substring `"SKIPPED"` appears exactly once (so we are not accidentally double-rendering). + - The line containing `"pkg/b"` also contains `"SKIPPED"` and the three `"---"` placeholders, confirming the row format. The simplest implementation scans `strings.Split(out, "\n")` for the line containing `pkg/b` and asserts it matches the expected pattern (contains `SKIPPED`, contains `---`). + +3. Keep `TestRun_Exclude_GoListError` (lines 283–317) unchanged — the error path is independent of the row-rendering change. + +4. Sanity-check that no other tests in the file rely on the old `STATUS` column width of 6. Grep mentally: existing tests use `strings.Contains` against substrings like `"PASS"`, `"FAIL"`, `"LOOSE"`, `"0.0%"`, `"80.0%"`, `"TIME"`, `"pkg/a"` — none of those care about column padding. `TestPrintHeader` (lines 17–31) checks `strings.Contains(out, "------")`; six dashes still appear as a substring of seven dashes, so the assertion remains true after the width change. No edits required there. + +### Milestone 3 — Validation + +Run preflight from the gotools repo root and iterate until clean. See "Concrete Steps" for exact commands. + +## Concrete Steps + +All commands run from `/home/ben/miru/workbench2/repos/gotools/`. The current branch is `feat/covgate-skipped-rows` (already created upstream by the orchestrator). Do not switch branches. + +### Step 0 — Sanity baseline + + git status + go build ./... + go test ./internal/services/covgate/... + +Expected: clean working tree (this plan file may be untracked/added), build exits 0 silently, tests pass. + +### Step 1 — Implement Milestone 1 (service edits) + +Edit `internal/services/covgate/covgate.go` per "Plan of Work — Milestone 1". + +Then: + + go build ./internal/services/covgate/... + +Expected: exits 0 with no output. + +Run the existing tests — many will now fail because their expectations have not been updated yet: + + go test ./internal/services/covgate/... + +Expected: failures in `TestRun_Exclude/Subset`, `TestRun_Exclude/MultiplePatternsWithWhitespace`, and `TestRun_Exclude/AllPackages` because their `wantNotContain` lists still reject `pkg/a`/`pkg/b`/`pkg/c` substrings which now reappear as SKIPPED rows. This is expected — the test updates land in Step 2. + +Commit: + + git add internal/services/covgate/covgate.go + git commit -m "feat(covgate): render --exclude packages as SKIPPED rows" + +### Step 2 — Implement Milestone 2 (test updates) + +Edit `internal/services/covgate/covgate_test.go` per "Plan of Work — Milestone 2". + + go test ./internal/services/covgate/... + +Expected: `ok github.com/mirurobotics/gotools/internal/services/covgate` with no failures. All five updated cases plus the new `OrderingAndCount` case pass. + +Commit: + + git add internal/services/covgate/covgate_test.go + git commit -m "test(covgate): assert SKIPPED rows render for --exclude and don't fail the run" + +### Step 3 — Milestone 3 preflight + + ./scripts/preflight.sh + +Expected final line: `=== All checks passed ===` with exit 0. + +If preflight is not clean, read the offending output (lint, covgate, or surface lint), apply fixes in place, and rerun. Common items: gofumpt format, golangci-lint findings on the new helper, surface-lint reaction to any newly exported symbol (this plan adds none — `skippedRow` is lowercase and `printResults`'s new args do not change its export status because it is already lowercase). + +If fixes were applied, commit them: + + git add -A + git commit -m "chore(covgate): preflight fixes" + +Skip this commit if preflight was clean on the first run. + +## Validation and Acceptance + +Acceptance is observable behavior plus a clean preflight, in this order. **Changes MUST NOT be published (no push, no PR) until preflight reports clean — preflight clean is the gate.** + +1. **Service tests pass.** From `gotools/`: + + go test ./internal/services/covgate/... + + Expected: `ok` line. Specifically: + - `TestRun_Exclude/NoExclude` — passes unchanged. + - `TestRun_Exclude/Subset` — passes with the updated assertions (`SKIPPED` present, `pkg/b` reappears as a SKIPPED row, no `FAIL`). + - `TestRun_Exclude/NoOpPattern` — passes unchanged (no SKIPPED row when pattern matches nothing). + - `TestRun_Exclude/MultiplePatternsWithWhitespace` — passes (`pkg/a` and `pkg/c` reappear as SKIPPED rows). + - `TestRun_Exclude/AllPackages` — passes (all three packages appear as SKIPPED rows; run succeeds; tally not flipped). + - `TestRun_Exclude/OrderingAndCount` — new subtest passes: SKIPPED block is at the end, exactly one `SKIPPED` substring, row contains the `---` placeholders. + - `TestRun_Exclude_GoListError` — passes unchanged. + - All other existing tests in the file still pass; the STATUS-column widening from 6 to 7 must not break any substring-based assertion. + + Before this change the new subtest does not exist; after, it passes. The updated subtests fail under the old assertions (because `SKIPPED` rows reintroduce excluded package names) and pass under the new ones. + +2. **Build is clean.** + + go build ./... + + Expected: exit 0, no output. + +3. **Preflight is clean — mandatory gate.** From `gotools/`: + + ./scripts/preflight.sh + + Expected final line: `=== All checks passed ===` with exit 0. **No changes are published, no PR is opened, and no branch is pushed until this command reports clean.** If it fails on any of lint, covgate, or surface lint, fix the underlying issue and rerun. Do not bypass preflight. + +4. **End-to-end smoke (manual, optional).** Build the `miru` binary and run against the gotools repo itself: + + go build -o /tmp/miru ./cmd/miru + /tmp/miru covgate --packages ./... --exclude ./internal/services/gocover/... + + Expected: the output contains `Excluded N package(s) from coverage measurement` for some `N >= 1`, and the table contains one `SKIPPED` row per excluded gocover package, each with `--- --- ---` in the coverage/required/time columns. The overall run succeeds (exit 0). Without `--exclude`, the gocover packages appear as PASS/FAIL rows as before. + +## Idempotence and Recovery + +- Service edits (Milestone 1): purely additive in semantics. If a partial edit leaves the file in a broken state, `git restore internal/services/covgate/covgate.go` recovers the pre-edit version; re-apply from the Plan of Work. +- Test edits (Milestone 2): if a partial edit leaves stale assertions, `git restore internal/services/covgate/covgate_test.go` recovers the previous state; re-apply. +- Preflight (Milestone 3): idempotent — re-running `./scripts/preflight.sh` repeatedly is safe and always reflects the current working tree. +- Commits: one commit per milestone per Concrete Steps. If a commit needs to be redone, prefer a new commit over `git commit --amend` so milestone boundaries stay visible in PR review. Do **not** use `git reset --hard` because it discards work in unrelated files; prefer `git restore ` on specific files. +- Rollback path: revert milestone commits in reverse order with `git revert `. The change is purely visual (a new row type and a wider STATUS column) with no semantics change for callers that do not parse covgate output; reverting is low-risk.