test(devin): assert which error the refused send raises and what the caller sees - #5170
Conversation
…caller sees Two follow-ups to #5152, both test-only. Nothing in production changes and the merged branch is untouched. The adapter-direct case caught the refusal with a bare catch that discarded it, so it passed for any error at all. If the adapter had thrown an ordinary upstream failure instead of the budget refusal, the case would still have been green while the turn was reported to the operator as a provider problem rather than a budget decision. It now pins SendBudgetExhaustedError. The outer case reported the HTTP status only inside its failure message. It is now asserted: the refusal reaches the client as an error code on a buffered failed response, so the status is 200. That is worth stating, because the obvious guess is 429 and this path does not use it. Local checks: NOT RUN. Hosted CI on this PR is the executable evidence.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe tests now verify that Devin send-budget refusal exposes ChangesSend budget refusal contract
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
|
Maintainer integration into Reviewed head: Exact-head applicable hosted CI and aggregate passed: https://github.com/lidge-jun/opencodex/actions/runs/35443894116 . Target enforcement passed at the same head in run 35443894094; a separate canceled duplicate is not used as proof. Event-inapplicable jobs remain skipped. Local tests, typecheck, build and runtime execution were not run, per the task constraint. |
Summary
Two test-only follow-ups to #5152. Nothing in production changes, and the merged branch is untouched.
The adapter-direct case for a refused initial send caught the refusal with a bare
.catch(() => {})and discarded it. That passed for any error at all: if the adapter had thrown an ordinary upstream failure instead of the budget refusal, the case would still have been green while the turn reached the operator as a provider problem rather than a budget decision. The case now captures what escaped and assertsSendBudgetExhaustedError, alongside its existing assertions that no inference request was made and no permit was spent.The outer case reported the HTTP status only inside its failure message. It is now asserted. The refusal reaches the client as an error code on a buffered failed response, so the outer status is 200, not 429. That is worth pinning precisely because 429 is the obvious guess and this path does not use it; a future change that turned the refusal into a transport-level status would now be caught here rather than discovered downstream.
Relates to #5122.
Verification
ocxcommand. Hosted CI on this PR is the executable evidence.Checklist
Summary by CodeRabbit