if cfg!(target_os = "linux") { window.zoom_window() }
else { window.titlebar_double_click() }
reads like a stylistic split and is a bug fix. gpui implements
`titlebar_double_click` on macOS only — everywhere else the trait method
is an empty default, so on Linux this row swallowed the double-click and
nothing zoomed. `zoom_window` is the maximise toggle there, and what
gpui-component's own `TitleBar` calls for the same reason. Windows needs
neither: the row maps to HTCAPTION and the OS has already acted.
Nothing in the code said any of that, so the obvious tidy — collapse the
branch to the one gpui documents — silently breaks Linux again. The
branch is byte-for-byte what it was when that was written.
Also `WindowMoveArm`, whose point is *where* the flag lives rather than
what it holds, and `last_focused`, which is why switching tabs returns
to the pane you left instead of the first one.
Checked `last_focused`'s reference to `remember_active_pane` before
restoring it, on the assumption it had rotted — it has not, the function
is still there and still called from two places, and `focus_target`
still falls back to `first_leaf()` exactly as described. My first grep
said otherwise because I had truncated it.
Measured function length across the tree while here: the only
production function over 400 lines is `Tty7App::render` at 578, which is
a gpui element tree — long, linear, and not something to split while
its behaviour cannot be verified visually.
`pane ls` reports PANE/WS/TAB/CWD/LIVE and `pane ls --all` reports
PANE/WS/OWNER/CWD/LIVE. The swap is right — a pane no workspace holds is
in no tab, so a TAB column would be empty for exactly the panes `--all`
exists to show — but nothing said so, and it is the sort of difference a
reader assumes they misread.
It also costs something specific: the @ numbers. A "no tab @7" error
sends the reader to `tty7 pane ls`, and reaching for `--all` there, as
one does when a thing seems to be missing, takes the column away.
Verified while here, and all correct: `--key C-c` interrupts a running
command (the `sleep` is gone and `procs` shows the shell back in the
foreground); `send` delivers quotes, backslashes, `$`, backticks, braces
and pipes verbatim; `-q` silences all twelve listing and creating verbs,
still prints errors, still exits 1, and wins over `--json` on success.
Same shape as the working directory, and the same surprise:
RALPH_MARKER=hello tty7 run -- sh -c 'echo [$RALPH_MARKER]'
[]
The pane is the server's child, so it gets the server's environment.
`FOO=bar tty7 run -- …` is a universal idiom and it silently does
nothing here, with no `--env` to reach for instead.
Documented beside the `--cwd` note, with the workaround that does work
— set the variable inside the command, where the shell running it can
see it. Adding `--env` would be a new flag, not a fix.
Checked while here, and correct: a pane does get a usable environment
(PATH, HOME) from the server; `$TTY7_PANE`, `$TTY7_WS` and
`$TTY7_CONFIG_DIR` are all set in a `run` pane as the top-level help
promises; and the payoff holds — `tty7 procs` with no address, run
inside a pane, resolves `$TTY7_PANE` and reports that pane's own
processes.
It does not run where you typed it. The pane is the server's child, so
it starts in the server's working directory — whatever that process was
launched from. Measured, not assumed: `tty7 run -- /bin/pwd` from /usr
and again from /tmp both answered with the directory the server had been
started in.
The help's own example is `tty7 run -- cargo test`, and a server the app
started is not sitting in your project. So the example as written builds
somewhere the reader did not choose. It fails loudly for a build — no
Cargo.toml — but not for anything that would happily run in the wrong
tree.
Documenting rather than changing it. Defaulting a local run to the
caller's directory is the behaviour most callers expect and would make
the example true, but it is a different result for every existing
invocation, and `-m` routes to a machine where the local path means
nothing. That is a decision to take deliberately, not a side effect of a
docs pass.
Checked while here, and correct: `split` inherits the split pane's cwd,
and a pane's reported cwd follows the shell through `cd`.
Twelve `tab new` at once against one workspace, then check that every
tab landed exactly once and that the tree's pane count still matches the
registry's.
Both invariants this leans on are invisible to a single-threaded test.
The store takes `notify_order` before its state lock and holds it across
delivery, so subscribers see mutations in the order they happened; and
each `tab new` spawns its pane before the tree is asked to hold it, so a
refusal in between leaves a pane running that nothing references. That
second one is quiet — it shows up only as these two counts disagreeing,
which is what `pane close --orphans` exists to mop up.
Verified by hand first: twelve racing creates, and a mixed race of
splits, tab creates, renames and closes, both left the counts equal with
no orphans; the same held with a GUI mirroring the changes live, whose
log recorded no resync or divergence. This is that check, kept.
Also measured while here, and sound, so it is not re-run: 180 pane
create/close cycles move the daemon from 10 fds and 5 threads to 13 and
7, and then stay there across two further rounds — one-time overhead,
not a leak.
Read as oracles first, on the theory that a comment stating a rule is a
test of whether the code still follows it. All four still do:
- `completion_generation` is the guard against a background generator
landing its results in a session the user has since closed or
replaced. Bumped on open and on close, and checked before the result
is used — `self.completion_generation != generation` is still there.
- `link_modifier_down` exists because mouse events can lag or omit the
modifier while a mouse-tracking TUI is foreground. Both hover sites
still read `mods.secondary() || v.link_modifier_down()`, so neither
depends on the move event's snapshot alone.
- `scroll_frac` is the sub-line part of the scroll position that makes
trackpad scrolling continuous, and is still reset at every path that
jumps the view.
- `scroll_debt` accumulates sub-line wheel deltas instead of rounding
each one away, which is what stops slow scrolling from never moving:
`total - lines` is still how it is spent.
`scroll_debt` was also the target of a doc link from `zoom_debt`
("kept apart from [`scroll_debt`]") pointing at an undocumented field,
so the pair reads as a pair again.
All four re-verified against the code rather than pasted back:
- `notify_order` is a `Mutex<()>` guarding nothing a reader can see, and
it is the reason subscribers apply history in the order it happened:
the state lock orders the mutations, but deltas go out after it is
released, so without this one writer's deltas can overtake another's
and leave every mirror on the losing state with no error to prompt a
re-pull. Taken at line 1067, before the state lock at 1071, and still
held when `notify_all` runs at 1093 — exactly what it claims.
- `MachineStore::open` is infallible on purpose: a machine whose tree
file is missing or unreadable still has to serve panes and files, and
an unparseable one is set aside as `machine.json.corrupt` first.
- `set_liveness_probe` exists because a seed used to enter the registry
`live: true` unconditionally, and a pane that died between its spawn
and its adopting operation had its death dropped — `note_pane_facts`
still returns early for a pane the tree does not hold, so nothing
would ever have flipped the record back.
- `ActiveTabChanged` is emitted for implicit changes too, so a mirroring
client need not re-implement the heal rule. Confirmed at all five
emission sites, and the one case it deliberately omits already has a
test: "losing the last tab needs no ActiveTabChanged".
Also swept for the class found last commit — a reply whose effect dies
with the connection — and found none left: the CLI speaks no watch verb,
and the server's teardown clears watches, releases workspaces and
unregisters the GUI on every exit path, EOF and protocol errors included.
`tty7 ws attach <ws>` answers `{"attached": "<id>", "took_over_from":
null}`, and `tty7 ws ls` shows the workspace unattached a moment later.
Both are correct: an attachment belongs to the connection that made it,
and this command's connection ends with the command.
A caller has no way to tell that from the reply. An agent reading
`attached` reasonably believes it now holds the workspace, and the next
`ws ls` says otherwise with nothing to explain the difference.
So say it. The claim does not outlive the process, that is not a
failure, and what does last is the displacement: the previous holder has
been told, and a dedicated one has been hung up. `took_over_from` is the
part of the reply worth acting on.
Behaviour unchanged — this is the same reasoning the `Attachment` type
already carries ("an attachment belongs to a live connection, and one
read back at boot would name a holder that no longer exists"), said
where the person running the command can see it.
Also checked while auditing, and sound: `tty7 <PATH>` already stats the
directory and refuses a file (`resolve_gui_path`), `ws stop` says "(not
implemented yet)" in its own help rather than only when run, and repeat
`ws detach` is idempotent on purpose for wire-compatibility reasons the
handler documents.
Auditing my own earlier fix, the way the last commit's lesson says to.
`run --cwd` and `new <path>` were fixed together and `tab new --cwd` was
missed, so it went on starting the tab's shell in whatever directory the
CLI happened to be run from and answering with a pane id:
tty7 tab new <ws> --cwd /nonexistent-xyz
%2
with `pane ls` then showing %2 rooted in the CLI's own directory.
The check is one function now, and the three verbs call it, which is
what should have happened when there were two of them. It refuses before
anything is spawned, and only judges paths on this machine — a routed
`--cwd` belongs to the far side's filesystem.
Pulling it into one place dropped the path from the sentence for a
moment ("--cwd: no such directory", naming the flag but not which of the
reader's paths was refused). The flag is optional in the message and the
path never is, since `new <path>` has no flag to name.
The sweep added last commit knew one prefix. `throwaway_dir` hands out
four — zsh's ZDOTDIR, and a scratch directory each for bash, nushell and
WSL — so bash's had gone on piling up untouched the whole time zsh's
were being cleared. There were 607 of them here against zsh's 3,850,
from the same killed daemons.
The prefixes now live in one list that the sweep reads, and the three
call sites take their prefix from it instead of spelling a literal. A
shell added to `setup_*` is swept the moment it takes its directory the
same way, and a prefix defined without being added to the list fails a
test rather than quietly leaking.
Verified by planting a directory for each of the four prefixes under a
pid that is not running, starting a daemon, and finding all four gone.
The living-owner guard still holds.
Every zsh pane gets a throwaway ZDOTDIR under the temp directory, four
redirector files in it, and the pane's own teardown removes it — stop a
server cleanly and it leaves none. A server that is killed never runs
that teardown, and nothing ever goes back to look: this machine had
3,850 of them, from months of crashes and `kill -9`s.
Same shape as the socket a killed daemon used to leave behind, and the
same answer: a later startup is the only thing in a position to notice,
so it sweeps beside the endpoint cleanup.
Timid on purpose. A directory goes only when the name is exactly ours,
the pid in it parses, and that pid is not a live process — so a running
daemon's directories are never touched, and a pid since reused by
something else just waits for another day. Verified after the sweep:
3,850 down to 14, and every one of those 14 belongs to a live process,
the installed tty7.app's daemon among them.
`process_alive` was wrong about that, which the test caught. `kill(pid,
0) == 0` is only half the answer: `EPERM` means the process exists and
belongs to someone else, and reading that as dead would have had the
sweep delete a live owner's directory. Both callers wanted the other
reading — one decides whether to clean up after a daemon, the other
whether to delete its files.
The two tests that bind an endpoint now hold a lock while they do.
`set_config_dir` is first-wins, so every test in the process shares one
socket path, and the one added here made the pair flaky together while
each passed alone.
Log lines read `23:00:24.563` on a machine whose clock said `07:00:24`.
The stamp is seconds since the epoch folded into a day, so it is UTC,
and nothing marked it — the line looked eight hours stale rather than
eight hours offset, which is the wrong conclusion to reach while reading
a log to work out when something happened.
One `Z`. UTC is the right choice for a daemon that outlives sessions and
can be read from anywhere; being unlabelled was the defect.
Still no date, and now the comment says why: turning epoch seconds into
a civil date is calendar arithmetic this crate would have to hand-roll,
and getting that subtly wrong is worse than a reader taking the day from
the file, whose path `server logs` prints directly above the lines.
Verified against a running server: the line now reads `23:02:15.701Z`
with `date -u` at `23:02:17`.
Nothing else came out of this sweep, which is worth recording: the
remaining `(s)` spellings are all either log lines or the *seconds* unit
in `Keepalive interval (s)` — a blind pass over them would have been
wrong. The daemon also survives malformed wire input intact (empty,
garbage, a 4 GB length header, unknown kinds, a 100 KB burst, and 200
junk connections) and leaks no threads doing it.
Three lines still wrote `pane(s)`: the partial-close report, the
hang-up failure, and the orphan note. All three are reachable with a
count of one, and all three are what a reader meets when something has
already gone wrong — a poor moment to look unfinished.
`pane(s)` is the form a codebase uses when it has not decided, and this
one has: the GUI counts through `t_plural` with one/other branches, and
`pane close` narrates a batch only when there is a batch. These were
what was left.
I said in the doctor fix that "1 panes" was the last of these. It was
not — I had swept the success paths and the format strings that spell
the word out, and these spell it `pane(s)`, so the search missed them.
Checked against a live server: "closed 1 pane; 1 could not be closed"
for one, "closed 0 panes" and "2 could not be closed" for the rest.
Following the shape found in the split fix to the other two verbs that
have it. Every verb adding a pane spawns it first and files it second,
because the seed carries the daemon's own pane id — there is no other
order. A refusal in that gap ended the command with a shell running that
no tree referenced: absent from `tty7 ls`, present only in `pane ls
--all`, and collectable only with `pane close --orphans`.
`tab new` and `new <path>` both had it. Confirmed rather than assumed:
the mock answers `TabCreate` with a reply `tab new` cannot read, and the
command left `spawned=1, killed=[]`.
One guard for all three sites now, since the ordering is forced and so
the window is permanent. `new <path>` also takes back the workspace it
made on the way in: it holds nothing, and leaving it adds a row to
`tty7 ls` the caller never asked for.
The mock grows a way to fail a chosen control call. Both cleanup paths
are reachable only after something has already been created, so there
was no way to test them from outside.
Verified against a live server that the ordinary paths are unchanged:
`new`, `tab new` and `split` leave the tree and the registry agreeing at
three panes, and `pane close --orphans` finds nothing.
`tty7 split` spawns the shell and only then asks the tree to hold it, so
any refusal in between leaves a pane running that nothing references —
`pane ls --all` shows it, the tree does not, and `pane close --orphans`
is the only way to be rid of it. Two attempts at `--ratio nan` left two.
NaN is the case that gets there, because it cannot be serialised onto
the control connection at all. The link drops mid-request, so the
daemon's own "a split ratio must be a finite number" never comes back:
the caller sees "control connection lost (request 2)" instead, which
says nothing about the ratio, and the pane is already running.
Refuse a non-finite ratio before the spawn, in the daemon's own terms.
Its clamp to a usable range is deliberate and stays its business — 0,
1 and 5.0 still land on the clamp, as intended; this only refuses what
the daemon would refuse anyway.
Then take the pane back down if the split fails for any other reason
too. Nobody asked for a pane that no tree holds, and leaving it for the
user to find with `--orphans` is not a refusal, it is a mess.
Verified against a live server: two refused splits now leave the pane
count where it was, and a valid `--ratio 0.3` split still works.
Swept the rest of the value-taking flags while here, all sound: `-m` on
an unknown machine, `--key` on an unknown key (which lists the real
ones), `--timeout 0` (exits 124, the conventional code) and a negative
timeout are each refused with the right message.
tty7 run --cwd /nonexistent -- /bin/pwd
/Users/thomas/repo/wt/ralph-wc
Exit 0, no warning, and the command ran wherever the CLI happened to be
started from. `tty7 run --cwd ~/porj -- make` builds the wrong tree and
reports success — and the caller most likely to typo a path is a script,
which has nothing to notice it by.
The daemon falls back to a directory that does resolve when the one it
is handed does not. That is right for a cwd inherited from a pane's OSC
7, which is only as good as the shell that reported it, and wrong for
one typed on the command line: an explicit `--cwd` is an instruction,
and one that cannot be carried out has to stop the run rather than be
quietly replaced. Same for `tty7 new <path>`, which rooted the workspace
in the CLI's directory instead and handed back an id as if it had not.
Both refuse before anything is spawned or created — a refusal after the
spawn leaves an orphan pane, and after `WorkspaceCreate` an empty
workspace to clean up. "No such directory" and "not a directory" are
told apart, because they read differently to whoever typed the path.
Only judged for this machine. A `-m` path belongs to the far side's
filesystem, which this process cannot stat — the reason `agent_hooks_state`
already gives for refusing to answer config questions when routed. Asking
the backend, so the two stay consistent.
The mock now defaults to *not* being this machine. It models a Windows
box (`C:\proj`), so leaving it on made two unrelated spawn-ordering
tests judge Windows paths against the host running the suite. A test
about local paths turns it on and uses paths that exist.
On a config dir it cannot write, `tty7 new` answers:
tty7: Permission denied (os error 13)
and that is the whole of it. The refusal is right and the exit code is
right — nothing is silently lost — but the sentence names neither what
tty7 was doing nor which file it could not write, on a machine where the
config dir is exactly what the reader has to go and fix. Every other
error this CLI prints says what failed and what to try.
The `io::Error` came off `OpenOptions::open` and travelled the wire
unchanged. Name it where the meaning is, keeping the kind so callers
that match on it still can:
tty7: could not write the machine tree at /…/machine.json:
Permission denied (os error 13)
The test skips itself under uid 0, where the mode bits it relies on do
not apply and it would fail for a reason it is not about.
Durability checked while here, and sound, so it is not re-tested: three
workspaces and a rename survive SIGTERM, and survive SIGKILL sent
immediately after the rename — the store persists on the mutation, not
at shutdown.
Six `tty7 server start` at once reported six different pids, five of
them gone by the time they were printed, and every one of them claimed
`"started": true`.
The singleton admits one server and the losers exit immediately — but
they all spawned a child first, and they all then see `running()` go
true, because the winner is up. Each reported the child it had spawned.
`pid` is the field a script keeps in order to watch or stop the server,
so five of six callers were handed a dead one.
Report the pid that is actually serving, read from the pidfile, and say
`started: false` when this call was not the one that started it — the
same answer `start` already gives when a server was up before it ran.
Readable by then: the daemon writes the pidfile after `bind`, and
`running()` needs a request answered, which is later still.
The ordinary path is untouched: a start that wins reports its own child,
because that is the serving pid.
Verified by racing six starts against an isolated config dir — all six
now report the one live pid, exactly one says it started it — and by a
single start, which reports the pid `ps` shows for that config dir.
Also checked while looking for this, and sound: a truncated, garbage,
empty or wrong-schema machine.json is quarantined (`.corrupt`, then
`.corrupt.N`) and the server starts clean; a killed server's pane
children do not leak, since closing the pty master hangs them up.
`kill -9` the server and it never comes back:
tty7-server did not open its endpoints within 10s —
it had already exited with an error
with the real reason only visible by running it by hand:
bind /var/…/T/tty7-<hash>.sock failed: Address already in use
A unix endpoint is a socket *file*. Killed uncleanly, the daemon leaves
it on disk, and `bind` on a path that exists fails instead of replacing
it. The user is stuck until they delete a file out of a temp directory
nothing points them at — after SIGKILL, an OOM kill, or a lost machine.
The startup path already had exactly the right code — probe the
endpoint, refuse if something answers, unlink it if nothing does — and
skipped it whenever the recorded daemon was gone. The comment gives the
reasoning: a dead recorded daemon cannot own a live endpoint, so there
is no point paying the refusal delay, and the bind below "overwrites the
file". That last part holds for a Windows daemon.port, which is a
recorded number; it does not hold for a socket file. So the shortcut
skipped the unlink precisely when the daemon had died the way that
leaves one behind.
Keep the shortcut for the probe, which is what costs, and unlink
whichever way the probe went.
Verified end to end on an isolated config dir: capture the socket path,
kill -9, confirm the file survives, and `server start` now recovers with
no manual cleanup. A live server is still refused and keeps serving —
the singleton seat turns the second one away before it reaches this
code, and only one server is left running.
The test pins the platform fact the fix rests on rather than the fix
itself: bind fails on a leftover socket path, and succeeds once it is
removed.
Found by running every `tty7 …` command the help and README recommend.
`tab ls @7` answered "no workspace named '@7'". `@7` is a well-formed
tab address typed into a *tab* command, and its own siblings `tab close
@7` and `tab rename @7` both answer "no tab @7" — only `ls` takes a
workspace in that slot, so the sigil looked like the problem instead of
the slot. `parse_workspace` accepts any word at all, unlike `parse_pane`
and `parse_tab` which each validate their own sigil, so nothing noticed.
Name it for what it is, `%` included. The hint is only reachable after
the lookup has already failed, so a workspace someone really did name
`@7` is found first and never sees it.
Then, checking my own new message did not recommend a dead end — the
mistake this same sweep caught last time — it did: `tty7 ls` renders
WORKSPACE/NAME/TABS/PANES/ATTACHED and has no @ column at all, so it
cannot answer "which workspace holds @7". The pre-existing "no tab @7 —
`tty7 ls` shows the @ numbers" was wrong the same way, and has been
since before this branch.
`tty7 pane ls` is the listing that actually carries panes, their tabs'
@ numbers and the workspace holding them, so all three now point there.
Verified against a live server: every message, and the command each one
recommends.
Also checked and found sound, so it is not re-swept: --json is a single
valid object for all ten listing verbs, errors are empty stdout plus a
message on stderr with rc=1, and a mistyped subcommand is named as one
rather than offered to the GUI (deliberate, and tested).
`tty7 wait`'s headline example is
tty7 wait %3 && tty7 capture %3 --plain
and `exit` is one of the three states `--until` waits for by default. Run
it that way and the pane is gone by the time capture runs: wait reports
`exit` and succeeds, `&&` proceeds, and capture answers that nothing is
running. An agent following the documented pattern gets no output for a
command that produced some.
The example is right for an agent — `waiting` and `done` leave the pane
alive. It is wrong for a plain command, which finishes by the shell
exiting. Spell both out in `wait`'s long help, and say plainly that
`exit` is the end state with nothing left to read.
Verified against a live server: `wait --until free` blocks for the
command (5s for a 4s sleep), and the capture after it finds the marker.
Also: a server running one pane had `doctor` say "1 panes" on its first
line. It is the first thing a new user runs. Every other count in the
tree is already plural-aware — the GUI routes these through `t_plural`
with "one"/"other" branches, and the CLI's other count (`closed N panes`)
handles the single case separately — so this was the last one.
Found by running the CLI rather than reading it. `tty7 run --keep` files
its pane into a workspace and the tree keeps that leaf after the command
exits — `tab ls --json` goes on reporting `panes: [31]`. But every
`%PANE` verb answered:
no pane %31 on this machine — `tty7 pane ls --all` lists them
which is wrong twice. The pane *is* on this machine. And `pane ls --all`
is documented as "every pane the server runs", so the command it sends
you to is the one guaranteed never to show this pane — the user follows
the advice, sees nothing, and has no next move.
`or_no_such_pane` consulted only the running registry, which cannot tell
"never existed" from "not running". It now asks the tree once it already
knows the pane is not running, and answers accordingly:
pane %31 is not running — ivory-heron still holds it, so there is
nothing to read or type into. `tty7 tab ls 1790a260` shows it;
opening the workspace in the GUI revives it.
The tree is fetched only on a path that has already failed, so no
ordinary call pays for it, and an unreadable tree falls back to the
plain sentence. A genuinely absent pane is unchanged.
Verified against a live daemon on an isolated config dir: both messages,
both exit 1, and the recommended `tab ls` really does list the pane —
having just fixed one dead-end recommendation, I checked the new one.
Third pass over #268, filtered to the claims that bind two places
together ("the only place that…", "keep in sync with…"). Those are the
ones 640 commits can silently break, unlike a comment that only
describes the line under it. 36 such blocks are still missing; these
five are checked and hold:
- `PaneRoute::for_workspace` is infallible on purpose — its callers (a
close, a restore probe) have nowhere to put an error, and `Unroutable`
is the safe answer where the local daemon would be the wrong one.
- `SshManager::probes` caches a `None` as firmly as a hit, so a host
with nothing to inject is not re-probed per tab.
- `identifier_could_match` is prefix-matching before the `;` and exact
after it, which is what keeps OSC `77;` from matching `777`.
- The `/proc/<pid>/cwd` reading is rejected unless it still stats as a
directory: a deleted cwd reads back as `<path> (deleted)`, and that is
broadcast now, so it would aim every new tab and split at it.
- `GRID_PAD_X/Y` are the single source of truth for the surface inset
that grid-aligned overlays offset by.
The `run` half of install's probe-cost invariant also still holds — it
derives usability from `stat` mode bits and never spends a probe on a
hit — so it needed no change.
`GRID_PAD` is restored with its claim corrected: the comment asked the
reader to keep `render`'s `.px()/.py()` in sync by hand, but render has
since been changed to apply the constants themselves. Telling anyone to
hand-sync that now would be inviting the drift it warns about.
Second pass over what #268 stripped, this time reading each comment as
a claim to check rather than prose to paste back. All eight hold, so
this is documentation only — but they were worth checking, and two of
them are the kind that gets "simplified" by someone who cannot see why:
- `OutputGate::queued` is `AtomicI64` because add runs per PTY read at
~100k/s and sub per socket write, so neither may take a lock; signed
so a decrement racing a reset drifts negative instead of underflowing.
- `wait_below_high_water` returns before touching the mutex when the
backlog is under the mark, which is the common path.
- `MAX_RING_SEGMENTS` bounds a leak: drag-resize cuts a segment per
column, none of them filling `RING_CAP`, so attach would degrade
linearly over a pane's life. Past the cap the two oldest merge.
- `CAPABILITY_ENV` keeps `TERM`/`COLORTERM` out of reach of the user's
env map — a fact about what the far end decodes, not a preference.
- `ForegroundProbes::agent` and `::cwd` distinguish "no reading" from
"nothing there"; the caller applies the outer Option, never flattens
it, so a backend with no process-table view cannot wipe an agent
identified by sentinel events.
- The spawn path avoids appending argv to `new_default_prog()`, which
portable-pty panics on by design.
Also fix an unresolved `[`ForwardEntry`]` link I added in 61002ec. It
only fails under `--document-private-items`; plain `cargo doc --no-deps`
never resolves private-item links, so it is the wrong gate for this
tree and I have moved to the stricter one.
#268 ("strip every comment from the Rust sources") removed 38,164
comment lines. Much has been rewritten since, but 7,587 blocks still sit
in front of code that is unchanged and now undocumented. Last commit I
started implementing a pair of deliberate no-op stubs before `git log -L`
turned up the stripped comment saying they were deliberate — which is
the failure mode this class of loss produces.
Seven restored here, chosen for documenting why something must not
change rather than what it does:
- `LinkShutdown` — "not optional politeness; without it a client cannot
be closed". A reader parked in a blocking read is not woken by any
flag, so closing has to act on the fd, and no std trait spans the
transports. Nothing else says this.
- `peek_frame_kind` / `is_error_kind` — why Attach is classified before
the payload is paid for.
- `FontFeatures` — a frozen config key deliberately replicating
gpui's type so tty7-core parses without linking gpui.
- `desired_tabs` / `every_leaf_is_native_ssh` — held vs
permanently-invisible tabs, and why conflating them either deletes a
daemon tab mid-revival or freezes a window's ordering forever.
- `control_for` / `TreeLink` / `classify_tree_link` — local and remote
links unified, and unserved as a fact about the peer rather than a
transient down.
Restored against the current code, not verbatim: three claims had gone
stale and are corrected. `Duplex` has since adopted `LinkShutdown`
(`Halves` carries one) rather than needing to; `peek_frame_kind` has two
callers now, not one; and `gpui_font_features` moved out of `ui::app`.
src/terminal/remote.rs opened with a blanket `#![allow(dead_code)]` —
every other suppression in the tree is per-item or cfg_attr'd. It was
hiding nine unused associated functions, and four of them are the sort
worth catching: `spawn`, `attach`, `list_panes` and `kill_pane`, the
local-only wrappers left behind when panes became route-addressed. Each
hardcodes `PaneRoute::Local`, so reaching for one instead of its `_on`
sibling silently talks to the local daemon whatever machine owns the
pane — the exact mistake the route argument exists to prevent. Better
they not be there to reach for.
The rest: `on_workspace_forwards`, a swallowing copy of what
`ForwardRoute::forwards` now returns as Option, and the four known-host
and loopback-forward wrappers no UI ever called.
Also restore, on the daemon side, the rationale that #268 stripped from
the two loopback stubs. Without it they read as an unfinished feature —
they are not, and the comment saying so is what stops the next reader
(this one included) from "finishing" them.
Windows CI is the check that cannot run here: the two platform-gated
regions in this file are self-contained, so nothing ungated is left
stranded, but only a Windows build proves no new dead_code warning.
A workspace's forwards are released by exactly one thing: the
TeardownForwards request the GUI sends as the workspace closes. Nothing
daemon-side reaps them, unlike a pane's, which DaemonPane::drop always
takes down. So the request failing is the case that leaves ports bound
for the rest of the daemon's life.
It was also the case nobody could see. ForwardRoute::teardown answered
Vec::new() both when the route could not be addressed and when the
request went unanswered, and the caller's only diagnostic fired on
`!left.is_empty()` — so the one path that partly worked warned, and all
three that did nothing at all stayed silent.
Hand back Option, as `forwards` already does one method up, for the
reason its doc comment already gives: an empty list is a far side with
nothing left, and a failed request says nothing about what it still
holds. Warn when no pane is left to address the workspace through, and
when the teardown is not delivered.
This makes the leak observable; it does not close it. A GUI that is
killed rather than closed still never sends the request.
`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`.
l0ng-ai