diff --git a/.superpowers/sdd/2026-08-15-unified-tht-cli-product-step/task-5-report.md b/.superpowers/sdd/2026-08-15-unified-tht-cli-product-step/task-5-report.md index b8ce8c9b..03b160b1 100644 --- a/.superpowers/sdd/2026-08-15-unified-tht-cli-product-step/task-5-report.md +++ b/.superpowers/sdd/2026-08-15-unified-tht-cli-product-step/task-5-report.md @@ -53,3 +53,28 @@ installation, Pi configuration edit, or documentation rewrite was performed. - The existing aggregate `tht doctor` command remains a separate implementation; Task 5 performs its equivalent setup-time prerequisite checks plus `pi.Doctor` without invoking a nested CLI process. + +## Fix round 1 + +The independent review identified three gaps. All were reproduced with RED tests before the +production change: + +- A rendered Compose document containing any one volume was accepted. `requireVolumes` now + requires `settings`, `pi-state`, `workspace-registry`, `workspace-secrets`, `sessions`, + `qdrant-data`, and `embedding-models`; tests reject each individual omission and an + unrelated-only volume set. +- Failures after `compose up` could return without recovery instructions. A single recovery + wrapper now preserves the underlying error while adding the retained-container, `tht logs + `, and `tht status` guidance for failed `up`, health, aggregate doctor, and Pi doctor + phases. Focused tests also prove build failure stops before attempting startup. +- LF inspection previously walked the full checkout. It now inspects only `compose.yaml`, + `deploy/`, and `docker/`; a test proves CRLF content under `node_modules/` is ignored. + +Verification added for this round: + +```bash +go test ./internal/setup -run 'TestRequireVolumes|TestRun(BuildFailure|UpFailure|AggregateDoctorFailure|PiDoctorFailure|IgnoresIrrelevant|TimesOut)' -count=1 +go test ./internal/setup -count=1 +``` + +Both passed before the final full-suite verification. No Docker or live operation was run. diff --git a/tools/tht/internal/setup/run.go b/tools/tht/internal/setup/run.go index f210e1b3..1597bea4 100644 --- a/tools/tht/internal/setup/run.go +++ b/tools/tht/internal/setup/run.go @@ -70,19 +70,19 @@ func Run(ctx context.Context, runner compose.Runner, request Request, input io.R } result.Built = true if err := runCompose(ctx, runner, installation, "up", "--detach", "--remove-orphans"); err != nil { - return Result{}, fmt.Errorf("setup stack start: %w", err) + return Result{}, withStartupRecovery(fmt.Errorf("setup stack start: %w", err), "core") } result.Started = true if err := waitForHealthyServices(ctx, runner, installation); err != nil { - return Result{}, err + return Result{}, withStartupRecovery(err, recoveryService(err)) } result.Healthy = true if err := aggregateDoctor(ctx, runner, installation); err != nil { - return Result{}, err + return Result{}, withStartupRecovery(err, "core") } controlled := compose.InstallationRunner{Installation: installation, Runner: runner} if err := pi.Doctor(ctx, controlled); err != nil { - return Result{}, fmt.Errorf("setup Pi doctor: %w", err) + return Result{}, withStartupRecovery(fmt.Errorf("setup Pi doctor: %w", err), "core") } fmt.Fprintf(output, "ThothII is ready at %s\nInstallation descriptor: %s\nNext: tht status\n", frontendURL(installation), result.DescriptorPath) return result, nil @@ -147,6 +147,21 @@ func aggregateDoctor(ctx context.Context, runner compose.Runner, installation co return nil } +func withStartupRecovery(cause error, service string) error { + if service == "" { + service = "core" + } + return fmt.Errorf("%w; containers were left running for diagnosis: tht logs %s; then run tht status", cause, service) +} + +func recoveryService(cause error) string { + var healthFailure healthFailure + if errors.As(cause, &healthFailure) && healthFailure.service != "" { + return healthFailure.service + } + return "core" +} + type serviceStatus struct { Service string `json:"Service"` State string `json:"State"` @@ -154,6 +169,15 @@ type serviceStatus struct { ExitCode json.RawMessage `json:"ExitCode"` } +type healthFailure struct { + service string + state string +} + +func (e healthFailure) Error() string { + return fmt.Sprintf("setup health timed out waiting for %s (last state: %s)", e.service, e.state) +} + func waitForHealthyServices(ctx context.Context, runner compose.Runner, installation config.Installation) error { healthContext, cancel := context.WithTimeout(ctx, setupHealthTimeout) defer cancel() @@ -178,7 +202,7 @@ func waitForHealthyServices(ctx context.Context, runner compose.Runner, installa } select { case <-healthContext.Done(): - return fmt.Errorf("setup health timed out waiting for %s (last state: %s); containers were left running for diagnosis: tht logs %s; then run tht status", lastService, lastState, lastService) + return healthFailure{service: lastService, state: lastState} case <-time.After(healthPollInterval): } } @@ -264,31 +288,59 @@ func requireVolumes(renderedConfig string) error { if err := json.Unmarshal([]byte(renderedConfig), &document); err != nil { return errors.New("Compose returned invalid rendered configuration") } - if len(document.Volumes) == 0 { - return errors.New("rendered Compose configuration declares no volumes") + for _, volume := range []string{"settings", "pi-state", "workspace-registry", "workspace-secrets", "sessions", "qdrant-data", "embedding-models"} { + if _, exists := document.Volumes[volume]; !exists { + return fmt.Errorf("rendered Compose configuration is missing required volume %s", volume) + } } return nil } func requireLF(root string) error { - return filepath.WalkDir(root, func(path string, entry os.DirEntry, walkErr error) error { + for _, path := range []string{filepath.Join(root, "compose.yaml"), filepath.Join(root, "deploy"), filepath.Join(root, "docker")} { + if err := requireLFPath(path); err != nil { + return err + } + } + return nil +} + +func requireLFPath(path string) error { + info, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil || info.Mode()&os.ModeSymlink != 0 { + return nil + } + if !info.IsDir() { + return requireLFFile(path, info.Mode()) + } + return filepath.WalkDir(path, func(path string, entry os.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } - if entry.IsDir() || entry.Type()&os.ModeSymlink != 0 || !requiresLF(entry.Name()) { + if entry.IsDir() || entry.Type()&os.ModeSymlink != 0 { return nil } - contents, err := os.ReadFile(path) - if err != nil { - return err - } - if strings.Contains(string(contents), "\r\n") { - return fmt.Errorf("CRLF line endings found in %s", filepath.Base(path)) - } - return nil + return requireLFFile(path, entry.Type()) }) } +func requireLFFile(path string, mode os.FileMode) error { + if !mode.IsRegular() || !requiresLF(filepath.Base(path)) { + return nil + } + contents, err := os.ReadFile(path) + if err != nil { + return err + } + if strings.Contains(string(contents), "\r\n") { + return fmt.Errorf("CRLF line endings found in %s", filepath.Base(path)) + } + return nil +} + func requiresLF(name string) bool { if name == "Dockerfile" || strings.HasPrefix(name, "Dockerfile.") || strings.HasSuffix(name, ".Dockerfile") { return true diff --git a/tools/tht/internal/setup/run_test.go b/tools/tht/internal/setup/run_test.go index a0e96223..4de8838d 100644 --- a/tools/tht/internal/setup/run_test.go +++ b/tools/tht/internal/setup/run_test.go @@ -94,6 +94,104 @@ func TestRunTimesOutWithPartialStartupGuidance(t *testing.T) { } } +func TestRunBuildFailureDoesNotAttemptContainerStartup(t *testing.T) { + _, request := setupRunFixture(t, false) + runner := &setupRunner{failureAt: "compose build"} + + _, err := Run(context.Background(), runner, request, strings.NewReader(""), io.Discard) + if err == nil || !strings.Contains(err.Error(), "setup image build") { + t.Fatalf("Run() error = %v, want original build failure", err) + } + if strings.Contains(err.Error(), "tht status") { + t.Fatalf("Run() error = %q, must not offer partial-start recovery before compose up", err) + } + assertSetupStages(t, runner, "docker engine", "docker compose", "architecture", "compose config", "compose build") +} + +func TestRunUpFailurePreservesCauseAndOffersRecovery(t *testing.T) { + _, request := setupRunFixture(t, false) + runner := &setupRunner{failureAt: "compose up"} + + _, err := Run(context.Background(), runner, request, strings.NewReader(""), io.Discard) + assertRecoveryFailure(t, err, "setup stack start", "core") + assertSetupStages(t, runner, "docker engine", "docker compose", "architecture", "compose config", "compose build", "compose up") +} + +func TestRunAggregateDoctorFailurePreservesCauseAndOffersRecovery(t *testing.T) { + _, request := setupRunFixture(t, false) + runner := &setupRunner{failureAt: "doctor"} + + _, err := Run(context.Background(), runner, request, strings.NewReader(""), io.Discard) + assertRecoveryFailure(t, err, "setup doctor", "core") + assertSetupStages(t, runner, "docker engine", "docker compose", "architecture", "compose config", "compose build", "compose up", "health", "doctor") +} + +func TestRunPiDoctorFailurePreservesCauseAndOffersRecovery(t *testing.T) { + _, request := setupRunFixture(t, false) + runner := &setupRunner{failureAt: "pi doctor"} + + _, err := Run(context.Background(), runner, request, strings.NewReader(""), io.Discard) + assertRecoveryFailure(t, err, "setup Pi doctor", "core") + assertSetupStages(t, runner, "docker engine", "docker compose", "architecture", "compose config", "compose build", "compose up", "health", "doctor", "pi doctor") +} + +func TestRequireVolumesRequiresEveryInstallationVolume(t *testing.T) { + all := []string{"settings", "pi-state", "workspace-registry", "workspace-secrets", "sessions", "qdrant-data", "embedding-models"} + for _, missing := range all { + t.Run("missing "+missing, func(t *testing.T) { + volumes := make([]string, 0, len(all)-1) + for _, name := range all { + if name != missing { + volumes = append(volumes, name) + } + } + if err := requireVolumes(renderedConfigForVolumes(volumes...)); err == nil || !strings.Contains(err.Error(), missing) { + t.Fatalf("requireVolumes() error = %v, want missing %q", err, missing) + } + }) + } + if err := requireVolumes(renderedConfigForVolumes("unrelated")); err == nil { + t.Fatal("requireVolumes() error = nil, want required-volume failure for unrelated-only configuration") + } + if err := requireVolumes(renderedConfigForVolumes(all...)); err != nil { + t.Fatalf("requireVolumes() error = %v, want complete installation volume set", err) + } +} + +func TestRunIgnoresIrrelevantCRLFFilesDuringLineEndingCheck(t *testing.T) { + projectRoot, request := setupRunFixture(t, true) + irrelevant := filepath.Join(projectRoot, "node_modules", "unrelated", "generated.yml") + if err := os.MkdirAll(filepath.Dir(irrelevant), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(irrelevant, []byte("generated: true\r\n"), 0o600); err != nil { + t.Fatal(err) + } + + if _, err := Run(context.Background(), &setupRunner{}, request, strings.NewReader(""), io.Discard); err != nil { + t.Fatalf("Run() error = %v, want irrelevant CRLF file ignored", err) + } +} + +func assertRecoveryFailure(t *testing.T, err error, cause, service string) { + t.Helper() + if err == nil { + t.Fatal("Run() error = nil, want failure") + } + for _, text := range []string{cause, "tht logs " + service, "tht status", "left running"} { + if !strings.Contains(err.Error(), text) { + t.Errorf("Run() error = %q, want %q", err, text) + } + } +} + +func assertSetupStages(t *testing.T, runner *setupRunner, want ...string) { + t.Helper() + if got := collapseStages(runner.stages); strings.Join(got, " | ") != strings.Join(want, " | ") { + t.Fatalf("runner stages = %v, want %v", got, want) + } +} + const unhealthyServicesJSON = `[ {"Service":"core","State":"running","Health":"healthy"}, {"Service":"frontend","State":"running","Health":"starting"}, @@ -187,7 +285,15 @@ func setupStage(args []string) (string, compose.Result) { } } -const renderedSetupConfig = `{"volumes":{"settings":{}},"services":{"core":{"image":"thothii-core:local","environment":{"THT_LLM_URL":"https://llm.example.invalid"}}}}` +const renderedSetupConfig = `{"volumes":{"settings":{},"pi-state":{},"workspace-registry":{},"workspace-secrets":{},"sessions":{},"qdrant-data":{},"embedding-models":{}},"services":{"core":{"image":"thothii-core:local","environment":{"THT_LLM_URL":"https://llm.example.invalid"}}}}` + +func renderedConfigForVolumes(volumes ...string) string { + entries := make([]string, 0, len(volumes)) + for _, volume := range volumes { + entries = append(entries, `"`+volume+`":{}`) + } + return `{"volumes":{` + strings.Join(entries, ",") + `}}` +} func setupRunFixture(t *testing.T, configureOnly bool) (string, Request) { t.Helper()