fix(auxiliary_client): dedup 3 remaining resolve_provider_client dead-ends - #56367
fix(auxiliary_client): dedup 3 remaining resolve_provider_client dead-ends#56367srojk34 wants to merge 1 commit into
Conversation
…-ends Same anti-pattern the previous two dedup passes fixed (unknown provider, unhandled auth_type, unsupported external-process/OAuth provider): three more static-misconfiguration fall-throughs in the named-custom-provider and copilot-acp branches logged via logger.warning on every call for a persistently-misconfigured provider, spamming the logs forever until the user edits config.yaml: - named custom provider with no resolvable api_key - named custom provider with no base_url - copilot-acp requested with no model configured anywhere Demote all three to logger.debug with the same per-process dedup-set pattern already established in this file (first occurrence still surfaces, identical repeats suppressed for the process lifetime).
tonydwb
left a comment
There was a problem hiding this comment.
LGTM — dedup of remaining resolve_provider_client dead-ends. The pattern is consistent with prior dedup work: demote to debug, first-occurrence surfacing, per-provider dedup sets. Tests verify both the dedup and the level demotion.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying the resolver dedup pattern through the remaining retry-prone paths. Current main still has the hard no-base-url and copilot-acp/no-model returns at agent/auxiliary_client.py:4818-4821 and agent/auxiliary_client.py:4995-5000, and the proposed set-based DEBUG dedup matches mainline commits c0d3ceb17 and 9cf47fef5.
Problems
- The no-key branch is not a static misconfiguration dead-end:
agent/auxiliary_client.py:4736-4817intentionally usesno-key-requiredand constructs a client, whilewebsite/docs/integrations/providers.md:1190-1197documents omitted keys for local named providers. The added comments/test description should not state that configuration must be edited or that the branch itself guarantees a 401.
Suggested changes
- Reword the no-key comments and test docstring to distinguish supported keyless local endpoints from auth-required endpoints; retain the deduplicated DEBUG diagnostic behavior.
Automated hermes-sweeper review.
| # with no matching handler. Keyed by provider name. | ||
| _LOGGED_UNSUPPORTED_EXTPROC_KEYS: set = set() | ||
| _LOGGED_UNSUPPORTED_OAUTH_KEYS: set = set() | ||
| # Same treatment for the remaining static-misconfiguration dead-ends in the |
There was a problem hiding this comment.
Keyless named custom providers are supported for local endpoints (website/docs/integrations/providers.md:1190-1197), so please avoid calling this a static-misconfiguration dead end. Reword this comment to describe a repeated diagnostic for endpoints that require authentication; the dedup behavior can remain unchanged.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address retry-driven log spam in resolve_provider_client: merged #56334 established per-process DEBUG dedup for four fall-through branches, while #56367 extends that pattern to the remaining named-provider and copilot-acp paths.
Related pull requests
- #56334 [merged]
related— (+157/-7) — merged reference implementation: demotes and deduplicates repeated diagnostics for unknown providers, unhandled auth types, and unsupported external-process/OAuth providers, with focused regression tests; it remains relevant because #56367 builds directly on this established pattern. - #56367
related— (+110/-13) — keep open, then merge after revision: the diff correctly adds first-occurrence DEBUG dedup for missingbase_url, missingcopilot-acpmodel, and absent custom-provider keys, with tests for level and suppression. The contributor keep_open review must be addressed by correcting the no-key comments and test description: keyless local providers are supported, so this branch is not inherently a static misconfiguration or guaranteed 401.
Suggested consolidation
Merge #56367 after its no-key documentation and test wording are corrected as requested by the contributor review; its code extends rather than duplicates the merged reference implementation in #56334, so no PR should be closed as a duplicate.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 4 kB of issue/PR text, 1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
Two commits merged earlier today demoted 4 of
resolve_provider_client's static-misconfiguration dead-ends fromlogger.warningto dedupedlogger.debug(unknown provider, unhandled auth_type, unsupported external-process provider, unsupported OAuth provider) — each of these fires on every retry/call for a persistently-misconfigured provider, spamming the logs forever until the user editsconfig.yaml.Three more fall-throughs in the same function have the identical shape and weren't covered by either commit:
agent/auxiliary_client.py:4227— named custom provider (config.yamlproviders/custom_providersentry) with no resolvableapi_key. Worse than the others: it doesn'treturn None, None— it continues with a placeholder key, so it also guarantees a 401 on every call.agent/auxiliary_client.py:4310— named custom provider with nobase_url— a hardreturn None, Nonedead-end, same shape as the two already-fixed cases.agent/auxiliary_client.py:4490—copilot-acprequested with no model resolvable anywhere (explicit arg / provider default / main model) — textually 20 lines above the already-fixed "external-process provider not supported" branch in the sameauth_type == "external_process"block.Changes
agent/auxiliary_client.py: three new module-level dedup sets (_LOGGED_NAMED_CUSTOM_NOKEY_KEYS,_LOGGED_NAMED_CUSTOM_NOBASEURL_KEYS,_LOGGED_COPILOT_ACP_NOMODEL_KEYS), following the exact pattern already established in this file: first occurrence still surfaces (real diagnostic value), identical repeats suppressed atlogger.debugfor the process lifetime.tests/agent/test_auxiliary_client_resolve_dedup.py: three new test classes mirroring the existingTestUnsupportedOAuthDedup/TestUnhandledAuthTypeDedupstyle — each asserts the first call logs at DEBUG (never WARNING) and a second identical call is suppressed.Test plan
pytest tests/agent/test_auxiliary_client_resolve_dedup.py -q— 8 passed (5 pre-existing + 3 new)pytest tests/agent/test_auxiliary_client.py tests/agent/test_auxiliary_client_azure_foundry.py tests/agent/test_auxiliary_client_anthropic_custom.py tests/agent/test_auxiliary_client_proxy_env.py tests/agent/test_auxiliary_client_xai_oauth_recovery.py tests/agent/test_auxiliary_client_base_url_host_validation_52608.py -q— 311 passedruff checkon all changed files — clean