Repository navigation
Conversation
|
Comments Outside DiffThese findings could not be posted inline.
|
22c9a65 to
10a6fee
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
10a6fee to
1f73954
Compare
c1d4cae to
0012d22
Compare
|
Rebased onto current master and CI is green. This follows the review recommendation from your comment on #1302 (replay cannot duplicate partial output/tools, capability records keyed by actual provider/profile/model). Ready for review whenever you have time, happy to address any feedback. |
|
Small correction to my earlier note: upstream CI never actually ran on this branch. The runs are sitting in action_required because fork PRs need a maintainer to approve workflows first, so "CI is green" was wrong of me. What I actually verified: full local test suite passing on the branch, plus a local mirror of your CI gates (fmt, check, clippy, ratchets) clean relative to master baselines. Sorry for the confusion. |
0012d22 to
7024331
Compare
|
I slimmed the branch while re-verifying: the manifest-lock work (file_lock.rs, the install scripts, the BuildManifest update_with machinery) was riding along from my fork history and overlaps PR #1392, so I removed it from this one. The branch is now the modality recovery alone. The lock work is preserved on my side for a separate PR if there is interest. Local gates (fmt, check, clippy, budget scripts, e2e cohorts) now match master's baseline exactly, with no new findings introduced by the branch. |
ee31ba1 to
452348d
Compare
452348d to
fae6dbe
Compare
|
Fresh rebased single-commit head (fae6dbe). The latest deferred-tool-references finding from the review bot is fixed with a pinning test, and every earlier thread is addressed. Ready for review. |
fae6dbe to
5402f5c
Compare
b003994 to
a3a7a10
Compare
5731636 to
816193f
Compare
816193f to
cf65dff
Compare
c408fa0 to
e234267
Compare
| // Keys are normalized (trim + lowercase) so every caller agrees even when | ||
| // they hold different spellings of the same model id (raw runtime string, | ||
| // stripped/lowercased lookups, session-restored forms). | ||
| let model = model.trim().to_ascii_lowercase(); |
There was a problem hiding this comment.
Distinct models lose image support
If a direct endpoint serves distinct case-sensitive IDs such as text-only model-a and vision-capable Model-A, an image rejection from the first disables image input for the second. The new process-wide rejection cache lowercases both IDs, although custom-endpoint model selection preserves their exact spelling. This affects independent sessions on the same endpoint: the vision session replaces valid images with omission markers for up to 30 minutes, and new tool images are left out of its conversation.
Preserve the exact trimmed, session-prefix-stripped model ID in rejection records and lookups, including the runtime capability lookup. Keep catalog-specific case normalization separate.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-core/src/image_capability.rs
Line: 124-127
Comment:
**Distinct models lose image support**
If a direct endpoint serves distinct case-sensitive IDs such as text-only `model-a` and vision-capable `Model-A`, an image rejection from the first disables image input for the second. The new process-wide rejection cache lowercases both IDs, although custom-endpoint model selection preserves their exact spelling. This affects independent sessions on the same endpoint: the vision session replaces valid images with omission markers for up to 30 minutes, and new tool images are left out of its conversation.
Preserve the exact trimmed, session-prefix-stripped model ID in rejection records and lookups, including the runtime capability lookup. Keep catalog-specific case normalization separate.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 159ff39: rejection records and lookups now keep the exact case-sensitive model id, so a text-only model-a no longer disables images for a distinct vision-capable Model-A on the same endpoint. Added a regression test covering the mixed-case pair.
159ff39 to
81a59df
Compare
Text-only models behind optimistic OpenAI-compatible endpoints hard-fail the moment a tool result carries pixels, and the image block staying in history poisons every later turn. Adds runtime modality recovery in three layers: tool-result injection is capability-aware so pixels never reach a text-only provider, the system prompt gains a model image capability section telling the model up front to use text alternatives, and a new image_capability record (per provider+model, 30-min TTL) stores runtime rejections so supports_image_input mutes them and both the failover layer and the OpenRouter streaming path replay a rejected request once with images filtered to text markers on the same provider, without burning a failover or a retry slot. Streaming rejections are handled inside run_stream_with_retries because OpenAI-compat 400s surface asynchronously, after the outer retry decision point. Record keys use the stripped model id and stay case-sensitive, so records follow the actual provider/profile/model the rejection came from even when the runtime model string transiently carries a session-profile prefix after session restore. The replay filters image blocks before the first streamed token, so partial output and tool calls cannot be duplicated. Closes 1jehuang#1302.
81a59df to
2259d1f
Compare
Closes #1302.
Text-only models served behind optimistic OpenAI-compatible endpoints hard-fail the moment a tool result carries pixels, and because the image block stays in history every subsequent turn of the session is poisoned. This adds runtime modality recovery in three coordinated layers:
tool_output_to_content_blocks_with_image_supportat all three agent injection sites): for text-only providers, image blocks become an explicit text omission note, so pixels never reach the request.PromptCapabilities.text_only_modeladds a Model Image Capability section to the system prompt so the model knows up front to use text alternatives (OCR, file contents) or ask the user.image_capabilityrecord (per provider+model, 30-min TTL) stores runtime image-input rejections;supports_image_inputconsults it, and both the failover layer and the OpenRouter streaming path replay a modality-rejected request once with images filtered to text markers on the same provider (no failover burn, no retry slot burned: deterministic failures replay inside the attempt).Subtleties covered per the review guidance on #1302:
run_stream_with_retriesbecause OpenAI-compat 400s surface asynchronously in the stream, after the outer retry decision point.max_retries=1replay covered by wire-level red-green tests (record key, stream replay, filtered replay).Validation: wire-level red-green tests plus crate suites at baseline (
jcode-baseimage capability tests 27/27 green after rebase). One caveat from the issue: the endpoint side of the end-to-end validation used a scripted mock OpenAI-compat server; no live text-only model was reachable from the test machine, so the real-endpoint path is recorded as blocked, not silently claimed.Branch is a single commit rebased onto current master.