Repository navigation
Conversation
A chat Send while the agent worked went to the bridge's pending list, which is bound to the sending connection's pane lease. A pane or workspace switch, a sleeping phone, a reconnect or a server restart before the turn ended held the message with "The pane connection changed", and it went only after Send now. Send now types the message in at once, as Enter in the agent's terminal does, and the agent's own input queue keeps it (Claude Code until the turn ends; Codex takes it into the turn it is on). The new shared setting Settings > Composer > Queue messages while the agent works (off by default) brings back the bridge's queue; Send queued messages automatically shows under it. A saved record with automatic sending turned off starts with queueing on, so nothing that user asked to hold is typed by itself. The setting joins the telemetry snapshot like every other. scripts/send-while-working-regression.ts reproduces the issue's steps on a stand-in agent. The browser scripts that check the bridge's queue turn the setting on. Closes devswha#874 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
…bar (#964) ## Problem A phone shows a pane in the terminal lens while a desktop tab of the same pane takes the pty back to its own size (a click or key in the desktop terminal, or the desktop window coming back to the front). The phone's xterm stays at phone width while herdr streams rows drawn for the desktop's width, so lines wrap in the wrong place and agent TUIs look scrambled. Typing in the input line, Send, Enter and key-bar taps did not fix it. Only a touch on the grid itself, a focus change or a reload did. ## Cause In the terminal lens, an interact client stores `pane-geometry` in `sharedGridRef` and then ignores it, because it drives the grid itself. The only thing that took the size back while the window kept its focus was `reclaim`, and that runs only for a pointerdown or keydown whose target is inside `term.element`. The phone's usual input paths are all outside that element: - `sendTerminalLine` submits from the `TerminalInput` textarea. - `pressEnter` sends `\r`/keys from the same line. - `pressKey` calls `term.input()` from the key bar, with no DOM event on the grid. ## Fix - The size check moves out of `reclaim` into `reassertIfStale()`. When the shared grid differs from this grid, the tab is in use and has not released its pane, it calls `refit()` (force resize). - `reclaim` keeps its event filters (target inside the grid, no IME/repeat key, primary button) and then calls the helper. - `sendTerminalLine`, `pressEnter` and `pressKey` call it before they send, so the resize frame goes out before the input on the same socket. - `TerminalInput` gets an optional `onFocus`, wired to the same helper. Tapping into the input line counts as the user being back, the same way a tap on the grid does. - No resize was added for the keyboard opening or closing. That is a separate policy question (viewport-resize-2). ## Tests - `scripts/chat-size-regression.ts` `checkFocusedWindowReclaimsTerminalSize` (already in `ui-contexts-checks.ts`, so it runs in the browser lane) now continues with two clients after its existing steps. A desktop at 1280x800 and a phone at 390x844 in line mode take turns: - a tap into the input line; - a line sent with Enter; - a key-bar Ctrl+C; - Enter on the empty line. Each phone step comes after a desktop key has taken the pty back. Each step asserts that the phone's own-size `resize` frame goes out before its `submit`/`input` frame, and that the pane's shell (`stty size`) reports the phone's size again. - `CHAT_SIZE_CASE=reclaim bun scripts/chat-size-regression.ts` now runs that check on its own. - Without the `src/` change the new step fails ("a tap into the input line takes the pane back to the phone's grid"). With it, `bun run check run --build bun scripts/chat-size-regression.ts` (with `CHAT_SIZE_CASE=reclaim`) passes. - `bun run typecheck` and `bun run test:unit` (3069 pass, 0 fail) pass. - `check full` / `concurrent` was not run. ## Notes - Source: a mobile-terminal stability audit, finding viewport-resize-1. Related: #472 (the chat-lens version) and #712 (an attach holds the pane's size). Neither covers this path. - No open PR touches `reclaim`, `sharedGridRef`, `sendTerminalLine`, `pressEnter` or `pressKey`. #951, #950, #946, #945, #936, #935, #932, #910, #894, #859 and #822 edit other parts of `PaneTerminal.tsx`. #936 also edits `TerminalInput.tsx` (dictation), so a small textual conflict is possible there. - The size tug-of-war between two devices that both type into the same pane remains by design: whichever device the user is typing on takes the pty. A "sized by another device" notice and passive adoption of the foreign grid were left out. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
…appers (#970) ## Problem `server/herdr/client.ts` exported an untyped `herdrRpc(method: string, params: Record<string, unknown>)`. Eleven server files called it directly for four methods (`pane.process_info` x10, `agent.get` x6, `pane.send_text` x2, `pane.send_keys` x1), each with its own inline result type. Nothing stopped `{ target }` reaching `pane.process_info`, which herdr then silently answers for the focused pane (the AGENTS.md rule was prose only). ## Change - `server/herdr/methods.ts`: `HerdrMethods`, a `{ [method]: { params; result } }` map for those four methods. Params and result bodies come from the generated schema types; `PaneProcessInfoParams`, `AgentTarget` and `PaneProcessInfo` are now generator roots (`scripts/generate-protocol-types.ts`, regenerated `shared/herdr-api.generated.ts`). The map itself is hand-written because herdr's schema links a method to its params (`request.oneOf`) but not to its answer: answers are one `ResponseResult.oneOf` told apart only by a `type` tag (`pane_process_info`, `agent_info`, `ok`). - `herdrRpc<M extends keyof HerdrMethods>(method, params: HerdrMethods[M]["params"], …): Promise<HerdrMethods[M]["result"]>` is the first overload. An untyped fallback overload stays for methods the map does not list yet. It refuses a typed method name (`UntypedHerdrMethod<M>` is `never` for one), so `herdrRpc("pane.process_info", { target })` is a compile error. `server/herdr/seam.test.ts` pins that with `@ts-expect-error` and checks the wire params of the new wrappers against a stand-in socket. - New wrappers: `paneProcessInfo(paneId)` returns one shared `ProcessInfo` (= generated `PaneProcessInfo`: shell_pid, foreground_processes with pid/name/argv0/argv) and `agentGet(paneId)` returns `AgentInfo`. The existing `paneSendText` / `paneSendKeys` now go through the typed overload. The wrappers call the exported `herdrRpc` binding, so tests that `spyOn(herdr, "herdrRpc")` keep working. - Migrated (no open PR touches them): `server/pi.ts`, `server/omp.ts`, `server/grok-store.ts`, `server/agy-store.ts`, `server/codex.ts` (x3). Behaviour is unchanged: the same params go on the wire, with the same default timeout and the same `.catch` / `try` at each call site. `codex.ts` keeps its old `argv?: string[]` reading through a commented cast, because the schema allows `argv: null` and that path has never handled null. Changing that would change behaviour, so it is left for a separate fix. - Guard: `server/conventions.test.ts` ("herdr socket") now fails when server code outside `server/herdr/` uses `herdrRpc`. The message names the fix ("add a typed wrapper in server/herdr/client.ts; for pane status use the server/collector.ts listeners") and the allowlist `HERDR_RPC_ALLOWED`. A stale allowlist entry also fails. The `events.subscribe` / `subscribeEvents` rules from #949 were already there; their message now also says where `events.subscribe` lives. - AGENTS.md: the `pane.process_info` line points at `paneProcessInfo` / `HerdrMethods` and names the enforcing test. Only that one line changes; #812, #888 and #894 also edit AGENTS.md. ## Allowlist (`HERDR_RPC_ALLOWED`, migrated in PR B) | File | Open PRs touching it | |------|----------------------| | `server/conversation.ts` | #869 #912 #923 | | `server/gjc-runtime.ts` | #912 | | `server/hermes.ts` | #910 | | `server/omo.ts` | #923 | | `server/shell-agent.ts` | #885 #923 | Built from `gh pr list --state open --limit 100 --json number,files` on 2026-10-14. ## Known limits (closed in PR B) - The export is still named `herdrRpc`. Renaming it to `herdrRpcUnsafe` would touch about 33 test files and 30 `scripts/` harnesses, several of them in open PRs (api.contract, gjc.contract, omo.contract, ui-regression, pending-input…). Tests and `scripts/` QA harnesses are outside this guard's scope: they drive herdr (`pane.report_agent`, `workspace.create`, …) and are not part of the bridge seam. - A call with an explicit result type, `herdrRpc<Inline>("pane.process_info", …)`, still reaches the untyped fallback. That is the form the allowlisted files use, and they may not change here. In server code the source guard closes this gap. The fallback goes away once the allowlist is empty. ## Validation - `bun run check fast`: green. A first run had one failure in `server/portal.test.ts` ("does not treat an arbitrary interpreter as a Portal script": the process argv read raced and returned undefined). This change does not touch that file, it passed alone, and the rerun was fully green. - `bun run test:integration`: 455 pass, 0 fail, 14 skip in 38 files (includes the omp, codex, codex-binding, pi, gjc, omo and hermes contract tests). - I checked by hand that the guard fails on a new `herdr.herdrRpc` use in `server/pi.ts`. ## Follow-up: PR B Once #869, #885, #910, #912 and #923 land: migrate the five allowlisted files to `paneProcessInfo` / `agentGet` / `paneSendText` / `paneSendKeys`, empty `HERDR_RPC_ALLOWED`, drop the untyped fallback overload (or rename it `herdrRpcUnsafe` for tests and scripts), and consider handling `argv: null` in `codex.ts`. ## Review Fixed in 290d5f1: - **Union of typed methods (medium, part of finding 1).** The typed overload takes `SingleHerdrMethod<M>`, so `herdrRpc(cond ? "pane.process_info" as const : "agent.get" as const, { target })` no longer compiles. `seam.test.ts` pins this with a `@ts-expect-error` line. Checked with tsc: the inferred `herdrRpc("pane.process_info", { pane_id })` still returns the typed result. - **Allowlist missed #928 and #859 (medium).** `conventions.test.ts` adds `HERDR_RPC_PENDING` with `server/gjc-status.ts` (#928) and `server/index.ts` (#859). Those PRs add a `pane.process_info` call, but neither file calls herdrRpc on main yet, so the "no longer calls" check skips them. It would be better for both PRs to switch to `paneProcessInfo(paneId)`, which is a one-line change in each. After that, these entries come out. - **Docs claimed more than the PR delivers (low).** AGENTS.md now says the guard enforces the rule, not the compiler, and that the allowlisted and pending files still call herdrRpc. The changelog fragment now says "no longer compiles without an explicit result type". "PR B" is replaced with "a follow-up PR that moves these files to the wrappers once their open PRs land". The fallback docblock in `client.ts` and the `UntypedHerdrMethod` comment in `methods.ts` list the holes. Deferred to the follow-up PR that drains `HERDR_RPC_ALLOWED`: - **Explicit result type and widened string (rest of finding 1).** `herdrRpc<T>("pane.process_info", { target })` and `herdrRpc(m as string, { target })` still compile. With an explicit T, the fallback's `M` stays at `string`, and that cannot be closed while the fallback shares the name. Closing it means splitting the fallback out as `herdrRpcUntyped` (or `herdrRpcUnsafe`) and moving about 400 test and script call sites onto it. It also means converting the `herdrRpc<T>("pane.process_info" | "agent.get", …)` calls in the five allowlisted files, which conflicts with their open PRs. `seam.test.ts` keeps both forms as compiling lines with a comment, so the follow-up can turn them into `@ts-expect-error`. - **Error message.** Two overloads are still present, so tsc reports a bad inferred call such as `{ target }` to pane.process_info as `not assignable to parameter of type 'never'` instead of naming the wrong param. That message only gets better with the split. - **Stale pending entries.** Nothing removes a `HERDR_RPC_PENDING` entry on its own. If #928 or #859 switches to `paneProcessInfo` or closes, its entry stays until someone takes it out by hand. - **Tell the open PRs.** #912, #923, #928, #859 and #910 call `herdrRpc<T>("pane.process_info", …)`. They should hear about the switch to `paneProcessInfo` before the split lands. I have not commented on those PRs. Checks: `bun run check fast` passed (typecheck, build, 3452 unit tests). `bun run test:integration` passed (455 pass, 0 fail). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
Closing: typing a Send into the agent automatically conflicts with the pending-input invariant (automatic delivery needs the original live connection and pane lease; see AGENTS.md, Bridge invariants). The held-copy work continues in #1088. Thank you! |
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.
Closes #874
Change
Problem. A chat Send while the agent works goes to the bridge's pending list. That list is
bound to the attach lease of the connection that sent it:
leavePanedetaches on every paneswitch, and a detach, a WebSocket close or an attach restart reaches
holdPending. So a pane orworkspace switch, a sleeping phone, a short network loss or a server restart before the turn
ends holds the message with "The pane connection changed; send this message explicitly after
reviewing it". It goes only after Send now. The 0.4.8 setting Send queued messages
automatically does not help: automatic delivery still needs the original connection and lease.
Behavior after this PR. Send while the agent works types the message in at once, as Enter in
the agent's own terminal does, and the agent's own input queue keeps it. Claude Code holds it
until the turn ends. Codex takes it into the turn it is working on. Nothing waits in the browser,
so leaving the pane cannot hold it. The server's immediate path already handles a working agent:
for Claude it runs the same
claudeInputDraft/freeClaudeBoxcheck and refuses withinput_draftwhen the box holds the user's own typing.New setting. Settings → Composer → Queue messages while the agent works (
queueWhileWorking,off by default, shared like
autoSendQueued) brings back the bridge's queue unchanged. Sendqueued messages automatically now shows under it only while it is on, since it has no effect
otherwise. A saved settings record with
autoSendQueued: falseand noqueueWhileWorkingstartswith queueing on: that user asked that nothing go by itself, and Send at once would do exactly that.
Why a setting, not removing the queue
Two options were weighed:
queued. It is the smallest client diff, but it turns Send queued messages automatically (feat(chat): add a setting to hold queued messages for Send now #836,
merged a day before this) into a dead switch, drops the bridge's next-turn delivery for Codex
users who rely on it, and leaves the server's automatic queue reachable only from older clients.
the issue for everyone by default and keeps every existing behavior one switch away: next-turn
queue with automatic send, or hold for Send now.
This PR takes (b). Criteria: no dead setting or unreachable feature, no change to the server or
the wire protocol, the issue fixed by default, and an opt-out for the Codex case below. A boolean
next to the existing toggle is smaller than turning
autoSendQueuedinto an enum, which wouldrename a shared preference key and change a telemetry field's type.
Codex trade-off. Codex treats Enter during a turn as steering: the message joins the current
turn instead of waiting for the next one. That is what typing in Codex's own terminal does, and it
is what this PR makes Send do. A user who wants next-turn delivery for Codex turns the new setting
on. If you prefer the queue to stay the default, flip
DEFAULT_SETTINGS.queueWhileWorkingtotrue: everything else, including the regression script, stays valid.Telemetry deploy note.
TelemetrySourceSettingsisOmit<Settings, text-only keys>, so the typesystem requires every new setting in the snapshot.
queueWhileWorkingis added toshared/telemetry.ts(type and validator). The receiver refuses unknown fields, so the Worker needsa redeploy before the release that ships this, as
telemetry/README.mddescribes.Files
src/lib/compose.ts:composerDelivery(agent, status, queue)returns"queue"only when queueingis on and the agent works.
src/lib/settings.ts,shared/preferences.ts,shared/telemetry.ts: the setting, its default,sanitize with the migration, shared scope, telemetry snapshot.
src/components/PaneTerminal.tsx: passes the setting tocomposerDelivery.src/components/SettingsDialog.tsx: the new row; the automatic-send row shows under it.src/lib/i18n.{ko,ja,zh,tr}.ts: two new strings.scripts/send-while-working-regression.ts(new), wired intoscripts/ci-browser.shaftersticky-modifiers-regression.tsso that the lane order pinned inscripts/ci-browser.test.tsstays as it is; that test's script count goes from 43 to 44.
scripts/pending-pane-switch-regression.ts,scripts/ui-regression.ts(through Settings, beforeits pending section), the
queue-*scenarios ofscripts/chat-size-regression.ts,scripts/composer-fit-demo-regression.tsandscripts/machines-browser-qa.ts.AGENTS.md,src/AGENTS.md,docs/guide.md,CHANGELOG.md(Unreleased → Fixed).src/lib/compose.test.ts,src/lib/settings.test.ts, the snapshot key count insrc/lib/api.test.ts, the fixture intelemetry/worker.test.ts.No server, protocol or demo-transport change: the demo already answers an immediate submit.
Validation
Run on Windows 11 x64, Bun 1.4.3, herdr 0.9.3, Chrome. Browser scripts used the disposable
herdr-web-ui-testsession.scripts/test-herdr.tsonmainresolves the test socket underXDG_CONFIG_HOMEon Windows, so a one-line localAPPDATApath fix was applied for these runs only.It is not part of this PR.
Regression, RED then GREEN.
scripts/send-while-working-regression.tsstarts a stand-in agent(a copy of the Bun binary named
claude, recording every byte the pane receives) and reports itworking. It sends from the chat, switches to another workspace, ends the turn, comes back, and
expects the text in the pane exactly once, committed with Enter, with no pending row. Then it turns
the new setting on in Settings and checks that Send waits in the bridge's queue with nothing typed,
and that Discard types nothing.
main's client (fix stashed, rebuilt)AssertionError: timed out: the message reached the agent rather than a list the pane switch heldPASS Send while the agent works types the message in at once; a workspace switch and the turn's end cannot hold itandPASS with Queue messages while the agent works on, Send waits in the bridge's queue for the next turnUnit tests, RED then GREEN. The new cases in
compose.test.tsandsettings.test.tsfailedbefore the change (
Expected: "immediate", Received: "queue";Expected: false, Received: undefined; 2 fail, exit 1) and pass after it.Other browser scripts run here:
scripts/composer-fit-demo-regression.ts: exit 0, all PASS lines. A copy without the opt-infailed (exit 1,
TimeoutErrorwaiting for the pending strip), so the opt-in is needed.scripts/settings-pages-demo-regression.ts: exit 0. Every Settings page, including Chat with thenew row, fits a 390px and a 320px phone in en, ko, ja and zh.
Not run here (Linux CI or an SSH fixture needed):
scripts/ui-regression.ts(with thechat-size-regression.tschecks it imports),scripts/pending-pane-switch-regression.ts(itsstand-in uses
exec, which a Windows PowerShell pane does not have) andscripts/machines-browser-qa.ts(needs
SSH_TEST_KEEP=1 bun run test:ssh). Their changes only turn the setting on before theirqueue checks: a settings record in an init script, as the browser scripts already seed settings
(
machines-browser-qa.tsgains one, seeded only when no record exists), or Settings → Chat inui-regression.ts, as it already does for Chat width. A typecheck of these scripts with the repo'stsconfig.jsonfound no error on a changed line.server/telemetry.test.tshas one Windows-only failure (a0o600file-mode check). It fails thesame way on unmodified
main.🤖 Generated with Claude Code