Skip to content

fix(cursor-cli-oauth): keep tool protocol out of assistant text - #1107

Merged
code-yeongyu merged 4 commits into
code-yeongyu:mainfrom
eddieparc:cursor-cli-oauth-hide-tool-frames
Aug 29, 2026
Merged

fix(cursor-cli-oauth): keep tool protocol out of assistant text#1107
code-yeongyu merged 4 commits into
code-yeongyu:mainfrom
eddieparc:cursor-cli-oauth-hide-tool-frames

Conversation

@eddieparc

@eddieparc eddieparc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop mapping Cursor CLI internal tool_call protocol into assistant text
  • keep Cursor-executed operations out of host toolCall blocks so Senpi cannot execute them a second time
  • add a regression proving only final assistant prose reaches text deltas and stored content

User-facing OmO issue: code-yeongyu/oh-my-openagent#7169

Root cause

renderToolFrame() serialized Cursor started and completed tool events, including arguments and results, into <cursor-cli-tool> strings. appendToolFrame() sent those strings through pushTextDelta(), so consumers persisted and rendered provider-internal JSON as assistant prose.

Cursor already executed these tools in its subprocess. Suppressing the protocol frames at the provider boundary avoids both conversation-context contamination and duplicate host-side execution.

Validation

  • focused regression: 13/13 passed
  • npm run check: passed
  • npm run build: passed
  • full workspace suite: 8,685 passed, 36 skipped; one unrelated timestamp precision assertion failed and its isolated rerun passed 6/6
  • real provider QA: locally built Senpi returned the independently confirmed result 22 and emitted no <cursor-cli-tool> or serialized tool JSON in visible output

Summary by cubic

Keeps Cursor CLI tool protocol out of assistant text and prevents duplicate host execution of tools Cursor already ran. Previously tool_call events were serialized into <cursor-cli-tool> frames and pushed through text deltas; now they're suppressed at the provider boundary, and the open text segment is closed and snapshot tracking reset so post-tool prose isn't lost.

  • tool_call events no longer emit host toolCall blocks; Cursor still executes tools in its subprocess.
  • Text deltas and stored messages contain only the assistant's prose, split at each tool boundary with no empty blocks.
  • Regression in stream.test.ts verifies only model prose appears before and after tool frames, including a post-tool cumulative fragment.
  • AGENTS.md, changes.md, and CHANGELOG.md document the behavior and resolve the concurrent release entry.
  • Remove downstream parsing of <cursor-cli-tool> assistant text; those frames no longer emit.

Written for commit e53a28c. Summary will update on new commits.

Review in cubic

Keep Cursor-executed tool protocol out of assistant text and host tool calls, with regression coverage and provider documentation.

OmO issue: code-yeongyu/oh-my-openagent#7169

Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
Resolve the concurrent release changelog while preserving the Cursor provider fix.

Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST CHANGES

B1. The PR is currently unmergeable: GitHub reports mergeable: CONFLICTING / mergeStateStatus: DIRTY. Rebase or merge the current main, resolve the conflicts (at minimum the concurrently modified coding-agent changelog and any other conflict markers), and rerun the required checks on the resolved tree. Check and test and Changelog gate are green for the stale PR head, but that does not validate a merge result.

B2. The suppression is not behaviorally equivalent to hiding the frame. The deleted appendToolFrame() path called ensureOpen(mapper, "tool"); when text was open, that closed the text block and openBlock() reset textAccumulated. The new break leaves both the current text block and the old cumulative-snapshot accumulator alive across a tool boundary. A legitimate incremental assistant fragment after a tool can therefore begin with the text already emitted before that tool and be misclassified by appendAssistantFragment() as a cumulative snapshot; its matching prefix is stripped and real prose is silently lost. Preserve the protocol's text-segment boundary/deduplication semantics while suppressing the frame, and cover the exact before-tool / tool / after-tool ordering.

B3. The replacement regression is too weak to establish the claimed safety. toolsTurnFixture() emits tool frames before any assistant prose and only asserts one final TOOLS OK fragment. It does not exercise the production captures' assistant prose before and after tool calls, nor a post-tool fragment that shares a prefix with prior text. Add an integration regression that asserts exact text deltas and stored content for those interleavings (including both started/completed frames); otherwise the central "no legitimate text swallowed" failure remains untested.

The intended provider boundary is reasonable, and the parser's split-chunk handling is unchanged, but these blockers must be resolved before this can merge.

@@ -651,7 +624,9 @@ export function streamCursorCliOauth(
for (const block of event.message.content) appendAssistantFragment(mapper, block.text);
break;
case "tool_call":

Copy link
Copy Markdown
Owner

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 reached ensureOpen(mapper, "tool"), which reset textAccumulated through openBlock(). 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.

Copy link
Copy Markdown
Owner

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.

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"]);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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.

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-2 re-review at e53a28c.

B1 (round-1 conflict with main): RESOLVED. The PR is now MERGEABLE, and the merge commit 0d4a5c4 has both the prior PR tree and current origin/main as parents. The merged coding-agent changelog retains the Cursor entry and the concurrent main entries; the surrounding PR files remain present. git diff --check is clean. The only ||||||| line in the current tree is pre-existing in origin/main and the PR's pre-merge parent, not introduced by this merge.

B2 (textAccumulated boundary state): RESOLVED. resetTextSegment() calls closeOpen() and clears textAccumulated; the tool_call switch invokes it for every parsed tool event. The parser emits both started and completed shapes as tool_call, so started/completed, interleaved, and consecutive events all take the reset path. Consecutive resets are harmless because the second closeOpen() is a no-op, and the next assistant fragment opens a fresh text block with a clean accumulator.

B3 (regression coverage): RESOLVED. The updated hermetic fixture emits assistant prose before the tool, both started and completed frames, then a post-tool cumulative fragment sharing the BEFORE TOOL prefix. The test asserts exact text deltas and exact persisted text blocks, and checks that protocol tags, tool kind, and tool output are absent. Removing the reset would make the second expected delta/block fail because BEFORE TOOL would be stripped.

New-issue review: no defensible blocker found. The reset does not create empty blocks when no text is open, does not expose or execute provider tools, preserves the existing parser/router behavior, and is isolated to the Cursor CLI OAuth stream. Source and test diagnostics are clean; the focused stream suite passes 13/13, and all PR checks pass.

@code-yeongyu
code-yeongyu merged commit ac2f171 into code-yeongyu:main Aug 29, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants