Skip to content

Extract shared Lark outbound retry helper (DRY TrySendWithFallbackAsync between SkillRunner and FeishuCardHumanInteractionPort) #414

@eanzhao

Description

@eanzhao

Context

Surfaced as architecture observation #1 in PR #412 review.

Problem

PR #412 added near-identical TrySendWithFallbackAsync + SendOutcome record + SendOutboundAsync in two places:

FeishuCardHumanInteractionPort's implementation has a comment that explicitly says "Mirrors SkillRunnerGAgent.TrySendWithFallbackAsync". Already drifting:

  • Different log strings: Skill runner ... primary delivery target rejected as ... vs Feishu human interaction port primary delivery target rejected as ...
  • SendOutcome record duplicated as a private nested type in each class

This violates CLAUDE.md "删除优先:空转发、重复抽象、无业务价值代码直接删除". Future retry-policy tweaks (additional error codes, backoff, telemetry) require touching both copies and they will keep drifting.

Proposal

Extract a shared retry helper that takes the send delegate plus the primary/fallback target pair:

internal static class LarkOutboundRetryPolicy
{
    public static async Task<SendOutcome> SendWithFallbackAsync(
        Func<LarkReceiveTarget, CancellationToken, Task<string>> sendAsync,
        LarkReceiveTarget primary,
        LarkReceiveTarget? fallback,
        ILogger logger,
        string callerContext,
        CancellationToken ct)
    {
        // shared retry-on-230002 policy here
    }
}

Each callsite passes its own sendAsync delegate (different proxy paths, different headers) and a callerContext string for logs. The retry policy itself lives in one place.

Acceptance:

  • Single implementation of the retry policy (one SendOutcome type, one log line shape, one error-code allowlist).
  • SkillRunnerGAgent.SendOutputAsync and FeishuCardHumanInteractionPort.SendMessageAsync both call the shared helper.
  • Existing tests at both call sites continue to pass without test-side duplication.
  • Adding a new error code to the retry allowlist (e.g. a third Lark bot_kicked variant) requires editing exactly one file.

Out of scope

  • Moving the retry policy into a different DI-resolved abstraction (ILarkOutboundDispatcher) is tracked separately as the third architecture follow-up — that's a deeper boundary change. This issue is the minimum-viable DRY-up.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions