fix(install): match a quoted fish path, spaces included

[LukasWodka]

Closes the last open Bugbot finding from today's staging promotion (cli#438). It's a regression introduced by #434, so worth fixing before it reaches a customer.

The bug

rc_lists_dir field-split fish_add_path arguments on whitespace:

for (i = 1; i <= NF; i++) if ($i !~ /^-/ && $i !~ /fish_add_path/) cand[++n] = $i

#434 started writing fish_add_path "$PREFIX". So a prefix containing spaces arrived here as two fields — "/opt/my and tools/bin" — matched neither, and the installer appended a second block directly beneath a line already naming that exact directory, while printing that it had added a PATH entry. Self-inconsistent within one change: the write path quotes, the read path splits.

Notably the ownership reader further up the same file (the ^[[:space:]]*fish_add_path[[:space:]] branch) already keeps the remainder intact and is space-safe — so one reader was right and the other wasn't.

The fix

The fish branch now parses quotes rather than field-splitting:

• a quoted argument is one path, spaces included (this is the form we write)
• an unquoted line still splits on whitespace, so fish_add_path /opt/a /opt/b keeps working
• single quotes are handled too — the old code missed those as well
• leading flags (-g, -p, …) are still skipped

Verified in both directions

The two new harness cases fail against the current installer and pass with the change — I checked that rather than assuming they cover it:

unfixed installer:  install-verify: 29 passed, 2 failed
with this change:   install-verify: 31 passed, 0 failed

The pre-fix failure output shows the bug exactly: the installer's own block appended immediately below an existing fish_add_path for the same directory.

Also exercised directly across ten variants (quoted/unquoted, single/double, spaces, multiple paths, -g flag, comment lines, trailing-slash normalisation, and negative cases) — all correct, with the two space cases flipping from unmatched to matched and nothing else changing.

sh -n, dash -n, bash -n and shellcheck --shell=sh all clean.

---

Note

Low Risk
Scoped to installer awk PATH detection for fish shells; behavior change only affects idempotent rc writes and messaging, with targeted harness coverage.

Overview
Fixes a #434 regression where the installer writes fish_add_path "$PREFIX" but rc_lists_dir split arguments on whitespace, so prefixes with spaces were not seen as already listed and a duplicate PATH block could be appended.

rc_lists_dir in install.sh now tokenizes fish_add_path arguments with a small quote-aware parser (first fish_add_path via index(), not a greedy strip; double/single quotes; multiple paths per line; flags skipped; stops at # comments), aligned with the ownership reader above it.

install-verify.sh adds cases 16–18: spaced prefixes in double/single quotes, inline comments mentioning fish_add_path, and the install prefix as the second quoted argument on a multi-path line.

^{Reviewed by Cursor Bugbot for commit c73e366. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

bugbot run

[LukasWodka]

👋 Heads-up — Code review queue is at 34 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

• averaging-service#253 — feat(blob_store): write averaged global weights through blob_store (#1162) · author: @saadqbal · no reviewer assigned
• backend#1146 — docs(rfc): platform cost & autoscaling architecture · author: @saadqbal · reviewer: @LukasWodka, @saqlainsyed007, @aptracebloc
• backend#1201 — docs(rfc): Data spaces & federated dataset composition (draft) · author: @LukasWodka · reviewer: @saadqbal
• backend#1217 — docs(rfc): Dataset integrity & score reproducibility (draft) · author: @LukasWodka · reviewer: @saadqbal
• backend#1261 — RFC-BACKEND-0008: board & process reset (decision RFC — for markup) · author: @LukasWodka · reviewer: @saadqbal
• backend#1284 — docs(deploy): backend deploy runbook skeleton (#1263) · author: @LukasWodka · no reviewer assigned
• backend#1332 — feat(check-model): allow KNN/GaussianProcess on a single-edge owned dataset (#923 follow-up) · author: @divyasinghds · no reviewer assigned
• backend#1353 — chore(ci): baseline historical gitleaks findings + wire caller · author: @LukasWodka · no reviewer assigned
• backend#1361 — feat(tasks): bound ExperimentQueue growth for PAUSED experiments (backend#1351) · author: @saqlainsyed007 · reviewer: @saadqbal
• backend#1367 — feat(blob_store): migrate check_model_utils (CheckModelMixin) to blob_store (#1162) · author: @saadqbal · no reviewer assigned

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

[shujaatTracebloc]

Reviewed and verified locally.

Diagnosis is correct. rc_lists_dir's fish branch field-split fish_add_path arguments on whitespace, while since #434 we write fish_add_path "$PREFIX". A prefix with spaces arrived as two fields, matched nothing, and the installer appended a duplicate block while reporting it had added the entry — the write path quoted, the read path split.

Fix is sound. The quote-aware parse (double + single quotes as one path, unquoted still whitespace-split, leading flags skipped) now agrees with the space-safe sibling reader in strip_tb_path_block. Downstream, candidates still get quotes/parens stripped and trailing slashes normalized, so the #433 component-match guarantee is preserved.

Verified in both directions myself:

• With the fix: install-verify: 31 passed, 0 failed.
• Reverted to the old field-split one-liner: 29 passed, 2 failed, and the failure output is exactly the bug — a second fish_add_path block appended below an existing line for the same directory. So the two new cases genuinely exercise the regression.
• Ran an extra adversarial matrix directly against the awk (14 cases): quoted/unquoted, single/double, -g and --path flags, trailing-slash normalization on both sides, leading indent, plus negatives (/opt/tb vs /opt/tb2 substring, comment line, bare mention) — all correct.

CI green across the board; Cursor Bugbot completed with no findings on 95cb01d. LGTM.

[LukasWodka]

Correct — fixed, and it caught a second problem on the way.

The greedy /^.*fish_add_path/ strips through the last occurrence, so fish_add_path "/opt/tb/bin" # set by fish_add_path left set by fish_add_path-adjacent text as the "path" and missed the real argument — reintroducing precisely the bug this branch fixes. As you noted, the old field-split avoided it by filtering the command token instead. Now index($0, "fish_add_path") takes the first occurrence.

The second problem, which my own re-test caught: substr() doesn't consume the whitespace the greedy regex used to, so rest began with "/path". The quote check then saw a space instead of a quote, fell through to field-splitting, and broke the spaced-path fix this PR exists for — two of twelve cases regressed. Restored the explicit leading-whitespace strip.

Worth naming: I only found that because I re-ran the whole case set rather than just the new case. Fixing one edge case and re-testing only that edge case would have shipped a PR that no longer did its own job.

Verified in both directions again:

current branch (greedy):  install-verify: 31 passed, 1 failed
with this change:         install-verify: 32 passed, 0 failed

Plus all twelve direct variants green — the comment case, both quote styles with spaces, multi-path lines, -g flag, leading whitespace, trailing-slash normalisation, and three negatives.

sh -n, dash -n, bash -n clean.

bugbot run

[LukasWodka]

Right again — and I've stopped patching and written the thing properly.

You're correct that fish_add_path takes multiple directories and my quoted branch stopped at the first closing quote, so a prefix appearing as the second quoted argument was missed. The old field-split did see it.

That's three rounds on one small function, and each patch fixed its own case while breaking or missing another:

attempt	fixed	broke / missed
field-split → quote-aware	spaced prefixes	greedy .* lost the argument to an inline comment
greedy → index()	the comment case	dropped the leading space, so spaced prefixes regressed
first quoted arg only	—	missed the prefix as a later argument

So it is now a single tokenizer rather than accumulated special cases: walk the argument list; a quoted token is one path (spaces included) and parsing continues past its closing quote; bare tokens split on whitespace; flags are skipped; a trailing # ends parsing. Unterminated quotes still yield the remainder rather than nothing.

Verified: 18 direct variants (both quote styles, multi-arg quoted/bare/mixed, flags mid-list, inline comments, unterminated quote, trailing-slash normalisation, and four negatives including "a flag must never be a candidate") plus the harness, checked in both directions:

branch as-is:      install-verify: 32 passed, 1 failed
with the tokenizer: install-verify: 33 passed, 0 failed

The lesson I should have applied two rounds ago: when a parser keeps failing on new input shapes, the input needs parsing, not another regex. Three review rounds is the cost of not doing that first.

bugbot run

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit c73e366. Configure here.}

[LukasWodka]

/fr-pass