Repository navigation
fix(http-client-python): bind response in LRO output callback when it is read - #12019
Shubham Padkonde (Shubham-Padkonde) wants to merge 2 commits into
Conversation
… 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: 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. |
There was a problem hiding this comment.
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
responsewhen 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.
|
On the Copilot note about coverage through the generation/template path: the operations template gets the callback body straight from |
I kept the test at the serializer level deliberately, following what this package already does for the same class of bug. The templates call End-to-end coverage through the generation/template path in this repo lives in |
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>
|
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 The test still pins the fix after that change — reverting only and with the fix: 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 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 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 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. |
|
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! |
Fixes #11966
In
models-mode: nonewith TypedDict generation (generate_typeddict_only),_LROOperationSerializer.get_long_running_outputonly emittedresponse = pipeline_response.http_responsewhen the final LRO response had headers. The deserialization it emitted next still readresponse.content/response.json(), so a successful LRO failed withNameError: name 'response' is not defined.The assignment is now also emitted whenever the generated deserialization lines reference the local
response. The regex match deliberately excludespipeline_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.pybuilds an LRO operation with a JSON final response and no headers. It generatesget_long_running_output(sync and async) for three configurations:models-mode: nonewith TypedDicts;models-mode: none;dpg.For each, it asserts
responseis bound whenever it's used, thenexecs the generated callback against a fake pipeline response.pytest tests/unit: 154 passed. That excludes 3 files that need the generatedspecialwords/isodatetest packages, which I didn't build locally.test_typeddict.py::test_models_mode_typeddict_does_not_write_empty_models_folderfails the same way onmainin my environment, so it's unrelated.black(repo config) andpylint(repopylintrc) 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