Skip to content

fix(minimald): refresh session ctx/graph after a patched build - #1182

Merged
twitchyliquid64 merged 1 commit into
mainfrom
tom/sftp
Aug 7, 2026
Merged

fix(minimald): refresh session ctx/graph after a patched build#1182
twitchyliquid64 merged 1 commit into
mainfrom
tom/sftp

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

Testing

Checklist

  • Docs updated if behavior changed
  • BREAKING CHANGE: footer present if this is a breaking change

Note

Refresh session context and graph after a patched build in minimald

  • Adds a refresh helper on SessionChannel that reinitializes the Context, rebuilds the Graph from all packages, and resyncs has_packages via name-based lookup, writing client-readable errors on failure.
  • run_patched_pkg now calls refresh before execution, resolves the package in the refreshed graph, and updates self.ctx, self.graph, and self.has_packages after a successful build.
  • run_task now calls refresh before task lookup and execution, using the refreshed context and graph throughout.
  • Behavioral Change: both patched-pkg and run <task> re-read minimal.toml and rebuild the graph on every invocation; patched-pkg will now error to the client immediately if the package name is missing from the refreshed graph.

Macroscope summarized e3af0d9.

Summary by CodeRabbit

  • Bug Fixes
    • Commands now use the latest project manifest and complete package state before execution.
    • Updated package references are handled correctly when running patched packages.
    • Commands stop safely and report an error if the project state cannot be refreshed.
    • Improved reliability when project dependencies or package information change between commands.
    • Refreshed project context is consistently applied to builds and task environments.

@twitchyliquid64
twitchyliquid64 requested a review from a team as a code owner August 6, 2026 20:14
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

SessionChannel refreshes manifest-derived state before patched-package and task commands. It rebuilds the context and package graph, remaps installed-package references, reports refresh failures, and stores refreshed state after command execution.

Changes

Manifest refresh and command integration

Layer / File(s) Summary
Refresh manifest-derived session state
crates/minimald/src/env.rs
SessionChannel::refresh rebuilds context and package graph state, remaps installed-package references, and reports refresh or missing-package errors.
Use refreshed state for commands
crates/minimald/src/env.rs
run_patched_pkg and run_task resolve and execute using refreshed state. Patched builds use the refreshed daemon ID, and completed builds store refreshed state on the channel.

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
Loading

Poem

I’m a rabbit with a freshly mapped trail,
Context and packages aligned without fail.
Patched builds hop, task runners go,
Stale references now know where to flow.
Refresh, then run—away we grow!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes the required headings but provides no author-written summary, testing evidence, or completed checklist items. Add a concise Summary, document the tests and results, and complete the Checklist, including the breaking-change footer when applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title uses the required Conventional Commit format and clearly describes the session context and graph refresh after patched builds.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tom/sftp

Comment @coderabbitai help to get the list of available commands.

@twitchyliquid64
twitchyliquid64 enabled auto-merge (rebase) August 6, 2026 20:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between f5a9a98 and bbb8bec.

📒 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 --list

Repository: 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 || true

Repository: 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])
PY

Repository: 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
PY

Repository: 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

Comment on lines +914 to +916
self.ctx = ctx;
self.graph = graph;
self.has_packages = new_has_packages;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines +1163 to +1178
// 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,
);
})?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines +1164 to +1167
let new_has_packages = self
.has_packages
.iter()
.map(|bsr| &self.graph.get(bsr).expect("bsrs always exist").name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

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.

@twitchyliquid64
twitchyliquid64 merged commit e7ee243 into main Aug 7, 2026
30 checks passed
@twitchyliquid64
twitchyliquid64 deleted the tom/sftp branch August 7, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants