Repository navigation
Conversation
…protocol `pdfcraft-cli view <file.pdf> [--pages 1,3-5] [--fit page|width] [--pager|--no-pager] [--compression auto|fast|small] [--protocol auto|kitty] [--password PW]` draws pages in kitty, Ghostty, WezTerm and other terminals that speak the kitty graphics protocol: directly, over SSH, and inside tmux 3.3 or later. Unix only (macOS, Linux, FreeBSD); `render` without --out points to it. - Paging: more than one page on a terminal opens a pager on the alternate screen, like less: one page, or two side by side when the window is wide enough, turning like a book; less-like keys, fit page or width, a status line. A single page, --no-pager or output that isn't a terminal prints the pages at the cursor instead, like cat. - Turning a page is instant: two render threads work ahead (the next two screens and the one before), each page goes to the terminal as soon as it is ready, and turning to it only places it. The terminal keeps up to 12 pages and 192 MiB of them: past kitty's 320 MB quota it drops nearly every image without a placement at once. A page it drops anyway is answered (ENOENT) when placed, and sent again. - Transport: on this machine, pages go through temporary files of raw pixels (t=t, f=32) once a one-pixel probe shows the terminal takes them: kitty takes a page that way in about 10 ms, against 50-65 ms as a PNG in the stream. Otherwise PNGs, compressed fast on this machine and small over SSH (pdfcraft-engine's encode_png_with takes a PngEffort; Fast is 5-7 times faster for about twice the bytes). The files live in a 0700 directory of the user's own inside the temporary directory, are created new with mode 0600, and go once read, when stale, at exit, or by a later run once their process is gone. They are worked on by a thread of their own: a filesystem that stalls never stalls the keys. - Cell size: docker exec and kubectl exec pass rows and columns but no pixel size. view then asks the terminal once (CSI 16 t, with DA1 behind it so a terminal that doesn't know the query ends the wait at once), and assumes 10 x 20 px cells if it doesn't say; inside tmux, tmux says. - tmux: escape codes through passthrough (allow-passthrough turned on for view's own pane when off), pages as Unicode placeholders that tmux keeps as text. The pager asks tmux after writing and every second, and sends again what tmux threw away and what a hidden pane or a newly attached terminal missed. With more than one terminal showing the pane, no files. - Hostile input: nothing a document, the terminal or tmux sends can panic or hang it. Terminal answers are never read as keys; control characters are replaced in file names, messages and warnings; pages are capped at 8192 px a side and 16 MP; the terminal is restored on every exit path, panics included. Page lists (shared with edit, extract and split) are capped at 1,000,000 pages in all. Documented in docs/cli-view.md (usage) and apps/pdfcraft-cli/src/view/README.md (how it works, with measurements). Tests: unit and property tests per module; end to end on script(1) pseudo-terminals (paging, two-up, temporary files, terminal answers) and in private tmux servers (flips, a hidden pane, a second client), skipped with a warning where those can't run. Every fix from two security reviews was checked to fail its test with the fix reverted. Also adds my entry to contributors/people.toml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FredApps
left a comment
There was a problem hiding this comment.
Reviewed current head 35771c9 against base 9e5b70d. Two changes requested:
-
[P2] Do not permanently enable tmux passthrough as a side effect of viewing a PDF.
apps/pdfcraft-cli/src/view/tmux.rs:52-61 (ensure_passthrough), called from mod.rs::tmux_terminal.
When the user's effective allow-passthrough setting is off, this writes a pane-localonoverride and never restores it. This is not restricted to PDF images: the tmux option lets subsequent programs bypass tmux with wrapped terminal escape sequences. Thus opening a document changes the shell pane's behaviour after the viewer exits, including when output is redirected with --protocol kitty. Printing a notice after changing it does not preserve the user's explicit/inherited setting. Please leave an off setting unchanged by default and provide an actionable opt-in/setup message (or an explicit opt-in flag with properly scoped restoration). Test both an explicit off setting and inherited off, and verify no persistent override is left on normal/error exits. This is the default-setting question highlighted in the PR description, not an undisclosed implementation surprise. Reference: https://man.openbsd.org/tmux#allow-passthrough . -
[P2] Do not turn incomplete CSI replies into the pager's quit key after 50 ms.
apps/pdfcraft-cli/src/view/keys.rs:50-54 and pager.rs:145-176.
decode(b"\x1b[6;", false)correctly waits, but the next idle read uses flush=true and theNone if flushbranch produces Key::Escape, then decodes the remaining bytes as ordinary keys. With no typed page number, Nav::key(Escape) quits. A fragmented late cell-size/device-attributes reply, or a split arrow-key sequence over SSH, can therefore close the viewer without a quit key. Only a genuinely lone ESC should get this timeout interpretation; once a CSI prefix has been recognized, keep bounded sequence state or discard the incomplete sequence as a unit, rather than replaying it as keys. Please add a PTY regression with a CSI prefix, a delay longer than ESCAPE_WAIT, and the remaining reply; keep a control for the actual Escape key. Existing split graphics-answer coverage starts after the complete ESC_G prefix and does not cover this CSI case.
Code quality: Good separation of pure protocol/layout/navigation logic from I/O, clear resource caps, create-new owner-private temporary files, explicit cleanup paths, and substantial unit/property/PTY/tmux coverage. The additive PNG API also preserves the existing default encoding path. The remaining concerns are behavioural boundaries: respecting terminal configuration and keeping terminal replies distinct from user commands. No unrelated style rewrite is requested.
Validation: I read all production changes, the added tests, and relevant renderer/CLI dependencies. The live synthetic merge 7b780dc has exactly the stated base/head parents. All four hosted workflows currently await approval (action_required); no hosted passing result is established. Independent scoped Linux tests and a parent-authored fragmented-reply regression are in progress, not yet results. The two findings above are source-derived, not claims of an independently reproduced runtime failure. No native kitty/Ghostty/WezTerm/macOS/FreeBSD run was performed by me.
|
Independent Linux validation of unchanged head 35771c9 is complete. The fragmented-reply finding is now reproduced in a real pseudo-terminal, not just inferred from source. Using the existing view_pager.rs harness, after page 1/3 and its three image transmissions were ready, I sent ESC [ 6 ;, waited 300 ms without sending a quit key, and asserted that the child was still running. It had exited. The regression failed at the survival assertion (exit 101), including with process-local umask 0077. This is a Linux PTY result, not a native kitty/Ghostty/WezTerm or macOS/FreeBSD claim. Checks actually run:
The two original failures have a separate test-fixture explanation: view_pager.rs::tmpdir uses create_dir_all, so this machine's normal umask 0002 produced mode 0775. files.rs::own_dir correctly rejects a group-writable, non-sticky parent, hence no temporary-file probe. Re-running just those two unchanged tests in a child shell with umask 0077 created owner-only directories and both passed; the fragmented-reply regression still failed. No existing permissions, persistent settings, production code or assertions were changed. Please make the test-created temp directories explicitly private so these tests are independent of the invoking user's umask; do not weaken the production directory check. The initial failures and all controlled reruns are retained. This is not a claim that the full suite passed in the original environment, or that hosted CI ran. The added regression can be appended to the existing view_pager.rs harness: #[test]
fn parent_fragmented_cell_reply_does_not_quit_the_pager() {
if !pty_works() { return; }
let pdf = fixture("parent-fragmented-cell", 3);
let mut view = Session::start(&pdf, &["--protocol", "kitty", "--compression", "fast"], 80, 60);
view.wait_until("initial pages ready", |t| t.contains(" page 1/3 ") && transmitted(t) == 3);
view.press("\x1b[6;");
std::thread::sleep(Duration::from_millis(300));
assert!(view.child.try_wait().unwrap().is_none(), "a partial terminal cell-size reply quit the pager without a user quit key");
view.press("25;14t");
std::thread::sleep(Duration::from_millis(100));
assert!(view.child.try_wait().unwrap().is_none(), "the completed terminal reply quit the pager");
view.press("n");
view.wait_until("next page", |t| t.contains(" page 2/3 "));
view.press("q");
assert!(view.finish().success());
} |
Review of storytold#2920: when the pane's allow-passthrough was off, view turned it on for the pane and never put it back. The option lets any program in the pane send escape codes past tmux, not only view's images, so opening a PDF changed how the shell's pane behaves afterwards. view now only reads the pane's effective value (show-options -Ap). When it is off and stdout is the pane, view stops with the command that turns it on for that pane, naming it (tmux set -p -t %N allow-passthrough on: run from a shell without -t, tmux sets the session's active pane, which may be another), or set -g allow-passthrough on for ~/.tmux.conf. Output to a file gets the same text as a warning, and is written. Test: in a private tmux server with tmux's defaults, view under an inherited off, an off of the pane's own, an inherited off with output to a file, and an on of the pane's own: it stops, stops, writes and prints, and the pane's own setting and the global one are unchanged every time. It fails against the previous commit. The other tmux tests now start their server with allow-passthrough on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of storytold#2920: the pager took an escape sequence still cut off after 50 ms (ESCAPE_WAIT) as the Esc key, and its other bytes as keys. So a late cell-size or device-attributes answer, or an arrow key over SSH, that arrived in two parts further apart than that quit the pager. Now only a lone ESC with nothing after it is the Esc key. Past the ESC, a cut-off CSI, SS3 or ESC _ waits for the rest, up to the pager's existing bounds (1 s, 1 KiB), and is dropped whole if it never comes. Tests: a unit test cuts the cell-size and device-attributes answers, and both together, at every point past an ESC (no keys) and Page Down in two (Page Down); on a pseudo-terminal, ESC [ 6 ; then 250 ms later 25;14t, and ESC [ then ?62;c, leave the pager open with no page number typed, ESC [ 6 then ~ turns the page, and Esc alone still quits. Both fail against the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of storytold#2920: under a umask of 0002, common on Linux, the directory view_pager.rs makes for view's temporary files came out 0775. view rightly refuses a parent that others can write to without the sticky bit, so no file probe went out, and the two tests that expect one failed. The test now sets the directory to 0700; the check in files.rs is unchanged. Both tests fail under umask 0002 without this, and pass with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for both reviews and the Linux validation. Everything is fixed in three commits on top of 35771c9, so each change is easy to see:
The pane is named because an untargeted New end-to-end test
In every case the pane's own Cut-off CSI. Only a lone ESC, with nothing after it for Tests:
Both fail with the fix reverted. Your umask. Reproduced on macOS: under What's left:
|
FredApps
left a comment
There was a problem hiding this comment.
Re-reviewed exact head eba6af3, including the changes since my earlier review.
The earlier blockers are addressed: view now reads tmux's passthrough setting without changing it; an incomplete CSI sequence past its ESC is not treated as a quit key; and the temporary-directory test fixture now establishes its required owner-only permissions independently of the caller's umask.
Independent Linux validation at this head:
- View unit tests: 84 passed (5 unrelated tests filtered).
- view integration: 11 passed.
- view_pager: 12 passed.
- Private-server view_tmux: 4 passed.
- The unchanged independently supplied fragmented-CSI regression that failed on the earlier head: 1 passed.
- Repository-config formatting and scoped CLI Clippy, all targets with -D warnings: passed.
The ordinary run used umask 0002 with TMPDIR unset. Actual PTY availability was confirmed; there were no PTY/tmux skip warnings or ignored tests. Both former fixture failures and the unchanged regression also passed individually under umask 0077. Production source remained unchanged and the PR head was verified before and after testing.
No remaining blocker found in this revision. This does not claim a full workspace gate, native kitty/Ghostty/WezTerm visual qualification, or macOS/FreeBSD testing.
What this changes
pdfcraft-cli view report.pdfdraws PDF pages straight into the terminal, through the kitty graphics protocol (kitty, Ghostty, WezTerm and others): on macOS, Linux and FreeBSD, directly, over SSH and inside tmux 3.3 or later.pdfcraft-cli-view.mp4
less. More than one page opens a pager on the alternate screen: one page at a time, or two side by side when the window is wide enough, turning like a book. It has less-like keys, fits the page or the width, and follows the window when it is resized or split.cat. A single page (--pages 3),--no-pager, or output to a file (with--protocol kitty) prints at the cursor instead. The page stays in the scrollback, like text.docker execandkubectl exec. These pass rows and columns but no pixel size, soviewasks the terminal how big its cells are (CSI 16 t) once at start. Pages come out as sharp as over SSH.pdfcraft-engine. It gainsPngEffort,encode_png_withandunpremultiply. These are additions only:encode_pngis unchanged.Usage is in
docs/cli-view.md. How it works inside, with the measurements behind each choice, is inapps/pdfcraft-cli/src/view/README.md.DISCLAIMER: almost entirely coded with Claude Code, Opus 5.5 and Codex (mostly for reviews).
How you verified it
cargo fmt --check-D warningsonpdfcraft-cliandpdfcraft-engine, on macOS and Linuxpdfcraft-cli179/179,pdfcraft-engine285/285pdfcraft-cli182/182 in each of three runs,pdfcraft-engine285/285cargo xtask layersandcargo xtask assetsTests, at every scale:
script(1)pseudo-terminals. A fake kitty-like terminal on the other side covers paging, two-up, temporary files, terminal answers and the cell-size query.-L): page flips, a hidden pane, a second client.By hand:
docker exec.docker execinto a Debian 13 container with tmux 3.5a.Never-crash
unwrap/expect/panic!/unreachable!/todo!outside tests, unless provably infallible with a comment saying whyunsafe:rustixgives safetermios, and shared memory (t=s) is not used because it would needmmapProvenance
CSI 16 t, DA1);cargo xtask assetspassesSecurity
--rootis unaffected.viewreads the one file named on its command line, asrenderdoes, and dispatches no tools.Parity and reach
parity/: n/a, since Acrobat has no terminal viewerautomation: n/a, sincepage_renderalready renders pages headlessly andviewis a terminal displaydocs/cli-view.md, the module README,README.md,docs/architecture.md, and a line inROADMAP.mdAnything a reviewer should look at first
allow-passthrough.When it is off,Without it nothing draws.viewturns it on for its own pane only (tmux set-option -p allow-passthrough on) and says so on stderr.The setting stays on that pane afterChanged after review:viewexits. Refusing with a message would be the other choice.viewnow refuses with a message (see the updates below).viewwritesCSI 16 t+ DA1 to the terminal at start. DA1 ends the wait at once on a terminal that doesn't know the query, and an answer that arrives late is ignored, never read as a key. Not when it arrived cut in two: fixed after review (see the updates below).files.rs). Check the safety checks on the directory, and the cleanup of another run's stale files once its process is gone.page_listis shared withedit,extractandsplit. It already capped a single range at 1,000,000 pages, and now caps the whole list too, since repeating a range used to multiply the allocation. Those commands now refuse such lists.Updates after review
viewleavesallow-passthroughalone. It only reads the setting. When it is off,viewstops with the command that turns it on for that pane (tmux set -p -t %N allow-passthrough on), orset -g allow-passthrough onfor~/.tmux.conf. Output to a file gets the same text as a warning. Test: in a private tmux server, with the setting inherited off, off for the pane, and on, the pane's own value and the global one are unchanged afterwards.ESC, with nothing after it for 50 ms, is the Esc key. A sequence cut off after itsESCwaits up to 1 s for the rest, and is dropped whole if the rest doesn't come. Tests: a unit test, and a pseudo-terminal test that sendsESC[6;, then25;14t250 ms later.viewrefuses a parent directory others can write to, as it should.docs/cli-view.mdand the module README are updated to match.pdfcraft-cli183/183 (under umask 0002); Linux 186/186 in each of three runs (two under umask 0002).pdfcraft-engineis unchanged.🤖 Generated with Claude Code