Repository navigation
fix(session): end at the step cap with a toolless answer, and report empty task results #1632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -720,6 +720,11 @@ export namespace SessionPrompt { | |
| let structuredOutput: unknown | undefined | ||
|
|
||
| let step = 0 | ||
| // altimate_change start — the agent's `steps` cap counts model requests only. `step` also | ||
| // counts iterations that run a compaction or a subtask and `continue`, which ended capped | ||
| // agents several requests early. | ||
| let modelSteps = 0 | ||
| // altimate_change end | ||
| // altimate_change start — which tool catalogs of this loop run the workspace | ||
| // engine's turn boundary: the first, and the first after a new user request | ||
| // joins the loop. `step` counts loop iterations, and an iteration can | ||
|
|
@@ -1516,7 +1521,10 @@ export namespace SessionPrompt { | |
| // normal processing | ||
| const agent = await Agent.get(lastUser.agent) | ||
| const maxSteps = agent.steps ?? Infinity | ||
| const isLastStep = step >= maxSteps | ||
| // altimate_change start — see `modelSteps` | ||
| modelSteps++ | ||
| const isLastStep = modelSteps >= maxSteps | ||
| // altimate_change end | ||
| // altimate_change start — only a completed compaction may change this prefix. | ||
| // Recover the marker from persisted history too, including a process restart | ||
| // between summary completion and the next model call. | ||
|
|
@@ -2040,6 +2048,10 @@ export namespace SessionPrompt { | |
| // altimate_change start - every successful commit makes the next generation an echo, including after compaction | ||
| const echoCommittedAnswer = | ||
| format.type === "text" && agent.name === "analyst" && committedAnswer !== undefined | ||
| // altimate_change start — at the step cap a text answer is produced with tools off. A | ||
| // json_schema request keeps its tools: its answer is the StructuredOutput tool call. | ||
| const textFinalStep = isLastStep && !echoCommittedAnswer && format.type !== "json_schema" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: a tool call in answer to the toolless request keeps the loop going, with no bound. If the provider returns a tool call anyway, the repair path maps it to Base also makes 12 requests, and there the tools really run, so this PR is an improvement, but the cap still does not end the run. Suggested fix: after a Not blocking: edge input (a provider returning tool calls with none declared), and main has the same loop in a worse form. |
||
| // altimate_change end | ||
| const deferredText: MessageV2.TextPart[] = [] | ||
| // altimate_change end | ||
|
|
||
|
|
@@ -2151,20 +2163,27 @@ export namespace SessionPrompt { | |
| : isLastStep | ||
| ? [ | ||
| { | ||
| role: "assistant" as const, | ||
| // altimate_change start — the step-cap instruction is a user turn, like the echo | ||
| // above. Sent as a trailing assistant message with every tool still offered, the | ||
| // gateway rejected it and the run ended with no answer. | ||
| role: "user" as const, | ||
| // altimate_change end | ||
| content: MAX_STEPS, | ||
| }, | ||
| ] | ||
| : []), | ||
| // altimate_change end | ||
| ], | ||
| // altimate_change start - the final echo cannot spend another step on tools | ||
| tools: echoCommittedAnswer ? {} : tools, | ||
| // altimate_change start - the final echo, and the step-cap answer, cannot spend another step on tools. | ||
| // Text finalization uses the same toolless shape as compaction and title generation: no | ||
| // tools and toolChoice "none", so llm.ts flattens historical tool calls for the gateway. | ||
| tools: echoCommittedAnswer || textFinalStep ? {} : tools, | ||
| deferText: echoCommittedAnswer ? deferredText : undefined, | ||
| // altimate_change end | ||
| model, | ||
| // altimate_change start - a user output schema still takes precedence over text finalization | ||
| toolChoice: echoCommittedAnswer ? "none" : format.type === "json_schema" ? "required" : undefined, | ||
| toolChoice: | ||
| echoCommittedAnswer || textFinalStep ? "none" : format.type === "json_schema" ? "required" : undefined, | ||
| // altimate_change end | ||
| }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,3 +22,20 @@ export async function interruptedTaskOutput(sessionID: SessionID, parentID?: Mes | |
| ].join("\n") | ||
| } | ||
| } | ||
|
|
||
| // altimate_change start — the newest model-written text of a child task's current prompt, for a | ||
| // task that ends on an error or without a final text answer (see tool/task.ts emptyTaskStatus) | ||
| export async function lastTaskText(sessionID: SessionID, parentID?: MessageID): Promise<string | undefined> { | ||
| for await (const message of MessageV2.stream(sessionID)) { | ||
| if (message.info.role === "user") break | ||
| if (message.info.role !== "assistant") continue | ||
| if (parentID && message.info.parentID !== parentID) continue | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: after a compaction in the child this loses all text, and the parent is told "It wrote no text". After a compaction the newest user message is the compaction continuation. Post-compaction assistants carry I ran it through the real prompt loop on a mock provider: the child writes "Found 3 candidates so far.", a usage-triggered compaction runs, and the next request returns 400. Suggested fix: walk past user messages that carry a compaction part or are synthetic, stop only at the task's prompt id, and drop the Not blocking: it needs a compaction, then an error, with no later text. The error reason still reaches the parent, and the wording errs toward "unfinished". |
||
| const text = message.parts | ||
| .filter((part): part is MessageV2.TextPart => part.type === "text" && !part.ignored && !part.synthetic) | ||
| .map((part) => part.text) | ||
| .join("\n") | ||
| .trim() | ||
| if (text) return text | ||
| } | ||
| } | ||
| // altimate_change end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ import { RunBudget } from "@/altimate/run-budget" | |
| import { ModelID, ProviderID } from "@/provider/schema" | ||
| import { Effect } from "effect" | ||
| // altimate_change start — upstream_fix: recover committed child findings when cancellation interrupts a task. | ||
| import { interruptedTaskOutput } from "../session/task-output" | ||
| import { interruptedTaskOutput, lastTaskText } from "../session/task-output" | ||
| // altimate_change end | ||
| const log = Log.create({ service: "tool.task" }) | ||
|
|
||
|
|
@@ -253,12 +253,17 @@ export const TaskTool = Tool.define("task", async (ctx) => { | |
| if (ctx.abort.aborted) partialOutput ??= await interruptedTaskOutput(session.id, messageID) | ||
|
|
||
| const text = result?.parts.findLast((x) => x.type === "text")?.text ?? "" | ||
| // altimate_change start — a child that ends on an error or with no text must not reach the | ||
| // parent as an empty result, which reads as "done, nothing found". | ||
| const status = text.trim() || ctx.abort.aborted ? [] : await emptyTaskStatus(session.id, messageID, result) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: an errored child with a closed text block still reads as an answer.
Suggested fix: emit the reason whenever Not blocking: pre-existing on main, and the PR's claim is about empty results, so this is safe in a follow-up. |
||
| // altimate_change end | ||
|
|
||
| const output = partialOutput ?? [ | ||
| `task_id: ${session.id} (for resuming to continue this task if needed)`, | ||
| "", | ||
| "<task_result>", | ||
| ...(ctx.abort.aborted ? ["Task interrupted. Findings may be incomplete."] : []), | ||
| ...status, | ||
| text, | ||
| "</task_result>", | ||
| ].join("\n") | ||
|
|
@@ -278,3 +283,27 @@ export const TaskTool = Tool.define("task", async (ctx) => { | |
| }, | ||
| } | ||
| }) | ||
|
|
||
| // altimate_change start — explicit status for a child that produced no final text | ||
| async function emptyTaskStatus( | ||
| sessionID: SessionID, | ||
| messageID: MessageID, | ||
| result: MessageV2.WithParts | undefined, | ||
| ): Promise<string[]> { | ||
| const error = result?.info.role === "assistant" ? result.info.error : undefined | ||
| const reason = error | ||
| ? `Task stopped with an error before giving a final answer (${errorText(error)}).` | ||
| : "Task ended without a final text answer." | ||
| const partial = await lastTaskText(sessionID, messageID).catch(() => undefined) | ||
| return partial | ||
| ? [reason, "Its last written text, which may be incomplete:", partial] | ||
| : [reason, "It wrote no text. Resume it with task_id or treat its work as unfinished."] | ||
| } | ||
|
|
||
| function errorText(error: NonNullable<MessageV2.Assistant["error"]>): string { | ||
| const message = "data" in error && error.data && typeof error.data === "object" && "message" in error.data | ||
| ? String(error.data.message) | ||
| : "" | ||
| return message ? `${error.name}: ${message}` : error.name | ||
| } | ||
| // altimate_change end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| // A child task that ends on an error (for example the request at its step cap) or with | ||
| // no text used to reach the parent as an empty <task_result>, which reads as "done, nothing found". | ||
| import { expect, spyOn, test } from "bun:test" | ||
| import { Effect } from "effect" | ||
| import { Agent } from "../../src/agent/agent" | ||
| import { Config } from "../../src/config/config" | ||
| import { Session } from "../../src/session" | ||
| import { MessageV2 } from "../../src/session/message-v2" | ||
| import { MessageID, SessionID } from "../../src/session/schema" | ||
| import { TaskTool, type TaskPromptOps } from "../../src/tool/task" | ||
| import { initTool } from "../altimate/tool-fixture" | ||
|
|
||
| const cases = [ | ||
| { | ||
| name: "an errored child with earlier text returns the error and that text", | ||
| error: { name: "UnknownError", data: { message: "Could not get a response from the agent." } }, | ||
| text: "", | ||
| earlier: "Found 3 candidates so far.", | ||
| contains: [ | ||
| "Task stopped with an error before giving a final answer (UnknownError: Could not get a response from the agent.).", | ||
| "Its last written text, which may be incomplete:", | ||
| "Found 3 candidates so far.", | ||
| ], | ||
| }, | ||
| { | ||
| name: "a child with no text at all says so", | ||
| error: undefined, | ||
| text: "", | ||
| earlier: undefined, | ||
| contains: [ | ||
| "Task ended without a final text answer.", | ||
| "It wrote no text. Resume it with task_id or treat its work as unfinished.", | ||
| ], | ||
| }, | ||
| { | ||
| name: "a child with a final answer is passed through unchanged", | ||
| error: undefined, | ||
| text: "Child answer", | ||
| earlier: undefined, | ||
| contains: ["<task_result>\nChild answer\n</task_result>"], | ||
| }, | ||
| ] as const | ||
|
|
||
| for (const item of cases) { | ||
| test(item.name, async () => { | ||
| const parentID = SessionID.make("ses_empty_parent") | ||
| const childID = SessionID.make("ses_empty_child") | ||
| const messageID = MessageID.make("msg_empty_parent") | ||
| const agent = { name: "general", mode: "subagent", permission: [], options: {} } as unknown as Agent.Info | ||
| let childPromptID: MessageID | undefined | ||
| const mocks = [ | ||
| spyOn(Agent, "list").mockResolvedValue([agent]), | ||
| spyOn(Agent, "get").mockResolvedValue(agent), | ||
| spyOn(Config, "get").mockResolvedValue({}), | ||
| spyOn(Session, "get").mockImplementation( | ||
| Object.assign( | ||
| async (id: SessionID) => | ||
| ({ | ||
| id, | ||
| permission: [], | ||
| ...(id === childID ? { parentID, metadata: { taskAgent: "general" } } : {}), | ||
| }) as never, | ||
| Session.get, | ||
| ), | ||
| ), | ||
| spyOn(MessageV2, "get").mockResolvedValue({ | ||
| info: { role: "assistant", parentID: messageID, modelID: "test", providerID: "test" }, | ||
| parts: [], | ||
| } as never), | ||
| // Newest first, like the real stream: the failed final message, then an earlier one with text. | ||
| spyOn(MessageV2, "stream").mockImplementation(async function* () { | ||
| yield { info: { role: "assistant", parentID: childPromptID }, parts: [] } as never | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the new tests miss the paths above, and this mock hides the compaction case. The new tests do what they claim. Run against base, the 4 failing cases fail for the stated reasons (role This mock gives every assistant the prompt's Not blocking: test-only. |
||
| if (item.earlier) | ||
| yield { | ||
| info: { role: "assistant", parentID: childPromptID }, | ||
| parts: [{ type: "text", text: item.earlier }], | ||
| } as never | ||
| yield { info: { role: "user" }, parts: [] } as never | ||
| } as never), | ||
| ] | ||
| const promptOps: TaskPromptOps = { | ||
| cancel: () => Effect.void, | ||
| resolvePromptParts: (text) => Effect.succeed([{ type: "text", text }]), | ||
| prompt: (input) => | ||
| Effect.sync(() => { | ||
| childPromptID = input.messageID | ||
| return { | ||
| info: { role: "assistant", ...(item.error ? { error: item.error } : {}) }, | ||
| parts: item.text ? [{ type: "text", text: item.text }] : [], | ||
| } as unknown as MessageV2.WithParts | ||
| }), | ||
| } | ||
| try { | ||
| const tool = await initTool(TaskTool) | ||
| const result = await tool.execute( | ||
| { description: "Inspect records", prompt: "Summarize the records", subagent_type: "general", task_id: childID }, | ||
| { | ||
| sessionID: parentID, | ||
| messageID, | ||
| agent: "build", | ||
| abort: new AbortController().signal, | ||
| messages: [], | ||
| extra: { promptOps, bypassAgentCheck: true }, | ||
| }, | ||
| ) | ||
| for (const text of item.contains) expect(result.output).toContain(text) | ||
| if (item.text) expect(result.output).not.toContain("Task ended without") | ||
| } finally { | ||
| for (const mock of mocks) mock.mockRestore() | ||
| } | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: a user request that joins the running loop after the cap gets no tools.
modelStepslives for the generation and does not reset when a new request joins (wherecurrentTurnchanges). Withsteps: 2and a request queued during the capped request, the follow-up at this head goes out with 0 tools andMAX_STEPSas the trailing user message, after the real question. At base it went out with all 94 tools and the assistantMAX_STEPS.MAX_STEPSsays "Tools are disabled until next user input", and a follow-up sent after idle gets a fresh count, so the outcome depends on timing. The cap text on that request is pre-existing; losing the tools is new.Suggested fix: reset
modelStepswherecurrentTurnmoves to a new request.Not blocking: edge input (a capped agent plus a follow-up queued in that window), and the model can still answer.