Skip to content

feat: add restore capture, filling the capture slot on demand - #452

Open
ssgreg wants to merge 1 commit into
umputun:masterfrom
ssgreg:feat/restore-capture
Open

feat: add restore capture, filling the capture slot on demand#452
ssgreg wants to merge 1 commit into
umputun:masterfrom
ssgreg:feat/restore-capture

Conversation

@ssgreg

@ssgreg ssgreg commented Aug 18, 2026

Copy link
Copy Markdown

Adds restore capture, which fills the restore slot with every pane's running command now instead of at the next clean quit.

$ agtermctl restore capture
captured 1 pane
$ agtermctl restore capture --json
{"result":{"count":1,"text":"captured 1 pane"},"ok":true}

restore clear drops those captured commands and session restore pins 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 captureForegroundCommands the quit path runs and persists through the same saveAllOpen: 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:

$ agtermctl restore capture
error: "Restore running commands on restart" is off, nothing was captured

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.

count is 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 own result.text too, 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 cmd now names itself in the error instead of answering "the data couldn't be read".

The two UI tests start their foreground tee through session.type rather than typeText. Event synthesis needs the machine running the tests to be allowed to post events, session.type needs 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/testRestoreCaptureSurvivesForceQuit plus testRestoreCaptureRefusesWhenTheSettingIsOff. Checked on a live isolated instance rather than reasoned about: sleep 12345 in a pane, capture reports 1, the window snapshot on disk carries ["sleep","12345"], then kill -9, and the relaunch re-runs it with the slot cleared after consumption.

Docs updated in site/commands.html and site/docs.html, the bundled skill (SKILL.md, reference.md, examples.md), .claude/rules/control-api.md and .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.html paragraph now states both that refusal and this command.

Follows #447.

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ssgreg
ssgreg force-pushed the feat/restore-capture branch from c1fbdae to 0a8f9e5 Compare August 19, 2026 06:10
@ssgreg

ssgreg commented Aug 19, 2026

Copy link
Copy Markdown
Author

Both blockers and the count are already in the branch, and your line numbers say why they read as open: ControlServer.swift:507-514 is captureRestoreCommands as it stood in the first push, before the gate moved the function down. c1fbdae went up at 10:50Z, the review is stamped 17:58Z, so GitHub attached the newer head to a read of 982698d. Head is now 0a8f9e5, with one thing added that you were right about.

The count. ControlServer.swift:520-521 builds the sentence and always sets result.text; count stays for --json.

$ agtermctl restore capture
captured 1 pane
$ agtermctl restore capture --json
{"result":{"count":1,"text":"captured 1 pane"},"ok":true}

It also counts what the call wrote rather than the slots holding argv once it returns: AppDelegate.swift:349-361 increments per assignment and nils the split slot when isSplit is false, so a hidden or closed split cannot inflate the number.

closeSplit. AppStore+Panes.swift:95-96 clears splitForegroundCommand and pendingSplitForegroundCommand beside the two pin clears, pinned by AppStorePaneTests.closeSplitDropsTheCapturedSplitCommand.

Your question. Deliberate, and the opposite way round from what you read: with the master setting off the command now refuses and captures nothing.

$ agtermctl restore capture
error: "Restore running commands on restart" is off, nothing was captured

Exit 1. A session.restore pin outlives the toggle, so note-and-succeed fits there; a capture nothing will replay only goes stale, and persisting argv for someone who opted out was the part worth removing. The difference is recorded in .claude/rules/settings.md.

The framing. Gone from all four places you listed plus site/docs.html, which promised "a force-quit or crash captures nothing" with no way out. They now name a force quit, a crash, a hard reset, a power loss, and say a shutdown, restart or logout captures by itself since #447.

Staleness. That one was genuinely missing. MiscCommands.swift:76 carries it after the scheduled-job advice, with the pointer to restore clear and restore-denylist.conf, and reference.md the same clause.

Tests. Hosted coverage over a real ControlServer, WindowLibrary and SettingsModel sits in agtermTests/ControlServerRestoreCaptureTests.swift: refuses with the setting off, writes nothing to disk in that state, and reports the pane count in its own text. I put it beside ControlServerPickTests and ControlServerWorkspaceCommandsTests rather than inside the 654-line ControlServerSessionActionsTests, since those two set the same per-feature precedent; say the word and I fold it in. Two UI tests cover the SIGKILL round trip and the refusal, and they start the pane's foreground through session.type instead of typeText: event synthesis needs the machine running the tests to be allowed to post events, session.type needs nothing, so they run where a typing test cannot.

make test 2581, make test-app 250, make lint, make build, both UI tests green.

Read-back skipped, and agreed on the reasoning: that is a decision for both commands together.

@ssgreg
ssgreg force-pushed the feat/restore-capture branch from 0a8f9e5 to 8686a84 Compare August 19, 2026 06:30
@ssgreg

ssgreg commented Aug 19, 2026

Copy link
Copy Markdown
Author

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 site/docs.html, where your 131dfc6 sentence about a control byte or lossy UTF-8 starting a plain shell met my clause about restore capture covering the force-quit case. The resolved paragraph carries both. 131dfc6 also closes #454, and wider than I reported it, so the capture inherits that refusal at replay rather than needing its own guard.

make test 2601, make test-app 250, make lint, make build, both UI tests green on the new base.

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ssgreg
ssgreg force-pushed the feat/restore-capture branch from 8686a84 to ab9dc3f Compare August 19, 2026 06:55
@ssgreg

ssgreg commented Aug 19, 2026

Copy link
Copy Markdown
Author

The blocker is real and it was mine. Head is ab9dc3f.

WindowAccessor.swift:156 is now } else if !library.isTerminating {, exactly your fix, and the comment records why the terminating path belongs to neither arm.

Two proofs, since "every ⌘Q restores plain shells" deserves more than a diff. First an isolated instance (AGTERM_STATE_DIR, own socket) quit by an Apple Event addressed to that pid alone, sleep 12345 running in the pane:

before the fix:  foregroundCommand on disk: []
after the fix:   foregroundCommand on disk: [["sleep","12345"]]

Second a regression case on the quit path, testRestoreCaptureSurvivesCleanQuit: capture, gracefulQuit, assert the argv survived on disk, relaunch, assert it re-ran. It fails on the old code at the on-disk assertion.

I also owe you a correction on my last comment. I said the typing tests could not run on my machine; a stale testmanagerd from an earlier run was eating the synthesized events, and after restarting it the whole class runs: agtermUITests/RestoreCommandUITests, 23 tests, 0 failures, 220s, testRestoreReRunsForegroundCommand among them. Running that class is what would have caught this before you did, and it is on me that I concluded "environment" instead of digging. The session.type seam stays in the two new tests, since it is faster and needs no event synthesis, but not as a workaround for something broken here.

On the minors:

saveAllOpen swallowing the result: agreed, and worth doing rather than documenting. WindowLibrary.saveAllOpenChecked() attempts every store and reports whether all of them landed, saveAllOpen() is that with the result discarded so the two cannot drift, and restore.capture now answers ok: false with "captured N panes but the save failed, so nothing reached disk". WindowLibraryTests.saveAllOpenCheckedReportsAFailedWrite pins it with the unwritable-windows-directory lever your stale-file test already uses.

The vacuous hosted case: rewritten as testTheRefusalLeavesAnEarlierCaptureAlone. It presets ["sleep","12345"] in every slot, so deleting the gate runs the capture over a store with no surfaces, assigns nil, and fails the test. An all-nil fixture could not.

testWindowCloseCapturedCommandDoesNotReplayOnMidRunReopen: it now calls restore.capture first and asserts the argv reached the window file before the close, so the clearing branch is exercised full-to-empty rather than nil-to-nil.

The site/docs.html conflict is gone: I rebased onto 2683dbd before this review arrived, and the resolved paragraph carries both your 131dfc6 sentence and this command. 131dfc6 also closes #454, wider than I filed it, so the capture inherits that refusal at replay.

make test 2602, make test-app 250, make lint, make build, RestoreCommandUITests 23/23.

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

umputun added a commit that referenced this pull request Aug 19, 2026
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.
@ssgreg
ssgreg force-pushed the feat/restore-capture branch from ab9dc3f to 497b7d3 Compare August 20, 2026 09:15
@ssgreg

ssgreg commented Aug 20, 2026

Copy link
Copy Markdown
Author

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 splitForegroundCommand too and asserts that one first, since it is the slot the capture's else nils unconditionally for a session with no shown split. The main slot never reaches its assignment without a GhosttySurfaceView, so you were right that it pinned nothing, and the comment now says which slot does the work.

Checked by mutation rather than by reading: with the guard at ControlServer.swift:513 deleted, make test-app reports 4 failures and testTheRefusalLeavesAnEarlierCaptureAlone is among them. With it back, 250 pass.

make test 2602, make test-app 250, make lint, make build.

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

umputun added a commit that referenced this pull request Aug 20, 2026
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A control byte in a captured argv breaks the restored line: the pane comes back stuck at quote>

2 participants