fix: verify downloaded files before completion#3040
Open
lhx077 wants to merge 1 commit into
Open
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
审阅者指南（在小型 PR 上折叠）审阅者指南为每个已下载文件增加下载后的完整性校验：在下载完成后立即重新检查文件，如果校验返回错误则使整个操作失败，从而在保留现有重试与状态处理逻辑的同时，拒绝受损或被篡改的制品。 文件级变更
提示与命令与 Sourcery 交互
自定义你的体验访问你的控制面板以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideAdds post-download integrity verification for each downloaded file by re-checking it immediately after download and failing the operation if verification returns an error, so that compromised or corrupted artifacts are rejected while preserving the existing retry and state-handling logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我在这里给出了一些整体性的反馈：
- 当完整性检查失败时，可以考虑在重新抛出异常之前删除部分/错误下载的文件，这样后续重试时就不会一直验证同一个有问题的构件。
- 在抛出的 IOException 中使用的硬编码中文错误信息，可能更适合通过你现有的本地化机制或常量来表达，以保持应用内错误信息的一致性。
- 你可能需要在 IOException 的错误信息中加入更多上下文信息（例如 file.LocalPath 或具有代表性的 URL），以便在日志中更容易诊断到底是哪个文件验证失败。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- When the integrity check fails, consider deleting the partially/incorrectly downloaded file before rethrowing so subsequent retries don’t keep validating the same bad artifact.
- The hard-coded Chinese error message in the thrown IOException may be better expressed via your existing localization mechanism or a constant, to keep error messaging consistent across the app.
- You might want to include more context (e.g., file.LocalPath or a representative URL) in the IOException message to make diagnosing which file failed verification easier in logs.帮我变得更有用！请对每条评论点 👍 或 👎，我会根据这些反馈改进为你提供的评审。
Original comment in English
Hey - I've left some high level feedback:
- When the integrity check fails, consider deleting the partially/incorrectly downloaded file before rethrowing so subsequent retries don’t keep validating the same bad artifact.
- The hard-coded Chinese error message in the thrown IOException may be better expressed via your existing localization mechanism or a constant, to keep error messaging consistent across the app.
- You might want to include more context (e.g., file.LocalPath or a representative URL) in the IOException message to make diagnosing which file failed verification easier in logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When the integrity check fails, consider deleting the partially/incorrectly downloaded file before rethrowing so subsequent retries don’t keep validating the same bad artifact.
- The hard-coded Chinese error message in the thrown IOException may be better expressed via your existing localization mechanism or a constant, to keep error messaging consistent across the app.
- You might want to include more context (e.g., file.LocalPath or a representative URL) in the IOException message to make diagnosing which file failed verification easier in logs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 906d16a9e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+140
to
+142
| var checkError = file.Check?.Check(file.LocalPath); | ||
| if (checkError is not null) | ||
| throw new IOException($"下载文件校验失败：{checkError}"); |
There was a problem hiding this comment.
When the first URL returns a complete but invalid file, this check throws only after FileDownloader.Download(...) has already returned; DownloadCoreAsync stops at the first transport-successful URL (FileDownloader.cs lines 51-57), so the exception is caught by the outer retry loop and the same first URL is retried up to four times instead of falling through to later mirrors. For multi-source downloads where mirror 1 is corrupt or malicious but mirror 2 is valid, this makes the whole download fail despite an available good source; the integrity check needs to participate in the per-URL failure path or otherwise advance to the next URL.
Useful? React with 👍 / 👎.
whitecat346
approved these changes
Jun 10, 2026
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
ModBase.FileChecker(expected size/hash) for sensitive artifacts such as Minecraft libraries and assets, so downloads must be re-checked after being written.Description
file.Check?.Check(file.LocalPath)immediately afterFileDownloader.Download(...)inLoaderDownload.ProcessFileAsyncofPlain Craft Launcher 2/Modules/Network/Loaders/LoaderDownload.cs.IOExceptionwith the checker message, which routes the error through the existing retry/failure handling path.Finishedafter verification.Testing
file.Check?.Check(file.LocalPath)is present before theFinishedstate, and the assertion passed.git diff --checkto ensure no whitespace errors and it passed.dotnet build 'Plain Craft Launcher 2/Plain Craft Launcher 2.csproj' -c Debug --no-restorebut it could not be executed in this environment becausedotnetis not installed.Codex Task
Summary by Sourcery
Bug Fixes:
Original summary in English
Summary by Sourcery
Bug Fixes: