Skip to content

fix(active-memory): keep timeout partial replies valid at emoji caps#102940

Closed
zhangguiping-xydt wants to merge 2 commits into
openclaw:mainfrom
zhangguiping-xydt:fix/problem-active-memory-partial-transcript-surrogate
Closed

fix(active-memory): keep timeout partial replies valid at emoji caps#102940
zhangguiping-xydt wants to merge 2 commits into
openclaw:mainfrom
zhangguiping-xydt:fix/problem-active-memory-partial-transcript-surrogate

Conversation

@zhangguiping-xydt

@zhangguiping-xydt zhangguiping-xydt commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where active-memory timeout recovery could attach a partial assistant transcript containing a broken UTF-16 surrogate when the transcript cap lands inside an emoji or other surrogate-pair character.

Why This Change Was Made

The partial transcript reader now applies the same UTF-16-safe truncation used by other active-memory text caps before storing or returning bounded assistant text. The fix stays limited to partial assistant transcript collection and does not change transcript parsing, memory search behavior, or summary generation.

User Impact

Users who hit active-memory timeout recovery can expect partial assistant replies to remain valid text instead of exposing replacement characters or malformed emoji boundaries in the recovered timeout context.

Evidence

  • Confirmed current origin/main still used raw slice(0, remaining) and final joined slice(0, resolvedLimits.maxChars) in readPartialAssistantText; fix(active-memory): recall context breaks when recent turns contain emoji #102877 covers the recent-turn recall query path, not the partial assistant transcript timeout recovery path.

  • Checked fix(active-memory): recall context breaks when recent turns contain emoji #102877 after maintainer commits: it still only changes the recent-turn recentUserChars / recentAssistantChars truncation path, while this PR changes readPartialAssistantText.

  • Issue-specific runtime proof: the active-memory extension shard exercises the before_prompt_build timeout recovery flow that reads partial assistant transcript JSONL, returns timeout_partial, and verifies recovered partial text is surfaced through the active-memory prompt/status path. The same shard includes the new surrogate-boundary regression for the partial assistant transcript cap.

  • Focused validation on the current PR head:

    $ node scripts/test-projects.mjs extensions/active-memory/index.test.ts -- --maxWorkers=1
[test] starting test/vitest/vitest.extension-active-memory.config.ts
The `envFile` option is deprecated, please use `envDir: false` instead.

 RUN  v4.1.9 [local path redacted]

[vitest] still running with no output for 30000ms (test/vitest/vitest.extension-active-memory.config.ts).
[vitest] still running with no output for 60000ms (test/vitest/vitest.extension-active-memory.config.ts).

 Test Files  1 passed (1)
      Tests  159 passed (159)
   Start at  23:43:38
   Duration  60.67s (transform 9.58s, setup 531ms, import 16.77s, tests 42.98s, environment 0ms)

[test] passed 1 Vitest shard in 73.20s

  • CI note: the remaining repeated failure is checks-node-compact-small-whole-2 timing out in src/tui/tui-pty-local.e2e.test.ts > creates and adopts a fresh session through the real Gateway backend. That TUI PTY test config disables plugins/memory (plugins.enabled=false, slots.memory="none"), and the same focused test passed locally on this PR head, so I did not change active-memory code for that unrelated CI timeout.

@clawsweeper

clawsweeper Bot commented Jul 9, 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.

This PR can close as superseded because the same timeout-partial UTF-16 fix is already included in the broader canonical active-memory fix at #102877, which also covers recent recall snippets, multi-record timeout boundaries, and stronger proof.

Root-cause cluster
Relationship: superseded
Canonical: #102877
Summary: The broader active-memory UTF-16 truncation PR is the canonical landing path because it includes this timeout-partial work plus adjacent recent-recall coverage and stronger proof.

Members:

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

Canonical path: Use #102877 as the single landing path for the active-memory UTF-16 truncation fix, then close the timeout-only duplicates.

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

Review details

Best possible solution:

Use #102877 as the single landing path for the active-memory UTF-16 truncation fix, then close the timeout-only duplicates.

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

Yes. Current main and the latest release both show raw UTF-16 code-unit slicing in the timeout partial transcript path, and the canonical PR includes red/green proof for this exact failure mode.

Is this the best way to solve the issue?

No. This timeout-only branch is not the best landing path because #102877 includes the same fix plus adjacent recent-recall coverage, multi-record timeout coverage, and stop-on-backoff behavior.

Security review:

Security review cleared: Cleared: the diff only changes active-memory string truncation and tests, with no dependency, workflow, credential, package, or code-execution surface added.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md and extensions/AGENTS.md were read fully; the review applied the extension-boundary and whole-path PR review guidance. (AGENTS.md:1, 4f9a371c9530)
  • Current main timeout path still has raw UTF-16 slicing: Current main uses raw slice calls in readPartialAssistantText, so the timeout-partial bug surface is real but not yet solved on main. (extensions/active-memory/index.ts:2254, 4f9a371c9530)
  • Current PR scope: The current PR only changes readPartialAssistantText and adds one single-record timeout transcript surrogate-boundary test. (extensions/active-memory/index.ts:2254, f68679ca36a7)
  • Canonical PR covers this work with broader behavior: The canonical PR includes the timeout partial transcript fix, adds a joined multi-record regression, stops on UTF-16-safe truncation backoff, and also fixes recent user/assistant recall snippet truncation. (extensions/active-memory/index.ts:2251, c0d70155b316)
  • Canonical PR is viable and proof-positive: The canonical PR is open, mergeable, labeled proof sufficient, assigned for maintainer review, and its comments include linked Crabbox RED/GREEN proof plus exact-head CI evidence. (c0d70155b316)
  • Related search found the duplicate cluster: Searches for the timeout-partial surrogate problem found this PR, the canonical broader PR, and the earlier closed timeout-only duplicate.

Likely related people:

  • Tak Hoffman: Full file history shows the Active Memory recall plugin and its central tests were introduced in the merged feature commit. (role: introduced behavior and feature owner; confidence: high; commits: b83726d13e33; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)
  • wangyan2026: Current-main blame for readPartialAssistantText and buildQuery points to the current implementation commit that carried these truncation paths. (role: recent area contributor; confidence: medium; commits: 2e9e46bb3794; files: extensions/active-memory/index.ts)
  • steipete: The canonical PR is assigned to steipete and includes steipete-authored follow-up commits and proof on the same active-memory UTF-16 truncation path. (role: recent follow-up owner and reviewer; confidence: high; commits: 2ebf0d61eff6, 98ae0ec3b337, c0d70155b316; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)

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

@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. labels Jul 9, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
@zhangguiping-xydt
zhangguiping-xydt force-pushed the fix/problem-active-memory-partial-transcript-surrogate branch from 9030a7e to f68679c Compare July 9, 2026 15:54
@clawsweeper clawsweeper Bot added the P2 Normal backlog priority with limited blast radius. label Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks @zhangguiping-xydt. Recording the final landed proof on this already-closed resubmission.

The live final head is f68679ca. Its +28/-7 two-file diff is exactly identical to #102920, with the same blobs for both changed files, so no unique code or regression remains here.

The canonical fold, #102877, landed on current main as f1e7081b. It includes this PR's single-record timeout regression and the stronger bounded-prefix implementation: UTF-16-safe per-record truncation, stop-on-truncation or surrogate backoff, exact separator accounting, and an added multi-record boundary regression. It also fixes the sibling recent-conversation recall path.

Your timeout-path work remains credited in folded source commit c0d70155 and in the landed commit. Full red/green and exact-head proof is recorded in the land-ready evidence. Thank you.

@steipete steipete mentioned this pull request Jul 9, 2026
Closed
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

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS 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.

2 participants

@zhangguiping-xydt @steipete