fix(providers): detect image paths with spaces - #9387
Conversation
![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: 4da4bb9aba
ℹ️ 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 Some(ext_idx) = suffix.to_lowercase().find(ext) else { | ||
| break; | ||
| }; | ||
| let abs_ext_idx = start_idx + ext_idx; | ||
| let end_idx = abs_ext_idx + ext.len(); |
There was a problem hiding this comment.
detect_image_path derives ext_idx from suffix.to_lowercase().find(ext) and then reuses that byte index to slice the original text. This is incorrect for Unicode where lowercasing can change byte length (for example İ becomes i + combining dot), so abs_ext_idx/end_idx can point to the wrong byte range and a valid absolute image path is missed. In prompts containing such characters before the path, local image detection silently fails and the image is not attached.
Useful? React with 👍 / 👎.
The previous version called `to_lowercase()` on the search window and used the resulting byte index against the original `text`. That is unsound because Unicode lowercasing can change byte length (e.g. Turkish capital `İ` lowercases to `i` + combining dot above, two bytes longer), so the computed index can land mid-codepoint or off the real extension entirely. Any prompt with such a character before the path would silently miss detection. Add a small ASCII case-insensitive search over the original byte buffer. Extensions (`.png`/`.jpg`/`.jpeg`) are pure ASCII, so this is both correct and faster than allocating a lowercased copy on every iteration. Tests cover the Turkish-İ prefix case and an uppercase `.PNG` extension. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed Codex's P2 in 1f6ff98 — replaced |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f6ff98e2a
ℹ️ 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 ext in &extensions { | ||
| let mut start_idx = 0; | ||
| while let Some(abs_ext_idx) = find_ascii_ci(text, ext, start_idx) { |
There was a problem hiding this comment.
Iterating extensions in fixed order (.png, then .jpg, then .jpeg) changes selection semantics from “first path in the prompt” to “first path for the first matching extension type.” In prompts containing multiple absolute image paths of different types, this can attach the wrong image (for example, an earlier .jpg and a later .png will always choose the later .png), which changes model input and can produce incorrect responses. The scan should prioritize earliest match position in text, not extension precedence.
Useful? React with 👍 / 👎.
The previous version iterated extensions in fixed order (.png, then .jpg, then .jpeg), so a prompt mentioning an earlier .jpg followed by a later .png would attach the .png. Restore "first path wins" semantics by scanning all extensions in lockstep and picking the match with the earliest byte position. Adds a test with a .jpg followed by a .png in the same prompt and asserts the .jpg is selected. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed Codex's P2 in 6ebb907 — the loop now picks the earliest extension match across .png/.jpg/.jpeg at each step instead of exhausting one extension before trying the next, restoring "first path in the prompt wins" semantics. Added a test with an earlier .jpg followed by a later .png that asserts the .jpg is returned. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ebb907bf3
ℹ️ 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 (abs_ext_idx, ext_len) = earliest?; | ||
| let end_idx = abs_ext_idx + ext_len; | ||
|
|
There was a problem hiding this comment.
detect_image_path treats any .png/.jpg/.jpeg substring as a candidate end and then searches backward for an existing file, but it never checks that the extension is actually the end of the referenced path. If a prompt contains something like /tmp/photo.png.backup and /tmp/photo.png exists, this will incorrectly attach /tmp/photo.png even though the user did not reference that image. This is a regression from the old ends_with behavior and can cause unintended image data to be sent to providers.
Useful? React with 👍 / 👎.
The backward scan happily accepted `.png` even when it appeared in the middle of a longer name like `photo.png.backup`. If a sibling `photo.png` existed on disk, that wrong file would be attached. Require the extension to terminate the filename: the byte right after must not be alphanumeric, `_`, or `-`. A literal `.` is allowed only when followed by a non-alphanumeric (so end-of-sentence punctuation like `see /tmp/foo.png.` still resolves correctly, while compound extensions like `.png.backup` are rejected). Tests cover both the rejection and the trailing-punctuation case. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed Codex's P1 in 27a1118 — the extension match now has to terminate the filename. The byte right after |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a1118028
ℹ️ 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 i in (0..abs_ext_idx).rev() { | ||
| let Some(candidate) = text.get(i..end_idx) else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
The backward scan currently tries every start byte before the matched extension and accepts the first absolute file that exists, without requiring a delimiter before the candidate. In text like https://example.com/home/alice/photo.png, this can extract /home/alice/photo.png from the URL path and attach a local file if it exists, even though the user only referenced a remote URL. This is a data-leak regression from the old token-based behavior and should be constrained to true standalone path boundaries (e.g., whitespace/quotes/brackets before the path start).
Useful? React with 👍 / 👎.
| Some(b) if b.is_ascii_alphanumeric() => true, | ||
| Some(b'_' | b'-') => true, | ||
| Some(b'.') => bytes | ||
| .get(end_idx + 1) | ||
| .is_some_and(|c| c.is_ascii_alphanumeric()), | ||
| _ => false, |
There was a problem hiding this comment.
The extension-termination check only treats ASCII alnum/_/- (and some . cases) as continuation characters. If a filename continues with a non-ASCII letter (for example /tmp/foto.pngé), continues_filename is false, so the code can incorrectly truncate to /tmp/foto.png and attach that sibling image if it exists. This is the same class of false-positive the .backup guard addresses, but still reproducible for Unicode filename suffixes.
Useful? React with 👍 / 👎.
…e suffixes Two more soundness issues in the backward-scan extraction: 1. A URL like `https://example.com/home/alice/photo.png` could leak a local file if `/home/alice/photo.png` happened to exist, because the scan accepted any absolute substring without checking what sat to the left. Add `extends_path_left`: the byte immediately before the candidate must not be alphanumeric or any of `/ \ . - _ ~ :`, and any non-ASCII byte is conservatively treated as filename continuation. The candidate at position 0 is always at a boundary. 2. The right-side termination check only treated ASCII alphanumerics (and a few punctuation bytes) as filename continuation, so `/tmp/foto.pngé` could be truncated to `/tmp/foto.png` and attach a sibling that exists on disk. Extend the rule so any non-ASCII byte after the extension — or after a trailing `.` — counts as filename continuation. Tests cover both: a URL containing a real local path, and a Unicode suffix on top of an existing sibling file. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed both Codex comments in 005be54:
Two tests added covering both cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 005be543fd
ℹ️ 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".
| Some(b) if b.is_ascii_alphanumeric() => true, | ||
| Some(b'_' | b'-') => true, | ||
| Some(b'.') => bytes | ||
| .get(end_idx + 1) | ||
| .is_some_and(|c| !c.is_ascii() || c.is_ascii_alphanumeric()), | ||
| _ => false, |
There was a problem hiding this comment.
The extension-termination guard in detect_image_path does not treat / (or \\) as continuation characters, so text like /tmp/photo.png/thumbnail is considered a match for /tmp/photo.png. If that file exists locally, the code will attach it even though the prompt referenced a longer path/route segment, which is a regression from the previous ends_with behavior and can leak unintended local image data. Consider marking slash separators as continuation so only true terminal extensions are accepted.
Useful? React with 👍 / 👎.
`/tmp/photo.png/thumbnail` was being accepted because the extension guard only flagged alphanumerics and `_`/`-`. With a `/` (or `\`) right after `.png`, the .png is a directory component in a deeper path, not a file reference, and we shouldn't truncate to `…/foo.png` even if that file happens to exist locally. Adds `/` and `\` to the continues_filename set, with a test. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed Codex's P1 in bc64948 — added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc64948f69
ℹ️ 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 bytes = text.as_bytes(); | ||
| let continues_filename = match bytes.get(end_idx) { | ||
| None => false, | ||
| Some(b) if !b.is_ascii() => true, |
There was a problem hiding this comment.
The extension terminator check currently treats every non-ASCII byte after .png/.jpg/.jpeg as a filename continuation, so prompts like /tmp/photo.png please describe (ideographic space) or those using non-breaking space are skipped even when the file exists. This is a regression from the previous split_whitespace behavior, which accepted Unicode whitespace separators, and it causes valid local images in multilingual prompts to be silently ignored.
Useful? React with 👍 / 👎.
| if !b.is_ascii() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The left-boundary guard marks any non-ASCII preceding byte as "gluing," which rejects valid absolute paths when they are preceded by Unicode whitespace (for example an ideographic space before /tmp/photo.png). Because text.get(i..end_idx) iterates byte indices, this also evaluates continuation bytes of multibyte separators as non-ASCII glue, preventing detection of otherwise valid image paths in internationalized text.
Useful? React with 👍 / 👎.
Both the post-extension and pre-candidate guards were doing byte-level
classification, which conflates Unicode whitespace (NBSP `\u{00A0}`,
ideographic space `\u{3000}`, etc.) with filename material because
their UTF-8 lead/continuation bytes have the high bit set.
Switch both checks to char-level. The new rule for filename
continuation is:
- alphanumeric (any script) → continuation
- `_ - / \` → continuation
- `.` followed by alphanumeric → continuation
- anything else (whitespace, punctuation, emoji, …) → terminator
`extends_path_left` likewise looks at the preceding *char* via
`char_indices().next_back()`. Unicode whitespace now correctly opens
a left boundary, matching the original `split_whitespace` semantics.
Tests cover ideographic-space prefix and NBSP suffix.
Reported by Codex review on PR aaif-goose#9387.
Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed both Codex P2s in 61e0255 — switched both the post-extension and pre-candidate guards from byte-level to char-level classification. The new continuation rule: alphanumeric (any script) / Tests added for ideographic-space prefix and NBSP suffix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e0255c4e
ℹ️ 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".
| .get(end_idx + 1..) | ||
| .and_then(|s| s.chars().next()) | ||
| .is_some_and(|c| c.is_alphanumeric()), | ||
| Some(c) => c.is_alphanumeric(), |
There was a problem hiding this comment.
The extension boundary check only treats alphanumerics (plus _, -, /, \, and . followed by alnum) as continuation, so valid filename characters like @, +, ~, ) and others are treated as terminators. In text such as /tmp/photo.png@2x, this code can truncate to /tmp/photo.png and attach that file if it exists, even though the user referenced a different path token; this is a regression from the previous ends_with behavior and can leak unintended local image data.
Useful? React with 👍 / 👎.
Addresses three soundness issues raised in the latest code review:
P1.1 — Boundary rule was a deny-list, so zero-width spaces, combining
marks, bidi/format controls, emoji, and arbitrary symbols all
silently acted as separators. That let payloads like
`/Users/me/private.png\u{200B}.backup` or
`https://example.com\u{200B}/Users/me/private.png` extract a sibling
local file. Replace the rule with `is_path_separator`, an explicit
allow-list of whitespace plus a small set of punctuation. Everything
else is conservatively treated as filename continuation, so an
attacker cannot use a non-printing character to break out of a
compound name or URL.
P1.2 — Worst case was O(n^2) on multi-megabyte prompts: the outer
loop re-scanned the remaining text for each absent extension, the
inner backward walk could traverse the whole prompt per match, and
`clean_path` allocated a fresh String on every iteration. Replace
the outer with a single forward byte scan (`match_image_extension`),
bound the backward walk to `MAX_PATH_LEN` (4096, > Linux PATH_MAX),
and switch `clean_path` to return `Cow<str>` so the common path
borrows. Added a regression test that runs detect_image_path against
a 1 MB prompt and asserts a sub-second time.
P1.3 — TOCTOU in `load_image_file`: validated via one `File::open`
in `is_image_file`, then read via a second `std::fs::read`. A
swapped symlink between the two opens could substitute an unrelated
file. Read once, validate the magic bytes from the buffer we are
about to base64-encode. Factored out `is_image_bytes(&[u8])` so the
same check works on a path-opened file and on the loaded buffer.
Also adds tests for: zero-width-space / combining mark / bidi
attacks, path at byte 0, file:// URI false negative,
backslash-escaped and quoted paths through `load_image_file`, and a
non-image .png-named file (must reject after the buffer-validation
fix).
Reported by Codex review on PR aaif-goose#9387.
Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed all three P1s in 1f4110f:
Tests for P2 partials added: path at byte 0, file:// URI false negative, backslash-escaped and quoted paths through |
Codex flagged that `/tmp/photo.png@2x` could truncate to `/tmp/photo.png` under the previous deny-list boundary rule. Commit 1f4110f already replaced that rule with the conservative `is_path_separator` allow-list, which treats `@`, `~`, `+`, and similar characters as continuation by default. Add explicit tests so the regression can't sneak back in. Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
FYI — this finding was reviewed against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 903602efeb
ℹ️ 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".
| // this into O(n^2). We don't expect a real absolute path to be | ||
| // longer than this. | ||
| let scan_floor = abs_ext_idx.saturating_sub(MAX_PATH_LEN); | ||
| for j in (scan_floor..abs_ext_idx).rev() { |
There was a problem hiding this comment.
Iterating j in reverse means the detector tests the shortest absolute suffix first and returns immediately on the first existing file. That can attach the wrong local image when the intended path contains another absolute path as a tail (for example, prompt references /workspace/goose/tmp/photo.png but /tmp/photo.png also exists, so the code returns the latter). This is a correctness and data-exposure regression from the prior whole-token behavior; the scan should prefer the leftmost/full candidate for a matched extension, not the first existing suffix.
Useful? React with 👍 / 👎.
Previously the inner walk went from `abs_ext_idx - 1` down to `scan_floor`, which means it tested the *shortest* absolute suffix first. The `extends_path_left` guard already rejected sub-suffixes of a longer path (the previous char wasn't a separator), so in practice the result was the same — but the iteration order made it non-obvious and Codex flagged it as a latent regression. Switch to forward iteration. The first valid candidate is now also the longest, so the intent "prefer the leftmost / full path" is encoded by the loop itself, not solely by the boundary check. Same PATH_MAX bound, same boundary rule. Add a test that builds a nested path and asserts the full path is returned (not any tail substring that ends in `.png`). Reported by Codex review on PR aaif-goose#9387. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
|
Addressed in dfc3240. The previous code did iterate backwards (shortest-suffix first), but the Switched to forward iteration. The first valid candidate is now also the longest, so 'longest valid path wins' is encoded in the loop itself rather than relying on the boundary check to reject sub-suffixes. Added a test that builds a nested wrapper path and asserts the full path round-trips through detect_image_path. |
|
External re-review came back clean — declared ship-safe. Three follow-up notes; one was actionable and just fixed, two tracked separately:
|
detect_image_path split on whitespace, so an absolute path containing spaces (a macOS screenshot like /…/Screen Shot 2026.png) was never matched. Anchor on each image-extension occurrence instead and walk back over '/'-rooted starts, returning the longest candidate that is an existing image file; the backward scan is bounded to avoid quadratic work on extension-heavy text. Extension matching is now case-insensitive. Existing behavior (relative/fake/nonexistent) is preserved; adds spaces + flood regression tests. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
ff26ac5 to
91ddcb4
Compare
|
Rebased onto current main. The original change targeted |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91ddcb43d1
ℹ️ 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 Some(end) = EXTENSIONS | ||
| .iter() | ||
| .any(|ext| word.to_lowercase().ends_with(ext)) | ||
| { | ||
| let path = Path::new(word); | ||
| // Check if it's an absolute path and file exists | ||
| if path.is_absolute() && path.is_file() { | ||
| // Verify it's actually an image file | ||
| if is_image_file(path) { | ||
| return Some(word); | ||
| .filter_map(|ext| find_ascii_ci(text, ext, from).map(|i| i + ext.len())) | ||
| .min() |
There was a problem hiding this comment.
When the prompt contains many occurrences of only one supported extension (for example pasted logs with thousands of .png names and no .jpg/.jpeg), this recomputes find_ascii_ci for the absent extensions across the entire remaining suffix on every loop iteration before taking min(). Even though the backward path scan is bounded, this forward search still makes detect_image_path roughly quadratic in extension-heavy text, whereas the previous whitespace scan was linear and provider formatting can stall before sending the request.
Useful? React with 👍 / 👎.
…xtensions The simplified backward-scan reintroduced two boundary regressions: a path that is a suffix of a URL (https://host/x.png) could be extracted via the '://' separator, and a backup file (/tmp/x.png.backup) could be truncated to the bare image path. Require the extension to terminate the candidate and the leading '/' to follow a whitespace/quote boundary, and add regression tests. Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
|
Thanks for this — the core fix (detecting absolute paths with spaces) is a real improvement. I pushed a follow-up commit to your branch (
One thing to tidy up on your side: the PR description is now stale — it still describes a
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7912b812c
ℹ️ 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 terminated = text | ||
| .get(end..) | ||
| .and_then(|rest| rest.chars().next()) | ||
| .is_none_or(|c| c == '/' || c.is_whitespace()); |
There was a problem hiding this comment.
When a user includes a local path in quotes, e.g. "/tmp/Screen Shot.png", the leading quote is allowed by the start-boundary check below, but the closing quote makes terminated false because only end-of-text, /, or whitespace are accepted after the extension. This causes common quoted paths with spaces to be silently ignored even though the new scanner is meant to handle space-containing paths; include matching quote characters as valid terminators before probing the candidate.
Useful? React with 👍 / 👎.
A quoted path like "/tmp/Screen Shot.png" was silently ignored because the closing quote was not a valid terminator after the extension, defeating the whole point of handling space-containing paths. Treat quote characters as terminators alongside whitespace and '/', and add regression tests for quoted paths with spaces. Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8aea934b2
ℹ️ 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 Some(candidate) = text.get(start..end) else { | ||
| continue; | ||
| }; | ||
| let path = Path::new(candidate); | ||
| if path.is_absolute() && path.is_file() && is_image_file(path) { |
There was a problem hiding this comment.
When the prompt text itself contains a shell-style escaped space, such as a file dragged or pasted into the interactive terminal as /tmp/Screen\ Shot.png, this new scanner builds the candidate with the backslash still present and probes that literal path. On Unix that is a different filename from /tmp/Screen Shot.png, so the path-with-spaces case still silently fails outside of -t/shell-processed arguments; unescape \ before Path::new/loading or probe the cleaned candidate.
Useful? React with 👍 / 👎.
Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bda01f70c
ℹ️ 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 terminated = | ||
| terminator.is_none_or(|c| c == '/' || c.is_whitespace() || c == '"' || c == '\''); |
There was a problem hiding this comment.
When a filename contains an earlier image extension followed by a space, this treats that space as the end of the path and immediately returns the prefix if it exists. For example, if both /tmp/Screen Shot.png and /tmp/Screen Shot.png edited.jpg exist and the user asks about the latter, the scan stops at the .png prefix and attaches the wrong image before ever considering the later .jpg; the detector should keep looking for a longer existing candidate before accepting a whitespace-terminated extension.
Useful? React with 👍 / 👎.
Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
…dary A whitespace-terminated extension is ambiguous because the filename may continue to a later extension (e.g. "Screen Shot.png edited.jpg"). Instead of returning the first existing prefix, keep scanning and return the longest existing candidate. Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 066dc83aba
ℹ️ 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 best.is_none_or(|b| candidate.len() > b.len()) { | ||
| best = Some(candidate); |
There was a problem hiding this comment.
Fresh evidence: this version now keeps best by candidate.len() across the whole prompt, so when a user mentions multiple existing images in one message, the formatter still attaches only one image but this detector can replace an earlier shorter path with a later longer one. For example, compare /tmp/a.png with /tmp/much-longer.png attaches the later file instead of the first referenced image, which is a regression from the old split_whitespace() scan order and changes the model input the user intended.
Useful? React with 👍 / 👎.
…idates The longest-wins rule must only apply when a longer match extends the same start (a spaced filename whose earlier extension is a prefix). Across distinct paths, preserve the first referenced one to match the prior scan-order semantics rather than attaching a later, longer path. Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
|
Thanks @DOsinga — really appreciate you taking it the rest of the way. The two boundary regressions were exactly the cases my rebase onto Done on my side: I've updated the PR description to match the current code — dropped the stale |
|
great. I'm going to merge this FWIW we're probably moving away from this and let the model handle this direclty using a tool call. more agentic! |
2 participants
Closes #9576
Summary
detect_image_pathdropped any image path containing a space — most importantly macOS screenshots likeScreenshot 2026-05-04 at 10.32.57 PM.png. Local vision through OpenAI-format providers (LM Studio, Ollama, etc.) then silently received no image, and the model would reply that it couldn't see the file. The function has since moved out ofcrates/goose/src/providers/utils.rsinto the newgoose-providerscrate (crates/goose-providers/src/images.rs) and was rewritten to split on whitespace there, which reintroduced the same blind spot — this PR re-applies and hardens the fix in its new home.Change (
crates/goose-providers/src/images.rs)Replace the whitespace split with an extension-anchored scan:
.png/.jpg/.jpegoccurrence (case-insensitive, allocation-free viafind_ascii_ciso the returned byte index stays valid for slicing), walk backward over/-rooted candidates and return one that is an existing image file (is_absolute+is_file+ magic-byteis_image_file)./, whitespace, a quote, or end of input) — so/tmp/shot.png.backupisn't truncated to/tmp/shot.png./must follow a whitespace/quote boundary — so a local path isn't carved out of a URL likehttps://host/some/local/path.png."/tmp/Screen Shot.png") match.Screen Shot.png edited.jpg).MAX_PATH_LEN, keeping the function linear even on extension-heavy prompts.Credit
The
mainmerge, the URL/longer-extension boundary hardening, and thecargo fmtfix were pushed to this branch by @DOsinga — thank you.Test plan (
images.rsunit tests, all passing)test_detect_image_path— existing relative / fake / nonexistent behavior preservedtest_detect_image_path_with_spaces— absolute and quoted paths containing spacestest_detect_image_path_ignores_urls_and_longer_extensions— URL suffix and.png.backupnot mis-extractedtest_detect_image_path_ignores_extension_flood— bounded scan on extension-heavy inputtest_load_image_filecargo fmtclean;cargo clippy -p goose-providers --all-targetsand theimagestests pass locally.