Skip to content

fix(codex): keep app-inventory cache/runtime label truncation UTF-16 safe#101634

Closed
lsr911 wants to merge 1 commit into
openclaw:mainfrom
lsr911:fix/codex-utf16
Closed

fix(codex): keep app-inventory cache/runtime label truncation UTF-16 safe#101634
lsr911 wants to merge 1 commit into
openclaw:mainfrom
lsr911:fix/codex-utf16

Conversation

@lsr911

@lsr911 lsr911 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Two Codex app-server call sites truncate text with raw .slice(0, N):

  • app-inventory-cache: redacted error data strings @500 — agent tool/function output
  • run-attempt: dynamic tool runtime type label @80 — agent-generated type info

Both paths handle text from agent output that may contain emoji/CJK characters.

Why This Change Was Made

truncateUtf16Safe from openclaw/plugin-sdk/text-utility-runtime is the SDK-appropriate import for extension code.

Evidence

Branch prerun (commit adadfa0)

=== app-inventory @500 boundary ===
Raw .slice(0,500) ends: "x\ud83d" (dangling high surrogate)
truncateUtf16Safe: len=499 | no U+FFFD: true

=== run-attempt label @80 boundary ===
Raw .slice(0,80) ends: "x\ud83d" (dangling)
truncateUtf16Safe: len=79 | no U+FFFD: true

Files Changed

File Change
extensions/codex/src/app-server/app-inventory-cache.ts .slice(0, 500)truncateUtf16Safe + import
extensions/codex/src/app-server/run-attempt.ts .slice(0, 80)truncateUtf16Safe + import

…safe

Replace raw .slice(0, N) with truncateUtf16Safe in two Codex
app-server paths:

- app-inventory-cache: redacted error data strings @500
- run-attempt: dynamic tool runtime type label @80

Both truncate user/agent-generated text that may contain emoji
or CJK characters.
@openclaw-barnacle openclaw-barnacle Bot added extensions: codex size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 7, 2026, 8:23 AM ET / 12:23 UTC.

Summary
The branch imports truncateUtf16Safe into the Codex app-server inventory cache and run-attempt path, replacing raw truncation for serialized error data and unsupported dynamic-tool type labels.

PR surface: Source +52. Total +52 across 2 files.

Reproducibility: yes. from source and PR proof: current main uses raw .slice at the reported 500 and 80 UTF-16 code-unit boundaries, and the PR body shows the dangling-surrogate case for both. I did not run a local repro because this review was read-only.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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:

  • none.

Next step before merge

  • No ClawSweeper repair lane is needed; ordinary PR review and merge gates can handle this focused, clean bugfix.

Security
Cleared: The diff only adds an existing local SDK helper import and replaces two string truncation calls; it does not touch dependencies, workflows, lockfiles, secrets, or executable supply-chain surfaces.

Review details

Best possible solution:

Land a narrow Codex app-server fix that keeps extension code on the existing Plugin SDK text helper and leaves broader truncation sweeps to separate focused PRs.

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

Yes, from source and PR proof: current main uses raw .slice at the reported 500 and 80 UTF-16 code-unit boundaries, and the PR body shows the dangling-surrogate case for both. I did not run a local repro because this review was read-only.

Is this the best way to solve the issue?

Yes: the existing truncateUtf16Safe SDK helper is the narrowest maintainable fix for extension code, and sibling Codex protocol inspection confirms the runtime label path handles string dynamic-tool types. A new local sanitizer or SDK surface would add unnecessary duplication.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2ba622ca3019.

Label changes

Label changes:

  • add P2: This is a concrete Codex app-server text-corruption bug with limited blast radius and a narrow existing-helper fix.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal-style after-fix proof for both changed boundaries, showing raw slice producing a dangling high surrogate and truncateUtf16Safe avoiding replacement-character corruption.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal-style after-fix proof for both changed boundaries, showing raw slice producing a dangling high surrogate and truncateUtf16Safe avoiding replacement-character corruption.

Label justifications:

  • P2: This is a concrete Codex app-server text-corruption bug with limited blast radius and a narrow existing-helper fix.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal-style after-fix proof for both changed boundaries, showing raw slice producing a dangling high surrogate and truncateUtf16Safe avoiding replacement-character corruption.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal-style after-fix proof for both changed boundaries, showing raw slice producing a dangling high surrogate and truncateUtf16Safe avoiding replacement-character corruption.
Evidence reviewed

PR surface:

Source +52. Total +52 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 2 74 22 +52
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 74 22 +52

What I checked:

  • Current main app-inventory behavior: redactErrorData currently truncates long string data with value.slice(0, 500), which can cut between UTF-16 surrogate halves when the boundary lands inside an emoji or other supplementary-plane character. (extensions/codex/src/app-server/app-inventory-cache.ts:362, 2ba622ca3019)
  • Current main runtime-label behavior: formatUnsupportedCodexDynamicToolOutput currently derives the 80-character label with rawType.slice(0, 80), so source inspection supports the PR's reported boundary case. (extensions/codex/src/app-server/run-attempt.ts:406, 2ba622ca3019)
  • Existing helper contract: truncateUtf16Safe clamps to the requested length and delegates to sliceUtf16Safe, whose end-boundary logic backs off when the last included code unit would be a high surrogate followed by a low surrogate. (packages/normalization-core/src/utf16-slice.ts:44, 2ba622ca3019)
  • Helper regression coverage: Existing tests cover truncateUtf16Safe at surrogate-pair boundaries, including the case where truncating 👨👩 at code-unit length 1 returns an empty string instead of a dangling surrogate. (packages/normalization-core/src/utf16-slice.test.ts:84, 2ba622ca3019)
  • Codex protocol contract checked: The sibling Codex protocol defines dynamic tool output content as inputText with string text or inputImage with string imageUrl, matching OpenClaw's transcript mapping path for unsupported item types. (../codex/codex-rs/app-server-protocol/schema/typescript/v2/DynamicToolCallOutputContentItem.ts:5)
  • Codex app-server response path checked: The sibling Codex app-server decodes DynamicToolCallResponse, converts content_items into core dynamic-tool response items, and falls back to an inputText error on invalid responses, so the OpenClaw mapping is on the real Codex runtime path. (../codex/codex-rs/app-server/src/dynamic_tools.rs:35)

Likely related people:

  • steipete: Recent GitHub path history shows substantial Codex app-server run-attempt.ts refactoring and adjacent UTF-16 Codex test follow-up, including merged work in fix(codex): use truncateUtf16Safe for attempt notification text truncation #101534 and fix(codex): use UTF-16-safe truncation for approval display paths #100177. (role: recent area contributor; confidence: high; commits: 554d772c1a83, 34f168c140df, 2525a7e84a2a; files: extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/attempt-notifications.test.ts, extensions/codex/src/app-server/approval-bridge.test.ts)
  • kevinslin: GitHub path history for app-inventory-cache.ts shows repeated Codex plugin app inventory and cache changes authored by kevinslin around the current cache behavior. (role: feature owner; confidence: high; commits: a1ac559ed7e6, f169e0aafde8, 389c355bcf3f; files: extensions/codex/src/app-server/app-inventory-cache.ts)
  • dwc1997: Authored the merged tests for the shared surrogate-safe UTF-16 slicing helpers that this PR reuses. (role: adjacent owner; confidence: medium; commits: 5de7c6c9710f, 143cdba51e62; files: packages/normalization-core/src/utf16-slice.test.ts)
  • lsr911: Beyond authoring this PR, lsr911 also authored merged adjacent Codex UTF-16-safe truncation work for attempt notifications. (role: adjacent contributor; confidence: medium; commits: ecd7135771f6; files: extensions/codex/src/app-server/attempt-notifications.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 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. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Closed as superseded by #101685, landed in 176fee5d071dd03f6cbbc54b029f23380b04e911. The replacement carries this useful UTF-16 boundary fix on current main, uses the existing shared helper, expands caller-level coverage, and preserves your commit credit in the consolidated commit. Thank you, @lsr911, for finding and fixing this.

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

Labels

extensions: codex P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S 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.

2 participants