diff --git a/server/pkg/agent/kiro.go b/server/pkg/agent/kiro.go index 00a644e2f..1facffaa8 100644 --- a/server/pkg/agent/kiro.go +++ b/server/pkg/agent/kiro.go @@ -107,8 +107,27 @@ func (b *kiroBackend) Execute(ctx context.Context, prompt string, opts ExecOptio var outputMu sync.Mutex var output strings.Builder var streamingCurrentTurn atomic.Bool + // Completion-preservation state for the -32603 close-handshake guard. + // We track each finish-signalling tool (goal_complete, `multica issue + // comment add`) along three axes so the guard can tell three cases + // apart: + // - saw*Use: we observed the tool being invoked this turn. + // - saw*Result: we observed a terminal ToolResult for it (any status). + // - sawCompleted*: that terminal ToolResult was status=="completed". + // The Claude/Kiro path always emits a completed ToolResult, so + // sawCompleted* is enough there. The GPT-5.6 Sol adapter, however, can + // leave the terminal tool parked at "running" and never emit a + // completed/failed ToolCallUpdate — so no ToolResult is ever produced + // (hermes.handleToolCallUpdate only dispatches on completed/failed). + // For that path we fall back to "saw the use but never saw a result" + // (see the guard below), which stays safe because a genuinely failed + // tool still emits a failed ToolResult and trips saw*Result. var sawCompletedGoalComplete atomic.Bool var sawCompletedIssueComment atomic.Bool + var sawGoalCompleteUse atomic.Bool + var sawGoalCompleteResult atomic.Bool + var sawIssueCommentUse atomic.Bool + var sawIssueCommentResult atomic.Bool var goalCompleteCallIDs sync.Map var issueCommentCallIDs sync.Map @@ -130,17 +149,33 @@ func (b *kiroBackend) Execute(ctx context.Context, prompt string, opts ExecOptio msg.Tool = kiroToolNameFromTitle(msg.Tool) if msg.Tool == "goal_complete" && msg.CallID != "" { goalCompleteCallIDs.Store(msg.CallID, struct{}{}) + sawGoalCompleteUse.Store(true) } - if msg.CallID != "" && isKiroIssueCommentAddTool(msg) { - issueCommentCallIDs.Store(msg.CallID, struct{}{}) + // Recognize `multica issue comment add` by its command + // payload regardless of how the adapter titled the tool. + // GPT-5.6 Sol may label the shell tool something other than + // the aliases kiroToolNameFromTitle folds into "terminal" + // (e.g. "execute"/"run"), so gating on msg.Tool=="terminal" + // here would miss it entirely. + if isKiroIssueCommentAddTool(msg) { + sawIssueCommentUse.Store(true) + if msg.CallID != "" { + issueCommentCallIDs.Store(msg.CallID, struct{}{}) + } } } if msg.Type == MessageToolResult { if _, ok := goalCompleteCallIDs.LoadAndDelete(msg.CallID); ok { - sawCompletedGoalComplete.Store(msg.Status == "completed") + sawGoalCompleteResult.Store(true) + if msg.Status == "completed" { + sawCompletedGoalComplete.Store(true) + } } if _, ok := issueCommentCallIDs.LoadAndDelete(msg.CallID); ok { - sawCompletedIssueComment.Store(msg.Status == "completed") + sawIssueCommentResult.Store(true) + if msg.Status == "completed" { + sawCompletedIssueComment.Store(true) + } } } if msg.Type == MessageText { @@ -333,7 +368,22 @@ func (b *kiroBackend) Execute(ctx context.Context, prompt string, opts ExecOptio } else { finalStatus = "failed" finalError = fmt.Sprintf("kiro session/prompt failed: %v", err) - if (sawCompletedGoalComplete.Load() || sawCompletedIssueComment.Load()) && isKiroGoalCompleteCloseError(err) { + // Completion-preservation guard for the -32603 close + // handshake Kiro raises after a task has finished its work. + // + // completedResult — we saw a completed ToolResult for + // goal_complete / comment-add (Claude path). + // useWithoutResult — we saw the finishing tool invoked but + // never saw ANY terminal ToolResult for it + // (GPT-5.6 Sol path: the tool stays parked + // at "running"). We deliberately exclude + // the case where a failed ToolResult DID + // arrive — that is a real failure and must + // stay failed. + completedResult := sawCompletedGoalComplete.Load() || sawCompletedIssueComment.Load() + useWithoutResult := (sawGoalCompleteUse.Load() && !sawGoalCompleteResult.Load()) || + (sawIssueCommentUse.Load() && !sawIssueCommentResult.Load()) + if (completedResult || useWithoutResult) && isKiroGoalCompleteCloseError(err) { b.cfg.Logger.Warn("kiro session/prompt failed after completed task result; preserving completed task status", "error", err) finalStatus = "completed" finalError = "" @@ -431,10 +481,14 @@ func isKiroGoalCompleteCloseError(err error) bool { return strings.Contains(strings.ToLower(rpcErr.Data), "failed to generate a response") } +// isKiroIssueCommentAddTool reports whether a tool-use message is a +// `multica issue comment add` invocation. It keys purely off the command +// payload, not the normalized tool name: GPT-5.6 Sol adapters may title the +// shell tool with a name that doesn't fold into "terminal" (the earlier +// msg.Tool=="terminal" gate silently dropped those and left completed tasks +// marked failed — see #5509). isKiroIssueCommentAddCommand is strict enough +// that no non-shell tool's input will accidentally match. func isKiroIssueCommentAddTool(msg Message) bool { - if msg.Tool != "terminal" { - return false - } command, _ := msg.Input["command"].(string) return isKiroIssueCommentAddCommand(command) } diff --git a/server/pkg/agent/kiro_test.go b/server/pkg/agent/kiro_test.go index 60f341e6d..116ae532f 100644 --- a/server/pkg/agent/kiro_test.go +++ b/server/pkg/agent/kiro_test.go @@ -412,6 +412,228 @@ func TestKiroIssueCommentAddCommand(t *testing.T) { } } +// TestKiroIssueCommentAddToolIgnoresToolTitle pins the #5509 decoupling: +// `multica issue comment add` must be recognized from its command payload +// regardless of the tool's normalized name. The GPT-5.6 Sol adapter can title +// the shell tool something that doesn't fold into "terminal", and the old +// msg.Tool=="terminal" gate silently dropped those. +func TestKiroIssueCommentAddToolIgnoresToolTitle(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + tool string + want bool + }{ + {"terminal", "terminal", true}, + {"gpt-style execute", "execute_bash", true}, + {"gpt-style run", "run", true}, + {"empty tool name", "", true}, + {"tool name is irrelevant when command matches", "read_file", true}, + } + for _, tt := range tests { + msg := Message{ + Type: MessageToolUse, + Tool: tt.tool, + Input: map[string]any{"command": "multica issue comment add issue-1 --content-file ./reply.md"}, + } + if got := isKiroIssueCommentAddTool(msg); got != tt.want { + t.Errorf("%s: isKiroIssueCommentAddTool(tool=%q) = %v, want %v", tt.name, tt.tool, got, tt.want) + } + } + + // A non-comment-add command must still be rejected regardless of title. + notComment := Message{Type: MessageToolUse, Tool: "terminal", Input: map[string]any{"command": "ls -la"}} + if isKiroIssueCommentAddTool(notComment) { + t.Errorf("isKiroIssueCommentAddTool should reject a non-comment-add command") + } +} + +// fakeKiroACPRunningToolCloseErrorScript reproduces the GPT-5.6 Sol failure +// mode from #5509: the finishing tool is invoked (ToolCall notification) but +// the adapter never emits a completed/failed ToolCallUpdate — the tool stays +// parked at "running", so no ToolResult is ever produced. Kiro then raises the +// -32603 "failed to generate a response" close handshake. `toolName` lets the +// caller pick a title that does NOT normalize to "terminal", and `command` +// selects goal_complete vs comment-add. +func fakeKiroACPRunningToolCloseErrorScript(toolName, command string) string { + params := `{}` + if command != "" { + params = `{"command":"` + command + `"}` + } + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":true}}}\n' "$id" + ;; + *'"method":"session/new"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"ses_running_done"}}\n' "$id" + ;; + *'"method":"session/prompt"'*) + printf '{"jsonrpc":"2.0","method":"session/notification","params":{"sessionId":"ses_running_done","update":{"type":"ToolCall","toolCallId":"tc-running","name":"` + toolName + `","status":"pending","parameters":` + params + `}}}\n' + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Internal error","data":"Kiro failed to generate a response"}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestKiroBackendTreatsRunningCommentAddCloseErrorAsCompleted is the primary +// #5509 regression: a `multica issue comment add` shell tool titled with a +// non-terminal name, left at "running" (no completed ToolResult), followed by +// the -32603 close handshake, must be preserved as completed. +func TestKiroBackendTreatsRunningCommentAddCloseErrorAsCompleted(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "kiro-cli") + writeTestExecutable(t, fakePath, []byte(fakeKiroACPRunningToolCloseErrorScript( + "execute_bash", + "multica issue comment add issue-1 --content-file ./reply.md", + ))) + + backend, err := New("kiro", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new kiro backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{Timeout: 5 * time.Second}) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "completed" { + t.Fatalf("expected status=completed for a running comment-add + close error, got %q (error=%q)", result.Status, result.Error) + } + if result.Error != "" { + t.Fatalf("expected close-handshake error to be suppressed, got %q", result.Error) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + +// TestKiroBackendTreatsRunningGoalCompleteCloseErrorAsCompleted covers the +// goal_complete sibling of the running-tool path. +func TestKiroBackendTreatsRunningGoalCompleteCloseErrorAsCompleted(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "kiro-cli") + writeTestExecutable(t, fakePath, []byte(fakeKiroACPRunningToolCloseErrorScript("goal_complete", ""))) + + backend, err := New("kiro", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new kiro backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{Timeout: 5 * time.Second}) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "completed" { + t.Fatalf("expected status=completed for a running goal_complete + close error, got %q (error=%q)", result.Status, result.Error) + } + if result.Error != "" { + t.Fatalf("expected close-handshake error to be suppressed, got %q", result.Error) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + +// fakeKiroACPFailedRunningToolCloseErrorScript is the safety counterpart: the +// finishing tool DID emit a failed ToolResult before the close error. That is +// a genuine failure and must stay failed even though we saw the tool use. +func fakeKiroACPFailedRunningToolCloseErrorScript() string { + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":true}}}\n' "$id" + ;; + *'"method":"session/new"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"ses_failed_done"}}\n' "$id" + ;; + *'"method":"session/prompt"'*) + printf '{"jsonrpc":"2.0","method":"session/notification","params":{"sessionId":"ses_failed_done","update":{"type":"ToolCall","toolCallId":"tc-fail","name":"execute_bash","status":"pending","parameters":{"command":"multica issue comment add issue-1 --content-file ./reply.md"}}}}\n' + printf '{"jsonrpc":"2.0","method":"session/notification","params":{"sessionId":"ses_failed_done","update":{"type":"ToolCallUpdate","toolCallId":"tc-fail","status":"failed","name":"execute_bash","output":"exit status 1"}}}\n' + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Internal error","data":"Kiro failed to generate a response"}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestKiroBackendDoesNotCompleteAfterFailedCommentAddResult guards the +// tradeoff: a failed ToolResult must not be rescued by the running-tool path. +func TestKiroBackendDoesNotCompleteAfterFailedCommentAddResult(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "kiro-cli") + writeTestExecutable(t, fakePath, []byte(fakeKiroACPFailedRunningToolCloseErrorScript())) + + backend, err := New("kiro", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new kiro backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{Timeout: 5 * time.Second}) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "failed" { + t.Fatalf("expected status=failed after a failed comment-add result, got %q (error=%q)", result.Status, result.Error) + } + if !strings.Contains(result.Error, "Kiro failed to generate a response") { + t.Fatalf("expected original prompt error to be preserved, got %q", result.Error) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + // fakeKiroACPStaleLoadSetModelScript impersonates kiro when a resumed // session is gone and the caller picked a model: session/load returns // an empty result (so the requested id is kept), then