fix(adapters): notify agent of failed image attachments so reply aligns with warning#826
fix(adapters): notify agent of failed image attachments so reply aligns with warning#826howie wants to merge 17 commits into
Conversation
…odel Fixes openabdev#776. When a Slack bot token lacks the `files:read` OAuth scope, Slack serves the workspace login HTML page (~55 KB) at HTTP 200 with a `text/html` Content-Type instead of the requested file binary. `download_and_encode_image` previously accepted this response because: 1. It never inspected the HTTP response `Content-Type` header. 2. On `resize_and_compress` failure for a body ≤ 1 MB it fell back to forwarding the raw bytes under the Slack-reported MIME (`image/png`), bypassing any format check. The result: a `ContentBlock::Image { media_type: "image/png", data: <base64 HTML> }` flowed through to Anthropic, which 400'd with "Could not process image". Because claude-agent-acp persists the user message into the session JSONL before the API reply, the bad block replayed on every subsequent turn in that Slack thread until an operator manually deleted the JSONL inside the pod. Changes: - Add `MediaFetchError` enum to `src/media.rs` so callers can distinguish "not an image, skip silently" (`NotAnImage`) from "claimed image, got unexpected bytes" (`UnsupportedResponseType`, `InvalidImageBody`). - Add `validate_image_response(content_type, body)` pure helper that: - Rejects any HTTP response whose Content-Type (stripped of params, lowercased) is not in `{image/png, image/jpeg, image/gif, image/webp}`. - Sniffs magic bytes via `image::ImageReader::with_guessed_format()` (no new dependencies) and rejects anything that doesn't decode as one of the four supported formats. - Change `download_and_encode_image` signature from `-> Option<ContentBlock>` to `-> Result<ContentBlock, MediaFetchError>`, capturing the Content-Type header before consuming the response with `.bytes()`. - Remove the ≤ 1 MB resize-error fallback that was the direct bug path. - Update `src/slack.rs` call site: on validation failure, collect filenames and post one aggregated user-visible warning to the Slack thread: ":warning: I couldn't access the file(s) you shared (`<name>`). This often means the bot is missing the `files:read` OAuth scope. Please ask an admin to reinstall the app with that scope." - Update `src/discord.rs` call site: `warn!` log on failure (Discord URLs are signed-public so the Slack scope hint is not applicable there). Preserve the existing `is_video_file` fallback for `Err(NotAnImage)`. - Add 12 unit tests for `validate_image_response` including the exact bug repro case (HTML body labeled `image/png`, first 8 bytes `3c21444f43545950`). Out of scope / follow-up issues: - Secondary defense: deferring claude-agent-acp JSONL persistence until after model returns 200 (requires changes in the claude-agent-acp Node project). - Startup preflight calling Slack `auth.test` to warn loudly on missing scopes. - Same Content-Type/magic-byte hardening for `download_and_transcribe` and `download_and_read_text_file`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove dead hinted field from UnsupportedResponseType (always None) - Eliminate double reader.format() call with fmt@ binding - Deduplicate hex_prefix() in resize error path (compute once, reuse) - Promote strip_mime_params to media::strip_mime_params (pub crate), slack.rs delegates to it -- single source of truth for MIME stripping Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Critical: change Content-Type check from allow-list to block-list (Codex finding). The allow-list rejected application/octet-stream before magic-byte check ran, silently dropping valid images from CDNs. Only text/* is now rejected early; everything else falls through to magic-byte verification. Also: - Soften Slack warning message: no longer attributes all failures to files:read scope; now mentions format support as a second cause - Add SizeExceeded to Slack user notification (was silent) - Log failures from send_message() instead of using let _ = - Log discarded io::Error from with_guessed_format - Fix doc comments: download_and_encode_image (SizeExceeded fires pre-HTTP), validate_image_response (Content-Type check short-circuits, not sequential) - Replace inline "Validate Content-Type..." comment with WHY explanation - Restore doc comment on strip_mime_params wrapper in slack.rs - Add tests: octet-stream acceptance (Codex regression fix), JSON body rejection by magic bytes, missing Content-Type + invalid body Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex adversarial review found that user-controlled filenames embedded in the mrkdwn warning message could inject Slack markup (backtick break-out, <!here> mentions, <@uid> pings). Replace backticks and angle brackets with safe ASCII equivalents before embedding in the message. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
F1: validate_gif_body now decodes only the first frame instead of
collect_frames() — avoids full in-memory decode of large animated GIFs.
F2: remove duplicate validate_gif_body call from resize_and_compress;
download_and_encode_image already runs validate_image_response before
calling resize, so the second call was redundant.
F3: add MediaFetchError::ProcessingFailed(image::ImageError) for the case
where body passed validation but resize/compress failed — previously
returned the misleading InvalidImageBody variant for a validated image.
F4: extend Slack warning message to mention "file is too large" so the
message is accurate when SizeExceeded failures are included.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Behavior: - slack: add explicit ProcessingFailed arm -> push to failed_image_files and log "post-processing failed" (not "download failed") - slack: extract sanitize_slack_filename() pub(crate) fn; add 4 unit tests for backtick/angle-bracket injection prevention API: - validate_image_response: change return type Result<ImageFormat> -> Result<()> (sole caller only checked Ok/Err; format detection ran twice) Docs: - validate_image_response: add block-list vs allow-list design rationale - validate_gif_body: add doc comment explaining first-frame-only and cursor independence; log original error via debug! before mapping to InvalidImageBody - ProcessingFailed variant: expand doc to clarify semantic difference from InvalidImageBody and expected caller behavior - download_and_encode_image: add ProcessingFailed to error listing Tests: - validate_rejects_mixed_case_text_content_type: pin .to_lowercase() normalization Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: This PR will be automatically closed in 3 days if the link is not added. |
github-actions
Bot
added
the
closing-soon
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening reportblocked before `gh` could run. the shell sandbox fails immediately with:so i could not verify the first Draft screening report from the provided source data: IntentPR #826 fixes a UX mismatch when image attachment validation fails. The user already sees an adapter warning, but the agent reply does not know an image was attempted, so it can incorrectly respond as if no image was attached. FeatFix. The adapters now pass failed-attachment context into the model prompt so the agent can acknowledge the validation failure and continue coherently. Who It ServesDiscord users, Slack users, and reviewers validating attachment-handling behavior. Rewritten PromptUpdate Slack and Discord image attachment handling so that when an attachment fails validation or fetch/encode processing, the user still receives the existing visible warning, and the agent also receives a structured text note describing the failed attachment filenames. Add shared formatting in Merge PitchThis should move forward after #793 lands because it closes a real conversational gap without changing the visible warning contract. Risk is moderate because it touches both adapters and media error handling; the likely reviewer concern is ordering and whether the injected note leaks adapter-specific language into model context. Best-Practice ComparisonOpenClaw and Hermes scheduling patterns mostly do not apply here. The relevant best-practice overlap is explicit delivery routing and run/context logs: the adapter should preserve enough context for downstream execution instead of silently dropping failed input. This PR moves in that direction by making attachment failure explicit to the agent. Implementation OptionsConservative: only add the model note in Slack, where the original issue was observed. Balanced: add shared note formatting in Ambitious: introduce a typed adapter event for failed attachments, then let prompt packing render those events consistently across all adapters. Comparison Table
RecommendationTake the balanced path after #793 is merged or this branch is rebased onto the post-#793 Agent-ran OpenAB PR screening. Feedback welcome; react thumbs-up if useful. |
|
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1505114156376784946 |
- Restore ≤1MB raw-byte fallback when resize_and_compress fails after validate_image_response passes. The body is confirmed valid by magic-byte check, so forwarding original bytes is safe. Prevents regression for formats the image crate can detect but not fully decode (e.g. animated WebP). - Add HttpStatus 4xx match arm in Slack handler to push filename into failed_image_files. HTTP 4xx (401/403) indicates a persistent permission problem (similar root cause to openabdev#776) and the user should be notified. 5xx and Network errors remain log-only (transient).
…t reply aligns with warning When download_and_encode_image rejects an attachment, the Slack/Discord adapter already sends the user a visible⚠️ message. Without this change, the agent sees no attachment and may reply "I don't see any image attached" -- contradicting the warning the user just received. - Add media::format_failed_attachment_note() as a shared helper (single source of truth) for the ContentBlock::Text injected into extra_blocks. - Slack: push the note after the existing warning send (pack_arrival_event partitions Text blocks before the typed prompt; stable iteration order means push trails any STT transcript, which is correct). - Discord: introduce failed_image_files Vec, split the catch-all Err arm into variant-specific arms matching Slack, send a Discord-flavored user warning (without the Slack files:read scope hint), and inject the note. - Add three unit tests for format_failed_attachment_note in media::tests. Addresses the minor UX observation from the openabdev#776 final review comment (antigenius0910, 2026-05-14). Depends on PR openabdev#793 (failed_image_files Vec and MediaFetchError variants introduced there). Fixes openabdev#776 (UX follow-up). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- format_failed_attachment_note: &[&str] -> &[String] to remove intermediate Vec<&str> at both call sites - Remove inaccurate "Caller must guarantee non-empty" doc precondition - discord.rs: ProcessingFailed(ref e) -> ProcessingFailed(e) (spurious ref) - discord.rs: sanitize filenames before embedding in user warning (backtick closes code span; <> enables mention injection in Discord) - slack.rs: trim 4-line ordering comment to 3-line essential WHY Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ize helper - Move send_message into the correct thread: warn_channel was built from msg.channel_id before get_or_create_thread ran, routing the warning to the parent channel while the agent reply landed in the newly-created thread. Now send_message uses thread_channel (resolved first). - Extract sanitize_discord_filename (parity with sanitize_slack_filename) and add 3 tests covering normal, backtick, and injection-attempt cases. - Add channel_id to the warning-send failure log for debuggability. - Add comment on catch-all Err arm: Network/HTTP drops are intentional. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l path - format_failed_attachment_note replaces backtick with quote in filenames to prevent broken Markdown code-spans in the LLM prompt. - Add test: format_failed_attachment_note_escapes_backtick_in_filename. - Document the get_or_create_thread failure early-return: agent dispatch and user-visible warning are both dropped, consistent with pre-existing thread-fail behavior for any message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Addresses two regression surfaces flagged in reviewer Pass-2 follow-up: 1. media.rs: resize-fallback threshold (restored by 6d7fd5f, missing from this branch before the rebase). Extract encode_validated_image() to make the <=1 MB fallback testable without HTTP I/O. Add two new tests: - resize_fail_under_1mb_falls_back_to_original_bytes - resize_fail_over_1mb_returns_processing_failed 2. slack.rs / discord.rs: HTTP 4xx silently dropped by catch-all Err(e) arm. Add shared media::failed_attachment_entry() helper (pure fn, no logging) that maps MediaFetchError -> Option<String>: 4xx returns Some (user notified, agent receives format_failed_attachment_note); 5xx/network returns None (logged only). Both adapters now call the helper instead of duplicating the multi-arm match, and discord.rs gains parity with the 4xx arm that 6d7fd5f added only to slack.rs. New tests in media::tests (5) and discord::tests (2) covering: - failed_attachment_entry: NotAnImage, SizeExceeded, 401, 403, 502 - discord parity: 403 notifies, 500 is logged-only Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
howie
force-pushed
the
fix/776-agent-reply-alignment
branch
from
a5e0d49 to
1bbe2d5
Compare
|
Rebased onto fix/776-validate-fetched-image-bytes so 6d7fd5f's two restorations sit beneath this PR's UX commits, and the two Pass-2 surfaces are now pinned with tests. 1. media.rs: resize-fail fallback (<=1 MB path) Extracted encode_validated_image() to make the fallback testable without HTTP I/O. New tests:
2. slack.rs: HTTP 4xx -> failed_image_files arm Added media::failed_attachment_entry(filename, e) -> Option: pure fn returning Some for 4xx + validation/processing errors (user-notifiable), None for 5xx + network (logged only). Both slack.rs and discord.rs now call this instead of duplicating the multi-arm match. Five new tests in media::tests cover: NotAnImage, SizeExceeded, 401, 403, 502. Two parity tests in discord::tests confirm Discord's path is identical. 3. discord.rs: 4xx parity Discord's catch-all Err(e) previously dropped 4xx silently (same as the old Slack path). Both adapters now share failed_attachment_entry, so Discord CDN 4xx (expired URL, restricted attachment) pushes into failed_image_files -> user warning + agent format_failed_attachment_note. All 344 tests pass; build clean (no warnings). |
- Extract sanitize_attachment_filename into media.rs as canonical helper; sanitize_slack_filename and sanitize_discord_filename delegate to it - Collapse multi-arm image-error matches in slack.rs and discord.rs to two arms (NotAnImage no-op + unified Err(e) via failed_attachment_entry) - Remove two redundant discord parity tests that duplicated media.rs coverage - Add wildcard comment to failed_attachment_entry clarifying opt-in default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- failed_attachment_entry: SizeExceeded returns bare filename only - failed_attachment_entry: replace wildcard with explicit Network/HttpStatus arms - encode_validated_image: narrow WebP fallback to mime == image/webp only - encode_validated_image: elevate fallback log from debug to warn - discord.rs: move agent-note push to after thread_channel is resolved - discord.rs: remove duplicate doc on sanitize_discord_filename - slack.rs: clarify push comment re STT insert ordering dependency - media.rs: document format_failed_attachment_note sanitizes internally - media.rs: document NotAnImage => None is deliberate skip not transient - Tests: update SizeExceeded assertion; add non-WebP resize-fail test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Summary
Follow-up to #793, addressing the minor UX observation noted in the final review comment on #776 (antigenius0910, 2026-05-14):
Root cause
download_and_encode_imagerejects the bad attachment and the adapter sends the user a:warning:message. Butextra_blockscontains no trace of the failed attachment, so the agent prompt is identical to a text-only message — the agent has no way to know an image was attempted and silently dropped.Fix
After sending the user-visible warning, push a
ContentBlock::Textsystem note intoextra_blocks:AdapterRouter::pack_arrival_eventpartitions Text blocks before the typed prompt (stable insertion order), so the note appears after any STT transcript but before the user's message — the correct position for meta-context.Changes
src/media.rsformat_failed_attachment_note(filenames: &[&str]) -> Stringhelper + 3 unit testssrc/slack.rsextra_blocksafter the existing:warning:send (~4 lines)src/discord.rsfailed_image_filesVec; split catch-allErr(e)into variant-specific arms (mirroring Slack); send Discord-flavored user warning (without Slack-specificfiles:readscope hint); inject noteBefore / After (Slack thread, missing
files:read)Before (#793): user sees
:warning:, then agent: "I don't see any image attached — could you share it again?"After: user sees
:warning:, then agent: "I see your image couldn't be processed due to a validation error. If you've added thefiles:readscope, try sharing it again in a new thread."Design notes
format_failed_attachment_notelives insrc/media.rsnext toMediaFetchError— single source of truth shared by both adapters.push(notinsert(0, …)) because STT transcripts are the primary content and useinsert(0, …)to win front position; failed-image notes are meta-info and correctly trail any transcript."big.png (exceeds 10000000 byte limit)"), which renders fine inside backticks in the agent note.sanitize_slack_filenameneeded.Dependency
This PR depends on #793. The
failed_image_filesVec andMediaFetchErrorvariants used here were introduced in that PR and do not exist inmainyet. Please merge #793 first (or rebase this branch ontomainafter it lands — the diff is clean).Test plan
cargo test --locked media::tests format_failed_attachment_note— 3 new tests (single file, multiple files, size-suffix preserved)cargo test --locked— no regressions expected (additive change only)files:read→ upload image → user sees:warning:(unchanged); agent reply now acknowledges the failure rather than asking "where's the image?"Closes #776 (UX follow-up noted in final review comment).