Conversation
A run killed before its backend minted a session id — most often a cezar restart while it queued for the repository working tree — had nothing for Continue to reattach to. Recovery gave up, the composer went read-only, and the only action left on work that was genuinely in flight was Delete. Continue now covers that case by opening a NEW session briefed with the old one's record: the original task, how the previous attempt ended, and a bounded replay of the conversation. Resuming is still preferred and still happens byte-for-byte as before whenever there is something to resume. Spec: .ai/specs/2026-09-11-continue-without-a-session.md
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code review — om-auto-review-pr
Verdict: CHANGES REQUESTED — 1 blocker (merge conflict), 1 major (the briefing loses "how the previous session ended" on the restart-recovery path, which is the reported scenario), plus 1 minor and 3 nits.
The change itself is well argued and well tested: the collapse of resume ? sessionStep.sessionId : undefined into one resumeSessionId genuinely does make "nothing recorded" and "an id this runner/account cannot resolve" one rule, the hasSession gate is removed from exactly the surfaces that no longer need it and kept on the two that do (Terminal, open-in-cli), and moving the usage-limit gate to fire time in fireAutoResume is a real catch — reconcileAutoResumes arms from the record, so without it relaxing continueRun would have quietly weakened an on-by-default unattended automation. The guard test that passes both with and without the change is the right shape for pinning "a run that has a session still resumes it byte-for-byte".
Caveat: the head conflicts with
main, so this review is of the diff as pushed. Resolving the conflict may changerun-actions.test.ts; re-check whatever the resolution touches.
⛔ Blockers
1. The head cannot merge — it conflicts with main.
mergeable: CONFLICTING / mergeStateStatus: DIRTY at e29dba6f against main e8c95f3a. Conflicting paths, confirmed locally with git merge-tree:
CHANGELOG.md— ordinary churn in the# Unreleased→## 🐛 Fixesblock.packages/web/src/routes/task-thread/run-actions.test.ts—#938("pin tasks to the top",44a8dbba) landed apinflag onRunActionFlagsand edited the same visibility-matrix comment block this PR rewrites.
Both are mechanical (keep both sides), but they block the merge. Merge main forward, then re-run the gate — run-actions.ts needs pin: !run.archived and continueRun: !active in the same object.
🔴 Majors
2. Restart recovery — the exact scenario this PR fixes — briefs the fresh agent without telling it the previous attempt was interrupted.
packages/cezar/src/workflows/run.ts:2131-2136 (the deferForCapacity branch of continueRun) writes error: undefined on the record before the continuation is dequeued. runContinuation then builds the briefing from that record (run.ts:2189-2192), so input.error is undefined and buildSessionRecap skips the whole section (session-recap.ts:354).
recover() is a deferring caller — this.continueRun(run.id, {...}, true) at run.ts:1116-1122 — so every post-restart continuation takes this path.
Verified on this branch with a probe test (two runs, same fixture, one per path):
| Path | briefing has ## Previous session (cezar) |
briefing has How the previous session ended |
|---|---|---|
manager.continueRun(id, {text}) — user presses Continue |
✅ | ✅ |
manager.recover() — cezar restarted |
✅ | ❌ (record.error is undefined by then) |
Failure scenario, end to end — the one in the PR description: cezar is killed while a task queues for the repository working tree, so no session id exists. On boot, recover() continues it in a fresh session. The agent receives the original task and then Nothing was exchanged — the session ended before the agent produced any output. — and is never told that interrupted — cezar process exited during the run. That one line is the whole difference between "the previous attempt crashed mid-flight, the worktree may hold partial work" and "the previous attempt ran and produced nothing", and it is the fact the agent most needs before deciding whether to re-do work. Both the spec (.ai/specs/2026-09-11-continue-without-a-session.md, Architecture) and the CHANGELOG promise the briefing carries "how the previous attempt ended".
It is also a test-coverage hole, which is why the suite reads as covering this. continue-without-session.test.ts asserts opening?.userText contains interrupted — cezar process exited during the run, but only in the case that calls manager.continueRun(...) directly (deferForCapacity defaulted to false). The recover() case asserts the lifecycle lines and that continue-1 exists, and never looks at the briefing — so the one assertion that would catch this is on the one path where it holds. A regression test belongs on the recover() path specifically.
Fix direction (author's call): either carry error on PendingContinuation so it survives the defer, or stop clearing it in continueRun's deferred branch and let runContinuation clear it at run.ts:2234-2240, where it already does — that line runs after the recap is built. Note the same gap reaches reviveQueuedRun (run.ts:989-1016), which rebuilds a PendingContinuation from a record whose error was cleared before the restart; there the original error is only recoverable from the NDJSON lifecycle events, so it is worth deciding deliberately what that path should say.
🟡 Minors
3. ask-answer.ts:160-167 still refuses to reroute a dropped answer when no session was recorded — the rule this PR removes everywhere else.
The live→resume fallback on a 409 from POST /messages is gated on lastSessionId(run) !== undefined. That gate existed because /continue used to refuse without a session; it no longer does. So for a record that still looks active but whose step has not recorded a session id — the precise window this PR exists for — a typed ask answer or a one-click "Resolve conflicts" prompt surfaces session closed and is lost, where a continuation would now carry it.
Reachability is genuinely low (the engine seeds a session id at step start, and deferMessage buffers the starting-up case), so this is a minor rather than a major — but the guard now contradicts the rule the rest of the change establishes, and the next reader has no way to tell which of the two is intended.
🔵 Nits
4. follow-up-engine.tsx:64-65 — stale comment. "Only a run that can actually be continued fetches at all — every other thread (running, queued, closed with no session) would be fetching it to render nothing." A closed run with no session is continuable now, so it does fetch the model catalog. The comment names the very case the PR changed.
5. session-recap.ts:339-364 — "Nothing was exchanged" can be asserted about a conversation that existed. If maxChars is ever below one clipped message, the drop loop empties kept and the else branch claims the session produced no output. Unreachable on the defaults (a clipped line is at most 2 021 chars against a 12 000 budget) and SessionRecapLimits has no non-test caller — but both are exported, so keeping the last message (or emitting the …N earlier messages omitted… note with an empty body) would make it safe by construction rather than by arithmetic.
6. The task is restated twice in the deferred opening prompt. hydrateQueuedContinuation (run.ts:1690-1692) already appends Current task and queued updates:\n\n{task}, and the briefing prepends **Original task**\n\n{task} in front of it. Cosmetic, but it doubles the most-repeated block of the prompt on exactly the recovery path.
✅ Checked and clean
BACKWARD_COMPATIBILITY.md— no protected surface broken.POST /api/v1/runs/:id/continuekeeps its shape and only accepts a case it used to reject (a widening, per §2's additive rule);POST /api/v1/runs/:id/open-in-clistill answers409 no agent session to resume(server.ts:3874); no v1/v2 eventtyperemoved (noteis existing vocabulary, §7); noRunRecordfield added or made required (§3).AskDeliveryMode/AskBlockedReasonnarrow, butpackages/webships in lockstep with the server and is not a published surface.- Secrets —
RunRecord.errorgoes throughredactPatch(store.ts:692-699) and events are redacted on write, so the replay carries already-scrubbed text. Nothing new reaches disk. - No stale gates left behind — every remaining
hasSession/lastSessionIdguard inpackages/webis Terminal/open-in-cli(run-actions.ts:118,git-actions.ts:129,task-changes.tsx:113) or the minor above; no'unavailable'/'no-session'reference survives outside unrelated features. askDeliveryMode's rewrite is behaviour-preserving — it droppedrunActionFlags(run).continueRun, which is now exactly!isRunActive(run.status), so the two spellings agree by construction rather than by luck.reviveQueuedRun's widened branch — with nosessionStep,sessionBackendfalls back tobackend, so the equality holds andsessionIdisundefined; a fresh queued run never carries acontinue-step, so the workflow-revival fallback is unreachable only for genuine continuations.- Test quality —
session-recap.test.tscovers ordering, coalescing, exclusions, per-message clipping, the drop-oldest budget and the empty conversation; the engine suite pins that no--resumereaches the CLI and that the transcript persists the user's prompt alone. Confirming three of the four engine cases red withrun.tsstashed is the right evidence to have gathered.
🧪 Validation gate
Run in an isolated worktree at the PR head (e29dba6f), npm ci, TMPDIR=/tmp.
| Command | Result |
|---|---|
npm run typecheck |
✅ pass |
npm test |
|
npm run test:unit |
✅ pass |
npm run build |
✅ pass (incl. check:pack) |
npm run test:package |
✅ pass |
The two failures are settings/accounts-defaults.test.tsx › "survives a server that has never heard of agent defaults" (expected [] to have a length of 5) and task-git/task-files.test.tsx › "renders the header with Files active…". Neither file is in this diff and neither imports anything it changes; both pass when re-run on this same head in isolation, so they are load-dependent waitFor flakes in the full run, not a regression here. Worth a separate flake issue.
No required-check signal to add: branch protection is not readable on main (404), and the only check reported on this PR is license/cla — SUCCESS. Nothing was pending at verdict time.
Next: merge main forward to clear the conflict, then close the major — carry the run's error into the deferred continuation and pin it with a regression test on the recover() path. The minor and the three nits are yours to take or decline; each one that is declined is fine, it just needs a reason in the thread so it does not come back next review.
pat-lewczuk
added
changes-requested
|
🤖
|
|
@patzick, two things before this can merge — full review:
One minor ( Push the update and re-request review. |
|
🤖 1 blocker (merge conflict against autofix: skipped (not my PR — re-run with --autofix to fix it here) |
Reconciles this branch with #954 (`feat(tasks): switch runners with persisted context`), which landed the same mechanism from the other direction: a fresh session briefed with the previous one's conversation, for a runner/account switch. Rather than keep two builders with two budgets and two marker policies, this branch's `runs/session-recap.ts` is deleted and the fresh-session Continue now opens on `workflows/continuation-context.ts`. #954's builder is the better of the two — it reads the normalized v2 message stream, strips CEZ: markers (a replayed CEZ:DONE would close the session it was handed to) and carries branch/worktree/step state. What this branch keeps is everything that made the briefing REACHABLE for a run that never recorded a session at all: the `continueRun` refusal, the cockpit's `hasSession` gate, the ask-delivery dead end, and `reviveQueuedRun`'s workflow-restart fallback. `recover()`'s no-session branch is dropped as unreachable: #972 now re-queues a `running` run that never reached an agent, above the continuation — a better exit than continuing a task that had done nothing yet. Conflicts: run.ts (took main's briefing wiring), CHANGELOG.md (main cut 0.10.1 and reformatted to one-line entries, so this change re-opens `# Unreleased` in the new format), run-actions.test.ts (comment, kept both notes). Also fixes `agent-profile-wiring.test.ts`, which arrived from main already failing: #972 added CEZ_BIN and CEZ_API_URL to the base run env without updating that assertion. Not part of this branch's change.
Four commits, none of which touch this change's seams: per-message timestamps (#942), copy-branch-name (#956), sidebar group ordering (#953), base-branch picker filter (#973). One conflict, CHANGELOG.md, and only because both sides added to `# Unreleased` — main two features, this branch one fix. Kept both, in the section order the released blocks use, and restored this entry to PROSE: the one-line format (#963) is how RELEASED entries read, while `# Unreleased` is authored long and condensed at release time, which is what main's own new entries do. The text is also brought up to date with the post-#954 design — it no longer claims a restart-recovery change this branch dropped as unreachable.
… case CI failed on `agent-profile-wiring.test.ts`, and my previous commit's claim that it "arrived from main already failing" was wrong — it fails only when the suite is run from INSIDE a cezar task. `agentEnv` reads `CEZ_API_URL`/`CEZ_BIN` off the ambient `process.env` (set by `serveCommand`, #972). A plain shell and a CI runner have neither, so the exact key-set assertion holds; vitest started by a cezar-served task inherits both, `agentEnv` adds two keys, and the assertion fails. I had "fixed" that by adding the two keys to the expectation, which is the reverse of correct and broke CI. The expectation goes back to the six base keys, and the case now clears both variables in `beforeEach` and restores them in `afterEach` — which is what the test is actually about: the zero-config env, not whatever started vitest. Verified passing with the variables both set and unset.
|
Correction to an earlier claim in this PR: I wrote that It fails only when the suite is run from inside a cezar task.
|
📦 npm preview published —
|
patzick
added
review
2 participants
Problem
Reported from the thread of a real run:
…with the composer underneath reading
Session closed — no session to resume.Nothing about the task was broken. The worktree, the branch, the handoff file and the user's prompt were all intact — the run simply died in the window between "accepted" and "spawned", which is precisely where a session id does not exist yet. Three surfaces then agreed the task was over, and the only action left was Delete.
What changed
Continue covers that case now by opening a new session briefed with the old one's record. Resuming is still preferred and still happens byte-for-byte as before whenever there is something to resume.
packages/cezar/src/runs/session-recap.ts(new) —buildSessionRecap({task, error, events}). Pure, store-free, separately testable. Coalesces consecutive same-speaker events back into turns (the engine appends onetextevent per streamed chunk), clips per message, then drops from the front until under budget and says how many it dropped.user-message+textRunManager.continueRundrops the refusal.resume ? sessionStep.sessionId : undefinedcollapses into oneresumeSessionIdwhoseundefinedmeans "open a new session" for either reason — nothing recorded, or a runner/account switch that strands the id inside another config dir. One rule covers both, so neither can drift.RunManager.runContinuationbuilds the briefing before it appends this continuation's ownuser-message(so the replay cannot contain the prompt it is being prepended to), appends anotesaying the session is fresh, and prepends the briefing behind a---rule. Delivery-only: the transcript persists the user's words alone, because the thread already renders every line the briefing replays.RunManager.recoverkeeps its lifecycle line honest —resuming the interrupted task from its last sessionvsno session to resume; continuing the interrupted task in a fresh session.reviveQueuedRunadopts a pendingcontinue-Nstep whether or not a session precedes it. It used to fall through to reviving the workflow, re-running the task from step one against a worktree that already held its results.Cockpit.
runActionFlags.continueRunis!active;terminalkeeps!active && hasSession, because handing a shellclaude --resume <id>needs an id that exists. The Continue tooltip and the composer placeholder distinguish the two promises —Reopen the sessionvsStart a new session — the previous conversation is replayed to the agent— since only one of the two paths can claim "pick up where you left off".askDeliveryMode'sunavailablemode and theno-sessionblocked reason are removed: every closed run can take an answer now, so the ask card has no inert state left to render.The one deliberate exception
The usage-limit auto-resume stays session-only, and this PR moves that gate to fire time as well.
scheduleAutoResumeIfLimitedalready checked for a session, butreconcileAutoResumesarms from the record, so a deadline that survived a restart never passed back through it — without a fire-time check, relaxingcontinueRunwould have silently weakened an on-by-default unattended automation. Restarting a conversation from a summary is a thing a present user may ask Continue for; it is not a thing a timer may decide hours later with nobody watching.What does NOT change
--resumeargument, no briefing — it is already inside that conversation. Pinned by a guard test that passes both with and without this change.POST /api/v1/runs/:id/open-in-clistill answers409 no agent session to resume.POST /api/v1/runs/:id/continueis unchanged in shape. It accepts a case it used to reject (a widening, not a break); its other 409s are untouched.Testing
packages/cezar/src/runs/session-recap.test.ts— ordering, coalescing, exclusions, clipping, the drop-oldest budget, the empty-conversation case.packages/cezar/src/workflows/continue-without-session.test.ts— end to end through the engine underCEZ_DRY_RUN=1: the briefing reaches the agent, no--resumereaches the CLI, the transcript persists the user's prompt alone,recover()continues instead of giving up, and the guard case above.Three of those four engine cases were confirmed red with
workflows/run.tsstashed; the guard case passes both ways, which is what it is for. The web suites' updated cases were confirmed red the same way.Targeted validation: 33 files / 786 tests green across every touched area (session-recap, continue-without-session, run, auto-resume, recover-*, continue-run, open-in-cli-resume,
task-thread/*, reference-conflict-action), plusnpm run typecheckandnpm run test:unitclean. Full suite left to CI.Two failures investigated and ruled out by baselining against stashed sources:
server/automations-gate.test.ts"starts once the flag is on" is a pre-existing 50 ms race (fails 4/4 without this change), and the "outside a git repository" / "clean tree" cases fail only whenTMPDIRpoints inside the repo — all pass underTMPDIR=/tmp.Docs
New spec
.ai/specs/2026-09-11-continue-without-a-session.md(with the resolved-assumptions table behind each knob above), a CHANGELOG entry under# Unreleased, and superseded notes in.ai/specs/2026-07-21-queued-session-prompt-stacking.mdwhere the old composer copy was documented as current.