fix(agent): handle config rejection in streaming_chat path#2346
fix(agent): handle config rejection in streaming_chat path#2346YellowSnnowmann wants to merge 2 commits into
Conversation
…ections in OpenAiCompatibleProvider
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds provider config rejection error classification and logging to five OpenAI-compatible provider request paths. Each function now detects and logs config rejection responses via newly-inserted ChangesProvider Config Rejection Error Classification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
M3gA-Mind
left a comment
There was a problem hiding this comment.
Walkthrough
This PR closes a genuine gap: all five request arms in OpenAiCompatibleProvider (responses_api, streaming_chat, chat_completions, native_chat, stream_chat) were missing the is_provider_config_rejection_http branch that api_error in ops.rs already had. The fix is mechanically correct — each arm gets an identical else if inserted between the existing access-policy check and the generic should_report_provider_http_failure fallback, and anyhow::bail!(message) still fires below so the error is still propagated to callers. The provider_name.as_str() in the stream_chat arm (vs self.name.as_str() elsewhere) is the right choice for that closure context.
| Area | Files | Result |
|---|---|---|
| Rust core — error classification | compatible.rs |
5 arms corrected, logic consistent |
| Tests | compatible_tests.rs |
Missing — new branches uncovered |
One blocking issue before this can merge.
| Some(native_request.model.as_str()), | ||
| status, | ||
| ); | ||
| } else if super::is_provider_config_rejection_http(status, self.name.as_str(), &body) { |
There was a problem hiding this comment.
[major] Five new else if branches (39 lines across all five arms) but zero new tests. The project's testing strategy requires at least one failure-path test per new code path, and the coverage gate enforces ≥ 80% on changed lines via diff-cover — these branches won't be covered by the existing suite.
The helpers themselves are exercised in ops.rs (provider_config_rejection_suppression module), but the new arms in compatible.rs are not reachable from those unit tests. compatible_tests.rs has no mock-HTTP infrastructure that exercises error classification paths.
Suggested fix: add a #[tokio::test] in compatible_tests.rs (or a new compatible_config_rejection_tests.rs) using wiremock or httpmock to serve a 400 with a config-rejection body, then assert that the result is Err(...) (error still propagated) and that report_error was not called (i.e., no Sentry upload). One test covering the representative streaming_chat arm plus a brief comment that the same path applies to the other four would satisfy the gate.
Summary
OpenAiCompatibleProvider, route HTTP errors that match a known provider-config rejection (e.g. invalid/unauthorized API key, disabled model, billing block) tolog_provider_config_rejectioninstead ofobservability::report_error.responses_api,streaming_chat,chat_completions,native_chat, andstream_chat.OPENHUMAN-TAURI-NJ, which was being driven by user-config rejections being misclassified as provider bugs.should_report_provider_http_failure→report_erroras before.Problem
OpenAiCompatibleProviderwas funneling every non-401/429 HTTP error intoobservability::report_error, including HTTP responses that are expected when the user's provider config is wrong (bad key, wrong base URL, unsupported model, account suspended, etc.).OPENHUMAN-TAURI-NJaccumulated a high-volume stream of "errors" that were actually user-actionable config problems, not bugs. Real provider regressions got buried in the noise.is_provider_config_rejection_httpexists in the parent module — but only one arm (or none, depending on the path) was consulting it. The five request entry points had drifted out of sync.Solution
else if super::is_provider_config_rejection_http(status, self.name.as_str(), &error_body)branch to each of the five request paths incompatible.rs, sitting between the auth/rate-limit handling and the genericshould_report_provider_http_failurefallback.super::log_provider_config_rejection(<arm_name>, provider, model, status)so the rejection is recorded as a structured log line (with the originating arm as the first argument, e.g."responses_api","streaming_chat", …) instead of being uploaded to Sentry.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.N/A: error-classification refinement, no feature row added/removed/renamed.## Related—N/A: no matrix row touched.docs/RELEASE-MANUAL-SMOKE.md) —N/A: routing fix on an existing error path; no new smoke step.Closes #NNNin the## RelatedsectionImpact
OpenAiCompatibleProvider. No mobile/web/CLI surface touched directly.matcharm + helper call on the error path. Hot path (success) is unchanged.log_provider_config_rejectionfollows the same redaction rules as the existing log helpers; no new fields, no PII leakage.OPENHUMAN-TAURI-NJshould drop materially. In exchange, expect a corresponding rise in structured[inference][config_rejection]log lines, tagged with the originating arm so triage can tell which request shape tripped.Related
Summary by CodeRabbit