Repository navigation
feat(sessions): update session status - #597
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds ChangesSession status field and store validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/store.rs`:
- Around line 92-96: Update the error documentation for Store::save and
Store::rename to include the stale-key case that returns ErrorKind::NotFound.
The current docs for save only mention InvalidInput, AlreadyExists, and I/O
errors, so add NotFound with a brief description that it occurs when the
provided key is stale; make sure rename’s docs (or its shared trait/docs block)
reflect the same inherited behavior from save.
- Around line 756-767: The current guard in save only checks that short exists
in index, but it must also verify that the stored session id matches the key’s
id before writing. Update the check around short_to_id in store.rs so save
compares the resolved id for short against key.id() and returns NotFound if they
differ, preventing stale keys from overwriting a reused short. Use the save path
and the short_to_id lookup in SessionStore as the main place to enforce this
ownership check.
🪄 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: 2c1cb62f-a4a7-4c2f-9411-414b9555c761
📒 Files selected for processing (10)
crates/minimal2/src/main.rscrates/minimald/src/exec.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/sftp.rscrates/minvmd/examples/exec.rscrates/minvmd/tests/minimald_session_e2e.rscrates/sessions/src/lib.rscrates/sessions/src/store.rs
| } | ||
| } | ||
|
|
||
| /// Lifecycle status of a [`Record`]. |
Contributor
There was a problem hiding this comment.
SessionStatus implies its the state machine for the session- given its more the loadout vibes, do we want to call it Loadout or LoadoutStatus ?
Member
Author
There was a problem hiding this comment.
I think it is actually part of the session lifecycle itself. A session is created as a Draft once the client sends the CreateSession request, along with the loadout information, and then it becomes active once the client gets back to it with the verdict on package and project contributions. It's not really a loadout specific thing.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/sessions/src/store.rs (1)
95-97: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the reused-short
NotFoundcase.Line 95 says stale keys are only when the short is no longer in the index, but
savealso returnsNotFoundwhen that short now maps to a different session id.Suggested doc update
- /// - `NotFound` if `key` is stale — its short is no longer in the - /// index (e.g. the session was deleted after the caller obtained - /// the key). Prevents silent resurrection of removed sessions. + /// - `NotFound` if `key` is stale — its short is no longer in the + /// index, or it now maps to a different session id (e.g. after + /// deletion and short reuse). Prevents silent resurrection or + /// overwriting an unrelated session.🤖 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 95 - 97, Update the `SessionStore::save` docs to cover the reused-short `NotFound` case as well as stale keys. The current comment only mentions the key being stale when its short is missing from the index, but `save` can also return `NotFound` when the short exists and now points to a different session id. Adjust the doc comment near `save` and the `NotFound` bullet so both conditions are clearly described.
🤖 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.
Duplicate comments:
In `@crates/sessions/src/store.rs`:
- Around line 95-97: Update the `SessionStore::save` docs to cover the
reused-short `NotFound` case as well as stale keys. The current comment only
mentions the key being stale when its short is missing from the index, but
`save` can also return `NotFound` when the short exists and now points to a
different session id. Adjust the doc comment near `save` and the `NotFound`
bullet so both conditions are clearly described.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2a08f8f-834a-40e4-82a3-87420c64a94e
📒 Files selected for processing (1)
crates/sessions/src/store.rs
evanspearman
force-pushed
the
evan/updatesession
branch
from
June 29, 2026 17:10
c308c6a to
32a5a8e
Compare
twitchyliquid64
approved these changes
Jun 29, 2026
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Storage scaffolding for the daemon-side draft-session flow.
SessionStatus { Draft, Active }onRecord(defaults toActive,#[serde(default)]for backward-compat with on-diskrecords predating the field). Store records the value verbatim;
state-machine rules belong to higher layers.
Loader::save(&mut self, key, record). Overwrites the rowin place; refuses id changes (
InvalidInput), stale keys(
NotFound), and real name collisions (AlreadyExists).Same-name re-save is a no-op. Atomic via the existing
temp + renameat2plumbing.Loader::renameis now sugar oversave(one write path).RecordderivesPartialEq + Eq; trivially supported byevery field.
Summary by CodeRabbit
New Features
Bug Fixes