feat(minimald): implement check session side-op - #1000
Merged
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds cancellation-aware checker execution and introduces Check side operation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionHandle
participant SessionSideOp
participant Checker
Client->>SessionHandle: Start min check with arguments
SessionHandle->>SessionSideOp: Start check operation
SessionSideOp->>Checker: Run checks with cancellation
Checker-->>SessionSideOp: Stream checked results
SessionSideOp-->>Client: Render updates and terminal outcome
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/minimald/src/sessions.rs (1)
1607-1647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider bounding the drain loop with a timeout.
If the terminal update is ever lost the loop blocks forever and CI hangs instead of failing. Wrapping the drain in
tokio::time::timeoutturns a regression into a fast, readable failure.🤖 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 1607 - 1647, Bound the update-draining loop in start_check_closes_with_exactly_one_terminal_outcome with a tokio::time::timeout. Preserve the existing collection and assertion of CheckOutcome values, but make expiration fail the test with a clear timeout message instead of allowing CI to hang when the terminal update is missing.crates/minimald/src/session.rs (1)
1015-1029: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sopsnever sheds completed side-ops.Every
start_check/start_buildpushes ontoSessionInner::Active::sopsand nothing removes finished entries untilstop_running, so a long-lived session that runsmin checkrepeatedly accumulatesSideOps (each holding aJoinHandleplus the op'sArc<Mutex<Inner<_>>>). Pruning finished ops on push would bound it.🤖 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 1015 - 1029, Update the side-operation registration used by start_check (and the corresponding start_build path) so completed entries in Active::sops are pruned before pushing a newly spawned SideOp. Retain only running operations, then append the new operation while preserving existing shutdown and stop_running behavior.crates/check/src/lib.rs (1)
1415-1424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests share a fixed temp dir and leak it on failure.
make_projectderives the path from pid+suffix only, so a re-run in the same process (or a panic beforeremove_dir_all) leaves a populated dir behind that the next run silently reuses.tempfile::TempDirwould give per-run isolation and drop-time cleanup, matching the rest of the workspace's tests.🤖 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/check/src/lib.rs` around lines 1415 - 1424, Update the test helper make_project to create and return a tempfile::TempDir-backed project instead of a pid-and-suffix PathBuf, preserving the existing packages, profiles, and stacks layout while relying on TempDir for unique per-run isolation and drop-time cleanup. Adjust callers to use the TempDir path and retain the directory handle for the test lifetime.crates/check/src/outputs.rs (1)
163-167: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Error::Cancelledis flattened toError::Otheron this path.The blocking closure's error is stringified at Line 369 and re-wrapped at Line 373, so a cancelled run here reports
Other("cancelled"). It's currently masked becausecheck_packagecallsctx.bail_if_cancelled()before consuming this checker's result, but the variant is lost if that ordering ever changes. Returning the cancellation out-of-band (e.g. a bool/Option<Error>alongside the string) would keep the classification.Also applies to: 206-208, 278-279
🤖 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/check/src/outputs.rs` around lines 163 - 167, Preserve Error::Cancelled when errors from the blocking checker closures are converted to strings and re-wrapped, rather than flattening cancellation into Error::Other. Update the spawn_blocking error-handling paths at the shown locations, including the analogous sections around the other reported ranges, to return cancellation metadata out-of-band (such as an optional cancellation error) and reconstruct the original variant at the caller.
🤖 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_sop.rs`:
- Around line 172-201: Update SopCheckArgs::from_args to reject any unrecognized
token beginning with "--" instead of adding it to filter_names. Preserve
existing handling for recognized flags and ordinary object-name filters, and
make the invalid-flag failure visible through the established argument-error
mechanism.
- Around line 460-504: Cancel the run token whenever the drain loop in the
outcome handling exits early with CheckOutcome::Failed or
CheckOutcome::Cancelled, before dropping the stream or reporting Finished. Reuse
the existing cancel flag/token associated with CheckCtx and ensure normal
CheckOutcome::Completed remains unchanged, so detached StandaloneTestCheck work
observes cancellation during terminal error paths.
---
Nitpick comments:
In `@crates/check/src/lib.rs`:
- Around line 1415-1424: Update the test helper make_project to create and
return a tempfile::TempDir-backed project instead of a pid-and-suffix PathBuf,
preserving the existing packages, profiles, and stacks layout while relying on
TempDir for unique per-run isolation and drop-time cleanup. Adjust callers to
use the TempDir path and retain the directory handle for the test lifetime.
In `@crates/check/src/outputs.rs`:
- Around line 163-167: Preserve Error::Cancelled when errors from the blocking
checker closures are converted to strings and re-wrapped, rather than flattening
cancellation into Error::Other. Update the spawn_blocking error-handling paths
at the shown locations, including the analogous sections around the other
reported ranges, to return cancellation metadata out-of-band (such as an
optional cancellation error) and reconstruct the original variant at the caller.
In `@crates/minimald/src/session.rs`:
- Around line 1015-1029: Update the side-operation registration used by
start_check (and the corresponding start_build path) so completed entries in
Active::sops are pruned before pushing a newly spawned SideOp. Retain only
running operations, then append the new operation while preserving existing
shutdown and stop_running behavior.
In `@crates/minimald/src/sessions.rs`:
- Around line 1607-1647: Bound the update-draining loop in
start_check_closes_with_exactly_one_terminal_outcome with a
tokio::time::timeout. Preserve the existing collection and assertion of
CheckOutcome values, but make expiration fail the test with a clear timeout
message instead of allowing CI to hang when the terminal update is missing.
🪄 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: a5dfae44-fd0f-4368-88f5-7a45bd05f6d4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdcrates/check/Cargo.tomlcrates/check/src/lib.rscrates/check/src/outputs.rscrates/mctx/src/error.rscrates/mctx/src/lib.rscrates/minimald/src/env.rscrates/minimald/src/exec.rscrates/minimald/src/session.rscrates/minimald/src/session_sop.rscrates/minimald/src/sessions.rscrates/op/src/standalone_test.rs
bryan-minimal
approved these changes
Jul 28, 2026
bryan-minimal
left a comment
Member
There was a problem hiding this comment.
LGTM, one issue Claude found,
Unknown --flags silently become name filters → a typo'd flag exits 0 (medium — the one I'd fix pre-merge). CheckOpts::from_args routes any unrecognized token into filter_names. So min check --packags (typo) filters for a name that matches
nothing → zero objects checked → Completed{failed: false} → exit 0. On the SSH path, whose entire contract is the exit status for scripted use (their own test says so), a typo'd flag is a silent pass. Suggest rejecting unknown --* tokens (bare
words stay filters). Inherited from the old inline parser, but the old parser didn't feed CI scripts.
twitchyliquid64
force-pushed
the
tom/check-sop
branch
from
July 28, 2026 20:07
1666b4c to
a8ad007
Compare
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/sessions.rs`:
- Around line 1631-1638: Update the loop consuming updates in the test around
updates.recv().await to apply a timeout to each receive or the overall wait,
causing the test to fail promptly when the check side-op stalls or the sender
remains open. Preserve the existing CheckUpdate::Checked panic and Finished
outcome handling.
🪄 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: 3cf21887-127f-40e7-8a1d-3acf222f5858
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdcrates/check/Cargo.tomlcrates/check/src/lib.rscrates/check/src/outputs.rscrates/mctx/src/error.rscrates/mctx/src/lib.rscrates/minimald/src/env.rscrates/minimald/src/exec.rscrates/minimald/src/session.rscrates/minimald/src/session_sop.rscrates/minimald/src/sessions.rscrates/op/src/standalone_test.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/op/src/standalone_test.rs
- crates/check/Cargo.toml
- crates/mctx/src/error.rs
- crates/mctx/src/lib.rs
- crates/minimald/src/env.rs
- crates/minimald/src/session.rs
- crates/check/src/outputs.rs
- crates/check/src/lib.rs
- crates/minimald/src/session_sop.rs
twitchyliquid64
force-pushed
the
tom/check-sop
branch
from
July 28, 2026 20:12
a8ad007 to
6b2fbd6
Compare
twitchyliquid64
enabled auto-merge (rebase)
July 28, 2026 20:13
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/check/src/lib.rs`:
- Around line 526-536: Update the profile and stack checking flows around the
permit acquisition and the `profile::check_profile`/`check_stack` calls so
cancellation is checked between each parse, typecheck, compile, and checker
stage after work begins. When `ctx.cancel` is triggered, stop before the next
stage and return `Error::Cancelled`, while preserving the existing stage order
and results for non-cancelled checks.
🪄 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: e7fba3e0-35ff-4a37-82f3-61f1b4b748ad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdcrates/check/Cargo.tomlcrates/check/src/lib.rscrates/check/src/outputs.rscrates/mctx/src/error.rscrates/mctx/src/lib.rscrates/minimald/src/env.rscrates/minimald/src/exec.rscrates/minimald/src/session.rscrates/minimald/src/session_sop.rscrates/minimald/src/sessions.rscrates/op/src/standalone_test.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/check/Cargo.toml
- crates/mctx/src/error.rs
- AGENTS.md
- crates/minimald/src/env.rs
- crates/op/src/standalone_test.rs
- crates/mctx/src/lib.rs
- crates/minimald/src/session.rs
- crates/minimald/src/exec.rs
- crates/check/src/outputs.rs
- crates/minimald/src/session_sop.rs
- crates/minimald/src/sessions.rs
Comment on lines
+526
to
+536
| let name = pd.to_str().unwrap().to_string(); | ||
| let _permit = tokio::select! { | ||
| biased; | ||
| _ = ctx.cancel.cancelled() => { | ||
| return (CheckObj::Profile(name), Err(Error::Cancelled)); | ||
| } | ||
| permit = ctx.semaphore.acquire() => permit.unwrap(), | ||
| }; | ||
| ( | ||
| CheckObj::Profile(pd.to_str().unwrap().to_string()), | ||
| profile::check_profile(pd.to_str().unwrap().to_string(), &ctx, profiles_dir) | ||
| .await, | ||
| CheckObj::Profile(name.clone()), | ||
| profile::check_profile(name, &ctx, profiles_dir).await, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Observe cancellation after profile/stack checks start.
These select! blocks only cancel queued work. After acquiring a permit, check_profile and check_stack run their full parse/typecheck/compile and checker sequences without observing ctx.cancel, so cancellation cannot prevent remaining stages. Add cancellation checks between those stages (and return Error::Cancelled).
Also applies to: 581-591
🤖 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/check/src/lib.rs` around lines 526 - 536, Update the profile and stack
checking flows around the permit acquisition and the
`profile::check_profile`/`check_stack` calls so cancellation is checked between
each parse, typecheck, compile, and checker stage after work begins. When
`ctx.cancel` is triggered, stop before the next stage and return
`Error::Cancelled`, while preserving the existing stage order and results for
non-cancelled checks.
This was referenced Jul 29, 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.
Implements
checksession side op, workable via:min check <args>in the sessionmin check <args>over ssh execSummary by CodeRabbit
min checksupport end-to-end (SSH exec and daemon/session streaming), including incremental output and a single final success/failure summary.min attach -cdocumentation to reflect the acceptedmin checkandmin package buildforms.min check, rendering/summarization, and cancellation behavior.Note
Implement
min checkas a session side-op inminimaldSideOp::spawn_checkinsession_sop.rsto run checks as background side-ops with cooperative cancellation, streaming per-objectCheckUpdateresults and a terminalCheckOutcometo subscribers.CheckOptsargument parsing formin check, rejecting unknown flags with an error rather than misinterpreting them as name filters.min checkinto the SSH exec handler (exec.rs) and the env socket handler (env.rs), streaming progress and exiting with status 0 (success), 1 (failure), or 130 (cancelled).CancellationTokentoCheckCtxin thecheckcrate so individual checkers, semaphore waits, and sandboxed standalone tests all stop promptly when cancelled.SessionHandle::start_checksends aStartCheckmessage to the session actor, which tracks the run and cancels it on shutdown.Macroscope summarized 6b2fbd6.