Skip to content

fix(agents): keep compaction safeguard summary truncation UTF-16 safe#101692

Closed
chengzhichao-xydt wants to merge 3 commits into
openclaw:mainfrom
chengzhichao-xydt:pr/utf16-safe-compaction-safeguard
Closed

fix(agents): keep compaction safeguard summary truncation UTF-16 safe#101692
chengzhichao-xydt wants to merge 3 commits into
openclaw:mainfrom
chengzhichao-xydt:pr/utf16-safe-compaction-safeguard

Conversation

@chengzhichao-xydt

@chengzhichao-xydt chengzhichao-xydt commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • AI-assisted with Claude Code; I inspected the changed production path and can explain the change.
  • Compaction safeguard summary and context truncation in truncateFailureText, capCompactionSummary, formatRecentTurnsText, and formatSummaryContext now preserves UTF-16 boundaries when the character cap lands between the code units of an emoji or CJK surrogate pair.
  • Uses the existing truncateUtf16Safe helper from @openclaw/normalization-core/utf16-slice instead of raw .slice(0, N) at five truncation sites.
  • Intentionally out of scope: unrelated truncation sites in other modules, config changes, compaction logic changes, and any behavior outside this specific truncation safety boundary.

What Problem This Solves

Compaction summaries, failure text, recent-turn text, and summary context are truncated to fit character budgets before being included in the model prompt. When those cap boundaries fall between the two UTF-16 code units of an emoji, the raw .slice(0, N) produces a lone surrogate — a malformed string that may cause rendering issues or unexpected model behavior.

Why This Change Was Made

The existing shared truncateUtf16Safe helper already handles this boundary correctly and is used by 15+ call sites across the codebase. The five truncation sites in compaction-safeguard.ts were among the remaining raw-slice sites in src/agents/.

Evidence

  • Tests added: 3 new test cases — emoji at capCompactionSummary truncation boundary, emoji at truncateFailureText boundary (tested via capCompactionSummary with marker budget), and emoji at capCompactionSummaryPreservingSuffix body truncation. All assert no lone surrogates in the output.
  • Existing tests: 97/97 pass unchanged.
  • Test run: node scripts/run-vitest.mjs run src/agents/agent-hooks/compaction-safeguard.test.ts — 100 passed (97 existing + 3 new).

@Alix-007 @claude
fix(agents): keep compaction safeguard summary truncation UTF-16 safe
b01c5dc 
Replace raw string slicing with truncateUtf16Safe in truncateFailureText,
capCompactionSummary, formatRecentTurnsText, and formatSummaryContext so
emoji and CJK surrogate pairs in compaction summaries and context are not
split mid-pair at the truncation caps.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: this PR is a useful partial compaction fix, but it still misses the suffix-tail UTF-16 boundary, while #101711 covers the same compaction paths plus the missing tail branch with positive proof.

Root-cause cluster
Relationship: superseded
Canonical: #101711
Summary: The current PR covers the compaction-safeguard subset of the same bounded-text UTF-16 problem, but the broader open PR is the canonical replacement because it also fixes the missing suffix-tail edge and carries positive proof.

Members:

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

Canonical path: Close this branch as superseded and land the consolidated UTF-16 bounded-text fix through #101711 after its own review cleanup.

So I’m closing this here and keeping the remaining discussion on #101711.

Review details

Best possible solution:

Close this branch as superseded and land the consolidated UTF-16 bounded-text fix through #101711 after its own review cleanup.

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

Yes. Source inspection plus a raw-slice probe show the remaining suffix.slice(-maxChars) branch can start on a low surrogate when the cap cuts through an emoji; I did not run a full live compaction session for this branch.

Is this the best way to solve the issue?

No. Replacing prefix truncations with truncateUtf16Safe is a good partial fix, but the best solution also uses sliceUtf16Safe for the oversized suffix-tail path, which the consolidated replacement PR does.

Security review:

Security review cleared: The diff only imports an existing internal normalization helper and adds tests; I found no concrete security or supply-chain regression.

AGENTS.md: found and applied where relevant.

What I checked:

  • Current PR head still misses suffix-tail truncation: At head 829d8d9bba6e8f1fb43b6984cc9697797ea2ed48, capCompactionSummaryPreservingSuffix still returns suffix.slice(-maxChars) when the preserved suffix alone exceeds the cap. (src/agents/agent-hooks/compaction-safeguard.ts:590, 829d8d9bba6e)
  • Canonical replacement covers the missing branch: The broader replacement PR head imports sliceUtf16Safe and changes the same oversized-suffix branch to sliceUtf16Safe(suffix, -maxChars). (src/agents/agent-hooks/compaction-safeguard.ts:591, 20a2c38d2c25)
  • Canonical replacement adds the missing regression: The broader replacement PR adds a regression asserting the oversized preserved suffix path keeps UTF-16 valid at the leading edge. (src/agents/agent-hooks/compaction-safeguard.test.ts:553, 20a2c38d2c25)
  • Helper contract supports safe tail slicing: sliceUtf16Safe handles negative starts and adjusts both slice edges so callers do not return dangling surrogate halves. (packages/normalization-core/src/utf16-slice.ts:16, c8d95da14c5b)
  • Remaining bug is source-reproducible: A read-only Node probe of the same raw slice(-maxChars) shape returned a leading lone low surrogate ("\ude80tail"), matching the prior review's suffix-tail concern.
  • Replacement PR is the viable canonical target: The related PR is open, mergeable, labeled proof: sufficient and status: 👀 ready for maintainer look, and its body reports focused Testbox and live proof for the broader bounded-text fix. (20a2c38d2c25)

Likely related people:

  • steipete: Authored the broader replacement PR that covers this compaction suffix-tail edge and appears in current-main blame for the grafted compaction helper block. (role: recent consolidation owner; confidence: high; commits: 20a2c38d2c25, 903a09ab858d; files: src/agents/agent-hooks/compaction-safeguard.ts, src/agents/agent-hooks/compaction-safeguard.test.ts)
  • Pandadadadazxf: Merged PR history shows this author added the earlier compaction summary budget and truncation-marker behavior that the current path descends from. (role: introduced earlier summary budget behavior; confidence: medium; commits: 9d15f5dd746a; files: src/agents/pi-extensions/compaction-safeguard.ts, src/agents/pi-extensions/compaction-safeguard.test.ts)
  • nxmxbbd: Recent merged history touched the same compaction-safeguard module for tool-failure summary behavior. (role: recent compaction-safeguard contributor; confidence: medium; commits: 079397b2c385; files: src/agents/agent-hooks/compaction-safeguard.ts, src/agents/agent-hooks/compaction-safeguard.test.ts)
  • hugenshen: Recent merged history changed the same compaction-safeguard path and adjacent compaction tests for provider abort handling. (role: recent adjacent compaction contributor; confidence: medium; commits: 509a193de6f3; files: src/agents/agent-hooks/compaction-safeguard.ts, src/agents/agent-hooks/compaction-safeguard.test.ts, src/agents/compaction.ts)

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

@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. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@chengzhichao-xydt

chengzhichao-xydt commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

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.

@steipete steipete mentioned this pull request Jul 7, 2026
Merged
@Alix-007 @claude
fix(agents): avoid variable shadow in compaction safeguard test
829d8d9 
Use testing.capCompactionSummaryPreservingSuffix alias instead of destructuring
to avoid eslint no-shadow with the upper-scope import.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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 Jul 7, 2026
@Alix-007 @claude
fix(test): restore capCompactionSummaryPreservingSuffix reference in …
2074284 
…existing test

The earlier sed fix was too aggressive and renamed the function call in
an existing test (line 462) to 'capper', which is only defined in the
new test. Restore the correct function reference.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@chengzhichao-xydt

chengzhichao-xydt commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@chengzhichao-xydt

chengzhichao-xydt commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #101711 which covers the same compaction UTF-16 truncation paths plus the missing suffix.slice(-maxChars) tail branch. Closing in favor of the canonical consolidated fix.

@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Superseded by #101711, now landed as 87fe266c5e4d790e8f15278ecc417d3a4d418b91.

The landed compaction fix covers the proposed summary prefixes and recent/workspace sections, and improves the suffix-only fallback by using a UTF-16-safe negative-start slice so the prioritized tail stays intact without a malformed leading surrogate. Exact-head CI, focused compaction tests, full check:changed, and autoreview all passed.

Thanks @chengzhichao-xydt for finding the compaction boundary.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chengzhichao-xydt @steipete @Alix-007