Conversation
📝 WalkthroughWalkthroughThe changes add X11 clipboard support to the run command, enabling image pasting into containers. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR adds a Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["run() called with --paste-images"] --> B{paste_images == True?}
B -- No --> E[Continue without X11]
B -- Yes --> C{DISPLAY env var set?}
C -- No --> D["warning: skipping X11 forwarding"]
C -- Yes --> F["_x11_volumes_and_env(display)"]
F --> G["volumes: /tmp/.X11-unix → /tmp/.X11-unix rw"]
F --> H["env: DISPLAY=<value>"]
G --> I["extra_volumes.extend(x11_vols)"]
H --> J["merged_env.update(x11_env)"]
I --> K["manager.run_agent(..., extra_volumes, env=merged_env)"]
J --> K
D --> E
E --> K
Last reviewed commit: 61b48e1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert ("/tmp/.X11-unix", "/tmp/.X11-unix", "rw") in captured.get( | ||
| "extra_volumes", [] | ||
| ), f"X11 socket not found in extra_volumes: {captured.get('extra_volumes')}" |
There was a problem hiding this comment.
Test does not assert DISPLAY env var is injected
test_paste_images_flag_adds_x11_volumes_and_env only verifies that the X11 socket appears in extra_volumes, but it never checks that DISPLAY is present in the env dict passed to run_agent. The captured kwargs already contain env (it is passed as env=merged_env), so the assertion can be added alongside the volume check.
Without this assertion, the merged_env.update(x11_env) line in run.py could be silently deleted and this test would still pass, leaving the container missing the DISPLAY variable entirely.
| assert ("/tmp/.X11-unix", "/tmp/.X11-unix", "rw") in captured.get( | |
| "extra_volumes", [] | |
| ), f"X11 socket not found in extra_volumes: {captured.get('extra_volumes')}" | |
| assert ("/tmp/.X11-unix", "/tmp/.X11-unix", "rw") in captured.get( | |
| "extra_volumes", [] | |
| ), f"X11 socket not found in extra_volumes: {captured.get('extra_volumes')}" | |
| assert captured.get("env", {}).get("DISPLAY") == ":0", ( | |
| f"DISPLAY not injected into container env: {captured.get('env')}" | |
| ) |
| if paste_images: | ||
| display = os.environ.get("DISPLAY", "") | ||
| if not display: | ||
| warning("--paste-images requires DISPLAY to be set; skipping X11 forwarding") | ||
| else: | ||
| x11_vols, x11_env = _x11_volumes_and_env(display) | ||
| extra_volumes.extend(x11_vols) | ||
| merged_env.update(x11_env) |
There was a problem hiding this comment.
No validation that /tmp/.X11-unix exists before mounting
DISPLAY being set does not guarantee the X11 socket directory exists on the host. Remote display values (e.g. DISPLAY=remote-host:0) are perfectly valid and will have no corresponding local socket. If /tmp/.X11-unix does not exist, Docker will either silently create an empty directory and mount it (useless for X11) or fail with an opaque error, with no helpful message to the user.
Consider checking path existence before adding the mount:
if paste_images:
display = os.environ.get("DISPLAY", "")
if not display:
warning("--paste-images requires DISPLAY to be set; skipping X11 forwarding")
elif not Path("/tmp/.X11-unix").exists():
warning("--paste-images: /tmp/.X11-unix not found; skipping X11 forwarding")
else:
x11_vols, x11_env = _x11_volumes_and_env(display)
extra_volumes.extend(x11_vols)
merged_env.update(x11_env)| def _x11_volumes_and_env(display: str) -> tuple[list[tuple[str, str, str]], dict[str, str]]: | ||
| """Return X11 socket volumes and DISPLAY env for paste-image support.""" | ||
| volumes: list[tuple[str, str, str]] = [("/tmp/.X11-unix", "/tmp/.X11-unix", "rw")] | ||
| env: dict[str, str] = {"DISPLAY": display} | ||
| return volumes, env |
There was a problem hiding this comment.
X11 socket mounted read-write grants full display access to container
Mounting /tmp/.X11-unix into the agent container with rw mode gives the container unrestricted access to the host X11 display. This goes well beyond clipboard reading — the container process can capture keystrokes from any X11 window, take screenshots, and inject synthetic input events into other applications running on the same display.
For a tool that executes untrusted or AI-generated code inside containers, this is a meaningful privilege escalation. At a minimum, the help text and/or a runtime warning should inform the user of the full scope of access being granted. Functionally, connecting to an X11 socket does require a writable file-descriptor to the socket file, so ro is not a workable alternative here; the concern is primarily one of documentation and informed consent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_run.py (1)
185-227: Consider extracting the duplicated stub class and adding DISPLAY env assertion.The
_CapturingDockerManagerclass is duplicated three times. Also, the tests verifyextra_volumesbut don't assert thatDISPLAYis passed in the container'senvdictionary whenpaste_images=True.♻️ Proposed refactor to reduce duplication and improve assertions
Extract the capturing manager and add an env assertion:
class _CapturingDockerManager: """Stub that captures run_agent kwargs.""" def __init__(self) -> None: self.captured: dict = {} def ensure_network(self, name: str) -> None: pass def networks_with_running_containers(self) -> list[str]: return [] def pull_image(self, image: str) -> None: pass def ensure_proxy(self, **kwargs) -> None: pass def run_agent(self, **kwargs) -> object: self.captured.update(kwargs) container = type( "_Container", (), { "name": "vibepod-claude-test", "id": "abc123", "status": "running", "attrs": {"NetworkSettings": {"Networks": {}}}, "reload": lambda self: None, "labels": {}, "logs": lambda self, **kw: b"", }, )() return containerThen in
test_paste_images_flag_adds_x11_volumes_and_env, add:assert captured.get("env", {}).get("DISPLAY") == ":0", "DISPLAY should be passed to container env"Also applies to: 230-271, 274-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_run.py` around lines 185 - 227, Tests duplicate the _CapturingDockerManager stub and only assert extra_volumes; refactor by extracting a single reusable _CapturingDockerManager that stores kwargs (e.g., self.captured updated in run_agent) and replace the three in-test copies with that class, and add an assertion in test_paste_images_flag_adds_x11_volumes_and_env to check that captured.get("env", {}).get("DISPLAY") == ":0" so the DISPLAY env is passed when paste_images=True; keep references to run_agent, captured, and extra_volumes when making the changes.src/vibepod/commands/run.py (1)
309-316: Consider adding X11 authentication forwarding for broader compatibility.The current implementation mounts the X11 socket and passes DISPLAY, which works for simple cases. However, many X11 setups also require
XAUTHORITYfor authentication. Without it, containers may fail to connect to the X server with "authorization required" errors.♻️ Proposed enhancement to support XAUTHORITY
In
_x11_volumes_and_env:def _x11_volumes_and_env(display: str) -> tuple[list[tuple[str, str, str]], dict[str, str]]: """Return X11 socket volumes and DISPLAY env for paste-image support.""" volumes: list[tuple[str, str, str]] = [("/tmp/.X11-unix", "/tmp/.X11-unix", "rw")] env: dict[str, str] = {"DISPLAY": display} + xauthority = os.environ.get("XAUTHORITY") + if xauthority and Path(xauthority).exists(): + volumes.append((xauthority, xauthority, "ro")) + env["XAUTHORITY"] = xauthority return volumes, env🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vibepod/commands/run.py` around lines 309 - 316, The current X11 forwarding only mounts the X11 socket and sets DISPLAY, which fails for setups that require XAUTHORITY; update the paste_images handling and the helper _x11_volumes_and_env to detect and forward XAUTHORITY: when paste_images is true and DISPLAY is set, check for XAUTHORITY env var (or default to ~/.Xauthority), if the file exists add its path to extra_volumes (bind-mount) and set merged_env["XAUTHORITY"] accordingly; ensure _x11_volumes_and_env returns both the socket volume list and any XAUTHORITY volume plus the env mapping so code that currently uses x11_vols, x11_env can mount and export the auth file in addition to DISPLAY.
🤖 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/vibepod/commands/run.py`:
- Around line 309-316: The current X11 forwarding only mounts the X11 socket and
sets DISPLAY, which fails for setups that require XAUTHORITY; update the
paste_images handling and the helper _x11_volumes_and_env to detect and forward
XAUTHORITY: when paste_images is true and DISPLAY is set, check for XAUTHORITY
env var (or default to ~/.Xauthority), if the file exists add its path to
extra_volumes (bind-mount) and set merged_env["XAUTHORITY"] accordingly; ensure
_x11_volumes_and_env returns both the socket volume list and any XAUTHORITY
volume plus the env mapping so code that currently uses x11_vols, x11_env can
mount and export the auth file in addition to DISPLAY.
In `@tests/test_run.py`:
- Around line 185-227: Tests duplicate the _CapturingDockerManager stub and only
assert extra_volumes; refactor by extracting a single reusable
_CapturingDockerManager that stores kwargs (e.g., self.captured updated in
run_agent) and replace the three in-test copies with that class, and add an
assertion in test_paste_images_flag_adds_x11_volumes_and_env to check that
captured.get("env", {}).get("DISPLAY") == ":0" so the DISPLAY env is passed when
paste_images=True; keep references to run_agent, captured, and extra_volumes
when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9571d6c-0644-44d1-945d-65adc195d7a2
📒 Files selected for processing (2)
src/vibepod/commands/run.pytests/test_run.py
1 participant
Adds support for enabling image pasting via the X11 clipboard in agent containers, controlled by a new
--paste-imagesflag. It introduces logic to mount the X11 socket and set theDISPLAYenvironment variable when this flag is enabled, and includes comprehensive tests to ensure correct behavior in various scenarios.X11 clipboard support for image pasting:
_x11_volumes_and_envhelper to return the X11 socket mount andDISPLAYenvironment variable for containerized agents.--paste-imagesflag to theruncommand, enabling X11 clipboard support for image pasting if theDISPLAYenvironment variable is set.runcommand logic to mount the X11 socket and inject theDISPLAYvariable into the container environment when--paste-imagesis used andDISPLAYis set; otherwise, a warning is shown and X11 is skipped.Testing enhancements:
_x11_volumes_and_envand for the--paste-imagesflag, covering cases whereDISPLAYis set, unset, and when the flag is not used, to ensure correct mounting and environment handling.Summary by CodeRabbit
Release Notes
--paste-imagescommand-line option to enable pasting images via X11 clipboard.