diff --git a/packages/cli/package.json b/packages/cli/package.json index 29a729d..eb99194 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -62,7 +62,8 @@ "@stackbilt/policies": "workspace:*", "@stackbilt/surface": "workspace:*", "@stackbilt/types": "workspace:*", - "@stackbilt/validate": "workspace:*" + "@stackbilt/validate": "workspace:*", + "zod": "^3.24.1" }, "license": "Apache-2.0", "author": "Stackbilt LLC" diff --git a/packages/cli/src/__tests__/adf-patch.test.ts b/packages/cli/src/__tests__/adf-patch.test.ts index a9f7023..cd3c2a2 100644 --- a/packages/cli/src/__tests__/adf-patch.test.ts +++ b/packages/cli/src/__tests__/adf-patch.test.ts @@ -163,6 +163,33 @@ describe('adf patch JSON changes array', () => { expect(fs.readFileSync(file, 'utf-8')).not.toContain('Prefer immutability'); }); + it('malformed ops: null array element is rejected cleanly, not as a TypeError', async () => { + const dir = tmpDir(); + const file = path.join(dir, 'core.adf'); + fs.writeFileSync(file, METRIC_ADF); + + // Previously crashed in the before-capture pass with an uncaught + // "Cannot read properties of null (reading 'op')" TypeError. + await expect(adfCommand(baseOptions, ['patch', file, '--ops', '[null]'])).rejects.toMatchObject({ + name: 'CLIError', + message: expect.stringContaining('Invalid --ops operation'), + }); + // The file must be left untouched when validation fails. + expect(fs.readFileSync(file, 'utf-8')).toBe(METRIC_ADF); + }); + + it('malformed ops: non-object and unknown-op elements are rejected with CLIError', async () => { + const dir = tmpDir(); + const file = path.join(dir, 'core.adf'); + fs.writeFileSync(file, METRIC_ADF); + + for (const bad of ['[123]', '[{"section":"x","index":0}]', '[{"op":"FROBNICATE","section":"x"}]']) { + await expect(adfCommand(baseOptions, ['patch', file, '--ops', bad])).rejects.toMatchObject({ + name: 'CLIError', + }); + } + }); + it('error: returns patched:false with error message, no changes', async () => { const dir = tmpDir(); const file = path.join(dir, 'core.adf'); diff --git a/packages/cli/src/__tests__/hook-git-error.test.ts b/packages/cli/src/__tests__/hook-git-error.test.ts new file mode 100644 index 0000000..6e43c79 --- /dev/null +++ b/packages/cli/src/__tests__/hook-git-error.test.ts @@ -0,0 +1,36 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { CLIOptions } from '../index'; +import { hookCommand } from '../commands/hook'; + +// Simulate a git environment where the repo check passes but resolving the +// hooks directory fails (e.g. `git rev-parse --git-dir` errors). This path +// previously escaped as a raw `Error`; it must now surface as a clean CLIError. +// hook.ts consumes only these three helpers from git-helpers. +vi.mock('../git-helpers', () => ({ + isGitRepo: () => true, + runGit: (args: string[]) => { + throw Object.assign(new Error(`git ${args.join(' ')} failed`), { + stderr: 'fatal: not a git repository', + }); + }, + getGitErrorMessage: (err: unknown) => { + const e = err as Error & { stderr?: string }; + return e?.stderr?.trim() || e?.message || 'Unknown git error.'; + }, +})); + +const baseOptions: CLIOptions = { + configPath: '.charter', + format: 'text', + ciMode: false, + yes: false, +}; + +describe('hook install — git resolution failure', () => { + it('surfaces a CLIError (not a raw Error) when the hooks dir cannot be resolved', async () => { + await expect(hookCommand(baseOptions, ['install', '--commit-msg'])).rejects.toMatchObject({ + name: 'CLIError', + message: expect.stringContaining('Could not resolve git hooks directory'), + }); + }); +}); diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 826f07f..3312793 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -57,6 +57,21 @@ describe('hookCommand', () => { expect(content).toContain('echo "custom"'); }); + it('surfaces a CLIError (not a raw fs Error) when the hook file cannot be written', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + + // Point the hooks dir at an existing regular file so creating the hooks + // directory fails for real — no fs mocking, exercises the actual guard. + const blocker = path.join(tempDir, 'blocker'); + fs.writeFileSync(blocker, 'i am a file, not a directory'); + execFileSync('git', ['config', 'core.hooksPath', blocker], { stdio: 'ignore' }); + + await expect(hookCommand(baseOptions, ['install', '--commit-msg'])).rejects.toMatchObject({ + name: 'CLIError', + message: expect.stringContaining('Could not write git hook'), + }); + }); + it('hook print --claude returns 0 and outputs UserPromptSubmit config', async () => { const logs: string[] = []; vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { diff --git a/packages/cli/src/__tests__/serve-context.test.ts b/packages/cli/src/__tests__/serve-context.test.ts index 10fe3ed..c4a42e6 100644 --- a/packages/cli/src/__tests__/serve-context.test.ts +++ b/packages/cli/src/__tests__/serve-context.test.ts @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { CLIOptions } from '../index'; import { EXIT_CODE } from '../index'; import { CLIError } from '../index'; -import { loadCharterContextSnapshot } from '../commands/serve'; +import { loadCharterContextSnapshot, serveCommand } from '../commands/serve'; const contextRefreshCommandMock = vi.hoisted(() => vi.fn()); vi.mock('../commands/context-refresh', () => ({ @@ -102,3 +102,29 @@ describe('loadCharterContextSnapshot', () => { expect((result.snapshot as { version: number }).version).toBe(1); }); }); + +describe('serveCommand startup guards', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('names the resolved --ai-dir path when the directory is missing', async () => { + const missing = path.join(makeTempDir(), 'does-not-exist'); + vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + + await expect(serveCommand(baseOptions, ['--ai-dir', missing])).rejects.toMatchObject({ + name: 'CLIError', + message: expect.stringContaining(path.resolve(missing)), + }); + }); + + it('names the resolved manifest path when manifest.adf is missing', async () => { + const dir = makeTempDir(); // exists, but contains no manifest.adf + vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + + await expect(serveCommand(baseOptions, ['--ai-dir', dir])).rejects.toMatchObject({ + name: 'CLIError', + message: expect.stringContaining(path.join(path.resolve(dir), 'manifest.adf')), + }); + }); +}); diff --git a/packages/cli/src/commands/adf.ts b/packages/cli/src/commands/adf.ts index 568a339..08d43bd 100644 --- a/packages/cli/src/commands/adf.ts +++ b/packages/cli/src/commands/adf.ts @@ -28,6 +28,7 @@ import { NAMED_MODULE_SCAFFOLDS, NAMED_MODULE_DEFAULT_TRIGGERS, } from './adf-named-scaffolds'; +import { PatchOperationArraySchema } from '../schemas/patch-ops'; // Re-export named-scaffold registry for programmatic consumers and tests. export { @@ -631,17 +632,24 @@ function adfPatch(options: CLIOptions, args: string[]): number { const rawOps = opsFile ? readFlagFile(opsFile, '--ops-file') : opsJson!; - let ops: PatchOperation[]; + let parsed: unknown; try { - ops = JSON.parse(rawOps); - if (!Array.isArray(ops)) { - throw new Error('ops must be an array'); - } + parsed = JSON.parse(rawOps); } catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); throw new CLIError(`Invalid --ops JSON: ${msg}`); } + // Validate structure at the boundary so malformed ops surface as a clean + // error here rather than crashing the before-capture pass below. + const validation = PatchOperationArraySchema.safeParse(parsed); + if (!validation.success) { + const issue = validation.error.issues[0]; + const where = issue?.path.length ? ` at ops[${issue.path.join('.')}]` : ''; + throw new CLIError(`Invalid --ops operation${where}: ${issue?.message ?? 'does not match a known patch operation'}`); + } + const ops: PatchOperation[] = validation.data; + const input = fs.readFileSync(filePath, 'utf-8'); const doc = parseAdf(input); diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index 5865dd6..da649ab 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -8,7 +8,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { CLIOptions } from '../index'; import { CLIError, EXIT_CODE } from '../index'; -import { runGit, isGitRepo } from '../git-helpers'; +import { runGit, isGitRepo, getGitErrorMessage } from '../git-helpers'; interface HookInstallResult { status: 'INSTALLED' | 'SKIPPED'; @@ -183,12 +183,9 @@ function installCommitMsgHook(force: boolean): HookInstallResult { reason: 'existing commit-msg hook is not managed by Charter', }; } - } else { - fs.mkdirSync(hooksDir, { recursive: true }); } - fs.writeFileSync(hookPath, COMMIT_MSG_HOOK_CONTENT); - setExecutableBit(hookPath); + writeHookFile(hookPath, hooksDir, COMMIT_MSG_HOOK_CONTENT, exists); return { status: 'INSTALLED', @@ -214,12 +211,9 @@ function installPreCommitHook(force: boolean): HookInstallResult { reason: 'existing pre-commit hook is not managed by Charter', }; } - } else { - fs.mkdirSync(hooksDir, { recursive: true }); } - fs.writeFileSync(hookPath, PRE_COMMIT_HOOK_CONTENT); - setExecutableBit(hookPath); + writeHookFile(hookPath, hooksDir, PRE_COMMIT_HOOK_CONTENT, exists); return { status: 'INSTALLED', @@ -227,13 +221,36 @@ function installPreCommitHook(force: boolean): HookInstallResult { }; } +/** + * Create the hooks dir (when new) and write the hook file. Any filesystem + * failure (missing/unwritable hooks dir, permissions) surfaces as a clean + * CLIError rather than a raw Error escaping to the top-level handler. + */ +function writeHookFile(hookPath: string, hooksDir: string, content: string, exists: boolean): void { + try { + if (!exists) { + fs.mkdirSync(hooksDir, { recursive: true }); + } + fs.writeFileSync(hookPath, content); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new CLIError(`Could not write git hook to ${hookPath.replace(/\\/g, '/')}: ${msg}`); + } + setExecutableBit(hookPath); +} + function resolveHooksDir(): string { const configuredPath = getGitConfig('core.hooksPath'); if (configuredPath && configuredPath.trim().length > 0) { return path.resolve(configuredPath.trim()); } - const gitDir = runGit(['rev-parse', '--git-dir']).trim(); + let gitDir: string; + try { + gitDir = runGit(['rev-parse', '--git-dir']).trim(); + } catch (err) { + throw new CLIError(`Could not resolve git hooks directory: ${getGitErrorMessage(err)}`); + } return path.resolve(gitDir, 'hooks'); } diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index e3d8880..46eb424 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -74,14 +74,15 @@ export async function serveCommand(options: CLIOptions, args: string[]): Promise const customName = getFlag(args, '--name'); if (!fs.existsSync(aiDir)) { - const errMsg = `No .ai/ directory found. Run: charter init`; + const errMsg = `No .ai/ directory found at ${aiDir}. Run: charter init (or pass --ai-dir )`; if (transport === 'stdio') { process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32000, message: `charter serve: ${errMsg}` } }) + '\n'); } throw new CLIError(errMsg); } - if (!fs.existsSync(path.join(aiDir, 'manifest.adf'))) { - const errMsg = `.ai/manifest.adf not found. Run: charter adf init`; + const manifestPath = path.join(aiDir, 'manifest.adf'); + if (!fs.existsSync(manifestPath)) { + const errMsg = `ADF manifest not found at ${manifestPath}. Run: charter adf init`; if (transport === 'stdio') { process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32000, message: `charter serve: ${errMsg}` } }) + '\n'); } diff --git a/packages/cli/src/schemas/patch-ops.ts b/packages/cli/src/schemas/patch-ops.ts new file mode 100644 index 0000000..5839c59 --- /dev/null +++ b/packages/cli/src/schemas/patch-ops.ts @@ -0,0 +1,60 @@ +/** + * Zod schema for `adf patch --ops` input. + * + * Validates externally-supplied patch operations at the CLI boundary (the + * `--ops` JSON / `--ops-file` contents) before they reach the patch engine. + * Without this, a malformed op (e.g. a `null` array element) crashed the + * before-capture pass with an uncaught `TypeError` instead of a clean error. + * + * The schema is the runtime authority; `@stackbilt/adf` remains the source of + * truth for the `PatchOperation` *type*. The compile-time guard at the bottom + * fails the build if the two ever drift, without making adf depend on zod. + */ + +import { z } from 'zod'; +import type { PatchOperation } from '@stackbilt/adf'; + +const AdfContentSchema = z.discriminatedUnion('type', [ + z.object({ type: z.literal('text'), value: z.string() }), + z.object({ type: z.literal('list'), items: z.array(z.string()) }), + z.object({ + type: z.literal('map'), + entries: z.array(z.object({ key: z.string(), value: z.string() })), + }), + z.object({ + type: z.literal('metric'), + entries: z.array( + z.object({ + key: z.string(), + value: z.number(), + ceiling: z.number(), + unit: z.string(), + }) + ), + }), +]); + +export const PatchOperationSchema = z.discriminatedUnion('op', [ + z.object({ op: z.literal('ADD_BULLET'), section: z.string(), value: z.string() }), + z.object({ op: z.literal('REPLACE_BULLET'), section: z.string(), index: z.number(), value: z.string() }), + z.object({ op: z.literal('REMOVE_BULLET'), section: z.string(), index: z.number() }), + z.object({ + op: z.literal('ADD_SECTION'), + key: z.string(), + decoration: z.string().nullable().optional(), + content: AdfContentSchema, + weight: z.enum(['load-bearing', 'advisory']).optional(), + }), + z.object({ op: z.literal('REPLACE_SECTION'), key: z.string(), content: AdfContentSchema }), + z.object({ op: z.literal('REMOVE_SECTION'), key: z.string() }), + z.object({ op: z.literal('UPDATE_METRIC'), section: z.string(), key: z.string(), value: z.number() }), +]); + +export const PatchOperationArraySchema = z.array(PatchOperationSchema); + +// Compile-time drift guard: the inferred schema type and adf's public +// `PatchOperation` must be mutually assignable. If either side changes without +// the other, one of these aliases fails to resolve and the build breaks. +type AssertAssignable = A; +type _SchemaMatchesType = AssertAssignable, PatchOperation>; +type _TypeMatchesSchema = AssertAssignable>;