mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
c7b5a24b4f
## 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>
33 lines
1.4 KiB
Python
33 lines
1.4 KiB
Python
"""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
|