feat(config): add maa-cli-config for v2 models#534
Open
wangl-cc wants to merge 4 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并且留了一些整体性的反馈：
- 当前的反序列化逻辑实际上不会产生
ValidationError的这些变体：EmptyWeekdays、EmptyAllCondition、EmptyAnyCondition和UnknownCondition（这些情况已经由 NonEmptyVec 和 deny_unknown_fields 处理了），所以要么在自定义的反序列化器里真正使用它们，要么把它们删除，以保持错误类型的最小化。 - 在
TimeRange::contains中，即使反序列化器会拒绝(None, None)这种状态，该分支目前仍然返回true；可以考虑改成unreachable!()/debug_assert!，或者直接删掉这个分支，让这个不变量更加显式。 task_resolve.rs中前两个测试（resolve_task_params_after_conditions_and_overrides和resolved_params_preserve_variable_placeholders）做了相同的初始化并包含完全相同的断言；你可以把它们合并成一个测试，或者抽取公共的辅助函数来避免重复。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 当前的反序列化逻辑实际上不会产生 `ValidationError` 的这些变体：`EmptyWeekdays`、`EmptyAllCondition`、`EmptyAnyCondition` 和 `UnknownCondition`（这些情况已经由 NonEmptyVec 和 deny_unknown_fields 处理了），所以要么在自定义的反序列化器里真正使用它们，要么把它们删除，以保持错误类型的最小化。
- 在 `TimeRange::contains` 中，即使反序列化器会拒绝 `(None, None)` 这种状态，该分支目前仍然返回 `true`；可以考虑改成 `unreachable!()`/`debug_assert!`，或者直接删掉这个分支，让这个不变量更加显式。
- `task_resolve.rs` 中前两个测试（`resolve_task_params_after_conditions_and_overrides` 和 `resolved_params_preserve_variable_placeholders`）做了相同的初始化并包含完全相同的断言；你可以把它们合并成一个测试，或者抽取公共的辅助函数来避免重复。
## Individual Comments
### Comment 1
<location path="crates/maa-cli-config/src/task/condition.rs" line_range="329-331" />
<code_context>
+ let field = object.keys().next().map(String::as_str).unwrap_or_default();
+ Err(D::Error::unknown_field(field, CONDITION_FIELDS))
+ }
+ Value::Bool(_) => Err(D::Error::invalid_type(
+ serde::de::Unexpected::Bool(false),
+ &"a string or object condition",
+ )),
+ Value::Number(_) => Err(D::Error::invalid_type(
</code_context>
<issue_to_address>
**suggestion:** 在为非对象/字符串条件构造类型错误时，使用实际出现的非预期值。
在 `Bool`、`Number`、`Array` 和 `Null` 分支中，传给 `invalid_type` 的 `Unexpected` 是硬编码的（例如 `Unexpected::Bool(false)`、`Unexpected::Other("number")`），而不是使用真实的值。这会丢失错误信息中有用的上下文。
建议对具体的值做模式匹配并转发，例如：
```rust
Value::Bool(b) => Err(D::Error::invalid_type(
serde::de::Unexpected::Bool(b),
&"a string or object condition",
)),
```
对数字（`Unexpected::Signed`/`Unsigned`/`Float`）、数组（`Unexpected::Seq`）和 null（`Unexpected::Unit`）也做类似处理。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
ValidationErrorvariants likeEmptyWeekdays,EmptyAllCondition,EmptyAnyCondition, andUnknownConditionare never produced by the current deserialization logic (NonEmptyVec and deny_unknown_fields handle these cases), so either wire them into the custom deserializers or remove them to keep the error surface minimal. - In
TimeRange::contains, the(None, None)arm currently returnstrueeven though the deserializer rejects that state; consider replacing it with anunreachable!()/debug_assert!or removing the arm to make the invariant explicit. - The first two tests in
task_resolve.rs(resolve_task_params_after_conditions_and_overridesandresolved_params_preserve_variable_placeholders) perform the same setup and identical assertions; you can collapse them into a single test or factor out the shared helper to avoid duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ValidationError` variants like `EmptyWeekdays`, `EmptyAllCondition`, `EmptyAnyCondition`, and `UnknownCondition` are never produced by the current deserialization logic (NonEmptyVec and deny_unknown_fields handle these cases), so either wire them into the custom deserializers or remove them to keep the error surface minimal.
- In `TimeRange::contains`, the `(None, None)` arm currently returns `true` even though the deserializer rejects that state; consider replacing it with an `unreachable!()`/`debug_assert!` or removing the arm to make the invariant explicit.
- The first two tests in `task_resolve.rs` (`resolve_task_params_after_conditions_and_overrides` and `resolved_params_preserve_variable_placeholders`) perform the same setup and identical assertions; you can collapse them into a single test or factor out the shared helper to avoid duplication.
## Individual Comments
### Comment 1
<location path="crates/maa-cli-config/src/task/condition.rs" line_range="329-331" />
<code_context>
+ let field = object.keys().next().map(String::as_str).unwrap_or_default();
+ Err(D::Error::unknown_field(field, CONDITION_FIELDS))
+ }
+ Value::Bool(_) => Err(D::Error::invalid_type(
+ serde::de::Unexpected::Bool(false),
+ &"a string or object condition",
+ )),
+ Value::Number(_) => Err(D::Error::invalid_type(
</code_context>
<issue_to_address>
**suggestion:** Use the actual unexpected value when constructing type errors for non-object/string conditions.
In the `Bool`, `Number`, `Array`, and `Null` branches, the `Unexpected` passed to `invalid_type` is hardcoded (e.g. `Unexpected::Bool(false)`, `Unexpected::Other("number")`) instead of using the actual value. This drops useful context in error messages.
Pattern‑match the concrete value and forward it, e.g.:
```rust
Value::Bool(b) => Err(D::Error::invalid_type(
serde::de::Unexpected::Bool(b),
&"a string or object condition",
)),
```
and similarly for numbers (`Unexpected::Signed`/`Unsigned`/`Float`), arrays (`Unexpected::Seq`), and null (`Unexpected::Unit`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-0.8 #534 +/- ##
===========================================
+ Coverage 72.14% 76.46% +4.32%
===========================================
Files 72 82 +10
Lines 6541 8240 +1699
Branches 6541 8240 +1699
===========================================
+ Hits 4719 6301 +1582
- Misses 1509 1612 +103
- Partials 313 327 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new maa-cli-config crate to model the v2 profile/task configuration format (including condition/time handling and task resolution) and integrates it into the workspace, while also fixing MAAValueTemplate::Optional JSON schema generation to match its flattened serde shape.
Changes:
- Introduce
maa-cli-configwith v2 profile/task domain models, condition/time evaluation, schema generation (feature-gated), and fixture-backed tests. - Fix
MAAValueTemplate::Optionalschema to reflect flattenedvaluerepresentation and add a regression test. - Register the new crate in the workspace and update markdownlint configuration for the spec doc.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/maa-value/src/value.rs | Adjust Optional schema shape + add schema regression test. |
| crates/maa-cli-config/tests/task_resolve.rs | Add task resolution tests using fixtures and a non-interactive resolver. |
| crates/maa-cli-config/tests/profile_examples.rs | Add profile parsing tests across TOML/YAML/JSON fixtures. |
| crates/maa-cli-config/src/version.rs | Define v2 config Version type (+ schema when enabled). |
| crates/maa-cli-config/src/task/time.rs | Implement time zone parsing and generic time/date range handling. |
| crates/maa-cli-config/src/task/single.rs | Add task template/runtime models, overrides, and parameter resolution logic. |
| crates/maa-cli-config/src/task/session.rs | Add session template/runtime models and resolution. |
| crates/maa-cli-config/src/task/mod.rs | Wire up task submodules and public re-exports. |
| crates/maa-cli-config/src/task/config.rs | Add task-config template/runtime models, mode validation, fixtures/tests, schema wrapper. |
| crates/maa-cli-config/src/task/condition.rs | Implement condition model, parsing, schema, and evaluation tests. |
| crates/maa-cli-config/src/profile.rs | Add profile model/types, connection variants, and basic parsing tests. |
| crates/maa-cli-config/src/lib.rs | Crate entrypoint: exports, schema helper fns, schema smoke tests. |
| crates/maa-cli-config/src/error.rs | Define validation error types/messages used by config parsing. |
| crates/maa-cli-config/SPEC.md | Add v2 configuration specification document (Chinese). |
| crates/maa-cli-config/README.md | Add crate README and link to the spec. |
| crates/maa-cli-config/fixtures/task/tasks.yaml | Add v2 task config example (YAML). |
| crates/maa-cli-config/fixtures/task/tasks.toml | Add v2 task config example (TOML). |
| crates/maa-cli-config/fixtures/task/tasks.json | Add v2 task config example (JSON). |
| crates/maa-cli-config/fixtures/task/sessions.yaml | Add multi-session task config example (YAML). |
| crates/maa-cli-config/fixtures/task/sessions.toml | Add multi-session task config example (TOML). |
| crates/maa-cli-config/fixtures/task/placeholders.yaml | Add placeholder/template-value task fixture. |
| crates/maa-cli-config/fixtures/task/conditions.yaml | Add condition coverage fixture. |
| crates/maa-cli-config/fixtures/profile/waydroid.yaml | Add profile fixture (Waydroid). |
| crates/maa-cli-config/fixtures/profile/playcover.yaml | Add profile fixture (PlayCover). |
| crates/maa-cli-config/fixtures/profile/mumupro.json | Add profile fixture (MuMuPro). |
| crates/maa-cli-config/fixtures/profile/inherit.yaml | Add profile fixture demonstrating inherits. |
| crates/maa-cli-config/fixtures/profile/general.toml | Add general profile fixture. |
| crates/maa-cli-config/fixtures/profile/avd.toml | Add AVD profile fixture. |
| crates/maa-cli-config/fixtures/invalid/version.yaml | Add invalid fixture for version validation. |
| crates/maa-cli-config/fixtures/invalid/missing_mode.yaml | Add invalid fixture for missing task mode. |
| crates/maa-cli-config/fixtures/invalid/empty_weekdays.yaml | Add invalid fixture for empty weekdays. |
| crates/maa-cli-config/fixtures/invalid/empty_time.yaml | Add invalid fixture for empty time range. |
| crates/maa-cli-config/fixtures/invalid/empty_tasks.yaml | Add invalid fixture for empty tasks list. |
| crates/maa-cli-config/fixtures/invalid/empty_sessions.yaml | Add invalid fixture for empty sessions list. |
| crates/maa-cli-config/fixtures/invalid/empty_session_tasks.yaml | Add invalid fixture for empty session tasks. |
| crates/maa-cli-config/fixtures/invalid/empty_datetime.yaml | Add invalid fixture for empty datetime range. |
| crates/maa-cli-config/fixtures/invalid/empty_any.yaml | Add invalid fixture for empty any. |
| crates/maa-cli-config/fixtures/invalid/empty_all.yaml | Add invalid fixture for empty all. |
| crates/maa-cli-config/fixtures/invalid/conflicting_modes.yaml | Add invalid fixture for conflicting modes. |
| crates/maa-cli-config/fixtures/invalid/account_name_with_sessions.yaml | Add invalid fixture for disallowed top-level account name in sessions mode. |
| crates/maa-cli-config/Cargo.toml | Define new crate package metadata, deps, and schema feature. |
| Cargo.toml | Register maa-cli-config in workspace dependencies. |
| Cargo.lock | Lockfile updates for new crate inclusion. |
| .markdownlint.yaml | Allow duplicate top-level headings across non-sibling scopes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+141
to
+168
| pub trait TimeIn: Ord { | ||
| fn not_after(&self, until: &Self) -> bool { | ||
| self <= until | ||
| } | ||
|
|
||
| fn is_after(&self, from: &Self) -> bool { | ||
| self > from | ||
| } | ||
|
|
||
| fn is_in_range(&self, from: &Self, until: &Self) -> bool { | ||
| self.is_after(from) && self.not_after(until) | ||
| } | ||
| } | ||
|
|
||
| impl TimeIn for DateTime {} | ||
|
|
||
| impl TimeIn for NaiveDateTime {} | ||
|
|
||
| impl TimeIn for NaiveTime { | ||
| // This is a workaround to allow cross-midnight time ranges for NaiveTime. | ||
| fn is_in_range(&self, from: &Self, until: &Self) -> bool { | ||
| if from <= until { | ||
| from <= self && self < until | ||
| } else { | ||
| from <= self || self < until | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TimeIn/TimeRange::contains currently use inconsistent and likely unintended boundary semantics: TimeIn::is_in_range is (from, until] (exclusive start, inclusive end), while NaiveTime overrides to [from, until) and wraparound logic is also inclusive on from. This makes time_range/date_range/OnSideStory conditions exclude the exact from instant (and include until) and also makes open-ended ranges (from only / until only) behave differently than bounded ranges. Consider standardizing on a single convention (commonly [from, until) and inclusive for open-ended bounds) by adjusting is_after/not_after or TimeRange::contains to use >= for from and < for until consistently across DateTime, NaiveDateTime, and NaiveTime.
Comment on lines
+8
to
+16
| #[cfg(feature = "schema")] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, PartialEq, Deserialize)] | ||
| pub struct VersionedTaskConfig { | ||
| pub version: crate::Version, | ||
| #[serde(flatten)] | ||
| pub config: TaskConfigTemplate, | ||
| } | ||
|
|
There was a problem hiding this comment.
VersionedTaskConfig is entirely #[cfg(feature = "schema")], which means normal (non-schema) builds cannot deserialize/validate the required top-level version field at all, and TaskConfigTemplate will silently ignore version as an unknown field. If version is part of the v2 file format (and is needed for correctness, not only schema generation), consider making VersionedTaskConfig always available and only gating the JsonSchema derive/impl with cfg_attr(feature = "schema", ...), so runtime parsing can reject unsupported versions too.
Comment on lines
+4
to
+11
| #[cfg(feature = "schema")] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, PartialEq, Deserialize)] | ||
| pub struct VersionedProfileConfig { | ||
| pub version: crate::Version, | ||
| #[serde(flatten)] | ||
| pub config: ProfileConfig, | ||
| } |
There was a problem hiding this comment.
VersionedProfileConfig is fully behind #[cfg(feature = "schema")], so non-schema builds parsing ProfileConfig will accept and ignore any version value (including unsupported ones). If v2 profiles require version = 2, consider making the versioned wrapper type available unconditionally (and only gating the JsonSchema derive) so consumers can validate version at deserialization time.
Comment on lines
+75
to
+110
| #[test] | ||
| fn resolved_params_preserve_variable_placeholders() { | ||
| let task_config: TaskConfigTemplate = | ||
| serde_yaml::from_str(include_str!("../fixtures/task/placeholders.yaml")).unwrap(); | ||
| let session = &task_config.sessions[0]; | ||
|
|
||
| let now = chrono::DateTime::<Utc>::UNIX_EPOCH + chrono::TimeDelta::minutes(30); | ||
| let context = ConditionContext { | ||
| now, | ||
| side_story_open_time: Some(( | ||
| chrono::DateTime::<Utc>::UNIX_EPOCH, | ||
| chrono::DateTime::<Utc>::UNIX_EPOCH + chrono::TimeDelta::hours(1), | ||
| )), | ||
| }; | ||
|
|
||
| let mut resolver = PanicResolver; | ||
| let params = session.tasks[0] | ||
| .clone() | ||
| .resolve(&context, &mut resolver) | ||
| .unwrap() | ||
| .unwrap() | ||
| .params; | ||
|
|
||
| assert_eq!( | ||
| params.get("stage"), | ||
| Some(&MAAValue::Primitive(MAAPrimitive::String( | ||
| "${stage}".into() | ||
| ))) | ||
| ); | ||
| assert_eq!( | ||
| params.get("note"), | ||
| Some(&MAAValue::Primitive(MAAPrimitive::String( | ||
| "side ${stage}".into() | ||
| ))) | ||
| ); | ||
| } |
There was a problem hiding this comment.
resolved_params_preserve_variable_placeholders duplicates resolve_task_params_after_conditions_and_overrides (same fixture, same context, same assertions). This adds test runtime without increasing coverage. Consider removing one test or changing one to cover a distinct behavior (e.g., verify the non-side-story path keeps the base note, or verify inactive tasks don’t resolve).
Comment on lines
+705
to
+713
| #[test] | ||
| fn optional_schema_matches_flattened_value_shape() { | ||
| let schema = schemars::schema_for!(MAAValueTemplate); | ||
| let value = serde_json::to_value(&schema).unwrap(); | ||
| let schema_text = value.to_string(); | ||
|
|
||
| assert!(schema_text.contains("\"conditions\"")); | ||
| assert!(!schema_text.contains("\"required\":[\"conditions\",\"value\"]")); | ||
| } |
There was a problem hiding this comment.
The schema regression test is implemented via substring checks on serde_json::Value::to_string(), which can be brittle (e.g., schema output ordering changes) and doesn’t assert the actual structure of the Optional branch (like additionalProperties shape). Consider asserting on the relevant JSON pointers/fields within the schema value (navigate into oneOf variants and check required/additionalProperties directly) to make the test robust and targeted.
wangl-cc
force-pushed
the
feat/config-v2
branch
from
abb67b0 to
563d2fc
Compare
Add a dedicated crate for the v2 profile and task config model. It provides schema generation, domain-focused template/runtime separation, config resolution, and fixture-backed regression tests.
Support inherited profile connections with a resolved profile type while reorganizing the profile config code into focused modules. Keep validation in the config layer and leave runtime-specific resolution to upper layers.
wangl-cc
force-pushed
the
feat/config-v2
branch
from
563d2fc to
ff583f6
Compare
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 47 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+141
to
+152
| pub trait TimeIn: Ord { | ||
| fn not_after(&self, until: &Self) -> bool { | ||
| self <= until | ||
| } | ||
|
|
||
| fn is_after(&self, from: &Self) -> bool { | ||
| self > from | ||
| } | ||
|
|
||
| fn is_in_range(&self, from: &Self, until: &Self) -> bool { | ||
| self.is_after(from) && self.not_after(until) | ||
| } |
There was a problem hiding this comment.
TimeIn range semantics are inconsistent: the default implementation uses self > from and self <= until, so DateTime/NaiveDateTime ranges exclude the start boundary but include the end boundary. Meanwhile NaiveTime::is_in_range includes from and excludes until. This can make conditions like OnSideStory unexpectedly inactive exactly at the opening timestamp, and makes TimeRange::contains behave differently depending on T. Consider standardizing the inclusivity (e.g., include from and exclude until across all types, or otherwise make the behavior consistent).
Comment on lines
+8
to
+24
| #[cfg(feature = "schema")] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, PartialEq, Deserialize)] | ||
| pub struct VersionedTaskConfig { | ||
| pub version: crate::Version, | ||
| #[serde(flatten)] | ||
| pub config: TaskConfigTemplate, | ||
| } | ||
|
|
||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, PartialEq, Deserialize)] | ||
| #[serde(try_from = "RawTaskConfig")] | ||
| pub struct TaskConfigTemplate { | ||
| pub manage_environment_lifecycle: bool, | ||
| pub manage_game_lifecycle: bool, | ||
| pub sessions: NonEmptyVec<SessionTemplate>, | ||
| } |
There was a problem hiding this comment.
TaskConfigTemplate ignores the top-level version field (it deserializes via RawTaskConfig, which has no version and doesn’t deny unknown fields). Because VersionedTaskConfig is only available under the schema feature, builds without schema can’t validate version and will silently accept unsupported versions. If version is meant to be required (per SPEC), consider making the versioned wrapper available unconditionally (keeping only the JsonSchema derive behind cfg_attr), and/or requiring/validating version during normal deserialization.
Comment on lines
+6
to
+17
| #[cfg(feature = "schema")] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, PartialEq, Deserialize)] | ||
| pub struct VersionedProfileConfig { | ||
| pub version: crate::Version, | ||
| #[serde(flatten)] | ||
| pub config: ProfileConfig, | ||
| } | ||
|
|
||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[derive(Clone, Debug, Default, PartialEq, Deserialize)] | ||
| pub struct ProfileConfig { |
There was a problem hiding this comment.
ProfileConfig similarly ignores the top-level version field, and VersionedProfileConfig is only available under the schema feature. That means (without schema) unsupported versions can’t be rejected at parse time. If version is required for the v2 format, consider exposing the versioned wrapper unconditionally (with only schema-related derives behind cfg_attr) and using it for parsing/validation.
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体性的反馈：
- 对 AVD
sdk_path的校验逻辑在ProfileConfig::validate和ProfileConfig::resolve中重复出现；可以考虑让resolve调用validate（或一个共享的辅助函数），把检查集中到一个地方。 ValidationError中包含EmptyWeekdays、EmptyAllCondition、EmptyAnyCondition和EmptyConditionObject等枚举变体，看起来没有被使用；对应的校验目前是由nonempty-vec和自定义的 serde 提示信息驱动的。可以考虑要么在条件反序列化逻辑中接入这些变体，要么将其移除以减少死代码。tests/task_resolve.rs中的测试resolve_task_params_after_conditions_and_overrides和resolved_params_preserve_variable_placeholders实际上验证的是同样的行为，而且断言完全相同；可以考虑把它们合并成一个测试以避免冗余。
给 AI Agent 的提示
请根据下面这次代码评审中的意见进行修改：
## 总体评论
- 对 AVD `sdk_path` 的校验逻辑在 `ProfileConfig::validate` 和 `ProfileConfig::resolve` 中重复出现；可以考虑让 `resolve` 调用 `validate`（或一个共享的辅助函数），把检查集中到一个地方。
- `ValidationError` 中包含 `EmptyWeekdays`、`EmptyAllCondition`、`EmptyAnyCondition` 和 `EmptyConditionObject` 等枚举变体，看起来没有被使用；对应的校验目前是由 `nonempty-vec` 和自定义的 serde 提示信息驱动的。可以考虑要么在条件反序列化逻辑中接入这些变体，要么将其移除以减少死代码。
- `tests/task_resolve.rs` 中的测试 `resolve_task_params_after_conditions_and_overrides` 和 `resolved_params_preserve_variable_placeholders` 实际上验证的是同样的行为，而且断言完全相同；可以考虑把它们合并成一个测试以避免冗余。
## 具体评论
### 评论 1
<location path="crates/maa-cli-config/src/task/time.rs" line_range="141-150" />
<code_context>
+ .expect("time offset within supported hour range")
+}
+
+pub trait TimeIn: Ord {
+ fn not_after(&self, until: &Self) -> bool {
+ self <= until
+ }
+
+ fn is_after(&self, from: &Self) -> bool {
+ self > from
+ }
+
+ fn is_in_range(&self, from: &Self, until: &Self) -> bool {
+ self.is_after(from) && self.not_after(until)
+ }
+}
+
+impl TimeIn for DateTime {}
+
+impl TimeIn for NaiveDateTime {}
+
+impl TimeIn for NaiveTime {
+ // This is a workaround to allow cross-midnight time ranges for NaiveTime.
+ fn is_in_range(&self, from: &Self, until: &Self) -> bool {
</code_context>
<issue_to_address>
**issue (bug_risk):** Date/DateTime 范围在 `from` 上是开区间、在 `until` 上是闭区间，这和 `NaiveTime` 的实现不一致，可能会让人困惑。
`DateTime`/`NaiveDateTime` 的 `TimeIn::is_in_range` 使用的是 `self > from && self <= until`（`(from, until]`），而 `NaiveTime` 使用的是 `from <= self && self < until`（或跨午夜时的环绕逻辑）（`[from, until)`）。对通过 `TimeRange::contains` 使用这些类型时，如果两者在区间边界约定上不一致，很容易引入微妙的 off-by-one（边界）错误。建议统一这些类型的语义，或者在确实有意为之的情况下明确记录这一差异，并增加针对边界的测试来保证行为不被意外改变。
</issue_to_address>
### 评论 2
<location path="crates/maa-cli-config/src/task/condition.rs" line_range="344-345" />
<code_context>
+ let field = object.keys().next().map(String::as_str).unwrap_or_default();
+ Err(D::Error::unknown_field(field, CONDITION_FIELDS))
+ }
+ Value::Bool(_) => Err(D::Error::invalid_type(
+ serde::de::Unexpected::Bool(false),
+ &"a string or object condition",
+ )),
</code_context>
<issue_to_address>
**nitpick：** 使用 `Unexpected::Bool(false)` 会忽略实际的布尔值，可能导致错误信息具有误导性。
这里你丢弃了真实的布尔值，而是始终报告 `Unexpected::Bool(false)`，即使输入是 `true` 也是如此。为了让错误信息更加准确，建议绑定实际值（例如 `Value::Bool(v)`），并传入 `Unexpected::Bool(v)`。
</issue_to_address>
### 评论 3
<location path="crates/maa-cli-config/src/profile/config.rs" line_range="71-78" />
<code_context>
+ /// Validate that a resolved profile has all required fields.
+ ///
+ /// After inheritance is resolved, `connection` must be present.
+ pub fn validate(&self) -> Result<(), crate::ValidationError> {
+ match self.connection.as_ref() {
+ None => Err(crate::ValidationError::MissingConnection),
+ Some(ConnectionConfig::AVD(crate::profile::connection::AvdConnectionConfig {
+ sdk_path: Some(path),
+ ..
+ })) if path.trim().is_empty() => Err(crate::ValidationError::EmptyAvdSdkPath),
+ Some(_) => Ok(()),
+ }
+ }
</code_context>
<issue_to_address>
**suggestion：** AVD `sdk_path` 的校验逻辑在 `validate` 和 `resolve` 中重复出现。
在 `validate` 和 `resolve` 中都进行同样的 AVD 空/仅空白字符 `sdk_path` 检查，会增加未来规则变更时两者行为不一致的风险。建议将这段检查逻辑抽取到一个共享的辅助函数（例如 `fn validate_connection(&ConnectionConfig) -> Result<(), ValidationError>`），或者让 `resolve` 调用 `validate`，从而只在一个地方维护校验逻辑。
建议实现：
```rust
/// Validate that a resolved profile has all required fields.
///
/// After inheritance is resolved, `connection` must be present.
fn validate_connection(
connection: &ConnectionConfig,
) -> Result<(), crate::ValidationError> {
match connection {
ConnectionConfig::AVD(crate::profile::connection::AvdConnectionConfig {
sdk_path: Some(path),
..
}) if path.trim().is_empty() => Err(crate::ValidationError::EmptyAvdSdkPath),
_ => Ok(()),
}
}
pub fn validate(&self) -> Result<(), crate::ValidationError> {
match self.connection.as_ref() {
None => Err(crate::ValidationError::MissingConnection),
Some(connection) => Self::validate_connection(connection),
}
}
```
现在，AVD `sdk_path` 的校验集中在 `validate_connection` 中。要彻底去除重复逻辑：
1. 在该 profile 的 `resolve` 实现中（很可能在同一个文件），将任何内联的 AVD `sdk_path` 检查替换为对 `Self::validate_connection(&connection)` 的调用，例如：
- 对 `ConnectionConfig::AVD(AvdConnectionConfig { sdk_path: Some(path), .. })` 的匹配，并带有 `trim().is_empty()` 条件保护的地方。
如果 `resolve` 返回的是一个已经设置好 `connection` 的已解析 profile，也可以直接调用 `resolved.validate()`。
2. 确保 `resolve` 的返回类型能传播 `crate::ValidationError`，使 `validate_connection` / `validate` 能直接融入它的 `Result` 流程中。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- There is duplicated validation logic for AVD
sdk_pathinProfileConfig::validateandProfileConfig::resolve; consider havingresolvedelegate tovalidate(or a shared helper) to keep the checks in one place. ValidationErrorincludes variants likeEmptyWeekdays,EmptyAllCondition,EmptyAnyCondition, andEmptyConditionObjectthat appear unused while corresponding validation is currently driven bynonempty-vecand custom serde messages; it may be worth either wiring these variants into the condition deserialization path or removing them to reduce dead code.- The tests
resolve_task_params_after_conditions_and_overridesandresolved_params_preserve_variable_placeholdersintests/task_resolve.rsexercise effectively the same behavior with identical assertions; consider consolidating them into a single test to avoid redundancy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is duplicated validation logic for AVD `sdk_path` in `ProfileConfig::validate` and `ProfileConfig::resolve`; consider having `resolve` delegate to `validate` (or a shared helper) to keep the checks in one place.
- `ValidationError` includes variants like `EmptyWeekdays`, `EmptyAllCondition`, `EmptyAnyCondition`, and `EmptyConditionObject` that appear unused while corresponding validation is currently driven by `nonempty-vec` and custom serde messages; it may be worth either wiring these variants into the condition deserialization path or removing them to reduce dead code.
- The tests `resolve_task_params_after_conditions_and_overrides` and `resolved_params_preserve_variable_placeholders` in `tests/task_resolve.rs` exercise effectively the same behavior with identical assertions; consider consolidating them into a single test to avoid redundancy.
## Individual Comments
### Comment 1
<location path="crates/maa-cli-config/src/task/time.rs" line_range="141-150" />
<code_context>
+ .expect("time offset within supported hour range")
+}
+
+pub trait TimeIn: Ord {
+ fn not_after(&self, until: &Self) -> bool {
+ self <= until
+ }
+
+ fn is_after(&self, from: &Self) -> bool {
+ self > from
+ }
+
+ fn is_in_range(&self, from: &Self, until: &Self) -> bool {
+ self.is_after(from) && self.not_after(until)
+ }
+}
+
+impl TimeIn for DateTime {}
+
+impl TimeIn for NaiveDateTime {}
+
+impl TimeIn for NaiveTime {
+ // This is a workaround to allow cross-midnight time ranges for NaiveTime.
+ fn is_in_range(&self, from: &Self, until: &Self) -> bool {
</code_context>
<issue_to_address>
**issue (bug_risk):** Date/DateTime ranges are exclusive on `from` and inclusive on `until`, which differs from the NaiveTime implementation and may be surprising.
`TimeIn::is_in_range` for `DateTime`/`NaiveDateTime` uses `self > from && self <= until` (`(from, until]`), while `NaiveTime` uses `from <= self && self < until` (or wraparound) (`[from, until)`). Using different boundary conventions for types consumed via `TimeRange::contains` can easily introduce subtle off-by-one bugs. It would be good to align these semantics across types, or clearly document the intentional difference and add boundary-focused tests to enforce it.
</issue_to_address>
### Comment 2
<location path="crates/maa-cli-config/src/task/condition.rs" line_range="344-345" />
<code_context>
+ let field = object.keys().next().map(String::as_str).unwrap_or_default();
+ Err(D::Error::unknown_field(field, CONDITION_FIELDS))
+ }
+ Value::Bool(_) => Err(D::Error::invalid_type(
+ serde::de::Unexpected::Bool(false),
+ &"a string or object condition",
+ )),
</code_context>
<issue_to_address>
**nitpick:** Using `Unexpected::Bool(false)` ignores the actual boolean value and may produce confusing error messages.
Here you discard the actual boolean value and always report `Unexpected::Bool(false)`, even when the input is `true`. To keep error messages accurate, bind the value (e.g. `Value::Bool(v)`) and pass `Unexpected::Bool(v)` instead.
</issue_to_address>
### Comment 3
<location path="crates/maa-cli-config/src/profile/config.rs" line_range="71-78" />
<code_context>
+ /// Validate that a resolved profile has all required fields.
+ ///
+ /// After inheritance is resolved, `connection` must be present.
+ pub fn validate(&self) -> Result<(), crate::ValidationError> {
+ match self.connection.as_ref() {
+ None => Err(crate::ValidationError::MissingConnection),
+ Some(ConnectionConfig::AVD(crate::profile::connection::AvdConnectionConfig {
+ sdk_path: Some(path),
+ ..
+ })) if path.trim().is_empty() => Err(crate::ValidationError::EmptyAvdSdkPath),
+ Some(_) => Ok(()),
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** AVD `sdk_path` validation logic is duplicated between `validate` and `resolve`.
Having the same `AVD` empty/whitespace `sdk_path` check in both `validate` and `resolve` increases the risk they diverge if the rules change. Extract this check into a shared helper (e.g., `fn validate_connection(&ConnectionConfig) -> Result<(), ValidationError>`) or call `validate` from `resolve` so the logic lives in a single place.
Suggested implementation:
```rust
/// Validate that a resolved profile has all required fields.
///
/// After inheritance is resolved, `connection` must be present.
fn validate_connection(
connection: &ConnectionConfig,
) -> Result<(), crate::ValidationError> {
match connection {
ConnectionConfig::AVD(crate::profile::connection::AvdConnectionConfig {
sdk_path: Some(path),
..
}) if path.trim().is_empty() => Err(crate::ValidationError::EmptyAvdSdkPath),
_ => Ok(()),
}
}
pub fn validate(&self) -> Result<(), crate::ValidationError> {
match self.connection.as_ref() {
None => Err(crate::ValidationError::MissingConnection),
Some(connection) => Self::validate_connection(connection),
}
}
```
The AVD `sdk_path` validation is now centralized in `validate_connection`. To fully remove duplication:
1. In the `resolve` implementation for this profile (likely in the same file), replace any inline AVD `sdk_path` checks such as:
- Pattern matches on `ConnectionConfig::AVD(AvdConnectionConfig { sdk_path: Some(path), .. })` with a `trim().is_empty()` guard
with a call to `Self::validate_connection(&connection)` (or `resolved.validate()` if `resolve` returns a resolved profile and already has `connection` set).
2. Ensure the return type of `resolve` propagates `crate::ValidationError` so that `validate_connection` / `validate` can be used directly in its `Result` flow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Unify task time-range boundary semantics, expose versioned config wrappers in normal builds, improve condition type errors, and remove duplicated task resolve coverage.
Member
|
Are there any presets for AVD "before" this PR is merged? I managed to make it work on Fedora in Wayland but there's no docs on it |
Member
Author
Sorry for the delayed response. Currently, there are no AVD presets available, so you will need to manage it manually for now. |
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
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.
What changed
maa-cli-configcrate for the v2 profile and task config modelMAAValueTemplate::Optionalschema to match its flattened serde shape and add direct schema generation tests inmaa-valueValidation
cargo +nightly fmt --allcargo clippy -p maa-cli-configcargo clippy -p maa-value --features schemacargo test -p maa-cli-configcargo test -p maa-cli-config --features schemacargo test -p maa-value --features schemacargo x testSummary by Sourcery
引入新的
maa-cli-configcrate，用于定义 v2 配置文件（profile）和任务（task）配置模型，包括条件（condition）、时间（time）以及会话/任务解析支持，并提供模式（schema）生成和基准数据（fixtures）。New Features:
maa-cli-configcrate，用于建模 v2 CLI 配置文件、连接（connections）、行为（behaviors）和高级设置（advanced settings），并支持继承与校验。Bug Fixes:
Enhancements:
maa-valueOptional 模板 schema 覆盖范围，使其更贴近对应的 serde 结构，并添加直接的 schema 测试。Documentation:
SPEC.md中记录 v2 配置设计与语义，涵盖配置文件（profiles）、任务（tasks）、生命周期处理（lifecycle handling）、条件（conditions）以及输入提示（input prompts）。Tests:
Original summary in English
Summary by Sourcery
Introduce a new maa-cli-config crate that defines the v2 profile and task configuration model, including condition, time, and session/task resolution support, along with schema generation and fixtures.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
引入新的
maa-cli-configcrate，用于定义 v2 配置文件（profile）和任务（task）配置模型，包括条件（condition）、时间（time）以及会话/任务解析支持，并提供模式（schema）生成和基准数据（fixtures）。New Features:
maa-cli-configcrate，用于建模 v2 CLI 配置文件、连接（connections）、行为（behaviors）和高级设置（advanced settings），并支持继承与校验。Bug Fixes:
Enhancements:
maa-valueOptional 模板 schema 覆盖范围，使其更贴近对应的 serde 结构，并添加直接的 schema 测试。Documentation:
SPEC.md中记录 v2 配置设计与语义，涵盖配置文件（profiles）、任务（tasks）、生命周期处理（lifecycle handling）、条件（conditions）以及输入提示（input prompts）。Tests:
Original summary in English
Summary by Sourcery
Introduce a new maa-cli-config crate that defines the v2 profile and task configuration model, including condition, time, and session/task resolution support, along with schema generation and fixtures.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: