`CONTROL_VERSION`'s doc states the rule — move it whenever a variant is
added to or removed from `ControlRequest`, `ReplyOk` or `ControlEvent` —
and states what happens when it is missed: the frame fails to decode, the
read loop that failed on it takes the whole link down, and a remote
workspace opens with no tabs. The same doc records that this is not
hypothetical. The v5→v6 drift was most of the dialect, and all of it
shipped against a number that never moved.
Only one of the three enums was actually protected, and by accident:
`ControlRequest::deadline` matches exhaustively, so a new request cannot
be added without the compiler asking about it. `ReplyOk` and
`ControlEvent` had nothing. Checked rather than assumed — a probe variant
added to both compiled clean across the whole workspace, all targets, no
warnings.
The hand-written `every_reply` / `every_event` lists cannot catch it
either. They are cross-checked against other hand-written lists, so a
variant missing from the enum coverage *and* the list leaves every count
in agreement.
So: one exhaustive match per enum, in the test module, called from a test
so it cannot rot into dead code. Confirmed it fails for the case it
exists for — a probe variant in either enum is now an E0004 naming the
variant, at compile time, where it cannot be skipped.
`Config` is `#[serde(default)]`, which is what makes this worth asserting.
A key whose serialized and deserialized names have drifted apart does not
fail to load — it silently resolves to the default, so the setting is
forgotten the next time the file is read, with nothing on any error path
to say so. Nothing was checking that the names still agree.
The test nudges every bool and number off its default, writes the config,
reads it back, and requires the value to return rather than the default.
Strings are skipped on purpose: most are enums in kebab-case, and
`de_lenient` deliberately resolves a spelling it does not know to the
default, so an arbitrary string would report that intended behaviour as a
failure.
Confirmed to fail for the case it exists for, rather than assumed: an
injected `rename(serialize = "cursor_blink", deserialize = "cursor_blink_x")`
trips it. A *symmetric* rename does not, and should not — that changes
the file format consistently and loses nothing.
Floats compare loosely because several fields are `f32` and a value
nudged through `f64` returns a fraction off; an exact compare reported
arithmetic as a lost setting on the first run. `checked > 20` guards the
loop against silently testing nothing.
No key fails today.
Measuring first changed what was worth doing. Of the fixtures left after
the previous commit, six prefixes turned out to create no directory at
all, while two accounted for 6,676 of them: `completion` (14 fixtures,
4,219 directories) and `file_copy` (15, 2,457). Both build every fixture
through one helper, so both are one edit.
A `cargo test` run of the bin crate now adds 3 directories where it added
37 before this pair of commits, and both converted prefixes hold at
exactly the count they started at.
The three that remain are deliberate. `pin_test_config_dir` and the
`set_config_dir` fixtures in `session`, `tree_sync`, `windows` and
`main` all point *process-global* state at their directory: no scope owns
it, tests in the same binary run in parallel against it, and a guard that
freed it at the end of one test would pull it out from under another.
`file_copy` compared paths it got back from the system against its
fixture, so it canonicalized — macOS answers `/private/var` for a temp
dir under `/var`. `TempRoot::canonicalized` resolves in place so the
removal stays attached, rather than handing back a bare path again.
The completion fixtures said `dir.as_path()`, which was inherent on
`PathBuf` and is not on the guard; they take a `&Path` by deref now.
`env::temp_dir().join(format!("tty7-x-{pid}"))`, wiped on the way in and
left behind on the way out, reads as self-cleaning and is not. The pid is
in the name so two concurrent `cargo test` runs cannot share a fixture —
which also means a run never finds the previous run's directory to wipe.
One directory per fixture per run, kept forever: this working copy had
25,639 of them, and `cargo test` added about 37 more each time.
Two modules account for 10,904 of those, and both build every fixture
through one helper, so both are one edit: `ssh_config` (18 fixtures) and
`cli_install` (19). They now go through `testutil::temp_root`, which
hands back a guard that removes the tree when it drops — the pattern
`core::git::log` already uses, rather than a fourth copy of it.
Measured rather than assumed: a full bin-test run leaves both prefixes at
exactly the count they started at (5217 and 5687), where they used to
grow every run. The remaining 29 per run are the fixtures in modules not
converted here.
The guard is deliberately not `Clone` — copying something that deletes a
directory on drop is not a copy — so three sites that wanted a plain path
say `to_path_buf()`. Eight more chained `tmpdir(..).join(..)`, where the
guard is a temporary that drops at the end of the statement and takes the
directory with it before the test looks at it; the test suite caught that
as a failure and they now bind the guard first.
`pin_test_config_dir` keeps its uncleaned directory on purpose: it sets a
process-global config dir, so no scope owns it. `view.rs` was spelling
that function out by hand and now calls it.
#550 settled the rule: one range, defined where the value is validated,
so a config-legal number cannot be turned around by a widget's narrower
clamp. `sanitize_clamps_to_the_same_bounds_the_gui_steps_within` states
it and pins the font pair. The two side panels broke it.
`sanitize` clamped both widths to 100–2000, and
`docs/reference/configuration.mdx` published that range, while the
sidebar floored itself at 180 and the right panel at 216. So a
documented `sidebar_width: 120` was accepted by sanitize, kept in the
file, and drawn at 180 — the file said one thing and the window showed
another, with nothing to explain the difference.
The floors move to `core::config` beside the font bounds, and the two
widget constants are defined from them, which is the direction that
cannot drift: the widget cannot be narrowed without moving the floor
the file is validated against. Docs updated to the real numbers.
The *ceiling* is deliberately not shared. Both panels also cap against
the viewport, but a panel wider than its window is a different question
from a panel wider than the setting allows, and only the second belongs
in `sanitize`.
`guard_off_ui` is the debug assertion that catches a blocking `Host` call
made on the UI thread, which is a frozen window. `LocalHost` arms it on
each of its fifteen blocking methods. `RemoteHost` armed it nowhere, so
the protection ran backwards: a `read_dir` left on the UI thread tripped
the assertion when it hit a local disk, and passed silently when it went
to a machine across a network, where the freeze is a round trip instead
of a syscall.
Guarding the round trip rather than the methods. `RemoteHost::call` is
the choke point for fifteen of the seventeen; `read_file` and
`git_with_deadline` reach `ControlClient` directly and arm it themselves.
`is_connected` is deliberately left alone — it reads a flag the client
already holds and rendering asks it constantly, so guarding it would fire
every frame.
Checked before adding an assertion that can panic a dev build: every
blocking call from the UI goes through a `HostOps::run*` closure, which
registers the UI thread and then dispatches through `off_thread`. The one
path local never exercises — `file_copy::copy_file`, which returns early
via `std::fs::copy` for a local host — reaches the wire from inside such
a closure too.
`file_command_template_keeps_path_only_token_and_unknown_placeholder`
asserts that `code --goto {path}:{line}` with no line number produces
`["code", "--goto"]` — the path token is dropped, not kept. The name said
the opposite of the assertion, which is worse than no name: it describes
the behaviour someone would reasonably expect, so a reader skimming for
whether this edge is covered would conclude it is covered the other way.
The behaviour itself is deliberate and documented in the settings hint
("a flag whose value is absent is dropped"), and dropping the whole token
is right for the `--line={line}` case that hint is written around. Left
as it is; the comment now states the edge instead of hiding it, since a
`code --goto file:line` config opens no file at all on a link without a
line number and the caller only errors when every token has gone.
`explain_io` exists because "Permission denied (os error 13)" answers a
developer's question and not the reader's — its own doc says so. Every
failure routed through `HostOps::notify_err` gets it. Four that build
their notification string by hand did not, and printed the raw error.
The clearest symptom was inside one file: saving a file in the editor
went through `notify_err` and explained itself, while opening the same
file, denied for the same reason, said `os error 13`. Both file-link
openers are changed together — the file tree's opener deliberately shares
its wording with the terminal's (#542), so explaining one and not the
other would have split a pairing that was on purpose.
The `log::warn!` next to each call keeps the exact error. A developer
reading a log and a person who just lost a save want different things,
so both are written rather than one chosen — noted on `explain_io` along
with the rule, since it was the absence of a stated rule that let four
callers drift.
`tab_strip::clusters` already writes down why a label may only be cut on
grapheme boundaries: `👨👩👧` loses the joiner holding it together, `❤️`
loses the variation selector that makes it an emoji, and `🇨🇳` leaves a
lone regional indicator that renders as a bare letter. Three other
functions cut display text by `char` and so do exactly that.
`elide_middle` is the one that shows the gap most clearly. It documents
its cuts as safe because "everything here walks `chars()`, never bytes" —
true, and it guards the hazard that produces invalid UTF-8, which is not
the hazard that reaches the screen. Its sibling test is named
`never_cuts_a_multibyte_char_in_half`; the cluster is the unit above that
one, and nothing was holding it.
Each function keeps its own algorithm — head cut, middle cut, ellipsis
budget — and only the unit it counts in changes. That also makes the
budgets more honest than they were: a budget in clusters is a budget in
what actually gets drawn, where a budget in `char`s let one flag spend
two of it.
Callers pass ASCII in every existing test, so no rendered string that was
already correct changes.
`path_display` opens by calling itself the one place a path is measured
against `~`, because three rows spelling that check their own way is what
#544 was. Three sites in src/ui had since gone back to reading the
environment directly, and two of them read only `HOME` — the variable
#544 recorded as "often unset" on Windows:
- the file tree's fallback root, so a Windows window with no resolved
root drew an empty tree instead of the home directory;
- `expand_path`, so a `~/`-prefixed theme in config fell through to
`themes_dir().join("~/theme.json")` — a path that cannot exist, and no
error to say why.
The third (the SFTP download directory) already checked `USERPROFILE`,
but as a second copy of the canonical lookup under the same name, minus
its empty-string filter: `HOME=` set-but-empty resolved downloads to a
relative `Downloads`. It keeps its `.` last resort, which is a real
difference worth stating rather than deleting — a download has to be
offered somewhere, while every other caller wants the `None`.
No behaviour change on Unix, where `HOME` was already the answer.
A filename is bytes to the kernel, so `touch $'a\nb'` makes a file the
tree has to draw, and gpui breaks text on `\n` whatever the row says.
The row grows to two lines, its `text_ellipsis` contract stops holding,
and the rows below it slide down — one odd file misaligns the column.
The terminal's history rows already met this and already carry the fold
(`one_line`), so reuse it rather than spell a second one. What differs
here is where it may be applied: a history entry is only ever drawn, but
a filename is also handed to `join`, `rename` and `remove`. Folding at
ingestion would splice a `↵` into a name that has to match what is on
disk, so the fold sits at the two `.child(...)` sites and nothing on the
operations path sees it.
Whatever runs in a pane sets this, and `OscTokenizer` ends an OSC only on
BEL or ESC -- every other byte is payload, newlines included. `parse_osc_title`
then trimmed the ends and kept the middle, so `printf '\033]2;a\nb\007'`
produced a title carrying a newline.
That title is drawn as a tab label and written into the machine file, and
this tree already knows what a newline does to a label: "gpui breaks text on
`\n` whatever `white_space` says, so an entry carrying one paints its tail
over whatever sits below it" -- the reason history rows are folded through
`one_line_char` before they are drawn. A tab label had no such step, and
unlike a history row the string survives a restart.
Folded at the one place a title enters, so the record on disk is one line
too, rather than at each of the places that draw one. Character for
character, so the length cap still counts what the user will see, and
trimmed afterwards since folding can blank the edges.
Verified the test earns its keep: without the fold it fails with
`left: Some("first\nsecond")`.
`f32::clamp` returns NaN for a NaN input rather than rejecting it, so the
clamp guarding a stored split ratio is weaker than it reads: a NaN would be
kept and then multiplied through every size beneath it.
Nothing can supply one today, but establishing that meant reading three
files -- the machine tree refuses a non-finite ratio (`clamp_ratio`), a
session file cannot carry a NaN because JSON has no such literal and an
out-of-range number arrives as an infinity that clamps like any other large
value, and both places that compute a ratio are guarded (`set_run_shares`
divides only `if total > 0.`, which is false for NaN too; `resize_focused`
adds a caller's constant).
Written down at the line that would have to change, because the thing that
makes it safe is somewhere else entirely, and the daemon already spells the
same rule out with an explicit `is_finite`. No behaviour change: adding a
branch for an input that cannot arrive would be the other mistake.
Ending a pane was one function: work out which parked panes nothing holds,
then hang them up. Only the second half needs a running daemon, and mixing
them meant the rule could only be checked by watching a fuzz run -- which
swings from fifty operations to five between runs on the same seed. That is
how a change of mine looked effective for five iterations while doing
nothing at all.
`stranded_of` is now the rule on its own, and a test states it directly: a
pane on screen is never ended whatever the tree says, a pane the tree still
names is never ended whatever the window shows, and only one held by neither
goes. Each of those three has been the difference between a leaked shell and
a killed one at some point in this file's history.
No behaviour change -- same filter, same order, called from the same place.
Verified the test earns its keep: dropping the `showing` check from the
filter fails it with `left: [1, 4], right: [4]`.
The note already said what to do about the fork being older than the
client-side pre-auth fixes: rebase onto v0.62.6 and move the rev forward.
Checking today, there is nothing to move it to — every branch on the fork
sits at or before v0.62.2, its default branch head is still the v0.62.2 tag
commit, and upstream has published nothing after v0.62.6.
So the rebase is work somebody has to do rather than a newer rev waiting to
be picked up, which is worth saying: it is the difference between a one-line
bump and a fork to maintain.
Both hang-up paths guarded on "the window that raised this is not showing
the pane". The answer they are guarding is the daemon's, and the daemon's is
machine-wide: `collect_orphan_panes` names any registered pane no workspace
holds, whichever window spawned it.
So with two windows on one machine, the second could spawn a pane, have it
registered, and still have its TabCreate queued -- and a close raised by the
first would see it named as detached, find the *first* window is not showing
it, and end it. A pane the other window is about to draw.
The same mistake as judging a pane against a tree that has not caught up,
one window over. Now the guard is the union over every open window, and a
window that will not answer abandons the sweep rather than shrinking it: one
that cannot be read might be showing anything.
Found by re-reading my own two commits rather than by a failure -- the fuzz
that verified them drives a single window, so it could not have caught this.
Sixteen `#[allow(clippy::…)]` in the tree; thirteen carry a comment saying
why. These were the three that did not, and the reason turned out to differ
for each.
`spawn_once` takes eight arguments against a threshold of nine, so its
`too_many_arguments` allowed nothing at all -- removed. `render_ssh_row` does
trip it, at ten with `self`, and `LocalHost::new` really does hand back a
`SharedHost` rather than a `Self`; both now say so.
Removing all three first and reading what came back is what separated them,
and it needed two passes: clippy stops at the first crate that fails, so
tty7-core's error hid settings.rs's until it was fixed. A single clean run is
not evidence about anything downstream of the first failure.
Last of the three item kinds. 48 structs, enums and traits were `pub` with
no name outside tty7-core, and a `pub` type hides more than a function does:
its fields and variants are invisible to the lint too.
Nine more items surfaced, and the same split as before -- most are used only
by tests and take the house idiom; three are unreached here and carry the
reason instead. `absorb` and its neighbours `len`/`is_empty` on
GitignoreChain are simply spare: the matcher sets are built whole rather than
merged.
Most of the work was the compiler correcting me, and it corrected two things
a search could not:
- A type can be *used* without its name ever appearing -- through inference,
a method's return, a chain. `AgentEvent`, `ManagedWorktree`,
`ProfileUsage`, `RouteAction`, `WorkingDirectory` and `KiPrompt` are all
named nowhere outside this crate and all needed by it.
- Reachability is not just signatures. `AgentEventKind` sits behind a public
*field*, `Duplex` and `PaneDirectory` behind public bounds, and `Halves`
behind an associated type. Those are the `private_interfaces` lints, and
only `cargo clippy` reports them -- `cargo build` was clean while five of
them stood.
So the rule for anyone repeating this: narrow, then let the compiler put back
what it must, and read clippy rather than build.
The same blindness as the last commit, one item kind over: 139 constants and
type aliases were `pub` with no name outside tty7-core, and a `pub` constant
is as invisible to the dead_code lint as a `pub` function. Most are the frame
kinds and asset names the protocol and installer keep to themselves.
It turned up one: DEFAULT_REMOTE_SERVER_CMD, which only the router's tests
still name -- the production path builds the command from the install it
found. Kept, with the house idiom and a note saying what it now is: the
statement of that default, and tests that hold it to it.
`SharedConnection` stays `pub`; ssh::mod re-exports it, and the compiler says
so plainly.
tty7-core is `publish = false` and has exactly three consumers, all in this
workspace. 199 of its functions were `pub` without a caller outside it, and
that is not merely untidy: **rustc never reports a `pub` item as dead**, since
it cannot know what an external crate uses. Every one of them was a place the
lint could not look.
Narrowing them turned the lint on and it found things immediately -- 25 items
that had been invisible:
- Most are used only by tests, and take the house
#[cfg_attr(not(test), allow(dead_code))].
- Seven are unreached on this platform and are now annotated with the
reason. Two of those are WSL helpers a #[cfg(windows)] test does use, so
they read as dead here and are very much alive there -- exactly why none
of them were deleted on a macOS build.
Left `pub`: `host::conformance`, whose functions the
`host_conformance_suite!` macro expands into other crates' test binaries. The
compiler caught that one; a search for the names could not, because the only
mention is inside the macro in this crate.
The seven unreached ones are now visible and worth a decision. Some are
plainly spare (`connect_string` wraps `to_connect_string`, which callers use
directly); others are one half of a pair whose other half is used, where
deleting one is worse than keeping both.
The field was listed in `run`'s JSON and nowhere explained, on the surface
agents parse. It says whether the server managed to read a status before it
stopped waiting; when it did not, `run` exits 1 as a stand-in and says so on
stderr, and the flag is the only way to tell that 1 from a real one.
A command killed by a signal also comes back as `exit: 1` -- not the 128+N a
shell reports -- and with `exit_code_known: true`, because a status really was
read. The two are byte-identical in the JSON, so an agent cannot tell a
command the OOM killer took from one that exited 1 by itself, and it should
not be left to find that out the hard way.
That last part is a limitation rather than a choice: the pty crate keeps the
signal in a private field with no accessor, and hands back a placeholder code
of 1 in its place. `success()` cannot separate the two either, since it is
false for both. Reporting the real 128+N needs the child reaped directly --
which the daemon already does for adopted panes, where it gets this right.
Found by running the commands, not by reading them.
Three verbs, one condition, three sentences:
procs %99 no pane %99 on this machine — `tty7 pane ls --all` lists them
capture %99 observing pane %99: daemon refused Observe: no such pane 99
send %99 hi sending input to pane %99: no such pane 99
The daemon's own refusal makes a fine diagnostic and a poor sentence: it
names the wire request rather than the verb that was typed, so `capture`
told the user about "Observe", a word that appears nowhere else they can
see. This is the CLI agents read stderr from, and it was reporting one state
of the world three ways.
They now all answer with the line `procs` already used. The registry is
consulted only once something has failed, so the ordinary path still costs a
single round trip, and a failure with the pane present keeps its own words --
this must not swallow a connection error and call it a missing pane.
`resolve::no_such_pane` is now that sentence's one definition; the copy in
`workspace_of_pane` had drifted to suggesting plain `pane ls`, which does not
list the orphaned pane most likely to be asked about.
Found by running the verbs against a live daemon rather than by reading:
every one of these paths looks right in isolation.
The window drops a view whenever its layout is rewritten -- a delta arriving
for a tab it had built differently, a rebuild from the tree -- and dropping a
view does not end the shell behind it, because that is also what detaching
is. So every reshape left panes running that nothing could reach again. This
is where most of them came from: a 150-op fuzz against an open window shows
31 layout rewrites to 2 rebuilds and 12 refused ops.
Panes put down this way are *parked*, not ended, and judged later. At the
moment of the drop neither mirror can say whether a pane is still somebody's:
the machine copy is missing whatever this window has just done, since a
client is left out of the deltas its own ops raise (#612), and this
workspace's copy runs ahead of or behind ops still in flight. Judging there
does end live panes -- both a machine-mirror check and a two-mirror check
were tried, and the fuzz caught each of them killing panes the tree still
named. The parked set is swept only against a tree the window has just
pulled, which is the one account of the machine here not assembled from
deltas, and only for panes the window is also not showing. Anything that
came back into the tree or onto the screen meanwhile is forgotten rather
than ended, because that is exactly what the mirrors were wrong about.
Over six fuzz seeds: no run ends a pane the tree still names, and where a
pull lands during the run the stranded shells go from 15-26 to 0-2. A run
with no pull sweeps nothing and leaks as before -- the parking is correct,
the coverage is only as good as how often the window pulls -- and
`tty7 pane close --orphans` still reclaims whatever is left.
TabClose and PaneClose answer with the panes the tree let go of, and the
shells behind them keep running until somebody hangs them up. The daemon
will not: the same removal is how a pane crosses to another tab, so it
cannot tell an ending from a move, and whoever sent the op has to say.
`tty7 pane close` always has -- hang_up_removed_panes exists for exactly
this. The window did it only where a *person* closed something, and never
for the identical ops its own reconciliation raises, because `pump` sent the
batch with `if let Err(e) = client.call(op)` and dropped the Ok on the
floor. Every pane the window put down while squaring its layout with the
tree left a live shell nothing could reach.
Measured with a 150-op tree fuzz driving the CLI against an open window:
5-6 shells per run were being stranded this way, and are not any more.
It is not the whole leak -- around 14 orphans per run survive, from the
paths ITER 13 named (a superseded hydration, a delta that would not apply),
which strand panes without sending an op at all.
A pane the window is showing is never hung up, whatever the tree says. The
daemon computes its answer with collect_orphan_panes across the whole
machine *after* the change, so a pane that merely moved is never named --
but a pane this window has spawned and registered and not yet placed in the
tree is, because at that instant nothing in the tree holds it, and its
TabCreate may still be queued behind the op that produced the answer.
Ending that one takes down a pane the user is looking at (#628). If the
window cannot be read at all, nothing is hung up: a leaked shell is
recoverable with `tty7 pane close --orphans`, a shell ended under a live
window is not.
The check runs on the main thread after the batch returns rather than
beside the calls, because `pump` may be running inside an App that already
holds the window entity, and gpui answers that by silently dropping the
update -- which stops the window syncing at all.
gpui picks a prompt button's role by matching the English word -- "cancel"
becomes a cancel answer, anything else becomes a plain Other that answers
neither Escape nor Return. `confirm_answers` exists to say which is which
instead, and sixteen of the app's dialogs use it.
The two SCM confirmations built their answers by hand from localized
strings, so `t(Cancel)` came out as a real cancel answer in English and as
an Other in Chinese and Japanese: discard, reset and amend -- the dialogs
guarding the operations that lose work -- had nothing on Escape for those
users. They also passed Cancel as answer 0, which gpui draws rightmost and
gives Return, so the position that means "do it" everywhere else in the app
meant "don't" here.
Both now go through the helper, with the index check flipped to Ok(0) to
match. The single-button restart-failed dialog had the same problem from the
other side: t(Ok) is "确定" in Chinese, so its only button was not an Ok
button either.
Add a test for the underlying rule rather than the three call sites: it
pins that a localized cancel label does not become a cancel answer on its
own, and says so in the failure message if gpui ever starts reading roles
another way.
app.rs's server-mismatch prompt still builds its answers by hand, and stays
that way -- it is documented as having no safe answer to give Escape to.
confirm_question returned the hard-reset question for GitOp::Reset in any
mode, and that text ends "and uncommitted changes are discarded" -- true of
--hard, untrue of --soft and --mixed, which both keep the worktree.
It is right today only because GitOp::destructive classifies soft and mixed
as harmless, so they never reach a confirmation at all. That invariant lives
in another crate and nothing here said so, leaving a reader to go and check
before they could tell whether a mixed reset shows a warning about losing
work it does not lose.
No behaviour change -- the arm is unreachable for the other two modes either
way. It now states its own precondition instead of borrowing one.
confirm_verb keeps matching every mode on purpose, with a note saying why:
"Reset" is the honest button label whichever mode it is.
The codebase already decided a secret must not reach a formatter --
NativeSshSpec spells its Debug out by hand, AuthResponse prints
Secret(<redacted>), and secrets_are_redacted_in_debug_output pins both.
Three types carrying secrets were missed and still derived Debug.
- KeychainWrite::SetPassword/SetKeyPassphrase hold the plaintext the user
just typed. password_submit returns one of these *beside* an
AuthResponse built from the same String, so the identical secret was
redacted in one return value and printable in the other.
- Attachment::token is the proof a connection holds a workspace. Its own
doc says it "goes over no wire and onto no disk" and #[serde(skip)]
enforces that -- but the log file is disk too, and the token is reached
by the derived Debug of Workspace, Machine, and the whole tree.
- ControlHello::client_token is the same kind of proof, in the one module
that already logs protocol values with {event:?}.
None of the three is formatted anywhere today, so nothing was leaking; this
closes the gap while that is still true, since a single {:?} added later
would have written a password or a capability token to the log.
Each gets a test in the shape of the existing one. Verified non-vacuous:
putting the derive back on Attachment fails with
token leaked: Attachment { token: "s3cr3t-capability", ... }
Five modules carried #[allow(dead_code)] on the `mod` line itself, which
silences the lint for everything inside them -- for good. host_ops and
host_registry are live infrastructure (73 and 22 references between them);
one unused item each had switched the lint off for the whole file, so
anything that died there later would never be reported.
Two of the five were covering nothing at all: tty7-core's core::keychain
and core::ssh_profile have no dead items.
What the allows were actually hiding, and what happened to it:
- HostOps::run_or_notify, HostRegistry::local and HostRegistry::len are
unused everywhere, tests included -- deleted. run_or_notify was only
run_in composed with notify_err, both of which stay.
- InFlight::len and ByHost::len are used, but only by tests, so they take
the house #[cfg_attr(not(test), allow(dead_code))].
- status_rank is used only by the test that guards it against DecoStatus's
Ord, which its doc comment already explains. Same treatment.
The two `len`s are why this is worth doing carefully: --all-targets still
reports them as never used, because the warning comes from the non-test
build of the binary. Deleting on that evidence broke the test build. The
compiler, not the lint, settled which ones were really dead.
Each of these sits on an item the code does use: ssh_config's
import_profiles, merge_imported and jump_alias, SshConnection's key field
and key(), NativeSshSpec::without_secrets, CmdEditor::cursor, and
Pane::ssh_connection. Verified by removing every dead_code allow in the
workspace and reading back what the compiler then reported -- none of
these appeared, in either a production-only or an all-targets build.
A stale allow is worse than no allow. It reads as "this is dead, on
purpose", so the next person leaves it alone, and it goes on covering the
item after the code around it changes -- at which point the item really can
die without anyone hearing about it.
cursor_byte is the one that is genuinely unused outside tests, so it moves
to the house idiom #[cfg_attr(not(test), allow(dead_code))], which still
reports it if the tests stop using it too.
"the server answered a machine tree with {reply}" reads as though a machine
tree was the thing answered. It is the request that was for the machine
tree, and the reply is whatever came back instead.
The Chinese translator had already worked this out and supplied the missing
word -- "回复了机器树请求" says "replied to the machine-tree request" -- so
zh needed no change. English and Japanese carried the ambiguity.
"daemon" is the code's word for it -- the --daemon flag, daemon/server.rs,
the RemoteDaemon* keys -- and the UI deliberately uses "server": the
dialogs say "Restart Server?" and "Quit and Stop Server?", the settings
section is headed "Server", and the errors either side of the one fixed
here say "tty7's local server".
RemoteDaemonTooOld broke that in all three languages. It called the local
process a daemon while naming the far-end one a server in the same
sentence, so the two read as different components and the instruction
"Quit tty7 (which stops the daemon)" sent the user looking for something
the rest of the app never names. It now says "tty7's local server",
matching the two keys directly above it.
The ja settings heading said "デーモンサーバー" where en and zh both say
just "Server" -- mod.rs already documents that this heading is meant to be
that word on its own.
Add a test: no user-facing string may call it a daemon in any of the three
languages. The search-keyword lists are skipped, since they carry the word
on purpose to match what a user might type.
tty7 calls the split regions inside a tab panes and the docked regions
around them panels, and Chinese has a separate word for each -- 窗格 and
面板. Eleven strings across four keys used 面板 for a pane, so they named
a different part of the window rather than reading as a loose synonym.
The settings page showed both senses a few rows apart: "text size ...
tabs, panels and settings" is genuinely 面板, while "give each pane its
own shell history" right below it claimed panels have their own shell
history -- they have no shell at all. The proxy description, the stale
daemon notice and the restart-server body had the same swap.
Japanese was already consistent (ペイン / パネル), and the English is the
source, so only zh.rs changes.
Add a test over every key: when the English names one sense and not the
other, the translation may not carry the wrong word. It matches whole
words, since "panel" contains "pane".
The previous commit documented the `events` schema and listed `agent_status` as
one of the kinds a listener sees. It is not: `ControlEvent::AgentStatus` has the
same shape of problem as `PaneExited`, which is how it slipped through — the
only two references anywhere are the round-trip test in its own file and the
formatter in `tty7-cli`, with no emitter.
Auditing all six protocol enums is what turned it up. `ControlRequest` (42
variants), `ClientMsg` (30) and `DaemonMsg` (28) are clean; every `LayoutDelta`
and every other `ControlEvent` has a real emitter — `Preempted` and
`LayoutResync` in `host::server`, `GuiOpen` beside them — which is exactly what
made these two look supported.
Agent status is reported the same way a pane exit is: a `layout` delta whose
`pane_facts` carries the pane, with `agent.status` on it (`AgentFacts.status`
rides on `PaneRecord`). The note now covers both, and points at `tty7 wait` and
`tty7 agents` as the supported way to ask about an agent, since neither is
built on this stream.
Both variants now carry the comment; the recommendation is unchanged — emit
them or remove them together with their `event_line` arms, but not in passing,
because it is a dialect change.
2951 tests pass.
`events` is the streaming interface an agent builds on, and it was the one verb
whose JSON shape the reference did not give — every other one lists its fields
exactly. Now it does: the externally tagged envelope, the four event kinds a
listener sees, and the thirteen `layout` delta kinds.
The prose also listed "pane exits" as an event type. There is no such event.
`ControlEvent::PaneExited` is defined, encodes, and has a line
`tty7 events` would print for it — and nothing anywhere constructs one outside
the round-trip tests in its own file. `AgentStatus`, `Preempted` and
`LayoutResync` have 2, 10 and 6 emitters respectively; this one has none. An
agent waiting for a pane-exit line waits for good.
What actually happens is checked against a running server: exit the shell in a
pane and exactly one event arrives — a `layout` delta carrying `pane_facts` for
that pane with `"live": false`, and the pane is gone from `pane ls --all`. The
docs now say to watch for that, in a note that says outright there is no
pane-exit line whatever the wire protocol's shape suggests.
The variant keeps a comment saying the same thing, and that resolving it means
either emitting it where the pane is reaped or removing it along with the
`event_line` arm — not something to do in passing, since taking a variant out
of a wire enum is a dialect change.
2951 tests pass.
The warning on `pane close --orphans` said an orphan "can still be doing real
work — an interrupted `run` leaves the command running". True, and it stops one
step short of the case that costs someone work.
A `run` that is executing *right now* is an orphan too. `run --ws W` stamps its
pane with `W` as the owner and files it into no tab until `--keep` does, so for
the whole length of the command the pane reads exactly like a leftover.
Confirmed against a live server: `tty7 run --ws W -- sleep 45` shows up as
`orphan=true, owner=<W>`, the same shape a leaked pane has, and
`pane close --orphans` duly reported `closed 2 panes` — one of which was the
running command.
So the old advice — "look at `pane ls --all` first" — cannot be followed:
nothing in that listing tells the two apart. The docs now say that, and say
what to do instead (close by id when anything might be running).
This also rules out the reaper people will reach for when they meet the pane
leak in `ui::tree_sync`: the daemon's own `spawn_orphan_sweep` already computes
this exact set and deliberately only reports it, and an in-flight `run` is why
acting on it would be wrong. `orphan_panes` now carries that reasoning.
The distinction that would work is whether a client is still attached — an
interrupted `run` has none, a running one does. The daemon knows and
`PaneInfo` does not say; adding the field is backward compatible, since every
other field on it is already `#[serde(default)]`, but it is a wire change and
wants more than a doc pass.
2951 tests pass.
The daemon answers a pane it is not running exactly as it answers an idle one:
`registry.get(pane_id)` misses and the reply is an empty `PaneProcs`. So
tty7 procs %999
printed `nothing running in this pane` and exited 0 for a pane that has never
existed. Every other verb taking a `%PANE` says when the pane is not there, and
an agent reading this one could not tell the two apart — which is the whole
point of the machine-readable half.
Checked against the registry rather than the workspace tree, because a pane no
workspace holds is still a pane the server runs and still worth reporting on;
that is exactly what `pane ls --all` surfaces it for. Verified live: a real
pane and an orphaned pane both still answer with exit 0, and %999 now exits 1
with "no pane %999 on this machine — `tty7 pane ls --all` lists them".
Asked only when the answer came back empty, so a pane with anything running in
it still costs one request.
One existing test needed the mock's registry seeded alongside its machine tree.
That is the fixture becoming faithful rather than the check being loosened: a
server running the pane its tree names is what the real pair look like, and the
mock had the tree without the registry.
2951 tests pass.
With no daemon running, most verbs give the message that helps:
tty7 ls could not reach the tty7 server on this machine —
`tty7 server start` brings one up
The three that go to a pane instead of to the control socket did not:
tty7 send %1 hi sending input to pane %1: No such file or directory (os error 2)
tty7 capture %1 observing pane %1: No such file or directory (os error 2)
tty7 procs %1 No such file or directory (os error 2)
All three read as if the *pane* were missing, or some file — and `procs` gave
no context at all. Panes are reached over the daemon's own socket, and with no
daemon there is no socket, so the connect fails with a bare `NotFound` that
each caller then wrapped in its own words.
`local_control` has contexted its connect all along; this is the same courtesy
one socket over. The local pane client is probed once when it is built, so all
three now say what `ls` says.
Local only, deliberately: the message names *this* machine, and a routed client
would pay a round trip across the link to be told something untrue of the far
end. Cached with the client, so it costs one round trip per process.
Nothing real is swallowed — a daemon that is running answers a bad pane id with
its own message, and still does:
send %999 sending input to pane %999: no such pane 999
capture %999 observing pane %999: daemon refused Observe: no such pane 999
Checked against a live instance both ways, plus a full send/capture round trip
on a real pane.
2949 tests pass.
`serve_sigterm` blocks SIGTERM and waits for it on a dedicated thread, so that
`store_scrollback_now` can write every pane's screen one last time on the way
out — "the periodic writer covers the deaths nobody gets to prepare for; this
covers the ones we do".
It ran too late to do that. `pthread_sigmask` blocks a signal on the *calling*
thread only, and a signal sent to a process is delivered to any one thread that
has not blocked it. The call sat after the control listener was already
serving, so several threads had SIGTERM unblocked and the kernel could hand it
to one of them, where the default disposition ends the process on the spot:
`sigwait` never returns, no screen is written, and nothing is logged.
Measured against a running daemon, sending SIGTERM directly:
before the marker printed after the last periodic snapshot was absent from
the store, and "daemon shutting down on SIGTERM" never appeared
after the marker is in the store and the line is logged
So a machine shutting down, a logout, a supervisor stopping the daemon or a
plain `kill` cost every pane up to SNAPSHOT_INTERVAL — 30 seconds — of screen
that the code was written to keep.
The fix is where the call sits, not what it does: immediately after the
registry exists and before the control listener, which is the first thing in
`run_daemon` to start a thread. A thread inherits the mask of the one that
created it, so blocking there makes the whole process deaf to SIGTERM except on
the waiter. Blocking and waiting stay in the same call, so there is never a
window where the signal is blocked with nobody to answer it.
`tty7 server stop` was never affected: it asks over the protocol
(`ClientMsg::Shutdown`) and only falls back to signals, so it took the other
`store_scrollback_now` path.
2949 tests pass.
`Config::load` runs on every pane spawn and every palette command, and the
parse-failure arm logged a warning each time. One stray comma in config.json
therefore produced a log made of the same line: measured at 23 copies after
opening three tabs and sending one command, growing for as long as the session
lasts and burying every other warning in the file.
The answer was already there. `quarantine` refuses to keep a second copy of
contents it has kept before — that check is what stops a broken config filling
the directory with identical `.corrupt` files — so it already knows whether a
breakage is newly seen. It now says so, and the parse message is a warning the
first time and a debug line afterwards.
Nothing else moves: the file is still kept aside, `save` still refuses to run
against a quarantined config, and the user's file is untouched.
Verified against a running instance with a deliberately malformed config.json:
the same four operations that produced 23 warnings now produce 1, there is
still exactly one `.corrupt` file, and config.json is byte-identical to what
was written before the app started.
2949 tests pass.
The note said refused tree operations are how this window strands a shell.
They are one way, and the minority one — which matters, because it is the way
that would attract a fix.
Measured with a randomized tree fuzz over the CLI (70 operations: tab new,
split, pane close, tab close, tab move, invariants checked after each):
daemon only, no GUI 0 orphaned shells
same run, window open 16 orphaned shells, 12 panes actually held either way
A second run of 70 left 10 orphans, and the warning this note sits on fired for
3 of them. The other 7 were spawned for a layout the window then threw away
without sending an operation at all — the log shows "dropping a superseded
hydration" and a delta that "did not apply cleanly" forcing a re-pull. Nothing
is refused on those paths, so nothing reaches this arm.
They are live `zsh` processes, not bookkeeping: each orphan answers
`tty7 procs` with a pid.
So the note now says which share this path accounts for, and that the sweep has
to be phrased against the end state — a pane this window spawned that neither
the machine tree nor any live view holds is stranded however it got there —
rather than against any single failure, which would fix a third of it.
Still diagnosis only, for the reason already recorded: the sweep belongs after
a hydration settles, and verifying it needs a GUI this environment cannot
watch.
2949 tests pass.
Last commit hedged on whether the missing russh fixes matter to a client,
because the release notes describe them as a client crashing its own session.
Matching the whole lockfile against the GitHub advisory database settles it —
the advisory titles say plainly what the release notes did not:
GHSA-5xvq-cp9x-6p6r "Pre-auth remote panic via all-zero Curve25519 peer
public value (encode_mpint OOB)"
GHSA-g9hv-x236-4qp3 "client wrong-length X25519 clone_from_slice panic
(pre-auth DoS)"
tty7 is the client, the peer is whatever server a profile dials, and this
lands before authentication — so anything sitting in front of that server
reaches it too. Both are patched in 0.62.4; this rev is from 2026-07-14 and
predates it.
The other three are named too, with the two server-side ones marked as such
since tty7 runs no SSH server.
Still documentation only, for the reason the previous commit gives: moving the
SSH layer wants an SSH connection to test against, and connecting needs the GUI
connection manager, which this environment cannot drive. The fix is unchanged —
rebase the fork onto v0.62.6+, and drop the patch once #738 ships in a release.
2937 tests pass.
The patch note said "temporary until upstream releases gssapi-with-mic
support (PR #737)" and left it there. Checking that exit condition turned up
three things worth writing down.
The PR number is wrong: #737 does not exist. The real one is Eugeny/russh#738,
and it was merged upstream on 2026-08-11T09:27:51Z — so the condition this
patch was waiting for has half arrived. Only half: v0.62.6 was published at
09:26:56Z the same day, one minute before the merge, so no release carries it
yet and the patch still has to stay.
The part that matters more: this rev is dated 2026-07-14 and branched before
v0.62.3, so it is missing every security fix upstream has published since —
v0.62.4's three (malformed PTY request, malformed Curve25519 KEX packet, zero
Curve25519 key), v0.62.5's channel-ID validation and v0.62.6's
max_auth_attempts. The last two are described as server-side and tty7 runs no
SSH server, but "fix mpint encoding and validate curve25519 keys" is in key
exchange, which a client runs too, against whatever server it dials. Flagged
for assessment rather than asserted: I have not reproduced it.
No dependency change here. Moving the SSH layer wants an SSH connection to
test it against, and connecting needs the GUI's connection manager, which
cannot be driven from this environment. The interim fix is to rebase the fork
onto v0.62.6 or later; the real one is to drop the patch when #738 ships.
Verified with the GitHub API rather than assumed: commit dates on both sides,
and the fork does not contain any of the four fix commits.
2937 tests pass.
tty7 has a `ClipboardLoad` arm that reads the system clipboard and writes it
back down the PTY, and nothing in this crate says that must not happen. It does
not happen — `alacritty_terminal` refuses a paste request before it becomes an
event, because `Config::osc52` defaults to `OnlyCopy` — but that is the VT
crate's decision, taken by a field tty7 never sets, protecting a handler tty7
has already written.
So the exposure is one upstream default away: a release that moved `osc52` to
`CopyPaste` would hand any program able to write to a pane whatever is on the
clipboard, with no prompt and nothing in the sequence identifying who asked.
The program can be on the far end of an SSH connection, and a clipboard
routinely holds a password, a token or a private key. Terminals that offer the
read at all put it behind a prompt or an explicit opt-in.
`osc52: Osc52::OnlyCopy` is now stated where the rest of the terminal config
is, so writes still work and reads stay refused by tty7's own decision. No
behaviour change today. Same shape as `conpty_resize` two lines up, and it gets
the same kind of test, for the reason that one documents: a field whose default
already matches is a field whose line can vanish unnoticed.
The `ClipboardLoad` arm keeps a note saying what makes it unreachable and that
unlocking it needs a consent step first.
2937 tests pass.
A kitty graphics escape is untrusted input — any program running in a pane can
print one — and every bound on that path is deliberate and spelled out, except
this one. `to_rgba8` clamps an inflate to `min(declared, MAX_IMAGE_BYTES)` and
says why the declared size cannot be trusted on its own; the PNG branch called
`image::load_from_memory`, which leaves the ceiling to `Limits::default()`.
That default is 512 MiB — a real bound, eight times `MAX_IMAGE_BYTES`, and
somebody else's number. A patch release that relaxed it would silently widen
what a hostile escape sequence can allocate, and this runs in the GUI process,
so an OOM here takes every pane down rather than the pane that asked for the
image. The limit is now stated locally at the value the default has today, so
behaviour is unchanged and the invariant stops depending on a dependency's
opinion.
Why the number is not simply `MAX_IMAGE_BYTES`: that bound comes from what a
wire frame carries, and a PNG crosses the frame still compressed, so its
decoded size is the one thing on this path not already settled. Lowering it
would bound this properly but decides how large an image tty7 means to display,
which is a product call and not a cleanup — noted in the comment.
The PNG branch had no test at all, so the change is covered both ways: a real
2x1 PNG decodes at its own dimensions rather than the escape's `s=`/`v=`, and
a 1x1 PNG whose IHDR is rewritten to 0xffff x 0xffff (~17 GB, CRC repaired so
the decoder parses it) is refused. Checked out of band that the refusal really
is "Memory limit exceeded" and not a malformed-file rejection, so the test
cannot pass for the wrong reason.
2936 tests pass.
`StdioDuplex::take` dup'd stdin and stdout, redirected both to /dev/null, and
only then wrapped the raw descriptors in `File`. Any of the four `?`s between
the first `dup` and the wrap returned without closing what it already held —
a descriptor leak on the error path.
`dup_fd` now hands back an `OwnedFd`. `dup` returns a fresh descriptor nothing
else holds, which is exactly that type's contract, and ownership from the
moment it exists is what makes the early returns safe. `take` drops two
`unsafe` blocks in the process: `File::from(OwnedFd)` is the safe conversion.
Immaterial in production — `take` is called once in tty7-server's main and the
`?` there exits the process — but it is unsafe-adjacent code where the correct
version is also the shorter one.
Exercised by the 55 `stdio_conformance` tests, which drive
`tty7-server --stdio` through this constructor. 2934 pass.
The table synthesized the `local` row itself while `--json` serialized the
server's routes alone, and a route is a link to some *other* machine. So on a
machine with no remotes — which is every machine until someone connects one —
`tty7 machine ls` printed a row for itself and `tty7 machine ls --json`
answered `{"machines":[]}`. An agent enumerating machines read that as "there
are none", including the one it was running on.
The docs already said what it should be: "the local machine plus every link",
with `{"machines":[{"key","kind","connected"}]}`.
The list is now assembled once in `machine_ls` and handed to both renderings,
so the two cannot disagree again; `routes_table` renders what it is given
instead of adding a machine to it. Pinned by a test that asserts the table and
the JSON agree, with and without a link.
Found by running the command against a dev instance rather than by reading it —
the human output looked right, and it was the half nothing prints by default
that was wrong.
2934 tests pass.
`TabCreate`, `PaneSplit` and `PaneReplace` each carry a pane the window has
already spawned — the shell is running on the machine before the tree is told
about it. When the tree refuses one of those, nothing ever takes the pane and
the re-pull leaves the window without the tab it was for, so the shell goes on
running with nothing referencing it. The log said only that an operation was
refused, which is the one thing that does not point at the leak.
Reproducible against a running instance: with a window open on a workspace,
`tty7 tab new` and `tty7 tab close` back to back. The GUI restores the new tab,
finds its pane already hung up, spawns a replacement, and `PaneReplace` is
refused because the tab is gone. Four cycles in five leak a shell; leave half a
second between the two and none do. Killing the GUI and repeating the loop
leaks nothing — the CLI half is correct throughout.
Not swept here, and the comment says why: at the point of the refusal the
window still holds a view for the pane and only drops it once the re-pull
lands, so hanging it up here would kill a pane that is still on screen. The
sweep belongs after the pull settles and has to test the whole machine tree
rather than this workspace's mirror, since a pane belonging to another
workspace on the same host is not this window's to end. That change wants a
GUI it can be watched in; this one only makes the leak say its own name, and
`tty7 pane ls --all` already points at the recovery.
`seeded_pane` is pinned by a test, because the set of requests that name a pane
into existence is exactly the set a refusal can strand one from — a new one
added without it would leak silently.
2933 tests pass.
Found by driving a dev instance with the CLI rather than by reading, which is
the only way any of these would have surfaced.
`ws attach` is documented as "become its controlling client", and the CLI
cannot be one: the claim belongs to the connection that made it and the
connection ends when the command does, so `tty7 ls` reads unattached again the
moment it returns. Its real and only lasting effect is displacing whoever held
it — which is exactly what the human output says (`took over from HOST`) and
what the docs never did.
`ATTACHED` said it names "a GUI window, or another client". A GUI only claims a
workspace it is showing as a *remote* one; a window on a workspace of its own
machine claims nothing, because there is no second client to arbitrate against.
So with a window sitting on it `tty7 ls` prints `-`, and the column read as
"nothing has this open" when it means "no remote client holds this".
`tab move @TAB INDEX` never said which end `INDEX` counts from. It is 0-based —
beside an `@N` address that is 1-based, on the same command line — so
`tab move @1 2` moves the first tab to the third slot. Verified against a
running server: `@1 0` leaves it, `@1 1` puts it second, past the end clamps to
last the way `split --ratio` already documents. `to` in the JSON echoes the
number you asked for rather than where the tab landed, which is worth saying
since the clamp makes those differ.
Also records why `WorkspaceDetach` throws away the one thing it computes: the
reply has always been `Unit`, and the dialect is spoken to whatever build was
pushed to a remote machine, so widening it to `Bool` would break every server
already out there.
2932 tests pass.
Nothing had ever run `cargo doc`, so 16 warnings had collected. Four were
links to items that do not exist, and two of those were worse than a dead
link: `git_badge` and `info_chip` documented their sizes in terms of
`PANEL_TEXT` and `PANEL_TEXT_META`, px constants deleted when the interface
font scale landed. The module comment twenty lines up already says they went;
the prose downstream still derived pixel arithmetic from them, so a reader was
being told the pill is 20px tall against a 19px neighbour when both are now
rems that move with `ui_font_size`.
Rewritten against the ladder that exists (`META_MONO` beside `TEXT_MONO`), and
`info_chip`'s comment now records what its own numbers imply: its padding and
radius are pixels wrapped around rem-sized text, so the two stop agreeing once
the interface scale leaves 100% — the same trap `PIP_SIZE` right below it is
written in rems to avoid. Left as a note rather than changed, because that is a
visual decision and this cannot see the result.
The other ten are `private_intra_doc_links`, and that lint does not apply here:
it exists so a *published* crate does not ship docs whose links dead-end, and
all four crates are `publish = false`. Allowed at the crate root with that
reason, because a public item explaining how it relates to a private one is the
useful half of these comments.
The `clippy` job becomes `lint` and runs rustdoc too, on the same warm cache.
Still non-required.
2932 tests pass.
`theme_preset` replaced `theme` on 2026-07-17, but the field stayed on the
struct, so every save since has written a `"theme": "light"` into the user's
config.json that nothing reads. It has no doc comment — the only field in the
file without one — no migration reads it, and grepping for it finds only the
`ui::theme` module.
Removing it is backward compatible in the direction that matters: `Config`
takes no `deny_unknown_fields`, so a file still carrying the key loads and the
key simply stops being written back. There is nothing to migrate; by the time
it was removed no build in the wild consulted it, which is also why a config
upgraded from before July never had its theme carried across.
Pinned by a test, because "the old file still loads" is the whole claim.
Found by diffing the documented configuration keys against the struct: it was
one of three fields with no entry in docs/reference/configuration.mdx, and the
other two (`command_frecency`, `ssh_profile_frecency`) are usage counters
rather than settings and are right to be undocumented.
2932 tests pass.