Skip to content

devpod's on-disk layout is spelled in four places, and its module is missing #394

Closed

Description

@blooop

Finding

devpod's on-disk layout is knowledge with no module. clients/devpod.rs is the seam for devpod-the-command; devpod-the-filesystem has none, so the path is rebuilt wherever it is needed.

rust/devlaunch-core/src/flows/lifecycle.rs:2861 already claims otherwise — the paths go through shared helpers "so devpod's on-disk layout is spelled out in one place and not three". It is spelled in four:

  • flows/lifecycle.rs:2779 — contexts/<ctx>/workspaces/<id>/workspace.json
  • flows/lifecycle.rs:2799 — .../workspace_result.json
  • flows/lifecycle.rs:2859 — read_dir(devpod_home.join("contexts"))
  • flows/provision/verdict_cache.rs:322 — contexts/.../workspaces/.../workspace_result.json

And four more times in tests, each reproducing the convention literally: flows/launch.rs:3153, flows/lifecycle.rs:3159, flows/lifecycle.rs:3253, flows/provision.rs:4767.

The missing module has already cost real surface. Because a devpod-home builder had nowhere to live, lifecycle's test module was promoted to pub(crate) mod tests so two other flows could import a fixture from it:

  • flows/provision/verdict_cache.rs:313 — use crate::flows::lifecycle::tests::devpod_home_with;
  • flows/provision.rs:4746 — same fixture

A test fixture is part of the crate's internal surface because a module is missing.

Shape

A DevpodHome adapter under clients/, taking the home path rather than resolving it — the binary already resolves lifecycle::devpod_home() and hands it down (dl/src/commands.rs:718, with the reasoning in the comment there), so the convention exists.

DevpodHome::at(path)
  .workspace(id)   -> the workspace record
  .result(id)      -> the create result
  .contexts()      -> the context walk
  .repoint(id, source)

repoint_devpod_source (lifecycle.rs:2936) writes into devpod's own JSON, and its doc already argues why (devpod v0.26.1 has no subcommand that changes an existing workspace's source). That is an adapter's job; it is currently sited in flows/.

Two adapters make the seam real: the real home in production, a temp-dir one for tests — which is what devpod_home_with already is, minus a home to live in.

Proving it

The red is the duplication itself: after the move, grep -rn '"contexts"' rust/devlaunch-core/src should match one module. And pub(crate) mod tests in lifecycle.rs should go back to mod tests, with nothing outside importing from it.

Found by an architecture review; the full write-up is in the review report, not in the repo.

Activity

  1. self-assigned this
    on Aug 24, 2026
  2. blooop commented on Aug 24, 2026

    @blooop
    OwnerAuthor

    Built in #427 (refactor/devpod-home-adapter), open against main, not merged.

    The finding held, and the count was exactly right: eight sites, four in implementation and four in fixtures. The red is a source guard — devlaunch-core/tests/devpod_layout.rs fails if any file but clients/devpod_home.rs holds a string literal starting contexts — and it named all eight before the module existed. A behaviour test could not have caught this: a second copy of the layout gives the right answer until devpod moves it.

    Three things beyond what the ticket listed:

    • Host::devpod_config was joining config.yaml onto the same bare path, so it moved too.
    • repoint takes (context, workspace_id, source) rather than &Adoptable. Moving it as-is would have left an adapter under clients/ naming a reconcile-flow type, which is a seam pointing the wrong way.
    • create_record and sole_workspace_result stayed free functions taking Option<&DevpodHome> instead of becoming methods. "No devpod home" is one of the answers each gives; as methods, five call sites would each have to decide again what a missing home means, and they do not all read it the same way.

    flows::lifecycle's mod tests is private again — the fixture is a plain #[cfg(test)] item at the adapter's module scope, deliberately not inside the adapter's own mod tests, which would have moved the leak rather than removed it. Nothing outside imported anything else from there.

    One thing for a reviewer to weigh: api::workspace_delete's promised signature changes (one row of public-api.api.txt) because the pass-through parameters now take DevpodHome where they took &Path. Both snapshots are hand-edited; CI's regeneration is authoritative.

    Leaving this open per the map's rule.

  3. blooop commented on Aug 24, 2026

    @blooop
    OwnerAuthor

    wf-review on #427: request changes — two blockers. clients/devpod_home.rs:36 imports crate::flows::repo_manager::system_words, against lib.rs:15's "a client never names a flow" (the only such import in the crate, introduced by the move), and public-api.rest.txt has the DevpodHome block before RepointFailure where the generator puts it after — CI's public-api and gate are red on it. The ticket's own red is discharged: eight spellings to one, pub(crate) mod tests gone, and the guard confirmed failing on the merge-base with byte-identical output. Full report: #427 (review)

  4. blooop commented on Aug 24, 2026

    @blooop
    OwnerAuthor

    Review findings on #427 addressed in f5a2777; the PR is green on every check and still open, not merged.

    Two blockers, both fixed. The move had left clients/devpod_home.rs importing crate::flows::repo_manager::system_words — the only use crate::flows:: under src/clients/ in the crate, and the exact shape of upward seam this ticket was filed to remove. system_words now lives in osext, the leaf that ports the process boundary to Python's os semantics: it is OSError.strerror, sitting beside os.environ.get, str.strip(), posixpath.expanduser("~") and tempfile.gettempdir(), and it exists for the same reason they do — std::io::Error's Display appends " (os error {errno})" where Python's does not. A leaf is the only module both flows::repo_manager and clients::devpod_home can name without one of them pointing upward, and git grep "use crate::flows" over src/clients/, src/domain/ and src/osext.rs now returns nothing. The second was public-api.rest.txt ordering: the hand-edit put the DevpodHome struct block before the RepointFailure enum block where the generator puts it after, which is the enums-then-structs convention clients::devpod and flows::lifecycle both already show. CI's public-api job was red on it and is green now.

    The PR body was corrected on three counts the review caught: the #410 regions do overlap (hunks at old lifecycle.rs 977 and 1095 sit inside 806-1175), only the config.yaml join moved rather than Host::devpod_config itself, and the Gate section now names the red job instead of reading as though the run was clean.

    Left deliberately, and recorded in the body rather than acted on: api still does not re-export DevpodHome, so api::workspace_delete is not callable from api alone. #410 deletes that function in favour of workspace_remove, so the row is due for removal rather than repair — and #410 inherits the constraint that every parameter type of whatever replaces it must be nameable from api. DevpodHome::at and ::path stay pub for the same reason: narrowing them would settle that question by default and in the direction #410 may want reversed.

    The ticket's own red is unchanged and still discharged. Leaving this open per the map's rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions