Skip to content

fix: avoid duplicate sends on NapCat timeout - #32

Merged
FlanChanXwO merged 1 commit into
masterfrom
codex/fix-napcat-timeout-duplicates
Jul 9, 2026
Merged

fix: avoid duplicate sends on NapCat timeout#32
FlanChanXwO merged 1 commit into
masterfrom
codex/fix-napcat-timeout-duplicates

Conversation

@FlanChanXwO

@FlanChanXwO FlanChanXwO commented Jul 9, 2026

Copy link
Copy Markdown
Member

Fixes the remaining duplicate-image issue observed on production NapCat/aiocqhttp.

Diagnosis on ssh atri showed the deployed plugin was still v2.1.1, and logs for the failing scenario had count=1 and items=1. The duplicate did not come from provider over-return. Instead, send_group_msg returned retcode 1200 with NTQQ sendMsg timeout wording after raw file:// passthrough, and the plugin treated it as a confirmed failure, triggering normal/stream fallback for the same image.

Modifications / 改动点

  • Treat OneBot/NapCat retcode 1200 as pending delivery only when the platform is OneBot-like and the error text matches the known NTQQ sendMsg timeout markers.

  • Keep unrelated retcode 1200 errors, and the same NapCat wording on non-OneBot platforms, on the normal failure path instead of widening pending delivery to other adapters.

  • Use one _platform_name() helper across direct send and forward send so platform classification is consistent.

  • Apply the uncertain-delivery classification to both direct send and forward send paths.

  • Add regression tests for the positive NapCat timeout cases, non-OneBot direct/forward negative cases, and unrelated OneBot retcode 1200 wording.

  • Document the narrowed NapCat timeout duplicate-send fix in CHANGELOG.md.

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

PYTHONPATH=/tmp:/Users/flanchan/Development/SourceCode/GithubProjects/AstrBot:/Users/flanchan/Development/SourceCode/GithubProjects/astrbot-plugin-dev/data/plugins /Users/flanchan/Development/SourceCode/GithubProjects/astrbot-plugin-dev/.venv/bin/python -m pytest tests/infrastructure/test_image_sender.py -k 'onebot_action_timeout or non_onebot or unrelated_onebot or html_card_only' -q
# 6 passed, 28 deselected

PYTHONPATH=/tmp:/Users/flanchan/Development/SourceCode/GithubProjects/AstrBot:/Users/flanchan/Development/SourceCode/GithubProjects/astrbot-plugin-dev/data/plugins /Users/flanchan/Development/SourceCode/GithubProjects/astrbot-plugin-dev/.venv/bin/python -m pytest -q
# 190 passed in 0.62s

pre-commit run --files tests/infrastructure/test_image_sender.py
# Passed

Checklist / 检查清单

  • 😊 If there are new features added in the PR, I have discussed them with the authors through issues/emails, etc.
    / 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
    No new feature is added; this is a bugfix.

  • 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
    / 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”

  • 🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txt 文件相应位置。

  • 😮 My changes do not introduce malicious code.
    / 我的更改没有引入恶意代码。

Copilot AI review requested due to automatic review settings July 9, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sourcery-ai

sourcery-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

将特定的 OneBot/NapCat 超时错误在直接发送和转发发送场景中都归类为“不确定投递”而不是“硬失败”,为 NapCat 动作超时添加回归测试以防止重复发送图片,并在变更日志中记录该修复。

直接发送场景下 OneBot/NapCat 超时处理的时序图

sequenceDiagram
    participant ImageSender
    participant SendStrategy
    participant NapCatAdapter
    participant SendAttemptResult

    ImageSender->>SendStrategy: send_with_status(event, chain)
    SendStrategy->>NapCatAdapter: send_group_msg
    NapCatAdapter-->>SendStrategy: Exception retcode_1200
    SendStrategy->>SendStrategy: _is_onebot_uncertain_delivery_error(exc)
    SendStrategy->>SendAttemptResult: pending_delivery("onebot send confirmation timed out")
    SendStrategy-->>ImageSender: SendAttemptResult
Loading

转发发送场景下 OneBot/NapCat 超时处理的时序图

sequenceDiagram
    participant ImageSender
    participant SendStrategy
    participant NapCatAdapter
    participant SendAttemptResult

    ImageSender->>SendStrategy: _send_nodes_direct_with_status(event, nodes)
    SendStrategy->>NapCatAdapter: send_group_forward_msg
    NapCatAdapter-->>SendStrategy: Exception retcode_1200
    SendStrategy->>SendStrategy: _is_onebot_uncertain_delivery_error(exc)
    SendStrategy->>SendAttemptResult: pending_delivery("forward onebot confirmation timed out")
    SendStrategy-->>ImageSender: SendAttemptResult
Loading

File-Level Changes

Change Details Files
将 OneBot/NapCat retcode 1200 超时错误分类为“等待投递”,以避免重复发送。
  • 引入辅助方法,根据 retcode 和与超时相关的文案检测 OneBot/NapCat 的不确定投递错误。
  • 在直接发送的错误处理逻辑中使用该辅助方法,在出现此类错误时返回“等待投递”状态,而不是触发回退逻辑。
  • 在转发发送的错误处理逻辑中使用该辅助方法,使转发消息的超时同样被视为“等待投递”。
src/infrastructure/sending/send_strategies.py
添加回归测试,确保 NapCat 原始动作超时被视为“等待投递”,不会触发回退或重新发送。
  • 定义一个类似 NapCat 的 ActionTimeout 异常,包含 retcode 1200 和 NTQQ sendMsg 超时相关的文案。
  • 配置一个 OneBot/NapCat 传输场景,使 send_group_msg 在本地文件图片发送时抛出 ActionTimeout。
  • 断言图片发送结果为“等待投递”,send_group_msg 仅被调用一次,并且不会触发任何流式或上下文的回退路径。
tests/infrastructure/test_image_sender.py
记录 NapCat 超时导致重复发送问题的修复。
  • 在变更日志中添加条目,描述针对 OneBot/NapCat retcode 1200 的 send_group_msg 超时的新处理方式及其对重复发送问题的影响。
CHANGELOG.md

Tips and commands

Interacting with Sourcery

  • 触发新的审查: 在 Pull Request 中评论 @sourcery-ai review
  • 继续讨论: 直接回复 Sourcery 的审查评论。
  • 从审查评论生成 GitHub Issue: 回复 Sourcery 的审查评论,请求从该评论创建一个 issue;也可以在审查评论下回复 @sourcery-ai issue 来创建对应的 issue。
  • 生成 Pull Request 标题: 在 Pull Request 标题的任意位置写上 @sourcery-ai,即可随时生成标题。也可以在 Pull Request 中评论 @sourcery-ai title 来(重新)生成标题。
  • 生成 Pull Request 摘要: 在 Pull Request 描述正文任意位置写上 @sourcery-ai summary,即可在指定位置生成 PR 摘要。也可以在 Pull Request 中评论 @sourcery-ai summary 来在任意时间(重新)生成摘要。
  • 生成审查者指南: 在 Pull Request 中评论 @sourcery-ai guide,即可在任意时间(重新)生成审查者指南。
  • 解决所有 Sourcery 评论: 在 Pull Request 中评论 @sourcery-ai resolve,即可将所有 Sourcery 评论标记为已解决。如果你已经处理完所有评论且不希望再看到它们,这会很有用。
  • 关闭所有 Sourcery 审查: 在 Pull Request 中评论 @sourcery-ai dismiss,即可关闭所有现有的 Sourcery 审查。尤其在你希望重新开始一次新的审查时很有用——别忘了再评论 @sourcery-ai review 来触发新的审查!

Customizing Your Experience

访问你的 dashboard 以:

  • 启用或停用审查功能,比如 Sourcery 生成的 Pull Request 摘要、审查者指南等。
  • 更改审查语言。
  • 添加、删除或编辑自定义审查说明。
  • 调整其他审查设置。

Getting Help

Original review guide in English

Reviewer's Guide

Classifies specific OneBot/NapCat timeout errors as uncertain delivery instead of hard failures for both direct and forward sends, adds a regression test to prevent duplicate image sends on NapCat action timeouts, and documents the fix in the changelog.

Sequence diagram for direct send OneBot/NapCat timeout handling

sequenceDiagram
    participant ImageSender
    participant SendStrategy
    participant NapCatAdapter
    participant SendAttemptResult

    ImageSender->>SendStrategy: send_with_status(event, chain)
    SendStrategy->>NapCatAdapter: send_group_msg
    NapCatAdapter-->>SendStrategy: Exception retcode_1200
    SendStrategy->>SendStrategy: _is_onebot_uncertain_delivery_error(exc)
    SendStrategy->>SendAttemptResult: pending_delivery("onebot send confirmation timed out")
    SendStrategy-->>ImageSender: SendAttemptResult
Loading

Sequence diagram for forward send OneBot/NapCat timeout handling

sequenceDiagram
    participant ImageSender
    participant SendStrategy
    participant NapCatAdapter
    participant SendAttemptResult

    ImageSender->>SendStrategy: _send_nodes_direct_with_status(event, nodes)
    SendStrategy->>NapCatAdapter: send_group_forward_msg
    NapCatAdapter-->>SendStrategy: Exception retcode_1200
    SendStrategy->>SendStrategy: _is_onebot_uncertain_delivery_error(exc)
    SendStrategy->>SendAttemptResult: pending_delivery("forward onebot confirmation timed out")
    SendStrategy-->>ImageSender: SendAttemptResult
Loading

File-Level Changes

Change Details Files
Classify OneBot/NapCat retcode 1200 timeout errors as pending delivery to avoid duplicate sends.
  • Introduce helper to detect OneBot/NapCat uncertain delivery errors based on retcode and timeout-related wording.
  • Use the helper in direct send error handling to return a pending-delivery status instead of triggering fallback when such errors occur.
  • Use the helper in forward-send error handling so forwarded message timeouts are also treated as pending delivery.
src/infrastructure/sending/send_strategies.py
Add regression test ensuring NapCat raw action timeouts are treated as pending and do not trigger fallback or resends.
  • Define a NapCat-like ActionTimeout exception with retcode 1200 and NTQQ sendMsg timeout wording.
  • Configure a OneBot/NapCat transport scenario where send_group_msg raises ActionTimeout during local-file image send.
  • Assert that the image send result is pending delivery, that send_group_msg is called once, and no stream or context fallback paths are invoked.
tests/infrastructure/test_image_sender.py
Document the NapCat timeout duplicate-send fix.
  • Add a changelog entry describing the new handling of OneBot/NapCat retcode 1200 send_group_msg timeouts and its effect on duplicate sends.
CHANGELOG.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

针对 OneBot/NapCat 平台,新增统一平台名提取函数与超时错误识别逻辑,使 send_group_msg 返回 retcode=1200 且匹配已知 NTQQ sendMsg 超时标记的场景被判定为待确认投递(pending_delivery),而非触发失败后续发送或降级流程,并补充相应测试与变更日志。

Changes

NapCat 发送确认超时去重

Layer / File(s) Summary
平台名与超时识别辅助函数
src/infrastructure/sending/send_strategies.py
新增 _platform_name 统一提取平台名、_is_onebot_uncertain_delivery_error 识别 OneBot/NapCat 超时错误,以及相关常量。
发送策略异常处理调整
src/infrastructure/sending/send_strategies.py
DirectSendStrategy.send_with_status_requires_onebot_passthroughForwardSendStrategy._send_nodes_direct_with_status 改用统一平台名,命中已知超时标记时返回 pending_delivery 而非失败。
测试用例与变更记录
tests/infrastructure/test_image_sender.py, CHANGELOG.md
新增模拟超时异常类与多组异步测试验证 pending 判定,并在变更日志中记录该修复。

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • FlanChanXwO/astrbot_plugin_setu#28: 本 PR 对 send_strategies.py 的改动直接建立在该 PR 的 ImageSender/发送策略重构以及超时/待确认投递测试覆盖之上。

Poem

兔子敲代码,耳朵竖得高,
retcode 1200,别再慌张跳,
超时不是败,只是慢慢到,
pending 一小会,图片不会重复冒~ 🐰📮

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题简洁且准确概括了核心修改:处理 NapCat 超时以避免重复发送。
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - 我发现了 1 个问题,并留下了一些整体反馈:

  • 建议收紧或更清晰地结构化 _is_onebot_uncertain_delivery_error 的启发式逻辑(例如:匹配已知的子串模式或常量),以避免未来文案调整时不小心把不相关的错误也归类为“待投递”。
  • 当前平台名的提取逻辑在“直接发送”和“转发发送”之间略有不一致;如果能统一到同一个辅助函数或模式,会让整体行为更容易理解和维护。
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider tightening or structuring the `_is_onebot_uncertain_delivery_error` heuristics (e.g., matching against a known substring pattern or constants) so future wording changes don’t accidentally classify unrelated errors as pending delivery.
- The platform name extraction logic is slightly inconsistent between direct send and forward send; aligning on a single helper or pattern would make the behavior easier to reason about and maintain.

## Individual Comments

### Comment 1
<location path="tests/infrastructure/test_image_sender.py" line_range="214-190" />
<code_context>
+    mock_event.platform.name = "aiocqhttp"
</code_context>
<issue_to_address>
**suggestion (testing):** 考虑增加一个“负向测试”,验证非 OneBot 类的平台不会把相同的异常视为待投递

当前测试已经通过 `mock_event.platform.name = "aiocqhttp"` 覆盖了 OneBot 类平台的路径,请再补充一个使用非 OneBot 类平台名(例如你现有适配器栈里的另一个平台)的测试,其中同样的 `ActionTimeout` 会导致正常失败,而不是被标记为 `pending_delivery`。这样可以验证基于平台的分类逻辑,并防止未来的回归不小心把 OneBot/NapCat 的行为扩展到更广的平台范围。

建议的实现方式:

```python
    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # OneBot-like platform: aiocqhttp should treat ActionTimeout as pending_delivery
    mock_event.platform.name = "aiocqhttp"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = with_napcat_transport(
        without_delivery_notices(sample_config_dict),
        local_file_mode="always",
        local_file_allowed_roots=[str(tmp_path / "shared")],
    )



async def test_image_sender_action_timeout_on_non_onebot_platform_is_failure(
    tmp_path: Path,
    sample_config_dict: dict[str, Any],
    mock_event: MagicMock,
) -> None:
    """
    Non-OneBot-like platforms (e.g. telegram) should not classify ActionTimeout
    as pending delivery; instead the send should fail normally.
    """

    # Arrange
    message = (
        "Timeout: NTEvent serviceAndMethod:NodeIKernelMsgService/sendMsg "
        "ListenerName:NodeIKernelMsgListener/onMsgInfoListUpdate EventRet:\n{}\n"
    )
    wording = message

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # Use a non-OneBot-like platform name from the existing adapter stack
    mock_event.platform.name = "telegram"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = without_delivery_notices(
        sample_config_dict,
    )

    # Act
    image_sender = ImageSender(config_dict)
    with pytest.raises(ActionTimeout):
        await image_sender.send_image(mock_event, wording)

    # Assert
    # The failure should be a normal timeout; nothing should be marked as pending delivery
    assert not getattr(mock_event, "pending_delivery", False)

```

1. 确认 `ImageSender``send_image` 以及 `pending_delivery` 属性(或同等标记)的名字与该测试模块内实际使用的保持一致;如果你的代码使用了不同的发送类、方法或标记字段来表示“待投递”,请相应调整。
2. 如果现有的 OneBot/NapCat 测试目前对“待投递”有特定的结构断言(例如:返回结果对象中带 `pending_delivery` 字段,或使用专门的状态枚举),请在这里镜像同样的结构,并断言平台 `"telegram"` 会产生正常失败,而不是该待投递状态。
3.`"telegram"` 替换为你当前适配器栈中已经存在的其他非 OneBot 适配器名称(例如 `"kook"``"discord"` 等),以保持测试与实际支持的平台一致。
4. 如果非 OneBot 平台的“正常失败路径”是以不同方式暴露的(比如返回结果对象,而不是直接抛出 `ActionTimeout`),那就需要调整 `with pytest.raises(ActionTimeout)` 这段代码,使之匹配你的图片发送器实际暴露失败的方式,并断言它不会被标记为待投递。
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据这些反馈改进后续的评审。
Original comment in English

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

  • Consider tightening or structuring the _is_onebot_uncertain_delivery_error heuristics (e.g., matching against a known substring pattern or constants) so future wording changes don’t accidentally classify unrelated errors as pending delivery.
  • The platform name extraction logic is slightly inconsistent between direct send and forward send; aligning on a single helper or pattern would make the behavior easier to reason about and maintain.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider tightening or structuring the `_is_onebot_uncertain_delivery_error` heuristics (e.g., matching against a known substring pattern or constants) so future wording changes don’t accidentally classify unrelated errors as pending delivery.
- The platform name extraction logic is slightly inconsistent between direct send and forward send; aligning on a single helper or pattern would make the behavior easier to reason about and maintain.

## Individual Comments

### Comment 1
<location path="tests/infrastructure/test_image_sender.py" line_range="214-190" />
<code_context>
+    mock_event.platform.name = "aiocqhttp"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a negative test showing non-OneBot-like platforms do not treat similar exceptions as pending delivery

Since this test covers the OneBot-like path via `mock_event.platform.name = "aiocqhttp"`, please also add a complementary test using a non-OneBot-like platform name (e.g., another adapter in your stack) where the same `ActionTimeout` results in a normal failure rather than `pending_delivery`. This will verify the platform-based classification and help prevent regressions that accidentally extend the OneBot/NapCat behavior more broadly.

Suggested implementation:

```python
    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # OneBot-like platform: aiocqhttp should treat ActionTimeout as pending_delivery
    mock_event.platform.name = "aiocqhttp"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = with_napcat_transport(
        without_delivery_notices(sample_config_dict),
        local_file_mode="always",
        local_file_allowed_roots=[str(tmp_path / "shared")],
    )



async def test_image_sender_action_timeout_on_non_onebot_platform_is_failure(
    tmp_path: Path,
    sample_config_dict: dict[str, Any],
    mock_event: MagicMock,
) -> None:
    """
    Non-OneBot-like platforms (e.g. telegram) should not classify ActionTimeout
    as pending delivery; instead the send should fail normally.
    """

    # Arrange
    message = (
        "Timeout: NTEvent serviceAndMethod:NodeIKernelMsgService/sendMsg "
        "ListenerName:NodeIKernelMsgListener/onMsgInfoListUpdate EventRet:\n{}\n"
    )
    wording = message

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # Use a non-OneBot-like platform name from the existing adapter stack
    mock_event.platform.name = "telegram"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = without_delivery_notices(
        sample_config_dict,
    )

    # Act
    image_sender = ImageSender(config_dict)
    with pytest.raises(ActionTimeout):
        await image_sender.send_image(mock_event, wording)

    # Assert
    # The failure should be a normal timeout; nothing should be marked as pending delivery
    assert not getattr(mock_event, "pending_delivery", False)

```

1. Ensure the names `ImageSender`, `send_image`, and the `pending_delivery` attribute (or equivalent) match what is actually used in this test module; adjust them if your code uses a different sender class, method, or flag to indicate pending delivery.
2. If the existing OneBot/NapCat test currently asserts a specific structure for the "pending delivery" classification (e.g. a result object with a `pending_delivery` field or a dedicated status enum), mirror that structure here and assert that the platform `"telegram"` produces a normal failure instead of that pending-delivery classification.
3. Replace `"telegram"` with another non-OneBot adapter name already present in your stack (e.g. `"kook"`, `"discord"`, etc.) to keep the test aligned with the actual platforms supported by your infrastructure.
4. If the normal failure path for non-OneBot platforms is surfaced differently (for example, returning a result object instead of raising `ActionTimeout`), adapt the `with pytest.raises(ActionTimeout)` block to match the way your image sender exposes failures and then assert that it is not marked as pending delivery.
</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.

@@ -190,6 +190,60 @@ async def test_send_images_treats_napcat_none_ack_as_pending(
mock_event.bot.call_action.assert_not_called()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): 考虑增加一个“负向测试”,验证非 OneBot 类的平台不会把相同的异常视为待投递

当前测试已经通过 mock_event.platform.name = "aiocqhttp" 覆盖了 OneBot 类平台的路径,请再补充一个使用非 OneBot 类平台名(例如你现有适配器栈里的另一个平台)的测试,其中同样的 ActionTimeout 会导致正常失败,而不是被标记为 pending_delivery。这样可以验证基于平台的分类逻辑,并防止未来的回归不小心把 OneBot/NapCat 的行为扩展到更广的平台范围。

建议的实现方式:

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # OneBot-like platform: aiocqhttp should treat ActionTimeout as pending_delivery
    mock_event.platform.name = "aiocqhttp"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = with_napcat_transport(
        without_delivery_notices(sample_config_dict),
        local_file_mode="always",
        local_file_allowed_roots=[str(tmp_path / "shared")],
    )



async def test_image_sender_action_timeout_on_non_onebot_platform_is_failure(
    tmp_path: Path,
    sample_config_dict: dict[str, Any],
    mock_event: MagicMock,
) -> None:
    """
    Non-OneBot-like platforms (e.g. telegram) should not classify ActionTimeout
    as pending delivery; instead the send should fail normally.
    """

    # Arrange
    message = (
        "Timeout: NTEvent serviceAndMethod:NodeIKernelMsgService/sendMsg "
        "ListenerName:NodeIKernelMsgListener/onMsgInfoListUpdate EventRet:\n{}\n"
    )
    wording = message

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # Use a non-OneBot-like platform name from the existing adapter stack
    mock_event.platform.name = "telegram"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = without_delivery_notices(
        sample_config_dict,
    )

    # Act
    image_sender = ImageSender(config_dict)
    with pytest.raises(ActionTimeout):
        await image_sender.send_image(mock_event, wording)

    # Assert
    # The failure should be a normal timeout; nothing should be marked as pending delivery
    assert not getattr(mock_event, "pending_delivery", False)
  1. 确认 ImageSendersend_image 以及 pending_delivery 属性(或同等标记)的名字与该测试模块内实际使用的保持一致;如果你的代码使用了不同的发送类、方法或标记字段来表示“待投递”,请相应调整。
  2. 如果现有的 OneBot/NapCat 测试目前对“待投递”有特定的结构断言(例如:返回结果对象中带 pending_delivery 字段,或使用专门的状态枚举),请在这里镜像同样的结构,并断言平台 "telegram" 会产生正常失败,而不是该待投递状态。
  3. "telegram" 替换为你当前适配器栈中已经存在的其他非 OneBot 适配器名称(例如 "kook""discord" 等),以保持测试与实际支持的平台一致。
  4. 如果非 OneBot 平台的“正常失败路径”是以不同方式暴露的(比如返回结果对象,而不是直接抛出 ActionTimeout),那就需要调整 with pytest.raises(ActionTimeout) 这段代码,使之匹配你的图片发送器实际暴露失败的方式,并断言它不会被标记为待投递。
Original comment in English

suggestion (testing): Consider a negative test showing non-OneBot-like platforms do not treat similar exceptions as pending delivery

Since this test covers the OneBot-like path via mock_event.platform.name = "aiocqhttp", please also add a complementary test using a non-OneBot-like platform name (e.g., another adapter in your stack) where the same ActionTimeout results in a normal failure rather than pending_delivery. This will verify the platform-based classification and help prevent regressions that accidentally extend the OneBot/NapCat behavior more broadly.

Suggested implementation:

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # OneBot-like platform: aiocqhttp should treat ActionTimeout as pending_delivery
    mock_event.platform.name = "aiocqhttp"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = with_napcat_transport(
        without_delivery_notices(sample_config_dict),
        local_file_mode="always",
        local_file_allowed_roots=[str(tmp_path / "shared")],
    )



async def test_image_sender_action_timeout_on_non_onebot_platform_is_failure(
    tmp_path: Path,
    sample_config_dict: dict[str, Any],
    mock_event: MagicMock,
) -> None:
    """
    Non-OneBot-like platforms (e.g. telegram) should not classify ActionTimeout
    as pending delivery; instead the send should fail normally.
    """

    # Arrange
    message = (
        "Timeout: NTEvent serviceAndMethod:NodeIKernelMsgService/sendMsg "
        "ListenerName:NodeIKernelMsgListener/onMsgInfoListUpdate EventRet:\n{}\n"
    )
    wording = message

    context = MagicMock()
    context.send_message = AsyncMock()
    set_plugin_context(context)

    # Use a non-OneBot-like platform name from the existing adapter stack
    mock_event.platform.name = "telegram"
    mock_event.get_group_id.return_value = "123456"
    mock_event.get_sender_id.return_value = "654321"
    mock_event.bot = MagicMock()
    mock_event.bot.send_group_msg = AsyncMock(side_effect=ActionTimeout())
    mock_event.bot.call_action = AsyncMock()

    config_dict = without_delivery_notices(
        sample_config_dict,
    )

    # Act
    image_sender = ImageSender(config_dict)
    with pytest.raises(ActionTimeout):
        await image_sender.send_image(mock_event, wording)

    # Assert
    # The failure should be a normal timeout; nothing should be marked as pending delivery
    assert not getattr(mock_event, "pending_delivery", False)
  1. Ensure the names ImageSender, send_image, and the pending_delivery attribute (or equivalent) match what is actually used in this test module; adjust them if your code uses a different sender class, method, or flag to indicate pending delivery.
  2. If the existing OneBot/NapCat test currently asserts a specific structure for the "pending delivery" classification (e.g. a result object with a pending_delivery field or a dedicated status enum), mirror that structure here and assert that the platform "telegram" produces a normal failure instead of that pending-delivery classification.
  3. Replace "telegram" with another non-OneBot adapter name already present in your stack (e.g. "kook", "discord", etc.) to keep the test aligned with the actual platforms supported by your infrastructure.
  4. If the normal failure path for non-OneBot platforms is surfaced differently (for example, returning a result object instead of raising ActionTimeout), adapt the with pytest.raises(ActionTimeout) block to match the way your image sender exposes failures and then assert that it is not marked as pending delivery.

Copilot AI review requested due to automatic review settings July 9, 2026 13:34
@FlanChanXwO
FlanChanXwO force-pushed the codex/fix-napcat-timeout-duplicates branch from b9ebdb0 to fc705cc Compare July 9, 2026 13:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/infrastructure/test_image_sender.py (1)

275-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

建议补充 ForwardSendStrategy 在 OneBot 平台的正向 pending 测试。

当前测试覆盖了 DirectSendStrategy 的正向场景(test_send_images_treats_onebot_action_timeout_as_pending)和两种策略的负向场景,但缺少 ForwardSendStrategy 在 OneBot 平台上将 OneBotActionTimeout 判定为 pending 的正向用例。由于两条路径的异常处理是独立复制的而非共享,正向测试可以确保转发路径的 pending 判定不会回归。

💡 建议的测试用例
+@pytest.mark.asyncio
+async def test_forward_send_strategy_treats_onebot_action_timeout_as_pending(
+    mock_event,
+) -> None:
+    """合并转发在 OneBot 平台将 NapCat/NTQQ timeout 归类为待确认投递。"""
+    context = MagicMock()
+    context.send_message = AsyncMock(side_effect=OneBotActionTimeout())
+    strategy = ForwardSendStrategy(context)
+
+    mock_event.platform.name = "aiocqhttp"
+    mock_event.get_self_id.return_value = "10000"
+
+    result = await strategy.send_with_status(
+        mock_event,
+        [Comp.Image.fromBytes(b"image-data")],
+    )
+
+    assert result.accepted is True
+    assert result.pending is True
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/infrastructure/test_image_sender.py` around lines 275 - 292, The
ForwardSendStrategy coverage is missing the positive OneBot pending case for
OneBotActionTimeout, so add a test alongside
test_forward_send_strategy_keeps_action_timeout_failure_on_non_onebot that uses
a OneBot-like platform and verifies send_with_status returns pending=True and
accepted=True behavior consistent with
test_send_images_treats_onebot_action_timeout_as_pending. Use
ForwardSendStrategy, send_with_status, and OneBotActionTimeout to locate the
path and keep the non-OneBot negative test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/infrastructure/test_image_sender.py`:
- Around line 275-292: The ForwardSendStrategy coverage is missing the positive
OneBot pending case for OneBotActionTimeout, so add a test alongside
test_forward_send_strategy_keeps_action_timeout_failure_on_non_onebot that uses
a OneBot-like platform and verifies send_with_status returns pending=True and
accepted=True behavior consistent with
test_send_images_treats_onebot_action_timeout_as_pending. Use
ForwardSendStrategy, send_with_status, and OneBotActionTimeout to locate the
path and keep the non-OneBot negative test unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c907ddf7-6b0b-4745-bc37-7cf5e6cc7d49

📥 Commits

Reviewing files that changed from the base of the PR and between bc41a8f and fc705cc.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • src/infrastructure/sending/send_strategies.py
  • tests/infrastructure/test_image_sender.py

Copilot AI review requested due to automatic review settings July 9, 2026 13:43
@FlanChanXwO
FlanChanXwO force-pushed the codex/fix-napcat-timeout-duplicates branch from fc705cc to 9933daa Compare July 9, 2026 13:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@FlanChanXwO
FlanChanXwO merged commit 57e2667 into master Jul 9, 2026
3 checks passed
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