From 0bc9bc25b47d201cc1963d3d96df55d5b93e20b2 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 2 Jun 2026 08:40:49 +0200 Subject: [PATCH] fix(agent): gated tool_arg harmony detection on trailing-garbage T co-signal - Added T co-signal requirement for tool_arg surface so legitimate edits carrying the marker are never hard-aborted. - Extended detectHarmonyLeakInAssistantMessage with optional toolArgParseEnd resolver; agent loop omits it, keeping tool_arg inert. - Updated tests to inject a boundary-at-0 helper for corpus cases and added T-gate unit tests. --- packages/agent/CHANGELOG.md | 4 ++ packages/agent/src/harmony-leak.ts | 35 +++++++++++-- packages/agent/test/agent-loop.test.ts | 39 +++++++++++++++ packages/agent/test/harmony-leak.test.ts | 63 ++++++++++++++++++++---- 4 files changed, 128 insertions(+), 13 deletions(-) diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 1731dd054..ad3524d04 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Engaged GPT-5 Harmony leak detection on the committed assistant message (openai-codex only). `detectHarmonyLeakInAssistantMessage` now runs on the streamed `done`/`error` result and the trailing fallback, so a leaked final response is aborted-and-retried by the existing mitigation instead of being committed as-is. Tool-argument (`tool_arg`) scanning is gated on the trailing-garbage `T` co-signal and only fires when a caller supplies a parse boundary via `detectHarmonyLeakInAssistantMessage`'s new optional `toolArgParseEnd` resolver. The agent loop passes none — it cannot bound a streamed tool DSL — so that surface stays inert and a legitimate codex tool call whose content legitimately carries `to=functions.*` next to a channel word or non-Latin script (e.g. editing the harmony fixtures) is never hard-aborted. + ## [15.7.4] - 2026-05-31 ### Removed diff --git a/packages/agent/src/harmony-leak.ts b/packages/agent/src/harmony-leak.ts index 6d59e5f26..3ea34b2e4 100644 --- a/packages/agent/src/harmony-leak.ts +++ b/packages/agent/src/harmony-leak.ts @@ -129,8 +129,18 @@ export function signalListLabel(signals: readonly HarmonySignal[]): string { * (`C`/`G`/`S`/`B`/`R`/`T`). Bare `M` does not trip — this document, its * tests, and bug reports legitimately carry the marker. * + * The `tool_arg` surface is held to a stricter rule. A tool argument is + * arbitrary file/data content that can legitimately carry the marker, a + * channel word, harmony control tokens, or a non-Latin script run (editing + * these very fixtures does exactly that). The only robust leak signal there + * is content trailing the structurally-valid parse, so a `tool_arg` detection + * additionally requires the `T` co-signal. Absent a `parsedEnd` boundary `T` + * is never set, so `tool_arg` scanning stays inert and a legitimate codex tool + * call is never hard-aborted. `assistant_text`/`assistant_thinking` keep the + * base rule. + * * `parsedEnd`, when supplied, marks the byte at which a structurally valid - * tool-argument parse ends; markers strictly after it set the `T` co-signal. + * tool-argument parse ends; markers at or past it set the `T` co-signal. * `contentIndex`/`toolName`/`toolCallId` flow through to the returned * detection for downstream auditing. */ @@ -177,6 +187,12 @@ export function detectHarmonyLeak( } if (signals.length === 0) return undefined; + // Tool arguments are data: they can legitimately embed the marker, a channel + // word, harmony control tokens, or a non-Latin script run. Only a marker + // trailing the structurally-valid parse (`T`) is a reliable leak signal, so + // refuse to trip a `tool_arg` detection without it. Without a `parsedEnd` + // boundary `T` is never set and the surface stays inert. + if (surface === "tool_arg" && !signals.some(s => s.classes.includes("T"))) return undefined; signals.sort((a, b) => a.start - b.start || a.end - b.end); return { surface, @@ -187,8 +203,20 @@ export function detectHarmonyLeak( }; } -/** Scan an assistant message's content blocks; return the first detection. */ -export function detectHarmonyLeakInAssistantMessage(message: AssistantMessage): HarmonyDetection | undefined { +/** + * Scan an assistant message's content blocks; return the first detection. + * + * `toolArgParseEnd`, when supplied, resolves the byte offset at which a tool + * call's structurally-valid argument parse ends (the `T` co-signal in + * {@link detectHarmonyLeak}). Callers that can parse a tool's argument DSL pass + * it to enable `tool_arg` leak detection; omitting it keeps that surface inert + * — the safe default the agent loop relies on, since it cannot bound a streamed + * tool DSL and must never hard-abort a legitimate tool call. + */ +export function detectHarmonyLeakInAssistantMessage( + message: AssistantMessage, + toolArgParseEnd?: (toolCall: ToolCall) => number | undefined, +): HarmonyDetection | undefined { for (let i = 0; i < message.content.length; i++) { const block = message.content[i]; if (block.type === "text") { @@ -204,6 +232,7 @@ export function detectHarmonyLeakInAssistantMessage(message: AssistantMessage): contentIndex: i, toolName: block.name, toolCallId: block.id, + parsedEnd: toolArgParseEnd?.(block), }); if (d) return d; } diff --git a/packages/agent/test/agent-loop.test.ts b/packages/agent/test/agent-loop.test.ts index 0ac9f624c..ff085f383 100644 --- a/packages/agent/test/agent-loop.test.ts +++ b/packages/agent/test/agent-loop.test.ts @@ -87,6 +87,45 @@ describe("agentLoop with AgentMessage", () => { expect(JSON.stringify(messages)).not.toContain("to=functions."); }); + it("does not hard-abort a codex tool call whose argument legitimately carries the marker", async () => { + // A legit edit of a file (e.g. these harmony fixtures) whose content carries + // `to=functions.*` next to a channel word + non-Latin script. tool_arg is + // gated on the trailing-garbage `T` co-signal, and the loop supplies no parse + // boundary, so the call commits + executes once instead of being detected as + // a leak and retried/escalated. + const toolSchema = z.object({ input: z.string() }); + const executed: string[] = []; + const tool: AgentTool = { + name: "edit", + label: "Edit", + description: "Edit tool", + parameters: toolSchema, + async execute(_toolCallId, params) { + executed.push(params.input); + return { content: [{ type: "text", text: "ok" }], details: { input: params.input } }; + }, + }; + const context: AgentContext = { systemPrompt: [""], messages: [], tools: [tool] }; + const leakyArg = "@fixtures/corpus.json\n+\tanalysis to=functions.edit code 大发官网\n"; + const mock = createMockModel({ + provider: "openai-codex", + responses: [ + { content: [{ type: "toolCall", id: "tool-1", name: "edit", arguments: { input: leakyArg } }] }, + { content: ["done"] }, + ], + }); + const config: AgentLoopConfig = { model: mock.model, convertToLlm: identityConverter }; + const stream = agentLoop([createUserMessage("edit a fixture")], context, config, undefined, mock.stream); + for await (const _event of stream) { + // drain + } + // The tool ran on the original (unmodified) argument and the turn was not + // retried — a hard-abort would have left `executed` empty and consumed the + // "done" response as a clean retry instead. + expect(executed).toEqual([leakyArg]); + expect(mock.calls).toHaveLength(2); + }); + it("emits an aborted assistant message when cancellation happens before provider events", async () => { const context: AgentContext = { systemPrompt: ["You are helpful."], diff --git a/packages/agent/test/harmony-leak.test.ts b/packages/agent/test/harmony-leak.test.ts index 201782e54..bd3d785c6 100644 --- a/packages/agent/test/harmony-leak.test.ts +++ b/packages/agent/test/harmony-leak.test.ts @@ -42,6 +42,13 @@ function makeToolCallMessage(toolName: string, input: string | null, argJson: st return createAssistantMessage([toolCall], "toolUse"); } +// Corpus tool-arg entries are wholly-contaminated args captured from real +// production leaks. detectHarmonyLeakInAssistantMessage keeps the tool_arg +// surface inert without a parse boundary (the production default), so these +// tests inject a boundary at 0 — the entire payload is treated as trailing +// garbage past the valid parse — to exercise tool-arg detection and recovery. +const wholePayloadTrailing = () => 0; + describe("isHarmonyLeakMitigationTarget", () => { it("targets every openai-codex model (don't enumerate ids)", () => { expect(isHarmonyLeakMitigationTarget(codexModel)).toBe(true); @@ -55,7 +62,10 @@ describe("isHarmonyLeakMitigationTarget", () => { describe("detectHarmonyLeak — negative cases (must NOT trip)", () => { for (const neg of negatives) { it(neg.name, () => { - const detection = detectHarmonyLeak(neg.input, "tool_arg"); + // Base content guards (co-signal requirement, fence exemption) are + // surface-independent; the extra `tool_arg` `T`-gate would mask them, so + // assert them on a surface without it — a pass proves the heuristics. + const detection = detectHarmonyLeak(neg.input, "assistant_text"); expect(detection).toBeUndefined(); }); } @@ -79,8 +89,12 @@ describe("detectHarmonyLeak — positive corpus cases (must trip with co-signal) const surfaceText = pos.input ?? pos.argJson; if (surfaceText === null) continue; it(`${pos.id} (${pos.kind}) trips with co-signals`, () => { + // Production feeds no boundary, so tool_arg stays inert. These corpus + // payloads are wholly-contaminated args from real leaks, so a boundary at + // 0 treats the whole payload as trailing and exercises the heuristics. const detection = detectHarmonyLeak(surfaceText, "tool_arg", { toolName: pos.kind === "eval" ? "eval" : "edit", + parsedEnd: 0, }); expect(detection).toBeDefined(); // Every signal that did fire must include `M` plus at least one co-signal. @@ -92,12 +106,41 @@ describe("detectHarmonyLeak — positive corpus cases (must trip with co-signal) } }); +describe("detectHarmonyLeak — tool_arg T-gate", () => { + // The strongest co-signals (channel word + marker + CJK) that a legitimate + // edit of these very fixtures would carry inside its `input` argument. + const embedded = "header line one\nanalysis to=functions.edit code 大发官网"; + + it("does not trip on tool_arg without a parse boundary (no false hard-abort)", () => { + expect(detectHarmonyLeak(embedded, "tool_arg")).toBeUndefined(); + }); + + it("trips the same content on a non-tool surface (gate is tool_arg-specific)", () => { + const detection = detectHarmonyLeak(embedded, "assistant_text"); + expect(detection).toBeDefined(); + expect(detection!.signals.some(s => s.classes.includes("M"))).toBe(true); + }); + + it("does not trip when the marker precedes the parse boundary (embedded content)", () => { + // Whole payload is structurally valid → marker is before parsedEnd → no `T`. + expect(detectHarmonyLeak(embedded, "tool_arg", { parsedEnd: embedded.length })).toBeUndefined(); + }); + + it("trips when the marker trails the parse boundary (real leak signature)", () => { + const boundary = embedded.indexOf("analysis"); + const detection = detectHarmonyLeak(embedded, "tool_arg", { parsedEnd: boundary }); + expect(detection).toBeDefined(); + // Every fired signal carries the `T` co-signal the gate requires. + expect(detection!.signals.every(s => s.classes.includes("T"))).toBe(true); + }); +}); + describe("recoverHarmonyToolCall — edit DSL", () => { const editDsl = positives.filter(p => p.kind === "edit_dsl"); for (const fix of editDsl) { it(`${fix.id}: produces an args-truncated message ending with the *** Abort sentinel`, () => { const message = makeToolCallMessage("edit", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message); + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing); expect(detection).toBeDefined(); const recovered = recoverHarmonyToolCall(message, detection!); expect(recovered).toBeDefined(); @@ -125,9 +168,9 @@ describe("recoverHarmonyToolCall — edit DSL", () => { it("idempotence: re-running detect+recover on the cleaned message is a no-op", () => { const fix = editDsl[0]; const message = makeToolCallMessage("edit", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message)!; + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing)!; const recovered = recoverHarmonyToolCall(message, detection)!; - const second = detectHarmonyLeakInAssistantMessage(recovered.message); + const second = detectHarmonyLeakInAssistantMessage(recovered.message, wholePayloadTrailing); expect(second).toBeUndefined(); }); @@ -137,7 +180,7 @@ describe("recoverHarmonyToolCall — edit DSL", () => { const applyPatchInput = "*** Begin Patch\n*** Update File: a.ts\n@@\n-old\n+new\n*** End Patch\n analysis to=functions.edit code 大发官网"; const message = makeToolCallMessage("edit", applyPatchInput, null); - const detection = detectHarmonyLeakInAssistantMessage(message)!; + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing)!; expect(detection).toBeDefined(); const recovered = recoverHarmonyToolCall(message, detection); expect(recovered).toBeUndefined(); @@ -148,7 +191,7 @@ describe("recoverHarmonyToolCall — edit JSON-schema (must NOT recover)", () => for (const fix of positives.filter(p => p.kind === "edit_json")) { it(`${fix.id}: detects but refuses to recover`, () => { const message = makeToolCallMessage("edit", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message); + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing); expect(detection).toBeDefined(); const recovered = recoverHarmonyToolCall(message, detection!); // argJson cases either lack a string `input` field, or their `input` @@ -162,7 +205,7 @@ describe("recoverHarmonyToolCall — eval", () => { for (const fix of positives.filter(p => p.kind === "eval")) { it(`${fix.id}: cleaned input ends with *** Abort sentinel`, () => { const message = makeToolCallMessage("eval", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message); + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing); expect(detection).toBeDefined(); const recovered = recoverHarmonyToolCall(message, detection!); expect(recovered).toBeDefined(); @@ -182,7 +225,7 @@ describe("recoverHarmonyToolCall — unsupported tools", () => { '{"path":"src/foo.ts","sel":"raw"}' /* legitimate-looking */ + " \tchangedFiles to=functions.read code 天天中彩票"; const message = makeToolCallMessage("read", text, null); - const detection = detectHarmonyLeakInAssistantMessage(message); + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing); // Detector trips because of `G` (changedFiles) + `M`. expect(detection).toBeDefined(); // But `read` is not in RECOVERY_REGISTRY, so no recovery offered. @@ -195,7 +238,7 @@ describe("extractHarmonyRemoved", () => { it("returns the contaminated tail of a tool argument", () => { const fix = positives.filter(p => p.kind === "edit_json")[0]; const message = makeToolCallMessage("edit", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message)!; + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing)!; const removed = extractHarmonyRemoved(message, detection); expect(removed.length).toBeGreaterThan(0); expect(removed.startsWith("to=functions.")).toBe(true); @@ -215,7 +258,7 @@ describe("createHarmonyAuditEvent", () => { it("captures sha + redacted preview by default; raw blob hidden", () => { const fix = positives.filter(p => p.kind === "edit_dsl")[0]; const message = makeToolCallMessage("edit", fix.input, fix.argJson); - const detection = detectHarmonyLeakInAssistantMessage(message)!; + const detection = detectHarmonyLeakInAssistantMessage(message, wholePayloadTrailing)!; const recovered = recoverHarmonyToolCall(message, detection)!; const event = createHarmonyAuditEvent({ action: "truncate_resume",