Skip to content

Conversation

@wangl-cc
Copy link
Member

@wangl-cc wangl-cc commented Jan 4, 2026

Summary by Sourcery

调整 GitHub Actions 配置中的 CI 工作流触发条件,并修复条件测试命令

CI:

  • 通过移除对内部复合 action 目录的触发,收紧 CI 工作流的路径过滤规则
  • .github/actions 中的 setup 步骤测试命令现在使用正确的 GitHub Actions 表达式语法,有条件地传递 --with-core 参数
Original summary in English

Summary by Sourcery

Adjust CI workflow triggers and fix conditional test command in the GitHub Actions configuration

CI:

  • Narrow CI workflow path filters by removing triggers on internal composite action directories
  • .github/actions setup step test command now conditionally passes the '--with-core' flag using proper GitHub Actions expression syntax

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - 我发现了 1 个问题,并且有一些总体性的反馈:

  • 条件性的 --with-core 标志如果移到单独的步骤级 if: 条件中(例如拆成两个 Test 步骤,分别带和不带该 flag),会更易读也更不脆弱,而不是把 GitHub 表达式嵌入到 shell 命令里。
  • 请再次确认从 on: push/pull_request.paths 中移除 .github/actions/setup/**.github/actions/install-core/** 路径过滤是否是对所有分支都刻意为之,因为这样会导致当只修改这些复合 action 时,CI 将不会运行。
给 AI Agent 的提示
Please address the comments from this code review:

## Overall Comments
- 条件性的 `--with-core` 标志如果移到单独的步骤级 `if:` 条件中(例如拆成两个 `Test`步骤,分别带和不带该 flag),会更易读也更不脆弱,而不是把 GitHub 表达式嵌入到 shell 命令里。
- 请再次确认从 `on: push/pull_request.paths` 中移除 `.github/actions/setup/**``.github/actions/install-core/**` 路径过滤是否是对所有分支都刻意为之,因为这样会导致当只修改这些复合 action 时,CI 将不会运行。

## Individual Comments

### Comment 1
<location> `.github/workflows/ci.yml:117` </location>
<code_context>
         run: |
           cargo run -pxtask test  \
-            ${{ matrix.runner != 'windows-11-arm' && --with-core }} \
+            ${{ matrix.runner != 'windows-11-arm' && '--with-core' }} \
             -- --include-ignored

</code_context>

<issue_to_address>
**issue (bug_risk):** 条件表达式可能会插入 `true`/`false` 而不是空字符串,从而影响 cargo 命令。

在 GitHub Actions 中,像 `matrix.runner != 'windows-11-arm' && '--with-core'` 这样的 `&&` 链在条件不满足时会得到 `false`(布尔值),在 shell 中会被渲染为字符串 `false`,从而导致最终命令变成 `cargo run ... false -- --include-ignored`。如果你希望在这种情况下不传递任何参数,可以使用类似 `${{ matrix.runner != 'windows-11-arm' && '--with-core' || '' }}` 的三元风格模式,或者调整步骤逻辑,使该 flag 只在条件满足时才被添加。
</issue_to_address>

Sourcery 对开源项目是免费的——如果你觉得我们的审查有帮助,欢迎分享 ✨
帮我变得更有用!请对每条评论点 👍 或 👎,我会根据这些反馈改进后续的审查。
Original comment in English

Hey - I've found 1 issue, and left some high level feedback:

  • The conditional --with-core flag would be easier to read and less brittle if moved into a separate step-level if: condition (e.g. two Test steps with and without the flag) rather than embedding a GitHub expression inside the shell command.
  • Double-check that removing the .github/actions/setup/** and .github/actions/install-core/** path filters from on: push/pull_request.paths is intentional for all branches, since this will stop CI from running when only those composite actions change.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The conditional `--with-core` flag would be easier to read and less brittle if moved into a separate step-level `if:` condition (e.g. two `Test` steps with and without the flag) rather than embedding a GitHub expression inside the shell command.
- Double-check that removing the `.github/actions/setup/**` and `.github/actions/install-core/**` path filters from `on: push/pull_request.paths` is intentional for all branches, since this will stop CI from running when only those composite actions change.

## Individual Comments

### Comment 1
<location> `.github/workflows/ci.yml:117` </location>
<code_context>
         run: |
           cargo run -pxtask test  \
-            ${{ matrix.runner != 'windows-11-arm' && --with-core }} \
+            ${{ matrix.runner != 'windows-11-arm' && '--with-core' }} \
             -- --include-ignored

</code_context>

<issue_to_address>
**issue (bug_risk):** Conditional expression may interpolate `true`/`false` instead of an empty string, affecting the cargo command.

In GitHub Actions, an `&&` chain like `matrix.runner != 'windows-11-arm' && '--with-core'` evaluates to `false` (a boolean) when the condition fails, which is rendered as the string `false` in the shell, yielding `cargo run ... false -- --include-ignored`. If you want no argument in that case, use a ternary-style pattern like `${{ matrix.runner != 'windows-11-arm' && '--with-core' || '' }}` or adjust the step so the flag is only added conditionally.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link

codecov bot commented Jan 4, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.06%. Comparing base (2daf7a1) to head (cc2889e).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
crates/maa-cli/src/config/mod.rs 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #468       +/-   ##
===========================================
- Coverage   69.25%   59.06%   -10.20%     
===========================================
  Files          56       56               
  Lines        5601     5560       -41     
  Branches     5601     5560       -41     
===========================================
- Hits         3879     3284      -595     
- Misses       1430     2018      +588     
+ Partials      292      258       -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wangl-cc wangl-cc merged commit 7a94b26 into main Jan 4, 2026
33 of 34 checks passed
@wangl-cc wangl-cc deleted the ci/xtask-test branch January 4, 2026 06:49
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