Repository navigation
Opt-in dotfiles refresh on attach #183
Description
Background
PR #44 originally carried two features: the dl <ws> dotfiles subcommand, and an automatic dotfiles refresh on every attach. On rebase the second half was dropped and the PR reduced to the subcommand alone. This issue tracks the dropped half.
The gap it aimed at is real. devpod applies dotfiles when it provisions a workspace, and main passes --dotfiles/--dotfiles-script to devpod up (devlaunch/dl.py:1934-1937). But attaching to an already-Running workspace deliberately skips devpod up — test/test_devpod_spawn_counts.py::test_attaching_to_a_running_workspace pins that path to one status and two ssh calls — so a long-lived workspace keeps whatever dotfiles it was born with until someone runs dl <ws> dotfiles, restart, or recreate. Doing that refresh automatically is a reasonable thing to want.
Why the first attempt did not land
It refreshed unconditionally on every attach. Injecting that hook into attach_workspace() (devlaunch/dl.py:2120) fails 5 tests in test/test_devpod_spawn_counts.py:
FAILED TestHotCommandSpawnCounts::test_attaching_to_a_running_workspace
FAILED TestHotCommandSpawnCounts::test_a_one_shot_command_skips_the_hostname_round_trip
FAILED TestHotCommandSpawnCounts::test_a_git_spec_one_shot_on_a_running_workspace
FAILED TestAttachHelper::test_an_interactive_attach_sets_the_hostname
FAILED TestAttachHelper::test_a_one_shot_command_does_not
Per #139, a devpod ssh --command trip costs ~1.73s and is ~99% connection setup, and the hook adds one before chezmoi update (a network git pull) and pixi global sync even start. attach_workspace() already skips the far cheaper hostname round-trip for one-shot commands on the grounds that it "would buy that command nothing"; an unconditional dotfiles refresh runs against that reasoning.
Requirements
An implementation is worth landing if it meets all three:
- Off by default, behind an env var or a config setting. A user who has not asked for it must see no change in attach latency.
- A timeout on the network call, so an unreachable dotfiles remote fails fast and hands over the shell instead of hanging. Today a non-zero exit is tolerated (warn and continue), but nothing bounds how long
chezmoi updateorgit clonemay block on a network timeout or a credential prompt. - Skipped entirely for one-shot
dl <ws> -- cmd, matching howattach_workspace()already treats the hostname round-trip. This is the path Launch latency: dl <spec> -- <cmd> to a running command #139 is about and the shape wayfinder uses for every agent launch.
Worth considering beyond the minimum: a cooldown so repeated attaches within some window refresh at most once, and/or running the refresh in the background so it never sits in front of the shell.
Notes
test/test_devpod_spawn_counts.pyis the suite any implementation has to satisfy. Its docstring is explicit that it exists to catch reintroduced round-trips, so a change that needs those assertions edited should come with an argument for why the new cost is worth it.dotfiles_update()already exists on main once Adddl <workspace> dotfilesrefresh subcommand #44 lands; this issue is only about when and whether it is called automatically.- Related: Launch latency: dl <spec> -- <cmd> to a running command #139 (launch latency).
Activity
blooop commentedon Aug 15, 2026
breadcrumb: Building the opt-in refresh. Restating the contract as I read it: a dotfiles refresh runs in front of an interactive attach only, only when the user has switched it on, and only for a bounded number of seconds.
Seams, all three already established by the ticket or the suite it names — nothing new invented:
Measured before designing, rather than assumed: a
timeout Nwrapper puts the managed command in its own process group and signals the group, so the grandchild that is actually doing the network work dies too. Checked that it releases a held pipe rather than leaving a survivor holding the session open — the run returned at 2.003s against a 2s bound with a 41s sleep behind it. That is the property the requirement depends on ("fails fast and hands over the shell"), and it would have been a plausible thing to get wrong.On the two extras the ticket floats, I expect to decline both and will argue it on the PR rather than assert it here.
blooop commentedon Aug 15, 2026
breadcrumb: PR #220 — the refresh is opt-in behind an environment variable, skipped for one-shot commands, and bounded at 60s.
The design decision worth recording is where the bound lives. It is spent inside the container rather than as a host-side deadline on the
devpod sshclient, and that is not a style choice: killing the client would leave the remote git process alive and still holding the session open, which is the exact hang the requirement exists to prevent.timeoutputs the managed command in its own process group and signals the group, so the process actually blocked on the network dies with the shell that started it. Measured against a hanging stub rather than assumed — 124 at 1.01s against a 1s bound, no survivor.Both floated extras declined, and for different reasons, which is why neither was a close call:
dotfilessubcommand already does. Ordering is now asserted by a test rather than left implicit.One thing a reviewer should push on rather than take on trust: the refresh is not restricted to workspaces that were already Running, so it also fires on the cold path where dotfiles were just installed and it is redundant. Narrowing it means threading workspace state into a function that deliberately does not know it, to save one bounded trip on a path that just spent ~60s building a container. Judged not worth it and flagged rather than built, but it is the judgement call in this change.
Also worth knowing for anyone testing this by hand: the payload crosses as
bash -lc, and a login shell resets PATH — a stubbedchezmoion PATH gets bypassed and the real one runs. That is why the suite asserts the bound as a literal instead of executing the payload; a test that ran it for real would run real dotfiles commands on CI.Green everywhere:
pixi run ciexit 0, pylint 10.00/10, 1739 passed locally; all 9 required checks pass on the PR across py310-py313 plus e2e. Stopping here — not reviewing my own work.blooop commentedon Aug 15, 2026
wf-review verdict on #220: Request changes → resolved during review. All three hard requirements independently reproduced (additions-only diff on the spawn-count file; the quoting stack traced with an apostrophe-bearing URL; the 124-at-1.01s process-group kill re-measured). One blocking finding — the new env var unscrubbed in conftest turned two pins red in an opted-in developer's shell — fixed as 5c417dd. Both extras' declinations judged sound. Merging once checks re-green.