From 4d9038ad31bb86e310d3638204a875682e77f70f Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 11:26:55 -0600 Subject: [PATCH 1/7] Add pager support to log output in printLogs Introduces use of a pager for log output in the printLogs function. If the pager fails to start, an error message is printed to stderr. This improves log readability for long outputs. --- pkg/cmd/agent-task/view/view.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cmd/agent-task/view/view.go b/pkg/cmd/agent-task/view/view.go index 8d43b1f7e..832c20806 100644 --- a/pkg/cmd/agent-task/view/view.go +++ b/pkg/cmd/agent-task/view/view.go @@ -325,6 +325,12 @@ func printLogs(opts *ViewOptions, capiClient capi.CapiClient, sessionID string) renderer := opts.LogRenderer() + if err := opts.IO.StartPager(); err == nil { + defer opts.IO.StopPager() + } else { + fmt.Fprintf(opts.IO.ErrOut, "error starting pager: %v\n", err) + } + if opts.Follow { var called bool fetcher := func() ([]byte, error) { From 37d8e0a4380aa16971b7ac200e3129d56a754d0f Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 11:46:16 -0600 Subject: [PATCH 2/7] Refactor generic tool call titles map to package scope Moved the genericToolCallNamesToTitles map from inside the renderGenericToolCall function to a package-level variable for improved readability and potential reuse. This change also updates the function signature order for consistency. --- pkg/cmd/agent-task/shared/log.go | 111 ++++++++++++++++--------------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/pkg/cmd/agent-task/shared/log.go b/pkg/cmd/agent-task/shared/log.go index 7ca3dd9e5..9a91c2a14 100644 --- a/pkg/cmd/agent-task/shared/log.go +++ b/pkg/cmd/agent-task/shared/log.go @@ -422,62 +422,63 @@ func renderToolCallTitle(w io.Writer, cs *iostreams.ColorScheme, toolName, title } } +// genericToolCallNamesToTitles maps known generic tool call identifiers to human-friendly titles. +var genericToolCallNamesToTitles = map[string]string{ + // Custom tools, the GitHub UI doesn't currently have these. + "codeql_checker": "Run CodeQL analysis", + + // Playwright tools. + "playwright-browser_navigate": "Navigate Playwright web browser to a URL", + "playwright-browser_navigate_back": "Navigate back in Playwright web browser", + "playwright-browser_navigate_forward": "Navigate forward in Playwright web browser", + "playwright-browser_click": "Click element in Playwright web browser", + "playwright-browser_take_screenshot": "Take screenshot of Playwright web browser", + "playwright-browser_type": "Type in Playwright web browser", + "playwright-browser_wait_for": "Wait for text to appear/disappear in Playwright web browser", + "playwright-browser_evaluate": "Run JavaScript in Playwright web browser", + "playwright-browser_snapshot": "Take snapshot of page in Playwright web browser", + "playwright-browser_resize": "Resize Playwright web browser window", + "playwright-browser_close": "Close Playwright web browser", + "playwright-browser_press_key": "Press key in Playwright web browser", + "playwright-browser_select_option": "Select option in Playwright web browser", + "playwright-browser_handle_dialog": "Interact with dialog in Playwright web browser", + "playwright-browser_console_messages": "Get console messages from Playwright web browser", + "playwright-browser_drag": "Drag mouse between elements in Playwright web browser", + "playwright-browser_file_upload": "Upload file in Playwright web browser", + "playwright-browser_hover": "Hover mouse over element in Playwright web browser", + "playwright-browser_network_requests": "Get network requests from Playwright web browser", + + // GitHub MCP server common tools + "github-mcp-server-get_file_contents": "Get file contents from GitHub", + "github-mcp-server-get_pull_request": "Get pull request from GitHub", + "github-mcp-server-get_issue": "Get issue from GitHub", + "github-mcp-server-get_pull_request_files": "Get pull request changed files from GitHub", + "github-mcp-server-list_pull_requests": "List pull requests on GitHub", + "github-mcp-server-list_branches": "List branches on GitHub", + "github-mcp-server-get_pull_request_diff": "Get pull request diff from GitHub", + "github-mcp-server-get_pull_request_comments": "Get pull request comments from GitHub", + "github-mcp-server-get_commit": "Get commit from GitHub", + "github-mcp-server-search_repositories": "Search repositories on GitHub", + "github-mcp-server-search_code": "Search code on GitHub", + "github-mcp-server-get_issue_comments": "Get issue comments from GitHub", + "github-mcp-server-list_issues": "List issues on GitHub", + "github-mcp-server-search_pull_requests": "Search pull requests on GitHub", + "github-mcp-server-list_commits": "List commits on GitHub", + "github-mcp-server-get_pull_request_status": "Get pull request status from GitHub", + "github-mcp-server-search_issues": "Search issues on GitHub", + "github-mcp-server-get_pull_request_reviews": "Get pull request reviews from GitHub", + "github-mcp-server-download_workflow_run_artifact": "Download GitHub Actions workflow run artifact", + "github-mcp-server-get_job_logs": "Get GitHub Actions job logs", + "github-mcp-server-get_workflow_run": "Get GitHub Actions workflow run", + "github-mcp-server-get_workflow_run_logs": "Get GitHub Actions workflow run logs", + "github-mcp-server-get_workflow_run_usage": "Get GitHub Actions workflow usage", + "github-mcp-server-list_workflow_jobs": "List GitHub Actions workflow jobs", + "github-mcp-server-list_workflow_run_artifacts": "List GitHub Actions workflow run artifacts", + "github-mcp-server-list_workflow_runs": "List GitHub Actions workflow runs", + "github-mcp-server-list_workflows": "List GitHub Actions workflows", +} + func renderGenericToolCall(w io.Writer, cs *iostreams.ColorScheme, name string) { - genericToolCallNamesToTitles := map[string]string{ - // Custom tools, the GitHub UI doesn't currently have these. - "codeql_checker": "Run CodeQL analysis", - - // Playwright tools. - "playwright-browser_navigate": "Navigate Playwright web browser to a URL", - "playwright-browser_navigate_back": "Navigate back in Playwright web browser", - "playwright-browser_navigate_forward": "Navigate forward in Playwright web browser", - "playwright-browser_click": "Click element in Playwright web browser", - "playwright-browser_take_screenshot": "Take screenshot of Playwright web browser", - "playwright-browser_type": "Type in Playwright web browser", - "playwright-browser_wait_for": "Wait for text to appear/disappear in Playwright web browser", - "playwright-browser_evaluate": "Run JavaScript in Playwright web browser", - "playwright-browser_snapshot": "Take snapshot of page in Playwright web browser", - "playwright-browser_resize": "Resize Playwright web browser window", - "playwright-browser_close": "Close Playwright web browser", - "playwright-browser_press_key": "Press key in Playwright web browser", - "playwright-browser_select_option": "Select option in Playwright web browser", - "playwright-browser_handle_dialog": "Interact with dialog in Playwright web browser", - "playwright-browser_console_messages": "Get console messages from Playwright web browser", - "playwright-browser_drag": "Drag mouse between elements in Playwright web browser", - "playwright-browser_file_upload": "Upload file in Playwright web browser", - "playwright-browser_hover": "Hover mouse over element in Playwright web browser", - "playwright-browser_network_requests": "Get network requests from Playwright web browser", - - // GitHub MCP server common tools - "github-mcp-server-get_file_contents": "Get file contents from GitHub", - "github-mcp-server-get_pull_request": "Get pull request from GitHub", - "github-mcp-server-get_issue": "Get issue from GitHub", - "github-mcp-server-get_pull_request_files": "Get pull request changed files from GitHub", - "github-mcp-server-list_pull_requests": "List pull requests on GitHub", - "github-mcp-server-list_branches": "List branches on GitHub", - "github-mcp-server-get_pull_request_diff": "Get pull request diff from GitHub", - "github-mcp-server-get_pull_request_comments": "Get pull request comments from GitHub", - "github-mcp-server-get_commit": "Get commit from GitHub", - "github-mcp-server-search_repositories": "Search repositories on GitHub", - "github-mcp-server-search_code": "Search code on GitHub", - "github-mcp-server-get_issue_comments": "Get issue comments from GitHub", - "github-mcp-server-list_issues": "List issues on GitHub", - "github-mcp-server-search_pull_requests": "Search pull requests on GitHub", - "github-mcp-server-list_commits": "List commits on GitHub", - "github-mcp-server-get_pull_request_status": "Get pull request status from GitHub", - "github-mcp-server-search_issues": "Search issues on GitHub", - "github-mcp-server-get_pull_request_reviews": "Get pull request reviews from GitHub", - "github-mcp-server-download_workflow_run_artifact": "Download GitHub Actions workflow run artifact", - "github-mcp-server-get_job_logs": "Get GitHub Actions job logs", - "github-mcp-server-get_workflow_run": "Get GitHub Actions workflow run", - "github-mcp-server-get_workflow_run_logs": "Get GitHub Actions workflow run logs", - "github-mcp-server-get_workflow_run_usage": "Get GitHub Actions workflow usage", - "github-mcp-server-list_workflow_jobs": "List GitHub Actions workflow jobs", - "github-mcp-server-list_workflow_run_artifacts": "List GitHub Actions workflow run artifacts", - "github-mcp-server-list_workflow_runs": "List GitHub Actions workflow runs", - "github-mcp-server-list_workflows": "List GitHub Actions workflows", - } - toolName, ok := genericToolCallNamesToTitles[name] if !ok { toolName = fmt.Sprintf("Call to %s", name) From d636f4c2136ed9ce06b7fdf7a4816f764cda6bee Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 11:47:48 -0600 Subject: [PATCH 3/7] Clarify comment on CRLF normalization in tests Updated comments in log_test.go to clarify that CRLF is normalized to LF for OS-agnostic test behavior, improving code readability. --- pkg/cmd/agent-task/shared/log_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/agent-task/shared/log_test.go b/pkg/cmd/agent-task/shared/log_test.go index f5913bf4b..586890d9f 100644 --- a/pkg/cmd/agent-task/shared/log_test.go +++ b/pkg/cmd/agent-task/shared/log_test.go @@ -34,7 +34,7 @@ func TestFollow(t *testing.T) { raw, err := os.ReadFile(tt.log) require.NoError(t, err) - // Delete all the `/r` to make the tests OS-agnostic. + // Normalize CRLF to LF to make the tests OS-agnostic. raw = []byte(strings.ReplaceAll(string(raw), "\r\n", "\n")) lines := slices.DeleteFunc(strings.Split(string(raw), "\n"), func(line string) bool { @@ -63,7 +63,7 @@ func TestFollow(t *testing.T) { want, err := os.ReadFile(tt.want) require.NoError(t, err) - // Delete all the `/r` to make the tests OS-agnostic. + // Normalize CRLF to LF to make the tests OS-agnostic. want = []byte(strings.ReplaceAll(string(want), "\r\n", "\n")) assert.Equal(t, string(want), stdout.String()) From 97d3253aaf11ba6f1a366afc8be6772719b301cd Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 11:58:58 -0600 Subject: [PATCH 4/7] Prefix rendered shell commands with '$ ' in logs Shell commands in log output are now prefixed with '$ ' for improved readability and consistency with common shell output conventions. Updated related test data and test helper comments to reflect this change. --- pkg/cmd/agent-task/shared/log.go | 2 +- pkg/cmd/agent-task/shared/log_test.go | 3 ++- pkg/cmd/agent-task/shared/testdata/log-1-want.txt | 12 ++++++------ pkg/cmd/agent-task/shared/testdata/log-2-want.txt | 14 +++++++------- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/agent-task/shared/log.go b/pkg/cmd/agent-task/shared/log.go index 9a91c2a14..e58bed79a 100644 --- a/pkg/cmd/agent-task/shared/log.go +++ b/pkg/cmd/agent-task/shared/log.go @@ -153,7 +153,7 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I contentWithCommand := choice.Delta.Content if v.Command != "" { - contentWithCommand = fmt.Sprintf("%s\n%s", v.Command, choice.Delta.Content) + contentWithCommand = fmt.Sprintf("$ %s\n%s", v.Command, choice.Delta.Content) } if err := renderFileContentAsMarkdown("commands.sh", contentWithCommand, w, io); err != nil { return false, fmt.Errorf("failed to render bash command output: %w", err) diff --git a/pkg/cmd/agent-task/shared/log_test.go b/pkg/cmd/agent-task/shared/log_test.go index 586890d9f..4c95a9853 100644 --- a/pkg/cmd/agent-task/shared/log_test.go +++ b/pkg/cmd/agent-task/shared/log_test.go @@ -58,7 +58,8 @@ func TestFollow(t *testing.T) { // Handy note for updating the testdata files when they change: // ext := filepath.Ext(tt.log) // stripped := strings.TrimSuffix(tt.log, ext) - // os.WriteFile(stripped+".want"+ext, stdout.Bytes(), 0644) + // stripped = strings.TrimSuffix(stripped, "-input") + // os.WriteFile(stripped+"-want"+ext, stdout.Bytes(), 0644) want, err := os.ReadFile(tt.want) require.NoError(t, err) diff --git a/pkg/cmd/agent-task/shared/testdata/log-1-want.txt b/pkg/cmd/agent-task/shared/testdata/log-1-want.txt index e36d011a7..6ba206dda 100644 --- a/pkg/cmd/agent-task/shared/testdata/log-1-want.txt +++ b/pkg/cmd/agent-task/shared/testdata/log-1-want.txt @@ -11,7 +11,7 @@ View repository README.md Bash: Check current directory and list all files -cd /home/runner/work/foo/foo && pwd && ls -la +$ cd /home/runner/work/foo/foo && pwd && ls -la /home/runner/work/foo/foo total 20 drwxr-xr-x 4 runner docker 4096 Sep 9 10:17 . @@ -36,8 +36,8 @@ View README.md # foo Bash: Check if there are any existing scripts or build files -cd /home/runner/work/foo/foo && find . -name "*.sh" -o -name "Makefile" -o - - name "package.json" -o -name "*.yml" -o -name "*.yaml" +$ cd /home/runner/work/foo/foo && find . -name "*.sh" -o -name "Makefile" - + o -name "package.json" -o -name "*.yml" -o -name "*.yaml" ./.github/workflows/upload-artifact.yml ./.github/workflows/manual-dispatch.yml ./.github/workflows/cache.yml @@ -195,7 +195,7 @@ Create: os_info.sh Now let me make the script executable and test it: Bash: Make script executable and test it -cd /home/runner/work/foo/foo && chmod +x os_info.sh && ./os_info.sh +$ cd /home/runner/work/foo/foo && chmod +x os_info.sh && ./os_info.sh ================================================== OPERATING SYSTEM INFORMATION ================================================== @@ -270,7 +270,7 @@ diff --git a/home/runner/work/foo/foo/README.md Let me verify the changes look good: Bash: Check file permissions and final status -cd /home/runner/work/foo/foo && ls -la os_info.sh && echo "---" && git +$ cd /home/runner/work/foo/foo && ls -la os_info.sh && echo "---" && git status -rwxr-xr-x 1 runner docker 2139 Sep 9 10:18 os_info.sh --- @@ -323,7 +323,7 @@ Perfect! Let me test the script one more time to make sure everything is working correctly and commit the changes: Bash: Final test of the script -cd /home/runner/work/foo/foo && echo "Testing script execution:" && . +$ cd /home/runner/work/foo/foo && echo "Testing script execution:" && . /os_info.sh | head -10 Testing script execution: ================================================== diff --git a/pkg/cmd/agent-task/shared/testdata/log-2-want.txt b/pkg/cmd/agent-task/shared/testdata/log-2-want.txt index 066658f91..7665b4d51 100644 --- a/pkg/cmd/agent-task/shared/testdata/log-2-want.txt +++ b/pkg/cmd/agent-task/shared/testdata/log-2-want.txt @@ -6,7 +6,7 @@ I'll help you create a .yaml file with random fields in the repository. Let and then create a plan. Bash: Find documentation and config files to understand the repository -cd /home/runner/work/foo/foo && find . -type f -name "*.md" -o -name "*. +$ cd /home/runner/work/foo/foo && find . -type f -name "*.md" -o -name "*. txt" -o -name "*.json" -o -name "*.yaml" -o -name "*.yml" | head -20 ./README.md ./.github/workflows/upload-artifact.yml @@ -23,7 +23,7 @@ View repository README.md Bash: List all files in the root directory -cd /home/runner/work/foo/foo && ls -la +$ cd /home/runner/work/foo/foo && ls -la total 20 drwxr-xr-x 4 runner runner 4096 Sep 11 13:35 . drwxr-xr-x 3 runner runner 4096 Sep 11 13:34 .. @@ -37,7 +37,7 @@ View README.md # foo Bash: Check git status to understand current state -cd /home/runner/work/foo/foo && git --no-pager status +$ cd /home/runner/work/foo/foo && git --no-pager status On branch copilot/fix-5b1c2fcb-9248-492c-84fa-04a3e0304d25 Your branch is up to date with 'origin/copilot/fix-5b1c2fcb-9248-492c-84fa- 04a3e0304d25'. @@ -197,13 +197,13 @@ Now let me verify that the YAML file is valid by testing it with a YAML parser: Bash: Validate the YAML file syntax -cd /home/runner/work/foo/foo && python3 -c "import yaml; yaml. +$ cd /home/runner/work/foo/foo && python3 -c "import yaml; yaml. safe_load(open('sample-data.yaml', 'r')); print('YAML file is valid!')" Bash: Validate YAML and show structure -cd /home/runner/work/foo/foo && python3 -c "import yaml; data = yaml. +$ cd /home/runner/work/foo/foo && python3 -c "import yaml; data = yaml. safe_load(open('sample-data.yaml', 'r')); print('YAML file is valid!'); print(f'Loaded {len(data)} top-level keys: {list(data.keys())}')" @@ -213,7 +213,7 @@ cd /home/runner/work/foo/foo && python3 -c "import yaml; data = yaml. Let me try a different approach to verify the YAML: Bash: Detailed YAML validation with content verification -cd /home/runner/work/foo/foo && python3 -c " +$ cd /home/runner/work/foo/foo && python3 -c " import yaml try: with open('sample-data.yaml', 'r') as f: @@ -237,7 +237,7 @@ cd /home/runner/work/foo/foo && python3 -c " Perfect! Let me also check the file contents and verify the current status: Bash: Check git status to see the new file -cd /home/runner/work/foo/foo && git --no-pager status +$ cd /home/runner/work/foo/foo && git --no-pager status On branch copilot/fix-5b1c2fcb-9248-492c-84fa-04a3e0304d25 Your branch is up to date with 'origin/copilot/fix-5b1c2fcb-9248-492c-84fa- 04a3e0304d25'. From 65b45adbd83a98daa57770e340b448924edeca55 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:21:01 -0600 Subject: [PATCH 5/7] Improve log rendering error handling and test coverage Refactors log rendering to print errors to stderr and continue processing on JSON parse or rendering failures, instead of returning early. Updates tests to check both stdout and stderr outputs, and adds new test cases and testdata for tolerant parsing and error scenarios. --- pkg/cmd/agent-task/shared/log.go | 36 ++++++++------ pkg/cmd/agent-task/shared/log_test.go | 48 ++++++++++++++----- .../shared/testdata/log-3-input.txt | 27 +++++++++++ .../log-3-synthetic-failures-input.txt | 27 +++++++++++ .../log-3-synthetic-failures-want-stderr.txt | 10 ++++ .../log-3-synthetic-failures-want.txt | 39 +++++++++++++++ .../agent-task/shared/testdata/log-3-want.txt | 39 +++++++++++++++ 7 files changed, 199 insertions(+), 27 deletions(-) create mode 100644 pkg/cmd/agent-task/shared/testdata/log-3-input.txt create mode 100644 pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-input.txt create mode 100644 pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want-stderr.txt create mode 100644 pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want.txt create mode 100644 pkg/cmd/agent-task/shared/testdata/log-3-want.txt diff --git a/pkg/cmd/agent-task/shared/log.go b/pkg/cmd/agent-task/shared/log.go index e58bed79a..a1b1740e0 100644 --- a/pkg/cmd/agent-task/shared/log.go +++ b/pkg/cmd/agent-task/shared/log.go @@ -133,15 +133,16 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I case "view": args := viewToolArgs{} if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil { - return false, fmt.Errorf("failed to parse 'view' tool call arguments: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to parse 'view' tool call arguments: %v\n", err) + continue } - fmt.Fprintf(w, "View %s\n", cs.Bold(relativeFilePath(args.Path))) + renderToolCallTitle(w, cs, fmt.Sprintf("View %s", cs.Bold(relativeFilePath(args.Path))), "") content := stripDiffFormat(choice.Delta.Content) - // TODO: Strip the diff formatting from this, but for now render as it is. if err := renderFileContentAsMarkdown(args.Path, content, w, io); err != nil { - return false, fmt.Errorf("failed to render viewed file content: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render viewed file content: %v\n\n", err) + fmt.Fprintln(io.ErrOut, content) // raw fallback } case "bash": if v := unmarshal[bashToolArgs](args); v != nil { @@ -156,7 +157,8 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I contentWithCommand = fmt.Sprintf("$ %s\n%s", v.Command, choice.Delta.Content) } if err := renderFileContentAsMarkdown("commands.sh", contentWithCommand, w, io); err != nil { - return false, fmt.Errorf("failed to render bash command output: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render bash command output: %v\n\n", err) + fmt.Fprintln(io.ErrOut, contentWithCommand) } } // TODO: consider including more details for these bash-related tool calls. @@ -193,24 +195,26 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I case "think": args := thinkToolArgs{} if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil { - return false, fmt.Errorf("failed to parse 'think' tool call arguments: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to parse 'think' tool call arguments: %v\n", err) + continue } // NOTE: omit the delta.content since it's the same as thought renderToolCallTitle(w, cs, "Thought", "") if err := renderRawMarkdown(args.Thought, w, io); err != nil { - return false, fmt.Errorf("failed to render thought: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render thought: %v\n", err) } case "report_progress": args := reportProgressToolArgs{} if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil { - return false, fmt.Errorf("failed to parse 'report_progress' tool call arguments: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to parse 'report_progress' tool call arguments: %v\n", err) + continue } renderToolCallTitle(w, cs, "Progress update", cs.Bold(args.CommitMessage)) if args.PrDescription != "" { if err := renderRawMarkdown(args.PrDescription, w, io); err != nil { - return false, fmt.Errorf("failed to render PR description: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render PR description: %v\n", err) } } @@ -218,29 +222,33 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I if choice.Delta.Content != "" { // Try to treat this as JSON if err := renderContentAsJSONMarkdown("", choice.Delta.Content, w, io); err != nil { - return false, fmt.Errorf("failed to render progress update content: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render progress update content: %v\n", err) } } case "create": args := createToolArgs{} if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil { - return false, fmt.Errorf("failed to parse 'create' tool call arguments: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to parse 'create' tool call arguments: %v\n", err) + continue } renderToolCallTitle(w, cs, "Create", cs.Bold(relativeFilePath(args.Path))) if err := renderFileContentAsMarkdown(args.Path, args.FileText, w, io); err != nil { - return false, fmt.Errorf("failed to render created file content: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render created file content: %v\n\n", err) + fmt.Fprintln(io.ErrOut, args.FileText) } case "str_replace": args := strReplaceToolArgs{} if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil { - return false, fmt.Errorf("failed to parse 'str_replace' tool call arguments: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to parse 'str_replace' tool call arguments: %v\n", err) + continue } renderToolCallTitle(w, cs, "Edit", cs.Bold(relativeFilePath(args.Path))) if err := renderFileContentAsMarkdown("output.diff", choice.Delta.Content, w, io); err != nil { - return false, fmt.Errorf("failed to render str_replace diff: %w", err) + fmt.Fprintf(io.ErrOut, "\nfailed to render str_replace diff: %v\n\n", err) + fmt.Fprintln(io.ErrOut, choice.Delta.Content) } default: // Unknown tool call. For example for "codeql_checker": diff --git a/pkg/cmd/agent-task/shared/log_test.go b/pkg/cmd/agent-task/shared/log_test.go index 4c95a9853..07b562dc8 100644 --- a/pkg/cmd/agent-task/shared/log_test.go +++ b/pkg/cmd/agent-task/shared/log_test.go @@ -13,19 +13,26 @@ import ( func TestFollow(t *testing.T) { tests := []struct { - name string - log string - want string + name string + log string + wantStdoutFile string + wantStderrFile string }{ { - name: "sample log 1", - log: "testdata/log-1-input.txt", - want: "testdata/log-1-want.txt", + name: "sample log 1", + log: "testdata/log-1-input.txt", + wantStdoutFile: "testdata/log-1-want.txt", }, { - name: "sample log 2", - log: "testdata/log-2-input.txt", - want: "testdata/log-2-want.txt", + name: "sample log 2", + log: "testdata/log-2-input.txt", + wantStdoutFile: "testdata/log-2-want.txt", + }, + { + name: "sample log 3 (tolerant parse failures)", + log: "testdata/log-3-synthetic-failures-input.txt", + wantStdoutFile: "testdata/log-3-synthetic-failures-want.txt", + wantStderrFile: "testdata/log-3-synthetic-failures-want-stderr.txt", }, } @@ -50,7 +57,7 @@ func TestFollow(t *testing.T) { return []byte(strings.Join(lines[0:hits], "\n\n")), nil } - ios, _, stdout, _ := iostreams.Test() + ios, _, stdout, stderr := iostreams.Test() err = NewLogRenderer().Follow(fetcher, stdout, ios) require.NoError(t, err) @@ -60,14 +67,29 @@ func TestFollow(t *testing.T) { // stripped := strings.TrimSuffix(tt.log, ext) // stripped = strings.TrimSuffix(stripped, "-input") // os.WriteFile(stripped+"-want"+ext, stdout.Bytes(), 0644) + // if tt.wantStderrFile != "" { + // os.WriteFile(stripped+"-want-stderr"+ext, stderr.Bytes(), 0644) + // } - want, err := os.ReadFile(tt.want) + wantStdout, err := os.ReadFile(tt.wantStdoutFile) require.NoError(t, err) // Normalize CRLF to LF to make the tests OS-agnostic. - want = []byte(strings.ReplaceAll(string(want), "\r\n", "\n")) + wantStdout = []byte(strings.ReplaceAll(string(wantStdout), "\r\n", "\n")) - assert.Equal(t, string(want), stdout.String()) + assert.Equal(t, string(wantStdout), stdout.String()) + + if tt.wantStderrFile != "" { + wantStderr, err := os.ReadFile(tt.wantStderrFile) + require.NoError(t, err) + + // Normalize CRLF to LF to make the tests OS-agnostic. + wantStderr = []byte(strings.ReplaceAll(string(wantStderr), "\r\n", "\n")) + + assert.Equal(t, string(wantStderr), stderr.String()) + } else { + require.Empty(t, stderr, "expected no stderr output") + } }) } } diff --git a/pkg/cmd/agent-task/shared/testdata/log-3-input.txt b/pkg/cmd/agent-task/shared/testdata/log-3-input.txt new file mode 100644 index 000000000..5fa8e0a63 --- /dev/null +++ b/pkg/cmd/agent-task/shared/testdata/log-3-input.txt @@ -0,0 +1,27 @@ +data: {"id": "bad1", "object": "chat.completion.chunk", "choices": [ { "delta": { "tool_calls": [ { "function": { "name": "view", "arguments": "{bad json" } } ] } } ] } + +data: {"id":"v1","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line\nINSIDE A VIEW CALL","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md\"}"},"id":"tc1","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"v1b","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md\"}"},"id":"tc1b","index":0}]}],"finish_reason":"tool_calls","index":0}]} + +data: {"id":"v1","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line\nINSIDE A VIEW CALL","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md"},"id":"tc1","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"t1","object":"chat.completion.chunk","choices":[{"delta":{"content":"THINK","tool_calls":[{"function":{"name":"think","arguments":"{\"thought\":123"},"id":"tc2","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"t2","object":"chat.completion.chunk","choices":[{"delta":{"content":"A valid thought to render.","reasoning_text":"Interim reasoning that should show as raw markdown.","tool_calls":[{"function":{"name":"think","arguments":"{\"thought\":\"A valid thought to render.\"}"},"id":"tc3","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"rp1","object":"chat.completion.chunk","choices":[{"delta":{"content":"RP","tool_calls":[{"function":{"name":"report_progress","arguments":"{\"commitMessage\": 5"},"id":"tc4","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"rp2","object":"chat.completion.chunk","choices":[{"delta":{"content":"not-json","tool_calls":[{"function":{"name":"report_progress","arguments":"{\"commitMessage\":\"Valid commit msg\"}"},"id":"tc5","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"c1","object":"chat.completion.chunk","choices":[{"delta":{"content":"CREATE","tool_calls":[{"function":{"name":"create","arguments":"{\"path\":\"/abs/path/file.txt\""},"id":"tc6","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"c2","object":"chat.completion.chunk","choices":[{"delta":{"content":"CREATE2","tool_calls":[{"function":{"name":"create","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/new.txt\",\"file_text\":\"hello world\"}"},"id":"tc7","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"sr1","object":"chat.completion.chunk","choices":[{"delta":{"content":"SR","tool_calls":[{"function":{"name":"str_replace","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/file.diff"},"id":"tc8","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"sr2","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line","tool_calls":[{"function":{"name":"str_replace","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/file.diff\"}"},"id":"tc9","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"u1","object":"chat.completion.chunk","choices":[{"delta":{"content":"{\"foo\":1}","tool_calls":[{"function":{"name":"mystery_tool","arguments":"{\"bar\":2}"},"id":"tc10","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"end","object":"chat.completion.chunk","choices":[{"delta":{"content":"","tool_calls":[],"role":"assistant"},"finish_reason":"stop","index":0}]} diff --git a/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-input.txt b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-input.txt new file mode 100644 index 000000000..5fa8e0a63 --- /dev/null +++ b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-input.txt @@ -0,0 +1,27 @@ +data: {"id": "bad1", "object": "chat.completion.chunk", "choices": [ { "delta": { "tool_calls": [ { "function": { "name": "view", "arguments": "{bad json" } } ] } } ] } + +data: {"id":"v1","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line\nINSIDE A VIEW CALL","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md\"}"},"id":"tc1","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"v1b","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md\"}"},"id":"tc1b","index":0}]}],"finish_reason":"tool_calls","index":0}]} + +data: {"id":"v1","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line\nINSIDE A VIEW CALL","tool_calls":[{"function":{"name":"view","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/README.md"},"id":"tc1","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"t1","object":"chat.completion.chunk","choices":[{"delta":{"content":"THINK","tool_calls":[{"function":{"name":"think","arguments":"{\"thought\":123"},"id":"tc2","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"t2","object":"chat.completion.chunk","choices":[{"delta":{"content":"A valid thought to render.","reasoning_text":"Interim reasoning that should show as raw markdown.","tool_calls":[{"function":{"name":"think","arguments":"{\"thought\":\"A valid thought to render.\"}"},"id":"tc3","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"rp1","object":"chat.completion.chunk","choices":[{"delta":{"content":"RP","tool_calls":[{"function":{"name":"report_progress","arguments":"{\"commitMessage\": 5"},"id":"tc4","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"rp2","object":"chat.completion.chunk","choices":[{"delta":{"content":"not-json","tool_calls":[{"function":{"name":"report_progress","arguments":"{\"commitMessage\":\"Valid commit msg\"}"},"id":"tc5","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"c1","object":"chat.completion.chunk","choices":[{"delta":{"content":"CREATE","tool_calls":[{"function":{"name":"create","arguments":"{\"path\":\"/abs/path/file.txt\""},"id":"tc6","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"c2","object":"chat.completion.chunk","choices":[{"delta":{"content":"CREATE2","tool_calls":[{"function":{"name":"create","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/new.txt\",\"file_text\":\"hello world\"}"},"id":"tc7","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"sr1","object":"chat.completion.chunk","choices":[{"delta":{"content":"SR","tool_calls":[{"function":{"name":"str_replace","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/file.diff"},"id":"tc8","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"sr2","object":"chat.completion.chunk","choices":[{"delta":{"content":"@@ -1,2 +1,2 @@\n-old line\n+new line\nunchanged line","tool_calls":[{"function":{"name":"str_replace","arguments":"{\"path\":\"/home/runner/work/repo/owner/repo/file.diff\"}"},"id":"tc9","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"u1","object":"chat.completion.chunk","choices":[{"delta":{"content":"{\"foo\":1}","tool_calls":[{"function":{"name":"mystery_tool","arguments":"{\"bar\":2}"},"id":"tc10","index":0}]},"finish_reason":"tool_calls","index":0}]} + +data: {"id":"end","object":"chat.completion.chunk","choices":[{"delta":{"content":"","tool_calls":[],"role":"assistant"},"finish_reason":"stop","index":0}]} diff --git a/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want-stderr.txt b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want-stderr.txt new file mode 100644 index 000000000..199ab66f3 --- /dev/null +++ b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want-stderr.txt @@ -0,0 +1,10 @@ + +failed to parse 'view' tool call arguments: unexpected end of JSON input + +failed to parse 'think' tool call arguments: unexpected end of JSON input + +failed to parse 'report_progress' tool call arguments: unexpected end of JSON input + +failed to parse 'create' tool call arguments: unexpected end of JSON input + +failed to parse 'str_replace' tool call arguments: unexpected end of JSON input diff --git a/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want.txt b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want.txt new file mode 100644 index 000000000..52a36d427 --- /dev/null +++ b/pkg/cmd/agent-task/shared/testdata/log-3-synthetic-failures-want.txt @@ -0,0 +1,39 @@ +View repo/README.md + +old line + new line + unchanged line + INSIDE A VIEW CALL + + +Interim reasoning that should show as raw markdown. + +Thought + +A valid thought to render. + +Progress update: Valid commit msg +Create: repo/new.txt +hello world + +Edit: repo/file.diff +@@ -1,2 +1,2 @@ + -old line + +new line + unchanged line + +Call to mystery_tool + +Output: + +{ + "foo": 1 + } + + +Input: + +{ + "bar": 2 + } + diff --git a/pkg/cmd/agent-task/shared/testdata/log-3-want.txt b/pkg/cmd/agent-task/shared/testdata/log-3-want.txt new file mode 100644 index 000000000..52a36d427 --- /dev/null +++ b/pkg/cmd/agent-task/shared/testdata/log-3-want.txt @@ -0,0 +1,39 @@ +View repo/README.md + +old line + new line + unchanged line + INSIDE A VIEW CALL + + +Interim reasoning that should show as raw markdown. + +Thought + +A valid thought to render. + +Progress update: Valid commit msg +Create: repo/new.txt +hello world + +Edit: repo/file.diff +@@ -1,2 +1,2 @@ + -old line + +new line + unchanged line + +Call to mystery_tool + +Output: + +{ + "foo": 1 + } + + +Input: + +{ + "bar": 2 + } + From c0a2648436c8f232f0b550130bc1dff53682cc4a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:34:57 -0600 Subject: [PATCH 6/7] Use filepath.Ext to detect file extension in markdown renderer Replaces manual string splitting with filepath.Ext for determining the file extension in renderFileContentAsMarkdown. This improves accuracy, especially for files with multiple dots in their names. --- pkg/cmd/agent-task/shared/log.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/agent-task/shared/log.go b/pkg/cmd/agent-task/shared/log.go index a1b1740e0..28b98eea7 100644 --- a/pkg/cmd/agent-task/shared/log.go +++ b/pkg/cmd/agent-task/shared/log.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "path/filepath" "slices" "strings" @@ -366,10 +367,9 @@ func stripDiffFormat(diff string) string { // renderFileContentAsMarkdown renders the given content as markdown // based on the file extension of the path. func renderFileContentAsMarkdown(path, content string, w io.Writer, io *iostreams.IOStreams) error { - parts := strings.Split(path, ".") - lang := parts[len(parts)-1] + lang := filepath.Ext(filepath.ToSlash(path)) - if lang == "md" { + if lang == ".md" { return renderRawMarkdown(content, w, io) } From b3401ffd740d67dedd83933ae0d72b5278004f10 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:46:43 -0600 Subject: [PATCH 7/7] Refactor stripDiffFormat guard clause logic Rearranged the guard clause in stripDiffFormat to return early if no hunk header is found, improving code clarity and reducing nesting. --- pkg/cmd/agent-task/shared/log.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/agent-task/shared/log.go b/pkg/cmd/agent-task/shared/log.go index 28b98eea7..4959cecbd 100644 --- a/pkg/cmd/agent-task/shared/log.go +++ b/pkg/cmd/agent-task/shared/log.go @@ -342,14 +342,15 @@ func stripDiffFormat(diff string) string { } } - // If we found the hunk header end, we strip everything before it. - if hunkEndIndex != -1 { - lines = lines[hunkEndIndex+1:] - } else { - // This isn't a diff, so we defensively just return the original string. + // Guard clause: if we didn't find a hunk header, this isn't a diff, so + // we defensively just return the original string. + if hunkEndIndex == -1 { return diff } + // We found the hunk header end; strip everything before it. + lines = lines[hunkEndIndex+1:] + // Now we strip the leading + and - from lines, if they exist. // Note: most of the time, but not all the time, we get a diff without // these prefixes.