Skip to content

fix(agents): normalize non-array tool-result content at transcript ingest#98891

Merged
obviyus merged 1 commit into
mainfrom
fix/tool-result-string-replay
Jul 2, 2026
Merged

fix(agents): normalize non-array tool-result content at transcript ingest#98891
obviyus merged 1 commit into
mainfrom
fix/tool-result-string-replay

Conversation

@obviyus

@obviyus obviyus commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #98825 — after #97742, a tool result whose persisted content is a plain string (JSONL transcript passthrough) replays as empty tool output. extractToolResultText() iterates its input; a string iterates character-by-character, every 1-char primitive fails the object-block check, and the extracted replay text is "". The tool run succeeds, but the model's next turn sees nothing (src/llm/providers/tool-result-text.ts, consumed by all provider converters and transport streams).

The same type lie also crashes the paths that call msg.content.some(...) / .filter(...) (openai-completions, openai-responses-shared, mistral, openai-transport-stream, vision-model Google paths).

Why This Change Was Made

ToolResultMessage.content is typed (TextContent | ImageContent)[] (packages/llm-core/src/types.ts), but session JSONL load passed whatever the file contained straight through. The repo already normalizes the identical case for assistant content (src/llm/providers/transform-messages.ts — "JSONL passthrough") — tool results never got the same treatment, and transformMessages wouldn't be enough anyway since transport streams don't call it.

This PR fixes the lie once at the ingest boundary instead of teaching every consumer to tolerate it: when a session entry is parsed from JSONL, toolResult string content becomes [{ type: "text", text }] and a single non-array object becomes [object]. Every downstream consumer — provider converters, transport streams, the .some()/.filter() sites, and the shared helpers — keeps receiving the canonical typed array with no signature changes.

Alternative approach considered: #98826 / #98838 / #98863 widen the shared helpers to accept unknown and normalize inside them. That keeps ToolResultMessage.content dishonest, spreads tolerance across consumers (plus a new extractToolResultImageBlocks helper for the .some() sites), and needs as never casts in tests to exercise states the type system says can't exist. Normalizing at ingest deletes the problem instead; helper signatures stay readonly unknown[].

Also dedups parseSessionEntries into parseJsonlEntries (their bodies were identical), so all session-file readers (SessionManager.open, transcript file state, btw transcripts, CLI session history) share the one normalizing parser. Net prod diff is ±0 LOC.

User Impact

  • Replayed sessions with string tool-result content show the actual tool output to the model again instead of silently empty text / (no output) — the "tool ran fine but the model went blank" bug from bug(llm): tool-result replay can drop non-array output content #98825.
  • Single-object tool-result content replays as redacted structured JSON text (same expected behavior list as the issue).
  • No provider/transport behavior change for well-formed sessions; no config or API surface changes.

Thanks @snowzlmbot for the report and analysis in #98825.

Evidence

  • Boundary regression test: JSONL toolResult with string content loads as one text block; with { output: ... } object content loads as a one-element array (src/agents/sessions/session-manager.tool-result-replay.test.ts).
  • Symptom regression test (fails without the fix): a session file with string tool-result content replayed through streamAnthropic produces the string as tool_result content — red-checked on the pre-fix tree, where the Anthropic payload carried an empty string.
  • node scripts/run-vitest.mjs src/agents/sessions/session-manager.tool-result-replay.test.ts src/agents/sessions/session-manager.test.ts src/agents/embedded-agent-runner/transcript-file-state.test.ts — 76 tests passing.
  • Caller-side proof for the parseSessionEntries dedup: node scripts/run-vitest.mjs src/agents/compaction.test.ts src/agents/cli-runner/session-history.test.ts — 38 tests passing.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Jul 2, 2026
@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 2, 2026, 12:14 AM ET / 04:14 UTC.

Summary
The PR normalizes string or single-record toolResult.content loaded from JSONL session transcripts into the canonical block-array shape and adds focused replay regression tests.

PR surface: Source +6, Tests +197. Total +203 across 2 files.

Reproducibility: yes. Source inspection gives a high-confidence current-main reproduction path: JSONL session load preserves malformed toolResult.content, while provider replay expects a block array and can produce empty output or hit .some() assumptions.

Review metrics: 1 noteworthy metric.

  • Persisted Session Ingest Paths: 2 normalized. The PR changes both full JSONL parsing and verified append-cache advancement, which are the two session-ingest paths that can feed replay from persisted bytes.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/session-manager.tool-result-replay.test.ts, serialized state: src/agents/sessions/session-manager.ts, unknown-data-model-change: src/agents/sessions/session-manager.tool-result-replay.test.ts, unknown-data-model-change: src/agents/sessions/session-manager.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98825
Summary: This PR is a candidate fix for the canonical non-array tool-result replay bug, using session-ingest normalization rather than broad provider-helper tolerance.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Let the pending exact-head CI lanes finish before merge.

Risk before merge

  • [P1] The diff changes persisted JSONL session ingest and the verified append-cache path, so maintainers should treat the upgrade behavior as session-state sensitive even though it does not rewrite files on disk.
  • [P1] Several exact-head CI lanes were still in progress during review; merge should wait for the normal required checks to finish cleanly.
  • [P1] This PR overlaps with other open candidate fixes for the same issue, so maintainers should choose this ingest-boundary approach before closing or superseding the broader helper-level attempts.

Maintainer options:

  1. Gate On Exact-Head CI (recommended)
    Wait for the remaining exact-head CI lanes to finish cleanly before merging the persisted session-ingest change.
  2. Accept The Ingest Boundary
    If maintainers agree malformed tool-result content should be normalized only when read from session JSONL, merge this PR and close or supersede the broader helper-level attempts.
  3. Pause For Broader Contract
    If maintainers want provider helpers to accept arbitrary non-array content from any caller, pause this PR and choose one broader candidate explicitly.

Next step before merge

  • [P1] This is an active protected-label implementation PR with no line-level blocker found; maintainers should resolve exact-head CI and choose this narrow candidate over the overlapping attempts before merge.

Security
Cleared: The diff changes TypeScript session parsing and tests only; it does not broaden dependency, workflow, credential, package, or secret-handling surfaces.

Review details

Best possible solution:

Land this ingest-boundary normalization if exact-head CI stays green, then close the canonical issue and reconcile the broader overlapping PRs as superseded or follow-up only if maintainers want a wider provider-helper contract.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection gives a high-confidence current-main reproduction path: JSONL session load preserves malformed toolResult.content, while provider replay expects a block array and can produce empty output or hit .some() assumptions.

Is this the best way to solve the issue?

Yes. Normalizing once at the JSONL ingest boundary is the best narrow fix for persisted-session replay because it restores the canonical ToolResultMessage contract without spreading malformed-input tolerance across provider and transport consumers.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6e79ca3cbc7d.

Label changes

Label justifications:

  • P2: This is a bounded session/provider replay bugfix that can restore dropped tool output, with limited blast radius and no emergency signal.
  • merge-risk: 🚨 session-state: The patch changes how persisted session JSONL toolResult.content is interpreted when sessions are reopened or cached appends are advanced.
  • merge-risk: 🚨 message-delivery: The affected replay path controls whether successful tool output reaches the next model turn instead of being empty or crashing during conversion.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor proof gate is not applied because this PR carries the protected maintainer label; the PR body and passed Real behavior proof check remain useful supplemental validation.
Evidence reviewed

PR surface:

Source +6, Tests +197. Total +203 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 24 18 +6
Tests 1 197 0 +197
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 221 18 +203

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/agents/sessions/session-manager.tool-result-replay.test.ts src/agents/sessions/session-manager.test.ts src/agents/embedded-agent-runner/transcript-file-state.test.ts.
  • [P1] node scripts/run-vitest.mjs src/agents/compaction.test.ts src/agents/cli-runner/session-history.test.ts.

What I checked:

  • Repository policy read: Read the full root policy and scoped src/agents/AGENTS.md; the protected-label, persisted-session-state, and whole-surface review guidance affected this PR verdict. (AGENTS.md:1, 6e79ca3cbc7d)
  • Scoped agents policy read: The touched src/agents subtree has scoped guidance for agent tests and runtime import discipline; the new test keeps to focused session/provider replay coverage without broad runtime bootstrap. (src/agents/AGENTS.md:1, 6e79ca3cbc7d)
  • Current-main ingest gap: Current main parses JSONL session entries and pushes the parsed FileEntry without normalizing malformed tool-result content, so persisted string/object content can reach replay consumers as a non-array. (src/agents/sessions/session-manager.ts:952, 6e79ca3cbc7d)
  • Canonical message contract: ToolResultMessage.content is typed as a text/image content-block array, which supports fixing the lie at transcript ingest instead of widening all provider callers. (packages/llm-core/src/types.ts:306, 6e79ca3cbc7d)
  • Replay consumers assume arrays: The shared extractor iterates blocks, and sibling provider/transport callers call .some() on msg.content, so non-array content can replay as empty text or hit non-array assumptions. (src/llm/providers/tool-result-text.ts:180, 6e79ca3cbc7d)
  • PR-head fix shape: At PR head, parseJsonlEntries wraps loaded tool-result strings as text blocks and single JSON records as one-element arrays; the verified append-cache path uses the same normalizer before freezing. (src/agents/sessions/session-manager.ts:956, dc5e04ef118b)

Likely related people:

  • obviyus: Current-main blame for the central session/replay files points to Ayaan Zaidi history, and live PR metadata shows obviyus merged the structured tool-result replay predecessor. (role: recent area contributor and predecessor merger; confidence: high; commits: ce9ec4e1c3d2, b63e06f68aa0, 1ececc049f6e; files: src/agents/sessions/session-manager.ts, src/llm/providers/tool-result-text.ts, src/agents/anthropic-transport-stream.ts)
  • snowzlmbot: snowzlmbot authored the merged structured replay work and opened the canonical non-array replay issue that this PR targets. (role: feature contributor and canonical issue author; confidence: high; commits: 524a4932a1ed, b63e06f68aa0; files: src/llm/providers/tool-result-text.ts, src/llm/providers/openai-responses-shared.ts, extensions/google/transport-stream.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 2, 2026
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
Open
@clawsweeper clawsweeper Bot added merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jul 2, 2026
@obviyus
obviyus force-pushed the fix/tool-result-string-replay branch from 8b2d871 to dc5e04e Compare July 2, 2026 04:09
@obviyus obviyus self-assigned this Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Closed
@obviyus
obviyus merged commit e095dc8 into main Jul 2, 2026
96 checks passed
@obviyus
obviyus deleted the fix/tool-result-string-replay branch July 2, 2026 04:19
@obviyus

obviyus commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Landed via squash onto main.

  • Scoped validation: node scripts/run-vitest.mjs src/agents/sessions/session-manager.tool-result-replay.test.ts src/agents/sessions/session-manager.test.ts src/agents/embedded-agent-runner/transcript-file-state.test.ts (76 tests), node scripts/run-vitest.mjs src/agents/compaction.test.ts src/agents/cli-runner/session-history.test.ts (38 tests), node scripts/run-tsgo.mjs -p tsconfig.core.json, node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json, node scripts/run-oxlint.mjs on touched files. Full CI green (52 checks) on dc5e04ef118.
  • Changelog: not edited — CHANGELOG.md is release-only in this repo; release-note context is in the PR body.
  • Land commit: dc5e04e
  • Merge commit: e095dc8

This was referenced Jul 2, 2026
Closed
Closed
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 2, 2026
@obviyus
fix(agents): normalize string tool-result replay (openclaw#98891)
aad08a0
@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Merged
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
Open
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@obviyus @chenyangjun-xy
fix(agents): normalize string tool-result replay (openclaw#98891)
32847ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@obviyus obviyus

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(llm): tool-result replay can drop non-array output content

1 participant

@obviyus