From 378ad1877c9b5f8f8bd82ffc6a190844231beb7c Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Tue, 4 Aug 2026 13:44:29 -0500 Subject: [PATCH 1/2] fix(build): anchor sdist include patterns to the project root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [tool.hatch.build.targets.sdist] listed "src/", "docs/", "tests/" and friends without a leading slash. Hatchling matches those at any depth, and .claude/worktrees/agent-*/ contains full copies of this repo, so each nested checkout's own docs/, src/ and tests/ were swept in: 1350 extra files, 2 MiB instead of 434 KiB. Some of those copies predate the commit that replaced real example identifiers with placeholders, so the sdist carried tenant values that no longer exist anywhere in the tracked tree. The wheel was unaffected — it builds from `packages` — and `git ls-files` came back clean, so this was invisible to a source audit and only showed up when the built artifact itself was inspected. Anchored every pattern with a leading slash. Verified: sdist identifier matches 35 -> 0, wheel 0, contents now exactly src/tests/packs/docs/examples plus the intended root files. --- CHANGELOG.md | 10 ++++++++++ pyproject.toml | 28 ++++++++++++++++++---------- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a8ffc..1353127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The sdist no longer sweeps in nested checkouts. `[tool.hatch.build.targets.sdist]` + listed patterns like `docs/` and `src/` without a leading slash, so they matched at + any depth — and `.claude/worktrees/agent-*/` holds full copies of the repo. The + 0.7.0 sdist picked up 1350 extra files that way (2 MiB instead of 434 KiB), + including older copies of `docs/configuration.md` and `docs/multi-company.md` from + before example identifiers were replaced with placeholders. The wheel was never + affected, since it builds from `packages`, and `git ls-files` was clean — only + inspecting the built artifact showed it. Patterns are now anchored to the project + root. + - `build_url` now validates `entity_set_name` and `record_id` as single URL path components, rejecting raw `/`, `\`, `?`, `#` and the `.`/`..` segments. Both values are spliced directly into the request path, so a record key containing diff --git a/pyproject.toml b/pyproject.toml index f633fcd..ecd0b38 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,17 +108,25 @@ packages = ["src/bcli", "src/bcli_cli", "src/bcli_mcp"] "packs" = "bcli/packs/_builtin" [tool.hatch.build.targets.sdist] +# Leading slashes anchor these to the project root. Without them the patterns +# match at any depth, so a nested checkout — .claude/worktrees/agent-*/ holds +# full copies of the repo — had its own docs/, src/ and tests/ swept into the +# sdist: 1350 extra files, including pre-sanitisation docs still carrying real +# tenant identifiers. The wheel was unaffected (it builds from `packages`), and +# `git ls-files` was clean, so only inspecting the built artifact caught it. +# If you add a pattern here, anchor it, then verify with: +# uv build && tar -tzf dist/bc_cli-*.tar.gz | awk -F/ 'NF>1{print $2}' | sort -u include = [ - "src/", - "packs/", - "tests/", - "examples/", - "docs/", - "README.md", - "LICENSE", - "NOTICE", - "CHANGELOG.md", - "pyproject.toml", + "/src", + "/packs", + "/tests", + "/examples", + "/docs", + "/README.md", + "/LICENSE", + "/NOTICE", + "/CHANGELOG.md", + "/pyproject.toml", ] [tool.pytest.ini_options] From 3f3946ed02fccf44e4403c9f0628e4e9bf339190 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Tue, 4 Aug 2026 15:50:27 -0500 Subject: [PATCH 2/2] fix(transport): do not retry non-idempotent requests without an idempotency key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry loop keyed only on status: `if status in _RETRYABLE and attempt < max_retries`, with no notion of whether repeating the request was safe. Network errors were treated the same way. So a 429/503/504 or a dropped connection on a POST, PATCH or DELETE re-sent a request the server may already have applied. That is not hypothetical for this API. Bound actions such as the LLP-utilisation recalculation take no arguments and mutate on every invocation, so there is no harmless repeat — one gateway 503 could recalculate twice and nothing in the log would say so. The header plumbing for Idempotency-Key already existed and simply was never a precondition for retrying. Retry now requires either a read-only method or a supplied Idempotency-Key, which is what allows a gateway or a future server-side implementation to collapse the duplicate. Otherwise the error surfaces: a visible transient failure the caller can retry deliberately beats an invisible double-write. DELETE and PUT are deliberately excluded despite being idempotent by HTTP semantics — a repeat there surfaces as a 404 or overwrites a concurrent change, and that is not a call an automatic retry should make for the caller. Behaviour change, called out in the changelog. 23 tests; no existing test depended on a mutation being retried. --- CHANGELOG.md | 10 ++ src/bcli/client/_transport.py | 31 ++++- tests/test_client/test_retry_idempotency.py | 124 ++++++++++++++++++++ 3 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 tests/test_client/test_retry_idempotency.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1353127..a1c0b09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Automatic retry no longer repeats non-idempotent requests. The transport + retried 429/503/504 and network errors for every method, so a lost response to + a POST/PATCH/DELETE that the server had already applied would duplicate a + create or re-run a bound action. Some bound actions in this API take no + arguments and mutate on every call, so a repeat is never harmless. Retries now + require either a read-only method or an explicit `Idempotency-Key`; otherwise + the error surfaces so the caller can decide. **This is a behaviour change**: a + transient 503 on a write that previously succeeded after a silent retry will + now raise. Pass `idempotency_key=` to opt back in. + - The sdist no longer sweeps in nested checkouts. `[tool.hatch.build.targets.sdist]` listed patterns like `docs/` and `src/` without a leading slash, so they matched at any depth — and `.claude/worktrees/agent-*/` holds full copies of the repo. The diff --git a/src/bcli/client/_transport.py b/src/bcli/client/_transport.py index d0e3576..0d51920 100644 --- a/src/bcli/client/_transport.py +++ b/src/bcli/client/_transport.py @@ -41,6 +41,13 @@ # Retryable status codes _RETRYABLE = {429, 503, 504} +#: Methods that can be repeated without applying an effect twice. Deliberately +#: excludes DELETE and PUT: both are idempotent by HTTP semantics, but a repeat +#: here surfaces as a 404 or overwrites a concurrent change, and neither is what +#: an automatic retry should decide on the caller's behalf. Pass an +#: ``idempotency_key`` to opt a mutation back into retrying. +_IDEMPOTENT_METHODS = frozenset({"GET", "HEAD"}) + DEFAULT_TIMEOUT = 60 DEFAULT_MAX_RETRIES = 3 INITIAL_BACKOFF = 1.0 # seconds @@ -144,6 +151,21 @@ async def _request( backoff = INITIAL_BACKOFF t0 = time.monotonic() + # Repeating a request is only safe when it cannot apply an effect twice. + # A read can always be repeated. A POST / PATCH / DELETE cannot: the + # server may already have applied it when the response was lost, so a + # retry duplicates a create or re-runs a business action. Some bound + # actions in this API take no arguments and mutate on every invocation, + # so there is no such thing as a harmless repeat — one 503 from a gateway + # could recalculate twice with nothing in the log to say so. + # + # An Idempotency-Key re-enables retry, because that is what lets a + # gateway (or a future server-side implementation) collapse the + # duplicate. Without one, the error surfaces instead: a visible + # transient failure the caller can retry deliberately beats an invisible + # double-write. + retry_safe = method.upper() in _IDEMPOTENT_METHODS or idempotency_key is not None + for attempt in range(self._max_retries + 1): try: auth_headers = await self._inject_auth() @@ -194,8 +216,8 @@ async def _request( bc_message, correlation_id = _parse_bc_error(response) status = response.status_code - # Retry on retryable errors - if status in _RETRYABLE and attempt < self._max_retries: + # Retry on retryable errors — but only if repeating is safe. + if status in _RETRYABLE and attempt < self._max_retries and retry_safe: retry_after = _get_retry_after(response) wait = retry_after if retry_after else backoff logger.warning( @@ -238,7 +260,10 @@ async def _request( httpx.RemoteProtocolError, ) as e: last_error = e - if attempt < self._max_retries: + # The most dangerous case for a mutation: the request may have + # reached the server and been applied before the connection + # dropped, so there is no way to know a retry is safe. + if attempt < self._max_retries and retry_safe: logger.warning( "Network error on %s %s: %s, retrying in %.1fs", method, url, e, backoff, diff --git a/tests/test_client/test_retry_idempotency.py b/tests/test_client/test_retry_idempotency.py new file mode 100644 index 0000000..6b16c56 --- /dev/null +++ b/tests/test_client/test_retry_idempotency.py @@ -0,0 +1,124 @@ +"""Automatic retry must not silently re-run a non-idempotent request. + +The transport retried 429/503/504 and network errors for *every* method. For a +GET that is free. For a POST, PATCH or DELETE it is not: the server may already +have applied the request when the response was lost, so a retry duplicates a +create or re-runs a business action. + +That is not theoretical for this SDK. Bound actions like the LLP-utilisation +recalculation take no arguments and mutate on every invocation — there is no +such thing as a harmless repeat. A single 503 from a gateway could recalculate +twice, and nothing in the log would say so. + +So retries are now allowed only when repeating the request is safe: + +* the method is read-only (GET / HEAD), or +* the caller supplied an ``Idempotency-Key``, which is what lets a gateway (or a + future server-side implementation) collapse the duplicate. + +Otherwise the error surfaces. A visible transient failure the caller can retry +deliberately is strictly better than an invisible double-write. +""" + +from __future__ import annotations + +import httpx +import pytest + +from bcli.client._transport import BCTransport +from bcli.errors import ServerError + + +class _StubAuth: + async def get_access_token(self) -> str: + return "token" + + def clear_cache(self) -> None: + return None + + +def _transport(handler: httpx.MockTransport) -> BCTransport: + t = BCTransport(_StubAuth(), max_retries=2) + t._client = httpx.AsyncClient(transport=handler) + return t + + +def _counting_handler(status: int): + """Always answers `status`; records how many times it was called.""" + calls: list[str] = [] + + def handle(request: httpx.Request) -> httpx.Response: + calls.append(request.method) + return httpx.Response(status, json={"error": {"message": "nope"}}) + + return httpx.MockTransport(handle), calls + + +class TestReadsStillRetry: + @pytest.mark.parametrize("status", [429, 503, 504]) + async def test_get_retries_as_before(self, status): + handler, calls = _counting_handler(status) + t = _transport(handler) + with pytest.raises(Exception): + await t._request("GET", "https://example.test/x") + assert len(calls) == 3, "GET should still use all attempts" + + +class TestMutationsDoNotRetryWithoutAKey: + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + @pytest.mark.parametrize("status", [429, 503, 504]) + async def test_single_attempt_only(self, method, status): + handler, calls = _counting_handler(status) + t = _transport(handler) + with pytest.raises(Exception): + await t._request(method, "https://example.test/x") + assert len(calls) == 1, ( + f"{method} was retried without an idempotency key; a lost response " + f"would duplicate the write" + ) + + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + async def test_network_error_is_not_retried_either(self, method): + """The dangerous case: the request may have been applied before the + connection dropped, so there is no way to know a retry is safe.""" + calls: list[str] = [] + + def handle(request: httpx.Request) -> httpx.Response: + calls.append(request.method) + raise httpx.ReadTimeout("boom", request=request) + + t = _transport(httpx.MockTransport(handle)) + with pytest.raises(ServerError): + await t._request(method, "https://example.test/x") + assert len(calls) == 1 + + +class TestAnIdempotencyKeyReEnablesRetry: + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + async def test_key_allows_retry(self, method): + handler, calls = _counting_handler(503) + t = _transport(handler) + with pytest.raises(Exception): + await t._request(method, "https://example.test/x", idempotency_key="k-1") + assert len(calls) == 3 + + async def test_the_key_is_actually_sent(self): + seen: list[str | None] = [] + + def handle(request: httpx.Request) -> httpx.Response: + seen.append(request.headers.get("Idempotency-Key")) + return httpx.Response(200, json={}) + + t = _transport(httpx.MockTransport(handle)) + await t._request("POST", "https://example.test/x", idempotency_key="k-2") + assert seen == ["k-2"] + + +class TestSuccessPathUnaffected: + @pytest.mark.parametrize("method", ["GET", "POST", "PATCH", "DELETE"]) + async def test_a_successful_mutation_still_works(self, method): + def handle(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"ok": True}) + + t = _transport(httpx.MockTransport(handle)) + assert await t._request(method, "https://example.test/x") == {"ok": True}