feat(#323): fish-shell support in post-install PATH suggestion

[abrauchli]

Enhancement PR

---

Linked Issue

Closes #323

Issue #323 carries the approved-enhancement label (approved by @trek-e on 2026-06-01:
"fish-shell support fits the existing mode-based projection and rc-file list patterns
as additive entries; non-breaking").

---

What this enhancement improves

The post-install PATH-suggestion seam in bin/install.js (maybeSuggestPathExport)
and its projection in shell-command-projection (projectPathActionProjection, persist
mode). Two additive changes, both scoped to existing functions:

1. fish-native suggestion. The "not on your PATH" warning now lists a fish line
   alongside zsh/bash, using fish_add_path instead of the inert export PATH=….
2. fish-aware coverage detection. A new homePathCoveredByFishConfig() suppresses the
   false-positive warning when fish's own config already covers the directory — parallel to
   the existing homePathCoveredByRc() for sh-style shells.

Before / After

Before — a fish user whose npm global bin is not on PATH sees only inert sh commands,
and a fish user whose fish_user_paths already covers it gets a false-positive warning on
every install (fish has no sh-style rc file, so homePathCoveredByRc never sees it):

⚠ /home/<user>/.local/share/nvm/<v>/bin is not on your PATH.
  Add it with one of:
    zsh:  echo 'export PATH="…:$PATH"' >> ~/.zshrc
    bash: echo 'export PATH="…:$PATH"' >> ~/.bashrc

After — the suggestion list includes a copy-paste-correct, fish-native line, and the
warning is suppressed when fish config already covers the dir:

⚠ /home/<user>/.local/share/nvm/<v>/bin is not on your PATH.
  Add it with one of:
    zsh:  echo 'export PATH="…:$PATH"' >> ~/.zshrc
    bash: echo 'export PATH="…:$PATH"' >> ~/.bashrc
    fish: fish_add_path '/home/<user>/.local/share/nvm/<v>/bin'   ← new

# …or, when fish_user_paths already covers it:
⚠ …'s directory is already on your PATH via fish's universal variables —
  open a new fish session (or run exec fish).

No change for bash/zsh/PowerShell/cmd/Git-Bash users — their entries and command strings
are byte-for-byte unchanged; the fish entry is additive and the fish detector only narrows
the set of cases that warn.

How it was implemented

• src/shell-command-projection.cts — append a third shellActions entry
  (label: 'fish', command: fish_add_path '<dir>') to the persist branch of
  projectPathActionProjection(). The directory is single-quoted with the same POSIX
  single-quote escaping (escapeSingleQuotedShellLiteral) as the zsh/bash siblings — '\''
  is also a valid escaped single quote in fish's unquoted context. (Source of truth is the
  .cts; the gitignored gsd-core/bin/lib/*.cjs is built by npm run build:lib.)
• bin/install.js — add homePathCoveredByFishConfig(globalBin, homeDir[, fishConfigDir]),
  call it from maybeSuggestPathExport() right after the rc check, and export it for tests.
  Two side-effect-free detection routes (no fish subprocess is spawned):
  1. Universal-variable store ~/.config/fish/fish_variables. fish serializes
     fish_user_paths with full_escape: every byte outside [A-Za-z0-9/_] becomes \xHH
     (space → \x20, - → \x2d, . → \x2e, $ → \x24, unicode → \uXXXX) and list
     elements are joined by the literal 4-char token \x1e (not a raw 0x1e byte). The
     detector splits on \x1e, decodes the escapes, and compares each as an absolute literal.
  2. config.fish — fish_add_path …, set -gx PATH …, set -Ux fish_user_paths …
     lines (plain shell tokens; $HOME/${HOME}/~ expanded, $VAR tokens skipped).
     Honours $XDG_CONFIG_HOME and always also checks ~/.config/fish.

Design note — the installer never mutates fish config. Consistent with the
existing seam, this PR is suggest-only: just as the zsh/bash entries print an
echo … >> ~/.zshrc command for the user to run rather than writing the rc file,
the fish entry prints fish_add_path '<dir>' for the user to run. fish_add_path
is fish's own supported API for this (≥ fish 3.2); the installer does not write
fish_variables (hand-editing that internal universal-variable store is
discouraged by fish). The only contact with fish_variables/config.fish is the
read-only coverage probe above — no fish subprocess, no writes.

Testing

How I verified the enhancement works

Automated (node:test, no external framework):

• tests/install-path-detection.test.cjs — fish detection/suppression/emission: uvar-store
  parsing with real fish escaping, a dot/hyphen/space/$-literal decode regression,
  all three config.fish routes, commented-out lines, relative-segment guard, unreadable-file
  fault injection, and suppression + emission through maybeSuggestPathExport.
• tests/bug-3441-path-action-projection.test.cjs — updated the persist projection length
  assertion (2 → 3) and added the fish entry's escaping assertions.
• Result: 34 tests across the two files pass; full install suite 723/723 (run with
  --test-concurrency=2; at full parallelism an unrelated migration test flakes on an
  installer-subprocess exit). ESLint: 0 errors. Shell-projection drift lint: clean.

Manual, against real fish 3.7.0 (the encoding details above were reverse-engineered from
od -c of real fish_variables output, then locked into the fixtures):

• Ran the exact fish_add_path '<dir>' the installer suggests, verbatim in fish, and
  confirmed fish puts the dir on $PATH.
• Adversarial round-trip for directory names containing a single quote, space, $, *,
  backtick, ;&, tab, and unicode (é) — all stored by fish and detected correctly. This
  surfaced and fixed one real bug: a literal $ in a decoded uvar entry was wrongly rejected
  by the config.fish $-guard; the uvar route now compares decoded entries as absolute
  literals.
• Verified the documented nvm caveat / parity: on a real nvm (~/.nvm) and a real
  nvm.fish install, the managed bin is on PATH via dynamic startup activation, not
  fish_user_paths, so the detector correctly returns false — identical to how bash/zsh
  nvm users are treated today.

Platforms tested

• macOS
• Windows (including backslash path handling) — N/A: persist/fish detection is POSIX-only;
  the Windows branch and its command strings are untouched (the install-path-detection
  suite is skipped on Windows)
• Linux
• N/A (not platform-specific)

Runtimes tested

• Claude Code
• Gemini CLI
• OpenCode
• Other: ___
• N/A (not runtime-specific — this is the npm installer's post-install output)

---

Scope confirmation

• The implementation matches the scope approved in the linked issue — no additions or removals
• If scope changed during implementation, I updated the issue and got re-approval before continuing
  (no scope change; the proposal's two changes, A and B, are implemented as approved)

Implementation note vs. the proposal: the proposal assumed fish_variables stores
absolute, \x1e-separated, unescaped values. Real fish 3.7.0 escapes the value
(full_escape) and uses a literal \x1e token as the separator, so the detector adds a
faithful decoder. This is within the approved scope (route 1 still parses fish_variables);
it is called out here for reviewer awareness.

---

Documentation

• Updated the relevant file(s) under docs/ — docs/how-to/install-on-your-runtime.md
  gains an "… is not on your PATH" section documenting the fish suggestion and the
  config-coverage suppression (satisfies the Changed-changeset Docs-Required gate).
• All docs/ content added in this PR is written in English
• If genuinely no user-facing docs impact … (N/A — docs updated)

Checklist

• Issue linked above with Closes #323
• Linked issue has the approved-enhancement label
• Changes are scoped to the approved enhancement — nothing extra included
• All existing tests pass (npm test)
• New or updated tests cover the enhanced behavior
• .changeset/ fragment added — pending the assigned PR number
  (npm run changeset -- --type Changed --pr <NNN> --body "…")
• No unnecessary dependencies added (Node built-ins only)

Breaking changes

None for bash/zsh/PowerShell/cmd/Git-Bash users — those entries and command strings are
unchanged. For fish users: a previously-emitted false-positive warning stops when
fish_user_paths/config.fish already covers the global bin (a silent improvement), and the
suggestion list grows by one line. No file formats or hook contracts change; the persist
shellActions IR gains one entry, and the only consumer iterates it.

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

[abrauchli]

Note for reviewers: this overlaps with #721 (same issue #323), which was opened first. The two are near-identical except in how fish_variables is parsed. I verified against real fish 3.7.0 on Linux that fish serializes fish_user_paths with full_escape (.→\x2e, -→\x2d, etc.) and a literal 4-char \x1e separator — this PR decodes that; #721 splits on a raw 0x1e byte and compares un-escaped, so its uvar-store detection misses any realistic (dot-containing) path. Details and the od -c evidence are in #721 (comment).

I have no preference on which PR merges as long as the correct decoder ships — equally happy to see #721 fixed and merged instead. Maintainer's call.

[trek-e]

Approve — accepted implementation for #323

This is the implementation we're taking for #323; the overlapping #721 has been closed because its fish_variables decoder was incorrect against real fish. Thanks @abrauchli — and for the good citizenship in cross-linking the duplicate and flagging #721's parsing bug with od -c evidence rather than letting two near-identical PRs sit silently.

Verified locally (checkout of the branch)

Check	Result
npm run build:lib (tsc)	✓
node --test on install-path-detection + bug-3441-path-action-projection	34/34 pass
npx eslint .	0 errors
lint:changeset	✓ (type: Changed, pr: 727)
lint:docs (base next)	✓ docs updated
lint:pr-checks	✓

Correctness — the fish_variables decoder is right

Verified against fish's own source (fish-shell/fish-shell, src/env_universal_common.rs): UVAR_ARRAY_SEP ('\x1e') is written through full_escape, so list elements land in the file as the literal 4-char token \x1e, and is_universal_safe_to_encode_directly keeps only [A-Za-z0-9/_] literal (so .→\x2e, -→\x2d, space→\x20). This PR splits on the literal token and decodes those escapes before comparing — matching the real format — and its fixtures (fishEncodeUniversalList) reproduce that encoding rather than a fabricated one. The dot/hyphen/space and literal-$ regression cases give real negative proof.

Security

Read-only parsing of the user's own fish config (try/catch around every read, no subprocess, no writes); decoded values feed only path-equality comparison — no exec/traversal/injection sink. The suggested fish_add_path '<dir>' is a printed string the installer never runs, single-quote-escaped consistently with the zsh/bash siblings.

Non-blocking notes (optional, not gating this approval)

• The fish coverage check runs regardless of the user's actual shell, so a non-fish user who happens to have a config.fish covering the dir would see the "open a new fish session" note. Harmless and consistent with the existing homePathCoveredByRc cross-shell behavior — flagging only for awareness.
• #721 carried a nice win32 negative-projection test (asserting no fish action is projected on win32). Worth porting here in a follow-up to lock that in, if you're inclined.

LGTM.

[trek-e]

Requesting changes — superseding my earlier approval (documentation style)

To be transparent: I approved this earlier on code/test/security grounds, all of which still stand (decoder verified against fish source, 34/34 tests, eslint clean, security clean). But documentation is a required, blocking part of a feature/enhancement here, and it must be written in the repo's house documentation style — and the doc in this PR isn't. So this flips to request-changes on that one axis. The code is not the problem; the prose placement is.

Blocker — how-to guide contains explanation/reference content

GSD Core's docs are organised by Diátaxis (docs/USER-GUIDE.md line 1: "GSD Core's documentation is organised by Diataxis"), with how-to/, reference/, explanation/, and tutorials/ as distinct quadrants. A how-to guide must stay in the action / application quadrant: direct the working reader, address real-world complexity with conditionals, and not describe internal mechanism — that is reference/explanation material and belongs in those quadrants.

The new section in docs/how-to/install-on-your-runtime.md crosses that boundary:

"The installer also suppresses this warning when your shell already covers the directory — for fish that means an existing fish_user_paths entry (in ~/.config/fish/fish_variables) or a fish_add_path / set -gx PATH line in ~/.config/fish/config.fish."

The bolded part describes how the installer's detection works internally (which files it probes) — cognition, not action. Likewise the inline code-comment # fish — persists via fish's universal variables is a "why" (explanation) embedded in a how-to.

What's already right (keep it)

• Correct quadrant: the content answers "How do I deal with the 'not on your PATH' warning?" → Action + Application → how-to. ✅
• Good conditional flow: "If the directory is already covered but the warning still mentions reopening your shell, open a new session (exec fish)" is exactly how a how-to should handle real-world complexity. ✅
• One concrete command, problem-framed heading. ✅

How to fix

Keep the how-to action-only. Trim the internal-mechanism clause to a directive, e.g.:

If the directory is already on your PATH but the installer still warns, open a new fish session (exec fish) to pick it up.

If the detection mechanism (the fish_variables / config.fish probe) is worth documenting, that's genuinely useful — but it belongs in the explanation quadrant. There's no installer page under docs/explanation/ today, so either drop the mechanism detail here, or add a short docs/explanation/-class note and link to it from the how-to (See [why the installer stops warning](…)). Don't inline it in the how-to.

The tool we used to assess this — so you can self-check

We evaluated this with the writing-documentation-with-diataxis skill (a Diátaxis reviewer). Its core mechanism is the Diátaxis compass — two questions that place any doc in exactly one of four quadrants:

1. Action or Cognition? (doing vs understanding)
2. Acquisition or Application? (learning vs working)

	Acquisition	Application
Action	Tutorial	How-to guide
Cognition	Explanation	Reference

The boundary rule it enforces: "Is there reference/explanation detail in your how-to? Link to it instead." A working reader following a how-to doesn't need to know which dotfiles the installer reads — they need to know what to do. Reference: https://diataxis.fr (the how-to and explanation pages there are the relevant ones).

Everything else is good to go — fix the doc placement and I'll re-approve.

[trek-e]

Summary

feat(#323): fish-shell support in post-install PATH suggestion makes two additive changes to the post-install PATH seam: (A) a fish-native fish_add_path '<dir>' entry in the persist branch of projectPathActionProjection() (src/shell-command-projection.cts), and (B) a side-effect-free homePathCoveredByFishConfig() detector in bin/install.js that suppresses the false-positive "not on your PATH" warning when fish's fish_user_paths/config.fish already covers the directory.

The code, tests, and security posture are solid and I independently re-verified them. This review nonetheless requests changes on one axis only — documentation — because the Diátaxis blocker @trek-e raised on 2026-06-06 is still unaddressed: HEAD is the same commit (256c14a8) that review was submitted against, and the offending how-to text is present verbatim.

Classification & gate compliance

• Track: Enhancement. Linked issue #323 carries approved-enhancement (RULESET.CONTRIB.CLASSIFY.enhancement=requires approved-enhancement before implementation). ✅
• Issue link: Closes #323 present (CI.GATE.issue-link-required). ✅
• ADR gate (enhancement): ADR-0009 (Shell Command Projection Module) governs the seam being touched. The change adds one entry to the existing shellActions IR list — additive use within the documented contract, not a change to the module boundary/decision the ADR records — so no signed ADR addendum is required. ✅
• Documentation gate (HARD BLOCKER): ❌ — see blocker below. A how-to was added but mixes quadrants.
• Branch / commits / changeset: feat/323-fish-path-suggestion ✅; conventional commits ✅; .changeset/witty-jays-jump.md is well-formed (type: Changed, pr: 727), CHANGELOG.md untouched ✅ (PRED.k320.rule).
• Scope: one concern, no incidental variable renames (RULESET.PR-SCOPE.one-concern-per-pr). ✅
• External-tool gate (fish): fish_add_path '<dir>' matches fish's documented, dedup-safe persistent-PATH API (fish ≥3.2; https://fishshell.com/docs/current/cmds/fish_add_path.html — modifies the $fish_user_paths universal variable). Detection is read-only file parsing with no fish subprocess, no process management, no output scraping. No hacks. ✅

Functional checklist (exercised on the PR branch, HEAD 256c14a)

• ✅ npm run build:lib (tsc) clean; fish entry compiled into bin/lib/shell-command-projection.cjs.
• ✅ node --test tests/install-path-detection.test.cjs tests/bug-3441-path-action-projection.test.cjs → 34/34 pass.
• ✅ A1 — fish suggestion line emitted: bug-3441 asserts shellActions[2].label === 'fish', command starts fish_add_path , contains no export, and single-quote escaping round-trips (fish_add_path '/tmp/O'\''Neil/bin').
• ✅ A2 — projection length updated 2→3.
• ✅ B1 — uvar-store route: detects fish_user_paths, decodes real fish full_escape (\x2e/\x20/\x1e literal token) and a literal-$ entry.
• ✅ B2 — config.fish routes: fish_add_path, set -gx PATH, set -Ux fish_user_paths; commented lines ignored; relative-segment guard; unreadable-file fault injection swallowed.
• ✅ Suppression + emission verified end-to-end through maybeSuggestPathExport.

Findings by severity

Blocker

1. Diátaxis: the how-to contains explanation/reference content (unchanged since @trek-e's request-changes). docs/how-to/install-on-your-runtime.md still has:

   • the inline code comment # fish — persists via fish's universal variables (a "why" — explanation), and
   • "The installer also suppresses this warning when your shell already covers the directory — for fish that means an existing fish_user_paths entry (in ~/.config/fish/fish_variables) or a fish_add_path / set -gx PATH line in ~/.config/fish/config.fish." (internal detection mechanism — reference/explanation).

   A how-to lives in the Action / Application quadrant and must direct the working reader, not describe internal mechanism. Fix: keep the how-to action-only — trim to a directive such as "If the directory is already on your PATH but the installer still warns, open a new fish session (exec fish) to pick it up." If the fish_variables/config.fish probe is worth documenting, put it in an explanation-quadrant note and link to it from the how-to; don't inline it. Self-check with the /writing-documentation-with-diataxis skill (the Diátaxis compass: Action-vs-Cognition × Acquisition-vs-Application). Reference: https://diataxis.fr/. Per RULESET.PR-FLOW.docs-by-changeset-type, a Changed fragment requires docs, and the docs must be in-house Diátaxis style — so this is a request-changes blocker, not a nit.

Minor

2. No fast-check property test for the fish-escape decoder. decodeFishUniversalValue() is a parsing/transformation routine; RULESET.TESTS.property-based-testing=modules implementing parsing / transformation / … must include at least one fast-check (fc) property test. The example-based coverage is good (dot/hyphen/space/$/unicode), but a round-trip property (decode(fishEncode(p)) === p over arbitrary path strings) would lock the bijection. The sibling homePathCoveredByRc also lacks one, so this is non-gating — but worth adding while the encoder fixture (fishEncodeUniversalList) is already in hand.

Awareness / follow-up (non-blocking)

3. Cross-shell suppression. homePathCoveredByFishConfig runs regardless of $SHELL, so a non-fish user who happens to have a covering config.fish would see the "open a new fish session" note and have the real suggestion suppressed. This mirrors the existing cross-shell behavior of homePathCoveredByRc, and @trek-e already flagged it as acceptable — recording for awareness only.
4. Port the win32 negative-projection test from the closed #721 (assert no fish action is projected on win32) in a follow-up, to lock the POSIX-only contract.

Verdict

Request changes. Code, tests (34/34), build, security (read-only parse, no exec/traversal sink), and the external-tool integration (documented fish_add_path API) are all in good shape. The sole blocker is the standing Diátaxis documentation issue, which an approval may never override. Fix the how-to placement (finding 1) and this is clear to re-approve.

[abrauchli]

@trek-e — thanks, all three actionable items are addressed; pushed in 5534742.

Blocker (Diátaxis, Finding 1) — fixed. The how-to is now action-only. Removed the # fish — persists via fish's universal variables comment and the "…that means an existing fish_user_paths entry (in ~/.config/fish/fish_variables) or a fish_add_path / set -gx PATH line in ~/.config/fish/config.fish" mechanism clause. What remains is one command plus the conditional directive you suggested:

If the directory is already on your PATH but the installer still warns, open a new fish session (exec fish) to pick it up.

I went with the "drop the mechanism detail" option rather than adding a docs/explanation/ note — the existing explanation docs are full design essays, and a one-paragraph stub on which dotfiles the probe reads felt like it'd invite the opposite objection. Happy to add a proper explanation page in a follow-up if you'd rather the mechanism be documented somewhere.

Minor (Finding 2) — added. Extracted decodeFishUniversalValue to a pure, exported function and added a fast-check round-trip property — decode(fishEscape(p)) === p over arbitrary unicode (incl. astral code points), an absolute-path variant, and a never-throws/totality property. The fishEscape fixture mirrors fish's full_escape faithfully (\xHH ≤ 0xFF, \uXXXX otherwise) so the round-trip is meaningful. Consolidated into install-path-detection.test.cjs to stay under the install module's test-file-count ratchet (no allowlist change).

Follow-up (Finding 4) — ported. Added the win32 negative-projection test from #721: asserts no fish action on win32 and that the persist projection there is exactly ['PowerShell', 'cmd.exe', 'Git Bash'].

Finding 3 (cross-shell suppression): left as-is per your note that it's acceptable and consistent with homePathCoveredByRc.

One extra thing I noticed while in here (non-blocking, not changed in this PR to keep scope tight): the config.fish route splits tokens on whitespace, so a quoted path containing spaces in config.fish (e.g. fish_add_path '/my dir/bin') wouldn't match. The fish_variables uvar route — fish's actual persistence target for fish_add_path — handles spaces correctly, and homePathCoveredByRc has the same simplification, so this is a rare edge. Happy to harden it in a follow-up if you think it's worth it.

Re-verified locally: node --test on the two files 39/39; broader install suite 183/183; build:lib, eslint (0 errors), lint:docs, lint:changeset, lint:pr-checks, lint:test-file-count all clean.

[abrauchli]

Ready for review @trek-e . Let me know if you need me to rebase. So far it merges cleanly.

[trek-e]

Summary

Re-review at HEAD 5534742 (fix pushed 2026-06-07T18:59, after the 2026-06-07 request-changes; "ready for review" 2026-06-09). Two additive changes to the post-install PATH-suggestion seam: (A) a third shellActions entry (label: 'fish', fish_add_path '<dir>') in the persist branch of projectPathActionProjection(); (B) homePathCoveredByFishConfig() + exported decodeFishUniversalValue() in bin/install.js, wired into maybeSuggestPathExport(). Plus an action-only how-to section and a type: Changed changeset. Author is not the maintainer; Closes #323; #727 is the only open PR for the issue (#721 closed).

Classification & gate compliance

• Track: Enhancement — #323 carries approved-enhancement (approved 2026-06-01). ✅
• ADR gate: ADR-0009 (Shell Command Projection Module) governs this seam; the change is an additive entry to the existing typed shellActions IR — exactly the pattern ADR-0009 prescribes (typed IR outputs tests assert against), not a boundary/decision change. No signed addendum required. ✅
• Docs (HARD gate, Diátaxis): prior blocker resolved — docs/how-to/install-on-your-runtime.md is now action-only (one description sentence, one fish_add_path block, one exec fish directive); the fish_variables/config.fish mechanism prose that mixed explanation into the how-to is gone. ✅
• Format: branch feat/323-fish-path-suggestion, conventional commits, changeset witty-jays-jump.md (type: Changed, pr: 727), CHANGELOG.md untouched, one concern, no variable renames. ✅

External-tool gate (fish)

Verified against official fish docs via context7:

• fish_add_path '<dir>' — documented persistent-PATH API (fish ≥ 3.2), modifies the fish_user_paths universal variable, deduplicating (fishshell.com/docs/current/cmds/fish_add_path.html). No bash-isms, no subprocess, no output scraping, no monkey-patching.
• Single-quote escaping '\'' — fish's language spec confirms \' is a meaningful escape between single-quoted spans, so the POSIX idiom yields a literal apostrophe; locked by the O'Neil test at bug-3441-path-action-projection.test.cjs:97.
• decodeFishUniversalValue inverts fish's full_escape (\xHH/\uXXXX/\UXXXXXXXX/\n/\r/\t/\\, unknown sequences passed through) — covered by 3 fast-check properties + 6 adversarial examples. ✅

Functional checklist (exercised on HEAD 5534742)

All ✅: fish action emitted at shellActions[2] (fish_add_path prefix, no export, projection length 2→3); single-quote round-trip; no fish action on win32 (new negative test); homePathCoveredByFishConfig detects uvar SETUVAR fish_user_paths: entries with \x2e/\x20/\x2d decoding, literal $ not mistaken for a variable, and config.fish routes (fish_add_path/set -gx PATH/set -Ux), ignoring comments and rejecting relative segments; end-to-end suppression when covered and emission when not. node --test tests/install-path-detection.test.cjs tests/bug-3441-path-action-projection.test.cjs → 39/39.

Test quality / CONTEXT.md

RULESET.TESTS.no-source-grep ✅ (.includes() on captured output/fixtures, not a source-bound var); RULESET.TESTS.property-based-testing ✅ (prior missing-property blocker resolved — round-trip/abs-path/totality properties on decodeFishUniversalValue); no timing assertions; no subprocess; reads are try/catch fault-tolerant. homePathCoveredByFishConfig uses fs.readFileSync directly, matching the pre-existing homePathCoveredByRc pattern — consistent with existing code, no new seam violation.

Findings by severity

Blocker / Major: none.

Minor

1. Unused after import — tests/install-path-detection.test.cjs:13 changes the import to { test, describe, before, after } but after is never used; ESLint flags 'after' is assigned a value but never used (a new warn introduced by this PR; the base import has no after). Please drop after from the destructure.

Nit

2. src/shell-command-projection.cts:364 comment "fish's unquoted context" is technically correct but may confuse a reader who expects \' to work inside single quotes; "between quote spans" reads clearer. Comment-only.
3. config.fish route doesn't detect a single-quoted path containing a space (fish_add_path '/my dir/bin') — same known limitation as homePathCoveredByRc, and the fish_variables uvar route (what fish_add_path actually persists to) handles it. You already flagged this; noting for awareness, not a blocker.

Verdict

Comment — no blockers. Every prior request-changes item is resolved at HEAD, the fish integration follows the documented fish_add_path API, and the test suite (39/39, incl. fast-check + win32 guard) is solid. The only thing standing between this and approval is the one-line unused-after import (minor #1) — remove it and this is mergeable. Nice, careful work on the uvar decoding edge cases.

[abrauchli]

@trek-e — thanks for the re-review. Both items addressed and pushed in c574ead:

Minor 1 (unused after import) — fixed. Dropped after from the destructure in tests/install-path-detection.test.cjs; ESLint is back to 0 warnings.

Nit 2 (comment wording) — applied. The escaping comment in src/shell-command-projection.cts now reads "between quote spans" as you suggested.

Nit 3 (config.fish quoted-space limitation) left as-is per your note — happy to harden alongside homePathCoveredByRc in a follow-up.

Re-verified locally: build:lib clean, eslint 0 errors / 0 warnings, node --test on the two files 39/39.

[trek-e]

Re-review at HEAD c574ead7 — approve

Re-review after the 2026-06-11 push (c574ead7), the response to my 2026-06-10 comment. Checked out the branch, rebuilt bin/lib, and re-ran the gates.

Resolution of prior items

Prior item	Resolution
Minor #1 — unused after import (tests/install-path-detection.test.cjs:13)	Resolved. Import is now const { test, describe, before } = require('node:test');. ESLint on the changed files is clean — the one no-unused-vars warn this PR had introduced is gone.
Nit #2 — escaping comment (src/shell-command-projection.cts:364)	Resolved. Now reads "…a valid escaped single quote in fish between quote spans."

git show c574ead7f is 2 files / 2 one-line changes — no scope creep, no variable renames (RULESET.PR-SCOPE.one-concern-per-pr ✅).

Gate compliance (re-confirmed)

• Track: Enhancement — #323 carries approved-enhancement. Closes #323 (CI.GATE.issue-link-required). ✅
• Docs (Diátaxis HARD gate): the prior blocker is resolved and stays resolved — docs/how-to/install-on-your-runtime.md fish section is action-only (one description sentence, one fish_add_path block, one exec fish directive). ✅
• External-tool gate: fish_add_path '<dir>' is the documented persistent-PATH API (fish ≥3.2); decodeFishUniversalValue inverts fish's full_escape and carries round-trip / abs-path / totality fast-check properties (RULESET.TESTS.property-based-testing ✅). No subprocess, no scraping, read-only parsing.
• Format: branch feat/323-fish-path-suggestion, conventional commits, .changeset/witty-jays-jump.md (type: Changed, pr: 727), CHANGELOG.md untouched (PRED.k320). ✅

Verification

node --test tests/install-path-detection.test.cjs tests/bug-3441-path-action-projection.test.cjs
→ tests 39  pass 39  fail 0

ESLint clean on the changed files; CI 24/24 green.

Verdict

Approved. Every item from the prior review is addressed; the integration follows the documented fish_add_path API and is well-tested. Thanks for the careful work on the uvar decoding edge cases.

[trek-e]

Re-review at HEAD e05f688a — approve

Re-check of PR #727 as of 2026-06-20 against current next (10 commits ahead of the branch tip).

---

Prior review status

All prior change-request cycles are resolved. The review history tells the complete story:

Review	Date	State	Summary
trek-e	2026-06-06T14:43Z	APPROVED	Initial approval on code/test/security
trek-e	2026-06-06T14:46Z	CHANGES_REQUESTED	Diátaxis blocker — how-to mixed explanation prose with action content
trek-e	2026-06-07T14:19Z	CHANGES_REQUESTED	Same Diátaxis blocker unresolved at HEAD 256c14a8
trek-e	2026-06-10T15:40Z	COMMENTED	Blocker resolved; one Minor (unused after import) and one Nit (comment wording) flagged
trek-e	2026-06-14T18:50Z	APPROVED	Both items resolved in c574ead7; 39/39 tests; CI 24/24 green

The current PR labels needs changes / review: changes requested are stale — they were not cleared after the 2026-06-14 approval.

---

Classification & gate compliance (re-confirmed at HEAD)

• Track: Enhancement — #323 carries approved-enhancement. Closes #323 in body. ✅
• ADR gate: Additive entry to the typed shellActions IR under ADR-0009 (Shell Command Projection). No new boundary decision; no addendum required. ✅
• Branch: feat/323-fish-path-suggestion ✅
• Changeset: .changeset/witty-jays-jump.md — type: Changed, pr: 727 ✅
• Docs gate (Diátaxis): docs/how-to/install-on-your-runtime.md — action-only section (one description sentence, one fish_add_path block, one exec fish directive). No explanation/reference prose mixed in. ✅
• CI: all checks SUCCESS (32 success, 2 skipped/N/A). ✅

---

Scope

Six files changed, all within scope:

• src/shell-command-projection.cts — third shellActions entry (fish, fish_add_path) in persist branch of projectPathActionProjection()
• bin/install.js — decodeFishUniversalValue() + homePathCoveredByFishConfig() helper, wired into maybeSuggestPathExport(); both exported
• tests/bug-3441-path-action-projection.test.cjs — updated length assertion 2→3, new fish and win32 exclusion tests
• tests/install-path-detection.test.cjs — fish detection suite (39 total, including 3 fast-check property tests on decodeFishUniversalValue)
• .changeset/witty-jays-jump.md — correct Changed fragment
• docs/how-to/install-on-your-runtime.md — action-only PATH warning section

No scope creep. No variable renames. CHANGELOG.md untouched. ✅

---

Security (re-confirmed)

• homePathCoveredByFishConfig is read-only: fs.readFileSync with try/catch; no fish subprocess; no writes to any file.
• decodeFishUniversalValue is pure; no eval, no dynamic code execution.
• The fish suggestion string uses escapeSingleQuotedShellLiteral (same escaping as zsh/bash siblings) — consistent with RULESET.ARGUMENTS-SANITIZE. ✅
• No prototype-pollution vectors; path.resolve normalises inputs before comparison.

---

Findings

Blockers / Major / Minor / Nit: none — all prior findings resolved.

---

Outstanding housekeeping for author / maintainer

1. Stale labels — needs changes and review: changes requested should be removed now that the 2026-06-14 approval is in effect. Maintainer action.
2. Branch is BEHIND next — mergeStateStatus: BEHIND (next has 10 commits the branch doesn't include, but no merge conflicts are reported). The merge commit e05f688a already incorporated next as of 2026-06-14; the subsequent 10 commits to next are the gap. Author should run gh pr update-branch 727 (or push a rebase) so the merge commit is current before the maintainer merges.

---

Verdict

Approved. The 2026-06-14 approval stands. Every change-request item is resolved; the fish integration is correct, well-tested (including fast-check round-trip properties), and consistent with the existing seam's patterns. The only open items are housekeeping (stale labels, branch behind next) — not code findings. Ready to merge once the branch is updated against current next.

[trek-e]

Independent review at HEAD e05f688a

30/30 CI, mergeableState: clean, 5 prior review cycles fully resolved. Reading the full implementation including the two sections rtk had truncated (83 lines of homePathCoveredByFishConfig, 219 lines of tests).

Summary of changes (confirmed)

Surface	Change
src/shell-command-projection.cts	third shellActions entry — label:'fish', command:'fish_add_path \'...\'; POSIX branch only
bin/install.js	decodeFishUniversalValue() (pure decoder, char-by-char); homePathCoveredByFishConfig() (read-only, two routes); both exported; wired into maybeSuggestPathExport
tests/	39 example-based tests + 3 fast-check property tests on the decoder round-trip
docs/how-to/	action-only PATH warning section
changeset	Changed, pr:727

Implementation correctness

decodeFishUniversalValue — char-by-char loop, handles \n, \r, \t, \\, \xHH, \uXXXX, \UXXXXXXXX; unknown sequences pass through verbatim; backslash at end of string falls through to the final else { out += c } branch correctly. Property test decode(fishEscape(p)) === p locks the bijection.

Route 1 (fish_variables) — regex SETUVAR(?:\s+--\S+)*\s+fish_user_paths:(.*) correctly anchors to line start and captures flags. Split on the literal 4-char token \x1e (not a raw byte). matchesLiteral normalises via path.resolve and strips trailing slashes. Adversarial test with literal $ in directory name confirms the uvar route doesn't apply matchesTarget's $-guard (correctly — these are decoded literals, not shell tokens).

Route 2 (config.fish) — three regex patterns cover the common forms. Tokens split by whitespace; flag tokens (-g, --path) skipped by - guard. matchesTarget strips surrounding quotes, expands $HOME/~/, rejects anything still containing $. Inline-comment tokens (#, main, toolchain) are rejected because path.isAbsolute('#') is false. Relative-path regression test (fish_add_path bin) correctly returns false.

Test coverage — all cases present: fish_variables detection, hex-escaped paths (dots/hyphens/spaces/$), config.fish variants, negative cases (no config, wrong dir, commented-out line, unreadable file-replaced-by-dir), end-to-end maybeSuggestPathExport suppression and emission tests.

Findings

Nit — The warning message hardcodes "via fish's universal variables" in all suppression cases, but homePathCoveredByFishConfig has two routes — Route 1 (universal variables) and Route 2 (explicit config.fish mutations). A user whose coverage comes from fish_add_path /usr/local/bin in config.fish sees "via fish's universal variables", which is inaccurate. The user action ("open a new fish session") is the same in both cases, so this is a label nit only. Non-blocking.

No security issues. No blockers. Approved.