From 1d054d300a954fce223af4f98244f2177d0686d3 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 15:56:58 -0700 Subject: [PATCH 01/14] Fix kitty capture returning nothing; bump to 0.1.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported in the field: `tcap` after `pwd` in kitty printed the header with no output at all. Three separate bugs, none caught by tests because the suite only drives tmux. - Wrong extent. kitty counts the running `tcap` as the current command, so `last_cmd_output` returned tcap's own empty output. Measured in a scratch window: last_cmd_output 0 bytes, last_non_empty_output 20. That extent is meant for a keybinding pressed at the prompt, where nothing is running. - Wrong window. Without `--match`, get-text reads whichever window is *focused*, not the one tcap runs in — hit while diagnosing this, where it returned an unrelated window's scrollback. Now pinned to $KITTY_WINDOW_ID. `--match` is a get-text option, so it follows the subcommand. - Wrong advice. Under `allow_remote_control socket-only` the failure told the user to set `allow_remote_control yes`, i.e. to loosen a deliberately restricted setting to fix an unrelated cause. The real fix is `listen_on`. The hint is now chosen from the mode actually in force, and covers the password mode too. `doctor` reports which channel it connected over. Caveat now documented: asking for the last non-empty output means a command that printed nothing yields an older command's output under the current header. tmux is unaffected, having real boundaries. 🤖 Built with SMT --- AGENTS.md | 9 +++ Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 16 ++++-- npm/package.json | 2 +- src/backend/kitty.rs | 129 +++++++++++++++++++++++++++++++++++-------- 6 files changed, 130 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5246c90..1cffe57 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,6 +49,15 @@ newlines need no escaping. 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`. + ## Verification status - **tmux** — fully working, covered by end-to-end tests. diff --git a/Cargo.lock b/Cargo.lock index e3dc2c9..3eb121b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -245,7 +245,7 @@ dependencies = [ [[package]] name = "tcap" -version = "0.1.0" +version = "0.1.1" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index 96c1e7d..d705156 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" diff --git a/README.md b/README.md index f6412a6..9c1a9b4 100644 --- a/README.md +++ b/README.md @@ -152,10 +152,18 @@ 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 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. 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` together with `listen_on unix:/tmp/kitty-{kitty_pid}`. When something is missing, `tcap` says what and why: 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/kitty.rs b/src/backend/kitty.rs index a6aa9cf..683d974 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,57 @@ impl Kitty { .ok() .filter(|s| !s.is_empty()) } + + /// Turn kitty's refusal into the fix for the mode actually in force. + /// + /// `allow_remote_control` has several values and they need different + /// answers: telling a `socket-only` user to set `yes` is wrong advice, and + /// its real cause — no socket to talk to — is not obvious from kitty's + /// message alone. + fn remote_control_hint(err: &anyhow::Error, listen_on: &Option) -> String { + let msg = err.to_string().to_lowercase(); + + if msg.contains("socket only") || msg.contains("socket-only") { + return match listen_on { + None => "kitty is set to `allow_remote_control socket-only`, but \ + $KITTY_LISTEN_ON is unset, so there is no socket to use.\n\ + Add to ~/.config/kitty/kitty.conf and restart kitty:\n \ + listen_on unix:/tmp/kitty-{kitty_pid}\n\ + If it is set in your shell but not here, something in between is \ + clearing the environment." + .to_string(), + Some(sock) => format!( + "the socket at {sock} was refused. Restart kitty so the running \ + instance and $KITTY_LISTEN_ON agree." + ), + }; + } + + if msg.contains("password") { + return "kitty is requiring a remote-control password, which tcap does not \ + send. Allow this one command without a password in kitty.conf:\n \ + remote_control_password \"\" get-text" + .to_string(); + } + + "kitty remote control appears to be disabled. In ~/.config/kitty/kitty.conf \ + either:\n allow_remote_control yes\nor keep it restricted and give tcap a \ + socket:\n allow_remote_control socket-only\n listen_on unix:/tmp/kitty-{kitty_pid}\n\ + Then restart kitty." + .to_string() + } + + /// `--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 triggered from a script, or with focus + /// on another split, silently returns another window's text. + fn window_match() -> Option { + std::env::var("KITTY_WINDOW_ID") + .ok() + .filter(|s| !s.is_empty()) + .map(|id| format!("id:{id}")) + } } impl Backend for Kitty { @@ -96,26 +148,35 @@ impl Backend for Kitty { 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 window = Self::window_match(); let mut args = self.rc_args(&listen_on); - args.extend_from_slice(&["get-text", "--extent", "last_cmd_output"]); + args.push("get-text"); + // `--match` is a get-text option, not a global one: before the + // subcommand kitty rejects it with "Unknown option: --match". + if let Some(w) = &window { + args.extend_from_slice(&["--match", w]); + } + // `last_non_empty_output`, not `last_cmd_output`. + // + // kitty counts the command currently executing — tcap itself — as the + // last command, so `last_cmd_output` returns tcap's own output, which is + // empty. That extent is meant for a keybinding pressed at the prompt, + // where nothing is running. Skipping empty output steps back over + // tcap's own invocation to the command the user actually cares about. + args.extend_from_slice(&["--extent", "last_non_empty_output"]); if raw { args.push("--ansi"); } - 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)." - ) - }) + run(&self.exe, &args) + .map(Fetched::from) + .map_err(|e| anyhow!("{e}\n\n{}", Self::remote_control_hint(&e, &listen_on))) } fn diagnose(&self) -> Vec { @@ -125,25 +186,47 @@ impl Backend for Kitty { let mut args = self.rc_args(&listen_on); args.push("ls"); + let channel = match &listen_on { + Some(sock) => format!("socket {sock}"), + None => "escape-code channel (no $KITTY_LISTEN_ON)".to_string(), + }; d.push(match run(&self.exe, &args) { - Ok(_) => Diagnostic::ok("remote control", "enabled and reachable"), + Ok(_) => Diagnostic::ok("remote control", format!("reachable via {channel}")), Err(e) => Diagnostic::bad( "remote control", - format!("{e} — set `allow_remote_control yes` in kitty.conf"), + format!("{}\n{}", channel, Self::remote_control_hint(&e, &listen_on)), ), }); // The real test of shell integration is whether the extent resolves. + let window = Self::window_match(); let mut probe = self.rc_args(&listen_on); - probe.extend_from_slice(&["get-text", "--extent", "last_cmd_output"]); + probe.push("get-text"); + if let Some(w) = &window { + probe.extend_from_slice(&["--match", w]); + } + probe.extend_from_slice(&["--extent", "last_non_empty_output"]); d.push(match run(&self.exe, &probe) { - Ok(_) => Diagnostic::ok("shell integration", "last_cmd_output resolves"), + Ok(t) if t.trim().is_empty() => Diagnostic::bad( + "shell integration", + "the extent resolved but returned nothing — kitty's own shell \ + integration is probably off (`shell_integration enabled` in kitty.conf)", + ), + Ok(_) => Diagnostic::ok("shell integration", "last_non_empty_output resolves"), Err(e) => Diagnostic::bad( "shell integration", format!("{e} — kitty needs shell_integration enabled for this extent"), ), }); + d.push(match Self::window_match() { + Some(w) => Diagnostic::ok("window", w), + None => Diagnostic::bad( + "window", + "$KITTY_WINDOW_ID unset — kitty will read the focused window", + ), + }); + d.push(Diagnostic::bad( "history depth", "kitty serves only the latest command; -c 2 and beyond need tmux", From 6b46c368c70c1b5cb4afb417b384d78540f7f62d Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 16:15:27 -0700 Subject: [PATCH 02/14] Address review: refuse unscoped kitty capture, keep errors, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers found real problems in the first pass. Codex, rated high: with $KITTY_WINDOW_ID unset, fetch omitted --match and fell back to kitty's *focused* window, putting another pane's scrollback — tokens, passwords, whatever was on screen — under this command's header. Having called unscoped capture unsafe in the same PR, it now refuses instead, and the id is parsed as a number so a poisoned variable cannot widen the match. Claude, four points, all taken: - doctor discarded the underlying kitty error and printed only a keyword-guessed hint, so anything unrecognised ("Unknown option", connection refused) reached the user as "remote control appears disabled". It keeps {e} now, and the fallback hint hedges rather than asserting a cause. - fetch and doctor built the same argv independently, so a change to one would have doctor reporting the capture path healthy while fetch failed. Extracted get_text_args, shared by both. - The PR added no tests. The argv order, extent choice and hint selection are pure functions: five unit tests now lock them in, no kitty required. - Undocumented: two tcap runs in a row return the first run's own output, since that was the last non-empty thing printed. tcap now warns on stderr when the preceding command was itself tcap, and both READMEs say so. Stale AGENTS.md/README references to last_cmd_output corrected — that file is what future agents read as spec, so a stale line there reintroduces the bug. Verified live in kitty: warning fires only on the second consecutive run, the missing-window path refuses, and a normal capture is unaffected. 🤖 Built with SMT --- AGENTS.md | 6 +- README.md | 12 ++- npm/README.md | 13 ++- src/backend/kitty.rs | 247 +++++++++++++++++++++++++++++++++---------- src/main.rs | 23 +++- 5 files changed, 230 insertions(+), 71 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1cffe57..1f90679 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,7 +45,7 @@ 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. @@ -61,7 +61,7 @@ socket-only` only the socket works, so the error must advise `listen_on`, not ## 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/README.md b/README.md index 9c1a9b4..5839f2d 100644 --- a/README.md +++ b/README.md @@ -140,7 +140,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,7 +149,7 @@ 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 only the @@ -160,8 +160,10 @@ 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. tmux has -no such ambiguity, because the shell hook records real boundaries. kitty also +returns an *older* command's output while the header names the last one — and two +`tcap` runs in a row return the first run's own output, since that was the last +thing to print. tcap warns on stderr when it can tell. 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` together with `listen_on unix:/tmp/kitty-{kitty_pid}`. @@ -197,7 +199,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 # 54 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..6095510 100644 --- a/npm/README.md +++ b/npm/README.md @@ -103,10 +103,15 @@ unrelated jobs. 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. So a command that printed nothing, or two +`tcap` runs in a row, yields an older output under the current header. tcap warns +on stderr when it can tell; 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/src/backend/kitty.rs b/src/backend/kitty.rs index 683d974..f49d910 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -118,24 +118,73 @@ impl Kitty { .to_string(); } - "kitty remote control appears to be disabled. In ~/.config/kitty/kitty.conf \ - either:\n allow_remote_control yes\nor keep it restricted and give tcap a \ - socket:\n allow_remote_control socket-only\n listen_on unix:/tmp/kitty-{kitty_pid}\n\ - Then restart kitty." + "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 allow_remote_control socket-only\n \ + listen_on unix:/tmp/kitty-{kitty_pid}\nThen restart kitty. If the message above \ + says something else, that is the real cause." .to_string() } /// `--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 triggered from a script, or with focus - /// on another split, silently returns another window's text. + /// 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 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 { std::env::var("KITTY_WINDOW_ID") + .ok()? + .trim() + .parse::() .ok() - .filter(|s| !s.is_empty()) .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 { @@ -154,25 +203,8 @@ impl Backend for Kitty { } let listen_on = Self::listen_on(); - let window = Self::window_match(); - let mut args = self.rc_args(&listen_on); - args.push("get-text"); - // `--match` is a get-text option, not a global one: before the - // subcommand kitty rejects it with "Unknown option: --match". - if let Some(w) = &window { - args.extend_from_slice(&["--match", w]); - } - // `last_non_empty_output`, not `last_cmd_output`. - // - // kitty counts the command currently executing — tcap itself — as the - // last command, so `last_cmd_output` returns tcap's own output, which is - // empty. That extent is meant for a keybinding pressed at the prompt, - // where nothing is running. Skipping empty output steps back over - // tcap's own invocation to the command the user actually cares about. - args.extend_from_slice(&["--extent", "last_non_empty_output"]); - if raw { - args.push("--ansi"); - } + let window = Self::require_window()?; + let args = self.get_text_args(&listen_on, &window, raw); run(&self.exe, &args) .map(Fetched::from) @@ -183,55 +215,156 @@ impl Backend for Kitty { let mut d = vec![Diagnostic::ok("kitty binary", self.exe.clone())]; let listen_on = Self::listen_on(); - let mut args = self.rc_args(&listen_on); - args.push("ls"); - 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", 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!("{}\n{}", channel, Self::remote_control_hint(&e, &listen_on)), + 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 window = Self::window_match(); - let mut probe = self.rc_args(&listen_on); - probe.push("get-text"); - if let Some(w) = &window { - probe.extend_from_slice(&["--match", w]); - } - probe.extend_from_slice(&["--extent", "last_non_empty_output"]); - d.push(match run(&self.exe, &probe) { - Ok(t) if t.trim().is_empty() => Diagnostic::bad( - "shell integration", - "the extent resolved but returned nothing — kitty's own shell \ - integration is probably off (`shell_integration enabled` in kitty.conf)", - ), - Ok(_) => Diagnostic::ok("shell integration", "last_non_empty_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())); - d.push(match Self::window_match() { - Some(w) => Diagnostic::ok("window", w), - None => Diagnostic::bad( + // Probe exactly what fetch runs, so doctor cannot report a path + // healthy that fetch would fail on. + let probe = self.get_text_args(&listen_on, &w, false); + d.push(match run(&self.exe, &probe) { + Ok(t) if t.trim().is_empty() => Diagnostic::bad( + "shell integration", + "the extent resolved but returned nothing — kitty's own shell \ + integration is probably off (`shell_integration enabled`)", + ), + 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 unset — kitty will read the focused 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 { + let got = raw.trim().parse::().ok().map(|id| format!("id:{id}")); + assert_eq!(got.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("remote_control_password"), "{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/main.rs b/src/main.rs index ddef38e..d394ded 100644 --- a/src/main.rs +++ b/src/main.rs @@ -130,7 +130,13 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { println!("{text}"); if !settings.quiet { - emit_hints(&caps, backend.as_ref(), mode); + // Unfiltered: nth_from_end skips tcap invocations, but for this warning + // we need to know whether tcap itself ran most recently. + let after_tcap = records + .last() + .map(|r| state::is_tcap_invocation(&r.command)) + .unwrap_or(false); + emit_hints(&caps, backend.as_ref(), mode, after_tcap); } if settings.copy { @@ -142,7 +148,20 @@ 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) { +fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode, after_tcap: bool) { + // Backends without real command boundaries return the last *non-empty* + // output. tcap's own output is non-empty, so a capture straight after + // another one hands back the previous capture rather than a command. + if after_tcap && !backend.supports_history() { + eprintln!( + "tcap: the previous command was tcap itself, and {} can only report the\n\ + \x20 last non-empty output — so this is probably tcap's own output rather\n\ + \x20 than a command's. Re-run the command you meant to capture, or use\n\ + \x20 tmux, which records exact boundaries.", + backend.name() + ); + } + // Only the annotated view promises metadata, so only it can disappoint. if mode != cli::Mode::Annotated { return; From cf88203d3626fb10b016402b77362f2799224934 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 16:30:09 -0700 Subject: [PATCH 03/14] Address second review: safe password advice, label approximate captures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex, second high: the password-mode hint recommended `remote_control_password "" get-text`, which removes authentication from the one call that dumps terminal contents — for any process that can reach the socket — to work around a tcap limitation. It now reports password-protected kitty as unsupported and points at tmux or a user-only Unix socket instead. A test asserts the advice never returns. Claude's main point was the deeper version of the bug this PR started from: `ls`, then a silent `cd /tmp`, then tcap gives kitty's `ls` output stamped with `cd /tmp`'s command, exit code and duration — authoritative-looking metadata over the wrong body, the exact failure the tmux coordinate work exists to prevent. The previous commit only warned when the preceding command was tcap itself. Backends now declare `has_command_boundaries()`, and captures from those that lack it are marked approximate: the annotated header carries a `# note:` line naming what the output actually is, and `--json` sets `"approximate": true`. This also fixes the hint firing for iTerm2, which does have real prompt boundaries and none of this behaviour — it was gated on `supports_history()`, a proxy for the wrong property. Also: the window-id test re-implemented the parse instead of calling it, so it could not have caught a regression in the security property it guards — `parse_window_id` is now extracted and tested directly; doctor no longer reports a remote-control failure a second time under the shell-integration label; and the `--match` doc no longer claims more isolation than instance-local ids provide. 🤖 Built with SMT --- README.md | 14 ++++++++----- npm/README.md | 8 +++++--- src/backend/kitty.rs | 47 +++++++++++++++++++++++++++++++++----------- src/backend/mod.rs | 20 ++++++++++++++----- src/capture.rs | 5 +++++ src/main.rs | 24 ++++++++++++++++------ src/render.rs | 37 ++++++++++++++++++++++++++++++++++ 7 files changed, 125 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 5839f2d..c40eb69 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 ``` @@ -162,7 +164,8 @@ wrong block. 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 — and two `tcap` runs in a row return the first run's own output, since that was the last -thing to print. tcap warns on stderr when it can tell. tmux has no such +thing to print. The annotated header carries a `# note:` line saying so, and `--json` sets +`"approximate": true`. 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` together with `listen_on unix:/tmp/kitty-{kitty_pid}`. @@ -187,9 +190,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 @@ -199,7 +203,7 @@ under scrolling: as lines scroll off, `history_size` grows by exactly as much as ## Development ```sh -cargo test # 54 unit + 11 end-to-end tests +cargo test # 56 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 6095510..b42688b 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,7 +98,7 @@ 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 | @@ -110,8 +112,8 @@ block. **On kitty, output is the last *non-empty* output**, because kitty counts the running `tcap` as the current command. So a command that printed nothing, or two -`tcap` runs in a row, yields an older output under the current header. tcap warns -on stderr when it can tell; tmux has no such ambiguity. +`tcap` runs in a row, yields an older output under the current header. The header carries a `# note:` line saying so, and `--json` sets +`"approximate": true`; 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/src/backend/kitty.rs b/src/backend/kitty.rs index f49d910..23c5110 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -112,9 +112,17 @@ impl Kitty { } if msg.contains("password") { - return "kitty is requiring a remote-control password, which tcap does not \ - send. Allow this one command without a password in kitty.conf:\n \ - remote_control_password \"\" get-text" + // 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 "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 — either run inside tmux, which needs \ + no kitty remote control at all, or expose kitty on a Unix socket only your \ + user can reach:\n allow_remote_control socket-only\n \ + listen_on unix:/tmp/kitty-{kitty_pid}" .to_string(); } @@ -134,16 +142,21 @@ impl Kitty { /// 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. + /// /// 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 { - std::env::var("KITTY_WINDOW_ID") - .ok()? - .trim() - .parse::() - .ok() - .map(|id| format!("id:{id}")) + 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 { @@ -192,6 +205,12 @@ 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!( @@ -222,6 +241,7 @@ impl Backend for Kitty { let mut args = self.rc_args(&listen_on); args.push("ls"); + let reachable = run(&self.exe, &args).is_ok(); d.push(match run(&self.exe, &args) { Ok(_) => Diagnostic::ok("remote control", format!("reachable via {channel}")), // The underlying error is kept: the hint is a guess from keywords, @@ -241,7 +261,12 @@ impl Backend for Kitty { d.push(Diagnostic::ok("window", w.clone())); // Probe exactly what fetch runs, so doctor cannot report a path - // healthy that fetch would fail on. + // healthy that fetch would fail on. Skipped when remote control + // is already down, otherwise the same failure gets reported a + // second time under an unrelated label. + if !reachable { + return d; + } let probe = self.get_text_args(&listen_on, &w, false); d.push(match run(&self.exe, &probe) { Ok(t) if t.trim().is_empty() => Diagnostic::bad( @@ -358,7 +383,7 @@ mod tests { assert!(with_sock.contains("unix:/tmp/k"), "{with_sock}"); let pw = Kitty::remote_control_hint(&anyhow!("Error: password required"), &None); - assert!(pw.contains("remote_control_password"), "{pw}"); + 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); 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 d394ded..51e0986 100644 --- a/src/main.rs +++ b/src/main.rs @@ -108,7 +108,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. @@ -152,12 +157,13 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode, // Backends without real command boundaries return the last *non-empty* // output. tcap's own output is non-empty, so a capture straight after // another one hands back the previous capture rather than a command. - if after_tcap && !backend.supports_history() { + if after_tcap && !backend.has_command_boundaries() { eprintln!( "tcap: the previous command was tcap itself, and {} can only report the\n\ - \x20 last non-empty output — so this is probably tcap's own output rather\n\ - \x20 than a command's. Re-run the command you meant to capture, or use\n\ - \x20 tmux, which records exact boundaries.", + \x20 last non-empty output — so this is most likely the previous capture's\n\ + \x20 own output, or whatever its pipeline printed, rather than a command's.\n\ + \x20 Re-run the command you meant to capture, or use tmux, which records\n\ + \x20 exact boundaries.", backend.name() ); } @@ -209,8 +215,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..a711a32 100644 --- a/src/render.rs +++ b/src/render.rs @@ -126,6 +126,16 @@ 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 — it may be an earlier command's\n", + cap.source + )); + } + s } @@ -198,6 +208,7 @@ mod tests { duration_ms: Some(3200), output: "Error: Cannot find module 'foo'".into(), source: "tmux".into(), + approximate: false, } } @@ -282,6 +293,32 @@ mod tests { 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(); From 8c70b0a933759f718ef90b76a9e33f5078ca6551 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 17:06:28 -0700 Subject: [PATCH 04/14] Address third review: probe once, test the real parser, safer socket advice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers independently flagged the same two, and both were mine: - `window_match_requires_a_number` still re-implemented the parse instead of calling it, so the only test guarding the cross-window property could not have caught a regression in it. I believed I had fixed this last round; the patch had silently not applied. Verified this time by breaking `parse_window_id` on purpose and watching the test fail. - `kitty @ ls` was spawned twice in `diagnose`, so the two results could disagree and leave `reachable` inconsistent with the line just reported. Probed once. Also from the review: - Skipping the get-text probe when remote control is down used an early return, which dropped the `history depth` diagnostic below it. Only the probe is skipped now. - The socket keyword match required a specific word order. kitty 0.48.2 does say "Remote control is allowed over a socket only" — captured live, not invented — but matching the bare word is one less thing to break on a version bump. - The password hint offered a /tmp socket as reachable by "only your user". Connecting needs write permission on the socket, so under a group-writable umask that is more than just you. It now suggests a 0700 directory and says why /tmp is not equivalent. 🤖 Built with SMT --- src/backend/kitty.rs | 66 ++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 24 deletions(-) diff --git a/src/backend/kitty.rs b/src/backend/kitty.rs index 23c5110..d5c49ed 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -95,7 +95,11 @@ impl Kitty { fn remote_control_hint(err: &anyhow::Error, listen_on: &Option) -> String { let msg = err.to_string().to_lowercase(); - if msg.contains("socket only") || msg.contains("socket-only") { + // 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 something to depend on across versions, and by this + // point we already know it is a remote-control failure. + if msg.contains("socket") { return match listen_on { None => "kitty is set to `allow_remote_control socket-only`, but \ $KITTY_LISTEN_ON is unset, so there is no socket to use.\n\ @@ -119,10 +123,13 @@ impl Kitty { return "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 — either run inside tmux, which needs \ - no kitty remote control at all, or expose kitty on a Unix socket only your \ - user can reach:\n allow_remote_control socket-only\n \ - listen_on unix:/tmp/kitty-{kitty_pid}" + is on screen, credentials included — run inside tmux, which needs no kitty \ + remote control at all. If you would rather use a socket, put it somewhere \ + only you can open, such as a directory of your own with mode 0700:\n \ + allow_remote_control socket-only\n \ + listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid}\n\ + A socket in /tmp is reachable by anyone with write permission on it, which \ + under a group-writable umask is more than just you." .to_string(); } @@ -241,8 +248,12 @@ impl Backend for Kitty { let mut args = self.rc_args(&listen_on); args.push("ls"); - let reachable = run(&self.exe, &args).is_ok(); - d.push(match run(&self.exe, &args) { + // 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 @@ -261,22 +272,23 @@ impl Backend for Kitty { d.push(Diagnostic::ok("window", w.clone())); // Probe exactly what fetch runs, so doctor cannot report a path - // healthy that fetch would fail on. Skipped when remote control - // is already down, otherwise the same failure gets reported a - // second time under an unrelated label. - if !reachable { - return d; + // 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) { + Ok(t) if t.trim().is_empty() => Diagnostic::bad( + "shell integration", + "the extent resolved but returned nothing — kitty's own shell \ + integration is probably off (`shell_integration enabled`)", + ), + Ok(_) => { + Diagnostic::ok("shell integration", "last_non_empty_output resolves") + } + Err(e) => Diagnostic::bad("shell integration", e.to_string()), + }); } - let probe = self.get_text_args(&listen_on, &w, false); - d.push(match run(&self.exe, &probe) { - Ok(t) if t.trim().is_empty() => Diagnostic::bad( - "shell integration", - "the extent resolved but returned nothing — kitty's own shell \ - integration is probably off (`shell_integration enabled`)", - ), - 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", @@ -361,8 +373,14 @@ mod tests { ("1 or 2", None), ]; for (raw, want) in cases { - let got = raw.trim().parse::().ok().map(|id| format!("id:{id}")); - assert_eq!(got.as_deref(), want, "input {raw:?}"); + // 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:?}" + ); } } From 7631b7f51d0caabe783343a845bd0aa5464e2fd6 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 20:22:32 -0700 Subject: [PATCH 05/14] Address fourth review: warn in piped modes, fix the socket advice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The approximate marker only surfaced in annotated mode and --json, so `tcap --output | llm` under kitty could hand over an earlier command's output with nothing anywhere saying so — the exact failure the rest of this codebase refuses to allow. The warning now fires in every mode, on stderr so the pipe stays clean, and --quiet still silences it. Verified both live. - My own "safe" alternative was wrong on macOS: kitty expands env vars in listen_on, and XDG_RUNTIME_DIR is a systemd convention that is normally unset there, so `unix:${XDG_RUNTIME_DIR}/kitty-...` would have resolved to the filesystem root and failed for a reason the message did not explain. The advice now names both platforms and lives in one constant, since it was offered in three places and only one of them had been made safe. - doctor called a healthy setup broken: `last_non_empty_output` is also empty in a window where nothing has printed yet, so `tcap doctor` as the first command in a fresh window reported shell integration as failing. It now says what to do rather than asserting a cause it cannot distinguish. - The socket branch inferred `socket-only` from one word of kitty's message and stated it flatly; hedged to match the unrecognised-error branch. 🤖 Built with SMT --- README.md | 4 ++- src/backend/kitty.rs | 61 +++++++++++++++++++++++++++----------------- src/main.rs | 31 +++++++++++++++------- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index c40eb69..0b7d0b6 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,9 @@ thing to print. The annotated header carries a `# note:` line saying so, and `-- `"approximate": true`. 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` together with `listen_on unix:/tmp/kitty-{kitty_pid}`. +`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: diff --git a/src/backend/kitty.rs b/src/backend/kitty.rs index d5c49ed..10643f4 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -92,6 +92,16 @@ impl Kitty { /// answers: telling a `socket-only` user to set `yes` is wrong advice, and /// its real cause — no socket to talk to — is not obvious from kitty's /// message alone. + /// The socket recommendation, given once so the three places that offer it + /// cannot drift apart — and correct per platform: `$XDG_RUNTIME_DIR` is a + /// systemd convention, normally unset on macOS, where it would expand to + /// nothing and leave the socket at the filesystem root. + const SOCKET_ADVICE: &'static str = " allow_remote_control socket-only\n \ + listen_on unix:${TMPDIR}/kitty-{kitty_pid} # macOS: $TMPDIR is per-user, 0700\n \ + listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid} # Linux\n\ + Put it somewhere only you can open. A socket in /tmp is reachable by anyone with \ + write permission on it, which under a group-writable umask is more than just you."; + fn remote_control_hint(err: &anyhow::Error, listen_on: &Option) -> String { let msg = err.to_string().to_lowercase(); @@ -101,13 +111,16 @@ impl Kitty { // point we already know it is a remote-control failure. if msg.contains("socket") { return match listen_on { - None => "kitty is set to `allow_remote_control socket-only`, but \ - $KITTY_LISTEN_ON is unset, so there is no socket to use.\n\ - Add to ~/.config/kitty/kitty.conf and restart kitty:\n \ - listen_on unix:/tmp/kitty-{kitty_pid}\n\ + // "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." - .to_string(), + 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." @@ -120,25 +133,23 @@ impl Kitty { // 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 "kitty is requiring a remote-control password, which tcap cannot send, \ - so this setup is unsupported.\n\ + 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, put it somewhere \ - only you can open, such as a directory of your own with mode 0700:\n \ - allow_remote_control socket-only\n \ - listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid}\n\ - A socket in /tmp is reachable by anyone with write permission on it, which \ - under a group-writable umask is more than just you." - .to_string(); + remote control at all. If you would rather use a socket:\n{}", + Self::SOCKET_ADVICE + ); } - "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 allow_remote_control socket-only\n \ - listen_on unix:/tmp/kitty-{kitty_pid}\nThen restart kitty. If the message above \ - says something else, that is the real cause." - .to_string() + 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. @@ -278,10 +289,14 @@ impl Backend for Kitty { 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", - "the extent resolved but returned nothing — kitty's own shell \ - integration is probably off (`shell_integration enabled`)", + "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") diff --git a/src/main.rs b/src/main.rs index 51e0986..53d2016 100644 --- a/src/main.rs +++ b/src/main.rs @@ -157,15 +157,28 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode, // Backends without real command boundaries return the last *non-empty* // output. tcap's own output is non-empty, so a capture straight after // another one hands back the previous capture rather than a command. - if after_tcap && !backend.has_command_boundaries() { - eprintln!( - "tcap: the previous command was tcap itself, and {} can only report the\n\ - \x20 last non-empty output — so this is most likely the previous capture's\n\ - \x20 own output, or whatever its pipeline printed, rather than a command's.\n\ - \x20 Re-run the command you meant to capture, or use tmux, which records\n\ - \x20 exact boundaries.", - backend.name() - ); + // Every mode, not just annotated: --output and --raw are the ones piped + // into another tool, so they are exactly where silently handing over an + // earlier command's text does the most damage. stderr keeps the pipe clean, + // and --quiet still silences it. + if caps.iter().any(|c| c.approximate) { + if after_tcap { + eprintln!( + "tcap: the previous command was tcap itself, and {} can only report the\n\ + \x20 last non-empty output — so this is most likely the previous capture's\n\ + \x20 own output, or whatever its pipeline printed, rather than a command's.\n\ + \x20 Re-run the command you meant to capture, or use tmux, which records\n\ + \x20 exact boundaries.", + backend.name() + ); + } else { + eprintln!( + "tcap: {} returns the last non-empty output, so if the command named above\n\ + \x20 printed nothing this is an earlier command's output. tmux records\n\ + \x20 exact boundaries; --quiet silences this.", + backend.name() + ); + } } // Only the annotated view promises metadata, so only it can disappoint. From a0469e37e383e908a780bfcc15eea79ec12869df Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 21:01:10 -0700 Subject: [PATCH 06/14] Address fifth review: make the kitty.conf advice paste-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error text exists to be pasted into kitty.conf, and it would have broken anyone who did. Verified against kitty 0.48.2 rather than assumed: a `listen_on unix:/tmp/sock # note` line produced a socket literally named `/tmp/sock # note-69755`. kitty takes the rest of the line as the value; `#` only opens a comment at the start of a line. - Trailing comments removed, and the snippet now emits one `listen_on` line for the platform it is running on. Offering both meant last-one-wins handed a macOS user the Linux line, where $XDG_RUNTIME_DIR is normally unset and expands to nothing — putting the socket at the filesystem root, the exact hazard the surrounding text warns about. - `password` is now checked before `socket`, so a password refusal naming the socket it arrived over is not answered with "restart kitty". - A window that cannot be found is diagnosed as a shared or stale $KITTY_LISTEN_ON pointing at another instance, rather than falling through to "enable remote control". The first attempt keyed on "match", which caught "Unknown option: --match" — the misdiagnosis the branch exists to prevent. The test now covers both strings. - doctor indents every line of a multi-line hint, not just the first. 🤖 Built with SMT --- src/backend/kitty.rs | 95 +++++++++++++++++++++++++++++--------------- 1 file changed, 64 insertions(+), 31 deletions(-) diff --git a/src/backend/kitty.rs b/src/backend/kitty.rs index 10643f4..02c32e0 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -86,29 +86,68 @@ impl Kitty { .filter(|s| !s.is_empty()) } - /// Turn kitty's refusal into the fix for the mode actually in force. - /// - /// `allow_remote_control` has several values and they need different - /// answers: telling a `socket-only` user to set `yes` is wrong advice, and - /// its real cause — no socket to talk to — is not obvious from kitty's - /// message alone. /// The socket recommendation, given once so the three places that offer it - /// cannot drift apart — and correct per platform: `$XDG_RUNTIME_DIR` is a - /// systemd convention, normally unset on macOS, where it would expand to - /// nothing and leave the socket at the filesystem root. - const SOCKET_ADVICE: &'static str = " allow_remote_control socket-only\n \ - listen_on unix:${TMPDIR}/kitty-{kitty_pid} # macOS: $TMPDIR is per-user, 0700\n \ - listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid} # Linux\n\ - Put it somewhere only you can open. A socket in /tmp is reachable by anyone with \ - write permission on it, which under a group-writable umask is more than just you."; + /// 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") { + "${TMPDIR}" + } else { + "${XDG_RUNTIME_DIR}" + }; + format!( + " allow_remote_control socket-only\n \ + listen_on unix:{dir}/kitty-{{kitty_pid}}\n\ + Keep the socket somewhere only you can open — the path above already is. \ + 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: everything after the value is part of it." + ) + } + /// 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() + ); + } + + // 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 something to depend on across versions, and by this - // point we already know it is a remote-control failure. + // 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 @@ -119,7 +158,7 @@ impl Kitty { 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 + Self::socket_advice() ), Some(sock) => format!( "the socket at {sock} was refused. Restart kitty so the running \ @@ -128,19 +167,13 @@ impl Kitty { }; } - 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(); } format!( @@ -148,7 +181,7 @@ impl Kitty { ~/.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 + Self::socket_advice() ) } From f8004b928da765e2b4c74977161fed7fa5b91d78 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 23:36:55 -0700 Subject: [PATCH 07/14] Address sixth review: fail closed on certain mismatch; fix hint ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex asked for fail-closed rather than emit-then-warn, and it is right for the case where the mismatch is *known*: after a previous tcap, the last non-empty output is that capture, and `tcap --output | some-service` has already sent it by the time stderr is read. That case now refuses with exit 1. Where a mismatch is merely possible — the last command may have printed nothing — the capture proceeds and is labelled, since refusing every kitty capture would remove the backend for the common case, which works correctly. From Claude's review: - `socket` was tested before `no matching`/`no window`. kitty's error usually echoes the socket path it was given, so a genuine stale-instance failure hit the substring on the user's own path and was told to restart kitty instead of to stop sharing a socket between instances. Specific phrases now go first. - fetch returned Ok with an empty body, rendering as a bare header with nothing under it — the exact symptom this PR is named for. It now fails with the same explanation doctor gives. - The Linux socket advice recommended $XDG_RUNTIME_DIR unconditionally, but that is a systemd convention, absent on non-systemd distros, in containers and under some su/ssh paths, where it expands to nothing and puts the socket at /. It is only suggested when actually set. - The approximate warning fired in --command mode, where the body is the shell's exact record and kitty's text is never printed. Also measured `--match state:self`, suggested as a way to drop the $KITTY_WINDOW_ID dependency: it does resolve the invoking window even unfocused, but fails without that variable ("No matching windows for expression: state:self"), so it hides the same dependency behind a worse message. Keeping id:N, with the finding recorded so it is not re-investigated. 🤖 Built with SMT --- src/backend/kitty.rs | 72 ++++++++++++++++++++++++++++++++------------ src/main.rs | 72 ++++++++++++++++++++++++-------------------- 2 files changed, 91 insertions(+), 53 deletions(-) diff --git a/src/backend/kitty.rs b/src/backend/kitty.rs index 02c32e0..cd56bd4 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -104,17 +104,31 @@ impl Kitty { // macOS $TMPDIR is already per-user and 0700; $XDG_RUNTIME_DIR is the // equivalent on Linux. let dir = if cfg!(target_os = "macos") { - "${TMPDIR}" + // Already per-user and 0700. + Some("${TMPDIR}".to_string()) } else { - "${XDG_RUNTIME_DIR}" + // 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}}"), + None => " listen_on unix:/run/user/$(id -u)/kitty-{kitty_pid} <- or any \ + directory only you can open; $XDG_RUNTIME_DIR is unset here" + .to_string(), + }; + format!( - " allow_remote_control socket-only\n \ - listen_on unix:{dir}/kitty-{{kitty_pid}}\n\ - Keep the socket somewhere only you can open — the path above already is. \ - 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: everything after the value is part of it." + " allow_remote_control socket-only\n{line}\n\ + 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: everything after the \ + value is part of it." ) } @@ -141,6 +155,15 @@ impl Kitty { ); } + 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". // @@ -167,15 +190,6 @@ impl Kitty { }; } - 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(); - } - 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 \ @@ -197,6 +211,11 @@ impl Kitty { /// 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. @@ -276,9 +295,22 @@ impl Backend for Kitty { let window = Self::require_window()?; let args = self.get_text_args(&listen_on, &window, raw); - run(&self.exe, &args) - .map(Fetched::from) - .map_err(|e| anyhow!("{e}\n\n{}", Self::remote_control_hint(&e, &listen_on))) + 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. + if text.trim().is_empty() { + 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." + )); + } + + Ok(Fetched::from(text)) } fn diagnose(&self) -> Vec { diff --git a/src/main.rs b/src/main.rs index 53d2016..5fa36aa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -129,19 +129,36 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { )); } + // Unfiltered: nth_from_end skips tcap invocations, but here we need to know + // whether tcap itself ran most recently. + let after_tcap = records + .last() + .map(|r| state::is_tcap_invocation(&r.command)) + .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 was tcap itself, and {} can only\n\ + report the last non-empty output — which is that capture, not a command.\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}"); if !settings.quiet { - // Unfiltered: nth_from_end skips tcap invocations, but for this warning - // we need to know whether tcap itself ran most recently. - let after_tcap = records - .last() - .map(|r| state::is_tcap_invocation(&r.command)) - .unwrap_or(false); - emit_hints(&caps, backend.as_ref(), mode, after_tcap); + emit_hints(&caps, backend.as_ref(), mode); } if settings.copy { @@ -153,32 +170,21 @@ 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, after_tcap: bool) { - // Backends without real command boundaries return the last *non-empty* - // output. tcap's own output is non-empty, so a capture straight after - // another one hands back the previous capture rather than a command. - // Every mode, not just annotated: --output and --raw are the ones piped - // into another tool, so they are exactly where silently handing over an - // earlier command's text does the most damage. stderr keeps the pipe clean, - // and --quiet still silences it. - if caps.iter().any(|c| c.approximate) { - if after_tcap { - eprintln!( - "tcap: the previous command was tcap itself, and {} can only report the\n\ - \x20 last non-empty output — so this is most likely the previous capture's\n\ - \x20 own output, or whatever its pipeline printed, rather than a command's.\n\ - \x20 Re-run the command you meant to capture, or use tmux, which records\n\ - \x20 exact boundaries.", - backend.name() - ); - } else { - eprintln!( - "tcap: {} returns the last non-empty output, so if the command named above\n\ - \x20 printed nothing this is an earlier command's output. tmux records\n\ - \x20 exact boundaries; --quiet silences this.", - backend.name() - ); - } +fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) { + // 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, so if the command named above\n\ + \x20 printed nothing this is an earlier command's output. tmux records\n\ + \x20 exact boundaries; --quiet silences this.", + backend.name() + ); } // Only the annotated view promises metadata, so only it can disappoint. From f43c94386b8699596554e42c6ed589be1e117528 Mon Sep 17 00:00:00 2001 From: Brent G Date: Thu, 10 Sep 2026 23:53:58 -0700 Subject: [PATCH 08/14] Address seventh review: detect captures by marker, not by parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex was right that the fail-closed guard rested on `is_tcap_invocation`, which only reads command text — so `command tcap`, `env tcap`, an alias or a shell function walked straight past it and a following `tcap --output | some-service` would send the previous capture. The reliable signal was already available: tcap captures *during* a command, and the hook records that same command when it finishes. A capture now drops a marker which `__record` collects into `was_capture` on the record it writes. That holds whatever the command was called. Verified live with `alias t=tcap`: the capture through `t` is recognised and the next one refuses, though the recorded text is just "t". The text heuristic stays as a fallback for records written before the marker existed, and now also looks past `command`, `builtin`, `exec`, `env`, `nohup`, `time` and a leading backslash — with tests for each, as asked. 🤖 Built with SMT --- src/main.rs | 11 ++++-- src/state.rs | 101 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 98 insertions(+), 14 deletions(-) diff --git a/src/main.rs b/src/main.rs index 5fa36aa..3e4068c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -90,6 +90,7 @@ fn record(args: &RecordArgs) -> Result<()> { duration_ms, start_line, end_line, + was_capture: state::take_capture_marker(), }) } @@ -129,11 +130,11 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { )); } - // Unfiltered: nth_from_end skips tcap invocations, but here we need to know - // whether tcap itself ran most recently. + // Unfiltered: nth_from_end skips captures, but here we need to know whether + // the most recent command *was* one. let after_tcap = records .last() - .map(|r| state::is_tcap_invocation(&r.command)) + .map(state::is_capture_record) .unwrap_or(false); // Refuse rather than emit, in the one case where the output is known to be @@ -157,6 +158,10 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { println!("{text}"); + // Marked after emitting: the next command's hook collects this, so a + // following capture knows the screen's last output is ours. + state::mark_capture(); + if !settings.quiet { emit_hints(&caps, backend.as_ref(), mode); } diff --git a/src/state.rs b/src/state.rs index 97c387f..eed79f3 100644 --- a/src/state.rs +++ b/src/state.rs @@ -34,6 +34,32 @@ 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() { + let _ = fs::create_dir_all(state_dir()); + let _ = fs::write(marker_path(), b"1"); +} + +/// Whether a capture happened during this command, clearing the marker. +pub fn take_capture_marker() -> bool { + fs::remove_file(marker_path()).is_ok() } /// Directory holding one log per terminal session. @@ -134,26 +160,43 @@ 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) }) } +/// Whether this record is a tcap capture, by any spelling. +/// +/// `was_capture` is authoritative: the hook set it during the command that +/// captured, so it holds for aliases and shell functions that no amount of text +/// matching would catch. +pub fn is_capture_record(r: &Record) -> bool { + r.was_capture || is_tcap_invocation(&r.command) +} + /// 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> { @@ -176,6 +219,7 @@ mod tests { duration_ms: None, start_line: None, end_line: None, + was_capture: false, } } @@ -189,6 +233,41 @@ 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 marker catches what text matching cannot: an alias or function whose + /// name says nothing about tcap. + #[test] + fn was_capture_beats_the_text_heuristic() { + let mut aliased = rec("t"); + assert!( + !is_capture_record(&aliased), + "text alone cannot see an alias" + ); + aliased.was_capture = true; + assert!( + is_capture_record(&aliased), + "the hook's marker is authoritative" + ); + + assert!(is_capture_record(&rec("tcap --json"))); + assert!(!is_capture_record(&rec("npm run build"))); + } + #[test] fn does_not_match_unrelated_commands() { assert!(!is_tcap_invocation("npm run build")); From ac4c2fb1ea893079717ec5a0f319beb3a2f1087d Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 00:04:30 -0700 Subject: [PATCH 09/14] Address eighth review: fix indexing regression and false refusals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers caught the same regression: I added `is_capture_record` but left `nth_from_end` on the old text heuristic, so a capture made through an alias was still indexed as a real command — pairing its metadata with unrelated output, on tmux as much as kitty. Fixed, with a test. Then separating two questions I had conflated: - Indexing asks "was this a tcap run at all". The text heuristic is fine there. - The refusal asks "is tcap's own text the last thing on screen". That is true only when the capture actually displayed, so it uses the marker alone. `tcap --output > notes.txt` prints nothing to the terminal, so the screen's last output is still a real command's and the next capture is valid. Gating the marker on `stdout().is_terminal()` was half the fix; while the refusal still consulted the text heuristic it kept refusing anyway. Verified live: after a redirected capture the next one succeeds, after an on-screen one it refuses. Also, the $XDG_RUNTIME_DIR-unset branch broke the paste-safety rule documented eight lines above it — a trailing explanation on the value line, plus `$(id -u)`, which kitty.conf does not substitute either. Concrete path, caveat in the prose. Docs updated: two runs in a row are now an error, not a labelled capture. 🤖 Built with SMT --- README.md | 8 ++++---- npm/README.md | 7 ++++--- src/backend/kitty.rs | 19 +++++++++++++------ src/main.rs | 25 ++++++++++++++++--------- src/state.rs | 15 ++++++++++++++- 5 files changed, 51 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 0b7d0b6..c0c38b5 100644 --- a/README.md +++ b/README.md @@ -162,10 +162,10 @@ 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 — and two -`tcap` runs in a row return the first run's own output, since that was the last -thing to print. The annotated header carries a `# note:` line saying so, and `--json` sets -`"approximate": true`. tmux has no such +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 diff --git a/npm/README.md b/npm/README.md index b42688b..853364c 100644 --- a/npm/README.md +++ b/npm/README.md @@ -111,9 +111,10 @@ latest output and nothing older; iTerm2 exposes only its latest prompt. So `tcap block. **On kitty, output is the last *non-empty* output**, because kitty counts the -running `tcap` as the current command. So a command that printed nothing, or two -`tcap` runs in a row, yields an older output under the current header. The header carries a `# note:` line saying so, and `--json` sets -`"approximate": true`; tmux has no such ambiguity. +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/src/backend/kitty.rs b/src/backend/kitty.rs index cd56bd4..16d1d95 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -118,17 +118,24 @@ impl Kitty { let line = match &dir { Some(d) => format!(" listen_on unix:{d}/kitty-{{kitty_pid}}"), - None => " listen_on unix:/run/user/$(id -u)/kitty-{kitty_pid} <- or any \ - directory only you can open; $XDG_RUNTIME_DIR is unset here" - .to_string(), + // 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\ + " 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: everything after the \ - value is part of it." + 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 { + "" + } ) } diff --git a/src/main.rs b/src/main.rs index 3e4068c..d36dfda 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}; @@ -130,12 +131,14 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { )); } - // Unfiltered: nth_from_end skips captures, but here we need to know whether - // the most recent command *was* one. - let after_tcap = records - .last() - .map(state::is_capture_record) - .unwrap_or(false); + // 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 @@ -158,9 +161,13 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { println!("{text}"); - // Marked after emitting: the next command's hook collects this, so a - // following capture knows the screen's last output is ours. - state::mark_capture(); + // Only when the text actually reached the screen. Under `tcap --output > + // notes.txt` or `| pbcopy` nothing new is displayed, so marking would make + // the next capture refuse for a collision that never happened — in exactly + // the piping workflows the README leads with. + if std::io::stdout().is_terminal() { + state::mark_capture(); + } if !settings.quiet { emit_hints(&caps, backend.as_ref(), mode); diff --git a/src/state.rs b/src/state.rs index eed79f3..c1a9fac 100644 --- a/src/state.rs +++ b/src/state.rs @@ -203,7 +203,7 @@ pub fn nth_from_end(records: &[Record], n: usize) -> Option<&Record> { records .iter() .rev() - .filter(|r| !is_tcap_invocation(&r.command)) + .filter(|r| !is_capture_record(r)) .nth(n.saturating_sub(1)) } @@ -286,6 +286,19 @@ mod tests { assert!(nth_from_end(&records, 3).is_none()); } + /// An aliased capture has no tcap in its text, so only `was_capture` keeps + /// it from being indexed as a real command — which would pair its metadata + /// with unrelated output, on tmux as much as kitty. + #[test] + fn nth_skips_captures_made_through_an_alias() { + let mut aliased = rec("t"); + aliased.was_capture = true; + let records = vec![rec("npm run build"), rec("ls"), aliased]; + + assert_eq!(nth_from_end(&records, 1).unwrap().command, "ls"); + assert_eq!(nth_from_end(&records, 2).unwrap().command, "npm run build"); + } + #[test] fn nth_returns_none_past_the_start_of_history() { assert!(nth_from_end(&[], 1).is_none()); From 3871c424d8925c75f6642eede094f8ed0d3e6fa5 Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 00:16:15 -0700 Subject: [PATCH 10/14] Address ninth review: harden state files, mark on any terminal output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex found a real vulnerability class, and it predates the marker: state files live at predictable paths under a directory that on Linux falls back to the shared /tmp, and plain writes follow symlinks. Another local user could pre-create the directory and leave a symlink where a state file goes, and tcap would truncate whatever it pointed at. The directory is now created 0700 and checked with symlink_metadata to be a real directory owned by the caller, and every state write opens with O_NOFOLLOW and mode 0600 — the log as much as the marker, since the log always had the same exposure. Verified by planting symlinks at both exact paths: the marker write leaves the target intact and the log write fails with ELOOP. Both reviewers also noted the marker was set only on the successful stdout path, while warnings and errors still print to the terminal — so `tcap --output | svc` leaves its warning on kitty's screen unmarked, and the next capture could return that. Marking now happens for any tcap output that reaches a terminal, stdout or stderr, success or failure, excluding the record hook which prints nothing. libc is added for getuid, O_NOFOLLOW and the mode bits, replacing a hand-rolled extern "C" getuid. 🤖 Built with SMT --- Cargo.lock | 7 ++++++ Cargo.toml | 1 + src/main.rs | 34 ++++++++++++++++++++-------- src/state.rs | 64 ++++++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 84 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3eb121b..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" @@ -249,6 +255,7 @@ version = "0.1.1" dependencies = [ "anyhow", "clap", + "libc", "serde", "serde_json", "toml", diff --git a/Cargo.toml b/Cargo.toml index d705156..851fb3a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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/src/main.rs b/src/main.rs index d36dfda..89f3674 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,12 +20,34 @@ use cli::{Cli, RecordArgs, ShellKind, Sub}; use state::Record; fn main() { - if let Err(e) = run() { - eprintln!("tcap: {e:#}"); + let failed = match run() { + Ok(()) => false, + Err(e) => { + eprintln!("tcap: {e:#}"); + true + } + }; + + // Whatever tcap just wrote to the terminal — a capture, a warning, or an + // error — is now the screen's last non-empty output, which is what a + // boundary-less backend would hand back next time. Marking only the + // successful stdout path left the warnings and errors unaccounted for, and + // `tcap --output | svc` still prints its warning to the tty. + // + // The record hook is excluded: it prints nothing. + if !matches!(std::env::args().nth(1).as_deref(), Some("__record")) && wrote_to_terminal() { + state::mark_capture(); + } + + if failed { std::process::exit(1); } } +fn wrote_to_terminal() -> bool { + std::io::stdout().is_terminal() || std::io::stderr().is_terminal() +} + fn run() -> Result<()> { let cli = Cli::parse(); @@ -161,14 +183,6 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { println!("{text}"); - // Only when the text actually reached the screen. Under `tcap --output > - // notes.txt` or `| pbcopy` nothing new is displayed, so marking would make - // the next capture refuse for a collision that never happened — in exactly - // the piping workflows the README leads with. - if std::io::stdout().is_terminal() { - state::mark_capture(); - } - if !settings.quiet { emit_hints(&caps, backend.as_ref(), mode); } diff --git a/src/state.rs b/src/state.rs index c1a9fac..cc4f1d9 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::os::unix::fs::{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. @@ -53,8 +54,7 @@ fn marker_path() -> PathBuf { } pub fn mark_capture() { - let _ = fs::create_dir_all(state_dir()); - let _ = fs::write(marker_path(), b"1"); + let _ = secure_open(&marker_path()).map(|mut f| f.write_all(b"1")); } /// Whether a capture happened during this command, clearing the marker. @@ -72,12 +72,52 @@ 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(); + fs::create_dir_all(&dir).with_context(|| format!("creating {}", dir.display()))?; + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)) + .with_context(|| format!("securing {}", dir.display()))?; + + // symlink_metadata, so a symlink in place of the directory is caught rather + // than followed. + let md = fs::symlink_metadata(&dir)?; + if !md.is_dir() { + return Err(anyhow!("{} is not a directory", dir.display())); } - unsafe { getuid() } + if md.uid() != current_uid() { + return Err(anyhow!( + "{} is owned by uid {}, not you — refusing to use it", + dir.display(), + md.uid() + )); + } + Ok(dir) +} + +/// 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. @@ -112,9 +152,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)?; @@ -122,6 +160,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}")?; @@ -142,7 +182,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(()) } From f2b728dd4bd2731dd7233b00925b791d640ee27f Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 00:36:51 -0700 Subject: [PATCH 11/14] Address tenth review: mark where output is written, not by isatty at exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's "mark if a terminal is attached" was wrong three ways, all found by review: - `eval "$(tcap init zsh)"` pipes stdout but leaves stderr a tty, so every shell startup marked itself. The marker then survived to the user's first real command, which was filtered out of indexing and hard-refused on kitty — naming "tcap" for a command that was `ls`. This hit tmux users too, and the e2e suite missed it only because it redirects both streams during setup, which real installs do not. - `tcap --output > notes.txt` marked despite displaying nothing, the exact false refusal the commit claimed to fix. - clap prints `--help`, `--version` and usage errors itself and exits, so none of the marking ran at all: `tcap --help` then `tcap` returned the help text as a command's output. Marking now happens at each write, gated on that stream being a terminal: stdout for captures, doctor and init; stderr for hints and errors; clap routed through try_parse so its own output counts. The record hook prints nothing and so never marks. Verified live: a fresh shell captures its first real command. Also from review: state reads bypassed the checks writes had just gained — a pre-created directory with a crafted log could inject records whose command text is printed back, escape sequences included. Reads go through the same ownership check and O_NOFOLLOW now. And `set_permissions` ran before the symlink check, so a planted symlink would have had its target chmod'ed before being rejected; the directory is created 0700 up front and inspected before anything is mutated. Both verified by planting symlinks: the write fails with ELOOP, the read returns nothing, and a symlinked state dir is refused with its target's mode left untouched. 🤖 Built with SMT --- src/main.rs | 76 +++++++++++++++++++++++++++++++++------------------- src/state.rs | 58 +++++++++++++++++++++++++++++++-------- 2 files changed, 96 insertions(+), 38 deletions(-) diff --git a/src/main.rs b/src/main.rs index 89f3674..81cdaf3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,46 +20,61 @@ use cli::{Cli, RecordArgs, ShellKind, Sub}; use state::Record; fn main() { - let failed = match run() { - Ok(()) => false, - Err(e) => { - eprintln!("tcap: {e:#}"); - true - } - }; - - // Whatever tcap just wrote to the terminal — a capture, a warning, or an - // error — is now the screen's last non-empty output, which is what a - // boundary-less backend would hand back next time. Marking only the - // successful stdout path left the warnings and errors unaccounted for, and - // `tcap --output | svc` still prints its warning to the tty. - // - // The record hook is excluded: it prints nothing. - if !matches!(std::env::args().nth(1).as_deref(), Some("__record")) && wrote_to_terminal() { - state::mark_capture(); + if let Err(e) = run() { + eprintln!("tcap: {e:#}"); + mark_if_stderr_is_terminal(); + std::process::exit(1); } +} - if failed { - 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() { + state::mark_capture(); } } -fn wrote_to_terminal() -> bool { - std::io::stdout().is_terminal() || std::io::stderr().is_terminal() +fn mark_if_stderr_is_terminal() { + if std::io::stderr().is_terminal() { + state::mark_capture(); + } } 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(); + if e.use_stderr() { + mark_if_stderr_is_terminal(); + } else { + 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 @@ -182,9 +197,11 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { let text = render::render(&caps, mode); println!("{text}"); + mark_if_stdout_is_terminal(); - if !settings.quiet { - emit_hints(&caps, backend.as_ref(), mode); + if !settings.quiet && emit_hints(&caps, backend.as_ref(), mode) { + // The hint itself is now the screen's last output. + mark_if_stderr_is_terminal(); } if settings.copy { @@ -196,7 +213,9 @@ 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, @@ -211,15 +230,16 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) \x20 exact 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(); @@ -247,6 +267,8 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) backend.name() ); } + + true } fn detect_shell() -> String { diff --git a/src/state.rs b/src/state.rs index cc4f1d9..88c8ebb 100644 --- a/src/state.rs +++ b/src/state.rs @@ -10,8 +10,8 @@ use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; use std::fs::{self, OpenOptions}; -use std::io::Write; -use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; +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 @@ -86,15 +86,28 @@ fn current_uid() -> u32 { /// already per-user) but the check is cheap and the fallback is not. fn ensure_state_dir() -> Result { let dir = state_dir(); - fs::create_dir_all(&dir).with_context(|| format!("creating {}", dir.display()))?; - fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)) - .with_context(|| format!("securing {}", dir.display()))?; - // symlink_metadata, so a symlink in place of the directory is caught rather - // than followed. - let md = fs::symlink_metadata(&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 not a directory", dir.display())); + return Err(anyhow!( + "{} is a symlink or not a directory — refusing to use it", + dir.display() + )); } if md.uid() != current_uid() { return Err(anyhow!( @@ -103,9 +116,32 @@ fn ensure_state_dir() -> Result { 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()))?; + } + 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) +} + /// Open a state file for writing without following symlinks. fn secure_open(path: &Path) -> Result { ensure_state_dir()?; @@ -172,7 +208,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(); @@ -190,7 +226,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() From ccc2db7cffa6437af025ead2bf2dc4f75e544e1a Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 01:05:30 -0700 Subject: [PATCH 12/14] Address eleventh review: harden the iTerm2 shim, widen the caveat wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shim write was the same vulnerability class, in a path the last commit missed: `iterm2_capture.py` was written with a plain `fs::write` to a predictable state path and then handed to `python3`. A symlink or pre-created file there is code execution, not a truncated file. It goes through the ownership-checked, O_NOFOLLOW path now, and an audit found no other unhardened state writes. Wording, both from review: - The `# note:` line and the stderr hint asserted the body was an *earlier* command's. After `tcap --output | llm` the downstream tool's output is what is on screen, so it can as easily belong to a later one. Both now say it may not belong to the named command, and name the two ways that happens. - The refusal said "the previous command was tcap itself". The marker means tcap's output is on screen, which a script that merely calls tcap also satisfies, so it no longer claims what ran. iTerm2's `has_command_boundaries()` is now explicitly flagged as an assumption: `async_get_last_prompt` may resolve to the prompt tcap is running under, which is the same trap that made kitty's `last_cmd_output` return tcap's own empty output. Nobody has run that backend; `tcap doctor` under iTerm2 would settle it. README test count corrected to 59. 🤖 Built with SMT --- README.md | 2 +- src/backend/iterm2.rs | 25 ++++++++++++++++++------- src/main.rs | 12 +++++++----- src/render.rs | 3 ++- src/state.rs | 11 +++++++++++ 5 files changed, 39 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index c0c38b5..66fc321 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,7 @@ under scrolling: as lines scroll off, `history_size` grows by exactly as much as ## Development ```sh -cargo test # 56 unit + 11 end-to-end tests +cargo test # 59 unit + 11 end-to-end tests git config core.hooksPath .githooks # run fmt, clippy and tests before every push ``` 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/main.rs b/src/main.rs index 81cdaf3..722d428 100644 --- a/src/main.rs +++ b/src/main.rs @@ -185,8 +185,9 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { // 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 was tcap itself, and {} can only\n\ - report the last non-empty output — which is that capture, not a command.\n\n\ + "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() @@ -225,9 +226,10 @@ fn emit_hints(caps: &[Capture], backend: &dyn backend::Backend, mode: cli::Mode) // 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, so if the command named above\n\ - \x20 printed nothing this is an earlier command's output. tmux records\n\ - \x20 exact boundaries; --quiet silences this.", + "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; diff --git a/src/render.rs b/src/render.rs index a711a32..45f9300 100644 --- a/src/render.rs +++ b/src/render.rs @@ -131,7 +131,8 @@ fn header(cap: &Capture) -> String { // 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 — it may be an earlier command's\n", + "# note: {} returns the last non-empty output, which may not be this \ + command's\n", cap.source )); } diff --git a/src/state.rs b/src/state.rs index 88c8ebb..1e6e1b3 100644 --- a/src/state.rs +++ b/src/state.rs @@ -142,6 +142,17 @@ fn secure_read(path: &Path) -> Option { 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()?; From a14ce87815b216f2ef3f91636bec0bdbd99b963d Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 01:17:39 -0700 Subject: [PATCH 13/14] Address twelfth review: stop was_capture hiding real commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A regression I introduced two commits ago, and the reverse of what an earlier round advised: `nth_from_end` was switched to `is_capture_record`, which ORs in `was_capture`. But that flag means "tcap wrote to this terminal during this command", which is equally true of a wrapper script, a Makefile target or a git alias that calls tcap. Those were being skipped entirely, so `-c 1` returned the command before them — on tmux too, where boundaries are exact and none of this machinery applies, and with header and body mutually consistent so nothing looks wrong. Indexing is back on the text check. The two signals answer different questions and now stay in their own places: `was_capture` gates the refusal, the text check gates indexing. Conflating them has now broken each direction once, so there is a test asserting they are independent. The cost is accepting that an aliased capture is indexable — visibly odd, where silently skipping someone's deploy script is not. Also: - The `# note:` was dropped whenever there was no other metadata, which is exactly kitty without shell integration — approximate body, nothing in-band to say so. It now renders on its own. - `secure_read` returning None for a refused directory made `load` look like a fresh session, silently switching off the guard built on it. The reason is surfaced instead. - `mark_capture` swallowed write failures, so a full or read-only filesystem left the guard off unannounced. It reports now, without being fatal: refusing to capture because a temp file is unwritable would be the worse trade. 🤖 Built with SMT --- README.md | 2 +- src/main.rs | 28 +++++++++++++++++-- src/render.rs | 18 +++++++++++- src/state.rs | 76 ++++++++++++++++++++++++++++++--------------------- 4 files changed, 89 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 66fc321..aa48bf9 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,7 @@ under scrolling: as lines scroll off, `history_size` grows by exactly as much as ## Development ```sh -cargo test # 59 unit + 11 end-to-end tests +cargo test # 60 unit + 11 end-to-end tests git config core.hooksPath .githooks # run fmt, clippy and tests before every push ``` diff --git a/src/main.rs b/src/main.rs index 722d428..defdf39 100644 --- a/src/main.rs +++ b/src/main.rs @@ -37,13 +37,26 @@ fn main() { /// `--help` exits before any of it runs. fn mark_if_stdout_is_terminal() { if std::io::stdout().is_terminal() { - state::mark_capture(); + warn_if_unmarked(state::mark_capture()); } } fn mark_if_stderr_is_terminal() { if std::io::stderr().is_terminal() { - state::mark_capture(); + 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." + ); } } @@ -134,6 +147,17 @@ fn record(args: &RecordArgs) -> Result<()> { 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; diff --git a/src/render.rs b/src/render.rs index 45f9300..04d540e 100644 --- a/src/render.rs +++ b/src/render.rs @@ -146,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); @@ -288,6 +292,18 @@ 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); diff --git a/src/state.rs b/src/state.rs index 1e6e1b3..9c44ce1 100644 --- a/src/state.rs +++ b/src/state.rs @@ -53,8 +53,9 @@ fn marker_path() -> PathBuf { state_dir().join(format!("{}.capturing", session_key())) } -pub fn mark_capture() { - let _ = secure_open(&marker_path()).map(|mut f| f.write_all(b"1")); +pub fn mark_capture() -> Result<()> { + secure_open(&marker_path())?.write_all(b"1")?; + Ok(()) } /// Whether a capture happened during this command, clearing the marker. @@ -62,6 +63,15 @@ 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. pub fn state_dir() -> PathBuf { let base = std::env::var_os("TMPDIR") @@ -275,22 +285,18 @@ pub fn is_tcap_invocation(cmd: &str) -> bool { }) } -/// Whether this record is a tcap capture, by any spelling. -/// -/// `was_capture` is authoritative: the hook set it during the command that -/// captured, so it holds for aliases and shell functions that no amount of text -/// matching would catch. -pub fn is_capture_record(r: &Record) -> bool { - r.was_capture || is_tcap_invocation(&r.command) -} - /// 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() - .filter(|r| !is_capture_record(r)) + .filter(|r| !is_tcap_invocation(&r.command)) .nth(n.saturating_sub(1)) } @@ -336,23 +342,29 @@ mod tests { } } - /// The marker catches what text matching cannot: an alias or function whose - /// name says nothing about tcap. + /// 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 was_capture_beats_the_text_heuristic() { + fn the_two_capture_signals_are_independent() { let mut aliased = rec("t"); - assert!( - !is_capture_record(&aliased), - "text alone cannot see an alias" - ); aliased.was_capture = true; assert!( - is_capture_record(&aliased), - "the hook's marker is authoritative" + !is_tcap_invocation(&aliased.command), + "text cannot see an alias" ); - assert!(is_capture_record(&rec("tcap --json"))); - assert!(!is_capture_record(&rec("npm run build"))); + 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] @@ -373,16 +385,18 @@ mod tests { assert!(nth_from_end(&records, 3).is_none()); } - /// An aliased capture has no tcap in its text, so only `was_capture` keeps - /// it from being indexed as a real command — which would pair its metadata - /// with unrelated output, on tmux as much as kitty. + /// 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_skips_captures_made_through_an_alias() { - let mut aliased = rec("t"); - aliased.was_capture = true; - let records = vec![rec("npm run build"), rec("ls"), aliased]; + 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, "ls"); + assert_eq!(nth_from_end(&records, 1).unwrap().command, "./deploy.sh"); assert_eq!(nth_from_end(&records, 2).unwrap().command, "npm run build"); } From feb017377f19a3852ed0826fd40bb652b7587d67 Mon Sep 17 00:00:00 2001 From: Brent G Date: Fri, 11 Sep 2026 01:33:25 -0700 Subject: [PATCH 14/14] Mark only on captured text; whitespace output is real output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marking on stderr made `tcap --output | llm` refuse the *next* capture, because the hint printed to a tty while the captured text went to the pipe — and the outcome flipped on whether --quiet was passed. A one-line diagnostic is not the text this guard is about. Marking now happens only where captured content is printed: the capture itself, doctor, init, and --help. Errors and hints do not mark, so a failed `tcap -c 2` no longer poisons the following capture. kitty's empty check used `trim()`, which discarded legitimate whitespace-only output such as `printf ' \n'` — and --raw/--output promise exact bytes. Only newlines now count as nothing. Adds the missing round-trip test for the marker file, which decides whether a capture is refused and had no coverage at all. Documents the one case the marker cannot see: after `tcap --output | llm` the downstream tool's answer is what is on screen, with no marker set, so the next capture is labelled rather than refused. tcap cannot observe what another process printed; that is a limitation to state, not a check to add. 🤖 Built with SMT --- AGENTS.md | 11 +++++++++++ README.md | 2 +- src/backend/kitty.rs | 4 +++- src/main.rs | 24 +++++++++++------------- src/state.rs | 13 +++++++++++++ 5 files changed, 39 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1f90679..d91588e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,6 +58,17 @@ either the escape-code channel or `--to $KITTY_LISTEN_ON`; under `allow_remote_c 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. diff --git a/README.md b/README.md index aa48bf9..6c56e2e 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,7 @@ under scrolling: as lines scroll off, `history_size` grows by exactly as much as ## Development ```sh -cargo test # 60 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/src/backend/kitty.rs b/src/backend/kitty.rs index 16d1d95..7a179ea 100644 --- a/src/backend/kitty.rs +++ b/src/backend/kitty.rs @@ -308,7 +308,9 @@ impl Backend for Kitty { // `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. - if text.trim().is_empty() { + // 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\ diff --git a/src/main.rs b/src/main.rs index defdf39..4d895fa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,8 +21,9 @@ 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:#}"); - mark_if_stderr_is_terminal(); std::process::exit(1); } } @@ -41,12 +42,6 @@ fn mark_if_stdout_is_terminal() { } } -fn mark_if_stderr_is_terminal() { - if std::io::stderr().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 @@ -66,9 +61,10 @@ fn run() -> Result<()> { Err(e) => { // --help and --version print to stdout, usage errors to stderr. let _ = e.print(); - if e.use_stderr() { - mark_if_stderr_is_terminal(); - } else { + // --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()); @@ -224,9 +220,11 @@ fn capture_and_print(cli: &Cli, settings: &cli::Settings) -> Result<()> { println!("{text}"); mark_if_stdout_is_terminal(); - if !settings.quiet && emit_hints(&caps, backend.as_ref(), mode) { - // The hint itself is now the screen's last output. - mark_if_stderr_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); } if settings.copy { diff --git a/src/state.rs b/src/state.rs index 9c44ce1..4e72caa 100644 --- a/src/state.rs +++ b/src/state.rs @@ -400,6 +400,19 @@ mod tests { 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());