fix(providers): treat unicode punctuation as image path terminators - #10106
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: 40bc73312e
ℹ️ 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".
| ..='\u{201F}' | '\u{2026}' | '\u{2039}' | '\u{203A}' | ||
| ) | ||
| || ('\u{2600}'..='\u{27BF}').contains(&c) | ||
| || ('\u{1F300}'..='\u{1FAFF}').contains(&c) |
There was a problem hiding this comment.
When a user appends a flag emoji immediately after an image path, e.g. /tmp/photo.png🇺🇸, the first scalar is a Regional Indicator Symbol (U+1F1E6..U+1F1FF), which is outside this range. detect_image_path therefore still treats the flag as part of the filename and returns None, so the newly added emoji-terminator behavior misses a common emoji class; include that range or use Unicode emoji properties if emoji suffixes are meant to terminate paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 87b633a by adding the Regional Indicator Symbol range (U+1F1E6..U+1F1FF) and covering a flag emoji suffix in the unicode separator regression test. Local verification: cargo fmt --check && cargo test -p goose-providers test_detect_image_path.
|
Thank you, I'm eagerly awaiting its merge with my major. |
|
@Osraka can you please look into merge conflicts |
87b633a to
22900a1
Compare
|
Resolved the merge conflicts by rebasing the branch onto current I kept the upstream Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22900a1ffa
ℹ️ 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".
| || ('\u{2600}'..='\u{27BF}').contains(&c) | ||
| || ('\u{1F1E6}'..='\u{1F1FF}').contains(&c) | ||
| || ('\u{1F300}'..='\u{1FAFF}').contains(&c) |
There was a problem hiding this comment.
When a user appends a common emoji outside these hard-coded ranges, such as /tmp/photo.png⭐ (U+2B50) or /tmp/photo.png⌚ (U+231A), is_path_terminator returns false, so detect_image_path treats the emoji as part of the filename and misses the existing image. This is still reproducible with U+2B50 even after the regional-indicator fix; include the remaining emoji blocks or use a Unicode emoji property if emoji suffixes are meant to terminate paths.
Useful? React with 👍 / 👎.
|
Addressed the latest Codex review note in I added the common emoji blocks that cover U+231A / U+2B50-style suffixes and added regression assertions for Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac42487c6
ℹ️ 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".
| '"' | '\'' | '\u{00AB}' | '\u{00BB}' | '\u{2013}' | ||
| ..='\u{201F}' | '\u{2026}' | '\u{2039}' | '\u{203A}' |
There was a problem hiding this comment.
When a user wraps the path in smart quotes, e.g. “/tmp/photo.png”, detection still returns None: the trailing ” now passes this terminator check, but the slash is skipped because the leading-boundary check still accepts only whitespace and ASCII quotes. Since ASCII-quoted paths are supported and this change adds Unicode quote terminators, the matching opening Unicode quotes should be accepted as path boundaries too.
Useful? React with 👍 / 👎.
|
Addressed the latest Codex review note in I added Unicode quote characters as valid leading path boundaries, matching the existing Unicode quote terminators. The regression coverage now includes smart double quotes and guillemets around paths. Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e846c72912
ℹ️ 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".
| || ('\u{2300}'..='\u{23FF}').contains(&c) | ||
| || ('\u{2600}'..='\u{27BF}').contains(&c) | ||
| || ('\u{2B00}'..='\u{2BFF}').contains(&c) | ||
| || ('\u{1F1E6}'..='\u{1F1FF}').contains(&c) | ||
| || ('\u{1F300}'..='\u{1FAFF}').contains(&c) |
There was a problem hiding this comment.
When a user appends a keycap emoji immediately after an existing image path, e.g. /tmp/photo.png1️⃣, the first scalar after .png is the ASCII digit 1, so it does not match any of these emoji ranges and detect_image_path treats the emoji sequence as part of the filename and returns None. After the added emoji ranges, this multi-scalar emoji case still remains outside the per-character predicate; handle keycap sequences (or use Unicode emoji segmentation/properties) before rejecting the suffix.
Useful? React with 👍 / 👎.
Port audited changes from aaif-goose/goose#10106. Upstream commits: 859672814, 22900a1ff, 0ac42487c, 29c47afa5. Local audit: upstream path crates/goose-provider-types/src/images.rs maps to gosling's crates/goose-providers/src/images.rs; production logic and tests applied cleanly after that path translation. Gate: source bin/activate-hermit && cargo fmt && cargo test -p goose-providers images::tests::test_detect_image_path
3 participants
Summary
file:casesFixes #9399.
Why
After the stricter path parsing changes, visually separated prompts like
{path}.png🙂or{path}.png… more textare treated as if the following Unicode character is part of the filename. That makesdetect_image_pathmiss valid local image paths.This keeps the broader rule conservative:
:is still not a terminator, and zero-width / combining characters remain glued so they do not reintroduce the false-positive class guarded by the earlier parser hardening.Verification
cargo fmt --checkcargo test -p goose-providers test_detect_image_path