Testcibom#2810
Closed
Chiloven945 wants to merge 2 commits into
Closed
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
pcl-ce-automation
Bot
removed
🛠️ 等待审查
审查者指南添加一个 GitHub Actions 工作流程，用于检查拉取请求中所有变更文件是否包含 Unicode 字节顺序标记（BOM）。一旦检测到 BOM，该流程会在 PR 上发表评论提供详情，并将工作流程标记为失败。 BOM 检测与 PR 评论更新的时序图sequenceDiagram
actor Dev
participant PR as PullRequest
participant GH as GitHub
participant WF as Workflow_check-bom
participant JS as BOM_check_script
participant API as GitHub_REST_API
Dev->>GH: Open or update PR
GH-->>WF: Trigger pull_request_target event
WF->>WF: Run job check-bom
WF->>JS: Execute github-script step
JS->>API: pulls.listFiles(owner, repo, pull_number)
API-->>JS: Changed files list
loop For each non-removed file
JS->>JS: Read first 4 bytes from file
JS->>JS: detectBom(buffer)
JS->>JS: Accumulate files with BOM
end
alt No files with BOM
JS-->>WF: Log "No BOM found"
WF-->>GH: Job succeeds
else Files with BOM found
JS->>API: issues.listComments(owner, repo, issue_number)
API-->>JS: Existing comments
alt Existing marker comment found
JS->>API: issues.updateComment(owner, repo, comment_id, body)
else No marker comment
JS->>API: issues.createComment(owner, repo, issue_number, body)
end
JS-->>WF: core.setFailed("BOM found...")
WF-->>GH: Job fails
GH-->>PR: Updated bot comment with BOM details
end
文件级变更
提示与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 以：
获取帮助Original review guide in EnglishReviewer's GuideAdds a GitHub Actions workflow that checks all changed files in pull requests for the presence of Unicode byte order marks (BOMs) and comments on the PR with details when any are detected, failing the workflow in that case. Sequence diagram for BOM detection and PR comment updatesequenceDiagram
actor Dev
participant PR as PullRequest
participant GH as GitHub
participant WF as Workflow_check-bom
participant JS as BOM_check_script
participant API as GitHub_REST_API
Dev->>GH: Open or update PR
GH-->>WF: Trigger pull_request_target event
WF->>WF: Run job check-bom
WF->>JS: Execute github-script step
JS->>API: pulls.listFiles(owner, repo, pull_number)
API-->>JS: Changed files list
loop For each non-removed file
JS->>JS: Read first 4 bytes from file
JS->>JS: detectBom(buffer)
JS->>JS: Accumulate files with BOM
end
alt No files with BOM
JS-->>WF: Log "No BOM found"
WF-->>GH: Job succeeds
else Files with BOM found
JS->>API: issues.listComments(owner, repo, issue_number)
API-->>JS: Existing comments
alt Existing marker comment found
JS->>API: issues.updateComment(owner, repo, comment_id, body)
else No marker comment
JS->>API: issues.createComment(owner, repo, issue_number, body)
end
JS-->>WF: core.setFailed("BOM found...")
WF-->>GH: Job fails
GH-->>PR: Updated bot comment with BOM details
end
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 - 我发现了两个问题，并给出了一些整体反馈：
- 用于评论的
body模板字符串在脚本块中有缩进，这会在发布的评论中引入前导空格/换行。建议将模板字符串内容左对齐，或者使用trim()来生成更干净的评论格式。 - 在检测现有机器人评论时，你通过
comment.user.type === 'Bot'进行过滤，这可能会遗漏来自 GitHub Apps 的评论；仅通过标记检查（或同时将user.login与 workflow 机器人账号比对）会让更新逻辑更健壮。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- 用于评论的 `body` 模板字符串在脚本块中有缩进，这会在发布的评论中引入前导空格/换行。建议将模板字符串内容左对齐，或者使用 `trim()` 来生成更干净的评论格式。
- 在检测现有机器人评论时，你通过 `comment.user.type === 'Bot'` 进行过滤，这可能会遗漏来自 GitHub Apps 的评论；仅通过标记检查（或同时将 `user.login` 与 workflow 机器人账号比对）会让更新逻辑更健壮。
## Individual Comments
### Comment 1
<location path=".github/workflows/bom-check.yml" line_range="4-5" />
<code_context>
+name: Check BOM in PR files
+
+on:
+ pull_request_target:
+ types: [opened, synchronize, reopened, ready_for_review]
+
+permissions:
</code_context>
<issue_to_address>
**🚨 issue (security):** Reconsider using `pull_request_target` given the permissions and untrusted PR context.
`pull_request_target` runs with a write-capable GITHUB_TOKEN on the base repo while using potentially untrusted fork code. Since this job posts comments and could grow over time, prefer `pull_request` unless you truly need `pull_request_target` (e.g., for secrets). If you keep it, please confirm that this event type is strictly required for your use case.
</issue_to_address>
### Comment 2
<location path=".github/workflows/bom-check.yml" line_range="77-88" />
<code_context>
+ continue;
+ }
+
+ const fd = fs.openSync(absolutePath, 'r');
+ const buffer = Buffer.alloc(4);
+ const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
+ fs.closeSync(fd);
+
+ const bom = detectBom(buffer.subarray(0, bytesRead));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wrap file descriptor operations to avoid leaks if an exception occurs during read.
If `fs.readSync` throws (e.g., due to a transient filesystem error), the file descriptor is never closed and will leak. Please wrap the open/read/close sequence in a `try`/`finally` so `fs.closeSync` always runs:
```js
let fd;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
const bom = detectBom(buffer.subarray(0, bytesRead));
// ...
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
```
```suggestion
const absolutePath = path.join(workspace, file.filename);
if (!fs.existsSync(absolutePath)) {
continue;
}
let fd;
let bom;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
bom = detectBom(buffer.subarray(0, bytesRead));
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进之后的 review。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The
bodytemplate literal for the comment is indented inside the script block, which will introduce leading spaces/newlines in the posted comment; consider left-aligning the template literal content or usingtrim()to produce cleaner comment formatting. - When detecting an existing bot comment you filter by
comment.user.type === 'Bot', which may miss comments from GitHub Apps; checking for the marker alone (or also checkinguser.loginagainst the workflow bot) would make the update logic more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `body` template literal for the comment is indented inside the script block, which will introduce leading spaces/newlines in the posted comment; consider left-aligning the template literal content or using `trim()` to produce cleaner comment formatting.
- When detecting an existing bot comment you filter by `comment.user.type === 'Bot'`, which may miss comments from GitHub Apps; checking for the marker alone (or also checking `user.login` against the workflow bot) would make the update logic more robust.
## Individual Comments
### Comment 1
<location path=".github/workflows/bom-check.yml" line_range="4-5" />
<code_context>
+name: Check BOM in PR files
+
+on:
+ pull_request_target:
+ types: [opened, synchronize, reopened, ready_for_review]
+
+permissions:
</code_context>
<issue_to_address>
**🚨 issue (security):** Reconsider using `pull_request_target` given the permissions and untrusted PR context.
`pull_request_target` runs with a write-capable GITHUB_TOKEN on the base repo while using potentially untrusted fork code. Since this job posts comments and could grow over time, prefer `pull_request` unless you truly need `pull_request_target` (e.g., for secrets). If you keep it, please confirm that this event type is strictly required for your use case.
</issue_to_address>
### Comment 2
<location path=".github/workflows/bom-check.yml" line_range="77-88" />
<code_context>
+ continue;
+ }
+
+ const fd = fs.openSync(absolutePath, 'r');
+ const buffer = Buffer.alloc(4);
+ const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
+ fs.closeSync(fd);
+
+ const bom = detectBom(buffer.subarray(0, bytesRead));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wrap file descriptor operations to avoid leaks if an exception occurs during read.
If `fs.readSync` throws (e.g., due to a transient filesystem error), the file descriptor is never closed and will leak. Please wrap the open/read/close sequence in a `try`/`finally` so `fs.closeSync` always runs:
```js
let fd;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
const bom = detectBom(buffer.subarray(0, bytesRead));
// ...
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
```
```suggestion
const absolutePath = path.join(workspace, file.filename);
if (!fs.existsSync(absolutePath)) {
continue;
}
let fd;
let bom;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
bom = detectBom(buffer.subarray(0, bytesRead));
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+4
to
+5
| pull_request_target: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
🚨 issue (security): 鉴于权限范围以及 PR 代码是不受信任的上下文，请重新考虑是否需要使用 pull_request_target。
pull_request_target 会在基础仓库上使用具备写权限的 GITHUB_TOKEN 来运行，但执行的却可能是来自 fork 仓库的不受信任代码。由于这个工作流会发布评论并且未来可能扩展，除非你确实需要 pull_request_target（例如为了访问机密），否则更推荐使用 pull_request。如果你选择保留它，请确认该事件类型对你的用例是严格必需的。
Original comment in English
🚨 issue (security): Reconsider using pull_request_target given the permissions and untrusted PR context.
pull_request_target runs with a write-capable GITHUB_TOKEN on the base repo while using potentially untrusted fork code. Since this job posts comments and could grow over time, prefer pull_request unless you truly need pull_request_target (e.g., for secrets). If you keep it, please confirm that this event type is strictly required for your use case.
Comment on lines
+77
to
+88
| const absolutePath = path.join(workspace, file.filename); | ||
|
|
||
| if (!fs.existsSync(absolutePath)) { | ||
| continue; | ||
| } | ||
|
|
||
| const fd = fs.openSync(absolutePath, 'r'); | ||
| const buffer = Buffer.alloc(4); | ||
| const bytesRead = fs.readSync(fd, buffer, 0, 4, 0); | ||
| fs.closeSync(fd); | ||
|
|
||
| const bom = detectBom(buffer.subarray(0, bytesRead)); |
There was a problem hiding this comment.
suggestion (bug_risk): 建议包装文件描述符相关操作，以避免在读取过程中发生异常时造成泄漏。
如果 fs.readSync 抛出异常（例如由于临时文件系统错误），文件描述符将不会被关闭并导致泄漏。请将 open/read/close 的调用序列包裹在 try/finally 中，以确保 fs.closeSync 始终会执行：
let fd;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
const bom = detectBom(buffer.subarray(0, bytesRead));
// ...
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
Suggested change
| const absolutePath = path.join(workspace, file.filename); | |
| if (!fs.existsSync(absolutePath)) { | |
| continue; | |
| } | |
| const fd = fs.openSync(absolutePath, 'r'); | |
| const buffer = Buffer.alloc(4); | |
| const bytesRead = fs.readSync(fd, buffer, 0, 4, 0); | |
| fs.closeSync(fd); | |
| const bom = detectBom(buffer.subarray(0, bytesRead)); | |
| const absolutePath = path.join(workspace, file.filename); | |
| if (!fs.existsSync(absolutePath)) { | |
| continue; | |
| } | |
| let fd; | |
| let bom; | |
| try { | |
| fd = fs.openSync(absolutePath, 'r'); | |
| const buffer = Buffer.alloc(4); | |
| const bytesRead = fs.readSync(fd, buffer, 0, 4, 0); | |
| bom = detectBom(buffer.subarray(0, bytesRead)); | |
| } finally { | |
| if (fd !== undefined) { | |
| fs.closeSync(fd); | |
| } | |
| } |
Original comment in English
suggestion (bug_risk): Wrap file descriptor operations to avoid leaks if an exception occurs during read.
If fs.readSync throws (e.g., due to a transient filesystem error), the file descriptor is never closed and will leak. Please wrap the open/read/close sequence in a try/finally so fs.closeSync always runs:
let fd;
try {
fd = fs.openSync(absolutePath, 'r');
const buffer = Buffer.alloc(4);
const bytesRead = fs.readSync(fd, buffer, 0, 4, 0);
const bom = detectBom(buffer.subarray(0, bytesRead));
// ...
} finally {
if (fd !== undefined) {
fs.closeSync(fd);
}
}
Suggested change
| const absolutePath = path.join(workspace, file.filename); | |
| if (!fs.existsSync(absolutePath)) { | |
| continue; | |
| } | |
| const fd = fs.openSync(absolutePath, 'r'); | |
| const buffer = Buffer.alloc(4); | |
| const bytesRead = fs.readSync(fd, buffer, 0, 4, 0); | |
| fs.closeSync(fd); | |
| const bom = detectBom(buffer.subarray(0, bytesRead)); | |
| const absolutePath = path.join(workspace, file.filename); | |
| if (!fs.existsSync(absolutePath)) { | |
| continue; | |
| } | |
| let fd; | |
| let bom; | |
| try { | |
| fd = fs.openSync(absolutePath, 'r'); | |
| const buffer = Buffer.alloc(4); | |
| const bytesRead = fs.readSync(fd, buffer, 0, 4, 0); | |
| bom = detectBom(buffer.subarray(0, bytesRead)); | |
| } finally { | |
| if (fd !== undefined) { | |
| fs.closeSync(fd); | |
| } | |
| } |
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
1 participant
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.
Summary by Sourcery
CI：
Original summary in English
Summary by Sourcery
CI: