feat(sessions): SubmitVerdict handler resumes pending sessions - #618
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a pending-session flow: daemon-composed items can be returned as ChangesPending session composition and verdict submission
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as minimal CLI
participant Daemon as minimald RPC
participant Manager as SessionsManager
participant Composer as SessionComposer
CLI->>Daemon: CreateSession(contribution)
Daemon->>Manager: CreateSession
Manager->>Composer: compose(session_id, options)
alt daemon vars/patches empty
Composer-->>Manager: ComposeOutcome::Ready(Composition)
Manager-->>Daemon: CreateSessionResponse::Ready { id }
Daemon-->>CLI: Ready { id }
else needs approval
Composer-->>Manager: ComposeOutcome::Pending(response, state)
Manager->>Manager: stash PendingComposeState, persist Pending record
Manager-->>Daemon: CreateSessionResponse::Pending { id, response }
Daemon-->>CLI: Pending { id, response }
CLI->>CLI: handle_response + UserPolicy + AbortOnUnapproved
CLI->>Daemon: SubmitVerdict(verdict)
Daemon->>Manager: SubmitVerdict
Manager->>Composer: resume_from_verdict(state, verdict)
Composer-->>Manager: Composition
Manager->>Manager: promote Pending -> Active
Manager-->>Daemon: SessionStep::Active { id }
Daemon-->>CLI: Active { id }
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
evanspearman
force-pushed
the
evan/createsession-wire3
branch
from
July 2, 2026 15:56
00f444d to
7bf3d2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/minimal/src/main.rs`:
- Around line 480-489: The local gating path in main around handle_response
currently returns before SubmitVerdict, which can leave a
CreateSessionResponse::Pending record and pending-stash slot behind when
AbortOnUnapproved aborts. Update the verdict handling so this branch explicitly
cleans up the daemon pending session—either by invoking an abort/cleanup path or
by submitting an explicit denial that causes the manager to remove the pending
state—using the existing handle_response, AbortOnUnapproved, and SubmitVerdict
flow as the place to wire it in.
In `@crates/minimald-rpc/src/lib.rs`:
- Around line 232-236: The status note in the documentation comment is outdated
and still implies `Pending` is unavailable and `SubmitVerdict` has not landed.
Update the comment near the composer flow to describe the actual `Pending ->
SubmitVerdict -> Active` path, keeping the explanation aligned with the current
behavior of the manager/composer logic and removing any language that suggests
the transition is still missing.
In `@crates/minimald/src/sessions.rs`:
- Around line 168-175: The destroy path in `Manager`/`sessions.rs` removes the
session record but leaves the matching `pending` stash entry behind, so
destroyed pending sessions still count against `MAX_PENDING_SESSIONS`. Update
the session destroy handler to also remove the in-memory entry from `pending`
using the session id (for example, alongside the existing delete logic in the
destroy method that handles `SessionId`), so `PendingComposeState` is cleaned up
whenever a session is destroyed.
- Around line 529-579: The resume/ready flow in `resume_from_verdict` handling
should not mark a session `Active` while the resumed composition is still being
dropped. Add the same non-empty composition guard used by the Ready path before
calling `store.save` and returning `SessionStep::Active`, and if `_composition`
contains approved vars/patches/packages/hooks, return a fault instead of
promoting the record. This keeps `SessionStep::Active` and the `Pending ->
Active` transition from silently losing the resumed composition.
- Around line 513-579: The pending session stash is being removed too early in
the SubmitVerdict flow, before promotion to Active is durably saved. In the
sessions::SessionStore resume path around `self.pending.remove`,
`resume_from_verdict`, and `store.save`, keep the pending entry until after
`store.save` succeeds so a failed promotion can still be retried. Adjust the
logic to borrow or clone the pending state for `resume_from_verdict`, delete
`self.pending` only after the on-disk status update is committed, and retain the
immediate removal only for terminal resume failures that already return a Fault.
In `@crates/sessions/docs/COMPOSITION.md`:
- Around line 47-55: The status note in the COMPOSITION docs is stale and still
says `ComposeOutcome::Pending` is rejected and `SubmitVerdict` is missing;
update the paragraph around `SessionComposer::compose` and
`CreateSessionResponse::Ready` so it reflects the current `Pending ->
SubmitVerdict -> Active` lifecycle and removes the outdated rejection wording.
In `@crates/sessions/src/daemon/composer.rs`:
- Around line 351-358: The approval handling in Composer::pending_vars /
WireVarVerdict::Approved currently trusts the client-returned value and can
install a different variable name than the one originally routed by PendingId.
When processing the Approved branch, compare the name in value against the
stashed pending var from take_pending and reject or ignore any mismatch so only
the original pending var name is accepted; use the existing pending-var lookup
path and SessionVar::new construction to keep the routed name authoritative.
🪄 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: 65fc5ca1-b8a9-4ba9-b482-7d1314c4f030
📒 Files selected for processing (12)
crates/minimal/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/sessions.rscrates/minvmd/tests/minimald_session_e2e.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/src/core/compose.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/lib.rscrates/sessions/src/store.rscrates/sessions/src/wire/errors.rscrates/sessions/src/wire/request.rs
Comment on lines
+168
to
+175
| /// Per-session stash for in-flight `CreateSession` flows that | ||
| /// reached [`ComposeOutcome::Pending`]. Keyed by the daemon's | ||
| /// allocated [`SessionId`]; the matching `SubmitVerdict` pops | ||
| /// the entry and finalizes. The stash is in-memory only — if | ||
| /// the daemon restarts mid-flow the on-disk | ||
| /// `SessionStatus::Pending` record loses its match and is | ||
| /// reaped by [`Manager::reap_orphan_pending`] at startup. | ||
| pending: BTreeMap<SessionId, PendingComposeState>, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear pending stash entries when sessions are destroyed.
The new pending map is keyed by session id, but the existing destroy path deletes the record without removing the matching stash entry. Destroyed Pending sessions can keep consuming MAX_PENDING_SESSIONS capacity until daemon restart.
Add self.pending.remove(&id) in the destroy handler when deleting a session id.
🤖 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.rs` around lines 168 - 175, The destroy path in
`Manager`/`sessions.rs` removes the session record but leaves the matching
`pending` stash entry behind, so destroyed pending sessions still count against
`MAX_PENDING_SESSIONS`. Update the session destroy handler to also remove the
in-memory entry from `pending` using the session id (for example, alongside the
existing delete logic in the destroy method that handles `SessionId`), so
`PendingComposeState` is cleaned up whenever a session is destroyed.
Comment on lines
+513
to
+579
| let stash_hit = self.pending.remove(&session_id); | ||
| let store = &mut self.store; | ||
| responder | ||
| .handle(async move { | ||
| let state = match stash_hit { | ||
| Some(s) => s, | ||
| None => { | ||
| return Ok(SessionStep::Fault { | ||
| error: sessions::wire::errors::WireError::UnknownSessionId, | ||
| }); | ||
| } | ||
| }; | ||
| // Run Phase 4 resume. A verdict that the | ||
| // composer can't apply (mismatched | ||
| // PendingIds, denied items, etc.) maps to a | ||
| // `Fault` via `WireError::from(ComposeError)`. | ||
| // The resumed composition is dropped today: | ||
| // session activation doesn't yet consume it. | ||
| let _composition = match resume_from_verdict(state, verdict) { | ||
| Ok(c) => c, | ||
| Err(e) => { | ||
| // Verdict couldn't be applied — the | ||
| // session is dead. Destroy the | ||
| // matching on-disk Pending record so | ||
| // the name is freed, `list()` doesn't | ||
| // show a phantom entry, and the next | ||
| // reap pass has nothing to clean up. | ||
| // A delete failure here is logged but | ||
| // doesn't override the verdict-level | ||
| // error returned to the client. | ||
| if let Some(k) = store.find_by_id(&session_id)? | ||
| && let Err(del_err) = store.delete(&k) | ||
| { | ||
| tracing::warn!( | ||
| session_id = %session_id, | ||
| error = %del_err, | ||
| "failed to delete Pending record after \ | ||
| resume failure; record will be reaped on \ | ||
| next daemon restart", | ||
| ); | ||
| } | ||
| return Ok(SessionStep::Fault { error: e.into() }); | ||
| } | ||
| }; | ||
| // R2.1: reject a policy incompatible with the network | ||
| // mode (e.g. egress on a non-`OwnIp` PTask) at | ||
| // declaration time, so an invalid session is never | ||
| // written to the store rather than only failing when | ||
| // a client later attaches. | ||
| record.validate_policy().map_err(|e| { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidInput, e) | ||
| })?; | ||
| let k = self.store.create(record)?; | ||
| Ok(minimald_rpc::CreateSessionResponse::Ready { id: *k.id() }) | ||
| // Promote the on-disk record `Pending → | ||
| // Active`. A mid-flight delete or a status | ||
| // already past `Pending` is degenerate but | ||
| // surfaces as a structured `WrongState`. | ||
| let k = match store.find_by_id(&session_id)? { | ||
| Some(k) => k, | ||
| None => { | ||
| return Ok(SessionStep::Fault { | ||
| error: sessions::wire::errors::WireError::UnknownSessionId, | ||
| }); | ||
| } | ||
| }; | ||
| let mut record = store.get(&k)?.record().clone(); | ||
| if record.status != sessions::SessionStatus::Pending { | ||
| return Ok(SessionStep::Fault { | ||
| error: sessions::wire::errors::WireError::WrongState { | ||
| what: format!("expected Pending, found {:?}", record.status,), | ||
| }, | ||
| }); | ||
| } | ||
| record.status = sessions::SessionStatus::Active; | ||
| store.save(&k, &record)?; | ||
| Ok(SessionStep::Active { id: session_id }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the pending stash until promotion is durable.
Line 513 removes the stash before store.save succeeds. If promotion fails, the on-disk record remains Pending but the in-memory state needed to retry SubmitVerdict is gone.
Prefer cloning/borrowing the state for resume_from_verdict, then remove self.pending only after store.save succeeds; remove it immediately only for terminal resume failures.
🤖 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.rs` around lines 513 - 579, The pending session
stash is being removed too early in the SubmitVerdict flow, before promotion to
Active is durably saved. In the sessions::SessionStore resume path around
`self.pending.remove`, `resume_from_verdict`, and `store.save`, keep the pending
entry until after `store.save` succeeds so a failed promotion can still be
retried. Adjust the logic to borrow or clone the pending state for
`resume_from_verdict`, delete `self.pending` only after the on-disk status
update is committed, and retain the immediate removal only for terminal resume
failures that already return a Fault.
Comment on lines
+529
to
+579
| // The resumed composition is dropped today: | ||
| // session activation doesn't yet consume it. | ||
| let _composition = match resume_from_verdict(state, verdict) { | ||
| Ok(c) => c, | ||
| Err(e) => { | ||
| // Verdict couldn't be applied — the | ||
| // session is dead. Destroy the | ||
| // matching on-disk Pending record so | ||
| // the name is freed, `list()` doesn't | ||
| // show a phantom entry, and the next | ||
| // reap pass has nothing to clean up. | ||
| // A delete failure here is logged but | ||
| // doesn't override the verdict-level | ||
| // error returned to the client. | ||
| if let Some(k) = store.find_by_id(&session_id)? | ||
| && let Err(del_err) = store.delete(&k) | ||
| { | ||
| tracing::warn!( | ||
| session_id = %session_id, | ||
| error = %del_err, | ||
| "failed to delete Pending record after \ | ||
| resume failure; record will be reaped on \ | ||
| next daemon restart", | ||
| ); | ||
| } | ||
| return Ok(SessionStep::Fault { error: e.into() }); | ||
| } | ||
| }; | ||
| // R2.1: reject a policy incompatible with the network | ||
| // mode (e.g. egress on a non-`OwnIp` PTask) at | ||
| // declaration time, so an invalid session is never | ||
| // written to the store rather than only failing when | ||
| // a client later attaches. | ||
| record.validate_policy().map_err(|e| { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidInput, e) | ||
| })?; | ||
| let k = self.store.create(record)?; | ||
| Ok(minimald_rpc::CreateSessionResponse::Ready { id: *k.id() }) | ||
| // Promote the on-disk record `Pending → | ||
| // Active`. A mid-flight delete or a status | ||
| // already past `Pending` is degenerate but | ||
| // surfaces as a structured `WrongState`. | ||
| let k = match store.find_by_id(&session_id)? { | ||
| Some(k) => k, | ||
| None => { | ||
| return Ok(SessionStep::Fault { | ||
| error: sessions::wire::errors::WireError::UnknownSessionId, | ||
| }); | ||
| } | ||
| }; | ||
| let mut record = store.get(&k)?.record().clone(); | ||
| if record.status != sessions::SessionStatus::Pending { | ||
| return Ok(SessionStep::Fault { | ||
| error: sessions::wire::errors::WireError::WrongState { | ||
| what: format!("expected Pending, found {:?}", record.status,), | ||
| }, | ||
| }); | ||
| } | ||
| record.status = sessions::SessionStatus::Active; | ||
| store.save(&k, &record)?; | ||
| Ok(SessionStep::Active { id: session_id }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not promote while dropping the resumed composition.
The Ready path rejects non-empty compositions, but SubmitVerdict drops _composition and still marks the record Active. Any approved pending vars/patches/packages/hooks are silently lost.
Add the same non-empty guard here until the apply layer can persist/consume the resumed composition.
🤖 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.rs` around lines 529 - 579, The resume/ready
flow in `resume_from_verdict` handling should not mark a session `Active` while
the resumed composition is still being dropped. Add the same non-empty
composition guard used by the Ready path before calling `store.save` and
returning `SessionStep::Active`, and if `_composition` contains approved
vars/patches/packages/hooks, return a fault instead of promoting the record.
This keeps `SessionStep::Active` and the `Pending -> Active` transition from
silently losing the resumed composition.
Comment on lines
+351
to
+358
| WireVarVerdict::Approved { id, value } => { | ||
| let pv = take_pending(&mut pending_vars, id, "var")?; | ||
| // The verdict's `value` is the client-resolved | ||
| // (possibly user-edited at prompt) value; the | ||
| // stashed `pv` supplies the source the wire schema | ||
| // doesn't echo. | ||
| let (_, source) = pv.into_parts(); | ||
| accepted.push(SessionVar::new(value.into(), source)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate approved var names against the stash.
A verdict with a valid PendingId can currently change the variable name in value, causing Phase 4 to install a different var than the daemon originally routed to Pending.
Proposed fix
WireVarVerdict::Approved { id, value } => {
let pv = take_pending(&mut pending_vars, id, "var")?;
+ let expected_name = pv.var().name();
+ if value.name != expected_name {
+ return Err(ComposeError::InvalidWireItem {
+ what: "approved var name does not match pending var",
+ context: format!(
+ "pending id {id:?}: expected `{expected_name}`, got `{}`",
+ value.name,
+ ),
+ });
+ }
// The verdict's `value` is the client-resolved
// (possibly user-edited at prompt) value; the
// stashed `pv` supplies the source the wire schema📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| WireVarVerdict::Approved { id, value } => { | |
| let pv = take_pending(&mut pending_vars, id, "var")?; | |
| // The verdict's `value` is the client-resolved | |
| // (possibly user-edited at prompt) value; the | |
| // stashed `pv` supplies the source the wire schema | |
| // doesn't echo. | |
| let (_, source) = pv.into_parts(); | |
| accepted.push(SessionVar::new(value.into(), source)); | |
| WireVarVerdict::Approved { id, value } => { | |
| let pv = take_pending(&mut pending_vars, id, "var")?; | |
| let expected_name = pv.var().name(); | |
| if value.name != expected_name { | |
| return Err(ComposeError::InvalidWireItem { | |
| what: "approved var name does not match pending var", | |
| context: format!( | |
| "pending id {id:?}: expected `{expected_name}`, got `{}`", | |
| value.name, | |
| ), | |
| }); | |
| } | |
| // The verdict's `value` is the client-resolved | |
| // (possibly user-edited at prompt) value; the | |
| // stashed `pv` supplies the source the wire schema | |
| // doesn't echo. | |
| let (_, source) = pv.into_parts(); | |
| accepted.push(SessionVar::new(value.into(), source)); |
🤖 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/daemon/composer.rs` around lines 351 - 358, The approval
handling in Composer::pending_vars / WireVarVerdict::Approved currently trusts
the client-returned value and can install a different variable name than the one
originally routed by PendingId. When processing the Approved branch, compare the
name in value against the stashed pending var from take_pending and reject or
ignore any mismatch so only the original pending var name is accepted; use the
existing pending-var lookup path and SessionVar::new construction to keep the
routed name authoritative.
evanspearman
force-pushed
the
evan/createsession-wire3
branch
from
July 3, 2026 18:39
7bf3d2c to
3342964
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/sessions/docs/COMPOSITION.md (1)
44-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "Pending outcomes are still rejected" status note persists.
This paragraph still states
Pendingoutcomes are rejected withInvalidInputand that theSubmitVerdicthandler "hasn't landed yet." This was already flagged in a prior review round, and it remains unresolved: this very diff addsresume_from_verdict, theSubmitVerdictRPC dispatch, and the full Pending→Active flow described just a few lines below (Phase 4, step 5: "Replies withSessionStep::Active { id }"). The status note now directly contradicts the rest of the document it introduces.📝 Proposed fix
-> `client::handler::handle_response`. Phase 2 (daemon-side routing) -> lives in `SessionComposer::compose` and produces a -> `ComposeOutcome::Ready` or `ComposeOutcome::Pending`. Phase 4 is -> partially wired — the `Ready` outcome is consumed (the manager -> persists the session and returns `CreateSessionResponse::Ready`), -> but `Pending` outcomes are still rejected with `InvalidInput` -> because the `SubmitVerdict` handler hasn't landed yet. No daemon- -> side project/package contributors are wired in either, so every -> caller still hits the all-decided path today. +> `client::handler::handle_response`. Phase 2 (daemon-side routing) +> lives in `SessionComposer::compose` and produces a +> `ComposeOutcome::Ready` or `ComposeOutcome::Pending`. Both +> outcomes are now wired end-to-end: `Ready` persists the session +> as `Active` directly, and `Pending` is resumed via the +> `SubmitVerdict` RPC and `resume_from_verdict` (see Phase 4). No +> daemon-side project/package contributors are wired in either, so +> every caller still hits the all-decided path today.🤖 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/docs/COMPOSITION.md` around lines 44 - 56, Update the status note in COMPOSITION.md so it no longer says `Pending` outcomes are rejected with `InvalidInput` or that `SubmitVerdict` “hasn't landed yet”; this text is now stale. In the section describing `core::compose`, `SessionComposer::compose`, and `ComposeOutcome::Pending`, revise the note to reflect the implemented Pending→Active flow and the new `resume_from_verdict` / `SubmitVerdict` path, keeping it consistent with the Phase 4 description below.
🧹 Nitpick comments (1)
crates/sessions/src/core/compose.rs (1)
982-1039: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate conflict-check logic between
extend_from_wireandextend_with.Both methods run the identical
check_var_mismatches/check_patch_mismatchespair againstself.{vars,patches}.iter().chain(incoming.iter()). Extracting a shared private helper avoids the two call sites drifting apart if the conflict rules ever change.♻️ Proposed refactor
+ fn check_conflicts( + &self, + incoming_vars: &[SessionVar], + incoming_patches: &[SessionPatch], + ) -> Result<(), ComposeError> { + check_var_mismatches( + self.vars.iter().chain(incoming_vars.iter()), + |v| v.var().name(), + |v| v.var().value(), + )?; + check_patch_mismatches( + self.patches.iter().chain(incoming_patches.iter()), + |p| p.patch().destination(), + |p| p.patch().host_path().as_str(), + )?; + Ok(()) + } + pub(crate) fn extend_from_wire(...) -> Result<(), ComposeError> { ... - check_var_mismatches( - self.vars.iter().chain(incoming_vars.iter()), - |v| v.var().name(), - |v| v.var().value(), - )?; - check_patch_mismatches( - self.patches.iter().chain(incoming_patches.iter()), - |p| p.patch().destination(), - |p| p.patch().host_path().as_str(), - )?; + self.check_conflicts(&incoming_vars, &incoming_patches)?; ... } pub(crate) fn extend_with( &mut self, vars: Vec<SessionVar>, patches: Vec<SessionPatch>, ) -> Result<(), ComposeError> { - check_var_mismatches( - self.vars.iter().chain(vars.iter()), - |v| v.var().name(), - |v| v.var().value(), - )?; - check_patch_mismatches( - self.patches.iter().chain(patches.iter()), - |p| p.patch().destination(), - |p| p.patch().host_path().as_str(), - )?; + self.check_conflicts(&vars, &patches)?; self.vars.extend(vars); self.patches.extend(patches); Ok(()) }🤖 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/core/compose.rs` around lines 982 - 1039, `extend_with` duplicates the same conflict-check sequence used by `extend_from_wire`, so factor the shared `check_var_mismatches` and `check_patch_mismatches` logic into a private helper in `compose.rs`. Have both `extend_with` and `extend_from_wire` call that helper with their existing iterators/accessors, keeping the atomic precheck behavior and existing `ComposeError::Conflict` semantics unchanged.
🤖 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/docs/COMPOSITION.md`:
- Around line 44-56: Update the status note in COMPOSITION.md so it no longer
says `Pending` outcomes are rejected with `InvalidInput` or that `SubmitVerdict`
“hasn't landed yet”; this text is now stale. In the section describing
`core::compose`, `SessionComposer::compose`, and `ComposeOutcome::Pending`,
revise the note to reflect the implemented Pending→Active flow and the new
`resume_from_verdict` / `SubmitVerdict` path, keeping it consistent with the
Phase 4 description below.
---
Nitpick comments:
In `@crates/sessions/src/core/compose.rs`:
- Around line 982-1039: `extend_with` duplicates the same conflict-check
sequence used by `extend_from_wire`, so factor the shared `check_var_mismatches`
and `check_patch_mismatches` logic into a private helper in `compose.rs`. Have
both `extend_with` and `extend_from_wire` call that helper with their existing
iterators/accessors, keeping the atomic precheck behavior and existing
`ComposeError::Conflict` semantics unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddd368e9-e76c-4f39-bdcb-7f4ab8b2bb10
📒 Files selected for processing (10)
crates/minimal/src/main.rscrates/minimald/src/rpc.rscrates/minimald/src/sessions.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/src/core/compose.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/lib.rscrates/sessions/src/store.rscrates/sessions/src/wire/errors.rscrates/sessions/src/wire/request.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/sessions/src/wire/errors.rs
- crates/minimald/src/rpc.rs
- crates/sessions/src/lib.rs
- crates/minimal/src/main.rs
- crates/sessions/src/store.rs
- crates/sessions/src/wire/request.rs
- crates/sessions/src/daemon/composer.rs
- crates/minimald/src/sessions.rs
twitchyliquid64
approved these changes
Jul 3, 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.
Depends on #602
Wires up the
SubmitVerdictRPC end-to-end so aPendingsession can be resumed after the client returns its per-item verdict. Manager pops the stashedPendingComposeState, runsresume_from_verdict, promotes the recordPending → Active, replies withSessionStep::Active { id }.sessions:resume_from_verdict(walks verdict → reattaches provenance → assemblesComposition);WireError::WrongState,SessionStep::Activewire additions;Draft → Pendingstatus rename (serde alias keeps old records loading).minimald:Manager.pendingin-memory stash (capMAX_PENDING_SESSIONS,ResourceBusyon overflow);SubmitVerdicthandler;CreateSessionPending branch actually returns Pending now;GetSessionrefuses non-Active; startup reap of orphanPendingrecords; failed resume destroys the on-disk record.minimal:cmd_activatedrives Phase 3 (handle_response) →SubmitVerdict→ unwrapSessionStep::Activeinstead of erroring on Pending. Deny-allPolicyHooksstub until interactive prompts land.minimald-rpc:SubmitVerdictRPC dispatch.Stash is in-memory only — restart-survives-pending is out of scope; the reap pass is the correctness floor. Ready-path composition is still dropped (apply-layer plumbing separate work); guard on non-empty
Compositionprevents silent data loss.Summary by CodeRabbit
New Features
Bug Fixes
Documentation