Repository navigation
test(pending): start a first message's same-kind replacement before the first agent ends - #2222
Conversation
…he first agent ends The shell was alone in front between SESSION1's kill and SESSION2's exec, and a read there rightly holds the message as first_message_launch_ended (#1969), so the test expecting pending_target_changed failed now and then (main run 40644839354, #2176's run 40643516119). The launcher fixture now starts SESSION2 on SIGUSR1 and ends SESSION1 once it runs.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Launcher-driven replacement test server/pending-input.contract.test.ts, changelog.d/integration-flake-first-message-replacement.md |
The launcher starts SESSION2 on SIGUSR1 and kills SESSION1 after SESSION2 spawns. The test signals the launcher, waits for SESSION1 to disappear, and checks for a foreground Claude replacement. The changelog describes the prior timing gap and intermittent failure. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other · Severity of issue fixed: Low
Possibly related PRs
- devswha/ovis#1969: This PR updates the same first-message contract fixture to test replacement while the launcher remains in front.
Suggested reviewers: odfalik
Merge Risk: 
ffa76
-
No concrete merge-blocking risk was established for this fixture-only change.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the main change: a test change that starts a first message's same-kind replacement before the first agent ends. The PR objectives confirm the fixture now starts SESSION2 on SIGUSR1 and… Description check The description follows the template. The Change section describes the problem (a test flake caused by the shell being in front between SESSION1 and SESSION2) and the resulting behavior (the launcher … Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: a test change that starts a first message's same-kind replacement before the first agent ends. The PR objectives confirm the fixture now starts SESSION2 on SIGUSR1 and ends SESSION1 after it spawns, which fits the title. The title is specific and single-sentence, and it uses the conventional test(scope) prefix.
Full details: Description check
-
Explanation
The description follows the template. The Change section describes the problem (a test flake caused by the shell being in front between SESSION1 and SESSION2) and the resulting behavior (the launcher fixture now starts SESSION2 before SESSION1 ends). The Validation section lists the mutation test, the stress run, and the local check results, along with the review outcome. The header references CONTRIBUTING.md for checks, but the description does not state whether contract-test coverage applies. This is a test-only change with no HTTP/WS changes, so that omission is minor. The description also includes a changelog entry in changelog.d/, which the template comment asks for.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…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).
|
Merging ffa7612, a test-only fix from lane main-integration for the 'first message bound to the agent process' Integration failure seen on main. The test now starts the first message's same-kind replacement before the first agent ends. Codex reviewed it and found no weakened assertion, no production change and no fixed sleep, and its mocked launcher probes support the stated failure mechanism. The required checks are green, and CodeRabbit did not review this head. A LOW timing risk in a neighbouring, unchanged test goes to a follow-up issue. |
…omplete (#2223) 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 Enter` runs right after `delivers 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 `@effect` produces, `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 `@effect` redraw by 400 ms fails both cases 3 of 3, with CI's `ok: true` and the recorded outcome. ## Change - The stand-in draws the recorded closed-autocomplete screen on Ctrl-L. - `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 - **With the fix:** the same 400 ms delay passes 4 of 4, and 3 of 3 again after review. - **Stress:** `server/opencode.contract.test.ts` (24 tests) passed 10 of 10 on CI's Node 22 and herdr 0.9.3. That includes #2200's `closes the shared stream when its last verified pane ends or stops being OpenCode`. Its failure in run 40639664878 was #2200's pre-merge head, which still had the stop-then-create test that the merged version replaced. - **Local checks:** `bun run check fast` ok. Test-only. Splits: #2222 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).
…the first one ends (#2380) 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" expected `pending_target_changed` and received `first_message_launch_ended`. ## Cause (test flake) The test brought the launched agent up with `agent; exec agent2` and killed the first one. Until the shell ran `exec`, the shell alone was in front. If a status event arrived in that gap, the drain's `learnLaunched` read the shell back in front (`launchGone`'s shell-back branch in `server/core/pending.ts`) and rightly held the first message with `first_message_launch_ended`. A held reason is never revised by later events, so the test's `pending_target_changed` failed. 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 exact `Received: "first_message_launch_ended"`. A `sleep 1` gap does not reproduce it: then `sleep` is 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 - **Fail-first:** with the gap and the status events, the old test fails 5/5. - **After:** with the same idle/working events fired during the replacement, the new test passes 5/5. Plain runs pass 10/10, then 3/3 after the review's control-line change. - **Whole file:** `server/pending-input.contract.test.ts` passes 105/0. - **Local checks:** `bun run check fast` ok. One earlier run failed only on the `server/codex-processes.test.ts` READY-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) - The same file's `: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 - **omo-native-code-reviewer:** APPROVE, CLEAR. It confirmed the cause in `launchGone`/`learnLaunched`. As a mutation check, it stopped `learn()` from recording the launched process, and the test failed with the message typed into the replacement, so it still tests "never typed". - LOW-1 (the control line also passed with no claude listed) was taken: the test now checks the replacement exists first. - LOW-2: the launcher command repeats `setup()`'s, because this test creates its pane through `/api/tab/create`. - Test-only, so no Codex review (notes/pr-policy.md).
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 theexec, bash alone is in front. A pane event in that gap makeslaunchGone(shell back in front,server/core/pending.ts) hold the message asfirst_message_launch_ended. That is the documented behaviour from #1969: the launched agent did end, and no agent was in front. The test expectedpending_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'sReceived: "first_message_launch_ended". Asleep 1there does not reproduce it, becausesleepis another process and reads as a changed target.Change
Proof
samePendingIdentityignoring process pid and birth, the new test fails 2 of 2 (the message is typed into SESSION2).dori heavyon a loaded host.bun run check fastok.Test-only: no app code changes. Splits from the same lane: #2223 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.