Repository navigation
Conversation
|
Review complete. 🟠 1 high · 🟡 2 medium 💬 Inline comments (3)
Reviewed commit: 61c72d4 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c72d4770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c72d4770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
|
Review complete. 🟡 3 medium 💬 Inline comments (2)
📍 Outside the diff (1)
Reviewed commit: f3389ed |
61c72d4 to
f3389ed
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3389ed0ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f3389ed to
29baf66
Compare
|
Review complete. 🟡 2 medium 💬 Inline comments (2)
Reviewed commit: 29baf66 |
There was a problem hiding this comment.
Expand to see previous review
Adds an agent-resume chooser with session reservations, candidate resolution, and PTY recovery install across renderer terminal panes; two medium bugs found around latch spending on visibility loss and keyboard/focus selection divergence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29baf66c7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
29baf66 to
603e3f0
Compare
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
Reviewed commit: 603e3f0 |
There was a problem hiding this comment.
Expand to see previous review
Adds an agent auto-resume recovery flow for orphaned agent sessions, with a modal chooser, candidate scanning/resolution, and reservation lifecycle; two minor issues: chooser strings missing from non-English locales and a startup-build failure misreported as session-claimed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 603e3f085b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
603e3f0 to
f63e4e6
Compare
|
Review complete. 🟠 2 high · 🟡 1 medium 💬 Inline comments (3)
🧹 Nitpicks (1) — 🟢 1 low
Reviewed commit: f63e4e6 |
f63e4e6 to
603e3f0
Compare
5427f80 to
2efa685
Compare
603e3f0 to
f63e4e6
Compare
| session.agentResumeRecoveryUnregister = registerAgentResumePaneHandler( | ||
| session.cacheKey, | ||
| session.transport, | ||
| resumeWithCandidate | ||
| ) |
There was a problem hiding this comment.
🟠 bug · high
Resume handler registered before session.transport exists
installAgentResumeRecovery registers the pane handler with session.transport as owner, but in connectPanePty it runs before installPtyInputRecovery assigns session.transport, so the handler is registered with owner undefined (src/renderer/src/components/terminal-pane/pty-connection/agent-resume-recovery-install.ts:97). Choices are later published with the real transport as owner, so getAgentResumePaneHandler always mismatches and returns undefined. Every chooser interaction — Enter, number key, click — silently does nothing; only the automatic sole-candidate resume path works.
📋 Prompt for AI Agents
In src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.ts, installAgentResumeRecovery(session) (line 172) runs before installPtyInputRecovery(session) (line 180) assigns session.transport, so agent-resume-recovery-install.ts lines 95-99 register the resume pane handler with owner undefined while pending choices are published with the real transport as owner — getAgentResumePaneHandler in AgentResumeChooserPortals.tsx line 31 always returns undefined and every chooser selection is a silent no-op. Move the registerAgentResumePaneHandler call to after the transport assignment (e.g. invoke it from the end of installPtyInputRecovery, or change installAgentResumeRecovery to accept an owner getter), keeping the unregister wired to dispose.
| const failures = (result.issues ?? []).filter((issue) => issue.kind !== 'notice') | ||
| if (failures.length > 0) { | ||
| return 'scan-issues' | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Notice rows hide dropped failure issues from completeness check
scanIncompleteReason treats a notice-only issue list as a complete scan (src/renderer/src/lib/agent-resume-candidate-source.ts:125), but a notice row is emitted precisely when the issue array overflowed SCAN_ISSUE_LIMIT and further failure rows were dropped (src/main/ai-vault/session-scan-issues.ts:20-25). A stalled WSL distro or unreachable remote root records one refusal per discovered path, so a 500+ refusal scan ships only the notice; the renderer filters zero failures, answers complete, and the surviving subset can be auto-resumed as a sole candidate — forking the wrong transcript.
📋 Prompt for AI Agents
In src/renderer/src/lib/agent-resume-candidate-source.ts around lines 125-128, scanIncompleteReason filters issues to issue.kind !== 'notice', but a kind: 'notice' row is also how the main scanner signals that additional failure rows were dropped at SCAN_ISSUE_LIMIT (src/main/ai-vault/session-scan-issues.ts). Change the function so the 'Additional scan issues were omitted.' notice makes the scan unverifiable with reason 'scan-issues', while genuine per-transcript commentary notices still count as complete — distinguish them by path/message. Update agent-resume-candidate-source.test.ts accordingly.
| // while a newer one survives, and the resolver reads that subset as the whole. An answer sitting | ||
| // exactly at the cap is indistinguishable from a truncated one, so it is `unverifiable`: a | ||
| // subset must never be read as the whole (docs/reference/ssh-execution-boundary.md). | ||
| return (result.sessions ?? []).length >= DEFAULT_AI_VAULT_SCAN_LIMIT ? 'scan-truncated' : null |
There was a problem hiding this comment.
🟡 bug · medium
Truncation heuristic misjudges both capped and complete scans
scanIncompleteReason flags any answer with sessions.length >= DEFAULT_AI_VAULT_SCAN_LIMIT as scan-truncated (src/renderer/src/lib/agent-resume-candidate-source.ts:135), but the request is unlimited: true and an honouring host returns its uncapped session list — so a host holding 1000+ sessions total (the PR's own motivating example) answers completely, gets labelled truncated, and every recovery attempt returns unverifiable, permanently disabling auto-resume on exactly the machines that accumulate the most transcripts. Conversely, the heuristic cannot detect an older host that caps below 1000, so a genuinely truncated subset can pass as complete.
📋 Prompt for AI Agents
In src/renderer/src/lib/agent-resume-candidate-source.ts line 135, the sessions.length >= DEFAULT_AI_VAULT_SCAN_LIMIT heuristic misclassifies a complete uncapped answer as truncated (disabling auto-resume on hosts with 1000+ total sessions) and cannot detect a host that caps below the constant. Replace it with an explicit host signal: extend the listSessions result consumed here (respecting remote-wire-compatibility for the optional field) so the host reports whether it applied a cap, return 'scan-truncated' only when a cap was actually applied, and keep the length heuristic only as a fallback for hosts older than the new field.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63e4e6cb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (resolveAgentResumePaneLiveness(state, rowPaneKey) !== 'exited') { | ||
| claimed.add(entry.providerSession.id) |
There was a problem hiding this comment.
When two execution hosts or providers expose the same raw session ID, this global claim set treats them as one conversation even though only one belongs to the recovering pane's host. The repository's agentProviderSessionsEqual also establishes that Pi and Prime Agent identities include transcriptPath, but claims and reservations retain only .id; a live session on host A, or a distinct Pi transcript with the same ID, can therefore suppress a valid recovery on host B indefinitely. Key claims by execution host, agent, and complete provider metadata rather than the raw ID.
AGENTS.md reference: AGENTS.md:L89-L91
Useful? React with 👍 / 👎.
| {panes.map((pane) => ( | ||
| <AgentResumeChooserPortal key={pane.id} paneKey={makePaneKey(tabId, pane.leafId)} /> |
There was a problem hiding this comment.
When two visible split panes both produce ambiguous recovery results, this maps every pane to an open app-modal Dialog, creating stacked focus traps and unlabeled decisions whose selected session applies to whichever hidden portal owns the top layer. Fresh evidence in this revision is that TerminalPaneSurface gates only on the tab-level isActive, while this loop still mounts a chooser for every split pane rather than the active pane. Filter to the active pane or serialize the pending choices.
Useful? React with 👍 / 👎.
Refuses rather than guesses: filters by pane agent, exact cwd scope, a substance floor and live-pane claims, then resolves by sole survivor, tab session id, or a unique host-clock lifetime-window match; otherwise asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The source asks one execution host which sessions it holds for a workspace and validates every field the resolver will act on at that boundary; an unreachable host answers with no candidates rather than throwing, so the pane falls back to a plain shell and never a wrong resume. The chooser is the one visible surface of auto-resume. It renders only when the resolver refused to decide, and it is wired to replace the spawn rather than sit over a live shell, so no resume command is ever typed into a running terminal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… session Runs after the plain shell has spawned rather than holding every fresh spawn behind a host round trip, so a terminal in a workspace that never ran an agent pays nothing. A recovered session replaces the shell by respawning, which keeps a resume riding a spawn instead of being typed into a live terminal. Three gates refuse: the pane already owns a record (a second resume would fork one transcript across two panes), the pane is hidden (a host restart must not respawn a workspace in bulk), or the user has already typed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kept out of the persisted session state on purpose: a pending choice is transient UI state, and persisting it would let a stale list be revived against sessions the host no longer has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ties the host query, the resolver and the pane gates together for a pane whose resume handle was lost. The gates are read live rather than captured: asking the host means a transcript scan measured at ~2s on a host holding 1200 of them, which is long enough for the user to start typing, and replacing a shell someone is using is the one outcome this path must never produce. The chooser reaches its pane through a handler registry rather than React props, matching how the pty layer already keys per-pane state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registers each pane's resume handler on connect, clears it on dispose, and attempts recovery when the pane first becomes visible rather than on mount, so a host restart never respawns a workspace behind the user's back. A recovered session has no captured launchConfig — the handle that held it is what was lost — so the pane's configured defaults for that agent apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Recovery only ran on a hidden -> visible flip, so the main case (a tab already active when Orca opens) never recovered; it now also runs when a visible pane's fresh shell spawns, behind the same attempt-once latch. A pane must now show positive evidence it hosted an agent before anything is offered, two panes can no longer resolve the same conversation because the final claim is a synchronous reservation taken after the claim set is re-read, candidate metadata is built per agent and unusable candidates are dropped, a failed attempt no longer disables recovery for the connection, offers and disposal are guarded against a successor, and the chooser ages rows against the newest candidate's host clock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round two of review feedback on the pane recovery path. The recovery guard read `resolveExpectedLaunchTuiAgent`, which accepts the tab-wide `launchAgent` — a value that describes the tab's ORIGINAL pty only, so a plain-shell split in an agent-launched tab passed it and would have had its shell replaced by an unrelated conversation. Recovery now requires evidence read under this pane's own key: its startup, its registered launch config, its status row, or its own foreground inference. `aiVault.listSessions` can resolve `cancelled` or carry scan issues instead of rejecting. Those answers said nothing about the host, yet a subset could be read as the sole candidate and resumed, and an empty one spent the pane's single attempt. The scan now reports completeness, an incomplete scan refuses to resolve, and it leaves the attempt unspent. Dispose released the pane's resume reservation without checking this binding still owns the pane, so a retired binding could free a successor's reservation and reopen the window where two panes resume one transcript. It now sits behind the same successor guard as the pending-choice clear. The tab-session-match and lifetime-window rungs were unreachable: the caller never passed their evidence, and it cannot. A tab carries no last provider session id — that handle is exactly what was lost — and `tab.createdAt` is a renderer-clock value the resolver may never compare to a host-clock `updatedAt`. Both rungs are removed rather than left as code the tests pretended covered something; the chooser asks the user instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1. Untruncated scan. `listSessions` sliced a recency-sorted list at 1,000, and the in-scope bypass that survives that slice covers Claude files only — so for the other 17 resumable agents an older matching session vanished while a newer one survived, and the resolver read that subset as the whole and auto-resumed the survivor. The scan now asks for an uncapped answer, and an answer that still arrives at the default cap is `unverifiable`: a host too old to honour the flag is indistinguishable from one with exactly that many sessions, and a subset must never be read as the whole. 2. Revalidate the claim at selection. Selecting a chooser row checked only the renderer-local reservation map, but another pane's cold restore and an Agent Session History launch reserve nothing there. The host-owned status store is now re-read in the same synchronous step as the claim, immediately before the spawn. 3. Binding identity on a published choice. Pending choices carried none, so a chooser published for a retired connection stayed on screen and reached the SUCCESSOR's handler under the same stable pane key — replacing a live shell with a session scanned for a connection that no longer owns the pane. A choice now names the binding that scanned it, only that binding's handler can be reached through it, and dispose retires its own choice and leaves a successor's alone. 4. Mid-scan visibility loss no longer spends the latch. The contract is that a hidden pane keeps its attempt and retries when revealed; the post-scan gate answers `pane-hidden` for a pane the user switched away from inside the ~2s scan, and that answer was burning the attempt. 5/6. The chooser is now the repo's `Dialog` primitive, and its rows are no longer native buttons. As an inline overlay it trapped no focus — Tab reached the xterm underneath, so the user could type into the very shell a row was about to replace. The same Tab moved real focus between row buttons without moving `selected`, and Enter then resumed `rows[selected]`: a different session than the focused one, silently forking the wrong transcript. One focusable listbox naming its active row through `aria-activedescendant` makes focus and selection one value, so fix 6 subsumes fix 5 rather than both being fixed twice. The Dialog is app-modal, so it renders for the active tab only. 7. Selected rows use the documented jump-palette selection surface and inset ring; flat `bg-accent` is near-invisible on a light dialog, and the cursor is what decides which transcript Enter resumes. 8. Folder workspaces. Recovery gated on `session.worktree?.path`, so agent panes in a folder workspace — whose root is `folderWorkspace.folderPath` — could never recover. The scope is derived from either representation. 9. Equivalent path spellings. Scope matching compared raw `cwd` strings, which false-negatives on `C:\repo` vs `C:/repo`, drive-letter casing, a trailing separator, and macOS NFD against an agent's NFC cwd. Both sides now go through `normalizeRuntimePathForComparison`, the normalizer the vault's own scope machinery uses. Still equality, not containment: a sibling worktree and the repo root keep missing, which is what scopes the resume at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oser Three round-four review findings on the auto-resume path. THE PANE-IDENTITY CLAIM RULE replaces `entry.state !== 'done'`, which was wrong in both directions. Too broad: after a host restart the hook server republishes the recovering pane's OWN persisted row as `restoredUnconfirmed`, provider session included, so the pane treated its own conversation as claimed, the sole candidate was filtered out, and a restart during working/blocked/waiting left a plain shell — the feature's primary scenario. Too narrow: `done` means "task complete but pane live", so excluding those rows let a live idle sibling's transcript be resumed into a second pane. Ownership now comes from pane identity and liveness from the pane's own terminal binding, in the live/unverifiable/ exited vocabulary: the pane's own row never claims against it whatever its state, another pane's row claims whenever that pane is live or unverifiable whatever its state, and only a pane this renderer no longer holds releases its session. A WSL workspace is stored as its UNC spelling while the transcripts inside the distro report a Linux cwd, so the single normalized scope key matched nothing and recovery never ran on WSL at all. The resolver now admits both spellings of the one directory, the same representation pair session-scanner-scope-discovery.ts already expands for the scan. Still equality, not containment: a sibling worktree and the repo root keep missing. The four chooser strings existed only in en.json, so a surface that decides which conversation resumes rendered in English for every other locale. Added under the identical path in es, fr, ja, ko and zh, matching each catalog's existing tone and its convention of keeping Enter/Esc as literal key names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects that all end the same way — a pane that silently does the wrong thing, or nothing. **The chooser has never worked.** `installAgentResumeRecovery` ran before `installPtyInputRecovery`, and the latter is what assigns `session.transport`. So the pane handler was registered under owner `undefined` while choices were published under the real transport, and every lookup mismatched: Enter, number key and click all did nothing. Only the automatic sole-candidate path worked. No unit test could see it, because every test handed the installer a session bag that already had a transport. The new test binds through the real `connectPanePty` sequence, so a future reorder fails the suite. The registry now also admits an absent owner in its type and refuses it loudly — the session bag is an index-signature type, so nothing else would have caught it — and an absent owner no longer matches an absent registration, which was a crash and a hole in the identity guard. **A `notice` row meant the opposite of what was assumed.** Round three read a notice-only issue list as a complete scan. But a list AT `SCAN_ISSUE_LIMIT` is one `recordSessionScanIssue` stopped appending to: the rows that did not fit were dropped. A stalled WSL distro records one refusal per discovered path, so filtering notices out of that list answers about the handful that survived, not about the scan. The limit moves to shared and the reader treats a list at the cap as unverifiable. A short notice-only list is still what it looks like — per-transcript commentary — and stays complete. **The truncation heuristic was wrong in both directions.** It called any answer with 1000+ sessions `scan-truncated`, but the request now sends `unlimited: true`, so a host that honours it and holds many transcripts was permanently refused — exactly the machines this feature exists for. It also could not see a host capping BELOW 1000 at all. There was no completeness signal in the API, so this adds one: `AiVaultListResult.appliedSessionDepth`, the bound the answering scan actually applied. Optional, because hosts and clients update independently, and absent means "coverage cannot be proved" — never "complete". That fails safe: refusing costs a bare shell, which is the pre-feature behaviour, while reading a slice as the whole lets a candidate be auto-resumed that is only sole because the rest were cut. It is stamped where a bound is applied (local scan, truncate, merge), folded across legs so one unprovable leg makes the merge unprovable, and carried through `restampAiVaultListResult` and the relay parser — both rebuild the result field by field and would otherwise have dropped it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… manifest (OTA phase C follow-up) (stablyai#22376) * perf(mobile): keep four bundle chunk reads in flight across the whole manifest The fetch ran one worker per asset and paged inside an asset sequentially, so the largest script's 71 chunks were 71 serial round trips while the other readers idled. One window of four chunk reads now covers every (asset, offset) on the host's chunk grid, largest asset first. A read_limited refusal narrows the window and retries the read; eof is still read from the reply. Synthetic manifest (one 71-chunk asset, five small): 72 round trips -> 19. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the bundle-fetch family under pipelined reads Baseline moves to a1ee317, the pipelined fetch. 781 goldens change only their `baseline` header line. Six bodies move: mobile-web-bundle-fetch-paged, mobile-web-bundle-build-changed, and the four matrix-mobileweb.bundle-fetch-* goldens. The two bundle-fetch scenarios now bind requests in pipelined order, largest asset first, with every chunk sent before any reply: index.html@0 (#1), index.html@16 (#2), assets/app.js@0 (#3). - fetch-paged: the request set is identical, only reordered. The chunk sender names/ordinals and the scenarioSha256 moved; the replies and the fetched bytes did not. - build-changed: the same reorder, plus one request that is new because pipelining puts it in flight before the refusal lands (index.html@16). The refusal and the checkpoint are unchanged. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): the bundle chunk comment no longer says a reply picks the next offset Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the bundle comment fix Baseline moves from a1ee317 to e94bde3, the comment-only commit on a fenced path. All 787 goldens and the scenarios file change only their `baseline` line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): pool four workers over planned bundle chunks, report progress per chunk Design-review fix round, sketch C: four workers take reads from one planned chunk queue, largest asset first. They replace the central pump and the read_limited narrowing. A host frees its read slot before it replies, so a lone fetch capped at four cannot trip the limit. A refusal now fails the fetch, as it did on base, and stops the other reads. - Each asset's buffer is allocated when the plan is built. That removes the nullable buffer and its guard. The per-asset byte count is gone, and the hash is the oracle (S1, S2). - The caller's signal is checked before each read and once after the pool drains, so an abort during the final window rejects with fetch-stopped (S3). The stopped check now covers only the caller's abort. The internal stop only makes late replies skip checks, hashing and progress (S4). - Progress is reported per accepted chunk. completedAssets still counts on completion (S6). - Renames: MAX_CONCURRENT_CHUNK_READS, and `reply` for the RPC reply (N1). - The slot check states exact-slot acceptance once, then classifies the refusal (N3). - Stale test titles and comments are renamed (N4). The synthetic manifest still takes 19 round trips. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): say why bundle reads settle at on-settle under pipelined chunks Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the bundle-fetch family under per-chunk progress Baseline moves to 34fda6f. 782 goldens and the scenarios file change only their `baseline` line. Five bodies move: mobile-web-bundle-fetch-paged and the four matrix-mobileweb.bundle-fetch-* goldens. The only change is bundle-progress effects. One report now lands after the first accepted chunk of index.html (0 assets, 16 bytes), and the later progress ordinals shift by one. Requests, replies and fetched bytes are identical. mobile-web-bundle-build-changed keeps its body, because its one accepted chunk is the whole of assets/app.js. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): drain the fake host after the fetch settles so the sibling-stop bounds can fail The wave host stopped releasing replies once the fetch settled. Reads that should have been stopped were never answered, so the read_limited bound (7) and the chunk-failure bound (5) held even with no sibling stop at all. It now drains until nothing waits. With the worker's stopped.abort() removed, both bounds fail at 76 requests. assertChunkDescribesAsset's parameter is renamed to `reply`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the sibling-stop test fix Baseline moves from 34fda6f to 97b13ec. All 787 goldens and the scenarios file change only their `baseline` line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
…otiated mobileWeb.bundle.range (OTA phase C follow-up) (stablyai#22381) * feat(mobile-web): serve gzipped 384 KiB bundle ranges behind a capability Adds mobileWeb.bundle.range with its own strict params and result, so shipped chunk readers see no reply change. The host gzips each range at level 6 and sends identity when gzip does not shrink it, sharing the chunk method's read-slot budget and per-asset verification. status.get advertises mobileWeb.bundle.range.v1 beside mobileWeb.bundle.v1, and the method is allowlisted for paired phones. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): read the bundle range capability and range replies Adds the range reply reader and operation, and picks range or chunk from the status.get capabilities the connection already proved, so an older desktop keeps being paged in chunks with no probe round trip. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * perf(mobile): keep four bundle chunk reads in flight across the whole manifest The fetch ran one worker per asset and paged inside an asset sequentially, so the largest script's 71 chunks were 71 serial round trips while the other readers idled. One window of four chunk reads now covers every (asset, offset) on the host's chunk grid, largest asset first. A read_limited refusal narrows the window and retries the read; eof is still read from the reply. Synthetic manifest (one 71-chunk asset, five small): 72 round trips -> 19. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * chore(mobile): add fflate 0.8.2 for gzip bundle ranges Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): decode gzip bundle ranges into a bounded buffer Inflates each range into a buffer one byte past its window, so a gzip bomb costs at most that allocation and an overlong body is visible. A corrupt, truncated or unknown-encoding body refuses as range-undecodable; a body of the wrong decoded length refuses as range-length-mismatch. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): pass the bundle read method from the session to the fetch The download reads the capabilities of the gates the reducer decided under and hands the fetch range or chunk. The fetch does not act on it yet; the range read lands on the pipelined window. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the bundle-fetch family under pipelined reads Baseline moves to a1ee317, the pipelined fetch. 781 goldens change only their `baseline` header line. Six bodies move: mobile-web-bundle-fetch-paged, mobile-web-bundle-build-changed, and the four matrix-mobileweb.bundle-fetch-* goldens. The two bundle-fetch scenarios now bind requests in pipelined order, largest asset first, with every chunk sent before any reply: index.html@0 (#1), index.html@16 (#2), assets/app.js@0 (#3). - fetch-paged: the request set is identical, only reordered. The chunk sender names/ordinals and the scenarioSha256 moved; the replies and the fetched bytes did not. - build-changed: the same reorder, plus one request that is new because pipelining puts it in flight before the refusal lands (index.html@16). The refusal and the checkpoint are unchanged. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): the bundle chunk comment no longer says a reply picks the next offset Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): read gzipped bundle ranges on the pipelined window A host that advertised mobileWeb.bundle.range.v1 is paged in 384 KiB ranges on the range grid, through the same four-read window and queue as chunks; any other host keeps the chunk grid. Each range is decoded to its exact window length before the fill checks and the asset hash. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the bundle comment fix Baseline moves from a1ee317 to e94bde3, the comment-only commit on a fenced path. All 787 goldens and the scenarios file change only their `baseline` line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the corpus at the range-read pin Repins baseline to the range-read commit and re-records every golden. Only the baseline and lockfileSha256 headers move: the lockfile gained fflate, and the bundle-fetch adapter pages the chunk path, whose requests and replies are unchanged, so no golden body moves. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): state the on-settle reason that holds for pipelined bundle reads Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the on-settle comment fix Baseline moves from 252c592 to fe41226, the comment-only commit on a fenced path. Re-recorded: all 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * chore(mobile): keep the lockfile's patch block in main's form Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the lockfile patch-block restore Baseline moves from fe41226 to 84d6fca, which restores main's patchedDependencies form in mobile/pnpm-lock.yaml. Re-recorded: all 787 goldens change only baseline and lockfileSha256, and the scenarios file only baseline. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): pool four workers over planned bundle chunks, report progress per chunk Design-review fix round, sketch C: four workers take reads from one planned chunk queue, largest asset first. They replace the central pump and the read_limited narrowing. A host frees its read slot before it replies, so a lone fetch capped at four cannot trip the limit. A refusal now fails the fetch, as it did on base, and stops the other reads. - Each asset's buffer is allocated when the plan is built. That removes the nullable buffer and its guard. The per-asset byte count is gone, and the hash is the oracle (S1, S2). - The caller's signal is checked before each read and once after the pool drains, so an abort during the final window rejects with fetch-stopped (S3). The stopped check now covers only the caller's abort. The internal stop only makes late replies skip checks, hashing and progress (S4). - Progress is reported per accepted chunk. completedAssets still counts on completion (S6). - Renames: MAX_CONCURRENT_CHUNK_READS, and `reply` for the RPC reply (N1). - The slot check states exact-slot acceptance once, then classifies the refusal (N3). - Stale test titles and comments are renamed (N4). The synthetic manifest still takes 19 round trips. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): say why bundle reads settle at on-settle under pipelined chunks Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile-web): announce bundle ranges on the manifest reply The manifest reply now names the range grid in an optional rangeBytes, beside chunkBytes, and the status capability is gone. The range method takes exactly the chunk params on that grid instead of a caller length. Both methods share one verified read that returns the six-field header, and the range handler checks the connection again before deflating. Range schemas move into the bundle RPC contract; SHA256_PATTERN is shared from the manifest contract. Shared refusals are tested once over both methods. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): drop the range capability read and its session threading The phone will read rangeBytes off the loose manifest reply instead, so the read-method module goes and the session effects and hook return to the pipeline branch's version. Range imports move to the bundle RPC contract, the reply reader reuses the shared SHA256_PATTERN, and a new test pins that node's level-6 gzip from the host encoder inflates with fflate to the same bytes. The fetch and window-read modules still import the deleted names until part 2. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the bundle-fetch family under per-chunk progress Baseline moves to 34fda6f. 782 goldens and the scenarios file change only their `baseline` line. Five bodies move: mobile-web-bundle-fetch-paged and the four matrix-mobileweb.bundle-fetch-* goldens. The only change is bundle-progress effects. One report now lands after the first accepted chunk of index.html (0 assets, 16 bytes), and the later progress ordinals shift by one. Requests, replies and fetched bytes are identical. mobile-web-bundle-build-changed keeps its body, because its one accepted chunk is the whole of assets/app.js. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): page bundle ranges through one window reader chosen by the manifest The fetch keeps the pipeline's four-worker pool and builds one window reader from the manifest reply: ranges on rangeBytes when the host names it, chunks on chunkBytes otherwise. The reader returns the six-field header and a lazy bytes() so the stop and misroute checks run before any decode. A range that inflates to the wrong length now falls to the slot checks, with the one-byte-over buffer as the memory bound, so range-length-mismatch is gone. A rangeBytes this build cannot page reads as absent. Fetch names say window, not chunk. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): drain the fake host after the fetch settles so the sibling-stop bounds can fail The wave host stopped releasing replies once the fetch settled. Reads that should have been stopped were never answered, so the read_limited bound (7) and the chunk-failure bound (5) held even with no sibling stop at all. It now drains until nothing waits. With the worker's stopped.abort() removed, both bounds fail at 76 requests. assertChunkDescribesAsset's parameter is renamed to `reply`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the window-reader commit Baseline moves to a25a355. Re-recorded: every golden and the scenarios file move only baseline, and the five bundle-fetch goldens also move lockfileSha256 to this branch's lockfile. Every golden body is byte-identical to the pipeline branch's. The recording adapter's scripted host names no rangeBytes, so the bundle family still records the chunk path. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus after the sibling-stop test fix Baseline moves from 34fda6f to 97b13ec. All 787 goldens and the scenarios file change only their `baseline` line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the second pipeline merge Baseline moves from a25a355 to 4d2cab3, the merge of the pipeline's sibling-stop test fix. Re-recorded: every golden and the scenarios file move only baseline. Against the pipeline branch, only baseline and lockfileSha256 differ; every golden body is identical. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): read the bomb inflation without depending on call order With the fetch's sibling stop removed, a read left over from the previous test inflated into the bomb test's record first, and indexOf(601) picked it. The test now asserts some inflation stopped at 601 and none exceeded its buffer, whatever else ran. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the bomb-test fix Baseline moves from 4d2cab3 to 73fde15, the test-only commit on a fenced path. All 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile-web): tighten the bundle window contract and pin the range sibling stop The range bomb test reads only its own host's inflations, keyed by the gzip bodies that host sent, and plans twenty reads so a missing sibling stop is visible: with stopped.abort() removed it sends all twenty. The range params are an alias of the chunk params, and the chunk data bound is the exact base64 length of a full chunk. The phone's chunk and range replies share one header shape. The window reader closes over the client and bytes() takes the slot length the fetch computes. The host's positional read is readMobileWebBundleAssetWindow. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the window-contract commit Baseline moves from 73fde15 to 7346e00. All 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the main merge Baseline moves from 7346e00 to 9c0fe1a, the merge of main at 98a6a53. Recorded with --record: all 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile-web): drop a test cast and shape-named field maps The bomb test's inflation log is typed by its hoisted factory's return instead of an assertion, and the zod field maps shared by the bundle window schemas are windowParamsFields and windowHeaderFields. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the lint fix Baseline moves from 9c0fe1a to f4f0915. Recorded with --record: all 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): page the range fetch fixture on a small advertised grid The fake host names a 4 KiB range grid and a 1 KiB chunk grid on its manifest reply, which the phone pages as it would the real ones, so the fixtures shrink to a few KiB with the same shapes and each asset is hashed once. The file runs in about 360 ms instead of 3.5 s, which a loaded CI runner pushed past the 5 s test timeout. The desktop range suite still pins that the real host names MOBILE_WEB_BUNDLE_RANGE_BYTES. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-pin the recording corpus at the range fixture fix Baseline moves from f4f0915 to d4d6aad. Recorded with --record: all 787 goldens and the scenarios file change only their baseline line. No golden body moved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
f63e4e6 to
5a5f0a1
Compare
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (1)
📍 Outside the diff (1)
🧹 Nitpicks (1) — 🟢 1 low
Reviewed commit: 5a5f0a1 |
| ) { | ||
| return false | ||
| } | ||
| void session.startFreshColdRestoreAgentResume(startup) |
There was a problem hiding this comment.
🟡 bug · medium
Reservation leaks when resume spawn fails
The reservation taken in resumeWithCandidate is never released if the subsequent spawn fails: void session.startFreshColdRestoreAgentResume(startup) swallows rejection and no release runs on that path (src/renderer/src/components/terminal-pane/pty-connection/agent-resume-recovery-install.ts:91). The entry stays in paneKeyBySessionId for the pane's lifetime, and the dispose-time release is additionally skipped when a successor binding already occupies the pane slot. Impact: a session whose respawn failed stays reserved for other panes, so agentResumeSessionsReservedElsewhere filters it out of every future chooser and the transcript can never be resumed elsewhere.
📋 Prompt for AI Agents
In src/renderer/src/components/terminal-pane/pty-connection/agent-resume-recovery-install.ts around lines 87-92, after reserveAgentResumeSession(candidate.providerSession.id, session.cacheKey) succeeds, attach a .catch to the promise from session.startFreshColdRestoreAgentResume(startup) that releases this pane's reservation for candidate.providerSession.id (add a single-entry release such as releaseAgentResumeSession(sessionId, paneKey) that deletes the map entry only when the holder equals paneKey, to src/renderer/src/lib/agent-resume-session-reservations.ts) so a failed spawn does not leave the session permanently reserved and unresumable in other panes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a5f0a168c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const depth = result.appliedSessionDepth | ||
| if (depth === undefined) { | ||
| return 'scan-truncated' |
There was a problem hiding this comment.
When an SSH host lacks the relay aiVault.listSessions method and scanSshAiVaultSessions falls back to scanRemoteAiVaultSessions, the returned result has no appliedSessionDepth (remote-session-scanner.ts:129-133). Therefore this branch classifies every otherwise successful filesystem fallback as scan-truncated, even though the caller requested unlimited: true, so automatic recovery can never work through the supported legacy SSH fallback. Stamp that fallback result with the depth it actually applied.
AGENTS.md reference: AGENTS.md:L89-L91
Useful? React with 👍 / 👎.
Found on PR #2 against the same code, never raised here. The plain shell this connection just spawned still owns the pane's stable key, so `startFreshColdRestoreAgentResume` reached `beginPtyIpcSpawn`, found that owner, and REATTACHED it instead of running the resume command. Recovery and every chooser selection reported success and closed the chooser over the same untouched shell. `reattach-result-handler` retires an adopted shell only for a passive sleeping-record restore, and this startup deliberately carries no record, so nothing retired it. The binding is now disconnected and cleared first, the way that passive path does it, before the replacement spawn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This PR is superseded by #6, and its base branch is merged nowhere — so merging it would land nothing. Here is where each open finding actually stands, and my recommendation. Why it cannot merge as-isIts base, Retargeting does not fix it. Against Where each open finding stands
The bound-shell P1 was the valuable one. It was raised here and never on #6, but #6 carries the same code, so it was live there too: the plain shell still owned the pane's stable key, RecommendationClose this once #6 lands, rather than retarget. #6 covers every file this PR touches and carries seven further fix commits. The eleven-commit history and five rounds of discussion stay on Not closing it myself: it is @sawfish-f6's, and the upstream PR they will cut from |
|
Closing as superseded: #6 is merged into This is the disposition set out above, now that its condition has been met. To restate why closing rather than retargeting:
Nothing is lost. The eleven-commit history stays on The remaining work is the upstream PR into Deferred findings from this feature are recorded on #6 rather than dropped: the Windows SSH shell family (needs a new host-reported field across the remote boundary), claim identity scoping by host and agent, split-pane chooser stacking, the evidence watcher not re-arming, a reservation leak on a failed spawn, and unparseable-timestamp rows vanishing instead of forcing ambiguity. One caveat worth carrying upstream: no test exercises a real host restart, which is this feature's entire premise. |
ELI5
Stacked on #1. Once a terminal remembers which conversation it had, this makes it come back on its own. If the machine can't tell which of several conversations belonged to that terminal, it asks you instead of guessing — because picking wrong forks the conversation permanently.
What Changed
Before: a pane whose resume handle was lost (restarts before #1 landed, or an agent started outside Orca) reopens as a bare shell forever, even though the host still holds the transcript.
After: when you open such a pane, Orca asks its execution host which sessions exist for that workspace, and either resumes the right one or shows a small chooser.
The mechanism, in layers, each independently tested:
agent-resume-candidate-source— asks one host (aiVault.listSessions, scoped to the workspace) and validates every field at that boundary. An unreachable host answers with no candidates rather than throwing.agent-resume-candidate-resolver— a pure function that refuses rather than guesses. Filters: agent match, exact cwd (never a prefix — that would pull in a sibling worktree), a message-count floor, and exclusion of any session a live pane already holds. Then rungs: sole candidate → resume; the tab's own last session id → resume; exactly one inside the tab's lifetime window → resume; otherwise offer a choice.agent-resume-pane-decision— three gates before anything destructive: the pane already owns a record, the pane is hidden, or the user has already typed.AgentResumeChooser— the one visible surface, keyboard-first, shown only when the resolver refused to decide.Timing. The recovery runs after the pane's plain shell has spawned, and a recovered session replaces it by respawning. Holding every spawn behind the host query would tax every terminal you open: the answer comes from a transcript scan measured at ~2.0s on a host holding 1200 transcripts (547 MB). The cost is a brief flicker on panes that actually recover. A resume still only ever rides a spawn — it is never typed into a live shell (
docs/reference/omp-resume-transcript-locator.md).Why
The alternative — auto-picking the newest transcript — is unsafe, and that is not theoretical. During development the newest transcript in a worktree was a 39-line diagnostic session, ahead of the real 758-line one. A resolver that ranked by recency would have resumed the wrong conversation and forked it. Hence the substance floor and the refuse-don't-guess rungs.
Agent-agnostic throughout: resume argv comes from
getAgentResumeArgvfor all ofRESUMABLE_TUI_AGENTS; noclaudespecial case and no hardcoded transcript path.Linked Issue
Fixes #
Visual Proof
Pending. The chooser is new UI and this needs a before/after capture before merge. Everything below it is tested headlessly; the chooser surface itself has not been visually reviewed yet.
Testing
96 tests across the new modules, plus the existing pane suite for the wiring:
pnpm test src/renderer/src/lib/agent-resume*andpending-agent*— 96 passedpnpm test src/renderer/src/components/terminal-pane— 4563 passed, 1 skipped (475 files), confirming the pane wiring breaks nothingpnpm tc— exit 0pnpm run check:code-quality:changed— 0 findings across 27 changed files (including the design-system gate)Notable coverage: the gates are checked before the host is asked (a hidden pane never pays the scan) and again after, so a pane the user starts typing into while the host is answering is never replaced.
Not yet exercised against a real host restart end to end; that is the remaining validation.
AI Disclosure
Authored with Claude (Opus 5), across parallel workers with human-directed review and integration.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Known limitations, stated rather than hidden:
redactForTransportwithholdsfilePathfrom paired clients. They remain record-only.launchConfig— the handle that held it is what was lost — so the pane returns with the agent's configured defaults, not the original turn's flags.listSessionsscans rather than reading an index.searchSessionsis index-backed and would be cheaper, but its empty-query behaviour is unverified and no index was present on the test hosts.Checklist
pnpm lint,pnpm typecheck,pnpm testpassNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.