main: don't honour PATH_DIRS for a precommand's target command - #987
main: don't honour PATH_DIRS for a precommand's target command#987yfwmaniish wants to merge 2 commits into
Conversation
sudo(8) (and env, nice, and other entries in \$precommand_options) resolve their target command via execvp(3)-style lookup, which never searches \$path for a name containing a slash -- unlike the shell's own PATH_DIRS option. So "sudo foo/bar" was highlighted as a valid command whenever the user had PATH_DIRS set and some \$path element made "foo/bar" resolvable that way, even though sudo itself would fail to find it. :sudo_opt: already marks every word from a recognised precommand up to and including its actual command word (for any precommand in \$precommand_options, not just sudo). Key off that existing marker to shadow PATH_DIRS out of \$options_to_set for just those two _zsh_highlight_main__type calls, via an anonymous-function scope so the shadow reverts automatically and normal command-word classification elsewhere in the line is unaffected. Fixes zsh-users#595.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the main highlighter’s command classification so that PATH_DIRS is not applied to the target command word that follows a recognized “precommand” (e.g. sudo, env, nice), matching execvp(3) semantics for slash-containing names and fixing incorrect highlighting reported in #595.
Changes:
- Mask
PATH_DIRSduring_zsh_highlight_main__typeclassification when the word is marked with:sudo_opt:(precommand context). - Add a regression test ensuring
sudo bar/testing-issue-228is highlighted asunknown-tokeneven whenPATH_DIRSwould make it resolvable as a direct command.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| highlighters/main/main-highlighter.zsh | Adjusts command-type checking to avoid honoring PATH_DIRS for the precommand’s target command word. |
| highlighters/main/test-data/sudo-path_dirs.zsh | Adds a test fixture covering the sudo + PATH_DIRS slash-path false-positive case. |
Suppressed comments (2)
highlighters/main/main-highlighter.zsh:720
- The PATH_DIRS masking here can be bypassed by
_zsh_highlight_main__type’s command-type cache: the cache key is only the command name, so if the same slash-containing command was previously classified with PATH_DIRS enabled, this call may return the cachedcommandeven though PATH_DIRS was removed fromoptions_to_set. Consider also shadowing the cache locally in this scope (or otherwise keying the cache by option state) when:sudo_opt:is present.
# highlighted as a valid command when sudo itself would fail to find
# it (issue #595).
[[ $this_word == *':sudo_opt:'* ]] && local -a options_to_set=( ${options_to_set:#PATH_DIRS} )
_zsh_highlight_main__type "$arg" "$(( ! ${+seen_alias[$arg]} ))"
}
highlighters/main/main-highlighter.zsh:754
- Same as above: this
_zsh_highlight_main__typecall can still be affected by_zsh_highlight_main__type’s cache (which is not keyed by option state). If the command name was cached under PATH_DIRS earlier in the same prompt, removing PATH_DIRS fromoptions_to_sethere won’t change the cached result.
_zsh_highlight_main_highlighter_expand_path $arg
() {
[[ $this_word == *':sudo_opt:'* ]] && local -a options_to_set=( ${options_to_set:#PATH_DIRS} )
_zsh_highlight_main__type "$REPLY" 0
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| () { | ||
| # :sudo_opt: marks every word from a recognised precommand (sudo, | ||
| # env, nice, ...) up to and including its actual command word. Those | ||
| # precommands spawn their target via execvp(3)-style lookup, which | ||
| # (unlike the shell's own PATH_DIRS option) never searches $path for |
There was a problem hiding this comment.
Good catch, thanks -- that's a real bug. Fixed in 3d15f05: gave _zsh_highlight_main__type an explicit no_cache param, and added _zsh_highlight_main__type_maybe_no_pathdirs() which only bypasses the cache (and only shadows $options_to_set) when PATH_DIRS was actually present to remove, so it's a no-op for users without PATH_DIRS set. Also strengthened sudo-path_dirs.zsh to classify the same name twice in one buffer (plain, then sudo-prefixed) specifically to exercise this cache interaction -- confirmed it fails as expected (observes "command" instead of "unknown-token") against the pre-fix highlighter, and passes cleanly with the fix, via a real zsh 5.9 make test run.
Per Copilot review on zsh-users#987: _zsh_highlight_main__type's cache is keyed on the command name alone, so classifying a name once under PATH_DIRS (e.g. as a plain top-level command) would silently poison later lookups of that same name after sudo -- and vice versa -- regardless of the PATH_DIRS removal, since the cache lookup happens before $options_to_set is even consulted. Give _zsh_highlight_main__type an explicit no_cache parameter, and add _zsh_highlight_main__type_maybe_no_pathdirs(), a small wrapper that only shadows $options_to_set (and only bypasses the cache) when PATH_DIRS was actually present to remove -- so a user without PATH_DIRS set sees this codepath do nothing at all. Both call sites in the main word-classification block now go through this wrapper instead of duplicating the shadowing logic inline. Strengthened sudo-path_dirs.zsh to classify the same name twice in one buffer (plain, then sudo-prefixed) specifically to exercise this cache interaction, not just the PATH_DIRS removal in isolation. Verified via WSL zsh 5.9: make test passes (same 28 pre-existing TODO failures, zero new), and the strengthened test fails as expected (observes "command" instead of "unknown-token" on the sudo-prefixed occurrence) when run against the pre-fix highlighter.
Summary
sudo(8)(and every other entry in$precommand_options--env,nice,strace, etc.) resolves its target command viaexecvp(3)-style lookup, which never searches$pathfor a name containing a slash. That's a plain POSIXexec()property, unrelated to the shell. zsh'sPATH_DIRSoption is a shell-specific extension that makes the shell itself search$pathfor slash-containing names -- something none of these spawned-child precommands replicate.Because the highlighter applied the user's real
PATH_DIRSsetting uniformly to every command word,sudo foo/barwould get highlighted as a validcommandwheneverPATH_DIRSwas set and some$pathelement madefoo/barresolvable that way -- even thoughsudoitself would fail to find it and error out.Fix
:sudo_opt:already marks every word from a recognised precommand up to and including its actual target command word (per the existing code's own comment, this applies to any precommand in$precommand_options, not justsudo). This PR keys off that existing marker: for the two_zsh_highlight_main__typecalls in the alias/type-check block, if$this_wordcarries:sudo_opt:,PATH_DIRSis shadowed out of$options_to_setfor just that call via an anonymous-function scope, so it reverts automatically afterward and normal command-word classification everywhere else in the line is unaffected.Fixes #595.
Test plan
highlighters/main/test-data/sudo-path_dirs.zsh, reusing the exact fixture from the existingoption-path_dirs.zsh(afoo/bar/testing-issue-228executable made reachable only viaPATH_DIRS+ a$pathentry), but preceded bysudo.make test-- full suite passes. Same 28 pre-existing# TODO "issue #NNN"failures as baseline (unrelated to this change), zero new failures.main-highlighter.zsh(keeping only the new test) and confirmed it fails exactly as expected --bar/testing-issue-228observed ascommandinstead of the expectedunknown-token. Restored the fix and re-ran the full suite clean.option-path_dirs.zsh(the existing test for the non-sudo case) still passes unchanged --PATH_DIRScontinues to apply normally to a plain top-level command word; only the post-precommand word is affected.