Repository navigation
os/v3/puppy/console-tui: text from a log or a catalogue is cleaned before it becomes a cell - #1118
Conversation
|
CI on One thing said precisely rather than glossed: the GitHub-managed Rebase note, unchanged: if #1088 lands first this needs one hunk in |
|
The rebase I said to expect, resolved and measured before anyone has to spend time on it. Test-merging #1088 for l in tail_file(LEMLOG, 256 * 1024, LOG_LINES * 8) {
out.push(format!("LEM {}", screen_text(&l))); // <- this PR; `LOG_LINES * 8` and pane_lines() are #1088's
}
pane_lines(out, LOG_LINES)The combination was then built and tested, not eyeballed: So whichever merges first, the second one is a three-line resolution. If #1088 lands first, say the word and I will push the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7142fa13 — I ran my own assertion set against the function, and it holds on the vectors that matter.
I extracted screen_text() and compiled it standalone rather than relying on the PR's tests, then ran eight cases of my choosing:
"\u{1b}[2J\u{1b}[31mRED" -> "RED" CSI removed whole (the measured case from the body)
"a\u{1b}]0;title\u{7}b" -> "ab" OSC terminated by BEL
"a\u{1b}]0;t\u{1b}\\b" -> "ab" OSC terminated by ST <- the one that is usually missed
"tab\there" -> "tab here" TAB becomes a space
"c1\u{9b}[31mdanger" -> "c1[31mdanger" C1 CSI (U+009B) dropped <- a real vector on some terminals
"°C é ─ text" -> "°C é ─ text" text stays text
"bell\u{7}del\u{7f}" -> "belldel" C0 and DEL dropped
"\u{1b}(Bcharset" -> "Bcharset" 2-byte fallback
0 case(s) mismatched
cargo test --offline --release at this head: 54 passed (53 + the new case).
Why the OSC-ST case is the one I checked hardest: an OSC that ends only on BEL is the easy half; ESC \ is the terminator that a hand-written scanner forgets, and a forgotten ST means everything after it — including the next prompt — is swallowed or leaked depending on the terminal. It is handled. C1 CSI matters for the same reason: U+009B is a char, not a byte, so a sanitiser written against \x1b[ alone lets it through, and some terminals honour it.
One nit: the fallback for an unrecognised escape consumes two characters, so ESC ( B (a 3-byte charset designation) leaves a stray B in the output. Harmless — a letter, not a control — but charsets do appear in captured output, so a two- or three-byte table for the ESC (/ESC ) family would be tidier than the present guess.
The scoping is right and worth saying: the sanitiser is applied at every point where text from outside becomes a cell — the catalogue id, the backend name, the recipe, and both log feeds (KRN/LEM) — and nowhere else. That is the correct blast radius: it is the terminal that interprets escapes, so cells are the boundary, and treating the whole app as untrusted text would have been the over-broad version of this fix.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Conflict flag, not a verdict change: this PR is now mergeStateStatus=DIRTY — it conflicts on os/v3/puppy/console-tui/src/facts.rs.
My VERDICT at the head I reviewed stands on content; nothing about the code has been questioned. But it cannot merge while it conflicts, so it needs a rebase. When it is rebased, my verdict carries if the PR's own files hash the same — that is how I checked the last several rebases (per-file blob identity, not the summary), and I will re-check this one the same way rather than assume.
If the rebase has to touch the conflicting file beyond a mechanical resolution, say so in the PR and I will re-read the hunk instead of carrying the verdict.
…fore it becomes a cell The console draws two things it did not write: the tail of Lemonade's log and the model/recipe/backend names from /api/v1/models. A cell's symbol is written through to the terminal unchanged, so an escape sequence that arrives as data is an escape sequence on the appliance's own screen. Measured on main: a log line containing ESC[2J ESC[31m put both sequences into the rendered frame byte for byte (2 ESC bytes, positioned at the pane's row) - a clear-screen and a colour command in the middle of the console a person is standing in front of. The engine also emits escapes on purpose: every progress bar line ends with ESC[K, which reached the pane as a literal "[K" on the cycle-25 take's own log. facts::screen_text() strips CSI and OSC sequences whole (the bytes after an escape are control, not text), turns TAB into a space because the panes have no tab stops, drops every other control character, and leaves text alone - degree signs, accents and the box-drawing the panes use all stay. Applied where external text enters: every LEM and KRN log line, and the catalogue's id/recipe/backend. Measured after: the same staged line renders "evilRED", the frame has 0 ESC bytes, and both committed snapshots are byte-identical (the fixtures carry no escapes). 54 tests (53 + a_log_line_or_a_model_name_cannot_carry_an_escape_into_the_screen, which covers CSI, OSC 8 with ST and with BEL, the two-byte escapes, TAB/C0/DEL, four non-ASCII strings kept verbatim, and the catalogue path). clippy --all-targets and cargo fmt --check clean, tools/copyright.py --check rc 0. Artifact b36f0b4a7374c2b9, 1,585,760 B, static. Not in scope: this crate's JSON reader does not implement \uXXXX escapes, so a name that arrives as "\u00e9" would display as "u00e9" - a separate, older parser limitation, noted here rather than fixed inside an escaping change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7142fa1 to
845d153
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 845d153d — screen_text is byte-identical across the rebase, and the crate's tests pass at this head.
facts.rs: 74 changed lines, all of them #1088's log-pane work arriving from main (`pane_lines`, the LOG_LINES*8 read, the polling comment)
screen_text(): IDENTICAL (diffed the function body alone, not the file)
cargo test --offline --release: 55 passed, 0 failed (54 before; +1 from #1088's pane test)
So the sanitizer I verified with my own eight-case probe is the same code, and the extra test count is the sibling PR's, not a
change to mine.
Restating what that probe established, because it is the content of this PR: CSI removed whole, OSC terminated by BEL and by
ST, C1 CSI (U+009B) dropped, TAB → space, C0/DEL dropped, and °C é ─ preserved — with the one nit that ESC ( B leaves a
stray B because the unrecognised-escape fallback consumes two characters of a three-byte sequence. The scoping (applied at
every point where external text becomes a cell: catalogue id, backend name, recipe, both log feeds) is the correct blast radius.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
…nges and stops saying 52 tests (#1149) The crate's README describes the frame heal (#1093) but not the two changes that arrived with it, so a reader of the shipped crate cannot see why a control byte from Lemonade's log is harmless, or why an interrupted paint does not end the screen: * #1118 - facts::screen_text() cleans text from outside (CSI and OSC removed whole, TAB to a space, other C0/DEL dropped) before it becomes a cell. Measured against the recorded take's own log: the shipped c26 candidate (5b21a0a8, heal and no sanitiser) leaves 4 escape bytes in one rendered frame and on a 240x67 pty cuts its progress row at the first ESC[K; this build is 0 bytes. On the appliance's own 160x50 VT (booted c26 candidate, QEMU) the same wrapped progress line came back whole on BOTH binaries - the once-a-second full paint rewrites the cells the erase cleared - so the README calls this defence in depth rather than a visible repair, which is what the measurement says. Test: a_log_line_or_a_model_name_cannot_carry_an_escape_into_the_screen * #1102 - a paint that fails with ErrorKind::Interrupted repaints instead of ending the console (crossterm's SIGWINCH handler makes an ssh resize the realistic trigger); every other kind still exits to the dialog menu. Test: an_interrupted_paint_is_repainted_but_a_dead_terminal_ends_the_console Also corrects the test count: cargo test is 55, not the 52 the README has said since #1050/#1059. Docs only - no code, no frame, no gate string touched. cargo test 55/55, tools/copyright.py --check rc 0. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
… is not cafu00e9 `json_raw()` handled `\n` and `\t` and passed every other escape through as the character that followed the backslash. So `\u00e9` became the four characters `u00e9`, `\r` became the letter `r`, and a model name or a recipe that arrived escaped was drawn wrong on the pane: Lemonade's catalogue is JSON, and a serialiser that escapes non-ASCII (Python's json.dumps does it by default) sends `caf\u00e9`, not the UTF-8 byte. #1118 named this and left it out of scope; this closes it. The decoder is one function (json_unicode) and one arm of the reader: \uXXXX -> the character (4 hex digits, as few or as many as are there) surrogate pair -> ONE character (U+D83D U+DE00 -> U+1F600); a lone half -> U+FFFD truncated \u12 -> "u12", and it consumes only the hex digits it took (so the closing quote still closes the string) \r \b \f -> the control character, not the letter unknown \q -> q, as before Order matters and is now stated where the sanitiser is: the escape is decoded FIRST, then screen_text() strips what it decoded to. A body carrying `\u001b[2J` used to reach the sanitiser as the six characters `u001b`, so #1118 had nothing to remove; now the ESC is real and is removed. Measured: cargo test 64 passed (62 on main + 2), and the two new tests are RED with the old reader put back - rewound and re-run: left: Some("cafu00e9") right: Some("café") left: ["qwen3-cafu00e9"] right: ["qwen3-café"] Frames unmoved: --snapshot 80x25/240x67 --fixture are byte-identical to the committed snapshots (the fixtures carry no escapes). clippy --all-targets -D warnings clean, cargo fmt --check clean (facts.rs was fmt-clean on main), tools/copyright.py --check rc 0, static rebuild be28882b1016b3c4, 1,596,512 B. The README stops carrying a test count as well: it said "55 tests" and the number has been wrong three times in two days (52 -> 55 -> 58 -> 62), so the line now says what the suite covers instead of a digit that the next test-bearing PR invalidates. Not measured end to end on the appliance: the API host is a const (127.0.0.1:13305) and this box has a live lemond there, so the proof is at the reader and at the catalogue/engine row, not a photographed frame.
… is not cafu00e9 (#1194) `json_raw()` handled `\n` and `\t` and passed every other escape through as the character that followed the backslash. So `\u00e9` became the four characters `u00e9`, `\r` became the letter `r`, and a model name or a recipe that arrived escaped was drawn wrong on the pane: Lemonade's catalogue is JSON, and a serialiser that escapes non-ASCII (Python's json.dumps does it by default) sends `caf\u00e9`, not the UTF-8 byte. #1118 named this and left it out of scope; this closes it. The decoder is one function (json_unicode) and one arm of the reader: \uXXXX -> the character (4 hex digits, as few or as many as are there) surrogate pair -> ONE character (U+D83D U+DE00 -> U+1F600); a lone half -> U+FFFD truncated \u12 -> "u12", and it consumes only the hex digits it took (so the closing quote still closes the string) \r \b \f -> the control character, not the letter unknown \q -> q, as before Order matters and is now stated where the sanitiser is: the escape is decoded FIRST, then screen_text() strips what it decoded to. A body carrying `\u001b[2J` used to reach the sanitiser as the six characters `u001b`, so #1118 had nothing to remove; now the ESC is real and is removed. Measured: cargo test 64 passed (62 on main + 2), and the two new tests are RED with the old reader put back - rewound and re-run: left: Some("cafu00e9") right: Some("café") left: ["qwen3-cafu00e9"] right: ["qwen3-café"] Frames unmoved: --snapshot 80x25/240x67 --fixture are byte-identical to the committed snapshots (the fixtures carry no escapes). clippy --all-targets -D warnings clean, cargo fmt --check clean (facts.rs was fmt-clean on main), tools/copyright.py --check rc 0, static rebuild be28882b1016b3c4, 1,596,512 B. The README stops carrying a test count as well: it said "55 tests" and the number has been wrong three times in two days (52 -> 55 -> 58 -> 62), so the line now says what the suite covers instead of a digit that the next test-bearing PR invalidates. Not measured end to end on the appliance: the API host is a const (127.0.0.1:13305) and this box has a live lemond there, so the proof is at the reader and at the catalogue/engine row, not a photographed frame. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Closes #1117. Found by asking the next question after the repaint fix: the console now survives what other processes write - but
what does it do with what it draws itself? Two of its inputs are not ours: Lemonade's log, and the model/recipe/backend names
from
/api/v1/models. Both went into cells unchanged, and ratatui writes a cell's symbol through to the terminal unchanged.Measured on
maintodayA log line containing
\x1b[2J\x1b[31m(staged at/var/log/lemond-onebit.log, then--snapshot 80x25- the same way thecommitted frames are rendered):
So the frame the console hands the terminal contains a clear-screen and a colour command, positioned at the pane's row: a
clear and a colour change in the middle of the screen a person is standing in front of. And the bytes arrive without anyone being
malicious - at
log_level: debugLemonade's log carries chat prompts (a demo page's textarea is a paste target), model names andthe engine's own output, and the engine emits escapes on purpose: every progress bar line ends with
\x1b[K, which reachedthe pane as a literal
[Kon the cycle-25 take's own log. The catalogue is the second path: a name from a Hub is drawn in theMODELS panel and in the
engine:row.The change (
src/facts.rs, +100/−4)facts::screen_text(&str) -> String:ESC [ … final)[K,[2J,[31m)ESC ] … BELorESC \)ESC xSoC 78°C,café,┌─┐are text and stayApplied where external text enters, so it cannot be forgotten at a draw site: every
LEMandKRNlog line, and thecatalogue's
id/recipe/backend.Tests
54 (53 +
a_log_line_or_a_model_name_cannot_carry_an_escape_into_the_screen): CSI (ESC[K,ESC[2J ESC[31m), OSC 8 with STand with BEL, a two-byte escape, TAB/C0/DEL, empty string, four non-ASCII strings kept verbatim, and the catalogue path with a
literal ESC in the id and in
onebit_backend(asserting what is drawn, not what the parser returned).Also on the artifact: both committed snapshots byte-identical (the fixtures carry no escapes, so no baseline churn), the eight
strings
vm-check.sh/menugategrep still on the 80x25 frame, clippy--all-targetsclean,cargo fmt --checkclean,tools/copyright.py --checkrc 0. Artifactb36f0b4a7374c2b9, 1,585,760 B, static.Not in scope, and said rather than fixed
This crate's JSON reader does not implement
\uXXXX, so a"caf\u00e9"in a catalogue answer displays ascafu00e9. That is anolder parser limitation, it is not an escaping hole, and it does not belong inside this change - flagged so a reviewer does not
have to find it.
No image build, no board run, no device ticket,
:18097untouched. Nothing overlaps #1088 (facts.rs's log pane is theirs, butthis touches
models_from,logs()'s twoformat!calls and a new function; if #1088 lands first I will rebase - measuredconflict expected only in
logs(), one hunk).