From 0b794e0c280c73de001fcde1539bae743f1b6a68 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 7 Oct 2026 07:02:47 +0700 Subject: [PATCH] docs: a review pass, fifteen claims the code contradicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each was checked against the code or by running it before it changed; one more item is a style tidy, marked as such. README: - `Screen::parse` reads the snapshot format, not an insta `.snap`: it rejects the `---` header on line 1. The body below it parses, and the `termlens` command reads a whole `.snap` because it strips the header. - The skill has five recipes, not four. SKILL.md: - §7's mask row said `mask_matching` "spans a wrap the way `find_all` does". A value split by a soft wrap is two rows and stays unmasked (measured: 10 columns, `SECRET-VALUE-42`); a needle with `\n` is what spans rows. Rule 12 already says so for `contains`. - Rule 10 said `format!("{:?}")` is the same as `Display`. It wraps the text in `Screen(…)`, which `Screen::parse`, `diff` and `render` refuse. - §6 said every error's `Display` ends with the screen. Only the wait errors (timeout, EOF, emulator) and a failed write carry one. SECURITY.md: "bot-authored commits are rejected by CI" is true except for Dependabot, which CONTRIBUTING's AI tooling policy exempts from the identity rule only; the provenance line now says so. DESIGN.md: - The writer queue is unbounded (`mpsc::channel`), capped on reply bytes; one paragraph said bounded, the next unbounded. - The user-facing Windows list is in docs/LIMITATIONS.md, not the README. - `CSI 18 t` is answered, and `14 t`/`16 t` once a cell size is declared, so "the non-pixel `CSI t` reports" were never all unanswered; the same correction in LIMITATIONS.md names `11 t` and `19 t` instead. - The compatibility corpus starts at 0.10.1, not "each published release". emit's steps: an argument beginning with `--` must be a step, so `emit --foo` exits 2 rather than printing `--foo`; TEXT and `--text` say so. Rustdoc: - `Unsupported`'s summary was `Clipboard`'s first sentence, so docs.rs described the type as clipboard data; the sentence is back on `Clipboard`, which had no summary. - `assert_screen_snapshot!` repeated the Display-versus-Debug reason that #559 corrected in the skill. - `wait_frame` pointed at a README Windows note that is not in the README. - `Screen`'s Debug impl said "exactly like Display"; it adds `Screen(…)`. - Style, not a contradiction: `mask_matching` carried the crate's only `# Examples` heading; the rest of the crate's doctests have none. Found by a documentation sweep; the same sweep found the relative `current_dir` bug and the inspect help errors fixed in #561. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- README.md | 8 +++++--- SECURITY.md | 6 ++++-- crates/termlens/src/lib.rs | 5 +++-- crates/termlens/src/screen.rs | 18 ++++++++---------- crates/termlens/src/terminal.rs | 4 ++-- docs/DESIGN.md | 33 ++++++++++++++++++--------------- docs/LIMITATIONS.md | 4 +++- fixtures/emit/src/steps.txt | 3 ++- skills/termlens/SKILL.md | 15 +++++++++------ 9 files changed, 54 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index c5b1157..115cd0b 100644 --- a/README.md +++ b/README.md @@ -171,8 +171,10 @@ insta::assert_snapshot!(s.mask_matches(®ex::Regex::new(r"\d\d:\d\d:\d\d")?, ' `Screen::diff(&other)` reports what changed between two screens cell by cell — `changed_rows()`, `style_changes()`, and a rendering of only the rows that changed; `Screen::parse` reads a saved snapshot back, so the text -termlens prints — an insta `.snap`, the block a wait error leaves in a log, -what `termlens inspect` writes to stdout — is also its input format. +termlens prints — the block a wait error leaves in a log, what `termlens +inspect` writes to stdout, the body of an insta `.snap` below its `---` +header — is also its input format. The `termlens` command reads a whole +`.snap`, header included. ## What a test can see @@ -322,7 +324,7 @@ The short list; the full one, with the reason behind each entry, is Agents write terminal tests badly in predictable ways — a `sleep` where a wait belongs, a snapshot taken mid-repaint, `(row, col)` handed to a method that wants `(col, row)`. [`skills/termlens/SKILL.md`](https://github.com/vyncint/termlens/blob/main/skills/termlens/SKILL.md) -is the counter to each: the model, the rules, the API on one page and four +is the counter to each: the model, the rules, the API on one page and five recipes. Every Rust block in it is compiled against the crate in CI. ```sh diff --git a/SECURITY.md b/SECURITY.md index 4d3ee92..888f844 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -32,8 +32,10 @@ What the project does continuously, enforced by required CI on every change: - **Release integrity**: `v*` tags are ruleset-protected (admin-only), releases re-run the full CI gates, and publishing prefers crates.io Trusted Publishing (short-lived OIDC tokens) over stored secrets. -- **Provenance**: every commit requires a DCO sign-off from a human author - of record; bot-authored commits are rejected by CI. +- **Provenance**: every commit requires a DCO sign-off, and CI rejects + bot-authored commits except Dependabot's, which is exempt from the + identity rule only: its messages are checked like any other + (CONTRIBUTING.md, AI tooling policy). Resource-exhaustion notes for the paranoid: every buffer a child's output can reach is bounded, and a hostile child can at worst waste its own test's diff --git a/crates/termlens/src/lib.rs b/crates/termlens/src/lib.rs index 5ec8747..79b8572 100644 --- a/crates/termlens/src/lib.rs +++ b/crates/termlens/src/lib.rs @@ -216,8 +216,9 @@ pub use insta; /// Given a [`Screen`] instead of a terminal, the macro records it as it is /// (`after =` is refused: an instant has nothing to wait for). Failures come /// from insta unchanged; review them with `cargo insta review`. The macro -/// uses `?`, so the test returns [`Result`] — which every test should, since -/// the `Display` of every error carries the screen. +/// uses `?`, so the test returns [`Result`], as every test should: a failed +/// wait then ends it with its description and the screen printed (the +/// harness shows the error's `Debug` form, which carries both). /// /// `insta::assert_snapshot!(t.screen())` remains the low-level spelling for /// a screen already waited for by hand. diff --git a/crates/termlens/src/screen.rs b/crates/termlens/src/screen.rs index 7926b8c..50a7503 100644 --- a/crates/termlens/src/screen.rs +++ b/crates/termlens/src/screen.rs @@ -278,8 +278,6 @@ pub enum CursorShape { Bar, } -/// What an application copied with `OSC 52`, as observed at one snapshot. -/// /// What the emulator did not implement while producing a [`Screen`] — the /// view [`Screen::unsupported`] returns. /// @@ -399,6 +397,8 @@ impl PartialEq<&[&str]> for Unsupported<'_> { } } +/// What an application copied with `OSC 52`, as observed at one snapshot. +/// /// Read it from a snapshot via [`Screen::clipboard`]. A toast on screen /// proves the copy path ran; this proves the payload, which is usually the /// behaviour actually under test. @@ -1752,8 +1752,6 @@ impl Screen { /// With the `regex` feature, `mask_matches` takes a pattern instead; the /// two share one engine. /// - /// # Examples - /// /// Mask a clock so a snapshot stops changing on every run. The masked /// time keeps its eight columns, so nothing after it moves: /// @@ -2350,12 +2348,12 @@ impl fmt::Display for ScreenWithStyles<'_> { } impl fmt::Debug for Screen { - /// Deliberately compact: the header plus the rendered text, exactly like - /// [`Display`](fmt::Display). The derived alternative — thousands of - /// [`Cell`]s on one line — makes `Err(Error::Timeout { .. })` in a - /// `Result`-returning test unreadable (and long enough that CI log - /// pipelines drop the line entirely). Use [`Screen::cell`] to inspect - /// individual cells. + /// Deliberately compact: the header plus the rendered text, as + /// [`Display`](fmt::Display) writes them, inside `Screen(…)`. The + /// derived alternative — thousands of [`Cell`]s on one line — makes + /// `Err(Error::Timeout { .. })` in a `Result`-returning test unreadable + /// (and long enough that CI log pipelines drop the line entirely). Use + /// [`Screen::cell`] to inspect individual cells. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Screen({self})") } diff --git a/crates/termlens/src/terminal.rs b/crates/termlens/src/terminal.rs index f228568..f906644 100644 --- a/crates/termlens/src/terminal.rs +++ b/crates/termlens/src/terminal.rs @@ -3446,8 +3446,8 @@ impl Terminal { /// ``` /// /// (Unix in the example, because `wait_frame` is: ConPTY closes a DEC - /// 2026 bracket before the content it wrapped — see the README's Windows - /// note.) + /// 2026 bracket before the content it wrapped — see the Windows section + /// of `docs/LIMITATIONS.md`.) /// /// # Errors /// diff --git a/docs/DESIGN.md b/docs/DESIGN.md index e021994..a31c8db 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -120,12 +120,13 @@ released — the state lock and the writer lock are never held together. **Nothing writes to the child on a thread that cannot afford to wait.** Every write — query replies and typed input alike — is handed to a -dedicated writer thread over a bounded queue. Typed input carries an -acknowledgement channel, so the test thread applies the terminal's -deadline and returns `Error::Write` with the screen ("the application is -not reading its input") instead of blocking forever; there is no portable -way to ask whether a PTY write *would* block, since `POLLOUT` on a macOS -master reports writable and then blocks anyway. +dedicated writer thread over an unbounded queue, capped on reply bytes +(below). Typed input carries an acknowledgement channel, so the test +thread applies the terminal's deadline and returns `Error::Write` with the +screen ("the application is not reading its input") instead of blocking +forever; there is no portable way to ask whether a PTY write *would* +block, since `POLLOUT` on a macOS master reports writable and then blocks +anyway. Which failures are errors and which are panics is a deliberate split, not an accident of history. `send`/`send_str`/`paste`/`click`/`drag`/`scroll` @@ -193,11 +194,12 @@ the explicit "unsupported" reply, which is the half that matters: a capability we guessed at would be believed, while a refusal lets the application decide instead of wait. -Whatever remains unanswered (DECRQSS, the non-pixel `CSI t` reports, …) is -recorded, and the next wait timeout names it: "the application queried -the terminal (`^[[14t`) and received no answer" — a hang becomes a -diagnosis. `answer_queries(false)` mutes the responder for tests that -need a silent terminal; the diagnosis still works. +Whatever remains unanswered (DECRQSS, the `CSI t` reports other than the +text-area and pixel sizes, …) is recorded, and the next wait timeout names +it: "the application queried the terminal (`^[[14t`) and received no +answer" — a hang becomes a diagnosis. `answer_queries(false)` mutes the +responder for tests that need a silent terminal; the diagnosis still +works. ### Windows: ConPTY renders, and that decides what is claimed @@ -229,9 +231,10 @@ creates it without `PSEUDOCONSOLE_PASSTHROUGH_MODE`. Measured with signal instead (`EXIT_CLOSES_THE_TERMINAL`, `ExitWatch`). Each test a row above makes impossible is `#[cfg_attr(windows, ignore)]` -with that row as its reason; the README carries the user-facing list. The -`windows` workflow re-runs the probe and the whole suite on demand, and the -`windows-check` gate keeps the build compiling from a Linux runner. +with that row as its reason; `docs/LIMITATIONS.md` carries the user-facing +list. The `windows` workflow re-runs the probe and the whole suite on +demand, and the `windows-check` gate keeps the build compiling from a +Linux runner. ## 2. Wait semantics @@ -806,7 +809,7 @@ there must be `rows` of them; a file that does not hold together is an error, never a screen that panics on its first `cell()`. The compatibility corpus under `crates/termlens/tests/compat/` holds both -formats as written by each published release, and `tests/compat.rs` +formats as written by each release since 0.10.1, and `tests/compat.rs` holds every later release to reading them (#327). ### Masks diff --git a/docs/LIMITATIONS.md b/docs/LIMITATIONS.md index e48bcea..7e80777 100644 --- a/docs/LIMITATIONS.md +++ b/docs/LIMITATIONS.md @@ -68,7 +68,9 @@ CHANGELOG entry. [DESIGN.md](DESIGN.md) §2. - Some questions stay deliberately unanswered — kitty's `CSI ? u`, DECRQSS, DA3, XTVERSION, the `OSC 4` palette query, `OSC 12`, `OSC 52` *reads*, and - the non-pixel `CSI … t` reports — because a guessed reply is worse than + the other `CSI … t` reports, such as `11 t` and `19 t` (`18 t`, the + text-area size, is answered, and the pixel sizes `14 t` and `16 t` are + once a cell size is declared) — because a guessed reply is worse than none. An application blocked on one is **named in the next timeout** rather than left to hang unexplained. diff --git a/fixtures/emit/src/steps.txt b/fixtures/emit/src/steps.txt index 79f51b8..d96c884 100644 --- a/fixtures/emit/src/steps.txt +++ b/fixtures/emit/src/steps.txt @@ -1,6 +1,7 @@ TEXT literal text — any argument that is not a step below + and does not begin with `--`, which must be a step NL CR a newline / a carriage return ---text WORD literal text that happens to spell a step name +--text WORD literal text that begins with `--`, a step name or not --esc BYTES ESC followed by BYTES `--esc '(0'` is ESC ( 0 --csi BYTES ESC [ followed by BYTES `--csi '?2026h'` --raw SPEC bytes with escapes: \e \n \r \t \a \\ and \xNN diff --git a/skills/termlens/SKILL.md b/skills/termlens/SKILL.md index 443d0f2..ca94ba3 100644 --- a/skills/termlens/SKILL.md +++ b/skills/termlens/SKILL.md @@ -142,9 +142,10 @@ your test ── send(Key) · click · paste · resize ──▶ PTY └─ the grid; `screen.with_styles()` adds a `styles:` block that catches a colour regression. The one-liner that gets all three decisions right — wait, settle, styles — is `termlens::assert_screen_snapshot!(t, after = - |s| s.contains("Ready"))`. `.text()` drops the header and - `format!("{:?}")` is the same as `Display`. Review changes with `cargo - insta review`; never blind-accept with `INSTA_UPDATE=always`. + |s| s.contains("Ready"))`. `.text()` drops the header, and + `format!("{:?}")` wraps the same text in `Screen(…)`, which is not a + saved screen. 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 @@ -399,8 +400,10 @@ fn snapshot_diff_and_mask() -> termlens::Result<()> { ## 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 ---`). +A wait error's `Display` (a timeout, an EOF, an emulator failure) and a +failed write's end with the screen, under a header that says which screen +it is (`--- screen at timeout ---`, `--- final screen ---`); a spawn, size +or input error carries none. Read the first line for the cause. A test that fails through `?` or `unwrap()` prints the `Debug` form instead, which carries the same text in another shape: `timed out after 5s while waiting for X` is @@ -487,7 +490,7 @@ from_r, to_c, to_r)`, `scroll(col, row, Scroll::Down)`, `resize(cols, rows)`, | `unsupported()` / `insert_mode()` | an `Unsupported` view of the sequences the emulator did not implement (`^[[20h`…) — `is_empty()`, `contains("^[[5m")`, `iter()`, `overflow()`, and `assert_eq!(s.unsupported(), ["^[[59m"])` pins it — so a plausible grid can be told from a right one / IRM left on | | `with_styles()` | `ScreenWithStyles`, a `Display` with a `styles:` block; snapshot this to catch colour regressions | | `diff(&other)` | `ScreenDiff`: `is_empty()`, `cells()`, `changed_rows()`, `style_changes()`, and a `Display` of only the rows that changed | -| `mask_rect(cols, rows)` / `mask_matching(literal, fill)` / `mask_cells(pred)` | a new `Screen` with those cells replaced, styles and columns intact. `mask_matching` is a literal (rows included — it spans a wrap the way `find_all` does); `mask_cells` blanks by predicate | +| `mask_rect(cols, rows)` / `mask_matching(literal, fill)` / `mask_cells(pred)` | a new `Screen` with those cells replaced, styles and columns intact. `mask_matching` is a literal (a needle with `\n` spans rows, as `find_all`'s does; a value split by a soft wrap is two rows and is not matched); `mask_cells` blanks by predicate | | `to_ansi()` / `to_svg()` / `to_html()` | renderings a person can see — the SVG carries `role="img"` and a `` naming its size and the app's title; `Screen::parse(text)` reads the text format back | **Style** (`Copy`, public fields): `fg`, `bg` (`Color::Default` /