feat(ledger): resolve a reconciling item against the preview it was decided on - #235
Merged
Merged
Conversation
…ecided on resolve_reconciling_item takes expected_drift_detected_at, the preview's drift_detected_at; the server refuses the resolution when the item was flagged again since. Optional, so existing calls are unchanged.
jfrench9
marked this pull request as ready for review
October 5, 2026 22:04
jfrench9
added a commit
that referenced
this pull request
Oct 5, 2026
…236) ## Summary Final fixes from the 2.6.0 verification pass. The main one: #234's `monitor_operation(timeout=…)` raised `TimeoutError` about 30 seconds late. Its timer closed the stream from another thread, but that does not wake a socket read that is already blocked. The read only returned when the stream's own 30-second read timeout ran out, even with the server sending keepalives. #234's tests replaced the stream with a mock, so they never touched a real socket. ## Changes - **`OperationClient.monitor_operation`:** with a `timeout`, the stream is read on a worker thread, and the caller waits on an event set by a terminal event or by the stream ending. The timeout now fires on time. A finished run returns at once, even if the server holds the socket open after the terminal event. Without a `timeout`, the call reads the stream on the calling thread as before. - **GraphQL reads (`GraphQLClient`):** the resolved token now replaces any credential in the static headers, so exactly one is sent, as the REST writes already do. Before, a static `X-API-Key` plus a `token_provider` JWT sent both headers. - **`create_report`:** `period_start` / `period_end` are annotated `str | datetime.date`, which is what they already accepted. ## Compatibility Rides in the next client release with #235. That release is a **minor** (2.7.0) because #235 adds a facade parameter. This PR changes no signatures; the annotation change only widens a type. ## Testing - **New:** `test_monitor_timeout_fires_on_time_over_a_real_socket` runs a local SSE server that holds the stream open with keepalives. `timeout=1` now raises in about 1 s; without the fix the same test fails after 31 s. - **New:** two `GraphQLClient` tests check that exactly one credential is sent. Both fail without the fix. - **`just test-all`:** 654 passed, 17 skipped; ruff, format and basedpyright are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds optional
expected_drift_detected_atto resolving a reconciling item: pass the preview'sdrift_detected_at, and the server refuses the resolution when the item was flagged again since.ResolveReconcilingItemRequestgains the field, and nothing else in the regen changed.Draft until robosystems #1696 is deployed: the field only exists on the server from that PR. The Python facade's
resolve_reconciling_itemtakes it as a keyword.🤖 Generated with Claude Code