feat(attachments): deliver images to the model and share one ingestion layer

[pepmach]

Part 1 of #923. Discord's adapter follows in a second PR; this one makes images work everywhere and builds the seam that adapter plugs into.

Problem

Images sent from any surface never reached the model as vision input.

AcpProvider.start() replaces AcpClient with AcpSessionProvider (providers/acp.py:624), and AcpSessionHandle.prompt() hardcoded a single text block (acp/session_handle.py:396). The only code that built an ACP image block lived on AcpClient — the path that had just been replaced (acp/client.py:3644).

So Slack downloaded an image to a temp file and passed the filesystem path as prose. The dashboard did the same via ![image](path). The model could only see the picture by opening that path with a tool, and only while the temp file survived — Slack deletes it when the turn ends, so replaying history yields a dead path.

Why it matters

Sending a screenshot is one of the most natural things a user does in a chat channel. It silently did nothing useful, with no error and no explanation.

Fix (symptom → root cause → change)

The symptom was "the model ignores my image". The root cause was two prompt paths where only the dead one could encode images. The change makes prompt_blocks.build_prompt_blocks() the single builder used by both, so they cannot drift apart again.

AcpSessionHandle.prompt() now emits real image blocks, gated on promptCapabilities.image — which the handshake previously parsed and threw away (acp/runtime.py kept only loadSession). When the agent does not advertise image support, the path stays in the text as a tool-openable reference rather than being dropped, which is a strictly better fallback than silence.

Two further defects surfaced while writing the tests:

• Multi-image messages lost every image. The legacy regex was greedy with \s in its character class, so /tmp/a.png and /tmp/b.png matched as ONE span ending at the final .png — not a file, so all images were skipped. This was live on Slack, not theoretical. Fixed with a non-greedy quantifier; filenames containing spaces still work.
• .svg was unreachable by accident. It sat in the media map while the regex omitted it. Now excluded deliberately and documented: scriptable XML, not a raster format.

Shared ingestion layer

messaging/attachments.py owns everything that happens after bytes land — classification, caps, magic-byte validation, redaction, document extraction via doc_parser, SEL audit, temp cleanup — behind a channel-supplied download callback. Host allowlisting and auth deliberately stay channel-side, because only the channel knows them (Slack needs a bearer token; Discord needs none but signed CDN URLs).

slack/files.py becomes a thin adapter over it and keeps its (image_paths, text_blocks) contract, so events.py and the busy-message queue are untouched. Its existing test suite is the evidence the migration is faithful.

Three holes closed relative to the original Slack implementation:

Hole	Before	Now
Size cap	trusted the channel's size, which defaults to 0 when absent — a missing or dishonest value bypassed the cap entirely	re-checked on the downloaded bytes
Content type	filename/metadata only	validated by signature; the true type wins, so a real JPEG mislabelled image/png still works and its temp file is retyped so the encoder emits truthful mimeType, while a script claiming to be a PNG is rejected (CWE-434)
Count	unbounded — one message could trigger unlimited downloads	per-message cap

Rejections are returned, not swallowed. Silently dropping an attachment is the defect this work exists to fix. Video is rejected with a visible reason rather than ignored: kiro-cli advertises promptCapabilities.image only (docs/kiro-cli/acp.md:135-146), and the models behind it do not accept video.

The security-posture allowlist moved with the code: messaging/attachments.py is registered as inbound sanitisation, and slack/files.py was removed because it no longer calls a redactor. The drift guard caught that staleness on its own, which is a good sign it works.

Tests

• test_acp_prompt_blocks.py (17) — block shapes and ordering, the capability gate's false branch, over-cap fallback, multi-image, same-path dedupe, every supported suffix → mime, SVG exclusion, bare filenames not probed as paths, directory-with-image-suffix not read
• test_messaging_attachments.py (38) — post-download size enforcement with lying metadata, masquerading content rejected, mislabelled-but-valid retyped, RIFF/WAVE not mistaken for WebP, redaction before truncation, video/unsupported rejection wording, count cap, no temp-file leak on download failure, one bad attachment not losing the others, safe_suffix never yielding a path component
• test_slack_files.py — three assertions updated where silence deliberately became a visible rejection (missing URL, too-large, download failure), and the PNG fixtures now carry real 8-byte signatures instead of the \x89PNG shorthand. The audio case stays silent on purpose: audio is transcribed on a separate upstream path.

Full backend suite: 22,084 passed. isort / flake8 clean; mypy clean across 565 files.

Manual verification

N/A for UI — backend only, no user-visible surface changed, so no screenshots.

The wire shape was verified directly rather than only through mocks: a real 1×1 PNG produces {"type":"image","mimeType":"image/png"} whose base64 round-trips to the original bytes; the capability-gated path preserves the reference as text; over-cap and missing-file cases fall back without dropping the reference.

Pre-existing failures (not from this PR)

Verified on a clean origin/main worktree, all in files this PR does not touch:

• test_sandbox_*, test_source_providers, test_issue_radar_gh_bin, test_beacon, test_cli pidfd, test_deploy_round30_fixes hardlink — host capability (unshare unavailable, hardlink EPERM) and host install-path assumptions.
• test_telegram.py::test_concurrent_queue_adds_share_one_receipt — genuinely flaky concurrency race; fails on clean main too (2/5 there, 4/5 here across isolated repeats). No Telegram or queue code is touched by this PR.

Correction: an earlier revision of this description listed test_slack_files.py::test_text_temp_file_cleaned as a pre-existing flake. That was wrong. It failed in CI with ModuleNotFoundError: No module named 'kiro_crew.slack.files.tempfile' because this PR moved temp handling into the neutral layer, so the patch target stopped existing — the failure was mine. The same root cause explains the local symptom: patch("...slack.files.tempfile.mkstemp") reaches the global tempfile module, so the "exactly 1 temp file" assertion was counting unrelated subsystems' temp files, including the SEL audit's. Now targets the neutral layer and scopes the count to the attachment under test. The file passes in full.

Review round (all four findings were real)

Each was validated against the code with file:line evidence before any change; none was rebutted. Two were fixed with a different remedy than prescribed, for reasons stated in the disposition comment.

• Image reads now go through hooks.safe_read_file_bytes. The raw read_bytes() was inherited from the old encoder, but this PR changed that code from dead to live, so the gate is this PR's responsibility. Paths come from message text and are therefore user-influenced. Did not make the builder async: the gate is synchronous and so was the read it replaces, so awaiting would churn both call sites while changing nothing about blocking.
• Document parsing moved off the event loop via asyncio.to_thread. Traced with no thread boundary: Socket Mode → _route_message → ingest_attachments → extract_text, so a large PDF stalled every session. Deliberately not subprocess_executor(), whose workers are documented as reserved for PTY teardown and orphan reaping.
• Windows paths now match. Not speculative — the Windows CI shards failed my own new tests (assert ['text'] == ['text', 'image']), because tmp_path yields C:\... there. Implemented platform-gated: \ and : are legal POSIX filename characters, so one merged pattern would let merely-mentioned prose like C:\docs\logo.png match a real file in the CWD.
• Slack voice memos no longer get a spurious video rejection. Slack ships them as video/webm — the repo's own pre-existing _AUDIO_MIMETYPES already encoded that — so a transcribed memo also received an "unsupported video" note. Root cause was two sources of truth; SLACK_AUDIO_MIMETYPES is now defined once and classify() accepts a channel-declared override. The override is opt-in and scoped: video/mp4 is still rejected, with a test pinning it.

Follow-ups (not in scope)

• Discord adapter — PR 2 of Inbound attachments: channels drop files, and images never reach the model as vision #923. InboundMessage.attachments already exists and DiscordInboundMessage already inherits it, so no shared-dataclass change is needed.
• Telegram / Teams / Webex / WeCom adapters.
• Whether unsupported binaries should be stored-and-referenced rather than rejected.
• A retention/TTL policy for downloaded files; today cleanup is caller-driven.

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging e88788206fb3c27fb752891620c4d00810e6c449.

Second-order review for e88788206fb3c27fb752891620c4d00810e6c449; this comment is updated in place on each push.

Review details

Both line-level reviewers (Opus 5 and GPT 5.6) reported no findings, so the only sub-threshold material is the design reviewer's two items: the prose round-trip / regex path-scraper contract concern, and the duplicated 10 MB constant. Judging each against the narrow bar:

Prose round-trip contract (design reviewer "Watch"). The channel→provider seam is an internal, in-process contract — not a persisted schema, wire format, or public API. Passing image_paths structurally through prompt() later requires only coordinated edits within this repo (the InboundMessage.attachments type already exists), with no migration or breaking rollout. The "third channel hardens the convention" risk is real architectural-erosion pressure, but the Discord adapter isn't in this PR, and erosion-that-compounds is explicitly a follow-up, not a block. The side observation that a merely-mentioned local image path still gets read and shipped is materially mitigated by this diff (lookbehind guards, safe_read_file_bytes sensitive-path gate, capability gating) relative to the pre-existing direct-client scraper — this PR narrows, not worsens, that surface, and the residual is a design-hardening suggestion, not a concrete security regression with a named trigger.

Duplicated 10 MB cap (design reviewer "Suggestion"). A one-line constant import; a future drift would cause an oversized image to fall back to a path reference (with a warning logged), not data loss or a crash. Reversible any time.

Neither item is a one-way door or concrete harm caused by this diff.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

• Pass image paths structurally through prompt() before the Discord adapter lands — design reviewer's Watch item: ingestion already produces structured image_paths, but the channel flattens them into prose and build_prompt_blocks regex-mines them back out; two live defects (greedy quantifier, \s-newline chaining) came from exactly this shape. Safe to wait because it's an internal seam reversible in one coordinated change, but it should land before a third channel (Discord) hardens the prose convention. Fix in acp/session_handle.py / acp/prompt_blocks.py plus the channel call sites, keeping the regex as a fallback for dashboard/free-text references.
• Unify the 10 MB image cap — prompt_blocks.MAX_IMAGE_BYTES and IngestLimits.max_image_bytes match by convention only ("Matches the Slack producer cap"); import one from the other so a future limit bump can't silently reopen the drop-at-encoder gap. One-line change in acp/prompt_blocks.py or messaging/attachments.py; safe to wait because drift today only degrades to sending the path instead of the inline image, with a warning logged.

[ARBITER-REVIEWED] e887882

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter e88788206fb3c27fb752891620c4d00810e6c449: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e88788206fb3c27fb752891620c4d00810e6c449 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e887882

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e88788206fb3c27fb752891620c4d00810e6c449: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of e88788206fb3c27fb752891620c4d00810e6c449 — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound seam and real fix, but images still round-trip through prose — the regex path-scraper stays the load-bearing contract, and it's a proven defect magnet.

Watch

• The channel→provider contract remains a single string: ingestion produces structured image_paths, the channel flattens them into message text, and build_prompt_blocks regex-mines them back out. This PR itself documents two live defects born from exactly that shape (greedy quantifier, \s-newline chaining) and needs platform-gated POSIX/Windows grammars plus lookbehinds to keep merely-mentioned paths from being inlined — any existing local image file whose path appears in prose still gets read and shipped to the model (safe_read_file_bytes only fences sensitive paths). With InboundMessage.attachments already existing and the Discord adapter about to plug into this seam, consider passing image paths structurally through prompt() before a third channel hardens the prose convention; the regex can remain as a fallback for dashboard/free-text references.

Suggestions

• prompt_blocks.MAX_IMAGE_BYTES and IngestLimits.max_image_bytes are the same 10 MB by convention only ("Matches the Slack producer cap"); import one from the other so a future limit bump can't silently reopen the drop-at-encoder gap the alignment exists to prevent.

[DESIGN-REVIEWED] e887882

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed e88788206fb3c27fb752891620c4d00810e6c449 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e887882

Verdict parsed from the review's SHA-scoped output markers for commit e88788206fb3c27fb752891620c4d00810e6c449.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e88788206fb3c27fb752891620c4d00810e6c449: <one-sentence reason>

[pepmach]

Disposition for 8d274cf7 → fixed in b3075726

All four findings were validated against the real code before I touched anything. Three were correct and are fixed; on two of them I implemented a different remedy than prescribed, explained below. Nothing was rebutted as a false positive.

1. BLOCKING — image reads bypass the sensitive-path gate → FIXED, correct finding

Confirmed. hooks.safe_read_file_bytes already existed on main (src/kiro_crew/hooks.py:977) and gating externally-influenced paths is the established convention here, not an optional extra:

• dashboard/handlers/files.py:1401 → _validate_dashboard_path → hooks.validate_file_path
• apps/builtins/file_explorer/server.py:922 → _safe_path + safe_read_file_bytes
• deploy/handlers.py:552 → is_sensitive_path + safe_read_file_bytes

Worth being precise about provenance: the raw read_bytes() was inherited — the original encoder in acp/client.py did the same and was never gated. But that does not excuse it here, because this PR changed its reachability from dead to live. AcpProvider.start replaces AcpClient, so the old encoder never ran in production; the shared builder now runs on AcpSessionHandle. I widened the blast radius, so the gate is mine to add.

Paths reaching the builder are scraped from message text, which is attacker-influenceable on any inbound channel, so this is a genuine sink.

Divergence: I did not make the builder async and offload the read. safe_read_file_bytes is synchronous (hooks.py:977), and the read it replaces was already synchronous on this path, so awaiting it would ripple an API change through both prompt call sites while changing no blocking characteristics. The finding conflated the security gate with an async offload; only the gate is load-bearing, and the read stays bounded by the pre-existing 10 MiB stat check. Applied the gate, skipped the API churn.

Tests: TestSensitivePathGate — a refusal produces no image block and leaves the path in the text; the gate receives the path; the encoded payload provably comes from the gate rather than a second unguarded read.

2. BLOCKING — document parsing blocks the gateway event loop → FIXED, correct finding

Confirmed with a full call trace, no thread boundary anywhere: Socket Mode _on_event (slack/events.py:802) → _route_message (:899) → process_slack_files (:2198) → ingest_attachments → extract_text. It runs as an asyncio task on the gateway loop, so a large PDF stalls every other session's streaming. Input is capped at 20 MiB, which bounds but does not eliminate the stall.

Also inherited — main's slack/files.py:_download_document called extract_text synchronously inside an async def too. Fixed here because this PR centralizes that code, making it a one-line fix at the new chokepoint instead of a defect spread across future adapters.

Divergence: used asyncio.to_thread, not subprocess_executor(). That pool is explicitly reserved for subprocess/PTY teardown and orphan reaping, and its own docstring states those workers must stay free so a teardown blocked on a wedged kernel resource can still be recovered from. Parking a slow document parse there would defeat that isolation for no benefit — extract_text is pure-Python parsing and spawns nothing. maintenance_executor is likewise documented as "reserved for the fast periodic sweeps", which a 20 MiB parse is not. If a dedicated parsing pool is wanted, that is worth doing deliberately rather than borrowing one with a conflicting contract.

3. FINDING — /-anchored regex cannot match Windows paths → FIXED, and it was real

Not merely plausible — CI proved it. My own new tests failed on the Windows shards:

test_acp_prompt_blocks.py: assert ['text'] == ['text', 'image']

tmp_path yields C:\...\tmp.png on Windows, the POSIX grammar matched nothing, so no image block was produced. On the real gateway the temp file is then deleted at end of turn, leaving a dead prose reference. Windows has four dedicated CI lanes, so this is a supported platform silently losing the PR's central feature.

Implemented as platform-gated, which the finding did not specify and which matters. Merging Windows shapes into one pattern would regress POSIX: \ and : are legal POSIX filename characters, so prose such as the path C:\docs\logo.png is an example becomes a match candidate, and a file with that literal name can exist in the CWD — inlining a file the user only mentioned. _PATH_RE now selects _WINDOWS_PATH_RE or _POSIX_PATH_RE by os.name, covering drive-letter, forward-slash-drive, and UNC forms.

Tests: TestPlatformPathGrammar, including an explicit assertion that Windows-looking prose is not recognized on POSIX, plus an end-to-end case using a natively-produced path so each host verifies its own grammar.

4. FINDING — video/webm voice memos get a spurious video rejection → STILL VERIFYING

Taking this one seriously rather than guessing, because the consequence is user-visible: a rejection note stapled onto a transcript. Verification of the actual Slack/Discord voice-memo mimetypes and whether such a file even reaches process_slack_files (transcription happens upstream) is still in flight. I will follow up on this comment with either a fix or evidence that the path is unreachable — it is not being dropped.

Unrelated CI failure, now fixed

test_slack_files.py::test_text_temp_file_cleaned was failing with ModuleNotFoundError: No module named 'kiro_crew.slack.files.tempfile'. Mine: temp handling moved into the neutral layer, so the patch target stopped existing. The same root cause explains a local failure I had wrongly written off as a pre-existing flake — patch("...slack.files.tempfile.mkstemp") reaches the global tempfile module, so the "exactly 1 temp file" assertion was counting unrelated subsystems' temp files, including the SEL audit's. Now targets the neutral layer and scopes the count to the attachment under test. The whole file passes.

[pepmach]

Finding 4 resolved in 96f9c3b3 — video/webm voice memos

Following up on the one item I left open. The finding was correct, and the repo's own code proves it rather than any external guess:

# src/kiro_crew/slack/events.py:1399 (pre-existing)
_AUDIO_MIMETYPES = {"audio/", "video/webm"}

Slack genuinely ships voice memos with a video container mimetype, and the transcription path has always relied on that. Confirmed double-processing: the same unfiltered files list goes to _transcribe_with_reaction (events.py:2175) and then to process_slack_files (:2198). handle_audio=False only skips attachments classified AUDIO, so video/webm fell through to the unsupported-video branch. Net user-visible effect: a transcript and a contradictory "video is not supported" note in the same turn.

Fix — root cause, not the symptom

The real defect was two sources of truth for "what Slack calls audio". Special-casing video/webm inside the neutral layer would have hard-coded one channel's quirk into channel-neutral code, and left the duplication in place for the next reader to trip over.

Instead:

• SLACK_AUDIO_MIMETYPES is now defined once in slack/files.py, which sits below events.py in the import graph, so the transcriber and the ingestion adapter share one definition. events.py:_AUDIO_MIMETYPES is now that constant — verified identical by object identity, not by eye.
• classify() / ingest_attachments() accept audio_mimetypes, so a channel declares its own quirks instead of the shared layer accumulating per-channel branches.

I deliberately did not take the "filter already-transcribed files before calling process_slack_files" option: that leaves the two mimetype lists free to drift, which is the actual bug.

Also honoured the review's warning not to over-generalize: the override is opt-in and scoped. video/mp4 is still rejected as video even with the override active — there is a test asserting exactly that, since blanket-classifying video/* as audio would silently accept real video the model cannot process.

Verification

End-to-end through the real Slack adapter, a video/webm memo now yields image_paths=[], text_blocks=[] — no rejection text beside the transcript.

Tests added: override beats the video prefix; the same type is still video without the override; video/mp4 unaffected by it; the declared type can be returned for transcription when handle_audio=True.

Full backend suite: 21,963 passed. The 5 remaining failures are host-environment (/tmp teardown races, hardlink EPERM, pidfd) plus one genuinely flaky Telegram concurrency test — all reproduced on a clean origin/main worktree, and my diff touches no Telegram or queue code.

[pepmach]

Round 2 disposition for 96f9c3b3 → fixed in 8caba3cf

Both findings were real. The second one is the most severe bug this PR has surfaced, and I reproduced it before changing anything.

1. BLOCKING — synchronous image reads block the event loop → FIXED

Correct, and a fair press on exactly what I deferred last round. I had argued the gate needed no async change because the read it replaced was already synchronous. That defended the gate decision but left the blocking read in place, and it is the same defect class I had just fixed for extract_text one file over — so declining it here was inconsistent.

Fixed by offloading the whole builder rather than making it async internally:

"prompt": await asyncio.to_thread(
    build_prompt_blocks, message, allow_image=self._runtime.supports_image_prompt
),

This covers the stat, the gated read, and the base64 encode in one hop, at both call sites (session_handle.prompt, client._send_prompt), and keeps build_prompt_blocks a synchronous pure function — so all 36 tests exercise it directly with no event loop.

2. BLOCKING — newline matching drops images that follow a URL → FIXED, and worse than described

Confirmed by reproduction, not inspection. slack/events.py emits "<user text>\n<image path>", so:

see https://example.com/docs
/tmp/tmpXXXX.png

matched as the single span //example.com/docs\n/tmp/tmpXXXX.png → not a file → blocks=['text'], image dropped, temp file then deleted at end of turn. Any Slack message containing a link lost its image. Links in Slack messages are common, so this was likely the most frequently-hit defect in the branch.

The prescribed fix is insufficient. [^\S\r\n] repairs the newline case but not the same-line one — see https://example.com/docs /tmp/a.png still captures //example.com/docs /tmp/a.png. I verified this directly against the suggested pattern before rejecting it.

Implemented instead: horizontal-whitespace-only class plus a (?<![\w:/]) lookbehind so a match cannot begin inside a URL or mid-path. Measured against 9 cases, this is the only candidate that passes all of them — including a bonus the finding did not ask for: a remote URL ending in .png is no longer probed as a local file at all.

case	before	reviewer's fix	shipped
url \n path	✗	✓	✓
url + space + path	✗	✗	✓
https://x.com/logo.png treated as a path	✗	✗	✓
filename containing spaces	✓	✓	✓
two paths, one line	✓	✓	✓
	✓	✓	✓

Both properties of that regex are now documented in-place with the defect each one prevents, since both were learned from real failures rather than reasoned about up front.

Tests added (8): url+newline, url+space, multiple URLs then a path, two images after a URL, a newline never appearing inside a captured path, remote .png URL not matched, filenames with spaces preserved, markdown-image syntax.

Verification

21,533 passed; isort/flake8 clean; mypy clean across 565 files. One failure, test_sandbox_backend_cache.py::test_fail_closed_transient_message_advises_retry_not_optout, reproduced on a clean origin/main worktree — host cannot unshare, and this PR touches no sandbox code.

[pepmach]

Round 3 disposition for 8caba3cf → fixed in 832a5c96

BLOCKING — cancelled preprocessing permanently marks the session active → FIXED, correct finding

Confirmed, and this one was introduced by my previous round's fix — offloading the builder with asyncio.to_thread added a cancellation point inside the turn-state guard.

The guard was except Exception, and the comment sitting directly above it already described this exact hazard ("_turn_done would stay cleared forever — is_turn_active would report True permanently and every future prompt() on this handle would be rejected"). But asyncio.CancelledError derives from BaseException, so it walked straight past the handler the comment was justifying. Now except BaseException: with an unchanged re-raise, so cancellation still propagates normally.

Worth noting for the record: await self._runtime.send_request(...) was already a cancellation point inside that same block on main, so the leak predates this PR. My change did not create the hole — it widened it materially, since prompt assembly now does file I/O (up to MAX_IMAGE_BYTES per image) and is therefore far more likely to be in-flight when a turn times out. Fixing it here is right either way, and the fix covers the pre-existing send_request case too.

Tests

Two added next to the existing send_request-failure test, one per cancellation point:

• test_prompt_resets_turn_done_when_cancelled — cancellation at the send await
• test_prompt_resets_turn_done_when_cancelled_while_building_blocks — cancellation at the new assembly await

Both were verified to be real guards rather than tautologies: with the handler temporarily reverted to except Exception, both fail; with BaseException, all five in that group pass.

Verification

21,511 passed, zero failures — the host-environment failures from earlier rounds are excluded by file, and nothing else regressed. isort/flake8 clean; mypy clean across 565 files.

[pepmach]

Round 4 disposition for bc98c481 → fixed in e8878820

BLOCKING — blocking close on the gateway event loop → FIXED, but the prescribed fix was insufficient

The concern is valid: _fetch runs synchronous filesystem work on the gateway event loop, and TMPDIR is not guaranteed to be local, so a network- or FUSE-backed temp dir can stall every session.

But the fix as written — await asyncio.to_thread(os.close, fd) — targets the cheaper of the two adjacent syscalls and would have left the stall in place:

fd, dest = tempfile.mkstemp(suffix=suffix)   # line 230: O_CREAT|O_EXCL + directory
                                            #   lookups, retried on collision
os.close(fd)                                # line 231: releases a descriptor whose
                                            #   file has NO dirty pages yet

Nothing has been written to the file when close runs, so there is no writeback to block on. On a stalled temp directory mkstemp hangs first and for longer. Offloading only line 231 moves the hang up one line rather than removing it, while making the PR look resolved.

Fixed by pairing both halves in a _make_temp() helper offloaded in one hop — one await instead of two, and it actually covers the described failure.

Same class, fixed for consistency

Auditing the rest of the async path found two more blocking calls that read real file content, which I had left inline while offloading extract_text last round. That split was arbitrary — a plain-text read blocks exactly as a PDF parse does:

• the TEXT branch's open(...).read() → _read_text_file(), offloaded
• sniff_image_mime(), which opens and reads the file header, offloaded

Left inline deliberately: os.path.getsize, os.replace, os.unlink. These are O(1) metadata operations with no data transfer, and the cleanup paths must stay simple and exception-safe. Flagging that as a judgement call rather than an oversight.

A pre-existing blocking call this surfaced (not fixed here)

Writing the guard below initially failed with two mkstemp calls — one on a worker thread (mine) and one still on the event loop thread. The second is the SEL audit subsystem, which writes .sel_hmac_*.tmp synchronously from _audit().

That is inherited — main's slack/files.py called sel().log_api_access(...) inline the same way — and it is a repo-wide security-audit chokepoint, so quietly rerouting it through an executor inside an attachments PR would be the wrong place for that change. Reporting it rather than widening this diff. The test is scoped to this module's own helper so it does not silently absorb that call site.

Tests — asserting the invariant, not the wrapper

Four guards in TestBlockingWorkLeavesTheEventLoop. They assert the call executes on a different thread than the loop, rather than asserting it was wrapped in to_thread, so they cannot be satisfied by a wrapper that is later unwrapped:

• temp creation off the loop thread
• text read off the loop thread
• image signature sniff off the loop thread
• the loop keeps making progress during a deliberately slow (150ms) blocking read

The fourth needed a second pass: as first written it passed with the offloads removed, because the awaited download yields to the loop on its own. It only discriminates once the blocking step is given real duration. Verified all four fail with the offloads reverted and pass with them restored.

Verification

21,548 passed, zero failures. isort/flake8 clean; mypy clean across 565 files. Single commit on b60d0eba.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #8238 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8238: CONTINUE_DEVELOPMENT. 8238 reverses a documented deliberate decision from 974 rather than fixing a defect in it, so the design owner should confirm the retirement explicitly. Files: src/kiro_crew/acp/prompt_blocks.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.