fix(minimald): refresh session ctx/graph after a patched build - #1182
Conversation
📝 WalkthroughWalkthrough
ChangesManifest refresh and command integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionChannel
participant Context
participant PackageGraph
participant PatchedBuild
participant TaskEnvironment
participant Client
SessionChannel->>Context: reinitialize from current manifest
SessionChannel->>PackageGraph: rebuild from all packages
SessionChannel->>SessionChannel: remap installed package references
SessionChannel->>Client: report refresh errors when needed
SessionChannel->>PackageGraph: resolve package or task
SessionChannel->>PatchedBuild: run with refreshed context and daemon ID
SessionChannel->>TaskEnvironment: create task environment with refreshed context
SessionChannel->>SessionChannel: store refreshed state
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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/env.rs`:
- Around line 1163-1178: Update the BuildSpecRefs remapping logic in the
surrounding refresh flow so entries are retained only when the same-name package
in the new graph has the same specification hash as the old package. When the
hash differs, exclude that package from the collected materialized set while
preserving the existing missing-package error handling.
- Around line 1164-1167: Update install and the refresh logic around
self.has_packages so newly materialized package references are staged locally
and inserted only after cache lookup and hardlinking complete successfully.
Replace the self.graph.get(...).expect("bsrs always exist") path in the
new_has_packages mapping with client-error handling for missing graph entries,
preserving atomic bookkeeping when installation fails.
- Around line 914-916: Keep the refreshed session graph and context unmodified
after refresh instead of storing command-specific graph selections as session
state. In the command dispatch around Context::task and the install/patched-pkg
paths, pass clones of the refreshed context, graph, and remapped package state
to each command. Commit those refreshed values back to session state only after
the command completes successfully, preserving top-level packages for later
rootfs materialization.
- Line 877: Update run_patched_pkg so the create_dir_all call executes inside
tokio::task::spawn_blocking rather than on the session actor. Await the blocking
task and propagate both JoinError and filesystem errors, preserving the existing
output_base behavior without discarding the result.
🪄 Autofix
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: 39e4ccca-3d9f-4d98-ad26-887c5b1219e3
📒 Files selected for processing (1)
crates/minimald/src/env.rs
| let output_base = build_ctx.builds_base_dir(); | ||
| let remote_storage = ctx.remote_storage().await.map_err(err_to_io)?; | ||
| let output_base = ctx.builds_base_dir(); | ||
| let _ = std::fs::create_dir_all(&output_base); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline crates/minimald/src/env.rs
printf '%s\n' '--- target context ---'
sed -n '820,910p' crates/minimald/src/env.rs
printf '%s\n' '--- blocking-work patterns ---'
rg -n -C 3 'spawn_blocking|block_in_place|create_dir_all|std::fs::' crates/minimald crates --glob '*.rs' | head -n 300
printf '%s\n' '--- async context around target ---'
rg -n -C 8 'create_dir_all|async fn|tokio::spawn|spawn\(' crates/minimald/src/env.rs
printf '%s\n' '--- just recipes ---'
just --listRepository: gominimal/minimal
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact target implementation ---'
sed -n '840,925p' crates/minimald/src/env.rs
printf '%s\n' '--- minimald blocking and async filesystem conventions ---'
rg -n -C 5 'spawn_blocking|tokio::fs|std::fs::create_dir_all|std::fs::write|std::fs::read|std::fs::canonicalize' crates/minimald/src --glob '*.rs' | head -n 260
printf '%s\n' '--- PatchedBuild implementation and call sites ---'
rg -n -C 8 'struct PatchedBuild|impl.*PatchedBuild|PatchedBuild|builds_base_dir' crates --glob '*.rs' | head -n 320
printf '%s\n' '--- runtime and dependency configuration ---'
rg -n -C 4 'tokio|test-cross|test' Cargo.toml Cargo.lock justfile Justfile .justfile docs --glob '!target/**' | head -n 260
printf '%s\n' '--- available just executable ---'
command -v just || trueRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
p = Path("crates/minimald/src/env.rs")
s = p.read_text()
needle = "let _ = std::fs::create_dir_all(&output_base);"
pos = s.index(needle)
start = s.rfind("async fn ", 0, pos)
end = s.find("\n }\n", pos)
fn = s[start:end]
print("function:", re.search(r"async fn\\s+\\w+", fn).group(0))
print("contains direct synchronous create_dir_all:", needle in fn)
print("discards create_dir_all result:", "let _ =" in fn and needle in fn)
print("contains await after target:", ".await" in s[pos + len(needle):end])
PYRepository: gominimal/minimal
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact test recipes ---'
sed -n '260,330p' justfile
rg -n -A 12 -B 4 '^test-cross|^test(-[a-z-]+)?:' justfile
printf '%s\n' '--- session actor spawn and handler dispatch ---'
sed -n '510,620p' crates/minimald/src/env.rs
sed -n '1320,1375p' crates/op/src/patched.rs 2>/dev/null || true
sed -n '1,90p' crates/op/src/patched.rs
printf '%s\n' '--- corrected deterministic verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
p = Path("crates/minimald/src/env.rs")
s = p.read_text()
needle = "let _ = std::fs::create_dir_all(&output_base);"
pos = s.index(needle)
async_positions = [m.start() for m in re.finditer(r"\basync\s+fn\b", s)]
containing = max((x for x in async_positions if x < pos), default=-1)
fn_end = s.find("\n }\n", pos)
fn_text = s[containing:fn_end]
print("target line:", s[:pos].count("\n") + 1)
print("enclosing declaration:", re.search(r"async\s+fn\s+\w+", fn_text).group(0))
print("direct synchronous filesystem call:", needle in fn_text)
print("result discarded:", "let _ =" in fn_text and needle in fn_text)
print("subsequent await in same async block:", ".await" in s[pos + len(needle):fn_end])
assert "async fn run_patched_pkg" in fn_text
assert needle in fn_text
assert "let _ =" in fn_text
PYRepository: gominimal/minimal
Length of output: 18886
Move create_dir_all off the session actor.
run_patched_pkg runs on the async session actor. Wrap this filesystem call in tokio::task::spawn_blocking, propagate both task and filesystem errors, and do not discard the result. Run just test on Linux or just test-cross on macOS.
🤖 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/env.rs` at line 877, Update run_patched_pkg so the
create_dir_all call executes inside tokio::task::spawn_blocking rather than on
the session actor. Await the blocking task and propagate both JoinError and
filesystem errors, preserving the existing output_base behavior without
discarding the result.
Sources: Coding guidelines, Learnings
| self.ctx = ctx; | ||
| self.graph = graph; | ||
| self.has_packages = new_has_packages; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the refreshed session graph separate from command-specific graph selection.
Line 872 reduces graph.top_levels to the patched package, then Lines 914-916 save that graph as session state. install treats top-level packages as already installed, but patched-pkg only writes the package to cache. A later add-* request for that package can skip rootfs materialization.
run_task also consumes the refreshed graph through Context::task, which changes its top levels to task packages, but then discards all refreshed state. Keep an unmodified refreshed session snapshot. Pass a clone to each command. Commit the refreshed context, graph, and remapped package state only after successful command completion.
Also applies to: 921-935
🤖 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/env.rs` around lines 914 - 916, Keep the refreshed
session graph and context unmodified after refresh instead of storing
command-specific graph selections as session state. In the command dispatch
around Context::task and the install/patched-pkg paths, pass clones of the
refreshed context, graph, and remapped package state to each command. Commit
those refreshed values back to session state only after the command completes
successfully, preserving top-level packages for later rootfs materialization.
| // Recompute BuildSpecRefs for the new graph. | ||
| let new_has_packages = self | ||
| .has_packages | ||
| .iter() | ||
| .map(|bsr| &self.graph.get(bsr).expect("bsrs always exist").name) | ||
| .map(|name| match graph.by_name(name) { | ||
| Some(bsr) => Ok(*bsr), | ||
| None => Err(name.clone()), | ||
| }) | ||
| .collect::<Result<HashSet<BuildSpecRef>, String>>() | ||
| .map_err(|e| { | ||
| Self::write_error( | ||
| &Error::Graph(Box::new(graph::Error::NoSuchPkg { name: e })), | ||
| stream, | ||
| ); | ||
| })?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not remap materialized packages by name alone.
A package can keep its name while its build specification changes after minimal.toml is refreshed. This code then maps the old rootfs contents to the new BuildSpecRef and marks the new package as materialized. Later installation can skip the updated package and use stale files.
Retain a remapped entry only when the old and new package specifications have the same spec hash. Treat a same-name package with a changed hash as not materialized.
🤖 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/env.rs` around lines 1163 - 1178, Update the
BuildSpecRefs remapping logic in the surrounding refresh flow so entries are
retained only when the same-name package in the new graph has the same
specification hash as the old package. When the hash differs, exclude that
package from the collected materialized set while preserving the existing
missing-package error handling.
| let new_has_packages = self | ||
| .has_packages | ||
| .iter() | ||
| .map(|bsr| &self.graph.get(bsr).expect("bsrs always exist").name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the panic path and make installation bookkeeping atomic.
install inserts into self.has_packages before cache lookup and hardlinking complete. If either operation fails, self.graph remains unchanged while has_packages contains a reference that the old graph does not contain. Line 1167 then panics on the next refresh.
Stage newly materialized references locally. Add them to self.has_packages only after cache resolution and hardlinking succeed. Return a client error for any inconsistent legacy state instead of calling expect.
🤖 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/env.rs` around lines 1164 - 1167, Update install and the
refresh logic around self.has_packages so newly materialized package references
are staged locally and inserted only after cache lookup and hardlinking complete
successfully. Replace the self.graph.get(...).expect("bsrs always exist") path
in the new_has_packages mapping with client-error handling for missing graph
entries, preserving atomic bookkeeping when installation fails.
bbb8bec to
5e8b509
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
5e8b509 to
e3af0d9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
Testing
Checklist
BREAKING CHANGE:footer present if this is a breaking changeNote
Refresh session context and graph after a patched build in
minimaldrefreshhelper onSessionChannelthat reinitializes theContext, rebuilds theGraphfrom all packages, and resyncshas_packagesvia name-based lookup, writing client-readable errors on failure.run_patched_pkgnow callsrefreshbefore execution, resolves the package in the refreshed graph, and updatesself.ctx,self.graph, andself.has_packagesafter a successful build.run_tasknow callsrefreshbefore task lookup and execution, using the refreshed context and graph throughout.patched-pkgandrun <task>re-readminimal.tomland rebuild the graph on every invocation;patched-pkgwill now error to the client immediately if the package name is missing from the refreshed graph.Macroscope summarized e3af0d9.
Summary by CodeRabbit