Fix kitty capture returning nothing

[guohai]

tcap after pwd in kitty printed the header with no output. Three bugs, none
covered 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:

extent	bytes
last_cmd_output	0
last_non_empty_output	20 (the real output)

Wrong window — without --match, get-text reads whichever window is
focused, not the one tcap runs in. Now pinned to $KITTY_WINDOW_ID.

Wrong advice — under allow_remote_control socket-only the error told the
user to set allow_remote_control yes, loosening a deliberately restricted
setting to fix an unrelated cause. The real fix is listen_on.

Verified live in kitty for both the socket and escape-code paths.

Caveat documented: the last non-empty output means a silent command yields an
older command's output under the current header. tmux is unaffected.

Generated with SMT smt@agora.build

[github-actions]

🤖 Codex Code Review

• High — Missing window ID still captures another window’s potentially secret contents. src/backend/kitty.rs:164 only adds --match when $KITTY_WINDOW_ID exists. Kitty can still be selected through $TERM or --backend kitty, so without that variable fetch() silently falls back to kitty’s focused window. This can place another pane’s scrollback—tokens, passwords, or other sensitive output—under the current command’s header. Since the PR identifies unscoped capture as unsafe, fetch() should fail when the window ID is unavailable rather than invoke get-text without a matcher. The diagnostic probe at src/backend/kitty.rs:205 should likewise avoid reading the focused window.

No other substantive issues found. git diff --check passed; cargo test could not complete because the restricted environment could not resolve index.crates.io.

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref (23617f8) against b8c193e. The core fix is right: last_non_empty_output is the correct extent given kitty counts the running tcap as the current command, --match id:$KITTY_WINDOW_ID genuinely closes a cross-window leak (previously a capture with focus elsewhere returned another window's scrollback), and --match does have to follow the subcommand. Args are built as separate argv elements with no shell, so nothing here is injectable, and diagnose checks the probe's emptiness without printing captured text. Four things below.

1. diagnose now throws away the underlying kitty error (src/backend/kitty.rs:196-200)

Err(e) => Diagnostic::bad("remote control", format!("{}\n{}", channel, Self::remote_control_hint(&e, &listen_on))),

e is consumed only for keyword matching. In the fallback branch of remote_control_hint — which is every error that isn't literally "socket only" or "password" — the real message (Unknown option: --match, Unknown extent, connection refused, kitty not running) never reaches the user. The pre-PR code interpolated {e}, and the fetch path still does. This is a diagnosability regression in the one command whose job is diagnosis. Suggest format!("{e}\n{channel}\n{}", hint).

2. The fallback hint misattributes unrelated failures

Any error that doesn't match the two keywords produces "kitty remote control appears to be disabled". A kitty too old for last_non_empty_output, or a non-numeric KITTY_WINDOW_ID making --match id:… fail, both land there and send the user editing allow_remote_control for a problem that isn't remote control. Fixing (1) mostly defuses this; alternatively make the fallback hedged ("if this is a remote-control problem…"). Same env caveat worth noting: KITTY_WINDOW_ID is interpolated into the match expression unvalidated — not injectable into a shell, but a poisoned value could in principle broaden the match to another window. A parse::<u64>() filter in window_match costs one line and also removes the misdiagnosis path.

3. fetch and diagnose build the same argv twice, independently (kitty.rs:154-171 vs kitty.rs:188-193)

Identical sequence — get-text, optional --match, --extent last_non_empty_output — duplicated. If someone changes the extent or the flag order in one, doctor reports "shell integration: resolves" while fetch fails, which is the worst possible failure mode for this tool. Extracting a fn get_text_args(&self, listen_on, ansi) -> Vec<&str> fixes that and makes the invariant testable: AGENTS.md now says all three kitty traps were "found in the field rather than by tests", but the --match-after-subcommand ordering and the extent choice are pure argv assertions that a unit test can lock in without kitty installed. remote_control_hint is likewise a pure function with no test. This PR adds no tests at all.

4. Undocumented consequence of last_non_empty_output: back-to-back tcap runs return tcap's own output

The module doc and README cover "last command printed nothing → older output, wrong header". The likelier case isn't covered: run tcap, then run tcap again (or tcap -r). The first invocation's rendered output is non-empty, so it becomes the last non-empty output and the second capture returns tcap's own formatted block, header included. Given fetch already receives rec, a cheap guard is to check whether the recorded command is tcap itself and warn; at minimum it belongs in the README caveat next to the empty-output one.

Doc nit worth fixing in this PR: AGENTS.md:64 still reads "kitty — implemented against --extent=last_cmd_output", directly contradicting the new paragraph eleven lines above it. AGENTS.md:48 has the same stale reference (README's equivalent line was updated; this one wasn't). Since AGENTS.md/CLAUDE.md is what future agents read as spec, a stale line here reintroduces the bug.

I was unable to run cargo test (command approval declined), so the above is static review only.

[github-actions]

🤖 Codex Code Review

• High — Password-protected kitty users are advised to disable authentication for scrollback access. src/backend/kitty.rs:114 recommends remote_control_password "" get-text. This grants passwordless access to get-text, whose output may contain credentials or tokens, and the permission is not constrained to tcap’s window. Any process able to reach kitty’s remote-control channel/socket could request terminal contents. Since tcap does not send passwords, it should report password-protected setups as unsupported or recommend a tightly permissioned Unix socket—not remove authentication for the sensitive capture operation.

No other concrete issues found. The numeric KITTY_WINDOW_ID validation and refusal of unscoped capture correctly prevent cross-window secret exposure. This PR does not change tmux coordinate arithmetic or shell hooks.

I could not run the Rust tests or Clippy because dependencies and the crates.io index were unavailable in this environment; git diff --check passed.

[github-actions]

🤖 Claude Code Review

Summary

The core fix is right and the reasoning is well documented. last_non_empty_output correctly avoids kitty counting the running tcap as the current command, --match is correctly placed after the subcommand, KITTY_WINDOW_ID is parsed as u64 so a poisoned variable can't widen the match expression, and require_window() refusing rather than falling back to the focused window is the right call for a tool that prints other people's scrollback. Sharing get_text_args between fetch and diagnose closes a real drift hazard. No shell is involved anywhere, and no captured text leaks into error strings. Test count in the README (54) matches the tree.

Findings below, roughly in order of how much they'd bite.

1. The remaining silent-mislabel case is the one the project's own rules forbid — and it's unwarned

src/backend/kitty.rs:1-12 documents it and README.md explains it, but nothing surfaces it at runtime. Sequence: ls (prints), cd /tmp (prints nothing), tcap. records.last() is cd /tmp, so after_tcap is false and no hint fires; kitty returns ls's output; build_capture (src/main.rs:213-227) stamps it with cd /tmp, exit 0 and that command's duration. The user gets authoritative-looking metadata over the wrong body, which is exactly the "silently returns the wrong text rather than failing" failure mode the tmux coordinate work exists to prevent.

The new after_tcap hint only covers the sub-case where the previous command was tcap. Consider making annotated mode on a boundary-less backend qualify what it's showing — e.g. render the source as kitty (last non-empty output) or drop the exit/duration line — rather than presenting shell-record metadata as if it described the returned text.

2. The after_tcap hint is kitty-specific but fires for iTerm2 too

src/main.rs:153-164 gates on !backend.supports_history(), which is also true for iTerm2. But iTerm2 fetches its latest prompt with real boundaries and its own command/exit code (src/backend/iterm2.rs:111-130) — it has no "last non-empty output" behaviour at all, so the stated cause is wrong there. Either gate on backend.name() == "kitty", or add a has_command_boundaries() (or similar) to the trait so the hint follows the actual property rather than a proxy for it.

Related wording nit in the same message: after tcap | sgpt "fix it" — the README's headline usage — is_tcap_invocation is true, but tcap's own output went into the pipe, so what kitty returns is sgpt's output, not tcap's. "probably tcap's own output" is confidently wrong in the most common invocation. "probably the previous capture's pipeline output, not a command's" would cover both.

3. window_match_requires_a_number doesn't test window_match

src/backend/kitty.rs:317-330 re-implements the parse inline (raw.trim().parse::<u64>().ok().map(...)) rather than calling the function. It asserts that the expression it just wrote does what it does; if window_match() regressed to, say, dropping the parse, this test would still pass — while being the only test guarding the security property the doc comment describes. Extract the parse as fn parse_window_id(raw: &str) -> Option<String>, have window_match() call it, and point the test at that. (Avoids the set_var raciness that presumably motivated the duplication.)

4. Minor: doctor can report a remote-control failure under the "shell integration" label

src/backend/kitty.rs:249 — when remote control is unreachable, the ls check fails (reported correctly) and the get-text probe fails with the same underlying error, which is then printed as a shell-integration fault. The ls line above it disambiguates, so this is cosmetic, but the Err branch also dropped all remediation text; skipping the probe when the ls check already failed would read better.

5. Note on the scope of the window guarantee

The doc comment at src/backend/kitty.rs:127-137 says --match prevents reading another window. That holds within one kitty instance — id:N is instance-local, and the target instance is whatever --to $KITTY_LISTEN_ON reaches. With a fixed listen_on path or an inherited KITTY_LISTEN_ON, id:N could resolve to an unrelated window in another instance. Not a regression and not worth code today, but the comment's guarantee is stated more absolutely than the mechanism supports; worth softening so a future reader doesn't lean on it.

[github-actions]

🤖 Codex Code Review

• Low — src/backend/kitty.rs:247: diagnose() executes the same kitty @ ... ls probe twice—once to set reachable, then again to construct the diagnostic. Besides unnecessary process/IPC work, the results can disagree: a transient first success followed by failure still causes the subsequent get-text probe to run, while the inverse skips it despite reporting remote control as healthy. Execute it once and derive both reachable and the diagnostic from that result.

• Low — src/backend/kitty.rs:327: window_match_requires_a_number duplicates parse_window_id instead of calling it. The production parser can regress while this security-oriented test continues passing. Use Kitty::parse_window_id(raw) in the assertion.

No other substantive security, coordinate-arithmetic, shell-hook, or data-exposure regressions found. The tmux and shell-hook paths are unchanged, tmux still wins detection, and kitty window IDs are constrained to numeric exact matches.

Tests could not run because dependencies were not cached and network access to crates.io is unavailable.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref. src/backend/tmux.rs and shell/ are untouched by this PR, so the scrollback-coordinate and $?-ordering invariants are unaffected. The core fix is sound: last_non_empty_output is the right extent given kitty counts the running tcap as current, --match correctly follows the subcommand, and scoping the capture to $KITTY_WINDOW_ID — refusing rather than falling back to the focused window — is a genuine fix for cross-pane leakage. The approximate flag plumbed through to the header and JSON is the right way to avoid presenting shell-record metadata as authoritative over text kitty can't vouch for.

Findings below.

1. The window-id guard is effectively untested (src/backend/kitty.rs:352)

parse_window_id is documented as "split out so the numeric guarantee is testable", but window_match_requires_a_number never calls it — it reimplements the parse inline:

let got = raw.trim().parse::<u64>().ok().map(|id| format!("id:{id}"));
assert_eq!(got.as_deref(), want, "input {raw:?}");

The test passes regardless of what parse_window_id does. If someone later relaxes it to pass the raw string through, the test stays green while KITTY_WINDOW_ID="all" or "state:focused" widens the match expression and dumps another window's scrollback — exactly the leak the guard exists to prevent. Change the body to call Kitty::parse_window_id(raw).

2. diagnose drops the "history depth" line when remote control is down (src/backend/kitty.rs:267)

if !reachable {
    return d;
}

This early-returns from the middle of the Some(w) arm, skipping the trailing history depth diagnostic. The None arm falls through and still emits it, so doctor output is inconsistent depending on an unrelated failure. Skipping the probe is right; skipping the rest of the function isn't. Wrap just the probe in if reachable { ... }.

3. kitty @ ls is spawned twice (src/backend/kitty.rs:243-245)

let reachable = run(&self.exe, &args).is_ok();
d.push(match run(&self.exe, &args) {

Two subprocess round-trips for one question, and the two can disagree (socket appearing/disappearing between them), leaving reachable inconsistent with the diagnostic just reported. let ls = run(&self.exe, &args); let reachable = ls.is_ok(); match ls { ... }.

4. The socket-only keyword match may not match kitty's actual message (src/backend/kitty.rs:98)

msg.contains("socket only") || msg.contains("socket-only") won't match a phrasing like "remote control is allowed only over a socket" — word order matters here and the only test coverage uses a string the PR invented ("Error: Remote control is allowed over a socket only"), not one captured from kitty. Failure is graceful (generic hint), but the whole point of this function is mode-specific advice. Matching on msg.contains("socket") inside a branch that already knows it's a remote-control failure would be more robust, or verify the literal against kitty 0.48.2's output since the rest of this PR was verified live.

5. "only your user can reach" overstates a /tmp socket (src/backend/kitty.rs:117-124, README)

The password hint recommends listen_on unix:/tmp/kitty-{kitty_pid} as a socket "only your user can reach". Connecting to a Unix socket needs write permission on it, so this holds under a default umask 022 — but under a group-writable umask (002/007, not unusual in shared or org-managed environments) any group member can connect and run get-text, which is precisely the credential-dumping call the surrounding comment is protecting. Since this text is offered as the security-preserving alternative, either recommend unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid} (0700 dir) or soften the claim.

Note: I did not run cargo test — it needed approval that wasn't granted, so the above is from reading only.

[github-actions]

🤖 Codex Code Review

• Medium – Socket setup advice can expose terminal scrollback to other local users. src/backend/kitty.rs:107 and src/backend/kitty.rs:139 recommend placing the unauthenticated remote-control socket directly in /tmp. With a group-writable or permissive umask, another local user may be able to connect and invoke get-text, exposing terminal contents and credentials. The password-specific branch already recognizes this risk and recommends a private 0700 directory at line 128; use that safe recommendation consistently. README.md:171 should be updated as well.

No other concrete regressions found. The tmux coordinate logic and shell hooks are unchanged, backend detection still prioritizes tmux, and the numeric window-ID validation prevents match-expression injection and accidental focused-window capture.

Tests could not be run because this review environment is read-only.

[github-actions]

🤖 Claude Code Review

No changes to src/backend/tmux.rs or shell/ in this PR, so the scrollback-coordinate arithmetic and the $?-first/single-argv invariants are untouched. The kitty fix itself is sound: --match correctly follows the get-text subcommand, the window id is parsed as u64 so a poisoned $KITTY_WINDOW_ID cannot widen the match expression, and require_window() refusing outright rather than letting kitty fall back to the focused window is the right call for a tool that dumps screen contents. The after_tcap check reading records.last() unfiltered is also correct — all three hooks call tcap __record at precmd, so the in-flight tcap is not yet in the log and last() really is the previous command.

Three things worth addressing:

1. approximate is invisible in exactly the modes most likely to be piped somewhere (src/render.rs:129, src/main.rs:156)

The # note: line is emitted only in Mode::Annotated, and --json carries the field. --output, --command and --raw get nothing. The stderr warning in emit_hints covers only the after-tcap case; the broader one — last command printed nothing (cd, export, git add, make -s) — is silent there.

So tcap --output | llm under kitty hands over an older command's output with no indication anywhere, which is the failure mode the rest of this codebase goes out of its way to avoid ("fail rather than return the wrong text"). emit_hints already writes to stderr and already honours --quiet, so warning whenever cap.approximate is set, in every mode, would close the gap without polluting the pipe. The cost is a note on every kitty capture — but that is the truthful state of affairs for this backend.

2. The password hint recommends a path that is usually empty on macOS (src/backend/kitty.rs:130-136)

listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid}

kitty expands environment variables in listen_on, and XDG_RUNTIME_DIR is a systemd convention that is normally unset on macOS — where this code already expects to be running, given the /Applications/kitty.app fallback in find_exe. An unset variable expands to nothing, leaving unix:/kitty-1234 at the filesystem root, which fails for a reason unrelated to anything the message explains. Since the point of this revision was to give advice that is both safe and correct, it's worth naming a per-platform target (macOS's $TMPDIR is already per-user and 0700; on Linux $XDG_RUNTIME_DIR is right) or telling the user to create the 0700 directory explicitly.

3. doctor reports healthy shell integration as broken in a fresh window (src/backend/kitty.rs:283-289)

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`)",
),

last_non_empty_output is empty when no command has produced output yet, not only when shell integration is off. tcap doctor as the first command in a new kitty window therefore reports a correctly-configured setup as failing — and doctor exists precisely so people trust its verdict. An empty result can't distinguish the two causes, so the wording should hedge: "no output to read yet — run a command that prints something, then re-run doctor; if it stays empty, check shell_integration."

Minor: the remote_control_hint keyword dispatch is a reasonable heuristic and correctly keeps the underlying error, but msg.contains("socket") with listen_on == None asserts flatly that "kitty is set to allow_remote_control socket-only". That's an inference from one word. Softening it to "this looks like socket-only" would match the hedging the unrecognised-error branch already does.

[github-actions]

🤖 Codex Code Review

No actionable findings.

The changes correctly:

• Use last_non_empty_output to avoid capturing the running tcap command’s empty output.
• Scope capture with a numeric KITTY_WINDOW_ID, preventing cross-window scrollback disclosure.
• Pass all environment-derived values as discrete argv elements, avoiding shell injection.
• Preserve tmux-first backend detection and leave tmux coordinate arithmetic and shell hooks unchanged.
• Clearly mark kitty captures as approximate and warn in piped/output modes.
• Avoid recommending weaker remote-control authentication when passwords are required.

Residual risk: kitty behavior is covered by argument-construction unit tests and live verification, but not an automated kitty integration test. I could not run the test suite because this review environment is read-only.

[github-actions]

🤖 Claude Code Review

I couldn't run cargo test or cargo check here (command approval was declined), so the review below is from reading the diff, not from a build.

Overall: the fix is sound and well-targeted. The three field traps (last_non_empty_output, --match after the subcommand, --to before it) are now locked down by argv tests, get_text_args being shared by fetch and diagnose closes the real risk of doctor blessing a path that fetch fails on, and require_window correctly treats a missing KITTY_WINDOW_ID as fatal rather than silently reading the focused window. The tmux coordinate arithmetic and the shell hooks are untouched by this PR, so the $?-first-line and single-argv invariants are unaffected. No new paths expose captured text: the diagnose probe discards the body and only surfaces run's stderr, and remote_control_hint echoes only the socket path.

The kitty.conf snippet in SOCKET_ADVICE is not usable as printed

src/backend/kitty.rs:99-103 emits:

  allow_remote_control socket-only
  listen_on unix:${TMPDIR}/kitty-{kitty_pid}      # macOS: $TMPDIR is per-user, 0700
  listen_on unix:${XDG_RUNTIME_DIR}/kitty-{kitty_pid}   # Linux

Two problems, and this text is the entire payoff of the error path — it's shown precisely to a user who is about to paste it into kitty.conf:

1. Trailing comments. kitty.conf takes the rest of the line as the option value; # only starts a comment at the beginning of a line. Pasted verbatim, listen_on becomes the literal string unix:${TMPDIR}/kitty-{kitty_pid} # macOS: $TMPDIR is per-user, 0700. The user then gets a different failure with no hint that the advice caused it. Worth confirming against the kitty version you verified on before restructuring, but if it holds, the comments need to move to their own # lines.
2. Two listen_on lines. Last one wins, so a user pasting the block gets the Linux line on macOS — where ${XDG_RUNTIME_DIR} is normally unset, putting the socket at the filesystem root. That is exactly the hazard the doc comment two lines above warns about, so the snippet is inviting the failure it documents. Better to print only the line for the platform tcap is running on (#[cfg(target_os = "macos")]), or to label them as alternatives on separate comment lines.

Minor: "no matching window" lands in the generic hint

window_match's doc comment correctly notes the guarantee is instance-local and a shared or stale $KITTY_LISTEN_ON can point at another instance. That's a realistic setup — a listen_on without {kitty_pid} means instance 2 inherits instance 1's socket, and --match id:N can then resolve to a same-numbered window in the wrong instance (silently wrong text) or to nothing. The "nothing" case produces a kitty error containing neither socket nor password, so remote_control_hint falls through to "kitty needs remote control enabled" — misleading advice for a problem the code already understands. A branch on matching/match pointing at a stale or shared $KITTY_LISTEN_ON would turn the most likely residual failure into a diagnosable one. The silently-wrong-window variant is a real residual gap that neither README nor the new # note: surfaces, though it's acknowledged in the code comment.

Smaller notes

• remote_control_hint checks socket before password. If kitty's password refusal ever mentions the socket it was reached over, the user gets "restart kitty" instead of the password explanation. Matching password first would be strictly safer — both keyword sets are guesses anyway.
• The doctor failure string hand-indents only its first two lines ("\n via {channel}\n {}"), but remote_control_hint itself is multi-line, so the rest renders unindented. Cosmetic.
• Refusing to send remote_control_password "" for get-text rather than weakening auth on the one call that dumps screen contents is the right call, and the reasoning belongs in the comment where you put it.

[github-actions]

🤖 Codex Code Review

• High — kitty can silently capture and pipe unrelated terminal contents (src/backend/kitty.rs, src/main.rs): last_non_empty_output fixes the empty result by selecting an older command whenever the intended command was silent. That older output may contain credentials or other sensitive data, and tcap --output | <external service> transmits it before the warning is printed to stderr. --quiet removes even that warning, while raw/output modes carry no in-band approximate marker. The known consecutive-tcap case is also detected only after output has already been emitted. This should fail closed when mismatch is known and require an explicit opt-in for inherently approximate kitty captures, rather than returning unrelated scrollback by default.

No other concrete issues found. The tmux coordinate arithmetic and shell hooks were unchanged. I could run git diff --check, but not the test suite because this review environment is read-only.

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref diff (b8c193e…f41257a). src/backend/tmux.rs and shell/ are untouched by this PR, so the coordinate arithmetic and the $?-first-line / single-argv invariants are unaffected. The security direction is right: --match id:N narrows get-text to the invoking window instead of the focused one (strictly less cross-window leakage than before), require_window fails closed rather than falling back, diagnose's new probe discards the captured text and only reports whether the extent resolved, and the password branch correctly refuses to advise remote_control_password "" get-text. Argv assembly is shared between fetch and diagnose, so the two can't drift.

Four things worth changing:

1. remote_control_hint: the socket keyword is tested before the unambiguous no-window phrases (src/backend/kitty.rs:151, :170)
msg is the whole of kitty's stderr, which frequently echoes the address it was given. A user whose config says listen_on unix:${XDG_RUNTIME_DIR}/kitty.socket and who hits a genuine stale-instance failure gets the substring hit on socket from their own path and is told "the socket at … was refused. Restart kitty" — swallowing the no matching/no window branch, which is the one that actually explains their problem (shared listen_on across instances). no matching and no window are multi-word phrases and effectively unambiguous; socket is one common word. Test the specific phrases first, and only then fall through to the socket inference.

2. fetch doesn't handle an empty result, while diagnose does (src/backend/kitty.rs:264, vs :311)
run succeeding with empty stdout returns Ok(Fetched) with an empty body. caps.is_empty() in main.rs:125 only checks the vector, so the user gets a bare $ cmd / # exit: 0 header and nothing else — which is the exact symptom this PR is titled for, and the one case where doctor has a genuinely useful message ("no output to read yet … check shell_integration"). Mirroring that message from fetch when the text trims to empty would close the loop.

3. socket_advice can emit a root-level socket path on Linux (src/backend/kitty.rs:103)
The doc comment rejects offering both platforms' lines precisely because an unset $XDG_RUNTIME_DIR "expands to nothing, putting the socket at the filesystem root" — but that hazard isn't macOS-specific. XDG_RUNTIME_DIR is a systemd convention and is routinely unset on non-systemd distros, in containers, and under su/some SSH paths, so a Linux user pasting this gets listen_on unix:/kitty-1234. Since this runs at capture time, it can check std::env::var("XDG_RUNTIME_DIR") and only recommend the path when it is actually set and non-empty (otherwise say explicitly that the socket needs a directory only the user can open). Note the abstract-namespace form unix:@kitty-… is not a substitute — it has no filesystem permissions at all.

4. The approximate warning fires in --command mode (src/main.rs:164)
In Mode::Command the rendered body is cap.command from the shell record, which is exact; kitty's output text is never printed. The warning about "the last non-empty output" there points at nothing the user is seeing. Gating the block on mode != Mode::Command keeps it where it's load-bearing (Annotated/Output/Raw/Json). The reasoning for keeping it in the piped modes is otherwise sound.

Two smaller notes, not blockers:

• has_command_boundaries() stays true for iTerm2, but async_get_last_prompt returns the prompt tcap itself was typed at, so the same "kitty counts the running tcap as the current command" hazard plausibly applies there — likely surfacing as command: "tcap" with empty output. Unverifiable here (the backend has never been run), but the new trait method is where that asymmetry now lives.
• If kitty @ get-text supports --self (or --match state:self) in your target versions, it would target the invoking window without depending on $KITTY_WINDOW_ID, removing both the hard failure in require_window and the documented cross-instance id-collision hole. I couldn't check kitty's docs from this environment, so treat that as a lead rather than a claim.

I was not able to run cargo test here (command approval denied), so the new kitty unit tests are reviewed by reading only.

[github-actions]

🤖 Codex Code Review

Findings

• High — repeated captures can still leak stale terminal contents. src/main.rs relies on is_tcap_invocation to refuse a capture after tcap, but src/state.rs only recognizes tcap as the first word after assignments. Common invocations such as command tcap, env tcap, an alias, or a shell function are missed. A subsequent tcap --output | some-service can therefore send the previous capture—including secrets—instead of refusing. The safety check needs a reliable invocation marker rather than heuristic parsing of command text, or at minimum must recognize standard wrappers with regression tests.

• Medium — the Linux fallback configuration is not paste-safe. src/backend/kitty.rs emits listen_on unix:/run/user/$(id -u)/kitty-{kitty_pid} <- .... Kitty configuration does not execute shell command substitution, and everything after the value becomes part of the socket path—as the surrounding comment itself notes. When XDG_RUNTIME_DIR is unset, following this advice leaves remote control unusable. Resolve the numeric UID in Rust and put the explanatory text on a separate line.

• Medium — --command now fails when kitty has no output. src/backend/kitty.rs rejects an empty get-text result before rendering mode is considered. Consequently, tcap --command fails after a silent command or in a new window even though it only needs the exact shell record and never displays terminal output. Command-only mode should bypass kitty capture, or empty output should only be an error for modes that consume it.

I could not run the test suite because this review environment is read-only. git diff --check passes.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref. src/backend/tmux.rs and shell/ are untouched by this PR, so the coordinate arithmetic and the $?/single-argv invariants are unaffected.

The core fix is sound: last_non_empty_output, --match after the subcommand, --match id:N parsed as a u64 so a poisoned $KITTY_WINDOW_ID can't widen the expression, and require_window() failing closed rather than silently reading the focused window. That last one is the right call for a tool whose output may contain secrets, and diagnose() correctly never echoes the probed text itself.

Three issues.

1. socket_advice()'s Linux fallback emits a kitty.conf line that will not work — and violates the rule documented directly above it. src/backend/kitty.rs:121-124

  listen_on unix:/run/user/$(id -u)/kitty-{kitty_pid}   <- or any directory only you can open; $XDG_RUNTIME_DIR is unset here

The function's own doc comment (lines 95-98) says kitty takes the rest of the line as the value and that this was verified live, producing a socket literally named /tmp/sock # note-69755. This branch does exactly that: everything from <- or any onwards becomes part of the socket path. Separately, kitty.conf does no shell command substitution, so $(id -u) stays literal — the parent directory won't exist and the socket won't be created. A user on a non-systemd distro or in a container (precisely the population this branch targets) pastes it and gets a silent remote-control failure, then re-runs tcap and is told to restart kitty.

Put the advisory prose on its own line, and use a form kitty will actually expand — ${TMPDIR} with the same "directory only you can open" caveat, or ${HOME}/.kitty-{kitty_pid}.

2. The no matching / no window hint asserts a cause that can't apply on the escape-code channel. src/backend/kitty.rs:158-165

It unconditionally blames $KITTY_LISTEN_ON pointing at a different instance, but that branch is reached with listen_on == None too, where the escape-code channel always reaches this instance and the theory is impossible. Match on listen_on here the way the socket branch below it already does, or hedge the text.

3. The "certain mismatch" refusal is narrower than its comment claims, and fires on a case it shouldn't. src/main.rs:132-153

The guard only inspects records.last(), so it catches tcap; tcap but not tcap; cd /tmp; tcap — a silent command in between and kitty still returns the previous capture's text, now under cd /tmp's header, in exactly the --output | some-service path the refusal exists to protect. The stderr warning and # note: still fire, so it degrades to the labelled path rather than a wrong-and-silent one, but the comment ("the one case where the output is known to be wrong") overstates the coverage.

In the other direction, is_tcap_invocation matches any segment, so tcap | sgpt "fix it" records as a tcap invocation — and the next tcap, which legitimately wants sgpt's output, hard-fails with "the previous command was tcap itself". That's a false positive turning a working capture into an error. Checking only the first segment of the record for the refusal (while keeping the broad match for nth_from_end's skip logic) would fix it.

Minor, non-blocking: the stderr warning at src/main.rs:182 says "the command named above", but in --output and --raw modes no command is printed.

I could not run cargo test in this environment (sandbox denied), so the README's updated test count and the new unit tests are unverified here.

[github-actions]

🤖 Codex Code Review

• High — Silent commands can leak unrelated terminal secrets. src/backend/kitty.rs always requests last_non_empty_output. If a secret-producing command is followed by a silent command, tcap --output | <external service> sends the older secret instead of empty output. The warning in src/main.rs is emitted only after stdout has already entered the pipe, so approximate metadata does not mitigate exfiltration. This needs a fail-closed mechanism that establishes the returned extent belongs to the recorded command; otherwise piped modes should refuse kitty captures where ownership cannot be established.

• Medium — Alias/function captures are still selected as “real commands.” src/state.rs filters with is_tcap_invocation(&r.command) instead of the newly introduced is_capture_record(r). Consequently, a capture invoked through an alias or shell function has was_capture = true but remains eligible for nth_from_end, pairing its metadata with unrelated output—especially visible in --command, which bypasses the consecutive-capture refusal. Use is_capture_record here and add an nth_from_end test containing a marked alias record.

• Medium — The fallback kitty configuration advice is invalid and paste-unsafe. src/backend/kitty.rs emits listen_on unix:/run/user/$(id -u)/... <- or any directory.... kitty configuration does not perform shell command substitution, and—as the surrounding comment notes—everything after the value becomes part of the socket path. On systems without XDG_RUNTIME_DIR, following this advice therefore creates an invalid/unusable configuration. Resolve the UID in Rust and place the explanation on a separate line.

The tmux backend and shell hooks are unchanged. git diff --check passes; I could not complete cargo test because the environment cannot reach index.crates.io.

[github-actions]

🤖 Claude Code Review

The kitty argv fixes (last_non_empty_output, --match id:N after the subcommand, numeric-only window id, mode-aware remote-control hints) are correct and well covered by the new unit tests. The approximate plumbing through Capture/render/--json is sound, and skip_serializing_if = "Not::not" keeps the JSON shape stable for existing consumers. Four issues:

1. nth_from_end ignores the new authoritative signal — silently wrong text on tmux (src/state.rs:206)

The PR adds was_capture precisely because text matching cannot see an alias or a shell function, and adds is_capture_record to combine the two — but the selection path still filters on is_tcap_invocation alone:

.filter(|r| !is_tcap_invocation(&r.command))

With alias t=tcap: run make (fails), then t, then t. The second t picks the first t's record (command text "t", unmatched by the heuristic) as "the last real command", and on tmux that record carries start_line/end_line pointing at the previous capture's region. tmux then returns the previous capture's text under a header naming t — wrong text, no error. This is the highest-severity path in the codebase and the fix is one word: !is_capture_record(r). The main.rs refusal doesn't cover it, because on tmux has_command_boundaries() is true so approximate is never set.

2. The $XDG_RUNTIME_DIR-unset advice is not paste-safe, contradicting its own doc comment (src/backend/kitty.rs:121)

None => "  listen_on unix:/run/user/$(id -u)/kitty-{kitty_pid}   <- or any \
     directory only you can open; $XDG_RUNTIME_DIR is unset here"

The doc comment eight lines above states the invariant — kitty.conf takes the rest of the line as the value, # only opens a comment at line start — and this branch breaks it in two ways. Pasted verbatim it yields a socket path containing the literal <- or any directory only you can open; $XDG_RUNTIME_DIR is unset here, and kitty.conf does no command substitution, so $(id -u) stays literal too (kitty expands ${ENV} and {kitty_pid}, not subshells). The Some(d) branch gets this right; this one needs the caveat moved to the prose paragraph below and a concrete path rather than $(id -u).

3. The capture marker is set even when nothing reached the screen (src/main.rs:163)

mark_capture() runs unconditionally after println!, including when stdout is redirected or piped. After tcap --output > notes.txt or tcap --output | pbcopy, the terminal shows nothing new, yet the next kitty capture hard-fails with "the previous command was tcap itself … refusing to capture" — a false refusal in exactly the piping workflows the README leads with. Gating on std::io::stdout().is_terminal() matches what the marker actually means: "tcap's own text is the last thing on this screen."

4. Docs describe the pre-refusal behaviour (README.md:163, npm/README.md:113)

Both say two tcap runs in a row return the first run's output with a # note: header. That case is now a hard error; the note/approximate labelling applies only to the possible mismatch (last command printed nothing).

No issues found in the tmux coordinate arithmetic (untouched) or the shell hooks (untouched), and nothing in the new code paths writes captured text anywhere new — the marker file contains only "1".

[github-actions]

🤖 Codex Code Review

• High – predictable marker file permits arbitrary file truncation via symlink. src/state.rs writes to a predictable path under /tmp/tcap-<uid> using fs::write, which follows symlinks and truncates the target. Because state_dir() accepts a pre-existing directory without verifying its owner or permissions, another local user can pre-create that directory and place <session>.capturing as a symlink to any victim-writable file. Running tcap on a terminal then truncates that file. Create the state directory securely with mode 0700, verify ownership/type, and create/update marker files without following symlinks.

• Medium – redirected/piped captures still print to the terminal but are not marked. src/main.rs marks a capture only when stdout is a TTY, but emit_hints() subsequently writes the approximate-capture warning to stderr. Thus advertised workflows such as tcap --output | some-service still leave non-empty tcap output on kitty’s screen, while the marker remains absent. The next capture can silently return that warning as though it belonged to the next command. Mark whenever either output stream writes visible tcap content, or avoid emitting the warning to a terminal in redirected modes.

• Medium – the single marker can be assigned to the wrong command for background captures. src/state.rs uses one boolean file that is consumed by the next __record. With tcap ... &, the shell may record the background launch before tcap reaches mark_capture(); the marker then remains and is consumed by the following unrelated command. That record is subsequently skipped as a capture and may trigger false refusals. The marker needs invocation identity/sequence information, or should be established before the shell can record completion.

The tmux coordinate arithmetic and shell-hook argument/status handling were unchanged and remain consistent with the documented invariants. Tests could not run because dependencies were unavailable locally and network access to crates.io failed.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref (kitty argv/extent fix, has_command_boundaries, the capture marker, and the refusal/labelling path). src/backend/tmux.rs and shell/ are untouched, so the coordinate arithmetic and the $?/single-argv invariants are unaffected. The kitty argv work looks right: --match is numeric-validated and follows the subcommand, require_window fails closed rather than reading the focused window, and remote_control_hint never advises loosening allow_remote_control for a socket-only user. Three real issues below, the first of which reopens the bug this PR exists to close.

The capture marker is only set on the successful-capture path, so tcap's own text still gets returned as a command's output

state::mark_capture() is called only at src/main.rs:168, after a capture prints. Every other way tcap writes to the terminal leaves screen text behind with no marker:

• the refusal itself (src/main.rs:150) — printed via eprintln! in main (src/main.rs:24), which goes to the tty
• tcap doctor (src/main.rs:36), which prints a screenful and is exactly what the refusal and the kitty error messages tell users to run next
• any other error path (kitty returned no output, nothing captured)

On kitty, last_non_empty_output counts all of those. Concretely:

make            # prints output
tcap            # capture displayed, marker set
tcap            # refused (correct) — but the refusal text is now on screen, no marker
tcap            # records.last().was_capture == false → no refusal
                # → returns the refusal message, under make's header

Step 4 is the wrong-text-under-a-confident-header failure the PR is fixing. tcap doctor; tcap produces the same thing with doctor's output. Note after_tcap reads r.was_capture only (deliberately, per the comment at src/main.rs:134), so the text heuristic doesn't rescue it.

The marker's real meaning is "tcap put text on this screen", not "a capture succeeded". Setting it from one place at exit whenever tcap wrote to a terminal — covering stderr and doctor — closes all of these at once.

was_capture conflates "was a capture" with "was displayed", which defeats the alias case it was added for

Record::was_capture is documented (src/state.rs:37-41) as "Set when tcap performed a capture during this command … holds for an alias, a shell function or any other spelling that text matching cannot see", and is_capture_record/nth_from_end rely on that for indexing. But the marker is written only when stdout is a terminal, so with alias t='tcap --output':

t | llm        # no marker (stdout is a pipe); is_tcap_invocation("t") is false
tcap -c 1      # the aliased capture is indexed as a real command

Under tmux that record has real start_line/end_line, so -c 1 returns the previous capture's own region verbatim. nth_skips_captures_made_through_an_alias (src/state.rs) only passes because it sets was_capture by hand; nothing exercises the path that sets it.

These are two independent bits — "this command was a tcap run" (always true, needed for indexing) and "tcap's text is what's on screen" (needed for after_tcap). Recording both, e.g. by writing the marker unconditionally with the terminal-ness as its content, would fix this and the issue above together.

Related, smaller: the marker has no expiry or owner. If it's ever written without a following __record (tcap invoked from a nested shell or a prompt hook in the same TMUX_PANE/KITTY_WINDOW_ID), the next unrelated command inherits was_capture=true, becomes invisible to -c indexing, and triggers "the previous command was tcap itself" for a command that wasn't.

State directory is world-readable on Linux, and now holds one more file

state_dir() is created with a plain create_dir_all (src/state.rs:116, and now src/state.rs:56), so under the usual umask /tmp/tcap-<uid>/ is 0755 and the per-session .jsonl — full command text as typed, which routinely includes -p<password>, --token=…, AWS_SECRET_…= assignments — is readable by any local user. macOS is fine since TMPDIR is already per-user 0700; Linux /tmp is not.

This predates the PR, but it's worth fixing here given the PR adds a file to that directory and its own socket_advice now tells users to "keep the socket in a directory only you can open." A 0700 mode on the directory (and 0600 on the log) would make tcap follow its own advice. The same permissive-mode concern applies to compact_if_needed's temp file.

Nits

• src/backend/kitty.rs diagnose: the comment says an empty window "must not call a healthy setup broken", then returns Diagnostic::bad, which renders as [FAIL] for a brand-new window. The message text is careful, but the verdict still contradicts the comment.
• README.md:208 says "56 unit + 11 end-to-end tests"; the tree has 59 #[test] under src/ and 11 under tests/.

[github-actions]

🤖 Codex Code Review

• High — State reads bypass the new ownership and symlink checks. src/state.rs:192 reads the predictable session path directly, without calling ensure_state_dir() or opening with O_NOFOLLOW. On systems using /tmp, another local user can pre-create /tmp/tcap-<uid> and a crafted JSONL file or symlink before the victim’s first run. This permits record injection—including terminal escape sequences in displayed command metadata—and can force incorrect capture selection/refusal. Validate the directory before reads and open the log with no-follow semantics.

• Medium — The directory check mutates a symlink target before rejecting it. src/state.rs:89 calls set_permissions before symlink_metadata. If another user pre-creates /tmp/tcap-<uid> as a symlink, set_permissions follows it, allowing the attacker to make the victim chmod an attacker-selected file or directory owned by the victim. Inspect/create the directory without following symlinks before changing permissions.

• Medium — Merely having a terminal-backed stderr falsely marks redirected captures as displayed. src/main.rs:38 uses descriptor type rather than whether anything was written. For example, tcap --quiet --output > notes.txt has terminal stderr but emits no terminal output, yet its record gets was_capture = true; the following kitty capture is then incorrectly refused at src/main.rs:171. Track actual stdout/stderr emission or determine marking from the selected mode, redirections, and error/warning paths.

The kitty argument ordering, numeric window matching, and last_non_empty_output change otherwise look correct. Shell hooks and tmux coordinate arithmetic were unchanged. Tests could not run because the environment cannot reach crates.io and has no cached dependency index.

[github-actions]

🤖 Claude Code Review

The kitty argv fixes (--match placement, last_non_empty_output, numeric window-id validation) and the approximate labelling are well-targeted, and the error-hint tests are good. Three issues in the new capture-marker mechanism, plus one in the state-dir hardening.

---

tcap init marks itself as a capture, poisoning the first real command of every shell — src/main.rs:38

The marker is set for any subcommand whose stdout or stderr is a tty. The installed usage is eval "$(tcap init zsh)": stdout is a command-substitution pipe, but stderr is still the terminal, so wrote_to_terminal() is true and mark_capture() runs at shell startup. The first precmd is a no-op (_TCAP_CMD empty), so the marker survives until the user's first actual command, which __record then stores with was_capture: true.

Consequences, on a fresh shell:

• nth_from_end filters that command out entirely, so tcap (tmux included) reports the command before it, or "nothing captured".
• On kitty, after_tcap is true at src/main.rs:163 and the capture is hard-refused with "the previous command was tcap itself" — naming a command that was ls.

This is invisible to the e2e suite only because tests/tmux_e2e.rs:194 redirects both streams: eval "$(tcap init {sh})" >> {log} 2>&1. Real installs don't.

Restrict the mark to the capture path (capture_and_print, after it has actually printed), or at minimum exclude init/doctor-style subcommands the way __record is excluded.

---

wrote_to_terminal() tests fd type, not whether anything was written — src/main.rs:47

The comment at src/main.rs:158-162 states that tcap --output > notes.txt prints nothing to the terminal and so must not block the next capture. But with stdout redirected, stderr is still a tty, so the marker is set and the next kitty capture is refused — the code does the opposite of what it documents. Same for tcap --output | svc with --quiet, where no hint is emitted either. You need to track whether a write actually happened (e.g. set the marker where output is emitted, gated on stdout().is_terminal(), plus in the eprintln! paths) rather than inferring it from isatty.

---

clap's own output bypasses the marker entirely — src/main.rs:52

Cli::parse() calls e.exit() on --help, --version and usage errors, so main()'s marking code never runs. tcap --help then tcap in kitty: is_tcap_invocation("tcap --help") skips the record for indexing, records.last().was_capture is false so no refusal fires, and kitty happily returns the help text as the body under an unrelated command's header. That's precisely the silent-wrong-text case the PR is closing. Use Cli::try_parse() and route the error through the same exit path, or mark before dispatch.

---

ensure_state_dir chmods before the symlink check — src/state.rs:90-95

fs::create_dir_all succeeds on a pre-existing symlink-to-directory, and fs::set_permissions is chmod(2), which follows symlinks. So the 0700 is applied to the link target before symlink_metadata gets a chance to reject it. The impact is bounded (chmod only succeeds on directories we already own, so this is a nuisance/DoS — an attacker-planted /tmp/tcap-<uid> → ~/shared-project gets stripped to 0700 — not a disclosure), but the fix is free: do the symlink_metadata / is_dir / uid check first and only chmod once the path is known not to be a link. Ideally open the directory with O_DIRECTORY | O_NOFOLLOW and fchmod the fd to close the residual TOCTOU.

---

No issues found in the tmux coordinate arithmetic (untouched here), the shell hooks ($? still first line of precmd, command text still a single argv element), or in what the new error/hint strings expose — none of them echo captured text, and socket_advice uses the literal ${XDG_RUNTIME_DIR} rather than interpolating its value.

[github-actions]

🤖 Codex Code Review

No actionable findings.

The kitty fix correctly:

• Uses last_non_empty_output.
• Places --match id:$KITTY_WINDOW_ID after get-text.
• Validates the window ID as numeric, preventing match-expression injection.
• Preserves tmux-first backend detection and leaves its absolute-coordinate arithmetic unchanged.
• Avoids recommending weaker remote-control settings for socket-only.
• Labels kitty captures as approximate and refuses the known consecutive-capture mismatch.
• Hardens state files against symlink attacks and restricts the state directory to 0700.

Shell hooks are unchanged, so the $? ordering and single-argument command-text behavior are unaffected.

Verification limitation: cargo fmt --check passed, but cargo test --all-targets could not run because the environment could not resolve index.crates.io, and the dependencies were not cached for offline use.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref against b8c193e, read the full changed files (not just the hunks), and checked the shell hooks and the unchanged tmux coordinate code for interactions. src/backend/tmux.rs is untouched and the hooks still read $?/$status on the first line and pass --command "$_TCAP_CMD" as one argv element, so the two invariants in the review brief are intact.

Findings

1. nth_from_end skips tcap pipelines, but kitty's body comes from them — header names the wrong command, and neither the refusal nor the note fires. (src/main.rs:174-192, src/state.rs:265-273)

The flagship workflow is tcap --output | llm. There, stdout is a pipe, so mark_if_stdout_is_terminal() does not fire and was_capture stays false — but the downstream tool writes to the terminal, so kitty's last_non_empty_output is that pipeline's output. On the next tcap:

• after_tcap is false → no refusal.
• nth_from_end still filters that record out, because is_capture_record falls back to the text heuristic and "tcap --output | llm" matches.
• So the header names the command before the pipeline, while the body is the LLM's output.

The # note: line doesn't describe this case either — it says "if the command named above printed nothing this is an earlier command's output", but here the named command did print, and the body belongs to a later command. The two signals are intentionally split (the comment at src/main.rs:170-176 argues for it), but the split leaves this gap: the marker governs the refusal while the text heuristic governs indexing, and a piped tcap lands in between. Making the indexing filter and the mismatch check agree — or widening the note to "may not belong to the command named above" — would close it.

2. The marker poisons any command that merely calls tcap, and the refusal message then misreports what ran. (src/state.rs:56-63, src/main.rs:181-192)

mark_capture() is written whenever tcap's stdout is a terminal, and the next __record attributes it to the whole command line. A script or wrapper that shells out to tcap internally — ./deploy.sh, a Makefile target — gets was_capture: true on its own record. Two consequences: nth_from_end skips ./deploy.sh entirely, so -c 1 silently returns the command before it; and the next capture on kitty is refused with "the previous command was tcap itself", which is false and gives the user nothing to act on. The doc comment frames the marker as catching aliases and shell functions, which it does, but it cannot distinguish "was tcap" from "called tcap", and the error text asserts the former.

3. iTerm2 inherits has_command_boundaries() == true while having the same hazard kitty was just fixed for. (src/backend/mod.rs:72-77, python/iterm2_capture.py:65)

The shim calls async_get_last_prompt, which — by the same reasoning that made last_cmd_output return tcap's own empty output — resolves to the prompt tcap is currently running under. If that holds, iTerm2 returns an empty body and renders a bare header: the exact symptom this PR's title describes, now unqualified because the default is true. iTerm2 is documented as never having been run, so this is a note rather than a blocker, but the new trait method makes the unverified claim explicit where it was previously implicit.

Looks good

• The state-file hardening is correct in the details that usually go wrong: symlink_metadata before set_permissions (so a planted symlink can't redirect the chmod), uid check, DirBuilder::mode(0o700) at creation rather than a chmod afterwards, and O_NOFOLLOW on both reads and writes. secure_read failing closed to an empty record set is the right default.
• parse_window_id rejecting non-numeric $KITTY_WINDOW_ID rather than interpolating it into --match closes a real cross-window read; require_window() treating None as fatal is the right call given the failure mode is another pane's scrollback.
• run() only surfaces stderr on failure, so the kitty hint paths can't leak captured text into an error message.
• No captured terminal content is persisted anywhere new — the marker file holds a single 1.
• --match after the subcommand and the last_non_empty_output extent are both locked in by get_text_argv_order_and_extent, which is the right thing to pin given both were field bugs.

Nit

README.md:208 says "56 unit + 11 end-to-end tests"; #[test] count across src/ is 59.

[github-actions]

🤖 Codex Code Review

• Medium — capture-loop safeguard fails open when marker persistence fails. src/state.rs:56 discards every error from secure_open/write_all, but src/main.rs:178 relies on that marker to prevent kitty from returning a previous tcap capture. If the filesystem is full/read-only or the marker path is unusable, a successful capture is displayed without recording the marker; the next tcap --output | <service> can therefore send that prior capture—potentially containing secrets—under the wrong command. Make successful terminal-bound captures fail before printing when the marker cannot be persisted, or otherwise propagate the failure so the documented “hard error” remains fail-closed.

No other concrete issues found. The kitty extent/window argument construction is correct, tmux still wins detection, tmux coordinate conversion remains absolute-to-relative, and the shell-hook invariants are unchanged.

Tests could not run because dependencies are unavailable locally and network access to crates.io is restricted.

[github-actions]

🤖 Claude Code Review

The core of the change — the kitty argv fix, the --match id: refusal, and the state-file hardening — looks solid. The tmux coordinate arithmetic and the shell hooks are untouched by this PR, so the history_size + cursor_y invariant and the $?-first / single-argv-element rules are unaffected.

Three things worth addressing.

1. was_capture now hides real commands from tmux indexing — src/state.rs:293

nth_from_end changed from !is_tcap_invocation(&r.command) to !is_capture_record(r), and is_capture_record ORs in was_capture. But was_capture doesn't mean "this record is a tcap invocation" — it means "a tcap process wrote to this terminal at some point during this command" (src/main.rs:38-48 marks on any tty write; src/main.rs:131 collects it for whatever command was running).

So any ordinary command that shells out to tcap and lets it print — a wrapper script, a Makefile target, a git alias, a TUI that runs tcap for a subprocess — gets was_capture: true and is then skipped entirely by nth_from_end. Under tmux, where boundaries are exact and this whole mechanism is unnecessary, tcap -c 1 then returns the command before it: header and body are mutually consistent, so there is no error and nothing looks wrong — it is just the wrong command. That is the silent-wrong-text failure mode this codebase is otherwise careful about, and it's a regression from the previous text-only filter.

The refusal check at src/main.rs:178 already uses the marker alone and deliberately avoids is_capture_record, with a good comment explaining why the two questions differ. The same reasoning applies here: nth_from_end is asking "was this a tcap run", which is the text question. Suggest either keeping nth_from_end on is_tcap_invocation and reserving was_capture for the boundary-less-backend guard, or gating the was_capture arm on !backend.has_command_boundaries().

2. The # note: line is dropped in exactly the configuration most likely to need it — src/render.rs:149

render_one returns bare cap.output when !has_metadata(), before header() is ever called. On kitty without shell integration — no record, so command/exit_code/cwd/duration_ms are all None, and Fetched from kitty supplies none of them — approximate is true but the note never renders. after_tcap is also false (no records at all), so the hard refusal can't fire either. tcap > notes.txt or tcap 2>/dev/null | llm then ships possibly-mismatched output with no in-band marker at all; only the stderr hint carries it, and that's the stream most likely to be discarded. Emitting the note even when there's no other metadata would close this.

3. secure_read silently disables the guard it protects — src/state.rs:133-143

secure_read returns None for every failure mode, including ensure_state_dir() rejecting the directory on ownership. load() (src/state.rs:250) turns that into an empty Vec, which makes after_tcap false at src/main.rs:178 — so the "refusing to capture" safety net quietly switches off in precisely the scenario the hardening was written for. The --output/--raw/--json paths print no hint about missing metadata either, so there's no user-visible signal. Worth distinguishing "no log yet" from "refused to read the log" and surfacing the latter.

Notes, not blockers

• secure_write truncates before writing, so a concurrent tcap could hand python3 a partially-written iterm2_capture.py (src/backend/iterm2.rs:64). The staleness check keeps the window to first-run/post-upgrade, but a write-to-temp-plus-rename would remove it entirely.
• Pre-existing log files created by 0.1.0 keep their 0644 mode — append only applies .mode() on creation. The directory chmod in ensure_state_dir makes them unreachable, so this is covered, but it's worth knowing the files themselves are never retightened.
• No new path writes captured terminal text to disk, and the kitty diagnose probe discards the text it fetches rather than surfacing it. Good.

[github-actions]

🤖 Codex Code Review

Found two issues:

• [P1] Piped terminal output bypasses the repeated-capture guard — src/main.rs:224. The marker is set only when tcap’s own stdout/stderr is a TTY. With tcap --quiet | cat, stdout is a pipe and stderr is silent, but cat still writes the captured contents into kitty. The resulting record has was_capture = false, so the next tcap --output | some-service is allowed and last_non_empty_output returns the previous capture, potentially sending terminal secrets to an unintended destination. The marker cannot reliably determine whether downstream pipeline output reached the terminal; either conservatively mark successful captures even when redirected or avoid claiming this case is safely detected.

• [P2] Whitespace-only output is incorrectly treated as no output — src/backend/kitty.rs:311. text.trim().is_empty() rejects legitimate output such as printf ' \n', including in --raw/--output modes where exact bytes are expected. Check text.is_empty() instead; if kitty returns only a trailing newline for an actually empty extent, normalize that explicitly without discarding other whitespace.

The PR does not change src/backend/tmux.rs or the shell hooks, so the absolute-coordinate arithmetic and hook quoting/status invariants are unaffected.

I could not run the test suite because the environment has no cached dependencies and network access to crates.io is unavailable. git diff --check passed.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref (b8c193e...a14ce87) statically — cargo check/cargo test require approval in this sandbox, so I did not build.

Overall this is careful work. The three kitty fixes (last_non_empty_output, --match after the subcommand, mode-aware remote-control advice) are correct against kitty's CLI, and require_window() treating a missing $KITTY_WINDOW_ID as fatal closes a real cross-window leak rather than papering over it. The state-directory hardening is sound: symlink_metadata before set_permissions (which follows symlinks), DirBuilder::mode(0o700) instead of create-then-chmod, and O_NOFOLLOW on every open. The create_dir_all + AlreadyExists swallow is safe because the non-directory and symlink-to-directory cases both fall through to the symlink_metadata check. tmux coordinate arithmetic is untouched by this PR.

Two things worth changing.

1. emit_hints writing to stderr marks the session, causing a false hard refusal after the flagship pipe workflow — src/main.rs:227

On kitty with shell integration installed, run tcap --output | llm "fix this":

• stdout is a pipe, so mark_if_stdout_is_terminal() correctly does nothing.
• emit_hints prints the "last non-empty output" warning to stderr, which is a tty, so mark_if_stderr_is_terminal() fires and the record gets was_capture = true.
• The very next tcap hits src/main.rs:210 and hard-errors with "refusing to capture: the previous command left tcap's own output on screen" — but tcap's captured text never reached the screen. The last output block is the llm's answer, which is exactly what the user wants.

The behaviour also flips on an unrelated flag: tcap --output --quiet | llm prints no hint, so no mark, so the next capture succeeds. Same input, opposite outcome depending on --quiet.

The marker's stated meaning is "is tcap's own captured text the last thing on screen" (the comment at src/main.rs:195), but a one-line diagnostic on stderr is not that. Suggest marking only where capture content is written — the println! path and the doctor/init/--help paths — and leaving hints and error messages unmarked. mark_if_stderr_is_terminal() in main()'s error path (src/main.rs:25) is the same class: a failed tcap -c 2 on kitty now poisons the following tcap.

2. The new refusal logic has no test — src/state.rs:319-401, src/main.rs:202-219

mark_capture / take_capture_marker round-tripping through marker_path(), and the after_tcap && approximate gate, are the riskiest new code here: they decide whether a capture is refused outright. The two added tests (the_two_capture_signals_are_independent, nth_keeps_commands_that_merely_called_tcap) only assert that the two signals differ on hand-constructed records; nothing exercises the marker file or the gate. Given the interaction in (1), a test over (marker present?, stdout tty?, mode) → refuse/label/proceed would have caught it.

Minor, non-blocking:

• src/backend/iterm2.rs:118 — has_command_boundaries() returns true on a backend the module header says has never been run, and the doc comment itself concedes it may be wrong. The project's stated principle elsewhere is to label or fail rather than assert exactness; false would be the safer default until tcap doctor settles it, at the cost of a note line iTerm2 users may not need.
• src/main.rs:144 — take_capture_marker() deletes the marker inside the Record literal, so if state::append then fails the flag is consumed with no record written. Low impact, since a failing append usually means load() fails too and the guard is already off.
• src/state.rs:184-198 — session_key() falls back to ppid-<pid>, and PIDs are reused. A recycled ppid can now inherit a stale .capturing marker, not just a stale log. Pre-existing, but the blast radius grew from "wrong metadata" to "spurious refusal".

[github-actions]

🤖 Codex Code Review

No actionable findings.

The kitty fix looks sound:

• Uses last_non_empty_output, avoiding capture of the running tcap command’s empty output.
• Places --match id:$KITTY_WINDOW_ID after get-text and validates the ID as numeric, preventing accidental cross-window disclosure.
• Preserves tmux-first detection and does not alter tmux’s absolute-to-relative coordinate arithmetic.
• Keeps shell hooks unchanged, including first-line exit-status capture and single-argument command forwarding.
• Handles kitty’s unavoidable silent-command ambiguity explicitly through approximate labels and refusal after displayed tcap output.
• Hardens state and iTerm2 shim files with private permissions, ownership checks, and O_NOFOLLOW.

I could not execute the test suite because dependencies are not cached and network access to crates.io is unavailable. Static inspection found no regression in the reviewed paths. The remaining residual risk is the already-documented lack of automated/live kitty and iTerm2 coverage; iTerm2 remains unverified.

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref (4afd099) against b8c193e. I couldn't build — cargo test/clippy need approval in this sandbox — so this is a static review.

src/backend/tmux.rs and shell/ are untouched, so the scrollback coordinate arithmetic and the $?/single-argv invariants are unaffected by this PR.

The state-directory hardening (0700 at creation, symlink_metadata + uid check before any mutation, O_NOFOLLOW on every open, chmod only after the dir is proven ours) is well-reasoned, and the ordering comment about set_permissions following symlinks is correct. The kitty fixes — last_non_empty_output, --match after the subcommand, refusing rather than defaulting to the focused window — match what AGENTS.md documents.

Findings below.

---

iTerm2 still falls back to the focused session — the leak the kitty change just closed

src/backend/iterm2.rs:171 advertises this as expected behaviour ("$ITERM_SESSION_ID unset — tcap will fall back to the focused session"), and python/iterm2_capture.py:55-60 implements it:

if session is None:
    # Fall back to whatever is focused, e.g. when ITERM_SESSION_ID is unset
    window = app.current_terminal_window

This is exactly the hazard Kitty::require_window now refuses, with the same consequence its doc comment spells out: "the fallback would put another pane's scrollback, secrets included, under this command's header." Since iterm2.rs is in this PR anyway, the fallback should become an error with the same shape as require_window's, and the diagnostic should say "will refuse" rather than "will fall back."

ITerm2::has_command_boundaries() -> true picks the unsafe side of an unverified assumption

src/backend/iterm2.rs:113-120 documents the risk honestly, then returns the value that produces no warning if the guess is wrong. Capture::approximate defaults false, so a mismatched iTerm2 capture gets an authoritative header, no # note: line, no stderr hint, and no refusal after a prior capture — the precise failure mode this PR exists to fix, on the one backend nobody has ever run. The project's stated principle elsewhere (WezTerm: "a guess would return wrong text rather than fail") argues for false until tcap doctor under iTerm2 settles it; over-labelling costs a note line, under-labelling costs silence.

The repeat-capture guard is one-shot: the third consecutive tcap returns tcap's own refusal text

On kitty with hooks installed:

1. tcap prints to a tty → marker written → hook records was_capture: true.
2. tcap → src/main.rs:206 refuses. The error goes to stderr and is now on screen. record() at src/main.rs:140 already consumed the marker in step 1, and error paths are deliberately unmarked, so this record gets was_capture: false.
3. tcap → after_tcap is false → kitty's last non-empty output is the refusal message, returned as the capture (labelled approximate, but returned).

A user who is told "re-run the command you want to capture, then tcap" and instead just retries lands on step 3. The guard prevents one repeat, not the repeat that follows it. The deliberate "don't mark errors" rule (src/main.rs:10-11) was about not letting a failed tcap -c 2 poison the next capture; this specific refusal is different in that tcap's text genuinely is the last thing on screen. Either mark on this one error path, or fall back to is_tcap_invocation(&last.command) when was_capture is false — the trade-off against tcap --output > notes.txt is worth stating explicitly either way.

fetch and diagnose emptiness checks have drifted apart

get_text_args exists so the two paths cannot disagree, but the emptiness predicates were not shared:

• src/backend/kitty.rs:311 — text.chars().all(|c| c == '\n' || c == '\r')
• src/backend/kitty.rs:389 — Ok(t) if t.trim().is_empty()

After printf ' \n', fetch succeeds (correctly — commit feb0173 made whitespace real output) while doctor reports shell integration broken and tells the user to check kitty.conf. It's the harmless direction, but it's the same drift the shared-argv helper was built to prevent. Extract the predicate.

Minor

• emit_hints was given a -> bool and a printed accumulator (src/main.rs:240-295), but the sole call site at src/main.rs:227 discards it. Dead return value; also true is returned unconditionally at the end rather than printed.
• capture_marker_round_trips_and_is_consumed_once (src/state.rs) calls take_capture_marker() against the real state dir. session_key() prefers TMUX_PANE/KITTY_WINDOW_ID from the environment, so running cargo test inside kitty or tmux deletes the live session's marker and silently disarms the guard for the next capture. Pointing TMPDIR at a temp dir for the test would isolate it.
• src/state.rs:107-108: the AlreadyExists arm is unreachable — DirBuilder::recursive(true) returns Ok for an existing directory. Harmless, just noting it isn't doing what it looks like it's doing.