Derive a workspace id once, and make the triple the way in - #534
Conversation
value() ran the whole derivation on every call -- a SHA-256, three slug passes and a truncation budget -- and a single launch asks several times. The id is derived at the parse now and lent out after that. Three signatures that had a triple in hand and flattened it to loose strings take the parsed triple instead: clone_dir, WorktreeInfo::new and resolve_known_workspace. Each of them permitted a disagreement nothing checked -- a clone directory filed under one repository whose leaf is another one's id, a record whose stored triple and stored id are about two workspaces, a notice naming a derived id that no triple derives. WorktreeInfo::as_an_older_dl_recorded_it is the test-only way to build the record whose halves disagree, because that record is real: the derivation moved once and metadata.json still holds them.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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:
|
Reviewer's GuideThe PR makes Sequence diagram for workspace id derivation and launch resolutionsequenceDiagram
participant Launch
participant WorkspaceId
participant Lifecycle
participant Metadata
Launch->>WorkspaceId: new(owner, repo, git_ref)
WorkspaceId->>WorkspaceId: derive()
WorkspaceId-->>Launch: parsed WorkspaceId
Launch->>Lifecycle: resolve_known_workspace(workspace)
Lifecycle->>WorkspaceId: value()
WorkspaceId-->>Lifecycle: &str
Lifecycle->>Metadata: recorded_id()
Metadata-->>Lifecycle: recorded id or None
Lifecycle-->>Launch: Known workspace or recorded-id fallback
Flow diagram for validated workspace identity propagationflowchart LR
Triple[owner / repo / git_ref] --> Parse[WorkspaceId::new]
Parse --> Cached["derive() once and retain value"]
Cached --> Clone[clone_dir]
Cached --> Record[WorktreeInfo::new]
Cached --> Resolve[lifecycle::resolve_known_workspace]
Clone --> Path[validated clone path]
Record --> Metadata[consistent metadata.json record]
Resolve --> Notice[consistent recorded-id fallback]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
new() built the struct with an empty id and then filled it in, which is a moment where the invariant the type exists for does not hold. parts() is a free function over the triple now, so the derivation runs first and the struct is built once, complete.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review, fresh context, nothing of this branch written by me. Reviewed the three-dot diff against merge base 1509f3397d44181ebaca79a2d969058518df0683 (which is also the current origin/main tip). Preflight: every check green; cargo test --workspace re-run green in an isolated worktree (1,558 lib tests + integration).
The claim this PR turns on -- that the other 66 signatures are not migratable -- was audited independently rather than taken on the PR description. Result up front: it holds.
Standards
Minor -- one new broken intra-doc link, and nothing in CI catches it. rust/devlaunch-core/src/domain/workspace_id.rs:24. Turning the parts method into the free parts_of left the module doc's [`WorkspaceId::parts`] dangling; rustdoc emits unresolved link to WorkspaceId::parts. Measured both sides: main has 0 unresolved links, this branch has 1. Nothing gates cargo doc, so it will sit there. It lands on the module's load-bearing sentence -- "both come out of one WorkspaceId::parts", the claim that value and label agree by construction -- so the one sentence justifying the design now points at nothing. Same stale name at :495 on Parts's own doc, private so rustdoc stays quiet, but equally wrong. Both want parts_of.
Minor -- as_an_older_dl_recorded_it used for records that are not old. flows/listing.rs:2441, :2450, flows/workspace_clone.rs:3120. All three pass the freshly derived id (clean_id, asserted == "demo-feature-j53q" fifteen lines up) beside the triple that derives it, so the halves agree and WorktreeInfo::new(&triple, path) fits exactly. The constructor's name asserts a history the fixture does not have, and spending the escape hatch as the general-purpose test builder is what makes an escape hatch stop reading as one. The other six call sites genuinely hold a non-derived id and read correctly.
Minor -- three new hardcoded goldens outside domain::workspace_id. flows/lifecycle/tests.rs:3398 and :3402 pin "r-main-znkz" twice; domain/model.rs:1020 and flows/repo_manager.rs:2269 add "devlaunch-main-3j1t". The repo already states the rule against this, verbatim, in a sibling module -- workspace_clone.rs's leaf(): "Restating it here would pin the same fact twice and make these tests fail for the wrong reason." workspace_id: workspace.value().to_owned() tests the same property without the second copy.
Nit -- one of the two caching tests does not discriminate. I reduced the assertion and compiled it against a value() -> String shape: the_id_is_derived_once_and_lent_out_after_that genuinely panics on main, in debug and release both (two live String temporaries in one assert_eq! scrutinee cannot share an address). Good test. a_clone_lends_out_its_own_copy_of_the_id passes on main too, so it never separated the implementations -- it documents rather than pins.
Nit -- a second copy of the clone-path layout the PR just made removable. flows/listing.rs:1538-1544: the RecordedOrDerived test double hand-rolls repos_dir.join(owner).join(repo).join(id.value()). Pre-existing, but a WorkspaceId is already in hand two lines up, so it is now clone_dir(&self.repos_dir, &id) exactly.
Checked and clean. No serde on WorkspaceId anywhere, so new() really is the only way in and the value field inside derived Eq/Hash cannot disagree with the three fields above it. as_an_older_dl_recorded_it is #[cfg(test)] and pub(crate), doubly unreachable from production and from the integration tests in rust/devlaunch-core/tests/ (separate crates, no cfg(test)). CHANGELOG sits under [Unreleased] -> ### Changed with zero deletions. test_docs_prose.py globs README.md + docs/*.md only, so neither the CHANGELOG's em dashes nor the -- in Rust doc comments are in scope. Exactly one row in public-api.rest.txt; the promise file and the runner untouched. flows/lifecycle/state.rs is pure plumbing -- the old call site passed exactly workspace.owner()/repo()/git_ref() and workspace.value(), byte-identical. Only 8 production sites gained .to_owned(), every one storing into an owned String field that no Deref/AsRef would have avoided; the no-Deref argument survives contact with the call sites.
Spec
The ticket, in full: "Cache the rendered id, give the type the borrowing impls it needs, and migrate signatures so an unvalidated &str no longer satisfies the downstream paths."
Clause 1, "Cache the rendered id" -- met. workspace_id.rs:354 is the only Self { literal in the crate; fields private, no setter, no &mut accessor, no Deserialize. value is computed from the same three arguments in the same expression, so it cannot drift.
Clause 2, "give the type the borrowing impls it needs" -- met, and the "needs" is empirically zero. I checked the author's reasoning rather than accepting it. No production map is keyed by WorkspaceId: prune_plan::sources_by_workspace returns HashMap<String, String> and prune_status.rs:108 probes it with &record.workspace_id, a recorded String. The only HashSet<WorkspaceId> is a test at workspace_id.rs:1883, never probed by &str. No impl AsRef<str> / impl Into<String> parameter in the tree receives a workspace id. AsRef/Borrow would have been rows with no caller. The one place the change made a call site worse -- launch.rs:7806, Vec::contains becoming .iter().any() -- would not have been fixed by either impl, since Vec<String>::contains wants &String.
Clause 3, "an unvalidated &str no longer satisfies the downstream paths" -- met where migrated, and the un-migrated remainder is verified structural. For clone_dir, WorktreeInfo::new, resolve_known_workspace (and holds_id, a fourth the description does not count): one definition each, all &WorkspaceId, no string overload, so yes, the old call is a type error. Neither migration changed behavior -- WorkspaceId::new stores owner/repo verbatim (folding happens only inside identity_of), so migration.rs:286's destination path is byte-identical; and workspace_clone.rs:874 already destructured the triple out of workspace, so the recorded branch is unchanged and could never have been a ref new() rejects.
The 66: an independent audit enumerated 93 raw hits, discarded the non-id parameters, traced 34 of the surviving ~62 in depth across all ten files, and got 3 TRIPLE / 59 OPAQUE / 0 MIXED. 0 MIXED is not merely observed, it is unreachable: there are exactly two merge points where a triple could survive downward, launch.rs:3350 place_triple and dl/src/target.rs:164-215 triple(), and each reduces the WorkspaceId to a String inside its own body. Grep confirms it by construction -- no production call site anywhere passes workspace.value() into any of the 62. Stronger than the PR claims: three of the 59 would regress if migrated. listing.rs:1171 unsaved_work_in, reached from the dl <ws> rm guard, looks a record up by recorded id; demanding a triple and deriving would miss every old-scheme record and silently disarm the unsaved-work guard. workspace_clone.rs:1257 remove_workspace_by_id has that exact regression recorded in its own doc comment. ssh.rs:100 host_alias must spell the id devpod published.
Minor -- two TRIPLE-shaped signatures do remain, both invisible to a workspace_id: &str grep. Neither is a defect and neither blocks:
flows/workspace_clone.rs:532 prepare_cold(storage, owner, repo, branch, ...). Sole production callerlaunch.rs:3016holds a&WorkspaceIdand destructures it so that:544can re-derive and re-validate the same three strings.PrepareColdError::UnsafeTripleis therefore an arm production cannot reach. Defensible as a deliberate parse boundary, but unlikeworkspace_pathit has one caller and that caller already parsed.flows/lifecycle/state.rs:200 recorded_devpod_workspace_id(storage, owner, repo, branch). Sole production caller islaunch.rs:2865-- a call site this PR wrote, flattening&WorkspaceIdinto three strings two lines after removing the same shape next door.
Minor -- clause 4's tier. WorkspaceId is one of the 36 residual types, so by docs/development.md:95-96's own rule a moved row on it reads as "a contract change", not routine. Nothing external breaks (wf goes through dl --ls --json), but the CHANGELOG's "the one row that moves" does not say which tier it moved in. Separately, the PR body's "579 rows" does not match the doc's deliberately-round "close to six hundred"; the guard diffs only the type count (36, unchanged), so nothing is at risk -- worth a second look at the figure before it is quoted again.
Disposition on #316
The operative clause names no number: 69 is the evidence that opens the question, not a target. Given that 59 of 62 cannot take a WorkspaceId without inventing a triple, and three of them would regress if they did, #316 as literally written is not achievable -- leaving it open would park an impossible target on map #299. So: close it with this PR, but only once the successor exists. The real finding here is the rendered id type the author argues for on the ticket, and if #316 closes with that argument living only in a comment thread, the finding evaporates with the issue. Open the successor first, name the four legitimate producers in it, and fold prepare_cold and recorded_devpod_workspace_id in as its small mechanical half.
Verdict
Comment. (No --approve: GitHub refuses it on a same-account PR. Read this as an approve-with-follow-ups; the written verdict is the gate.)
Nothing blocks the merge. The diff is regression-free, the caching is real and pinned by a test that genuinely fails against main, the three migrations are correct, and the empirical claim the PR rests on survived independent audit. The findings above are all one-line fixes or follow-ups, and the only one I would ask for before merge is the two-word parts -> parts_of doc repair, since nothing in CI will ever catch it.
The method became a free function in this branch, so both [`WorkspaceId::parts`] references dangled. cargo doc is back to zero unresolved links, which is where main is.
1 participant
Closes #316
WorkspaceIdvalidated a triple, derived an id from it, and then the id wentback to being a string. This does two things about that: the derivation runs
once instead of on every read, and the three signatures that genuinely had a
triple in hand stop flattening it into loose strings.
The derivation runs at the parse
value()ran the whole thing on every call -- a SHA-256 over the folded triple,three slug passes and the truncation budget -- and a launch asks several times,
once at every layer it hands the id down through. It runs in
new()now, wherethe triple is validated, and
value()lends out the string it already has. Thatis why it answers
&strrather thanString, which is the one row that moves inthe snapshots.
The test that pins it is on where the bytes live, not on what they say:
Comparing the strings would pass either way. Against
mainthis fails with twodifferent addresses; a second test holds the other side of it, that a clone gets
its own copy and borrows nothing from the original.
Three signatures take the triple
clone_dir,WorktreeInfo::newandlifecycle::resolve_known_workspaceeachtook a triple and its derived id as three or four separate strings, and each
permitted a disagreement that nothing checked:
repository's id,
metadata.jsonrecord whose stored triple and stored id are about twodifferent workspaces -- which is the exact pair the collision guard reads back
and compares,
<recorded>instead of<derived>for<owner>/<repo>@<branch>" in which the derived id is not one that triplederives.
All three take the parsed
WorkspaceIdnow. The old call is a type error:The record whose halves disagree is real, so it is still constructible -- the
derivation moved once and
metadata.jsonholds records written under the oldone, which is what the collision guard and the recorded-id fallback exist for.
WorktreeInfo::as_an_older_dl_recorded_itis#[cfg(test)]and named forexactly that; production reads such a record off disk through
from_jsonor notat all.
Every test call site that had to change is the gate working. The
resolve_known_workspacetests were passing(OWNER, REPO, "main")beside amade-up
"r-main-new", which is the defect in miniature, and now probe the idthe triple actually derives.
No
Deref, and whyDeliberately no
Deref<Target = str>, noAsRef<str>, noBorrow<str>.Derefis the pointed one: it would let a validated triple be spent as a plain string by
coercion and in silence at every call site that takes one, which is the erasure
this change undoes. The other two buy nothing today -- nothing generic over
impl AsRef<str>consumes a workspace id and no map is keyed by one -- so theywould be surface with no caller. The reasoning is written on the type.
What is not here, and it is most of the 69
The ticket counted 69 production signatures taking
workspace_id: &str. Three ofthem had a triple behind them. The other 66 do not, and it is not an accident:
the id reaching those paths is often not a triple's derivation at all. It can
be a ref-less git source's hashed id, a path spec's directory leaf, a bare name
the user typed, an id
devpod listprinted, or an idmetadata.jsonrecordedunder an older scheme.
Placementcarries aStringfor that reason and says sobeside the field: a triple is not recoverable from an id.
So there is a real boundary in the middle of the ticket. Above it a triple exists
and the signature can demand one, which is what this does. Below it, demanding
one would mean inventing a triple for workspaces that have none, and
dl --purgeand
dl --pruneare defined over exactly those. Making those paths refuse anarbitrary string wants a second type -- a rendered id whose constructors are
the four legitimate producers -- and that is a design decision with behaviour
attached at each of those boundaries, not a mechanical migration. The
measurement, function by function, is in a comment on #316.
Gates
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check,pytest test/andpixi run ci(pylint 10.00/10) allgreen. The three
cargo public-apisnapshots were regenerated on nightly withthe pinned cargo-public-api 0.52.0, after a control run confirmed the toolchain
reproduces
origin/main's three files byte identically. One row moves,WorkspaceId::value's return type; the promise file and the runner file areunchanged.
--print-residualis byte identical tomain's at 36 types and 579rows, so the figures in
docs/development.mdstand.Summary by Sourcery
Cache derived workspace IDs at parse time and pass validated workspace identities through triple-aware APIs to prevent inconsistent workspace addressing.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: