Strip internal Git context tags from agent responses - #4343
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1dee2ec4-d6d9-40ef-aa89-b76c28e714ef) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32fc2475bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The overall approach is sound: a streaming-aware sanitizer that holds back partial markers, applied both to display text and to the finalized AI SDK messages, plus a rules-doc update. The diff is complete (not truncated), so confidence in the analysis below is good. Two issues stand out. The blocking one is that stripping a text part down to nothing produces an empty text part that is then persisted into Issues Summary
🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
github-actions
Bot
added
the
needs-human:review-issue
32fc247 to
1d88652
Compare
1d88652 to
1c8ea0a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c8ea0aea9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Sanitize reasoning and cross-part assistant content safely\n- Bound incomplete tag buffering and preserve Unicode indexes\n- Drop empty assistant blocks and flush text at stream boundaries
50cdc41 to
197211f
Compare
197211f to
fb9a903
Compare
Drop short, distinctive Git-context prefixes when a stream ends.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bae88afd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🤖 Claude Code Review SummaryPR Confidence: 3/5All known review findings are resolved and local tests, formatting, lint, and type-checks pass, but the latest CI/review cycle remains pending and the automated Codex review has a runner authentication failure. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
- Drop modified provider-bound reasoning parts\n- Discard exact unterminated Git-context markers
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bb9dbc7fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const sanitizer = new GitContextEchoSanitizer(); | ||
| const sanitizedTextByIndex = new Map<number, string>(); | ||
| let lastSanitizedPartIndex = -1; | ||
|
|
||
| message.content.forEach((part, index) => { | ||
| if (part.type === "text" || part.type === "reasoning") { | ||
| sanitizedTextByIndex.set(index, sanitizer.push(part.text)); |
There was a problem hiding this comment.
When an assistant reasoning or text part ends with any prefix of the marker—even a lone <—and another text-like part follows, this shared sanitizer carries the buffered suffix into that later part. For example, signed reasoning ending in < is rewritten without that character, causing the providerOptions branch below to discard the entire reasoning block while prepending < to the answer, even though no Git-context tag existed; this corrupts persisted aiMessagesJson and subsequent replay. Flush while preserving the owning content part, or maintain separate state across only genuinely contiguous compatible parts. rules/local-agent-tools.mdL231-L231
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Confidence score: 2/5
- In
src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts, a provider-bound reasoning part can be dropped when it ends with a buffered partial marker even without a Git-context tag, while the suffix is moved into following text; preserve the reasoning content and add coverage for this boundary case. - In
src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts, the<=bound also treats full-length markers as partial matches, making the helper’s behavior inconsistent with its name and framing; restrict the match to strict prefixes or update the helper contract and tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts:105">
P1: When a provider-bound reasoning part merely ends with a buffered partial marker, this `return` drops the entire reasoning part even though no Git-context tag exists, while the buffered suffix moves into the next text part. Preserve the partial suffix with its owning part or isolate sanitizer state to genuinely contiguous compatible parts before discarding the reasoning part.</violation>
<violation number="2" location="src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts:160">
P3: Changing the bound to `<=` makes `startsWithDistinctivePartialMarker` match full (equal-length) markers, not just strict prefixes, so its name and the 'strip partial tags' framing are now inaccurate for the equal-length case. Rename it to something like `startsWithOrEqualsGitTagMarker` (or add a comment) so future readers don't assume full, unclosed markers are treated differently than the current fix intends.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| text !== part.text && | ||
| part.providerOptions | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
P1: When a provider-bound reasoning part merely ends with a buffered partial marker, this return drops the entire reasoning part even though no Git-context tag exists, while the buffered suffix moves into the next text part. Preserve the partial suffix with its owning part or isolate sanitizer state to genuinely contiguous compatible parts before discarding the reasoning part.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts, line 105:
<comment>When a provider-bound reasoning part merely ends with a buffered partial marker, this `return` drops the entire reasoning part even though no Git-context tag exists, while the buffered suffix moves into the next text part. Preserve the partial suffix with its owning part or isolate sanitizer state to genuinely contiguous compatible parts before discarding the reasoning part.</comment>
<file context>
@@ -97,6 +97,13 @@ export function stripGitContextEchoesFromAssistantMessages(
+ text !== part.text &&
+ part.providerOptions
+ ) {
+ return;
+ }
content.push({ ...part, text });
</file context>
| normalized.length >= minimumDistinctivePrefix.length && | ||
| GIT_CONTEXT_TAG_MARKERS.some( | ||
| (marker) => | ||
| normalized.length <= marker.length && marker.startsWith(normalized), |
There was a problem hiding this comment.
P3: Changing the bound to <= makes startsWithDistinctivePartialMarker match full (equal-length) markers, not just strict prefixes, so its name and the 'strip partial tags' framing are now inaccurate for the equal-length case. Rename it to something like startsWithOrEqualsGitTagMarker (or add a comment) so future readers don't assume full, unclosed markers are treated differently than the current fix intends.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pro/main/ipc/handlers/local_agent/git_context_sanitizer.ts, line 160:
<comment>Changing the bound to `<=` makes `startsWithDistinctivePartialMarker` match full (equal-length) markers, not just strict prefixes, so its name and the 'strip partial tags' framing are now inaccurate for the equal-length case. Rename it to something like `startsWithOrEqualsGitTagMarker` (or add a comment) so future readers don't assume full, unclosed markers are treated differently than the current fix intends.</comment>
<file context>
@@ -150,7 +157,7 @@ function startsWithDistinctivePartialMarker(text: string): boolean {
GIT_CONTEXT_TAG_MARKERS.some(
(marker) =>
- normalized.length < marker.length && marker.startsWith(normalized),
+ normalized.length <= marker.length && marker.startsWith(normalized),
)
);
</file context>
| text !== part.text && | ||
| part.providerOptions | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
🟡 MEDIUM
Dropping signed reasoning parts can break provider replay
When a reasoning part's text changes and it carries provider-bound metadata (e.g. an Anthropic thinking signature), the part is dropped entirely. The sanitized array feeds both aiMessagesJson persistence and currentMessageHistory for the next pass in the same turn, so the assistant message can end up keeping its tool-call parts while losing the thinking block that preceded them. Providers that require thinking blocks to accompany tool_use reject that shape, which would fail the whole agent turn. The trigger is not exotic: the local-agent system prompt explicitly describes , so a model quoting the tag while reasoning about recent history is a realistic path into this branch.
💡 Suggestion: Prefer leaving reasoning parts with provider-bound signatures untouched (the streamed display text is already sanitized separately) rather than deleting them, or drop the part only when it is not accompanied by tool-call parts in the same message.
| const response = await streamResult.response; | ||
| steps = (await streamResult.steps) ?? []; | ||
| responseMessages = response.messages; | ||
| responseMessages = stripGitContextEchoesFromAssistantMessages( |
There was a problem hiding this comment.
🟡 MEDIUM
Sanitizing before mid-turn compaction slicing can misalign the offset
responseMessages is now the sanitized array, but the mid-turn compaction path slices it using prevStepMessages.length taken from the unsanitized steps[...].response.messages. stripGitContextEchoesFromAssistantMessages can remove whole messages (an assistant message whose only content was an echoed tag returns an empty array), so the two lengths can disagree and responseMessages.slice(prevStepMessages.length) then drops real post-compaction messages or keeps pre-compaction ones. That silently corrupts the persisted transcript and the history replayed for the rest of the turn.
💡 Suggestion: Compute the slice from the raw response.messages and sanitize afterwards (sanitize messagesToAccumulate), so the offset and the array being sliced come from the same source.
|
|
||
| message.content.forEach((part, index) => { | ||
| if (part.type === "text" || part.type === "reasoning") { | ||
| sanitizedTextByIndex.set(index, sanitizer.push(part.text)); |
There was a problem hiding this comment.
🟡 MEDIUM
Shared sanitizer state bleeds held text across assistant content parts
A single GitContextEchoSanitizer is pushed through every text and reasoning part of an assistant message, and the trailing finish() output is appended to the last sanitized part regardless of where it originated. Any part ending in a marker-like prefix (a trailing '<' or '<d') is held and then emitted into a later part, so for content like [reasoning '...<', tool-call, text 'Answer'] the held reasoning fragment is concatenated onto the visible text part - moving content across a tool-call boundary and leaking reasoning text into the answer. The streaming path correctly uses two separate sanitizers for text and reasoning; this path does not.
💡 Suggestion: Use a separate sanitizer per contiguous run of same-type parts (or at minimum one for text and one for reasoning), and flush each run's trailing buffer into the last part of that run rather than the last part of the message.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues The core idea is sound: sanitize only assistant-authored text, hold partial tags across chunk boundaries, and apply the same strip before Three MEDIUM issues are worth a look before merge; none of them block on their own. The diff was complete (not truncated), so confidence in the reading of the changed code is high; the provider-rejection risk in the first item depends on runtime provider behavior I could not verify from the diff alone. Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results✅ All tests passed!
Total: 294 tests passed (12 skipped) |
|
Superseded by #4375, which uses the narrower user-message reminder approach discussed in review. |
1 participant
Summary
Prevent model-echoed internal Git provenance markup from leaking into visible chat responses or being persisted for future replay.
aiMessagesJson, preventing hallucinated provenance from re-entering later model history.Note
Medium Risk
Touches Local Agent stream assembly and persisted transcript text. Incorrect stripping could hide real assistant output or leave provenance tags in history, but the change is narrowly scoped to one tag family with tests.
Overview
Stops models from leaking internal
<dyad-git-context>provenance into visible chat or persistedaiMessagesJson, so hallucinated Git hashes cannot re-enter later replay as model-authored text.Streaming
text-deltachunks now go through a stateful sanitizer that holds partial tags across chunk boundaries, then the same strip runs on assistant SDK messages before persistence. User literals and non-text tool parts are left alone; inner tag body text is kept.Reviewed by Cursor Bugbot for commit 32fc247. Bugbot is set up for automated code reviews on this repo. Configure here.