Skip to content

fix(env): call fish built-ins with builtin in env.fish - #3353

Merged
fengmk2 merged 4 commits into
voidzero-dev:mainfrom
naokihaba:fix/fish-builtin-commands
Nov 5, 2026
Merged

fengmk2 merged 4 commits into
voidzero-dev:mainfrom
naokihaba:fix/fish-builtin-commands

Conversation

@naokihaba

Copy link
Copy Markdown
Collaborator

Context

When a user defines a fish function with the name of a built-in, env.fish calls the user function instead of the built-in. With a user count function, vp env use reports success, but the Node.js version does not change.

The fish documentation for function warns about this case. It says that a function with the name of a built-in often causes problems "in scripts written by others". env.fish is such a script.

This PR adds builtin to each built-in that env.fish calls: count, math, contains, complete, commandline, and source. builtin skips functions and runs only the built-in. #3123 did the same for Nushell with the % sigil.

This PR does not change the vp completion. clap_complete registers it with bare complete and commandline, so env.fish cannot protect it. fish's own completions also call bare commandline.

# fish 4.9.3, vp built from 09961cfa5
# $R is a temporary VP_HOME. A line that starts with > is a command. [exit N] is the exit status of the command above it.

$ VP_HOME=$R VP_SELF_SETUP_NO_MODIFY_PATH=1 ./target/debug/vp env setup --refresh
[exit 0]
$ sed 's/builtin //g' $R/env.fish > $R/env-old.fish
$ diff $R/env-old.fish $R/env.fish | grep -c '^>'
14

## env-old.fish
> function count; echo 0; end
[exit 0]
> source env-old.fish
[exit 0]
> set -gx VP_NODE_VERSION 18.20.0
[exit 0]
> vp env use 20.20.2
Installing Node.js v20.20.2...
Using Node.js v20.20.2 (resolved from 20.20.2)
[exit 0]
> echo VP_NODE_VERSION=$VP_NODE_VERSION
VP_NODE_VERSION=18.20.0
[exit 0]
> node --version
v18.20.0
[exit 0]

## env.fish
> function count; echo 0; end
[exit 0]
> source env.fish
[exit 0]
> set -gx VP_NODE_VERSION 18.20.0
[exit 0]
> vp env use 20.20.2
Using Node.js v20.20.2 (resolved from 20.20.2)
[exit 0]
> echo VP_NODE_VERSION=$VP_NODE_VERSION
VP_NODE_VERSION=20.20.2
[exit 0]
> node --version
v20.20.2
[exit 0]

User functions named like fish built-ins replaced them in env.fish.
A user count function made vp env use report success without setting
VP_NODE_VERSION.

Closes voidzero-dev#3128
@naokihaba naokihaba self-assigned this Nov 4, 2026
@naokihaba

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-04T15:04:02.507618Z 09961cf Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 09961cfa54

ℹ️ 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".

@naokihaba
naokihaba marked this pull request as ready for review November 4, 2026 15:41
@naokihaba
naokihaba requested review from fengmk2 and nekomoyi and removed request for nekomoyi November 4, 2026 15:41
@naokihaba

Copy link
Copy Markdown
Collaborator Author

Maybe we should wait for #3354 to be merged first and then do a final check on this PR just to be safe.

@naokihaba

Copy link
Copy Markdown
Collaborator Author

I merged main into this branch to get #3354, and then checked commit aa317daba on macOS with fish 4.9.3.

All 3 command_env_fish snapshot cases pass: setup, use, and shadowed_builtins. The 2 unit tests from #3354 also pass: test_parse_task_completion_lines and test_sanitize_task_current_prefix.

I also ran a manual check with the debug vp two times. The first run had no user functions. The second run had user functions that shadow count, math, contains, and source, as in #3128. The two runs gave the same result:

  • env.fish loads without errors.
  • vp env use and vp -C . env use set VP_NODE_VERSION, and vp env use --help does not change it.
  • The vpr completion does not show the comment keys // and # Scripts for docs #.
  • Each vpr completion candidate shows its task command as the description.
  • The input test\:u matches test:unit.
  • env.fish does not replace the user's count function.
$ builtin source <tmp>/env.fish
  -> status=0
$ vp env use 20.20.2 --no-install
Using Node.js v20.20.2 (resolved from 20.20.2)
  -> status=0
  VP_NODE_VERSION=20.20.2
$ vp env use --help >/dev/null
  -> status=0
  VP_NODE_VERSION=20.20.2 (expected: unchanged)
$ vp -C . env use 22.23.4 --no-install
Using Node.js v22.23.4 (resolved from 22.23.4)
  -> status=0
  VP_NODE_VERSION=22.23.4
$ complete -C 'vpr '
  build      vite build
  test:e2e   playwright test
  test:unit  vitest run
$ complete -C 'vpr test\:u'
  test:unit  vitest run
$ count a b c
  0

@fengmk2
fengmk2 merged commit fc9d5ba into voidzero-dev:main Nov 5, 2026
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User fish functions named like built-ins break the vp wrapper and vpr completion in env.fish

3 participants