feat: add remote coasts infrastructure (Phase 0) - #2
Open
VAIBHAVSING wants to merge 10 commits into
Open
Conversation
… protocol, handlers
📝 WalkthroughWalkthroughAdds a remote development feature: new CLI commands (remote, sync), protocol types and responses for remote/sync, daemon handlers and routing, remote setup and SSH tunnel managers, Mutagen-based sync manager, DB schema and state APIs for remotes/tunnels/sync, UI badge, scripts, i18n entries, and related test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Daemon as Daemon/Handler
participant DB as Database
participant Setup as RemoteSetup
participant VM as Remote VM
CLI->>Daemon: RemoteSetupRequest(name, force)
Daemon->>DB: get_remote(name)
DB-->>Daemon: Remote config
alt coastd already installed & not forced
Daemon-->>CLI: RemoteSetupResponse(success: true)
else
Daemon->>Setup: full_setup(remote)
Setup->>VM: SSH (connectivity, docker install, download/install coastd, systemd)
VM-->>Setup: command output / exit
Setup-->>Daemon: setup result (success/version or error)
Daemon-->>CLI: RemoteSetupResponse(...)
end
sequenceDiagram
participant CLI as CLI
participant Daemon as Daemon/Handler
participant DB as Database
participant TunnelMgr as TunnelManager
participant SSH as SSH process
CLI->>Daemon: RemoteConnectRequest(name)
Daemon->>DB: get_remote(name)
DB-->>Daemon: Remote config
Daemon->>TunnelMgr: connect(remote)
alt existing tunnel
TunnelMgr-->>Daemon: ConnectResult (reuse local_port)
else new tunnel
TunnelMgr->>SSH: spawn ssh -N -L local:127.0.0.1:31415 ...
SSH-->>TunnelMgr: child started
TunnelMgr-->>Daemon: ConnectResult (local_port, tunnel_state)
end
Daemon->>DB: upsert_tunnel(tunnel_state)
DB-->>Daemon: persisted
Daemon-->>CLI: RemoteConnectResponse(connected: true, local_port)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
🧹 Nitpick comments (10)
coast-daemon/src/analytics.rs (1)
289-291: Add fixtures forRequest::RemoteandRequest::Sync.These helpers now have extra branches, but the tests in this file never construct either request type, and the hard-coded variant count is still pinned to the pre-remote surface. A small pair of assertions here would keep the command-name/context mapping from drifting silently.
Also applies to: 497-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/analytics.rs` around lines 289 - 291, Tests that assert the Request variant count and the command-name/context mapping are missing fixtures for Request::Remote and Request::Sync, so add test helpers that construct minimal instances of Request::Remote and Request::Sync (matching the shapes expected by the mapping functions) and include assertions that they appear in the command-name/context mapping; also update any hard-coded variant count assertions to include these two variants (or assert mapping.len() == Request::VARIANT_COUNT if such a constant exists) so the tests fail if new Request variants are added. Locate and update the test helpers that build Request variants (the existing fixtures for other Request::... variants) and the assertions that compare the expected number of variants or mapping length to ensure Remote and Sync are covered.coast-daemon/src/remote/tunnel.rs (2)
322-335: Read lock held across async TCP connect operation.
check_tunnel_healthholds theRwLockread guard while performing an async TCP connect, which could block other tunnel operations if the connect times out.Consider extracting the port before releasing the lock:
async fn check_tunnel_health(&self, remote_name: &str) -> bool { let port = { let tunnels = self.tunnels.read().await; tunnels.get(remote_name).map(|t| t.local_port) }; if let Some(port) = port { tokio::net::TcpStream::connect(format!("127.0.0.1:{}", port)).await.is_ok() } else { false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/remote/tunnel.rs` around lines 322 - 335, The check_tunnel_health function currently holds the RwLock read guard (self.tunnels.read()) while awaiting tokio::net::TcpStream::connect, which can block other operations; change it to grab only the local_port inside the read lock (use self.tunnels.read().await, lookup remote_name and map to tunnel.local_port), drop the guard, then perform the async TcpStream::connect using the extracted port and return its is_ok() result; reference check_tunnel_health and the tunnels RwLock/read guard to locate the change.
111-117: Port allocation never reuses ports from disconnected tunnels.The
next_portcounter increments monotonically without recycling ports from disconnected tunnels. Over time with many connect/disconnect cycles, this could exhaust the port range.Consider tracking available ports or resetting the counter when all tunnels are disconnected:
♻️ Suggested improvement
+ /// Return a port to the available pool when a tunnel disconnects. + async fn release_port(&self, _port: u16) { + // For Phase 0, we don't recycle ports. + // Future: track available ports for reuse. + }Or reset
next_portto base whentunnelsis empty indisconnect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/remote/tunnel.rs` around lines 111 - 117, The current port allocator in the block using self.next_port (symbol: next_port) never reclaims ports from closed tunnels, causing eventual exhaustion; change allocation to first check a reusable pool (e.g., a free_ports Vec/VecDeque/HashSet owned by the same struct) and pop from it if non-empty, otherwise use/increment next_port, and in the tunnel disconnect path (symbol: disconnect and tunnels) push the closed tunnel's local_port back into that free pool (or, as a simpler alternative, reset next_port to the base when tunnels is empty). Ensure updates to both next_port and the free pool are performed under the same async write lock used now to avoid races.coast-cli/src/commands/remote.rs (1)
258-294: Table column widths may truncate long hostnames.The table uses fixed widths (
{:<25}for HOST), which could truncate longer hostnames or user@host combinations. Consider dynamic column sizing or truncation with ellipsis for better display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-cli/src/commands/remote.rs` around lines 258 - 294, The HOST column in format_remotes_table currently uses a fixed width "{:<25}" which can truncate long user@host values; update format_remotes_table to compute column widths dynamically (e.g., compute max length of remote.name, host_str = format!("{}@{}", remote.user, remote.host), port, status, project_count capped to a sane maximum like 40) and use those widths in the format string, or implement a small truncate_with_ellipsis(host_str, max_width) helper and apply it to the host_str before formatting; ensure you reference the existing symbols remote.name, remote.user, remote.host, remote.port, remote.project_count, tunnel_status and replace the hardcoded "{:<25}" with the computed/truncated value so long hostnames no longer silently get cut off.coast-daemon/src/state/mod.rs (1)
196-205: Consider adding foreign key constraint tolocal_port_forwards.The
local_port_forwardstable lacks a foreign key constraint toremotes, unliketunnels,project_modes, andsync_sessions. This may leave orphaned records when a remote is deleted.If intentional (e.g., port forwards should persist independently), this is fine. Otherwise, consider adding:
FOREIGN KEY (project, instance_name) REFERENCES instances(project, name) ON DELETE CASCADE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/state/mod.rs` around lines 196 - 205, The local_port_forwards table is missing a foreign key linking (project, instance_name) to the instances table, which can leave orphaned rows when a remote/instance is removed; update the local_port_forwards definition (same area as CREATE TABLE local_port_forwards) to add a FOREIGN KEY (project, instance_name) REFERENCES instances(project, name) ON DELETE CASCADE (matching the pattern used by tunnels, project_modes, and sync_sessions) so port-forwards are removed when their instance is deleted.coast-daemon/src/remote/setup.rs (2)
340-362: Downloaded binary is not checksum-verified before installation.The coastd binary is downloaded and installed without verifying its integrity via checksum. This could be a security concern if the download is intercepted or the release server is compromised.
Consider adding checksum verification:
// Download checksum file curl -fsSL -o "$TMP.sha256" "{url}.sha256" // Verify before installing sha256sum -c "$TMP.sha256"For Phase 0, this is acceptable but should be addressed before production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/remote/setup.rs` around lines 340 - 362, The install flow builds and runs install_cmd which downloads the remote coastd binary to TMP and moves it to REMOTE_COASTD_PATH via ssh_exec without verifying integrity; update the logic around install_cmd (and the call site using ssh_exec and REMOTE_COASTD_PATH) to also download and verify a checksum before moving the binary (e.g., download a {url}.sha256 alongside the binary, run sha256sum -c or equivalent verification against the downloaded TMP, and abort/return an error if verification fails) so the installation only proceeds when checksum verification succeeds and failure is logged/reported.
264-293: Docker installation assumes sudo is passwordless.The
install_dockerfunction usessudocommands without any interactive authentication. If the remote user doesn't have passwordless sudo configured, the SSH command will fail silently or hang waiting for input (thoughBatchMode=yesshould cause immediate failure).Consider documenting this requirement or adding a check:
// Verify sudo access before attempting installation self.ssh_exec(remote, "sudo -n true").await.map_err(|_| CoastError::Remote { message: "passwordless sudo required for Docker installation".to_string(), })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/remote/setup.rs` around lines 264 - 293, The install_docker function assumes passwordless sudo but runs multiple sudo commands via ssh_exec_streaming; add an explicit pre-check using ssh_exec (e.g., call self.ssh_exec(remote, "sudo -n true").await) and if it fails return a CoastError::Remote with a clear message like "passwordless sudo required for Docker installation"; alternatively, document this requirement in the function comment and only proceed to call ssh_exec_streaming/install_script when the sudo check succeeds (references: install_docker, ssh_exec_streaming, ssh_exec, CoastError::Remote).coast-daemon/src/state/remotes.rs (1)
86-91: DateTime parse failure silently falls back toUtc::now().When parsing
created_attimestamps, if RFC3339 parsing fails, the code silently falls back toUtc::now(). This masks data corruption and produces inaccurate timestamps.Consider logging a warning on parse failure:
let created_at = DateTime::parse_from_rfc3339(&created_at_str) .map(|dt| dt.with_timezone(&Utc)) - .unwrap_or_else(|_| Utc::now()); + .unwrap_or_else(|e| { + tracing::warn!(error = %e, raw = %created_at_str, "failed to parse created_at, using current time"); + Utc::now() + });Also applies to: 126-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/state/remotes.rs` around lines 86 - 91, The parsing of created_at currently uses DateTime::parse_from_rfc3339(...).map(...).unwrap_or_else(|_| Utc::now()) which silently hides malformed timestamps; change this to explicitly handle the Err case so you log a warning (e.g. using warn! or tracing::warn!) including the offending created_at_str and any relevant row identifier, then fall back to Utc::now() only after logging; update the same pattern at the other occurrence (the block around the created_at parsing at lines ~126-129) so both use the explicit match/if let Err handling with a warning log instead of silent unwrap_or_else.coast-daemon/src/handlers/remote.rs (2)
110-162: DB mutex held across async operation inhandle_remote_list.The
dbmutex guard at line 111 is held while awaitingtm.get_tunnel_statuses().awaitat line 115 and while iterating over remotes (including thecount_projects_for_remotecall at line 135). This could cause contention.Consider fetching tunnel statuses before acquiring the DB lock, or restructuring to minimize lock hold time:
♻️ Proposed restructure
async fn handle_remote_list(state: &Arc<AppState>) -> Response { + // Get tunnel statuses first (no DB lock needed) + let tunnel_statuses = if let Some(tm) = state.tunnel_manager.as_ref() { + tm.get_tunnel_statuses().await + } else { + std::collections::HashMap::new() + }; + let db = state.db.lock().await; match db.list_remotes() { Ok(remotes) => { - let tunnel_statuses = if let Some(tm) = state.tunnel_manager.as_ref() { - tm.get_tunnel_statuses().await - } else { - std::collections::HashMap::new() - }; - let remote_infos: Vec<RemoteInfo> = remotes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/handlers/remote.rs` around lines 110 - 162, handle_remote_list holds the db mutex across an awaited call and while iterating, causing contention; change the flow to call state.db.lock().await only to call db.list_remotes() and then drop the guard immediately (capture remotes into a Vec), fetch tunnel statuses from state.tunnel_manager (tm.get_tunnel_statuses().await) outside the DB lock, and then either (a) for each remote re-acquire the DB lock briefly to call db.count_projects_for_remote(&r.name) or (b) add a batch method (e.g., db.count_projects_for_remotes(&[names])) to fetch all counts in one short DB lock—update the RemoteInfo construction to use these pre-fetched counts and the tunnel_statuses so no long-lived DB guard spans async awaits or iteration.
66-108: Persisting tunnel state after remote deletion may fail due to FK constraint.In
handle_remote_remove, the tunnel disconnect at lines 68-83 attempts to persist the disconnected tunnel state (line 74), but then the remote is deleted (line 86) which cascades to delete the tunnel record anyway (due toON DELETE CASCADE).This is not harmful but the
upsert_tunnelat line 74 is unnecessary since the tunnel row will be deleted moments later. Consider simplifying:♻️ Suggested simplification
async fn handle_remote_remove(req: RemoteRemoveRequest, state: &Arc<AppState>) -> Response { // First, disconnect any active tunnel if let Some(tunnel_manager) = state.tunnel_manager.as_ref() { match tunnel_manager.disconnect(&req.name).await { - Ok(result) => { - // Persist tunnel state if we have one - if let Some(ref tunnel_state) = result.tunnel_state { - let db = state.db.lock().await; - if let Err(e) = db.upsert_tunnel(tunnel_state) { - warn!(remote = %req.name, error = %e, "failed to update tunnel state during removal"); - } - } - } + Ok(_) => { + // Tunnel state will be cascade-deleted with the remote + } Err(e) => { warn!(remote = %req.name, error = %e, "failed to disconnect tunnel during removal"); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/handlers/remote.rs` around lines 66 - 108, The disconnect path in handle_remote_remove currently calls db.upsert_tunnel(tunnel_state) after tunnel_manager.disconnect(...) but then immediately calls db.delete_remote(&req.name) which will cascade-delete the tunnel; remove the unnecessary persistence to avoid FK/conflict: inside the Ok(result) arm of tunnel_manager.disconnect in handle_remote_remove, delete the block that locks state.db and calls db.upsert_tunnel(tunnel_state) (and its associated warn on error) so we no longer attempt to persist the tunnel state before delete_remote is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coast-cli/src/commands/remote.rs`:
- Around line 229-256: parse_connection currently uses rfind(':') and treats the
trailing segment as a port which breaks IPv6 parsing; update parse_connection to
first handle bracketed IPv6 (host_port starting with '[') by locating the
matching ']' and then optionally parsing a :port after it, and for non-bracketed
hosts avoid treating the last ':' as a port when the host contains multiple ':'
characters (i.e., an unbracketed IPv6) — only parse a port for non-bracketed
hosts when there is exactly one ':'; adjust the logic around host_port,
colon_idx and potential_port to implement these checks and return (user, host,
port) accordingly.
In `@coast-daemon/src/remote/setup.rs`:
- Around line 154-178: The current code reads child.stdout then child.stderr
sequentially (using child.stdout.take(), child.stderr.take(), and
self.report(&line)), which can deadlock if one pipe fills; change to read both
streams concurrently by spawning or joining two async tasks that iterate over
the BufReader::lines() for stdout and stderr (reporting stderr lines with the
"stderr: " prefix), and then await child.wait().await only after both reader
tasks complete; keep the existing reporting logic but run the stdout reader, the
stderr reader, and the final child.wait() in a non-blocking concurrent pattern
(e.g., tokio::spawn or tokio::join!/try_join!) so neither pipe can block the
other.
In `@coast-daemon/src/remote/tunnel.rs`:
- Around line 97-109: The returned ConnectResult currently sets ssh_pid: None
for an already-connected tunnel which is inaccurate; change the code in the
branch that checks self.get_tunnel_port(&remote.name) to look up the stored
TunnelProcess (e.g., via the in-memory store used for active tunnels such as a
map of TunnelProcess entries or a helper like
self.get_tunnel_process(&remote.name)), extract its pid (as Some(pid) when
present) and populate Tunnel.ssh_pid with that value (fall back to None only if
no TunnelProcess is found), keeping the rest of the ConnectResult/Tunnel fields
the same.
- Around line 303-316: The code creates a placeholder Tunnel with local_port: 0
when check_tunnel_health returns false, which discards the real port/metadata;
change the block in the remote_names loop (where
check_tunnel_health(&name).await is called and tunnel_state is built) to lookup
the existing tunnel entry (e.g. from self.tunnels or the in-memory state map)
for `name` and populate the new Tunnel with that entry's local_port, ssh_pid,
and connected_at, while still setting status to TunnelStatus::Disconnected; keep
creating a Tunnel only when needed and fall back to sensible defaults if the
existing state is missing.
In `@coast-daemon/src/server.rs`:
- Around line 1538-1539: The new Request::Remote and Request::Sync branches call
handlers::remote::handle_remote and handlers::remote::handle_sync directly,
bypassing begin_socket_update_operation and leaving prepare_for_update unaware
of in-flight mutating operations; change these branches to route through the
self-update gate: only bypass the gate for read-only subcommands (List, Ping,
Status) but for mutating subcommands register the appropriate
UpdateOperationKind (e.g.,
RemoteAdd/RemoteRemove/RemoteSetup/RemoteConnect/RemoteDisconnect/SyncPause/SyncResume)
before invoking the handlers, and ensure
begin_socket_update_operation/prepare_for_update see those registrations so the
active-operation registry is complete.
In `@coast-i18n/locales/es.json`:
- Line 16: The Spanish locale key "error.remote" currently contains an English
string; update the es.json entry for "error.remote" to a proper Spanish
translation (e.g., "Error remoto: %{message}") preserving the interpolation
token `%{message}` exactly so formatted error messages remain intact.
In `@coast-i18n/locales/ja.json`:
- Line 16: The ja locale has an English value for the key "error.remote";
replace the English string with a Japanese translation while preserving the
interpolation placeholder %{message} (e.g., change "error.remote": "Remote
error: %{message}" to a Japanese equivalent like "error.remote":
"リモートエラー：%{message}").
In `@coast-i18n/locales/ko.json`:
- Line 16: Translate the "error.remote" value in the ko locale to Korean while
keeping the interpolation placeholder intact: update the "error.remote" entry in
coast-i18n/locales/ko.json (key "error.remote") to a Korean string such as "원격
오류: %{message}" so user-facing errors are fully localized and %{message} remains
unchanged.
In `@coast-i18n/locales/pt.json`:
- Line 16: The "error.remote" string in the Portuguese locale is still English;
update the value for the "error.remote" key to a Portuguese translation (e.g.,
"Erro remoto: %{message}") keeping the interpolation token %{message} intact,
ensure valid JSON quoting and punctuation, and save it back into the pt locale
so runtime error messages are fully localized.
In `@coast-i18n/locales/ru.json`:
- Line 16: The localization key "error.remote" is still in English; update its
value to a Russian translation while preserving the interpolation token
%{message} (e.g., use "Удалённая ошибка: %{message}" or similar) so the key
error.remote returns fully localized Russian output without changing the
placeholder.
In `@coast-i18n/locales/zh.json`:
- Line 16: The zh locale contains an English string for the key "error.remote"
which causes mixed-language output; update the value for "error.remote" in
coast-i18n/locales/zh.json to a proper Chinese translation while preserving the
interpolation placeholder %{message} (e.g., translate to something like
"远程错误：%{message}" or another appropriate Chinese phrasing) so the key remains
identical and formatting/placeholder usage is unchanged.
---
Nitpick comments:
In `@coast-cli/src/commands/remote.rs`:
- Around line 258-294: The HOST column in format_remotes_table currently uses a
fixed width "{:<25}" which can truncate long user@host values; update
format_remotes_table to compute column widths dynamically (e.g., compute max
length of remote.name, host_str = format!("{}@{}", remote.user, remote.host),
port, status, project_count capped to a sane maximum like 40) and use those
widths in the format string, or implement a small
truncate_with_ellipsis(host_str, max_width) helper and apply it to the host_str
before formatting; ensure you reference the existing symbols remote.name,
remote.user, remote.host, remote.port, remote.project_count, tunnel_status and
replace the hardcoded "{:<25}" with the computed/truncated value so long
hostnames no longer silently get cut off.
In `@coast-daemon/src/analytics.rs`:
- Around line 289-291: Tests that assert the Request variant count and the
command-name/context mapping are missing fixtures for Request::Remote and
Request::Sync, so add test helpers that construct minimal instances of
Request::Remote and Request::Sync (matching the shapes expected by the mapping
functions) and include assertions that they appear in the command-name/context
mapping; also update any hard-coded variant count assertions to include these
two variants (or assert mapping.len() == Request::VARIANT_COUNT if such a
constant exists) so the tests fail if new Request variants are added. Locate and
update the test helpers that build Request variants (the existing fixtures for
other Request::... variants) and the assertions that compare the expected number
of variants or mapping length to ensure Remote and Sync are covered.
In `@coast-daemon/src/handlers/remote.rs`:
- Around line 110-162: handle_remote_list holds the db mutex across an awaited
call and while iterating, causing contention; change the flow to call
state.db.lock().await only to call db.list_remotes() and then drop the guard
immediately (capture remotes into a Vec), fetch tunnel statuses from
state.tunnel_manager (tm.get_tunnel_statuses().await) outside the DB lock, and
then either (a) for each remote re-acquire the DB lock briefly to call
db.count_projects_for_remote(&r.name) or (b) add a batch method (e.g.,
db.count_projects_for_remotes(&[names])) to fetch all counts in one short DB
lock—update the RemoteInfo construction to use these pre-fetched counts and the
tunnel_statuses so no long-lived DB guard spans async awaits or iteration.
- Around line 66-108: The disconnect path in handle_remote_remove currently
calls db.upsert_tunnel(tunnel_state) after tunnel_manager.disconnect(...) but
then immediately calls db.delete_remote(&req.name) which will cascade-delete the
tunnel; remove the unnecessary persistence to avoid FK/conflict: inside the
Ok(result) arm of tunnel_manager.disconnect in handle_remote_remove, delete the
block that locks state.db and calls db.upsert_tunnel(tunnel_state) (and its
associated warn on error) so we no longer attempt to persist the tunnel state
before delete_remote is executed.
In `@coast-daemon/src/remote/setup.rs`:
- Around line 340-362: The install flow builds and runs install_cmd which
downloads the remote coastd binary to TMP and moves it to REMOTE_COASTD_PATH via
ssh_exec without verifying integrity; update the logic around install_cmd (and
the call site using ssh_exec and REMOTE_COASTD_PATH) to also download and verify
a checksum before moving the binary (e.g., download a {url}.sha256 alongside the
binary, run sha256sum -c or equivalent verification against the downloaded TMP,
and abort/return an error if verification fails) so the installation only
proceeds when checksum verification succeeds and failure is logged/reported.
- Around line 264-293: The install_docker function assumes passwordless sudo but
runs multiple sudo commands via ssh_exec_streaming; add an explicit pre-check
using ssh_exec (e.g., call self.ssh_exec(remote, "sudo -n true").await) and if
it fails return a CoastError::Remote with a clear message like "passwordless
sudo required for Docker installation"; alternatively, document this requirement
in the function comment and only proceed to call
ssh_exec_streaming/install_script when the sudo check succeeds (references:
install_docker, ssh_exec_streaming, ssh_exec, CoastError::Remote).
In `@coast-daemon/src/remote/tunnel.rs`:
- Around line 322-335: The check_tunnel_health function currently holds the
RwLock read guard (self.tunnels.read()) while awaiting
tokio::net::TcpStream::connect, which can block other operations; change it to
grab only the local_port inside the read lock (use self.tunnels.read().await,
lookup remote_name and map to tunnel.local_port), drop the guard, then perform
the async TcpStream::connect using the extracted port and return its is_ok()
result; reference check_tunnel_health and the tunnels RwLock/read guard to
locate the change.
- Around line 111-117: The current port allocator in the block using
self.next_port (symbol: next_port) never reclaims ports from closed tunnels,
causing eventual exhaustion; change allocation to first check a reusable pool
(e.g., a free_ports Vec/VecDeque/HashSet owned by the same struct) and pop from
it if non-empty, otherwise use/increment next_port, and in the tunnel disconnect
path (symbol: disconnect and tunnels) push the closed tunnel's local_port back
into that free pool (or, as a simpler alternative, reset next_port to the base
when tunnels is empty). Ensure updates to both next_port and the free pool are
performed under the same async write lock used now to avoid races.
In `@coast-daemon/src/state/mod.rs`:
- Around line 196-205: The local_port_forwards table is missing a foreign key
linking (project, instance_name) to the instances table, which can leave
orphaned rows when a remote/instance is removed; update the local_port_forwards
definition (same area as CREATE TABLE local_port_forwards) to add a FOREIGN KEY
(project, instance_name) REFERENCES instances(project, name) ON DELETE CASCADE
(matching the pattern used by tunnels, project_modes, and sync_sessions) so
port-forwards are removed when their instance is deleted.
In `@coast-daemon/src/state/remotes.rs`:
- Around line 86-91: The parsing of created_at currently uses
DateTime::parse_from_rfc3339(...).map(...).unwrap_or_else(|_| Utc::now()) which
silently hides malformed timestamps; change this to explicitly handle the Err
case so you log a warning (e.g. using warn! or tracing::warn!) including the
offending created_at_str and any relevant row identifier, then fall back to
Utc::now() only after logging; update the same pattern at the other occurrence
(the block around the created_at parsing at lines ~126-129) so both use the
explicit match/if let Err handling with a warning log instead of silent
unwrap_or_else.
🪄 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: 3b4682d8-a61a-4e22-9bcc-75509caab8f5
📒 Files selected for processing (23)
coast-cli/src/commands/mod.rscoast-cli/src/commands/remote.rscoast-cli/src/lib.rscoast-core/src/error.rscoast-core/src/protocol/mod.rscoast-core/src/protocol/remote.rscoast-daemon/src/analytics.rscoast-daemon/src/handlers/mod.rscoast-daemon/src/handlers/remote.rscoast-daemon/src/lib.rscoast-daemon/src/remote/mod.rscoast-daemon/src/remote/setup.rscoast-daemon/src/remote/tunnel.rscoast-daemon/src/server.rscoast-daemon/src/state/mod.rscoast-daemon/src/state/remotes.rscoast-i18n/locales/en.jsoncoast-i18n/locales/es.jsoncoast-i18n/locales/ja.jsoncoast-i18n/locales/ko.jsoncoast-i18n/locales/pt.jsoncoast-i18n/locales/ru.jsoncoast-i18n/locales/zh.json
Comment on lines
+229
to
+256
| /// Parse a connection string in format user@host[:port]. | ||
| fn parse_connection(connection: &str) -> Result<(String, String, u16)> { | ||
| // Split on @ to get user and host:port | ||
| let parts: Vec<&str> = connection.splitn(2, '@').collect(); | ||
| if parts.len() != 2 { | ||
| bail!( | ||
| "Invalid connection format. Expected user@host[:port], got: {}", | ||
| connection | ||
| ); | ||
| } | ||
|
|
||
| let user = parts[0].to_string(); | ||
| let host_port = parts[1]; | ||
|
|
||
| // Check for port | ||
| if let Some(colon_idx) = host_port.rfind(':') { | ||
| // Could be IPv6 address or host:port | ||
| // Try parsing as port first | ||
| let potential_port = &host_port[colon_idx + 1..]; | ||
| if let Ok(port) = potential_port.parse::<u16>() { | ||
| let host = host_port[..colon_idx].to_string(); | ||
| return Ok((user, host, port)); | ||
| } | ||
| } | ||
|
|
||
| // No port specified, use default | ||
| Ok((user, host_port.to_string(), 22)) | ||
| } |
There was a problem hiding this comment.
IPv6 address parsing may fail or produce incorrect results.
The parse_connection function uses rfind(':') to detect a port, which incorrectly handles IPv6 addresses like user@[::1]:2222 or user@2001:db8::1. For example, user@::1 would parse :1 as the port attempt.
Consider bracketed IPv6 notation or explicit port handling:
♻️ Proposed fix for IPv6 support
fn parse_connection(connection: &str) -> Result<(String, String, u16)> {
// Split on @ to get user and host:port
let parts: Vec<&str> = connection.splitn(2, '@').collect();
if parts.len() != 2 {
bail!(
"Invalid connection format. Expected user@host[:port], got: {}",
connection
);
}
let user = parts[0].to_string();
let host_port = parts[1];
+ // Handle bracketed IPv6 addresses: [::1]:port or [2001:db8::1]
+ if host_port.starts_with('[') {
+ if let Some(bracket_end) = host_port.find(']') {
+ let host = host_port[1..bracket_end].to_string();
+ let remainder = &host_port[bracket_end + 1..];
+ if remainder.is_empty() {
+ return Ok((user, host, 22));
+ } else if let Some(port_str) = remainder.strip_prefix(':') {
+ let port = port_str.parse::<u16>().map_err(|_| {
+ anyhow::anyhow!("Invalid port in connection string: {}", connection)
+ })?;
+ return Ok((user, host, port));
+ }
+ }
+ bail!("Invalid bracketed IPv6 address format: {}", connection);
+ }
+
// Check for port
if let Some(colon_idx) = host_port.rfind(':') {
- // Could be IPv6 address or host:port
- // Try parsing as port first
+ // Only treat as port if there's exactly one colon (not IPv6)
+ let colon_count = host_port.matches(':').count();
+ if colon_count > 1 {
+ // Likely an unbracketed IPv6 address, use default port
+ return Ok((user, host_port.to_string(), 22));
+ }
let potential_port = &host_port[colon_idx + 1..];
if let Ok(port) = potential_port.parse::<u16>() {
let host = host_port[..colon_idx].to_string();
return Ok((user, host, port));
}
}
// No port specified, use default
Ok((user, host_port.to_string(), 22))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-cli/src/commands/remote.rs` around lines 229 - 256, parse_connection
currently uses rfind(':') and treats the trailing segment as a port which breaks
IPv6 parsing; update parse_connection to first handle bracketed IPv6 (host_port
starting with '[') by locating the matching ']' and then optionally parsing a
:port after it, and for non-bracketed hosts avoid treating the last ':' as a
port when the host contains multiple ':' characters (i.e., an unbracketed IPv6)
— only parse a port for non-bracketed hosts when there is exactly one ':';
adjust the logic around host_port, colon_idx and potential_port to implement
these checks and return (user, host, port) accordingly.
Comment on lines
+154
to
+178
| // Stream stdout | ||
| let stdout = child.stdout.take(); | ||
| if let Some(stdout) = stdout { | ||
| let reader = BufReader::new(stdout); | ||
| let mut lines = reader.lines(); | ||
| while let Ok(Some(line)) = lines.next_line().await { | ||
| self.report(&line); | ||
| } | ||
| } | ||
|
|
||
| // Stream stderr | ||
| let stderr = child.stderr.take(); | ||
| if let Some(stderr) = stderr { | ||
| let reader = BufReader::new(stderr); | ||
| let mut lines = reader.lines(); | ||
| while let Ok(Some(line)) = lines.next_line().await { | ||
| self.report(&format!("stderr: {}", line)); | ||
| } | ||
| } | ||
|
|
||
| let status = child.wait().await.map_err(|e| CoastError::Remote { | ||
| message: format!("failed to wait for SSH command: {e}"), | ||
| })?; | ||
|
|
||
| Ok(status.code().unwrap_or(-1)) |
There was a problem hiding this comment.
Sequential stdout/stderr reading may deadlock on large output.
ssh_exec_streaming reads all stdout lines, then all stderr lines, then waits. If the remote command writes enough to stderr to fill the OS pipe buffer (~64KB) while stdout is being read, the process will block and deadlock.
Consider reading both streams concurrently:
♻️ Proposed fix using tokio::join!
- // Stream stdout
- let stdout = child.stdout.take();
- if let Some(stdout) = stdout {
- let reader = BufReader::new(stdout);
- let mut lines = reader.lines();
- while let Ok(Some(line)) = lines.next_line().await {
- self.report(&line);
- }
- }
-
- // Stream stderr
- let stderr = child.stderr.take();
- if let Some(stderr) = stderr {
- let reader = BufReader::new(stderr);
- let mut lines = reader.lines();
- while let Ok(Some(line)) = lines.next_line().await {
- self.report(&format!("stderr: {}", line));
- }
- }
+ let stdout = child.stdout.take();
+ let stderr = child.stderr.take();
+
+ let stdout_task = async {
+ if let Some(stdout) = stdout {
+ let reader = BufReader::new(stdout);
+ let mut lines = reader.lines();
+ while let Ok(Some(line)) = lines.next_line().await {
+ // Note: self.report cannot be called here due to borrow
+ // Consider collecting lines or using channels
+ }
+ }
+ };
+
+ let stderr_task = async {
+ if let Some(stderr) = stderr {
+ let reader = BufReader::new(stderr);
+ let mut lines = reader.lines();
+ while let Ok(Some(line)) = lines.next_line().await {
+ // Collect or channel stderr lines
+ }
+ }
+ };
+
+ tokio::join!(stdout_task, stderr_task);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/setup.rs` around lines 154 - 178, The current code
reads child.stdout then child.stderr sequentially (using child.stdout.take(),
child.stderr.take(), and self.report(&line)), which can deadlock if one pipe
fills; change to read both streams concurrently by spawning or joining two async
tasks that iterate over the BufReader::lines() for stdout and stderr (reporting
stderr lines with the "stderr: " prefix), and then await child.wait().await only
after both reader tasks complete; keep the existing reporting logic but run the
stdout reader, the stderr reader, and the final child.wait() in a non-blocking
concurrent pattern (e.g., tokio::spawn or tokio::join!/try_join!) so neither
pipe can block the other.
Comment on lines
+97
to
+109
| if let Some(port) = self.get_tunnel_port(&remote.name).await { | ||
| info!(remote = %remote.name, port, "tunnel already connected"); | ||
| return Ok(ConnectResult { | ||
| local_port: port, | ||
| tunnel_state: Tunnel { | ||
| remote_name: remote.name.clone(), | ||
| local_port: port, | ||
| ssh_pid: None, // Already connected, pid unknown | ||
| status: TunnelStatus::Connected, | ||
| connected_at: Some(Utc::now()), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Returning ssh_pid: None for already-connected tunnel is inaccurate.
When the tunnel already exists, the returned ConnectResult has ssh_pid: None, but the SSH process IS running. This could mislead callers or cause incorrect state persistence.
Consider retrieving the actual PID from the stored TunnelProcess:
♻️ Proposed fix
if let Some(port) = self.get_tunnel_port(&remote.name).await {
info!(remote = %remote.name, port, "tunnel already connected");
+ let tunnels = self.tunnels.read().await;
+ let ssh_pid = tunnels.get(&remote.name).and_then(|t| t.child.id());
return Ok(ConnectResult {
local_port: port,
tunnel_state: Tunnel {
remote_name: remote.name.clone(),
local_port: port,
- ssh_pid: None, // Already connected, pid unknown
+ ssh_pid,
status: TunnelStatus::Connected,
connected_at: Some(Utc::now()),
},
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/tunnel.rs` around lines 97 - 109, The returned
ConnectResult currently sets ssh_pid: None for an already-connected tunnel which
is inaccurate; change the code in the branch that checks
self.get_tunnel_port(&remote.name) to look up the stored TunnelProcess (e.g.,
via the in-memory store used for active tunnels such as a map of TunnelProcess
entries or a helper like self.get_tunnel_process(&remote.name)), extract its pid
(as Some(pid) when present) and populate Tunnel.ssh_pid with that value (fall
back to None only if no TunnelProcess is found), keeping the rest of the
ConnectResult/Tunnel fields the same.
Comment on lines
+303
to
+316
| for name in remote_names { | ||
| let healthy = self.check_tunnel_health(&name).await; | ||
| let tunnel_state = if !healthy { | ||
| Some(Tunnel { | ||
| remote_name: name.clone(), | ||
| local_port: 0, // Will be updated on reconnect | ||
| ssh_pid: None, | ||
| status: TunnelStatus::Disconnected, | ||
| connected_at: None, | ||
| }) | ||
| } else { | ||
| None | ||
| }; | ||
| results.push((name, healthy, tunnel_state)); |
There was a problem hiding this comment.
Health check returns placeholder local_port: 0 for unhealthy tunnels.
When a tunnel fails the health check, the returned Tunnel has local_port: 0, losing the actual port information. This may cause issues if the state is persisted.
♻️ Proposed fix
- let tunnel_state = if !healthy {
- Some(Tunnel {
- remote_name: name.clone(),
- local_port: 0, // Will be updated on reconnect
- ssh_pid: None,
- status: TunnelStatus::Disconnected,
- connected_at: None,
- })
+ let tunnel_state = if !healthy {
+ let tunnels = self.tunnels.read().await;
+ let port = tunnels.get(&name).map(|t| t.local_port).unwrap_or(0);
+ Some(Tunnel {
+ remote_name: name.clone(),
+ local_port: port,
+ ssh_pid: None,
+ status: TunnelStatus::Disconnected,
+ connected_at: None,
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for name in remote_names { | |
| let healthy = self.check_tunnel_health(&name).await; | |
| let tunnel_state = if !healthy { | |
| Some(Tunnel { | |
| remote_name: name.clone(), | |
| local_port: 0, // Will be updated on reconnect | |
| ssh_pid: None, | |
| status: TunnelStatus::Disconnected, | |
| connected_at: None, | |
| }) | |
| } else { | |
| None | |
| }; | |
| results.push((name, healthy, tunnel_state)); | |
| for name in remote_names { | |
| let healthy = self.check_tunnel_health(&name).await; | |
| let tunnel_state = if !healthy { | |
| let tunnels = self.tunnels.read().await; | |
| let port = tunnels.get(&name).map(|t| t.local_port).unwrap_or(0); | |
| Some(Tunnel { | |
| remote_name: name.clone(), | |
| local_port: port, | |
| ssh_pid: None, | |
| status: TunnelStatus::Disconnected, | |
| connected_at: None, | |
| }) | |
| } else { | |
| None | |
| }; | |
| results.push((name, healthy, tunnel_state)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/tunnel.rs` around lines 303 - 316, The code creates a
placeholder Tunnel with local_port: 0 when check_tunnel_health returns false,
which discards the real port/metadata; change the block in the remote_names loop
(where check_tunnel_health(&name).await is called and tunnel_state is built) to
lookup the existing tunnel entry (e.g. from self.tunnels or the in-memory state
map) for `name` and populate the new Tunnel with that entry's local_port,
ssh_pid, and connected_at, while still setting status to
TunnelStatus::Disconnected; keep creating a Tunnel only when needed and fall
back to sensible defaults if the existing state is missing.
| "error.dangling_container": "プロジェクト '%{project}' のインスタンス '%{name}' に対する残存 Docker コンテナ '%{container_name}' が見つかりましたが、Coast データベースに対応するインスタンスが存在しません。以前の実行失敗や中断された削除の残りと考えられます。\nこのコンテナを削除して続行するには、--force-remove-dangling を付けて再実行してください:\n coast run %{name} --force-remove-dangling", | ||
| "error.runtime_unavailable": "ランタイム '%{runtime}' は利用できません: %{reason}", | ||
| "error.protocol": "プロトコルエラー: %{message}", | ||
| "error.remote": "Remote error: %{message}", |
There was a problem hiding this comment.
Localize error.remote for Japanese output consistency.
Line 16 is English in the ja locale, causing mixed-language error rendering.
🌐 Suggested fix
- "error.remote": "Remote error: %{message}",
+ "error.remote": "リモートエラー: %{message}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "error.remote": "Remote error: %{message}", | |
| "error.remote": "リモートエラー: %{message}", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-i18n/locales/ja.json` at line 16, The ja locale has an English value
for the key "error.remote"; replace the English string with a Japanese
translation while preserving the interpolation placeholder %{message} (e.g.,
change "error.remote": "Remote error: %{message}" to a Japanese equivalent like
"error.remote": "リモートエラー：%{message}").
| "error.dangling_container": "프로젝트 '%{project}'의 인스턴스 '%{name}'에 대한 잔여 Docker 컨테이너 '%{container_name}'이(가) 발견되었으나, Coast 데이터베이스에 해당 인스턴스가 존재하지 않습니다. 이전 실행 실패나 중단된 삭제로 인해 남은 것일 수 있습니다.\n해당 컨테이너를 제거하고 계속하려면 --force-remove-dangling 옵션을 추가하여 다시 실행하세요:\n coast run %{name} --force-remove-dangling", | ||
| "error.runtime_unavailable": "런타임 '%{runtime}'을(를) 사용할 수 없습니다: %{reason}", | ||
| "error.protocol": "프로토콜 오류: %{message}", | ||
| "error.remote": "Remote error: %{message}", |
There was a problem hiding this comment.
Localize error.remote for Korean output consistency.
Line 16 is English in the ko locale, which leads to mixed-language user-facing errors.
🌐 Suggested fix
- "error.remote": "Remote error: %{message}",
+ "error.remote": "원격 오류: %{message}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "error.remote": "Remote error: %{message}", | |
| "error.remote": "원격 오류: %{message}", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-i18n/locales/ko.json` at line 16, Translate the "error.remote" value in
the ko locale to Korean while keeping the interpolation placeholder intact:
update the "error.remote" entry in coast-i18n/locales/ko.json (key
"error.remote") to a Korean string such as "원격 오류: %{message}" so user-facing
errors are fully localized and %{message} remains unchanged.
| "error.dangling_container": "Um contêiner Docker órfão '%{container_name}' foi encontrado para a instância '%{name}' no projeto '%{project}', mas não existe registro correspondente no banco de dados Coast. Provavelmente é resíduo de uma execução anterior que falhou ou de uma remoção interrompida.\nPara removê-lo e prosseguir, execute novamente com --force-remove-dangling:\n coast run %{name} --force-remove-dangling", | ||
| "error.runtime_unavailable": "O runtime '%{runtime}' não está disponível: %{reason}", | ||
| "error.protocol": "Erro de protocolo: %{message}", | ||
| "error.remote": "Remote error: %{message}", |
There was a problem hiding this comment.
Localize error.remote for Portuguese output consistency.
Line 16 remains in English in the pt locale and will produce mixed-language errors.
🌐 Suggested fix
- "error.remote": "Remote error: %{message}",
+ "error.remote": "Erro remoto: %{message}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-i18n/locales/pt.json` at line 16, The "error.remote" string in the
Portuguese locale is still English; update the value for the "error.remote" key
to a Portuguese translation (e.g., "Erro remoto: %{message}") keeping the
interpolation token %{message} intact, ensure valid JSON quoting and
punctuation, and save it back into the pt locale so runtime error messages are
fully localized.
| "error.dangling_container": "Обнаружен висячий Docker-контейнер '%{container_name}' для экземпляра '%{name}' в проекте '%{project}', но соответствующая запись в базе данных Coast отсутствует. Вероятно, он остался после неудачного запуска или прерванного удаления.\nДля его удаления и продолжения повторите команду с --force-remove-dangling:\n coast run %{name} --force-remove-dangling", | ||
| "error.runtime_unavailable": "Среда выполнения '%{runtime}' недоступна: %{reason}", | ||
| "error.protocol": "Ошибка протокола: %{message}", | ||
| "error.remote": "Remote error: %{message}", |
There was a problem hiding this comment.
Localize error.remote for Russian output consistency.
Line 16 is still in English while this locale is Russian. This will produce mixed-language CLI/API errors.
🌐 Suggested fix
- "error.remote": "Remote error: %{message}",
+ "error.remote": "Ошибка удалённого подключения: %{message}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "error.remote": "Remote error: %{message}", | |
| "error.remote": "Ошибка удалённого подключения: %{message}", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-i18n/locales/ru.json` at line 16, The localization key "error.remote"
is still in English; update its value to a Russian translation while preserving
the interpolation token %{message} (e.g., use "Удалённая ошибка: %{message}" or
similar) so the key error.remote returns fully localized Russian output without
changing the placeholder.
| "error.dangling_container": "发现项目 '%{project}' 中实例 '%{name}' 的残留 Docker 容器 '%{container_name}'，但 Coast 数据库中不存在对应的实例记录。这可能是之前运行失败或中断删除留下的。\n要移除该容器并继续，请使用 --force-remove-dangling 重新运行：\n coast run %{name} --force-remove-dangling", | ||
| "error.runtime_unavailable": "运行时 '%{runtime}' 不可用：%{reason}", | ||
| "error.protocol": "协议错误：%{message}", | ||
| "error.remote": "Remote error: %{message}", |
There was a problem hiding this comment.
Localize error.remote for Chinese output consistency.
Line 16 uses English text in the zh locale and will surface mixed-language errors.
🌐 Suggested fix
- "error.remote": "Remote error: %{message}",
+ "error.remote": "远程错误：%{message}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "error.remote": "Remote error: %{message}", | |
| "error.remote": "远程错误：%{message}", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-i18n/locales/zh.json` at line 16, The zh locale contains an English
string for the key "error.remote" which causes mixed-language output; update the
value for "error.remote" in coast-i18n/locales/zh.json to a proper Chinese
translation while preserving the interpolation placeholder %{message} (e.g.,
translate to something like "远程错误：%{message}" or another appropriate Chinese
phrasing) so the key remains identical and formatting/placeholder usage is
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
coast-daemon/src/remote/setup.rs (1)
154-176:⚠️ Potential issue | 🟠 MajorDrain stdout and stderr concurrently in
ssh_exec_streaming().Lines 154-172 still read stdout to EOF before touching stderr. If a remote command fills stderr first, the child can block on a full pipe and
coast remote setuphangs indefinitely.install_docker()already uses this helper, so this is user-facing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/remote/setup.rs` around lines 154 - 176, The stdout/stderr readers in ssh_exec_streaming() must be run concurrently so one full pipe cannot block the other; change the sequential reads into two concurrent tasks that drain each pipe and forward lines back to the main context, then await both tasks before calling child.wait(). To avoid borrowing self across tasks, create an mpsc channel, spawn one async task to read BufReader::lines() from child.stdout.take() and another from child.stderr.take() (prefix stderr lines as needed) sending each line into the channel, then in the main task receive from that channel and call self.report(...) for each message; finally await both reader tasks and then call child.wait().coast-cli/src/commands/remote.rs (1)
235-260:⚠️ Potential issue | 🟠 MajorHandle IPv6 and malformed
:portsuffixes explicitly.The last-colon heuristic still misparses bracketed IPv6 literals and silently accepts bad suffixes like
user@host:notaportas part of the hostname. That letscoast remote addpersist invalid connection data instead of failing fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-cli/src/commands/remote.rs` around lines 235 - 260, The parse_connection function must explicitly handle bracketed IPv6 and reject non-numeric port suffixes: if host_port starts with '[' find the matching ']' and, if a ":" follows, parse the substring after ']' as u16 (bail! on parse failure); otherwise treat the bracketed content as the host and use default port 22. If host_port does not start with '[', check whether it contains multiple ':' characters (unbracketed IPv6) and in that case do not treat the last ':' as a port separator—return (user, host_port, 22). For the normal host:port case (no brackets and single ':'), parse potential_port as u16 and bail! on parse failure; keep using the existing parse_connection signature and bail macro for errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coast-daemon/src/handlers/remote.rs`:
- Around line 337-339: The current handlers call
db.upsert_tunnel(&result.tunnel_state) and only log a warning on Err, which lets
a successful connect/disconnect reply be returned while DB persistence failed;
change both occurrences (the upsert_tunnel calls around the connect and
disconnect flows in remote.rs) to propagate the error instead of swallowing
it—on Err(e) return an appropriate error Response/Result (or map the DB error
into the handler's error type) so the RPC returns failure when upsert_tunnel
fails, ensuring the live tunnel state and DB remain consistent.
- Around line 181-199: The early return in the setup flow uses
setup.check_coastd(&remote).await to detect the presence of the coastd binary
and short-circuits setup; change this to verify the runtime/service health
before returning: after Ok(Some(version)) call
setup.verify_coastd(&remote).await (or check the service status) and only return
the RemoteSetupResponse success path when verify_coastd() indicates the service
is running/healthy; otherwise proceed with the repair/install path. Ensure the
logic still respects req.force and preserve the error logging branch that warns
on check_coastd errors.
In `@coast-daemon/src/remote/setup.rs`:
- Around line 350-357: The code uses a hard-coded placeholder URL when
binary_url is None (the let url = binary_url... block), causing
handle_remote_setup() to call full_setup(&remote, None) with an
environment-specific artifact source; instead, change the fallback behavior so
the default URL is derived from the configured release channel or fail fast:
update the logic around binary_url and url (and any use of arch_suffix) so that
when no explicit binary_url is provided you either compute the URL from the
application's release channel configuration (and pass that channel into
full_setup rather than None) or return an error to prompt the caller to supply a
binary_url; ensure handle_remote_setup() / full_setup(&remote, None) are updated
to accept and propagate the chosen channel/URL rather than relying on the
hard-coded placeholder.
In `@coast-daemon/src/remote/tunnel.rs`:
- Around line 275-298: The shutdown path never calls
TunnelManager::disconnect_all, so active tunnels are not reconciled to
Disconnected; update the daemon shutdown logic that currently only stores the
manager (the code in server shutdown) to call manager.disconnect_all().await,
collect the returned Vec<Tunnel>, and persist or apply those Tunnel states (or
update the state store) before exit; ensure you await the future, handle and log
any errors from disconnect_all, and keep the existing manager variable and
Tunnel type names (disconnect_all, disconnect, Tunnel) to locate the changes.
- Around line 95-117: The connect() method races because get_tunnel_port() and
the later allocation/insert are separated by awaits, allowing two callers to
allocate different local_port for the same remote.name; fix by reserving the
remote atomically before spawning SSH: acquire a write-side reservation in the
tunnels map (or a per-remote async mutex) inside connect() immediately after the
initial get_tunnel_port() check, allocate next_port and insert a placeholder
Tunnel (e.g., status Pending/Connecting with the reserved local_port and ssh_pid
None) into the same tunnels collection, then release and continue to spawn the
SSH process and update that Tunnel entry on success/failure; reference
connect(), get_tunnel_port(), next_port, the tunnels map insert/update and the
SSH spawn/update logic so the reservation is done under the same async lock to
make connect idempotent.
---
Duplicate comments:
In `@coast-cli/src/commands/remote.rs`:
- Around line 235-260: The parse_connection function must explicitly handle
bracketed IPv6 and reject non-numeric port suffixes: if host_port starts with
'[' find the matching ']' and, if a ":" follows, parse the substring after ']'
as u16 (bail! on parse failure); otherwise treat the bracketed content as the
host and use default port 22. If host_port does not start with '[', check
whether it contains multiple ':' characters (unbracketed IPv6) and in that case
do not treat the last ':' as a port separator—return (user, host_port, 22). For
the normal host:port case (no brackets and single ':'), parse potential_port as
u16 and bail! on parse failure; keep using the existing parse_connection
signature and bail macro for errors.
In `@coast-daemon/src/remote/setup.rs`:
- Around line 154-176: The stdout/stderr readers in ssh_exec_streaming() must be
run concurrently so one full pipe cannot block the other; change the sequential
reads into two concurrent tasks that drain each pipe and forward lines back to
the main context, then await both tasks before calling child.wait(). To avoid
borrowing self across tasks, create an mpsc channel, spawn one async task to
read BufReader::lines() from child.stdout.take() and another from
child.stderr.take() (prefix stderr lines as needed) sending each line into the
channel, then in the main task receive from that channel and call
self.report(...) for each message; finally await both reader tasks and then call
child.wait().
🪄 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: e678149b-0fd5-4e12-869e-7bc2e03dc811
📒 Files selected for processing (4)
coast-cli/src/commands/remote.rscoast-daemon/src/handlers/remote.rscoast-daemon/src/remote/setup.rscoast-daemon/src/remote/tunnel.rs
Comment on lines
+181
to
+199
| // Check if coastd is already installed (unless force is set) | ||
| let setup = RemoteSetup::new(); | ||
| if !req.force { | ||
| match setup.check_coastd(&remote).await { | ||
| Ok(Some(version)) => { | ||
| return Response::Remote(RemoteResponse::Setup(RemoteSetupResponse { | ||
| success: true, | ||
| version: Some(version), | ||
| message: "coastd is already installed".to_string(), | ||
| })); | ||
| } | ||
| Ok(None) => { | ||
| // Not installed, continue with setup | ||
| } | ||
| Err(e) => { | ||
| warn!(remote = %req.name, error = %e, "failed to check coastd status"); | ||
| // Continue with setup attempt | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't short-circuit setup on binary presence alone.
check_coastd() only proves /usr/local/bin/coastd exists. If the service is stopped or unhealthy, coast remote setup returns success here and skips the repair path. Gate the early return on verify_coastd() (or service status) so setup stays idempotent.
Proposed fix
if !req.force {
match setup.check_coastd(&remote).await {
Ok(Some(version)) => {
- return Response::Remote(RemoteResponse::Setup(RemoteSetupResponse {
- success: true,
- version: Some(version),
- message: "coastd is already installed".to_string(),
- }));
+ if setup.verify_coastd(&remote).await.unwrap_or(false) {
+ return Response::Remote(RemoteResponse::Setup(RemoteSetupResponse {
+ success: true,
+ version: Some(version),
+ message: "coastd is already installed".to_string(),
+ }));
+ }
+ warn!(remote = %req.name, "coastd binary exists but health check failed; continuing with setup");
}
Ok(None) => {
// Not installed, continue with setup
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check if coastd is already installed (unless force is set) | |
| let setup = RemoteSetup::new(); | |
| if !req.force { | |
| match setup.check_coastd(&remote).await { | |
| Ok(Some(version)) => { | |
| return Response::Remote(RemoteResponse::Setup(RemoteSetupResponse { | |
| success: true, | |
| version: Some(version), | |
| message: "coastd is already installed".to_string(), | |
| })); | |
| } | |
| Ok(None) => { | |
| // Not installed, continue with setup | |
| } | |
| Err(e) => { | |
| warn!(remote = %req.name, error = %e, "failed to check coastd status"); | |
| // Continue with setup attempt | |
| } | |
| } | |
| // Check if coastd is already installed (unless force is set) | |
| let setup = RemoteSetup::new(); | |
| if !req.force { | |
| match setup.check_coastd(&remote).await { | |
| Ok(Some(version)) => { | |
| if setup.verify_coastd(&remote).await.unwrap_or(false) { | |
| return Response::Remote(RemoteResponse::Setup(RemoteSetupResponse { | |
| success: true, | |
| version: Some(version), | |
| message: "coastd is already installed".to_string(), | |
| })); | |
| } | |
| warn!(remote = %req.name, "coastd binary exists but health check failed; continuing with setup"); | |
| } | |
| Ok(None) => { | |
| // Not installed, continue with setup | |
| } | |
| Err(e) => { | |
| warn!(remote = %req.name, error = %e, "failed to check coastd status"); | |
| // Continue with setup attempt | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/handlers/remote.rs` around lines 181 - 199, The early return
in the setup flow uses setup.check_coastd(&remote).await to detect the presence
of the coastd binary and short-circuits setup; change this to verify the
runtime/service health before returning: after Ok(Some(version)) call
setup.verify_coastd(&remote).await (or check the service status) and only return
the RemoteSetupResponse success path when verify_coastd() indicates the service
is running/healthy; otherwise proceed with the repair/install path. Ensure the
logic still respects req.force and preserve the error logging branch that warns
on check_coastd errors.
Comment on lines
+337
to
+339
| if let Err(e) = db.upsert_tunnel(&result.tunnel_state) { | ||
| warn!(remote = %req.name, error = %e, "failed to persist tunnel state"); | ||
| } |
There was a problem hiding this comment.
Don't treat tunnel-state persistence as best-effort.
If upsert_tunnel() fails in these paths, the handler still returns a successful connect/disconnect response. That leaves the live tunnel state and the database out of sync for a feature whose contract is state-backed remote management.
Also applies to: 373-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/handlers/remote.rs` around lines 337 - 339, The current
handlers call db.upsert_tunnel(&result.tunnel_state) and only log a warning on
Err, which lets a successful connect/disconnect reply be returned while DB
persistence failed; change both occurrences (the upsert_tunnel calls around the
connect and disconnect flows in remote.rs) to propagate the error instead of
swallowing it—on Err(e) return an appropriate error Response/Result (or map the
DB error into the handler's error type) so the RPC returns failure when
upsert_tunnel fails, ensuring the live tunnel state and DB remain consistent.
Comment on lines
+350
to
+357
| // Determine download URL | ||
| let url = binary_url.map(|s| s.to_string()).unwrap_or_else(|| { | ||
| // Default to a placeholder - in production this would be a real release URL | ||
| format!( | ||
| "https://github.com/anomalyco/coast/releases/latest/download/coastd-linux-{}", | ||
| arch_suffix | ||
| ) | ||
| }); |
There was a problem hiding this comment.
The default coastd download URL is still a placeholder.
When binary_url is None, this falls back to a hard-coded placeholder release URL, and handle_remote_setup() calls full_setup(&remote, None) on Line 203 in coast-daemon/src/handlers/remote.rs. That makes the normal coast remote setup path depend on an environment-specific artifact source instead of a configured release channel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/setup.rs` around lines 350 - 357, The code uses a
hard-coded placeholder URL when binary_url is None (the let url = binary_url...
block), causing handle_remote_setup() to call full_setup(&remote, None) with an
environment-specific artifact source; instead, change the fallback behavior so
the default URL is derived from the configured release channel or fail fast:
update the logic around binary_url and url (and any use of arch_suffix) so that
when no explicit binary_url is provided you either compute the URL from the
application's release channel configuration (and pass that channel into
full_setup rather than None) or return an error to prompt the caller to supply a
binary_url; ensure handle_remote_setup() / full_setup(&remote, None) are updated
to accept and propagate the chosen channel/URL rather than relying on the
hard-coded placeholder.
Comment on lines
+95
to
+117
| pub async fn connect(&self, remote: &Remote) -> Result<ConnectResult> { | ||
| // Check if already connected | ||
| if let Some(port) = self.get_tunnel_port(&remote.name).await { | ||
| info!(remote = %remote.name, port, "tunnel already connected"); | ||
| return Ok(ConnectResult { | ||
| local_port: port, | ||
| tunnel_state: Tunnel { | ||
| remote_name: remote.name.clone(), | ||
| local_port: port, | ||
| ssh_pid: None, // Already connected, pid unknown | ||
| status: TunnelStatus::Connected, | ||
| connected_at: Some(Utc::now()), | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| // Allocate a local port | ||
| let local_port = { | ||
| let mut next = self.next_port.write().await; | ||
| let port = *next; | ||
| *next += 1; | ||
| port | ||
| }; |
There was a problem hiding this comment.
Serialize connect() per remote.
The "already connected" check and the later insert are separated by several awaits, so two callers can both create tunnels for the same remote.name and return different local_ports. Reserve the remote atomically before spawning SSH so connect stays idempotent.
Also applies to: 204-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/tunnel.rs` around lines 95 - 117, The connect()
method races because get_tunnel_port() and the later allocation/insert are
separated by awaits, allowing two callers to allocate different local_port for
the same remote.name; fix by reserving the remote atomically before spawning
SSH: acquire a write-side reservation in the tunnels map (or a per-remote async
mutex) inside connect() immediately after the initial get_tunnel_port() check,
allocate next_port and insert a placeholder Tunnel (e.g., status
Pending/Connecting with the reserved local_port and ssh_pid None) into the same
tunnels collection, then release and continue to spawn the SSH process and
update that Tunnel entry on success/failure; reference connect(),
get_tunnel_port(), next_port, the tunnels map insert/update and the SSH
spawn/update logic so the reservation is done under the same async lock to make
connect idempotent.
Comment on lines
+275
to
+298
| /// Disconnect all tunnels. | ||
| /// | ||
| /// Returns a list of tunnel states to persist. | ||
| pub async fn disconnect_all(&self) -> Vec<Tunnel> { | ||
| let remote_names: Vec<String> = { | ||
| let tunnels = self.tunnels.read().await; | ||
| tunnels.keys().cloned().collect() | ||
| }; | ||
|
|
||
| let mut states = Vec::new(); | ||
| for name in remote_names { | ||
| match self.disconnect(&name).await { | ||
| Ok(result) => { | ||
| if let Some(state) = result.tunnel_state { | ||
| states.push(state); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| error!(remote = %name, "failed to disconnect tunnel: {e}"); | ||
| } | ||
| } | ||
| } | ||
| states | ||
| } |
There was a problem hiding this comment.
Wire disconnect_all() into daemon shutdown.
This helper is the only bulk cleanup path, but nothing in the provided lifecycle calls it (coast-daemon/src/server.rs:183-186 only stores the manager). On daemon exit, active tunnel state is never reconciled back to Disconnected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/tunnel.rs` around lines 275 - 298, The shutdown path
never calls TunnelManager::disconnect_all, so active tunnels are not reconciled
to Disconnected; update the daemon shutdown logic that currently only stores the
manager (the code in server shutdown) to call manager.disconnect_all().await,
collect the returned Vec<Tunnel>, and persist or apply those Tunnel states (or
update the state store) before exit; ensure you await the future, handle and log
any errors from disconnect_all, and keep the existing manager variable and
Tunnel type names (disconnect_all, disconnect, Tunnel) to locate the changes.
VAIBHAVSING
commented
Mar 28, 2026
VAIBHAVSING
left a comment
VAIBHAVSING
left a comment
Owner
Author
There was a problem hiding this comment.
I reviewed the PR and found a few correctness issues worth fixing before merge:
coast-daemon/src/remote/setup.rs:verify_coastd()probeshttp://127.0.0.1:31415/health, but the daemon does not expose a/healthroute. As written,coast remote setupverification andcoast remote pingwill report a healthy remote daemon as down.coast-daemon/src/handlers/remote.rs:handle_remote_add()usesdb.upsert_remote(&remote), socoast remote add <name> ...silently overwrites an existing remote instead of rejecting duplicates. For anaddcommand this is surprising and loses config accidentally.coast-cli/src/commands/remote.rs:parse_connection()accepts malformed inputs too easily. Examples like@host,user@, anduser@host:notaportcurrently get stored instead of failing fast, which pushes input errors into later SSH/setup flows.coast-daemon/src/remote/setup.rs:install_coastd()still falls back to a placeholder release URL whenbinary_urlisNone, andhandle_remote_setup()callsfull_setup(&remote, None). That means the default setup path is not actually wired to a real artifact source.coast-daemon/src/server.rs:Request::RemoteandRequest::Syncare dispatched directly and bypass the existing self-update operation gate. That leavesprepare_for_updateunaware of in-flight remote setup/connect/disconnect work.
Add complete Mutagen-based file synchronization for Remote Coasts: Core Implementation: - MutagenManager for sync session lifecycle (create, pause, resume, flush, terminate) - One-way sync (local → remote) with 'one-way-safe' mode - Session naming: coast-<project>-<branch>-<remote> - Remote workspace path: ~/coast-workspaces/<project>/<branch>/ - Support for .coastignore patterns (node_modules, .git, *.log) - State persistence in sync_sessions table CLI Commands: - coast sync create <project> <remote> --local-path <path> --branch <branch> - coast sync status [project] - list active sync sessions - coast sync pause/resume <project> - control sync - coast sync flush <project> - force immediate sync - coast sync terminate <project> - stop and remove session Protocol: - SyncCreateRequest/Response with local_path, branch, remote - SyncStatusRequest/Response with session info - SyncPauseRequest/Response, SyncResumeRequest/Response - SyncFlushRequest/Response, SyncTerminateRequest/Response Testing: - Comprehensive test scripts (test_sync_simple.sh, test_sync_quick.sh) - Setup automation (setup_test.sh, install_mutagen.sh) - Detailed testing guides (QUICK_TEST.md, TEST_SYNC.md) - Tests: initial sync, real-time updates, ignore patterns, pause/resume Technical Details: - Mutagen ~100MB on local, ~10MB agent on remote (auto-installed) - Uses SSH for transport with optional key configuration - Sync status tracking: Initial, Syncing, Paused, Error - Handler integration in handlers/remote.rs - Analytics support for all sync operations Phase 2 complete - workspace sync fully functional. Next: Phase 3 (remote build/run), Phase 4 (remote exec/logs)
- Installs Docker, Docker Compose, and Mutagen on remote VM - Builds coastd-dev binary on remote (avoids glibc mismatch) - Starts daemon and creates workspace directories - Supports manual remote development environment setup
Add remote daemon client and request forwarding infrastructure: - Create RemoteDaemonClient for communicating with remote daemons over SSH tunnels (coast-daemon/src/remote/client.rs) - Add get_remote_route() to check project mode and tunnel status - Add ensure_synced() to flush Mutagen before remote operations - Add forward_streaming_to_remote() for proxying streaming responses - Modify handle_build_streaming() to route to remote when configured - Modify handle_run_streaming() to route to remote when configured Flow: CLI -> Local Daemon -> Check Mode -> Sync Flush -> Forward via SSH Tunnel -> Remote Daemon -> Proxy responses back Also includes Phase 2 fixes: - Fix get_sync_session method name in handlers - Fix truncate_str test assertion - Clean up unused imports
Add remote_name field to CoastInstance to track which remote an instance is running on. This enables routing exec and logs requests to the correct remote daemon based on instance location. Key changes: - Add remote_name: Option<String> to CoastInstance struct - Add DB migration for remote_name column in instances table - Add get_instance_remote_route() to look up instance's remote location - Add forward_to_remote() for non-streaming requests (exec, logs) - Add forward_logs_streaming_to_remote() for streaming logs - Modify handle_connection to route exec/logs based on instance location - Update forward_run_to_remote to create shadow instance records locally with remote_name set, enabling future exec/logs routing - Fix all test files to include remote_name: None field Phase 4 routes based on instance location (remote_name field), unlike Phase 3 which routes based on project mode.
feat: implement workspace sync with Mutagen (Phase 2)
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (2)
coast-daemon/src/handlers/remote.rs (2)
186-195:⚠️ Potential issue | 🟠 MajorDon't report setup success on binary presence alone.
Line 189 only proves the binary exists. If the systemd unit is stopped or the health check is failing,
coast remote setupstill returns success here and skips the repair path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/handlers/remote.rs` around lines 186 - 195, The current early return on Ok(Some(version)) from setup.check_coastd only proves the binary exists and incorrectly reports success; update the logic so that after RemoteSetup::new() and setup.check_coastd(&remote).await returning Some(version) you additionally verify the service is active and healthy (e.g., call or add methods like RemoteSetup::is_systemd_active(&remote) and RemoteSetup::run_health_check(&remote) or extend check_coastd to return a richer status enum), and only return RemoteSetupResponse.success=true when both the systemd unit is active and the health check passes; if either check fails, fall through to the repair/setup path instead of returning success.
335-341:⚠️ Potential issue | 🟠 MajorReturn failure when tunnel-state persistence fails.
Both paths log
upsert_tunnel()failures and still report success. That leaves the live tunnel state and the persisted state divergent, which will break later status/reconnect behavior after a daemon restart.Also applies to: 372-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/handlers/remote.rs` around lines 335 - 341, The current logic in the tunnel setup (inside the match handling tunnel_manager.connect) logs failures from db.upsert_tunnel(&result.tunnel_state) but continues to treat the overall operation as success; change this so that when db.upsert_tunnel returns Err you propagate/return an error response (or Err from the handler) instead of proceeding. Locate the block that calls state.db.lock().await and db.upsert_tunnel(&result.tunnel_state) (and the similar block at the second occurrence) and convert the if let Err(e) = ... { warn!(...); } into an early return that returns an appropriate failure result containing the error (or maps it into the handler's response type) so tunnel persistence failures are reported to the caller.
🧹 Nitpick comments (4)
simple_deploy.sh (1)
53-90: Verify that the VM actually needs a full Mutagen install.For SSH endpoints, Mutagen copies agent binaries to the remote over
scp/sshand doesn’t require a manual install on the remote endpoint. Unlesscoastd-devitself shells out tomutagenon the VM, this whole step is just extra deployment time and one more version to keep aligned. (mutagen.io)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simple_deploy.sh` around lines 53 - 90, The current install block in simple_deploy.sh (the logic that checks command -v mutagen and then downloads/install MUTAGEN_VERSION into ~/.local/bin and agents into ~/.local/libexec) may be unnecessary for SSH endpoints because Mutagen can copy agent binaries over ssh/scp and doesn’t require a full remote install; update the script to first detect whether the deployment target actually runs mutagen locally (e.g., whether coastd-dev or any remote process will invoke the mutagen binary on the VM) and skip the full install if not needed: replace or gate the existing install branch (the command -v mutagen check and subsequent install steps, including MUTAGEN_VERSION, ~/.local/bin, and ~/.local/libexec handling) with a conditional that only installs when remote runs mutagen itself, otherwise rely on mutagen’s agent transfer behavior or document why a full install is required.coast-core/src/types/tests.rs (1)
155-155: LGTM!Test fixture correctly updated to include
remote_name: Nonefor the serialization round-trip test.Consider adding an explicit assertion for
remote_nameto make the test more comprehensive:assert_eq!(deserialized.runtime, RuntimeType::Dind); assert_eq!(deserialized.commit_sha, Some("abc123def456".to_string())); + assert_eq!(deserialized.remote_name, None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-core/src/types/tests.rs` at line 155, The test fixture was updated to include remote_name: None but lacks an explicit assertion; update the serialization round-trip test (the test that constructs the fixture and deserializes it) to assert that the deserialized object's remote_name field is None (e.g., assert_eq!(deserialized.remote_name, None)) so the test verifies the field is preserved through serialize/deserialize; locate the variable holding the deserialized value (commonly named deserialized, round_trip, or result) and add the assertion there.coast-core/src/protocol/tests.rs (1)
597-620: Add one roundtrip case withremote_name: Some(...).This update covers the
Nonepath, but adding one non-empty value would guard remote metadata serialization regressions.🧪 Suggested test addition
+#[test] +fn test_ls_response_roundtrip_with_remote_name() { + roundtrip_response(Response::Ls(LsResponse { + instances: vec![InstanceSummary { + name: "main".to_string(), + project: "my-app".to_string(), + status: InstanceStatus::Running, + branch: Some("main".to_string()), + runtime: RuntimeType::Dind, + checked_out: true, + project_root: None, + worktree: None, + build_id: None, + coastfile_type: None, + port_count: 1, + primary_port_service: None, + primary_port_canonical: None, + primary_port_dynamic: None, + primary_port_url: None, + down_service_count: 0, + remote_name: Some("remote-dev-1".to_string()), + }], + known_projects: Vec::new(), + })); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-core/src/protocol/tests.rs` around lines 597 - 620, Add a second roundtrip case in test_ls_response_roundtrip that covers serialization when InstanceSummary.remote_name is Some(...): duplicate the existing Response::Ls(LsResponse { instances: vec![InstanceSummary { ... }], ... }) entry but set remote_name: Some("origin".to_string()) (or similar non-empty string) and run roundtrip_response on it; modify the test to include both the original None case and this new Some(...) case so LsResponse/InstanceSummary remote metadata serialization is exercised.coast-daemon/src/handlers/ls.rs (1)
244-323: Add oneremote_name: Some(...)assertion inlshandler tests.Fixtures were updated, but a positive-path assertion would better lock in remote listing behavior.
🧪 Suggested test tweak
db.insert_instance(&CoastInstance { name: "feat-a".to_string(), project: "my-app".to_string(), @@ - remote_name: None, + remote_name: Some("remote-1".to_string()), }) .unwrap(); @@ let checked_out = result @@ .unwrap(); +let remote = result.instances.iter().find(|i| i.name == "feat-a").unwrap(); +assert_eq!(remote.remote_name.as_deref(), Some("remote-1"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coast-daemon/src/handlers/ls.rs` around lines 244 - 323, Tests for the ls handler lack a positive assertion for remote_name after fixtures were updated; update the test setup in test_ls_multiple_instances by setting remote_name: Some("origin".to_string()) (or similar) on one of the inserted CoastInstance fixtures (identify the CoastInstance structs inserted in test_ls_multiple_instances) and then after calling handle(req, &state).await assert that the returned result.instances for the corresponding instance contains remote_name == Some("origin".to_string()); this ensures the Ls handler (handle and LsRequest) is verified to return remote_name correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coast-cli/src/commands/sync.rs`:
- Around line 205-210: The current truncation slices strings by byte offsets
(path_display logic and truncate_str) which panics on multi-byte UTF-8
characters; change both to use character-aware truncation (operate on .chars())
so you count characters and take the last N characters safely (e.g., build the
tail with chars().rev().take(...) then reverse/collect and prefix "..."). Update
the session.local_path truncation (path_display) and the truncate_str function
to use this char-based approach rather than byte indexing.
In `@coast-daemon/src/handlers/remote.rs`:
- Around line 116-123: You are holding the DB mutex across an await: the call
sequence around state.db.lock().await -> db.list_remotes() currently keeps the
lock while awaiting tm.get_tunnel_statuses().await. To fix, acquire the lock,
call db.list_remotes(), clone or extract any needed data from that result, then
drop the mutex (let db_guard go out of scope) before checking
state.tunnel_manager and awaiting tm.get_tunnel_statuses().await; reference the
symbols state.db.lock().await, db.list_remotes(), state.tunnel_manager and
tm.get_tunnel_statuses().await to locate the code to change.
- Around line 418-429: The filter is currently using contains(project) and
reconstructing names with a hard-coded "main", which causes incorrect/ambiguous
matches and loses non-main branches; update the logic to use the exact persisted
session identifier instead: when building SyncSessionInfo (and in the other
similar blocks at the noted ranges) read and match the actual session_name (or
branch) stored in the DB record rather than using contains(project) or
generate_session_name(..., "main", ...); change the filter to compare
s.name.as_ref().map_or(false, |n| n == &db_session.session_name) (or compare
branch fields exactly) and ensure MutagenManager::generate_session_name is
called using the persisted branch from db_sessions (not a hard-coded "main") so
sessions created by handle_sync_create() remain addressable after restart.
In `@coast-daemon/src/remote/mutagen.rs`:
- Around line 319-330: The SyncSession struct and session persistence need to
include branch so sessions for non-main branches survive restarts: add a branch:
String field to SyncSession, set it when constructing the SyncSession in
create_session (use the incoming branch parameter so
mutagen_session_id/remote_path etc. are persisted together with branch), update
any DB save/serialize code to persist the new branch column, and change
load_sessions_from_db to read the persisted branch value (instead of hardcoding
"main") when reconstructing the in-memory session key and SessionInfo so the
session_name uses coast-<project>-<branch>-<remote> and lookups work correctly.
- Around line 335-357: get_session_id_by_name currently calls mutagen with
--label-selector (which searches labels) so it never finds sessions created with
--name; change the Command invocation in get_session_id_by_name to pass the
session name as a positional argument to `mutagen sync list` (mirroring the
pattern used in pause/resume/terminate) rather than using "--label-selector",
and then parse the JSON output as before; also ensure related callers/getters
(get_session_status and list_all_sessions) follow the correct pattern—use
positional name arguments when querying a single session and for
list_all_sessions call `mutagen sync list -o json` (no name filter) and filter
the returned sessions in Rust by session.name.starts_with("coast-") to implement
prefix filtering.
In `@coast-daemon/src/remote/setup.rs`:
- Around line 353-378: The code interpolates binary_url (via the local variable
url) directly into install_cmd which is executed over SSH, allowing shell
injection; before building install_cmd, shell-quote/escape the URL (e.g. use
shlex::quote or shell_escape::escape) and use that escaped value when
constructing install_cmd so special chars like quotes/backticks/$(...) cannot be
interpreted; update the code that sets url (and later used in install_cmd and
reported via self.report) to use the escaped variant while keeping
REMOTE_COASTD_PATH unchanged.
- Around line 280-296: The install/uninstall/systemd script strings (e.g.,
install_script) are being flattened by replace('\n', " ") before passing into
ssh_exec_streaming, which removes line breaks and breaks shell semantics;
instead stop replacing newlines and send the script as a proper multiline
payload (for example use a heredoc or quoted EOF in the remote command, or
base64-encode the script and decode on the remote side) when calling
ssh_exec_streaming; update all call sites (ssh_exec_streaming usages around
install_script and the other script variables at the same pattern) to preserve
newlines so systemd unit content and shell separators remain intact.
In `@coast-daemon/src/server.rs`:
- Around line 2137-2143: write_response(writer, &response).await? currently
returns early on socket errors so the final-response handling and shadow record
status-update/delete never run; change the logic to catch and log write_response
errors (do NOT early-return) so you still check is_final and run the shadow
finalization block (status updates/deletes) even when writes to the client fail,
and apply the same change to the analogous block around lines 2148-2168;
reference write_response, writer, response, is_final and the shadow
status-update/delete/finalize code to locate where to convert the fallible write
into a non-fatal logged error and ensure finalization always executes.
- Around line 572-579: The remote-forwarding branches that call
get_instance_remote_route(...) then return after
forward_streaming_to_remote(...) must first acquire the same local mutation gate
used for local operations (the per-project semaphore obtained via
begin_streaming_update_operation or begin_socket_update_operation) so forwarded
mutating operations are serialized with local ones; change each matching branch
(e.g. where Assign/Unassign/Start/Stop/Rm/Build/Run create a remote_request and
call forward_streaming_to_remote or forward_socket_to_remote) to acquire the
appropriate begin_*_operation guard for req.project before calling
forward_*_to_remote, defer tracking/return until after the guard is held, and
ensure the guard is released after forward completes (or on error) so
self-update quiescing and per-project serialization behave identically for
remote-routed mutations.
- Around line 1800-1804: The code currently treats a project with mode=remote
but no remote_name as a silent fallback by returning Ok(None); change this to
surface a hard error: replace the warn!/return Ok(None) branch that checks let
Some(remote_name) = config.remote_name else { ... } with an error return (e.g.,
return Err(...)) that includes context (project id/name and that
project_modes.remote_name is missing). Keep or convert the warn! log to an error
log and construct the returned error using the crate's error type (e.g., anyhow!
or the function's existing error type) so callers cannot silently downgrade to
local execution; reference the matched symbols remote_name, config.remote_name,
warn!, and Ok(None) when locating the change.
In `@coast-daemon/src/state/mod.rs`:
- Around line 165-172: Add a host-wide uniqueness constraint for local_port by
creating a UNIQUE index (or adding UNIQUE to the column) on local_port for both
the tunnels table and the forwards table (referencing the tunnels table
definition with columns remote_name, local_port and the forwards table
definition that scopes by project/instance_name) and add a DB migration that (a)
detects and resolves existing duplicate local_port rows (e.g., fail fast with
clear error or de-duplicate/assign new ports according to project policy) and
(b) creates the unique index in a safe ALTER step; update the migration
registration code in mod.rs so the new migration runs for existing DBs.
In `@install_mutagen.sh`:
- Around line 8-10: The script hardcodes amd64 in the download URL
(MUTAGEN_VERSION and the curl command); detect the machine architecture (e.g.,
via uname -m) and map it to Mutagen's archive suffix (x86_64 -> amd64, aarch64
-> arm64, armv7l/armv6l -> arm, etc.), set an ARCH variable accordingly, then
build the download URL using that ARCH when calling curl (replace the hardcoded
"amd64" in the mutagen_linux_amd64_v${MUTAGEN_VERSION}.tar.gz URL with the ARCH
variable) and fall back with a helpful error if the architecture is unsupported.
- Around line 12-19: The script currently extracts mutagen.tar.gz and only moves
the mutagen CLI binary to ~/.local/bin, leaving mutagen-agents.tar.gz behind and
breaking SSH endpoints; update install_mutagen.sh to preserve the full release
by creating a single install prefix (e.g., ~/.local/mutagen), move the extracted
mutagen binary into PREFIX/bin (keeping the name mutagen), move the
mutagen-agents.tar.gz bundle into PREFIX/libexec (or extract it there), and set
executable permissions on PREFIX/bin/mutagen so both the CLI (mutagen) and the
agent bundle (mutagen-agents.tar.gz) live together (references: tar -xzf
mutagen.tar.gz, mutagen, mutagen-agents.tar.gz, mkdir -p ~/.local/bin, mv
mutagen).
In `@simple_deploy.sh`:
- Around line 7-8: Remove the hardcoded VM_HOST and VM_PASSWORD variables
(VM_HOST, VM_PASSWORD) and stop disabling host-key verification in the
ssh/sshpass invocation (the use of StrictHostKeyChecking=no and
UserKnownHostsFile=/dev/null). Instead accept host and credential input via
environment variables or CLI flags (e.g., DEPLOY_VM_HOST and DEPLOY_VM_PASSWORD
or --host/--password) and validate they are present; require normal known_hosts
verification or a pinned host key file provided via an env/flag (e.g.,
DEPLOY_KNOWN_HOSTS) and fail if the key is missing rather than falling back to
insecure options. Ensure the ssh/sshpass call uses the provided host/key
variables and does not include -o StrictHostKeyChecking=no or -o
UserKnownHostsFile=/dev/null so that standard SSH host verification is enforced.
- Around line 201-207: The coastd-dev status check is being evaluated locally
because the $(...) substitution inside the SSH payload is not escaped; update
the SSH payload so the coastd-dev check uses a backslash-escaped substitution
(prefix the $(...) with a backslash) so the test for ~/.local/bin/coastd-dev
runs on the remote VM, keeping the surrounding double-quoted SSH string intact
and matching the escaping style used for the other checks.
---
Duplicate comments:
In `@coast-daemon/src/handlers/remote.rs`:
- Around line 186-195: The current early return on Ok(Some(version)) from
setup.check_coastd only proves the binary exists and incorrectly reports
success; update the logic so that after RemoteSetup::new() and
setup.check_coastd(&remote).await returning Some(version) you additionally
verify the service is active and healthy (e.g., call or add methods like
RemoteSetup::is_systemd_active(&remote) and
RemoteSetup::run_health_check(&remote) or extend check_coastd to return a richer
status enum), and only return RemoteSetupResponse.success=true when both the
systemd unit is active and the health check passes; if either check fails, fall
through to the repair/setup path instead of returning success.
- Around line 335-341: The current logic in the tunnel setup (inside the match
handling tunnel_manager.connect) logs failures from
db.upsert_tunnel(&result.tunnel_state) but continues to treat the overall
operation as success; change this so that when db.upsert_tunnel returns Err you
propagate/return an error response (or Err from the handler) instead of
proceeding. Locate the block that calls state.db.lock().await and
db.upsert_tunnel(&result.tunnel_state) (and the similar block at the second
occurrence) and convert the if let Err(e) = ... { warn!(...); } into an early
return that returns an appropriate failure result containing the error (or maps
it into the handler's response type) so tunnel persistence failures are reported
to the caller.
---
Nitpick comments:
In `@coast-core/src/protocol/tests.rs`:
- Around line 597-620: Add a second roundtrip case in test_ls_response_roundtrip
that covers serialization when InstanceSummary.remote_name is Some(...):
duplicate the existing Response::Ls(LsResponse { instances: vec![InstanceSummary
{ ... }], ... }) entry but set remote_name: Some("origin".to_string()) (or
similar non-empty string) and run roundtrip_response on it; modify the test to
include both the original None case and this new Some(...) case so
LsResponse/InstanceSummary remote metadata serialization is exercised.
In `@coast-core/src/types/tests.rs`:
- Line 155: The test fixture was updated to include remote_name: None but lacks
an explicit assertion; update the serialization round-trip test (the test that
constructs the fixture and deserializes it) to assert that the deserialized
object's remote_name field is None (e.g., assert_eq!(deserialized.remote_name,
None)) so the test verifies the field is preserved through
serialize/deserialize; locate the variable holding the deserialized value
(commonly named deserialized, round_trip, or result) and add the assertion
there.
In `@coast-daemon/src/handlers/ls.rs`:
- Around line 244-323: Tests for the ls handler lack a positive assertion for
remote_name after fixtures were updated; update the test setup in
test_ls_multiple_instances by setting remote_name: Some("origin".to_string())
(or similar) on one of the inserted CoastInstance fixtures (identify the
CoastInstance structs inserted in test_ls_multiple_instances) and then after
calling handle(req, &state).await assert that the returned result.instances for
the corresponding instance contains remote_name == Some("origin".to_string());
this ensures the Ls handler (handle and LsRequest) is verified to return
remote_name correctly.
In `@simple_deploy.sh`:
- Around line 53-90: The current install block in simple_deploy.sh (the logic
that checks command -v mutagen and then downloads/install MUTAGEN_VERSION into
~/.local/bin and agents into ~/.local/libexec) may be unnecessary for SSH
endpoints because Mutagen can copy agent binaries over ssh/scp and doesn’t
require a full remote install; update the script to first detect whether the
deployment target actually runs mutagen locally (e.g., whether coastd-dev or any
remote process will invoke the mutagen binary on the VM) and skip the full
install if not needed: replace or gate the existing install branch (the command
-v mutagen check and subsequent install steps, including MUTAGEN_VERSION,
~/.local/bin, and ~/.local/libexec handling) with a conditional that only
installs when remote runs mutagen itself, otherwise rely on mutagen’s agent
transfer behavior or document why a full install is required.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 13831d20-4772-4885-8987-be437d59692d
📒 Files selected for processing (57)
REMOTE_COAST_TEST.mdcoast-cli/src/commands/builds.rscoast-cli/src/commands/ls.rscoast-cli/src/commands/mod.rscoast-cli/src/commands/sync.rscoast-cli/src/lib.rscoast-core/src/protocol/query.rscoast-core/src/protocol/remote.rscoast-core/src/protocol/tests.rscoast-core/src/types/instance.rscoast-core/src/types/tests.rscoast-daemon/src/analytics.rscoast-daemon/src/api/streaming.rscoast-daemon/src/api/tests.rscoast-daemon/src/git_watcher.rscoast-daemon/src/handlers/agent_shell.rscoast-daemon/src/handlers/archive.rscoast-daemon/src/handlers/assign/mod.rscoast-daemon/src/handlers/builds.rscoast-daemon/src/handlers/checkout.rscoast-daemon/src/handlers/exec.rscoast-daemon/src/handlers/logs.rscoast-daemon/src/handlers/lookup.rscoast-daemon/src/handlers/ls.rscoast-daemon/src/handlers/mod.rscoast-daemon/src/handlers/ports.rscoast-daemon/src/handlers/ps.rscoast-daemon/src/handlers/rebuild.rscoast-daemon/src/handlers/remote.rscoast-daemon/src/handlers/rerun_extractors.rscoast-daemon/src/handlers/restart_services.rscoast-daemon/src/handlers/rm.rscoast-daemon/src/handlers/rm_build.rscoast-daemon/src/handlers/run/finalize.rscoast-daemon/src/handlers/run/mod.rscoast-daemon/src/handlers/run/validate.rscoast-daemon/src/handlers/secret.rscoast-daemon/src/handlers/start.rscoast-daemon/src/handlers/stop.rscoast-daemon/src/handlers/unassign.rscoast-daemon/src/handlers/update_safety.rscoast-daemon/src/lib.rscoast-daemon/src/remote/client.rscoast-daemon/src/remote/mod.rscoast-daemon/src/remote/mutagen.rscoast-daemon/src/remote/setup.rscoast-daemon/src/remote/tunnel.rscoast-daemon/src/server.rscoast-daemon/src/state/instances.rscoast-daemon/src/state/mod.rscoast-daemon/src/state/remotes.rscoast-guard/src/components/RemoteBadge.tsxcoast-guard/src/pages/InstanceDetailPage.tsxcoast-guard/src/pages/ProjectDetailPage.tsxinstall_mutagen.shintegration-tests/tests/integration_test.rssimple_deploy.sh
✅ Files skipped from review due to trivial changes (15)
- coast-daemon/src/handlers/secret.rs
- coast-daemon/src/handlers/ps.rs
- coast-daemon/src/handlers/update_safety.rs
- coast-daemon/src/handlers/checkout.rs
- coast-daemon/src/handlers/stop.rs
- coast-daemon/src/handlers/ports.rs
- coast-daemon/src/handlers/unassign.rs
- coast-daemon/src/handlers/rm.rs
- coast-daemon/src/handlers/rerun_extractors.rs
- coast-daemon/src/handlers/restart_services.rs
- coast-daemon/src/handlers/exec.rs
- coast-daemon/src/handlers/rm_build.rs
- coast-daemon/src/lib.rs
- coast-guard/src/components/RemoteBadge.tsx
- REMOTE_COAST_TEST.md
🚧 Files skipped from review as they are similar to previous changes (7)
- coast-cli/src/lib.rs
- coast-cli/src/commands/mod.rs
- coast-daemon/src/analytics.rs
- coast-daemon/src/handlers/mod.rs
- coast-daemon/src/remote/tunnel.rs
- coast-daemon/src/state/remotes.rs
- coast-core/src/protocol/remote.rs
Comment on lines
+205
to
+210
| // Truncate path for display | ||
| let path_display = if session.local_path.len() > 38 { | ||
| format!("...{}", &session.local_path[session.local_path.len() - 35..]) | ||
| } else { | ||
| session.local_path.clone() | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do Rust str slices require UTF-8 character boundaries, and does slicing at arbitrary byte offsets panic at runtime?
💡 Result:
Yes.
&stris guaranteed to be valid UTF-8, so any&s[a..b]slice must start/end on a UTF-8 code point (“char”) boundary (checked viais_char_boundary). Ifaorbis not a character boundary, slicing panics at runtime. [1]- Slicing also panics if the range is otherwise invalid (e.g.,
a > borb > s.len()). [1] - The indices in
s[a..b]are byte offsets, not character indices; attempting to slice “in the middle” of a multi-byte UTF-8 character is a common cause of a runtime panic. [2]
If you want to avoid panics, use s.get(a..b) which returns Option<&str> instead of panicking. [1]
Sources:
[1] Rust docs: str slicing panic conditions (core::primitive::str) (doc.rust-lang.org)
[2] Rust Book example showing panic when byte index is not a char boundary (docs1.w3cub.com)
Citations:
- 1: https://doc.rust-lang.org/beta/core/primitive.str.html?utm_source=openai
- 2: https://docs1.w3cub.com/rust/book/strings/?utm_source=openai
🏁 Script executed:
# Read the sync.rs file to examine lines 205-210 and 225-228
wc -l coast-cli/src/commands/sync.rsRepository: VAIBHAVSING/coasts
Length of output: 96
🏁 Script executed:
# Get lines 200-215 to see the first instance with context
sed -n '200,215p' coast-cli/src/commands/sync.rsRepository: VAIBHAVSING/coasts
Length of output: 702
🏁 Script executed:
# Get lines 220-235 to see the second instance with context
sed -n '220,235p' coast-cli/src/commands/sync.rsRepository: VAIBHAVSING/coasts
Length of output: 367
🏁 Script executed:
# Search for truncate_str helper function mentioned in the scratchpad
rg "truncate_str|format_sessions_table" coast-cli/src/commands/sync.rs -B2 -A5Repository: VAIBHAVSING/coasts
Length of output: 2126
Replace unsafe byte slicing with char-boundary-aware truncation.
Both the inline path truncation (line 205-210) and the truncate_str helper (line 226-229) slice strings using byte offsets without checking UTF-8 character boundaries. A non-ASCII project name, remote name, or filesystem path will panic at runtime with "byte index is not a char boundary". Replace the byte-index slicing with character-aware truncation to handle multi-byte UTF-8 characters safely.
Also applies to: truncate_str function at lines 226-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-cli/src/commands/sync.rs` around lines 205 - 210, The current
truncation slices strings by byte offsets (path_display logic and truncate_str)
which panics on multi-byte UTF-8 characters; change both to use character-aware
truncation (operate on .chars()) so you count characters and take the last N
characters safely (e.g., build the tail with chars().rev().take(...) then
reverse/collect and prefix "..."). Update the session.local_path truncation
(path_display) and the truncate_str function to use this char-based approach
rather than byte indexing.
Comment on lines
+116
to
+123
| let db = state.db.lock().await; | ||
| match db.list_remotes() { | ||
| Ok(remotes) => { | ||
| let tunnel_statuses = if let Some(tm) = state.tunnel_manager.as_ref() { | ||
| tm.get_tunnel_statuses().await | ||
| } else { | ||
| std::collections::HashMap::new() | ||
| }; |
There was a problem hiding this comment.
Drop the DB mutex before awaiting the tunnel manager.
The lock acquired on Line 116 is still held when Line 120 awaits tm.get_tunnel_statuses(). That can block unrelated DB work for the duration of the tunnel lookup and creates a lock-order deadlock risk if the tunnel manager ever needs DB-backed state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/handlers/remote.rs` around lines 116 - 123, You are holding
the DB mutex across an await: the call sequence around state.db.lock().await ->
db.list_remotes() currently keeps the lock while awaiting
tm.get_tunnel_statuses().await. To fix, acquire the lock, call
db.list_remotes(), clone or extract any needed data from that result, then drop
the mutex (let db_guard go out of scope) before checking state.tunnel_manager
and awaiting tm.get_tunnel_statuses().await; reference the symbols
state.db.lock().await, db.list_remotes(), state.tunnel_manager and
tm.get_tunnel_statuses().await to locate the code to change.
Comment on lines
+418
to
+429
| let session_infos: Vec<SyncSessionInfo> = if let Some(ref project) = req.project { | ||
| // Filter by project | ||
| mutagen_sessions | ||
| .iter() | ||
| .filter(|s| { | ||
| s.name.as_ref().map_or(false, |n| n.contains(project)) | ||
| }) | ||
| .map(|s| { | ||
| let db_session = db_sessions.iter().find(|ds| { | ||
| MutagenManager::generate_session_name(&ds.project, "main", &ds.remote_name) | ||
| == s.name.clone().unwrap_or_default() | ||
| }); |
There was a problem hiding this comment.
Use a stable, exact sync-session identifier.
Line 423 filters with contains(project), and the DB fallback paths rebuild session names with a hard-coded main branch. handle_sync_create() creates sessions with req.branch, so non-main sessions become unaddressable after a restart, while short project names can also over-match unrelated sessions. Persist the actual session_name (or at least the branch) and match it exactly.
Also applies to: 447-450, 520-524, 574-578, 628-632, 778-782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/handlers/remote.rs` around lines 418 - 429, The filter is
currently using contains(project) and reconstructing names with a hard-coded
"main", which causes incorrect/ambiguous matches and loses non-main branches;
update the logic to use the exact persisted session identifier instead: when
building SyncSessionInfo (and in the other similar blocks at the noted ranges)
read and match the actual session_name (or branch) stored in the DB record
rather than using contains(project) or generate_session_name(..., "main", ...);
change the filter to compare s.name.as_ref().map_or(false, |n| n ==
&db_session.session_name) (or compare branch fields exactly) and ensure
MutagenManager::generate_session_name is called using the persisted branch from
db_sessions (not a hard-coded "main") so sessions created by
handle_sync_create() remain addressable after restart.
Comment on lines
+319
to
+330
| Ok(CreateSessionResult { | ||
| session: session_info, | ||
| db_session: SyncSession { | ||
| project: project.to_string(), | ||
| remote_name: remote.name.clone(), | ||
| local_path: local_path.to_string_lossy().to_string(), | ||
| remote_path, | ||
| mutagen_session_id: Some(session_id), | ||
| status: SyncStatus::Initial, | ||
| last_sync_at: None, | ||
| created_at: now, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 100
🏁 Script executed:
cat -n coast-daemon/src/remote/mutagen.rs | head -100Repository: VAIBHAVSING/coasts
Length of output: 3732
🏁 Script executed:
cat -n coast-daemon/src/remote/mutagen.rs | sed -n '300,350p'Repository: VAIBHAVSING/coasts
Length of output: 2326
🏁 Script executed:
cat -n coast-daemon/src/remote/mutagen.rs | sed -n '600,660p'Repository: VAIBHAVSING/coasts
Length of output: 2859
🏁 Script executed:
rg "SyncSession\|CreateSessionResult" coast-daemon/src/remote/mutagen.rs -A 5 -B 2Repository: VAIBHAVSING/coasts
Length of output: 44
🏁 Script executed:
cat -n coast-daemon/src/remote/mutagen.rs | sed -n '200,300p'Repository: VAIBHAVSING/coasts
Length of output: 4292
🏁 Script executed:
rg "fn create_session" coast-daemon/src/remote/mutagen.rs -A 10Repository: VAIBHAVSING/coasts
Length of output: 434
🏁 Script executed:
grep -n "generate_session_name" coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 608
🏁 Script executed:
find coast-daemon -name "*.rs" -type f | xargs grep -l "struct SyncSession" | head -5Repository: VAIBHAVSING/coasts
Length of output: 95
🏁 Script executed:
cat coast-daemon/src/state/remotes.rs | grep -A 20 "struct SyncSession"Repository: VAIBHAVSING/coasts
Length of output: 669
🏁 Script executed:
cat -n coast-daemon/src/remote/mutagen.rs | sed -n '140,160p'Repository: VAIBHAVSING/coasts
Length of output: 914
Persist branch information to prevent session loss on non-main branches after restart.
The create_session method receives a branch parameter and uses it to generate the correct session_name (format: coast-<project>-<branch>-<remote>). However, the SyncSession struct persisted to the database lacks a branch field. When load_sessions_from_db reloads sessions, it hardcodes "main" as the branch (lines 633, 641), reconstructing the session key as coast-<project>-main-<remote>. This causes non-main sessions to be stored under the wrong in-memory key, making them unreachable by subsequent lookups using the real branch name.
Add a branch field to SyncSession, persist the actual branch value in create_session (lines 319–330), and use the persisted value in load_sessions_from_db (lines 624–649) instead of hardcoding "main".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/mutagen.rs` around lines 319 - 330, The SyncSession
struct and session persistence need to include branch so sessions for non-main
branches survive restarts: add a branch: String field to SyncSession, set it
when constructing the SyncSession in create_session (use the incoming branch
parameter so mutagen_session_id/remote_path etc. are persisted together with
branch), update any DB save/serialize code to persist the new branch column, and
change load_sessions_from_db to read the persisted branch value (instead of
hardcoding "main") when reconstructing the in-memory session key and SessionInfo
so the session_name uses coast-<project>-<branch>-<remote> and lookups work
correctly.
Comment on lines
+335
to
+357
| async fn get_session_id_by_name(&self, session_name: &str) -> Result<Option<String>> { | ||
| let output = Command::new("mutagen") | ||
| .args(["sync", "list", "--label-selector", &format!("name={}", session_name), "-o", "json"]) | ||
| .output() | ||
| .await | ||
| .map_err(|e| CoastError::Remote { | ||
| message: format!("failed to list Mutagen sessions: {e}"), | ||
| })?; | ||
|
|
||
| if !output.status.success() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| if let Ok(list) = serde_json::from_str::<MutagenListOutput>(&stdout) { | ||
| if let Some(sessions) = list.sessions { | ||
| if let Some(session) = sessions.first() { | ||
| return Ok(session.identifier.clone()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(None) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "mutagen.rs" | head -20Repository: VAIBHAVSING/coasts
Length of output: 98
🏁 Script executed:
# Get repo structure overview
git ls-files | grep -E "(mutagen|coast-daemon)" | head -30Repository: VAIBHAVSING/coasts
Length of output: 1126
🏁 Script executed:
# Check the specific file and lines mentioned
wc -l coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 100
🏁 Script executed:
# Read the get_session_id_by_name function around line 335-357
sed -n '330,365p' coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 1246
🏁 Script executed:
# Read the list_all_sessions and related functions around line 539-621
sed -n '535,625p' coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 3733
🏁 Script executed:
# Search for where sessions are created to see what --name is used
rg "create.*session\|--name" coast-daemon/src/remote/mutagen.rs -A 3 -B 3Repository: VAIBHAVSING/coasts
Length of output: 44
🏁 Script executed:
# Search for create_session or where sessions are created
grep -n "create.*session\|--name\|sync.*create" coast-daemon/src/remote/mutagen.rs | head -40Repository: VAIBHAVSING/coasts
Length of output: 565
🏁 Script executed:
# Look at the entire create_session function
sed -n '1,150p' coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 4661
🏁 Script executed:
# Look for the create function in the file
rg "fn create\|async fn create" coast-daemon/src/remote/mutagen.rs -B 2 -A 15Repository: VAIBHAVSING/coasts
Length of output: 44
🏁 Script executed:
# Let me look at the actual create_session function more completely
sed -n '206,320p' coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 4135
🏁 Script executed:
# Let me search the codebase for any Mutagen documentation references or comments
rg "mutagen|--name|--label-selector" coast-daemon/src/remote/mutagen.rs -B 2 -A 2 | head -100Repository: VAIBHAVSING/coasts
Length of output: 3736
🏁 Script executed:
# Check if there are any tests or examples showing how mutagen commands work
find . -type f -name "*.rs" -exec grep -l "mutagen" {} \; | head -20Repository: VAIBHAVSING/coasts
Length of output: 266
🏁 Script executed:
# Look for any comments about the difference between names and labels in Mutagen
rg "name.*label|label.*name" coast-daemon/src/remote/mutagen.rs -B 2 -A 2Repository: VAIBHAVSING/coasts
Length of output: 1027
🌐 Web query:
Mutagen sync list --name vs --label-selector documentation
💡 Result:
In Mutagen, --name is not a mutagen sync list filter flag—it’s how you assign an (optional, non-unique) name when creating a session. After that, you select by name by passing the name as a positional argument to session-management commands (including list). Mutagen also supports selecting sessions by labels (key/value metadata) using --label-selector, which uses Kubernetes label/selector syntax. [1]
So, for listing sessions:
- By name (positional arg):
mutagen sync list my-session-name(selects sessions whose Name matches)
- By labels (query):
mutagen sync list --label-selector 'project=myproj'(selects sessions whose labels match)
If you’re using mutagen project, the sessions it creates are “normal sessions” that are bound to the project via a label, so --label-selector is commonly used to list just that project’s sessions. [2]
Docs: Names/labels/identifiers and --label-selector behavior are documented here. [1]
Sources: [1] [2]
🌐 Web query:
Mutagen CLI names and labels distinction
💡 Result:
In Mutagen, names and labels are two different ways to identify/select sessions:
-
Name
- A single, optional, user-provided string attached to a session.
- Not guaranteed unique (multiple sessions can share the same name).
- Best for simple/manual workflows (e.g., create with
--name=web-app-code, then refer to it by that name in later commands). [1][2]
-
Labels
- Optional key/value pairs attached to a session (you can have multiple labels per session).
- Designed for grouping and querying sessions using label selectors via
--label-selector(Mutagen reuses Kubernetes label/selector semantics). [2] - Common when you want “operate on all sessions matching X” (e.g., all sessions for a project/environment). Mutagen “projects” work this way: project sessions are “normal sessions” that include a label that binds them to the project. [3]
Also note Mutagen has session identifiers (auto-generated, unique) for unambiguous targeting. [2]
Sources
- Mutagen “Getting started” (examples using
--name, and session identification overview) — https://mutagen.io/documentation/introduction/getting-started - Mutagen “Names, labels, and identifiers” (formal definitions +
--label-selector) — https://mutagen.io/documentation/introduction/names-labels-identifiers/ - Mutagen “Projects” (project sessions are bound via a label) — https://mutagen.io/documentation/orchestration/projects/
🏁 Script executed:
# Let me verify the exact Mutagen sync list help or examples to confirm
# First, let's check if there are any tests or documentation in the repo itself
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "README*" \) -exec grep -l "mutagen\|sync" {} \; | head -20Repository: VAIBHAVSING/coasts
Length of output: 2124
🏁 Script executed:
# Check if there's anything in git history that shows this was working before or if it was always wrong
git log --oneline coast-daemon/src/remote/mutagen.rs | head -10Repository: VAIBHAVSING/coasts
Length of output: 133
🏁 Script executed:
# Let me also check what the actual behavior would be - look at pause/resume/terminate
# which seem to use the session_name as a positional argument
sed -n '395,450p' coast-daemon/src/remote/mutagen.rsRepository: VAIBHAVSING/coasts
Length of output: 2016
Replace --label-selector with positional session name arguments in session lookups.
Mutagen distinguishes between session names and labels. The --name flag during creation assigns a session name (not a label); to query sessions by name, pass the name as a positional argument to mutagen sync list, not via --label-selector. The current code creates sessions with --name but queries them with --label-selector name=..., which looks for labels instead and will always fail to find the sessions. This causes get_session_id_by_name to return None, get_session_status to report "not found" even when sessions exist, and list_all_sessions to return empty results.
Correct pattern (already used in pause, resume, terminate):
- .args(["sync", "list", "--label-selector", &format!("name={}", session_name), "-o", "json"])
+ .args(["sync", "list", session_name, "-o", "json"])For list_all_sessions, since there's no mutagen sync list coast-* prefix matching, list all sessions and filter by name in Rust:
- .args(["sync", "list", "--label-selector", "name=coast-*", "-o", "json"])
+ .args(["sync", "list", "-o", "json"])Then filter results with name.starts_with("coast-").
Reference: mutagen.io/documentation/introduction/names-labels-identifiers
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn get_session_id_by_name(&self, session_name: &str) -> Result<Option<String>> { | |
| let output = Command::new("mutagen") | |
| .args(["sync", "list", "--label-selector", &format!("name={}", session_name), "-o", "json"]) | |
| .output() | |
| .await | |
| .map_err(|e| CoastError::Remote { | |
| message: format!("failed to list Mutagen sessions: {e}"), | |
| })?; | |
| if !output.status.success() { | |
| return Ok(None); | |
| } | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| if let Ok(list) = serde_json::from_str::<MutagenListOutput>(&stdout) { | |
| if let Some(sessions) = list.sessions { | |
| if let Some(session) = sessions.first() { | |
| return Ok(session.identifier.clone()); | |
| } | |
| } | |
| } | |
| Ok(None) | |
| async fn get_session_id_by_name(&self, session_name: &str) -> Result<Option<String>> { | |
| let output = Command::new("mutagen") | |
| .args(["sync", "list", session_name, "-o", "json"]) | |
| .output() | |
| .await | |
| .map_err(|e| CoastError::Remote { | |
| message: format!("failed to list Mutagen sessions: {e}"), | |
| })?; | |
| if !output.status.success() { | |
| return Ok(None); | |
| } | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| if let Ok(list) = serde_json::from_str::<MutagenListOutput>(&stdout) { | |
| if let Some(sessions) = list.sessions { | |
| if let Some(session) = sessions.first() { | |
| return Ok(session.identifier.clone()); | |
| } | |
| } | |
| } | |
| Ok(None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/remote/mutagen.rs` around lines 335 - 357,
get_session_id_by_name currently calls mutagen with --label-selector (which
searches labels) so it never finds sessions created with --name; change the
Command invocation in get_session_id_by_name to pass the session name as a
positional argument to `mutagen sync list` (mirroring the pattern used in
pause/resume/terminate) rather than using "--label-selector", and then parse the
JSON output as before; also ensure related callers/getters (get_session_status
and list_all_sessions) follow the correct pattern—use positional name arguments
when querying a single session and for list_all_sessions call `mutagen sync list
-o json` (no name filter) and filter the returned sessions in Rust by
session.name.starts_with("coast-") to implement prefix filtering.
Comment on lines
+165
to
+172
| CREATE TABLE IF NOT EXISTS tunnels ( | ||
| remote_name TEXT PRIMARY KEY, | ||
| local_port INTEGER NOT NULL, | ||
| ssh_pid INTEGER, | ||
| status TEXT NOT NULL, | ||
| connected_at TEXT, | ||
| FOREIGN KEY (remote_name) REFERENCES remotes(name) ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
Make localhost ports unique across remotes and forwards.
local_port is a host-wide resource, but both tables only scope it by remote_name or (project, instance_name). That allows two rows to reserve the same local port, so the next SSH bind can fail or a lookup can point at the wrong forward. Add a uniqueness constraint/index on local_port in both tables, with a migration for existing DBs.
Suggested direction
CREATE TABLE IF NOT EXISTS tunnels (
remote_name TEXT PRIMARY KEY,
- local_port INTEGER NOT NULL,
+ local_port INTEGER NOT NULL UNIQUE,
ssh_pid INTEGER,
status TEXT NOT NULL,
connected_at TEXT,
FOREIGN KEY (remote_name) REFERENCES remotes(name) ON DELETE CASCADE
);
CREATE TABLE IF NOT EXISTS local_port_forwards (
project TEXT NOT NULL,
instance_name TEXT NOT NULL,
service_name TEXT,
local_port INTEGER NOT NULL,
remote_port INTEGER NOT NULL,
ssh_pid INTEGER,
- PRIMARY KEY (project, instance_name, local_port)
+ PRIMARY KEY (project, instance_name, local_port),
+ UNIQUE(local_port)
);Also applies to: 197-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coast-daemon/src/state/mod.rs` around lines 165 - 172, Add a host-wide
uniqueness constraint for local_port by creating a UNIQUE index (or adding
UNIQUE to the column) on local_port for both the tunnels table and the forwards
table (referencing the tunnels table definition with columns remote_name,
local_port and the forwards table definition that scopes by
project/instance_name) and add a DB migration that (a) detects and resolves
existing duplicate local_port rows (e.g., fail fast with clear error or
de-duplicate/assign new ports according to project policy) and (b) creates the
unique index in a safe ALTER step; update the migration registration code in
mod.rs so the new migration runs for existing DBs.
Comment on lines
+8
to
+10
| # Download latest version | ||
| MUTAGEN_VERSION="0.17.6" | ||
| curl -L "https://github.com/mutagen-io/mutagen/releases/download/v${MUTAGEN_VERSION}/mutagen_linux_amd64_v${MUTAGEN_VERSION}.tar.gz" -o /tmp/mutagen.tar.gz |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Mutagen v0.17.6 GitHub releases architectures linux
💡 Result:
For Mutagen v0.17.6 on GitHub, the Linux release binaries follow the pattern:
mutagen_linux_amd64_v0.17.6.tar.gz(x86_64 / amd64)mutagen_linux_arm64_v0.17.6.tar.gz(AArch64 / arm64)mutagen_linux_arm_v0.17.6.tar.gz(32-bit ARM / armhf)
These correspond to the Linux targets referenced by WakeMeOps’ packaging blueprint (which pulls directly from Mutagen’s GitHub release assets using mutagen_linux_{{target}}_v{{version}}.tar.gz). [1] The v0.17.6 release itself is published on the mutagen-io/mutagen GitHub Releases page. [2]
Sources: [1] (docs.wakemeops.com) [2] (github.com)
Citations:
- 1: https://docs.wakemeops.com/packages/mutagen/
- 2: https://github.com/mutagen-io/mutagen/releases/tag/v0.17.6
🌐 Web query:
Mutagen installation documentation Linux architecture support
💡 Result:
Mutagen’s official install docs say that on Linux you can either install via Homebrew or “download the appropriate release” from GitHub and put it on your PATH. [1]
For Linux architecture support, the published release artifacts include (at least) these Linux builds:
linux_amd64(x86_64) [2]linux_arm64(AArch64) [2]
Sources: Mutagen install docs [1] and the release artifact URLs for v0.18.1 (showing mutagen_linux_amd64_… and mutagen_linux_arm64_…). [2]
References
[1] (mutagen.io)
[2] (docs.wakemeops.com)
Citations:
- 1: https://mutagen.io/documentation/introduction/installation
- 2: https://docs.wakemeops.com/packages/mutagen/?utm_source=openai
Use dynamic architecture detection instead of hardcoding amd64.
Mutagen releases platform-specific binaries. The v0.17.6 release includes separate archives for amd64, arm64, and 32-bit ARM. Hardcoding mutagen_linux_amd64_v0.17.6.tar.gz causes the installer to fail on non-x86_64 Linux systems. The official Mutagen documentation explicitly states to "download the appropriate release" for the platform.
Suggested fix
-MUTAGEN_VERSION="0.17.6"
-curl -L "https://github.com/mutagen-io/mutagen/releases/download/v${MUTAGEN_VERSION}/mutagen_linux_amd64_v${MUTAGEN_VERSION}.tar.gz" -o /tmp/mutagen.tar.gz
+MUTAGEN_VERSION="0.17.6"
+case "$(uname -m)" in
+ x86_64|amd64) MUTAGEN_ARCH="amd64" ;;
+ aarch64|arm64) MUTAGEN_ARCH="arm64" ;;
+ *)
+ echo "Unsupported architecture: $(uname -m)" >&2
+ exit 1
+ ;;
+esac
+curl -L "https://github.com/mutagen-io/mutagen/releases/download/v${MUTAGEN_VERSION}/mutagen_linux_${MUTAGEN_ARCH}_v${MUTAGEN_VERSION}.tar.gz" -o /tmp/mutagen.tar.gz📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Download latest version | |
| MUTAGEN_VERSION="0.17.6" | |
| curl -L "https://github.com/mutagen-io/mutagen/releases/download/v${MUTAGEN_VERSION}/mutagen_linux_amd64_v${MUTAGEN_VERSION}.tar.gz" -o /tmp/mutagen.tar.gz | |
| # Download latest version | |
| MUTAGEN_VERSION="0.17.6" | |
| case "$(uname -m)" in | |
| x86_64|amd64) MUTAGEN_ARCH="amd64" ;; | |
| aarch64|arm64) MUTAGEN_ARCH="arm64" ;; | |
| *) | |
| echo "Unsupported architecture: $(uname -m)" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| curl -L "https://github.com/mutagen-io/mutagen/releases/download/v${MUTAGEN_VERSION}/mutagen_linux_${MUTAGEN_ARCH}_v${MUTAGEN_VERSION}.tar.gz" -o /tmp/mutagen.tar.gz |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install_mutagen.sh` around lines 8 - 10, The script hardcodes amd64 in the
download URL (MUTAGEN_VERSION and the curl command); detect the machine
architecture (e.g., via uname -m) and map it to Mutagen's archive suffix (x86_64
-> amd64, aarch64 -> arm64, armv7l/armv6l -> arm, etc.), set an ARCH variable
accordingly, then build the download URL using that ARCH when calling curl
(replace the hardcoded "amd64" in the
mutagen_linux_amd64_v${MUTAGEN_VERSION}.tar.gz URL with the ARCH variable) and
fall back with a helpful error if the architecture is unsupported.
Comment on lines
+12
to
+19
| # Extract | ||
| cd /tmp | ||
| tar -xzf mutagen.tar.gz | ||
|
|
||
| # Install to user bin | ||
| mkdir -p ~/.local/bin | ||
| mv mutagen ~/.local/bin/ | ||
| chmod +x ~/.local/bin/mutagen |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "install_mutagen.sh" -type fRepository: VAIBHAVSING/coasts
Length of output: 82
🏁 Script executed:
cat -n ./install_mutagen.shRepository: VAIBHAVSING/coasts
Length of output: 1261
🌐 Web query:
Mutagen installation requirements SSH agent binaries
💡 Result:
For Mutagen over SSH, the key installation/runtime requirements around “SSH agent binaries” are:
- Local machine: You must have an OpenSSH client available (specifically
sshandscp). Mutagen usesscpto copy its own small agent binaries to the remote andsshto run/communicate with those agents. [1] - Remote machine: You do not manually install Mutagen on the remote endpoint—Mutagen injects/updates the remote Mutagen agent binary automatically via
scpand runs it viassh. You just need an SSH server that’s compatible with OpenSSH-stylessh/scpbehavior. [1][2] - Path/config control: On POSIX, Mutagen uses the first
ssh/scpfound in yourPATH; you can override which OpenSSH client is used withMUTAGEN_SSH_PATH. [1] - Windows note: Mutagen requires you to “bring your own” OpenSSH client on Windows (commonly Git for Windows/MSYS2/Cygwin OpenSSH). PuTTY is not supported. [1]
Sources:
[1] Mutagen docs — SSH transport (requirements/implementation/Windows) (mutagen.io)
[2] Mutagen docs — Overview (agent binaries injected; no manual remote install) (mutagen.io)
Citations:
- 1: https://mutagen.io/documentation/transports/ssh/
- 2: https://mutagen.io/documentation/introduction
🌐 Web query:
Mutagen release tarball contents what files included
💡 Result:
For mutagen-io/mutagen (the Go-based file sync/forwarding tool), “release tarball” can mean two different things:
1) GitHub “Source code (tar.gz)” tarball
This is an auto-generated snapshot of the repository at a tag. It includes the repo contents such as top-level directories like cmd/, pkg/, scripts/, images/, sspl/, tools/, plus files like README.md, LICENSE, BUILDING.md, go.mod, and go.sum. [1]
2) GitHub Release binary .tar.gz assets (per-OS/arch downloads)
These are the manually downloaded release archives referenced by the Mutagen install docs (“download the appropriate release and add its contents to your path”). [2] They contain the compiled Mutagen executable(s) for that OS/architecture (what you actually put on your PATH). [2]
Separately, Mutagen uses agent binaries that end up under ~/.mutagen/agents/<version>/mutagen-agent during use. [3]
Note: This is not the same as the unrelated Python “mutagen” audio-metadata library. [1]
Sources: [1] [2] [3]
🌐 Web query:
Mutagen linux release tarball binary distribution mutagen agent files
💡 Result:
The Mutagen (mutagen-io/mutagen) Linux release tarball (e.g. mutagen_linux_amd64_vX.Y.Z.tar.gz) is a binary distribution that contains:
mutagen— the CLI binarymutagen-agents.tar.gz— a bundled archive of precompiledmutagen-agentbinaries for many platforms/architectures (used for SSH/Docker endpoints) [1][2]
Where the agent files come from / where they live
- In typical installs, the bundled agent archive is located under the install prefix, e.g.
.../libexec/mutagen-agents.tar.gz[3]. (Some Linux packages place it at/usr/libexec/mutagen-agents.tar.gz.) [2] - When connecting to endpoints, Mutagen copies an agent binary to the target (e.g., into a per-version directory under
~/.mutagen/agents/...) and then runs it to perform syncing/forwarding [3][4].
Sources: GitHub release assets listing [1], packaging blueprint showing mutagen-agents.tar.gz installation target [2], manual agent note describing libexec/mutagen-agents.tar.gz [3], and Mutagen docs describing copying agent binaries into containers [4].
🌐 Web query:
Mutagen mutagen-agents.tar.gz required SSH endpoint sync
💡 Result:
For Mutagen file sync over an SSH endpoint, mutagen-agents.tar.gz is the agent bundle that Mutagen uploads to the remote host via scp and then runs via ssh. If that tarball isn’t available on the machine where you run mutagen sync create, the SSH endpoint can’t be initialized (because the remote mutagen-agent can’t be installed/launched). [1]
What to verify/fix:
- OpenSSH client is available locally (Mutagen relies on
ssh+scp). Ifsshisn’t found inPATH, Mutagen fails before it can upload/run the agent. [1][2] - The agent bundle exists where Mutagen expects it, typically in a
libexecdirectory alongside the installed Mutagen (commonly.../libexec/mutagen-agents.tar.gz). [3][4] - Reinstall Mutagen from an official distribution/package so
mutagenand itslibexec/mutagen-agents.tar.gzare installed together (don’t move only themutagenbinary without itslibexec). The WakeMeOps package layout, for example, places it at/usr/libexec/mutagen-agents.tar.gz. [4]
Sources: SSH transport/implementation details (uses scp/ssh) [1]; example failure when ssh missing [2]; agent bundle location notes [3]; example packaged location /usr/libexec/mutagen-agents.tar.gz [4].
🌐 Web query:
mutagen release tarball structure libexec directory layout
💡 Result:
For the Mutagen file-sync tool (mutagen-io), each platform release tarball (e.g. mutagen_linux_amd64_v0.18.1.tar.gz) is intended to be unpacked and then installed as:
mutagen→ an executable placed onPATH(e.g./usr/bin/mutagen).mutagen-agents.tar.gz→ an agents bundle placed in alibexec-style location (e.g./usr/libexec/mutagen-agents.tar.gz). This bundle contains the per-platform/per-archmutagen-agentbinaries that Mutagen deploys/uses remotely. [1][2][3]
So the common libexec layout is effectively:
${prefix}/libexec/mutagen-agents.tar.gz(agents bundle “data”)${prefix}/bin/mutagen(the CLI users run)
Example (Homebrew): the agents bundle is stored under the versioned Cellar path like ${HOMEBREW_CELLAR}/mutagen/<version>/libexec/mutagen-agents.tar.gz. [3]
Sources: GitHub releases (asset naming) [1], packaging blueprint showing mutagen + mutagen-agents.tar.gz install targets [2], and an installation note documenting the Homebrew libexec/mutagen-agents.tar.gz location [3], plus Go package docs showing the agents bundle name mutagen-agents.tar.gz [4].
References:
[1] https://github.com/mutagen-io/mutagen/releases
[2] https://docs.wakemeops.com/packages/mutagen/
[3] https://www.chazeon.com/notes/software/mutagen-agents/
[4] https://pkg.go.dev/github.com/havoc-io/mutagen/pkg/agent
Extract and preserve the full Mutagen release, including the agent bundle required for SSH endpoints.
The release tarball contains both the mutagen CLI binary and mutagen-agents.tar.gz—a bundle of precompiled agent binaries needed for SSH sync endpoints. Moving only the binary to ~/.local/bin/ leaves the agent bundle behind, causing mutagen version to succeed but SSH endpoint setup and sync create operations to fail. The proper installation structure keeps both the binary in bin/ and the agent bundle in libexec/ within the same installation directory.
Suggested approach
-# Extract
-cd /tmp
-tar -xzf mutagen.tar.gz
-
-# Install to user bin
-mkdir -p ~/.local/bin
-mv mutagen ~/.local/bin/
-chmod +x ~/.local/bin/mutagen
+# Extract the full release into a dedicated install directory
+install_dir="$HOME/.local/lib/mutagen"
+bin_dir="$HOME/.local/bin"
+rm -rf "$install_dir"
+mkdir -p "$install_dir" "$bin_dir"
+tar -xzf /tmp/mutagen.tar.gz -C "$install_dir"
+ln -sf "$install_dir/mutagen" "$bin_dir/mutagen"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Extract | |
| cd /tmp | |
| tar -xzf mutagen.tar.gz | |
| # Install to user bin | |
| mkdir -p ~/.local/bin | |
| mv mutagen ~/.local/bin/ | |
| chmod +x ~/.local/bin/mutagen | |
| # Extract the full release into a dedicated install directory | |
| install_dir="$HOME/.local/lib/mutagen" | |
| bin_dir="$HOME/.local/bin" | |
| rm -rf "$install_dir" | |
| mkdir -p "$install_dir" "$bin_dir" | |
| tar -xzf /tmp/mutagen.tar.gz -C "$install_dir" | |
| ln -sf "$install_dir/mutagen" "$bin_dir/mutagen" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install_mutagen.sh` around lines 12 - 19, The script currently extracts
mutagen.tar.gz and only moves the mutagen CLI binary to ~/.local/bin, leaving
mutagen-agents.tar.gz behind and breaking SSH endpoints; update
install_mutagen.sh to preserve the full release by creating a single install
prefix (e.g., ~/.local/mutagen), move the extracted mutagen binary into
PREFIX/bin (keeping the name mutagen), move the mutagen-agents.tar.gz bundle
into PREFIX/libexec (or extract it there), and set executable permissions on
PREFIX/bin/mutagen so both the CLI (mutagen) and the agent bundle
(mutagen-agents.tar.gz) live together (references: tar -xzf mutagen.tar.gz,
mutagen, mutagen-agents.tar.gz, mkdir -p ~/.local/bin, mv mutagen).
Comment on lines
+7
to
+8
| VM_HOST="ubuntu@192.168.122.139" | ||
| VM_PASSWORD="ubuntu" |
There was a problem hiding this comment.
Remove the committed VM password and the host-key bypass before merging.
This script hardcodes a VM credential and then uses it in sshpass while turning off host key verification on Line 24. That is not safe to ship in-repo. Take the host/auth inputs from env or CLI args and require a pinned host key or normal known-host verification.
Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simple_deploy.sh` around lines 7 - 8, Remove the hardcoded VM_HOST and
VM_PASSWORD variables (VM_HOST, VM_PASSWORD) and stop disabling host-key
verification in the ssh/sshpass invocation (the use of StrictHostKeyChecking=no
and UserKnownHostsFile=/dev/null). Instead accept host and credential input via
environment variables or CLI flags (e.g., DEPLOY_VM_HOST and DEPLOY_VM_PASSWORD
or --host/--password) and validate they are present; require normal known_hosts
verification or a pinned host key file provided via an env/flag (e.g.,
DEPLOY_KNOWN_HOSTS) and fail if the key is missing rather than falling back to
insecure options. Ensure the ssh/sshpass call uses the provided host/key
variables and does not include -o StrictHostKeyChecking=no or -o
UserKnownHostsFile=/dev/null so that standard SSH host verification is enforced.
Comment on lines
+201
to
+207
| sshpass -p "$VM_PASSWORD" ssh "$VM_HOST" " | ||
| echo \" - Docker: \$(docker --version 2>/dev/null || echo 'not found')\" | ||
| echo \" - Docker Compose: \$(docker compose version 2>/dev/null || echo 'not found')\" | ||
| echo \" - Mutagen: \$(~/.local/bin/mutagen version 2>/dev/null || echo 'not found')\" | ||
| echo \" - Rust: \$(~/.cargo/bin/rustc --version 2>/dev/null || echo 'not found')\" | ||
| echo " - coastd-dev: $(test -x ~/.local/bin/coastd-dev && echo 'installed' || echo 'not found')" | ||
| " |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f simple_deploy.shRepository: VAIBHAVSING/coasts
Length of output: 78
🏁 Script executed:
sed -n '201,207p' simple_deploy.shRepository: VAIBHAVSING/coasts
Length of output: 557
🏁 Script executed:
sed -n '195,215p' simple_deploy.shRepository: VAIBHAVSING/coasts
Length of output: 920
Escape the $(...) so the status check runs on the VM.
Line 206 uses an unescaped $(test -x ~/.local/bin/coastd-dev ...) inside the double-quoted SSH payload. Unlike lines 202–205, which correctly escape their substitutions as \$(...), this line will evaluate on the local shell before SSH runs. The check therefore reports whether the tool is installed locally, not on the remote VM.
Fix
- echo " - coastd-dev: $(test -x ~/.local/bin/coastd-dev && echo 'installed' || echo 'not found')"
+ echo " - coastd-dev: \$(test -x ~/.local/bin/coastd-dev && echo 'installed' || echo 'not found')"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sshpass -p "$VM_PASSWORD" ssh "$VM_HOST" " | |
| echo \" - Docker: \$(docker --version 2>/dev/null || echo 'not found')\" | |
| echo \" - Docker Compose: \$(docker compose version 2>/dev/null || echo 'not found')\" | |
| echo \" - Mutagen: \$(~/.local/bin/mutagen version 2>/dev/null || echo 'not found')\" | |
| echo \" - Rust: \$(~/.cargo/bin/rustc --version 2>/dev/null || echo 'not found')\" | |
| echo " - coastd-dev: $(test -x ~/.local/bin/coastd-dev && echo 'installed' || echo 'not found')" | |
| " | |
| sshpass -p "$VM_PASSWORD" ssh "$VM_HOST" " | |
| echo \" - Docker: \$(docker --version 2>/dev/null || echo 'not found')\" | |
| echo \" - Docker Compose: \$(docker compose version 2>/dev/null || echo 'not found')\" | |
| echo \" - Mutagen: \$(~/.local/bin/mutagen version 2>/dev/null || echo 'not found')\" | |
| echo \" - Rust: \$(~/.cargo/bin/rustc --version 2>/dev/null || echo 'not found')\" | |
| echo " - coastd-dev: \$(test -x ~/.local/bin/coastd-dev && echo 'installed' || echo 'not found')" | |
| " |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 206-206: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simple_deploy.sh` around lines 201 - 207, The coastd-dev status check is
being evaluated locally because the $(...) substitution inside the SSH payload
is not escaped; update the SSH payload so the coastd-dev check uses a
backslash-escaped substitution (prefix the $(...) with a backslash) so the test
for ~/.local/bin/coastd-dev runs on the remote VM, keeping the surrounding
double-quoted SSH string intact and matching the escaping style used for the
other checks.
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
1 participant
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
Implements Phase 0 of Remote Coasts - the foundational infrastructure for running coast development environments on remote VMs instead of locally.
What's included
State Layer (
coast-daemon/src/state/remotes.rs)Protocol Types (
coast-core/src/protocol/remote.rs)RemoteRequest/RemoteResponsefor remote management (add, remove, list, setup, ping, connect, disconnect)SyncRequest/SyncResponsefor file sync management (status, pause, resume)SSH Tunnel Manager (
coast-daemon/src/remote/tunnel.rs)Remote Setup (
coast-daemon/src/remote/setup.rs)CLI Commands (
coast-cli/src/commands/remote.rs)coast remote add <name> <user@host>- Register a remote VMcoast remote remove <name>- Remove a remotecoast remote ls- List configured remotescoast remote setup <name>- Install coastd on remotecoast remote ping <name>- Check remote connectivitycoast remote connect <name>- Establish SSH tunnelcoast remote disconnect <name>- Close SSH tunnelDaemon Handlers (
coast-daemon/src/handlers/remote.rs)Architecture
Testing
PR Stack
Next Phases
Summary by CodeRabbit
Release Notes
New Features
coast remotecommand suite (add, remove, list, setup, ping, connect, disconnect)coast synccommand suite (create, status, pause, resume, flush, terminate)UI Updates