feat(media): give audio and Slack video a URL the agent can actually fetch

[ShinyChang]

What problem does this solve?

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

Two inbound attachment classes never reach the agent in a form it can act on. One
delivers nothing at all, the other delivers a link the agent cannot open. Both break the
same rule: an attachment block is only worth emitting if the agent can actually fetch
what it points at.

Audio delivers nothing. The audio branch is a dead end on all three inbound paths.
With STT enabled the agent gets a transcript and nothing else; with STT disabled it gets
nothing at all (Discord/Slack add a 🎤 reaction and drop the file, the gateway logs at
debug! and drops it). The original file never reaches the agent either way, so a skill
cannot do anything a transcript cannot already express: no diarisation, no timeline, no
re-transcription with a domain glossary, no local ASR tuned for a non-English language.

Slack video delivers an unusable link. slack.rs special-cases video inside the
NotAnImage arm:

if media::is_video_file(filename, Some(mimetype)) {
    // [Video attachment] ... url: {url}   <- raw url_private_download
} else {
    // download_and_upload_any_file -> filestore presigned URL
}

slack_file_download_url returns url_private_download (or url_private), both of
which require an Authorization: Bearer <bot token> header that the agent does not
hold. So the video branch steps around filestore to emit a URL that 403s, with no note
explaining why, while the sibling else branch for PDF/ZIP does the right thing. The
agent cannot tell a dead link from a live one, so it fails silently. Discord is
unaffected (cdn.discordapp.com needs no credentials) and the gateway has no video
branch at all, so this is Slack-only.

Together these are the last two attachment classes without a working passthrough. Images
get an [Image attachment] block with a URL, text files are inlined or uploaded, and
since #738 binary files (PDF/ZIP/docx) get a [File: ...] block with a presigned URL.

The concrete case: a user drops a meeting recording (.m4a) into Slack and asks the agent
for minutes with speakers and action items. A transcript alone cannot produce that, and the
agent has no way to reach the file. Drop an .mp4 in the same thread and the agent
receives a link it will fetch and fail on.

Refs #738 (established the presigned-URL passthrough pattern this extends),
Refs #251 (established the current 🎤 reaction behaviour, which this keeps).

Review Contract

Goal

Every inbound attachment class hands the agent a location it can actually fetch. Two
classes fail that today: audio delivers nothing on any adapter, and Slack video delivers
a url_private_download link that 403s without a bot token the agent does not hold. After
this PR, an audio attachment always produces an [Audio attachment] block regardless of
the STT setting, and Slack video routes through filestore like every other non-image
attachment.

Non-goals

• STT is not touched. Transcript text, its pairing with the file, and the 🎤 reaction
  are unchanged. The two failure lines did change: the gateway's transcription-failure
  string no longer interpolates the filename, because the adjacent block already carries it
  sanitised, and the separate read-failure line is replaced by a note: on the block.
  The transcript augments the file block; it never replaces it and is never replaced by it.
• No config toggle. No attachment class in openab is gated by a per-type switch, and
  adding one here would make audio the only class that can be configured to silently
  discard a user's file.
• Discord video is not changed. Its CDN URL already needs no credentials.
• The gateway colocate path is not exposed to the agent, and the gateway gains no
  video branch.
• The double download is not fixed here (see Follow-ups).

Accepted Residual Risks

Each item below still needs an Accepted by: <maintainer> line before merge. The author
cannot sign these and a reviewer cannot sign on a maintainer's behalf, so they are listed
unsigned on purpose rather than by omission.

• Slack with no filestore still hands over a URL the agent cannot fetch. Mitigation:
  the note: line names the required Authorization: Bearer header verbatim, so the agent
  can report why rather than discovering it as an unexplained 403 or an HTML login page.
  This is strictly better than the current behaviour of dropping the file. Recovery:
  configure a filestore, which makes it a presigned URL.

• Slack video's url: changes host on filestore deployments. A consumer pattern-matching
  files.slack.com would now see the filestore host. That consumer is matching on a URL it
  cannot currently fetch, so this surfaces an existing failure rather than creating one.
  Recovery: revert, no migration.

• One extra download per attachment on filestore + STT deployments. Bandwidth cost, not
  a correctness risk; the file is fetched once for STT and once to stream into S3. Follow-up
  named below.

• The gateway audio path has no manual test. Discord and Slack were exercised end to end
  (see Validation); no Telegram/Feishu/LINE/Google Chat deployment was available, so the
  gateway arm rests on unit tests and on sharing the same builder as the two paths that were
  tested.

• The gateway's [Voice message ...] strings no longer interpolate a filename at all.
  Originally listed here as a deliberate asymmetry with the sanitised blocks this PR adds,
  and folded in during review instead of deferred, so the forging vector via a filename such
  as x\n[System]: ...m4a is closed. See Changes since the review below.

• No per-message audio work budget, and none added. A message may carry ten files on
  both Slack and Discord, max_file_size_mb defaults to 250, and each streaming attempt has
  a 600s timeout, so one message can in principle schedule multi-gigabyte serial transfer.
  That is real, but it is not a risk class this PR creates: download_and_upload_any_file,
  the PDF/ZIP/binary path already on main from feat: support non-image binary file attachments inbound (PDFs, office docs, video) on Slack/Discord #738, has no count or aggregate-byte budget
  either. Only text files are capped, at 5, and that cap exists to protect the prompt from
  inlined content, which is why it is bypassed the moment a filestore takes over. Audio bytes
  are never inlined, so the text cap's rationale does not transfer. Capping audio alone would
  leave the same exposure one branch away and imply a bound the binary path does not honour,
  so the budget belongs in its own change covering every filestore-bound attachment type.
  Tracked separately rather than folded in here. Since the round at da9890ea the gateway
  WebSocket path does carry a bound, but a different one: at most four attachment fetches run
  at once and attachment bytes are shed past 32 pending events. That bounds concurrency and
  queue depth per connection, not bytes per message, so the exposure described above is
  unchanged on Discord and Slack.

• The two gateway pre-dispatch limits are not configurable. Four concurrent attachment
  fetches and 32 pending events are compile-time constants. An operator who reaches the second
  one sees attachment bytes stop being fetched with no knob to turn. Deliberate: they are safety
  valves against a slow store, and a config key would invite raising them back toward the
  unbounded behaviour they exist to prevent. Documented in docs/inbound-attachments.md.
  Recovery: the limits release themselves as the queue drains, and nothing is persisted.

Acceptance Criteria

• An audio attachment produces an [Audio attachment] block on Discord, Slack, and the
  gateway with STT both enabled and disabled. With STT enabled the transcript rides on
  top of that block when transcription succeeds; when it fails, Discord and Slack emit
  the block alone and the gateway adds [Voice message - transcription failed].
• With a filestore configured, the url: line is a presigned URL that resolves without
  credentials; with none, it is the platform URL plus a note: naming its requirement,
  or absent entirely on the gateway.
• A Slack video attachment's url: resolves without credentials when a filestore is
  configured.
• Discord's [Video attachment] output is byte-identical to main for every filename
  and content type main itself would emit. The shared builder diverges in five ways,
  each one deliberate: it strips control and separator characters from the filename,
  truncates the filename at 200 chars, renders a filename made entirely of stripped
  characters as unnamed, restricts the MIME to [A-Za-z0-9/-+.;= ] and truncates it
  at 100 chars, and renders an empty MIME as unknown. A crafted filename or an exotic
  content type therefore yields different, safer output.
  Pinned by the_video_block_matches_main_except_where_sanitizing_makes_it_differ,
  which compares against a main_video_block reference reproducing main's format
  string, plus the_video_block_truncates_a_name_at_200_and_a_mime_at_100 and
  the_video_block_substitutes_unknown_for_an_empty_mime_and_strips_quoting.
• The #738 binary-file path ([File: ...]) is byte-identical to main, including its
  three degraded hint strings.
• A message carrying two voice notes renders each transcript immediately before its own
  [Audio attachment] block, on Discord, Slack, and the gateway.
• On the gateway WebSocket path, a voice note and the text sent after it reach the
  dispatcher in that order even when the voice note's upload is slow, and a /reset sent
  while one is still being prepared drops it instead of letting it open the new session.
  Pinned by same_thread_events_reach_the_dispatcher_in_arrival_order and
  a_message_being_prepared_when_reset_arrives_is_dropped.
• cargo clippy -p openab-core -- -D warnings is clean with and without the filestore
  feature, and cargo test is green in both configurations.

Follow-ups

• Single-download refactor of the Discord/Slack STT path. download_and_transcribe owns
  its own download, while stt::transcribe already accepts bytes, as the gateway path shows.
  Downloading once and feeding both removes the extra fetch and makes the two adapters match
  the gateway's shape. Deferred because it is a change to the STT path, which this PR
  deliberately does not touch.
• A gateway video branch. The gateway has no [Video attachment] handling at all;
  whether it should upload video bytes to filestore the way it now does for audio is a
  separate design question.
• The [File: ...] binary path still says nothing on TooLarge or DownloadFailed.
  Audio now explains both conditions in a note:, so a 300 MB .m4a and a 300 MB .zip in
  one message get an explanation and total silence respectively. Closing it means changing
  the #738 path, which AC-5 deliberately holds byte-identical, so it belongs in its own
  change.
• The five remaining .filter(|c| !c.is_control()) call sites. They cover C0/C1 only, so
  the separators and bidi formatters splits_a_prompt_line handles survive them. Four feed
  the #738 blocks that AC-5 pins byte-identical; converting them is a behaviour change to
  that path and is scoped out for the same reason.
• store.rs's size ceiling disagrees with the STT ceiling. The two constants differ (20
  vs 25 MB) with no comment explaining which one an operator is actually bounded by. Not
  touched here because neither value changes in this PR.
• A harness for the adapters' attachment loops. Extracting the url/note/size decision into
  attachment_url_note_size and giving each adapter a row test closes the mutation that
  rewrites the Slack fallback arm, and makes both note constants assertable. Three of the four
  mutations the review listed survive regardless, because they delete or rewrite statements
  inside EventHandler::message and handle_message themselves: dropping the
  extra_blocks.extend(...) call, passing None instead of Some(bot_token) to the Slack
  upload, and swapping stt_line = Some(...) for extra_blocks.insert(0, ...). Reaching those
  needs a fixture that drives a whole handler with a fake platform client, which does not exist
  in this crate today and is a larger change than this PR.
• The post-response failure classification is pinned, but its wiring is not. The new test
  drives StreamUploadCause through the classifier, AudioStoreError, and the note text for
  all three causes, which is the mutation that matters. It does not cover the one line inside
  download_and_presign_any_file that calls the classifier, and an integration test proving a
  genuinely interrupted body stream produces SourceRead needs a filestore test double:
  Filestore wraps a concrete aws_sdk_s3::Client, so nothing can inject a failing upload
  today. That double is its own change.
• The unified entry point has no equivalent ordering ticket. main.rs already wraps each
  process_gateway_event in its own tokio::spawn, so same-thread ordering and the /reset
  boundary there rest on tokio's scheduling exactly as they did on main. Giving that path the
  same ticket is a change to code this PR does not otherwise touch, and the gap is pre-existing
  rather than introduced here.
• The Discord [Image attachment] block now sanitises but has no test of its own. It is
  built inline in the message handler rather than by a pure builder, so the coverage it has
  is sanitize_attachment_meta's own unit tests. Extracting it the way audio and video were
  extracted is the follow-up.

Changes since the review at cd38142

Behaviour change: Discord and Slack audio block order. media::audio_attachment_blocks
now owns the order of one attachment's blocks and all three adapters extend from it.
Discord and Slack previously inserted the transcript at index 0 of extra_blocks from
inside the loop over attachments, so three voice notes rendered as
[t3, t2, t1, m1, m2, m3]: transcripts reversed and detached from the files they describe.
Order is now transcript then metadata, per attachment.

• Who is affected: anyone sending more than one audio attachment in one Discord or Slack
  message, or audio alongside another attachment type. A single audio attachment on its own
  is byte-identical to before, which is why this order was chosen over metadata-first.
• Gateway: the transcript now precedes the metadata block, where it previously followed.
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Behaviour change: audio classification. media::audio_mime falls back to the filename
extension when the platform omits the MIME, so a clip.ogg or meeting.m4a arriving with an
empty or application/octet-stream type now enters the audio branch. It previously fell
through to the image path and was dropped, or became a generic [File: ...] block.

• Who is affected: Discord and Slack messages carrying audio the platform did not type.
  Anything already labelled audio/* is unchanged. STT now runs on these attachments where
  it did not before, so a deployment with STT enabled will see transcripts appear for files
  that were previously silent.
• Why a MIME and not a bool: stt::transcribe builds its multipart body with
  Part::mime_str(...).ok()?, which discards the request when the value does not parse.
  Admitting an attachment whose MIME is "" would have traded "audio silently dropped" for
  "audio always fails to transcribe".
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Fix: gateway audio content type. Filestore::upload_and_presign hardcoded
text/plain; charset=utf-8, so the same .m4a was stored as audio/mp4 from Slack and
text/plain from the gateway platforms. An earlier revision fixed this by adding a required
content_type argument, which was a compile break for anyone outside this crate, since
openab-core carries no publish = false and pub mod filestore is exported. The
two-argument method is restored with its original text/plain; charset=utf-8 behaviour, and
the new upload_and_presign_with_content_type mirrors the argument stream_upload_and_presign
already took. Everything this branch newly added for the adapters is pub(crate). One
pre-existing pub item was narrowed alongside them, which was the same mistake in
miniature; that is reverted below.

Fix: presigned_ttl rendering. Every site rendered the lifetime as ttl / 60, so
anything under 60s reported "expires in 0 minutes". An earlier revision fixed this with a
60-second floor, which silently lengthened a configured authorization lifetime and had no
business riding along with attachment passthrough. The configured value is now passed through
untouched and the shared formatter reports seconds below a minute. The pre-existing 7-day cap
is unchanged, and at 60s and above the formatter emits exactly the string ttl / 60 produced,
so the #738 hint stays byte-identical at every TTL that path could already render.

Closed from Follow-ups. The four unsanitised [Voice message ...] filename
interpolations are gone. Two were the redundant read-failure line, dropped; the other two
no longer name the file, because the adjacent metadata block already carries it sanitised.

Changes since the review at a44e8ea

Fix: Discord no longer erases a DownloadFailed. When a configured filestore failed
because the bot's own fetch of the CDN link failed, Discord fell back to that same link
carrying its ordinary "expires ~24h" note, telling the agent to fetch a URL the bot had just
proved it could not fetch. DownloadFailed is now the one store outcome that overrides the
platform's "the agent can fetch this" claim. TooLarge and UploadFailed still fall back
quietly, because the CDN link genuinely works in those cases.

Fix: the measured byte count wins over the platform's advisory size. size_bytes: was
taken from the platform's reported size even when the file had just been streamed through the
filestore and counted on the way. Discord's size is advisory and Slack's can be absent, so
download_and_presign_attachment now returns a StoredAttachment carrying the count it
measured, and the block renders that.

New: one decision function for the url / note / size row. media::attachment_url_note_size
takes the store outcome plus a PlatformUrl saying whether the agent can fetch the platform's
own link unaided, and returns the three values a block renders with. Both adapters route
through it, and each carries a test walking every row (Ok, TooLarge, UploadFailed,
DownloadFailed, no filestore) with no S3 client involved. DISCORD_CDN_NOTE and
SLACK_URL_REQUIREMENT moved to module scope so those tests can pin them literally; before
the move, emptying either constant left the whole suite green.

Fix: audio_mime compared a half-normalised MIME. Audio/OGG read as an explicit
non-audio type and suppressed the extension fallback, so a correctly typed file was dropped
by its capitalisation. The value is lower-cased once, up front.

Hardening: the sibling blocks that still interpolated raw metadata. Both gateway
[System: attachment "..." was not delivered ...] sites now share one
undelivered_attachment_line builder that sanitises through sanitize_attachment_meta, and
Discord's [Image attachment] block sanitises the same way. The gateway line was the sharper
case, since a filename carrying a newline could forge a second [System: ...] line. Slack
also now skips a file with no private URL instead of emitting a block with an empty url:.

• Who is affected: these two blocks exist on main and passed the filename and MIME
  through raw, so this is a behaviour change outside the audio and video blocks the rest of
  the PR adds. A filename or content type containing control or separator characters now
  renders stripped, a filename longer than 200 chars renders truncated, one made entirely of
  stripped characters renders as unnamed, and a MIME outside [A-Za-z0-9/-+.;= ] renders
  filtered and truncated at 100. Ordinary filenames are byte-identical to before.
• Why not left as a follow-up: the PR's own argument for sanitising the audio and video
  blocks is that attachment metadata reaches the prompt attacker-controlled. That argument
  does not stop at the two blocks this PR happens to add, and the gateway line is the sharper
  case of the two.
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Tests: the paths the review showed were unpinned. An stt_on_unreachable() fixture points
STT at a closed port, so the gateway's STT-failure arm runs deterministically with no network
and no mock; restoring the filename to that failure string now fails a test. The video block
gained a parity table against a main_video_block reference that reproduces main's format
string, so every divergence is declared rather than assumed, plus the 200-char and 100-char
truncation boundaries. A MIME-forging test pins what the allow-list actually protects: an
attacker-supplied content_type cannot add a line or a second url:, though it does survive
as inert text.

Docs: the turn-boundary ADR's voice-only scenario. Adding a metadata block changed the
block count the ADR's rollback hatch keys on: with STT a voice-only arrival is two blocks so
extra_blocks.len() == 1 && prompt.is_empty() never fires, and without STT the single block
is the metadata, which the hatch would promote into the prompt slot. Section 3.1, the Scenario
D worked example (now shown for both STT states), and the hatch's two other citations now
identify the transcript block explicitly. ADR bumped to 0.7 with a changelog entry.

Docs: drift outside the diff. telegram.toml and line.toml still described core's only
audio action as transcription, while their four siblings were updated in this PR;
line.toml's voice_stt also contradicted line.md. filestore.md's size-refusal row
claimed a note Discord never emits, and listed two Future Directions that ship here.
slack.md's files:read row omitted video and binaries. discord.toml claimed non-image
files are warned to the user, when the warning fires only for images that failed to download.
Two code comments were wrong in the same way: audio_attachment_block's said a gateway url
is always None, and gateway_audio_blocks's misnamed the drift that motivated it (both
copies emitted the read-failure block; what one copy dropped was the logging).

Changes since the review at 471cd64b

Three of the four findings from that round are fixed here. The fourth is a scheduling decision
and is stated as open below rather than answered quietly.

Fix: the public MIME helper is public again. is_audio_mime was pub at the merge base,
media is a pub mod, and the crate carries no publish = false, so narrowing it to
pub(crate) was a compile break for an external consumer even though its behaviour never
changed. It is public again at its original signature, with the doc pointing adapters at
audio_mime, which is the one that also reads the extension when a platform sends a missing or
generic type. Worth being explicit that this reverses a narrowing an earlier round asked for:
that request and the compatibility request from the round before it were in conflict, and this
resolves it on the compatibility side. Nothing this branch newly added is public.

Fix: a post-response failure names the component actually at fault. The prechecks correctly
separate a non-2xx response, a Content-Length overrun, and a reported-size overrun. After
bytes_stream() starts, though, two more things can go wrong: the platform's body stream can
fail, and the bytes actually read can overrun max_file_size, which is the only authoritative
measurement. Both returned a bare anyhow::Error and both were flattened into
PresignError::UploadFailed, so the agent read the upload did not complete when the truth was
either that the platform withheld the bytes or that the file was over the limit. That points
diagnosis at the wrong component, and it is worst on exactly the chunked or mis-sized files the
measured-size handling exists for.

stream_upload_and_presign is itself pub, so changing its return type would have repeated the
first finding. Instead a StreamUploadCause now rides in the error chain and
presign_error_for_upload classifies with downcast_ref. The cause is the source rather than
the outer context, so every Display string and every log line is byte-identical to before.
Four sites are tagged, including stream produced no data, whose own comment already said it
indicates a download failure. A test drives all three causes through the classifier and on to
the note the agent reads.

• Who is affected: anyone with a filestore configured whose platform truncates a body
  mid-transfer or under-reports a size. They previously got a note naming the store; they now
  get one naming the platform or the size cap. No successful path changes.
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Fix: the presigned_ttl docs state the zero-second exception. cap_presigned_ttl raises a
configured 0 to 1 because S3 rejects X-Amz-Expires=0 outright, and its unit test pins
that. Two references still said the configured value is never raised. Both now name the
exception and why it exists. This is a doc correction, not a behaviour change: normalising zero
was deliberate, since a URL that cannot work at all is worse than one that expires immediately.

Fix: object-storage transfer no longer runs inside the gateway receive loop. In
run_gateway_adapter the attachment loop sat inside the msg = ws_rx.next() arm of the select,
with tasks.spawn after it, so while gateway_audio_blocks awaited an upload the socket read
nothing else. Two details made it worse than it first looked. On main, with STT disabled, that
arm was a single debug!, so for a filestore deployment with STT off this was a new blocking
remote call rather than an inherited one. And the slash-command handling sat after the loop, so a
/cancel arriving behind a large upload waited on that upload too, which is precisely when a
user would send one.

The loop is now a named assemble_attachment_blocks awaited inside the spawned per-event work.
The receive arm keeps only the two clones that work needs. The unified entry point was never
affected, because main.rs already wraps each process_gateway_event in its own tokio::spawn,
but it shares the helper anyway: the two inline copies had already drifted, one logging a
rejected attachment and the other logging an unreadable text file, with neither logging both.
The shared version logs both.

• Who is affected: any gateway deployment with a filestore configured. A slow or unavailable
  store no longer stalls receipt of later events. Slash commands now short-circuit before
  attachment assembly, so an attachment riding on a /cancel is no longer uploaded and then
  discarded. There is no reaction to re-time on this path; the 🎤 reaction is Discord's, not the
  gateway's. One log line is new on the WebSocket path, the text_file read failure that only
  the unified copy used to report.
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Tests the extraction made possible. The loop was inline in two entry points and had no test
at all; naming it is what allowed two. One drives a mixed list (a rejected attachment, an audio
attachment, an unrenderable sticker) and pins that arrival order survives and that a type with no
branch contributes nothing rather than an empty block. The other pins that a rejected attachment
is reported with no filestore in play at all.

On the regression test the finding asked for, proving that a stalled upload cannot stall the
loop: that one is still not deliverable, for the reason in Follow-ups. Filestore wraps a
concrete aws_sdk_s3::Client, so nothing can inject an upload that never returns. The scheduling
seam is the part that was deliverable, and it is now explicit rather than implied.

Changes since the review at da9890ea

All four findings from that round are fixed. Two of them are this branch's own doing, one round
old, and are described that way below rather than as neutral improvements.

Fix: receipt order and reset boundaries survive delayed assembly. Moving attachment fetches
into the spawned per-event task, the previous round's fix, did keep object storage off the
receive path. It also removed what made three properties true for free, because serial execution
on that path was itself the mechanism:

• Same-thread events reached the dispatcher in arrival order. submit now sits after an await
  that can run for the streaming timeout, so a voice note could be overtaken by the text sent
  after it, and the dispatcher's per-thread queue would take them in that order. dispatch.rs
  states no merging, splitting, or reordering as a broker invariant, and the batching ADR's
  prohibited-transformations list names ordering inversion among the things the broker is not
  authorised to do.
• /reset only had to cancel buffered messages, because nothing else was in flight. A message
  still being assembled would submit into the session created after the reset.
• Exactly one fetch ran at a time, so nothing needed a concurrency limit.

PreDispatchOrder restores the first two. A ticket is taken on the receive path, in arrival
order, carrying the session generation it was taken in. It waits for its predecessor immediately
before the dispatcher handoff, never before the fetch, which is the part meant to run
concurrently, and then drops itself if /reset has bumped the generation since. Dropping a
ticket releases its successor, so a cancelled or panicking task cannot wedge a thread, and idle
threads are swept once more than 256 are tracked.

Bounding is three things: a semaphore of four concurrent fetches, try_join_next reaping on
every event rather than only at shutdown and reconnect, and load shedding past 32 pending events.
Shedding drops the bytes, not the message: the agent still gets the event, carrying the same
[System: attachment ...] line a platform-side rejection produces with the limit named as the
reason. Losing a user's text to load shedding is worse than losing the file attached to it.

Both limits are compile-time constants with no config key, documented in
docs/inbound-attachments.md as safety valves rather than tuning knobs, and the ADR's ordering
clause now names the ticket instead of leaving serial execution implied.

• Who is affected: any gateway deployment. Ordering and reset behaviour return to what they
  were before the previous round's commit. New under load only: at 32 pending events attachment
  bytes stop being fetched, which no deployment reaches without a store that is already failing
  slowly. A message dropped by /reset while being prepared is logged at info! and is not
  counted in the reset reply's Dropped n buffered message(s), which still counts buffered
  messages only.
• Rollback: revert the commit. No config, schema, or wire-format change is involved.

Tests. same_thread_events_reach_the_dispatcher_in_arrival_order drives the actual failure:
the first event blocks on a stand-in for the fetch, the second runs, and the test asserts nothing
has reached the dispatcher before the first is released. Neutering wait_for_turn fails it.
a_message_being_prepared_when_reset_arrives_is_dropped pins the generation check in both
directions, including that another thread's reset is not this thread's business.
a_dropped_event_releases_the_next_one pins the no-wedge property,
a_second_thread_is_not_held_behind_the_first that the order is per thread, and
an_idle_thread_stops_being_tracked the sweep. The shed decision and its block are pinned by
attachment_work_is_shed_only_once_the_queue_is_full and
a_shed_attachment_still_tells_the_agent_what_arrived, and
the_order_key_matches_the_one_reset_scopes_to pins the one string the two halves have to agree
on.

The regression test for the original stall, that a hung upload cannot stall the receive loop, is
still not deliverable for the reason in Follow-ups: Filestore wraps a concrete
aws_sdk_s3::Client.

Fix: a real extension is required before classifying audio by name. rsplit('.').next()
returns the whole string when there is no dot, so a file literally named mp3 matched the
extension fallback and was handed to STT on its name alone. rsplit_once plus a non-empty stem
makes the dot required. A test covers mp3, wav, ogg, .mp3, and . alongside the positive
cases. Three other rsplit('.') call sites in media.rs have the same shape; all three are
pre-existing on main and none is on this branch's path, so they are left alone.

• Who is affected: an extension-less file whose whole name happens to be an audio extension.
  It previously reached STT and the audio block; it now falls through to normal type handling.
• Rollback: revert the commit.

Base moved: upstream/main merged in. The LINE WORKS adapter (#1456) landed while this
round was in progress and conflicted in docs/inbound-attachments.md, which is why the diff now
shows a row for a platform this PR never touched. The support matrix keeps this branch's
file + STT wording and its Slack video row, and takes upstream's new LINE WORKS row with the
same wording applied: LINE WORKS forwards audio as an audio attachment through the gateway, so
it goes through exactly the arm this PR changed and gets the metadata block whether or not STT is
on. LINE WORKS is also added to the audio URL table's gateway enumeration, so the two tables
agree. Everything else in that commit merged cleanly; the only source overlap was one line in
gateway.rs adding lineworks to NON_EDITABLE_PLATFORMS.

Fix: the turn-limit doc is back above its own constant. Hoisting DISCORD_CDN_NOTE to module
scope two rounds ago inserted it between MAX_CONSECUTIVE_BOT_TURNS and the doc comment
describing it, so the bot-turn cap read as documentation for the CDN note. Comment move only.

Changes since the review at 8c4b0769

One finding, and it is the other half of the round before it. The ticket introduced then fenced
/reset against work still being prepared, but the fence was a single check taken immediately
before the dispatcher handoff, which left two holes.

Fix: the fence covers the handoff instead of preceding it. Dispatcher::submit parks when the
thread's queue is full. A /reset landing during that park drops the consumer, so the parked send
returns SendError, and submit transparently retries it onto a consumer it creates fresh, which
belongs to the session the reset just started. A message admitted before the reset could therefore
be the one that opened the session after it, which is the opposite of what the reset was asked to
do. The generation now lives in a watch channel, so a ticket parked in the handoff is told about
the reset rather than only being able to look before it starts waiting, and the handoff future is
abandoned when that happens. Abandoning is safe: a parked mpsc send has enqueued nothing, and a
send that already completed lands on the pre-reset consumer, which the reset aborts anyway.

Fix: a reset detaches the events that follow it. Bumping the generation left the discarded
events chained ahead of later ones, so the first message of a new session waited for a discarded
upload to finish before it could reach the dispatcher, up to the streaming timeout. reset now
clears the thread's tail as well, so post-reset events start a fresh chain.

• Who is affected: any gateway deployment where a /reset can race an attachment upload. Both
  holes need a reset concurrent with in-flight preparation, so neither is reachable without one.
  No successful path changes, and no config, schema, or wire-format change is involved.
• Rollback: revert the commit.

Tests. a_reset_during_a_parked_handoff_abandons_the_message drives the retry race with a
handoff that never resolves, standing in for the parked send, and asserts the reset releases it;
removing the reset branch from the handoff hangs it until the timeout.
a_post_reset_event_does_not_wait_for_pre_reset_work holds a pre-reset ticket open and asserts the
post-reset event has no predecessor and does not wait; leaving the tail attached fails it.
a_reset_before_the_handoff_abandons_the_message and a_handoff_that_lands_first_counts_as_submitted
pin the two non-racing outcomes.

The documented /reset guarantee in docs/inbound-attachments.md is updated in the same push to
state what the fence now actually covers, rather than the weaker property it described before.

Changes since the review at 487e59d8

Two findings, both in the gateway pre-dispatch scheduling, both fixed.

Fix: a reset cancels the whole of a discarded event's preparation. The fence added in the
previous round covered the dispatcher handoff and nothing before it, so an event the reset had
already invalidated went on holding a fetch slot, uploading bytes nobody would read, and could
still reach create_thread and create a forum topic. The first event of the new session then
queued behind exactly that work at fetch_slots.acquire(), which is the opposite of the guarantee
this PR's documentation claimed. The fence now wraps the whole spawned body, so cancelling returns
the fetch slot and the source budget at once and stops the side effects before they happen.

One edge remains rather than being claimed away: a remote call already in flight when the reset
lands, a forum-topic creation whose request has left the process, may still take effect on the
platform. Cancellation stops the broker from acting on the result, not the platform from having
received the request. That is stated in docs/inbound-attachments.md.

Fix: an admitted attachment is read before its event queues. The fetch slot was acquired before
the colocated source was read, and the gateway store evicts media 120 seconds after it lands,
sweeping every 30. Four stalled uploads could therefore hold a fifth event past its source's
lifetime, and when a slot finally opened the read failed, so the agent got a read-failure block for
an attachment that existed when the event arrived. This sat well under the 32-event shedding
threshold, so nothing announced it. Sources are now read at admission, ahead of the queue.

Holding those bytes is what the new admitted-source budget bounds: 256 MiB across admitted events,
measured at this point against the platform's declared sizes, which the next round replaces with the
bytes actually held. Over that budget an event is
admitted without its attachment bytes, taking the same undelivered line the pending-event limit
produces, rather than being queued. Reading before queueing would otherwise trade a read failure
for unbounded memory, which is not a trade worth making silently.

• Who is affected: any gateway deployment. The reset behaviour changes only when a reset races
  in-flight preparation. The source read is unconditional but strictly earlier than before, so the
  only new cost is the memory the budget bounds. No config, schema, or wire-format change.
• Rollback: revert the commit.

Tests. a_reset_releases_the_fetch_slot_the_new_session_needs reproduces the reviewer's case:
a stale task holds the only slot, the reset lands, and the test asserts the slot comes back;
fencing only the handoff leaves it held until the test times out.
a_reset_stops_the_work_before_any_of_it_runs asserts the body never starts, which is what makes
the forum-topic side effect unreachable. an_admitted_attachment_survives_a_source_that_expires_while_it_queues
reads the source, deletes the file, asserts the deletion really took (or the test would prove
nothing), and then asserts the assembled block is the attachment rather than a read failure;
reading inside assembly again turns it into a read failure. the_source_budget_bounds_what_admitted_events_may_hold
and an_abandoned_task_returns_its_source_budget pin the budget and its release on cancellation.

A note on the shape of this PR. This is the fourth consecutive round spent on gateway
pre-dispatch scheduling, all of it downstream of one decision: moving attachment assembly off the
WebSocket receive loop. Each round has found a real defect in that machinery, and the machinery now
runs to a ticket, a generation watch, a cancellation fence, a fetch semaphore, a byte budget, and a
shedding policy, none of which the audio and video contracts this PR exists for actually need. The
alternative that was available at the time, and still is, is to put assembly back on the receive
path and bound the stall with a much shorter timeout: that makes ordering, reset, and boundedness
correct by construction, at the cost of leaving the receive-loop finding only partly addressed. It
is worth a maintainer's explicit call whether this scheduling work should stay here or land as its
own change, rather than continuing to accrete inside an attachment-contract PR.

Changes since the review at 9c83db99

Two findings, both mine, both introduced by the previous round's budget work. Both fixed.

Fix: the source budget is charged for bytes actually held. The 256 MiB budget reserved against
attachment.size, the size the platform declares in the event. A gateway platform is free to
under-report it, and an attachment declaring size: 0 reserved nothing and then read its file in
full, so the budget bounded a number rather than the memory it exists to bound. Enough of them and
the limit is decorative. Reservations are now taken against an upper bound derived from the source
itself: fs::metadata for a colocated path, or the length base64 can decode to for inline data. The
read is then capped at what was reserved (AsyncReadExt::take), so even an upper bound that is
wrong cannot overshoot the reservation.

An attachment the budget refuses now renders as its own [System: attachment ... was not delivered ...] line, naming the budget, instead of a read failure. Nothing failed to read, and telling the
agent otherwise sends it to retry a fetch that was never attempted.

Fix: the rejection reason cannot restructure the prompt line. undelivered_attachment_line
sanitized the filename and the MIME type beside it, and then interpolated the reason verbatim. The
reason is attacker-controlled on the same footing: telegram.rs builds it as
unsupported format: {ext}, straight from the filename extension, so a crafted filename could put
line breaks or bidi overrides into a [System: ...] line the agent reads as broker-authored. The
filename sanitizer is now a reusable fragment helper (sanitize_prompt_fragment) and every
untrusted piece of that line, reason included, goes through it.

• Who is affected: any gateway deployment. No config, schema, or wire-format change. The budget
  now refuses per attachment rather than per event, so a message whose second attachment is refused
  still delivers its first. Rollback: revert the commit.

Tests. an_under_reported_size_cannot_bypass_the_source_budget sends two attachments declaring
size: 0 whose real bytes each exceed half the budget; charging the declared size again lets the
second through. the_source_budget_bounds_what_is_retained and
an_abandoned_task_returns_its_source_budget pin the limit and its release.
a_refused_source_tells_the_agent_rather_than_claiming_a_read_failure pins the wording split.
a_rejection_reason_cannot_restructure_the_prompt_line feeds a reason carrying \n[System]: and a
bidi override; interpolating verbatim fails it.
a_reason_made_only_of_stripped_characters_still_reads_as_a_reason covers the empty-after-stripping
case, so the line never degrades into a dangling dash.

The limits table in docs/inbound-attachments.md is corrected in the same push: it described the
budget as measured against advisory declared sizes, which this change makes false.

Changes since the review at 7c0bc5f3

One finding, mine, introduced with the budget itself. Fixed. The budget covered the source buffers and
nothing built from them, so it stopped bounding memory at the point where the memory actually grew.
Three distinct gaps, all real:

Fix: the source moves into its block instead of being cloned. assemble_attachment_blocks took
&[Result<Vec<u8>, _>] and opened with bytes.clone(), putting a second copy of every attachment
outside the reservation. It now takes the sources by value and moves each one into its block. The
copy is gone by construction rather than by convention: the signature no longer offers a borrow to
clone from.

Fix: the reservation covers the block, not just the source. An image holds its source and the
base64 encoding of that source at the same time, and base64 spends four characters on every three
bytes, so the peak was about 2.3x what was charged. A text file holds its source and the code block
wrapping it. Reservations are now source + inline_payload_bytes(type, source), so the peak is what
gets charged. Audio and video are unchanged and charge nothing extra: they carry a URL and metadata
whatever their source weighs.

Fix: the queued payload is bounded. The guards were scoped to the assembly branch, so the
reservation was released the moment the blocks existed, and those blocks then sat in the dispatcher
queue. That queue is bounded by max_buffered_messages (10 per thread) and not by bytes, so nothing
bounded it in size at all. Guards are now held for the whole task, and a per-message cap bounds the
handover: 24 MiB of inlined payload, past which the attachment is described rather than inlined,
taking the same not delivered line the other limits produce.

The two byte limits deliberately cover different lifetimes, which the limits table in
docs/inbound-attachments.md now states: the 256 MiB budget covers preparation and is returned at
submit, the per-message cap covers what the dispatcher then holds.

• Who is affected: any gateway deployment. No config, schema, or wire-format change. A message
  inlining more than 24 MiB now describes the overflow instead of inlining it; nothing else changes
  shape. Rollback: revert the commit.

Tests. an_image_reserves_what_its_encoded_block_will_hold gives an image exactly enough budget
for its source and asserts it is still refused, with a positive control at the full bound so the test
cannot pass vacuously; charging the source alone fails it.
a_near_limit_image_is_described_rather_than_inlined sets the cap to exactly one encoded block and
asserts the second image comes back as a not delivered line rather than an image; dropping the cap
fails it. the_inline_budget_admits_exactly_the_limit pins the arithmetic and the saturating
boundary.

One caveat stated rather than claimed away: the guard-lifetime change is a scoping change at a single
call site inside the spawned per-event task, and is verified by reading rather than by a test. The
unit tests drive read_attachment_sources and assemble_attachment_blocks directly, so they cannot
observe where that call site binds its guards.

Changes since the review at 6e07f6fb

Four findings. Three fixed, one fixed in part with the remainder stated plainly below rather than
implied away.

Fix (F1, memory half): a shed attachment releases its bytes. Shedding described the attachment
from metadata but left the payload in the event the spawned task then captured, so an event shed
precisely because the broker was over its limit went on holding those bytes through an unbounded wait
for its turn. The limit named memory and bounded none of it. shed_attachment_payload now builds the
description and drops the bytes on the receive path, before any task can capture them.

Not fixed (F1, admission half), and this is the honest part: the number of spawned tasks is still
unbounded. Bounding it means choosing one of two things the review cannot choose on the author's
behalf. Either the broker drops user messages under load, which is a product decision with its own
impact statement, or the design changes from one task per event to one consumer per thread draining a
queue, which is a redesign of this scheduler rather than a fix to it. What the shed fix does buy is
that a parked task's cost is now metadata and prompt text rather than attachment bytes, so the
failure mode is bounded in kind even where it is not bounded in count. This belongs with the scope
question already open below.

Fix (F3): the inline cap charges only what reaches the prompt. The cap ran before the text branch
decided whether a filestore would take the file, so a large text file delivered as a URL spent an
allowance the outgoing message never used, and a later image was refused for space that was in fact
free. The delivery decision is now made first, in goes_to_filestore, and the charge follows it.

Fix (F2): an upload reserves for the copy it makes. retained_upper_bound treated audio as
source-only, but ByteStream::from(data.to_vec()) builds a second full buffer while the original is
still alive for STT. Text above the inline limit takes the same path. Both now reserve for source
plus copy, so concurrent uploads cannot sit outside the 256 MiB bound.

Fix (F4): the gateway video contract says one thing. The pre-dispatch section claimed gateway
audio and video carry URL metadata. Gateway video is rejected before Core sees it and has no branch
in the assembly match at all, which the Unsupported Types section already said. Only audio is
claimed now, and the surrounding text spells out which bytes each of the two byte limits counts.

• Who is affected: any gateway deployment. No config, schema, or wire-format change. A shed
  message is unchanged in what the agent sees and only changes in what the broker keeps. Deployments
  with a filestore will find large text files no longer consume the inline allowance, which admits
  strictly more attachments than before. Rollback: revert the commit.

Tests. a_shed_event_carries_no_attachment_bytes asserts both halves, that the agent still gets
a description naming the limit and that the payload is gone; keeping the payload fails it.
a_filestore_moves_the_charge_from_the_prompt_to_the_upload pins the delivery-mode split at the
inline limit and the doubled reservation for an upload; dropping the upload charge fails it, and so
does charging externalized text against the inline cap, independently.

One test the review asked for is not here: a filestore-enabled mixed text-plus-image integration
test. Filestore is a live S3 client with no test double in this repo, so the reachable equivalent
is the pure decision the integration test would be exercising, which is what the test above covers.
Building an S3 double is worth doing and is worth doing outside this PR.

Changes since the review at 0587c93b

Two findings, both fixed. The second is the admission bound left open last round.

Fix: the encoded input is charged and then released. An attachment can arrive as base64 on the
event rather than as a colocated path, and serde allocates that string when the event is parsed.
Nothing charged it and nothing freed it, so a 20 MiB inline attachment carried roughly 26.7 MiB of
base64 through assembly and dispatcher handoff, entirely outside the budget that claimed to bound
attachment memory. A refused attachment kept it as well, which is precisely backwards: the refusal
existed to avoid holding those bytes. read_attachment_sources now takes the input off the event with
mem::take before decoding, on every path including both refusal paths, and the reservation covers
it alongside the buffer decoded from it.

Fix: admission is bounded. Past 256 events in preparation the broker refuses the event and tells
the sender to send it again, rather than admitting work it has no way to bound.

The trade-off is worth stating rather than burying, because there was no option without one.
Bounding admission means either refusing work or stalling the socket, and stalling the socket takes
/cancel down with it, which is the exact failure this whole path was built to avoid. Refusing is
visible to the sender and recoverable by them in a way that a queue growing until the process dies is
not. The limit is a compile-time constant with no config key, consistent with the others here: 256
events in flight is a load problem to report, not a number to tune.

• Who is affected: any gateway deployment. Under normal load nothing changes, since 256 events in
  simultaneous preparation is far past ordinary traffic. Under sustained overload a sender now gets a
  refusal notice instead of the broker silently accumulating work. No config, schema, or wire-format
  change. Rollback: revert the commit.

Tests. the_reservation_covers_the_encoded_input_and_then_frees_it gives an attachment exactly
enough budget for its decoded buffer and asserts it is still refused, then asserts the input is gone
from the event on both the refused and admitted paths; not charging the input fails it, and so does
keeping it on the event, independently. admission_stops_at_the_limit pins the boundary.
an_image_reserves_what_its_encoded_block_will_hold and
an_under_reported_size_cannot_bypass_the_source_budget were updated to the new peak arithmetic and
still falsify what they were written for.

Correcting the previous section. The note under 6e07f6fb above said the unbounded task count
was left to a maintainer decision. That is no longer accurate: the policy is chosen and implemented
here. The section stays as the record of what was true at that commit.

Changes since the review at a90bb6cc

Four findings, all fixed. Two of them are on the unified ingress path, which had none of the controls
the WebSocket path has accumulated.

Fix: unified ingress shares one set of limits. The bridge spawns a task per event and
process_gateway_event constructed SourceBudget::new(MAX_ADMITTED_SOURCE_BYTES) inside each one, so
every concurrent event could independently reserve the entire 256 MiB. A per-event budget is
arithmetically the same as no budget. The budget, a fetch semaphore, and an admission counter now
live on GatewayIngressLimits, held by the shared event context, which is built once and cloned per
event. The refusal reply is the same one the WebSocket path sends.

Fix: control commands run before any attachment work. The unified path read and assembled
attachments before checking /reset, /cancel, and config commands, so a /cancel carrying a voice
note could upload to object storage and run transcription before returning without dispatching
anything. Commands are now handled first, matching the WebSocket path.

Fix: text is charged what it renders to. String::from_utf8_lossy spends a three-byte
replacement character on every malformed byte, so a 20 MiB invalid-UTF-8 text file passed the 24 MiB
inline cap and produced roughly a 60 MiB block. Validity is checked with str::from_utf8 before the
conversion, so the answer is known without allocating the expansion it predicts, and the charge
follows it. Anything charged before its bytes are read now assumes the worst case, which is the only
safe assumption available at that point.

Fix: a truncated read is a failure, not an attachment. file.take(limit).read_to_end returns a
prefix as success, so a colocated file replaced between the metadata read and the data read was
delivered truncated with nothing to say so. The read now looks one byte past the reservation and
fails if it finds one.

• Who is affected: unified deployments gain limits they did not previously have, so under
  concurrent load they now refuse rather than reserving without bound; a control command carrying
  audio stops doing discarded work. Valid text files are charged exactly as before. No config,
  schema, or wire-format change. Rollback: revert the commit.

Tests. malformed_text_is_charged_for_what_it_renders_to pins the lossy split and the worst-case
pre-read charge. a_source_that_grew_after_admission_is_not_delivered_as_a_prefix writes a file,
reserves its length, grows it, and asserts the read fails.
unified_ingress_limits_are_shared_not_per_event and unified_admission_releases_its_slot prove a
clone sees the same budget and the same admission counter. Each of the three corresponding falsifiers
fails only its own test.

One gap, stated rather than glossed: the control-command ordering has no runtime test. Proving it
needs a GatewayEventContext, which means adapter, dispatcher, and router doubles that this module's
test suite does not have, and building the first such fixture is a larger piece of work than the fix
it would cover. The fix itself is a reordering in which each command branch returns before any
attachment code is reachable.

At a Glance

                       BEFORE                                   AFTER

  Discord / Slack                                Discord / Slack
    audio attachment                               audio attachment
      |                                              |
      +-- STT on  --> [Voice transcript]             +-- STT on  --> [Voice transcript]
      |                                              |
      +-- STT off --> 🎤 reaction, file dropped      +-- STT off --> 🎤 reaction
                                                     |
                                                     +-> [Audio attachment]      <-- always
                                                          filename / type / size
                                                          url: presigned (filestore)
                                                               or platform URL

  Gateway (Telegram/Feishu/LINE/GChat)           Gateway
    audio attachment (bytes, no URL)               audio attachment (bytes, no URL)
      |                                              |
      +-- STT on  --> [Voice transcript]             +-- STT on  --> [Voice transcript]
      |                                              |
      +-- STT off --> debug! log, dropped            +-> [Audio attachment]      <-- always
                                                          url: presigned (filestore)
                                                          or metadata only + note

  Slack video                                    Slack video
    [Video attachment]                             [Video attachment]
      url: url_private_download                      url: presigned (filestore)
           (403s without a bot token)                     or url_private_download
      (no note)                                      note: how to reach the url

The transcript is unchanged in content and position. The audio block is additive. The
Slack video block keeps its shape and gains a working url plus a note line; see the
impact statement under Why this approach?.

Discord video is untouched: its URL already needs no credentials, so it passes None for
the note and its emitted text stays byte-identical (asserted by
video_attachment_block_omits_note_line_when_none).

Prior Art & Industry Research

Both reference projects treat inbound audio as a transcription source only: the agent sees
text, never the file. OpenClaw goes further and deletes the media after the preflight pass.
Neither has a "STT is off, so hand the agent the file instead" path, so this is a gap rather
than a solved problem with a house style to copy.

Project / SDK	Mechanism	What the agent receives	Behaviour when STT is off / fails
OpenClaw (extensions/slack/src/monitor/message-handler/preflight-audio.ts, docs/nodes/audio.md)	Per-channel audio preflight before mention detection; a provider/CLI ladder transcribes, then the body is replaced with an [Audio] block	Transcript text only. resolveSlackPreflightAudioTranscript returns { transcript, mediaIndex }, no path or bytes	Returns null and the message falls back to text-only mention detection. discardSlackPreflightMedia then fs.rms the downloaded media, so the file is unrecoverable
Hermes Agent (agent/transcription_provider.py, agent/transcription_registry.py)	Provider registry dispatching to local/Groq/OpenAI backends behind one envelope contract	Transcript text only. The success envelope is {"success": true, "transcript": ...}; there is no field for the source file	Returns {"success": false, "transcript": "", "error": ...}. No fallback that surfaces the audio itself
openab today	media::is_audio_mime branch in each adapter	Transcript only	🎤 reaction (Discord/Slack) or a debug! log (gateway); file dropped
openab, this PR	Same branch, plus an unconditional metadata block reusing the #738 filestore path	Transcript (when enabled) and an [Audio attachment] block with a fetchable URL	The metadata block is emitted either way; STT is orthogonal

OpenClaw: transcript only, and it actively destroys the file.
preflight-audio.ts
runs a per-channel audio preflight before mention detection;
resolveSlackPreflightAudioTranscript returns { transcript, mediaIndex } with no path or
bytes, and docs/nodes/audio.md
confirms the message body is replaced with an [Audio] block. When transcription fails it
returns null and discardSlackPreflightMedia calls fs.rm on the download, so the file is
unrecoverable. There is no "STT is off, hand over the file" path.

Hermes Agent: transcript only, with no field for the source file.
agent/transcription_provider.py
defines a provider registry over local/Groq/OpenAI backends behind one envelope; success is
{"success": true, "transcript": ...} and failure is
{"success": false, "transcript": "", "error": ...}. Neither shape can carry the audio, so
there is no fallback that surfaces it.

Other references: openab's own #738 is the closest prior art and is in-repo. It resolved
the same question for binary files by rejecting raw platform URLs in favour of filestore
presigned URLs; docs/filestore.md records why, naming PR #1346's raw-URL hint and its
limitations. This PR applies that resolution to audio and Slack video rather than reopening it.

What we learn: the transcript-only shape is the industry default, and it is precisely why both
projects must treat "STT unavailable" as "message unavailable". Decoupling the file from the
transcript removes that coupling, and openab is well placed to do it because #738 already built
the credential-free URL mechanism that OpenClaw and Hermes lack.

Proposed Solution

An [Audio attachment] block is emitted for every audio attachment, on every adapter,
independent of the STT setting. Shape matches the existing [Image attachment] /
[Video attachment] blocks:

[Audio attachment]
filename: meeting.m4a
content_type: audio/mp4
size_bytes: 8342016
url: https://<bucket>.s3.<region>.amazonaws.com/incoming/meeting.m4a?X-Amz-Signature=...
note: presigned URL, expires in 60 minutes

Which URL the agent gets follows the #738 precedent, filestore first with a per-platform
fallback:

Adapter	Filestore configured	No filestore
Discord	presigned S3 URL	attachment.url, note records the ~24h CDN expiry
Slack	presigned S3 URL	url_private_download, note records that a Bearer bot token is required
Gateway	presigned S3 URL (uploads the bytes it already holds)	no url line, note points at filestore

The gateway asymmetry is structural, not an oversight: openab-gateway's Attachment
carries base64 data or a colocate path and never a platform URL, because the gateway
already consumed the platform credential during download. The colocate path is deliberately
not exposed to the agent either, since store.rs evicts it after 120s and it would be a
dead path by the time most skills fetch it.

Slack video takes the same route. It now tries filestore first and falls back to
url_private_download with a note naming the bearer-token requirement, so the agent is
never handed a link without being told what it needs. Discord video is unchanged.

Implementation:

• media::audio_attachment_block() and media::video_attachment_block(), two shared
  builders over one sanitize_attachment_meta() helper. Filename and MIME are sanitised
  (control characters stripped, capped at 200/100 chars) before entering the prompt, per the
  same rule the filestore hints follow. The video builder replaces a private copy in
  discord.rs and an inline format! in slack.rs that had already drifted apart.
• media::download_and_presign_attachment() and media::upload_bytes_and_presign(), for
  callers that build their own block (URL-based adapters and the gateway's in-memory bytes
  respectively). Both return the presigned URL paired with its note, built by one private
  presigned_note(). The five call sites would otherwise each rebuild the same
  "presigned URL, expires in N minutes" wording, which is the drift this PR is removing
  elsewhere.
• download_and_upload_any_file() is refactored onto a shared download_and_presign_any_file()
  core returning Result<_, PresignError>. Its three degraded hint strings are preserved
  verbatim, so the feat: support non-image binary file attachments inbound (PDFs, office docs, video) on Slack/Discord #738 PDF/ZIP path is byte-identical.
• The gateway's two "audio" if stt_config.enabled arms plus their fallthrough arms collapse
  into one "audio" arm, so the passthrough cannot be forgotten on one of the two paths.

Not changed: transcript text and its pairing with the file, EchoEntry behaviour, the
🎤 reaction, Discord video, and every remaining attachment branch.

Why this approach?

Why unconditional rather than behind a config flag. No attachment class in openab is
gated by a per-type toggle: FilestoreConfig (config.rs:149) has no enable_* field, and
no per-attachment-type passthrough setting exists anywhere in the config tree. The only knob has always been whether a filestore is
configured, which this change honours. A toggle here would make audio the one class that can
be configured to silently discard a user's file, which is the behaviour #251 was filed to fix.

Impact statement (behaviour change on upgrade). Two changes, of different kinds.

Audio is purely additive. Every deployment that receives audio sees one additional text
block per audio attachment, roughly 40 to 60 tokens, the same order as the existing
[Image attachment] block. Nothing that exists today changes.

Slack video is a modification of an existing block, and is the only place where output
a deployment already depends on will differ. Concretely:

	Before	After
Slack + filestore	url: is url_private_download, 403s for the agent	url: is a presigned URL that resolves, plus a note: line
Slack, no filestore	url: is url_private_download, no explanation	same URL, plus a note: naming the bearer-token requirement
Discord	unchanged	unchanged, byte-identical for filenames main would emit, asserted by test

Anything parsing the block by line prefix keeps working, since note: is appended after
url: and the four existing lines keep their order and names. The risk is a consumer that
pattern-matches the URL for a files.slack.com host, which would now see the filestore
host instead. That consumer is currently matching on a URL it cannot fetch, so this is
the failure surfacing rather than a new one. There is no config, schema, or wire-format
change either way, no new failure mode (every failure path falls back to a less specific
block rather than an error), and rollback is a revert with no migration.

Dual-path validation is under Validation: the suite is green both with and without the filestore
feature, and the no-filestore path is the one that exercises the platform-URL fallback.

On the opt-out that the behaviour-change policy asks for. Raising it rather than hoping it
goes unnoticed: this PR ships no new flag. The reasoning is that the policy's stated purpose
is that "every existing deployment switches behavior on upgrade", and here the Slack video
half is a bug fix rather than a default flip. The behaviour it replaces is a URL that returns
403 to the agent every time, so an opt-out would be an option to keep a link that never
worked, and no deployment can be depending on it succeeding.

What does exist, and is validated, is the degradation path: a deployment with no filestore
keeps url_private_download exactly as today and gains only the additive note: line. That
is the "old behaviour is still reachable" property the policy is protecting, and the suite is
green in that configuration. If a maintainer reads the rule more strictly than that, the
cheapest resolutions are to split the video half into its own PR where it can carry a flag,
or to gate it behind [slack] video_via_filestore defaulting to false and flip in a
follow-up. Say which you prefer and it will be done rather than argued.

Accepted limitation: a second download on filestore deployments. With STT enabled and a
filestore configured, Discord and Slack now download the file twice, once inside
download_and_transcribe and once to stream into S3. The cause is that
download_and_transcribe owns its download; stt::transcribe itself already takes bytes, as
the gateway path shows, so the fix is to download once and feed both. That is a change to the
STT path and this PR deliberately does not touch STT, per one-concern-per-PR. Named follow-up:
have the Discord/Slack audio branch download once and call upload_bytes_and_presign +
stt::transcribe on the same buffer, which also lets the two adapters share the gateway's
shape. The gateway path already has no double download.

On the error shape of the two new presign helpers. They return
Result<(String, String), AudioStoreError>, not the Option their main siblings use.
An earlier revision of this PR matched the neighbours and was wrong to: collapsing a size
refusal, a failed platform download and a failed upload into one None is what let the
gateway report a configured-but-broken filestore as a missing one. The typed error is what
lets each note name the cause that actually occurred. download_and_upload_any_file keeps
its Option signature, so the #738 path is untouched.

Alternatives Considered

Metadata block with the platform URL only, no filestore (PRD "Option A"). Smallest diff,
about 5 lines per adapter, but it does not work where it matters most. Slack's
url_private_download needs a Bearer token, and putting a bot token in the agent's prompt is
the security regression already argued against in #738. It also cannot serve the gateway,
which holds no platform URL at all.

docs/filestore.md already records this judgement: it names PR #1346's raw-URL hint and lists
exactly these limitations (Discord CDN URLs expiring in ~24h, Slack's url_private_download
requiring "a Bearer token the agent does not have"). This PR does not relitigate that. The
filestore presigned URL is the primary path on all three adapters, and the platform URL appears
only as a labelled degradation for deployments with no filestore, where the alternative is the
current behaviour of dropping the file silently. The note field states the applicable caveat
verbatim, so an agent can tell a fetchable URL from one it will be refused, rather than
discovering it through an unexplained 403 or an HTML login page.

Filestore upload only (PRD "Option B"). Clean and credential-free, and the right answer
when a filestore exists, but it silently does nothing for deployments without one. That is the
majority of small self-hosted setups, and they are exactly the ones that would notice audio
still vanishing.

Chosen: filestore with a per-platform fallback (PRD "Option C"). Presigned URL where
available, honest platform URL plus a caveat note where not, metadata only where neither
exists. Every deployment gains something and none regresses.

Exposing the gateway's colocate path. Tempting, since the agent shares the filesystem in
colocate mode and a local path suits file-processing tooling better than a URL. Rejected on
openab-gateway/src/store.rs:37: TTL_SECS = 120, with a background task evicting on that
schedule. A path that is usually dead on arrival would invite skills to build on it.

Download the audio to disk and hand over a path (the shape originally proposed in #254 and
#738). Superseded by #738's resolution, which chose presigned URLs. Following the pattern that
landed keeps one mechanism instead of two.

Validation

• cargo check passes
• cargo test passes (including new tests)
• cargo clippy clean
• Manual testing (see Live run below)

Base: main @ 53061d69, rustc/clippy 1.91.0. Every command below is one ci.yml runs for
crates/**, and each was run twice, once on this branch and once in a detached worktree
at the unmodified base, so pre-existing failures are separated from regressions rather than
asserted to be pre-existing.

$ cargo check --workspace
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 26.14s

$ cargo build --features unified
    base: 0 errors      this branch: 0 errors

$ cargo test --workspace
    base: 707 passed; 1 failed        this branch: 747 passed; 1 failed

Both re-measured against upstream/main at 53061d69, so 747 - 707 = 40 is the current
net delta. The single failure is secrets::tests::resolve_exec_nonzero_exit on both sides,
confirmed by name; it is a macOS-only /bin/false failure untouched here. The delta was taken
by diffing cargo test -- --list between the two trees rather than by counting #[test]
attributes: 41 names added, 1 removed (video_attachment_block_includes_actionable_metadata,
moved out of discord.rs into media.rs alongside the function it covers). The 41 are the
audio and video block tests, the audio_mime classifier tests including the MIME-casing and
explicit-non-audio boundaries, the gateway outcome and store-failure tests, the four
gateway-arm tests, the two per-adapter store-outcome row tests, the presigned_ttl cap and
lifetime-rendering tests, the upload-API compatibility test, the sanitiser and
prompt-forging tests, the video parity table with its two truncation-boundary companions,
the post-response failure-classification test, and the two attachment-assembly tests.

Scoped clippy, five feature configurations (dual-path validation for the impact statement
under Why this approach?). Narrowing the new items to pub(crate) is what made the extra three
worth running: pub items are exempt from the dead-code lint and pub(crate) ones are not, so
the configurations with no filestore and with no adapter feature each turned up unreachable
code that a pub surface had been hiding. All five are clean:

$ cargo clippy -p openab-core -- -D warnings                                          # default
$ cargo clippy -p openab-core --features filestore -- -D warnings
$ cargo clippy -p openab-core --no-default-features -- -D warnings
$ cargo clippy -p openab-core --no-default-features --features slack,discord -- -D warnings
$ cargo clippy -p openab-core --no-default-features --features slack,discord,filestore -- -D warnings
    all five: Finished `dev` profile [unoptimized + debuginfo]

$ cargo test -p openab-core --features filestore --lib
    base: 674 passed; 1 failed        this branch: 714 passed; 1 failed

$ cargo test -p openab-core --lib          # default features, no filestore
    base: 670 passed; 1 failed        this branch: 705 passed; 1 failed

Two pre-existing failures on main, both reproduced on the unmodified base. Neither is
touched by this PR and neither is fixed by it, per one concern per PR.

1. cargo clippy --workspace -- -D warnings fails on crates/openab-core/src/pre_seed.rs:471
   with collapsible_else_if, a lint that newer clippy applies to code this PR does not touch.
   The base worktree exits 101 with the identical error.
2. cargo clippy --workspace --features unified -- -D warnings reports 2 errors. The error
   set is byte-identical between base and this branch, compared by diffing the sorted ^error
   lines from both runs.

cargo fmt --check is not a gate for this PR. It exists only in ci-openab-agent.yml,
which is path-filtered to openab-agent/** and crates/openab-mcp/** and runs with
working-directory: openab-agent; ci.yml, the workflow that covers crates/**, has no fmt
step.

For the record rather than as a claim of cleanliness: workspace-wide cargo fmt --check under
rustfmt 1.8.0 reports 488 diffs on the unmodified base and 482 on this branch. Every
line this PR adds was hand-formatted to rustfmt's shape, verified by fingerprinting each
Diff in hunk on both sides and confirming that no hunk is unique to this branch. The net -6
is pre-existing hunks this PR happens to rewrite out of existence, most of them inside the two
gateway attachment loops that collapsed into one helper. No crate-wide cargo fmt was run, because it would reformat several hundred
untouched lines across files this PR does not own, for no CI benefit.

The 1 failed in every run above is secrets::tests::resolve_exec_nonzero_exit, a third
pre-existing failure and an environmental one. It shells out to /bin/false, which recent
macOS does not ship, so the error text is "not found" rather than the asserted "exited with".
The base worktree fails it identically. It passes on Linux CI, which is where this matters.

New tests (crates/openab-core/src/media.rs)

$ cargo test -p openab-core --features filestore attachment_block
test media::tests::audio_attachment_block_includes_actionable_metadata ... ok
test media::tests::audio_attachment_block_omits_url_line_when_none ... ok
test media::tests::audio_attachment_block_strips_injected_lines_from_filename ... ok
test media::tests::audio_attachment_block_falls_back_to_unknown_mime ... ok
test media::tests::video_attachment_block_includes_actionable_metadata ... ok
test media::tests::video_attachment_block_omits_note_line_when_none ... ok
test media::tests::video_attachment_block_appends_note_when_present ... ok
test media::tests::video_attachment_block_strips_injected_lines_from_filename ... ok
test result: ok. 8 passed; 0 failed

The audio four cover the presigned-URL shape, the gateway no-URL shape (asserting the
url: line is absent rather than empty), prompt-injection hygiene (a filename carrying
\nurl: ... must not forge a second field), and the empty-MIME fallback.

The video four are the regression guard for the impact statement under Why this approach?.
video_attachment_block_omits_note_line_when_none is the load-bearing one: it asserts the
exact full string, not contains, so any future drift in Discord's output fails the
build. video_attachment_block_includes_actionable_metadata is the test that previously
lived in discord.rs, moved unchanged alongside the function it covers.

These are pure-function tests on the block builders, matching how the adapters are covered
today: discord.rs tests the image block's format string the same way, and the adapter
modules have no mock-transport harness to extend.

Docs. docs/inbound-attachments.md gains the block format, the per-platform URL table,
and the gateway rationale; its support matrix now reads "file + STT". docs/stt.md no longer
claims audio is "silently skipped" when STT is disabled, on both the config table row and the
disabled-behaviour paragraph. Anchors verified against the rendered headings.

Live run. This branch's code has been running on a private 9-agent Slack/Discord
deployment since 2026-07-27, against a real Cloudflare R2 bucket through the
S3-compatible API. The filestore chain is confirmed from its logs end to end: a 1.0MB
attachment streamed up (filestore streaming upload complete ... size=1009827), was
presigned with X-Amz-Expires=3600, and the agent then fetched it with curl -sL -o
against that URL, unprompted, from the block alone. Zero upload, presign, or timeout
errors over the period.

Manual testing: both new paths, end to end on Slack. Object keys and signature
parameters are redacted in the transcripts and masked in the screenshots, because the
bucket is publicly served in this deployment and the key alone is enough to fetch the
object. The screenshots also mask the audio's transcript text, which is unrelated private
content.

Video. A 1MB Big_Buck_Bunny_360_10s_1MB.mp4 posted to a Slack thread. The agent
received the [Video attachment] block and, unprompted, ran:

curl -sSL -o /tmp/bbb.mp4 "https://<account>.r2.cloudflarestorage.com/<bucket>/incoming/<uuid>_Big_Buck_Bunny_360_10s_1MB.mp4?x-id=GetObject&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=<redacted>&X-Amz-Expires=<redacted>&X-Amz-Signature=<redacted>" && ls -la /tmp/bbb.mp4

It then probed the duration with ffprobe, extracted 8 frames with ffmpeg, read them,
and identified the clip as the opening forest shot of Blender's Big Buck Bunny. This is
the case that fails on main: the URL would have been url_private_download and the
curl would have returned 403 with no note explaining why.

Audio. A 23-second, 202KB .m4a posted to the same workspace. The agent received the
[Audio attachment] block and ran:

mkdir -p /home/node/workspace/audio-<date> && curl -s -o /home/node/workspace/audio-<date>/<name>.m4a "https://<account>.r2.cloudflarestorage.com/<bucket>/incoming/<uuid>_.m4a?x-id=GetObject&X-Amz-Algorithm=AWS4-HMAC-SHA256&<redacted>"

then ran a transcription skill against the downloaded path. The message carries both the
🎤 STT reaction and the passthrough, so the transcript and the file block coexisted rather
than one replacing the other, which is the property this PR exists to establish.

Not covered. The media::video_attachment_block extraction and the note-pairing
refactor are both newer than the build that produced the runs above; that build carried the
same routing logic inline. Both refactors are behaviour-preserving and the exact-string test
video_attachment_block_omits_note_line_when_none is what asserts it, but the refactored
form has not itself been exercised in production.

The gateway adapters were not manually tested; no Telegram/Feishu/LINE/Google Chat
deployment was available. Their audio path is covered by unit tests and by the shared
builder the Slack and Discord paths exercise.

There is also no mock-server integration test for the filestore upload itself, since
openab-core's src/ has no precedent to follow (the #[ignore] integration tests live in
the agent crates).

[dogzzdogzz]

Reviewed at cd38142. (Edited: same findings as before, re-organised by severity — 🔴 blocker / 🟡 nit / 🟢 information. No content dropped.)

🔴 Blockers

None. The change is well-scoped and the description is unusually honest about its own residual risks, which made verification easy. Nothing below should hold the merge.

🟡 Nits

Ordered by what I'd actually act on. Line numbers are at cd38142.

1. Duplicate failure block on the gateway read-failure path

gateway.rs:1138-1152 and the mirror in process_gateway_event (~1638-1652).

When bytes fail to read and STT is enabled, two blocks are pushed that say the same thing:

[Audio attachment]
...
note: attachment bytes unavailable (read failed)
[Voice message — read failed for <filename>]

This is specific to the read-failed branch — in the transcription-failed case the two blocks are genuinely complementary (the file arrived, transcription didn't work), so the pairing there is correct. Here they're redundant, and the agent has to reconcile two failure signals for one event. Suggest dropping the legacy string on this branch only, in both functions.

2. Gateway audio is uploaded to filestore with the wrong content type

upload_bytes_and_presign (media.rs:918) → Filestore::upload_and_presign (filestore.rs:95), which hardcodes .content_type("text/plain; charset=utf-8") at filestore.rs:125. That method has no content_type parameter at all.

The Discord/Slack path doesn't have this problem: it goes through stream_upload_and_presign, which honours the caller's MIME (filestore.rs:207, content_type.unwrap_or("application/octet-stream")). So the same .m4a gets served as audio/mp4 from Slack and text/plain; charset=utf-8 from Telegram/Feishu/LINE/Google Chat.

The bytes are correct and Content-Disposition carries the real filename, so this isn't fatal — but a skill that checks Content-Type before decoding will mis-handle exactly the gateway case the PR is adding. Fix is probably a content_type: Option<&str> parameter threaded through upload_bytes_and_presign, matching the streaming path.

3. The gateway match arms have no test coverage, in a config CI never tests

All the new tests target media.rs pure helpers (audio_attachment_block, video_attachment_block). Nothing exercises the three call sites, and specifically nothing exercises the gateway.rs arms — which is where #1 lives, and it's the path the PR itself notes had no manual test either.

Worth pairing with a CI gap: ci.yml:50 runs cargo clippy --workspace --features unified and ci.yml:59-60 runs cargo build --features unified, but there is no cargo test --features unified — while ci.yml:57-58 already establishes exactly that pattern for the acp feature (cargo test -p openab-gateway --features acp) with a comment explaining why default-feature cargo test isn't enough. So the gateway audio arms are currently verified by neither manual testing nor automated testing; they are compiled and linted only. That gap predates this PR, but this PR is the first change to lean on it.

4. No per-message cap on audio attachments

Text files are capped (TEXT_FILE_COUNT_CAP = 5, discord.rs:872 / slack.rs:1498); audio has no equivalent. With STT disabled, audio used to cost nothing; now every audio attachment triggers a full download plus a filestore upload, each bounded only by max_file_size. Ten voice notes in one message is now ten downloads and ten uploads. Given the existing precedent for text, a cap (or an explicit note that the size limit is the only bound) seems worth having.

5. "Always emitted" is conditional on Slack returning a private URL

slack.rs:1513-1517:

let url = slack_file_download_url(file);
if url.is_empty() {
    continue;
}

This guard predates the PR and sits before the is_audio_mime check, so it drops the whole attachment — audio included — when Slack's file JSON has neither url_private_download nor url_private. Not a regression, and probably rare, but it means the guarantee is "always, provided Slack returned a private URL." Might be worth a footnote in the docs table rather than a code change.

6. On the four unsanitised [Voice message …] sites

Confirmed there are exactly four, all in gateway.rs (1128, 1148, 1628, 1647) — discord.rs:894 and slack.rs:1540 only interpolate the transcript, not the filename, so the surface is narrower than it might look.

Agreed these are out of scope for this PR. The one nudge: they're reachable on demand (send un-transcribable audio with a crafted filename to force the failure branch), and the sanitiser now lives three functions away, so if it's a follow-up it'd be good to have it as a filed issue rather than only a PR-description note.

7. Cosmetic

• presigned_note (media.rs:908) does presigned_ttl_secs() / 60, so a sub-60s TTL renders as "expires in 0 minutes". Only reachable on an unusual config.
• Block ordering differs by adapter: Discord/Slack insert(0, transcript) then push the metadata block (discord.rs:891), while the gateway pushes metadata first then the transcript. Harmless — the agent gets both — but if the ordering is meant to be meaningful, it's currently adapter-specific.
• The "byte-identical to main" claim is worth scoping in the description: it holds for benign filenames, but the new shared builder sanitises and truncates where the old private video_attachment_block did not, so pathological filenames now produce different (better) output. That's a feature, not a regression — just not literally byte-identical.

🟢 Information

Load-bearing claims that hold under trace:

• "Always emitted regardless of STT" holds on all three paths. On Discord mime_clean is hoisted out of the STT branch and the extra_blocks.push sits unconditionally after the if/else; on the gateway both run_gateway_adapter and process_gateway_event collapse the old "audio" if stt_config.enabled / bare "audio" split into one arm, and both the Ok(bytes) and Err(e) sub-branches push a block. No early return or continue sits between the STT decision and the passthrough push. (Caveat: 🟡 fix: use PR for chart bump instead of direct push #5.)
• The [File: ...] refactor is byte-identical, including all three degraded hint strings — PresignError::{Unavailable,UploadFailed,UploadTimedOut} maps one-to-one onto the old None / upload-failed / timed-out arms, and the hint templates are unchanged.
• #[cfg(feature = "filestore")] gating is consistent — every filestore-only item is gated (presigned_note, PresignError, upload_bytes_and_presign, download_and_presign_any_file, download_and_presign_attachment, download_and_upload_any_file) and each caller's let stored = None fallback arm references no gated symbol.
• sanitize_attachment_meta is not bypassable via the url: line. Filestore::upload_and_presign filters the S3 key to is_ascii_graphic(), so a filename carrying a newline cannot forge a line through the presigned URL, and the note: values are all static strings.

Undersold in the description: the diff actually narrows the injection surface — the old Discord/Slack video block interpolated the filename raw, and routing it through sanitize_attachment_meta is a quiet security fix.

Method: this review is from reading the diff and the surrounding files at cd38142 — no build, test, or clippy run was executed, so treat the compilation-related observations (🟡 #3, the cfg gating check) as reasoning rather than execution, and let CI be the authority on the filestore-on/off matrix in your acceptance criteria.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The audio passthrough does not cover common MIME fallback cases, lacks message-level work limits, and its published storage contract disagrees with the implementation.

What This PR Does

This PR makes Discord, Slack, and gateway audio attachments available as metadata blocks independently of STT. It also routes Slack video through filestore when configured, so the agent can receive a credential-free presigned URL rather than an unusable Slack private URL.

How It Works

Shared media helpers build sanitized audio and video blocks and pair presigned URLs with their expiry notes. Discord and Slack stream URL-backed media to filestore; gateway adapters upload bytes they already hold. The patch also preserves transcript-to-file ordering and passes the real MIME type to the gateway single-PUT path.

Findings

#	Severity	Finding	Location
1	🟡	Audio classification accepts only audio/* MIME values, so a valid .ogg or .m4a with a missing or generic MIME bypasses the new passthrough.	crates/openab-core/src/media.rs:392-395, discord.rs:874-936, slack.rs:1506-1593
2	🟡	Documentation promises storage behavior that the code deliberately does not provide, and omits storage behavior that it now does provide.	docs/inbound-attachments.md:115-124, docs/filestore.md:135-145
3	🟡	Each audio attachment can start expensive download/upload work without a message-level count, byte, or time budget.	discord.rs:874-935, slack.rs:1506-1592, media.rs:1072-1105
4	🟡	New behavior is tested only through pure block builders; adapter routing and both duplicated gateway paths have no regression coverage.	crates/openab-core/src/gateway.rs:1076-1141, gateway.rs:1562-1635, media.rs:1515-1620
5	🟢	The shared builders sanitize prompt-visible metadata, and the gateway upload now retains the attachment content type.	media.rs:397-505, filestore.rs:108-174

Finding Details

🟡 F1: Classify audio by MIME or recognized filename extension

is_audio_mime is only mime.starts_with("audio/"). Discord supplies an empty string when content_type is absent, and Slack does the same for absent mimetype; application/octet-stream also fails this test. The adjacent video classifier already falls back to filename extensions. Consequently, an ordinary clip.ogg or meeting.m4a with generic metadata enters the non-audio path: without filestore it can be dropped, and with filestore it is represented as a generic file instead of receiving the promised audio/STT behavior.

Requested change: add an audio classifier that accepts the platform MIME or a bounded list of recognized audio extensions, use it in both adapters, and test empty and application/octet-stream metadata at the adapter decision boundary.

🟡 F2: Make the storage documentation match the implemented matrix

The video table says Discord with filestore returns a presigned S3 URL, but discord.rs always emits attachment.url for video and explicitly bypasses filestore. Conversely, the filestore table still says videos are excluded and gateway uploads cover text only, although Slack video is now presigned and gateway audio is uploaded with a single PUT. These contradictions cause operators to choose a storage configuration based on behavior the application will not deliver.

Requested change: document the current implementation consistently: Discord video remains CDN-only; Slack video and audio use filestore when configured; gateway audio uses the buffered single-PUT path; and the relevant fallback behavior remains explicit.

🟡 F3: Bound aggregate audio work per inbound message

The loops await every audio attachment and apply no audio-specific count or aggregate-byte budget. The configured filestore default permits 250 MB per file (up to 500 MB), each streaming attempt has a 600-second timeout, and the new docs state that Slack and Discord may supply ten files per message. A single allowed message can therefore schedule multi-gigabyte transfer and serially hold the handler for many timeout windows. STT plus filestore also performs a second download by design.

Requested change: add a small audio attachment count limit and aggregate transfer budget before starting downloads, with a clear degraded block or log for skipped items. Cover the cap and a multi-attachment message in tests.

🟡 F4: Cover routing rather than only formatting helpers

The new tests invoke audio_attachment_block and audio_attachment_blocks, but no test invokes the Discord, Slack, or gateway routing branches. The gateway logic exists twice, and neither path is exercised for STT enabled/disabled, filestore success/failure, or byte-read failure. This is the code that caused the earlier duplicate failure-message and content-type defects; helper tests cannot detect a missed call, wrong ordering, or divergent fallback at a call site.

Requested change: introduce a testable routing seam or focused adapter tests. At minimum cover Discord and Slack MIME fallback plus both gateway entry points for STT on/off and filestore/read-failure outcomes.

🟢 F5: Good shared safety and content-type handling

The shared builders remove control characters from user-controlled metadata before it becomes prompt text. The current head also threads gateway audio MIME to upload_and_presign, fixing the prior text/plain object metadata mismatch.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and merge base: 53061d696148106b2b7529f9d6c5dd802dff4545
• Reviewed head: 30e596933d42bf1d7b3a73e271bf793bf30340bd
• Diff stat: 8 files changed, 767 additions, 182 deletions
• Main already has: transcript-only audio handling, Discord CDN video metadata, and binary filestore uploads.
• Net-new value: audio metadata passthrough across three adapters, Slack video presigning, shared block construction, and content-type propagation for gateway audio.

Addressing External Reviewer Feedback

@dogzzdogzz

Gateway read-failure output was duplicated and gateway audio used text/plain; gateway routing lacked tests; audio had no per-message cap; Slack could omit a private URL; and prompt metadata or TTL/order behavior needed hardening.

• Addressed in the reviewed head: duplicate gateway read-failure output was removed, upload_and_presign now receives the real content type, metadata is sanitized, the TTL has a one-minute floor, and per-attachment transcript ordering is centralized.
• Accepted documentation clarification: the Slack private-URL precondition is now described.
• Still open: the message-level audio-work limit and gateway call-site coverage remain F3 and F4 above. Documentation alone does not constrain transfer or handler time.

Reviewer Aggregation

Reviewer	Result
Reviewer A - correctness and safety	F1, F3
Reviewer B - tests and maintainability	F1, F2, F4
Reviewer C - architecture and operations	F2, F3

5. Three Reasons We Might Not Need This PR

1. Transcript-only delivery is a simpler established model - it avoids retaining and serving another copy of potentially sensitive media.
2. Deployments without filestore still cannot give gateway media a fetchable URL - the feature is most complete only where object storage is configured.
3. The initial implementation duplicates remote work - on Discord and Slack, STT plus filestore fetches the same audio twice, so a single-download design may be preferable before expanding passthrough.

What's Good (🟢)

• The transcript remains additive, and the shared helper keeps each transcript adjacent to its own file metadata.
• The Slack fallback names its credential requirement instead of leaking a token.
• git diff --check passes and all 42 completed GitHub checks for the reviewed SHA are successful (with the expected skipped operator job).

[ShinyChang]

All findings from both review rounds are addressed at 24f7d5d0, except one that is declined with reasoning. Details per finding.

From the first review

#	Outcome
1 duplicate read-failure block	Dropped on that branch only (44ff6a3b). The transcription-failure branch keeps its pairing, since there the file did arrive.
2 gateway audio content type	upload_and_presign now takes content_type and defaults to application/octet-stream, matching the streaming path (cdba4a15). The text caller passes its previous value explicitly, so its output is unchanged.
3 gateway arms untested	Addressed, see F4 below.
4 no per-message audio cap	Declined, with the reasoning in Accepted Residual Risks.
5 Slack private-URL precondition	Documented (30e59693).
6 four unsanitised [Voice message ...] sites	Now zero (44ff6a3b). Two were the redundant read-failure line. The other two no longer name the file, because the adjacent metadata block already carries it sanitised.
7 cosmetics	presigned_ttl gains a 60s floor (c80593d1), so ttl / 60 cannot render "0 minutes" at any of its three sites. Block order is centralised. The "byte-identical" claim is scoped in the description.

Two notes on that round.

The ordering point was worse than cosmetic. insert(0, ...) sat inside the loop over attachments, so three voice notes rendered as [t3, t2, t1, m1, m2, m3]: transcripts reversed and detached from the files they describe. It only became reachable because this PR added the metadata block. The chosen order is transcript then metadata, which leaves the single-attachment case byte-identical.

On the CI half of #3: cargo test --features unified is indeed absent, but it is not the cause here. crates/openab-core/src/gateway.rs is not behind unified, which gates the openab-gateway platform adapters, so those arms already compile and run under plain cargo test --workspace. The missing coverage was real; the feature gap was not what caused it.

From the second review

F1, audio classification. Fixed in af5640e1, and the fix goes further than the requested change for a reason. A bool classifier would have fixed only half of it: stt::transcribe builds its multipart body with Part::mime_str(mime_type).ok()?, which discards the request when the value does not parse. Admitting a clip.ogg whose MIME is "" would therefore have traded "audio silently dropped" for "audio always fails to transcribe". media::audio_mime returns a MIME synthesised from the extension instead of a bool, and one of its tests uses Part::mime_str itself as the oracle. The extension list deliberately omits webm, mp4 and ogv so it never claims an attachment is_video_file should handle.

F2, storage documentation. Fixed in dbef0e60. All three contradictions confirmed: the video table claimed Discord returns a presigned URL when discord.rs always emits attachment.url and excludes video from the filestore branch; filestore.md still described video as never uploaded and gateway uploads as text-only. The tables now split video by platform and carry audio rows.

F3, per-message work budget. Declined for this PR, reasoning added to Accepted Residual Risks. The numbers hold: the loop awaits serially, STREAM_TIMEOUT is 600s, max_file_size_mb defaults to 250. But this is not a risk class this PR creates. download_and_upload_any_file, the PDF/ZIP/binary path already on main from #738, has no count or aggregate-byte budget either. Only text files are capped, at 5, and that cap exists to protect the prompt from inlined content, which is why it is bypassed the moment a filestore takes over. Audio bytes are never inlined, so its rationale does not transfer. Capping audio alone would leave the same exposure one branch away and imply a bound the sibling path does not honour, so the budget belongs in a change covering every filestore-bound attachment type.

F4, routing coverage. Addressed by removing what made it dangerous. The audio arm existed twice, and the copies had already drifted twice in this branch's own history: the duplicated read-failure block and an STT-failure warn! present on one side only. 24f7d5d0 collapses both entry points into a single gateway_audio_blocks, including the filestore upload, which is where the content-type defect diverged. There is no second copy left to drift.

Two tests drive that arm directly. STT disabled with no filestore reaches neither the network nor AWS, so they run in CI rather than under the #[ignore] this repo requires of tests touching either. Both cfg branches compile and both tests pass with and without the filestore feature.

Still uncovered, stated plainly: the filestore-success and STT-enabled outcomes, which need a fake for an S3 client and an HTTP endpoint. The MIME-fallback boundary is covered as pure tests on audio_mime.

cargo clippy --workspace --features unified -- -D warnings and cargo clippy -p openab-core -- -D warnings are both clean, and cargo test --workspace is green apart from secrets::tests::resolve_exec_nonzero_exit, which fails on macOS only and is untouched by this branch.

[antigenius0910]

Re-tested at dbef0e60 after @ShinyChang pushed the seven follow-up commits (cdba4a1 → dbef0e6). All the concerns from my previous comment are addressed; here's what I verified on live Slack.

F1 — audio classifier now falls back to filename extension (verified)

Uploaded the same 18785-byte m4a payload as before, but this time named voice.opus. Slack labels it application/octet-stream. On the old head that went down the generic-file path; on dbef0e60:

openab_core::slack: audio attachment not transcribed (STT disabled) filename="voice.opus"
openab_core::filestore: filestore streaming upload complete bucket=openab-files key=incoming/…_voice.opus size=18785

Prompt block delivered:

[Audio attachment]
filename: voice.opus
content_type: audio/opus
size_bytes: 18785
url: http://openab-minio:9000/openab-files/incoming/b188dd63-…_voice.opus?…&X-Amz-Signature=…
note: presigned URL, expires in 60 minutes

Agent's reply: "content_type: audio/opus; block is Audio." — the audio branch is reached and the synthesised MIME is correctly what audio_mime_from_extension produces (audio/opus for .opus, not the platform-supplied application/octet-stream).

I also ran the negative case (voice.bin, same bytes) to confirm the fallback doesn't over-claim. That one still lands as [File: voice.bin] Type: application/octet-stream … — correct: .bin isn't in the extension list, so the generic-file path takes it. F1 fix is precise, not blanket.

The commit message's rationale for returning Option<String> instead of bool is worth reading — it explicitly handles the multipart-body case where stt::transcribe would silently drop a request with an unparseable MIME. Small detail, right call.

F2 — docs matrix now matches implementation (verified)

dbef0e6 says Discord video stays on CDN with or without a filestore, and adds a paragraph explaining why. Cross-checked against discord.rs: video still emits media::video_attachment_block(..., None) on the NotAnImage branch, never touching filestore. Docs and code now agree.

F3 — documented rather than code-changed

30e5969 adds a Slack URL caveat ("always emitted, provided Slack returned a private URL") and notes the platform-level 10-file cap as the effective bound. The commit message argues the audio bytes are never inlined into the prompt, so the text-file precedent (TEXT_FILE_COUNT_CAP = 5) doesn't transfer — the risk is bandwidth, not context-window inflation. That's a defensible framing. Not a blocker.

F4 — gateway paths now share one seam (verified via code review)

3dc4014 collapses the two gateway audio arms onto media::audio_attachment_blocks() (plural — takes optional STT line, returns Vec<ContentBlock>). Both run_gateway_adapter and process_gateway_event now call the same helper. The helper is unit-tested in media.rs. No gateway rig here to exercise both arms at runtime, but the duplicate branches that had no coverage before are now one branch that does.

TC-1 re-check on new head — still passes

Video with filestore configured: [Video attachment] block with presigned MinIO URL, agent fetched via curl and returned 00 00 00 20 66 74 79 70 69 73 6f 6d 00 00 02 00 69 73 6f 6d. No regression from the seven follow-up commits.

Verdict — LGTM at dbef0e60

Every concern from the earlier round is either fixed in code (F1, F2, F4, gateway MIME per dogzzdogzz #2, duplicate-block-on-read-fail per dogzzdogzz #1, presigned-ttl floor) or explicitly documented as a design choice (F3, Slack URL caveat). The runtime behavior I re-exercised — F1 refix positive + negative, and TC-1 spot-check — matches what the diff claims. Recommend merging.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Gateway filestore failures are reported as missing configuration, and gateway/STT documentation still describes superseded behavior.

What This PR Does

This PR makes inbound audio available to the agent independently of speech-to-text and makes Slack video use a credential-free presigned URL when filestore is configured. It also centralizes attachment metadata formatting, MIME fallback, and gateway audio routing.

How It Works

• Shared media builders sanitize attachment metadata, keep each transcript adjacent to its audio block, and support filename-extension MIME fallback.
• Discord and Slack preserve their platform-specific fallback URLs; Slack video uses filestore when available.
• Gateway entry points call one audio helper, which uploads held bytes to filestore before emitting a metadata block and optionally adding STT output.

Findings

#	Severity	Finding	Location
1	🟡	A configured filestore that rejects, times out, or fails an audio upload is represented as if no filestore were configured, so the agent receives an incorrect recovery instruction.	crates/openab-core/src/gateway.rs:99-137, crates/openab-core/src/media.rs:1004-1034
2	🟡	Several supported gateway/STT configuration references still document behavior changed by this PR.	docs/feishu.md:221, docs/config-reference.md:684,709, docs/platforms/schema/googlechat.toml:184, docs/filestore.md:369-376
3	🟢	The current head fixes the prior MIME fallback, content-type, transcript ordering, TTL-floor, and duplicated-gateway-arm concerns with focused coverage.	crates/openab-core/src/media.rs:391-523, crates/openab-core/src/gateway.rs:68-138

Finding Details

🟡 F1: Preserve the configured-filestore failure reason

upload_bytes_and_presign returns None both when no upload can be made because the audio exceeds the configured limit and when upload_and_presign fails. gateway_audio_blocks maps every such None to AudioOutcome::NoStore. That outcome emits the note "configure a filestore to give the agent a downloadable link," although a filestore is already configured in these paths.

This makes an outage, bad credentials, presign failure, or size rejection look like a missing configuration problem and directs the agent/operator to the wrong remediation. Keep enough typed outcome information to distinguish NoStore from configured-store rejection/failure, and emit a truthful no-URL note for the latter. Add tests for at least a configured upload failure and a configured size rejection.

🟡 F2: Synchronize the remaining user-facing contracts

The PR updates the new inbound-attachment and STT pages, but other supported references still contradict the reviewed behavior: Feishu says audio is skipped when STT is disabled or fails; the Google Chat schema says STT-disabled audio becomes a transcription-failed note; and the config reference says a no-filestore build leaves all behavior unchanged. The new gateway path instead always emits an audio metadata block, with no STT line when STT is disabled.

The filestore failure table also promises a degraded availability hint for a configured upload failure, while the current audio path produces the misleading NoStore configuration instruction described in F1. Update these references together with F1 so operators and platform consumers receive one accurate matrix.

🟢 F3: Good convergence of the earlier fixes

The current head uses audio_mime to synthesize parseable audio MIME values for recognized extensions, routes both gateway entry points through one helper, sends gateway audio with its actual content type, preserves transcript-to-attachment ordering, and clamps presigned TTL to at least one minute. The focused helper and gateway no-store tests cover these behaviors.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and merge base: 53061d696148106b2b7529f9d6c5dd802dff4545
• Reviewed head: 24f7d5d09b41744127b7050fe6e28bd430ae16ba
• Diff stat: 8 files changed, 998 additions, 204 deletions
• Main already has: transcript-only audio handling, Discord CDN video metadata, and generic binary filestore uploads.
• Net-new value: audio metadata passthrough across Discord, Slack, and gateway; credential-aware Slack video routing; shared attachment builders; and corrected gateway audio content type.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered duplicate gateway read-failure blocks, gateway audio content type, missing MIME fallback, storage docs, routing coverage, and attachment work limits.

The reviewed head resolves the duplicate read-failure block, preserves gateway audio MIME, adds extension fallback, centralizes the two gateway arms, and updates the primary storage matrix. The per-message work-limit concern is documented by the author as a broader filestore concern and is not re-raised here. F1 is a separate configured-store failure path: it was introduced by collapsing upload outcomes to None and is not covered by the no-filestore tests.

@antigenius0910

The follow-up head was manually checked for MIME fallback, Slack video presigning, documentation alignment, and gateway helper convergence.

Those improvements remain present at the reviewed head. This round adds the remaining configured-filestore failure semantic and the references outside the pages updated by the follow-up.

5. Three Reasons We Might Not Need This PR

1. Transcript-only delivery is simpler - it avoids duplicating downloads and retaining another copy of potentially sensitive media.
2. Gateway media is fully actionable only with filestore - without it, gateway payloads have no platform URL to provide.
3. A shared attachment-budget change may be a better prerequisite - audio now joins existing filestore-bound file paths that lack one aggregate work budget.

What's Good (🟢)

• Prompt-visible filename and MIME metadata is sanitized by shared builders.
• Slack fallback describes its bearer-token requirement rather than leaking credentials.
• git diff --check passes, and all 40 GitHub check runs completed; 38 succeeded and 2 expected jobs were skipped.
• Local test and clippy re-runs were not possible because this environment has no cargo executable.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Moving gateway attachment assembly off the receive loop currently violates same-thread event ordering and lets pre-reset work enter a new session.

What This PR Does

This PR makes audio attachments actionable independently of STT and routes Slack video through filestore when available. It also centralizes attachment formatting, MIME handling, failure notes, and gateway attachment assembly.

How It Works

Shared media helpers build sanitized audio/video blocks, retain measured stored sizes, and distinguish configured-store failures. Discord and Slack use URL-backed uploads; gateway adapters process held bytes and may upload them before dispatching the event.

Findings

#	Severity	Finding	Location
1	🔴	Asynchronous gateway attachment assembly can submit later same-thread events first and can submit a pre-reset event after /reset.	crates/openab-core/src/gateway.rs:1214-1266
2	🟡	Attachment work is admitted without a bound before Dispatcher backpressure, and the JoinSet is only drained at shutdown/reconnect.	crates/openab-core/src/gateway.rs:1060,1214,1289-1297
3	🟡	A dotless filename equal to a recognized extension is classified as audio when MIME is missing/generic.	crates/openab-core/src/media.rs:433-443
4	🟡	The bot-turn-limit documentation is attached to DISCORD_CDN_NOTE, leaving MAX_CONSECUTIVE_BOT_TURNS undocumented.	crates/openab-core/src/discord.rs:28-35
5	🟢	The current head preserves prior compatibility and failure-handling fixes: public MIME API, typed storage outcomes, measured sizes, and prompt metadata sanitization.	crates/openab-core/src/media.rs, filestore.rs

Finding Details

🔴 F1: Preserve receipt order and reset boundaries

run_gateway_adapter spawns a task at line 1214, awaits attachment assembly in that task, and calls Dispatcher::submit only at lines 1264-1266. If event A has a slow attachment and later event B has none, B reaches the per-thread FIFO first. This violates Dispatcher invariant I3: no reordering.

The same gap bypasses /reset: a reset cancels only already-submitted handles, while A is still assembling outside Dispatcher. Once reset creates the new session, A can finish and submit into it. Capture a sequence/generation at receipt, commit same-thread events in sequence, and invalidate/cancel prior-generation work on reset. Add delayed-assembly ordering and reset-race tests.

🟡 F2: Bound pre-dispatch attachment work

Every accepted WebSocket event immediately starts reads, base64 decoding, STT, and potentially a 600-second filestore transfer before Dispatcher capacity applies. There is no admission semaphore/queue, and tasks.join_next() occurs only on shutdown or reconnect. A burst can therefore create unbounded concurrent attachment work and retain completed task results for the connection lifetime. Add bounded admission, continuous reaping, and an overload policy before assembly.

🟡 F3: Require an actual extension separator

filename.rsplit('.').next() returns the entire string when no dot exists. Thus audio_mime("mp3", None) returns audio/mpeg, despite mp3 not having an extension. Use rsplit_once('.') (requiring a non-empty stem and extension) and add dotless recognized-name regression cases.

🟡 F4: Reattach the constant documentation

The first two doc-comment lines describe the 1000-turn guard but are now attached to DISCORD_CDN_NOTE. Keep the CDN-note explanation with that constant and move the turn-limit explanation directly above MAX_CONSECUTIVE_BOT_TURNS.

🟢 F5: Prior fixes remain effective

The reviewed head retains public is_audio_mime, preserves typed presign outcomes and measured sizes, and applies the shared metadata sanitizer. These are meaningful improvements over earlier heads.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch and merge base: main / 53061d696148106b2b7529f9d6c5dd802dff4545
• Reviewed head: da9890ea53ea8a288ec6defab1b2e7ee4f48ce77
• Diff stat: 23 files changed, 2187 additions, 475 deletions
• Main already has: transcript-only audio handling, public Discord video URLs, and generic binary filestore uploads.
• Net-new value: audio metadata passthrough, Slack video presigning, shared attachment rendering, typed adapter outcomes, and expanded attachment documentation.
• Validation: local git diff --check passed. Local Rust tests could not run because this environment lacks cargo and rustc. Remote checks are successful except an in-progress builder job; two superseded runs are cancelled and the operator job is skipped.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway-arm duplication, MIME fallback, content type, documentation, routing coverage, and attachment work limits.

The current head centralizes gateway audio handling, preserves actual MIME, handles explicit non-audio MIME conflicts, and updates the attachment matrix. F1/F2 identify the new scheduling consequence of moving that work outside the receive loop; this is distinct from the earlier duplicate-arm issue.

@antigenius0910

Earlier testing confirmed generic-MIME audio routing and Slack video presigning on an older head.

Those behaviors remain present. This round re-reviewed the later gateway scheduling refactor and does not dispute the earlier adapter-path validation.

@howie

Earlier feedback requested truthful download-failure handling, stored-size accuracy, and an updated voice-only batching contract.

The current source retains the typed DownloadFailed handling, measured stored-byte propagation, and revised batching documentation. The remaining blocker is receipt-to-dispatch ordering introduced by the latest receive-loop offload.

Reviewer Aggregation

Reviewer	Result
Reviewer A - correctness and safety	F1, F3
Reviewer B - architecture and operability	F1, F2
Reviewer C - integration and maintainability	F4, F5

5. Three Reasons We Might Not Need This PR

1. Transcript-only handling is simpler - it avoids retaining another fetchable media copy.
2. Gateway media is only fully actionable with filestore - without storage it has metadata but no fetch target.
3. A shared attachment scheduler may be the better prerequisite - it could establish ordering, reset cancellation, and resource limits for every filestore-bound type.

What's Good (🟢)

• Shared metadata sanitization reduces prompt-structure injection risk.
• Presign failure classification and measured sizes address important correctness problems from earlier heads.
• The exact reviewed diff has no whitespace errors, and current completed remote validation jobs are successful.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Preserve gateway event ordering/reset boundaries and fix the remaining important findings.

Consolidated review: #1460 (comment)

[chaodu-obk]

🔴 F1 - Preserve receipt order and reset boundaries

This task awaits attachment assembly before it reaches Dispatcher::submit. A slow earlier event can therefore be submitted after a later event in the same thread, despite the Dispatcher FIFO contract. A /reset in between only cancels messages already submitted, so the earlier task can also enter the fresh post-reset session.

Requested change: assign an ingress sequence/generation before spawning, commit same-thread events in sequence, and cancel or invalidate pre-reset work. Add delayed-assembly ordering and reset-race tests.

[chaodu-obk]

🟡 F2 - Bound attachment work before dispatch

Each received event starts attachment reads, decoding, STT, and possible filestore I/O before Dispatcher capacity applies. This JoinSet has no admission bound and is only drained on shutdown or reconnect, so a burst can create unbounded concurrent work and retain task results for the connection lifetime.

Requested change: add bounded admission/backpressure before assembly, continuously reap tasks, and define a safe overload policy with a regression test.

[chaodu-obk]

🟡 F3 - Require an actual filename extension

rsplit('.').next() returns the whole filename when no dot exists. With absent or generic MIME, a dotless filename such as mp3 is therefore classified as audio/mpeg and can be routed through STT/audio passthrough even though it has no extension.

Requested change: use rsplit_once('.') and require both a non-empty stem and extension. Add dotless recognized-name regression tests.

[chaodu-obk]

🟡 F4 - Reattach the constant documentation

The existing first two doc-comment lines describe the consecutive-bot-turn guard, but these new lines extend that block so it now documents DISCORD_CDN_NOTE; MAX_CONSECUTIVE_BOT_TURNS has no documentation.

Requested change: keep only the CDN explanation above DISCORD_CDN_NOTE and move the turn-limit explanation directly above MAX_CONSECUTIVE_BOT_TURNS.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The gateway ticket check does not fence a pre-reset event through the asynchronous dispatcher handoff, so the event can be retried into the new session.

What This PR Does

This PR makes inbound audio actionable independently of STT and gives Slack video a credential-free presigned URL when filestore is configured. It also centralizes attachment formatting, media error handling, and gateway attachment assembly.

How It Works

Shared media helpers classify and sanitize attachment metadata, retain measured upload sizes, and render platform-specific fallback notes. The gateway moves attachment work out of the WebSocket receive loop, limits concurrent fetches, and uses PreDispatchOrder tickets to preserve same-thread arrival order and invalidate work after /reset.

Findings

#	Severity	Finding	Location
1	🟡 Important	A pre-reset event can pass its generation check, then be retried by Dispatcher::submit after reset has removed the old consumer, recreating a consumer for the new session. Post-reset events also remain chained behind stale preparation work.	crates/openab-core/src/gateway.rs:1438-1448; crates/openab-core/src/dispatch.rs:378-422
2	🟢 Praise	Shared media builders, typed storage outcomes, measured sizes, public API compatibility, and prompt metadata sanitization address the earlier attachment correctness and safety gaps.	crates/openab-core/src/media.rs; crates/openab-core/src/filestore.rs

Finding Details

🟡 F1: Make the reset fence cover dispatcher enqueue and retry

PreDispatchOrder::is_current() is checked once immediately before await dispatcher.submit(...). A /reset can run after that check and call cancel_buffered_thread, which removes and aborts the current consumer. If the original tx.send() was parked because the queue was full, it returns SendError; Dispatcher::submit then transparently creates or reuses a fresh consumer and re-sends the old BufferedMessage. That message was admitted before reset, but can now enter the new session.

The same generation bump leaves the old ticket tail attached. A post-reset event therefore waits for a stale attachment assembly to finish before it can reach the new session, potentially for the upload timeout.

This contradicts the documented guarantee that /reset beats work in flight. Fence enqueue/retry with the ticket generation (or a reset cancellation token), and detach the new generation from the old tail. Add a deterministic full-queue/reset test that proves an old message is neither retried nor allowed to delay the first post-reset message.

🟢 P1: Earlier attachment hardening remains effective

The current head keeps the successful earlier fixes: MIME fallback is bounded, prompt-visible metadata is sanitized, public Filestore and MIME-helper contracts are preserved, configured-store failures retain their reason, and adapter output uses measured bytes after a successful upload.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and local merge base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 8c4b07694d7422b4cc8cf33a6d702d27fdedd5f2
• Diff stat: 23 files changed, 2549 additions, 478 deletions
• Main already has: transcript-only audio handling, public Discord video URLs, and generic binary filestore uploads.
• Net-new value: audio metadata passthrough, Slack video presigning, typed attachment outcomes, prompt metadata hardening, and bounded gateway pre-dispatch assembly.
• Validation: git diff --check origin/main...HEAD passed locally. The exact head has 43 completed GitHub checks, all successful or expected skipped. Local Rust execution was unavailable because neither cargo nor rustc is installed in this environment.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway duplication, content type, MIME fallback, documentation, routing coverage, and attachment work limits.

Addressed in the current head: gateway audio routing is shared, gateway uploads preserve MIME, missing/generic MIME fallback is bounded, ordering is paired per attachment, and the affected documentation was updated. The broader attachment-work budget remains a documented cross-type follow-up; it is not the reason for this verdict.

@antigenius0910

Earlier live validation confirmed generic-MIME audio routing and Slack video presigning.

Those adapter-path improvements remain present. The later receive-loop offload and ticket implementation require a separate review; this round identifies the remaining reset handoff race in that newer code.

@howie

Earlier feedback requested truthful Discord download-failure handling, stored-size accuracy, and a voice-only rollback contract aligned with the new block shape.

Verified as addressed in the current head: download failures retain a truthful note, stored attachments carry their measured byte count, and the turn-boundary ADR identifies transcript blocks rather than relying on the old block count.

Reviewer Aggregation

Reviewer group	Result
Concurrency, correctness, and API review	F1 confirmed independently
Security, error-outcome, tests, and documentation review	No additional Critical or Important finding

5. Three Reasons We Might Not Need This PR

1. Transcript-only handling is simpler - it avoids retaining another fetchable copy of potentially sensitive media.
2. Gateway media remains limited without object storage - without filestore, gateway adapters can provide metadata but no fetch target.
3. A common attachment transport scheduler may be preferable - it could eliminate Discord/Slack double downloads and establish one reset, cancellation, and backpressure policy for every attachment type.

What's Good

• The shared audio/video builders materially reduce prompt-structure injection risk.
• The gateway no longer performs remote attachment work in the WebSocket receive loop, and it now has explicit concurrency and load-shedding limits.
• The current head preserves prior compatibility fixes and has clean remote CI.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Gateway pre-dispatch scheduling still lets stale attachment work block a reset session and can lose queued colocated attachments.

What This PR Does

This PR makes inbound audio actionable independently of STT and gives Slack video a credential-free URL when filestore is configured. It also moves gateway attachment assembly off the WebSocket receive loop, adding ordering tickets, bounded fetch concurrency, and load shedding.

How It Works

Shared media helpers sanitize metadata, retain measured stored sizes, and distinguish storage outcomes. On the gateway WebSocket path, each event takes a PreDispatchOrder ticket at receipt, then a spawned task acquires one of four fetch slots, assembles attachments, and gates Dispatcher handoff through the ticket generation.

Findings

#	Severity	Finding	Location
1	🟡	/reset invalidates only dispatcher handoff; stale tasks can still consume fetch slots, upload, and create a forum topic before the generation is checked.	crates/openab-core/src/gateway.rs:1381,1436-1503
2	🟡	A task waits for a fetch slot before reading its colocated path, so queued media can expire before Core reads it.	crates/openab-core/src/gateway.rs:1436, gateway.rs:167-170
3	🟢	The current head preserves earlier fixes for MIME classification, public API compatibility, measured sizes, typed storage outcomes, and prompt metadata sanitization.	crates/openab-core/src/media.rs, filestore.rs

Finding Details

🟡 F1: Cancel stale attachment preparation on reset

/reset changes the PreDispatchOrder generation at line 1381, but a spawned task awaits the global semaphore and runs assemble_attachment_blocks at lines 1436-1443 before it first observes that generation at lines 1489-1498. Reset therefore prevents dispatch, not preparation.

Reproduction: hold all four permits with slow pre-reset attachment uploads, send /reset, then send a post-reset attachment. The new ticket has no stale predecessor, but its attachment still waits behind the pre-reset tasks at fetch_slots.acquire(). A stale supergroup task can also execute create_thread at lines 1448-1462 before it is dropped at handoff. This contradicts the documented guarantee that the first event in the new session never waits out discarded upload work.

Requested change: make reset cancellation cover semaphore acquisition, attachment assembly, and pre-dispatch side effects, not only Dispatcher::submit. Add a deterministic test that holds old-generation permits, resets, and proves a new-generation attachment can start without waiting for those stale tasks; also prove a reset prevents stale forum-topic creation.

🟡 F2: Preserve an admitted colocated attachment while it waits

The semaphore is acquired before assemble_attachment_blocks, while a colocated attachment is first read inside that helper. The gateway store evicts media older than 120 seconds and sweeps every 30 seconds (openab-gateway/src/store.rs:36-37,75-100). Four stalled uploads can therefore keep a fifth non-shed event queued beyond the source file lifetime. When a slot finally opens, tokio::fs::read fails and the agent receives a read-failure block instead of the attachment.

This is below the 32-event shedding threshold, so it contradicts the changed documentation saying that events queued for a fetch slot lose nothing.

Requested change: establish a source lease before a task can wait behind remote work, for example by reading/pinning admitted colocated bytes with an explicit bounded-memory policy, or by extending the store lease through queued processing. Add a regression test where the queue delay exceeds the media TTL and assert that an admitted attachment is still available. Update the queue documentation to match the selected overload behavior.

🟢 F3: Earlier attachment hardening remains intact

The current source retains the previously requested public wrappers, bounded MIME fallback, measured upload sizes, typed storage failure notes, and prompt-structure sanitization. The ticket/handoff tests cover reset during a parked Dispatcher send; the missing coverage is reset during the earlier semaphore and assembly phase.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch and merge base: main / c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 487e59d8bd5004e51a4db9a5a59e0ed8bb64ad7d
• Diff stat: 23 files changed, 2682 additions, 483 deletions
• Main already has: transcript-only audio handling, public Discord video links, and generic binary filestore uploads.
• Net-new value: audio metadata passthrough, Slack video presigning, typed attachment outcomes, and bounded gateway pre-dispatch assembly.
• Local validation: git diff --check origin/main...HEAD passed. Rust tests could not run because neither cargo nor rustc is installed in this environment.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway routing, MIME fallback, content type, documentation, and attachment work limits.

Those routing, MIME, content-type, and documentation fixes remain in the reviewed head. F1 and F2 are later scheduling consequences of moving gateway assembly out of the receive loop; they are distinct from the earlier duplicate-arm issue.

@antigenius0910

Earlier live validation confirmed generic-MIME routing and Slack video presigning on an intermediate head.

Those adapter-path improvements remain present. This review evaluates the later current-head gateway scheduler and does not dispute that earlier live validation.

@howie

Earlier feedback requested truthful Discord failure handling, measured stored sizes, and a voice-only rollback contract aligned with the new block shape.

The current source retains the corrected download-failure note, measured-size propagation, and transcript-based rollback documentation. The findings above concern the subsequent semaphore and reset implementation.

5. Three Reasons We Might Not Need This PR

1. Transcript-only delivery is simpler - it avoids retaining another fetchable media copy and the scheduling surface it introduces.
2. Gateway media is only fully actionable with filestore - without object storage, gateway adapters can provide metadata but no fetch target.
3. A common attachment scheduler may be preferable - it could define one lease, cancellation, ordering, and overload policy for every gateway attachment type before expanding passthrough.

What's Good

• Shared builders materially improve prompt-metadata safety and prevent drift across adapters.
• The ticket correctly preserves same-thread submission order and fences a reset during a parked Dispatcher handoff.
• The semaphore and shedding limit are useful bounds; the remaining issue is their interaction with stale work and short-lived colocated media.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The new gateway admission limit does not bound retained attachment bytes, and rejected-attachment reasons can still restructure the agent prompt.

What This PR Does

This PR makes inbound audio actionable independently of STT and gives Slack video a credential-free presigned URL when filestore is configured. It also centralizes attachment rendering, MIME handling, failure notes, and gateway pre-dispatch scheduling.

How It Works

Shared media helpers build sanitized audio/video blocks, preserve measured stored sizes, and select platform-specific fallback notes. The gateway moves attachment assembly out of the WebSocket receive loop, preserves per-thread arrival order, cancels stale work on /reset, and adds concurrency, pending-event, and source-byte limits.

Findings

#	Severity	Finding	Location
1	🟡	The 256 MiB source budget is charged from advisory attachment sizes before data is read, so under-reported attachments can retain far more memory than the documented limit.	crates/openab-core/src/gateway.rs:1479-1501
2	🟡	The new rejected-attachment prompt line sanitizes filename and MIME but interpolates the rejection reason verbatim; a Telegram reason can contain a user-controlled filename extension.	crates/openab-core/src/gateway.rs:1933-1937
3	🟢	Reset cancellation now covers stale preparation, releases its permit and budget, and prevents it from blocking a new session.	crates/openab-core/src/gateway.rs:1508-1529
4	🟢	Shared outcome selection continues to preserve measured upload sizes and truthful storage-failure notes across Discord, Slack, and gateway paths.	crates/openab-core/src/media.rs:1325-1391

Finding Details

🟡 F1: Charge the retained bytes, not an advisory size

event_bytes is the sum of GwAttachment.size, but the following task reads each path or decodes each base64 payload into retained Vec<u8> values. The documentation explicitly says the platform sizes are advisory. A gateway event reporting zero or too-small sizes can therefore pass the 256 MiB admission check while retaining much more data; cloning sources during assembly can increase the peak further.

Requested change: reserve a conservative upper bound before reading each event, or perform bounded reads that can charge actual bytes before retaining them. Reconcile the guard to actual retained bytes and shed/reject safely when the event cannot fit. Add a regression with under-reported attachment sizes proving the configured limit cannot be bypassed.

🟡 F2: Sanitize the rejection reason before it enters a prompt line

undelivered_attachment_line uses sanitize_attachment_meta for filename and MIME, then formats reason directly. The function is new in this PR, and its test exercises only a static "too large" reason. Telegram constructs a reason from the untrusted filename extension (format!("unsupported format: {ext}")), so Unicode line/paragraph separators or bidi controls can survive in the final [System: ...] text despite the new metadata sanitizer.

Requested change: apply the same single-line structural-character filtering to reason (with a suitable bounded text fallback) before formatting it. Add a test using a user-derived reason containing U+2028/U+2029 and bidi controls, and assert one safe prompt line.

🟢 F3: Full stale-work cancellation

The latest revision wraps the spawned preparation body in run_unless_reset, so cancellation releases the source-budget guard and fetch permit while preventing obsolete attachment work from reaching forum-topic creation or dispatch. The dedicated reset and source-survival tests cover the previously reported races.

🟢 F4: Consistent attachment outcomes

The shared decision function keeps the successful-upload measured size and distinguishes absent storage from configured-store failures. This preserves the earlier fixes for Discord download failures, Slack private URLs, and gateway no-store behavior.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and merge base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 9c83db9901c1fe2d324ac7857269e147725ef2a2
• Diff stat: 23 files changed, 2904 additions, 478 deletions
• Main already has: transcript-only audio handling, public Discord video links, and the generic binary filestore path.
• Net-new value: audio metadata passthrough, Slack video presigning, typed attachment outcomes, prompt metadata hardening, and bounded gateway pre-dispatch assembly.
• Validation: git diff --check origin/main...HEAD passed. Local Rust tests could not run because this environment has no cargo or rustc; the exact head has 43 completed GitHub checks, all successful or expected skipped.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback on gateway-arm drift, MIME fallback, content type, documentation, and attachment work limits is retained in the current head: the audio arm is shared, explicit non-audio MIME conflicts stay out of STT, gateway uploads preserve MIME, and the attachment matrix was updated. The new F1 is narrower: the newly added byte limit is not a real memory bound when declared sizes are inaccurate.

@antigenius0910

Earlier live validation of generic-MIME audio routing and Slack video presigning remains useful for those adapter paths. This review covers later gateway scheduling and prompt-construction changes at the new exact head.

@howie

Earlier findings about truthful Discord download failures, measured stored sizes, and the voice-only rollback contract are preserved as fixed in the current head. F1 and F2 concern the final pre-dispatch and prompt-sanitization changes added afterward.

5. Three Reasons We Might Not Need This PR

1. Transcript-only handling is simpler - It avoids retaining and serving another fetchable copy of sensitive media.
2. Gateway media is only fully actionable with object storage - Without filestore, gateway adapters can provide metadata but no fetch target.
3. A shared attachment transport redesign may be preferable - It could establish one bounded-memory, cancellation, and scheduling policy for every gateway attachment type before expanding passthrough.

What's Good

• The current ticket and reset fence address the stale-work and post-reset blocking defects found in earlier rounds.
• Shared audio/video rendering materially improves prompt metadata hygiene and avoids adapter outcome drift.
• The exact head has no whitespace errors and all completed remote checks are successful or expected skips.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Fix the gateway's bypassable source-memory limit and sanitize rejection reasons before they enter agent prompt lines.

Consolidated review: #1460 (comment)

[chaodu-obk]

🟡 F1 - Charge retained bytes, not advisory sizes

GwAttachment.size is documented as advisory, but this value is reserved before source paths are read or inline data is decoded. Under-reported sizes can therefore admit and retain far more data than the documented 256 MiB limit.

Requested change: reserve a conservative upper bound or use bounded reads and reconcile the guard to actual retained bytes; add a regression proving under-reported attachments cannot bypass the limit.

[chaodu-obk]

🟡 F2 - Sanitize the rejection reason before formatting it

Filename and MIME are sanitized, but reason is copied verbatim into a [System: ...] prompt line. Telegram can derive this reason from an untrusted filename extension, so Unicode separators or bidi controls can bypass the new structural-character hardening.

Requested change: filter and bound reason with the same single-line policy before formatting it, and add a user-derived U+2028/U+2029 and bidi regression test.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The new gateway source budget does not bound the actual peak or queued attachment memory.

What This PR Does

This PR makes inbound audio actionable independently of STT and routes Slack video through filestore when configured, while adding gateway scheduling and attachment-memory safeguards.

How It Works

Shared media helpers render sanitized attachment blocks and preserve typed storage outcomes. The gateway reads attachment sources before queued work, reserves a 256 MiB source budget, assembles blocks asynchronously, and uses ordering/reset controls before Dispatcher handoff.

Findings

#	Severity	Finding	Location
1	🟡	The 256 MiB source reservation covers only the original Vec<u8> values, but assembly clones each buffer and image conversion creates a base64 payload; the guards are then dropped before queued ContentBlocks enter the Dispatcher.	crates/openab-core/src/gateway.rs:327-352, 1619-1633

Finding Details

🟡 F1: Keep the memory bound valid through assembly and queueing

read_attachment_sources reserves a source-sized guard and retains the original buffers. assemble_attachment_blocks immediately calls bytes.clone() for every successful source. For images it then base64-encodes that clone into a ContentBlock::Image. The guard only covers the original source bytes, and _guards is dropped when the attachment-assembly branch returns, before extra_blocks is placed in BufferedMessage for Dispatcher queueing.

A near-limit image can therefore hold the reserved source, its uncharged clone, and its uncharged base64 payload at once; after assembly, the queued payload remains while the reservation is released. This contradicts the stated peak/retained-memory guarantee and permits substantially more than 256 MiB under attachment load.

Requested change: consume/move each source into assembly instead of cloning it, and account for any encoded/queued ContentBlock payload until it leaves the Dispatcher. If an output payload is intentionally outside this budget, add a separate explicit output/queue memory limit and test a near-limit image path.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch and merge base: main / c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 7c0bc5f3dd246d6885365cd121f9b348c027f366
• Diff stat: 23 files changed, 3109 additions, 478 deletions
• Main already has: transcript-only audio handling, public Discord video links, and generic binary filestore uploads.
• Net-new value: audio passthrough, Slack video presigning, typed outcomes, prompt-metadata hardening, and gateway pre-dispatch scheduling/resource controls.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway routing, MIME fallback, content type, documentation, and attachment work limits.

The current head retains the shared gateway audio path, bounded generic-MIME fallback, MIME-preserving upload path, and updated attachment documentation. This review found a separate accounting gap in the newly added source-budget implementation.

@antigenius0910

Earlier live testing verified generic-MIME audio routing and Slack video presigning on an intermediate head.

Those adapter-path improvements remain relevant, but this review independently evaluates the later gateway memory/scheduling changes at the current head.

@howie

Earlier feedback requested truthful Discord download failures, measured stored sizes, and an updated voice-only batching contract.

The current implementation retains typed download-failure handling, measured stored-size propagation, and the transcript-based batching documentation. None resolves the source-buffer clone and post-assembly queue accounting described above.

5. Three Reasons We Might Not Need This PR

1. Transcript-only handling is simpler - It avoids retaining and serving another fetchable media copy.
2. Gateway media is fully actionable only with filestore - Without it, gateway adapters provide metadata but no fetch target.
3. A shared attachment scheduler may be preferable - It could establish one ownership and memory-lifetime policy before extending media passthrough.

What's Good (🟢)

• Shared media builders materially improve prompt-structure hygiene and preserve typed storage outcomes.
• The ordering ticket and reset fence address the earlier asynchronous handoff races.
• Local git diff --check origin/main...origin/pr-1460-head passed. All 43 completed GitHub checks are successful or expected skipped.
• Local Rust tests were not rerun because this environment has no cargo or rustc executable.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Gateway pre-dispatch limits still fail to bound queued attachment work and do not fully account for memory retained during audio and text handling.

What This PR Does

This PR makes inbound audio actionable independently of speech-to-text and routes Slack video through filestore when configured. It also centralizes attachment metadata construction, MIME classification, failure reporting, and gateway pre-dispatch scheduling.

How It Works

• Shared media helpers build sanitized audio and video blocks, preserve typed storage outcomes, and use measured upload sizes.
• Discord and Slack attach an audio block alongside optional transcript output; Slack video uses a presigned URL when storage is available.
• Gateway adapters read attachment sources before delayed assembly, then use tickets, reset cancellation, a fetch semaphore, and a source budget before dispatch.

Findings

#	Severity	Finding	Location
1	🔴	The 32-event rule sheds fetches but not spawned tasks or their captured attachment payloads, so a blocked same-thread dispatcher can accumulate unbounded task-held data.	crates/openab-core/src/gateway.rs:1651-1675,1738-1743
2	🟡	The 256 MiB source budget charges audio only once, but filestore creates a second full Vec for ByteStream while the original remains live.	crates/openab-core/src/gateway.rs:101-104; crates/openab-core/src/filestore.rs:180-188
3	🟡	The 24 MiB inline cap charges every text-file source before deciding whether filestore will externalize it, so valid externalized text can incorrectly cause later inline attachments to be dropped.	crates/openab-core/src/gateway.rs:379-400,426-442
4	🟡	The gateway limits documentation says audio and video carry URL metadata, but the implementation explicitly rejects gateway video as unsupported.	docs/inbound-attachments.md:142,181-188; crates/openab-core/src/gateway.rs:403-495
5	🟢	The ticket and reset fence correctly preserve receipt order and cancel stale WebSocket-path preparation, and focused tests cover those prior races.	crates/openab-core/src/gateway.rs:560-685,1612-1755

Finding Details

🔴 F1: Bound admission, not only attachment fetching

tasks.len() only decides whether shed_attachment_blocks is used. The receive loop nevertheless admits every event, allocates an order ticket, and unconditionally calls tasks.spawn. Shed tasks still retain event.content.attachments while waiting for their predecessor; inline attachment data remains in that captured value. When Dispatcher::submit parks on its bounded per-thread queue, later same-thread tickets remain parked too, so a message burst can grow the JoinSet and retained payloads beyond the advertised 32-event guard.

Requested change: apply a bounded admission policy before spawning work and discard attachment bytes before the task captures the event when shedding. Add a test that holds a same-thread dispatch send and proves task count and retained attachment payloads stay bounded past the limit.

🟡 F2: Charge the audio upload copy

retained_upper_bound treats audio as source-only. Gateway audio passes &bytes to upload_bytes_and_presign, and upload_and_presign_with_content_type builds ByteStream::from(data.to_vec()). The upload copy coexists with the original source, which may then be consumed by STT. Four concurrent 20 MiB uploads add up to 80 MiB outside the stated source reservation.

Requested change: either reserve the transient upload copy or refactor the upload ownership so the original allocation is reused. Cover concurrent max-size audio uploads and assert the claimed 256 MiB bound includes the peak.

🟡 F3: Count only payload that will be inlined

The cap check runs before the text-file branch determines whether bytes exceed TEXT_INLINE_LIMIT and will be uploaded to filestore. A 20 MiB text file that becomes a small URL block consumes almost the whole 24 MiB inline allowance, so a later 4 MiB image is described as over budget despite the queued message containing only the image data and a text URL.

Requested change: decide the text delivery mode before charging the inline cap, charging only the branch that retains text in the outgoing block. Add a filestore-enabled mixed text-plus-image regression test.

🟡 F4: Keep the gateway video contract consistent

The same document first states that gateway video is rejected as unsupported, which agrees with the match lacking a video arm. Its pre-dispatch section later says gateway audio and video carry URL metadata. Operators cannot rely on both statements.

Requested change: correct the pre-dispatch text to describe the implemented gateway types, or implement and test the advertised video path.

🟢 F5: Prior ordering and reset fixes hold

The current WebSocket implementation takes tickets on receipt, uses a generation watch to cancel pre-reset work, detaches the old tail on reset, and limits active attachment fetches. The targeted ordering, reset-handoff, stale-work cancellation, and source-survival tests substantiate those earlier corrections.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and merge base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 6e07f6fb25ac496608babe82a8c4b5590c84f48d
• Diff stat: 23 files changed, 3266 additions, 478 deletions
• Main already has: transcript-only audio delivery, public Discord video URLs, and generic binary filestore uploads.
• Net-new value: audio passthrough across adapters, Slack video presigning, typed media outcomes, and gateway scheduling/resource controls.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway duplication, MIME fallback, content type, documentation, routing coverage, and attachment work limits.

The current head retains the shared gateway audio path, explicit-non-audio MIME handling, MIME-preserving upload, measured stored sizes, and updated attachment documentation. The remaining F1-F3 are new defects in the later scheduler and memory-accounting design, not a restatement of the earlier adapter-routing concerns.

@antigenius0910

Earlier live validation confirmed generic-MIME audio routing and Slack video presigning.

Accepted as useful validation for those adapter paths. It predates this exact head; the current findings concern later gateway pre-dispatch changes and do not dispute the observed Slack behavior.

@howie

Earlier feedback requested truthful Discord download-failure handling, measured stored sizes, the batching-contract update, and later review of gateway resource controls.

The current source preserves the earlier failure, size, and batching fixes. The final source-budget revision still leaves the task-admission, audio-copy, and externalized-text accounting gaps identified above.

Reviewer Aggregation

Reviewer group	Result
Gateway concurrency and memory review	F1, F2, F3
Documentation and contract review	F4
Correctness and regression review	F5

5. Three Reasons We Might Not Need This PR

1. Transcript-only delivery is simpler - It avoids retaining and serving another fetchable copy of potentially sensitive media.
2. Gateway media remains limited without object storage - Without filestore, gateway adapters can provide metadata but no fetch target.
3. A shared attachment scheduler may be a better prerequisite - It could establish one admission, ownership, and overload policy for every gateway attachment type before expanding passthrough.

What's Good

• Shared metadata sanitization and typed store outcomes retain the correctness and prompt-safety improvements from earlier rounds.
• The current source preserves the public MIME helper and existing Filestore compatibility boundary.
• git diff --check passed locally, and all 43 exact-head GitHub check runs completed successfully or were expected skips. This environment has no cargo or rustc, so local Rust tests were not rerun.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Bound gateway task admission and complete the attachment-memory accounting fixes.

Consolidated review: #1460 (comment)

[chaodu-obk]

🔴 F1 - Bound admission, not only attachment fetching

The 32-event check only chooses shed_attachment_blocks; this unconditional spawn still captures the attachment vector, including inline data, and queues behind the same-thread ticket when dispatch is backpressured. A burst can therefore grow task-held payloads without bound after shedding begins.

Requested change: enforce a bounded admission policy before spawning and discard attachment bytes before a shed task captures the event. Add a backpressured-dispatch regression test proving both task count and retained payloads remain bounded.

[chaodu-obk]

🟡 F2 - Include the audio upload copy in the reservation

Audio reserves only its source bytes, but this borrowed call reaches ByteStream::from(data.to_vec()), creating a second full buffer while the original remains live for optional STT. Four concurrent 20 MiB uploads add 80 MiB outside the documented 256 MiB limit.

Requested change: reserve the transient copy or refactor ownership to avoid it, then cover concurrent max-size gateway audio uploads.

[chaodu-obk]

🟡 F3 - Charge only text that is actually inlined

This check runs before the text branch decides whether a file above TEXT_INLINE_LIMIT will be externalized through filestore. The source bytes consume the 24 MiB inline allowance even though the queued block is only a URL, so a later image can be incorrectly replaced by a payload-limit notice.

Requested change: select the text delivery mode before charging the inline budget and add a filestore-enabled mixed text-plus-image regression test.

[chaodu-obk]

🟡 F4 - Make the gateway video contract consistent

This section says gateway audio and video carry URL metadata, but the earlier Unsupported Types section and assemble_attachment_blocks both reject gateway video. The two contracts cannot both be true.

Requested change: document only the implemented gateway attachment types here, or add and test the advertised video forwarding path.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The gateway source budget omits inline base64 payloads, and the 32-event policy still does not bound task admission.

What This PR Does

This PR makes inbound audio available independently of STT and routes Slack video through filestore when configured. It also adds shared attachment rendering, typed storage outcomes, and gateway pre-dispatch scheduling and resource controls.

How It Works

Discord and Slack build audio blocks alongside optional transcripts. Gateway events read attachment sources before asynchronous assembly, then use ordering tickets, reset cancellation, a fetch semaphore, a source budget, and attachment shedding before dispatcher handoff.

Findings

#	Severity	Finding	Location
1	🔴	Inline base64 input remains live but is not included in the advertised 256 MiB source reservation.	crates/openab-core/src/gateway.rs:142-184,253-265,1736-1753
2	🟡	The 32-event rule sheds attachment bytes but does not cap spawned tasks or queued event/prompt metadata.	crates/openab-core/src/gateway.rs:1703-1725
3	🟢	Shed events now discard attachment payloads before task capture, and upload/output copies are explicitly accounted for.	crates/openab-core/src/gateway.rs:253-265,567-570

Finding Details

🔴 F1: Account for the inline base64 representation

read_attachment_sources only borrows &[GwAttachment], decodes att.data into a new Vec<u8>, and keeps the owning event alive through assembly, ordering, and dispatcher submission. retained_upper_bound charges the decoded source plus an assembled payload or upload copy, but never charges the original base64 String.

For a 20 MiB inline audio attachment with filestore, the reservation is 40 MiB for decoded source plus upload body, while the still-live base64 input adds about 26.7 MiB. Refused attachments retain the same input because the Undeliverable path does not clear data. This defeats the stated memory bound for the backward-compatible inline-data path.

Requested change: include encoded input in peak accounting and consume or clear GwAttachment.data immediately after decoding or refusal. Add a regression that uses real inline data, asserts the full peak reservation, and proves rejected payload bytes are released.

🟡 F2: Bound admission rather than only attachment fetching

Once tasks.len() reaches 32, the receive path replaces attachment bytes with descriptive blocks, but it still admits an ordering ticket and unconditionally calls tasks.spawn. A blocked same-thread dispatcher handoff leaves later tickets parked, so an unbounded burst can accumulate task state, event text, and generated metadata even though attachment bytes were shed.

Requested change: choose and implement a bounded admission policy before spawning work, or replace one-task-per-event with bounded per-thread consumers. Add a test that blocks dispatcher handoff and proves task count and retained event metadata remain bounded beyond the threshold.

🟢 F3: Useful progress on the prior memory defects

The latest revision clears content.attachments before a shed task captures the event, reserves for upload copies, and differentiates inline payload accounting from filestore externalization. These are meaningful corrections, but they do not cover the retained base64 source or task admission above.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch: main
• Base commit and merge base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: 0587c93b26c51bea56f2e095d46cf306ea671b1d
• Diff stat: 23 files changed, 3416 additions, 479 deletions
• Main already has: transcript-only audio handling, public Discord video URLs, and generic binary filestore uploads.
• Net-new value: audio passthrough, Slack video presigning, typed media outcomes, prompt-metadata hardening, and gateway pre-dispatch scheduling/resource controls.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier feedback covered gateway routing, MIME fallback, content type, documentation, and attachment work limits.

The current head retains the shared gateway audio path, bounded MIME fallback, MIME-preserving uploads, and the updated attachment matrix. This round does not re-raise those resolved adapter-path concerns.

@antigenius0910

Earlier live validation confirmed generic-MIME audio routing and Slack video presigning on an intermediate head.

Those adapter-path results remain relevant. The findings here concern later gateway scheduling and memory-limit changes, not the validated Slack routing.

@howie

Earlier review requested gateway resource controls and highlighted memory/admission risks.

The latest change correctly discards shed attachment payloads and charges several transient copies. F1 shows that the inline base64 representation remains outside that accounting, and F2 remains open because task admission is explicitly unbounded.

@ShinyChang

The PR description notes that unbounded spawned-task admission is not yet fixed pending a product/design decision.

That acknowledgement is accurate but does not make the resulting resource exposure non-blocking. Please select and implement a bounded policy before merge.

5. Three Reasons We Might Not Need This PR

1. Transcript-only delivery is simpler - It avoids another fetchable media copy and the gateway scheduling machinery needed to prepare it.
2. Gateway media is incomplete without object storage - Without filestore, gateway adapters can provide metadata but no fetch target.
3. A common attachment scheduler may be the better prerequisite - It could define one bounded admission, ownership, and cancellation policy for every attachment type before expanding passthrough.

What's Good (🟢)

• Shared builders and typed outcomes preserve the earlier correctness and prompt-safety improvements.
• The ticket/reset fence and shed-payload release address several prior asynchronous and retention defects.
• git diff --check origin/main...0587c93b passes. All 41 completed GitHub checks are successful or expected skips.
• Local Rust tests could not run because this review environment has no cargo executable.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Gateway resource limits can be bypassed or silently corrupt an attachment on the current head.

What This PR Does

This PR makes inbound audio available as attachment metadata whether or not speech-to-text is enabled. It also routes Slack video through filestore when available, so agents receive a fetchable presigned URL instead of an unusable private Slack URL.

How It Works

Shared media helpers classify audio, sanitize attachment metadata, preserve typed storage outcomes, and render measured upload sizes. Discord and Slack add audio blocks beside optional transcripts; gateway paths prepare attachment blocks, optionally upload gateway audio to filestore, and use WebSocket-side ordering, reset, and resource controls.

Findings

#	Severity	Finding	Location
1	🟡	The 24 MiB inline limit accounts for source bytes, but lossy UTF-8 conversion can expand malformed text to roughly three times that size before it is queued.	crates/openab-core/src/gateway.rs:438-489
2	🟡	The source-read cap accepts a prefix as a successful attachment if a colocated file grows after its metadata length is read.	crates/openab-core/src/gateway.rs:368-378
3	🟡	Unified ingress creates a fresh source budget for every concurrently spawned event, bypassing the global admission, byte, and fetch limits implemented for the WebSocket path.	crates/openab-core/src/gateway.rs:2092-2108
4	🟡	Unified /reset, /cancel, and config commands assemble attachments before command dispatch, so an attached audio command can upload and transcribe work that is then discarded.	crates/openab-core/src/gateway.rs:2092-2139

Finding Details

🟡 F1: Charge the rendered text, not only its input bytes

inline_payload_bytes charges a text file by bytes.len(), then the text branch uses String::from_utf8_lossy. A file containing invalid UTF-8 can replace each malformed byte with the three-byte replacement character. For example, a 20 MiB malformed text attachment passes the 24 MiB check yet produces an approximately 60 MiB prompt block. This violates both the stated per-message cap and the source reservation assumptions.

Requested change: calculate the queued text payload after conversion, or conservatively account for the maximum lossy expansion before admitting it. Add a malformed UTF-8 boundary test that proves the rendered block cannot exceed the inline limit.

🟡 F2: Reject a source that exceeds its reserved length

source_upper_bound records the colocated file length, then read_at_most uses file.take(limit).read_to_end. take(limit) returns success after reading a prefix and never checks EOF. If the file is replaced or grows between metadata and the read, audio, text, or image handling proceeds with a truncated payload and no failure note.

Requested change: detect a byte beyond the reservation and render a size/budget failure rather than publishing a corrupted partial attachment. Cover a file that grows after the metadata check.

🟡 F3: Give unified ingress the same shared admission boundary

The unified bridge creates a task per incoming event, while process_gateway_event constructs SourceBudget::new(MAX_ADMITTED_SOURCE_BYTES) inside each task. The WebSocket path instead shares one budget, applies a 256-event admission limit, and limits concurrent fetches. Under concurrent unified webhook traffic, every task can independently reserve up to 256 MiB and start storage work.

Requested change: put the shared budget, admission counter, and fetch semaphore in GatewayEventContext (or an equivalent shared ingress state), and apply the same refusal/shedding policy before spawning unbounded preparation work.

🟡 F4: Intercept unified control commands before attachment assembly

The unified path calls read_attachment_sources and assemble_attachment_blocks before checking /reset, /cancel, or config commands. Unlike the WebSocket path, a command carrying audio can therefore execute new filestore and STT work only to return without dispatching its blocks.

Requested change: parse and handle control commands immediately after the event passes trust gating, before reading or uploading any attachment. Add a regression that verifies an attached /cancel or /reset causes no attachment processing.

Baseline Check

• PR opened: 2026-07-28T04:20:58Z
• Base branch and merge base: main / c5a75ac6e8fdc11a3b229a0c609769e90d261daf
• Reviewed head: a90bb6cc09975a24c4001be85a7dc1df9563812a
• Diff stat: 23 files changed, 3521 additions, 480 deletions
• Main already has: transcript-only audio handling, Discord CDN video metadata, and generic filestore uploads.
• Net-new value: audio passthrough across adapters, Slack video presigning, typed attachment outcomes, and gateway resource controls.

Addressing External Reviewer Feedback

@dogzzdogzz

Earlier review covered gateway-arm drift, MIME fallback, content type, documentation, and attachment work limits.

The current head retains the shared gateway audio helper, extension fallback limited to missing/generic MIME, real gateway audio content type, and updated attachment documentation. The current findings concern later resource-control code and the unified ingress path.

@howie

Earlier review requested truthful Discord download failures, measured stored sizes, and an updated voice-only rollback contract.

The current source preserves typed DownloadFailed handling, measured stored-byte propagation, and the transcript-based voice-only documentation. These do not protect the malformed-text expansion or the colocated-file growth case identified above.

@antigenius0910

Earlier live validation covered generic-MIME audio routing and Slack video presigning on an intermediate head.

Those observations remain useful for the Slack adapter paths. This review concerns the current gateway attachment lifecycle and resource bounds.

Reviewer Aggregation

Reviewer group	Result
Gateway concurrency and resource review	F1, F2, F3, F4
Adapter and storage contract review	No additional blocking finding

5. Three Reasons We Might Not Need This PR

1. Transcript-only handling is simpler - It avoids retaining another fetchable media copy and the gateway scheduling/resource surface needed to prepare it.
2. Gateway media remains incomplete without object storage - Without filestore, gateway adapters provide metadata but no fetch target.
3. A shared gateway attachment scheduler may be the better prerequisite - It can establish uniform admission, reset, ownership, and memory policies for WebSocket and unified ingress before this feature expands media handling.

What's Good

• Shared audio/video builders improve prompt metadata hygiene and avoid adapter formatting drift.
• MIME fallback respects explicit non-audio types, and stored attachments now preserve measured byte counts and failure reasons.
• The WebSocket path has focused ordering, reset, shedding, and memory-accounting tests; exact-head GitHub checks are successful or expected skips.

Validation

• git diff --check origin/main...pr-1460 passed locally.
• GitHub reports 43 completed check runs for this head, all successful or expected skips.
• Local Rust tests could not be rerun because cargo and rustc are not installed in this environment.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Gateway resource limits can be bypassed or silently corrupt an attachment on the current head.

Consolidated review: #1460 (comment)

[chaodu-obk]

🟡 F1 - Account for rendered lossy text

This cap charges bytes.len(), but String::from_utf8_lossy below can turn each malformed byte into a three-byte replacement character. A 20 MiB invalid UTF-8 text file therefore passes the 24 MiB check and can queue about 60 MiB.

Requested change: charge the post-conversion text length (or a conservative maximum expansion) and add an invalid-UTF-8 boundary test.

[chaodu-obk]

🟡 F2 - Do not publish a capped prefix as a complete attachment

take(limit) stops successfully at the metadata length and never verifies EOF. If the colocated source grows or is replaced after source_upper_bound, this returns a truncated prefix that audio, text, or image handling treats as a successful file.

Requested change: probe for one additional byte and report a size/budget failure rather than processing a partial payload.

[chaodu-obk]

🟡 F3 - Share unified ingress resource limits

This allocation is per process_gateway_event call, while the unified bridge spawns one call per incoming event. Unlike the WebSocket path, concurrent unified events therefore do not share the 256 MiB budget, event admission limit, or fetch semaphore.

Requested change: move these guards into shared GatewayEventContext state and apply admission before unbounded preparation tasks are spawned.

[chaodu-obk]

🟡 F4 - Handle unified control commands before attachment work

The unified route assembles blocks before the /reset, /cancel, and config-command checks below. An attached audio control command can therefore run new filestore and STT work that is discarded instead of short-circuiting like the WebSocket path.

Requested change: intercept control commands before reading or uploading attachments, and cover an attached /cancel or /reset regression.

[github-actions]

Caution

This PR has been waiting on the author for more than 2 days (labeled pending-contributor since 2026-07-30).
It will be automatically closed in 24 hours if there is no update.

@ShinyChang — You must add a new comment on this PR to remove the closing-soon label and keep it open. Pushing commits alone is not sufficient. Feel free to reopen a new PR later if it gets closed and you want to pick it back up.

[ShinyChang]

Still active. Here is where this stands.

What changed in the last round

Head is now f3946d7d, which addresses all four findings from the last review:

• The unified ingress path had none of the WebSocket path's controls.
  process_gateway_event was constructing its own source budget inside each
  call, so N concurrent events each got the full allowance rather than sharing
  one. Both ingress paths now share a single set of limits (source budget, fetch
  semaphore, in-flight counter with an RAII guard) held on the event context,
  and refusal uses the same overload reply the WebSocket path already used.

• Attachment work ran above slash-command interception. /cancel with a
  voice note still uploaded the file and ran speech-to-text before returning
  without dispatching. The attachment work now sits below the command branches,
  so each command returns before any attachment code is reachable.

• Lossy text was charged its input size, not its rendered size.
  String::from_utf8_lossy emits three bytes per malformed byte, so a 20 MiB
  malformed input renders to roughly 60 MiB while being charged 20 MiB, past the
  cap. Accounting now detects invalid UTF-8 before the conversion and charges the
  expanded size. Deliberately not a flat 3x for all text: that would refuse valid
  large text files that render unchanged, so the validity check costs one scan
  and keeps the common case exact.

• A truncated read was reported as success. take(limit).read_to_end stops
  at the limit and returns Ok, so a colocated file replaced between the
  metadata call and the read yielded a silent prefix with no failure note. It now
  reads limit + 1 and fails when the extra byte arrives.

Three of the four carry a test that goes red when the fix is reverted. The
command reordering is the exception: proving it at runtime needs adapter,
dispatcher and router doubles that this module's tests do not have, and that
fixture is a larger piece of work than the fix itself. That gap is stated in the
PR body rather than papered over.

Gates on this head: full workspace suite passes except one pre-existing
macOS-only failure that fails identically on main, and clippy is clean except
for one pre-existing error also present on main. The gateway tests pass both
with and without the filestore feature, and the unified build is green.

What is blocking

The PR carries review-limit-reached, and the OpenAB PR Review commit status
is error: Circuit breaker: exceeded 30 review cycles. Both review paths skip
PRs carrying that label, so no further automated review can arrive, and nothing
in the diff can clear it. A maintainer would need to remove the label or override
the status.

One question on scope

Most of the recent rounds have been ingress resource controls rather than the
audio and Slack video passthrough this PR set out to do. I am happy to split
those controls into their own PR built on the shared gateway attachment scheduler
that has been suggested across several rounds, leaving this one as the
passthrough slice it started as. A steer either way would help before I put more
work in.