From ce27e1546a57ede44564957256ee0e2076207e3a Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 4 Nov 2026 00:06:42 +0530 Subject: [PATCH 1/2] fix(cli): compact tool output and report shell failures accurately --- cmd/chat_compact_results_test.go | 51 +++++++++++++++++ cmd/chat_model.go | 2 - cmd/chat_subcommand_shell_test.go | 11 ++++ cmd/chat_update_stream.go | 2 +- cmd/chat_viewport_render.go | 45 +++++++-------- internal/prompts/templates/communication.md | 3 +- internal/prompts/templates/tools.md | 4 +- internal/tool/bash.go | 35 ++++++------ internal/tool/bash_result_test.go | 57 +++++++++++++++++++ .../tool/execution_safety_regression_test.go | 3 +- internal/tool/retry.go | 6 +- internal/tool/task_tools.go | 8 ++- 12 files changed, 176 insertions(+), 51 deletions(-) create mode 100644 cmd/chat_compact_results_test.go create mode 100644 internal/tool/bash_result_test.go diff --git a/cmd/chat_compact_results_test.go b/cmd/chat_compact_results_test.go new file mode 100644 index 000000000..a33430d91 --- /dev/null +++ b/cmd/chat_compact_results_test.go @@ -0,0 +1,51 @@ +package cmd + +import ( + "strings" + "testing" + + "charm.land/bubbles/v2/viewport" +) + +func TestCompactToolResultKeepsFullOutputExpandable(t *testing.T) { + raw := "first\nsecond\nhidden-a\nhidden-b\nlast" + got := stripANSI(collapseToolResult(raw, 0, nil, false)) + if !strings.Contains(got, "first\nsecond") || strings.Contains(got, "hidden-a") || !strings.Contains(got, "Enter to expand") { + t.Fatalf("verbose or unusable preview: %q", got) + } + if expanded := collapseToolResult(raw, 0, map[int]bool{0: true}, false); expanded != raw { + t.Fatalf("expanded output lost content: %q", expanded) + } + failed := stripANSI(collapseToolResult(raw, 0, nil, true)) + if !strings.Contains(failed, "last") { + t.Fatalf("failure preview hid the diagnostic tail: %q", failed) + } +} + +func TestCollapsedToolResultHitTestingUsesVisibleRows(t *testing.T) { + m := &chatModel{width: 80, viewport: viewport.New(viewport.WithWidth(80), viewport.WithHeight(1))} + m.messages = []displayMsg{ + {role: "tool_result", content: strings.Repeat("first result\n", 50)}, + {role: "tool_result", content: strings.Repeat("second result\n", 10)}, + } + prefix := renderDisplayMessage(m.messages[0], 0, m.messages, 80, nil) + m.viewport.SetContent(renderMessagesRange(m.messages, 0, len(m.messages), 80, nil)) + m.viewport.SetYOffset(strings.Count(prefix, "\n")) + if got := m.toolResultIndexAtViewportCenter(80); got != 1 { + t.Fatalf("Enter targets message %d, want visible second result", got) + } +} + +func TestCodeCopyAfterCollapsedToolUsesVisibleRows(t *testing.T) { + m := &chatModel{width: 80, viewport: viewport.New(viewport.WithWidth(80), viewport.WithHeight(1))} + m.messages = []displayMsg{ + {role: "tool_result", content: strings.Repeat("long result\n", 50)}, + {role: "assistant", content: "Example:\n\n```go\nfmt.Println(\"hello\")\n```"}, + } + prefix := renderDisplayMessage(m.messages[0], 0, m.messages, 80, nil) + m.viewport.SetContent(renderMessagesRange(m.messages, 0, len(m.messages), 80, nil)) + m.viewport.SetYOffset(strings.Count(prefix, "\n") + 2) + if code, ok := m.codeBlockAtViewportCenter(); !ok || !strings.Contains(code, "fmt.Println") { + t.Fatalf("code copy missed visible block: %q, %v", code, ok) + } +} diff --git a/cmd/chat_model.go b/cmd/chat_model.go index ac0fb6376..834cd2bd1 100644 --- a/cmd/chat_model.go +++ b/cmd/chat_model.go @@ -366,8 +366,6 @@ type chatModel struct { vpLastMsgLen int // Cached expanded map for viewport position math (avoids repeated allocations). - cachedExpandedMap map[int]bool - cachedExpandedLen int // Streaming-partial render cache: rendered output of the completed // markdown blocks of m.partial (see renderStreamTail). diff --git a/cmd/chat_subcommand_shell_test.go b/cmd/chat_subcommand_shell_test.go index d4de22e95..e9c4cd898 100644 --- a/cmd/chat_subcommand_shell_test.go +++ b/cmd/chat_subcommand_shell_test.go @@ -30,6 +30,17 @@ func TestRunSubcommand_UsesBashToolForSafeCommand(t *testing.T) { } } +func TestRunSubcommand_ReportsFailedPipelineWithDiagnostics(t *testing.T) { + m := newTestChatModel() + m.session.PermSvc().SetAutonomy(safety.AutonomyYOLO) + result, _ := (&runSubcommand{}).Handle(m, []string{"printf 'build diagnostic\\n'; false | tail -5"}, "/run printf 'build diagnostic\\n'; false | tail -5") + cm := requireChatModel(t, result) + last := cm.messages[len(cm.messages)-1] + if last.role != "error" || !strings.Contains(last.content, "build diagnostic") || !strings.Contains(last.content, "exit code:") { + t.Fatalf("failed check appeared successful or lost its diagnostic: role=%s output=%q", last.role, last.content) + } +} + func TestRunSubcommand_RespectsSpecStageGate(t *testing.T) { m := newTestChatModel() m.session.PermSvc().SetSpecStage(safety.SpecStageSpecify) diff --git a/cmd/chat_update_stream.go b/cmd/chat_update_stream.go index 3134214c6..6f355d684 100644 --- a/cmd/chat_update_stream.go +++ b/cmd/chat_update_stream.go @@ -113,7 +113,7 @@ func (m chatModel) handleToolResult(msg toolResultMsg) (tea.Model, tea.Cmd) { } // The preceding tool_use message renders the tool name as this block's // header, so the result contains only the returned content. - m.messages = append(m.messages, displayMsg{role: "tool_result", content: msg.content}) + m.messages = append(m.messages, displayMsg{role: "tool_result", content: msg.content, toolState: msg.state, toolID: msg.id, toolName: msg.name}) m.viewDirty = true // Persist completed tool results incrementally so a crash mid-turn does // not lose them before the final session save. diff --git a/cmd/chat_viewport_render.go b/cmd/chat_viewport_render.go index 1fe1338da..e2db64f74 100644 --- a/cmd/chat_viewport_render.go +++ b/cmd/chat_viewport_render.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/GrayCodeAI/reflect/internal/engine" "github.com/GrayCodeAI/reflect/internal/ui/icons" ) @@ -25,8 +26,8 @@ func (m *chatModel) invalidateViewportCache() { // toolResultCollapseLines are collapsed to toolResultPreviewLines with a // toggle indicator; the user presses Enter in scrollback focus to expand. const ( - toolResultCollapseLines = 15 // collapse rendered output longer than this - toolResultPreviewLines = 8 // preview lines shown when collapsed + toolResultCollapseLines = 3 // collapse rendered output longer than this + toolResultPreviewLines = 2 // preview lines shown when collapsed ) func renderDisplayMessage(msg displayMsg, i int, messages []displayMsg, viewWidth int, expanded map[int]bool) string { @@ -122,7 +123,8 @@ func renderDisplayMessage(msg displayMsg, i int, messages []displayMsg, viewWidt toolWrapped := wrapText(display, viewWidth-6, 0) inner.WriteString(toolDimStyle.Render(" " + strings.ReplaceAll(toolWrapped, "\n", "\n "))) } - b.WriteString(collapseToolResult(inner.String(), i, expanded)) + failed := msg.toolState == engine.ToolStateFailed || msg.toolState == engine.ToolStateTimedOut + b.WriteString(collapseToolResult(inner.String(), i, expanded, failed)) case "thinking": thinkWrapped := wrapText(msg.content, viewWidth-4, 3) b.WriteString(dimStyle.Render(icons.Brain() + " " + thinkWrapped)) @@ -189,18 +191,26 @@ func renderDisplayMessage(msg displayMsg, i int, messages []displayMsg, viewWidt // [+N lines] toggle indicator when the result is not expanded. Short results // (ā‰¤ toolResultCollapseLines) pass through unchanged. The expanded map is keyed // by message index. -func collapseToolResult(rendered string, msgIdx int, expanded map[int]bool) string { +func collapseToolResult(rendered string, msgIdx int, expanded map[int]bool, failed bool) string { if expanded[msgIdx] { return rendered } // Count visible lines. ANSI styling codes don't contain '\n', so a simple // split accurately reflects the rendered line count. lines := strings.Split(rendered, "\n") - if len(lines) <= toolResultCollapseLines { + threshold := toolResultCollapseLines + if failed { + threshold = 2 * toolResultPreviewLines + } + if len(lines) <= threshold { return rendered } preview := strings.Join(lines[:toolResultPreviewLines], "\n") hidden := len(lines) - toolResultPreviewLines + if failed { + preview += "\nā€¦\n" + strings.Join(lines[len(lines)-toolResultPreviewLines:], "\n") + hidden -= toolResultPreviewLines + } toggle := fmt.Sprintf("[+%d lines ā€” Enter to expand]", hidden) return preview + "\n" + toolDimStyle.Render(" "+toggle) } @@ -310,15 +320,14 @@ func trailingNewlines(s string) string { // toolResultIndexAtViewportCenter returns the index of the tool_result message // whose rendered content contains the viewport's center line, or -1 if none. -// It fully renders each tool_result (ignoring collapse state) to compute true -// line heights, then finds which message spans the center Y offset. +// Use the visible collapse state so shortcuts target what is on screen. func (m *chatModel) toolResultIndexAtViewportCenter(viewWidth int) int { if m.viewport.Height() <= 0 { return -1 } centerY := m.viewport.YOffset() + m.viewport.Height()/2 cumulative := 0 - expanded := m.fullExpandedMap(len(m.messages)) + expanded := m.toolResultExpanded for i, msg := range m.messages { if msg.role != "tool_result" { // Still need to count lines for non-tool messages to track offset. @@ -326,7 +335,7 @@ func (m *chatModel) toolResultIndexAtViewportCenter(viewWidth int) int { cumulative += strings.Count(rendered, "\n") continue } - // Render fully (expanded) to get true line count. + // Count the rows currently displayed, including collapsed previews. rendered := renderDisplayMessage(msg, i, m.messages, viewWidth, expanded) lineCount := strings.Count(rendered, "\n") if centerY >= cumulative && centerY < cumulative+lineCount { @@ -337,22 +346,6 @@ func (m *chatModel) toolResultIndexAtViewportCenter(viewWidth int) int { return -1 } -// fullExpandedMap returns a map where every index is expanded ā€” used to compute -// true (uncollapsed) line heights for viewport position math. -// Uses a cached map to avoid repeated allocations. -func (m *chatModel) fullExpandedMap(n int) map[int]bool { - if m.cachedExpandedMap != nil && m.cachedExpandedLen == n { - return m.cachedExpandedMap - } - expanded := make(map[int]bool, n) - for i := 0; i < n; i++ { - expanded[i] = true - } - m.cachedExpandedMap = expanded - m.cachedExpandedLen = n - return expanded -} - // codeBlockAtViewportCenter finds the fenced code block in an assistant message // whose rendered content contains the viewport's center line. Returns the raw // code content (without fences) and true if found. @@ -381,7 +374,7 @@ func (m *chatModel) codeBlockAtViewportCenter() (string, bool) { // Slow path: render messages incrementally, stopping early when we pass center. cumulative := 0 - expanded := m.fullExpandedMap(len(m.messages)) + expanded := m.toolResultExpanded for i, msg := range m.messages { rendered := renderDisplayMessage(msg, i, m.messages, viewWidth, expanded) lineCount := strings.Count(rendered, "\n") diff --git a/internal/prompts/templates/communication.md b/internal/prompts/templates/communication.md index ef68c1eb6..5c6dc1d1a 100644 --- a/internal/prompts/templates/communication.md +++ b/internal/prompts/templates/communication.md @@ -4,5 +4,6 @@ - State what you did, not what you're about to do. - Don't narrate your thought process unless asked. - Don't apologize or hedge. -- Show code/diffs rather than describing changes. +- Summarize the outcome, changed files, and verification in a few sentences. Show full code, diffs, or command output only when requested. +- Keep progress updates brief and report new findings; do not repeat that a check is still running. - If you made a mistake, fix it directly. diff --git a/internal/prompts/templates/tools.md b/internal/prompts/templates/tools.md index 049679e15..1c18ad645 100644 --- a/internal/prompts/templates/tools.md +++ b/internal/prompts/templates/tools.md @@ -22,7 +22,9 @@ When exploring a codebase: When making changes: - Read the file first to understand context - Make minimal, focused edits -- Verify changes compile/run if possible +- Prefer Edit/Write for source changes. Replace complete code blocks rather than deleting matching lines with shell substitutions. +- Inspect the final diff, format modified source, and run the relevant build or tests before claiming a fix is complete. Repair syntax errors immediately. +- If a check fails, use its diagnostics to fix the cause. Do not repeat unchanged checks merely to count passing packages. When using Bash: - Prefer non-destructive commands diff --git a/internal/tool/bash.go b/internal/tool/bash.go index 5803191b0..a26ce94a5 100644 --- a/internal/tool/bash.go +++ b/internal/tool/bash.go @@ -395,27 +395,22 @@ func (BashTool) Execute(ctx context.Context, input json.RawMessage) (string, err if workdir == "" { workdir, _ = os.Getwd() } - out, err := sandbox.Run(ctx, sbCfg, workdir, p.Command) + // Match host Bash semantics even when the sandbox transport uses sh. + out, err := sandbox.Run(ctx, sbCfg, workdir, bashSandboxCommand(p.Command)) result := strings.TrimSpace(out) result = TruncateOutput(result) - if err != nil { - if ctx.Err() == context.DeadlineExceeded { - return result + "\n\n(command timed out)", nil - } - return fmt.Sprintf("%s\n\nexit code: %s", result, err.Error()), nil - } - return result, nil + return bashExecutionResult(ctx, result, err) } if p.RunInBackground { - id, err := startBackgroundBash(ctx, p.Command, "bash", []string{"-c", p.Command}) + id, err := startBackgroundBash(ctx, p.Command, "bash", []string{"-o", "pipefail", "-c", p.Command}) if err != nil { return "", err } return fmt.Sprintf("Started background task %s. Use TaskOutput with task_id=%q to read output, or TaskStop to stop it.", id, id), nil } - cmd := exec.CommandContext(ctx, "bash", "-c", p.Command) // #nosec G204 -- command parsed from tool-configured command string (lint/test command) + cmd := exec.CommandContext(ctx, "bash", "-o", "pipefail", "-c", p.Command) // #nosec G204 -- command parsed from tool-configured command string (lint/test command) // Never pass provider API keys to the child: the guard regexes block // obvious dumps, but any process the agent runs can otherwise read // ANTHROPIC_API_KEY etc. from the inherited environment. @@ -447,11 +442,19 @@ func (BashTool) Execute(ctx context.Context, input json.RawMessage) (string, err result = TruncateOutput(result) result = strings.TrimRight(result, "\n") - if err != nil { - if ctx.Err() == context.DeadlineExceeded { - return result + "\n\n(command timed out)", nil - } - return fmt.Sprintf("%s\n\nexit code: %s", result, err.Error()), nil + return bashExecutionResult(ctx, result, err) +} + +func bashExecutionResult(ctx context.Context, output string, err error) (string, error) { + if err == nil { + return output, nil + } + if ctx.Err() != nil { + return output, ctx.Err() } - return result, nil + return output, fmt.Errorf("exit code: %w", err) +} + +func bashSandboxCommand(command string) string { + return "bash -o pipefail -c '" + strings.ReplaceAll(command, "'", "'\\''") + "'" } diff --git a/internal/tool/bash_result_test.go b/internal/tool/bash_result_test.go new file mode 100644 index 000000000..6091a16cf --- /dev/null +++ b/internal/tool/bash_result_test.go @@ -0,0 +1,57 @@ +package tool + +import ( + "encoding/json" + "os/exec" + "strings" + "testing" +) + +func TestBashPipelineFailurePreservesDiagnostics(t *testing.T) { + input, _ := json.Marshal(map[string]string{"command": "printf 'compiler diagnostic\\n'; false | tail -5"}) + out, err := RetryExecutor(testCtx(), BashTool{}, input, DefaultRetryPolicy()) + if err == nil || !strings.Contains(err.Error(), "exit code:") || !strings.Contains(out, "compiler diagnostic") { + t.Fatalf("pipeline failure lost its status or diagnostic: output=%q error=%v", out, err) + } +} + +func TestSandboxBashQuotingAndPipelineStatus(t *testing.T) { + command := bashSandboxCommand("printf '%s\\n' 'quoted value'; false | cat") + out, err := exec.CommandContext(t.Context(), "bash", "-c", command).CombinedOutput() + if err == nil || !strings.Contains(string(out), "quoted value") { + t.Fatalf("sandbox wrapper changed quoting or hid failure: %q, %v", out, err) + } +} + +func TestBashPipelineSuccess(t *testing.T) { + input := json.RawMessage(`{"command":"printf 'checked\\n' | tail -1"}`) + out, err := (BashTool{}).Execute(testCtx(), input) + if err != nil || out != "checked" { + t.Fatalf("successful pipeline failed: %q, %v", out, err) + } +} + +func TestBashBackgroundPipelineFails(t *testing.T) { + input := json.RawMessage(`{"command":"false | cat","run_in_background":true}`) + out, err := (BashTool{}).Execute(testCtx(), input) + if err != nil { + t.Fatal(err) + } + id := extractTaskID(out) + if id == "" { + t.Fatalf("missing background task: %q", out) + } + task, ok := getBackgroundTask(id) + if !ok { + t.Fatal("task was not registered") + } + select { + case <-task.done: + case <-t.Context().Done(): + t.Fatal("task did not finish") + } + status, _, _, _, err := resolveTaskOutput(t.Context(), id) + if err != nil || status != "failed" { + t.Fatalf("pipeline was not marked failed: status=%s error=%v", status, err) + } +} diff --git a/internal/tool/execution_safety_regression_test.go b/internal/tool/execution_safety_regression_test.go index d454adaa8..40f4a0807 100644 --- a/internal/tool/execution_safety_regression_test.go +++ b/internal/tool/execution_safety_regression_test.go @@ -3,6 +3,7 @@ package tool import ( "context" "encoding/json" + "errors" "os" "os/exec" "path/filepath" @@ -100,7 +101,7 @@ func TestBashDeadlineTerminatesDescendants(t *testing.T) { input := json.RawMessage(`{"command":"sleep 1; echo survived > survived.txt"}`) started := time.Now() output, err := (BashTool{}).Execute(ctx, input) - if err != nil || !strings.Contains(output, "timed out") { + if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("expected deadline result, got %q, %v", output, err) } if elapsed := time.Since(started); elapsed > 750*time.Millisecond { diff --git a/internal/tool/retry.go b/internal/tool/retry.go index 4860e21cb..914b40ddd 100644 --- a/internal/tool/retry.go +++ b/internal/tool/retry.go @@ -81,16 +81,18 @@ func RetryExecutor(ctx context.Context, t Tool, input []byte, policy RetryPolicy return t.Execute(ctx, input) } var lastErr error + var lastOutput string delay := policy.BaseDelay for attempt := 0; attempt <= policy.MaxRetries; attempt++ { if err := ctx.Err(); err != nil { - return "", err + return lastOutput, err } out, err := t.Execute(ctx, input) if err == nil { return out, nil } lastErr = err + lastOutput = out transient := IsTransientError(err) retry := transient if policy.ShouldRetry != nil { @@ -112,7 +114,7 @@ func RetryExecutor(ctx context.Context, t Tool, input []byte, policy RetryPolicy delay = policy.MaxDelay } } - return "", lastErr + return lastOutput, lastErr } // IsTransientFileErr reports whether err is a transient file-I/O error diff --git a/internal/tool/task_tools.go b/internal/tool/task_tools.go index b201e0992..f2fd37118 100644 --- a/internal/tool/task_tools.go +++ b/internal/tool/task_tools.go @@ -147,11 +147,15 @@ func startBackgroundBash(ctx context.Context, command string, execName string, e } else if waitErr != nil { status = taskruntime.StatusFailed errMsg = waitErr.Error() + task.exitText = "exit code: " + waitErr.Error() } out := task.output.String() + if task.exitText != "" { + out += "\n\n" + task.exitText + } task.mu.Unlock() - close(task.done) taskruntime.Default.FinishExternal(id, status, out, errMsg) + close(task.done) shellCancel() }() @@ -206,6 +210,8 @@ func (t *backgroundTask) snapshot() (status, output string) { status = "completed" if t.stopped { status = "stopped" + } else if t.exitText != "" { + status = "failed" } default: } From c87c4385992caf6009829e043c88765575929083 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 4 Nov 2026 02:56:10 +0530 Subject: [PATCH 2/2] fix(cli): preserve test and lint repair diagnostics --- cmd/chat_subcommand_lint.go | 7 +++--- cmd/chat_subcommand_shell.go | 14 +++++++++--- cmd/chat_subcommand_shell_test.go | 35 +++++++++++++++++++++++++++++ cmd/chat_subcommand_test_cmd.go | 5 +++-- internal/engine/stream_tool_exec.go | 23 +++++++++++++++++-- 5 files changed, 74 insertions(+), 10 deletions(-) diff --git a/cmd/chat_subcommand_lint.go b/cmd/chat_subcommand_lint.go index b917605d0..e91778d91 100644 --- a/cmd/chat_subcommand_lint.go +++ b/cmd/chat_subcommand_lint.go @@ -23,12 +23,13 @@ func (l *lintSubcommand) Handle(m *chatModel, args []string, text string) (tea.M if len(args) >= 1 { cmdStr = strings.TrimSpace(strings.TrimPrefix(text, "/lint")) } - result, isErr := runSlashShellCommand(m, cmdStr) - if isErr { + status := runSlashShellCommandResult(m, cmdStr) + result := status.Output + if status.IsError && !status.CommandFailed { m.messages = append(m.messages, displayMsg{role: "error", content: result}) return m, nil } - if result == "" { + if !status.IsError { m.messages = append(m.messages, displayMsg{role: "system", content: "No lint issues."}) } else { m.messages = append(m.messages, displayMsg{role: "system", content: fmt.Sprintf("Lint issues:\n%s", result)}) diff --git a/cmd/chat_subcommand_shell.go b/cmd/chat_subcommand_shell.go index 616dc5e79..ef4935172 100644 --- a/cmd/chat_subcommand_shell.go +++ b/cmd/chat_subcommand_shell.go @@ -3,14 +3,22 @@ package cmd import ( "context" "strings" + + "github.com/GrayCodeAI/reflect/internal/engine" ) func runSlashShellCommand(m *chatModel, command string) (string, bool) { + result := runSlashShellCommandResult(m, command) + return result.Output, result.IsError +} + +func runSlashShellCommandResult(m *chatModel, command string) engine.ShellCommandResult { if m == nil || m.session == nil { - return "Error: no active session", true + return engine.ShellCommandResult{Output: "Error: no active session", IsError: true} } - out, isErr := m.session.RunUserShellCommand(context.Background(), command, 0) - return strings.TrimSpace(out), isErr + result := m.session.RunUserShellCommandResult(context.Background(), command, 0) + result.Output = strings.TrimSpace(result.Output) + return result } func shellCommandFailed(output string, isErr bool) bool { diff --git a/cmd/chat_subcommand_shell_test.go b/cmd/chat_subcommand_shell_test.go index e9c4cd898..0d1c5f8eb 100644 --- a/cmd/chat_subcommand_shell_test.go +++ b/cmd/chat_subcommand_shell_test.go @@ -119,3 +119,38 @@ func TestTestSubcommand_DetectsNonzeroExit(t *testing.T) { t.Fatalf("session messages = %#v, want failure context", msgs) } } + +func TestLintSubcommandQueuesActualFailureOnly(t *testing.T) { + for _, tc := range []struct { + command string + failed bool + }{ + {command: "printf 'compiler diagnostic'; false | tail -5", failed: true}, + {command: "printf '0 issues.'"}, + } { + t.Run(tc.command, func(t *testing.T) { + m := newTestChatModel() + m.session.PermSvc().SetAutonomy(safety.AutonomyYOLO) + result, _ := (&lintSubcommand{}).Handle(m, []string{tc.command}, "/lint "+tc.command) + cm := requireChatModel(t, result) + messages := cm.session.RawMessages() + if tc.failed { + if len(messages) != 1 || !strings.Contains(messages[0].Content, "Please fix these lint issues") || !strings.Contains(messages[0].Content, "compiler diagnostic") { + t.Fatalf("lost repair diagnostics: %#v", messages) + } + } else if len(messages) != 0 || !strings.Contains(cm.messages[len(cm.messages)-1].content, "No lint issues") { + t.Fatalf("successful linter triggered unnecessary repairs: %#v", messages) + } + }) + } +} + +func TestTestSubcommandDoesNotTreatDenialAsTestFailure(t *testing.T) { + m := newTestChatModel() + m.session.PermSvc().SetAutonomy(safety.AutonomyYOLO) + result, _ := (&testSubcommand{}).Handle(m, []string{"env"}, "/test env") + cm := requireChatModel(t, result) + if len(cm.session.RawMessages()) != 0 || cm.messages[len(cm.messages)-1].role != "error" { + t.Fatal("blocked command was queued as a code repair") + } +} diff --git a/cmd/chat_subcommand_test_cmd.go b/cmd/chat_subcommand_test_cmd.go index ab3388283..fc5bc4d7a 100644 --- a/cmd/chat_subcommand_test_cmd.go +++ b/cmd/chat_subcommand_test_cmd.go @@ -23,8 +23,9 @@ func (t *testSubcommand) Handle(m *chatModel, args []string, text string) (tea.M if len(args) >= 1 { cmdStr = strings.TrimSpace(strings.TrimPrefix(text, "/test")) } - result, isErr := runSlashShellCommand(m, cmdStr) - if isErr { + status := runSlashShellCommandResult(m, cmdStr) + result, isErr := status.Output, status.IsError + if isErr && !status.CommandFailed { m.messages = append(m.messages, displayMsg{role: "error", content: result}) return m, nil } diff --git a/internal/engine/stream_tool_exec.go b/internal/engine/stream_tool_exec.go index fb112e210..b50971ec3 100644 --- a/internal/engine/stream_tool_exec.go +++ b/internal/engine/stream_tool_exec.go @@ -2,8 +2,10 @@ package engine import ( "context" + "errors" "fmt" "os" + "os/exec" "slices" "strings" @@ -195,6 +197,20 @@ func isNetworkReadOnlyTool(name string) bool { // permission, approval, timeout, retry, truncation, hook, // and post-processing path used by model-initiated Bash tool calls. func (s *Session) RunUserShellCommand(ctx context.Context, command string, timeoutSeconds int) (string, bool) { + result := s.RunUserShellCommandResult(ctx, command, timeoutSeconds) + return result.Output, result.IsError +} + +// ShellCommandResult distinguishes failed commands from denied tool calls so +// test/lint commands can offer repair feedback without treating denials as code defects. +type ShellCommandResult struct { + Output string + IsError bool + CommandFailed bool +} + +// RunUserShellCommandResult includes the process failure status alongside output. +func (s *Session) RunUserShellCommandResult(ctx context.Context, command string, timeoutSeconds int) ShellCommandResult { if ctx == nil { ctx = context.Background() } @@ -208,7 +224,8 @@ func (s *Session) RunUserShellCommand(ctx context.Context, command string, timeo Name: "Bash", Arguments: args, }, tool.BashTool{}, ch, 0, command) - return result.output, result.isErr + var exitErr *exec.ExitError + return ShellCommandResult{Output: result.output, IsError: result.isErr, CommandFailed: errors.As(result.err, &exitErr)} } // executeSingleTool runs one tool call with permission checks and all post-processing. @@ -239,7 +256,9 @@ func (s *Session) executeSingleToolWithTool(ctx context.Context, tc types.ToolCa output, execErr, isErr := core.output, core.err, core.isErr if isErr { s.Logger().Warn("tool execution error", map[string]interface{}{"tool": tc.Name, "error": execErr.Error()}) - output = fmt.Sprintf("Error: %s", execErr.Error()) + if strings.TrimSpace(output) == "" { + output = fmt.Sprintf("Error: %s", execErr.Error()) + } if s.LifecycleSvc().Backtrack() != nil { s.LifecycleSvc().Backtrack().MarkOutcome(turnCount, "failure") }