fix(doctor): name the check groups dropped when config load fails

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)

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)
+		}
+	}
+}

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 --

internal/citylayout/runtime.go

@@ -169,17 +169,29 @@ 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))
 	}
 	return env
 }
 
 // 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)
 	}

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")
+}