Skip to content

Rhidp 14314/attachment type model validation#3629

Open
JslYoon wants to merge 12 commits into
redhat-developer:mainfrom
JslYoon:rhidp-14314/attachment-type-model-validation
Open

Rhidp 14314/attachment type model validation#3629
JslYoon wants to merge 12 commits into
redhat-developer:mainfrom
JslYoon:rhidp-14314/attachment-type-model-validation

Conversation

@JslYoon

@JslYoon JslYoon commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Hey, I just made a Pull Request!

  • Add /v1/validate-model-vision endpoint that probes model vision
    capability by sending a minimal test JPEG to LCS /v1/responses
    • Add validateAttachmentsForModel middleware on /v1/query to reject
      image attachments for non-vision models
    • Add ModelCapabilitiesCache singleton to cache vision capability
      results per model
    • Remove stray console.log debug statement from notebooks router
    • Unit tests for the cache and integration tests for both the endpoint and
      query validation
      https://redhat.atlassian.net/browse/RHIDP-14314

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

@github-actions

Copy link
Copy Markdown
Contributor

This pull request adds a new top-level directory under workspaces/. Please follow Submitting a Pull Request for a New Workspace in CONTRIBUTING.md.

@rhdh-gh-app

rhdh-gh-app Bot commented Jun 29, 2026

Copy link
Copy Markdown

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @red-hat-developer-hub/backstage-plugin-intelligent-assistant-backend

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-intelligent-assistant-backend workspaces/intelligent-assistant/plugins/intelligent-assistant-backend none v3.0.3

@JslYoon
JslYoon marked this pull request as ready for review June 29, 2026 21:13
@rhdh-qodo-merge

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

Grey Divider

Qodo Logo

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Add model vision validation and block image attachments for non-vision models

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add /v1/validate-model-vision endpoint to probe vision capability via a minimal JPEG request.
• Enforce image-attachment gating on /v1/query using cached per-model vision results.
• Add cache/unit tests and router integration tests covering vision validation and query rejection
 paths.
Diagram

graph TD
  C[Client] --> V["POST /v1/validate-model-vision"] --> MC[("ModelCapabilitiesCache")]
  V --> LCS["LCS /v1/responses"]
  C --> Q["POST /v1/query"] --> MW["validateAttachmentsForModel"] --> MC
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use LCS /v1/models supports_vision metadata (no probing)
  • ➕ Lower latency and load than sending a test /v1/responses request
  • ➕ Deterministic behavior with clearer 'model not found' semantics
  • ➕ Avoids false negatives caused by transient /v1/responses errors
  • ➖ Depends on LCS accurately maintaining supports_vision for each provider/model
  • ➖ May diverge from actual runtime capability if metadata is stale
2. Lazy validation on first image query (no pre-validation endpoint)
  • ➕ Simpler client flow; no extra endpoint call required
  • ➕ Validation stays close to the real usage path
  • ➖ Adds latency to the first /v1/query with images
  • ➖ Harder to distinguish transient failures vs true lack of capability
3. Provider+model keyed cache with TTL (vs model-only, permanent cache)
  • ➕ Avoids collisions when different providers reuse the same model identifier
  • ➕ TTL reduces risk from permanent false negatives after transient outages
  • ➖ Slightly more implementation complexity (keying + eviction/expiry)

Recommendation: The overall direction (explicit capability validation + middleware gating) is sound, but the current implementation should be aligned to a clear contract: (1) decide whether validation is based on /v1/models metadata or an active /v1/responses probe and make endpoint semantics reflect that choice; (2) key the cache by provider+model (and ideally add a TTL) to avoid cross-provider collisions and long-lived false negatives. Also, the router/middleware responses should be made consistent with the tests (field naming and error messages) to avoid an API/test contract mismatch.

Files changed (6) +412 / -1

Enhancement (4) +163 / -1
attachment-validation.tsIntroduce in-memory ModelCapabilitiesCache singleton +35/-0

Introduce in-memory ModelCapabilitiesCache singleton

• Adds a simple singleton cache for storing model -> supportsVision boolean. Exposes get/set/has/clear helpers for shared use by routing and validation logic.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/attachment-validation.ts

constant.tsAdd minimal base64 JPEG constant for vision probing +11/-0

Add minimal base64 JPEG constant for vision probing

• Defines TEST_VISION_JPEG as a 1x1 base64-encoded JPEG payload. Intended to be embedded as a data URL when probing model vision support.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/constant.ts

router.tsWire attachment validation middleware and add /v1/validate-model-vision endpoint +75/-1

Wire attachment validation middleware and add /v1/validate-model-vision endpoint

• Adds validateAttachmentsForModel into the /v1/query pipeline after request validation. Introduces a new POST /v1/validate-model-vision endpoint that probes LCS /v1/responses using a minimal JPEG and caches the result in ModelCapabilitiesCache.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts

validation.tsAdd validateAttachmentsForModel middleware for image support enforcement +42/-0

Add validateAttachmentsForModel middleware for image support enforcement

• Adds middleware that inspects attachments for image content and requires a prior cached vision validation. Rejects requests when the model was not validated or is cached as non-vision-capable; otherwise allows the request to proceed.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/validation.ts

Tests (2) +249 / -0
attachment-validation.test.tsAdd unit tests for ModelCapabilitiesCache behavior +56/-0

Add unit tests for ModelCapabilitiesCache behavior

• Introduces unit coverage for setting, retrieving, updating, and clearing cached vision capability values per model. Ensures unknown models return undefined and has() behaves correctly.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/attachment-validation.test.ts

router.test.tsAdd integration tests for model vision validation and query attachment gating +193/-0

Add integration tests for model vision validation and query attachment gating

• Extends router integration tests to cover the new /v1/validate-model-vision endpoint and attachment behavior on /v1/query. Clears the ModelCapabilitiesCache between cases to avoid cross-test contamination.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts

@rhdh-qodo-merge rhdh-qodo-merge Bot added enhancement New feature or request Tests labels Jun 29, 2026
@JslYoon
JslYoon force-pushed the rhidp-14314/attachment-type-model-validation branch 3 times, most recently from 27c2c65 to 2d859f5 Compare July 1, 2026 19:51
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.22222% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.15%. Comparing base (460877e) to head (8a402e6).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3629      +/-   ##
==========================================
- Coverage   57.34%   56.15%   -1.20%     
==========================================
  Files        2381     2383       +2     
  Lines       95589    95592       +3     
  Branches    26720    26728       +8     
==========================================
- Hits        54819    53679    -1140     
+ Misses      40531    40233     -298     
- Partials      239     1680    +1441     
Flag Coverage Δ *Carryforward flag
adoption-insights 84.54% <ø> (ø) Carriedforward from ad6810f
ai-integrations 69.16% <ø> (+0.10%) ⬆️ Carriedforward from ad6810f
app-defaults 69.79% <ø> (ø) Carriedforward from ad6810f
augment 46.67% <ø> (ø) Carriedforward from ad6810f
boost 75.89% <ø> (ø) Carriedforward from ad6810f
bulk-import 72.54% <ø> (-0.07%) ⬇️ Carriedforward from ad6810f
cost-management 13.55% <ø> (ø) Carriedforward from ad6810f
dcm 60.72% <ø> (ø) Carriedforward from ad6810f
extensions 56.31% <ø> (+0.01%) ⬆️ Carriedforward from ad6810f
global-floating-action-button 71.18% <ø> (ø) Carriedforward from ad6810f
global-header 62.24% <ø> (+0.07%) ⬆️ Carriedforward from ad6810f
homepage 47.67% <ø> (-0.13%) ⬇️ Carriedforward from ad6810f
install-dynamic-plugins 56.77% <ø> (ø) Carriedforward from ad6810f
intelligent-assistant 74.30% <92.22%> (+0.24%) ⬆️
konflux 91.98% <ø> (ø) Carriedforward from ad6810f
lightspeed 69.02% <ø> (ø) Carriedforward from ad6810f
mcp-integrations 83.40% <ø> (ø) Carriedforward from ad6810f
orchestrator 62.65% <ø> (-0.02%) ⬇️ Carriedforward from ad6810f
quickstart 65.04% <ø> (ø) Carriedforward from ad6810f
sandbox 79.56% <ø> (ø) Carriedforward from ad6810f
scorecard 82.64% <ø> (+<0.01%) ⬆️ Carriedforward from ad6810f
theme 83.85% <ø> (ø) Carriedforward from ad6810f
translations 5.12% <ø> (ø) Carriedforward from ad6810f
x2a 55.05% <ø> (-24.26%) ⬇️ Carriedforward from ad6810f

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 460877e...8a402e6. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JslYoon
JslYoon requested a review from its-mitesh-kumar July 1, 2026 20:14
@JslYoon
JslYoon force-pushed the rhidp-14314/attachment-type-model-validation branch from 8a5871f to a6b43f4 Compare July 1, 2026 20:20
},
);
const supportsVision = testResponse.ok;
ModelCapabilitiesCache.set(model, supportsVision);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we also include provider in the Key for cache.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@JslYoon
JslYoon force-pushed the rhidp-14314/attachment-type-model-validation branch from a6b43f4 to 31ee1ba Compare July 2, 2026 18:49
@its-mitesh-kumar

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:55 PM UTC · Completed 11:12 PM UTC
Commit: d903ed7 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [api-contract] router.ts:743 — The /v1/validate-model-vision endpoint destructures model and provider from request.body without validating they are present and non-empty strings. If either is missing, cacheKey becomes undefined/undefined, an upstream call is made with a malformed model identifier, and the result is cached under the wrong key. Unvalidated model/provider strings are also interpolated into log messages.
    Remediation: Add input validation at the top of the handler: if (typeof model !== 'string' || !model.trim() || typeof provider !== 'string' || !provider.trim()) { response.status(400).json({ error: 'model and provider are required' }); return; }

  • [error-handling] router.ts:800 — The catch block in /v1/validate-model-vision returns HTTP 200 with { supportsVision: false } on any exception (network error, DNS failure, timeout). The caller cannot distinguish a genuine "model does not support vision" from a transient failure. Unlike the happy path, the catch block does not cache the result, so every subsequent call will retry and potentially fail again. Other endpoints in this router return status 500 on unexpected exceptions.
    Remediation: Return an error status code (e.g., 502) in the catch block: response.status(502).json({ error: 'Vision capability check failed', model, provider });

  • [missing-rate-limiting] router.ts:740 — The /v1/validate-model-vision endpoint makes an outbound HTTP request to LCS /v1/responses but has no rate limiter in its middleware chain, unlike other endpoints in this router that make outbound calls (which use expensiveRateLimiter or generalRateLimiter).
    Remediation: Add generalRateLimiter or expensiveRateLimiter to the middleware chain.

  • [scope-creep] types.ts — The Attachments interface changes from {name: string, content: string} to {attachment_type: string, content_type: string, content: string}. This is a breaking change to the QueryRequestBody.attachments contract — the name field is removed and replaced with attachment_type and content_type. Any existing callers sending {name, content} will silently lose the name field. The interface is exported.
    Remediation: Confirm the attachment schema change is authorized. Document the breaking change and verify all consumers are updated.

Low

  • [edge-case] router.ts:750 — TOCTOU pattern in has()/get() calls: ModelCapabilitiesCache.has(cacheKey) followed by ModelCapabilitiesCache.get(cacheKey)! with a non-null assertion. The has() method internally calls get(), which deletes expired entries. If expiry falls between the two synchronous calls, get() returns undefined despite has() having returned true. The same pattern exists in validateAttachmentsForModel.
    Remediation: Replace with a single get() call: const cached = ModelCapabilitiesCache.get(cacheKey); if (cached !== undefined) { ... }

  • [denial-of-service] attachment-validation.ts:18ModelCapabilitiesCache is an unbounded in-memory cache with no maximum entry count. Authenticated users can create entries that persist for 24 hours by calling /v1/validate-model-vision with arbitrary model/provider strings.
    Remediation: Add a maximum cache size (e.g., LRU eviction with a cap of 1000 entries).

  • [naming-convention] types.ts:59 — The interface Attachments uses a plural name for a type representing a single attachment. Every other interface in the codebase uses singular nouns. Array<Attachments> reads awkwardly.
    Remediation: Rename to Attachment (singular) and update all references.

  • [tier-mismatch] router.ts:741 — The /v1/validate-model-vision endpoint uses lightspeedChatReadPermission, but it sends a POST with a test image to LCS /v1/responses, which is a write-like operation. Other endpoints that POST upstream use lightspeedChatCreatePermission. The store: false flag mitigates this somewhat.
    Remediation: Consider using lightspeedChatCreatePermission, or document the rationale for the lighter permission.

  • [code-organization] attachment-validation.tsModelCapabilitiesCache is a module-level singleton plain object, diverging from the DI pattern used throughout the plugin (e.g., lcsUrlCache is scoped within createRouter). The file is named attachment-validation.ts but only contains the cache — the actual validation logic lives in validation.ts. The publicly exposed .cache property leaks internals (tests directly mutate it).
    Remediation: Move the cache into createRouter closure (like lcsUrlCache), or rename to model-capabilities-cache.ts.

  • [api-shape] validation.ts:170validateAttachmentsForModel returns { error, model } for some 400 responses while the rest of the codebase consistently returns { error } only.
    Remediation: Remove the model field from error responses for consistency.

  • [edge-case] validation.ts:57 — Image validation only checks for JPEG magic bytes regardless of content_type. A PNG with attachment_type: 'image' and content_type: 'image/png' would be rejected by the JPEG magic byte check.
    Remediation: Gate the JPEG check on content_type === 'image/jpeg', or explicitly reject non-JPEG image types with a clear error.

  • [performance] validation.ts:36hasValidJpegMagicBytes decodes the entire base64 string into a Buffer to check the first 3 bytes. For 20MB attachments, this allocates a ~15MB buffer unnecessarily.
    Remediation: Slice first: Buffer.from(extractBase64(content).slice(0, 4), 'base64').

  • [test-inadequate] router.test.ts:1718 — Integration tests for /v1/validate-model-vision do not test the cache-hit path or the error/catch path.

Previous run

Review

Reason: stale-head

The review agent reviewed commit cd8e4353c6f2b2340d38b96ef0f264fd77b33081 but the PR HEAD is now 8d61302f6051a3322cf5b17df83250c2f4a8bd5b. This review was discarded to avoid approving unreviewed code.

Previous run (2)

Review

Findings

High

  • [merge-conflict-risk] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts:598 — The diff context for /v1/query shows middleware order as express.json, validateCompletionsRequest, validateAttachmentsForModel, requirePermission — but the current main branch has expensiveRateLimiter between express.json and validateCompletionsRequest. The PR branch is based on a pre-rate-limiting revision. Merging will likely cause a conflict, or if auto-merged, could silently drop the rate limiter from /v1/query.
    Remediation: Rebase onto current main.

Medium

  • [error-handling-gap] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — The /v1/validate-model-vision endpoint catches all errors and caches false (model does not support vision). Any transient error (network failure, timeout, 5xx) permanently caches the model as not supporting vision since ModelCapabilitiesCache has no TTL. A transient failure permanently blocks image attachments for that model until backend restart. The catch block also responds with 200 { supportsVision: false } instead of following the codebase's established 500 error pattern.
    Remediation: Do not cache false on transient errors. Only cache false on explicit 4xx rejection from the LLM. Or add a TTL to negative cache entries.

  • [unbounded-cache] workspaces/lightspeed/plugins/lightspeed-backend/src/service/attachment-validation.ts:18ModelCapabilitiesCache is a module-level plain object with no TTL, max size, or eviction policy. Any authenticated user with lightspeedChatReadPermission can call /v1/validate-model-vision with arbitrary model/provider strings, each creating a new cache entry. An attacker could exhaust server memory. The lack of TTL also means stale results are served indefinitely if a model's capabilities change.
    Remediation: Use a bounded LRU cache with a max size (~1000 entries) and a TTL (~1 hour). Validate that model/provider values match expected patterns before caching.

  • [missing-rate-limiting] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts:760 — The /v1/validate-model-vision endpoint has no rate limiter middleware, unlike every other POST endpoint in this router (which use either expensiveRateLimiter or generalRateLimiter). Each uncached call triggers an outbound LLM request.
    Remediation: Add expensiveRateLimiter to the /v1/validate-model-vision middleware chain.

  • [two-step-protocol] workspaces/lightspeed/plugins/lightspeed-backend/src/service/validation.ts:109 — The validateAttachmentsForModel middleware requires clients to call /v1/validate-model-vision before /v1/query when sending image attachments. This coupling through shared mutable in-memory cache state is fragile: if the backend restarts between the validate and query calls, the user gets a confusing 400 error.
    Remediation: Consider performing the vision check inline in the validation middleware (with caching as an optimization) rather than requiring a separate pre-flight call.

  • [scope-creep] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts:607 — The PR adds attachment content processing logic in /v1/query that prepends non-image attachment content to the query string and deletes the attachments field from the request body. While the changeset mentions "capability to add attachments on /v1/query", this query-mutation behavior could benefit from more explicit documentation in the PR description.

  • [interface-breaking-change] workspaces/lightspeed/plugins/lightspeed-backend/src/service/types.ts:62QueryRequestBody.attachments changes from {name, content} to {attachment_type, content_type, content}. The frontend already uses the new shape, so this aligns backend with frontend. However, the changeset marks this as minor rather than a breaking/major change. Any other consumer sending the old shape will silently break.
    Remediation: Confirm no other API consumers use the old shape. Consider adding validation that rejects the old shape with a clear error message.

Low

  • [middleware-ordering] router.ts:600validateAttachmentsForModel runs before requirePermission on /v1/query. An authenticated but unauthorized user can trigger validation logic and observe different error messages based on cache state, though the information leakage is minimal.
  • [input-validation] router.ts:762 — No validation that model and provider are present, non-empty strings before constructing the cache key and outbound request.
  • [info-disclosure] validation.ts:127 — Error message "Please call /v1/validate-model-vision first" leaks internal endpoint path.
  • [authorization-scope] router.ts:760/v1/validate-model-vision uses lightspeedChatReadPermission but triggers actual LLM inference (cost implications).
  • [inline-types] router.ts:618 — Repeated inline type annotations for attachment arrays instead of referencing QueryRequestBody or extracting a named Attachment type.
  • [express-json] router.ts:757/v1/validate-model-vision lacks per-route express.json() with a size limit, unlike other POST endpoints.
  • [constant-placement] constant.ts:213TEST_VISION_JPEG is a base64 blob in the constants file; other constants are simple scalars.
  • [singleton-pattern] validation.ts:101validateAttachmentsForModel imports a module-level singleton cache, a new pattern; existing shared state uses router closure scope.

Labels: PR introduces new API endpoint and modifies validation middleware with security-relevant findings (missing rate limiting, unbounded cache, middleware ordering).

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

/fs-review

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:03 PM UTC · Completed 10:17 PM UTC
Commit: 00a479b · View workflow run →

JslYoon and others added 11 commits July 20, 2026 18:21
Add backend validation for image attachments, including a model vision
capability check via /v1/validate-model-vision endpoint and middleware
to reject attachments for non-vision models on /v1/query.

RHIDP-14314

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Lucas <lyoon@redhat.com>
Signed-off-by: Lucas <lyoon@redhat.com>
Signed-off-by: Lucas <lyoon@redhat.com>
Signed-off-by: Lucas <lyoon@redhat.com>
Signed-off-by: Lucas <lyoon@redhat.com>
Signed-off-by: Lucas <lyoon@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 6:31 PM UTC · Ended 6:47 PM UTC
Commit: 2e23467 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 6:31 PM UTC · Completed 6:47 PM UTC
Commit: 2e23467 · View workflow run →

@sonarqubecloud

Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants