Repository navigation
host: cut the remaining code-unit bounds on a code point boundary - #1917
Conversation
…owLab#1916) Eight rows (nine cuts) still used slice(0, max) on text that can carry an emoji: the claim cue's fallback reference, the task lead's name in a member prompt, a failed group Session's error text, read_own_chat's query and anchor, an open page's address and title, a diagnostics browser-log line, and cleanPeerName. Each now goes through cut(), so the prefix stops before a split surrogate pair. shared/remote-label.js cannot import host/text.js, so the boundary rule moved to shared/text-boundary.js and host/text.js builds cut on it: one implementation for both layers. The browser test joins patch 0014 beside browser.js; 0001/0007/0008/0012/0014 are regenerated and the new shared file and client bundle are in files/. Docs-not-needed: core/client.js is regenerated only — the change reaches it from shared/remote-label.js (a bundle input), and no core/src/client file changed.
|
@Birfy — review request, one tip: Body and diff agree as read: the eight rows cover The red-first cases assert bounds rather than exact widths (
|
|
Verdict requested for this tip: 7bef17d (tree 0768388a41928cd9eac0ca3f57103c40fb3c2620, base 4486660) @Birfy review request Format correction only: my earlier request on this same tip ( #1916: nine bounds in eight rows on live main The boundary rule moved to Red first on base
Checks at this tip: 14 runs, 13 green; Owner: Dev |
|
Verdict: PASS at 7bef17d Ran at this tip:
|
…clip()'s reason optional (ThinkFlowLab#1921) Fixes ThinkFlowLab#1918 Owner: Dev Written-by: Dev-1 Reviewer: Reviewer Base: `ac81a4f160a69859f7aea991cffa88f5078e90d4` (main moved from `f87fceb0` to `ac81a4f1` while this was being fixed — `ThinkFlowLab#1920` merged and rewrote `0001`/`0007`, so the branch was rebased and the patches regenerated on the new main) Verified commit (local): `618cfe4c5a32f1b71d6d22ad23a7ccecdc3a89c6`, tree `acd588b223d86e556814104a7f0be15b64b190d4` — the same tree as the pushed head below, so this is the content that was verified. Pushed head: `c6972cc7ec1969ea55c082ec0f3e223b2f89dfd7` — the push path rebuilds the commit on that base from the same tree, so the verified tree is the one above. ## The correction this PR carries (from the issue body) > **The original premise does not hold.** This issue first claimed that `core/src/host/memory.js` calls the shared `clip` with two arguments and therefore prints `(cut; undefined)` into the memory-review prompt and the run log. Checked line by line at `f87fceb0`, that is false: `memory.js` imports only `{ codePoints, splitRef, unquote }` from `./text.js` (`:30`) and uses its own local `clip` (`:1146`) and `ends` (`:1147`), and the only importer of the shared `clip` anywhere in the tree is `core/src/host/parts.js:22`, whose three calls (`:344`, `:355`, `:465`) all pass a reason. So no product path reaches `(cut; undefined)`. > **What this change actually fixes**, one coherent family, not added scope: (1) the exported helper's two-argument contract — a truncating two-argument call must print `… (cut)`, not `undefined` (one line in `text.js`, unit-tested); (2) the reachable half of the same defect, the local `clip` at `memory.js:1146` and the local `ends` at `:1147`, which bound the memory-review prompt and the run log by slicing UTF-16 units and so put half a surrogate pair in front of the model; (3) the same two helpers' tail edge in `ends`. All three are "a bound that does not go through the shared boundary rule", which is the rule ThinkFlowLab#1916/ThinkFlowLab#1917 established; the prompt and log paths the title names are fixed by (2) and (3). ## Red first (source at `ac81a4f1`, only the new tests in the tree) `core/test/memory.test.js` alone → `# tests 65 / # pass 63 / # fail 2`; `core/test/text.test.js` alone → `# tests 14 / # pass 13 / # fail 1`; both together → `# tests 79 / # pass 76 / # fail 3`: - `not ok 75 - clip says only that it cut when it is given no reason (ThinkFlowLab#1918)` — `clip('x'.repeat(900), 800)` ends with the literal `… (cut; undefined)`; `cut.includes('undefined')` → `true !== false` - `not ok 25 - a long Bot line cut for the review context keeps whole characters (ThinkFlowLab#1918)` — the prompt is not well formed; `prompt.isWellFormed()` → `false !== true` - `not ok 26 - a long user line cut for the review keeps whole characters at both ends (ThinkFlowLab#1918)` — label `aaaaaa`, `prompt.isWellFormed()` → `false !== true`. The case message is 3001 units, but `ends` is handed the rendered line, `'User: ' + message` (3007), so the 2000-unit head budget spends six units on that prefix and cuts at unit 1994 of the message — the emoji's low half for this sample. The same case keeps both ends asserted, so its tail edge (3008 - 1000 = 2008, the emoji's low half) is checked too; an earlier sample put the head cut on a plain `a` and reached only the tail. ## The bound holds from both sides A legal prompt is not enough: a cut that threw most of the line away would also be well formed. In the two user-line cases `keptHead + keptTail` is 2993 and 2994 against `MAX = 3000`, and the test asserts `<= MAX` and `> MAX - 10` (never `=== MAX`). A temporary mutation proves the lower bound bites: with `REVIEW_USER_LINE_MAX` set to 1_500 (the 2000-unit head budget halved to 1000) the same test goes red — not ok 26 - a long user line cut for the review keeps whole characters at both ends (ThinkFlowLab#1918) error: 'short: 994 + 500' `not ok 32 - a backlog past the text limit is reviewed in passes, oldest first` also goes red in that run (`1 !== 2`): halving the constant changes how many passes the backlog takes, which is the mutation's own effect, not a defect. The constant was restored before the commit, and the run above with it restored is green. This mutation was measured again on the rebased tip, with the same result. ## What changed | file | change | | --- | --- | | `core/src/host/text.js` | `clip(text, max, rest)` makes `rest` optional: a truncating two-argument call prints `… (cut)`, a call with a reason prints `… (cut; <reason>)` byte for byte, and `text.length > max` / `codePointEnd` are untouched. | | `core/src/host/memory.js` | imports `{ codePointStart, cut }` from `../shared/text-boundary.js`; the local `clip` at `:1146` cuts through `cut` and keeps its ` …(cut)` suffix verbatim; the local `ends` at `:1147` takes its head through `cut` and moves the tail's start back with `codePointStart`, keeping the head budget for a tail that steps back one unit, so the kept text never exceeds the bounds it had. | | `core/test/text.test.js` | the two `clip` cases: no `undefined` with two arguments, reason preserved byte for byte with three. | | `core/test/memory.test.js` | the Bot-line case and the two-ended user-line case (head and tail edges), both through the real review prompt, with a two-sided bound on the kept line. | | `docs/subsystems/core-host.md` | the `text.js` row notes ThinkFlowLab#1918: the two review bounds go through the same rule, and `clip`'s reason is optional. | | `changelog.d/1918-clip-reason-optional.fixed.md` | the release note. | | `patches/ds-bot/0001-core-fixes.patch` | regenerated from `core/` (the patch that owns `text.js` and `memory.js`); a second `generate` is a no-op. | ## Ran - `TZ=UTC ARCH_DOCS_BASE=ac81a4f160a69859f7aea991cffa88f5078e90d4 npm run verify` on the committed tree above → exit 0: core **2321**, plugins social **541**, tree **127**, timeline **135**, workflow **336**, `.github/scripts` **563**, interop **37**, live-e2e **24** — 0 failures in every suite; `arch-docs: OK (7 changed file(s))` - `node scripts/ds-bot-patches.mjs check` → `ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70`, `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/` - `node scripts/ds-bot-patches.mjs canonical` → `ok patches/ds-bot/ is what generate writes (generate is a no-op)`; `git status --porcelain` empty after the second `generate` ## Not run - The CI `package` job (pack check, typecheck, actionlint) — not runnable here. - Smoke: the repository has no smoke job; `verify` is the gate. - The sandbox's `~/.npm` is read-only, so plugin packing in `verify` used `npm_config_cache` pointed at a workspace path (`tmp/npm-cache`); the suite itself ran as above. <!-- flybot:checkpoint --> ```json { "version": 1, "repo": "ThinkFlowLab/FlyBot", "issue": 1918, "pr": 1921, "claim": "Dev", "branch": "dev/1-clip-rest-optional", "base": "ac81a4f160a69859f7aea991cffa88f5078e90d4", "head": "c6972cc7ec1969ea55c082ec0f3e223b2f89dfd7", "tree": "acd588b223d86e556814104a7f0be15b64b190d4", "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=ac81a4f160a69859f7aea991cffa88f5078e90d4 npm run verify", "result": "green", "at": "c6972cc7ec1969ea55c082ec0f3e223b2f89dfd7" }, "next": "review", "files": [ "core/src/host/text.js", "core/src/host/memory.js", "core/test/text.test.js", "core/test/memory.test.js", "docs/subsystems/core-host.md", "changelog.d/1918-clip-reason-optional.fixed.md", "patches/ds-bot/0001-core-fixes.patch" ], "summary": "Made `clip()`'s reason optional so a two-argument cut prints `… (cut)` instead of `(cut; undefined)`. Routed the memory-review prompt and run-log bounds through the shared `cut`/`codePointStart` boundary rule, including `ends`'s tail edge, so no surrogate pair is split. Added the ThinkFlowLab#1918 tests and regenerated `patches/ds-bot/0001-core-fixes.patch`." } ``` <!-- /flybot:checkpoint -->
Fixes #1916
Owner: Dev
Written-by: Dev-2
Reviewer: Reviewer
Claim-Id: 243cd3afc769474d8eaa02a692835b71
Base: 4486660
Tree: 0768388a41928cd9eac0ca3f57103c40fb3c2620
Tip SHA: 7bef17d
Verified commit: d19a950 (the pushed tip
7bef17d7d8efcd30f71290f36dd36cc6a6feaf0bcarries the same tree0768388a, so that run covers the pushed code)Diff: 24 files, +444/−76
What
Nine cuts in eight rows on live main
44866600still sliced text on UTF-16 units, so a bound that landed between the halves of a surrogate pair stored or showed U+FFFD. Each now goes throughcut(text, max), so the prefix stops before a split pair while the limit stays the same:core/src/host/groups.js:51claimCueTextfallbackrefcore/src/host/groups.js:330task lead's name in a member promptcore/src/host/groups.js:722failed group Session's error textcore/src/host/groups.js:964read_own_chatquerycore/src/host/groups.js:965read_own_chataroundcore/src/host/browser.js:254open pageurl/titlecore/src/host/diagnostics.js:211and:220browser-logvalue/text/codecore/src/shared/remote-label.js:6cleanPeerNameshared/cannot importhost/text.js, so the boundary rule moved to a shared module instead of a second copy:core/src/shared/text-boundary.jsholdscodePointStart,codePointEndandcut,host/text.jsnow builds itscut(andexcerpt/clip) on that one implementation, andcleanPeerNameimports it directly.groups.js:330is a Bot name the owner sets (emoji included) that rides into every other member's system prompt;groups.js:722is the text of a failed group Session that lands in the room notice.meetings.js:30stays out: the issue's "Not in this change (next round)" records it as backlog.The #1915 changelog sentence about the claim line only holds once this change lands.
Red first (base
44866600, tests in place, source reverted)Seven new cases across five files; with the source files at their
44866600blobs they fail exactly as the issue predicts, and the same files pass after the change:The assertions are bounds, not exact widths:
length <= max,isWellFormed(), no U+FFFD, and the kept prefix is a byte-for-byte UTF-8 prefix of the input after the tail is dropped — a cut landing on a whole character keeps that character.Ran
TZ=UTC ARCH_DOCS_BASE=448666002cf9 npm run verifyon the committed treed19a950b(clean,git status --porcelainempty): exit 0.arch-docs: OK (24 changed file(s)); 0 lint errors (core 73 warnings, unchanged); patchesok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70,ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/,ok patches/ds-bot/ is what generate writes (generate is a no-op); core 2316 pass, social 541, tree 127, timeline 135, workflow 336,.github/scripts563, interop 37, live-e2e 24 — 0 failures.node scripts/ds-bot-patches.mjs generatefrom zero (0001, 0007, 0008, 0012, 0014 rewritten) and again as a no-op;npm --prefix core run buildfor the client bundle.Did not run
core/index.jsor process-level change) and the desktop app.Notes for review
core/client.jschanged only because it bundlesshared/remote-label.js, whosecleanPeerNamenow uses the shared boundary; nocore/src/client/file changed, which is why the commit carriesDocs-not-needed:for that owned path. The subsystem doc updated here isdocs/subsystems/core-host.md(thetext.jsrow and theremote-label.jsrow).test/browser-channel.test.jsis an upstream file, so it joinedpatches/ds-bot/topics.jsonunder0014-flybot-brandingbesidesrc/host/browser.js; that one line is why 0014 grew by the test diff.#1900's no-dossier-read-on-a-tick decision is not in this diff.Checkpoint
{ "pr": 1917, "claim": "Dev", "issue": 1916, "summary": "Nine bounds in eight rows still sliced on UTF-16 units, so a bound landing between the halves of an emoji stored or showed U+FFFD: the claim cue fallback, the task lead name, a failed Session's error text, read_own_chat query and anchor, an open page's address and title, a diagnostics browser-log line, and cleanPeerName. The boundary rule now lives in shared/text-boundary.js so host/text.js and the shared label code share one cut. Limits are unchanged and the change carries a red-first case per row.", "files": ["core/src/shared/text-boundary.js", "core/src/host/text.js", "core/src/host/groups.js", "core/src/host/browser.js", "core/src/host/diagnostics.js", "core/src/shared/remote-label.js", "core/test/group-work.test.js", "core/test/claim-cue.test.js", "core/test/browser-channel.test.js", "core/test/diagnostics.test.js", "core/test/group-bot-label.test.js", "docs/subsystems/core-host.md"], "checks": {"ran": "TZ=UTC ARCH_DOCS_BASE=448666002cf9 npm run verify", "result": "green", "at": "d19a950b8d49d098c5b2c321ab70f68bc38b522b"}, "next": "none — ready for review" }