-
Notifications
You must be signed in to change notification settings - Fork 121
fix(sdk): add Query.continue() to resume after a failed model turn #82
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 |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { Agent } from "@mariozechner/pi-agent-core"; | ||
| import type { AgentEvent, AgentTool } from "@mariozechner/pi-agent-core"; | ||
| import type { AgentEvent, AgentMessage, AgentTool } from "@mariozechner/pi-agent-core"; | ||
| import type { AssistantMessage } from "@mariozechner/pi-ai"; | ||
| import { loadAgent } from "./loader.js"; | ||
| import type { AgentManifest } from "./loader.js"; | ||
|
|
@@ -35,6 +35,9 @@ import { | |
| interface Channel<T> { | ||
| push(v: T): void; | ||
| finish(): void; | ||
| // Undoes finish() so a completed query can resume streaming — used by | ||
| // Query.continue() to reopen the channel after the run first went idle. | ||
| reopen(): void; | ||
| pull(): Promise<IteratorResult<T>>; | ||
| } | ||
|
|
||
|
|
@@ -59,6 +62,9 @@ function createChannel<T>(): Channel<T> { | |
| resolve = null; | ||
| } | ||
| }, | ||
| reopen() { | ||
| done = false; | ||
| }, | ||
| pull(): Promise<IteratorResult<T>> { | ||
| if (buffer.length) { | ||
| return Promise.resolve({ value: buffer.shift()!, done: false }); | ||
|
|
@@ -73,6 +79,14 @@ function createChannel<T>(): Channel<T> { | |
|
|
||
| // ── Extract text/thinking from AssistantMessage ──────────────────────── | ||
|
|
||
| function toUserMessage(text: string): AgentMessage { | ||
| return { | ||
| role: "user", | ||
| content: [{ type: "text", text }], | ||
| timestamp: Date.now(), | ||
| } as AgentMessage; | ||
| } | ||
|
|
||
| function extractContent(msg: AssistantMessage): { text: string; thinking: string } { | ||
| let text = ""; | ||
| let thinking = ""; | ||
|
|
@@ -111,6 +125,9 @@ export function query(options: QueryOptions): Query { | |
| let sandboxCtx: SandboxContext | undefined; | ||
| // Local session (hoisted for cleanup in catch) | ||
| let localSession: LocalSession | undefined; | ||
| // Agent instance (hoisted so steer()/continue() can reach it once the | ||
| // agent is loaded, including after the initial run has gone idle) | ||
| let agent: Agent | undefined; | ||
|
|
||
| // OpenTelemetry session span — opened immediately so it covers agent | ||
| // load + prompt + cleanup. Closed in the IIFE's finally so every exit | ||
|
|
@@ -299,14 +316,17 @@ export function query(options: QueryOptions): Query { | |
| } | ||
|
|
||
| // 8. Create Agent | ||
| const agent = new Agent({ | ||
| agent = new Agent({ | ||
| initialState: { | ||
| systemPrompt, | ||
| model: loaded.model, | ||
| tools, | ||
| ...modelOptions, | ||
| }, | ||
| }); | ||
| // Non-null alias: `agent` is a `let` captured by closures below, so TS | ||
| // can't narrow it past the assignment above once inside them. | ||
| const activeAgent = agent; | ||
|
|
||
| // 9. Subscribe to events and map to GCMessage | ||
| agent.subscribe((event: AgentEvent) => { | ||
|
|
@@ -508,7 +528,7 @@ export function query(options: QueryOptions): Query { | |
| } | ||
| } | ||
| await otelContext.with(_session.ctx, () => | ||
| agent.prompt(options.prompt as string), | ||
| activeAgent.prompt(options.prompt as string), | ||
| ); | ||
| } else { | ||
| // Multi-turn: iterate the async iterable | ||
|
|
@@ -532,7 +552,7 @@ export function query(options: QueryOptions): Query { | |
| } | ||
| } | ||
| await otelContext.with(_session.ctx, () => | ||
| agent.prompt(userMsg.content), | ||
| activeAgent.prompt(userMsg.content), | ||
| ); | ||
| } | ||
| } | ||
|
|
@@ -593,7 +613,51 @@ export function query(options: QueryOptions): Query { | |
| ac.abort(); | ||
| }, | ||
|
|
||
| steer(_message: string) { | ||
| steer(message: string) { | ||
| if (!agent) { | ||
| throw new Error("Cannot steer: agent not yet initialized (query hasn't started)"); | ||
| } | ||
| agent.steer(toUserMessage(message)); | ||
| }, | ||
|
|
||
| async continue(message?: string) { | ||
| if (!agent) { | ||
| throw new Error("Cannot continue: agent not yet initialized (query hasn't started)"); | ||
| } | ||
| if (agent.signal) { | ||
| throw new Error("Cannot continue: agent is still processing a turn"); | ||
|
Contributor
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. Bug 1 (confirmed, fix incomplete): this guard fires a false positive when The sequence: Two options:
|
||
| } | ||
|
|
||
| // The initial run may have already reached agent_end and finished | ||
| // the channel — reopen it so further messages can stream out. | ||
| channel.reopen(); | ||
|
|
||
| try { | ||
| if (message !== undefined) { | ||
| pushMsg({ type: "user", content: message }); | ||
| agent.followUp(toUserMessage(message)); | ||
|
Contributor
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. Bug 4 (new, minor but observable): Consider pushing the user message only after |
||
| } else { | ||
|
Contributor
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. Bug 2 (confirmed, triggering condition narrower than stated): In the current code paths this is handled: |
||
| // No new input: retry the turn the model failed to answer. | ||
| // Drop the failed assistant message so the transcript ends | ||
| // on the user/tool-result message it never responded to — | ||
| // pi-agent-core's continue() requires that to retry in place. | ||
| const messages = agent.state.messages as any[]; | ||
| const last = messages[messages.length - 1]; | ||
| if (last?.role === "assistant" && (last.stopReason === "error" || last.stopReason === "aborted")) { | ||
| agent.state.messages = messages.slice(0, -1); | ||
| } | ||
| } | ||
|
|
||
| await otelContext.with(_session.ctx, () => (agent as Agent).continue()); | ||
| } catch (err: any) { | ||
|
Contributor
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. Bug 5 (new, minor): The session span should remain open until the |
||
| pushMsg({ | ||
| type: "system", | ||
| subtype: "error", | ||
| content: err.message, | ||
| }); | ||
| channel.finish(); | ||
| throw err; | ||
| } | ||
| }, | ||
|
|
||
| sessionId() { | ||
|
|
||
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.
Bug 3 (new, moderate): every
continue()call re-emits asession_startsystem message into the channel. Theagent_startevent fires again at the top ofrunWithLifecycle->runAgentLoopContinuefor both paths (with and without a new message), and the subscriber that handlesagent_startunconditionally emits{ type: "system", subtype: "session_start" }. A query that fails and is continued twice will produce threesession_startand threesession_endmessages within the sameQuerylifetime.Any consumer that treats
session_startas a once-per-query lifecycle marker — initialising UI state, starting timers, logging a session — will misfire on everycontinue()call. Consider gating the emit on a flag (e.g.let sessionStartEmitted = false) so it only fires once perQuery.