* feat(#323): fish-shell support in post-install PATH suggestion Two additive changes to the post-install PATH-suggestion seam, both scoped to existing functions. A. Projection: add a fish entry to the persist-mode shell-action list in projectPathActionProjection() (src/shell-command-projection.cts). fish has no `export`/`$PATH`-list syntax, so the existing zsh/bash `export PATH=...` commands are inert when pasted. The new entry emits the fish-native `fish_add_path '<dir>'` (fish 3.2+, persists via the universal-variable store, de-duplicating). The directory is single-quoted with the same POSIX literal escaping as the zsh/bash siblings; verified round-tripping through real fish 3.7.0 for paths containing quotes, spaces, `$`, `*`, backticks and unicode. B. Detection: add homePathCoveredByFishConfig() in bin/install.js, called from maybeSuggestPathExport() alongside homePathCoveredByRc(). fish does not use sh-style `export PATH=` rc files, so a fish user whose fish_user_paths already covers the global bin would otherwise get a false-positive "not on your PATH" warning on every install. Two side-effect-free detection routes (no fish subprocess): 1. The universal-variable store (~/.config/fish/fish_variables). fish serializes this 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, then compares each as an absolute literal — a decoded `$` is part of the directory name, not an unexpanded variable. Verified against real fish 3.7.0 output. 2. config.fish (`fish_add_path`, `set -gx PATH`, `set -Ux fish_user_paths`) — plain shell tokens: HOME forms ($HOME/${HOME}/~) are expanded and a token still holding `$` (e.g. `$PATH`, `$fish_user_paths`) is skipped. Honours $XDG_CONFIG_HOME and always also checks ~/.config/fish. No behaviour change for bash/zsh/PowerShell/cmd/Git-Bash users: their entries and command strings are unchanged; the fish entry is additive and the fish detector only narrows the set of cases that warn. Tests: update the projection length assertion (2 -> 3) and fish escaping in bug-3441; add fish detection + suppression cases in install-path-detection (uvar store with real fish escaping, dot/hyphen/space/$-literal decode regressions, config.fish routes, commented-out, relative-segment guard, unreadable-file fault injection, suppression and emission via maybeSuggestPathExport). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(changeset): add Changed fragment for #323 fish PATH support (#727) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#323): address review — action-only fish docs, decoder property test, win32 guard Addresses @trek-e's review on #727: - docs (blocker): keep the how-to action-only (Diátaxis). Drop the `# fish — persists via …` comment and the internal-mechanism clause naming fish_variables/config.fish; leave one command + the exec-fish directive. - tests (minor): extract decodeFishUniversalValue to a pure, exported module function and add fast-check round-trip properties (decode(fishEscape(p)) === p over arbitrary unicode, abs-path variant, totality). Consolidated into install-path-detection.test.cjs to respect the install test-file-count ratchet. - tests (follow-up): port #721's win32 negative-projection test (no fish action on win32; persist projection is PowerShell/cmd.exe/Git Bash). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#323): address review — drop unused 'after' import, clarify escaping comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Andreas Brauchli
@@ -39,11 +39,36 @@ describe('bug #3441: PATH guidance is projected from typed shell action IR', ()
|
||||
platform: 'linux',
|
||||
});
|
||||
assert.ok(Array.isArray(posix.shellActions));
|
||||
assert.equal(posix.shellActions.length, 2);
|
||||
assert.equal(posix.shellActions.length, 3);
|
||||
assert.equal(posix.shellActions[0].label, 'zsh');
|
||||
assert.equal(posix.shellActions[1].label, 'bash');
|
||||
assert.equal(posix.shellActions[2].label, 'fish');
|
||||
assert.ok(posix.shellActions[0].command.includes('~/.zshrc'));
|
||||
assert.ok(posix.shellActions[1].command.includes('~/.bashrc'));
|
||||
// #323: fish gets a fish-native fish_add_path suggestion, not `export`.
|
||||
assert.ok(posix.shellActions[2].command.startsWith('fish_add_path '));
|
||||
assert.ok(!posix.shellActions[2].command.includes('export'));
|
||||
});
|
||||
|
||||
// #323 (ported from the closed #721): the fish suggestion is POSIX-only.
|
||||
// On win32 the persist branch projects PowerShell / cmd.exe / Git Bash —
|
||||
// no fish action — locking the POSIX-only contract.
|
||||
test('no fish action is projected on win32', () => {
|
||||
const win = projection.projectPathActionProjection({
|
||||
mode: 'persist',
|
||||
targetDir: 'C:\\Users\\me\\AppData\\npm',
|
||||
platform: 'win32',
|
||||
});
|
||||
assert.ok(Array.isArray(win.shellActions));
|
||||
assert.equal(
|
||||
win.shellActions.some((a) => a.shell === 'fish' || a.label === 'fish'),
|
||||
false,
|
||||
'win32 persist projection must not include a fish action',
|
||||
);
|
||||
assert.deepEqual(
|
||||
win.shellActions.map((a) => a.label),
|
||||
['PowerShell', 'cmd.exe', 'Git Bash'],
|
||||
);
|
||||
});
|
||||
|
||||
test('POSIX repair mode escapes double-quoted shell metacharacters', () => {
|
||||
@@ -67,6 +92,9 @@ describe('bug #3441: PATH guidance is projected from typed shell action IR', ()
|
||||
});
|
||||
assert.equal(projected.shellActions[0].command.includes("/tmp/O'\\''Neil/bin"), true);
|
||||
assert.equal(projected.shellActions[1].command.includes("/tmp/O'\\''Neil/bin"), true);
|
||||
// #323: fish entry single-quotes the dir with the same POSIX literal
|
||||
// escaping (`'\''` is also a valid escaped quote in fish unquoted context).
|
||||
assert.equal(projected.shellActions[2].command, "fish_add_path '/tmp/O'\\''Neil/bin'");
|
||||
});
|
||||
|
||||
test('maybeSuggestPathExport renders commands projected by path-action seam', () => {
|
||||
|
||||
@@ -15,6 +15,7 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
const fc = require('./helpers/fast-check-setup.cjs');
|
||||
|
||||
const INSTALL_PATH = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
@@ -317,4 +318,323 @@ describe('installer HOME-relative PATH detection (#2620)',
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
// #323 — fish has no sh-style `export PATH=` rc file, so homePathCoveredByRc
|
||||
// can never see a fish user's PATH. homePathCoveredByFishConfig parses fish's
|
||||
// universal-variable store (fish_variables) and config.fish so the installer
|
||||
// does not emit a false-positive warning for fish users whose
|
||||
// fish_user_paths already covers globalBin.
|
||||
describe('fish-shell PATH coverage detection (#323)', () => {
|
||||
function writeFishFile(home, name, content) {
|
||||
const fishDir = path.join(home, '.config', 'fish');
|
||||
fs.mkdirSync(fishDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(fishDir, name), content);
|
||||
}
|
||||
|
||||
// Mirror fish's universal-variable serialization (`full_escape`): every
|
||||
// byte outside [A-Za-z0-9/_] is written as `\xHH`, and list elements are
|
||||
// joined by the literal 4-char token `\x1e` — NOT a raw 0x1e byte.
|
||||
// Verified against fish 3.7.0 output (space -> \x20, `-` -> \x2d, `.` ->
|
||||
// \x2e). Fixtures use this so the decoder is tested against real format.
|
||||
function fishEncodeUniversalList(paths) {
|
||||
const esc = (p) => p.replace(/[^A-Za-z0-9/_]/g, (ch) =>
|
||||
'\\x' + ch.charCodeAt(0).toString(16).padStart(2, '0'));
|
||||
return paths.map(esc).join('\\x1e');
|
||||
}
|
||||
|
||||
test('homePathCoveredByFishConfig is exported', () => {
|
||||
assert.strictEqual(
|
||||
typeof installer.homePathCoveredByFishConfig,
|
||||
'function',
|
||||
'bin/install.js must export homePathCoveredByFishConfig for #323',
|
||||
);
|
||||
});
|
||||
|
||||
test('detects fish_user_paths in the universal-variable store', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
writeFishFile(
|
||||
home,
|
||||
'fish_variables',
|
||||
[
|
||||
'SETUVAR --export LANG:en_US',
|
||||
`SETUVAR fish_user_paths:${fishEncodeUniversalList([globalBin, '/usr/local/bin'])}`,
|
||||
'',
|
||||
].join('\n'),
|
||||
);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
// Regression: fish escapes `-`, `.`, and space in the universal-variable
|
||||
// store, so the detector must decode `\xHH` before comparing. A raw
|
||||
// string match (the original, naive implementation) fails here. Mirrors a
|
||||
// real nvm path (dots + hyphens) plus a space-containing sibling.
|
||||
test('decodes fish-escaped paths (dots, hyphens, spaces) in fish_variables', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'versions', 'node', 'v24.15.0', 'bin');
|
||||
const spaced = path.join(home, 'my tools', 'bin');
|
||||
const encoded = fishEncodeUniversalList([spaced, globalBin]);
|
||||
// Sanity: the fixture really is escaped, not a plain path.
|
||||
assert.ok(encoded.includes('\\x2e') && encoded.includes('\\x20') && encoded.includes('\\x1e'));
|
||||
writeFishFile(home, 'fish_variables', `SETUVAR fish_user_paths:${encoded}\n`);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(spaced, home), true);
|
||||
assert.strictEqual(
|
||||
installer.homePathCoveredByFishConfig(path.join(home, 'not', 'there'), home),
|
||||
false,
|
||||
);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
// Adversarial regression: fish stores a literal `$` in a directory name as
|
||||
// `\x24` in the universal store, so the decoded entry contains `$`. That
|
||||
// `$` is part of the path, not an unexpanded variable — the uvar route
|
||||
// must still match it. (config.fish tokens keep the `$VAR` guard.)
|
||||
test('detects a fish_user_paths entry whose directory name contains a literal $', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, 'has $VAR dir', 'bin');
|
||||
writeFishFile(home, 'fish_variables', `SETUVAR fish_user_paths:${fishEncodeUniversalList([globalBin])}\n`);
|
||||
assert.ok(fishEncodeUniversalList([globalBin]).includes('\\x24'), 'fixture must encode $ as \\x24');
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('detects fish_add_path in config.fish (with flag)', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
writeFishFile(home, 'config.fish', `fish_add_path -g ${globalBin}\n`);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('detects set -gx PATH in config.fish', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
writeFishFile(home, 'config.fish', `set -gx PATH $PATH ${globalBin}\n`);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('detects set -Ux fish_user_paths in config.fish', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
writeFishFile(home, 'config.fish', `set -Ux fish_user_paths ${globalBin} /usr/bin\n`);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), true);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('ignores commented-out fish_add_path lines', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
writeFishFile(home, 'config.fish', `# fish_add_path ${globalBin}\n`);
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), false);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('returns false when fish config does not cover globalBin', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
writeFishFile(home, 'config.fish', 'fish_add_path /opt/some/other/bin\n');
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), false);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('returns false when no fish config exists', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), false);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('does not resolve a bare relative fish_add_path segment against HOME', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
writeFishFile(home, 'config.fish', 'fish_add_path bin\n');
|
||||
const globalBin = path.join(home, 'bin');
|
||||
assert.strictEqual(
|
||||
installer.homePathCoveredByFishConfig(globalBin, home),
|
||||
false,
|
||||
'relative fish_add_path segments must not be resolved against $HOME',
|
||||
);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('swallows an unreadable fish config without throwing', () => {
|
||||
const home = createTempHome();
|
||||
try {
|
||||
const fishDir = path.join(home, '.config', 'fish');
|
||||
fs.mkdirSync(fishDir, { recursive: true });
|
||||
fs.mkdirSync(path.join(fishDir, 'config.fish')); // dir where a file is expected
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
assert.doesNotThrow(() => installer.homePathCoveredByFishConfig(globalBin, home));
|
||||
assert.strictEqual(installer.homePathCoveredByFishConfig(globalBin, home), false);
|
||||
} finally {
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('maybeSuggestPathExport suppresses suggestion when fish config covers globalBin', () => {
|
||||
const home = createTempHome();
|
||||
const origPath = process.env.PATH;
|
||||
try {
|
||||
const globalBin = path.join(home, '.nvm', 'bin');
|
||||
fs.mkdirSync(globalBin, { recursive: true });
|
||||
// globalBin not on the current PATH, no sh rc files — only fish covers it.
|
||||
process.env.PATH = '/usr/bin';
|
||||
writeFishFile(home, 'fish_variables', `SETUVAR fish_user_paths:${fishEncodeUniversalList([globalBin])}\n`);
|
||||
|
||||
const logs = [];
|
||||
const origLog = console.log;
|
||||
console.log = (...args) => { logs.push(args.join(' ')); };
|
||||
try {
|
||||
installer.maybeSuggestPathExport(globalBin, home);
|
||||
} finally {
|
||||
console.log = origLog;
|
||||
}
|
||||
|
||||
const joined = logs.join('\n');
|
||||
assert.ok(
|
||||
!/fish_add_path/.test(joined) && !/Add it with one of/.test(joined),
|
||||
`installer should not emit a PATH suggestion when fish already covers it; got:\n${joined}`,
|
||||
);
|
||||
assert.ok(
|
||||
/universal variables/.test(joined),
|
||||
`installer should print the fish reopen note; got:\n${joined}`,
|
||||
);
|
||||
} finally {
|
||||
if (origPath === undefined) delete process.env.PATH; else process.env.PATH = origPath;
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
|
||||
test('maybeSuggestPathExport emits fish_add_path suggestion when nothing covers globalBin', () => {
|
||||
const home = createTempHome();
|
||||
const origPath = process.env.PATH;
|
||||
try {
|
||||
const globalBin = path.join(home, '.npm-global', 'bin');
|
||||
fs.mkdirSync(globalBin, { recursive: true });
|
||||
process.env.PATH = '/usr/bin';
|
||||
|
||||
const logs = [];
|
||||
const origLog = console.log;
|
||||
console.log = (...args) => { logs.push(args.join(' ')); };
|
||||
try {
|
||||
installer.maybeSuggestPathExport(globalBin, home);
|
||||
} finally {
|
||||
console.log = origLog;
|
||||
}
|
||||
|
||||
const joined = logs.join('\n');
|
||||
const projected = projection.projectPersistentPathExportActions({
|
||||
targetDir: globalBin,
|
||||
platform: process.platform,
|
||||
});
|
||||
const fishAction = projected.shellActions.find((a) => a.shell === 'fish');
|
||||
assert.ok(fishAction, 'projection must include a fish action');
|
||||
assert.ok(
|
||||
joined.includes(fishAction.command),
|
||||
`installer should render the projected fish command "${fishAction.command}". Output:\n${joined}`,
|
||||
);
|
||||
} finally {
|
||||
if (origPath === undefined) delete process.env.PATH; else process.env.PATH = origPath;
|
||||
cleanup(home);
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// #323 — property-based coverage for the fish universal-variable decoder.
|
||||
// `decodeFishUniversalValue` is the inverse of fish's `full_escape`; the
|
||||
// example-based cases above cover dot/hyphen/space/$/unicode, this locks the
|
||||
// bijection itself: decode(fishEscape(p)) === p over arbitrary strings.
|
||||
// Platform-agnostic (a pure string transform), so this block is NOT skipped on
|
||||
// Windows — unlike the rc/fish-config probes above.
|
||||
describe('decodeFishUniversalValue: round-trip properties (#323)', () => {
|
||||
let installer;
|
||||
before(() => { installer = loadInstaller(); });
|
||||
|
||||
// Faithful inverse of decodeFishUniversalValue, matching fish's full_escape:
|
||||
// keep [A-Za-z0-9/_] literal, \xHH for code units <= 0xFF, \uXXXX otherwise
|
||||
// (each UTF-16 code unit is <= 0xFFFF, so astral code points encode as their
|
||||
// two surrogate units and decode back identically).
|
||||
function fishEscape(value) {
|
||||
let out = '';
|
||||
for (let i = 0; i < value.length; i++) {
|
||||
const ch = value[i];
|
||||
if (/[A-Za-z0-9/_]/.test(ch)) { out += ch; continue; }
|
||||
const code = value.charCodeAt(i);
|
||||
out += code <= 0xff
|
||||
? '\\x' + code.toString(16).padStart(2, '0')
|
||||
: '\\u' + code.toString(16).padStart(4, '0');
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
test('decodeFishUniversalValue is exported', () => {
|
||||
assert.strictEqual(typeof installer.decodeFishUniversalValue, 'function');
|
||||
});
|
||||
|
||||
// The bijection across arbitrary unicode (spaces, dots, hyphens, $, quotes,
|
||||
// astral code points).
|
||||
test('decode(fishEscape(p)) === p for arbitrary strings', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.string({ unit: 'binary', maxLength: 64 }), (p) => {
|
||||
assert.strictEqual(installer.decodeFishUniversalValue(fishEscape(p)), p);
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
// Realistic shape: absolute POSIX paths built from arbitrary segments — the
|
||||
// actual fish_user_paths entries the detector compares — still round-trip.
|
||||
test('decode(fishEscape(absPath)) === absPath for arbitrary path segments', () => {
|
||||
const segment = fc.string({ unit: 'binary', minLength: 1, maxLength: 24 })
|
||||
.filter((s) => !s.includes('/'));
|
||||
fc.assert(
|
||||
fc.property(fc.array(segment, { minLength: 1, maxLength: 5 }), (segs) => {
|
||||
const abs = '/' + segs.join('/');
|
||||
assert.strictEqual(installer.decodeFishUniversalValue(fishEscape(abs)), abs);
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
// Total function: any unrecognised `\`-sequence (incl. truncated escapes)
|
||||
// passes through verbatim and it never throws.
|
||||
test('never throws and is total over arbitrary escaped input', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.string({ unit: 'binary', maxLength: 64 }), (raw) => {
|
||||
assert.doesNotThrow(() => installer.decodeFishUniversalValue(raw));
|
||||
assert.strictEqual(typeof installer.decodeFishUniversalValue(raw), 'string');
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user