From e264677d5677239b49778e64b94629eee2308223 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:10:35 +0700 Subject: [PATCH 1/9] fix(security): clean a .art header where it enters (GHSA-jp9f-97rv-j4hx) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three header fields were printed exactly as the file wrote them, while every other piece of untrusted text went through `printable()` at its entry point — the rule `src/lib.rs` states in as many words. A `.art` file is the one thing this project asks strangers to send. #57 is an open invitation to contribute templates and CONTRIBUTING §11 makes the review path "open the file with `mossaic-art`", so a reviewer running `--matrix theirs.art`, or anyone running `--list-templates`, executed whatever the header said: a window-title change, an `OSC 52` clipboard write, or a cursor-position query whose reply is typed back into the shell once the tool exits. Nothing warned, because the point of an escape sequence is that it is not displayed, and `--no-colour` suppressed it on no path. It travelled, too: through `--save` into the plan and back out raw on reload, through `--format json`'s `headline` and `text`, through `--format markdown` into `$GITHUB_STEP_SUMMARY`, and out of the Action's `headline` output where the consumer republishes it. `build.rs` embeds `art/templates/*.art`, so a merged template would have shipped its payload to every user on every listing. Cleaned in the `Canvas` meta reader, which covers the report header, the template listing, both report formats, the saved plan and the editor title in one place — and bounded to 200 characters, because an unbounded `# name:` produced a 200,061-byte first output line. Cleaning rather than refusing: a file authored on Windows carries a trailing CR on every line, and refusing any control character would reject it for something nobody typed. The regression test covers an OSC-and-SGR name, a bare carriage return, and the length bound. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- .claude/skills/termlens/SKILL.md | 452 +++++++++++++++++++++++++++++++ Cargo.lock | 4 +- Cargo.toml | 2 +- SECURITY.md | 24 +- src/art.rs | 31 ++- src/render_tests.rs | 55 ++++ 6 files changed, 561 insertions(+), 7 deletions(-) create mode 100644 .claude/skills/termlens/SKILL.md diff --git a/.claude/skills/termlens/SKILL.md b/.claude/skills/termlens/SKILL.md new file mode 100644 index 0000000..311e12a --- /dev/null +++ b/.claude/skills/termlens/SKILL.md @@ -0,0 +1,452 @@ +--- +name: termlens +description: Write, fix or review headless terminal tests for a Rust CLI or TUI (ratatui, crossterm, cursive, plain println) with the termlens crate — spawn the real binary in a PTY, wait on the rendered screen without sleeping, snapshot it with insta, assert on cells and styles. Use whenever a test spawns a terminal program, whenever a terminal test is flaky, sleeps or times out, and whenever someone asks how to test a TUI end to end. +--- + +# Testing terminal programs with termlens + +Written against **termlens 0.9.0**. Every `rust` block below is a complete +integration test that is compiled against the crate in CI, so the API it +shows is the API that exists. The recipes spawn a binary called `myapp` +that draws a list with a `> ` highlight, a status line ending in +`Ready: j/k move, q quits`, and prints usage on `--help`; substitute your +application's own texts where the comments say so. + +## 1. What termlens is, and when to reach for it + +termlens spawns your **real binary** in a **real pseudo-terminal**, drains +its output on a reader thread through a VT emulator into an in-memory +**screen grid**, and lets a test wait on and assert against that grid — +Playwright for the terminal. Unix only (Linux, macOS). + +Use it for the things an in-process mock cannot see: + +- raw-mode and alternate-screen entry and exit, and whether the terminal is + left broken after a panic or a `q`; +- what the user actually sees — box drawing, colours, wide characters, + cursor position — after the bytes have been through a terminal; +- key, mouse, paste and resize handling as the terminal really encodes them + under the modes the application enabled; +- a CLI's `--help`, its exit code, its behaviour when the terminal is 40 + columns wide; +- anything printed outside the framework: a stray `println!`, a logger, a + panic message. + +Keep using `ratatui::backend::TestBackend` (or plain unit tests) for widget +layout and rendering logic: it is faster and finer-grained. termlens is the +second layer — a small number of end-to-end flows through the real binary. + +## 2. The model in sixty seconds + +```text +your binary ──PTY──▶ reader thread ──▶ VT emulator ──▶ Screen (immutable snapshot) + ▲ +your test ── send(Key) · click · paste · resize ──▶ PTY └── wait_until / snapshot_after / wait_exit +``` + +- **A `Screen` is one consistent instant.** Every accessor on it reads the + same snapshot; two `screen()` calls are two instants. +- **Every wait is deadline-bounded** (5 s by default) and **every failure + embeds the screen**, so a timeout in CI shows what the application was + displaying. There is no unbounded wait, on purpose. +- **Nothing sleeps.** The reader thread drains continuously; waits wake on + new output. This is what makes the tests fast when green and readable + when red. +- **Nothing is claimed that the emulator cannot see.** Terminal queries the + application sends (cursor position, device attributes, mode probes) are + answered truthfully or left unanswered and named in the next timeout. + +## 3. Golden rules — read these before writing a test + +1. **Never `thread::sleep`.** A sleep is either too short (flaky under CI + load) or too long (slow every run), and it hides *what* you were waiting + for. Wait on the screen instead: `wait_until(|s| …)` for a fact, + `snapshot_after(|s| …)` for a fact followed by a whole-screen snapshot, + `wait_stable(quiet)` for "the picture stopped changing". The single + sanctioned delay is `send_after(delay, key)`, which exists because `Esc` + followed immediately by another key is byte-identical to an `Alt` chord. + +2. **Snapshot only a settled screen.** `wait_until(pred)` guarantees the + bytes that made `pred` true were processed — and nothing more. A repaint + has no end marker, so the predicate can fire on a half-painted screen, + including half a row. Either wait on the **last** thing the application + paints, or use `snapshot_after`, which waits for the predicate and then + for the picture to hold still for 100 ms before handing you the screen. + +3. **One predicate per instant.** Everything you assert about one moment + goes into one closure: `wait_until(|s| s.contains("NORMAL") && + s.contains("Tasks 1/10"))`. `wait_until(a)` followed by + `assert!(screen().b)` is a race between two instants. + +4. **Spawn your own binary with `termlens::bin!("myapp")`.** It expands to + the builder chain every test otherwise repeats — `size(80, 24)`, + `env_clear()`, `timeout(5 s)`, `spawn(env!("CARGO_BIN_EXE_myapp"))` — and + a misspelled name is a **compile error**, not a spawn failure at run + time. Builder calls after the name override any default: + `bin!("myapp", size(120, 40), env("NO_COLOR", "1"))`. + +5. **Geometry is `(cols, rows)`, from 2 to 1000 per axis.** `size(0, 0)` and + `size(1, 1)` are refused with `Error::Size`: one column panics the + emulator on a double-width character and one row panics it on a line + that wraps. Grids past 1000 per axis are refused because every snapshot + costs one entry per cell. 80x24 is the default and is what you want. + +6. **Two coordinate orders exist; do not mix them.** Everything that + addresses a cell is **row-first**: `find` → `(row, col)`, `cell(row, + col)`, `row_text(row)`, `cursor()` → `(row, col, visible)`. Everything + that speaks of terminal geometry or a pointer is **column-first**: + `size()` → `(cols, rows)`, `resize(cols, rows)`, `click(col, row)`, + `scroll(col, row, …)`, `drag(button, (col, row), (col, row))`. Never + pass a `find` result straight into `drag` — the tuple types match and + the axes do not. + +7. **Always finish the process.** Send the quit key, `wait_exit()?` and + assert on the `ExitStatus` (`success()`, `code()`, `signal()`), then + assert `!t.screen().alternate_screen()` so an application that leaves + the user's terminal in the alternate screen fails the test. `Drop` kills + and reaps whatever is left, so a failing test never leaks a process. + +8. **`wait_frame` only works for applications that emit DEC 2026 + synchronized updates.** Stock ratatui 0.30 with crossterm does **not** + (measured: `repaints()` stays 0), so `wait_frame` times out against it + with a message saying exactly that. Default to `snapshot_after`. Use + `wait_frame` only if the application brackets its repaints in + `BeginSynchronizedUpdate` / `EndSynchronizedUpdate`. + +9. **Return `termlens::Result<()>` from the test and use `?`.** The + `Display` of every error carries the screen, so a failing wait prints + the grid the application was showing instead of `called unwrap() on Err`. + +10. **Snapshot the `Screen`, not its text.** `insta::assert_snapshot!(screen)` + records the header (`size: 80x24 cursor: 3,5` or `cursor: hidden`) and + the grid; `screen.with_styles()` adds a `styles:` block that catches a + colour regression. `.text()` drops the header and `format!("{:?}")` is + the same as `Display`. Review changes with `cargo insta review`; never + blind-accept with `INSTA_UPDATE=always`. + +11. **The environment is hermetic by default — set what the app reads.** + Under `env_clear()` (which `bin!` applies) the child sees only + `TERM=xterm-256color`, `SHELL=/bin/sh` and what you set with `env(…)`. + No `HOME`, no `LANG`, no `COLORTERM`, no `NO_COLOR`, no `PATH` — so a + bare program name cannot resolve (use an absolute path or `bin!`), and + an application that checks `NO_COLOR` or `COLORTERM` needs them set + explicitly for the case under test. + +12. **The grid is Unicode-aware; think in cells.** A double-width character + (CJK, most emoji) occupies two cells: the leading one `is_wide()`, the + next `is_wide_continuation()`. `find` reports real terminal columns. + `contains` and `find` fold both sides to NFC and search the **visible + screen only** — text that scrolled off is in `full_text()`, and a line + that wrapped is two rows, so a needle spanning the wrap is not found. + +## 4. Setup + +```toml +[dev-dependencies] +termlens = "0.9" +insta = "1" # for the snapshot recipes; termlens also re-exports it as `termlens::insta` +``` + +- Put the tests in `tests/` **of the package that owns the `[[bin]]`**: + Cargo sets `CARGO_BIN_EXE_` only there, and `bin!` needs it at + compile time. For a binary in a sibling crate, build it and pass the path + to `Terminal::builder().spawn(path)` instead. +- The binary is built by `cargo test` before the tests run. Tests run in + parallel by default; each spawns its own PTY, which is fine. +- Gate the test file with `#![cfg(unix)]` if the crate must also build on + Windows. +- `add --features decode` only if you assert on the pixels of inline images. + +## 5. Recipes + +### Recipe A — hermetic CLI snapshot (`myapp --help`) + +```rust +use termlens::Terminal; + +#[test] +fn help_renders_and_exits_zero() -> termlens::Result<()> { + // 80x24, cleared environment, 5 s deadline, compile-time-checked path. + let mut t = termlens::bin!("myapp", args(["--help"]))?; + + // Wait for the LAST line of the help text before waiting for exit. A + // program that prints and exits within a millisecond can, rarely and + // under load on macOS, lose its tail to PTY teardown; waiting on the + // tail first turns that into a loud timeout instead of a truncated + // snapshot that passes. + t.wait_until(|s| s.contains("q quit"))?; // your help's last line + let status = t.wait_exit()?; + assert!(status.success(), "exit status: {status}"); + + // The header records the size and cursor; the body is the grid. + insta::assert_snapshot!(t.screen()); + Ok(()) +} + +#[test] +fn a_bad_flag_is_reported_with_an_exit_code() -> termlens::Result<()> { + let mut t = Terminal::builder() + .size(80, 24) + .env_clear() + .timeout(std::time::Duration::from_secs(5)) + .args(["--definitely-not-a-flag"]) + .spawn(env!("CARGO_BIN_EXE_myapp"))?; + // stderr lands on the same screen as stdout: it is one terminal. + t.wait_until(|s| s.contains("unexpected argument"))?; + let status = t.wait_exit()?; + // Assert what your CLI promises; clap exits 2 on a usage error. + assert_eq!(status.code(), Some(2), "status: {status}"); + assert_eq!(status.signal(), None, "exited, not killed: {status}"); + Ok(()) +} +``` + +### Recipe B — interactive ratatui navigation and keystrokes + +```rust +use termlens::Key; + +#[test] +fn moving_the_highlight_and_quitting_cleanly() -> termlens::Result<()> { + let mut t = termlens::bin!("myapp")?; + + // Predicate, then a 100 ms settle, then the screen: the safe sequence + // for a whole-screen snapshot. Name the last thing the app paints. + let first = t.snapshot_after(|s| s.contains("Ready"))?; + assert!(first.alternate_screen(), "a TUI should be on the alternate screen:\n{first}"); + assert!(first.contains("> Alpha"), "{first}"); + insta::assert_snapshot!("initial_frame", first); + + // Send a key, then wait on what the key CHANGES — not on text that was + // already true before the key, or the wait returns the old screen. + t.send(Key::Char('j'))?; + let moved = t.snapshot_after(|s| s.contains("> Beta"))?; + assert!(!moved.contains("> Alpha"), "{moved}"); + + // Arrow keys and chords encode as the terminal would (DECCKM-aware). + t.send(Key::Down)?; + t.wait_until(|s| s.contains("> Gamma"))?; + t.send(Key::Up)?; + t.wait_until(|s| s.contains("> Beta"))?; + + // Finish: quit, assert the exit, and assert the terminal was restored. + t.send(Key::Char('q'))?; + let status = t.wait_exit()?; + assert!(status.success(), "status: {status}"); + assert!(!t.screen().alternate_screen(), "the app left the terminal in the alternate screen"); + Ok(()) +} +``` + +If the flow needs `Esc` followed by another key, use the one sanctioned +delay — `t.send_after(Duration::from_millis(20), Key::Char('j'))?` — so the +application's read boundary falls between the two writes and it sees two +presses rather than one `Alt-j` chord. + +### Recipe C — overriding the defaults (size, environment, deadline) + +```rust +use std::time::Duration; +use termlens::Color; + +#[test] +fn honours_no_color_at_a_custom_size() -> termlens::Result<()> { + // Builder calls after the name override bin!'s defaults; the rest stay. + let mut t = termlens::bin!( + "myapp", + size(100, 30), // (cols, rows) + env("NO_COLOR", "1"), // the app reads this at startup + timeout(Duration::from_secs(10)), // every wait's default deadline + )?; + let s = t.snapshot_after(|s| s.contains("Ready"))?; + + assert_eq!(s.size(), (100, 30), "{s}"); + + // With NO_COLOR the title is drawn in the default colour, not cyan. + let (row, col) = s.find("myapp").expect("title is on screen"); + let title = s.cell(row, col).expect("in range"); + assert_eq!(title.style().fg, Color::Default, "{}", s.with_styles()); + assert!(!title.style().bold); + + // One slow step gets its own deadline instead of a slower suite. + t.wait_until_for(|s| s.contains("Ready"), Duration::from_secs(30))?; + Ok(()) +} +``` + +### Recipe D — targeted screen and style assertions + +```rust +use termlens::Color; + +#[test] +fn cells_styles_and_wide_characters() -> termlens::Result<()> { + let mut t = termlens::bin!("myapp")?; + let s = t.snapshot_after(|s| s.contains("Ready"))?; + + // Text: visible screen, NFC-folded, trailing padding never matched. + assert!(s.contains("Alpha") && s.contains("Beta"), "{s}"); + assert_eq!(s.find("Ready"), Some((23, 0)), "status line sits on the last row: {s}"); + + // Cells and styles: the highlighted row is drawn in reverse video. + let (row, col) = s.find("> Alpha").expect("highlight"); + let cell = s.cell(row, col).expect("in range"); + assert!(cell.style().reverse, "{}", s.with_styles()); + // A coloured, bold title: ratatui's Cyan is ANSI colour 6. + let (trow, tcol) = s.find("myapp").expect("title"); + let title = s.cell(trow, tcol).unwrap().style(); + assert_eq!((title.fg, title.bold), (Color::Indexed(6), true)); + + // Find a cell by a property of the cell rather than by its text. + assert_eq!(s.find_by(|c| c.style().reverse), Some((row, col))); + + // Wide characters: one glyph, two cells, real columns reported. + let (crow, ccol) = s.find("東京").expect("CJK item"); + assert!(s.cell(crow, ccol).unwrap().is_wide()); + assert!(s.cell(crow, ccol + 1).unwrap().is_wide_continuation()); + assert_eq!(s.row_text(crow).trim_matches(['│', ' ']), "東京"); + + // Regions and the cursor. rect_text is (cols, rows), like size(). + let list_pane = s.rect_text(0..20, 0..6); + assert!(list_pane.contains("Gamma"), "{list_pane}"); + let (_, _, visible) = s.cursor(); + assert!(!visible, "a list view hides the cursor: {s}"); + + t.send(termlens::Key::Char('q'))?; + assert!(t.wait_exit()?.success()); + Ok(()) +} +``` + +## 6. Reading a failure + +Every error's `Display` ends with the screen, under a header that says +which screen it is (`--- screen at timeout ---`, `--- final screen ---`). +Read the first line for the cause: + +| First line says | Meaning | Do | +|---|---|---| +| `timed out after 5s while waiting for the screen predicate to hold` | the predicate never became true | look at the embedded grid; the text is usually spelled differently, on another row, or scrolled off (the note says how many rows scrolled) | +| `… note: N rows have scrolled off the top` | the text went into history | assert with `full_text()` / `scrollback_text()` | +| `… note: the application queried the terminal (^[[?u …) and received no answer` | the app is blocked on a probe termlens deliberately does not answer | the app needs a fallback; see the termlens README's Known limitations | +| `terminal closed (EOF) while waiting for …` | the app exited before the predicate held | check `wait_exit()` first, or the app crashed — the final screen shows why | +| `the application never emitted a DEC 2026 synchronized update` | `wait_frame` against an app without synchronized output | use `snapshot_after` / `wait_until` (rule 8) | +| `input not receivable: the application has not enabled mouse tracking` | `click`/`drag`/`scroll` before the app enabled the mouse | `wait_until(|s| s.mouse_mode() != MouseMode::None)` first | +| `input not receivable: mouse at (50, 2) is outside the 20x5 grid` | coordinates swapped or out of range | rule 6 | +| `failed to spawn \`sh\`: \`sh\` is a bare program name and env_clear() removed PATH` | bare program name under `env_clear` | absolute path, `bin!`, or `.env("PATH", …)` | +| `invalid terminal size: a terminal needs at least 2 columns and 2 rows` | geometry below 2x2 (past 1000 has its own message) | rule 5 | +| `the terminal emulator failed and the screen stopped advancing` (`Error::Emulator`) | a bug in the emulation, not in your app | report it to termlens with the detail it names | + +## 7. API cheat sheet + +**Spawn** — `termlens::bin!("name" $(, method(args))*)` or +`Terminal::builder()`: + +| Builder method | Meaning | +|---|---| +| `.size(cols, rows)` | 2..=1000 each; default 80x24 | +| `.timeout(Duration)` | default deadline for every wait (5 s) | +| `.arg(a)` / `.args([..])` | program arguments | +| `.env(k, v)` / `.envs([..])` / `.env_clear()` | environment; `env_clear` keeps `TERM` and `SHELL` pinned and drops the rest | +| `.current_dir(path)` | default: the test process's directory | +| `.scrollback(rows)` | history retained (default 1000, text only) | +| `.spawn(program) -> Result` | program is a path or a name on `PATH` | + +**Wait** (all return `termlens::Result`, all embed the screen on failure, all have a `_for(…, timeout)` twin): + +| Method | Returns | Use for | +|---|---|---| +| `wait_until(\|s\| bool)` | `()` | a fact about the screen | +| `snapshot_after(\|s\| bool)` | `Screen` | a fact, then a settled whole-screen snapshot | +| `wait_stable(quiet)` | `Screen` | the picture unchanged for `quiet`; bells and no-op repaints do not reset it | +| `wait_idle(quiet)` | `()` | no *bytes* for `quiet` — a weaker, older sibling of `wait_stable` | +| `wait_frame(\|s\| bool)` | `Screen` | complete DEC 2026 frames only (rule 8) | +| `wait_exit()` | `ExitStatus` | the child's exit; `success()`, `code() -> Option`, `signal() -> Option<&str>` | + +**Drive**: `send(Key)`, `send_str("text")` (no Enter — send `Key::Enter` +yourself; `"\n"` would send LF, not CR), `paste("text")` (bracketed if the +app enabled it), `send_after(delay, Key)`, `click(col, row)`, +`click_with(MouseButton::Right, col, row)`, `drag(MouseButton::Left, (c, r), +(c, r))`, `scroll(col, row, Scroll::Down)`, `resize(cols, rows)`, +`focus_in()` / `focus_out()`, `signal(Signal::Term)` (Unix), `pid()`. + +**Keys**: `Key::Char('j')`, `Enter`, `Esc`, `Tab`, `BackTab`, `Backspace`, +`Delete`, `Insert`, `Up`/`Down`/`Left`/`Right`, `Home`/`End`, +`PageUp`/`PageDown`, `F(1..=12)`, `Ctrl('c')`, `Alt('x')`; chords on any key: +`Key::Right.ctrl()`, `Key::F(5).ctrl().shift()`. + +**Screen** (immutable; every accessor reads one instant): + +| Accessor | Returns | +|---|---| +| `contains(&str)` / `find(&str)` | `bool` / `Option<(row, col)>` — visible grid, NFC-folded | +| `find_by(\|&Cell\| bool)` | `Option<(row, col)>` | +| `cell(row, col)` | `Option<&Cell>`: `contents()`, `style()`, `is_wide()`, `is_wide_continuation()` | +| `row_text(row)` / `text()` / `rect_text(cols, rows)` | `String` | +| `full_text()` / `scrollback_text()` / `scrollback_rows()` | history + screen / history / count | +| `size()` / `cols()` / `rows()` | `(cols, rows)` | +| `cursor()` | `(row, col, visible)`; `cursor_shape()`, `cursor_blink()` | +| `alternate_screen()`, `bracketed_paste()`, `application_cursor()`, `focus_events()` | mode flags | +| `mouse_mode()` / `mouse_modes()` | reporting protocol / the set the app enabled | +| `title()`, `clipboard()`, `links()`, `bells()`, `repaints()`, `graphics()` | out-of-band state | +| `with_styles()` | `Display` with a `styles:` block; snapshot this to catch colour regressions | + +**Style** (`Copy`, public fields): `fg`, `bg` (`Color::Default` / +`Color::Indexed(u8)` / `Color::Rgb(u8, u8, u8)`), `bold`, `dim`, `italic`, +`underline`, `reverse`, `blink`, `conceal`, `strikethrough`. Overline and +double underline are not modelled. + +**Errors** (`termlens::Error`, `#[non_exhaustive]`): `Timeout { waiting_for, +timeout, screen }`, `Eof { waiting_for, screen }`, `Spawn { command, reason }`, +`Size(String)`, `Input(String)`, `Write { what, screen }`, `Emulator { +detail, screen }`, `Pty(String)`, `Io(std::io::Error)`. `err.screen()` returns +the embedded screen when there is one. + +## 8. Pitfalls an agent falls into, and the fix + +| You are about to write | Write instead | +|---|---| +| `std::thread::sleep(Duration::from_millis(500)); let s = t.screen();` | `let s = t.snapshot_after(\|s\| s.contains("…"))?;` | +| `t.wait_until(\|s\| s.contains("title"))?; insta::assert_snapshot!(t.screen());` | `let s = t.snapshot_after(\|s\| s.contains("…last painted…"))?; insta::assert_snapshot!(s);` | +| `t.wait_until(a)?; assert!(t.screen().b);` | `t.wait_until(\|s\| a(s) && b(s))?;` | +| `.size(0, 0)` / `.size(1, 1)` | leave the 80x24 default, or `.size(cols, rows)` with both in 2..=1000 | +| `Terminal::builder().spawn("myapp")` | `termlens::bin!("myapp")?` — a name on `PATH` is not your binary, and under `env_clear` there is no `PATH` | +| `t.click(row, col)` / `t.drag(b, s.find("x").unwrap(), …)` | `t.click(col, row)`; destructure the `find` result and swap | +| `t.send_str("quit\n")` | `t.send_str("quit")?; t.send(Key::Enter)?;` | +| `t.wait_frame(…)` against a ratatui app | `t.snapshot_after(…)` unless the app emits synchronized updates | +| `t.click(3, 4)?` as the first thing after spawn | `t.wait_until(\|s\| s.mouse_mode() != MouseMode::None)?;` first | +| `.timeout(Duration::from_secs(60))` to stop a flake | find the race (rules 2 and 3); use `_for` on the one slow step | +| `insta::assert_snapshot!(t.screen().text())` | `insta::assert_snapshot!(t.screen())` or `.with_styles()` | +| `assert!(t.screen().contains("done"))` after the app printed a lot | `t.screen().full_text().contains("done")` — it scrolled | +| `.unwrap()` everywhere in a `fn test()` | `-> termlens::Result<()>` and `?`, so the failure prints the screen | +| a test that never quits the app | send the quit key, `wait_exit()?`, assert `!alternate_screen()` | +| `INSTA_UPDATE=always cargo test` | `cargo insta review`, and read every diff | + +## 9. Pairing with insta + +- `insta::assert_snapshot!(screen)` — text grid with header. Stable across + runs as long as the application draws nothing volatile. +- `insta::assert_snapshot!(screen.with_styles())` — adds `styles:` runs like + `0: 1-5 fg=6 bold`; use it where a colour or a highlight is the point. +- `insta::assert_snapshot!("name", screen)` — several snapshots in one test. +- Inline snapshots work: `insta::assert_snapshot!(screen, @"")`, then + `cargo insta review` fills the literal. +- `termlens::assert_screen_snapshot!(screen)` is the same call through the + `insta` termlens re-exports, for crates that do not want their own `insta` + dev-dependency. +- Volatile content (a clock, a PID, a spinner) breaks whole-screen + snapshots. insta's text filters are not grid-aware — a shorter replacement + shifts every column after it — so prefer asserting the stable region with + `rect_text(cols, rows)` and the volatile field with `contains`/`find`, + and snapshot the whole screen only when nothing on it moves. +- Snapshot files live in `tests/snapshots/`; commit them. Review every + change with `cargo insta review`; a diff you cannot explain is a bug. + +## 10. A checklist before you finish + +- [ ] No `sleep` anywhere; every wait names what it waits for. +- [ ] Every whole-screen snapshot comes from `snapshot_after` or a wait on the last-painted text. +- [ ] Each test quits the app, asserts the exit status, and asserts `!alternate_screen()`. +- [ ] Coordinates: `(row, col)` from `find`/`cell`, `(col, row)` into `click`/`size`. +- [ ] Environment set explicitly for anything the app reads; `bin!` used for own binaries. +- [ ] Tests return `termlens::Result<()>`; snapshots reviewed with `cargo insta review`. diff --git a/Cargo.lock b/Cargo.lock index 673d705..5d4f88c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1560,9 +1560,9 @@ dependencies = [ [[package]] name = "termlens" -version = "0.7.0" +version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "037018c17834c72ff38b952b5de436b67cb396c43e5c12a839cbf79b0ceb0613" +checksum = "0fca989672430e13284b48504c44b499d88f5b39cb36b0e8dc5b45f06df50b09" dependencies = [ "insta", "libc", diff --git a/Cargo.toml b/Cargo.toml index 5103d91..23b0ac4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,7 +43,7 @@ libc = "0.2" # The end-to-end suite: the real binary, in a real PTY, asserted on the rendered # screen — and, with `decode`, on the pixels of the images that went out over # kitty and sixel, which no rendered screen can show. -termlens = { version = "0.7.0", features = ["decode"] } +termlens = { version = "0.9", features = ["decode"] } [profile.release] lto = true diff --git a/SECURITY.md b/SECURITY.md index cb565a4..0d56e8c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -47,9 +47,27 @@ What the project does continuously, enforced by required CI on every change: ## What has been found and fixed -The 0.1.0 review, in the order the findings mattered. Each has a regression -test named after it in `src/render_tests.rs`. - +The 0.1.0 review, in the order the findings mattered, and what has been found +since. Each has a regression test named after it in `src/render_tests.rs`. + +- **Escape sequences in a `.art` header reached the terminal, the saved plan + and the Action output** (GHSA-jp9f-97rv-j4hx, fixed in 0.7.0). The three + header fields — `# name:`, `# author:`, `# description:` — were printed + exactly as the file wrote them, while every other piece of untrusted text + went through `printable()` where it enters. A `.art` file is the one thing + this project asks strangers to send: a reviewer running + `mossaic-art --matrix theirs.art`, or anyone running `--list-templates`, + executed whatever the header said — a window-title change, an `OSC 52` + clipboard write, or a cursor-position query whose reply is typed back into + the shell after the tool exits. `--no-colour` suppressed none of it. It rode + through `--save` into the plan, through `--format json`'s `headline`, through + `--format markdown` into `$GITHUB_STEP_SUMMARY` and out of the Action's + `headline` output; and `build.rs` embeds `art/templates/*.art`, so a merged + template would have shipped its payload to every user. The header is now + cleaned in the `Canvas` meta reader — where it enters, covering every + downstream printer at once — and bounded to 200 characters, since an + unbounded `# name:` produced a 200,061-byte first output line. + This is the same finding as the calendar-path one below, one file over. - **Escape sequences from a calendar reached the terminal.** A crafted `--file` snapshot, or a hostile API response, could put `ESC` into the login or an error message. The renderer was never the problem — ratatui drops diff --git a/src/art.rs b/src/art.rs index 339ed95..26823e6 100644 --- a/src/art.rs +++ b/src/art.rs @@ -962,16 +962,45 @@ impl Canvas { } } +/// The most a header field may carry. +/// +/// A `# name:` is a title in a listing and the first word of a report line, so +/// a few dozen characters is generous. Unbounded, a 200,000-character name +/// produced a 200,061-byte first output line — and rode through `--save` into +/// the plan and out of the Action's `headline` output. +const MAX_META: usize = 200; + /// Read one `# key: value` header line into `meta`. /// /// Unknown keys are ignored rather than refused. A `.art` file is a document as /// well as data — a contributor may want a `# note:` line — and a format that /// rejects a comment it does not recognise is one that breaks when it grows. +/// +/// **The value is cleaned here**, which is the rule `crate::printable` states: +/// untrusted text is cleaned where it enters rather than at each of the places +/// that print it. A `.art` file is exactly the thing this project asks +/// strangers to send — issue #57 invites it, and CONTRIBUTING §11 makes the +/// review path "open the file with `mossaic-art`" — so a control character in +/// a header reached the reviewer's terminal, the saved plan, the JSON and +/// markdown reports and the Action's `headline` output, unfiltered, on a +/// binary that had already fixed this same bug on the calendar path. +/// `--no-colour` suppressed none of it, because the point of an escape +/// sequence is that it is not displayed. +/// +/// Cleaning here covers every downstream printer at once: the report header, +/// `--list-templates`, both report formats, the saved plan's `art` string and +/// the editor's title. fn read_meta(comment: &str, meta: &mut Meta) { let Some((key, value)) = comment.split_once(':') else { return; }; - let value = value.trim().to_string(); + // `printable` first, then trim: an escape sequence around whitespace + // would otherwise leave the whitespace behind. + let value: String = crate::printable(value.trim()) + .trim() + .chars() + .take(MAX_META) + .collect(); if value.is_empty() { return; } diff --git a/src/render_tests.rs b/src/render_tests.rs index cdca357..7c2bda0 100644 --- a/src/render_tests.rs +++ b/src/render_tests.rs @@ -2449,6 +2449,61 @@ fn control_characters_never_leave_the_parser() { assert_eq!(crate::printable("héllo ✓"), "héllo ✓", "text is left alone"); } +/// GHSA-jp9f-97rv-j4hx. +/// +/// The calendar path was cleaned in the 0.1.0 review; the `.art` header was +/// not, and a `.art` file is the one thing this project asks strangers to +/// send. A reviewer running `mossaic-art --matrix theirs.art` or +/// `--list-templates` executed whatever the header said: a window-title +/// change, an `OSC 52` clipboard write, or a cursor-position query whose +/// reply is typed back into the shell once the tool exits. `build.rs` embeds +/// `art/templates/*.art`, so a merged template shipped its payload to every +/// user on every listing. +#[test] +fn control_characters_never_leave_an_art_header() { + let evil = "\u{1b}]0;PWNED\u{7}\u{1b}[31mEVIL\u{1b}[0m"; + let source = format!( + "# name: {evil}\n# author: {evil}\n# description: {evil}\n\ + 0100010\n0010100\n0001000\n0010100\n0100010\n0000000\n0000000\n" + ); + let canvas = crate::art::Canvas::parse(&source).expect("the body is a valid canvas"); + let meta = canvas.meta(); + for (label, field) in [ + ("name", meta.name.as_deref()), + ("author", meta.author.as_deref()), + ("description", meta.description.as_deref()), + ] { + let field = field.unwrap_or_else(|| panic!("{label} is read")); + assert!( + !field.chars().any(char::is_control), + "{label} still carries control characters: {field:?}" + ); + assert!( + field.contains("PWNED"), + "the text itself is harmless: {field:?}" + ); + } + + // A bare carriage return is enough to overwrite what was shown, and does + // not look like an escape sequence to anyone reading the file. + let source = "# name: SAFE\rEVIL\n0100010\n0010100\n0001000\n0010100\n\ + 0100010\n0000000\n0000000\n"; + let canvas = crate::art::Canvas::parse(source).unwrap(); + assert_eq!(canvas.meta().name.as_deref(), Some("SAFEEVIL")); + + // And a header is bounded: unbounded, a 200,000-character name produced a + // 200,061-byte first output line and rode through `--save` into the plan. + let long = format!( + "# name: {}\n0100010\n0010100\n0001000\n0010100\n0100010\n0000000\n0000000\n", + "A".repeat(200_000) + ); + let canvas = crate::art::Canvas::parse(&long).unwrap(); + assert!( + canvas.meta().name.as_deref().unwrap().chars().count() <= 200, + "a header field must be bounded" + ); +} + #[test] fn a_calendar_cannot_span_more_than_a_year() { // Two dates millennia apart used to size the grid by the distance between From 0507111eaa085c482411fd1f403750b823bb14c2 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:17:33 +0700 Subject: [PATCH 2/9] fix: give the terminal back on a signal, and stop panicking on a closed pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the four ways out of a TUI had no guard at all. **Signals.** Under a pty, `kill -INT`, `-TERM` or `-HUP` on either binary emitted *zero bytes* to the tty: the shell was left inside the alternate screen with mouse tracking on, ECHO, ICANON and ISIG off, and no working Ctrl-C. The cure is to type `reset` blind, which a first-time user does not know, and nothing on screen says so — the failure is in what was not written. Whoever reaches for a signal is already having a bad time: the chart is sitting on "loading" because `gh` is slow, so they `kill %1` from another pane, or a script signalled the process group. The project had already decided this state was unacceptable — `main` installs a panic hook and says why in as many words — and `mossaic-art --draw`, which also takes the alternate screen and also enables mouse reporting, had neither guard. Both now share one `restore` module. `signal-hook` rather than an `unsafe_code` exception: `sigaction` is an unsafe call and `SECURITY.md` advertises the forbid as posture. The crate registers safely, does its work on a thread rather than in a handler (a real handler may call only async-signal-safe functions, and writing escapes through Rust's stdout is not one), and re-raises with the default disposition so the exit still reports as death-by-signal. It is also already in the tree — crossterm pulls it through ratatui at the same version — so this is an import, not a dependency. **Broken pipes.** `mossaic-art --font | less` and quitting, `--list-templates | head`, `--format json | jq -e` with a jq that exits early: all ordinary, all a panic at exit 101, over a message nobody was listening to, with a backtrace note that reads like a crash in mossaic. Which commands escaped was a pipe-buffer accident — the coloured glyph sheet is 53 KB and always went, the uncoloured one is 9 KB and survived on Linux but not macOS — so it read as a flake. `--png` is the one with a price: a valid, complete PNG is already on disk and the caller was handed 101, so a wrapper that checks the status deletes the file and retries. That is why this is a write-side hook rather than restoring the default SIGPIPE disposition, which would give the wrong answer there — and which `unsafe_code = "forbid"` bars anyway. A hook rather than a rewrite of 141 `println!` sites, because this is a property of every printing path and a hook cannot be forgotten at the site somebody adds next. Tests: the three signals against the editor in a real pty, asserting the emulated terminal state the shell would inherit rather than the bytes (the emulator consumes them) — confirmed red with the guard removed; and the writer's *own* status through a reader that closes first, which is the thing a pipeline hides. Nothing in CI could see either: `install.yml` only pipes into `tee`, which never exits early, and Actions' `bash -e` has no `pipefail`. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- - | Bin 0 -> 5845 bytes Cargo.lock | 85 ++++++++++++------------- Cargo.toml | 1 + src/bin/mossaic-art.rs | 6 ++ src/bin/mossaic-glyphs.rs | 2 + src/lib.rs | 91 +++++++++++++++++++++++++++ src/main.rs | 14 +---- src/restore.rs | 129 ++++++++++++++++++++++++++++++++++++++ tests/canvas_pty.rs | 79 +++++++++++++++++++++++ tests/chart_cli.rs | 99 +++++++++++++++++++++++++++++ 10 files changed, 453 insertions(+), 53 deletions(-) create mode 100644 - create mode 100644 src/restore.rs diff --git a/- b/- new file mode 100644 index 0000000000000000000000000000000000000000..046e5a09144913785130996d4b94482bba4fd080 GIT binary patch literal 5845 zcmd^Dc~p{V`!>sEDza>G915qgY_Xy=!3}NIY_V0`Ce$=_05dfeB)2Th3~4g6P_weh z3KftrQ*ap<#tPI)1=Mgs#l#H}ckFx7={NJu?>lGy{m%LRdCzms`#$%3KhJYr&wX8Y z;weXa(9-Xgs;Q}g9F811qo%fG6EME5tqwem$A)=oYU?r_4(&gO%NWe8K(BYujn}Z8 z+^c2A-uaW>a)vbN;ZUpvGs02qdb`dJN7C#JFsv$#H>l7v_+WiIu=v~Y8!&VLsprw@ zaO6d&Lv~eKU-m>_L>}_-iA2BZ?<+|;dwt~r?Ty!0es|u!*La_5bU^z?qd6@oc>3}j zzs%s?@|o=Lxq${@7(R^Mk~4TFo@h0Ed3a!KhHtAJ8Bgme*+ST~*yq9WuxPe$^jPj| zmJS4(%uf<0vE86PE>`qct_)7`D2|Q97$Ge&HSYbHnU_S?k#n6rJ%deyXI#e=iHp>tUVI#OR|$hzha?&7uAUD#g~oc1~VDU)0= zNWxFwSypwF(;7dFPtu(N8JCf;$x=D%?FP zF*W(k%tI#2>--M4n@(IZqB)f#WxAQte|I$%1rPTOI$2abv&!uPp|n+2 zsi#+sDZVUE1nRhy`tc(oT12z#<5-M8VW%q$SETTUp$tX&Dx9`JjI5GHf3% zmZ%G*H2}jZo+^8F@F0IJyq=DU1HTiXJP!g9?*ZS4P@aZisqFKmU{(AW+t>6bY-BWBi^J?Pf!m;+zfp4Mo(}v)8oP~Vhvae)%en5 zZ@wn{9-Unt4@BGwe8Xm!+YQX>8UqP8M4b;>yrBeE^yK!S222$(QBTkZ3@=F1Ts{NI zj7x8U`vlAi&_yhi_5_G{^dGl?8BZK6;Os9tUwF#JmZkh5qsSi}bYFE@C$#X!1LMQcz(;rmtYsrdjd%RI6_D6E3HG$Y(1F~ zOI(A|gtd@N6ZEI5k0B;9^#p3L=OgOi@|q$5=hb8YV&V^8c2Tl3b&(PcSPXF+t6Y<| zCtpu+gzgJ4$UNl1&mOGCkam15$kTf%l(qdfIrEhSD{1KEe$+GYNWBxkJf)&hzB{mR4$CBsR^gWjCaoumXWsf`HJ2FAAvo6( zM;VPa0clpkyexPt8N)i$b*)j0$ynX*?PN;jESzj(h?4b|m;HB4n=0QbBJXYDiN9i5 zdr({OM~yo1gXiM;VzqH^Vhm^j)%BYSqW87+SCBhJ#afNlHp{Ex&eGY)AYN*_@bd&U zdb)EE`ab;R_z}(DXy57-wv0)x>g2d7Bv|Ot&h^-`z=A1&a%na6&t!87E8I&XrGH-f<{{@VQix;-rYVaNdHhbuDUMVYff%<` z;yE$IB%9TwiP|faB_1MFYGwR#$j%D8p!-?zguOzk)3Pb%tyKH@jKZge9DwVr<3ng_ z9t<6k>l9r`P7B#RjvB$SA%x7igj5f* zgUYcT{x>3VMjvI^?9jQt9P+*dFCVMsIB$NovC?P6MebIO2b7em$blbVe!7LJmjooG%=ve5b;e7oP!O3DGECg@8~YRzwVhPRmu5M1l&pO&j)YGm+F*j9g)|S zVO+*YhIemCS`coQzdyK0rCd#hXm(th#J|`YF|)*$MoN7sZW$!3lp^eeU^^FN7bxYfK9zH8w|;rV{E%& zTyULFS=WpW{rJ5tC=KqaKY-xjLeY1D5Db0nCW-^arD;r+X%ovVsPGjiVxSSY^@kS$ zs%uCCHsW|Xr7_+yBNmiNUIL2&c^anc3CaoOfTRMX8S8B0CgzjXex3(&jDRw-Snyjf zFa-jXp&nI?HD(UL>huNmFCBn)YOn&J*08hy3cdwmSRFob25cFm3A=*U1>VIFvHz97 zomhq)c^k@_z||GLhooOF!Fq9#a8R8sYe!&#iGYwH>tEOcQ^65r3X=Zna~-7a6a|mz z!a9M+1TWcHUg8RIwe0zax30f+l&lsL2QzI6SE9~pD&fZZZNHLFIvU)E@6fltA1?3x zoJ67?waf+Ti%5LiImhTFDAU{>&uDrENc?FA$FvbGBAdH6X(GbV!wvAc(1{d|O|7~D9g5-!C*hFmVDQiJF~6g$6>`z08tdK3#9OBCw>;<(G!@wxf&>u4c=Q65T%UzhiE#*{s$HWK~}zrcnoW;OtQXyJuRU%c*uh? z>nnAJ80>m8IxV%Bf~=Gre-e1SAxq2ux^uf5d;4o&E)qqe+E=xfrt!x=;~4Dwd)&)L zCtH3rrg9#o3^u*V*fvJoHZ9*x964R~w!yw0-Hh$Yw3;1REgENEy>o*&SpBi|bH?gdki30@S5ywqWBYc|7g;D+VFyH(~wpj;~0Isv$f zVGrIKl^glNv+~60;IlU;Y@k|kfcU@pi5f!;22}stT+i_am0S4V3Goz4^BQ|OE-^se z4~){EJwdx0*(?$(x|(E<0@4sG;s@{k<9<&s2df-3{37GhI+MwvEOU!Um#7Mp$^5UB z2rC6B5zaQl6pl7my_*lt%R*R|ZJV)gUQ?W z^QF`Gb&6gOGvA%&vgAGnS2MQE^np>C>rXpdqA2G4v0@8@TLGnCKbJsk`@?H;KBq4v zLgrq^6P%*d-gm4EKF6He(5(5FnU)kYKRNQ#J_hLk=N($djcawhJRYpF46l)3Txjs2 z^YfNAqzbf`t`5UWHW+zU5>T0G64x6~G0nq-$n;6nL|)wIqUm8ZyS`uZ2Z zkgN|{d?Ggn)QV5|MsOK0#DHo;FQz&szgY0o$wje7$ny9>R^<|G5kRqWI}9_{d5eAH zyq~wVZ`-%p*t0Thp1nAr7G*PA$|)&^{qy~VEW=BW6W7wbtcG_l0$)-Yp$4~rj;7jG zq4Q?GZWX+~YN>#h8U7#!1Xp<;|2)`8fA`Je`DTLTCfNUi23ElPr)58f(mF?0%Y-bX z#jA&Jd`+zZl{Kq&zhYCDgYrLzA~C$)yy942gBz-oeoYR4U{hH$E6kRs zn>=Q@0xg>8uKBuwtxmtOyvFLoJO)(7B2ZKRD*vm@%Ao?x0K)f1AeuEr;{b}Bh5*rh zg353Ufd!t(5H4UNuXn@q>=KK0$M0qRG7k9g+97@@wIs`J{3oTvxW;{G9y4EUiY2$$ z)MbtszM!qFBK`8Z{U;UEK>B&ZhW)RUN_W0A7j2mS#)vnNEua^)#3AJ?>KnF3Yb>~v z>tTPI?!UnPtE2JPjl2OO=@hx(8HgkIxW_VThbO0)1Z_co#TC}i(uX!L*BXBj zcJCs~=}2Tab*Ny}mvGn(1`RSQJKN3gS3E~j+o~zz9_w3~+22u{Fg_L~-c2BV0z<_!rjkmq|m#ZOPtcFk7CHz<6Q z0He}H%xr((N>O#Nyt#xhc7IT15c_HVRs6zCcg6&wH_c3%i&vq9e=LNQ>A|AQmti|H ze|?W%V`UF?e*leiO)IHa6qO;I#ToXfWX_E=`PQ)H7^1-J8|)IDbzHX6oG~S{dk3Y( zhA)LmP7kbK1g2wj1=^4uWWr34uUt&L=b;i$C|m=07yZdB`Qf&qS#t+5AGa1vk&9l-rGXHng>pOw$blpUtX+ zaXBNwkZRs4+Nu(xSYk)T)3SOK+5awWem4P^Noe() + .map(String::as_str) + .or_else(|| info.payload().downcast_ref::<&str>().copied()) + .unwrap_or_default(); + let printing = payload.starts_with("failed printing to stdout") + || payload.starts_with("failed printing to stderr"); + if printing && payload.contains("Broken pipe") { + // The reader got what it wanted and hung up. Nothing is left to + // say and nothing went wrong. + std::process::exit(0); + } + previous(info); + })); +} + +/// Print `text` to stdout, treating a reader that has gone as a clean exit. +/// +/// Rust's runtime sets `SIGPIPE` to `SIG_IGN`, so `println!` into a closed +/// pipe panics and the process exits **101** — a code outside the set these +/// binaries document, over a message nobody was listening to, with a +/// backtrace note that reads like a crash in mossaic rather than the user +/// closing a pager. `mossaic-art --font | less` and quitting, +/// `--list-templates | head`, `--format json | jq -e …` with a jq that exits +/// early: all ordinary, all a panic. +/// +/// Which commands escaped was a pipe-buffer accident — the coloured glyph +/// sheet is 53 KB and always panicked, the uncoloured one is 9 KB and +/// survived on Linux but not macOS — so it read as a flake. +/// +/// The `--png` case is the one with a price: a valid, complete PNG is +/// already on disk and the caller was handed 101, so a wrapper that checks +/// the status deletes the file and retries. Exiting **0** there is the +/// correct answer, and is why this is a write-side check rather than +/// restoring the default `SIGPIPE` disposition — which `unsafe_code = +/// "forbid"` bars anyway. +/// +/// Any other write failure is a real one and goes through the caller's own +/// `fail`. +pub fn print_or_exit(text: &str) { + use std::io::Write; + let mut out = std::io::stdout(); + match out.write_all(text.as_bytes()).and_then(|()| out.flush()) { + Ok(()) => {} + // `| head -1`, `| grep -m1`, a pager the user quit: the reader got + // what it wanted and hung up. There is nothing left to say and + // nothing went wrong. + Err(e) if e.kind() == std::io::ErrorKind::BrokenPipe => std::process::exit(0), + Err(e) => { + eprintln!("mossaic: cannot write to stdout: {e}"); + std::process::exit(2); + } + } +} + +/// [`print_or_exit`] with a trailing newline, for the `println!` shape. +pub fn println_or_exit(text: &str) { + print_or_exit(&format!("{text}\n")); +} diff --git a/src/main.rs b/src/main.rs index e4022e5..98a7c69 100644 --- a/src/main.rs +++ b/src/main.rs @@ -88,6 +88,8 @@ struct Invocation { } fn main() { + // Before anything prints: a reader that closes early is not a crash. + mossaic::quiet_broken_pipe(); let Some(invocation) = parse_args() else { return; }; @@ -129,7 +131,7 @@ fn main() { // screen where the first frame paints over it. let mut app = App::new(login, year, source); app.configure(term::probe(PROBE), options); - restore_mouse_on_panic(); + mossaic::restore::guard_terminal(); let outcome = run(&mut terminal, &mut app); let restored = ratatui::try_restore(); @@ -264,16 +266,6 @@ fn report_capabilities(options: Options) { println!("cells {}", app.protocol_name()); } -/// A panic that unwinds past the event loop would otherwise leave mouse reporting -/// on, and the shell printing escape codes at every click. -fn restore_mouse_on_panic() { - let previous = std::panic::take_hook(); - std::panic::set_hook(Box::new(move |info| { - let _ = execute!(io::stdout(), DisableMouseCapture); - previous(info); - })); -} - /// Returns `None` when there is nothing left to run, e.g. after printing help. fn parse_args() -> Option { let mut login = None; diff --git a/src/restore.rs b/src/restore.rs new file mode 100644 index 0000000..2dc97d9 --- /dev/null +++ b/src/restore.rs @@ -0,0 +1,129 @@ +//! Putting the terminal back, on every way out. +//! +//! A TUI borrows the terminal: the alternate screen, raw mode, mouse +//! reporting. The borrow has to be returned on **every** exit, and there are +//! four — the ordinary one, a panic, a signal, and a write that fails +//! because the reader has gone. +//! +//! Before 0.7.0 two of the four were covered. `main` restored on the way out +//! and installed a panic hook that says why in as many words: "A panic that +//! unwinds past the event loop would otherwise leave mouse reporting on, and +//! the shell printing escape codes at every click." A **signal** reached the +//! same state by a path with no guard at all: under a pty, `kill -INT` on +//! either binary emitted zero bytes to the tty, leaving the shell inside the +//! alternate screen with mouse tracking on, ECHO, ICANON and ISIG off, and +//! no working Ctrl-C. The way out is to type `reset` blind, which a +//! first-time user does not know, and nothing on screen says so — the +//! failure is in what was *not* written. +//! +//! Whoever reaches for a signal is already having a bad time: the chart is +//! sitting on "loading" because `gh` is slow, so they `kill %1` from another +//! pane, or they wrapped it in `timeout`, or a script signalled the process +//! group. +//! +//! ## Why `signal-hook` rather than `libc::sigaction` +//! +//! `Cargo.toml` is `unsafe_code = "forbid"`, and `SECURITY.md` advertises +//! that posture ("the one libc dependency is used for a single constant, not +//! a call"). `sigaction` is an unsafe call, so the obvious route is barred. +//! +//! `signal-hook` registers safely and — the part that matters — is *already +//! in the tree*: crossterm pulls it through ratatui for its event stream, at +//! the same version pinned here. So this adds an import, not a dependency: +//! no new third-party code, no new licence to review, no new supply chain. +//! +//! The work happens on a thread rather than in a handler, which is what +//! makes it safe to do anything at all: a real signal handler may call only +//! async-signal-safe functions, and writing escape sequences through Rust's +//! stdout is not one of them. + +use std::io; + +use ratatui::crossterm::cursor::Show; +use ratatui::crossterm::event::{DisableMouseCapture, EnableMouseCapture}; +use ratatui::crossterm::execute; + +/// Give the terminal back: mouse reporting off, alternate screen off, raw +/// mode off, cursor shown. +/// +/// Best-effort by construction. This runs on the way out of a process that +/// may already be failing, and on a terminal that may already have gone; an +/// error here must not mask the reason we are leaving. +pub fn terminal() { + // Mouse first: `try_restore` leaves the alternate screen, and a mouse + // report arriving after that lands on the shell's own screen. + let _ = execute!(io::stdout(), DisableMouseCapture); + let _ = ratatui::try_restore(); + // Explicitly, and last. Leaving the alternate screen restores that + // screen's cursor state, which is not necessarily the shell's: a hidden + // cursor is the one piece of this a user cannot see is missing, and + // cannot guess the cure for. + let _ = execute!(io::stdout(), Show); +} + +/// Restore the terminal on a panic, then let the previous hook run. +/// +/// Kept beside [`on_signal`] because they are the same obligation: the two +/// exits nobody writes code for. +pub fn on_panic() { + let previous = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + terminal(); + previous(info); + })); +} + +/// Restore the terminal on `SIGINT`, `SIGTERM` and `SIGHUP`, then die of the +/// signal. +/// +/// Re-raising with the default disposition matters: a process killed by a +/// signal must still *report* as killed by that signal, or a shell's `$?`, a +/// `timeout` wrapper and a supervisor all learn the wrong thing about why it +/// stopped. `emulate_default_handler` does exactly that. +/// +/// Ctrl-C *typed into* a TUI is a key, handled by the event loop, and does +/// not come through here — the damage always needed an actual signal. +/// +/// Failing to register is not worth failing the run over: the terminal is +/// no worse off than it was before 0.7.0, and the user asked to draw a +/// chart, not to install a signal handler. +pub fn on_signal() { + use signal_hook::consts::{SIGHUP, SIGINT, SIGTERM}; + let Ok(mut signals) = signal_hook::iterator::Signals::new([SIGINT, SIGTERM, SIGHUP]) else { + return; + }; + std::thread::spawn(move || { + for signal in signals.forever() { + terminal(); + // Unregisters our handler and re-raises, so the exit status is + // death-by-signal rather than a plain code. + let _ = signal_hook::low_level::emulate_default_handler(signal); + } + }); +} + +/// Everything a TUI owes the terminal, installed in one call. +/// +/// Both binaries take the alternate screen and both enable mouse reporting, +/// so both need all three guards. `mossaic-art --draw` had none of them. +pub fn guard_terminal() { + on_panic(); + on_signal(); +} + +/// Turn mouse reporting on, for a view that reads the pointer. +/// +/// Here rather than at the call sites so that the enable and the disable are +/// written next to each other and cannot drift apart. +/// +/// # Errors +/// +/// Propagates the write to stdout. +pub fn capture_mouse(on: bool) -> io::Result<()> { + let mut out = io::stdout(); + if on { + execute!(out, EnableMouseCapture) + } else { + execute!(out, DisableMouseCapture) + } +} diff --git a/tests/canvas_pty.rs b/tests/canvas_pty.rs index c01ee4f..62659e5 100644 --- a/tests/canvas_pty.rs +++ b/tests/canvas_pty.rs @@ -398,3 +398,82 @@ fn a_dark_day_inside_the_picture_still_says_stay_dark() -> termlens::Result<()> let _ = std::fs::remove_file(&art); Ok(()) } + +// ------------------------------------------------------- signals + +/// A signal must hand the terminal back before it kills the process. +/// +/// This is the out-of-process behaviour a PTY harness exists for: what is +/// wrong is *what was not written to the tty*, so nothing in-process can see +/// it. Before 0.7.0, `kill -INT` on either binary emitted **zero bytes** +/// after the signal — leaving the shell inside the alternate screen with +/// mouse tracking on, ECHO/ICANON/ISIG off, and no working Ctrl-C. The cure +/// is to type `reset` blind, which a first-time user does not know, and +/// nothing on screen says so. +/// +/// The project had already decided this state was unacceptable: `main` +/// installs a panic hook and says why. A signal reached the same state by a +/// path with no guard at all, and `mossaic-art --draw` had neither guard. +/// +/// Ctrl-C *typed into* the editor is a key, handled by the event loop; the +/// damage always needed an actual signal, which is why `send(Key::Ctrl('c'))` +/// would not reproduce it. +fn a_signal_gives_the_terminal_back( + signal: termlens::Signal, + expect: &str, +) -> termlens::Result<()> { + let out = scratch("sig.art"); + let mut terminal = spawn(&["--draw", "--year", "2027", "-o", out.to_str().unwrap()])?; + // Sync on the editor having painted: signalling a process that has not + // yet taken the terminal proves nothing about giving it back. + terminal.wait_frame(ready)?; + + terminal.signal(signal)?; + let status = terminal.wait_exit()?; + + // Still death-by-signal. A process that swallowed the signal and exited + // 0 would teach a shell's `$?`, a `timeout` wrapper and a supervisor the + // wrong thing about why it stopped. + let named = status + .signal() + .unwrap_or_else(|| panic!("must still die of the signal, got {status}")); + assert!( + named.contains(expect), + "expected {expect}, got {named} ({status})" + ); + + // And the terminal is back. Asserted on the emulated *state* rather than + // on the bytes: the emulator consumes the escape sequences, and the + // state they leave behind is what the user's shell would inherit — which + // is the thing that was wrong. Before the fix, zero bytes reached the tty + // after the signal, so all three of these stayed as the editor left them. + let screen = terminal.screen(); + assert!( + !screen.alternate_screen(), + "{expect} left the shell inside the alternate screen" + ); + assert!( + screen.mouse_modes().is_empty(), + "{expect} left mouse reporting on: {:?}", + screen.mouse_modes() + ); + let (_, _, visible) = screen.cursor(); + assert!(visible, "{expect} left the cursor hidden"); + let _ = std::fs::remove_file(&out); + Ok(()) +} + +#[test] +fn sigterm_gives_the_terminal_back() -> termlens::Result<()> { + a_signal_gives_the_terminal_back(termlens::Signal::Term, "Terminated") +} + +#[test] +fn sigint_gives_the_terminal_back() -> termlens::Result<()> { + a_signal_gives_the_terminal_back(termlens::Signal::Int, "Interrupt") +} + +#[test] +fn sighup_gives_the_terminal_back() -> termlens::Result<()> { + a_signal_gives_the_terminal_back(termlens::Signal::Hup, "Hangup") +} diff --git a/tests/chart_cli.rs b/tests/chart_cli.rs index d8ee8fa..3253273 100644 --- a/tests/chart_cli.rs +++ b/tests/chart_cli.rs @@ -66,3 +66,102 @@ fn png_needs_no_terminal() { assert!(path.is_file(), "it wrote {}", path.display()); let _ = std::fs::remove_file(&path); } + +/// A reader that closes first is not a crash. +/// +/// Rust sets `SIGPIPE` to `SIG_IGN`, so `println!` into a closed pipe +/// panicked and the process exited **101** — outside the set these binaries +/// document — with a backtrace note that reads like a crash in mossaic +/// rather than the user quitting a pager. Which commands escaped was a +/// pipe-buffer accident, so it read as a flake: the coloured glyph sheet is +/// 53 KB and always went, the uncoloured one is 9 KB and survived on Linux +/// but not on macOS. +/// +/// Nothing in CI could see it: `install.yml` only ever pipes into `tee`, +/// which never exits early, and Actions' `bash -e` has no `pipefail`, so a +/// panicking writer left the step green. This asserts the **writer's own** +/// status, which is the thing a pipeline hides. +#[test] +fn a_reader_that_closes_early_is_not_a_crash() { + use std::process::Stdio; + + // One case per binary, plus the two the report named by hand. + let cases: [(&str, &[&str]); 5] = [ + ( + env!("CARGO_BIN_EXE_mossaic-art"), + &["--font", "--color", "always"], + ), + (env!("CARGO_BIN_EXE_mossaic-art"), &["--list-templates"]), + (env!("CARGO_BIN_EXE_mossaic-glyphs"), &["--no-colour"]), + (env!("CARGO_BIN_EXE_mossaic"), &["--capabilities"]), + ( + env!("CARGO_BIN_EXE_mossaic"), + &["--demo", "--graphics", "text", "--png", "-"], + ), + ]; + + for (binary, args) in cases { + let mut child = std::process::Command::new(binary) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap_or_else(|e| panic!("{binary} runs: {e}")); + // Close the read end while the writer is still going. + drop(child.stdout.take()); + let out = child.wait_with_output().expect("wait"); + let text = String::from_utf8_lossy(&out.stderr); + assert_ne!( + out.status.code(), + Some(101), + "{binary} {args:?}: a closed reader must not be a panic\n{text}" + ); + assert!( + !text.contains("panicked"), + "{binary} {args:?}: no panic text\n{text}" + ); + } +} + +/// `--png` writes the file, so the status is 0 even when the reader has gone. +/// +/// The opposite failure to closed #29 ("--png writes an invalid zero-width +/// PNG and reports success"): here a valid, complete PNG is on disk and the +/// caller was handed 101, so a wrapper that checks the status deletes the +/// file and retries. It is also why this is a write-side fix rather than +/// restoring the default `SIGPIPE` disposition, which would give the wrong +/// answer here. +#[test] +fn a_written_png_reports_success_even_when_the_reader_has_gone() { + use std::process::Stdio; + + let png = std::env::temp_dir().join(format!("mossaic-bp-{}.png", std::process::id())); + let _ = std::fs::remove_file(&png); + let mut child = std::process::Command::new(env!("CARGO_BIN_EXE_mossaic")) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .args(["--demo", "--png", png.to_str().unwrap()]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("the chart binary runs"); + drop(child.stdout.take()); + let out = child.wait_with_output().expect("wait"); + + assert_eq!( + out.status.code(), + Some(0), + "the file is written, so the status is success\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let bytes = std::fs::read(&png).expect("the PNG is on disk"); + assert!(bytes.starts_with(b"\x89PNG\r\n\x1a\n"), "and it is a PNG"); + assert!( + bytes.len() > 1000, + "and a complete one: {} bytes", + bytes.len() + ); + let _ = std::fs::remove_file(&png); +} From 6b24199feb249f2949e2cbe9bdb0120d74daa09e Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:24:23 +0700 Subject: [PATCH 3/9] fix(cli): refuse a flag that does nothing, accept the text that could not be typed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings in the argument surface, all of them the tool accepting something and then doing other than what was asked. **Flags honoured in one mode and ignored in the rest.** `--png` was taken in every mode and wrote the file in exactly one; `-o` without `--draw` and `--format` without `--track` were the same shape. #26 settled the principle — "They are side effects and inputs the user explicitly asked for … Either honour them or refuse the combination" — and enumerated three flags, all refused; these were missed. Refused rather than honoured, deliberately: honouring `--png` would mean deciding what a PNG of a preview, a template list, a tracking report and a backfill each *are*, and `--snapshot` plus `mossaic --file --png` already covers the one people want. The cost of the silence was a script's: `--template dragon --png preview.png` printed a cheerful report, exited 0 and produced nothing. **The hyphen the font draws could not start a text.** `-` is listed three times — README's punctuation line, `action.yml`'s `text:` description, and the binary's own no-glyph message — and `mossaic-art -` was `unknown option "-"`. Reaching for `--` gave `unknown option "--"`, so the escape hatch named itself as the mistake, and no quoting helped; the only route through was a hand-written plan JSON that nothing documents. You reach for `mossaic-art -- "$TEXT"` precisely when the text is not yours to control, which is what the Action does. `--` now means the rest is positional, in the shared parser, so a dash-led login works in the chart too — and an argument that could have been the text names `--` in its error instead of just "try --help". **`--track --save --format json` wrote prose into a machine document.** The `saved …` confirmation was the single `println!` that could run ahead of the document, at exit 0 with an empty stderr, so there was no signal anywhere — and it bit only on the run that writes the plan, so it read as a flake on first use. It is on stderr now, where every other note a track run makes already goes, and still shown in all three formats. **`--help` priced the wrong thing.** "commits per lit day" applied to the shipped dragon predicts 584 where the tool prints 442 — a 32% overestimate from following the help exactly. `--commits` prices the *brightest* day; docs/ART.md and the source comment both say so. The help is the only description a `cargo install` user gets, since `docs/*` is excluded from the published archive and README never mentions the flag. The test checks the help line against a real report's own arithmetic. And the three plural sites 0.6.3's pass missed — the two preview headers and the editor's per-level row, which read "1 days · 1 commits" on the first line a new user sees — plus the markdown `holed` sentence it reworded into nonsense: the verb moved in front of "inside the letters", so "already lit" became a clause on *the letters* rather than the days. Only the path the Action publishes regressed; the text renderer still read correctly. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- src/bin/mossaic-art.rs | 106 +++++++++++++-- src/cli.rs | 45 ++++++- src/draw.rs | 12 +- src/main.rs | 28 +++- src/plan.rs | 14 +- tests/art_cli.rs | 285 ++++++++++++++++++++++++++++++++++++++++- 6 files changed, 469 insertions(+), 21 deletions(-) diff --git a/src/bin/mossaic-art.rs b/src/bin/mossaic-art.rs index 2babcc7..38b9fe4 100644 --- a/src/bin/mossaic-art.rs +++ b/src/bin/mossaic-art.rs @@ -58,7 +58,8 @@ WHAT TO DRAW WHERE IT GOES --year YEAR which year's calendar (default: this one) --start-week N left edge, in weeks (default: centred) - --commits N commits per lit day (default 4) + --commits N commits for the brightest day; darker shades are priced + against it (default 4) --top ROW first calendar row, 0 = Sunday (default 1, so Mon-Fri). Text only; a picture uses all seven rows --background LEVEL (--bg) draw the background as a shade 0-3 rather than @@ -232,7 +233,15 @@ fn main() { }; spec.save(&options.plan_path) .unwrap_or_else(|error| fail(&error)); - println!( + // stderr, not stdout. Under `--format json` or `--format markdown` + // stdout is a *document*: this was its first line, so + // `--track --save --format json | jq .` handed a parser two lines of + // prose and still exited 0, with an empty stderr — no signal + // anywhere. Every other note a track run makes already goes here; + // this was the single `println!` that could run ahead of the + // document. It bit only on the run that writes the plan, so it read + // as a flake on first use. + eprintln!( "saved {} — from now on:\n mossaic-art --track\n", options.plan_path.display() ); @@ -261,13 +270,15 @@ fn main() { let art_level = art::level(options.commits, peak); let total = placed.total(); println!( - "{} · {} · {} of {} columns · {} days · {} commits\n", + "{} · {} · {} of {} columns · {} {} · {} {}\n", options.text.to_uppercase(), grid.year, columns.len(), grid.weeks, placed.lit.len(), + plural(placed.lit.len(), "day", "days"), thousands(total), + plural(total, "commit", "commits"), ); let palette = options @@ -616,7 +627,15 @@ fn run_canvas(options: &Options, grid: &Grid, name: &str, canvas: &art::Canvas) }; spec.save(&options.plan_path) .unwrap_or_else(|error| fail(&error)); - println!( + // stderr, not stdout. Under `--format json` or `--format markdown` + // stdout is a *document*: this was its first line, so + // `--track --save --format json | jq .` handed a parser two lines of + // prose and still exited 0, with an empty stderr — no signal + // anywhere. Every other note a track run makes already goes here; + // this was the single `println!` that could run ahead of the + // document. It bit only on the run that writes the plan, so it read + // as a flake on first use. + eprintln!( "saved {} — from now on:\n mossaic-art --track\n", options.plan_path.display() ); @@ -658,12 +677,14 @@ fn run_canvas(options: &Options, grid: &Grid, name: &str, canvas: &art::Canvas) let histogram = canvas.histogram(); println!( - "{name} · {} · {} of {} columns · {} days · {} commits\n", + "{name} · {} · {} of {} columns · {} {} · {} {}\n", grid.year, canvas.width(), grid.weeks, commits.len(), + plural(commits.len(), "day", "days"), thousands(total), + plural(total, "commit", "commits"), ); let palette = options @@ -1782,6 +1803,24 @@ fn parse_args() -> Option { let mut args = Args::from_env("mossaic-art"); while let Some(arg) = args.next_arg() { + // Past a bare `--`, every argument is the text — checked before the + // flag names, or `-- --year` would still match the `--year` arm and + // `--` would mean "only the next one", which is nobody's convention. + if args.past_end_of_options() { + if options.text.is_empty() { + options.text = art::canonical(&arg).unwrap_or_else(|error| fail(&error)); + } else { + // The trap `--` sets for a first-time user: it means + // *everything* after it, as it does in `ls` and `git`, so + // options have to come first. Say that rather than leaving + // them to work it out. + fail(&format!( + "unexpected argument {arg:?} — everything after -- is the text, \ + so put the options before it" + )); + } + continue; + } match arg.as_str() { "-h" | "--help" => { println!("{HELP}"); @@ -1877,8 +1916,17 @@ fn parse_args() -> Option { tracking = args.next_arg(); } } - other if other.starts_with('-') => { - fail(&format!("unknown option {other:?} — try --help")) + // `is_positional` rather than a bare `starts_with('-')`: after a + // bare `--`, an argument that looks like a flag is text. The + // hyphen is a glyph the font draws and three documents list, and + // it could not be typed in the position they put it in. + other if !args.is_positional(other) => { + let hint = if options.text.is_empty() { + " — if that is the text, write it after --" + } else { + " — try --help" + }; + fail(&format!("unknown option {other:?}{hint}")) } other if options.text.is_empty() => { // Expanded here rather than at every use: the plan is saved @@ -1890,6 +1938,49 @@ fn parse_args() -> Option { } } + // The same rule, for the three flags that named a companion and were + // then ignored without it. #26 settled the principle in the maintainer's + // own words — "They are side effects and inputs the user explicitly + // asked for … Either honour them or refuse the combination" — and + // enumerated `--snapshot`, `--write` and `--file`, all three of which + // are refused above. These were missed. + // + // Refusing rather than honouring, deliberately: honouring `--png` would + // mean deciding what a PNG of a preview, a template list, a tracking + // report and a backfill each *are*, and `--snapshot` plus + // `mossaic --file --png` already covers the one people want. Refusing + // keeps that door open. + // + // The cost of the silence was the shape a script cannot see: + // `mossaic-art --template dragon --png preview.png` in a workflow printed + // a cheerful report, exited 0 and produced nothing, so the next step read + // a file that was not there — or, in a job that regenerates art, + // republished the previous run's stale PNG. + for (flag, ignored, needs, what) in [ + ( + "--png", + png_path.is_some() && !font, + "--font", + "writes the glyph sheet", + ), + ( + "-o", + options.output.is_some() && !options.draw, + "--draw", + "names the file the editor saves to", + ), + ( + "--format", + args.was_typed("--format") && !track, + "--track", + "chooses how the tracking report is written", + ), + ] { + if ignored { + fail(&format!("{flag} {what} — it needs {needs}")); + } + } + if list_templates { show_templates(options.colour.enabled()); return None; @@ -1957,7 +2048,6 @@ fn parse_args() -> Option { )); } } - if let Some(path) = &picture { let canvas = mossaic::image::load(path, image_options).unwrap_or_else(|error| fail(&error)); let name = canvas diff --git a/src/cli.rs b/src/cli.rs index 5f2a365..2b9582f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -26,6 +26,9 @@ pub struct Args { /// Long options that were actually typed, so a saved plan or a config can /// fill in the rest without overriding them. typed: Vec, + /// Set once a bare `--` has been read: everything after it is a + /// positional, whatever it looks like. + end_of_options: bool, } impl Args { @@ -51,19 +54,57 @@ impl Args { program, rest, typed: Vec::new(), + end_of_options: false, } } /// The next argument, whatever it is. Not `next`: this is not an iterator, /// and reading it as one would be a subtle way to lose an argument. + /// + /// A bare `--` is consumed here and switches the parser into + /// end-of-options: after it, [`Args::is_positional`] is true of + /// everything, so an argument that looks like a flag is text. pub fn next_arg(&mut self) -> Option { - self.rest.pop_front() + let arg = self.rest.pop_front()?; + if arg == "--" && !self.end_of_options { + self.end_of_options = true; + return self.rest.pop_front(); + } + Some(arg) + } + + /// Whether `arg` should be read as text rather than matched against the + /// option names. + /// + /// The hyphen is the case that forced this. `-` is a glyph the font + /// draws, README lists it first among the punctuation, `action.yml` + /// names it in the `text:` input description and the binary's own + /// no-glyph message prints it in the alphabet — and it could not be + /// typed in the position all three documents put it in. `mossaic-art -` + /// was `unknown option "-"`, and `--` was itself `unknown option "--"`, + /// so the escape hatch a user reaches for reported the escape hatch as + /// the mistake. No quoting helped; the only route through was a + /// hand-written plan JSON, which nothing documents. + /// + /// Deliberately *not* "it looks like text, so treat it as text": that + /// would turn a mistyped `--yaer 2027` into a drawing. + #[must_use] + pub fn is_positional(&self, arg: &str) -> bool { + self.end_of_options || !arg.starts_with('-') + } + + /// Whether a bare `--` has been read. + #[must_use] + pub fn past_end_of_options(&self) -> bool { + self.end_of_options } /// Whether an argument follows that is not itself an option — for the flags /// that take an optional value, like `--track [USER]`. pub fn peek_value(&self) -> bool { - self.rest.front().is_some_and(|next| !next.starts_with('-')) + self.rest + .front() + .is_some_and(|next| self.end_of_options || !next.starts_with('-')) } /// The value belonging to `flag`, or a readable exit. diff --git a/src/draw.rs b/src/draw.rs index ec39c1c..e45deb8 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -525,10 +525,14 @@ pub fn render(frame: &mut Frame<'_>, editor: &Editor, palette: &Palette) { Span::raw(if level == 0 { " (must stay dark)".to_string() } else { - format!( - " {} commits each", - thousands(art::commits_to_reach(level, peak)) - ) + { + let each = art::commits_to_reach(level, peak); + format!( + " {} {} each", + thousands(each), + crate::plural(each, "commit", "commits") + ) + } }), ])); } diff --git a/src/main.rs b/src/main.rs index 98a7c69..4587af0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -281,6 +281,23 @@ fn parse_args() -> Option { let mut args = Args::from_env("mossaic"); while let Some(arg) = args.next_arg() { + // Past a bare `--`, every argument is the login. See the same block + // in mossaic-art: a dash-led login has the same hole. + if args.past_end_of_options() { + if login.is_none() { + login = Some(arg); + } else { + // The trap `--` sets for a first-time user: it means + // *everything* after it, as it does in `ls` and `git`, so + // options have to come first. Say that rather than leaving + // them to work it out. + fail(&format!( + "unexpected argument {arg:?} — everything after -- is the login, \ + so put the options before it" + )); + } + continue; + } match arg.as_str() { "-h" | "--help" => { println!("{HELP}"); @@ -344,8 +361,15 @@ fn parse_args() -> Option { "--cell" => options.cell = Some(parse_cell(&args.value("--cell"))), "--png" => png = Some(args.value("--png")), "--capabilities" => capabilities = true, - other if other.starts_with('-') => { - fail(&format!("unknown option {other:?} — try --help")) + // The same hole, in the same shared parser: `mossaic -- -weirdlogin` + // reported `--` as the unknown option. + other if !args.is_positional(other) => { + let hint = if login.is_none() { + " — if that is the login, write it after --" + } else { + " — try --help" + }; + fail(&format!("unknown option {other:?}{hint}")) } other => login = Some(other.to_string()), } diff --git a/src/plan.rs b/src/plan.rs index c3de168..dcf1fee 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -946,10 +946,18 @@ impl Report { out.push_str(&match self.verdict { "drawn" => format!("**{} is drawn.**\n\n", self.text), "holed" => format!( - "**Cannot be drawn cleanly** — {} {} inside the letters already \ - lit, and nothing takes those away.\n\n", + // The noun, not the verb. 0.6.3 moved the verb in front of + // "inside the letters", which turned "already lit" into a + // reduced relative clause on *the letters* — the sentence + // said the letters were lit rather than the days. The text + // renderer's equivalent still read correctly, so only the + // path the Action publishes regressed. + "**Cannot be drawn cleanly** — {} {} inside the letters {} already \ + lit, and nothing takes {} away.\n\n", self.holes, - plural(self.holes, "day is", "days are") + plural(self.holes, "day", "days"), + plural(self.holes, "is", "are"), + plural(self.holes, "it", "them") ), _ => format!( "**On track** — {} {} to go{}.\n\n", diff --git a/tests/art_cli.rs b/tests/art_cli.rs index b0e7268..87f1d87 100644 --- a/tests/art_cli.rs +++ b/tests/art_cli.rs @@ -759,9 +759,12 @@ fn a_saved_plan_makes_the_flags_optional() { "{}", String::from_utf8_lossy(&saved.stderr) ); + // On stderr since 0.7.0: under `--format json` or `--format markdown` + // stdout is a document, and this line was its first. It is still shown, + // in every format, where a human at a terminal sees it. assert!( - stdout(&saved).contains("mossaic-art --track"), - "it says what is next" + String::from_utf8_lossy(&saved.stderr).contains("mossaic-art --track"), + "it says what is next, on stderr" ); let spec: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(dir.join("mossaic-plan.json")).unwrap()) @@ -1891,3 +1894,281 @@ fn every_binary_parses_arguments_the_same_way() { ); } } + +/// A flag that names a companion either honours it or refuses the pair. +/// +/// #26 settled this in the maintainer's own words — "They are side effects +/// and inputs the user explicitly asked for … Either honour them or refuse +/// the combination" — and enumerated `--snapshot`, `--write` and `--file`, +/// all three of which are refused. `--png`, `-o` and `--format` were missed: +/// each was accepted in every mode and honoured in one, at exit 0, with an +/// empty stderr and no file. +/// +/// The cost is the shape a script cannot see. +/// `mossaic-art --template dragon --png preview.png` in a workflow printed a +/// cheerful report, exited 0 and produced nothing, so the next step read a +/// file that was not there — or republished the previous run's stale PNG. +/// `install.yml` only ever runs `--font --png`, the one pair that works. +#[test] +fn a_flag_that_names_a_companion_is_refused_without_it() { + let png = scratch("refused.png"); + let png = png.to_str().unwrap(); + let cases: [(&[&str], &str); 7] = [ + ( + &["--template", "dragon", "--year", "2027", "--png"], + "--font", + ), + ( + &[ + "--matrix", + "art/templates/dragon.art", + "--year", + "2027", + "--png", + ], + "--font", + ), + (&["VYNCINT", "--year", "2027", "--png"], "--font"), + (&["--list-templates", "--png"], "--font"), + (&["--track", "--png"], "--font"), + (&["--backfill", "--repo", "/tmp/nope", "--png"], "--font"), + ( + &["--image", "art/dragon.png", "--year", "2027", "--png"], + "--font", + ), + ]; + for (args, needs) in cases { + let _ = std::fs::remove_file(png); + let mut argv: Vec<&str> = args.to_vec(); + argv.push(png); + argv.extend(["--no-colour", "--plan", "/dev/null"]); + let out = art(&argv); + let text = String::from_utf8_lossy(&out.stderr); + assert_eq!( + out.status.code(), + Some(2), + "{args:?} must be refused\n{text}" + ); + assert!( + text.contains(needs), + "{args:?}: the message names {needs}\n{text}" + ); + assert!( + !Path::new(png).exists(), + "{args:?}: a refused run writes no file" + ); + } + + // The one pair that works is untouched. + let out = art(&["--font", "--png", png, "--no-colour"]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(Path::new(png).exists(), "--font --png still writes it"); + let _ = std::fs::remove_file(png); + + // The two adjacent silent no-ops. + for (args, needs) in [ + ( + vec!["-o", "/tmp/nope.art", "VYNCINT", "--year", "2027"], + "--draw", + ), + ( + vec!["VYNCINT", "--year", "2027", "--format", "json"], + "--track", + ), + ] { + let mut argv = args.clone(); + argv.extend(["--no-colour", "--plan", "/dev/null"]); + let out = art(&argv); + let text = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(2), "{args:?}\n{text}"); + assert!(text.contains(needs), "{args:?}\n{text}"); + } +} + +/// `--` means the rest is text, including the hyphen the font draws. +/// +/// `-` is a glyph this project lists three times — README's punctuation +/// line, `action.yml`'s `text:` description, and the binary's own no-glyph +/// message — and it could not be typed in the position all three put it in. +/// Worse, `--` was itself reported as the unknown option, so the escape +/// hatch a user reaches for named itself as the mistake, and no quoting +/// helped. You reach for `mossaic-art -- "$TEXT"` precisely when the text is +/// not yours to control, which is what the Action does with `text:`. +#[test] +fn a_double_dash_makes_the_rest_text() { + for (text, columns) in [("-", 5), ("-.-", 17), ("A-B", 17)] { + let out = art(&[ + "--year", + "2027", + "--no-colour", + "--plan", + "/dev/null", + "--", + text, + ]); + let first = String::from_utf8_lossy(&out.stdout) + .lines() + .next() + .unwrap_or_default() + .to_string(); + assert!( + out.status.success(), + "{text:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + first.starts_with(&text.to_uppercase()), + "{text:?} is the subject: {first}" + ); + assert!( + first.contains(&format!("{columns} of 53 columns")), + "{text:?} draws {columns} columns: {first}" + ); + } + + // An argument that looks like a flag is text after `--`. + let out = art(&["--no-colour", "--plan", "/dev/null", "--", "-h"]); + assert!( + String::from_utf8_lossy(&out.stdout).starts_with("-H"), + "`-- -h` draws rather than printing help" + ); + + // A genuine unknown option is still one, and the message points at `--` + // when the argument could plausibly have been the text. + let out = art(&["-A-"]); + let text = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(2)); + assert!(text.contains("unknown option"), "{text}"); + assert!(text.contains("--"), "the way out is named: {text}"); + + // And `--` means *everything* after it, as it does in `ls` and `git`, so + // an option written after the text is an error that says so. + let out = art(&["--", "-", "--year", "2027"]); + let text = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(2), "{text}"); + assert!(text.contains("put the options before it"), "{text}"); +} + +/// `--track --save` writes a machine document to stdout and nothing else. +/// +/// The `saved …` confirmation was the single `println!` that could run ahead +/// of the document, so `--format json | jq .` got two lines of prose first — +/// at exit 0, with an empty stderr, so there was no signal anywhere. It bit +/// only on the run that writes the plan, so it read as a flake on first use. +#[test] +fn saving_a_plan_does_not_write_prose_into_a_machine_document() { + let plan = scratch("saveformat.json"); + let merge = Path::new(env!("CARGO_MANIFEST_DIR")).join("art/vyncint-2026.json"); + + for format in ["json", "markdown"] { + let _ = std::fs::remove_file(&plan); + let out = art(&[ + "VYNCINT", + "--year", + "2026", + "--start-week", + "6", + "--track", + "--save", + "--plan", + plan.to_str().unwrap(), + "--merge", + merge.to_str().unwrap(), + "--today", + "2026-08-19", + "--format", + format, + "--no-colour", + ]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let text = String::from_utf8_lossy(&out.stdout); + match format { + "json" => { + serde_json::from_str::(&text) + .unwrap_or_else(|e| panic!("stdout must be one JSON document: {e}\n{text}")); + } + _ => assert!( + text.starts_with("### "), + "stdout must start with the heading:\n{text}" + ), + } + // The confirmation is still shown — on stderr, where a human sees it. + assert!( + String::from_utf8_lossy(&out.stderr).contains("mossaic-art --track"), + "the confirmation is not lost, only moved" + ); + assert!(plan.exists(), "and the plan is written"); + } + let _ = std::fs::remove_file(&plan); +} + +/// `--help`'s description of `--commits` has to price what the tool prices. +/// +/// It said "commits per lit day", and a reader who did that multiplication +/// on the shipped dragon arrived at 584 where the tool prints 442 — a 32% +/// overestimate from following the help exactly. `--commits` prices the +/// *brightest* day; darker shades are priced against it, which docs/ART.md +/// and the source comment both say correctly. The help is the only +/// description a `cargo install` user gets: `docs/*` is excluded from the +/// published archive and README never mentions the flag. +#[test] +fn the_help_prices_commits_the_way_the_report_does() { + let help = String::from_utf8_lossy(&art(&["--help"]).stdout).into_owned(); + let line = help + .lines() + .find(|l| l.trim_start().starts_with("--commits")) + .expect("--commits is documented"); + assert!( + line.contains("brightest"), + "the help must price the brightest shade, not every lit day: {line}" + ); + + // And the arithmetic it now describes is the one the report performs. + let report = String::from_utf8_lossy( + &art(&[ + "--template", + "dragon", + "--year", + "2027", + "--no-colour", + "--plan", + "/dev/null", + ]) + .stdout, + ) + .into_owned(); + let header: u32 = report + .lines() + .next() + .and_then(|l| l.rsplit("·").next()) + .and_then(|tail| tail.split_whitespace().next().map(str::to_string)) + .and_then(|n| n.replace(',', "").parse().ok()) + .expect("the header names a commit total"); + // Sum the level rows: ` 4 75 4`. + let mut table = 0u32; + for line in report.lines() { + let cells: Vec<&str> = line.split_whitespace().collect(); + // Edition 2021 here, so no let-chains. + if let [level, days, each] = cells.as_slice() { + if let (Ok(_), Ok(days), Ok(each)) = ( + level.parse::(), + days.replace(',', "").parse::(), + each.replace(',', "").parse::(), + ) { + table += days * each; + } + } + } + assert_eq!( + table, header, + "the level table must price out to the header:\n{report}" + ); +} From 1d9e307c515fc67fa1c30b5b11f1320879b2ddaa Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:29:49 +0700 Subject: [PATCH 4/9] fix: count the days the year has, and the days that have happened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two figures the tool computes from the wrong set. **The cost table counted cells, not days.** A full-width picture is 7 x 53 = 371 cells against a year of 365, so the preview table disagreed with the header directly above it and with what `--write` makes — immediately after a note saying cells had been dropped. Measured: a gradient image priced out 322 days and 742 commits while the header said 317 and 722, and `--write` made 722. README says of the editor panel "the same arithmetic `--write` uses, not an estimate of it", and it was not. The tracking renderer always did it correctly, which is why the shipped docs disagree with themselves — ART.md prints one figure for a preview and another for the tracking table of the same plan, exactly the out-of-year cells apart. The legibility verdict came from the same raw canvas, so a picture that drew literally nothing inside the year still reported `shades 0 4 · closest pair 0 and 4 · ΔE 70, clear`: the one check this project tells you to read twice, passing on a drawing that does not exist. It now prints no verdict at all there. The editor panel had the same split. `estimate()` already filtered by `date_at`, so the rows said `level 4 1 day 4 commits each` four lines above `0 commits in total` — for a cell the panel itself draws as `·` and describes as costing nothing. The four shipped templates cannot catch any of this: all of them are drawn clear of the partial weeks across 2000-2100, so the test plants its own ink in the leading and trailing cells and asserts header == table == the days the year has. **The chart header counted the whole year.** Every figure honoured `--today` except the biggest one and the only one a screenshot carries. It took `Calendar::total` — GitHub's number for Jan-1..Dec-31 — while everything below came from `elapsed()`, so reading the shipped calendar as of March printed 9,527 above a grid showing 2,043, beside "23 active days". It also broke the two features `--today` was built for together: the `--snapshot` preview flow advertised the whole plan's cost as already paid while the planner, on the same data, said thousands were still owed. It now uses the elapsed sum when the calendar holds any future day, which is the same `any(future)` test that gates the "blank = still to come" legend, so the two turn on together. A finished year and a current year fetched with no `--today` are unchanged: `Calendar::total` is GitHub's own figure and can legitimately exceed the sum of visible days, which is why the switch is on future days rather than on comparing the two. The test asserts the header against the footer, so it cannot be satisfied by a header that is merely different. Two editor PTY tests now move the cursor a column right before painting: the cursor starts on a cell outside 2027, and painting there correctly moves no row any more. That distinction is the fix. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- src/art.rs | 55 +++++++++++++++- src/bin/mossaic-art.rs | 20 +++++- src/draw.rs | 32 +++++++++- src/render_tests.rs | 58 +++++++++++++++++ src/ui.rs | 34 ++++++++-- tests/art_cli.rs | 138 +++++++++++++++++++++++++++++++++++++++++ tests/canvas_pty.rs | 8 +++ 7 files changed, 330 insertions(+), 15 deletions(-) diff --git a/src/art.rs b/src/art.rs index 26823e6..a1f87d1 100644 --- a/src/art.rs +++ b/src/art.rs @@ -802,7 +802,12 @@ impl Canvas { .flat_map(|column| column.iter().copied()) } - /// How many days sit at each level, indexed by level. + /// How many *cells* sit at each level, indexed by level. + /// + /// This counts the canvas, which is 7 x width — not the year. Use it for + /// questions about the drawing itself; for anything a user budgets + /// against, ask [`levels_histogram`] about the days the calendar + /// actually has. #[must_use] pub fn histogram(&self) -> [usize; 5] { let mut counts = [0usize; 5]; @@ -826,7 +831,12 @@ impl Canvas { /// Every level the picture uses, darkest first. #[must_use] pub fn palette(&self) -> Vec { - let histogram = self.histogram(); + Self::palette_of(&self.histogram()) + } + + /// The same, from a histogram somebody else counted. + #[must_use] + pub fn palette_of(histogram: &[usize; 5]) -> Vec { (0..=4u8) .filter(|level| histogram[usize::from(*level)] > 0) .collect() @@ -849,7 +859,20 @@ impl Canvas { /// `None` for a canvas of one shade, which has no pair to compare. #[must_use] pub fn closest_pair(&self) -> Option<(u8, u8, Legibility, f32)> { - let used = self.palette(); + Self::closest_pair_of(&self.histogram()) + } + + /// The same, over the shades a given histogram holds. + /// + /// Taking the histogram rather than the canvas is what lets the preview + /// ask about the shades that land *inside the year*: a picture whose only + /// ink falls in the partial weeks at either end drew nothing, and used to + /// report `shades 0 4 · closest pair 0 and 4 · ΔE 70, clear` — the one + /// check this project tells you to read twice, passing on a drawing that + /// does not exist. + #[must_use] + pub fn closest_pair_of(histogram: &[usize; 5]) -> Option<(u8, u8, Legibility, f32)> { + let used = Self::palette_of(histogram); let mut worst: Option<(u8, u8, f32)> = None; for (index, low) in used.iter().enumerate() { for high in used.iter().skip(index + 1) { @@ -870,6 +893,8 @@ impl Canvas { /// The busiest day this canvas needs the year to have before its shades can /// be told apart. /// + /// (see [`levels_histogram`] for the calendar-side counterpart) + /// /// GitHub's scale has four steps, so a year whose busiest day is 1 holds /// exactly two shades: empty and full. A picture using any level between /// needs a peak of at least 4, where the counts 1, 2, 3, 4 land on levels @@ -970,6 +995,30 @@ impl Canvas { /// the plan and out of the Action's `headline` output. const MAX_META: usize = 200; +/// How many days sit at each level, over the days the calendar actually has. +/// +/// The canvas-side [`Canvas::histogram`] counts cells — 7 x width, which for +/// a full-width picture is 371 against a year of 365 or 366. The preview +/// table and the editor panel used it, so they disagreed with the header +/// above them and with what `--write` makes: a picture with ink in the +/// partial weeks at either end priced out 322 days and 742 commits while the +/// header said 317 and 722, and `--write` made 722. The note directly above +/// the table had just said cells were dropped, and the table counted them +/// anyway. +/// +/// The tracking renderer always did it this way, which is why the shipped +/// docs disagree with themselves: `docs/ART.md` prints one figure for the +/// preview and another for the tracking table of the same plan, exactly the +/// out-of-year cells apart. +#[must_use] +pub fn levels_histogram(levels: &std::collections::BTreeMap) -> [usize; 5] { + let mut counts = [0usize; 5]; + for level in levels.values() { + counts[usize::from(*level).min(4)] += 1; + } + counts +} + /// Read one `# key: value` header line into `meta`. /// /// Unknown keys are ignored rather than refused. A `.art` file is a document as diff --git a/src/bin/mossaic-art.rs b/src/bin/mossaic-art.rs index 38b9fe4..7d1a46c 100644 --- a/src/bin/mossaic-art.rs +++ b/src/bin/mossaic-art.rs @@ -675,7 +675,12 @@ fn run_canvas(options: &Options, grid: &Grid, name: &str, canvas: &art::Canvas) return; } - let histogram = canvas.histogram(); + // The days the *calendar* holds, not the cells the canvas has. A + // full-width picture is 7 x 53 = 371 cells against a year of 365, and the + // table used to count all of them — disagreeing with the header directly + // above it and with what `--write` makes, after a note that had just said + // cells were dropped. + let histogram = art::levels_histogram(&levels); println!( "{name} · {} · {} of {} columns · {} {} · {} {}\n", grid.year, @@ -709,8 +714,17 @@ fn run_canvas(options: &Options, grid: &Grid, name: &str, canvas: &art::Canvas) // Whether a reader will see a picture or a smudge. The pair measured is the // darkest and brightest the drawing actually uses: if those two are faint, // everything between them is worse. - if let Some((low, high, legibility, delta)) = canvas.closest_pair() { - let used: Vec = canvas.palette().iter().map(u8::to_string).collect(); + // Asked about the shades that land inside the year. A picture whose only + // ink falls in the partial weeks drew nothing and still reported + // `shades 0 4 · ΔE 70, clear`: the one check this project tells you to + // read twice, passing on a drawing that does not exist. `closest_pair_of` + // returns `None` for a single shade, so such a run now prints no verdict + // at all rather than a flattering one. + if let Some((low, high, legibility, delta)) = art::Canvas::closest_pair_of(&histogram) { + let used: Vec = art::Canvas::palette_of(&histogram) + .iter() + .map(u8::to_string) + .collect(); println!( "\n shades {} · closest pair {low} and {high} · ΔE {delta:.0}, {legibility}", used.join(" ") diff --git a/src/draw.rs b/src/draw.rs index e45deb8..bb32a5b 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -222,6 +222,27 @@ impl Editor { }) } + /// How many days sit at each level, over the days the year actually has. + /// + /// `Canvas::histogram` counts *cells*, and the partial weeks at either + /// end are cells with no date behind them — the panel drew them as `·` + /// and told the user they "cost nothing", then counted them anyway. So + /// the rows read `level 4 1 day 4 commits each` four lines above + /// `0 commits in total`. `estimate()` has always filtered by `date_at`; + /// this is the same filter, for the rows beside it. + #[must_use] + pub fn histogram(&self) -> [usize; 5] { + let mut counts = [0usize; 5]; + for week in 0..self.canvas.width() { + for row in 0..CANVAS_ROWS { + if self.date_at(week, row).is_some() { + counts[usize::from(self.canvas.at(week, row)).min(4)] += 1; + } + } + } + counts + } + /// The two shades in the drawing that look most alike, and how far apart /// they are in the worst palette a reader might have. /// @@ -230,7 +251,11 @@ impl Editor { /// hold two shades nobody can separate. See [`art::Canvas::closest_pair`]. #[must_use] pub fn legibility(&self) -> Option<(u8, u8, Legibility, f32)> { - self.canvas.closest_pair() + // Over the shades that land inside the year, for the same reason the + // preview does: a drawing whose only ink is in the partial weeks + // draws nothing, and a verdict on it would be about cells nobody + // will see. + art::Canvas::closest_pair_of(&self.histogram()) } /// Handle a keystroke. @@ -496,7 +521,7 @@ pub fn render(frame: &mut Frame<'_>, editor: &Editor, palette: &Palette) { frame.render_widget(Paragraph::new(lines), body); // The numbers, which are the reason to draw here rather than in an editor. - let histogram = editor.canvas.histogram(); + let histogram = editor.histogram(); let peak = editor.peak(); let mut stats: Vec> = Vec::new(); stats.push(Line::from(match editor.cursor_date() { @@ -515,7 +540,8 @@ pub fn render(frame: &mut Frame<'_>, editor: &Editor, palette: &Palette) { for level in (0..=4u8).rev() { let count = histogram[usize::from(level)]; let rgb = palette.levels[usize::from(level)]; - let bar = "█".repeat(count * 30 / (editor.canvas.width() * CANVAS_ROWS).max(1)); + let total: usize = histogram.iter().sum(); + let bar = "█".repeat(count * 30 / total.max(1)); stats.push(Line::from(vec![ Span::raw(format!( " level {level} {count:>4} {:<6} ", diff --git a/src/render_tests.rs b/src/render_tests.rs index 7c2bda0..085d762 100644 --- a/src/render_tests.rs +++ b/src/render_tests.rs @@ -2449,6 +2449,64 @@ fn control_characters_never_leave_the_parser() { assert_eq!(crate::printable("héllo ✓"), "héllo ✓", "text is left alone"); } +/// The header counts what happened, not what the year will hold. +/// +/// Every figure on the chart honoured `--today` except the biggest one. It +/// took `Calendar::total` — GitHub's number for the whole Jan-1..Dec-31 +/// range — while everything below came from `elapsed()`, so reading the +/// shipped calendar as of March printed December's 9,527 above a grid +/// showing 2,043 and beside "23 active days": a 4.7x overstatement of the +/// one figure a screenshot carries. +/// +/// Asserted against the *footer*, so the test cannot be satisfied by a +/// header that is merely different — the two have to agree. +#[test] +fn the_header_agrees_with_the_footer_when_a_year_is_read_as_of_a_day() { + let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("art/vyncint-2026.json"); + let Ok(calendar) = crate::github::from_file( + path.to_str().unwrap(), + Some(NaiveDate::from_ymd_opt(2026, 3, 31).unwrap()), + ) else { + panic!("the shipped calendar loads"); + }; + let elapsed: u32 = calendar.elapsed().map(|day| day.count).sum(); + let total = calendar.total; + assert_ne!(elapsed, total, "the fixture must have future days to test"); + + let mut app = ready(calendar); + let frame = render(&mut app, 170, 22); + let wanted = format!("{} contributions in 2026", crate::thousands(elapsed)); + assert!( + frame.contains(&wanted), + "the header must count the elapsed days ({wanted}):\n{frame}" + ); + assert!( + !frame.contains(&format!( + "{} contributions in 2026", + crate::thousands(total) + )), + "and must not print the whole year's figure over a partial grid" + ); + + // A finished year is unchanged: `total` is GitHub's own figure and can + // legitimately exceed the sum of visible days, which is why the switch + // is on `any(future)` rather than on a comparison. + let whole = crate::github::from_file( + path.to_str().unwrap(), + Some(NaiveDate::from_ymd_opt(2026, 12, 31).unwrap()), + ) + .expect("the same file, read whole"); + let mut app = ready(whole); + let frame = render(&mut app, 170, 22); + assert!( + frame.contains(&format!( + "{} contributions in 2026", + crate::thousands(total) + )), + "a finished year still shows GitHub's own total:\n{frame}" + ); +} + /// GHSA-jp9f-97rv-j4hx. /// /// The calendar path was cleaned in the 0.1.0 review; the `.art` header was diff --git a/src/ui.rs b/src/ui.rs index cb455d9..f9ba32d 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -470,13 +470,35 @@ fn header(app: &App) -> Line<'static> { ]; if let Load::Ready(calendar) = &app.load { spans.push(separator(palette)); - // github.com's own wording, under its own chart. + // github.com's own wording, under its own chart — and github.com + // counts what has *happened*, because it has no future data to count. + // + // This was the one figure `--today` did not move. It took + // `Calendar::total`, GitHub's number for the whole Jan-1..Dec-31 + // range, while every statistic below it comes from `elapsed()` — + // "days that have actually happened, which is what statistics are + // drawn from". Reading the shipped calendar as of 2026-03-31 put + // December's 9,527 above a grid showing 2,043, a 4.7x overstatement, + // beside "23 active days"; docs/DESIGN.md §2 says days still to come + // "are excluded from every statistic", and this is the biggest one + // and the only one a screenshot carries. + // + // It also broke the two features where `--today` was meant to be + // used together: the `--snapshot` preview flow advertised the whole + // plan's cost as already paid, while the planner on the same data + // said thousands of contributions were still owed. + // + // The same `any(future)` test gates the "blank = still to come" + // legend below, so the header and that legend turn on together. A + // finished year, and the current year fetched with no `--today`, are + // unchanged: `total` and the sum of visible days agree there. + let counted = if calendar.days().any(|day| day.future) { + calendar.elapsed().map(|day| day.count).sum() + } else { + calendar.total + }; spans.push(Span::styled( - format!( - "{} contributions in {}", - thousands(calendar.total), - app.year - ), + format!("{} contributions in {}", thousands(counted), app.year), Style::new().fg(palette.ansi(palette.fg)), )); } diff --git a/tests/art_cli.rs b/tests/art_cli.rs index 87f1d87..4eb4f8e 100644 --- a/tests/art_cli.rs +++ b/tests/art_cli.rs @@ -2172,3 +2172,141 @@ fn the_help_prices_commits_the_way_the_report_does() { "the level table must price out to the header:\n{report}" ); } + +/// The cost table counts the days the year has, not the cells the canvas has. +/// +/// A full-width picture is 7 x 53 = 371 cells against a year of 365, and the +/// preview table counted all of them — so it disagreed with the header +/// directly above it and with what `--write` makes, immediately after a note +/// saying cells had been dropped. The four shipped templates cannot catch +/// this: all of them are drawn clear of the partial weeks across 2000-2100, +/// which is why this plants its own ink there. +/// +/// README says of the editor panel "the same arithmetic `--write` uses, not +/// an estimate of it", and the table is what somebody budgets against before +/// making commits that cannot be unmade. +#[test] +fn the_cost_table_prices_out_to_the_header() { + // 2027-01-01 is a Friday, so column 0's Sun..Thu and column 52's tail are + // outside the year. Ink in both, plus one cell that is genuinely inside. + let mut rows = vec![vec!['0'; 53]; 7]; + rows[0][0] = '4'; // outside: before Jan 1 + rows[6][52] = '4'; // outside: after Dec 31 + rows[5][0] = '4'; // inside: Friday of week 0 is Jan 1 itself + rows[3][10] = '2'; // inside, comfortably + let body = format!( + "# name: Edges\n{}\n", + rows.iter() + .map(|row| row.iter().collect::()) + .collect::>() + .join("\n") + ); + let path = scratch("edges.art"); + std::fs::write(&path, body).unwrap(); + + let out = art(&[ + "--matrix", + path.to_str().unwrap(), + "--year", + "2027", + "--no-colour", + "--plan", + "/dev/null", + ]); + let report = String::from_utf8_lossy(&out.stdout).into_owned(); + let _ = std::fs::remove_file(&path); + + // The header: `Edges · 2027 · N of 53 columns · D days · C commits` + let header = report.lines().next().unwrap_or_default(); + let fields: Vec<&str> = header.split('·').map(str::trim).collect(); + let header_days: usize = fields[3] + .split_whitespace() + .next() + .unwrap() + .parse() + .unwrap(); + let header_commits: u32 = fields[4] + .split_whitespace() + .next() + .unwrap() + .replace(',', "") + .parse() + .unwrap(); + + // The table: ` 4 2 4` and ` 0 363 must stay dark`. + let (mut table_days, mut table_commits, mut dark) = (0usize, 0u32, 0usize); + for line in report.lines() { + let cells: Vec<&str> = line.split_whitespace().collect(); + if cells.len() == 3 { + if let (Ok(level), Ok(days), Ok(each)) = ( + cells[0].parse::(), + cells[1].replace(',', "").parse::(), + cells[2].replace(',', "").parse::(), + ) { + if level > 0 { + table_days += days; + table_commits += days as u32 * each; + } + } + } + if cells.len() == 5 && cells[0] == "0" && line.contains("must stay dark") { + dark = cells[1].replace(',', "").parse().unwrap(); + } + } + + assert_eq!( + table_days, header_days, + "the level rows must sum to the header's day count:\n{report}" + ); + assert_eq!( + table_commits, header_commits, + "and price out to its commit total:\n{report}" + ); + assert_eq!( + table_days + dark, + 365, + "lit rows plus level 0 are the days 2027 has, not 53x7:\n{report}" + ); +} + +/// A picture whose only ink falls outside the year gets no legibility verdict. +/// +/// The verdict was computed from the raw canvas, so a drawing that puts +/// nothing at all inside the year still reported `shades 0 4 · closest pair +/// 0 and 4 · ΔE 70, clear` — the one check this project tells you to read +/// twice, passing on a drawing that does not exist. +#[test] +fn a_picture_that_draws_nothing_claims_no_legibility() { + let mut rows = vec![vec!['0'; 53]; 7]; + rows[0][0] = '4'; + rows[6][52] = '4'; + let body = format!( + "# name: Outside\n{}\n", + rows.iter() + .map(|row| row.iter().collect::()) + .collect::>() + .join("\n") + ); + let path = scratch("outside.art"); + std::fs::write(&path, body).unwrap(); + let out = art(&[ + "--matrix", + path.to_str().unwrap(), + "--year", + "2027", + "--no-colour", + "--plan", + "/dev/null", + ]); + let report = String::from_utf8_lossy(&out.stdout).into_owned(); + let _ = std::fs::remove_file(&path); + + assert!( + !report.contains("closest pair"), + "a drawing with no ink in the year has no shades to compare:\n{report}" + ); + assert!( + report.contains("0 days") && report.contains("0 commits"), + "and its header says so:\n{report}" + ); +} diff --git a/tests/canvas_pty.rs b/tests/canvas_pty.rs index 62659e5..44e304f 100644 --- a/tests/canvas_pty.rs +++ b/tests/canvas_pty.rs @@ -213,6 +213,12 @@ fn undo_takes_the_drawing_back_and_says_when_there_is_no_more() -> termlens::Res let mut terminal = spawn(&["--draw", "--year", "2027", "--plan", "/dev/null"])?; terminal.wait_until(ready)?; + // Right one column first. The cursor starts at Sunday of week 0, which + // for 2027 is *outside* the year — the panel draws it `·` and says it + // costs nothing — and since 0.7.0 the level rows count only days the + // calendar has, so painting there moves no row. That distinction is the + // point of the fix, so the test has to respect it. + terminal.send(Key::Char('l'))?; terminal.send(Key::Char('3'))?; terminal.wait_until(|screen| screen.contains("level 3 1 day "))?; terminal.send(Key::Char('u'))?; @@ -289,6 +295,8 @@ fn quitting_puts_the_terminal_back() -> termlens::Result<()> { fn an_unsaved_drawing_is_not_lost_quietly() -> termlens::Result<()> { let mut terminal = spawn(&["--draw", "--year", "2027", "--plan", "/dev/null"])?; terminal.wait_until(ready)?; + // Inside the year: see the note in `undo_takes_the_drawing_back`. + terminal.send(Key::Char('l'))?; terminal.send(Key::Char('4'))?; terminal.wait_until(|screen| screen.contains("level 4 1 day "))?; terminal.send(Key::Char('q'))?; From 95505ac97b0fa47d78ad58243bcf203062fe9139 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:34:21 +0700 Subject: [PATCH 5/9] fix: name the file, the key and the template that went wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three loaders that were loud about one kind of wrong and silent about another. **A template that does not parse was invisible.** The skip is deliberate policy — one broken file must not take out `--list-templates` — but `templates::read_dir`'s own doc comment calls that command "the command you would reach for to find out which one is broken", and it named nothing, counted nothing and wrote no byte to stderr. So the same file got "a canvas is exactly 7 rows" from `--matrix` and "no template named sixer" from `--template`: an error about a *name*, for a file sitting right there under that name, which sends the search to the wrong place. #57 — this project's own good-first-issue walkthrough — puts a first-time contributor on exactly that path. The listing now names each skipped file and why, `--template ` gives the parse error rather than a name miss, and a broken local file no longer silently shadows a built-in — the same command used to draw two different pictures depending on whether the user's file happened to parse, with nothing printed either way. The skip-rather-than-fail policy is unchanged. **A plan's mistyped key was applied at its default.** `validate()` bounds every value and no part of the shape, so `background: 99` was refused by name and `backgruond: 2` was accepted in silence — turning about 290 background days into keep-dark days, the one kind the Action's README calls "do not commit today". Dropping `art` turned a 146-day picture into a 79-day text, and `--backfill` then asked for a different date range and a different total. A plan is the input to `--backfill --write`, and contributions cannot be unlit. `deny_unknown_fields`, which costs the forward direction CHANGELOG 0.6.0 left open: a plan written by a newer mossaic is now refused. That is the honest trade — the file is version-locked to the tool that wrote it in practice already — and docs/ART.md's "Saving the plan" section says so. **A malformed `--file` blamed gh.** `--file` and the network share one parser whose only wording was "unexpected response from gh", so a truncated local file was reported as the GitHub CLI returning something odd, on a run where gh was never executed — and `{"data":{}}` was reported as GitHub having no such login. The reader then checks `gh auth status`, the username and the network: everything except the JSON in front of them. `--file` is the flag most likely to be handed a file another process is still writing, which is exactly the truncated case, and the plan loader two commands away already got this right. The source is threaded through `parse` now, and `--merge` stops quoting the path where nothing else does. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- src/bin/mossaic-art.rs | 36 ++++++-- src/github.rs | 57 +++++++++++- src/plan.rs | 24 ++++- src/templates.rs | 84 ++++++++++++++--- tests/art_cli.rs | 200 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 380 insertions(+), 21 deletions(-) diff --git a/src/bin/mossaic-art.rs b/src/bin/mossaic-art.rs index 7d1a46c..67fd001 100644 --- a/src/bin/mossaic-art.rs +++ b/src/bin/mossaic-art.rs @@ -518,7 +518,11 @@ fn run_editor(options: &Options, grid: &Grid, name: &str, canvas: art::Canvas) { /// `--list-templates`: the catalogue, with where each one came from. fn show_templates(colour: bool) { let catalogue = templates::catalogue(); - if catalogue.is_empty() { + // Read before the early return: a directory holding only broken files + // used to print "no templates installed", which is the least helpful + // true sentence available. + let skipped = templates::skipped(); + if catalogue.is_empty() && skipped.is_empty() { println!("no templates installed"); return; } @@ -577,7 +581,26 @@ fn show_templates(colour: bool) { } println!(); } - println!("draw one: mossaic-art --template {}", catalogue[0].name); + // What was skipped, and why. `templates::read_dir`'s own doc comment + // already called this command "the command you would reach for to find + // out which one is broken" — and it named nothing, counted nothing and + // wrote no byte to stderr. #57's walkthrough puts a first-time + // contributor on exactly this path: `cp your-name.art templates/ && + // mossaic-art --list-templates`. + if !skipped.is_empty() { + println!( + "{} {} skipped:", + skipped.len(), + plural(skipped.len(), "file was", "files were") + ); + for broken in &skipped { + println!(" {}: {}", broken.file, broken.why); + } + println!(); + } + if let Some(first) = catalogue.first() { + println!("draw one: mossaic-art --template {}", first.name); + } } /// Everything a run that draws a **picture** does, from preview to commits. @@ -1688,9 +1711,12 @@ fn write_font_sheet(path: &Path) { } /// Existing contributions from a saved `gh api graphql` response. -fn load(path: &PathBuf, grid: &Grid) -> BTreeMap { - let calendar = github::from_file(&path.to_string_lossy(), None) - .unwrap_or_else(|error| fail(&format!("could not read {path:?}: {error}"))); +fn load(path: &Path, grid: &Grid) -> BTreeMap { + // `from_file` names the path itself now, so re-prefixing would print it + // twice — and it used to quote the path where every other path message + // in these tools does not. + let calendar = + github::from_file(&path.to_string_lossy(), None).unwrap_or_else(|error| fail(&error)); let kept: BTreeMap = calendar .days() .filter(|day| day.count > 0 && grid.holds(day.date)) diff --git a/src/github.rs b/src/github.rs index 60d1e05..e98938f 100644 --- a/src/github.rs +++ b/src/github.rs @@ -35,7 +35,7 @@ pub fn fetch(login: &str, year: i32, today: NaiveDate) -> Result) -> Result now.map_or_else(|| Local::now().year(), |date| date.year()), &body, now, + Source::File(path), ) } @@ -92,10 +93,58 @@ fn run_query(vars: &[(&str, &str)]) -> Result { Ok(stdout) } +/// Where a response came from, so an error can name the right thing. +/// +/// `--file` and the network share one parser, and the parser's only wording +/// was "unexpected response from gh". So a truncated local file the user +/// wrote themselves was reported as their GitHub CLI returning something +/// odd — on a run where `gh` was never executed — and `{"data":{}}` was +/// reported as GitHub having no such login. They then checked +/// `gh auth status`, the username and the network: everything except the +/// JSON in front of them. +/// +/// `--file` is also the flag most likely to be handed a file another +/// process is still writing, which is exactly the truncated case. The plan +/// loader two commands away already gets this right. +#[derive(Debug, Clone, Copy)] +enum Source<'a> { + /// The `gh` subprocess. + Gh, + /// A file the user named. + File(&'a str), +} + +impl Source<'_> { + /// "…is not a saved contributions response: ", or the gh wording. + fn not_readable(self, detail: &str) -> String { + match self { + Source::Gh => format!("unexpected response from gh: {detail}"), + Source::File(path) => { + format!("{path} is not a saved contributions response: {detail}") + } + } + } + + /// The document parsed but holds no user. + fn no_user(self) -> String { + match self { + Source::Gh => "GitHub returned no user for that login".to_string(), + Source::File(path) => { + format!("{path} holds no user — a saved response has data.user") + } + } + } +} + /// `now` decides which days count as still to come; `None` means none of them do. -fn parse(fallback_year: i32, body: &str, now: Option) -> Result { +fn parse( + fallback_year: i32, + body: &str, + now: Option, + source: Source<'_>, +) -> Result { let resp: Response = - serde_json::from_str(body).map_err(|e| format!("unexpected response from gh: {e}"))?; + serde_json::from_str(body).map_err(|e| source.not_readable(&e.to_string()))?; // Everything below this line came from somewhere else, so it is stripped of // control characters before it can reach a terminal. @@ -111,7 +160,7 @@ fn parse(fallback_year: i32, body: &str, now: Option) -> Result BTreeMap Result { let body = std::fs::read_to_string(path) .map_err(|error| format!("could not read {}: {error}", path.display()))?; - let spec: Self = serde_json::from_str(&body) - .map_err(|error| format!("{} is not a mossaic plan: {error}", path.display()))?; + let spec: Self = serde_json::from_str(&body).map_err(|error| { + // serde names the key it did not know and lists the ones it does, + // which is the whole of what a typo needs. + format!("{} is not a mossaic plan: {error}", path.display()) + })?; // Named as a problem with the file, and with the way out of it: 0.2.0 // let `--commits -1` through, which `--save` then wrote down as four // billion, so a plan in the wild can be one this refuses. Saving it diff --git a/src/templates.rs b/src/templates.rs index fff3d3f..5419af0 100644 --- a/src/templates.rs +++ b/src/templates.rs @@ -103,7 +103,7 @@ pub fn catalogue() -> Vec