fix: preserve failed action details#1320
Open
zitonwei wants to merge 3 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些高层次的反馈：
- 在
Actuator::run中，当action_id为MaaInvalidId时，后备分支会完全替换整个ActionResult；如果调用方可能在失败结果上填充其他字段，建议只填补缺失的字段（例如action_id、name、action、box、success），而不是丢弃已有的详细信息。 - 新增的
RunWithoutFile测试只断言了action_details中少数字段的存在；如果下游使用者也依赖命中框（hit box），建议同时校验期望的box字段存在且不是默认值，以防在保留几何信息方面出现回归。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- 在 `Actuator::run` 中，当 `action_id` 为 `MaaInvalidId` 时，后备分支会完全替换整个 `ActionResult`；如果调用方可能在失败结果上填充其他字段，建议只填补缺失的字段（例如 `action_id`、`name`、`action`、`box`、`success`），而不是丢弃已有的详细信息。
- 新增的 `RunWithoutFile` 测试只断言了 `action_details` 中少数字段的存在；如果下游使用者也依赖命中框（hit box），建议同时校验期望的 `box` 字段存在且不是默认值，以防在保留几何信息方面出现回归。
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Task/Component/Actuator.cpp" line_range="139" />
<code_context>
+ result = ActionResult {
+ .action_id = action_id_,
+ .name = pipeline_data.name,
+ .action = action_iter == kTypeNameMap.end() ? std::string() : action_iter->second,
+ .box = reco_hit,
+ .success = false,
</code_context>
<issue_to_address>
**suggestion:** 使用空字符串来表示未知的 action 类型可能会降低可调试性。
当在 `kTypeNameMap` 中找不到该类型时，`action` 会变成空字符串，这会让日志和诊断信息不够直观。建议使用更有意义的后备值（例如 `"<unknown>"` 或 `std::to_string(static_cast<int>(pipeline_data.action_type))`），这样下游使用者仍然可以识别出触发该动作的原始类型。
建议实现：
```cpp
.action = action_iter == kTypeNameMap.end()
? std::string("<unknown:") +
std::to_string(static_cast<int>(pipeline_data.action_type)) + ">"
: action_iter->second,
```
1. 确保在当前翻译单元中已包含 `<string>` 头文件（如果尚未包含），因为现在依赖 `std::string` 和 `std::to_string`。
2. 如果代码库中存在集中式的日志/格式化辅助函数（例如自定义的 `Format` 或类 `fmt` 的工具），为保持与现有约定一致，可能希望使用这些工具而不是手动拼接字符串。
</issue_to_address>
### Comment 2
<location path="test/pipeline/module/RunWithoutFile.cpp" line_range="94-100" />
<code_context>
+
+ MaaTaskerRemoveContextSink(tasker_handle, sink_id);
+
+ if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty()
+ || capture.action != "Click" || capture.success) {
+ std::cout << "Failed to preserve action detail on failed action" << std::endl;
+ return false;
</code_context>
<issue_to_address>
**suggestion (testing):** 加强断言，以验证捕获的 `action_id` 与传入的 `failed_id` 一致。
当前检查只验证了 `capture.action_id` 被设置，而没有验证它是否与 `failed_id` 相匹配。为更好地覆盖这个回归场景，请增加断言 `capture.action_id == failed_id`（并在失败条件与日志信息中体现这一点），这样当捕获到了错误的动作详情时测试也会失败，而不仅仅是在信息缺失时才失败。
```suggestion
MaaTaskerRemoveContextSink(tasker_handle, sink_id);
if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty()
|| capture.action != "Click" || capture.success || capture.action_id != failed_id) {
std::cout << "Failed to preserve or correctly associate action detail on failed action" << std::endl;
return false;
}
```
</issue_to_address>帮我变得更有用！请对每条评论点击 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
Actuator::run, the fallback branch replaces the entireActionResultwhenaction_idisMaaInvalidId; if callers may populate other fields on a failed result, consider only filling missing fields (e.g.,action_id,name,action,box,success) instead of discarding any existing detail. - The new
RunWithoutFiletest only asserts presence of a few fields fromaction_details; if consumers rely on the hit box as well, consider also validating that the expectedboxis present and non-default to catch regressions in preserved geometry.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Actuator::run`, the fallback branch replaces the entire `ActionResult` when `action_id` is `MaaInvalidId`; if callers may populate other fields on a failed result, consider only filling missing fields (e.g., `action_id`, `name`, `action`, `box`, `success`) instead of discarding any existing detail.
- The new `RunWithoutFile` test only asserts presence of a few fields from `action_details`; if consumers rely on the hit box as well, consider also validating that the expected `box` is present and non-default to catch regressions in preserved geometry.
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Task/Component/Actuator.cpp" line_range="139" />
<code_context>
+ result = ActionResult {
+ .action_id = action_id_,
+ .name = pipeline_data.name,
+ .action = action_iter == kTypeNameMap.end() ? std::string() : action_iter->second,
+ .box = reco_hit,
+ .success = false,
</code_context>
<issue_to_address>
**suggestion:** Using an empty string for unknown action types may reduce debuggability.
When the type isn’t found in `kTypeNameMap`, `action` becomes an empty string, which makes logs and diagnostics less informative. Prefer a meaningful fallback (e.g. `"<unknown>"` or `std::to_string(static_cast<int>(pipeline_data.action_type))`) so downstream consumers can still identify the originating action type.
Suggested implementation:
```cpp
.action = action_iter == kTypeNameMap.end()
? std::string("<unknown:") +
std::to_string(static_cast<int>(pipeline_data.action_type)) + ">"
: action_iter->second,
```
1. Ensure `<string>` is included in this translation unit if it is not already, as we now rely on `std::string` and `std::to_string`.
2. If your codebase has a centralized logging/formatting helper (e.g., a custom `Format` or `fmt`-like utility), you may want to use that instead of manual string concatenation, to be consistent with existing conventions.
</issue_to_address>
### Comment 2
<location path="test/pipeline/module/RunWithoutFile.cpp" line_range="94-100" />
<code_context>
+
+ MaaTaskerRemoveContextSink(tasker_handle, sink_id);
+
+ if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty()
+ || capture.action != "Click" || capture.success) {
+ std::cout << "Failed to preserve action detail on failed action" << std::endl;
+ return false;
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the assertion to verify the captured `action_id` matches the posted `failed_id`.
The current check only verifies that `capture.action_id` is set, not that it matches the `failed_id`. To better cover the regression, please assert `capture.action_id == failed_id` (and reflect this in the failure condition / log message) so the test fails if the wrong action detail is captured, not just when it’s missing.
```suggestion
MaaTaskerRemoveContextSink(tasker_handle, sink_id);
if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty()
|| capture.action != "Click" || capture.success || capture.action_id != failed_id) {
std::cout << "Failed to preserve or correctly associate action detail on failed action" << std::endl;
return false;
}
```
</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
+94
to
+100
| MaaTaskerRemoveContextSink(tasker_handle, sink_id); | ||
|
|
||
| if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty() | ||
| || capture.action != "Click" || capture.success) { | ||
| std::cout << "Failed to preserve action detail on failed action" << std::endl; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): 加强断言，以验证捕获的 action_id 与传入的 failed_id 一致。
当前检查只验证了 capture.action_id 被设置，而没有验证它是否与 failed_id 相匹配。为更好地覆盖这个回归场景，请增加断言 capture.action_id == failed_id（并在失败条件与日志信息中体现这一点），这样当捕获到了错误的动作详情时测试也会失败，而不仅仅是在信息缺失时才失败。
Suggested change
| MaaTaskerRemoveContextSink(tasker_handle, sink_id); | |
| if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty() | |
| || capture.action != "Click" || capture.success) { | |
| std::cout << "Failed to preserve action detail on failed action" << std::endl; | |
| return false; | |
| } | |
| MaaTaskerRemoveContextSink(tasker_handle, sink_id); | |
| if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty() | |
| || capture.action != "Click" || capture.success || capture.action_id != failed_id) { | |
| std::cout << "Failed to preserve or correctly associate action detail on failed action" << std::endl; | |
| return false; | |
| } |
Original comment in English
suggestion (testing): Strengthen the assertion to verify the captured action_id matches the posted failed_id.
The current check only verifies that capture.action_id is set, not that it matches the failed_id. To better cover the regression, please assert capture.action_id == failed_id (and reflect this in the failure condition / log message) so the test fails if the wrong action detail is captured, not just when it’s missing.
Suggested change
| MaaTaskerRemoveContextSink(tasker_handle, sink_id); | |
| if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty() | |
| || capture.action != "Click" || capture.success) { | |
| std::cout << "Failed to preserve action detail on failed action" << std::endl; | |
| return false; | |
| } | |
| MaaTaskerRemoveContextSink(tasker_handle, sink_id); | |
| if (failed_id == MaaInvalidId || !capture.seen || capture.action_id == MaaInvalidId || capture.name.empty() | |
| || capture.action != "Click" || capture.success || capture.action_id != failed_id) { | |
| std::cout << "Failed to preserve or correctly associate action detail on failed action" << std::endl; | |
| return false; | |
| } |
Author
|
I checked the failing CI jobs. They seem to fail in the Node binding preparation step before reaching this C++ change:
This PR only changes
Please let me know if you would like me to adjust anything in this PR. |
Author
|
您好，感谢之前的 review 和 approval。 我看到 #1322 现在已经关闭了，这个 PR 目前还处于 open 状态，并且还有一些 CI job 没有通过。我这边看日志感觉剩余失败项仍然不像是这个 C++ 改动引起的，不过想确认一下：这个 PR 目前还需要我这边继续调整什么吗？ 谢谢！ |
Member
|
上面是机器人回复的 啥情况会进来一个 InvalidId? |
Author
|
抱歉，上面 CI 那段我理解偏了。 这里的 一个可复现的场景是 Click 的 target 解析失败：例如我测试里用的 auto target_rect = helper_.get_target_rect(param.target, box);
if (target_rect.empty()) {
LogError << "failed to get target rect" << VAR(name);
return { };
}这个 这个 PR 的处理就是在这种 action 内部已经确认失败、但还没生成完整 |
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
2 participants
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.
Fixes #1258
Summary
This PR fixes missing
action_detailsinNode.Action.Failedcallbacks when an action exits through an early failure path.Previously, some failed actions returned an empty
ActionResult, which leftaction_idasMaaInvalidId. As a result, the failed action detail written to runtime cache and emitted through callbacks could lose useful metadata.Now
Actuator::run()fills a failedActionResultwith the current action id, node name, action type, hit box, andsuccess=falsebefore storing it.Validation
cmake --build build-make --target MaaFramework -j 8cmake --build build-make --target PipelineTesting -j 8./build-make/bin/PipelineTesting ./test/TestingDataSetgit diff --checkSummary by Sourcery
确保失败的操作始终记录并发出详细的失败元数据，而不是返回空结果。
错误修复：
Actuator::run()中填充一个默认的失败ActionResult，以便在运行时缓存和回调中保留失败操作的详细信息。测试：
RunWithoutFilepipeline 测试，发布一个失败的操作，捕获Node.Action.Failed上下文消息，并断言action_id、name、action和success字段被正确填充。Original summary in English
Summary by Sourcery
Ensure failed actions always record and emit detailed failure metadata instead of returning an empty result.
Bug Fixes:
Tests: