diff --git a/CHANGELOG.md b/CHANGELOG.md index 10b0043..ad16226 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,20 @@ note in the 0.9.0 entry. `files` whitelist, so packaged installs shipped no skills; `commands/` dropped from the whitelist with the migration). +### Fixed + +- **Install verification now fails on a blank/partial skill surface** (#235, + from the #230 audit). Since #228 the agent skills are the sole command + surface, but `recall_verify_install` and `recall doctor` derived their probes + from whatever canonicals existed with no lower bound — a bad npm pack, partial + checkout, or an interrupt before the skill copy step yielded zero probes and + install printed "All symlinks verified" over an empty surface. + `recall_verify_install` now asserts a floor (source ships skill dirs ⇒ the + canonical count must match), and `recall doctor` WARNs when the install root + exists with zero skill canonicals. `tests/install/npm-pack.test.ts` now + asserts all 9 `agent-skills//SKILL.md` are packed and `commands/` is + excluded; the install-sentinel unconditional-step guard now covers `Skills`. + ## [0.9.2] — 2026-07-13 — "hybrid search at scale" ### Added diff --git a/lib/install-lib.sh b/lib/install-lib.sh index 5e045c6..d21967d 100644 --- a/lib/install-lib.sh +++ b/lib/install-lib.sh @@ -1344,6 +1344,31 @@ recall_verify_install() { _check_symlink "$CLAUDE_DIR/MEMORY/extract_prompt.md" "$RECALL_SHARED_DIR/extract_prompt.md" fi + # Skill command surface floor (#235). Since #228 the agent skills are the + # SOLE command surface, and the loop above derives its probes from whatever + # canonicals happen to exist — so a bad npm pack, partial checkout, or an + # interrupt before _recall_copy_skill_files yields zero/fewer probes, `missing` + # stays empty, and verification would pass green on a blank surface. Assert a + # floor instead: if the source tree ships skill dirs, the canonical dir must + # hold at least as many, else the copy step didn't complete — fail red. The + # glob matches directories only, so agent-skills/AGENTS.md never inflates the + # source count (the copy loop excludes it identically). + local skills_src="$RECALL_REPO_DIR/agent-skills" + if [[ -d "$skills_src" ]]; then + local sd src_count=0 canon_count=0 + for sd in "$skills_src"/*/; do + [[ -d "$sd" ]] && src_count=$((src_count + 1)) + done + if [[ -d "$RECALL_SHARED_SKILLS_DIR" ]]; then + for sd in "$RECALL_SHARED_SKILLS_DIR"/*/; do + [[ -d "$sd" ]] && canon_count=$((canon_count + 1)) + done + fi + if [[ $src_count -gt 0 ]] && [[ $canon_count -lt $src_count ]]; then + missing+=("agent skill canonicals: found $canon_count of $src_count under $RECALL_SHARED_SKILLS_DIR (command surface incomplete)") + fi + fi + if [[ ${#missing[@]} -eq 0 ]] && [[ ${#wrong_target[@]} -eq 0 ]]; then return 0 fi diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index a244470..07ababf 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -470,6 +470,24 @@ export interface SymlinkProbe { canonical: string; // path the symlink should point at } +// Enumerate the depth-1 skill canonical files (name/file pairs) under the +// install root's shared/skills dir. Shared by buildSymlinkProbes (to derive +// per-file symlink probes) and probeSkillSurface (to floor the command surface, +// #235) so both agree on what counts as a shipped skill file. +function listSkillCanonicalFiles(root: string): { name: string; file: string }[] { + const skillsCanonicalDir = join(root, 'shared', 'skills'); + if (!existsSync(skillsCanonicalDir)) return []; + let skillNames: string[] = []; + try { skillNames = readdirSync(skillsCanonicalDir); } catch { return []; } + const out: { name: string; file: string }[] = []; + for (const name of skillNames) { + let files: string[] = []; + try { files = readdirSync(join(skillsCanonicalDir, name)); } catch { continue; } + for (const file of files) out.push({ name, file }); + } + return out; +} + function buildSymlinkProbes(): SymlinkProbe[] { const home = homedir(); const root = join(home, '.agents', 'Recall'); @@ -502,27 +520,39 @@ function buildSymlinkProbes(): SymlinkProbe[] { // recall_copy_canonical but never reached recall_link); covering it here // lets `recall doctor --fix` repair without reinstall. (The former slash // commands migrated to these skills — #228.) - const skillsCanonicalDir = join(root, 'shared', 'skills'); - if (existsSync(skillsCanonicalDir)) { - let skillNames: string[] = []; - try { skillNames = readdirSync(skillsCanonicalDir); } catch { skillNames = []; } - for (const name of skillNames) { - const skillDir = join(skillsCanonicalDir, name); - let files: string[] = []; - try { files = readdirSync(skillDir); } catch { continue; } - for (const file of files) { - probes.push({ - label: `agent skill: ${name}/${file}`, - target: join(home, '.claude', 'skills', name, file), - canonical: join(skillDir, file), - }); - } - } + for (const { name, file } of listSkillCanonicalFiles(root)) { + probes.push({ + label: `agent skill: ${name}/${file}`, + target: join(home, '.claude', 'skills', name, file), + canonical: join(root, 'shared', 'skills', name, file), + }); } return probes; } +// Skill command surface floor (#235). Since #228 the agent skills are the sole +// command surface, and buildSymlinkProbes derives probes from whatever +// canonicals exist — so a missing/empty shared/skills dir yields zero probes and +// doctor stays green on a blanked surface. Surface it as a WARN when the install +// root exists but no skill canonicals are present. Exported + path-injected for +// unit testing, mirroring probeInstallSentinel. +export function probeSkillSurface(root: string): CheckResult { + const label = 'Agent skill command surface'; + if (!existsSync(root)) { + return { label, status: 'INFO', message: 'Recall install root not present — skipping skill surface check' }; + } + const count = listSkillCanonicalFiles(root).length; + if (count === 0) { + return { + label, + status: 'WARN', + message: 'No agent skill canonicals under shared/skills — the recall-* command surface is blank; re-run ./install.sh to converge', + }; + } + return { label, status: 'PASS', message: `${count} agent skill file(s) present` }; +} + function hashFile(path: string): string | null { try { const buf = readFileSync(path); @@ -889,6 +919,11 @@ export async function runDoctor(opts: DoctorOptions = {}): Promise { } } + // Skill command surface floor (#235): the per-file probes above go silent + // when zero skill canonicals exist, so add an explicit WARN for a blanked + // command surface — the sole command surface since #228. + results.push(probeSkillSurface(join(homedir(), '.agents', 'Recall'))); + // MCP env health (issue #28): the recall-memory registration must carry // env.RECALL_DB_PATH matching the resolved DB path, or the MCP server can // diverge from the CLI after the next Claude restart. Same repair contract as diff --git a/tests/commands/doctor-skill-surface.test.ts b/tests/commands/doctor-skill-surface.test.ts new file mode 100644 index 0000000..bec4a05 --- /dev/null +++ b/tests/commands/doctor-skill-surface.test.ts @@ -0,0 +1,42 @@ +// Unit tests for the skill-surface floor probe used by `recall doctor` (#235). +// probeSkillSurface() is pure with respect to its arg (the install root), so we +// drive it directly against a temp root rather than invoking runDoctor +// end-to-end — same approach as doctor-install-sentinel.test.ts. + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { probeSkillSurface } from '../../src/commands/doctor'; + +let root: string; + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'recall-doctor-skills-')); +}); + +afterEach(() => { + if (existsSync(root)) rmSync(root, { recursive: true, force: true }); +}); + +describe('probeSkillSurface', () => { + test('install root absent → INFO (nothing to check)', () => { + const r = probeSkillSurface(join(root, 'does-not-exist')); + expect(r.status).toBe('INFO'); + }); + + test('root present but zero skill canonicals → WARN (blank command surface)', () => { + const r = probeSkillSurface(root); + expect(r.status).toBe('WARN'); + expect(r.message).toContain('command surface'); + }); + + test('root with a skill canonical → PASS', () => { + const skillDir = join(root, 'shared', 'skills', 'recall-scout'); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), '# scout\n'); + const r = probeSkillSurface(root); + expect(r.status).toBe('PASS'); + expect(r.message).toContain('1 agent skill file'); + }); +}); diff --git a/tests/install/install-sentinel.test.ts b/tests/install/install-sentinel.test.ts index d5e9675..f8bc798 100644 --- a/tests/install/install-sentinel.test.ts +++ b/tests/install/install-sentinel.test.ts @@ -165,6 +165,22 @@ describe('install.sh completion sentinel (#27)', () => { expect(r.stderr).toContain('re-run ./install.sh'); // recovery via log_error → stderr }); + // ── Skill command surface floor (#235) ────────────────────────────────────── + // + // Since #228 the agent skills are the SOLE command surface. recall_verify_install + // derives its skill probes from whatever canonicals exist, so before this floor a + // partial/absent copy step (bad pack, partial checkout, interrupt) left `missing` + // empty and verification passed green on a blank surface. The fakeRepo fixture + // ships one source skill (agent-skills/recall-scout/SKILL.md) but no canonical was + // copied here, so the floor must fail red. + test('verify fails red when source ships skills but zero canonicals exist', () => { + const r = runDriver(['recall_verify_install']); + expect(r.status).not.toBe(0); + // Pin the distinctive floor message so an unrelated missing symlink can't + // green this test for the wrong reason. + expect(r.stderr).toContain('agent skill canonicals'); + }); + // ── DRY: one canonical recovery story ─────────────────────────────────────── test('recovery wording has a single source of truth (recall_print_recovery)', () => { @@ -188,7 +204,7 @@ describe('install.sh completion sentinel (#27)', () => { const src = readFileSync(INSTALL_SH, 'utf-8'); for (const step of [ 'Backup', 'Migrate', 'Installing', 'Building', 'Linking', 'Database', - 'MCP', 'Hooks', 'Guide', 'Commands', 'CLAUDE.md', + 'MCP', 'Hooks', 'Guide', 'Commands', 'Skills', 'CLAUDE.md', ]) { expect(src).toContain(`_step "${step}"`); } diff --git a/tests/install/npm-pack.test.ts b/tests/install/npm-pack.test.ts index 12c74f7..2e7fe88 100644 --- a/tests/install/npm-pack.test.ts +++ b/tests/install/npm-pack.test.ts @@ -66,6 +66,20 @@ describe('npm package: files whitelist', () => { expect(has('FOR_PI.md')).toBe(true); }); + // #235: the agent skills are the sole command surface since #228, so the pack + // must ship every one — a partial pack (the pre-#230 failure mode) silently + // blanks the surface. Enumerate all 9 explicitly rather than trusting the + // agent-skills/ prefix alone (which passes with just 1 of 9 present). + test('bundles all 9 agent-skills//SKILL.md', () => { + const skills = [ + 'recall-add', 'recall-doctor', 'recall-dump', 'recall-loa', 'recall-recent', + 'recall-scout', 'recall-search', 'recall-stats', 'recall-update', + ]; + for (const name of skills) { + expect(has(`agent-skills/${name}/SKILL.md`)).toBe(true); + } + }); + test('excludes dev / local bloat', () => { for (const bad of [ 'tests/', @@ -75,6 +89,7 @@ describe('npm package: files whitelist', () => { 'scripts/', '.agents/', '.claude/', + 'commands/', // #235: the migrated slash-command surface must not ship ]) { expect(hasUnder(bad)).toBe(false); }