Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The smoke-test error handler checks an exception type the client never raises, hiding useful API error details.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds configurable HTTP timeouts and nullable condition support to the Python client library.
Changes:
- Forwards custom timeouts to sync and async HTTP clients.
- Allows marking decision points without an assigned condition.
- Adds documentation, tests, and a live smoke-test script.
| File | Description |
|---|---|
clientlibs/python/src/upgrade_client_lib/api_service.py |
Applies timeouts and accepts nullable condition codes. |
clientlibs/python/src/upgrade_client_lib/client.py |
Exposes timeout configuration and nullable conditions. |
clientlibs/python/tests/test_api_service.py |
Tests timeout forwarding and nullable marks. |
clientlibs/python/tests/test_client.py |
Adds client-level coverage. |
clientlibs/python/README.md |
Documents the new APIs. |
clientlibs/python/quickTest.py |
Adds a live smoke-test utility. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zackcl
left a comment
zackcl
left a comment
There was a problem hiding this comment.
The timeout and nullable-condition changes look good to me.
With the recent /reward API changes, could we expose an optional await_result=False in the async and sync reward methods and pass it through as the awaitResult query parameter?
I think including mode and the optional success field in SendRewardResponse would help keep the client aligned with the updated API. It would be good to cover both response modes and HTTP 409 handling when await_result=True in the tests.
| async def mark_decision_point( | ||
| self, | ||
| condition: str, | ||
| condition: str | None, |
There was a problem hiding this comment.
Could we change MarkDecisionPointCondition.conditionCode in types/requests.py to str | None to match this signature? The request builder already handles None, but the public request model still rejects it. A small validation/serialization test for "conditionCode": null might help keep them consistent.
| log_error("Assign (ignore cache)", error) | ||
|
|
||
|
|
||
| async def do_get_decision_point_assignment(client: UpgradeClient) -> str | None: |
There was a problem hiding this comment.
Could we add from __future__ import annotations after the module docstring? The package supports Python 3.9+, but evaluating str | None here raises a TypeError on Python 3.9 when the function is defined, before it can even be called. Postponing annotation evaluation should address that compatibility issue.
There was a problem hiding this comment.
🟢 Approval recommended
The additive changes preserve existing defaults, align with the reviewed backend contracts, and include targeted regression coverage with no identified blocking issues.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
No description provided.