From def69e157b7c4d4c4dbef4d7bc31b3891fccc934 Mon Sep 17 00:00:00 2001 From: digitallamb Date: Mon, 10 Aug 2026 22:47:56 -0700 Subject: [PATCH] fix(cli): use imported `resolve`/`sep` in hermesGlobalHome (#521) The function called `path.resolve` and `path.sep` but only named- imports `resolve` and `sep` from `node:path`. The ReferenceError was swallowed by the try/catch, so $HERMES_HOME was silently ignored and profile-scoped installs always landed in ~/.hermes instead of the active profile. Greptile (P1) and Cursor Bugbot (High) flagged this on 2026-08-10. Adds 6 regression tests covering default, default- profile, active-profile, cross-home leakage, the override map integration, and the full e2e pipeline. Verified by reverting the fix and observing the relevant tests fail. --- cli/bin/commands/skills.mjs | 8 +- tests/skills-cli.test.js | 150 ++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 3 deletions(-) diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index e33490f76..1242c1058 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -102,13 +102,13 @@ function hermesGlobalHome(home) { const envHome = process.env.HERMES_HOME; if (envHome) { try { - const resolvedEnv = path.resolve(envHome); - const resolvedHome = path.resolve(home); + const resolvedEnv = resolve(envHome); + const resolvedHome = resolve(home); // Honor HERMES_HOME only when it lives under the active home (real // ~/.hermes or ~/.hermes/profiles/). Cross-home inheritance is // treated as not-set, so a test running under HOME=/tmp/... doesn't // pick up the developer's real ~/.hermes. - if (resolvedEnv === resolvedHome || resolvedEnv.startsWith(resolvedHome + path.sep)) { + if (resolvedEnv === resolvedHome || resolvedEnv.startsWith(resolvedHome + sep)) { return resolvedEnv; } } catch { @@ -2271,6 +2271,8 @@ export { expectedHookDests, extractZip, formatInstallDetectionLines, + hermesGlobalHome, + HOME_SKILLS_DIR_OVERRIDES, linkProviderSkills, mergeHookManifests, migrateUnprefixImpeccable, diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index 86ecc63d8..a10611325 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -1766,3 +1766,153 @@ describeRemote('skills install: production universal bundle download', () => { expect(skills).not.toContain('i-impeccable'); }, 90000); }); + +describe('hermesGlobalHome resolver (PR #521)', () => { + // hermesGlobalHome was added in PR #521 to honor $HERMES_HOME for + // profile-scoped installs. The original PR had a P1 bug at lines + // 105-111 of cli/bin/commands/skills.mjs: it called `path.resolve` and + // `path.sep` but the file only named-imports `resolve` and `sep` from + // `node:path`. The ReferenceError was swallowed by the catch block, so + // $HERMES_HOME was silently ignored and installs always landed in + // ~/.hermes regardless of the active profile. + // + // These tests exercise the real implementation (via the export + // added to the skills.mjs test surface), not a reimplementation. + + // The resolver is internal to skills.mjs. It reads $HERMES_HOME and + // returns the home dir it should use for ~/.hermes/skills. We import + // it via the public test surface — see the export block at the bottom + // of skills.mjs. + let hermesGlobalHome; + + beforeAll(async () => { + // Dynamic import so the test can use the same surface as the + // production code without forcing a re-export gymnastics on the + // rest of the test file. + const mod = await import('../cli/bin/commands/skills.mjs'); + hermesGlobalHome = mod.hermesGlobalHome; + }); + + test('default (no HERMES_HOME) returns /.hermes', () => { + // Use a fresh tmp HOME so the test never depends on the dev's real + // ~/.hermes leaking through. The `delete env.HERMES_HOME` happens + // in the caller; here we just verify the function honors an + // explicitly-unset env (process.env is set per test below). + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-default-')); + try { + expect(hermesGlobalHome(home)).toBe(join(home, '.hermes')); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + test('HERMES_HOME=/.hermes is honored (default profile)', () => { + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-real-')); + const prev = process.env.HERMES_HOME; + process.env.HERMES_HOME = join(home, '.hermes'); + try { + // The resolver returns $HERMES_HOME (resolved) when it lives + // under the active home. Callers append 'skills'. + expect(hermesGlobalHome(home)).toBe(join(home, '.hermes')); + } finally { + if (prev === undefined) delete process.env.HERMES_HOME; + else process.env.HERMES_HOME = prev; + rmSync(home, { recursive: true, force: true }); + } + }); + + test('HERMES_HOME=/.hermes/profiles/forge is honored (active profile)', () => { + // The whole point of the resolver: a Hermes invocation with + // HERMES_HOME pointing at an active profile should install into + // that profile's skills dir, not the default ~/.hermes. The + // original bug had install/update/check landing in ~/.hermes for + // every profile, which is the cross-profile data-corruption class + // that hermes_constants.py's active_profile fallback warning is + // designed to detect. + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-profile-')); + const prev = process.env.HERMES_HOME; + process.env.HERMES_HOME = join(home, '.hermes', 'profiles', 'forge'); + try { + const resolved = hermesGlobalHome(home); + expect(resolved).toBe(join(home, '.hermes', 'profiles', 'forge')); + // And critically: it must NOT fall back to the default profile + // when an active profile is selected. + expect(resolved).not.toBe(join(home, '.hermes')); + } finally { + if (prev === undefined) delete process.env.HERMES_HOME; + else process.env.HERMES_HOME = prev; + rmSync(home, { recursive: true, force: true }); + } + }); + + test('HERMES_HOME outside the active home is ignored (cross-home leakage guard)', () => { + // If the developer's shell has HERMES_HOME=/home/dev/.hermes and a + // test runs under HOME=/tmp/imp-home-xxx, the resolver must NOT + // pick up the dev's real ~/.hermes. Otherwise test output (and + // potentially writes) leak into the developer's working state. + // The cross-home guard turns the inherited HERMES_HOME into a + // not-set, so the resolver falls back to /.hermes. + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-xhome-')); + const otherHome = mkdtempSync(join(tmpdir(), 'imp-home-hermes-xhome-other-')); + const prev = process.env.HERMES_HOME; + process.env.HERMES_HOME = join(otherHome, '.hermes', 'profiles', 'main'); + try { + // HERMES_HOME is set but it doesn't sit under `home`, so the + // resolver should treat it as not-set and return /.hermes. + expect(hermesGlobalHome(home)).toBe(join(home, '.hermes')); + } finally { + if (prev === undefined) delete process.env.HERMES_HOME; + else process.env.HERMES_HOME = prev; + rmSync(home, { recursive: true, force: true }); + rmSync(otherHome, { recursive: true, force: true }); + } + }); + + test('HOME_SKILLS_DIR_OVERRIDES[".hermes"] returns /skills under an active profile', async () => { + // Integration check: the resolver is wired through the override + // map, so this is what the install path actually consumes. The + // import is cached across the suite (ESM module singleton), so + // the same `hermesGlobalHome` from the unit tests above applies + // here. We assert inside the async block so process.env is still + // set when the override function reads it (the unit-test version + // returns synchronously, but this one uses async import to share + // the module reference). + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-override-')); + const prev = process.env.HERMES_HOME; + process.env.HERMES_HOME = join(home, '.hermes', 'profiles', 'savant'); + try { + const mod = await import('../cli/bin/commands/skills.mjs'); + const override = mod.HOME_SKILLS_DIR_OVERRIDES['.hermes']; + expect(override(home)).toBe(join(home, '.hermes', 'profiles', 'savant', 'skills')); + } finally { + if (prev === undefined) delete process.env.HERMES_HOME; + else process.env.HERMES_HOME = prev; + rmSync(home, { recursive: true, force: true }); + } + }); + + test('end-to-end: --scope=user --providers=hermes with HERMES_HOME=profile lands in the active profile', () => { + // The full pipeline: drive the real CLI under a controlled HOME and + // HERMES_HOME. This catches any regression that breaks the wiring + // between hermesGlobalHome and the install path (e.g. if a future + // refactor moves the override out of HOME_SKILLS_DIR_OVERRIDES, or + // if copyProviderSkills stops reading from it). + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-hermes-e2e-')); + const home = mkdtempSync(join(tmpdir(), 'imp-home-hermes-e2e-')); + execSync('git init', { cwd: tmp }); + const bundleRoot = createFakeUniversalBundle(tmp, ['.hermes']); + const baseEnv = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot }; + delete baseEnv.HERMES_HOME; + const profileDir = join(home, '.hermes', 'profiles', 'forge'); + const env = { ...baseEnv, HERMES_HOME: profileDir }; + + run('skills install -y --providers=hermes --scope=user --no-hooks', { cwd: tmp, env }); + + // Landed in the active profile, not the default ~/.hermes. + expect(existsSync(join(profileDir, 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + expect(existsSync(join(home, '.hermes', 'skills', 'impeccable', 'SKILL.md'))).toBe(false); + + rmSync(tmp, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); + }, 20000); +});