test: stabilize Windows fixture waits and case cleanup - #4015
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe tests now capture child output once for synchronization races and await complete cleanup for routed streaming cases. They also standardize xAI test-server setup and validate newer model publication after concurrent sync requests. ChangesRetained root serialization tests
xAI streaming test cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This test-only change improves Windows fixture synchronization and streaming cleanup without changing runtime behavior. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review Please review head All 7 tests / 35 assertions passed on this base and on an isolated journal-PR overlay; the overlay was restored. Typecheck, privacy scan and diff checks passed. Full contributor CI is https://github.com/luvs01/opencodex/actions/runs/34213292833. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='7797586a8899c673eab48886a490e85b480c6d72'
head='37a4cab907216fcae02b95843d2250f06b73da2b'
echo '--- changed files and diffstat ---'
git diff --stat "$base" "$head"
git diff --name-status "$base" "$head"
echo '--- full diff ---'
git diff --find-renames --find-copies --no-ext-diff "$base" "$head"
echo '--- changed file outlines ---'
git diff --name-only "$base" "$head" | while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --view expanded || true
doneLength of output: 8805 🏁 Script executed: #!/bin/bash
set -euo pipefail
file='tests/codex-integration/codex-retained-root-serialization.test.ts'
echo '--- fixture setup, cleanup, and barrier helpers ---'
sed -n '1,245p' "$file"
echo '--- retained-catalog success path ---'
sed -n '340,430p' "$file"
echo '--- persisted-runtime success path ---'
sed -n '435,510p' "$file"
echo '--- relevant identifiers with context ---'
rg -n -C 4 '\b(raceBarrier|captureChildResult|Bun\.spawn|\.stdout|\.stderr|\.exited|afterEach|teardownSandbox)\b' "$file"
echo '--- all direct pipe reads in the fixture ---'
rg -n -C 2 'new Response\([^)]*\.(stdout|stderr)\)\.text\(\)' "$file"
echo '--- whitespace errors in reviewed range ---'
git diff --check '7797586a8899c673eab48886a490e85b480c6d72' '37a4cab907216fcae02b95843d2250f06b73da2b'Length of output: 31704
No blocking findings.
The new early-exit test verifies the exit code, both diagnostic streams, and subsequent access to the same captured result. The late-rejection handler on the derived barrier promise remains in place. Sandbox cleanup still reaps tracked children before root deletion. Static review only. I did not execute the test suite in this sandbox. ✅ Action performedReview finished.
|
luvs01
changed the title
|
@coderabbitai review Please review head The final CI-mode run passed 13 tests / 101 assertions, typecheck, privacy scan and diff checks. Removing the production catalog evidence guard now fails the convergence case by replacing the newer model with the stale one; runtime source was restored byte-for-byte. Removing the xAI drain await fails its gated-finally control. The xAI 10-second timeout and 1.5-second first-delta assertion remain unchanged. Independent read-only review found no required changes. Full contributor CI is https://github.com/luvs01/opencodex/actions/runs/34218151305. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='7797586a8899c673eab48886a490e85b480c6d72'
head='4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0'
echo '--- scope ---'
git diff --stat "$base" "$head"
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
echo '--- diff with zero context ---'
git diff --find-renames --find-copies --no-ext-diff --unified=80 "$base" "$head"
echo '--- changed file outlines ---'
git diff --name-only "$base" "$head" | while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --view expanded || true
done
echo '--- asynchronous, stream, global-state, and catalog references in changed files ---'
git diff --name-only "$base" "$head" | while IFS= read -r file; do
echo "### $file"
rg -n -C 5 \
'\b(captureChildResult|raceBarrier|Bun\.spawn|stdout|stderr|exited|publisher|catalog|stale|fetch|HOME|homedir|AbortController|abort|drain|finally|afterEach|beforeEach|teardown|ownership|native)\b' \
"$file" || true
done
echo '--- source locations for production catalog evidence guards and xAI routed tests ---'
rg -n -C 4 \
'\b(evidence|catalog|convergen|xAI|AbortController|globalThis\.fetch|process\.env\.HOME|homedir|ownership)\b' \
--glob '!tests/codex-integration/codex-retained-root-serialization.test.ts' \
--glob '!tests/**/xai*.test.*' \
. || trueLength of output: 201703 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='7797586a8899c673eab48886a490e85b480c6d72'
head='4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0'
mapfile -t files < <(git diff --name-only "$base" "$head")
echo '--- changed paths ---'
printf '%s\n' "${files[@]}"
echo '--- commit-scoped diff ---'
git diff --no-ext-diff --unified=35 "$base" "$head" -- "${files[@]}"
for file in "${files[@]}"; do
echo "--- outline: $file ---"
ast-grep outline "$file" --items all --view expanded || true
echo "--- numbered content: $file ---"
nl -ba "$file"
done
echo '--- direct stream reads and shared-result call sites in changed files ---'
rg -n -C 4 \
'new Response\([^)]*\.(stdout|stderr)\)\.text\(\)|captureChildResult\(|raceBarrier\(|\.exited\b|Promise\.(all|race|allSettled)\(' \
"${files[@]}" || true
echo '--- changed production-symbol definitions and changed-test references only ---'
for symbol in mergeCatalogEntriesForSync refreshCodexModelCatalog restore globalThis.fetch AbortController; do
echo "### $symbol"
rg -n -C 3 "\\b${symbol//./\\.}\\b" -- "${files[@]}" || true
doneLength of output: 1438 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='7797586a8899c673eab48886a490e85b480c6d72'
head='4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0'
paths_file="$(mktemp)"
trap 'rm -f "$paths_file"' EXIT
git diff --name-only "$base" "$head" > "$paths_file"
echo '--- changed paths ---'
cat "$paths_file"
echo '--- commit-scoped diff ---'
git diff --no-ext-diff --unified=35 "$base" "$head" -- $(cat "$paths_file")
while IFS= read -r file; do
echo "--- outline: $file ---"
ast-grep outline "$file" --items all --view expanded || true
echo "--- numbered content: $file ---"
nl -ba "$file"
echo "--- relevant asynchronous and cleanup paths: $file ---"
rg -n -C 6 \
'new Response\([^)]*\.(stdout|stderr)\)\.text\(\)|captureChildResult\(|raceBarrier\(|\.exited\b|Bun\.spawn|Promise\.(all|race|allSettled)\(|AbortController|\.abort\(|globalThis\.fetch|process\.env\.(HOME|USERPROFILE)|homedir\(|finally|afterEach|beforeEach|teardown|catalog|publisher|ownership' \
"$file" || true
done < "$paths_file"Length of output: 43037 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='7797586a8899c673eab48886a490e85b480c6d72'
head='4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0'
retained='tests/codex-integration/codex-retained-root-serialization.test.ts'
xai='tests/server/server-xai-responses-streaming.test.ts'
echo '--- complete compact diff ---'
git diff --no-ext-diff --unified=12 "$base" "$head" -- "$retained" "$xai"
echo '--- retained-root helpers and changed race tests ---'
sed -n '1,230p' "$retained" | cat -n
sed -n '330,530p' "$retained" | cat -n
echo '--- all retained-root pipe consumers and barrier invocations ---'
rg -n -C 5 \
'captureChildResult\(|raceBarrier\(|new Response\([^)]*\.(stdout|stderr)\)\.text\(\)|Bun\.spawn\(|\.exited\b' \
"$retained" || true
echo '--- xAI fixture complete content ---'
sed -n '1,620p' "$xai" | cat -n
echo '--- xAI routed-case ownership, global restoration, and server cleanup ---'
rg -n -C 8 \
'runRoutedCase\(|drainRoutedCase\(|activeRoutedCase|beforeEach|afterEach|globalThis\.fetch|OPENCODEX_HOME|startXaiTestServer\(|startServer\(|server\.stop\(|signal\.throwIfAborted|signal,' \
"$xai" || trueLength of output: 98233
No blocking findings.
The publisher race test now uses distinct snapshots. Lines 416-418 assert that
The ownership-query exclusion is contained in Static review only. I did not execute the test suite in this sandbox. You are interacting with an AI system. |
리뷰 · 우선순위 44 / 80이 PR은 제품 동작이 아니라 Windows CI 픽스처 수명 문제를 줄이려는 테스트 전용 패치다. 지금 retained-sync 고침은 점수가 낮은 이유: draft이고 제품 회귀가 아니며, 예산 2배는 flaky를 가릴 수도 있다. 다만 파이프 단일 소비자·abort drain은 원인이 분명한 수정이다. types/config 분할과 무관하고, 중복 PR로 바로 닫을 대상도 아니다. hygiene/enforce-target는 현재 초록이다. 라인 tests/codex-integration/codex-retained-root-serialization.test.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
* test(lib): make idle deadline reset timing deterministic Carried from #4041 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test: stabilize Windows fixture waits and case cleanup Carried from #4015 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): verify timeout termination without racing child timers Carried from #4012 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): hold prompt-probe admission through document edits Carried from #4014 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(clients): bound transaction fixture child completion Carried from #4004 (author luvs01) for maintainer-side CI and sequential integration. Closes #4003 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): retain overlapping multiline TOML terminators Carried from #4039 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(cli): reject unsupported caps and report ignored legacy values Carried from #4043 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): share trigger-only delegation guidance with v1 Carried from #4034 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): preserve settings when journal injection hashes are missing Carried from #4006 (author luvs01) for maintainer-side CI and sequential integration. Closes #4005 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(cli): note that --injection still accepts none and minimal CodeRabbit review on #4049: the effort reference documents caps only, so the new rejection of none/minimal reads as a global removal. --injection keeps both values because it sets the injection effort rather than a cap. --------- Co-authored-by: t <a@b.com> Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Landed on Verification at the exact merged head Closing this PR because the change is already on |
lidge-jun
added
the
landed-via-maintainer
2 participants
Summary
Windows shards exposed two fixture lifetime problems: retained-sync barrier and success paths competed for the same child output streams, and an xAI test body could continue after its timeout into the next test's fetch mock. A separate retained-sync failure timed out during the management child's startup before it reached the provider barrier.
finally, before restoring fetch/home globals. Use the existing native-ownership test seam to avoid incidental Windows service queries. Preserve the routed test's 10-second timeout, the first-delta 1.5-second assertion, and all wire assertions.Only two test files change; runtime behavior is unchanged.
Verification
4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0, based ondev7797586a8899c673eab48886a490e85b480c6d72. Currentdev29bb221c3cfad89e4920ac411c9681073929c152is four commits ahead, changing only the package version and release records; neither test file overlaps.CI=true bun run test -- --timeout 60000 --parallel=1 tests/codex-integration/codex-retained-root-serialization.test.ts tests/server/server-xai-responses-streaming.test.ts: 13 tests / 101 assertions passed in 61.37 seconds. Typecheck, privacy scan andgit diff --checkpassed.sync.exitedbefore the original final read reproduced it deterministically; shared capture passed, including an exit-code-7 diagnostic control.fixture/race-modelreplacesfixture/newer-race-model. The former identical-response fixture passed that ablation. The production guard was restored byte-for-byte; the retained publisher variant retains other fences and did not fail this partial ablation.finallycontrol verifies that drain cannot return early; removing its await fails the control. This is a body-lifetime control, not a simulated end-to-end HTTP timeout.4141281b14cc7dad3e3a8b06b727ae4b2ec42ac0, attempt 2. The first attempt passed the changed fixtures in Windows shard 6/6 but failed two unchanged shim cases in shard 1/6 at their 15-second child readiness limits. One job-only rerun passed. Missing child error/exit diagnostics prevent a definitive cause attribution for those original failures; this PR does not claim to fix those shim fixtures.Checklist
Review readiness checklist
Summary by CodeRabbit