From 7e3537e3b9f37e964808ed67dc01cf8d56771a39 Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 3 Apr 2026 12:36:50 +0200 Subject: [PATCH 1/2] Apply structured output formatter to setup execution --- docs/AGENT_SETUP_IMPROVEMENTS.md | 8 +- internal/setup/executor.go | 88 ++++++++++--- internal/setup/executor_test.go | 213 +++++++++++++++++++++++-------- internal/setup/helpers_test.go | 16 +++ internal/setup/runner_test.go | 4 +- 5 files changed, 257 insertions(+), 72 deletions(-) diff --git a/docs/AGENT_SETUP_IMPROVEMENTS.md b/docs/AGENT_SETUP_IMPROVEMENTS.md index 6f24557..a90d9e2 100644 --- a/docs/AGENT_SETUP_IMPROVEMENTS.md +++ b/docs/AGENT_SETUP_IMPROVEMENTS.md @@ -57,10 +57,10 @@ Replace raw `fmt.Println` calls with a structured, visually clear output format ### 2.2 Apply formatter to setup execution -- [ ] Update `Executor.Execute` in `internal/setup/executor.go` to use the formatter instead of raw `fmt.Printf` -- [ ] Show per-command durations on the same line as the step name -- [ ] On failure, show the failing command's stderr indented beneath the step -- [ ] Show skipped phases (phases that never ran due to early failure) at the bottom +- [x] Update `Executor.Execute` in `internal/setup/executor.go` to use the formatter instead of raw `fmt.Printf` +- [x] Show per-command durations on the same line as the step name +- [x] On failure, show the failing command's stderr indented beneath the step +- [x] Show skipped phases (phases that never ran due to early failure) at the bottom ### 2.3 Apply formatter to agent mode diff --git a/internal/setup/executor.go b/internal/setup/executor.go index aa5d638..8c509d0 100644 --- a/internal/setup/executor.go +++ b/internal/setup/executor.go @@ -3,19 +3,24 @@ package setup import ( "context" "fmt" + "os" "strings" "time" + + "github.com/InitiatDev/initiat-cli/internal/output" ) type Executor struct { secrets map[string]string commandExecutor CommandExecutor + formatter *output.Formatter } func NewExecutor(secrets map[string]string) *Executor { return &Executor{ secrets: secrets, commandExecutor: NewRealCommandExecutor(), + formatter: output.NewFormatter(os.Stdout), } } @@ -23,6 +28,15 @@ func NewExecutorWithCommandExecutor(secrets map[string]string, executor CommandE return &Executor{ secrets: secrets, commandExecutor: executor, + formatter: output.NewFormatter(os.Stdout), + } +} + +func NewExecutorWithFormatter(secrets map[string]string, executor CommandExecutor, formatter *output.Formatter) *Executor { + return &Executor{ + secrets: secrets, + commandExecutor: executor, + formatter: formatter, } } @@ -33,24 +47,44 @@ func (e *Executor) Execute(plan *ExecutionPlan) error { Commands: []CommandExecutionRecord{}, } - fmt.Printf("🚀 Executing setup script: %d phases, %d steps, %d commands\n\n", - len(plan.Summary.Phases), plan.Summary.TotalSteps, plan.Summary.TotalCommands) + f := e.formatter + currentPhase := "" + completedPhases := map[string]bool{} for _, cmd := range plan.Commands { - fmt.Printf("[%s] %s", cmd.Phase, cmd.StepName) - if cmd.Description != "" { - fmt.Printf(": %s", cmd.Description) + if cmd.Phase != currentPhase { + if currentPhase != "" { + f.PhaseEnd() + } + currentPhase = cmd.Phase + f.PhaseStart(currentPhase) } - fmt.Println() record, err := e.executeCommandWithReport(cmd) report.Commands = append(report.Commands, record) + duration := lastAttemptDuration(record) + stepLabel := cmd.StepName + if stepLabel == "" { + stepLabel = cmd.Description + } + if err != nil { + stderr := lastAttemptStderr(record) + f.StepFailure(stepLabel, duration, stderr) + if cmd.ContinueOnError { - fmt.Printf("⚠️ Command failed but continuing: %v\n", err) continue } + + f.PhaseEnd() + completedPhases[currentPhase] = true + + skipped := skippedPhaseNames(plan.Summary.Phases, completedPhases) + if len(skipped) > 0 { + f.PhasesSkipped(skipped) + } + report.FinishedAt = time.Now() return &SetupExecutionError{ Report: report, @@ -58,14 +92,43 @@ func (e *Executor) Execute(plan *ExecutionPlan) error { Err: fmt.Errorf("command failed: %w", err), } } + + f.StepSuccess(stepLabel, duration) + completedPhases[cmd.Phase] = true + } + + if currentPhase != "" { + f.PhaseEnd() } - fmt.Println() - fmt.Println("✅ Setup script completed successfully!") report.FinishedAt = time.Now() return nil } +func lastAttemptDuration(record CommandExecutionRecord) time.Duration { + if len(record.Attempts) == 0 { + return 0 + } + return record.Attempts[len(record.Attempts)-1].Duration +} + +func lastAttemptStderr(record CommandExecutionRecord) string { + if len(record.Attempts) == 0 { + return "" + } + return record.Attempts[len(record.Attempts)-1].Stderr +} + +func skippedPhaseNames(phases []PhaseSummary, completed map[string]bool) []string { + var skipped []string + for _, p := range phases { + if !completed[p.Name] { + skipped = append(skipped, p.Name) + } + } + return skipped +} + func (e *Executor) executeCommandWithReport(cmd ExecutableCommand) (CommandExecutionRecord, error) { record := CommandExecutionRecord{ Phase: cmd.Phase, @@ -89,7 +152,6 @@ func (e *Executor) executeCommandWithReport(cmd ExecutableCommand) (CommandExecu for attempt := 1; attempt <= attempts; attempt++ { if attempt > 1 { - fmt.Printf(" ↻ Retry attempt %d/%d...\n", attempt, attempts) if cmd.Retries != nil && cmd.Retries.Backoff > 0 { time.Sleep(cmd.Retries.Backoff) } @@ -99,17 +161,11 @@ func (e *Executor) executeCommandWithReport(cmd ExecutableCommand) (CommandExecu record.Attempts = append(record.Attempts, e.toAttemptRecord(attempt, res, err)) if err == nil { - if attempt > 1 { - fmt.Printf(" ✅ Command succeeded on retry\n") - } record.Success = true return record, nil } lastErr = err - if attempt < attempts { - fmt.Printf(" ⚠️ Command failed, will retry: %v\n", err) - } } record.Success = false diff --git a/internal/setup/executor_test.go b/internal/setup/executor_test.go index 8551335..95eaab7 100644 --- a/internal/setup/executor_test.go +++ b/internal/setup/executor_test.go @@ -1,13 +1,21 @@ package setup import ( + "bytes" "errors" + "strings" "testing" "time" - "github.com/InitiatDev/initiat-cli/internal/testutil" + "github.com/InitiatDev/initiat-cli/internal/output" ) +func newTestExecutor(secrets map[string]string, mock *mockCommandExecutor) (*Executor, *bytes.Buffer) { + var buf bytes.Buffer + f := output.NewFormatter(&buf, output.WithColor(false), output.WithFancy(false)) + return NewExecutorWithFormatter(secrets, mock, f), &buf +} + func TestExecutor_Execute(t *testing.T) { tests := []struct { name string @@ -41,14 +49,12 @@ func TestExecutor_Execute(t *testing.T) { secrets: nil, wantError: false, contains: []string{ - "Executing setup script", - "setup", - "test", - "completed successfully", + "== setup ==", + "[ok] test", }, }, { - name: "multiple commands", + name: "multiple commands across phases", plan: &ExecutionPlan{ Commands: []ExecutableCommand{ { @@ -78,10 +84,10 @@ func TestExecutor_Execute(t *testing.T) { secrets: nil, wantError: false, contains: []string{ - "bootstrap", - "setup", - "step1", - "step2", + "== bootstrap ==", + "[ok] step1", + "== setup ==", + "[ok] step2", }, }, { @@ -105,18 +111,15 @@ func TestExecutor_Execute(t *testing.T) { secrets: map[string]string{"API_KEY": "secret123"}, wantError: false, contains: []string{ - "completed successfully", + "[ok] test", }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - capture := testutil.CaptureStdout() - defer capture.Restore() - - mockExecutor := newMockCommandExecutor() - executor := NewExecutorWithCommandExecutor(tt.secrets, mockExecutor) + mock := newMockCommandExecutor() + executor, buf := newTestExecutor(tt.secrets, mock) err := executor.Execute(tt.plan) if (err != nil) != tt.wantError { @@ -124,12 +127,17 @@ func TestExecutor_Execute(t *testing.T) { return } + out := buf.String() for _, text := range tt.contains { - capture.AssertContains(t, text) + if !strings.Contains(out, text) { + t.Errorf("Expected output to contain %q, got:\n%s", text, out) + } } for _, text := range tt.notContains { - capture.AssertNotContains(t, text) + if strings.Contains(out, text) { + t.Errorf("Expected output NOT to contain %q, got:\n%s", text, out) + } } }) } @@ -166,22 +174,25 @@ func TestExecutor_Execute_ContinueOnError(t *testing.T) { }, } - capture := testutil.CaptureStdout() - defer capture.Restore() - - mockExecutor := newMockCommandExecutor() - mockExecutor.SetError("false", errors.New("command failed")) - executor := NewExecutorWithCommandExecutor(nil, mockExecutor) + mock := newMockCommandExecutor() + mock.SetError("false", errors.New("command failed")) + executor, buf := newTestExecutor(nil, mock) err := executor.Execute(plan) if err != nil { t.Errorf("Execute() should not fail with continue_on_error, got error: %v", err) } - capture.AssertContains(t, "success") - capture.AssertContains(t, "Command failed but continuing") - capture.AssertContains(t, "final") - capture.AssertContains(t, "completed successfully") + out := buf.String() + if !strings.Contains(out, "[ok] success") { + t.Errorf("Expected success step, got:\n%s", out) + } + if !strings.Contains(out, "[FAIL] fail") { + t.Errorf("Expected failure step, got:\n%s", out) + } + if !strings.Contains(out, "[ok] final") { + t.Errorf("Expected final step, got:\n%s", out) + } } func TestExecutor_Execute_Retry(t *testing.T) { @@ -202,19 +213,91 @@ func TestExecutor_Execute_Retry(t *testing.T) { }, } - capture := testutil.CaptureStdout() - defer capture.Restore() - - mockExecutor := newMockCommandExecutor() - mockExecutor.SetError("false", errors.New("command failed")) - executor := NewExecutorWithCommandExecutor(nil, mockExecutor) + mock := newMockCommandExecutor() + executor, buf := newTestExecutor(nil, mock) err := executor.Execute(plan) if err != nil { t.Errorf("Execute() error = %v", err) } - capture.AssertContains(t, "completed successfully") + out := buf.String() + if !strings.Contains(out, "[ok] test") { + t.Errorf("Expected success output, got:\n%s", out) + } +} + +func TestExecutor_Execute_FailureShowsStderr(t *testing.T) { + plan := &ExecutionPlan{ + Commands: []ExecutableCommand{ + { + Phase: "provision", + StepName: "Run migrations", + Command: "migrate", + Args: []string{"up"}, + }, + }, + Summary: ExecutionSummary{ + TotalSteps: 1, + TotalCommands: 1, + Phases: []PhaseSummary{ + {Name: "provision", StepCount: 1, CommandCount: 1}, + {Name: "setup", StepCount: 1, CommandCount: 1}, + }, + }, + } + + mock := newMockCommandExecutor() + mock.SetError("migrate", errors.New("migration failed")) + mock.SetResult("migrate", &CommandResult{ + ExitCode: 1, + Stderr: "relation \"users\" already exists", + }) + executor, buf := newTestExecutor(nil, mock) + err := executor.Execute(plan) + + if err == nil { + t.Fatal("Expected error, got nil") + } + + out := buf.String() + if !strings.Contains(out, "[FAIL] Run migrations") { + t.Errorf("Expected failure step, got:\n%s", out) + } + if !strings.Contains(out, "relation \"users\" already exists") { + t.Errorf("Expected stderr in output, got:\n%s", out) + } +} + +func TestExecutor_Execute_SkippedPhases(t *testing.T) { + plan := &ExecutionPlan{ + Commands: []ExecutableCommand{ + { + Phase: "bootstrap", + StepName: "install", + Command: "fail-cmd", + }, + }, + Summary: ExecutionSummary{ + TotalSteps: 3, + TotalCommands: 3, + Phases: []PhaseSummary{ + {Name: "bootstrap", StepCount: 1, CommandCount: 1}, + {Name: "provision", StepCount: 1, CommandCount: 1}, + {Name: "setup", StepCount: 1, CommandCount: 1}, + }, + }, + } + + mock := newMockCommandExecutor() + mock.SetError("fail-cmd", errors.New("failed")) + executor, buf := newTestExecutor(nil, mock) + _ = executor.Execute(plan) + + out := buf.String() + if !strings.Contains(out, "provision") || !strings.Contains(out, "setup") || !strings.Contains(out, "skipped") { + t.Errorf("Expected skipped phases in output, got:\n%s", out) + } } func TestExecutor_shouldRedact(t *testing.T) { @@ -267,8 +350,8 @@ func TestExecutor_shouldRedact(t *testing.T) { } func TestExecutor_CommandExecution(t *testing.T) { - mockExecutor := newMockCommandExecutor() - executor := NewExecutorWithCommandExecutor(nil, mockExecutor) + mock := newMockCommandExecutor() + executor, _ := newTestExecutor(nil, mock) plan := &ExecutionPlan{ Commands: []ExecutableCommand{ @@ -293,7 +376,7 @@ func TestExecutor_CommandExecution(t *testing.T) { t.Errorf("Execute() error = %v", err) } - executed := mockExecutor.GetExecuted() + executed := mock.GetExecuted() if len(executed) != 1 { t.Errorf("Expected 1 command execution, got %d", len(executed)) } @@ -313,18 +396,48 @@ func TestExecutor_CommandExecution(t *testing.T) { } } -func containsString(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - (len(s) > 0 && len(substr) > 0 && - (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || - indexOfString(s, substr) >= 0))) -} +func TestExecutor_Execute_PhaseTransitions(t *testing.T) { + plan := &ExecutionPlan{ + Commands: []ExecutableCommand{ + {Phase: "bootstrap", StepName: "install deps", Command: "echo"}, + {Phase: "bootstrap", StepName: "check versions", Command: "echo"}, + {Phase: "setup", StepName: "build", Command: "echo"}, + }, + Summary: ExecutionSummary{ + TotalSteps: 3, + TotalCommands: 3, + Phases: []PhaseSummary{ + {Name: "bootstrap", StepCount: 2, CommandCount: 2}, + {Name: "setup", StepCount: 1, CommandCount: 1}, + }, + }, + } -func indexOfString(s, substr string) int { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return i - } + mock := newMockCommandExecutor() + executor, buf := newTestExecutor(nil, mock) + err := executor.Execute(plan) + if err != nil { + t.Fatalf("Execute() error = %v", err) + } + + out := buf.String() + // Verify phase ordering: bootstrap appears before setup + bootstrapIdx := strings.Index(out, "== bootstrap ==") + setupIdx := strings.Index(out, "== setup ==") + if bootstrapIdx < 0 || setupIdx < 0 { + t.Fatalf("Expected both phase headers, got:\n%s", out) + } + if bootstrapIdx >= setupIdx { + t.Errorf("Expected bootstrap before setup, got:\n%s", out) + } + + // Both steps in bootstrap phase appear between bootstrap and setup headers + installIdx := strings.Index(out, "install deps") + checkIdx := strings.Index(out, "check versions") + if installIdx < bootstrapIdx || installIdx > setupIdx { + t.Errorf("Expected 'install deps' within bootstrap phase, got:\n%s", out) + } + if checkIdx < bootstrapIdx || checkIdx > setupIdx { + t.Errorf("Expected 'check versions' within bootstrap phase, got:\n%s", out) } - return -1 } diff --git a/internal/setup/helpers_test.go b/internal/setup/helpers_test.go index 7fd85ac..5b750ee 100644 --- a/internal/setup/helpers_test.go +++ b/internal/setup/helpers_test.go @@ -93,6 +93,22 @@ func TestCollectSecretNames(t *testing.T) { } } +func containsString(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(substr) == 0 || + (len(s) > 0 && len(substr) > 0 && + (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || + indexOfString(s, substr) >= 0))) +} + +func indexOfString(s, substr string) int { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return i + } + } + return -1 +} + func TestDetectShell(t *testing.T) { originalShell := os.Getenv("SHELL") diff --git a/internal/setup/runner_test.go b/internal/setup/runner_test.go index 9540dc6..60c5b3f 100644 --- a/internal/setup/runner_test.go +++ b/internal/setup/runner_test.go @@ -42,7 +42,7 @@ func TestSetupRunner_Run(t *testing.T) { return } - capture.AssertContains(t, "completed successfully") + capture.AssertContains(t, "[ok] test-step") } func TestSetupRunner_Run_NoCommands(t *testing.T) { @@ -200,6 +200,6 @@ setup: t.Logf("Run error (may be expected): %v", err) } } else { - capture.AssertContains(t, "completed successfully") + capture.AssertContains(t, "[ok] test") } } From f8774c74c297a5122101fbd022b162b06d7e174b Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 3 Apr 2026 12:40:18 +0200 Subject: [PATCH 2/2] Fix line length lint violation in NewExecutorWithFormatter --- internal/setup/executor.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/setup/executor.go b/internal/setup/executor.go index 8c509d0..791f09c 100644 --- a/internal/setup/executor.go +++ b/internal/setup/executor.go @@ -32,7 +32,9 @@ func NewExecutorWithCommandExecutor(secrets map[string]string, executor CommandE } } -func NewExecutorWithFormatter(secrets map[string]string, executor CommandExecutor, formatter *output.Formatter) *Executor { +func NewExecutorWithFormatter( + secrets map[string]string, executor CommandExecutor, formatter *output.Formatter, +) *Executor { return &Executor{ secrets: secrets, commandExecutor: executor,