Repository navigation
diagnostics: cut the report's stack summary on a code point boundary - #1920
Merged
Birfy merged 1 commit intoOct 31, 2026
Merged
Conversation
The report's `<details><summary>Stack: …` line kept the first 80 UTF-16 units of the message, so a message whose 80th unit fell inside an emoji showed half a surrogate pair, i.e. U+FFFD, in a report meant for a public issue. It now uses the host's `cut`, as the two neighbouring bounds in the same file already do (ThinkFlowLab#1916, ThinkFlowLab#1917), so the summary stops before a split pair and its limit is unchanged. Red first: `core/test/diagnostics.test.js` adds one case with `'a'.repeat(79)` + an emoji + `tail`; with the source reverted to `entry.text.slice(0, 80)` it fails on `isWellFormed() === true` (16 tests, 1 fail), and passes with the fix (16/16). The case asserts `length <= 80`, `isWellFormed()` and a byte-for-byte UTF-8 prefix of the message, not an exact length. Docs-not-needed: the stack summary keeps its 80-unit bound and the report's shape; only the unit the prefix stops on changes, and no owning subsystem page states the old slicing.
Contributor
Author
|
Verdict requested for this tip: 6af6eea (tree 005def4f91740a70f19d74e68d5f7065a0830358, base f87fceb) @Birfy review request #1919: the diagnostics report's Red first on base
Checks at this tip: 13 runs, 11 green completed, 1 in progress ( Owner: Dev |
Collaborator
|
Verdict: PASS at 6af6eea Ran at this tip: (The first tip run hit 6 ENOENTs because TMPDIR did not exist yet; the numbers above are the rerun after creating it. tip run at 10:35Z, red run at 10:36Z.) 审查要点：
Metadata: |
HugoXK
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 31, 2026
…rong bound ships green (ThinkFlowLab#1924) Fixes ThinkFlowLab#1922 Owner: Dev Written-by: Dev-2 Reviewer: Reviewer Claim-Id: 95d0e4245fd14bcc90983a9d998a1a0b Base: ac81a4f Tree: b607de8 Tip SHA: 934fa2f Verified commit: f9fc88d878939aba1d6dc6d2f0409ea4d12cbe07 (the pushed tip rebuilds the same tree `b607de83`, so that run covers the pushed code) Diff: 3 files, +50/−12 ## What ThinkFlowLab#1919's case for the report's stack summary pinned only the upper bound — `length <= 80`, `isWellFormed()`, a UTF-8 byte prefix — and none of those moves when the bound itself is wrong: with `cut(entry.text, 80)` changed to `cut(entry.text, 40)` the merged case stays 16/16 green (run 1 below). The same case now pins the bound by content: for `'a'.repeat(79) + '\u{1F600}' + 'tail'` the helper retracts one unit, so `assert.equal(shown, 'a'.repeat(79))` is exact and a halved bound fails it (run 2). `<` escaping is asserted in the same case, on input `'<'×3 + 'c'×200`: the summary shows `'<'×3 + 'c'×77`, contains no bare `<`, and unescaping gives back exactly the 80 units the cut produced — so the escape still happens after the cut. The stack body below the summary is compared byte for byte against `['', '```', 'at fake (/tmp/x.js:1:1)', '```', '</details>']` and against the stored entry's `stack`. This follows the boundary rule ThinkFlowLab#1918 and ThinkFlowLab#1916 settled: cut with the shared `cut`, and assert `length <= max`, `isWellFormed() === true` and a byte-for-byte UTF-8 prefix — never "exactly max", because the helper backs off when a pair straddles the bound. The content assertion is the addition, because an upper bound alone cannot see the bound move. `changelog.d/1919-stack-summary-cut.fixed.md` now says what the code does: the code point that straddles the bound is dropped whole, so the summary ends on the complete character prefix before it — aligned with ThinkFlowLab#1916's "stopped … showing half of an emoji". No source change: `core/src/host/diagnostics.js:87` stays `cut(entry.text, 80).text` on live main. ## Red first and mutation check Four runs of `core/test/diagnostics.test.js` alone (16 tests each, `PATH=/workspace/tools/node24/bin:$PATH node --import ./test/harness-resolve.mjs --test test/diagnostics.test.js` in `core/`): | # | source | case | result | |---|---|---|---| | 1 | live main, `cut(entry.text, 40)` | ThinkFlowLab#1919's case as merged (upper bound only) | **16/16 green** — the false green this issue is about | | 2 | live main, `cut(entry.text, 40)` | this PR's case | **15/16**, `not ok 5` | | 3 | pre-fix blob `f87fceb0:core/src/host/diagnostics.js` (`entry.text.slice(0, 80)`) | this PR's case | **15/16**, `not ok 5` | | 4 | live main, unmutated | this PR's case | **16/16 green** | Run 3 is the red ThinkFlowLab#1919 never had: the merged case is green on that blob, because it asserts only `isWellFormed` and the prefix — the content assertion is what catches the split pair. ```text run 2 — source mutated to cut(entry.text, 40), strengthened case not ok 5 - a stack summary cut at its bound never splits a surrogate pair (ThinkFlowLab#1919) expected: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' actual: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' run 3 — source at its f87fceb blob (entry.text.slice(0, 80)), strengthened case not ok 5 - a stack summary cut at its bound never splits a surrogate pair (ThinkFlowLab#1919) expected: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' actual: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ud83d' runs 1 and 4 — 16/16, # tests 16 / # pass 15 or 16 / # fail 0 ``` The case keeps ThinkFlowLab#1919's name; it is that same case, strengthened, not a new one. ## Ran - `TZ=UTC ARCH_DOCS_BASE=ac81a4f160a69859f7aea991cffa88f5078e90d4 npm run verify` on the committed tree `f9fc88d8` (`git status --porcelain` empty before and after): exit 0. `arch-docs: OK (3 changed file(s))`; 0 lint errors (core 73 warnings, unchanged from main); patches `ok 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 **2317** pass, social 541, tree 127, timeline 135, workflow 336, `.github/scripts` 563, interop 37, live-e2e 24 — 0 failures. - `node scripts/ds-bot-patches.mjs generate` twice (second run `git status` empty), then `check` and `canonical` green; only `patches/ds-bot/0007-version-and-tooling.patch` was rewritten, because the test lives in the upstream mirror. - The four runs in the table above, with the source restored from the committed tree afterwards. ## Did not run - The smoke test (no `core/index.js` or process-level change), the desktop app, and the merge queue — that is the review side's step. ## Notes for review - `Docs-not-needed:` is in the commit body as well as below: only the test and its changelog fragment change, the report's shape and its 80-unit bound are untouched, and no owning subsystem page describes the old wording. - No product code changes here. This is the test and changelog accounting ThinkFlowLab#1919's own Red-first section asked for, landed after ThinkFlowLab#1920 merged, plus the `0007` mirror that `generate` writes for the test file. Docs-not-needed: no owned subsystem file changes; the report's shape and its 80-unit bound are unchanged, so no architecture page needs an edit. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1924, "claim": "Dev", "issue": 1922, "summary": "The ThinkFlowLab#1919 case for the report's stack summary pinned only the upper bound, so a bound of 40 or 60 still shipped green. It now pins the bound by content: for the emoji input the summary must be exactly 'a'.repeat(79), '<' must be escaped after the cut, and the stack body below must be byte for byte the entry's stack. The changelog fragment now says the code point that straddles the bound is dropped whole instead of claiming the whole character is shown. No source change: diagnostics.js:87 keeps cut(entry.text, 80).text.", "files": [ "core/test/diagnostics.test.js", "changelog.d/1919-stack-summary-cut.fixed.md", "patches/ds-bot/0007-version-and-tooling.patch" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=ac81a4f160a69859f7aea991cffa88f5078e90d4 npm run verify", "result": "green", "at": "934fa2fd2699bece29f007d8f3a43a29733a140f" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
HugoXK
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 31, 2026
…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 -->
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 #1919
Owner: Dev
Written-by: Dev-2
Reviewer: Reviewer
Claim-Id: 876dfee430e04177bc605e408d386a87
Base: f87fceb
Tree: 005def4f91740a70f19d74e68d5f7065a0830358
Tip SHA: 6af6eea
Verified commit: fc3cbfc67e1d6f0f19d14d1815f306cd29a4566c (the pushed tip
6af6eeaea1f9carries the same tree005def4f91, so that run covers the pushed code)Diff: 5 files, +48/−2
What
The diagnostics report's stack summary kept its first 80 UTF-16 units with
entry.text.slice(0, 80), so a message whose 80th unit fell between the halves of a surrogate pair showed U+FFFD in a report meant for a public issue.core/src/host/diagnostics.js:87now usescut(entry.text, 80).text, the same helper the two neighbouring bounds in that file have used since #1916/#1917, so the prefix stops before a split pair; the 80-unit limit, the<escaping and the stack body below the summary are unchanged.The red-first case feeds a message of
'a'.repeat(79)+ an emoji +tailthroughnoteFailure, so the old slice ends on the high surrogate. It assertslength <= 80,isWellFormed() === true, no U+FFFD, and that what the summary shows is a byte-for-byte UTF-8 prefix of the message — not an exact width, because the helper returns 79 units rather than splitting the pair.A
changelog.d/1919-stack-summary-cut.fixed.mdfragment names the report line;patches/ds-bot/0001-core-fixes.patch(the source) and0007-version-and-tooling.patch(the test) are whatgeneraterewrote, and both are committed with it.Red first (base
f87fceb0, test in place, source at itsf87fceb0blob)With the fix the same file is 16/16, and the full core suite is 2317/2317.
Ran
TZ=UTC ARCH_DOCS_BASE=f87fceb0148f60ad8c20d02f9838e91c3debd5a8 npm run verifyon the committed treefc3cbfc6(clean,git status --porcelainempty): exit 0.arch-docs: OK (5 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 2317 pass, social 541, tree 127, timeline 135, workflow 336,.github/scripts563, interop 37, live-e2e 24 — 0 failures.node scripts/ds-bot-patches.mjs generateafter the edit (0001, 0007, 0008, 0012, 0014 rewritten), thencheckandcanonicalgreen withgeneratea no-op.Did not run
core/index.jsor process-level change), the desktop app, and the merge queue — that is the review side's step.Notes for review
Docs-not-needed:is in the commit body: the summary keeps its 80-unit bound and the report's shape; only the unit the prefix stops on changes, and no owning subsystem page describes the old slicing.005def4f; the verify above ran onfc3cbfc6with that same tree.Checkpoint
{ "pr": 1920, "claim": "Dev", "issue": 1919, "summary": "The diagnostics report's stack summary kept 80 UTF-16 units of the message, so a bound inside an emoji showed U+FFFD in a report meant for a public issue. diagnostics.js:87 now uses the host cut, as its two neighbouring bounds already do, so the prefix stops before a split pair while the 80-unit limit, the < escaping and the stack body stay the same. One red-first case fails on the old source and passes with the fix, asserting length <= 80, isWellFormed and a byte prefix rather than an exact width.", "files": ["core/src/host/diagnostics.js", "core/test/diagnostics.test.js", "changelog.d/1919-stack-summary-cut.fixed.md", "patches/ds-bot/0001-core-fixes.patch", "patches/ds-bot/0007-version-and-tooling.patch"], "checks": {"ran": "TZ=UTC ARCH_DOCS_BASE=f87fceb0148f60ad8c20d02f9838e91c3debd5a8 npm run verify", "result": "green", "at": "fc3cbfc67e1d6f0f19d14d1815f306cd29a4566c"}, "next": "none — ready for review" }