Skip to content

feat(dingtalk): DingTalk channel (Stream mode + AI Card streaming)#1429

Open
connermo wants to merge 16 commits into
nextlevelbuilder:devfrom
connermo:feat/dingtalk-channel
Open

feat(dingtalk): DingTalk channel (Stream mode + AI Card streaming)#1429
connermo wants to merge 16 commits into
nextlevelbuilder:devfrom
connermo:feat/dingtalk-channel

Conversation

@connermo

Copy link
Copy Markdown

Overview

Adds dingtalk as a first-class GoClaw channel type. Inbound via DingTalk Stream mode (WebSocket — no public IP, callback domain, or ICP filing required), outbound via AI Card streaming in both groups and DMs with plain text / markdown fallback.

Ported from the official dingtalk-openclaw-connector (TypeScript, for OpenClaw) — used purely as a protocol spec, since its config schema is already ~1:1 with GoClaw's channel config (both descend from OpenClaw). Transport uses the official github.com/open-dingtalk/dingtalk-stream-sdk-go (diverging from Feishu's hand-rolled WS codec, since DingTalk ships a maintained Go SDK).

Plan: plans/260709-2230-dingtalk-channel/plan.md — all 7 phases complete, with an adversarial audit pass (reports/).

What's included

  • Phase 1–2: Channel type registration + factory, Stream transport, dual-host API client
  • Phase 3: Inbound path with ack-then-process
  • Phase 4–5: Outbound text/markdown, inbound + outbound media
  • Phase 6: AI Card streaming (posts on first token, configurable repaint interval, thinking reaction on the user's message)
  • Phase 7: Cross-surface parity (docs, web UI channel schema/config) + live verification
  • Slash commands: /new, /stop, /stopall with a fail-closed group gate
  • Pins the Stream SDK past a process-killing panic in v0.9.1

Surface parity

  • Gateway: new internal/channels/dingtalk/ package, registered in factory + capabilities
  • Web UI: channel schema, constants, config bindings updated (ui/web/src/pages/channels/, constants/channels.ts)
  • Docs: docs/05-channels-messaging.md updated with DingTalk setup

Testing

Extensive unit coverage across the package (card, client, commands, config, emotion, factory, inbound, media, outbound, parse). Live-verified against a real DingTalk app (see phase-07 report).

Conner Mo added 16 commits July 12, 2026 16:58
First phase of porting DingTalk's official OpenClaw connector (TypeScript)
to a native Go channel. This lands registration and config resolution; the
Stream-mode transport, inbound path, and AI Card streaming follow.

Registration comes before behavior on purpose. isValidChannelType is
hand-copied into internal/http and internal/gateway/methods, and its own doc
comment records that the twins have drifted three times (facebook, pancake,
bitrix24) — when they do, the WS-driven UI rejects channels the HTTP API
accepts. Both twins change here, guarded by a test in each package that
asserts every channels.Type* constant is accepted. A dingtalk instance is
inert until the transport lands, so enabling it early is safe.

DingTalk is DB-instance-only, like bitrix24 and unlike feishu: credentials
live in channel_instances.credentials (encrypted by internal/crypto), so
internal/config is untouched. channel_type is free-form VARCHAR(50) with no
CHECK or enum in either migration system, so no migration is needed.

Config field names are snake_case to match every other GoClaw channel rather
than the connector's camelCase; the semantics are identical. The connector's
accounts{} multi-account map is dropped — one instance carries one DingTalk
app, which is what channel_instances already models.

Both policies default to "pairing" and require_mention defaults to true: an
instance is reachable by anyone who finds the bot, so it must be secure by
default. Enum fields are validated at factory time so a typo in
group_reply_mode fails when the operator saves the instance, not hours later
when the first group message silently takes the wrong branch.

Plan and red-team audit: plans/260709-2230-dingtalk-channel/
Adds the two seams the rest of the channel builds on: the REST client and
the Stream-mode WebSocket transport. Start() now dials for real; inbound
messages are acked and dropped until phase 3.

Uses the official dingtalk-stream-sdk-go rather than hand-rolling the socket
as feishu does. Feishu hand-rolls because Lark ships no Go SDK. DingTalk does,
and it already subscribes to the system "disconnect" and "ping" topics, runs a
ping/pong watchdog on a 120s idle timer, and auto-reconnects — all by default
(client/client.go:49,53,54,56). The upstream TypeScript connector disables its
Node SDK's keepAlive and reconnect and hand-rolls a 10s/20s heartbeat plus a
console.info monkey-patch; those are workarounds for Node-SDK defects the Go
SDK does not have. Porting them would put two heartbeats on one socket, so
they are deliberately absent.

The transport sits behind a streamTransport interface so phase 3 can drive the
inbound pipeline from a fake without opening a connection. WithAutoReconnect
and WithKeepAlive are not passed explicitly: they are already the SDK defaults,
and passing them would imply otherwise.

A dial failure is returned rather than retried. The manager records it as a
start failure and keeps booting other channels, so a bad AppKey shows up on the
dashboard instead of hiding behind an infinite retry loop. The failure kind is
Unknown on purpose: the SDK negotiates the socket endpoint using the app
credentials, so a wrong AppKey and an unreachable network surface identically,
and claiming either would mislead half the time.

The client carries two independent token caches because DingTalk splits its
surface across two hosts with two auth mechanisms: api.dingtalk.com takes a
header, oapi.dingtalk.com (media upload only) takes a query parameter. The
legacy host answers HTTP 200 for failures, so doOAPI checks errcode before
decoding into the caller's struct — otherwise a caller reading only its own
fields would see a zero value and carry on. The token mutex is held across the
fetch so fifty goroutines finding an expired token produce one request, which
is pinned by a -race test.
Parses DingTalk callbacks, gates them through the shared policy engine, and
publishes to the bus. The channel now answers.

Returning from the SDK's chatbot handler IS the ack DingTalk waits for, so the
handler must not block. An earlier draft of the plan assumed bus.PublishInbound
was non-blocking; it is a bare channel send on a 1000-deep buffer
(internal/bus/bus.go:36). A saturated bus would have stalled the socket's read
loop and blown the ack deadline. So the handler dedups synchronously — cheap,
and it must precede the spawn or a server resend forks a second agent run —
then hands the rest to a goroutine and returns. A test fills the bus and
asserts the handler still returns promptly.

That goroutine runs on the channel-scoped context captured at Start, not on the
callback's ctx, which dies with the frame.

Mention gating reads the SDK's IsInAtList. The upstream connector has no
mention gate at all: its requireMention is declared in the schema and never
read, because in Stream mode the platform only delivers group messages where
the bot was @'d. GoClaw's require_mention is a user-facing promise, and the SDK
hands us the flag, so we enforce it rather than inherit the assumption.

Payload shapes that silently drop messages if gotten wrong, each pinned by a
test: markdown carries its body in text.content rather than content; content
arrives as a JSON object on some msgtypes and as a JSON string on others;
richText has an old and a new item-list shape. Voice notes carry DingTalk's own
transcription in content.recognition, so unlike Feishu we do not run STT.

Attachments are parsed into mediaRefs but not yet fetched — a text-less message
is dropped with a log rather than published as an empty turn. Phase 5 redeems
the download codes.

Pairing replies need a send path, and a policy gate whose user-visible notice
never arrives is a silent failure, so a minimal session-webhook text reply
lands here. The full outbound path is phase 4.
Send() chunks the reply and delivers each chunk over whichever transport can
carry it: the inbound message's session webhook while it lives, the proactive
robot OpenAPI otherwise.

The session webhook is pre-signed and needs no token, but it is scoped to one
inbound message and expires. An agent run routinely outlives it, so Send checks
the stamped expiry and falls back. It also falls back when the webhook rejects
a live-looking request — revoked webhooks and archived conversations are not
predictable from the timestamp, and one token fetch is cheaper than a dropped
reply.

Metadata does not flow freely from inbound to outbound: channels.routingMetaKeys
is an explicit allowlist, so the session webhook, its expiry, the conversation
id, and the chat type are added there. Without that, Send would never see them.
They carry a dingtalk_ prefix, following feishu_reply_target_id, rather than
colliding with the shared sender_id / chat_type conventions.

The conversation id travels separately from the ChatID because
group_session_scope=group_sender suffixes the ChatID with the sender; the bare
conversation id cannot be recovered from it, and a proactive group send needs
the bare one.

group_reply_mode governs groups only. A DM keeps markdown rendering (and, once
phase 6 lands, the streaming card) no matter how groups are configured. Reading
the connector as "text mode turns cards off everywhere" is the easy misreading,
so the asymmetry has its own truth-table test.

msgParam is a JSON-encoded string rather than a nested object, and a 200 with no
processQueryKey means DingTalk queued nothing — returning nil there would report
a delivered message that never arrives. Both are pinned by tests.
Attachments now round-trip. Inbound: redeem the downloadCode for a signed OSS
URL, fetch, cap, write to a temp file with its extension preserved so the
agent's MIME dispatch keeps working. Outbound: upload, then post the media id
through the proactive API.

The download request must carry no Content-Type header. OSS signs over the
request headers, and one the signer did not include makes the signature
mismatch — the download fails with an opaque 403. Go does not add a
Content-Type to a bodyless GET today, but a shared Transport or an interceptor
could, so a stub server rejects any request carrying one.

Attachments are resolved before the mention gate, matching feishu: a file
dropped into a group without an @ still reaches the agent when someone @s
later. A failed download is logged and skipped rather than aborting the
message — a broken attachment must not silence the text that came with it.

Media upload is the only reason this channel holds an oapi.dingtalk.com token:
it has no v1.0 equivalent. Media ids come back prefixed with '@', and passing
that through verbatim makes the subsequent send fail with an unhelpful "invalid
media". Files over 20MB take the three-step chunk transaction, whose
chunk_number is 1-based; a 0-based sequence is accepted and reassembles wrong.

Image (10MB) and voice (2MB) caps are checked before the bytes go out so the
operator sees the real limit. Video and file are not capped there because the
chunk path handles them.

sampleVideo requires a cover image (picMediaId) that we do not have, so a video
is delivered as a file: a worse preview, but a working download beats a
rejected send.

Media never rides the session webhook, which cannot carry it. TypeDingtalk is
added to channels.mediaCapableTypes now — and only now — because Send() finally
consumes msg.Media, which is the condition that map's comment states.
Implements channels.StreamingChannel so an answer types into a DingTalk AI Card
instead of arriving as one late message.

The model is telegram/stream.go, not feishu — feishu does not implement
StreamingChannel at all; its `streaming` config flows into a struct and is never
read. A card instance is naturally per-run (one outTrackId), which is exactly
what the interface's per-run ChannelStream wants, so cards are never keyed by
chatID.

Stuck cards are the hazard worth designing against: a card left at INPUTING
spins forever in the DingTalk UI, and the upstream connector needed a dedicated
fixStuckCards repair RPC because of it. Three guarantees:

  - Stop() drives the card terminal, and the framework calls Stop on
    run.completed, run.failed AND run.cancelled (events.go:265,277,296).
  - Stop() is idempotent, and stamps the terminal status even when the final
    content frame fails.
  - Channel.Stop() aborts every still-open card, so a gateway restart mid-run
    does not abandon one. It stamps FAILED there rather than FINISHED: the
    answer never arrived, and claiming otherwise would lie.

Stop() carries no error, so a failed run cannot be told from a completed one and
both land on FINISHED. A terminal card with a generous status beats a card frozen
mid-thought; the framework separately publishes a readable error message.

FinalizeStream runs only on run.completed, and hands the card to Send(), which
repaints it with the formatted answer rather than posting a second message
beneath it. On a failed run there is no handoff, so the error message arrives as
its own message — which is what a reader wants.

Two throttles, as upstream: an 800ms per-card debounce that advances its clock
before the request rather than after, so a slow write cannot let a concurrent
update slip in behind it; and a QPS gate. The gate is per-channel, not
process-global as upstream: DingTalk meters the card API per app, and a channel
instance is exactly one app. A rate-limited frame retries once with a fresh guid
— reusing it makes DingTalk drop the frame as a duplicate.

isFull is always true: DingTalk wants the whole text each frame, not a delta.
Non-final frames have their trailing newline stripped or the card flickers.

ReasoningStreamEnabled is false: the stock template exposes a single msgContent
key, and a reasoning lane needs a second one registered in
sys_full_json_obj.order — a template change, not a code change.
… (phase 7)

Makes the channel visible and operable, then hardens it against what a
red-team pass over the implementation turned up.

Surface parity: CHANNEL_TYPES, channel-schemas (credentials + config),
channels-status-utils labels, bindings-section, contacts-page, agent instance
display labels, contacts permissionsNote in all four locales (ko is a real
fourth locale, contrary to CLAUDE.md, and newer channels have shipped without
it), and a DingTalk section in docs/05-channels-messaging.md.

requiredScopes deliberately has no dingtalk entry. That map lists exact
permission ids an operator pastes into the provider console, and DingTalk's ids
for the card / robot / media permissions could not be verified from either
upstream repo. A wrong id fails opaquely in the console; the docs list the
permissions by name instead.

Review fixes:

- Data race on c.transport between Start and Stop. These genuinely race:
  startChannelWithTimeout runs Start on its own goroutine and calls Stop from
  another when Start overruns, while Start is still inside transport.Start.
  Now guarded by stateMu. Removing the mutex makes the new regression test
  report 8 races under -race.

- dm_policy and group_policy typos failed OPEN. validate() checked the reply
  enums but not the policies, and BaseChannel's policy switches treat an
  unrecognized value as their permissive default. group_policy: "opne" made the
  bot answer every group message from anyone, silently. Both are now validated
  at instance-save time.

- async_mode and ack_text were dead config: parsed, shown in the UI, never
  implemented. An operator enabling them got streaming silently disabled and no
  acknowledgement. Removed from the config, the factory, StreamEnabled, and the
  UI; documented as unsupported.

- A saturated bus pinned inbound goroutines forever, and Stop neither cancelled
  nor drained them. runCtx is now cancellable, Stop cancels it, and publish()
  retries TryPublishInbound on a tick, giving up when the channel stops.
  Backpressure survives; the wait is interruptible.

- Concurrent runs in one chat overwrite the card handoff, so the loser's answer
  posts as its own message rather than repainting its card. No card gets stuck
  (Stop drives both terminal before FinalizeStream runs). Telegram's
  placeholders map has the same shape; the overwrite now logs a warning naming
  both cards instead of passing silently.

Plan, plan audit, and implementation audit: plans/260709-2230-dingtalk-channel/
…fication

Local testing against a real gateway showed the docs and the phase-7 commit
message overstated when policy validation runs. `requireOneOf` lives in the
factory, so a `group_policy: "opne"` typo saves fine and is only rejected when
the channel starts:

  failed to reload channel instance name=badpolicy type=dingtalk
  error="dingtalk group_policy \"opne\": want one of pairing, open, allowlist, disabled"

It still fails closed — a misconfigured instance never runs — but the operator
has to check the channel's health to find out why, which is worth saying
plainly rather than claiming save-time rejection.

Also records what the live run did confirm: both isValidChannelType twins
accept dingtalk over their respective APIs and reject an unknown type;
credentials round-trip through AES-GCM at rest; and the Stream transport
genuinely reaches DingTalk — fake credentials come back as
`code: authFailed, message: 鉴权失败` from the SDK's endpoint negotiation,
without taking the gateway down.
…eway

A live run against real DingTalk credentials panicked the whole process:

  panic: close of closed channel
    channels.(*PendingHistory).StopFlusher(history_flush.go:24)
    channels/dingtalk.(*Channel).Stop(dingtalk.go:185)
    channels.(*InstanceLoader).Reload(instance_loader.go:130)

PendingHistory.StopFlusher is a bare close(ph.stopCh) with no guard. Every
other channel calls it from exactly one place, Stop(). This channel had two:
phase 2 added a StopFlusher to Start's failure branch so a bad AppKey would not
leave the flusher spinning, and then the InstanceLoader's reload called Stop on
the same channel. Enabling an instance with real credentials — which triggers a
reload right after a start — hit both.

Both paths now go through stopHistory(), guarded by a sync.Once.

The existing tests could not have caught this. They construct the channel with
a nil PendingMessageStore, and StopFlusher returns early when the store is nil,
so the double close never happened. The regression tests add a fake store so
the flusher is real; without the sync.Once they reproduce the panic exactly.
…yped hooks

A live group chat surfaced this:

  pending_history.db_fallback_failed channel=dingtalk-http key="cid6Rjc…" error="tenant_id required"
  pending_history.clear_db_failed    channel=dingtalk-http key="cid6Rjc…" error="tenant_id required"

Every group-history DB write failed, so un-@'d group messages were never
persisted and the context a later @mention should have seen was silently lost.

The factory builds PendingHistory before the InstanceLoader assigns the tenant,
so the loader calls back with SetPendingHistoryTenantID (instance_loader.go:326)
to fill it in. Five channels implement that method. This one did not — and the
loader reaches for it through an anonymous interface assertion, so the call was
skipped with no compile error, no test failure, and no log line saying a hook
had been missed. Only a real group conversation showed it.

Adds the method, and pins the whole set of hooks the loader and manager
duck-type onto channels with a compile-time assertion. Everything except
SetPendingHistoryTenantID comes free from BaseChannel; deleting it now fails the
build naming the missing method, instead of degrading in production.
Live group test: an un-@'d message produced no callback at all — zero inbound
between it and the @mention that followed. DingTalk's Stream robot callback
fires for a group message only when the bot is @mentioned, which the platform
docs state outright ("在群中发消息并且@机器人,就会在回调接口中收到相关信息").

Two things this makes false, both now corrected:

- A comment claimed attachments are resolved before the mention gate "so a file
  dropped into a group without an @ still reaches the agent when someone does @
  later." That cannot happen; the message never arrives.
- `require_mention: false` reads as "answer every group message". It cannot.
  The setting is still enforced rather than assumed — the SDK hands us
  IsInAtList and we read it — but there is nothing for it to let through.

The record-without-reply branch is kept and marked unreachable rather than
deleted: it is the correct behavior the moment an app is granted group-message
listening, and silently answering every group message would be the worse failure
if that ever changes.

Also notes it in docs/05-channels-messaging.md, the require_mention help text in
the channel config UI, and the phase-07 manual checklist, which had told the
tester to expect exactly the behavior the platform does not provide.
…pens

A live DM produced two bubbles: an empty card, then the answer. The empty one
came from CreateStream.

The framework opens a stream at run.started — before a single token exists —
and stops it again at the first tool call, without finalizing it
(events.go:53, :107). CreateStream was eagerly POSTing create + deliver, so
that first, contentless stream already put a card in the conversation; Stop then
stamped it FINISHED and abandoned it, and the answer went into a second card.

Telegram does not have this problem because DraftStream defers: "No placeholder
seeding — DraftStream creates its own message on first flush()" (stream.go:312).
The same reasoning applies here and the port should have followed it.

CreateStream now touches the network not at all. ensureCard posts create +
deliver on the first Update that carries non-blank text. Stop and abort on a
stream that never materialized are no-ops, and FinalizeStream does not hand such
a stream to Send() — otherwise Send would try to repaint a card that does not
exist instead of posting the answer as a message.

The existing tests asserted the eager behavior, which is why they passed. They
now assert the deferred one, and two regression tests reproduce the bug when the
eager path is restored: one counts requests issued by CreateStream (3, want 0),
the other walks a tool-call run and counts delivered cards (2, want 1 — "an empty
card was left in the chat").
Implements channels.ReactionChannel, so while a run is in flight the bot posts
🤔思考中 on the message that triggered it and recalls it when the run ends —
the behavior the upstream connector shows and that phase 6 had deferred.

reaction_level is `on`/`off`, not Feishu's `off`/`minimal`/`full`. DingTalk's
emotion API is keyed by a numeric emotionId, and the only id whose meaning is
established is the thinking face the connector hardcodes (2659900). Inventing
further ids to fill out a "minimal" tier would be fabrication, so the tier does
not exist and the divergence is documented rather than papered over.

The framework reports "thinking" at run.started and then one status per tool
call (events.go:405-423). All of them mean the same thing here — there is one
reaction — so the first posts it and the rest are no-ops, tracked by message id.
A failed post clears that mark so a later status retries.

openConversationId cannot be derived from the chatID the interface hands us: a
DM's chatID is the sender's staff id, and group_session_scope=group_sender
suffixes a group's. It comes from the chatMeta recorded when the message
arrived, which is also why a cron or delegate run — no inbound message, no
conversation — posts nothing rather than erroring.

Reactions are cosmetic: every failure is logged and swallowed. A run must never
fail because an emoji did not stick.
card_update_interval_ms, default 800ms to match upstream, floor 100ms.

Measured against a live card rather than assumed. The gateway pushes a full-text
frame (isFull: true) on each repaint; over a 10s answer that was 14 frames of
30-45 new characters each. The user saw smooth per-character typing. Nobody types
40 characters instantaneously and then pauses 830ms, so the animation is drawn by
the DingTalk client between frames — the protocol carries coarse full-text
snapshots, not a token stream.

That makes the interval a cost knob rather than a UX knob: one billed card API
call per frame, and raising it to 1500-3000ms cuts calls by half to two-thirds
while the typing still reads as smooth. Upstream's 800ms appears to be aimed at
"looks real-time", which the client provides regardless.

What streaming actually buys is time to first token. On a 22k-token prompt the
first frame landed 5.6s after the inbound message; the full answer arrived at
15.7s. With streaming off — which is also what group_reply_mode text/markdown
selects — nothing is visible until generation finishes.

The floor exists because a very small interval queues behind the 20 QPS gate,
where the card stops advancing and reads as a hang rather than a rate limit.
An idle gateway died on its own:

  panic: send on closed channel
    dingtalk-stream-sdk-go@v0.9.1/client/client.go:161
    processLoop.func6, created by processLoop at client.go:156

v0.9.1's processLoop defers close(closeChan) while the reader goroutine it
spawned still does `closeChan <- struct{}{}` on a read error. Whenever
processLoop returns first — an idle keepalive timeout, a reconnect, a network
blip — the reader sends on a closed channel and takes the whole process with it.
No user activity preceded the crash here; the last log line was a usage rollup.

This is not defensible from our side. The panic happens in the SDK's own
goroutine, and Go cannot recover a panic across goroutines, so safego.Recover on
our transport buys nothing. The dependency has to carry the fix.

Upstream replaced closeChan with a context (issues nextlevelbuilder#27, nextlevelbuilder#28, nextlevelbuilder#32) and merged it
in nextlevelbuilder#34 on 2026-07-05, but has not cut a tag: v0.9.1 is still latest. So the
module is pinned to that commit, and stream.go says why, because the next person
to see an untagged pseudo-version will otherwise "tidy" it back to v0.9.1 and
reintroduce the crash.

Verified: the pinned tree has the loopCtx guard and no unguarded channel send,
and the API surface we use (NewStreamClient, WithAppCredential,
NewAppCredentialConfig, Start, Close, RegisterChatBotCallbackRouter,
IChatBotMessageHandler, BotCallbackDataModel incl. IsInAtList) is unchanged.
gorilla/websocket and google/uuid resolve to the same versions as before.
… gate

DingTalk had no way to start a fresh conversation. Auto-compaction bounds a long
session's token count but never forgets it, so the accumulated summary rides
every prompt forever; /new is the deliberate drop.

Parsing is per-channel — Telegram, WhatsApp (nextlevelbuilder#819), and now DingTalk each do
their own — but none reset or cancel anything directly. They publish an inbound
message carrying Metadata[tools.MetaCommand], and the shared consumer turns it
into a reset or run cancellation, rebuilding the session key as a normal turn
would, then continues without an agent run. Only the string parsing was missing;
the reset machinery was already shared.

Aliases match the upstream connector (session.ts): /new /reset /clear plus the
Chinese equivalents.

Group /new is gated on the group file-writer permission — fail-closed, unlike
Telegram's fail-open: a nil store, an agent-resolve failure, or a CheckPermission
error all deny. A destructive op on shared group state must not default to
permitted because the permission check is broken. /stop and /stopall touch only
the sender's own run and are ungated. DMs never reach the gate. The hook sits
after the policy gates and before media fetch.

Also adds the Russian (ru) dingtalk permissionsNote; upstream added ru as a
fifth locale (nextlevelbuilder#1391) after the channel's i18n pass.
@feixiang-lex

Copy link
Copy Markdown

DingTalk Channel PR Review

Hi Conner — thanks for the substantial work here. Overall, this is a thoughtful implementation: the fail-closed permission checks, ACK-then-process flow, deduplication, config validation, SDK pin, and test coverage are all especially solid.

I did find several issues that I think should be addressed before merging:

Blocking issues

1. [Security] Secrets may leak into logs

oapiToken() puts appsecret in the query string, and doOAPI() does the same with access_token. When http.Client.Do() fails, the resulting *url.Error contains the full URL, including its query string. These errors eventually reach structured logs through the outbound dispatcher.

The signed OSS download URL has the same issue: a failed download can log the complete signed URL.

Relevant locations:

  • internal/channels/dingtalk/client.go:143
  • internal/channels/dingtalk/client.go:271
  • internal/channels/dingtalk/media_inbound.go:125
  • internal/channels/dispatch.go:108

Could we sanitize query parameters at the HTTP client boundary before wrapping errors? Doing it centrally would be safer than relying on every logging caller to redact them.

2. [Reliability] AI Card writes synchronously block event broadcasting

Manager.HandleAgentEvent() is documented as needing to remain non-blocking, but cardStream.Update() synchronously performs several HTTP calls when materializing the first card: create, deliver, state update, and streaming push.

MessageBus.Broadcast() invokes subscribers synchronously and serially, so a slow DingTalk API call can block subsequent agent events and every later subscriber—potentially for multiple HTTP timeout periods.

Relevant locations:

  • internal/channels/events.go:17
  • internal/channels/dingtalk/card.go:181
  • internal/bus/bus.go:120

I think Card writes need a per-stream, ordered async worker that can coalesce stale repaint frames.

3. [Lifecycle] A timed-out start can later mark a stopped channel healthy

The instance loader calls Stop() after a start timeout. However, if transport.Start() later returns successfully, Channel.Start() still unconditionally calls SetRunning(true) and MarkHealthy().

That can leave the transport and history flusher stopped while the dashboard reports the channel as healthy.

Relevant locations:

  • internal/channels/instance_loader.go:428
  • internal/channels/dingtalk/dingtalk.go:195

Could this use an explicit starting/running/stopped state, or at least re-check whether the channel was stopped before marking it healthy?

4. [Permissions] group_sender session scope leaks into group-level identities

With group_session_scope=group_sender, chatID() appends the sender ID. That derived ID is then used for group pairing and the file-writer permission scope.

This changes pairing from group-level to per-member pairing and prevents permissions granted to the real group scope from matching /new.

Relevant locations:

  • internal/channels/dingtalk/inbound.go:299
  • internal/channels/dingtalk/policy.go:42
  • internal/channels/dingtalk/commands.go:138

Group pairing and permission scopes should use the original ConversationID; only the session key should include the sender suffix.

Other issues

5. The command path can block forever on a saturated inbound bus

Normal messages use cancellable TryPublishInbound() retries, but handleCommand() directly calls blocking PublishInbound(). During shutdown, a command goroutine can therefore remain stuck indefinitely.

  • internal/channels/dingtalk/commands.go:99

Could this reuse Channel.publish(ctx, msg) and only acknowledge the command if publishing succeeds?

6. /stop and /stopall send duplicate and potentially contradictory feedback

The DingTalk handler immediately replies with “task stopped,” while the shared consumer later sends feedback based on the actual cancellation result—including “no active task.”

Users can therefore receive two messages whose claims disagree.

  • internal/channels/dingtalk/commands.go:86
  • cmd/gateway_consumer_handlers.go:469

As with Telegram, it would be better for stop commands to let the consumer send the single authoritative result.

7. Card creation and termination have a race

ensureCard() checks created and releases the mutex before performing network calls. Concurrent updates can both create cards and overwrite outTrackID.

There is also an Update/Stop race: Stop() can observe created=false, mark the stream done, and return while an in-flight ensureCard() subsequently delivers a card. That card may remain permanently in the INPUTING state.

  • internal/channels/dingtalk/card.go:210
  • internal/channels/dingtalk/card.go:371

An explicit creating state or a serialized per-card lifecycle would close both windows.

8. The configured 60-second file download timeout is ineffective

All requests use an http.Client with a 30-second timeout, so the 60-second file-download context can never be reached. The same client-level limit also applies to larger chunk uploads.

  • internal/channels/dingtalk/client.go:73
  • internal/channels/dingtalk/media_inbound.go:115

A dedicated media client or transport-level timeouts combined with per-request context deadlines would preserve the intended budgets.

9. The v1.0 API token has no fallback TTL

apiToken() caches the token using expireIn directly. If DingTalk omits the field or returns zero, the cached token expires immediately, so every API operation fetches another token. The token-cache mutex prevents a refresh stampede, but it also serializes every request behind that extra network round trip.

  • internal/channels/dingtalk/client.go:126
  • internal/channels/dingtalk/client.go:136

oapiToken() already handles the equivalent case with defaultOAPITokenTTL. The v1.0 path should apply the same kind of positive fallback TTL.

10. Proactive group delivery has an unsafe fallback when conversation metadata is missing

For group messages, postProactive() requires the original DingTalk openConversationId. If dingtalk_conversation_id is missing, it falls back to msg.ChatID. Under group_session_scope=group_sender, however, that value is the derived session identity (conversation:sender), not a valid DingTalk conversation ID.

  • internal/channels/dingtalk/outbound.go:132
  • internal/channels/dingtalk/outbound.go:135

The normal inbound-reply path preserves both metadata fields together, so this mainly affects metadata-less or externally initiated outbound messages. Deriving the conversation ID by trimming a suffix would be ambiguous; it would be safer to reject proactive group delivery when the required metadata is absent, or to carry the original conversation ID as an explicit routing field for cron/delegate sends.

Lower-priority observations

  • takeCard() removes the card before repaint succeeds. A repaint failure loses the handoff and also prevents media delivery; a later retry can then post a duplicate standalone message instead of repainting the card.
  • Outbound attachments are fully loaded with os.ReadFile() before chunking, so large files can cause significant memory spikes.
  • Blank updates advance lastPush, which can delay the first real token by one repaint interval.

I also checked a couple of possible false positives:

  • Moving github.com/ollama/ollama from indirect to direct is correct—the project imports its API package directly.
  • The runCtx lifetime is currently safe because the loader intentionally passes a long-lived context rather than its start-timeout context.

go test -race ./internal/channels/dingtalk passes, but the current tests do not appear to cover late start completion, concurrent Card Update/Stop, a saturated command bus, or the end-to-end stop feedback path.

Thanks again—there is a lot of good engineering in this channel implementation. The main things I’d want resolved before merge are the credential logging risk, event-loop blocking, lifecycle race, and group-scope identity issue.

@clark-cant

Copy link
Copy Markdown
Contributor

🤖 Maintainer Triage (github-maintain)

Status: Blocked — merge conflicts detected (DIRTY state)

Assessment:

  • 6,952 additions across 48 files — substantial new channel implementation
  • DingTalk Stream mode + AI Card streaming is a valuable contribution
  • Merge state is DIRTY: branch needs rebase onto current dev

Action required from author:

  1. Rebase feat/dingtalk-channel onto latest origin/dev
  2. Resolve merge conflicts
  3. Push updated branch

After rebase:

  • CI checks will run automatically
  • Maintainers will review the implementation

Labels added: pr:oversized (for reviewer attention), status:blocked (merge conflicts)


Posted by github-maintain at 2026-07-15T00:30:00Z

@clark-cant

Copy link
Copy Markdown
Contributor

🔍 Maintainer Triage (github-maintain)

PR type: Feature — new channel (DingTalk with Stream mode + AI Card streaming)
Size: 48 files, +6952/-19 lines — oversized for automated review
Merge state: DIRTY (needs rebase onto dev)
CI: No checks reported

Assessment

This is a substantial feature PR adding a new first-class channel type. The scope and quality look impressive — Stream transport, AI Card streaming, media handling, slash commands, web UI parity, and extensive unit tests.

Blockers before review

  1. Rebase required — branch is DIRTY, needs rebase onto current dev
  2. Oversized for automated review — 6952 additions across 48 files exceeds bounded cron review budget. This will need maintainer deep-review.

Recommendation

  • Author: please rebase onto latest dev
  • Maintainer: schedule dedicated review session for this PR (not suitable for 15-min cron slot)

Notes

  • Plan document referenced (plans/260709-2230-dingtalk-channel/plan.md) — good practice
  • Uses official DingTalk Stream SDK (good choice over hand-rolled WS)
  • Companion docs update included

Posted by github-maintain at $(date -u +'%Y-%m-%dT%H:%M:%SZ')

@clark-cant clark-cant added agent:github-maintain Processed by github-maintain automation maintain:triaged Triaged by maintain workflow labels Jul 15, 2026
@clark-cant

Copy link
Copy Markdown
Contributor

🦸‍♂️ Maintainer check — github-maintain

This PR adds a substantial new channel (DingTalk with Stream mode + AI Card streaming) — 48 files, +6952 lines. Well-documented with test coverage.

Current blockers:

  1. Merge conflicts — branch has conflicts with dev base (mergeState: DIRTY). Please rebase/merge latest dev and resolve conflicts.
  2. No CI checks — GitHub Actions have not run on this PR. CI needs to pass before review can proceed.

Next steps:

  1. Rebase onto latest dev and resolve conflicts
  2. Push to trigger CI
  3. Once CI is green, we'll do a full review

The PR structure looks solid (channel package follows existing patterns like Feishu/Discord, factory registration, web UI schema updates). Just needs the branch cleanup before we can proceed with review.

Posted by github-maintain at $(date -u +'%Y-%m-%dT%H:%M:%SZ')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:github-maintain Processed by github-maintain automation maintain:triaged Triaged by maintain workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants