Skip to content

fix(agy): stop delivering the same answer twice when deltas are mangled - #1629

Open
Overbaker wants to merge 1 commit into
tiann:mainfrom
Overbaker:fix/agy-duplicate-planner-response
Open

fix(agy): stop delivering the same answer twice when deltas are mangled#1629
Overbaker wants to merge 1 commit into
tiann:mainfrom
Overbaker:fix/agy-duplicate-planner-response

Conversation

@Overbaker

Copy link
Copy Markdown

Fixes #1628.

Problem

On agy sessions, a long non-ASCII answer is delivered to the chat twice:
once with scattered ���, then once clean. Both copies are real rows in the
hub DB (~2 ms apart), so refreshing does not help. Every long CJK answer in the
affected session was doubled; ASCII answers never were.

agy's text_delta stream decodes the model output chunk by chunk without
carrying a partial UTF-8 sequence across the boundary, so a multi-byte character
split by a chunk boundary arrives as a run of U+FFFD — a 3-byte CJK character
becomes exactly three replacement chars (one orphan byte left of the split, two
right of it). The result envelope carries the same answer decoded from the
complete buffer, so it is clean. hapi's own stdout reader is not at fault;
setEncoding('utf8') handles split sequences correctly.

The driver decided whether the envelope duplicated the streamed answer with

if (lastPlannerContent?.trim() !== response) { /* emit again */ }

For any answer that hit a split the two renderings differ, the guard misfires,
and the whole answer is emitted a second time. ASCII answers are always
byte-identical, which is why this was not caught.

Changes

  • agyHeadlessDriver.ts: hold the last completed planner step back
    (stagePlanner / releaseStagedPlanner / takeStagedPlanner) instead of
    emitting it immediately. The result envelope replaces it in place, so the
    authoritative rendering supersedes the delta-assembled one rather than landing
    next to it. Anything that proves the held entry was not the final answer — a
    tool step, a later planner step, turn close — releases it unchanged, so wire
    order and the existing crash/truncation paths are untouched.
  • agyMessageText.ts: add isSameAgyResponse(), used on the remaining path
    where no planner step is left to carry the response (completed pre-tool
    narration echoed by the envelope). It treats each run of replacement
    characters as a short unknown span, with the segment count and span width
    bounded so the generated pattern cannot blow up.

No protocol, hub, or web change; the emitted agy_message payload is unchanged.

Tests

  • agyHeadlessDriver.test.ts: two regression tests reproducing the real stream
    (mangled deltas + clean envelope), one for each path. On main the first
    fails with two planner entries — the mangled rendering followed by the clean
    one — and the second with expected length 1, got 2.
  • agyMessageText.test.ts: 6 unit tests for the comparison, including negative
    cases (a genuinely different answer, a clean-vs-clean difference, regex
    metacharacters treated literally).

cd cli && bunx vitest run src/agy → 87 passed.
cd cli && bun run typecheck → clean.
Full cli suite: 2380 passed; the 11 failures are pre-existing ripgrep /
difftastic ENOENT failures in my environment (vendored binaries not
downloaded), untouched by this change.

AI disclosure

Per CONTRIBUTING.md: investigated and authored with Claude Opus 5 (Claude Code).
Root cause was established by decompressing and diffing the message rows of a
real affected session in the local hub SQLite DB, not inferred.

agy's `text_delta` stream decodes the model output chunk by chunk without
carrying a partial UTF-8 sequence across the boundary, so a multi-byte
character split by a chunk boundary reaches the driver as a run of U+FFFD
(a 3-byte CJK character becomes three replacement chars: one orphan byte
left of the split, two right of it). The `result` envelope carries the
same answer decoded from the complete buffer, so it is clean. hapi's own
stdout reader is not at fault -- setEncoding('utf8') carries partial
sequences correctly on both Node and Bun.

The driver decided whether the envelope duplicated the streamed answer
with `lastPlannerContent?.trim() !== response`. For any non-ASCII answer
that hit a split the two renderings differ, the guard misfires, and the
whole answer is delivered a second time: the chat (and the hub DB) end up
holding two agent messages, the corrupted one followed by the clean one.
ASCII answers are always byte-identical, which is why this went unnoticed.

Two changes:

- Hold the last completed planner step back instead of emitting it right
  away. The result envelope that follows replaces it in place, so the
  authoritative rendering supersedes the delta-assembled one instead of
  landing next to it. Anything that proves the held entry was not the
  final answer (a tool step, a later planner step, turn close) releases
  it unchanged, so wire order and the crash/truncation paths are
  untouched.
- When no planner step is left to carry the response, compare with
  isSameAgyResponse(), which treats each run of replacement characters as
  a short unknown span.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] The replacement-run matcher can suppress a distinct authoritative result — each corruption run currently matches any 1–8 UTF-16 code units. For example, "Outcome: ���." and "Outcome: blocked." are classified as the same response. In the post-tool path, that makes alreadySent true and drops the authoritative final answer entirely. Evidence: cli/src/agy/utils/agyMessageText.ts:41, consumed at cli/src/agy/headless/agyHeadlessDriver.ts:690.

    Suggested fix:

    const runs = streamed.match(/+/g) ?? []
    const pattern = segments.map((segment, index) => {
        if (index === 0) return escapeRegExp(segment)
        const corruptedBytes = runs[index - 1]!.length
        const minUnits = Math.max(1, Math.ceil(corruptedBytes / 4))
        const maxUnits = Math.max(minUnits, Math.floor(corruptedBytes / 2))
        return `[\\s\\S]{${minUnits},${maxUnits}}${escapeRegExp(segment)}`
    }).join('')
  • [Minor] Completed narration remains staged through the whole tool execution — the staged planner is released only inside event.isDone. An ACTIVE tool event already proves the text is pre-tool narration, but a long-running or hung command now hides that narration until DONE/close. Evidence: cli/src/agy/headless/agyHeadlessDriver.ts:610.

    Suggested fix:

    releaseStagedPlanner()
    if (event.isDone) {
        sendTool(event.entry, event.toolCall)
    }

Summary

Review mode: initial. Two issues found: one possible final-answer loss from over-broad fuzzy matching, and one live-delivery regression for pre-tool narration.

Testing

Not run (automation); review performed statically because PR code must not be executed. GitHub integration check passed; the main test check was pending at review time. Add a near-match negative case for isSameAgyResponse, plus a scriptable-child test asserting narration is emitted after tool ACTIVE and before tool DONE.

HAPI Bot

// A stream this mangled is not recognisable anyway, and the regex below
// would get expensive: bail out instead.
if (segments.length - 1 > MAX_REPLACEMENT_RUNS) return false
const pattern = segments.map(escapeRegExp).join(`[\\s\\S]{1,${MAX_CHARS_PER_RUN}}`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Distinct final results can be classified as duplicates

This wildcard is broader than the corruption model: one replacement run matches any 1–8 UTF-16 code units. Thus isSameAgyResponse("Outcome: ���.", "Outcome: blocked.") returns true. At agyHeadlessDriver.ts:690, a post-tool result then looks already sent and the authoritative final answer is omitted.

Suggested fix:

const runs = streamed.match(/+/g) ?? []
const pattern = segments.map((segment, index) => {
    if (index === 0) return escapeRegExp(segment)
    const corruptedBytes = runs[index - 1]!.length
    const minUnits = Math.max(1, Math.ceil(corruptedBytes / 4))
    const maxUnits = Math.max(minUnits, Math.floor(corruptedBytes / 2))
    return `[\\s\\S]{${minUnits},${maxUnits}}${escapeRegExp(segment)}`
}).join('')

Please also add a near-match negative test such as "Outcome: ���." versus "Outcome: blocked.".

// Prose that precedes a tool call is narration,
// not the final answer: release it so it keeps
// its place ahead of the tool card.
releaseStagedPlanner();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Release narration when the tool starts

This only runs for the DONE tool event. The ACTIVE event already proves the staged planner is pre-tool narration, so a long-running or hung tool now keeps that narration invisible until the tool finishes or the process closes.

Suggested fix:

releaseStagedPlanner()
if (event.isDone) {
    sendTool(event.entry, event.toolCall)
}

A scriptable-child test should feed planner DONE and tool ACTIVE, then assert the planner message was sent before feeding tool DONE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

agy: long non-ASCII answers are delivered twice (once mangled, once clean)

1 participant