feat: add restore capture, filling the capture slot on demand - #452
feat: add restore capture, filling the capture slot on demand#452ssgreg wants to merge 1 commit into
Conversation
982698d to
c1fbdae
Compare
umputun
left a comment
There was a problem hiding this comment.
two blockers, the rest is minor. The verb itself is right, restore clear and session restore had no counterpart that fills the slot.
first, the framing. The policy-driven macOS update case is already covered since #446/#447, your own fix: QuitReason.isSystemQuit returns .terminateNow for shutdown/restart/logout, so that exit reaches applicationWillTerminate and captures normally. What is left uncovered is a crash, kill -9, power loss. That is still worth a command, but it needs to be what the docs say.
blocking
agterm/Control/ControlServer.swift:507-514 - the pane count goes out in result.count, and the CLI renders a bare count as count == 0 ? "ok" : "N diagnostic(s)" (SocketClient.swift:205). So three captured panes print 3 diagnostic(s), and with the setting off text wins and the count never prints at all. .claude/rules/control-api.md:111 reserves count for diagnostics/search. Fix: build the human string and always set result.text, keep count for --json, same shape as session.search at ControlServer+SurfaceIO.swift:445.
agtermCore/Sources/agtermCore/AppStore+Panes.swift:79-103 - closeSplit clears splitRestoreCommand and pendingSplitRestoreCommand but not splitForegroundCommand, while closePrimaryPane:148 does clear it. That field used to be written only at teardown, this PR makes it live mid-run. Capture with a split running tail -f app.log, close the split, ⌘D a new one later, then crash: the next launch types tail -f app.log into a pane that never ran it. Fix: nil it next to the two pin clears at :92-93.
minor
agterm/Control/ControlServer.swift:507-509 - count sums non-nil slots after the capture, not what the capture wrote, so a split captured earlier and since hidden or closed still counts. Your own doc comment at :502 says "main and shown split each counting one". Fix: gate the split term on $1.isSplit && $1.splitForegroundCommand != nil.
MiscCommands.swift:65-74, reference.md:1169, SKILL.md:485, site/commands.html:2616 - all four lead with the macOS-update case above. Fix: drop that example, keep crash / kill -9 / power loss.
MiscCommands.swift:63-77 - the "run it from a scheduled job every so often" advice has no staleness note. A capture is only as fresh as its last run, so a pager or a build that finished since still re-runs after a crash. One clause covers it, plus a pointer at restore clear and restore-denylist.conf.
tests - dispatcher routing and CLI parse only, nothing reaches captureRestoreCommands and nothing renders its response, which is why the diagnostic(s) output went unnoticed. restore.clear has restoreClearRoundTrips (ControlProtocolTests.swift:1305) and testRestoreClearSucceeds (ControlAPIUITests.swift:144). The one that pays here is a hosted case in ControlServerSessionActionsTests.swift, it already stands up a real ControlServer over a real WindowLibrary and SettingsModel, so the count and the setting-off text are testable without a UI test.
one question
both existing capture arms gate on restoreRunningCommand (AppDelegate.swift:322, WindowAccessor.swift:154), this one captures regardless and reports it in result.text. Is that deliberate? It means argv gets persisted for someone who turned the feature off, and it replays if he flips the setting on before the next launch.
on the read-back you offered: skip it. Matching restore.clear is the right call, and whether those slots become a public read surface is a decision for both commands at once, not something to bolt onto this one.
c1fbdae to
0a8f9e5
Compare
|
Both blockers and the count are already in the branch, and your line numbers say why they read as open: The count. It also counts what the call wrote rather than the slots holding argv once it returns: closeSplit. Your question. Deliberate, and the opposite way round from what you read: with the master setting off the command now refuses and captures nothing. Exit 1. A The framing. Gone from all four places you listed plus Staleness. That one was genuinely missing. Tests. Hosted coverage over a real
Read-back skipped, and agreed on the reasoning: that is a decision for both commands together. |
0a8f9e5 to
8686a84
Compare
|
Rebased onto master (2683dbd), single commit 8686a84. That is also why the previous push showed no CI: the PR had gone CONFLICTING, so GitHub could not build the merge ref and never created a run. It is MERGEABLE again and the run is going. The only conflict was
|
umputun
left a comment
There was a problem hiding this comment.
head 0a8f9e5 breaks the clean quit path, and not just for restore.capture callers: every ⌘Q with the setting on now restores plain shells. testRestoreReRunsForegroundCommand passes on master and fails here at :51.
blocking
agterm/Views/WindowAccessor.swift:156-167 - the new else fires whenever ANY of the three conditions on :153-154 is false, and one of them is !library.isTerminating. Clean quit order: applicationWillTerminate sets the flag, captures every pane, and saveAllOpens it (AppDelegate.swift:319-327); willClose then runs with the store still loaded, since closeWindow no-ops under that flag, so library.isOpen on :147 is true; the else nils foregroundCommand, splitForegroundCommand and the pending fields, and store.save() on :167 writes nulls over what :327 just persisted. testRestoreCaptureSurvivesForceQuit cannot see it, kill -9 never fires willClose.
three runs of that one test: master green, head red at :51, head plus } else if !library.isTerminating { green. That is also the fix, the terminating path has to keep the quit-time capture. Worth a regression case that exits with ⌘Q rather than SIGKILL, since nothing today covers this branch on the quit path.
minor
agterm/Control/ControlServer.swift:519-522 - saveAllOpen goes through AppStore.save, which throws away saveChecked's Bool (AppStore.swift:920-922), so the command answers ok and prints a count with nothing on disk. AppStore.swift:924-926 states that contract and names setRestoreCommand as the caller keeping it today. A checked saveAllOpen does not exist yet, so this is more than a one-liner, your call whether the one command whose whole point is that the write reached disk should carry it.
agtermTests/ControlServerRestoreCaptureTests.swift:53-60 - testWritesNothingWithTheSettingOff presets every foregroundCommand to nil, then asserts they are nil. A hosted store has no GhosttySurfaceView, so nothing can write one and the case stays green with the gate deleted. testRefusesWithTheSettingOff already pins the gate.
agtermUITests/RestoreCommandUITests.swift:357 - testWindowCloseCapturedCommandDoesNotReplayOnMidRunReopen never calls restore.capture, so the new clearing branch is only ever seen nil to nil, and deleting the else leaves it green. Capture first, assert the argv reached the window file, then close.
merge - conflicts with master in site/docs.html, the rest auto-merges.
everything from round 1 checks out at head.
8686a84 to
ab9dc3f
Compare
|
The blocker is real and it was mine. Head is ab9dc3f.
Two proofs, since "every ⌘Q restores plain shells" deserves more than a diff. First an isolated instance ( Second a regression case on the quit path, I also owe you a correction on my last comment. I said the typing tests could not run on my machine; a stale On the minors:
The vacuous hosted case: rewritten as
The
|
umputun
left a comment
There was a problem hiding this comment.
two minors, both one-liners. Last round's items check out at ab9dc3f.
minor
agterm/Control/ControlServer.swift:522-523 - "nothing reached disk" is false in two cases. saveAllOpenChecked returns false when ANY store fails, so with two windows open the first one's write can already be on disk. And the captured argv stays in the live foregroundCommand fields either way, so the next ordinary store.save() (session select, rename, split) writes exactly what the caller was told was never written. The ok: false itself is right, only the clause is wrong. Fix: say what happened, e.g. "captured N pane(s) but at least one window's save failed; the captured argv stays in memory and will be written by the next save".
agtermTests/ControlServerRestoreCaptureTests.swift:53-64 - testTheRefusalLeavesAnEarlierCaptureAlone still does not pin the gate. Line 55 presets foregroundCommand only, and the fixture's seeded session has surface == nil, so captureForegroundCommands never reaches the assignment inside if let view = session.surface as? GhosttySurfaceView. Delete the guard at ControlServer.swift:513 and this test stays green, which is the same shape as the all-nil version from round 2. The comment at :59-61 is wrong for the same reason: only splitForegroundCommand is written unconditionally, by the new else. Fix: preset session.splitForegroundCommand = ["sleep","12345"] as well and assert it survives, since that one does go red without the guard, then reword :59-61 to name the split slot as the one the else nils.
Surfaced reviewing PR #452, which adds the second copy: the non-last-window close arm spells out the reset ControlServer.clearRestoreCommands already writes, instead of a helper on Session.
`restore clear` drops the captured foreground commands on demand and `session restore` pins one on demand, but nothing creates one: the capture happens only at a clean quit, so an exit that never reaches applicationWillTerminate takes every pane's command with it. A force quit, a crash, a hard reset and a power loss all land there, which the docs concede as "a force-quit or crash captures nothing". A shutdown, restart or logout no longer belongs in that set: umputun#447 made it quit the app normally, so it captures by itself. restore.capture runs the same captureForegroundCommands the quit path runs, persists through the same saveAllOpen, and reports how many panes it captured a command for. Nothing else moves: same slots, replay stays launch-only and one-shot, and the denylist still decides what re-runs. App-global like its inverse restore.clear, so no --window selector. It refuses while "Restore running commands on restart" is off: unlike a session.restore pin, which outlives the toggle and is worth persisting, a capture nothing will replay only goes stale. The count is the slots the call actually WROTE, not a count read afterwards, which would report a stale hidden-split capture as a fresh one. It travels with its own result.text, because the shared human formatter prints a bare count as "N diagnostic(s)". A capture that can run mid-run breaks two invariants that used to hold by construction, both fixed here: closing a split left its captured command in the slot, where the next split would fire it, and a NON-last window close left argv in that window's file, which the never-windowless reopen fallback could replay. That clearing is scoped to a close OUTSIDE termination: at a clean quit applicationWillTerminate has already captured over live surfaces and saved, and closeWindow no-ops under the flag, so clearing there would write nulls over the capture and every quit would restore plain shells. restore.capture also acks only after the write lands: saveAllOpen swallows AppStore.saveChecked, so WindowLibrary gains a checked variant and the command answers with an error when the snapshot never reached disk. An unknown cmd now reports the DecodingError's debugDescription, so an agterm older than its agtermctl names the command it rejected instead of answering "data couldn't be read". Verified end to end in an isolated instance via AGTERM_STATE_DIR: `sleep 12345` running in a pane, `restore capture` reports count 1, the window snapshot on disk carries ["sleep","12345"], then kill -9 and the relaunch re-runs it with the slot cleared after consumption.
ab9dc3f to
497b7d3
Compare
|
Both right, both fixed at 497b7d3. The error clause now says what happened: "captured N pane(s) but at least one window's save failed; the argv stays in memory and the next save writes it". Your two cases are exactly why the old wording was a lie: the verdict is an AND across stores, and nothing rolls the fields back. The hosted case now presets Checked by mutation rather than by reading: with the guard at
|
umputun
left a comment
There was a problem hiding this comment.
two minors, both one-liners in files you already touched here. Round 3's items check out at 497b7d3, and the refusal test does pin the gate now: deleting the guard at ControlServer.swift:509 turns it red on the split-slot assertion, so that oracle is real this time.
minor
agterm/Control/ControlServer.swift:512 - an interactive capture records itself. While agtermctl restore capture blocks on the response it is that pane's foreground process group, so command(for:) reads its argv. isIdleShell keeps it (argv[0] is not a shell), the denylist has no built-ins and the seeded starter file is only tmux/screen/zellij, and shouldRestore refuses nothing here, so the slot gets ["agtermctl","restore","capture"] and the pane comes back running it, printing a stray "captured N panes". It re-arms itself too: at launch the replayed invocation is the foreground group again when the capture it triggers runs. Typing it at a prompt is how anyone tries the command before wiring the keybind or the job. One narrower case behind it: a session.new --command pane whose command has already exited gets hadForeground: true from this, and restorePlan then suppresses the initialCommand that would otherwise re-run.
An agent calling it from inside a session is unaffected, a tool-spawned child never tcsetpgrps, so the group leader is still claude/codex, which is the argv you want captured.
Fix: docs, not code. A built-in agtermctl filter contradicts "there is NO built-in list" and would suppress a legitimate agtermctl events read --follow, and per-connection pane attribution does not exist for an app-global command. One clause in MiscCommands.swift:63-82 and in reference.md's restore section: a capture typed at a prompt records the invocation in that pane, so prefer a keybind or a scheduled job, or restore clear after an interactive one.
agterm/Control/ControlServer.swift:334 - the decode-error drive-by does not do what its comment and .claude/rules/control-api.md:110-112 say. DecodingError's debugDescription is @available(macOS 26.4, *) and we deploy to 14.0 (project.yml:18), so below 26.4 String(describing:) falls back to the reflection dump, dataCorrupted(Swift.DecodingError.Context(codingPath: [...], debugDescription: "Cannot initialize Command from invalid String value restore.capture", ...)). The rejected cmd is still in there so the point survives, it just arrives as a dump rather than a sentence. It renders as the sentence on 26.x, which is why it looks right locally. swiftc -target arm64-apple-macosx14.0 on e.debugDescription is a hard error, so the rules bullet asserts something nobody can call at our deployment target.
Fix: pull the Context out of the four cases and use its debugDescription, version-independent and the same sentence on 14. Or keep String(describing:) and reword both the comment and the rules bullet to say so.
… window Verified against the code before filing. `saveIndex()` returns Void and swallows its own write failure (`WindowLibrary.swift:544`), and `bootstrap()` returns as soon as `loadIndex()` gives a valid index (`:568`), so `recoverOrphanedWindows()` at `:570` never runs for an index that parses but has lost an entry. The orphaned `windows/<id>.json` is then never scanned and the window is gone. Found while reviewing PR #452, which neither causes nor worsens it.
Adds
restore capture, which fills the restore slot with every pane's running command now instead of at the next clean quit.restore cleardrops those captured commands andsession restorepins one, but nothing created one. #447 took the shutdown and restart path off the table, agterm quits normally there and captures on its own. What stays uncovered is a force quit, a crash, a hard reset, a power loss, and each of them brings every pane back a plain shell.It runs the same
captureForegroundCommandsthe quit path runs and persists through the samesaveAllOpen: same slots, replay stays launch-only and one-shot, the denylist still decides what re-runs, app-global so no--window. Bind it or run it from a scheduled job and an exit nobody was there for restores like a deliberate quit.It refuses while the master setting is off, rather than writing argv a user who opted out will never see replayed:
That is the one place this command does not follow
session.restore, which succeeds with a note in the same state. A pin outlives the toggle and is worth keeping; a capture nothing will replay only goes stale.countis the slots the call itself wrote, not a tally taken once it returns: a session whose split is hidden or gone can still hold argv from an earlier capture, and a tally reports that as fresh. The response carries its ownresult.texttoo, or the shared formatter prints the total as "N diagnostic(s)".Two invariants held by construction until a capture could run mid-run, both fixed here. Closing a split left its captured command in the slot for the next ⌘D to fire. A non-last window close left argv in that window's file, which the never-windowless reopen fallback could replay. Unrelated but in the way: an unknown
cmdnow names itself in the error instead of answering "the data couldn't be read".The two UI tests start their foreground
teethroughsession.typerather thantypeText. Event synthesis needs the machine running the tests to be allowed to post events,session.typeneeds nothing, and a capture test only wants a live foreground process. The existing typing tests are untouched.Tested with
swift test(2581),make test-app(250),make lint,make build, and-only-testing:agtermUITests/RestoreCommandUITests/testRestoreCaptureSurvivesForceQuitplustestRestoreCaptureRefusesWhenTheSettingIsOff. Checked on a live isolated instance rather than reasoned about:sleep 12345in a pane, capture reports 1, the window snapshot on disk carries["sleep","12345"], thenkill -9, and the relaunch re-runs it with the slot cleared after consumption.Docs updated in
site/commands.htmlandsite/docs.html, the bundled skill (SKILL.md,reference.md,examples.md),.claude/rules/control-api.mdand.claude/rules/settings.md, which record the gate, the refusal and why the count is what it is.Rebased on master, so the capture inherits 131dfc6: an argv carrying a control byte or lossy UTF-8 is refused at replay and the pane comes back a plain shell. That closes #454 for an on-demand capture too, and the
site/docs.htmlparagraph now states both that refusal and this command.Follows #447.