Scrub stale mise install paths before launching harness - #79
Conversation
|
This diagnosis matches upstream mise discussion/PR:
Important release detail: latest release This sessions PR still seems useful as a harness-level defense until the fixed mise version is released and installed everywhere. It also makes long-lived agent sessions less sensitive to the wake stack's repo/tool versions. Separate note: this upstream mise fix is about PATH/tool-version leakage. It does not address the unrelated inherited |
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the engine PATH scrubbing change and regression coverage. I didn't find any blocking issues. Verified with cd cli && mix test and git diff --check.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the path sanitization change and regression coverage. Verified with cd cli && mix test test/engine_test.exs and cd cli && mix test.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the PATH scrubbing change and regression coverage. I did not find any blocking issues. Verified with git diff --check origin/main...HEAD, cd cli && mix test test/engine_test.exs, and cd cli && mix test.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the PATH sanitization change and regression test. Verified with cd cli && mix test (passes). No blocking issues found.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the PATH scrubbing change and regression coverage. The engine still scrubs CALLER_PWD env while now overriding PATH with non-mise entries plus shims when available, which matches the stale direct-install failure mode.\n\nVerification: cd cli && mix test
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the PATH scrub change and regression coverage. I didn't find any blocking issues.\n\nVerification: cd cli && mix test passes locally.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the engine PATH scrubbing changes and regression coverage. Verified with cd cli && mix test and cd cli && mix format --check-formatted lib/cli/engine.ex test/engine_test.exs.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one issue before this lands:\n\n- cli/lib/cli/engine.ex:111: the sanitizer derives both installs and shims from HOME/.local/share/mise only. mise supports alternate data dirs via MISE_DATA_DIR (and XDG_DATA_HOME), and in those setups the stale direct install PATH entries are exactly under that alternate data dir, so this leaves them in place and prepends the wrong shim dir. Please derive the mise data dir from MISE_DATA_DIR/XDG_DATA_HOME before falling back to HOME/.local/share/mise, and add coverage for that case.\n\nI ran: cd cli && mix test
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The default mise data-dir case is covered and \ passes here. One edge case to consider before relying on this broadly: the sanitizer hard-codes , so installs/shims under a configured \ (or equivalent non-default data dir) would not be scrubbed/added and could still leak stale direct install bins into the harness PATH.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Reviewed the PATH scrub in Cli.Engine and the regression coverage. cd cli && mix test passes locally. No blocking findings.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The default mise data-dir case is covered and cd cli && mix test passes here. One edge case to consider before relying on this broadly: the sanitizer hard-codes ~/.local/share/mise, so installs/shims under a configured MISE_DATA_DIR (or equivalent non-default data dir) would not be scrubbed/added and could still leak stale direct install bins into the harness PATH.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one portability gap: the scrubber only considers the default \ tree, but mise installs/shims can live under . In sessions launched from an environment with a custom data dir, stale \ entries would still be inherited and the corresponding shims dir would not be restored, so the original stale-direct-install problem can persist. Please derive the mise data dir from \ when set, falling back to the HOME default.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one blocking issue: sanitized_path/1 only derives the mise data directory from HOME, so installs/shims under a configured MISE_DATA_DIR are left untouched. In environments that set MISE_DATA_DIR, stale entries like /installs/.../bin will still be inherited by the harness, and the wrong ~/.local/share/mise/shims may be prepended instead. Please resolve the data dir from MISE_DATA_DIR with the current ~/.local/share/mise fallback and add coverage for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks — the PATH scrubbing logic looks generally right, and cd cli && mix test passes locally. I found one blocker before merge: the implementation hard-codes the default mise data dir under HOME. If the launching environment sets MISE_DATA_DIR, direct install bins under that data dir will still leak into harness sessions, and the shims from that dir will not be restored. Since mise supports/uses MISE_DATA_DIR as the data root, this would leave the stale-install issue unfixed for those environments. Please derive the installs/shims root from MISE_DATA_DIR when present, falling back to /home/runner/.local/share/mise.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one portability gap: the scrubber only considers the default $HOME/.local/share/mise tree, but mise installs/shims can live under MISE_DATA_DIR. In sessions launched from an environment with a custom data dir, stale $MISE_DATA_DIR/installs/.../bin entries would still be inherited and the corresponding shims dir would not be restored, so the original stale-direct-install problem can persist. Please derive the mise data dir from MISE_DATA_DIR when set, falling back to the HOME default.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one remaining correctness gap: the scrub only considers the default ~/.local/share/mise tree. If the launcher was running with MISE_DATA_DIR set, stale direct-install bins under that directory will still be inherited by the harness and can continue to beat shims, which is the same failure mode this PR is trying to prevent. Please derive the install/shims root from MISE_DATA_DIR when present (falling back to the current HOME default) and add a regression for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one issue that keeps this from covering non-default mise layouts: the scrubber derives both installs and shims solely from HOME, but mise can be configured with MISE_DATA_DIR. In those sessions, PATH entries under $MISE_DATA_DIR/installs survive and the harness can still pick up stale tools; the shim re-addition also points at the wrong directory. Please derive the mise data dir from MISE_DATA_DIR when present (falling back to HOME) and cover that with a test.\n\nValidated locally: cd cli && mix test
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one correctness gap before merge: the scrub path is hard-coded to /home/runner/.local/share/mise, but mise can be configured with MISE_DATA_DIR. In that case the stale installs/.../bin entries that caused this issue remain in the harness PATH, and the shim re-add points at the wrong location. Please derive the data dir from MISE_DATA_DIR when set, falling back to the current HOME default, and add coverage for that case.\n\nI also ran cd cli && mix test locally (passes with the current working tree).
x1f9-ricon
left a comment
There was a problem hiding this comment.
I found one correctness issue: configured mise data dirs are not scrubbed, so stale install paths can still leak into harness sessions.
| end | ||
|
|
||
| defp sanitized_path(path) do | ||
| home = System.get_env("HOME") |
There was a problem hiding this comment.
This derives the mise data dir only from HOME, so it misses the standard MISE_DATA_DIR override. In that setup a harness launched with MISE_DATA_DIR=/opt/mise and PATH=/opt/mise/installs/.../bin:... keeps the stale install entry and also won't prepend /opt/mise/shims, which is the exact class of leak this patch is meant to prevent. Please resolve the data dir the same way mise does (at least honoring non-empty MISE_DATA_DIR) before computing installs/shims, and add coverage for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix — the default HOME-based mise path is covered and \Compiling 1 file (.ex)
Generated cli app passes locally. I found one blocking gap: this hard-codes mise's data directory to . mise supports , and if sessions is launched from an environment using a custom data dir, stale entries under \ will remain in the harness PATH while the wrong shims dir may be prepended. Please derive the installs/shims base from \ when set, falling back to the HOME path.
x1f9-ricon
left a comment
There was a problem hiding this comment.
I found one correctness gap that should be fixed before merge.\n\nThe sanitizer hard-codes mise's default data dir under HOME, but mise installs and shims can live under MISE_DATA_DIR. In that setup, inherited PATH entries like /installs/.../bin are left intact and /shims is not added, so the stale direct-install shadowing this PR is meant to prevent can still happen for non-default mise data dirs. Please derive the mise data dir from MISE_DATA_DIR when it is set (falling back to ~/.local/share/mise) and add a regression for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one blocking issue: the sanitizer is hard-coded to /home/runner/.local/share/mise, but mise installs can live under . In that configuration the stale install entries remain in PATH and the PR does not fix the failure mode. Please derive the mise data dir from MISE_DATA_DIR when set (falling back to the current default) and add coverage for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the clear root-cause writeup and tests. I found one gap to address before merge: the scrubber hard-codes the default mise data directory under HOME. If the launcher is running with MISE_DATA_DIR set, the inherited direct-install bins live under $MISE_DATA_DIR/installs, so they will survive this filter and can still beat the shims. The shims reinsertion also points at the wrong directory in that setup. Please derive the installs/shims paths from MISE_DATA_DIR when present (falling back to the current HOME default) and add a regression for that case.\n\nVerified locally: , \Compiling 1 file (.ex)
Generated cli app, and .
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one path-scrubbing gap for custom mise data directories.
| defp sanitized_path(path) do | ||
| home = System.get_env("HOME") | ||
|
|
||
| mise_installs = |
There was a problem hiding this comment.
This only scrubs the default $HOME/.local/share/mise tree. Mise installations can be relocated with MISE_DATA_DIR; in that configuration PATH entries like $MISE_DATA_DIR/installs/shiv-codebase/.../bin will survive this filter, so the stale install path issue still reproduces (and we may prepend the wrong shims dir). Please derive the data dir from MISE_DATA_DIR when it is set, falling back to the HOME default, and cover that case in the test.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one environment coverage gap that can leave the stale install path issue in place.\n\nThe sanitizer hard-codes the default mise data dir under HOME (~/.local/share/mise) for both installs and shims. mise supports overriding this with MISE_DATA_DIR; when an agent launcher uses that (common in isolated CI/agent installs), direct bins like /installs/shiv-codebase/0.1.0/bin will remain ahead of shims, so the regression this PR addresses can still reproduce. The shim insertion would also point at the wrong directory. Please derive the mise data dir from MISE_DATA_DIR when set (falling back to the current default) and add a regression for that case.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Found one correctness gap: the sanitizer hard-codes /home/runner/.local/share/mise, so it does not scrub installs when mise is using MISE_DATA_DIR or XDG_DATA_HOME. Please derive the mise data dir the same way mise does before merging.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. I ran cd cli && mix test successfully on the PR branch.\n\nOne issue I think should be addressed before merge: sanitized_path/1 hard-codes mise's data directory to /home/runner/.local/share/mise. mise can relocate that via MISE_DATA_DIR (and commonly via XDG_DATA_HOME/mise), so in those environments the inherited .../installs/.../bin entries would not be scrubbed and the actual shims directory would not be re-added. That leaves the same stale direct-install precedence bug for non-default mise installs, and can also prepend the wrong shims directory. Please derive the mise data dir from the environment before falling back to ~/.local/share/mise.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the fix. I found one blocking issue:
cli/lib/cli/engine.ex:111-116hard-codes mise's data dir to$HOME/.local/share/mise. mise also supportsMISE_DATA_DIR; when that is set, install entries are under$MISE_DATA_DIR/installsand shims under$MISE_DATA_DIR/shims. In that common configuration this sanitizer leaves the stale install bins in PATH and may prepend the wrong shims dir, so the issue can still reproduce. Please derive the mise data dir fromMISE_DATA_DIRfirst (falling back to the current default) and cover it with a test.
Tests run: cd cli && mix test
x1f9-ricon
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. I found one correctness gap that should be addressed before merge: the sanitizer derives mise's installs/shims root only from $HOME/.local/share/mise. mise can run with a configured data root (MISE_DATA_DIR, and commonly XDG_DATA_HOME/mise), and in those environments stale direct-install PATH entries under that configured installs directory will still be inherited while the matching shims directory is not restored. That leaves the same stale-tool precedence bug this PR is meant to prevent for non-default mise layouts. Please derive the data dir from the environment before falling back to the HOME default and add coverage for that case.\n\nVerification: cd cli && mix test test/engine_test.exs; cd cli && mix test.
…h-scrub Honor configured mise data dir in harness PATH scrub
|
Merged x1f9's fix-it #85 into this branch. Post-merge CI is green on Ubuntu and macOS. Requesting re-review to clear the prior change request. |
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85. MISE_DATA_DIR/XDG/HOME handling covers the stale mise install path case, shims are restored when available, and CI plus local cd cli && mix test are passing. This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now honors MISE_DATA_DIR/XDG_DATA_HOME/default mise locations, preserves non-mise PATH entries, and CI/local tests are green. This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR follow-up. The PATH scrub now covers MISE_DATA_DIR, XDG_DATA_HOME, and default HOME-derived mise data dirs, preserves non-mise PATH entries, and restores shims when present. CI is green and I also verified locally with \ (121 tests, 4 doctests). This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 merged. The PATH scrub now respects MISE_DATA_DIR/XDG_DATA_HOME/default mise data dir, CI is green, and I verified locally with cd cli && mix test. This resolves my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR follow-up. The PATH scrub now covers MISE_DATA_DIR, XDG_DATA_HOME, and default HOME-derived mise data dirs, preserves non-mise PATH entries, and restores shims when present. CI is green and I also verified locally with cd cli && mix test (121 tests, 4 doctests). This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85. The PATH scrub now honors MISE_DATA_DIR/XDG_DATA_HOME fallback, preserves non-mise PATH entries, and reinserts shims only when present. CI is green and I also verified locally with cd cli && mix test (121 tests, 4 doctests). Clearing my previous changes-requested state.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR/XDG_DATA_HOME support landed in #85. The PATH scrub now derives the mise data directory from the configured environment before falling back to the default, preserves/introduces shims where available, and test coverage exercises the default, MISE_DATA_DIR, and XDG_DATA_HOME cases. CI is green and local cd cli && mix test passes (121 tests, 4 doctests). No remaining blockers from my previous review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. MISE_DATA_DIR/XDG_DATA_HOME/default mise data-dir handling now covers the PATH scrub cases I flagged, and CI is green. I also reran cd cli && mix test locally (121 tests, 4 doctests passing).
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME, then the HOME default; stale direct install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. CI is green and local cd cli && mix test passes. This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. MISE_DATA_DIR/XDG_DATA_HOME/HOME handling now covers custom and default mise data dirs, shims are restored when present, and the regression coverage passes locally with cd cli && mix test. No remaining blockers from me.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME/mise, then the HOME default; stale direct-install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. Regression coverage exercises the default, MISE_DATA_DIR, and XDG_DATA_HOME cases.
Verification:
mise installcd cli && mix test test/engine_test.exscd cli && mix testgit diff --check origin/main...HEADgh pr checks 79 --watch --interval 10
No remaining blockers from my previous review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR support landed in #85. The PATH scrub now respects MISE_DATA_DIR/XDG/default mise data locations, preserves non-mise PATH entries, and adds shims when available. CI is green and I verified locally with cd cli && mix test. Looks good to merge.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. MISE_DATA_DIR/XDG_DATA_HOME/HOME handling now covers custom and default mise data dirs, shims are restored when present, and the regression coverage passes locally with cd cli && mix test. No remaining blockers from me.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 merged. The MISE_DATA_DIR/XDG/HOME resolution is covered, CI is green, and local cd cli && mix test passes. This clears my previous changes-requested state.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR/XDG_DATA_HOME fix landed. CI is green and local cd cli && mix test passes (4 doctests, 121 tests). Looks good to merge.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME, then the HOME default; stale direct install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. CI is green and local verification passes: cd cli && mix test test/engine_test.exs && mix test (6/6 engine tests, 121 tests + 4 doctests overall). This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The MISE_DATA_DIR/XDG_DATA_HOME handling addresses my prior blocker, tests cover the configured data-dir cases, and CI is green. Local cd cli && mix test passes (121 tests, 4 doctests).
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0b15902 after #85 was merged into the branch. The PATH scrub now honors MISE_DATA_DIR, XDG_DATA_HOME, and the default mise data dir, removes stale mise install entries while preserving non-mise PATH entries, and prepends existing shims. CI is green on Ubuntu/macOS and I verified locally with cd cli && mix test. Looks good to merge.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME/mise, then the HOME default; stale direct-install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. Regression coverage covers default, MISE_DATA_DIR, and XDG_DATA_HOME cases.\n\nVerification: cd cli && mix test test/engine_test.exs && mix test; CI is green on Ubuntu and macOS. No remaining blockers from my previous review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME, then the HOME default; stale direct install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. CI is green and local verification passes with cd cli && mix test (121 tests + 4 doctests). This clears my previous changes-requested review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed after the MISE_DATA_DIR/XDG_DATA_HOME fix landed. CI is green on Ubuntu/macOS and local cd cli && mix test passes (4 doctests, 121 tests). The PATH scrub now covers configured mise data dirs, falls back through XDG_DATA_HOME/HOME, preserves non-mise PATH entries, and only prepends mise shims when present. Looks good to merge.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0b15902 after #85 landed. The PATH scrub now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME/mise, then the HOME default; stale direct-install bins are removed, non-mise PATH entries are preserved, and shims are restored when present. Regression coverage exercises the default, MISE_DATA_DIR, and XDG_DATA_HOME cases.
Verification:
mise installcd cli && mix test test/engine_test.exscd cli && mix testgit diff --check origin/main...HEADgh pr checks 79 --watch --interval 10
No remaining blockers from my previous review.
x1f9-ricon
left a comment
There was a problem hiding this comment.
Approved. The merged #85 fix resolves my previous blocker: Cli.Engine now derives the mise data dir from MISE_DATA_DIR, then XDG_DATA_HOME, then HOME, so non-default mise installs have their direct installs/*/bin entries scrubbed and their shims restored when present.
I re-read the diff and checked the harness-env boundary: caller-cwd scrubbing still happens, PATH entries outside mise installs are preserved, direct mise install paths are removed, and the new tests cover default, MISE_DATA_DIR, and XDG_DATA_HOME layouts.
Verification:
mise trustmise installmise run cli:buildcd cli && mix test(121 tests + 4 doctests)env -u usage_cwd -u usage_headless -u usage_message -u usage_meta -u usage_model -u usage_session -u usage_session_id -u usage_timeout mise run test(222/222)git diff --check origin/main...HEADcd cli && mix format --check-formatted lib/cli/engine.ex test/engine_test.exsgh pr checks 79 --watch(Ubuntu + macOS green)
Notes: a plain mise run test in this headless session still fails from inherited usage_* environment variables, matching the known ambient-env issue; unsetting those gives a clean full suite. Whole-repo mix format --check-formatted also reports pre-existing formatting drift in files outside this PR diff, while the changed Elixir files pass format check.
Summary
~/.local/share/mise/installs/...entries from the PATH inherited by the harness processWhy
During Brownie's CI review of KnickKnackLabs/wallpapers#55, a bare
codebase ...from~/agents/brownie/wallpapersresolved to fold'sshiv-codebase@0.1.0direct install, even though wallpapers declaredshiv:codebase=latestandmise.lockresolved0.2.0.Root cause: the agent process is launched from nested mise/shiv tasks, so it inherits direct install bin paths from the launcher repo (
foldpins codebase 0.1.0). Latercdinto another repo does not reorder those already-prepended direct install entries, so bare commands can beat the mise shims and ignore the target repo's lockfile.Long-lived agent sessions should inherit shims, not fixed-version launcher install bins. This keeps bare commands repo-sensitive from the agent's perspective.
Verification
cd cli && mix test test/engine_test.exscd cli && mix testgit diff --checkNote:
mise run testcurrently has unrelated failures in existing BATS around meta/search/wake state; I did not treat that as verification for this PR.