diff --git a/AGENTS.md b/AGENTS.md index 5246c90..d91588e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,7 @@ Two halves that fill each other's gaps: **tmux must win backend detection whenever `$TMUX` is set.** Inside kitty running tmux, `kitty @ get-text` returns tmux's *rendered viewport*, not the shell's scrollback, and tmux -swallows the OSC 133 prompt marks that kitty's `last_cmd_output` depends on. Asking kitty +swallows the OSC 133 prompt marks that kitty's output extents depend on. Asking kitty there returns plausible but wrong text. Order lives in `backend::detect`. **Scrollback coordinates are absolute, never pane-relative.** `capture-pane -S/-E` are @@ -45,14 +45,34 @@ it, tcap installs its own trap. Both branches are covered by **Command text reaches the binary as a single argv element**, so quoting, backslashes and newlines need no escaping. -**Only tmux can reach past the most recent command.** kitty exposes a `last_cmd_output` +**Only tmux can reach past the most recent command.** kitty exposes a latest-output extent and nothing older; iTerm2 exposes only its latest prompt. `-c 2` on those must fail with a pointer to tmux, never return the wrong block. +**kitty has three traps, all found in the field rather than by tests.** `get-text` must be +asked for `last_non_empty_output`, not `last_cmd_output`: kitty counts the running `tcap` as +the current command, so the latter returns tcap's own empty output. `--match id:$KITTY_WINDOW_ID` +is required or kitty reads the *focused* window, which need not be the one tcap runs in — and +`--match` is a `get-text` option, so it must follow the subcommand. Remote control works over +either the escape-code channel or `--to $KITTY_LISTEN_ON`; under `allow_remote_control +socket-only` only the socket works, so the error must advise `listen_on`, not +`allow_remote_control yes`. + +**The capture marker records that tcap's own text reached the screen**, so the next capture on +a boundary-less backend can refuse rather than return it. It is written only where captured +*content* is printed — not for hints or errors, which are not that text, and marking those made +`tcap --output | llm` refuse the following capture, with the outcome flipping on `--quiet`. + +One case it cannot see: `tcap --output | llm` pipes tcap's stdout, but the downstream tool +writes its answer to the terminal, so kitty's last non-empty output is that answer with no +marker set. The next capture proceeds and is labelled approximate rather than refused. tcap +cannot observe what a downstream process printed, so this is a limitation to state, not a check +to add. + ## Verification status - **tmux** — fully working, covered by end-to-end tests. -- **kitty** — implemented against `--extent=last_cmd_output`, verified present in kitty 0.48.2. +- **kitty** — implemented against `--extent=last_non_empty_output`, verified live in kitty 0.48.2. - **iTerm2** — written to the documented Python API but **never run**; iTerm2 was not installed on the machine it was written on. Treat as unverified. - **WezTerm** — detection only. Deliberately not implemented: there is no verified way to map diff --git a/Cargo.lock b/Cargo.lock index e3dc2c9..53a5bfe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -144,6 +144,12 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" +[[package]] +name = "libc" +version = "0.2.189" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" + [[package]] name = "memchr" version = "2.8.3" @@ -245,10 +251,11 @@ dependencies = [ [[package]] name = "tcap" -version = "0.1.0" +version = "0.1.1" dependencies = [ "anyhow", "clap", + "libc", "serde", "serde_json", "toml", diff --git a/Cargo.toml b/Cargo.toml index 96c1e7d..851fb3a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "tcap" -version = "0.1.0" +version = "0.1.1" edition = "2021" description = "Capture the last command and its output from your terminal" license = "MIT" @@ -12,6 +12,7 @@ path = "src/main.rs" [dependencies] anyhow = "1" clap = { version = "4", features = ["derive", "env"] } +libc = "0.2" serde = { version = "1", features = ["derive"] } serde_json = "1" toml = "0.8" diff --git a/README.md b/README.md index f6412a6..6c56e2e 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,8 @@ tcap --output # output only, no header tcap --command # the command text only tcap --raw # verbatim, ANSI colour intact, no header tcap --json # {command, exit_code, cwd, duration_ms, output, source} + # plus "approximate": true when the backend cannot vouch + # that the output belongs to that command tcap --copy # also copy to the clipboard ``` @@ -140,7 +142,7 @@ Norway problem, where `quiet: no` silently parses as `false`. | Terminal | Last command | Older commands (`-c 2`) | Notes | |---|---|---|---| | **tmux** | yes | **yes** | The only backend that can reach back | -| **kitty** | yes | no | Needs `allow_remote_control yes` | +| **kitty** | yes | no | Needs remote control (`yes`, or `socket-only` + `listen_on`) | | **iTerm2** | yes | no | Needs Shell Integration + `pip install iterm2` | | **WezTerm** | not yet | no | Detected; run inside tmux | | Ghostty, Terminal.app, Alacritty | no | no | No remote-control API — run inside tmux | @@ -149,13 +151,26 @@ Two things worth knowing: **tmux wins whenever it is running.** Inside kitty running tmux, asking kitty for its scrollback returns tmux's *rendered viewport*, not the shell's real history — -and tmux swallows the prompt marks kitty's `last_cmd_output` depends on. So tmux +and tmux swallows the prompt marks kitty's output extents depend on. So tmux is always treated as authoritative when it is in the stack. -**Only tmux can reach past the most recent command.** kitty exposes a -`last_cmd_output` extent and nothing for older commands; iTerm2 exposes only its -latest prompt. `tcap -c 2` therefore fails with a pointer to tmux rather than -silently returning the wrong block. +**Only tmux can reach past the most recent command.** kitty exposes only the +latest output and nothing older; iTerm2 exposes only its latest prompt. `tcap -c +2` therefore fails with a pointer to tmux rather than silently returning the +wrong block. + +**On kitty, output is the last *non-empty* output.** kitty counts the running +`tcap` as the current command, so it has to be asked for the last output that +wasn't empty. The consequence: if your last command printed nothing, kitty +returns an *older* command's output while the header names the last one. The +annotated header carries a `# note:` line saying so, and `--json` sets +`"approximate": true`. Two `tcap` runs in a row are a **hard error** rather than +a label, since there the mismatch is certain. tmux has no such +ambiguity, because the shell hook records real boundaries. kitty also +needs remote control reachable — either `allow_remote_control yes`, or +`socket-only` with a `listen_on` socket in a directory only you can open +(`${TMPDIR}` on macOS, `${XDG_RUNTIME_DIR}` on Linux). A socket in `/tmp` is +reachable by anyone with write permission on it. When something is missing, `tcap` says what and why: @@ -177,9 +192,10 @@ and the next begins. `tcap` fills those gaps from two sides: - **Shell hooks** (`tcap init`) record the command text, exit code, cwd and duration on every prompt. -- **Terminal adapters** retrieve the actual text. kitty and iTerm2 know their own - command boundaries; tmux does not (it has no OSC 133 support), so the hook also - records where output starts and ends. +- **Terminal adapters** retrieve the actual text. tmux has no OSC 133 support, so + the hook also records where output starts and ends — which is why it is the + only backend with exact boundaries. iTerm2 tracks its own prompts; kitty can + only return its last non-empty output. Those tmux coordinates are stored as `history_size + cursor_y`, which is stable under scrolling: as lines scroll off, `history_size` grows by exactly as much as @@ -189,7 +205,7 @@ under scrolling: as lines scroll off, `history_size` grows by exactly as much as ## Development ```sh -cargo test # 49 unit + 11 end-to-end tests +cargo test # 61 unit + 11 end-to-end tests git config core.hooksPath .githooks # run fmt, clippy and tests before every push ``` diff --git a/npm/README.md b/npm/README.md index f8baa94..853364c 100644 --- a/npm/README.md +++ b/npm/README.md @@ -84,6 +84,8 @@ tcap --output # output only, no header tcap --command # the command text only tcap --raw # verbatim, ANSI colour intact tcap --json # {command, exit_code, cwd, duration_ms, output, source} + # plus "approximate": true when the backend cannot vouch + # that the output belongs to that command tcap --copy # also copy to the clipboard tcap --max-bytes 8000 # elide the middle to fit a smaller context window ``` @@ -96,17 +98,23 @@ unrelated jobs. | Terminal | Last command | Older (`-c 2`) | Needs | |---|---|---|---| | **tmux** | yes | **yes** | nothing | -| **kitty** | yes | no | `allow_remote_control yes` | +| **kitty** | yes | no | remote control: `yes`, or `socket-only` + `listen_on` | | **iTerm2** | yes | no | Shell Integration + `pip install iterm2` | | **WezTerm** | not yet | no | run inside tmux | | Ghostty, Terminal.app, Alacritty | no | no | run inside tmux | Two things worth knowing up front: -**Only tmux can reach past the most recent command.** kitty exposes a -`last_cmd_output` extent and nothing older; iTerm2 exposes only its latest -prompt. So `tcap -c 2` fails there with a pointer to tmux rather than quietly -returning the wrong block. +**Only tmux can reach past the most recent command.** kitty exposes only the +latest output and nothing older; iTerm2 exposes only its latest prompt. So `tcap +-c 2` fails there with a pointer to tmux rather than quietly returning the wrong +block. + +**On kitty, output is the last *non-empty* output**, because kitty counts the +running `tcap` as the current command. A command that printed nothing therefore +yields an older output under the current header; it is labelled with a `# note:` +line and `"approximate": true` in `--json`. Two `tcap` runs in a row are a hard +error instead, since the mismatch is certain there. tmux has no such ambiguity. **tmux wins whenever it's running.** Inside kitty running tmux, asking kitty for its scrollback returns tmux's rendered viewport rather than the shell's real diff --git a/npm/package.json b/npm/package.json index de7459e..720365e 100644 --- a/npm/package.json +++ b/npm/package.json @@ -1,6 +1,6 @@ { "name": "@agora-build/tcap", - "version": "0.1.0", + "version": "0.1.1", "description": "Capture the last command and its output from your terminal, ready to pipe into an LLM", "bin": { "tcap": "bin/tcap" diff --git a/src/backend/iterm2.rs b/src/backend/iterm2.rs index e4d403c..2536c8b 100644 --- a/src/backend/iterm2.rs +++ b/src/backend/iterm2.rs @@ -49,17 +49,19 @@ impl ITerm2 { Self } - /// Materialise the shim next to our state so we are not re-writing it into - /// a world-writable temp path on every call. + /// Materialise the shim under the state directory. + /// + /// Goes through the same ownership-checked, `O_NOFOLLOW` path as every other + /// state file. This one matters more than most: the file is handed straight + /// to `python3`, so a symlink or a pre-created file here would be code + /// execution, not just a truncated file. fn shim_path() -> Result { - let dir = crate::state::state_dir(); - std::fs::create_dir_all(&dir)?; - let path = dir.join("iterm2_capture.py"); - let stale = std::fs::read_to_string(&path) + let path = crate::state::state_dir().join("iterm2_capture.py"); + let stale = crate::state::secure_read_file(&path) .map(|existing| existing != SHIM) .unwrap_or(true); if stale { - std::fs::write(&path, SHIM)?; + crate::state::secure_write(&path, SHIM.as_bytes())?; } Ok(path) } @@ -108,6 +110,15 @@ impl Backend for ITerm2 { "iterm2" } + /// Assumed, not verified. `async_get_last_prompt` may resolve to the prompt + /// tcap is itself running under — the same trap that made kitty's + /// `last_cmd_output` return tcap's own empty output — in which case this + /// should be `false` and captures should be labelled approximate. Nobody has + /// run this backend; `tcap doctor` under iTerm2 is what would settle it. + fn has_command_boundaries(&self) -> bool { + true + } + fn fetch(&self, index: usize, _rec: Option<&Record>, raw: bool) -> Result { if index > 1 { return Err(anyhow!( diff --git a/src/backend/kitty.rs b/src/backend/kitty.rs index a6aa9cf..7a179ea 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -1,14 +1,15 @@ -//! kitty backend. +//! kitty backend, via `kitty @ get-text` and kitty's own shell-integration +//! prompt marks. Verified against kitty 0.48.2. //! -//! kitty is the one terminal that ships this feature outright: -//! `kitty @ get-text --extent=last_cmd_output` returns exactly the last -//! command's output, using kitty's own shell-integration prompt marks. Verified -//! against kitty 0.48.2, whose help states the extent "requires -//! shell_integration to be enabled". +//! Two things this backend cannot do: //! -//! The limitation is that kitty exposes only the *latest* command block. There -//! is no extent for "the one before that", so `-c 2` cannot be served here and -//! fails with a pointer to tmux rather than silently returning the wrong text. +//! - Reach past the latest output. There is no extent for "the command before +//! that", so `-c 2` fails with a pointer to tmux rather than returning the +//! wrong text. +//! - Tie output to a specific command. It asks for the last *non-empty* output +//! (see `fetch` for why), so when the last command printed nothing, kitty +//! returns an older command's output while the header names the last one. +//! tmux has no such ambiguity, having real boundaries. use anyhow::{anyhow, Result}; @@ -84,6 +85,196 @@ impl Kitty { .ok() .filter(|s| !s.is_empty()) } + + /// The socket recommendation, given once so the three places that offer it + /// cannot drift apart. + /// + /// Two things this text has to get right, because it exists to be pasted + /// into kitty.conf: + /// + /// - **No trailing comments.** kitty takes the rest of the line as the + /// value; `#` only opens a comment at the start of a line. Verified: a + /// `listen_on unix:/tmp/sock # note` produced a socket literally named + /// `/tmp/sock # note-69755`. + /// - **One `listen_on` line.** Last one wins, so offering both platforms' + /// lines would hand a macOS user the Linux one — where + /// `$XDG_RUNTIME_DIR` is a systemd convention that is normally unset and + /// expands to nothing, putting the socket at the filesystem root. + fn socket_advice() -> String { + // macOS $TMPDIR is already per-user and 0700; $XDG_RUNTIME_DIR is the + // equivalent on Linux. + let dir = if cfg!(target_os = "macos") { + // Already per-user and 0700. + Some("${TMPDIR}".to_string()) + } else { + // Only when it is actually set: it is a systemd convention, absent + // on non-systemd distros, in containers and under some su/ssh paths, + // where it would expand to nothing and put the socket at /. + std::env::var("XDG_RUNTIME_DIR") + .ok() + .filter(|v| !v.is_empty()) + .map(|_| "${XDG_RUNTIME_DIR}".to_string()) + }; + + let line = match &dir { + Some(d) => format!(" listen_on unix:{d}/kitty-{{kitty_pid}}"), + // Still one paste-safe line: kitty.conf does no command + // substitution either, so `$(id -u)` would stay literal. The caveat + // belongs in the prose below, not on the line itself. + None => " listen_on unix:/run/user/1000/kitty-{kitty_pid}".to_string(), + }; + + format!( + " allow_remote_control socket-only\n{line}\n{note}\ + Keep the socket in a directory only you can open. One in /tmp is reachable by \ + anyone with write permission on it, which under a group-writable umask is more \ + than just you. Note kitty.conf has no trailing comments and no command \ + substitution: everything after the value is part of it, literally.", + note = if dir.is_none() { + "$XDG_RUNTIME_DIR is unset here, so the path above is a guess — replace 1000 \ + with your uid, or use any directory only you can open.\n" + } else { + "" + } + ) + } + + /// Turn kitty's refusal into the fix for the mode actually in force. + /// + /// `allow_remote_control` has several values needing different answers: + /// telling a `socket-only` user to set `yes` is wrong advice, and the real + /// cause — no socket to talk to — is not obvious from kitty's message. + fn remote_control_hint(err: &anyhow::Error, listen_on: &Option) -> String { + let msg = err.to_string().to_lowercase(); + + if msg.contains("password") { + // Deliberately not `remote_control_password "" get-text`: that would + // remove authentication from the one call that dumps terminal + // contents, for every process that can reach the socket, to work + // around tcap's own limitation. Better to be unsupported. + return format!( + "kitty is requiring a remote-control password, which tcap cannot send, so \ + this setup is unsupported.\n\ + Rather than weaken authentication for `get-text` — which returns whatever \ + is on screen, credentials included — run inside tmux, which needs no kitty \ + remote control at all. If you would rather use a socket:\n{}", + Self::socket_advice() + ); + } + + if msg.contains("no matching") || msg.contains("no window") { + return "kitty could not find the window tcap asked for.\n\ + $KITTY_LISTEN_ON probably points at a different kitty instance — a \ + `listen_on` without {kitty_pid} is shared between instances, so window ids \ + from one do not exist in another. Give each instance its own socket:\n" + .to_string() + + &Self::socket_advice(); + } + + // Checked after `password`, so a password refusal that happens to name + // the socket it arrived on is not answered with "restart kitty". + // + // Matched on the bare word rather than a phrase: kitty 0.48.2 says + // "Remote control is allowed over a socket only" (captured live), but + // word order is not worth depending on across versions, and by here we + // already know it is a remote-control failure. + if msg.contains("socket") { + return match listen_on { + // "socket" is one word out of kitty's message, so this is an + // inference, not a reading of the config — hedged accordingly. + None => format!( + "This looks like `allow_remote_control socket-only` with \ + $KITTY_LISTEN_ON unset, leaving no socket to use.\n\ + Add to ~/.config/kitty/kitty.conf and restart kitty:\n{}\n\ + If it is set in your shell but not here, something in between is \ + clearing the environment.", + Self::socket_advice() + ), + Some(sock) => format!( + "the socket at {sock} was refused. Restart kitty so the running \ + instance and $KITTY_LISTEN_ON agree." + ), + }; + } + + format!( + "If this is a remote-control problem, kitty needs it enabled. In \ + ~/.config/kitty/kitty.conf either:\n allow_remote_control yes\nor keep it \ + restricted and give tcap a socket:\n{}\nThen restart kitty. If the message \ + above says something else, that is the real cause.", + Self::socket_advice() + ) + } + + /// `--match id:N` for the window tcap is running in. + /// + /// Without it kitty reads whichever window is *focused*, which is not + /// necessarily this one — a capture from a script, or with focus on another + /// split, returns that window's text instead. Callers must treat `None` as + /// fatal rather than omitting the flag: the fallback would put another + /// pane's scrollback, secrets included, under this command's header. + /// + /// The guarantee is bounded: `id:N` is instance-local, so it identifies this + /// window within whichever instance `--to` reaches. A stale or shared + /// $KITTY_LISTEN_ON could still point at a different instance. + /// + /// `--match state:self` was measured as an alternative and rejected: it + /// resolves the invoking window correctly even when unfocused, but still + /// fails without $KITTY_WINDOW_ID ("No matching windows for expression: + /// state:self"), so it hides the same dependency behind a worse message. + /// + /// The id is parsed as a number so a poisoned variable cannot widen the + /// match expression, and so a malformed one is reported as such instead of + /// surfacing later as an unexplained remote-control failure. + fn window_match() -> Option { + Self::parse_window_id(&std::env::var("KITTY_WINDOW_ID").ok()?) + } + + /// Split out so the numeric guarantee is testable without touching the + /// environment, which is racy across threads. + fn parse_window_id(raw: &str) -> Option { + raw.trim().parse::().ok().map(|id| format!("id:{id}")) + } + + fn require_window() -> Result { + Self::window_match().ok_or_else(|| { + anyhow!( + "$KITTY_WINDOW_ID is missing or not a number, so tcap cannot tell kitty\n\ + which window to read.\n\n\ + Without it kitty returns whichever window is focused, which may be a\n\ + different pane — tcap refuses rather than risk showing you another\n\ + window's output, which could contain secrets.\n\n\ + kitty sets this automatically; if it is missing, the environment was\n\ + cleared somewhere between kitty and this shell." + ) + }) + } + + /// The full `get-text` argv, shared by `fetch` and `diagnose`. + /// + /// Built once so the two cannot drift: if they disagreed, `doctor` would + /// report the capture path healthy while `fetch` failed — the worst outcome + /// for a tool whose job is telling you what is wrong. + fn get_text_args<'a>( + &self, + listen_on: &'a Option, + window: &'a str, + ansi: bool, + ) -> Vec<&'a str> { + let mut args = self.rc_args(listen_on); + args.push("get-text"); + // `--match` is a get-text option, not a global one: placed before the + // subcommand kitty rejects it with "Unknown option: --match". + args.extend_from_slice(&["--match", window]); + // `last_non_empty_output`, not `last_cmd_output`: kitty counts the + // running tcap as the current command, so the latter returns tcap's own + // (empty) output. That extent is for a keybinding pressed at the prompt. + args.extend_from_slice(&["--extent", "last_non_empty_output"]); + if ansi { + args.push("--ansi"); + } + args + } } impl Backend for Kitty { @@ -91,64 +282,221 @@ impl Backend for Kitty { "kitty" } + /// kitty returns the last non-empty output, which need not belong to the + /// command tcap's shell record names. + fn has_command_boundaries(&self) -> bool { + false + } + fn fetch(&self, index: usize, _rec: Option<&Record>, raw: bool) -> Result { if index > 1 { return Err(anyhow!( "kitty can only report its most recent command's output, so -c {index} \ is not available here.\n\n\ - kitty exposes a `last_cmd_output` extent but nothing for older\n\ + kitty exposes only the latest output and nothing for older\n\ commands. To reach further back, run inside tmux." )); } let listen_on = Self::listen_on(); - let mut args = self.rc_args(&listen_on); - args.extend_from_slice(&["get-text", "--extent", "last_cmd_output"]); - if raw { - args.push("--ansi"); + let window = Self::require_window()?; + let args = self.get_text_args(&listen_on, &window, raw); + + let text = run(&self.exe, &args) + .map_err(|e| anyhow!("{e}\n\n{}", Self::remote_control_hint(&e, &listen_on)))?; + + // `run` succeeding with an empty body would render as a bare header with + // nothing under it — the exact symptom this backend was fixed for — so + // it fails with the same explanation doctor gives. + // Only newlines count as nothing: `printf ' \n'` is real output, and + // --raw/--output promise the exact bytes. + if text.chars().all(|c| c == '\n' || c == '\r') { + return Err(anyhow!( + "kitty returned no output for this window.\n\n\ + Nothing has printed here yet, or kitty's own shell integration is off.\n\ + Run a command that prints something and try again; if it stays empty,\n\ + check `shell_integration` in ~/.config/kitty/kitty.conf." + )); } - run(&self.exe, &args).map(Fetched::from).map_err(|e| { - anyhow!( - "{e}\n\n\ - kitty remote control may be disabled. Add to ~/.config/kitty/kitty.conf:\n \ - allow_remote_control yes\n\ - and make sure shell integration is enabled (it is by default)." - ) - }) + Ok(Fetched::from(text)) } fn diagnose(&self) -> Vec { let mut d = vec![Diagnostic::ok("kitty binary", self.exe.clone())]; let listen_on = Self::listen_on(); + let channel = match &listen_on { + Some(sock) => format!("socket {sock}"), + None => "escape-code channel (no $KITTY_LISTEN_ON)".to_string(), + }; + let mut args = self.rc_args(&listen_on); args.push("ls"); - - d.push(match run(&self.exe, &args) { - Ok(_) => Diagnostic::ok("remote control", "enabled and reachable"), + // Probed once: running it twice costs a second round trip and lets the + // two results disagree, so the get-text probe could be skipped while + // remote control was reported healthy, or vice versa. + let ls = run(&self.exe, &args); + let reachable = ls.is_ok(); + d.push(match ls { + Ok(_) => Diagnostic::ok("remote control", format!("reachable via {channel}")), + // The underlying error is kept: the hint is a guess from keywords, + // and for anything it does not recognise the real message is the + // only useful thing doctor can say. Err(e) => Diagnostic::bad( "remote control", - format!("{e} — set `allow_remote_control yes` in kitty.conf"), + format!( + "{e}\n via {channel}\n {}", + Self::remote_control_hint(&e, &listen_on) + ), ), }); - // The real test of shell integration is whether the extent resolves. - let mut probe = self.rc_args(&listen_on); - probe.extend_from_slice(&["get-text", "--extent", "last_cmd_output"]); - d.push(match run(&self.exe, &probe) { - Ok(_) => Diagnostic::ok("shell integration", "last_cmd_output resolves"), - Err(e) => Diagnostic::bad( - "shell integration", - format!("{e} — kitty needs shell_integration enabled for this extent"), - ), - }); + match Self::window_match() { + Some(w) => { + d.push(Diagnostic::ok("window", w.clone())); + + // Probe exactly what fetch runs, so doctor cannot report a path + // healthy that fetch would fail on. Only the probe is skipped + // when remote control is already down — an early return here + // would also drop the diagnostics below it. + if reachable { + let probe = self.get_text_args(&listen_on, &w, false); + d.push(match run(&self.exe, &probe) { + // Empty is also what a brand-new window returns, so this + // must not call a healthy setup broken — doctor is only + // useful if its verdicts can be trusted. + Ok(t) if t.trim().is_empty() => Diagnostic::bad( + "shell integration", + "no output to read yet. Run a command that prints something and \ + re-run doctor; if it is still empty, check `shell_integration` \ + in kitty.conf", + ), + Ok(_) => { + Diagnostic::ok("shell integration", "last_non_empty_output resolves") + } + Err(e) => Diagnostic::bad("shell integration", e.to_string()), + }); + } + } + None => d.push(Diagnostic::bad( + "window", + "$KITTY_WINDOW_ID missing or not a number — tcap will refuse to capture \ + rather than read whichever window is focused", + )), + } d.push(Diagnostic::bad( "history depth", - "kitty serves only the latest command; -c 2 and beyond need tmux", + "kitty serves only the latest output; -c 2 and beyond need tmux", )); d } } + +#[cfg(test)] +mod tests { + use super::*; + + fn kitty() -> Kitty { + Kitty { + exe: "kitten".into(), + } + } + + /// Locks in the two argv facts that were wrong in the field: the extent, and + /// `--match` following the subcommand rather than preceding it. + #[test] + fn get_text_argv_order_and_extent() { + let k = kitty(); + let args = k.get_text_args(&None, "id:7", false); + assert_eq!( + args, + vec![ + "@", + "get-text", + "--match", + "id:7", + "--extent", + "last_non_empty_output" + ] + ); + + let sub = args.iter().position(|a| *a == "get-text").unwrap(); + let m = args.iter().position(|a| *a == "--match").unwrap(); + assert!(m > sub, "--match is a get-text option, not a global one"); + assert!( + !args.contains(&"last_cmd_output"), + "last_cmd_output returns tcap's own empty output" + ); + } + + #[test] + fn socket_is_targeted_before_the_subcommand() { + let k = kitty(); + let sock = Some("unix:/tmp/kitty-1".to_string()); + let args = k.get_text_args(&sock, "id:7", false); + assert_eq!(&args[..3], &["@", "--to", "unix:/tmp/kitty-1"]); + assert_eq!( + args[3], "get-text", + "--to is global, so it precedes get-text" + ); + } + + #[test] + fn ansi_only_when_raw() { + let k = kitty(); + assert!(!k.get_text_args(&None, "id:1", false).contains(&"--ansi")); + assert!(k.get_text_args(&None, "id:1", true).contains(&"--ansi")); + } + + /// A non-numeric id must be rejected rather than widening the match. + #[test] + fn window_match_requires_a_number() { + let cases = [ + ("7", Some("id:7")), + (" 7 ", Some("id:7")), + ("", None), + ("all", None), + ("1 or 2", None), + ]; + for (raw, want) in cases { + // Must call the production parser: reimplementing it here would + // assert only that this line does what this line does, while being + // the sole test guarding the cross-window guarantee. + assert_eq!( + Kitty::parse_window_id(raw).as_deref(), + want, + "input {raw:?}" + ); + } + } + + #[test] + fn hint_matches_the_mode_in_force() { + let socket_err = anyhow!("Error: Remote control is allowed over a socket only"); + + // socket-only with no socket: the fix is listen_on, never `yes`. + let no_sock = Kitty::remote_control_hint(&socket_err, &None); + assert!(no_sock.contains("listen_on"), "{no_sock}"); + assert!( + !no_sock.contains("allow_remote_control yes"), + "must not tell a socket-only user to loosen the setting: {no_sock}" + ); + + // socket-only with a socket that was refused: restart, not reconfigure. + let with_sock = Kitty::remote_control_hint(&socket_err, &Some("unix:/tmp/k".into())); + assert!(with_sock.contains("unix:/tmp/k"), "{with_sock}"); + + let pw = Kitty::remote_control_hint(&anyhow!("Error: password required"), &None); + assert!(pw.contains("unsupported"), "{pw}"); + + // Anything unrecognised must hedge rather than assert a cause. + let other = Kitty::remote_control_hint(&anyhow!("Error: Unknown option: --match"), &None); + assert!( + other.contains("If this is a remote-control problem"), + "{other}" + ); + } +} diff --git a/src/backend/mod.rs b/src/backend/mod.rs index 74739f9..4030817 100644 --- a/src/backend/mod.rs +++ b/src/backend/mod.rs @@ -1,9 +1,10 @@ //! Terminal adapters. //! //! Each backend answers one question: "give me the text of command block N". -//! They differ enormously in what they can do — kitty knows its own command -//! boundaries and needs no help, while tmux knows nothing and relies entirely -//! on the coordinates our shell hook recorded. +//! They differ enormously in what they can do — tmux needs boundaries supplied +//! by the shell hook but then knows them exactly, while kitty needs no help yet +//! can only return its last non-empty output, which may be an earlier command's. +//! `has_command_boundaries` is that distinction. use anyhow::{anyhow, Context, Result}; @@ -68,6 +69,15 @@ pub trait Backend { /// only use it for metadata. fn fetch(&self, index: usize, rec: Option<&Record>, raw: bool) -> Result; + /// Whether the backend can tie output to a specific command. + /// + /// kitty cannot: it returns the last *non-empty* output, which may belong to + /// an earlier command. Callers use this to qualify what they are showing + /// rather than presenting shell-record metadata as if it described the text. + fn has_command_boundaries(&self) -> bool { + true + } + /// Whether this backend can reach past the most recent command. /// /// Only tmux can. kitty and iTerm2 expose their latest command block and @@ -82,8 +92,8 @@ pub trait Backend { /// Order matters: **tmux wins whenever it is present.** Inside kitty running /// tmux, `kitty @ get-text` returns tmux's rendered viewport rather than the -/// shell's scrollback, and tmux swallows the prompt marks `last_cmd_output` -/// relies on — so asking kitty there silently returns the wrong text. +/// shell's scrollback, and tmux swallows the prompt marks kitty's output extents +/// rely on — so asking kitty there silently returns the wrong text. pub fn detect(forced: Option<&str>) -> Result> { if let Some(name) = forced { return match name { diff --git a/src/capture.rs b/src/capture.rs index 7f17398..1078291 100644 --- a/src/capture.rs +++ b/src/capture.rs @@ -18,6 +18,10 @@ pub struct Capture { pub duration_ms: Option, pub output: String, pub source: String, + /// True when the backend cannot tie this output to the named command, so + /// the metadata above may describe a different command than the body. + #[serde(skip_serializing_if = "std::ops::Not::not")] + pub approximate: bool, } impl Capture { @@ -29,6 +33,7 @@ impl Capture { duration_ms: None, output, source: source.to_string(), + approximate: false, } } diff --git a/src/main.rs b/src/main.rs index ddef38e..4d895fa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ mod doctor; mod render; mod state; +use std::io::IsTerminal; use std::time::{SystemTime, UNIX_EPOCH}; use anyhow::{anyhow, Result}; @@ -20,23 +21,69 @@ use state::Record; fn main() { if let Err(e) = run() { + // Deliberately unmarked: an error message is not captured text, and + // marking it would make a failed `tcap -c 2` poison the next capture. eprintln!("tcap: {e:#}"); std::process::exit(1); } } +/// Record that tcap put text on the screen. +/// +/// Called at the point of each write, gated on *that stream* being a terminal — +/// not inferred from isatty at exit. Inferring was wrong three ways: under +/// `eval "$(tcap init zsh)"` stdout is a pipe but stderr is still a tty, so +/// every shell startup marked itself and poisoned its first real command; +/// `tcap --output > f` marked despite printing nothing visible; and clap's +/// `--help` exits before any of it runs. +fn mark_if_stdout_is_terminal() { + if std::io::stdout().is_terminal() { + warn_if_unmarked(state::mark_capture()); + } +} + +/// A marker that could not be written leaves the next capture unguarded, so the +/// failure is reported rather than swallowed. It is not fatal: refusing to +/// capture at all because a temp file is unwritable would be a worse trade than +/// capturing with the guard announced as off. +fn warn_if_unmarked(r: Result<()>) { + if let Err(e) = r { + eprintln!( + "tcap: could not record that this output is on screen ({e:#}).\n\ + \x20 The check that stops the next capture returning this text is off." + ); + } +} + fn run() -> Result<()> { - let cli = Cli::parse(); + let cli = match Cli::try_parse() { + Ok(c) => c, + Err(e) => { + // --help and --version print to stdout, usage errors to stderr. + let _ = e.print(); + // --help and --version put a screenful on stdout, which does become + // the last output. Usage errors go to stderr and are not marked, for + // the same reason other diagnostics are not. + if !e.use_stderr() { + mark_if_stdout_is_terminal(); + } + std::process::exit(e.exit_code()); + } + }; match &cli.command { // doctor must survive a broken config — reporting the breakage is // exactly its job, so the error is handed over rather than propagated. + // doctor and init put their text on screen too when not redirected — + // `eval "$(tcap init zsh)"` pipes stdout, so that correctly does not mark. Some(Sub::Doctor) => { print!("{}", doctor::run(config::load())); + mark_if_stdout_is_terminal(); Ok(()) } Some(Sub::Init { shell }) => { print!("{}", init_script(*shell)); + mark_if_stdout_is_terminal(); Ok(()) } // The record hook runs before every prompt and must not depend on, or @@ -90,11 +137,23 @@ fn record(args: &RecordArgs) -> Result<()> { duration_ms, start_line, end_line, + was_capture: state::take_capture_marker(), }) } fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { let backend = backend::detect(settings.backend.as_deref())?; + + if !settings.quiet { + if let Some(why) = state::unusable_reason() { + eprintln!( + "tcap: cannot use the state directory ({why}).\n\ + \x20 Continuing without command history: no exit codes, and the check\n\ + \x20 that stops a capture returning tcap's own output is off." + ); + } + } + let records = state::load(); let mode = settings.mode; let raw = mode == cli::Mode::Raw; @@ -108,7 +167,12 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { let rec = state::nth_from_end(&records, index); match backend.fetch(index, rec, raw) { - Ok(fetched) => caps.push(build_capture(fetched, rec, backend.name())), + Ok(fetched) => caps.push(build_capture( + fetched, + rec, + backend.name(), + !backend.has_command_boundaries(), + )), // The block the user explicitly asked for must report its error. // Extra blocks requested via -C are best-effort: running out of // history is normal and should not fail the whole capture. @@ -124,12 +188,42 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { )); } + // Only the marker, deliberately — not `is_capture_record`. The two answer + // different questions: indexing asks "was this a tcap run at all", which the + // text heuristic approximates well enough; this asks "is tcap's own text the + // last thing on screen", true only when the capture actually displayed. A + // redirected `tcap --output > notes.txt` printed nothing to the terminal, so + // the screen's last output is still a real command's and the next capture is + // perfectly valid. + let after_tcap = records.last().map(|r| r.was_capture).unwrap_or(false); + + // Refuse rather than emit, in the one case where the output is known to be + // wrong: a boundary-less backend returns the last non-empty output, and + // after a previous tcap that is the previous capture. Printing it first and + // warning afterwards is no use in `tcap --output | some-service`, where the + // text has already left by the time stderr is read. Where the mismatch is + // only possible rather than certain, the capture proceeds and is labelled. + if after_tcap && caps.iter().any(|c| c.approximate) && mode != cli::Mode::Command { + return Err(anyhow!( + "refusing to capture: the previous command left tcap's own output on screen,\n\ + and {} can only report the last non-empty output — which is that text, not\n\ + a command's. (It may have been tcap directly, or a script that called it.)\n\n\ + Re-run the command you want to capture, then tcap. Inside tmux this is not\n\ + a limitation, because the shell hook records exact boundaries.", + backend.name() + )); + } + render::prepare(&mut caps, mode, &cli.select, settings.max_bytes); let text = render::render(&caps, mode); println!("{text}"); + mark_if_stdout_is_terminal(); if !settings.quiet { + // Not marked: a one-line diagnostic is not the captured text this guard + // is about. Marking it made `tcap --output | llm` refuse the *next* + // capture — and flipped that outcome on whether --quiet was passed. emit_hints(&caps, backend.as_ref(), mode); } @@ -142,14 +236,34 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { /// Warn about degraded captures, naming the cause and the fix. Goes to stderr /// so `tcap | sgpt` still pipes clean text. -fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) { +/// Returns whether anything was printed. +fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) -> bool { + let mut printed = false; + // Every mode but Command: --output and --raw are the ones piped into another + // tool, so they are where handing over an earlier command's text does the + // most damage. Command mode renders the shell's own record, which is exact, + // and never prints the terminal text this warning is about. + // + // The *certain* mismatch — a capture straight after another one — does not + // reach here at all; capture_and_print refuses it outright. + if mode != cli::Mode::Command && caps.iter().any(|c| c.approximate) { + eprintln!( + "tcap: {} returns the last non-empty output, which may not belong to the\n\ + \x20 command named above — a silent command yields an earlier one's output,\n\ + \x20 and a previous `tcap | …` leaves its pipeline's. tmux records exact\n\ + \x20 boundaries; --quiet silences this.", + backend.name() + ); + printed = true; + } + // Only the annotated view promises metadata, so only it can disappoint. if mode != cli::Mode::Annotated { - return; + return printed; } if caps.iter().any(Capture::has_metadata) { - return; + return printed; } let shell = detect_shell(); @@ -177,6 +291,8 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) backend.name() ); } + + true } fn detect_shell() -> String { @@ -190,8 +306,14 @@ fn detect_shell() -> String { /// The shell record wins where both have a value — it saw the command as typed /// and is the only source of duration. Backend metadata fills the gaps, which is /// what makes iTerm2 usable with no shell integration at all. -fn build_capture(fetched: backend::Fetched, rec: Option<&Record>, source: &str) -> Capture { +fn build_capture( + fetched: backend::Fetched, + rec: Option<&Record>, + source: &str, + approximate: bool, +) -> Capture { let mut cap = Capture::new(fetched.output, source); + cap.approximate = approximate; cap.command = fetched.command; cap.exit_code = fetched.exit_code; diff --git a/src/render.rs b/src/render.rs index 92355ef..04d540e 100644 --- a/src/render.rs +++ b/src/render.rs @@ -126,6 +126,17 @@ fn header(cap: &Capture) -> String { s.push_str(&format!("# {}\n", meta.join(" "))); } + // Without real boundaries the metadata above is the shell's record of the + // last command, while the body is whatever the terminal last had on screen. + // Usually the same thing; when the last command printed nothing, not. + if cap.approximate { + s.push_str(&format!( + "# note: {} returns the last non-empty output, which may not be this \ + command's\n", + cap.source + )); + } + s } @@ -135,7 +146,11 @@ fn render_one(cap: &Capture, mode: Mode) -> String { Mode::Command => cap.command.clone().unwrap_or_default(), Mode::Raw | Mode::Output => cap.output.clone(), Mode::Annotated => { - if !cap.has_metadata() { + // `approximate` counts as something worth saying, even with no + // command or exit code to show: kitty without shell integration has + // no metadata at all, and that is precisely where an unlabelled body + // is most likely to be the wrong one. + if !cap.has_metadata() && !cap.approximate { return cap.output.clone(); } let h = header(cap); @@ -198,6 +213,7 @@ mod tests { duration_ms: Some(3200), output: "Error: Cannot find module 'foo'".into(), source: "tmux".into(), + approximate: false, } } @@ -276,12 +292,50 @@ mod tests { ); } + /// Without shell integration there is no metadata, but an approximate body + /// still has to say so. + #[test] + fn approximate_is_labelled_even_with_no_metadata() { + let mut c = cap_with_output("mystery output"); + c.source = "kitty".into(); + c.approximate = true; + let out = render(&[c], Mode::Annotated); + assert!(out.contains("# note:"), "{out}"); + assert!(out.contains("mystery output"), "{out}"); + } + #[test] fn annotated_falls_back_to_bare_output_without_metadata() { let out = render(&[cap_with_output("just text")], Mode::Annotated); assert_eq!(out, "just text"); } + /// Metadata over a body the backend cannot vouch for must say so, or the + /// header reads as authoritative for text it may not describe. + #[test] + fn approximate_captures_are_labelled() { + let mut c = full_capture(); + c.source = "kitty".into(); + c.approximate = true; + let out = render(&[c], Mode::Annotated); + assert!( + out.contains("# note: kitty returns the last non-empty output"), + "{out}" + ); + + // Exact backends must stay unqualified. + let exact = render(&[full_capture()], Mode::Annotated); + assert!(!exact.contains("# note:"), "{exact}"); + } + + #[test] + fn approximate_is_only_serialised_when_true() { + let mut c = full_capture(); + c.approximate = true; + assert!(render(&[c], Mode::Json).contains("\"approximate\": true")); + assert!(!render(&[full_capture()], Mode::Json).contains("approximate")); + } + #[test] fn exit_zero_is_still_reported() { let mut c = full_capture(); diff --git a/src/state.rs b/src/state.rs index 97c387f..4e72caa 100644 --- a/src/state.rs +++ b/src/state.rs @@ -7,11 +7,12 @@ //! JSON Lines: appending keeps the hook cheap, and compaction past //! [`COMPACT_AT`] bounds the file in a long-lived shell. -use anyhow::{Context, Result}; +use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; use std::fs::{self, OpenOptions}; -use std::io::Write; -use std::path::PathBuf; +use std::io::{Read, Write}; +use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt}; +use std::path::{Path, PathBuf}; /// Records retained when the log is compacted. Must comfortably exceed the /// largest `-n`/`-c` anyone would plausibly type. @@ -34,6 +35,41 @@ pub struct Record { pub start_line: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub end_line: Option, + /// Set when tcap performed a capture during this command. + /// + /// Recorded by the hook rather than inferred from the command text, so it + /// holds for `command tcap`, `env tcap`, an alias, a shell function or any + /// other spelling that text matching cannot see. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub was_capture: bool, +} + +/// Dropped by a capture, collected by the next `__record`. +/// +/// tcap captures *during* a command; the hook records that same command when it +/// finishes. So a marker present at record time means this command is the one +/// that captured, whatever it was called. +fn marker_path() -> PathBuf { + state_dir().join(format!("{}.capturing", session_key())) +} + +pub fn mark_capture() -> Result<()> { + secure_open(&marker_path())?.write_all(b"1")?; + Ok(()) +} + +/// Whether a capture happened during this command, clearing the marker. +pub fn take_capture_marker() -> bool { + fs::remove_file(marker_path()).is_ok() +} + +/// Why the state directory is unusable, if it is. +/// +/// `load` failing closed to an empty history also switches off the guard that +/// depends on it, so the caller needs to be able to say so rather than behave as +/// though this were a fresh session. +pub fn unusable_reason() -> Option { + ensure_state_dir().err().map(|e| format!("{e:#}")) } /// Directory holding one log per terminal session. @@ -46,12 +82,99 @@ pub fn state_dir() -> PathBuf { base.join(format!("tcap-{}", current_uid())) } -/// Avoids a `libc` dependency for a single call. fn current_uid() -> u32 { - extern "C" { - fn getuid() -> u32; + // Safe: getuid cannot fail and touches no memory we own. + unsafe { libc::getuid() } +} + +/// The state directory, created 0700 and checked to be ours. +/// +/// The paths under it are predictable, and on Linux the fallback is the shared +/// /tmp. Without this, another local user could pre-create the directory and +/// leave a symlink where a state file goes: an ordinary write would then follow +/// it and truncate whatever it pointed at. macOS is not exposed (TMPDIR is +/// already per-user) but the check is cheap and the fallback is not. +fn ensure_state_dir() -> Result { + let dir = state_dir(); + + // Created 0700 from the outset rather than chmod'ed afterwards, so there is + // no window where it exists world-readable. + match fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(&dir) + { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e).with_context(|| format!("creating {}", dir.display())), + } + + // Inspected before anything is mutated: `set_permissions` follows symlinks, + // so tightening first would let a planted symlink redirect the chmod onto a + // file of the attacker's choosing that the victim owns. + let md = fs::symlink_metadata(&dir).with_context(|| format!("checking {}", dir.display()))?; + if !md.is_dir() { + return Err(anyhow!( + "{} is a symlink or not a directory — refusing to use it", + dir.display() + )); + } + if md.uid() != current_uid() { + return Err(anyhow!( + "{} is owned by uid {}, not you — refusing to use it", + dir.display(), + md.uid() + )); + } + // Only now that it is known to be our own real directory. + if md.mode() & 0o077 != 0 { + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)) + .with_context(|| format!("securing {}", dir.display()))?; } - unsafe { getuid() } + + Ok(dir) +} + +/// Read a state file with the same checks the writes get. +/// +/// Reads matter as much as writes here: a pre-created directory with a crafted +/// log would let another user inject records, and their command text is printed +/// back — escape sequences included. +fn secure_read(path: &Path) -> Option { + ensure_state_dir().ok()?; + let mut f = OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path) + .ok()?; + let mut buf = String::new(); + f.read_to_string(&mut buf).ok()?; + Some(buf) +} + +/// Write a file under the state directory with the same protections. +pub fn secure_write(path: &Path, bytes: &[u8]) -> Result<()> { + secure_open(path)?.write_all(bytes)?; + Ok(()) +} + +/// Read a file under the state directory with the same protections. +pub fn secure_read_file(path: &Path) -> Option { + secure_read(path) +} + +/// Open a state file for writing without following symlinks. +fn secure_open(path: &Path) -> Result { + ensure_state_dir()?; + OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .mode(0o600) + // The whole point: a symlink here fails instead of being followed. + .custom_flags(libc::O_NOFOLLOW) + .open(path) + .with_context(|| format!("opening {}", path.display())) } /// Identify the current terminal session. @@ -86,9 +209,7 @@ pub fn log_path() -> PathBuf { /// Append one record, compacting the log if it has grown too long. pub fn append(rec: &Record) -> Result<()> { - let dir = state_dir(); - fs::create_dir_all(&dir) - .with_context(|| format!("creating state directory {}", dir.display()))?; + ensure_state_dir()?; let path = log_path(); let line = serde_json::to_string(rec)?; @@ -96,6 +217,8 @@ pub fn append(rec: &Record) -> Result<()> { let mut f = OpenOptions::new() .create(true) .append(true) + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW) .open(&path) .with_context(|| format!("opening {}", path.display()))?; writeln!(f, "{line}")?; @@ -106,7 +229,7 @@ pub fn append(rec: &Record) -> Result<()> { } fn compact_if_needed(path: &PathBuf) -> Result<()> { - let Ok(text) = fs::read_to_string(path) else { + let Some(text) = secure_read(path) else { return Ok(()); }; let lines: Vec<&str> = text.lines().collect(); @@ -116,7 +239,7 @@ fn compact_if_needed(path: &PathBuf) -> Result<()> { let kept = &lines[lines.len() - KEEP..]; // Write via a temp file so a concurrent reader never sees a partial log. let tmp = path.with_extension("jsonl.tmp"); - fs::write(&tmp, format!("{}\n", kept.join("\n")))?; + secure_open(&tmp)?.write_all(format!("{}\n", kept.join("\n")).as_bytes())?; fs::rename(&tmp, path)?; Ok(()) } @@ -124,7 +247,7 @@ fn compact_if_needed(path: &PathBuf) -> Result<()> { /// Load this session's records, oldest first. Unparseable lines are skipped /// rather than failing the whole read. pub fn load() -> Vec { - let Ok(text) = fs::read_to_string(log_path()) else { + let Some(text) = secure_read(&log_path()) else { return Vec::new(); }; text.lines() @@ -134,29 +257,42 @@ pub fn load() -> Vec { } /// `-c` counts real commands, so `tcap -c 1` must mean "the command I just ran", -/// never a previous capture. Checks the first word of every pipeline or list -/// segment, skipping leading `VAR=value` assignments. +/// never a previous capture. +/// +/// Text matching only, so it cannot see an alias or a shell function — which is +/// why the authoritative signal is [`Record::was_capture`], set by the hook. +/// This remains for records written before that existed, and for skipping in +/// [`nth_from_end`]. It looks past leading `VAR=value` assignments and the usual +/// wrappers. pub fn is_tcap_invocation(cmd: &str) -> bool { cmd.split(['|', ';', '&']).any(|seg| { let mut words = seg.split_whitespace().peekable(); - while let Some(w) = words.peek() { - // A leading assignment like `FOO=bar tcap` is still a tcap run. - if w.contains('=') && !w.starts_with('=') { + loop { + let Some(w) = words.peek() else { return false }; + let base = w.rsplit('/').next().unwrap_or(w); + // `FOO=bar tcap`, `command tcap`, `env FOO=1 tcap`, `\tcap`. + if (w.contains('=') && !w.starts_with('=')) + || matches!( + base, + "command" | "builtin" | "exec" | "env" | "nohup" | "time" + ) + { words.next(); - } else { - break; + continue; } + return base.trim_start_matches('\\') == "tcap"; } - words - .next() - .map(|first| first.rsplit('/').next().unwrap_or(first) == "tcap") - .unwrap_or(false) }) } /// The `n`th real command counting back from the most recent (`n == 1` is the /// last command), with `tcap` invocations skipped. pub fn nth_from_end(records: &[Record], n: usize) -> Option<&Record> { + // Deliberately the text check, not `is_capture_record`. `was_capture` means + // "tcap wrote to this terminal during this command", which is also true of a + // wrapper script, a Makefile target or a git alias that happens to call + // tcap. Skipping those would silently return the command before them — on + // tmux too, where boundaries are exact and none of this is needed. records .iter() .rev() @@ -176,6 +312,7 @@ mod tests { duration_ms: None, start_line: None, end_line: None, + was_capture: false, } } @@ -189,6 +326,47 @@ mod tests { assert!(is_tcap_invocation("ls; tcap")); } + /// Wrappers that the original first-word check walked straight past. + #[test] + fn detects_wrapped_invocations() { + for cmd in [ + "command tcap", + "env tcap --output", + "env FOO=1 tcap", + "builtin tcap", + "exec tcap", + "nohup tcap", + "command /usr/local/bin/tcap", + ] { + assert!(is_tcap_invocation(cmd), "should detect: {cmd}"); + } + } + + /// The two signals answer different questions and are used in different + /// places: `was_capture` gates the refusal (is tcap's text on screen), the + /// text check gates indexing (was this a tcap run). An alias is visible only + /// to the first; a piped capture sets only the second. Conflating them has + /// broken each direction once. + #[test] + fn the_two_capture_signals_are_independent() { + let mut aliased = rec("t"); + aliased.was_capture = true; + assert!( + !is_tcap_invocation(&aliased.command), + "text cannot see an alias" + ); + + let piped = rec("tcap --output | llm"); + assert!( + is_tcap_invocation(&piped.command), + "text sees an explicit run" + ); + assert!( + !piped.was_capture, + "but stdout was a pipe, so nothing was displayed" + ); + } + #[test] fn does_not_match_unrelated_commands() { assert!(!is_tcap_invocation("npm run build")); @@ -207,6 +385,34 @@ mod tests { assert!(nth_from_end(&records, 3).is_none()); } + /// A command that merely *called* tcap is still a real command, and must + /// stay indexable. `was_capture` cannot tell "was tcap" from "called tcap", + /// so indexing uses the text check and accepts missing the alias case — + /// naming an alias is visibly odd, whereas silently skipping someone's + /// deploy script returns the wrong command with nothing to notice. + #[test] + fn nth_keeps_commands_that_merely_called_tcap() { + let mut wrapper = rec("./deploy.sh"); + wrapper.was_capture = true; + let records = vec![rec("npm run build"), wrapper]; + + assert_eq!(nth_from_end(&records, 1).unwrap().command, "./deploy.sh"); + assert_eq!(nth_from_end(&records, 2).unwrap().command, "npm run build"); + } + + /// The marker decides whether a capture is refused outright, and nothing + /// exercised the file itself. + #[test] + fn capture_marker_round_trips_and_is_consumed_once() { + // session_key is ppid-based, so clear any stray marker first. + let _ = take_capture_marker(); + + assert!(!take_capture_marker(), "no marker to begin with"); + mark_capture().expect("marker should be writable"); + assert!(take_capture_marker(), "the marker is seen once"); + assert!(!take_capture_marker(), "and only once — it is consumed"); + } + #[test] fn nth_returns_none_past_the_start_of_history() { assert!(nth_from_end(&[], 1).is_none());