telegram: send chat files as documents/photos - #84
Conversation
There was a problem hiding this comment.
Hi @headlong42! From the review:
The file branch is placed correctly: it runs after the
source == "chat"guard, the telegram-name/identity checks, the allowlist check, and dedupe, andfile_payloadreads only fields off the step and reduces filenames withPath(...).name, so it cannot be steered into reading files off the box. Reading the checkout, five reviewers still found the slice not ready:
bin/chat(cmd_send_file, ~205-209) +telegram/src/headlong_telegram/filepayload.py— decide and implement how binary file content reaches the trajectory. Todaycontent=$(cat ...)plusjq -Rsadrops NULs and replaces invalid UTF-8, andmindlog(json.loads) always yieldsstr, so theisinstance(content, (bytes, bytearray))PNG-magic path is unreachable in production and a real PNG arrives corrupted. Either add an explicit base64-encoded field that the bridge decodes, or restrict this slice to text files and delete the bytes branches plus the tests that injectbytessteps.telegram/src/headlong_telegram/outbound.py(file branch) — onApiErrorfromsend_photo, retry the same payload withsend_documentbefore consuming the step, and widen theexceptto(ApiError, httpx.HTTPError).mindlog.followpersists the cursor before yielding, so today a rejected or timed-out upload is lost with no chat-visible trace, and a non-ApiErrortransport failure ends the outbound daemon thread.telegram/src/headlong_telegram/outbound.py— gate the file branch on the delivery target actually being authorized (e.g. requireconv.user == conv.chatfor private routes, or authorizeconv.chat). The allowlist checksconv.userwhilesend_photo/send_documenttargetconv.chat, andnaming.decodeacceptstelegram-<approved-user>-<other-chat>. The mismatch is pre-existing on the text path, so the maintainer may prefer to fix it repo-wide; either way, uploads should not ship on top of it unguarded.telegram/src/headlong_telegram/filepayload.py— key file detection on the explicitfilenamestamp thatchat send-filewrites; drop thefilealias, the<svg-prefix sniff, and theFILE_TYPES/typedfallback (unreachable, since the caller already requirestype == "message"). Also truncate captions to Telegram's 1024-character media-caption limit rather than 2000, since an over-long caption currently drops the whole file.telegram/tests/test_outbound.py— restoretest_recent_posts_dedupe_window. It was the only coverage ofRecentPosts, which is unchanged by this PR and is one of the guards the new branch depends on.
Definitely interested in this feature but please fix the above before merge.
Stamp files as content_b64, require an explicit filename (no SVG sniff), drop non-DM destinations, cap captions at 1024, and restore RecentPosts coverage.
|
@headlong42 Thanks for the iteration. The route check against the allowlist, the restored dedupe test, the httpx error handling, and the 1024 caption limit all look right. The fix commit introduced problems of its own:
Still open from the first review:
let me know when you make another pass. |
Address remaining laude-institute#84 review: stamp send-file via jq --rawfile so the payload never sits on argv, treat JPEG magic like PNG for sendPhoto, drop the duplicated outbound file-send block, and spell the inbound vs outbound media split in SECURITY.md.
…fail Finish remaining laude-institute#84 review: accept only canonical `filename`, run text file bodies and captions through strip_leaked_command, and fall back to a short text notice when sendPhoto/sendDocument fails after the cursor has already advanced.
|
Made another pass. Remaining items from the last review:
Latest commit: d423537. telegram filepayload/outbound/tgfmt tests: 28 passed. |
|
From review:
Thanks for the continued work on this @headlong42 — the Telegram side is in good shape now, and the review confirmed the base64 round-trip and the group-chat guard both hold up. On (1): please keep a short human-readable For (6), document the |
Nick's remaining laude-institute#84 notes: stamp a short human-readable `content` (e.g. `[file: fig.png]`) alongside `content_b64` so existing readers keep working, and document that pair as the file-step shape.
|
Pushed 34f8286 for the remaining notes:
Telegram side unchanged (already prefers |
|
Thanks @headlong42, the two fixes you pushed look good but you missed several notes though.
This will be mergeable after those four fixes. |
Nick's leftover laude-institute#84 notes: reject empty/oversize files at the producer, elide content_b64 from model-facing context, treat undecodable content_b64 as a failed delivery rather than falling through to sendMessage, and cover the producer with a bash test.
Outbound treated every mind-log message as sendMessage, so a PNG/SVG from chat send-file landed as raw bytes in the chat. Detect file-bearing steps, add Bot.send_document/send_photo, and keep the source=chat delivery guard.
Stamp files as content_b64, require an explicit filename (no SVG sniff), drop non-DM destinations, cap captions at 1024, and restore RecentPosts coverage.
Address remaining laude-institute#84 review: stamp send-file via jq --rawfile so the payload never sits on argv, treat JPEG magic like PNG for sendPhoto, drop the duplicated outbound file-send block, and spell the inbound vs outbound media split in SECURITY.md.
…fail Finish remaining laude-institute#84 review: accept only canonical `filename`, run text file bodies and captions through strip_leaked_command, and fall back to a short text notice when sendPhoto/sendDocument fails after the cursor has already advanced.
Nick's remaining laude-institute#84 notes: stamp a short human-readable `content` (e.g. `[file: fig.png]`) alongside `content_b64` so existing readers keep working, and document that pair as the file-step shape.
Nick's leftover laude-institute#84 notes: reject empty/oversize files at the producer, elide content_b64 from model-facing context, treat undecodable content_b64 as a failed delivery rather than falling through to sendMessage, and cover the producer with a bash test.
ad7f790 to
f68ae29
Compare
Drop unused `rel` from `traj new` output (SC2034) and fail if cd into the workdir fails (SC2164). CI shellcheck -S warning was failing on these two lines in tests/test_chat_send_file.sh.
Rebase conflict resolution had re-included reasoning steps in _recent_stream, which main and test_recent_stream_filter.sh both exclude. CI bash tests failed 3 cases because of it.
nickjalbert
left a comment
There was a problem hiding this comment.
Thanks! From review:
- Preserve
chat send-filebehavior for other transports (bin/chat:238)
chat send-fileis a shared Headlong command. Before this change, a text file's contents were stored incontent, so Slack and web chat delivered those contents. The new step stores only[file: name]incontent, while only Telegram decodescontent_b64. Slack and web chat now deliver the marker instead of the file contents.Preserve the existing text file behavior for other readers, or update every supported transport to consume the new file payload. Add a regression test showing that a text file sent through a non Telegram route still delivers its contents.
- Remove file bytes from the dashboard API response (web/src/headlong_web/trajectory.py:239)
The dashboard normalizer copies the complete step into
raw, includingcontent_b64, and the mind log endpoint sends that object to the browser. A 1 MiB test file produced a 1,398,488 byte response. A file at the new 10 MiB limit adds about 13.3 MiB to each response and remains in browser memory even though the dashboard displays only the short marker.Strip
content_b64from the dashboard wire representation during both normal ingestion and raw rehydration. Add a web API test showing thatfilenameand the marker remain visible whilecontent_b64is absent.
- Apply duplicate suppression to file uploads (telegram/src/headlong_telegram/outbound.py:81)
The file branch returns before the existing
RecentPostscheck at line 121. Two identical file steps therefore produce two Telegram uploads. I reproduced this with two steps containing the same filename and bytes.Build a bounded file signature, such as the filename plus a hash of the decoded bytes, and pass it through duplicate suppression before uploading. Test that identical files are suppressed while different contents with the same filename are both delivered.
Other comments
- Nit: Remove blank lines at end of the new Python tests (telegram/tests/test_filepayload.py:111)
git diff --checkreports extra blank lines at the ends oftest_filepayload.pyandtest_outbound.py.
Nick's latest laude-institute#84 review: - chat send-file keeps a text file's body in content so Slack/web still deliver it; binary files keep the short marker plus content_b64 - dashboard ingest and window rehydrate drop content_b64 from the wire form - Telegram file uploads go through RecentPosts via filename + content hash - drop extra blank lines at EOF in the new Python tests
This slice:
filename/ PNG / SVG sniff)Bot.send_documentandBot.send_photo(multipart)sendPhotoand everything else tosendDocumentIntentionally not in this PR: identity-local responder dest-history stub, notes, secrets, web build artifacts.
Pushed from
headlong42/headlongbecauseheadlong42has pull-only onlaude-institute/headlong(direct origin push 403).