`run --keep` resolves `--ws` against the machine tree before it spawns
anything, and takes `$TTY7_WS` on trust — it only parses the id. Filing
the pane happens after the spawn, so a shell whose workspace has since
been removed, or one opened against another machine, started the pane and
then failed to file it. The pane kept running with nothing holding it.
Measured against a live daemon: with a `$TTY7_WS` this machine does not
have, `run --keep` exited 1 with a clear message and left a `sleep`
behind, `pane ls --all` reporting `"orphans":1`. With `--ws` spelled out,
the same bad id spawned nothing.
Recoverable — `pane ls --all` finds it and `pane close` ends it, both
documented — but nobody asked for the pane, and `$TTY7_WS` is the
*documented default* for `--keep`, so this is the ordinary path rather
than an exotic one.
Resolving the inherited id is now the same check the explicit one gets,
and only when `--keep` needs it: a plain `run` uses the workspace as an
ownership stamp, where a stale id costs nothing and the round trip to
find out would.
The spawn still precedes the filing — `TabCreate` names the pane the
daemon assigned, so it has to, and
`run_keep_spawns_first_then_files_the_pane_into_the_workspace` pins that.
What moved is only where the workspace is checked.
`serve_sigterm` blocks SIGTERM on the daemon's main thread before any
other starts, so every thread inherits the block and only its `sigwait`
waiter ends the process. That is deliberate and the comment there
explains it well. What it also does is reach places it was never meant
to: `fork` copies the calling thread's mask into the child and `execve`
keeps it.
So every pane ran with SIGTERM blocked. `kill`, `pkill`, `timeout`, a
supervisor, a CI cancellation — none of them could stop anything in a
pane, and only `kill -9` would. Measured, not reasoned:
tty7 run -- sh -c 'kill -TERM $$; echo STILL-ALIVE; exit 7'
→ STILL-ALIVE, exit 7
portable-pty does reset the child's SIGTERM disposition to `SIG_DFL`, but
a blocked signal is never delivered for a disposition to apply, so that
was never going to help.
This is the unix half of the Windows line three above it, which resets
the inherited "ignore Ctrl+C" state for the same reason: the daemon's own
signal state is not the pane's. That one cost a pane every Ctrl+C
(#451, #314); this one cost it every kill.
The block is lifted for the spawn alone, because the mask a child forks
with is the one it keeps, and the guard restores the whole saved mask
rather than re-blocking, so a thread that never had it blocked is left as
it was. The cost is a fork-wide window where a SIGTERM meant for the
daemon could land on this thread and end it without the scrollback save;
that is a millisecond against every pane being unstoppable.
Verified against a running daemon, both halves: the command above is now
terminated with nothing printed, and a SIGTERM to the daemon with a live
pane still exits it and still leaves that pane's screen in
`scrollback/`, which is what `serve_sigterm` exists to do.
Checking whether the Windows arm shared the bug fixed in 8959421 found it
already right — and better. `rollback_portable_failure` is one helper
called from both failure sites, so its two arms cannot drift; it removes
the backup only `if restore.is_ok()`; and `recovery_error` ends the
message with "backup preserved at {path}".
macOS said only "restoring the previous app: {restore}". Same situation —
the update failed, the restore failed, there is no app — and the platform
that leaves someone without one was the platform that did not say where
the copy is. Both macOS arms now end the same way, with the same words
Windows uses.
Nobody is reading this message while things are going well. It is what is
left when an update has taken the app and failed to put it back, and a
path in it is the difference between a recoverable morning and a
reinstall.
`replace_and_relaunch` moves the current bundle into the stage, then
renames the replacement into its place. Between those two lines there is
no app at all, and the backup inside the stage is the only copy.
If the second rename failed, the restore was attempted with `let _ =` and
the stage was deleted regardless. A restore that failed therefore took
the backup with it: nothing at `current`, no copy left, and an error that
named only the swap. That is a reinstall.
The launch-failure arm ten lines below already does this correctly — it
matches on the restore, reports it in the message, and leaves the stage
alone so the backup survives. This one now does the same. Two arms of the
same function disagreeing about how much care a missing app deserves is
the whole of the bug.
The new test covers the swap-failure path, which had none: a replacement
that was never extracted fails the second rename, and the previous app
has to be back at `current` with its own marker. Removing the restore
fails it.
What it does not cover is the restore *itself* failing — the seams here
are `launch` and `report`, and neither runs between the two renames, so
forcing that would mean injecting the rename. The correctness of that arm
rests on mirroring the tested one below it.
Checking whether any setting is written but never read turned up one
field named nowhere outside `config.rs` — `agent_commands`. It is read,
through `agent_commands_cached`, which is the point: that is the only
setting behind a `OnceLock`, so a running server keeps the map it built
at startup.
Every other key here is picked up by a reload. This one is not, and
nothing about editing the file says so: a user adding `{"cc": "claude"}`
sees the wrapper go on being unrecognised and has no reason to suspect
the server rather than the spelling.
The cache is right — the map is read on every pane spawn, and the
alternative is `Config::load()` off disk each time a pane starts. So the
row says `tty7 server restart` and the function says why it costs that,
each pointing at the other.
All 77 fields are read; this was the only one worth a word.
`l10n_keys!` carries `#[allow(dead_code)]` for a good reason — whether a
key is reachable is a per-platform question, and a macOS build flags a
dozen that Windows and Linux do use — and it names the audit that gets
around that: clippy per platform, intersected.
A text search over the whole tree answers the same question in one pass
and sees `cfg`-ed-out code too, which the per-platform build cannot. It
leaves 16 keys nothing names. Eight are accounted for: three
`SettingsLanguage*` are `label_key`s in `SUPPORTED_LANGUAGES`, and five
`Scm*` sit in `SCM_KEYS_AWAITING_A_CALLER`, which exists to say exactly
that. The other eight are named by nothing at all:
- six `SwitcherStatus{Restarting,Installing,Connecting,ConnectFailed,
NotConnected,Reconnecting}` — the row shows link state "as the dot's
color" now, so the words they were written for are not rendered
anywhere. `SwitcherStatusTakenOver` beside them is still live, which
is why the family reads as used at a glance.
- `SwitcherConnectToUse`, last touched by "stop opening context menus
with every row greyed out" — the affordance went, the string stayed.
- `AppAgentHooksOpFailed`, never wired since the locale split added it.
Twenty-four translations for nine strings nobody can see. Deleting the
enum variants is what makes it verifiable: the three locale tables match
`L10nKey` exhaustively, so the compiler picked out every arm.
`SCM_KEYS_AWAITING_A_CALLER` is the convention here — a key with no
caller is either named there or gone. These were neither.
`SplitRight` and `SplitDown` were taken out of `HOME_SHORTCUTS` because
both need a pane and the home page is what a window shows without one —
"advertising two chords that do nothing from the only screen that offers
them", as the comment there puts it, with a test to keep them out.
Their labels stayed. `home_shortcut_label` still had an arm for each, and
`HomeSplitRight`/`HomeSplitDown` were carried in three locale tables to
serve those arms and nothing else.
Nothing could point this out. The single caller iterates
`HOME_SHORTCUTS`, so the arms are unreachable — but `_ => action` makes
the match total, so no compiler warning, and the keys are textually
referenced, so `dead_code` sees them as used. Only reading the caller
does it.
Removing the enum variants is what makes this safe rather than hopeful:
the three locale tables match `L10nKey` exhaustively, so the compiler
named every translation to delete and would have caught a fourth.
The note said the fork's position was verified and the exploitability was
upstream's word. The first half is now first-hand too: the vendored
checkout says 0.62.2, and `kex/curve25519.rs` still does
`remote_pubkey.0.clone_from_slice(remote_pubkey_)` into a `[0; 32]` with
nothing checking the length of the peer's slice — GHSA-g9hv-x236-4qp3 as
written, reached before authentication.
The second half is what the note was missing: what it costs here. The
panic lands in a task on `SshManager`'s tokio runtime, which unwinds the
task and keeps its workers, and it cannot poison `conns` or `probes`
because nothing in `daemon/ssh` holds a `std::sync::Mutex` across an
`.await` — `await_holding_lock` is warn-by-default and the gate is
`-D warnings`, with no `allow` for it anywhere. So a hostile server hangs
up one dial rather than taking the SSH subsystem or the daemon with it.
That does not retire the rebase, it prices it: a per-connection denial,
recovered by dialling again, not a way into the process.
Static reading, not a live reproduction, and the note says so.
`portable-pty` builds a signalled child's status as `code: 1` —
`std::process::ExitStatus::code()` is `None` for one on Unix and its
`From` impl falls back to 1 — and keeps the signal in a private field
with no accessor. So `exit_code()` returns 1 and `success()` returns
false for a command killed by SIGKILL *and* for one that exited 1 on its
own, identically.
The probe passed that straight through, which reached the CLI as
`exit_code_known: true` — the field `docs/cli/reference.mdx` describes as
"how you tell a real 1 from a stand-in". An agent reading it was told a
killed command had chosen to exit 1.
Signalled now reports unknown, which the CLI already renders as an exit
of 1 *with* `exit_code_known: false` and a line on stderr. The exit code
a caller sees does not change; what changes is that it is no longer
claimed as the child's own.
Not the `128 + n` an adopted pane gets: that path calls `waitpid` itself
and has the number. Here the only thing left is a name from `strsignal`,
which is localized, so mapping it back would be a guess dressed as a
fact — and a wrong 137 is worse than an honest "unknown".
Sniffing `Display` is the only discrimination the crate's public API
offers. The test builds both cases from the crate's own constructors and
asserts the exact prefix, so an upstream rewording fails there, beside
the reasoning, instead of quietly turning every signalled pane back into
an exit of 1. It also pins `killed.exit_code() == 1`, which is the trap.
`open_config_file` has been `#[allow(dead_code)]` since
22e1ab16 — the initial commit — with nothing to say why. It is not
staged for anything: it was dead when it arrived.
It also hand-rolls a platform opener, picking between `open`, `explorer`
and `xdg-open` itself, while this file already reaches for
`cx.open_with_system` two thousand lines up and `terminal::view::
open_file_path` exists for the same job. Whoever wires an "open
config.json" affordance will write one line, not these twenty, and will
get the error reporting the file-link opener already has.
The other `allow(dead_code)` nearby is the counter-example and stays:
`palette::CheckoutBranch` says in its doc what it is waiting for —
"nothing emits it until the picker can list refs" — which is a parked
feature rather than a forgotten one. An allow that explains itself is
worth keeping; this one never did.
The panel picks a free name, downloads to a sibling temp, then renames
onto it — and `rename` replaces whatever it lands on without a word. The
gap between choosing the name and arriving at it is the whole transfer,
and `~/Downloads` is a directory a browser, another tool, or the user
writes to as well. `claimed_downloads` closes that gap against tty7
racing itself and can say nothing about anyone else, so the comment that
numbering the second copy means a download "cannot lose a file" was true
only of tty7's own copies.
Now the rename is refused if anything is sitting there. That costs a
retry, which numbers the file the way a second download already does, so
the promise holds. Still a check before an act — nothing portable makes
rename refuse an occupied name — but the window goes from a whole
transfer to the moment either side of one line.
`symlink_metadata`, not `metadata`: the latter follows a link, finds
nothing, and calls a dangling symlink a free name — and the rename would
then destroy a link the user made. The two read as interchangeable, so
there is a test for that case specifically.
The download itself has no harness here — it needs a live `SftpSession`,
and this module's tests cover pure helpers — so the guard is carried by
that predicate test and the compiler, not by an end-to-end run.
`query_reply_refuses_shm_file_on_remote_pane` probed `t=d` and `t=s` and
stopped, while its local counterpart probes all three of `t=s`, `t=f` and
`t=t`. The refusing side is the one where a gap would matter: `t=f` and
`t=t` name a path on *this* machine, and on a remote pane the sender is
at the other end of an ssh link.
The behaviour was already right — `honored` is
`medium == Direct || honors_indirect_media()`, one classification for
every indirect medium, so file and tempfile were refused by the same
branch shm was. This is coverage catching up with the name, not a fix.
Confirmed it earns its place: honoring `Medium::File` on a remote pane
now fails the test naming the probe, where before it passed untouched.
The message spells the probe out the way the shm-name test does, so a
failure reads as a probe rather than 36 bytes.
`GitOp::validate` is where a ref, a branch or a remote is checked before
it reaches git's argv, and it ended in `_ => {}`. The default of that
catch-all is "nothing to check", so an op added later carrying a name
would be spawned unvalidated — and the `Push` arm right above says what
that is worth: the branch goes into a `HEAD:<branch>` refspec, where a
`:` smuggles in a second one and `:foo` alone deletes remote `foo`.
`check_branch` rejects a leading `-` for the same reason.
Nothing falls through wrongly today. The staging verbs and both discards
carry only paths, and the pathspec check at the top of the function has
already run over every one of them; `Commit` and `Stash` carry a message,
which git reads as the value of `-m` wherever it starts; `Pull` names its
mode with an enum. Saying so is the point — the `_` hid that these were
each considered.
A new variant now trips E0004 in four places: `label`, `destructive`,
`argv`, and this. Verified by adding a probe variant carrying a `rev`,
which is exactly the shape that would have slipped through.
`every_dispatchable_action_has_a_slot_to_bind_it_in` states its invariant
exactly — an action `make_binding` answers to but `default_bindings` does
not list "cannot be bound at all: not from config.json, which drops it
silently, and not from the Keybindings page, which reads the second" —
and then checked three names against it. Two hand-maintained lists, 106
entries each, and a sample of three.
The set only exists as a `match` in this file, so there is nothing to ask
at runtime. Reading the arms out of the source is the same move `aumid`'s
test makes against `windows-installer.iss`, and it covers both directions
now: dispatchable with no slot, and a slot that dispatches nothing —
which binds a key to silence.
Confirmed against both. Deleting `NewTab` from either list fails the test
naming it; the lists agree at 106 today, so neither failure was waiting.
A guard on the parse too: fewer than 100 arms found means the scan has
stopped reading what it thinks it is, and would otherwise pass by
checking almost nothing.
`index_titles_match_rendered_row_labels` walks eleven titles and asserts
each one is in the index. It never touches a rendered row, so it says
nothing about whether every row has an entry — which is the question its
name answers, and the one worth asking: a setting added without an index
entry cannot be found by search, and nothing fails.
Renamed to what it does, with the gap written down rather than implied.
Walking the rows would mean rendering the page — they are built inline
rather than from a table, so there is no list to compare the index
against — and that is worth knowing before someone reaches for this test
expecting coverage it does not have.
No claim here that an entry is actually missing today: 69 are indexed and
I did not enumerate the rows to check.
`assets/icons/chevrons-up-down.svg` is drawn to the weight every other
glyph in that directory uses — `stroke-width="2.1"`, all twenty of them —
but it was the one path missing from `agent_icon`, so it fell through to
`gpui_component_assets` and the tab strip drew the stock 2.0 chevron
beside neighbours a hair heavier. Nothing failed, which is why nothing
said so.
`every_git_icon_resolves` could not catch it: it walks three paths
written out by hand, and its own comment names this exact risk — "an SVG
on disk that nobody added to the match above silently renders as
nothing". A list cannot notice the file it was never told about.
So the new test reads `assets/icons` instead and requires every `.svg` in
it to come back from `agent_icon`. Confirmed it fails without the arm
this commit adds, naming the file.
Found by checking shipped data rather than code — the same pass that
turned up the newline descriptions in `assets/completions`.
`elide` exists to make a completion description fit one row, and it only
ever bounded the length. A newline defeats the row at any length: gpui
breaks on it whatever the row says, so the description grows a second
line and its tail paints over whatever the menu is floating above.
This is shipped data, not a hypothetical. 122 of the specs in
`assets/completions` carry a newline in a description, and the break is
early enough to land inside the budget the menu hands out: ten of them
break before column 30. `go build -race` opens "Enable data race
detection." and breaks at column 27.
Folding in `elide` rather than at the call site, because every caller of
it wants one line by definition — and because the early return for text
already under budget was the path that handed a newline straight back
untouched, which a fix at the call site would have had to remember.
One cluster in, one cluster out, so the budget still measures what is
drawn.
Found by finishing the enumeration 1c203cc started, this time by asking
which *data source* a drawn string comes from rather than which module
draws it. Completion descriptions come from spec files, and 97 of those
ship in this repo.
The file tree stopped drawing raw filenames in 25d1f06, on the reasoning
that it was the one place a name arrives from a filesystem nobody here
chose. That was wrong by one: the SFTP panel draws names from a listing
on *another machine*, which is the same hazard with more claim to it —
gpui breaks a row on a newline whatever the row says, and the bytes are
picked somewhere this window has no say over.
Both sites the panel draws a name at: the entry row, and the transfer
row's `remote_basename`.
Display-only, as before. `entry.name` is what `remote_join` builds every
download, delete and rename path from, at five call sites, and a `↵`
spliced into one of those names nothing on the server. Checked after the
fact that every `one_line` in the file sits in a label.
Found by re-reading the scoping note from that commit while auditing
something else. It said only the file tree took names from an arbitrary
filesystem, and listed where not to bother. A remote listing is more
arbitrary than a local one, not less.
`remote_bootstrap` cached whatever `probe_remote_shell` returned, and
that function answers `None` for two unrelated reasons: the remote runs a
shell there is no bootstrap for, and the probe never got asked — the
channel would not open, the exec failed, or the five-second read ended
before the answer arrived.
The cache is a `HashMap` on the process-wide `SshManager`, behind a
`OnceLock`, with no TTL and nothing that evicts. So one slow or busy
moment on the first pane to a host turned into "this host has no shell
integration" for the rest of the daemon's life: no prompt marks, no cwd
tracking, no command status, no error, and reconnecting does not clear it
because the entry outlives the connection.
Told the two apart at the only place that can tell them apart — the
output. `probe_answer` reads exactly as far as `parse_probe` does before
it can form an opinion, and returns `None` when the marker and the line
it introduces did not both arrive. Only an answer is remembered; a
silence is asked again on the next pane.
The cost lands on the case that deserves it: a host that genuinely cannot
be probed pays one probe per pane instead of one per daemon, bounded by
the same timeout as before. A host that answers pays nothing extra.
The decision is a pure function over the probe's text, so it is tested
rather than argued about; the async path around it is unchanged bar the
return type.
Auditing wall-clock staleness decisions turned this one up: for a writer
whose start time cannot be read, the guard holds while
`written.elapsed().is_ok_and(|age| age <= GUARD_TTL)`. `SystemTime::elapsed`
is an error when the timestamp is in the future, so a guard dated ahead of
now — the clock stepped back under it, or a filesystem answered with a
skewed timestamp — reads as "does not hold" and the file is removed.
That looks like a fail-open bug on a guard whose whole job is holding
spawns back during an install, and it is not. This branch exists because
an unverifiable pid may be a recycled one, and `GUARD_TTL` is there to
bound how long such a pid may block spawns. An age that cannot be
measured cannot be bounded, so holding would let some unrelated
long-lived process wearing that number stop tty7 spawning for as long as
it runs. Releasing costs one install window in a case that needs a
backwards clock step; holding costs an unbounded block.
Six cases were pinned here and this one was not, which is how it would
get "fixed" into `map_or(true, ..)` by someone reading only the code. Now
it is a case with the reasoning next to it.
Verified as far as this machine allows: the module is `cfg(windows)`, and
a cross-check against that target dies in `aws-lc-sys`'s build script for
want of `windows.h`, so it is CI that will type-check this. The semantics
were confirmed standalone (a future `SystemTime` gives `elapsed().is_ok()
== false`), and rustfmt parses the file on any target.
Auditing the capability strings for one advertised-but-unchecked or
checked-but-unadvertised name — either is a capability that silently
never turns on — turned up one of the first: `stdio-bridge` is put in
every hello and read by nothing, here or in any build that has shipped.
Its only other appearance in the history is a design note deleted a month
ago.
Kept rather than dropped. It costs one string in a hello that already
travels once per host, and leaving it means a client that later wants to
detect the bridge can do so against servers already deployed; removing it
takes that away and gains nothing. What it did cost was the search I just
made, so the comment now says it, and `MACHINE_TREE` says why it is the
one that comes and goes.
The other three are honest: `CONTROL` and `HOST_RPC` are advertised and
read, and `MACHINE_TREE` is advertised only when the peer really keeps a
tree — which is what `tree_sync` gates on.
`confirm_question` answers `Destructive::RewritesHistory` and sends
everything else to the discard wording — "Discard all unstaged and
untracked changes? Staged changes are kept." That sentence is about
files, and it describes nothing that happens when commits go.
`LosesCommits` reaches it. Today that is harmless by accident:
`Reset { Hard }` is answered above by its own question, and the only
other producer is `GitOp::DeleteBranch`, which is fully built in
`ops.rs` — label, argv, validation, tests — and constructed by nothing
in the UI. Every destructive op goes through this one gate, so the day
branch deletion is wired up it inherits a dialog about the working tree,
reassures the reader that "staged changes are kept", and offers Discard
as the button to confirm it with.
A `debug_assert_ne!` on the loss kind rather than on the op, so it also
catches the next op that loses commits, and it names the op in the
message. Nothing changes in release, and nothing changes today in debug
either — verified by driving a `DeleteBranch` through the function, which
trips it with that message.
Left as an assertion rather than new dialog copy: writing a branch-delete
question, in three languages, for an operation that has no UI, is a
feature waiting for its caller to decide what it should say.
`GitOp::destructive` decides whether an operation gets a confirmation
before it runs, and it ended in `_ => return None`. The default of that
catch-all is "needs no confirmation", so a verb added later reaches the
user ungated and takes their work with it — the one direction a safety
gate must not fail in.
A new variant does already break the build, but on `label` above, which
asks a different question and is answered by adding a string. Nothing
then asks about this one, and `destructive_marks_exactly_what_can_lose_work`
cannot: it walks two hand-written lists, so an op in neither passes.
Spelling the safe ones out puts the compile error at the place the answer
belongs. It also says out loud what the `_` was hiding — that `Pull`,
`CherryPick`, `Revert` and a lease-less `Push` were each considered and
found harmless, rather than never considered at all.
No behaviour change: every variant returns exactly what it returned
before. Confirmed the guard fires for the case it exists for — a probe
variant is now an E0004 at `destructive` as well as at `label`.
`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", ... }
l0ng-ai