Restore toolchain resolution in nested shells - #3014
Merged
Conversation
`git commit` from an agent session resolves the wrong ruby. The hook lints against system ruby 2.6.10 instead of the project's 3.4.10, and says nothing: it reports plausible paths throughout. Live on all five default branches since bcenv was retired, and what drove the `-n` bypass on bc3 master (d0edc1283b). `mise exec` does not prepend its tool directories unconditionally. When the shims directory is already on the inherited PATH it substitutes the install dirs at that entry's position, so inherited ordering -- not the hook -- decides the toolchain: shims ahead of /usr/bin installs land ahead project ruby /usr/bin ahead of shims installs land behind SYSTEM ruby shims absent entirely installs prepended project ruby A shipyard login shell puts shims first. An agent harness PATH puts /usr/bin at ~#12 and shims at ~#25. All five bin/rubocop are `#!/usr/bin/env ruby`, so the nested PATH picks the interpreter directly. bin/bcenv was immune because it re-derived PATH from the login profile rather than inheriting it; retiring it removed that protection. Arch is not affected -- shims #1, /usr/bin #4 in both login and non-login shells -- so this is macOS-agent-shell exposure today. The fix is ordering-independence, which is the property worth having everywhere. Pre-merge revisions carried exactly this fix and it was retired as unnecessary, on the finding that the broken ordering was "an artifact of a nested agent shell rather than a fleet condition". That premise is now false: shipyard provisions Claude Code, Codex and opencode on every machine, so the nested agent shell IS the fleet condition. Prepend rather than drop, decided on evidence. The retired code removed the shims entry, which works but needs canonical path comparison: a trailing-slash spelling defeats an exact-string filter and mise then behaves as if nothing was removed (measured: /usr/bin:$shims/:/bin resolves /usr/bin/ruby). That comparison was a ~20-line helper, itself refined twice for its own bugs. Prepending needs no equality logic and fixes every case that defeats dropping -- trailing slash, exact-but- late, shims absent, shims already first. The precedence chain mirrors mise's own, so a customised store is still honoured; checked against `mise doctor` for default storage, MISE_DATA_DIR, XDG_DATA_HOME, MISE_SHIMS_DIR, and overlapping overrides. The suite passed 14/14 against the broken hook, because neither existing scenario can reach the bug: the GUI one builds a shims-free PATH, and the terminal one harvests an already-ordered PATH from the login profile. So a third scenario models an agent session, ordering shims below /usr/bin. It forwards its own store variables rather than re-deriving them, because run_hook rebuilds the environment with `env -i` -- the same defect already found and fixed for the terminal scenario -- and carries __MISE_ORIG_PATH as a real agent session does. MISE_SHIMS_DIR joins the terminal harvest, since hook behaviour now depends on it. That scenario also asserts the resolved path, not just the version. A version comparison cannot detect this bug on Arch at all, where /usr/bin/ruby is also 3.4.10 and a broken hook passes. The assertion is scoped to the agent scenario deliberately: the expected path is derived in the invoking environment, so it names the invoking environment's store, while the GUI scenario deliberately receives no store variables and resolves the default one. Applying it globally would contradict the GUI model and fail a working hook -- reproduced with MISE_DATA_DIR pointed at an empty store, where the whole suite still passes, the agent scenario resolving out of that empty store and the other two out of the default one. Negative control run on both platforms: stripping only the prepend fails the agent scenario on macOS by version and by path, and on Arch by path alone. Verified end-to-end in a real agent shell, and confirmed inert where behaviour was already correct -- clean login shell and bare GUI environment unchanged on both platforms.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b205899218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The new assertion compares an INSTALL path, and run_hook rebuilds the environment with `env -i`. Measured against `mise which ruby`, exactly two variables relocate the install tree: MISE_INSTALLS_DIR and MISE_DATA_DIR (XDG_DATA_HOME only through the latter's default). Only the second was forwarded, so under a MISE_INSTALLS_DIR override the probe resolved the default tree while expected_ruby_path named the custom one -- failing a working hook. Same defect already fixed for the terminal scenario, so it joins that harvest too. Reproduced with a populated custom install tree: without the forward the agent scenario fails 16/17, resolving ~/.local/share/mise/installs/ruby against an expected .../instdir/ruby; with it, 17/17. Found by Codex review.
The header recorded that shims handling was dropped because shipyard "already orders ~/.local/bin and mise's shims above /usr/bin, so `mise exec` resolves the toolchain unaided". That is the premise this branch overturns, so leaving it in place would have the file arguing against its own third scenario. It now states both halves: a login shell does order them that way, an agent session does not, and the new scenario is what holds that premise to account. The agent scenario also reuses $_mise_dirs rather than repeating it. The two lists existed for one reason -- name the mise configuration the expected values were derived from -- and the copy had already drifted once, which is precisely how the MISE_INSTALLS_DIR gap arose.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4fce5357a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The paragraph sat after "Run it directly: ./test/prek-hook-test", where "Git also runs it" read as the test rather than the hook. The header already enumerated the ways git runs the hook, so the agent session joins that list -- two ways becomes three -- and the correction of the stale premise follows it.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 904f666e82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…test expected_ruby and expected_ruby_path are derived in the invoking environment, which may itself be an agent shell with shims below /usr/bin -- so it looks like they could pick up the system ruby and report every scenario as failing. They cannot, and nothing in the file said why. The distinction is what mise is asked to resolve. `mise exec -- ruby` and `mise which ruby` name a TOOL, which mise resolves from its own registry. `mise exec -- <script>` only hands the script a PATH and lets the script resolve ruby itself; that is the hook's form, and how prek reaches bin/rubocop's `#!/usr/bin/env ruby`. Measured with a fake ruby placed first, ahead of both /usr/bin and the shims entry: the tool form returned 3.4.10, the script form ran the fake. Without the decoy, on the plain agent ordering: 3.4.10 and 2.6.10 respectively. Raised by Codex review; the finding does not hold, but the file was silent on it.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b338d64d20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mise honours MISE_SHARED_INSTALL_DIRS, so a store reachable only that way relocates the install tree too. run_hook's `env -i` dropped it, and the agent scenario asserts an install path -- so in a shared-store setup ground truth named the shared binary while the probe resolved, or auto-installed, a local copy, failing a working hook. The earlier revision asserted "exactly two variables relocate the install tree" and was measured wrongly: against a populated local store, which satisfies the lookup first and masks every fallback behind it. Re-measured against an empty local store, three do -- MISE_INSTALLS_DIR, MISE_SHARED_INSTALL_DIRS and MISE_DATA_DIR (XDG_DATA_HOME only through the latter's default) -- while MISE_SHIMS_DIR, MISE_STATE_DIR, XDG_STATE_HOME, MISE_CACHE_DIR, MISE_CONFIG_DIR and MISE_DOWNLOADS_DIR do not. Two things keep that method honest, and the comment now names both. A made-up variable name must leave `mise which ruby` unresolved, or the probe is measuring nothing. And mise distinguishes "Setting [x] is not set" from "Unknown setting: x"; only the second means mise does not read it. Reading the first as the second is what produced the wrong claim. Control, in a shared-store-only configuration (empty local store, empty config dir so no global tools are dragged in): without the forward the agent scenario fails 16/17, expected $shared/ruby/3.4.10/bin/ruby against a locally installed copy; with it, 17/17 resolving the shared path. The passing run has to start from a pristine store, or it passes off the local copy the failing run created -- the same masking again. Found by Codex review, twice: raised, wrongly rebutted here, re-raised with the flaw in my method named exactly.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The list is bounded by measurement, not enumerated from mise, and the comment now says so. `mise settings --all` does not report the path settings at all -- not data_dir, not installs_dir, not shared_install_dirs -- so there is no authoritative set to copy, and a variable nobody thought to probe would be missed the same way MISE_SHARED_INSTALL_DIRS was. Worth stating rather than implying completeness, because two rounds of review found holes in it. What bounds the damage is the direction of the failure: the assertion compares the hook's resolution against mise's own, so a missing variable makes the two disagree and this scenario fail under a store override. It cannot turn into a false pass.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
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.
git commitfrom an agent session resolves the wrong ruby. The hook lints againstsystem ruby 2.6.10 instead of the project's 3.4.10, and says nothing — it reports
plausible paths throughout. Live on all five default branches since bcenv was retired,
and what drove the
-nbypass on bc3 master (d0edc1283b). Reproduced independently inboth Claude Code and Codex sessions.
Independently hit again during this review, by an unrelated agent session working in a
different bc3 worktree — reported as "prek rubocop hook is broken repo-wide, pre-existing",
surfacing as
You must use Bundler 4 or greater with this lockfile(system Ruby 2.6'sbundled Bundler, rather than the traceback seen elsewhere), and worked around with
--no-verifyafter runningbin/rubocopby hand. Reproduced in that worktree: thecurrent hook fails there with exactly that message, and this branch's hook reports
rubocop … Passed. Their observation thatmise exec -- ruby -vprints 3.4.10 in the sameshell where the hook still fails is the tool-vs-script distinction described below.
Mechanism
mise execdoes not prepend its tool directories unconditionally. When the shimsdirectory is already on the inherited PATH it substitutes the install dirs at that
entry's position, so inherited ordering — not the hook — decides the toolchain:
mise exec/usr/bin/usr/binahead of shimsA shipyard login shell puts shims first. An agent harness PATH puts
/usr/binat ~#12and shims at ~#25. All five
bin/rubocopare#!/usr/bin/env ruby, so the nested PATHpicks the interpreter directly.
bin/bcenvwas immune because it re-derived PATH fromthe login profile rather than inheriting it; retiring it removed that protection.
Arch is not affected — shims #1,
/usr/bin#4 in both login and non-login shells — sothis is macOS-agent-shell exposure today. The fix is ordering-independence, which is the
property worth having everywhere.
This is not a new idea, and that matters. Pre-merge revisions carried exactly this
fix and it was retired as unnecessary, on the finding that the broken ordering was "an
artifact of a nested agent shell rather than a fleet condition". That premise is now
false: shipyard provisions Claude Code, Codex and opencode on every machine, so the
nested agent shell is the fleet condition.
Prepend rather than drop — decided on evidence
The retired code removed the shims entry. That works, but needs canonical path
comparison: a trailing-slash spelling defeats an exact-string filter and mise then
behaves as if nothing was removed (measured:
/usr/bin:$shims/:/bin→/usr/bin/ruby).That comparison was a ~20-line
physical()helper, itself refined twice for its ownbugs. Prepending needs no equality logic and fixes every case that defeats dropping —
trailing slash, exact-but-late, shims absent, shims already first. The precedence chain
mirrors mise's own, so a customised store is still honoured; checked against
mise doctorfor default storage,MISE_DATA_DIR,XDG_DATA_HOME,MISE_SHIMS_DIR,and overlapping overrides.
Why the suite passed 14/14 against a broken hook
Neither existing scenario can reach the bug: the GUI one builds a shims-free PATH, and
the terminal one harvests an already-ordered PATH from the login profile. So a third
scenario models an agent session, ordering shims below
/usr/bin. It forwards its ownstore variables rather than re-deriving them, because
run_hookrebuilds the environmentwith
env -i— the same defect already found and fixed for the terminal scenario — andcarries
__MISE_ORIG_PATHas a real agent session does.MISE_SHIMS_DIRjoins theterminal harvest, since hook behaviour now depends on it.
That scenario also asserts the resolved path, not just the version. A version
comparison cannot detect this bug on Arch at all, where
/usr/bin/rubyis also 3.4.10and a broken hook passes:
/usr/bin/ruby…/mise/installs/ruby/3.4.10/bin/ruby/usr/bin/rubyThe assertion is scoped to the agent scenario deliberately, not for tidiness. The
expected path is derived in the invoking environment, so it names the invoking
environment's store, while the GUI scenario deliberately receives no store variables and
resolves the default one. Applying it globally would contradict the GUI model and fail
a working hook.
Verification
shellcheck .githooks/pre-commit test/prek-hook-testclean in all five.sha256of both files identicalacross all five repos: hook
f7ed8a4b…, test2f22d3e5….agent scenario — on macOS by version and path (
2.6.10,/usr/bin/ruby), on Arch bypath alone (
/usr/bin/rubyvsmise which ruby, both 3.4.10). The Arch run is theevidence that a version-only assertion would have been decorative there.
/usr/binBump rails from16d8b82to72741db#22, shims Bump rails from16d8b82to7e0df2f#35), finalexecswapped fora probe: before,
2.6.10at/usr/bin/ruby; after,3.4.10at~/.local/share/mise/installs/ruby/3.4.10/bin/ruby, matchingmise which ruby.Then the unmodified hook, with a real Ruby file staged so
rubocopactually runs —the pre-fix hook dies inside
/System/Library/Frameworks/Ruby.framework/Versions/2.6/.../bundler.rband exits 1;this one reports
rubocop … Passedand exits 0. That is the failure the-nbypasswas working around.
byte-identical before and after, on both platforms; plus Arch's native shims-first
ordering.
MISE_DATA_DIRpointed at an empty store: 17/17, with theagent scenario resolving out of that empty store and GUI/terminal out of the default
one — confirming the path assertion did not leak past the agent scenario.
Assertion count goes 14 → 17 here (20 in launchpad, which has a second
[env]key).From review
Four Codex rounds. Two real bugs, both in the test harness, both with the same shape:
the agent scenario asserts an install path, and
run_hookrebuilds the environmentwith
env -i, so any store variable it fails to forward makes the harness name one storewhile the hook derives another — failing a working hook.
MISE_INSTALLS_DIRwas not forwarded. Fixed, with its own negative control againsta populated custom tree: 16/17 without, 17/17 with.
MISE_SHARED_INSTALL_DIRSwas not forwarded either. Fixed. Codex raised this, Irebutted it wrongly, and it re-raised it with the flaw in my method named exactly —
I had measured against a populated local store, which satisfies the lookup first and
masks every fallback behind it. I also misread mise's own signal: it distinguishes
Setting [x] is not set(known, unset) fromUnknown setting: x, and I read the firstas the second, inverting the one probe that pointed the right way.
Re-bounded properly — empty local store, plus a made-up variable name as a control that
must fail to resolve — three variables relocate the install tree:
MISE_INSTALLS_DIR,MISE_SHARED_INSTALL_DIRS,MISE_DATA_DIR(XDG_DATA_HOMEonly viathe latter's default).
MISE_SHIMS_DIR,MISE_STATE_DIR,XDG_STATE_HOME,MISE_CACHE_DIR,MISE_CONFIG_DIR,MISE_DOWNLOADS_DIRdo not. All three are forwarded,and the method — including that a populated local store hides the result — is recorded in
the file, because the comment that stood there asserted "exactly two".
Control for the second fix, in a shared-store-only configuration: without the forward the
agent scenario fails 16/17, expecting the shared binary and getting a locally
auto-installed one; with it, 17/17 resolving the shared path. The passing run must start
from a pristine store, or it passes off the copy the failing run created — the same
masking again. Confirmed honoured on both platforms and both fleet mise versions.
One round was a genuine rebuttal: whether
expected_rubycould itself pick up the systemruby, since it is derived in the invoking environment. It cannot —
mise exec -- rubyandmise which rubyname a tool, which mise resolves from its own registry, whilemise exec -- <script>only hands the script a PATH. The second form is the hook's,because prek reaches
bin/rubocop, which is#!/usr/bin/env ruby; that is themechanism. Measured with a decoy
rubyplaced ahead of everything: tool form3.4.10,script form ran the decoy. The finding does not hold, but nothing in the file said why, so
the distinction now sits beside the ground-truth lines.