devpod's on-disk layout gets the module it was missing - #427
Conversation
`clients::devpod` is the seam for devpod-the-command. devpod-the-filesystem had no seam at all, so `<devpod home>/contexts/<ctx>/workspaces/<id>/…` was rebuilt wherever it was wanted: three times in `flows/lifecycle.rs`, once in `flows/provision/verdict_cache.rs`, and four more in test fixtures that reproduced the convention literally. The comment at lifecycle.rs:2861 claimed the paths went through shared helpers "so devpod's on-disk layout is spelled out in one place and not three". It was spelled in four, and the test fixtures made it eight. `devlaunch-core/tests/devpod_layout.rs` is what makes the claim checkable rather than aspirational: it walks the crate's sources and fails if any file but `clients/devpod_home.rs` contains a string literal starting `contexts`. Before this change it named all eight, which is the honest red — the failure a layout copy causes is not a wrong answer but a second copy that agrees with the first until devpod moves it, and nothing but a guard catches that. A second test asserts the adapter *does* spell it, so the guard cannot pass by the layout having been deleted. Prose is deliberately not matched (a doc comment costs nothing when devpod moves the layout), nor is devpod `config.yaml`'s top-level `contexts:` key, which is a different fact about a different file. `dl`'s end-to-end tests are out of scope on purpose: they stand outside the crate and check that what dl wrote landed where devpod will look for it, which is exactly the assertion that must not be routed through the code under test. Moved into `clients::devpod_home`, whole: `devpod_home`, the record and result path builders, `CreateRecord`/`create_record`, the contexts walk, `sole_workspace_result`, `repoint_devpod_source` and `RepointFailure`. Plus `Host::devpod_config`, which was joining `config.yaml` onto the same bare path. Four decisions worth the words: - **The home is taken, not resolved.** `DevpodHome::at` takes the directory and the binary resolves it once, which is the convention `dl/src/commands.rs` already states at its `workspace_delete` call. `DevpodHome::locate` is the one place that reads `DEVPOD_HOME`, and it splits into a thin environment read over a pure `located(configured, home_dir)`, the shape `osext::home_dir` uses. The old test could only assert that this machine has a home directory; the pure half pins that an empty `DEVPOD_HOME` falls through to `~/.devpod` rather than naming the current directory, without mutating an environment the whole test binary shares. - **`repoint` takes `(context, workspace_id, source)`, not `&Adoptable`.** An adapter under `clients/` that named a reconcile-flow type would be a seam pointing the wrong way. Its doc — devpod v0.26.1 has no subcommand that changes an existing workspace's source, so the choice is one field of one JSON file or no repair at all — carries across, and is what makes writing into devpod's own file an adapter's job rather than a leak. - **`create_record` and `sole_workspace_result` stay free functions taking `Option<&DevpodHome>`, rather than becoming methods.** "This machine has no devpod home" is one of the answers each gives, and absorbing it in the module keeps five call sites from each deciding again what a missing home means — they would not all decide it the same way, and one of them reads the absence as `!= NeverCompleted`. - **The pass-through signatures take the type.** `workspace_delete`, `purge_all_data` and `apply_reconciliation` took a `&Path` named `devpod_home`; a bare path is what anybody can join, which is how there came to be eight copies. This changes `api::workspace_delete`'s promised signature — the one row of `public-api.api.txt` this touches — and the argument for paying that is the same one: the parameter was always devpod's home and now says so. The win this protects: `flows::lifecycle`'s `mod tests` is private again. It was `pub(crate)` solely so `flows/provision.rs` and `flows/provision/verdict_cache.rs` could import `tests::devpod_home_with` — a test fixture in the crate's internal surface because a devpod-home module did not exist. The fixture now sits beside the layout it builds as a plain `#[cfg(test)]` item at module scope, which is what keeps it shareable without a second `pub(crate) mod tests` in its new home. Nothing outside imported anything else from there. Both snapshots are hand-edited: `cargo-public-api` needs a nightly toolchain this container cannot install, so CI's regeneration is authoritative over these rows.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideCentralizes devpod's filesystem layout and record manipulation in a new Sequence diagram for reconciliation source repointingsequenceDiagram
participant Command as Reconcile command
participant Lifecycle as apply_reconciliation
participant Home as DevpodHome
participant Record as workspace.json
Command->>Lifecycle: apply_reconciliation(..., devpod_home, plan)
Lifecycle->>Home: repoint(context, workspace_id, source)
Home->>Record: read_to_string(path)
Home->>Record: update source.localFolder
Home->>Record: write temporary JSON
Home->>Record: rename temporary file over original
Home-->>Lifecycle: Result<(), RepointFailure>
Lifecycle-->>Command: ReconcileReport
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The
That is a breaking change to the frozen tier, and it is a different break from the one the row shows: not "the signature moved" but "the promise stopped being self-sufficient". It runs against what #313 settled for Two ways out, and the second is probably right:
If (2), this PR should say so in its body and #410 should carry the constraint explicitly. Flagging rather than deciding: the ordering between this and #410 is the reviewer's call, and both are open at once. Separately, worth a reviewer's eye: this is a hand-edited |
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.
This was generated by AI during review.
Reviewed at head 5b52fa1, merge-base 57955a3, in a fresh context that did not write the code. Two axes below, not merged or reranked. Everything marked [ran] was executed in a scratch worktree; [read] was reasoned from source or CI logs.
Standards
1. A client names a flow, and lib.rs says it must not — BLOCKING [ran]
src/lib.rs:15 states the crate's layering rule in as many words:
a flow or a domain type may name a tool client (
workspace_statereadsclients::git); a client never names a flow.
src/clients/devpod_home.rs:36 is use crate::flows::repo_manager::system_words;. It is the only use crate::flows:: anywhere under src/clients/ — git grep -rn "use crate::flows" rust/devlaunch-core/src/clients/ returns exactly that one line, and none existed at the merge-base. The move created it: system_words was in-layer while repoint_devpod_source lived in flows/lifecycle.rs, and became an upward dependency the moment the function crossed into clients/.
Sharper still, system_words now has no other consumer outside its own module: flows/lifecycle.rs dropped it from its import list in this very diff, so a flows:: helper's only external caller is a clients:: module. It is std::io::Error prose-trimming — nothing about it is a flow — so osext or domain is where it belongs.
This is the finding worth the most, because it is exactly the shape of defect #394 was filed to remove, introduced by the fix. It is also inconsistent with the PR's own stated reasoning: the body rejects repoint(&Adoptable) on the grounds that "an adapter under clients/ naming a reconcile-flow type is a seam pointing the wrong way", and then names a reconcile-flow module instead.
2. DevpodHome::at and ::path are pub with no consumer outside the crate [ran]
src/lib.rs:36-37: "everything the binaries do not need stays pub(crate)."
Every DevpodHome use in dl/ and aid/ is DevpodHome::locate() — dl/src/commands.rs:723,798,1017 and dl/src/launch.rs:144. Neither binary calls at() or path(); every caller of both is inside devlaunch-core, and of path() specifically, every caller is a test (verdict_cache.rs:354,382,423,434,445,464,480,507,564,578, provision.rs:4759). So two rows land on public-api.rest.txt — the file docs/development.md says to read "for the accidental pub" — that the stated rule would keep crate-private.
path() is the more interesting one: it hands out the raw root, which is precisely the bare path the module exists to stop anyone joining onto. The guard test only catches a .join("contexts") on it inside devlaunch-core/src.
Not a flat "make them pub(crate)", because it collides with §3 of the Spec axis: if api::workspace_delete is meant to stay callable from api alone, a consumer needs a constructor. Which way this goes depends on #410. But it should be a decision on the record, not a default.
Twelve test sites also read DevpodHome::at(home.path()) where home is a ScratchHome that already derefs to a DevpodHome — a round trip through a path to rebuild a value already in hand. Some((*home).clone()) says it directly.
3. The guard test is sound, and narrower than its own doc implies [ran]
Verified non-vacuous the only way that settles it — I copied tests/devpod_layout.rs onto merge-base 57955a3 and ran it:
test the_devpod_home_adapter_does_spell_it ... FAILED
test only_the_devpod_home_adapter_spells_devpods_on_disk_layout ... FAILED
---- the_devpod_home_adapter_does_spell_it stdout ----
no module spells devpod's on-disk layout, so this guard is guarding nothing
---- only_the_devpod_home_adapter_spells_devpods_on_disk_layout stdout ----
devpod's on-disk layout is spelled outside clients/devpod_home.rs:
src/flows/launch.rs:3153
src/flows/lifecycle.rs:2779
src/flows/lifecycle.rs:2799
src/flows/lifecycle.rs:2859
src/flows/lifecycle.rs:3159
src/flows/lifecycle.rs:3253
src/flows/provision/verdict_cache.rs:322
src/flows/provision.rs:4767
test result: FAILED. 0 passed; 2 failed
Byte-identical to the PR body's claim, on all eight sites. On the branch both pass, and cargo test -p devlaunch-core --lib is 1253 passed, 0 failed. The red is real and the second test does close the delete-it-all hole. Credit where it is due: flows/launch.rs:4192's "contexts: {}\n" — devpod's config.yaml key — is a genuine near-miss the two patterns correctly exclude.
Three holes, none fatal, all worth knowing:
- Only the
contextshead is guarded."workspaces","workspace.json"and"workspace_result.json"are not, so a module holding a record path and joiningparent().join("workspace_result.json")passes. Narrow, since you cannot reach a record without going throughcontexts— but the filename half of the layout is unprotected. - Scope is
devlaunch-core/srconly. The module doc argues for excludingdl's end-to-end tests, which is right; it does not argue for excludingdl/src, which importsDevpodHomeand can reachpath(). Nothing there spells it today. - Non-vacuity rests on one line. Exactly one line in the adapter (
devpod_home.rs:94) satisfiesthe_devpod_home_adapter_does_spell_it, and the check isline.contains("\"contexts\"")with no code/comment distinction — so a future doc comment quoting the literal would let the layout be deleted with the guard still green.
4. repoint's hazards are inherited, not introduced [read]
Diffed the moved body against 57955a3:src/flows/lifecycle.rs. source.display().to_string() (lossy for a non-UTF-8 clone path), the fixed path.with_extension("dl-tmp") (two concurrent repoints of one record race for the same temp name), and the un-fsync'd rename all moved byte-for-byte. Nothing introduced here. Recording them so the next reader does not re-derive them, not charging them to this PR.
DevpodHome::locate() / located() is behaviour-identical to the removed lifecycle::devpod_home(), and the pure split buys a real assertion the old test could not make (empty DEVPOD_HOME → ~/.devpod, not the cwd). Clean.
5. Smaller things [read]
src/lib.rs:8still lists the tool clients as "devpod,git,gh,ssh". A fifth was just added;lib.rsis untouched by the diff.- Two new rustdoc warnings in CI's
public-apilog, both from the new module's docs linkingpub(crate)items from apub mod:devpod_home.rs:20(DevpodHome::repoint) anddevpod_home.rs:59(Self::located). Warnings, not errors, among 113 pre-existing — but these two are new. #[cfg(test)] pub(crate) fn devpod_home_withat module scope is not the old leak relocated: it compiles into no shipped build and appears in no snapshot, wherepub(crate) mod testsput a whole test module into the crate's internal surface. Genuine improvement.ScratchHome'sDerefis a test-only RAII wrapper, not inheritance-by-deref.- The module has real depth — the contexts walk with its four ambiguities, the tri-state
CreateRecord, the atomic rewrite. It is an adapter, not a path-joiner.
Spec
Satisfied, literally, both halves of the ticket's stated red [ran]
"The red is the duplication itself: after the move,
grep -rn '"contexts"' rust/devlaunch-core/srcshould match one module. Andpub(crate) mod testsinlifecycle.rsshould go back tomod tests, with nothing outside importing from it."
- One module:
git grepreturns exactly one hit,src/clients/devpod_home.rs:94. lifecycle.rs:2867ismod tests. Nolifecycle::testsimport survives anywhere — the only surviving mention is a doc comment atdevpod_home.rs:321.repo_manager.rs:1717'spub(crate) mod testsis pre-existing and its file has an empty diff, as the PR body says.- The eight sites replay exactly at merge-base (see the guard output above).
The extraction landed the win the ticket was filed for, and the fixture did not move somewhere equally leaky.
6. public-api.rest.txt is wrong, and CI already says so — BLOCKING [ran/read]
The public-api job is FAILURE on this head (run 32762105921, job 97543106418), and gate fails behind it with results: success success success success failure success failure.
The error is ordering, and only ordering. CI's regeneration moves the thirteen DevpodHome rows from rest.txt:151-163 (before RepointFailure) to after rest.txt:183, immediately preceding pub mod devlaunch_core::clients::gh. The convention is visible one module up: clients::devpod lists its six enums alphabetically and then the Workspace struct, and flows::lifecycle does the same. Enums before structs. RepointFailure is an enum; DevpodHome is a struct.
Two things make this worth stating precisely rather than "regenerate the snapshots":
public-api.api.txt's single hand-edited row is correct. CI's diff touches onlyrest.txt. The re-typedworkspace_deleterow renders exactly as the code declares, and theimpl core::convert::Into<std::path::PathBuf>rendering onat()matches the four neighbouringimpl Traitparams in the file. So is every other hand-edited row —Adoption::Refused::failure,apply_reconciliation,purge_all_data,VerdictCache::under, thedevpod_home()removal, everyRepointFailurerow. One block, in the wrong place, is the whole of it.- The in-repo guard did not catch it, and
docs/development.mdsays it should have. That file claimstests/public_api_snapshots.rsholds the files "to the split itself … so a hand-edited snapshot fails in the Rust suite rather than in review." I ran it on this head:4 passed; 0 failed. It checks which tier each row belongs to, not row order, so a hand-edit that misorders rows sails through the local suite and dies in CI. That claim is falsified by this PR and is worth its own issue.
7. api::workspace_delete stops being callable from api alone — the existing comment holds [ran]
Verified independently, and the standing finding is right. src/lib.rs:125-146 is pub mod api; it re-exports workspace_delete and does not re-export DevpodHome, and lib.rs is not in this diff. The fifth parameter's type has no path through api. Before, it was Option<&std::path::Path> — nameable by anyone. Nothing else in the diff compensates: no pub use, no re-export, no alias.
That is against what #313 settled for Launch and takes the count #395 measured from 5 of 8 to 6 of 8. The finding is correct as posted. I am not overriding its own conclusion — that whether to re-export or to let #410 delete the row is the reviewer's call, and both are open — but the PR body should carry that decision explicitly, because as written it reads as though the re-typing is the whole of the cost.
8. "The regions do not overlap" is false [ran]
"#410 folds the removal guard around
flows/lifecycle.rs:806-1175; this works:2765-2960and its callers, so a rebase is expected but the regions do not overlap."
git diff 57955a3...HEAD -U0 -- .../lifecycle.rs reports hunks at old lines 977 (workspace_delete's devpod_home parameter) and 1095 (devcontainer_volumes). Both sit inside 806-1175. The rebase is trivial — two parameter types — but the claim as written is wrong, and it is the sentence a #410 author would read before deciding how carefully to merge.
9. Two smaller overstatements in the body [ran]
- Under "What moved … whole": "Plus
Host::devpod_config, which was joiningconfig.yamlonto the same bare path".Host::devpod_configdid not move — it is still atflows/launch.rs:230, now delegating toDevpodHome::config. The layout fact moved; the function did not. - The Gate section lists
cargo test --workspace, clippy, fmt,prekandpixi run test"all clean" and does not mention thatpublic-apiis red. The PR does pre-warn that the snapshots are hand-edited and CI is authoritative — but a reader of the Gate section alone would conclude the run was green.
10. Unargued shape deviations from the ticket [read] — suspicion, not a defect
#394 proposed .workspace(id) / .result(id) / .contexts(). What was built is record(context, id) / result(context, id) — an extra parameter and a rename — with contexts() private. The PR body argues repoint's arity, locate's environment read, and the two free functions, at length and convincingly. It does not argue these. They are defensible (ids are unique per context, so id alone cannot address a record), just not on the record in a PR that put everything else on it.
Nothing in the diff is outside #394's scope.
Verdict
Request changes. (Posted as a comment: GitHub refuses --request-changes on a PR from the same account. The written verdict is the one that counts.)
The refactor is good and the ticket is genuinely discharged — eight spellings to one, the pub(crate) mod tests leak gone with nothing importing from it, a guard that I confirmed goes red on the merge-base with byte-identical output to the claim, and 1253 lib tests green. Two blocking findings and one open decision:
clients/devpod_home.rs:36importscrate::flows::repo_manager::system_words, againstlib.rs:15's "a client never names a flow" — the only such import in the crate, introduced by this PR, in the module whose whole point is a correctly-pointed seam. Movesystem_wordsdown a layer.public-api.rest.txtblock ordering. Moverest.txt:151-163to after:183.api.txtis correct as hand-edited; nothing else in either snapshot is wrong. Re-runpublic-apiandgate.api::workspace_delete's parameter type is unnameable fromapi. Not blocking on its own — but the body should record which way it went and why, and #410 should carry the constraint if the answer is "let #410 delete the row."
Non-blocking, in order: at()/path() are pub with no consumer outside the crate; the guard covers only the contexts head and only devlaunch-core/src; lib.rs:8's client roster is stale; two new private-intra-doc-link warnings; the "regions do not overlap" and "Host::devpod_config moved" sentences are wrong; the Gate section omits a red job.
Worth its own issue regardless of this PR: tests/public_api_snapshots.rs passed on a snapshot CI rejects, which contradicts docs/development.md's claim that a hand-edited snapshot "fails in the Rust suite rather than in review". Ordering is not checked. Since cargo-public-api needs a nightly this container cannot install, that local guard is the only thing standing between a hand-edit and CI.
Could not verify: the snapshots themselves cannot be regenerated here (nightly + pinned cargo-public-api, neither installable in this devcontainer), so every snapshot statement above is read against CI's regeneration diff and the file's own neighbouring conventions rather than against a fresh generation. CI's public-api job remains authoritative. I did not run the dl/aid integration suites; CI's rust and e2e jobs are green on this head and the known flakes (#401/#416) did not need to be ruled out.
`clients/devpod_home.rs` was importing `crate::flows::repo_manager::system_words`
— the only `use crate::flows::` under `src/clients/` in the crate, and none
existed before this branch. `lib.rs:15` says the layers run strictly downward and
"a client never names a flow", so the import is exactly the seam-pointing-the-
wrong-way this branch's own body rejected `repoint(&Adoptable)` for. Naming a
reconcile-flow *module* is the same objection as naming a reconcile-flow *type*.
It moves to `osext`, which is where it belonged before it had two callers.
`osext` is the process boundary ported to Python's `os` semantics: `env_str` is
`os.environ.get`, `strip` is `str.strip()`, `home_dir` is
`posixpath.expanduser("~")`, `temp_dir` is `tempfile.gettempdir()` — and
`system_words` is `OSError.strerror`, a reading of the host that exists because
`std::io::Error`'s `Display` appends `" (os error {errno})"` where Python's does
not. Nothing about it was ever a flow; it sat in `repo_manager` because that is
where its first caller was. `osext` is also a leaf that depends on nothing, which
is the only kind of module both a flow and a client may name, so the shared
helper stops being an upward edge for either of them.
`public-api.rest.txt` gets the `DevpodHome` block moved after `RepointFailure`,
which is where the generator puts it and where CI's `public-api` job said it
should be. The convention is one module up: `clients::devpod` lists its six enums
first and `struct Workspace` last, and `flows::lifecycle` does the same. Enums
before structs; `RepointFailure` is an enum and `DevpodHome` is a struct. Nothing
else in either snapshot changes — sorting both revisions of the file shows an
identical multiset of rows, so this is ordering and only ordering, and
`public-api.api.txt`'s one hand-edited row was already right.
Two smaller things while here. `lib.rs:8`'s roster of tool clients still read
"`devpod`, `git`, `gh`, `ssh`" after a fifth was added. And the new module's docs
linked `DevpodHome::repoint` and `Self::located`, both of which are private to
the crate, from a `pub mod` — two fresh `private_intra_doc_links` warnings in
CI's rustdoc output. They become plain code spans; the sentences meant them as
names rather than as destinations.
`DevpodHome::at` and `::path` stay `pub`. They have no consumer outside the crate
and `lib.rs:37` would keep them `pub(crate)` for that, but `locate()` is not a
substitute for `at()` for anyone holding a directory, and whether an outside
caller needs to construct one at all is the `api::workspace_delete` question that
belongs to #410. Narrowing them here would settle that question by default and in
the direction #410 may want reversed.
|
Both blockers fixed in f5a2777, and the body corrections are in. Replying to the review point by point. Fixed§1 — a client named a flow. The layering point generalises, so I checked it: §6 — §5 and §9 — the smaller things. §8 — "the regions do not overlap". Corrected to say they do, with the two hunk offsets, and to tell a #410 author to expect a resolution rather than a clean merge. §9's second half — the Gate section. It now says local green is not the whole gate, names the red Deliberately left§7 / the §2 — §3's three guard holes — the unguarded filename half, the §4 — §10 — The local-snapshot-test gap you flagged as worth its own issue is #431. Gate on this head
|
Three resolutions, only one of which git flagged. CHANGELOG.md: textual conflict, kept both sides. lifecycle.rs:6986: no textual conflict, but the test main gained in #428 (a_record_removed_while_the_plan_sat_there_is_not_reported_as_re_pointed) still bound devpod_home as a bare PathBuf where the signatures this branch rewrites want &DevpodHome. Two E0308s in a tree git called clean. Now DevpodHome::at(..), matching its eight siblings. osext.rs: no textual conflict, but system_words was written pub while the module was pub(crate); #413 has since made osext a pub mod, so the merge would have added a public-surface row absent from both snapshots and reddened the public-api job. Demoted to pub(crate) alongside strip, home_dir and temp_dir, and its doc bullet unbracketed to match. It has no consumer outside the crate.
As pushed, `public-api.api.txt` named `agent_worktrees::Standing` in `RemovalRefused` and did not promise it: the type has no struct or impl rows in that file at all. A consumer holding only `api` got a struct with a field whose type it could not name, which is devlaunch#531's gap and would have been its third instance after #427 and #516. Promoting it honestly was the other option and it is not small. `Standing` reaches `StandingSite`, `Reason`, `Place`, `Blank`, `Subject` and `NonEmpty<Loss>`, and `agent_worktrees` has over three hundred rows in the binary-surface snapshot. That is most of a module's internal vocabulary arriving in the one tier whose worth is being small and stable. So the promoted shape was either incomplete or far too wide, and rendering at the seam is the only option that is both complete and narrow. It is the move the `--ls --json` payload already makes for the wire, at the same boundary and for the same reason. `RemovalRefused` now carries a `RemovalGrounds`, which is made of `String`: `WouldLose`, `CouldNotTell`, or `BothAtOnce`. Three arms rather than two options, because a standing is non-empty and every reason in it answers one of the two, so "neither" cannot happen -- and both render sites carried a fourth arm apologising for being unreachable, which this deletes rather than comments. `BothAtOnce` is what keeps #446 true across the seam: a refusal still never picks one of two true things to say. Nothing inside `flows` changed. `Standing` is exactly as it was, the domain type still carries the whole standing, and the conversion is a private free function at the boundary rather than a method -- a public constructor taking a `Standing` would put it straight back into the promised tier's signatures. Also removes `Standing::any_unproved`, which this branch added and nothing ever called. `Standing` is in the residual, so an uncalled reader there is rows a consumer can bind to for nothing.
1 participant
Closes #394. Map: #406. Unblocks #314 —
flows/lifecycle.rs's:2765-2960group is not a lifecycle submodule at all, so the split has one less section to place, and thepub(crate) mod testsleak that ticket's re-measurement named is gone.The red
rust/devlaunch-core/tests/devpod_layout.rswalks the crate's sources and fails if any file butclients/devpod_home.rsholds a string literal startingcontexts. Before the adapter existed it named exactly the eight sites the ticket lists:A guard rather than a behaviour test, because the failure a second copy of the layout causes is not a wrong answer — it is a copy that agrees with the first until devpod moves it. The second test is the other half, so the guard cannot pass by the layout having been deleted. Every behaviour test that exercised these functions still runs; they moved with the code and now go through the new interface.
What moved
Into
rust/devlaunch-core/src/clients/devpod_home.rs, whole:devpod_home, the record and result path builders,CreateRecord/create_record, the contexts walk,sole_workspace_result,repoint_devpod_sourceandRepointFailure. Plus theconfig.yamljoin thatHost::devpod_configwas doing onto the same bare path, which the ticket did not list — the layout fact moved, not the function:Host::devpod_configis still atflows/launch.rs:230and now delegates toDevpodHome::config.Decisions, and what was rejected:
DevpodHome::attakes the directory; the binary resolves once, per the conventiondl/src/commands.rs:718already states.DevpodHome::locateis the one environment read, split into a thin wrapper over a purelocated(configured, home_dir)— the shapeosext::home_diruses.osextstayspub(crate). The old test could only assert this machine has a home directory; the pure half now pins that an emptyDEVPOD_HOMEfalls through to~/.devpodrather than naming the current directory.repointtakes(context, workspace_id, source), not&Adoptable. Rejected the direct move: an adapter underclients/naming a reconcile-flow type is a seam pointing the wrong way. Its doc — devpod v0.26.1 has no subcommand that changes an existing workspace's source, so the choice is one field of one JSON file or no repair — carries across, and is what makes writing into devpod's own file an adapter's job.system_wordsmoves down toosext, for the same reason.repointbuilds its refusals out of it, and the first cut left it inflows::repo_manager— so the adapter carrieduse crate::flows::repo_manager::system_words, the onlyuse crate::flows::undersrc/clients/, which is the module-level version of the objection the bullet above makes about the type.osextis the process boundary ported to Python'sos:env_strisos.environ.get,home_dirisposixpath.expanduser("~"), andsystem_wordsisOSError.strerror, which exists becausestd::io::Error'sDisplayappends" (os error {errno})"and Python's does not. It is a leaf that depends on nothing, so bothrepo_manageranddevpod_homemay name it.create_recordandsole_workspace_resultstay free functions takingOption<&DevpodHome>. Rejected making them methods: "this machine has no devpod home" is one of the answers each gives, and methods would push that decision to five call sites that do not all read it the same way (one reads the absence as!= NeverCompleted).workspace_delete,purge_all_dataandapply_reconciliationtook a&Pathnameddevpod_home; a bare path is what anybody can join, which is how there came to be eight copies.pub(crate) mod testsis goneflows::lifecycle'smod testsis private again. It waspub(crate)solely soflows/provision.rs:4746andflows/provision/verdict_cache.rs:313could importtests::devpod_home_with. The fixture is now a plain#[cfg(test)]item at module scope in the adapter — deliberately not inside the adapter's ownmod tests, which would have moved the leak rather than removed it. Checked that nothing outside imported anything else fromlifecycle::tests;repo_manager'spub(crate) mod testsis untouched and pre-existing.Public surface
Both snapshots are hand-edited —
cargo-public-apineeds a nightly toolchain this container cannot install, so CI's regeneration is authoritative over these rows.public-api.rest.txt:flows::lifecycle::devpod_homeremoved;flows::lifecycle::RepointFailureand its impls move toclients::devpod_home; newclients::devpod_homemodule withDevpodHome(at,locate,path) and its derives;purge_all_data,apply_reconciliation,workspace_deleteandVerdictCache::underre-typed.public-api.api.txt: one row.api::workspace_delete's fifth parameter isOption<&DevpodHome>where it wasOption<&Path>. This is a change to the promised tier and should be read as one; the argument for paying it is that the parameter was always devpod's home and now says so.apidoes not re-exportDevpodHome, and that is left as it is. Soapi::workspace_deleteis no longer callable fromapialone — before, the parameter was astdtype any caller could name. That runs against what Make api::Launch self-sufficient and type ColdRefused #313 settled forLaunch, and takes the count api::workspace_delete is the delete without the unsaved-work guard #395 measured on this function from 5 of 8 to 6 of 8. The alternative is apub useon the frozen tier, and it is not taken here because Fold the removal guard into one workspace_remove and swap the api row #410 deletesapi::workspace_deleteoutright in favour ofworkspace_remove: the row is due to be removed rather than fixed, and promoting a type onto the promise file only to remove it again is churn on the one file whose diffs are meant to mean something. Fold the removal guard into one workspace_remove and swap the api row #410 carries the constraint: whatever replaces this function must have every parameter type nameable fromapi.DevpodHome::atand::patharepubwith no consumer outside the crate, whichlib.rs:37would ordinarily keeppub(crate). Leftpubdeliberately, because narrowing them decides the question above by default —locate()is no substitute forat()for a caller holding a directory — and in the direction Fold the removal guard into one workspace_remove and swap the api row #410 may want reversed.Gate
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkall clean locally, pluspixi run prekandpixi run test(410 passed).aid/tests/interactive.rs(#401) did not flake on these runs;dl/tests/picker.rs(#416) did once and passed on rerun.Local green is not the whole gate. At
5b52fa1CI'spublic-apijob was red — the hand-editedpublic-api.rest.txthad theDevpodHomeblock beforeRepointFailurewhere the generator puts it after — andgatewas red behind it.f5a2777reorders that block.tests/public_api_snapshots.rspasses either way: it checks which tier a row belongs to, not row order, so a misordered hand-edit dies in CI and not in the local suite. That gap is worth its own issue, and it is why the note above about CI being authoritative is load-bearing rather than boilerplate.Concurrency
Branched from
origin/main. #410 folds the removal guard aroundflows/lifecycle.rs:806-1175; this works:2765-2960and its callers. The regions do overlap:git diff 57955a3...HEAD -U0 -- rust/devlaunch-core/src/flows/lifecycle.rsreports hunks at old lines 977 (workspace_delete'sdevpod_homeparameter) and 1095 (devcontainer_volumes), both inside 806-1175. The conflict is two parameter types and the rebase is trivial, but a #410 author should expect to resolve it rather than expecting a clean merge.🤖 Generated with Claude Code
Summary by Sourcery
Centralize devpod filesystem access behind a typed adapter and update callers and tests to use it.
New Features:
DevpodHomeadapter that centralizes devpod’s on-disk layout, home discovery, record lookup, result tracking, and source repointing.Bug Fixes:
DEVPOD_HOMEvalues fall back to the user’s.devpoddirectory rather than the current directory.Enhancements:
DevpodHomethrough launch, lifecycle, reconciliation, purge, and verdict-cache APIs instead of bare paths.Documentation:
Tests:
Chores: