-
Notifications
You must be signed in to change notification settings - Fork 41
fix(opencode): order legacy message loop by time #149
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
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 |
|---|---|---|
|
|
@@ -460,6 +460,46 @@ noLLMServer.instance( | |
| { config: cfg }, | ||
| ) | ||
|
|
||
| noLLMServer.instance( | ||
| "loop exits for a completed parent turn with nonmonotonic message IDs", | ||
| () => | ||
| Effect.gen(function* () { | ||
| const prompt = yield* SessionPrompt.Service | ||
| const sessions = yield* Session.Service | ||
| const chat = yield* sessions.create({ title: "Pinned" }) | ||
| const userID = MessageID.make("msg_z_user") | ||
| const assistantID = MessageID.make("msg_a_assistant") | ||
| yield* sessions.updateMessage({ | ||
| id: userID, | ||
| role: "user", | ||
| sessionID: chat.id, | ||
| agent: "build", | ||
| model: ref, | ||
| time: { created: 100 }, | ||
| }) | ||
| yield* sessions.updateMessage({ | ||
| id: assistantID, | ||
| role: "assistant", | ||
| parentID: userID, | ||
| sessionID: chat.id, | ||
| mode: "build", | ||
| agent: "build", | ||
| cost: 0, | ||
| path: { cwd: "/tmp", root: "/tmp" }, | ||
| tokens: { input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, | ||
| modelID: ref.modelID, | ||
| providerID: ref.providerID, | ||
| time: { created: 200, completed: 201 }, | ||
| finish: "stop", | ||
| }) | ||
|
|
||
| const result = yield* prompt.loop({ sessionID: chat.id }) | ||
|
|
||
| expect(result.info.id).toBe(assistantID) | ||
|
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. P3: This test does not discriminate the fix: with a single user message and a single assistant message, Prompt for AI agents |
||
| }), | ||
| { config: cfg }, | ||
| ) | ||
|
|
||
| it.instance("loop exits without an LLM request for interrupted orphan tool calls", () => | ||
| Effect.gen(function* () { | ||
| const { llm } = yield* useServerConfig(providerCfg) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
P2: The new exit condition
lastAssistant.parentID === lastUser.idnever holds when the final completed assistant message has no resolvableparentID, which can happen for the restored/imported sessions this PR targets (session fork/import in session.ts only preserves parentID when the parent id resolves inidMap). In that case the loop does not recognize the completed turn and issues an unwanted extra LLM call before self-healing. Consider treating a missing/unmatched parentID (e.g. no later user than the assistant's parent by time) as a completed turn so restored legacy sessions exit without a redundant request.Prompt for AI agents