feat(ci): introduce unified CI Required Gate workflow#6835
feat(ci): introduce unified CI Required Gate workflow#6835Jacobinwwey wants to merge 8 commits intoAstrBotDevs:masterfrom
Conversation
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the
dashboardjob,pnpm i --save-dev @types/markdown-itwill mutate dev dependencies during CI and diverge from the repo’s lockfile; it would be safer to add this dependency to the project up front (and commit it) instead of installing it dynamically in the workflow. - The
dashboardjob currently installspnpmglobally on every run (npm install pnpm -g); consider switching topnpm/action-setup(and enabling dependency caching viaactions/setup-node) to reduce build time and make the toolchain version explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `dashboard` job, `pnpm i --save-dev @types/markdown-it` will mutate dev dependencies during CI and diverge from the repo’s lockfile; it would be safer to add this dependency to the project up front (and commit it) instead of installing it dynamically in the workflow.
- The `dashboard` job currently installs `pnpm` globally on every run (`npm install pnpm -g`); consider switching to `pnpm/action-setup` (and enabling dependency caching via `actions/setup-node`) to reduce build time and make the toolchain version explicit.
## Individual Comments
### Comment 1
<location path=".github/workflows/ci-required-gate.yml" line_range="180-27" />
<code_context>
+ - name: Checkout
+ uses: actions/checkout@v6
+
+ - name: Setup Node.js
+ uses: actions/setup-node@v6
+ with:
+ node-version: '24.13.0'
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The specified Node.js version `24.13.0` is unlikely to be available on GitHub runners and may break the workflow.
This value will likely fail because GitHub-hosted runners only provide released Node versions (currently up to v22), so the job may never start. Please use a supported version (e.g. `22.x` or your target LTS) and/or a less specific semver (e.g. `22`) to avoid hard failures when an exact patch isn’t available.
</issue_to_address>
### Comment 2
<location path=".github/workflows/ci-required-gate.yml" line_range="189-190" />
<code_context>
+ run: |
+ cd dashboard
+ npm install pnpm -g
+ pnpm install
+ pnpm i --save-dev @types/markdown-it
+ pnpm run build
+
</code_context>
<issue_to_address>
**suggestion (performance):** Installing `@types/markdown-it` as a devDependency on every CI run is slow and can mutate the lockfile unexpectedly.
This command triggers an extra install and tries to update `package.json`/the lockfile on every CI run, which is both slow and risks drift between local and CI if those changes are ever committed. Instead, add `@types/markdown-it` to `devDependencies` in `package.json` and rely on the existing `pnpm install`/`pnpm install --frozen-lockfile` step so installs remain fast and deterministic.
Suggested implementation:
```
- name: Build dashboard
run: |
cd dashboard
npm install pnpm -g
pnpm install
pnpm run build
```
1. In `dashboard/package.json`, add `@types/markdown-it` under `devDependencies`, for example:
```json
"devDependencies": {
"@types/markdown-it": "^x.y.z",
...
}
```
Use the version that matches your local environment or the one already used in the project.
2. From the `dashboard` directory, run `pnpm install` locally to update `pnpm-lock.yaml` with the new devDependency and commit both `package.json` and `pnpm-lock.yaml`.
3. If your CI uses `pnpm install --frozen-lockfile` elsewhere, ensure this workflow is consistent with that convention.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
testjob currently installsuvbut never runsuv sync, which may cause tests to run without the expected dependencies; consider mirroring thelint/smokejobs’ dependency sync (or otherwise ensuring the test environment is fully prepared) before invokingrun_pytests_ci.sh. - The
dashboardjob usesactions/setup-nodewithnode-version: '22.x', while the PR description mentions Node 24.13.0; it would be good to align this with the version actually required/used by the dashboard to avoid subtle build or lockfile issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test` job currently installs `uv` but never runs `uv sync`, which may cause tests to run without the expected dependencies; consider mirroring the `lint`/`smoke` jobs’ dependency sync (or otherwise ensuring the test environment is fully prepared) before invoking `run_pytests_ci.sh`.
- The `dashboard` job uses `actions/setup-node` with `node-version: '22.x'`, while the PR description mentions Node 24.13.0; it would be good to align this with the version actually required/used by the dashboard to avoid subtle build or lockfile issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Python/uv setup is duplicated across
lint,test, andsmokejobs; consider extracting this into a reusable workflow or composite action to keep the CI definition DRY and easier to change later. - The
docs_onlydetection currently only whitelistsdocs/, rootREADME*.md, andchangelogs/; if other markdown or documentation-like paths (e.g..github/,docs-*/) should also benefit from the fast path, you may want to expand or centralize these patterns to avoid surprising full CI runs on docs-only changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Python/uv setup is duplicated across `lint`, `test`, and `smoke` jobs; consider extracting this into a reusable workflow or composite action to keep the CI definition DRY and easier to change later.
- The `docs_only` detection currently only whitelists `docs/`, root `README*.md`, and `changelogs/`; if other markdown or documentation-like paths (e.g. `.github/`, `docs-*/`) should also benefit from the fast path, you may want to expand or centralize these patterns to avoid surprising full CI runs on docs-only changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Python setup (checkout, setup-python, pip install uv, and uv sync) is duplicated across
lint,test, andsmokejobs; consider factoring this into a composite action or reusable workflow to reduce maintenance overhead and keep the steps in sync. - The dashboard job pins Node to
24.13.0, which is quite specific and may not exist or may go stale; consider using a major/minor constraint like24(or an LTS alias) plus the pnpm lockfile to keep builds stable while avoiding a brittle patch-level pin.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Python setup (checkout, setup-python, pip install uv, and uv sync) is duplicated across `lint`, `test`, and `smoke` jobs; consider factoring this into a composite action or reusable workflow to reduce maintenance overhead and keep the steps in sync.
- The dashboard job pins Node to `24.13.0`, which is quite specific and may not exist or may go stale; consider using a major/minor constraint like `24` (or an LTS alias) plus the pnpm lockfile to keep builds stable while avoiding a brittle patch-level pin.
## Individual Comments
### Comment 1
<location path=".github/workflows/ci-required-gate.yml" line_range="87-90" />
<code_context>
+ with:
+ python-version: '3.12'
+
+ - name: Install uv
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install uv
+
+ - name: Sync dependencies
</code_context>
<issue_to_address>
**suggestion:** The Python setup and `uv` installation are duplicated across multiple jobs; consider consolidating into a reusable workflow or composite action.
The `lint`, `test`, and `smoke` jobs all repeat the same `actions/setup-python` and `python -m pip install uv` steps, which increases maintenance overhead and risk of configuration drift. Consider moving these steps into a composite action (e.g., under `.github/actions`) or a reusable workflow and invoking that from each job to keep configuration consistent and easier to update.
</issue_to_address>Hi @Jacobinwwey! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Python environment setup (installing uv and syncing deps) is duplicated across
lint,test, andsmoke; consider extracting this into a reusable workflow/composite action or using a shared cache to avoid repeated installs and speed up CI. - The
dashboardjob pins Node.js to version24, which is not an LTS release; it may be safer to align this with an LTS version (e.g., 20/22) or whatever version other existing workflows use to avoid subtle version skew.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Python environment setup (installing uv and syncing deps) is duplicated across `lint`, `test`, and `smoke`; consider extracting this into a reusable workflow/composite action or using a shared cache to avoid repeated installs and speed up CI.
- The `dashboard` job pins Node.js to version `24`, which is not an LTS release; it may be safer to align this with an LTS version (e.g., 20/22) or whatever version other existing workflows use to avoid subtle version skew.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the composite
setup-python-uvaction, uv is installed unpinned viapython -m pip install uv; consider pinning a specific uv version (or using a hash-locked installer) to avoid future CI instability due to upstream uv changes. - The
gatejob treats any non-failure/non-cancelled result as acceptable, which meansskippedresults (e.g., docs-only PRs or non-dashboard changes) implicitly pass; if this is intentional, you may want to explicitly surfaceskippedas an expected state in the summary or narrowneedsto only jobs that can be required in each path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the composite `setup-python-uv` action, uv is installed unpinned via `python -m pip install uv`; consider pinning a specific uv version (or using a hash-locked installer) to avoid future CI instability due to upstream uv changes.
- The `gate` job treats any non-failure/non-cancelled result as acceptable, which means `skipped` results (e.g., docs-only PRs or non-dashboard changes) implicitly pass; if this is intentional, you may want to explicitly surface `skipped` as an expected state in the summary or narrow `needs` to only jobs that can be required in each path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
changesjob, whenchanged_filesends up empty (e.g., force-push adjusting commit metadata or odd diff situations),docs_onlystaystrueand will short-circuit lint/tests; consider explicitly treating an empty file list asdocs_only=falseto avoid silently skipping the main checks in ambiguous cases. - The composite
setup-python-uvaction installsuvviapipon every run; you could speed things up by switching to an officialsetup-uvaction or adding caching around the uv installation so it isn't reinstalled for each job. - The dashboard build uses Node.js
24, which is not an LTS release; it may be more robust to align this with the dashboard project's declared supported Node version (often an LTS like 20/22) to reduce the risk of subtle version-specific issues in CI.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `changes` job, when `changed_files` ends up empty (e.g., force-push adjusting commit metadata or odd diff situations), `docs_only` stays `true` and will short-circuit lint/tests; consider explicitly treating an empty file list as `docs_only=false` to avoid silently skipping the main checks in ambiguous cases.
- The composite `setup-python-uv` action installs `uv` via `pip` on every run; you could speed things up by switching to an official `setup-uv` action or adding caching around the uv installation so it isn't reinstalled for each job.
- The dashboard build uses Node.js `24`, which is not an LTS release; it may be more robust to align this with the dashboard project's declared supported Node version (often an LTS like 20/22) to reduce the risk of subtle version-specific issues in CI.
## Individual Comments
### Comment 1
<location path=".github/workflows/ci-required-gate.yml" line_range="132-136" />
<code_context>
+ sync-deps: 'true'
+ sync-args: '--group dev'
+
+ - name: Startup smoke test
+ shell: bash
+ run: |
+ set -euo pipefail
+ uv run main.py &
+ app_pid=$!
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Smoke test doesn't fail fast when the app exits early
If `uv run main.py` exits early with a non-zero status, the background process will die but the loop will still poll `curl` for up to 60s and only fail with a timeout, hiding the real error and slowing feedback. Inside the loop, also check whether `app_pid` is still running (e.g., via `ps`/`wait -n`) and fail immediately if it has exited.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
.github/actions/setup-python-uv/action.yml, consider quoting theuv syncarguments (e.g.,uv sync ${{ inputs.sync-args }}→uv sync $SYNC_ARGSwith proper quoting) to avoid unexpected word splitting or globbing when multiple or special-character arguments are passed viasync-args. - The
changesjob currently treats any non-docs path (including things like.github/**or misc config files) as non-docs-only and therefore runs the full CI; if this is more conservative than desired, you could explicitly whitelist additional documentation/config paths to keep the fast path effective for those changes as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.github/actions/setup-python-uv/action.yml`, consider quoting the `uv sync` arguments (e.g., `uv sync ${{ inputs.sync-args }}` → `uv sync $SYNC_ARGS` with proper quoting) to avoid unexpected word splitting or globbing when multiple or special-character arguments are passed via `sync-args`.
- The `changes` job currently treats any non-docs path (including things like `.github/**` or misc config files) as non-docs-only and therefore runs the full CI; if this is more conservative than desired, you could explicitly whitelist additional documentation/config paths to keep the fast path effective for those changes as well.
## Individual Comments
### Comment 1
<location path=".github/workflows/ci-required-gate.yml" line_range="25-26" />
<code_context>
+ docs_only: ${{ steps.detect.outputs.docs_only }}
+ dashboard_changed: ${{ steps.detect.outputs.dashboard_changed }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v6
+ with:
+ fetch-depth: 0
</code_context>
<issue_to_address>
**issue (bug_risk):** Using @v6 tags for core GitHub actions will currently fail because those versions do not exist.
This workflow uses `actions/checkout@v6`, `actions/setup-python@v6`, and `actions/setup-node@v6`, but the latest published major versions are currently `checkout@v4`, `setup-python@v5`, and `setup-node@v4`. Please update these to existing versions (or pin to specific SHAs) so the jobs don’t fail at the `uses:` resolution step.
</issue_to_address>Hi @Jacobinwwey! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.| - name: Checkout | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
issue (bug_risk): Using @v6 tags for core GitHub actions will currently fail because those versions do not exist.
This workflow uses actions/checkout@v6, actions/setup-python@v6, and actions/setup-node@v6, but the latest published major versions are currently checkout@v4, setup-python@v5, and setup-node@v4. Please update these to existing versions (or pin to specific SHAs) so the jobs don’t fail at the uses: resolution step.
There was a problem hiding this comment.
- 未把 actions/checkout|setup-python|setup-node 从 @v6 改回旧版本。
原因:当前官方仓库已存在 v6 标签(可解析),该建议前提不成立,改回旧版本无必要。 - 未扩大 docs_only 白名单(如 .github/**)。
原因:当前“保守触发完整 CI”的策略更安全,避免配置类变更被误判为 docs-only。 - 未把 dashboard 的 Node 从 24 改到 LTS。
原因:当前仓库 dashboard/release 现有主链路使用 Node 24 系,保持一致性优先。
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The workflow references several GitHub Actions with non-existent major versions (e.g.,
actions/checkout@v6,actions/setup-python@v6,actions/setup-node@v6), which will fail at runtime—these should be updated to the latest supported majors (currently@v4for checkout,@v5for setup-python,@v4for setup-node, etc.).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow references several GitHub Actions with non-existent major versions (e.g., `actions/checkout@v6`, `actions/setup-python@v6`, `actions/setup-node@v6`), which will fail at runtime—these should be updated to the latest supported majors (currently `@v4` for checkout, `@v5` for setup-python, `@v4` for setup-node, etc.).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
未把 actions/checkout|setup-python|setup-node 从 @v6 改回旧版本。 |
gh api repos/actions/checkout/tags --jq '.[0:12][] | .name' 结果都包含 v6 与对应 v6.x.y。
gh api repos/actions/checkout/git/ref/tags/v6 返回均为 object.type=commit.
git ls-remote --refs --tags https://github.com/actions/checkout.git 'refs/tags/v6*' 可直接看到 refs/tags/v6 及 refs/tags/v6.x.y。
curl -I https://codeload.github.com/actions/checkout/zip/refs/tags/v6 HTTP 都是 200。
curl https://raw.githubusercontent.com/actions/checkout/v6/action.yml 都返回 200 且内容有效。 |
|
[Nearly Passed the review by Sourcery] |
Motivation / 修改目的
当前 AstrBot 的 CI 检查分散在多个 workflow 中(format / unit tests / smoke / dashboard 等),但缺少一个统一、可直接被分支保护规则要求的“单一门禁信号”。
本 PR 的目标是引入
CI Required Gate:Overall Design / 整体思路
引入新的 workflow:
.github/workflows/ci-required-gate.yml核心逻辑:
changesjob)。gatejob 聚合所有上游结果,输出单一门禁状态CI Required Gate。该设计与“分散检查 + 人工拼接判断”相比,降低 reviewer 决策成本,并且更适合后续分支规则直接配置 Required Check。
Modifications / 改动项详解
1) Workflow 触发策略
设置说明:
pull_request覆盖master与dev,解决不同目标分支下检查覆盖不一致的问题。push master让主分支持续接受同构验证(便于基线监控)。workflow_dispatch便于手动复验。预期实践:
pull_request结果。push master用于持续观测主分支健康度。可行性:
2) 并发控制
设置说明:
预期实践:
可行性:
3)
changes变更范围探测changesjob 通过base..headdiff 计算两个输出:docs_onlydashboard_changed并包含边界处理:
github.event.before为空或全 0(首推/特殊场景)时回退到HEAD^;预期实践:
可行性:
4)
lint质量门执行:
uv sync --group devuv run ruff format --check .uv run ruff check .触发条件:
docs_only != true预期实践:
可行性:
5)
test单元测试门执行:
bash ./scripts/run_pytests_ci.sh ./tests触发条件:
docs_only != true预期实践:
可行性:
6)
smoke启动可用性门执行:
uv run main.py启动应用;http://localhost:6185最长 60 秒;trap保证进程清理。触发条件:
docs_only != true预期实践:
可行性:
7)
dashboard条件构建门执行:
24.13.0pnpm installpnpm run build触发条件:
dashboard_changed == true预期实践:
可行性:
8)
gate聚合门(核心)判定规则:
failure/cancelled->gate失败;预期实践:
CI Required Gate。可行性:
Scope Boundary / 变更边界
本 PR 只新增:
.github/workflows/ci-required-gate.yml未修改任何业务代码、测试代码、依赖文件或其它 workflow。
Validation / 验证
本地验证:
python3+pyyaml解析 workflow 文件,确认 YAML 结构合法。结果:
yaml_okPractical Rollout Plan / 建议落地方式
CI Required Gate设为 required check。Summary by Sourcery
Introduce a unified "CI Required Gate" GitHub Actions workflow that aggregates key quality checks into a single required status for PRs and master pushes.
CI:
CI Required Gateworkflow that orchestrates change detection, linting, tests, smoke checks, and conditional dashboard builds into a single gate job.setup-python-uvGitHub Action to standardize Python and uv setup and optional dependency syncing across CI jobs.