Repository navigation
test(portal): wait for the unrelated shell to run its script before resuming - #1638
Merged
Merged
Conversation
…esuming "does not stop an unrelated process whose argv merely mentions expose and the identity path" called resume() right after Bun.spawn. Until the shell has exec'd, /proc/<pid>/cmdline can still show the parent's argv or read empty mid-exec, so identifyProcess() threw "Could not verify the identity of recorded Portal process" and Fast checks failed on main twice today (runs 39733017457 and 39734683255). The shell now prints READY first and the test waits for it, as #1419 did for the sibling interpreter test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main test change: waiting for the unrelated shell to run its script before resuming. |
| Description check | |
The description includes the required Change and Validation sections. It explains the failure, the resulting behavior, the scope, and the validation results. |
| Linked Issues check | |
No active directly linked issue remains. Issue #1419 is closed and provides historical context only. No linked-issue coding requirements apply to this pull request. |
| Out of Scope Changes check | |
The change is limited to server/portal.test.ts. It waits for the spawned shell's READY output before resume(). This directly addresses the reported test race. The summary reports no production changes… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
Full details: Out of Scope Changes check
-
Explanation
The change is limited to server/portal.test.ts. It waits for the spawned shell's READY output before resume(). This directly addresses the reported test race. The summary reports no production changes or unrelated files.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
✨ 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.
devswha
added a commit
that referenced
this pull request
Oct 22, 2026
## Change Makes five tests that failed on CI pass reliably. All five failures were in the tests' own setup, not in product code; no product code changes. No timeout, retry, fixed sleep or weakened assertion was added. - **omo multiline paste** (`server/core/omo-pane-send.test.ts`, failed on main 10-22 05:29Z and 08:15Z: expected bracketed paste, received bare `a\nb`). - Cause: the fixture shell printed "ready" and then `exec`'d `sleep`. During that exec, Linux can briefly show an empty `/proc/<pid>/environ`, so the composer missed `OMO_CODING_AGENT_DIR` and the holder record. - Fix: the ready shell now stays blocked on stdin, and cleanup waits for it to exit. - Under load: 7 failures in 500 runs before, 0 in 500 after. - **Heavy paging tests** (`opencode.test.ts` 1.x and long-session paging and /undo, `conversation.test.ts` Devin cache and Agy ledger; 60 s timeouts at 65-108 s on CI). - Cause: the SQLite fixtures synced every row to disk. Devin alone spent 11.2 s in 496 `fsync` calls. - Fix: the throwaway fixture writers set `PRAGMA synchronous = OFF`; committed visibility, locking, data volume and assertions are unchanged. This follows #1578. - Per test, alone: 5-20 s before, under 0.2 s after. - With eight heavy files at once: 1,506 pass, 0 fail. - **OpenCode v2 contract tests** (`opencode.contract.test.ts`: `@effect` queued while the pane works, failed 3 times, once on main; first key never waits on the event stream). - Cause: the pending receipt confirmed the PTY write before the stand-in had read Enter, and cases moved on before the previous send was consumed. - Fix: each case waits for the stand-in's byte reports to confirm consumption, and matches reports by their offset in the shared log. - Results: 1 failure in 5 runs before, 0 in 13 after. The whole file run 3 times: 60 pass. Also checked: the Portal "unrelated process" failures on main (10-22 06:17Z and 06:33Z) predate #1638, which fixed them. That fixture fails 3 times in 500 runs without #1638's change and never with it. ## Validation - `bun run check fast`: passes. - The changed files on this branch: unit 170 pass, contract 20 pass. Test-only changes; no changelog fragment.
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.
Change
server/portal.test.ts"does not stop an unrelated process whose argv merely mentions expose and the identity path" calledresume()right afterBun.spawn. Until the shell has exec'd,/proc/<pid>/cmdlinecan still show the parent's argv or read empty mid-exec, soidentifyProcess()threwCould not verify the identity of recorded Portal process <pid>.It failed Fast checks on main twice today (runs 39733017457 after #1590 and 39734683255 after #1594; the runs in between passed). The test dates from #746; it started showing up under today's CI load.
The shell now prints
READYbefore its loop and the test waits for that line (bounded) before resuming, the same fix #1419 made for the sibling interpreter test. No production code changes.Validation
HERDR_TEST_MODE=unit bun test server/portal.test.ts: 38 pass, 0 fail, three runs in a row.Test-only change: no changelog fragment, no CodeRabbit re-review needed.
🤖 Generated with Claude Code