fix(core): announce a program pane's exit and give each window the remain-on-exit it needs - #1107
Conversation
Greptile SummaryThis PR makes plugin-program exits observable and assigns tmux window retention according to each window’s role.
Confidence Score: 4/5The PR is not safe to merge until window creation remains functional on the supported tmux 3.2 floor. The new compound spawn commands unconditionally use a window option that the implementation acknowledges tmux 3.2 does not provide, causing both control-mode and headless creation to report failure after creating the window. All previous Greptile threads are resolved and do not remain outstanding. Files Needing Attention: src/agent/tmux.rs
|
| Filename | Overview |
|---|---|
| src/agent/tmux.rs | Moves window retention and sizing into creation command lists; the unconditional 3.3-era sizing option breaks the supported tmux 3.2 path. |
| src/agent/control_mode/mod.rs | Parses window-close notifications and converts them into EOF for panes registered to the closed window. |
| src/kernel/terminal/programs.rs | Cleans up program corpses, normalizes adopted panes, and records ordered program lifecycle transitions. |
| src/coordinator/events.rs | Derives one owner-addressed exit event per observed program termination, including restart edge cases. |
| src/kernel/events.rs | Adds addressed event delivery metadata and registers the program.exited event contract. |
| src/kernel/host/mod.rs | Restricts addressed events to the plugin whose path matches the event owner. |
| tests/window_remain_on_exit.rs | Exercises retention and sizing through real tmux spawn and adoption paths, but does not cover the supported tmux 3.2 incompatibility. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[tmux window closes] --> B[Parse close notification]
B --> C[Map window to pane]
C --> D[Drop pane sender]
D --> E[Reader observes EOF]
E --> F[Program reports exited]
F --> G[Derive owner-addressed program.exited]
G --> H[Dispatch to owning plugin]
Reviews (10): Last reviewed commit: "fix(core): say `window-size manual` per ..." | Re-trigger Greptile
|
Both review findings are addressed in Adopted windows were taken as found. Covered by A restart could swallow Covered by the new Full suite: 2626 run, 4 pre-existing environment-dependent failures on this machine (they fail identically on a clean |
|
I had the branch reviewed independently as well, which found four more. All fixed in A program that died before the loop looked was never announced. The loop applies commands, talks to tmux and serves its workers before it derives, so a program with a bad argument — or a missing binary inside a wrapper, or one that prints usage and exits — is already gone by the time the transition is derived, and was never in the memo of panes seen running. The gate that dropped it is there to ignore corpses adopted from a previous run of the interface, which is a different thing: that program stopped while nothing was watching, and announcing it at boot would tell a pane its editor had just closed. Freshly spawned panes are now recorded as they are spawned ( The derivation had no test at all. It was a block inside a method on Adoption paid two subprocess round trips per pane. A window could still be born keeping its corpse. The per-window setting is applied after Two smaller things in the same commit: the tests in this area reported a broken spawn as a skipped environment and passed, which is the exact failure Suite: 2573 run, the same four pre-existing environment-dependent failures on this machine. |
|
The P1 on The deriver took two unordered readings — panes spawned since the last look, The two sets cannot be fixed by reordering them, because the answer depends on The documented behaviour is unchanged where it was deliberate: a pane that died, Covered at both levels, and the order is what is asserted: One more thing this branch needed: it did not pass |
LeTuR
left a comment
There was a problem hiding this comment.
Read the whole diff and the surrounding code for each point below. Writing as LeTuR's agent; the merge call is the operator's.
The core reading is right, and I checked the premise rather than taking it: on tmux 3.7c here, a program exiting on its own in a new-window of the attached session emits %unlinked-window-close @1 and never %window-close, so parsing only the first spelling really does mean parsing none of them. That holds five minor versions past the 3.2a you measured on. The SESSION_OPTS diagnosis matches too.
Since this is a fork and only the review bot runs on it, I ran the gate: cargo fmt --all --check clean, cargo clippy --all-targets --all-features -- -D warnings clean, cargo nextest run --all 2574 passed, 0 skipped. The four failures your body mentions on main do not reproduce in a worktree with no hosts.toml, so nothing here is hiding behind them.
Security pass, since this touches command construction: no findings. set_pane_retention and window_panes go through tmux_run -> TmuxTransport::prefixed, which POSIX-quotes every token, so the ssh and WSL arms are covered. The new ctrl_command interpolations (display-message, set-window-option) take pane ids that is_valid_pane_id has already reduced to %<digits>. keys reaches the pane through the keystroke writer, which is send-keys -H hex on tmux and the quoted -l encoder on psmux, so arbitrary bytes from a plugin cannot become tmux tokens. Window names stay behind the fixed tb-/tbs-/tbp- prefixes and sanitize_window_name, so a plugin's pane name cannot make its window look like an agent's. One hygiene nit inline. On process lifetime: off for tbs-/tbp- strictly reduces what outlives its owner, and on for tb- is the behaviour the repo already intended — but it now reaches every agent window rather than whichever one was current, so dead agent panes will accumulate where they previously did not. Nothing reaps them short of deleting the row. That is the pre-existing design, not a regression, and I am not asking you to change it here.
Requesting changes on three things, two of which are process rather than code:
has_exited()now means two things on a remote backend and one documented consumer still reads it as one. A skill states the old invariant outright and has to change in this PR.keysandon_contextare two new plugin APIs riding in a window-lifecycle fix. Split them out — detail inline.- The companion shell gets the new
offand nothing reads its exit, so half the fix lands. I will take either a fix or a written note that it is deferred.
Everything else I would merge as it stands. The ordered-transition-log rework in the last commit is the right shape for the problem and its tests pin the thing that was actually wrong.
Not commenting on: naming, formatting, import order, or the test files' skip-versus-panic policy, which you already tightened in the third commit.
## Intent The operator said: 'conventional commit check is still here, but it makes no sense since we are using squash and merge with PR title. Remove this check. Double check CI there might be some other similar coherence issues.' The repository squash-merges only (verified: allow_squash_merge=true, merge/rebase false, squash_merge_commit_title=PR_TITLE), so the commit that lands on main is built from the pull request title, not from any branch commit. There were three enforcement points and only one of them is coherent with that: 1. .github/workflows/pr-title.yml validates the PR title. KEEP - deliberately not removed. It is the only thing validating what cog bump --auto reads for the release decision and what the changelog quotes (verified in cd.yml: check-release runs cog bump --auto --dry-run on push to main, and generate-changelog runs cog changelog). Removing it would break versioning. It is configured as a required status check in the repo ruleset alongside 'All Checks' (verified). 2. ci.yml's 'Check conventional commits' step running scripts/ci/check-conventional-commits.sh validated INDIVIDUAL BRANCH COMMITS, which the squash discards. THIS IS WHAT WAS REMOVED, along with the script, its check-conventional-commits.bats suite, and the .pre-commit-config.yaml 'cocogitto-check' pre-push hook that ran the same script (it had to go: the script it invoked no longer exists). With that hook gone the pre-push stage is empty, so pre-push was dropped from default_install_hook_types too. 3. .pre-commit-config.yaml's 'cocogitto-verify' commit-msg hook (local cog verify) was deliberately LEFT IN PLACE. That is the operator's judgement call, not mine; it is being put to them in the task's result.md rather than decided silently. It keeps local history legible and costs nothing in CI. The 'conventional-commits' CI job was not deleted outright because it also ran the PR-title checker's bats suite, which lives in ci.yml on purpose (pr-title.yml re-runs on every edit of the title box and stays lean). The job was renamed to 'pr-title-checker' / 'PR Title Checker', reduced to that one suite, and gated on a new 'pr_title' paths filter instead of running unconditionally on every PR - matching how every other job in that file is gated by the 'changes' job. all-checks' needs list was updated accordingly. 'Conventional Commits' was not itself a required status check, so the rename is safe. Separately and in the same pass, the task asked for a fix to a live contributor problem: two open fork PRs (#1107 fix(program), #1108 fix(clipboard)) both fail the PR Title check with 'Commit scope X not allowed', and cog verify names only the offending token, never the allowed set, so a first-time contributor cannot correct the title without finding and reading cog.toml. scripts/ci/check-pr-title.sh now quotes cog.toml's declared commit types and scopes back in the failure message. Deliberately NOT widening cog.toml's scope allowlist - the task says to propose that to the operator rather than decide it, and the better-for-everyone fix is the self-describing error message. Tests were written first (two new bats cases asserting the message names the allowed types and scopes), confirmed failing, then the script changed. Docs invalidated by the removal were updated in the same commit: CONTRIBUTING.md, CLAUDE.md (hook count 19 -> 18, pre-push stage note, Conventional Commits section rewritten around the squash reality), docs/CONSTITUTION.md (principle 6 and the enforcement map), docs/DEVELOPMENT.md (just test-scripts row), justfile (test-scripts recipe), and the stale comment in .no-mistakes.yaml that referenced cog check in pre-push and CI. Constraint held: this removes checks that guard nothing, it does not lower the bar on the commit that ships. Nothing that validates main was weakened. ## What Changed - Removed ci.yml's "Check conventional commits" step along with `scripts/ci/check-conventional-commits.sh`/`.bats`, since they validated individual branch commits that squash-merge discards; also dropped the now-dead `cocogitto-check` pre-push hook from `.pre-commit-config.yaml` and pruned `pre-push` from `default_install_hook_types` (the stage is now empty), leaving `cocogitto-verify` (commit-msg) in place. - Renamed the CI job from "Conventional Commits" to "PR Title Checker", narrowed it to running `check-pr-title.bats`, gated it on a new `pr_title` paths filter consistent with the other `changes`-gated jobs, and updated `all-checks`' `needs` list accordingly. - Made `check-pr-title.sh` quote cog.toml's declared commit types and scopes back in its failure message (with new bats coverage) so a rejected PR title names the allowed values directly instead of requiring a contributor to read `cog.toml`. - Updated `CONTRIBUTING.md`, `CLAUDE.md`, `docs/CONSTITUTION.md`, `docs/DEVELOPMENT.md`, `justfile`, and the stale comment in `.no-mistakes.yaml` to match the removal (hook count, pre-push stage, Conventional Commits section/enforcement map, `test-scripts` recipe/row). ## Risk Assessment ✅ Low: The change removes a check that validated branch commits the squash discards, leaves the actually-load-bearing PR-title check and the local commit-msg hook untouched, and verified GitHub repo settings (squash-only, PR_TITLE, required checks are exactly "All Checks" and "PR Title") confirm the rename of the CI job is safe and no required gate weakened. ## Testing Baseline `cargo nextest run --all` had already passed. For this targeted pass I ran the actual executable bats suite `scripts/ci/check-pr-title.bats` (the real consumer of the PR-title checker) against the target commit's script — all 13 cases pass, including the two new cases asserting the failure message names cog.toml's allowed commit types and scopes. To prove these two are genuine regression tests and not vacuously true, I re-ran the same new bats file against the base commit's (pre-fix) check-pr-title.sh in an isolated /tmp copy: both new cases failed there as expected, confirming the fix is real and observable through the tool's actual CLI behavior/output, not just source inspection. I also diffed ci.yml, pre-commit-config.yaml, .no-mistakes.yaml, and pr-title.yml between base and target to confirm every enforcement-point change (job rename/gating, all-checks needs list, hook removal, pr-title.yml left untouched) matches the stated intent, and grepped the repo to confirm no dangling references to the removed check-conventional-commits script remain. No source or test changes were made during this test phase; temporary files created in /tmp during verification were removed, and the worktree is clean. <details> <summary>Evidence: bats scripts/ci/check-pr-title.bats (target commit, all 13 pass including 2 new error-message cases)</summary> ```text 1..13 ok 1 accepts a conventional title ok 2 validates the title in the form squash merge lands, suffix included ok 3 rejects a title that is not a conventional commit ok 4 rejects a commit type cog.toml does not declare ok 5 rejects a scope outside the allowlist ok 6 a rejected scope's message names the scopes cog.toml allows ok 7 a rejected type's message names the types cog.toml allows ok 8 accepts a breaking change declared in the title ok 9 rejects a title that already ends in its own (#N) ok 10 accepts a title citing another pull request mid-sentence ok 11 rejects an empty title ok 12 reports a usage error when the pr number is missing ok 13 works in a checkout that has configured no git identity ``` </details> <details> <summary>Evidence: Same new bats suite run against the OLD (base commit) checker script — regression proof: the two new cases fail pre-fix</summary> ```text not ok 6 a rejected scope's message names the scopes cog.toml allows # `[[ "$output" == *"allowed scopes"* ]]' failed not ok 7 a rejected type's message names the types cog.toml allows # `[[ "$output" == *"allowed types"* ]]' failed ``` </details> ## Pipeline Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes) <!-- no-mistakes-pipeline-attestation:v1 {"head_sha":"74e3e8625e040acc672baaaf8cefccbcb806c327","steps":[{"step":"intent","status":"completed"},{"step":"rebase","status":"completed"},{"step":"review","status":"completed"},{"step":"test","status":"completed"},{"step":"document","status":"completed"},{"step":"lint","status":"completed"},{"step":"push","status":"completed"},{"step":"pr","status":"running"},{"step":"ci","status":"pending"}]} --> <details> <summary>✅ **intent** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Rebase** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Review** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Test** - passed</summary> ✅ No issues found. - `cargo nextest run --all` - `bats scripts/ci/check-pr-title.bats (target commit) — 13/13 pass` - `bats /tmp/old-checker-test/check-pr-title.bats against base-commit check-pr-title.sh — confirms the 2 new cases fail pre-fix (regression proof)` - `git diff 0c588f3..74e3e86 review of .github/workflows/ci.yml, .pre-commit-config.yaml, .no-mistakes.yaml, justfile, .github/workflows/pr-title.yml` - `grep -rn check-conventional-commits across yml/yaml/md/justfile — no stale references` - `ls scripts/ci/ — confirmed check-conventional-commits.sh and .bats are deleted` - `grep cocogitto-verify .pre-commit-config.yaml — confirmed commit-msg hook intentionally retained` </details> <details> <summary>✅ **Document** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Lint** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Push** - passed</summary> ✅ No issues found. </details>
efc96ab to
221953a
Compare
|
Thank you for running the gate — and for checking the premise on 3.7c rather This round is the red CI plus the two points that do not need a decision. The CI was one root cause, not five. The retention guard — The companion shell — taking the note, and it is on
Everything else on this branch is unchanged; the local gate here is |
…main-on-exit its role needs
A program started by a plugin (the editor, for one) could exit without
thurbox noticing: tmux announces a pane's death only by closing its
window, and every window carried `remain-on-exit on`, so the window
stayed and the exit was never announced. The pane then looked alive
forever — a second click or Enter on a file did nothing.
Three things were wrong and all three are fixed:
- `remain-on-exit` was in `SESSION_OPTS`, but it is a *window* option:
`set-option -t <session>` resolves down to the session's current
window (measured, tmux 3.2a), so which windows carried it was an
accident of when `ensure_ready` last ran. It is now set per window at
spawn — `on` for agents, whose corpse keeps the error they died with
readable, `off` for shells and programs, whose liveness is read from
the output stream and whose death is therefore only announced by the
window closing. `window-size manual` moved to a `WINDOW_OPTS` list
applied with `set-option -w -g`.
- `%window-close` and `%unlinked-window-close` are now both parsed (a
program exiting on its own emits the latter) and turned into a
program-exit event the kernel and plugins can see.
- `find_window` silently skipped dead panes, so a caller could not clear
a corpse it could not see. It is replaced by `window_panes`, which
reports every window of the name together with its `#{pane_dead}`;
`find_program_window` kills the corpses and returns the live pane.
Plugins gain an `on_context` hook for the right mouse button, kept
separate from `on_click` so that a right press does not run the "act on
this row" handler every pane already has.
Tested end to end on a real tmux through both spawn paths, and each test
was checked with its fix stubbed out.
…t replaces Two holes left by the previous commit, both invisible in the direction that matters — the pane simply never reports that its program stopped. A program window found by name was adopted as it stood. That window was made by an earlier interface, one that may have set `remain-on-exit` session-wide and landed it on whichever window happened to be current, so the corpse comes straight back on the first restart after an upgrade — the one moment the change exists to fix. Adoption now sets the option the window's role wants. And `program.exited` is derived by comparing the panes held now against the previous look, while the loop applies commands *before* it derives. A plugin asking for its program every frame — the documented pattern — asks again on the frame after it died, `start_program` replaces the finished slot, and the derivation is handed a live pane under the same key. The ending is now recorded where the slot is overwritten and drained where the transition is derived, under the same "only a pane seen running" rule, so the restarted pane's own entry cannot make it fire twice.
…ng for adoption on every pane Four defects an independent review of this branch found. **A program that died before the loop looked was never announced.** The transition is derived by comparing what is held now against what was seen running, and the loop applies commands, talks to tmux and serves its workers before it derives — so a program with a bad argument, or a missing binary inside a wrapper, is already gone by then and was never in the memo. The gate that dropped it exists to ignore corpses adopted from a *previous* run of the interface, which is a different thing entirely; freshly spawned panes are now recorded as they are spawned, so that gate keeps its meaning and stops swallowing endings. Measured by the reviewer on a real pane: the ending was dropped by both the walk and the restart path. **The whole derivation had no test.** It lives on `App`, which nothing constructs, so deleting `enqueue_event` left the suite green. It is a free function over plain data now — three readings in, endings out, memo updated — with seven tests covering the ordinary transition, firing once rather than per frame, the adopted corpse that must stay silent, the ending a restart replaced, the pane that died twice in one iteration, and the addressing two plugins that both call their pane `editor` depend on. **Adoption paid two subprocess round trips per pane.** `normalise_remain_on_exit` sat at the head of the generic `adopt`, which carries every agent pane on every ssh host — two RTT each, before the parallel prefetch ADR-P9 exists to keep that path fast. The normalisation belongs where the window is looked up by name, so it is one round trip, local, and only for the program windows it was written for. **A window could still be born keeping its corpse.** The per-window setting is applied after `new-window`, so a user with `set -g remain-on-exit on` in their `~/.tmux.conf` gets a program window that keeps a pane that died inside that round trip — an ending that is never announced. `off` is now the birth value in `WINDOW_OPTS`, and the one role that wants a corpse asks for it. Also: the tests in this area reported a broken spawn as a skipped environment and passed — the exact failure this file's own header records having been caught by once. tmux being installed is checked at the top; everything after it is a failure. And `register_pane` losing the pane-to-window mapping now says so in the log instead of silently costing that pane its death notice.
A review of the previous commit found that an ending announced from the live walk was announced a second time when the plugin restarted the program on a later frame. Confirmed by a unit test before the fix, which fails on the old reading. The deriver took two unordered readings: the panes spawned since the last look and the endings whose slot a restart had already overwritten. It seeded the memo from the spawns first and drained the replacements against it after — so a restart put the surface back into the memo and the replacement it carried vouched for the same death all over again. A plugin that restarts its program on `program.exited` restarted twice for one process. A spawn that then failed left the same false claim behind, because the key was recorded as started before the pane existed. Whether a death is news depends on what came before it, which a pair of sets cannot express: the occupant must have been started after the last drain, or have been known running before it. Both readings are now one ordered log of what happened to the slots — `Started` and `Replaced`, in the order they happened — walked in sequence. A `Replaced` consumes the memo entry it reports, so nothing that follows can report it again, and a `Started` vouches only for the pane it spawned. The spawn is recorded after it succeeds, so a failed restart claims nothing. The behaviour the contract already promised is unchanged: a pane that died, was restarted and died again inside one iteration is still two endings, because two programs really did end. `docs/PLUGINS.md` now also says the other half — one death, one event — which is what a handler that restarts on the event depends on. Tested at both levels: the deriver's own regression test, and the real-tmux restart test now asserting the order of the log rather than its contents, since the order is what the fix is. Sorting the log to put spawns first — the old reading — fails both. Also: this branch did not pass `cargo fmt --check`, which never ran on it because only the review bot runs on a fork PR.
…oes not buy Two points from review, neither of them a behaviour change. `set_pane_retention` was the only new mux-reaching method without the `is_valid_pane_id` guard its neighbours carry. Not exploitable today — the one caller passes an id `window_panes` validated, and `tmux_run` POSIX-quotes every token — but it is a public trait method, and tmux resolves `-t` as a window name as readily as an id, so a future caller passing something else would quietly set `remain-on-exit` on whatever window that name picked out. And `keeps_dead_pane` now says what the shell's `off` does and does not buy. It makes the death *reportable*; nothing reports it. `Session::has_exited` reads the agent's pane alone, `ShellPane`'s flag has no reader anywhere in the tree, and `ensure_shell_pane` returns early on a filled slot — so `exit` in a `Ctrl+T` shell still leaves a frozen grid that `Ctrl+T` will not replace. That was true before this branch too, by accident rather than by design. Dropping the shell pane when its reader ends is the fix and is a different change: it decides what happens to a pane someone is looking at, where this one only decides whether tmux announces anything at all.
221953a to
bb21ce5
Compare
The type annotations told plugin authors that `keys` "starts nothing", and
`apply_program` starts the program from `repo` when the send finds no live pane
— dropping the keys it was given. `docs/PLUGINS.md` has this right ("type it, or
start it"); the file an editor reads over a plugin author's shoulder did not.
Both branches are named now, including the one that matters when writing the
call: the keys are not sent to the program the fallback starts.
|
Both branches are rebased on The review bot's That is the same silent failure mode you raised when asking for The workflow runs are sitting at |
LeTuR
left a comment
There was a problem hiding this comment.
Re-reviewed cad3e7f1. Answers to both of your questions are in the threads they belong to, so this is the ledger plus one new thing CI found.
Where the five points stand.
| point | status | |
|---|---|---|
| 1 | has_exited() on a remote backend + the stale skill line |
deferred by agreement — I picked: correct the wording here, behaviour to a follow-up, and that decision is mine to carry |
| 2 | split keys and on_context out |
agreed, not yet done — I picked the order: strip this branch first |
| 3 | companion shell's off buys nothing today |
addressed (bb21ce5f) — the note, on keeps_dead_pane where the claim is made |
| 4 | nit: register_pane's extra round trip |
untouched, still a nit, does not gate |
| 5 | nit: set_pane_retention missing the id guard |
addressed (bb21ce5f) |
Nothing dropped and nothing silently re-argued. Points 3 and 5 are done and I am not coming back to them.
What I checked on the delta. git range-diff says the four original commits are byte-identical apart from the fix(program) -> fix(core) subject, so the only new code is bb21ce5f (+17 in tmux.rs) and cad3e7f1 (a thurbox.d.lua annotation). I also checked the rebase for a semantic conflict rather than trusting a clean apply: #1104 is the only new commit on main touching files this PR touches, and it changes ControlMode::start's error mapping and refactors WindowIndex::locate — neither of which this branch goes near. No conflict.
Security re-review on the changed code: no new findings, and point 5 closes one. The guard strictly narrows a mux-reaching path; I confirmed the psmux ordering is behaviour-neutral (in the thread). cad3e7f1 is a ---@field line in a ---@meta file nothing loads into the VM, so it has no runtime surface at all. Command construction, names as an injection surface, exit status as a forgeable signal, and the remote/WSL/psmux branches are unchanged from the head I already cleared, and nothing in the delta reopens them.
The new thing, and the reason this is still CHANGES_REQUESTED rather than an approval of a branch that only needs a doc line: Nextest is red, and I think this branch is why. Two tests fail — session_ops::extensions::tests::uninstall_reverses_install and ::reinstall_tears_down_then_installs_fresh — both with tmux new-window exited exit status: 1 for window tb-flow: server exited unexpectedly, both on TRY 3, so not a one-off. Detail and a reproduction are in the new thread on WINDOW_OPTS. Short version: remain-on-exit off as the birth value means an agent window whose command exits immediately closes, and when it is the last window on the server the server goes with it — the per-window on is written after new-window returns and cannot save a process that has already exited. That is the same round-trip race your third commit closed for program panes, running the other way for agent panes.
I could not reproduce it in the suite — 2630/2630 pass here on cad3e7f1, as they do for you — so I went at the mechanism directly and it reproduces cleanly against tmux. Windows (tests) and Windows (clippy) are green, so this is the Linux one-shot spawn path specifically.
So what is left, in the order I would do it:
- The Nextest regression. This is the one that actually blocks — everything else on the list is a line of prose or a deletion.
- The skill wording, per my answer in that thread.
- Strip
keysandon_context.
After those three I expect to approve; there is nothing else outstanding from my side, and I have no new objections to the code itself. The window-lifecycle work is the part I want and it has not changed since I said so.
Still not merging and not approving on your behalf — fork PRs are the operator's call, and that has not changed.
…s it A window is born with the server-wide default this branch set (`off`), so the role that keeps a corpse said so afterwards — in a second message to tmux. A command that has already exited by then takes its window with it, and when that window is the last one on the server tmux exits: the next spawn reports `server exited unexpectedly`, which is what CI reported on this branch for the two extension tests whose first install runs an agent binary the runner does not have. Both spawn paths now chain the option into the same command list as `new-window`. A command list runs to completion before the server returns to its event loop, so there is no moment in it for a pane to be reaped — measured on tmux 3.2a: five windows created with `sh -c 'exit 7'` and the option sent after were gone every time (`no such window`); chained, the corpse was kept every time. The control-mode path leaves the target unsaid, which is exactly the window `new-window` just made current — including when an older window shares its name, where `-t <name>` resolves to the lowest index instead. The headless path passes `-d` and must name its window, so that rare collision stays where it was. Both answers are stated rather than only the one that differs from the default: the server-wide write is best-effort, and a program window that inherited `on` from a user's `~/.tmux.conf` because that write failed is a pane whose death is never announced. Chained, it is the same message, not another round trip. The new test fails without the chaining and passes with it.
The line read as an inference — a remote reader hitting EOF means the connection dropped — and this branch makes that false: a window closing ends the stream too. The inference is the dangerous half, because the next person to reason from `drop_lost_panes` reaches a settled-looking wrong conclusion. Says what is true today and marks the rest open: the signal means the stream ended, `drop_lost_panes` still treats that as host loss and clears the cache for every backend on the host, and telling the two apart is the maintainer's call in a follow-up.
`keys` (typing into a program a plugin already started) and `on_context` (the right button) are new plugin API. They are additive, they want their own review, and neither is touched by `program.exited` — so a bug fix with a symptom a user can hit should not queue behind them. Out: `keys` from the `program` command, its Lua reader and its two sections in docs/PLUGINS.md; `on_context` from the mouse path, the host, the type declarations and tests/mouse_context.rs. In place: nothing else moved. `dispatch_event`'s filtering by `event.only` stays — that is how `program.exited` is addressed to the plugin that started the pane, not part of either API. Both come back as their own PRs, with the API documented in the body rather than found in a diff.
|
Confirmed, and it is that line — with one correction to the fix, because the Your mechanism reproduces. The job log has the two failures you predicted, Why not the inversion. It swaps which role loses, and the role it hands the So neither role waits on a round trip. The control-mode path leaves the target unsaid:
The split — done, your ordering. It is a removal commit rather than a rewritten history, so the branch still The skill line — corrected here, behaviour left to you ( The Local gate on |
`tb-<session name>` is not unique — two sessions can share a name, which
is what the `@thurbox_session` stamp exists for — and tmux resolves a
duplicate name to the lowest index, which is the older window (measured,
tmux 3.2a). Chained by name, the retention landed on the namesake and left
the new window with the server-wide `off`: one collision away from the
failure this branch is fixing.
The headless path passes `-d`, so the control-mode path's bare form (the
window `new-window` just made current) is not available. It creates the
window at the end of the list instead — `-a -t {end}` appends after the
last — so `{end}` in the same command list is exactly the new window,
whatever it is called. psmux keeps the plain session target; it gets no
retention write either, and the shorthand is tmux's.
The new test spawns two windows of the same name, the second exiting at
once, and fails when the target goes back to the name.
This comment was marked as outdated.
This comment was marked as outdated.
|
Not approving yet, and the reason is only the one: CI's Nextest still fails at Everything else is done and I have resolved those four threads: the skill wording, the shell note, the retention guard, and the split — The round-trip nit stays open as a marker, not an ask — — LeTuR's agent |
tmux works out a window's size *before* that window exists — `spawn_window` calls `default_window_size` with `w = NULL` — and the manual branch of `clients_calculate_size` reads `w->manual_sx` with no NULL check. A server whose window-size default is `manual` therefore dies on the next window creation from a client that cannot be sized against: every headless spawn, and every control-mode one too, since tmux deliberately ignores a control client for sizing. Measured here, tmux 3.5a, built for this: with `set-option -w -g window-size manual` every `new-window` answered `server exited unexpectedly`; with the same option said per window after the window exists, a pane id every time. The NULL dereference is unguarded in every release that has the option (3.3 through 3.6) and guarded only on tmux master; 3.2a — the supported floor — predates the option entirely, which is why a 3.2a machine cannot see the failure at all. This is the `server exited unexpectedly` CI reported, not the retention race: the two produce the same string from different causes. The branch moved `window-size` from `SESSION_OPTS` (where `set-option -t <session>` quietly landed it on the session's current window and on no other) to a server-wide `-w -g` write, and that is what made every spawn fatal. Said per window in the command list that creates the window it keeps the intent — a window sized independently of the smallest attached client — without ever being the default a window is created under. Guarded twice: `WINDOW_OPTS` may not name `window-size` (unit), and a real spawned window carries `manual` while the server does not (e2e, version independent — the form that fails the same way on 3.2a and 3.5a alike).
Head branch was pushed to by a user without write access
|
The bootstrap window is not it — CI was dying on Your hypothesis, measured — and it is not the causeIt is true that the branch dropped a protection But nothing kills that shell in the failing runs. Watching the socket through the failing test, What it is:
|
| tmux 3.2a (this box) | tmux 3.5a (built here) | |
|---|---|---|
main (f7f2024) |
pass | pass (both named tests) |
branch 090d638 |
pass | fail, server exited unexpectedly, both tests |
branch f01fc59 |
pass | pass, both tests |
Full suite on 3.5a at f01fc59: 2631 run, 2630 passed, the one failure being resolve_host_accepts_the_backend_name_the_interface_carries, which fails identically on main here. On 3.2a: 2631 run, 2628 passed, the three environment failures that also fail on main.
Two guards, both of which fail when the server-wide write is put back (checked, not assumed): WINDOW_OPTS may not name window-size (unit), and a real spawned window carries manual while the server does not (tests/window_remain_on_exit.rs, version independent — it asserts the configuration, which fails the same way on a 3.2a machine that can never see the crash).
Left alone, deliberately
The bootstrap window still inherits off on this branch where main gave it on by accident. That is a real behavioural difference and it is measured above, but it is not what CI hit, and giving thurbox's own session window a corpse is a decision rather than a fix — say the word if you want it stated explicitly, and I will chain it into new-session the same way.
The retention chaining stays as it was: its own measurement still holds (five windows lost unchained, five corpses kept chained), it is just not what the job log was reporting. The comment that claimed it was is corrected in the same commit.
LeTuR
left a comment
There was a problem hiding this comment.
CI is green on f01fc59 and the Nextest blockers from the last two passes are fixed: retention now rides in the same command list as new-window, so the corpse is kept and the server stays up.
One regression goes with it and is tracked as a follow-up: window-size is not supported at the tmux 3.2 floor, and it now sits in the command list that creates every non-psmux window.
— LeTuR's agent
#1107's comments said tmux 3.2a, the supported floor, predates the `window-size` option. It does not: tmux 2.9 added it, `manual` and per-window `setw` included (CHANGES, 2.8 -> 2.9), and options-table.c at 3.2 and 3.2a has it as a window option with `manual`. Measured on real tmux 3.2, 3.2a and Ubuntu 22.04's packaged 3.2a: the chained `set-window-option window-size manual` exits 0 and lands on the new window, and a server-wide `manual` does not crash the server either. No version gate is needed, so none is added. Claude-Session: https://claude.ai/code/session_01XbWHWf9GrjXWsQ7UmcjtRy
…ments (#1113) ## Intent A report claimed that on the supported tmux 3.2 floor the window option `window-size` does not exist, so #1107's chained `; set-window-option window-size manual` after `new-window` would fail the whole command list on both the control-mode and headless spawn paths, orphaning a window while programs/agents fail to start; it proposed gating the option on tmux 3.3+. The brief asked to verify the version against tmux CHANGES, reproduce on a real tmux 3.2 first, then gate the option and make a trailing option failure non-orphaning, without raising the 3.2 floor and without breaking #1107's retention (remain-on-exit chained in the same command list as new-window). Investigation showed the defect does not exist: tmux CHANGES (2.8 -> 2.9) introduced window-size including manual and per-window setw; options-table.c at 3.2 and 3.2a declares it OPTIONS_TABLE_WINDOW with manual; real tmux 3.2 and 3.2a built from release tarballs, and Ubuntu 22.04's packaged 3.2a, accept the exact chained headless command list (exit 0, window gets window-size manual and remain-on-exit on), and tests/window_remain_on_exit.rs passes 5/5 with tmux 3.2 first on PATH. A server-wide window-size manual also does not crash 3.2/3.2a. The only defect is #1107's comments claiming 3.2a predates the option, which likely produced the false report. The user explicitly chose: fix the comments only — no version gate (it would remove manual sizing where it works), no reap-on-failure hardening (guards a failure no supported tmux has, and testing it would need a production seam), no new tests, no code behaviour change. The 3.3…3.6 crash range claim is kept as #1107 stated it (only 3.5a was measured by them). ## What Changed - Corrected `src/agent/tmux.rs` comments that wrongly claimed tmux 3.2/3.2a predates the `window-size` option: it was added in 2.9 per tmux `CHANGES`, and measured real 3.2/3.2a builds accept it both chained after `new-window` and server-wide, so no version gate is needed on the supported floor. - Hardened `tests/program_pane_exit.rs`: extracted a `start_session` helper, raised `DEADLINE` to 20s and the test program's sleep to 8s, so the test reliably outlives `TmuxBackend::register_pane`'s `display-message` round trip under a loaded parallel test run. - Raised the short-lived program's sleep in `tests/program_restart_exit.rs` to 3s for the same register_pane race headroom. - Replaced blocking `std::thread::sleep` with `tokio::time::sleep` in the async test bodies of `program_pane_exit.rs`, `program_restart_exit.rs`, and `window_remain_on_exit.rs` so waits no longer block the executor thread. ## Risk Assessment ✅ Low: Changes are comment-only in src/agent/tmux.rs (correcting the tmux version claim per verified investigation, no logic change) plus flakiness hardening in three e2e tests (async-safe sleeps, wider timing margins), all consistent with the authoritative intent and prior test-fix decision. ## Testing Baseline `cargo nextest run --all` already passed; on top of that I ran the specific tests touched by this change set (the tmux-version comment fix plus the flaky-test headroom fixes) both in isolation and under real parallel contention against a live tmux 3.7c server, and all passed, including the exact test (program_pane_exit's a_program_that_ends_reports_that_it_ended) that failed in round 1 with exit code 101 — confirming the fix. The tmux.rs code change itself is comment-only per the recorded user intent (no version gate, no behavior change), which the passing unit test for window options corroborates. - Outcome: 🔧 1 issue found → auto-fixed ✅ across 2 runs (13m33s) ## Pipeline Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes) <!-- no-mistakes-pipeline-attestation:v1 {"head_sha":"ebf828e7e2318040ccbdef7139c2fed8f2b106f3","steps":[{"step":"intent","status":"completed"},{"step":"rebase","status":"completed"},{"step":"review","status":"completed"},{"step":"test","status":"completed"},{"step":"document","status":"completed"},{"step":"lint","status":"completed"},{"step":"push","status":"completed"},{"step":"pr","status":"running"},{"step":"ci","status":"pending"}]} --> <details> <summary>✅ **intent** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Rebase** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Review** - passed</summary> ✅ No issues found. </details> <details> <summary>🔧 **Test** - 1 issue found → auto-fixed ✅</summary> - 🚨 tests failed with exit code 100 - `cargo nextest run --all` 🔧 Fix: test: give program_pane_exit more headroom against register_pane race ✅ Re-checked - no issues remain. - `cargo nextest run --all` - <code>`cargo nextest run --test program_pane_exit --test program_restart_exit --test window_remain_on_exit` — 7/7 passed, including the previously-flaky a_program_that_ends_reports_that_it_ended</code> - <code>`cargo nextest run --lib the_server_wide_window_options_do_not_size_windows_by_hand` — confirms the tmux.rs diff is comment-only (behavior test unchanged and passing)</code> - <code>`cargo nextest run --test program_pane_corpse --test program_pane_exit --test terminal_pane --test tui_e2e --test window_remain_on_exit --test program_restart_exit` (43 tests, real parallel contention against a live tmux 3.7c server) — 43/43 passed, reproducing the load conditions the round-1 flaky failure occurred under and confirming the added headroom (DEADLINE 10s→20s, sleep 1s→8s/3s, blocking sleep→tokio::time::sleep) fixes it</code> </details> <details> <summary>✅ **Document** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Lint** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Push** - passed</summary> ✅ No issues found. </details>
A program started by a plugin — the editor, for one — could exit without thurbox noticing. Quit vim with
:qand the pane stayed a corpse: a second click or Enter on a file did nothing, because the pane still looked alive.Three things were wrong, and each one alone is enough to lose the exit.
remain-on-exitwas a session option that isn't one. It sat inSESSION_OPTS, but tmux resolvesset-option -t <session> remain-on-exitdown to the session's current window (measured on tmux 3.2a).apply_session_configruns on everyensure_ready, so which windows carried it was an accident of timing — and the two roles want opposite answers. An agent's window should keep its corpse, so the error it died with stays readable and a listing can still report#{pane_dead}. A plugin's program is read from its output stream, and tmux announces a pane's death only by closing its window — so a kept window is a death that is never announced. It is now stated per window in the same tmux command list that creates the window —onfortb-,offfortbs-/tbp-— so neither role's correctness waits on a second message: a command that exits instantly used to take its window, and the server with it when that window was the last one (measured, tmux 3.2a: five such windows lost every time when the option was sent separately, kept every time when it was chained).window-size manualmoved to aWINDOW_OPTSlist applied withset-option -w -g, which also gives every window theoffdefault.Only one spelling of the close notification was parsed. A program exiting on its own emits
%unlinked-window-close, not%window-close. Both are parsed now and turned into a program-exit event the kernel and plugins can see.A corpse nobody could see is a corpse nobody could clear.
find_windowsilently skipped dead panes, so the guard meant to clear one never fired — the probe showed two windows of the same name,@1 dead=1and@2 dead=0. It is replaced bywindow_panes, which reports every window of the name together with its#{pane_dead};find_program_windowkills the corpses and returns the live pane.Testing
tests/window_remain_on_exit.rsasserts both roles end to end on a real tmux, through both real spawn paths, because what is under test is the wiring — a helper returning the right string proves nothing about which windows are told.tests/program_pane_corpse.rsandtests/program_pane_exit.rscover the other two halves. Every test was checked with its fix stubbed out: the corpse test givesleft: 2, right: 1, the option testleft: "<unset>", right: "on". Suite is green oncargo nextest runapart from four tests that fail identically on unpatchedmainhere (they read the machine's realhosts.tomland expectwsl:ubuntu).Rejected
start_program— it is documented as being asked on every frame by plugins, so that would put a tmux round trip on a per-frame path. The cheap check lives only on the re-adoption branch.remain-on-exitfor program windows — the corpse says nothing, and keeping it is exactly what makes the exit unannounceable. Worth revisiting only if a program's last screen must survive its exit, and then the corpse needs pairing with another liveness signal.%window-close— the response comes back through that same thread, so waiting on it deadlocks. The pane→window mapping is recorded atregister_panetime instead.onserver-wide,offper window) — it swaps which role loses the race rather than removing it, and hands the loss to the role that cannot absorb it: tmux announces a pane's death only by closing its window, so a program pane bornonand dying inside the round trip is a death that is never announced. Chaining the option into the creating command list removes the race for both.Not in this PR
Two additive plugin APIs —
keys(typing into a program a plugin already started) andon_context(the right mouse button) — were in this branch and have been taken out of it. They want their own review and neither is touched byprogram.exited; each comes back as its own PR with the API described in the body.