Skip to content

fix(session): prefer real tool result over synthetic error in transcript repair#84419

Merged
steipete merged 6 commits into
openclaw:mainfrom
Jerry-Xin:fix/transcript-repair-prefer-real-result-84134
May 31, 2026
Merged

fix(session): prefer real tool result over synthetic error in transcript repair#84419
steipete merged 6 commits into
openclaw:mainfrom
Jerry-Xin:fix/transcript-repair-prefer-real-result-84134

Conversation

@Jerry-Xin

@Jerry-Xin Jerry-Xin commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix transcript repair to prefer real tool results over synthetic "missing tool result" errors during deduplication.

When a channel (e.g. Feishu) has a flush-race condition, the session-tool-result-guard may write a synthetic error result before the real result arrives from the channel. On the next session load, repairToolUseResultPairing deduplicates by keeping the first-seen result — which is the synthetic error. The user sees "Something went wrong" even though the tool call actually succeeded.

This PR adds isSyntheticMissingToolResult() detection and replaces an existing synthetic result with a later real result during deduplication.

Note: This is a transcript-repair mitigation. The underlying flush-race in the session-tool-result-guard is a separate root-cause concern tracked independently in #84134. This PR does not close that issue.

Changes

File Change
session-transcript-repair.ts Extract DEFAULT_MISSING_TOOL_RESULT_TEXT constant; add isSyntheticMissingToolResult() with exact-match detection; track output positions; replace synthetic with real during dedup; fix droppedDuplicateCount in span-level and pushToolResult paths
session-transcript-repair.test.ts 7 regression tests with toHaveLength(1) assertions: synthetic-first, real-first, two-real, two-synthetic, span-level replacement, substring false-positive guard, changed-flag verification

Tests

$ node scripts/run-vitest.mjs src/agents/session-transcript-repair.test.ts
 ✓ unit-fast src/agents/session-transcript-repair.test.ts (44 tests) 7ms
 Test Files  1 passed (1)
       Tests  44 passed (44)

Real behavior proof

  • Behavior or issue addressed: When a synthetic [openclaw] missing tool result error is persisted before the real result arrives (Feishu channel flush race), the repair pass keeps the synthetic error and the user sees "Something went wrong" even though the tool call succeeded. Additionally, the isSyntheticMissingToolResult detector must use exact text matching to avoid false positives on real errors containing the marker substring.

  • Real environment tested: Docker container (node:24-slim, Node.js v24.16.0, linux arm64) with fresh npm install against local checkout at 307c5a5e.

  • Failing proof before fix: On upstream/main, repairToolUseResultPairing keeps the first-seen result in spanResultsById. When a synthetic error arrives before the real result (flush-race ordering), the synthetic error wins. The old substring-based .includes() detection could also false-positive on real error output containing the marker text.

  • Exact steps or command run after this patch:

docker run --rm -v ./:/app -w /app node:24-slim \
  sh -c 'npm install && npx tsx proof-84419-runner.mts'
  • Evidence after fix:
=== Real Behavior Proof: PR #84419 ===
Commit: 307c5a5e
Node: v24.16.0
Platform: linux arm64

--- Scenario 1: Synthetic error arrived BEFORE real result (Feishu flush race) ---
  Input: 3 messages (1 assistant + 1 synthetic + 1 real)
  Output: 2 messages, 1 tool result(s)
  Kept result isError: false
  Kept result text: "Message sent successfully to #general"
  droppedDuplicateCount: 1
  ✅ PASS: Real result preferred over synthetic error

--- Scenario 2: Real result arrived BEFORE synthetic ---
  Output: 2 messages, 1 tool result(s)
  Kept result isError: false
  droppedDuplicateCount: 1
  ✅ PASS: Real result kept, synthetic dropped

--- Scenario 3: Real error with marker substring is NOT treated as synthetic ---
  Output: 1 tool result(s)
  Kept result text: "[openclaw] missing tool result... (plus extra real error context)"
  ✅ PASS: Real error with superset text kept as first-seen (not misclassified as synthetic)

--- Scenario 4: Span-level synthetic then real within same assistant span ---
  Output: 3 messages, 1 tool result(s)
  Kept result isError: false
  Kept text: "Delivered to Feishu user lark_user_456"
  ✅ PASS: Span-level real result preferred

=== All scenarios complete ===
  • Regression tests in Docker:
 ✓ unit-fast src/agents/session-transcript-repair.test.ts (44 tests) 7ms
 Test Files  1 passed (1)
       Tests  44 passed (44)
  • Observed result after fix: All 4 proof scenarios pass. The repair function correctly:

    1. Replaces synthetic placeholders with real results regardless of arrival order
    2. Uses exact text matching (===) instead of substring (.includes()) to avoid false-positive misclassification of real error output
    3. Accurately counts dropped duplicates via droppedDuplicateCount
    4. Handles span-level deduplication with the same preference logic
  • What was not tested: Live Feishu channel flush-race reproduction (requires specific provider timeout timing). The underlying flush race in session-tool-result-guard ([Bug]: Feishu channel: message tool triggers "missing tool result in session history" in v2026.5.16+ #84134) is not addressed by this PR.

Merge note

⚠️ The first commit message contains Fixes #84134. When squash-merging, please change the merge message to use Refs #84134 instead, so the root-cause Feishu issue remains open.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S proof: supplied External PR includes structured after-fix real behavior proof. labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 12:24 PM ET / 16:24 UTC.

Summary
The branch changes repairToolUseResultPairing to recognize default synthetic missing-tool-result entries, prefer later real duplicate results, update duplicate counts, and add regression tests.

PR surface: Source +60, Tests +160. Total +220 across 2 files.

Reproducibility: yes. for the shared repair path: current main source keeps the first span result for a duplicate id, and the PR proof exercises synthetic-first and real-first constructed transcripts. No live Feishu race reproduction is available, so the linked root-cause issue remains separate.

Review metrics: 1 noteworthy metric.

  • Session repair behavior: 1 dedupe rule changed. Maintainers should notice that existing persisted transcripts can now keep a later real tool result instead of the first default synthetic result.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] Merging intentionally changes how repaired transcripts deduplicate duplicate tool results: a later real result can replace a persisted default synthetic missing-result error, so maintainers should accept this session-state mitigation explicitly.
  • [P1] The proof exercises constructed Docker transcripts through the shared repair path, but it does not live-reproduce the Feishu flush-race timing; the root cause remains open at [Bug]: Feishu channel: message tool triggers "missing tool result in session history" in v2026.5.16+ #84134.

Maintainer options:

  1. Accept the mitigation and keep the root cause open (recommended)
    Merge after normal checks if maintainers agree that preferring a later real default-marker result is the desired repair behavior while the Feishu race remains tracked separately.
  2. Require live Feishu proof first
    Ask for a live Feishu replay only if maintainers want transport-timing evidence before accepting the shared repair-path mitigation.
  3. Pause in favor of the root-cause issue
    Close or pause this PR if maintainers do not want transcript repair to compensate for this race and prefer to handle only the channel flush fix.

Next step before merge

  • [P2] No narrow automated repair remains; maintainers need to decide whether to accept the session-state mitigation and then rely on normal merge/check gates.

Security
Cleared: The diff is limited to transcript repair logic and tests, with no dependency, workflow, secret, package, or code-execution surface added.

Review details

Best possible solution:

Land only if maintainers accept this as a shared transcript-repair mitigation while keeping #84134 open for the channel flush-race root cause.

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

Yes for the shared repair path: current main source keeps the first span result for a duplicate id, and the PR proof exercises synthetic-first and real-first constructed transcripts. No live Feishu race reproduction is available, so the linked root-cause issue remains separate.

Is this the best way to solve the issue?

Yes as a narrow mitigation: the diff changes only transcript repair dedupe for the default synthetic marker and leaves the guard race untouched. The safer end state is to land this only with maintainer acceptance that it is mitigation, not the root-cause fix.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 5b310a7b27fa.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR provides Docker terminal proof for the changed repair behavior using constructed transcripts and the focused regression test output; live Feishu race proof is intentionally out of scope for this mitigation.

Label justifications:

  • P1: The linked failure is a broken agent/channel workflow where successful tool calls can be surfaced to users as generic errors.
  • merge-risk: 🚨 session-state: The patch changes transcript repair selection for duplicate tool results, which can alter persisted session history replay behavior.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR provides Docker terminal proof for the changed repair behavior using constructed transcripts and the focused regression test output; live Feishu race proof is intentionally out of scope for this mitigation.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR provides Docker terminal proof for the changed repair behavior using constructed transcripts and the focused regression test output; live Feishu race proof is intentionally out of scope for this mitigation.
Evidence reviewed

PR surface:

Source +60, Tests +160. Total +220 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 65 5 +60
Tests 1 160 0 +160
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 225 5 +220

What I checked:

  • Current main keeps the first duplicate span result: On current main, repairToolUseResultPairing only stores a span result when spanResultsById does not already have that id, so a default synthetic result that appears first wins over a later real result in the same span. (src/agents/session-transcript-repair.ts:582, 5b310a7b27fa)
  • PR replaces default synthetic duplicates with real results: The PR head adds exact default synthetic-result detection plus output-position tracking so a later non-synthetic duplicate can replace an already-pushed default synthetic result. (src/agents/session-transcript-repair.ts:519, 5f84700fb549)
  • PR covers both span and duplicate-count behavior: The PR head adds synthetic-first, real-first, two-real, two-synthetic, span-level replacement, substring false-positive, and changed-flag duplicate tests. (src/agents/session-transcript-repair.test.ts:421, 5f84700fb549)
  • Synthetic source uses the same default text: The session tool-result guard creates missing-result synthetics through makeMissingToolResult with missingToolResultText undefined for non-OpenAI paths, matching the default marker the PR detects. (src/agents/session-tool-result-guard.ts:648, 5b310a7b27fa)
  • Codex contract checked for custom aborted output: Upstream Codex inserts missing function-call outputs with the text aborted, which supports keeping OpenClaw's custom OpenAI/Codex missingToolResultText path separate from the default OpenClaw marker detection. (../codex/codex-rs/core/src/context_manager/normalize.rs:34, cdde711fac00)
  • Contributor real behavior proof: The PR body and refreshed comment include Docker terminal output for four constructed transcript scenarios using the actual repair path, plus the focused session-transcript-repair.test.ts run; they explicitly do not live-reproduce the Feishu race. (5f84700fb549)

Likely related people:

  • @steipete: Current blame and history show Peter Steinberger carrying the extracted transcript repair module, session tool-result guard, and most recent central implementation lines. (role: primary area owner; confidence: high; commits: d1bec469af22, 08cc8f2281f7, f5d5661adfec; files: src/agents/session-transcript-repair.ts, src/agents/session-tool-result-guard.ts)
  • Shakker: Recent history shows a run of signed-thinking and replay-sanitization changes in the same transcript repair area, including preserving and gating replay repair behavior. (role: recent adjacent contributor; confidence: medium; commits: 2d1f4af67ab4, 408e07f96b6f, 941aca5e5eb7; files: src/agents/session-transcript-repair.ts)
  • Aisling Cahill: Authored the aborted/errored assistant pairing fix that protects nearby transcript repair semantics for malformed tool-use turns. (role: adjacent repair contributor; confidence: medium; commits: 861725fba1b7; files: src/agents/session-transcript-repair.ts, src/agents/session-transcript-repair.test.ts)
  • Clayton Shaw: Authored the orphaned tool_result compaction repair that overlaps the same session-state cleanup boundary. (role: adjacent compaction repair contributor; confidence: medium; commits: 6ba4d0ddc324; files: src/agents/session-transcript-repair.ts, src/agents/session-transcript-repair.test.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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg: ✨ hatched 🥚 common Clockwork Clawlet. Rarity: 🥚 common. Trait: sniffs out flaky tests.

Details

Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Clockwork Clawlet in ClawSweeper.
Hatchability:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

About:

  • Eggs appear after real-behavior proof passes. They are collectible flavor only.
  • Review momentum changes the shell state: follow-up work warms it, re-review makes it wobble, and a clean final review lets it hatch.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@Jerry-Xin
Jerry-Xin force-pushed the fix/transcript-repair-prefer-real-result-84134 branch from ef458a8 to 953e136 Compare May 20, 2026 04:08
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Real behavior proof

  • Behavior or issue addressed: When transcript repair encounters both a synthetic [openclaw] missing tool result error and a real tool result for the same toolCallId, the synthetic placeholder should be replaced by the real result instead of keeping whichever appeared first.
  • Real environment tested: Local patched OpenClaw checkout at 953e1365dd, macOS arm64, Node.js v24.10.0.
  • Failing proof before fix: The 5 new tests in the repairToolUseResultPairing prefers real result over synthetic error describe block do not exist on upstream/main. Running upstream/main's version of the test file yields 35 tests, 35 passed — the preference logic is absent, so when a synthetic error is emitted first and a real result follows, the synthetic is kept and the real result is silently dropped as a duplicate.
  • Exact steps or command run after this patch:
    git checkout origin/fix/transcript-repair-prefer-real-result-84134 --detach
    node scripts/run-vitest.mjs src/agents/session-transcript-repair.test.ts
    
  • Evidence after fix:
     ✓ unit-fast ../../src/agents/session-transcript-repair.test.ts (40 tests) 5ms
    
     Test Files  1 passed (1)
          Tests  40 passed (40)
       Start at  12:10:39
       Duration  155ms (transform 41ms, setup 0ms, import 53ms, tests 5ms, environment 0ms)
    
  • Observed result after fix: All 40 tests pass (35 existing + 5 new). The new tests cover: (1) synthetic-first then real-second → keeps real, (2) real-first then synthetic-second → keeps real, (3) two real results → keeps first (unchanged behavior), (4) two synthetic errors → keeps first (unchanged behavior), (5) span-level synthetic-then-real within a span → picks real. This confirms the preference logic correctly replaces synthetic placeholders while preserving existing deduplication behavior.
  • What was not tested: No live session replay was performed; all tests use in-memory message arrays. The isSyntheticMissingToolResult detection only matches the default marker text — custom missingToolResultText (e.g. Codex "aborted") is intentionally not detected, as documented in the code comments.

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Real behavior proof (live runtime)

  • Behavior or issue addressed: Session transcript repair prefers synthetic "missing tool result" error over real result when both exist for the same toolCallId.

  • Real environment tested: Local OpenClaw checkout at ef458a80c6 (fix), macOS arm64, Node.js v24.10.0.

  • Failing proof before fix: On main, the dedup logic keeps the first-seen entry. When a synthetic error is inserted before the real tool result arrives (race condition during gateway timeout), the synthetic error permanently replaces the real result.

  • Exact steps or command run after this patch:

$ node /tmp/test-transcript-repair-live.mjs

--- Scenario: Synthetic error arrived BEFORE real result ---
Transcript entries for toolCallId 'call_abc123':
  Entry 1 (synthetic): "[openclaw] missing tool result in session history; inserted ..."
  Entry 2 (real): "Message sent successfully to #general"

--- BEFORE FIX (main branch behavior) ---
  Kept: SYNTHETIC ERROR ❌

--- AFTER FIX (this PR) ---
  Decision: replaced synthetic error with real result
  Kept: REAL RESULT ✓
  Content: "Message sent successfully to #general"

--- Scenario 2: Real result arrived BEFORE synthetic ---
  Decision: kept existing real result, discarded incoming synthetic
  Kept: REAL RESULT ✓
  • Evidence after fix: Both ordering scenarios produce the correct result — the real tool result is always preferred over the synthetic error marker.
  • Observed result after fix: The dedup function now checks isSyntheticMissingToolResult() and replaces synthetic entries with real ones regardless of arrival order.
  • What was not tested: Live Feishu/session persistence race condition was not reproduced (requires specific provider timeout timing). The fix logic is validated against constructed transcript entries matching the exact synthetic error format used in production.

@openclaw-barnacle openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. and removed proof: supplied External PR includes structured after-fix real behavior proof. labels May 20, 2026
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. label May 20, 2026
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Updated: scoped as transcript-repair mitigation

Based on ClawSweeper's review, this PR is now explicitly scoped as a transcript-repair mitigation, not a complete Feishu channel fix.

Addressing ClawSweeper concerns:

Proof: Added before/after proof running the actual repairToolUseResultPairing function from the source tree against a realistic Feishu session scenario:

  • Main branch: synthetic error kept → user sees "Something went wrong" ❌
  • Fix branch: real result preferred → user sees successful delivery ✅
  • Both orderings tested (synthetic-first, real-first)

Scope: The underlying Feishu flush race (#84134) is tracked separately. This PR ensures that even when the race occurs and both a synthetic error and real result are persisted, the transcript repair pass prefers the real result.

Patch quality: ClawSweeper rated patch quality at 🐚 platinum hermit. The implementation is narrow and well-tested (5 new regression tests, 40/40 total pass).

Signal for maintainers:

This aligns with ClawSweeper's Option 2: "Accept as partial mitigation." The transcript-repair fix is independently valuable — any channel can hit this flush-race pattern, not just Feishu.

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 20, 2026
@Jerry-Xin
Jerry-Xin force-pushed the fix/transcript-repair-prefer-real-result-84134 branch from 953e136 to d9c8f5e Compare May 20, 2026 08:22
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 20, 2026
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Rebased on latest upstream/main (110042d, clean rebase, no conflicts).

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 20, 2026
@Jerry-Xin
Jerry-Xin force-pushed the fix/transcript-repair-prefer-real-result-84134 branch from d9c8f5e to 86fc180 Compare May 20, 2026 10:06
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 20, 2026
@Jerry-Xin
Jerry-Xin force-pushed the fix/transcript-repair-prefer-real-result-84134 branch from 1a86a16 to f41d85b Compare May 29, 2026 06:15
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 29, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 29, 2026
@RomneyDa

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@Jerry-Xin
Jerry-Xin force-pushed the fix/transcript-repair-prefer-real-result-84134 branch from f41d85b to 27a7b19 Compare May 29, 2026 08:07
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 29, 2026
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Rebased on latest upstream/main (e01b04d, clean rebase, no conflicts) — should resolve the Dependency Guard check.

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

PR body cleaned up — addressing ClawSweeper feedback

Changes made:

  1. Proof labels reformatted to exact parser-compatible format (added missing "Exact steps or command run after this patch" and "Observed result after fix" sections)
  2. Scope clarified — explicitly noted this is a transcript-repair mitigation, not a root-cause fix for [Bug]: Feishu channel: message tool triggers "missing tool result in session history" in v2026.5.16+ #84134
  3. Merge note added — flagged that the first commit contains Fixes #84134 closing syntax; squash merge message should use Refs #84134 to keep the root-cause issue open
  4. Removed closing syntax from PR body — all references to [Bug]: Feishu channel: message tool triggers "missing tool result in session history" in v2026.5.16+ #84134 now use plain #84134 without closing keywords

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Real behavior proof (refreshed for current HEAD)

  • Behavior or issue addressed: When transcript repair encounters both a synthetic [openclaw] missing tool result error and a real tool result for the same toolCallId, the synthetic placeholder should be replaced by the real result. Additionally, the isSyntheticMissingToolResult detector must use exact text matching to avoid false positives on real errors containing the marker substring.

  • Real environment tested: Docker container (node:24-slim, Node.js v24.16.0, linux arm64) with fresh npm install against local checkout at 307c5a5e.

  • Failing proof before fix: On upstream/main, repairToolUseResultPairing keeps the first-seen result in spanResultsById. When a synthetic error arrives before the real result (flush-race ordering), the synthetic error wins. The old substring-based .includes() detection could also false-positive on real error output containing the marker text.

  • Exact steps or command run after this patch:

docker run --rm -v /tmp/openclaw-84419-docker:/app -w /app node:24-slim \
  sh -c 'npm install && npx tsx proof-84419-runner.mts'
  • Evidence after fix:
=== Real Behavior Proof: PR #84419 ===
Commit: 307c5a5e
Node: v24.16.0
Platform: linux arm64

--- Scenario 1: Synthetic error arrived BEFORE real result (Feishu flush race) ---
  Input: 3 messages (1 assistant + 1 synthetic + 1 real)
  Output: 2 messages, 1 tool result(s)
  Kept result isError: false
  Kept result text: "Message sent successfully to #general"
  droppedDuplicateCount: 1
  ✅ PASS: Real result preferred over synthetic error

--- Scenario 2: Real result arrived BEFORE synthetic ---
  Output: 2 messages, 1 tool result(s)
  Kept result isError: false
  droppedDuplicateCount: 1
  ✅ PASS: Real result kept, synthetic dropped

--- Scenario 3: Real error with marker substring is NOT treated as synthetic ---
  Output: 1 tool result(s)
  Kept result text: "[openclaw] missing tool result... (plus extra real error context)"
  ✅ PASS: Real error with superset text kept as first-seen (not misclassified as synthetic)

--- Scenario 4: Span-level synthetic then real within same assistant span ---
  Output: 3 messages, 1 tool result(s)
  Kept result isError: false
  Kept text: "Delivered to Feishu user lark_user_456"
  ✅ PASS: Span-level real result preferred

=== All scenarios complete ===
  • Regression tests in Docker:
 ✓ unit-fast src/agents/session-transcript-repair.test.ts (44 tests) 7ms
 Test Files  1 passed (1)
       Tests  44 passed (44)
  • Observed result after fix: All 4 proof scenarios pass. The repair function correctly:

    1. Replaces synthetic placeholders with real results regardless of arrival order
    2. Uses exact text matching (===) instead of substring (.includes()) to avoid false-positive misclassification of real error output
    3. Accurately counts dropped duplicates via droppedDuplicateCount
    4. Handles span-level deduplication with the same preference logic
  • What was not tested: Live Feishu channel flush-race reproduction (requires specific provider timeout timing). The underlying flush race in session-tool-result-guard ([Bug]: Feishu channel: message tool triggers "missing tool result in session history" in v2026.5.16+ #84134) is not addressed by this PR.

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Rebased on latest upstream/main (clean rebase, no conflicts). Fixed closing reference: changed Fixes #84134 to Ref #84134 in the first commit message so that squash-merging this mitigation PR will not auto-close the still-open root-cause issue.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

忻役 and others added 3 commits June 1, 2026 00:17
…ipt repair

When a message tool call succeeds but its result races with a premature
flush of pending tool state, the transcript can contain both a synthetic
"missing tool result" error and the real result for the same toolCallId.

Previously, repairToolUseResultPairing kept the first result encountered
(often the synthetic error), discarding the real one. Now it prefers
non-error results over synthetic missing-result errors when deduplicating.

Ref openclaw#84134
…esults

When a real tool result is already recorded and a synthetic duplicate
arrives second, the duplicate is correctly dropped but changed was not
set to true. This caused the repair function to incorrectly report no
modifications when the transcript was actually altered.

Add an else branch to set changed = true whenever a duplicate tool
result (synthetic or repeated real) is dropped from the span.
…e count

- Extract DEFAULT_MISSING_TOOL_RESULT_TEXT constant and use exact match
  instead of substring .includes() to prevent false positives on real
  error output containing the marker text
- Add toHaveLength(1) assertions to all duplicate-preference tests so
  they verify deduplication actually removes the extra entry
- Increment droppedDuplicateCount in span-level and pushToolResult
  replacement paths that were previously undercounting
- Add edge case tests: substring false-positive guard and changed flag
  verification

Refs openclaw#84134
@Jerry-Xin

Copy link
Copy Markdown
Contributor Author

Rebased on latest upstream/main (5f84700, clean rebase, no conflicts).

@steipete

Copy link
Copy Markdown
Contributor

Land-ready proof for head c7d2740.

Best-fix judgment: this is the right shared transcript-repair mitigation for duplicated synthetic/missing tool results versus later real tool results. It does not close the root Feishu timing race in #84134; that remains the broader follow-up.

Local proof:

  • node scripts/run-vitest.mjs src/agents/session-transcript-repair.test.ts src/agents/session-tool-result-guard.test.ts
  • Result: 83 tests passed.

Autoreview:

  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --parallel-tests "node scripts/run-vitest.mjs src/agents/session-transcript-repair.test.ts src/agents/session-tool-result-guard.test.ts"
  • Result: clean; no accepted/actionable findings.

GitHub proof:

  • CI run 26719731616 passed, including checks-node-agentic-agents-core and related shards.
  • Real behavior proof run 26719737260 passed.
  • Dependency guard run 26719730799 passed.
  • Security/OpenGrep/CodeQL-relevant checks completed pass/skip as applicable for the changed paths.

Known gap: no live Feishu timing repro was run for the upstream race; this PR repairs the shared transcript state when the duplicate-result state already exists.

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

Labels

agents Agent runtime and tooling merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants