Skip to content

fix(http-client-python): bind response in LRO output callback when it is read - #12019

Closed
Shubham Padkonde (Shubham-Padkonde) wants to merge 2 commits into
microsoft:mainfrom
Shubham-Padkonde:fix-python-lro-output-response
Closed

Shubham Padkonde (Shubham-Padkonde) wants to merge 2 commits into
microsoft:mainfrom
Shubham-Padkonde:fix-python-lro-output-response

Conversation

@Shubham-Padkonde

Copy link
Copy Markdown
Contributor

Fixes #11966

In models-mode: none with TypedDict generation (generate_typeddict_only), _LROOperationSerializer.get_long_running_output only emitted response = pipeline_response.http_response when the final LRO response had headers. The deserialization it emitted next still read response.content / response.json(), so a successful LRO failed with NameError: name 'response' is not defined.

The assignment is now also emitted whenever the generated deserialization lines reference the local response. The regex match deliberately excludes pipeline_response. The existing conditions are unchanged, so generated output only differs in the case that was broken.

Tests

New tests/unit/test_lro_output_callback.py builds an LRO operation with a JSON final response and no headers. It generates get_long_running_output (sync and async) for three configurations:

  • models-mode: none with TypedDicts;
  • models-mode: none;
  • dpg.

For each, it asserts response is bound whenever it's used, then execs the generated callback against a fake pipeline response.

  • Without the change, the two TypedDict cases fail. With it, all 6 pass.
  • pytest tests/unit: 154 passed. That excludes 3 files that need the generated specialwords / isodate test packages, which I didn't build locally.
  • test_typeddict.py::test_models_mode_typeddict_does_not_write_empty_models_folder fails the same way on main in my environment, so it's unrelated.
  • black (repo config) and pylint (repo pylintrc) are clean on the changed files.

I did not regenerate the checked-in test SDKs. Existing configurations produce the same output, so none should change.

Added a chronus change file (fix, @typespec/http-client-python).

This change was written with help from an AI coding assistant (Claude Code). I reviewed and tested it as described above.

🤖 Generated with Claude Code

… is read

With models-mode none and TypedDict generation, get_long_running_output
only assigned `response = pipeline_response.http_response` when the
final LRO response had headers, but the deserialization still read
`response.content` / `response.json()`. A successful LRO then raised
NameError. Emit the assignment whenever the generated deserialization
uses `response`.

Fixes microsoft#11966

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the emitter:client:python Issue for the Python client emitter: @typespec/http-client-python label Sep 19, 2026

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Add coverage through the normal generation/template path, not only the serializer directly.

Review effort: Lite
Findings: None

What changed in this PR

Fixes unbound response references in generated Python LRO callbacks for model-less clients.

Changes:

  • Bind response when generated deserialization uses it.
  • Add sync/async regression coverage across model configurations.
  • Add a Chronus fix entry.
File Summary
packages/​http-client-python/​tests/​unit/​test_lro_output_callback.py Tests callback generation and execution across configurations.
packages/​http-client-python/​generator/​pygen/​codegen/​serializers/​builder_serializer.py Emits the response binding when required by deserialization.
.chronus/​changes/​python-lro-output-response-2026-8-19.md Records the package bug fix.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Shubham-Padkonde

Copy link
Copy Markdown
Contributor Author

On the Copilot note about coverage through the generation/template path: the operations template gets the callback body straight from _LROOperationSerializer.get_long_running_output, so the new unit test covers the lines that end up in the generated file. It also execs the generated callback to show it runs without the NameError. The regenerate step in CI covers the full template path: for every existing configuration the emitted response = … line is unchanged, so the checked-in generated SDKs shouldn't change. If you'd also like a spec under the generator test suites that exercises models-mode: none with a headerless LRO final response, I can add one.

@Shubham-Padkonde

Copy link
Copy Markdown
Contributor Author

Add coverage through the normal generation/template path, not only the serializer directly.

I kept the test at the serializer level deliberately, following what this package already does for the same class of bug. tests/unit/ has five tests that build a CodeModel and drive a serializer directly — test_paging_json_body_placement.py, test_content_type_optional_body.py, test_api_version_validation.py, test_sse_streaming.py and test_typeddict.py. The first of those is the closest analogue: it is regression coverage for an UnboundLocalError in generated paging code, exactly the shape of this bug (a NameError on response), and it asserts on the serializer's output rather than generating a package.

The templates call get_long_running_output and emit its result verbatim, so the serializer output is the generated code for this path. The test goes one step further than the existing ones and execs the generated callback against a fake pipeline response, which is what actually reproduces the NameError.

End-to-end coverage through the generation/template path in this repo lives in tests/generated/, which is regenerated from the specs in CI rather than hand-written. If you'd like a spec added there for a model-less LRO with a JSON final response and no headers, I'm happy to do that in this PR — just point me at the right spec set.

The serializer reaches the response through builder.lro_response, which
is an LROResponse in a real code model. The test built a plain Response,
so it exercised a slightly different model than generation does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 21, 2026 08:45

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Shubham-Padkonde

Copy link
Copy Markdown
Contributor Author

Thanks — chasing this down found a real fidelity gap in the test, which I've fixed, and I want to be straight about how far I got on the rest.

Fixed: the test built its response as a plain Response, but the serializer reaches it through builder.lro_response, which is an LROResponse in a real code model. So the test was exercising a slightly different model than generation does. It now builds an LROResponse with the poller metadata a real one carries. Pushed in 35dba90dd.

The test still pins the fix after that change — reverting only builder_serializer.py to main:

E  AssertionError: assert '    response = pipeline_response.http_response' in
   ['def get_long_running_output(pipeline_response):', '    if response.content:',
    '        deserialized = response.json()', ...]
2 failed, 4 passed

and with the fix: 6 passed.

On going through the template path: I tried it rather than assuming. The Jinja template does call the serializer directly —

{{ op_tools.serialize(operation_serializer.get_long_running_output(operation)) | indent }}

— so rendering lro_operation.py.jinja2 would be genuine end-to-end coverage of exactly this line. I got the render most of the way there by supplying the missing code-model options (tracing, flavor, package-name) and the LROResponse poller fields, but it then needs initial_call and return_lro_poller, which require client.link_lro_initial_operations() to have run over a client whose operation groups actually contain the operation. Building that graph synthetically is a good deal more scaffolding than any test in tests/unit/ currently does — the existing ones (test_sse_streaming.py, test_paging_json_body_placement.py, test_optional_return_type.py) all construct models and call serializers directly, as this one does.

Worth noting the test is not only asserting on strings: it compiles and executes the generated callback against a fake pipeline response across all six parameterisations (three model configurations × sync/async), so a NameError for an unbound response fails the test outright rather than relying on a substring match.

If you'd still like the full template render, I'm happy to add it — either with the scaffolding above, or by pointing it at one of the specs under tests/ if there's an existing helper for generating from a spec that I've missed. Just let me know which you'd prefer.


Disclosure: this change was written by Claude Code (Claude Opus 5) working as my agent, at my direction. The test output quoted above comes from real runs in my local environment; I am accountable for this PR.

@l0lawrence

Copy link
Copy Markdown
Member

Hi Shubham Padkonde (@Shubham-Padkonde) thanks for opening this PR. We’ve decided to resolve this issue through a different approach, so I’m closing this PR. Thanks for the contribution!

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

Labels

emitter:client:python Issue for the Python client emitter: @typespec/http-client-python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: [Python] Modeless ARM LRO callback references undefined response

3 participants