Repository navigation
fix(minimald): refuse at activate a session package that does not resolve - #1935
Conversation
…olve Session finalize promoted a Materializing record to Active without resolving the composition's package names, so an unknown package (from the project or a loadout) surfaced only at the first spawn as `no such package`. Finalize now evaluates the session graph and resolves every composed name with the lookup Env::build uses, refusing with InvalidInput and naming each unknown package and its declarer. Refs gominimal/inbox#816 (packages half)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe change adds a shared package lookup and checks composed package names during session finalization. Unknown names prevent promotion. Context or graph failures and deadline expiry are logged, and the check proceeds without blocking finalization. ChangesPackage Validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Finalization
participant SessionPackageGraph
participant SessionRecord
Finalization->>SessionPackageGraph: Resolve composed package names
SessionPackageGraph-->>Finalization: Return resolution results
alt Unknown package
Finalization-->>SessionRecord: Return InvalidInput and prevent promotion
else Packages resolve or the deadline expires
Finalization->>SessionRecord: Continue finalization
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Session finalization now rejects unknown package names up front instead of failing on the first exec or attach. The check steps aside on timeout or on context errors, so the existing behavior is preserved in those cases. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit checks names in the session’s green field Comment |
|
Approved: no findings at Nothing to do. Clobber: none: |
|
/revise fix review concerns |
|
Round 1 of 1 is on this pull request. branch Updated 2026-10-06T05:03:07.486Z. |
…refuses-unknown-packages
The package check the activation path added evaluates the session graph inside the FinalizeSession round-trip, and that evaluation can clone the upstream repo when the daemon's cache is cold — network I/O with no bound of its own. The only deadline on the round-trip was the client's (minimal-client's 60 s base plus the activate-hook budget), and a client that expires does not cancel the daemon-side finalize: the session actor runs on, promotes the record, and runs the activate hooks on the way, while the client — which has already reported a failed activation and best-effort-destroyed the session — believes nothing was activated. The daemon now bounds the work itself: past PACKAGE_CHECK_DEADLINE (30 s, half the client's base, so patch materialization and a hook's host mint keep the other half) the check logs a warning and steps aside, and finalize proceeds exactly as it did before the check existed — the launch resolves names at first exec, as it always has. Expiry cancels the wait, not the evaluation: the detached spawn_blocking runs to completion and warms the cache the launch reads. The expiry path is provable without a slow graph: the test-only switch now carries the check's deadline, and a zero deadline exercises the step-aside (the check body cannot complete in the single poll a zero deadline gives it, since its graph evaluation round-trips the blocking pool).
…refuses-unknown-packages
|
Round 1 of 1 on a /revise note, pushed as No Review threads the coder judged:
A cold-cache Notes from the agent: The finding was judged valid against the code (Finalize runs inline in the session actor; serve_finalize_session has no server-side deadline; graph_from_all_packages clones the upstream on a cold cache) and fixed by bounding the check rather than all of finalize: patch materialization and the activate-hook host mint are still bounded only by the client's window, exactly as before this PR, and giving the whole finalize a server-side deadline would have changed hook semantics — so the residual window that predates the PR remains, but the PR no longer widens it. The expiry path is testable because the test-only CheckPackagesAtFinalize switch now carries the check's deadline; a zero deadline deterministically exercises expiry (tokio polls the future once before the delay, and the check body cannot complete in that single poll since its graph evaluation round-trips the blocking pool). just e2e-native and the VM lanes could not be completed on this box: no /dev/kvm (just e2e, just test-vm impossible; jq/cpio also absent) and no network for the e2e-native's upstream package fetch — its activate half passed and its exec failure reproduces identically at the branch head without this round's changes. just clippy's 4 hits on this box are toolchain skew (rustc 1.98.1 installed directly, no rustup, repo pins 1.9…[truncated] |
Refs gominimal/inbox#816 (packages half)
Problem
min session activatereturned 0 and a session id for a session that named a package nobody declares, either in[session] packagesor in a loadout'spackages. The failure only appeared on the first exec or attach, assession spawn: no such package: ….Session::finalizepromotes aMaterializingrecord toActiveafter checking only the patches and hook-script markers. Package names were first resolved inEnv::build, at launch.Change
crates/minimald/src/env.rs: theby_namelookup thatEnv::buildruns on each session package is now one helper,session_package.Env::buildbehaves as before.crates/minimald/src/session.rs: in theMaterializingarm offinalize, before the patches are materialized and before any activate hook runs,check_composed_packagesbuilds the session context, evaluates the package graph on the blocking pool (as the launch does), and resolves every composed package name throughsession_package. Any unknown name fails the finalize withInvalidInput, so the record is not promoted. The error lists each unknown name and who declared it, for exampleno such package: no-such-pkg-qq (declared by user loadout `editor`); the session was not activated. The check builds nothing and keeps no graph; the launch still evaluates its own.test/test-supportit defaults off, because those sessions run offline and compose names (the scaffoldedbase/vim) that only an upstream declares. A test-only actor message turns it on.Not in this PR
A bad
locked_commitor an upstream repo that does not exist also activates today and fails on first exec. Refusing those at activate is a follow-up.Note
Finalize now evaluates the graph, and it may clone the upstream on a cold cache. That work used to happen at the first attach. It now falls inside the
FinalizeSessiondeadline (60 s base, plus the activate-hook budget).Tests
New tests in
crates/minimald/src/sessions.rs:finalize_refuses_an_unknown_session_packagefinalize_refuses_an_unknown_loadout_packagefinalize_with_resolvable_packages_still_activatesVerification
just ci(macOS scope):ci: local PR gates greencross clippy -p minimald --all-targets --target aarch64-unknown-linux-musl --locked -- -D warnings: cleancross test -p minimald --lib -- sessions::tests::finalize sessions::tests::needed_packages sessions::tests::create_active:test result: ok. 5 passed; 0 failedcross test -p minimald --lib -- sessions::tests:test result: ok. 35 passed; 0 failedSummary by CodeRabbit