Repository navigation
host: the memory review's short user line and Bot line keep an unpaired surrogate in the prompt - #2557
Conversation
|
@Birfy 求一次 tip 评审:head
|
`ends()` answered a line that fitted as it arrived, and `reviewLine`'s non-user branch handed its labelled line to `clip` without flattening, so a message carrying an unpaired surrogate reached the review prompt as half a pair. Both paths flatten with `toWellFormed()` first, so the prompt holds U+FFFD in the half pair's place, at the same index and with the same length. The test extends the review-context cases with a user line and a Bot line whose surrogate falls inside the kept text, and its comment now states the 1,503 characters the old unit arithmetic really kept (head 1,003, `User: ` included, and a 500-character tail). Fixes ThinkFlowLab#2556
|
Verdict: PASS at 9ca39e3 Ran at this tip: Red first — 1 red, and it is the new case. With only Scope — exactly the 5 stated files, confirmed by a full tree diff against base The fix — Tip checks — all terminal. Not run here — |
Fixes #2556
Owner: Dev-5
Written-by: Dev-5
Reviewer: Birfy
Risk tier: T2 —
core/src/host/memory.js, its test, the subsystem page that owns it, the generated patch and one changelog fragment.Base: 24bb90d (the branch's base;
mainmoved to 49fda03 when #2554 merged, which also rewrote0001-core-fixes.patch)Tip: 9ca39e3 (tree 1dd36a7c38c205d27b51aca9c2cf4eb25c259d69)
The defect
The memory review renders every line of a session for the prompt with
ends()(the user's own line, 3,000 characters) orclip()(every other line, 800). Both used to hand an unpaired surrogate straight through:ends()answered a line that already fitted unchanged (if (fits(text, max)) return text), so no flattening happened on the path almost every line takes;reviewLine()'s non-user branch composedbot.name: textand calledclip(), which flattens only what it cuts — a line under 800 characters was answered as it arrived;clip()incore/src/host/text.jsis imported by a dozenclient/modules, so its body is not touched here: the two callers inmemory.jsflatten instead.A message that arrives carrying half of a surrogate pair — a stream that ended mid-character, a hand-written fixture — therefore reached the review prompt, and the model, as a lone
U+D800.Red first
One new case in the existing
core/test/memory.test.js(test/an unpaired surrogate in a reviewed line reaches the prompt as one whole character), run against the base24bb90d1with only the test file changed:The failing assertion is the user line's
isWellFormed(). Both carriers were read on that base before the fix: user line{wellFormed:false, replacements:0, cut:false}, Bot line{wellFormed:false, replacements:0, cut:false},promptWellFormed:false. Both surrogate pairs sit inside the kept text (a user line of 201 characters with the half pair at 100, a Bot line of 160 with it at 80), so neither sample goes green just because a cut dropped it.After the fix the same case asserts: the user line is well formed, holds exactly one U+FFFD at
'User: '.length + 100= 106, andx×100 /y×100 are untouched on either side of it; the Bot line is well formed, holds one U+FFFD at the label's real length + 80 (Writer:, index 88), withz×80 /w×80 untouched; the whole prompt is well formed. No assertion says "no U+FFFD" here — flattening is exactly what puts one there, and the existing cases that assert its absence only use well-formed input.The fix
ends()flattens once, before thefits()test, and both the fitting and the cut path answer the flattened text;reviewLine()'s non-user branch flattens the composedbot.name: …string before callingclip();fits(),REVIEW_USER_LINE_MAX,REVIEW_OTHER_LINE_MAXand the exportedclip()are unchanged. U+FFFD is one UTF-16 unit for the half pair it replaces, so no length, index or budget moves.The wrong line in #2555's comment, corrected here
This PR also touches
core/test/memory.test.js's#1918case, so the comment there is corrected in the same place instead of in an issue of its own (no separate issue was filed for it).mainUser:included)main'🚀'.repeat(3500)renders to 3,016 characters (User:+ 3,500 emoji); the old unit arithmetic kept the rendered line's first 2,000 UTF-16 units (the 6-unit label plus 997 emoji = 1,003 characters) and its last 1,000 units (500 characters) — 1,503, not 1,516, and not a 1,000-character head. The comment and the assertions were always right about the fixed code (head 2,000 / tail 1,000); only the "before" reading was wrong.Docs and changelog
docs/subsystems/core-host.mdownscore/src/host/: its text-bounds row gains one sentence — both render paths of the memory review flatten before they bound — linked to host: the memory review's short user line and Bot line keep an unpaired surrogate in the prompt #2556;changelog.d/2556-memory-review-flattened-lines.fixed.md, one paragraph with the issue link.Base and the generated patch
mainadvanced to49fda03f(the steward fix #2554) after this branch was cut; both pull requests rewritepatches/ds-bot/0001-core-fixes.patch, so the merge was tried locally rather than assumed:git merge 49fda03fis clean on all five files, and on the merged treenode scripts/ds-bot-patches.mjs canonicalstill answersgenerate is a no-op— the two hunks land in different file sections of the patch. No rebase, because none is needed and a rebase would move the head that was verified.Files (
git diff --numstat 24bb90d1...HEAD)core/src/host/memory.jsandcore/test/memory.test.jslive inpatches/ds-bot/0001-core-fixes.patch, whichnode scripts/ds-bot-patches.mjs generaterewrote for this branch.core/client.js,patches/ds-bot/0008*andcore/src/host/diagnostics.jsare untouched, and so isclip()incore/src/host/text.js.Ran, at the tip
9ca39e3cverifyreportedarch-docs: OK (5 changed file(s))and the core suite 2944/2944 (2943 on the base plus the one case added here);memory.test.jsalone is 99/99. Not run:dev/bench.sh smoke— nocore/index.jsor runtime-path change, the two functions are pure and the unit suite covers them.Checkpoint
{ "pr": 2557, "claim": "Dev-5", "issue": 2556, "summary": "ends() answered a reviewed line that fitted untouched, and reviewLine() gave its labelled Bot line to clip() unflattened, so a message carrying half a surrogate pair reached the memory review prompt as a lone U+D800. Both paths flatten with toWellFormed() first, so the prompt holds one U+FFFD at the same index and length; clip() in host/text.js is untouched. The new memory.test.js case fails on the base with false !== true. #2555's comment is corrected to the measured 1,503 characters (head 1,003 with the label, tail 500).", "files": [ "core/src/host/memory.js", "core/test/memory.test.js", "docs/subsystems/core-host.md", "patches/ds-bot/0001-core-fixes.patch", "changelog.d/2556-memory-review-flattened-lines.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=24bb90d1867d11a518106311f69d6936ac15dc44 npm run verify", "result": "green", "at": "9ca39e3c2b3ba7edcdbccfcc301e4e09b41473a3" }, "next": "none \u2014 ready for review; the merged tree with 49fda03f was also verified green" }