Repository navigation
Opt-in dotfiles refresh on attach - #220
Conversation
devpod applies dotfiles when it provisions a workspace, so a long-lived one keeps whatever it was born with until somebody runs `dl <ws> dotfiles`. This closes that gap for anyone who sets DEVLAUNCH_DOTFILES_ON_ATTACH=1, and for nobody else. Off by default is the requirement, not a precaution. The first attempt at this refreshed unconditionally and charged every attach a ~1.7s devpod round-trip plus a git pull; the spawn-count pins that measure the attach path are the tests it failed, and they are unedited here and still green. Dropping the opt-in check makes exactly those pins fail again, which is what "off by default" is worth asserting as. Skipped for `dl <ws> -- cmd` on the reasoning the attach has always applied to that path: a one-shot renders no prompt and sources no interactive shell, so a refresh in front of it buys the command nothing and costs it a round-trip. Bounded at 60s, spent inside the container rather than as a host-side subprocess deadline: `timeout` signals the whole process group, so the git process actually blocked on the network dies with the shell that started it instead of lingering with the session held open. Verified against a hanging stub -- the payload returned 124 at 1.01s against a 1s bound and left no survivor. A host-side kill of the devpod client would not have achieved that. `dl <ws> dotfiles` typed by hand stays unbounded: a deadline is worth having on the refresh nobody asked for, and is a way to abandon a half-finished `pixi global sync` on the one somebody did. Closes #183
Reviewer's GuideImplements an opt-in, bounded dotfiles refresh that runs inside the container immediately before interactive attaches, while preserving existing spawn counts and skipping one-shot commands, and documents the new behavior and configuration flag. Sequence diagram for opt-in dotfiles refresh on interactive attachsequenceDiagram
actor User
participant dl
participant attach_workspace
participant refresh_on_attach_enabled
participant dotfiles_update
participant workspace_ssh
User->>dl: dl workspace_id
dl->>attach_workspace: attach_workspace(workspace_id, shell_command=None)
attach_workspace->>refresh_on_attach_enabled: refresh_on_attach_enabled()
alt dotfiles refresh enabled
attach_workspace->>dotfiles_update: dotfiles_update(workspace_id, timeout=DOTFILES_ATTACH_TIMEOUT_SECONDS)
dotfiles_update->>workspace_ssh: workspace_ssh(workspace_id, command=update_cmd)
Note over dotfiles_update,workspace_ssh: update_cmd wrapped by timeout bash -c when timeout is not None
else dotfiles refresh disabled
Note over attach_workspace: No dotfiles_update call
end
attach_workspace->>workspace_ssh: workspace_ssh(workspace_id, shell_command=None)
workspace_ssh-->>User: Interactive shell in workspace
rect rgb(240,240,240)
User->>dl: dl workspace_id -- command
dl->>attach_workspace: attach_workspace(workspace_id, shell_command=command)
Note over attach_workspace: shell_command is not None → dotfiles_update skipped
attach_workspace->>workspace_ssh: workspace_ssh(workspace_id, shell_command=command)
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
==========================================
+ Coverage 92.78% 92.80% +0.01%
==========================================
Files 24 24
Lines 3672 3681 +9
==========================================
+ Hits 3407 3416 +9
Misses 265 265
🚀 New features to boost your workflow:
|
The review's blocking finding: with the switch exported, two pre-existing attach-shape pins go red in exactly the shell of the developer who opted in — the same argument timing_switch_off already states. Scrubbed in the same fixture; verified 357 passed with the var exported.
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #220 against ticket #183 at merge-base 9681c6b. Preflight: all checks green, non-empty diff (5 files, +262/−1), MERGEABLE.
Standards
- BLOCKING — the new env var was not scrubbed in
test/conftest.py, so the suite goes red in the shell of the very user the feature is for. WithDEVLAUNCH_DOTFILES_ON_ATTACH=1exported, two pre-existing attach-shape pins fail. This is exactly the casetiming_switch_offalready argues for by name ("the developers this instrument exists for are the ones who will have it exported when they run the suite"). [Resolved during review — orchestrator commit 5c417dd adds the scrub to the same fixture; 357 passed with the var exported.] - Non-blocking — quoting composes correctly. Three shell layers, two
shlex.quoteapplications; an apostrophe-bearing URL survives to git intact. One wrinkle: the inner shell isbash -c, not-lc, so it relies on PATH inheritance rather than the parity ruleworkspace_sshdocuments atdl.py:3639. - Non-blocking —
_FALSEYis now a fourth copy (tools.py, gh_auth.py, tty_session.py, dl.py), andrefresh_on_attach_enabledlands bare in the 4000-line dl.py with a name that doesn't say what refreshes. - Non-blocking — the "~1.7s, ~99% connection setup" rationale is stated six times across docstrings/README/CHANGELOG/tests.
- Non-blocking — the gate also fires on
recreate/reset/first-provision, where devpod applied dotfiles seconds earlier (declared by the builder; opt-in only). - Non-blocking — test looseness: two new pins slice
argv[:2]against the file's "exact sequence" convention;test_the_refresh_trip_is_boundedformats the constant into its own expectation, so the promised60is pinned nowhere. - Non-blocking —
attach_workspacediscardsdotfiles_update's return silently; README's "Failure is a warning" is not literally true (dl logs nothing; the user sees the remote's own stderr).
Spec
Nothing blocks. All three hard requirements met and independently reproduced.
- Req 1 — "Off by default… no change in attach latency."
git diff --numstaton the spawn-count file is92 0— additions only. All 5 tests the ticket names as the first attempt's failures are intact and green (two were renamed on main pre-merge-base, not by this PR). Full suites: 357 passed. - Req 2 — "A timeout on the network call." Traced end-to-end with an apostrophe-bearing URL through all three shell levels; argv arrives intact. The "124 at 1.01s, no surviving grandchild" claim reproduces exactly on coreutils 9.4 —
timeoutsignals the process group as stated. Measured, not asserted. - Req 3 — "Skipped entirely for one-shot." Pinned with the switch ON: exactly
status+ssh --command. - Mutation reproduced: dropping the opt-in check fails 8 tests including 2 pre-existing unedited pins — the ticket's original failure, live again.
- Non-blocking: falsey semantics (
=off/=nenable it) are undocumented — consistent withNO_TTY/NO_GH_TOKENprecedent; README's "Failure is a warning" inaccurate (silent discard — a 60s timeout kill is an unattributed pause); undeclared dependency on atimeoutbinary in the container (absent → silent no-op). - Declinations both sound. Cooldown: state-cost argument holds against a "worth considering" extra. Background: correctly refused on correctness — a refresh landing after the session starts updates files that shell already sourced; the ordering is pinned.
- Deviations all four sound (cold-path firing declared and opt-in-only; constant not env var;
dl <ws> dotfilesdeliberately unbounded and pinned;devpod context optionssurfaced in the pin rather than hidden).
Verdict
Request changes → resolved during review. The single blocking finding (missing conftest scrub) was a one-fixture addition; applied as commit 5c417dd and verified with the var exported (357 passed). With that in, no findings block. Riders for later touchers: pin the literal 60, tighten the two argv[:2] pins, document the falsey semantics and the timeout-binary dependency, soften README's "Failure is a warning" to what actually happens. GitHub refuses author-account review states, so this is posted as a comment; this written verdict governs. Merging with a merge commit once checks re-green on 5c417dd.
Closes #183
devpod applies dotfiles when it provisions a workspace, so a workspace that has been up for a fortnight still has the dotfiles it was born with.
DEVLAUNCH_DOTFILES_ON_ATTACH=1makesdlrun the existing refresh just before it hands over an interactive shell.The three requirements
1. Off by default. The claim worth making is not that a flag exists but that the spawn counts did not move.
test/test_devpod_spawn_counts.pyis unedited — 0 deleted lines, 30 original tests still green — and the new pins sit beside them. Dropping the opt-in check (making it unconditional, as the first attempt did) failstest_attaching_to_a_running_workspaceandtest_an_interactive_attach_spawns_nothing_but_the_session, which are two of the five the ticket names. That mutation was run, not assumed.The switch reads the vocabulary of denials
DEVLAUNCH_NO_TTYandDEVLAUNCH_NO_GH_TOKENalready read ("",0,false,no, case- and space-insensitive), rather than a third spelling. The same expression flips sense for free: unset reads as the empty string, which is a denial, so off-by-default falls out of the idiom instead of being stated separately.2. A timeout. 60s, spent inside the container as
timeout 60 bash -c '<payload>', not as a host-side subprocess deadline. That choice is load-bearing and was measured before it was made:timeoutputs the managed command in its own process group and signals the group, so the git process actually blocked on the network dies with the shell that started it. Against a hanging stub the emitted payload returned 124 at 1.01s against a 1s bound, with no surviving grandchild — a host-side kill of thedevpod sshclient would have left the remote process holding the session open, which is the failure mode the requirement exists to prevent.3. Skipped for one-shot
dl <ws> -- cmd.shell_command is Noneis the gate, matching the reasoningattach_workspacehas always applied to that path — no prompt is rendered, no interactive shell is sourced, so a refresh in front of it buys the command nothing and costs it a round-trip. Pinned at the spawn boundary with the switch on: still exactlystatus+ssh --command.The two extras, both declined
Cooldown — declined. It buys back seconds and costs a new piece of persistent per-workspace state: a timestamp store, a location for it, an expiry, and its own failure modes when it is stale or unwritable. The repo's taste is against machinery for costs measured in seconds, and the user opted in; attaching five times means five refreshes were asked for. Anyone who finds that too often has
dl <ws> dotfilesand an unset variable.Background refresh — declined, and on correctness rather than cost. Backgrounding it would be defensible if the only objection were latency, but it defeats what the feature is for. The point of refreshing on attach is that the shell being handed over is the one that reads the new dotfiles. A refresh that lands after the session starts is updating files that shell has already finished sourcing — so it would help only the next attach, which is what
dl <ws> dotfilesalready does. It would also interleavechezmoioutput into a terminal devpod has taken over. Running it in front of the shell is the ordering the feature means, and one new test asserts that ordering.Deviations declared
attach_workspaceis reached from the cold path too, where devpod has just installed dotfiles and the refresh is redundant. Narrowing it would mean 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. The ticket lists three requirements and this is not among them; flagging rather than building it.dl <ws> dotfilesis deliberately left unbounded (timeoutdefaults toNone). A typed refresh is in the foreground and interruptible, and a firstpixi global syncis legitimately slow; killing it at a fixed second would abandon a half-finished sync to save nobody any time. Pinned by its own test so the asymmetry is intentional rather than incidental.devpod context options, via the existingdotfiles_update. It is cached on disk for an hour, so in practice it is a stat rather than a spawn — but it is visible in the new pin rather than hidden, which is why the pin lists four trips.Tests
Nine new pins in
test_devpod_spawn_counts.py(the file the ticket names as "the suite any implementation has to satisfy") plus two intest_dl.py. Every one was mutation-checked, each mutation verified to have actually applied — the first harness silently no-opped on a missingpythonand reported five false passes:test_a_one_shot_command_is_not_worth_a_refreshtest_the_refresh_trip_is_bounded+ the payload testThe timeout is asserted as a literal in the emitted payload rather than by waiting out a real hang. Executing it for real is what proved the design, but it cannot be a test:
workspace_sshwraps payloads inbash -lc, and a login shell resetsPATH— the verification run reached the host's realchezmoiandpixistraight past a stubbed one. A suite that did this would run real dotfiles commands on CI.CI:
pixi run ciexit 0, pylint 10.00/10, 1739 passed.🤖 Generated with Claude Code
Summary by Sourcery
Add an opt-in mechanism to refresh dotfiles on workspace attach while preserving existing attach behavior by default.
New Features:
Enhancements:
dl <ws> -- <command>invocations to avoid unnecessary overhead.dl <ws> dotfilesruns unbounded.Documentation:
Tests: