Skip to content

ci: add PR review workflow with high-risk detection#126

Open
lml2468 wants to merge 1 commit intomainfrom
ci/add-pr-review-workflow
Open

ci: add PR review workflow with high-risk detection#126
lml2468 wants to merge 1 commit intomainfrom
ci/add-pr-review-workflow

Conversation

@lml2468
Copy link
Copy Markdown
Collaborator

@lml2468 lml2468 commented Mar 26, 2026

What

为 dmwork-adapters 添加 PR review 自动化。

Why

唯一有完整 CI 的项目,但缺少 PR review 自动化。其他项目都有高风险检测,adapters 也应该有。

How

  • 高风险模式检测(webhook、auth、token、secrets、plugin 配置变更)
  • 触发后自动打 high-risk label + 要求人工 review
  • 所有 PR 自动添加 review checklist

- Auto-detect high-risk patterns (webhook, auth, token, secrets, plugin config)
- Add high-risk label and require manual review
- PR review checklist for all PRs
Copy link
Copy Markdown
Collaborator

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea to add automated high-risk detection! A few suggestions to make it more robust:

  1. Duplicate comment issue: Both steps trigger on synchronize (new pushes), which means every force-push or new commit will create duplicate comments. Consider checking if a similar comment already exists before posting, or switch to using a single comment that gets updated via issues.updateComment.

  2. Regex false positives: Patterns like /auth/i and /token/i are quite broad — they would match file names like author.ts or tokenizer.go. Consider word-boundary patterns (/\bauth\b/i) or limiting the scan to specific directories/extensions.

  3. Review checklist scope: The checklist is posted on every PR unconditionally. For doc-only or CI-only changes, the webhook/token checklist items are not relevant. Consider making it conditional based on changed file types, or merge it into the high-risk step.

Overall good direction for the project — just needs dedup logic to avoid noisy PRs. 👍


on:
pull_request:
types: [opened, synchronize]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The synchronize event fires on every push to the PR branch. Both steps below will create new comments each time, which can spam the PR. Consider checking for an existing bot comment and updating it instead of creating a new one.

/token/i,
/secret/i,
/\.env/,
/openclaw\.plugin\.json/,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These patterns are quite broad. For example, /auth/i will match filenames containing "author", "authorization-docs", etc. Consider using word-boundary anchors like /\bauth\b/i to reduce false positives.

labels: ['high-risk'],
});
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checklist is posted unconditionally on every PR event. Two issues: (1) it creates duplicate comments on each push, and (2) the checklist items are not always relevant (e.g., doc-only PRs). Consider deduplicating and/or making this conditional based on changed file types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants