feat(minimald): implement RPC for creating sessions, index by session name also - #265
Conversation
📝 WalkthroughWalkthroughAdds end-to-end session creation: persistent index and name-uniqueness in the sessions store, a manager actor create API, and an SSH oneshot CreateSession RPC with integration tests. ChangesSession Creation Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant SshDispatcher
participant CreateSessionHandler
participant SessionsManager
participant DiskStore
Client->>SshDispatcher: CreateSessionRequest(record)
SshDispatcher->>CreateSessionHandler: spawn handler for CreateSession::NAME
CreateSessionHandler->>SessionsManager: create_session(record)
SessionsManager->>DiskStore: create(record)
DiskStore->>DiskStore: enforce name uniqueness, insert into Index
DiskStore-->>SessionsManager: Uuid
SessionsManager-->>CreateSessionHandler: Uuid
CreateSessionHandler->>Client: CreateSessionResponse(id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/minimald/src/rpc.rs`:
- Around line 200-203: CreateSessionRequest's record field is private so
external clients cannot build the request; make the field public by changing the
struct field `record: sessions::Record` to `pub record: sessions::Record` in the
CreateSessionRequest definition (the same pattern used for other
request/response structs) so that code exercising the OneshotSshRpc
Serialize-bound Request can construct and serialize CreateSessionRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1664f24e-050e-47ff-a68e-dbe25918c3e2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/minimald/Cargo.tomlcrates/minimald/src/rpc.rscrates/minimald/src/sessions.rscrates/sessions/src/lib.rscrates/sessions/src/store.rs
dca16e7 to
cb1b680
Compare
cb1b680 to
d1eb82d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/minimald/src/rpc.rs (1)
359-399: ⚡ Quick winConsider covering the name-uniqueness conflict path.
This test validates only the happy path. Since enforcing unique session names is the primary goal of this stack, a test that issues a second
CreateSessionwith a duplicate name and asserts the RPC surfaces an error would guard the handler's error-propagation contract.Want me to draft that conflict-path test?
🤖 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/minimald/src/rpc.rs` around lines 359 - 399, Add a new test (or extend create_session_shows_in_get_and_list) to exercise the name-uniqueness conflict by calling the CreateSession RPC twice with the same sessions::Record.name via CreateSessionRequest and asserting the second call returns an error; specifically, call client.call::<CreateSession>(&CreateSessionRequest { ... name: "my session" ... }).await again and assert the RPC surfaces a uniqueness violation (match the returned error/result shape your RPC layer uses), ensuring the test references CreateSession, CreateSessionRequest, and the original session id/name for clarity.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/sessions/src/lib.rs`:
- Around line 21-23: Update the doc comment for the Record.id field to clearly
state that deserialized or caller-supplied ids may be Uuid::nil() and that the
store layer (Loader::create) will overwrite such incoming ids with a newly
generated id (Uuid::now_v7()); reference both Uuid::nil and
Loader::create/Uuid::now_v7 in the comment so callers know nil values are
expected and replaced by the store during creation.
---
Nitpick comments:
In `@crates/minimald/src/rpc.rs`:
- Around line 359-399: Add a new test (or extend
create_session_shows_in_get_and_list) to exercise the name-uniqueness conflict
by calling the CreateSession RPC twice with the same sessions::Record.name via
CreateSessionRequest and asserting the second call returns an error;
specifically, call client.call::<CreateSession>(&CreateSessionRequest { ...
name: "my session" ... }).await again and assert the RPC surfaces a uniqueness
violation (match the returned error/result shape your RPC layer uses), ensuring
the test references CreateSession, CreateSessionRequest, and the original
session id/name for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1380d9a5-27d9-48de-ac12-7228e020fa70
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/minimald/Cargo.tomlcrates/minimald/src/rpc.rscrates/minimald/src/sessions.rscrates/sessions/src/lib.rscrates/sessions/src/store.rs
✅ Files skipped from review due to trivial changes (1)
- crates/minimald/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/minimald/src/sessions.rs
- crates/sessions/src/store.rs
| .iter() | ||
| .map(|(short, id)| Self::Key { | ||
| session_uuid: *id, | ||
| dir_key: DaemonRelPath::try_new(short).unwrap(), |
There was a problem hiding this comment.
I suspect this is something that's going to keep coming up. We probably want to newtype Uuid as SessionId (this would be good regardless) and either implement a to_daemon_rel_path on it.
We should probably also add new_unchecked constructors (with big warnings on them) for the path types and just be really careful about where we use them.
Don't think we can do From<SessionId> for DaemonRelPath since we were talking about moving the paths module to its own crate and the sessions crate will be downstream of that.
There was a problem hiding this comment.
Slightly out of scope but we probably want to rename this method to keys which is more standard.
| pub struct DiskLoader { | ||
| minimal_dir: DaemonAbsPath, | ||
| index: BTreeMap<String, Uuid>, | ||
| index: Index, |
There was a problem hiding this comment.
Just to clarify, is this going to be always kept in sync or is it going to be lazy? If lazy, we probably want to wrap it in RefCell so any future read-only methods don't need to take &mut self. Looks like it's kept in sync so far, just wanted to check.
| } | ||
|
|
||
| impl Index { | ||
| pub fn insert(&mut self, short: String, uuid: Uuid, name: Option<String>) { |
There was a problem hiding this comment.
Minor, but do we want search to live in the index type as well?
|
Thanks! Given the dep from a later PR I'm going to merge this as-is and send a follow-up PR for the changes (newtype |
DiskLoaderindexes by name alsoCreateSessionRPC.This is technically breaking the format of
index.json, except there was never a path to write it before, so it should be empty.Summary by CodeRabbit
New Features
Improvements
Chores
Tests