Skip to content

Commit 6eafe96

Browse files
babakksbabakks
authored
Merge pull request cli#11752 from cli/kw/run-view-pr-feedback
`gh agent-task view`: Follow-up to cli#11743
2 parents c55003b + b3401ff commit 6eafe96

10 files changed

Lines changed: 288 additions & 107 deletions

‎pkg/cmd/agent-task/shared/log.go‎

Lines changed: 88 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"fmt"
77
"io"
8+
"path/filepath"
89
"slices"
910
"strings"
1011

@@ -133,15 +134,16 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I
133134
case "view":
134135
args := viewToolArgs{}
135136
if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil {
136-
return false, fmt.Errorf("failed to parse 'view' tool call arguments: %w", err)
137+
fmt.Fprintf(io.ErrOut, "\nfailed to parse 'view' tool call arguments: %v\n", err)
138+
continue
137139
}
138-
fmt.Fprintf(w, "View %s\n", cs.Bold(relativeFilePath(args.Path)))
140+
renderToolCallTitle(w, cs, fmt.Sprintf("View %s", cs.Bold(relativeFilePath(args.Path))), "")
139141

140142
content := stripDiffFormat(choice.Delta.Content)
141143

142-
// TODO: Strip the diff formatting from this, but for now render as it is.
143144
if err := renderFileContentAsMarkdown(args.Path, content, w, io); err != nil {
144-
return false, fmt.Errorf("failed to render viewed file content: %w", err)
145+
fmt.Fprintf(io.ErrOut, "\nfailed to render viewed file content: %v\n\n", err)
146+
fmt.Fprintln(io.ErrOut, content) // raw fallback
145147
}
146148
case "bash":
147149
if v := unmarshal[bashToolArgs](args); v != nil {
@@ -153,10 +155,11 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I
153155

154156
contentWithCommand := choice.Delta.Content
155157
if v.Command != "" {
156-
contentWithCommand = fmt.Sprintf("%s\n%s", v.Command, choice.Delta.Content)
158+
contentWithCommand = fmt.Sprintf("$ %s\n%s", v.Command, choice.Delta.Content)
157159
}
158160
if err := renderFileContentAsMarkdown("commands.sh", contentWithCommand, w, io); err != nil {
159-
return false, fmt.Errorf("failed to render bash command output: %w", err)
161+
fmt.Fprintf(io.ErrOut, "\nfailed to render bash command output: %v\n\n", err)
162+
fmt.Fprintln(io.ErrOut, contentWithCommand)
160163
}
161164
}
162165
// TODO: consider including more details for these bash-related tool calls.
@@ -193,54 +196,60 @@ func renderLogEntry(entry chatCompletionChunkEntry, w io.Writer, io *iostreams.I
193196
case "think":
194197
args := thinkToolArgs{}
195198
if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil {
196-
return false, fmt.Errorf("failed to parse 'think' tool call arguments: %w", err)
199+
fmt.Fprintf(io.ErrOut, "\nfailed to parse 'think' tool call arguments: %v\n", err)
200+
continue
197201
}
198202

199203
// NOTE: omit the delta.content since it's the same as thought
200204
renderToolCallTitle(w, cs, "Thought", "")
201205
if err := renderRawMarkdown(args.Thought, w, io); err != nil {
202-
return false, fmt.Errorf("failed to render thought: %w", err)
206+
fmt.Fprintf(io.ErrOut, "\nfailed to render thought: %v\n", err)
203207
}
204208
case "report_progress":
205209
args := reportProgressToolArgs{}
206210
if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil {
207-
return false, fmt.Errorf("failed to parse 'report_progress' tool call arguments: %w", err)
211+
fmt.Fprintf(io.ErrOut, "\nfailed to parse 'report_progress' tool call arguments: %v\n", err)
212+
continue
208213
}
209214

210215
renderToolCallTitle(w, cs, "Progress update", cs.Bold(args.CommitMessage))
211216
if args.PrDescription != "" {
212217
if err := renderRawMarkdown(args.PrDescription, w, io); err != nil {
213-
return false, fmt.Errorf("failed to render PR description: %w", err)
218+
fmt.Fprintf(io.ErrOut, "\nfailed to render PR description: %v\n", err)
214219
}
215220
}
216221

217222
// TODO: KW I wasn't able to get this case to populate ever.
218223
if choice.Delta.Content != "" {
219224
// Try to treat this as JSON
220225
if err := renderContentAsJSONMarkdown("", choice.Delta.Content, w, io); err != nil {
221-
return false, fmt.Errorf("failed to render progress update content: %w", err)
226+
fmt.Fprintf(io.ErrOut, "\nfailed to render progress update content: %v\n", err)
222227
}
223228
}
224229

225230
case "create":
226231
args := createToolArgs{}
227232
if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil {
228-
return false, fmt.Errorf("failed to parse 'create' tool call arguments: %w", err)
233+
fmt.Fprintf(io.ErrOut, "\nfailed to parse 'create' tool call arguments: %v\n", err)
234+
continue
229235
}
230236
renderToolCallTitle(w, cs, "Create", cs.Bold(relativeFilePath(args.Path)))
231237

232238
if err := renderFileContentAsMarkdown(args.Path, args.FileText, w, io); err != nil {
233-
return false, fmt.Errorf("failed to render created file content: %w", err)
239+
fmt.Fprintf(io.ErrOut, "\nfailed to render created file content: %v\n\n", err)
240+
fmt.Fprintln(io.ErrOut, args.FileText)
234241
}
235242
case "str_replace":
236243
args := strReplaceToolArgs{}
237244
if err := json.Unmarshal([]byte(tc.Function.Arguments), &args); err != nil {
238-
return false, fmt.Errorf("failed to parse 'str_replace' tool call arguments: %w", err)
245+
fmt.Fprintf(io.ErrOut, "\nfailed to parse 'str_replace' tool call arguments: %v\n", err)
246+
continue
239247
}
240248

241249
renderToolCallTitle(w, cs, "Edit", cs.Bold(relativeFilePath(args.Path)))
242250
if err := renderFileContentAsMarkdown("output.diff", choice.Delta.Content, w, io); err != nil {
243-
return false, fmt.Errorf("failed to render str_replace diff: %w", err)
251+
fmt.Fprintf(io.ErrOut, "\nfailed to render str_replace diff: %v\n\n", err)
252+
fmt.Fprintln(io.ErrOut, choice.Delta.Content)
244253
}
245254
default:
246255
// Unknown tool call. For example for "codeql_checker":
@@ -333,14 +342,15 @@ func stripDiffFormat(diff string) string {
333342
}
334343
}
335344

336-
// If we found the hunk header end, we strip everything before it.
337-
if hunkEndIndex != -1 {
338-
lines = lines[hunkEndIndex+1:]
339-
} else {
340-
// This isn't a diff, so we defensively just return the original string.
345+
// Guard clause: if we didn't find a hunk header, this isn't a diff, so
346+
// we defensively just return the original string.
347+
if hunkEndIndex == -1 {
341348
return diff
342349
}
343350

351+
// We found the hunk header end; strip everything before it.
352+
lines = lines[hunkEndIndex+1:]
353+
344354
// Now we strip the leading + and - from lines, if they exist.
345355
// Note: most of the time, but not all the time, we get a diff without
346356
// these prefixes.
@@ -358,10 +368,9 @@ func stripDiffFormat(diff string) string {
358368
// renderFileContentAsMarkdown renders the given content as markdown
359369
// based on the file extension of the path.
360370
func renderFileContentAsMarkdown(path, content string, w io.Writer, io *iostreams.IOStreams) error {
361-
parts := strings.Split(path, ".")
362-
lang := parts[len(parts)-1]
371+
lang := filepath.Ext(filepath.ToSlash(path))
363372

364-
if lang == "md" {
373+
if lang == ".md" {
365374
return renderRawMarkdown(content, w, io)
366375
}
367376

@@ -422,62 +431,63 @@ func renderToolCallTitle(w io.Writer, cs *iostreams.ColorScheme, toolName, title
422431
}
423432
}
424433

425-
func renderGenericToolCall(w io.Writer, cs *iostreams.ColorScheme, name string) {
426-
genericToolCallNamesToTitles := map[string]string{
427-
// Custom tools, the GitHub UI doesn't currently have these.
428-
"codeql_checker": "Run CodeQL analysis",
429-
430-
// Playwright tools.
431-
"playwright-browser_navigate": "Navigate Playwright web browser to a URL",
432-
"playwright-browser_navigate_back": "Navigate back in Playwright web browser",
433-
"playwright-browser_navigate_forward": "Navigate forward in Playwright web browser",
434-
"playwright-browser_click": "Click element in Playwright web browser",
435-
"playwright-browser_take_screenshot": "Take screenshot of Playwright web browser",
436-
"playwright-browser_type": "Type in Playwright web browser",
437-
"playwright-browser_wait_for": "Wait for text to appear/disappear in Playwright web browser",
438-
"playwright-browser_evaluate": "Run JavaScript in Playwright web browser",
439-
"playwright-browser_snapshot": "Take snapshot of page in Playwright web browser",
440-
"playwright-browser_resize": "Resize Playwright web browser window",
441-
"playwright-browser_close": "Close Playwright web browser",
442-
"playwright-browser_press_key": "Press key in Playwright web browser",
443-
"playwright-browser_select_option": "Select option in Playwright web browser",
444-
"playwright-browser_handle_dialog": "Interact with dialog in Playwright web browser",
445-
"playwright-browser_console_messages": "Get console messages from Playwright web browser",
446-
"playwright-browser_drag": "Drag mouse between elements in Playwright web browser",
447-
"playwright-browser_file_upload": "Upload file in Playwright web browser",
448-
"playwright-browser_hover": "Hover mouse over element in Playwright web browser",
449-
"playwright-browser_network_requests": "Get network requests from Playwright web browser",
450-
451-
// GitHub MCP server common tools
452-
"github-mcp-server-get_file_contents": "Get file contents from GitHub",
453-
"github-mcp-server-get_pull_request": "Get pull request from GitHub",
454-
"github-mcp-server-get_issue": "Get issue from GitHub",
455-
"github-mcp-server-get_pull_request_files": "Get pull request changed files from GitHub",
456-
"github-mcp-server-list_pull_requests": "List pull requests on GitHub",
457-
"github-mcp-server-list_branches": "List branches on GitHub",
458-
"github-mcp-server-get_pull_request_diff": "Get pull request diff from GitHub",
459-
"github-mcp-server-get_pull_request_comments": "Get pull request comments from GitHub",
460-
"github-mcp-server-get_commit": "Get commit from GitHub",
461-
"github-mcp-server-search_repositories": "Search repositories on GitHub",
462-
"github-mcp-server-search_code": "Search code on GitHub",
463-
"github-mcp-server-get_issue_comments": "Get issue comments from GitHub",
464-
"github-mcp-server-list_issues": "List issues on GitHub",
465-
"github-mcp-server-search_pull_requests": "Search pull requests on GitHub",
466-
"github-mcp-server-list_commits": "List commits on GitHub",
467-
"github-mcp-server-get_pull_request_status": "Get pull request status from GitHub",
468-
"github-mcp-server-search_issues": "Search issues on GitHub",
469-
"github-mcp-server-get_pull_request_reviews": "Get pull request reviews from GitHub",
470-
"github-mcp-server-download_workflow_run_artifact": "Download GitHub Actions workflow run artifact",
471-
"github-mcp-server-get_job_logs": "Get GitHub Actions job logs",
472-
"github-mcp-server-get_workflow_run": "Get GitHub Actions workflow run",
473-
"github-mcp-server-get_workflow_run_logs": "Get GitHub Actions workflow run logs",
474-
"github-mcp-server-get_workflow_run_usage": "Get GitHub Actions workflow usage",
475-
"github-mcp-server-list_workflow_jobs": "List GitHub Actions workflow jobs",
476-
"github-mcp-server-list_workflow_run_artifacts": "List GitHub Actions workflow run artifacts",
477-
"github-mcp-server-list_workflow_runs": "List GitHub Actions workflow runs",
478-
"github-mcp-server-list_workflows": "List GitHub Actions workflows",
479-
}
434+
// genericToolCallNamesToTitles maps known generic tool call identifiers to human-friendly titles.
435+
var genericToolCallNamesToTitles = map[string]string{
436+
// Custom tools, the GitHub UI doesn't currently have these.
437+
"codeql_checker": "Run CodeQL analysis",
438+
439+
// Playwright tools.
440+
"playwright-browser_navigate": "Navigate Playwright web browser to a URL",
441+
"playwright-browser_navigate_back": "Navigate back in Playwright web browser",
442+
"playwright-browser_navigate_forward": "Navigate forward in Playwright web browser",
443+
"playwright-browser_click": "Click element in Playwright web browser",
444+
"playwright-browser_take_screenshot": "Take screenshot of Playwright web browser",
445+
"playwright-browser_type": "Type in Playwright web browser",
446+
"playwright-browser_wait_for": "Wait for text to appear/disappear in Playwright web browser",
447+
"playwright-browser_evaluate": "Run JavaScript in Playwright web browser",
448+
"playwright-browser_snapshot": "Take snapshot of page in Playwright web browser",
449+
"playwright-browser_resize": "Resize Playwright web browser window",
450+
"playwright-browser_close": "Close Playwright web browser",
451+
"playwright-browser_press_key": "Press key in Playwright web browser",
452+
"playwright-browser_select_option": "Select option in Playwright web browser",
453+
"playwright-browser_handle_dialog": "Interact with dialog in Playwright web browser",
454+
"playwright-browser_console_messages": "Get console messages from Playwright web browser",
455+
"playwright-browser_drag": "Drag mouse between elements in Playwright web browser",
456+
"playwright-browser_file_upload": "Upload file in Playwright web browser",
457+
"playwright-browser_hover": "Hover mouse over element in Playwright web browser",
458+
"playwright-browser_network_requests": "Get network requests from Playwright web browser",
459+
460+
// GitHub MCP server common tools
461+
"github-mcp-server-get_file_contents": "Get file contents from GitHub",
462+
"github-mcp-server-get_pull_request": "Get pull request from GitHub",
463+
"github-mcp-server-get_issue": "Get issue from GitHub",
464+
"github-mcp-server-get_pull_request_files": "Get pull request changed files from GitHub",
465+
"github-mcp-server-list_pull_requests": "List pull requests on GitHub",
466+
"github-mcp-server-list_branches": "List branches on GitHub",
467+
"github-mcp-server-get_pull_request_diff": "Get pull request diff from GitHub",
468+
"github-mcp-server-get_pull_request_comments": "Get pull request comments from GitHub",
469+
"github-mcp-server-get_commit": "Get commit from GitHub",
470+
"github-mcp-server-search_repositories": "Search repositories on GitHub",
471+
"github-mcp-server-search_code": "Search code on GitHub",
472+
"github-mcp-server-get_issue_comments": "Get issue comments from GitHub",
473+
"github-mcp-server-list_issues": "List issues on GitHub",
474+
"github-mcp-server-search_pull_requests": "Search pull requests on GitHub",
475+
"github-mcp-server-list_commits": "List commits on GitHub",
476+
"github-mcp-server-get_pull_request_status": "Get pull request status from GitHub",
477+
"github-mcp-server-search_issues": "Search issues on GitHub",
478+
"github-mcp-server-get_pull_request_reviews": "Get pull request reviews from GitHub",
479+
"github-mcp-server-download_workflow_run_artifact": "Download GitHub Actions workflow run artifact",
480+
"github-mcp-server-get_job_logs": "Get GitHub Actions job logs",
481+
"github-mcp-server-get_workflow_run": "Get GitHub Actions workflow run",
482+
"github-mcp-server-get_workflow_run_logs": "Get GitHub Actions workflow run logs",
483+
"github-mcp-server-get_workflow_run_usage": "Get GitHub Actions workflow usage",
484+
"github-mcp-server-list_workflow_jobs": "List GitHub Actions workflow jobs",
485+
"github-mcp-server-list_workflow_run_artifacts": "List GitHub Actions workflow run artifacts",
486+
"github-mcp-server-list_workflow_runs": "List GitHub Actions workflow runs",
487+
"github-mcp-server-list_workflows": "List GitHub Actions workflows",
488+
}
480489

490+
func renderGenericToolCall(w io.Writer, cs *iostreams.ColorScheme, name string) {
481491
toolName, ok := genericToolCallNamesToTitles[name]
482492
if !ok {
483493
toolName = fmt.Sprintf("Call to %s", name)

‎pkg/cmd/agent-task/shared/log_test.go‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,26 @@ import (
1313

1414
func TestFollow(t *testing.T) {
1515
tests := []struct {
16-
name string
17-
log string
18-
want string
16+
name string
17+
log string
18+
wantStdoutFile string
19+
wantStderrFile string
1920
}{
2021
{
21-
name: "sample log 1",
22-
log: "testdata/log-1-input.txt",
23-
want: "testdata/log-1-want.txt",
22+
name: "sample log 1",
23+
log: "testdata/log-1-input.txt",
24+
wantStdoutFile: "testdata/log-1-want.txt",
2425
},
2526
{
26-
name: "sample log 2",
27-
log: "testdata/log-2-input.txt",
28-
want: "testdata/log-2-want.txt",
27+
name: "sample log 2",
28+
log: "testdata/log-2-input.txt",
29+
wantStdoutFile: "testdata/log-2-want.txt",
30+
},
31+
{
32+
name: "sample log 3 (tolerant parse failures)",
33+
log: "testdata/log-3-synthetic-failures-input.txt",
34+
wantStdoutFile: "testdata/log-3-synthetic-failures-want.txt",
35+
wantStderrFile: "testdata/log-3-synthetic-failures-want-stderr.txt",
2936
},
3037
}
3138

@@ -34,7 +41,7 @@ func TestFollow(t *testing.T) {
3441
raw, err := os.ReadFile(tt.log)
3542
require.NoError(t, err)
3643

37-
// Delete all the `/r` to make the tests OS-agnostic.
44+
// Normalize CRLF to LF to make the tests OS-agnostic.
3845
raw = []byte(strings.ReplaceAll(string(raw), "\r\n", "\n"))
3946

4047
lines := slices.DeleteFunc(strings.Split(string(raw), "\n"), func(line string) bool {
@@ -50,23 +57,39 @@ func TestFollow(t *testing.T) {
5057
return []byte(strings.Join(lines[0:hits], "\n\n")), nil
5158
}
5259

53-
ios, _, stdout, _ := iostreams.Test()
60+
ios, _, stdout, stderr := iostreams.Test()
5461

5562
err = NewLogRenderer().Follow(fetcher, stdout, ios)
5663
require.NoError(t, err)
5764

5865
// Handy note for updating the testdata files when they change:
5966
// ext := filepath.Ext(tt.log)
6067
// stripped := strings.TrimSuffix(tt.log, ext)
61-
// os.WriteFile(stripped+".want"+ext, stdout.Bytes(), 0644)
68+
// stripped = strings.TrimSuffix(stripped, "-input")
69+
// os.WriteFile(stripped+"-want"+ext, stdout.Bytes(), 0644)
70+
// if tt.wantStderrFile != "" {
71+
// os.WriteFile(stripped+"-want-stderr"+ext, stderr.Bytes(), 0644)
72+
// }
6273

63-
want, err := os.ReadFile(tt.want)
74+
wantStdout, err := os.ReadFile(tt.wantStdoutFile)
6475
require.NoError(t, err)
6576

66-
// Delete all the `/r` to make the tests OS-agnostic.
67-
want = []byte(strings.ReplaceAll(string(want), "\r\n", "\n"))
77+
// Normalize CRLF to LF to make the tests OS-agnostic.
78+
wantStdout = []byte(strings.ReplaceAll(string(wantStdout), "\r\n", "\n"))
79+
80+
assert.Equal(t, string(wantStdout), stdout.String())
6881

69-
assert.Equal(t, string(want), stdout.String())
82+
if tt.wantStderrFile != "" {
83+
wantStderr, err := os.ReadFile(tt.wantStderrFile)
84+
require.NoError(t, err)
85+
86+
// Normalize CRLF to LF to make the tests OS-agnostic.
87+
wantStderr = []byte(strings.ReplaceAll(string(wantStderr), "\r\n", "\n"))
88+
89+
assert.Equal(t, string(wantStderr), stderr.String())
90+
} else {
91+
require.Empty(t, stderr, "expected no stderr output")
92+
}
7093
})
7194
}
7295
}

0 commit comments

Comments
 (0)