Repository navigation
test(opencode): start the Tab agent-switch checks from a closed autocomplete - #2223
Merged
Merged
Conversation
…omplete The test before them leaves @effect's row selected on screen. A send that read that old screen before the stand-in's own redraw had no agent to compare after Tab, so the switch went unseen (main run 40643422650). The stand-in draws its closed prompt on Ctrl-L, and the checks wait for it.
|
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 1 minute. 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 |
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
…e, after an unobserved switch-back (#2224) Found by the main-integration lane: Integration failed on run 40646297534 (ci/self-hosted-runner-cache, main 70e19f4). ## Cause (test flake) The Copilot stand-in writes a `:switch` as two log lines, `Unregistering` and then `Registering`. A conversation read between them rightly fails closed: scrollback, no turns, and another `history_id`. The test's wait for the unobserved switch-back stopped at any `history_id` change, so it could stop on that read and then fail with `turns[0]...text` `undefined`. **Reproduced:** with 400 ms between the two writes, the old test fails 3 of 3 with CI's `Expected: "...a03" Received: undefined`, and the fixed one passes 3 of 3. ## Change The loop waits for the Copilot transcript itself: source `copilot-transcript`, a new `history_id`, and the resumed session's first turn. It still throws after 5 s if the binding never advances. ## Proof - **Stress:** `server/copilot.contract.test.ts` passed 10 of 10 on CI's Node 22 and herdr 0.9.3. - **Local note:** this file cannot run on herdr 0.9.5 with Node 24, because the stand-in's process shows as `MainThread` and herdr never names it Copilot. The runs above use CI's versions. - **Local checks:** `bun run check fast` ok. Test-only. Splits: #2222 and #2223. ## Reviews - **omo-native-code-reviewer**, round 1 on the combined change: BLOCK with two HIGHs, both fixed. One was a loop shape that `scripts/conventions.test.ts` counts as a fixed sleep; the other was the OpenCode helper not waiting for its own Ctrl-L. Round 2 on these heads: APPROVE, no findings above LOW. - Test-only, so no Codex review (notes/pr-policy.md: the test-only gate is omo alone).
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
…he first agent ends (#2222) Found by the main-integration lane: Integration failed on main run 40644839354 (30cd84f) and on #2176's run 40643516119. ## Cause (test flake, not a product regression) `a create request's first message > is bound to the agent process it was queued under: a replacement of the same kind ...` started SESSION2 from the shell (`recorder SESSION1; exec recorder SESSION2`). Between SIGKILL of SESSION1 and the `exec`, bash alone is in front. A pane event in that gap makes `launchGone` (shell back in front, `server/core/pending.ts`) hold the message as `first_message_launch_ended`. That is the documented behaviour from #1969: the launched agent did end, and no agent was in front. The test expected `pending_target_changed`, so it failed whenever a read landed in the gap. **Reproduced:** keeping the shell busy for a moment before the `exec` (a builtin loop, so the shell itself stays in front) fails the old test 3 of 3, with exactly CI's `Received: "first_message_launch_ended"`. A `sleep 1` there does not reproduce it, because `sleep` is another process and reads as a changed target. ## Change - The npm-style launcher fixture now also handles SIGUSR1: it starts SESSION2, and ends SESSION1 once SESSION2 has spawned, so an agent is in front throughout. Its exit-driven path (used by the #1545 test) is unchanged. - The test uses that launcher and waits until herdr no longer lists SESSION1 before it reports idle. ## Proof - **Mutation:** with `samePendingIdentity` ignoring process pid and birth, the new test fails 2 of 2 (the message is typed into SESSION2). - **Stress:** the first-message block plus the two replacement tests (21 tests) passed 10 of 10 on CI's Node 22 and herdr 0.9.3, through `dori heavy` on a loaded host. - **Local checks:** `bun run check fast` ok. Test-only: no app code changes. Splits from the same lane: #2223 and #2224. ## Reviews - **omo-native-code-reviewer**, round 1 on the combined change: BLOCK with two HIGHs, both fixed. One was a loop shape that `scripts/conventions.test.ts` counts as a fixed sleep; the other was the OpenCode helper not waiting for its own Ctrl-L. Round 2 on these heads: APPROVE, no findings above LOW. - Test-only, so no Codex review (notes/pr-policy.md: the test-only gate is omo alone).
Owner
Author
|
Merging 31f4d5f, a test-only fix from lane main-integration for the OpenCode v2 native Tab switch Integration failure seen on main. The Tab agent-switch checks now start from a closed autocomplete. Codex reviewed it and found no issues; its virtual-time probes support the stated failure mechanism and the fix. No production change and no fixed sleep were added. The required checks are green, and CodeRabbit did not review this head. |
devswha
added a commit
that referenced
this pull request
Oct 29, 2026
…s last pane stopped being OpenCode (#2389) Found by the flake-hunt lane: Integration failed once on 2026-10-28 12:54Z (job 122825435909), after #2223, at `server/opencode.contract.test.ts` "OpenCode v2 native service > closes the shared stream when its last verified pane ends or stops being OpenCode". The error was "the stream closing after the last verified pane stopped being OpenCode did not happen (native {"stream":true,"panes":["w8:p1"]})". A rerun passed. ## Cause (product race) Native OpenCode ownership (`verifiedPanes` in `server/opencode-native.ts`, and the shared event stream to `opencode serve`) was released only by `onStatus` naming another agent and by pane end (`server/core/status.ts`). The status collector (`server/collector.ts`) does not always call `onStatus`: - It stores a first-seen pane as a baseline without one. - It replays a gap by comparing status, not agent. - It recovers a lost subscription through `onResync`. So when a pane stopped being OpenCode and the bridge learned it only from a snapshot, the stream stayed open with nothing left to release it: an event stream to `opencode serve` held for good. The CI failure is one such ordering. **Reproduced:** with the real collector and the status hub's callbacks (`server/core/status.test.ts`, "native OpenCode ownership reconciliation"), a first baseline, an equal-status gap and a resync that each show the pane no longer OpenCode all leave `{ stream: true }` on origin/main: 6 cases fail. ## Change (`server/core/status.ts`, `server/opencode-native.ts`) - **When a snapshot releases:** a reconciled snapshot releases a verified pane when it names another agent, or a known status without an agent. - **Handover kept:** a pane with no agent and status `unknown`, as during a server handover, is kept. The exception is when `pane.process_info` proves the shell alone is in front: one foreground process that is the shell by name and argv, not just by pid, since `exec opencode` keeps the shell's pid. That read happens only for verified panes in that state, and not on Windows, which reports only the shell. - **Older snapshots lose:** a verification generation read before each snapshot keeps ownership verified since then. Only verified panes are released, so an older snapshot cancels neither a newer verification nor one still in flight. Replay releases go through the same guarded path. ## Proof - **Regression cases:** 21 passed and 6 failed before the fix. All 28 pass after, including exec, handover, newer verification and in-flight verification. Each guard's cases fail when that guard is removed (checked by both reviewers). - **Related suites:** `server/core/status.test.ts`, `server/collector.test.ts` and `server/opencode-native.test.ts` pass 149/0. - **Contract:** `bun run check run bun test --timeout 30000 ./server/opencode.contract.test.ts` passed 24/24, three times in a row. - **Local checks:** `bun run check fast` ok. ## Reviews - **Codex Sol 6.1 (fast, xhigh):** - Round 1: BLOCK, with two MEDIUMs. - The shell was proven by pid alone; after `exec opencode` that released a live pane. - An older snapshot released a newer verification. - Both fixed. - Round 2: APPROVE with no findings. It removed each guard in turn and saw its cases fail. - **omo-native-code-reviewer:** - REQUEST_CHANGES on one MEDIUM: snapshots also released never-verified panes, cancelling a verification still in flight and its send watches. Fixed: only verified panes are released, with a regression case. - Delta re-review: APPROVE. ## Known limits (LOW, not fixed here) - **Late verification:** a verification that starts while the pane is labelled OpenCode, during an event gap, can still complete after a newer snapshot shows another agent. The pane then holds the stream until the next release trigger: an agent-change event, pane end, a failed re-verification, or the next snapshot. A precise fix would compare a verification's start with the snapshot's read time. - **Live events:** a live event with no agent and status `unknown` still releases without shell proof, as before this change. - **Shell list:** it lacks pwsh, xonsh and elvish. That fails safe: the stream stays open. ## Second commit: a re-verification begun after a snapshot's read The merge pass blocked 0c9ceca on one reproduced MEDIUM: an older status snapshot could cancel a newer in-flight re-verification of an already-verified pane. The checkpoint guarded only completed verifications, so the successful session lookup was discarded and the shared stream closed under a pane that was still OpenCode. **Change:** each verification is stamped when it starts, from the same counter. `releaseOpenCodePane(pane, checkpoint)` stands down while the pane has a verification begun after that checkpoint; the newer attempt settles the pane itself. Releases without a checkpoint (status events, pane end) still cancel everything in flight. **Proof:** - **Regression:** a new test with prior ownership in `server/opencode-native.test.ts` failed before the fix (the re-verification resolved `null` and the stream closed) and passes after. - **Related suites:** 150/0. - **Contract:** the opencode contract file passed 24/24 three times. - **Local checks:** `bun run check fast` ok. **Reviews:** - **Codex Sol 6.1 (fast, xhigh):** APPROVE, no findings. It ran 100 repetitions in a source harness. - **omo-native-code-reviewer:** APPROVE, with LOW notes: - With an older and a newer verification overlapping, the early return spares the older one too. Two overlapping verifications can already race this way. - Two early exits of a newer attempt (process roots, a missing session id) return without releasing, so the stream waits for the next trigger. - No test covers a checkpoint release cancelling a verification begun before it.
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.
Found by the main-integration lane: Integration failed on main run 40643422650 (7dc187a).
Cause (test flake from #1991's test order, not a regression from #2185 or #2200)
refuses a confirmed Tab agent switch during immediate delivery without Enterruns right afterdelivers a queued message completely when Tab leaves the skill unconfirmed, and that test leaves @effect's autocomplete row selected (the recorded screen whose status row names no agent). The stand-in redraws only when it is typed to. If herdr has not yet drawn the screen that the next send's own@effectproduces,typeWithSkills(server/opencode-skill-attach.ts) reads the old box. The agent before Tab is then unknown, and the Build to Plan switch cannot be seen, so the send succeeds. The queue case then fails on the outcome the immediate case left behind.Reproduced: delaying the stand-in's
@effectredraw by 400 ms fails both cases 3 of 3, with CI'sok: trueand the recorded outcome.Change
closedPrompt()sends Ctrl-L, then waits until the stand-in has logged it and herdr shows no selected @effect row. Both Tab-switch cases call it before they note what was typed.Proof
server/opencode.contract.test.ts(24 tests) passed 10 of 10 on CI's Node 22 and herdr 0.9.3. That includes fix(server): close the OpenCode stream when the last server stops, and drop #1548's duplicate paths #2200'scloses the shared stream when its last verified pane ends or stops being OpenCode. Its failure in run 40639664878 was fix(server): close the OpenCode stream when the last server stops, and drop #1548's duplicate paths #2200's pre-merge head, which still had the stop-then-create test that the merged version replaced.bun run check fastok.Test-only. Splits: #2222 and #2224.
Reviews
scripts/conventions.test.tscounts as a fixed sleep; the other was the OpenCode helper not waiting for its own Ctrl-L. Round 2 on these heads: APPROVE, no findings above LOW.