feat(ai): keep attachment/link-only messages in to_ai_messages, add ChatTool.name (#198) - #281
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # scripts/fidelity_target.json
|
Merge gate: CI green on e24a4a2 (Lint & Type Check, test (3.12), test (3.13), CodeQL, Analyze (python), Analyze (actions)); local Codex review (gpt-6-astra, xhigh, --base origin/main) on 3737469: "No actionable regressions were found. All 143 targeted tests passed, along with focused Ruff checks and project-wide Pyrefly checking."; 3 astra rounds, all clean. e24a4a2 only merges origin/main (#272/#229/#273) and regenerates fidelity_target.json; the PR's src/+tests/ diff vs origin/main is byte-identical to the reviewed one and main's changes are disjoint from chat_sdk/ai/, so no re-review was needed. Local full validation on e24a4a2: pytest 6869 passed / 24 skipped, strict fidelity 733/733, pyrefly 0 errors. Bots: CodeRabbit rate-limited (no review). Merging with --admin (Protect Main requires a code-owner approval). |
Summary
to_ai_messagesused to drop every message whose text was empty or whitespace-only, so caption-less images, text-file uploads and link-only messages never reached the model. It now keeps any message with usable content (text, a fetchable image or text file, or a link preview) and skips only messages whose built content is empty, beforetransform_messageruns.ChatToolgainsname(upstream's framework-agnosticChatToolSpec.name).Upstream commits mapped
25f30998fix(chat): keep attachment/link-only messages in toAiMessages (#713, chat@4.35.0)Links:\n…with no leading blank line. Leading text part only when there is text or links. Empty-content skip (whitespace-only string / empty list) beforetransform_message.on_unsupported_attachmentstill fires for video/audio on text-less messages.21dc60c3refactor(chat): define chat/ai tools once as framework-agnostic specs (#935, chat@4.41.0)ChatTool.name: str = ""(last field), set by every factory to its camelCase id. Shared helpers (_render_link_for_prompt, MIME helpers,_sort_by_date_sent,_build_message_text,_is_unsupported_attachment,_attachment_to_part) moved to privatechat_sdk.ai.message_content(upstreamai/message-content.ts). The AI-SDKtoAiTool/ TanStack wrappers are TS-only (#203).eddcd7e4fix(telegram): return portable file data (#828, chat@4.39.0)messages.test.tscase only: ported withbytearrayandmemoryviewfetch_dataresults, assertingdata:image/png;base64,AQID. Telegram adapter half is #228.Tests ported
packages/chat/src/ai/messages.test.ts→tests/test_ai_messages.py:bytearray/memoryview)Python-specific:
test_ai_messages.py: the transform never sees a skipped empty message; no[name]:prefix on link-only messages withinclude_names; text-less text-file + link message leads with theLinks:part; the whitespace check uses JStrim's set (BOM-only skipped, NEL-only kept); a BOM-only message is also skipped withinclude_names(catches astr.strip()regression in the per-message text check directly).test_ai_tools.pyTestToolNames: every built tool'snameequals its key; anameoverride is ignored and not stashed inextras; positionalChatTool(...)construction is unchanged.The existing "filters out empty and whitespace-only text" and "returns empty array when all messages have empty text" tests pass unchanged. The 9 new upstream-named tests fail against
origin/main'smessages.py(checked by swapping the file in): 8 behavior tests plus the JS-whitespace one. The ArrayBuffer test already passed there, as the issue expected.Fidelity
Delta vs committed report (HEAD): missing 167 -> 158 (-9)packages/chat/src/ai/messages.test.ts: 9 -> 0 (-9)Strict pin (
chat@4.31.0): 733/733 (all TS tests have Python equivalents).Divergences
No new non-parity table rows. Recorded in
docs/UPSTREAM_SYNC.mdunder "AI messages without text and tool names (#198)":"name"is added to_PROTECTED_TOOL_FIELDS. This is Python-only, because upstream's AI SDK tools have no name. It keepsnamein sync with the dict key.data:URL. Upstream passes anArrayBufferthrough raw. As before, an unnamed attachment getsfilename="".trim's code-point set (shared._js_compat.JS_WHITESPACE) rather thanstr.strip(), to match upstream exactly.Consumer impact
Low. This only affects
chat_sdk.aiusers; there is no Slack/Teams streaming impact.to_ai_messagescan return more messages than before: image-, text-file- and link-only messages are now included.contentcan now start with a file part when the message has no text.ChatTool.namefield. It is the last field, so positional and keyword construction keeps working.overridescannot change it.TEXT_MIME_PREFIXESis still importable fromchat_sdk.aiandchat_sdk.ai.messages.Closes #198
Part of #184
Merge gate
Review findings (2 reviewers, both on
tests/test_ai_messages.py:875)"\ufeff", like the neighbouring"\x85".has_text = msg.text.strip() != ""mutation, because the downstream empty-content check strips it again. Addedtest_bom_only_message_skipped_with_include_names; with the mutation applied it fails (content would be"[testuser]: \ufeff"), and it passes on the real code.gpt-6-astra: 2 rounds, both clean.
8b08fc8: "No actionable regressions were found against the supplied merge base."3737469(empty commit to re-trigger Lint on the non-draft PR; same tree as8b08fc8): "No actionable regressions were found."Bots: CodeRabbit skipped the draft and was rate-limited after ready-for-review; no inline comments or reviews from CodeRabbit or gemini.
CI on
3737469: all green (Lint & Type Check incl. strict fidelity + pyrefly, test 3.12/3.13, CodeQL).Local full validation: ruff check/format, test-quality audit,
--check-docs, strict fidelity 733/733, pytest 6692 passed / 24 skipped, pyrefly 0 errors.origin/main(f2cf21b) already merged;fidelity_target.jsonregenerated (Delta vs committed report (HEAD): missing 158 -> 158 (+0); one extra Python test).Pre-merge main integration (e24a4a2): merged
origin/main(1ee200f: #272 lifecycle events, #229 Discord, #273 WhatsApp). Only conflict wasscripts/fidelity_target.json, regenerated per convention: vs main's committed report, target missing 160 -> 151 (all 9ai/messages.test.tsgaps closed by this PR). The PR'ssrc/+tests/diff vsorigin/mainis byte-identical to the astra-reviewed diff on3737469; main's changes are disjoint fromchat_sdk/ai/(types.py changes are additive event types only). Local full validation: ruff, format, audit,--check-docs, strict fidelity 733/733, pytest 6869 passed / 24 skipped, pyrefly 0 errors.