2 Commits

Include renames

Author	SHA1	Message	Date
Michael GrundbergandClaude Opus 5	39976041e0	fix(sessions): let a dismiss reach the entries a restore is holding ... Fourth review of the reboot-restore branch, and the third to find a defect in the previous round's fix. This one is the same shape as its predecessor: a counter keyed on one thing, compared against a set keyed on another. The generation counter was indexed by the entry's owner, while the in-flight set holds the caller doing the restoring. Those are the same person exactly when a user restores their own sessions, which is every case the tests covered. The route deliberately supports the other case: an admin may spend another user's entries. So when an admin restored Bob's sessions and Bob dismissed the banner, nothing matched, the entries came back, and a plan Bob had explicitly dismissed was re-armed for another twenty-four hours. Rather than reconcile the two key spaces, the counter is gone. `take()` now parks the entries it hands out, remembering which caller is spending them, and they stay parked until that restore ends. A dismiss filters the parked entries by `canAccess(entry.owner)` — the same predicate it already applies to the plan — so it reaches them wherever they are. `releaseFlight()` puts back only what is still parked. Expiry and a fresh boot plan unpark everything, for the same reason. There is one key space now, the entry's owner, and the spender is only ever used to tell two concurrent flights apart. That removes `generations`, `snapshotGenerations()`, `bump()`, `bumpAll()` and the argument threaded through the route. The discard grew the teardown it still lacked. A rebuild can fail after startInteractive() resolved, and a restored workspace still carries Codeman's hooks, so the CLI can post a hook event within milliseconds; the transcript watcher that starts from it, the attachment registry, the wait registry and the approvals inbox all outlive the listeners and would meet the retry, which reuses the session id by design. Its steps also run in reverse order now, so no live listener can reach a tracker that has already stopped, and the mux kill has its own guard, because stop() kills the pane in its last block after destroying four trackers. Tests. The run-summary test named an interval and asserted a map entry, so dropping stop() left it green; it now spies on stop(). Nothing pinned that before-spawn must precede setupSessionListeners, which reads the flag that phase restores, so swapping the two lines was silent; the ordering test now includes the listener setup. The retry assertion was a tautology and now asserts a different refs object. Both strengthened tests were verified by reverting their fix. Two new tests cover the admin-restores-another-owner cases this round was about. The server in the discard test is built once and stopped, since its constructor registers handlers on module-level watchers, and the workspace is removed through safeRmHomeTree. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-16 16:51:00 +02:00
Michael GrundbergandClaude Opus 5	71ed7b127c	fix(sessions): make the discard a real inverse of the construction ... Third review of the reboot-restore branch. The narrow discard the previous commit introduced avoided everything cleanupSession() did wrongly, and in dropping so much of it also dropped four things it had to keep. The worst broke the retry the whole design rests on. setupSessionListeners() returns early while sessionListenerRefs still holds the session id, and the discard never cleared that entry. So the advertised flow — a rebuild fails because the agent binary is missing, the user fixes their PATH and clicks again — reused the same id, wired no listeners at all, and produced a tab that never showed output, never updated its status and never persisted. That is worse than the leak the discard was added to prevent. Three more registrations leaked with it: a RunSummaryTracker and its interval, an image watcher on the workspace, and the Ralph fix-plan watcher. The discard now undoes each registration setupSessionListeners() makes, in its order, and the per-session custom-model config directory, which holds the endpoint's API key literally and which nothing else would ever remove. The image-watcher flag was restored after the code that reads it, so a session came back reporting the feature as on with nothing watching. It moves to the before-spawn phase, and that phase now runs before the listeners rather than after them. The generation counter that lets a mid-restore dismiss win was global while clear() is ownership-scoped, so one user's dismiss discarded another user's unspent entries, permanently, because nothing rebuilds an in-memory plan. It is now per owner. Bumping only the owners of entries the dismiss removed was not enough either: take() has already emptied the plan by then, so a dismiss landing mid-restore saw nothing of that owner's to remove and invalidated nothing. The owners that matter are those with a restore in flight, filtered by what the dismissing user may access, and that is what clear() now bumps. Plan expiry bumps too, so a restore straddling the 24-hour boundary cannot hand entries back and give an expired plan another full day. Tests. discardPartiallyBuiltSession had no test at all: the only implementation any test ran was the mock's one-line stub, which is why every defect above was invisible. test/discard-partially-built-session.ts drives the real WebServer, and the retry assertion fails if the listener refs are left behind — verified by reverting the fix. The dismiss-race test drove the registry by hand, so deleting the route's generation argument left it green; it now goes through the route, and two further tests cover the multi-user cases. The mock context has now gone stale twice, because route tests pass it as `ctx as never` and tsconfig.json includes only src, so nothing ever compares it to the ports. A type-level guard is therefore inert — I wrote one and confirmed it never fires. test/mocks/mock-route-context-completeness.ts compares the mock's keys against WebServer.createRouteContext() at runtime instead, and names what is missing. Also: the API reference now says workspace-forbidden is judged against the owner's grant, the banner's module header no longer claims Restore always dismisses it, and the detail span gets the same min-width: 0 the phone rule already needed. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-16 15:34:47 +02:00