fix(tests): put the real modules back after the image tests mock them

[FredAmartey]

Summary

• Five image test files replace modules with mock.module and never put them back. z-fulfill stubs src/images/artifacts and src/images/xai-client, download-cap-default stubs src/lib/pinned-http and node:dns/promises, loop and loop-reasoning-replay stub src/images/fulfill and src/web-search/progress-stream, and z-handler-activation stubs src/server/adapter-resolve, src/images/loop and src/web-search/index. Bun keeps a mock.module override for the rest of the process, and mock.restore(), which four of these files call in afterAll, does not reset it.
• Every file that runs after them in the same process gets the stubs. Some stubs lack exports (createVideoBudget, pinnedHttpPost, imageFulfillmentTailSnapshot, MAX_ENCODED_BYTES_PER_IMAGE), so later files fail on import. The others change behaviour: after z-handler-activation, every provider resolves to its fixture adapter. On dev, bun test tests/images gives 65 pass and 49 fail in one process, and loop is one of the files Batched test runner cannot run on macOS, and one-process runs give batch-size-dependent failure counts #5439 lists as failing only in batches.
• The damage reaches other directories. Run ahead of the 125 non-image test files that use the translator budget helper, in one process on dev, each file adds this many failures: z-handler-activation 241, download-cap-default 83, z-fulfill 53, loop-reasoning-replay 9 and loop 7.
• The z- prefix does not keep these files last. Bun runs a directory's files in the order the filesystem lists them, whatever order they are passed in, and on the macOS checkout this was measured on z-fulfill.test.ts runs first in tests/images. In a two-file check, Bun 1.3.11 and 1.4.0 also ran the same pair in opposite orders.
• Each file now copies the real modules before stubbing them and puts the copies back in afterAll. It has to be a copy: mock.module rewrites the namespace object's bindings in place, so the namespace would hand back the stub. No runtime code changes.
• artifacts-ssrf (node:dns/promises) and pinned-https-get (node:https) also leave mocks behind, but neither changes another file's result in any of the runs below, so they are left alone.

Verification

Head a54e23578 on dev 6d5d501a6, Bun 1.4.0 (the bun dependency in package.json). The measurements below were taken at dev 965a6e5b1. None of the 9 dev commits since then touches tests/images, src/images or the translator budget helper, and the two tests/images runs, the five files alone and the static checks were rerun on this head:

• Driven red first, across directories: each of the seven image files that mock a module, run ahead of the 125 non-image helper users in one process. On untouched dev five of them add the failures above; with this change all seven add 0.
• Inside tests/images: each mocking file run before each image file that comes after it, one pair per process. On untouched dev 12 of the 37 pairs change a result (z-fulfill breaks 7 files, download-cap-default breaks 5); with this change none does.
• bun test tests/images in one process: 65 pass, 49 fail on dev; 213 pass, 4 fail with this change. The 4 are a separate leak in the translator budget test helper, fixed in fix(tests): dispose test translator budgets in every file that creates them #5607; with both changes the directory gives 217 pass, 0 fail.
• bun test --isolate tests/images: 217 pass, 0 fail. The five changed files alone: 21, 2, 52, 1 and 7 pass.
• Full suite with CI's flags: every batch of shards 1/4 to 4/4 as scripts/ci/run-bun-test-batches.sh slices them (12 files per batch, bun test --isolate --timeout 60000, CI=true), run past failing batches because the script stops a shard at its first failure, with the four shards in parallel on one macOS machine: 28969 pass, 4 fail across 1506 files. No failing batch contains an image file. codex-runtime.test.ts and openai-provider-option-e2e.test.ts also fail in their batches on untouched dev at the same base; the second is one of the tests [Bug]: four tests exceed their timeouts in the unsharded macOS control lane, first observed once the lane was able to finish #4997 tracks. codex-shim.test.ts and macos-serial-lanes.test.ts failed only under the four-shard load: their batches pass on dev when rerun on their own.
• bun run typecheck, bun run structure:check, bun run privacy:scan and git diff --check: passed.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

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

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Tests
  • Improved test isolation by restoring mocked modules after image-related test suites complete.
  • Prevented test mocks from leaking into subsequent test files running in the same process.
  • Preserved existing test coverage and behavior while improving reliability across the full test suite.

[coderabbitai]

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: 66189964-b97b-4586-a99a-6c066bb7e3b1

📥 Commits

Reviewing files that changed from the base of the PR and between 6d5d501 and a54e235.

📒 Files selected for processing (5)

• tests/images/download-cap-default.test.ts
• tests/images/loop-reasoning-replay.test.ts
• tests/images/loop.test.ts
• tests/images/z-fulfill.test.ts
• tests/images/z-handler-activation.test.ts

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

---

📝 Walkthrough

Walkthrough

The tests now capture real module exports before applying mock.module overrides. Their afterAll hooks re-register those exports after mock.restore(), preventing mock overrides from affecting later test files in the same Bun process.

Changes

Test mock isolation

Layer / File(s)	Summary
Baseline module restoration tests/images/download-cap-default.test.ts:1-17, tests/images/loop-reasoning-replay.test.ts:21-54	The tests capture real module exports and re-register them during teardown after restoring mocks.
Image loop mock restoration tests/images/loop.test.ts:18-72	The test stores the real progress-stream and fulfillment exports before mocking and restores them in afterAll.
Fulfillment and handler mock restoration tests/images/z-fulfill.test.ts:13-51, tests/images/z-handler-activation.test.ts:48-141	The tests capture real client, artifact, resolver, image-loop, and web-search exports and restore them after mock cleanup.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a54e2

The changed test cleanup addresses the intended mock leakage, and no actionable merge-blocking risk remains.

🚥 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 1 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: restoring real modules after image tests apply Bun mock.module overrides.
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[lidge-jun]

리뷰 · 우선순위 68 / 80

이 PR은 이미지 테스트 다섯 파일이 mock.module로 진짜 모듈을 바꿔 놓고 끝내지 않아서, 같은 Bun 프로세스 안에서 뒤에 도는 파일이 가짜 모듈을 그대로 쓰게 되던 구멍을 막습니다. Bun의 mock.restore()는 함수 목만 되돌리고 mock.module 덮어쓰기는 안 지웁니다. 그래서 z-fulfill은 artifacts·xai-client, download-cap-default는 dns·pinned-http, loop와 loop-reasoning-replay는 fulfill·progress-stream, z-handler-activation은 adapter-resolve·loop·web-search를 남겼고, 뒤에 오는 파일이 import 단계에서 터지거나(없는 export), 동작이 바뀌었습니다(모든 provider가 fixture 어댑터). 작성자 측정으로는 bun test tests/images가 한 프로세스에서 65 pass / 49 fail이었고, 이 고침 뒤에는 213 pass / 4 fail입니다. 남은 4건은 번역 예산 헬퍼 누수라 #5607 쪽입니다. 고침 방식은 목하기 전에 진짜 모듈을 얕은 복사({ ...await import(...) })로 잡아 두고, afterAll에서 그 복사본을 다시 mock.module로 올리는 것입니다. 런타임 코드는 안 바꿉니다. base는 dev입니다. types/config 분할·프리뷰 배포와는 무관합니다. 지금 Draft이고 라벨은 bug입니다.

라인 - beforeAll 안에서 realXaiClient 등을 {}로 두고 나중에 채우는 네 파일 (loop, loop-reasoning-replay, z-fulfill, z-handler-activation) — beforeAll이 중간에 실패하면 afterAll이 빈 객체를 “진짜”로 다시 올릴 수 있습니다. download-cap-default처럼 파일 맨 위에서 먼저 복사해 두는 편이 더 안전합니다.

라인 - “진짜” 복사 시점 — mock.restore()는 mock.module을 안 지우므로, 앞 파일이 이미 같은 경로를 덮어쓴 뒤라면 await import(...)가 가짜를 줍니다. 그 가짜를 복사해 두면 afterAll이 가짜를 다시 심습니다. 이번에 고친 다섯 파일이 서로 되돌리면 tests/images 안 쌍 검사에서는 0건이라고 적혀 있어, 디렉터리 안에서는 맞물릴 가능성이 큽니다. 디렉터리 밖·다른 mock 파일과 섞이면 다시 약해질 수 있습니다.

라인 - artifacts-ssrf.test.ts(node:dns/promises), pinned-https-get.test.ts(node:https) — 작성자도 같은 종류의 누수를 남겨 두었고, 측정상 다른 파일 결과는 안 바꿨다고 합니다. download-cap-default가 dns를 되돌린 뒤에도 artifacts-ssrf가 나중에 돌면 dns 목이 다시 남을 수 있습니다. 지금은 “결과 변화 없음”에 기대는 선택입니다.

라인 - Draft / CI — 본문 checklist는 다 체크됐지만 PR은 아직 Draft입니다. CodeRabbit은 draft라 스킵했고, 호스트 hygiene·label·enforce-target 위주만 보입니다. 작성자 로컬 검증(쌍 검사, tests/images, 샤드 풀스위트, typecheck/structure/privacy)은 본문에 자세히 적혀 있습니다.

라인 - 형제 PR — #5607은 이 PR 뒤에도 남는 tests/images 4 fail(번역 예산)을 잡고, #5482는 다른 디렉터리의 adapter-resolve 캡처 패턴입니다. 파일 겹침은 없어 보이지만, “mock.module을 복사로 되돌리기” 계열이라 같이 보면 좋습니다.

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

artifacts-ssrf·pinned-https-get까지 이번 PR에서 같이 되돌릴지, 측정상 무해하니 후속으로 둘지. Draft를 Ready로 올린 뒤 호스트 테스트 샤드 초록을 머지 조건으로 둘지. #5607과 같이 머지할지, 이미지 mock만 먼저 넣을지.

너의 추천

원인·고침·검증이 한줄로 맞고, #5439류 “묶음으로만 실패”를 줄이는 데 바로 도움이 됩니다. Ready로 바꾼 뒤 호스트 CI(테스트 포함)만 확인하고 머지하는 쪽을 권합니다. beforeAll 실패 시 빈 객체 복원은 download-cap-default 패턴으로 맞추면 좋고, 남은 dns/https 누수는 필수는 아니니 후속 이슈로 남겨도 됩니다. #5607과 같이 넣으면 tests/images 한 프로세스 0 fail까지 갈 수 있습니다. 닫을 types/config 중복 PR은 없습니다.

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

[lidge-jun]

Carried onto dev in bundle PR #5672 (squash-merged as 7dd1db2), rebuilt on current dev as commit c52252b on the lane branch with a Co-authored-by trailer for you, so the credit stays on the merged commit. Closing this one as superseded. Thank you for the fix.