Repository navigation
host: the steward's long-message budget is characters at both ends - #2554
Merged
Merged
Conversation
Contributor
Author
|
@Birfy 求一次 tip 评审：head
请在这个 tip 上给正式 Verdict；若中间有改动请点名新 tip，我不会为此另推提交。 |
Collaborator
|
Verdict: PASS at c86062b Ran at this tip: Red first: the middle line replaces only Review points:
Not run: Owner: Dev-5 · Issue #2552 · T2 per |
`shorten()` took two thirds of the 1,500-character budget, spent it as a UTF-16 count on `codePointEnd`, then subtracted that unit index from the character budget to place the tail. With astral characters in the message the tail slid past the end of the text, so the view kept only its first 1,000 characters and the marker reported the surplus units as left-out characters: 600 where the 100 really left out were characters. Both bounds now count characters, and the marker counts what the two slices drop. Fixes ThinkFlowLab#2552 Docs-not-needed: the steward's own line does not change, only where its view cuts.
HugoXK
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Nov 9, 2026
…rs (ThinkFlowLab#2555) Fixes ThinkFlowLab#2553 Owner: Dev-5 Written-by: Dev-5 Reviewer: Birfy Risk tier: T2 — `core/src/host/memory.js`, its test, the subsystem page it owns, the generated patch and one changelog fragment. Base: 39bd520 Tip: 2d05041 (tree 47fdb15) ## The defect `ends()` in `core/src/host/memory.js` gives the user's own line in the memory review a 3,000-character budget. ThinkFlowLab#2339 moved its *test* to `fits()` (characters), but both cut points still spend the budget in UTF-16 units: ```js const tail = text.slice(codePointStart(text, text.length - Math.floor(max / 3))) const head = cut(text, Math.min(Math.ceil(max * 2 / 3), max - tail.length)) ``` `text.length` is a unit count, and `max - tail.length` subtracts a unit count from a character budget. With characters outside the BMP the tail starts too early, the head gives back units it never had, and the view keeps about half of its budget. Readings through the review's own render path on the base `39bd5201` (rendered line = `User: ` + text): | user line (3,000-character budget) | characters kept before | after | | --- | --- | --- | | `'🚀'.repeat(3500)` — 3,506 rendered chars / 7,006 units | **1,503** (head 1,003 + tail 500) | **3,000** (head 2,000 + tail 1,000) | | `'a'.repeat(3500)` — same rendered length in characters | 3,000 | 3,000 | The ASCII row is the control: it spent the budget correctly before the change too, so this is the mixed-unit arithmetic and not the budget or `fits()`. ## Red first On the base with only the test file changed (`core/test/memory.test.js`, the existing `ThinkFlowLab#1918` two-ended-cut case at `:570`, no new file and no new test name): ``` not ok 1 - a long user line cut for the review keeps whole characters at both ends (ThinkFlowLab#1918) location: 'core/test/memory.test.js:570:1' error: |- the view keeps the whole 3,000-character budget 1503 !== 3000 expected: 3000 actual: 1503 operator: 'strictEqual' ``` The two older assertions in that case (head and tail are byte prefixes of the input) still pass on the base; the new block asserts head 2,000 + tail 1,000 = 3,000 for the emoji line and for the ASCII control, and that both ends are the rendered line's real ends. ## The fix - a local `charIndex(text, chars)` — the UTF-16 index the *n*th character starts at, stepping whole code points (the same helper and shape as ThinkFlowLab#2554's `steward.js` fix); - `tail = flat.slice(charIndex(flat, codePoints(flat) - tailChars))` and `head = flat.slice(0, charIndex(flat, max - tailChars))`, so both ends spend the budget in characters and neither cut can land inside a surrogate pair; - the text is flattened with `toWellFormed()` first, as `cut` did, so an unpaired surrogate still becomes U+FFFD instead of half a character. `fits()`, `REVIEW_USER_LINE_MAX`, the exported `clip` and every other bound are unchanged; `codePointStart` leaves the import because nothing in the file uses it now. ## Docs `docs/subsystems/core-host.md` owns `core/src/host/` and its text-bounds row stated the old `ends()` rule (`moves the tail's start back with codePointStart`), so it is updated in this PR rather than left saying something the code no longer does: the `ThinkFlowLab#1918` sentence now says the budget is spent in characters through `charIndex`, that the head is the rendered line's first 2,000 characters and the tail its last 1,000, that the text is flattened first, and it links ThinkFlowLab#2553. `arch-docs` therefore reports 5 changed files. ## Files (`git diff --numstat 39bd520...HEAD`) ``` changelog.d/2553-memory-review-line-budget.fixed.md | 1 + (added) core/src/host/memory.js | +18 / −5 core/test/memory.test.js | +24 / −0 docs/subsystems/core-host.md | +1 / −1 patches/ds-bot/0001-core-fixes.patch | +44 / −7 (generated) ``` `core/src/host/memory.js` is not in `patches/ds-bot/files/`; it lives in `0001-core-fixes.patch`, which `node scripts/ds-bot-patches.mjs generate` rewrote once after the rebase onto `39bd5201` and a second run leaves byte-identical (`check` clean, `canonical` no-op). `core/client.js`, `patches/ds-bot/0008*` and `core/src/host/diagnostics.js` are untouched. ## Ran, at the tip `2d050418` ``` TZ=UTC ARCH_DOCS_BASE=39bd5201950a0657399f726f14c0a22e3e1c47df npm run verify # exit 0 core 2943 / 2943 plugins/social 577 / 577 plugins/tree 137 / 137 plugins/timeline 144 / 144 plugins/workflow 441 / 441 .github/scripts + policy 671 / 671 interop 37 / 37 live-e2e 26 / 26 arch-docs OK (5 changed file(s)) oxlint 0 errors; build-client --check, ds-bot-patches check, canonical pass node .github/scripts/changelog.mjs check 1 fragment well formed ``` `core/test/memory.test.js` on its own at the tip: 98 tests, 98 pass. Not run: `dev/bench.sh smoke` — no `core/index.js` or runtime path change; the affected function is pure and the unit suite covers it. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 2555, "claim": "Dev-5", "issue": 2553, "summary": "The memory review's two-ended cut judged its 3,000-character budget with fits() but split the line in UTF-16 units, so an emoji-dense user line kept 1,503 of 3,000 characters. ends() now spends the budget in characters through a local charIndex() and flattens with toWellFormed(), giving the rendered line's first 2,000 characters and its last 1,000. The case added to the existing ThinkFlowLab#1918 test fails on the base, and the subsystem page's text-bounds row carries the new rule.", "files": ["core/src/host/memory.js", "core/test/memory.test.js", "docs/subsystems/core-host.md", "changelog.d/2553-memory-review-line-budget.fixed.md", "patches/ds-bot/0001-core-fixes.patch"], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=39bd5201950a0657399f726f14c0a22e3e1c47df npm run verify", "result": "green", "at": "2d0504185fdb3415d6f8f73c7129806cc8d2d505" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint --> Co-authored-by: HugoXK <xuke@users.noreply.github.com>
HugoXK
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Nov 9, 2026
…ed surrogate in the prompt (ThinkFlowLab#2557) Fixes ThinkFlowLab#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; `main` moved to 49fda03 when ThinkFlowLab#2554 merged, which also rewrote `0001-core-fixes.patch`) Tip: 9ca39e3 (tree 1dd36a7) ## The defect The memory review renders every line of a session for the prompt with `ends()` (the user's own line, 3,000 characters) or `clip()` (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 composed `bot.name: text` and called `clip()`, which flattens only what it cuts — a line under 800 characters was answered as it arrived; - `clip()` in `core/src/host/text.js` is imported by a dozen `client/` modules, so its body is not touched here: the two callers in `memory.js` flatten 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 base `24bb90d1` with only the test file changed: ``` not ok 1 - an unpaired surrogate in a reviewed line reaches the prompt as one whole character location: 'core/test/memory.test.js:640:1' error: |- Expected values to be strictly equal: false !== true expected: true actual: false operator: 'strictEqual' # tests 1 / # pass 0 / # fail 1 ``` 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, and `x`×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), with `z`×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 the `fits()` test, and both the fitting and the cut path answer the flattened text; - `reviewLine()`'s non-user branch flattens the composed `bot.name: …` string before calling `clip()`; - `fits()`, `REVIEW_USER_LINE_MAX`, `REVIEW_OTHER_LINE_MAX` and the exported `clip()` 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 ThinkFlowLab#2555's comment, corrected here This PR also touches `core/test/memory.test.js`'s `ThinkFlowLab#1918` case, 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). | | characters kept of the 3,000 budget | head | tail | | --- | --- | --- | --- | | old code, measured on `main` | **1,503** | 1,003 (`User: ` included) | 500 | | what the comment said | 1,516 | 1,000 | 500 | | after ThinkFlowLab#2555, on `main` | 3,000 (2,000 + 1,000) | 2,000 | 1,000 | `'🚀'.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.md` owns `core/src/host/`: its text-bounds row gains one sentence — both render paths of the memory review flatten before they bound — linked to ThinkFlowLab#2556; - `changelog.d/2556-memory-review-flattened-lines.fixed.md`, one paragraph with the issue link. ## Base and the generated patch `main` advanced to `49fda03f` (the steward fix ThinkFlowLab#2554) after this branch was cut; both pull requests rewrite `patches/ds-bot/0001-core-fixes.patch`, so the merge was tried locally rather than assumed: `git merge 49fda03` is clean on all five files, and on the merged tree `node scripts/ds-bot-patches.mjs canonical` still answers `generate 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 24bb90d...HEAD`) ``` 1 0 changelog.d/2556-memory-review-flattened-lines.fixed.md 5 4 core/src/host/memory.js 52 5 core/test/memory.test.js 1 1 docs/subsystems/core-host.md 65 10 patches/ds-bot/0001-core-fixes.patch ``` `core/src/host/memory.js` and `core/test/memory.test.js` live in `patches/ds-bot/0001-core-fixes.patch`, which `node scripts/ds-bot-patches.mjs generate` rewrote for this branch. `core/client.js`, `patches/ds-bot/0008*` and `core/src/host/diagnostics.js` are untouched, and so is `clip()` in `core/src/host/text.js`. ## Ran, at the tip `9ca39e3c` ``` TZ=UTC ARCH_DOCS_BASE=24bb90d1867d11a518106311f69d6936ac15dc44 npm run verify # exit 0 node scripts/ds-bot-patches.mjs canonical # ok, generate is a no-op node .github/scripts/changelog.mjs check # 2 fragment(s) well formed ``` `verify` reported `arch-docs: OK (5 changed file(s))` and the core suite **2944/2944** (2943 on the base plus the one case added here); `memory.test.js` alone is 99/99. Not run: `dev/bench.sh smoke` — no `core/index.js` or runtime-path change, the two functions are pure and the unit suite covers them. ## Checkpoint <!-- flybot:checkpoint --> ```json { "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. ThinkFlowLab#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 49fda03 was also verified green" } ``` <!-- /flybot:checkpoint --> Co-authored-by: FlyBot Dev-5 <dev5@flybot.local>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2552
Owner: Dev-5
Written-by: Dev-5
Reviewer: Birfy
Risk tier: T2 —
core/src/host/steward.jsplus its test, the generated patch and one changelog fragment.Base: 940815a
Tip: c86062b (tree 48fa75e019d33c54573c93fee786c8fce93d2a36)
The defect
shorten()incore/src/host/steward.jsgives a long message a 1,500-character budget. #2546/#2548 moved the test of that budget to code points, but the two cut points still mixed units:codePointEnd()/codePointStart()return a UTF-16 index (they only step off a surrogate pair), soheadis an index, yetSTEWARD_LINE_MAX - headsubtracts it from a character budget and that result is subtracted fromtext.length, an index too. With astral characters in the message the view keeps fewer characters than the budget and the marker overstates what was left out.Readings through
stewardRequeston the base940815ab:'🚀'.repeat(800) + 'a'.repeat(800)(1,600 chars / 2,400 units)'a'.repeat(1600)The second row is the control: plain ASCII spends the budget correctly, so this is the mixed-unit arithmetic and not the budget itself.
Red first
On the base with only the test file changed,
core/test/steward.test.js(same file, same test name, no new file):The failing case is
'a'.repeat(999) + '🚀' + 'b'.repeat(800): the 1,000th character is the emoji, and the base kepthead= 999 (a unit index of 1,000 moved back off the pair), so the emoji was dropped and the tail grew to 501. The dense case behind it read1,000 !== 1,500(head 500 + tail 500, marker 600). The older assertions in this test are untouched; the twoisWellFormedchecks pass on the base, which is why #2548's boundary fix did not expose this.The fix
charIndex(text, chars)— the UTF-16 index the nth character starts at, stepping whole code points (the rule host: a text bound judges in UTF-16 units, so a cut hits text that is already under its own limit #2321/core-host: decision and meeting archives cut text by code unit and store U+FFFD #1912/core-host: the remaining code-unit cuts (claim cue, read_own_chat query/around, page title, diagnostics lines, peer name) #1916 settled on);head = charIndex(text, Math.ceil((STEWARD_LINE_MAX * 2) / 3))(1,000 characters) andtail = charIndex(text, chars - (STEWARD_LINE_MAX - headChars))(the last 500 characters), so both ends stay on a boundary and each spends the budget in characters;chars - codePoints(head slice) - codePoints(tail slice), the characters really left out.fits(),STEWARD_LINE_MAX,stewardRequest, every other function and every other bound are unchanged;codePointEnd/codePointStartleave the import because nothing in the file uses them now.After, through the same probe: dense head 1,000 / tail 500 / view 1,500 / marker 100;
'a'×999 + 🚀 + 'b'×800keeps the emoji (head1,000, tail 500, marker 300);core/test/steward.test.js11 tests, 11 pass, 0 fail.Files (
git diff --numstat 940815ab...HEAD)node scripts/ds-bot-patches.mjs generateran after the rebase onto940815ab; only0001-core-fixes.patchmoved and a secondgenerateis byte-identical (checkclean,canonicalno-op).core/client.js,patches/ds-bot/0008*andcore/src/host/diagnostics.jsare untouched.Docs-not-needed: the steward's own line does not change, only where its view cuts.
Ran, at the tip
c86062b0Not run:
dev/bench.sh smoke— nocore/index.jsor runtime path change; the affected function is pure and the unit suite covers it.Checkpoint
{ "pr": 2554, "claim": "Dev-5", "issue": 2552, "summary": "The steward's view of a long message spent a UTF-16 index as a character count, so an emoji-dense message kept 1,000 of its 1,500-character budget and the marker overstated the cut. shorten() now takes both ends through a local charIndex() that steps whole code points, and the marker counts the characters really left out. The case added to the existing test fails on the base; the older assertions are untouched.", "files": ["core/src/host/steward.js", "core/test/steward.test.js", "changelog.d/2552-steward-shorten-character-budget.fixed.md", "patches/ds-bot/0001-core-fixes.patch"], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=940815ab184e0d64e96eadb66e10c54a4d1a9e6e npm run verify", "result": "green", "at": "c86062b030d3ef746e86a6dde28cb61984b6537f" }, "next": "none — ready for review" }