diff --git a/src/cli.ts b/src/cli.ts index 0c54238f..3e796632 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -136,7 +136,7 @@ const LINKED_CASES_FILE = dataPath('linked-cases.json'); * Sync and tolerant on purpose: a missing or malformed registry means "no linked * cases", never a crash. */ -function resolveCliCasePath(name: string): string { +export function resolveCliCasePath(name: string): string { try { const linked = JSON.parse(readFileSync(LINKED_CASES_FILE, 'utf-8')) as Record; const target = linked?.[name]; @@ -154,17 +154,30 @@ function resolveCliCasePath(name: string): string { * `resolveCliCasePath()` above. The web server's automatic per-case injection * (`agentSkillEnabled`) covers multi-user spaces; this CLI is a local operator tool * and stays single-user. + * + * A missing case is REPORTED, not exited on: the exit lives in the wrapper below so + * this resolution (including the linked-cases lookup, which shipped unguarded) can be + * unit-tested without `process.exit(1)` taking the test runner down with it. */ -function resolveSkillTarget(options: { case?: string }): string { +export function resolveSkillTargetPath(options: { + case?: string; +}): { target: string; missingCase?: undefined } | { target?: undefined; missingCase: string } { if (options.case) { const casePath = resolveCliCasePath(options.case); - if (!existsSync(casePath)) { - console.error(chalk.red(`✗ Case not found: ${casePath}`)); - process.exit(1); - } - return join(casePath, '.claude', 'skills', 'codeman'); + if (!existsSync(casePath)) return { missingCase: casePath }; + return { target: join(casePath, '.claude', 'skills', 'codeman') }; } - return join(homedir(), '.claude', 'skills', 'codeman'); + return { target: join(homedir(), '.claude', 'skills', 'codeman') }; +} + +/** Exit-owning wrapper around `resolveSkillTargetPath()` for the two commands below. */ +function resolveSkillTarget(options: { case?: string }): string { + const resolved = resolveSkillTargetPath(options); + if (resolved.missingCase !== undefined) { + console.error(chalk.red(`✗ Case not found: ${resolved.missingCase}`)); + process.exit(1); + } + return resolved.target; } /** Print an AgentSkillApplyResult for humans; exit non-zero when nothing was done. */ diff --git a/test/agent-skill-endpoints-doc.test.ts b/test/agent-skill-endpoints-doc.test.ts new file mode 100644 index 00000000..940f553c --- /dev/null +++ b/test/agent-skill-endpoints-doc.test.ts @@ -0,0 +1,88 @@ +/** + * @fileoverview Static guard: every endpoint the packaged agent skill documents + * still exists in the routes it is documenting. + * + * `skills/codeman/reference/endpoints.md` is injected into cases and read by agents + * driving Codeman over HTTP. Nothing tied it to the server, so renaming or dropping a + * route left the skill confidently telling agents to call a 404. This parses the + * `METHOD /api/...` pairs out of the doc and matches them against the `app.()` + * registrations in src/web/routes/*.ts. + * + * Precision over recall on purpose: only a bare uppercase verb followed by an + * `/api/...` path counts, so prose that merely mentions a path (the `.../sessions/null` + * jq-pitfall example) is ignored, and a spuriously failing guard does not get deleted + * by the next person. `/api/v1` is a URL-rewrite alias (server.ts), so the version + * segment is dropped before matching, and param NAMES are normalized away since the + * doc's `:id` need not match a route's `:sessionId`. + * + * Port: N/A (pure static analysis). + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { join } from 'node:path'; + +const HERE = fileURLToPath(new URL('.', import.meta.url)); +const DOC_PATH = join(HERE, '../skills/codeman/reference/endpoints.md'); +const ROUTES_DIR = join(HERE, '../src/web/routes'); + +/** `METHOD /api/`, stopping before a query string, backtick or prose. */ +const DOC_ENDPOINT = /\b(GET|POST|PUT|PATCH|DELETE)\s+\/(api\/[A-Za-z0-9_:/-]+)/g; +/** `app.get('/api/…'`, where the path may sit on its own line (case-routes.ts, file-routes.ts). */ +const ROUTE_REGISTRATION = /app\.(get|post|put|patch|delete)\(\s*'([^']+)'/g; + +/** + * Strip the `/api/v1` alias and replace param names with a placeholder, so + * `GET /api/v1/sessions/:id` and `app.get('/api/sessions/:sessionId')` compare equal. + */ +function normalize(method: string, path: string): string { + const withoutVersion = path.replace(/^\/api\/v1\//, '/api/'); + const params = withoutVersion.replace(/\/:[^/]+/g, '/:p').replace(/\/$/, ''); + return `${method.toUpperCase()} ${params}`; +} + +function documentedEndpoints(): string[] { + const markdown = readFileSync(DOC_PATH, 'utf-8'); + const found = new Set(); + for (const match of markdown.matchAll(DOC_ENDPOINT)) { + found.add(normalize(match[1], `/${match[2]}`)); + } + return [...found].sort(); +} + +function registeredRoutes(): Set { + const registered = new Set(); + for (const file of readdirSync(ROUTES_DIR)) { + if (!file.endsWith('.ts')) continue; + const source = readFileSync(join(ROUTES_DIR, file), 'utf-8'); + for (const match of source.matchAll(ROUTE_REGISTRATION)) { + if (!match[2].startsWith('/api/')) continue; + registered.add(normalize(match[1], match[2])); + } + } + return registered; +} + +describe('skills/codeman/reference/endpoints.md', () => { + it('parses a plausible number of endpoints out of the doc', () => { + // A parser that silently matches nothing would make the real assertion below + // pass vacuously forever. + const documented = documentedEndpoints(); + expect(documented.length).toBeGreaterThanOrEqual(10); + expect(documented).toContain('POST /api/quick-start'); + expect(documented).toContain('GET /api/sessions/:p/wait'); + }); + + it('finds the route registrations it matches against', () => { + const registered = registeredRoutes(); + expect(registered.size).toBeGreaterThan(100); + expect(registered.has('GET /api/status')).toBe(true); + }); + + it('documents only endpoints that are actually registered', () => { + const registered = registeredRoutes(); + const missing = documentedEndpoints().filter((endpoint) => !registered.has(endpoint)); + expect(missing).toEqual([]); + }); +}); diff --git a/test/cli-skill-target.test.ts b/test/cli-skill-target.test.ts new file mode 100644 index 00000000..c25f5776 --- /dev/null +++ b/test/cli-skill-target.test.ts @@ -0,0 +1,144 @@ +/** + * @fileoverview Tests for `codeman skill install|uninstall` target resolution + * (`resolveCliCasePath` / `resolveSkillTargetPath` in src/cli.ts). + * + * The linked-cases lookup shipped in 1.14.2 with no guard: before it, `--case` + * rejected every case linked in from outside `~/codeman-cases` with "Case not + * found" even though the server resolved the same name fine. These tests pin both + * halves of that resolution (registry first, cases dir as fallback) and the + * tolerance rules around a missing or malformed registry. + * + * `test/setup.ts` gives this file its own temporary HOME, so `homedir()` and + * `dataPath()` already point into a per-file fixture: no os mocking needed. + * Port: N/A (pure path resolution, no server). + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdirSync, rmSync, writeFileSync, existsSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; +import { dataPath } from '../src/config/instance.js'; +import { program, resolveCliCasePath, resolveSkillTargetPath } from '../src/cli.js'; + +const LINKED_CASES_FILE = dataPath('linked-cases.json'); +const CASES_DIR = join(homedir(), 'codeman-cases'); +const LINKED_ROOT = join(homedir(), 'elsewhere'); + +/** Where the packaged skill lands under a case. Mirrors `applyAgentSkill()`. */ +function skillDirIn(casePath: string): string { + return join(casePath, '.claude', 'skills', 'codeman'); +} + +function writeLinkedCases(content: string): void { + mkdirSync(dataPath(), { recursive: true }); + writeFileSync(LINKED_CASES_FILE, content, 'utf-8'); +} + +beforeEach(() => { + rmSync(LINKED_CASES_FILE, { force: true }); + rmSync(CASES_DIR, { recursive: true, force: true }); + rmSync(LINKED_ROOT, { recursive: true, force: true }); +}); + +afterEach(() => { + rmSync(LINKED_CASES_FILE, { force: true }); + rmSync(CASES_DIR, { recursive: true, force: true }); + rmSync(LINKED_ROOT, { recursive: true, force: true }); +}); + +describe('resolveSkillTargetPath (global)', () => { + it('targets the user-scope skill dir when no --case is given', () => { + expect(resolveSkillTargetPath({})).toEqual({ + target: join(homedir(), '.claude', 'skills', 'codeman'), + }); + }); + + it('never consults the linked-cases registry for the global target', () => { + // A registry entry named after nothing in particular must not divert the + // global install, which is not case-scoped at all. + writeLinkedCases(JSON.stringify({ anything: join(LINKED_ROOT, 'anything') })); + expect(resolveSkillTargetPath({}).target).toBe(join(homedir(), '.claude', 'skills', 'codeman')); + }); +}); + +describe('resolveCliCasePath / resolveSkillTargetPath (--case)', () => { + it('resolves a LINKED case through linked-cases.json, not the cases dir', () => { + // The 1.14.2 regression: a case linked in from outside ~/codeman-cases was + // resolved to a cases-dir path that does not exist, so install refused it. + const linkedPath = join(LINKED_ROOT, 'my-repo'); + mkdirSync(linkedPath, { recursive: true }); + writeLinkedCases(JSON.stringify({ 'my-repo': linkedPath })); + + expect(resolveCliCasePath('my-repo')).toBe(linkedPath); + expect(resolveSkillTargetPath({ case: 'my-repo' })).toEqual({ target: skillDirIn(linkedPath) }); + expect(existsSync(join(CASES_DIR, 'my-repo'))).toBe(false); + }); + + it('falls back to the cases dir for a name the registry does not list', () => { + const casePath = join(CASES_DIR, 'plain-case'); + mkdirSync(casePath, { recursive: true }); + writeLinkedCases(JSON.stringify({ 'other-case': join(LINKED_ROOT, 'other-case') })); + + expect(resolveCliCasePath('plain-case')).toBe(casePath); + expect(resolveSkillTargetPath({ case: 'plain-case' })).toEqual({ target: skillDirIn(casePath) }); + }); + + it('reports the resolved path instead of exiting when the case does not exist', () => { + // process.exit(1) lives in the CLI wrapper on purpose: calling it here would + // kill the test runner. + expect(resolveSkillTargetPath({ case: 'nope' })).toEqual({ missingCase: join(CASES_DIR, 'nope') }); + }); + + it('reports the LINKED path when the registry points at a directory that is gone', () => { + const linkedPath = join(LINKED_ROOT, 'moved-away'); + writeLinkedCases(JSON.stringify({ 'moved-away': linkedPath })); + + expect(resolveSkillTargetPath({ case: 'moved-away' })).toEqual({ missingCase: linkedPath }); + }); +}); + +describe('linked-cases.json tolerance', () => { + const casePath = () => join(CASES_DIR, 'tolerant'); + + beforeEach(() => { + mkdirSync(casePath(), { recursive: true }); + }); + + it('degrades to the cases dir when the registry file is absent', () => { + expect(existsSync(LINKED_CASES_FILE)).toBe(false); + expect(resolveSkillTargetPath({ case: 'tolerant' })).toEqual({ target: skillDirIn(casePath()) }); + }); + + it('degrades to the cases dir on malformed JSON rather than throwing', () => { + writeLinkedCases('{ not json at all'); + expect(() => resolveCliCasePath('tolerant')).not.toThrow(); + expect(resolveSkillTargetPath({ case: 'tolerant' })).toEqual({ target: skillDirIn(casePath()) }); + }); + + it('degrades to the cases dir when the registry is valid JSON of the wrong shape', () => { + // A null / array / non-string-valued entry must read as "no linked case", + // never as a target path. + for (const body of ['null', '[]', JSON.stringify({ tolerant: 42 }), JSON.stringify({ tolerant: '' })]) { + writeLinkedCases(body); + expect(resolveCliCasePath('tolerant')).toBe(casePath()); + } + }); +}); + +describe('skill command wiring', () => { + it('registers install and uninstall, both accepting --case and --global', () => { + const skill = program.commands.find((cmd) => cmd.name() === 'skill'); + expect(skill).toBeDefined(); + + const subcommands = skill!.commands.map((cmd) => cmd.name()); + expect(subcommands).toEqual(expect.arrayContaining(['install', 'uninstall'])); + + for (const name of ['install', 'uninstall']) { + const flags = skill!.commands + .find((cmd) => cmd.name() === name)! + .options.map((opt) => opt.long) + .sort(); + expect(flags).toEqual(['--case', '--global']); + } + }); +}); diff --git a/test/mocks/mock-route-context.ts b/test/mocks/mock-route-context.ts index 0b2ca63f..633655d9 100644 --- a/test/mocks/mock-route-context.ts +++ b/test/mocks/mock-route-context.ts @@ -15,7 +15,7 @@ import { resolveTerminalHistoryConfig } from '../../src/config/terminal-history. * Creates a mock context that satisfies all port interfaces. * Pre-populated with one session for convenience. */ -export function createMockRouteContext(options?: { sessionId?: string }) { +export function createMockRouteContext(options?: { sessionId?: string; agentSkillEnabled?: boolean }) { const sessionId = options?.sessionId ?? 'test-session-1'; const session = createMockSession(sessionId); const sessions = new Map(); @@ -86,7 +86,10 @@ export function createMockRouteContext(options?: { sessionId?: string }) { getModelConfig: vi.fn(async () => null), getClaudeModeConfig: vi.fn(async () => ({})), getTerminalHistoryConfig: vi.fn(async () => resolveTerminalHistoryConfig({})), - getAgentSkillEnabled: vi.fn(async () => false), + // Default OFF mirrors the shipped setting, so existing tests never touch a + // case's .claude/skills. Overridable per test because the create-time + // injection call sites are otherwise unreachable from a route test. + getAgentSkillEnabled: vi.fn(async () => options?.agentSkillEnabled ?? false), getDefaultClaudeMdPath: vi.fn(async () => undefined), getLightState: vi.fn(() => ({ sessions: [], status: 'ok' })), getLightSessionsState: vi.fn(() => { diff --git a/test/routes/session-routes-agent-skill.test.ts b/test/routes/session-routes-agent-skill.test.ts new file mode 100644 index 00000000..38dbdb50 --- /dev/null +++ b/test/routes/session-routes-agent-skill.test.ts @@ -0,0 +1,121 @@ +/** + * @fileoverview Agent-skill injection on POST /api/sessions (docs/agent-control-plan.md §2). + * + * The quick-start half of this is covered end-to-end in test/quick-start.test.ts; + * the plain create path had NO coverage, because the shared mock context hardcoded + * `getAgentSkillEnabled` to false and nothing could flip it. This exercises the real + * `applyAgentSkill` against a temp working dir, so it asserts bytes on disk rather + * than a spy call. + * + * Uses app.inject(), so no real HTTP port is needed. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import fastifyCookie from '@fastify/cookie'; +import { mkdtemp, rm, readFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; +import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; +import { registerSessionRoutes } from '../../src/web/routes/session-routes.js'; + +const MARKER_PREFIX = '