From cf1a5720c6b4215fd652ee966e59f26d67bcc4bc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 21:09:50 +0900 Subject: [PATCH 1/9] fix(kiro): explain empty exec output and stop reopening a delivered final answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects made a Kiro code-mode session unable to see its own tool output and unable to end its turn. **Empty exec output was unexplained.** A code-mode `exec` cell that never calls `text()`/`notify()` returns nothing — the last expression value is not echoed. Kiro received the generic "The tool completed without textual output.", so the model concluded its earlier context was lost and restarted finished work. Cursor already had the actionable wording for exactly this state; Kiro had no equivalent. The detection now lives in `src/adapters/exec-tool-result-normalize.ts` and both adapters share one message, so the wording cannot drift. Cursor keeps `normalizeCursorToolResultText` as a wrapper: it owns Computer Use precedence and isError policy, which are not Kiro's. Checked before `text.trim()` deliberately — the wrapper form ("Script completed\nWall time ...\nOutput:\n") is non-blank and would otherwise pass through as though it were real output. Non-exec tools keep the generic message; asserting code-mode semantics for arbitrary tools would tell a model to call `text()` in a runtime that has neither. **A delivered final answer was told to continue.** The trailing-turn predicate was `turns.at(-1)?.kind === "assistant"`, which cannot tell "stopped mid-task" from "already answered". Both got a continuation or completion-retry prompt, so a closed task read as a still-open goal and looped. The turn now carries whether it was the delivered final answer, keyed off Responses `phase: "final_answer"` (preserved through the parser) rather than guessed from position. Kiro still requires a trailing user turn, so the turn is still appended — only its text changes, to a neutral acknowledgement that withholds any instruction to resume. Removing the append instead would throw at the `currentTurn` pop. The `text_fallback` and thinking-tag paths are both excluded, since either would re-append the resume instruction and reinstate the loop. A merged assistant turn counts as final only when its last component was, so commentary after an answer correctly reopens continuation. Verification: `bun x tsc --noEmit` clean; 201 tests pass across kiro-adapter, kiro-stream, cursor-exec-empty-result, cursor-toolresult-normalize and server-kiro-completion-e2e; privacy:scan and the core/lab boundary and repo-hygiene guards green. Both new assertions were driven red by reverting each fix individually, so neither is vacuous. --- src/adapters/cursor/tool-result-normalize.ts | 36 ++------ src/adapters/exec-tool-result-normalize.ts | 66 ++++++++++++++ src/adapters/kiro-constants.ts | 12 +++ src/adapters/kiro.ts | 68 ++++++++++++-- tests/kiro-adapter.test.ts | 95 +++++++++++++++++++- 5 files changed, 238 insertions(+), 39 deletions(-) create mode 100644 src/adapters/exec-tool-result-normalize.ts diff --git a/src/adapters/cursor/tool-result-normalize.ts b/src/adapters/cursor/tool-result-normalize.ts index c89e247e9a1..90526e2a80e 100644 --- a/src/adapters/cursor/tool-result-normalize.ts +++ b/src/adapters/cursor/tool-result-normalize.ts @@ -9,6 +9,12 @@ * dropping images oldest-first — see toolCallStep in protobuf-request.ts). */ +import { + EMPTY_EXEC_OUTPUT_MESSAGE, + EMPTY_EXEC_OUTPUT_REGEX, + isCodexExecBridgeTool, +} from "../exec-tool-result-normalize"; + const COMPUTER_USE_TOOL_NAMES = new Set([ "node_repl", "node_repl__js", @@ -29,31 +35,6 @@ function isNodeReplOrComputerUseTool(toolName?: string, toolNamespace?: string): return lower.startsWith("mcp__node_repl") || lower.startsWith("mcp__computer_use"); } -/** - * Codex exec / shell-bridge tool names (flat and MCP-prefixed display aliases). An empty result - * here is almost always a code-mode cell that never called text()/notify() — the cursor model - * reads the blank [tool_result], concludes prior results were lost, and spirals into - * re-orientation retries (devlog 260826_cursor_responses_gap, live subagent transcripts). - */ -function isCodexExecBridgeTool(toolName?: string, toolNamespace?: string): boolean { - if (toolNamespace && toolNamespace.includes("opencodex-responses")) return true; - if (!toolName) return false; - const lower = toolName.toLowerCase(); - return ( - lower === "exec" - || lower === "exec_command" - || lower === "shell_command" - // Codex CLI/desktop native tool names: the multi-round "이전 출력이 비어 있어 처음부터" - // restart loop reproduced via codex exec because `shell` was not in this set - // (devlog 260826 gap-8 QA round 2). - || lower === "shell" - || lower === "local_shell" - || lower === "container.exec" - || lower.startsWith("mcp_opencodex-responses_") - || lower.startsWith("mcp__opencodex-responses__") - ); -} - /** Failure states the Computer Use / node_repl runtime reports as PLAIN TEXT inside a non-error result. */ const RUNTIME_FAILURE_GUIDANCE: ReadonlyArray<{ marker: string; guidance: string }> = [ { @@ -74,9 +55,6 @@ const RUNTIME_FAILURE_GUIDANCE: ReadonlyArray<{ marker: string; guidance: string }, ]; -/** Matches exec wrappers whose only payload is an empty-output marker. */ -const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Script failed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; - export interface NormalizedToolResultText { text: string; isError: boolean; @@ -107,7 +85,7 @@ export function normalizeCursorToolResultText( } if (isCodexExecBridgeTool(options.toolName, options.toolNamespace) && EMPTY_EXEC_OUTPUT_REGEX.test(text.trim())) { return { - text: "[empty output: the exec cell completed but emitted nothing. This is NOT lost context and NOT a blocked tool — in code mode call text(...) or notify(...) on any value you need to see (a bare await tools.exec_command(...) is not echoed automatically); in shell mode the command simply printed nothing. Do not re-run the same call expecting different output.]", + text: EMPTY_EXEC_OUTPUT_MESSAGE, isError: false, changed: true, }; diff --git a/src/adapters/exec-tool-result-normalize.ts b/src/adapters/exec-tool-result-normalize.ts new file mode 100644 index 00000000000..7a7b093824d --- /dev/null +++ b/src/adapters/exec-tool-result-normalize.ts @@ -0,0 +1,66 @@ +/** + * Provider-neutral empty-exec-output normalization. + * + * A code-mode `exec` cell that never calls `text()`/`notify()` returns nothing: the last + * expression value is NOT echoed automatically. The routed model reads a blank tool result, + * concludes its earlier output was lost, and burns turns re-running the same call or restarting + * the task from scratch. Naming that state explicitly is what breaks the loop. + * + * This module owns the shared detection so every adapter reports the same thing. Cursor keeps its + * own wrapper (`normalizeCursorToolResultText`) for Computer Use precedence and isError policy; + * Kiro consumes this helper directly. + */ + +/** Matches exec wrappers whose only payload is an empty-output marker. */ +export const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Script failed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; + +/** + * The guidance itself. Worded to close all three wrong conclusions a model draws from a blank + * result: that context was lost, that the tool is blocked, and that retrying will differ. + */ +export const EMPTY_EXEC_OUTPUT_MESSAGE = + "[empty output: the exec cell completed but emitted nothing. This is NOT lost context and NOT a blocked tool — in code mode call text(...) or notify(...) on any value you need to see (a bare await tools.exec_command(...) is not echoed automatically); in shell mode the command simply printed nothing. Do not re-run the same call expecting different output.]"; + +/** + * Codex exec / shell-bridge tool names (flat and MCP-prefixed display aliases). An empty result + * here is almost always a code-mode cell that never called text()/notify(). + */ +export function isCodexExecBridgeTool(toolName?: string, toolNamespace?: string): boolean { + if (toolNamespace && toolNamespace.includes("opencodex-responses")) return true; + if (!toolName) return false; + const lower = toolName.toLowerCase(); + return ( + lower === "exec" + || lower === "exec_command" + || lower === "shell_command" + // Codex CLI/desktop native tool names: the multi-round "이전 출력이 비어 있어 처음부터" + // restart loop reproduced via codex exec because `shell` was not in this set + // (devlog 260826 gap-8 QA round 2). + || lower === "shell" + || lower === "local_shell" + || lower === "container.exec" + || lower.startsWith("mcp_opencodex-responses_") + || lower.startsWith("mcp__opencodex-responses__") + ); +} + +/** True when this result is an exec-bridge call that produced no usable output. */ +export function isEmptyExecToolResult( + text: string, + options: { toolName?: string; toolNamespace?: string } = {}, +): boolean { + return isCodexExecBridgeTool(options.toolName, options.toolNamespace) + && EMPTY_EXEC_OUTPUT_REGEX.test(text.trim()); +} + +/** + * Returns the guidance text when this is an empty exec-bridge result, else `undefined` so the + * caller keeps its own fallback. Undefined rather than the original text: an adapter must be able + * to tell "not my case" from "normalized to the same string". + */ +export function normalizeEmptyExecToolResultText( + text: string, + options: { toolName?: string; toolNamespace?: string } = {}, +): string | undefined { + return isEmptyExecToolResult(text, options) ? EMPTY_EXEC_OUTPUT_MESSAGE : undefined; +} diff --git a/src/adapters/kiro-constants.ts b/src/adapters/kiro-constants.ts index 4b3c709135e..a4e52472cd7 100644 --- a/src/adapters/kiro-constants.ts +++ b/src/adapters/kiro-constants.ts @@ -22,6 +22,18 @@ export const KIRO_COMPLETION_RETRY_MESSAGE = export const KIRO_TOOL_RESULT_CARRIER_MESSAGE = "The requested tool result is attached."; export const KIRO_EMPTY_TOOL_RESULT_MESSAGE = "The tool completed without textual output."; +/** + * Placeholder for the user turn Kiro requires after an assistant turn that ALREADY delivered its + * final answer. + * + * The protocol needs a trailing user turn, but the usual continuation/retry text instructs the + * model to keep working, which reopens a finished task and reads as a still-open goal. This states + * the delivered state and explicitly withholds a new request, so the turn stays structurally valid + * without asking for more work. + */ +export const KIRO_ANSWER_DELIVERED_MESSAGE = + "The previous final answer was delivered to the user and that task is closed. No new request has been made yet. Do not repeat, revise, or continue that work; wait for the user's next instruction."; + export const KIRO_COMPLETION_INSTRUCTIONS = `When tools are available, ordinary assistant text is mid-task commentary and does not end the turn. Continue using tools after progress updates. When the task is fully complete and no more tool calls are needed, call ${KIRO_COMPLETION_TOOL_NAME} exactly once with the complete user-facing final answer in \`answer\`. Do not provide the final answer as ordinary assistant text.`; diff --git a/src/adapters/kiro.ts b/src/adapters/kiro.ts index b4161be1964..c7ce8d922a0 100644 --- a/src/adapters/kiro.ts +++ b/src/adapters/kiro.ts @@ -42,9 +42,11 @@ import { extractKiroImages, normalizeKiroImages, type KiroImage } from "./kiro-i import { sniffImageDimensions } from "./anthropic-image-guard"; import { fetchKiroWithRetry, noteKiroTransientThrottle } from "./kiro-retry"; import { convertKiroToolContext } from "./kiro-tools"; +import { normalizeEmptyExecToolResultText } from "./exec-tool-result-normalize"; import { identifyRoutedModel } from "./identity"; import { buildNonOpenAIToolCatalogNudgeFromNames, isBareShellBridgeTool, isCodexCodeModeExecTool } from "./tool-catalog-nudge"; import { + KIRO_ANSWER_DELIVERED_MESSAGE, KIRO_COMPLETION_INSTRUCTIONS, KIRO_COMPLETION_RETRY_MESSAGE, KIRO_COMPLETION_TOOL_NAME, @@ -344,7 +346,19 @@ function validateKiroCapabilities(parsed: OcxParsedRequest): void { type KiroTurn = | { kind: "user"; content: string; images: KiroImage[]; toolResults: KiroToolResult[] } - | { kind: "assistant"; content: string; toolUses: KiroToolUse[]; redactedReasoning?: string }; + | { + kind: "assistant"; + content: string; + toolUses: KiroToolUse[]; + redactedReasoning?: string; + /** + * True when this assistant turn was the DELIVERED final answer (Responses + * `phase: "final_answer"`). A trailing assistant turn normally means the model stopped + * mid-task and needs a continuation prompt, but a delivered final answer already ended its + * turn — prompting it again restarts finished work as if a goal were still open. + */ + finalAnswer?: boolean; + }; function appendTurnText(target: string, next: string): string { if (!next) return target; @@ -521,15 +535,24 @@ export function buildKiroPayload( turns.push({ kind: "user", content, images: [...images], toolResults: [...toolResults] }); } }; - const pushAssistant = (content: string, toolUses: KiroToolUse[], redactedReasoning?: string): void => { + const pushAssistant = (content: string, toolUses: KiroToolUse[], redactedReasoning?: string, finalAnswer?: boolean): void => { const last = turns.at(-1); if (last?.kind === "assistant") { last.content = appendTurnText(last.content, content); last.toolUses.push(...toolUses); // Merged turns keep the newest blob: it covers the reasoning up to the merged turn's end. if (redactedReasoning) last.redactedReasoning = redactedReasoning; + // A merged turn is final only if its LAST component was: commentary appended after a final + // answer means the model kept working, so the turn is no longer terminal. + last.finalAnswer = finalAnswer === true; } else { - turns.push({ kind: "assistant", content, toolUses: [...toolUses], ...(redactedReasoning ? { redactedReasoning } : {}) }); + turns.push({ + kind: "assistant", + content, + toolUses: [...toolUses], + ...(redactedReasoning ? { redactedReasoning } : {}), + ...(finalAnswer ? { finalAnswer: true } : {}), + }); } }; @@ -559,14 +582,24 @@ export function buildKiroPayload( const hasReasoning = aMsg.content.some(part => part.type === "thinking" && part.thinking.trim()); if (hasReasoning) continue; } - pushAssistant(text, toolUses, aMsg.kiroRedactedReasoning); + // `phase` survives the Responses round trip (parser.ts assistant branch), so a replayed + // final answer is identifiable here rather than guessed from turn position. + pushAssistant(text, toolUses, aMsg.kiroRedactedReasoning, aMsg.phase === "final_answer" && toolUses.length === 0); } else if (msg.role === "toolResult") { const tr = msg as OcxToolResultMessage; if (tr.containsEncryptedContent) { throw new Error(`Kiro cannot translate encrypted output for tool call ${JSON.stringify(tr.toolCallId)}`); } const text = userContentText(tr.content); - const resultText = text.trim() ? text : KIRO_EMPTY_TOOL_RESULT_MESSAGE; + // An empty code-mode exec result needs the SPECIFIC reason, not the generic fallback: the + // model otherwise reads a blank result, concludes its earlier context was lost, and restarts + // the task instead of calling text()/notify(). Checked before `text.trim()` because the + // wrapper form ("Script completed\nWall time ...\nOutput:\n") is non-blank and would + // otherwise pass through as if it were real output. + const resultText = normalizeEmptyExecToolResultText(text, { + toolName: tr.toolName, + toolNamespace: tr.toolNamespace, + }) ?? (text.trim() ? text : KIRO_EMPTY_TOOL_RESULT_MESSAGE); const images = extractKiroImages(tr.content); const toolUseId = normalizeToolId(tr.toolCallId); if (!priorCalls.has(toolUseId)) { @@ -587,10 +620,20 @@ export function buildKiroPayload( if (turns.length === 0 || turns[0].kind === "assistant") { turns.unshift({ kind: "user", content: KIRO_CONTINUATION_MESSAGE, images: [], toolResults: [] }); } - if (turns.at(-1)?.kind === "assistant") { + // Kiro requires the request to end with a user turn, so a trailing assistant turn always gets + // one appended (the pop below throws otherwise). What that turn SAYS is the load-bearing part. + // + // Normally a trailing assistant turn means the model stopped mid-task, and a continuation/retry + // prompt is correct. A DELIVERED final answer is the exception: the turn already ended, and + // telling that model to "continue" or to call the completion tool again reopens finished work — + // the completed-task-behaves-like-an-open-goal loop. It gets a neutral acknowledgement instead: + // structurally valid, but carrying no instruction to resume. + const trailing = turns.at(-1); + if (trailing?.kind === "assistant") { + const resumeText = completionMode === "text_fallback" ? KIRO_COMPLETION_RETRY_MESSAGE : KIRO_CONTINUATION_MESSAGE; turns.push({ kind: "user", - content: completionMode === "text_fallback" ? KIRO_COMPLETION_RETRY_MESSAGE : KIRO_CONTINUATION_MESSAGE, + content: trailing.finalAnswer ? KIRO_ANSWER_DELIVERED_MESSAGE : resumeText, images: [], toolResults: [], }); @@ -638,10 +681,17 @@ export function buildKiroPayload( currentUim.userInputMessageContext = { ...(currentUim.userInputMessageContext ?? {}), tools: kiroTools }; } if (completionMode === "text_fallback") { - if (currentUim.content !== KIRO_COMPLETION_RETRY_MESSAGE) { + // Never append the retry instruction onto the answer-delivered placeholder: that placeholder + // exists precisely to avoid asking a finished turn for another completion call, and appending + // here would reinstate the loop the placeholder prevents. + if (currentUim.content !== KIRO_COMPLETION_RETRY_MESSAGE && currentUim.content !== KIRO_ANSWER_DELIVERED_MESSAGE) { currentUim.content = appendTurnText(currentUim.content, KIRO_COMPLETION_RETRY_MESSAGE); } - } else if (!currentUim.userInputMessageContext?.toolResults && currentUim.content !== KIRO_CONTINUATION_MESSAGE) { + } else if ( + !currentUim.userInputMessageContext?.toolResults + && currentUim.content !== KIRO_CONTINUATION_MESSAGE + && currentUim.content !== KIRO_ANSWER_DELIVERED_MESSAGE + ) { currentUim.content = injectKiroThinkingTags(currentUim.content, parsed); } diff --git a/tests/kiro-adapter.test.ts b/tests/kiro-adapter.test.ts index c08bd7e7173..8b5fce700b4 100644 --- a/tests/kiro-adapter.test.ts +++ b/tests/kiro-adapter.test.ts @@ -4,7 +4,14 @@ import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { createKiroAdapter } from "../src/adapters/kiro"; -import { KIRO_TOOL_RESULT_CARRIER_MESSAGE } from "../src/adapters/kiro-constants"; +import { + KIRO_ANSWER_DELIVERED_MESSAGE, + KIRO_COMPLETION_RETRY_MESSAGE, + KIRO_CONTINUATION_MESSAGE, + KIRO_EMPTY_TOOL_RESULT_MESSAGE, + KIRO_TOOL_RESULT_CARRIER_MESSAGE, +} from "../src/adapters/kiro-constants"; +import { EMPTY_EXEC_OUTPUT_MESSAGE } from "../src/adapters/exec-tool-result-normalize"; import { MAX_KIRO_TOOL_CATALOG_BYTES, MAX_KIRO_TOOL_COUNT } from "../src/adapters/kiro-tools"; import { applyProviderConfigHints, buildCatalogEntries } from "../src/codex/catalog"; import { getValidAccessTokenSnapshot } from "../src/oauth"; @@ -309,6 +316,92 @@ describe("kiro adapter — buildRequest", () => { expect(current.userInputMessageContext.toolResults[0].content[0].text.trim()).not.toBe(""); }); + // An empty code-mode exec result must say WHY it is empty. Without this the model reads a blank + // result, concludes earlier context was lost, and restarts finished work. + test("an empty code-mode exec result carries the actionable reason, not the generic fallback", async () => { + const execTool = { name: "exec", description: "Run JavaScript", parameters: { type: "object" } }; + for (const raw of ["", "Script completed\nWall time 0.1 seconds\nOutput:\n", ""]) { + const messages = [ + { role: "user", content: "run it" }, + { role: "assistant", content: [{ type: "toolCall", id: "call-x", name: "exec", arguments: {} }] }, + { role: "toolResult", toolCallId: "call-x", toolName: "exec", content: raw, isError: false }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest(parsedWith(messages, [execTool])); + const resultText = JSON.parse(body).conversationState.currentMessage.userInputMessage + .userInputMessageContext.toolResults[0].content[0].text; + + expect(resultText).toBe(EMPTY_EXEC_OUTPUT_MESSAGE); + // The generic fallback would leave the model to guess; assert it is NOT what shipped. + expect(resultText).not.toBe(KIRO_EMPTY_TOOL_RESULT_MESSAGE); + } + }); + + test("real exec output and empty non-exec results are left alone", async () => { + const execTool = { name: "exec", description: "Run JavaScript", parameters: { type: "object" } }; + const withExecOutput = [ + { role: "user", content: "run it" }, + { role: "assistant", content: [{ type: "toolCall", id: "call-x", name: "exec", arguments: {} }] }, + { role: "toolResult", toolCallId: "call-x", toolName: "exec", content: "Output:\nhello", isError: false }, + ]; + const execBody = await createKiroAdapter(provider).buildRequest(parsedWith(withExecOutput, [execTool])); + expect(JSON.parse(execBody.body).conversationState.currentMessage.userInputMessage + .userInputMessageContext.toolResults[0].content[0].text).toBe("Output:\nhello"); + + // A non-exec tool keeps the generic message: asserting code-mode semantics for arbitrary + // tools would tell the model to call text()/notify() in a runtime that has neither. + const nonExec = [ + { role: "user", content: "run it" }, + { role: "assistant", content: [{ type: "toolCall", id: "call-y", name: "bash", arguments: {} }] }, + { role: "toolResult", toolCallId: "call-y", toolName: "bash", content: "", isError: false }, + ]; + const bashBody = await createKiroAdapter(provider).buildRequest(parsedWith(nonExec, [bashTool])); + expect(JSON.parse(bashBody.body).conversationState.currentMessage.userInputMessage + .userInputMessageContext.toolResults[0].content[0].text).toBe(KIRO_EMPTY_TOOL_RESULT_MESSAGE); + }); + + // A delivered final answer already ended its turn. Asking it to continue reopens closed work, + // which is what made a finished task behave like a still-open goal. + test("a delivered final answer is not told to continue or to complete again", async () => { + const messages = [ + { role: "user", content: "do it" }, + { role: "assistant", phase: "final_answer", content: [{ type: "text", text: "Done: the answer." }] }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest(parsedWith(messages, [bashTool])); + const current = JSON.parse(body).conversationState.currentMessage.userInputMessage; + + expect(current.content).toBe(KIRO_ANSWER_DELIVERED_MESSAGE); + expect(current.content).not.toContain(KIRO_CONTINUATION_MESSAGE); + expect(current.content).not.toContain(KIRO_COMPLETION_RETRY_MESSAGE); + }); + + test("an unfinished trailing assistant turn still gets the continuation prompt", async () => { + // Same shape minus `phase`: proves the new branch keys off the delivered final answer and did + // not simply disable continuation for every trailing assistant turn. + const messages = [ + { role: "user", content: "do it" }, + { role: "assistant", content: [{ type: "text", text: "Working on it..." }] }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest(parsedWith(messages, [bashTool])); + const current = JSON.parse(body).conversationState.currentMessage.userInputMessage; + + expect(current.content).toContain(KIRO_CONTINUATION_MESSAGE); + expect(current.content).not.toBe(KIRO_ANSWER_DELIVERED_MESSAGE); + }); + + test("commentary after a final answer reopens continuation", async () => { + // A merged assistant turn is terminal only if its LAST component was the final answer. + const messages = [ + { role: "user", content: "do it" }, + { role: "assistant", phase: "final_answer", content: [{ type: "text", text: "Done." }] }, + { role: "assistant", content: [{ type: "text", text: "Actually, one more check." }] }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest(parsedWith(messages, [bashTool])); + const current = JSON.parse(body).conversationState.currentMessage.userInputMessage; + + expect(current.content).toContain(KIRO_CONTINUATION_MESSAGE); + expect(current.content).not.toBe(KIRO_ANSWER_DELIVERED_MESSAGE); + }); + test("tool result images are attached to Kiro carrier user messages", async () => { const messages = [ { role: "user", content: "look" }, From 60537f067ef220d9a925a302258d42e29d0a91fe Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 21:26:53 +0900 Subject: [PATCH 2/9] fix(kiro): address review findings on completion suppression and failed exec wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the automated reviewers on cf1a5720c, each verified against the code before changing it. **Suppressing the resume wording was not sufficient (Codex P2).** The neutral acknowledgement stopped the continuation text, but `completionMode` stayed `required`, so the request kept advertising `codex_kiro_final_answer` together with its instructions. The model then either answered again, or replied with ordinary text and tripped `needsFallback`, whose retry payload ends with `KIRO_COMPLETION_RETRY_MESSAGE` and explicitly reopens the finished task. A turn whose history already ends with a delivered final answer now resolves to `disabled`, so the completion contract is not offered at all. Detection reads the parsed messages via `hasTrailingDeliveredFinalAnswer`, since the mode is needed to build the tool catalog before the turn list exists. It mirrors the turn-merge rule: a tool call in that message, or any later user or tool-result message, means work continued. `forcedCompletionMode` still wins, so the fallback retry that passes `text_fallback` is never silently downgraded. **Internal state was inferred from user content (CodeRabbit).** The acknowledgement was recognized by comparing `currentUim.content` to the constant, so a genuine user message quoting that sentence was treated as proxy filler and lost both its thinking-tag injection and its completion retry. The generated turn now carries an `answerDeliveredAck` flag that survives the `currentTurn` pop, and the checks test the flag. **A failed exec wrapper was normalized as an empty success (Codex P2).** `Script failed\nWall time ...\nOutput:\n` matched the success regex, so the only failure signal was replaced with text claiming the cell completed, was not blocked, and should not be retried — reachable through Responses history, where `function_call_output` is parsed with `isError: false`. `Script failed` is out of the shared success regex and gets its own failure guidance. Cursor keeps the broader behavior through a local wrapper arm, because its Computer Use branch marks such results `isError` separately and its suite pins that. Verification: `bun x tsc --noEmit` clean; 203 tests pass across kiro-adapter, kiro-stream, cursor-exec-empty-result, cursor-toolresult-normalize and server-kiro-completion-e2e; privacy:scan, core-lab-boundary and repo-hygiene green. Each of the three new assertions was driven red by reverting its fix individually. --- src/adapters/cursor/tool-result-normalize.ts | 14 ++++- src/adapters/exec-tool-result-normalize.ts | 25 +++++++- src/adapters/kiro.ts | 61 +++++++++++++++--- tests/kiro-adapter.test.ts | 66 +++++++++++++++++++- 4 files changed, 153 insertions(+), 13 deletions(-) diff --git a/src/adapters/cursor/tool-result-normalize.ts b/src/adapters/cursor/tool-result-normalize.ts index 90526e2a80e..3d7ea614f61 100644 --- a/src/adapters/cursor/tool-result-normalize.ts +++ b/src/adapters/cursor/tool-result-normalize.ts @@ -12,9 +12,19 @@ import { EMPTY_EXEC_OUTPUT_MESSAGE, EMPTY_EXEC_OUTPUT_REGEX, + FAILED_EXEC_OUTPUT_REGEX, isCodexExecBridgeTool, } from "../exec-tool-result-normalize"; +/** + * Cursor treats a failed-but-empty wrapper as an empty result too (its Computer Use branch marks + * such results `isError` separately). The shared success regex deliberately excludes + * `Script failed`, so restore that arm here rather than widening the shared one. + */ +function isEmptyOrFailedExecWrapper(text: string): boolean { + return EMPTY_EXEC_OUTPUT_REGEX.test(text) || FAILED_EXEC_OUTPUT_REGEX.test(text); +} + const COMPUTER_USE_TOOL_NAMES = new Set([ "node_repl", "node_repl__js", @@ -76,14 +86,14 @@ export function normalizeCursorToolResultText( ): NormalizedToolResultText { const isError = options.isError === true; const computerUse = isNodeReplOrComputerUseTool(options.toolName, options.toolNamespace); - if (computerUse && EMPTY_EXEC_OUTPUT_REGEX.test(text.trim())) { + if (computerUse && isEmptyOrFailedExecWrapper(text.trim())) { return { text: "[empty output: the tool ran but produced no stdout or return value. Verify application state with get_app_state, or make the script emit output.]", isError: true, changed: true, }; } - if (isCodexExecBridgeTool(options.toolName, options.toolNamespace) && EMPTY_EXEC_OUTPUT_REGEX.test(text.trim())) { + if (isCodexExecBridgeTool(options.toolName, options.toolNamespace) && isEmptyOrFailedExecWrapper(text.trim())) { return { text: EMPTY_EXEC_OUTPUT_MESSAGE, isError: false, diff --git a/src/adapters/exec-tool-result-normalize.ts b/src/adapters/exec-tool-result-normalize.ts index 7a7b093824d..9b719b13f27 100644 --- a/src/adapters/exec-tool-result-normalize.ts +++ b/src/adapters/exec-tool-result-normalize.ts @@ -11,8 +11,23 @@ * Kiro consumes this helper directly. */ -/** Matches exec wrappers whose only payload is an empty-output marker. */ -export const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Script failed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; +/** + * Matches exec wrappers whose only payload is an empty-output marker. + * + * `Script failed` is deliberately NOT in this set. A failed cell with no captured output is still + * a FAILURE, and the success guidance below ("not a blocked tool", "do not re-run") would erase the + * only signal that anything went wrong — reachable through Responses history, where + * `function_call_output` is parsed with `isError: false`. Cursor keeps its own broader regex for + * Computer Use, where a failed wrapper is separately marked `isError`. + */ +export const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; + +/** Wrapper for a cell that FAILED without emitting output: empty, but not a success. */ +export const FAILED_EXEC_OUTPUT_REGEX = /^Script failed[^\n]*\n*(?:Wall time[^\n]*\n*)?(?:Output:\s*)?(?:)?\s*$/; + +/** Guidance for a failed cell whose output was empty: the failure must survive normalization. */ +export const FAILED_EXEC_OUTPUT_MESSAGE = + "[exec failed with no captured output: the cell raised before emitting anything. This is a real failure, not an empty success — inspect the call for a thrown error or syntax problem before retrying.]"; /** * The guidance itself. Worded to close all three wrong conclusions a model draws from a blank @@ -62,5 +77,9 @@ export function normalizeEmptyExecToolResultText( text: string, options: { toolName?: string; toolNamespace?: string } = {}, ): string | undefined { - return isEmptyExecToolResult(text, options) ? EMPTY_EXEC_OUTPUT_MESSAGE : undefined; + if (!isCodexExecBridgeTool(options.toolName, options.toolNamespace)) return undefined; + const trimmed = text.trim(); + // Failure first: a failed wrapper must never be described as an empty success. + if (FAILED_EXEC_OUTPUT_REGEX.test(trimmed)) return FAILED_EXEC_OUTPUT_MESSAGE; + return EMPTY_EXEC_OUTPUT_REGEX.test(trimmed) ? EMPTY_EXEC_OUTPUT_MESSAGE : undefined; } diff --git a/src/adapters/kiro.ts b/src/adapters/kiro.ts index c7ce8d922a0..8dff74e6209 100644 --- a/src/adapters/kiro.ts +++ b/src/adapters/kiro.ts @@ -345,7 +345,19 @@ function validateKiroCapabilities(parsed: OcxParsedRequest): void { } type KiroTurn = - | { kind: "user"; content: string; images: KiroImage[]; toolResults: KiroToolResult[] } + | { + kind: "user"; + content: string; + images: KiroImage[]; + toolResults: KiroToolResult[]; + /** + * True only for the proxy-generated acknowledgement that follows a delivered final answer. + * A flag rather than a content comparison: a real user message may legitimately quote the + * same sentence, and treating that as internal state would strip its thinking tags and + * completion retry. + */ + answerDeliveredAck?: boolean; + } | { kind: "assistant"; content: string; @@ -360,6 +372,27 @@ type KiroTurn = finalAnswer?: boolean; }; +/** + * True when the LAST content-bearing message is an assistant final answer that closed its turn. + * + * Mirrors the turn-merge rule: a tool call in that message, or any later user/tool-result message, + * means work continued, so the turn is no longer terminal. Empty assistant messages are skipped + * rather than treated as continuation, since they carry no visible turn. + */ +function hasTrailingDeliveredFinalAnswer(messages: readonly OcxMessage[]): boolean { + for (let i = messages.length - 1; i >= 0; i--) { + const msg = messages[i]; + if (msg.role !== "assistant") return false; + const aMsg = msg as OcxAssistantMessage; + const hasToolCall = (aMsg.content ?? []).some(part => part.type === "toolCall"); + if (hasToolCall) return false; + const hasText = (aMsg.content ?? []).some(part => part.type === "text" && part.text.trim()); + if (!hasText) continue; + return aMsg.phase === "final_answer"; + } + return false; +} + function appendTurnText(target: string, next: string): string { if (!next) return target; return target ? `${target}\n\n${next}` : next; @@ -467,8 +500,19 @@ export function buildKiroPayload( const registry = createKiroToolNameRegistry(); const toolContext = convertKiroToolContext(parsed, registry); const ordinaryTools = toolContext.tools; + // A turn whose history already ENDS with a delivered final answer has nothing to complete. + // Leaving completion "required" here would keep advertising codex_kiro_final_answer with its + // instructions, so the model answers again, or replies with ordinary text and trips the + // `needsFallback` retry, which ends its payload with KIRO_COMPLETION_RETRY_MESSAGE and reopens + // the finished task. Suppressing the mode is what actually closes that loop; the neutral + // acknowledgement below only stops the resume wording. + // + // Read from parsed messages because `completionMode` is needed to build the tool catalog, which + // happens before the turn list exists. `forcedCompletionMode` still wins: the fallback retry + // passes "text_fallback" explicitly and must not be silently downgraded. + const trailingDeliveredAnswer = hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed)); const completionMode: KiroCompletionMode = forcedCompletionMode - ?? (ordinaryTools.length > 0 ? "required" : "disabled"); + ?? (ordinaryTools.length > 0 && !trailingDeliveredAnswer ? "required" : "disabled"); const kiroTools = completionMode === "disabled" ? ordinaryTools : [...ordinaryTools, kiroCompletionTool()]; @@ -636,6 +680,7 @@ export function buildKiroPayload( content: trailing.finalAnswer ? KIRO_ANSWER_DELIVERED_MESSAGE : resumeText, images: [], toolResults: [], + ...(trailing.finalAnswer ? { answerDeliveredAck: true } : {}), }); } @@ -651,6 +696,8 @@ export function buildKiroPayload( const currentTurn = turns.pop(); if (!currentTurn || currentTurn.kind !== "user") throw new Error("Kiro request must end with a user turn"); + // Survives the pop as state, so the checks below never infer intent from user-supplied text. + const answerDeliveredAck = currentTurn.answerDeliveredAck === true; const toEntry = (turn: KiroTurn): KiroHistoryEntry => turn.kind === "assistant" ? { assistantResponseMessage: { @@ -681,16 +728,16 @@ export function buildKiroPayload( currentUim.userInputMessageContext = { ...(currentUim.userInputMessageContext ?? {}), tools: kiroTools }; } if (completionMode === "text_fallback") { - // Never append the retry instruction onto the answer-delivered placeholder: that placeholder - // exists precisely to avoid asking a finished turn for another completion call, and appending - // here would reinstate the loop the placeholder prevents. - if (currentUim.content !== KIRO_COMPLETION_RETRY_MESSAGE && currentUim.content !== KIRO_ANSWER_DELIVERED_MESSAGE) { + // Never append the retry instruction onto the answer-delivered acknowledgement: it exists + // precisely to avoid asking a finished turn for another completion call, and appending here + // would reinstate the loop it prevents. + if (currentUim.content !== KIRO_COMPLETION_RETRY_MESSAGE && !answerDeliveredAck) { currentUim.content = appendTurnText(currentUim.content, KIRO_COMPLETION_RETRY_MESSAGE); } } else if ( !currentUim.userInputMessageContext?.toolResults && currentUim.content !== KIRO_CONTINUATION_MESSAGE - && currentUim.content !== KIRO_ANSWER_DELIVERED_MESSAGE + && !answerDeliveredAck ) { currentUim.content = injectKiroThinkingTags(currentUim.content, parsed); } diff --git a/tests/kiro-adapter.test.ts b/tests/kiro-adapter.test.ts index 8b5fce700b4..b0d66ceb6bc 100644 --- a/tests/kiro-adapter.test.ts +++ b/tests/kiro-adapter.test.ts @@ -7,11 +7,12 @@ import { createKiroAdapter } from "../src/adapters/kiro"; import { KIRO_ANSWER_DELIVERED_MESSAGE, KIRO_COMPLETION_RETRY_MESSAGE, + KIRO_COMPLETION_TOOL_NAME, KIRO_CONTINUATION_MESSAGE, KIRO_EMPTY_TOOL_RESULT_MESSAGE, KIRO_TOOL_RESULT_CARRIER_MESSAGE, } from "../src/adapters/kiro-constants"; -import { EMPTY_EXEC_OUTPUT_MESSAGE } from "../src/adapters/exec-tool-result-normalize"; +import { EMPTY_EXEC_OUTPUT_MESSAGE, FAILED_EXEC_OUTPUT_MESSAGE } from "../src/adapters/exec-tool-result-normalize"; import { MAX_KIRO_TOOL_CATALOG_BYTES, MAX_KIRO_TOOL_COUNT } from "../src/adapters/kiro-tools"; import { applyProviderConfigHints, buildCatalogEntries } from "../src/codex/catalog"; import { getValidAccessTokenSnapshot } from "../src/oauth"; @@ -337,6 +338,21 @@ describe("kiro adapter — buildRequest", () => { }); test("real exec output and empty non-exec results are left alone", async () => { + // Review finding (Codex P2): a failed cell with no output is empty but NOT a success. The + // success guidance would erase the only failure signal — reachable via Responses history, + // where function_call_output is parsed with isError: false. + const execTool0 = { name: "exec", description: "Run JavaScript", parameters: { type: "object" } }; + const failed = [ + { role: "user", content: "run it" }, + { role: "assistant", content: [{ type: "toolCall", id: "call-f", name: "exec", arguments: {} }] }, + { role: "toolResult", toolCallId: "call-f", toolName: "exec", content: "Script failed\nWall time 0.1 seconds\nOutput:\n", isError: false }, + ]; + const failedBody = await createKiroAdapter(provider).buildRequest(parsedWith(failed, [execTool0])); + const failedText = JSON.parse(failedBody.body).conversationState.currentMessage.userInputMessage + .userInputMessageContext.toolResults[0].content[0].text; + expect(failedText).toBe(FAILED_EXEC_OUTPUT_MESSAGE); + expect(failedText).not.toBe(EMPTY_EXEC_OUTPUT_MESSAGE); + const execTool = { name: "exec", description: "Run JavaScript", parameters: { type: "object" } }; const withExecOutput = [ { role: "user", content: "run it" }, @@ -388,6 +404,54 @@ describe("kiro adapter — buildRequest", () => { expect(current.content).not.toBe(KIRO_ANSWER_DELIVERED_MESSAGE); }); + // Review finding (Codex P2): suppressing the resume wording is not enough. While completion + // stays "required" the request keeps advertising the completion tool, so the model answers again + // or trips the text_fallback retry, which reopens the finished task. + test("a delivered final answer stops advertising the completion tool", async () => { + const delivered = [ + { role: "user", content: "do it" }, + { role: "assistant", phase: "final_answer", content: [{ type: "text", text: "Done." }] }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest(parsedWith(delivered, [bashTool])); + const current = JSON.parse(body).conversationState.currentMessage.userInputMessage; + const toolNames = (current.userInputMessageContext?.tools ?? []) + .map((t: { toolSpecification?: { name?: string } }) => t.toolSpecification?.name); + + expect(toolNames).toContain("bash"); + expect(toolNames).not.toContain(KIRO_COMPLETION_TOOL_NAME); + // The instructions must go too: they tell the model to call a tool that is no longer offered. + expect(current.content).not.toContain(KIRO_COMPLETION_TOOL_NAME); + + // Control: an unfinished turn still gets the completion contract. + const unfinished = [ + { role: "user", content: "do it" }, + { role: "assistant", content: [{ type: "text", text: "Working..." }] }, + ]; + const open = await createKiroAdapter(provider).buildRequest(parsedWith(unfinished, [bashTool])); + const openNames = (JSON.parse(open.body).conversationState.currentMessage.userInputMessage + .userInputMessageContext?.tools ?? []) + .map((t: { toolSpecification?: { name?: string } }) => t.toolSpecification?.name); + expect(openNames).toContain(KIRO_COMPLETION_TOOL_NAME); + }); + + // Review finding (CodeRabbit): the acknowledgement was detected by comparing user content, so a + // real user message quoting that sentence lost its thinking tags and completion retry. + test("a user message quoting the acknowledgement is still treated as user content", async () => { + const messages = [ + { role: "user", content: "do it" }, + { role: "assistant", content: [{ type: "text", text: "ok" }] }, + { role: "user", content: KIRO_ANSWER_DELIVERED_MESSAGE }, + ]; + const { body } = await createKiroAdapter(provider).buildRequest({ + ...parsedWith(messages, [bashTool]), + options: { reasoning: "xhigh" }, + } as never); + const current = JSON.parse(body).conversationState.currentMessage.userInputMessage; + + // Real user text keeps its reasoning injection; internal state must not be inferred from it. + expect(current.content).toContain(""); + }); + test("commentary after a final answer reopens continuation", async () => { // A merged assistant turn is terminal only if its LAST component was the final answer. const messages = [ From 761cb4cfef21eaf59c336c960b4402c4490d85b2 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 21:37:10 +0900 Subject: [PATCH 3/9] fix(cursor): keep failure guidance for a failed exec wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review finding (CodeRabbit) on 60537f067, and a regression I introduced in the previous commit rather than a pre-existing one. Routing Cursor's Codex-exec branch through `isEmptyOrFailedExecWrapper` restored the old matching, but the branch still returned `EMPTY_EXEC_OUTPUT_MESSAGE` unconditionally. A `Script failed` wrapper therefore reported that execution succeeded with empty output, erasing the only failure signal — the same defect just fixed on the Kiro path, reintroduced one layer up. It classifies the text by wrapper now; Cursor's `isError` policy is untouched and still owned by the Computer Use branch above it. The re-posted Codex P2 about `completionMode` staying `required` was checked against current code and is already addressed at `kiro.ts:513-515`: a trailing delivered answer resolves the mode to `disabled`, so the completion tool and its instructions are never advertised and the `needsFallback` retry path cannot be entered. The comment it cites sits at the acknowledgement, which is the second half of that fix. Verification: `bun x tsc --noEmit` clean; 204 tests pass across the same five suites. The new Cursor assertion was driven red by reverting the fix. --- src/adapters/cursor/tool-result-normalize.ts | 6 +++++- tests/cursor-exec-empty-result.test.ts | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/adapters/cursor/tool-result-normalize.ts b/src/adapters/cursor/tool-result-normalize.ts index 3d7ea614f61..997ef56fea4 100644 --- a/src/adapters/cursor/tool-result-normalize.ts +++ b/src/adapters/cursor/tool-result-normalize.ts @@ -12,6 +12,7 @@ import { EMPTY_EXEC_OUTPUT_MESSAGE, EMPTY_EXEC_OUTPUT_REGEX, + FAILED_EXEC_OUTPUT_MESSAGE, FAILED_EXEC_OUTPUT_REGEX, isCodexExecBridgeTool, } from "../exec-tool-result-normalize"; @@ -95,7 +96,10 @@ export function normalizeCursorToolResultText( } if (isCodexExecBridgeTool(options.toolName, options.toolNamespace) && isEmptyOrFailedExecWrapper(text.trim())) { return { - text: EMPTY_EXEC_OUTPUT_MESSAGE, + // A `Script failed` wrapper is empty but NOT a success: reporting it as an empty success + // would erase the only failure signal. Text classification stays separate from Cursor's + // isError policy, which the Computer Use branch above owns. + text: FAILED_EXEC_OUTPUT_REGEX.test(text.trim()) ? FAILED_EXEC_OUTPUT_MESSAGE : EMPTY_EXEC_OUTPUT_MESSAGE, isError: false, changed: true, }; diff --git a/tests/cursor-exec-empty-result.test.ts b/tests/cursor-exec-empty-result.test.ts index 096aa01f9cd..098aaa08d38 100644 --- a/tests/cursor-exec-empty-result.test.ts +++ b/tests/cursor-exec-empty-result.test.ts @@ -29,6 +29,16 @@ describe("codex exec bridge empty-result normalization (devlog 260826 gap-7)", ( } }); + // A failed wrapper is empty but not a success: reporting it as an empty success would erase the + // only failure signal. Reachable with isError: false through Responses history. + test("a failed exec wrapper keeps failure guidance, not empty-success text", () => { + const out = normalizeCursorToolResultText("Script failed\nWall time 0.1 seconds\nOutput:\n", { toolName: "exec", isError: false }); + expect(out.changed).toBe(true); + expect(out.text).toContain("exec failed"); + expect(out.text).not.toContain("NOT lost context"); + expect(out.text).not.toContain("Do not re-run"); + }); + test("non-empty exec output passes through byte-identical", () => { const out = normalizeCursorToolResultText("Output:\nhello", { toolName: "exec" }); expect(out.changed).toBe(false); From b557a814003ad491c8b8c3159f6e94612330f1f0 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 21:55:21 +0900 Subject: [PATCH 4/9] fix(kiro): answer a delivered final answer locally instead of asking Kiro again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A turn whose replayed history already ENDS with a delivered final answer had nothing to ask upstream, but the adapter asked anyway. `buildKiroPayload` appends a trailing user turn because the protocol requires one, and cf1a5720c made that turn a neutral acknowledgement rather than a resume instruction. What it could not remove is the inference itself: a neutral prompt is still a prompt, so the model answered the closed task again and a finished turn kept behaving like a still-open goal. 60537f067 additionally stopped advertising the completion tool for that shape, which narrowed the loop without ending it. The boundary is now that no request happens at all. `ProviderAdapter` gains an optional `localTerminal` hook, consulted in `handleResponsesInner` after adapter resolution and BEFORE the request is built: no build (so no token estimate), no send (so `sendCount` stays 0), and the turn completes from a locally constructed terminal. Placement is load-bearing in three ways, each of which ruled out a simpler spot: `buildRequest` cannot express it — the contract returns an `AdapterRequest`, and events only exist once a `Response` reaches `parseStream`. Faking a `Response` inside `fetchResponse` is worse: it records a physical send. An outputless `done` from the ordinary event path cannot express it either. `guardEmptyCompletionEventStream` treats a terminal with no content event as a failed turn, suppresses it, and re-invokes the identical request; a second empty terminal becomes `empty_completion_retry_failed`. Routing the fix through that path would have re-sent the very request it exists to prevent — which is why the regression runs with `emptyCompletionRetry` both enabled and disabled rather than only in the default configuration. `runTurn` was considered and rejected: it routes every Kiro request into the custom transport branch, which waits on provider pacing and increments `sendCount` before any local decision, still meets the empty-completion guard afterwards, and would force the adapter to re-own transport, retries, failover, cancellation, and accounting it already gets from the standard path. The predicate is the existing `hasTrailingDeliveredFinalAnswer`, so there is one notion of "delivered": it keys off role and `phase`, which is why a user message merely QUOTING the acknowledgement is unaffected, and it returns false at the first non-assistant message, so a genuine follow-up still reaches Kiro. The adapter-owned bounded retry is untouched because it builds with a forced `text_fallback` mode and never passes through this hook. `localTerminalReason` is recorded on the request log so a zero-send request is explainable rather than looking like a lost one. It is a fixed identifier, never conversation-derived. Scope: this fixes the repeated inference only. The user-visible duplicate answer — ordinary text released as `commentary` and the completion answer as `final_answer` in the SAME inference, split into two messages by the bridge — is a separate mechanism and is planned as wp2 in devlog/_plan/260828_kiro_turn_termination/020. Verification: 214 tests pass across kiro-adapter, kiro-stream, server-kiro-completion-e2e, core-lab-boundary and repo-hygiene; 113 more across the empty-completion and request-log suites; `bun x tsc --noEmit` clean; `privacy:scan` green. All four new short-circuit assertions were driven red by reverting the hook call, so none is vacuous. Full-suite validation was skipped at the user's explicit instruction. --- .../000_research.md | 63 +++++++++ .../010_wp1_terminal_boundary.md | 81 ++++++++++++ .../011_audit_round1.md | 25 ++++ .../012_audit_round2.md | 55 ++++++++ .../020_wp2_duplicate_answer.md | 124 ++++++++++++++++++ src/adapters/base.ts | 26 ++++ src/adapters/kiro.ts | 18 +++ src/server/request-log.ts | 7 + src/server/responses/core.ts | 48 +++++++ tests/server-kiro-completion-e2e.test.ts | 97 ++++++++++++++ 10 files changed, 544 insertions(+) create mode 100644 devlog/_plan/260828_kiro_turn_termination/000_research.md create mode 100644 devlog/_plan/260828_kiro_turn_termination/010_wp1_terminal_boundary.md create mode 100644 devlog/_plan/260828_kiro_turn_termination/011_audit_round1.md create mode 100644 devlog/_plan/260828_kiro_turn_termination/012_audit_round2.md create mode 100644 devlog/_plan/260828_kiro_turn_termination/020_wp2_duplicate_answer.md diff --git a/devlog/_plan/260828_kiro_turn_termination/000_research.md b/devlog/_plan/260828_kiro_turn_termination/000_research.md new file mode 100644 index 00000000000..5bc66b1c671 --- /dev/null +++ b/devlog/_plan/260828_kiro_turn_termination/000_research.md @@ -0,0 +1,63 @@ +# Kiro turn termination — residual defect research + +Observed 2026-08-28 21:20 KST by the user in a Codex desktop session routed +`kiro/claude-opus-5` through the local proxy on port 10100. + +## Symptom + +1. A plain question ("근데 코드 모드가 뭐임") produced the final answer TWICE in + one turn, the second a near-duplicate rewrite of the first. +2. The user reports the "answer finishes, then continues like a goal" loop is + still present after `cf1a5720c`. + +## Live-state evidence + +- Listener PID 3653, started 2026-08-28 21:17:50, running the checkout at + `/Users/jun/Developer/new/700_projects/opencodex/src/cli/index.ts start --port 10100`. +- `cf1a5720c` committed 21:09:50 — the running process DID load that fix. + Confirmed independently: a probe that produced no stdout in this session + returned the new empty-exec wording added by that commit. +- HEAD advanced to `60537f067` at 21:26:53 (a different session's commit), so + the running proxy is stale relative to HEAD but not relative to `cf1a5720c`. + +So the residual behaviour is a real defect, not a stale process. + +## Mechanism 1 — duplicate final answer (rendering) + +Kiro emits answer-like ordinary text, then calls the private completion tool in +the SAME inference. The adapter releases the prose as `phase: "commentary"` and +the completion `answer` as `phase: "final_answer"`. `src/bridge.ts` closes the +commentary message on the phase change and opens a new assistant message, so the +client renders two assistant messages whose text is nearly identical. + +This is pinned by the existing suite, so it is verified behaviour rather than a +hypothesis — `tests/kiro-stream.test.ts` asserts exactly: + +``` +{ type: "text_delta", text: "Done.", phase: "commentary" }, +{ type: "text_delta", text: "Done.", phase: "final_answer" }, +``` + +## Mechanism 2 — non-terminating turn (upstream fetch) + +When replayed history ends in a delivered final answer, `buildKiroPayload` still +appends a synthetic trailing user turn carrying `KIRO_ANSWER_DELIVERED_MESSAGE` +and performs a real upstream inference. Neutral wording removes the instruction +to resume but does not remove the prompt: the model is asked again and answers +again. `60537f067` additionally suppresses the completion contract for that +shape, which narrows the loop, but there is still no terminal boundary that +avoids the fetch. + +## Hypotheses tested + +| id | claim | verdict | evidence | +|----|-------|---------|----------| +| H1 | the completion TOOL CALL never sets the phase flag | refuted | the proxy consumes the completion tool; a replayed history containing it throws at `src/adapters/kiro-wire.ts:88` (reproduced directly) | +| H2 | any synthetic trailing user turn re-invokes the model | confirmed | `src/adapters/kiro.ts` trailing-turn append still yields an upstream inference | +| H3 | another layer replays the answer | partial | the generic Responses guard is not involved; the Kiro-owned bounded completion fallback does perform a second fetch | +| H4 | commentary is the last recorded component | confirmed | ordinary text is forced to commentary; the completion answer is a separate final message, split at `src/bridge.ts:922` | + +## Verification baseline + +`bun test tests/kiro-adapter.test.ts tests/kiro-stream.test.ts tests/server-kiro-completion-e2e.test.ts` +-> 180 pass, 0 fail at `60537f067`. diff --git a/devlog/_plan/260828_kiro_turn_termination/010_wp1_terminal_boundary.md b/devlog/_plan/260828_kiro_turn_termination/010_wp1_terminal_boundary.md new file mode 100644 index 00000000000..67982ae7ddb --- /dev/null +++ b/devlog/_plan/260828_kiro_turn_termination/010_wp1_terminal_boundary.md @@ -0,0 +1,81 @@ +# wp1 — terminal boundary for a delivered final answer + +Consumes: `000_research.md` mechanism 2. + +## Problem + +`buildKiroPayload` (`src/adapters/kiro.ts`) turns a trailing delivered final +answer into a synthetic user turn carrying `KIRO_ANSWER_DELIVERED_MESSAGE` and +then performs a real upstream inference. Neutral wording is still a prompt, so +the model answers again — the closed task reads as an open goal. + +`60537f067` suppresses the completion contract for that shape. Keep that as +defence in depth; it is not the boundary. + +## Change (revised after the A-phase audit — audit verdict was FAIL on the +## original placement, see 011_audit_round1.md) + +Short-circuit BEFORE the provider fetch, but NOT inside `buildRequest` and NOT +as a bare outputless `done`. Three constraints the audit established, each +verified against current source: + +1. **An outputless `done` is retried, not accepted.** `guardEmptyCompletionEventStream` + treats a `done` with no content event as an empty completion, suppresses the + terminal, and re-invokes the identical turn; a second empty terminal becomes + `empty_completion_retry_failed` + (`src/server/responses/empty-completion-guard.ts:246-270`). So the naive + terminal turns one loop into either another inference or a stated error. + The local terminal must therefore bypass the empty-completion guard as well as + the transport. +2. **`buildRequest` cannot emit events.** The adapter contract returns an + `AdapterRequest`; events only exist once a `Response` reaches `parseStream` + (`src/adapters/base.ts`). The server then records and sends the attempt + unconditionally. Manufacturing a fake `Response` inside `fetchResponse` is + also wrong: it records a physical send and still meets the guard. +3. **A phantom estimate must not be logged.** Kiro attaches an estimated input + count during build and the server notes the attempt send before fetching, so + short-circuiting after a build would log a request that never happened. + +Placement: an explicit adapter-owned local-terminal decision consulted in +`handleResponsesInner` AFTER adapter resolution and BEFORE the ordinary +build/send path, short-circuiting to a locally constructed terminal response. +Reuse `hasTrailingDeliveredFinalAnswer` as the predicate; do not introduce a +second notion of "delivered". The hook must not intercept the adapter-owned +bounded retry, which builds with a forced `text_fallback` mode. + +Usage accounting for the local terminal: no build-time estimate, `sendCount` +zero, response usage explicitly zero for input/output/total, and no estimated +usage in the request log. + +## Out of scope + +- Any change to how the completion tool is parsed or consumed. +- Any change to the empty-exec normalisation from `cf1a5720c` / `60537f067`. +- The duplicate-rendering defect, which is wp2. + +## Criteria + +1. Replaying a delivered final answer issues ZERO upstream requests and yields a + completed turn with `endTurn: true`. +2. Criterion 1 holds with `emptyCompletionRetry` BOTH enabled and disabled, for + streaming and non-streaming Responses. This is the criterion the audit added; + without it the fix passes a test and still loops in the user's config. +3. The short-circuited turn logs `sendCount === 0` and no estimated usage. +4. A genuine later user message after a delivered final answer still performs a + normal inference (control). +5. An unfinished trailing assistant turn still gets the continuation prompt and + still performs an inference (control, already covered — must stay green). +6. The adapter-owned bounded `text_fallback` retry is NOT intercepted. +7. `60537f067`'s completion-mode suppression remains asserted. + +## Completion language + +Closing wp1 fixes the repeated inference ONLY. The user-visible duplicate answer +remains until wp2 lands, and the wp1 report must say so rather than implying the +reported symptom is fully resolved. + +## Evidence + +`bun test tests/kiro-adapter.test.ts tests/kiro-stream.test.ts tests/server-kiro-completion-e2e.test.ts` +plus new public-server coverage in `tests/server-kiro-completion-e2e.test.ts`, +each new assertion driven red once by reverting the change. diff --git a/devlog/_plan/260828_kiro_turn_termination/011_audit_round1.md b/devlog/_plan/260828_kiro_turn_termination/011_audit_round1.md new file mode 100644 index 00000000000..0c9e75d062e --- /dev/null +++ b/devlog/_plan/260828_kiro_turn_termination/011_audit_round1.md @@ -0,0 +1,25 @@ +# A-phase audit round 1 — verdict FAIL + +Reviewer: independent subagent (gpt-5.6-sol, medium effort), read-only lane. +Audited: `000_research.md`, `010_wp1_terminal_boundary.md` as first written. +Reviewer's own checks: 180 pass / 0 fail on the three Kiro suites, +`bun x tsc --noEmit` exit 0, live proxy untouched. + +The plan was rewritten rather than argued with. Findings and dispositions: + +| # | Finding | Disposition | +|---|---------|-------------| +| 1 | An outputless `done` is consumed by the empty-completion guard, which suppresses the terminal and re-invokes the identical turn (`src/server/responses/empty-completion-guard.ts:246-270`). The "safe terminal" becomes another inference or `empty_completion_retry_failed`. | ACCEPTED. Verified independently by reading the guard. wp1 now requires bypassing the guard as well as the transport, and adds a criterion covering `emptyCompletionRetry` both ON and OFF. | +| 2 | `buildRequest` cannot emit events under the adapter contract, and faking a `Response` in `fetchResponse` still records a physical send. | ACCEPTED. Placement moved to an adapter-owned local-terminal decision consulted in `handleResponsesInner` before the build/send path. | +| 3 | Short-circuiting after a build logs a phantom estimated request. | ACCEPTED. Criteria now demand no build-time estimate, `sendCount === 0`, zero response usage, and no estimated usage in the request log. | +| 4 | `hasTrailingDeliveredFinalAnswer` is sound, but the hook must not intercept the forced `text_fallback` build. | ACCEPTED as a criterion. The predicate was re-read directly: role-and-phase based, so a user message merely QUOTING the acknowledgement is unaffected. | +| 5 | wp1/wp2 separation is legitimate, but wp1's completion language must not imply the reported symptom is fully fixed. | ACCEPTED. wp1 now carries an explicit completion-language section. | +| 6 | The broad "buffer commentary" direction is wrong; required-mode commentary is ALREADY deferred, and the real defect is the unconditional flush before the validated answer. Retaining across the bounded fallback would hide progress during a long second inference. | ACCEPTED, and it improves the design: wp2 is now a change to WHEN the existing deferred run is released, not a new buffer. | +| 7 | No user-visible Responses-level regression was specified; adapter-event coverage cannot prove the rendered duplicate is gone. | ACCEPTED. Both work-phases now name `tests/server-kiro-completion-e2e.test.ts`. | + +Process note the reviewer raised: it expected a staged diff and found none, and +observed HEAD had moved to `761cb4cfe`. Correct on both counts — the plan unit is +untracked while in P, and two unrelated commits landed from another session +during the audit. Neither invalidates the substance, and the untracked worktree +changes in `src/adapters/cursor/` at dispatch time belonged to that other session +and were left alone. diff --git a/devlog/_plan/260828_kiro_turn_termination/012_audit_round2.md b/devlog/_plan/260828_kiro_turn_termination/012_audit_round2.md new file mode 100644 index 00000000000..800865a3d21 --- /dev/null +++ b/devlog/_plan/260828_kiro_turn_termination/012_audit_round2.md @@ -0,0 +1,55 @@ +# A-phase audit round 2 — verdict FAIL on wp2, wp1 cleared + +Same reviewer as round 1 (blocker-closure reuse). Round 1's seven findings were +all accepted; this round re-read the revised plan. + +## wp1 — cleared + +The reviewer confirms the revised placement, criteria, and completion language +are sufficient: bypassing transport, request building, and the empty-completion +guard, with guard-on/guard-off and streaming/non-streaming coverage, zero sends, +no estimated usage, and the forced `text_fallback` exclusion. + +It also rejected `runTurn` as the seam, with specifics worth keeping: adopting +`runTurn` routes EVERY Kiro request into the custom transport branch, which waits +on provider pacing and increments `sendCount` before any local decision, still +meets the empty-completion guard afterwards, and would force Kiro to re-own +transport, retries, failover, cancellation, and accounting that its existing +`buildRequest`/`fetchResponse`/`parseStream` path already provides. The local +terminal therefore belongs immediately after adapter resolution and before +`buildRequest`. + +## wp2 — two execution-path blockers, both accepted + +1. **The outer drain.** Skipping the inner flush at `src/adapters/kiro.ts:1467` + is not enough: `parseKiroAttempt` independently drains `deferred` at + `src/adapters/kiro.ts:996-999` after the inner generator returns. The final + answer would be emitted first and the commentary after it — the duplicate + survives, reversed. Found independently while reading the same file, so this + is confirmed twice. The deferred collection must be consumed, and the claim + that this stays clear of retention machinery is withdrawn. +2. **`text_fallback` has the same shape through a different collection.** It + retains in `fallbackEvents`, and `src/adapters/kiro.ts:1470-1477` emits all of + them and then the completion answer. The rule must apply independently inside + each inference. + +Criterion 3 was also overbroad — "any turn whose completion never arrives enters +the fallback exactly once" is false for real tools, provider/protocol failures, +and explicit stops like `MAX_TOKENS`. Narrowed to a clean required-mode +inference, with added controls for failure, explicit incomplete stop, +text_fallback duplication, and budget return-to-baseline. + +The six release paths the reviewer enumerated from source are now a table in +`020_wp2_duplicate_answer.md` and are treated as controls. + +## HEAD movement + +Verified: `60537f067..761cb4cfe` touches only +`src/adapters/cursor/tool-result-normalize.ts` and +`tests/cursor-exec-empty-result.test.ts`. No Kiro adapter, adapter contract, +Responses core, bridge, or Kiro test file changed. The plan is unaffected. + +## Disposition + +wp1 proceeds to implementation. wp2's plan page is corrected here and will be +re-audited as part of its own cycle rather than blocking wp1. diff --git a/devlog/_plan/260828_kiro_turn_termination/020_wp2_duplicate_answer.md b/devlog/_plan/260828_kiro_turn_termination/020_wp2_duplicate_answer.md new file mode 100644 index 00000000000..39ea47ec273 --- /dev/null +++ b/devlog/_plan/260828_kiro_turn_termination/020_wp2_duplicate_answer.md @@ -0,0 +1,124 @@ +# wp2 — one visible answer per turn + +Consumes: `000_research.md` mechanism 1. + +## Problem + +Kiro emits answer-like ordinary text and then calls the private completion tool +in the SAME inference. The adapter releases the prose as `phase: "commentary"` +and the completion `answer` as `phase: "final_answer"`; +`src/bridge.ts` closes the commentary message on the phase change and opens a +new assistant message. The client renders two assistant messages whose text is +nearly identical. This is what the user saw. + +The existing suite pins this pair, so the fix necessarily REPLACES an asserted +expectation rather than adding to it: + +``` +tests/kiro-stream.test.ts +{ type: "text_delta", text: "Done.", phase: "commentary" }, +{ type: "text_delta", text: "Done.", phase: "final_answer" }, +``` + +## Constraint that shapes the design + +Progress prose is load-bearing UX: a long tool-using turn streams commentary so +the user is not left staring at nothing. Withholding ALL commentary until the +turn resolves would trade a cosmetic duplicate for a silent turn, which the +repository's own comments call out (`#520` gates exist precisely to avoid +re-emitting or losing flushed progress). + +So the buffering must be NARROW: hold back only the trailing commentary run that +has not yet been followed by a real tool call, and only while the completion tool +is still capable of arriving. Release it unchanged the moment a real tool starts, +the stream ends without a completion answer, or the bounded fallback engages. + +## Options + +1. **Consume the same inference's retained text on a valid completion.** (CHOSEN — + narrowed in audit round 1, corrected in round 2.) No new buffer is needed: + required-mode commentary is ALREADY deferred. But skipping the inner flush is + NOT sufficient, and this is the correction that matters: + + - The inner flush is at `src/adapters/kiro.ts:1467`. + - `parseKiroAttempt` INDEPENDENTLY drains whatever remains in `deferred` at + `src/adapters/kiro.ts:996-999`, after the inner generator returns. + + So merely skipping the inner flush emits the final answer first and then the + commentary from the outer drain — the duplicate survives, in reversed order. + The deferred collection must be CONSUMED on a valid completion: discard and + release the redundant `text_delta` events, preserve and release every + non-text event, and leave nothing for the outer drain. That necessarily + touches retention ownership, so the earlier claim that this change stays + clear of the retention machinery is withdrawn. + + `text_fallback` has the SAME shape through a DIFFERENT collection: it retains + in `fallbackEvents`, and `src/adapters/kiro.ts:1470-1477` emits all of them + and then the completion answer. The rule must therefore apply independently + inside EACH inference: + + - required inference + valid completion -> suppress its deferred text, keep + non-text events, emit the completion answer; + - text_fallback inference + valid completion -> suppress that inference's + retained text, keep non-text events, emit the completion answer; + - never retain one inference's progress across the next inference. +2. **Retain commentary ACROSS the bounded fallback.** Rejected by the audit: the + second inference can be long, so withholding first-attempt progress across it + would make the turn look dead and contradicts the deliberate "first attempt + already flushed" gate. +3. **Suppress only on redundancy.** Emit commentary live, and skip the completion + answer if it is substantially the same text. Rejected: "substantially the same" + is a similarity heuristic, and a wrong guess either drops the real answer or + keeps the duplicate. +4. **Bridge-side coalesce.** Merge a commentary message and an immediately + following final answer into one assistant message. Rejected as the primary + seam: the phase distinction is deliberate protocol information, and the same + split is correct when the commentary genuinely preceded tool work. + +Because the deferral already exists, this is a change to WHEN the deferred run is +released, not a new retention mechanism — which also keeps it clear of the +retention/budget machinery where duplication bugs have previously lived. + +## Criteria + +1. A single inference emitting answer-like prose plus a completion answer yields + exactly ONE visible answer to the client. +2. Commentary followed by a REAL tool call is still emitted live and in order. +3. A CLEAN required-mode inference — text or reasoning present, no real tool, no + completion answer, no explicit non-completion stop reason — still shows its + progress prose and enters the bounded fallback exactly once. (Narrowed in + audit round 2: the unqualified form was false for real tools, provider and + protocol failures, and explicit stops such as `MAX_TOKENS` or + `CONTENT_FILTERED`.) +4. A Responses-protocol-level assertion, not only adapter events: the user-visible + duplicate must be proven gone through the bridge. Adapter-event coverage cannot + prove this, because the split happens in the bridge on the phase change. The + assertion belongs in `tests/server-kiro-completion-e2e.test.ts`: one upstream + request, exactly one visible assistant answer, one terminal completion, and the + near-duplicate prose absent. +5. The existing `tests/kiro-stream.test.ts` expectation that asserts the + commentary/final pair is UPDATED, not deleted — the replacement states the new + contract for the same scenario. +6. `text_fallback` ordinary text plus a valid completion also yields exactly one + visible answer. +7. The translator budget returns to baseline after suppressed events — suppression + must release retention, not leak it. + +## Release paths that must remain intact + +Enumerated from source in audit round 2; each is a control the implementation may +not regress: + +| trigger | release point | +|---------|---------------| +| a real tool starts (`sawRealTool`) | `src/adapters/kiro.ts:1172-1177`, released with the tool event | +| clean no-completion turn needing fallback | flush before `needsFallback` returns, `src/adapters/kiro.ts:1535-1542` / `1600-1603` | +| stream / protocol / provider failure | outer failure drain, `src/adapters/kiro.ts:996-999` | +| explicit non-completion stop | released before the incomplete/error branches, `src/adapters/kiro.ts:1545-1598` | +| plain-text fallback without completion | retained text promoted to `final_answer`, `src/adapters/kiro.ts:1488-1501` | +| empty / reasoning-only fallback | diagnostics released before the structured incomplete, `src/adapters/kiro.ts:1503-1517` | + +## Out of scope + +- The terminal-boundary fix (wp1). +- Changing what `phase` means in the Responses protocol. diff --git a/src/adapters/base.ts b/src/adapters/base.ts index 2cd481dfba6..f5a22b29690 100644 --- a/src/adapters/base.ts +++ b/src/adapters/base.ts @@ -37,6 +37,21 @@ export interface ProviderAdapter { */ buildRequest(parsed: OcxParsedRequest, incoming: IncomingMeta): AdapterRequest | Promise; + /** + * Decide, BEFORE any request is built or sent, that this turn has nothing to ask upstream. + * + * Returning a reason short-circuits the turn to a locally constructed completed response: no + * `buildRequest`, no send, no token estimate, and no empty-completion retry. That last part is + * why this cannot be expressed as an outputless `done` from `parseStream`: the empty-completion + * guard treats a terminal with no content as a failed turn and re-invokes the identical request, + * so an adapter that "successfully returned nothing" would be retried into the very loop it was + * trying to end. + * + * Only for turns whose input already contains the answer — see the Kiro adapter, where replayed + * history ending in a delivered final answer has nothing left to complete. + */ + localTerminal?(parsed: OcxParsedRequest): AdapterLocalTerminal | undefined; + fetchResponse?(request: AdapterRequest, ctx?: AdapterFetchContext): Promise; /** @@ -119,3 +134,14 @@ export interface AdapterFetchContext { /** Custom fetch executor to use for physical upstream network requests (defaults to globalThis.fetch). */ executor?: typeof globalThis.fetch; } + +/** + * An adapter's decision that a turn needs no upstream inference at all. + * + * `reason` is diagnostic only. It is never sent to the client and never logged as request + * content: it names the code path for a maintainer reading a request log, so it must stay a + * fixed identifier rather than anything derived from the conversation. + */ +export interface AdapterLocalTerminal { + reason: string; +} diff --git a/src/adapters/kiro.ts b/src/adapters/kiro.ts index 8dff74e6209..2523373041e 100644 --- a/src/adapters/kiro.ts +++ b/src/adapters/kiro.ts @@ -2002,6 +2002,24 @@ export function createKiroAdapter(provider: OcxProviderConfig): ProviderAdapter return { name: "kiro", + // A replayed history that already ENDS with a delivered final answer has nothing to ask Kiro. + // Before this hook the adapter still appended a trailing user turn — a neutral acknowledgement, + // but structurally still a prompt — and performed a real inference, so the model answered the + // closed task again and the finished turn behaved like a still-open goal. + // + // Suppressing the completion contract (above) removed the instruction to complete; it could not + // remove the inference. This is the boundary: no request is built, nothing is sent, and no token + // estimate is recorded. + // + // The forced-fallback build is deliberately NOT consulted here: this hook runs on the inbound + // turn only, and the adapter-owned bounded retry passes "text_fallback" through `build` + // directly, never through this path. + localTerminal(parsed: OcxParsedRequest) { + return hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed)) + ? { reason: "kiro_final_answer_already_delivered" } + : undefined; + }, + async buildRequest(parsed: OcxParsedRequest, incoming) { const built = await build(parsed); modelId = parsed.modelId; diff --git a/src/server/request-log.ts b/src/server/request-log.ts index dc10db78935..4c1779a4d5e 100644 --- a/src/server/request-log.ts +++ b/src/server/request-log.ts @@ -63,6 +63,13 @@ export interface RequestLogContext { * product: widening that enum would merge Responses and Chat Completions, * since both leave it undefined. */ inboundProtocol?: "responses" | "chat" | "messages"; + /** + * Set when an adapter answered the turn locally and no upstream request was made + * (`ProviderAdapter.localTerminal`). A fixed identifier naming the code path, never + * conversation-derived: it exists so a request log showing zero sends is explainable + * rather than looking like a lost request. + */ + localTerminalReason?: string; /** Stable non-PII Codex Pool account identity for durable usage attribution. */ accountLogLabel?: string; requestedModel?: string; diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 75d728f24ee..30226f28755 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -4858,6 +4858,54 @@ async function handleResponsesInner( // reuse is safe, and releaseBodyObservation is idempotent per build. let initialRequest: AdapterRequest | undefined; let inputTokenEstimate: number | undefined; + // An adapter may know the turn needs no inference at all — Kiro's replayed history ending in a + // delivered final answer. Answer it locally: no build (so no token estimate), no send (so + // sendCount stays 0), and crucially no empty-completion guard, which treats an outputless + // terminal as a failed turn and re-invokes the identical request. Routing this through the + // ordinary event path would therefore reinstate the loop it exists to end. + const localTerminal = activeAdapter.localTerminal?.(parsed); + if (localTerminal) { + logCtx.localTerminalReason = localTerminal.reason; + cleanupUpstreamAbort(); + upstream.abort(); + const terminalEvents: AdapterEvent[] = [{ + type: "done", + endTurn: true, + usage: { inputTokens: 0, outputTokens: 0, totalTokens: 0 }, + }]; + if (parsed.stream) { + return new Response( + bridgeToResponsesSSE( + (async function* () { yield* terminalEvents; })(), + parsed._responseModelId ?? parsed.modelId, + toolBridgeMaps.toolNsMap, + toolBridgeMaps.freeformToolNames, + toolBridgeMaps.toolSearchToolNames, + undefined, + 2_000, + { + translatorBudget, + ...(options.forceEmptyResponseId ? { responseId: "" } : {}), + ...(options.onFirstOutput ? { onFirstOutput: options.onFirstOutput } : {}), + }, + ), + { + headers: { + "Content-Type": "text/event-stream", + "Cache-Control": "no-cache", + "Connection": "keep-alive", + "X-Accel-Buffering": "no", + }, + }, + ); + } + return new Response( + JSON.stringify(buildResponseJSON(terminalEvents, parsed._responseModelId ?? parsed.modelId, { + translatorBudget, + })), + { headers: { "Content-Type": "application/json" } }, + ); + } try { initialRequest = await activeAdapter.buildRequest(parsed, { headers: selectedForwardHeaders, translatorBudget }); refreshRoutedNamespaceToolAliases(initialRequest); diff --git a/tests/server-kiro-completion-e2e.test.ts b/tests/server-kiro-completion-e2e.test.ts index 4f8f7196bb0..9a14d4baa64 100644 --- a/tests/server-kiro-completion-e2e.test.ts +++ b/tests/server-kiro-completion-e2e.test.ts @@ -250,4 +250,101 @@ describe("Kiro completion through public server endpoints", () => { upstream.server.stop(true); } }); + + // A turn whose replayed history already ENDS with a delivered final answer has nothing to ask + // Kiro. Before the local terminal, the adapter appended a trailing user turn — neutral wording, + // but structurally still a prompt — and performed a real inference, so the model answered the + // closed task again and a finished turn behaved like a still-open goal. + // + // `emptyCompletionRetry` is exercised BOTH ways on purpose. A local terminal produces no content, + // which is exactly the shape that guard re-invokes: if the short-circuit were routed through the + // ordinary event path, the enabled case would send the request the fix exists to prevent, and a + // guard-off-only test would pass while the user's own config still looped. + for (const emptyCompletionRetry of [false, true]) { + for (const stream of [true, false]) { + test(`a delivered final answer sends nothing upstream (emptyCompletionRetry=${emptyCompletionRetry}, stream=${stream})`, async () => { + const upstream = scriptedKiroUpstream([]); + saveConfig({ ...kiroConfig(upstream.server.url.toString()), emptyCompletionRetry } as OcxConfig); + const proxy = startServer(0); + try { + const response = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + phase: "final_answer", + content: [{ type: "output_text", text: "Code mode runs JavaScript that calls tools." }], + }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + + expect(response.status).toBe(200); + const body = await response.text(); + // The load-bearing assertion: zero upstream requests. The scripted upstream has no + // attempts queued, so any send would also answer 500 and fail the status check above. + expect(upstream.requests).toHaveLength(0); + + if (stream) { + const events = responseEvents(body); + expect(events.filter(event => event.name === "response.completed")).toHaveLength(1); + expect(events.some(event => event.name === "response.output_text.delta")).toBe(false); + // The empty-completion guard's failure event must not appear: a local terminal is a + // deliberate no-inference turn, not an upstream empty completion. + expect(body).not.toContain("empty_completion_retry_failed"); + } else { + const json = JSON.parse(body) as { status?: string; output?: unknown[] }; + expect(json.status).toBe("completed"); + expect(json.output ?? []).toHaveLength(0); + } + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); + } + } + + // Control for the above: the predicate keys off the trailing turn, so a genuine follow-up + // question after a delivered answer is an ordinary turn and MUST still reach Kiro. Without this, + // a short-circuit that swallowed every turn would pass the tests above. + test("a real user follow-up after a delivered final answer still reaches Kiro", async () => { + const upstream = scriptedKiroUpstream([completionFrames("Yes — one JavaScript cell, many tool calls.")]); + saveConfig(kiroConfig(upstream.server.url.toString())); + const proxy = startServer(0); + try { + const response = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + phase: "final_answer", + content: [{ type: "output_text", text: "Code mode runs JavaScript that calls tools." }], + }, + { type: "message", role: "user", content: [{ type: "input_text", text: "so it batches calls?" }] }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + + expect(response.status).toBe(200); + expect(upstream.requests).toHaveLength(1); + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); }); From 7cdb55b4fa724e673cb3c2b65dc9167ee1e61898 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 21:58:37 +0900 Subject: [PATCH 5/9] fix(kiro): track the local-terminal stream's lifetime like every other SSE return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local-terminal short-circuit returned its SSE body raw. Every other streaming return in handleResponsesInner wraps the bridge output in trackStreamLifetime, which is what releases the turn admission lease when the body finishes or the client disconnects. A turn that already holds all of its output would otherwise keep a lease alive until something else reclaimed it — the one resource the no-inference path had no excuse to hold. Found while checking the short-circuit against the neighbouring return sites rather than by a failing test, which is why no assertion changes here: the existing local-terminal regressions still pass, and a lease leak is not observable through them. --- src/server/responses/core.ts | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 30226f28755..4f900364b9a 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -4874,21 +4874,26 @@ async function handleResponsesInner( usage: { inputTokens: 0, outputTokens: 0, totalTokens: 0 }, }]; if (parsed.stream) { + const localSse = bridgeToResponsesSSE( + (async function* () { yield* terminalEvents; })(), + parsed._responseModelId ?? parsed.modelId, + toolBridgeMaps.toolNsMap, + toolBridgeMaps.freeformToolNames, + toolBridgeMaps.toolSearchToolNames, + undefined, + 2_000, + { + translatorBudget, + ...(options.forceEmptyResponseId ? { responseId: "" } : {}), + ...(options.onFirstOutput ? { onFirstOutput: options.onFirstOutput } : {}), + }, + ); + // Same lifetime tracking as every other streaming return in this function: the turn + // admission lease is released when the body finishes or the client disconnects. Returning + // the raw stream would hold a lease for a turn that already has all of its output. + const localTurnAc = new AbortController(); return new Response( - bridgeToResponsesSSE( - (async function* () { yield* terminalEvents; })(), - parsed._responseModelId ?? parsed.modelId, - toolBridgeMaps.toolNsMap, - toolBridgeMaps.freeformToolNames, - toolBridgeMaps.toolSearchToolNames, - undefined, - 2_000, - { - translatorBudget, - ...(options.forceEmptyResponseId ? { responseId: "" } : {}), - ...(options.onFirstOutput ? { onFirstOutput: options.onFirstOutput } : {}), - }, - ), + trackStreamLifetime(localSse, localTurnAc, undefined, options.turnAdmissionLease), { headers: { "Content-Type": "text/event-stream", From 7781a022fb534774e89394298875291b0f3c239b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 22:05:53 +0900 Subject: [PATCH 6/9] fix(kiro): log a locally answered turn as exact zero usage, not an estimate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The C-phase verifier failed wp1 criterion 3 on evidence, and it was right. The local terminal constructed explicit zero usage, but `usageForFinalLog` marks all Kiro and Cursor usage estimated provider-wide, so the row came out as `{inputTokens: 0, outputTokens: 0, estimated: true}` with `usageStatus: "estimated"`. Zero counts from a turn that made no inference are exact. A row claiming otherwise is indistinguishable from a real turn whose usage frame never arrived — which is the wrong signal on the one path whose whole point is that nothing was sent. The provider-wide marking exists because those adapters can only guess a real inference's usage. There is nothing to guess when there was no inference, so `usageForFinalLog` takes an explicit `locallyAnswered` argument rather than inferring it from the numbers: a genuine turn that happened to consume zero tokens is still an estimate, and a locally answered turn whose adapter DID estimate something still says so. It defaults false, so no existing caller changes behaviour. The second finding is fixed too: `localTerminalReason` lived only on the internal context and never reached `RequestLogEntry`, so the field could not actually make a zero-send row explainable as its own comment claimed. It is now copied into the entry, sanitized like the other metadata strings. Both gaps existed because the committed regressions asserted behaviour — nothing was sent — and never read the resulting log. `tests/server-kiro-completion-e2e.test.ts` now inspects the request-log row for both response modes: the reason field, `usageStatus: "reported"`, absent `estimated`, zero counts, `sendCount === 0`, and no `inputTokenEstimate`. Reverting the `locallyAnswered` argument turns exactly those two red. Verification: 363 tests pass across the kiro, usage-log, request-log, empty-completion, core-lab-boundary and repo-hygiene suites; `bun x tsc --noEmit` clean; `privacy:scan` green. Full-suite validation remains skipped per the user's instruction. --- src/server/request-log.ts | 13 +++++- src/usage/log.ts | 14 +++++- tests/server-kiro-completion-e2e.test.ts | 55 ++++++++++++++++++++++++ tests/usage-log.test.ts | 18 ++++++++ 4 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/server/request-log.ts b/src/server/request-log.ts index 4c1779a4d5e..ed8f58e080f 100644 --- a/src/server/request-log.ts +++ b/src/server/request-log.ts @@ -140,6 +140,12 @@ export interface RequestLogEntry { /** TTFT: ms from request start to the first non-empty model output delta; unset for non-streaming/tool-only. */ firstOutputMs?: number; surface?: "claude" | "claude-desktop" | "grok"; + /** + * Set when the proxy answered this turn locally and sent nothing upstream. Without it a zero-send + * row is indistinguishable from a request that vanished. A fixed adapter-supplied identifier, + * never conversation-derived. + */ + localTerminalReason?: string; /** The matched configured key's id. Set ONLY for admissionKind "configured" — * never a sentinel, so a hand-edited entry whose id happens to be "loopback" * cannot absorb unrelated traffic. */ @@ -949,6 +955,7 @@ export function addFinalRequestLog( logCtx.usage, logCtx.usageLogInputTokens, contextWindowForModel(logCtx.providerAdapter ?? logCtx.provider, logCtx.model), + logCtx.localTerminalReason !== undefined, ); const attempts = logCtx.attempts?.map(attempt => ({ ...attempt, @@ -976,6 +983,9 @@ export function addFinalRequestLog( ...(logCtx.apiKeyId ? { apiKeyId: logCtx.apiKeyId } : {}), ...(logCtx.admissionKind ? { admissionKind: logCtx.admissionKind } : {}), ...(logCtx.inboundProtocol ? { inboundProtocol: logCtx.inboundProtocol } : {}), + ...(logCtx.localTerminalReason + ? { localTerminalReason: sanitizeLogMetadataString(logCtx.localTerminalReason) } + : {}), ...(isCodexUsageAccountLogLabel(logCtx.accountLogLabel) ? { accountLogLabel: logCtx.accountLogLabel } : {}), @@ -1109,6 +1119,7 @@ function finalizedUsage( usage: OcxUsage | undefined, inputTokenEstimate: number | undefined, contextWindow: number | undefined, + locallyAnswered = false, ): FinalizedUsageResult { // The ESTIMATE itself is capped at the model's context window (codex-router PR #140). The // combined value below keeps its max(inputTokens, estimate) behavior — a provider-reported @@ -1118,7 +1129,7 @@ function finalizedUsage( && inputTokenEstimate >= 0 ? capEstimateAtContextWindow(inputTokenEstimate, contextWindow) : undefined; - const finalUsage = usageForFinalLog(adapter, usage); + const finalUsage = usageForFinalLog(adapter, usage, locallyAnswered); const usageFallback = !finalUsage && estimate !== undefined ? { inputTokens: estimate, outputTokens: 0, estimated: true } : undefined; diff --git a/src/usage/log.ts b/src/usage/log.ts index 3406ba9ba18..d0101ae3f9b 100644 --- a/src/usage/log.ts +++ b/src/usage/log.ts @@ -181,8 +181,20 @@ function isEstimatedUsageProvider(providerOrAdapter: string): boolean { || providerOrAdapter === "cursor" || providerOrAdapter.startsWith("cursor-"); } -export function usageForFinalLog(provider: string, usage: OcxUsage | undefined): OcxUsage | undefined { +export function usageForFinalLog( + provider: string, + usage: OcxUsage | undefined, + /** + * True when the proxy answered this turn locally and issued no upstream request. Such a turn's + * zero counts are EXACT, so the provider-wide estimated marking must not apply: Kiro and Cursor + * are marked estimated because their adapters can only guess a real inference's usage, and a + * turn with no inference has nothing to guess. Without this, a no-send turn is indistinguishable + * from a real one whose usage frame never arrived. + */ + locallyAnswered = false, +): OcxUsage | undefined { if (!usage) return undefined; + if (locallyAnswered) return usage; if (usage.estimated || isEstimatedUsageProvider(provider)) return { ...usage, estimated: true }; return usage; } diff --git a/tests/server-kiro-completion-e2e.test.ts b/tests/server-kiro-completion-e2e.test.ts index 9a14d4baa64..9479ee92858 100644 --- a/tests/server-kiro-completion-e2e.test.ts +++ b/tests/server-kiro-completion-e2e.test.ts @@ -6,6 +6,7 @@ import { KIRO_COMPLETION_TOOL_NAME } from "../src/adapters/kiro-constants"; import { saveConfig } from "../src/config"; import { encodeMessage } from "../src/lib/eventstream-decoder"; import { startServer } from "../src/server"; +import { clearRequestLogsForTests, getRequestLogEntries } from "../src/server/request-log"; import type { OcxConfig } from "../src/types"; import { installIsolatedCodexHome, type IsolatedCodexHome } from "./helpers/isolated-codex-home"; @@ -348,3 +349,57 @@ describe("Kiro completion through public server endpoints", () => { } }); }); + +// The behavioural tests above prove nothing was SENT. This one proves the request log says so. +// The C-phase verifier caught exactly this gap: the first implementation logged the local terminal +// with `estimated: true`, because Kiro usage is marked estimated provider-wide. Zero counts from a +// turn that made no inference are exact, and a row that claims otherwise is indistinguishable from +// a real turn whose usage frame never arrived. +describe("Kiro local terminal accounting", () => { + for (const stream of [true, false]) { + test(`a delivered final answer logs exact zero usage and no send (stream=${stream})`, async () => { + const upstream = scriptedKiroUpstream([]); + saveConfig(kiroConfig(upstream.server.url.toString())); + clearRequestLogsForTests(); + const proxy = startServer(0); + try { + const response = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + phase: "final_answer", + content: [{ type: "output_text", text: "Code mode runs JavaScript that calls tools." }], + }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + expect(response.status).toBe(200); + // The streaming log is written when the body finishes, so drain it before reading. + await response.text(); + expect(upstream.requests).toHaveLength(0); + + const entry = getRequestLogEntries().find(row => row.provider === "kiro-test"); + expect(entry).toBeDefined(); + expect(entry!.localTerminalReason).toBe("kiro_final_answer_already_delivered"); + // Exact, not estimated: this is the assertion the first implementation failed. + expect(entry!.usageStatus).toBe("reported"); + expect(entry!.usage?.estimated).toBeUndefined(); + expect(entry!.usage?.inputTokens).toBe(0); + expect(entry!.usage?.outputTokens).toBe(0); + expect(entry!.attempts?.every(attempt => attempt.sendCount === 0) ?? true).toBe(true); + expect(entry!.attempts?.every(attempt => attempt.inputTokenEstimate === undefined) ?? true).toBe(true); + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); + } +}); diff --git a/tests/usage-log.test.ts b/tests/usage-log.test.ts index e22c7e84b3c..ac5db1fa038 100644 --- a/tests/usage-log.test.ts +++ b/tests/usage-log.test.ts @@ -784,6 +784,24 @@ describe("usage log", () => { expect(usageForFinalLog("openai", { ...usage, estimated: true })).toEqual({ ...usage, estimated: true }); }); + // A turn the proxy answered locally issued no upstream request, so its zero counts are exact. + // The provider-wide estimated marking exists because the Kiro/Cursor adapters can only guess a + // real inference's usage; there is nothing to guess when there was no inference, and marking it + // estimated makes a no-send turn indistinguishable from one whose usage frame never arrived. + test("a locally answered turn keeps exact usage instead of the provider estimated marking", () => { + const zero = { inputTokens: 0, outputTokens: 0, totalTokens: 0 }; + expect(usageForFinalLog("kiro", zero, true)).toEqual(zero); + expect(usageForFinalLog("kiro-p9d8524", zero, true)).toEqual(zero); + // Default and explicit-false keep the existing behavior: this is opt-in per turn, so an + // ordinary Kiro turn cannot lose its estimated marking by omission. + expect(usageForFinalLog("kiro", zero)).toEqual({ ...zero, estimated: true }); + expect(usageForFinalLog("kiro", zero, false)).toEqual({ ...zero, estimated: true }); + // The flag reports a fact about the turn, not about the numbers: an adapter that genuinely + // estimated something still says so. + const guessed = { inputTokens: 4, outputTokens: 6, estimated: true }; + expect(usageForFinalLog("kiro", guessed, true)).toEqual(guessed); + }); + test("preserves cached token counts alongside estimated status", () => { appendUsageEntry({ requestId: "ocx-cache", From b0740840dbf1a09fba06c72c4f16cac425a5d1c1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 22:14:48 +0900 Subject: [PATCH 7/9] fix(kiro): mark the physical attempt as locally answered too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-verification failed criterion 3 again, one level down. The parent request-log row read exact, but `addFinalRequestLog` finalizes its active attempt first through `finishRequestAttempt`, which ran `finalizedUsage` without the new `locallyAnswered` argument. So the row said `usageStatus: "reported"` while its own embedded attempt still said `estimated: true` — and the attempt is the detailed accounting a maintainer actually reads for a zero-send turn. The flag travels ON the attempt rather than as a `finishRequestAttempt` parameter. That function is called from six places (core, collaboration, compact, encrypted-payload, policy-fallback, and the request-log module itself), and a new positional argument would silently default to the wrong answer at any call site that was missed. Setting it once where the local terminal is decided cannot be forgotten by a later caller. The gap survived the first fix for the same reason it existed: the accounting tests asserted the row and the attempt's `sendCount`/`inputTokenEstimate`, but not the attempt's usage provenance. They now assert `usageStatus` and absent `estimated` on every embedded attempt. Verification: 205 tests pass across server-kiro-completion-e2e, usage-log, request-log, kiro-adapter, tool-catalog-nudge and cursor-exec-empty-result; `bun x tsc --noEmit` clean. Full-suite validation skipped per instruction. --- src/server/request-log.ts | 1 + src/server/responses/core.ts | 5 +++++ src/usage/log.ts | 7 +++++++ tests/server-kiro-completion-e2e.test.ts | 7 +++++++ 4 files changed, 20 insertions(+) diff --git a/src/server/request-log.ts b/src/server/request-log.ts index ed8f58e080f..fee578e1b7e 100644 --- a/src/server/request-log.ts +++ b/src/server/request-log.ts @@ -1229,6 +1229,7 @@ export function finishRequestAttempt( usage ?? attempt.usage, attempt.inputTokenEstimate, contextWindowForModel(attempt.adapter, attempt.model), + attempt.locallyAnswered === true, ); attempt.status = status; attempt.durationMs = Math.max(0, durationMs); diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 4f900364b9a..23e82f720e3 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -4866,6 +4866,11 @@ async function handleResponsesInner( const localTerminal = activeAdapter.localTerminal?.(parsed); if (localTerminal) { logCtx.localTerminalReason = localTerminal.reason; + // Mark the physical attempt too, not just the parent row. `finishRequestAttempt` finalizes the + // attempt through the same estimated-provider path, so without this the row reads exact while + // its own attempt still claims an estimate — the detailed accounting a maintainer actually + // reads for a zero-send turn. + if (logCtx.activeAttempt) logCtx.activeAttempt.locallyAnswered = true; cleanupUpstreamAbort(); upstream.abort(); const terminalEvents: AdapterEvent[] = [{ diff --git a/src/usage/log.ts b/src/usage/log.ts index d0101ae3f9b..36648976a0b 100644 --- a/src/usage/log.ts +++ b/src/usage/log.ts @@ -51,6 +51,13 @@ export interface PersistedUsageAttempt { sendCount: number; recoveryKinds: AttemptRecoveryKind[]; usageStatus: UsageStatus; + /** + * True when the proxy answered this turn locally and issued no upstream request. It travels on + * the attempt itself rather than as a `finishRequestAttempt` argument because that function is + * called from six places, and a new parameter would silently default to the wrong answer at any + * one of them that was missed. Absent on ordinary attempts so old rows keep their exact shape. + */ + locallyAnswered?: boolean; /** Stable non-PII identity for the Codex pool account that served this attempt. */ accountLogLabel?: CodexUsageAccountLogLabel; inputTokenEstimate?: number; diff --git a/tests/server-kiro-completion-e2e.test.ts b/tests/server-kiro-completion-e2e.test.ts index 9479ee92858..6558414fdfa 100644 --- a/tests/server-kiro-completion-e2e.test.ts +++ b/tests/server-kiro-completion-e2e.test.ts @@ -396,6 +396,13 @@ describe("Kiro local terminal accounting", () => { expect(entry!.usage?.outputTokens).toBe(0); expect(entry!.attempts?.every(attempt => attempt.sendCount === 0) ?? true).toBe(true); expect(entry!.attempts?.every(attempt => attempt.inputTokenEstimate === undefined) ?? true).toBe(true); + // The EMBEDDED attempt must agree with its parent row. The re-verification caught this + // exact gap: the row read exact while its own attempt still said "estimated", and the + // attempt is the detailed accounting a maintainer reads for a zero-send turn. + for (const attempt of entry!.attempts ?? []) { + expect(attempt.usageStatus).toBe("reported"); + expect(attempt.usage?.estimated).toBeUndefined(); + } } finally { await proxy.stop(true); upstream.server.stop(true); From d9d26552f4d334ac8ea237a43d2db6c4586aea87 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 22:15:04 +0900 Subject: [PATCH 8/9] fix(tools): state the code-mode echo rule before the first call, not after a wasted one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EMPTY_EXEC_OUTPUT_MESSAGE` is a repair: it fires only once a model has already spent a call and read a blank result. That recovers the turn but cannot recover the round trip, and it leaves the model guessing whether its command failed or its output was merely dropped. Live 2026-08-28, a routed Kiro session did exactly that — reported a blank exec result as possible context loss, and learned the echo rule only from the repair text. The rule now also appears in the tool-catalog nudge, so a code-mode turn is told up front that nothing in the isolate is echoed automatically and that a bare trailing `await tools.(...)` is discarded. The wording is one exported constant shared by both surfaces and by the Cursor guidance note: two surfaces describing the same isolate must not drift, because that is how a model gets told two different things about whether its output survived. Scope is gated on an actual code-mode exec being advertised. A flat shell bridge echoes stdout on its own, so the same sentence there would be false — asserted directly rather than left to the reader. Verification: 205 tests pass across server-kiro-completion-e2e, usage-log, request-log, kiro-adapter, tool-catalog-nudge and cursor-exec-empty-result; `bun x tsc --noEmit` clean. The kiro-adapter assertion checks the ACTUAL wire prompt rather than the builder alone, since the session that misread a blank result was a routed Kiro turn. Full-suite validation skipped per instruction. --- src/adapters/cursor/tool-definitions.ts | 3 +- src/adapters/exec-tool-result-normalize.ts | 14 +++++++++ src/adapters/tool-catalog-nudge.ts | 3 +- tests/kiro-adapter.test.ts | 3 ++ tests/tool-catalog-nudge.test.ts | 35 ++++++++++++++++++++++ 5 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/adapters/cursor/tool-definitions.ts b/src/adapters/cursor/tool-definitions.ts index b0f2f26cf69..ae9bfcec4bb 100644 --- a/src/adapters/cursor/tool-definitions.ts +++ b/src/adapters/cursor/tool-definitions.ts @@ -3,6 +3,7 @@ import { ValueSchema } from "@bufbuild/protobuf/wkt"; import type { OcxRequestOptions, OcxTool } from "../../types"; import { namespacedToolName, toolChoiceAliases } from "../../types"; import { McpToolDefinitionSchema, McpToolsSchema, type McpToolDefinition } from "./gen/agent_pb"; +import { CODE_MODE_RESULT_ECHO_SENTENCE } from "../exec-tool-result-normalize"; export const OCX_RESPONSES_TOOL_PROVIDER = "opencodex-responses"; export const CODEX_EXEC_COMMAND_TOOL = "exec_command"; @@ -654,7 +655,7 @@ export function buildCursorToolGuidanceSystemNote( ? `\`${CODEX_UNIFIED_EXEC_TOOL}\` is Codex code mode: its body is JavaScript evaluated in a V8 isolate, not a shell command and not Node. Shell, file edits, and MCP are nested helpers called INSIDE that body as \`await tools.(...)\`, for example \`await tools.exec_command({cmd: \"ls\"})\`. Read the tool description and the isolate global \`ALL_TOOLS\` (not \`tools.ALL_TOOLS\`) for helpers this turn provides; absence from the top-level catalog or from \`exec\`'s description is not absence. Those nested helpers are not themselves top-level tools, so do not call \`exec_command\` or \`shell_command\` at the top level here${codeModeOtherTopLevelNames.length > 0 ? `; every other tool this turn lists, including ${quotedNames(codeModeOtherTopLevelNames)}, remains callable at the top level as usual` : ""}. Nested \`tools.apply_patch(input)\` is host-executed: the string must begin exactly with \`*** Begin Patch\` and end with \`*** End Patch\` (no trailing \`***\` on those lines). OpenCodex does not rewrite JavaScript inside exec, so a decorated \`*** Begin Patch ***\` envelope is rejected by Codex before the file is touched.` : undefined, codeMode - ? "In code mode the isolate returns nothing on its own: call `text(...)` (or `notify(...)`) on any value you need to see, or the call completes with empty output. There is no `require`, no `module`, and no filesystem or network globals; reach the host only through the nested helpers." + ? CODE_MODE_RESULT_ECHO_SENTENCE + " There is no `require`, no `module`, and no filesystem or network globals; reach the host only through the nested helpers." : undefined, codeMode ? "NEVER attempt Cursor-native Shell, Read, Grep, List, or any tool absent from the catalog — they are not executed in this environment and every probe wastes a turn. The exec code cell (with its nested helpers) is the ONLY execution surface; go to it directly on the FIRST attempt and do not narrate switching surfaces." diff --git a/src/adapters/exec-tool-result-normalize.ts b/src/adapters/exec-tool-result-normalize.ts index 9b719b13f27..f06a07c411f 100644 --- a/src/adapters/exec-tool-result-normalize.ts +++ b/src/adapters/exec-tool-result-normalize.ts @@ -36,6 +36,20 @@ export const FAILED_EXEC_OUTPUT_MESSAGE = export const EMPTY_EXEC_OUTPUT_MESSAGE = "[empty output: the exec cell completed but emitted nothing. This is NOT lost context and NOT a blocked tool — in code mode call text(...) or notify(...) on any value you need to see (a bare await tools.exec_command(...) is not echoed automatically); in shell mode the command simply printed nothing. Do not re-run the same call expecting different output.]"; +/** + * The SAME rule stated BEFORE the first call, for the code-mode tool-catalog nudge. + * + * `EMPTY_EXEC_OUTPUT_MESSAGE` above is a repair: it fires only after a model has already spent a + * call and read a blank result. That recovers the turn but cannot prevent the wasted round trip, + * and the model still has to guess whether its command failed or its output was merely dropped. + * Stating the echo rule up front removes the failure instead of explaining it afterwards. + * + * Kept beside the recovery text on purpose: the two are one pair guarding one defect, and wording + * that drifts apart is how a model gets told two different things about the same isolate. + */ +export const CODE_MODE_RESULT_ECHO_SENTENCE = + "Nothing in the isolate is echoed automatically: a bare trailing `await tools.(...)` or final expression value is DISCARDED, and the cell reports empty output. Pass anything you need to read to `text(...)` (or `notify(...)`) in the same cell — for example `text(JSON.stringify(await tools.exec_command({cmd: \"ls\"})))` — and treat an empty result as your own missing `text(...)` call rather than a failed command or lost context."; + /** * Codex exec / shell-bridge tool names (flat and MCP-prefixed display aliases). An empty result * here is almost always a code-mode cell that never called text()/notify(). diff --git a/src/adapters/tool-catalog-nudge.ts b/src/adapters/tool-catalog-nudge.ts index b14ff098372..8653fde1d7f 100644 --- a/src/adapters/tool-catalog-nudge.ts +++ b/src/adapters/tool-catalog-nudge.ts @@ -5,6 +5,7 @@ import { type OcxTool, type OcxProviderConfig, } from "../types"; +import { CODE_MODE_RESULT_ECHO_SENTENCE } from "./exec-tool-result-normalize"; // Tool names that exist only in OTHER agent harnesses (Claude Code and friends). Naming one // here tells a routed model not to call it unless this turn's catalog really lists it. @@ -120,7 +121,7 @@ export function buildNonOpenAIToolCatalogNudgeFromNames( "Call only listed names with their listed argument keys; do not invent, translate, or rename tools.", "Names mentioned only in instructions, tool descriptions, argument descriptions, or nested helper APIs are not additional top-level tools.", verifiedCodeModeExecName - ? "`" + verifiedCodeModeExecName + "` is Codex code mode: its body is JavaScript evaluated in a V8 isolate. Nested helpers are called INSIDE that body as `await tools.(...)`, for example `await tools.exec_command({cmd: \"ls\"})` or `await tools.codex_app__list_threads({})`. Absence from the top-level catalog or from `" + verifiedCodeModeExecName + "`'s description is not absence: deferred helpers stay callable on `tools.`. Discover them from the isolate global `ALL_TOOLS`, not `tools.ALL_TOOLS`. Do not skip an available nested helper because it is omitted from the listed top-level names. Nested `tools.apply_patch(input)` is host-executed: the string must begin exactly with `*** Begin Patch` and end with `*** End Patch` (no trailing `***` on those lines). OpenCodex does not rewrite JavaScript inside exec, so a decorated `*** Begin Patch ***` envelope is rejected by Codex before the file is touched." + ? "`" + verifiedCodeModeExecName + "` is Codex code mode: its body is JavaScript evaluated in a V8 isolate. Nested helpers are called INSIDE that body as `await tools.(...)`, for example `await tools.exec_command({cmd: \"ls\"})` or `await tools.codex_app__list_threads({})`. Absence from the top-level catalog or from `" + verifiedCodeModeExecName + "`'s description is not absence: deferred helpers stay callable on `tools.`. Discover them from the isolate global `ALL_TOOLS`, not `tools.ALL_TOOLS`. Do not skip an available nested helper because it is omitted from the listed top-level names. " + CODE_MODE_RESULT_ECHO_SENTENCE + " Nested `tools.apply_patch(input)` is host-executed: the string must begin exactly with `*** Begin Patch` and end with `*** End Patch` (no trailing `***` on those lines). OpenCodex does not rewrite JavaScript inside exec, so a decorated `*** Begin Patch ***` envelope is rejected by Codex before the file is touched." : "If a listed tool exposes nested helpers such as a tools.* API, call the listed parent tool and use those helpers only inside that tool's input.", unavailableNeighborNames.length > 0 ? "Do not use neighboring-agent tool names " + quoteNames(unavailableNeighborNames) + " unless this turn's catalog lists those exact names." diff --git a/tests/kiro-adapter.test.ts b/tests/kiro-adapter.test.ts index b0d66ceb6bc..2ae9bc3bf7b 100644 --- a/tests/kiro-adapter.test.ts +++ b/tests/kiro-adapter.test.ts @@ -1413,6 +1413,9 @@ describe("kiro code-mode catalog nudge", () => { expect(content).toContain("ALL_TOOLS"); expect(content).toContain("Codex code mode"); + // Reaches the ACTUAL Kiro wire prompt, not just the builder: the live 2026-08-28 session that + // misread a blank result was a routed Kiro turn. + expect(content).toContain("Nothing in the isolate is echoed automatically"); // The generic fallback must be gone, not merely accompanied. expect(content).not.toContain("If a listed tool exposes nested helpers such as a tools.* API"); }); diff --git a/tests/tool-catalog-nudge.test.ts b/tests/tool-catalog-nudge.test.ts index 702d0f3e1d9..53609821605 100644 --- a/tests/tool-catalog-nudge.test.ts +++ b/tests/tool-catalog-nudge.test.ts @@ -4,6 +4,7 @@ import { buildNonOpenAIToolCatalogNudgeFromNames, shouldInjectNonOpenAIToolCatalogNudge, } from "../src/adapters/tool-catalog-nudge"; +import { CODE_MODE_RESULT_ECHO_SENTENCE, EMPTY_EXEC_OUTPUT_MESSAGE } from "../src/adapters/exec-tool-result-normalize"; import type { OcxTool } from "../src/types"; describe("non-OpenAI tool catalog nudge", () => { @@ -217,4 +218,38 @@ describe("non-OpenAI tool catalog nudge", () => { expect(shouldInjectNonOpenAIToolCatalogNudge({ baseUrl: "https://chatgpt.com/backend-api/codex" })).toBe(false); expect(shouldInjectNonOpenAIToolCatalogNudge({ baseUrl: "https://api.kimi.com/coding/v1" })).toBe(true); }); + + // The empty-result guidance in exec-tool-result-normalize only fires AFTER a wasted call. Live + // 2026-08-28: a routed Kiro session read a blank result, reported it as possible context loss, + // and learned the echo rule only from the repair text. Stating it up front is the prevention. + describe("code-mode result echo rule", () => { + test("states the echo rule before the first call when code mode is advertised", () => { + const note = buildNonOpenAIToolCatalogNudgeFromNames(["exec"], name => name, "exec"); + + if (!note) throw new Error("Expected a nudge for a code-mode catalog"); + expect(note).toContain("Nothing in the isolate is echoed automatically"); + expect(note).toContain("is DISCARDED"); + expect(note).toContain("text(...)"); + // Names the wrong conclusions, so a blank result is not read as a failed command. + expect(note).toContain("rather than a failed command or lost context"); + }); + + test("omits the echo rule when the turn has no code-mode exec", () => { + const note = buildNonOpenAIToolCatalogNudgeFromNames(["exec_command", "mcp__fs__read_file"]); + + if (!note) throw new Error("Expected a nudge for a flat catalog"); + // A flat shell bridge echoes stdout on its own; this guidance would be a lie there. + expect(note).not.toContain("Nothing in the isolate is echoed automatically"); + }); + + test("shares one wording with the post-hoc empty-result guidance", () => { + const note = buildNonOpenAIToolCatalogNudgeFromNames(["exec"], name => name, "exec"); + + if (!note) throw new Error("Expected a nudge for a code-mode catalog"); + // Both surfaces must describe the same isolate. Drift here is how a model gets told two + // different things about whether its output survived. + expect(note).toContain(CODE_MODE_RESULT_ECHO_SENTENCE); + expect(EMPTY_EXEC_OUTPUT_MESSAGE).toContain("text(...)"); + }); + }); }); From 68eaf45d84b96bba5dc90fa48966d2aec7ff6422 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 28 Aug 2026 22:30:17 +0900 Subject: [PATCH 9/9] fix(kiro): remember a delivered final answer instead of trusting the client to echo phase Suppressing a finished turn keyed off the inbound `phase: "final_answer"` alone, which is a field the proxy emits but no client is obliged to send back. Measured against the running proxy: the same history WITH phase sent nothing upstream, and WITHOUT it spent 7686 input tokens redoing the closed task. `localTerminalReason` appeared 0 times across 1534 logged Kiro turns even though the suppression code was deployed and loaded, so the feature had never once fired in production. The proxy now records the final answer it actually emitted, keyed by the normalized conversation digest it already computes for logging, and consults that record when the inbound phase is missing. An explicit phase still wins; the record is an additional source. Scope is the load-bearing constraint. Only the 32-hex normalized digest may key the map, so no raw header or account identifier can be retained here, and an unresolvable scope means "no record" rather than a global fallback. Matching requires the remembered answer to still be the trailing content-bearing message: every legitimate next turn contains a new user message somewhere, so "a later user message exists" would suppress nothing, while keying off the trailing turn distinguishes a re-sent closed history from a genuine follow-up. A local terminal emits no output and therefore writes no record, which is what keeps suppression alive across the repeated identical histories the live loop actually produced. --- src/adapters/kiro.ts | 10 +- src/responses/turn-termination.ts | 107 ++++++++++++++ src/server/responses/core.ts | 13 ++ tests/server-kiro-completion-e2e.test.ts | 175 +++++++++++++++++++++++ 4 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 src/responses/turn-termination.ts diff --git a/src/adapters/kiro.ts b/src/adapters/kiro.ts index 2523373041e..ccfd80febea 100644 --- a/src/adapters/kiro.ts +++ b/src/adapters/kiro.ts @@ -36,6 +36,7 @@ import type { OcxTool, OcxUsage, } from "../types"; +import { hasRecordedTrailingDeliveredFinalAnswer } from "../responses/turn-termination"; import type { ProviderAdapter } from "./base"; import type { AdapterFetchContext, AdapterRequest } from "./base"; import { extractKiroImages, normalizeKiroImages, type KiroImage } from "./kiro-images"; @@ -379,7 +380,7 @@ type KiroTurn = * means work continued, so the turn is no longer terminal. Empty assistant messages are skipped * rather than treated as continuation, since they carry no visible turn. */ -function hasTrailingDeliveredFinalAnswer(messages: readonly OcxMessage[]): boolean { +function hasTrailingDeliveredFinalAnswer(messages: readonly OcxMessage[], parsed?: OcxParsedRequest): boolean { for (let i = messages.length - 1; i >= 0; i--) { const msg = messages[i]; if (msg.role !== "assistant") return false; @@ -388,7 +389,8 @@ function hasTrailingDeliveredFinalAnswer(messages: readonly OcxMessage[]): boole if (hasToolCall) return false; const hasText = (aMsg.content ?? []).some(part => part.type === "text" && part.text.trim()); if (!hasText) continue; - return aMsg.phase === "final_answer"; + return aMsg.phase === "final_answer" + || (parsed !== undefined && hasRecordedTrailingDeliveredFinalAnswer(parsed, messages)); } return false; } @@ -510,7 +512,7 @@ export function buildKiroPayload( // Read from parsed messages because `completionMode` is needed to build the tool catalog, which // happens before the turn list exists. `forcedCompletionMode` still wins: the fallback retry // passes "text_fallback" explicitly and must not be silently downgraded. - const trailingDeliveredAnswer = hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed)); + const trailingDeliveredAnswer = hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed), parsed); const completionMode: KiroCompletionMode = forcedCompletionMode ?? (ordinaryTools.length > 0 && !trailingDeliveredAnswer ? "required" : "disabled"); const kiroTools = completionMode === "disabled" @@ -2015,7 +2017,7 @@ export function createKiroAdapter(provider: OcxProviderConfig): ProviderAdapter // turn only, and the adapter-owned bounded retry passes "text_fallback" through `build` // directly, never through this path. localTerminal(parsed: OcxParsedRequest) { - return hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed)) + return hasTrailingDeliveredFinalAnswer(kiroPayloadMessages(parsed), parsed) ? { reason: "kiro_final_answer_already_delivered" } : undefined; }, diff --git a/src/responses/turn-termination.ts b/src/responses/turn-termination.ts new file mode 100644 index 00000000000..03834591eac --- /dev/null +++ b/src/responses/turn-termination.ts @@ -0,0 +1,107 @@ +import { createHash } from "node:crypto"; +import type { OcxAssistantMessage, OcxMessage, OcxParsedRequest } from "../types"; + +const DELIVERED_FINAL_ANSWER_TTL_MS = 60 * 60 * 1_000; +const DELIVERED_FINAL_ANSWER_MAX_ENTRIES = 1_024; + +interface DeliveredFinalAnswerRecord { + fingerprint: string; + createdAt: number; +} + +const scopesByRequest = new WeakMap(); +const deliveredFinalAnswers = new Map(); + +function pruneDeliveredFinalAnswers(at = Date.now()): void { + for (const [scope, record] of deliveredFinalAnswers) { + if (at - record.createdAt > DELIVERED_FINAL_ANSWER_TTL_MS) deliveredFinalAnswers.delete(scope); + } + while (deliveredFinalAnswers.size > DELIVERED_FINAL_ANSWER_MAX_ENTRIES) { + const oldest = deliveredFinalAnswers.keys().next().value; + if (oldest === undefined) break; + deliveredFinalAnswers.delete(oldest); + } +} + +function textFingerprint(text: string): string { + return createHash("sha256").update(text, "utf8").digest("hex"); +} + +function assistantText(message: OcxAssistantMessage): string | undefined { + if (message.content.some(part => part.type === "toolCall")) return undefined; + const text = message.content + .filter((part): part is Extract => part.type === "text") + .map(part => part.text) + .join(""); + return text.trim().length > 0 ? text : undefined; +} + +function deliveredFinalAnswerText(response: unknown): string | undefined { + if (!response || typeof response !== "object" || Array.isArray(response)) return undefined; + const output = (response as { output?: unknown }).output; + if (!Array.isArray(output)) return undefined; + for (let i = output.length - 1; i >= 0; i--) { + const item = output[i]; + if (!item || typeof item !== "object" || Array.isArray(item)) continue; + const message = item as { type?: unknown; role?: unknown; phase?: unknown; content?: unknown }; + if (message.type !== "message" || message.role !== "assistant" || message.phase !== "final_answer") continue; + if (!Array.isArray(message.content)) return undefined; + const text = message.content + .filter(part => !!part && typeof part === "object" && !Array.isArray(part) + && (part as { type?: unknown }).type === "output_text" + && typeof (part as { text?: unknown }).text === "string") + .map(part => (part as { text: string }).text) + .join(""); + return text.trim().length > 0 ? text : undefined; + } + return undefined; +} + +/** Bind the normalized per-conversation digest without adding proxy-private fields to the wire body. */ +export function bindTurnTerminationScope(parsed: OcxParsedRequest, scope: string | undefined): void { + // Only the normalized log-conversation digest may key this process-wide map. Refusing any raw + // fallback prevents a future caller from retaining a client header or account identifier here. + if (!scope || !/^[0-9a-f]{32}$/.test(scope)) return; + scopesByRequest.set(parsed, scope); +} + +/** Remember only a final-answer message the proxy actually emitted for this exact conversation. */ +export function rememberDeliveredFinalAnswer(parsed: OcxParsedRequest, response: unknown): void { + const scope = scopesByRequest.get(parsed); + if (!scope) return; + const text = deliveredFinalAnswerText(response); + if (!text) return; + const at = Date.now(); + pruneDeliveredFinalAnswers(at); + // Refresh insertion order so cap eviction removes the least recently delivered conversation. + deliveredFinalAnswers.delete(scope); + deliveredFinalAnswers.set(scope, { fingerprint: textFingerprint(text), createdAt: at }); + pruneDeliveredFinalAnswers(at); +} + +/** + * Match only when the delivered assistant answer is still the trailing content-bearing message. + * A later user/tool-result message is new work and must reach the provider, even though every + * legitimate next turn necessarily contains such a message somewhere in its history. + */ +export function hasRecordedTrailingDeliveredFinalAnswer( + parsed: OcxParsedRequest, + messages: readonly OcxMessage[], +): boolean { + const scope = scopesByRequest.get(parsed); + if (!scope) return false; + pruneDeliveredFinalAnswers(); + const record = deliveredFinalAnswers.get(scope); + if (!record) return false; + for (let i = messages.length - 1; i >= 0; i--) { + const message = messages[i]; + if (message.role !== "assistant") return false; + const text = assistantText(message as OcxAssistantMessage); + if (text === undefined) { + if ((message as OcxAssistantMessage).content.some(part => part.type === "toolCall")) return false; + continue; + } + return textFingerprint(text) === record.fingerprint; + } + return false; +} diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 23e82f720e3..2c916775b3c 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -40,6 +40,10 @@ import { previousResponseScopeMismatch, rememberResponseState, } from "../../responses/state"; +import { + bindTurnTerminationScope, + rememberDeliveredFinalAnswer, +} from "../../responses/turn-termination"; import { isValidProviderContinuationOwner, mergeProviderContinuationPayload, @@ -2380,6 +2384,10 @@ async function handleResponsesInner( threadIdHeader: req.headers.get("thread-id"), cursorConversationId: parsed._cursorConversationId, }); + bindTurnTerminationScope(parsed, resolvedConversationId); + const rememberKiroDeliveredFinalAnswer = (adapterName: string, response: unknown): void => { + if (adapterName === "kiro") rememberDeliveredFinalAnswer(parsed, response); + }; // _clientThreadId remains the routing/continuation identity supplied by Codex. Replay state uses // a dedicated raw conversation namespace so mixed headers that carry the same identity still // match, and a shared/synthetic session_id cannot coalesce distinct thread/Cursor conversations. @@ -4440,6 +4448,7 @@ async function handleResponsesInner( ...(options.forceEmptyResponseId ? { forceEmptyResponseId: true } : {}), onCompletedResponse: (response, providerState) => { commitReasoningReplayServingRoute(); + rememberKiroDeliveredFinalAnswer(adapter.name, response); rememberResponseState( parsed._rawBody, response, @@ -4755,6 +4764,7 @@ async function handleResponsesInner( }, onCompletedResponse: (response: Record, providerState?: OcxProviderContinuationState) => { commitReasoningReplayServingRoute(); + rememberKiroDeliveredFinalAnswer(adapter.name, response); if (!routedCompaction) { rememberResponseState( parsed._rawBody, @@ -4824,6 +4834,7 @@ async function handleResponsesInner( }, }); if (!routedCompaction) { + rememberKiroDeliveredFinalAnswer(adapter.name, json); rememberResponseState( parsed._rawBody, json, @@ -5764,6 +5775,7 @@ async function handleResponsesInner( }, onCompletedResponse: (response: Record, providerState?: OcxProviderContinuationState) => { commitReasoningReplayServingRoute(); + rememberKiroDeliveredFinalAnswer(activeAdapter.name, response); // Compaction turns must NOT enter the continuation cache: _rawBody still holds the full // PRE-compaction history, and a later previous_response_id expansion would rehydrate the // giant stale chain Codex just replaced. @@ -5841,6 +5853,7 @@ async function handleResponsesInner( }); // See the streaming branch: compaction turns skip the continuation cache. if (!routedCompaction) { + rememberKiroDeliveredFinalAnswer(activeAdapter.name, json); rememberResponseState( parsed._rawBody, json, diff --git a/tests/server-kiro-completion-e2e.test.ts b/tests/server-kiro-completion-e2e.test.ts index 6558414fdfa..5708c913e01 100644 --- a/tests/server-kiro-completion-e2e.test.ts +++ b/tests/server-kiro-completion-e2e.test.ts @@ -313,6 +313,181 @@ describe("Kiro completion through public server endpoints", () => { } } + test("a proxy-recorded final answer suppresses replay when the client drops phase", async () => { + const deliveredAnswer = "Code mode runs JavaScript that calls tools."; + const upstream = scriptedKiroUpstream([completionFrames(deliveredAnswer)]); + saveConfig(kiroConfig(upstream.server.url.toString())); + const proxy = startServer(0); + try { + const first = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-recorded-final-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: "what is code mode", + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + expect(first.status).toBe(200); + await first.text(); + expect(upstream.requests).toHaveLength(1); + + const replay = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-recorded-final-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + content: [{ type: "output_text", text: deliveredAnswer }], + }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + + expect(replay.status).toBe(200); + expect((await replay.json() as { output?: unknown[] }).output ?? []).toHaveLength(0); + // The second turn is byte-for-byte replay history except for the client-dropped phase. A + // send here means the proxy forgot its own delivered answer and restarted finished work. + expect(upstream.requests).toHaveLength(1); + + // Suppression must SURVIVE being used. A local terminal emits no output, so it writes no new + // record; if the read consumed or overwrote the original one, the second identical replay + // would send upstream and the loop this fix exists to end would return one turn later. The + // observed live failure was exactly a repeated identical history, not a single stray turn. + const replayAgain = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-recorded-final-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + content: [{ type: "output_text", text: deliveredAnswer }], + }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + expect(replayAgain.status).toBe(200); + expect((await replayAgain.json() as { output?: unknown[] }).output ?? []).toHaveLength(0); + expect(upstream.requests).toHaveLength(1); + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); + + test("a new user request after a proxy-recorded final answer is not suppressed", async () => { + const deliveredAnswer = "Code mode runs JavaScript that calls tools."; + const upstream = scriptedKiroUpstream([ + completionFrames(deliveredAnswer), + completionFrames("Yes — one JavaScript cell can make several tool calls."), + ]); + saveConfig(kiroConfig(upstream.server.url.toString())); + const proxy = startServer(0); + try { + const first = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-recorded-follow-up-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: "what is code mode", + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + expect(first.status).toBe(200); + await first.text(); + + const followUp = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-recorded-follow-up-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + content: [{ type: "output_text", text: deliveredAnswer }], + }, + { type: "message", role: "user", content: [{ type: "input_text", text: "so it batches calls?" }] }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + + expect(followUp.status).toBe(200); + await followUp.text(); + // A later user message is the new work. The remembered answer may suppress only when that + // same assistant answer is still the trailing content-bearing message. + expect(upstream.requests).toHaveLength(2); + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); + + test("a recorded final answer is isolated to its conversation scope", async () => { + const deliveredAnswer = "Code mode runs JavaScript that calls tools."; + const upstream = scriptedKiroUpstream([ + completionFrames(deliveredAnswer), + completionFrames("This is a separate thread, so I answered independently."), + ]); + saveConfig(kiroConfig(upstream.server.url.toString())); + const proxy = startServer(0); + try { + const first = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-record-owner-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: "what is code mode", + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + expect(first.status).toBe(200); + await first.text(); + + const otherThread = await originalFetch(new URL("/v1/responses", proxy.url), { + method: "POST", + headers: { "content-type": "application/json", session_id: "kiro-record-other-thread" }, + body: JSON.stringify({ + model: "kiro-test/gpt-5.6-sol", + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "what is code mode" }] }, + { + type: "message", + role: "assistant", + content: [{ type: "output_text", text: deliveredAnswer }], + }, + ], + tools: [{ type: "function", name: "bash", description: "Run a command", parameters: { type: "object" } }], + }), + }); + + expect(otherThread.status).toBe(200); + await otherThread.text(); + expect(upstream.requests).toHaveLength(2); + } finally { + await proxy.stop(true); + upstream.server.stop(true); + } + }); + // Control for the above: the predicate keys off the trailing turn, so a genuine follow-up // question after a delivered answer is an ordinary turn and MUST still reach Kiro. Without this, // a short-circuit that swallowed every turn would pass the tests above.