feat(custom):新增 screenshot_on_fail.py节点级截图sink - #14
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿——我发现了 3 个问题
面向 AI Agent 的提示
请处理此次代码审查中的评论：
## 各条评论
### 评论 1
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="130" />
<code_context>
+ if noti_type in (NotificationType.Succeeded, NotificationType.Failed):
</code_context>
<issue_to_address>
**issue (bug_risk)：** sink 会为每种通知类型保存截图，但只有在 `Succeeded` 和 `Failed` 情况下才会获取节点专属的识别数据。当回调收到 `Starting` 时，它会回退使用 `cached_image`，并将生成的文件标记为 `failed`，从而为尚未完成的识别事件生成具有误导性的失败截图。
**触发条件：** 当 `on_node_pipeline_node` 发出其正常的 `Starting` 通知时。
**建议修复：** 除非 `noti_type` 是 `NotificationType.Succeeded` 或 `NotificationType.Failed`，否则直接返回；或者为 `Starting` 处理其自身的状态和图像语义。
```suggestion
if noti_type not in (NotificationType.Succeeded, NotificationType.Failed):
return
node_name = detail.name or "unknown"
```
</issue_to_address>
### 评论 2
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="158-166" />
<code_context>
+ filename = f"{timestamp}_{node_name}_{reco_id}_{status}.jpg"
+ filepath = dirpath / filename
+
+ _save_image(img, filepath)
+ _cleanup_old_screenshots()
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk)：** 图像编码、文件创建和清理过程中的失败没有在回调边界处进行处理。`cv2.imencode`、`open`、`mkdir`、`unlink` 或 `_cleanup_old_screenshots` 可能抛出异常，导致 context sink 回调失败，而不是仅报告调试截图无法保存。
**触发条件：** 调试目录不可写、磁盘已满、截图使用了不受支持的数据类型/形状，或清理操作与其他文件系统变更发生竞争时。
**建议修复：** 使用异常处理包裹保存和清理操作，记录失败，并确保截图诊断不会中断任务执行。
```suggestion
try:
dirpath = Path(_SCREENSHOT_DIR)
dirpath.mkdir(parents=True, exist_ok=True)
timestamp = datetime.now().strftime("%Y.%m.%d-%H.%M.%S.%f")[:-3]
status = "success" if noti_type == NotificationType.Succeeded else "failed"
filename = f"{timestamp}_{node_name}_{reco_id}_{status}.jpg"
filepath = dirpath / filename
_save_image(img, filepath)
_cleanup_old_screenshots()
except Exception:
_log.exception("保存调试截图失败")
```
</issue_to_address>
### 评论 3
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="70" />
<code_context>
+ f.write(encoded.tobytes())
+ return True
+
+ return _save_bmp(img, filepath.with_suffix(".bmp"))
+
+
</code_context>
<issue_to_address>
**nitpick (bug_risk)：** 回退逻辑会写入 `filepath.with_suffix(".bmp")`，而调用方仍继续使用并记录原始的 `.jpg` 路径。因此，文档中描述的输出模式与实际的回退输出不一致；任何期望获得返回的 `.jpg` 文件名的调用方或工具都无法发现生成的 BMP 文件。
**触发条件：** cv2 不可用时。
**建议修复：** 让 `_save_image` 返回实际的输出路径，在写入前构造扩展名，并使 README/文档字符串中的文件名模式与回退行为保持一致。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会利用你的反馈来改进审查结果。
Original comment in English
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="130" />
<code_context>
+ if noti_type in (NotificationType.Succeeded, NotificationType.Failed):
</code_context>
<issue_to_address>
**issue (bug_risk):** The sink saves a screenshot for every notification type, but only retrieves node-specific recognition data for `Succeeded` and `Failed`. When the callback receives `Starting`, it falls back to `cached_image` and labels the resulting file `failed`, producing a misleading failure screenshot for an incomplete recognition event.
**Triggers:** When `on_node_pipeline_node` emits its normal `Starting` notification.
**Suggested fix:** Return unless `noti_type` is `NotificationType.Succeeded` or `NotificationType.Failed`, or handle `Starting` with its own status and image semantics.
```suggestion
if noti_type not in (NotificationType.Succeeded, NotificationType.Failed):
return
node_name = detail.name or "unknown"
```
</issue_to_address>
### Comment 2
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="158-166" />
<code_context>
+ filename = f"{timestamp}_{node_name}_{reco_id}_{status}.jpg"
+ filepath = dirpath / filename
+
+ _save_image(img, filepath)
+ _cleanup_old_screenshots()
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk):** Failures from image encoding, file creation, and cleanup are not handled at the callback boundary. `cv2.imencode`, `open`, `mkdir`, `unlink`, or `_cleanup_old_screenshots` can raise, causing the context sink callback to fail instead of merely reporting that a debug screenshot could not be saved.
**Triggers:** When the debug directory is unwritable, the disk is full, a screenshot has an unsupported dtype/shape, or cleanup races with another filesystem change.
**Suggested fix:** Wrap saving and cleanup in exception handling, log the failure, and ensure screenshot diagnostics cannot interrupt task execution.
```suggestion
try:
dirpath = Path(_SCREENSHOT_DIR)
dirpath.mkdir(parents=True, exist_ok=True)
timestamp = datetime.now().strftime("%Y.%m.%d-%H.%M.%S.%f")[:-3]
status = "success" if noti_type == NotificationType.Succeeded else "failed"
filename = f"{timestamp}_{node_name}_{reco_id}_{status}.jpg"
filepath = dirpath / filename
_save_image(img, filepath)
_cleanup_old_screenshots()
except Exception:
_log.exception("保存调试截图失败")
```
</issue_to_address>
### Comment 3
<location path="Storage/customs/BQ/screenshot-on-fail/screenshot_on_fail.py" line_range="70" />
<code_context>
+ f.write(encoded.tobytes())
+ return True
+
+ return _save_bmp(img, filepath.with_suffix(".bmp"))
+
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The fallback writes to `filepath.with_suffix(".bmp")`, while the caller continues to use and document the original `.jpg` path. The documented output pattern therefore does not match the actual fallback output, and any caller or tooling expecting the returned `.jpg` filename has no way to discover the generated BMP file.
**Triggers:** When cv2 is unavailable.
**Suggested fix:** Return the actual output path from `_save_image`, construct the extension before writing, and keep the README/docstring filename pattern consistent with the fallback behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
解决的问题
在 MaaFramework 开发中，很多开发者会把业务逻辑写在
on_error里，导致 Pipeline 节点真正失败时无法触发默认截图。用户反馈 bug 时，开发者拿不到失败截图，只能靠日志盲猜，排查效率很低。Screenshot On Fail是一个节点级 ContextEventSink，它绕过on_error机制，直接监听每个 Pipeline 节点的识别完成事件——无论成功还是失败都截图，确保开发者永远能拿到完整的运行现场。核心特性
on_error时间戳_节点名_识别ID_状态.jpg，一眼定位问题节点文件结构
maahub_meta.json— 元信息README.md— 详细说明main.py— 入口，导入即注册screenshot_on_fail.py— Sink 核心实现pipeline.json— 空配置（Sink 无需 pipeline）依赖
numpyopencv-python（可选，不可用时回退 BMP）Sourcery 摘要
新增节点级诊断截图接收器，用于记录成功和失败识别的 Pipeline 执行上下文。
新功能：
增强功能：
MDNA_DEBUG_DIR环境变量自定义调试输出目录。文档：
Original summary in English
Summary by Sourcery
Add a node-level diagnostic screenshot sink that records Pipeline execution context for both successful and failed recognitions.
New Features:
Enhancements:
Documentation: