sync: upstream Vercel Chat v4.26.0#43
Merged
Merged
Conversation
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)
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
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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
chat-sdk-python/src/chat_sdk/thread.py
Line 762 in 0ff1703
Handle pre-revived currentMessage in Thread.from_json
With the new standalone reviver used as json.loads(..., object_hook=reviver), nested dicts are revived before their parents, so a thread payload containing currentMessage will arrive at ThreadImpl.from_json with currentMessage already as a Message instance. This code unconditionally calls Message.from_json(current_msg_raw), which raises AttributeError (Message has no .get) and breaks deserialization for valid serialized threads that include currentMessage.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the SDK to version 0.4.26, syncing with Vercel Chat 4.26.0. Key additions include a standalone reviver for deserialization without a Chat instance and workflow-safe to_json methods. It also improves fallback streaming by guarding against empty content and fixes rendering issues for Slack tables and Google Chat links. Feedback includes correcting a typo in the CHANGELOG release date and moving local imports in tests to the module level to comply with PEP 8.
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
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
…ntain 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>
Collaborator
Author
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
🤖 Codex Code Review
Summary
Re-reviewed claude/port-upstream-4.26-b5zzY at 6070884. The earlier blockers are fixed: whitespace-only fallback streams now clear the placeholder correctly, and the standalone reviver() path now safely handles nested currentMessage values revived via json.loads(..., object_hook=reviver).
Findings
No issues found after review.
Verification Performed
- Ran
uv run ruff check src tests- result: passed
- Ran
uv run pytest tests/test_serialization.py tests/test_thread_faithful.py tests/test_chat_faithful.py tests/test_slack_format.py tests/test_gchat_format_extended.py -q- result:
343 passed, 2 skipped
- result:
- Verified the nested
currentMessageregression test intests/test_serialization.pypasses. - Re-ran a direct
json.loads(..., object_hook=reviver)repro and confirmed it now returns aThreadImplwith a real nestedMessageinstance.
Verdict
APPROVE
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607088421b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "currentMessage": self._current_message.to_json() if self._current_message else None, | ||
| "isDM": self._is_dm, | ||
| "adapterName": self.adapter.name, | ||
| "adapterName": self._adapter_name or self.adapter.name, |
There was a problem hiding this comment.
Using _adapter_name first causes to_json() to emit the original payload adapter name even after ThreadImpl.from_json(..., adapter=...) has bound a different adapter instance. In that scenario, runtime calls use the explicit adapter but serialized output points to a stale adapter, so a later deserialize can resolve the wrong adapter (or fail) despite the current thread being valid.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Addressed in commit 6c3871c (same commit). ThreadImpl.from_json(data, adapter=X) and ChannelImpl.from_json(data, adapter=X) now set _adapter_name = adapter.name so to_json() reflects the bound adapter, not a stale name from the payload. Regression tests in tests/test_serialization.py::test_should_sync_adapter_name_when_explicit_adapter_is_bound and tests/test_channel_faithful.py::test_should_sync_adapter_name_when_explicit_adapter_is_bound. Safe to resolve.
Generated by Claude Code
…ebind 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
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
Same as the entry on the type-cleanup branch — keeps the parallel-subagent worktree directory out of git. https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ea626be65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf7456355f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…>/|/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
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a lint/type-check CI workflow, bumps version to 0.4.26, introduces a top-level JSON Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer
participant Thread as ThreadImpl
participant Adapter as Adapter
Renderer->>Thread: provide rendered chunk / get_committable_text()
alt committable non-whitespace
Thread->>Adapter: post initial message (placeholder or content)
Adapter-->>Thread: returns message id
Thread->>Adapter: edit message for subsequent non-whitespace commits
else only whitespace or empty
Thread-->>Renderer: skip initial post/edit
alt stream ended whitespace-only and placeholder exists
Thread->>Adapter: edit placeholder to " "
Adapter-->>Thread: ack / error (logs warning on failure)
end
end
Thread->>Adapter: final post or final edit depending on last non-whitespace content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
patrick-chinchill
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6168961f41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5daeb324aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-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
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e141331e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c345ac3d82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ilure 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
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
…lder 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
…ty 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
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 lengthcheck 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 ofClick here (https://...).fix(chat): export standalone
reviverfor 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:
empty-content semantics (no more empty intermediate posts/edits)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests