fix: update models and caching price - #406
Conversation
There was a problem hiding this comment.
Line 15: This doc comment still says the SDK's isRetryable flag is always followed, but 2xx failures now override it. Add the 2xx exception to the doc comment and drop the inline comment inside the function.
Evidence
Lines 26–28 make every APICallError with a 2xx status retryable, whatever the provider's flag says. The doc comment above is what tells callers how classification works, and it no longer matches the code.
wmcmahan
left a comment
There was a problem hiding this comment.
Advisory review by the pr-review workflow — the human merge decision stands either way.
VERDICT: REVISE
Summary: The pricing and model changes are right, but this PR also changes how agent turns finish and which errors get retried, which CONTRIBUTING.md says belongs in its own PR. The retry docs and one extra check call also need fixing.
The good part: per-model cached-input pricing is clean. It validates the new field, its tests check exact amounts, and adding claude-sonnet-5-5 and gpt-6.1-sol needs no change in context-engine profiles, because the existing claude-sonnet-5 and gpt-6 prefixes already cover them.
Overall
- The PR covers more than its title. Under "fix: update models and caching price" it also ships:
-
- an agent behaviour change: continuations with no tools, and checking the fallback text against
finalAnswer;
- an agent behaviour change: continuations with no tools, and checking the fallback text against
-
- a retry-policy change: any
APICallErrorwith a 2xx status is now retryable;
- a retry-policy change: any
-
- new error logging through
describeError.
- new error logging through
CONTRIBUTING.mdsays "Keep PRs focused. One concern per PR." The retry change affects every node'sfailure_policy, so it deserves its own PR and review. It is already a separate changeset (agent-continuation-without-tools.md). Move it, with theexecutor.tsanderror-classification.tschanges and their tests, into a second PR.
-
2 findings are posted as comments on the diff.
Reviewed at 5eaf62a · with claude-opus-5-5 · workflow run
|
|
||
| // No continuation is left, so an answer that still fails the check | ||
| // is kept but reported. | ||
| const unresolved = text.trim() === '' ? undefined : finalAnswerProblem(options?.finalAnswer, text, agentId); |
There was a problem hiding this comment.
When the first check passes, this runs the user's finalAnswer check again on the same text. A check that throws then logs final_answer_check_failed twice per turn. Reuse the earlier result, and only re-check when a continuation changed text.
Evidence
- On the happy path the check runs at line 625 or 621, then again here on the same text.
- After a continuation it runs a third time: once for the
recoveredlog field at line 703, then again here on that same continuation text.
No description provided.