-
Notifications
You must be signed in to change notification settings - Fork 6.9k
fix:agent eval and doc file #6158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
TestGru AssignmentSummary
Tip You can |
|
xxyyh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
xxyyh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Preview sandbox Image: |
Preview mcp_server Image: |
Preview fastgpt Image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses three issues: preventing HTML-to-Markdown conversion failures for large documents, fixing admin deletion permissions for user-created evaluation tasks, and resolving UI display issues after deletion.
Key Changes:
- Introduces
MAX_HTML_TRANSFORM_CHARSenvironment variable (default 1MB) to control HTML-to-MD conversion size limits - Modifies evaluation authorization to scope queries by
teamIdand enables cleanup of evaluations when their parent app is deleted - Updates tests to align with the new 1MB threshold
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/app/.env.template | Adds environment variable template for HTML transform character limit |
| deploy/templates/docker-compose.prod.yml | Configures MAX_HTML_TRANSFORM_CHARS=1000000 for production deployment |
| packages/service/type/env.d.ts | Adds TypeScript type definition for MAX_HTML_TRANSFORM_CHARS |
| packages/service/worker/utils.ts | Exposes MAX_HTML_TRANSFORM_CHARS through getSafeEnv utility |
| packages/service/worker/htmlStr2Md/utils.ts | Implements configurable size limit and preserves extracted images on overflow |
| packages/service/support/permission/evaluation/auth.ts | Adds teamId scoping to evaluation queries and special handling for deleted apps |
| test/cases/service/worker/htmlStr2Md.test.ts | Updates test thresholds from 100KB to 1MB and adds boundary test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
| } catch (error) { | ||
| // If app does not exist, allow operation (app was deleted, allow eval cleanup) | ||
| if (error !== AppErrEnum.unExist) { |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error comparison using !== is incorrect. Error enums are typically objects with properties like code, statusText, and message, not primitive strings. This comparison will always evaluate to true because the error object will never strictly equal the enum value.
The correct approach is to compare the error's statusText property or use a type-safe error checking function. This should be:
if (error?.statusText !== AppErrEnum.unExist) or access the error's identifier property that matches the enum value.
| if (error !== AppErrEnum.unExist) { | |
| if ((error as any)?.statusText !== AppErrEnum.unExist) { |
| if (processedHtml.length > MAX_HTML_SIZE) { | ||
| return { rawText: processedHtml, imageList: [] }; | ||
| return { rawText: processedHtml, imageList: images }; |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the HTML exceeds the size limit, the function now returns the original HTML with extracted base64 images. However, this creates an inconsistent behavior: the caller receives HTML in rawText (not Markdown) but also gets the extracted images. This could cause issues for consumers expecting either:
- HTML with intact base64 images (original behavior), or
- Markdown text with separate image references
Consider whether it's more appropriate to return the original HTML string without extracting images when the size limit is exceeded, maintaining consistency with the input format.
| SHOW_DISCOUNT_COUPON=false | ||
| # 自定义 config.json 路径 | ||
| CONFIG_JSON_PATH= | ||
| # HTML 转 Markdown 最大字符数(超过字符数不执行转化) |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "HTML 转 Markdown 最大字符数(超过字符数不执行转化)" but doesn't specify a default value or expected format. This makes it unclear for users what happens if this variable is not set. Consider adding the default value (1000000) to the comment for better clarity.
| # HTML 转 Markdown 最大字符数(超过字符数不执行转化) | |
| # HTML 转 Markdown 最大字符数(超过字符数不执行转化,默认 1000000) |
| try { | ||
| // App read per | ||
| if (per === ReadPermissionVal) { | ||
| await authAppByTmbId({ | ||
| tmbId, | ||
| appId: evaluation.appId, | ||
| per: ReadPermissionVal, | ||
| isRoot | ||
| }); | ||
|
|
||
| return { | ||
| teamId, | ||
| tmbId, | ||
| evaluation | ||
| }; | ||
| } | ||
|
|
||
| // Write per | ||
| await authAppByTmbId({ | ||
| tmbId, | ||
| appId: evaluation.appId, | ||
| per: ReadPermissionVal, | ||
| per: ManagePermissionVal, | ||
| isRoot | ||
| }); | ||
| return { | ||
| teamId, | ||
| tmbId, | ||
| evaluation | ||
| }; | ||
| } catch (error) { | ||
| // If app does not exist, allow operation (app was deleted, allow eval cleanup) | ||
| if (error !== AppErrEnum.unExist) { | ||
| throw error; | ||
| } | ||
| } |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a ReadPermissionVal is requested and the check passes (lines 46-58), the function returns early. However, if the check fails with an error other than AppErrEnum.unExist, that error is re-thrown (line 71). But if it's AppErrEnum.unExist, the function continues to line 75 and returns successfully - this means users could get read access to an evaluation even if the app doesn't exist and they're not the owner. This bypasses the intended access control for read operations.
The try-catch block should only apply to write permission checks (ManagePermissionVal), not read permission checks. Consider restructuring so that read permission failures are always propagated.
| const result = html2md(hugeHtml); | ||
|
|
||
| expect(result.rawText).toBe(hugeHtml); | ||
| expect(result.imageList).toHaveLength(0); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects imageList to have length 0 when HTML exceeds the size limit, but the implementation was changed to return the extracted images even when the size limit is exceeded (line 75 in utils.ts returns imageList: images). This test assertion is now incorrect and will fail.
| expect(result.imageList).toHaveLength(0); | |
| // 超大 HTML 被拒绝时,保留原始文本,但实现仍可能返回已提取的图片列表 | |
| expect(Array.isArray(result.imageList)).toBe(true); |
* fix: santinize the upload filename (labring#6159) * feat: add user ip filter for chat log table (labring#6162) * feat: add user ip filter for chat log table * chore: fix menu placement * feat: export all chunks in collection (labring#6163) * feat: export all chunks in collection * perf: export collection api * doc --------- Co-authored-by: archer <545436317@qq.com> * fix:agent eval and doc file (labring#6158) * agent eval * eval auth * html transofrm size * fix: test --------- Co-authored-by: xxyyh <2289112474@qq> Co-authored-by: archer <545436317@qq.com> * upgrade: MongoDB 5.0.18 to 5.0.32 (labring#6148) * upgrade: MongoDB 5.0.18 to 5.0.32 Update MongoDB version from 5.0.18 to 5.0.32 in all deployment configurations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> * doc --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Happy <yesreply@happy.engineering> Co-authored-by: archer <545436317@qq.com> * perf: vector db log;perf: s3 mock (labring#6165) * stop design doc * remove invalid doc * remove log * perf: s3 mock * perf: vector db log * update lock * feat: workflow presentation ui (labring#6156) * icon & gradient * line & header ui * basic presentation mode * ui & icon * node * fix * source color schema * delete code * fix build * fix * fix * fix icon * function position * fix: workflow fixview padding & context menu position (labring#6169) * fix: vector ts (labring#6166) * stop design doc * remove invalid doc * rename * fix: ts * perf: auto fit * perf: comment menu * remove invalid checker * save button colorschema * fix: icon * perf: color schema * fix: icon * perf: icon * perf: icon * perf: icon * perf: icon --------- Co-authored-by: Roy <whoeverimf5@gmail.com> Co-authored-by: archer <545436317@qq.com> Co-authored-by: YeYuheng <57035043+YYH211@users.noreply.github.com> Co-authored-by: xxyyh <2289112474@qq> Co-authored-by: Finley Ge <32237950+FinleyGe@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Happy <yesreply@happy.engineering> Co-authored-by: heheer <heheer@sealos.io>
Note
HTML → Markdown limits
MAX_HTML_TRANSFORM_CHARSenv (default 1,000,000) to control HTML→MD conversion size; wired throughdocker-compose,.env.template,env.d.ts, and workergetSafeEnv.htmlStr2Md: uses the new limit and, when exceeding it, returns original HTML while preserving extracted base64imageList; performance/defensive tests updated to 1MB thresholds.Evaluation auth
authEval: query now scoped byteamId; enforces app-level read/manage checks; onAppErrEnum.unExist(app deleted) allows operation to enable eval cleanup.Written by Cursor Bugbot for commit 757cc47. This will update automatically on new commits. Configure here.