`settle_census` and `sweep_parked` each built `parked ∪ spawned` from the
state, separately. I introduced that duplication myself when I added the
first of them earlier on this branch, by copying the shape of the second.
They are not interchangeable readings. One decides whether the tree is
worth pulling, the other which panes to end — so drift means either
ending a pane the census never weighed, or pulling for one the sweep will
not judge. `census_of` is now the single answer to "what is this window
still holding".
Which also made the gap visible. Mutating the census showed the `spawned`
half held by nothing: `the_census_holds_the_panes_this_window_made` read
`s.spawned` straight off the state, so it proved the set was filled in
and never that the decision reads it. Dropping `spawned` from the census
left the test green while a pane spawned and not yet in the tree went
back to being invisible to the sweep — which is the whole of the stray
shell leak that set exists to close.
It asserts through `census_of` now, on both halves, and each fails when
dropped. `stranded_of`'s two conditions were already held.
Mutating `discard_all_ops` left two conditions standing.
The `-d` on `git clean`. Porcelain v2 names an untracked directory with a
trailing slash, and that slash is the whole of how this decides whether
to recurse. Measured against real git: `clean -f` removes loose.txt and
leaves newdir/ standing; `clean -fd` takes both. Without the flag,
"Discard all" quietly leaves every untracked directory where it was —
not what the button says, and not what its own confirmation described.
And the filter that drops a path git cannot be given. A name that is not
UTF-8 has no pathspec, and `GitOp::validate` rejects an *operation*
carrying one — so leaving it in the list turns "discard everything" into
an error that discards nothing, on a repository that happens to hold one
awkward filename. Dropping it there keeps the rest discardable, and the
test asserts the ops built are ones validate accepts.
The third condition — that discard-all reaches only unstaged paths — was
already held. So were all three of `orphan_panes_of`, which decides which
panes a card offers to close.
expand_file_command_template splits the template into tokens first and
substitutes {path} inside a token, which is what keeps
`/Users/me/My Notes/a.rs` together. Substituting first and splitting
after reads like the same operation and is not — the editor is handed
`/Users/me/My` and `Notes/a.rs` and opens neither.
Nothing pinned that order, and every existing case used a path without a
space, so the invariant that makes the design correct was never exercised.
Paths with spaces are the common case on the platform this ships on:
Application Support, My Documents.
Three shapes: the plain `{path}`, a compound `{path}:{line}:{column}` where
the glue would part company with it too, and a template carrying spaces of
its own, which are still separators — only the substituted value is
protected.
Checked by writing the refactor it guards against: the path comes back as
three arguments.
`in_group` decides which of Staged, Changes, Untracked and Merge a file
is drawn under, and every test of it was against records this file wrote
— which checks the mapping against our reading of the porcelain rather
than against git. The two rows a mistake there produces are a Stage
button on a file with nothing staged, and a Discard button on a file
whose only change is already in the index.
So the same tree described twice: once by `probe_status`, once by the
plumbing each group mirrors — `diff --cached --name-only`,
`diff --name-only`, `ls-files --others --exclude-standard`. They agree,
including the two cases worth arranging on purpose: a file staged and
then edited again belongs to Staged *and* Changes, and a deletion that
was never staged belongs to Changes.
Two ways this could have passed without proving anything, both closed:
every group is asserted non-empty, so agreement between two empty sets
does not count; and the run was checked to reach its assertions rather
than take the no-git early return.
`quote_for_shell` decides how a path is typed into the user's own shell,
so a name that argues with the quoting is a path that names the wrong
file. Posix and PowerShell had tests; cmd had none for quoting at all —
only `unquote_word` and `quoting_for` — so what it does with an awkward
name was written down nowhere.
The `"` case is genuinely closed and the doc already said why: a Windows
path cannot hold one. `%` is the open one. It is legal in a Windows
filename and cmd expands `%NAME%` inside double quotes, before it parses
them, so a file actually called `%USERPROFILE%.txt` pastes as a line
naming somewhere else. There is no escape for it on an interactive line —
`%%` is a batch-file rule and `^` is not read inside quotes — so this is
pinned and documented rather than fixed, and the doc now says what it
costs: the wrong path, never a command, because `%` substitutes an
environment variable and can do nothing else.
The rest is the coverage cmd was missing, plus a check that `is_bare`
never lets a metacharacter through unquoted — which is the actual
boundary, since anything bare is typed with no quoting whatsoever.
`SCM_KEYS_AWAITING_A_CALLER` says it out loud: "delete a key from this
list as soon as something renders it". `ScmCheckoutBranch` had been the
placeholder of the inline "switch to which branch" input for some time
and was still on the list.
That is not a warning anyone sees, but it spends the list's whole purpose
in reverse. It exists so `dead_code` keeps reporting on the rest of the
enum and a genuinely stale key cannot hide in a crowd of unused ones; a
key that is in use makes the list an inventory of nothing in particular.
The other five are real — commit detail, staged count, reset to commit,
branch search, stash and switch are all unbuilt.
The guard reads the sources rather than trusting the list, because
trusting the list is what failed. It fails with the offending key named,
which the instruction alone could not do.
`run_scm_action` opened with a guard that returned when the panel had no
active repository, and said nothing. Every Git verb goes through it:
commit, push, pull, sync, fetch, stage all, discard all, checkout,
refresh.
The panel's tiles and the Git menu disable themselves in that state, so
the mouse never reaches this. The key bindings and the palette do — and
the palette lists every Git command whatever the pane's directory is,
which `palette.rs` pins on purpose: "every Git action you can bind is
also a palette command". So in a directory that is not a repository,
choosing Git: Commit did nothing and reported nothing.
That is the case `scm_push` already argues about its own guard one level
down, citing the same issue: a swallowed click looks exactly like an
action that finished instantly, and the toast is the only place a key
binding or a palette entry can say why nothing moved (#545). The message
is `DiffNotARepo`, which already existed in all three locales, so the
wording matches what the diff overlay says about the same state.
Sparing them is the right bargain and it is argued for in place: one
dialog per tab is not a question anyone can answer, so "Close Other Tabs"
skips what it will not take — a tab holding unsaved edits, or an SSH
profile that asked to be warned about — and closes the rest.
It did that silently. Choosing Close Other Tabs on a window with three
such tabs left three tabs standing and said nothing, which reads as the
menu item having half worked, and gives the reader nothing to act on.
This is the rule `scm_push` already states about its own swallowed click —
"a swallowed click on Push looks exactly like a push that finished
instantly" — and a close that keeps something back is one of those. Both
bulk closes now count what they spared and name the number, in all three
locales; a close that took everything still says nothing, because there
is nothing to explain.
`write_clipboard_image` did `create_dir_all($TMPDIR/tty7-clipboard)` and
wrote into whatever that turned out to be. On macOS $TMPDIR is a
per-user /var/folders path and this is moot. On Linux it is /tmp, mode
1777: the directory lands 0755 under the stock umask, so any other
account can read the screenshots pasted through it — or create the name
first and be handed everything pasted afterwards, which is also a place
to plant a symlink and have the write land somewhere else.
The remote half of this feature already defends all of it, and says so:
"a staging directory anyone else can enter is one anyone else can read
the pasted screenshots out of", and `staging_dir_is_safe` proves
ownership by chmodding to 0700 and reading the mode back — POSIX lets
only the owner change a mode. The local half, 200 lines up, did nothing.
Same proof, same predicate shape, and the same refusal of symlinks. The
ordering is the part worth stating: `set_permissions` follows links, so
the symlink test has to come before the chmod, not after it. My first
attempt had it after, and re-permissioned the link's target on the way to
refusing it — the test caught that, and pins it now.
Failing closed was already safe: `None` here returns false and the caller
forwards the paste the ordinary way.
It holds `history` — every command with its cwd and exit status — plus
the SSH profiles in `config.json`, `session.json` and `views.json`.
`machine.json` and `appearance.json` are written 0600 and the sockets
are 0600, but those four are not, and the directory around them was made
with plain `create_dir_all`: 0755 under the stock umask of 022, with the
history file 0644 inside it. Another account on the machine could read
the lot.
The rule already existed twice. `daemon::history` closes its own
subdirectory, saying "closing the directory is what makes the mode of
what is inside it moot", and `transport::bind` closes the socket's parent
when it is the config dir. Neither covered the directory holding
everything else, and the daemon reaches it first through the pidfile, the
singleton lock and the TCP endpoint, none of which closed anything.
`ensure_private_dir` closes every directory the call creates, and the
config directory itself even when it already existed — so an install made
by an earlier build is repaired rather than left open for its lifetime.
Directories it did not create and that are not ours are left alone, which
is the distinction `transport::bind` drew with `owns_parent`: $HOME and
~/.config are on this walk on a first run.
Verified against a running daemon rather than only in a test: with umask
022 a fresh config dir came out drwxr-xr-x before and drwx------ after,
and a directory chmodded back to 755 was closed again on the next start.
The unit test caught a hole in the first attempt — closing only the leaf
left the config directory, which `create_dir_all` had just made, exactly
as open as before.
Both thread pools ran `job()` bare in their worker loop. A panic unwinds
out of `worker`, which skips the `threads -= 1` / `workers -= 1` that
every deliberate exit performs, so the count kept believing in a thread
that was gone.
`wants_another_thread` is `jobs > idle && threads < MAX`. After MAX
panics the count sits at the ceiling with nothing alive behind it: the
pool spawns no more workers and no worker is left to wake, so every later
job is queued and never run. On the UI side that stops the file tree, git
status, saving a file, SFTP and the diffs together; on the daemon side
the connection accepts requests and answers none. Both are silent, both
persist until a restart, and a daemon lives for days.
A host op runs somebody else's code — a git parse, an SFTP read, a
filesystem walk — so a panic there is exactly the event the pools should
survive. It is also what poisons the mutexes both of them already take
poison-tolerantly: the poison was handled, the accounting was not.
Each pool has a test that exhausts its ceiling, because counting workers
cannot distinguish one the pool believes in and has from one it believes
in and has lost. The op itself is still dropped — `off_thread`'s sender
drops unsent, so the landing never runs and the caller's in-flight flag
stays set. That costs one operation rather than every one, and landing
would need the value the panic is the reason we do not have.
`assign_keybinding` used `find`, so it unbound the first holder and left
any others. A chord can legitimately be held twice — `secondary-enter`
is Fullscreen in the window and Commit inside the commit box, which
`binding_conflicts` blesses because their scopes differ — and on macOS
that is a shipped default.
Rebinding it therefore left the new action sharing the chord with one
the note had not named. Which holder survived came down to their order
in `default_bindings`: Fullscreen sits at line 475 and Commit at 560, so
the right one was displaced by table position rather than by design, and
reordering the table would have silently swapped it.
Scope is still not consulted here, unlike in the conflict check. That is
the tested intent — a user who assigns a chord means it to be theirs
everywhere, which recording_an_extra_default_chord_displaces_its_owner
pins — so the fix is to displace every holder and name every one of them
in the note, not to start honouring scope.
The tree's rename / new-file / new-folder box subscribed to PressEnter
and Blur and no other key. So the way out of an edit you had changed
your mind about was to click somewhere else, and the other key already
under your hands — Return — commits the rename instead of abandoning it.
Every other box the app opens pairs Escape with Return: sftp_open_edit
says so out loud ("every other box in the app opens focused and answers
Return"), and the switcher, the branch inputs and the graph search all
handle both. The local file tree, the surface people touch most, was the
one that did not.
Both rows an edit can be drawn in are wired, through one free function
rather than a closure each, so they cannot drift apart on which keys
they answer — a test per row, each failing when only its own site loses
the handler.
Same rule, same reason, the surface next door. A branch name, a machine
label, a settings source path, an agent's display name, and the path out
of a terminal hyperlink — which is whatever the program writing to your
terminal chose to emit — all went into a notification raw.
An error message is deliberately left alone. In a toast the error *is*
the content, not a fragment inside a sentence of ours, and git and ssh
write genuinely multi-line errors whose second line is the useful one.
A dialog embeds one mid-question, so there it is still folded; the guard
now carries a different key list per surface and says why.
`terminal::view::one_line` states the rule and gives the reason:
"anything that draws a name it did not compose is exposed to it." Every
row surface followed it. No dialog did.
sftp.rs held both halves fifteen lines apart — the row folded
`entry.name` under a comment about bytes chosen on a machine this window
has no say over, and the delete confirmation for the same entry
interpolated it raw. A dialog is the worse place to lose it: gpui breaks
text on a newline whatever the style says, NSAlert renders one too, and
the dialog is where the destructive action gets authorised. A file named
`notes.txt\n\nThis one is safe to delete.` wrote its own second line into
the question.
Seventeen substitutions across eleven dialogs: delete (file tree, SFTP,
settings), discard changes, replace-on-drop, remove worktree, close
window, and the unsaved-edits prompts on close, quit and relaunch. Also
the machine label and the far end's error text in the remote dialogs,
which are no more ours than a hostname is.
The guard in ui::tests walks the prompt call sites rather than trusting
this sweep to have been complete — it found five of the seventeen after
I thought I was done.
`scm_commit_landed` clears `scm.drafts` — the commit message the user
typed. Two of the three conditions guarding that were held by nothing:
dropping the repo check let a commit in one working tree throw away
another's draft, and dropping the message check discarded text the user
had edited while the commit was in flight.
Both mutations now fail the suite. The third condition, waiting for HEAD
to move, was already covered — it is the one that keeps a message in the
box when a hook rejects the commit.
`remote_strip_action` exists because of a bug its own comment records: the
label and the action used to be decided separately, so a `ServerMismatch` —
the one state a retry cannot fix — wore a Retry Now button and looped on it
forever. Nothing tested it, so the regression could return in silence.
Three mutations left the suite green: making every state return `Retry`,
dropping the `hosts_our_server` check so a peer somebody else runs offers an
Update Server button that cannot work, and giving a state with no label a
button anyway.
Held now on all four answers — the install for a machine whose server is
ours, a retry for the states a retry fixes, and no button at all for
`Attached`, `RouteLost`, or a server that is not ours to install.
`can_spawn_locally` is one line over `WorkspaceStore::host_of` and nothing
called it. Inverting it left the suite green, as did making `spawn_host`
answer `LOCAL` for every workspace — so a window bound to a build box would
have started its pane on the laptop, which is the same class of mistake as
deleting a path on the wrong machine.
`host_for` underneath is well covered; it was the wrapper the decision
actually goes through that nothing exercised. Held now both ways: a window
with no remote may spawn here, the same window bound to a machine may not.
`install_phase_caption` is shared by the switcher's progress bar and the
strip's, precisely so a user watching both is not told two different things —
which also means a wrong caption is wrong in two places at once.
A server that sends no `content-length` gives `total: None`. Rendering that
through the with-total string reads "12 MB / 12 MB" while the transfer is
still running: it claims the download has finished, and the bar beside it
disagrees. Nothing failed when the two arms were swapped.
Held now across all four phases — no fraction when the size is unknown, one
when it is, an upload always knowing its total, and restarting having no
fraction at all.
No gap here — the property is already protected. This adds the assertions the
existing test cannot make, and records why.
`a_metacharacter_inside_quotes_does_not_start_a_new_command` asks
`at_command_position`, which is false whenever any non-whitespace sits between
the boundary and the word. A closing quote is non-whitespace, so that test
holds identically whether or not quoting suppresses a separator. The case that
separates the two readings is a separator inside an *unterminated* quote with
only space after it — `echo "a| ` and Tab — where reading the `|` as a pipe
fills the menu with every binary on PATH instead of the argument's files.
`segment_start` is now asserted on its own: real separators outside quotes,
the same characters inside terminated and unterminated quotes, an escaped one,
a backslash inside single quotes being literal so the quote closes at it, and
a closed quote no longer protecting what follows.
`lane_centres_rise_and_land_on_device_pixels` checks four scales and cannot
see this: with `GRAPH_PAD_L` at 6 and `GRAPH_LANE_W` at 12 every centre is
already `12 + 12·column`, an integer at 1x, 1.25x, 2x and 3x alike. Deleting
the whole body of `snap` left that test green.
So the rounding is insurance against a constant that stops dividing evenly —
change either to a half-pixel value and it starts carrying the lane strip, and
its own comment says a column of lines that changes width as it scrolls is the
most visible artefact this element can produce. Insurance that no test can
see is insurance somebody deletes.
Held on its own terms now: a centre between device pixels moves to one, a
centre already on one does not, the rounding is to the device grid rather
than the logical one, and a nonsense scale returns the value rather than NaN
geometry.
This codebase folds control characters in three deliberate places, each with
its reason written down: workspace and tab names at the machine store so no
two drawers disagree, an OSC title at the daemon so one line goes in and one
line is stored, and a *filename* at the point of drawing, because the raw
bytes are what `join` and `rename` are handed.
The sidebar draws two things in that third category and folded neither: the
cwd under a tab row, and the section headers built from repo roots and path
components. `mkdir $'a\nb'` is a directory somebody can be sitting in, and
gpui breaks a label on a newline whatever its wrapping says — the row grows
and paints over what is under it, which is the failure the rule's own comment
describes.
The grouping key is deliberately left raw: it is a path, and the sections are
grouped by comparing it. The test pins that too, since folding the key would
silently split one project into two.
The same bug as the delete, one entry point over, and a lexical search for it
missed this one: `file_tree_copy_into` looks the host up itself, and the
drop-replace path calls it *after* asking whether to replace. A workspace
repointed while that prompt is up would have put the files on the machine the
window had by then.
The host is now an argument, so the question of which machine cannot be
answered late: the caller decides before it asks, and the direct drop — which
never awaits — passes what it already had.
Checked by walking every awaiting closure in the file: none look up a host
after an await now. `sftp.rs` had the right shape all along, verifying its
pane is still the open one before acting; it is the model for this.
`active_host` is the host of the *window's workspace*, and a workspace can be
repointed at another machine while a prompt is up — a reconnect landing is
enough. Reading it after the answer meant the path the user was shown could be
deleted on whichever machine the window had by then.
`editor_save_file` already states the rule and the reason: the operation
belongs to the host it was asked about, however the window has moved since.
This was the one place that read the host after an await; every other
`active_host` in the file tree is synchronous.
Delete is the tree's one destructive action and had no test at all — the
prompt's wording was corrected earlier this session with nothing checking the
prompt is even reached. There is one now, both answers, and it watches the
file rather than assuming the removal lands in a single turn: it goes out on
a blocking pool that `run_until_parked` does not wait for, which is what made
the first version of the test fail. Checked by dropping the yes/no guard,
which makes the cancelled half delete.
The rustdoc gate is one this branch put in CI, and this branch had left it
failing — which is the worst state for a gate to be in, since it reads as
enforced and enforces nothing.
`displayed_registry` was the lower-case spelling of `DisplayedRegistry`, so
it resolves now. `PIP_SIZE` names nothing anywhere in the tree; rather than
guess which constant it used to be, the sentence now points at the rem-sized
constants it was contrasting against, which is the part that was actually
true.
All six CI gates now pass as CI runs them: fmt, host boundary, clippy with
`-D warnings`, rustdoc with `-D warnings`, the suite under `--locked`, and
the updater's own feature build.
CI runs `cargo fmt --check` and I had not run it once across this branch,
while making most edits by inserting text rather than writing it. 27 files
were non-conformant; `origin/main` is clean, so all of it is mine and CI
would have failed on the first push.
No behaviour change — the suite is identical either side of it. Also checked
clippy the way CI does, `--locked --workspace --all-targets -D warnings`,
which is stricter than the invocation I had been using.
The last way out of the app that had not learned about unwritten buffers.
⌘Q, the window close and the update relaunch each ask; the tray's Quit and
Stop asked its own question — every shell ends — and never mentioned them.
Worse than silence, its body reassures: "your tabs and layout reopen with
fresh shells next launch". They do. A buffer nobody has written down does
not, so a reader who accepts on the strength of that sentence loses something
the sentence implied was safe. It is the delete prompt's mistake in another
place: precise about what survives, quiet about what does not.
Named in the body rather than raised as a second dialog. They are already
being asked one question about what they are about to lose, and that answer
should account for all of it.
The Known section described a leak that has since been fixed, which makes it
worse than no entry: it tells a reader to expect a bug that is not there and
to reach for a recovery command they do not need. It is replaced by the fix.
Six more user-visible changes had landed without an entry — the reaper that
could take a live session mid-restore, the three ways out of the app that
discarded unwritten editor buffers, a saved file losing the line endings it
came with, a delete prompt implying a trash that does not exist, agent hooks
broken by a `$` in the install path, and one pane's panic costing every pane
on the machine.
Also corrects an issue reference I had wrong in both the changelog and a
comment: #672 is the rebuild that deleted tabs off the machine, not the
tab-close prompt for unsaved edits, which cites nothing.
The mirror had no test for `RatioChanged` at all. Writing a fixed 0.5 instead
of the ratio the delta carries passed the suite, and so did answering `true`
for a path that names no split.
The second is the worse half. That return value is how `apply_delta` tells
its caller the mirror is still in step with the machine; a `true` it has not
earned is a divergence nobody notices, and the re-pull that would have
repaired it never happens.
Pinned against a tree with a split inside a split, so "the right node" is a
claim the test can actually make: the inner ratio moves and the outer one
does not, an empty path is the root, and a path ending on a leaf, a path
running off the end, and an unknown tab each write nothing and say so.
The mirror exists to be identical to the machine's tree, and `tree_sync`
diffs a window against it to decide what to push back — so order is part of
that identity, not a presentation detail. A mirror that agrees on which tabs
exist but not on their sequence makes the next diff propose moves nobody
asked for.
Three of its rules turned out to be unverified. `TabCreated` could ignore its
index and append, `TabMoved` could land a slot late, and `TabClosed` could
leave `active_tab` naming a tab it had just removed — each passed the suite
untouched. The last is the worst of the three: `active_tab` is an id, so a
stale one points at nothing, and emptying the workspace is the one case with
no other tab to fall back to.
Clamping past the end and the two not-found paths are pinned along with them,
since those are the arms that decide between doing nothing and panicking.
The card's orphan list and the CLI reaper answer the same question for the
same user, and after the reaper learned to skip panes a client is watching
they would have answered it differently — the card offering to close panes
the command leaves alone.
`attached` is not redundant with either of the card's existing tests. The
tree holds nothing for a pane a window has spawned and not yet filed, and the
window's own answer counts a slot that is still connecting, which the daemon
has nobody attached to yet. Three questions, none of which subsumes the
others, and the list is what is left after all three.
The reaper's test was "the registry is running it and no workspace holds
it". A window restoring a layout spawns each pane, attaches to it, and only
then files it into the tree — so for a moment every pane it is adopting
answers to that description.
Not theoretical. Polling `pane ls --all` through a cold start of a seven-tab
window reported one, then two, then three, then six, then seven live panes as
held by no workspace, and the restore afterwards was correct: all seven were
wanted. `--orphans` at any point in that window takes the whole session, and
a script that reaps on a timer will eventually sit in it.
Attachment is the missing half, and it is the daemon's own fact rather than a
guess about timing: `attach` takes the pane's seat and the connection closing
calls `detach`, which clears it. A window adopting a pane is attached to it.
A pane whose layout was thrown away had its view dropped, which closed the
connection, which emptied the seat. Nothing else in the registry tells the
two apart, which is why the GUI had to ask its windows and the CLI could not.
`pane ls --all` counts the same set, because the line it prints tells the
reader to run the reaper.
Same race after the change: zero false positives across 300 polls, restore
intact. And a stray with nobody attached is still reaped — checked by
deleting the new filter, which fails both tests.
The switcher's orphan section asked one question — is this pane alive, and
does the local machine mirror hold it — and put everything else under a
button that hangs it up.
The mirror is the wrong sole authority for that, and `tree_sync` says why in
as many words: a pane a window has spawned and registered but not yet filed
is held by nothing in the tree at that instant, and a client is left out of
the deltas its own operations raise (#612). `hang_up_detached` and
`sweep_parked` both refuse to end a pane any window is showing, whatever the
tree says, because a leaked shell is recoverable and a shell killed under a
live window is not. This list did the same thing to the same panes and never
asked.
So it asks now. A window that will not answer subtracts nothing, which keeps
the list at worst what it has always been — it is the recovery tool for the
case where things have already gone wrong, so it must not empty itself when
a window is busy. And it can only ever shrink: a genuine stray is shown by
no window, so nothing that belongs on the list leaves it.
Three places spend `side_panel_max` on `clamp(own_floor, cap)` — the sidebar
and right-panel widths and both drag handlers — and `f32::clamp` panics when
its low bound is above its high one. A cap below the floor is therefore not a
layout glitch but the render path going down, on the frame a window happens
to get narrow enough.
The existing tests pin the answer at three widths. This pins the property
across every floor the callers pass and the degenerate widths a window
reports while it is being made or taken apart — zero, negative, infinite,
NaN — and makes the same clamp the callers make, so the test fails the way
they would.
Dropping the `.max(own_floor)` that guarantees it reports a cap of -360
against a floor of 180.
No behaviour change; the guarantee was already there and already relied on.
The last path in the family. An update relaunch is a quit with a restart
attached, and a code panel's buffers survive it no better than they survive
⌘Q — the layout comes back, the text does not.
The question has to be asked before `pending.launch()`, not at the `cx.quit()`
after it: past the launch the updater is already waiting on this process to
exit, so refusing there would hang the update rather than protect anything.
`core::update` knows about downloads and signatures and deliberately not
about windows, and the question needs a window to be asked in. So it exposes
a hook and the UI installs one, handing the launch itself over as the
continuation rather than returning a verdict, which is what lets the answer
arrive late the way every other prompt here does.
The hook is optional so a mistake in it can only fail to ask, never fail to
update — which also means an uninstalled guard looks exactly like a working
one until a relaunch quietly takes somebody's work, so there is a test that
startup claims it.
The window close learned this one commit ago; quit never knew it. `Quit` was
`cx.on_action(|_, cx| cx.quit())` and nothing more — the shortest path in the
product to losing text that cannot be got back, and the one most likely to
be pressed out of habit.
`on_app_quit` cannot be the guard: it hands back a future and gpui does not
let it refuse, so all it can do is save the session on the way out, which is
what it already does. The decision has to sit at the action.
Every window is asked, not the frontmost one, because quitting takes them
all — and the prompt is raised in front of the window the buffer is actually
in, with its tab brought forward, so the question is about something the
user can see. Answering it quits everything, which is what was asked for.
Only unwritten buffers stop it. The shells survive a quit-to-tray and belong
to the daemon anyway, so warning about those would be warning about nothing
— the same line drawn for the window close.
Verified the search really does walk past the first window: restricting it
to one window fails the test with the buffer in the second.
Closing a tab has asked since #672, and the reason given there was that
unsaved text is the one loss in this product that cannot be undone by doing
the thing again. Closing the *window* asked nothing.
It reads like the safe exit, and for everything else it is: the shells are
the daemon's and keep running, which is exactly why closing a window is the
keep-everything exit rather than a quit. But the code panel's buffers are
the window's alone, no session file carries them — the session records the
layout, not the text — so the window closing is the last moment they exist.
Every other guard in the app was pointed at the recoverable losses and this
one at nothing.
`on_window_should_close` must answer now and a prompt answers later, so the
first close is refused and the real one is made from the answer; a flag stops
that second close asking again, which would mean a window that never shuts.
Only unwritten buffers are asked about — a busy command and a live SSH link
survive the window and warning about them would be warning about nothing.
Three tests: the decision (which names the tab holding the buffer, not
whichever is in front), the flag, and the callback gpui actually calls,
driven through `simulate_close` — a guard wired to nothing is no guard, and
one wired wrongly is a window that will not close.
`scm_discard_all` asks once and then hands its second operation on as
`ScmFollowUp::Op`, which `scm_follow_up` runs straight through `run_git_op`.
That is the only place in the SCM panel where something destructive runs
with no confirmation of its own, and it is sound only while the follow-up is
something the single prompt actually described.
The existing test pins one working tree in detail. The property that keeps
the bypass safe is the other one — that nothing else can ever come out of
`discard_all_ops`, whatever the repository looks like — so this walks the
shapes a working tree is actually found in and holds every operation to the
two the prompt names.
Checked by adding a hard reset for staged changes: the test names it and the
reason it matters, which is that the approval given for discarding would
have carried it.
There are two, in two crates, and they are not redundant: one puts a path in
front of the user's own shell and has three dialects to pick between, the
other writes a line that some `sh` on a machine will re-read. A reader who
finds one and not the other has every reason to write a third.
Deleting from the file tree calls `LocalHost::remove` — `remove_file` and
`remove_dir_all`. The file is gone. The confirmation said only that it "will
be deleted", which on a Mac or in Explorer reads as "moved to the trash",
because in every file manager the user has ever used it is.
The remote prompts made it worse rather than better. They have always said
"there is no trash on the far side", and read next to a local prompt that
says nothing, that plainly implies the local side has one.
So the local prompts now say what the remote ones do, in all three locales,
and a test holds every delete confirmation to naming the trash it is not
using. That sentence is only read on the day it matters, so nobody would
notice it going missing again.
A preset rebinds only the actions it names, so every default it leaves alone
stays where it was — which is where a collision would come from, and it would
only show up for the people who chose that preset. It is clean today.
Two actions on one key is not always wrong: `secondary-enter` is fullscreen
in the window and commit inside the commit box, which is what a scope is for.
It is wrong when the scopes match, because then one of the two simply never
fires, and nothing on screen says which — the winner is whichever
`rebuild_keymap` installed last, which is an ordering this file has already
had a bug about.
Nothing checked it. The 112 defaults are clean today; the test says so, and
says which pair if that stops being true. Actions with no default key are
exempt — an empty string is "palette only", not a claim on a key, and about
sixty actions are deliberately in that state.
Closing a tab straight after making it stranded a live shell — six times out
of six, reproducibly. It holds a pty and an fd and runs until the machine
does not. A person cannot type that fast; an agent loop that opens a tab per
task and closes it when the task is done meets it on nearly every iteration,
which is the shape this product is built for.
The rule for judging a pane was already here and already right: one that no
window is showing and no workspace on the machine names is nobody's. Two
earlier attempts failed because of what was put in front of it. Both parked
panes at the *site* that dropped them, and the leak does not live at a site:
a hydration whose whole session is discarded never compares a before against
an after, so its panes were never offered for judgement at all.
So the window now keeps a census of every pane it has brought into existence,
taken at the one point both spawn routes meet, and judges the census rather
than a list of suspects.
The second half is when. The sweep ran only on a landing that rebuilt from a
full machine tree, and the failures do not lead there — a refused operation
calls `desync`, which primes, and a prime pulls this workspace's mirror
alone. Instrumented, the sweep did not run once across three cycles that
stranded three shells. It now also runs after a prime, and pays for the
machine-wide pull only when a censused pane is not on screen anywhere;
while everything the window made is still showing, that is one set
comparison and no request.
Measured against a live window, same reproducer, same machine:
before 6 create-and-close cycles 6 stranded shells
after 16 create-and-close cycles 0
and nothing else was harmed: four unrelated panes kept their ids and their
scrollback across ten churn cycles and a GUI restart, which is the failure
this has to be judged on — a leaked shell is recoverable with `pane close
--orphans`, a shell killed under a live window is not.
Enter belongs to the input widget and inserts a bare \n whatever the file
around it does. Editing one line of a file checked out with CRLF therefore
mixed the two endings, and git reports that as a rewrite of lines the user
never visited.
The buffer still keeps the exact bytes it was given — that part was already
right, and there are now tests holding it there, along with hard tabs and a
missing final newline. What changed is the save: a file that arrived
uniformly CRLF goes back out uniformly CRLF. Deliberately unanimous, so a
file that already mixes its endings is still written exactly as found rather
than being handed a winner it never asked for.
Saving had no test at all before this; the new one writes a real file and
reads the bytes back.
Tried the nearest of the two silent paths: parking the seeded panes of the
queued operations `desync` is about to discard. It looks the most tractable of
anything left, because an op that never left this process provably never filed
its pane, so those are stranded by construction.
It is not a fix. It fires rarely — 0 to 2 panes across ten create/close cycles
— and does not reliably reap even those, because the sweep needs a hydration
to settle and a parked pane can outlive the window's next few pulls. One run
parked two panes and one of them was still an orphan at the end. Totals: 4, 4,
5 against a baseline of 5, 5, 5, which is inside the noise.
Backed out, and recorded next to the first attempt, because what the two have
in common is the useful part. Both park at a *site*. The leak does not live at
one — this comment already said so, and two experiments now agree with it: a
sweep phrased against any particular failure keeps missing whichever path was
not instrumented. It has to be phrased against the end state, over every pane
this window spawned, which is what the paragraph above describes and what
nobody has built yet.
No behaviour change.
The total was already recorded; the shape matters more, and it is not volume.
Measured against a live window:
tab new / split / send, paced or as fast as the CLI will go 0 orphans
tab new then tab close, 1s apart, x4 0 orphans
tab new then tab close, no pause, x6 5 orphans
The last reproduced exactly — 5 of 6, three runs in a row. What strands a
shell is closing something before the window has finished reconciling the
thing that made it, and the gap that matters is under a second.
That reframes the cost. A person cannot type that fast, so nobody driving the
window by hand meets this. An agent does it by default: a loop that opens a
tab per task and closes it when the task is done leaks a live shell on nearly
every iteration, and that is the shape this product is built for.
The strays are visible — `pane ls --all`, the switcher, and `doctor` since
this afternoon — and `pane close --orphans` ends them. Nothing ends them
automatically, which is what the end-state sweep this comment asks for would
do. Recorded in the changelog under Known, because a user running an
orchestration loop should be told rather than left to find it in `pane ls`.
The comment here already has the right analysis: panes stranded by a refused
operation are the minority path, the other two throw their panes away without
a refusal, and the sweep belongs after a hydration settles rather than at any
one failure. Two things it did not say, both now measured.
The scale and the severity. 160 CLI operations against a live window left 17
orphans, and every one was a live `zsh` — not a stale record — each holding a
pty and its descriptors. `tty7 pane close --orphans` ended all 17, so the
documented recovery does work.
And a dead end, written down so nobody spends the evening on it twice. Parking
the pane here rather than only warning — handing it to the `sweep_parked` that
already exists, which judges against the whole machine's freshly pulled tree
and spares anything a live view shows — is safe, and does not measurably help.
Four runs: of the panes that reach this arm, 33–60% were still orphaned at the
end without it and 43–50% with it. They are mostly already caught by the sweep
the layout rewrite arms. The leak lives in the two paths that never refuse an
operation, exactly where this comment says the fix belongs.
No behaviour change; the experiment was backed out.
gpui resolves a keystroke by sorting the matches on context depth and then on
registration index, later winning:
matched_bindings.sort_by(|(depth_a, ix_a, _), (depth_b, ix_b, _)| {
depth_b.cmp(depth_a).then(ix_b.cmp(ix_a))
});
`rebuild_keymap` added the config's bindings and then the fixed ones, so a
fixed binding won any tie. Six of the seven are scoped — `Terminal`,
`Switcher`, `Palette` — and win on depth whatever the order. The seventh is
global: `secondary-+`, the font-size step. A config that asked for that chord
got `IncreaseFontSize` instead, with no error, nothing in the keybindings UI,
and nothing in the log — which is the silence this tree has already gone out
of its way to remove from an unparseable chord, a clamped setting and an
unread config key.
Swapping the two lines fixes it and changes nothing else: depth is compared
before index, so the scoped six still win. And it matches what "fixed" was
ever for, which the comment on `fixed_bindings` states — they are not in
`effective_bindings`, so a rebuild replaying only the config would drop them.
That is an argument about existing, not about outranking.
Found by checking the reverse direction of the shortcuts page: every chord it
prints against every chord tty7 binds. Nothing was undocumented — `⌘ C`,
`⌘ V`, `⌃ R` and the arrow shorthands are keys tty7 answers without a keymap
action, which is why a strict guard there would be wrong — but `⌘ +` turned
out to be bound twice, and following that up is what surfaced the ordering.
`the_settings_page_documents_every_section` checked two numbers: that the page
says how many sections there are, and that it shows that many `<Card>`s. Eight
and eight is just as true when one of them has been called something else for
a release, so a rename passed untouched — and the page is what someone reads
before they go looking in the window.
The names now have to match. They come from `SettingsSection::nav_label`, a
new method whose match is exhaustive: a section added to `ALL` has to be given
a name before this compiles, and the nav builder reads the same answer instead
of spelling eight `L10nKey`s inline beside eight variants. One list where
there were two.
All eight agree today. Verified by renaming a card on the page, which the test
now names.
git will not take a control character in a branch name:
$ git branch $'featx\ry'
fatal: 'featx?y' is not a valid branch name
and takes one in an author name without a word about it:
$ git -c user.name=$'Bad\rName' -c user.email=b@b commit --allow-empty -m x
$ git log --format=%an -1 | sed -n l
Bad\rName$
So a cloned repository can carry one into the byline under a commit subject,
which sits in a bar exactly one row tall beside a subject that is truncated to
fit. Ninth site in this tree, and the third whose input is a repository rather
than the machine tty7 runs on.
Both bylines: the diff overlay's `label_byline` and the commit detail's
`byline` are separate functions that had drifted into the same shape, and
folding one would have left the other. The overlay's commit *subject* is
folded at the same site for the same reason — the graph row already does it,
and this bar is no taller.
An author that is only control characters now reads as no author, so the
middle dot goes with it, which is what the surrounding tests already say
should happen for an empty one. Both checked against injected regressions.
l0ng-ai