Fix/stale summary jobs - #18
Merged
Merged
Conversation
The session-end hook stored the transcript, created a summary job and processed it in the same run. If that run died, the job stayed as it was and nobody ever picked it up again: 87 pending and 2 processing jobs had piled up, transcripts saved that would never get a summary. The hook now drains a small batch of those before handling the session that triggered it, so the queue empties on its own, session by session. It self-limits on both ends — a staleness margin above the hook's own safety timeout, and an attempts cap — so it neither delays the current session nor retries forever a job that can never be summarized. Completing a job was about to be written twice, so it was extracted to resolveSummaryJob and both paths now share it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every failed summary job carried the same text — "El resumidor no devolvió un resultado válido." — which told apart nothing: an agent that was never installed, a session that was never logged in and a reply that arrived but was useless all read the same. The agent's real output was right there and got thrown away. It is now stored as the error, and only an agent that returned nothing at all gets a message of its own, so a failed job is diagnosable without reproducing it. Both paths that resolve a job repeated that generic message separately, so both were changed: the hook's, and the panel's retry button, where the decision moved to classify_summary to be testable on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three points are kept rather than deleted: the why of each one is still what someone touching that area needs, and the "decided against" list only works next to it. The DMG one needed no code — the bundling failure was orphaned mounts from earlier attempts, and a full build now runs bundle_dmg.sh clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`spawn_detached` put the daemon in its own process group but left it in our session, so closing that terminal sent it SIGHUP and it died minutes later, looking like a crash. Pressing "n" in the panel then did nothing, because the request failed against a daemon that was no longer there. setsid() rather than both: it leaves the session and gives the child its own group, and asking for a group first makes setsid() fail with EPERM for already being a group leader. Requests now time out too. They are awaited on the panel's event loop, so an answer that never came froze the whole TUI — no redraw, no keys — which is what "it stops working out of nowhere" was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The terminal panel drew its own list and Review drew another by hand, with its own focus colour and its own idea of what a row looks like. `Sidebar` is the left rail: a titled section, rows of a label over a dimmed detail with a status dot, an optional action pinned to the bottom, and fixed header lines for the context a panel needs on screen at all times. `Pane` is its counterpart on the right: a titled frame that hands back the area inside it. Both are covered against the rendered buffer rather than by eye, because the hit-testing has to mirror what is actually painted — a click that selects a different row than the one under the pointer is worse than no click at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Connecting used to hand the whole screen to the remote pty, so the rail disappeared and there was no way back to the list until the session died. A pty speaks in absolute coordinates — "go to row 5", "clear the screen" — so its bytes could not simply be drawn in a box: cleared would have wiped the rail with it. They now go through a vt100 parser and the resulting grid is painted into the right pane, which is what lets the rail stay put. The rail keeps working while attached: dragging, folding, and clicking another row to switch to that terminal. F12 returns to the list; F5 opens an agent without leaving. Both are typable on any keyboard layout, unlike telnet's Ctrl+], which on a Spanish one already needs AltGr. vt100 underflows on a grid one row or column wide, so there is a floor with the reason written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The desktop app had its own orchestration in TypeScript while the CLI and the phone client used this engine, and the two had drifted: parallelism, lexis context, file contents in the prompt and repository snapshots each existed in one and not the other. The analyses now run at once rather than one after another, so three agents no longer take three times as long. Their text is collected per stage instead of streamed, because three agents writing into one buffer interleave into nonsense; each report is emitted whole under its own heading, in the order the agents were chosen. The prompt carries the changed files with the same per-file budget the desktop uses, and lexis is asked the same question — impact, callers, tests, blast radius — so a review reads the same wherever it was launched from. A snapshot before and after says whether the repo moved underneath it, which turns a stale report into a reported one. With several agents the last one no longer analyses: it reads the others' analyses from disk and verifies them. Handing it the reports inline meant truncating each to fit, so it graded material that stopped mid-sentence. It gets the full review prompt as well — it did not analyse, so that call is its only sight of the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cancelling aborted the task reading the socket and reported "cancelado" while the agents carried on running and billing. A client that goes away now cancels the run: the agents are spawned with kill_on_drop, so losing the race against the token is what kills the process. The daemon notices on its next write, so an agent that has been silent for a while keeps going until it says something — better than the old behaviour, which was never to stop at all. The rail's action button also ran start_run() while labelled "Parar", starting a second review instead of cancelling the first. Each report now says which agent wrote it. The marker carries the stage's own label, which already distinguishes an agent's pass from a slice of a large diff — calling both "pass N" claimed three agents had run when one had read the diff in three pieces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`review_run` streams the engine's events to the frontend the way the agent and pty commands already do, and `review_cancel` stops a run by id so Stop can reach one started by an earlier call. The frontend still orchestrates its own pipeline; this is the backend it will move onto. It was not switched over yet because the engine does not cover three things that side does: building the final document, saving a checkpoint after each stage, and salvaging what completed when a run fails halfway. Migrating without those would make the desktop worse exactly when a review crashes, which is when it matters most. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The panel orchestrated its own agents while the CLI and the phone client used `bento_review::engine`, and the two drifted feature by feature: parallelism, lexis context, per-file budgets, snapshots and a verifier that reads the other analyses each landed in one and not the other. It now calls `review_run` and rebuilds the runs the drawer renders from the engine's events, so the final document, the per-stage checkpoints and the salvage on failure keep working as they did. Stop calls `review_cancel`, which kills the agents rather than only unsubscribing from a stream that is still being paid for. The overview travels as the review context: it carries the PR number, title and body, and the engine only ever sees the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the phone It ran the engine in-process, which shared the logic but not the process: a review died with the app, and the TUI could not see one started from the desktop. All three now send `review.run` to the daemon, so a review belongs to the daemon and outlives whoever asked for it. Reading that stream back into events lived in the CLI, so this would have been a second copy of the wire format — which is how two clients stop agreeing on it. It moved to `bento_review::stream` and both parse with it. Stop drops the connection, which the daemon already reads as "cancel this run" and kills the agents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stale-job sweep selected a job and then marked it processing in a separate statement, so two session-end hooks running at once could both take the same one and pay for the summarizer twice. Claiming it is now a single conditional update witnessed by the timestamp the sweep read: whoever loses the race gets no rows back and skips it. The Review tab awaits daemon calls on its event loop, so a slow one (PRs shell out to `gh`) stops the redraw and the keys. It still blocks — that needs the requests moved off the loop — but it now paints a frame saying so first, instead of looking like it died. The header's own line count follows that flag, so clicks keep landing on the row under the pointer. The mouse goes to the remote program while it asks for it, which is what makes vim and htop usable; the rail takes it back when the program drops tracking. vt100 does not track the mode, so it is read off the stream. The full-screen Review views drew square, undimmed borders while the rest of the TUI is rounded and grey, which read as a different application. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Asking the daemon was awaited inline, so the panel stopped redrawing and stopped taking keys for as long as the call lasted — and `review.prs` shells out to `gh`, which can take a while or hang. A loading line made that visible last time, but it was still frozen. Deciding what to ask is synchronous now and the asking runs in its own task, reporting back on a channel the loop selects on alongside the review stream. Switching tab changes the tab immediately and fills the rows in when the answer lands; the keys keep working meanwhile. The indicator counts requests rather than holding a flag, so two overlapping ones do not have the first to land clear it. Both channels are awaited through one method because `select!` cannot borrow the state twice while the same loop also has to draw it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… screen Running a review switched to a full-screen view, so the rail and the file list vanished for as long as it lasted — and you were somewhere else, rather than watching the report next to the files it is about. `Drawer` is the third component: the right-hand column, holding the conversation about what the pane shows. It folds like the rail, with the chevron mirrored, remembers the width it had, and only takes room when there is a report in it. Its edge drags like the rail's, towards the middle rather than away from it. The full-screen output view is gone with it, and the keys it owned — asking a follow-up, paging through the report — moved to the panel, where the report now is. Its "d" would have clashed with deleting a checkpoint, so folding is "w". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Its visibility was derived from "is there any output", so a run that died before writing anything — an agent that is not installed, a bad base — ended with the drawer gone and no sign it had ever run. The one moment the panel had something to say, it said nothing. Visibility is an explicit flag now: once open it stays until folded. The reason was lost too. The daemon reports failures as progress, and progress is only shown while running, so it vanished with the drawer. A failure goes into the report instead, where it is still readable after the run stops; tool lines stay as progress, because they are noise once the report exists. Also removed the TypeScript pipeline the engine replaced: the prompt builders, the context provider that gathered lexis and file contents, and the Tauri commands behind them. Nothing imported them any more, and dead copies of logic that moved are how it grows back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…agents The drawer had no focus of its own, so the arrow keys never reached it: right stopped at the file list and up/down always moved a selection somewhere else. It is a third focus now, where the arrows scroll — there is nothing to select in a report — and right skips it when it is closed, so the keys never land somewhere invisible. Its hint also said "d plegar" while the key is "w", which is the third stale shortcut label this panel has had. And the bug the review itself found: with exactly two agents the second is the verifier, leaving one analysis, but the verification only ran with two or more reports. The agent was picked, shown in the rail, and never run. It is gated on having a verifier and something to read now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A checkpoint was filed under cwd+base alone, so a second review of the same branch overwrote the first — its report, and the verifier session you could still ask questions of. Which is the one worth keeping: the last agent read everyone's analysis and wrote the final report. Each run now has its own id, stable across the saves one review makes as it goes and different between runs, so both survive. A checkpoint without one keeps its old location, so anything already saved stays findable. The drawer lists them: "l" swaps it between the report and the reviews saved for this project, with the date, the branch and whether it can still be resumed. Enter opens one — report and session together, so the conversation picks up where it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit added the field to the shared checkpoint and to the CLI, but not to the Tauri command or its caller — so the desktop stopped compiling, and I committed it without building that side. Every review the desktop runs is now its own entry as well, rather than overwriting the branch's last one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The run id reached the shared checkpoint, the CLI and the desktop, but not the phone: it went on saving under cwd+base alone, so a second review of the same branch still overwrote the first there. Its history now opens the run you picked rather than the branch's newest, which is what several-per-branch needs to be worth anything, and the HTTP get takes a run id for it. Without one it still answers with the latest, so a client that has not been updated keeps working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.