Skip to content

fix(agents): surface Claude CLI exit errors - #190

Merged
kunchenguid merged 4 commits into
kunchenguid:mainfrom
jasonqlwilliams-alt:fm/gnhf-claude-error-surface-c1
Aug 9, 2026
Merged

fix(agents): surface Claude CLI exit errors#190
kunchenguid merged 4 commits into
kunchenguid:mainfrom
jasonqlwilliams-alt:fm/gnhf-claude-error-surface-c1

Conversation

@jasonqlwilliams-alt

Copy link
Copy Markdown
Contributor

Intent

Fix issue #157 in the public repo kunchenguid/gnhf: the claude agent adapter swallows the agent's real error on a non-zero exit and reports an empty claude exited with code 1: with nothing after the colon. Users cannot tell an authentication failure from a bad model name from a rate limit, which is especially bad for an overnight tool where this is the only trace left behind in the morning.

Confirmed cause (verified end to end, not assumed): src/core/agents/claude.ts never captured stdout at all - parseJSONLStream silently drops unparseable lines, and the child 'close' handler built its reported detail from the accumulated stderr alone. isPermanentClaudeError read the same stderr-only string, so a permanent error reported on stdout was misclassified as retryable and burned retries. When the claude CLI reports its real failure on stdout (plain text, or a structured JSON result event), stderr is empty and the message degrades to the bare prefix.

Required changes:

  • Include the captured stdout (a BOUNDED tail, not unbounded) in the reported detail, and parse a structured error field out of it when the CLI emits JSON.
  • Reconsider whether permanent-error classification should read the same combined output rather than stderr alone, and fix it if so.
  • Keep the message useful when both streams are empty: say that the CLI exited with the code and produced no output, rather than trailing a bare colon.
  • Do NOT change behaviour on the success path.

Acceptance criteria:

  • An end-to-end reproduction shows the real error text after the fix (a unit test alone is not proof).
  • Tests cover a non-zero exit carrying its message on stdout, one carrying it on stderr, and one with both streams empty.
  • No change to unrelated adapters. The same stderr-only defect exists in the shared setupChildProcessHandlers in src/core/agents/stream-utils.ts used by codex/copilot/pi; it is DELIBERATELY out of scope per the acceptance criteria and was noted rather than widening the task. Do not flag that as an oversight.

Constraint from the user: gnhf is a public open-source project with active outside contributors, so keep the change tight and reviewable, follow the surrounding code's existing style, and do not reformat or refactor files that are not being fixed.

Work done and tradeoffs made:

  • Reproduced the empty message end to end BEFORE fixing, per the user's standing rule that bug fixes must be reproduced end to end first: a stub executable named 'claude' earlier on PATH printing an error payload and exiting non-zero, driving a real gnhf run against a temp git repo. Before: 'claude exited with code 1: ' in both the exit summary and gnhf.log. After: the real error text. All three variants (structured JSON stdout, plain-text stdout, both streams empty) verified end to end against the built dist/cli.mjs.
  • Bounded 4000-char stdout tail (appendBoundedTail) captured via a second passive 'data' listener on child.stdout alongside the existing parseJSONLStream consumer, rather than reworking parseJSONLStream - keeps the change local to claude.ts and avoids touching the shared helper used by other adapters.
  • extractStdoutError parses each stdout line as JSON and pulls the CLI's own message from 'error' (string or {message}) and from 'result'/'message'/'subtype' on events with is_error: true or type: 'error', falling back to a bounded raw tail so plain-text stdout is still surfaced.
  • formatExitFailure joins trimmed stderr with the stdout-derived text; when both are empty it emits 'claude exited with code and produced no output' instead of a trailing bare colon. The stderr-only wording is deliberately unchanged so existing behaviour and tests for that path are preserved.

A PRIOR no-mistakes review run already raised three ask-user findings on this branch. They were escalated and the decision came back to FIX all three, and those fixes are ALREADY APPLIED in the second commit on this branch ('no-mistakes(review): narrow claude permanent-error scope, bound raw tail, add e2e'). These are accepted, deliberate decisions - do not re-litigate them:

  1. permanent-classification-over-broad: isPermanentClaudeError is deliberately restricted to CLI-controlled text only - the stderr string plus the EXTRACTED STRUCTURED error message from stdout JSON. It deliberately does NOT run over the raw non-structured stdout tail, assistant message text, or tool_result payloads, because a retryable failure aborting the whole overnight run as permanent merely because the phrase appeared in agent stdout is a real regression that outweighs the narrower detection. A low credit balance arriving as a structured error field on stdout is still classified permanent.
  2. raw-jsonl-tail-in-notes: the unstructured raw fallback is deliberately elided to a short bounded tail (MAX_RAW_TAIL_CHARS, far smaller than the 4000-char capture bound) with an explicit truncation marker, because that text lands in notes.md and is replayed in every later iteration prompt. The full output remains in the iteration log.
  3. no-committed-e2e-regression: an executable e2e regression test was added using the repo's existing PATH-stub harness (e2e/e2e.test.ts prepends e2e/fixtures to PATH and drives the built dist/cli.mjs against a temp git repo, with e2e/fixtures/opencode as the precedent stub), covering the stdout-carried error and the both-streams-empty case, so the manual reproduction is now executable in CI.

Gates run locally on the first commit before it was committed: pnpm run typecheck, pnpm run lint, and pnpm test (673 tests across 42 files) all green.

What Changed

  • Capture a bounded Claude stdout tail on non-zero exits and surface structured JSON errors or concise plain-text output alongside stderr.
  • Classify structured stdout credit-balance errors as permanent while avoiding false positives from ordinary agent output, and report an explicit no-output message when both streams are empty.
  • Add unit and end-to-end coverage for stdout, stderr, combined-stream, bounded-output, permanent-error, and empty-output failure cases.

Risk Assessment

✅ Low: The change is tightly scoped to the Claude adapter, preserves the success path, bounds surfaced stdout, safely limits permanent-error classification to CLI-controlled text, and adds behavior-level unit and end-to-end coverage for the required failure modes.

Testing

The author-reported prior full-suite, lint, and typecheck baselines were not repeated in this targeted phase. I ran the complete Claude adapter unit file, built the CLI, ran the focused executable e2e regression, and manually exercised three end-user CLI runs covering stdout, stderr, and empty output. All checks passed, the persisted morning-after notes contain the expected useful messages, and the worktree was left clean.

Evidence: Structured stdout error persisted in notes.md
# gnhf run: verify-stdout-error-ad90ac

Objective: see .gnhf/runs/verify-stdout-error-ad90ac/prompt.md

## Iteration Log

### Iteration 1

**Summary:** [ERROR] claude exited with code 1: Invalid model name: claude-nonexistent-5
Evidence: Stderr error persisted in notes.md
# gnhf run: verify-stderr-error-f24578

Objective: see .gnhf/runs/verify-stderr-error-f24578/prompt.md

## Iteration Log

### Iteration 1

**Summary:** [ERROR] claude exited with code 1: Invalid API key - please run /login
Evidence: Empty-stream fallback persisted in notes.md
# gnhf run: verify-no-output-sur-b4bf70

Objective: see .gnhf/runs/verify-no-output-sur-b4bf70/prompt.md

## Iteration Log

### Iteration 1

**Summary:** [ERROR] claude exited with code 1 and produced no output

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • Inspected git diff 2c9f3bfcb519e99731a15ce8df7460fbfc904fb6..050d4aa29b6e732fe245650a0d6e9a4064a009f8 for scope and acceptance coverage.
  • pnpm exec vitest run src/core/agents/claude.test.ts
  • pnpm run build
  • pnpm exec vitest run e2e/e2e.test.ts -t "surfaces the claude CLI's own failure text"
  • Ran three real built-CLI sessions against temporary git repositories with the PATH-based Claude stub using GNHF_MOCK_CLAUDE_MODE=stdout-error, stderr-error, and no-output; inspected and preserved each generated notes.md.
  • git status --short --ignored and git diff --check after removing generated worktree artifacts.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

Jason Williams added 4 commits August 9, 2026 00:18
The claude adapter built its failure detail from stderr alone, so a CLI
that reports its error on stdout (plain text or a structured JSON result
event) degraded to a bare `claude exited with code 1: ` with nothing
after the colon. Permanent-error classification read the same stderr-only
string, so a low credit balance reported on stdout burned retries.

Capture a bounded stdout tail, extract the CLI's own error text from it
when the output is JSON, classify permanence against the combined detail,
and say the CLI produced no output when both streams are empty.

Closes kunchenguid#157
@kunchenguid
kunchenguid merged commit 3041614 into kunchenguid:main Aug 9, 2026
5 checks passed
@kunchenguid

Copy link
Copy Markdown
Owner

Thanks @jasonqlwilliams-alt - merged! Really appreciate the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants