Repository navigation
Conversation
A session file with no session_end record was indistinguishable from a session whose run was still in progress, because a running review has not written session_end yet. A writing process now holds an exclusive OS lock on a <session-id>.lock sidecar for the life of the run, so the kernel - not a heuristic over timestamps - decides whether a session is alive. The lock is released however the process ends, so a crashed or killed run needs no cleanup to read as finished. Close joins Finalize for a run that is abandoned rather than completed: it releases the file handles and the lock without claiming completion.
The viewer reported every session without a session_end as "aborted", which described every review still in progress as well as every one that had died. Status now consults the liveness lock first, so an in-flight run reads as running and a dead one still reads as aborted. Duration is computed in the browser from the start timestamp, because a run that has not written session_end has no final duration to report. The page reloads itself once the run ends, so the terminal figures always come from the server rather than from a guess made client-side. The repository list shows a running badge linking straight to the live session, and both pages poll while a run is in flight.
Two affordances for working through a review from the session page. Collapse all folds every per-file finding group to a one-line summary, so a page with hundreds of findings becomes a scannable index of files and their counts. Groups hidden by the current filters are left alone, keeping what looks open in step with what is visible. Fix with agent carries one finding - file, line range, severity, category, description and both versions of the code - into Claude Code, Codex or Cursor, prefilled. Targets are ordered app then editor extension, using each vendor's documented scheme. No vendor documents a prefilled-prompt scheme for its CLI, so the CLI is reached by copying the same text instead of a link that would silently do nothing. Every one of these prefills a composer without submitting, so the user always confirms before an agent runs.
The menu item handler cancelled the click, assigned the deep link to the anchor's href, and re-dispatched the click on a later turn of the event loop. That deferred click runs outside the transient user activation every current browser requires before handing an external protocol to the desktop, so the navigation was dropped and the click did nothing at all. Verified in headless Chromium: zero navigations requested before this change, one after. The href is now built while the menu opens, which is itself a real click, and the item's own click is left to the browser. Placeholder hrefs become about:blank rather than "#", so a link followed before the script rewrites it cannot scroll the page.
Documents the running status and ticking duration, what the .lock sidecar is for, the collapse-all control, and the agent handoff including that no vendor prefills a CLI prompt and that every target waits for the user to confirm before running. All five locales updated.
…ndoff The repositories page could not detect a review *starting*. watchRepoActivity returned early when no repo was active, and then compared against a marker element that was truthy on every path reaching it, so it only ever noticed runs ending. Opening the viewer and then launching a review - the ordinary workflow, and the one this feature exists for - showed no badge at all. It now captures the initial state and compares against the server each poll, in either direction. The same function checked document.hidden at setup rather than per tick, so a page opened in a background tab never started polling at all. agent.js registered one document-level click listener per finding, so a session with hundreds of findings ran hundreds of listeners on every click anywhere in the viewer. The dismiss handler is now installed once and only touches menus that are open: 2 listeners for 40 findings, down from 41. Prompt truncation used a plain slice, which could cut inside a fenced code block and leave it unclosed, or split a surrogate pair and emit invalid UTF-16. It now backs off to a line boundary, never splits a pair, and closes an unbalanced fence. Also drops an unread field on the lock handle and replaces two `var`s with `const` in code that uses const throughout.
SaahiilM
force-pushed
the
feat/viewer-live-session-and-agent-handoff
branch
from
October 2, 2026 22:49
088fedf to
e846655
Compare
fix(llm): allow unlisted OpenRouter model overrides (alibaba#1610)
Pulls in main's commits (ACP service, ChatGPT auth, scan token-budget and prompt-overflow fixes, provider docs, dependency bumps, etc.) into the live-session and agent-handoff viewer branch. The only conflict was go.mod (version bumps for golang.org/x/sys, x/net, x/sync, x/text), resolved to main's versions; the branch builds cleanly afterwards.
The list test simulated an abandoned run by closing the session file directly, which left the liveness sidecar locked. An open handle cannot be deleted on Windows, so the test's temp dir cleanup failed there. Closing the session the way an abandoned run does releases the lock too.
os.UserHomeDir() prefers USERPROFILE on Windows, so setting HOME alone left the sessions these tests create in the real profile instead of the temp dir they inspect. The session package already redirects both; the viewer tests now do the same.
Owner
Author
|
Superseded by #5. This PR bundled two independent things: the live-session-status work (showing a running review as Upstream already has its own implementation of the liveness half in alibaba#1757, which uses the same sidecar path and defines the same #5 is based on upstream Closing this one. |
Owner
Author
|
Closed in favour of #5, which contains the agent handoff and collapse-all control on top of upstream main. The liveness half is covered by upstream's own PR, so keeping this one open would have produced a conflict. |
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
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.
Description
The viewer reported every review still in progress as aborted.
Aborteddefaulted totrueand was cleared only by asession_endrecord — but a running review has not writtensession_endyet, so the field really meant "nosession_end", conflating in-progress with dead.This PR separates them using an exclusive OS lock the writing process holds on a
<session-id>.locksidecar for the life of the run, so the kernel decides liveness. It also adds a collapse-all control for finding groups, and a per-finding handoff into Claude Code / Codex / Cursor.Verified in a real browser, not just tests: three session fixtures rendered through the actual handlers showed
runningfor the locked session,abortedfor the crashed one,legacyfor the finished one.What each commit does
feat(session)Closefor an abandoned runfeat(viewer)runningahead ofaborted; tick duration client-side; reload on completionfeat(viewer)fix(viewer)docsviewer.mdin all five localesEach builds and passes tests standalone — verified by checking out every commit individually.
Two things worth reviewing closely
The liveness mechanism. A sidecar file plus an OS lock, rather than an mtime heuristic. The kernel releases the lock however the process dies, so a crash needs no cleanup step. The tradeoff: the writer changes, and Windows needs
LockFileExrather thanflock. Worth a look at whether you would rather have the heuristic and keep the writer untouched.The deep-link fix is a separate commit on purpose. The first implementation did not work on any platform: the click handler cancelled the click, assigned the href, then re-dispatched the click via
setTimeout. That deferred click runs outside the transient user activation browsers require before handing an external protocol to the desktop, so it was silently dropped. Headless Chromium showed zero navigations requested before the fix, one after. The commits are split so the regression and its fix are each reviewable on their own.Known limitation
cursor://is documented by Cursor but no handler is registered on this machine, so that entry does nothing here until Cursor is installed.claude-cli://,codex://andvscode://are all registered and verified working.No vendor documents a prefilled-prompt scheme for its CLI, so the CLI is reached by copying the prompt rather than by a link that would silently do nothing. Every target prefills without submitting — the user always confirms before an agent runs.
Type of Change
How Has This Been Tested?
make testpasses locallymake check,make test(0 failures) andmake coverage(91.1%, threshold 90%) all pass. Cross-compiles clean for linux, darwin and windows.Manual testing:
running/aborted/legacyrespectively.xdg-mime query default.New tests cover the lock (held, released, missing sidecar, idempotent close), the status rendering, the agent handoff context, and a regression test that fails if the re-dispatched click is ever reintroduced.
Checklist
go fmt,go vet)AI disclosure
Tool: Claude Code (this session), model
claude-opus-5(anthropic/claude-opus-5[1m]).Also used: two read-only research subagents to verify vendor deep-link schemes against primary documentation, and one to check whether VS Code's
--open-urlis a supported flag (it is not — it is an internal marker, so it is unused here).Every commit was reviewed and rewritten by hand before being pushed. Two mistakes I introduced and corrected:
Co-Authored-Bytrailer. AGENTS.md rule 6 forbids AI attribution, so it was removed;git logshows no trailers on any commit.fixcommit rather than squashed, so the regression is visible rather than hidden.I understand every line above and can answer questions on the reasoning without consulting the tool.
Prompts used
For an agent reviewing this PR, these are the prompts that shaped it — they state the goal and the changes, so the intent is checkable against the diff.
Goal (the bug as reported):
This is the verbatim request. It named three symptoms — a live session shown as aborted, status/duration disagreeing with the CLI, and duration computed from the start timestamp in JS. It also contained a misremembering ("version command"); I asked and it meant the viewer, not
ocr version.Follow-up (agent menu and ordering):
This set the app → CLI → extension ordering. Verifying what each vendor actually supports changed the outcome: no CLI scheme exists for any of them, so the CLI became the copy action instead of a menu entry.
Report of the failure (leading to commit 4):
This is what exposed the user-gesture bug. I then verified it rather than assuming, by driving headless Chromium before and after the change and counting navigations.
Two design decisions I raised rather than assumed, both answered before implementing: how to distinguish a live run from a crashed one (lock file over mtime heuristic) and where to put the work (a worktree, to keep unrelated in-progress changes separate).
Related Issues
🤖 Generated with Claude Code