(feat/fix) [RHDHBUGS- 3302] using Markitdown for AI notebooks - #4020
(feat/fix) [RHDHBUGS- 3302] using Markitdown for AI notebooks#4020JslYoon wants to merge 10 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Unexpected ChangesetsThe following changeset(s) reference packages that have not been changed in this PR:
Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets. Changed Packages
|
PR Summary by QodoAI Notebooks: convert uploads to Markdown with markitdown before vectorizing
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
JslYoon
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4020 +/- ##
==========================================
- Coverage 58.11% 58.01% -0.11%
==========================================
Files 2422 2411 -11
Lines 96484 96226 -258
Branches 26885 26770 -115
==========================================
- Hits 56075 55828 -247
- Misses 38914 40209 +1295
+ Partials 1495 189 -1306
*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:
|
28ce2d8 to
46460bd
Compare
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewReason: stale-head The review agent reviewed commit |
|
/fs-review |
325de76 to
559cb4f
Compare
|
/fs-review |
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>
| --- | ||
| '@red-hat-developer-hub/backstage-plugin-lightspeed-backend': minor | ||
| --- | ||
|
|
||
| bugfix - Notebooks routes 404 passthrough error resolved |
There was a problem hiding this comment.
Are we including this bug fix in this PR and I have missed it? Or is this leftover and needs to be removed to avoid another minor bump?
There was a problem hiding this comment.
Lets remove this changeset if it is unused/not included in this PR to keep a clean history
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@red-hat-developer-hub/backstage-plugin-intelligent-assistant-backend': minor | |||
There was a problem hiding this comment.
I think this should be a patch if it is a bugfix
There was a problem hiding this comment.
I would say this is more of an enhancement, so that's why I put it as update, because I'm switching out a component to improve vector stores
There was a problem hiding this comment.
I'm on the side of if it isn't client facing / just internal then it should be a 'patch' but I will leave it up to you
Cover plaintext passthrough, markitdown conversion, extension mismatch validation, and empty output error handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:54 PM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@red-hat-developer-hub/backstage-plugin-lightspeed-backend': minor | |||
There was a problem hiding this comment.
[high] changeset-target-error
Changeset targets '@red-hat-developer-hub/backstage-plugin-lightspeed-backend', which is not a package in this repository. The actual package is '@red-hat-developer-hub/backstage-plugin-intelligent-assistant-backend'. This changeset will either be silently ignored or cause a build error during release.
Suggested fix: Change the package name in the changeset to '@red-hat-developer-hub/backstage-plugin-intelligent-assistant-backend'.
|
|
||
| if (!req.file) { | ||
| handleError(logger, res, 'No file uploaded'); | ||
| return; |
There was a problem hiding this comment.
[medium] Input validation regression
The PR removes the explicit isValidFileSize(file.size) check that was in the removed parseFileContent(). While multer's limits.fileSize config enforces the same 20MB limit at the transport layer, the application-layer defense-in-depth validation is removed.
Suggested fix: If defense-in-depth is desired, re-add the explicit file size check before calling convertToMarkdown().
| */ | ||
| export async function convertToMarkdown( | ||
| buffer: Buffer, | ||
| originalName: string, |
There was a problem hiding this comment.
[medium] Denial of service via untrusted content parsing
markitdown-ts v0.0.10 is an early-stage package with heavyweight transitive dependencies (jsdom, mammoth, pdf-parse, xlsx) that process user-supplied file buffers. The convertBuffer call has no timeout or resource limits.
Suggested fix: Add a timeout wrapper around convertBuffer() (e.g., Promise.race with a 30-second timeout). Consider pinning the exact version.
| buffer: Buffer, | ||
| originalName: string, | ||
| fileType: string, | ||
| ): Promise<string> { |
There was a problem hiding this comment.
[medium] error handling idiom
Exceptions from markitdown.convertBuffer() propagate as-is. The removed fileParser.ts wrapped parsing errors in InputError (HTTP 400). If markitdown-ts throws a generic Error, it will surface as HTTP 500 instead of 400.
Suggested fix: Wrap the convertBuffer call in try/catch and re-throw as InputError.
| * Convert a document buffer to markdown using markitdown-ts. | ||
| * Plain-text formats (json, yaml, log) are passed through as-is. | ||
| */ | ||
| export async function convertToMarkdown( |
There was a problem hiding this comment.
[low] Input validation regression
JSON/YAML format validation removed. The old fileParser.ts validated JSON (JSON.parse) and YAML (yaml.load) before passing through. The new code treats these as plaintext with no validation.
| : ''; | ||
| if (nameExt && nameExt !== ext) { | ||
| throw new InputError( | ||
| `File extension "${nameExt}" does not match declared file type "${fileType}"`, |
There was a problem hiding this comment.
[low] Filename injection into error messages
User-controlled originalName interpolated into error messages without sanitization. Low risk since Backstage returns InputError as structured JSON.
| return; | ||
| } | ||
|
|
||
| const markdown = await convertToMarkdown( |
There was a problem hiding this comment.
[low] Prompt injection defense gap
sanitizeContentForRAG() exists but is not called on converted markdown before upload. Pre-existing gap, not a regression.
|
|
||
| import { MarkItDown } from 'markitdown-ts'; | ||
|
|
||
| const markitdown = new MarkItDown(); |
There was a problem hiding this comment.
[low] instantiation pattern
Module-level singleton instantiated at import time. Consider lazy initialization for consistency with codebase patterns.
2 participants
Summary
Replaces the custom per-format file parser (
fileParser.ts) with markitdown-ts for converting uploaded documents to markdown before vectorization in AI notebooks.Why
The previous implementation maintained separate parsing logic for each file type (txt, md, json, yaml, pdf) using
pdfjs-dist,js-yaml, and manual text extraction.markitdown-tshandles all of these with a single library, reducing maintenance surface and improving conversion quality — especially for PDFs and rich document formats.What changed
fileParser.ts(224 lines) and its 541-line test suite — custom per-format parsing logic (text, JSON, YAML, PDF)parseFileContentandstripHtmlTagsfromdocumentHelpers.ts— no longer needed since markitdown handles conversionmarkitdownClient.ts— thin wrapper aroundmarkitdown-tsthat:markitdown-tsfor markdown conversionmarkitdownClient.test.ts— 17 unit tests covering plaintext passthrough, markitdown conversion, extension mismatch errors, and empty output handling (100% coverage)documentService.tsandnotebooksRouters.tsto useconvertToMarkdowninstead of the oldparseFilepipelinemarkitdown-ts, removedpdfjs-distandjs-yamlImpact
✔️ Checklist
https://redhat.atlassian.net/browse/RHDHBUGS-3302