diff --git a/DEADCODE.md b/DEADCODE.md new file mode 100644 index 00000000000..4876555fc5b --- /dev/null +++ b/DEADCODE.md @@ -0,0 +1,227 @@ +# Dead Code Removal Plan + +## ⚠️ Critical Lesson Learned (Session 1 failure) + +Running `deadcode ./cmd/...` only analyses the main binary entry points in `cmd/`. +It is **blind to `internal/tools/` programs**, which are separate binaries with their own `main()` functions, called by the Makefile and CI. + +In Session 1 we deleted `pkg/cli/actions_build_command.go` and `internal/tools/actions-build/` because `deadcode ./cmd/...` reported them as unreachable — but they are actively used by `make actions-build` / `make actions-validate` in CI. + +**Correct command:** +```bash +deadcode ./cmd/... ./internal/tools/... 2>/dev/null +``` + +This covers all entry points. The 381-entry list from Session 1 is **invalid** — regenerate it. + +--- + +## Methodology + +Dead code is identified using: +```bash +deadcode ./cmd/... ./internal/tools/... 2>/dev/null +``` + +The tool reports unreachable functions/methods from ALL entry points (`cmd/` + `internal/tools/`). +It does NOT report unreachable constants, variables, or types — only functions. + +**Important rules:** +- **Always include `./internal/tools/...` in the deadcode command** +- Run `go build ./...` after every batch +- Run `go vet ./...` to catch test compilation errors (cheaper than `go test`) +- Run `go test -tags=integration ./pkg/affected/...` to spot-check +- Always check if a "fully dead" file contains live constants/vars before deleting +- The deadcode list was generated before any deletions; re-run after major batches + +--- + +## ⚠️ Status: Plan needs regeneration + +The phases and batches below were based on the **incorrect** `./cmd/...`-only scan. +Before proceeding, reset to main and regenerate the list with: + +```bash +deadcode ./cmd/... ./internal/tools/... 2>/dev/null | tee /tmp/deadcode-correct.txt | wc -l +``` + +The groups below are a rough guide but individual entries may differ. + +--- + +## Session 2 Analysis (2026-02-28) + +**Command:** `deadcode ./cmd/... ./internal/tools/... 2>/dev/null` +**Total dead entries:** 362 +**Fully dead files:** 25 +**Partially dead files:** 117 + +Confirmed NOT dead (correctly excluded now): `pkg/cli/actions_build_command.go`, `pkg/cli/generate_action_metadata_command.go` + +--- + +## Phase 1: Fully Dead Files + +These files have ALL their functions dead. Each must be checked for: +- [ ] Live constants, variables, or types used elsewhere +- [ ] Test files that reference the deleted functions +- [ ] `internal/tools/` dependencies + +### Group 1A: CLI fully dead files (4 files) +- [ ] `pkg/cli/exec.go` (4/4 dead) +- [ ] `pkg/cli/logs_display.go` (1/1 dead) → surgery on `logs_overview_test.go` +- [ ] `pkg/cli/mcp_inspect_safe_inputs_inspector.go` (1/1 dead) → delete `mcp_inspect_safe_inputs_test.go` +- [ ] `pkg/cli/validation_output.go` (2/2 dead) + +### Group 1B: Console fully dead files (3 files) +- [ ] `pkg/console/form.go` (1/1 dead) → delete `form_test.go` +- [ ] `pkg/console/layout.go` (4/4 dead) → surgery on `golden_test.go` +- [ ] `pkg/console/select.go` (2/2 dead) + +### Group 1C: Misc utility fully dead files (4 files) +- [ ] `pkg/logger/error_formatting.go` (1/1 dead) +- [ ] `pkg/parser/ansi_strip.go` (1/1 dead) → surgery on frontmatter tests +- [ ] `pkg/parser/virtual_fs_test_helpers.go` (1/1 dead, test helper only) +- [ ] `pkg/stringutil/paths.go` (1/1 dead) → delete `paths_test.go` + +### Group 1D: Workflow bundler fully dead files (5 files) +These are the JS bundler subsystem — entirely unused. +- [ ] `pkg/workflow/bundler.go` (6/6 dead) → delete 14+ bundler test files +- [ ] `pkg/workflow/bundler_file_mode.go` (12/12 dead) — **CAUTION: contains live const `SetupActionDestination`** +- [ ] `pkg/workflow/bundler_runtime_validation.go` (3/3 dead) +- [ ] `pkg/workflow/bundler_safety_validation.go` (3/3 dead) +- [ ] `pkg/workflow/bundler_script_validation.go` (2/2 dead) + +### Group 1E: Workflow other fully dead files (9 files) +- [ ] `pkg/workflow/compiler_string_api.go` (2/2 dead) → delete `compiler_string_api_test.go` +- [ ] `pkg/workflow/compiler_test_helpers.go` (3/3 dead) — test helper, check usage +- [ ] `pkg/workflow/copilot_participant_steps.go` (3/3 dead) +- [ ] `pkg/workflow/dependency_tracker.go` (2/2 dead) +- [ ] `pkg/workflow/env_mirror.go` (2/2 dead) +- [ ] `pkg/workflow/markdown_unfencing.go` (1/1 dead) +- [ ] `pkg/workflow/prompt_step.go` (2/2 dead) — **CAUTION: may be referenced by tests** +- [ ] `pkg/workflow/safe_output_builder.go` (10/10 dead) — **CAUTION: contains live type `ListJobBuilderConfig`** +- [ ] `pkg/workflow/sh.go` (5/5 dead) — **CAUTION: contains live constants (prompts dir, file names) and embed directive** + +--- + +## Phase 2: Near-Fully Dead Files (high value, some surgery) + +These files are mostly dead and worth cleaning next: + +- [ ] `pkg/workflow/script_registry.go` (11/13 dead) — keep only `GetActionPath`, `DefaultScriptRegistry` +- [ ] `pkg/workflow/artifact_manager.go` (14/16 dead) — remove 14 functions +- [ ] `pkg/constants/constants.go` (13/27 dead) — remove 13 constants +- [ ] `pkg/workflow/map_helpers.go` (5/7 dead) — remove 5 functions +- [ ] `pkg/workflow/js.go` (17/47 dead) — remove 17 JS bundle functions +- [ ] `pkg/workflow/compiler_types.go` (17/45 dead) — remove 17 types/methods + +--- + +## Phase 3: Partially Dead Files (1–6 dead per file) + +Individual function removals across ~100 files. To be tackled after Phase 1 and 2. + +High-count files to prioritize: +- `pkg/workflow/expression_builder.go` (9/27 dead) +- `pkg/workflow/validation_helpers.go` (6/10 dead) +- `pkg/cli/docker_images.go` (6/11 dead) +- `pkg/workflow/domains.go` (10/27 dead) + +--- + +## Batch Execution Log + +### Session 1 — ABORTED (incorrect deadcode command) + +Used `deadcode ./cmd/...` — missed `internal/tools/` entry points. Deleted: +- `pkg/cli/actions_build_command.go` — **WRONG: used by `make actions-build` via `internal/tools/actions-build/`** +- `pkg/cli/exec.go`, `pkg/cli/generate_action_metadata_command.go`, etc. +- `internal/tools/actions-build/`, `internal/tools/generate-action-metadata/` +- CI job `actions-build` from `.github/workflows/ci.yml` + +PR #18782 failed CI with `make: *** No rule to make target 'actions-build'`. Reset to main. + +### Session 2 — In Progress + +#### Batch 1: Groups 1A (CLI) + 1B (Console) + 1C (Misc) — COMPLETE ✅ + +Deleted 17 files, surgery on 6 test files. `go build ./...` + `go vet ./...` + `make fmt` all clean. + +Deferred `pkg/stringutil/paths.go` to Batch 2 — callers in bundler files still present. + +#### Batch 2: Groups 1D + 1E (Workflow fully dead) — TODO +#### Batch 3: Phase 2 (Near-fully dead, high-value partial files) — TODO +#### Batch 4: Phase 3 (Individual function removals) — TODO + +--- + +## Key Constant/Var Dependencies (must rescue before deleting) + +These live values are defined in files that are otherwise fully dead: + +| Const/Var | Used by live code | Currently in | +|-----------|-------------------|--------------| +| `SetupActionDestination` | `safe_outputs_steps.go` etc. | `bundler_file_mode.go` | +| `cacheMemoryPromptFile` | `cache.go` | `sh.go` | +| `cacheMemoryPromptMultiFile` | `cache.go` | `sh.go` | +| `promptsDir` | `unified_prompt_step.go`, `repo_memory_prompt.go` | `sh.go` | +| `prContextPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `tempFolderPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `playwrightPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `markdownPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `xpiaPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `repoMemoryPromptFile` | `repo_memory_prompt.go` | `sh.go` | +| `repoMemoryPromptMultiFile` | `repo_memory_prompt.go` | `sh.go` | +| `safeOutputsPromptFile` | `unified_prompt_step.go` | `sh.go` | +| `safeOutputsCreatePRFile` | `unified_prompt_step.go` | `sh.go` | +| `safeOutputsPushToBranchFile` | `unified_prompt_step.go` | `sh.go` | +| `safeOutputsAutoCreateIssueFile` | `unified_prompt_step.go` | `sh.go` | +| `githubContextPromptText` (embed) | `unified_prompt_step.go` | `sh.go` | +| `ListJobBuilderConfig` type | `add_labels.go` (dead), `safe_output_builder.go` (dead) | `safe_output_builder.go` | + +**Strategy:** Create `pkg/workflow/workflow_constants.go` to hold rescued constants and embed. +`ListJobBuilderConfig` is only used by dead code, so needs no rescue. + +--- + +## Test Files to Delete (when their entire subject is deleted) + +| Test file | Reason to delete | +|-----------|-----------------| +| `pkg/cli/exec_test.go` | Tests deleted exec functions | +| `pkg/cli/validation_output_test.go` | Tests deleted functions | +| `pkg/cli/mcp_inspect_safe_inputs_test.go` | References `spawnSafeInputsInspector` (deleted) | +| `pkg/console/form_test.go` | Tests deleted `RunForm` | +| `pkg/stringutil/paths_test.go` | Tests deleted `NormalizePath` | +| `pkg/workflow/compiler_string_api_test.go` | Tests deleted `ParseWorkflowString` | +| `pkg/workflow/script_registry_test.go` | Tests dead registry methods | +| All `pkg/workflow/bundler_*_test.go` | Tests deleted bundler | + +## Test Files Needing Surgery + +| Test file | What to remove | +|-----------|---------------| +| `pkg/cli/logs_overview_test.go` | Remove 4 tests using deleted `DisplayLogsOverview` | +| `pkg/console/golden_test.go` | Remove tests using deleted `LayoutTitleBox` | +| `pkg/parser/frontmatter_utils_test.go` | Remove `TestStripANSI`, `BenchmarkStripANSI` | +| `pkg/parser/frontmatter_merge_test.go` | Remove stray comment | +| `pkg/workflow/compiler_custom_actions_test.go` | Remove tests using dead registry methods | +| `pkg/workflow/compiler_action_mode_test.go` | Remove tests using dead registry methods | +| `pkg/workflow/custom_action_copilot_token_test.go` | Remove test using `RegisterWithAction` | + +--- + +## PR Strategy + +**PR 1:** Phase 1 Groups 1A + 1B + 1C (CLI, console, misc utilities — no workflow risk) +- 13 files deleted +- Clean, low-risk, easy to review + +**PR 2:** Phase 1 Groups 1D + 1E (bundler + workflow dead files) +- 14 files deleted +- More complex due to constant rescue and test surgery + +**PR 3:** Phase 2 (near-fully dead) + +**PR 4:** Phase 3 (individual function removals, many files) diff --git a/pkg/cli/copilot_agent_test.go b/pkg/cli/copilot_agent_test.go index 0ed4ebfde5a..2829182d6e8 100644 --- a/pkg/cli/copilot_agent_test.go +++ b/pkg/cli/copilot_agent_test.go @@ -7,8 +7,6 @@ import ( "path/filepath" "strings" "testing" - - "github.com/github/gh-aw/pkg/logger" ) func TestCopilotCodingAgentDetector_IsGitHubCopilotCodingAgent(t *testing.T) { @@ -245,49 +243,6 @@ func TestExtractToolName(t *testing.T) { } } -func TestExtractErrorMessage(t *testing.T) { - tests := []struct { - name string - line string - expected string - }{ - { - name: "removes ISO timestamp", - line: "2024-01-15T10:00:00.123Z ERROR: Connection failed", - expected: "Connection failed", - }, - { - name: "removes bracketed timestamp", - line: "[2024-01-15 10:00:00] ERROR: File not found", - expected: "File not found", - }, - { - name: "removes log level prefix", - line: "ERROR: Invalid input", - expected: "Invalid input", - }, - { - name: "handles warning prefix", - line: "WARNING: Deprecated API", - expected: "Deprecated API", - }, - { - name: "handles plain message", - line: " Simple error message ", - expected: "Simple error message", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := logger.ExtractErrorMessage(tt.line) - if result != tt.expected { - t.Errorf("Expected '%s', got '%s'", tt.expected, result) - } - }) - } -} - func TestIntegration_CopilotCodingAgentWithAudit(t *testing.T) { // Create a temporary directory that simulates a GitHub Copilot coding agent run // NOTE: GitHub Copilot coding agent runs do NOT have aw_info.json (that's for agentic workflows) diff --git a/pkg/cli/exec.go b/pkg/cli/exec.go deleted file mode 100644 index 4d50af16ce2..00000000000 --- a/pkg/cli/exec.go +++ /dev/null @@ -1,139 +0,0 @@ -package cli - -import ( - "bytes" - "os" - "os/exec" - "strings" - - "github.com/cli/go-gh/v2" - "github.com/github/gh-aw/pkg/logger" -) - -var execLog = logger.New("cli:exec") - -// ghExecOrFallback executes a gh CLI command if GH_TOKEN is available, -// otherwise falls back to an alternative command. -// The gh CLI arguments are inferred from the fallback command arguments. -// Returns the stdout, stderr, and error from whichever command was executed. -func ghExecOrFallback(fallbackCmd string, fallbackArgs []string, fallbackEnv []string) (string, string, error) { - ghToken := os.Getenv("GH_TOKEN") - - if ghToken != "" { - // Use gh CLI when GH_TOKEN is available - // Infer gh args from fallback args - ghArgs := inferGhArgs(fallbackCmd, fallbackArgs) - execLog.Printf("Using gh CLI: gh %s", strings.Join(ghArgs, " ")) - stdout, stderr, err := gh.Exec(ghArgs...) - return stdout.String(), stderr.String(), err - } - - // Fall back to alternative command when GH_TOKEN is not available - execLog.Printf("Using fallback command: %s %s", fallbackCmd, strings.Join(fallbackArgs, " ")) - cmd := exec.Command(fallbackCmd, fallbackArgs...) - - // Add custom environment variables if provided - if len(fallbackEnv) > 0 { - cmd.Env = append(os.Environ(), fallbackEnv...) - } - - // Capture stdout and stderr separately like gh.Exec - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - err := cmd.Run() - return stdout.String(), stderr.String(), err -} - -// inferGhArgs infers gh CLI arguments from fallback command arguments -func inferGhArgs(fallbackCmd string, fallbackArgs []string) []string { - if fallbackCmd != "git" || len(fallbackArgs) == 0 { - // For non-git commands, use gh exec - return append([]string{"exec", "--", fallbackCmd}, fallbackArgs...) - } - - // Handle git commands - gitCmd := fallbackArgs[0] - - switch gitCmd { - case "clone": - // git clone [options] - // -> gh repo clone [options] - return buildGhCloneArgs(fallbackArgs[1:]) - default: - // For other git commands, use gh exec - return append([]string{"exec", "--", "git"}, fallbackArgs...) - } -} - -// buildGhCloneArgs builds gh repo clone arguments from git clone arguments -func buildGhCloneArgs(gitArgs []string) []string { - ghArgs := []string{"repo", "clone"} - - var repoURL string - var targetDir string - var otherArgs []string - - // Options that take a value - optsWithValue := map[string]bool{ - "--branch": true, - "--depth": true, - "--origin": true, - "--template": true, - "--config": true, - "--server-option": true, - "--upload-pack": true, - "--reference": true, - "--reference-if-able": true, - "--separate-git-dir": true, - } - - // Parse git clone arguments - for i := 0; i < len(gitArgs); i++ { - arg := gitArgs[i] - if strings.HasPrefix(arg, "https://") || strings.HasPrefix(arg, "git@") { - repoURL = arg - } else if strings.HasPrefix(arg, "-") { - // It's an option - otherArgs = append(otherArgs, arg) - // Check if this option takes a value - if optsWithValue[arg] && i+1 < len(gitArgs) { - i++ // Move to next arg - otherArgs = append(otherArgs, gitArgs[i]) - } - } else if repoURL != "" && targetDir == "" { - // This is the target directory - targetDir = arg - } - } - - // Extract repo slug from URL (remove https://github.com/ or enterprise domain) - repoSlug := extractRepoSlug(repoURL) - - // Build gh args: gh repo clone -- [git options] - ghArgs = append(ghArgs, repoSlug) - if targetDir != "" { - ghArgs = append(ghArgs, targetDir) - } - - if len(otherArgs) > 0 { - ghArgs = append(ghArgs, "--") - ghArgs = append(ghArgs, otherArgs...) - } - - return ghArgs -} - -// extractRepoSlug extracts the owner/repo slug from a GitHub URL -func extractRepoSlug(repoURL string) string { - githubHost := getGitHubHost() - - // Remove the GitHub host from the URL - slug := strings.TrimPrefix(repoURL, githubHost+"/") - - // Remove .git suffix if present - slug = strings.TrimSuffix(slug, ".git") - - return slug -} diff --git a/pkg/cli/exec_test.go b/pkg/cli/exec_test.go deleted file mode 100644 index 8e9137d1468..00000000000 --- a/pkg/cli/exec_test.go +++ /dev/null @@ -1,229 +0,0 @@ -//go:build !integration - -package cli - -import ( - "strings" - "testing" -) - -func TestGhExecOrFallback(t *testing.T) { - tests := []struct { - name string - ghToken string - fallbackCmd string - fallbackArgs []string - fallbackEnv []string - expectError bool - description string - }{ - { - name: "uses git when GH_TOKEN not set", - ghToken: "", - fallbackCmd: "echo", - fallbackArgs: []string{"fallback executed"}, - fallbackEnv: nil, - expectError: false, - description: "should use fallback command when GH_TOKEN is not set", - }, - { - name: "uses fallback with custom env", - ghToken: "", - fallbackCmd: "sh", - fallbackArgs: []string{"-c", "echo $TEST_VAR"}, - fallbackEnv: []string{"TEST_VAR=test_value"}, - expectError: false, - description: "should pass custom environment variables to fallback command", - }, - { - name: "fallback command failure", - ghToken: "", - fallbackCmd: "false", // command that always fails - fallbackArgs: []string{}, - fallbackEnv: nil, - expectError: true, - description: "should return error when fallback command fails", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Set or unset GH_TOKEN based on test case - if tt.ghToken != "" { - t.Setenv("GH_TOKEN", tt.ghToken) - } - - stdout, _, err := ghExecOrFallback(tt.fallbackCmd, tt.fallbackArgs, tt.fallbackEnv) - - if tt.expectError && err == nil { - t.Errorf("Expected error for test '%s', got nil", tt.description) - } else if !tt.expectError && err != nil { - t.Errorf("Unexpected error for test '%s': %v", tt.description, err) - } - - // For successful fallback tests, verify output - if !tt.expectError && tt.fallbackCmd == "echo" { - if !strings.Contains(stdout, "fallback executed") { - t.Errorf("Expected stdout to contain 'fallback executed', got: %s", stdout) - } - } - - // For env test, verify environment variable was passed - if !tt.expectError && tt.fallbackCmd == "sh" && len(tt.fallbackEnv) > 0 { - if !strings.Contains(stdout, "test_value") { - t.Errorf("Expected stdout to contain 'test_value', got: %s", stdout) - } - } - - // With separated stdout/stderr, we don't expect both to be populated - // This is a change from the previous CombinedOutput behavior - }) - } -} - -func TestGhExecOrFallbackWithGHToken(t *testing.T) { - // This test verifies behavior when GH_TOKEN is set - // Note: We can't easily test actual gh.Exec without a real token, - // so we test that the function attempts to use gh CLI - - // Set a placeholder token - t.Setenv("GH_TOKEN", "placeholder_token_for_test") - - // This will likely fail since we don't have a valid token, - // but we're testing that it attempts gh.Exec path - _, _, err := ghExecOrFallback( - "echo", - []string{"fallback"}, - nil, - ) - - // We expect an error because gh.Exec will fail with invalid token/nonexistent repo - // The important part is that it tried the gh.Exec path - if err == nil { - // If it succeeded, it means it used the fallback, which is wrong - t.Error("Expected function to attempt gh.Exec with GH_TOKEN set") - } -} - -func TestGhExecOrFallbackIntegration(t *testing.T) { - // Integration test: verify the function works end-to-end without GH_TOKEN - // (GH_TOKEN is not set by default in this test) - - // Use a simple command that we know will work - stdout, _, err := ghExecOrFallback( - "echo", - []string{"integration test output"}, - nil, - ) - - if err != nil { - t.Errorf("Unexpected error in integration test: %v", err) - } - - if !strings.Contains(stdout, "integration test output") { - t.Errorf("Expected output to contain 'integration test output', got: %s", stdout) - } -} - -func TestExtractRepoSlug(t *testing.T) { - tests := []struct { - name string - repoURL string - githubHost string - expectedSlug string - }{ - { - name: "standard GitHub URL", - repoURL: "https://github.com/owner/repo", - githubHost: "", - expectedSlug: "owner/repo", - }, - { - name: "GitHub URL with .git suffix", - repoURL: "https://github.com/owner/repo.git", - githubHost: "", - expectedSlug: "owner/repo", - }, - { - name: "enterprise GitHub URL", - repoURL: "https://github.enterprise.com/owner/repo", - githubHost: "https://github.enterprise.com", - expectedSlug: "owner/repo", - }, - { - name: "enterprise GitHub URL with .git", - repoURL: "https://github.enterprise.com/owner/repo.git", - githubHost: "https://github.enterprise.com", - expectedSlug: "owner/repo", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Set environment - if tt.githubHost != "" { - t.Setenv("GITHUB_SERVER_URL", tt.githubHost) - } - - slug := extractRepoSlug(tt.repoURL) - if slug != tt.expectedSlug { - t.Errorf("Expected slug '%s', got '%s'", tt.expectedSlug, slug) - } - }) - } -} - -func TestInferGhArgs(t *testing.T) { - tests := []struct { - name string - fallbackCmd string - fallbackArgs []string - expectedGhArgs []string - }{ - { - name: "git clone simple", - fallbackCmd: "git", - fallbackArgs: []string{"clone", "https://github.com/owner/repo", "/tmp/dir"}, - expectedGhArgs: []string{"repo", "clone", "owner/repo", "/tmp/dir"}, - }, - { - name: "git clone with depth", - fallbackCmd: "git", - fallbackArgs: []string{"clone", "--depth", "1", "https://github.com/owner/repo", "/tmp/dir"}, - expectedGhArgs: []string{"repo", "clone", "owner/repo", "/tmp/dir", "--", "--depth", "1"}, - }, - { - name: "git clone with branch", - fallbackCmd: "git", - fallbackArgs: []string{"clone", "--depth", "1", "https://github.com/owner/repo", "/tmp/dir", "--branch", "main"}, - expectedGhArgs: []string{"repo", "clone", "owner/repo", "/tmp/dir", "--", "--depth", "1", "--branch", "main"}, - }, - { - name: "git checkout", - fallbackCmd: "git", - fallbackArgs: []string{"-C", "/tmp/dir", "checkout", "abc123"}, - expectedGhArgs: []string{"exec", "--", "git", "-C", "/tmp/dir", "checkout", "abc123"}, - }, - { - name: "non-git command", - fallbackCmd: "echo", - fallbackArgs: []string{"hello"}, - expectedGhArgs: []string{"exec", "--", "echo", "hello"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ghArgs := inferGhArgs(tt.fallbackCmd, tt.fallbackArgs) - if len(ghArgs) != len(tt.expectedGhArgs) { - t.Errorf("Expected %d args, got %d: %v", len(tt.expectedGhArgs), len(ghArgs), ghArgs) - return - } - for i, arg := range ghArgs { - if arg != tt.expectedGhArgs[i] { - t.Errorf("Arg %d: expected '%s', got '%s'", i, tt.expectedGhArgs[i], arg) - } - } - }) - } -} diff --git a/pkg/cli/logs_display.go b/pkg/cli/logs_display.go deleted file mode 100644 index 06df28487db..00000000000 --- a/pkg/cli/logs_display.go +++ /dev/null @@ -1,220 +0,0 @@ -// This file provides command-line interface functionality for gh-aw. -// This file (logs_display.go) contains functions for displaying workflow logs information -// to the console, including summary tables and metrics. -// -// Key responsibilities: -// - Rendering workflow logs overview tables -// - Formatting metrics for display (duration, tokens, cost) -// - Aggregating totals across multiple runs - -package cli - -import ( - "fmt" - "os" - "path/filepath" - "strconv" - "strings" - "time" - - "github.com/github/gh-aw/pkg/console" - "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/timeutil" -) - -var logsDisplayLog = logger.New("cli:logs_display") - -// displayLogsOverview displays a summary table of workflow runs and metrics -func displayLogsOverview(processedRuns []ProcessedRun, verbose bool) { - if len(processedRuns) == 0 { - logsDisplayLog.Print("No processed runs to display") - return - } - - logsDisplayLog.Printf("Displaying logs overview: runs=%d, verbose=%v", len(processedRuns), verbose) - - // Prepare table data - headers := []string{"Run ID", "Workflow", "Status", "Duration", "Tokens", "Cost ($)", "Turns", "Errors", "Warnings", "Missing Tools", "Missing Data", "Noops", "Safe Items", "Created", "Logs Path"} - var rows [][]string - - var totalTokens int - var totalCost float64 - var totalDuration time.Duration - var totalTurns int - var totalErrors int - var totalWarnings int - var totalMissingTools int - var totalMissingData int - var totalNoops int - var totalSafeItems int - - for _, pr := range processedRuns { - run := pr.Run - // Format duration - durationStr := "" - if run.Duration > 0 { - durationStr = timeutil.FormatDuration(run.Duration) - totalDuration += run.Duration - } - - // Format cost - costStr := "" - if run.EstimatedCost > 0 { - costStr = fmt.Sprintf("%.3f", run.EstimatedCost) - totalCost += run.EstimatedCost - } - - // Format tokens - tokensStr := "" - if run.TokenUsage > 0 { - tokensStr = console.FormatNumber(run.TokenUsage) - totalTokens += run.TokenUsage - } - - // Format turns - turnsStr := "" - if run.Turns > 0 { - turnsStr = strconv.Itoa(run.Turns) - totalTurns += run.Turns - } - - // Format errors - errorsStr := strconv.Itoa(run.ErrorCount) - totalErrors += run.ErrorCount - - // Format warnings - warningsStr := strconv.Itoa(run.WarningCount) - totalWarnings += run.WarningCount - - // Format missing tools - var missingToolsStr string - if verbose && len(pr.MissingTools) > 0 { - // In verbose mode, show actual tool names - toolNames := make([]string, len(pr.MissingTools)) - for i, tool := range pr.MissingTools { - toolNames[i] = tool.Tool - } - missingToolsStr = strings.Join(toolNames, ", ") - // Truncate if too long - if len(missingToolsStr) > 30 { - missingToolsStr = missingToolsStr[:27] + "..." - } - } else { - // In normal mode, just show the count - missingToolsStr = strconv.Itoa(run.MissingToolCount) - } - totalMissingTools += run.MissingToolCount - - // Format missing data - var missingDataStr string - if verbose && len(pr.MissingData) > 0 { - // In verbose mode, show actual data types - dataTypes := make([]string, len(pr.MissingData)) - for i, data := range pr.MissingData { - dataTypes[i] = data.DataType - } - missingDataStr = strings.Join(dataTypes, ", ") - // Truncate if too long - if len(missingDataStr) > 30 { - missingDataStr = missingDataStr[:27] + "..." - } - } else { - // In normal mode, just show the count - missingDataStr = strconv.Itoa(run.MissingDataCount) - } - totalMissingData += run.MissingDataCount - - // Format noops - var noopsStr string - if verbose && len(pr.Noops) > 0 { - // In verbose mode, show truncated message preview - messages := make([]string, len(pr.Noops)) - for i, noop := range pr.Noops { - msg := noop.Message - if len(msg) > 30 { - msg = msg[:27] + "..." - } - messages[i] = msg - } - noopsStr = strings.Join(messages, ", ") - // Truncate if too long - if len(noopsStr) > 30 { - noopsStr = noopsStr[:27] + "..." - } - } else { - // In normal mode, just show the count - noopsStr = strconv.Itoa(run.NoopCount) - } - totalNoops += run.NoopCount - - // Format safe items count - safeItemsStr := strconv.Itoa(run.SafeItemsCount) - totalSafeItems += run.SafeItemsCount - - // Truncate workflow name if too long - workflowName := run.WorkflowName - if len(workflowName) > 20 { - workflowName = workflowName[:17] + "..." - } - - // Format relative path - relPath, _ := filepath.Rel(".", run.LogsPath) - - // Format status - show conclusion directly for completed runs - statusStr := run.Status - if run.Status == "completed" && run.Conclusion != "" { - statusStr = run.Conclusion - } - - row := []string{ - strconv.FormatInt(run.DatabaseID, 10), - workflowName, - statusStr, - durationStr, - tokensStr, - costStr, - turnsStr, - errorsStr, - warningsStr, - missingToolsStr, - missingDataStr, - noopsStr, - safeItemsStr, - run.CreatedAt.Format("2006-01-02"), - relPath, - } - rows = append(rows, row) - } - - // Prepare total row - totalRow := []string{ - fmt.Sprintf("TOTAL (%d runs)", len(processedRuns)), - "", - "", - timeutil.FormatDuration(totalDuration), - console.FormatNumber(totalTokens), - fmt.Sprintf("%.3f", totalCost), - strconv.Itoa(totalTurns), - strconv.Itoa(totalErrors), - strconv.Itoa(totalWarnings), - strconv.Itoa(totalMissingTools), - strconv.Itoa(totalMissingData), - strconv.Itoa(totalNoops), - strconv.Itoa(totalSafeItems), - "", - "", - } - - // Render table using console helper - tableConfig := console.TableConfig{ - Title: "Workflow Logs Overview", - Headers: headers, - Rows: rows, - ShowTotal: true, - TotalRow: totalRow, - } - - logsDisplayLog.Printf("Rendering table: total_tokens=%d, total_cost=%.3f, total_duration=%s", totalTokens, totalCost, totalDuration) - - fmt.Fprint(os.Stderr, console.RenderTable(tableConfig)) -} diff --git a/pkg/cli/logs_overview_test.go b/pkg/cli/logs_overview_test.go index 7324be2a4e2..e78c4f19f7b 100644 --- a/pkg/cli/logs_overview_test.go +++ b/pkg/cli/logs_overview_test.go @@ -4,61 +4,9 @@ package cli import ( "testing" - "time" ) // TestLogsOverviewIncludesMissingTools verifies that the overview table includes missing tools count -func TestLogsOverviewIncludesMissingTools(t *testing.T) { - processedRuns := []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 12345, - WorkflowName: "Test Workflow A", - Status: "completed", - Conclusion: "success", - CreatedAt: time.Now(), - Duration: 5 * time.Minute, - TokenUsage: 1000, - EstimatedCost: 0.01, - Turns: 3, - ErrorCount: 0, - WarningCount: 2, - MissingToolCount: 1, - LogsPath: "/tmp/gh-aw/run-12345", - }, - MissingTools: []MissingToolReport{ - {Tool: "terraform", Reason: "Infrastructure automation needed"}, - }, - }, - { - Run: WorkflowRun{ - DatabaseID: 67890, - WorkflowName: "Test Workflow B", - Status: "completed", - Conclusion: "failure", - CreatedAt: time.Now(), - Duration: 3 * time.Minute, - TokenUsage: 500, - EstimatedCost: 0.005, - Turns: 2, - ErrorCount: 1, - WarningCount: 0, - MissingToolCount: 3, - LogsPath: "/tmp/gh-aw/run-67890", - }, - MissingTools: []MissingToolReport{ - {Tool: "kubectl", Reason: "K8s management"}, - {Tool: "docker", Reason: "Container runtime"}, - {Tool: "helm", Reason: "K8s package manager"}, - }, - }, - } - - // Capture output by redirecting - this is a smoke test to ensure displayLogsOverview doesn't panic - // and that it processes the MissingToolCount field - displayLogsOverview(processedRuns, false) - displayLogsOverview(processedRuns, true) -} // TestWorkflowRunStructHasMissingToolCount verifies that WorkflowRun has the MissingToolCount field func TestWorkflowRunStructHasMissingToolCount(t *testing.T) { @@ -116,118 +64,6 @@ func TestLogsOverviewHeaderIncludesMissing(t *testing.T) { } // TestDisplayLogsOverviewWithVariousMissingToolCounts tests different scenarios -func TestDisplayLogsOverviewWithVariousMissingToolCounts(t *testing.T) { - testCases := []struct { - name string - processedRuns []ProcessedRun - expectedNonPanic bool - }{ - { - name: "no missing tools", - processedRuns: []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 1, - WorkflowName: "Clean Workflow", - MissingToolCount: 0, - LogsPath: "/tmp/gh-aw/run-1", - }, - MissingTools: []MissingToolReport{}, - }, - }, - expectedNonPanic: true, - }, - { - name: "single missing tool", - processedRuns: []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 2, - WorkflowName: "Workflow with One Missing", - MissingToolCount: 1, - LogsPath: "/tmp/gh-aw/run-2", - }, - MissingTools: []MissingToolReport{ - {Tool: "terraform", Reason: "Need IaC"}, - }, - }, - }, - expectedNonPanic: true, - }, - { - name: "multiple missing tools", - processedRuns: []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 3, - WorkflowName: "Workflow with Multiple Missing", - MissingToolCount: 5, - LogsPath: "/tmp/gh-aw/run-3", - }, - MissingTools: []MissingToolReport{ - {Tool: "terraform", Reason: "IaC"}, - {Tool: "kubectl", Reason: "K8s"}, - {Tool: "docker", Reason: "Containers"}, - {Tool: "helm", Reason: "Packages"}, - {Tool: "argocd", Reason: "GitOps"}, - }, - }, - }, - expectedNonPanic: true, - }, - { - name: "mixed missing tool counts", - processedRuns: []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 4, - WorkflowName: "Workflow A", - MissingToolCount: 0, - LogsPath: "/tmp/gh-aw/run-4", - }, - MissingTools: []MissingToolReport{}, - }, - { - Run: WorkflowRun{ - DatabaseID: 5, - WorkflowName: "Workflow B", - MissingToolCount: 2, - LogsPath: "/tmp/gh-aw/run-5", - }, - MissingTools: []MissingToolReport{ - {Tool: "kubectl", Reason: "K8s"}, - {Tool: "docker", Reason: "Containers"}, - }, - }, - { - Run: WorkflowRun{ - DatabaseID: 6, - WorkflowName: "Workflow C", - MissingToolCount: 1, - LogsPath: "/tmp/gh-aw/run-6", - }, - MissingTools: []MissingToolReport{ - {Tool: "helm", Reason: "Packages"}, - }, - }, - }, - expectedNonPanic: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - // This test ensures displayLogsOverview doesn't panic with various missing tool counts - defer func() { - if r := recover(); r != nil && tc.expectedNonPanic { - t.Errorf("displayLogsOverview panicked with: %v", r) - } - }() - displayLogsOverview(tc.processedRuns, false) - displayLogsOverview(tc.processedRuns, true) - }) - } -} // TestTotalMissingToolsCalculation verifies totals are calculated correctly func TestTotalMissingToolsCalculation(t *testing.T) { @@ -252,83 +88,8 @@ func TestTotalMissingToolsCalculation(t *testing.T) { } // TestOverviewDisplayConsistency verifies that the overview function is consistent -func TestOverviewDisplayConsistency(t *testing.T) { - // Create a run with known values - processedRuns := []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 99999, - WorkflowName: "Consistency Test", - Status: "completed", - Conclusion: "success", - Duration: 10 * time.Minute, - TokenUsage: 2000, - EstimatedCost: 0.02, - Turns: 5, - ErrorCount: 1, - WarningCount: 3, - MissingToolCount: 2, - CreatedAt: time.Date(2024, 1, 15, 10, 30, 0, 0, time.UTC), - LogsPath: "/tmp/gh-aw/run-99999", - }, - MissingTools: []MissingToolReport{ - {Tool: "terraform", Reason: "IaC"}, - {Tool: "kubectl", Reason: "K8s"}, - }, - }, - } - - // Call displayLogsOverview - it should not panic and should handle all fields - defer func() { - if r := recover(); r != nil { - t.Errorf("displayLogsOverview panicked: %v", r) - } - }() - - displayLogsOverview(processedRuns, false) - displayLogsOverview(processedRuns, true) -} // TestMissingToolsIntegration tests the full flow from ProcessedRun to display -func TestMissingToolsIntegration(t *testing.T) { - // Create a ProcessedRun with missing tools - processedRuns := []ProcessedRun{ - { - Run: WorkflowRun{ - DatabaseID: 11111, - WorkflowName: "Integration Test Workflow", - Status: "completed", - Conclusion: "success", - MissingToolCount: 2, - }, - MissingTools: []MissingToolReport{ - { - Tool: "terraform", - Reason: "Infrastructure automation needed", - Alternatives: "Manual AWS console", - Timestamp: "2024-01-15T10:30:00Z", - WorkflowName: "Integration Test Workflow", - RunID: 11111, - }, - { - Tool: "kubectl", - Reason: "Kubernetes cluster management", - WorkflowName: "Integration Test Workflow", - RunID: 11111, - }, - }, - }, - } - - // Verify count is correct - if processedRuns[0].Run.MissingToolCount != 2 { - t.Errorf("Expected MissingToolCount to be 2, got %d", processedRuns[0].Run.MissingToolCount) - } - - // Display should work without panicking - displayLogsOverview(processedRuns, false) - displayLogsOverview(processedRuns, true) -} // TestMissingToolCountFieldAccessibility verifies field is accessible func TestMissingToolCountFieldAccessibility(t *testing.T) { diff --git a/pkg/cli/mcp_inspect_safe_inputs_inspector.go b/pkg/cli/mcp_inspect_safe_inputs_inspector.go deleted file mode 100644 index 386c513579e..00000000000 --- a/pkg/cli/mcp_inspect_safe_inputs_inspector.go +++ /dev/null @@ -1,134 +0,0 @@ -package cli - -import ( - "errors" - "fmt" - "os" - "os/exec" - "path/filepath" - "time" - - "github.com/github/gh-aw/pkg/console" - "github.com/github/gh-aw/pkg/parser" - "github.com/github/gh-aw/pkg/types" - "github.com/github/gh-aw/pkg/workflow" -) - -// spawnSafeInputsInspector generates safe-inputs MCP server files, starts the HTTP server, -// and launches the inspector to inspect it -func spawnSafeInputsInspector(workflowFile string, verbose bool) error { - mcpInspectLog.Printf("Spawning safe-inputs inspector for workflow: %s", workflowFile) - - // Check if node is available - if _, err := exec.LookPath("node"); err != nil { - return fmt.Errorf("node not found. Please install Node.js to run the safe-inputs MCP server: %w", err) - } - - // Resolve the workflow file path - workflowPath, err := ResolveWorkflowPath(workflowFile) - if err != nil { - return err - } - - // Convert to absolute path if needed - if !filepath.IsAbs(workflowPath) { - cwd, err := os.Getwd() - if err != nil { - return fmt.Errorf("failed to get current directory: %w", err) - } - workflowPath = filepath.Join(cwd, workflowPath) - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Inspecting safe-inputs from: "+workflowPath)) - } - - // Use the workflow compiler to parse the file and resolve imports - // This ensures that imported safe-inputs are properly merged - compiler := workflow.NewCompiler( - workflow.WithVerbose(verbose), - ) - workflowData, err := compiler.ParseWorkflowFile(workflowPath) - if err != nil { - return fmt.Errorf("failed to parse workflow file: %w", err) - } - - // Get safe-inputs configuration from the parsed WorkflowData - // This includes both direct and imported safe-inputs configurations - safeInputsConfig := workflowData.SafeInputs - if safeInputsConfig == nil || len(safeInputsConfig.Tools) == 0 { - return errors.New("no safe-inputs configuration found in workflow") - } - - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Found %d safe-input tool(s) to configure", len(safeInputsConfig.Tools)))) - - // Create temporary directory for safe-inputs files - tmpDir, err := os.MkdirTemp("", "gh-aw-safe-inputs-*") - if err != nil { - return fmt.Errorf("failed to create temporary directory: %w", err) - } - defer func() { - if err := os.RemoveAll(tmpDir); err != nil && verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to cleanup temporary directory: %v", err))) - } - }() - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Created temporary directory: "+tmpDir)) - } - - // Write safe-inputs files to temporary directory - if err := writeSafeInputsFiles(tmpDir, safeInputsConfig, verbose); err != nil { - return fmt.Errorf("failed to write safe-inputs files: %w", err) - } - - // Find an available port for the HTTP server - port := findAvailablePort(safeInputsStartPort, verbose) - if port == 0 { - return errors.New("failed to find an available port for the HTTP server") - } - - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Using port %d for safe-inputs HTTP server", port))) - } - - // Start the HTTP server - serverCmd, err := startSafeInputsHTTPServer(tmpDir, port, verbose) - if err != nil { - return fmt.Errorf("failed to start safe-inputs HTTP server: %w", err) - } - defer func() { - if serverCmd.Process != nil { - // Try graceful shutdown first - if err := serverCmd.Process.Signal(os.Interrupt); err != nil && verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to send interrupt signal: %v", err))) - } - // Wait a moment for graceful shutdown - time.Sleep(500 * time.Millisecond) - // Attempt force kill (may fail if process already exited gracefully, which is fine) - _ = serverCmd.Process.Kill() - } - }() - - // Wait for the server to start up - if !waitForServerReady(port, 5*time.Second, verbose) { - return errors.New("safe-inputs HTTP server failed to start within timeout") - } - - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Safe-inputs HTTP server started successfully")) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Server running on: http://localhost:%d", port))) - fmt.Fprintln(os.Stderr) - - // Create MCP server config for the safe-inputs server - safeInputsMCPConfig := parser.MCPServerConfig{ - BaseMCPServerConfig: types.BaseMCPServerConfig{ - Type: "http", - URL: fmt.Sprintf("http://localhost:%d", port), - Env: make(map[string]string), - }, - Name: "safeinputs", - } - - // Inspect the safe-inputs MCP server using the Go SDK (like other MCP servers) - return inspectMCPServer(safeInputsMCPConfig, "", verbose, false) -} diff --git a/pkg/cli/mcp_inspect_safe_inputs_test.go b/pkg/cli/mcp_inspect_safe_inputs_test.go deleted file mode 100644 index 60d89e2e8f2..00000000000 --- a/pkg/cli/mcp_inspect_safe_inputs_test.go +++ /dev/null @@ -1,264 +0,0 @@ -//go:build !integration - -package cli - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/workflow" -) - -// TestSpawnSafeInputsInspector_NoSafeInputs tests the error case when workflow has no safe-inputs -func TestSpawnSafeInputsInspector_NoSafeInputs(t *testing.T) { - // Create temporary directory with a workflow file - tmpDir := t.TempDir() - workflowsDir := filepath.Join(tmpDir, ".github", "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create workflows directory: %v", err) - } - - // Create a test workflow file WITHOUT safe-inputs - workflowContent := `--- -on: push -engine: copilot ---- -# Test Workflow - -This workflow has no safe-inputs configuration. -` - workflowPath := filepath.Join(workflowsDir, "test.md") - if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { - t.Fatalf("Failed to write workflow file: %v", err) - } - - // Change to the temporary directory - originalDir, _ := os.Getwd() - defer os.Chdir(originalDir) - os.Chdir(tmpDir) - - // Try to spawn safe-inputs inspector - should fail - err := spawnSafeInputsInspector("test", false) - if err == nil { - t.Error("Expected error when workflow has no safe-inputs, got nil") - } - - // Verify error message mentions "no safe-inputs" - if err != nil && err.Error() != "no safe-inputs configuration found in workflow" { - t.Errorf("Expected specific error message, got: %v", err) - } -} - -// TestSpawnSafeInputsInspector_WithSafeInputs tests file generation with a real workflow -func TestSpawnSafeInputsInspector_WithSafeInputs(t *testing.T) { - // This test verifies that the function correctly parses a workflow and generates files - // We can't actually start the server or inspector in a test, but we can verify file generation - - // Create temporary directory with a workflow file - tmpDir := t.TempDir() - workflowsDir := filepath.Join(tmpDir, ".github", "workflows") - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create workflows directory: %v", err) - } - - // Create a test workflow file with safe-inputs - workflowContent := `--- -on: push -engine: copilot -safe-inputs: - echo-tool: - description: "Echo a message" - inputs: - message: - type: string - description: "Message to echo" - required: true - run: | - echo "$message" ---- -# Test Workflow - -This workflow has safe-inputs configuration. -` - workflowPath := filepath.Join(workflowsDir, "test.md") - if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { - t.Fatalf("Failed to write workflow file: %v", err) - } - - // Change to the temporary directory - originalDir, _ := os.Getwd() - defer os.Chdir(originalDir) - os.Chdir(tmpDir) - - // We can't fully test spawnSafeInputsInspector because it tries to start a server - // and launch the inspector, but we can test the file generation part separately - // by calling writeSafeInputsFiles directly - - // Parse the workflow using the compiler to get safe-inputs config - // (including any imported safe-inputs) - compiler := workflow.NewCompiler() - workflowData, err := compiler.ParseWorkflowFile(workflowPath) - if err != nil { - t.Fatalf("Failed to parse workflow: %v", err) - } - - safeInputsConfig := workflowData.SafeInputs - if safeInputsConfig == nil { - t.Fatal("Expected safe-inputs config to be parsed") - } - - // Create a temp directory for files - filesDir := t.TempDir() - - // Write files - err = writeSafeInputsFiles(filesDir, safeInputsConfig, false) - if err != nil { - t.Fatalf("writeSafeInputsFiles failed: %v", err) - } - - // Verify the echo-tool.sh file was created - toolPath := filepath.Join(filesDir, "echo-tool.sh") - if _, err := os.Stat(toolPath); os.IsNotExist(err) { - t.Error("echo-tool.sh not found") - } - - // Verify tools.json contains the echo-tool - toolsPath := filepath.Join(filesDir, "tools.json") - toolsContent, err := os.ReadFile(toolsPath) - if err != nil { - t.Fatalf("Failed to read tools.json: %v", err) - } - - // Simple check that the tool name is in the JSON - if len(toolsContent) < 50 { - t.Error("tools.json seems too short") - } -} - -// TestSpawnSafeInputsInspector_WithImportedSafeInputs tests that imported safe-inputs are resolved -func TestSpawnSafeInputsInspector_WithImportedSafeInputs(t *testing.T) { - // Create temporary directory with workflow and shared files - tmpDir := t.TempDir() - workflowsDir := filepath.Join(tmpDir, ".github", "workflows") - sharedDir := filepath.Join(workflowsDir, "shared") - if err := os.MkdirAll(sharedDir, 0755); err != nil { - t.Fatalf("Failed to create workflows directory: %v", err) - } - - // Create a shared workflow file with safe-inputs - sharedContent := `--- -safe-inputs: - shared-tool: - description: "Shared tool from import" - inputs: - param: - type: string - description: "A parameter" - required: true - run: | - echo "Shared: $param" ---- -# Shared Workflow -` - sharedPath := filepath.Join(sharedDir, "shared.md") - if err := os.WriteFile(sharedPath, []byte(sharedContent), 0644); err != nil { - t.Fatalf("Failed to write shared workflow file: %v", err) - } - - // Create a test workflow file that imports the shared workflow - workflowContent := `--- -on: push -engine: copilot -imports: - - shared/shared.md -safe-inputs: - local-tool: - description: "Local tool" - inputs: - message: - type: string - description: "Message to echo" - required: true - run: | - echo "$message" ---- -# Test Workflow - -This workflow imports safe-inputs from shared/shared.md. -` - workflowPath := filepath.Join(workflowsDir, "test.md") - if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { - t.Fatalf("Failed to write workflow file: %v", err) - } - - // Change to the temporary directory - originalDir, _ := os.Getwd() - defer os.Chdir(originalDir) - os.Chdir(tmpDir) - - // Parse the workflow using the compiler to get safe-inputs config - // This should include both local and imported safe-inputs - compiler := workflow.NewCompiler() - workflowData, err := compiler.ParseWorkflowFile(workflowPath) - if err != nil { - t.Fatalf("Failed to parse workflow: %v", err) - } - - safeInputsConfig := workflowData.SafeInputs - if safeInputsConfig == nil { - t.Fatal("Expected safe-inputs config to be parsed") - } - - // Verify both local and imported tools are present - if len(safeInputsConfig.Tools) != 2 { - t.Errorf("Expected 2 tools (local + imported), got %d", len(safeInputsConfig.Tools)) - } - - // Verify local tool exists - if _, exists := safeInputsConfig.Tools["local-tool"]; !exists { - t.Error("Expected local-tool to be present") - } - - // Verify imported tool exists - if _, exists := safeInputsConfig.Tools["shared-tool"]; !exists { - t.Error("Expected shared-tool (from import) to be present") - } - - // Create a temp directory for files - filesDir := t.TempDir() - - // Write files - err = writeSafeInputsFiles(filesDir, safeInputsConfig, false) - if err != nil { - t.Fatalf("writeSafeInputsFiles failed: %v", err) - } - - // Verify both tool handler files were created - localToolPath := filepath.Join(filesDir, "local-tool.sh") - if _, err := os.Stat(localToolPath); os.IsNotExist(err) { - t.Error("local-tool.sh not found") - } - - sharedToolPath := filepath.Join(filesDir, "shared-tool.sh") - if _, err := os.Stat(sharedToolPath); os.IsNotExist(err) { - t.Error("shared-tool.sh not found") - } - - // Verify tools.json contains both tools - toolsPath := filepath.Join(filesDir, "tools.json") - toolsContent, err := os.ReadFile(toolsPath) - if err != nil { - t.Fatalf("Failed to read tools.json: %v", err) - } - - // Check that both tool names are in the JSON - toolsJSON := string(toolsContent) - if !strings.Contains(toolsJSON, "local-tool") { - t.Error("tools.json should contain 'local-tool'") - } - if !strings.Contains(toolsJSON, "shared-tool") { - t.Error("tools.json should contain 'shared-tool'") - } -} diff --git a/pkg/cli/validation_output.go b/pkg/cli/validation_output.go deleted file mode 100644 index f27ad3663be..00000000000 --- a/pkg/cli/validation_output.go +++ /dev/null @@ -1,54 +0,0 @@ -package cli - -import ( - "fmt" - "os" - - "github.com/github/gh-aw/pkg/console" - "github.com/github/gh-aw/pkg/logger" -) - -var validationOutputLog = logger.New("cli:validation_output") - -// FormatValidationError formats validation errors for console output -// Preserves structured error content while applying console styling -// -// This function bridges the gap between pure validation logic (plain text errors) -// and CLI presentation layer (styled console output). By keeping validation errors -// as plain text at the validation layer, we maintain testability and reusability -// while providing consistent styled output in CLI contexts. -// -// The function handles both simple single-line errors and complex multi-line -// structured errors (like GitHubToolsetValidationError) by applying console -// formatting to preserve the error structure and readability. -func FormatValidationError(err error) string { - if err == nil { - return "" - } - - errMsg := err.Error() - validationOutputLog.Printf("Formatting validation error: %s", errMsg) - - // Apply console formatting to the entire error message - // This preserves structured multi-line errors while adding visual styling - return console.FormatErrorMessage(errMsg) -} - -// PrintValidationError prints a validation error to stderr with console formatting -// -// This is a convenience helper that combines formatting and printing in one call. -// All validation errors should be printed using this function to ensure consistent -// styling across the CLI. -// -// Example usage: -// -// if err := ValidateWorkflow(config); err != nil { -// PrintValidationError(err) -// return err -// } -func PrintValidationError(err error) { - if err == nil { - return - } - fmt.Fprintln(os.Stderr, FormatValidationError(err)) -} diff --git a/pkg/cli/validation_output_test.go b/pkg/cli/validation_output_test.go deleted file mode 100644 index 167e36bf0e4..00000000000 --- a/pkg/cli/validation_output_test.go +++ /dev/null @@ -1,234 +0,0 @@ -//go:build !integration - -package cli - -import ( - "errors" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/workflow" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestFormatValidationError verifies that validation errors are formatted with console styling -func TestFormatValidationError(t *testing.T) { - tests := []struct { - name string - err error - expectEmpty bool - mustContain []string - mustNotChange string // Content that must be preserved - }{ - { - name: "nil error returns empty string", - err: nil, - expectEmpty: true, - }, - { - name: "simple single-line error", - err: errors.New("missing required field 'engine'"), - expectEmpty: false, - mustContain: []string{ - "missing required field 'engine'", - }, - mustNotChange: "missing required field 'engine'", - }, - { - name: "error with example", - err: errors.New("invalid engine: unknown. Valid engines are: copilot, claude, codex, custom. Example: engine: copilot"), - expectEmpty: false, - mustContain: []string{ - "invalid engine", - "Valid engines are", - "Example:", - }, - mustNotChange: "invalid engine: unknown. Valid engines are: copilot, claude, codex, custom. Example: engine: copilot", - }, - { - name: "multi-line error", - err: errors.New(`invalid configuration: - field 'engine' is required - field 'on' is missing`), - expectEmpty: false, - mustContain: []string{ - "invalid configuration", - "field 'engine' is required", - "field 'on' is missing", - }, - }, - { - name: "structured validation error (GitHubToolsetValidationError)", - err: workflow.NewGitHubToolsetValidationError(map[string][]string{ - "issues": {"list_issues", "create_issue"}, - }), - expectEmpty: false, - mustContain: []string{ - "ERROR", - "issues", - "list_issues", - "create_issue", - "Suggested fix", - }, - }, - { - name: "error with formatting characters", - err: errors.New("path must be relative, got: /absolute/path"), - mustContain: []string{ - "path must be relative", - "/absolute/path", - }, - mustNotChange: "path must be relative, got: /absolute/path", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := FormatValidationError(tt.err) - - if tt.expectEmpty { - assert.Empty(t, result, "Expected empty string for nil error") - return - } - - // Verify content is preserved - if tt.mustNotChange != "" { - assert.Contains(t, result, tt.mustNotChange, - "Formatted error must contain original error message") - } - - // Verify all required content is present - for _, expected := range tt.mustContain { - assert.Contains(t, result, expected, - "Formatted error must contain: %s", expected) - } - - // Verify formatting is applied (should not be identical to plain error) - if tt.err != nil && !tt.expectEmpty { - plainMsg := tt.err.Error() - // The formatted message should be longer (due to ANSI codes or prefix) - // or at minimum have the error symbol prefix - if result == plainMsg { - t.Errorf("Expected formatting to be applied, but result matches plain error.\nPlain: %s\nFormatted: %s", - plainMsg, result) - } - } - }) - } -} - -// TestPrintValidationError verifies that PrintValidationError outputs to stderr -// Note: This is a smoke test to ensure the function doesn't panic -func TestPrintValidationError(t *testing.T) { - tests := []struct { - name string - err error - }{ - { - name: "nil error does not panic", - err: nil, - }, - { - name: "simple error does not panic", - err: errors.New("test error"), - }, - { - name: "complex structured error does not panic", - err: workflow.NewGitHubToolsetValidationError(map[string][]string{ - "repos": {"get_repository"}, - }), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // This test ensures PrintValidationError doesn't panic - // Actual output testing would require capturing stderr - require.NotPanics(t, func() { - PrintValidationError(tt.err) - }, "PrintValidationError should not panic") - }) - } -} - -// TestFormatValidationErrorPreservesStructure verifies that multi-line errors maintain their structure -func TestFormatValidationErrorPreservesStructure(t *testing.T) { - // Create a structured error with multiple lines and sections - structuredErr := workflow.NewGitHubToolsetValidationError(map[string][]string{ - "issues": {"list_issues", "create_issue"}, - "actions": {"list_workflows"}, - }) - - result := FormatValidationError(structuredErr) - - // Verify structure is preserved - require.NotEmpty(t, result, "Result should not be empty") - - // Verify line breaks are maintained (multi-line error) - assert.Contains(t, result, "\n", "Multi-line structure should be preserved") - - // Verify all sections are present - sections := []string{ - "ERROR", - "actions", - "issues", - "list_workflows", - "list_issues", - "create_issue", - "Suggested fix", - "toolsets:", - } - - for _, section := range sections { - assert.Contains(t, result, section, - "Structured error should contain section: %s", section) - } - - // Verify the error message contains the original structured content - originalMsg := structuredErr.Error() - lines := strings.SplitSeq(originalMsg, "\n") - for line := range lines { - if strings.TrimSpace(line) != "" { - assert.Contains(t, result, strings.TrimSpace(line), - "Structured error should preserve line: %s", line) - } - } -} - -// TestFormatValidationErrorContentIntegrity verifies that formatting doesn't alter error content -func TestFormatValidationErrorContentIntegrity(t *testing.T) { - errorMessages := []string{ - "simple error", - "error with special chars: @#$%^&*()", - "error with path: /home/user/file.txt", - "error with URL: https://example.com", - "error with code snippet: engine: copilot", - "multi\nline\nerror\nwith\nbreaks", - "error with numbers: 123 456 789", - "error with quotes: 'single' and \"double\"", - } - - for _, msg := range errorMessages { - t.Run("content_integrity_"+strings.ReplaceAll(msg, "\n", "_"), func(t *testing.T) { - err := errors.New(msg) - result := FormatValidationError(err) - - // Verify the original message content is present in the result - assert.Contains(t, result, msg, - "Formatted error must preserve original content") - - // Verify no content is lost or corrupted - // The formatted version should contain at least as many meaningful characters - originalLength := len(strings.TrimSpace(msg)) - // Remove common ANSI codes to get actual content length - cleanResult := strings.ReplaceAll(result, "\033[", "") - cleanResult = strings.ReplaceAll(cleanResult, "\x1b[", "") - - if len(cleanResult) < originalLength { - t.Errorf("Formatting appears to have removed content. Original: %d chars, Result: %d chars", - originalLength, len(cleanResult)) - } - }) - } -} diff --git a/pkg/console/form.go b/pkg/console/form.go deleted file mode 100644 index 91078e0e0e9..00000000000 --- a/pkg/console/form.go +++ /dev/null @@ -1,122 +0,0 @@ -//go:build !js && !wasm - -package console - -import ( - "errors" - "fmt" - - "github.com/charmbracelet/huh" - "github.com/github/gh-aw/pkg/tty" -) - -// RunForm executes a multi-field form with validation -// This is a higher-level helper that creates a form with multiple fields -func RunForm(fields []FormField) error { - // Validate inputs first before checking TTY - if len(fields) == 0 { - return errors.New("no form fields provided") - } - - // Validate field configurations before checking TTY - for _, field := range fields { - if field.Type == "select" && len(field.Options) == 0 { - return fmt.Errorf("select field '%s' requires options", field.Title) - } - if field.Type != "input" && field.Type != "password" && field.Type != "confirm" && field.Type != "select" { - return fmt.Errorf("unknown field type: %s", field.Type) - } - } - - // Check if stdin is a TTY - if not, we can't show interactive forms - if !tty.IsStderrTerminal() { - return errors.New("interactive forms not available (not a TTY)") - } - - // Build form fields - var huhFields []huh.Field - for _, field := range fields { - switch field.Type { - case "input": - inputField := huh.NewInput(). - Title(field.Title). - Description(field.Description). - Placeholder(field.Placeholder) - - if field.Validate != nil { - inputField.Validate(field.Validate) - } - - // Type assert to *string - if strPtr, ok := field.Value.(*string); ok { - inputField.Value(strPtr) - } else { - return fmt.Errorf("input field '%s' requires *string value", field.Title) - } - - huhFields = append(huhFields, inputField) - - case "password": - passwordField := huh.NewInput(). - Title(field.Title). - Description(field.Description). - EchoMode(huh.EchoModePassword) - - if field.Validate != nil { - passwordField.Validate(field.Validate) - } - - // Type assert to *string - if strPtr, ok := field.Value.(*string); ok { - passwordField.Value(strPtr) - } else { - return fmt.Errorf("password field '%s' requires *string value", field.Title) - } - - huhFields = append(huhFields, passwordField) - - case "confirm": - confirmField := huh.NewConfirm(). - Title(field.Title) - - // Type assert to *bool - if boolPtr, ok := field.Value.(*bool); ok { - confirmField.Value(boolPtr) - } else { - return fmt.Errorf("confirm field '%s' requires *bool value", field.Title) - } - - huhFields = append(huhFields, confirmField) - - case "select": - selectField := huh.NewSelect[string](). - Title(field.Title). - Description(field.Description) - - // Convert options to huh.Option format - huhOptions := make([]huh.Option[string], len(field.Options)) - for i, opt := range field.Options { - huhOptions[i] = huh.NewOption(opt.Label, opt.Value) - } - selectField.Options(huhOptions...) - - // Type assert to *string - if strPtr, ok := field.Value.(*string); ok { - selectField.Value(strPtr) - } else { - return fmt.Errorf("select field '%s' requires *string value", field.Title) - } - - huhFields = append(huhFields, selectField) - - default: - } - } - - // Create and run the form - form := huh.NewForm( - huh.NewGroup(huhFields...), - ).WithAccessible(IsAccessibleMode()) - - return form.Run() -} diff --git a/pkg/console/form_test.go b/pkg/console/form_test.go deleted file mode 100644 index 64efed30b96..00000000000 --- a/pkg/console/form_test.go +++ /dev/null @@ -1,169 +0,0 @@ -//go:build !integration - -package console - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestRunForm(t *testing.T) { - t.Run("function signature", func(t *testing.T) { - // Verify the function exists and has the right signature - _ = RunForm - }) - - t.Run("requires fields", func(t *testing.T) { - fields := []FormField{} - - err := RunForm(fields) - require.Error(t, err, "Should error with no fields") - assert.Contains(t, err.Error(), "no form fields", "Error should mention missing fields") - }) - - t.Run("validates input field", func(t *testing.T) { - var name string - fields := []FormField{ - { - Type: "input", - Title: "Name", - Description: "Enter your name", - Value: &name, - }, - } - - err := RunForm(fields) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) - - t.Run("validates password field", func(t *testing.T) { - var password string - fields := []FormField{ - { - Type: "password", - Title: "Password", - Description: "Enter password", - Value: &password, - }, - } - - err := RunForm(fields) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) - - t.Run("validates confirm field", func(t *testing.T) { - var confirmed bool - fields := []FormField{ - { - Type: "confirm", - Title: "Confirm action", - Value: &confirmed, - }, - } - - err := RunForm(fields) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) - - t.Run("validates select field with options", func(t *testing.T) { - var selected string - fields := []FormField{ - { - Type: "select", - Title: "Choose option", - Description: "Select one", - Value: &selected, - Options: []SelectOption{ - {Label: "Option 1", Value: "opt1"}, - {Label: "Option 2", Value: "opt2"}, - }, - }, - } - - err := RunForm(fields) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) - - t.Run("rejects select field without options", func(t *testing.T) { - var selected string - fields := []FormField{ - { - Type: "select", - Title: "Choose option", - Value: &selected, - Options: []SelectOption{}, - }, - } - - err := RunForm(fields) - require.Error(t, err, "Should error with no options") - assert.Contains(t, err.Error(), "requires options", "Error should mention missing options") - }) - - t.Run("rejects unknown field type", func(t *testing.T) { - var value string - fields := []FormField{ - { - Type: "unknown", - Title: "Test", - Value: &value, - }, - } - - err := RunForm(fields) - require.Error(t, err, "Should error with unknown field type") - assert.Contains(t, err.Error(), "unknown field type", "Error should mention unknown type") - }) - - t.Run("validates input field with custom validator", func(t *testing.T) { - var name string - fields := []FormField{ - { - Type: "input", - Title: "Name", - Description: "Enter your name", - Value: &name, - Validate: func(s string) error { - if len(s) < 3 { - return errors.New("must be at least 3 characters") - } - return nil - }, - }, - } - - err := RunForm(fields) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) -} - -func TestFormField(t *testing.T) { - t.Run("struct creation", func(t *testing.T) { - var value string - field := FormField{ - Type: "input", - Title: "Test Field", - Description: "Test Description", - Placeholder: "Enter value", - Value: &value, - } - - assert.Equal(t, "input", field.Type, "Type should match") - assert.Equal(t, "Test Field", field.Title, "Title should match") - assert.Equal(t, "Test Description", field.Description, "Description should match") - assert.Equal(t, "Enter value", field.Placeholder, "Placeholder should match") - }) -} diff --git a/pkg/console/golden_test.go b/pkg/console/golden_test.go index 0c2acc0413d..648da3cf655 100644 --- a/pkg/console/golden_test.go +++ b/pkg/console/golden_test.go @@ -7,9 +7,7 @@ import ( "strings" "testing" - "github.com/charmbracelet/lipgloss" "github.com/charmbracelet/x/exp/golden" - "github.com/github/gh-aw/pkg/styles" ) // TestGolden_TableRendering tests table rendering with different configurations @@ -132,36 +130,6 @@ func TestGolden_BoxRendering(t *testing.T) { } // TestGolden_LayoutBoxRendering tests layout box rendering (returns string) -func TestGolden_LayoutBoxRendering(t *testing.T) { - tests := []struct { - name string - title string - width int - }{ - { - name: "layout_narrow", - title: "Test", - width: 30, - }, - { - name: "layout_medium", - title: "Trial Execution Plan", - width: 60, - }, - { - name: "layout_wide", - title: "GitHub Agentic Workflows Compilation Report", - width: 100, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutTitleBox(tt.title, tt.width) - golden.RequireEqual(t, []byte(output)) - }) - } -} // TestGolden_TreeRendering tests tree rendering with different hierarchies func TestGolden_TreeRendering(t *testing.T) { @@ -467,94 +435,8 @@ func TestGolden_MessageFormatting(t *testing.T) { } // TestGolden_LayoutComposition tests composing multiple layout elements -func TestGolden_LayoutComposition(t *testing.T) { - tests := []struct { - name string - sections func() []string - }{ - { - name: "title_and_info", - sections: func() []string { - return []string{ - LayoutTitleBox("Trial Execution Plan", 60), - "", - LayoutInfoSection("Workflow", "test-workflow"), - LayoutInfoSection("Status", "Ready"), - } - }, - }, - { - name: "complete_composition", - sections: func() []string { - return []string{ - LayoutTitleBox("Trial Execution Plan", 60), - "", - LayoutInfoSection("Workflow", "test-workflow"), - LayoutInfoSection("Status", "Ready"), - "", - LayoutEmphasisBox("⚠️ WARNING: Large workflow file", styles.ColorWarning), - } - }, - }, - { - name: "multiple_emphasis_boxes", - sections: func() []string { - return []string{ - LayoutEmphasisBox("✓ Success", styles.ColorSuccess), - "", - LayoutEmphasisBox("⚠️ Warning", styles.ColorWarning), - "", - LayoutEmphasisBox("✗ Error", styles.ColorError), - } - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - sections := tt.sections() - output := LayoutJoinVertical(sections...) - golden.RequireEqual(t, []byte(output)) - }) - } -} // TestGolden_LayoutEmphasisBox tests emphasis boxes with different colors -func TestGolden_LayoutEmphasisBox(t *testing.T) { - tests := []struct { - name string - content string - color lipgloss.AdaptiveColor - }{ - { - name: "error_box", - content: "✗ ERROR: Compilation failed", - color: styles.ColorError, - }, - { - name: "warning_box", - content: "⚠️ WARNING: Deprecated syntax", - color: styles.ColorWarning, - }, - { - name: "success_box", - content: "✓ SUCCESS: All tests passed", - color: styles.ColorSuccess, - }, - { - name: "info_box", - content: "ℹ INFO: Processing workflow", - color: styles.ColorInfo, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutEmphasisBox(tt.content, tt.color) - golden.RequireEqual(t, []byte(output)) - }) - } -} // TestGolden_InfoSection tests info section rendering func TestGolden_InfoSection(t *testing.T) { diff --git a/pkg/console/layout.go b/pkg/console/layout.go deleted file mode 100644 index 3ea85a55978..00000000000 --- a/pkg/console/layout.go +++ /dev/null @@ -1,162 +0,0 @@ -//go:build !js && !wasm - -// Package console provides layout composition helpers for creating styled CLI output with Lipgloss. -// -// # Layout Composition Helpers -// -// The layout package provides reusable helper functions for common Lipgloss layout patterns. -// These helpers automatically respect TTY detection and provide both styled (TTY) and plain text -// (non-TTY) output modes. -// -// # Usage Example -// -// Here's a complete example showing how to compose a styled CLI output: -// -// import ( -// "fmt" -// "os" -// "github.com/github/gh-aw/pkg/console" -// "github.com/github/gh-aw/pkg/styles" -// ) -// -// // Create layout elements -// title := console.LayoutTitleBox("Trial Execution Plan", 60) -// info1 := console.LayoutInfoSection("Workflow", "test-workflow") -// info2 := console.LayoutInfoSection("Status", "Ready") -// warning := console.LayoutEmphasisBox("⚠️ WARNING: Large workflow file", styles.ColorWarning) -// -// // Compose sections vertically with spacing -// output := console.LayoutJoinVertical(title, "", info1, info2, "", warning) -// fmt.Fprintln(os.Stderr, output) -// -// # TTY Detection -// -// All layout helpers automatically detect whether output is going to a terminal (TTY) or being -// piped/redirected. In TTY mode, they use Lipgloss styling with colors and borders. In non-TTY -// mode, they output plain text suitable for parsing or logging. -// -// # Available Helpers -// -// - LayoutTitleBox: Centered title with double border -// - LayoutInfoSection: Info section with left border emphasis -// - LayoutEmphasisBox: Thick-bordered box with custom color -// - LayoutJoinVertical: Composes sections with automatic spacing -// -// # Comparison with Existing Functions -// -// These helpers complement the existing RenderTitleBox, RenderInfoSection, and -// RenderComposedSections functions in console.go. The key differences: -// -// - Layout helpers return strings instead of []string for simpler composition -// - LayoutInfoSection takes separate label and value parameters -// - LayoutEmphasisBox provides custom color support with thick borders -// - Layout helpers are designed for inline composition and chaining -package console - -import ( - "strings" - - "github.com/charmbracelet/lipgloss" - "github.com/github/gh-aw/pkg/styles" - "github.com/github/gh-aw/pkg/tty" -) - -// LayoutTitleBox renders a title with a double border box as a single string. -// In TTY mode, uses Lipgloss styled box centered with the Info color scheme. -// In non-TTY mode, renders plain text with separator lines. -// This is a simpler alternative to RenderTitleBox that returns a string instead of []string. -// -// Example: -// -// title := console.LayoutTitleBox("Trial Execution Plan", 60) -// fmt.Fprintln(os.Stderr, title) -func LayoutTitleBox(title string, width int) string { - if tty.IsStderrTerminal() { - // TTY mode: Use Lipgloss styled box - box := lipgloss.NewStyle(). - Bold(true). - Foreground(styles.ColorInfo). - Border(lipgloss.DoubleBorder(), true, false). - Padding(0, 2). - Width(width). - Align(lipgloss.Center). - Render(title) - return box - } - - // Non-TTY mode: Plain text with separators - separator := strings.Repeat("=", width) - return separator + "\n " + title + "\n" + separator -} - -// LayoutInfoSection renders an info section with left border emphasis as a single string. -// In TTY mode, uses Lipgloss styled section with left border and padding. -// In non-TTY mode, adds manual indentation. -// This is a simpler alternative to RenderInfoSection that returns a string and takes label/value. -// -// Example: -// -// info := console.LayoutInfoSection("Workflow", "test-workflow") -// fmt.Fprintln(os.Stderr, info) -func LayoutInfoSection(label, value string) string { - content := label + ": " + value - - if tty.IsStderrTerminal() { - // TTY mode: Use Lipgloss styled section with left border and padding - section := lipgloss.NewStyle(). - Border(lipgloss.NormalBorder(), false, false, false, true). - BorderForeground(styles.ColorInfo). - PaddingLeft(2). - Render(content) - return section - } - - // Non-TTY mode: Add manual indentation - return " " + content -} - -// LayoutEmphasisBox renders content in a rounded-bordered box with custom color. -// In TTY mode, uses Lipgloss styled box with rounded border for a polished appearance. -// In non-TTY mode, renders content with surrounding marker lines. -// -// Example: -// -// warning := console.LayoutEmphasisBox("⚠️ WARNING: Large workflow", styles.ColorWarning) -// fmt.Fprintln(os.Stderr, warning) -func LayoutEmphasisBox(content string, color lipgloss.AdaptiveColor) string { - if tty.IsStderrTerminal() { - // TTY mode: Use Lipgloss styled box with rounded border for a softer appearance - box := lipgloss.NewStyle(). - Bold(true). - Foreground(color). - Border(styles.RoundedBorder). - BorderForeground(color). - Padding(0, 2). - Render(content) - return box - } - - // Non-TTY mode: Content with marker lines - marker := strings.Repeat("!", len(content)+4) - return marker + "\n " + content + "\n" + marker -} - -// LayoutJoinVertical composes sections vertically with automatic spacing. -// In TTY mode, uses lipgloss.JoinVertical for proper composition. -// In non-TTY mode, joins sections with newlines. -// -// Example: -// -// title := console.LayoutTitleBox("Plan", 60) -// info := console.LayoutInfoSection("Status", "Ready") -// output := console.LayoutJoinVertical(title, info) -// fmt.Fprintln(os.Stderr, output) -func LayoutJoinVertical(sections ...string) string { - if tty.IsStderrTerminal() { - // TTY mode: Use Lipgloss to compose sections vertically - return lipgloss.JoinVertical(lipgloss.Left, sections...) - } - - // Non-TTY mode: Join with newlines - return strings.Join(sections, "\n") -} diff --git a/pkg/console/layout_test.go b/pkg/console/layout_test.go deleted file mode 100644 index 6360c99d06e..00000000000 --- a/pkg/console/layout_test.go +++ /dev/null @@ -1,383 +0,0 @@ -//go:build !integration - -package console - -import ( - "strings" - "testing" - - "github.com/charmbracelet/lipgloss" - "github.com/github/gh-aw/pkg/styles" -) - -func TestLayoutTitleBox(t *testing.T) { - tests := []struct { - name string - title string - width int - expected []string // Substrings that should be present in output - }{ - { - name: "basic title", - title: "Test Title", - width: 40, - expected: []string{ - "Test Title", - }, - }, - { - name: "longer title", - title: "Trial Execution Plan", - width: 80, - expected: []string{ - "Trial Execution Plan", - }, - }, - { - name: "title with special characters", - title: "⚠️ Important Notice", - width: 60, - expected: []string{ - "⚠️ Important Notice", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutTitleBox(tt.title, tt.width) - - // Check that output is not empty - if output == "" { - t.Error("LayoutTitleBox() returned empty string") - } - - // Check that title appears in output - for _, expected := range tt.expected { - if !strings.Contains(output, expected) { - t.Errorf("LayoutTitleBox() output missing expected string '%s'\nGot:\n%s", expected, output) - } - } - }) - } -} - -func TestLayoutInfoSection(t *testing.T) { - tests := []struct { - name string - label string - value string - expected []string // Substrings that should be present in output - }{ - { - name: "simple label and value", - label: "Workflow", - value: "test-workflow", - expected: []string{ - "Workflow", - "test-workflow", - }, - }, - { - name: "status label", - label: "Status", - value: "Active", - expected: []string{ - "Status", - "Active", - }, - }, - { - name: "file path value", - label: "Location", - value: "/path/to/file", - expected: []string{ - "Location", - "/path/to/file", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutInfoSection(tt.label, tt.value) - - // Check that output is not empty - if output == "" { - t.Error("LayoutInfoSection() returned empty string") - } - - // Check that expected strings appear in output - for _, expected := range tt.expected { - if !strings.Contains(output, expected) { - t.Errorf("LayoutInfoSection() output missing expected string '%s'\nGot:\n%s", expected, output) - } - } - }) - } -} - -func TestLayoutEmphasisBox(t *testing.T) { - tests := []struct { - name string - content string - color lipgloss.AdaptiveColor - expected []string // Substrings that should be present in output - }{ - { - name: "warning message", - content: "⚠️ WARNING", - color: styles.ColorWarning, - expected: []string{ - "⚠️ WARNING", - }, - }, - { - name: "error message", - content: "✗ ERROR: Failed", - color: styles.ColorError, - expected: []string{ - "✗ ERROR: Failed", - }, - }, - { - name: "success message", - content: "✓ Success", - color: styles.ColorSuccess, - expected: []string{ - "✓ Success", - }, - }, - { - name: "info message", - content: "ℹ Information", - color: styles.ColorInfo, - expected: []string{ - "ℹ Information", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutEmphasisBox(tt.content, tt.color) - - // Check that output is not empty - if output == "" { - t.Error("LayoutEmphasisBox() returned empty string") - } - - // Check that content appears in output - for _, expected := range tt.expected { - if !strings.Contains(output, expected) { - t.Errorf("LayoutEmphasisBox() output missing expected string '%s'\nGot:\n%s", expected, output) - } - } - }) - } -} - -func TestLayoutJoinVertical(t *testing.T) { - tests := []struct { - name string - sections []string - expected []string // Substrings that should be present in output - }{ - { - name: "single section", - sections: []string{"Section 1"}, - expected: []string{"Section 1"}, - }, - { - name: "multiple sections", - sections: []string{"Section 1", "Section 2", "Section 3"}, - expected: []string{ - "Section 1", - "Section 2", - "Section 3", - }, - }, - { - name: "sections with empty strings", - sections: []string{"Section 1", "", "Section 2"}, - expected: []string{ - "Section 1", - "Section 2", - }, - }, - { - name: "empty sections", - sections: []string{}, - expected: []string{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutJoinVertical(tt.sections...) - - // For empty sections, output should be empty - if len(tt.sections) == 0 { - if output != "" { - t.Errorf("LayoutJoinVertical() expected empty string, got: %s", output) - } - return - } - - // Check that expected strings appear in output - for _, expected := range tt.expected { - if expected == "" { - continue - } - if !strings.Contains(output, expected) { - t.Errorf("LayoutJoinVertical() output missing expected string '%s'\nGot:\n%s", expected, output) - } - } - }) - } -} - -func TestLayoutCompositionAPI(t *testing.T) { - t.Run("compose multiple layout elements", func(t *testing.T) { - // Test the API example from the documentation - title := LayoutTitleBox("Trial Execution Plan", 60) - info := LayoutInfoSection("Workflow", "test-workflow") - warning := LayoutEmphasisBox("⚠️ WARNING", styles.ColorWarning) - - // Compose sections vertically with spacing - output := LayoutJoinVertical(title, "", info, "", warning) - - // Verify all elements are present in output - expected := []string{ - "Trial Execution Plan", - "Workflow", - "test-workflow", - "⚠️ WARNING", - } - - for _, exp := range expected { - if !strings.Contains(output, exp) { - t.Errorf("Composed output missing expected string '%s'\nGot:\n%s", exp, output) - } - } - }) -} - -func TestLayoutWidthConstraints(t *testing.T) { - tests := []struct { - name string - width int - }{ - {"narrow width", 40}, - {"medium width", 60}, - {"wide width", 80}, - {"very wide width", 120}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - output := LayoutTitleBox("Test", tt.width) - - // Output should not be empty - if output == "" { - t.Error("LayoutTitleBox() returned empty string") - } - - // In non-TTY mode, separator length should match width - // We can't test TTY mode easily, but we can check non-TTY - lines := strings.Split(output, "\n") - if len(lines) > 0 { - // First line should contain separators or styled content - if len(lines[0]) == 0 { - t.Error("LayoutTitleBox() first line is empty") - } - } - }) - } -} - -func TestLayoutWithDifferentColors(t *testing.T) { - colors := []struct { - name string - color lipgloss.AdaptiveColor - }{ - {"error color", styles.ColorError}, - {"warning color", styles.ColorWarning}, - {"success color", styles.ColorSuccess}, - {"info color", styles.ColorInfo}, - {"purple color", styles.ColorPurple}, - {"yellow color", styles.ColorYellow}, - } - - for _, c := range colors { - t.Run(c.name, func(t *testing.T) { - output := LayoutEmphasisBox("Test Content", c.color) - - // Output should not be empty - if output == "" { - t.Error("LayoutEmphasisBox() returned empty string") - } - - // Content should be present - if !strings.Contains(output, "Test Content") { - t.Errorf("LayoutEmphasisBox() missing content, got: %s", output) - } - }) - } -} - -func TestLayoutNonTTYOutput(t *testing.T) { - // These tests verify that non-TTY output is plain text - // In actual non-TTY environment, output should be plain without ANSI codes - - t.Run("title box non-tty format", func(t *testing.T) { - output := LayoutTitleBox("Test", 40) - // Should contain the title - if !strings.Contains(output, "Test") { - t.Errorf("Expected title in output, got: %s", output) - } - }) - - t.Run("info section non-tty format", func(t *testing.T) { - output := LayoutInfoSection("Label", "Value") - // Should contain label and value - if !strings.Contains(output, "Label") || !strings.Contains(output, "Value") { - t.Errorf("Expected label and value in output, got: %s", output) - } - }) - - t.Run("emphasis box non-tty format", func(t *testing.T) { - output := LayoutEmphasisBox("Content", styles.ColorWarning) - // Should contain content - if !strings.Contains(output, "Content") { - t.Errorf("Expected content in output, got: %s", output) - } - }) -} - -// Example demonstrates how to compose a styled CLI output -// using the layout helper functions. -func Example() { - // Create layout elements - title := LayoutTitleBox("Trial Execution Plan", 60) - info1 := LayoutInfoSection("Workflow", "test-workflow") - info2 := LayoutInfoSection("Status", "Ready") - warning := LayoutEmphasisBox("⚠️ WARNING: Large workflow file", styles.ColorWarning) - - // Compose sections vertically with spacing - output := LayoutJoinVertical(title, "", info1, info2, "", warning) - - // In a real application, you would output to stderr: - // fmt.Fprintln(os.Stderr, output) - - // For test purposes, just verify the output contains expected content - if !strings.Contains(output, "Trial Execution Plan") { - panic("missing title") - } - if !strings.Contains(output, "test-workflow") { - panic("missing workflow name") - } - if !strings.Contains(output, "WARNING") { - panic("missing warning") - } -} diff --git a/pkg/console/select.go b/pkg/console/select.go deleted file mode 100644 index 0d2a94a0ba6..00000000000 --- a/pkg/console/select.go +++ /dev/null @@ -1,91 +0,0 @@ -//go:build !js && !wasm - -package console - -import ( - "errors" - - "github.com/charmbracelet/huh" - "github.com/github/gh-aw/pkg/tty" -) - -// PromptSelect shows an interactive single-select menu -// Returns the selected value or an error -func PromptSelect(title, description string, options []SelectOption) (string, error) { - // Validate inputs first - if len(options) == 0 { - return "", errors.New("no options provided") - } - - // Check if stdin is a TTY - if not, we can't show interactive forms - if !tty.IsStderrTerminal() { - return "", errors.New("interactive selection not available (not a TTY)") - } - - var selected string - - // Convert options to huh.Option format - huhOptions := make([]huh.Option[string], len(options)) - for i, opt := range options { - huhOptions[i] = huh.NewOption(opt.Label, opt.Value) - } - - form := huh.NewForm( - huh.NewGroup( - huh.NewSelect[string](). - Title(title). - Description(description). - Options(huhOptions...). - Value(&selected), - ), - ).WithAccessible(IsAccessibleMode()) - - if err := form.Run(); err != nil { - return "", err - } - - return selected, nil -} - -// PromptMultiSelect shows an interactive multi-select menu -// Returns the selected values or an error -func PromptMultiSelect(title, description string, options []SelectOption, limit int) ([]string, error) { - // Validate inputs first - if len(options) == 0 { - return nil, errors.New("no options provided") - } - - // Check if stdin is a TTY - if not, we can't show interactive forms - if !tty.IsStderrTerminal() { - return nil, errors.New("interactive selection not available (not a TTY)") - } - - var selected []string - - // Convert options to huh.Option format - huhOptions := make([]huh.Option[string], len(options)) - for i, opt := range options { - huhOptions[i] = huh.NewOption(opt.Label, opt.Value) - } - - multiSelect := huh.NewMultiSelect[string](). - Title(title). - Description(description). - Options(huhOptions...). - Value(&selected) - - // Set limit if specified (0 means no limit) - if limit > 0 { - multiSelect.Limit(limit) - } - - form := huh.NewForm( - huh.NewGroup(multiSelect), - ).WithAccessible(IsAccessibleMode()) - - if err := form.Run(); err != nil { - return nil, err - } - - return selected, nil -} diff --git a/pkg/console/select_test.go b/pkg/console/select_test.go deleted file mode 100644 index 9e513e8ac33..00000000000 --- a/pkg/console/select_test.go +++ /dev/null @@ -1,87 +0,0 @@ -//go:build !integration - -package console - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestPromptSelect(t *testing.T) { - t.Run("function signature", func(t *testing.T) { - // Verify the function exists and has the right signature - _ = PromptSelect - }) - - t.Run("requires options", func(t *testing.T) { - title := "Select an option" - description := "Choose one" - options := []SelectOption{} - - _, err := PromptSelect(title, description, options) - require.Error(t, err, "Should error with no options") - assert.Contains(t, err.Error(), "no options", "Error should mention missing options") - }) - - t.Run("validates parameters with options", func(t *testing.T) { - title := "Select an option" - description := "Choose one" - options := []SelectOption{ - {Label: "Option 1", Value: "opt1"}, - {Label: "Option 2", Value: "opt2"}, - } - - _, err := PromptSelect(title, description, options) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) -} - -func TestPromptMultiSelect(t *testing.T) { - t.Run("function signature", func(t *testing.T) { - // Verify the function exists and has the right signature - _ = PromptMultiSelect - }) - - t.Run("requires options", func(t *testing.T) { - title := "Select options" - description := "Choose multiple" - options := []SelectOption{} - limit := 0 - - _, err := PromptMultiSelect(title, description, options, limit) - require.Error(t, err, "Should error with no options") - assert.Contains(t, err.Error(), "no options", "Error should mention missing options") - }) - - t.Run("validates parameters with options", func(t *testing.T) { - title := "Select options" - description := "Choose multiple" - options := []SelectOption{ - {Label: "Option 1", Value: "opt1"}, - {Label: "Option 2", Value: "opt2"}, - {Label: "Option 3", Value: "opt3"}, - } - limit := 10 - - _, err := PromptMultiSelect(title, description, options, limit) - // Will error in test environment (no TTY), but that's expected - require.Error(t, err, "Should error when not in TTY") - assert.Contains(t, err.Error(), "not a TTY", "Error should mention TTY") - }) -} - -func TestSelectOption(t *testing.T) { - t.Run("struct creation", func(t *testing.T) { - opt := SelectOption{ - Label: "Test Label", - Value: "test-value", - } - - assert.Equal(t, "Test Label", opt.Label, "Label should match") - assert.Equal(t, "test-value", opt.Value, "Value should match") - }) -} diff --git a/pkg/logger/error_formatting.go b/pkg/logger/error_formatting.go deleted file mode 100644 index 6ba2d55086e..00000000000 --- a/pkg/logger/error_formatting.go +++ /dev/null @@ -1,47 +0,0 @@ -package logger - -import ( - "regexp" - "strings" -) - -// Pre-compiled regexes for performance (avoid recompiling in hot paths). -var ( - // Timestamp patterns for log cleanup - // Pattern 1: ISO 8601 with T or space separator (e.g., "2024-01-01T12:00:00.123Z " or "2024-01-01 12:00:00 "). - timestampPattern1 = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}[T\s]\d{2}:\d{2}:\d{2}(\.\d+)?([+-]\d{2}:\d{2}|Z)?\s*`) - // Pattern 2: Bracketed date-time (e.g., "[2024-01-01 12:00:00] "). - timestampPattern2 = regexp.MustCompile(`^\[\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}:\d{2}\]\s*`) - // Pattern 3: Bracketed time only (e.g., "[12:00:00] "). - timestampPattern3 = regexp.MustCompile(`^\[\d{2}:\d{2}:\d{2}\]\s+`) - // Pattern 4: Time only with optional milliseconds (e.g., "12:00:00.123 "). - timestampPattern4 = regexp.MustCompile(`^\d{2}:\d{2}:\d{2}(\.\d+)?\s+`) - - // Log level pattern for message cleanup (case-insensitive). - logLevelPattern = regexp.MustCompile(`(?i)^\[?(ERROR|WARNING|WARN|INFO|DEBUG)\]?\s*[:-]?\s*`) -) - -// ExtractErrorMessage extracts a clean error message from a log line. -// It removes timestamps, log level prefixes, and other common noise. -// If the message is longer than 200 characters, it will be truncated. -func ExtractErrorMessage(line string) string { - // Remove common timestamp patterns using pre-compiled regexes - cleanedLine := line - cleanedLine = timestampPattern1.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern2.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern3.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern4.ReplaceAllString(cleanedLine, "") - - // Remove common log level prefixes using pre-compiled regex - cleanedLine = logLevelPattern.ReplaceAllString(cleanedLine, "") - - // Trim whitespace - cleanedLine = strings.TrimSpace(cleanedLine) - - // If the line is too long (>200 chars), truncate it - if len(cleanedLine) > 200 { - cleanedLine = cleanedLine[:197] + "..." - } - - return cleanedLine -} diff --git a/pkg/logger/error_formatting_test.go b/pkg/logger/error_formatting_test.go deleted file mode 100644 index c07856146ae..00000000000 --- a/pkg/logger/error_formatting_test.go +++ /dev/null @@ -1,177 +0,0 @@ -//go:build !integration - -package logger - -import ( - "strings" - "testing" -) - -func TestExtractErrorMessage(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "ISO 8601 timestamp with T separator and Z", - input: "2024-01-01T12:00:00.123Z Error: connection failed", - expected: "connection failed", - }, - { - name: "ISO 8601 timestamp with T separator and timezone offset", - input: "2024-01-01T12:00:00.123+00:00 Error: connection failed", - expected: "connection failed", - }, - { - name: "Date-time with space separator", - input: "2024-01-01 12:00:00 Error: connection failed", - expected: "connection failed", - }, - { - name: "Date-time with space separator and milliseconds", - input: "2024-01-01 12:00:00.456 Error: connection failed", - expected: "connection failed", - }, - { - name: "Bracketed date-time", - input: "[2024-01-01 12:00:00] Error: connection failed", - expected: "connection failed", - }, - { - name: "Bracketed time only", - input: "[12:00:00] Error: connection failed", - expected: "connection failed", - }, - { - name: "Time only with milliseconds", - input: "12:00:00.123 Error: connection failed", - expected: "connection failed", - }, - { - name: "Time only without milliseconds", - input: "12:00:00 Error: connection failed", - expected: "connection failed", - }, - { - name: "ERROR prefix with colon", - input: "ERROR: connection failed", - expected: "connection failed", - }, - { - name: "ERROR prefix without colon", - input: "ERROR connection failed", - expected: "connection failed", - }, - { - name: "Bracketed ERROR prefix", - input: "[ERROR] connection failed", - expected: "connection failed", - }, - { - name: "Bracketed ERROR prefix with colon", - input: "[ERROR]: connection failed", - expected: "connection failed", - }, - { - name: "WARNING prefix", - input: "WARNING: disk space low", - expected: "disk space low", - }, - { - name: "WARN prefix", - input: "WARN: deprecated API used", - expected: "deprecated API used", - }, - { - name: "INFO prefix", - input: "INFO: service started", - expected: "service started", - }, - { - name: "DEBUG prefix", - input: "DEBUG: processing request", - expected: "processing request", - }, - { - name: "Case insensitive log level", - input: "error: connection failed", - expected: "connection failed", - }, - { - name: "Combined timestamp and log level", - input: "2024-01-01 12:00:00 ERROR: connection failed", - expected: "connection failed", - }, - { - name: "Combined ISO timestamp with Z and log level", - input: "2024-01-01T12:00:00Z ERROR: connection failed", - expected: "connection failed", - }, - { - name: "Multiple timestamps - only first is removed", - input: "[12:00:00] 2024-01-01 12:00:00 ERROR: connection failed", - expected: "2024-01-01 12:00:00 ERROR: connection failed", - }, - { - name: "No timestamp or log level", - input: "connection failed", - expected: "connection failed", - }, - { - name: "Empty string", - input: "", - expected: "", - }, - { - name: "Only whitespace", - input: " ", - expected: "", - }, - { - name: "Truncation at 200 chars", - input: "ERROR: " + strings.Repeat("a", 250), - expected: strings.Repeat("a", 197) + "...", - }, - { - name: "Exactly 200 chars - no truncation", - input: "ERROR: " + strings.Repeat("a", 193), - expected: strings.Repeat("a", 193), - }, - { - name: "Real world example from metrics.go", - input: "2024-01-15 14:30:22 ERROR: Failed to connect to database", - expected: "Failed to connect to database", - }, - { - name: "Real world example from copilot_agent.go", - input: "2024-01-15T14:30:22.123Z ERROR: API request failed", - expected: "API request failed", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ExtractErrorMessage(tt.input) - if result != tt.expected { - t.Errorf("ExtractErrorMessage(%q) = %q, want %q", tt.input, result, tt.expected) - } - }) - } -} - -func BenchmarkExtractErrorMessage(b *testing.B) { - testLine := "2024-01-01T12:00:00.123Z ERROR: connection failed to remote server" - - for b.Loop() { - ExtractErrorMessage(testLine) - } -} - -func BenchmarkExtractErrorMessageLong(b *testing.B) { - testLine := "2024-01-01T12:00:00.123Z ERROR: " + strings.Repeat("very long error message ", 20) - - for b.Loop() { - ExtractErrorMessage(testLine) - } -} diff --git a/pkg/parser/ansi_strip.go b/pkg/parser/ansi_strip.go deleted file mode 100644 index a8d911ca9fa..00000000000 --- a/pkg/parser/ansi_strip.go +++ /dev/null @@ -1,12 +0,0 @@ -package parser - -import ( - "github.com/github/gh-aw/pkg/stringutil" -) - -// StripANSI removes ANSI escape codes from a string. -// This is a thin wrapper around stringutil.StripANSI for backward compatibility. -// The comprehensive implementation lives in pkg/stringutil/ansi.go. -func StripANSI(s string) string { - return stringutil.StripANSI(s) -} diff --git a/pkg/parser/frontmatter_merge_test.go b/pkg/parser/frontmatter_merge_test.go index e0c23c3ef56..7af88499027 100644 --- a/pkg/parser/frontmatter_merge_test.go +++ b/pkg/parser/frontmatter_merge_test.go @@ -259,5 +259,3 @@ func TestMergeToolsFromJSON(t *testing.T) { }) } } - -// Test StripANSI function diff --git a/pkg/parser/frontmatter_utils_test.go b/pkg/parser/frontmatter_utils_test.go index c0142145d93..b9dba2eb71c 100644 --- a/pkg/parser/frontmatter_utils_test.go +++ b/pkg/parser/frontmatter_utils_test.go @@ -6,7 +6,6 @@ import ( "encoding/json" "os" "path/filepath" - "strings" "testing" "github.com/github/gh-aw/pkg/testutil" @@ -385,220 +384,8 @@ name: Test } // Test mergeToolsFromJSON function -func TestStripANSI(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "empty string", - input: "", - expected: "", - }, - { - name: "plain text without ANSI", - input: "Hello World", - expected: "Hello World", - }, - { - name: "simple CSI color sequence", - input: "\x1b[31mRed Text\x1b[0m", - expected: "Red Text", - }, - { - name: "multiple CSI sequences", - input: "\x1b[1m\x1b[31mBold Red\x1b[0m\x1b[32mGreen\x1b[0m", - expected: "Bold RedGreen", - }, - { - name: "CSI cursor movement", - input: "Line 1\x1b[2;1HLine 2", - expected: "Line 1Line 2", - }, - { - name: "CSI erase sequences", - input: "Text\x1b[2JCleared\x1b[K", - expected: "TextCleared", - }, - { - name: "OSC sequence with BEL terminator", - input: "\x1b]0;Window Title\x07Content", - expected: "Content", - }, - { - name: "OSC sequence with ST terminator", - input: "\x1b]2;Terminal Title\x1b\\More content", - expected: "More content", - }, - { - name: "character set selection G0", - input: "\x1b(0Hello\x1b(B", - expected: "Hello", - }, - { - name: "character set selection G1", - input: "\x1b)0World\x1b)B", - expected: "World", - }, - { - name: "keypad mode sequences", - input: "\x1b=Keypad\x1b>Normal", - expected: "KeypadNormal", - }, - { - name: "reset sequence", - input: "Before\x1bcAfter", - expected: "BeforeAfter", - }, - { - name: "save and restore cursor", - input: "Start\x1b7Middle\x1b8End", - expected: "StartMiddleEnd", - }, - { - name: "index and reverse index", - input: "Text\x1bDDown\x1bMUp", - expected: "TextDownUp", - }, - { - name: "next line and horizontal tab set", - input: "Line\x1bENext\x1bHTab", - expected: "LineNextTab", - }, - { - name: "complex CSI with parameters", - input: "\x1b[38;5;196mBright Red\x1b[48;5;21mBlue BG\x1b[0m", - expected: "Bright RedBlue BG", - }, - { - name: "CSI with semicolon parameters", - input: "\x1b[1;31;42mBold red on green\x1b[0m", - expected: "Bold red on green", - }, - { - name: "malformed escape at end", - input: "Text\x1b", - expected: "Text", - }, - { - name: "malformed CSI at end", - input: "Text\x1b[31", - expected: "Text", - }, - { - name: "malformed OSC at end", - input: "Text\x1b]0;Title", - expected: "Text", - }, - { - name: "escape followed by invalid character", - input: "Text\x1bXInvalid", - expected: "TextInvalid", - }, - { - name: "consecutive escapes", - input: "\x1b[31m\x1b[1m\x1b[4mText\x1b[0m", - expected: "Text", - }, - { - name: "mixed content with newlines", - input: "Line 1\n\x1b[31mRed Line 2\x1b[0m\nLine 3", - expected: "Line 1\nRed Line 2\nLine 3", - }, - { - name: "common terminal output", - input: "\x1b[?25l\x1b[2J\x1b[H\x1b[32m✓\x1b[0m Success", - expected: "✓ Success", - }, - { - name: "git diff style colors", - input: "\x1b[32m+Added line\x1b[0m\n\x1b[31m-Removed line\x1b[0m", - expected: "+Added line\n-Removed line", - }, - { - name: "unicode content with ANSI", - input: "\x1b[33m🎉 Success! 测试\x1b[0m", - expected: "🎉 Success! 测试", - }, - { - name: "very long CSI sequence", - input: "\x1b[1;2;3;4;5;6;7;8;9;10;11;12;13;14;15mLong params\x1b[0m", - expected: "Long params", - }, - { - name: "CSI with question mark private parameter", - input: "\x1b[?25hCursor visible\x1b[?25l", - expected: "Cursor visible", - }, - { - name: "CSI with greater than private parameter", - input: "\x1b[>0cDevice attributes\x1b[>1c", - expected: "Device attributes", - }, - { - name: "all final CSI characters test", - input: "\x1b[@\x1b[A\x1b[B\x1b[C\x1b[D\x1b[E\x1b[F\x1b[G\x1b[H\x1b[I\x1b[J\x1b[K\x1b[L\x1b[M\x1b[N\x1b[O\x1b[P\x1b[Q\x1b[R\x1b[S\x1b[T\x1b[U\x1b[V\x1b[W\x1b[X\x1b[Y\x1b[Z\x1b[[\x1b[\\\x1b[]\x1b[^\x1b[_\x1b[`\x1b[a\x1b[b\x1b[c\x1b[d\x1b[e\x1b[f\x1b[g\x1b[h\x1b[i\x1b[j\x1b[k\x1b[l\x1b[m\x1b[n\x1b[o\x1b[p\x1b[q\x1b[r\x1b[s\x1b[t\x1b[u\x1b[v\x1b[w\x1b[x\x1b[y\x1b[z\x1b[{\x1b[|\x1b[}\x1b[~Text", - expected: "Text", - }, - { - name: "CSI with invalid final character", - input: "Before\x1b[31Text after", - expected: "Beforeext after", - }, - { - name: "real world lipgloss output", - input: "\x1b[1;38;2;80;250;123m✓\x1b[0;38;2;248;248;242m Success message\x1b[0m", - expected: "✓ Success message", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := StripANSI(tt.input) - if result != tt.expected { - t.Errorf("StripANSI(%q) = %q, want %q", tt.input, result, tt.expected) - } - }) - } -} // Benchmark StripANSI function for performance -func BenchmarkStripANSI(b *testing.B) { - testCases := []struct { - name string - input string - }{ - { - name: "plain text", - input: "This is plain text without any ANSI codes", - }, - { - name: "simple color", - input: "\x1b[31mRed text\x1b[0m", - }, - { - name: "complex formatting", - input: "\x1b[1;38;2;255;0;0m\x1b[48;2;0;255;0mComplex formatting\x1b[0m", - }, - { - name: "mixed content", - input: "Normal \x1b[31mred\x1b[0m normal \x1b[32mgreen\x1b[0m normal \x1b[34mblue\x1b[0m text", - }, - { - name: "long text with ANSI", - input: strings.Repeat("\x1b[31mRed \x1b[32mGreen \x1b[34mBlue\x1b[0m ", 100), - }, - } - - for _, tc := range testCases { - b.Run(tc.name, func(b *testing.B) { - for range b.N { - StripANSI(tc.input) - } - }) - } -} func TestIsWorkflowSpec(t *testing.T) { tests := []struct { diff --git a/pkg/parser/virtual_fs_test_helpers.go b/pkg/parser/virtual_fs_test_helpers.go deleted file mode 100644 index 72e1095b67e..00000000000 --- a/pkg/parser/virtual_fs_test_helpers.go +++ /dev/null @@ -1,12 +0,0 @@ -package parser - -// SetReadFileFuncForTest overrides the file reading function for testing. -// This enables testing virtual filesystem behavior in native (non-wasm) builds. -// Returns a cleanup function that restores the original. -func SetReadFileFuncForTest(fn func(string) ([]byte, error)) func() { - original := readFileFunc - readFileFunc = fn - return func() { - readFileFunc = original - } -} diff --git a/pkg/workflow/metrics_test.go b/pkg/workflow/metrics_test.go index 8ad1a4ad57f..72d384997ad 100644 --- a/pkg/workflow/metrics_test.go +++ b/pkg/workflow/metrics_test.go @@ -5,8 +5,6 @@ package workflow import ( "encoding/json" "testing" - - "github.com/github/gh-aw/pkg/logger" ) func TestExtractFirstMatch(t *testing.T) { @@ -668,94 +666,6 @@ func TestPrettifyToolName(t *testing.T) { } } -func TestExtractErrorMessage(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "Simple error message", - input: "Failed to connect to server", - expected: "Failed to connect to server", - }, - { - name: "Error with timestamp prefix", - input: "2024-01-01 12:00:00 Connection timeout", - expected: "Connection timeout", - }, - { - name: "Error with timestamp and milliseconds", - input: "2024-01-01 12:00:00.123 Connection refused", - expected: "Connection refused", - }, - { - name: "Error with bracket timestamp", - input: "[12:00:00] Permission denied", - expected: "Permission denied", - }, - { - name: "Error with ERROR prefix", - input: "ERROR: File not found", - expected: "File not found", - }, - { - name: "Error with [ERROR] prefix", - input: "[ERROR] Invalid configuration", - expected: "Invalid configuration", - }, - { - name: "Warning with WARN prefix", - input: "WARN - Deprecated API usage", - expected: "Deprecated API usage", - }, - { - name: "Error with WARNING prefix", - input: "WARNING: Resource limit reached", - expected: "Resource limit reached", - }, - { - name: "Timestamp and log level combined", - input: "2024-01-01 12:00:00 ERROR: Failed to initialize", - expected: "Failed to initialize", - }, - { - name: "Very long message truncation", - input: "This is a very long error message that exceeds the maximum character limit and should be truncated to prevent overly verbose output in the audit report which could make it harder to read and understand the key issues", - expected: "This is a very long error message that exceeds the maximum character limit and should be truncated to prevent overly verbose output in the audit report which could make it harder to read and unders...", - }, - { - name: "Empty string", - input: "", - expected: "", - }, - { - name: "Only whitespace", - input: " \t ", - expected: "", - }, - { - name: "Case insensitive ERROR prefix", - input: "error: Connection failed", - expected: "Connection failed", - }, - { - name: "Mixed case WARNING prefix", - input: "Warning: Low memory", - expected: "Low memory", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := logger.ExtractErrorMessage(tt.input) - if result != tt.expected { - t.Errorf("logger.ExtractErrorMessage(%q) = %q, want %q", tt.input, result, tt.expected) - } - }) - } -} - func TestFinalizeToolMetrics(t *testing.T) { tests := []struct { name string