Skip to content

test: drain Windows ACL work and startup child output - #6495

Merged
lidge-jun merged 2 commits into
devfrom
codex/train-windows-test-lifecycle
Oct 3, 2026
Merged

lidge-jun merged 2 commits into
devfrom
codex/train-windows-test-lifecycle

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Summary

Make Windows test lifecycle reliable: drain pending Nous ACL hardening before removing owned temporary homes, and make startup failures diagnosable by draining both child output pipes immediately. Final Windows7 timed out waiting for a runtime record without retaining child output; the fixture now keeps bounded stdout/stderr tails, child state and a sticky startup marker.

The Nous fixed-directory cleanup raced asynchronous hardening; each test now uses the canonical owned temp home and awaits its hardening/reap drain before removal. Existing credential security assertions and production permissions stay unchanged.

This is a test-only diagnostic/lifecycle repair. The original hosted timeout was not reproduced or root-caused, and this PR does not claim otherwise. Existing 15-second deadlines and shared-client ownership assertions are unchanged; no retries or skips were added.

Verification

  • Nous final full file:57 passed locally and57 passed on Windows; removing the awaited ACL drain deterministically fails on both. The original timing race was not reproduced by the isolated natural baseline, so the cause is grounded in source/lifecycle plus deterministic scheduling proof. Independent review PASS.
  • Five new diagnostic tests passed locally, covering bounded capture, split decoding/marker retention, finish/cancellation and failure contents. A diagnostic-omission mutation fails the message assertion.
  • Matching Windows Bun1.4.0 full file:32 passed,1 existing POSIX-only skip,0 failures; all original ownership cases passed.
  • Local full file:31 passed,2 failures. The same two ownership failures reproduce on unchanged frozen dev (26 passed,2 failures); these are reported as unresolved local failures, not passing evidence. Native/hosted environment differences remain explicit.
  • A 16MiB undrained-output counterprobe passed on the original local fixture, so pipe pressure is not asserted as the hosted cause. The retained test verifies useful capture rather than claiming a reproduced deadlock.
  • Independent review PASS after fixing a final-tail assertion race. Root typecheck, file-size, privacy and diff checks passed. No production changes, builds, full repository suites or dependency installs.
  • Required PR CI and the subsequent complete dev cross-platform run remain gates. No production-readiness claim.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed (test-only behavior, no product contract change).
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Maintainer integration

Owner lidge-jun elects integration into dev at 0d99a2313469a826ffe15bb749f55cf121a51b36. Exact-head pull_request CI 37109537371, attempt1, passed all requested jobs. Independent review and exact native fixture proof/limits are recorded above. The original Windows7 timeout cause remains unconfirmed; this is bounded diagnostic improvement, not a claimed production repair. Final full dev dispatch will validate the combined candidate. This is maintainer integration, not self-approval.

Collect stdout and stderr immediately, retain bounded tails and a sticky startup marker, and include child state/timing in readiness failures. Keep the 15-second deadlines and all shared-client ownership assertions unchanged. Cover high-volume streams, diagnostic failure output, split decoding and bounded cancellation; the hosted timeout root cause remains unconfirmed.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 3, 2026 08:18
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-03T08:21:03.947964Z f9a0559 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

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

coderabbitai Bot commented Oct 3, 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: 160638cd-a24b-4f98-83fa-60755b13173e
📥 Commits

Reviewing files that changed from the base of the PR and between b254139 and 0d99a23.

📒 Files selected for processing (3)
  • tests/cli/sibling-home-client-sync.test.ts
  • tests/helpers/startup-child-output.ts
  • tests/providers/nous-oauth.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

CLI tests now capture bounded stdout and stderr from spawned children and use the capture for startup checks, failure diagnostics, and cleanup. OAuth tests add per-test temporary-home cleanup that waits for directory hardening, and remove suite-specific home-directory setup from several test suites.

Changes

Child output capture

Layer / File(s) Summary
Bounded child-output capture
tests/helpers/startup-child-output.ts
The helper reads stdout and stderr concurrently, keeps the last 8192 characters from each stream, detects the startup marker across chunks, and reports stream and timing state. finish() waits for both readers or its timeout, then stops unfinished reads.
CLI test integration
tests/cli/sibling-home-client-sync.test.ts
Spawned children are registered with the capture helper. Startup and runtime-publication failures include captured diagnostics. New tests cover large concurrent output, split UTF-8, marker detection, and bounded handling of pending or rejected cancellation.

OAuth test home cleanup

Layer / File(s) Summary
Temporary-home lifecycle and suite setup
tests/providers/nous-oauth.test.ts
Tests add per-test temporary-home setup and asynchronous cleanup that waits for directory-hardening work. A Windows-specific test checks cleanup while hardening is blocked. Several suites remove their suite-specific OPENCODEX_HOME setup and teardown.

Priority: ⬇️ Low

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

Change: Other

Merge Risk: ⚪ Minimal · up to 0d99a

This is a test-only change with no production impact. At worst, one rare startup-failure case in the CLI tests fails more slowly than before. The change is mergeable.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 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 clearly summarizes both main test changes: draining Windows ACL work and capturing startup child output.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@lidge-jun lidge-jun changed the title test(cli): preserve bounded startup diagnostics on Windows test: drain Windows ACL work and startup child output Oct 3, 2026
@lidge-jun
lidge-jun merged commit 7c59baf into dev Oct 3, 2026
38 of 40 checks passed
@lidge-jun
lidge-jun deleted the codex/train-windows-test-lifecycle branch October 3, 2026 08:33
This was referenced Oct 3, 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.

1 participant