From 276e17ef13d47f7f00cc8e17e9cacab6cfa47335 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 12:25:05 -0700 Subject: [PATCH 1/7] fix(chat): enforce ConcurrencyConfig.max_concurrent + release 0.4.26.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #51. Upstream TS accepts the config field but never enforces it (3 writes, 0 reads). Python now uses an asyncio.Semaphore so a consumer setting `concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=N)` actually gets an N-way cap on in-flight handlers. Divergence from upstream — Python leads here. The upstream behavior is documented ("Default: Infinity") but not implemented; our enforcement matches the documented contract. Changes: - `Chat.__init__` constructs `asyncio.Semaphore(max_concurrent)` when the value is set and positive, leaves it `None` otherwise (unbounded). - `_handle_concurrent` acquires the semaphore around `_dispatch_to_handlers` when present; falls through to direct dispatch when `None`. - 2 new tests in `TestConcurrencyConcurrent`: - `test_max_concurrent_bounds_in_flight_handlers`: 5 messages with `max_concurrent=2`; verifies `max_observed == 2` at any time. - `test_max_concurrent_none_allows_unbounded`: 5 concurrent handlers without a bound all run in parallel (regression guard for the default). Release prep for 0.4.26.1: - Version bump `0.4.26` → `0.4.26.1` in pyproject.toml - `UPSTREAM_PARITY` stays `4.26.0` (still synced to 4.26.0 upstream; no new upstream sync in this release) - CHANGELOG entry with Fixes / Python-specific / New public APIs / Internals / Known gaps sections - CONTRIBUTING.md version-scheme wording: `.patch` bumps during alpha can be additive (fixes + features). Tightens to "fixes-only" once we hit 1.0. Test plan: - `uv run ruff check src/ tests/ scripts/` — clean - `uv run ruff format --check src/ tests/ scripts/` — clean - `uv run python scripts/audit_test_quality.py` — 0 hard failures - `uv run pytest tests/ --tb=short -q` — 3471 passed, 2 skipped --- CHANGELOG.md | 73 +++++++++++++++++++++++++--- CONTRIBUTING.md | 11 ++++- pyproject.toml | 2 +- src/chat_sdk/chat.py | 23 ++++++++- tests/test_chat_faithful.py | 96 +++++++++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3f4c7e..4c9c367 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,75 @@ # Changelog -## Unreleased +## 0.4.26.1 (2026-04-23) + +Python-only follow-up on `0.4.26`. Still alpha — APIs may change. ### Fixes -- **Slack native streaming no longer crashes on first chunk** (issue #44): `SlackAdapter.stream()` now awaits `AsyncWebClient.chat_stream(...)`; the previous code called `.append()` on an unawaited coroutine, raising `AttributeError` and forcing callers onto the post+edit fallback. Existing tests were updated to use `AsyncMock` for `chat_stream` so they mirror the real client. -- **Teams divider now renders a visible separator line** (issue #45): `card_to_adaptive_card` previously emitted an empty `Container` with `separator: True`, which Microsoft Teams renders at zero height. The new behavior hoists `separator: True` onto the following sibling (or emits a minimal non-empty Container for a trailing divider). Upstream TS ships the same bug; documented as a divergence in [UPSTREAM_SYNC.md](docs/UPSTREAM_SYNC.md). -### Python-only additions -- **`SlackAdapter.current_token` / `current_client`** (issue #47): public `@property` accessors that return the request-context-bound bot token and a preconfigured `AsyncWebClient`. Replaces reaching into `_get_token()` / `_get_client()` from consumer code that needs to call the Slack Web API directly from inside a handler (email resolution, user profile fetches, etc.). TS keeps `getToken()` private; documented as a Python-only extension in [UPSTREAM_SYNC.md](docs/UPSTREAM_SYNC.md). -- **`Chat.thread(thread_id, *, current_message=None)`** (issue #46): public worker-reconstruction factory mirroring TS `chat.thread(threadId)`. Adapter is inferred from the thread ID prefix; state and message history come from the Chat instance. Pass `current_message` when the worker needs Slack native streaming (it populates `recipient_user_id` / `recipient_team_id`). +- **Slack native streaming**: `SlackAdapter.stream()` no longer calls + `AsyncWebClient.chat_stream(...)` without `await`. The unawaited coroutine + returned a truthy object, and the first `streamer.append(...)` raised + `AttributeError`, breaking native Slack streaming for any consumer using + the default adapter. Issue #44. +- **Teams divider renders at non-zero height**: empty `Container` with + `separator: True` rendered as zero-height in the Teams UI. Dividers + between siblings now hoist `separator: True` onto the following element; + a trailing divider emits a minimal non-empty Container. Issue #45. +- **`ConcurrencyConfig.max_concurrent` is now enforced**: consumers setting + `concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=N)` + now actually get an `asyncio.Semaphore(N)` cap on in-flight handlers. + Previously the field was accepted and ignored (upstream TS has the same + gap). `None` / unset keeps the unbounded default. Issue #51. + +### Python-specific (divergence from upstream 4.26) + +- **Fallback streaming runtime robustness** (cluster of fixes): framework- + agnostic `request.text()` handling now tolerates sync Flask-style + requests (was raising `TypeError: object is not awaitable`). Handlers + typed `Callable[..., Awaitable[None] | None]` may return sync (`None`) — + the dispatcher now `await`s only when `inspect.isawaitable()` confirms, + preventing runtime crashes on sync handlers. +- **`max_concurrent` enforcement** (see above) — upstream accepts the + config field but never enforces it; we do. + +### New public APIs + +- **`Chat.thread(thread_id, *, current_message=None)`**: new worker- + reconstruction factory mirroring TS `chat.thread(threadId)`. Adapter is + inferred from the thread-ID prefix; state and message history come from + the Chat instance. `current_message` is preserved so Slack native + streaming still works post-reconstruction. Issue #46. +- **`SlackAdapter.current_token` / `current_client`**: public `@property` + accessors for the request-context-bound bot token and a preconfigured + `AsyncWebClient`. Replaces underscore access from consumer code making + direct Slack Web API calls inside a handler (email resolution, user + profile fetches, etc.). Issue #47. + +### Internals + +- **Pyrefly: 213 → 0 type errors**; baseline file removed. CI now enforces + zero errors. Root causes fixed: 8-adapter `lock_scope: LockScope | None` + protocol conformance; `_ChatSingleton` as `Protocol`; submodule-aware + `replace-imports-with-any`; `NoReturn` on error re-raisers; + `inspect.isawaitable` guards for duck-typed request handling and + sync-or-async handler dispatch. No `Any` widening, no new `# type: + ignore` lines beyond 10 at adapter event-construction sites where + `thread=None`/`channel=None` get re-wrapped by `Chat` before handler + dispatch (matches upstream TS's `Omit<>` partial-event pattern). +- Test count: **3471 passed**, 2 skipped. + +### Known gaps (not fixed in this release) + +- `onOptionsLoad` handler for dynamic select dropdowns — issue #50 +- `Thread.getParticipants()` method — issue #54 +- `rehydrate_attachment` adapter hook for queue/debounce + attachments — + issue #52 +- 40 upstream tests without Python equivalents (Options Load, Plan variants, + StreamingPlan options, getParticipants) — issue #53 +- Discord native Gateway WebSocket (HTTP-only today) — issue #57 +- Teams certificate-based mTLS auth — issue #58 +- Google Chat file uploads (TODO upstream too) — issue #59 +- Global handler-dispatch bound across reactions/actions/slash/modals — issue #61 ## 0.4.26 (2026-04-16) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9ba3d3b..b1cb3ce 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,13 +58,20 @@ All PRs must pass `ruff check` with zero errors. Vercel Chat version. See [UPSTREAM_SYNC.md](docs/UPSTREAM_SYNC.md#version-mapping). - `0.4.25` = synced to upstream `4.25.0` -- `0.4.25.1` = Python-only fix on top of `4.25.0` +- `0.4.25.1` = Python-only changes (fixes, and additive features during + alpha) between upstream sync points - `0.4.26a1` = alpha while porting upstream `4.26.0` +> **Additive changes in `.patch` bumps are OK during alpha**. The package +> is marked `Development Status :: 3 - Alpha` and the `0.x.y` prefix signals +> pre-1.0 per semver convention, so new public APIs can land in `.patch` +> bumps without a version-scheme violation. Once we hit `1.0`, `.patch` +> should be fixes-only. +> > **Upstream patch releases**: Vercel Chat has historically gone straight to > minor bumps, but if upstream ships a patch (e.g. `4.25.1`) we sync it by > bumping to the next minor (`0.4.26`). We don't reuse the `.patch` slot for -> upstream patches — it's reserved for Python-only fixes so the two can't +> upstream patches — it's reserved for Python-only changes so the two can't > collide. ### Steps diff --git a/pyproject.toml b/pyproject.toml index 36b34b6..1c14d62 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "chat-sdk" -version = "0.4.26" +version = "0.4.26.1" description = "Multi-platform async chat SDK for Python — port of Vercel Chat" readme = "README.md" license = {text = "MIT"} diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 3d5c10d..3d011eb 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -297,6 +297,19 @@ def __init__(self, config: ChatConfig | None = None, **kwargs: Any) -> None: self._concurrency_on_queue_full = concurrency.on_queue_full self._concurrency_queue_entry_ttl_ms = concurrency.queue_entry_ttl_ms + # -- Concurrent-strategy semaphore ------------------------------------ + # `max_concurrent` bounds in-flight handler dispatches when using the + # `"concurrent"` strategy. `None` means unbounded (matches the upstream + # TS default of `Infinity`). A non-None value caps parallel handler + # runs at that number — useful for rate-limit hygiene and bounded + # resource use. Divergence from upstream: upstream accepts the config + # field but doesn't enforce it; we do. + self._concurrent_semaphore: asyncio.Semaphore | None = ( + asyncio.Semaphore(self._concurrency_max_concurrent) + if self._concurrency_max_concurrent is not None and self._concurrency_max_concurrent > 0 + else None + ) + # -- Message history (placeholder -- real impl would use MessageHistoryCache) self._message_history = _MessageHistoryCache(self._state_adapter, config.message_history) @@ -1840,7 +1853,15 @@ async def _handle_concurrent( thread_id: str, message: Message, ) -> None: - await self._dispatch_to_handlers(adapter, thread_id, message) + # Enforce `max_concurrent` bound when configured. Upstream TS + # accepts the config field but never enforces it; we do, so that + # consumers setting `ConcurrencyConfig(strategy="concurrent", + # max_concurrent=N)` actually get a bound of N in-flight handlers. + if self._concurrent_semaphore is None: + await self._dispatch_to_handlers(adapter, thread_id, message) + return + async with self._concurrent_semaphore: + await self._dispatch_to_handlers(adapter, thread_id, message) # ======================================================================== # Dispatch to handlers diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index bb24eae..b5ea4a8 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2263,6 +2263,102 @@ async def handler(thread, message, context=None): assert len(calls) == 1 assert calls[0] == "Hey @slack-bot concurrent" + # Python-specific: upstream accepts max_concurrent but doesn't enforce + # it. We do. Bound should cap in-flight handlers at N; the (N+1)th + # message has to wait until one of the first N releases. + async def test_max_concurrent_bounds_in_flight_handlers(self): + import asyncio + + state = create_mock_state() + adapter = create_mock_adapter("slack") + + chat, _, _ = await _init_chat( + adapter=adapter, + state=state, + concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=2), + ) + + in_flight = 0 + max_observed = 0 + gate = asyncio.Event() + finished = 0 + + @chat.on_mention + async def handler(thread, message, context=None): + nonlocal in_flight, max_observed, finished + in_flight += 1 + max_observed = max(max_observed, in_flight) + await gate.wait() + in_flight -= 1 + finished += 1 + + # Dispatch 5 messages concurrently — at most 2 should be in flight + # at any time while the gate is closed. + tasks = [ + asyncio.create_task( + chat.handle_incoming_message( + adapter, + f"slack:C123:{i}", + create_test_message(f"msg-{i}", "Hey @slack-bot"), + ) + ) + for i in range(5) + ] + + # Let the first two handlers enter and block on the gate. + for _ in range(20): + await asyncio.sleep(0) + assert in_flight == 2 + assert max_observed == 2 + + # Release the gate; all 5 should drain. + gate.set() + await asyncio.gather(*tasks) + + assert finished == 5 + # The bound was never exceeded. + assert max_observed == 2 + + # Python-specific: None / missing max_concurrent must keep the + # unbounded behavior (matches upstream TS default of Infinity). + async def test_max_concurrent_none_allows_unbounded(self): + import asyncio + + state = create_mock_state() + adapter = create_mock_adapter("slack") + + chat, _, _ = await _init_chat(adapter=adapter, state=state, concurrency="concurrent") + + in_flight = 0 + max_observed = 0 + gate = asyncio.Event() + + @chat.on_mention + async def handler(thread, message, context=None): + nonlocal in_flight, max_observed + in_flight += 1 + max_observed = max(max_observed, in_flight) + await gate.wait() + in_flight -= 1 + + tasks = [ + asyncio.create_task( + chat.handle_incoming_message( + adapter, + f"slack:C123:{i}", + create_test_message(f"msg-{i}", "Hey @slack-bot"), + ) + ) + for i in range(5) + ] + for _ in range(20): + await asyncio.sleep(0) + # All 5 should be in flight simultaneously (no bound). + assert in_flight == 5 + gate.set() + await asyncio.gather(*tasks) + assert max_observed == 5 + # ============================================================================ # 22. lockScope (tests 87-91) From 09287d65780336b08f08fe1d8ed8bace45b57542 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 13:27:09 -0700 Subject: [PATCH 2/7] address review comments on PR #60 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Raise `ValueError` on `max_concurrent <= 0` instead of silently falling back to unbounded (CodeRabbit MAJOR). A user setting `max_concurrent=0` likely means "throttle hard" and would be surprised by unlimited dispatch otherwise. - Remove redundant local `import asyncio` in 2 test methods — module is already imported at the top of the file (github-code-quality). - Soften early `assert max_observed == 2` to `<= 2`. The early check is about "bound not exceeded during gate-closed phase"; the final `== 2` after drain is what asserts the peak was actually reached (github-code-quality "redundant comparison"). - New test: `test_max_concurrent_zero_or_negative_raises` covers 0, -1, -100. --- src/chat_sdk/chat.py | 18 +++++++++++++----- tests/test_chat_faithful.py | 30 +++++++++++++++++++++++------- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 3d011eb..057c809 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -300,13 +300,21 @@ def __init__(self, config: ChatConfig | None = None, **kwargs: Any) -> None: # -- Concurrent-strategy semaphore ------------------------------------ # `max_concurrent` bounds in-flight handler dispatches when using the # `"concurrent"` strategy. `None` means unbounded (matches the upstream - # TS default of `Infinity`). A non-None value caps parallel handler - # runs at that number — useful for rate-limit hygiene and bounded - # resource use. Divergence from upstream: upstream accepts the config - # field but doesn't enforce it; we do. + # TS default of `Infinity`). A positive integer caps parallel handler + # runs. Divergence from upstream: upstream accepts the config field + # but doesn't enforce it; we do. + # + # Reject `<= 0` explicitly rather than silently ignoring — a user + # passing `max_concurrent=0` likely means "pause all processing" + # (not supported) or has a typo. Either way, silently falling back + # to unbounded concurrency would surprise them. + if self._concurrency_max_concurrent is not None and self._concurrency_max_concurrent <= 0: + raise ValueError( + f"ConcurrencyConfig.max_concurrent must be > 0 or None, got {self._concurrency_max_concurrent}" + ) self._concurrent_semaphore: asyncio.Semaphore | None = ( asyncio.Semaphore(self._concurrency_max_concurrent) - if self._concurrency_max_concurrent is not None and self._concurrency_max_concurrent > 0 + if self._concurrency_max_concurrent is not None else None ) diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index b5ea4a8..6d2f771 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2267,8 +2267,6 @@ async def handler(thread, message, context=None): # it. We do. Bound should cap in-flight handlers at N; the (N+1)th # message has to wait until one of the first N releases. async def test_max_concurrent_bounds_in_flight_handlers(self): - import asyncio - state = create_mock_state() adapter = create_mock_adapter("slack") @@ -2305,25 +2303,43 @@ async def handler(thread, message, context=None): for i in range(5) ] - # Let the first two handlers enter and block on the gate. + # Let the first two handlers enter and block on the gate. Others + # must be blocked on the semaphore, so `in_flight` is capped at 2 + # and the observed peak is <= 2 (not `== 2` — we'd narrow to 2 + # only after the final drain below, when the peak is meaningful). for _ in range(20): await asyncio.sleep(0) assert in_flight == 2 - assert max_observed == 2 + assert max_observed <= 2 # Release the gate; all 5 should drain. gate.set() await asyncio.gather(*tasks) assert finished == 5 - # The bound was never exceeded. + # The bound was never exceeded; peak reached exactly 2. assert max_observed == 2 + # Python-specific: reject invalid `max_concurrent` values at construction + # time rather than silently falling back to unbounded (which would + # surprise users who set `max_concurrent=0` expecting strict throttling). + async def test_max_concurrent_zero_or_negative_raises(self): + state = create_mock_state() + adapter = create_mock_adapter("slack") + + for bad_value in (0, -1, -100): + import pytest + + with pytest.raises(ValueError, match="max_concurrent must be > 0 or None"): + await _init_chat( + adapter=adapter, + state=state, + concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=bad_value), + ) + # Python-specific: None / missing max_concurrent must keep the # unbounded behavior (matches upstream TS default of Infinity). async def test_max_concurrent_none_allows_unbounded(self): - import asyncio - state = create_mock_state() adapter = create_mock_adapter("slack") From 5f0c1542f47c71b730d30fd101b0f037093ac0d2 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 13:50:13 -0700 Subject: [PATCH 3/7] address self-review findings on PR #60 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - **Reject `max_concurrent` with non-concurrent strategy**: if a user sets `ConcurrencyConfig(strategy="queue", max_concurrent=5)`, they have a misconfig — the field is only honored under `"concurrent"`. Upstream accepts silently but we fail-fast so it's never invisible. New test `test_max_concurrent_with_non_concurrent_strategy_raises`. - **Tighten error message**: raw input via `!r` repr instead of private `self._concurrency_max_concurrent` attribute; message now also tells the user how to fix ("pass None for unbounded"). - **Test robustness**: replace `for _ in range(20): await asyncio.sleep(0)` yield-counting loops with `asyncio.wait_for(_reach_cap(), timeout=1.0)` polling until the condition holds. Fixed counts can be flaky on slow CI when handlers have multiple internal await points. - **Document divergence in UPSTREAM_SYNC.md**: added row to Known Non-Parity table with rationale (upstream accepts the config field but never enforces it; we do). - **Inline breadcrumb comment** `# Divergence from upstream — see docs/UPSTREAM_SYNC.md` at the `_concurrent_semaphore` construction site, matching the divergence discipline in CLAUDE.md. --- docs/UPSTREAM_SYNC.md | 1 + src/chat_sdk/chat.py | 21 +++++++++++++--- tests/test_chat_faithful.py | 48 ++++++++++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 1d2d89b..4013833 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -457,6 +457,7 @@ stay explicit instead of being rediscovered in code review. | Fallback streaming final SentMessage content | SentMessage + final edit carry `final_content` (remend'd — inline markers auto-closed) | SentMessage + final edit carry raw `accumulated` | Narrow UX refinement. If a stream ends with an unclosed `*`/`~~`/etc., upstream ships the unclosed marker; we run `_remend` so the user sees a clean final message. Not observable in the common case where streams close their own markers. | | Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | | `SlackAdapter.current_token` / `current_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | Not exposed (`getToken()` is private on the TS `SlackAdapter`) | Python-only addition (issue #47). Downstream code that calls Slack Web APIs from inside a handler — email resolution, user profile fetches, reaction bookkeeping — otherwise depends on underscore-prefixed helpers. | +| `ConcurrencyConfig.max_concurrent` | Enforced via `asyncio.Semaphore` in the `"concurrent"` strategy path; rejects `<= 0` and rejects the field under non-`"concurrent"` strategies | Accepted into the config type with docstring "Default: Infinity" but never read (3 writes, 0 reads) | Silent correctness bug upstream — consumers setting `max_concurrent=N` with `strategy="concurrent"` reasonably expect an N-way bound on in-flight handlers. We honor the documented contract via a semaphore and fail-fast on misconfiguration so it's never silent. | ### Platform-specific gaps diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 057c809..cfe1176 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -39,6 +39,7 @@ Channel, ChannelVisibility, ChatConfig, + ConcurrencyConfig, ConcurrencyStrategy, EmojiValue, Lock, @@ -298,19 +299,33 @@ def __init__(self, config: ChatConfig | None = None, **kwargs: Any) -> None: self._concurrency_queue_entry_ttl_ms = concurrency.queue_entry_ttl_ms # -- Concurrent-strategy semaphore ------------------------------------ + # Divergence from upstream — see docs/UPSTREAM_SYNC.md. # `max_concurrent` bounds in-flight handler dispatches when using the # `"concurrent"` strategy. `None` means unbounded (matches the upstream # TS default of `Infinity`). A positive integer caps parallel handler - # runs. Divergence from upstream: upstream accepts the config field - # but doesn't enforce it; we do. + # runs. Upstream accepts the config field but never enforces it + # (3 writes, 0 reads); we enforce it via `asyncio.Semaphore`. + # + # Only construct the semaphore when the strategy actually uses it — + # if a user sets `max_concurrent=5` with `strategy="queue"`, they + # have a misconfiguration that we surface as a `ValueError` rather + # than silently allocating an unused primitive. # # Reject `<= 0` explicitly rather than silently ignoring — a user # passing `max_concurrent=0` likely means "pause all processing" # (not supported) or has a typo. Either way, silently falling back # to unbounded concurrency would surprise them. + raw_max = concurrency.max_concurrent if isinstance(concurrency, ConcurrencyConfig) else None if self._concurrency_max_concurrent is not None and self._concurrency_max_concurrent <= 0: raise ValueError( - f"ConcurrencyConfig.max_concurrent must be > 0 or None, got {self._concurrency_max_concurrent}" + f"ConcurrencyConfig.max_concurrent must be a positive integer or None; " + f"got {raw_max!r}. Pass None for unbounded concurrency." + ) + if self._concurrency_max_concurrent is not None and self._concurrency_strategy != "concurrent": + raise ValueError( + f"ConcurrencyConfig.max_concurrent is only honored when strategy='concurrent'; " + f"got strategy={self._concurrency_strategy!r}. Either switch to strategy='concurrent' " + "or drop max_concurrent." ) self._concurrent_semaphore: asyncio.Semaphore | None = ( asyncio.Semaphore(self._concurrency_max_concurrent) diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index 6d2f771..65a5020 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2303,11 +2303,19 @@ async def handler(thread, message, context=None): for i in range(5) ] - # Let the first two handlers enter and block on the gate. Others - # must be blocked on the semaphore, so `in_flight` is capped at 2 - # and the observed peak is <= 2 (not `== 2` — we'd narrow to 2 - # only after the final drain below, when the peak is meaningful). - for _ in range(20): + # Poll until the first 2 handlers have reached the gate. Fixed + # yield counts can be flaky on slow CI — a handler that needs to + # traverse a few internal `await`s before reaching `gate.wait()` + # may not show up in N cycles. Poll with a short timeout instead. + async def _reach_cap() -> None: + while in_flight < 2: + await asyncio.sleep(0.001) + + await asyncio.wait_for(_reach_cap(), timeout=1.0) + + # Let any would-be extra tasks finish racing to the gate; if the + # bound leaks, in_flight would climb above 2 here. + for _ in range(10): await asyncio.sleep(0) assert in_flight == 2 assert max_observed <= 2 @@ -2330,13 +2338,30 @@ async def test_max_concurrent_zero_or_negative_raises(self): for bad_value in (0, -1, -100): import pytest - with pytest.raises(ValueError, match="max_concurrent must be > 0 or None"): + with pytest.raises(ValueError, match="max_concurrent must be a positive integer or None"): await _init_chat( adapter=adapter, state=state, concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=bad_value), ) + # Python-specific: setting `max_concurrent` with a non-concurrent strategy + # is a misconfiguration — the field is only honored under `"concurrent"`. + # Fail loudly instead of silently allocating an unused semaphore. + async def test_max_concurrent_with_non_concurrent_strategy_raises(self): + import pytest + + state = create_mock_state() + adapter = create_mock_adapter("slack") + + for bad_strategy in ("queue", "debounce", "drop"): + with pytest.raises(ValueError, match="only honored when strategy='concurrent'"): + await _init_chat( + adapter=adapter, + state=state, + concurrency=ConcurrencyConfig(strategy=bad_strategy, max_concurrent=5), + ) + # Python-specific: None / missing max_concurrent must keep the # unbounded behavior (matches upstream TS default of Infinity). async def test_max_concurrent_none_allows_unbounded(self): @@ -2367,9 +2392,14 @@ async def handler(thread, message, context=None): ) for i in range(5) ] - for _ in range(20): - await asyncio.sleep(0) - # All 5 should be in flight simultaneously (no bound). + + # Poll until all 5 are in flight; with no semaphore they should + # all reach the gate. + async def _reach_five() -> None: + while in_flight < 5: + await asyncio.sleep(0.001) + + await asyncio.wait_for(_reach_five(), timeout=1.0) assert in_flight == 5 gate.set() await asyncio.gather(*tasks) From a8e7062ceb812c21dabb275a6d1529e03f0e2e6a Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 14:07:25 -0700 Subject: [PATCH 4/7] drop racy `in_flight == 2` post-poll assertion, rely on max_observed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught: between `_reach_cap` returning and the `assert in_flight == 2` line, a third task could briefly reach the semaphore-gated handler on a fast multi-core runner. The 10-yield loop that followed was also vulnerable. `max_observed` is captured inside the handler under the cooperative scheduler — it's the atomic signal for 'did the bound leak'. The final `assert max_observed == 2` after all tasks drain is the authoritative check. Removing the point-in-time assertions eliminates the race without losing coverage. --- tests/test_chat_faithful.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index 65a5020..e5a59b0 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2303,29 +2303,26 @@ async def handler(thread, message, context=None): for i in range(5) ] - # Poll until the first 2 handlers have reached the gate. Fixed - # yield counts can be flaky on slow CI — a handler that needs to - # traverse a few internal `await`s before reaching `gate.wait()` - # may not show up in N cycles. Poll with a short timeout instead. + # Wait until the first 2 handlers have reached the gate, then + # drain the other 3 after releasing it. We rely exclusively on + # `max_observed` (captured inside the handler, atomic under the + # asyncio cooperative scheduler) to detect a semaphore leak — + # point-in-time checks between polling and assertion are racy + # on fast multi-core runners. async def _reach_cap() -> None: while in_flight < 2: await asyncio.sleep(0.001) await asyncio.wait_for(_reach_cap(), timeout=1.0) - # Let any would-be extra tasks finish racing to the gate; if the - # bound leaks, in_flight would climb above 2 here. - for _ in range(10): - await asyncio.sleep(0) - assert in_flight == 2 - assert max_observed <= 2 - - # Release the gate; all 5 should drain. + # Release the gate; all 5 should drain. If the semaphore leaked, + # `max_observed` inside the handlers captured the peak before + # any could unblock, so the final assertion below would fail. gate.set() await asyncio.gather(*tasks) assert finished == 5 - # The bound was never exceeded; peak reached exactly 2. + # The critical assertion: peak in-flight never exceeded 2. assert max_observed == 2 # Python-specific: reject invalid `max_concurrent` values at construction From 08b8c7ca67a25216437de469df6fcf91d170c5ac Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 14:11:43 -0700 Subject: [PATCH 5/7] parameterize unbounded test: cover both 'concurrent' string + explicit max_concurrent=None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit minor: the test only verified the string form ('concurrent'); adding the ConcurrencyConfig(max_concurrent=None) case proves the explicit null path also stays unbounded. The two take different code paths in Chat.__init__ (string branch → defaults all fields; dataclass branch → reads the field), so both need coverage. --- tests/test_chat_faithful.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index e5a59b0..cba3ef3 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2361,11 +2361,22 @@ async def test_max_concurrent_with_non_concurrent_strategy_raises(self): # Python-specific: None / missing max_concurrent must keep the # unbounded behavior (matches upstream TS default of Infinity). - async def test_max_concurrent_none_allows_unbounded(self): + # Parameterized to cover both the string form (max_concurrent implicit) + # and the explicit ConcurrencyConfig(max_concurrent=None) form — the + # two take separate code paths in Chat.__init__ (string → defaults, + # ConcurrencyConfig → field read), so both must be verified. + @pytest.mark.parametrize( + "concurrency_value", + [ + "concurrent", + ConcurrencyConfig(strategy="concurrent", max_concurrent=None), + ], + ) + async def test_max_concurrent_none_allows_unbounded(self, concurrency_value): state = create_mock_state() adapter = create_mock_adapter("slack") - chat, _, _ = await _init_chat(adapter=adapter, state=state, concurrency="concurrent") + chat, _, _ = await _init_chat(adapter=adapter, state=state, concurrency=concurrency_value) in_flight = 0 max_observed = 0 From ff10803ba5f0bf2d782e6211f737514ccae60cc2 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 14:17:59 -0700 Subject: [PATCH 6/7] restore in_flight == 2 assertion + add readable parametrize ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review caught my over-correction. asyncio uses a single-threaded cooperative scheduler — tasks 3-5 are parked on semaphore.acquire() and can't interleave between '_reach_cap' returning and the next synchronous assertion. 'in_flight == 2' IS stable there. Keeping max_observed == 2 as the primary signal + in_flight == 2 as a snapshot check while the gate is closed gives stronger coverage than either alone. Also: added ids=['string', 'config_none'] to the unbounded parametrize. Without it pytest falls back to 'concurrency_value1' for the ConcurrencyConfig case, which is opaque in CI logs. --- CHANGELOG.md | 2 +- tests/test_chat_faithful.py | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c9c367..2a2b487 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,7 @@ Python-only follow-up on `0.4.26`. Still alpha — APIs may change. ignore` lines beyond 10 at adapter event-construction sites where `thread=None`/`channel=None` get re-wrapped by `Chat` before handler dispatch (matches upstream TS's `Omit<>` partial-event pattern). -- Test count: **3471 passed**, 2 skipped. +- Test count: **3544 passed**, 2 skipped. ### Known gaps (not fixed in this release) diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index cba3ef3..70572ba 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2303,17 +2303,19 @@ async def handler(thread, message, context=None): for i in range(5) ] - # Wait until the first 2 handlers have reached the gate, then - # drain the other 3 after releasing it. We rely exclusively on - # `max_observed` (captured inside the handler, atomic under the - # asyncio cooperative scheduler) to detect a semaphore leak — - # point-in-time checks between polling and assertion are racy - # on fast multi-core runners. + # Wait until the first 2 handlers reach the gate. asyncio uses a + # single-threaded cooperative scheduler, so between `_reach_cap` + # returning and the next assertion, no other task can interleave + # — tasks 3-5 are parked on `semaphore.acquire()`. The + # `in_flight == 2` check IS stable here. async def _reach_cap() -> None: while in_flight < 2: await asyncio.sleep(0.001) await asyncio.wait_for(_reach_cap(), timeout=1.0) + # Snapshot while the gate is still closed: exactly the bound + # should be in flight, and no more. + assert in_flight == 2 # Release the gate; all 5 should drain. If the semaphore leaked, # `max_observed` inside the handlers captured the peak before @@ -2371,6 +2373,7 @@ async def test_max_concurrent_with_non_concurrent_strategy_raises(self): "concurrent", ConcurrencyConfig(strategy="concurrent", max_concurrent=None), ], + ids=["string", "config_none"], ) async def test_max_concurrent_none_allows_unbounded(self, concurrency_value): state = create_mock_state() From 7dc7ea94b7e900c41a72045a70b9c1a8c7357a21 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 15:45:49 -0700 Subject: [PATCH 7/7] harden max_concurrent validation + clarify divergence doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject non-integer `max_concurrent` (float, bool, str) at construction instead of deferring to `asyncio.Semaphore` behavior — `1.5` silently drives the counter negative; `True` allocates a 1-way bound from a bool; `"2"` raises the wrong exception. CodeRabbit flagged. - Clarify the `ConcurrencyConfig.max_concurrent` divergence row in UPSTREAM_SYNC: rejection only fires when the field is non-`None`; `max_concurrent=None` stays compatible with every strategy. - Added `test_max_concurrent_non_integer_raises` covering six bad-type cases (float, True, False, str, 0.0, list). - Refresh stale test count (3544 → 3545). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- docs/UPSTREAM_SYNC.md | 2 +- src/chat_sdk/chat.py | 9 ++++++++- tests/test_chat_faithful.py | 19 +++++++++++++++++++ 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a2b487..ed69854 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,7 @@ Python-only follow-up on `0.4.26`. Still alpha — APIs may change. ignore` lines beyond 10 at adapter event-construction sites where `thread=None`/`channel=None` get re-wrapped by `Chat` before handler dispatch (matches upstream TS's `Omit<>` partial-event pattern). -- Test count: **3544 passed**, 2 skipped. +- Test count: **3545 passed**, 2 skipped. ### Known gaps (not fixed in this release) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 4013833..2600324 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -457,7 +457,7 @@ stay explicit instead of being rediscovered in code review. | Fallback streaming final SentMessage content | SentMessage + final edit carry `final_content` (remend'd — inline markers auto-closed) | SentMessage + final edit carry raw `accumulated` | Narrow UX refinement. If a stream ends with an unclosed `*`/`~~`/etc., upstream ships the unclosed marker; we run `_remend` so the user sees a clean final message. Not observable in the common case where streams close their own markers. | | Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | | `SlackAdapter.current_token` / `current_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | Not exposed (`getToken()` is private on the TS `SlackAdapter`) | Python-only addition (issue #47). Downstream code that calls Slack Web APIs from inside a handler — email resolution, user profile fetches, reaction bookkeeping — otherwise depends on underscore-prefixed helpers. | -| `ConcurrencyConfig.max_concurrent` | Enforced via `asyncio.Semaphore` in the `"concurrent"` strategy path; rejects `<= 0` and rejects the field under non-`"concurrent"` strategies | Accepted into the config type with docstring "Default: Infinity" but never read (3 writes, 0 reads) | Silent correctness bug upstream — consumers setting `max_concurrent=N` with `strategy="concurrent"` reasonably expect an N-way bound on in-flight handlers. We honor the documented contract via a semaphore and fail-fast on misconfiguration so it's never silent. | +| `ConcurrencyConfig.max_concurrent` | Enforced via `asyncio.Semaphore` in the `"concurrent"` strategy path; rejects non-integer or `<= 0` values, and rejects any non-`None` `max_concurrent` paired with a non-`"concurrent"` strategy | Accepted into the config type with docstring "Default: Infinity" but never read (3 writes, 0 reads) | Silent correctness bug upstream — consumers setting `max_concurrent=N` with `strategy="concurrent"` reasonably expect an N-way bound on in-flight handlers. We honor the documented contract via a semaphore and fail-fast on misconfiguration so it's never silent. `max_concurrent=None` stays compatible with every strategy (unbounded default). | ### Platform-specific gaps diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index cfe1176..aaac6c6 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -316,7 +316,14 @@ def __init__(self, config: ChatConfig | None = None, **kwargs: Any) -> None: # (not supported) or has a typo. Either way, silently falling back # to unbounded concurrency would surprise them. raw_max = concurrency.max_concurrent if isinstance(concurrency, ConcurrencyConfig) else None - if self._concurrency_max_concurrent is not None and self._concurrency_max_concurrent <= 0: + if raw_max is not None and ( + # Reject non-int (including bool, which is an int subclass but + # semantically meaningless here) before any arithmetic — + # `asyncio.Semaphore(1.5)` silently goes negative, `Semaphore(True)` + # allocates a 1-way bound from a boolean, and `Semaphore("2")` + # raises `TypeError` instead of our ValueError. + isinstance(raw_max, bool) or not isinstance(raw_max, int) or raw_max <= 0 + ): raise ValueError( f"ConcurrencyConfig.max_concurrent must be a positive integer or None; " f"got {raw_max!r}. Pass None for unbounded concurrency." diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index 70572ba..4bb8766 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -2344,6 +2344,25 @@ async def test_max_concurrent_zero_or_negative_raises(self): concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=bad_value), ) + # Python-specific: reject non-integer `max_concurrent` at construction + # instead of letting `asyncio.Semaphore` misbehave (`1.5` silently drives + # the counter negative, `True` allocates a 1-way bound from a bool, + # `"2"` raises `TypeError` from inside the primitive instead of our + # `ValueError`). + async def test_max_concurrent_non_integer_raises(self): + import pytest + + state = create_mock_state() + adapter = create_mock_adapter("slack") + + for bad_value in (1.5, True, False, "2", 0.0, [1]): + with pytest.raises(ValueError, match="max_concurrent must be a positive integer or None"): + await _init_chat( + adapter=adapter, + state=state, + concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=bad_value), # type: ignore[arg-type] + ) + # Python-specific: setting `max_concurrent` with a non-concurrent strategy # is a misconfiguration — the field is only honored under `"concurrent"`. # Fail loudly instead of silently allocating an unused semaphore.