Repository navigation
fix(workspace): attach the engine for a message sent while the assistant is busy #1461
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
Open
ralphstodomingo
wants to merge
3
commits into
main
Choose a base branch
from
fix/engine-attach-on-queued-message
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
d84d9ac
fix(workspace): attach the engine for a message sent while the assist…
ralphstodomingo 5c38528
fix(workspace): pick the newest user request by id, not by position
ralphstodomingo bed0007
fix(workspace): run the boundary for any unseen user request, not onl…
ralphstodomingo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // altimate_change - new file | ||
| import type { MessageV2 } from "../../session/message-v2" | ||
|
|
||
| /** | ||
| * The user request a message carries, or undefined when it carries none. | ||
| * | ||
| * This is the rule lesson retrieval set in the prompt loop: a user message is a request when it has a text part that | ||
| * is neither synthetic nor ignored, or a file part. Auto-compaction can replay a user message under a fresh id. While | ||
| * lessons are delivered for the session, the replay names the request it replays (`metadata.learnOriginalMessage` on | ||
| * one of its text parts) and stays that request; otherwise it is a request of its own. | ||
| * | ||
| * The user messages the loop writes itself carry none: the summary after a subtask, the validator retry, the | ||
| * compaction marker and the continue message after compaction. | ||
| */ | ||
| export function requestID(message: MessageV2.WithParts): string | undefined { | ||
| if (message.info.role !== "user") return undefined | ||
| const asked = message.parts.some( | ||
| (part) => (part.type === "text" && !part.synthetic && !part.ignored) || part.type === "file", | ||
| ) | ||
| if (!asked) return undefined | ||
| const original = message.parts.find( | ||
| (part): part is MessageV2.TextPart => | ||
| part.type === "text" && typeof part.metadata?.learnOriginalMessage === "string", | ||
|
ralphstodomingo marked this conversation as resolved.
|
||
| ) | ||
| return String(original?.metadata?.learnOriginalMessage ?? message.info.id) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // altimate_change - new file | ||
| import type { MessageV2 } from "../../session/message-v2" | ||
| import { requestID } from "../session/user-request" | ||
|
|
||
| /** | ||
| * Decides, at each tool catalog of one prompt loop, whether the workspace engine's turn boundary runs. | ||
| * | ||
| * It runs on the loop's first catalog, and again on the first catalog after a new user request joins the loop. A | ||
| * message sent while the assistant is busy does not start a loop of its own: the running loop picks it up. Without the | ||
| * second rule, that message would keep the engine and tool pin the loop started with, even if the engine was installed | ||
| * in between. | ||
| * | ||
| * "New user request" is `requestID` in `altimate/session/user-request.ts`, the rule lesson retrieval uses. The user | ||
| * messages the loop writes itself (subtask summary, validator retry, compaction marker and continue message) carry no | ||
| * request, so none of them move the turn to a new engine mid-task. A compaction replay stays its original request | ||
| * while lessons are delivered; otherwise it counts as new, so the boundary runs once more where the request restarts | ||
| * from the summary. Every request in history is checked, not only the newest: history is not in id order after a | ||
| * compaction that keeps a tail, and a loop-written message or a replay can arrive after a queued request. | ||
| */ | ||
| export function tracker(): (messages: MessageV2.WithParts[]) => boolean { | ||
| let catalogued = false | ||
| const seen = new Set<string>() | ||
| return (messages) => { | ||
| let unseen = false | ||
| for (const message of messages) { | ||
| const request = requestID(message) | ||
| if (request === undefined || seen.has(request)) continue | ||
| seen.add(request) | ||
| unseen = true | ||
| } | ||
| const boundary = !catalogued || unseen | ||
| catalogued = true | ||
| return boundary | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
63 changes: 63 additions & 0 deletions
63
packages/opencode/test/altimate/session/user-request.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // altimate_change - new file | ||
| // | ||
| // Which user messages are requests. The rule is lesson retrieval's (the prompt loop picks a request's | ||
| // lessons by it); the workspace engine's turn boundary reuses it. Each row is the shape one writer | ||
| // produces: the user, or the loop writing a message itself. | ||
| import { describe, expect, test } from "bun:test" | ||
| import type { MessageV2 } from "../../../src/session/message-v2" | ||
| import { requestID } from "../../../src/altimate/session/user-request" | ||
|
|
||
| let counter = 0 | ||
| const next = (prefix: string) => `${prefix}_${String(++counter).padStart(4, "0")}` | ||
|
|
||
| function message(role: "user" | "assistant", parts: Record<string, unknown>[], id = next("msg")): MessageV2.WithParts { | ||
| return { | ||
| info: { id, sessionID: "ses_test", role, time: { created: 0 } }, | ||
| parts: parts.map((part) => ({ id: next("prt"), sessionID: "ses_test", messageID: id, ...part })), | ||
| } as unknown as MessageV2.WithParts | ||
| } | ||
|
|
||
| const text = (value: string, extra: Record<string, unknown> = {}) => ({ type: "text", text: value, ...extra }) | ||
| const file = (mime: string) => ({ type: "file", mime, url: "file:///tmp/notes.txt", filename: "notes.txt" }) | ||
| const ORIGINAL = "msg_original" | ||
|
|
||
| describe("requestID", () => { | ||
| const rows: [string, MessageV2.WithParts, "own" | typeof ORIGINAL | undefined][] = [ | ||
| ["a user's text", message("user", [text("connect jira")]), "own"], | ||
| [ | ||
| "a user's text beside synthetic file expansions", | ||
| message("user", [text("read this"), text("<file contents>", { synthetic: true }), file("text/plain")]), | ||
| "own", | ||
| ], | ||
| ["a user's file alone", message("user", [file("text/plain")]), "own"], | ||
| [ | ||
| "the subtask summary the loop writes", | ||
| message("user", [text("Summarize the task tool output above and continue with your task.", { synthetic: true })]), | ||
| undefined, | ||
| ], | ||
| ["the validator retry the loop writes", message("user", [text("validator failures: ...", { synthetic: true })]), undefined], | ||
| ["the compaction marker", message("user", [{ type: "compaction", auto: true }]), undefined], | ||
| ["the continue message after compaction", message("user", [text("Continue if you have next steps", { synthetic: true })]), undefined], | ||
| ["a message whose only text is ignored", message("user", [text("reminder", { ignored: true })]), undefined], | ||
| [ | ||
| "a replay stamped with the request it replays", | ||
| message("user", [text("connect jira", { metadata: { learnOriginalMessage: ORIGINAL } })]), | ||
| ORIGINAL, | ||
| ], | ||
| [ | ||
| "an attachment-only replay: its file, and the stamp on an ignored carrier", | ||
| message("user", [ | ||
| file("text/plain"), | ||
| text("", { synthetic: true, ignored: true, metadata: { learnOriginalMessage: ORIGINAL } }), | ||
| ]), | ||
| ORIGINAL, | ||
| ], | ||
| ["an unstamped replay (lessons not delivered)", message("user", [text("connect jira")]), "own"], | ||
| ["an assistant message", message("assistant", [text("done")]), undefined], | ||
| ] | ||
| for (const [name, msg, expected] of rows) { | ||
| test(name, () => { | ||
| expect(requestID(msg)).toBe(expected === "own" ? msg.info.id : expected) | ||
| }) | ||
| } | ||
| }) |
114 changes: 114 additions & 0 deletions
114
packages/opencode/test/altimate/workspace/turn-boundary.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // altimate_change - new file | ||
| // | ||
| // When one prompt loop runs the workspace engine's turn boundary. Each test replays the history a | ||
| // loop sees at its successive tool catalogs: the boundary runs on the first catalog and on the first | ||
| // catalog after a new user request joins the loop, never on a message the loop wrote itself. | ||
| import { describe, expect, test } from "bun:test" | ||
| import type { MessageV2 } from "../../../src/session/message-v2" | ||
| import { tracker } from "../../../src/altimate/workspace/turn-boundary" | ||
|
|
||
| let counter = 0 | ||
| const next = (prefix: string) => `${prefix}_${String(++counter).padStart(4, "0")}` | ||
|
|
||
| function message(role: "user" | "assistant", parts: Record<string, unknown>[]): MessageV2.WithParts { | ||
| const id = next("msg") | ||
| return { | ||
| info: { id, sessionID: "ses_test", role, time: { created: 0 } }, | ||
| parts: parts.map((part) => ({ id: next("prt"), sessionID: "ses_test", messageID: id, ...part })), | ||
| } as unknown as MessageV2.WithParts | ||
| } | ||
|
|
||
| const asked = (value: string) => message("user", [{ type: "text", text: value }]) | ||
| const reply = () => message("assistant", [{ type: "text", text: "working" }]) | ||
| const loopWrote = (value: string) => message("user", [{ type: "text", text: value, synthetic: true }]) | ||
| const marker = () => message("user", [{ type: "compaction", auto: true }]) | ||
| const replayOf = (original: MessageV2.WithParts) => | ||
| message("user", [{ type: "text", text: "again", metadata: { learnOriginalMessage: original.info.id } }]) | ||
|
|
||
| /** Runs the tracker over the history as it stood at each catalog; returns which catalogs ran the boundary. */ | ||
| function catalogs(...histories: MessageV2.WithParts[][]): boolean[] { | ||
| const boundary = tracker() | ||
| return histories.map((history) => boundary(history)) | ||
| } | ||
|
|
||
| describe("workspace turn boundary", () => { | ||
| test("runs on the loop's first catalog, then not again for the same request", () => { | ||
| const a = asked("connect jira") | ||
| expect(catalogs([a], [a, reply()], [a, reply(), reply()])).toEqual([true, false, false]) | ||
| }) | ||
|
|
||
| test("runs on the first catalog even when the loop resumes on a message it wrote", () => { | ||
| expect(catalogs([marker(), loopWrote("Continue if you have next steps")])).toEqual([true]) | ||
| }) | ||
|
|
||
| test("does not run for the messages the loop writes itself", () => { | ||
| const a = asked("connect jira") | ||
| const h1 = [a, reply()] | ||
| const h2 = [...h1, loopWrote("Summarize the task tool output above and continue with your task.")] | ||
| const h3 = [...h2, reply(), loopWrote("validator failures: ...")] | ||
| const h4 = [...h3, reply(), marker(), reply(), loopWrote("Continue if you have next steps")] | ||
| expect(catalogs([a], h2, h3, h4)).toEqual([true, false, false, false]) | ||
| }) | ||
|
|
||
| test("does not run for a stamped compaction replay of the current request", () => { | ||
| const a = asked("connect jira") | ||
| expect(catalogs([a], [a, reply(), marker(), reply(), replayOf(a)])).toEqual([true, false]) | ||
| }) | ||
|
|
||
| test("runs once for a message queued while the assistant was busy", () => { | ||
| const a = asked("first") | ||
| const b = asked("now use the engine") | ||
| const h1 = [a, reply()] | ||
| const h2 = [...h1, b] | ||
| const h3 = [...h2, reply()] | ||
| expect(catalogs([a], h2, h3)).toEqual([true, true, false]) | ||
| }) | ||
|
|
||
| test("runs for a queued message even when the loop writes one after it before the next catalog", () => { | ||
| const a = asked("first") | ||
| const b = asked("/review with a subtask") | ||
| const h2 = [a, reply(), b, reply(), loopWrote("Summarize the task tool output above and continue with your task.")] | ||
| expect(catalogs([a], h2)).toEqual([true, true]) | ||
| }) | ||
|
|
||
| test("runs for a message queued during a compaction that keeps a tail, which reorders history", () => { | ||
| // The trace from review: request A is catalogued; compaction starts and keeps A as its tail; B | ||
| // is queued after the marker, before the summary is saved; compaction writes its continuation. | ||
| // `filterCompacted` returns [marker, B, summary, A, continuation]: A last by position, B newest by id. | ||
| const a = asked("first") | ||
| const m = marker() | ||
| const b = asked("sent during compaction") | ||
| const summary = reply() | ||
| const continuation = loopWrote("Continue if you have next steps") | ||
| expect(catalogs([a], [m, b, summary, a, continuation])).toEqual([true, true]) | ||
| }) | ||
|
|
||
| test("runs for a queued message that a stamped replay arrives after", () => { | ||
| // From review: A is catalogued; B is queued while a compaction runs, and the compaction then | ||
| // writes a stamped replay of A. The replay is newest by id but maps back to A, already seen. | ||
| const a = asked("first") | ||
| const b = asked("sent during compaction") | ||
| expect(catalogs([a], [a, reply(), marker(), b, reply(), replayOf(a)])).toEqual([true, true]) | ||
| }) | ||
|
|
||
| test("does not run again for a stamped replay of an earlier request", () => { | ||
| const a = asked("first") | ||
| const b = asked("second") | ||
| const h2 = [a, reply(), b] | ||
| const h3 = [...h2, reply(), marker(), reply(), replayOf(a)] | ||
| expect(catalogs([a], h2, h3)).toEqual([true, true, false]) | ||
| }) | ||
|
|
||
| test("runs once more for an unstamped replay, where the request restarts from the summary", () => { | ||
| // With lessons not delivered, compaction does not stamp the replay, so it is a request of its own. | ||
| const a = asked("first") | ||
| const h2 = [a, reply(), marker(), reply(), asked("first")] | ||
| expect(catalogs([a], h2)).toEqual([true, true]) | ||
| }) | ||
|
|
||
| test("each loop keeps its own record", () => { | ||
| const a = asked("first") | ||
| expect(catalogs([a], [a])).toEqual([true, false]) | ||
| expect(catalogs([a])).toEqual([true]) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
WARNING: Count subtask-only commands as user requests for the turn boundary
The built-in
/reviewcommand hassubtask: true(command/index.ts) andSessionPrompt.command()saves it as a user message containing only asubtaskpart. If it is queued while request A is busy, the running loop executes the subtask and then writes a synthetic summary; neither message passes this text-or-file check.latestRequestID()still returns A, so the next catalog skipsatTurnStart()and the command's parent continuation keeps A's engine and tool pin, even when an engine was installed between requests. The tracker needs to recognize a user-authoredsubtaskas a new boundary without treating the loop-written summary as one; the queued-subtask test currently models/reviewas ordinary text.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.