Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes a first batch of dead CLI/console/logger/parser code and trims the associated tests, plus adds documentation describing the dead-code removal methodology and plan.
Changes:
- Deleted unused CLI helpers (exec/log display/validation output/safe-inputs inspector) and console UI helpers (form/select/layout), along with their tests.
- Removed unused logger/parser utilities (error message extraction wrapper + ANSI stripping wrapper + virtual FS test helper) and updated tests that referenced them.
- Added
DEADCODE.mddocumenting the deadcode workflow, lessons learned, and phased removal plan.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/metrics_test.go | Removes tests/imports tied to deleted logger.ExtractErrorMessage. |
| pkg/parser/virtual_fs_test_helpers.go | Deletes unused test-only hook for virtual FS reads. |
| pkg/parser/frontmatter_utils_test.go | Removes ANSI-strip tests/benchmarks that depended on deleted wrapper. |
| pkg/parser/frontmatter_merge_test.go | Removes now-stale comment related to ANSI-strip tests. |
| pkg/parser/ansi_strip.go | Deletes thin parser.StripANSI wrapper in favor of stringutil.StripANSI. |
| pkg/logger/error_formatting_test.go | Deletes tests/benchmarks for removed error-extraction helper. |
| pkg/logger/error_formatting.go | Deletes ExtractErrorMessage implementation (confirmed no remaining references). |
| pkg/console/select_test.go | Deletes tests for removed interactive select helpers. |
| pkg/console/select.go | Deletes non-wasm interactive select implementation. |
| pkg/console/layout_test.go | Deletes tests for removed layout composition helpers. |
| pkg/console/layout.go | Deletes non-wasm layout composition helpers. |
| pkg/console/golden_test.go | Removes golden coverage for deleted layout helpers and trims unused imports. |
| pkg/console/form_test.go | Deletes tests for removed interactive form helper. |
| pkg/console/form.go | Deletes non-wasm interactive form helper. |
| pkg/cli/validation_output_test.go | Deletes tests for removed validation formatting/printing helpers. |
| pkg/cli/validation_output.go | Deletes dead validation error formatting/printing helpers. |
| pkg/cli/mcp_inspect_safe_inputs_test.go | Deletes tests for removed safe-inputs inspector entrypoint. |
| pkg/cli/mcp_inspect_safe_inputs_inspector.go | Deletes dead safe-inputs inspector spawning codepath. |
| pkg/cli/logs_overview_test.go | Removes tests that exercised deleted log overview rendering helper. |
| pkg/cli/logs_display.go | Deletes dead log overview table rendering helper. |
| pkg/cli/exec_test.go | Deletes tests for removed gh/fallback exec helper. |
| pkg/cli/exec.go | Deletes dead gh/fallback exec helper. |
| pkg/cli/copilot_agent_test.go | Removes tests/imports tied to deleted logger.ExtractErrorMessage. |
| DEADCODE.md | Adds documentation for dead-code removal methodology and phased plan. |
Comments suppressed due to low confidence (3)
pkg/cli/logs_overview_test.go:68
- This header comment is now orphaned after removing the corresponding test. Please remove it (or reintroduce the test) to avoid confusing readers about what’s covered.
// TestDisplayLogsOverviewWithVariousMissingToolCounts tests different scenarios
// TestTotalMissingToolsCalculation verifies totals are calculated correctly
pkg/cli/logs_overview_test.go:93
- These header comments reference removed tests (
TestOverviewDisplayConsistency,TestMissingToolsIntegration). Please remove or update them so the file’s comments match its current contents.
// TestOverviewDisplayConsistency verifies that the overview function is consistent
// TestMissingToolsIntegration tests the full flow from ProcessedRun to display
pkg/console/golden_test.go:133
- There is an orphaned header comment for
TestGolden_LayoutBoxRenderingbut the actual test function has been removed. Please delete or update this comment so it doesn’t suggest a missing golden test.
// TestGolden_LayoutBoxRendering tests layout box rendering (returns string)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
437
to
440
| // 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)) | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These two header comments refer to removed golden tests (TestGolden_LayoutComposition / TestGolden_LayoutEmphasisBox). Consider removing them to avoid implying coverage that no longer exists.
This issue also appears on line 132 of the same file.
See below for a potential fix:
Comment on lines
386
to
389
| // 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) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
After removing TestStripANSI / BenchmarkStripANSI, the section header comments here are now orphaned. Please remove or rewrite them so the file doesn’t look like it’s missing tests/benchmarks.
See below for a potential fix:
// Tests for isWorkflowSpec function.
Comment on lines
9
to
11
| // 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 |
There was a problem hiding this comment.
This comment refers to TestLogsOverviewIncludesMissingTools, but the test body has been removed. Please delete or update the comment to reflect the remaining assertions in this file.
This issue also appears in the following locations of the same file:
- line 66
- line 90
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/cli,pkg/console,pkg/logger, andpkg/parser, including unused exec helpers, log display, form/select UI, layout components, and ANSI stripping wrappersgo build,go vet, andmake fmtall passDEADCODE.mddocumenting the methodology, session history, and a phased removal plan for future batches