Repository navigation
perf(ai): skip discarded text assembly for image tool results - #140409
Merged
Merged
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
steipete
marked this pull request as ready for review
September 6, 2026 20:05
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 4:19 PM ET / 20:19 UTC. ClawSweeper reviewWhat this changesAvoids assembling unused aggregate text when converting image-bearing tool results into Anthropic request messages. Merge readiness✅ Ready for maintainer review This remains a useful, focused optimization: current main still performs the discarded extraction, and the patch introduces no blocking correctness finding. The maintainer-authored, protected PR should remain available for normal landing. Priority: P3 Review scores
Verification
How this fits togetherOpenClaw’s Anthropic message converter prepares conversation history for both provider and transport requests. It converts tool output into text or ordered text-and-image blocks before the request is sent. flowchart TD
A[Conversation tool results] --> B[Anthropic message converter]
B --> C{Images supported and present?}
C -->|No| D[Extract aggregate text]
C -->|Yes| E[Extract individual blocks and normalize images]
D --> F[Anthropic request messages]
E --> F
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -1. Total -1 across 1 file. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep aggregate extraction local to the text-only branch while preserving the existing per-block image conversion and sanitation path. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the redundant work: an image-bearing tool result causes aggregate extraction whose value is discarded before per-block extraction repeats it. Timing measurements were not rerun. Is this the best way to solve the issue? Yes, moving the computation to its sole consumer is the narrowest repair and preserves both provider and transport output paths without adding another abstraction. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e1c1bc37c77f. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Contributor
Author
|
Merged via squash.
|
vincentkoc
added a commit
that referenced
this pull request
Sep 6, 2026
…rol-ui * origin/main: docs: remove the empty release-sections nav group (#140427) fix(tooling): omit Codex machine squash credit (#140420) chore(ui): refresh control ui locales (#140384) refactor(plugin-sdk): reuse native approval input contracts (#140415) fix: silent model WebSocket streams hang past the idle timeout (#140335) perf(ai): avoid unused text extraction for image results (#140409) fix(ui): distinguish failed GitHub status checks from disconnected accounts (#140333) refactor(cron): share runtime feedback schema fields (#140408) fix: avoid duplicate cloud worker teardown during recovery (#140377) fix(cli): show executable approval scope (#138386) refactor(ui): reuse shared requirement records (#140407) test(gateway): table-drive device token lifecycle checks (#140403) fix(chat): fence delayed history at terminal retirement (#140379) test(skills): compare large upload bytes natively (#140402) refactor(delivery): reuse the canonical rendered batch plan type (#140386) docs(start): repair the first-run path from landing page to first channel (#140390) # Conflicts: # docs/.i18n/glossary.zh-CN.json
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Anthropic tool results containing images assemble an aggregate text result that the image branch never uses, then extract and sanitize each block again. Structured results also repeat serialization and redaction unnecessarily.
Why This Change Was Made
Move aggregate text extraction into its existing text-only consumer. Keep the image branch's per-block extraction, redaction, ordering, normalization and image budget unchanged. This removes one production line and needs no new API, cache, configuration or dependency.
User Impact
In the checksum-corrected R2 comparison, structured/resource image-result medians improved 45–52%, and 20-result histories improved 42–45%, across both orders on image-emitting paths. Provider and transport capability differences remain unchanged, including non-vision transport behavior.
All adverse observations remain: 47/128 medians, 50/128 means and 49/128 maximum batch means worsened. The provider-vision image-only case was +32% (+11.8 µs) forward, while its 64 KiB text/image case was +11.3%/+6.8% (+3.1/+2.0 µs). Peak process RSS increased slightly in both orders. These are payload-conversion measurements, not provider, Gateway, memory-reduction or native-image-codec claims.
Evidence
All 257 existing tests across provider, transport, generic extraction and image-budget suites passed. All 27 normal changed-file checks and fresh independent Codex review through P2 passed; no tests were weakened or added merely to assert a call count.
The fixed B0/C0/C1/B1 cohort calls the actual shared converter with real core redaction and image normalization. Its 64 cases cover all four provider/transport and vision/non-vision combinations. All 9,472 complete input/wire records match both independent fixture expectations and the other variants. All 2,048 retained batch means remain available; individual call timings were not separately recorded. Process closure, restored candidate bytes and selected runtime/source pins were independently verified. No provider request or real credential was used in this projection-only comparison.
R1 remains retained separately: its tiny PNG had an incorrect IDAT checksum, although the compressed data inflated and the actual MIME/normalization route accepted it. R2 corrected only three checksum bytes; file length, compressed pixels, cases, order and timing settings stayed unchanged. All three R2 chunk CRCs validate. R1's measurements and regressions are preserved and were not pooled into or replaced by R2. This proves fixture structure/checksum/inflation, not native codec rendering.