feat!(sessions): newtype session ID, refactor store index - #275

crates/sessions/src/lib.rs (1)

33-37: ⚡ Quick win

Inherent to_string shadows Display trait method.

The inherent to_string() returns the raw UUID string (self.0.to_string()), but Display formats as SessionId(uuid). This inconsistency triggers Clippy's inherent_to_string_shadow_display lint and may confuse callers expecting consistent behavior between format!("{}", id) and id.to_string().

Consider removing the inherent method and relying on Display, or aligning both to return the same format.

Option A: Remove inherent method, use Display for plain UUID

-    /// Constructs a string representing the given UUID.
-    #[must_use]
-    pub fn to_string(&self) -> String {
-        self.0.to_string()
-    }
 }
 
 impl AsRef<Uuid> for SessionId {
     fn as_ref(&self) -> &Uuid {
         &self.0
     }
 }
 
 impl fmt::Display for SessionId {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "SessionId({})", self.0)
+        write!(f, "{}", self.0)
     }
 }

Option B: Keep both, rename inherent method

-    /// Constructs a string representing the given UUID.
+    /// Returns the underlying UUID as a hyphenated string.
     #[must_use]
-    pub fn to_string(&self) -> String {
+    pub fn as_uuid_string(&self) -> String {
         self.0.to_string()
     }

Also applies to: 46-50

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/lib.rs` around lines 33 - 37, The inherent pub fn
to_string(&self) on SessionId shadows the Display trait's to_string and triggers
clippy; fix by removing or renaming it so behavior is consistent—either delete
the inherent to_string and rely on the Display impl for string conversion, or
rename the inherent method (e.g., as_uuid or to_uuid_string) and update all call
sites; ensure the Display impl for SessionId remains unchanged and adjust the
other similar method at lines 46-50 the same way to avoid duplicate shadowing.

crates/sessions/src/lib.rs (1)

20-20: ⚡ Quick win

Consider deriving Hash alongside Eq.

The SessionId type derives Eq but not Hash. It's a Rust best practice to derive Hash whenever Eq is derived to maintain the invariant that equal values must have equal hashes. Adding Hash also future-proofs the type for use in HashMap or HashSet as keys.

♻️ Add Hash to the derive list

-#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct SessionId(Uuid);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/lib.rs` at line 20, The SessionId struct currently
derives Eq but not Hash; update its derive annotation (the line with
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd,
Ord)]) to also include Hash so SessionId implements std::hash::Hash and can be
used reliably as a key in HashMap/HashSet while preserving the Eq/Hash
invariant.

crates/sessions/src/store.rs (2)

266-271: ⚡ Quick win

Variable name uuid should be id for consistency.

The pattern variable is named uuid but the type is &SessionId, which is inconsistent with the refactoring's naming conventions used elsewhere in this file.

✨ Suggested fix

     fn find_by_name<S: AsRef<str>>(&self, name: S) -> Result<Option<Self::Key>, std::io::Error> {
-        match self.index.find_by_name(name) {
-            Some(uuid) => self.find_by_id(uuid),
+        match self.index.find_by_name(name) {
+            Some(id) => self.find_by_id(id),
             None => Ok(None),
         }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/store.rs` around lines 266 - 271, The match binding in
find_by_name uses the variable name `uuid` while the type is &SessionId and
other code uses `id`; rename the pattern variable to `id` to match the
refactoring conventions: update the match arm from `Some(uuid) =>
self.find_by_id(uuid)` to `Some(id) => self.find_by_id(id)` and adjust any
related local uses in the find_by_name implementation (symbols: find_by_name,
self.index.find_by_name, find_by_id, SessionId, Self::Key).

---

139-145: 💤 Low value

Consider a reverse index for O(1) ID-to-short lookups.

short_by_id performs a linear scan through short_to_id, making find_by_id O(n) in the number of sessions. For typical session counts this is fine, but if session volume grows, a reverse BTreeMap<SessionId, String> (id_to_short) maintained alongside short_to_id would provide O(log n) lookups.

This is acceptable as-is for now given expected usage patterns.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/store.rs` around lines 139 - 145, short_by_id currently
scans self.short_to_id linearly making lookups O(n); add a reverse index (e.g.,
a BTreeMap<SessionId, String> named id_to_short) and change short_by_id to
consult id_to_short for O(log n) lookups. Maintain id_to_short wherever
short_to_id is mutated: on insert/update populate
id_to_short.insert(session_id.clone(), short.clone()), and on remove delete
id_to_short.remove(session_id). Update any constructors, insert/remove helpers,
and tests that touch short_to_id to keep both maps consistent.