Merged
Conversation
📝 WalkthroughWalkthroughJobs are now planned into a RunPlanRecord with Prepare and Execute nodes; run flow builds and persists a plan, then executes planned jobs using per-job HostContext. VFKit runner materialization was refactored into dedicated installable-producing helpers and API surface expanded. Changes
Sequence DiagramsequenceDiagram
participant Submit as Job Submitter
participant Builder as Plan Builder
participant Store as Plan Store
participant Executor as Per-Job Executor
participant Host as Host Environment
Submit->>Builder: run_jobs_against_snapshot(jobs)
Builder->>Builder: build_run_plan(jobs)
Builder->>Builder: create PrepareNode (vfkit runner)
Builder->>Builder: create ExecuteNode (VmCommand per job)
Builder->>Store: write_run_plan_record(plan.json)
Store-->>Builder: plan_path
Builder->>Executor: for each PlannedJob (plan_node_id, ctx)
Executor->>Host: run_one_job(plan_node_id, ctx)
Host-->>Executor: execution result
Executor-->>Submit: RunRecord (includes plan_path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
justinmoon
force-pushed
the
pika-ci-merge
branch
from
1e07ca7 to
cbda437
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/pikaci/src/run.rs (1)
436-450: Keep plan construction side-effect free.Line 438 materializes files on disk while just building metadata, and
crates/pikaci/src/executor.rsLine 62 does the same work again during execution. That makes planning mutate job state and couplesbuild_run_plan()to executor internals more tightly than needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pikaci/src/run.rs` around lines 436 - 450, The plan currently calls materialize_vfkit_runner_flake during planning (in the build_run_plan path where job.runner_kind() == RunnerKind::VfkitLocal) which mutates disk state; instead have build_run_plan record only the installable descriptor/flake reference (e.g., a serializable Installable/flake identifier) in the PlanNodeRecord::Prepare / PrepareNode::NixBuild without calling materialize_vfkit_runner_flake, and move the actual materialization call into the executor path (the code around the materialization in crates/pikaci/src/executor.rs) so materialize_vfkit_runner_flake is invoked at execution time; update any types/serde for the PrepareNode::NixBuild payload so it can carry the flake/installable metadata rather than a materialized path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pikaci/src/run.rs`:
- Around line 136-141: The loop over plan.jobs presently propagates any Err from
run_one_job() and exits without persisting a terminal run state; update the loop
to catch errors from run_one_job(&planned_job.job, &planned_job.execute_node_id,
&planned_job.ctx) and, on error, set the run status to a terminal state (e.g.,
Failed), set finished_at to now, and write the updated status.json / run.json to
disk before returning the error; ensure the write/directory creation logic used
elsewhere for persisting terminal status is invoked here (or factored into a
helper) and that write failures are handled/logged or propagated so the run
cannot remain permanently marked as running.
---
Nitpick comments:
In `@crates/pikaci/src/run.rs`:
- Around line 436-450: The plan currently calls materialize_vfkit_runner_flake
during planning (in the build_run_plan path where job.runner_kind() ==
RunnerKind::VfkitLocal) which mutates disk state; instead have build_run_plan
record only the installable descriptor/flake reference (e.g., a serializable
Installable/flake identifier) in the PlanNodeRecord::Prepare /
PrepareNode::NixBuild without calling materialize_vfkit_runner_flake, and move
the actual materialization call into the executor path (the code around the
materialization in crates/pikaci/src/executor.rs) so
materialize_vfkit_runner_flake is invoked at execution time; update any
types/serde for the PrepareNode::NixBuild payload so it can carry the
flake/installable metadata rather than a materialized path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d204ea6-d10a-4dd7-a20c-ce2d16e859bc
📒 Files selected for processing (5)
crates/pikaci/src/executor.rscrates/pikaci/src/lib.rscrates/pikaci/src/main.rscrates/pikaci/src/model.rscrates/pikaci/src/run.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/pikaci/src/main.rs
Comment on lines
+136
to
141
| for planned_job in &plan.jobs { | ||
| let job_record = run_one_job( | ||
| job, | ||
| &snapshot.snapshot_dir, | ||
| &prepared.jobs_dir, | ||
| &prepared.shared_cargo_home_dir, | ||
| &prepared.run_target_dir, | ||
| &planned_job.job, | ||
| &planned_job.execute_node_id, | ||
| &planned_job.ctx, | ||
| )?; |
There was a problem hiding this comment.
Persist a terminal run state when job execution setup fails.
At Line 137, any Err from run_one_job() exits this function with run.json still marked as running, because the terminal status and finished_at are only written later. A failed status.json write or directory creation will leave the run permanently dangling.
💡 Suggested fix
let mut run_failed = false;
for planned_job in &plan.jobs {
- let job_record = run_one_job(
- &planned_job.job,
- &planned_job.execute_node_id,
- &planned_job.ctx,
- )?;
+ let job_record = match run_one_job(
+ &planned_job.job,
+ &planned_job.execute_node_id,
+ &planned_job.ctx,
+ ) {
+ Ok(job_record) => job_record,
+ Err(err) => {
+ run_record.status = RunStatus::Failed;
+ run_record.finished_at = Some(Utc::now().to_rfc3339());
+ run_record.message = Some(format!("{err:#}"));
+ write_run_record(&prepared.run_dir, &run_record)?;
+ return Err(err);
+ }
+ };
if job_record.status == RunStatus::Failed {
run_failed = true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikaci/src/run.rs` around lines 136 - 141, The loop over plan.jobs
presently propagates any Err from run_one_job() and exits without persisting a
terminal run state; update the loop to catch errors from
run_one_job(&planned_job.job, &planned_job.execute_node_id, &planned_job.ctx)
and, on error, set the run status to a terminal state (e.g., Failed), set
finished_at to now, and write the updated status.json / run.json to disk before
returning the error; ensure the write/directory creation logic used elsewhere
for persisting terminal status is invoked here (or factored into a helper) and
that write failures are handled/logged or propagated so the run cannot remain
permanently marked as running.
This was referenced Mar 8, 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
1 participant
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.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests