Skip to content

Commit 1bbe2d5

Browse files
howieclaude
andcommitted
fix(media,slack,discord): pin 4xx+resize-fallback surfaces with tests
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>
1 parent 3a78718 commit 1bbe2d5

3 files changed

Lines changed: 151 additions & 66 deletions

File tree

src/discord.rs

Lines changed: 37 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -739,47 +739,26 @@ impl EventHandler for Handler {
739739
));
740740
}
741741
}
742-
Err(media::MediaFetchError::SizeExceeded { actual, limit }) => {
743-
tracing::warn!(
744-
url = %attachment.url,
745-
filename = %attachment.filename,
746-
actual,
747-
limit,
748-
"image exceeds size limit"
749-
);
750-
failed_image_files.push(format!(
751-
"{} (exceeds {limit} byte limit)",
752-
attachment.filename
753-
));
754-
}
755-
Err(
756-
media::MediaFetchError::UnsupportedResponseType { .. }
757-
| media::MediaFetchError::InvalidImageBody { .. },
758-
) => {
759-
tracing::warn!(
760-
url = %attachment.url,
761-
filename = %attachment.filename,
762-
"image validation failed; body is not a supported image"
763-
);
764-
failed_image_files.push(attachment.filename.clone());
765-
}
766-
Err(media::MediaFetchError::ProcessingFailed(e)) => {
767-
tracing::warn!(
768-
url = %attachment.url,
769-
filename = %attachment.filename,
770-
error = %e,
771-
"image post-processing failed"
772-
);
773-
failed_image_files.push(attachment.filename.clone());
774-
}
775-
// Network/HTTP failures: transient; warn in logs only, don't notify user.
776742
Err(e) => {
777-
tracing::warn!(
778-
url = %attachment.url,
779-
filename = %attachment.filename,
780-
error = %e,
781-
"image attachment failed"
782-
);
743+
if let Some(entry) =
744+
media::failed_attachment_entry(&attachment.filename, &e)
745+
{
746+
tracing::warn!(
747+
url = %attachment.url,
748+
filename = %attachment.filename,
749+
error = %e,
750+
"image attachment failed"
751+
);
752+
failed_image_files.push(entry);
753+
} else {
754+
// Network/HTTP 5xx failures: transient; warn in logs only.
755+
tracing::warn!(
756+
url = %attachment.url,
757+
filename = %attachment.filename,
758+
error = %e,
759+
"image download failed"
760+
);
761+
}
783762
}
784763
}
785764
}
@@ -2246,4 +2225,22 @@ mod tests {
22462225
fn normal_channel_creates_thread() {
22472226
assert!(!should_skip_thread_creation(false, false));
22482227
}
2228+
2229+
// --- failed_attachment_entry parity tests (mirrors media::tests) ---
2230+
2231+
/// HTTP 4xx from Discord CDN (e.g. expired signed URL) must push into
2232+
/// failed_image_files so the user gets a warning and the agent is notified.
2233+
#[test]
2234+
fn discord_failed_attachment_entry_http_4xx_notifies_user() {
2235+
let e = media::MediaFetchError::HttpStatus(reqwest::StatusCode::FORBIDDEN);
2236+
let entry = media::failed_attachment_entry("photo.png", &e).unwrap();
2237+
assert_eq!(entry, "photo.png");
2238+
}
2239+
2240+
/// HTTP 5xx is transient; should not notify the user.
2241+
#[test]
2242+
fn discord_failed_attachment_entry_http_5xx_is_logged_only() {
2243+
let e = media::MediaFetchError::HttpStatus(reqwest::StatusCode::INTERNAL_SERVER_ERROR);
2244+
assert!(media::failed_attachment_entry("photo.png", &e).is_none());
2245+
}
22492246
}

src/media.rs

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,28 @@ pub fn format_failed_attachment_note(filenames: &[String]) -> String {
8383
)
8484
}
8585

86+
/// Returns the entry to push into `failed_image_files` for the given error, or
87+
/// `None` if the error is transient / network-level and should only be logged.
88+
///
89+
/// HTTP 4xx responses are treated as permanent-ish failures (missing scope,
90+
/// revoked credentials, expired URL) and warrant user notification. HTTP 5xx
91+
/// and network errors are transient and are logged only.
92+
pub(crate) fn failed_attachment_entry(filename: &str, e: &MediaFetchError) -> Option<String> {
93+
match e {
94+
MediaFetchError::NotAnImage => None,
95+
MediaFetchError::SizeExceeded { limit, .. } => {
96+
Some(format!("{filename} (exceeds {limit} byte limit)"))
97+
}
98+
MediaFetchError::UnsupportedResponseType { .. }
99+
| MediaFetchError::InvalidImageBody { .. }
100+
| MediaFetchError::ProcessingFailed(_) => Some(filename.to_string()),
101+
MediaFetchError::HttpStatus(status) if status.is_client_error() => {
102+
Some(filename.to_string())
103+
}
104+
_ => None,
105+
}
106+
}
107+
86108
/// Strip MIME parameters and trim whitespace. `"image/png; charset=binary"` → `"image/png"`.
87109
pub(crate) fn strip_mime_params(mime: &str) -> &str {
88110
mime.split(';').next().unwrap_or(mime).trim()
@@ -285,7 +307,19 @@ pub async fn download_and_encode_image(
285307
return Err(e);
286308
}
287309

288-
let (output_bytes, output_mime) = match resize_and_compress(&bytes) {
310+
encode_validated_image(&bytes, mime, filename)
311+
}
312+
313+
/// Resize, compress and Base64-encode bytes that have already passed
314+
/// `validate_image_response`. On resize failure, falls back to the raw
315+
/// validated bytes when the body is ≤ 1 MB (e.g. animated WebP that the
316+
/// image crate cannot decode but is small enough to pass through).
317+
fn encode_validated_image(
318+
bytes: &[u8],
319+
mime: &str,
320+
filename: &str,
321+
) -> Result<ContentBlock, MediaFetchError> {
322+
let (output_bytes, output_mime) = match resize_and_compress(bytes) {
289323
Ok(result) => result,
290324
Err(e) => {
291325
if bytes.len() <= 1024 * 1024 {
@@ -841,4 +875,77 @@ mod tests {
841875
assert!(note.contains("`fi'le.png`"));
842876
assert!(!note.contains("``"));
843877
}
878+
879+
// --- encode_validated_image: resize-fallback threshold tests ---
880+
881+
/// Minimal WebP header stub: RIFF magic + WEBP FourCC.
882+
/// `validate_image_response` accepts it (format detected as WebP);
883+
/// `resize_and_compress`'s `decode()` fails because there are no VP8 chunks.
884+
/// Generated inline — no copyright concern.
885+
fn make_webp_stub() -> Vec<u8> {
886+
b"RIFF\x00\x00\x00\x00WEBP".to_vec()
887+
}
888+
889+
#[test]
890+
fn resize_fail_under_1mb_falls_back_to_original_bytes() {
891+
let bytes = make_webp_stub(); // 12 bytes << 1 MB
892+
let result = encode_validated_image(&bytes, "image/webp", "test.webp");
893+
let block = result.expect("should fall back to original bytes, not error");
894+
match block {
895+
ContentBlock::Image { media_type, data } => {
896+
assert_eq!(media_type, "image/webp");
897+
// Data is Base64 of the original stub bytes.
898+
let decoded = BASE64.decode(&data).expect("valid Base64");
899+
assert_eq!(decoded, bytes);
900+
}
901+
other => panic!("expected ContentBlock::Image, got {other:?}"),
902+
}
903+
}
904+
905+
#[test]
906+
fn resize_fail_over_1mb_returns_processing_failed() {
907+
let mut bytes = make_webp_stub();
908+
// Pad to just over 1 MB so the >1 MB branch fires.
909+
bytes.resize(1024 * 1024 + 1, 0x00);
910+
let result = encode_validated_image(&bytes, "image/webp", "big.webp");
911+
assert!(
912+
matches!(result, Err(MediaFetchError::ProcessingFailed(_))),
913+
"expected ProcessingFailed for >1MB resize failure, got {result:?}"
914+
);
915+
}
916+
917+
// --- failed_attachment_entry tests ---
918+
919+
#[test]
920+
fn failed_attachment_entry_not_an_image_returns_none() {
921+
let e = MediaFetchError::NotAnImage;
922+
assert!(failed_attachment_entry("img.png", &e).is_none());
923+
}
924+
925+
#[test]
926+
fn failed_attachment_entry_size_exceeded_includes_limit() {
927+
let e = MediaFetchError::SizeExceeded { actual: 20_000_000, limit: 10_000_000 };
928+
let entry = failed_attachment_entry("big.png", &e).unwrap();
929+
assert_eq!(entry, "big.png (exceeds 10000000 byte limit)");
930+
}
931+
932+
#[test]
933+
fn failed_attachment_entry_http_4xx_notifies_user() {
934+
let e = MediaFetchError::HttpStatus(reqwest::StatusCode::FORBIDDEN);
935+
let entry = failed_attachment_entry("photo.png", &e).unwrap();
936+
assert_eq!(entry, "photo.png");
937+
}
938+
939+
#[test]
940+
fn failed_attachment_entry_http_5xx_is_logged_only() {
941+
let e = MediaFetchError::HttpStatus(reqwest::StatusCode::BAD_GATEWAY);
942+
assert!(failed_attachment_entry("photo.png", &e).is_none());
943+
}
944+
945+
#[test]
946+
fn failed_attachment_entry_http_401_notifies_user() {
947+
let e = MediaFetchError::HttpStatus(reqwest::StatusCode::UNAUTHORIZED);
948+
let entry = failed_attachment_entry("secret.png", &e).unwrap();
949+
assert_eq!(entry, "secret.png");
950+
}
844951
}

src/slack.rs

Lines changed: 6 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,32 +1048,13 @@ async fn handle_message(
10481048
extra_blocks.push(block);
10491049
}
10501050
Err(media::MediaFetchError::NotAnImage) => {}
1051-
Err(media::MediaFetchError::SizeExceeded { actual, limit }) => {
1052-
warn!(filename, actual, limit, "image exceeds size limit");
1053-
failed_image_files.push(format!("{filename} (exceeds {limit} byte limit)"));
1054-
}
1055-
Err(
1056-
media::MediaFetchError::UnsupportedResponseType { .. }
1057-
| media::MediaFetchError::InvalidImageBody { .. },
1058-
) => {
1059-
warn!(
1060-
filename,
1061-
"image validation failed; server may have returned non-image content"
1062-
);
1063-
failed_image_files.push(filename.to_string());
1064-
}
1065-
Err(media::MediaFetchError::ProcessingFailed(ref e)) => {
1066-
warn!(filename, error = %e, "image post-processing failed");
1067-
failed_image_files.push(filename.to_string());
1068-
}
1069-
Err(media::MediaFetchError::HttpStatus(status))
1070-
if status.is_client_error() =>
1071-
{
1072-
warn!(filename, %status, "image download denied");
1073-
failed_image_files.push(filename.to_string());
1074-
}
10751051
Err(e) => {
1076-
warn!(filename, error = %e, "image download failed");
1052+
if let Some(entry) = media::failed_attachment_entry(filename, &e) {
1053+
warn!(filename, error = %e, "image attachment failed");
1054+
failed_image_files.push(entry);
1055+
} else {
1056+
warn!(filename, error = %e, "image download failed");
1057+
}
10771058
}
10781059
}
10791060
}

0 commit comments

Comments
 (0)