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/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/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..c0648382e0 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -27,6 +27,17 @@ 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. 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`) // can project every result without re-running checks. @@ -121,6 +132,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 +307,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 +326,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 }