Retire repos_dir - #481
Conversation
`worktree.repos_dir` is out of `config.toml`, `WorktreeConfig` and the public surface. It had no writer, no documentation, no test that exercised a non-default value, and `XDG_CACHE_HOME` has always been the supported way to move the cache. It also had authority over where clones went, and that is the part worth the change. The clone root is now `xdg::clone_root_in(cache_dir)` and the only production constructor for a clone manager takes the cache directory rather than a root. So every clone dl makes is inside the directory `is_devlaunch_clone` tests against, by construction: the `devlaunch: false` / `SIZE -` / `unsaved: null` degradation on one of dl's own clones has no representation any more, rather than a second predicate to guard it with. The migration is one line on stderr. A config still naming the key gets the directory quoted back, verbatim, with a pointer at XDG_CACHE_HOME and the assurance that nothing at that path was moved or removed. Not an error, because a stale file is not punished here; not silence either, because silence is what strands a tree. Fired whenever the key is present rather than only when it resolves somewhere other than the cache: resolving is what can be silently wrong, and wrong there means saying nothing about a tree nothing else will name. Loading a configuration is now pure but for the read: `ensure_repos_dir` and `is_ours_to_create` go with the key they existed for, and so does `expand_tilde`, since nothing resolves the value any more. Closes #467
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRetires Sequence diagram for retired repos_dir reportingsequenceDiagram
participant Config as config.toml
participant Core as Core config loader
participant Session as open_records
participant Binary as dl report
participant User as stderr
Config->>Core: worktree_config()
Core-->>Session: WorktreeConfig, RetiredKey::ReposDir
Session->>Binary: report(records)
Binary->>User: retired_keys()
User-->>User: One non-fatal migration notice
Note over User: Notice names the configured path and says nothing was moved or removed
File-Level Changes
Assessment against linked issues
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.
This was generated by AI during review.
Reviewed at 0e42ba7d5cf9fa7febc572c4145496192634f613 in fresh context, against #467 and the decision on #460. Locally: cargo test --workspace pass, cargo clippy --locked --all-targets -- -D warnings clean, cargo fmt --check clean, pytest test/ 420 passed / 26 deselected. All 15 CI checks green.
Standards
Non-blocking.
-
test/conftest.py:71-79now calls theXDG_CONFIG_HOMEscoping "belt-and-braces" and justifies keeping it only in the read direction ("keeps a developer's own config.toml out of the suite's assertions"). The same PR addstest/unit/test_config_retired_keys.py:26-31, which writes$XDG_CONFIG_HOME/devlaunch/config.toml. That makes the guard load-bearing again in the write direction: someone acting on the new docstring and dropping thesetenvwould have the suite overwrite a developer's real~/.config/devlaunch/config.toml. Nothing is at risk at this commit — thesetenvis there. The defect is that the reason now recorded for keeping it no longer covers the strongest reason. One sentence. -
rust/devlaunch-core/src/domain/config.rs:232keepsrepos_dir: Option<String>, which is right for detection, but it also keeps the key's power to refuse the whole load. Verified against the release binary:repos_dir = 5makesdl --ls --jsonexit 1 withinvalid type: integer 5, expected a string, on every records-opening command, over a key dl does not read — and the retired-key notice never prints, because the load it rides on never succeeds. The module doc two paragraphs up states "a stale file is not punished". Not a regression (main behaves the same), so nothing to fix under principle 1; it is a veto with nothing left to protect. The comment claiming "a wrong-typed value refuses the load exactly as it did before" is true but unpinned — the wrong-type loop atconfig.rs:427lists three keys and not this one, so relaxing toOption<toml::Value>would go green. -
rust/dl/src/session.rs:111—session::worktree_config()is now a pure pass-through with exactly one caller, in its own module (session.rs:160); its second caller was deleted here. Middle Man. -
rust/devlaunch-core/src/flows/completion_cache.rs:273-277—update_completion_cache(cache_dir, repos_dir)ispuband still takes both, soupdate_completion_cache(cacheA, clone_root_in(cacheB))compiles. The invariantworkspace_clone.rs:334states ("the caller passes the same cache directory it decides ownership with, so the two cannot be built from different answers") is enforced atin_cacheand not at this seam. No production caller does it;commands.rs:381passes the matched pair. -
test/unit/test_config_retired_keys.py:57-63asserts stderr only, noreturncode, unlike its three siblings: a--prunethat started refusing would still pass. -
Vocabulary:
xdg.rs:80establishes "clone root" whileworkspace_clone.rs:305,repo_manager.rsandmigration.rskeeprepos_diras the internal path name (194 hits underrust/). Greppingrepos_dirno longer separates the retired key from the field.
Checked, clean. The tuple return matches MetadataStorage::open exactly. RetiredKey/render::retired_keys mirrors Notice/metadata_notices: typed fact in core, sentence in the binary. clone_root_in belongs in xdg, whose stated job is precisely this non-drift. No dead code, no unused imports. No em or en dash added under docs/; test_docs_prose.py globs README plus docs/*.md only, so AGENTS.md's dashes are out of scope; no docs/ page says "this README".
Spec
Non-blocking. #460 §6 items 1 through 6 are done.
-
docs/cleanup.md:137is now wrong for the exact population this PR exists for. "Two thingsdldoes create are in that second list rather than the first." On a machine that hadrepos_dirset there is a third, and I reproduced it: a seeded workspace under the retired root shows up asLeaving 1 workspace(s) devlaunch did not create. §6.4 asked for theconfig.tomlsentence to go and it went, but the count went with it. "At least two", or keep a clause for the retired key. -
Keeping
StoredWorktree::repos_dir(config.rs:232) is not a violation of §6.1's "Droprepos_dirfromStoredWorktree", despite what the bot review says. §6.1 and §6.2 are in direct tension:StoredConfigdoes not deny unknown keys, so dropping the field makes the key invisible to serde and the §6.2 notice unbuildable. §6.2 is the load-bearing half. The field is private, reaches noWorktreeConfig, and is documented in place. -
The
"/custom/repos"test was updated, not deleted as §6's "How it is tested" says (config.rs:298-311). Divergence in letter, better than the letter: that test pins six keys, and deleting it would lose unrelated coverage. -
§6.1 asks
in_cacheto take "the cache-derived root plusfetch_interval"; it takes&WorktreeConfig. Wider than specified, and it buys the cache-dir/ownership coupling the PR is built on. -
Snapshot rows verified against the declarations, one by one, since they went in by hand:
RetiredKeyplus its five impl rows match#[derive(Debug, Clone, PartialEq, Eq)]and the row shape of the file's other derived enums;worktree_config()'s tuple;in_cache(&Path, &WorktreeConfig, Git<'r>) -> Self;clone_root_in(&Path) -> PathBuf;WorktreeConfig::repos_dirremoved leaving five alphabetical fields. Sort positions correct (ConfigError<RetiredKey<WorktreeConfig<worktree_config;clone_root_in<config_home<devlaunch_cache).defaults_in->defaultsispub(crate)and correctly has no row.public-api.api.txtcorrectly unchanged. CI's regenerate-and-diff passing is the real proof. -
Both #467 seams built: the completeness assertion at
listing.rs:1744, and the notice judged from outside through the shipped binary. #461 untouched and unblocked; #455 correctly out of scope;docs/rust-rewrite-plan.md:172correctly left as the archival record.
What I pressed on, and what I found
(a) The completeness claim: I could not construct the degradation from a configuration. I could construct it from a prior build.
From a configuration, the claim holds and I tried to break it. All five callers of WorkspaceCloneManager::new (migration.rs:1414, workspace_clone.rs:1395, lifecycle.rs:3383, lifecycle.rs:6468, launch.rs:3421) sit past their file's #[cfg(test)] line (432, 1341, 2911, 3242), so pub(crate) is doing no work that #[cfg(test)] is not already doing: in_cache is the only production constructor. Its two production reachers, dl/src/session.rs:173 and dl/src/commands.rs:381, both derive the root with xdg::clone_root_in from xdg::devlaunch_cache(), and nothing in production mutates XDG_CACHE_HOME mid-process. For every clone this build places, devlaunch:false / SIZE - / unsaved:null is unreachable.
It is not unreachable on the machine this migration is for. Seeding one devpod workspace whose localFolder is <retired repos_dir>/blooop/devlaunch/devlaunch-main-3j1t — exactly what a pre-#467 dl left behind on any machine that set the key — the release binary answers:
$ dl --ls --json
[ { "id": "devlaunch-main-3j1t", "devlaunch": false, "repo": null, "branch": null,
"checkedOut": null, "path": null, "state": "Running", ..., "unsaved": null } ]
with SIZE reading - in the table. That is the triad verbatim, and it persists after the key is deleted from config.toml, at which point nothing prints the notice either.
There is a real consequence and not only a wording one: --prune's scan root moved inwards to <cache>/repos, so on those machines dl now reclaims nothing there, where before this PR --prune scanned that tree. #460 §3 accepted the stranding knowingly and refused the alternatives, so I am not asking for code. I am asking for the claim to be narrowed. listing.rs:451-462 says "A clone's directory is clone_root_in of the cache directory plus owner, repo and id, so it is inside cache_dir by construction", and carves out a moved XDG_CACHE_HOME but not a prior build's configured root; #467's breadcrumb says the case "has no way to be built". True of every clone this build places. Not true of the machine the notice is written for.
(b) The notice. The divergence from #460 is the right call, the reach is as claimed, nothing on disk is touched.
I agree with firing on presence rather than on difference. #460's version needs the value resolved and compared against a derived default, and §6.1 deletes the resolver in the same breath; a wrong comparison fails silently toward the one outcome principle 1 forbids. The cost is one true line to a user whose value happened to equal the default.
Reach verified live against the release binary with a devpod shim: --ls --json, --prune -y and --reconcile -y each print it exactly once; dl --ls's table is silent as designed. No double-print is possible from the background refresh child, because ProcessRunner::detach nulls stderr (devlaunch-runner/src/lib.rs:637); each command opens records at most once and cold.rs:43 memoises report().
Principle-1 line holds. The seeded tree at the retired root survived --ls, --prune -y, --reconcile -y and --purge -y intact. Nothing in the tree ever writes the configured value; it reaches render::retired_keys and stops.
(d) --purge is the one omission I would want closed or written down.
rust/dl/src/commands.rs:801 — purge_devlaunch_data goes through lifecycle::purge_plan and never session::open_records, so report() never runs. Verified: with the key set, --purge -y prints no notice. What the user sees instead is
Leaving 1 workspace(s) devlaunch did not create:
- devlaunch-main-3j1t
an opaque id with no path (render.rs:1334 prints workspace.id alone), while the same run deletes <cache>/devlaunch including the metadata.json record that was the last thing pointing at that tree. The tree survives, and the notice returns on the next records-opening command while the key remains in config.toml, so this is not permanent silence. But --purge is the one command whose whole promise is "nothing of devlaunch's is left", and it is the run that will not mention the directory. Naming the sources in the leaving list is #461's; printing this notice is not — render::retired_keys needs only session::worktree_config(). #467's breadcrumb enumerates the commands that say it and omits purge rather than declining it, which is why I would rather see the decision recorded on #461 than left implicit.
(c) The refresh_cache regression is small, and the direction is right.
rust/dl/src/commands.rs:367. Verified with fetch_interval = "soon": dl --refresh and dl --completion-data now exit 0 and do their work, while dl --ls --json still exits 1 naming the file and the parse error. So a broken config is never silently in effect: the only live key left is fetch_interval, refresh_cache no longer reads it, and every path that would act on a config still refuses. What is lost is early diagnosis on three maintenance commands, which is a fair trade against a malformed config blocking completions from installing. Accepted. See Standards 2 for the sharper edge that survives.
(e) expand_tilde is dead, and the value is quoted verbatim.
No reference to config::expand_tilde, is_ours_to_create or ensure_repos_dir survives. The only expand_tilde left in the tree is an unrelated private one at flows/completion.rs:458 over rc-file paths, which pre-dates this PR and is a different signature. Nothing is lost: ~user was already left alone, and the config path was its only caller. Verbatim quoting confirmed live — repos_dir = "~/custom/repos" prints '~/custom/repos', unresolved.
Verdict
Approve. No blocking findings. (--approve is refused on a same-account PR, so this is posted as a comment and the written verdict is the gate.)
The deletion is the right shape, the invariant is genuinely enforced for everything this build places, and the hard principle-1 line holds under test: nothing at a configured repos_dir is moved, deleted, or written by any command I could reach.
Two things I would rather see land before merge, neither of which blocks:
--purgesays nothing (commands.rs:801). Either print the notice there, or record on #461 that purge deliberately does not.test/conftest.py:71-79now understates a guard this same PR made load-bearing in the write direction.
And one wording change with no code behind it: narrow the completeness claim at listing.rs:451-462 to the clones this build places, since the degraded row is still reachable on the machine the notice is written for.
The review built the degradation this ticket closes. Not from a configuration, which is now unable to express placement, but from a prior build: a clone an earlier dl put under repos_dir still reads as someone else's, and retiring the key moved the pruned root inwards, so nothing reaches it. Deleting the key stops the notice too, and then nothing names that tree at all. #460 accepted that stranding knowingly. What is fixed is that no new such clone can be made; what is not fixed is the ones already on disk. Say that in both places rather than claiming the whole population, and correct the count on the contract page, where a third thing was reproduced under a heading that said two.
1 participant
Closes #467. Carries out the decision on #460:
worktree.repos_dircomes out ofconfig.toml,WorktreeConfigand the public surface, nothing on disk is moved or deleted, and the whole of the migration is one line on stderr.The point is the completeness, not the smaller struct
is_devlaunch_cloneis a single containment test against the cache directory, andlayout_of_cloneis gated on the same answer, so one predicate decidesdevlaunch,disk,unsaved, theSIZEcolumn and the picker's owner/repo grouping together. It was only ever sound if nothing could put one of dl's own clones outside that directory, andrepos_dircould.So the clone root is now a function of the cache directory and of nothing else:
xdg::clone_root_in(cache) -> cache.join("repos"), in the module whose doc already exists to keep the cache dir, the clone root andmetadata.json's path from drifting. Unifying the spelling left one way to drift anyway; this closes it.WorkspaceCloneManager::in_cache(cache_dir, config, git). It takes the cache directory, never a root, and derives the root itself.newstill takes an arbitrary root and ispub(crate), which is what the tests drive; no production path can reach it.WorktreeConfigcarries no paths at all now, so a configuration cannot express placement.The binary passes
in_cachethe samecache_dirit decides ownership with, so the two cannot be built from different answers. A clone of dl's own that the listing reads as someone else's is unrepresentable, for a given cache home. (A movedXDG_CACHE_HOMEstill strands what the old one held, as it does for any tool that respects it. That is not this key's doing.)The two reds
1. The completeness claim (
flows::listing,every_clone_a_configuration_can_place_is_one_this_recognises). A configuration naming a root outside the cache, the clone directory a launch would derive from it, and the assertion that the listing recognises it:The test survives the change with only its construction line altered, and it now also asserts
measurable_clone,owner_ofandrepo_ofon the same path, which are the three answers that degraded together.2. The notice (
test/unit/test_config_retired_keys.py), judged from outside through the shipped binary, because "the user is told" is a claim about what a run prints:What the notice says, and where it fires
Core carries the typed fact (
config::RetiredKey::ReposDir { named }) and the binary writes the sentence, like every other notice here. It rides with dl's records, so every command that opens the cache says it once: a launch above all, plus--ls --json,--prune,--reconcile.dl --ls's table stays silent, deliberately, because it reads devpod and nothing else and that one-round-trip property is worth more than a second place to say this.Two calls worth flagging for review:
<cache>/reposas Is repos_dir supported or vestigial #460 wrote it. Deciding "somewhere other" means expanding~and comparing against a derived default, and that comparison is the thing that can be silently wrong: a symlinked home, a cache home that has moved since the tree was made. Wrong in that direction is silence about a tree nothing else will ever name, which is the one outcome principle 1 rules out. A user whose value happens to equal the default is told to delete a dead key, which is true and costs one line. This is also what letsexpand_tildedie as the ticket asked: the value is quoted back verbatim, resolved by nothing.refresh_cacheno longer loads the config at all (its only use of it wasrepos_dir), so--install,--refreshand--completion-datano longer refuse on an unreadableconfig.toml. Consequence rather than goal, and the good direction: a malformed config should not stop completions being installed.Snapshots
Hand-edited into
public-api.rest.txt, since the container has the pinnedcargo-public-apibut no nightly rustdoc backend; CI's regenerate-and-diff is what proves the rendering right. Four changes, all on the binary surface and none inpublic-api.api.txt:WorktreeConfig::repos_dirremoved.config::RetiredKeyadded, with its derived impls.worktree_config()now answers(WorktreeConfig, Vec<RetiredKey>), matchingMetadataStorage::open's shape.WorkspaceCloneManager::from_configbecomes::in_cache, taking the cache directory.xdg::clone_root_inadded.Docs
docs/cleanup.md's purge paragraph loses theconfig.tomlcase, so what--purgeleaves is two things and not three.AGENTS.md's scratch-cache guidance loses the hazard behindXDG_CACHE_HOME: one variable now scopes the whole of whatdlstores, andtest/conftest.py'sXDG_CONFIG_HOMEscoping is belt-and-braces rather than load-bearing, which its docstring now says.docs/rust-rewrite-plan.mdrow 9 is untouched: it records a port that happened.Out of scope
repos_dir" from the list of workspaces a user cannot tell apart, because there is no such workspace any more; the--lsdegradation it deferred to this ticket needed no build work beyond the placement change. Naming the sources in the leaving list is still Name the sources in --purge's leaving list #461's.enabled,cleanup.auto_prune,cleanup.prune_after_daysandfallback_imageare parsed, defaulted, unit-tested and read by nothing outsideconfig.rs. Is repos_dir supported or vestigial #460 recorded that and left it; deciding their fate before the prune unit is decided would be guessing.Summary by Sourcery
Retire worktree.repos_dir and make the cache directory the single source of truth for clone placement and ownership.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: