fix(llm): actionable provider errors instead of a raw JSON repr (#1110) - #1126
Conversation
The two most likely first-run failures both reached the user as a Python repr
of the provider's JSON body, at the first AI-backed command in the README
quickstart:
Error: Error code: 401 - {'type': 'error', 'error': {'type':
'authentication_error', 'message': 'API key is invalid.'}, 'request_id': None}
It names neither the env var to fix nor a next step, and the 404 form exposes an
internal model constant the user never chose.
New codeframe/adapters/llm/errors.py maps a provider SDK exception to a typed
LLMError with text the user can act on. It lives on the exception rather than in
each command's error handler, so it reaches every LLM-backed surface — prd
generate, tasks generate, work start --execute, prd stress-test, the server —
through the `except ... as e: print(e)` each already has. One place, whole
surface, no per-command plumbing.
401 -> names the provider, the env var the key is read from (per provider, so
an openai user is not told about ANTHROPIC_API_KEY), and `cf env check`
404 -> names the model that failed and the CODEFRAME_<PURPOSE>_MODEL override
that controls it. New LLMModelNotFoundError: a retired model ID is the
failure that made 0.9.1 unusable and deserves its own type.
429 -> rate limited, wait and retry
5xx/529 -> new LLMOverloadedError: the provider's side, not your config
The raw payload is never lost: `raise ... from exc` keeps it on __cause__, and
CODEFRAME_VERBOSE=1 prints it. The default message advertises that switch.
Adapter wiring:
- Anthropic sync complete() had NO mapping at all — that is the code path
behind the report, letting the SDK exception escape verbatim.
- Anthropic async and both OpenAI paths produced typed errors but stringified
the raw SDK body into them. OpenAI sync raised bare ValueError.
- Status is read from the exception, falling back to the exception's type name,
so a partially-constructed error still classifies correctly.
core/tasks.py also lets LLMError escape the task-generation fallback, for the
same reason ValueError already did: degrading a bad key into bullet extraction
hides the one thing the user needs to read.
Two openai tests asserted the old bare-ValueError contract and are updated.
Verified live against the real API on both paths, including CODEFRAME_VERBOSE=1
reproducing the original payload.
|
Claude finished @frankbria's task in 7m 2s —— View job Precision bug-hunting review — completeReviewed PR #1126 for concrete defects only (logic errors, crashes, security, data loss, breaking changes). Style/architecture/coverage are covered by a separate review.
Result: 1 finding (minor), no critical/major issues.
Summary comment and inline finding posted. |
ReviewThis is a solid fix — the design (mapping at the adapter boundary rather than per-command) is the right call and matches the "one place, whole surface" reasoning in the PR description. Tests are thorough and the live-API evidence is convincing. A few findings: 1.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
GLM precision reviewScoped to concrete defects only (logic errors, crashes, security, data loss, breaking changes). Style, architecture, naming, and coverage are covered by a separate review.
Notable things checked and cleared:
1 finding (1 minor). No critical/major issues. |
…convention (#1110) Three review findings, all real. 1. OpenAIProvider serves openai, ollama, vllm and compatible, but hardcoded provider="openai" in its error mapping. So an ollama user was told to check $OPENAI_API_KEY — which local providers do not use at all; get_provider constructs them with api_key="not-required". This undercut the whole point of the change for three of the four types it covers. The provider now knows which one it is, and the 401 branch gives local providers endpoint advice instead of key advice. 2. 403 was recognised in _TYPE_TO_STATUS but had no branch, so PermissionDenied fell through to the least useful message in the file. It now says the key is valid but lacks access to the model. 3. CODEFRAME_VERBOSE used a bare truthy check, so =0/false/no/off still printed the raw payload. Now uses the same truthy set as env_provenance, hook_trust and notifications_config.
|
Claude finished @frankbria's task in 4m 34s —— View job GLM precision review (re-trigger on synchronize)Scoped to concrete defects only (logic errors, crashes, security, data loss, breaking changes). Style/architecture/coverage covered by the separate general review. The synchronize brought commit
Result: 1 new finding (minor). No critical/major.
Things checked and cleared in the new commit:
Not re-posted (already on the PR record): the ungated |
|
All three findings were real and are fixed in the latest commit. 1. 2. 403 recognised but unhandled (minor) — correct, it fell through to the least useful message in the file. It now has its own branch naming the model and the account whose key was used. 3. Six new tests cover all three, including one asserting an ollama failure never mentions Thanks — #1 in particular was the kind of thing that would have read as a fix while still being wrong for local-model users. |
ReviewRe-reviewed after the follow-up commit (07fb1ef). This addresses three of the four findings from my earlier review and the GLM/Claude-workflow reviews on this PR:
Still open — not addressed by the follow-up commit:
Everything else from earlier passes still holds: the adapter-boundary design is correct, 🤖 Generated with Claude Code |
…ssages # Conflicts: # codeframe/core/tasks.py
|
Merged #1115 (now on main) deletes the task-generation fallback entirely, so the Both features verified together: 333 passed across |
|
Claude finished @frankbria's task in 5m 8s —— View job GLM precision review (re-trigger on synchronize)Scoped to concrete defects only (logic errors, crashes, security, data loss, breaking changes). Style/architecture/coverage covered by the separate general review. The synchronize (
Result: ✅ no new defects in this synchronize. Verified clean on this pass:
Carried forward — already on the PR record from earlier commits, not re-posted (consistent with the convention the prior GLM round set for these two):
Neither was touched by the merge. |
ReviewRe-reviewed at the current head ( One thing not yet raised in the four prior review passes on this PR:
|
#1110) Review finding against my own previous fix. get_provider hands OPENAI_API_KEY to ollama/vllm/compatible whenever it is set, and only substitutes "not-required" when it is not. So "This provider runs locally and needs no API key" was false in exactly the case that matters: CODEFRAME_LLM_PROVIDER=compatible against a gateway, with a wrong key set — the user is told the key is irrelevant while the key is the thing being rejected, and the message never names OPENAI_API_KEY. The advice now branches on whether a key was actually sent, so a key-present failure names the variable and a key-absent one points at the endpoint.
|
Claude finished @frankbria's task in 3m 36s —— View job GLM precision review (re-trigger on synchronize)Scoped to concrete defects only (logic errors, crashes, security, data loss, breaking changes). Style/architecture/coverage covered by the separate general review. The synchronize brought commit
Result: ✅ GLM review: no new defects in this synchronize. The new
So the message can no longer assert "was sent no API key" / "needs no API key" in the one case that mattered ( Carried forward — already on the PR record from earlier commits, not re-posted (consistent with the convention the prior two GLM rounds set):
|
ReviewThis is at the current head ( Still open (raised in earlier rounds on this PR, not yet addressed)1. if status == 401 or "authentication" in str(exc).lower():This still runs unconditionally ahead of 2. New minor nit
What's solid (confirmed at this head)
Neither open item is a blocker on its own — #1 has a narrow, plausible trigger condition, and #2 only bites on a malformed/nonstandard provider response — but both are one-line-ish fixes that would close out the last gaps this thread has already surfaced. |
1 participant
Closes #1110.
The change
A new
codeframe/adapters/llm/errors.pymaps a provider SDK exception to a typedLLMErrorcarrying text the user can act on.The important design choice: the message lives on the exception, not in each
CLI command's error handler. Every LLM-backed command already ends in some form
of
except ... as e: print(e), so putting the mapping at the adapter boundarycovers
prd generate,tasks generate,work start --execute,prd stress-testand the server surfaces at once — and covers commands added later — instead of
requiring a per-command edit that would drift.
LLMAuthErrorcf env checkLLMModelNotFoundError(new)CODEFRAME_<PURPOSE>_MODELoverrideLLMRateLimitErrorLLMOverloadedError(new)The env var is resolved per provider, so an
openaiuser is never told tocheck
ANTHROPIC_API_KEY.Adapter wiring — including the one that caused the report
complete()had no error mapping at all. That is the pathbehind the issue: the SDK exception escaped verbatim.
the raw SDK body into them; OpenAI sync raised a bare
ValueError.partially-constructed error (or a stubbed one) still classifies correctly.
core/tasks.pynow also letsLLMErrorescape the task-generation fallback, forthe same reason
ValueErroralready did — degrading a bad key into bulletextraction hides the one thing the user needs to read. (This is order-independent
with #1115, which removes that fallback entirely.)
Evidence — live against the real API
Bad key, via
cf prd stress-test:The retired model from the report (
CODEFRAME_PLANNING_MODEL=claude-3-5-haiku-20241022):Same command with
CODEFRAME_VERBOSE=1— the original payload, unmodified:19 new tests;
tests/adapters/204 passed.Acceptance criteria
CODEFRAME_*_MODELoverride__cause__always,CODEFRAME_VERBOSE=1to printJudgment calls
CODEFRAME_VERBOSE=1rather than a--verboseflag. The AC says"
--verbose/ in the event log".prd generateandtasks generatehave no--verbosetoday, so a flag would mean adding and threading one through eachcommand — a larger, driftier diff for the same result. An env var works
everywhere immediately, including the server. Say the word if you'd rather have
the flags and I'll add them on top.
LLMErrordoes not inheritValueError. OpenAI sync previously raisedValueError, so this is a contract change; two tests asserted the old shape andare updated. I checked the
except ValueErrorsites — the only one in aprovider path is the task-generation fallback, handled above.
Known limitations
async_stream) still use their original narrower handlers.They are not on the first-run path this issue is about; folding them in is
mechanical follow-up if wanted.
"authentication"in the message text as abackstop for providers that report auth failures without a status.