Skip to content

test: stabilize full-suite isolation and integration budgets - #5849

Closed
lzfxxx wants to merge 8 commits into
lidge-jun:devfrom
lzfxxx:fix/test-fixtures-isolated-dev
Closed

lzfxxx wants to merge 8 commits into
lidge-jun:devfrom
lzfxxx:fix/test-fixtures-isolated-dev

Conversation

@lzfxxx

@lzfxxx lzfxxx commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Stabilize isolated fixtures for compaction replay, service claim ownership, and remote workspace argv validation. The service-path assertion now permits only the fixture path or the guarded legacy test home; the Unix argv fixture chooses a single-link system executable rather than assuming /usr/bin/env is one.
  • Run shared-state service/Codex suites and roster-retention in fresh bounded lanes. The latter stalled a long-lived macOS isolate worker before any test case but finished 25/25 in a fresh process.
  • Use the server-test budget for real replay and native combo recall cases whose internal wait may exceed Bun's 5s default. Bound adjacent WebSocket, shim and chat-identity checks, and wait for launcher config injection before asserting shutdown. Unexpected caller-main retry status now reports response and dispatch context.

Only test code and the test runner's lane list change. Agent-message recovery is already on dev.

Verification

  • Final head 866406bc8 (rebased onto 82cb66e82): Bun 1.4.0, Node 22, macOS bun run test: 31,848 pass, 60 skip, 0 fail, exit 0. Parallel lane: 31,216 pass / 37 skip; all fresh-process lanes passed. This result is from the final code after the review fixes.
  • bun run typecheck, bun run privacy:scan, git diff --check upstream/dev..HEAD: pass.
  • Focused: replay-state file 24/24, compaction-routing file 126/126, test-runner file 57 pass / 3 platform skips, service-claim 8/8, remote workspace command runner 13/13. Caller-main 429/402 cases passed 40/40 across 20 independent processes.
  • Earlier full runs exposed an intermittent caller-main 402 mismatch, two 5s integration-test timeouts with later same-file failures, and one 900s runner stall at roster-retention. The timeouts and isolation are addressed here. A stale orphan Bun worker from this temporary checkout was consuming a CPU core and was terminated before the final full run. The intermittent 402 was not root-caused; the new diagnostic captures its response and dispatch sequence if it recurs. No production route changed.
  • Cross-platform CI and React Doctor are action_required on this head and have not run to completion; a maintainer must approve the fork workflows. Keep this PR in draft and do not merge based only on local macOS evidence.

Review readiness checklist

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2d7e1ac4-db39-4f77-a99b-18c65de1166c

📥 Commits

Reviewing files that changed from the base of the PR and between bcdbfc6 and d9a37ce.

📒 Files selected for processing (3)
  • tests/clients/remote-workspace-command-runner.test.ts
  • tests/responses/responses-compaction-routing.test.ts
  • tests/service/service-claim.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Three test changes adjust a non-Windows executable fixture, increase a compaction-routing test timeout, and update service-state path assertions.

Changes

Remote workspace command runner test

Layer / File(s) Summary
Non-Windows executable fixture
tests/clients/remote-workspace-command-runner.test.ts
The fixture now returns /usr/bin/env on non-Windows platforms. Windows retains the existing fixture creation path. The comments describe the Unix test setup.

Compaction routing test

Layer / File(s) Summary
Opaque replay timeout
tests/responses/responses-compaction-routing.test.ts
The test now has a 20-second timeout instead of the default timeout.

Service claim test

Layer / File(s) Summary
Primary service state path assertions
tests/service/service-claim.test.ts
The test checks that the primary service state path starts with the temporary home and appears in the state-path list. It no longer requires every listed path to start with the temporary home.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other

Merge Risk: 🟡 Moderate · up to d9a37

The changes are test-only, but the required cross-platform run for this head has not succeeded, leaving platform-specific regressions unchecked. Complete the run before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the test-only changes that improve suite isolation and add an integration test timeout budget. It is concise and specific enough for the main change.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_coauthor_credit — This pull request says it reimplements, supersedes, carries, or rebases another author's pull request, but no Co-authored-by trailer names that author. Prose in a commit body is not read by anything; the trailer is what GitHub counts. Add it to the description or a commit, or obtain attribution-approved. Paths: #5743.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 25, 2026
@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

3/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 25, 2026
@lzfxxx
lzfxxx marked this pull request as ready for review September 25, 2026 12:38
@github-actions
github-actions Bot marked this pull request as draft September 25, 2026 12:39
@github-actions
github-actions Bot marked this pull request as ready for review September 25, 2026 12:41
@lzfxxx
lzfxxx force-pushed the fix/test-fixtures-isolated-dev branch from d9a37ce to 46418c9 Compare September 25, 2026 12:56
@github-actions
github-actions Bot marked this pull request as draft September 25, 2026 12:56
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 34 / 80

이 풀 리퀘스트는 테스트 세 곳만 고칩니다. 제품이 사용자 요청을 처리하는 방식은 그대로입니다. 테스트를 따로 돌릴 때 괜찮았는데도 실패로 보이던 자리를 막으려는 수정입니다. 기준 브랜치는 dev이고, dev보다 커밋 1개 뒤, 1개 앞입니다.

압축 결과를 다시 재생하는 테스트가 Bun의 기본 제한 5초를 넘겼습니다. 작성자가 본 시간은 5.37초입니다. 시간이 넘어가면 같은 파일의 다음 검사가 401처럼 잘못 보입니다. 그 테스트에만 20초를 줍니다.

서비스 소유권 테스트는 상태 파일 경로가 모두 이번 임시 폴더 안에 있는지 확인했습니다. 테스트 모음 실행기는 일부러 따로 둔 HOME을 씁니다. 예전 기본 집 경로는 그 임시 폴더 밖에 있어서 검사가 깨졌습니다. 이제는 지금 쓰는 경로가 임시 폴더로 시작하는지만 보고, 그 경로가 목록에 들어 있는지만 확인합니다.

원격 작업 공간 테스트는 bubblewrap을 실행하지 않습니다. 만들어질 명령 인자만 봅니다. Bun이 /tmp 아래에 있으면, 그 옆에 만든 가짜 실행 파일은 상위 폴더를 아무나 쓸 수 있어서 신뢰 검사에서 먼저 탈락합니다. 유닉스에서는 시스템 프로그램 /usr/bin/env를 대신 씁니다. 윈도우는 예전처럼 가짜 파일을 만듭니다.

라인 - tests/service/service-claim.test.ts 의 새 검사는 전보다 헐겁습니다. 이 테스트는 기록 함수를 가짜로 바꾸지 않아서, 실제 기록이 serviceStatePaths()에 있는 경로를 쓸 수 있습니다. 예전 검사는 그 목록이 전부 이번 임시 폴더 안인지 확인했습니다. 새 검사는 기본 경로 하나만 확인합니다. 둘째 줄은 목록에 기본 경로가 있는지만 보는데, 기본 경로는 원래 그 목록에 들어갑니다. 임시 폴더 밖 경로가 남아도 이 테스트는 통과합니다.

라인 - tests/clients/remote-workspace-command-runner.test.ts 의 /usr/bin/env는 같은 파일이 다른 이름으로 묶여 있으면 안 됩니다. 제품 함수 trustedBubblewrap은 링크 개수(nlink)가 1이 아니면 거절합니다. 이 리눅스 머신에서는 /usr/bin/env가 일반 파일이고 링크 개수가 1이라 통과합니다. env가 busybox 하나로 묶인 이미지에서는 인자 검사 전에 다시 실패합니다.

라인 - 같은 압축 테스트의 20초는 관측된 5.37초보다 넓습니다. 테스트가 정말 느려져도 20초가 되기 전에는 실패로 나오지 않습니다.

메인테이너의 판단이 필요한 지점

위생 봇 댓글은 #5743 작성자를 Co-authored-by로 적으라고 남아 있습니다. 최신 hygiene 체크는 통과입니다. 바뀐 파일은 테스트뿐이고, 본문은 에이전트 메시지 복구가 이미 dev에 있다고 합니다. 그 트레일러가 아직 필요한지, 댓글이 이전 실행의 결과인지를 정해야 합니다.

serviceStatePaths()가 모음 실행기의 HOME에 있는 예전 경로를 남기는 것이 맞다면, 그 경로가 개발자의 진짜 집(~/.opencodex)은 아니라는 확인을 이 테스트에 남길지 정해야 합니다. 지금 수정은 그 확인이 빠져 있습니다.

이 풀 리퀘스트는 드래프트입니다. 준비 체크리스트 4칸은 모두 비어 있습니다. 작성자도 이 커밋의 여러 운영체제 CI가 끝나기 전에는 합치지 말라고 적었습니다.

너의 추천

실패 원인과 테스트 수정은 서로 맞습니다. 서비스 클레임 테스트에는 "목록의 경로가 이번 임시 폴더 안이거나, 가드가 허용한 모음용 격리 집이다"라는 한 줄을 다시 넣는 쪽을 권합니다. /usr/bin/env는 리눅스 CI에서 링크 개수가 1인지 확인하고, 1이 아니면 상위 폴더를 아무나 쓸 수 없는 다른 시스템 파일을 고르면 됩니다. 20초는 같은 파일의 거짓 401을 막는 용도로 받아도 됩니다. 체크리스트를 채우고 CI가 통과한 뒤에 합치면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@lzfxxx
lzfxxx force-pushed the fix/test-fixtures-isolated-dev branch from 46418c9 to d77851e Compare September 26, 2026 03:40
@lzfxxx lzfxxx changed the title test: stabilize isolated suite fixtures and compaction replay test: stabilize full-suite isolation and integration budgets Sep 26, 2026

lzfxxx commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

@lidge-jun Thanks for the review. I tightened the service-state assertion to accept only the fixture path or the exact legacy path when the test-home guard says it is safe, and the Unix argv fixture now selects a single-link system executable rather than assuming /usr/bin/env qualifies. I also fixed the 5s native combo recall timeout that caused a follow-on same-file failure. On the current head 866406bc8, the macOS full suite exits 0 (31,848 pass / 60 skip), and typecheck and privacy scan pass. Cross-platform CI and React Doctor are still action_required for this fork head. Could a maintainer approve those workflows? I'll keep this draft until their results are available.

lidge-jun added a commit that referenced this pull request Sep 26, 2026
…et replacement, test stability) (#5918)

* fix(adapters): remint duplicate tool call ids on the openai-chat lane (#5914)

Carried from #5914 as one squashed commit.

Co-authored-by: moseoridev <sjssjs1344@gmail.com>

* fix(chat-native): refetch on zero-output mid-stream socket reset (#5882)

Carried from #5882 as one squashed commit.

Co-authored-by: Yum-wu <1172989563@qq.com>

* test: stabilize full-suite isolation and integration budgets (#5849)

Carried from #5849 as one squashed commit. The tests/service/service-claim.test.ts hunk is dropped: dev already sandboxes that case with a homedir spy and a stricter assertion.

Co-authored-by: Zhaofeng Li <lzfxxx@gmail.com>

* test(chat-native): prove the replacement request copy is released mid-relay

The #5882 regression streamed a 64 KiB delta against a 32 MiB turn budget, so it passed with or without the release. The new case rotates the key between sends, holds the replacement body after its first frame, and reads the live translator charge while the stream is relayed: 1x the request size with the release, 2x without it (verified red by reverting 318520e). Also drops a trailing blank line in src/lib/upstream-retry.ts and adds the batch plan.

* fix(upstream-retry): require the resend gate on the zero-output wrapper

wrapWithZeroOutputRefetch forwarded its options object, so the source guard that proves every post-header replacement is authorized could not see a gate at the new call. The wrapper now requires authorize in its type and passes it explicitly, and the guard scans wrapWithZeroOutputRefetch call sites as well. Fixes the red tests/lib/ambiguous-resend-composition.test.ts on test 1/4.

---------

Co-authored-by: moseoridev <sjssjs1344@gmail.com>
Co-authored-by: Yum-wu <1172989563@qq.com>
Co-authored-by: Zhaofeng Li <lzfxxx@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Thanks! This landed on dev through the bug-PR merge train batch #5918 (merge 76b26a0). Your change is one commit on dev with you as the author and a Co-authored-by trailer. One hunk was dropped: dev already sandboxes the service-claim refusal case with a homedir spy and a stricter assertion, so that file keeps the dev version. Closing since the content is now on dev.

@lidge-jun lidge-jun closed this Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants