fix(cursor): split input messages into delta + full, fix composer-2.5-fast s2 - #64
Conversation
…-fast s2 - Add gen_ai.input.messages_delta (per-step increment) alongside existing gen_ai.input.messages (cumulative) on llm.request records - Deep-clone both arrays via JSON.parse(JSON.stringify) to isolate emitted records from later cumulative mutations - Fix composer-2.5-fast missing s2: when afterAgentResponse arrives with no prior afterAgentThought, open s1 for buffered tools first, then let the normal branch open s2 for the final response text
linrunqi08
left a comment
There was a problem hiding this comment.
Code Review — PR #64
Overall this is solid work: the delta + full split is well-motivated, the deep-clone isolation is correct, and the new tests are thorough. A few findings worth considering:
1. [Simplification] Three near-identical deltaMessages construction blocks
Files: react-assembler.mjs — openNewStep() (~L369), composer-2.5-fast block (~L416), tools-only epilogue (~L529)
Each site repeats the same ~10-line pattern:
const deltaMessages = [];
if (<isFirst> && <userPrompt>) { deltaMessages.push({role:'user', ...}); }
if (previousToolResults.length > 0) {
const toolMessage = toolResultsToMessage(previousToolResults);
deltaMessages.push(toolMessage);
cumulativeInputMessages.push(toolMessage);
}But each uses a different "isFirst" idiom: isFirst && userPrompt vs stepRound === 1 && ctx.userPrompt vs isFirstStep && ctx.userPrompt. A single buildDeltaAndUpdateCumulative(isFirst, prompt, toolResults, cumulative) helper would eliminate the three-way maintenance burden and prevent future fixes from missing one site.
2. [Altitude] composer-2.5-fast block duplicates openNewStep logic
File: react-assembler.mjs ~L411-438
The 30-line inline block manually replicates openNewStep's step-opening sequence (increment stepRound, create stepId, initialize stepToolCalls, build delta, call buildLlmRequestWithTs) but with subtle divergences:
- Does NOT call
finalizeCurrentLlmResponse()(safe today, sincecurrentLlmResponseis null) - Does NOT reset
previousToolResults = []after consuming it (relies onopenNewStepfor s2 to do it) - Uses
stepRound === 1(post-increment) to detect first step vsopenNewStep's caller-suppliedisFirst(pre-increment)
If openNewStep were generalized to accept an optional synthetic event and empty-response flag, this block could collapse to a single function call, removing the risk of the two paths silently diverging.
3. [Fragility] previousToolResults not reset in composer-2.5-fast path
File: react-assembler.mjs ~L425-437
After checking previousToolResults at L425 (currently always empty) and emitting the s1 llm.request, the fast path does NOT reset previousToolResults = []. Then flushPendingTools at L437 pushes pendingToolResults into previousToolResults. When openNewStep runs for s2, it consumes the flushed items and resets — so it's correct today.
However, if a future change ever populates previousToolResults before the fast path, those items would be: (a) pushed to cumulativeInputMessages at L428, (b) NOT cleared, (c) combined with flushed items, and (d) pushed to cumulativeInputMessages again in s2's openNewStep — producing duplicate tool-role messages in gen_ai.input.messages for all subsequent steps. Adding previousToolResults = [] after L432 (mirroring openNewStep L389) would close this gap.
4. [Fragility] Implicit ordering dependency between delta computation and flushPendingTools
File: react-assembler.mjs ~L425-437
In the fast path, s1's delta is computed before flushPendingTools (so previousToolResults is empty, correct). In the normal afterAgentThought path, openNewStep is called first (which reads previousToolResults), then flushPendingTools runs after (L398). The ordering is opposite between the two paths. If someone reorders the fast-path block to call flushPendingTools before delta computation (to "clean up"), s1 would incorrectly include tool results in its delta while s2 would get an empty delta. A brief comment documenting this ordering invariant would prevent the footgun.
5. [Efficiency] cloneMessages uses JSON.parse(JSON.stringify) — consider structuredClone
File: react-assembler.mjs L585
structuredClone is available in Node.js 17+ and is faster for this workload (pure data objects). It also preserves undefined values inside arrays (JSON round-tripping silently converts them to null), which could matter if a message part ever has an optional undefined field. Minor improvement, but worth considering for consistency.
6. [Consistency] previousToolResults nullability guard inconsistent across three sites
File: react-assembler.mjs
openNewStepL376:previousToolResults && previousToolResults.length > 0(nullability guard)- Fast path L425:
previousToolResults.length > 0(no guard) - Epilogue L536:
previousToolResults.length > 0(no guard)
previousToolResults is initialized as [] and always reassigned to [], never null/undefined, so the guard in openNewStep is redundant. Cosmetic, but the inconsistency signals uncertainty about the variable's lifecycle.
Summary: No correctness bugs found in the current code paths — the delta/full logic, deep-clone isolation, and composer-2.5-fast handling are all correct. The main concern is the duplicated delta-construction logic across three sites with subtly different "isFirst" checks, which creates a maintenance risk for future changes. Consider extracting a shared helper. Nice test coverage! 🎯
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Implements delta + full input message split on Cursor llm.request records and fixes the composer-2.5-fast missing s2 path. The deep-clone isolation via cloneMessages is correct, the buildLlmRequestWithTs refactor is clean, and the test coverage is solid (delta/full assertions, deep-clone verification, composer-2.5-fast scenario). Code is correct from independent review.
Agree with @linrunqi08's findings on the duplicated deltaMessages construction across three sites — extracting a shared helper would reduce maintenance risk. One additional observation below.
Findings
- [Info]
gen_ai.input.messagesnow carries the full cumulative context rather than step-local input — a behavioral change in the telemetry data shape worth a CHANGELOG note.
Automated review by github-manager-bot
| 'gen_ai.input.messages_delta': deltaMessages && deltaMessages.length > 0 | ||
| ? cloneMessages(deltaMessages) | ||
| : undefined, | ||
| 'gen_ai.input.messages': fullMessages && fullMessages.length > 0 |
There was a problem hiding this comment.
[Info] Behavioral change — gen_ai.input.messages now carries full cumulative context. Previously the guard userPrompt && (!prevToolResults || prevToolResults.length === 0) meant s2+ steps had gen_ai.input.messages = [tool_result] only (no user prompt). Now gen_ai.input.messages includes the full conversation context (user prompt + all prior tool results). This is correct per GenAI semantic conventions (the attribute should represent the complete LLM input), but downstream dashboards/exporters parsing this attribute will now see larger arrays on later steps. A CHANGELOG note would help consumers.
- Replace three near-identical deltaMessages construction blocks (each with subtly different "isFirst" idiom) with a single buildDeltaMessages helper - Add previousToolResults = [] reset in composer-2.5-fast path to prevent future duplicate tool messages in cumulative input - Document ordering invariant: delta must be computed before flushPendingTools
Summary
llm.requestrecords now carry bothgen_ai.input.messages_delta(本 step 增量)和gen_ai.input.messages(累积全量),两者均通过JSON.parse(JSON.stringify)深拷贝隔离,防止后续 cumulative 修改污染已 emit 的记录afterAgentThought,afterAgentResponse到来时缓冲的 tools 还没有 step 承载。新增 pre-branch 先开 s1 承载工具调用,再让正常分支开 s2 承载最终文本响应openNewStep和 tools-only 兜底分支统一改为构造deltaMessages+cumulativeInputMessages双数组,由buildLlmRequestWithTs接收新签名(reqTs, ev, ctx, stepId, deltaMessages, fullMessages, timeSource)Test plan
cursor-react-assembler.test.ts9/9 pass(含 2 个 skip)gen_ai.input.messages拆分为 delta + full 校验cursor-source-event.test.mjs2/2 pass(未修改)