Rhidp 14314/attachment type model validation#3629
Conversation
|
This pull request adds a new top-level directory under |
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
JslYoon
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
PR Summary by QodoAdd model vision validation and block image attachments for non-vision models
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
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>
JslYoon
force-pushed
the
rhidp-14314/attachment-type-model-validation
branch
2 times, most recently
from
4171980 to
27c2c65
Compare
Signed-off-by: Lucas <lyoon@redhat.com>
JslYoon
force-pushed
the
rhidp-14314/attachment-type-model-validation
branch
from
27c2c65 to
2d859f5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3629 +/- ##
==========================================
+ Coverage 54.00% 54.03% +0.02%
==========================================
Files 2328 2329 +1
Lines 89277 89336 +59
Branches 24979 24987 +8
==========================================
+ Hits 48218 48271 +53
- Misses 39452 39458 +6
Partials 1607 1607
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
JslYoon
force-pushed
the
rhidp-14314/attachment-type-model-validation
branch
from
8a5871f to
a6b43f4
Compare
| }, | ||
| ); | ||
| const supportsVision = testResponse.ok; | ||
| ModelCapabilitiesCache.set(model, supportsVision); |
There was a problem hiding this comment.
Shouldn't we also include provider in the Key for cache.
Signed-off-by: Lucas <lyoon@redhat.com>
JslYoon
force-pushed
the
rhidp-14314/attachment-type-model-validation
branch
from
a6b43f4 to
31ee1ba
Compare
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:55 PM UTC · Completed 11:12 PM UTC |
ReviewFindingsHigh
Medium
Low
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]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -593,6 +598,7 @@ export async function createRouter( | |||
| '/v1/query', | |||
There was a problem hiding this comment.
[high] merge-conflict-risk
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.
Suggested fix: Rebase onto current main to pick up the rate-limiting middleware.
| */ | ||
|
|
||
| export const ModelCapabilitiesCache = { | ||
| cache: {} as Record<string, boolean>, |
There was a problem hiding this comment.
[medium] unbounded-cache
ModelCapabilitiesCache is a module-level plain object with no TTL, max size, or eviction policy. Any authenticated user can create unlimited cache entries with arbitrary model/provider strings. Stale results are served indefinitely.
Suggested fix: Use a bounded LRU cache with max ~1000 entries and TTL (~1 hour). Validate model/provider values.
| }, | ||
| ); | ||
|
|
||
| router.post( |
There was a problem hiding this comment.
[medium] missing-rate-limiting
The /v1/validate-model-vision endpoint has no rate limiter middleware, unlike every other POST endpoint in this router. Each uncached call triggers an outbound LLM request.
Suggested fix: Add expensiveRateLimiter to the /v1/validate-model-vision middleware chain.
| req: Request, | ||
| res: Response, | ||
| next: NextFunction, | ||
| ) => { |
There was a problem hiding this comment.
[medium] two-step-protocol
validateAttachmentsForModel requires clients to call /v1/validate-model-vision first. This coupling through shared mutable in-memory cache is fragile: backend restart between calls produces confusing 400 errors.
Suggested fix: Consider performing the vision check inline with caching as optimization rather than requiring a pre-flight call.
| @@ -601,6 +607,27 @@ export async function createRouter( | |||
|
|
|||
There was a problem hiding this comment.
[medium] scope-creep
PR adds attachment content processing (prepending non-image content to query, deleting attachments field) beyond stated validation scope. This query-mutation behavior could benefit from more explicit documentation.
| system_prompt?: string; | ||
|
|
||
| // Attachments array (file content sent with the query) | ||
| // Attachments array (e.g. DOM context, screenshots) |
There was a problem hiding this comment.
[medium] interface-breaking-change
QueryRequestBody.attachments changes from {name, content} to {attachment_type, content_type, content}. Frontend already uses new shape but changeset marks as minor, not breaking. Other consumers would silently break.
Suggested fix: Confirm no other consumers use old shape. Add validation rejecting old shape with clear error.
| response.json({ model, provider, supportsVision }); | ||
| } catch (error) { | ||
| logger.error(`Vision test error for ${cacheKey}:`, error); | ||
| ModelCapabilitiesCache.set(cacheKey, false); |
There was a problem hiding this comment.
I think we should not cache the false in case of errors, it can happen due to transient failure which will result into permanently mark the model as non-vision-capable.
| }, | ||
| ); | ||
| const supportsVision = testResponse.ok; | ||
| ModelCapabilitiesCache.set(cacheKey, supportsVision); |
There was a problem hiding this comment.
Should we consider adding TTL for it. I know once a model has vision support it might not change ?
2 participants
Hey, I just made a Pull Request!
/v1/validate-model-visionendpoint that probes model visioncapability by sending a minimal test JPEG to LCS
/v1/responsesvalidateAttachmentsForModelmiddleware on/v1/queryto rejectimage attachments for non-vision models
ModelCapabilitiesCachesingleton to cache vision capabilityresults per model
console.logdebug statement from notebooks routerquery validation
https://redhat.atlassian.net/browse/RHIDP-14314
✔️ Checklist