Repository navigation
test(agents): prove a version run ended by its own answer, not elapsed time - #2397
Merged
Merged
Conversation
…d time The two runVersion tests checked that the run took under 2 s. Each now asserts what the run itself says instead: - a flag that hangs (a 30 s sleep holding its output) under a 300 ms limit comes back timed out with no exit code of its own, which only the limit can produce; - a flag that prints its version and exits while a child holds its output comes back with exit code 0 and not timed out, so it was not kept until its limit (now 3 s, under the test's own 5 s, so such a regression fails on this assertion rather than the test timeout). With the after-exit read stop removed, this assertion fails (timed out: true). Both still check that the child they started is gone.
|
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 13 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 929c112. Test-only: the two runVersion tests in server/agent-catalog-detect.test.ts (from #2326) no longer assert elapsed time; they assert the detector's own answer, [timedOut, exitCode] = [true, null] when the 300 ms limit ends a 30 s flag and [false, 0] when the flag exits on its own while a child still holds its output (this fails if the after-exit read stop is removed), and that the child is gone. Diff read by the merge pass; no CodeRabbit review needed. |
devswha
added a commit
that referenced
this pull request
Oct 29, 2026
…s instead of racing 2 s (#2402) Found by the flake-hunt lane, with a Dori pin. The Pi listing tests added by #2377 raced the two-second production deadline on busy runners: - **`server/pi-session-metadata.test.ts`** "lists the newest 50 long sessions within the deadline and reads zero bytes on repeat": - #2391's Fast checks (run 40955829630): 6.6 s, "Pi sessions could not be read right now". - #2397's Fast checks (run 40960224000): 8.96 s. - Both passed on rerun. - **`server/api.contract.test.ts`** "lists a 202-session Pi store with 50 long transcripts through the real API without timing out": #2395's Integration run (job 123744721790), a 503 for the same reason. ## Cause (test flake) Both tests list about 200 MiB of Pi transcripts and check what is listed: - the newest 50 rows, with their titles and first prompts; - zero bytes read on a repeat listing; - chunk-sized reads, and an empty queue; - a later rename (API test). They did this under the listing's real deadline, `PI_READ_DEADLINE_MS` (2 s of wall clock, queue time included). On a loaded runner, reading 200 MiB takes longer than that. ## Change The deadline is injected rather than raced: - **Reader:** `PiSessionReader` takes an optional `deadlineMs` beside its existing `read` test seam and exposes it as `listingDeadlineMs`. `piSessions` uses that for the listing's deadline. Unset, it is still `PI_READ_DEADLINE_MS`. - **Server:** `createServer` gets a test option, `piSessionDeadlineMs`, passed to its reader. It is next to the other `*Ms` test options, such as `attachHoldMs`. - **The two tests:** each passes 120 s, past its own timeout. The unit test's timeout is 30 s; the API case's goes from 20 s to 60 s. The "zero bytes on repeat" and chunk assertions stay. - **Docs and changelog:** `docs/development.md` and the changelog fragment say this. The shared two-second deadline is still covered, with a fake clock: aggregate expiry, independent deadlines, queued expiry and FIFO admission, in `pi-session-metadata.test.ts:484-505`, `:585` and `:627`. Those tests also assert that the default is `PI_READ_DEADLINE_MS`. The API's 503 shape is still checked by its own case. ## Proof - **Fail-first (unit test):** with every 64 KiB read delayed 1 ms (scratch), the old test failed 2/2 with the CI error ("could not be read right now"), and the new one passed 3/3. - **Wiring (API test):** with `piSessionDeadlineMs: 1` (scratch), the API case fails with CI's 503, so the option reaches the listing. - **Runs:** the API case passed 3/3 (`bun run check run bun test ./server/api.contract.test.ts -t lists.a.202-session`). The unit file passed 5/5, then 39/39 again after the wording edits. - **Local checks:** `bun run check fast` ok. ## Reviews The PR changes product code (the reader and server test seams, with no production behaviour change), so both reviewers ran. - **omo-native-code-reviewer:** APPROVE (CLEAR) on both rounds. It confirmed that no production caller sets the option: `server/index.ts:738`, `server/remote-entry.ts:88`, the default reader, and the QA script. - LOWs fixed: the test name had claimed the shared-budget proof, and the comment said "a bound no slow runner reaches". Both now describe what the test checks. - LOW, follow-up only: the neighbouring API case "streams ..." reads about 17 MiB under the real 2 s deadline. That is about 0.7 s at CI's worst rate, and it has never failed. If it ever does, it can pass the same option. - LOW, style: the docs paragraph's wording, and the fixture helper's argument order. - **Codex Sol 6.1 Fast xhigh:** APPROVE on both rounds. - Round 1's LOW: the docs still said "under the ordinary deadline". Fixed. - Round 2: no findings.
Merged
devswha
added a commit
that referenced
this pull request
Oct 30, 2026
## Release 0.4.21 The release PR, prepared by step 1 of [docs/development.md#releasing](https://github.com/devswha/ovis/blob/main/docs/development.md#releasing), now re-folded on main `1ef6ea329108a0985b823e8235b03daddcd0a5f4` after the owner held 0.4.21 for more work. The merge pass reported the 0.4.21 queue empty. **Do not merge yet:** the owner judges the final build before release. No tag and no Release run from this PR's author. - **Version:** `version` is 0.4.21 in `package.json` and `herdr-plugin.toml`. - **Changelog:** `bun scripts/changelog-fold.ts 0.4.21 2026-10-30` folded 63 fragments into the 2026-10-30 section of `CHANGELOG.md` (71 entries), linked their PRs and deleted the fragments; `changelog.d` holds only `README.md`. - `by @login` on every outside contributor's PR link: #2361 @eddieparc, #2333 @IdrisGit, #2355 @imurfutur3, #2341 @sulthonzh, #2359 @YuuuXie, #2377 @IdrisGit, #2269 @CharleoYang, #2405 @jpcarranza94, #2414 @homebodify, #2417 @cheol-95, #2418 @eddieparc, #2435 @cheol-95. - Two outside PRs had no fragment and got an entry by hand: #2341 (OpenCode background commands, Added) and #2359 (installer test timing, Maintenance). - Issue reporters are credited in their entries: @blaker3115-Agri (#1488), @Hyd3-14 (#2026), @osamahbeig (#2343), @Shunamxiao (#2329), @matiemer (#2362), @kilhyeonjun (#2344), @eddieparc (#2360), @Ploppy3 (#2236), @sulthonzh (#2340), @WOULDU-pres (#2056), @odfalik (#2038), @aNNdii (#511), @Noobear (#2281, fixed by #2424); #2435 fixes @cheol-95's own #2434. - Two fragments ran past the changelog's width (#2418, #2269); their entries are re-wrapped, words unchanged. #2382's entry reads "OpenCode's installer puts its folder first" (reviewed earlier on this PR). - Maintainer PRs without a fragment and without a user-facing change have no entry: #2353 (check tool), #2371 (guide docs), #2396 and #2397 (tests), #2403 and #2433 (server and client transport seams, no behaviour change), #2437 (test-only fix for #2435's check). #2438's own entry (the sidebar selection marked by its fill again) is dropped: the stroke #2352 added never shipped, so users see no change; #2352's entry keeps only the Windows high-contrast Highlight bar. - **Release summaries:** en, ko, ja and zh each have 5 new, 5 improved and 5 fixed lines. Since the first review: New lists the in-browser Claude Code / Codex sign-in (#2428) in place of the tab/workspace shortcut line; Improved lists Settings → Agents (#2427) in place of the sidebar fold line; Fixed lists the Codex wrong-session fix (#2426, #2430) in place of the phone screenshot paste line. The Artifacts and Claude status-line lines keep their reviewed wording (turned on in Settings). Not in 0.4.21: #2420 (Codex chat behind Kiro), #2065 (Portal sign-in) and #2436 (relay foundations) stay open for a later release. - **Remote bundle:** `REMOTE_BUNDLE_VERSION` rises from 39 to 40 because the bundle inputs (`server/`, `shared/`, `patches/`) changed since remote-v39. After merge: push `remote-v40` on the merged commit and wait for the Remote PC bundles workflow before running Release. - **iOS:** `CURRENT_PROJECT_VERSION` stays 3 (#2370 changed `Info.plist`; build 3 is not known to be uploaded yet). ## Validation - `bun scripts/release-guards.ts --base v0.4.20`: "Release 0.4.21: remote bundle, telemetry Worker and release metadata are in order." - `dori heavy … bun run check fast` on this branch: workflow syntax, generated types and themes, typecheck, build, unit tests all ok. Unit: 9166 pass, 0 fail, 36 skip in 591 files. - Every PR in `v0.4.20..main` was pre-run for the owner's hands-on guide on isolated copies (own herdr session, fresh state, never the owner's live session). Items that need a real device (iPhone tap dictation #2370, dictation keyboards #2386, the installed desktop app's title-bar overlay #2381) are left for the owner's final check. This is a release-metadata PR: it changes no code path. Per the release steps, merge it after CI and the owner's go, then push `remote-v40`, wait for its bundles, and run Release with `0.4.21`.
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
Test-only,
server/agent-catalog-detect.test.ts. The tworunVersiontests from #2326 checkedperformance.now() - started < 2_000, which goes against the rule that tests make no wall-clock assertions. Each now asserts what the run itself returns:sleep 30and then sleeps 30 s), with a 300 ms limit, must come back[timedOut, exitCode] = [true, null]. A null exit code only happens when the time limit wins the race inrunVersion. A read that hung on the pipe the child holds would now fail on the test's own timeout.[false, 0]with the version.timedOutis set only by the timer, sofalseproves it returned before its limit. That limit is now 3 s, under the test's 5 s, so a regression fails on this assertion and not on the test timeout. WithrunVersion's after-exit read stop removed, this test fails withtimedOut: true; I checked that by changing the code.Both tests still check that the child they started is gone. That check uses the existing
ends()helper: a bounded wait for the condition, not a time assertion.Validation
bun test server/agent-catalog-detect.test.ts: 18 pass, 1 Windows-only skip, three runs in a row.bun test scripts/conventions.test.tspasses.bun run check fastis green.