Repository navigation
test(pending): start a launched agent's same-kind replacement before the first one ends - #2380
Merged
Merged
Conversation
…the first one ends The create-request test for a replacement before the page attaches ran `agent; exec agent2`: after the first agent was killed, the shell alone was in front until the exec. A status event in that gap made the drain read the shell back in front, rightly hold the first message with first_message_launch_ended, and later events never revise a held reason, so the test's pending_target_changed failed (2026-10-28, after #2222 fixed the sibling test). Start the replacement through the launcher's SIGUSR1, which ends the first agent only once the second runs, as #2222 does, and wait until herdr no longer lists the first.
|
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 39 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 |
Owner
Author
|
Merging 4b57385. Test-only: the pending-input contract case for a launched agent's same-kind replacement now uses a launcher that starts the second agent before ending the first, so a read in the gap between them (where the shell alone is in front) no longer races the assertion. Diff read by the merge pass; no CodeRabbit review needed. |
devswha
added a commit
that referenced
this pull request
Oct 29, 2026
…s exit too (#2396) This is the second commit of #2373, which merged at its first commit (7597f65) before this one reached the branch. It was reviewed there; it is cherry-picked onto current main here. Found by the flake-hunt lane on #2380's Integration run (job 123700768179): `server/take-over.contract.test.ts` "consumes a click when the running ordinary retry succeeds, before later displacement" failed at `:89`, where `pty-data` contained "terminal attach taken over". ## Cause (test flake) This is the same cause as #2373: the 100 ms default attach hold. Displaced by another client's `--takeover`, herdr prints its teardown and "terminal attach taken over", then exits. The bridge keeps an attach's last line in the output tail for the hold (`tailTimer` in `server/core/terminal-process.ts`) and drops it at a displaced exit (`flushTail(displaced)`). An exit more than 100 ms after the line, as on a busy runner, let the line reach clients first. ## Change A 2 s hold for this test too, as #2373 gave the ordinary-failure test. The changelog fragment from #2373 now names both tests. ## Proof - **Fail-first:** with the fixture's `real` wrapper exiting 300 ms after herdr (scratch), the old test failed 3/3 with CI's `not.toContain` and the new one passed 3/3. - **Whole file:** passed 9/9 three times. On current main with this cherry-pick it passed 9/9 again (`bun run check run bun test server/take-over.contract.test.ts`). - **Local checks:** `bun run check fast` ok. Test-only. ## Reviews - **omo-native-code-reviewer:** APPROVE, CLEAR (on #2373's branch). It confirmed the tail-hold mechanism and that the test still asserts the consumed click, the retry without `--takeover`, and no taken-over text. - LOW product note, not fixed here: production also holds the tail 100 ms, so on a loaded machine, when another client takes over, a user can briefly see herdr's teardown line until the next attach redraws. A longer tail hold for displacement would remove it. - Test-only, so no Codex review (notes/pr-policy.md).
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 flake-hunt lane: Integration failed and passed on a rerun with no code change on 2026-10-28 13:24Z (job 122843077413, after #2222 fixed the sibling test the same way):
server/pending-input.contract.test.ts"a create request's first message > queued while the shell was still in front, is bound to the agent the launch brought up: a same-kind replacement before its page attaches gets it held, never typed" expectedpending_target_changedand receivedfirst_message_launch_ended.Cause (test flake)
The test brought the launched agent up with
agent; exec agent2and killed the first one. Until the shell ranexec, the shell alone was in front. If a status event arrived in that gap, the drain'slearnLaunchedread the shell back in front (launchGone's shell-back branch inserver/core/pending.ts) and rightly held the first message withfirst_message_launch_ended. A held reason is never revised by later events, so the test'spending_target_changedfailed. Both reasons hold the message and never type it, so this is not a delivery bug.Reproduced: with a builtin gap (
read -t 1, which keeps the shell itself in front) and an idle/working report during it, the old test fails 5/5 with CI's exactReceived: "first_message_launch_ended". Asleep 1gap does not reproduce it: thensleepis in front, and the read counts as a target change.Change
As #2222 does for the sibling test, the replacement starts through the launcher's SIGUSR1. The launcher ends the first agent only once the second runs, so a claude process is in front throughout. The test then waits until herdr no longer lists the first agent, and checks that the replacement is a live foreground agent with another pid.
Proof
server/pending-input.contract.test.tspasses 105/0.bun run check fastok. One earlier run failed only on theserver/codex-processes.test.tsREADY-line flake, which this PR does not touch and which is fixed separately; the rerun was ok.Test-only.
Not reproduced (noted, not fixed here)
:475"queued command was not recorded" (the "see C:\temp" test, once on main 2026-10-29 01:09Z): the recorder saw no CR within 8 s, and the log holds no state to tell an unsent Enter from a missed notification.Reviews
launchGone/learnLaunched. As a mutation check, it stoppedlearn()from recording the launched process, and the test failed with the message typed into the replacement, so it still tests "never typed".setup()'s, because this test creates its pane through/api/tab/create.