feat(sessions): tighten daemon composer pending/ready routing - #602
Merged
Conversation
# Conflicts: # crates/minimald/src/sessions.rs
|
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 (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR reworks Phase 2 session composition so ChangesCompose Outcome Routing
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant SessionComposer
participant Manager
participant SessionStore
Manager->>SessionComposer: compose(session_id, options)
SessionComposer->>SessionComposer: inspect vars and patches
alt all-decided
SessionComposer-->>Manager: ComposeOutcome::Ready(Composition)
Manager->>SessionStore: persist Active session record
Manager-->>Manager: return CreateSessionResponse::Ready { id }
else pending
SessionComposer-->>Manager: ComposeOutcome::Pending { response, state }
Manager-->>Manager: reject as InvalidInput
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
crates/minimald/src/rpc.rs (1)
628-639: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the SSH-derived username is persisted.
CreateSessionnow intentionally takesusernamefrom the SSH connection context rather than the request. Add an assertion here so this RPC path catches regressions that storeNoneor accidentally reintroduce client-supplied identity.Suggested assertion
assert_eq!( get_session.record.as_ref().unwrap().project_path, HostAbsPath::try_new("/uwu").unwrap() ); + assert_eq!( + get_session.record.as_ref().unwrap().username, + Some("test".to_string()) + );🤖 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 628 - 639, The GetSessionRecord assertion block in the CreateSession RPC test should also verify that the SSH-derived username is persisted. Update this test around the existing get_session.record checks to assert the record’s username matches the SSH connection context value, so the CreateSession path continues to use server-derived identity rather than client-supplied data.
🤖 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-rpc/src/lib.rs`:
- Around line 227-231: Update the documentation for the `Pending` variant in
`minimald-rpc::lib` so it reflects the current state of daemon-side Phase 2
routing instead of describing it as purely reserved for a future milestone.
Reword the comment near the `Ready`/`Pending` enum docs to mention that
`Pending` is now part of the Phase 2 flow and that the remaining missing piece
is the manager/`SubmitVerdict` resume path. Keep the wire-shape compatibility
note, but make sure callers understand `Pending` is no longer theoretical.
In `@crates/minimald/src/sessions.rs`:
- Around line 83-86: Refresh the comment on the contribution field in
SessionComposer-related session state so it matches the current behavior: the
manager now passes non-empty WireContribution through this boundary instead of
always rejecting it. Update the documentation around contribution to reflect the
actual decision point—either Ready Composition is applied or non-empty input is
rejected—so the comment no longer says the handler only accepts the default and
defers everything to a future phase.
- Around line 293-346: The CreateSession flow in sessions.rs is discarding the
Ready result from SessionComposer::compose, so any merged WireContribution is
lost while only the out-of-band sessions::Record is persisted. Update the Ready
branch in the CreateSession handler to either apply/plumb the returned
composition into the session persistence path before calling self.store.create,
or reject non-empty compositions until that support exists, using
SessionComposer::new, compose, ComposeOutcome::Ready, and sessions::Record as
the key locations.
In `@crates/minvmd/examples/exec.rs`:
- Line 30: The CreateSession response handling in exec.rs is decoding the wrong
type, so it will miss daemon error messages when the response is an error
wrapper. Update the CreateSession response decode path to use
Errorable<CreateSessionResponse> first, then match on Ready only after
unwrapping the wrapper, following the same pattern used by the e2e helper and
the CreateSession handling around the minimald_rpc types.
In `@crates/minvmd/tests/minimald_session_e2e.rs`:
- Around line 228-236: The CreateSession retry in minimald_session_e2e is
reusing the hardcoded session name, which causes later attempts to collide with
the first created session instead of retrying the flaky exec path. Update the
session name generation in the CreateSession setup so each retry attempt gets a
unique value, following the same per-invocation suffix approach used by exec.rs,
and keep the change localized to the SessionConfig name field in
minimald_session_e2e.
In `@crates/sessions/docs/COMPOSITION.md`:
- Around line 135-144: Update the Response shape paragraph in COMPOSITION.md to
match the current behavior: `SessionComposer::compose` already returns
`ComposeOutcome::{Ready, Pending}`, so remove the stale note that `Pending` is
only future wire-defined daemon-side Phase 2 routing. Instead, describe that
`Pending` is currently produced by composition but rejected by the manager with
`InvalidInput`, while `Ready` remains the only accepted path today.
- Around line 92-100: The COMPOSITION docs currently describe packages as being
included in the pending wire response, but `SessionComposer::compose` only sends
vars, patches, and lifecycle hooks in `ContributionResponse` while packages
remain in `PendingComposeState`. Update the pending path wording to match the
actual flow, and make sure references to `ContributionResponse`,
`PendingComposeState`, and `SessionComposer::compose` reflect that packages stay
daemon-side and are not part of the response payload.
---
Nitpick comments:
In `@crates/minimald/src/rpc.rs`:
- Around line 628-639: The GetSessionRecord assertion block in the CreateSession
RPC test should also verify that the SSH-derived username is persisted. Update
this test around the existing get_session.record checks to assert the record’s
username matches the SSH connection context value, so the CreateSession path
continues to use server-derived identity rather than client-supplied data.
🪄 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: 8454e329-f109-47f7-90c5-191237c1fe5e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/minimal2/src/main.rscrates/minimald-rpc/Cargo.tomlcrates/minimald-rpc/src/lib.rscrates/minimald/src/exec.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/sftp.rscrates/minimald/src/test_harness.rscrates/minvmd/examples/exec.rscrates/minvmd/tests/minimald_session_e2e.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/src/core/compose.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/lib.rs
Member
|
minimal/minimal2 crates were moved, but you can get a clanker to rebase with this prompt: |
evanspearman
force-pushed
the
evan/createsession-wire2
branch
from
July 2, 2026 15:30
4865519 to
fd473df
Compare
Member
Author
|
@twitchyliquid64 Thanks, it's rebased now |
| // wired in. The defensive guard below catches | ||
| // it anyway. | ||
| let composer = SessionComposer::new(contribution); | ||
| match composer.compose(SessionId::nil(), ComposeOptions::default()) { |
Member
There was a problem hiding this comment.
Hmm, thoughts on the contribution being passed through to the Session actor and keeping Sessions mostly as a dumb router? We would need to let the Session handle storage of the composition, implying its separate to the session record itself, but that seems doable (we already let the Session actor manage the dirs for the working tree + session "homedir")
Member
There was a problem hiding this comment.
Oh I guess we manage the state of the composition in the session record.. hmmmm
twitchyliquid64
approved these changes
Jul 3, 2026
evanspearman
force-pushed
the
evan/createsession-wire2
branch
from
July 3, 2026 17:17
fd473df to
f970142
Compare
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
2 participants
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.
Depends on #600
SessionComposer::composenow returns aComposeOutcome::{Ready, Pending}instead of a finalizedComposition, and the manager handler inminimaldconsumes the new shape.ContributionResponseso the client'spolicyandPolicyHooksgate them — the daemon never runs user policy. Daemon-collected packages and lifecycle hooks have no per-item verdict slot in the wire schema, so they pass through into the assembledCompositionon the all-decided path.CreateSessioncaller still sends an emptyWireContribution, so the manager hits the Ready fast path. The Pending branch is guarded with a defensiveInvalidInputerror in the manager until theSubmitVerdicthandler lands (next step).PendingComposeStateretains the daemon-side packages, hooks, and the client's wire contribution across a Pending outcome so Phase 4 can finalize after the client's verdict comes back.crates/sessions/docs/COMPOSITION.mddescribes the new 4-phase flow; tests cover the empty-input fast path, the packages/hooks-only fast path, the var → Pending routing, and that the client's wire contribution merges into the composition byte-for-byte on the all-decided path.Summary by CodeRabbit
Pendingnot reachable yet).