Fix Pi global install path (#338)

* Fix Pi global install path

* Simplify Pi skills-path helpers and consolidate tests

One userProviderSkillsDir helper owns the HOME_SKILLS_DIR_OVERRIDES
lookup, read paths share existingSkillsDirs, and the five Pi install
tests collapse into two that keep the same coverage: global detection
plus the agent-path write, and project scope in a home-rooted repo.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Respect requested scope when resolving Pi skills dirs

An explicit install scope now narrows providerSkillsDirCandidates to
the matching layout, so a project-scope install in a home-rooted repo
no longer matches an existing global Pi install and get swallowed by
the already-installed refresh path. Update/check flows still probe
both layouts since they have no scope. Covers the T-Rex repro in the
home-rooted regression test.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Refresh every existing Pi layout on unscoped update

deduplicateProviders keeps one entry per existing layout instead of
only the first, so unscoped check/update refresh both ~/.pi/agent/skills
and ~/.pi/skills when a home-rooted repo holds copies in each. Home-dir
detection now compares realpaths, since findProjectRoot resolves
symlinks while homedir() does not.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Abdul Wahab <abdulwahab@Abduls-MacBook-Pro-2.local>
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Abdul Wahab
2026-07-07 17:19:13 -07:00
committed by GitHub
co-authored by Abdul Wahab Cursor
parent 0092df907b
commit c775e03c1d
3 changed files with 191 additions and 74 deletions
+133 -73
View File
@@ -59,6 +59,13 @@ const PROVIDER_DISPLAY = {
};
const PROVIDER_INPUT_ORDER = ['claude', 'codex', 'cursor', 'gemini', 'github', 'kiro', 'opencode', 'pi', 'qoder', 'trae', 'trae-cn', 'rovo-dev'];
// Providers whose GLOBAL (home) skills dir is not `<provider>/skills`.
// Pi discovers global skills from ~/.pi/agent/skills/; project scope
// stays .pi/skills/. See issue #327.
const HOME_SKILLS_DIR_OVERRIDES = {
'.pi': join('.pi', 'agent', 'skills'),
};
// When a project has no harness folder yet, infer the target from globally
// installed harnesses (~/.claude, ~/.codex, ...). Codex reads skills from
// .agents/skills, so ~/.codex maps to the .agents bundle variant.
@@ -69,6 +76,7 @@ const GLOBAL_HARNESS_HINTS = [
{ home: '.gemini', provider: '.gemini' },
{ home: '.kiro', provider: '.kiro' },
{ home: '.opencode', provider: '.opencode' },
{ home: '.pi', provider: '.pi' },
{ home: '.qoder', provider: '.qoder' },
{ home: '.rovodev', provider: '.rovodev' },
];
@@ -111,6 +119,45 @@ const PROVIDER_HOOK_ARTIFACTS = {
],
};
function userProviderSkillsDir(home, provider) {
if (HOME_SKILLS_DIR_OVERRIDES[provider]) return join(home, HOME_SKILLS_DIR_OVERRIDES[provider]);
return join(home, provider, 'skills');
}
// Compare via realpath: the project root comes from process.cwd() (symlinks
// resolved) while homedir() reflects $HOME verbatim, so a home dir reached
// through a symlink (e.g. /tmp -> /private/tmp) would fail a string compare.
function isHomeDir(root) {
if (root === homedir()) return true;
try {
return realpathSync(root) === realpathSync(homedir());
} catch {
return false;
}
}
// Every layout a provider's installed skills can live in under `root`.
// `scope` narrows the answer when the caller knows which install it is
// acting on: 'user' means the provider's global layout, 'project' means
// `<provider>/skills`. Without a scope (update/check, where installs of
// either kind may live under `root`) both layouts are candidates when
// `root` is the home dir, since an overridden provider (Pi) keeps its
// global skills elsewhere while a repo rooted at ~ still uses the project
// layout. Scoping matters for the same reason: a project-scope install in
// a home-rooted repo must not be conflated with an existing global one.
function providerSkillsDirCandidates(root, provider, scope) {
if (scope === 'user') return [userProviderSkillsDir(root, provider)];
const dirs = [join(root, provider, 'skills')];
if (scope !== 'project' && HOME_SKILLS_DIR_OVERRIDES[provider] && isHomeDir(root)) {
dirs.unshift(userProviderSkillsDir(root, provider));
}
return dirs;
}
function existingSkillsDirs(root, provider, scope) {
return providerSkillsDirCandidates(root, provider, scope).filter(existsSync);
}
let pipedAnswers = null;
class PromptAbortError extends Error {
constructor() {
@@ -415,13 +462,15 @@ async function showHelp() {
/**
* Read the skills version from the impeccable SKILL.md frontmatter.
*/
function getSkillsVersion(root) {
function getSkillsVersion(root, scope) {
for (const d of PROVIDER_DIRS) {
const skillMd = join(root, d, 'skills', 'impeccable', 'SKILL.md');
if (!existsSync(skillMd)) continue;
const content = readFileSync(skillMd, 'utf-8');
const match = content.match(/^version:\s*(.+)$/m);
if (match) return match[1].trim().replace(/^["']|["']$/g, '');
for (const skillsDir of providerSkillsDirCandidates(root, d, scope)) {
const skillMd = join(skillsDir, 'impeccable', 'SKILL.md');
if (!existsSync(skillMd)) continue;
const content = readFileSync(skillMd, 'utf-8');
const match = content.match(/^version:\s*(.+)$/m);
if (match) return match[1].trim().replace(/^["']|["']$/g, '');
}
}
return null;
}
@@ -535,14 +584,17 @@ function hashSkillFile(filePath) {
* per unique real path. The first provider that maps to a real path
* wins (so the bundle uses that provider's build).
*/
function deduplicateProviders(root, providers) {
function deduplicateProviders(root, providers, scope) {
const seen = new Map(); // realPath -> { provider, localSkillsDir }
for (const provider of providers) {
const skillsDir = join(root, provider, 'skills');
if (!existsSync(skillsDir)) continue;
const real = realpathSync(skillsDir);
if (!seen.has(real)) {
seen.set(real, { provider, localSkillsDir: skillsDir });
// A provider can hold real installs in more than one layout (a home-rooted
// repo may carry both ~/.pi/agent/skills and ~/.pi/skills). Keep each as
// its own entry so update/check touch every tree, not just the first.
for (const skillsDir of existingSkillsDirs(root, provider, scope)) {
const real = realpathSync(skillsDir);
if (!seen.has(real)) {
seen.set(real, { provider, localSkillsDir: skillsDir });
}
}
}
return [...seen.values()];
@@ -556,8 +608,8 @@ function deduplicateProviders(root, providers) {
* SKILL.md, so script-only fixes and removed files are detected.
* Returns true if every bundle skill matches the local copy.
*/
function isUpToDate(root, providers, bundleDir) {
const unique = deduplicateProviders(root, providers);
function isUpToDate(root, providers, bundleDir, scope) {
const unique = deduplicateProviders(root, providers, scope);
if (unique.length === 0) return false;
for (const { provider, localSkillsDir } of unique) {
@@ -621,20 +673,20 @@ async function check() {
// ─── skills install ───────────────────────────────────────────────────────────
// Check if impeccable skills are already present in any provider folder
function isAlreadyInstalled(root) {
function isAlreadyInstalled(root, scope) {
for (const d of PROVIDER_DIRS) {
const skillsDir = join(root, d, 'skills');
if (!existsSync(skillsDir)) continue;
try {
const entries = readdirSync(skillsDir);
// Look for 'impeccable' skill (or prefixed variant, or legacy 'teach-impeccable')
if (entries.some(e =>
e === 'impeccable' || e.endsWith('-impeccable') ||
e === 'teach-impeccable' || e.endsWith('-teach-impeccable')
)) {
return d;
}
} catch {}
for (const skillsDir of existingSkillsDirs(root, d, scope)) {
try {
const entries = readdirSync(skillsDir);
// Look for 'impeccable' skill (or prefixed variant, or legacy 'teach-impeccable')
if (entries.some(e =>
e === 'impeccable' || e.endsWith('-impeccable') ||
e === 'teach-impeccable' || e.endsWith('-teach-impeccable')
)) {
return d;
}
} catch {}
}
}
return null;
}
@@ -678,26 +730,26 @@ function isRealSkillDir(skillsDir, name) {
* by name -- never touches third-party skills that happen to start with `i-`.
* Returns the number of skills migrated.
*/
function migrateUnprefixImpeccable(root) {
function migrateUnprefixImpeccable(root, scope) {
let migrated = 0;
for (const d of PROVIDER_DIRS) {
const skillsDir = join(root, d, 'skills');
if (!existsSync(skillsDir)) continue;
let entries;
try { entries = readdirSync(skillsDir); } catch { continue; }
for (const name of entries) {
// A prefixed impeccable skill is `<prefix>impeccable`, not the canonical
// `impeccable` and not an unrelated legacy skill name.
if (name === 'impeccable' || name === 'teach-impeccable') continue;
if (!name.endsWith('-impeccable')) continue;
if (!isRealSkillDir(skillsDir, name)) continue;
for (const skillsDir of existingSkillsDirs(root, d, scope)) {
let entries;
try { entries = readdirSync(skillsDir); } catch { continue; }
for (const name of entries) {
// A prefixed impeccable skill is `<prefix>impeccable`, not the canonical
// `impeccable` and not an unrelated legacy skill name.
if (name === 'impeccable' || name === 'teach-impeccable') continue;
if (!name.endsWith('-impeccable')) continue;
if (!isRealSkillDir(skillsDir, name)) continue;
const dest = join(skillsDir, 'impeccable');
try {
rmSync(dest, { recursive: true, force: true });
renameSync(join(skillsDir, name), dest);
migrated++;
} catch {}
const dest = join(skillsDir, 'impeccable');
try {
rmSync(dest, { recursive: true, force: true });
renameSync(join(skillsDir, name), dest);
migrated++;
} catch {}
}
}
}
return migrated;
@@ -774,7 +826,7 @@ function uniquePaths(paths) {
function userSkillProbePaths(home, harnessDir, provider) {
return uniquePaths([
join(home, provider, 'skills'),
userProviderSkillsDir(home, provider),
join(home, harnessDir, 'skills'),
]);
}
@@ -804,7 +856,7 @@ function collectInstallDetections(root, home = homedir()) {
scope: 'user',
foundPath,
installRoot: home,
installPath: join(home, provider, 'skills'),
installPath: userProviderSkillsDir(home, provider),
skillProbePaths,
hasRealSkills: skillProbePaths.some(hasRealSkillEntries),
reason: 'user harness folder',
@@ -1044,13 +1096,17 @@ function isInProjectProviderLink(localSkillsDir, root, provider) {
* Copy each target provider's compiled skill variant from an extracted bundle
* into the project. Writes real directories (copy, never symlink) so every
* harness keeps the build that was compiled for it. Returns skills written.
* `scope: 'user'` writes to the provider's global skills layout (see
* HOME_SKILLS_DIR_OVERRIDES); anything else writes `<provider>/skills`.
*/
function copyProviderSkills(bundleDir, root, targets) {
function copyProviderSkills(bundleDir, root, targets, { scope } = {}) {
let written = 0;
for (const provider of targets) {
const srcDir = join(bundleDir, provider, 'skills');
if (existsSync(srcDir)) {
const localSkillsDir = join(root, provider, 'skills');
const localSkillsDir = scope === 'user'
? userProviderSkillsDir(root, provider)
: join(root, provider, 'skills');
// A previous `npx skills` install may have left this provider's skills dir
// as a symlink to ANOTHER in-project provider's canonical copy. Drop only
// that link so we write a real, provider-specific directory. A user's
@@ -1072,8 +1128,8 @@ function copyProviderSkills(bundleDir, root, targets) {
return written;
}
function refreshProviderSkills(bundleDir, root, providers) {
const unique = deduplicateProviders(root, providers);
function refreshProviderSkills(bundleDir, root, providers, scope) {
const unique = deduplicateProviders(root, providers, scope);
let updated = 0;
for (const { provider, localSkillsDir } of unique) {
const srcDir = join(bundleDir, provider, 'skills');
@@ -1532,13 +1588,13 @@ async function install(flags) {
}
const { targets, installRoot, hookRoot, scope } = plan;
const existing = isAlreadyInstalled(installRoot);
const existing = isAlreadyInstalled(installRoot, scope);
if (existing && !force) {
console.log(`Impeccable skills are already installed (found in ${existing}/).`);
const installedTargets = findInstalledProviders(installRoot);
const installedTargets = findInstalledProviders(installRoot, scope);
const selectedInstalledTargets = targets.filter(provider => installedTargets.includes(provider));
const linkedTargets = findLinkedProviders(installRoot, selectedInstalledTargets);
const linkedTargets = findLinkedProviders(installRoot, selectedInstalledTargets, scope);
const copyTargets = selectedInstalledTargets.filter(provider => !linkedTargets.includes(provider));
const hookTargets = selectedInstalledTargets;
const wantHooks = installHooks && await decideHookInstall(hookRoot, hookTargets, { yes });
@@ -1565,10 +1621,10 @@ async function install(flags) {
}
}
if (!updateCheckSkipped && copyTargets.length > 0 && !isUpToDate(installRoot, copyTargets, bundleDir)) {
migrateUnprefixImpeccable(installRoot);
updated = refreshProviderSkills(bundleDir, installRoot, copyTargets);
const v = getSkillsVersion(installRoot);
if (!updateCheckSkipped && copyTargets.length > 0 && !isUpToDate(installRoot, copyTargets, bundleDir, scope)) {
migrateUnprefixImpeccable(installRoot, scope);
updated = refreshProviderSkills(bundleDir, installRoot, copyTargets, scope);
const v = getSkillsVersion(installRoot, scope);
console.log(`Updated ${updated} skill(s)${v ? ` to v${v}` : ''}.`);
}
@@ -1581,7 +1637,7 @@ async function install(flags) {
console.log('Existing skills were left unchanged.');
console.log('Run with --force to reinstall.\n');
} else if (updated === 0 && writtenHookTargets.length === 0) {
const v = getSkillsVersion(installRoot);
const v = getSkillsVersion(installRoot, scope);
console.log(`Skills are up to date${v ? ` (v${v})` : ''}.`);
console.log('Run with --force to reinstall.\n');
} else {
@@ -1620,12 +1676,12 @@ async function install(flags) {
// Retire any old `i-`-prefixed install so the fresh copy lands on the
// canonical `impeccable` dir instead of orphaning the prefixed one.
migrateUnprefixImpeccable(installRoot);
migrateUnprefixImpeccable(installRoot, scope);
let written = 0;
let hookTargets = [];
try {
written = copyProviderSkills(bundleDir, installRoot, targets);
written = copyProviderSkills(bundleDir, installRoot, targets, { scope });
hookTargets = wantHooks ? copyProviderHooks(bundleDir, hookRoot, targets, { force, skillRoot: installRoot }) : [];
} catch (e) {
rmSync(bundleDir, { recursive: true, force: true });
@@ -1655,27 +1711,31 @@ function findProjectRoot() {
return process.cwd();
}
function findInstalledProviders(root) {
function findInstalledProviders(root, scope) {
const found = [];
for (const d of PROVIDER_DIRS) {
const skillsDir = join(root, d, 'skills');
if (!existsSync(skillsDir)) continue;
try {
const entries = readdirSync(skillsDir);
if (entries.some(name => isSkillDir(skillsDir, name))) found.push(d);
} catch {}
for (const skillsDir of existingSkillsDirs(root, d, scope)) {
try {
const entries = readdirSync(skillsDir);
if (entries.some(name => isSkillDir(skillsDir, name))) {
found.push(d);
break;
}
} catch {}
}
}
return found;
}
function findLinkedProviders(root, providers) {
function findLinkedProviders(root, providers, scope) {
return providers.filter(provider => {
const skillDir = join(root, provider, 'skills', 'impeccable');
try {
return lstatSync(skillDir).isSymbolicLink();
} catch {
return false;
for (const skillsDir of providerSkillsDirCandidates(root, provider, scope)) {
const skillDir = join(skillsDir, 'impeccable');
try {
if (lstatSync(skillDir).isSymbolicLink()) return true;
} catch {}
}
return false;
});
}
+1 -1
View File
@@ -79,7 +79,7 @@ Notes:
| GitHub Copilot | `.github/skills/` | `.agents/skills/`, `.claude/skills/` |
| Kiro | `.kiro/skills/` | - |
| OpenCode | `.opencode/skills/` | `.agents/skills/`, `.claude/skills/` |
| Pi | `.pi/skills/` | `.agents/skills/` |
| Pi | `.pi/skills/` (project), `~/.pi/agent/skills/` (global) | `.agents/skills/` |
| Qoder | `.qoder/skills/` | `~/.qoder/skills/` (user-level) |
| Trae China | `.trae-cn/skills/` | TBD |
| Trae International | `.trae/skills/` | TBD |
+57
View File
@@ -833,6 +833,63 @@ describe('skills install/update: local universal bundle e2e', () => {
rmSync(home, { recursive: true, force: true });
}, 15000);
// Pi discovers global skills from ~/.pi/agent/skills/, not ~/.pi/skills/ (#327).
// Also covers the GLOBAL_HARNESS_HINTS detection: no --providers is passed, so
// the ~/.pi dir alone must route the install to Pi's agent skills path.
test('global install detects ~/.pi and writes Pi skills to ~/.pi/agent/skills', () => {
const tmp = mkdtempSync(join(tmpdir(), 'imp-test-scope-user-pi-'));
const home = mkdtempSync(join(tmpdir(), 'imp-home-scope-user-pi-'));
execSync('git init', { cwd: tmp });
mkdirSync(join(home, '.pi'), { recursive: true });
const bundleRoot = createFakeUniversalBundle(tmp, ['.pi']);
const output = run('skills install -y --scope=global --no-hooks', {
cwd: tmp,
env: { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot },
});
expect(output).toContain('Installed impeccable into: .pi (global)');
expect(existsSync(join(home, '.pi', 'agent', 'skills', 'impeccable', 'SKILL.md'))).toBe(true);
expect(existsSync(join(home, '.pi', 'skills', 'impeccable'))).toBe(false);
expect(existsSync(join(tmp, '.pi', 'skills', 'impeccable', 'SKILL.md'))).toBe(false);
rmSync(tmp, { recursive: true, force: true });
rmSync(home, { recursive: true, force: true });
}, 15000);
// Project scope must stay at .pi/skills/ even when the git root IS the home
// dir (dotfiles repos), where scope can't be inferred from the path alone.
// An existing global install at ~/.pi/agent/skills must not swallow the
// project-scope request into its already-installed refresh path.
test('project-scope install keeps Pi skills in .pi/skills even for a home-rooted repo', () => {
const home = mkdtempSync(join(tmpdir(), 'imp-home-rooted-project-pi-'));
execSync('git init', { cwd: home });
writeSkill(join(home, '.pi'), 'agent', 'impeccable');
const bundleRoot = createFakeUniversalBundle(home, ['.pi']);
const output = run('skills install -y --providers=pi --no-hooks', {
cwd: home,
env: { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot },
});
expect(output).toContain('Installed impeccable into: .pi (project)');
expect(existsSync(join(home, '.pi', 'skills', 'impeccable', 'SKILL.md'))).toBe(true);
// The pre-existing global copy is untouched, not refreshed in place.
expect(readFileSync(join(home, '.pi', 'agent', 'skills', 'impeccable', 'SKILL.md'), 'utf8')).toContain('name: impeccable');
expect(readFileSync(join(home, '.pi', 'agent', 'skills', 'impeccable', 'SKILL.md'), 'utf8')).not.toContain('Local deterministic bundle');
// An unscoped update from the same root must refresh BOTH Pi trees, not
// just the first layout it finds.
run('skills update -y --no-hooks', {
cwd: home,
env: { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot },
});
expect(readFileSync(join(home, '.pi', 'skills', 'impeccable', 'SKILL.md'), 'utf8')).toContain('Local deterministic bundle');
expect(readFileSync(join(home, '.pi', 'agent', 'skills', 'impeccable', 'SKILL.md'), 'utf8')).toContain('Local deterministic bundle');
rmSync(home, { recursive: true, force: true });
}, 15000);
test('honors an existing hook in shared settings.json and never duplicates into settings.local.json', () => {
const tmp = mkdtempSync(join(tmpdir(), 'imp-test-local-shared-hook-'));
execSync('git init', { cwd: tmp });