Refactor chaterror.Classify to normalize evidence gathering
- Dominant language
- No language data
- Stars
- 3
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
> Note: this issue was filed by an over-eager agent with too many MCP tools.
## Context
PR coder/coder#27913 fixed a bug where a Bedrock credential resolution failure was incorrectly retried as a generic 500. The fix widened several signal checks in `chaterror.Classify` to read `combinedText` (the merged transport wrapper + structured response body) instead of `lower` (just the wrapper).
This exposed a structural issue: the classifier has two parallel text sources (`lower` from `err.Error()` and `structured.detail` from `ProviderError.ResponseBody`) and signal checks inconsistently pick which to match against. PR coder/coder#27913 widened the body-only signals, but the underlying design is fragile.
## Problems
1. **Two-source inconsistency**: Some signals check `lower`, some check `combinedText`, and it is unclear which uses which without reading each line. New signal checks can silently regress to `lower`-only.
2. **Pattern sprawl**: Too many string patterns across too many signal lists (`overloadedPatterns`, `authStrongPatterns`, `configPatterns`, `timeoutPatterns`, etc.) without documentation of which provider incident motivated each pattern.
3. **Signal/display coupling**: `structured.detail` (used for signal matching) is the first line of the response body only (`providerErrorResponseMessage` truncates at the first newline). If the useful signal text appears on line 2+, it is missed. The signal source and the user-facing detail display are the same string; they should be decoupled.
## Proposed approach
**Data-flow normalization**, not a rule engine:
1. One `gatherEvidence` step at the top of `Classify` that produces a single struct: lowercased combined text (transport error + full structured body, not just first line), status code, and any structured provider fields.
2. Every signal check reads only from the evidence struct. No signal touches raw inputs.
3. Keep the prioritized rule ordering as plain Go code. Do not build a pattern-matching DSL or data-driven rule table.
4. Decouple signal matching text from user-facing detail display text.
## Non-goals
- No new error taxonomy.
- No change to `Classify`'s public signature.
- No pattern DSL or data-driven rule engine.
## Test strategy
Convert signal tests to a matrix that programmatically asserts each source-agnostic signal classifies correctly when the pattern arrives via body-only and via transport-only. This makes "forgot to check the other source" impossible to reintroduce silently.
## Related
- PR coder/coder#27913 (the incremental fix)
- `coderd/x/chatd/chaterror/classify.go` (the classifier)
- `coderd/x/chatd/chaterror/signals.go` (the pattern definitions)
- `coderd/x/chatd/chaterror/provider_error.go` (response body extraction)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with coderd/x/chatd/chaterror/classify.go, then read signals.go and provider_error.go to trace how transport and structured response text are currently gathered and displayed. Run the existing chaterror signal tests, then make the evidence sources consistent and add the body-only/transport-only matrix described in the issue; done means all signals classify identically regardless of source without changing Classify’s public signature.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100