Skip to content

fix(transport): drop invalid request_overrides keys instead of forwarding them - #61355

Closed
Sahil-SS9 wants to merge 3 commits into
NousResearch:mainfrom
Sahil-SS9:fix/issue-61030-request-overrides-system-kwarg
Closed

fix(transport): drop invalid request_overrides keys instead of forwarding them#61355
Sahil-SS9 wants to merge 3 commits into
NousResearch:mainfrom
Sahil-SS9:fix/issue-61030-request-overrides-system-kwarg

Conversation

@Sahil-SS9

@Sahil-SS9 Sahil-SS9 commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

request_overrides were merged directly into top-level api_kwargs, so a stray key could reach chat.completions.create() and raise an unexpected-keyword TypeError.

This fixes #61030: a legacy top-level system key from an old profile config was forwarded as a kwarg. It does not fix or close #60821; that report was resolved by correcting the third-party skill-enforce plugin's request["system"] mutation.

Fix

  • Filter unsupported top-level Chat Completions override keys.
  • Preserve and merge extra_body / extra_headers overrides.
  • Cover both ProviderProfile and no-provider_profile legacy paths.

Scope

Chat Completions only. The Responses API remains out of scope because it has a different argument surface.

Verification

python -m pytest tests/providers/test_profile_wiring.py -q — 26 passed.

Closes #61030

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jul 9, 2026
@rdxhemadri

rdxhemadri commented Jul 9, 2026

Copy link
Copy Markdown

@Sahil-SS9 so does it fix my #60821 or not, also thank you for looking into it

@Sahil-SS9
fix(transport): drop invalid request_overrides keys instead of forwar…
28b7b4e 
…ding them

request_overrides were merged directly into the top-level api_kwargs via
dict.update(), so any stray key (e.g. a legacy top-level `system` from an
old profile config) reached chat.completions.create() and raised
`TypeError: Completions.create() got an unexpected keyword argument '<key>'`
(hermes-agent#60821 / NousResearch#61030).

Add a _VALID_TOP_LEVEL_API_KWARGS allowlist and a _safe_merge_request_overrides
helper. Unknown keys are dropped with a debug log; extra_body/extra_headers are
deep-merged into the existing values rather than clobbered. Applied to both the
OpenAI and Gemini build_kwargs paths.

Tests: tests/providers/test_profile_wiring.py::TestRequestOverridesInvalidKeys
@Sahil-SS9
Sahil-SS9 force-pushed the fix/issue-61030-request-overrides-system-kwarg branch from 8d6cb5f to 28b7b4e Compare July 9, 2026 10:37
@falkoro falkoro mentioned this pull request Jul 9, 2026
Open
11 tasks
@alt-glitch alt-glitch mentioned this pull request Jul 9, 2026
Closed
teknium1
teknium1 reviewed Jul 10, 2026
View reviewed changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding defensive validation around the two Chat Completions override merge sites.

Problems

  • The claimed #60821 root cause does not match the linked investigation: its reporter confirmed that correcting the third-party skill-enforce plugin's request["system"] mutation resolved the crash. Please do not close #60821 from this PR without a separate request_overrides reproduction.
  • tests/providers/test_profile_wiring.py:302 covers only the ProviderProfile path, while this PR also replaces the legacy-path merge at agent/transports/chat_completions.py:538.
  • agent/transports/codex.py:295-297 still blindly updates Responses kwargs from request_overrides. Either cover that sibling API-specific path or narrow the PR's stated scope to Chat Completions.

Suggested changes

  • Reframe the issue linkage, add a legacy-path regression test, and clarify whether other transports are intentionally out of scope.

Automated hermes-sweeper review.

Comment thread tests/providers/test_profile_wiring.py
class TestRequestOverridesInvalidKeys:
"""Stray keys in request_overrides must NOT reach chat.completions.create()."""

def test_profile_path_drops_invalid_system_key(self, transport):

@teknium1 teknium1 Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exercises only the ProviderProfile path. _safe_merge_request_overrides() also replaces the legacy api_kwargs.update(overrides) path, so add a no-provider_profile invalid-key regression test to cover both changed call sites.

@Sahil-SS9

Sahil-SS9 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Addressed — added the no-provider-profile legacy-path regression and narrowed the PR to Chat Completions; Responses remains explicitly out of scope. Updated the PR description to make clear this fixes #61030, not #60821. Targeted tests: 26 passed.

@Sahil-SS9

Sahil-SS9 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@rdxhemadri Thanks — no, this does not fix #60821. That was resolved by correcting the skill-enforce plugin mutation. This PR separately prevents invalid request_overrides keys from reaching Chat Completions.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@Sahil-SS9

Sahil-SS9 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Hi @rdxhemadri, thanks for the question. This PR does not fix #60821 — #60821 was resolved by correcting the third-party skill-enforce plugin that was mutating request["system"]. This PR fixes a separate but related issue (#61030) where stray request_overrides keys from old profile configs could reach chat.completions.create() as unexpected kwargs. The PR description has been updated to reflect this distinction.

@Sahil-SS9 Sahil-SS9 closed this Aug 2, 2026
@Sahil-SS9
Sahil-SS9 deleted the fix/issue-61030-request-overrides-system-kwarg branch August 2, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teknium1 teknium1 teknium1 left review comments

Assignees

No one assigned

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Milestone

No milestone

4 participants

@Sahil-SS9 @rdxhemadri @teknium1 @alt-glitch