Skip to content

test: stabilize packed CLI fixture lifecycle - #209

Merged
mohanagy merged 3 commits into
developmentfrom
fix/205-fixture-suite-stability
Jul 24, 2026
Merged

test: stabilize packed CLI fixture lifecycle#209
mohanagy merged 3 commits into
developmentfrom
fix/205-fixture-suite-stability

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Fixes #205

What changed

  • Runs the packed CLI's broad happy-path workflow against the existing real local Streamable HTTP MCP fixture, using a non-blocking installed-binary runner.
  • Keeps the intentional packed stdio contracts for readiness, degraded discovery, and failed-init redaction/shutdown.
  • Preserves argument-array execution and shell: false; Windows timeout cleanup now uses an absolute System32\\taskkill.exe /T /F process-tree teardown.

TDD and validation

  • Added a focused non-blocking runner regression; it failed before the runner existed and now passes.
  • Added a focused Windows descendant-tree timeout regression; it failed before tree cleanup existed and now passes.
  • npm test: 114 passed files, 1 skipped; 1,320 passed tests, 23 skipped.
  • npm run test:coverage: same test counts; existing thresholds passed (95.4% statements/lines, 91.97% branches, 98.38% functions).
  • npm run test:core, npm run lint, npm run typecheck, npm run build, npm run smoke:cli, npm run check:pack, and npm run test:package all passed.
  • Final npm run test:package: 5/5 serial passes, with no residual fake fixture process.

No production timeout, test scope, coverage threshold, or process containment was weakened.

Summary by CodeRabbit

  • Tests
    • Expanded CLI contract coverage for remote upstream connections and request handling.
    • Added validation that long-running commands time out cleanly without blocking other test activity.
    • Improved verification of process termination, including cleanup of child processes on Windows.
    • Added checks for command output, exit status, startup failures, and cleanup errors.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e0366d91-741f-4f37-ab4f-7c2b36f47d86

📥 Commits

Reviewing files that changed from the base of the PR and between 320d357 and 30e0310.

📒 Files selected for processing (3)
  • src/audit/audit-journal.ts
  • tests/audit-integrity.test.ts
  • tests/package-contract.test.ts
📝 Walkthrough

Walkthrough

The package contract suite adds asynchronous installed-binary execution with output capture, timeout escalation, and Windows process-tree cleanup. Tests now cover pending binaries, doctor flows, failed initialization, and automation against a fake remote streamable HTTP upstream.

Changes

Package Contract Tests

Layer / File(s) Summary
Async installed-binary runner
tests/package-contract.test.ts
Adds asynchronous process execution with captured output, timeout escalation, structured results, and Windows taskkill process-tree termination.
Async contract flow coverage
tests/package-contract.test.ts
Updates responsiveness, Windows timeout, doctor, and failed upstream initialization cases to use the asynchronous runner.
Remote automation contract coverage
tests/package-contract.test.ts
Adds fake remote upstream setup, streamable HTTP configuration, asynchronous CLI execution, request/header assertions, and guaranteed cleanup.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • mohanagy/miftah#138: Both update package contract process spawning and output or termination handling.
  • mohanagy/miftah#183: Both modify installed-binary package contract process management.

Poem

A rabbit watched the binaries run,
With streams captured one by one.
When timeout thumped, TERM led the way,
Then KILL cleared the tree away.
Remote requests hopped through the night—
Contract tests now finish right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed, but it omits the required Security impact section and does not follow the checklist-style Validation template. Add a ## Security impact section and reformat Validation to match the repository's checklist template.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: stabilizing the packed CLI fixture lifecycle.
Linked Issues check ✅ Passed The PR meets #205 by adding focused regressions, a non-blocking runner, and Windows process-tree cleanup while preserving containment guarantees.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes are evident; the additions stay within the fixture-lifecycle stabilization and regression goals.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/205-fixture-suite-stability

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/package-contract.test.ts`:
- Around line 416-423: Update the synchronous runInstalledBinary implementation
to call installedBinaryInvocation(binary, args) and use its returned command and
args instead of duplicating the Windows-specific cmd.exe wrapping and
buildWindowsCommand logic. Preserve the existing execution behavior on both
Windows and non-Windows platforms.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 341d99a4-c883-48cb-8dcf-cc06cab3404f

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb349a and 320d357.

📒 Files selected for processing (1)
  • tests/package-contract.test.ts

Comment thread tests/package-contract.test.ts
@mohanagy
mohanagy merged commit f38d068 into development Jul 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: make full and coverage suites reliable under fixture startup pressure

1 participant