feat(vpn): auto-disconnect VPN on project and organization switch - #568
feat(vpn): auto-disconnect VPN on project and organization switch#568rAJ-2301 wants to merge 4 commits into
Conversation
When switching project or organization, the CLI now automatically disconnects the active VPN (tailscale) and cleans up /etc/hosts entries. Use --keep-vpn to suppress this when an active SSH session into a device on the previous project needs to be preserved. Power users can set auto_disconnect_vpn: false in ~/.rio-cli/config.json to permanently opt out of auto-disconnect. Fixes #2108 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🤖 Pull Request Artifacts (#31670563413) 🎉 |
There was a problem hiding this comment.
Pull request overview
Adds VPN (Tailscale) auto-disconnect behavior to the rio project select and rio organization select flows, so switching context doesn’t accidentally keep you connected to the previous project/org’s VPN environment.
Changes:
- Introduces
should_disconnect_vpn()to centralize the opt-out logic (--keep-vpnandauto_disconnect_vpn: false). - Updates project/org “select” commands to optionally disconnect Tailscale and clean up VPN
/etc/hostsentries. - Adds unit tests covering default behavior,
--keep-vpn, and config opt-out.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
riocli/vpn/util.py |
Adds should_disconnect_vpn() helper to decide whether auto-disconnect should run. |
riocli/project/select.py |
Adds --keep-vpn option and auto-disconnect + hosts cleanup logic on project switch. |
riocli/organization/select.py |
Adds --keep-vpn option and auto-disconnect + hosts cleanup logic on organization switch. |
tests/unit/vpn/test_auto_disconnect.py |
Adds unit tests for the helper and for project/org select VPN disconnect behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Skip hosts file cleanup when stop_tailscale() fails, keeping /etc/hosts intact while the VPN is still active - Update should_disconnect_vpn docstring to mention hosts cleanup is also gated by this function - Remove unused PROJECT_PATCHES and ORG_PATCHES from tests - Add test cases covering the stop-fails scenario for project and org select Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ankitrgadiya
left a comment
There was a problem hiding this comment.
Verified locally against 24d5f69:
uv run pytest tests/unit/ -v— 270 passed, including the 16 new onesuv run ruff checkandruff format --checkon all four changed files — cleanpython -c "import riocli.project.select, riocli.organization.select, riocli.vpn.util"— clean
Three things to fix before merge:
- The documented opt-out path
~/.rio-cli/config.jsonis not where the CLI reads its config, so the config-based opt-out silently does nothing for anyone who follows the docs —riocli/project/select.py:59,riocli/vpn/util.py:95, and the same string in rapyuta-robotics/rr_io_docs#105. - Both the new setting and the current project guid should be read through properties on
Configuration, not off the rawdatadict —riocli/vpn/util.py:90-99. rio project select <already-selected-project>disconnects the VPN for the project you are staying on —riocli/project/select.py:67. The organization command guards against the no-op switch; the project command does not.
Please confirm one case I cannot run here (no VPN-enabled project or registered device): stop_tailscale() shells out to sudo tailscale down and sudo tailscale logout, and run_bash_with_return_code inherits stdin (riocli/utils/__init__.py:68-80). So on a non-root shell without passwordless sudo, rio project select now blocks on a password prompt where it previously did not. Please check what that does in the nightly and CLI test runners, which call rio project select non-interactively — if tailscale happens to be up there, the command will hang rather than fail.
One thing outside this diff: rio auth login switches organization and project through riocli/auth/util.py:92 rather than through these two commands, so a login-time switch still leaves the VPN connected. If rapyuta-robotics/rapyuta_io#2108 is meant to cover that path too, it needs a follow-up.
| hosts file will be cleaned up. Use --keep-vpn to suppress this, | ||
| for example when you have an active SSH session into a device on | ||
| the previous project. You can also set ``auto_disconnect_vpn: false`` | ||
| in ~/.rio-cli/config.json to permanently suppress auto-disconnect. |
There was a problem hiding this comment.
issue (blocking): This config path does not exist, so the opt-out it documents will silently not work.
Configuration.filepath resolves to click.get_app_dir("rio-cli")/config.json (riocli/config/config.py:157-161):
$ python -c "from click import get_app_dir; print(get_app_dir('rio-cli'))"
/home/ankit/.config/rio-cli
That is ~/.config/rio-cli/config.json on Linux and ~/Library/Application Support/rio-cli/config.json on macOS, and $RIO_CONFIG overrides both. ~/.rio-cli is what get_app_dir returns with force_posix=True, which the CLI does not pass. A user who creates ~/.rio-cli/config.json gets no opt-out and no error.
| in ~/.rio-cli/config.json to permanently suppress auto-disconnect. | |
| in the CLI config file (``~/.config/rio-cli/config.json`` on Linux) | |
| to permanently suppress auto-disconnect. |
The same string is in riocli/vpn/util.py:95 and in rapyuta-robotics/rr_io_docs#105 (source/features/vpn.md:230) — all three need it. While you are in there, organization/select.py's docstring does not mention the new behaviour at all, unlike this one.
| def should_disconnect_vpn(config: dict, keep_vpn: bool) -> bool: | ||
| """Returns True if VPN should be auto-disconnected on project/org switch. | ||
|
|
||
| When True, both the VPN tunnel and the /etc/hosts cleanup are performed. | ||
| Returns False (skipping both) if --keep-vpn flag is passed, or if the | ||
| user has set auto_disconnect_vpn: false in ~/.rio-cli/config.json. | ||
| """ | ||
| if keep_vpn: | ||
| return False | ||
| return config.get("auto_disconnect_vpn", True) |
There was a problem hiding this comment.
suggestion (blocking): Expose auto_disconnect_vpn as a property on Configuration rather than reading the key out of the raw data dict here.
Every other user-overridable setting in the CLI is a class-level default plus a non-raising property on Configuration — piping_server, diff_tool, merge_tool, device_flow_client_id, oidc_server at riocli/config/config.py:199-217. This function bypasses that and reaches into config["auto_disconnect_vpn"] directly, which means the key name, the default, and the type live in riocli/vpn/util.py instead of with the rest of the config surface.
# riocli/config/config.py, next to DIFF_TOOL / MERGE_TOOL
AUTO_DISCONNECT_VPN = True
@property
def auto_disconnect_vpn(self: Configuration) -> bool:
return self.data.get("auto_disconnect_vpn", self.AUTO_DISCONNECT_VPN)Then should_disconnect_vpn either takes the Configuration object instead of a dict, or drops out entirely in favour of if not keep_vpn and ctx.obj.auto_disconnect_vpn: at the two call sites. A property is also the one place to normalize the value — right now whatever JSON holds is returned as-is, so a hand-written "auto_disconnect_vpn": "false" is truthy and silently opts the user in.
Same for the guard in the comment on riocli/project/select.py:67: read the current project guid through a property, not ctx.obj.data.get("project_id"). Note Configuration.project_guid (config.py:163-175) raises NoProjectSelected when nothing is selected, so that comparison needs a non-raising sibling rather than the existing property.
| f"{Symbols.WARNING} Failed to clean up hosts file: {str(e)}", | ||
| fg=Colors.YELLOW, | ||
| ) | ||
| if should_disconnect_vpn(ctx.obj.data, keep_vpn): |
There was a problem hiding this comment.
issue: Re-selecting the project you are already on tears down that project's VPN.
organization/select.py:95-100 returns early when organization_id already equals the target, so a no-op org switch never reaches the teardown. There is no equivalent guard here, and name_to_guid (riocli/project/util.py:27-57) resolves the name to a guid without short-circuiting, so rio project select A while already on A falls straight into this block.
Concretely: rio vpn connect --update-hosts on project A, an SSH session open to a device, then re-run rio project select A — from shell history, or from a script that re-asserts context before doing work — and you get tailscale down + tailscale logout plus the /etc/hosts entries removed, for a switch that never happened.
No inline suggestion because the fix has to straddle line 63: the previous guid has to be captured before ctx.obj.data["project_id"] overwrites it, and per the comment on riocli/vpn/util.py it should be read through a Configuration property rather than the data dict.
| vpn_was_up = is_tailscale_up() | ||
| disconnected = True | ||
| if vpn_was_up: | ||
| disconnected = stop_tailscale() |
There was a problem hiding this comment.
question (non-blocking): How should this behave when the live tailscale session is not one rio created?
is_tailscale_up() (riocli/vpn/util.py:64-66) only asks whether any tailscale session is up, and stop_tailscale() runs sudo tailscale down followed by sudo tailscale logout (riocli/vpn/util.py:73-82) — the logout invalidates the node key, so the user has to re-authenticate. Until now that only happened when someone explicitly ran rio vpn disconnect. Making it implicit on every switch means a developer with their own or a corporate Tailscale login gets logged out of it by an unrelated rio project select.
Was that considered? rio vpn connect knows the headscale login server it connected to (_TAILSCALE_CMD_FORMAT, riocli/vpn/connect.py:37-40), so gating the teardown on "this tunnel points at our control server" looks feasible, though it probably needs connect to record it. If the team's position is that any tailscale session on a rio user's machine is fair game, saying so in the docs would be enough.
| if should_disconnect_vpn(ctx.obj.data, keep_vpn): | ||
| vpn_was_up = is_tailscale_up() | ||
| disconnected = True | ||
| if vpn_was_up: | ||
| disconnected = stop_tailscale() | ||
| if disconnected: | ||
| click.secho( | ||
| f"{Symbols.SUCCESS} VPN disconnected.", | ||
| fg=Colors.GREEN, | ||
| ) | ||
| else: | ||
| click.secho( | ||
| f"{Symbols.WARNING} Failed to disconnect VPN.", | ||
| fg=Colors.YELLOW, | ||
| ) | ||
| if not vpn_was_up or disconnected: | ||
| try: | ||
| cleanup_hosts_file() | ||
| except Exception as e: | ||
| click.secho( | ||
| f"{Symbols.WARNING} Failed to clean up hosts file: {str(e)}", | ||
| fg=Colors.YELLOW, | ||
| ) |
There was a problem hiding this comment.
suggestion (non-blocking): Extract this block — it is byte-identical to riocli/project/select.py:67-89.
A disconnect_vpn_for_switch(config: Configuration, keep_vpn: bool) -> None in riocli/vpn/util.py, next to should_disconnect_vpn, would leave one call site in each command and let the unit tests target the helper directly instead of driving both commands through CliRunner. The drift risk is already visible: project/select.py documents the new behaviour in its docstring and this command does not.
| @patch("riocli.project.select.is_tailscale_up", return_value=True) | ||
| @patch("riocli.project.select.stop_tailscale", return_value=False) | ||
| @patch("riocli.project.select.cleanup_hosts_file") | ||
| def test_hosts_not_cleaned_when_stop_fails(self, mock_cleanup, mock_stop, mock_is_up): |
There was a problem hiding this comment.
praise: Good to see the failure path pinned — asserting /etc/hosts is left alone when stop_tailscale() returns False is exactly the case that would otherwise rot silently.
- Extract duplicate VPN disconnect block from project/select.py and organization/select.py into disconnect_vpn_for_switch() in vpn/util.py - Add auto_disconnect_vpn property and current_project_id property to Configuration, following the existing pattern for user-overridable settings - Guard project select against no-op switches: skip VPN lifecycle when re-selecting the already-active project - Fix documented config path (~/.rio-cli vs ~/.config/rio-cli) in docstrings - Add VPN behaviour description to organization select docstring Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When switching project or organization, the CLI now automatically disconnects the active VPN (tailscale) and cleans up /etc/hosts entries.
Use --keep-vpn to suppress this when an active SSH session into a device on the previous project needs to be preserved. Power users can set auto_disconnect_vpn: false in ~/.rio-cli/config.json to permanently opt out of auto-disconnect.
Fixes #2108