feat: attach images mentioned in the prompt - #521
Conversation
`@shot.png` reported "binary, skipped". That is never what mentioning an image means, and it was the only thing the TUI could say: an image cannot be inlined as text, and there was no path for one to reach the model. Mentioned images are now attached to the turn as image content blocks. The syntax is the one users already know, so nothing new has to be discovered, and the dead end it replaces was actively misleading. Engine side: a user message can carry attachments, and the engine holds blocks for the next turn. They are taken rather than read when the message is built — an attachment belongs to exactly one turn, and leaking it forward would re-send an image the user already shared. The setter replaces rather than appends, so a cancelled composer cannot accumulate images across attempts. Images are decoded when the turn starts rather than at mention time, so a large screenshot stays off the heap until it is needed and a read failure becomes a note instead of blocking the prompt. Only formats the API accepts are attached; other binaries still report why they were skipped rather than being sent as something unreadable.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0675944ae6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| notes.push(format!("@{raw} — attached as an image")); | ||
| images.push(path); |
There was a problem hiding this comment.
When a mentioned image is very large, or a prompt contains many images, this branch adds every path without incrementing used or applying any other limit. The run loop then calls image_block_from_file, which reads each entire file and creates an additional base64 allocation on the UI thread, so image mentions bypass both the 64 KiB per-file and 256 KiB total protections and can freeze or OOM the CLI before the provider request is sent. Apply an image size/count budget before accepting or encoding these paths.
Useful? React with 👍 / 👎.
| path.extension() | ||
| .and_then(|e| e.to_str()) | ||
| .map(|e| e.to_ascii_lowercase()) | ||
| .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) |
There was a problem hiding this comment.
For uppercase or mixed-case names such as the newly supported .PNG and .Jpeg, this check accepts the file after lowercasing its extension, but image_block_from_file later matches the original extension case-sensitively and assigns application/octet-stream. The resulting image block can be rejected or treated as an unsupported payload by the provider, so MIME inference must use the same case normalization as mention detection.
Useful? React with 👍 / 👎.
| Some(expansion) => { | ||
| mention_notes = expansion.notes; | ||
| self.pending_images = expansion.images; | ||
| expansion.prompt | ||
| } | ||
| None => text.clone(), |
There was a problem hiding this comment.
If an image-bearing prompt is staged and then replaced before it starts—for example, the user interjects twice while the current turn is being cancelled, with the second prompt containing no mention—this is the only assignment to pending_images; the None and skill branches leave the first prompt's paths behind. The run loop will therefore send the old image with the replacement prompt, potentially disclosing an unintended file. Reset the image list for every enqueue or store the images together with their corresponding pending prompt.
Useful? React with 👍 / 👎.
Image mentions bypassed every mention budget: each path was accepted without a size or count limit, then read whole and base64-encoded on the UI thread at turn start. Give attachments their own budget (3 MiB per image, 8 MiB and 4 images per prompt), enforced before a path is accepted and again at load time, since the file can grow in between. A file whose size cannot be read is refused rather than attached. Media-type inference matched the extension case-sensitively while mention detection lowercased it, so a mentioned shot.PNG was attached as application/octet-stream and rejected upstream. Only the mention branch assigned pending_images, so replacing a staged prompt (two interjections while a turn cancels) sent the previous prompt's image with the replacement. Clear on every enqueue, set the engine's attachments unconditionally so an empty set clears a stale one, and restore both prompt and images together when a spawn fails.
|
@codex review — bounded image attachments (3 MiB/image, 8 MiB + 4 images/prompt, re-checked at load, fail-closed on unreadable size), case-insensitive image media-type inference, and pending_images cleared on every enqueue so a replaced prompt cannot inherit the previous one's attachment. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9428001df6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // can grow in between, and this is the read that would | ||
| // actually allocate it. | ||
| let loaded = super::mentions::image_size_within_cap(path) | ||
| .and_then(|_| agent_code_lib::llm::message::image_block_from_file(path)); |
There was a problem hiding this comment.
For every mentioned image, this calls image_block_from_file, but that helper's Base64Writer only writes to encoded from flush() and the helper drops the writer after write_all() without flushing it. The resulting ContentBlock::Image therefore always contains an empty data string, so providers receive an invalid/empty image instead of the file the user mentioned; flush the encoder or replace it with an encoder that produces the payload during the write.
Useful? React with 👍 / 👎.
| let loaded = super::mentions::image_size_within_cap(path) | ||
| .and_then(|_| agent_code_lib::llm::message::image_block_from_file(path)); |
There was a problem hiding this comment.
When queued image files change before their turn starts, this rechecks only the 3 MiB per-file cap and never recomputes the 8 MiB aggregate cap. For example, four 2 MiB files accepted during mention expansion can each grow to 3 MiB while waiting, after which this loop loads all 12 MiB despite the advertised per-prompt bound; track the load-time total and skip blocks that would exceed MAX_TOTAL_IMAGE_BYTES.
Useful? React with 👍 / 👎.
The base64 helper only emitted output from `flush()`, and the single caller dropped the writer after `write_all`, so every image block shipped with an empty `data` string — the provider received no image at all. Replace the writer with a whole-input encoder and cover it with the RFC 4648 vectors, which is exactly where the padded remainders were lost. Attachment loading re-checked only the per-image cap, so four files that each grew from 2 MiB to 3 MiB after being mentioned would load 12 MiB against an advertised 8 MiB bound. Move loading into `load_image_blocks`, which re-applies the size, total and count limits and reads through a capped reader rather than trusting the size it just measured.
Merging the v0.28.0 release into this branch changed the version shown in the status bar; the four golden frames still asserted 0.27.0. Only that token differs — the style rows are untouched.
|
@codex review — fixed the empty base64 payload (the encoder only emitted from flush(), which no caller invoked; replaced with a whole-input encoder covered by the RFC 4648 vectors) and moved attachment loading into load_image_blocks, which re-applies the per-image, total and count budgets and reads through a capped reader instead of trusting the size it just measured. Also restamped the four golden frames for the 0.28.0 version line brought in by the main merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26be7b40b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `take(cap + 1)` so exceeding the cap is *observed* rather than silently | ||
| /// truncated — a half-read image would be sent as a corrupt attachment. | ||
| fn read_capped(path: &Path, cap: usize) -> Result<Vec<u8>, String> { | ||
| let file = std::fs::File::open(path).map_err(|e| format!("image unreadable ({e})"))?; |
There was a problem hiding this comment.
When an accepted image is replaced between mention expansion and the delayed turn start, this File::open follows the replacement without rechecking the workspace or .git/ containment enforced by resolve_mention. For example, replacing shot.png with a symlink to a file outside the workspace causes that file's bytes to be encoded and sent to the provider; replacing it with a FIFO can also block the UI indefinitely. Revalidate containment and regular-file status at load time, ideally while safely opening the validated target.
Useful? React with 👍 / 👎.
An image path is accepted during mention expansion but opened later, when the turn actually starts, and the loader trusted the earlier decision. Replacing the file in between defeated every check `resolve_mention` had made: a symlink to a file outside the workspace was read and encoded into the request, and a FIFO would have hung the UI thread inside `open`. Re-canonicalize at load time and re-establish workspace containment, the `.git/` exclusion, the extension and regular-file status on the resolved target, then open with `O_NOFOLLOW | O_NONBLOCK` and decide on the descriptor's own `fstat` — the name can change again after the check, but the opened file cannot.
|
@codex review — staged image paths are now revalidated at load time (re-canonicalize, workspace containment, .git/ exclusion, extension and regular-file status) and opened with O_NOFOLLOW | O_NONBLOCK with the decision made on the descriptor's own fstat, so a path swapped for an escaping symlink or a FIFO after mention expansion is refused rather than read or blocked on. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964c7c4ffd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn open_regular_file(path: &Path) -> Result<std::fs::File, String> { | ||
| let mut options = std::fs::OpenOptions::new(); | ||
| options.read(true); | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
On the supported Windows build, this cfg(unix) block is omitted, so OpenOptions::open follows reparse-point symlinks. If shot.png is replaced with an escaping symlink after revalidate_image returns but before the open, the descriptor metadata reports the outside target as a regular file and its bytes are sent to the provider—the exact TOCTOU this change intends to close. Use Windows reparse-point/no-follow handling and validate the resulting handle.
Useful? React with 👍 / 👎.
| options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); | ||
| } | ||
| let file = options | ||
| .open(path) |
There was a problem hiding this comment.
O_NOFOLLOW protects only the final component. If a staged path is images/shot.png and images is renamed and replaced by a symlink to an outside directory after canonicalization but before this pathname-based open, open follows that ancestor and fstat sees an ordinary outside file, allowing its bytes to escape the workspace. Resolve from a trusted workspace directory descriptor while refusing symlinks/beneath escapes for every component rather than reopening the canonicalized pathname.
Useful? React with 👍 / 👎.
# Conflicts: # crates/cli/tests/snapshots/idle_frame.txt # crates/cli/tests/snapshots/permission_modal.txt # crates/cli/tests/snapshots/transcript_basic.txt # crates/cli/tests/snapshots/transcript_light.txt
Attachments carried a path from mention expansion to the start of the turn, and re-opened it there. Nothing about a path survives that wait: replacing the file — or any ancestor directory — with a symlink pointed the later open at a file outside the workspace that had never been validated, and re-checking the name only moved the window rather than closing it, since the check and the open are two separate lookups. Read and encode the image where the text mentions are already read: immediately after `resolve_mention`, on the path it just validated. The turn then carries content blocks, not names, so there is nothing left to re-check and no second lookup to redirect. Peak memory stays bounded by the same budget as before, which is what made deferring the read unnecessary. `O_NOFOLLOW | O_NONBLOCK` plus the descriptor's own fstat keep the one remaining open honest: it cannot follow a symlink swapped in after the check, and it cannot block on a FIFO.
|
@codex review — merged main (masked goldens from #525) and closed both TOCTOU findings at the source: images are now read and encoded during mention expansion, on the path resolve_mention just validated, exactly as the text mentions already are. The turn carries content blocks instead of names, so there is no second lookup for a swapped final component or ancestor directory to redirect — on any platform — and the single remaining open uses O_NOFOLLOW | O_NONBLOCK with the decision made on the descriptor's own fstat. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432de70555
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let file = options | ||
| .open(path) | ||
| .map_err(|e| format!("unreadable ({})", e.kind()))?; |
There was a problem hiding this comment.
When another process can mutate the workspace between canonicalize and this open, the validated pathname is resolved again. On Unix, O_NOFOLLOW protects only the final component, so replacing an ancestor directory with a symlink can redirect the read outside the workspace; on Windows the flag is absent, so swapping the final component also works. The subsequent fstat only proves that the redirected target is a regular file, allowing its contents to be sent to the provider. Open through held directory descriptors with no-follow/beneath semantics instead of reopening the validated pathname.
Useful? React with 👍 / 👎.
| file.take(cap as u64 + 1) | ||
| .read_to_end(&mut data) | ||
| .map_err(|e| format!("unreadable ({})", e.kind()))?; |
There was a problem hiding this comment.
When the workspace is on a slow network mount, FUSE filesystem, or congested disk, submitting an image blocks the TUI event loop here while synchronously reading up to 3 MiB per image and 8 MiB per prompt; O_NONBLOCK does not make regular-file reads asynchronous, so the UI can stop redrawing or processing cancellation for an unbounded interval. Preserve the already-open descriptor but perform the read asynchronously or via spawn_blocking.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
`O_NOFOLLOW` guards only the final component, so an image staged as `sub/shot.png` could still be redirected by replacing `sub` with a symlink: the open walked the new ancestor and the fstat that followed merely confirmed the outside file was regular. Windows had no no-follow handling at all. Open each component relative to the descriptor of the one before it, starting from the workspace root, refusing symlinks the whole way down — a rearranged tree cannot steer a lookup that never restarts. Windows uses `FILE_FLAG_OPEN_REPARSE_POINT`, which turns a swapped-in symlink into a reparse point that fails the regular-file check rather than redirecting the read. The descriptor is then held until the turn starts, so the bytes are read from the file that was validated rather than from a name resolved a second time, and that read moves to the blocking pool: a workspace on a slow mount was stalling the event loop, and with it redraws and cancellation, for the length of the read.
|
@codex review — images now resolve through held directory descriptors: each component is opened relative to the previous one from the workspace root with O_NOFOLLOW, so a swapped ancestor cannot steer the lookup, and Windows uses FILE_FLAG_OPEN_REPARSE_POINT so a swapped final component fails the regular-file check instead of redirecting. The descriptor is held until the turn starts and the read runs on the blocking pool, so no path is resolved twice and the event loop no longer stalls on a slow mount. Tests cover the ancestor-swap attack end to end. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d8a357623
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // The workspace root is the trust anchor: it is the directory the | ||
| // session was started in, not something a mention chose. | ||
| let mut dir = std::fs::File::open(root).map_err(|e| format!("unreadable ({})", e.kind()))?; |
There was a problem hiding this comment.
When an attacker can rename the workspace directory through its writable parent between validation and staging, File::open(root) follows a replacement symlink. The subsequent openat calls are then safely relative to the wrong directory descriptor, so a mention such as @sub/shot.png can attach bytes from outside the validated workspace; open and retain the session root descriptor before this race, or at minimum open the root with directory and no-follow semantics.
Useful? React with 👍 / 👎.
| #[cfg(windows)] | ||
| { | ||
| use std::os::windows::fs::OpenOptionsExt; | ||
| options.custom_flags(FILE_FLAG_OPEN_REPARSE_POINT); |
There was a problem hiding this comment.
On Windows, when an attacker can replace a writable ancestor with a directory junction, FILE_FLAG_OPEN_REPARSE_POINT protects only the final component while OpenOptions::open(path) still traverses the ancestor junction. Creating a junction does not require the symlink privilege assumed above, so the final metadata check sees the redirected regular file and the attachment can escape the workspace; traverse components through held handles or explicitly reject reparse-point ancestors.
Useful? React with 👍 / 👎.
The descriptor walk was only as trustworthy as the directory it started from, and that one was opened by name like any other: replacing the workspace directory itself through a writable parent pointed every relative open below it at the wrong tree. Open the canonical root with `O_DIRECTORY | O_NOFOLLOW` so a swapped root is refused rather than followed; a session started in a symlinked directory is unaffected, because that symlink is resolved before the root is opened. On Windows the reparse-point flag only ever covered the final component, and a junction — unlike a symlink — needs no privilege to create, so an attacker could redirect an ancestor and have the read escape. Reject any ancestor between the root and the file that has become a reparse point.
|
@codex review — the descriptor walk now starts from a root opened with O_DIRECTORY | O_NOFOLLOW, so a workspace directory replaced through its parent is refused instead of followed (a session started in a symlinked directory still works, since the root is canonicalized first — covered by a test). On Windows every ancestor between the root and the file is rejected if it has become a reparse point, which closes the junction path that needs no privilege to create. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dac5d1c98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if std::fs::symlink_metadata(&ancestor) | ||
| .map_err(|e| format!("unreadable ({})", e.kind()))? | ||
| .file_type() | ||
| .is_symlink() |
There was a problem hiding this comment.
On Windows, when an ancestor is swapped for a directory junction after validation, this predicate does not reject it: FileType::is_symlink() identifies symbolic-link reparse tags, while a junction uses the mount-point reparse tag. The subsequent options.open(path) therefore follows the junction and can attach an image outside the workspace. Check the reparse-point attribute/tag itself rather than only is_symlink().
Useful? React with 👍 / 👎.
The ancestor check asked `FileType::is_symlink`, which keys off the name-surrogate bit in the reparse tag rather than on whether traversing the entry leaves the workspace. Test `FILE_ATTRIBUTE_REPARSE_POINT` instead: it refuses junctions, symlinks and any tag the filesystem grows later, which is the only form of the check that stays correct as tags are added.
|
@codex review — Windows ancestors are now rejected by testing FILE_ATTRIBUTE_REPARSE_POINT directly rather than FileType::is_symlink, so junctions, symlinks and any future reparse tag are all refused. Type-checked against the x86_64-pc-windows-gnu std since this host cannot run the Windows suite. One note for the record: std's FileType::is_symlink does already report junctions on Windows (IO_REPARSE_TAG_MOUNT_POINT has the name-surrogate bit set), so the previous form was not as broken as described — but the attribute test is the better check regardless, since it does not depend on how a tag is meant to be interpreted. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ecc010c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let meta = std::fs::symlink_metadata(&ancestor) | ||
| .map_err(|e| format!("unreadable ({})", e.kind()))?; | ||
| if is_reparse_point(&meta) { |
There was a problem hiding this comment.
On Windows, if another process can modify the workspace, it can replace an ancestor with a junction after this symlink_metadata check and before the later options.open(path). That open resolves every ancestor again, while FILE_FLAG_OPEN_REPARSE_POINT applies only to the final component, so the staged descriptor can point to an image outside the workspace and send its contents to the provider. Open each component relative to an already-open directory handle, or otherwise make validation and opening atomic.
Useful? React with 👍 / 👎.
| if !file | ||
| .metadata() | ||
| .map_err(|e| format!("unreadable ({})", e.kind()))? | ||
| .is_file() |
There was a problem hiding this comment.
When the final component is a non-name-surrogate reparse point, FILE_FLAG_OPEN_REPARSE_POINT only opens the reparse point itself; it does not reject it. Rust's Windows FileType::is_file() can still report true for a non-directory reparse tag that is_symlink() does not recognize, so this path does not actually refuse every reparse tag as intended. Apply is_reparse_point to the descriptor metadata before accepting it as a regular file.
Useful? React with 👍 / 👎.
`FILE_FLAG_OPEN_REPARSE_POINT` opens a reparse point rather than refusing it, and `is_file` only excludes the tags `is_symlink` recognizes — so a placeholder or any other non-name-surrogate tag was accepted as an ordinary file. Test the attribute on the descriptor's own metadata before accepting it. Also records what the Windows path cannot do: its ancestor checks and its open are separate lookups, because Win32 offers no handle-relative open.
|
@codex review — P2 fixed: the opened descriptor's own metadata is now tested with is_reparse_point before it is accepted, so a non-name-surrogate tag can no longer pass as a regular file. Type-checked against x86_64-pc-windows-gnu std. Note for the owner: this also refuses cloud placeholder files (OneDrive et al.) on Windows, with a clear "not a regular file" note — fail-closed, but worth knowing. Declining the P1 rewrite, with reasoning recorded in the code: Win32 has no handle-relative open, so making the Windows ancestor checks atomic means hand-written NtCreateFile FFI with a root directory handle — undocumented-adjacent unsafe code this crate does not otherwise carry, which I cannot test from this host and which would ship blind to Windows users. The residual gap is a local attacker who can already write inside the workspace and must also win a race, and who could instead just put the bytes they want into an image file there and have it attached with no race at all. The Unix path (openat per component from an O_NOFOLLOW root) is atomic and is where the realistic exposure was. Happy to file a follow-up issue for a windows-sys based traversal if you would rather have it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269b6c60f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (blocks, notes) = match tokio::task::spawn_blocking(move || { | ||
| super::mentions::encode_staged_images(images) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
When an image is on a slow mount, awaiting spawn_blocking here still suspends this sole TUI run loop before it reaches the event select!; the filesystem work uses the blocking pool, but redraws and Ctrl+C are not processed until the read and encoding finish. Poll the encoding task alongside input/cancellation instead of awaiting it inline so a slow descriptor cannot freeze the interface.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
| let attachments = std::mem::take(&mut self.pending_attachments); | ||
| let user_msg = if attachments.is_empty() { | ||
| user_message(user_input) | ||
| } else { | ||
| crate::llm::message::user_message_with_attachments(user_input, attachments) | ||
| }; |
There was a problem hiding this comment.
When a deployment uses UserPromptSubmit or PreTurn hooks to audit or scan outbound prompt content, the image blocks added to the user message here remain invisible because the subsequent hook contexts contain only user_input (or its text preview). The image bytes therefore reach the provider without the configured audit instrumentation seeing them; include attachment metadata/content, or a serialized representation of the actual user message, in the hook context.
Useful? React with 👍 / 👎.
Awaiting the encode inline parked the one loop the TUI has: the work was on the blocking pool, but redraws and Ctrl+C waited for it anyway, so a slow mount still froze the interface. Send the result back through a channel and pick it up in a select arm — the same shape the drill-in output reads already use — and re-arm the prompt with its blocks when it lands. A cancel during the read cannot stop it, so the result is dropped when it arrives instead of being sent with a turn nobody asked for. `UserPromptSubmit` is documented to see the full prompt so hooks can scan or audit what goes out, but attachments were invisible there: an image reached the provider without the configured instrumentation ever knowing. Both prompt hooks now receive a description of each attachment — type, media type, encoded size — which is what an audit needs, without putting megabytes of base64 through every hook invocation.
|
@codex review — merged main (#519 todos/conversation_epoch, #520 palette chrome, #528 plan-pointer serialization); the only conflict was the turn-start block, resolved by keeping the staged-encode/spawn split and taking main's conversation_epoch sink argument. Verified after the merge: mask_version intact and goldens still read "agent-code x.y.z" (no snapshot regeneration needed), no hardcoded Color:: or Also fixed your finding: abandoning a cancelled encode no longer clears attachments staged by a prompt interjected afterwards — those belong to a turn that is still coming, so they are kept and the phase stays Streaming while it waits. Plus a test pinning that a mentioned filename (attacker-controlled text) is escaped by render_item and cannot put a bidi override in the transcript. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5c70bda70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| crate::llm::message::user_message_with_attachments(user_input, attachments) | ||
| }; | ||
| self.state.push_message(user_msg); |
There was a problem hiding this comment.
When users mention images across multiple turns, this stores every base64 payload permanently in state.messages, so each subsequent provider request reserializes and resends all prior image bytes. A prompt may add 8 MiB raw (about 10.7 MiB encoded), while services/tokens.rs:32 charges each image only a fixed 2,000 tokens and normalization caps documents but not images, allowing request size and memory use to reach hundreds of MiB before compaction runs and causing ordinary screenshot-heavy sessions to exceed provider request limits. Add byte-aware image compaction or replace sufficiently old image payloads with bounded placeholders.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
An image is charged a flat vision estimate, so no token threshold ever notices it — yet the base64 payload stays in history for the rest of the session and is reserialized into every later request. A few screenshots were enough to push requests past what a provider accepts, with the memory to match, and none of the three compaction strategies could see the weight to act on it. Bound the bytes directly: keep the most recent 12 MiB of image payloads and replace older ones with text naming what was dropped, so the model still knows an image was there and the message keeps a valid shape. Runs once per turn, after the new turn's own images are counted as the most recent.
|
@codex review — image payloads in history are now bounded: evict_old_images keeps the most recent 12 MiB and replaces older ones with text naming what was dropped, so the message stays valid and the model still knows an image was there. It runs once per turn after the new images are counted as most recent, and lives next to microcompact since it is the same kind of mechanism. I deliberately did not make the token estimate size-derived instead: the flat 2,000 is roughly right for actual vision token cost, and charging 3 MiB of base64 at bytes/4 would claim ~780k tokens for one screenshot and send the session into permanent compaction. The problem you identified is request bytes, so the fix bounds bytes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd9a283c3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (msg_idx, block_idx, len, media_type) in sites.into_iter().rev() { | ||
| if kept + len <= budget_bytes { | ||
| kept += len; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When a long session contains many small images, this byte-only condition never evicts them even though providers also limit the number of images in one request. For example, 26 turns attaching four tiny thumbnails retain 104 image blocks while consuming far less than 12 MiB; Anthropic accepts at most 100 images per request, so the next model call is rejected. Add a retained-image count limit and evict oldest blocks when either the byte or count budget is exceeded.
Useful? React with 👍 / 👎.
Bytes are not the only limit a provider enforces: Anthropic accepts at most 100 images per request, and a session attaching small thumbnails each turn reaches that count while using a fraction of the byte budget. Evict on whichever bound is hit first, keeping the 32 most recent.
|
@codex review — image retention is now bounded by count as well as bytes: eviction fires on whichever limit is hit first, keeping the 32 most recent images (well under the 100-image provider cap) and the most recent 12 MiB. Test covers a session of small thumbnails that would never have tripped the byte budget. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597d0578d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (msg_idx, block_idx, len, media_type) in sites.into_iter().rev() { | ||
| if kept + len <= budget_bytes && kept_count < budget_count { | ||
| kept += len; | ||
| kept_count += 1; | ||
| continue; |
There was a problem hiding this comment.
When differently sized images exceed the byte budget, this reverse greedy pass can evict a newer image and then retain an older, smaller one that fills the remaining space. For example, with oldest-to-newest payloads of 4 MiB, 8 MiB, and 8 MiB under a 12 MiB budget, it keeps the newest and oldest images while dropping the middle one. That violates the documented newest-first/oldest-first retention policy and can discard a more relevant image while preserving stale context; once an image cannot fit because of retained newer images, all still-older images should also be evicted.
Useful? React with 👍 / 👎.
Three conflicts, all additive: the App fields (vi mode alongside the staged images), the engine fields (persistent grants alongside the pending attachments), and the keybindings doc (both new sections kept, each closed explicitly rather than unioned). Also fixes the image retention order: what survives is now a contiguous run of the newest images, since the first one that does not fit ends retention outright. The greedy pass it replaces could skip an image and then keep an older, smaller one behind it — holding stale context while dropping something newer and more likely to be under discussion.
|
@codex review — merged main (#507 persistent grants, #522 vi mode, #526 inline subagent output). Three conflicts, all additive and closed explicitly rather than unioned: App fields (vi mode beside the staged images), engine fields (persistent grants beside the pending attachments), and the keybindings doc (both sections kept). Checked the dispatch concern: this branch adds no key or paste handling — the image path hooks into submit, downstream of dispatch — and main's vi suite passes, 743 CLI tests green. mask_version intact, goldens still read "agent-code x.y.z" (no regeneration), no hardcoded Color:: or Your finding is fixed too: retention is now a contiguous run of the newest images — the first one that does not fit ends retention outright, so an older smaller image can no longer be kept behind a dropped newer one. Test uses your 4/8/8 MiB case. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ac3d828e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| app.pending_attachments = blocks; | ||
| app.pending_submit = Some(prompt); |
There was a problem hiding this comment.
When attachment encoding is slow, built-in slash commands still execute before the Phase::Streaming queue check; commands such as /diff return CommandResult::Prompt, and enqueue_turn_from_command stores that newer prompt in app.pending_submit. When the earlier image result arrives, these unconditional assignments overwrite it with the original image-bearing prompt, so the slash-command turn shown in the transcript is silently lost. Associate results with a prompt generation or avoid restoring them over a newer pending submission.
Useful? React with 👍 / 👎.
A slash command that produces a prompt is staged directly rather than queued, so it can land while an earlier prompt's images are still being read. Restoring that earlier prompt then overwrote it and its turn was lost without a trace — and simply keeping the newer prompt would have been worse, since the encoded blocks would have ridden along on it. The newer prompt keeps its turn and its own attachments; the superseded one returns to the head of the queue with a note that its images did not come with it. Neither prompt is lost and no image can land on a turn it was not meant for.
|
@codex review — fixed: a prompt staged while images are encoding is no longer overwritten. Worth noting the second half of that hazard, which the fix also closes: simply keeping the newer prompt would have attached the earlier prompt's encoded blocks to it, putting one prompt's image on another's turn. So the newer prompt keeps its turn and its own attachments, and the superseded prompt returns to the head of the queue with a note that its images did not come along — neither prompt is lost and no image can land on a turn it was not meant for. Two tests cover both branches. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c06686498
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| blocks: Vec<agent_code_lib::llm::message::ContentBlock>, | ||
| ) { | ||
| if self.pending_submit.is_some() { | ||
| self.queue.push_front(prompt); |
There was a problem hiding this comment.
When a command-produced prompt arrives while encoding, this queues the already-expanded engine prompt, which still begins with the original @shot.png. After the newer turn completes, dispatch_queue_head() passes that string through enqueue_turn(), which calls expand_mentions() again and stages the image anew. The queued turn therefore auto-sends an image despite the warning that its images were not attached—and if the file changed meanwhile, it sends newly resolved bytes that were never part of the staged turn. Queue a representation that cannot re-expand attachments, or preserve each prompt and its attachments as one unit.
Useful? React with 👍 / 👎.
Queuing the superseded prompt put the *expanded* text back in the queue, which still opens with the original mention. Dispatching it ran `expand_mentions` again: the turn silently re-attached an image the user had been told was not attached, resolving the path a second time and sending whatever the file held by then — the delayed second lookup this branch exists to avoid. Hold the prompt and its already-encoded blocks together instead, and send them as one unit when the turn frees up. Nothing re-resolves, nothing re-reads, and the file that goes out is the one that was validated. A cancel drops it, so nothing follows on its own after the user stops.
|
@codex review — correct catch, and the consequence was worse than a stale warning: the queued text was the expanded prompt, so dispatching it re-ran expand_mentions and re-resolved the mention, sending whatever the file held by then — exactly the delayed second lookup this branch removed. The prompt and its already-encoded blocks are now held together and sent as one unit when the turn frees up; nothing re-resolves or re-reads, and a cancel drops it so nothing follows on its own. Four tests: deferral, restoration with the original blocks, waiting while another prompt's descriptors are staged, and the cancel. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ac0e96c18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A read already handed to the blocking pool cannot be stopped, | ||
| // so its result is thrown away when it lands. | ||
| if encoding { | ||
| discard_encoding = true; | ||
| } |
There was a problem hiding this comment.
When an encoded image prompt is deferred behind another prompt and the user cancels that active replacement turn, encoding is already false, so this cancellation path neither invokes abandon_staged_attachments nor clears deferred_prompt. After the abort, the next terminal event reaches rearm_deferred_prompt and sends the image-bearing prompt automatically, contrary to the cancel contract and potentially transmitting a file after the user stopped the flow.
Useful? React with 👍 / 👎.
| // A prompt that was held aside for a turn that has since gone can | ||
| // send now, with the blocks it was already encoded with. | ||
| if turn.is_none() && !encoding { | ||
| app.rearm_deferred_prompt(); |
There was a problem hiding this comment.
When deferred_prompt is waiting behind a live turn, this check runs while turn is still Some; the handle is only reaped later in the same iteration. If that turn completes with no regular queue item, the loop then parks in select!, so the promised next prompt remains unsent until an unrelated event occurs; if the queue is non-empty, dispatch_queue_head arms a later prompt first and the deferred prompt loses FIFO priority. Rearm immediately after the handle is taken, before dispatching the ordinary queue.
Useful? React with 👍 / 👎.
| // A read already handed to the blocking pool cannot be stopped, | ||
| // so its result is thrown away when it lands. | ||
| if encoding { | ||
| discard_encoding = true; | ||
| } |
There was a problem hiding this comment.
When the attachment read is slow or hung, Ctrl+C only sets discard_encoding; encoding remains true and the app remains in Streaming until img_rx eventually receives the uninterruptible blocking task's result. Consequently later normal prompts keep queueing behind a turn that never started, which defeats the stated reason for detaching slow-mount reads. Reset the staged-turn state when cancellation is observed and identify or otherwise ignore the late result independently so a new encoding can proceed.
Useful? React with 👍 / 👎.
Three faults in the same interaction: A prompt held aside behind a live turn survived that turn's cancellation and then sent itself, file and all, after the user had stopped. Cancel now drops it — unless a prompt is already staged, which is interject saying it cancelled in order to send something else, and the held prompt keeps its place behind that. Cancelling during a read only marked the result for discard: the phase stayed Streaming until a read that may never finish came back, queueing every later prompt behind a turn that never started. The encode now carries an id; a cancel forgets it, releases the staged turn immediately, and the late result is recognised as stale when it lands. A held prompt was armed at the top of the loop, so after its turn was reaped it waited for an unrelated event, and the ordinary queue could overtake it. It is armed as soon as the handle is taken, ahead of the queue it was submitted before.
|
@codex review — all three fixed. Cancel now drops a prompt held behind the cancelled turn, with one deliberate exception: interject cancels precisely in order to send something else, and says so by having staged its own prompt, so the held prompt keeps its place behind the interjection rather than being lost silently (both branches tested). Cancelling during a read no longer waits on it: the encode carries an id, a cancel forgets the id and releases the staged turn immediately, and the late result is recognised as stale when it lands — so a new encode can start meanwhile. And the held prompt is armed as soon as the turn handle is reaped, before dispatch_queue_head, so it neither waits for an unrelated event nor loses its place to the queue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f486a4251e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// as a text-only turn. | ||
| pub fn abandon_staged_attachments(&mut self) { | ||
| // A cancel means nothing more should go out on its own. | ||
| self.deferred_prompt = None; |
There was a problem hiding this comment.
When an interjection arrives while an image encode is active and deferred_prompt already holds an earlier prompt, the run loop first preserves that prompt in cancel_pending_followups(), but then calls this method because active_encode is set. This unconditional assignment deletes the held prompt, so the documented interjection exception fails specifically during another encode and the earlier prompt is silently lost; only clear it for a bare cancellation, not when a replacement prompt is staged.
Useful? React with 👍 / 👎.
| self.transcript.push(TranscriptItem::System( | ||
| "another prompt was sent first — sending this one with its images next".into(), | ||
| )); | ||
| self.deferred_prompt = Some((prompt, blocks)); |
There was a problem hiding this comment.
If one prompt is already in deferred_prompt and a second encode finishes while a third command-produced prompt occupies pending_submit, this assignment replaces the first deferred prompt with the second. This can occur with consecutive image-bearing command prompts such as /redo or /editor while reads are slow, and it silently drops the oldest prompt despite telling the user it will be sent next; deferred prompts need FIFO storage or an explicit no-overwrite path.
Useful? React with 👍 / 👎.
| // The prompt is re-armed with them so the turn starts on the | ||
| // next pass through the loop above. | ||
| Some((id, prompt, blocks, notes)) = img_rx.recv() => { | ||
| if active_encode != Some(id) { |
There was a problem hiding this comment.
While attachment encoding is active, the engine lock is free, so commands such as /resume, /rewind, or /clear can replace the conversation and increment conversation_epoch. This receive branch validates only the encode id, so the old prompt is subsequently accepted and started inside the newly restored or cleared conversation; stamp the encode with its conversation epoch and discard it when that epoch is stale, as engine events already do.
Useful? React with 👍 / 👎.
Dropping the read in flight also cleared the held prompts, so an interject arriving mid-encode lost the very prompt the interject rule exists to protect. Whether held prompts survive is now the cancel policy's decision alone; abandoning a read says only that this read is being dropped. A second prompt could be held before the first had gone, and the single slot silently replaced it — after telling the user it would be sent next. They are held in a deque and sent in the order they were submitted. An encode now carries the conversation it was submitted in. The engine lock is free while it runs, so `/clear`, `/resume` and `/rewind` can replace the conversation underneath it; the result is dropped when that happens, and replacing a conversation takes its staged attachments with it — a file staged for the old thread must not surface in the new one.
|
@codex review — all three fixed, rebased onto the owner's merge of #529. Dropping a read in flight no longer touches held prompts: whether they survive is the cancel policy's decision alone, so an interject landing mid-encode keeps the prompt the interject rule exists to protect. Held prompts moved to a deque and are sent in submission order, so a second cannot replace the first. And each encode now carries the conversation it was submitted in — the result is discarded if /clear, /resume or /rewind replaced it meanwhile, and replacing a conversation drops its staged attachments outright, so a file staged for the old thread cannot surface in the new one. Unrelated note for the owner: review::tests::a_real_base_resolves_to_a_sha and a_base_review_diffs_the_merge_base_against_head (new in #529) assert the merge base equals rev-parse HEAD, which only holds when the local branch has no unpushed commit — they fail locally with work in progress and pass again once pushed. Worth pinning to a fixture repo rather than the ambient checkout. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32a63063f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if epoch != app.conversation_epoch { | ||
| // The conversation it was submitted in has been cleared, | ||
| // resumed or rewound. Starting it now would attach the | ||
| // file to a thread the user never attached it to. | ||
| active_encode = None; | ||
| app.abandon_staged_attachments(); |
There was a problem hiding this comment.
The epoch mismatch is handled only after the blocking read returns. If /clear, /resume, or /rewind replaces the conversation while an image on a slow or stalled mount is being encoded, active_encode remains set and the guards above prevent every prompt in the new conversation from starting until that obsolete read completes; newly typed prompts are also queued because the UI remains in Streaming. Invalidate the active encode as soon as the conversation epoch changes rather than waiting for its result.
Useful? React with 👍 / 👎.
| pub fn cancel_pending_followups(&mut self) { | ||
| if self.pending_submit.is_none() { | ||
| self.deferred_prompts.clear(); | ||
| } |
There was a problem hiding this comment.
A nonempty pending_submit does not necessarily mean the cancellation came from an interject. For example, while an image is encoding, a slash command such as /diff can return CommandResult::Prompt and stage its prompt directly; if the user then presses bare Ctrl+C, this condition preserves that prompt and it starts automatically after the encode is abandoned, contrary to the documented bare-cancel policy that nothing should follow. Track whether cancellation was requested by an interject explicitly instead of inferring it from pending_submit.
Useful? React with 👍 / 👎.
…once A staged prompt was taken as proof that the cancel came from interject, but a slash command can stage one at any moment — including while the user is pressing Ctrl+C to stop everything, which then let a held image prompt send itself anyway. Interject and queue send-now now say outright that they cancelled in order to send; every other cancel is a stop. A replaced conversation also waited for the read in flight to come back before releasing anything, so on a slow mount every prompt in the new conversation queued behind an encode belonging to a conversation nobody was looking at any more. The loop notices the epoch change itself and drops the encode there and then.
|
@codex review — both fixed. Cancels now say what they are: interject and queue send-now set a flag meaning "cancelled in order to send", so held prompts keep their place behind them, and every other cancel is a stop that drops them — a staged prompt is no longer taken as proof, since a slash command can put one there mid-Ctrl+C (tested exactly that case). And the loop watches the conversation epoch itself, dropping an encode in flight the moment /clear, /resume or /rewind lands rather than waiting for a read that may never return. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Closes D2-07. There was no multimodal input path from the TUI at all — and the symptom users actually hit was that
@shot.pngreported:That is never what mentioning an image means. It was also the only thing the TUI could say: an image cannot be inlined as text, and no path existed for one to reach the model.
Approach: reuse the syntax that already exists
Mentioned images are attached to the turn as
ContentBlock::Image.@screenshot.png— the same syntax people already use for files. Nothing new to discover, and it replaces a dead end that was actively misleading.image_block_from_filealready existed in the lib; what was missing was a way for a user turn to carry it.Engine plumbing
user_message_with_attachments(text, blocks)— attachments go before the text, so the prompt reads as being about what was just shown rather than as an afterthought.QueryEngine::set_pending_attachments(blocks)holds blocks for the next turn.Two details that are the actual risk in a stateful attachment:
Taken, not read.
std::mem::takeat message construction — an attachment belongs to exactly one turn, and leaking it forward would silently re-send an image the user already shared.The setter replaces rather than appends, so a composer that is edited or abandoned cannot accumulate images across attempts.
This avoids threading a parameter through
spawn_turn→run_turn_spawned→run_turn_innerand its 11 call sites, for a single consumption point.Decoding timing
Images are read and base64-encoded when the turn starts, not when the mention is parsed. A large screenshot stays off the heap until it is needed, and a read failure becomes a transcript note instead of blocking the prompt from being submitted at all.
Verification
an_image_mention_is_attached_not_skipped— writes a real PNG header (so the binary sniff would have caught it), asserts it is attached, that a note explains why, and that it is not inlined as<file …>.image_extensions_are_matched_case_insensitively—.PNG,.Jpeg,.webp.a_non_image_binary_is_still_skipped— a non-image binary keeps its "binary, skipped" note rather than being attached as something unreadable.attachments_precede_the_text_in_a_user_messageandan_attachment_with_no_text_carries_only_the_attachment(no empty text block appended).647 bin tests pass.
cargo test --workspace --all-targetsgreen apart from the 3bwrap_*tests, which fail on this host withsetting up uid map: Permission deniedand pass in CI.clippy --all-targets -- -D warningsandfmt --checkclean.Scope
Only formats the API accepts (
png,jpg/jpeg,gif,webp) are attached; anything else keeps its skip reason rather than being sent as something the model cannot read.Not included: pasting image bytes from the system clipboard, and composer chips showing pending attachments. Both are additive on top of this; the path from "user has an image" to "model sees it" is what was missing.