add SSH environments from the composer - #274
Conversation
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. The remote-host lifecycle can be overwritten by stale connection completions and status events, the legacy lock fallback can reclaim a live successor lock, and reserved selector values collide with valid SSH aliases. The changed tests do not honestly exercise one pending-connect race. This is a meaningful UI change, but the supplied PR evidence contains no screenshots or screen recording; please add screenshots or a short screen recording. GitHub check evidence is present and currently pending, so required checks still govern merge readiness.
Deterministic publication result: 3 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.
🤖 Blocking · Screenshots needed
This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.
| : {}), | ||
| ...(payload.error ? { error: payload.error } : {}), | ||
| set((state) => { | ||
| if (state.forgottenHosts[payload.host]) return state; |
There was a problem hiding this comment.
🤖 P1 · Stale lifecycle updates win (blocking)
A new connection clears the forget tombstone and increments lifecycleByHost, but applyStatusEvent accepts every later event while the tombstone is clear, and ensureHostConnected applies success or failure without checking that its captured lifecycle is still current. A delayed event or completion from an older connection can therefore overwrite a replacement lifecycle or restore a host after it was forgotten.
User effect: A newly reconnected SSH environment can suddenly appear failed or disconnected, and a host the user forgot can reappear after an older connection finishes.
Recommended fix: Carry the backend connection generation or instance identity through status events and connection results, then apply renderer updates only when they match the host's current lifecycle and the host is not tombstoned.
Test: Add discriminating tests that complete two connection lifecycles out of order, deliver a stale status event after reconnect, and complete an admitted connection after Forget; assert that only the newest live lifecycle remains visible and persisted.
| # distinguish that window from a process that died before publication. | ||
| if [ "$stale_observations" -ge 20 ]; then | ||
| legacy_claim="$STATE_DIR/.daemon.lock.legacy.$NONCE.$$.$compat_attempt" | ||
| if mv "$LEGACY_LOCK_DIR" "$legacy_claim" 2>/dev/null; then |
There was a problem hiding this comment.
🤖 P1 · Legacy lock reclaims successor (blocking)
After repeated stale-owner observations, the compatibility path renames the shared daemon.lock pathname without proving it is still the lock generation that was observed. A live process can replace or recreate that directory between the last observation and mv, causing this process to move and delete the successor's live lock.
User effect: Two remote-backend operations can run as if each owns the lock, producing unreliable connection state or corrupting the daemon's shared records.
Recommended fix: Make legacy reclamation generation-specific, or perform an atomic claim with an identity check that cannot cross pathname reuse; never remove the shared path based only on earlier observations.
Test: Add a race test that replaces the observed stale legacy lock with a live successor immediately before reclamation and proves the successor lock remains intact and exclusive.
| event.preventDefault(); | ||
| if (pending) return; | ||
|
|
||
| const host = hostDraft.trim(); |
There was a problem hiding this comment.
🤖 P1 · SSH aliases collide with actions (blocking)
The add dialog accepts any non-empty SSH host, while the selector reserves literal string values for the local destination and Add action in the same namespace used by host values. A valid SSH config alias matching either reserved value is interpreted as a command instead of a host.
User effect: People with a valid alias named local or add_ssh_environment cannot select that remote machine; Berd silently chooses the local computer or reopens the add dialog instead.
Recommended fix: Represent selector entries as discriminated host and action values so command identity cannot collide with user-controlled host names. If the component cannot support that, validate and clearly reject reserved aliases at first ingress.
Test: Add selector tests for both reserved-looking aliases and verify each can be added and selected as a host without triggering local selection or the Add action.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn forget_removes_inactive_slot_by_its_exact_key() { |
There was a problem hiding this comment.
🤖 P2 · Race test is vacuous (non-blocking)
The test named pending_connect_observes_tombstone_after_forget retains an Arc, calls forget, and inspects the forgotten flag, but never starts a connect, blocks it on connect_lock, or verifies that an admitted pending connection is superseded without establishment side effects. It can pass even if the pending-connect guard is removed or misplaced.
User effect: A future regression could let a forgotten host finish connecting again while the test suite continues to report that the race is protected.
Recommended fix: Exercise the real ordering: hold the connection lock, admit a connect, forget the host, release the lock, and assert the connect is superseded and no tunnel or ready state is established.
Test: Rewrite this test as the described controlled pending-connect race and assert both the returned outcome and absence of connection side effects.
2 participants
Category: improvement
User Impact: Users can add and select a new SSH environment directly from the chat composer.
Problem: The environment picker only lists SSH hosts Berd already knows about, so adding a new host requires leaving the chat flow and finding the remote-host settings. Solution: Add a separated “Add SSH environment” action that opens an accessible connection dialog, accepts an SSH config alias or
user@host, selects the environment after a successful connection, and keeps loading and error feedback in context. This PR is stacked on #269 and should merge after it.File changes
src/features/chat/ui/AddRemoteHostDialog.tsx
Adds the focused connection dialog, including validation, pending-state protection, backend error feedback, and selection after a successful connection.
src/features/chat/ui/RemoteHostSelector.tsx
Adds the separated “Add SSH environment” menu action and preserves focus while transitioning from the environment menu into the dialog.
src/features/chat/ui/tests/RemoteHostSelector.test.tsx
Covers the empty-host menu, successful add-and-select flow, and retained dialog feedback when connection fails.
src/shared/i18n/locales/en/chat.json
Adds localized copy and accessible labels for the new environment action and dialog.