fix(cursor-cli-oauth): keep tool protocol out of assistant text - #1107
Conversation
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
left a comment
There was a problem hiding this comment.
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": | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Summary
tool_callprotocol into assistant texttoolCallblocks so Senpi cannot execute them a second timeUser-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 throughpushTextDelta(), 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
npm run check: passednpm run build: passed22and emitted no<cursor-cli-tool>or serialized tool JSON in visible outputSummary by cubic
Keeps Cursor CLI tool protocol out of assistant text and prevents duplicate host execution of tools Cursor already ran. Previously
tool_callevents 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_callevents no longer emit hosttoolCallblocks; Cursor still executes tools in its subprocess.stream.test.tsverifies only model prose appears before and after tool frames, including a post-tool cumulative fragment.AGENTS.md,changes.md, andCHANGELOG.mddocument the behavior and resolve the concurrent release entry.<cursor-cli-tool>assistant text; those frames no longer emit.Written for commit e53a28c. Summary will update on new commits.