natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P1] Make dynamic session completion local-only #42

New issue
Closed
opened 2026-09-03 16:30:52 +09:00 by natsukium · 2 comments
natsukium commented 2026-09-03 16:30:52 +09:00
Owner
Copy link

Parent: #12 (P1). This issue retains the actionable completion finding from the SSH and CLI-argument reviews; the broader SSH spawn and timeout questions are already settled by the control-surface and carrier design records.

Why

Fish and zsh invoke the hidden __complete-sessions helper on <TAB>. The generated overlays currently forward --host and --ssh-arg, so completion can start an interactive SSH connection with no felis-side deadline. An unreachable host, password prompt, or host-key prompt can therefore block the user's shell.

The implementation also contradicts docs/explanation/architecture/control-surfaces.md, which says the hidden helper stays local-only because it runs once per <TAB>. docs/reference/ipc.md currently describes the implemented remote behavior and must be corrected with the code.

Decision

Dynamic session completion must never start SSH. Keep local daemon completion, including an explicitly selected local --socket, but emit no session candidates when the in-flight source or retarget target is an SSH destination. SSH destination fields continue to use the shell's own host-name completion.

Do not add a general felis SSH timeout or a headless --spawn flag here. OpenSSH owns transport timeout and multiplexing policy, and the existing session-verb versus window/spawn autostart split is already intentional and documented.

Acceptance criteria

  • Pressing <TAB> with an in-flight --host or SSH retarget target never executes ssh or another network operation.
  • Session-id completion still works for the default local daemon and an explicit local --socket.
  • Fish and zsh overlay tests prove that SSH carrier tokens are not forwarded to __complete-sessions.
  • SSH destination completion still delegates to the shell's local host completer.
  • docs/reference/cli.md, docs/reference/ipc.md, docs/explanation/architecture/control-surfaces.md, generated completions, and CHANGELOG.md describe the same local-only contract.
Parent: #12 (P1). This issue retains the actionable completion finding from the SSH and CLI-argument reviews; the broader SSH spawn and timeout questions are already settled by the control-surface and carrier design records. ## Why Fish and zsh invoke the hidden `__complete-sessions` helper on `<TAB>`. The generated overlays currently forward `--host` and `--ssh-arg`, so completion can start an interactive SSH connection with no felis-side deadline. An unreachable host, password prompt, or host-key prompt can therefore block the user's shell. The implementation also contradicts `docs/explanation/architecture/control-surfaces.md`, which says the hidden helper stays local-only because it runs once per `<TAB>`. `docs/reference/ipc.md` currently describes the implemented remote behavior and must be corrected with the code. ## Decision Dynamic session completion must never start SSH. Keep local daemon completion, including an explicitly selected local `--socket`, but emit no session candidates when the in-flight source or retarget target is an SSH destination. SSH destination fields continue to use the shell's own host-name completion. Do not add a general felis SSH timeout or a headless `--spawn` flag here. OpenSSH owns transport timeout and multiplexing policy, and the existing session-verb versus window/spawn autostart split is already intentional and documented. ## Acceptance criteria - [ ] Pressing `<TAB>` with an in-flight `--host` or SSH retarget target never executes `ssh` or another network operation. - [ ] Session-id completion still works for the default local daemon and an explicit local `--socket`. - [ ] Fish and zsh overlay tests prove that SSH carrier tokens are not forwarded to `__complete-sessions`. - [ ] SSH destination completion still delegates to the shell's local host completer. - [ ] `docs/reference/cli.md`, `docs/reference/ipc.md`, `docs/explanation/architecture/control-surfaces.md`, generated completions, and `CHANGELOG.md` describe the same local-only contract.
natsukium commented 2026-09-03 16:46:07 +09:00
Author
Owner
Copy link

CLI args focused review (#55) で timeout scope に completion 経路の抜けを確認した。

fish/zsh overlay は session-id slot の <TAB> ごとに felis ... __complete-sessions を起動し、in-flight --host を転送して SSH dial する。connect_stdio_command は SSH stderr/TTY を継承し felis-side timeout を持たないため、不達 host、password、host-key prompt で対話シェルの completion 自体が停止し得る。

SSH timeout 方針を決める際は通常 verb だけでなく hidden completion dial も対象にしたい。候補は remote dynamic completion を削る/local-only に戻す、または短い hard timeout + noninteractive SSH + cache。なお explanation/architecture/control-surfaces.md は helper を local-only と書く一方、実装と reference/cli.md は remote forwarding を契約化しており docs も不整合。

CLI args focused review (#55) で timeout scope に completion 経路の抜けを確認した。 fish/zsh overlay は session-id slot の `<TAB>` ごとに `felis ... __complete-sessions` を起動し、in-flight `--host` を転送して SSH dial する。`connect_stdio_command` は SSH stderr/TTY を継承し felis-side timeout を持たないため、不達 host、password、host-key prompt で対話シェルの completion 自体が停止し得る。 SSH timeout 方針を決める際は通常 verb だけでなく hidden completion dial も対象にしたい。候補は remote dynamic completion を削る/local-only に戻す、または短い hard timeout + noninteractive SSH + cache。なお `explanation/architecture/control-surfaces.md` は helper を local-only と書く一方、実装と `reference/cli.md` は remote forwarding を契約化しており docs も不整合。
natsukium changed title from [v0.1/SSH Review] SSH stdio のタイムアウト/ハンドシェイク/スポーン方針の未統一 to [v0.1/P1] Make dynamic session completion local-only 2026-09-03 16:57:31 +09:00
natsukium commented 2026-09-03 20:35:18 +09:00
Author
Owner
Copy link

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12.

Claim check

Accurate at HEAD, including the docs contradiction.

  • The overlays forward the SSH carrier. fish __felis_carrier_flags echoes every in-flight --host/--socket/--ssh-arg token (crates/felis-cli/src/cli_completions.rs:131-152), and __felis_complete_retarget_target_sessions rebuilds --host <dest> from the retarget verb's --host, the ssh alias's positional, and --ssh-arg (154-199). zsh does the same in _felis_complete_sessions (255-283) and _felis_complete_retarget_target_sessions (285-327). The module doc states it as intended (cli_completions.rs:10-13), and the unit test fish_overlay_covers_option_slots_and_forwards_the_carrier (500-516) pins the forwarding.
  • The helper dials whatever carrier it is handed. Cmd::CompleteSessions resolves cli.host/cli.ssh_arg (main.rs:443-454) and run_complete_sessions calls conn::dial (cli_completions.rs:70-79) → connect_carrier(…, RemoteSpawn::Refuse) (conn.rs:103-105) → connect_stdio_command for Carrier::Ssh (felis-client-core/src/connector.rs:466-470, 252-266).
  • No felis-side deadline and an interactive stderr/tty. connect_stdio_command leaves cmd.stderr at the parent default by design so password prompts reach the user (connector.rs:249-251); there is no timeout around the spawn or the handshake. ROSTER_FETCH_TIMEOUT (dial.rs:199) covers only the GUI attach roster, not this path. The overlays' 2>/dev/null hides ssh's stderr but ssh still opens /dev/tty for a password or host-key prompt, so the <TAB> blocks inside the interactive shell.
  • Docs disagree. docs/explanation/architecture/control-surfaces.md:412-416: the helper "stays local-only because it runs once per shell <TAB>". docs/reference/ipc.md:1611-1613 and docs/reference/cli.md:733-741: the overlays forward --host/--socket/--ssh-arg so the roster comes from the daemon the verb will dial. The reference pages describe the code; the explanation describes the intent.
  • The #55 comment's timeout scope is correctly narrowed by the issue: #38's closed record keeps transport timeouts with OpenSSH; #44 folded the headless --spawn question away. Nothing to reopen.

Nothing already fixed.

Verdict

accept. Principle 1 test: the shell's own host completer already owns SSH destinations, and the daemon roster is the one candidate source only felis can provide; both stay. Removing the network hop on <TAB> adds no capability and removes an unbounded, interactive side effect from a keystroke. Principle 4 (explicit): the contract becomes one sentence rather than "sometimes remote, sometimes not, depending on what is on the line".

Approach

Code (crates/felis-cli)

  • main.rs:443-454 Cmd::CompleteSessions: resolve the target with host = None, ssh_args = &[] regardless of what the root flags carry, and exit 0 with no output when cli.host.is_some() (the local roster would be wrong candidates for a remote verb). Two layers on purpose: an already-installed overlay from an older build keeps forwarding --host, and the binary must still never dial ssh from it.
  • cli_completions.rs:70-79 run_complete_sessions: guard if matches!(target.carrier, Carrier::Ssh { .. }) { return 0; } before dial, so the function itself is safe whatever the caller resolved. Update the module doc (10-16) and the fn doc (67-69).
  • fish overlay: __felis_carrier_flags (131-148) forwards --socket/--socket=* only and, on seeing --host/--host=*, returns a sentinel so __felis_complete_sessions (150-152) emits nothing. __felis_complete_retarget_target_sessions (154-199): keep the --to-socket → --socket mapping; on --host/positional destination (after #23: --to-host) emit nothing; drop the ssh_args accumulator. The -l host -a '(__fish_complete_user_at_hosts)' and ssh destination lines (402-404) stay (shell-local host completion is untouched).
  • zsh overlay: same edits in _felis_complete_sessions (258-274) and _felis_complete_retarget_target_sessions (290-318); the _hosts substitutions (365-366) stay.
  • No --spawn, no felis-side ssh timeout (per the issue's decision; record the rejection in the explanation twin with the pointer to #38's OpenSSH-owns-timeouts record).

Tests

  • cli_completions.rs unit tests: replace fish_overlay_covers_option_slots_and_forwards_the_carrier (500-516) with fish_overlay_forwards_only_the_local_socket (assert the case list contains --socket and not --host/--ssh-arg; assert the retarget helper contains no --host branch that invokes felis); same for ZSH_OVERLAY (zsh_overlay_routes_option_slots_by_value_name, 521-531). Because the overlays are strings, add a stronger executable check where the interpreter exists: a #[test] that skips unless fish/zsh is on PATH, sources the generated script with felis replaced by a stub that records its argv, simulates felis --host box sessions info <TAB>, and asserts the stub was never invoked with --host (the flake's dev shell can carry both shells so CI runs it; note this in testing.md).
  • run_complete_sessions unit test: a Reconnector with Carrier::Ssh { destination: "nowhere", .. } and PATH="" returns 0 immediately, prints nothing, and spawns no child (measure elapsed < 1s; a spawn attempt would fail on NotFound but the point is no spawn at all — assert via a ssh stub on a temp PATH that writes a marker file, and check it is absent).
  • e2e (tests/cli_sessions.rs): felis --socket <sock> __complete-sessions still lists the fixture daemon's session; felis --host nowhere __complete-sessions exits 0 with empty stdout in well under a second.

Cascade

  • docs/reference/cli.md:733-741 completions bullet: "fish/zsh complete session ids from the default local daemon or an explicit --socket; with an SSH destination on the line no session candidates are offered; destination slots use the shell's own host completer".
  • docs/reference/ipc.md:1611-1613: replace "even the hidden helper honors them" with the local-only sentence.
  • docs/explanation/architecture/control-surfaces.md:412-416: keep the claim and give it its rationale inline: once per <TAB>, ssh has no felis-side deadline (owned by OpenSSH, #38 record in architecture/ipc.md), and an interactive prompt from a completion function blocks the shell; rejected: a short hard timeout + BatchMode=yes + cache (felis would be choosing ssh policy the user's config already owns, and a cache is state the completion path has no place for). Revisit if OpenSSH grows a non-interactive probe felis can call without owning policy.
  • cli_completions.rs module doc; CHANGELOG.md "Changed: shell completion no longer dials a remote daemon on <TAB>".
  • skills/felis/SKILL.md: no mention of completion today (grep -n complet finds only unrelated words), so no change.
  • __mangen: the hidden verb is excluded (cli_mangen.rs:6-7), no change. just schema: not affected.

Dependencies

  • None hard. Land before #23: #23 renames --host → --to-host on the retarget verbs and deletes felis ssh, which rewrites the same overlay functions; doing #42 first turns #23's overlay work into a token rename and deletes the alias branch it would otherwise have to port. If #23 lands first, fold this into its overlay rewrite instead of a separate pass.
  • #38's record (closed, no code change) is the source for "OpenSSH owns timeouts"; cite it rather than restating.

Risk/effort

S. Two shell-script strings, one guard, one dispatch tweak, three doc sites. Main risk: an installed older completion script keeps forwarding --host until the user regenerates it; the binary-side guard covers that. Secondary: the zsh overlay's substitution anchors (cli_completions.rs:512-535) are unchanged by this issue, so the tripwire test (536-549) still holds.

Labels

Keep priority/P1, release/v0.1.0 (the #12 comment already lists it as a closure dependency). It is small enough to land ahead of the larger CLI items and unblocks a cleaner #23.

## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. ## Claim check Accurate at HEAD, including the docs contradiction. - **The overlays forward the SSH carrier.** fish `__felis_carrier_flags` echoes every in-flight `--host`/`--socket`/`--ssh-arg` token (`crates/felis-cli/src/cli_completions.rs:131-152`), and `__felis_complete_retarget_target_sessions` rebuilds `--host <dest>` from the retarget verb's `--host`, the `ssh` alias's positional, and `--ssh-arg` (`154-199`). zsh does the same in `_felis_complete_sessions` (`255-283`) and `_felis_complete_retarget_target_sessions` (`285-327`). The module doc states it as intended (`cli_completions.rs:10-13`), and the unit test `fish_overlay_covers_option_slots_and_forwards_the_carrier` (`500-516`) pins the forwarding. - **The helper dials whatever carrier it is handed.** `Cmd::CompleteSessions` resolves `cli.host`/`cli.ssh_arg` (`main.rs:443-454`) and `run_complete_sessions` calls `conn::dial` (`cli_completions.rs:70-79`) → `connect_carrier(…, RemoteSpawn::Refuse)` (`conn.rs:103-105`) → `connect_stdio_command` for `Carrier::Ssh` (`felis-client-core/src/connector.rs:466-470, 252-266`). - **No felis-side deadline and an interactive stderr/tty.** `connect_stdio_command` leaves `cmd.stderr` at the parent default by design so password prompts reach the user (`connector.rs:249-251`); there is no timeout around the spawn or the handshake. `ROSTER_FETCH_TIMEOUT` (`dial.rs:199`) covers only the GUI attach roster, not this path. The overlays' `2>/dev/null` hides ssh's stderr but ssh still opens `/dev/tty` for a password or host-key prompt, so the `<TAB>` blocks inside the interactive shell. - **Docs disagree.** `docs/explanation/architecture/control-surfaces.md:412-416`: the helper "stays local-only because it runs once per shell `<TAB>`". `docs/reference/ipc.md:1611-1613` and `docs/reference/cli.md:733-741`: the overlays forward `--host`/`--socket`/`--ssh-arg` so the roster comes from the daemon the verb will dial. The reference pages describe the code; the explanation describes the intent. - The #55 comment's timeout scope is correctly narrowed by the issue: #38's closed record keeps transport timeouts with OpenSSH; #44 folded the headless `--spawn` question away. Nothing to reopen. Nothing already fixed. ## Verdict **accept.** Principle 1 test: the shell's own host completer already owns SSH destinations, and the daemon roster is the one candidate source only felis can provide; both stay. Removing the network hop on `<TAB>` adds no capability and removes an unbounded, interactive side effect from a keystroke. Principle 4 (explicit): the contract becomes one sentence rather than "sometimes remote, sometimes not, depending on what is on the line". ## Approach ### Code (`crates/felis-cli`) - `main.rs:443-454` `Cmd::CompleteSessions`: resolve the target with `host = None, ssh_args = &[]` regardless of what the root flags carry, **and** exit 0 with no output when `cli.host.is_some()` (the local roster would be wrong candidates for a remote verb). Two layers on purpose: an already-installed overlay from an older build keeps forwarding `--host`, and the binary must still never dial ssh from it. - `cli_completions.rs:70-79` `run_complete_sessions`: guard `if matches!(target.carrier, Carrier::Ssh { .. }) { return 0; }` before `dial`, so the function itself is safe whatever the caller resolved. Update the module doc (`10-16`) and the fn doc (`67-69`). - fish overlay: `__felis_carrier_flags` (`131-148`) forwards `--socket`/`--socket=*` only and, on seeing `--host`/`--host=*`, returns a sentinel so `__felis_complete_sessions` (`150-152`) emits nothing. `__felis_complete_retarget_target_sessions` (`154-199`): keep the `--to-socket` → `--socket` mapping; on `--host`/positional destination (after #23: `--to-host`) emit nothing; drop the `ssh_args` accumulator. The `-l host -a '(__fish_complete_user_at_hosts)'` and `ssh` destination lines (`402-404`) stay (shell-local host completion is untouched). - zsh overlay: same edits in `_felis_complete_sessions` (`258-274`) and `_felis_complete_retarget_target_sessions` (`290-318`); the `_hosts` substitutions (`365-366`) stay. - No `--spawn`, no felis-side ssh timeout (per the issue's decision; record the rejection in the explanation twin with the pointer to #38's OpenSSH-owns-timeouts record). ### Tests - `cli_completions.rs` unit tests: replace `fish_overlay_covers_option_slots_and_forwards_the_carrier` (`500-516`) with `fish_overlay_forwards_only_the_local_socket` (assert the `case` list contains `--socket` and not `--host`/`--ssh-arg`; assert the retarget helper contains no `--host` branch that invokes `felis`); same for `ZSH_OVERLAY` (`zsh_overlay_routes_option_slots_by_value_name`, `521-531`). Because the overlays are strings, add a stronger executable check where the interpreter exists: a `#[test]` that skips unless `fish`/`zsh` is on `PATH`, sources the generated script with `felis` replaced by a stub that records its argv, simulates `felis --host box sessions info <TAB>`, and asserts the stub was never invoked with `--host` (the flake's dev shell can carry both shells so CI runs it; note this in `testing.md`). - `run_complete_sessions` unit test: a `Reconnector` with `Carrier::Ssh { destination: "nowhere", .. }` and `PATH=""` returns 0 immediately, prints nothing, and spawns no child (measure elapsed < 1s; a spawn attempt would fail on NotFound but the point is no spawn at all — assert via a `ssh` stub on a temp `PATH` that writes a marker file, and check it is absent). - e2e (`tests/cli_sessions.rs`): `felis --socket <sock> __complete-sessions` still lists the fixture daemon's session; `felis --host nowhere __complete-sessions` exits 0 with empty stdout in well under a second. ### Cascade - `docs/reference/cli.md:733-741` completions bullet: "fish/zsh complete session ids from the default local daemon or an explicit `--socket`; with an SSH destination on the line no session candidates are offered; destination slots use the shell's own host completer". - `docs/reference/ipc.md:1611-1613`: replace "even the hidden helper honors them" with the local-only sentence. - `docs/explanation/architecture/control-surfaces.md:412-416`: keep the claim and give it its rationale inline: once per `<TAB>`, ssh has no felis-side deadline (owned by OpenSSH, #38 record in `architecture/ipc.md`), and an interactive prompt from a completion function blocks the shell; rejected: a short hard timeout + `BatchMode=yes` + cache (felis would be choosing ssh policy the user's config already owns, and a cache is state the completion path has no place for). *Revisit if* OpenSSH grows a non-interactive probe felis can call without owning policy. - `cli_completions.rs` module doc; `CHANGELOG.md` "Changed: shell completion no longer dials a remote daemon on `<TAB>`". - `skills/felis/SKILL.md`: no mention of completion today (`grep -n complet` finds only unrelated words), so no change. - `__mangen`: the hidden verb is excluded (`cli_mangen.rs:6-7`), no change. `just schema`: not affected. ## Dependencies - None hard. **Land before #23**: #23 renames `--host` → `--to-host` on the retarget verbs and deletes `felis ssh`, which rewrites the same overlay functions; doing #42 first turns #23's overlay work into a token rename and deletes the alias branch it would otherwise have to port. If #23 lands first, fold this into its overlay rewrite instead of a separate pass. - #38's record (closed, no code change) is the source for "OpenSSH owns timeouts"; cite it rather than restating. ## Risk/effort **S.** Two shell-script strings, one guard, one dispatch tweak, three doc sites. Main risk: an installed older completion script keeps forwarding `--host` until the user regenerates it; the binary-side guard covers that. Secondary: the zsh overlay's substitution anchors (`cli_completions.rs:512-535`) are unchanged by this issue, so the tripwire test (`536-549`) still holds. ## Labels Keep **priority/P1**, **release/v0.1.0** (the #12 comment already lists it as a closure dependency). It is small enough to land ahead of the larger CLI items and unblocks a cleaner #23.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#42
No description provided.