ci: add cross-platform validation to ci.yaml by kartikloops · Pull Request #89 · luarss/openroad-mcp

kartikloops · 2026-03-27T07:15:52Z

Summary

Folds cross-platform OS matrix (Ubuntu 22.04, 24.04, macOS ARM64) into the existing test job as requested
Adds a host-pty test type on macos-14 to run PTY integration tests natively
Adds drain_output() to PTYHandler to handle macOS's async PTY buffer flushing, fixing 5 flaky tests
Verifies Docker image builds with --platform linux/amd64 on macOS

Fixes #90

coderabbitai · 2026-03-27T07:16:20Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Added a new async method PTYHandler.drain_output() to collect remaining PTY output with timeout/idle-time handling; updated integration tests to use drain_output() after process exit; and expanded CI .github/workflows/ci.yaml matrix to run on multiple OS runners and added macOS-specific host-pty and docker-build variants with strategy.fail-fast: false.

Changes

Cohort / File(s)	Summary
CI workflow `.github/workflows/ci.yaml`	Changed `test` job to `runs-on: ${{ matrix.os }}` and set `strategy.fail-fast: false`; extended matrix `include` to add `core` runs on `ubuntu-22.04`, `ubuntu-24.04`, and `macos-14`; added `macos-14` `host-pty` (runs `uv run pytest tests/integration`) and `docker-build` (builds `linux/amd64` test image) variants; conditional steps remain matrix-driven.
PTY handler `src/openroad_mcp/interactive/pty_handler.py`	Added `async def drain_output(self, timeout: float = 2.0, idle_timeout: float = 0.1) -> bytes` which loops calling `read_output()` and accumulates chunks until total timeout or idle period elapses; uses event-loop time and brief sleeps to handle late-arriving output (macOS).
Integration tests `tests/integration/test_pty_integration.py`	Replaced post-exit single/chunked `read_output()` usage with `await pty_handler.drain_output(...)` and decode/assert on the aggregated bytes; simplified large-output collection and ensured consistent post-exit draining across tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇
I hopped through buffers, quiet and spry,
Drained the last whispers before they fly.
Tests now sip output, whole and neat,
CI runs on many paws and feet. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding cross-platform validation to the CI workflow by extending the matrix to include Ubuntu 22.04, Ubuntu 24.04, and macOS runners.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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

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

Copilot

Pull request overview

Adds a new cross-platform CI job to validate that the project’s core test suite runs cleanly across multiple GitHub-hosted runner OS images and to sanity-check Docker image buildability.

Changes:

Introduce a cross-platform matrix job running on ubuntu-22.04, ubuntu-24.04, and macos-latest.
Run make sync + make test on each OS.
Add a macOS-gated docker build --platform linux/amd64 verification step.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yaml:
- Around line 65-67: The GitHub Actions job "Verify Docker image builds
(linux/amd64)" is running on macos-latest which lacks a Docker daemon and cannot
use docker/setup-qemu-action@v3 or docker/setup-buildx-action@v3; change the job
to run on an Ubuntu runner (runs-on: ubuntu-latest) or gate it so the Docker
build step only executes on ubuntu hosts (e.g., update the job or its if
condition referencing matrix.os) so QEMU/buildx setup actions and the docker
build --platform linux/amd64 step execute on a Linux runner; if you still need
macOS ARM builds, create a separate job for macos-latest that skips
multi-platform Docker steps.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: e3f7490e-807f-4088-9c6c-aec5cb45fe3e

📥 Commits

Reviewing files that changed from the base of the PR and between 95c310e and ffc29f7.

📒 Files selected for processing (1)

.github/workflows/ci.yaml

coderabbitai

🧹 Nitpick comments (1)

src/openroad_mcp/interactive/pty_handler.py (1)

200-224: Use bytearray in drain_output() to avoid repeated bytes reallocations.

collected += chunk in a loop can become expensive for large PTY output. bytearray.extend() keeps this linear and is a drop-in behavioral equivalent here.

♻️ Proposed refactor

-        collected = b""
+        collected = bytearray()
@@
             chunk = await self.read_output()
             if chunk:
-                collected += chunk
+                collected.extend(chunk)
                 last_data_at = loop.time()
@@
-        return collected
+        return bytes(collected)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openroad_mcp/interactive/pty_handler.py` around lines 200 - 224, The loop
in drain_output currently accumulates bytes with repeated concatenation
(collected += chunk), which reallocates; change collected to a bytearray (e.g.,
collected = bytearray()), call collected.extend(chunk) when chunk is truthy, and
return bytes(collected) at the end to preserve the function's bytes return type;
update references in async def drain_output to use extend instead of += and
ensure chunk is handled as bytes before extending.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/openroad_mcp/interactive/pty_handler.py`:
- Around line 200-224: The loop in drain_output currently accumulates bytes with
repeated concatenation (collected += chunk), which reallocates; change collected
to a bytearray (e.g., collected = bytearray()), call collected.extend(chunk)
when chunk is truthy, and return bytes(collected) at the end to preserve the
function's bytes return type; update references in async def drain_output to use
extend instead of += and ensure chunk is handled as bytes before extending.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e05499b7-210b-4b1c-b879-2829679032e6

📥 Commits

Reviewing files that changed from the base of the PR and between 3bece66 and 4027e50.

📒 Files selected for processing (2)

src/openroad_mcp/interactive/pty_handler.py
tests/integration/test_pty_integration.py

kartikloops · 2026-03-27T23:14:01Z

@luarss made the changes you requested, folded the matrix into the existing test job. Also fixed #90 in this PR since the macOS runner was already being added.

ArmaanBawa · 2026-03-28T02:11:11Z

@luarss @kartikloops
I ran into the same Docker-on-macOS issue in my PR. The problem is that macOS GitHub Actions runners don't have a Docker daemon, so the docker build step fails.

Two options to fix it:

Add if: runner.os == 'Linux' to the Docker build step so it only runs on Ubuntu runners
Move the Docker build step to a separate job with runs-on: ubuntu-latest
Also for the drain_output() fix CodeRabbit suggested — replace collected = b"" with collected = bytearray(), use collected.extend(chunk) in the loop, and return bytes(collected) at the end. Avoids repeated memory reallocation for large PTY output."

luarss · 2026-03-28T03:30:29Z

How about an conditional pre setup Docker step for macos?

…g pytest-timeout flag

kartikloops · 2026-03-28T03:41:44Z

@luarss Fixed two CI failures on the macos-14 jobs:

docker-build: replaced manual brew install colima docker + colima start with docker/setup-docker-action@v4, which handles socket linking and PATH setup correctly on macOS runners
host-pty: removed --timeout=60 from pytest args since pytest-timeout is not in the project's dependencies; kept -x for fail-fast behavior

kartikloops · 2026-04-01T04:27:58Z

@luarss could you please run the CI tests? I want to check whether anything is failing so I can debug.

Copilot AI review requested due to automatic review settings March 27, 2026 07:15

Copilot started reviewing on behalf of kartikloops March 27, 2026 07:16 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Comment thread .github/workflows/ci.yaml Outdated

coderabbitai bot reviewed Mar 27, 2026

View reviewed changes

Comment thread .github/workflows/ci.yaml Outdated

luarss reviewed Mar 27, 2026

View reviewed changes

Comment thread .github/workflows/ci.yaml Outdated

coderabbitai bot reviewed Mar 27, 2026

View reviewed changes

kartikloops added 4 commits March 27, 2026 19:13

restructure cross-platform job

2fe7719

unavailable runner clean up

adb0bb3

added matrix

602da31

fold macOS cross-platform jobs into test matrix

1f3cd4d

kartikloops force-pushed the feat/cross-platform-ci branch from 4027e50 to 1f3cd4d Compare March 28, 2026 01:13

luarss requested changes Mar 28, 2026

View reviewed changes

Comment thread .github/workflows/ci.yaml

Comment thread .github/workflows/ci.yaml

use docker/setup-docker-action for macOS Docker setup and drop missin…

edd0841

…g pytest-timeout flag

kartikloops added 2 commits March 30, 2026 00:37

removed docker-build matrix entry

c4a6754

fixed race conditions in test_basic_echo_command

abcec76

use select() in executor instead of non-blocking read

ca8515d

kartikloops requested a review from luarss April 1, 2026 21:07

Conversation

kartikloops commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

coderabbitai bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

kartikloops commented Mar 27, 2026

Uh oh!

Uh oh!

Uh oh!

ArmaanBawa commented Mar 28, 2026

Uh oh!

luarss commented Mar 28, 2026

Uh oh!

kartikloops commented Mar 28, 2026

Uh oh!

kartikloops commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kartikloops commented Mar 27, 2026 •

edited

Loading

coderabbitai bot commented Mar 27, 2026 •

edited

Loading