fix(session): prefer real tool result over synthetic error in transcript repair#84419
Conversation
|
Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 12:24 PM ET / 16:24 UTC. Summary 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.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest 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 changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +60, Tests +160. Total +220 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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 PR egg: ✨ hatched 🥚 common Clockwork Clawlet. Rarity: 🥚 common. Trait: sniffs out flaky tests. DetailsShare on X: post this hatch
About:
|
ef458a8 to
953e136
Compare
Real behavior proof
|
Real behavior proof (live runtime)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Updated: scoped as transcript-repair mitigationBased 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
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. |
|
@clawsweeper re-review |
953e136 to
d9c8f5e
Compare
|
Rebased on latest upstream/main (110042d, clean rebase, no conflicts). |
d9c8f5e to
86fc180
Compare
1a86a16 to
f41d85b
Compare
|
Heads up: this PR needs to be updated against current |
f41d85b to
27a7b19
Compare
|
Rebased on latest upstream/main (e01b04d, clean rebase, no conflicts) — should resolve the Dependency Guard check. |
PR body cleaned up — addressing ClawSweeper feedbackChanges made:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Real behavior proof (refreshed for current HEAD)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Rebased on latest upstream/main (clean rebase, no conflicts). Fixed closing reference: changed @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…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
|
Rebased on latest upstream/main (5f84700, clean rebase, no conflicts). |
|
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:
Autoreview:
GitHub proof:
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. |
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,
repairToolUseResultPairingdeduplicates 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.Changes
session-transcript-repair.tsDEFAULT_MISSING_TOOL_RESULT_TEXTconstant; addisSyntheticMissingToolResult()with exact-match detection; track output positions; replace synthetic with real during dedup; fixdroppedDuplicateCountin span-level and pushToolResult pathssession-transcript-repair.test.tstoHaveLength(1)assertions: synthetic-first, real-first, two-real, two-synthetic, span-level replacement, substring false-positive guard, changed-flag verificationTests
Real behavior proof
Behavior or issue addressed: When a synthetic
[openclaw] missing tool resulterror 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, theisSyntheticMissingToolResultdetector 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 freshnpm installagainst local checkout at307c5a5e.Failing proof before fix: On
upstream/main,repairToolUseResultPairingkeeps the first-seen result inspanResultsById. 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'Observed result after fix: All 4 proof scenarios pass. The repair function correctly:
===) instead of substring (.includes()) to avoid false-positive misclassification of real error outputdroppedDuplicateCountWhat 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
Fixes #84134. When squash-merging, please change the merge message to useRefs #84134instead, so the root-cause Feishu issue remains open.