fix: coerce corpus route filters#1776
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Summary by CodeRabbit
WalkthroughAdds request-body coercion and validation to the POST /api/corpus handler: introduces ALLOWED_CORPUS_TYPES, coerceStringArray(), and coercePositiveInteger(); normalizes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as CorpusRoutes
participant Builder as CorpusBuilder
participant Res as Response
Client->>Route: POST /api/corpus { types, concepts, files, limit }
Route->>Route: coerceStringArray(types/concepts/files)
Route->>Route: coercePositiveInteger(limit)
alt invalid input
Route->>Res: res.status(400).json(error)
else valid input
Route->>Builder: build({ filter, limit })
Builder->>Route: build result
Route->>Res: res.json(result)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/worker/http/routes/CorpusRoutes.ts (1)
15-15: Avoid duplicating the corpus-type allowlist.This set now mirrors the literal union in
src/services/worker/knowledge/types.ts. If a new corpus type is added there later, this route will start rejecting valid requests until both places are updated. Please move the runtime list into a shared exported constant and consume it here as the single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/worker/http/routes/CorpusRoutes.ts` at line 15, The route currently defines a local ALLOWED_CORPUS_TYPES Set that duplicates the union from the knowledge types module; remove this local definition in CorpusRoutes.ts and import a single exported constant from the shared types module (e.g., export a CORPUS_TYPES or ALLOWED_CORPUS_TYPES array/Set from the knowledge types file where the union is declared), then use that imported constant in place of the local Set (convert to a Set if necessary when doing membership checks) so the route always reads the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/worker/http/routes/corpus-routes-coercion.test.ts`:
- Around line 136-160: Add a test that exercises the new allowlist branch for
types by creating a request with types: ['typo'] (e.g., name: 'bad-types',
types: ['typo']), invoking handler(req, res), awaiting flushPromises(), and
asserting statusSpy was called with 400 and mockBuild was not called; this
targets the CorpusRoutes types allowlist branch and uses the existing helpers
createMockReqRes, handler, flushPromises, statusSpy and mockBuild to locate and
verify the behavior.
---
Nitpick comments:
In `@src/services/worker/http/routes/CorpusRoutes.ts`:
- Line 15: The route currently defines a local ALLOWED_CORPUS_TYPES Set that
duplicates the union from the knowledge types module; remove this local
definition in CorpusRoutes.ts and import a single exported constant from the
shared types module (e.g., export a CORPUS_TYPES or ALLOWED_CORPUS_TYPES
array/Set from the knowledge types file where the union is declared), then use
that imported constant in place of the local Set (convert to a Set if necessary
when doing membership checks) so the route always reads the single source of
truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5eb64930-32e2-4e27-8024-d8cb52726c45
📒 Files selected for processing (2)
src/services/worker/http/routes/CorpusRoutes.tstests/worker/http/routes/corpus-routes-coercion.test.ts
|
Follow-up update: I addressed the CodeRabbit regression-test suggestion in the latest push (730f81e). The new test covers an unsupported corpus type value ( |
3 participants
Summary
types,concepts, andfilesinPOST /api/corpuswhen clients send JSON-encoded or comma-separated stringslimitvalues to positive integers before passing filters intoCorpusBuilderWhy
CorpusBuilderassumes corpus filter arrays and calls.join(',')ontypes,concepts, andfiles. If an MCP/HTTP client sends string-encoded arrays, the route currently passes those strings through and the builder can throw at runtime.DataRoutesalready handles the same MCP-client coercion shape for batch endpoints, so this keeps the boundary handling consistent.Tests
bun test tests/worker/http/routes/corpus-routes-coercion.test.tsbun test tests/worker/http/routes/data-routes-coercion.test.ts tests/worker/http/routes/corpus-routes-coercion.test.tstsc --noEmitwas not runnable in this clean worktree becausenode_modules/@types/nodeis not installed locally (Cannot find type definition file for 'node').