feat: add GUI screenshot support with headless X11 capture#49
Open
Pavankumar07s wants to merge 24 commits intoluarss:mainfrom
Open
feat: add GUI screenshot support with headless X11 capture#49Pavankumar07s wants to merge 24 commits intoluarss:mainfrom
Pavankumar07s wants to merge 24 commits intoluarss:mainfrom
Conversation
- Add GuiScreenshotResult dataclass with fields for image data, path, format, resolution, size, and timestamp - Inherits from BaseResult for consistent error handling Part of luarss#16
- Add GuiScreenshotTool inheriting from BaseTool - Launch OpenROAD GUI under Xvfb virtual framebuffer - Capture screenshots via gui::save_image Tcl command - Poll for file creation with configurable timeout - Return base64-encoded PNG with metadata - Handle session errors and file size limits gracefully Part of luarss#16
- Export GuiScreenshotTool from tools package - Add GuiScreenshotResult to _format_result type union - Register gui_screenshot as @mcp.tool endpoint - Allow xvfb-run in ALLOWED_COMMANDS setting Part of luarss#16
- 17 mocked unit tests covering all code paths - Test happy path with PNG round-trip validation - Test auto-session creation via xvfb-run - Test default/custom resolution and timeout forwarding - Test temp file generation and stale file cleanup - Test polling loop with delayed file appearance - Test all error handlers: XvfbNotFound, ScreenshotFailed, FileTooLarge, SessionNotFound, SessionError, UnexpectedError - Test _xvfb_available helper function - Verify complete result field structure Part of luarss#16
- Full screenshot round-trip test under xvfb-run + openroad - Session reuse test with existing GUI session - Custom output path test - Invalid session ID negative test - Skip gracefully when xvfb/openroad not available Part of luarss#16
- Install xvfb, x11-utils, imagemagick in test container - Document gui_screenshot tool in README Available Tools - Add headless GUI support section to README Part of luarss#16
Replace the PTY-based gui::save_image approach with X11 root window capture using ImageMagick's import command. OpenROAD GUI mode does not read Tcl commands from stdin/PTY — the Tcl console is a Qt widget with internal I/O, making the previous approach fundamentally broken. New architecture: - Start Xvfb on a deterministic display number - Launch openroad -gui with DISPLAY pointed at that Xvfb - Capture via 'import -window root' on the same display - Track display-to-session mapping for session reuse Also adds pre-flight checks for Xvfb, openroad, and ImageMagick import binaries, plus cleanup_display() for Xvfb process lifecycle.
The GUI tool no longer wraps openroad with xvfb-run. Instead, Xvfb is started as a standalone subprocess and openroad connects to it via the DISPLAY environment variable. Only 'openroad' needs to be in the allowed commands list.
Use typing.Annotated with pydantic.Field(description=...) for each gui_screenshot parameter so that MCP clients (e.g. Inspector, Claude) display meaningful parameter hints instead of just type signatures. Without this, the MCP Inspector was mapping positional values to the wrong parameters due to ambiguous parameter ordering.
Update all 26 unit tests to match the new screenshot architecture: - Mock asyncio.create_subprocess_exec instead of manager.execute_command - Test Xvfb lifecycle: start, display tracking, cleanup_display() - Test import subprocess: success, failure (non-zero rc), timeout - Add tests for new pre-flight checks: ImportNotFound, XvfbStartFailed - Test session-display association and rejection of unknown sessions - Remove obsolete PTY/Tcl command and polling tests All tests use mocked dependencies — no Xvfb/OpenROAD/ImageMagick needed.
Adapt integration tests to the new Xvfb + import architecture: - Remove xvfb-run from allowed commands fixture (not needed) - Session reuse test now registers display via tool's internal state - Remove obsolete skip conditions (GUIStartupTimeout no longer exists) - Increase timeouts for reliable Docker execution - All 5 GUI tests now pass without skips in Docker
Users with OpenROAD installed at non-standard locations (e.g. via
OpenROAD-flow-scripts) get OpenROADNotFound errors because the binary
is not on PATH.
Add _get_openroad_exe() that checks the OPENROAD_EXE environment
variable first, then falls back to shutil.which('openroad'). The
resolved absolute path is passed directly to create_session().
- Check OPENROAD_EXE env var → resolve to absolute path → verify exists
- Fall back to shutil.which('openroad') if env var not set
- Improved error message guides users to set OPENROAD_EXE or update PATH
- Added 3 unit tests for env var resolution logic
- Updated all existing mocks from _openroad_available to _get_openroad_exe
Move GUI_DISPLAY_RESOLUTION and GUI_CAPTURE_TIMEOUT_MS from hardcoded module-level constants in gui.py into the central Settings class so they can be overridden via environment variables: OPENROAD_GUI_DISPLAY_RESOLUTION (default: 1280x1024x24) OPENROAD_GUI_CAPTURE_TIMEOUT_MS (default: 8000) This follows the same env-var override pattern used by the rest of the configuration (OPENROAD_COMMAND_TIMEOUT, etc.). Also fixes the stale gui_screenshot docstring in server.py that still referenced xvfb-run and gui::save_image from the abandoned PTY approach.
Remove all hardcoded values from gui.py and make them configurable through the centralised Settings class with environment variable overrides, following the same pattern as existing configuration. Settings added (all with OPENROAD_GUI_* env var overrides): - GUI_MAX_SCREENSHOT_SIZE_MB (default: 50) - GUI_IMPORT_TIMEOUT_S (default: 15.0) - GUI_DISPLAY_START / GUI_DISPLAY_END (default: 42-100) - GUI_STARTUP_TIMEOUT_S (default: 15.0) - GUI_STARTUP_POLL_INTERVAL_S (default: 0.5) Replace fixed asyncio.sleep(6) with xdpyinfo readiness polling: - New _wait_for_display() polls xdpyinfo on the target display - Returns immediately when the display is responsive - Falls back gracefully after configurable timeout - Reduces Docker integration test time from ~23s to ~4s ALLOWED_COMMANDS validation already handles absolute paths by extracting os.path.basename() before checking the allowlist, so OPENROAD_EXE paths like /path/to/openroad correctly match 'openroad'. - 4 new tests for _wait_for_display (immediate, retry, timeout, OSError) - Updated all existing tests to use settings-based values - 33 unit tests + 15 Docker integration tests passing
… for token optimization Address mentor feedback on token-limit handling for GUI screenshots. The raw PNG from ImageMagick import is now post-processed through Pillow before being returned, enabling significant token savings for LLM consumers. New parameters on gui_screenshot tool: - image_format: 'png', 'jpeg' (default), or 'webp' - quality: 1-100 compression quality for JPEG/WebP - scale: 0.0-1.0 downscale factor (applied after crop) - crop: 'x0 y0 x1 y1' pixel region extraction - return_mode: 'base64' (full), 'path' (no data), 'preview' (256px thumb) New result fields: original_size_bytes, return_mode, compression_applied, compression_ratio, width, height. Settings additions: GUI_DEFAULT_IMAGE_FORMAT, GUI_DEFAULT_JPEG_QUALITY (configurable via OPENROAD_GUI_DEFAULT_* env vars). Bug fix: quality=0 was treated as None due to falsy 'or' expression; changed to explicit 'is not None' check. Test coverage: 22 new unit tests (55 total for GUI tool), all 266 tests passing including 15 Docker integration tests with real Xvfb+OpenROAD.
The screenshot was capturing a blank black frame because it fired immediately after Xvfb became responsive, before OpenROAD's Qt GUI had time to render. Add _wait_for_gui_ready() which polls 'xwininfo -root -children' to detect when a real application window appears on the X display. This runs after _wait_for_display() (Xvfb ready) and before the import capture, ensuring the GUI has actually rendered. Before: original_size_bytes=295 (solid black 1280x1024 PNG) After: original_size_bytes=78230 (44% non-black pixels, real GUI) New settings: GUI_APP_READY_TIMEOUT_S (default 15s), GUI_APP_READY_POLL_INTERVAL_S (default 0.5s). Tests: 4 new unit tests for _wait_for_gui_ready, 270 total passing.
- Treat empty strings as None for image_format, return_mode, resolution, output_path, and crop (MCP Inspector sends '' for blank optional fields) - Infer image format from output_path extension when image_format not specified - Auto-create parent directories for output_path - Correct file extension when it doesn't match the requested format - Add 'or None' normalization in server.py for all string params - Add 9 new unit tests covering all edge cases (68 GUI tests total)
- Change string params from 'str | None = None' to 'str = ""' so
MCP Inspector renders simple text inputs instead of anyOf type toggles
that silently send null regardless of user input
- Shorten gui_screenshot docstring for readability
- Support comma-separated crop values ('x0,y0,x1,y1')
- Accept .jpeg extension for jpeg format
- Strip whitespace on all string params
- Add debug logging for parameter tracing
Move 6 previously hardcoded magic numbers in gui.py into Settings with environment variable overrides: - GUI_PREVIEW_SIZE_PX (256) — preview thumbnail max dimension - GUI_XVFB_SETTLE_S (1.0) — delay after Xvfb startup - GUI_SUBPROCESS_TIMEOUT_S (3.0) — xdpyinfo/xwininfo timeout - GUI_DISPLAY_FALLBACK_RANGE (200) — random display fallback range - GUI_ERROR_TRUNCATE_CHARS (500) — stderr truncation limit - GUI_TEMP_UUID_LENGTH (12) — hex chars in temp filenames All values retain their original defaults and are overridable via OPENROAD_GUI_* environment variables.
1. Change timeout_ms, quality, scale from int|None / float|None to str='' in server.py to avoid MCP Inspector anyOf type-toggle bug (same root cause as the string param fix). Parse with _int()/_float() helpers before passing to execute(). 2. When output_path is an existing directory (or ends with '/'), generate a default filename inside it instead of treating the directory name as a file. Prevents write errors and matches user expectations. Added 5 tests: directory output_path (3), scale reset to default, quality None default. 306 tests pass.
📝 WalkthroughWalkthroughAdds a headless GUI screenshot feature: Docker test-image packages, new GUI settings, a GuiScreenshotTool (Xvfb/OpenROAD/ImageMagick/Pillow) with session management, server endpoint wiring and result model, exports, and extensive unit + integration tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server as MCP_Server
participant Tool as GuiScreenshotTool
participant Xvfb
participant OpenROAD
participant Import as ImageMagick
participant Pillow as ImageProcessing
Client->>Server: gui_screenshot(params)
Server->>Tool: execute(session_id?, resolution, ...)
alt create session
Tool->>Xvfb: start Xvfb on free DISPLAY
Xvfb-->>Tool: DISPLAY assigned
Tool->>OpenROAD: launch openroad -gui with DISPLAY
OpenROAD-->>Tool: process running
Tool->>Tool: wait for display and window readiness
else reuse session
Tool->>Tool: validate existing session DISPLAY
end
Tool->>Import: import -window root -> raw PNG
Import-->>Tool: raw PNG file
Tool->>Pillow: crop/scale/convert -> final image
Pillow-->>Tool: processed image
Tool->>Tool: enforce size/format, encode or write path
Tool-->>Server: GuiScreenshotResult (base64/path/preview)
Server-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/openroad_mcp/config/settings.py (1)
61-68: Consider adding validation for display range consistency.There's no validation ensuring
GUI_DISPLAY_START < GUI_DISPLAY_END. If misconfigured (e.g., via environment variables), this could cause issues in_find_free_display.♻️ Suggested enhancement with Pydantic model_validator
+from pydantic import model_validator class Settings(BaseModel): """Configuration settings for the OpenROAD MCP server.""" + + `@model_validator`(mode='after') + def validate_gui_display_range(self) -> 'Settings': + if self.GUI_DISPLAY_START >= self.GUI_DISPLAY_END: + raise ValueError( + f"GUI_DISPLAY_START ({self.GUI_DISPLAY_START}) must be less than " + f"GUI_DISPLAY_END ({self.GUI_DISPLAY_END})" + ) + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openroad_mcp/config/settings.py` around lines 61 - 68, Add validation to ensure GUI_DISPLAY_START < GUI_DISPLAY_END by implementing a Pydantic validator (e.g., model_validator or root_validator) on the settings model that checks the two fields and raises a clear ValueError if the condition fails; update the settings class that defines GUI_DISPLAY_START and GUI_DISPLAY_END so misconfigured env values are rejected early and _find_free_display can assume a valid range.tests/tools/test_gui_tool.py (1)
1030-1040: Remove unusedtmp_pathfixture.The
tmp_pathfixture is declared but not used in this test since it explicitly passesoutput_path=""to trigger temp file creation.♻️ Suggested fix
- async def test_explicit_png_format_with_empty_string_output(self, tool, tmp_path): + async def test_explicit_png_format_with_empty_string_output(self, tool): """image_format='png' with output_path='' uses temp file with .png ext.""" self._register_display(tool, "s1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/test_gui_tool.py` around lines 1030 - 1040, Remove the unused tmp_path fixture from the test function signature to avoid an unused parameter: update the test_explicit_png_format_with_empty_string_output definition in tests/tools/test_gui_tool.py to drop the tmp_path argument (keeping the rest of the test body unchanged) so the test only accepts (self, tool) and continues to call tool.execute(..., output_path="", image_format="png").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 128-130: The README's "Headless GUI Support" section is outdated
and describes behavior (use of xvfb-run, gui::save_image, PNG output, and
arbitrary session reuse) that the implementation does not follow; update the
documentation for the gui_screenshot tool to state that it uses a persistent
Xvfb display (Xvfb), captures via ImageMagick's import -window root, defaults to
JPEG output, and only reuses GUI sessions previously created by gui_screenshot
(rather than arbitrary session_id reuse); mention the xvfb dependency and that
the Docker test image is pre-configured, and replace references to xvfb-run,
gui::save_image, and PNG accordingly.
In `@src/openroad_mcp/server.py`:
- Around line 170-193: The helpers _int and _float currently call int()/float()
directly and let ValueError bubble up; change them to catch ValueError and raise
the tool's structured parameter error so execute() can return the normal
InvalidParameter JSON. Specifically, wrap the parse in try/except ValueError
inside _int(v: str) and _float(v: str), and on parse failure raise
gui_screenshot_tool.InvalidParameter (or the module's equivalent structured
exception) including the parameter name (e.g., "timeout_ms" or "scale") and the
original raw value so GuiScreenshotTool.execute() can convert it to the standard
tool response.
In `@src/openroad_mcp/tools/gui.py`:
- Around line 131-142: The probe subprocesses started in _wait_for_gui_ready
(the "xdpyinfo" and "xwininfo" launches) are left running when asyncio.wait_for
times out; modify the timeout handling so that on TimeoutError or OSError you
kill the spawned proc (call proc.kill()), then await proc.wait() (with a short
timeout) to reap it before retrying, and only then continue the loop; apply this
cleanup pattern to both the block that launches "xdpyinfo" (around the code that
creates proc and awaits proc.wait()) and the block that launches "xwininfo" so
no orphaned subprocesses accumulate.
- Around line 104-113: The display allocation in _find_free_display (and the
code that starts Xvfb in gui_screenshot) is racy: two concurrent requests can
see the same free /tmp/.X*-lock and both try to start Xvfb. Make allocation
atomic by either (preferred) switching to Xvfb's -displayfd mode so the X server
chooses and returns an FD/number atomically, updating the Xvfb startup path and
downstream parsing logic in gui_screenshot to use that returned display, or
(alternative) introduce a module-level asyncio.Lock used by _find_free_display
and the Xvfb-start sequence in gui_screenshot to ensure only one coroutine
probes/claims a display at a time; update both locations mentioned (function
_find_free_display and the Xvfb-start code paths around gui_screenshot) to
acquire/release that lock while probing and immediately creating the lock file /
starting Xvfb so no other coroutine can race.
- Around line 712-741: Wrap the call to self.manager.create_session in a
try/except/finally so that if await self.manager.create_session(...) raises, you
cleanly stop the started Xvfb process (use xvfb_proc.kill() or terminate() and
await xvfb_proc.wait()) and do not leave stale state; only assign
self._session_displays[session_id] and self._xvfb_pids[session_id] after
create_session succeeds. Ensure you handle/propagate the original exception
after teardown so callers still see the failure.
In `@tests/tools/test_gui_tool.py`:
- Around line 1260-1265: The test test_find_free_display_no_locks includes an
unused tmp_path fixture; remove the tmp_path parameter from the test signature
to avoid lint/test warnings. Open the test function
(test_find_free_display_no_locks) and delete the tmp_path argument so the
function becomes def test_find_free_display_no_locks(): while leaving the Path
mock and assertions against _find_free_display and settings.GUI_DISPLAY_START
unchanged.
---
Nitpick comments:
In `@src/openroad_mcp/config/settings.py`:
- Around line 61-68: Add validation to ensure GUI_DISPLAY_START <
GUI_DISPLAY_END by implementing a Pydantic validator (e.g., model_validator or
root_validator) on the settings model that checks the two fields and raises a
clear ValueError if the condition fails; update the settings class that defines
GUI_DISPLAY_START and GUI_DISPLAY_END so misconfigured env values are rejected
early and _find_free_display can assume a valid range.
In `@tests/tools/test_gui_tool.py`:
- Around line 1030-1040: Remove the unused tmp_path fixture from the test
function signature to avoid an unused parameter: update the
test_explicit_png_format_with_empty_string_output definition in
tests/tools/test_gui_tool.py to drop the tmp_path argument (keeping the rest of
the test body unchanged) so the test only accepts (self, tool) and continues to
call tool.execute(..., output_path="", image_format="png").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01ef3e19-96f0-4442-b6cc-e5c59cff2be4
📒 Files selected for processing (10)
Dockerfile.testREADME.mdsrc/openroad_mcp/config/settings.pysrc/openroad_mcp/core/models.pysrc/openroad_mcp/server.pysrc/openroad_mcp/tools/__init__.pysrc/openroad_mcp/tools/base.pysrc/openroad_mcp/tools/gui.pytests/integration/test_gui.pytests/tools/test_gui_tool.py
Author
|
There are some feedback given by codeRabbit , let me go though it and fix the major points in code. |
- Update README Headless GUI section to match implementation (Xvfb + ImageMagick import, JPEG default, session reuse constraints) - Add try/except ValueError in _int()/_float() server helpers so bad numeric input returns structured InvalidParameter JSON instead of crashing - Kill timed-out xdpyinfo/xwininfo probe subprocesses before retrying to prevent orphaned process accumulation - Add module-level asyncio.Lock around display allocation + Xvfb startup to prevent concurrent race on the same display number - Wrap manager.create_session() in try/except to terminate Xvfb if OpenROAD session creation fails (prevents leaked processes) - Wire cleanup_display() into server shutdown so Xvfb processes are cleaned up on graceful exit - Add Pydantic model_validator to reject GUI_DISPLAY_START >= GUI_DISPLAY_END at settings load time - Remove unused tmp_path fixtures from two tests - Fix _make_xvfb_proc mock to use sync terminate() matching real API
CodeRabbit review fixes: - Update README Headless GUI section to match implementation (Xvfb + ImageMagick import, JPEG default, session reuse constraints) - Add try/except ValueError in _int()/_float() server helpers with 'from None' chaining; return structured InvalidParameter JSON - Kill timed-out xdpyinfo/xwininfo probe subprocesses before retrying - Add module-level asyncio.Lock around display allocation + Xvfb start - Wrap manager.create_session() in try/except to terminate Xvfb on failure - Wire cleanup_display() into server shutdown for graceful Xvfb cleanup - Add Pydantic model_validator: GUI_DISPLAY_START < GUI_DISPLAY_END - Remove unused tmp_path fixtures from two tests - Fix _make_xvfb_proc mock to use sync terminate() Preview mode bugs: - Fix width/height/size_bytes metadata to reflect thumbnail dimensions - Fix resolution field to show thumbnail size instead of display resolution - Save thumbnail to disk (was saving full-size image at image_path) - Update preview test to verify file on disk is the actual thumbnail 306 passed, 15 skipped.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/openroad_mcp/server.py (1)
221-225:⚠️ Potential issue | 🟠 MajorAlso hook GUI cleanup into normal session termination.
This only handles process-exit cleanup. A GUI session terminated through the public session-termination path still never calls
gui_screenshot_tool.cleanup_display(session_id), so its Xvfb sidecar can live until server shutdown. Please wire the same cleanup into the regular session lifecycle as well, ideally after a successful terminate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openroad_mcp/server.py` around lines 221 - 225, The GUI Xvfb cleanup is only performed on process exit; ensure gui_screenshot_tool.cleanup_display(session_id) is also called when a session is terminated via the normal public termination path by adding a call after the session termination completes (e.g., in your session termination handler such as terminate_session / SessionManager.terminate or wherever await manager.terminate(session_id) is invoked). Check gui_screenshot_tool._session_displays for the session_id first (or handle KeyError) and call gui_screenshot_tool.cleanup_display(session_id) after a successful termination so the Xvfb sidecar is removed immediately rather than waiting for manager.cleanup_all().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openroad_mcp/config/settings.py`:
- Around line 118-125: The validator method _validate_display_range currently
only checks GUI_DISPLAY_START and GUI_DISPLAY_END; add a check in that same
method to reject non-positive GUI_DISPLAY_FALLBACK_RANGE by raising a ValueError
if settings.GUI_DISPLAY_FALLBACK_RANGE <= 0 so startup fails fast; reference
GUI_DISPLAY_FALLBACK_RANGE in the error message and mention _find_free_display
in the comment or message to indicate why a positive fallback range is required.
In `@src/openroad_mcp/tools/gui.py`:
- Around line 399-402: The temp raw capture file (raw_path) is only unlinked on
the success path; wrap the capture and post-processing block that uses tmp_dir,
raw_name, and raw_path in a try/finally so raw_path.unlink(missing_ok=True)
always runs in the finally block to guarantee cleanup on early returns or
exceptions; apply the same try/finally pattern to the analogous block referenced
at lines 556-557 to ensure both code paths always remove the temporary
openroad_gui_raw_*.png file.
- Around line 115-116: The fallback display calculation currently uses start +
uuid.uuid4().int % settings.GUI_DISPLAY_FALLBACK_RANGE which can produce values
inside the exhausted primary range; change it to begin at end instead of start
so the fallback is always outside the configured range (use end +
(uuid.uuid4().int % settings.GUI_DISPLAY_FALLBACK_RANGE) or equivalent). Update
the expression that returns the fallback display number (referencing start, end,
settings.GUI_DISPLAY_FALLBACK_RANGE, and uuid.uuid4().int) so the generated
display is offset from end, preventing collisions that cause XvfbStartFailed.
In `@tests/tools/test_gui_tool.py`:
- Around line 1271-1276: The test currently allows any value in the fallback
window but should assert the helper's exact contract: update
test_find_free_display_no_locks to assert that _find_free_display() ==
settings.GUI_DISPLAY_START (use the same patched Path.exists behavior), and add
a new test (e.g., test_find_free_display_all_occupied) that simulates all
displays occupied (make Path.exists return True for the full range) and asserts
the returned value is >= settings.GUI_DISPLAY_END to verify the fallback
behavior; reference the helper function _find_free_display and the constants
settings.GUI_DISPLAY_START and settings.GUI_DISPLAY_END when making these
assertions.
---
Duplicate comments:
In `@src/openroad_mcp/server.py`:
- Around line 221-225: The GUI Xvfb cleanup is only performed on process exit;
ensure gui_screenshot_tool.cleanup_display(session_id) is also called when a
session is terminated via the normal public termination path by adding a call
after the session termination completes (e.g., in your session termination
handler such as terminate_session / SessionManager.terminate or wherever await
manager.terminate(session_id) is invoked). Check
gui_screenshot_tool._session_displays for the session_id first (or handle
KeyError) and call gui_screenshot_tool.cleanup_display(session_id) after a
successful termination so the Xvfb sidecar is removed immediately rather than
waiting for manager.cleanup_all().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bc9607c-895b-46d2-b464-9b5a478adf87
📒 Files selected for processing (5)
README.mdsrc/openroad_mcp/config/settings.pysrc/openroad_mcp/server.pysrc/openroad_mcp/tools/gui.pytests/tools/test_gui_tool.py
- Add GUI_DISPLAY_FALLBACK_RANGE > 0 validation in settings model_validator - Fix fallback display allocation to start from end of range (not start) to prevent overlap with already-exhausted primary range - Wrap raw capture file (steps 3-6) in try/finally for guaranteed cleanup instead of explicit unlink on the success path only - Tighten test_find_free_display_no_locks assertion to exact equality - Add test_find_free_display_all_occupied for fallback range coverage
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/openroad_mcp/tools/gui.py (1)
678-687:⚠️ Potential issue | 🟠 Major
cleanup_display()still isn't wired into normal session teardown.In the provided wiring,
src/openroad_mcp/server.py:216-230only calls this during global shutdown. Sessions terminated individually through the manager can still leave their Xvfb sidecar running.
🧹 Nitpick comments (2)
tests/tools/test_gui_tool.py (2)
242-257: This timeout test is exercising the error path today.
_make_import_proc(out_file)writes the mocked PNG to the final output path, butexecute()expectsimportto create the raw temp file first. The test still passes because it only inspectswait_forarguments, so it won't catch happy-path regressions.🛠️ Suggested change
- mock_proc = _make_import_proc(out_file) - with ( - patch("asyncio.create_subprocess_exec", return_value=mock_proc), + patch("asyncio.create_subprocess_exec", side_effect=_make_import_side_effect()), patch("asyncio.wait_for", wraps=asyncio.wait_for) as mock_wait, ): - await tool.execute(session_id="s1", output_path=str(out_file), timeout_ms=20_000) + raw = await tool.execute(session_id="s1", output_path=str(out_file), timeout_ms=20_000) + + result = json.loads(raw) + assert result["error"] is None # wait_for should have been called with timeout=20.0 _, kwargs = mock_wait.call_args assert kwargs.get("timeout") == 20.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/test_gui_tool.py` around lines 242 - 257, The test currently fakes the import subprocess by having _make_import_proc write the final PNG directly, but execute() expects the import step to create the intermediate raw/temp file first; update the test so the mocked process creates the same intermediate temp/raw file name that execute() will look for (instead of writing directly to out_file) — i.e., change _make_import_proc (or the test setup in test_custom_timeout) to write the expected temp/raw path that execute() uses before any final move/convert so the test exercises the happy path and still asserts wait_for was called with timeout=20.0; reference symbols: test_custom_timeout, _make_import_proc, execute, output_path, and the import/temp file behavior.
398-417: Assert the Xvfb cleanup side effect in this regression test.Right now this only proves the returned
SessionError. If_create_gui_session()stops terminating the spawned Xvfb again, this test still passes.🛠️ Suggested change
async def test_session_error_on_create(self, tool, mock_manager): """Returns SessionError when create_session raises.""" from openroad_mcp.interactive.models import SessionError as SE mock_manager.create_session.side_effect = SE("boom") + xvfb_proc = _make_xvfb_proc() with ( patch("openroad_mcp.tools.gui._xvfb_available", return_value=True), patch("openroad_mcp.tools.gui._get_openroad_exe", return_value="/usr/bin/openroad"), patch("openroad_mcp.tools.gui._import_available", return_value=True), - patch("asyncio.create_subprocess_exec", return_value=_make_xvfb_proc()), + patch("asyncio.create_subprocess_exec", return_value=xvfb_proc), patch("asyncio.sleep", new_callable=AsyncMock), patch("openroad_mcp.tools.gui._wait_for_display", new_callable=AsyncMock, return_value=True), patch("openroad_mcp.tools.gui._wait_for_gui_ready", new_callable=AsyncMock, return_value=True), ): raw = await tool.execute() result = json.loads(raw) assert result["error"] == "SessionError" assert "boom" in result["message"] + xvfb_proc.terminate.assert_called_once() + xvfb_proc.wait.assert_awaited()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tools/test_gui_tool.py` around lines 398 - 417, The test test_session_error_on_create currently only checks the returned SessionError but must also assert that the spawned Xvfb process is cleaned up; update the test to capture the patched asyncio.create_subprocess_exec mock (the one used when creating the Xvfb via _create_gui_session) and after awaiting tool.execute() assert that the created process object's cleanup method (e.g., kill() or terminate(), whatever _make_xvfb_proc exposes) was called/awaited on the subprocess mock returned by asyncio.create_subprocess_exec, ensuring _create_gui_session terminates the Xvfb even on SessionError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openroad_mcp/config/settings.py`:
- Around line 118-130: The validator _validate_display_range currently checks
GUI_DISPLAY_* values but does not validate the poll intervals; update the same
model_validator (or add a similar "after" validator on the Settings model) to
assert that OPENROAD_GUI_STARTUP_POLL_INTERVAL_S and
OPENROAD_GUI_APP_READY_POLL_INTERVAL_S are strictly greater than 0 and raise a
ValueError with a clear message (include the offending variable name and value)
if not; reference the Settings class and the _validate_display_range validator
so reviewers can locate where to add these checks.
In `@src/openroad_mcp/tools/gui.py`:
- Around line 127-158: The loop in the display readiness probe (function
handling display_num, using asyncio.create_subprocess_exec and variables
timeout, interval, elapsed) only increments elapsed by sleep intervals, so
long-running probes can exceed the configured timeout; change to compute a
monotonic deadline (e.g., start = time.monotonic(); end = start + timeout) and
in each iteration calculate remaining = end - time.monotonic(), break if
remaining <= 0, then pass min(settings.GUI_SUBPROCESS_TIMEOUT_S, remaining) as
the per-probe timeout to asyncio.wait_for and to any sleep (await
asyncio.sleep(min(interval, remaining))); apply the exact same pattern in the
other helper (_wait_for_gui_ready) to cap probes by the remaining budget and
ensure the function returns or times out correctly.
---
Nitpick comments:
In `@tests/tools/test_gui_tool.py`:
- Around line 242-257: The test currently fakes the import subprocess by having
_make_import_proc write the final PNG directly, but execute() expects the import
step to create the intermediate raw/temp file first; update the test so the
mocked process creates the same intermediate temp/raw file name that execute()
will look for (instead of writing directly to out_file) — i.e., change
_make_import_proc (or the test setup in test_custom_timeout) to write the
expected temp/raw path that execute() uses before any final move/convert so the
test exercises the happy path and still asserts wait_for was called with
timeout=20.0; reference symbols: test_custom_timeout, _make_import_proc,
execute, output_path, and the import/temp file behavior.
- Around line 398-417: The test test_session_error_on_create currently only
checks the returned SessionError but must also assert that the spawned Xvfb
process is cleaned up; update the test to capture the patched
asyncio.create_subprocess_exec mock (the one used when creating the Xvfb via
_create_gui_session) and after awaiting tool.execute() assert that the created
process object's cleanup method (e.g., kill() or terminate(), whatever
_make_xvfb_proc exposes) was called/awaited on the subprocess mock returned by
asyncio.create_subprocess_exec, ensuring _create_gui_session terminates the Xvfb
even on SessionError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 793a78e0-cb79-42bb-9e87-44626b2f83f9
📒 Files selected for processing (3)
src/openroad_mcp/config/settings.pysrc/openroad_mcp/tools/gui.pytests/tools/test_gui_tool.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/openroad_mcp/tools/gui.py (3)
660-668: Consider usinglogger.exceptionforSessionError.Line 661 uses
logger.errorbut the static analyzer suggestslogger.exceptionto include the traceback. For session-related errors that may indicate bugs (vs. expected terminations), having the traceback could aid debugging.♻️ Suggested change
except (SessionTerminatedError, SessionError) as e: - logger.error("GUI session error for %s: %s", session_id, e) + logger.exception("GUI session error for %s: %s", session_id, e) return self._format_result(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openroad_mcp/tools/gui.py` around lines 660 - 668, The handler for SessionTerminatedError/SessionError currently logs with logger.error which omits traceback; change the logging call to logger.exception when catching SessionError to capture stack information while preserving the existing return behavior from _format_result(GuiScreenshotResult(...)); specifically, in the except block handling SessionTerminatedError and SessionError, call logger.exception("GUI session error for %s: %s", session_id, e) (or conditionally call logger.exception only for SessionError and keep logger.error for SessionTerminatedError) before returning the GuiScreenshotResult with error="SessionError" and message=str(e).
427-431: Minor: Unusedstdoutvariable.The
stdoutvariable fromproc.communicate()is captured but never used. Consider using_to indicate it's intentionally ignored.♻️ Suggested change
- stdout, stderr = await asyncio.wait_for( + _, stderr = await asyncio.wait_for( proc.communicate(), timeout=import_timeout, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openroad_mcp/tools/gui.py` around lines 427 - 431, The captured stdout from proc.communicate() is unused; update the await asyncio.wait_for(...) call to ignore it by replacing the assigned variable name stdout with an underscore (or _stdout) so only stderr is retained, i.e. change the tuple assignment from (stdout, stderr) to (_, stderr) while keeping proc.communicate(), import_timeout and the surrounding await asyncio.wait_for(...) call unchanged.
588-624: Preview mode overwrites the full-size image with the thumbnail.Lines 600-601 overwrite
final_pathwith the preview thumbnail bytes. If the user specifies anoutput_path, the full-resolution image is lost and only the 256px thumbnail is saved. This behavior may be surprising if users expect both the full image and a preview.Consider either:
- Keeping the full-size image and writing the preview to a separate file (e.g.,
*_preview.jpg)- Documenting this behavior more explicitly in the docstring
If this is intentional to save disk space, adding a brief comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openroad_mcp/tools/gui.py` around lines 588 - 624, The preview branch currently overwrites the full-size file (final_path.write_bytes(preview_bytes)) when mode == "preview", losing the original image; change this to preserve the full-resolution output by writing the preview to a separate file (e.g., construct a preview path from final_path like final_path.stem + "_preview" + final_path.suffix) and write preview_bytes there, then populate GuiScreenshotResult.image_path with the preview path (or include both paths in the result if desired), or alternatively add a clear comment/docstring near the preview branch explaining the intentional overwrite behavior if keeping the current behavior is desired; update any references to image_path/image_format/size_bytes/resolution in the GuiScreenshotResult population accordingly.
🤖 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/tools/gui.py`:
- Around line 660-668: The handler for SessionTerminatedError/SessionError
currently logs with logger.error which omits traceback; change the logging call
to logger.exception when catching SessionError to capture stack information
while preserving the existing return behavior from
_format_result(GuiScreenshotResult(...)); specifically, in the except block
handling SessionTerminatedError and SessionError, call logger.exception("GUI
session error for %s: %s", session_id, e) (or conditionally call
logger.exception only for SessionError and keep logger.error for
SessionTerminatedError) before returning the GuiScreenshotResult with
error="SessionError" and message=str(e).
- Around line 427-431: The captured stdout from proc.communicate() is unused;
update the await asyncio.wait_for(...) call to ignore it by replacing the
assigned variable name stdout with an underscore (or _stdout) so only stderr is
retained, i.e. change the tuple assignment from (stdout, stderr) to (_, stderr)
while keeping proc.communicate(), import_timeout and the surrounding await
asyncio.wait_for(...) call unchanged.
- Around line 588-624: The preview branch currently overwrites the full-size
file (final_path.write_bytes(preview_bytes)) when mode == "preview", losing the
original image; change this to preserve the full-resolution output by writing
the preview to a separate file (e.g., construct a preview path from final_path
like final_path.stem + "_preview" + final_path.suffix) and write preview_bytes
there, then populate GuiScreenshotResult.image_path with the preview path (or
include both paths in the result if desired), or alternatively add a clear
comment/docstring near the preview branch explaining the intentional overwrite
behavior if keeping the current behavior is desired; update any references to
image_path/image_format/size_bytes/resolution in the GuiScreenshotResult
population accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f39dd074-8468-4a5a-b489-102af3673886
📒 Files selected for processing (2)
src/openroad_mcp/config/settings.pysrc/openroad_mcp/tools/gui.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
luarss
requested changes
Mar 15, 2026
Comment on lines
+128
to
+130
| ### Headless GUI Support | ||
|
|
||
| The `gui_screenshot` tool launches OpenROAD's GUI under a persistent **Xvfb** virtual display and captures the screen using ImageMagick's `import -window root` command — no physical display is required. When called without a `session_id` the tool automatically starts Xvfb, launches `openroad -gui -no_init` on that display, waits for the GUI window to render, and returns the screenshot as base64-encoded JPEG by default (configurable to PNG or WebP). You can reuse a previously created GUI session by passing its `session_id` (only sessions created by `gui_screenshot` can be reused). This feature requires `xvfb` and ImageMagick to be installed (`apt-get install -y xvfb imagemagick`) and is pre-configured in the Docker test image. |
Owner
There was a problem hiding this comment.
Suggested change
| ### Headless GUI Support | |
| The `gui_screenshot` tool launches OpenROAD's GUI under a persistent **Xvfb** virtual display and captures the screen using ImageMagick's `import -window root` command — no physical display is required. When called without a `session_id` the tool automatically starts Xvfb, launches `openroad -gui -no_init` on that display, waits for the GUI window to render, and returns the screenshot as base64-encoded JPEG by default (configurable to PNG or WebP). You can reuse a previously created GUI session by passing its `session_id` (only sessions created by `gui_screenshot` can be reused). This feature requires `xvfb` and ImageMagick to be installed (`apt-get install -y xvfb imagemagick`) and is pre-configured in the Docker test image. |
This should be removed - would like to keep README lean.
| # Install dependencies | ||
| RUN uv sync --all-extras --inexact | ||
|
|
||
| # Install X virtual framebuffer and utilities for headless GUI tests |
Owner
There was a problem hiding this comment.
If this dep is not going to be changed often we should put it earlier to benefit from layer caching
Author
There was a problem hiding this comment.
Yes @luarss great idea, we should change it inorder to get benefits from layer caching.
| "READ_CHUNK_SIZE": ("OPENROAD_READ_CHUNK_SIZE", int), | ||
| "LOG_LEVEL": ("LOG_LEVEL", str), | ||
| "LOG_FORMAT": ("LOG_FORMAT", str), | ||
| "GUI_DISPLAY_RESOLUTION": ("OPENROAD_GUI_DISPLAY_RESOLUTION", str), |
Owner
There was a problem hiding this comment.
Do we really need all these config variables? There's like almost 20 of them.
| ) | ||
|
|
||
| # GUI screenshot settings | ||
| GUI_DISPLAY_RESOLUTION: str = Field( |
| default="1280x1024x24", | ||
| description="Default Xvfb virtual display resolution (WxHxDepth)", | ||
| ) | ||
| GUI_CAPTURE_TIMEOUT_MS: int = Field( |
Owner
There was a problem hiding this comment.
Timeout could potentially be a constant?
| through the PTY (which OpenROAD ignores in GUI mode). | ||
| """ | ||
|
|
||
| def __init__(self, manager: OpenROADManager) -> None: |
Owner
There was a problem hiding this comment.
Please review how the existing tools are written. Tools should be stateless
| logger.info("Initiating graceful shutdown of OpenROAD services...") | ||
|
|
||
| # Clean up any Xvfb displays managed by the GUI tool | ||
| for sid in list(gui_screenshot_tool._session_displays): |
Owner
There was a problem hiding this comment.
Typically - internal attributes (prefixed by underscore) should not be accessed externally.
| # Module-level names kept for backward compatibility and test imports. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| MAX_SCREENSHOT_SIZE_MB: int = settings.GUI_MAX_SCREENSHOT_SIZE_MB |
Comment on lines
+171
to
+191
| def _clean(v: str) -> str | None: | ||
| v = str(v).strip() | ||
| return v if v else None | ||
|
|
||
| def _int(v: str, name: str = "parameter") -> int | None: | ||
| v = str(v).strip() | ||
| if not v: | ||
| return None | ||
| try: | ||
| return int(v) | ||
| except ValueError: | ||
| raise ValueError(f"Invalid integer value for {name}: '{v}'") from None | ||
|
|
||
| def _float(v: str, name: str = "parameter") -> float | None: | ||
| v = str(v).strip() | ||
| if not v: | ||
| return None | ||
| try: | ||
| return float(v) | ||
| except ValueError: | ||
| raise ValueError(f"Invalid numeric value for {name}: '{v}'") from None |
Owner
There was a problem hiding this comment.
you should not have to validate tools inside this scope. It should be consolidated higher up in core/
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a
gui_screenshotMCP tool that captures screenshots from OpenROAD's GUI running in a headless X11 display. OpenROAD in-guimode does not accept Tcl commands via stdin/PTY, so this tool uses Xvfb + ImageMagickimport -window rootto capture the framebuffer directly.Fixes #16
What's New
Core Feature
openroad -gui -no_init→ waits for display + window readiness → captures screenshotbase64— full image as base-64 (default)path— file path only, no image datapreview— 256px thumbnail + file pathToken Optimization
JPEG default with quality=85 reduces screenshot payloads by 70–90% compared to raw PNG base64. The
pathandpreviewreturn modes save even more by avoiding large base64 payloads entirely.Configuration
All tunable values are centralized in
Settingswith environment variable overrides (OPENROAD_GUI_*prefix):GUI_DEFAULT_IMAGE_FORMATjpegOPENROAD_GUI_DEFAULT_IMAGE_FORMATGUI_DEFAULT_JPEG_QUALITY85OPENROAD_GUI_DEFAULT_JPEG_QUALITYGUI_CAPTURE_TIMEOUT_MS8000OPENROAD_GUI_CAPTURE_TIMEOUT_MSGUI_DISPLAY_RESOLUTION1280x1024x24OPENROAD_GUI_DISPLAY_RESOLUTIONGUI_PREVIEW_SIZE_PX256OPENROAD_GUI_PREVIEW_SIZE_PXGUI_MAX_SCREENSHOT_SIZE_MB10OPENROAD_GUI_MAX_SCREENSHOT_SIZE_MBMCP Inspector Compatibility
All tool parameters use
str = ""(notstr | Noneorint | None) to avoid the MCP InspectoranyOftype-toggle bug where clearing a field retains the previous value instead of resetting to default. Empty strings are parsed and normalized before reaching the tool logic.Smart
output_pathHandlingimage_formatis omittedFiles Changed (10 files, +2585 lines)
src/openroad_mcp/tools/gui.pytests/tools/test_gui_tool.pytests/integration/test_gui.pysrc/openroad_mcp/server.pygui_screenshotMCP toolsrc/openroad_mcp/config/settings.pysrc/openroad_mcp/core/models.pyGuiScreenshotResultdata modelsrc/openroad_mcp/tools/__init__.pyGuiScreenshotToolsrc/openroad_mcp/tools/base.py_format_resulthelperDockerfile.testREADME.mdTest Results
Prerequisites
apt-get install -y xvfb)apt-get install -y imagemagick)Summary by CodeRabbit
New Features
User-facing Data
Documentation
Tests
Chores