Python: Fix walrus operator precedence for model_id kwarg in AzureOpenAIResponsesClient

[moonbox3]

Motivation and Context

When passing model_id as a keyword argument to AzureOpenAIResponsesClient, the walrus operator (:=) bound more loosely than and, causing model_id to be assigned the boolean result of the full expression (True) instead of the actual model name string. This meant deployment_name was set to "True" instead of the intended model identifier.

Fixes #4299

Description

The root cause was missing parentheses around the walrus assignment model_id := kwargs.pop("model_id", None). Without them, Python parsed the expression as model_id := (kwargs.pop("model_id", None) and not deployment_name), assigning the boolean result to model_id. The fix adds parentheses to ensure model_id receives the popped value first, then the boolean check runs separately. A test was added to verify that model_id="gpt-4o" correctly propagates as the deployment name.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by moonbox3's agent

[moonbox3]

Automated Code Review

Reviewers: 3 | Confidence: 93%

✓ Correctness

The diff correctly fixes a Python operator precedence bug. In the original code, model_id := kwargs.pop("model_id", None) and not deployment_name was parsed as model_id := (kwargs.pop("model_id", None) and not deployment_name) due to := having lower precedence than and. This caused model_id to be assigned a boolean value instead of the actual model ID string, so str(model_id) would produce "True" rather than the intended deployment name. The fix correctly parenthesizes the walrus operator assignment. The accompanying test covers the fixed behavior.

✓ Security Reliability

The diff fixes a genuine operator-precedence bug where the walrus operator bound to the entire and expression, causing model_id to be assigned a boolean instead of the actual model ID string. The parenthesization fix is correct and the accompanying test validates the expected behavior. No security or reliability concerns are introduced by this change.

✗ Test Coverage

The bug fix correctly adds parentheses around the walrus operator to fix operator precedence (the old code assigned pop(...) and not deployment_name to model_id instead of just pop(...)). The new test covers the basic happy path but misses the other branch of the condition (and not deployment_name) and doesn't assert the deployment name itself, only the model_id attribute.

Blocking Issues

• The test does not cover the case where both model_id kwarg and deployment_name are provided. The fixed condition has an and not deployment_name guard, so a test should verify that an explicit deployment_name is NOT overridden by model_id. Without this, a future regression that drops the and not deployment_name check would go undetected.

Suggestions

• Consider asserting the underlying deployment name (e.g., via the inner OpenAI client's base URL or a deployment-name attribute) rather than only model_id, to more directly verify the bug fix.
• Consider adding a test with model_id set to an empty string or None to verify no assignment occurs in that edge case.

---

Automated review by moonbox3's agents

[markwallace-microsoft]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/core/agent_framework/azure
_responses_client.py	48	5	89%	216, 276–279
TOTAL	22215	2759	87%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
4710	247 💤	0 ❌	0 🔥	1m 18s ⏱️

[Copilot]

Pull request overview

Fixes a Python bug in AzureOpenAIResponsesClient where walrus-operator precedence caused model_id=... passed via **kwargs to be coerced into a boolean, leading to an incorrect deployment name being set.

Changes:

• Add parentheses around the walrus assignment to ensure model_id captures the popped value before boolean logic is applied.
• Add a unit test verifying model_id="gpt-4o" propagates correctly to client.model_id.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
python/packages/core/agent_framework/azure/_responses_client.py	Fix walrus precedence so model_id kwarg correctly maps to deployment_name.
python/packages/core/tests/azure/test_azure_responses_client.py	Add regression test covering the model_id kwarg behavior (issue #4299).

[moonbox3]

Automated Code Review

Reviewers: 3 | Confidence: 85%

✓ Correctness

The production code change correctly fixes a subtle bug where the walrus operator combined with truthiness checks (if (model_id := ...) and not deployment_name) would silently ignore falsy-but-valid values like model_id="" and would also treat deployment_name="" as unset. The new explicit is not None checks are semantically correct and handle edge cases properly. The tests cover the important scenarios well, though test_init_model_id_kwarg_empty_string relies on downstream validation to raise ValueError for an empty deployment name, which is not implemented in this diff.

✓ Security Reliability

This diff is a reliability improvement that fixes how model_id and deployment_name are evaluated during initialization. The old code used truthiness checks (and not deployment_name) which would incorrectly treat empty strings and other falsy values as unset. The new code correctly uses explicit is not None identity checks. The accompanying tests cover the key edge cases (precedence, empty string, None). No security or reliability issues found.

✗ Test Coverage

The diff fixes a truthiness bug in the model_id handling (empty-string and other falsy-but-not-None values were silently ignored) and adds three new tests. Two tests (test_init_model_id_kwarg_does_not_override_deployment_name and test_init_model_id_kwarg_none) are well-targeted and have meaningful assertions. However, test_init_model_id_kwarg_empty_string expects a ValueError to be raised, yet the code change itself only sets deployment_name = str(model_id) (i.e., ""); the ValueError would have to originate from downstream validation not visible in this diff. If that downstream validation doesn't exist or changes, the test will fail or give a false sense of coverage. Additionally, there is no test that verifies a falsy-but-valid model_id (e.g., model_id=0) is actually forwarded as the deployment name — which is the core scenario the truthiness fix enables.

Blocking Issues

• test_init_model_id_kwarg_empty_string asserts pytest.raises(ValueError) but the code change only assigns deployment_name = str(model_id) (an empty string). If no downstream validator raises ValueError for an empty deployment name, this test will fail. Either add explicit validation in the constructor (e.g., if model_id is not None and not model_id: raise ValueError(...)) or change the test expectation to match actual behaviour.

Suggestions

• In test_init_model_id_kwarg_empty_string, the test expects ValueError when model_id="" is passed, but the code in this diff does not add any such validation — it will simply set deployment_name = "" and rely on downstream constructor logic to reject it. Consider either adding explicit validation in __init__ (e.g., if model_id is not None and not model_id: raise ValueError(...)) or adding a comment in the test documenting which downstream component is expected to raise the error, to avoid confusion if that behavior changes.
• Consider adding a type check or assertion that model_id is a str (or None) before passing it to str(), since kwargs.pop can return any type. This would surface misuse earlier with a clear error rather than silently converting arbitrary objects to their string representation.
• Add a test for a falsy-but-non-empty model_id value (e.g., model_id=0) to directly validate the fix for the truthiness bug — this is the exact scenario the old if (model_id := …) pattern mishandled.
• Consider adding explicit empty-string validation in the constructor so the ValueError contract tested in test_init_model_id_kwarg_empty_string is clearly self-contained rather than depending on downstream behaviour.

---

Automated review by moonbox3's agents