From 1b23bb77592eef37bd216c7c1460fb58722dc4a2 Mon Sep 17 00:00:00 2001 From: Ben Wisecup Date: Wed, 5 Aug 2026 15:03:38 -0400 Subject: [PATCH 1/2] fix(doctor): name the check groups dropped when config load fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gc doctor` gates seven groups of checks on the city config loading cleanly. When that load fails, every pack, pack-source, config-validation, rig, data, session and Dolt-ops check disappears without a trace. The run prints a shorter summary that names the config error itself but says nothing about which families of checks were dropped, so a summary reading "36 passed" can describe a factory that was barely inspected. Measured on a city whose config does not load, before and after: before 41 checks, no skipped count, none of the dropped groups named after 48 checks, "7 check groups skipped", every group named Worth stating plainly, because it narrows what this actually fixes: the exit code was already non-zero in that state. The pre-existing expanded-config-load check fails blocking whenever the config cannot be resolved. What was missing is the inventory, not the exit status. Two halves: Surface it. Each config-dependent group now registers a blocking SkippedCheck naming what did not run, the summary states how many groups were skipped, and the run ends with an explicit line saying the factory was not fully inspected. The underlying config error is carried as a detail rather than repeated on all seven lines, where it buried the group names it was meant to explain. Remove the cause. Pack scripts run with their working directory set to the pack dir, which for an imported pack lives inside gc's global import cache. gc exported GC_CITY_PATH but never FACTORY_ROOT, so a script resolving its state dir against FACTORY_ROOT fell back to the working directory and wrote into the cache clone. That left the clone dirty, and the next gc invocation refused the dirty cache, failed config load, and dropped every pack check, which is the loop that produced the numbers above. PackRuntimeEnv now exports FACTORY_ROOT. Both halves carry positive and negative controls: the skip must appear when config load fails and must stay absent when it succeeds. Each was verified by injecting the defect and confirming the test goes red. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-3 On behalf of: @benw5483 Co-Authored-By: --- cmd/gc/cmd_doctor.go | 72 +++++++- cmd/gc/cmd_doctor_config_skips_test.go | 134 ++++++++++++++ internal/citylayout/runtime.go | 12 ++ .../citylayout/runtime_factory_root_test.go | 56 ++++++ internal/doctor/doctor.go | 26 +++ internal/doctor/pack_checks.go | 4 + internal/doctor/skipped_check.go | 78 ++++++++ internal/doctor/skipped_check_test.go | 173 ++++++++++++++++++ internal/doctor/types.go | 8 + 9 files changed, 562 insertions(+), 1 deletion(-) create mode 100644 cmd/gc/cmd_doctor_config_skips_test.go create mode 100644 internal/citylayout/runtime_factory_root_test.go create mode 100644 internal/doctor/skipped_check.go create mode 100644 internal/doctor/skipped_check_test.go diff --git a/cmd/gc/cmd_doctor.go b/cmd/gc/cmd_doctor.go index 0161263207..83603e19e8 100644 --- a/cmd/gc/cmd_doctor.go +++ b/cmd/gc/cmd_doctor.go @@ -194,6 +194,64 @@ func doctorOrderFiringCurrentLastRunFunc(cityPath string, cfg *config.City, stde } } +// configDependentCheckGroups names the check families that only register when +// the city config loads cleanly. When it does not, each one registers a +// blocking skip result instead of vanishing. +// +// The list is ordered for the reader rather than for buildDoctorChecks: the +// pack-doctor entry leads because it is the group operators are most likely +// to assume ran, being the checks a pack ships to watch the factory's own +// moving parts. +// +// Maintenance contract: every `cfgErr == nil` gated register block in +// buildDoctorChecks must be covered by an entry here. A gated block with no +// entry is dropped silently on a failed config load, which is the exact +// defect this list exists to close. +var configDependentCheckGroups = []struct{ name, covers string }{ + {"pack-doctor-checks", "every check script shipped by an imported pack"}, + {"pack-source-checks", "pack import cache freshness and pack-source credential rules"}, + {"config-validation-checks", "config validity, references, semantics, and provider parity"}, + {"rig-checks", "per-rig path, git, branch, beads, and Dolt checks"}, + {"data-checks", "beads store, split-store, backlog depth, and order retention"}, + {"session-checks", "agent, zombie, and orphan session checks"}, + {"dolt-ops-checks", "Dolt topology, drift, and Postgres auth checks"}, +} + +// registerConfigDependentSkips records one blocking skip per config-dependent +// check group. It runs when the city config fails to load, which is the state +// that used to drop those groups silently: `gc doctor` printed a shorter +// summary that read healthier than the previous run, because the checks that +// had been failing disappeared along with everything else. +// +// A stray worktree change in gc's pack import cache is the common trigger, so +// the fix hint names the command that repairs it. +func registerConfigDependentSkips(register func(doctor.Check), cityPath string, cfgErr error) { + // The cause goes in the details, not the message: it is the same error on + // every line, and repeating it six times buries the group names. + var details []string + if cfgErr != nil { + details = []string{fmt.Sprintf("config load error: %v", cfgErr)} + } + const hint = "fix the config error above, then rerun gc doctor; if it names a cached import with local worktree changes, run \"gc import install\" to restore the cache" + + // Re-read the config when the check runs, not now. Under --fix an earlier + // check may repair the config that caused these skips (the v2 migration + // fixes do exactly that), and a skip that still reported a hard failure + // afterwards would be blaming a problem that no longer exists. + stillBroken := func() bool { + _, err := loadCityConfig(cityPath, io.Discard) + return err != nil + } + + for _, group := range configDependentCheckGroups { + register(doctor.SkippedCheck(group.name, + fmt.Sprintf("%s; the city config did not load", group.covers), + hint, + stillBroken, + details...)) + } +} + func buildDoctorChecks(cityPath string, cfg *config.City, cfgErr error, opts buildDoctorChecksOpts) []doctor.Check { var checks []doctor.Check register := func(c doctor.Check) { @@ -218,7 +276,13 @@ func buildDoctorChecks(cityPath string, cfg *config.City, cfgErr error, opts bui register(&doctor.DeprecatedAttachmentFieldsCheck{}) // Config-dependent checks run only when city.toml loaded cleanly. If it - // fails, the core config check above reports the parse error. + // fails, the core config check above reports the parse error — but the + // dropped groups also register a blocking skip result each, so the run + // cannot finish looking healthy just because most of the factory went + // uninspected. See registerConfigDependentSkips. + if cfgErr != nil || cfg == nil { + registerConfigDependentSkips(register, cityPath, cfgErr) + } if cfgErr == nil && cfg != nil { resolveRigPaths(cityPath, cfg.Rigs) if workspaceUsesManagedBdStoreContract(cityPath, cfg.Rigs) { @@ -591,6 +655,9 @@ type doctorJSONResult struct { // distinguish an abandoned check (outcome unknown, worth retrying) from a // check that ran and returned an ordinary advisory error. TimedOut bool `json:"timed_out,omitempty"` + // Skipped projects CheckResult.Skipped so automation can tell a group + // that never ran from a check that ran and failed. + Skipped bool `json:"skipped,omitempty"` } type doctorJSONReport struct { @@ -599,6 +666,7 @@ type doctorJSONReport struct { Failed int `json:"failed"` BlockingFailed int `json:"blocking_failed"` Fixed int `json:"fixed"` + Skipped int `json:"skipped"` Results []doctorJSONResult `json:"results"` Error string `json:"error,omitempty"` } @@ -632,6 +700,7 @@ func writeDoctorJSON(w io.Writer, report *doctor.Report) error { Failed: report.Failed, BlockingFailed: report.BlockingFailed, Fixed: report.Fixed, + Skipped: report.Skipped, Results: make([]doctorJSONResult, 0, len(report.Results)), } for _, r := range report.Results { @@ -646,6 +715,7 @@ func writeDoctorJSON(w io.Writer, report *doctor.Report) error { FixError: r.FixError, Fixed: r.Fixed, TimedOut: r.TimedOut, + Skipped: r.Skipped, }) } return writeCLIJSONLine(w, out) diff --git a/cmd/gc/cmd_doctor_config_skips_test.go b/cmd/gc/cmd_doctor_config_skips_test.go new file mode 100644 index 0000000000..43c3cd030f --- /dev/null +++ b/cmd/gc/cmd_doctor_config_skips_test.go @@ -0,0 +1,134 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gastownhall/gascity/internal/config" + "github.com/gastownhall/gascity/internal/doctor" +) + +// TestConfigDependentCheckGroupsAreDeclared pins the group list against an +// independent copy of the names. +// +// The other two tests in this file both iterate configDependentCheckGroups, so +// the list is simultaneously the thing under test and the oracle. Deleting an +// entry makes them test less while still passing, which is the failure mode a +// list-as-its-own-oracle always has. This test is the second source of truth: +// remove or rename a group and it fails here. +// +// What it deliberately does NOT cover: a config-gated register block added to +// buildDoctorChecks with no matching entry here. That block is still dropped +// silently, and catching it needs a tripwire over the gated blocks themselves +// rather than over this list. See the maintenance contract on +// configDependentCheckGroups. +func TestConfigDependentCheckGroupsAreDeclared(t *testing.T) { + t.Parallel() + + want := []string{ + "pack-doctor-checks", + "pack-source-checks", + "config-validation-checks", + "rig-checks", + "data-checks", + "session-checks", + "dolt-ops-checks", + } + got := make([]string, 0, len(configDependentCheckGroups)) + for _, group := range configDependentCheckGroups { + got = append(got, group.name) + if strings.TrimSpace(group.covers) == "" { + t.Errorf("group %q has an empty covers string; the skip message names it to the operator", group.name) + } + } + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("configDependentCheckGroups changed\n got: %v\nwant: %v\nIf a config-gated group was added or removed, update this list too, and check the skip still names every gated block.", got, want) + } +} + +// TestBuildDoctorChecksRegistersSkipsWhenConfigFails is the positive control +// for the silent-drop fix. A dirty pack import cache fails config load, which +// used to drop every pack, rig, and data check with no trace: the run printed +// roughly half as many checks and read healthier, because the failing ones +// left with the rest. Each dropped group must now leave a blocking result. +func TestBuildDoctorChecksRegistersSkipsWhenConfigFails(t *testing.T) { + // The config must be genuinely unloadable, not merely reported as such by + // the cfgErr argument: the skip re-reads the config when it runs, so that + // a cause repaired mid-run downgrades to a warning. Broken TOML keeps the + // cause unresolved, which is the dirty-cache case being pinned here. + cityDir := t.TempDir() + if err := os.WriteFile(filepath.Join(cityDir, "city.toml"), []byte("[workspace\nname = \"demo\"\n"), 0o644); err != nil { + t.Fatalf("write city.toml: %v", err) + } + + // The Dolt skips travel through opts, not the process environment: + // buildDoctorChecks never reads GC_DOLT (only runDoctor does, to populate + // these fields). So neither test here mutates the environment, which keeps + // them out of the cmd/gc environment debt ratchet in test/test-resources.toml. + cfgErr := errors.New(`city import "local-core": cached import file:///packs/local-core has local worktree changes; run "gc import install"`) + checks := buildDoctorChecks(cityDir, nil, cfgErr, buildDoctorChecksOpts{ + SkipCityDoltCheck: true, + SkipManagedDoltCheck: true, + }) + names := doctorCheckNames(checks) + + for _, group := range configDependentCheckGroups { + if doctorCheckIndex(names, group.name) < 0 { + t.Errorf("%s not registered; a dropped group must be named, not silently omitted. names=%v", group.name, names) + } + } + + // The result must gate the exit code and carry the underlying cause, + // otherwise `gc doctor` still exits 0 on an uninspected factory. + var found bool + for _, c := range checks { + if c.Name() != "pack-doctor-checks" { + continue + } + found = true + r := c.Run(&doctor.CheckContext{}) + if r.Status != doctor.StatusError || r.Severity != doctor.SeverityBlocking { + t.Errorf("pack-doctor-checks status/severity = %v/%v, want error/blocking", r.Status, r.Severity) + } + if !r.Skipped { + t.Error("pack-doctor-checks Skipped = false, want true") + } + if !strings.Contains(r.Message, "did not load") { + t.Errorf("message = %q, want it to name the config load as the cause", r.Message) + } + if !strings.Contains(strings.Join(r.Details, "\n"), "local worktree changes") { + t.Errorf("details = %q, want the underlying config-load error preserved", r.Details) + } + if !strings.Contains(r.FixHint, "gc import install") { + t.Errorf("fix hint = %q, want it to name the cache repair command", r.FixHint) + } + } + if !found { + t.Fatal("pack-doctor-checks check not found") + } +} + +// TestBuildDoctorChecksNoSkipsWhenConfigLoads is the negative control: on a +// healthy city no skip result may appear, or every clean run would claim the +// factory went uninspected and `gc doctor` would never exit 0 again. +func TestBuildDoctorChecksNoSkipsWhenConfigLoads(t *testing.T) { + cityDir := t.TempDir() + if err := os.WriteFile(filepath.Join(cityDir, "city.toml"), []byte("[workspace]\nname = \"demo\"\n"), 0o644); err != nil { + t.Fatalf("write city.toml: %v", err) + } + + cfg := &config.City{Workspace: config.Workspace{Name: "demo"}} + names := doctorCheckNames(buildDoctorChecks(cityDir, cfg, nil, buildDoctorChecksOpts{ + SkipCityDoltCheck: true, + SkipManagedDoltCheck: true, + })) + + for _, group := range configDependentCheckGroups { + if idx := doctorCheckIndex(names, group.name); idx >= 0 { + t.Errorf("%s registered at %d on a healthy config, want absent", group.name, idx) + } + } +} diff --git a/internal/citylayout/runtime.go b/internal/citylayout/runtime.go index b65ea47c45..f2aba12b90 100644 --- a/internal/citylayout/runtime.go +++ b/internal/citylayout/runtime.go @@ -169,8 +169,18 @@ func CityIdentityEnvMap(cityRoot string) map[string]string { } // PackRuntimeEnv returns city runtime env vars plus the canonical pack state dir. +// +// FACTORY_ROOT is exported alongside the GC_* vars because pack scripts are +// launched with their working directory set to the pack directory, which for +// an imported pack lives inside gc's global import cache rather than in the +// city tree. A script that resolves its state directory against FACTORY_ROOT +// with a fall back to the working directory therefore wrote into the cache +// clone, leaving it dirty; the next gc invocation then refused the dirty +// cache, failed config load, and dropped every pack check. Handing the script +// the city root closes that loop at the source. func PackRuntimeEnv(cityRoot, packName string) []string { env := CityRuntimeEnv(cityRoot) + env = append(env, "FACTORY_ROOT="+cityRoot) if packName != "" { env = append(env, "GC_PACK_STATE_DIR="+PackStateDir(cityRoot, packName)) } @@ -178,8 +188,10 @@ func PackRuntimeEnv(cityRoot, packName string) []string { } // PackRuntimeEnvMap returns city runtime env vars plus the canonical pack state dir. +// FACTORY_ROOT is included for the reason documented on PackRuntimeEnv. func PackRuntimeEnvMap(cityRoot, packName string) map[string]string { env := CityRuntimeEnvMap(cityRoot) + env["FACTORY_ROOT"] = cityRoot if packName != "" { env["GC_PACK_STATE_DIR"] = PackStateDir(cityRoot, packName) } diff --git a/internal/citylayout/runtime_factory_root_test.go b/internal/citylayout/runtime_factory_root_test.go new file mode 100644 index 0000000000..d7039c50d2 --- /dev/null +++ b/internal/citylayout/runtime_factory_root_test.go @@ -0,0 +1,56 @@ +package citylayout + +import ( + "strings" + "testing" +) + +// TestPackRuntimeEnvExportsFactoryRoot pins the cause-side half of the +// dirty-cache loop. Pack scripts run with their working directory set to the +// pack dir, which for an imported pack lives inside gc's global import cache. +// A script that resolves its state dir against FACTORY_ROOT and falls back to +// the working directory therefore wrote into the cache clone and left it +// dirty, and the next gc invocation refused the dirty cache and dropped every +// pack check. Exporting FACTORY_ROOT keeps that fallback from ever firing. +func TestPackRuntimeEnvExportsFactoryRoot(t *testing.T) { + const cityRoot = "/tmp/some-city" + + env := PackRuntimeEnv(cityRoot, "local-core") + + var got string + var found bool + for _, kv := range env { + if k, v, ok := strings.Cut(kv, "="); ok && k == "FACTORY_ROOT" { + got, found = v, true + } + } + if !found { + t.Fatalf("FACTORY_ROOT not exported to pack scripts; env=%v", env) + } + if got != cityRoot { + t.Errorf("FACTORY_ROOT = %q, want the city root %q", got, cityRoot) + } +} + +func TestPackRuntimeEnvMapExportsFactoryRoot(t *testing.T) { + const cityRoot = "/tmp/some-city" + + env := PackRuntimeEnvMap(cityRoot, "local-core") + + if got := env["FACTORY_ROOT"]; got != cityRoot { + t.Errorf("FACTORY_ROOT = %q, want the city root %q", got, cityRoot) + } +} + +// TestPackRuntimeEnvExportsFactoryRootWithoutPackName covers the unnamed-pack +// path: GC_PACK_STATE_DIR is conditional on the pack name, FACTORY_ROOT is not. +func TestPackRuntimeEnvExportsFactoryRootWithoutPackName(t *testing.T) { + const cityRoot = "/tmp/some-city" + + for _, kv := range PackRuntimeEnv(cityRoot, "") { + if kv == "FACTORY_ROOT="+cityRoot { + return + } + } + t.Errorf("FACTORY_ROOT not exported when packName is empty") +} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 48c56086c0..6733bc3f57 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -27,6 +27,11 @@ type Report struct { BlockingFailed int // Fixed is the number of checks remediated by --fix. Fixed int + // Skipped is the number of check groups that never ran, as reported by + // SkippedCheck. These are also counted in Failed and BlockingFailed — + // a group gc could not inspect gates the exit code the same way a + // failing check does. + Skipped int // Results holds the per-check results in the order they ran. Populated // by Run so callers that need structured output (e.g. `gc doctor --json`) // can project every result without re-running checks. @@ -121,6 +126,14 @@ func (d *Doctor) run(ctx *CheckContext, w io.Writer, fix, stream bool) *Report { // check counts as passed; a failing check increments BlockingFailed only when // its severity gates. func (r *Report) tally(result *CheckResult) { + // Counted outside the switch: a skipped group reports as an error when + // its cause is unresolved and as a warning when something repaired the + // cause mid-run. Either way the group did not run, so the summary must + // say so. + if result.Skipped && !result.Fixed { + r.Skipped++ + } + switch { case result.Fixed: r.Fixed++ @@ -288,6 +301,13 @@ func PrintSummary(w io.Writer, r *Report) { if r.Fixed > 0 { parts = append(parts, fmt.Sprintf("%d fixed", r.Fixed)) } + if r.Skipped > 0 { + noun := "check groups" + if r.Skipped == 1 { + noun = "check group" + } + parts = append(parts, fmt.Sprintf("%d %s skipped", r.Skipped, noun)) + } if len(parts) == 0 { fmt.Fprintln(w, "\nNo checks ran.") //nolint:errcheck // best-effort output return @@ -300,4 +320,10 @@ func PrintSummary(w io.Writer, r *Report) { fmt.Fprintf(w, "%s", p) //nolint:errcheck // best-effort output } fmt.Fprintf(w, "\n") //nolint:errcheck // best-effort output + if r.Skipped > 0 { + // Without this line the count above reads as a clean bill of health + // on a smaller factory, which is exactly how a dropped check group + // goes unnoticed. + fmt.Fprintf(w, "This factory was not fully inspected — see the skipped groups above.\n") //nolint:errcheck // best-effort output + } } diff --git a/internal/doctor/pack_checks.go b/internal/doctor/pack_checks.go index 6662b9016f..dcba9b0102 100644 --- a/internal/doctor/pack_checks.go +++ b/internal/doctor/pack_checks.go @@ -19,6 +19,10 @@ import ( // The script receives environment variables: // // GC_CITY_PATH — absolute path to the city root +// FACTORY_ROOT — absolute path to the city root, for scripts that key +// off it; set so a script never falls back to the working +// directory, which is the pack dir inside gc's import +// cache and must not be written to // GC_PACK_DIR — absolute path to the pack directory // // When FixScript is non-empty, the check also supports `gc doctor --fix`: diff --git a/internal/doctor/skipped_check.go b/internal/doctor/skipped_check.go new file mode 100644 index 0000000000..70c8f3d2af --- /dev/null +++ b/internal/doctor/skipped_check.go @@ -0,0 +1,78 @@ +package doctor + +import "fmt" + +// SkippedCheck returns a Check that reports a group of checks gc could not +// run at all. It exists so a check group that is dropped before it registers +// still leaves a visible result behind. +// +// The motivating failure: `gc doctor` gates seven groups of checks on the city +// config loading cleanly, and when that load fails every pack, pack-source, +// rig, data, session and Dolt-ops check silently disappears. The usual cause is +// the pack import cache picking up a stray worktree change. The run then prints +// a shorter summary that also reads healthier, because the checks that were +// failing left along with the rest, so an operator reading it has no way to +// tell the factory went largely uninspected. +// +// stillBroken decides the severity, and is evaluated when the check runs +// rather than when it is registered. That distinction matters under --fix: the +// check list is built once, up front, from a config that failed to load, but a +// fix earlier in the same run may repair the very config that caused the skip. +// Reporting a hard failure on that repaired run would be stale, and would make +// a successful `gc doctor --fix` exit non-zero. +// +// - stillBroken() true — the config is genuinely unusable, nothing inspected +// these groups, and nothing repaired them. StatusError with +// SeverityBlocking, so the run cannot exit 0 on an uninspected factory. +// That is the whole point of the check. +// - stillBroken() false — something fixed the config mid-run. These groups +// still did not run, so the result is not OK, but the honest report is a +// warning telling the operator to rerun rather than a failure blaming a +// problem that no longer exists. +// +// A nil stillBroken means "assume still broken", the conservative default. +// +// Any details are shown under --verbose. Callers should put the underlying +// cause there rather than in the reason when several groups share one cause: +// repeating a long config-load error on every skipped line buries the names +// of the groups themselves, which is the part the operator needs to see. +func SkippedCheck(name, reason, fixHint string, stillBroken func() bool, details ...string) Check { + return &skippedCheck{name: name, reason: reason, fixHint: fixHint, stillBroken: stillBroken, details: details} +} + +type skippedCheck struct { + name string + reason string + fixHint string + stillBroken func() bool + details []string +} + +func (c *skippedCheck) Name() string { return c.name } + +func (c *skippedCheck) CanFix() bool { return false } + +func (c *skippedCheck) WarmupEligible() bool { return false } + +func (c *skippedCheck) Fix(_ *CheckContext) error { return nil } + +func (c *skippedCheck) Run(_ *CheckContext) *CheckResult { + if c.stillBroken != nil && !c.stillBroken() { + return &CheckResult{ + Name: c.name, + Status: StatusWarning, + Skipped: true, + Message: fmt.Sprintf("not run this pass — %s; the config loads now, so rerun gc doctor to inspect them", c.reason), + Details: c.details, + } + } + return &CheckResult{ + Name: c.name, + Status: StatusError, + Severity: SeverityBlocking, + Skipped: true, + Message: fmt.Sprintf("not run — %s", c.reason), + Details: c.details, + FixHint: c.fixHint, + } +} diff --git a/internal/doctor/skipped_check_test.go b/internal/doctor/skipped_check_test.go new file mode 100644 index 0000000000..4448a5e13e --- /dev/null +++ b/internal/doctor/skipped_check_test.go @@ -0,0 +1,173 @@ +package doctor + +import ( + "bytes" + "strings" + "testing" +) + +func TestSkippedCheckIsBlockingFailure(t *testing.T) { + c := SkippedCheck("pack-doctor-checks", "the city config did not load", "fix the config", nil, "config load error: boom") + got := c.Run(&CheckContext{}) + + if got.Status != StatusError { + t.Errorf("status = %v, want StatusError; a skipped group must not read as a pass", got.Status) + } + if got.Severity != SeverityBlocking { + t.Errorf("severity = %v, want SeverityBlocking; an advisory skip would still exit 0", got.Severity) + } + if !got.Skipped { + t.Error("Skipped = false, want true") + } + if !strings.Contains(got.Message, "not run") { + t.Errorf("message = %q, want it to say the group did not run", got.Message) + } + if !strings.Contains(got.Message, "the city config did not load") { + t.Errorf("message = %q, want the reason preserved", got.Message) + } + if len(got.Details) != 1 || !strings.Contains(got.Details[0], "boom") { + t.Errorf("details = %q, want the underlying cause carried as a detail", got.Details) + } + if got.FixHint != "fix the config" { + t.Errorf("fix hint = %q, want it carried through", got.FixHint) + } + if c.CanFix() { + t.Error("CanFix = true, want false; a skipped group has nothing to remediate") + } +} + +// TestSkippedCheckWarnsWhenCauseRepairedMidRun covers the --fix interaction. +// The check list is built once, up front, from a config that failed to load; +// a fix earlier in the same run can repair that config. The skipped group +// still did not run, so this must not read as a pass — but blaming a config +// error that no longer exists would be stale, and would make a successful +// `gc doctor --fix` exit non-zero. +func TestSkippedCheckWarnsWhenCauseRepairedMidRun(t *testing.T) { + repaired := func() bool { return false } // no longer broken + c := SkippedCheck("pack-doctor-checks", "the city config did not load", "fix the config", repaired) + + got := c.Run(&CheckContext{}) + + if got.Status != StatusWarning { + t.Errorf("status = %v, want StatusWarning once the cause is repaired", got.Status) + } + if got.Severity == SeverityBlocking && got.Status == StatusError { + t.Error("a repaired-cause skip must not gate the exit code") + } + if !got.Skipped { + t.Error("Skipped = false, want true; the group still did not run") + } + if !strings.Contains(got.Message, "rerun") { + t.Errorf("message = %q, want it to tell the operator to rerun", got.Message) + } +} + +// TestSkippedCheckStillBlocksWhenCausePersists is the paired positive control: +// the run-time recheck must not soften a skip whose cause is unresolved, +// which is the dirty-import-cache case this whole check exists for. +func TestSkippedCheckStillBlocksWhenCausePersists(t *testing.T) { + broken := func() bool { return true } + c := SkippedCheck("pack-doctor-checks", "the city config did not load", "fix the config", broken) + + got := c.Run(&CheckContext{}) + + if got.Status != StatusError || got.Severity != SeverityBlocking { + t.Errorf("status/severity = %v/%v, want error/blocking while the cause persists", got.Status, got.Severity) + } +} + +// TestReportCountsSkippedGroupsWhenWarning pins that the summary still counts +// a skipped group that downgraded to a warning — it did not run either way. +func TestReportCountsSkippedGroupsWhenWarning(t *testing.T) { + repaired := func() bool { return false } + d := &Doctor{} + d.Register(SkippedCheck("pack-doctor-checks", "config did not load", "", repaired)) + + r := d.RunCollect(&CheckContext{}, false) + + if r.Skipped != 1 { + t.Errorf("Skipped = %d, want 1", r.Skipped) + } + if r.Warned != 1 { + t.Errorf("Warned = %d, want 1", r.Warned) + } + if r.BlockingFailed != 0 { + t.Errorf("BlockingFailed = %d, want 0; a repaired cause must not gate the exit code", r.BlockingFailed) + } +} + +// TestReportCountsSkippedGroups pins the accounting that makes a dropped +// group visible: it is counted as skipped AND as a blocking failure, so the +// summary names it and the exit code reflects it. +func TestReportCountsSkippedGroups(t *testing.T) { + d := &Doctor{} + d.Register(&mockCheck{name: "ran-fine", status: StatusOK, msg: "ok"}) + d.Register(SkippedCheck("pack-doctor-checks", "config did not load", "", nil)) + d.Register(SkippedCheck("rig-checks", "config did not load", "", nil)) + + r := d.RunCollect(&CheckContext{}, false) + + if r.Skipped != 2 { + t.Errorf("Skipped = %d, want 2", r.Skipped) + } + if r.BlockingFailed != 2 { + t.Errorf("BlockingFailed = %d, want 2; skipped groups must gate the exit code", r.BlockingFailed) + } + if r.Passed != 1 { + t.Errorf("Passed = %d, want 1", r.Passed) + } +} + +// TestReportSkippedZeroWhenNothingSkipped is the negative control for the +// counter: an ordinary failing check must not inflate the skipped count, or +// every red run would claim the factory went uninspected. +func TestReportSkippedZeroWhenNothingSkipped(t *testing.T) { + d := &Doctor{} + d.Register(&mockCheck{name: "ran-fine", status: StatusOK, msg: "ok"}) + d.Register(&mockCheck{name: "ran-and-failed", status: StatusError, msg: "broken"}) + + r := d.RunCollect(&CheckContext{}, false) + + if r.Skipped != 0 { + t.Errorf("Skipped = %d, want 0; a check that ran and failed is not a skipped group", r.Skipped) + } + if r.Failed != 1 { + t.Errorf("Failed = %d, want 1", r.Failed) + } +} + +func TestPrintSummaryReportsSkippedGroups(t *testing.T) { + var buf bytes.Buffer + PrintSummary(&buf, &Report{Passed: 32, Failed: 6, BlockingFailed: 6, Skipped: 6}) + out := buf.String() + + if !strings.Contains(out, "6 check groups skipped") { + t.Errorf("summary = %q, want it to state how many groups were skipped", out) + } + if !strings.Contains(out, "not fully inspected") { + t.Errorf("summary = %q, want an explicit warning that the run was partial", out) + } +} + +func TestPrintSummarySkippedSingular(t *testing.T) { + var buf bytes.Buffer + PrintSummary(&buf, &Report{Passed: 1, Failed: 1, BlockingFailed: 1, Skipped: 1}) + if out := buf.String(); !strings.Contains(out, "1 check group skipped") { + t.Errorf("summary = %q, want singular phrasing", out) + } +} + +// TestPrintSummaryQuietWhenNothingSkipped is the negative control for the +// summary: a healthy run must not carry the partial-inspection warning. +func TestPrintSummaryQuietWhenNothingSkipped(t *testing.T) { + var buf bytes.Buffer + PrintSummary(&buf, &Report{Passed: 62, Warned: 1, Failed: 3, BlockingFailed: 3}) + out := buf.String() + + if strings.Contains(out, "skipped") { + t.Errorf("summary = %q, want no skipped clause when nothing was skipped", out) + } + if strings.Contains(out, "not fully inspected") { + t.Errorf("summary = %q, want no partial-inspection warning on a complete run", out) + } +} diff --git a/internal/doctor/types.go b/internal/doctor/types.go index 17e61f3d66..f2983a045a 100644 --- a/internal/doctor/types.go +++ b/internal/doctor/types.go @@ -106,4 +106,12 @@ type CheckResult struct { // the runner reports StatusError/SeverityAdvisory so the run keeps // going without gating automation on an unfinished check. TimedOut bool + // Skipped is true when this result stands in for a group of checks + // that never ran, rather than for a check that ran and failed. Set by + // SkippedCheck; counted into Report.Skipped so the summary can state + // how much of the factory went uninspected. + // + // Distinct from TimedOut above: a timed-out check started and its + // outcome is unknown, whereas a skipped group never registered at all. + Skipped bool } From 66dffa870e0da98f97c2281b5feeadca65d062ca Mon Sep 17 00:00:00 2001 From: Ben Wisecup Date: Wed, 5 Aug 2026 18:38:39 -0400 Subject: [PATCH 2/2] test(doctor): pin skipped-group output end-to-end, correct Skipped doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review follow-ups on the skipped-check-inventory change. cmd/gc/testdata/doctor.txtar — the unit tests prove the skips are registered, but nothing proved they reach stdout. A later change to runDoctor that filtered or short-circuited output on the config-failure path would keep every unit test green while the operator saw nothing, which is the same silent-invisibility defect this change exists to close. The fixture already built a broken city and ran the real binary, so the three assertions ride along on it. Each assertion was proven non-vacuous by injection, one at a time, against an untouched upstream tree: with only 'pack-doctor-checks' added the fixture goes red naming that string, and the same holds for 'check groups skipped' and 'not fully inspected'. All three pass together on this branch. internal/doctor/doctor.go — the doc comment on Report.Skipped asserted that skipped groups are always counted in Failed and BlockingFailed. That is false once a --fix run repairs the cause mid-run: SkippedCheck downgrades to StatusWarning and tally counts it in Warned, which TestReportCountsSkippedGroupsWhenWarning already asserts. The field is projected to JSON, so a consumer trusting the old wording could gate CI on skipped > 0 implying blocking_failed > 0. The comment now states the blocking case, names the --fix exception, and warns off that inference. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-3 On behalf of: @benw5483 Co-Authored-By: --- cmd/gc/testdata/doctor.txtar | 6 ++++++ internal/doctor/doctor.go | 12 +++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/cmd/gc/testdata/doctor.txtar b/cmd/gc/testdata/doctor.txtar index 572b8b558b..4a0910e0fc 100644 --- a/cmd/gc/testdata/doctor.txtar +++ b/cmd/gc/testdata/doctor.txtar @@ -36,6 +36,12 @@ cd $WORK/broken-city stdout 'city-config' stdout 'parse error' stdout 'failed' +# A failed config load drops every config-dependent check group. The unit +# tests pin that the skips are registered; these lines pin that they reach +# the operator, which is the part that made the original defect invisible. +stdout 'pack-doctor-checks' +stdout 'check groups skipped' +stdout 'not fully inspected' -- empty -- -- broken-city/.gc/.keep -- diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 6733bc3f57..c0648382e0 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -28,9 +28,15 @@ type Report struct { // Fixed is the number of checks remediated by --fix. Fixed int // Skipped is the number of check groups that never ran, as reported by - // SkippedCheck. These are also counted in Failed and BlockingFailed — - // a group gc could not inspect gates the exit code the same way a - // failing check does. + // SkippedCheck. While the cause is unresolved such a group is also + // counted in Failed and BlockingFailed, so it gates the exit code the + // same way a failing check does. + // + // The exception is a group whose cause was repaired earlier in the same + // --fix run: SkippedCheck downgrades that to a warning, so it counts in + // Warned instead and does not gate the exit code. Automation must not + // read Skipped > 0 as implying BlockingFailed > 0. Either way the group + // did not run, so it is always counted here. Skipped int // Results holds the per-check results in the order they ran. Populated // by Run so callers that need structured output (e.g. `gc doctor --json`)