Skip to content

fix(skills): preserve emoji in long history scans#107720

Merged
steipete merged 2 commits into
openclaw:mainfrom
Leon-SK668:leon/utf16-history-scan
Jul 16, 2026
Merged

fix(skills): preserve emoji in long history scans#107720
steipete merged 2 commits into
openclaw:mainfrom
Leon-SK668:leon/utf16-history-scan

Conversation

@Leon-SK668

@Leon-SK668 Leon-SK668 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where Skill Workshop history scanning could produce malformed provider-bound transcript text when its per-session cap split an emoji or another supplementary character in the retained prefix, head, or tail.

Why This Change Was Made

The downstream history-scan cap now uses the normalization core's UTF-16-safe prefix and slice helpers while preserving the existing budget, omission marker, redaction order, and head/tail layout. This is distinct from #106370, which addresses the earlier 60,000-character experience-review formatter cap; this change covers the later 16,000-character per-session history-scan cap.

User Impact

Skill history scans now send well-formed Unicode transcripts at every truncation boundary without increasing their context budget.

Evidence

  • AI-assisted: yes. The truncation branches and downstream history-scan path were reviewed.
  • node scripts/run-vitest.mjs src/skills/workshop/history-scan.test.ts -t 'keeps .* UTF-16 safe' - 3/3 tests passed; 23 skipped.
  • Tests cover the prefix-only budget and retained head and tail boundaries.
  • git diff --check origin/main...HEAD - passed.
  • Focused validation used the local fallback because Blacksmith Testbox was unavailable and sanitized AWS Crabbox could not acquire a lease.
  • Initial CI run 29359504345 completed. Selected build, type, contract, QA smoke, and test lanes passed. The three failing source checks reproduce only current-base debt outside this PR: an unused Bonjour export, a stale QA Lab max-lines baseline entry, and two over-limit system-agent files.

@Leon-SK668

Copy link
Copy Markdown
Contributor Author

Exact-head CI refresh after the ordinary main sync:

  • Head: 1ba1110785b7cf5b482f268caf10fbf7723229c9
  • Synced baseline: 86772735ac1414407e0e88e31dfd8aa0f3396e83 (merge commit; no rebase or force-push)
  • The PR result relative to that baseline remains limited to the original two history-scan files.
  • Focused regression: node scripts/run-vitest.mjs src/skills/workshop/history-scan.test.ts -t 'UTF-16 safe' passed 3/3 tests.
  • Target formatting and git diff --check passed.
  • Exact-head CI run 29378711903 completed successfully with zero pending or failing checks.

This supersedes the initial red CI classification in the PR body: after syncing the upstream Bonjour and max-lines fixes, the same PR patch is fully green.

@Leon-SK668

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

The exact-head CI and proof have been refreshed above. Please review the current head.

@clawsweeper

clawsweeper Bot commented Jul 15, 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 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 15, 2026
@clawsweeper

clawsweeper Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 15, 2026, 10:07 PM ET / July 16, 2026, 02:07 UTC.

Summary
Uses the normalization core’s UTF-16-safe slicing helpers for Skill Workshop’s 16,000-character per-session history-scan cap and adds surrogate-boundary regression coverage.

PR surface: Source +1, Tests +31. Total +32 across 2 files.

Reproducibility: yes. —the raw prefix/head/tail slicing can deterministically split an emoji by positioning its surrogate pair across the configured cap boundary; the supplied production-path run verifies the repaired output, although an unpatched current-main live run was not executed in this review.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/skills/workshop/history-scan.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster ✨ media proof bonus
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] The PR head is mergeable but behind current main, so its focused tests and exact-head CI should be refreshed after synchronizing the latest base before landing.

Maintainer options:

  1. Decide the mitigation before merge
    Land the shared UTF-16 helper implementation after refreshing it against current main, retaining the existing cap, omission marker, redaction order, head/tail layout, and focused boundary tests.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed because the current patch has no actionable finding; it needs ordinary maintainer review and a current-main refresh before landing.

Security
Cleared: The patch adds no dependency, workflow, permission, secret, publishing, installation, or artifact-execution surface and reuses an existing internal normalization helper.

Review details

Best possible solution:

Land the shared UTF-16 helper implementation after refreshing it against current main, retaining the existing cap, omission marker, redaction order, head/tail layout, and focused boundary tests.

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

Yes—the raw prefix/head/tail slicing can deterministically split an emoji by positioning its surrogate pair across the configured cap boundary; the supplied production-path run verifies the repaired output, although an unpatched current-main live run was not executed in this review.

Is this the best way to solve the issue?

Yes—the PR applies the repository’s existing UTF-16-safe normalization helpers at the actual downstream cap instead of adding a parallel truncation implementation, while preserving the established budget and layout.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The linked exact-head workflow demonstrates the repaired production SQLite-to-provider path, including the deliberate surrogate boundary, bounded transcript, omission marker, two live loopback provider requests, and a successful skill_workshop completion round trip without a dangling surrogate.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (linked_artifact): The linked exact-head workflow demonstrates the repaired production SQLite-to-provider path, including the deliberate surrogate boundary, bounded transcript, omission marker, two live loopback provider requests, and a successful skill_workshop completion round trip without a dangling surrogate.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: Malformed provider-bound transcript text is a real bounded Skill Workshop bug with focused scope and no evidence of broad runtime outage or data loss.
  • 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 (linked_artifact): The linked exact-head workflow demonstrates the repaired production SQLite-to-provider path, including the deliberate surrogate boundary, bounded transcript, omission marker, two live loopback provider requests, and a successful skill_workshop completion round trip without a dangling surrogate.
  • proof: sufficient: Contributor real behavior proof is sufficient. The linked exact-head workflow demonstrates the repaired production SQLite-to-provider path, including the deliberate surrogate boundary, bounded transcript, omission marker, two live loopback provider requests, and a successful skill_workshop completion round trip without a dangling surrogate.
Evidence reviewed

PR surface:

Source +1, Tests +31. Total +32 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 4 3 +1
Tests 1 31 0 +31
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 35 3 +32

What I checked:

Likely related people:

  • vincentkoc: Recent merged history connects this contributor to the Skill Workshop surface, making them a plausible reviewer for workshop behavior, although the exact history-scan ownership trail is ambiguous. (role: adjacent Skill Workshop contributor; confidence: low; commits: 077d813b0; files: src/skills/workshop/)
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.
Review history (3 earlier review cycles)
  • reviewed 2026-07-15T07:35:33.558Z sha 1ba1110 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-15T08:51:27.422Z sha 1ba1110 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-15T10:05:02.259Z sha 1ba1110 :: needs real behavior proof before merge. :: none

@Leon-SK668

Copy link
Copy Markdown
Contributor Author

Exact-head real behavior proof is now available for the provider-bound history-scan path.

  • PR head: 1ba1110785b7cf5b482f268caf10fbf7723229c9 (unchanged)
  • Successful run: https://github.com/Leon-SK668/openclaw/actions/runs/29421699683
  • The proof persisted 13 messages in the real SQLite session store, then used the production listHistoryScanCandidates -> readHistoryScanSession -> runSkillHistoryScanReview path. Unrelated bundled/persisted plugin discovery was disabled; the core skill_workshop tool remained active.
  • The deliberately split source boundary was UTF-16 code units d83d/de00 at index 1287. The production transcript reader emitted a 2611-character bounded transcript with the omission marker.
  • The production embedded-agent runtime made two actual local HTTP POST /v1/responses requests. The captured provider-bound prompt was 4485 characters, retained the omission marker, and had danglingSurrogate=false.
  • The model fixture returned the real skill_workshop call { "action": "complete" }; its tool result reached the second provider request (completionRoundTrip=true). One persisted session was reviewed and the completion callback recorded zero proposals, as expected for the neutral fixture.
  • The run used only a loopback server and a dummy local token; no provider credentials or private transcript content were used or printed.

Machine result:

{"proof":"persisted-history-scan-provider-boundary","prHead":"1ba1110785b7cf5b482f268caf10fbf7723229c9","persistedMessages":13,"sourceBoundaryIndex":1287,"sourceBoundaryCodeUnits":["d83d","de00"],"providerRequests":2,"firstRequestPath":"/v1/responses","providerPromptChars":4485,"omissionPresent":true,"danglingSurrogate":false,"completionRoundTrip":true,"candidateCount":1,"reviewedSessions":1,"reviewIdeas":0,"completedIdeas":0}

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. 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 16, 2026
@steipete steipete self-assigned this Jul 16, 2026
@steipete
steipete merged commit 2fd3d86 into openclaw:main Jul 16, 2026
213 of 225 checks passed
@steipete

Copy link
Copy Markdown
Contributor

Merged via squash.

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

Labels

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS 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