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 - 我发现了两个问题，并给出了一些高层次的反馈：
into_parameters_no_context中对extra_tags_mode的手动校验可以移动到 clap 的定义中（例如value_parser = clap::value_parser!(i32).range(0..=2)），这样无效值会在解析阶段就被拒绝，而不是在转换阶段才发现。- 在
recruitment_time的解析循环中，建议使用splitn(2, '=')，并返回更结构化的错误信息（包含 level 和 minutes 部分），以便更容易调试格式错误的输入，并避免当=出现多于一次时产生意料之外的行为。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The manual validation of `extra_tags_mode` in `into_parameters_no_context` could be shifted into the clap definition (e.g., `value_parser = clap::value_parser!(i32).range(0..=2)`) so invalid values are rejected at parse time rather than at conversion.
- In the `recruitment_time` parsing loop, consider using `splitn(2, '=')` and a more structured error message (including the level and minutes parts) to make debugging malformed inputs easier and avoid surprises if `=` appears more than once.
## Individual Comments
### Comment 1
<location> `crates/maa-cli/src/run/preset/recruit.rs:134-146` </location>
<code_context>
+ params.insert("extra_tags_mode", self.extra_tags_mode);
+
+ // Times
+ params.insert("times", self.times);
+
+ // Set time only when times is 0
</code_context>
<issue_to_address>
**suggestion:** 考虑在将 `times` 插入参数之前，先校验它是否为非负数。
目前任何 `i32` 都会被接受（包括 `--times=-1`），这很可能是无效的，并可能导致下游行为令人困惑。可以加一个简单的检查，比如 `if self.times < 0 { bail!("times must be non-negative") }`，以便在遇到错误输入时尽早失败。
```suggestion
// Extra tags mode validation
if !(0..=2).contains(&self.extra_tags_mode) {
bail!("extra_tags_mode must be between 0 and 2");
}
params.insert("extra_tags_mode", self.extra_tags_mode);
// Times validation
if self.times < 0 {
bail!("times must be non-negative");
}
// Times
params.insert("times", self.times);
// Set time only when times is 0
if self.times == 0 {
params.insert("set_time", self.set_time);
}
```
</issue_to_address>
### Comment 2
<location> `crates/maa-cli/src/run/preset/recruit.rs:160-165` </location>
<code_context>
+ if !self.recruitment_time.is_empty() {
+ let mut time_map = std::collections::BTreeMap::new();
+
+ for time_spec in self.recruitment_time {
+ let mut parts = time_spec.split('=');
+ let level = parts.next();
+ let minutes = parts.next();
+
+ match (level, minutes) {
+ (Some(level), Some(minutes)) => {
+ let level_str = level.to_owned();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `recruitment_time` 解析器会静默忽略多余的 `=` 片段；建议显式拒绝这种格式错误的参数。
由于这里使用的是 `split('=')`，但只读取了两个元素，对类似 `--recruitment-time=3=540=foo` 的输入，解析结果是 `level="3"`、`minutes="540"`，而尾部的 `"foo"` 会被忽略。如果这不是期望的行为，建议在读取完 `level` 和 `minutes` 后检查 `parts.next().is_none()`，否则就返回错误，使格式错误的参数能尽早失败，而不是被部分接受。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈来改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The manual validation of
extra_tags_modeininto_parameters_no_contextcould be shifted into the clap definition (e.g.,value_parser = clap::value_parser!(i32).range(0..=2)) so invalid values are rejected at parse time rather than at conversion. - In the
recruitment_timeparsing loop, consider usingsplitn(2, '=')and a more structured error message (including the level and minutes parts) to make debugging malformed inputs easier and avoid surprises if=appears more than once.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual validation of `extra_tags_mode` in `into_parameters_no_context` could be shifted into the clap definition (e.g., `value_parser = clap::value_parser!(i32).range(0..=2)`) so invalid values are rejected at parse time rather than at conversion.
- In the `recruitment_time` parsing loop, consider using `splitn(2, '=')` and a more structured error message (including the level and minutes parts) to make debugging malformed inputs easier and avoid surprises if `=` appears more than once.
## Individual Comments
### Comment 1
<location> `crates/maa-cli/src/run/preset/recruit.rs:134-146` </location>
<code_context>
+ params.insert("extra_tags_mode", self.extra_tags_mode);
+
+ // Times
+ params.insert("times", self.times);
+
+ // Set time only when times is 0
</code_context>
<issue_to_address>
**suggestion:** Consider validating that `times` is non-negative before inserting it into parameters.
Currently any `i32` is accepted (including `--times=-1`), which is probably invalid and may lead to confusing downstream behavior. A small check like `if self.times < 0 { bail!("times must be non-negative") }` would fail fast on bad input.
```suggestion
// Extra tags mode validation
if !(0..=2).contains(&self.extra_tags_mode) {
bail!("extra_tags_mode must be between 0 and 2");
}
params.insert("extra_tags_mode", self.extra_tags_mode);
// Times validation
if self.times < 0 {
bail!("times must be non-negative");
}
// Times
params.insert("times", self.times);
// Set time only when times is 0
if self.times == 0 {
params.insert("set_time", self.set_time);
}
```
</issue_to_address>
### Comment 2
<location> `crates/maa-cli/src/run/preset/recruit.rs:160-165` </location>
<code_context>
+ if !self.recruitment_time.is_empty() {
+ let mut time_map = std::collections::BTreeMap::new();
+
+ for time_spec in self.recruitment_time {
+ let mut parts = time_spec.split('=');
+ let level = parts.next();
+ let minutes = parts.next();
+
+ match (level, minutes) {
+ (Some(level), Some(minutes)) => {
+ let level_str = level.to_owned();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `recruitment_time` parser silently ignores extra `=` segments; consider rejecting malformed specs explicitly.
Because you call `split('=')` and only read two elements, an input like `--recruitment-time=3=540=foo` is parsed as `level="3"`, `minutes="540"` and the trailing `"foo"` is ignored. If that’s not desired, consider checking `parts.next().is_none()` after reading `level` and `minutes` and returning an error otherwise, so malformed specs fail fast instead of being partially accepted.
</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
+134
to
+146
| // Extra tags mode validation | ||
| if !(0..=2).contains(&self.extra_tags_mode) { | ||
| bail!("extra_tags_mode must be between 0 and 2"); | ||
| } | ||
| params.insert("extra_tags_mode", self.extra_tags_mode); | ||
|
|
||
| // Times | ||
| params.insert("times", self.times); | ||
|
|
||
| // Set time only when times is 0 | ||
| if self.times == 0 { | ||
| params.insert("set_time", self.set_time); | ||
| } |
There was a problem hiding this comment.
suggestion: 考虑在将 times 插入参数之前，先校验它是否为非负数。
目前任何 i32 都会被接受（包括 --times=-1），这很可能是无效的，并可能导致下游行为令人困惑。可以加一个简单的检查，比如 if self.times < 0 { bail!("times must be non-negative") }，以便在遇到错误输入时尽早失败。
Suggested change
| // Extra tags mode validation | |
| if !(0..=2).contains(&self.extra_tags_mode) { | |
| bail!("extra_tags_mode must be between 0 and 2"); | |
| } | |
| params.insert("extra_tags_mode", self.extra_tags_mode); | |
| // Times | |
| params.insert("times", self.times); | |
| // Set time only when times is 0 | |
| if self.times == 0 { | |
| params.insert("set_time", self.set_time); | |
| } | |
| // Extra tags mode validation | |
| if !(0..=2).contains(&self.extra_tags_mode) { | |
| bail!("extra_tags_mode must be between 0 and 2"); | |
| } | |
| params.insert("extra_tags_mode", self.extra_tags_mode); | |
| // Times validation | |
| if self.times < 0 { | |
| bail!("times must be non-negative"); | |
| } | |
| // Times | |
| params.insert("times", self.times); | |
| // Set time only when times is 0 | |
| if self.times == 0 { | |
| params.insert("set_time", self.set_time); | |
| } |
Original comment in English
suggestion: Consider validating that times is non-negative before inserting it into parameters.
Currently any i32 is accepted (including --times=-1), which is probably invalid and may lead to confusing downstream behavior. A small check like if self.times < 0 { bail!("times must be non-negative") } would fail fast on bad input.
Suggested change
| // Extra tags mode validation | |
| if !(0..=2).contains(&self.extra_tags_mode) { | |
| bail!("extra_tags_mode must be between 0 and 2"); | |
| } | |
| params.insert("extra_tags_mode", self.extra_tags_mode); | |
| // Times | |
| params.insert("times", self.times); | |
| // Set time only when times is 0 | |
| if self.times == 0 { | |
| params.insert("set_time", self.set_time); | |
| } | |
| // Extra tags mode validation | |
| if !(0..=2).contains(&self.extra_tags_mode) { | |
| bail!("extra_tags_mode must be between 0 and 2"); | |
| } | |
| params.insert("extra_tags_mode", self.extra_tags_mode); | |
| // Times validation | |
| if self.times < 0 { | |
| bail!("times must be non-negative"); | |
| } | |
| // Times | |
| params.insert("times", self.times); | |
| // Set time only when times is 0 | |
| if self.times == 0 { | |
| params.insert("set_time", self.set_time); | |
| } |
Comment on lines
+160
to
+165
| for time_spec in self.recruitment_time { | ||
| let mut parts = time_spec.split('='); | ||
| let level = parts.next(); | ||
| let minutes = parts.next(); | ||
|
|
||
| match (level, minutes) { |
There was a problem hiding this comment.
suggestion (bug_risk): recruitment_time 解析器会静默忽略多余的 = 片段；建议显式拒绝这种格式错误的参数。
由于这里使用的是 split('=')，但只读取了两个元素，对类似 --recruitment-time=3=540=foo 的输入，解析结果是 level="3"、minutes="540"，而尾部的 "foo" 会被忽略。如果这不是期望的行为，建议在读取完 level 和 minutes 后检查 parts.next().is_none()，否则就返回错误，使格式错误的参数能尽早失败，而不是被部分接受。
Original comment in English
suggestion (bug_risk): The recruitment_time parser silently ignores extra = segments; consider rejecting malformed specs explicitly.
Because you call split('=') and only read two elements, an input like --recruitment-time=3=540=foo is parsed as level="3", minutes="540" and the trailing "foo" is ignored. If that’s not desired, consider checking parts.next().is_none() after reading level and minutes and returning an error otherwise, so malformed specs fail fast instead of being partially accepted.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
==========================================
+ Coverage 69.40% 69.66% +0.26%
==========================================
Files 56 57 +1
Lines 5608 5670 +62
Branches 5608 5670 +62
==========================================
+ Hits 3892 3950 +58
- Misses 1425 1428 +3
- Partials 291 292 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
在 CLI 中新增一个
recruit预设命令，用于配置和运行招募任务，并提供丰富的标签配置和报表选项。新功能：
RecruitCLI 子命令，通过通用的预设运行器来执行招募任务。RecruitParams预设配置，支持标签选择/确认、优先标签、额外标签选择模式、运行次数、时间限制、加急许可以及服务器选择。测试：
recruit命令新增全面的 CLI 解析测试，覆盖正常流程、参数组合以及校验错误等情况。Original summary in English
Summary by Sourcery
Add a new
recruitpreset command to the CLI for configuring and running recruitment tasks with rich tagging and reporting options.New Features:
RecruitCLI subcommand wired into the main command dispatcher to run recruitment tasks via the generic preset runner.RecruitParamspreset configuration supporting tag selection/confirmation, preferred tags, extra tag selection modes, run count, time limits, expedited permits, and server selection.Tests:
recruitcommand covering normal flows, parameter combinations, and validation error cases.