fix(cli): make dynamic session completion local-only #58

Merged

natsukium merged 13 commits from fix/completion-local-only-42 into main 2026-09-04 10:10:46 +09:00

Conversation 0 Commits 13 Files changed 11 +656 -101

natsukium commented 2026-09-03 23:47:20 +09:00

Owner

Copy link

Fixes #42.

The fish and zsh dynamic-completion overlays forwarded the in-flight
--host / --ssh-arg tokens to the hidden __complete-sessions helper, so
pressing <TAB> on a session-id slot with an SSH destination on the command
line started a real ssh child. That child has no felis-side deadline
(transport timeouts belong to OpenSSH) and inherits the tty for password and
host-key prompts, so an unreachable host or a prompt blocked the user's
interactive shell inside a completion function. It also contradicted
control-surfaces.md, which already documented the helper as local-only.

The overlays now forward only --socket (a retarget's --to-socket) and
emit nothing when the source or retarget target is an SSH destination;
destination slots keep the shell's own host completer. The binary enforces
the same rule twice — in dispatch and in run_complete_sessions — because a
completion script generated by an older build keeps forwarding --host
until the user regenerates it. A short hard timeout plus BatchMode=yes and
a roster cache was considered and rejected: felis would be choosing ssh
policy that belongs to the user's own ssh config.

A follow-up commit fixes an adjacent completion gap: the fish/zsh retarget
table only skipped --format's value inconsistently, so
retarget --to-socket X --format json --session <TAB> offered nothing.
Another keeps the new completion test helpers off non-unix builds so the
crate still compiles there.

Doc cascade:

• docs/explanation/... records the local-only shell-completion contract
  and the "no felis-side ssh deadline" design decision (rejected
  alternatives included), matching control-surfaces.md.
• CHANGELOG.md's Unreleased entry is trimmed to the slot-coverage and
  --socket behavior instead of describing the superseded
  remote-forwarding contract.

Tests drive the generated fish/zsh overlays through the real interpreters
(fish via complete -C, zsh by sourcing under zsh -f) against a
recording felis stub, skipping when the shell binary is unavailable; the
dev shell now carries fish and zsh so CI runs them. The e2e suite pins that
an SSH destination on the line exits 0 with no candidates and never spawns
an ssh stub.

Verified: just check green (fmt, clippy, nextest — 3018 passed / 15
skipped, deny); pi review unavailable (the openai-codex provider is
currently returning Not Found for every model on this account).

Fixes #42. The fish and zsh dynamic-completion overlays forwarded the in-flight `--host` / `--ssh-arg` tokens to the hidden `__complete-sessions` helper, so pressing `<TAB>` on a session-id slot with an SSH destination on the command line started a real `ssh` child. That child has no felis-side deadline (transport timeouts belong to OpenSSH) and inherits the tty for password and host-key prompts, so an unreachable host or a prompt blocked the user's interactive shell inside a completion function. It also contradicted `control-surfaces.md`, which already documented the helper as local-only. The overlays now forward only `--socket` (a retarget's `--to-socket`) and emit nothing when the source or retarget target is an SSH destination; destination slots keep the shell's own host completer. The binary enforces the same rule twice — in dispatch and in `run_complete_sessions` — because a completion script generated by an older build keeps forwarding `--host` until the user regenerates it. A short hard timeout plus `BatchMode=yes` and a roster cache was considered and rejected: felis would be choosing ssh policy that belongs to the user's own ssh config. A follow-up commit fixes an adjacent completion gap: the fish/zsh retarget table only skipped `--format`'s value inconsistently, so `retarget --to-socket X --format json --session <TAB>` offered nothing. Another keeps the new completion test helpers off non-unix builds so the crate still compiles there. Doc cascade: - `docs/explanation/...` records the local-only shell-completion contract and the "no felis-side ssh deadline" design decision (rejected alternatives included), matching `control-surfaces.md`. - `CHANGELOG.md`'s Unreleased entry is trimmed to the slot-coverage and `--socket` behavior instead of describing the superseded remote-forwarding contract. Tests drive the generated fish/zsh overlays through the real interpreters (fish via `complete -C`, zsh by sourcing under `zsh -f`) against a recording `felis` stub, skipping when the shell binary is unavailable; the dev shell now carries fish and zsh so CI runs them. The e2e suite pins that an SSH destination on the line exits 0 with no candidates and never spawns an `ssh` stub. Verified: `just check` green (fmt, clippy, nextest — 3018 passed / 15 skipped, deny); pi review unavailable (the openai-codex provider is currently returning `Not Found` for every model on this account).

natsukium added 5 commits 2026-09-03 23:47:20 +09:00

fix(cli): make dynamic session completion local-only ... d4ac5d5062

The fish and zsh overlays forwarded the in-flight `--host` /
`--ssh-arg` tokens to the hidden `__complete-sessions` helper, so
every `<TAB>` on a session-id slot with an SSH destination on the
line started an `ssh`. That child has no felis-side deadline
(transport timeouts belong to OpenSSH) and inherits the tty for
password and host-key prompts, so an unreachable host or a prompt
blocked the user's interactive shell inside a completion function.
It also contradicted control-surfaces.md, which already called the
helper local-only.

The overlays now forward only `--socket` (a retarget's
`--to-socket`) and emit nothing when the source or retarget target
is an SSH destination; destination slots keep the shell's own host
completer. The binary guards the same rule twice, in the dispatch and
in `run_complete_sessions`, because a completion script generated by
an older build keeps forwarding `--host` until the user regenerates
it. A short hard timeout plus `BatchMode=yes` and a roster cache was
rejected: felis would be choosing ssh policy the user's config owns.

Because the overlays are strings, the tests drive the generated
scripts through the real interpreters (fish via `complete -C`, zsh
by sourcing under `zsh -f` with `words` set) against a recording
`felis` stub; both skip when the shell is missing, so the dev shell
now carries fish and zsh to make CI run them. The e2e suite pins that
`--host` exits 0 with no candidates and never runs an `ssh` stub.

Refs #42

Assisted-by: Claude Code

docs: record the local-only shell completion contract ... 6d335b6357

cli.md and ipc.md described the forwarding the overlays used to do,
while control-surfaces.md stated the helper was local-only without
saying why; the code now matches the explanation, so the reference
twins state the resulting facts and the explanation carries the
rationale, the rejected timeout-plus-cache alternative, and its
revisit trigger. testing.md notes the interpreter-driven overlay
tests and why the dev shell carries fish and zsh.

Refs #42

Assisted-by: Claude Code

docs: record the no-deadline ssh decision the completion rule cites ... 42776ec39a

control-surfaces.md rejects a felis-side completion timeout by
pointing at "transport timeouts belong to OpenSSH", but no page
recorded that decision, so the citation dangled. architecture/ipc.md
now carries it inline (OpenSSH's config owns connection timeouts, and
a felis-side timer would fire under a password or host-key prompt the
inherited tty is still delivering) with its revisit trigger, and
reference/ipc.md states the resulting fact.

The same pages narrated the installed-script migration ("a script
from an older build") that CHANGELOG.md already carries; docs
describe the present contract only, so the sentences now state why
the binary applies the rule to its own arguments (a completion
script is regenerated only by rerunning `felis completions`), and
the dashes the prose norms thin out are gone.

reference/testing.md argued why the overlay tests run the real fish
and zsh; the reference lane states facts, so the argument (a string
assertion pins the script's text, not what the shell does with it)
moves to explanation/testing.md and the reference page links to it.

Refs #42

Assisted-by: Claude Code

fix(cli): keep completion test helpers off non-unix builds ... 5fb37e9d4d

The shell-driven overlay tests are cfg(unix), but the felis stub,
the PATH probe and the tab-line table they use were not, so the
Windows cross clippy job (-D warnings) saw an unconstructed struct,
an unused fn and an unused const in the test module and failed.
The helpers now carry the same gate.

The SSH-carrier guard in run_complete_sessions was pinned only by an
elapsed-time bound, which a fast-failing dial also satisfies; the
test cannot put an ssh stub on PATH because felis-cli forbids unsafe
(no set_var). The carrier's own ssh_args now carry a ProxyCommand
that writes a marker, which a real ssh runs before any network
access, so a spawn is observed directly and the time bound is only
the fallback where ssh is off PATH.

The module doc and the dispatch comment cite control-surfaces.md for
the local-only rationale instead of restating it, so the record has
one home.

Refs #42

Assisted-by: Claude Code

fix(cli): let retarget completion skip --format's value ... 70e8ef362e

The fish and zsh retarget helpers treat any bare word after the verb as
the `felis ssh` destination and stop, so `window retarget --to-socket /y
--format json --session <TAB>` offered nothing even though the local
`--to-socket` roster is the right answer. `--format` is the one
value-taking flag on `RetargetFlags` the skip list missed (via the
flattened `PointFormat`); the shell-driven table now pins that line.

The Unreleased changelog also still described the superseded
remote-forwarding behavior in its earlier "Fixed" entry, next to the
"Changed" entry that replaces it. Nothing has shipped, so the entry is
trimmed to the slot-coverage and `--socket` parts and points at the
local-only contract, rather than presenting both contracts side by
side in the v0.1 notes. The e2e test comment is rephrased as the
present-tense invariant instead of narrating older builds.

Refs #42

Assisted-by: Claude Code

natsukium added 1 commit 2026-09-04 00:14:36 +09:00

chore: re-run CI to rule out a zsh-completion test flake ... a0c2d58107

The pr / cargo build / clippy / test / deny job failed once on
cli_completions::tests::zsh_completion_never_invokes_the_helper_with_an_ssh_carrier
(empty candidates for the plain "felis sessions info " line). The
exact CI zsh (5.9.1, pinned nixpkgs rev 89570f24) reproduces clean
across five full-parallelism felis-cli suite runs locally, so this
looks like a transient CI-runner hiccup rather than a logic bug;
re-running confirms which it is before touching working code.

Assisted-by: Claude Code

natsukium added 1 commit 2026-09-04 00:53:26 +09:00

fix(cli): read the zsh completion helper's roster off a pipe ... 37c5b19276

<(...) resolves through /dev/fd, which a locked-down completion
environment (a container or sandbox with a restricted /proc) need
not provide; when it is unavailable the redirection silently opens
nothing, so felis $carrier __complete-sessions produces no rows and
the candidate list comes back empty. zsh already runs a pipeline's
last command in the current shell rather than a subshell, so piping
into the while-read loop keeps the same append-to-sessions behavior
without depending on /dev/fd at all.

Assisted-by: Claude Code

natsukium added 4 commits 2026-09-04 01:51:15 +09:00

docs(cli): scope the completion silence to the slot's own daemon ... 1cf7dae473

The local-only contract was written as "an SSH destination anywhere on
the line offers no candidates", but a retarget's `--from` names a
session on the daemon the command already speaks to, not on the ssh
target: `felis ssh box --from <TAB>` must keep listing the local
roster, and it does. Only the target slot (`--session`) follows the
`--to-socket`/ssh destination, and the global `--host` silences
everything.

An independent review read the overclaim as a hole in the guard, which
is the cost of a contract stated more broadly than the code keeps.
State the split instead, and pin both halves in `TAB_LINES` so the
next reader gets the answer from a test rather than from the prose.

Refs #42

Assisted-by: Claude Code

test(cli): make the --host completion refusal an observable decision ... 7aebe42358

`--host nowhere __complete-sessions` was asserted to print nothing
while the child had no default daemon to reach, so the assertion held
whether or not the dispatch guard existed: an unreachable local socket
is silent too. Stamping a live daemon as the child's `FELIS_SOCKET`
separates the two — without the guard the child now falls back to that
daemon and prints its roster, which the test catches.

Refs #42

Assisted-by: Claude Code

fix(cli): keep --from completing locally under a retarget's --host ... 1bf9ecc694

The overlays bailed on any `--host` anywhere on the line, but a
retarget carries its own `--host`: it names the daemon the window is
moving *to*, while `--from` names the session on the daemon this
command already speaks to. `felis window retarget --host box --from
<TAB>` and `sessions retarget-all --host=box --from <TAB>` therefore
offered nothing where the local roster is exactly the right answer.
Only the target slot (`--session`) follows the target carrier.

The global carrier flags are not clap globals, so they precede the
subcommand; stopping the scan at the retarget verb is enough to tell
the two spellings apart. Both interpreter-driven overlay tests now
carry the canonical spellings, and fail without the boundary.

The zsh test's per-line assertion also split the whole captured
stdout each time, so one line's candidates satisfied every later
case; it now consumes stdout marker by marker.

Refs #42

Assisted-by: Claude Code

fix(cli): step the completion scan over --ssh-arg's value ... 191b5adaea

The scan stops at the first `ssh` / `retarget` / `retarget-all` token
so a retarget's own target carrier cannot silence a source-daemon
slot. An `--ssh-arg` value is passed to `ssh` verbatim and may be any
word, so `felis --ssh-arg retarget --host box sessions info <TAB>`
ended the scan on the value and offered the local roster despite the
global `--host` that `--ssh-arg` requires. Skipping the value with
its flag closes that: `--host`, `--ssh-arg` and `--socket` are the
only value-taking global flags, and the other two were already
consumed with their values.

The overlay string test now bounds the number of tokens the fish
scanner may echo instead of naming the arms it may not have, since
`--ssh-arg` is a token it must recognize in order to skip.

Refs #42

Assisted-by: Claude Code

natsukium added 1 commit 2026-09-04 02:36:13 +09:00

fix(cli): read the zsh roster through command substitution ... 3c0dbf4b19

CI's `cargo test` job failed the zsh completion driver on every line
that expects candidates: `_describe` received an empty array, so the
`while read` loop's appends never reached the enclosing function. The
pipe form assumed zsh always runs a pipeline's last stage in the
calling shell; that held locally but not on the CI runner, and a
subshell there silently swallows the roster. Command substitution
carries no such assumption and still avoids the `/dev/fd` requirement
that pushed the helper off process substitution in the first place.

The test's failure message now carries the recorded helper calls and
zsh's stderr, so a future empty roster says whether the stub ran at
all instead of only that the candidates were missing.

Assisted-by: Claude Code

natsukium added 1 commit 2026-09-04 09:18:08 +09:00

fix(cli): pin the stub on PATH inside the zsh completion driver ... 72f042d405

CI's `cargo test` job kept failing the zsh completion driver on every
line that expects candidates, with no helper call recorded at all. The
cause is not the overlay: `zsh -f` still reads `/etc/zshenv`, and on a
NixOS host that file replaces `PATH` wholesale, so the `felis` stub the
test puts in the child's environment disappears before the overlay
runs. On the CI runner nothing then answers `felis` and the roster is
empty; on a developer machine the *real* `felis` answers instead and
the test passes for the wrong reason.

The driver now prepends the stub directory to `path` after the startup
files have run, and refuses to continue when `felis` still does not
resolve — the helper's own `2>/dev/null` otherwise turns a lost stub
into an empty roster indistinguishable from the SSH refusal the test is
actually pinning.

Assisted-by: Claude Code

natsukium merged commit a4e8c1dfa3 into main 2026-09-04 10:10:46 +09:00

natsukium deleted branch fix/completion-local-only-42 2026-09-04 10:10:46 +09:00

natsukium referenced this pull request 2026-09-08 01:39:59 +09:00

[v0.1/CLI Args Review] CLI 引数のリリース前設計レビュー #55

Sign in to join this conversation.

Reviewers

No reviewers

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

Subscribe

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!58

No description provided.