Repository navigation
dl looks for devpod's ssh config where devpod writes it - #433
Conversation
`dl <ws> -- <cmd>` runs over OpenSSH through the alias `devpod up` publishes, and `config_path()` decided whether that alias existed by reading a hardcoded `~/.ssh/config`. devpod does not necessarily write there. It targets the `SSH_CONFIG_INCLUDE_PATH` context option, else `--ssh-config` (whose default `inheritFlagsFromEnvironment` fills in from `$DEVPOD_SSH_CONFIG`), else the `SSH_CONFIG_PATH` context option, else `~/.ssh/config` -- and it rewrites whichever it picks and creates no other. So on a host exporting `DEVPOD_SSH_CONFIG` there was no `~/.ssh/config` to find at all, dl concluded the workspace had no alias, dropped every command to `devpod ssh --command`, and the only thing it said was that the workspace wanted restarting. Three parts. `ssh::config_path` takes a `ConfigSources` and resolves devpod's four candidates in devpod's order, expanding a leading `~/` the way `ResolveSSHConfigPath` does and reading an empty value as unset. Four fields rather than four arguments because they are all optional paths and a transposed argument list is the defect this fixes. The two context-option paths come from `already_cached_options`, which reads the disk cache and nothing else. `terminal_for` runs on every launch including the warm ones, and `devpod context options` costs 0.4-0.7s (#393) against a transport worth less than that; a cache miss answers with devpod's defaults, which is what dl assumed unconditionally before. That "no round trip" is settled by the signature rather than by a test: neither function takes a `Runner`. And `Terminal::NoAlias` splits. It was two facts wearing one name, and that is what let this ship: "this config has no entry for this workspace" and "there is no config where devpod publishes" reached the user as one sentence about restarting, which fixes the first and cannot touch the second. Now `NoAlias { config }`, `ConfigMissing { looked_in }` and `ConfigUnlocatable`, each with its own notice, each naming the path dl read or saying why there is none. `devpod_host_configured`'s bool becomes `ssh::Alias`, the three-arm answer those three states are read off. The e2e harness stops working around it. `route_ssh_through` symlinked a scratch `~/.ssh/config` onto the run's config so dl's hardcoded lookup would find it; that symlink is gone, so the suite's scratch home holds no ssh config and dl finding the alias anyway is what says the resolution order still holds. OpenSSH still needs its `-F` shim, which reads no environment variable of devpod's. Measurement consequence: scratch-scoped benchmark runs set `DEVPOD_SSH_CONFIG` (this repo's own convention, `test/conftest.py`, `scripts/bench_launch.py`), so they have been silently on the non-pty transport. Before/after numbers taken that way are not comparable across this change. Closes part 1 of #390's resolution; the ticket is #421.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideFixes the shipped pty-routing bug by making dl locate devpod’s published SSH aliases according to devpod’s precedence rules, using cached context options without adding a warm-launch round trip, and reporting missing aliases versus missing config locations distinctly. Regression tests and e2e harness changes ensure the configured path—not a hardcoded ~/.ssh/config—is exercised. Sequence diagram for pty routing through devpod SSH aliasessequenceDiagram
participant Launch as dl launch
participant Cache as Cached context options
participant Resolver as SSH config resolver
participant Config as Selected SSH config
participant OpenSSH as OpenSSH
participant Devpod as devpod ssh
Launch->>Cache: already_cached_options()
Launch->>Resolver: config_path(ConfigSources)
Resolver-->>Launch: selected config path
Launch->>Config: alias_in(config, workspace_id)
Config-->>Launch: Alias::Published
Launch->>OpenSSH: ssh -t workspace alias
OpenSSH->>Devpod: ProxyCommand devpod ssh --stdio
Devpod-->>OpenSSH: session transport
OpenSSH-->>Launch: interactive pty session
State diagram for SSH alias and terminal outcomesstateDiagram-v2
[*] --> TerminalCheck
TerminalCheck --> Absent: tty disabled or unavailable
TerminalCheck --> ConfigResolution: usable terminal
ConfigResolution --> Usable: alias published
ConfigResolution --> NoAlias: config readable, workspace absent
ConfigResolution --> ConfigMissing: selected config unreadable
ConfigResolution --> ConfigUnlocatable: no path available
Usable --> OpenSSH: command with pty
NoAlias --> DevpodCommand: fallback and restart notice
ConfigMissing --> DevpodCommand: fallback and config-path notice
ConfigUnlocatable --> DevpodCommand: fallback and unlocatable notice
Flow diagram for devpod SSH config resolutionflowchart TD
A[Start config_path] --> B{SSH_CONFIG_INCLUDE_PATH set?}
B -- Yes --> C[Use include path]
B -- No --> D{DEVPOD_SSH_CONFIG set?}
D -- Yes --> E[Use environment path]
D -- No --> F{SSH_CONFIG_PATH set?}
F -- Yes --> G[Use context path]
F -- No --> H{Home directory available?}
H -- Yes --> I[Use ~/.ssh/config]
H -- No --> J[ConfigUnlocatable]
C --> K[Expand leading ~/]
E --> K
G --> K
K --> L[Read selected SSH config]
I --> L
L --> M{Workspace alias found?}
M -- Yes --> N[Published]
M -- No --> O[WorkspaceAbsent]
L --> P[Read failed]
P --> Q[ConfigMissing]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
Reviewed in fresh context, having neither written nor read this change before. Two axes: correctness (including the headline question — does dl now read the file devpod actually writes?) and repo standards/guards.
The precedence order is right, and the transport still cannot run. config_path matches skevetter/devpod v0.26.1 exactly. But terminal_for now says Usable off a config that OpenSSH will not read, and ssh::command_args still emits no -F — so on the very hosts this PR is for, dl <ws> -- <cmd> at a terminal stops running at all rather than running without a pty. One blocking finding, one contradiction, everything else clean.
This was generated by AI during review.
Axis 1 — correctness
1. Verified against upstream: the precedence order is devpod's
Read at skevetter/devpod tag v0.26.1, not from the PR body. All four rungs check out, in this order:
pkg/ssh/config.go:41-47—targetPath := params.SSHConfigPath; if params.SSHConfigIncludePath != "" { targetPath = params.SSHConfigIncludePath }, then onewriteSSHConfig(targetPath, ...). So the include path wins outright, and only the chosen file is written — no second file, noIncludedirective injected (devpod's ownpkg/config/context.go:94-95tells the user to add that line).cmd/up.go:130—StringVar(&cmd.SSHConfigPath, "ssh-config", "", ...);cmd/root.go:164-175—suffix := strings.ToUpper(strings.ReplaceAll(flag.Name, "-", "_"))withconfig.EnvPrefix = "DEVPOD_"(pkg/config/env.go:86). Sossh-config→DEVPOD_SSH_CONFIG, exactly as the body claims, and the mechanism claimed for it is real.cmd/up.go:800-802—if cmd.SSHConfigPath == "" { cmd.SSHConfigPath = devPodConfig.ContextOption(config.ContextOptionSSHConfigPath) }.SSH_CONFIG_PATHis a fallback for the flag only, which is whatConfigSourcesencodes.pkg/ssh/config.go:359-373—ResolveSSHConfigPath: empty →filepath.Join(homeDir, ".ssh", "config"), andstrings.HasPrefix(sshConfigPath, "~/")→strings.Replace(..., "~", homeDir, 1).expand_home's "first~of a~/prefix and nothing else" is byte-for-byte that rule, and empty-as-unset is devpod'sif path == ""in four places.
So Alias::Published is now read from the file devpod wrote. That half is right.
2. BLOCKING — dl reads the file devpod wrote, and then tells OpenSSH to read a different one
ssh::command_args (clients/ssh.rs:117) is unchanged: ssh -t [-o SendEnv=…] <ws>.devpod <payload>, no -F. OpenSSH resolves the alias through getpwuid's ~/.ssh/config; it reads neither $DEVPOD_SSH_CONFIG nor $HOME. Before this change that divergence could not exist — config_path() was ~/.ssh/config, so deciding the alias existed and being able to use it were the same fact. This PR separates them and nothing re-joins them.
Take the case the PR is for: DEVPOD_SSH_CONFIG=/tmp/dl-bench/ssh_config, devpod up writes the alias only there, no ~/.ssh/config. Then terminal_for → Usable → ssh_with_terminal → OpenSSH:
$ export HOME=/scratch/home DEVPOD_SSH_CONFIG=/scratch/ssh_config # alias only in the latter
$ ssh -G myws.devpod | grep -E '^(hostname|proxycommand) '
hostname myws.devpod # no match; no proxycommand line at all
$ ssh -F "$DEVPOD_SSH_CONFIG" -G myws.devpod | grep -E '^(hostname|proxycommand) '
hostname 127.0.0.1
proxycommand /bin/true
$ ssh -t myws.devpod true
ssh: Could not resolve hostname myws.devpod: Name or service not known # exit 255
(OpenSSH_9.6p1, run in this container.) ssh_with_terminal has no fallback — it returns Session::Terminal { exit } — so the command does not run. The bug being fixed cost the user a pty; the fix costs them the command. That is a regression in the failure mode on exactly the population the change targets: a terminal plus a config anywhere other than ~/.ssh/config. The new README row (DEVPOD_SSH_CONFIG=<path> … "so it is where dl looks for them") now invites users into that configuration.
The diff itself states the fact that breaks it, in test/fixtures/e2e_helpers.py:374: "OpenSSH itself does not read DEVPOD_SSH_CONFIG or $HOME … ssh <alias> fails with 'Could not resolve hostname' — measured on this suite's own ssh." The e2e suite survives because route_ssh_through installs an ssh shim first on PATH that prepends -F <config>. Production has no shim.
Modelling half of the same finding: Terminal::Usable is the only arm that carries no path, while all three failure arms carry one. The value needed to build a working invocation is dropped exactly where it is needed and kept only where it is printed. Terminal::Usable { config: PathBuf } threaded into command_args as -F — the same thing the harness shim does — makes the divergence unrepresentable. (-F does replace the user's own ~/.ssh/config; devpod's block is self-contained and the e2e shim already runs that way, so this looks affordable, but it is a real trade worth stating in the CHANGELOG.) If -F is unwanted, then Usable has to be narrowed to "the resolved path is the one OpenSSH will read" and the rest reported.
3. The notice contradicts the reason the split exists
render.rs:1655 ends the NoDevpodSshConfig sentence with `dl <ws> restart` writes it. Three places in this same diff say the opposite: the enum doc ("restarting the workspace is the fix for that one and no fix at all for this one"), docs/cli.md ("because a restart cannot fix a dl that is reading the wrong file"), and the CHANGELOG ("ConfigMissing … which a restart cannot fix"). Since the whole justification for splitting the arm is that the advice differs, both halves now advising restart is the collapse in prose. each_way_of_losing_the_pty_transport_names_the_file_dl_read only asserts the two strings differ, so it does not catch it. Inline below.
4. Checked and clean
- "Never a round trip" holds, and the signature is what holds it.
already_cached_options(&Host, SystemTime),Host::ssh_config(&self, &ContextOptions)andterminal_for(&Host, &ContextOptions, &str)take noRunner;cached_optionsonly reads the cache file;ssh::config_pathandalias_inonly touch the filesystem. Nothing on the path reaches devpod by another route. Cold launches populate the cache inup_under_stagebeforeworkspace_sshruns, so the miss is confined to warm launches, as claimed. - Three-way split is distinct and exhaustive.
routeandrender::launch_noticeboth match every arm with no catch-all (launch_noticeends atLifecycle, no_ =>), so a fourth arm is a compile error at both.ssh::Aliasreplaces the bool with the three-arm answer the states are read off. The collapse cannot come back silently.ConfigUnlocatablecarrying nothing is right — what is missing is per-machine, not per-workspace. Some("")vsNoneinConfigSources. Two representations of "unset", normalised once inconfig_pathwitha_variable_exported_with_no_value_is_not_a_pathpinning it. Representable but harmless — devpod's context options really can hold an empty string, so the normalisation has to live somewhere, and one place with a test is the right somewhere. Not a finding.- Minor, no change needed:
dotfiles_update(launch.rs:1856) already holds freshContextOptionsandworkspace_sshre-reads the same cache underneath it. Harmless duplicate read.
Axis 2 — repo standards and guards
Everything I ran on 53d2e61, in this devcontainer:
| result | |
|---|---|
cargo test --workspace |
green — 1266 core, 115 dl, 0 failed across every suite |
cargo clippy --locked --all-targets -- -D warnings |
clean |
cargo fmt --check |
clean |
pytest test/ --ignore=test/e2e -q |
411 passed (needs cargo build --release -p dl -p aid first; without it 17 fail on "there is no dl to test", which is the harness, not the diff) |
pytest -m e2e -ra test/e2e/test_interactive_session.py |
12 passed, 1 skipped in 75s — real docker 29.7.2 + devpod v0.26.1, one workspace created and cleaned up. The skip is the declared "no claude in this workspace" opt-out. (CI's own e2e job on this head is green too.) |
The known flake did not fire: a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking passed in my cargo test --workspace run, so I neither reproduced nor re-established it, and I am attributing nothing to this change.
Public API — verified, claim holds. public-api.api.txt is byte-identical to origin/main (37 rows, no LaunchNotice row at all), so nothing entered the promised tier. public-api.rest.txt moves by exactly the three new arms plus NoTerminalAlias::config, which is what the tripwire is for. CI's public-api job is green.
The deleted symlink, and what the guard now covers. I ran it, so this is not taken on trust. The ~/.ssh/config symlink in route_ssh_through did exist because of this bug, and deleting it genuinely strengthens the harness: the scratch home now holds no ssh config, so test_host_alias_exists_for_the_workspace and the pty assertions passing do say dl's lookup resolved DEVPOD_SSH_CONFIG. But the argument in the PR body goes one step too far. The tests reach a container only because the ssh shim supplies -F, so what they cannot guard is the thing finding 2 is about — the invocation dl actually builds. Worth saying plainly in the harness comment, because the next reader will otherwise take a green e2e run as evidence the transport works off ~/.ssh/config, and it is not.
Docs and CHANGELOG are accurate about devpod's order (I checked each sentence against the Go source above) apart from the restart contradiction in finding 3. The measurement caveat is real and correctly stated: bench.yml and bench_launch.py's epilog both export DEVPOD_SSH_CONFIG, so trend points straddling this commit compare two transports. Note that the CI bench job has no tty (terminal_usable needs both stdin and stdout), so it stays on the devpod transport either way — the caveat bites the hand-run local recipe from the epilog, which is also where finding 2 bites.
Verdict
Request changes — one blocking correctness finding (2) and one contradiction to resolve (3). Everything else in the change is solid: the precedence order is verified against the actual upstream source rather than taken on trust, the no-round-trip constraint really is enforced by the signatures, the three-way split is distinct and exhaustive with no way back to the collapse, and every guard the repo has is green. But the change as it stands moves dl from silently losing the pty to failing the command outright on the hosts it was written for, and one -F (carried in Terminal::Usable) is the difference.
| return Terminal::ConfigUnlocatable; | ||
| }; | ||
| match ssh::alias_in(&config, workspace_id) { | ||
| ssh::Alias::Published => Terminal::Usable, |
There was a problem hiding this comment.
Blocking. Usable is the only arm that does not carry the path, and it is the arm that needs it. ssh::command_args (clients/ssh.rs:117) still builds ssh -t <ws>.devpod <payload> with no -F, and OpenSSH resolves that alias through getpwuid's ~/.ssh/config — it reads neither $DEVPOD_SSH_CONFIG nor $HOME. Before this change the two could not diverge, because config_path() was ~/.ssh/config.
So on the exact host this PR is for — DEVPOD_SSH_CONFIG=/scratch/ssh_config, alias written only there, no ~/.ssh/config:
$ ssh -G myws.devpod | grep '^hostname '
hostname myws.devpod # no match, and no proxycommand line
$ ssh -t myws.devpod true
ssh: Could not resolve hostname myws.devpod: Name or service not known # exit 255
(OpenSSH_9.6p1, reproduced in this container; -F "$DEVPOD_SSH_CONFIG" resolves it correctly.) ssh_with_terminal returns Session::Terminal { exit } with no fallback, so dl <ws> -- <cmd> at a terminal goes from works without a pty to does not run. The diff already writes down the reason, at test/fixtures/e2e_helpers.py:374; e2e only survives because route_ssh_through puts an ssh shim on PATH that prepends -F, and production has no shim.
Suggested: Terminal::Usable { config: PathBuf }, threaded into command_args as -F <config> — the same thing the harness shim does, which makes the divergence unrepresentable rather than documented. (-F does displace the user's own ~/.ssh/config; devpod's block is self-contained and the shim already runs this way, but that trade belongs in the CHANGELOG.)
There was a problem hiding this comment.
Fixed in 9c8dc90, and reproduced first both ways: by hand on OpenSSH_9.6p1 (exit 255, Could not resolve hostname; -F resolves it), and through dl against a real container once I deleted the e2e shim, where six of seven TestCommandGetsATerminal tests failed on ssh -t e2e-test-interactive.devpod with no -F.
Took the modelling suggestion as written: Terminal::Usable { config: PathBuf }, Route::Terminal { payload, config }, and ssh::command_args(config, ...) with the config as its first parameter, so an invocation with no -F is not expressible and nothing re-derives the path downstream of the decision that chose it. The trade is in the CHANGELOG and in docs/cli.md, including the part not in your comment: -F also makes ssh skip /etc/ssh/ssh_config. Why it cannot be conditional is in command_args doc: dl cannot know whether ssh would have read the file dl read, since $HOME and getpwuid home are allowed to differ, which is exactly the case the e2e suite creates.
The shim is deleted, because it was supplying the fix under test. What is left is a scratch HOME with no .ssh/config, a negative prop, so these tests reaching a container is itself the assertion. One new e2e test names that claim outright. Detail in the PR comment.
| } => format!( | ||
| "No ssh config at {}, which is where `devpod up` publishes its host aliases on this \ | ||
| machine, so {workspace_id} has none and this command gets no terminal; interactive \ | ||
| programs may exit immediately. `dl {workspace_id} restart` writes it.", |
There was a problem hiding this comment.
This sentence tells the user to restart, and three places in this same diff say a restart is exactly what cannot fix this state: the enum doc above ("restarting the workspace is the fix for that one and no fix at all for this one"), docs/cli.md ("because a restart cannot fix a dl that is reading the wrong file"), and the CHANGELOG entry ("ConfigMissing … which a restart cannot fix").
Since "the advice differs" is the whole justification for splitting the arm, both halves ending in restart is the collapse reappearing in prose. each_way_of_losing_the_pty_transport_names_the_file_dl_read only asserts the two strings differ, so it does not catch it. One of the two claims has to go — and note restart genuinely does write the file when the path is the right one and merely empty, so it may be the doc/CHANGELOG half that is wrong rather than this line.
There was a problem hiding this comment.
Resolved in 9c8dc90, in the direction your last sentence points: a restart genuinely does write the file when the path is right and the file merely absent, so "a restart cannot fix this" was the half that was too strong. But an unqualified promise is wrong too, because the two context-option paths come from a cache that can miss: dl can be reading ~/.ssh/config while devpod publishes to a context path, and then a restart publishes somewhere dl is not looking.
So the sentence is now conditional, and the conditionality is what distinguishes it from NoTerminalAlias flat advice:
dl myws restartpublishes there if that is the file devpod writes; if this comes back, DEVPOD_SSH_CONFIG or devpod ssh-config context options name a different one.
The enum doc, docs/cli.md and the CHANGELOG now say the same thing.
And you are right that distinctness is not the assertion the split needs, since three wrong-but-different sentences pass it. each_way_of_losing_the_pty_transport_names_the_file_dl_read keeps that job, and a new the_advice_each_pty_notice_gives_is_the_advice_that_arm_can_honour pins the claim per arm: NoTerminalAlias promises a restart outright, NoDevpodSshConfig offers it conditionally and is asserted not to carry the old unconditional phrasing, and SshConfigUnlocatable must not mention a restart at all. Red on 53d2e61.
Deciding the alias exists and being able to use it were one fact while `config_path()` was `~/.ssh/config`, and the previous commit separated them without rejoining them. `ssh::command_args` still emitted `ssh -t <ws>.devpod <payload>` with no `-F`, and OpenSSH resolves that alias through `getpwuid`'s config: it reads neither `$DEVPOD_SSH_CONFIG` nor `$HOME`. So on the hosts this branch is for, `dl <ws> -- <cmd>` at a terminal stopped running at all -- exit 255, `Could not resolve hostname` -- where before it had merely run without a pty. Reproduced first, twice. By hand on OpenSSH_9.6p1 with the alias only in `$DEVPOD_SSH_CONFIG` (`ssh -G` finds no match, `ssh -t` exits 255, `-F <that file>` resolves it), and then through dl against a real container once the e2e shim was gone: six of seven tests in `TestCommandGetsATerminal` failed on `SSH command: ssh -t e2e-test-interactive.devpod ...` / `Could not resolve hostname`. The path travels with the state rather than being fetched again. `Terminal::Usable` was the only arm carrying no config, and it is the arm that needs one; it now carries it, `Route::Terminal` carries it beside the payload for the same reason it carries the payload, and `ssh::command_args` takes it as its first parameter. An invocation with no `-F` is not expressible, and nothing re-derives the path downstream of the decision that chose it. The trade `-F` makes is real and is written down in the CHANGELOG and docs/cli.md: OpenSSH reads that file *instead of* `~/.ssh/config` and skips `/etc/ssh/ssh_config`, so a `Host *` block of the user's own does not apply to a devlaunch session. Taken deliberately, because dl cannot know whether ssh would have read the file dl read -- `$HOME` and `getpwuid`'s home are allowed to differ, which is exactly the case the e2e suite creates -- so a conditional `-F` would be a guess, and guessing wrong costs the command. The e2e `ssh` shim is deleted, because it was supplying the fix under test. It prepended `-F` to every invocation, which is why every pty assertion in `test_interactive_session.py` passed against a dl that emitted none. What is left is a scratch `HOME` holding no `.ssh/config`, which is a negative prop: with it, these tests reaching a container at all is the assertion that dl found devpod's config and handed it to OpenSSH. One test names that claim outright, pinning both the `-F` in the invocation dl logs and output from inside the container. And the `NoDevpodSshConfig` notice no longer promises what three other places in the diff denied. A restart writes that file only if devpod publishes there, so the sentence says so conditionally and names what to check when it does not; the enum doc, docs/cli.md and the CHANGELOG are corrected to the same claim, since "a restart cannot fix this" was too strong. `each_way_of_losing_the_pty_transport_names_the_file_dl_read` only asserted the three strings differ, which three wrong-but-different sentences would have passed, so a second test pins the advice each arm can honour instead of its distinctness.
|
Both findings addressed in Finding 2 (blocking) — dl now tells OpenSSH to read the file it readReproduced twice first. By hand, on this container's Then through dl against a real container, which is the reproduction that mattered: I deleted the e2e So the finding is exactly right, including the part about the failure mode getting worse: The modelling fix, not the symptom. The trade, since you asked for it in the CHANGELOG. It is there, and it is slightly worse than "displaces The harness shim: deleted, and you were right that it is what hid this. It became redundant the moment dl passes its own Tests added. Four, at three altitudes:
Finding 3 — the notice no longer promises what the rest of the diff deniesI resolved it the way your own note suggested, in the direction of the notice rather than against it: a restart genuinely does write that file when the path is right and the file is merely absent, so "a restart cannot fix this" was too strong. But an unqualified So the sentence is now conditional, and that conditionality is the difference from
The enum doc, And the test is strengthened rather than left to distinctness. You are right that What I did not touchEverything the review verified as correct is untouched: the precedence order and Guards, all run in this devcontainer on
|
| result | |
|---|---|
cargo test --workspace |
green — 1268 core, 116 dl, 0 failed across 25 suites |
cargo clippy --locked --all-targets -- -D warnings |
clean |
cargo fmt --check |
clean |
pytest test/ --ignore=test/e2e -q |
411 passed |
pytest -m e2e -ra test/e2e/test_interactive_session.py |
13 passed, 1 skipped in 55s — real docker + devpod v0.26.1, no ssh shim on PATH, one workspace created and cleaned up (devpod list and docker ps -a both empty afterwards). 13 rather than 12 because of the new test; the skip is the declared "no claude in this workspace" opt-out. |
ruff check / ruff format --check / pylint on the two changed Python files |
clean, 10.00/10 |
The known flake did not fire: a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking passed in every cargo test --workspace run here, so I am attributing nothing to it.
One measurement note that survives from the earlier commit and is now doubly true: a bench run that exports DEVPOD_SSH_CONFIG was on the devpod transport before this branch, and is on the OpenSSH one after it. Points straddling this commit compare two transports.
CHANGELOG.md conflicted: this branch's entries stay under [Unreleased], main's new [0.14.0] section sits below them. Also corrected the same overstated qualifier in two places. docs/cli.md and the CHANGELOG both said a 'Host *' block stops applying 'when devpod publishes elsewhere', attached to a sentence covering /etc/ssh/ssh_config as well. That qualifier is true of the user config and false of the system one: naming any file with -F makes OpenSSH skip /etc/ssh/ssh_config unconditionally, including when the file named is the user's own config. Measured on OpenSSH_9.6p1: ssh -G -F ~/.ssh/config drops sendenv LANG, sendenv LC_*, hashknownhosts and gssapiauthentication. ssh.rs's own doc comment already stated it correctly and unqualified; the two user-facing copies now match it.
This is a shipped bug, not a refactor. On any host that sets
DEVPOD_SSH_CONFIG,dlsilently loses the pty transport: everydl <ws> -- <cmd>falls back todevpod ssh --command, which never asks for a pty, and the only thingdlsays is that the workspace wants restarting.Ticket #421; slice 1 of #390's resolution, which blocks the connection-reuse slice behind it. #389 found the same thing independently and set it aside.
The bug
clients/ssh.rs::config_path()was~/.ssh/configand nothing else. devpod does not necessarily write there. Read out ofskevetter/devpodv0.26.1, it targets, in order:SSH_CONFIG_INCLUDE_PATH(pkg/ssh/config.go::ConfigureSSHConfig),--ssh-config, whose defaultcmd/root.go::inheritFlagsFromEnvironmentfills in from$DEVPOD_SSH_CONFIG,SSH_CONFIG_PATH(cmd/up.go::prepareClient),~/.ssh/config.And it rewrites whichever one it picks and creates no other. So on a host exporting that variable there was no
~/.ssh/configfordlto read,devpod_host_configuredanswered false,routereturnedDevpodCommand, and interactive programs met a pipe andTERM=dumb.The red test pre-existed only as a description. #421 says one exists; #390's "How it is tested" section proposes it ("red on merged main today") but no test was ever written, on
mainor onresearch/devpod-ssh-trip. So I wrote one, and proved the red first by hand onorigin/main, with a throwaway test since the fixed signature does not exist there:That throwaway is not in the diff; the permanent tests are.
What changed
1.
config_pathresolves devpod's order. It takes aConfigSourcesand walks the four candidates, expanding a leading~/the way devpod'sResolveSSHConfigPathdoes and reading an empty value as unset (devpod's ownif path == ""). Four fields rather than four arguments, because they are all optional paths and a transposed argument list is the defect this exists to fix.2. The context-option paths never cost a round trip. They come from
already_cached_options, which reads the disk cache and nothing else.terminal_forruns on every launch including the warm ones, anddevpod context optionsis 0.4-0.7s (#393) against a transport worth less than that. A cache miss answers with devpod's defaults, which is whatdlassumed unconditionally before, so a miss is never worse than the bug it replaces. That constraint is held by the signatures rather than by a test: neitherterminal_fornoralready_cached_optionstakes aRunner, so neither has anything to ask devpod with.3.
Terminal::NoAliassplits, because silence is what let this ship. It was two facts wearing one name: "this config has no entry for this workspace" and "there is no config where devpod publishes". Both reached the user as one sentence aboutdl <ws> restart, which fixes the first and cannot touch the second. Now:NoAlias { config }restartConfigMissing { looked_in }ConfigUnlocatabledevpod_host_configured's bool becomesssh::Alias(Published/WorkspaceAbsent/NoConfig), the three-arm answer those states are read off, so the collapse cannot come back by accident.4. The e2e harness stops working around the bug.
route_ssh_throughsymlinked a scratch~/.ssh/configonto the run's config precisely sodl's hardcoded lookup would find it. That symlink is gone: the suite's scratch home now holds no ssh config at all, sotest_interactive_session.pypassing is what says the resolution order still holds. OpenSSH still needs its-Fshim, since it reads neither$HOMEnor any variable of devpod's.Measurement consequence, and it is not small
Scratch-scoped runs set
DEVPOD_SSH_CONFIG(this repo's own convention in CLAUDE.md,test/conftest.py,scripts/bench_launch.py,.github/workflows/bench.yml), so every one of them has silently been taking the non-pty path. Before/after numbers taken that way are not comparable across this change: the "before" side measured a different transport. Anything on #139's trend that straddles this commit needs re-baselining rather than differencing.Guards
cargo test --workspace: green (1266 in core, 115 in dl, all suites).cargo clippy --locked --all-targets -- -D warnings: clean.cargo fmt --check: clean.pytest test/ --ignore=test/e2e: 411 passed.public-api.rest.txtupdated for the threeLaunchNoticearms; nothing enters the promisedapitier.Pre-existing flake, not this change:
aid'sa_pasted_multi_line_prompt_arrives_whole_rather_than_leakingfailed once under full-workspace load. Reproduced onorigin/mainwith this branch stashed: 1 failure in 8 runs under CPU load, 0 in 3 unloaded full-workspace runs. It touches no file this branch changes.🤖 Generated with Claude Code
Summary by Sourcery
Restore
dl's OpenSSH pty transport by following devpod's published SSH configuration and using it consistently for alias detection and command execution.Bug Fixes:
dllocate devpod SSH aliases in the same configuration file devpod publishes, preserving the OpenSSH pty transport whenDEVPOD_SSH_CONFIGor SSH context options are used.dlresolve correctly.Enhancements:
Documentation:
Tests: