-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Strip internal Git context tags from agent responses #4343
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 all commits
1c8ea0a
8a4d107
fb9a903
2bae88a
5bb9dbc
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 |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { | ||
| GitContextEchoSanitizer, | ||
| stripGitContextEchoes, | ||
| stripGitContextEchoesFromAssistantMessages, | ||
| } from "./git_context_sanitizer"; | ||
|
|
||
| describe("GitContextEchoSanitizer", () => { | ||
| it("strips an echoed Git-context tag split across stream chunks", () => { | ||
| const sanitizer = new GitContextEchoSanitizer(); | ||
|
|
||
| const output = [ | ||
| sanitizer.push("Done.<dyad-git-con"), | ||
| sanitizer.push('text commit="fake">'), | ||
| sanitizer.push("</dyad-git-context>"), | ||
| sanitizer.finish(), | ||
| ].join(""); | ||
|
|
||
| expect(output).toBe("Done."); | ||
| }); | ||
|
|
||
| it("preserves ordinary text around internal tag markup", () => { | ||
| expect( | ||
| stripGitContextEchoes( | ||
| 'Before <dyad-git-context commit="fake">unexpected</dyad-git-context> after', | ||
| ), | ||
| ).toBe("Before unexpected after"); | ||
| }); | ||
|
|
||
| it("does not strip similarly named user text", () => { | ||
| expect(stripGitContextEchoes("Use <dyad-git-contextual> here.")).toBe( | ||
| "Use <dyad-git-contextual> here.", | ||
| ); | ||
| }); | ||
|
|
||
| it("preserves unterminated prose instead of buffering it without bound", () => { | ||
| const sanitizer = new GitContextEchoSanitizer(); | ||
| const prose = `<dyad-git-context ${"ordinary prose ".repeat(24)}`; | ||
|
|
||
| expect(sanitizer.push(prose)).toBe(prose); | ||
| expect(sanitizer.finish()).toBe(""); | ||
| }); | ||
|
|
||
| it("keeps marker indexes aligned after Unicode with expanding lowercase forms", () => { | ||
| expect( | ||
| stripGitContextEchoes( | ||
| 'İ<dyad-git-context commit="fake"></dyad-git-context>after', | ||
| ), | ||
| ).toBe("İafter"); | ||
| }); | ||
|
|
||
| it("drops a distinctive internal tag fragment if the stream ends", () => { | ||
| const sanitizer = new GitContextEchoSanitizer(); | ||
|
|
||
| expect(sanitizer.push("Done.<dyad-git-con")).toBe("Done."); | ||
| expect(sanitizer.finish()).toBe(""); | ||
|
|
||
| const shortPrefixSanitizer = new GitContextEchoSanitizer(); | ||
| expect(shortPrefixSanitizer.push("Done.<dyad-git")).toBe("Done."); | ||
| expect(shortPrefixSanitizer.finish()).toBe(""); | ||
|
|
||
| const exactMarkerSanitizer = new GitContextEchoSanitizer(); | ||
| expect(exactMarkerSanitizer.push("Done.<dyad-git-context")).toBe("Done."); | ||
| expect(exactMarkerSanitizer.finish()).toBe(""); | ||
| }); | ||
|
|
||
| it("removes echoes from assistant message text only", () => { | ||
| const tag = '<dyad-git-context commit="fake"></dyad-git-context>'; | ||
| const messages = stripGitContextEchoesFromAssistantMessages([ | ||
| { role: "user", content: `User literal: ${tag}` }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: `Finished.${tag}` }, | ||
| { | ||
| type: "tool-call", | ||
| toolCallId: "call-1", | ||
| toolName: "read_file", | ||
| input: {}, | ||
| }, | ||
| ], | ||
| }, | ||
| ]); | ||
|
|
||
| expect(messages[0]).toEqual({ | ||
| role: "user", | ||
| content: `User literal: ${tag}`, | ||
| }); | ||
| expect(messages[1]).toMatchObject({ | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "Finished." }, | ||
| { type: "tool-call", toolCallId: "call-1" }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("sanitizes tags split across assistant text parts", () => { | ||
| const messages = stripGitContextEchoesFromAssistantMessages([ | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "Before <dyad-git-con" }, | ||
| { type: "text", text: 'text commit="fake">inside</dyad-git-' }, | ||
| { type: "text", text: "context> after" }, | ||
| ], | ||
| }, | ||
| ]); | ||
|
|
||
| expect(messages).toEqual([ | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "Before " }, | ||
| { type: "text", text: "inside" }, | ||
| { type: "text", text: " after" }, | ||
| ], | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("sanitizes reasoning parts and drops empty assistant content", () => { | ||
| const tag = '<dyad-git-context commit="fake"></dyad-git-context>'; | ||
| const messages = stripGitContextEchoesFromAssistantMessages([ | ||
| { role: "assistant", content: tag }, | ||
| { | ||
| role: "assistant", | ||
| content: [{ type: "text", text: tag }], | ||
| }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "reasoning", text: `Consider.${tag}` }, | ||
| { | ||
| type: "tool-call", | ||
| toolCallId: "call-1", | ||
| toolName: "read_file", | ||
| input: {}, | ||
| }, | ||
| { type: "text", text: tag }, | ||
| ], | ||
| }, | ||
| ]); | ||
|
|
||
| expect(messages).toEqual([ | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "reasoning", text: "Consider." }, | ||
| { | ||
| type: "tool-call", | ||
| toolCallId: "call-1", | ||
| toolName: "read_file", | ||
| input: {}, | ||
| }, | ||
| ], | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("drops modified reasoning parts with provider-bound metadata", () => { | ||
| const tag = '<dyad-git-context commit="fake"></dyad-git-context>'; | ||
|
|
||
| expect( | ||
| stripGitContextEchoesFromAssistantMessages([ | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { | ||
| type: "reasoning", | ||
| text: `Signed thought.${tag}`, | ||
| providerOptions: { | ||
| anthropic: { signature: "signed-original-reasoning" }, | ||
| }, | ||
| }, | ||
| { type: "text", text: "Answer" }, | ||
| ], | ||
| }, | ||
| ]), | ||
| ).toEqual([ | ||
| { | ||
| role: "assistant", | ||
| content: [{ type: "text", text: "Answer" }], | ||
| }, | ||
| ]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| import type { ModelMessage } from "ai"; | ||
|
|
||
| const GIT_CONTEXT_TAG_MARKERS = [ | ||
| "<dyad-git-context", | ||
| "</dyad-git-context", | ||
| ] as const; | ||
| const MAX_TAG_MARKUP_LENGTH = 256; | ||
|
|
||
| /** | ||
| * Removes internal Git-context tag markup without exposing partial tags while | ||
| * model text is streaming. Text between an opening and closing tag is | ||
| * preserved; only the internal markup itself is metadata. | ||
| */ | ||
| export class GitContextEchoSanitizer { | ||
| private pending = ""; | ||
|
|
||
| push(chunk: string): string { | ||
| this.pending += chunk; | ||
| let output = ""; | ||
|
|
||
| while (this.pending.length > 0) { | ||
| const markerIndex = findNextMarkerIndex(this.pending); | ||
| if (markerIndex === -1) { | ||
| const heldSuffixLength = getPotentialMarkerSuffixLength(this.pending); | ||
| const emitLength = this.pending.length - heldSuffixLength; | ||
| output += this.pending.slice(0, emitLength); | ||
| this.pending = this.pending.slice(emitLength); | ||
| break; | ||
| } | ||
|
|
||
| output += this.pending.slice(0, markerIndex); | ||
| this.pending = this.pending.slice(markerIndex); | ||
|
|
||
| const tagEndIndex = this.pending.indexOf(">"); | ||
| if (tagEndIndex === -1) { | ||
| if (this.pending.length > MAX_TAG_MARKUP_LENGTH) { | ||
| output += this.pending; | ||
| this.pending = ""; | ||
| } | ||
| break; | ||
| } | ||
| this.pending = this.pending.slice(tagEndIndex + 1); | ||
| } | ||
|
|
||
| return output; | ||
| } | ||
|
|
||
| finish(): string { | ||
| const pending = this.pending; | ||
| this.pending = ""; | ||
| return startsWithDistinctivePartialMarker(pending) ? "" : pending; | ||
| } | ||
| } | ||
|
|
||
| export function stripGitContextEchoes(text: string): string { | ||
| const sanitizer = new GitContextEchoSanitizer(); | ||
| return sanitizer.push(text) + sanitizer.finish(); | ||
| } | ||
|
|
||
| export function stripGitContextEchoesFromAssistantMessages( | ||
| messages: ModelMessage[], | ||
| ): ModelMessage[] { | ||
| return messages.flatMap((message) => { | ||
| if (message.role !== "assistant") { | ||
| return [message]; | ||
| } | ||
|
|
||
| if (typeof message.content === "string") { | ||
| const content = stripGitContextEchoes(message.content); | ||
| return content.length > 0 ? [{ ...message, content }] : []; | ||
| } | ||
|
|
||
| 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)); | ||
|
Comment on lines
+73
to
+79
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.
When an assistant reasoning or text part ends with any prefix of the marker—even a lone Useful? React with 👍 / 👎.
Contributor
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. 🟡 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. |
||
| lastSanitizedPartIndex = index; | ||
| } | ||
| }); | ||
|
|
||
| if (lastSanitizedPartIndex !== -1) { | ||
| const trailingText = sanitizer.finish(); | ||
| sanitizedTextByIndex.set( | ||
| lastSanitizedPartIndex, | ||
| (sanitizedTextByIndex.get(lastSanitizedPartIndex) ?? "") + trailingText, | ||
| ); | ||
| } | ||
|
|
||
| const content: typeof message.content = []; | ||
| message.content.forEach((part, index) => { | ||
| if (part.type !== "text" && part.type !== "reasoning") { | ||
| content.push(part); | ||
| return; | ||
| } | ||
| const text = sanitizedTextByIndex.get(index) ?? ""; | ||
| if (text.length > 0) { | ||
| if ( | ||
| part.type === "reasoning" && | ||
| text !== part.text && | ||
| part.providerOptions | ||
| ) { | ||
| return; | ||
|
Contributor
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. P1: When a provider-bound reasoning part merely ends with a buffered partial marker, this Prompt for AI agents
Contributor
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. 🟡 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. |
||
| } | ||
| content.push({ ...part, text }); | ||
|
keppo-bot[bot] marked this conversation as resolved.
|
||
| } | ||
| }); | ||
|
|
||
| return content.length > 0 ? [{ ...message, content } as ModelMessage] : []; | ||
| }); | ||
| } | ||
|
|
||
| function findNextMarkerIndex(text: string): number { | ||
| const normalized = foldAsciiCase(text); | ||
| let earliest = -1; | ||
| for (const marker of GIT_CONTEXT_TAG_MARKERS) { | ||
| let searchFrom = 0; | ||
| while (searchFrom < normalized.length) { | ||
| const index = normalized.indexOf(marker, searchFrom); | ||
| if (index === -1) { | ||
| break; | ||
| } | ||
| if (isTagBoundary(normalized[index + marker.length])) { | ||
| if (earliest === -1 || index < earliest) { | ||
| earliest = index; | ||
| } | ||
| break; | ||
| } | ||
| searchFrom = index + 1; | ||
| } | ||
| } | ||
| return earliest; | ||
| } | ||
|
|
||
| function getPotentialMarkerSuffixLength(text: string): number { | ||
| const normalized = foldAsciiCase(text); | ||
| const maxLength = Math.min( | ||
| normalized.length, | ||
| Math.max(...GIT_CONTEXT_TAG_MARKERS.map((marker) => marker.length - 1)), | ||
| ); | ||
|
|
||
| for (let length = maxLength; length > 0; length -= 1) { | ||
| const suffix = normalized.slice(-length); | ||
| if (GIT_CONTEXT_TAG_MARKERS.some((marker) => marker.startsWith(suffix))) { | ||
| return length; | ||
| } | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| function startsWithDistinctivePartialMarker(text: string): boolean { | ||
| const normalized = foldAsciiCase(text); | ||
| const minimumDistinctivePrefix = "<dyad-git"; | ||
| return ( | ||
| normalized.length >= minimumDistinctivePrefix.length && | ||
| GIT_CONTEXT_TAG_MARKERS.some( | ||
| (marker) => | ||
| normalized.length <= marker.length && marker.startsWith(normalized), | ||
|
Contributor
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. P3: Changing the bound to Prompt for AI agents |
||
| ) | ||
| ); | ||
| } | ||
|
|
||
| function foldAsciiCase(text: string): string { | ||
| return text.replace(/[A-Z]/g, (character) => character.toLowerCase()); | ||
| } | ||
|
|
||
| function isTagBoundary(character: string | undefined): boolean { | ||
| return ( | ||
| character === undefined || | ||
| character === ">" || | ||
| character === "/" || | ||
| /\s/.test(character) | ||
|
keppo-bot[bot] marked this conversation as resolved.
|
||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.