Repository navigation
fix: order stack tables by name so min init selects deterministically - #1332
Conversation
Graph.stacks was a HashMap, so Graph::iter_stacks yielded stacks in hash order. match_stack sorts by priority with a stable sort, which preserves that arbitrary order for equal-priority stacks, so min init could scaffold a different stack for the same project across runs. Store the graph and decode-layer stack and container tables in BTreeMaps so iteration is name-ordered and the tie-break is stable.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe change replaces stack and container hash maps with ChangesOrdered stack storage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change makes equally matched stacks resolve deterministically by name without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, affected behavior, regression test, and verification results. It provides the core required information, although it uses
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: matches the stated intent exactly — decode::Layer and graph::Graph stack (and container) tables move from HashMap to BTreeMap, plus one new regression test (iter_stacks_yields_name_order). No unrelated hunks.
Clobber: none. This continues the same HashMap→BTreeMap determinism pattern the team already applied in c37cdb7 (#1319) for env/patch maps; it doesn't revert or conflict with that work.
Correctness: verified the causal chain end-to-end — match_stack (crates/op/src/project/init.rs) uses a stable sort_by on priority, so ties previously resolved to whatever order HashMap iteration happened to yield; ordering the backing maps by name makes that deterministic. Checked every read site of the changed fields (loader ingest, wire reader/writer, mip dump's select_stacks) and none depends on HashMap-specific behavior. Confirmed SpecHash and the wire format's on-disk schema are unaffected by the ordering change, so this doesn't silently invalidate existing caches. The containers half of the change is currently inert (no consumer reads Layer::containers outside its own tests) but is harmless and consistent with the team's practice of converting related maps together.
Nothing here needed a build to check, and nothing was found worth flagging.
What ran
7 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. Read match_stack in crates/op/src/project/init.rs — self-reported
Confirmed match_stack sorts stacks with Vec::sort_by (stable) on matches_project_priority.reverse(), so equal-priority ties keep the iteration order of graph.iter_stacks(). Switching that iterator's backing map from HashMap to BTreeMap directly fixes the described nondeterminism, exactly as the PR body claims.
2. rg for all remaining usages of Graph.stacks / Layer.stacks / Layer.containers across graph, decode, op, mip crates — self-reported
Every read site (Graph::stack, Graph::iter_stacks, loader.rs ingest loop, wire.rs reader/writer, cmd_dump.rs select_stacks) works unchanged against BTreeMap's API; no call site relied on HashMap-specific methods (with_capacity was simply dropped, which BTreeMap doesn't support and doesn't need).
3. Checked whether decode::Layer.containers (also switched to BTreeMap in this diff) is consumed anywhere outside decode's own tests — self-reported
rg found no consumer of Layer.containers in graph/op/mip; the change is inert today but harmless, and mirrors the same HashMap-to-BTreeMap batch pattern the team already applied together in commit c37cdb7 (env_vars/state_dirs/fs_mapping_packages/patch dirs/files) rather than being scope creep unique to this PR.
4. git log -15 on the four changed files — self-reported
Most recent prior touch to crates/decode/src/lib.rs was c37cdb7 ('deterministic order for package-derived setup', #1319), which converted several other HashMaps to BTreeMap/BTreeSet for the same walk-order-independence reason. This PR is a continuation of that established pattern, not a novel or risky change, and does not revert or clobber that commit's work.
5. Checked whether stack/container map order feeds into any content hash (SpecHash) or wire-format compatibility — self-reported
SpecHash is computed per build-spec via SpecHasher::hash and does not incorporate stack/container map order; GraphReader rebuilds the map from serialized records regardless of on-disk order, so old serialized graphs still deserialize correctly and the wire schema itself is unchanged (only iteration order at write time changes).
6. Scanned crates/mip/src/cmd_dump.rs select_stacks / mip dump --stacks — self-reported
Confirmed the PR body's claim: select_stacks already sorts its output by name before dumping, so its behavior is unaffected by the underlying map's ordering change (a stale in-file comment there still says 'stored in a HashMap', but that file isn't touched by this diff so it's outside review scope).
7. grep the diff and touched files for secrets/credentials — self-reported
No matches for key/token/secret/PEM-header patterns; the diff is purely type-signature changes (HashMap -> BTreeMap) plus one new test.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head 34c0e28c3273 · verdict COMMENT · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABPDfrag
min initscaffolds against the stack whose rules match the project directory.match_stack(crates/op/src/project/init.rs) collectsGraph::iter_stacks(), sorts bymatches_project_prioritywith a stable sort, and takes the first match. The graph stored stacks in aHashMap, soiter_stacks()yielded them in hash order and the stable sort preserved that arbitrary order among stacks of equal priority — two equally-ranked stacks that both match a directory could therefore resolve differently across runs, scaffolding a different stack for the same project.This stores the graph and decode-layer stack (and container) tables in
BTreeMaps, soiter_stacks()is name-ordered and the equal-priority tie-break is stable.mip dump'sselect_stacksalready sorted its output by name and is unaffected. The new regression test lives with the changed behavior (iter_stacksordering);match_stackonly preserves that order.Verification
cargo fmt --all --check— cleancargo clippy --workspace --locked -- -D warnings— cleancargo build --workspace --locked— okcargo test --workspace --locked— all pass (addsiter_stacks_yields_name_order)Summary by CodeRabbit
Improvements
Tests