-
Notifications
You must be signed in to change notification settings - Fork 83
fix(cursor-cli-oauth): keep tool protocol out of assistant text #1107
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 2 commits
2b9512c
29ed15f
0d4a5c4
e53a28c
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 |
|---|---|---|
|
|
@@ -321,7 +321,7 @@ describe("cursor-cli-oauth stream mapping", () => { | |
| }); | ||
| }); | ||
|
|
||
| it("renders tool frames as display-only blocks labelled as executed by the Cursor CLI", async () => { | ||
| it("keeps Cursor CLI tool protocol frames out of assistant text", async () => { | ||
| const directory = temporaryDirectory(); | ||
| const deps: CursorCliStreamDeps = { | ||
| cwd: directory, | ||
|
|
@@ -336,12 +336,12 @@ describe("cursor-cli-oauth stream mapping", () => { | |
| const message = doneMessage(events); | ||
|
|
||
| expect(message.content.some((block) => block.type === "toolCall")).toBe(false); | ||
| const rendered = textBlocks(message).join("\n"); | ||
| expect(rendered).toContain("executed by the Cursor CLI"); | ||
| // Untrusted tool output stays inside the delimited display region. | ||
| const withoutDisplayRegions = rendered.replace(/<cursor-cli-tool>[\s\S]*?<\/cursor-cli-tool>/g, ""); | ||
| expect(withoutDisplayRegions).not.toContain("tooltest-force-77"); | ||
| expect(rendered).toContain("tooltest-force-77"); | ||
| expect(textDeltas(events)).toEqual(["TOOLS OK"]); | ||
|
Owner
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. B3: This fixture has no assistant text before the tool frames, so it cannot detect the state/deduplication regression at the changed branch. Exercise prose before and after both tool events, including a post-tool fragment sharing a prefix with the pre-tool text, and assert exact deltas plus persisted text blocks.
Owner
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. Expanded the integration fixture in e53a28c to emit prose before started/completed tool frames and a post-tool fragment sharing that prefix; it asserts exact text deltas and persisted text blocks. |
||
| expect(textBlocks(message)).toEqual(["TOOLS OK"]); | ||
| const rendered = textBlocks(message).join(""); | ||
| expect(rendered).not.toContain("<cursor-cli-tool>"); | ||
| expect(rendered).not.toContain("shellToolCall"); | ||
| expect(rendered).not.toContain("tooltest-force-77"); | ||
| }); | ||
|
|
||
| it("surfaces a zero-exit turn with no assistant events as an error, never an empty success", async () => { | ||
|
|
||
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.
B2: This no-op drops the old tool-boundary state transition as well as the visible frame.
appendToolFrame()previously reachedensureOpen(mapper, "tool"), which resettextAccumulatedthroughopenBlock(). With the no-op, a valid post-tool incremental fragment that starts with the earlier prose is treated as a cumulative snapshot and its prefix is removed. Suppress the tool output without losing that boundary semantics.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.
Fixed in e53a28c: hidden tool frames now close the prior text segment and reset cumulative-snapshot tracking without emitting or storing a tool block, so post-tool prose is preserved.