feat(*): send user attachments to the model, gated on vision support - #285
Conversation
turn.send gained an optional media param, so a caller can hand the spine attachment paths the way channels already do. Paths resolve with the filesystem tools' own policy, and one that does not resolve is dropped with a log line rather than failing the turn. A picture then reaches the model as an inline base64 image_url block when the model can see it, and as a note naming the file when it cannot. The verdict comes from the gateway catalog Raven already fetches for pricing, which publishes input_modalities for every model it lists, and it is warmed in the background because the pricing path only ever reaches that catalog for models LiteLLM's static table misses. Absence, staleness, an exception and a deployment name all read as "no answer", which resolves to optimism and is never cached. Being wrong that way fails loudly at the endpoint; being wrong the other way silently turns a picture into prose, and there is no recovery on the attachment path. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Three loose ends from the delivery change, all about what the model is told to do next. read_file is the tool that hands a model the picture, so the note it gets when a message cannot carry one now names read_file wherever read_file would work: past the blind check the model can see, and read_file downscales rather than refusing on size. The description tool is named only where it is the one that can help -- a non-image attachment, or a model with no vision at all -- and understand_media's own description now recognises the '[Image: ...]' note as a source of paths, which it had to after that note started pointing at it. read_file's description drops "when the active model can see images". It described an implementation detail the model cannot evaluate about itself, and the runtime now says so directly in the result instead. The blind branch also stops reading the file. Every reason to refuse is settled from the header and the path, so a model that cannot see a picture no longer pays to load one, up to the per-attachment ceiling. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
0xKT
left a comment
0xKT
left a comment
There was a problem hiding this comment.
Reviewed the full diff and verified the referenced symbols against main
(split_model_id, spec.client/is_local, _resolve_path, prepare_image,
Media fields, the attach_blocks -> pending_images hand-off). The
fail-direction design (unknown -> optimistic -> never cached) is
consistent across vision_verdict / _supports_vision and locked by tests.
CI is green and the 4 local test failures listed in Verification
reproduce on a pristine main. LGTM.
Three non-blocking notes, none worth holding the merge for:
R1 (nit) raven/context_engine/segments/render.py, build_user_content:
the can_see_images check runs after the file is read whole, so a blind
model attaching a large image reads up to 64MB just to emit a note.
Fix: hoist the can_see_images branch above raw = head + handle.read().
Verify: existing test_the_attachment_note_names_no_tool_when_none_is_registered
still passes; optionally assert reads == [_SNIFF_BYTES] for the blind path.
R2 (follow-up) raven/agent/loop/main.py, _supports_vision: self.provider
is passed for any routed id, so when the primary provider is
AzureOpenAIProvider every routed model classifies as caller-chosen and
stays optimistic. Only bites mixed Azure-primary + router setups and
fails in the loud direction; worth a line in the known-boundaries list
or a per-candidate provider when the fallback-chain follow-up lands.
R3 (nit) tests/test_read_file_image.py grew ~770 lines of vision/warm
coverage; the file's subject is read_file image handling. Consider
splitting a test_providers_capabilities.py in a follow-up.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Adversarial review pass. I read every hunk, opened the surrounding code, and
verified the first two findings by executing the code in a throwaway worktree at
this head (also ran tests/test_read_file_image.py test_plugin_tools.py test_tui_rpc_turn_send.py test_token_wise_pricing.py -> 201 passed).
The design is careful and the "absence means yes" bias is the right one -- the
asymmetry argued in the description holds up. The problems are all in the
exclusion list: the code that decides when a model string must not be looked up
in the vendor catalog. A name that is wrongly allowed through lands in exactly the
silent failure mode the description says must be avoided.
Five inline notes below, most severe first. Nothing here is a design objection.
Checked and found sound: the routing/assembly reorder (the router needs only
content); media_paths does not filter on Media.kind/mime, so kind="file"
is fine downstream; prepare_image meta keys and the 64-byte sniff are adequate
for detect_image_mime; the count and byte ceilings, and the header-only read for
blind and non-image attachments; _save_turn's image stripping keeps the path
notes; AssemblyContext/TurnContext are only ever constructed with keywords;
the Codex Responses provider does translate user-side image_url parts; pricing's
TTL logic is genuinely unchanged by the decision to leave _OPENROUTER_CACHE_TIME
alone.
| from raven.providers.azure_openai_provider import AzureOpenAIProvider | ||
| from raven.providers.registry import split_model_id | ||
|
|
||
| if isinstance(provider, AzureOpenAIProvider): |
There was a problem hiding this comment.
LazyProvider defeats this guard on the only path that reaches turn.send media.
The docstring above is right that "only the live provider instance knows" for a
bare Azure deployment name -- but raven tui builds the loop with
make_lazy_provider(), which is a plain proxy, not an AzureOpenAIProvider. So
for model="gpt-4" on an Azure deployment that has been repointed at gpt-4o:
- arm 1:
isinstance(provider, AzureOpenAIProvider)-> False (it is the lazy proxy) - arm 2:
split_model_id("gpt-4")[0]is not in_DEPLOYMENT_NAME_PREFIXES-> False - arm 3:
find_by_model("gpt-4")returns OpenAI's spec, sospec.is_localis
False andspec.client == "azure"is False
_model_id_is_caller_chosen returns False, vision_verdict consults the catalog,
the catalog answers about OpenAI's gpt-4, and vision is denied. The picture is
silently replaced by prose -- the failure mode the PR description singles out as
the one with no automatic recovery.
Verified at this head: vision_verdict("gpt-4", find_by_model("gpt-4"), lazy)
returns False, versus None when passed a real AzureOpenAIProvider.
Worth noting arm 3's spec.client == "azure" is unreachable for the same reason:
by the time a bare deployment name reaches here, find_by_model has already
resolved it to a vendor spec. Unwrapping the lazy proxy (or asking the provider a
question instead of type-testing it) would fix both arms.
| return True | ||
| if spec is None: | ||
| return False | ||
| return bool(spec.is_local) or spec.client == "azure" |
There was a problem hiding this comment.
custom belongs in this predicate, and the documented rollback is dead for it.
Any OpenAI-compatible endpoint configured as custom carries a served model
name chosen by whoever runs the endpoint -- the same situation as Azure, ollama and
vLLM, which are all excluded here. spec.is_local does not cover it (a custom
endpoint is frequently remote), so a served name like gpt-4 or llama-3-vision
gets joined against the vendor catalog and answered about somebody else's model.
The second half is worse: the PR describes ProviderSpec.vision_override as the
rollback for a wrong verdict, but for custom that escape hatch cannot be
reached. find_by_model() resolves a bare or openai/-prefixed id to the
vendor's spec, so the spec that arrives here is never the custom one --
route_names == {"custom"} while model_prefix == "openai". An override written
on the custom spec is never read, which means this case has no rollback at all,
not just a wrong default.
| """ | ||
| global _WARM_AT | ||
|
|
||
| if _OPENROUTER_CACHE: |
There was a problem hiding this comment.
A stale on-disk catalog suppresses the background warm permanently.
_cached_catalog_only (below) deliberately accepts a table of any age and, on the
way out, assigns it to _OPENROUTER_CACHE. This early return then sees a
non-empty cache and bails on every subsequent call.
So on a long-lived serve or TUI session that starts with a days-old cache file:
the first modality lookup populates _OPENROUTER_CACHE from disk, and the warm
never runs again for the life of the process. The cooldown reasoning in the
docstring covers the fetch-failed case but not this one -- there was never an
attempt to cool down from.
The two functions each make sense alone; the coupling is that "good enough for a
modality question" (any age) and "fresh enough that no warm is needed" are the
same flag. Gating this return on _OPENROUTER_CACHE_TIME freshness rather than on
_OPENROUTER_CACHE being non-empty would separate them.
| # window with both a fresher table and a fresh ``_OPENROUTER_CACHE_TIME``. | ||
| # Overwriting it with this stale copy would leave that timestamp vouching for | ||
| # the wrong table, and the fetch's TTL check would then skip the refetch. | ||
| if _OPENROUTER_CACHE: |
There was a problem hiding this comment.
This re-check is still an unlocked check-then-act, so it does not close the window the comment describes. (Lower confidence than the other notes -- I did not reproduce it.)
The comment above is precise about the hazard: a warm landing during load()
leaves a fresh _OPENROUTER_CACHE_TIME that would end up vouching for the stale
table. But the guard is a read at line 342 and a write at line 348 with no lock
between them, so the same interleaving simply moves:
- line 342 reads
_OPENROUTER_CACHE-- still empty, falls through - the warm thread completes: fresh table into
_OPENROUTER_CACHE, fresh
_OPENROUTER_CACHE_TIME - line 348 overwrites with
disk[0]
which is the exact state the comment says must not happen -- and now pricing, not
just modality lookups, serves the stale table until the TTL expires (up to an
hour), because the fresh timestamp survives.
The window is much narrower than the one before load(), so this may be an
acceptable trade rather than a bug. If it is, saying so in the comment would be
worth more than the current wording, which reads as though the re-check
eliminates the race.
| try: | ||
| resolved = _resolve_path(raw.strip(), workspace, allowed) | ||
| if not resolved.is_file(): | ||
| logger.warning("turn.send: attachment {} does not resolve to a file", raw) |
There was a problem hiding this comment.
A dropped attachment is invisible to both the model and the front end.
This continue (and the one at line 88) drops the path before
build_user_content ever sees it, so unlike an unreadable-but-resolved file --
which the description says "costs its own note in the message" -- a mis-spelled or
out-of-workspace path produces no [Attachment: ...] note at all. The RPC still
returns success, and the log line goes to the server log the user is not reading.
Net effect for a typo'd path: the model answers as though nothing was attached,
and the user believes it saw the image. That is the same silent shape the PR
argues against for the vision verdict, and it is reachable by ordinary user error
rather than by a catalog edge case.
Emitting a note for the dropped path (it does not need the file to exist -- the
raw string is enough) would make it visible in the transcript, which is where the
user would notice.
## Summary Closes out the provider path in one PR: the provider module redesign, every open issue and backlog item that lives on the identity / connection / routing / capability axes, and multi-endpoint failover as a new feature. 78 commits, one problem per commit, rebased onto current main. Redesign (base): four provider decisions were implemented outside `raven/providers/` and had drifted copies -- wire form, credential grammar, pin resolution, price/window ladder, cache dialect. Each now has one owning module the surfaces call. Fixes on that base, most visible first: - The context window walks one ladder (explicit config > the model's real window > documented fallback). Previously the config default 65536 fed trimming and budgets raw, so a 200k-window model lost two thirds of its context every turn; an unresolvable window now renders the gauge's empty state instead of a number that is nobody's. Construction and /model switches resolve without touching the network, and a switch parked behind a running turn re-resolves the window at adoption, not at the RPC call. - A provider without real streaming (azure, codex) no longer renders upstream errors as normal assistant text: the terminal stream delta carries the classification and joins the same recovery path the non-streaming call uses. Both classify their non-200 from the live status code (shared ProviderHTTPError) instead of regex-guessing the rendered text, which also removes the bare "404" substring match that misfiled a 400 whose body happened to embed one. - Codex SSE failures keep their structured code, so an overloaded backend is retried instead of classified unknown. - Orphan `</think>` recovery (backends launched without their reasoning parser) is gated to the backend shapes that produce it, so ordinary content mentioning the tag is never cut. - Fallback hops are vetoed when both identities are certain and disagree (previously a cross-vendor hop went out under the wrong key, or silently to the wrong backend on a shared model name); the knn path dispatches each hop to its own endpoint and inherits model_overrides through the rotor. - User model_overrides win over shipped extra_body defaults, and the merge no longer drops user keys behind a gateway. - The credential gate, the reader and the builder answer one way: a spec's shipped default address satisfies the gate exactly when the reader would serve it (custom runs on a bare key again, azure still demands its address), endpoint entries inherit the flat api_base/extra_headers per field, and every display face -- provider list, endpoint list, the TUI picker -- reports the same resolved view, secrets redacted (extra_headers values included, on the flat section field too). - The vision probe stops joining operator-chosen names against the vendor catalog: it reads through the TUI's lazy proxy to see the Azure transport, treats an explicit-selection gateway (custom) as caller-chosen, and the background catalog warm is no longer suppressed for the life of the process by a stale on-disk table. - Skill forge rewriter/gate follow the configured agent model (the only auxiliary LLM calls that did not). - The onboarding wizard refuses the six litellm vendors a bare API key cannot configure, with the actual requirement named, instead of writing a section that 401s forever. - The OAuth handoff replays a signal swallowed mid-login, so a SIGHUP no longer leaves a headless TUI. New feature -- multi-endpoint failover (several accounts on one vendor): - `providers.<name>.endpoints` (label / apiKey / apiBase / extraHeaders) with `endpointStrategy: sticky | round_robin`; the three credential spellings (explicit list, Gemini api_key_list, flat fields) resolve through one reader with strict precedence and no key merging. - `EndpointRotorProvider` rotates and fails over with per-endpoint cooldown (30s doubling to 300s, process-local state); auth failures rotate -- another account's key is exactly what a dead key needs -- while endpoint-agnostic failures return immediately. Streams rotate only before the first delta, so tokens are never replayed. - Managed from `raven provider endpoint add|remove|list` and the TUI model picker; the session footer names the active endpoint. Write faces refuse a keyless endpoint for key-credential providers (local deployments keep their legitimate keyless shape); invalid sections fail loudly instead of being read as empty and overwritten. - Verified live twice: the rotor directly, and the full stack from a config file through make_provider (a dead key 401s, cools, fails over, answers). The first live run caught the auth-rotation gap the mocks could not. Also in this PR: the onboarding wizard split (5069 -> 3000 lines, pure moves with every migrated monkeypatch target mutation-checked), CONTEXT.md terms (Provider Endpoint, window ladder), benchmark alignment (pinchbench now prices through the shared ladder instead of a private drifted copy), and the model picker reads the config twice per open instead of twice per provider row. Reviewed adversarially across three external rounds and four internal panel rounds on two model families: 24 before-merge findings raised in total, every one either fixed with a mutation-verified test or refuted with executed evidence (one reviewer finding was withdrawn after a 945-case main-parity sweep); final verdicts RATIFY. The rebase also adopted the review notes left on #282/#285 that landed on this code: image-capability verdicts are invalidated on a provider switch, and a parked switch logs its park and its adoption. Known follow-ups, named in the review thread (issues to follow): the fallback routing loop contradicts the registry's explicit-selection note for `custom` (pre-existing on main), the remaining bare status substrings in classify_error (429/5xx, pre-existing), per-hop identity rebuilding for fallback chains, and the pre-existing SessionInfo shape mismatch. ## Type - [x] Feature - [ ] Fix - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `uv run pytest tests/ -q` -- 6128 passed, 33 skipped, run after the rebase onto current main; the provider-path test files additionally re-run under an empty HOME with identical results. - `make lint-python` clean; commit messages pass commitlint and scripts/check_commit_messages.py across all 78 commits. - `cd ui-tui && npm run lint && npx tsc --noEmit && npx vitest run` -- 86 files, 982 tests passed; `npm run gen:rpc -- --check` in sync. - Live probes (OpenRouter, tiny max_tokens): rotor failover and full-stack assembly failover, both passing; logs kept locally. - Fixes are pinned by deletion mutations that turn a named test red; review rounds ran 22 such mutations and the two survivors were themselves fixed (one dead guard deleted, one vacuous test replaced). ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes User-visible behavior changes: `contextWindowTokens` unset now resolves to the model's real window (explicit values are fully respected and no longer overridden); the context gauge shows an empty state when the window is unknown; azure/codex upstream errors surface as errors instead of assistant text; the wizard refuses six key-only-unconfigurable vendors with the real requirement named; `custom` with only an apiKey starts again (as on main) and the picker no longer demands an address the gate does not; key-credential providers refuse keyless endpoints at write time and `endpoint add --api-key` becomes optional for local deployments; endpoint keys and extra_headers values are redacted in `provider get`/`list` and over RPC; a model switch logs when it parks behind a running turn and when it adopts. Rollback is a straight revert of the squash commit; no data migration is involved (config additions are opt-in fields). ## Related Issues Fixes #124, fixes #234, fixes #155, fixes #152, fixes #254, fixes #151, fixes #143, fixes #144, fixes #197. References #281 (not reproducible on current or reported code; the api_key forwarding it suspected is now pinned by regression tests), #119 (already fixed by #116; remaining item is the installer redirect, not provider code). --------- Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
3 participants
Summary
Before this change a picture a user attached never reached the model.
TurnRequest.mediawas wired end to end and channels already used it, butturn.sendhad nomediafield, so a front end could only paste the path into the message text. This adds that field and the delivery rules behind it.Getting the file in.
turn.sendtakes an optionalmediaarray of workspace paths, resolved with the filesystem tools' own policy (souploads/shot.pngmeans the same thing it means to every other tool). A path that does not resolve, resolves outside the allowed directory, or cannot be read costs its own note in the message rather than the whole turn. The list is bounded at 64 in the schema, and one message inlines at most 16 images or 16 MB of base64.Deciding how it reaches the model. A vision-capable model gets the image inlined as a base64
image_urlblock in the user message; anything else gets a text note naming the file, and the note names a description tool only when one is actually registered. The image is preprocessed by the sameprepare_imagethatread_filealready used: a 4032x3024 phone photo goes out as 1180x885, which is 30x less base64 than inlining it raw.Where the verdict comes from. The gateway catalog Raven already fetches and caches for pricing, which publishes
input_modalitiesfor every model it lists. That completeness is why it is the source rather than LiteLLM's price table, which statessupports_visionon under a third of its rows -- reading that silence as a denial would take a picture that reaches Grok, Llama 4 and the Qwen-VL family today and replace it with prose.The catalog is read from cache only, never fetched inside a turn, and warmed on a background thread with a cooldown. The warm is not optional: the pricing path asks LiteLLM first and only reaches this catalog when that table misses, which excludes every model Raven ships a default for, so without it the probe would answer optimistically forever on a fresh install.
Which way it fails. Absence, staleness, a lookup that raises, and a caller-chosen deployment name all read as "no answer", which resolves to optimism and is never cached. Being wrong that way fails loudly at the endpoint. Being wrong the other way is silent: the picture never arrives and the model answers from the surrounding text as though it had seen one, and unlike the tool-result path there is no automatic recovery. Deployment names are excluded for that reason --
gpt-4is the name Azure's own quickstarts use, and a team keeps the deployment name while repointing it at a newer model, so joining it against a vendor catalog answers about somebody else's model.Type
Verification
uv run pytest tests -q-> 1 failed, 5334 passed, 58 skippedLITELLM_LOCAL_MODEL_COST_MAP=true uv run pytest tests -q-> 3 failed, 5332 passed, 58 skippeduv run ruff check raven tests-> clean;uv run ruff format --check raven tests-> 791 files already formattednpm run lint:rpc-> generated.ts in sync;npm run type-check-> clean;npm test-> 969 passedAll 4 failures above reproduce on a pristine
mainworktree and are unrelated to this change:test_cli_theme.py::test_bold_accent_renders_styled_not_bareis arichversion issue, the twotest_token_wise_pricing.pyMiniMax-M3 assertions need a cost map the bundled LiteLLM does not carry, andtest_default_context_engine.py::TestTwoTrackConcurrency::test_skill_and_memory_run_concurrentlyis a load-sensitive timing assertion.Verified against a live catalog that the probe denies the five text-only models Raven ships defaults for and grants every vision model tested, while every deployment and local-runtime name stays optimistic.
Verified on a cold cache that the first turn answers optimistically without blocking, the background warm lands, and the next turn on the same long-lived loop gets the real verdict.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
mediais optional, so every existing caller is unaffected, and the channels producer ofTurnRequest.mediais untouched. Attachment paths are resolved with the same policy as the file tools, sorestrict_to_workspacestill bounds them and a path outside it is dropped.Pricing is deliberately unchanged: the catalog entry gained an
input_modalitiesfield and the on-disk cache version was bumped, but the pricing lookup itself has no deleted or modified lines, and cost and context-window resolution were compared againstmainacross 40 real model ids in both cost-map modes with identical results.Rollback is
ProviderSpec.vision_overrideper provider for a wrong verdict, and reverting the commit otherwise.Known scope boundary. Channels populate
TurnRequest.mediatoday and benefit immediately. The TUI does not reach this lane yet, and not for want of a client: it already implements both attachment gestures (a dropped path viaimage.attach, a clipboard image viaclipboard.paste), but the server side of those is anot supported in v0.1stub or absent entirely, and the TUI catches the failure silently. Connecting them is a follow-up PR, and it is server-side work --ui-tui/needs no change.Two follow-ups deliberately left out of scope: a
medialist that fails validation surfaces as-32603 internal_errorrather than-32602, which is how everyTurnSendParamsvalidation failure already behaves and needs a dispatcher change to fix; and the vision verdict is computed for the routed primary while a LiteLLM fallback further down the chain is sent the same message list, which the sibling transport probe shares and which needs the chain assembled per candidate.Related Issues
N/A