feat: unified error handling for all summary agents - #1005
Open
peyton-alt wants to merge 30 commits into
Open
Conversation
Introduces the shared typed error that every summary-capable provider's GenerateText will return on failure. Types only; the Classifier engine lands in a follow-up commit so this change is byte-safe to land alone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e9ccb88eea31
Adds the declarative types the per-agent classifiers will populate and the shared engine will consume. No behavior yet; Classify lands next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b54801a933f0
Mirrors 963's claudecode.isExecNotFound exactly so the shared classifier engine can preserve the same binary-not-found semantics across providers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: aceb86e01274
Implements the first two branches of the classification order. Envelope and phrase-matching follow in subsequent commits to keep each change reviewable on its own. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 90724971a8ed
Adds ParseEnvelope invocation (which takes precedence over stderr classification to match 963's behavior) and the provider-agnostic HTTP-status substring baseline (401/403/429/400/404). Per-agent phrase tables are checked after the baseline so any CLI that passes through an HTTP status in stderr gets uniform treatment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1d42f6886efd
Code review flagged that Classify was returning context.DeadlineExceeded even when the subprocess emitted an is_error envelope on stdout. 963's claudecode/generate.go:52-77 explicitly preferred the envelope so users don't lose actionable auth/rate-limit/config diagnostics when ctx and the subprocess fail concurrently. Reordered Classify, updated its doc comment, and pinned the behavior with a regression test. Also tightened matchPhrase to return (Kind, bool) instead of a pointer into the httpStatusBaseline slice — prevents accidental mutation of the shared package-level baseline via a returned pointer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2ecff3f35124
Tests case-insensitive first-match-wins, Unknown fallthrough preserving stderr verbatim, the empty-stderr "never empty" contract, and the 500-byte truncation inherited from 963. Also removes the now-redundant compile-time reference block — every declared type and stderrMessageMaxLen is now exercised by real tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6e48728667e6
Returns ExecResult with raw stdout/stderr/exitCode instead of a preformatted string error, so callers can feed these into Classifier.Classify. The old string-returning RunIsolatedTextGeneratorCLI stays in place until Chunk 7 so no provider is mid-migration when this lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3c98b100bea4
Protects against new callers landing on the soon-to-be-removed string helper during the provider migration window (Chunks 2-6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8c885c3d063e
This reverts commit afc6e4a. Entire-Checkpoint: 7ffd0f08dffd
Provides the shared formatter for *agent.TextGenError. Claude wording is byte-identical to 963 (pinned by behavior tests). Non-Claude providers prefer the CLI's own stderr verbatim; synthesis is generic-only (no invented subcommands) to prevent the failure mode flagged in the spec's Alternatives Considered section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: dcf0ce1a42ad
Code review flagged that the Claude Unknown-branch output diverged from
963's baseline wording ("Claude API" vs "Anthropic API"; capitalized
"Claude CLI" vs lowercase "claude CLI"). Accepting the normalization as
intentional (it's more internally consistent with the other Claude-prefixed
branches) and tightening the 4 Unknown-branch tests to pin the current
wording exactly. The load-bearing 963 messages (auth/rate-limit/config/
CLI-missing) remain byte-identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: e685ffe8ddce
Closes the atomic Claude migration. GenerateText now runs through RunIsolatedTextGeneratorCLIRaw + Classifier.Classify instead of inline subprocess management. The envelope-based classification Claude uniquely needs moves behind a ParseEnvelope hook, and the small stderr phrase fallback becomes declarative data on Classifier.Phrases. Deletes: - ClaudeError / ClaudeErrorKind / 5 Kind constants - classifyEnvelopeError / classifyStderrError / authStderrPhrases / hasAuthPhrase - isExecNotFound (replaced by shared isExecNotFoundErr in the engine) - stderrMessageMaxLen (shared engine owns this) - claudecode/error_test.go (all 13 test cases covered by successors in text_gen_error_test.go, envelope_parser_test.go, and renderTextGenError behavior-pin tests) Callers migrated in the same commit: - explain.go formatCheckpointSummaryError -> renderTextGenError delegation - explain_test.go + summarize/summarize_test.go switch *ClaudeError assertions to *agent.TextGenError with explicit Provider field - summarize/claude.go + summarize.go //nolint:wrapcheck comments reference the new typed error User-facing Claude error output is byte-identical to 963 (pinned by behavior tests in explain_summary_error_test.go). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c550c52fbb00
Migrates Codex off the deprecated RunIsolatedTextGeneratorCLI string
helper to the shared RunIsolatedTextGeneratorCLIRaw + Classifier engine.
User-facing summary errors now surface Codex's verbatim stderr ("ERROR:
unexpected status 401 Unauthorized: Missing bearer or basic
authentication") via renderTextGenError, instead of being double-wrapped
and unactionable.
Auth phrase ("Missing bearer or basic authentication") is seeded from the
2026-04-20 research pass — pinned as a fixture test in error_test.go so
a future Codex CLI change surfaces as a test failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: fe5bdd880042
Migrates Gemini CLI off the deprecated RunIsolatedTextGeneratorCLI
string helper to the shared RunIsolatedTextGeneratorCLIRaw + Classifier
engine. User-facing summary errors now surface Gemini's verbatim stderr
("Please set an Auth method in your settings.json or specify one of:
GEMINI_API_KEY, GOOGLE_GENAI_USE_VERTEXAI, GOOGLE_GENAI_USE_GCA") via
renderTextGenError, preserving the CLI's own three-env-var guidance
instead of double-wrapping into a generic error string.
Auth phrases ("Please set an Auth method", "GEMINI_API_KEY") seeded from
the 2026-04-20 research pass - pinned as a fixture test in error_test.go.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 029eeccc8770
Mirrors the gemini/codex migrations: cursor/generate.go now uses RunIsolatedTextGeneratorCLIRaw + Classifier.Classify, returning *agent.TextGenError instead of a wrapped string error. Adds a Classifier with HTTP-status baseline coverage; cursor-specific auth phrases are intentionally empty until a research pass captures verbatim stderr fixtures from the `agent` binary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 351e57a43041
Mirrors the cursor/gemini/codex migrations: copilotcli/generate.go now uses RunIsolatedTextGeneratorCLIRaw + Classifier.Classify, returning *agent.TextGenError instead of a wrapped string error. Adds a Classifier with HTTP-status baseline coverage; copilot-specific auth phrases are intentionally empty until a research pass captures verbatim stderr fixtures from the `copilot` binary. With this chunk, all five text-generation agents (claude-code, codex, gemini, cursor, copilot-cli) now return the shared typed error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c1a2af6730cc
Replace the conservatively-seeded "rate limit" / "quota" phrases with no per-agent phrases at all. The prior wording implied we had captured Cursor/Copilot stderr fixtures and would refine them later; in fact we hadn't, and inventing phrases risks miscategorizing stderr that happens to contain those substrings in non-rate-limit contexts. The shared HTTP-status baseline (in text_gen_error.go) already catches the load-bearing cases for both CLIs — they pass the upstream provider's HTTP status through on failure. Everything else falls through to Unknown, where renderTextGenError still shows the CLI's own stderr verbatim via TextGenError.Message, so the user always sees the real error text. Comments rewritten to state the design intent directly instead of apologizing for missing research. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4a8b55a83831
All five text-generation agents (claude-code, codex, gemini, cursor, copilot-cli) now go through RunIsolatedTextGeneratorCLIRaw + Classifier.Classify, so the old string helper has no callers. Remove it and its six dedicated tests, drop the now-stale "Replaces X..." line from the Raw helper's doc comment, and update copilotcli/AGENT.md's stale reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ec905137af6a
parseClaudeEnvelope has an intentional len(stdout)==0 carveout that lets empty stdout fall through to stderr/CLIMissing classification. When runErr is also nil (subprocess exits 0 with no output at all), Classify returns nil and the success-path re-parse fails with "unexpected end of JSON input", leaking as *fmt.wrapError rather than *agent.TextGenError. The other four summary providers (codex, gemini, cursor, copilot) already return a typed TextGenError with Message="X CLI returned empty output" in this case. Align Claude with the same shape. While here, convert the "defensive" parseErr branch to return TextGenError as well — if it ever does trigger (non-empty stdout that parseClaudeEnvelope called "success" but parseGenerateTextResponse cannot re-parse), downstream errors.As should still work. E2E verified via the same five-agent × seven-scenario shim used to audit the earlier chunks: Claude's empty-stdout row now renders "Claude failed to generate the summary: claude CLI returned empty output", matching the other four agents byte-for-byte (modulo display name). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 49c36d46d140
The Classifier struct + PhraseRule + ParseEnvelope hook indirection added
architectural complexity without enabling any user-visible functionality
that inline classification can't deliver. Each agent's GenerateText now
does its own classification directly:
- Context sentinels → passthrough
- IsExecNotFoundErr → CLIMissing typed error
- HTTP-status stderr scan (ClassifyStderrHTTPStatus) → Auth/RateLimit/Config/Unknown
- Empty stdout on exit 0 → Unknown with "X CLI returned empty output"
Gemini keeps an inline phrase check for "Please set an Auth method" /
"GEMINI_API_KEY" — gemini-cli's auth stderr does not include any HTTP
status, so the shared baseline misses it. Claude keeps inline phrase
check for "invalid api key" / "not logged in" on the non-envelope
path, matching 963's behavior. Codex/Cursor/Copilot do not need
phrases — their stderr carries HTTP status through.
Claude's envelope classifier collapsed from returning *EnvelopeResult
via the ParseEnvelope hook to returning *agent.TextGenError directly.
No behavior change.
Files deleted:
- 4 per-agent error.go + 4 per-agent error_test.go (cursor, copilot,
codex, gemini Classifier declarations — data-only files)
- claudecode/error.go (the only non-data Classifier)
Files simplified:
- text_gen_error.go: Classifier struct + Classify method + PhraseRule
+ matchPhrase + httpStatusBaseline + EnvelopeResult → gone.
ClassifyStderrHTTPStatus (12 lines) + IsExecNotFoundErr (exported)
+ TruncateStderr (exported) remain.
- text_gen_error_test.go: removed ~200 lines of Classifier tests.
Net branch reduction: +1895/-610 → +1439/-606 (about 456 lines saved).
User-visible output verified byte-identical across all 5 agents ×
7 scenarios via the same shim harness used to audit earlier chunks.
Claude's synth remediation lines (Run `claude login` and retry, etc.)
preserved. Empty-stdout fix from prior commit preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 65c58fb3a0de
Two simplifications that keep user-visible output byte-identical: 1. Shared helper — extract the identical ctx-sentinel / CLIMissing / stderr-classify / empty-stdout logic from 4 non-Claude generate.go files into agent.HandleTextGenResult. Each per-agent GenerateText becomes ~10 lines (build args, call Raw helper, call HandleTextGenResult). Gemini uses the extraClassify hook for its inline phrase heuristic. Claude stays inline because its envelope-first classification order differs. 2. Matrix test — consolidate cursor/codex/copilotcli generate_test.go into a single generate_matrix_test.go in agent_test package. Gemini keeps a minimal generate_test.go for its unique phrase path. 16 per-agent tests → one table-driven matrix covering CLIMissing, Auth-via-401, EmptyStdout, Success for all four non-Claude agents. Also collapsed four near-identical non-Claude auth cases in explain_summary_error_test.go to one representative case (others are covered by generate_matrix_test.go and their package tests), and trimmed the envelope-parser HTTP-status mapping test from 6 cases to 4. Branch diff: +1439/-606 → +1044/-609 (395 lines saved). User output verified byte-identical across all 5 agents × 7 scenarios via the same shim harness used for earlier verifications. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 9cb63826a622
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the typed, provider-aware text-generation error surface from Claude-only (PR #963) to all summary agents, so explain --generate can render consistent, actionable user-facing messages across providers while preserving raw CLI stderr.
Changes:
- Introduces shared
*agent.TextGenError+ helpers (ClassifyStderrHTTPStatus,HandleTextGenResult,RunIsolatedTextGeneratorCLIRaw) to unify subprocess signal capture and error classification. - Updates non-Claude agents (Codex/Gemini/Cursor/Copilot CLI) to return
*agent.TextGenErrorvia the shared helper, while keeping Claude’s envelope-first classification. - Adds/updates tests to pin error preservation through layers and provider-specific rendering rules.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/summarize/summarize_test.go | Updates typed-error preservation test to use *agent.TextGenError. |
| cmd/entire/cli/summarize/summarize.go | Preserves typed errors for errors.As at the explain layer. |
| cmd/entire/cli/summarize/claude.go | Preserves typed errors for errors.As at the explain layer. |
| cmd/entire/cli/explain_test.go | Migrates explain-layer tests from Claude-only typed errors to TextGenError. |
| cmd/entire/cli/explain_summary_error_test.go | Adds tests pinning exact user-facing rendering per provider/kind. |
| cmd/entire/cli/explain_summary_error.go | Adds unified renderer mapping (Kind × Provider) → user-facing messages. |
| cmd/entire/cli/explain.go | Switches explain-layer formatting to use renderTextGenError. |
| cmd/entire/cli/agent/text_generator_cli_test.go | Updates tests for new raw subprocess helper behavior. |
| cmd/entire/cli/agent/text_generator_cli.go | Replaces string-flattening helper with RunIsolatedTextGeneratorCLIRaw returning stdout/stderr/exit. |
| cmd/entire/cli/agent/text_gen_error_test.go | Adds unit tests for TextGenError and shared classification helpers. |
| cmd/entire/cli/agent/text_gen_error.go | Introduces shared typed error + stderr truncation + HTTP-status classification + result handler. |
| cmd/entire/cli/agent/generate_matrix_test.go | Adds cross-provider matrix tests for non-Claude agents. |
| cmd/entire/cli/agent/geminicli/generate_test.go | Adds Gemini-only phrase-classification test. |
| cmd/entire/cli/agent/geminicli/generate.go | Migrates Gemini agent to raw runner + HandleTextGenResult + phrase hook. |
| cmd/entire/cli/agent/cursor/generate.go | Migrates Cursor agent to raw runner + HandleTextGenResult. |
| cmd/entire/cli/agent/copilotcli/generate.go | Migrates Copilot CLI agent to raw runner + HandleTextGenResult. |
| cmd/entire/cli/agent/copilotcli/AGENT.md | Updates documentation to reference the new raw helper. |
| cmd/entire/cli/agent/codex/generate.go | Migrates Codex agent to raw runner + HandleTextGenResult. |
| cmd/entire/cli/agent/claudecode/response.go | Updates comment to reflect new envelope parsing/classification naming. |
| cmd/entire/cli/agent/claudecode/generate.go | Refactors Claude agent to use raw runner and return *agent.TextGenError. |
| cmd/entire/cli/agent/claudecode/envelope_parser_test.go | Adds tests for envelope-first classification into TextGenError. |
| cmd/entire/cli/agent/claudecode/envelope_parser.go | Adds envelope classifier producing *agent.TextGenError. |
| cmd/entire/cli/agent/claudecode/claude_test.go | Migrates Claude tests to assert *agent.TextGenError. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8fd64a6. Configure here.
…runcation Three critical bugs surfaced by PR #1005 review (Copilot + Cursor Bugbot + internal review agents). Each was independently confirmed by multiple reviewers. 1. Claude envelope preempts ctx.Canceled on partial stdout (claudecode/envelope_parser.go + generate.go) When ctx was cancelled mid-write, Claude's subprocess could leave partial non-JSON on stdout. classifyClaudeEnvelope then returned a "failed to parse claude CLI response" TextGenError that preempted the ctx sentinel, so the user saw a parse error instead of "summary generation canceled". Fix: classifyClaudeEnvelope now accepts runErr and returns nil on parse-failure + ctx sentinel, letting the caller surface context.Canceled / DeadlineExceeded unwrapped. Complete is_error envelopes still preempt ctx (963's intentional ordering — a structured diagnostic is more actionable than "cancelled"). Regression test added. 2. ClassifyStderrHTTPStatus bare-substring false positives (text_gen_error.go) strings.Contains(stderr, "401") matched port numbers ("port 14010"), millisecond latencies ("took 429ms"), byte counts ("14000 bytes"), request IDs ("404a9f"), and timestamps ("14:01:23"). All five non-Claude providers could mis-route infra/network errors to Auth/RateLimit/Config. Fix: word-boundary regex `\b4\d{2}\b` + leftmost-match-wins (typical "HTTP 401\n...retry window 429" pattern — leading status is primary). Six regression cases added covering the false-positive patterns the reviewers flagged. 3. TruncateStderr cuts UTF-8 mid-rune (text_gen_error.go) Byte-slicing at stderrMessageMaxLen could land inside a multi-byte rune, producing invalid UTF-8 in TextGenError.Message which flowed straight to user-facing output. Fix: strings.ToValidUTF8 on the truncated prefix replaces any broken trailing sequence with "". Test asserts valid UTF-8 for a 3-byte rune straddling the cut point. Full CI green; all 35 user-facing scenario outputs byte-identical across the 5 agents before and after these fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: edc8a7d436cf
Integrates three months of main into the typed-error branch in one merge: - explain: formatCheckpointSummaryError keeps main's structured failure block (label + rows); renderTextGenError is re-ported to return (label, rows, error) with remediation as lowercase "try" rows. Claude wording preserves 963's baseline; non-Claude providers show their CLI stderr verbatim with no synthesized remediation row. - pi (added on main after this branch forked): migrated GenerateText and ListModels to RunIsolatedTextGeneratorCLIRaw + HandleTextGenResult with no phrase rules (HTTP-status baseline, stderr verbatim), and added Pi to the provider display-name and remediation maps. - explain_summary_error_test.go re-pinned to the structured contract: every (label, rows, error) triple asserted exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Brought up to date with current main via a merge commit (
Verified: full Note for the queue: #964/#1230 stack on the same error surface — landing this first lets them adopt 🤖 Generated with Claude Code |
Round-2 fixes for the review comments on this PR. The word-boundary regex
that replaced substring matching fixed the reported false positives but
introduced a worse class, and three code paths captured subprocess signal
they then discarded.
Classification now requires an HTTP-status context rather than isolated
digits. `\b4\d{2}\b` rejects "port 14010" but ':' and '.' are themselves
non-word characters, so it matched stack frames ("at foo.js:401:12" ->
auth), decimals ("took 2.403 seconds" -> auth), source lines ("main.go:404")
and bare counts ("build 404 finished"). Gemini, Copilot and Cursor are Node
CLIs whose crash output is "file.js:LINE:COL", so that was the common case,
and a wrong Kind is worse than Unknown: it routes to a fabricated remediation
row ("wait and retry") for a bug that will never resolve, and hides the raw
stderr that Unknown would have shown.
Also:
- Scan for the first RECOGNIZED status, not merely the leftmost match. An
unmapped leading code no longer masks a later one ("413 ...; 401 ..." was
Unknown).
- Classify against the full stderr and truncate only for display. Truncating
first hid any status past the 500-byte cap, which is exactly where it lands
when a CLI prints a banner or stack preamble ahead of the real error. The
gemini phrase hook sees the full text too.
- Restore the three-tier detail fallback deleted with
RunIsolatedTextGeneratorCLI: stderr, then stdout, then the run error, so
Message is never empty. codex/cursor/copilot are stdout-primary and print
diagnostics there; a launch failure (permission denied, exec format error)
produces no output at all and only the run error describes it. Both
previously rendered "(no diagnostic detail available from X CLI)".
- Let a failed run's stderr outrank unparseable stdout in the Claude path.
classifyClaudeEnvelope returned a "failed to parse claude CLI response"
error for any non-JSON stdout, which preempted stderr classification, so a
real 401 surfaced as a JSON-parse complaint. This is the live half of the
cursor-bot finding on this PR; the cancellation half was already fixed.
- Narrow the gemini hook to "Please set an Auth method". A bare
GEMINI_API_KEY mention also appears in quota and deprecation messages, so
matching it reported a working credential as an auth failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sertion Addresses the three Windows-portability review comments on this PR. The new tests shell out to `echo`, `sh` and `true`; `echo` is a cmd.exe builtin with no executable on PATH, and the others do not exist on Windows by default. Three sibling tests in text_generator_cli_test.go already skipped on runtime.GOOS == windows, so the new files were inconsistent with the file they replaced. CI only runs test:e2e on windows-latest, so this is a contributor-experience fix rather than a CI failure. Two assertions were also not doing their job: - The ported cancellation test accepted `errors.Is(ctx.Err(), context.Canceled)` as an alternative to checking err. cmd.Run returns only after cancel() fires, so that disjunct was unconditionally true and the assertion could never fail — the helper's sentinel-preservation contract, which explain's timeout UX depends on, was unpinned by a test that looked like it covered it. - The success test asserted strings.Contains on raw stdout, which no longer pinned that the runner must NOT trim (Claude's envelope parser needs the bytes unmodified). Pin the exact bytes, and cover trimming where it actually happens in TestHandleTextGenResult_TrimsStdout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment rot found while addressing the review comments on this PR. These are load-bearing design-rationale comments, so a wrong one is worse than none. - claudecode/generate.go's numbered "Classification order" listed steps 3-4 as reachable for non-empty unparseable stdout; they were not, and the list contradicted the accurate contract on classifyClaudeEnvelope. Now describes the real order. - explain_summary_error.go stated syntheticFallback applies only when the provider is in providersNeedingSynthesizedRemediation AND Message is empty. The guard is an OR, so every provider gets a "try" row when Message is empty — which the tests already assert. A reader following the comment would conclude the opposite. - Two comments named symbols that do not exist: parseClaudeEnvelope (it is classifyClaudeEnvelope) and isExecNotFoundErr (it is IsExecNotFoundErr; the unexported form was deleted with claudecode/error.go). Both dead-end a grep. - The five //nolint:wrapcheck justifications implied wrapping defeats errors.As. It does not — %w preserves the chain, as pi/models.go demonstrates by wrapping the same error with no nolint. The real reason is that the explain layer renders label+message itself, so a wrap prefix would leak into user-facing output. - Dropped the hand-maintained provider list in providersNeedingSynthesizedRemediation, which already omitted Pi and would rot again on the next agent. - Documented that a path-containing argv skips LookPath and yields *fs.PathError/ENOENT, which IsExecNotFoundErr also matches — the clause two tests in this PR depend on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reconciles this PR's classification surface with #964, which merged 2026-07-23 (28a39a3) and rewrote the same 11 files. The PR description's queue note is inverted by that: it assumed #964 would adopt TextGenError, but #964's carrier landed first and is now the incumbent. The two error types are complementary, not competing, so they compose rather than one replacing the other: - *agent.TextGenError answers "what kind of failure?" (Kind, Provider, APIStatus) and drives the user-facing label and remediation. - *agent.TextGenerationError answers "what did the subprocess emit before it died?" (Stderr, StdoutBytes) and drives #964's timeout diagnostic. That evidence is the ONLY signal on the ctx path, where classification is meaningless. HandleTextGenResult and claudecode.GenerateText now wrap the classified error (or ctx sentinel) in *TextGenerationError, so errors.As recovers the classification, errors.Is recovers the sentinel, and errors.As recovers the evidence — all from one error. Returning either type bare silently regresses the other, which is why TestHandleTextGenResult_DeadlineCarriesPartialOutput (ported from main's deleted helper test) now asserts all three at once. Conflict resolutions of note: - claudecode/generate.go resolved toward MAIN's body. This PR's version predates the apiKeyHelper auth injection (writeAuthSettingsFile + --settings <0600 file>, never argv) that #964 added after finding it silently dropped API-billing auth. Taking this PR's side would have re-introduced that outage and lost the argv-exposure mitigation. Kept main's buildGenerateArgs(model, settingsPath) and layered classification on top. - generate_streaming.go had NO conflict marker but referenced the deleted classifyEnvelopeError — the silent half of the collision. Extracted classifyEnvelopeFields so both Claude paths share one implementation and cannot drift, which is what main's comment already said should hold. - formatCheckpointSummaryError adopts main's (err, *summaryAttempt) signature; the typed-error branch delegates to renderTextGenError. - generateCheckpointAISummary adopts main's 2-return signature with the timeout/progress/attempt tail. - Dropped RunIsolatedTextGeneratorCLI and its tests; the stdout-fallback behavior its deleted test pinned is now covered in HandleTextGenResult. - Dropped formatMessageSuffix/formatClaudeErrorSuffix (obsolete with ClaudeError); kept main's providerCLIName and timeoutDiagnostic. Verified: mise run test — 8452 tests, 0 failures. Both features' tests pass in one tree (#964's TestTimeoutDiagnostic_* alongside this PR's TestRenderTextGenError_*), and main's apiKeyHelper tests pass against the live GenerateText call path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01KYAZ33V93JY670BJYK46THFZ
The consumers that print TextGenError.Error() directly — `entire dispatch`, `entire review`'s synthesis sink, and runner setup — have no access to the explain layer's renderTextGenError (unexported, package cli), so Error() is what their users actually see. The CLIMissing constructions deliberately set no Message, which made that string pure jargon: before: codex CLI error (kind=cli_missing) main: codex CLI not found: exec: "codex": executable file not found in $PATH Cause was populated at every construction site and read by none, so the actionable text was in memory and discarded. Fall back to it when Message is empty. This matters most for Cursor, whose binary is `agent`, not `cursor` — Cause is the only place the real name a user would grep for appears. Message still takes precedence, so a classified failure is unchanged and the cause is never appended twice. Also documents TextGenError's field invariants, which were compressed into one sentence that was wrong for ExitCode: 0 is a real exit code on Claude's primary failure path (exit 0 with is_error:true), not only "not applicable". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
peyton-alt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/31103d7bc0c5
Summary
GenerateTextnow returns*agent.TextGenErrorwith a consistentKind(Auth / RateLimit / Config / CLIMissing / Unknown), and a unified renderer maps (Kind × Provider) to user-facing text.TextGenError.Messageand shown to the user — no speculative phrase matching, no inventing remediation for CLIs we haven't researched.What the user sees
<Display> CLI is not installed or not on PATH<Display> authentication failed: <stderr>(Claude also appendsRun \claude login` and retry`)<Display> rejected the summary request due to rate limits or quota: <stderr><Display> rejected the summary request: <stderr><Display> failed to generate the summary: <stderr><Display> failed to generate the summary: <display> CLI returned empty outputDesign
ClassifyStderrHTTPStatus) is the load-bearing classification signal — most CLIs passthrough upstream HTTP status in stderr.agent.HandleTextGenResult— the 4 non-Claude agents'GenerateTextbodies are ~10 lines each. Claude inlines its envelope-first classification because its order differs.Run \claude login` and retry` etc.) preserved from PR feat: classify Claude CLI errors into actionable user messages #963.Notable
*fmt.wrapErrorinstead of*agent.TextGenError.RunIsolatedTextGeneratorCLIstring helper and its 6 tests — all callers migrated toRunIsolatedTextGeneratorCLIRaw.Classifierstruct +PhraseRule+ParseEnvelopehook in favor of inline classification per agent (saved ~450 lines while keeping every user-visible output unchanged).Test plan
mise run test:cigreen on final commit8fd64a650(unit + integration + Vogon canary)golangci-lint run— 0 issuesGenerateText→Classifier→renderTextGenErrorchainagent_integrationE2E suite covers the subprocess boundary and can be run manually if reviewers want extra coverage)🤖 Generated with Claude Code
Note
Medium Risk
Touches shared subprocess execution and error classification/rendering used by all summary providers, so misclassification or wrapping could change user-facing guidance and retry behavior across agents.
Overview
Introduces a shared
*agent.TextGenErrortype (withAuth/RateLimit/Config/CLIMissing/Unknown) plus common helpers to run CLIs (RunIsolatedTextGeneratorCLIRaw) and map subprocess outcomes into typed errors (HandleTextGenResult).Migrates Codex/Copilot/Cursor/Gemini generators to the new raw runner + shared handler, and refactors Claude to return
TextGenErroras well while preserving Claude’s envelope-first classification via a newclassifyClaudeEnvelopehelper.Updates
explain --generateto render provider-aware, consistent user-facing messages viarenderTextGenError, removes the Claude-specificClaudeErrortype, and adds a focused test matrix covering the canonical success/failure scenarios (plus Gemini’s auth-phrase hook and Claude envelope parsing cases).Reviewed by Cursor Bugbot for commit 8fd64a6. Configure here.