test(clients): bound transaction fixture child completion

[luvs01]

Summary

Closes #4003.

The client transaction fixture can keep a worker blocked after its child prints valid JSON because its synchronous spawn has no deadline. Bound that child with the existing 15-second internal budget and SIGKILL; reject spawn errors, nonzero exits and signals before parsing output. Clean both temporary homes on process or parse failure, preserving the original failure if cleanup also fails.

Only tests/clients/client-connect.test.ts changes. Existing transaction, rollback and stderr assertions remain. This is a direct-child fixture bound, not a claim that this helper caused the earlier macOS CI cancellation or that all descendant-pipe lifecycle problems are resolved.

Verification

Head 9809dc4d62ab78626674f05a2a428ec303ed43f3, based on dev 402be7c1f88283eb8465c3aec8437ccecd2542ec.

• Bun 1.4.0 on Windows: the new retained-child regression failed before the fix because the unbounded helper returned normally after 5.09 seconds.
• Three focused regressions pass with 22 assertions: valid JSON followed by a held child, nonzero exit, and invalid JSON. The timeout case checks the natural-exit marker remains absent, the exact PID is gone, and both homes are removed.
• The complete affected file passed: 49 tests / 257 assertions in 233.94 seconds. A subsequent type annotation correction and cleanup of unexpected-success returns in the new regressions were followed by a passing rerun of those three cases; they do not change the existing transaction path.
• bun run typecheck, bun run privacy:scan, and git diff --check pass.
• Full contributor CI passed all 26 jobs on this exact head, attempt 2. The first attempt had 24 successful jobs; macOS 1/2 exceeded its 20-minute limit and the aggregate failed. All three new regressions and all seven existing transaction tests had passed in that original macOS job, including the timeout regression in 2,006.78ms. Progress later stopped after a held-lock test in another file. After the workflow was terminal, one retry of the failed macOS job and aggregate passed. The initial incomplete run remains recorded; no source or timeout changes were made to obtain the successful retry.
• CodeRabbit completed its review of this head without actionable inline findings. Its summary's possible duplicate-declaration claim was disproved by lexical scope and actual module execution; CodeRabbit explicitly withdrew it. Docstring coverage is advisory. No user-facing runtime or documentation behavior changes.

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

• All CI tests are green on my local testing.
• 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

• Bug Fixes
  • Improved cleanup after client process timeouts, failures, and invalid output.
  • Ensured retained child processes are terminated and temporary homes are removed.
  • Consolidated cleanup failures so multiple issues are reported together.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 38e49694-d3b2-4705-a5f7-0f6bd7fac997

📥 Commits

Reviewing files that changed from the base of the PR and between 402be7c and 9809dc4.

📒 Files selected for processing (1)

• tests/clients/client-connect.test.ts

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

---

📝 Walkthrough

Walkthrough

The transaction test fixture now bounds child execution, reports execution and output failures, aggregates cleanup errors, and verifies child termination plus temporary-home removal.

Changes

Transaction fixture safety

Layer / File(s)	Summary
Bounded execution and cleanup tests/clients/client-connect.test.ts	runTransactionScenario accepts custom scripts and timeouts. Child processes use a SIGKILL timeout. Execution and JSON errors become ClientStateProbeError instances. Both temporary homes are removed on success and failure, with cleanup failures combined in an AggregateError.
Timeout and failure-path tests tests/clients/client-connect.test.ts	Tests verify that retained children are terminated, child PIDs become unavailable, both temporary homes are removed, and nonzero or invalid-JSON children expose the expected error fields.

Priority: ⬇️ Low — Impact reflects low issue severity.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: Low

Merge Risk: 🟡 Moderate · up to 9809d

This test-only change improves timeout and cleanup behavior, but the added regression tests may contain duplicate declarations that prevent the test module from compiling. Confirm or fix that issue before merging.

Suggested reviewers: invalid-email-address

🚥 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 1 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	The changes address the requirements in issue [#4003]. The fixture now applies the internal child deadline and SIGKILL behavior, rejects spawn errors, nonzero exits, and signals before parsing output,…
Out of Scope Changes check	✅ Passed	The changes are limited to tests/clients/client-connect.test.ts and support the linked fixture-safety objectives in issue [#4003]. No production code, documentation, configuration, or unrelated behavi…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: bounding child-process completion in the client transaction test fixture.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• ✅ All CI tests are green on my local testing.
• ✅ 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 has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

Hygiene

✅ Deterministic PR hygiene checks passed.

[luvs01]

@coderabbitai full review

Please review head 9809dc4d62ab78626674f05a2a428ec303ed43f3. This is limited to the transaction fixture's direct-child deadline, failure cleanup, and three regressions. Existing transaction checks remain; the patch does not claim to diagnose the earlier macOS CI cancellation. Focused checks and type/privacy checks pass; full contributor CI is running.

[coderabbitai]

@luvs01 I will perform a full review of PR #4004 at head 9809dc4d62ab78626674f05a2a428ec303ed43f3. I will focus on the direct-child deadline, failure cleanup, and the three added regressions.

✅ Action performed

Full review finished.

[luvs01]

@coderabbitai The merge-risk summary's possible duplicate-declaration compilation issue is not present on reviewed head 9809dc4d62ab78626674f05a2a428ec303ed43f3.

proofHome, markerPath, and run at lines 434–445 belong to the timeout test callback starting at line 433. The declarations at lines 475–485 belong to the separate callback starting at line 474, inside the two-case loop. They do not share a lexical declaration scope.

The final committed test module was loaded and executed by pinned Bun 1.4.0 for all three new cases: 3 passed / 22 assertions. Loading it would fail before any case ran if there were duplicate lexical declarations. The full affected file had also passed 49 tests / 257 assertions before the subsequent type-only annotation and failure-cleanup refinement described in the PR. bun run typecheck passes too, although the direct Bun test-module execution is the relevant compilation evidence here.

Please correct that summary claim if it refers to these declarations; an exact line pair would be needed to assess a different concern. The docstring-coverage warning is advisory and does not identify a behavior defect. No source change or redundant CI restart was made for either summary item.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- checked-out commit ---'
git rev-parse HEAD
printf '%s\n' '--- relevant test structure ---'
ast-grep outline tests/clients/client-connect.test.ts --items all --type function --view expanded || true
printf '%s\n' '--- lines 420-500 ---'
nl -ba tests/clients/client-connect.test.ts | sed -n '420,500p'

Length of output: 1245

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '425,490p' tests/clients/client-connect.test.ts | awk 'BEGIN { line = 425 } { printf "%d\t%s\n", line++, $0 }'

Length of output: 3685

---

@luvs01 Correct. I verified commit 9809dc4d62ab78626674f05a2a428ec303ed43f3.

In tests/clients/client-connect.test.ts, Lines 433–468 are the timeout test callback. Its declarations are at Lines 434–445. Lines 470–490 start the two-case loop and a separate test callback at Line 474. Its declarations are at Lines 475–485.

These declarations do not share a lexical scope. The duplicate-declaration compilation-risk claim is invalid and should not appear in the merge-risk summary. I do not have a different conflicting declaration pair to identify.

The docstring-coverage item is advisory. It does not identify a behavior defect in this PR.

🐇

You are interacting with an AI system.

[lidge-jun]

리뷰 · 우선순위 62 / 80

이 PR은 사용자 기능이 아니라 테스트 도우미를 고칩니다. 지금 dev HEAD 402be7c1f의 tests/clients/client-connect.test.ts 안에 있는 runTransactionScenario는 Bun/Node 자식을 spawnSync로 띄우는데, 타임아웃이 없습니다. 그래서 자식이 올바른 JSON을 이미 stdout에 찍은 뒤에도 프로세스가 살아 있으면, 테스트 워커가 그 동기 호출에서 끝없이 기다릴 수 있습니다. 정리(임시 OPENCODEX_HOME / CODEX_HOME 삭제)도 호출이 끝난 뒤에야 돌아가서, 실패한 자식이 남긴 홈이 남을 수 있습니다. 이슈 #4003이 바로 그 구멍이고, 이 PR은 그 구멍을 같은 파일 안의 기존 패턴에 맞춰 막습니다.

같은 파일의 readStateProbe와 파일 하단 다른 spawnSync 경로는 이미 INTERNAL_DEADLINE_MS(15초)와 SIGKILL을 씁니다. tests/helpers/test-budget.ts도 “스폰이 증명의 일부일 때만 예산을 올린다”고 적혀 있습니다. 이번 변경은 그 규칙을 트랜잭션 픽스처에도 그대로 가져옵니다. 기본 예산은 15초, 회귀 테스트는 2초로 짧게 잡고, 유효 JSON 뒤에 5초짜리 타이머를 남긴 자식을 죽여서 타임아웃·PID 부재·양쪽 홈 삭제를 확인합니다. nonzero exit와 잘못된 JSON도 파싱 전에 거절하고 홈을 지웁니다. 런타임/문서 변경은 없고 범위는 이 테스트 파일 하나입니다.

다만 PR이 아직 draft이고, 체크리스트에도 전체 CI·리뷰 준비가 덜 끝났다고 적혀 있습니다. 게이트 잡(hygiene/label/enforce-target)은 통과했지만, 전체 스위트가 초록인지가 이 머지의 마지막 문입니다. 작성자가 Windows에서 회귀 3개와 파일 전체(49 tests)를 돌렸다고 적은 점은 신뢰에 도움이 됩니다. “이 픽스처가 예전 macOS CI 취소의 원인이다”라고 주장하지 않은 것도 좋습니다. 원인 단정 없이 픽스처 안전만 고칩니다.

라인 근처(diff 기준) - await runTransactionScenario(...)인데 헬퍼는 동기 함수다. 동작은 같지만 불필요한 await/async라서 나중에 읽는 사람이 비동기로 오해할 수 있다.
라인 근처 - 모든 기존 트랜잭션 시나리오에 기본 timeout: INTERNAL_DEADLINE_MS(15초)가 새로 붙는다. 의도된 안전장치지만, 느린 CI에서 정상 시나리오가 15초를 넘기면 예전엔 통과하던 케이스가 타임아웃으로 붉어질 수 있다.
경로 tests/clients/client-connect.test.ts의 다른 spawnSync(대략 연결/키 회전 헬퍼) - 이번 PR 범위 밖이라 그대로 두었고, 같은 파일에 비슷한 무한 대기 구멍이 더 있을 수 있다. 이번 PR이 그걸까지 건드릴 필요는 없다.
ClientStateProbeError의 pid: number - spawnSync의 result.pid는 플랫폼에 따라 비어 있을 수 있는데, 기존 하단 픽스처와 같은 타입 구멍을 그대로 쓴다. 새 회귀는 PID 마커로 보완하므로 실사용 위험은 작다.

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

• draft를 Ready로 올리고 전체 CI(특히 Windows/macOS 클라이언트 테스트)가 초록일 때만 머지할지
• 기본 15초 예산이 기존 트랜잭션 시나리오에 충분한지, 아니면 트랜잭션 전용 예산을 따로 둘지
• #4003을 이 PR 머지와 함께 닫을지(본문에 Closes Client transaction test fixture has no child timeout or failure cleanup #4003 있음)

너의 추천
CI 전체(최소 이 파일·Windows)가 초록이면 draft 해제 후 dev에 머지하고 #4003을 닫는다. 지금은 Ready 전이라 머지 보류. 다른 spawnSync 구멍은 후속 chore로 남겨도 된다.

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

[Ingwannu]

Reviewed exact head 9809dc4 against 402be7c. Approving this scoped test-only change.

The helper now rejects a timed-out/signalled/nonzero child before interpreting its JSON. Both temporary homes receive cleanup attempts, and cleanup failure preserves the original error. The new retained-child regression checks the typed timeout, exact child PID disappearance and both homes removed, rather than merely checking valid output. The existing success, catalog restoration and rollback assertions remain intact. The module-scope duplicate-declaration allegation was correctly withdrawn: these declarations belong to different callbacks.

I independently verified contributor CI run 34189808467, attempt 2: all 26 jobs succeeded at this exact head, including Windows/macOS/Linux. This updates the older draft/full-CI-pending assessment: the PR is now non-Draft. The original macOS attempt timed out later in a separate held-lock test; the successful unchanged-head retry is not proof that unrelated timeout is fixed. These are contributor CI results, not an upstream full-CI run.

Current dev 74f62f9 does not change this file, test-budget.ts or remove-tree.ts relative to the reviewed base. No product/runtime/workflow/default changes are included. Local contributor execution was not performed because a credential-free, network-isolated sandbox is unavailable here.

Risk assessment: low impact and likelihood, strong relevant regression protection, easy revert, moderate confidence; human-controlled merge only. This approval does not merge the PR or waive repository checks/Go-line tracking obligations.

[Ingwannu]

Correction to the last paragraph of my approval: current dev MAINTAINERS.md has retired the dev2-go line and its carry obligation. My reference to Go-line tracking was based on the stale local checkout policy and should be disregarded. There is no Go-port requirement for this PR. The exact-head test review and CI evidence are unchanged; no merge was performed.

[lidge-jun]

Landed on dev in cfff026 via #4049, which carried all nine PRs of this train as one sequentially reviewed integration branch. Your commit is preserved in the squash body with a Co-authored-by trailer, so the contribution stays attributed to you.

Verification at the exact merged head 8c58bac0863f1f2ca2b430e1177a0e417b474c85: Cross-platform CI run 34256853348 finished with 19 jobs successful and 0 failures, bun x tsc --noEmit exit 0, and 312 focused tests passing across the ten affected files. An independent review confirmed the stacked tree is byte-identical to the union of the nine PR diffs.

Closing this PR because the change is already on dev. Thank you.

[lidge-jun]

Landed via #4049 at cfff026