Skip to content
2 changes: 2 additions & 0 deletions .archgate/adrs/ARCH-002-error-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ Use four exit codes with clear semantics:
- Write errors to stderr (via `logError()`), not stdout
- **Commands that don't require `.archgate/` SHOULD fall back to `process.cwd()`** when `findProjectRoot()` returns null — e.g., `session-context` reads from `~/.claude/projects/` and uses `process.cwd()` as its path key when no project is found
- **Handle `ExitPromptError` from Inquirer as user cancellation** — catch it in the top-level error boundary and exit with code 130 (SIGINT convention) without logging an error or sending to Sentry
- **Handle `UserError` in the top-level safety net** — `main().catch()` in `src/cli.ts` MUST check `err instanceof UserError` and treat it as an expected failure (`logError()` + exit 1, NO Sentry capture) before falling through to the exit-2 + `captureException()` path, mirroring `handleCommandError()`. Only non-`UserError` errors reaching `main().catch()` are internal errors for Sentry

### Don't

Expand All @@ -60,6 +61,7 @@ Use four exit codes with clear semantics:
- Don't exit with code 0 when an operation fails
- Don't use exit codes other than 0, 1, 2, or 130
- Don't send user-cancellation errors (e.g., `ExitPromptError` from Inquirer) to Sentry — filter them in `beforeSend`
- Don't send `UserError` to Sentry from any error handler — including the `main().catch()` safety net. `UserError` means the user (or their environment) needs to fix something; capturing it floods Sentry with non-bugs (incident CLI-5)

## Implementation Pattern

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ All commands that operate on `.archgate/` project resources MUST use `findProjec
### Do

- Use `findProjectRoot()` from `src/helpers/paths.ts` in all commands that read from `.archgate/`
- Check the return value for `null` and exit with a helpful error message
- **Commands that REQUIRE a project MUST use `requireProjectRoot()`** from `src/helpers/paths.ts` — it throws a `UserError` with the message "No .archgate/ directory found. Run `archgate init` first.", which the command's ARCH-012 error boundary turns into exit 1 with no Sentry capture. Commands that can operate without a project (e.g. `session-context` with its cwd fallback) keep `findProjectRoot()` and handle `null` themselves
- Check the return value for `null` and exit with a helpful error message (only when using `findProjectRoot()` directly)
- Pass the resolved `projectRoot` to `projectPaths()` for derived paths

### Don't

- Don't use `process.cwd()` to locate `.archgate/` in command files (except `init`)
- Don't define local `findProjectRoot()` variants — use the shared implementation
- Don't hand-roll the `if (!projectRoot) { logError(...); await exitWith(1); return; }` guard in commands that require a project — that block was copy-pasted across ~10 command files before `requireProjectRoot()` replaced it
- Don't assume the user is running from the project root

## Consequences
Expand Down
6 changes: 4 additions & 2 deletions .archgate/adrs/ARCH-012-command-error-boundaries.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,15 @@ The top-level `main().catch()` in `cli.ts` remains as a safety net for truly une
### Do

- Wrap every async command action body in a try-catch
- **Cover the ENTIRE action body** — the try block MUST start at the first statement of the action and end at the last. A boundary that wraps only part of the body (e.g., a single risky call) still lets errors from the uncovered statements escape to `main().catch()`, converting expected failures (exit 1) into internal crashes (exit 2 + Sentry). Incident: `check.ts` once wrapped only `loadRuleAdrs()` — a `UserError` thrown later by `runChecks()` escaped and was reported to Sentry (issue CLI-5)
- Use `logError()` for error messages in the catch block
- Exit with code 1 for expected failures
- **Re-throw `ExitPromptError` in command error boundaries** — Commands that use Inquirer prompts (directly or via helpers like `promptEditorSelection`) MUST re-throw `ExitPromptError` from the catch block so `main().catch()` handles Ctrl+C with exit code 130. Pattern: `if (err instanceof Error && err.name === "ExitPromptError") throw err;`

### Don't

- Don't rely on `main().catch()` as the only error handler for commands
- Don't scope the try-catch to a subset of the action body — partial boundaries pass the automated presence check while still leaking errors from uncovered statements
- Don't catch and silently swallow errors — always log them
- Don't exit with code 2 in command catch blocks — that code is reserved for unexpected crashes
- Don't catch `ExitPromptError` as a command failure — it represents user cancellation (Ctrl+C), not an error. Let it propagate to `main().catch()` for exit code 130 handling (see [ARCH-002](./ARCH-002-error-handling.md))
Expand All @@ -84,12 +86,12 @@ The top-level `main().catch()` in `cli.ts` remains as a safety net for truly une

### Automated Enforcement

- **Archgate rule** `ARCH-012/async-action-error-boundary`: Scans async command actions for try-catch blocks. Severity: `warning` (some commands may have valid reasons for alternative patterns).
- **Archgate rule** `ARCH-012/async-action-error-boundary`: Walks the AST (`ctx.ast`) of each async command action and enforces two things: (1) the action body contains a top-level try-catch, and (2) no top-level awaited statement sits _outside_ that try block — escaped awaits are the exact statements whose rejections bypass the boundary into `main().catch()` (incident CLI-5). Awaits of the sanctioned exit paths (`exitWith`, `handleCommandError`) are exempt — they end in `process.exit()` and cannot produce a meaningful rejection, so early-return guards remain allowed. Severity: `warning` (some commands may have valid reasons for alternative patterns). **Remaining limitation:** synchronous statements outside the try are not flagged — sync throws from prelude code (e.g. argument validation) still escape; keep preludes trivial or move them inside the try.
- **Archgate rule** `ARCH-012/exit-prompt-error-rethrow`: Verifies that async command actions with try-catch blocks include the `ExitPromptError` re-throw pattern. Severity: `error` — missing re-throws silently convert user cancellation (Ctrl+C, exit 130) into command failure (exit 1).

### Manual Enforcement

Code reviewers MUST verify that new async commands include error boundaries.
Code reviewers MUST verify that new async commands include error boundaries AND that the try block covers the entire action body — not just the statements the author expected to fail.

## References

Expand Down
226 changes: 210 additions & 16 deletions .archgate/adrs/ARCH-012-command-error-boundaries.rules.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,151 @@
/// <reference path="../rules.d.ts" />

/**
* ARCH-012 enforcement, rewritten on top of ctx.ast() (ARCH-022).
*
* The previous implementation only regex-detected the PRESENCE of a try-catch
* inside an async action. That let partial boundaries pass: src/commands/
* check.ts once wrapped only loadRuleAdrs() in try/catch, and a UserError
* thrown later by runChecks() escaped to main().catch(), where it was
* miscaptured to Sentry with exit 2 (incident CLI-5). These rules now walk
* the ESTree produced by ctx.ast(file, "typescript"): the boundary rule
* additionally flags top-level awaited statements that sit OUTSIDE the
* action's try block — the exact statements whose rejections escape the
* boundary.
*/

/** Node types whose bodies run in their own context — awaits inside them are
* not executed at the action's top level, so don't descend into them. */
const FUNCTION_NODE_TYPES = new Set([
"FunctionDeclaration",
"FunctionExpression",
"ArrowFunctionExpression",
]);

/**
* Sanctioned exit paths that may be awaited outside the boundary. Both end
* in process.exit() and swallow their own internal failures, so they cannot
* produce a meaningful escaped rejection — flagging the common early-return
* guard pattern (`if (!x) { logError(...); await exitWith(1); return; }`)
* would be pure noise.
*/
const EXEMPT_AWAITED_CALLEES = new Set(["exitWith", "handleCommandError"]);

/** Depth-first walk over an ESTree-shaped tree. */
function walk(node: unknown, visit: (n: EsTreeNode) => void): void {
if (Array.isArray(node)) {
for (const item of node) walk(item, visit);
return;
}
if (!node || typeof node !== "object") return;
const n = node as EsTreeNode;
if (typeof n.type === "string") visit(n);
for (const value of Object.values(n)) {
if (value && typeof value === "object") walk(value, visit);
}
}

/** Callee name of a call expression node, or undefined for non-calls. */
function calleeName(node: EsTreeNode | undefined): string | undefined {
if (node?.type !== "CallExpression") return undefined;
const callee = node.callee as EsTreeNode | undefined;
if (callee?.type === "Identifier") return String(callee.name ?? "");
if (callee?.type === "MemberExpression") {
const property = callee.property as EsTreeNode | undefined;
if (property?.type === "Identifier") return String(property.name ?? "");
}
return undefined;
}

/**
* Does this statement contain an AwaitExpression executed at the statement's
* own level? Awaits inside nested function bodies are excluded — they belong
* to the nested function's execution, not the action body's control flow.
* Awaits of sanctioned exit paths (EXEMPT_AWAITED_CALLEES) are also excluded,
* though their arguments are still searched for nested non-exempt awaits.
*/
function containsDirectAwait(node: unknown): boolean {
if (Array.isArray(node))
return node.some((item) => containsDirectAwait(item));
if (!node || typeof node !== "object") return false;
const n = node as EsTreeNode;
if (typeof n.type === "string") {
if (n.type === "AwaitExpression") {
const arg = n.argument as EsTreeNode | undefined;
const name = calleeName(arg);
if (name === undefined || !EXEMPT_AWAITED_CALLEES.has(name)) {
return true;
}
// Exempt await — still search its arguments for nested awaits.
return containsDirectAwait(arg);
}
if (FUNCTION_NODE_TYPES.has(n.type)) return false;
}
for (const value of Object.values(n)) {
if (value && typeof value === "object" && containsDirectAwait(value)) {
return true;
}
}
return false;
}

/**
* Collect the async function bodies of every `<expr>.action(async ...)` call.
* Non-block bodies (implicit-return arrows like `.action(async () => run())`)
* are included too: they structurally cannot contain a try-catch, so the
* caller must flag them as missing the boundary rather than skip them.
*/
function findAsyncActionBodies(tree: EsTreeProgram): EsTreeNode[] {
const bodies: EsTreeNode[] = [];
walk(tree, (n) => {
if (n.type !== "CallExpression") return;
const callee = n.callee as EsTreeNode | undefined;
if (callee?.type !== "MemberExpression" || callee.computed === true) return;
const property = callee.property as EsTreeNode | undefined;
if (property?.type !== "Identifier" || property.name !== "action") return;
const handler = (n.arguments as EsTreeNode[] | undefined)?.[0];
if (
(handler?.type !== "ArrowFunctionExpression" &&
handler?.type !== "FunctionExpression") ||
handler.async !== true
) {
return;
}
const body = handler.body as EsTreeNode | undefined;
if (body) bodies.push(body);
});
return bodies;
}

/**
* Best-effort name of the awaited call for line lookup in the ORIGINAL
* source. ctx.ast(file, "typescript") parses Bun-transpiled output whose
* node.loc lines do not match the .ts source (see ARCH-022), so violations
* locate their line by searching the untranspiled text instead.
*/
function awaitedCalleeName(statement: EsTreeNode): string | undefined {
let name: string | undefined;
walk(statement, (n) => {
if (name !== undefined || n.type !== "AwaitExpression") return;
const candidate = calleeName(n.argument as EsTreeNode | undefined);
if (candidate !== undefined && !EXEMPT_AWAITED_CALLEES.has(candidate)) {
name = candidate;
}
});
return name;
}

/** Locate the 1-based line of `await <name>` in the original source. */
function findAwaitLine(source: string, name: string): number | undefined {
const lines = source.split("\n");
for (const [index, lineText] of lines.entries()) {
if (lineText.includes("await") && lineText.includes(name)) {
return index + 1;
}
}
return undefined;
}

export default {
rules: {
"async-action-error-boundary": {
Expand All @@ -13,25 +159,73 @@ export default {
);

const checks = files.map(async (file) => {
const content = await ctx.readFile(file);

// Find async action callbacks
const hasAsyncAction = /\.action\(\s*async\s/u.test(content);
if (!hasAsyncAction) return;

// Check if the async action body contains a try block
// Match: .action(async (...) => { ... try { ... } ... })
const hasTryCatch = /\.action\(\s*async\s[\s\S]*?\btry\s*\{/u.test(
content
);

if (!hasTryCatch) {
let tree: EsTreeProgram;
try {
tree = await ctx.ast(file, "typescript");
} catch (err) {
// Surface parse failures instead of silently treating the file
// as compliant — a transpiler edge case would otherwise mask
// coverage loss for this file.
ctx.report.warning({
message:
"Async command action should include a try-catch error boundary",
message: `Could not parse file for boundary analysis: ${
err instanceof Error ? err.message : String(err)
}`,
file,
fix: "Wrap the action body in try { ... } catch (err) { logError(...); process.exit(1); }",
fix: "Fix the parse error (or report it if the file is valid TypeScript) so ARCH-012 coverage analysis can run",
});
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Read once per file, outside the bodies loop — used for line
// lookup because AST loc refers to transpiled output (ARCH-022).
const source = await ctx.readFile(file);

for (const body of findAsyncActionBodies(tree)) {
// Implicit-return arrow bodies (`async () => run()`) can never
// contain a try-catch — an unavoidable missing boundary.
if (body.type !== "BlockStatement") {
ctx.report.warning({
message:
"Async command action uses an implicit-return arrow body, which cannot contain a try-catch error boundary",
file,
fix: "Convert to a block body: .action(async (...) => { try { ... } catch (err) { await handleCommandError(err); } })",
});
continue;
}
const statements = (body.body as EsTreeNode[] | undefined) ?? [];
const hasTopLevelTry = statements.some(
(s) => s.type === "TryStatement"
);

if (!hasTopLevelTry) {
ctx.report.warning({
message:
"Async command action should include a try-catch error boundary",
file,
fix: "Wrap the action body in try { ... } catch (err) { await handleCommandError(err); }",
});
continue;
}

// Coverage check: top-level awaited statements outside the try
// block reject straight past the boundary into main().catch(),
// converting expected failures (exit 1) into internal crashes
// (exit 2 + Sentry) — the CLI-5 incident pattern.
const escaped = statements.filter(
(s) => s.type !== "TryStatement" && containsDirectAwait(s)
);
for (const statement of escaped) {
const name = awaitedCalleeName(statement);
const line =
name === undefined ? undefined : findAwaitLine(source, name);
ctx.report.warning({
message: `Awaited statement${
name === undefined ? "" : ` (await ${name}(...))`
} sits outside the action's try-catch boundary — its rejection escapes to main().catch() as an internal crash`,
file,
...(line === undefined ? {} : { line }),
fix: "Move the statement inside the try block — the boundary must cover the entire action body (ARCH-012)",
});
}
}
});
await Promise.all(checks);
Expand Down
3 changes: 2 additions & 1 deletion .claude/agent-memory/archgate-developer/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv
- [Pick the right enforcement layer](feedback_prefer_tests_over_adr_rules.md) — static syntax → custom oxlint rule; executable behavior → tests; cross-file/governance → ADR `.rules.ts`
- [This repo is PUBLIC — no private sibling-repo internals in memory/PRs](feedback_public_repo_privacy.md)
- [Keep code comments and memory entries concise](feedback_concise_comments.md) — one line + terse why, link out for detail
- [Throw UserError in boundary-wrapped guards](feedback_throw_usererror_in_guards.md) — not logError + exitWith(1); the action's handleCommandError boundary does that

## Known Bugs

Expand All @@ -41,7 +42,7 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re
- [Test isolation gotchas](project_test_isolation_gotchas.md) — mock.module process-global leakage, Bun.env leaking across test files, Windows git-credential/GCM isolation, bun:sqlite EBUSY, macOS /var symlink, don't test PATH tools
- [Windows subprocess/path gotchas](project_windows_subprocess_gotchas.md) — Git Bash /tmp invisible to native tools, YAML backslash escaping, binary-upgrade `.old` cleanup, module-level `Bun.env` spread capture
- [CI workflow gotchas](project_ci_workflow_gotchas.md) — GITHUB_TOKEN pushes don't trigger workflows, secrets vs vars namespaces, jq CRLF on Windows Git Bash
- [Rules engine / command internals](project_rules_engine_internals.md) — Bun.Glob brace-pattern scan bug, commander option hoisting, cross-command I/O sharing pattern, verifying reviewer sub-agent ADR citations
- [Rules engine / command internals](project_rules_engine_internals.md) — Bun.Glob brace-pattern scan bug, commander option hoisting, cross-command I/O sharing pattern, verifying reviewer sub-agent ADR citations, dogfood+fire-test workflow for new .rules.ts
- [session-context --skip 1 inline-skill bug](project_session_context_skip_root_fix.md) — opencode fixed via top-level default + `--root`; other editors fixed with plain command; includes opencode.db inspection technique
- [CLI-skill flag sequencing across releases](project_cli_skill_flag_sequencing.md) — ship CLI first for flag additions, ship plugin promptly after for removals; installed lessons-learned skill v0.13.1 confirmed still broken
- [PR review thread triage](project_pr_review_thread_triage.md) — REST API doesn't expose resolved state; use GraphQL `reviewThreads.isResolved` to find genuinely outstanding comments
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
name: feedback-throw-usererror-in-guards
description: In command actions with a full error boundary, throw UserError instead of manual logError + exitWith(1)
metadata:
type: feedback
---

In command actions whose body is fully wrapped in try/catch → `handleCommandError`, early-return guards should `throw new UserError(...)` rather than `logError(...) + await exitWith(1) + return`.

**Why:** User review feedback on PR #467 (both guards in `check.ts`). The boundary already does logError + exit 1 without Sentry for UserError — the manual triple is redundant ceremony and drifts from the `user-error.ts` doctrine ("helpers throw UserError").

**How to apply:** When adding or touching a guard inside a boundary-wrapped action, prefer the throw. Note the test-shape difference: the exit spy then sees `exitWith(1, { errorKind: "user" })`, not `exitWith(1)`. Don't mass-convert other commands unprompted — apply opportunistically when editing them.
Loading