refactor: move all session logic into session actor, init loadouts in own RPC - #754
Merged
Conversation
|
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 (12)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe session lifecycle now separates session allocation from loadout configuration. Session actors own Draft/Active transitions, RPC routing delegates lifecycle operations to actors, activation scaffolding is non-blocking for non-interactive input, and execution/SFTP paths handle actor disappearance explicitly. ChangesSession activation and lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
twitchyliquid64
force-pushed
the
tom/session-inner
branch
5 times, most recently
from
July 15, 2026 03:07
4deadd2 to
ba25074
Compare
twitchyliquid64
marked this pull request as ready for review
July 15, 2026 03:10
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/sessions/composables.rs (1)
215-249: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winClient-facing error now leaks the daemon's internal workspace path instead of the client's project path.
project_pathhere is now aDaemonAbsPath(the session's daemon-side workspace), but theInvalidInputerror text at Line 232 still reads as if it's describing the client's own project directory:"projectminimal.tomlat {project_path} is invalid: {e}". This error propagates to the client viaConfigureLoadout'sErrorable::Err, so the client will see the daemon's internal session-workspace path rather than anything they recognize (their own upload's root), which is both confusing and exposes internal daemon file-layout details.🩹 Reference something client-meaningful instead of the daemon path
Err(e) => { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, - format!("project `minimal.toml` at {project_path} is invalid: {e}"), + format!("project `minimal.toml` in the session workspace is invalid: {e}"), )); }🤖 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/sessions/composables.rs` around lines 215 - 249, Update the InvalidInput error construction in resolve_project_ctx_and_graph so it does not interpolate the daemon-side project_path into the client-facing message. Use a client-meaningful description of the project or minimal.toml location while preserving the existing error propagation and underlying error details.
🤖 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/session.rs`:
- Around line 535-594: Update configure_loadout to reject sessions in
SessionInner::Draft with pending: Some(_) in addition to Active, using the same
AlreadyExists error path, so an outstanding composition cannot be overwritten.
Leave reconfiguration permitted for drafts without pending resume state.
In `@crates/minimald/src/sessions/composables.rs`:
- Around line 404-409: Update the ProjectComposable construction in the
effective composable path to preserve the daemon workspace path’s provenance
instead of converting it with paths::Host and new_unchecked. Use the appropriate
daemon/workspace path type or existing conversion expected by
ProjectComposable::new, while leaving the effective composable handling
unchanged.
In `@crates/minvmd/examples/exec.rs`:
- Around line 189-231: Update the ConfigureLoadout response handling block to
deserialize into <ConfigureLoadout as OneshotSshRpc>::Response, which is
Errorable<ConfigureLoadoutResponse>, instead of ConfigureLoadoutResponse
directly. Call .ok() and propagate the daemon-side error message before matching
the Ready/Pending ConfigureLoadoutResponse variants, following the existing test
helper behavior.
---
Outside diff comments:
In `@crates/minimald/src/sessions/composables.rs`:
- Around line 215-249: Update the InvalidInput error construction in
resolve_project_ctx_and_graph so it does not interpolate the daemon-side
project_path into the client-facing message. Use a client-meaningful description
of the project or minimal.toml location while preserving the existing error
propagation and underlying error details.
🪄 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: fd4dd052-8c5d-4e41-a2a2-2c61630496ef
📒 Files selected for processing (12)
crates/minimal/src/lib.rscrates/minimal/tests/cli.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/exec.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/sessions/composables.rscrates/minimald/src/sftp.rscrates/minimald/src/test_harness.rscrates/minvmd/examples/exec.rscrates/minvmd/tests/minimald_session_integration.rs
Comment on lines
+535
to
+594
| async fn configure_loadout( | ||
| &mut self, | ||
| contribution: WireContribution, | ||
| ) -> Result<Option<ContributionResponse>, std::io::Error> { | ||
| if matches!(self.inner, SessionInner::Active { .. }) { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::AlreadyExists, | ||
| "session loadout is already configured", | ||
| )); | ||
| } | ||
| let object = self.record.object().await?; | ||
| let workspace_path = object.workspace_path(); | ||
|
|
||
| // Deliberately no scaffold here: composing is not the moment to | ||
| // fabricate a project. `scaffold_default_mfile` resolves the default | ||
| // package repo's branch head over the network, and the default's | ||
| // packages reach the sandbox through the launcher's context (built | ||
| // from the workspace mfile) rather than through the composition — | ||
| // so paying for it here would buy nothing. A bare workspace composes | ||
| // to an empty loadout, and the scaffold lands at context-build time. | ||
|
|
||
| // Phase 1+2: resolve the project and drive the composer. Kept fully | ||
| // synchronous — its non-`Send` intermediaries must not cross an | ||
| // `.await`. | ||
| let outcome = composables::run_compose(&self.daemon_ctx, &workspace_path, contribution)?; | ||
|
|
||
| match outcome { | ||
| // The composition is complete: promote the record | ||
| // `Pending → Active` and hold the composition for the launcher. | ||
| ComposeOutcome::Ready(composition) => { | ||
| let mut record = object.record().clone(); | ||
| record.status = SessionStatus::Active; | ||
| self.record.write(record.clone()).await?; | ||
| self.inner = SessionInner::Active { | ||
| composition: Some(Arc::new(composition)), | ||
| host: None, | ||
| context: None, | ||
| }; | ||
| // The session is Active now — publish its PTask route | ||
| // (R3.1/R3.6). | ||
| #[cfg(target_os = "linux")] | ||
| self.register_hostname(&record); | ||
| Ok(None) | ||
| } | ||
| // The client must gate items before the composition completes. | ||
| // Park in `Draft` with the daemon-side resume state; the record | ||
| // is already `Pending` on disk, so nothing to write. | ||
| ComposeOutcome::Pending { | ||
| mut response, | ||
| state, | ||
| } => { | ||
| // The composer ran before the allocated id was known to it. | ||
| response.session_id = *self.record.id(); | ||
| self.inner = SessionInner::Draft { | ||
| pending: Some(Box::new(state)), | ||
| }; | ||
| Ok(Some(response)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any existing coverage of a duplicate ConfigureLoadout call
# against a session already parked Draft{pending:Some}.
rg -n -A5 'fn configure_loadout' crates/minimald/src/sessions.rs crates/minimald/src/session.rs
rg -n 'configure_loadout' crates/minimald/src/sessions.rs | rg -i 'pending|twice|again|duplicate'Repository: gominimal/minimal
Length of output: 2200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the session state machine and read the relevant implementations/tests.
ast-grep outline crates/minimald/src/session.rs --view expanded | sed -n '1,220p'
printf '\n--- configure_loadout and related state ---\n'
sed -n '500,700p' crates/minimald/src/session.rs
printf '\n--- submit verdict / pending-state handling ---\n'
rg -n -A40 -B20 'SubmitVerdict|submit_verdict|pending: Some|pending:None|Draft \{' crates/minimald/src/session.rs crates/minimald/src/sessions.rs
printf '\n--- tests mentioning Pending / verdict / reconfigure ---\n'
rg -n -A3 -B3 'Pending|verdict|configure_loadout|Draft' crates/minimald/src/sessions.rs | sed -n '1,240p'Repository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- attach / loadout call sites ---'
rg -n -A20 -B20 'ConfigureLoadout|SessionPending|LoadoutFailed|configure_loadout\(' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,260p'
printf '\n%s\n' '--- helpers / pending-session tests ---'
rg -n -A25 -B10 'create_pending_session|create_and_configure|Pending session|awaiting a verdict|resumable|reconfigure' crates/minimald/src/sessions.rs | sed -n '1,260p'
printf '\n%s\n' '--- docs/comments about Draft/Pending semantics ---'
rg -n -A3 -B3 'ready for another attempt|awaiting a verdict|can correct it and re-submit|already configured' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,220p'Repository: gominimal/minimal
Length of output: 34734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the attach path and any places that can call configure_loadout implicitly.
sed -n '695,910p' crates/minimald/src/session.rs
printf '\n--- callers of configure_loadout on handle/session ---\n'
rg -n -A3 -B3 'configure_loadout\(' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,220p'
printf '\n--- any guard for busy/pending state in manager APIs ---\n'
rg -n -A20 -B10 'SessionPending|is_busy|busy|pending: Some|AlreadyExists' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,220p'Repository: gominimal/minimal
Length of output: 31949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the attach implementation and any state transitions around loadout configuration.
sed -n '695,910p' crates/minimald/src/session.rs
printf '\n--- direct callers of configure_loadout ---\n'
rg -n -A4 -B4 'configure_loadout\(' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,220p'
printf '\n--- busy/pending semantics ---\n'
rg -n -A20 -B8 'SessionPending|is_busy|pending: Some|AlreadyExists|awaiting a verdict' crates/minimald/src/session.rs crates/minimald/src/sessions.rs | sed -n '1,240p'Repository: gominimal/minimal
Length of output: 34121
Reject reconfiguration while a verdict is outstanding
configure_loadout() only blocks Active, so a Draft { pending: Some(_) } session can be recomposed and overwrite the pending resume state. That invalidates the existing Pending response/item ids and makes a later SubmitVerdict hit the wrong state. Guard Draft { pending: Some(_) } too, or document that reconfiguring intentionally restarts the flow.
🤖 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/session.rs` around lines 535 - 594, Update
configure_loadout to reject sessions in SessionInner::Draft with pending:
Some(_) in addition to Active, using the same AlreadyExists error path, so an
outstanding composition cannot be overwritten. Leave reconfiguration permitted
for drafts without pending resume state.
Comment on lines
404
to
409
| (!effective.is_empty()).then(|| { | ||
| mfile::ProjectComposable::new(paths::HostPath::from(project_path.clone()), effective) | ||
| mfile::ProjectComposable::new( | ||
| paths::AbsPath::<paths::Host>::new_unchecked(project_path.as_utf8_path()).into(), | ||
| effective, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ProjectComposable's definition and how its path field is consumed/surfaced.
ast-grep outline crates/mfile --items all --type struct,impl --match 'ProjectComposable'
rg -nP -C4 '\bstruct\s+ProjectComposable\b' crates/mfile/src
rg -nP -C4 'impl\s+ProjectComposable\b' crates/mfile/src
rg -nP 'ProjectComposable::new' cratesRepository: gominimal/minimal
Length of output: 1875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' crates/mfile/src/project_composable.rs
printf '\n----\n'
sed -n '360,450p' crates/minimald/src/sessions/composables.rs
printf '\n----\n'
rg -n 'project_path|HostPath|Source::Project|composition provenance|log|serde|Display|Debug' crates/mfile/src/project_composable.rs crates/mfile/src -g '!target'Repository: gominimal/minimal
Length of output: 21526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Source type and whether provenance is serialized/surfaced.
sed -n '1,240p' crates/sessions/src/core/source.rs
printf '\n----\n'
sed -n '1,260p' crates/sessions/src/core/compose.rs
printf '\n----\n'
rg -n 'Source::Project|Source::Package|Provenanced|serde::Serialize|Serialize for Contribution|Debug for Contribution|Display for Source|path:' crates/sessions/src crates/mfile/src crates/minimald/srcRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether composition errors and provenance are exposed in client-facing paths.
rg -n --max-count 40 'Conflict::|ComposeError|Error::Conflict|error.*source|source\)|Display for Error|thiserror::Error|to_string\(\)|format!\(".*\{source\}' crates/sessions/src crates/minimald/src crates/mfile/src
printf '\n----\n'
sed -n '1,220p' crates/sessions/src/core/compose.rs | sed -n '1,120p'
printf '\n----\n'
sed -n '300,430p' crates/minimald/src/session_host.rs
printf '\n----\n'
sed -n '1,220p' crates/minimald/src/rpc.rsRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect path type aliases/newtypes and the conversion APIs around Host/Daemon paths.
rg -n --max-count 80 'type HostPath|HostPath|DaemonAbsPath|AbsPath<paths::Host>|new_unchecked|as_utf8_path|into\(\)' crates -g '!target'
printf '\n----\n'
sed -n '1,220p' crates/paths/src/lib.rs
printf '\n----\n'
sed -n '1,220p' crates/paths/src/abs.rsRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the path-branding types and the wire-facing source type.
rg -n --max-count 80 'struct HostPath|struct HostAbsPath|struct DaemonAbsPath|new_unchecked|as_utf8_path|type HostPath|type DaemonAbsPath' crates/paths crates -g '!target'
printf '\n----\n'
sed -n '1,260p' crates/paths/src/lib.rs
printf '\n----\n'
sed -n '1,260p' crates/sessions/src/wire/primitives.rsRepository: gominimal/minimal
Length of output: 30634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,235p' crates/minimald/src/sessions/composables.rs
printf '\n----\n'
sed -n '500,560p' crates/sessions/src/core/compose.rs
printf '\n----\n'
sed -n '620,710p' crates/sessions/src/wire/errors.rs
printf '\n----\n'
sed -n '240,320p' crates/sessions/src/client/handler.rsRepository: gominimal/minimal
Length of output: 8591
Don't cast the daemon workspace path into host provenance. ProjectComposable stores this in Source::Project, which is sent to the client and rendered in compose errors, so new_unchecked can expose the daemon workspace as if it were the client's host path.
🤖 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/sessions/composables.rs` around lines 404 - 409, Update
the ProjectComposable construction in the effective composable path to preserve
the daemon workspace path’s provenance instead of converting it with paths::Host
and new_unchecked. Use the appropriate daemon/workspace path type or existing
conversion expected by ProjectComposable::new, while leaving the effective
composable handling unchanged.
norrietaylor
approved these changes
Jul 15, 2026
twitchyliquid64
force-pushed
the
tom/session-inner
branch
from
July 15, 2026 03:36
ba25074 to
78d0e04
Compare
twitchyliquid64
enabled auto-merge (squash)
July 15, 2026 03:41
This was referenced Jul 17, 2026
This was referenced Jul 24, 2026
This was referenced Jul 31, 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
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.
ConfigureLoadout, which is fired between the session creation and the submit verdict.Summary by CodeRabbit
New Features
Bug Fixes