Python: Reject every multi-source combination in detect_media_type_from_base64 - #8557
sxh (sxh313) wants to merge 1 commit into
Conversation
…om_base64 The mutual-exclusion guard read a variable that only mirrored data_bytes, while the data URI payload was rebound into data_str, so passing both data_str and data_uri was silently accepted (the URI won) instead of raising. Move the check ahead of the rebind and split the pytest.raises blocks that hid the untested combinations.
|
sxh (@sxh313) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
sxh (@sxh313) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation enforces the documented contract and the tests cover every invalid source combination.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes mutual-exclusion validation for base64 media-type detection and strengthens regression coverage.
Changes:
- Rejects
data_strcombined withdata_uri. - Parametrizes all invalid source combinations.
- Removes previously unreachable test code.
| File | Description |
|---|---|
python/packages/core/agent_framework/_types.py |
Validates conflicting inputs before rebinding URI data. |
python/packages/core/tests/core/test_types.py |
Tests each invalid input combination independently. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
cd0b324 to
a2d7f08
Compare
Motivation & Context
detect_media_type_from_base64()documents that exactly one ofdata_bytes,data_str, ordata_urimust be provided, and raises
ValueErrorotherwise. That contract was not enforced for onecombination:
data_strtogether withdata_uri.The guard compared
data is not None, butdataonly ever mirrorsdata_bytes. When a data URI issupplied, its payload is rebound into the
data_strlocal variable (prefix, data_str = data_uri.split(",", 1)),so by the time the
data_strbranch runs, the caller'sdata_stris gone. The call was accepted and theURI silently won:
This is the root cause of the second block reported in #5115: five of the six documented cases sat inside a
single
with pytest.raises(...)block, so only the first ever executed and the bug stayed invisible.Description & Review Guide
python/packages/core/agent_framework/_types.py: the mutual-exclusion check inside thedata_uribranchnow also covers a caller-supplied
data_str, i.e. it runs before the URI payload is rebound into thatvariable.
python/packages/core/tests/core/test_types.py: the two multi-statementpytest.raisesblocks are splitinto one parametrized test that exercises every rejected combination (each in its own
raisesblock) plus aregression test that asserts the specific
data_str+data_uricase.Every input that supplies zero or more than one source now raises
ValueError("Provide exactly one of data_bytes, data_str, or data_uri."), matching the docstring. Single-sourcebehavior is unchanged. No new error type, message, or signature change, so callers that used the API as
documented are unaffected.
Whether
data_bytes+data_str+data_uri(all three) should keep the same message, which it does here,and whether you prefer this ordering fix over hoisting a single upfront
sum(...) != 1guard.Deliberately out of scope: the first block of #5115 also notes that
data_str=""returnsNonerather thanraising. That needs the maintainer decision recorded in the issue (whether an empty string is valid input or
should be rejected), so this PR only removes the dead statement and leaves the behavior as is.
The closed PR #5171 changed tests only; it did not fix the source and did not cover the silent
data_str+data_uriacceptance.Related Issue
Fixes #5115
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.