Repository navigation
fix: order package-derived sandbox setup collections for determinism - #1319
Merged
norrietaylor merged 1 commit intoAug 29, 2026
Merged
norrietaylor merged 1 commit into
norrietaylor merged 1 commit into
Conversation
Package-derived sandbox setup was carried in HashMap/HashSet collections, so the same package declarations could produce a different sandbox setup — filesystem mapping order, env vars, and /state subdirectory creation — from one run to the next, and EnvPatches (which is Serialize) churned on re-serialization. None of this reaches a cache key, so it is a reproducibility issue rather than a correctness one. Switch the collections keyed by path or variable name to BTreeMap/BTreeSet, so iteration and serialization follow key order: - mfile::EnvPatches dir/file - graph::SetupForPackages env_vars, state_dirs, fs_mapping_packages, and the build() locals feeding them - decode::patches_from_term patch tables - minimald::env::state_dirs_from_env_vars and the Env::build locals - mctx::env::declared_by and the declarations lookup - the minimald session parity test The type change does not compile crate by crate, so all five crates move together. EnvPatches::to_fs_mappings drops its explicit sort_by_key and takes order straight off the now-sorted maps. Two tests pin the property: identical patches built in opposite insertion orders lower to the same mapping sequence and serialize identically, and SetupForPackages::build over the same packages in two walk orders yields the same key sequences and state-dir set.
Contributor
|
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 (6)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe PR replaces hash-based environment and patch collections with ordered maps and sets. It updates package setup, environment construction, declaration attribution, serialization, fixtures, and regression tests for deterministic ordering. ChangesDeterministic collection ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes package-derived sandbox setup ordering deterministic without changing the intended setup contents; with the reported formatting, lint, build, and test checks passing, no actionable merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the change, motivation, affected crates, reproducibility impact, and verification results. It does not use the template headings or include the checklist, but it contains the required summary and testing information, so the description is mostly complete. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
norrietaylor
approved these changes
Aug 29, 2026
norrietaylor
deleted the
inbox-patch/deterministic-sandbox-setup-order-3cc05fe5d1735e10
branch
August 29, 2026 05:55
norrietaylor
added a commit
that referenced
this pull request
Aug 31, 2026
Review asked why the ordering comment sat above `sort_unstable`, and whether the sort should be stable instead. Neither: sort stability governs what happens to *equal* elements, and these are map keys, so `sort()` and `sort_unstable()` produce the same bytes. The real question was why the carrier was unordered at all. #1319 (gominimal/inbox#552) already answered that for the collections carrying package-derived sandbox setup — `HashMap` → `BTreeMap`, and the compensating `entries.sort_by_key(...)` deleted. A task's `env_vars` was not in that scope. This finishes the row. `mfile::Task::vars` becomes a `BTreeMap`, and the carriers it flows through follow so the guarantee is not re-lost at a boundary: `mctx::MakeEnvArgs::env_vars`, `minimald::env`'s `env_vars` field and `with_env_vars`, and the `decode::stacks` constructor that builds one. `resolve_task_env` then takes its order off the map and its sort is gone. Ordering was already deterministic for the prompt and the denial — that was the sort's doing. What it did not cover, and this does: the daemon `?`s on the first unresolvable `inherit` (`crates/minimald/src/env.rs`), so a task with two unset inherited vars named a different one in its error from run to run. The resulting environment was never affected — each key is inserted once into a map — so this is about diagnostics, not about what the sandbox sees. Records the rule in docs/rust-coding-standards.md, framed by the trigger rather than as a blanket ban: `HashMap` stays right for lookup that is never iterated. It also names the trap that makes "just sort it" look sufficient — a later stable sort preserves hash order faithfully, which is how `min init` picks between two equal-priority stacks today (gominimal/inbox#604). Follow-ups filed for the carriers still unordered: gominimal/inbox#603 (OCI image output vars), gominimal/inbox#604 (stack tables), and gominimal/inbox#605 (the task table and the sorts compensating for it). BREAKING CHANGE: `mfile::Task::vars`, `mctx::MakeEnvArgs::env_vars` and `minimald::env::EnvArgs::with_env_vars` take `BTreeMap` where they took `HashMap`. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 31, 2026
norrietaylor
added a commit
that referenced
this pull request
Sep 1, 2026
…olicy (#1312) * fix(task): resolve task env_vars on the client, not the daemon A task declaring `env_vars.X = { inherit = true }` failed at spawn with `inheriting environment variable 'X': environment variable not found`, even with X exported in the invoking shell. Both resolution sites read `std::env::var` — crates/minimald/src/env.rs and crates/mctx/src/env.rs. Reached from the task path, that is the daemon's own environment, and on macOS the daemon is not even on the same side of the VM boundary as the shell that typed the command. The user's export could never be visible to it. `[session.vars]` used the identical syntax and worked, because activation resolves it client-side before the value is shipped; the two paths diverged in one project, in one run. Resolve a task's env_vars where the values exist. `min task run` reads the task declaration it already loads for the declared-task check, resolves each entry against its own environment, and carries the results to the daemon as channel env requests under `MINIMAL_TASK_ENV_` — the same mechanism that already carries MINIMAL_SESSION_ID and TRACEPARENT. The exec dispatch strips the prefix and applies the values over the task's own declarations, so no `inherit` reaches the daemon-side resolver. The prefix carries the name and the SSH request carries the value, both verbatim, so nothing is escaped and any string a shell can export round-trips exactly. A variable declared `inherit` but unset now fails on the client, naming the task and the variable, instead of surfacing as a spawn failure after a session has already been built and uploaded. Version skew is inert in both directions: an older daemon accepts the env requests and ignores them, a newer daemon sees an empty set from an older client. Either way it falls back to resolving `inherit` itself, which is the pre-change behaviour. That same fallback still serves in-box `min run <task>`, which has no invoking client to read from — documented as the remaining gap rather than silently left to surprise. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(minimal): verify a task's env_vars against the user policy Client-side resolution reads a task's `env_vars` out of the invoking shell and ships the values into the box. Nothing consulted the user's `[vars]` policy on the way, so a `deny` rule — the one control that exists to say "this name never leaves my shell" — was silent on the path that actually reads the shell. Every declared name is now checked before its value is read, in the precedence `VarsPolicy::check` uses: `deny` first, then `ignore`. - `deny` fails the run, naming the task, the variable, and the file the rule lives in. Checked before the lookup, so the value is never read; checked before `ignore`, so a name matching both resolves as denied rather than being quietly dropped. - `ignore` drops the variable and lets the task run. Also checked before the lookup, so an ignored `inherit` need not be set at all. `allow` is deliberately not applied. A task's `env_vars` are declared in the project's own `minimal.toml` and named on the command line, and this path has no prompt: routing them through `allow` would hard-fail every scripted `min task run` until each name were allowlisted, which is the case client-side resolution exists to unblock. `deny` and `ignore` carry no prompt leg, so they apply cleanly. The policy is read before `ensure_daemon`, alongside the declared-task and unset-inherit checks, so a denied variable fails with no session built. It replaces the later read that fed the loadout gating rather than adding a second one. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SumDkFxhZfJZcHbQXH1dFN * docs(tasks): qualify the unset-inherit error against the ignore rule The unset-inherit paragraph states the error absolutely, and the `[vars]` policy section fifteen lines below introduces an exception to it: an ignored name is droppe 8000 d rather than looked up, so it need not be set. A reader hits the absolute claim first and has no reason to keep going. Forward-references the policy section rather than restating it. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): name ignored task vars on the wire so the daemon drops them `ignore` did not ignore anything. `resolve_task_env` skipped a matched name with `continue`, so it was merely *absent* from the map the client sends. But the daemon applies that map by insertion and never removes, so the declaration `minimal.toml` made survived untouched: - an ignored `env_vars.X = { inherit = true }` was still resolved daemon-side through `std::env::var` (crates/minimald/src/env.rs) — the exact indirection #585 exists to delete. It either failed the spawn, or handed the task whatever unrelated value the daemon happened to have under that name. - an ignored literal was untouched entirely; `ignore` did nothing to it. Both contradict what `docs/reference/tasks.md` claims. Absence is not a signal on this wire, because absence already means "an older client sent nothing, resolve it yourself". A drop has to be said out loud. So the client says it. `minimald_rpc::taskenv` gains `TASK_ENV_DROP`, carrying the matched names as a JSON array, and a `TaskEnv { set, drop }` pairing the two halves that have to travel together. The daemon removes before it applies. `TASK_ENV_DROP` is deliberately not under `TASK_ENV_PREFIX`: a name that started with it would be stripped by `from_channel_env` and delivered to the task as a variable called `DROP`. A test pins that. Skew stays inert in both directions. No drop list means remove nothing, which is the behaviour of every client that predates this. The `ignore` test asserted only that the name was absent from the resolved map — true of the broken version too, which is why the whole suite passed over it. It now asserts the name is on the drop list. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): name `min session run` among the daemon-resolved forms The paragraph scoped the fix to `min task run` and named one exception, the in-box `min run <task>`. `min session run <session> <task>` is a second host-invoked way to run a declared task, and it resolves `inherit` daemon-side too: it goes over the attach transport, whose `SendEnv` allowlist carries `MINIMAL_SESSION_ID`, `LANG`, `LC_*` and `TZ` — never the resolved task env. A reader had no way to tell the two apart, and would reasonably assume the fix covered both. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): resolve no env_vars for an echo task An echo task's `env_vars` are read by nobody. The daemon answers one straight from the declaration and returns (crates/minimald/src/exec.rs, the `as_echo` fast path) before it reaches the only place `task.vars` is consumed. So a task declaring both `action = "echo ..."` and `env_vars.X = { inherit = true }` ran fine with `X` unset — the value was never looked at. Client-side resolution broke that: it hard-failed the run, telling the user to export a variable nothing reads. The policy check inherited the same false positive, denying a run over a name the task never sees. `resolve_task_env` now returns an empty set for an echo task before it looks at anything. Placed there rather than at the call site so the rule sits with the resolution it qualifies, and can be tested directly. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(minimal): declare the echo fixture with the key mfile parses `action = 'echo hi'` is not how an echo task is written. `TaskAction` is `#[serde(flatten)]` and its echo variant is the bare `echo` key, so the fixture's `[tasks.t]` never materialised and the test panicked in `task_from_toml` without reaching the guard it exists to cover. Asserts the fixture parses as an echo task before anything else. Without that, a fixture that quietly stopped being one would leave every assertion below passing for the wrong reason — the same shape of hole that let the no-op `ignore` through a full green CI run. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(minimal)!: gate a task's env_vars on the full user policy Client-side resolution reads the user's own shell. That is the point of #585 — and it is also what makes a task's `env_vars` a way to ask for the user's secrets. Before it, `env_vars.AWS_SECRET_ACCESS_KEY = { inherit = true }` resolved against the daemon's environment and got nothing. After it, the same line hands a project a real credential. The first cut applied only `deny` and `ignore`, and argued `allow` away on ergonomics: routing project-declared names through it hard-fails a scripted `min task run` until each is allowlisted. But that is the mitigation working, not a defect in it — and a project you have not read is exactly what the policy exists to constrain. `[session.vars]` from the same `minimal.toml` has always passed the full gate under `Source::Project`; a task's `env_vars` were the inconsistency, not a deliberate exception to it. Every declared name now goes through `VarsPolicy::check` with project provenance — the composer's own call, not a reimplementation, so the precedence cannot drift. `deny` fails, `ignore` drops, `allow` carries, and anything else is referred to `PolicyHooks`: the interactive prompt on a terminal, `NoPromptHook` everywhere else, the same two lanes an activate already uses in this function. The check runs before the lookup. That ordering is the property, not an implementation detail — a gate that resolved first and asked afterwards would already be holding the value. A test drives it with an env closure that panics if called. Names are sorted, so which variable a user is asked about first does not depend on `HashMap` iteration order. An allowlisted name is allowed for every project, not only the one at hand. A per-project trust concept — like the path allowlist that gates project lifecycle hooks — would close that, and is left as follow-up work rather than bolted on here. BREAKING CHANGE: `min task run` on a task whose `env_vars` name anything outside `[vars] allow` now prompts on a terminal, and fails without one. Scripted and CI callers must allowlist the names the task declares; the error prints the exact `[vars]` block to add. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): name the echo exception in the policy section The section says every name a task declares is checked, and an `echo` task's are not: it answers from the declaration without building an environment, so nothing is looked up and there is nothing to gate. States it as the exception it is, and why it is not a hole — a reader who tries `echo` with `env_vars` and sees no prompt should find that documented rather than surprising. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): refuse, don't fake-approve, on the non-interactive gate `NoPromptHook` answers every unapproved item with `AllowOnce`. That is deliberate and safe where it came from: `cmd_activate` only submits a verdict, and checks the summary before it does, so a fake approval never becomes a real one. Its own comment says exactly that. This caller reads values. An `AllowOnce` from that hook flowed into the resolver's last pass and called `env(name)` on a project-declared name the policy had refused — so on a fresh install in CI, a `minimal.toml` naming `env_vars.AWS_SECRET_ACCESS_KEY = { inherit = true }` had it read out of the user's shell before anything looked at the summary and bailed. Nothing reached the daemon; the `TaskEnv` is dropped on that path. But check-before-read is the property the gate exists for, and it was broken in the unattended lane the gate matters most in. `RefuseAndRecord` records the names and returns `Abort`, so the resolver stops before the read. The caller builds the same `[vars] allow` snippet from those names through `toml_edit`, so the error is unchanged. The existing test could not catch this: it drives `resolve_task_env` with a hand-rolled double, never the hook the call site uses, and the double refused where the real one approved. The new test drives `RefuseAndRecord` itself with an env closure that panics if called — verified by reverting the hook to the fake-approving shape and watching it fail in that closure. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(sessions): extract the var gate so tasks don't copy it The task path grew its own classify/refer/apply loop because every reusable piece of the composer's was `pub(crate)`. That is not a reason to have two, and review caught it. `gate_names` is the gate with values factored out: `(name, source)` in, one `NameVerdict` each out, the same three passes. `gate_vars` is now that plus the value plumbing, and `crates/minimal/src/task.rs` calls it directly, dropping its duplicate loop and its `TaskVar` provenance shim. Factoring on names rather than exposing `gate_vars` is what lets the two share at all. `gate_vars` takes `ResolvedVar`, which already holds the value — the session path resolves and then gates. The task path must gate first, because it resolves against the invoking user's shell and reading an unapproved name is the leak the gate exists to stop. A gate that never sees a value serves both. Two behaviours are preserved deliberately in `gate_vars`: literals still skip the policy entirely (`carries_user_data`), and the emitted list keeps input order, so an ungated literal does not jump ahead of a gated var declared before it. Checked by mutation, not just by a green run: making `gate_names` treat an undecidable name as allowed fails 4 tests in `sessions` and 4 in `minimal`, so the branch is pinned from both sides. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): drop gate_names' panic and document its errors `gate_names` broke clippy in two lanes. `missing_panics_doc` and `missing_errors_doc` do not fire on a `pub(crate)` function, so making the gate public exposed both; every other `pub` fn in the crate already carries the sections. Rather than document a panic that should not exist, the panic is gone. Pass 1 seeds an undecided item as `Denied` and queues it, and pass 3 overwrites every one — so the `Option` and its `expect` are unnecessary. The placeholder is fail-closed by construction: an edit that ever left an entry unwritten would make the caller refuse the name rather than carry it, which is the right direction for this gate and better than the `expect` it replaces. Adds the `# Errors` section naming the three real paths: no hook for an item needing one, hook abort, hook contract violation. I ran clippy over the crates I set out to change and not over `sessions`, which is the crate this commit's parent exists to change. `just clippy` scopes to the workspace for a reason. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): use the tree's anchor form, and say why sort_unstable Two review points, both about something reading as other than it is. The policy section anchored itself with a raw `<a id="policy"></a>`. Every other anchor in `docs/` — 28 of them — uses the `{#slug}` form on the heading, and this was the only raw HTML one in the tree. The `[below](#policy)` link added alongside it depends on that anchor resolving, and the published docs render from a different repo whose renderer need not treat the two alike. The ordering comment sat directly above `sort_unstable` while talking about a fixed order, which reads as a contradiction. It is not one: `sort_unstable` describes what becomes of *equal* elements, and these are map keys, so there are none. Says so, rather than leaving the next reader to re-derive it. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): stop the batch at the first denial, before any hook runs Extracting `gate_names` lost the short-circuit. The old pass 1 called `apply_decision(d, ..)?`, which returned `Err` from inside the loop on the first `Denied`; the extracted version recorded a verdict and carried on, so a batch holding a denied name and an unapproved one consulted the hook before the denial ever surfaced. Caught in review. Both call sites were affected, and the session one is a regression in behaviour that predates this branch: - `min task run` non-interactively, with `A` denied and `B` unlisted: the hook is asked about `B` alone, and the operator is told to allow `B` — never `A`. They add `B`, re-run, and meet `A`'s denial for the first time. - Session composition, interactively: the user is prompted about `B`, and an "always allow" answer *writes a rule to user_policy.toml*, before the composition fails over `A`. A persisted policy change for a run that could never have succeeded. `gate_names` now returns `ComposeError::Denied` from the first denial. `NameVerdict` loses that variant — a denial ends the batch, so it is no longer something a caller can be handed — and `task.rs` matches the error to keep naming the task, the variable, and the policy file. The placeholder for a queued item is `Ignored`, still the fail-closed direction. `sessions`' three existing deny tests are all single-item, so nothing covered a mixed batch. Adds one to each crate: the `sessions` one drives a hook that panics if consulted, so the ordering is enforced rather than described; the `minimal` one checks the operator is told about the denial and that the hook recorded nothing. Verified by mutation. Restoring the non-short-circuiting pass 1 fails 4 tests in `sessions` and 3 in `minimal`. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(mfile)!: order a task's env_vars, and drop the sort for it Review asked why the ordering comment sat above `sort_unstable`, and whether the sort should be stable instead. Neither: sort stability governs what happens to *equal* elements, and these are map keys, so `sort()` and `sort_unstable()` produce the same bytes. The real question was why the carrier was unordered at all. #1319 (gominimal/inbox#552) already answered that for the collections carrying package-derived sandbox setup — `HashMap` → `BTreeMap`, and the compensating `entries.sort_by_key(...)` deleted. A task's `env_vars` was not in that scope. This finishes the row. `mfile::Task::vars` becomes a `BTreeMap`, and the carriers it flows through follow so the guarantee is not re-lost at a boundary: `mctx::MakeEnvArgs::env_vars`, `minimald::env`'s `env_vars` field and `with_env_vars`, and the `decode::stacks` constructor that builds one. `resolve_task_env` then takes its order off the map and its sort is gone. Ordering was already deterministic for the prompt and the denial — that was the sort's doing. What it did not cover, and this does: the daemon `?`s on the first unresolvable `inherit` (`crates/minimald/src/env.rs`), so a task with two unset inherited vars named a different one in its error from run to run. The resulting environment was never affected — each key is inserted once into a map — so this is about diagnostics, not about what the sandbox sees. Records the rule in docs/rust-coding-standards.md, framed by the trigger rather than as a blanket ban: `HashMap` stays right for lookup that is never iterated. It also names the trap that makes "just sort it" look sufficient — a later stable sort preserves hash order faithfully, which is how `min init` picks between two equal-priority stacks today (gominimal/inbox#604). Follow-ups filed for the carriers still unordered: gominimal/inbox#603 (OCI image output vars), gominimal/inbox#604 (stack tables), and gominimal/inbox#605 (the task table and the sorts compensating for it). BREAKING CHANGE: `mfile::Task::vars`, `mctx::MakeEnvArgs::env_vars` and `minimald::env::EnvArgs::with_env_vars` take `BTreeMap` where they took `HashMap`. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (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
1 participant
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.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABOFBLlQ
Package-derived sandbox setup — the filesystem mappings applied into a
session sandbox, the env vars set from
env_state_wiring, and the/statesubdirectories created — was carried inHashMap/HashSetcollections keyed by path and variable name, so the same package
declarations could produce a different setup order from run to run, and
EnvPatches(which isSerialize) re-serialized in an unstable order.This moves those path/name-keyed collections to
BTreeMap/BTreeSetacross
mfile,graph,decode,mctxandminimald(the typechange does not compile crate-by-crate, so they move together) and drops
the now-redundant explicit sort in
EnvPatches::to_fs_mappings. Twoadded tests assert the setup is insertion-order and walk-order
independent. None of this feeds a cache key, so it is a reproducibility
change, not a correctness fix.
Verification
Summary by CodeRabbit