fix: fix patched build clobbering session top levels - #1185
Conversation
|
|
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/env.rs`:
- Around line 1208-1212: Update remap_refs to preserve an installed package only
when the existing and refreshed BuildSpecRef values have matching spec hashes,
not merely matching names. When the same package name resolves to a changed
definition, invalidate the session entry or return the established refresh
conflict so stale rootfs contents cannot be treated as installed. Add a
regression test covering a same-name package with a changed definition.
🪄 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: 0d69fcc5-e470-491b-91bd-286ad6768996
📒 Files selected for processing (1)
crates/minimald/src/env.rs
| bsrs.into_iter() | ||
| .map(|bsr| { | ||
| let name = &from.get(bsr).expect("bsrs always exist").name; | ||
| to.by_name(name).copied().ok_or_else(|| name.clone()) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve installed-package identity across refreshes.
remap_refs resolves installed packages only by name. If minimal.toml changes a package with the same name, the session maps the old rootfs contents to the new BuildSpecRef. A later install then reports Already installed for content that was never injected.
Only preserve an installed entry when its old and new spec hashes match. Otherwise, invalidate the session entry and require a rebuild or return a refresh conflict. Add a regression test with the same package name and a changed package definition.
Also applies to: 1687-1716
🤖 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 1208 - 1212, Update remap_refs to
preserve an installed package only when the existing and refreshed BuildSpecRef
values have matching spec hashes, not merely matching names. When the same
package name resolves to a changed definition, invalidate the session entry or
return the established refresh conflict so stale rootfs contents cannot be
treated as installed. Add a regression test covering a same-name package with a
changed definition.
Summary
In a session,
min package patched-build <pkg>followed bymin add <pkg>silentlydoes nothing: the package is never injected into the live rootfs, and the command
exits 0 with no output.
SessionChannelusesself.graph.top_levelsas its record of what it has injected —install()early-returns on it.run_patched_pkgnarrowedtop_levelsto the singlepackage being built and then committed that graph as the session graph, so the next
min add <pkg>took the already-installed path.install()'s early return writesnothing to the wire, which is why it reads as success. The
add_depstail still runs,so
minimal.tomlgets the dependency while the rootfs does not.run_patched_pkgscopes the build to a throwawaybuild_graphclone; the sessiongraph keeps its own
top_levels.refresh()re-resolves the session'stop_levelsonto the reparsed graph by namerather than adopting the fresh parse's (which are the packages the repo declares,
not the ones the session has). Same remap it already did for
has_packages, nowfactored into a
remap_refshelper.install()'s already-installed path reportsAlready installed: <pkgs>instead ofreturning silently — on the wire it was indistinguishable from a real injection.
Testing
Besides automated tests I built a new package not merged yet and installed it successfully in the same session.
Checklist
BREAKING CHANGE:footer present if this is a breaking changeSummary by CodeRabbit
New Features
min installnow clearly reports when all requested packages are already installed.Bug Fixes
Note
Fix patched builds and session refresh clobbering installed top-level packages
run_patched_pkgnow builds against a temporary cloned graph instead of mutating the session graph'stop_levels, preventing patched builds from narrowing the installed set.refreshnow remaps the session's installed packages andhas_packagesonto the newly parsed graph by name, rather than adopting the repo's declared top-levels.remap_refshelper resolves refs from one graph against another by package name, returning an error if a package is missing.installnow sends amsg:Already installed: <pkg>line to the client when all requested packages are already present, instead of returning silently.Macroscope summarized 9d1dca3.