Skip to content

test: bound shell integration scenarios independently - #7449

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
oliver-mee:fix/test-reliability
Oct 21, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
oliver-mee:fix/test-reliability

Conversation

@oliver-mee

@oliver-mee oliver-mee commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #7448.

  • Give each release-gates Bash scenario an independent test case instead of sharing one 30-second matrix deadline. Each real Bash child retains its five-second deadline and executes the checked-in workflow script.
  • Run SSH discovery fixtures through the existing bounded asynchronous child helper. Split absent/broken candidates and Windows-like platforms into separate cases. Use the shared 45-second process-test budget around a 15-second child deadline, with explicit exit, pipe and signal checks before fixture cleanup.
  • Preserve all nine candidate priorities, NUL-separated argument fidelity, exit statuses and CI gate-policy assertions. No production code, workflows, dependencies, runtime configuration or global test budgets change.

The other three baseline failures already have fixes in #7447. I tested that PR's head ab3d4bd30d37ef0dab03efc71e903a0d8f8e07bf independently: 88 passed, 1 platform skip, 0 failures across the three affected files, and posted the result there. Those changes are not duplicated here.

Verification

Base: 12025981e5a304cdb6b1b54f525ba8554f623977. macOS 26.6.2, Bun 1.4.3, repository-isolated test homes and serial test execution.

  • Before: the two affected files produced 18 passed, 1 failed. The combined SSH missing-candidate case timed out at 5,024 ms and returned null rather than exit 127. The late winning-candidate case took 4,955 ms. The CI matrix passed in this fresh run; its prior 30-second failure on the same base was load-sensitive.
  • The initial fixed run passed 78 tests. The all-broken SSH scenario took 6,455 ms, longer than its former implicit test deadline.
  • Final bun run test:changed --parallel=1: 78 passed, 0 failed across the two affected files.
  • Explicit adjacent/source-as-data coverage: 114 passed, 1 Windows-only skip, 0 failed across ci-scope-reduction, cold-spawn-warmup, link-ssh-argv, file-size-ratchet, repo-hygiene, and both test-layout guards.
  • Controlled ablation: dropping forwarded SSH arguments caused 10 assertion failures; changing the extracted Bash gate's exit 1 branches to exit 0 caused 52 assertion failures. Both mutations were confined to test execution and removed afterward. The workflow itself was not edited.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, and git diff --cached --check: passed.

Full-suite scope exception: this is a two-file, test-only change. The full suite takes roughly 30 minutes on this shared test host and already has documented persistent base failures. It was not repeated for this patch; affected tests, subprocess/argv safety checks and source-as-data repository guards are run explicitly. This is not a claim that the unfiltered suite is green. Hosted CI still needs to run on the PR head before merge.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
    • No user-facing behavior or configuration changes; no documentation update needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
    • Production SSH argument handling and workflow policy are unchanged; the existing negative assertions remain exercised.

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 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.

Summary by CodeRabbit

  • Tests
    • Improved test coverage for release-gate execution and SSH candidate discovery.
    • Added checks for process completion, timeout handling, pipe closure, signal behavior, and cleanup.
    • Expanded coverage for successful, skipped, failed, absent, and broken candidate scenarios.

🤖 Generated with Claude Opus 5.5
🧑‍💻 Ideated, directed and reviewed by a human, @oliver-mee

Split CI aggregate matrix cases and SSH discovery scenarios into bounded asynchronous child invocations. Preserve gate-policy and argv assertions while allowing real candidate probes to finish and validating process cleanup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: 7a9cb603-b2f1-47ab-a139-685d810d9660
📥 Commits

Reviewing files that changed from the base of the PR and between 1202598 and 7c2302e.

📒 Files selected for processing: 2
  • tests/ci-workflows/ci-review-lanes.test.ts
  • tests/clients/link-ssh-argv-discovery.test.ts

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

📝 Walkthrough

The SSH discovery and release-gates tests now execute subprocess scenarios asynchronously with individual time budgets. They assert child-process cleanup and preserve their existing candidate-selection and gate-policy checks.

Changes: Subprocess test budgets

Layer / File(s) Summary
SSH discovery subprocess scenarios
tests/clients/link-ssh-argv-discovery.test.ts
SSH candidate probes now use spawnTestChild with a 15-second deadline and SPAWN_BUDGET_MS test budgets. The tests check that children are reaped, pipes close, and processes neither time out nor receive a signal. Fixture roots remain available while child processes or inherited pipes are active. Candidate selection, argument escaping, platform handling, and expected exit statuses remain covered.
Release-gates matrix scenarios
tests/ci-workflows/ci-review-lanes.test.ts
The aggregate contract is nested under a skipped describe block. Matrix invocations now use spawnTestChild with individual 5-second deadlines and SPAWN_BUDGET_MS test budgets. Assertions check child cleanup and retain coverage for success, skips, producer failures, and invalid selector statuses.

Priority: ⬇️ Low

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

Change: Other · Severity of issue fixed: Low

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to 7c230

The changed shell scenarios retain their individual time bounds, and no merge-blocking issue was identified. The PR is ready for normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed 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 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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: independently bounding shell integration test scenarios. It matches both affected test files and the stated pull request objective.
Linked Issues check Passed Issue #7448 requires independent release-gates scenarios and bounded SSH discovery probes. tests/ci-workflows/ci-review-lanes.test.ts replaces the aggregate spawnSync loop and 30,000 ms test with …
Out of Scope Changes check Passed The whole-PR diff contains changes only in tests/ci-workflows/ci-review-lanes.test.ts and tests/clients/link-ssh-argv-discovery.test.ts. The added asynchronous child handling, lifecycle assertions…
Full details: Linked Issues check

Explanation

Issue #7448 requires independent release-gates scenarios and bounded SSH discovery probes. tests/ci-workflows/ci-review-lanes.test.ts replaces the aggregate spawnSync loop and 30,000 ms test with independent test.each cases. Each Bash child uses spawnTestChild with a 5,000 ms deadline and checks timeout, reaping, pipe closure, signal, exit code, and gate output. tests/clients/link-ssh-argv-discovery.test.ts uses spawnTestChild for each fixture run, sets a 15,000 ms child deadline and the 45,000 ms test budget, and separates absent, broken, and Windows-like cases. The diff preserves all nine candidate-order cases, NUL-separated arguments, exit codes 127 and 125, and gate-policy assertions. The whole-PR diff contains only the two requested test-file changes and does not change production code, workflows, dependencies, runtime configuration, or global budgets.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff contains changes only in tests/ci-workflows/ci-review-lanes.test.ts and tests/clients/link-ssh-argv-discovery.test.ts. The added asynchronous child handling, lifecycle assertions, fixture retention, case splitting, and test budgets directly support issue #7448. No unrelated production, workflow, dependency, configuration, or global test-budget changes are present.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · 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.

@github-actions

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 20, 2026
@oliver-mee

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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 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.

✅ 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

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@oliver-mee
oliver-mee marked this pull request as ready for review October 20, 2026 16:21
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 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-20T16:26:58.508875Z 7c2302e Draft marked ready
ℹ️ 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.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 7c2302ec5b9ddd7711add2f94ee2d28c116c15a7 against current dev 12025981e5a3. The two-file test-only change preserves real Bash policy/exit assertions, SSH candidate order and exact forwarded argv while using the existing bounded child-process helper and independent scenario deadlines. Independent isolated Linux/Bun 1.4.3 execution passed 78 tests, 0 failures across both changed files. The actual hosted Cross-platform CI and React Doctor runs now completed successfully on this head, including all four selected test shards and gates; unselected OS jobs are not claimed as exercised. No outstanding review threads or maintainer change request was found. Approved; this is not a merge or release.

@lidge-jun
lidge-jun merged commit 46a9e97 into lidge-jun:dev Oct 21, 2026
39 checks passed
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). review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants