fix(cli): harden adf-patch / serve / hook error paths surfaced by telemetry triage

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"

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');

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'),
+    });
+  });
+});

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[]) => {

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')),
+    });
+  });
+});

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);
 

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,26 +211,46 @@ 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',
     hookPath: normalizedHookPath,
   };
 }
 
+/**
+ * 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');
 }
 

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 <path>)`;
     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');
     }

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 extends B, B> = A;
+type _SchemaMatchesType = AssertAssignable<z.infer<typeof PatchOperationSchema>, PatchOperation>;
+type _TypeMatchesSchema = AssertAssignable<PatchOperation, z.infer<typeof PatchOperationSchema>>;