fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice by Simon-XYDT · Pull Request #100014 · openclaw/openclaw

Simon-XYDT · 2026-07-04T16:14:45Z

Summary

sliceUtf16Safe silently swaps reversed slice bounds instead of returning "" like String.prototype.slice, creating a subtle logic-corruption footgun for callers that compute dynamic start/end pairs. Removed the swap branch and made to <= from return "" to match native .slice semantics.

Problem: sliceUtf16Safe("hello", 4, 1) returns "ell" while "hello".slice(4, 1) returns ""
Solution: Replace the silent swap with an early return "" when normalized end <= start
What changed: src/shared/utf16-slice.ts (remove swap, add early return), src/shared/utf16-slice.test.ts (update reversed-bounds expectation)
What did NOT change: All other behavior (surrogate-pair safety, negative index handling, truncation); no caller-side changes needed

Change Type (select all)

Scope (select all)

Linked Issue/PR

Fixes Unexpected behavior: sliceUtf16Safe swaps bounds when end < start instead of returning "" like String.prototype.slice #99980
This PR fixes a bug or regression

Motivation

sliceUtf16Safe is documented as a surrogate-safe drop-in substitute for String.prototype.slice and is widely re-exported through src/utils.ts and plugin SDK surfaces. When end < start after normalization it swaps the bounds and returns a non-empty substring, whereas native .slice returns "". This violates the documented drop-in contract and can cause subtle data corruption for any caller that computes dynamic start/end pairs where the end can land before the start.

Real behavior proof

Behavior addressed: sliceUtf16Safe now returns "" for reversed normalized bounds, matching String.prototype.slice
Real environment tested: Linux, Node 22, branch fix/utf16-slice-reversed-bounds-99980
Exact steps or command run after this patch:

node --experimental-strip-types -e "const { sliceUtf16Safe } = await import('./src/shared/utf16-slice.ts'); console.log(JSON.stringify({safe: sliceUtf16Safe('hello', 4, 1), native: 'hello'.slice(4, 1)}));"
pnpm test src/shared/utf16-slice.test.ts

Evidence after fix:

$ node --experimental-strip-types -e "const { sliceUtf16Safe } = await import('./src/shared/utf16-slice.ts'); console.log(JSON.stringify({safe: sliceUtf16Safe('hello', 4, 1), native: 'hello'.slice(4, 1)}));"
{"safe":"","native":""}

$ pnpm test src/shared/utf16-slice.test.ts
 RUN  v4.1.8

 ✓ src/shared/utf16-slice.test.ts (1)
   Tests  18 passed (18)

 Test Files  1 passed (1)

Observed result after fix:
1. sliceUtf16Safe("hello", 4, 1) now returns "", matching native .slice
2. All 18 existing tests continue to pass, including surrogate-pair safety
What was not tested: No live channel or E2E test — the fix is a pure helper-level behavior correction

Root Cause (if applicable)

Original implementation at src/shared/utf16-slice.ts:22 used a silent swap (if (to < from) { const tmp = from; from = to; to = tmp; }) instead of returning an empty string. This was inconsistent with String.prototype.slice semantics and created a footgun for callers with computed bounds.

Regression Test Plan (if applicable)

N/A — existing 18-test suite covers all surrogate-safe edge cases. The only expectation change is the reversed-bounds test.

User-visible / Behavior Changes

None. No production caller passes reversed normalized bounds (end < start). All callers use (text, 0, N) or (text, -N) forms.

Security Impact

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No

Human Verification (required)

Verified scenarios: ASCII slice with reversed bounds, normal forward slice, truncation
Edge cases checked: surrogates with forward bounds, empty string, negative indices, end beyond length
What you did NOT verify: Live channel E2E — not applicable for a shared helper

Compatibility / Migration

Backward compatible? Yes — no production caller relies on the swapped-bounds behavior
Config/env changes? No
Migration needed? No

Best-fix Verdict

Best fix: Yes. The narrowest correct fix is removing the swap branch and returning "" when to <= from, which is exactly how native String.prototype.slice works.
Refactor needed: No. The helper is already clean; this just aligns one behavior with its documented contract.
Alternative considered: Keeping the swap but adding a deprecation warning — unnecessary since no production caller depends on the swapped behavior and no shipped contract requires it.

AI Assistance 🤖

AI-assisted: Yes
AI model: deepseek-v4-flash
Human confirmed understanding of code changes: Yes
AI prompts / session excerpts: Issue analysis and fix based on reported bug at Unexpected behavior: sliceUtf16Safe swaps bounds when end < start instead of returning "" like String.prototype.slice #99980 with source inspection and full caller scan

Risks and Mitigations

Highest-risk area: None. Caller scan confirmed all 30+ production callers use (text, 0, N) or (text, -N) forms only; no caller passes reversed bounds.
Mitigation: Full caller grep across src/, extensions/, and packages/ verified no production code relies on the old swap behavior.
Compatibility impact: None.

Fixes #99980

clawsweeper · 2026-07-04T16:16:05Z

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 7:58 AM ET / 11:58 UTC.

Summary
The PR changes sliceUtf16Safe so normalized reversed or equal bounds return an empty string and updates the colocated regression test.

PR surface: Source -2, Tests 0. Total -2 across 2 files.

Reproducibility: yes. A direct current-main Node import reproduced sliceUtf16Safe('hello', 4, 1) returning ell while native String.prototype.slice returns an empty string.

Review metrics: 1 noteworthy metric.

Public helper behavior surface: 1 behavior changed; 2 plugin SDK text utility re-export paths affected. The changed helper is visible outside the package, so maintainers should notice the compatibility decision before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99980
Summary: This PR is the active fix candidate for the canonical open issue about reversed sliceUtf16Safe bounds; the older merged test PR is relevant provenance but not remaining work.

Members:

canonical: Unexpected behavior: sliceUtf16Safe swaps bounds when end < start instead of returning "" like String.prototype.slice #99980 - The PR body uses closing syntax for this open issue, and the issue describes the same reversed-bounds helper behavior.
partial_overlap: test(shared): add unit tests for surrogate-safe UTF-16 string slicing helpers #97805 - That merged PR added helper tests, including the swapped-bounds expectation, but it does not track the remaining fix.

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

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

[P2] Get maintainer acceptance of the exported helper semantics change before merge.

Risk before merge

[P1] sliceUtf16Safe is exported through normalization-core and plugin SDK text utility surfaces, so external plugin callers that intentionally depended on swapped reversed bounds would observe changed output even though the in-repo caller scan does not show that dependency.

Maintainer options:

Accept native slice semantics (recommended)
Maintainers can accept that the old swapped output was a bug and land the narrow helper/test change as-is.
Add SDK-facing contract text
Before merge, clarify in helper or SDK-facing documentation that reversed normalized bounds return an empty string.
Pause for external compatibility
If external plugin compatibility is a concern, pause this PR and decide whether a separate helper or deprecation path is needed.

Next step before merge

No automated repair is needed; the remaining action is maintainer acceptance of the exported helper behavior change before merge.

Maintainer decision needed

Question: Should sliceUtf16Safe, as exported through normalization-core and plugin SDK text utility surfaces, change reversed normalized bounds from swapped substring output to native slice empty-string output?
Rationale: The source and proof support the fix, but the shipped helper is externally visible and external plugin reliance on the old swapped behavior cannot be ruled out from in-repo tests alone.
Likely owner: steipete — This person introduced the helper behavior and recently moved the helper into the normalization-core package surface.
Options:
- Accept Native Slice Semantics (recommended): Land this PR with the updated regression test and treat the old swapped behavior as a helper bug rather than a supported public contract.
- Document Contract First: Add a small SDK-facing note or helper comment clarifying reversed-bound behavior before merge so external callers have a visible contract.
- Pause For Compatibility Path: Hold the PR if maintainers want a deprecation or alternate helper path for any external caller that may rely on swapped bounds.

Security
Cleared: The diff only changes a pure string helper and its unit test, with no dependency, workflow, secret, package, network, or command-execution surface changes.

Review details

Best possible solution:

Land the narrow helper and test change after maintainer acceptance that this exported helper should follow native String.prototype.slice ordering semantics for reversed bounds.

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

Yes. A direct current-main Node import reproduced sliceUtf16Safe('hello', 4, 1) returning ell while native String.prototype.slice returns an empty string.

Is this the best way to solve the issue?

Yes, with maintainer compatibility sign-off. Returning before surrogate-boundary adjustment when to <= from is the narrowest fix that matches native slice ordering while preserving forward and tail slice behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

P2: The PR addresses a focused shared-helper correctness bug with plausible downstream logic impact but no evidence of an active outage.
merge-risk: 🚨 compatibility: The PR changes behavior for a helper exposed through public plugin SDK text utility surfaces, so external plugin compatibility is not fully settled by green tests.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster 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 after-fix terminal output showing sliceUtf16Safe and native slice both returning an empty string, plus the focused helper test passing.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output showing sliceUtf16Safe and native slice both returning an empty string, plus the focused helper test passing.

Evidence reviewed

PR surface:

Source -2, Tests 0. Total -2 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	2	4	-2
Tests	1	2	2	0
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	4	6	-2

What I checked:

Root policy read: Root AGENTS.md was read fully and applied to the review depth, best-fix, proof, and compatibility checks for this exported helper change. (AGENTS.md:15, 0fc29969d960)
Current main behavior: Current main normalizes start/end and swaps them when to < from, producing a non-empty substring for reversed bounds. (packages/normalization-core/src/utf16-slice.ts:22, 0fc29969d960)
Current main reproduction: A direct Node import on current main returned {"safe":"ell","native":"","equal":""} for sliceUtf16Safe('hello',4,1) compared with native slice. (packages/normalization-core/src/utf16-slice.ts:16, 0fc29969d960)
PR implementation: The PR replaces the swap branch with an early empty-string return for to <= from. (packages/normalization-core/src/utf16-slice.ts:22, dcd458949fc4)
Regression test update: The PR updates the reversed-bounds test to expect an empty string, matching the proposed native-slice ordering contract. (packages/normalization-core/src/utf16-slice.test.ts:26, dcd458949fc4)
Exported helper surface: The helper is exported from @openclaw/normalization-core and re-exported through the plugin SDK text utility runtime, so the behavior change is externally visible. (packages/normalization-core/package.json:47, 0fc29969d960)

Likely related people:

steipete: Commit 63f5fa4 introduced the UTF-16 slicing helper with the reversed-bound swap, and commit 009b00f carried the helper into packages/normalization-core. (role: introduced behavior and recent package mover; confidence: high; commits: 63f5fa47deb6, 009b00f7bad1; files: src/utils.ts, packages/normalization-core/src/utf16-slice.ts)
dwc1997: Commit f92ec2d added the helper test file, including the swapped-bounds expectation changed by this PR. (role: test coverage contributor; confidence: high; commits: f92ec2d4e88b; files: src/shared/utf16-slice.test.ts, packages/normalization-core/src/utf16-slice.test.ts)
vincentkoc: GitHub metadata shows this person merged the helper test coverage PR, and the latest release commit also carries the shipped helper surface. (role: recent merger and release-area signal; confidence: medium; commits: f92ec2d4e88b, e085fa1a3ffd; files: src/shared/utf16-slice.test.ts, src/utils.ts, src/plugin-sdk/text-utility-runtime.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.

Review history (2 earlier review cycles)

reviewed 2026-07-04T16:22:22.554Z sha 65849dd :: needs maintainer review before merge. :: none
reviewed 2026-07-04T16:29:08.318Z sha 65849dd :: needs maintainer review before merge. :: none

…g native .slice sliceUtf16Safe silently swapped reversed bounds (to < from) instead of returning "" like String.prototype.slice, creating a subtle footgun for callers with dynamic start/end pairs. Caller scan across src/, extensions/, and packages/ confirmed no production code relies on the old swap behavior — all callers use (text, 0, N) or (text, -N) forms only. Co-Authored-By: Claude Opus 4.6 <[email protected]>

steipete · 2026-07-05T18:13:45Z

Thank you — this fix landed through maintainer batch PR #100399 in commit b22c36f, with contributor credit and changelog thanks preserved. Closing this source PR as superseded.

openclaw-barnacle Bot added the size: XS label Jul 4, 2026

Simon-XYDT force-pushed the fix/utf16-slice-reversed-bounds-99980 branch from 65849dd to dcd4589 Compare July 5, 2026 11:30

steipete self-assigned this Jul 5, 2026

This was referenced Jul 5, 2026

fix: land ten small reliability fixes #100383

Closed

fix: land ten small reliability fixes #100395

Closed

fix: land ten small reliability fixes #100399

Merged

steipete closed this Jul 5, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice#100014

fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice#100014
Simon-XYDT wants to merge 1 commit into
openclaw:mainfrom
Simon-XYDT:fix/utf16-slice-reversed-bounds-99980

Simon-XYDT commented Jul 4, 2026

Uh oh!

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

Simon-XYDT commented Jul 4, 2026

Summary

Change Type (select all)

Scope (select all)

Linked Issue/PR

Motivation

Real behavior proof

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Security Impact

Human Verification (required)

Compatibility / Migration

Best-fix Verdict

AI Assistance 🤖

Risks and Mitigations

Uh oh!

clawsweeper Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading