feat(sessions): wire project + package composables end-to-end into the sandbox - #650
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (13)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds project and package session contributors, threads them through daemon-side session composition, and propagates the resulting ChangesSession composition pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
evanspearman
force-pushed
the
evan/smushittogether
branch
from
July 7, 2026 19:04
ca60cf0 to
8e1e4dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/minimald/src/sessions/composables.rs (1)
118-126: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winComment overstates the safety net for schema-tag drift.
The doc says a
decodeenum-tag rendering change "breaks compilation" — butCREDENTIAL_CLASS_TAGis only ever runtime string-compared (Line 156-159), so a rendering drift wouldn't fail to compile; it would silently stop matching, lettingCredential-class mappings flow into the sandbox unfiltered. The actual protection is thecredential_class_fs_mappings_are_filtered_outtest, which only catches this if it's run/observed. Worth correcting the comment so the real (test-based, not compile-time) guarantee is clear to future maintainers of this credential-handling path.🤖 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/sessions/composables.rs` around lines 118 - 126, The comment for CREDENTIAL_CLASS_TAG overstates the safety guarantee by saying schema-tag drift would “break compilation,” but the value is only checked at runtime in the credential filtering path. Update the documentation near CREDENTIAL_CLASS_TAG in composables.rs to say that changes to decode::AttrValue::EnumTag rendering will cause the runtime string comparison to stop matching, and that the credential_class_fs_mappings_are_filtered_out test is the actual guard against regressions. Keep the note aligned with the behavior in the credential-handling code so future maintainers understand the real protection.
🤖 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/mfile/src/lib.rs`:
- Around line 374-406: Drop the manual Eq implementation for Session in lib.rs.
Session contains extra: HashMap<String, toml::Value>, and toml::Value only
supports PartialEq because Float(f64) can be NaN, so Session cannot validly
promise Eq. Remove the impl Eq for Session block and keep the derived
PartialEq/other derives on Session unless another type or trait bound in the
Session API explicitly requires Eq.
In `@crates/minimald/src/sessions.rs`:
- Around line 463-486: In sessions.rs, the composition is removed from
self.compositions before calling Session::run, so a failed spawn can lose the
stashed packages/vars for the record. Update the GetSession flow around
self.compositions.remove, obj.record().id, and Session::run so the removed
composition is restored back into the map if Session::run returns an error
before propagating the failure. Keep the successful path unchanged, but ensure
the error path reinserts the exact composition associated with that session id.
---
Nitpick comments:
In `@crates/minimald/src/sessions/composables.rs`:
- Around line 118-126: The comment for CREDENTIAL_CLASS_TAG overstates the
safety guarantee by saying schema-tag drift would “break compilation,” but the
value is only checked at runtime in the credential filtering path. Update the
documentation near CREDENTIAL_CLASS_TAG in composables.rs to say that changes to
decode::AttrValue::EnumTag rendering will cause the runtime string comparison to
stop matching, and that the credential_class_fs_mappings_are_filtered_out test
is the actual guard against regressions. Keep the note aligned with the behavior
in the credential-handling code so future maintainers understand the real
protection.
🪄 Autofix (Beta)
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: 26fdbddc-fba2-41cd-84ba-68f8ef7fb122
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
crates/graph/src/lib.rscrates/mctx/src/env.rscrates/mctx/src/lib.rscrates/mctx/src/project_setup.rscrates/mctx/src/scaffold.rscrates/mfile/Cargo.tomlcrates/mfile/src/lib.rscrates/mfile/src/package_composable.rscrates/mfile/src/project_composable.rscrates/minimal/src/main.rscrates/minimald/Cargo.tomlcrates/minimald/minimal-nclcrates/minimald/src/env.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/minimald/src/sessions/composables.rscrates/mip/src/cmd_run.rscrates/sessions/example_project/arch.mmdcrates/sessions/example_project/minimal.tomlcrates/sessions/src/core/compose.rscrates/sessions/src/core/loadout.rs
| /// then destroy) would leak indefinitely, which is why both | ||
| /// paths call `compositions.remove(&id)`. | ||
| /// | ||
| /// Held behind [`Arc`] so the drain-to-actor hop and any |
Member
There was a problem hiding this comment.
Can we do a pass through all the verbose commentary and trim them down to just the essentials?
i.e. this could just talk about how this is the pending composition for a in-construction session, discussion of the point of Arc and unbounded growth in the edge case is probably unnecessary
Member
Author
There was a problem hiding this comment.
Agreed. I'll get Claude to do a pass over all the doc comments in sessions and minimald.
| /// Lazily-built [`mctx::Context`] rooted at this session's | ||
| /// workspace, cached across [`Self::context`] calls so repeated | ||
| /// attach / task-exec paths don't rebuild the daemon setup and | ||
| /// re-parse the workspace mfile. |
Member
There was a problem hiding this comment.
There are cases where we want a fresh Context, like its totally valid for a user to edit their minimal.toml to add some new tasks and then run them - thats why i had it originally as being reconstructed every time someone called for a context via the session handle.
Happy to leave it like this for now, but probably want to circle back to this later.
| armed: true, | ||
| }); | ||
|
|
||
| // Package + env-var union of the launcher baseline and every |
Member
There was a problem hiding this comment.
Would a cleaner way to do this just be to have some "default" loadout/composition that gets unioned in earlier, rather than having a baseline set of packages and stuff that gets combined at the last moment ?
Member
Author
There was a problem hiding this comment.
I like that approach. I think I'll do that in a follow on as part of the loadout loading flow though.
twitchyliquid64
approved these changes
Jul 7, 2026
norrietaylor
added a commit
that referenced
this pull request
Jul 8, 2026
The DM1 re-run (2026-07-08, HEAD 9443bec with the G-N9 fix built in) surfaced two issues and qualified a third: - G-N10: in-sandbox `curl` vanished after #650, which made the session rootfs compose only declared packages; `curl` was only incidental to the old base rootfs. TC1b/TC2 now provision it per-session with `min add curl` (egress 200, peer YES), restoring the egress probes. - G-N11 (new): the G-N9 `:7654`/`:7655` proxy lease-routing returns 502 reproducibly once a prior own-ip session has driven real egress; from a churn-free VM the same TCs are 200. TC4 (direct ingress) is unaffected, so the fault is the proxy lease resolution under prior switch usage. - G-N12 (new): TC7 with-cert fails at the host `curl`/LibreSSL, which rejects the valid P-256 client cert from `minimal login`; the proxy mTLS gate (no-cert 401) works. So G-N9's proxy fix is confirmed on DM1 only in isolation. test-plan.sh wires `min add curl` into TC1b/TC2; test-plan.md records the 4-pass verdicts and adds G-N11/G-N12 to the gap register. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
10 tasks
norrietaylor
added a commit
that referenced
this pull request
Jul 10, 2026
The DM1 re-run (2026-07-08, HEAD 9443bec with the G-N9 fix built in) surfaced two issues and qualified a third: - G-N10: in-sandbox `curl` vanished after #650, which made the session rootfs compose only declared packages; `curl` was only incidental to the old base rootfs. TC1b/TC2 now provision it per-session with `min add curl` (egress 200, peer YES), restoring the egress probes. - G-N11 (new): the G-N9 `:7654`/`:7655` proxy lease-routing returns 502 reproducibly once a prior own-ip session has driven real egress; from a churn-free VM the same TCs are 200. TC4 (direct ingress) is unaffected, so the fault is the proxy lease resolution under prior switch usage. - G-N12 (new): TC7 with-cert fails at the host `curl`/LibreSSL, which rejects the valid P-256 client cert from `minimal login`; the proxy mTLS gate (no-cert 401) works. So G-N9's proxy fix is confirmed on DM1 only in isolation. test-plan.sh wires `min add curl` into TC1b/TC2; test-plan.md records the 4-pass verdicts and adds G-N11/G-N12 to the gap register. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Resolves https://github.com/gominimal/inbox/issues/198
Summary
Wires the daemon-side session composables end-to-end into the sandbox
launcher, so a session activated against a project actually gets that
project's packages, vars, and stack material — not the hardcoded 5-package
literal that previously ran.
Before this change: every session launched with the same static
["base", "bash", "socat", "coreutils", "claude-code"]package set,the mfile was never consulted, and
Compositionwas computed on thedaemon and then thrown away.
After:
CreateSessionbuilds aCompositionfrom all three composables(loadout, project, per-package), stashes it, and
SandboxLauncher::launchunions its packages and vars over a minimal baseline.
Every package included in the session goes through this pipeline:
(from
[session] packages,[stack] build_packages/runtime_packages,and the graph-level Stack's own package lists) form the top-level set.
env_state_wiringorenv_dir/file_mappingscontributes a
PackageComposable; the resulting vars and patches aretagged
Source::Packageand gated through the samePatchPolicy/VarsPolicyany other contribution goes through.EnvArgsat launch.The
Env::buildlegacy path stays intact formip run <task>— taskruns still use
SetupForPackagesas before. Session launches opt intothe composition path via
EnvArgs::without_package_attr_wiring().Notable pieces
mfile::ProjectComposableandmfile::PackageComposable: two newComposableimpls that read the[session]block and packageBuildSpec.attrsrespectively.minimald::sessions::composables: daemon-side pipeline that resolvesthe project mfile + graph, builds composables, and drives the
SessionComposer. Split into its own module so each stage is unit-testable without spinning the manager mainloop.
Env::buildcomposition branch: bypassesSetupForPackagesoutput forenv_vars/fs_mappings; state dirs derive from resolved var valuesshaped
/state/<prefix>instead.mctx::Contextsplit intoContext+Arc<DaemonContext>so per-session mctx state doesn't duplicate the daemon-scoped config, vcs,
and cache setup.
ApproveProjectAndPackageclient hook: defaultminimal activatepolicy that auto-approves
Source::Project/Source::Packagecontributions (project activation implicitly consents to what the
project declares).
crates/sessions/example_project/with ademo-punchline mfile — comment out
[session], re-activate, watchevery demo command break.
Deferred
tracing::warn!pendingthe secrets strategy.
read_only = trueis ignored (with warn); minimalis copy-based so the flag has no meaning today.
Summary by CodeRabbit
minimal.toml’s[session](packages, vars, patches, lifecycle hooks) and session-aware composition.