Skip to content

Fix bind_params_to_func silently accepting None for required parameters - #1225

Open
magic-peach wants to merge 1 commit into
dapr:mainfrom
magic-peach:fix/bind-params-silently-accepts-none-for-required-args
Open

magic-peach wants to merge 1 commit into
dapr:mainfrom
magic-peach:fix/bind-params-silently-accepts-none-for-required-args

Conversation

@magic-peach

Copy link
Copy Markdown
Contributor

Description

_coerce_and_validate in _conversation_helpers.py's bind_params_to_func silently accepted None for a required (non-Optional) parameter, either coercing it to the literal string 'None' or passing it through raw to crash downstream with an opaque AttributeError. Some Optional[str] = None cases were also affected depending on Union-member iteration order.

This adds an explicit check that raises a clear ToolArgumentError when None is passed for a required parameter, instead of coercing or silently passing it through.

Issue reference

Please reference the issue this PR will close: #[issue number]

N/A, self-discovered while auditing the Conversation API's tool-call argument binding, no existing issue filed.

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
@magic-peach
magic-peach requested review from a team as code owners September 20, 2026 14:26

@CasperGN CasperGN 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.

Approved.

I checked the case I was most worried about: optional parameters still accept None. The _is_union branch in _coerce_and_validate handles Optional[T] and returns None before the new if value is None guard is reached, and test_none_still_allowed_for_optional pins that. The guard only applies to non-Union types, which is the intent. Dropping or value is None from the dataclass branch is consistent, since the guard now catches it earlier.

One thing worth flagging for release notes: this is a visible behaviour change, not just a hardening. Models do pass explicit null for required parameters, and those calls previously bound successfully and failed later inside the tool body with an unrelated error. They now fail at bind time with a clear message. That is the better outcome, but callers relying on the old tolerance will see new ToolArgumentErrors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants