Skip to content

Commit 5dbc423

Browse files
committed
fix(media): address reviewer findings openabdev#1 openabdev#3 openabdev#5 openabdev#7
- openabdev#1: add url_expires param to url_hint_block and download_and_read_text_file; Discord caller passes true so the hint warns the agent that CDN URLs expire in ~24 hours; Slack passes false - openabdev#3: document in function doc that Slack URL hints are visibility-only for most agents since they do not hold the bot token - openabdev#5: replace hardcoded '512 KB' string in hint text with TEXT_INLINE_LIMIT / 1024 so the string stays in sync with the constant - openabdev#7: separate TEXT_INLINE_LIMIT doc comment from download_and_read_text_file doc comment; remove stray blank line between doc and fn that triggered clippy::empty_line_after_doc_comments Add test: url_hint_block_with_expiry_includes_expiry_note verifies the 24-hour expiry warning appears when url_expires is true
1 parent dfcfab6 commit 5dbc423

3 files changed

Lines changed: 97 additions & 35 deletions

File tree

crates/openab-core/src/discord.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,7 @@ impl EventHandler for Handler {
931931
&attachment.filename,
932932
u64::from(attachment.size),
933933
None,
934+
true, // Discord CDN URLs expire after ~24 hours
934935
)
935936
.await
936937
{

crates/openab-core/src/media.rs

Lines changed: 94 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -461,24 +461,6 @@ pub fn is_text_file(filename: &str, content_type: Option<&str>) -> bool {
461461
TEXT_FILENAMES.contains(&filename.to_lowercase().as_str())
462462
}
463463

464-
/// Download a text-based file and return it as a `ContentBlock::Text`.
465-
///
466-
/// Files at or below 512 KB are downloaded and inlined into the prompt verbatim.
467-
///
468-
/// Files above 512 KB are **not** downloaded; instead a URL-hint block is
469-
/// returned so the agent can fetch the content itself if it has a web-fetch
470-
/// tool available. The reported byte count in the hint path is `0` — the
471-
/// file is not inlined, so it does not contribute to the caller's aggregate
472-
/// size cap.
473-
///
474-
/// Pass `auth_token` for platforms that require per-request authentication
475-
/// (e.g. Slack private files). When `needs_auth` is `true` the hint block
476-
/// will include a note that the URL requires an `Authorization` header, so the
477-
/// agent is aware that a bare fetch will likely fail.
478-
///
479-
/// Note: the caller already guards total size via a 1 MB aggregate cap; the
480-
/// per-file `MAX_SIZE` check here is intentional defense-in-depth so this
481-
/// function remains self-contained and safe when called from other contexts.
482464
/// Maximum size for inlining a text file into the prompt.
483465
///
484466
/// Files at or below this limit are downloaded and embedded verbatim.
@@ -489,22 +471,46 @@ pub fn is_text_file(filename: &str, content_type: Option<&str>) -> bool {
489471
/// duplicating the magic number.
490472
pub const TEXT_INLINE_LIMIT: u64 = 512 * 1024; // 512 KB
491473

474+
/// Download a text-based file and return it as a `ContentBlock::Text`.
475+
///
476+
/// Files at or below [`TEXT_INLINE_LIMIT`] are downloaded and inlined into
477+
/// the prompt verbatim.
478+
///
479+
/// Files above [`TEXT_INLINE_LIMIT`] are **not** downloaded; instead a
480+
/// URL-hint block is returned so the agent can fetch the content itself if it
481+
/// has a web-fetch tool available. The reported byte count in the hint path
482+
/// is `0` — the file is not inlined, so it does not contribute to the
483+
/// caller's aggregate size cap.
484+
///
485+
/// Pass `auth_token` for platforms that require per-request authentication
486+
/// (e.g. Slack private files). When provided, the hint block will include a
487+
/// note that the URL requires an `Authorization` header. Note that for Slack,
488+
/// this hint is **visibility-only** for most agents — the URL cannot be
489+
/// fetched without the bot token, which the agent does not hold.
490+
///
491+
/// Note: the caller already guards total size via a 1 MB aggregate cap; the
492+
/// per-file `MAX_SIZE` check here is intentional defense-in-depth so this
493+
/// function remains self-contained and safe when called from other contexts.
492494
pub async fn download_and_read_text_file(
493495
url: &str,
494496
filename: &str,
495497
size: u64,
496498
auth_token: Option<&str>,
499+
url_expires: bool,
497500
) -> Option<(ContentBlock, u64)> {
498501
const MAX_SIZE: u64 = TEXT_INLINE_LIMIT;
499502

500503
if size > MAX_SIZE {
501504
tracing::info!(
502505
filename,
503506
size,
504-
"text file exceeds 512KB inline limit; returning URL hint for agent-side fetch"
507+
"text file exceeds inline limit; returning URL hint for agent-side fetch"
505508
);
506509
let needs_auth = auth_token.is_some();
507-
return Some((url_hint_block(filename, url, size, needs_auth), 0));
510+
return Some((
511+
url_hint_block(filename, url, size, needs_auth, url_expires),
512+
0,
513+
));
508514
}
509515

510516
let mut req = HTTP_CLIENT.get(url);
@@ -538,10 +544,13 @@ pub async fn download_and_read_text_file(
538544
tracing::info!(
539545
filename,
540546
size = actual_size,
541-
"downloaded text file exceeds 512KB inline limit; returning URL hint"
547+
"downloaded text file exceeds inline limit; returning URL hint"
542548
);
543549
let needs_auth = auth_token.is_some();
544-
return Some((url_hint_block(filename, url, actual_size, needs_auth), 0));
550+
return Some((
551+
url_hint_block(filename, url, actual_size, needs_auth, url_expires),
552+
0,
553+
));
545554
}
546555

547556
// from_utf8_lossy returns Cow::Borrowed for valid UTF-8 (zero-copy)
@@ -567,22 +576,42 @@ pub async fn download_and_read_text_file(
567576
///
568577
/// `needs_auth` should be `true` for platforms (e.g. Slack) whose download
569578
/// URLs require an `Authorization: Bearer` header — the hint will include a
570-
/// note so the agent knows a bare GET will be rejected.
571-
fn url_hint_block(filename: &str, url: &str, size: u64, needs_auth: bool) -> ContentBlock {
579+
/// note so the agent knows a bare GET will be rejected. For Slack this hint
580+
/// is visibility-only for most agents since they do not hold the bot token.
581+
///
582+
/// `url_expires` should be `true` for platforms (e.g. Discord CDN) that
583+
/// issue time-limited signed URLs — the hint will warn the agent to fetch
584+
/// promptly before the link expires.
585+
fn url_hint_block(
586+
filename: &str,
587+
url: &str,
588+
size: u64,
589+
needs_auth: bool,
590+
url_expires: bool,
591+
) -> ContentBlock {
572592
let size_kb = size / 1024;
573-
let auth_note = if needs_auth {
574-
"\nNote: this URL requires an Authorization header to download \
593+
let inline_limit_kb = TEXT_INLINE_LIMIT / 1024;
594+
let mut notes = String::new();
595+
if url_expires {
596+
notes.push_str(
597+
"\nNote: this URL is time-limited (Discord CDN links expire in approximately \
598+
24 hours — fetch promptly).",
599+
);
600+
}
601+
if needs_auth {
602+
notes.push_str(
603+
"\nNote: this URL requires an Authorization: Bearer header to download \
575604
(the platform uses authenticated file storage). A bare HTTP GET will likely \
576-
return 401 Unauthorized."
577-
} else {
578-
""
579-
};
605+
return 401 Unauthorized.",
606+
);
607+
}
580608
ContentBlock::Text {
581609
text: format!(
582610
"[File: {filename}]\n\
583-
This file ({size_kb} KB) exceeds the 512 KB inline limit and was not downloaded \
584-
by the bot. If you need its contents, fetch the URL below using a web-fetch tool:\n\
585-
{url}{auth_note}"
611+
This file ({size_kb} KB) exceeds the {inline_limit_kb} KB inline limit and was not \
612+
downloaded by the bot. If you need its contents, fetch the URL below using a \
613+
web-fetch tool:\n\
614+
{url}{notes}"
586615
),
587616
}
588617
}
@@ -595,7 +624,13 @@ mod tests {
595624

596625
#[test]
597626
fn url_hint_block_no_auth_contains_url_and_size() {
598-
let block = url_hint_block("big.txt", "https://example.com/big.txt", 600 * 1024, false);
627+
let block = url_hint_block(
628+
"big.txt",
629+
"https://example.com/big.txt",
630+
600 * 1024,
631+
false,
632+
false,
633+
);
599634
let text = match block {
600635
ContentBlock::Text { text } => text,
601636
_ => panic!("expected ContentBlock::Text"),
@@ -619,6 +654,7 @@ mod tests {
619654
"https://files.slack.com/report.txt",
620655
540 * 1024,
621656
true,
657+
false, // Slack URLs do not have a TTL
622658
);
623659
let text = match block {
624660
ContentBlock::Text { text } => text,
@@ -634,6 +670,29 @@ mod tests {
634670
);
635671
}
636672

673+
#[test]
674+
fn url_hint_block_with_expiry_includes_expiry_note() {
675+
let block = url_hint_block(
676+
"results.txt",
677+
"https://cdn.discordapp.com/results.txt",
678+
600 * 1024,
679+
false,
680+
true, // Discord CDN URLs expire
681+
);
682+
let text = match block {
683+
ContentBlock::Text { text } => text,
684+
_ => panic!("expected ContentBlock::Text"),
685+
};
686+
assert!(
687+
text.contains("24 hours"),
688+
"expiry note must mention 24-hour window"
689+
);
690+
assert!(
691+
!text.contains("Authorization"),
692+
"no auth note for unauthenticated URLs"
693+
);
694+
}
695+
637696
/// Regression test for the aggregate-cap pre-check bug.
638697
///
639698
/// A file larger than `TEXT_INLINE_LIMIT` must always produce a URL hint,
@@ -670,6 +729,7 @@ mod tests {
670729
"https://cdn.discordapp.com/large-results.txt",
671730
large_size,
672731
false,
732+
true, // Discord CDN URLs expire
673733
);
674734
let text = match block {
675735
ContentBlock::Text { text } => text,

crates/openab-core/src/slack.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1370,7 +1370,8 @@ async fn handle_message(
13701370
continue;
13711371
}
13721372
if let Some((block, actual_bytes)) =
1373-
media::download_and_read_text_file(url, filename, size, Some(bot_token)).await
1373+
media::download_and_read_text_file(url, filename, size, Some(bot_token), false)
1374+
.await
13741375
{
13751376
if text_file_bytes + actual_bytes > TEXT_TOTAL_CAP {
13761377
debug!(

0 commit comments

Comments
 (0)