Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .archgate/adrs/ARCH-002-error-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ Use four exit codes with clear semantics:
- Unexpected errors (exit code 2) may include stack traces when `DEBUG` or `TRACE` environment variables are set
- All error output goes to stderr, never stdout (stdout is reserved for command output and `--json` results)

**Broken output pipe (EPIPE):** a piped consumer closing early (`archgate adr list | head`) is success, not an error. The stream guards installed at CLI startup (`src/helpers/stream-guards.ts`) MUST exit 0 quietly on stdout EPIPE — no logging, no Sentry capture — swallow stderr EPIPE without exiting, and rethrow every other stream error so real bugs still crash loudly.

## Do's and Don'ts

### Do
Expand Down Expand Up @@ -142,6 +144,7 @@ Code reviewers MUST verify:
1. Error messages include actionable suggestions where possible
2. Expected failures exit with code 1, not code 2
3. No try-catch blocks that swallow errors without logging or re-throwing
4. `installStreamErrorGuards()` remains the first pre-main guard in `src/cli.ts` after the Bun check — stream `error` listeners must attach before any output is written

## References

Expand Down
1 change: 1 addition & 0 deletions .claude/agent-memory/archgate-developer/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code
- **Verify a review agent's claim before acting on it.** They misquote both ADRs and the files they have just read; `grep` the exact quoted string first. A governance finding citing no ADR cannot block on governance grounds — but a demonstrated defect blocks on its own merits.
- **A review agent's verdict on non-English prose is worthless**, and it invents the detail that supports it: an orthography pass over the pt-br docs returned PASS while asserting accents the words do not contain (`depreciadas` "(á)", `governança` "(ã)"). Grep cannot settle a claim about meaning, so verify mechanically instead — does the stripped form still occur, did fenced code blocks change — and leave the language judgement to a human speaker.
- **Reproduce a described failure before scheduling work from it** — including when your own scan reports zero. Of three issues one audit derived from memory files, two collapsed to nothing once the failure was actually tested (#517 Go proxy, #518 branch protection); the third was real but larger than described (#516). And prove a zero is a real zero: `\b` inside a JS template literal is a backspace, not a word boundary, so a regex built that way found no corruption where 69 occurrences sat.
- **Genuine OS-level EPIPE cannot be arranged from bun:test on Windows** — a spawned child's `stdout.cancel()` leaves the child's pipe open, and Git-Bash `cmd | true` pipelines may never break the pipe even unguarded (so a passing fire-test there is inconclusive, not proof). Test broken-pipe handling by synthesizing `process.stdout.emit("error", err)` with `code: "EPIPE"` — Bun's real delivery, re-emitted on every subsequent write — as in `tests/integration/stream-guards.test.ts`. A real break IS reproducible locally with a sustained writer piped to `head -c 100`.
- **Content filtering blocks policy/legal boilerplate** — generating a Contributor Covenant or license text can trip API filtering. Ask the user to copy it from the official source.
- **Files written under `/tmp` by this agent's own Bash/Write calls can vanish between tool calls** — observed for several scratch files with no deleting command run. Write anything that must survive several calls to a real Windows path instead (e.g. `C:/Users/<user>/AppData/Local/Temp/<task-name>/`); Bun/Node on Windows don't resolve Git-Bash-style `/c/Users/...` paths.
- **`archgate review-context`'s `--base` (auto-detect or explicit `origin/main`) diffs against the local `main`/`origin/main` ref, which can be stale-but-tree-identical after a squash merge** — same content, different commit hash, so it inflates `allChangedFiles` with every file from the last merged PR. Before trusting its output, `git fetch origin main:main`; the default fix is `git fetch origin` + explicit `--base origin/main` (also the only option in a git worktree, where `fetch origin main:main` is refused because `main` is checked out in the primary tree). Only when the current branch IS the stale `main` and `git diff origin/main HEAD --stat` is empty may `git reset origin/main` (never `--hard`) realign the pointer — it moves the branch and unstages any staged changes, so never run it on a feature branch with local commits.
Expand Down
7 changes: 7 additions & 0 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
flushSentry,
initSentry,
} from "./helpers/sentry";
import { installStreamErrorGuards } from "./helpers/stream-guards";
import {
flushTelemetry,
initTelemetry,
Expand All @@ -54,6 +55,12 @@ if (typeof Bun === "undefined")
"You need to run `archgate` with Bun. Do `bunx archgate [command]`"
);

// A piped consumer closing early (`archgate ... | head`, an agent harness
// tearing down) makes stream writes emit EPIPE `error` events; without
// listeners those escalate to fatal uncaught exceptions. Install before any
// output happens.
installStreamErrorGuards();

if (!semver.satisfies(Bun.version, ">=1.2.21")) {
logError(
"You need to update Bun to version 1.2.21 or higher.",
Expand Down
22 changes: 22 additions & 0 deletions src/helpers/exit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ export async function exitWith(
process.exit(code);
}

/**
* Quiet exit after the consumer of our output pipe went away (EPIPE). By
* pipeline convention (`archgate adr list | head`) a closed reader means
* "I have all I need" — success, not an error. Nothing is logged: the
* output channel is gone, and stderr may be too.
*
* @returns Typed `Promise<never>` — control never returns to the caller.
*/
export async function exitForBrokenPipe(): Promise<never> {
return exitWith(0, { outcome: "cancelled", errorKind: "broken_pipe" });
}

/**
* Centralized error handler for command catch blocks (ARCH-012). Helpers
* throw {@link UserError} for expected failures: those are logged and never
Expand Down Expand Up @@ -133,6 +145,16 @@ export async function handleCommandError(err: unknown): Promise<never> {
// Error classification
// ---------------------------------------------------------------------------

/**
* Whether an error is a broken-pipe (`EPIPE`) write failure — the reader
* side of stdout/stderr closed while the CLI was still writing.
*
* @param err - The error to inspect, of any shape.
*/
export function isEpipeError(err: unknown): boolean {
return err instanceof Error && "code" in err && err.code === "EPIPE";
}

/**
* Classify an error into a high-level bucket for telemetry.
*
Expand Down
79 changes: 79 additions & 0 deletions src/helpers/stream-guards.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 Archgate
/**
* Broken-pipe guards for `process.stdout`/`process.stderr`. When a piped
* reader closes early (`archgate adr list | head`, an agent harness tearing
* down), the next write emits an EPIPE `error` event — fatal without a
* listener. A closed stdout means "consumer is done" (quiet exit 0), a
* broken stderr is non-fatal, and every other stream error crashes loudly.
*/

import { exitForBrokenPipe, isEpipeError } from "./exit";

// ---------------------------------------------------------------------------
// Exit action (replaceable in tests)
// ---------------------------------------------------------------------------

function defaultBrokenPipeExit(): void {
// exitForBrokenPipe flushes telemetry over the network before exiting —
// safe with a dead stdout, since nothing is written to the streams.
void exitForBrokenPipe();
}

let brokenPipeExit: () => void = defaultBrokenPipeExit;
let exiting = false;

// ---------------------------------------------------------------------------
// Handlers
// ---------------------------------------------------------------------------

/**
* `error` listener for stdout. EPIPE means the consumer of the CLI's
* primary output is gone, so nothing further can be delivered: exit 0 (the
* pipeline convention). The `exiting` guard prevents double-firing while
* the async exit path (telemetry flush) is still in flight — Bun re-emits
* EPIPE on every subsequent write.
*
* @param err - The emitted stream error.
* @throws Non-EPIPE errors, so they escalate to an uncaught exception
* exactly as they would without the listener.
*/
export function handleStdoutError(err: unknown): void {
if (!isEpipeError(err)) throw err;
if (exiting) return;
exiting = true;
brokenPipeExit();
}

/**
* `error` listener for stderr. EPIPE is swallowed instead of exiting:
* diagnostics are best-effort, and the primary output channel (stdout) may
* still have a live consumer.
*
* @param err - The emitted stream error.
* @throws Non-EPIPE errors, preserving the loud-crash default.
*/
export function handleStderrError(err: unknown): void {
if (!isEpipeError(err)) throw err;
}

/**
* Attach both guards. Call once at CLI startup, before any output.
*/
export function installStreamErrorGuards(): void {
process.stdout.on("error", handleStdoutError);
process.stderr.on("error", handleStderrError);
}

// ---------------------------------------------------------------------------
// Testing helpers
// ---------------------------------------------------------------------------

/**
* Replace the broken-pipe exit action and reset the re-entrancy guard.
* Pass `null` to restore the default. For testing only.
*/
export function _setBrokenPipeExit(fn: (() => void) | null): void {
brokenPipeExit = fn ?? defaultBrokenPipeExit;
exiting = false;
}
25 changes: 25 additions & 0 deletions tests/helpers/exit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
beginCommand,
classifyErrorKind,
finalizeCommand,
isEpipeError,
_getExitState,
_resetExitState,
} from "../../src/helpers/exit";
Expand Down Expand Up @@ -67,6 +68,30 @@ describe("exit helper", () => {
});
});

describe("isEpipeError", () => {
test("matches an errno-style EPIPE write error", () => {
const err = Object.assign(new Error("EPIPE: broken pipe, write"), {
code: "EPIPE",
errno: -32,
syscall: "write",
});
expect(isEpipeError(err)).toBe(true);
});

test("rejects errors with other codes", () => {
const err = Object.assign(new Error("EACCES: permission denied"), {
code: "EACCES",
});
expect(isEpipeError(err)).toBe(false);
});

test("rejects errors without a code and non-Error values", () => {
expect(isEpipeError(new Error("EPIPE: broken pipe, write"))).toBe(false);
expect(isEpipeError("EPIPE")).toBe(false);
expect(isEpipeError(null)).toBe(false);
});
});

describe("classifyErrorKind", () => {
test("returns 'unknown' for non-Error values", () => {
expect(classifyErrorKind("string error")).toBe("unknown");
Expand Down
95 changes: 95 additions & 0 deletions tests/helpers/stream-guards.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 Archgate
import { afterEach, beforeEach, describe, expect, test } from "bun:test";

import {
handleStderrError,
handleStdoutError,
installStreamErrorGuards,
_setBrokenPipeExit,
} from "../../src/helpers/stream-guards";

function makeEpipeError(): NodeJS.ErrnoException {
return Object.assign(new Error("EPIPE: broken pipe, write"), {
code: "EPIPE",
errno: -32,
syscall: "write",
});
}

describe("stream-guards", () => {
let exitCalls: number;

beforeEach(() => {
exitCalls = 0;
_setBrokenPipeExit(() => {
exitCalls++;
});
});

afterEach(() => {
_setBrokenPipeExit(null);
});

describe("handleStdoutError", () => {
test("EPIPE triggers the broken-pipe exit action", () => {
handleStdoutError(makeEpipeError());
expect(exitCalls).toBe(1);
});

test("re-emitted EPIPE while exiting does not double-fire", () => {
// Bun re-emits EPIPE on every write attempt; only the first event
// may trigger the (async) exit path.
handleStdoutError(makeEpipeError());
handleStdoutError(makeEpipeError());
handleStdoutError(makeEpipeError());
expect(exitCalls).toBe(1);
});

test("non-EPIPE errors are rethrown without exiting", () => {
const err = Object.assign(new Error("EACCES: permission denied"), {
code: "EACCES",
});
expect(() => {
handleStdoutError(err);
}).toThrow("EACCES: permission denied");
expect(exitCalls).toBe(0);
});

test("non-Error values are rethrown", () => {
expect(() => {
handleStdoutError("not an error");
}).toThrow();
expect(exitCalls).toBe(0);
});
});

describe("handleStderrError", () => {
test("EPIPE is swallowed without exiting", () => {
expect(() => {
handleStderrError(makeEpipeError());
}).not.toThrow();
expect(exitCalls).toBe(0);
});

test("non-EPIPE errors are rethrown", () => {
const err = Object.assign(new Error("boom"), { code: "EIO" });
expect(() => {
handleStderrError(err);
}).toThrow("boom");
});
});

describe("installStreamErrorGuards", () => {
test("attaches the handlers as error listeners on both streams", () => {
installStreamErrorGuards();
try {
expect(process.stdout.listeners("error")).toContain(handleStdoutError);
expect(process.stderr.listeners("error")).toContain(handleStderrError);
} finally {
process.stdout.removeListener("error", handleStdoutError);
process.stderr.removeListener("error", handleStderrError);
}
});
});
});
Loading