Derive a workspace id once, and make the triple the way in

CHANGELOG.md

@@ -148,6 +148,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **A workspace id is derived once, and the three signatures that had a triple in
+  hand stopped flattening it into loose strings.** `WorkspaceId::value()` ran the
+  whole derivation on every call — a SHA-256 over the triple, three slug passes
+  and the truncation budget — and a single launch asks for it several times, at
+  every layer it hands the id down through. The derivation now runs at the parse,
+  where the triple is validated, and `value()` lends out what it already has. It
+  answers `&str` rather than `String` for that reason, which is the one row that
+  moves in the `cargo public-api` snapshots.
+
+  What that buys is not the arithmetic. `clone_dir`, `WorktreeInfo::new` and
+  `lifecycle::resolve_known_workspace` each took a triple and its derived id as
+  three or four separate strings, and each of them permitted a disagreement that
+  nothing checked: a clone directory filed under one repository whose leaf is
+  another repository's id, a `metadata.json` record whose stored triple and stored
+  id are about two different workspaces, and a notice reading "addressing
+  `<recorded>` instead of `<derived>` for `<owner>/<repo>@<branch>`" in which the
+  derived id is not one that triple derives. All three take the parsed
+  `WorkspaceId` now, so the disagreement is not expressible and an unvalidated
+  string does not satisfy them.
+
+  The record whose halves disagree is real, though — the derivation moved once and
+  `metadata.json` still holds records written under the old one, which is the
+  state the collision guard and the recorded-id fallback both exist to handle — so
+  there is a test-only constructor named for exactly that, and production reads
+  such a record off disk or not at all.
+
+  Deliberately no `Deref<Target = str>` on the type. It would let a validated
+  triple be spent as a plain string by coercion at every call site that takes one,
+  which is the erasure this change is undoing.
+
 - **`flows::lifecycle` is thirteen modules instead of one 9,000-line file.** It
   held five unrelated commands — stop, delete, purge, prune, reconcile — plus the
   refresh latch, the fetch sweep and the on-disk placement rules that serve the

rust/devlaunch-core/public-api.rest.txt

@@ -542,7 +542,7 @@ pub struct devlaunch_core::domain::workspace_id::WorkspaceId
 impl devlaunch_core::domain::workspace_id::WorkspaceId
 pub fn devlaunch_core::domain::workspace_id::WorkspaceId::label(&self) -> alloc::string::String
 pub fn devlaunch_core::domain::workspace_id::WorkspaceId::new(&str, &str, &str) -> core::result::Result<Self, devlaunch_core::domain::workspace_id::UnsafeName>
-pub fn devlaunch_core::domain::workspace_id::WorkspaceId::value(&self) -> alloc::string::String
+pub fn devlaunch_core::domain::workspace_id::WorkspaceId::value(&self) -> &str
 impl core::clone::Clone for devlaunch_core::domain::workspace_id::WorkspaceId
 pub fn devlaunch_core::domain::workspace_id::WorkspaceId::clone(&self) -> devlaunch_core::domain::workspace_id::WorkspaceId
 impl core::cmp::Eq for devlaunch_core::domain::workspace_id::WorkspaceId

rust/devlaunch-core/src/domain/metadata.rs

@@ -1434,7 +1434,7 @@ mod tests {
         {
             let mut storage = quiet_storage(dir.path());
             storage
-                .add_worktree(WorktreeInfo::new(
+                .add_worktree(WorktreeInfo::as_an_older_dl_recorded_it(
                     "owner1",
                     "repo1",
                     "branch1",

rust/devlaunch-core/src/domain/model.rs

@@ -28,6 +28,8 @@ use std::path::{Path, PathBuf};
 use jiff::civil;
 use serde::{Deserialize, Serialize, Serializer};
 
+use crate::domain::workspace_id::WorkspaceId;
+
 /// What `default_branch` is when a stored entry does not say.
 const DEFAULT_BRANCH: &str = "main";
 
@@ -367,7 +369,46 @@ impl BaseRepository {
 
 impl WorktreeInfo {
     /// A worktree recorded as created and used now.
-    pub(crate) fn new(
+    ///
+    /// Takes the parsed triple rather than its four parts, so the recorded triple
+    /// and the recorded id cannot be about different workspaces. That agreement is
+    /// what `flows::launch`'s collision guard reads this record back for: it
+    /// compares the *triple* to decide whether the record holding a derived id
+    /// belongs to the launch in front of it, and a record whose halves disagreed
+    /// would send it the wrong answer.
+    pub(crate) fn new(workspace: &WorkspaceId, local_path: PathBuf) -> Self {
+        let now = Timestamp::now();
+        Self {
+            owner: workspace.owner().to_owned(),
+            repo: workspace.repo().to_owned(),
+            branch: workspace.git_ref().to_owned(),
+            local_path,
+            workspace_id: workspace.value().to_owned(),
+            created_at: now.clone(),
+            last_used: now,
+            devpod_workspace_id: None,
+        }
+    }
+
+    /// A record whose id is **not** the one its triple derives, as an older `dl`
+    /// left behind.
+    ///
+    /// Test-only, and it has to exist because the state is real: the derivation
+    /// moved once (#81, and again when the suffix scheme changed), so
+    /// `metadata.json` genuinely holds records whose stored id no longer matches
+    /// their stored triple, and a hand-edited file can hold anything at all. That
+    /// is the state `flows::launch`'s collision guard and
+    /// `lifecycle::resolve_known_workspace` both exist to handle, so their tests
+    /// have to be able to build one. Production reads such a record off disk
+    /// through [`Self::from_json`] and cannot construct one: [`Self::new`] is the
+    /// only other way in, and it derives the id from the triple.
+    ///
+    /// The branch is a `&str` rather than a validated ref for the same reason.
+    /// The old derivation coerced unsafe refs instead of rejecting them, so a
+    /// stored branch is not necessarily a legal one -- `flows::migration` reports
+    /// exactly those records as unusable, and the test for it needs one.
+    #[cfg(test)]
+    pub(crate) fn as_an_older_dl_recorded_it(
         owner: &str,
         repo: &str,
         branch: &str,
@@ -956,9 +997,26 @@ mod tests {
 
     #[test]
     fn a_new_worktree_is_created_and_used_at_the_same_moment() {
-        let worktree = WorktreeInfo::new("o", "r", "b", PathBuf::from("/p"), "w");
+        let workspace = WorkspaceId::new("o", "r", "b").expect("a safe triple");
+        let worktree = WorktreeInfo::new(&workspace, PathBuf::from("/p"));
 
         assert_eq!(worktree.created_at, worktree.last_used);
         assert_eq!(worktree.devpod_workspace_id, None);
     }
+
+    #[test]
+    fn a_new_record_holds_the_triple_the_id_beside_it_was_derived_from() {
+        // The collision guard reads this record back and compares the *triple* to
+        // decide whether the id belongs to the launch in front of it, so the two
+        // halves agreeing is load-bearing rather than tidy. They used to be four
+        // independent arguments -- three parts and an id -- with nothing but the
+        // caller's care keeping them about one workspace.
+        let workspace = WorkspaceId::new("blooop", "devlaunch", "main").expect("a safe triple");
+        let record = WorktreeInfo::new(&workspace, PathBuf::from("/p"));
+
+        assert_eq!(record.owner, "blooop");
+        assert_eq!(record.repo, "devlaunch");
+        assert_eq!(record.branch, "main");
+        assert_eq!(record.workspace_id, "devlaunch-main-3j1t");
+    }
 }

rust/devlaunch-core/src/domain/spec.rs

@@ -188,7 +188,7 @@ pub fn identity(spec: &str) -> Result<SpecIdentity<'_>, UnsafeName> {
             repo,
             branch: Some(branch),
         } => Ok(SpecIdentity::Workspace(
-            WorkspaceId::new(owner, repo, branch)?.value(),
+            WorkspaceId::new(owner, repo, branch)?.value().to_owned(),
         )),
         WorkspaceSpec::OwnerRepo {
             repo, branch: None, ..