Skip to content

Tag models with vision capabilities and describe images for text-only models - #4114

Open
nourzakhama2003 wants to merge 4 commits into
dyad-sh:mainfrom
nourzakhama2003:feat/3982-vision-fallback-non-vision-models
Open

Tag models with vision capabilities and describe images for text-only models#4114
nourzakhama2003 wants to merge 4 commits into
dyad-sh:mainfrom
nourzakhama2003:feat/3982-vision-fallback-non-vision-models

Conversation

@nourzakhama2003

@nourzakhama2003 nourzakhama2003 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #3982
Closes #3774

Summary

Vision fallback for text-only models — Tags models with supportsVision and adds a fallback: when the selected model can't handle images, a vision-capable model (via dyad/vision/default alias) describes them instead, injecting the description as . Gated behind a new enableVisionFallback setting.

Text-only models (GLM 4.7/5/5.2, Qwen3 Coder, DeepSeek, MiniMax M2.5/M2.7, Nemotron) explicitly tagged false; unknown models default to capable.
Image parts stripped from replayed history and mid-turn tool injections (screenshots from web_crawl/run_tests) when the active model is text-only.
Error messages for capability failures translated into actionable text; also fixes a JSON.stringify circular-reference crash and an undefined request-ID bug.
No DB migration needed (settings/model fields only, not schema).
9 test files covering fallback logic, catalog overlay, history stripping, and settings defaults.

Tests

npm test -- src/ipc/utils/vision_fallback.test.ts
npm test -- src/ipc/utils/chat_attachment_utils.test.ts
npm test -- src/ipc/utils/token_utils.test.ts
npm test -- src/ipc/shared/remote_language_model_catalog.test.ts
npm test -- src/ipc/handlers/chat_stream_handlers.test.ts
npm test -- src/pro/main/ipc/handlers/local_agent/local_agent_handler.test.ts
npm test -- src/pro/main/ipc/handlers/local_agent/prepare_step_utils.test.ts
npm test -- src/main/settings.test.ts
npm test -- src/ipc/handlers/tests/vision_fallback.integration.test.tsx

@nourzakhama2003
nourzakhama2003 requested a review from a team July 27, 2026 04:14
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 1 inline finding(s).

Comment thread src/ipc/utils/vision_fallback.ts
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: ⛔ NO - Do NOT merge
Recommendation: human-review

Issues Summary

Severity File Issue
🔴 HIGH src/ipc/utils/vision_fallback.ts:126 Do not send attachments to another provider implicitly

The fallback selects the first configured vision provider and sends the user's image to it, even when that provider differs from the model/provider the user selected. This silently expands disclosure of potentially sensitive screenshots or project assets to an additional third party without consent or an explicit setting. The product needs an approved privacy boundary (for example, opt-in consent with the destination disclosed, or fallback restricted to the selected provider) before performing this upload.


Generated by Codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7b0a7075d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 17 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts
Comment thread src/ipc/utils/vision_fallback.ts
Comment thread src/ipc/utils/vision_fallback.ts Outdated

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/ipc/handlers/chat_stream_handlers.ts
Comment thread src/ipc/handlers/chat_stream_handlers.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

This PR tags models with vision capabilities and adds a robust text-description fallback for text-only models. The mechanism is sound and unusually well-tested (unit + integration + fake-catalog E2E). The four load-bearing correctness concerns I checked all hold up: the injected <dyad-image-description> block survives prepareMessageWithAttachments (the appended lastUserMessage is the same object passed in), supportsVision() treats unknown/undefined as capable and only an explicit false changes behavior, providerId is in scope for the convertRemoteCatalog overlay, and the second (auto/curated) mapping block correctly reads local MODEL_OPTIONS without needing the overlay. No database schema/Drizzle change, no UI-primitive (Base UI/Radix) change, no new renderer data-fetching, and the local-agent tool guards are untouched.

No HIGH-severity issues were found, so nothing blocks merge. The items below are informational MEDIUM/LOW notes.

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/chat_stream_handlers.ts:198 Capability-error substring matching can mislabel unrelated failures
🟡 MEDIUM src/ipc/handlers/chat_stream_handlers.ts:1751 VISION_UNAVAILABLE_NOTE shown even when a describer merely failed
🟢 Low Priority Notes (4 items)
  • 60s synchronous describe blocks the turn - describeImageAttachments runs inline on the user's turn behind an empty assistant placeholder for up to 60s with no progress signal. It is bounded and degrades to a note, but a slow vision model can make the chat look hung. Consider a shorter timeout or a transient "Describing attached image…" renderer status. (src/ipc/utils/vision_fallback.ts)
  • Only the closing tag is stripped from describer output - safeDescription strips a transcribed </dyad-image-description> (good escape defense) but not a stray opening <dyad-image-description>. Not an escape vector since the block is delimited by the single closing tag the module writes, but a nested opening tag could confuse a naive scanner. (src/ipc/utils/vision_fallback.ts)
  • Replayed past-turn images dropped with no user-facing note - When history replays image parts persisted under a prior vision model and the user switched to a text-only model, stripImageParts substitutes [image omitted: the selected model cannot read images], which reaches the model but there is no UI signal to the user that historical images were dropped and not re-described. Acceptable degradation; a one-time UI hint would improve transparency. (src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts)
  • Non-string last-user content skips injection - If lastUserMessage.content is unexpectedly non-string, injection is skipped with a warn log; any pre-existing image parts would then still reach a text-only model. Marked "shouldn't happen" and largely pre-existing behavior. (src/ipc/handlers/chat_stream_handlers.ts)

Note on confidence: the PR branch is not checked out in the working tree, so new-side line numbers for newly added files/blocks were derived from diff hunk headers against the current base files and may be off by a few lines. The verified base-file anchors (prepareMessageWithAttachments, convertRemoteCatalog, handleLocalAgentStream/loadChat) are exact.


Generated by Dyadbot persona-based code review

@nourzakhama2003
nourzakhama2003 marked this pull request as draft July 27, 2026 04:33
@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 27, 2026

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 1 inline finding(s).

Comment thread src/main/settings.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: ⛔ NO - Do NOT merge
Recommendation: human-review

Issues Summary

Severity File Issue
🔴 HIGH src/main/settings.ts:60 Require opt-in before sending images to another provider

The new fallback is enabled for every existing and new user, and it may upload image attachments to a provider other than the one selected for the chat. A settings-page hint does not provide consent before the first such upload, so upgrading Dyad can silently expand the set of third parties receiving a user's images. Make the cross-provider fallback opt-in (or obtain explicit consent at first use) before enabling it.


Generated by Codex

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 1 inline finding(s).

Comment thread src/ipc/handlers/chat_stream_handlers.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/chat_stream_handlers.ts:203 Generic image_url errors are misclassified as missing vision support

The bare image_url capability pattern also matches ordinary provider validation errors for malformed URLs, invalid image data, dimensions, or payload size. Those errors bypass the narrow media-type guard and are rewritten as “This model cannot read images,” sending users toward switching models while hiding the actionable attachment problem.


Generated by Codex

@nourzakhama2003
nourzakhama2003 marked this pull request as ready for review July 27, 2026 17:29
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 3 inline finding(s).

Comment thread src/pages/settings.tsx Outdated
Comment thread src/ipc/handlers/chat_stream_handlers.ts
Comment thread src/ipc/utils/vision_fallback.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Issues Summary

Severity File Issue
🟡 MEDIUM src/pages/settings.tsx:490 Disclose cross-provider image sharing beside the toggle
🟡 MEDIUM src/ipc/handlers/chat_stream_handlers.ts:412 Broad image_url match misclassifies invalid image data
🟡 MEDIUM src/ipc/utils/vision_fallback.ts:180 Preserve user-visible image names in descriptions

The visible setting does not disclose that fallback can send image attachments to a provider different from the selected chat provider. The capability-error matcher also treats any error containing image_url as a non-vision model error, which can hide malformed image-data failures behind incorrect advice to switch models. Finally, the describer labels images with hash-based stored filenames rather than the user-visible logical names, making multi-image descriptions difficult to associate with names used in the prompt.


Generated by Codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f857984f5d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/utils/token_utils.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec1b117528

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/chat_stream_handlers.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 1 inline finding(s).

Comment thread src/ipc/handlers/chat_stream_handlers.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/chat_stream_handlers.ts:414 Generic image_url errors are misreported as missing vision support

The new vision fallback is otherwise well-covered across attachment delivery, replayed history, and mid-turn local-agent injections.


Generated by Codex

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review: 1 inline finding(s).

Comment thread src/ipc/utils/vision_fallback.ts
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Code Review Summary (Codex)

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/utils/vision_fallback.ts:67 MIME-recognized images are skipped by the describer

The diff is complete, and no HIGH-severity issues were found.


Generated by Codex

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: ✅ YES - Ready to merge
Recommendation: ready

This PR tags models with supportsVision and adds a vision-fallback path that describes images with a vision-capable model when the selected model is text-only, gated behind a new opt-in enableVisionFallback setting. I reviewed all 25 changed files across correctness/security, code health, and UX, and cross-checked the key call sites against the repository source. The mechanism is coherent, well-guarded, and thoroughly tested. No HIGH or MEDIUM issues were found; only a few LOW-priority notes.

Things I specifically verified and found correct:

  • Default is consistently "off." DEFAULT_SETTINGS.enableVisionFallback = false, the switch reads ?? false, describeImageAttachments treats unset/false as off (safe on upgrade), and settings.test.ts plus the integration test agree. The PR description's "default on" wording is inaccurate, but the code is self-consistent and off-by-default is the correct privacy choice (images can be sent to a third-party provider).
  • No clobbering of the injected description. The appended <dyad-image-description> is written back to chatMessages[lastUserIndex], and the later prepareMessageWithAttachments reads that message's content as its base, so the description survives; for text-only models includeImageParts is false, so no image part is re-added.
  • History/tool stripping matches the two content representations. Replayed aiMessagesJson uses AI-SDK ImagePart (type: "image", handled by stripImageParts); mid-turn tool injections use Dyad's UserMessageContentPart (type: "image-url", handled by stripImageContentParts). Both text-only paths are covered, including compaction rebuild.
  • Dyad Pro fallback resolution correctly returns the first resolved alias without a per-provider key check (single gateway key), and the non-Pro path checks provider keys/env vars.
  • describeImageAttachments reads app-controlled stored file paths (not arbitrary user paths), bounds to MAX_DESCRIBED_IMAGES = 4 with a truncation note, applies a 60s timeout combined with the turn abort signal, strips the closing tag case/whitespace-insensitively to prevent block escape, and degrades to a retry note rather than a "switch models" nag on failure.
  • No Drizzle migration is required — the new fields are Zod/settings-JSON and ModelOption fields, not DB schema. The supportsVision custom-model gap is documented and acceptable (falls through to the error backstop).

The diff was complete (not truncated), which supports high confidence in this assessment.

🟢 Low Priority Notes (3 items)
  • Substring error patterns can misclassify non-capability "image" errors - IMAGE_INPUT_ERROR_PATTERNS uses bare includes checks such as "does not support image" / "not support image". A message like "does not support image generation" isn't caught by the format guard and would match here, prepending the misleading "Switch to a vision-capable model" advice. Impact is narrow (only when an image is attached and only for models the catalog hasn't tagged), and the code documents this as a best-effort backstop with logging. Optionally tighten to phrases like "support image input". (src/ipc/handlers/chat_stream_handlers.ts)
  • "Off by default" copy lives only in the search index - The settings-search description says "Off by default," but the Settings → Workflow page hint omits the default state and the PR body claims "default on." Consider adding "Off by default." to the page hint and correcting the PR description for a consistent user-facing message. (src/lib/settingsSearchIndex.ts)
  • Switch label is not internationalized - VisionFallbackSwitch hardcodes the English label/aria-label, whereas some sibling switches use translation keys. Minor and consistent with the surrounding hardcoded WorkflowSettings copy. (src/components/VisionFallbackSwitch.tsx)

Generated by Dyadbot persona-based code review

@github-actions

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 414 1 9 146
🪟 Windows 405 3 0 146

Summary: 819 passed, 4 failed, 9 flaky, 292 skipped

Failed Tests

🍎 macOS

  • package_manager.spec.ts > build mode - safe npm package installs through the real socket firewall path
    • Error: Command failed: npx --prefer-offline --yes sfw@2.0.4 --help

🪟 Windows

  • app_search.spec.ts > app search - empty search shows all apps
    • Error: expect(locator).toBeVisible() failed
  • app_search.spec.ts > app search - search by app name
    • Error: expect(locator).toBeVisible() failed
  • supabase_dependency_analysis.spec.ts > local-agent deploys affected Supabase functions using packaged dependency analysis
    • Error: expect(locator).toBeVisible() failed

📋 Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/package_manager.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • cloud_sandbox.spec.ts > cloud sandbox runtime mode runs previews (passed after 1 retry)
  • context_manage.spec.ts > manage context - smart context (passed after 1 retry)
  • debugging_logs.spec.ts > console logs should appear in the console (passed after 1 retry)
  • engine.spec.ts > regular auto should send message to engine (passed after 1 retry)
  • logs_server.spec.ts > system messages UI shows server logs with correct type (passed after 1 retry)
  • mention_files.spec.ts > mention file (passed after 1 retry)
  • multi_window_product_surface.spec.ts > opens an app in a second product window and survives closing the initiator (passed after 1 retry)
  • package_manager.spec.ts > build mode - blocked unsafe npm package shows the real socket verdict and preserves app files (passed after 1 retry)
  • turbo_edits_v2.spec.ts > turbo edits v2 - search-replace approve (passed after 1 retry)

📊 View full report

@wwwillchen wwwillchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nourzakhama2003 thanks for the PR. we should update our remote language catalog (which is in our private repo) to mark whether vision is supported for a model, before doing this PR.

The one thing I'm not sure about is the quality of getting a textual description of an image. Do you have some examples of descriptions generated using your prompt? in particular, including some examples in the PR description of image inputs and description outputs using gpt-5.6-luna which is a cheap and capable model, would be very helpful in validating whether this approach is effective. thanks

@nourzakhama2003

nourzakhama2003 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@nourzakhama2003 thanks for the PR. we should update our remote language catalog (which is in our private repo) to mark whether vision is supported for a model, before doing this PR.

The one thing I'm not sure about is the quality of getting a textual description of an image. Do you have some examples of descriptions generated using your prompt? in particular, including some examples in the PR description of image inputs and description outputs using gpt-5.6-luna which is a cheap and capable model, would be very helpful in validating whether this approach is effective. thanks

thanks for the review!

On the catalog: agreed, that should land first. The code already anticipates it , convertRemoteCatalog takes the server's supportsVision when present and only falls back to the local MODEL_OPTIONS tags otherwise, so the overlay becomes a no-op the moment the catalog ships the field. Happy to strip the local tags in a follow-up once it's out.

On description quality: fair concern, that's what decides whether this is worth having. I tested it two ways with gpt-5.6-luna as the describer.


1. End-to-end in Dyad

Chat model: GLM 5.2 (tagged supportsVision: false). Describer: gpt-5.6-luna. The input is a stock dashboard mockup

image

Asked the text-only model what do you see? in Ask mode. Its own reasoning trace, unedited:

The user is asking "what do you see" - they want me to describe the image they've shared. Based on the detailed description provided by the vision-capable model, I can describe what's in the dashboard mockup image.

image
Full response from GLM 5.2
Based on the image description, here's what I see:

Dashboard UI Mockup
It's a clean, modern dashboard interface with a light gray background, dark blue
and orange accents, soft drop shadows, and rounded corners.

Layout

Left sidebar (dark blue):
  - User profile at the top — avatar, name "JOHN DON", and email "johndon@company.com"
  - Six navigation links below: home, file, messages, notification, location, graph

Main content area (light gray):

  Header — "Dashboard User" title with a hamburger menu icon in the top-right

  Top row — 4 metric cards:
    Earning (dark blue card): $ 628
    Share (white card): 2434
    Likes (white card): 1259
    Rating (white card): 8.5

  Middle row — bar chart card:
    Title "Result" with an orange "Check Now" button
    Grouped bar chart (dark blue & orange) across Jan–Sep
    Tooltip showing 28,79 on the June column
    Legend for 2019 (orange) and 2020 (dark blue)

  Bottom row — two cards side by side:
    Left (wide): An area/wave chart with two overlapping gradient waves
      (orange & dark blue) on the left half, and a mini calendar grid on the right half
    Right (narrow): A donut chart showing 45% in the center, a list of four
      "Lorem ipsum" rows, and an orange "Check Now" button

Every value checks out against the source image: the name and email, all six nav items in order, all four KPI figures, the tooltip on the June column, the 2019/2020 legend colors, the 45% donut, the four Lorem ipsum rows, both "Check Now" buttons. Only drift is comma-vs-period decimals (8,58.5). It never saw the image.

Then Build this dashboard. in Agent mode — same text-only model, working from the description alone:

image

Log lines from that run:

vision gate: model=openrouter/z-ai/glm-5.2 supportsVision=false
Describing 1 image(s) with openai/gpt-5.6-luna

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

2 participants