From 95cb01d996c1458e037160e8bf25e520e6df3b88 Mon Sep 17 00:00:00 2001 From: LukasWodka Date: Thu, 30 Jul 2026 16:12:09 +0200 Subject: [PATCH 1/5] fix(install): match a quoted fish path, spaces included rc_lists_dir field-split fish_add_path arguments on whitespace. Since #434 started writing fish_add_path "$PREFIX", a prefix containing spaces became two fields, matched neither, and the installer appended a SECOND block directly beneath an existing line naming the same directory -- while reporting that it had added a PATH entry that was already there. The read path now parses quotes like the ownership reader further up the file already does: a quoted argument is one path, spaces included, while an unquoted line still splits so several bare paths on one line keep working. Single quotes are handled too, which the old code also missed. Verified in both directions: the two new harness cases fail against the current installer (29 passed / 2 failed) and pass with this change (31 passed / 0 failed). Found by Bugbot on the develop->staging promotion (cli#438). --- scripts/install.sh | 22 +++++++++++++++++++++- scripts/tests/install-verify.sh | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/scripts/install.sh b/scripts/install.sh index e1507fe..25b4823 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -648,7 +648,27 @@ rc_lists_dir() { { n = 0 if ($0 ~ /(^|[^A-Za-z_])fish_add_path([^A-Za-z_]|$)/) { - for (i = 1; i <= NF; i++) if ($i !~ /^-/ && $i !~ /fish_add_path/) cand[++n] = $i + # Parse quotes instead of field-splitting. We WRITE + # fish_add_path "$PREFIX", so a prefix containing spaces became + # two whitespace fields here and never matched -- the installer + # then appended a second block and claimed it had added a PATH + # entry that was already present. The sibling reader further up + # already keeps the remainder intact; this now agrees with it. + rest = $0 + sub(/^.*fish_add_path[[:space:]]*/, "", rest) + while (rest ~ /^-/) { sub(/^-[^[:space:]]*[[:space:]]*/, "", rest) } + sub(/[[:space:]]+$/, "", rest) + q = substr(rest, 1, 1) + if (q == "\"" || q == "\047") { + # Quoted: everything up to the closing quote is ONE path, + # spaces included. + body = substr(rest, 2) + close_at = index(body, q) + cand[++n] = (close_at > 0) ? substr(body, 1, close_at - 1) : body + } else { + # Unquoted: several bare paths may share the line. + n = split(rest, cand, /[[:space:]]+/) + } } else if ($0 ~ /(^|[^A-Za-z_])[Pp][Aa][Tt][Hh][+]?=/) { value = $0 sub(/^[^=]*=/, "", value) diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index 1b42ca7..a82f581 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -469,6 +469,31 @@ for spec in "zsh:.zshrc:export PATH=" "fish:.config/fish/config.fish:fish_add_pa drop_sandbox done +# -- 16. a fish prefix containing spaces is recognised as already listed ------ +# We WRITE `fish_add_path "$PREFIX"`, so rc_lists_dir must parse the quotes +# rather than field-split: it used to see two whitespace fields, match neither, +# append a second block, and report adding an entry that was already present. +for quote_style in double single; do + make_sandbox yes + RC="$HOMEDIR/.config/fish/config.fish" + mkdir -p "$(dirname "$RC")" + SPACED="$SBX/my tools/bin" + mkdir -p "$SPACED" + if [ "$quote_style" = double ]; then + printf 'fish_add_path "%s"\n' "$SPACED" > "$RC" + else + printf "fish_add_path '%s'\n" "$SPACED" > "$RC" + fi + before=$(cat "$RC") + FAKE_SHELL=/bin/fish COSIGN_RESULT=0 run_installer_at "$SPACED" >/dev/null 2>&1 + if [ "$(cat "$RC")" = "$before" ]; then + ok "fish: $quote_style-quoted prefix with spaces seen as already listed" + else + bad "fish: $quote_style-quoted prefix with spaces not matched (rc rewritten)"; sed 's/^/ /' "$RC" + fi + drop_sandbox +done + echo echo "install-verify: $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ] From 4152d8648e1dcd78ad64b3f09a1f37db6529d644 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:20:58 +0200 Subject: [PATCH 2/5] fix(install): take the FIRST fish_add_path, not the last A greedy /^.*fish_add_path/ strips through the LAST occurrence on the line, so an inline comment mentioning fish_add_path left the comment text as the path and missed the real argument -- reintroducing the exact bug this branch fixes. index() takes the first occurrence instead. Also restores the leading-whitespace strip that the greedy regex used to consume: without it substr() left ' "/path"', so the quote check saw a space and fell back to field-splitting, breaking the spaced-path fix. Caught by re-running the whole case set rather than only the new one. Bugbot, cli#439. --- scripts/install.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index 25b4823..bf24004 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -654,8 +654,15 @@ rc_lists_dir() { # then appended a second block and claimed it had added a PATH # entry that was already present. The sibling reader further up # already keeps the remainder intact; this now agrees with it. - rest = $0 - sub(/^.*fish_add_path[[:space:]]*/, "", rest) + # index() finds the FIRST occurrence. A greedy /^.*fish_add_path/ + # would strip through the LAST one, so an inline comment that + # mentions fish_add_path would leave the comment text as the + # "path" and miss the real argument -- reintroducing this very + # bug (Bugbot, cli#439). The old field-split avoided it by + # filtering the command token out instead. + at = index($0, "fish_add_path") + rest = substr($0, at + 13) + sub(/^[[:space:]]+/, "", rest) while (rest ~ /^-/) { sub(/^-[^[:space:]]*[[:space:]]*/, "", rest) } sub(/[[:space:]]+$/, "", rest) q = substr(rest, 1, 1) From ebc2c52ef0bab84c6c9dea3cdcd456e83494739b Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:21:00 +0200 Subject: [PATCH 3/5] fix(install): take the FIRST fish_add_path, not the last A greedy /^.*fish_add_path/ strips through the LAST occurrence on the line, so an inline comment mentioning fish_add_path left the comment text as the path and missed the real argument -- reintroducing the exact bug this branch fixes. index() takes the first occurrence instead. Also restores the leading-whitespace strip that the greedy regex used to consume: without it substr() left ' "/path"', so the quote check saw a space and fell back to field-splitting, breaking the spaced-path fix. Caught by re-running the whole case set rather than only the new one. Bugbot, cli#439. --- scripts/tests/install-verify.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index a82f581..4881088 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -494,6 +494,23 @@ for quote_style in double single; do drop_sandbox done +# -- 17. an inline comment that mentions fish_add_path must not defeat the match +# A greedy strip through the LAST `fish_add_path` on the line would leave the +# comment text as the "path" and miss the real argument (Bugbot, cli#439). +make_sandbox yes +RC="$HOMEDIR/.config/fish/config.fish" +mkdir -p "$(dirname "$RC")" +printf 'fish_add_path "%s" # set by fish_add_path\n' "$SBX/s1" > "$RC" +mkdir -p "$SBX/s1" +before=$(cat "$RC") +FAKE_SHELL=/bin/fish COSIGN_RESULT=0 run_installer_at "$SBX/s1" >/dev/null 2>&1 +if [ "$(cat "$RC")" = "$before" ]; then + ok "fish: inline comment naming fish_add_path does not break the match" +else + bad "fish: inline comment naming fish_add_path broke the match (rc rewritten)"; sed 's/^/ /' "$RC" +fi +drop_sandbox + echo echo "install-verify: $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ] From 7122b5de5fe105fa6faae0e1b90af702a0987266 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:27:50 +0200 Subject: [PATCH 4/5] fix(install): tokenise the fish argument list properly Replaces three rounds of patching with one quote-aware tokenizer, because each patch fixed its own case and broke or missed another: * greedy .* strip lost the real argument to an inline comment * index() alone dropped the leading space, so the quote check failed and spaced paths regressed * taking only the first quoted argument missed the prefix when fish_add_path lists several directories The loop now walks the argument list: quoted tokens are one path (spaces included), bare tokens split on whitespace, flags are skipped, and a trailing # comment ends parsing. 18 direct variants and 33 harness cases green. Bugbot, cli#439. --- scripts/install.sh | 50 ++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index bf24004..12d32e7 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -654,27 +654,39 @@ rc_lists_dir() { # then appended a second block and claimed it had added a PATH # entry that was already present. The sibling reader further up # already keeps the remainder intact; this now agrees with it. - # index() finds the FIRST occurrence. A greedy /^.*fish_add_path/ - # would strip through the LAST one, so an inline comment that - # mentions fish_add_path would leave the comment text as the - # "path" and miss the real argument -- reintroducing this very - # bug (Bugbot, cli#439). The old field-split avoided it by - # filtering the command token out instead. + # Tokenise the argument list, honouring quotes. Three things + # this has to get right, each of which broke a previous + # attempt (cli#439): + # * FIRST occurrence of the command, via index() -- a greedy + # /^.*fish_add_path/ strips through an inline comment that + # mentions it and loses the real argument; + # * quoted arguments are ONE path, spaces included, because we + # write fish_add_path "$PREFIX"; + # * fish_add_path takes MULTIPLE directories, so parsing must + # continue past the first closing quote. at = index($0, "fish_add_path") rest = substr($0, at + 13) - sub(/^[[:space:]]+/, "", rest) - while (rest ~ /^-/) { sub(/^-[^[:space:]]*[[:space:]]*/, "", rest) } - sub(/[[:space:]]+$/, "", rest) - q = substr(rest, 1, 1) - if (q == "\"" || q == "\047") { - # Quoted: everything up to the closing quote is ONE path, - # spaces included. - body = substr(rest, 2) - close_at = index(body, q) - cand[++n] = (close_at > 0) ? substr(body, 1, close_at - 1) : body - } else { - # Unquoted: several bare paths may share the line. - n = split(rest, cand, /[[:space:]]+/) + while (length(rest) > 0) { + sub(/^[[:space:]]+/, "", rest) + if (length(rest) == 0) break + ch = substr(rest, 1, 1) + if (ch == "#") break # trailing comment + if (ch == "\"" || ch == "\047") { + body = substr(rest, 2) + close_at = index(body, ch) + if (close_at > 0) { + cand[++n] = substr(body, 1, close_at - 1) + rest = substr(body, close_at + 1) + } else { # unterminated quote + cand[++n] = body + rest = "" + } + } else { + sp = match(rest, /[[:space:]]/) + tok = (sp > 0) ? substr(rest, 1, sp - 1) : rest + rest = (sp > 0) ? substr(rest, sp) : "" + if (tok !~ /^-/) cand[++n] = tok # skip flags + } } } else if ($0 ~ /(^|[^A-Za-z_])[Pp][Aa][Tt][Hh][+]?=/) { value = $0 From c73e3664c16a4382009ed19a03cb110092aeb887 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:27:51 +0200 Subject: [PATCH 5/5] fix(install): tokenise the fish argument list properly Replaces three rounds of patching with one quote-aware tokenizer, because each patch fixed its own case and broke or missed another: * greedy .* strip lost the real argument to an inline comment * index() alone dropped the leading space, so the quote check failed and spaced paths regressed * taking only the first quoted argument missed the prefix when fish_add_path lists several directories The loop now walks the argument list: quoted tokens are one path (spaces included), bare tokens split on whitespace, flags are skipped, and a trailing # comment ends parsing. 18 direct variants and 33 harness cases green. Bugbot, cli#439. --- scripts/tests/install-verify.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index 4881088..58d30a1 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -511,6 +511,24 @@ else fi drop_sandbox +# -- 18. the prefix is the SECOND quoted arg on a multi-path fish line -------- +# fish_add_path accepts several directories. Parsing that stopped at the first +# closing quote missed a later one, appended a redundant block, and reported a +# PATH add that was already there (Bugbot, cli#439). +make_sandbox yes +RC="$HOMEDIR/.config/fish/config.fish" +mkdir -p "$(dirname "$RC")" +mkdir -p "$SBX/other" "$SBX/s1" +printf 'fish_add_path "%s" "%s"\n' "$SBX/other" "$SBX/s1" > "$RC" +before=$(cat "$RC") +FAKE_SHELL=/bin/fish COSIGN_RESULT=0 run_installer_at "$SBX/s1" >/dev/null 2>&1 +if [ "$(cat "$RC")" = "$before" ]; then + ok "fish: prefix as the second quoted arg is seen as already listed" +else + bad "fish: second quoted arg was not matched (rc rewritten)"; sed 's/^/ /' "$RC" +fi +drop_sandbox + echo echo "install-verify: $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ]