feat(minimald): initial, shitty impl of task execution - #338
Merged
Conversation
📝 WalkthroughWalkthroughSessions and Manager now persist minimal state+cache dirs and can build an mctx::Context on demand; exec handling changed to an Exec -> Stream model, adds a hakoniwa-backed TaskExec/HakoniwaProcess and routing for "min run" commands; CI frees tool cache and expands removed packages. ChangesTask Execution over SSH with Hakoniwa Backend
CI Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
twitchyliquid64
requested review from
0chroma,
evanspearman,
jtnkminimal and
norrietaylor
June 4, 2026 03:24
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/exec.rs`:
- Around line 195-233: The current start_kill() unconditionally sends
libc::kill(self.pid, SIGKILL), which can race with the spawned blocking task
created in wait() (spawn_blocking(child.wait())) and risk killing an unrelated
PID after the child was reaped; change start_kill() to first check whether the
wait has been started (self.wait_handle.is_some()) and, if so, do not send the
raw PID kill (instead return Ok(()) or a benign error), otherwise proceed to
call libc::kill(self.pid, libc::SIGKILL); update start_kill() to reference
wait_handle, pid, and the blocking wait created via spawn_blocking(child.wait())
so we avoid killing by stale PID after wait() has taken ownership of the Child.
- Around line 549-558: The current argv.strip_prefix("min run ") unconditionally
treats the entire suffix as the task name (used to build ExecTask/TaskSpawn)
which wrongly intercepts multi-token commands; instead, parse argv with
split_whitespace and only match the exact three-token form
["min","run","<task>"]. Change the condition around argv.strip_prefix("min run
") to tokenize argv (e.g., argv.split_whitespace().collect::<Vec<_>>()), check
tokens.len() == 3 && tokens[0] == "min" && tokens[1] == "run", and then set
TaskSpawn.task = tokens[2].to_string(); otherwise keep the existing behavior
that falls back to the TokioSpawn shell backend (do not construct
ExecTask/TaskSpawn).
- Around line 114-133: The code calls env.task_invocations(...) and then
unconditionally drops the interactive flag and indexes invs[0], which panics on
zero invocations and mishandles interactive tasks; update the logic that calls
task_invocations to inspect the returned (interactive, invs) tuple: if
interactive is true, return the same rejection as run_task does; if invs.len()
== 0 return an io::Error indicating "no invocations"; if invs.len() > 1 return
the existing error for multiple invocations; only when invs.len() == 1 proceed
to build the container and call env.command(...) using invs[0]; ensure error
messages reference task/container/command construction as currently done.
In `@crates/minimald/src/session.rs`:
- Around line 57-71: The code currently calls
std::fs::create_dir_all(&wsp).unwrap(), which panics on filesystem errors and
kills the session actor; replace that unwrap with error handling that sends the
failure back over the same responder (r) and returns, mirroring the
ConfigBuilder error handling: attempt std::fs::create_dir_all(&wsp), and on
Err(e) send r.send(Err(mctx::Error::from(e).to_string())) (or map the io::Error
into the same error string type used below) and return; keep the existing logic
that builds ConfigBuilder (with_repo_dir/wsp, with_cache_dir, with_state_dir)
untouched otherwise so failures are consistently propagated instead of
panicking.
🪄 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: 66c07433-c01f-46e3-aa27-266ecacb101f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/mctx/src/env.rscrates/minimald/Cargo.tomlcrates/minimald/src/exec.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rs
Comment on lines
+114
to
+133
| let (_interactive, invs) = env | ||
| .task_invocations(&task, self.args.as_ref()) | ||
| .await | ||
| .map_err(|e| std::io::Error::other(e.to_string()))?; | ||
|
|
||
| if invs.len() > 1 { | ||
| return Err(std::io::Error::other( | ||
| "Lol we don't yet implement tasks properly - just simp ones for now", | ||
| )); | ||
| } | ||
|
|
||
| let container = env | ||
| .container() | ||
| .map_err(|e| std::io::Error::other(format!("building container failed: {}", e)))?; | ||
|
|
||
| // TODO: Make this iterate through all the invocations, adapting | ||
| // the trait to do so (stream?). | ||
| let mut cmd = env | ||
| .command(&container, &invs[0].executable, invs[0].args.iter()) | ||
| .map_err(|e| std::io::Error::other(format!("building command failed: {}", e)))?; |
There was a problem hiding this comment.
Validate task_invocations() before using the first entry.
This path drops the interactive flag and Line 132 blindly indexes invs[0]. That gives the wrong behavior for interactive tasks and panics if a task resolves to zero invocations. The existing run_task path in crates/mctx/src/env.rs rejects interactive tasks first; this backend should do the same and handle 0/1/>1 explicitly.
Suggested fix
- let (_interactive, invs) = env
+ let (interactive, invs) = env
.task_invocations(&task, self.args.as_ref())
.await
.map_err(|e| std::io::Error::other(e.to_string()))?;
- if invs.len() > 1 {
- return Err(std::io::Error::other(
- "Lol we don't yet implement tasks properly - just simp ones for now",
- ));
- }
+ if interactive {
+ return Err(std::io::Error::other(
+ "interactive tasks are not supported over SSH exec yet",
+ ));
+ }
+
+ let invocation = match invs.as_slice() {
+ [] => return Err(std::io::Error::other("task resolved to no invocations")),
+ [inv] => inv,
+ _ => {
+ return Err(std::io::Error::other(
+ "multiple invocations are not supported over SSH exec yet",
+ ));
+ }
+ };
let container = env
.container()
.map_err(|e| std::io::Error::other(format!("building container failed: {}", e)))?;
// TODO: Make this iterate through all the invocations, adapting
// the trait to do so (stream?).
let mut cmd = env
- .command(&container, &invs[0].executable, invs[0].args.iter())
+ .command(&container, &invocation.executable, invocation.args.iter())
.map_err(|e| std::io::Error::other(format!("building command failed: {}", e)))?;📝 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
| let (_interactive, invs) = env | |
| .task_invocations(&task, self.args.as_ref()) | |
| .await | |
| .map_err(|e| std::io::Error::other(e.to_string()))?; | |
| if invs.len() > 1 { | |
| return Err(std::io::Error::other( | |
| "Lol we don't yet implement tasks properly - just simp ones for now", | |
| )); | |
| } | |
| let container = env | |
| .container() | |
| .map_err(|e| std::io::Error::other(format!("building container failed: {}", e)))?; | |
| // TODO: Make this iterate through all the invocations, adapting | |
| // the trait to do so (stream?). | |
| let mut cmd = env | |
| .command(&container, &invs[0].executable, invs[0].args.iter()) | |
| .map_err(|e| std::io::Error::other(format!("building command failed: {}", e)))?; | |
| let (interactive, invs) = env | |
| .task_invocations(&task, self.args.as_ref()) | |
| .await | |
| .map_err(|e| std::io::Error::other(e.to_string()))?; | |
| if interactive { | |
| return Err(std::io::Error::other( | |
| "interactive tasks are not supported over SSH exec yet", | |
| )); | |
| } | |
| let invocation = match invs.as_slice() { | |
| [] => return Err(std::io::Error::other("task resolved to no invocations")), | |
| [inv] => inv, | |
| _ => { | |
| return Err(std::io::Error::other( | |
| "multiple invocations are not supported over SSH exec yet", | |
| )); | |
| } | |
| }; | |
| let container = env | |
| .container() | |
| .map_err(|e| std::io::Error::other(format!("building container failed: {}", e)))?; | |
| // TODO: Make this iterate through all the invocations, adapting | |
| // the trait to do so (stream?). | |
| let mut cmd = env | |
| .command(&container, &invocation.executable, invocation.args.iter()) | |
| .map_err(|e| std::io::Error::other(format!("building command failed: {}", 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/exec.rs` around lines 114 - 133, The code calls
env.task_invocations(...) and then unconditionally drops the interactive flag
and indexes invs[0], which panics on zero invocations and mishandles interactive
tasks; update the logic that calls task_invocations to inspect the returned
(interactive, invs) tuple: if interactive is true, return the same rejection as
run_task does; if invs.len() == 0 return an io::Error indicating "no
invocations"; if invs.len() > 1 return the existing error for multiple
invocations; only when invs.len() == 1 proceed to build the container and call
env.command(...) using invs[0]; ensure error messages reference
task/container/command construction as currently done.
Comment on lines
+195
to
+233
| if self.wait_handle.is_none() { | ||
| let mut child = self | ||
| .child | ||
| .take() | ||
| .expect("HakoniwaProcess::wait invariant: child present until wait succeeds"); | ||
| self.wait_handle = Some(tokio::task::spawn_blocking(move || child.wait())); | ||
| } | ||
|
|
||
| // `&mut JoinHandle` is a cancel-safe Future: if the caller drops | ||
| // this `wait` future mid-poll the blocking task keeps running, | ||
| // and a subsequent call re-borrows the same handle and resumes. | ||
| let handle = self.wait_handle.as_mut().expect("set above"); | ||
| let result = handle.await; | ||
| self.wait_handle = None; | ||
|
|
||
| match result { | ||
| Ok(Ok(status)) => { | ||
| self.exit_code = Some(status.exit_code); | ||
| Ok(status.exit_code) | ||
| } | ||
| Ok(Err(e)) => { | ||
| self.wait_failed = true; | ||
| Err(io::Error::other(e)) | ||
| } | ||
| Err(join_err) => { | ||
| self.wait_failed = true; | ||
| Err(io::Error::other(join_err)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn start_kill(&mut self) -> io::Result<()> { | ||
| // The `Child` is owned by the blocking-wait task once `wait` | ||
| // has been called, so we can't go through `Child::kill`. Send | ||
| // SIGKILL directly; ESRCH (process already reaped) is benign. | ||
| // SAFETY: `kill(2)` is async-signal-safe and has no Rust-side | ||
| // invariants beyond the pid being a valid `pid_t`, which we | ||
| // captured from `Child::id()` at construction time. | ||
| unsafe { libc::kill(self.pid, libc::SIGKILL) }; |
There was a problem hiding this comment.
Avoid raw-PID kills after wait() has started.
Once Line 200 moves the child into spawn_blocking(child.wait()), that worker can reap the process before the next select tick. start_kill() still sends SIGKILL to the saved PID unconditionally, so a broken SSH stream can race with PID reuse and kill an unrelated host process.
🤖 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/exec.rs` around lines 195 - 233, The current start_kill()
unconditionally sends libc::kill(self.pid, SIGKILL), which can race with the
spawned blocking task created in wait() (spawn_blocking(child.wait())) and risk
killing an unrelated PID after the child was reaped; change start_kill() to
first check whether the wait has been started (self.wait_handle.is_some()) and,
if so, do not send the raw PID kill (instead return Ok(()) or a benign error),
otherwise proceed to call libc::kill(self.pid, libc::SIGKILL); update
start_kill() to reference wait_handle, pid, and the blocking wait created via
spawn_blocking(child.wait()) so we avoid killing by stale PID after wait() has
taken ownership of the Child.
Comment on lines
+57
to
+71
| let wsp = self.session.workspace_path(); | ||
| std::fs::create_dir_all(&wsp).unwrap(); | ||
| let config = match ConfigBuilder::new() | ||
| .with_repo_dir(wsp.as_utf8_path()) | ||
| .with_cache_dir(self.minimal_cache_dir.as_utf8_path()) | ||
| .with_state_dir(self.minimal_state_dir.as_utf8_path()) | ||
| .build() | ||
| { | ||
| Err(e) => { | ||
| let _ = r.send(Err(mctx::Error::from(e).to_string())); | ||
| return; | ||
| } | ||
| Ok(c) => c, | ||
| }; | ||
| let _ = r.send(mctx::Context::new(config).map_err(|e| e.to_string())); |
There was a problem hiding this comment.
Propagate workspace-init failures instead of panicking the actor.
create_dir_all(&wsp).unwrap() turns a recoverable filesystem error into a session actor crash here. After that, SessionHandle::context() hits its expect("corresponding session is dead") path, so one bad workspace path takes out the whole session.
Suggested fix
SessionMessage::MakeContext(r) => {
let wsp = self.session.workspace_path();
- std::fs::create_dir_all(&wsp).unwrap();
+ if let Err(e) = std::fs::create_dir_all(&wsp) {
+ let _ = r.send(Err(e.to_string()));
+ return;
+ }
let config = match ConfigBuilder::new()
.with_repo_dir(wsp.as_utf8_path())
.with_cache_dir(self.minimal_cache_dir.as_utf8_path())
.with_state_dir(self.minimal_state_dir.as_utf8_path())
.build()🤖 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 57 - 71, The code currently
calls std::fs::create_dir_all(&wsp).unwrap(), which panics on filesystem errors
and kills the session actor; replace that unwrap with error handling that sends
the failure back over the same responder (r) and returns, mirroring the
ConfigBuilder error handling: attempt std::fs::create_dir_all(&wsp), and on
Err(e) send r.send(Err(mctx::Error::from(e).to_string())) (or map the io::Error
into the same error string type used below) and return; keep the existing logic
that builds ConfigBuilder (with_repo_dir/wsp, with_cache_dir, with_state_dir)
untouched otherwise so failures are consistently propagated instead of
panicking.
twitchyliquid64
force-pushed
the
tom/update
branch
from
June 4, 2026 16:21
04b4aee to
9020f96
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
40-41: ⚡ Quick winMake Rust toolchain provisioning explicit after enabling tool-cache cleanup.
With
remove_tool_cache: trueon Line 40, this job depends on howendersonmenezes/free-disk-space@v3treats runner toolcache. Since later steps assumecargo/rustupalready exist, explicitly installing Rust here would make CI deterministic across runner image changes.Suggested hardening
- name: Free Disk Space uses: endersonmenezes/free-disk-space@v3 # Use `@main` for latest, `@v3` for stable with: remove_android: true remove_dotnet: true remove_haskell: true remove_tool_cache: true remove_packages: "azure-cli dotnet-sdk-8.0 temurin-* mysql* firefox" + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@stable - uses: actions/checkout@v6🤖 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 @.github/workflows/ci.yml around lines 40 - 41, Because remove_tool_cache: true can purge the runner toolcache and CI later assumes cargo/rustup exists, add an explicit step to provision the Rust toolchain after the cleanup step: detect the job step that sets remove_tool_cache: true and insert a step that installs rustup/cargo (e.g., via actions-rs/toolchain or rustup-init) and sets the desired toolchain (stable or a pinned version) so subsequent steps relying on cargo/rustup are deterministic; reference the remove_tool_cache: true setting and the remove_packages listing to place the new installation step immediately after the cleanup action.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 40-41: Because remove_tool_cache: true can purge the runner
toolcache and CI later assumes cargo/rustup exists, add an explicit step to
provision the Rust toolchain after the cleanup step: detect the job step that
sets remove_tool_cache: true and insert a step that installs rustup/cargo (e.g.,
via actions-rs/toolchain or rustup-init) and sets the desired toolchain (stable
or a pinned version) so subsequent steps relying on cargo/rustup are
deterministic; reference the remove_tool_cache: true setting and the
remove_packages listing to place the new installation step immediately after the
cleanup action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f019fe6-eeb6-49df-974a-101a0a9bf2b1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/ci.ymlcrates/mctx/src/env.rscrates/minimald/Cargo.tomlcrates/minimald/src/exec.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/minimald/src/server.rs
- crates/minimald/src/sessions.rs
- crates/mctx/src/env.rs
- crates/minimald/src/exec.rs
norrietaylor
reviewed
Jun 4, 2026
Comment on lines
+83
to
+84
| let graph = ctx | ||
| .graph_from_all_packages() |
Member
There was a problem hiding this comment.
Any concerns with calling sync functions like graph_from_all_packages from the this async fn?
norrietaylor
reviewed
Jun 4, 2026
| // SAFETY: `kill(2)` is async-signal-safe and has no Rust-side | ||
| // invariants beyond the pid being a valid `pid_t`, which we | ||
| // captured from `Child::id()` at construction time. | ||
| unsafe { libc::kill(self.pid, libc::SIGKILL) }; |
Member
There was a problem hiding this comment.
Any concerns about pid wrapping? (I guess that is what CR is talking about too)
Member
Author
There was a problem hiding this comment.
Good find! will launch em in a blocking task.
norrietaylor
reviewed
Jun 4, 2026
twitchyliquid64
force-pushed
the
tom/update
branch
from
June 4, 2026 19:26
9020f96 to
51e5c0a
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/session.rs`:
- Around line 91-95: The context() method currently panics on a closed session
actor because recv.await uses expect and send errors are ignored; update
Session::context to propagate errors instead: check the result of
self.0.send(SessionMessage::MakeContext(send)).await and return Err("session
actor is dead".into()) if that send fails, then await recv and match its
Result—on Ok(ctx) return Ok(ctx) and on Err(_) return Err("session actor is
dead".into())—so both send and recv failure paths return Err<String> rather than
panicking (referencing the context() function, SessionMessage::MakeContext,
self.0.send, and recv.await).
🪄 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: 23e5bf44-d0bd-4e55-87ed-5e43b5be0d99
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/ci.ymlcrates/mctx/src/env.rscrates/minimald/Cargo.tomlcrates/minimald/src/exec.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/minimald/src/server.rs
- .github/workflows/ci.yml
- crates/minimald/Cargo.toml
- crates/mctx/src/env.rs
- crates/minimald/src/sessions.rs
Comment on lines
+91
to
+95
| pub async fn context(&self) -> Result<mctx::Context, String> { | ||
| let (send, recv) = oneshot::channel(); | ||
| // Ignore send errors - the recv will also fail. | ||
| let _ = self.0.send(SessionMessage::MakeContext(send)).await; | ||
| recv.await.expect("corresponding session is dead") |
There was a problem hiding this comment.
Return an error when the session actor is gone.
This method advertises Result<_, String>, but a closed actor channel still panics via expect. If the session task dies, callers get an accepted exec that aborts mid-flight instead of a normal error.
Proposed fix
pub async fn context(&self) -> Result<mctx::Context, String> {
let (send, recv) = oneshot::channel();
- // Ignore send errors - the recv will also fail.
- let _ = self.0.send(SessionMessage::MakeContext(send)).await;
- recv.await.expect("corresponding session is dead")
+ self.0
+ .send(SessionMessage::MakeContext(send))
+ .await
+ .map_err(|_| "corresponding session is dead".to_string())?;
+ recv.await
+ .unwrap_or_else(|_| Err("corresponding session is dead".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/session.rs` around lines 91 - 95, The context() method
currently panics on a closed session actor because recv.await uses expect and
send errors are ignored; update Session::context to propagate errors instead:
check the result of self.0.send(SessionMessage::MakeContext(send)).await and
return Err("session actor is dead".into()) if that send fails, then await recv
and match its Result—on Ok(ctx) return Ok(ctx) and on Err(_) return Err("session
actor is dead".into())—so both send and recv failure paths return Err<String>
rather than panicking (referencing the context() function,
SessionMessage::MakeContext, self.0.send, and recv.await).
norrietaylor
approved these changes
Jun 4, 2026
This was referenced Jun 4, 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.
Demo:
That was a LOT of wrestling with all the layers. Hakoniwa isnt async so needed to convert into tokio-flavored wrappers for the pipes, and nickel internals have a bunch of Rc<> that does not play well across await's, etc.
This still need work, namely:
But this is a good stopping/review point.
Summary by CodeRabbit
New Features
Refactor