coder / coder/internal

Refactor chaterror.Classify to normalize evidence gathering

Open
#1,638 2 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.