fix(media): validate Content-Type and magic bytes before sending to model by howie · Pull Request #793 · openabdev/openab

howie · 2026-05-11T11:54:05Z

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1503586535088590868

Fixes #776.

Root cause

When a Slack bot token lacks the files:read OAuth scope, Slack serves the workspace login HTML page (~55 KB) at HTTP 200 with Content-Type: text/html instead of the requested file binary. download_and_encode_image accepted this response because:

It never inspected the HTTP response Content-Type header.
On resize_and_compress failure for a body <= 1 MB it fell back to forwarding raw bytes under the Slack-reported MIME (image/png), bypassing any format check.

The result: a ContentBlock::Image { media_type: "image/png", data: <base64 of HTML> } flowed 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 until an operator manually deleted the JSONL inside the pod.

Changes

src/media.rs (primary change)

Add MediaFetchError enum: NotAnImage (silent skip), UnsupportedResponseType, InvalidImageBody, SizeExceeded, Network, HttpStatus.
Add validate_image_response(content_type, body) pure helper that:
- Rejects any response Content-Type not in {image/png, image/jpeg, image/gif, image/webp} (strips params, case-insensitive).
- Sniffs magic bytes via image::ImageReader::with_guessed_format() (zero 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 (the direct bug path).

src/slack.rs (call site)

On validation failure, collect filenames and post one aggregated user-facing warning after the file loop:

":warning: I couldn't access the file(s) you shared (photo.png). This often means the bot is missing the files:read OAuth scope. Please ask an admin to reinstall the app with that scope."

Transient errors (Network, HttpStatus) log at warn! and skip silently.

src/discord.rs (call site)

Same Result pattern but log-only on failure (Discord URLs are signed-public; the Slack scope hint is not applicable). Preserves the existing is_video_file fallback for Err(NotAnImage).

Tests

13 new unit tests in src/media.rs::tests for validate_image_response, including the exact bug reproduction and a corrupt-GIF regression:

validate_rejects_html_body_labeled_as_image_png
  body: b"<!DOCTYPE html>..."
  content_type: Some("image/png")
  expected: Err(InvalidImageBody { magic_prefix_hex: "3c21444f43545950" })

Smoke validation:

RUSTFLAGS='-C linker=/tmp/zigcc-wrapper' CC=/tmp/zigcc-wrapper AR=/tmp/zigar-wrapper cargo test --locked media::tests

Result: 24 media tests passed.

Manual test plan (post-deploy)

Install with a bot token missing files:read. Confirm via x-oauth-scopes from auth.test.
Upload an image to the bot in a Slack thread.
Expected: bot replies with the scope warning. Anthropic is never called with the bad block. No JSONL poisoning.
Grant files:read, rotate token, redeploy. Upload an image in the same thread.
Expected: succeeds on first try -- no manual JSONL deletion needed.

Out of scope / follow-ups

Session JSONL persistence: deferring claude-agent-acp write-to-JSONL until after model 200 requires changes in the claude-agent-acp Node project (separate repo). This PR prevents bad bytes from reaching the child process.
Startup preflight: auth.test + apps.permissions.info at boot to warn on missing files:read (useful early-warning, separate concern).
download_and_transcribe / download_and_read_text_file: analogous hardening for audio/text-file paths (lower-priority, separate PR).

At a Glance

Slack/Discord attachment
  -> download_and_encode_image()
  -> reject text/error responses early
  -> sniff and validate image bytes
  -> resize/compress or pass through validated GIF
  -> model-bound ContentBlock::Image

Prior Art & Industry Research

Not applicable: this is a narrow bug fix in existing attachment validation, not a new runtime, persistence, delivery, scheduling, or architectural subsystem.

Why This Approach

The model contract requires real PNG/JPEG/GIF/WebP bytes. Validating after download but before building ContentBlock::Image blocks Slack HTML/login pages and corrupted image bodies at the boundary closest to the failure.

Generic binary responses such as application/octet-stream still pass through to magic-byte validation, so CDN behavior is preserved.

Alternatives Considered

Strict Content-Type allow-list: rejected valid CDN images served as application/octet-stream.
Keep the old raw-byte fallback after resize failure: preserved the original poisoning path.
Fix only Slack call sites: left Discord and future callers exposed to the same invalid-byte contract drift.

…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>

howie · 2026-05-11T12:58:06Z

Codex Challenge Report — Adversarial Review

Finding 1: Slack filename mrkdwn injection [FIXED]

Filenames embedded in the Slack warning message were user-controlled. A filename containing backticks, <@uid>, or <!here> could break out of the inline-code wrapper and inject Slack markup (mentions, @here pings, formatting). Fixed in commit 4e1a682: backticks and angle brackets are now sanitized before embedding.

Finding 2: Corrupt GIF bodies pass magic-byte check [Known Issue — not in scope]

GIF format is detected by magic bytes (GIF89a/GIF87a) but is passed through without decoding in resize_and_compress to preserve animation. A body with valid GIF magic bytes but corrupt/truncated payload will pass validate_image_response and be forwarded to Anthropic. PNG/JPEG/WebP are caught by the full decode step.

This is pre-existing behavior from before this PR. Fixing it would require decoding GIF frames for validation, which risks breaking animated GIF support. Filed as a known limitation; a follow-up PR should add frame-count validation for GIFs.

Finding 3: failed_image_files Vec is unbounded per event [Acceptable]

The Vec is bounded by Slack's own message attachment limit (~20 files). Not a persistent leak. Acceptable for now.

No TOCTOU between Content-Type capture and body read

Headers and body come from the same immutable reqwest::Response. Server can lie in headers but body validation catches that.

hex_prefix cannot panic

Uses .take(8) with no indexing; handles empty and short slices correctly.

Mixed success: one valid PNG + one HTML file in same message

Valid PNG → pushed to extra_blocks. HTML file → pushed to failed_image_files. Agent receives the valid PNG. User receives one warning message for the failed file. Behavior is correct.

Generated by /pr-review-cycle-codex Step 8 — Codex adversarial challenge

howie · 2026-05-12T02:37:12Z

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1503586535088590868

shaun-agent · 2026-05-13T09:56:24Z

OpenAB PR-Screening Report

This was generated by an agent-run OpenAB PR-screening workflow. Feedback is welcome; a 👍 reaction is useful if this format helped.

Intent

PR #793 fixes a Slack media-ingestion failure where OpenAB could fetch non-image bytes from Slack, label them as an image, and forward them to the model. The user-visible problem is a confusing Anthropic image-processing error; the operator-visible problem is worse: a bad image block can be persisted into the agent session history and poison later turns in the same Slack thread.

Feat

This is a bug fix and reliability hardening PR. It changes image downloading from a loose Option<ContentBlock> path into a typed Result<ContentBlock, MediaFetchError> path, validates HTTP response content and image magic bytes before forwarding to the model, removes the unsafe raw-byte fallback after resize failure, and adds Slack user-facing warnings for invalid/inaccessible image files.

Who It Serves

Primary beneficiaries are Slack users and OpenAB operators. Users get actionable feedback instead of a provider-level 400. Operators avoid manual PVC/session cleanup when Slack app scopes are wrong or file downloads return HTML/error pages. Discord users also benefit from the shared validation path, though Discord failures remain log-only because Discord attachment URLs have different auth semantics.

Rewritten Prompt

Fix Slack/Discord image ingestion so OpenAB never forwards unverified fetched bytes as model image input.

Requirements:

Capture the HTTP response Content-Type before consuming the body.
Reject explicitly textual responses such as text/html.
Validate downloaded image bodies by format detection/magic bytes before base64 encoding.
Support only model-compatible image formats: PNG, JPEG, GIF, WebP.
Remove any fallback that forwards raw bytes after decode/resize failure.
Return structured media-fetch errors so Slack can distinguish non-images, validation failures, oversized files, network failures, and HTTP failures.
In Slack, warn the user when an image cannot be processed because it is invalid, inaccessible, or likely blocked by missing files:read.
In Discord, preserve video-link fallback for non-image attachments.
Add focused tests for HTML mislabeled as image, missing/generic content type, truncated bodies, MIME parameters, and supported image formats.

Merge Pitch

This PR is worth moving forward because it closes a high-impact ingestion bug with a narrow, well-contained validation layer. The fix prevents a bad Slack fetch from reaching the model, improves user feedback, and reduces the chance of poisoned long-lived sessions. The main reviewer concern should be whether validation is strict enough for corrupted files without breaking legitimate Slack/CDN behavior, especially application/octet-stream downloads and GIF pass-through.

Best-Practice Comparison

OpenClaw principles relevant here:

explicit media loading/normalization pipeline: relevant; this PR moves OpenAB toward a more explicit media validation boundary.
delivery routing and run logs: partly relevant; warnings/logs improve diagnosis when a platform returns an auth/error page instead of media.
retry/backoff and durable job history: not directly relevant to media ingestion.

Hermes Agent principles relevant here:

self-contained prompt/session safety: relevant; bad media should not enter the session transcript as a durable poison pill.
atomic writes for persisted state: conceptually relevant, but the deeper JSONL persistence behavior lives in the agent runtime, not OpenAB core.
fresh session per scheduled run / daemon tick model: not relevant.

The practical lesson from both systems is to treat platform media fetches as untrusted input. Validate at the boundary, fail before model submission, and make recovery possible through normal user/operator action.

Implementation Options

Option A: Conservative
Keep this PR focused on response validation and user warnings. Accept that malformed-but-magic-valid GIFs may still pass through, and track deeper GIF validation/session persistence as follow-ups.

Option B: Balanced
Merge this validation layer, then immediately follow with two small hardening PRs: GIF frame validation and Slack startup preflight for files:read scope. This keeps the fix shippable while addressing the remaining known weak spots.

Option C: Ambitious
Build a full media-ingestion subsystem with adapter-specific fetch policies, strict decode validation for every supported format, provider capability checks at startup, and a session-write policy that only persists media blocks after a successful model call.

Comparison Table

Option	Speed to ship	Complexity	Reliability	Maintainability	User impact	Fit for OpenAB right now
A. Current focused validation	High	Low	High for #776 path	High	High	Strong
B. Validation + near-term hardening	Medium	Medium	Very high	High	Very high	Best next sequence
C. Full media subsystem	Low	High	Highest long-term	Medium	High	Too large for this PR

Recommendation

Recommend moving forward with Option A for this PR, with Option B follow-ups tracked explicitly.

The PR directly addresses #776 and includes the right core changes: validate before encoding, remove raw-byte fallback, and show Slack users an actionable warning. The remaining risk is bounded: GIF validation is still weaker than PNG/JPEG/WebP because the current path can accept GIF magic bytes without fully proving the payload is model-usable. That is a good follow-up, not a reason to hold the whole fix if CI is green.

Validation attempted:

cargo check was run in a clean detached worktree at /tmp/openab-pr793-clean.
The check could not complete because this container lacks linker cc: error: linker 'cc' not found.
PR metadata says the author ran the full test suite successfully, and GitHub discussion-check CI is green.

Project status:

PR fix(media): validate Content-Type and magic bytes before sending to model #793 is already in Project perf: cache dependency build layer in Dockerfile #1 status PR-Screening; no board move was needed.

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>

chaodu-agent

LGTM ✅ — prior image-validation findings are resolved. Latest head keeps the #776 fix intact, avoids full GIF frame decode, removes duplicate GIF validation, separates post-validation processing failures with ProcessingFailed, and updates the Slack warning to cover size-limit failures. CI is green.

- 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).

CHC-Agent

LGTM ✅ — Verified end-to-end on OrbStack k8s with a real Slack workspace.

Tested scenarios:

❌ files:read scope missing → bot correctly rejects text/html response (magic bytes 3c21444f43545950), logs warning, continues operating. No JSONL poisoning.
✅ files:read scope present → image downloads, validates, and reaches the model successfully.

Added commit 6d7fd5f with two minor improvements:

Restore ≤1MB raw-byte fallback when resize_and_compress fails after validate_image_response passes (prevents regression for formats the image crate can detect but not fully decode, e.g. animated WebP).
Add HttpStatus 4xx to failed_image_files in Slack handler so users are notified on persistent permission errors (not just text/html responses).

Build passes. Ship it.

chaodu-agent · 2026-05-16T17:32:17Z

🟢 APPROVE (with NITs)

Verdict: Solid defensive fix. No blocking issues. Recommend merge after contributor addresses optional NITs.

What it solves

When a Slack bot token lacks files:read, Slack returns an HTML login page at HTTP 200 with Content-Type: text/html. The old download_and_encode_image never validated the response body, and on resize failure for bodies ≤ 1MB, forwarded raw bytes under the Slack-reported MIME (image/png). This poisoned the session JSONL with invalid ContentBlock::Image data that replayed on every subsequent turn.

How it solves it

New MediaFetchError enum with 7 variants covering all failure paths
validate_image_response() with dual-layer defense: text/* Content-Type block-list + magic-byte detection via ImageReader::with_guessed_format()
GIF bodies get extra first-frame decode validation
application/octet-stream passes through to magic-byte check (CDN compatibility preserved)
Return type changed from Option<ContentBlock> to Result<ContentBlock, MediaFetchError>
Dangerous raw-byte fallback removed — validated bytes only
Slack call site: aggregated user-facing warning with files:read scope hint
Discord call site: NotAnImage preserves video fallback; other errors log at warn level
13 new unit tests including exact bug reproduction

🟢 INFO — well done

Root cause analysis is precise and the fix targets the exact boundary
Block-list + magic-byte dual-layer is the right trade-off (CDN-safe, Slack-safe)
sanitize_slack_filename prevents mrkdwn injection from user-controlled filenames
Test coverage is thorough — corrupt GIF, HTML-as-PNG, missing Content-Type, truncated headers

🟡 NIT — non-blocking suggestions

3 minor suggestions (click to expand)

Block-list extension — validate_image_response rejects text/* early. Consider also blocking application/xhtml+xml for early reject. Magic bytes will still catch it, but an explicit block avoids the ImageReader overhead for known non-image MIME families.
Discord ProcessingFailed path — The ProcessingFailed doc comment says "callers should surface the same user-facing warning as InvalidImageBody", but the Discord call site only logs at warn level with no user-facing message. This is likely intentional (Discord URLs are signed-public, so the Slack scope hint doesn't apply), but a brief code comment explaining the intentional divergence would help future readers.
Slack warning specificity — The aggregated warning message covers all failure reasons in one sentence ("This can happen when the bot lacks files:read... or the file format isn't supported... or the file is too large"). For SizeExceeded, the parenthetical (exceeds N byte limit) in the file list helps. For ProcessingFailed, the generic scope hint is misleading. Consider splitting the warning into scope-related vs format-related buckets if the failed_image_files list grows categories.

Tests

PR reports 24 media tests passed (13 new). Smoke validation command provided in PR description.

Review by 超渡法師 · 四問框架

chaodu-agent · 2026-05-16T17:35:06Z

🔴 SUGGESTED CHANGES — blocking regression identified

Update from initial review: 擺渡法師 (Codex) identified a regression in the ≤1MB fallback path that reopens the same class of JSONL poisoning as #776.

The problem

validate_image_response() for PNG/JPEG/WebP only calls with_guessed_format() — this validates the magic bytes header but does not prove the body is decodable. The latest commit (6d7fd5f) re-introduces the ≤1MB raw-byte fallback:

// src/media.rs ~L272
Err(e) => {
    if bytes.len() <= 1024 * 1024 {
        debug!(filename, error = %e, "resize failed, using validated original");
        (bytes.to_vec(), mime.to_string())  // ← uses Slack metadata MIME, not detected
    } else {
        return Err(MediaFetchError::ProcessingFailed(e));
    }
}

Attack path: A file with a valid PNG signature but truncated/corrupt body (≤1MB) will:

✅ Pass validate_image_response (magic bytes match PNG)
❌ Fail resize_and_compress (body can't decode)
⚠️ Hit the fallback → raw bytes forwarded under Slack's claimed MIME
💀 Model receives invalid ContentBlock::Image → same poisoning as bug(slack): unauthenticated file fetch returns HTML, gets forwarded to model as image, poisons session #776

Suggested fix (pick one)

Option A (preferred): Remove the ≤1MB raw-byte fallback entirely. If resize_and_compress fails, return Err(MediaFetchError::ProcessingFailed(e)) regardless of size.

Option B: Make validate_image_response do a full decode for PNG/JPEG/WebP (not just magic-byte sniff), and use the detected MIME from the decoder rather than Slack metadata for the fallback path.

Required regression test

Add a test case: full PNG signature + truncated body (≤1MB) must NOT produce ContentBlock::Image. It should return InvalidImageBody or ProcessingFailed.

#[test]
fn validate_rejects_truncated_png_body() {
    // Valid PNG signature (8 bytes) + IHDR chunk start but truncated
    let mut truncated_png = vec![0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a];
    truncated_png.extend_from_slice(&[0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52]);
    // No actual image data — just enough to pass magic-byte detection
    let result = validate_image_response(Some("image/png"), &truncated_png);
    // This MUST fail — magic bytes pass but body is not decodable
    assert!(result.is_err());
}

Updated verdict

~~🟢 APPROVE with NITs~~ → 🔴 Requires changes before merge.

The 3 NITs from the initial review remain valid but are non-blocking. This regression is blocking.

Joint review: 超渡法師 + 擺渡法師 (Codex)

chaodu-agent · 2026-05-16T17:35:40Z

Additional findings from 普渡法師 (Claude)

🔴 F2 — `sanitize_slack_filename` missing `&` escape (`src/slack.rs`)

The function escapes `, <, > but not &. Slack mrkdwn decodes HTML entities, so a filename like <@here> would bypass the current </> replacement and render as a <@here> mention ping after Slack's entity decoding.

Fix: Add .replace('&', "&") before the other replacements:

pub(crate) fn sanitize_slack_filename(s: &str) -> String {
    s.replace('&', "&amp;")
     .replace('`', "'")
     .replace('<', "(")
     .replace('>', ")")
}

🟡 F1 — `SizeExceeded` warning shows raw byte count (`src/slack.rs:1066`)

format!("{filename} (exceeds {limit} byte limit)")
// → "photo.png (exceeds 10485760 byte limit)"

Should format as human-readable size (e.g. "10 MB") for better UX.

Findings by 普渡法師 (Claude) · consolidated by 超渡法師

chaodu-agent · 2026-05-16T19:31:31Z

🔴 Consolidated Four-Monk Review — CHANGES REQUESTED

Blocking Issues (must fix before merge)

#	Finder	File	Issue
1	擺渡法師 (Codex)	`src/media.rs:108-120` + `src/media.rs:272-277`	≤1MB fallback regression: `validate_image_response()` only does magic-byte header sniff for PNG/JPEG/WebP (via `with_guessed_format()`), not full decode. The restored ≤1MB fallback forwards raw bytes after `resize_and_compress()` decode failure. A corrupt/truncated body with valid image magic bytes bypasses the decode gate and becomes a model `ContentBlock::Image` — same poisoning class as #776. Fix: Remove the ≤1MB fallback entirely, OR make `validate_image_response` do full decode for PNG/JPEG/WebP.
2	普渡法師 (Claude)	`src/slack.rs` (`sanitize_slack_filename`)	Missing `&` escape: Slack mrkdwn decodes HTML entities, so `<@here>` bypasses the current `<`/`>` replacement and renders as a mention ping. Fix: Add `.replace('&', "&")` before the other replacements.

Required Regression Test

#[test]
fn truncated_png_body_must_not_produce_content_block() {
    // Valid PNG signature (8 bytes) + partial IHDR but truncated — ≤1MB
    let mut truncated = vec![0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a];
    truncated.extend_from_slice(&[0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52]);
    // Must NOT pass validation or produce ContentBlock::Image
    let result = validate_image_response(Some("image/png"), &truncated);
    assert!(result.is_err());
}

Non-blocking NITs (address if convenient)

#	Finder	Issue
N1	超渡法師	Block-list could extend to `application/xhtml+xml` for early reject
N2	超渡法師	Discord `ProcessingFailed` path: add code comment explaining intentional no-user-message divergence
N3	超渡法師	Slack warning message could be more specific per error variant
N4	普渡法師	`SizeExceeded` warning shows raw byte count — format as human-readable (e.g. "10 MB")

Reviewers

超渡法師 (Kiro) — initial review + coordination
擺渡法師 (Codex) — identified blocking regression
普渡法師 (Claude) — identified & escape gap + confirmed regression
覺渡法師 (Gemini) — no response (timed out)

Four-Monk Collaborative Review · PR Review Practice

…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>

howie requested a review from thepagent as a code owner May 11, 2026 11:54

github-actions Bot added pending-screening PR awaiting automated screening closing-soon PR missing Discord Discussion URL — will auto-close in 3 days labels May 11, 2026

howie and others added 3 commits May 11, 2026 20:05

github-actions Bot added pending-maintainer and removed closing-soon PR missing Discord Discussion URL — will auto-close in 3 days labels May 13, 2026

shaun-agent added 2 commits May 13, 2026 10:00

fix(media): validate GIF bodies before pass-through

225ea2f

style(discord): apply rustfmt

63c4b8d

This comment has been minimized.

Sign in to view

chaodu-agent added pending-contributor and removed pending-maintainer labels May 13, 2026

howie and others added 2 commits May 13, 2026 20:56

antigenius0910 mentioned this pull request May 14, 2026

bug(slack): unauthenticated file fetch returns HTML, gets forwarded to model as image, poisons session #776

Open

github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label May 15, 2026

chaodu-agent previously approved these changes May 15, 2026

View reviewed changes

chaodu-agent added pending-maintainer and removed pending-contributor closing-soon PR missing Discord Discussion URL — will auto-close in 3 days pending-screening PR awaiting automated screening labels May 15, 2026

howie mentioned this pull request May 16, 2026

fix(adapters): notify agent of failed image attachments so reply aligns with warning #826

Draft

3 tasks

CHC-Agent dismissed chaodu-agent’s stale review via 6d7fd5f May 16, 2026 13:58

CHC-Agent approved these changes May 16, 2026

View reviewed changes

chaodu-agent added pending-contributor and removed pending-maintainer labels May 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(media): validate Content-Type and magic bytes before sending to model#793

fix(media): validate Content-Type and magic bytes before sending to model#793
howie wants to merge 9 commits into
openabdev:mainfrom
howie:fix/776-validate-fetched-image-bytes

howie commented May 11, 2026 •

edited by shaun-agent

Loading

Uh oh!

howie commented May 11, 2026

Uh oh!

howie commented May 12, 2026

Uh oh!

shaun-agent commented May 13, 2026

Uh oh!

This comment has been minimized.

chaodu-agent left a comment

Uh oh!

CHC-Agent left a comment

Uh oh!

chaodu-agent commented May 16, 2026

Uh oh!

chaodu-agent commented May 16, 2026

Uh oh!

chaodu-agent commented May 16, 2026

Uh oh!

chaodu-agent commented May 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

howie commented May 11, 2026 • edited by shaun-agent Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Changes

Tests

Manual test plan (post-deploy)

Out of scope / follow-ups

At a Glance

Prior Art & Industry Research

Why This Approach

Alternatives Considered

Uh oh!

howie commented May 11, 2026

Codex Challenge Report — Adversarial Review

Finding 1: Slack filename mrkdwn injection [FIXED]

Finding 2: Corrupt GIF bodies pass magic-byte check [Known Issue — not in scope]

Finding 3: failed_image_files Vec is unbounded per event [Acceptable]

No TOCTOU between Content-Type capture and body read

hex_prefix cannot panic

Mixed success: one valid PNG + one HTML file in same message

Uh oh!

howie commented May 12, 2026

Uh oh!

shaun-agent commented May 13, 2026

OpenAB PR-Screening Report

Intent

Feat

Who It Serves

Rewritten Prompt

Merge Pitch

Best-Practice Comparison

Implementation Options

Comparison Table

Recommendation

Uh oh!

This comment has been minimized.

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

CHC-Agent left a comment

Choose a reason for hiding this comment

Uh oh!

chaodu-agent commented May 16, 2026

🟢 APPROVE (with NITs)

What it solves

How it solves it

🟢 INFO — well done

🟡 NIT — non-blocking suggestions

Tests

Uh oh!

chaodu-agent commented May 16, 2026

🔴 SUGGESTED CHANGES — blocking regression identified

The problem

Suggested fix (pick one)

Required regression test

Updated verdict

Uh oh!

chaodu-agent commented May 16, 2026

Additional findings from 普渡法師 (Claude)

🔴 F2 — sanitize_slack_filename missing & escape (src/slack.rs)

🟡 F1 — SizeExceeded warning shows raw byte count (src/slack.rs:1066)

Uh oh!

chaodu-agent commented May 16, 2026

🔴 Consolidated Four-Monk Review — CHANGES REQUESTED

Blocking Issues (must fix before merge)

Required Regression Test

Non-blocking NITs (address if convenient)

Reviewers

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

howie commented May 11, 2026 •

edited by shaun-agent

Loading

🔴 F2 — `sanitize_slack_filename` missing `&` escape (`src/slack.rs`)

🟡 F1 — `SizeExceeded` warning shows raw byte count (`src/slack.rs:1066`)