Skip to content

fix: launch must-fix items — security, perf, docs#7

Merged
patrick-chinchill merged 1 commit into
mainfrom
fix/launch-must-fix-items
Apr 6, 2026
Merged

fix: launch must-fix items — security, perf, docs#7
patrick-chinchill merged 1 commit into
mainfrom
fix/launch-must-fix-items

Conversation

@patrick-chinchill
Copy link
Copy Markdown
Collaborator

Summary

  • Teams JWT bypass: Reject webhooks with 401 when app_id is empty instead of silently skipping JWT verification. Log a warning at init time when app_id is missing.
  • Teams SSRF protection: Validate service_url against the Bot Framework allow-list in open_dm and _cache_user_context before using/storing untrusted URLs.
  • Teams JWT issuer: Add issuer="https://api.botframework.com" to the pyjwt.decode call.
  • Slack client caching: Cache AsyncWebClient instances by token in _get_client() instead of creating a new instance per request.
  • README: Add "Why chat-sdk?" value prop and "Compared to Alternatives" table; update version from 0.0.1a1 to 0.0.1a3.
  • CONTRIBUTING.md: New file with dev setup, testing, adapter guide, and PR expectations.
  • CHANGELOG.md: New file documenting the 0.0.1a3 initial alpha release.

Test plan

  • Verify Teams adapter rejects webhooks when TEAMS_APP_ID is unset (returns 401)
  • Verify Teams adapter logs warning at init when app_id is empty
  • Verify _cache_user_context refuses to cache a non-allowlisted serviceUrl
  • Verify open_dm raises ValidationError for non-allowlisted serviceUrl
  • Verify JWT decode now requires issuer="https://api.botframework.com"
  • Verify Slack _get_client() returns cached client for same token
  • Verify README renders correctly and version matches pyproject.toml
  • uv run ruff check src/ passes clean

🤖 Generated with Claude Code

Security fixes:
- Teams: reject webhooks when app_id is empty instead of silently
  skipping JWT verification (was a complete auth bypass)
- Teams: add issuer validation to JWT decode (require
  https://api.botframework.com)
- Teams: validate service_url against SSRF allow-list in open_dm
  and _cache_user_context before storing/using untrusted URLs

Performance:
- Slack: cache AsyncWebClient instances by token instead of creating
  a new client on every request

Docs:
- README: add "Why chat-sdk?" and "Compared to Alternatives" sections,
  update version to 0.0.1a3
- Add CONTRIBUTING.md with dev setup, testing, and PR guidelines
- Add CHANGELOG.md for initial alpha release

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@patrick-chinchill patrick-chinchill merged commit 896fe58 into main Apr 6, 2026
3 checks passed
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the project documentation, including the CHANGELOG, README, and a new CONTRIBUTING guide. Functional changes include the introduction of a client cache in the Slack adapter for improved performance and several security enhancements in the Teams adapter, such as stricter app ID enforcement, SSRF protection for service URLs, and refined JWT verification. Review feedback highlights potential memory growth issues in the Slack client cache and the need to offload synchronous JWT key retrieval to a separate thread to prevent blocking the async event loop.

self._external_channels: set[str] = set()

# Cache of AsyncWebClient instances keyed by bot token
self._client_cache: dict[str, Any] = {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _client_cache dictionary stores AsyncWebClient instances indefinitely. In multi-tenant applications with a large number of workspaces or frequent token rotations, this could lead to significant memory consumption over time. Consider using a cache with an eviction policy (such as an LRU cache) or limiting the maximum number of cached clients to prevent unbounded growth.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in PR #8 (LRU-bounded OrderedDict cache with max=100) and further improved in PR #13 by making client_cache_max configurable via SlackAdapterConfig.

signing_key.key,
algorithms=["RS256"],
audience=self._app_id,
issuer="https://api.botframework.com",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The JWT verification process relies on pyjwt's PyJWKClient.get_signing_key_from_jwt, which is a synchronous method that performs network I/O to fetch signing keys. This blocks the event loop, which can impact performance under high concurrency. Since this PR aims to improve performance, consider offloading this blocking call to a thread using asyncio.to_thread (Python 3.9+) or loop.run_in_executor.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in PR #13 -- get_signing_key_from_jwt is now wrapped in asyncio.to_thread() to avoid blocking the event loop during the synchronous JWKS HTTP fetch.

patrick-chinchill added a commit that referenced this pull request Apr 7, 2026
- Fix Slack rate limit detection: _handle_slack_error now correctly
  handles SlackResponse objects (not just dicts) by checking resp.data,
  and extracts the Retry-After header for rate limit errors
- Fix Teams JWT verification blocking: wrap synchronous
  PyJWKClient.get_signing_key_from_jwt in asyncio.to_thread to avoid
  blocking the event loop during JWKS fetch
- Make Slack client cache max configurable: add client_cache_max field
  to SlackAdapterConfig (defaults to 100)
- Fix unsafe session cleanup on LRU eviction: remove create_task session
  close that could break concurrent requests using evicted clients; rely
  on garbage collection instead
- Add tests for SlackResponse handling, rate limiting, and configurable
  cache max

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
patrick-chinchill added a commit that referenced this pull request Apr 7, 2026
fix: Address unresolved PR review comments from #1, #7, #8
patrick-chinchill pushed a commit that referenced this pull request Apr 18, 2026
…-run

Ran the full 7-principle discipline (docs/SELF_REVIEW.md) via 3
subagents on the novel code in this PR after adding principle #7.
Caught 4 issues the previous rounds hadn't — the discipline is still
paying off.

[P2] Rebind leaks `_recent_messages` + `_message_history`
  (from_json subagent, principle #7)
  The last rebind-cache fix only cleared `_state_adapter_instance`
  and `_channel_cache`. `_recent_messages` (populated from the OLD
  adapter's fetches) and `_message_history` (the OLD chat's cache)
  were missed — both would keep routing to the previous context
  after a rebind. Adding them to the invalidation set on ThreadImpl
  (and `_message_history` on ChannelImpl).

[P2] adapter= and chat= were not orthogonal
  (from_json subagent, principle #3)
  `from_json(t, adapter=A, chat=chat_b)` hit the `if adapter:` branch
  and the `elif chat:` branch was silently skipped — so `_adapter=A`
  was applied but `_state_adapter_instance` was left at None
  (invalidated by the cache clear) and lazy-resolved via the
  singleton on next access, NOT chat_b. Split routing.

  Made the two arguments orthogonal: the `chat` block always applies
  its state, and resolves an adapter via chat ONLY when `adapter`
  wasn't passed. Singleton fallback only fires when neither is
  passed.

[P2] URLs containing `|` / `>` produce malformed `<url|text>` emit
  (Codex review of c381a63 — matches a P3 the emit/parse symmetry
  subagent had flagged earlier)
  `foo:bar|baz` emitted as `<foo:bar|baz|Label>` and parsed back with
  url=`foo:bar`. Added URL-delimiter check to the emit fallback set
  alongside the existing label check.

[P3 — defensive] Orthogonal `adapter`/`chat` pair previously fell
  through an `elif adapter is None and has_chat_singleton() ...`
  singleton-fallback branch that was unreachable before; same branch
  now correctly gated so it only fires when neither arg was passed.

3 new regression tests (one per P2):
- test_should_invalidate_recent_messages_and_message_history_on_rebind
- test_should_set_state_from_chat_when_both_adapter_and_chat_passed
- test_falls_back_to_parenthesized_form_for_urls_with_reserved_delimiters

Deferred from the review (not fixed here):
- Async generator hygiene when placeholder post_message raises
  before the try block — lazy generators don't actually leak.
- Asymmetric try/except on final-content edit vs placeholder-clear
  edit — the two branches handle different content shapes (user
  stream content vs forced whitespace). Would consider if an
  adapter issue arose.
- `_name` round-trip (upstream parity), `_is_subscribed_context`
  rebind (niche), KeyError on malformed dict (P3).

3464 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
patrick-chinchill pushed a commit that referenced this pull request Apr 18, 2026
Two things in one commit since they're both cleanup from the
transactional-rebind fix in 30f6ce5:

- ruff format: 30f6ce5's manual edits to thread.py / channel.py
  introduced formatting drift that CI caught. No semantic change.
- docs/SELF_REVIEW.md §6 (Rebind / idempotent-path state coherence)
  now explicitly covers error paths: "order the steps so any raise
  leaves the object in a well-defined state." Codex found a variant
  of principle #7 that the round-3 subagent's attribute walk had
  missed — the subagent checked WHICH attributes are invalidated
  on rebind, not WHEN they are invalidated relative to raises.
  The §6 note names this subtlety so future reviews catch it.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
patrick-chinchill added a commit that referenced this pull request Apr 23, 2026
* sync: upstream Vercel Chat v4.26.0

Ports four upstream commits (chat@4.25.0..chat@4.26.0):

- fix(chat): guard fallback streaming against empty content (#357)
  Thread._fallback_stream() no longer edits or posts empty content during
  the LLM warm-up or when a chunk buffers to whitespace. With the
  placeholder disabled and an empty stream, posts a single space so the
  stream still returns a SentMessage.

- feat(slack): empty header cells render as space in native table block
  (#373) - Replaces `or " "` truthiness fallback with an explicit length
  check so "0"-like strings are not replaced.

- fix(gchat): render markdown links with <url|text> custom-label syntax
  (#356) - `[Click here](https://example.com)` now renders as
  `<https://example.com|Click here>` instead of `Click here (https://...)`.

- fix(chat): export standalone `reviver` for workflow-safe deserialization
  (#257) - New top-level chat_sdk.reviver lets Vercel Workflow functions
  deserialize Thread/Channel/Message without pulling adapter dependencies
  through the Chat instance. Thread.to_json()/Channel.to_json() now prefer
  the stored _adapter_name so objects revived without a singleton can
  still re-serialize. Python usage: json.loads(payload, object_hook=reviver).

Also:
- UPSTREAM_PARITY = "4.26.0"
- Version bumped to 0.4.26
- README/CLAUDE.md/CHANGELOG.md updated
- Existing fallback-streaming tests updated to reflect the new
  empty-content semantics (no more empty intermediate posts/edits)

* fix(thread): clear fallback placeholder on whitespace-only streams

Upstream 4.26 guards against empty edits in fallback streaming, which
avoids platform APIs rejecting empty bodies but leaves the "..." placeholder
stranded on the message forever when a stream produces no real content.

We diverge by issuing one final edit_message(" ") when the placeholder is
still the latest visible content and the stream produced only whitespace.
This mirrors the behavior of the no-placeholder branch (which already posts
" " for empty streams) and eliminates the SentMessage.text-vs-rendered-
content mismatch that was surfaced in review.

Documented in docs/UPSTREAM_SYNC.md under Known Non-Parity.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: note placeholder-clear divergence in CHANGELOG and ARCHITECTURE

The fallback-streaming placeholder fix was added in f932f26 but only
recorded in UPSTREAM_SYNC.md. Surface it in the two other places
developers look: the CHANGELOG under 0.4.26, and ARCHITECTURE.md near
the fallback streaming description.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(reviver): make from_json idempotent for object_hook use

Codex identified that ``json.loads(..., object_hook=reviver)`` revives
nested dicts bottom-up, so by the time ``ThreadImpl.from_json`` runs,
a payload's ``currentMessage`` is already a ``Message`` instance, not a
dict. The old code then called ``Message.from_json(current_msg_raw)``
on that Message, which raised ``AttributeError`` on ``.get`` and broke
deserialization of any serialized thread containing a currentMessage.

Fix:
- ``Message.from_json`` returns ``data`` unchanged if it's already a Message
- ``ThreadImpl.from_json`` / ``ChannelImpl.from_json`` do the same, plus
  pass the (possibly already-Message) ``currentMessage`` through the
  now-idempotent ``Message.from_json``
- Regression test exercises the exact object_hook bottom-up path with a
  nested currentMessage

Also moved ``TestStandaloneReviver``'s per-method local imports to the
module top level per gemini-code-assist review (PEP 8 E402).

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* ci: add pyrefly type checking and expand lint coverage

Adds a dedicated `Lint & Type Check` workflow modeled on the chinchill-api
pattern:

- Ruff check/format now covers src/, tests/, and scripts/ (previously only
  src/, which let the Gemini PEP 8 finding on tests/test_serialization.py
  slip past CI).
- Pyrefly type check with a baseline file (.pyrefly-baseline.json).
  Baseline captures the 213 existing errors in src/ so we can enable
  pyrefly immediately without a large type-cleanup detour — new errors
  fail CI, pre-existing ones are allowed.
- Audit step moved from the test matrix into the lint job so the
  pytest workflow only runs tests.
- Per-step `continue-on-error: true` with aggregate failure at the end,
  so one failure doesn't hide the others — developers see all issues at
  once.

Optional adapter deps (slack_sdk, nacl, google, redis, asyncpg, httpx,
jwt) are listed in `replace-imports-with-any` so lazy imports don't
flag missing-import when the corresponding extra isn't installed.

Refresh baseline after legitimate fixes with:
  uv run pyrefly check --baseline=.pyrefly-baseline.json --update-baseline

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* ci: fix lint job cache + add least-privilege permissions

Two small CI fixes:

1. The new Lint & Type Check job failed on its first run because
   `astral-sh/setup-uv@v4`'s `enable-cache: true` and the `actions/cache`
   step both key off `uv.lock`, which we gitignore. Dropped both cache
   layers — matches the existing test.yml pattern and removes the
   hashFiles('uv.lock') failure.

2. github-advanced-security flagged the workflows for missing
   GITHUB_TOKEN permissions. Added `permissions: { contents: read }` at
   the workflow level to both lint.yml and test.yml — minimum needed for
   checkout + tests.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* Potential fix for pull request finding 'CodeQL / Workflow does not contain permissions'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: patrick-chinchill <patrick@chinchill.ai>

* fix(gchat,serde): round-trip <url|text> links; sync adapter_name on rebind

Two Codex P1/P2 review findings:

P1 (gchat): commit 4cc87dd in the 4.26 sync switched link rendering to
Google Chat's <url|text> custom-label syntax but didn't teach to_ast()
and extract_plain_text() to parse it back. Messages posted with
[label](url) round-tripped to raw "<url|label>" text with no link node,
breaking downstream handlers that expect structured links or clean
plain text. Added the inverse regex in both methods.

P2 (serde): ThreadImpl.to_json() / ChannelImpl.to_json() preferred
_adapter_name over self.adapter.name to preserve the original name
through standalone-reviver round-trips. But when a caller explicitly
binds a different adapter via from_json(data, adapter=X), the
serialized output kept the stale name from the payload while runtime
calls used X — a later deserialize could resolve the wrong adapter or
fail. Now from_json() sets _adapter_name = adapter.name when an
explicit adapter is bound so serialize and runtime stay in sync.

Regression tests:
- tests/test_gchat_format_extended.py: link parse + plain-text strip
- tests/test_serialization.py: thread adapter_name sync
- tests/test_channel_faithful.py: channel adapter_name sync

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* ci: dedupe permissions block in lint.yml

Copilot Autofix added a permissions block before `concurrency`
without noticing 0cc5d11 had already added one after `concurrency`.
Remove the redundant second block; Copilot's position (before
concurrency) is the more conventional GitHub Actions placement.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* chore: gitignore .claude/ (subagent worktree root)

Same as the entry on the type-cleanup branch — keeps the
parallel-subagent worktree directory out of git.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: mark c4e3bee gchat/serde fixes as divergences from upstream

Both Codex P1/P2 fixes in c4e3bee were introduced without checking
whether upstream handles these cases — turns out upstream has the
same latent bugs in both places:

- packages/adapter-gchat/src/markdown.ts toAst() doesn't parse
  <url|text> back to a link node.
- packages/chat/src/thread.ts fromJSON(json, adapter?) sets
  _adapter but leaves _adapterName at the payload value.

So our fixes are intentional divergences, not ports. Add both to
the Known Non-Parity table and call them out in the CHANGELOG so
we don't re-discover them on the next sync.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat): accept any URL scheme in <url|text> round-trip

Codex P2 caught a gap in c4e3bee: _node_to_gchat emits <url|text>
for every link node regardless of scheme, but the regex I added to
to_ast() / extract_plain_text() only matched http(s)://. So
[Email](mailto:test@example.com) would render as
<mailto:test@example.com|Email> and then round-trip back as raw
text — non-HTTP links still lost their link nodes and plain-text
extraction on the way back.

Widen both regexes to accept any RFC 3986 scheme
([a-zA-Z][a-zA-Z0-9+.-]*:), covering mailto, tel, ftp, etc. Add a
regression that exercises three representative schemes.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat): fall back to parenthesized form when link label contains >/|/newline

Codex P2: Google Chat's <url|text> syntax has no way to escape the `|` or
`>` delimiters inside the label, and a newline would break the single-line
form too. Emitting these characters produces malformed output — both
Google Chat's own renderer and our round-trip regex stop at the first
`>` / `|`, truncating the label.

Fall back to the pre-4.26 `text (url)` form in those cases. The URL is
still auto-detected as a link by Google Chat; the label text is
preserved intact; and the output round-trips cleanly through our
parsers (just without a structured link node for that edge case, which
matches upstream behavior for any non-`<url|text>` link form).

Regression test in test_gchat_format_extended.py covers `a > b`,
`a | b`, and `a\nb` labels.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: record the gchat reserved-char fallback as a Python divergence

Commit dfef313 added a fallback from <url|text> to "text (url)" when
the label contains |, >, or newline — upstream emits the malformed
<url|a > b> form verbatim. Add the divergence to both the
Known Non-Parity table and the 0.4.26 CHANGELOG entry so we don't
rediscover it next sync.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: codify divergence policy + add in-code breadcrumbs

Add a Divergence Policy section to docs/UPSTREAM_SYNC.md with:
- Criteria for when to diverge (data loss / malformed wire / hard UX
  failure; cosmetic issues stay on parity).
- Required workflow (file upstream issue, diverge commit prefix, add
  non-parity row, in-code breadcrumb, regression test, changelog entry).
- Review signal (sync PR title convention, budget of max 2 divergences
  per sync before escalating).

Retroactively add `# Divergence from upstream — see docs/UPSTREAM_SYNC.md`
breadcrumbs at the four divergence sites introduced in this sync:

- src/chat_sdk/thread.py `_adapter_name = adapter.name` in from_json
- src/chat_sdk/channel.py same in ChannelImpl.from_json
- src/chat_sdk/adapters/google_chat/format_converter.py to_ast()
  custom-link regex
- src/chat_sdk/adapters/google_chat/format_converter.py extract_plain_text()
  custom-link regex
- src/chat_sdk/adapters/google_chat/format_converter.py reserved-char
  fallback branch in _node_to_gchat

The placeholder-clear divergence in thread._fallback_stream already had
a full multi-line explanation; leaving that alone.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* address coderabbit review findings

- [critical] thread._fallback_stream: wrap the placeholder-clear
  edit_message(" ") in try/except so Telegram (which raises
  ValidationError on whitespace-only text) logs and falls back to
  upstream's stranded-placeholder behavior instead of propagating
  a crash up through thread.post().
- [major] pyrefly python-version 3.11 -> 3.10 to match
  requires-python. Otherwise type-check target drifts ahead of the
  package's claimed support floor.
- [major] test_chat_faithful.py: the fallback-streaming test now
  verifies the final posted markdown is "Hi", not just that some
  post happened on the right thread.
- [minor] test_thread_faithful.py: three "no empty content" tests
  were vacuous — they iterated PostableMarkdown calls inside an
  isinstance guard but never asserted that any calls happened, so
  the tests passed even if the SDK emitted nothing. Now assert at
  least one matching call before checking content.
- [nit] channel.py / thread.py from_json: replace `or` truthiness
  chain on camel-to-snake fallback keys with explicit None checks.
  `""` is a valid-but-falsy value that shouldn't silently fall
  through to the alias.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* tighten to_json adapter_name + exercise warm-up path in test

CodeRabbit follow-up on 8dd34d1:

- to_json(): switch adapter_name fallback from truthy to explicit
  `is not None`. This matches upstream's `??` semantics (preserve
  stored "" instead of rewriting it) AND avoids triggering a lazy
  `self.adapter.name` resolution when `_adapter_name` was set to ""
  but no `_adapter` is bound — that combination would raise
  RuntimeError from the adapter property.
- test_should_not_edit_placeholder_to_empty_during_llm_warmup:
  the original stream `["Hello world"]` never actually exercised
  the empty-edit guard path — by the time the edit loop fired the
  buffer already had real content. Now the stream yields a bare
  whitespace chunk, pauses long enough for the (10 ms) edit loop
  to fire, then yields the real content. A regression that emits
  empty intermediate edits would now fail here.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* address remaining codex + coderabbit findings

Three more findings from the latest review passes:

- [Codex P2] gchat <url|text> fallback now also triggers on `]` in the
  label, not just `|`/`>`/newline. Our to_ast() regex converts
  `<url|text>` back to Markdown `[text](url)` form, which breaks if
  `text` contains `]` because the parser closes the link at the first
  `]`. Added `]` to the unsafe-char set alongside the others; updated
  the regression test to cover it and to exercise the emit path via a
  hand-built AST (from_markdown can't construct labels containing `]`
  because Markdown itself splits on it).
- [CodeRabbit Major] ThreadImpl.from_json / ChannelImpl.from_json: the
  `isinstance(data, ThreadImpl): return data` / `isinstance(data,
  ChannelImpl): return data` shortcut bypassed the adapter/chat
  rebinding block, so `from_json(thread_instance, adapter=X)` silently
  left _adapter stale. Changed both to reuse the existing instance but
  still flow into the rebinding logic. Added a regression test that
  rebinds an already-revived ThreadImpl to a different adapter and
  asserts to_json reflects the new name.
- [CodeRabbit nit] tightened the whitespace-chunks-no-placeholder test
  to assert `markdown == " "` exactly (not just non-empty), so a
  regression that posts the raw buffer `"   \n"` would fail.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: add `]` to the gchat link-label non-parity row

1ce9cbf extended the reserved-char fallback to include `]`; keep the
UPSTREAM_SYNC.md table accurate for the next sync.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(types): rename format converter params to platform_text + pin pyrefly

Pyrefly 0.61.1 tightened bad-override-param-name detection and surfaced
7 real `to_ast` / `extract_plain_text` overrides whose parameter names
diverged from the BaseFormatConverter signature (`platform_text`):

- github/format_converter.py       `markdown`    -> `platform_text`
- google_chat/format_converter.py  `gchat_text`  -> `platform_text`
- google_chat/format_converter.py  `text`        -> `platform_text`
- slack/format_converter.py        `mrkdwn`      -> `platform_text` (x2)
- telegram/format_converter.py     `text`        -> `platform_text`
- whatsapp/format_converter.py     `markdown`    -> `platform_text`

Some subagents during the type-cleanup sweep renamed these; others
didn't, which is why 0.61.0 passed locally (baseline captured the
old errors at that version) but 0.61.1 in CI detected them as new.

Also pin pyrefly to `==0.61.1` — any future patch bump that tightens
detection further would flake CI against the frozen baseline. We can
move the pin forward deliberately when refreshing the baseline.

No runtime behavior change (parameter name is internal).

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(channel): use `is not None` for adapter_name serialization + document URL paren limitation

Two follow-ups to the latest Codex review:

- [P2] ChannelImpl.to_json: the same `or` truthiness trap I fixed in
  ThreadImpl.to_json in 8874aab was still present in channel.py.
  `self._adapter_name if self._adapter_name else self.adapter.name`
  triggers lazy adapter resolution when _adapter_name is "", which
  breaks the workflow-safe reserialization path added in earlier
  commits. Aligned with thread.py using explicit `is not None`.

- [P1 → Known limitation] Google Chat `<url|text>` decoding corrupts
  URLs containing unescaped `)` (e.g. Wikipedia-style links). A proper
  fix requires AST-level placeholder substitution because our Markdown
  parser doesn't implement CommonMark's balanced-parens rule for link
  destinations. Documented as a known limitation on the non-parity row
  and at the regex site in format_converter.py. Upstream doesn't
  round-trip at all, so our behavior is still strictly better for the
  common case (URLs without parens).

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* tighten two more CodeRabbit tests; hold one

Two of three latest CodeRabbit findings landed; one rejected as
incorrect.

Landed:
- tests/test_serialization.py: the channel re-serialize-without-
  singleton test now routes the payload through
  `json.loads(..., object_hook=reviver)` with an
  `isinstance(channel, ChannelImpl)` assertion. Before, only
  `ChannelImpl.from_json()` was exercised, so a regression in
  `chat_sdk.reviver`'s `chat:Channel` dispatch would not fail the
  test.
- tests/test_thread_faithful.py (no-placeholder "Hi"): strengthened
  to assert the exact call sequence — exactly one post_message("Hi")
  and zero edits. Previously only the last post was pinned, so a
  regression splitting the flow into an empty post + later edit
  would still pass.

Rejected:
- tests/test_thread_faithful.py "should_log_when_an_intermediate_edit_
  fails": CodeRabbit claimed the code logs "fallbackStream _edit_loop
  failed" from the background path, but the actual string at
  src/chat_sdk/thread.py:635 is "fallbackStream edit failed". Test
  matches code and passes; no change.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat): AST placeholder substitution for <url|text> round-trip

The regex-based `<url|text>` → `[text](url)` conversion in to_ast
couldn't handle URLs containing `)` (e.g. Wikipedia-style
`https://en.wikipedia.org/wiki/Foo_(bar)`) because our Markdown parser
doesn't implement CommonMark's balanced-parens rule for link
destinations. Codex flagged this as P1 and my last round deferred
it as a "known limitation".

Compact correct fix: bypass Markdown for the custom-link tokens.

1. Extract each `<scheme://url|text>` match to a private-use
   placeholder `\ue000LINK{idx}\ue000` and remember `(url, text)`.
2. Run the usual bold / strikethrough conversions + parse_markdown
   on the placeholder-bearing text.
3. Walk the resulting AST, find text nodes containing placeholders,
   and inject real link nodes in their place.

PUA code points (U+E000) are guaranteed to not appear in user-authored
text, so sentinel collisions aren't a concern.

- Wiki-style URLs with parens now round-trip intact
- Multiple <url|text> tokens in a single paragraph split cleanly
- mailto: / tel: / other schemes still work (regression test intact)
- Removed the "known limitation" row from UPSTREAM_SYNC.md; this path
  is now strictly better than upstream, which either leaves
  <url|text> as raw text or parses the whole thing as a malformed
  autolink URL.

No change to extract_plain_text — it was already correct (its regex
stops at `|`/`>`, not `)`).

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat): preserve <url|text> inside code spans, guard out-of-range placeholders

Two Codex findings on the AST-placeholder fix in e08b85e:

- [P1] `<url|text>` inside inline / fenced code was also extracted to
  placeholders during the pre-parse pass, but `_inject_link_placeholders`
  only visited `text` nodes. Code spans ended up with raw `\ue000LINK0\ue000`
  in their `value`, silently corrupting snippets that legitimately contain
  Google Chat link syntax (e.g. a code example). Now the walker also
  visits `inlineCode` and `code` nodes and rewrites any placeholders back
  to their original `<url|text>` literal — code spans stay code, not
  links.

- [P2] `links[int(part)]` had no bounds check, so crafted or accidental
  user input containing a stray `\ue000LINK999\ue000` token would raise
  `IndexError` and fail message parsing entirely. Now an unresolvable
  index is preserved as literal text — same defensive posture as the
  code-span case.

Added two regression tests:

- test_gchat_custom_link_syntax_inside_code_span_stays_literal covers
  both inline and fenced code.
- test_gchat_custom_link_tolerates_out_of_range_placeholder crafts a
  bogus placeholder alongside a real <url|text> and asserts the real
  one still parses (and we don't raise).

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* test: close the gaps the self-review surfaced

Four targeted regression tests, each justified by what it would catch:

1. test_should_swallow_placeholder_clear_error_on_strict_adapter
   (test_thread_faithful.py)
   Closes the meaningful gap: the try/except wrap added in 8dd34d1
   for adapters like Telegram that reject whitespace-only edit_message
   was untested. Mock adapter now raises ValueError when the
   placeholder-clear " " edit comes through; the test asserts (a)
   thread.post() returns a SentMessage instead of propagating, (b)
   exactly one clear attempt was made (no retry loop), and (c) the
   warn log fires with the exact expected message.

   Mutation-tested: narrowing the `except Exception` to
   `except RuntimeError` makes this test fail, confirming the swallow
   path is actually exercised.

2. test_should_rebind_adapter_when_data_is_already_a_channelimpl
   (test_channel_faithful.py)
   Symmetric with the ThreadImpl rebind regression in
   test_serialization.py. ChannelImpl.from_json had the same
   early-return-skips-rebinding bug fix in 1ce9cbf, but no test was
   added for the channel path.

3. test_strips_gchat_custom_link_with_balanced_parens_in_url
   (test_gchat_format_extended.py)
   Pins the plain-text-extraction behavior for URLs containing `)`.
   Worked already because the strip regex stops at `|`/`>`, but a
   future regex tweak could regress this without failing any other
   test.

4. test_gchat_multiple_custom_links_in_one_paragraph
   (test_gchat_format_extended.py)
   Asserts the exact (text, link, text, link, text, link, text)
   sequence for a paragraph with three `<url|text>` tokens. Catches
   off-by-one errors in the placeholder-substitution split logic
   that would otherwise pass single-link tests but produce wrong
   ordering or dropped fragments.

3450 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat): 3 round-trip corrections from Codex P2s

1. Placeholder collision (to_ast): user text literally containing the
   old fixed-format placeholder `\ue000LINK0\ue000` alongside any real
   <url|text> would rewrite the user's literal as a duplicate link
   node. Per-call random nonce (`secrets.token_hex(4)`) now makes
   placeholder tokens unforgeable — only the tokens emitted by this
   to_ast call will match its own injection regex.

2. Code-literal corruption (extract_plain_text): the new
   <url|text> strip ran after backtick-stripping, so
   `<https://example.com|Example>` in inline/fenced code came back as
   just "Example". Fix: stash code-span contents behind nonce'd
   placeholders first, run the other strips on the surrounding text,
   then restore code contents literally at the end. Mirrors the
   approach already used in to_ast.

3. Relative URLs (_node_to_gchat): emitted <url|text> for every link
   regardless of whether the URL had a scheme, but the reverse
   parsers only match `scheme:` prefixed URLs. `[doc](/docs)` went
   out as `</docs|doc>` and came back as raw text. Fix: fall back to
   `text (url)` form when the URL has no scheme (joins the existing
   fallback set alongside `|`/`>`/`]`/newline in labels).

3 new regression tests:
- test_gchat_custom_link_tolerates_in_range_forged_placeholder
- test_preserves_gchat_custom_link_literal_inside_{inline,fenced}_code
- test_falls_back_to_parenthesized_form_for_urls_without_a_scheme

Total: 3454 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: add Self-Review Discipline section to CLAUDE.md

Honest retro from this PR: review bots (Codex, CodeRabbit) surfaced
~13 valid findings; my own self-reviews surfaced ~3–4. Bots aren't
magic — they run adversarial checks consistently. The agent
guidelines didn't tell future sessions to do the same.

Adds a new "Self-Review Discipline" section between Principles and
Port Rules, covering the six specific blindspots this PR kept hitting:

1. Adversarial input sweep — what inputs would break this regex/
   substitution/tokenizer?
2. Emit/parse symmetry — does my parse accept every shape my emit
   produces?
3. Pass-interaction check — does my new pass compose cleanly with
   every existing pass in the pipeline?
4. Unforgeable sentinels — per-call nonce for any placeholder mapped
   back to structured data.
5. Divergence budget — enforce the 2-divergence-per-sync cap against
   myself, not just externally.
6. The Codex pre-ship question — "what would Codex find right now?"

Also cross-links from UPSTREAM_SYNC.md "How to land a divergence"
since divergence code is the highest-risk surface for these
blindspots.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: split Self-Review Discipline into dedicated docs/SELF_REVIEW.md

The 6-point discipline was too long to inline in CLAUDE.md and was
going to grow. Extracted to its own file following the same pattern
CLAUDE.md already uses for UPSTREAM_SYNC.md (compact pointer in the
root agent doc, full content in docs/).

Also reworked the content to be principle-based rather than tied to
specific findings from the 4.26 sync PR. Each principle describes a
*class* of bug; specific examples belong in commit messages and PR
descriptions, not in the ongoing guidelines doc.

UPSTREAM_SYNC.md's "How to land a divergence" checklist now points
at the new location.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(gchat, thread): 3 bugs found by self-review subagents

Three subagents applied docs/SELF_REVIEW.md's six principles to this
PR's novel code. Between them they found three valid bugs that the
review bots hadn't surfaced. The discipline works — writing it down
and running it deliberately pays off.

[P2] to_ast bold/strike pre-pass corrupts code-span content
  (pass-interaction subagent)
  Input `` `*foo*` `` came back as inlineCode with value `**foo**`:
  the bold pre-parse substitution rewrote `*foo*` → `**foo**` on the
  raw text, and parse_markdown then wrapped the doubled markers
  verbatim in the code node. Same class as the Codex finding on
  extract_plain_text, just in a different function.

  Fix: stash code-span contents behind nonce'd placeholders BEFORE
  the bold/strike passes, restore in the walker. The inline regex
  also needed (?<!`)...(?!`) guards so it wouldn't match backticks
  inside a fenced block that's already been placeholder'd.

[P2] _fallback_stream exception leaves placeholder stranded
  (adversarial-input subagent)
  If the stream raised mid-iteration (e.g. LLM connection drop),
  the `try: ... finally: stop_event.set()` block propagated the
  error immediately — skipping `await pending_edit`, the final
  flush, and the placeholder-clear. User saw "..." forever despite
  partial content being rendered, AND pending_edit leaked as an
  orphan task.

  Fix: capture the stream error, run the full flush+cleanup path,
  then re-raise. Caller still sees the original exception; the
  placeholder and orphan task issues are resolved.

[P1] Empty link label emits `<url|>` which parse rejects
  (emit/parse-symmetry subagent)
  An AST link node with empty children produced `<https://x.com|>`,
  which our parse regex requires ≥1 char in the label — so the
  output was raw angle brackets visible in the rendered message.
  Fix: emit the bare URL when the label is empty. Google Chat
  auto-links it and nothing informational is lost.

3 new regression tests, each mutation-testable:
- test_to_ast_preserves_bold_and_strike_markers_inside_code_spans
- test_should_flush_and_cleanup_when_stream_raises_midway
- test_emits_bare_url_when_link_label_is_empty

Total: 3457 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(thread): narrow except in _fallback_stream from BaseException to Exception

github-code-quality caught that the stream-error capture in f4c6253
used `except BaseException`, which would swallow CancelledError,
KeyboardInterrupt, SystemExit, and GeneratorExit along with the
normal runtime errors we want to flush around.

Narrowed to `except Exception`. Control-flow exceptions now propagate
immediately; the existing `finally: stop_event.set()` still signals
the background edit loop to exit cleanly on cancellation, so there's
no task leak on shutdown either.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(serde): invalidate state/channel caches on idempotent adapter rebind

Codex flagged a real P1 I introduced in the idempotent `from_json(
existing_instance, adapter=X)` path: I updated `_adapter` and
`_adapter_name` but left previously-populated caches alone. If the
instance had been previously bound (state adapter cached, channel
derived), the rebind would:

- leave `_state_adapter_instance` pointing at the OLD chat's state
  backend, so `get_state` / `set_state` would silently route to
  the wrong backend;
- leave `_channel_cache` referencing a channel built from the OLD
  adapter, so `thread.channel` operations would target the
  previous context.

Both ThreadImpl and ChannelImpl were affected.

Fix: when the `isinstance(data, ...)` idempotent branch fires AND we
have an explicit `adapter=` or `chat=`, drop the relevant caches
before the rebind block runs. The existing fresh-construction path
sets `_state_adapter_instance = None` and defers to lazy singleton
resolve, so clearing the cache here restores parity.

2 new regression tests:
- test_should_invalidate_state_and_channel_caches_on_idempotent_rebind
  (thread — populates channel cache via `.channel` access, state
  cache via explicit binding, asserts both drop after rebind)
- test_should_invalidate_state_cache_on_idempotent_rebind
  (channel — populates state cache, asserts it drops)

3459 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(serde): resolve adapter via _adapter.name on idempotent chat rebind

Codex found another variant of the cache/state rebinding class of
bugs (the chat= path this time). If an existing ThreadImpl or
ChannelImpl was constructed directly by a Chat instance, its
`_adapter_name` is None — only `_adapter` is set. The idempotent
chat-rebind path keyed the new-adapter lookup off `_adapter_name`:

    if thread._adapter_name:
        resolved = chat.get_adapter(thread._adapter_name)
        ...
    thread._state_adapter_instance = chat.get_state()

With `_adapter_name == None`, the lookup was skipped but state was
still reassigned to the new chat's backend. Result: post()/edit()
continued routing through the OLD adapter while get_state()/set_state()
hit the NEW chat — messages and state writes landing in different
Chat contexts (and potentially different platforms or environments).

Fix: in both ThreadImpl.from_json and ChannelImpl.from_json, fall
back to the currently-bound adapter's `.name` when `_adapter_name`
is empty, and record the name back onto the instance so future
serialization/rebinds see it.

2 new regression tests exercise the direct-construction + chat-
rebind path end-to-end (build two Chat instances each with a
different `slack` adapter, construct a thread/channel through
chat_a, rebind via from_json(chat=chat_b), assert both adapter AND
state now point at chat_b's objects).

3461 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs: add 7th principle — rebind/idempotent-path state coherence

Three Codex findings in a row landed on the same pattern: an idempotent
factory path (from_json with an existing instance) that updated one
binding but left stale caches or lookup keys from the previous binding.
The symptom is always "split routing" — messages go to the old context,
state to the new.

This wasn't covered by any of the six existing principles (input sweeps,
emit/parse symmetry, pass-interaction, sentinels, divergence budget,
pre-ship). Pass-interaction comes closest but is about *pipeline*
ordering, not *instance attribute* coherence across rebinds. Adding
it as a distinct check means future idempotent APIs get a dedicated
prompt: "walk every attribute and ask whether it came from the old
binding."

Also updates the CLAUDE.md summary to list the new check.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(serde, gchat): address 4 findings from 7-principle self-review re-run

Ran the full 7-principle discipline (docs/SELF_REVIEW.md) via 3
subagents on the novel code in this PR after adding principle #7.
Caught 4 issues the previous rounds hadn't — the discipline is still
paying off.

[P2] Rebind leaks `_recent_messages` + `_message_history`
  (from_json subagent, principle #7)
  The last rebind-cache fix only cleared `_state_adapter_instance`
  and `_channel_cache`. `_recent_messages` (populated from the OLD
  adapter's fetches) and `_message_history` (the OLD chat's cache)
  were missed — both would keep routing to the previous context
  after a rebind. Adding them to the invalidation set on ThreadImpl
  (and `_message_history` on ChannelImpl).

[P2] adapter= and chat= were not orthogonal
  (from_json subagent, principle #3)
  `from_json(t, adapter=A, chat=chat_b)` hit the `if adapter:` branch
  and the `elif chat:` branch was silently skipped — so `_adapter=A`
  was applied but `_state_adapter_instance` was left at None
  (invalidated by the cache clear) and lazy-resolved via the
  singleton on next access, NOT chat_b. Split routing.

  Made the two arguments orthogonal: the `chat` block always applies
  its state, and resolves an adapter via chat ONLY when `adapter`
  wasn't passed. Singleton fallback only fires when neither is
  passed.

[P2] URLs containing `|` / `>` produce malformed `<url|text>` emit
  (Codex review of c381a63 — matches a P3 the emit/parse symmetry
  subagent had flagged earlier)
  `foo:bar|baz` emitted as `<foo:bar|baz|Label>` and parsed back with
  url=`foo:bar`. Added URL-delimiter check to the emit fallback set
  alongside the existing label check.

[P3 — defensive] Orthogonal `adapter`/`chat` pair previously fell
  through an `elif adapter is None and has_chat_singleton() ...`
  singleton-fallback branch that was unreachable before; same branch
  now correctly gated so it only fires when neither arg was passed.

3 new regression tests (one per P2):
- test_should_invalidate_recent_messages_and_message_history_on_rebind
- test_should_set_state_from_chat_when_both_adapter_and_chat_passed
- test_falls_back_to_parenthesized_form_for_urls_with_reserved_delimiters

Deferred from the review (not fixed here):
- Async generator hygiene when placeholder post_message raises
  before the try block — lazy generators don't actually leak.
- Asymmetric try/except on final-content edit vs placeholder-clear
  edit — the two branches handle different content shapes (user
  stream content vs forced whitespace). Would consider if an
  adapter issue arose.
- `_name` round-trip (upstream parity), `_is_subscribed_context`
  rebind (niche), KeyError on malformed dict (P3).

3464 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix+docs: 7-principle re-review + upstream drift audit round 2

Ran the 7-principle discipline AGAIN with explicit upstream-TS drift
checks. Three subagents, each scoped to one file, returned 9 findings
between them. All addressed.

[P1] thread._fallback_stream used `get_committable_text()` instead
  of `render()` in the edit loop and first-post path.
  `get_committable_text` is for append-only native-streaming surfaces
  (Slack native) — it holds back unclosed lines and wraps tables in
  code fences. The fallback path uses full-message replacement edits,
  so the correct method is `render()` (remend'd markdown, no append-
  only buffering). Upstream packages/chat/src/thread.ts:697 uses
  `render()`; we diverged silently.

  Two existing tests needed updating (the test for `["H", "i"]` with
  placeholder disabled previously asserted "one post of 'Hi', no
  edits" — a byproduct of the get_committable_text holdback. The
  upstream-correct behavior is "post 'H' immediately, edit to 'Hi'".)

[P3] `_is_subscribed_context` not reset on idempotent rebind.
  A thread constructed inside a subscribed-context handler carries
  the flag; on rebind to a new adapter/chat, the new context has no
  active subscription, but `is_subscribed()` would still short-
  circuit to True. Added to the cache-invalidation list alongside
  the other stale-context attributes.

[Docs] Five divergence rows added / extended in UPSTREAM_SYNC.md
  non-parity table (with matching code-site breadcrumbs):
  - Heading rendering as `*text*` (upstream plain-text)
  - Image rendering as `{alt} ({url})` (upstream drops URL)
  - Fallback streaming stream-exception capture + flush
  - Fallback streaming final `final_content` (remend'd) vs
    upstream `accumulated` (raw)
  - Extended existing `<url|text>` label-fallback row to cover
    empty labels, schemeless URLs, and URLs containing `|` / `>`
    (all previously fixed but described in separate rows)

Kept from round-2 reports as "debatable, defer":
  - Symmetry between to_ast (guards adjacent markers) and
    extract_plain_text (doesn't) — stripping is a best-effort
    fallback, not a round-trip contract.
  - Stand-alone reviver signature adaptation is already documented
    in reviver.py docstring.

1 new regression test for `_is_subscribed_context` rebind clearing.
2 existing tests updated to match upstream-correct render() behavior.

3465 tests pass, 0 pyrefly errors, ruff clean.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(thread): SentMessage reflects actual platform content, not final_content

Codex P2 on 2e14133: `_create_sent_message` was always invoked with
`final_content` (from `renderer.finish()`) regardless of what
actually ended up on the platform. Most visible in two cases:

1. Strict adapter (Telegram) rejects the placeholder-clear " " edit
   → platform still shows "..." → sent.text was "" (mismatch).
2. No-placeholder path posts `accumulated if accumulated.strip()
   else " "` → platform shows " " for whitespace streams → but
   `last_edit_content = accumulated` (possibly empty) didn't match
   the posted value either.

Fix: track actual platform state via `last_edit_content` at every
post/edit point, and thread that into the SentMessage instead of
`final_content`. Concretely:

- In the `msg is None` branch, set `last_edit_content = markdown`
  (the exact value posted), not `accumulated`.
- After a successful final edit, set `last_edit_content = final_content`.
- Use `last_edit_content` (not `final_content`) when constructing
  the SentMessage.

Regression test pins the strict-adapter case — stranded placeholder
with sent.text matching "...". The whitespace-only happy path still
produces `sent.text == ""` because the markdown parser normalizes
a lone space to empty text (unchanged from before; fixing that
would require special-casing the parser and is a separate concern).

3466 tests pass, 0 pyrefly errors.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(serde): make idempotent rebind transactional on adapter-lookup failure

Codex P2 (both ThreadImpl and ChannelImpl): `from_json(existing_inst,
chat=Y)` was invalidating caches BEFORE calling `chat.get_adapter(name)`.
If the lookup failed and raised `RuntimeError`, the caller catching the
exception was left with a partially-mutated instance — `_state_adapter_
instance`, `_channel_cache`, `_message_history`, `_recent_messages`,
`_is_subscribed_context` all silently wiped, but `_adapter` unchanged.
Subsequent operations on the thread would re-resolve state/channel via
the singleton instead of the previously-bound chat.

Fix: pre-flight the adapter lookup BEFORE invalidating caches. If the
name doesn't resolve in the new chat, raise immediately with all
original state intact. The binding block below repeats the lookup
when it actually applies the result; that's a cheap duplicate in
exchange for transactional semantics.

Same fix in both `ThreadImpl.from_json` and `ChannelImpl.from_json`.

2 regression tests, each captures a snapshot of every cache-bearing
attribute before the rebind, calls `from_json(instance, chat=bad_chat)`
expecting RuntimeError, then asserts every snapshotted attribute is
byte-identical on the catch side:

- test_should_leave_thread_unchanged_when_chat_rebind_lookup_fails
- test_should_leave_channel_unchanged_when_chat_rebind_lookup_fails

3468 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* chore: ruff format + extend SELF_REVIEW §6 with error-path note

Two things in one commit since they're both cleanup from the
transactional-rebind fix in 30f6ce5:

- ruff format: 30f6ce5's manual edits to thread.py / channel.py
  introduced formatting drift that CI caught. No semantic change.
- docs/SELF_REVIEW.md §6 (Rebind / idempotent-path state coherence)
  now explicitly covers error paths: "order the steps so any raise
  leaves the object in a well-defined state." Codex found a variant
  of principle #7 that the round-3 subagent's attribute walk had
  missed — the subagent checked WHICH attributes are invalidated
  on rebind, not WHEN they are invalidated relative to raises.
  The §6 note names this subtlety so future reviews catch it.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* fix(thread): wrap final-edit in try/except to avoid orphaning placeholder

Round-4 subagent P2: the sibling placeholder-clear edit was wrapped
defensively in try/except (to handle Telegram-style whitespace
rejection), but the primary final-content edit wasn't. If the final
edit raises (rate-limit, transient 5xx, content-policy reject), the
exception propagates out of `_fallback_stream` and the caller never
gets a SentMessage — even though the placeholder message is already
sitting on the platform. The caller catching the exception has no
handle to edit/delete it, and the message is orphaned.

Symmetric fix with the placeholder-clear branch: try the edit, log
on failure, leave `last_edit_content` alone. The SentMessage returned
to the caller reflects whatever was last successfully on the platform
(via `last_edit_content`) and carries a valid message ID the caller
can still act on.

Regression test injects an always-failing `edit_message`, asserts
(a) thread.post() doesn't raise, (b) sent.text matches the
placeholder "..." that's actually on the platform, (c) the expected
warn log fires.

3469 tests pass.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

* docs(changelog): backfill 4 Python-specific divergences from non-parity table

Round-5 subagent found the one remaining gap: `docs/SELF_REVIEW.md` §5
requires a CHANGELOG entry for every divergence, and the non-parity
table has 8 Python-specific rows while the CHANGELOG had only 4
bullets. Backfill the missing four and broaden one existing bullet:

- Extended the `<url|text>` reserved-delimiters row to cover `]`,
  newlines, empty labels, schemeless URLs, and URLs containing
  `|`/`>` — all handled by the same emit-side fallback check, all
  accumulated over several commits during review iteration but only
  partially reflected in the CHANGELOG.
- Added dedicated bullets for Google Chat heading → bold rendering
  (upstream loses visual hierarchy by falling through to plain text).
- Added dedicated bullet for Google Chat image rendering (upstream
  silently drops the URL).
- Added bullet for fallback streaming's stream-exception capture +
  flush before re-raise (upstream propagates immediately, orphaning
  pendingEdit and stranding the placeholder).
- Added bullet for fallback streaming's `remend`'d final SentMessage
  content vs upstream's raw `accumulated`.

No code changes. Round-5 verdict on code: clean across all 7
principles. Only doc bookkeeping was missing.

https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG

---------

Signed-off-by: patrick-chinchill <patrick@chinchill.ai>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
patrick-chinchill pushed a commit that referenced this pull request May 9, 2026
Hardens the socket-mode port against the seven review findings on
PR #86:

* Add ``connect_timeout_s`` config (default 30s) and wrap the initial
  socket-mode handshake in ``asyncio.wait_for`` so a hung
  ``SocketModeClient.connect()`` can't make ``initialize()`` block
  forever (hazard #11). On timeout we tear the loop down before raising.
* Reject forwarded socket events whose ``timestamp`` field is outside
  the same 5-minute window ``_verify_signature`` enforces, so a captured
  forwarded payload can't be replayed indefinitely (hazard #12).
* Narrow the ``contextlib.suppress`` in ``stop_socket_mode`` to
  ``CancelledError`` only — surprising loop crashes are no longer
  silently swallowed during shutdown.
* Replace ``asyncio.get_event_loop().create_task`` in the socket-mode
  ``wrap_async`` helper with ``get_running_loop`` (Python 3.12+
  compatibility, hazard #5).
* Have the socket-mode interactive branch ack with
  ``response_action: errors`` instead of an empty ack when dispatch
  raises — an empty ack on ``view_submission`` silently closes the
  modal so the user sees no signal anything went wrong.
* Drop ``is_ext_shared_channel`` from the synthesized ``event_callback``
  payload in the socket-mode events_api branch so socket and webhook
  paths feed identical shapes into ``_process_event_payload`` (hazard
  #7).
* Add a regression test firing two concurrent ``_route_socket_event``
  events_api dispatches for different teams via ``asyncio.gather`` to
  pin down the existing ``copy_context()`` isolation against future
  drift (hazard #6).

Also fixes a pre-existing B010 lint warning in the test stub.

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
patrick-chinchill added a commit that referenced this pull request May 11, 2026
, #397) (#84)

* feat(externalselect): add initialOption + option_groups (vercel/chat#410, #397)

Ports two upstream PRs that together complete ExternalSelect support:

- vercel/chat#397 introduced ExternalSelectElement and the
  block_suggestion / onOptionsLoad runtime; the runtime half landed
  here in #66 but the modal element type was deferred. This PR adds
  the missing ExternalSelectElement TypedDict + ExternalSelect builder
  and wires up _external_select_to_block in the Slack modal renderer.
- vercel/chat#410 adds two new optional fields on top: initialOption
  (pre-selected option object) and option_groups (labeled sections,
  Slack max 100 groups x 100 options, label max 75 chars). The
  handler return type widens to OptionsLoadResult = list[options] |
  list[OptionsLoadGroup]; the Slack adapter detects grouped form by
  the presence of an "options" key on the first entry and emits
  Slack's option_groups response (mutually exclusive with options
  per Slack's spec).

Hazard #1 (truthiness): min_query_length=0 is preserved (0 means
"fire on every keystroke"); not silently dropped by an `or` fallback.

Hazard #7 (omit vs None): unset initial_option / placeholder /
min_query_length are omitted from the rendered Block Kit element,
not serialized as null.

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

* fix(slack): use is-not-None guard for initial_option in external_select renderer

Address review on PR #84 (modals.py:209). The TS expression
``if (select.initialOption)`` only filters null/undefined since ``{}`` is
truthy in JS. Python ``if initial_option:`` falsely drops a
hand-constructed ``initial_option={}`` because empty dicts are falsy.
Switch to ``is not None`` for parity with TS and consistency with the
``min_query_length is not None`` check three lines above.

Adds test_external_select_initial_option_empty_dict_renders regression
test that fails before the fix.

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

---------

Co-authored-by: Claude <noreply@anthropic.com>
patrick-chinchill pushed a commit that referenced this pull request May 22, 2026
Independent review surfaced three actionable items:

1. **Math/escape ordering** (review #1) -- `_strip_math_regions` ran
   BEFORE `_strip_escape_sequences`, so input like
   `\$opener *unclosed text closer\$` had its two escaped `$`s paired
   as math, eating the `*` opener inside. Reordered: escape strip
   first, then math strip. Verified the original bug repro now
   correctly produces a closing `*`.

2. **Empty task items** (review #5) -- `r"^\[([ xX])\]\s+(.*)"`
   required at least one whitespace AFTER `]`, so `- [ ]` with no
   trailing content silently fell through to plain text. Loosened to
   `r"^\[([ xX])\](?:\s+(.*))?$"` -- trailing whitespace+content is
   now optional. `- [ ]` and `- [x]` (no trailing) both produce
   `listItem(checked=False/True, children=[])`.

3. **Strikethrough test strengthened** (review #7) -- the existing
   `test_escaped_tilde_does_not_form_strikethrough` only asserted the
   absence of a `delete` node. A buggy impl that dropped the tildes
   entirely would have passed. Added a content-shape assertion that
   `~~not strike~~` appears in the text leaves.

Reviewer's #3 (the `r"a *b\* * c"` case) was investigated and
determined NOT to be a bug -- the trailing `*` between two spaces is
not a valid CommonMark closer (whitespace-flanked), so italic
correctly stays open. Pre-fix behavior happened to balance by ignoring
flanking; post-fix is CommonMark-correct. Pre-existing edge cases #4
(`\\*text*`) and #6 (link-text unescape implicit) are documented in
the existing code; not blockers.

Tests added:
- `test_empty_unchecked_task_item_no_content` and `_checked_` variant
- `test_remend_escaped_dollar_does_not_pair_with_unescaped_dollar`
  (the exact #1 repro)
- `test_remend_escaped_dollar_does_not_create_phantom_math_region`
- Strengthened `test_escaped_tilde_does_not_form_strikethrough`

3,625 pass / 1 pre-existing failure unrelated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant