fix(learn): don't shadow TIMEOUT/CONNECTION with the generic RUNTIME_ERROR (#2099)

## Description

`classify_error` (used by `headroom learn` to categorize failed tool
calls) miscategorizes timeouts and connection failures as generic
runtime errors.

The pattern list is checked in order, first match wins, and it puts the
generic catch-all *before* the specific categories:

```python
(re.compile(r"Traceback \(most recent|Exception:|Error:", re.I), ErrorCategory.RUNTIME_ERROR),
(re.compile(r"timed? ?out|TimeoutError|deadline exceeded", re.I), ErrorCategory.TIMEOUT),
...
(re.compile(r"ConnectionError|ConnectionRefused|ECONNREFUSED|network", re.I), ErrorCategory.CONNECTION_ERROR),
```

Every Python exception repr is `XxxError: ...` (or `Exception: ...`), so
the generic `Error:`/`Exception:` pattern matches first. A tool result
of `"TimeoutError: timed out after 30s"` is classified `RUNTIME_ERROR`
instead of `TIMEOUT`; `"ConnectionError: [Errno 111] Connection
refused"` is classified `RUNTIME_ERROR` instead of `CONNECTION_ERROR`.
The dedicated `TIMEOUT` and `CONNECTION_ERROR` categories — which
explicitly list `TimeoutError` and `ConnectionError` — are therefore
unreachable for the most common (colon-repr) message shape; they only
fire for tokenless phrasings like `deadline exceeded`. That mislabels
the learn digest's per-category error stats.

## Fix

Check the two specific categories (`TIMEOUT`, `CONNECTION_ERROR`) before
the generic `RUNTIME_ERROR` catch-all. A generic exception repr with no
timeout/connection token still classifies as `RUNTIME_ERROR`, so
existing behavior for those is unchanged (including the opencode
scanner's `"Error: command failed with exit code 1"` → `RUNTIME_ERROR`).

Closes #

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)

## Changes Made

- `headroom/learn/_shared.py`: move the `TIMEOUT` and `CONNECTION_ERROR`
patterns above the generic `RUNTIME_ERROR` pattern, with a comment
explaining the ordering.
- `tests/test_learn/test_error_classification.py`: new tests asserting
`TimeoutError:`/`ConnectionError:` reprs classify specifically, a
generic `Error:` stays `RUNTIME_ERROR`, and non-error text is `UNKNOWN`.
- `CHANGELOG.md`: Bug Fixes entry.

## Testing

- [ ] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [ ] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed

### Test Output

```text
$ uvx ruff@0.15.17 check headroom/learn/_shared.py tests/test_learn/test_error_classification.py
All checks passed!
$ python -m py_compile headroom/learn/_shared.py tests/test_learn/test_error_classification.py
OK
```

## Real Behavior Proof

- Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17`. Importing
`headroom` pulls in the torch/transformers stack and a full `pytest`
gets OOM-killed on this box, so I verified the ordering with a
dependency-free script that replicates the pattern list under both the
old and new orderings, and left the full pytest to CI.
- Exact command / steps: classified `"ConnectionError: [Errno 111]
Connection refused"` and `"TimeoutError: timed out after 30s"` under the
old order (RUNTIME before TIMEOUT/CONNECTION) and the new order
(TIMEOUT/CONNECTION before RUNTIME), plus the opencode scanner's
`"Error: command failed with exit code 1"` as a regression guard.
- Observed result: old order classifies both as `RUNTIME_ERROR`; new
order classifies them as `CONNECTION_ERROR` and `TIMEOUT` respectively;
the guard string stays `RUNTIME_ERROR` under both orderings, so the
existing opencode scanner test is unaffected.
- Not tested: a full `headroom learn` digest run; full local `pytest`
deferred to CI (OOM, per above).

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] I have updated the CHANGELOG.md if applicable

## Additional Notes

The "unit tests pass locally" and "type checking" boxes are unchecked
because the full suite imports the ML stack, which I can't run in this
environment; the change is a reordering of two entries in a pure pattern
list, verified by the standalone proof (which also confirms the one
existing test that touches this path stays green) and the new regression
tests for CI.

Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
Abhay Singh 2026-07-13 20:20:54 +05:30 committed by GitHub
parent e0232df9b4
commit c7b5a24b4f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 45 additions and 5 deletions

View file

@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Bug Fixes
* **learn:** classify timeout and connection tool failures correctly instead of as generic runtime errors. In `classify_error` the generic `RUNTIME_ERROR` pattern (`Traceback|Exception:|Error:`) was checked before the dedicated `TIMEOUT` and `CONNECTION_ERROR` patterns. Because every Python exception repr is `XxxError: ...`, a `TimeoutError: ...` or `ConnectionError: ...` matched the catch-all first and was miscategorized as `RUNTIME_ERROR`, leaving those two categories unreachable for the common colon-repr form (they only fired for tokenless phrasings like `deadline exceeded`). The `TIMEOUT` and `CONNECTION_ERROR` patterns are now checked before the generic catch-all; tokenless generic errors still classify as `RUNTIME_ERROR`.
* **tokenizers:** resolve HuggingFace tokenizer names by the most-specific prefix. `get_tokenizer_name` scanned `MODEL_TO_TOKENIZER` in dict-insertion order and returned the first key the model merely starts with, so a short family key shadowed a more-specific one — `qwen2-7b-instruct` matched `qwen` before `qwen2`/`qwen2-7b` and resolved to the Qwen1 tokenizer (a different vocabulary, hence wrong token counts); `qwen2.5-*` and `deepseek-v2.x` were mis-resolved the same way. It now picks the longest matching prefix, mirroring the order-dependent-prefix guard the sibling tiktoken `get_encoding_for_model` already documents.
* **pricing:** map retired `claude-3-sonnet-20240229` to a Sonnet-tier price instead of Haiku. When LiteLLM's cost DB lacks the retired model, resolution falls through to `MODEL_ALIASES`, which pointed Claude 3 Sonnet (a $3/$15-per-1M model) at `claude-3-haiku-20240307` ($0.25/$1.25) — a different tier that underpriced every cost/savings figure for that model ~12x on both input and output. It now aliases to `claude-sonnet-4-20250514`, the same-price target the other retired-Sonnet aliases already use.
* **cache/semantic:** don't evict an unrelated entry when re-storing a key that is already cached. `SemanticCache.put` ran its at-capacity eviction loop before computing the entry's key, so overwriting a key that was already present (a duplicate or retried store) still evicted the LRU-oldest distinct entry even though an in-place update grows nothing. That silently dropped a live entry and turned a later lookup for it into a false cache miss. The key is now computed first and the eviction loop only runs when the key is genuinely new (mirroring `CompressionCache.store_compressed`, which deletes-then-inserts).

View file

@ -51,8 +51,18 @@ _ERROR_PATTERNS: list[tuple[re.Pattern[str], ErrorCategory]] = [
),
(re.compile(r"EISDIR|Is a directory", re.I), ErrorCategory.IS_DIRECTORY),
(re.compile(r"SyntaxError|IndentationError", re.I), ErrorCategory.SYNTAX_ERROR),
(re.compile(r"Traceback \(most recent|Exception:|Error:", re.I), ErrorCategory.RUNTIME_ERROR),
# Specific runtime failures must be checked BEFORE the generic RUNTIME_ERROR
# catch-all below. Every Python exception repr is "XxxError: ..." (or
# "Exception: ..."), so the generic `Error:`/`Exception:` pattern would
# otherwise match first and a "TimeoutError: ..." / "ConnectionError: ..."
# would be miscategorized as RUNTIME_ERROR, making the dedicated TIMEOUT and
# CONNECTION_ERROR categories unreachable for the common colon-repr form.
(re.compile(r"timed? ?out|TimeoutError|deadline exceeded", re.I), ErrorCategory.TIMEOUT),
(
re.compile(r"ConnectionError|ConnectionRefused|ECONNREFUSED|network", re.I),
ErrorCategory.CONNECTION_ERROR,
),
(re.compile(r"Traceback \(most recent|Exception:|Error:", re.I), ErrorCategory.RUNTIME_ERROR),
(re.compile(r"No (?:matches|files|results) found|0 matches", re.I), ErrorCategory.NO_MATCHES),
(
re.compile(r"user.*reject|user.*denied|declined|didn't want to proceed", re.I),
@ -60,10 +70,6 @@ _ERROR_PATTERNS: list[tuple[re.Pattern[str], ErrorCategory]] = [
),
(re.compile(r"[Ss]ibling tool call errored", re.I), ErrorCategory.SIBLING_ERROR),
(re.compile(r"exit code|non-zero|exited with", re.I), ErrorCategory.EXIT_CODE),
(
re.compile(r"ConnectionError|ConnectionRefused|ECONNREFUSED|network", re.I),
ErrorCategory.CONNECTION_ERROR,
),
(
re.compile(r"BUILD FAILED|compilation error|compile error", re.I),
ErrorCategory.BUILD_FAILURE,

View file

@ -0,0 +1,33 @@
"""Error classification ordering: specific categories must not be shadowed by
the generic RUNTIME_ERROR catch-all."""
from __future__ import annotations
from headroom.learn._shared import classify_error
from headroom.learn.models import ErrorCategory
def test_timeout_repr_is_not_shadowed_by_runtime_error() -> None:
# "TimeoutError: ..." contains "Error:", which the generic RUNTIME_ERROR
# pattern also matches; TIMEOUT must still win.
assert classify_error("TimeoutError: timed out after 30s") == ErrorCategory.TIMEOUT
assert classify_error("operation timed out") == ErrorCategory.TIMEOUT
def test_connection_repr_is_not_shadowed_by_runtime_error() -> None:
assert (
classify_error("ConnectionError: [Errno 111] Connection refused")
== ErrorCategory.CONNECTION_ERROR
)
assert classify_error("ECONNREFUSED") == ErrorCategory.CONNECTION_ERROR
def test_generic_error_still_classifies_as_runtime() -> None:
# A plain exception repr with no more-specific token stays RUNTIME_ERROR
# (matches the opencode scanner's expectation).
assert classify_error("Error: command failed with exit code 1") == ErrorCategory.RUNTIME_ERROR
assert classify_error("Traceback (most recent call last):") == ErrorCategory.RUNTIME_ERROR
def test_non_error_text_is_unknown() -> None:
assert classify_error("all good, tests passed") == ErrorCategory.UNKNOWN