Repository navigation
Conversation
Adds per-image and batch AI descriptions to the gallery, mirroring the
existing AI Tagging feature: POST /api/gallery/{image_id}/ocr,
POST /api/gallery/ocr-batch, POST /api/gallery/clear-captions, an
AI Descriptions settings panel (model picker, progress bar, cancel,
clear, X/Y described badge), and a per-image redo action in the detail
dropdown.
Long captions collapse behind a show more / show less toggle
(_captionBlock in gallery.js) because thinking-capable vision models
produce long multi-paragraph output — the shared VISION_DESCRIBE_PROMPT
puts a short overview first, then a blank line, then everything else,
and the toggle splits on that blank line. Full text stays in the DOM
for search either way. The toggle buttons use a new
.gallery-caption-toggle-btn class in style.css instead of a duplicated
inline style attribute (same declarations, still var(--red), no new
literals).
A vision model that spends its whole token budget thinking and returns
empty content falls back to a plain-text rendering of its reasoning
(_reasoning_as_caption), so a deployment running only a thinking-capable
vision model still gets a usable caption instead of nothing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/api/gallery/{image_id}/ocr opens its own 300s httpx client, but the
app's request-hard-timeout middleware aborts anything not on its exempt
list at 45s. Added a regex-based exemption (the image id sits mid-path,
so a plain prefix can't express it) scoped to exactly this new route
(^/api/gallery/[^/]+/ocr$). Not the same change as fix/vision-request-timeout
(a separate branch/PR), which exempts the pre-existing ai-tag route —
a different endpoint. Both branches independently declare
_TIMEOUT_EXEMPT_PATTERNS in app.py, so whichever merges second will need
a trivial one-line merge to combine the two tuples.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
analyze_image_with_vl_result's llm_call now passes max_tokens=VISION_MAX_TOKENS and temperature=0.2 (same thinking-model fix as the ai-tag path), and its timeout moved 120 -> 300 for the same reason the OCR route needs one. The test's fake_llm_call didn't accept the new kwargs, so it raised TypeError and the test silently asserted against the exception fallback text instead of the real call. Found while running the full suite before opening the gallery-ocr-descriptions PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
the
ready for review
The prompt bundled 4 distinct, enumerable rules (detail + overview/ blank-line structure, no-markdown formatting, and a report-only-visible/ transcribe-exactly accuracy clause) across several sentences, including a meta-explanation of *why* (rendered as plain text). A thinking-capable vision model itemized and audited its own compliance against each rule number-by-number in its reasoning instead of describing the image — reproduced live on a batch OCR run over an unusually text/UI-dense image (a screenshot of the app's own vision-text editor). Compresses to two short sentences with no explicit rule framing. Keeps the overview/blank-line/rest structure intact (_captionBlock in gallery.js splits on that blank line for the show-more toggle) and the accuracy guard, drops the meta-explanation and the itemized no-markdown list. Ran a controlled A/B against the same troublesome image before attributing the fix, since a higher VISION_MAX_TOKENS was floated as a competing explanation: old prompt + 1200 tokens: rambles (original report) old prompt + 2000 tokens: still rambles (tested) new prompt + 2000 tokens: clean, single-pass description (tested) Confirms the prompt structure is the variable, not the token budget. VISION_MAX_TOKENS left at 1200 here, unchanged — matches fix/vision-tag-max-tokens (PR odysseus-dev#5965), not the 2000 used for this same A/B test on my local instance; reconciling those two values is a separate call, not something to bundle into this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ran the full 2x2 test (old/new prompt x 1200/2000 tokens) against the same troublesome image on a live deployment: old prompt + 1200: rambles old prompt + 2000: rambles new prompt + 1200: rambles new prompt + 2000: clean, single-pass description 1200 (matching fix/vision-tag-max-tokens, PR odysseus-dev#5965) isn't enough headroom for a thinking-capable model to finish reasoning through a complex or text-dense image and still produce a full answer, regardless of prompt wording — that's a separate constraint from the rule-itemizing fix in the prior commit. 2000 is required for the describe/OCR path. VISION_MAX_TOKENS is shared with the ai-tag route too, so this also raises its ceiling — harmless there since tagging output is short (PR odysseus-dev#5965 measured ~850 tokens for a six-tag request against the same model), it just won't ever hit the new higher cap. This deliberately diverges from odysseus-dev#5965's 1200. Describing an image in detail and listing a handful of tags are different-shaped tasks that happen to share one constant; 1200 was calibrated for the latter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Observed live on my local instance: ai-tag batch calls failing repeatedly with
httpx.ReadTimeout at exactly ~60s (the client's own timeout, separate
from the app's 45s hard-timeout middleware, which ai-tag is already
exempt from). The exception handler correctly reports
{"error": "Auto-tagging failed"} rather than silently succeeding, so
batch failure counts were accurate — the calls were genuinely timing out.
Directly caused by this PR's own VISION_MAX_TOKENS bump (1200 -> 2000,
prior commit): more budget means the model can legitimately take longer
to respond, and VISION_MAX_TOKENS is shared between ai-tag and OCR. The
OCR route already uses timeout=300 for the same reason; ai-tag never
got the same treatment until now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Live evidence on my local instance: a batch ai-tag call succeeded (HTTP 200, no timeout) but wrote an empty ai_tags string — confirmed against the DB. ai_tag_image never had the same fallback the OCR path already has for this exact failure mode (thinking models spend the whole token budget in `reasoning` and leave `content` empty). Raising VISION_MAX_TOKENS and the httpx timeout earlier fixed the timeout failures but not this — a verbose enough thinking model can still exhaust 2000 tokens of reasoning on a tag-listing task and never reach content. Reuses the existing _reasoning_as_caption() salvage helper. It's tuned for prose (captions), not a clean tag list, so the result may be a handful of long, sentence-shaped 'tags' rather than a tidy comma list — strictly better than silently writing nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ions/tags
Live evidence: when a thinking model spends its whole VISION_MAX_TOKENS
budget on reasoning and returns empty content, the previous fallback
(_reasoning_as_caption) tried to salvage prose out of the raw reasoning
block and write it as the caption/tags. Its cleanup heuristics only
catch one specific rambling shape (bullets, 'Input:'/'Task:' labels,
'wait'/'let me' self-corrections) — a model that instead numbers a
markdown-bold planning list ('1. **One or two sentence overview.**'),
or writes 'Drafting the response:' / 'Final Polish:' section headers,
sails straight through unfiltered. Reproduced live: a product-label
image's caption ended up as 5000+ characters of raw planning scaffolding
because of exactly this.
Since the fallback's own output quality is unpredictable and heuristic
matching against free-form model rambling is an unwinnable game of
whack-a-mole, drop the salvage attempt entirely. Return a clear,
actionable error instead — 'This vision model spent its whole budget
thinking instead of answering. Try again, or switch to a different
vision model in Settings → Vision.' — and leave the existing
caption/tags untouched rather than overwriting them with either
garbage or nothing.
This is non-deterministic (temperature=0.3): the exact same image,
prompt, and token budget can succeed cleanly on one attempt and hit
this path on the next, confirmed by retrying the product-label image
and getting a clean caption immediately after. The error message tells
the user that directly, since it's genuinely worth just trying again.
Removes _reasoning_as_caption() entirely — no longer called anywhere,
confirmed via grep across routes/, src/, and tests/.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
'Settings → Vision' understates the actual path — the Vision model picker lives under Settings → AI Defaults → Vision (id=set-vlModelSelect in index.html, under the AI Defaults tab), not directly under Settings. Fixes all 9 occurrences across gallery_routes.py and document_processor.py, including the new error message added in the prior commit, which copied the existing (already wrong) convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RaresKeY
left a comment
RaresKeY
left a comment
There was a problem hiding this comment.
Thanks for the work on AI-generated gallery descriptions and OCR. I reviewed the latest head across the gallery OCR/tag routes, vision-provider integration, timeout handling, and gallery UI, and found several author-actionable issues affecting provider compatibility and long-running operations.
Findings
-
Problem: The new OCR handler builds an Anthropic or OpenAI Chat Completions request and posts it directly to the resolved endpoint, then parses only
choices[0].message. Native Ollama and ChatGPT Subscription use different request/response contracts. -
Impact: OCR fails for supported native Ollama and ChatGPT Subscription configurations even when the selected model is vision-capable.
-
Ask: Route gallery OCR through the existing provider-aware vision/LLM transport, or implement equivalent provider-specific handling, with focused mocked coverage for each supported provider shape.
-
Location:
routes/gallery/gallery_routes.py:2308-2371
-
Problem: The OCR route resolves one model and performs one remote request, without using the configured vision fallback chain used by the existing document vision path.
-
Impact: A primary endpoint outage or transient failure returns an OCR error even when a working vision fallback is configured.
-
Ask: Reuse the owner-aware vision fallback chain for gallery OCR and cover primary-fails/fallback-succeeds behavior.
-
Location:
routes/gallery/gallery_routes.py:2314-2365
-
Problem: The handler now waits up to 300 seconds, but the request-timeout middleware exempts only the OCR route; AI tagging remains under the default 45-second deadline.
-
Impact: A slower valid vision request is cancelled around 45 seconds and returns a timeout instead of completing.
-
Ask: Align the AI-tag route with the intended long-running timeout policy and add a regression test for the chosen contract.
-
Location:
routes/gallery/gallery_routes.py:2480-2484,app.py:180-205
-
Problem: Each OCR request calls synchronous model discovery, including blocking model-list probes, and the batch UI repeats that route once per image.
-
Impact: Slow or unavailable endpoint probes add avoidable per-image latency and can block unrelated async requests during a batch.
-
Ask: Resolve/cache the selected endpoint once per batch or move blocking discovery off the event loop, then add a bounded unavailable-endpoint or multi-image check.
-
Location:
routes/gallery/gallery_routes.py:2311-2319,src/ai_interaction.py:135-174
-
Problem: The gallery selector flattens every model returned by
/api/modelsand ignores the endpointmodel_type, while the backend accepts the selected override without requiring a vision-capable model. -
Impact: Users can choose a text model from an OCR control and receive a failed or nonsensical image request.
-
Ask: Restrict the selector and backend override to vision-capable models, or reject incompatible choices with an actionable error.
-
Location:
static/js/gallery.js:2227-2243,routes/gallery/gallery_routes.py:2314-2323
-
Problem: OCR batch selection includes all active media, video/non-image extensions fall back to
image/jpeg, and the detail menu exposes OCR without checking for video entries. -
Impact: Video bytes are sent to an image model and users see an action that cannot produce a valid description.
-
Ask: Filter OCR to image media, reject unsupported files before encoding, and hide or disable OCR for videos.
-
Location:
routes/gallery/gallery_routes.py:2259-2279,routes/gallery/gallery_routes.py:2300-2306,static/js/gallery.js:1460-1463
-
Problem: Individual OCR success updates the detail object but does not refresh the described counter, library data, or existing grid DOM.
-
Impact: The gallery can continue showing an old described count or card state until another library refresh.
-
Ask: Update shared gallery state or refresh the library after individual OCR success, with a focused UI-state check.
-
Location:
static/js/gallery.js:1735-1744,static/js/gallery.js:78-88
-
Problem: The new OCR model selector displays
Use Settings → Vision, but the actionable settings path isSettings → AI Defaults → Vision. -
Impact: Users who need to configure a vision model may not find the setting required to use OCR.
-
Ask: Change the selector label to the current
Settings → AI Defaults → Visionpath and add a small UI-string regression check if this navigation text is contractual. -
Location:
static/js/gallery.js:2202
Validation
- I did not run provider, browser, database-concurrency, or CI checks in this pass. Please add focused mocked provider/fallback tests, timeout coverage, model/media validation, UI-string coverage, and individual OCR UI refresh checks.
- The latest head was checked against current dev at the source level; live CI/check status and runtime behavior still need confirmation before publication.
…sport Addresses all 8 review findings on this PR: - P1: ocr_image/ai_tag_image hand-built Anthropic- and OpenAI-Chat-Completions -shaped requests directly, so they failed outright against native Ollama and ChatGPT Subscription vision endpoints. Both routes now go through a shared _call_vision_model() helper that calls src.llm_core.llm_call_async per candidate — the same transport chat/document vision calls already use, which knows how to speak to all four provider shapes. - P2: neither route consulted the configured vision fallback chain (Settings -> AI Defaults -> Vision -> Fallbacks). _resolve_vision_candidates() now builds [primary] + resolve_vision_fallback_candidates(), and _call_vision_model() walks it in order, falling through on a hard failure *or* on a candidate that returns only thinking markup (empty after strip_think) — improving on the old single-shot "reasoning-only -> immediate error" behavior for OCR specifically. - P2: ai-tag was still cut off by the 45s request-timeout middleware deadline (only ocr had the exemption, despite ai-tag also waiting up to 300s in the handler). _TIMEOUT_EXEMPT_PATTERNS now matches both. - P2: model discovery (_resolve_vl_model) makes a blocking HTTP probe for OpenAI-compatible/native-Ollama endpoints; it now runs via asyncio.to_thread inside _resolve_vision_candidates() instead of blocking the event loop per-image in batch OCR/tagging runs. - P2: an explicit OCR ?model= override wasn't checked for vision capability. _resolve_vision_candidates() now rejects a non-vision override via src.chat_helpers.model_supports_vision(); the admin-configured default is still trusted without this check, same as everywhere else it's used. The gallery.js model picker also now filters obviously-non-vision models out of the dropdown (same heuristic Settings -> AI Defaults -> Vision uses). - P3: OCR/AI-tag were offered for video gallery entries, sending video bytes to an image model. Both routes now reject video filenames up front (_gallery_filename_is_video), ocr-batch excludes them from the queue/count, and the gallery.js detail menu no longer renders either action for videos. - P3: describing a single photo from the detail view left the "X/Y described" badge stale until the next full library refresh. The click handler now increments the counter locally for a genuinely new (non-skip, non-redo) description. - P3: the OCR model picker's placeholder said "Use Settings -> Vision" instead of the correct "Settings -> AI Defaults -> Vision" path. New tests: tests/test_gallery_vision_transport.py covers the fallback chain, the thinking-only-response fallthrough, the vision-capability gate on an explicit override (and that the configured default skips it), video rejection, and the timeout-exemption pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rt rewrite
- test_vision_owner_scope.py::test_request_vision_call_sites_pass_owner
asserted the exact literal call shape gallery_routes.py used to resolve
the vision model inline (`_resolve_vl_model(configured, owner=user)`).
The provider-aware rewrite moved that call inside a shared
_resolve_vision_candidates() helper, where the parameter is named `owner`
rather than `user` by the time it reaches _resolve_vl_model. Owner is
still threaded through end-to-end (route -> _call_vision_model(..., user,
...) -> _resolve_vision_candidates(model_override, owner) -> here) — only
the literal text changed. Updated the assertion to match.
- test_gallery_endpoint_hardening.py::test_no_raw_exception_string_in_client_responses
flagged `except ValueError as e: return {"error": str(e)}` in the two
rewritten routes. The hardening test's actual concern is exception
internals leaking to the client; that's not what's happening here — every
ValueError _call_vision_model raises carries a hand-authored, client-safe
message, never a wrapped caught exception. Renamed the catch variable
(not `e`) with a comment explaining why, which both satisfies the
guard's real intent and stops the false positive.
- test_pr6020_browser_review_regressions.py's Node smoke-test harness was
missing a `navigator` global stub, which static/js/sessions.js reads at
module load — same pre-existing gap already fixed on my local instance (commit
f3e78935) and on PR odysseus-dev#6148's branch, surfacing here because this branch's
history never had it. One-line fix, not otherwise related to this PR's
vision-transport change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rt rewrite
- test_vision_owner_scope.py::test_request_vision_call_sites_pass_owner
asserted the exact literal call shape gallery_routes.py used to resolve
the vision model inline (`_resolve_vl_model(configured, owner=user)`).
The provider-aware rewrite moved that call inside a shared
_resolve_vision_candidates() helper, where the parameter is named `owner`
rather than `user` by the time it reaches _resolve_vl_model. Owner is
still threaded through end-to-end (route -> _call_vision_model(..., user,
...) -> _resolve_vision_candidates(model_override, owner) -> here) — only
the literal text changed. Updated the assertion to match.
- test_gallery_endpoint_hardening.py::test_no_raw_exception_string_in_client_responses
flagged `except ValueError as e: return {"error": str(e)}` in the two
rewritten routes. The hardening test's actual concern is exception
internals leaking to the client; that's not what's happening here — every
ValueError _call_vision_model raises carries a hand-authored, client-safe
message, never a wrapped caught exception. Renamed the catch variable
(not `e`) with a comment explaining why, which both satisfies the
guard's real intent and stops the false positive.
- test_pr6020_browser_review_regressions.py's Node smoke-test harness was
missing a `navigator` global stub, which static/js/sessions.js reads at
module load — same pre-existing gap already fixed on dev-local (commit
f3e78935) and on PR odysseus-dev#6148's branch, surfacing here because this branch's
history never had it. One-line fix, not otherwise related to this PR's
vision-transport change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GQJfuAUg9wFgcfwA8z3dcU
|
Thanks for the thorough review — fixed all 8 findings in f2831c2 (+ f93dab4 for the test-suite fallout from the rewrite), both pushed. P1 — Gallery OCR bypasses provider-specific vision transports P2 — Gallery OCR ignores configured vision fallbacks P2 — AI tagging still cut off by the 45s deadline P2 — OCR performs blocking model discovery inside an async request P2 — OCR model picker offers non-vision models P3 — OCR/AI-tag offered for video gallery entries P3 — Individual OCR success leaves the gallery summary stale P3 — OCR selector points to the wrong settings path Full suite run in-container after the rewrite: 11 pre-existing failures unrelated to this branch ( |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # raises carries a hand-authored, client-safe message (see | ||
| # its docstring) — it never wraps a caught exception's own | ||
| # text, so surfacing it to the client is intentional. | ||
| return {"error": str(vision_err)} |
There was a problem hiding this comment.
False positive, believe it's safe to dismiss — this is the same pattern already called out in the code comment right above this line (added when the internal test_no_raw_exception_string_in_client_responses hardening test flagged the same shape).
Every ValueError that reaches this except comes from _call_vision_model/_resolve_vision_candidates, and all of their raise sites construct the message from a fixed, hand-authored string literal — never from a caught lower-level exception's own text. The one place a real underlying exception is captured (last_err in _call_vision_model) is logged server-side only via type(e).__name__ (routes/gallery/gallery_routes.py:137) and deliberately never included in the message that gets raised (line 155 raises a plain "Vision model request failed", no interpolation). So str(vision_err) here can never carry stack-trace/internal-exception content — CodeQL's dataflow analysis can't see that guarantee since it's enforced by convention across the raise sites rather than by a type the analyzer can verify, hence the flag.
Happy to restructure this (e.g. thread the safe messages back as plain return values instead of exceptions) if that's preferred over dismissing, just let me know.
| content, _model_name = await _call_vision_model(img_path, tag_prompt, user) | ||
| except ValueError as vision_err: | ||
| # See the matching comment in ocr_image above. | ||
| return {"error": str(vision_err)} |
There was a problem hiding this comment.
Same reasoning as the reply on the sibling finding at line 2452 — see that comment for the full explanation.
…gnment
CI (Node 22) flagged this on the pushed head: Node 21+ ships its own
built-in `navigator` global as a getter-only property, so
`globalThis.navigator = { platform: "" }` throws "Cannot set property
navigator of #<Object> which has only a getter". The container this branch
was verified in runs Node 20, which doesn't predefine `navigator` at all, so
the plain assignment silently worked there and the CI-only failure wasn't
caught before pushing.
Object.defineProperty(...) overrides the property regardless of whether
Node predefines it (configurable: true lets it replace Node's own getter-
only descriptor) or not. Verified directly against both:
- host Node v22.22.1 (closest local match to CI's v22.23.2): reproduced the
original TypeError, confirmed the fix resolves it, and ran the full smoke
script end-to-end (not just the isolated stub line) with the expected
{activeController: true, touched: true, synced: true} result.
- container Node v20.19.2: full test file still green (12 passed).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same bug caught live on PR odysseus-dev#6019's CI run (Node 22), present here too: the navigator stub this file added for sessions.js's module-load read used a plain assignment, globalThis.navigator = { platform: "" }. Node 21+ ships its own real `navigator` global as a getter-only property, so that throws "Cannot set property navigator ... which has only a getter" on Node 22 (GitHub's CI runner). This repo's Docker containers run Node 20, which doesn't predefine `navigator` at all, so the plain assignment silently worked there and this was never caught locally before now. Object.defineProperty(...) overrides Node's own descriptor when present and still works identically when it isn't. Verified directly against both before pushing (not just the container, which had already given false confidence on the identical bug once): host Node v22.22.1 running the full smoke-test script end-to-end (not just the isolated stub line), and container Node v20.19.2 (12/12 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQJfuAUg9wFgcfwA8z3dcU
… first _resolve_vl_model()'s auto-detect path (vision_model unset) only tried a fixed list of generic hosted-model names (gpt-4o, claude-*, llava, ...), never the endpoint/model actually configured as the app's default chat model. gallery_routes.py's own _resolve_vision_candidates() (added earlier in this PR) calls this same function, so on a local-only setup where the default chat model is vision-capable but carries a custom name (e.g. a locally built Ollama Modelfile), OCR/AI-tag's auto-detect could never resolve to anything — surfacing "no vision model configured" even though a perfectly good vision model is the one already in everyday use. In-scope fix since it's this PR's own auto-detect code path, not a pre-existing issue being reached across to fix. Now tries the configured default_model first (only using it if model_supports_vision() actually agrees it's vision-capable) before falling through to the generic candidate list, which is also expanded and brought in line with the app's own current curated hosted-provider roster (routes/model_routes.py) and local vision-model keyword families (src/chat_helpers.py's _VISION_MODEL_KEYWORDS).
|
Pushed a fix ( The bug: The fix:
Verified against a live instance (confirmed the bug reproduced with |
…'s model Extends the auto-detect fix from 7fefaad with the same priority upstream change already applied on my local instance for the chat "Vision text" editor: a gallery image promoted from a chat upload (GalleryImage.session_id) may have been uploaded in a session running a different model than the app-wide default — the session it actually came from is a more specific, more likely answer than the global default_model. _resolve_vl_model() now takes an optional session_id and tries that session's own model first (only using it if model_supports_vision() agrees it's vision-capable), before falling through to default_model and then the generic candidate list exactly as before. Threaded through _resolve_vision_candidates() and _call_vision_model() down to both ocr_image and ai_tag_image, which already have `img` (the GalleryImage row) in scope and pass img.session_id. session_id is trusted only after confirming it belongs to the requesting owner (checked inside _resolve_vl_model) — a foreign or bogus id just silently doesn't match, same fail-safe behavior as the chat-side version. Adds a test confirming session_id reaches _resolve_vl_model from _resolve_vision_candidates, and fixes existing tests' mocks/fakes (_resolve_vl_model, _resolve_vision_candidates) and the literal source-text regression guard in test_request_vision_call_sites_pass_owner for the new parameter/call shape. Verified with an isolated one-off image build + throwaway container (not layered on the running local container) — 17/17 in the relevant test files pass.
|
Pushed a follow-up ( Change: auto-detect's priority order is now session's own model → app-wide Added a test confirming |
…iptions # Conflicts: # routes/gallery/gallery_routes.py # src/constants.py # static/js/gallery.js # static/style.css # tests/test_pr6020_browser_review_regressions.py
|
Synced with
I also checked that this PR doesn't duplicate anything that came in with the release merge. Tests: across the 138 gallery/vision/OCR/caption test files shared with |
…rt rewrite
- test_vision_owner_scope.py::test_request_vision_call_sites_pass_owner
asserted the exact literal call shape gallery_routes.py used to resolve
the vision model inline (`_resolve_vl_model(configured, owner=user)`).
The provider-aware rewrite moved that call inside a shared
_resolve_vision_candidates() helper, where the parameter is named `owner`
rather than `user` by the time it reaches _resolve_vl_model. Owner is
still threaded through end-to-end (route -> _call_vision_model(..., user,
...) -> _resolve_vision_candidates(model_override, owner) -> here) — only
the literal text changed. Updated the assertion to match.
- test_gallery_endpoint_hardening.py::test_no_raw_exception_string_in_client_responses
flagged `except ValueError as e: return {"error": str(e)}` in the two
rewritten routes. The hardening test's actual concern is exception
internals leaking to the client; that's not what's happening here — every
ValueError _call_vision_model raises carries a hand-authored, client-safe
message, never a wrapped caught exception. Renamed the catch variable
(not `e`) with a comment explaining why, which both satisfies the
guard's real intent and stops the false positive.
- test_pr6020_browser_review_regressions.py's Node smoke-test harness was
missing a `navigator` global stub, which static/js/sessions.js reads at
module load — same pre-existing gap already fixed on dev-local (commit
f3e78935) and on PR odysseus-dev#6148's branch, surfacing here because this branch's
history never had it. One-line fix, not otherwise related to this PR's
vision-transport change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7a3c9de to
15b4b9f
Compare
Same bug caught live on PR odysseus-dev#6019's CI run (Node 22), present here too: the navigator stub this file added for sessions.js's module-load read used a plain assignment, globalThis.navigator = { platform: "" }. Node 21+ ships its own real `navigator` global as a getter-only property, so that throws "Cannot set property navigator ... which has only a getter" on Node 22 (GitHub's CI runner). This repo's Docker containers run Node 20, which doesn't predefine `navigator` at all, so the plain assignment silently worked there and this was never caught locally before now. Object.defineProperty(...) overrides Node's own descriptor when present and still works identically when it isn't. Verified directly against both before pushing (not just the container, which had already given false confidence on the identical bug once): host Node v22.22.1 running the full smoke-test script end-to-end (not just the isolated stub line), and container Node v20.19.2 (12/12 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both "regenerate" entry points — the per-message footer's ↻ "Regenerate
from here" button, and the vision editor's "Regenerate message" button —
call POST /api/session/{id}/truncate, which does a real, permanent SQL
delete of every message after the point clicked, then generates a new
reply. Clicking either on anything but the very last exchange did this with
zero warning: the button's own label doesn't say it also erases the rest
of the conversation, and neither button visually signals it's destructive
differently from any other message-footer action.
Caught live: a mis-click destroyed a real conversation with no way to
recover it (no backup covered the affected window). This adds a
confirmation prompt — via the existing uiModule.styledConfirm — whenever
the action would delete more than just the message(s) being regenerated,
stating the exact count of messages that would be lost. Regenerating the
most recent exchange (the everyday case, nothing exists after it) is
unaffected and still proceeds with zero friction.
New test: tests/test_regenerate_truncate_warning_js.py — drives the real
regenerateFrom/resendUserMessage functions under Node with a fake message
list DOM, confirming: the last-exchange case never prompts and truncates
directly; an earlier case prompts with the correct count; cancelling
aborts before any network call; confirming proceeds with the correct
keep_count. Also fixed a pre-existing, unrelated gap this surfaced while
verifying the full suite: test_pr6020_browser_review_regressions.py's
Node harness never stubbed `navigator` at all (sessions.js reads
navigator.platform at module load) — a gap on origin/dev itself, not
caused by this branch's diff, using the same Object.defineProperty fix
already applied on odysseus-dev#6019/odysseus-dev#6148 for the sibling issue there (Node 21+'s own
getter-only navigator global, not relevant to this specific failure but
the same defensive pattern).
Verified: full pytest suite in-container, 5798 passed / 11 pre-existing
unrelated failures (README/docs/agent-loop baseline) / 0 new.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
15b4b9f to
416d47e
Compare
Same bug caught live on PR odysseus-dev#6019's CI run (Node 22), present here too: the navigator stub this file added for sessions.js's module-load read used a plain assignment, globalThis.navigator = { platform: "" }. Node 21+ ships its own real `navigator` global as a getter-only property, so that throws "Cannot set property navigator ... which has only a getter" on Node 22 (GitHub's CI runner). This repo's Docker containers run Node 20, which doesn't predefine `navigator` at all, so the plain assignment silently worked there and this was never caught locally before now. Object.defineProperty(...) overrides Node's own descriptor when present and still works identically when it isn't. Verified directly against both before pushing (not just the container, which had already given false confidence on the identical bug once): host Node v22.22.1 running the full smoke-test script end-to-end (not just the isolated stub line), and container Node v20.19.2 (12/12 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@RaresKeY the changes you requested here have been addressed (see the replies above), and the branch is synced with post-release CI hasn't run on this PR yet: the fork's workflows are waiting for a maintainer to approve them. |
…refresh grid Finishes the odysseus-dev#6019 review: - Gallery vision failures are typed (VisionRequestError subclasses), each with one fixed message. The OCR and AI-tag routes answer with _vision_error_message(err), chosen by type, so no exception text reaches a client response (the two CodeQL "information exposure" alerts). - _call_vision_model rejects anything but PNG, JPEG, WebP and GIF before resolving models or reading bytes, instead of labelling it image/jpeg. - The OCR model picker skips endpoints whose model_type is not 'llm' (image generation and similar), as the review pointed out. - A single description now re-renders the grid as well as the count, since each card's label comes from its caption. tests/test_gallery_vision_review.py covers each provider shape through the real llm_core with mocked HTTP and Codex streams (OpenAI-compatible, native Ollama, Anthropic, ChatGPT Subscription), unsupported files and videos rejected before anything is sent, fixed route messages, discovery running off the event loop, the picker filter, the settings path copy and the single-description refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@RaresKeY following up on your review: the transport, fallback, timeout, off-loop discovery, video and copy fixes were in, but several asks weren't fully met. This push closes them.
The coverage you asked for is in
Primary-fails/fallback-succeeds and the AI-tag timeout exemption were already covered in |
odysseus-dev#6019 replaces ai_tag's inline vision request with the shared _call_vision_model, so this test's patches no longer apply here. It still runs on odysseus-dev#5965's own branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It holds AI-written descriptions as well as text read off the image. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matches the panel heading and the other AI description controls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… unused OCR cancel button Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The candidate list replaced dev's and only had qwen2-vl and qwen3-vl, so Ollama's qwen2.5vl:7b and HF-style Qwen2.5-VL-7B-Instruct were never picked (odysseus-dev#6487). Add both spellings, matching odysseus-dev#6489, and carry its test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ges; offer fork (#6225) * fix(chat): warn before "regenerate" permanently deletes later messages Both "regenerate" entry points — the per-message footer's ↻ "Regenerate from here" button, and the vision editor's "Regenerate message" button — call POST /api/session/{id}/truncate, which does a real, permanent SQL delete of every message after the point clicked, then generates a new reply. Clicking either on anything but the very last exchange did this with zero warning: the button's own label doesn't say it also erases the rest of the conversation, and neither button visually signals it's destructive differently from any other message-footer action. Caught live: a mis-click destroyed a real conversation with no way to recover it (no backup covered the affected window). This adds a confirmation prompt — via the existing uiModule.styledConfirm — whenever the action would delete more than just the message(s) being regenerated, stating the exact count of messages that would be lost. Regenerating the most recent exchange (the everyday case, nothing exists after it) is unaffected and still proceeds with zero friction. New test: tests/test_regenerate_truncate_warning_js.py — drives the real regenerateFrom/resendUserMessage functions under Node with a fake message list DOM, confirming: the last-exchange case never prompts and truncates directly; an earlier case prompts with the correct count; cancelling aborts before any network call; confirming proceeds with the correct keep_count. Also fixed a pre-existing, unrelated gap this surfaced while verifying the full suite: test_pr6020_browser_review_regressions.py's Node harness never stubbed `navigator` at all (sessions.js reads navigator.platform at module load) — a gap on origin/dev itself, not caused by this branch's diff, using the same Object.defineProperty fix already applied on #6019/#6148 for the sibling issue there (Node 21+'s own getter-only navigator global, not relevant to this specific failure but the same defensive pattern). Verified: full pytest suite in-container, 5798 passed / 11 pre-existing unrelated failures (README/docs/agent-loop baseline) / 0 new. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(chat): cover plain resend after dev's replace-by-default change dev made plain resend (no options) replace from the clicked turn by default, with append-only resend opt-in via { append: true } (see test_resend_message_nondestructive.py). The footer "Resend message" button calls resendUserMessage(el) with no options, so it now truncates everything after the clicked turn and goes through this PR's warning. Replace the old "plain resend never prompts" test, which assumed the previous append-by-default behavior, with one asserting that plain resend on an earlier pair prompts with the right count, and one asserting that explicit append-only resend never truncates or prompts. Also resolve ui.js/sessions.js through the same cache-busting query string chat.js imports them with. ES modules cache by full URL, so the harness's unversioned import of ui.js was a second module instance and its uiMod.styledConfirm stub never reached the instance chat.js calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(chat): warn before an edit deletes later messages; offer to fork instead Sending an edited user message truncates the chat at that message, the same unrecoverable server-side delete as regenerate and resend, but did it with no prompt. It now goes through the same confirmation, worded for the action ("Sending this edit will also permanently delete the N messages after it"). Cancelling leaves the editor open with the draft. The confirmation for all three actions (regenerate, resend, edit) also offers "Fork from here": it forks the chat just before the user turn, switches to the new chat, and replays the action there (the same message, or the edited text), leaving the original chat untouched. forkFrom now shares the same fork helper. Tests drive the real editUserMessage Send handler under Node and cover the edit prompt (count, cancel, last-message no-prompt) and the fork choice for each action (fork keep_count, no truncate, replayed text). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(chat): cut and fork a chat by message id, not loaded-page position Regenerate and edit sent keep_count, the clicked message's position among the messages loaded in the page. A long chat loads only its newest messages until the user scrolls up, so that position is smaller than the real one and the truncate deleted real messages above the clicked one (regenerating the latest reply in a ~50-message chat removed about a dozen earlier messages). Resend already sent before_msg_id; regenerate and edit now do the same. The fork options this PR adds had the same flaw. Fork now accepts before_msg_id (fork before that message) and through_msg_id (include it), resolved against the stored messages; keep_count stays as the fallback for a message that has no id yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(chat): keep a regenerated turn's message id current; stop on a refused cut Regenerate keeps the original user bubble on screen and resubmits its text without drawing a new bubble. The resubmitted message is a new DB row, but the bubble kept the deleted row's id, so the next regenerate's cut was refused (404) and the page sent the turn anyway, stacking a second reply. The stream now reports the saved user message's id (user_message_saved), which the page attaches to the user bubble, or to the bubble a regenerate kept. Regenerate and edit stop with an error when the server refuses the cut instead of sending. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(chat): name the buttons in words instead of symbols CONTRIBUTING asks for no Unicode emoji in UI or code; the test comments used the regenerate and pencil glyphs to name buttons. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(chat): share the fork-and-send step across edit, resend and regenerate Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(chat): pass the regenerate fork its attachment ids explicitly Resend already hands its collected file ids to _forkAndSend; regenerate relied on the earlier _pendingRegenAttachments assignment. Pass _regenIds too so both fork paths read the same, and check the hand-off in the regenerate tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(chat): assert fork choices keep the user message's attachments Run regenerate and resend's "Fork from here" against a user message with an uploaded file and check that the send fired in the fork carries its file id, replacing the source-text check. chat.js exposes a read-only _pendingRegenAttachmentIds() for this, like cookbook-hwfit.js's test hooks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(chat): preserve attachments across edit and regeneration --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Alexandre Teixeira <general@alexandreteixeira.dev>
Summary
Adds AI-generated photo descriptions to the gallery, mirroring the existing AI Tagging feature: a per-image action, a batch mode, a model picker, and a clear-all.
Scope note relative to #6017: this PR is a manual mitigation, not the fix for that issue's automatic-captioning gap. #6017 is about
chat_handler.pynever syncing a caption when the configured chat model is multimodal — this PR doesn't touchchat_handler.pyat all and doesn't change that behavior. What it does provide is a way to get a caption for any gallery image on demand, chat-model-agnostic, which covers the same end-user need (a described photo) through a different, user-triggered mechanism. The automatic fix — a post-response hook that captions fresh multimodal attachments without touching the existing chat branch — is #6148, which closes #6017.POST /api/gallery/{image_id}/ocr— describes one image with a vision model, writesgallery_images.caption.?model=overrides the configured Vision model;?force=1overwrites an existing caption.POST /api/gallery/ocr-batch— returns the ids still needing a description; the client loops over them one at a time so cancelling mid-batch just stops issuing requests (everything already processed stays committed).POST /api/gallery/clear-captions— mirrors the existingclear-ai-tags.X/Y describedbadge), plus a per-image "AI description" / "Redo AI description" item in the detail dropdown._captionBlockingallery.jssplits on that blank line to collapse the rest behind a "show more" toggle. Full text stays in the DOM for search either way, only the display is truncated.contentfalls back to a plain-text rendering ofreasoning, so a deployment running only a thinking-capable vision model still gets a usable caption instead of nothing.Two supporting changes this PR's own code requires, in scope because the feature above doesn't work without them:
/api/gallery/and end with/ocror/ai-tag.tests/test_vision_owner_scope.py: this branch's vision call now passesmax_tokens=/temperature=kwargs (same thinking-model fix as the existing AI Tagging path), but the test'sfake_llm_calldidn't accept them, so it silently asserted against an exception-fallback value instead of the real call. Fixed the mock and updated its assertions.Target branch
dev, notmainLinked Issue
Part of #6017 — this is the manual mitigation, not the automatic fix; see the scope note in the Summary above.
Part of #5983 —
?force=1re-runs a photo's description, which is that issue's gallery half. #5968 covers the chat half.The vision candidate list here replaces dev's, so it carries #6489's Qwen2.5-VL names (#6487) and its test.
Type of Change
Checklist
?force=1re-run is its gallery half, and feat(chat): add Re-run button to the vision text editor #5968 covers the chat half.devdocker compose up --build) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
docker compose up -d --build, open Gallery → Settings.X/Y describedbadge update.0/Y.Checks run:
python -m py_compile app.py routes/gallery/gallery_routes.py src/constants.py src/document_processor.py,node --check static/js/gallery.js,docker compose config, and the fullpytestsuite inside the built container (tests/isn't shipped in the image, so copied in for the run) — 4901 passed, 4 pre-existing failures unrelated to this change (README wordmark/ASCII-banner formatting and an.env-handling test; this PR touches none of those files), 1 failure found and fixed (see above).Environment tested: Docker deployment, Ollama backend, Ubuntu.
Visual / UI changes — REQUIRED if you touched anything that renders
memory-count,settings-select,memory-toolbar-btn,gallery-select-btn,admin-card) — no new colors, font sizes, or spacing units there. The one place this branch did introduce a new bit of styling — the caption show more/show less toggle — used to be a duplicated inlinestyle="..."attribute on both buttons; pulled it into a single.gallery-caption-toggle-btnclass instyle.css(same declarations, still built onvar(--red), no new literals).Co-Authored-Bytrailers.Screenshots / clips
AI Descriptions settings panel — model picker, progress bar, Clear/Start buttons, live count badge.
Caption, collapsed (show more) — long OCR captions show a short overview with a "show more" link instead of the full text.
Caption, expanded (show less) — same image, after clicking "show more": full description with a "show less" link to re-collapse.
