Skip to content

fix(dingtalk): fix multiple bugs in DingTalk platform adapter#9003

Closed
yyq4193 wants to merge 1 commit into
NousResearch:mainfrom
yyq4193:fix/dingtalk-platform-bugs
Closed

fix(dingtalk): fix multiple bugs in DingTalk platform adapter#9003
yyq4193 wants to merge 1 commit into
NousResearch:mainfrom
yyq4193:fix/dingtalk-platform-bugs

Conversation

@yyq4193

@yyq4193 yyq4193 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes multiple bugs in the DingTalk platform adapter that prevented it from working correctly.

Bugs Fixed

  1. Fix async stream client: Replace asyncio.to_thread(start) with await start() — the SDK's start() is an async method, not a blocking function. Using asyncio.to_thread() creates a coroutine that is never awaited, so the WebSocket loop never starts.

  2. Make _IncomingHandler.process an async method: The SDK's raw_process() does await self.process(). A sync method returning a tuple cannot be awaited, causing object tuple can't be used in 'await' expression.

  3. Convert CallbackMessage.data to ChatbotMessage: The SDK passes a CallbackMessage wrapper. The actual chatbot payload is in callback_message.data as a dict that must be converted via ChatbotMessage.from_dict().

  4. Use fire-and-forget pattern for message processing: process() must return quickly so the SDK can ACK to DingTalk server. Blocking on _on_message causes the SDK to miss server heartbeats and disconnect.

  5. Fix session_webhook regex to match oapi.dingtalk.com: DingTalk uses oapi.dingtalk.com for session webhooks, not api.dingtalk.com.

  6. Fix log level for inbound messages: Changed from debug to info for easier tracing.

  7. Fix platform_toolsets None AttributeError in tools_config.py: When platform_toolsets is an empty YAML key, config.get() returns None instead of empty dict, causing AttributeError on .get(platform).

Testing

All fixes have been tested and verified — DingTalk now connects stably and correctly processes/replies to messages.

@yyq4193
fix(dingtalk): fix multiple bugs in DingTalk platform adapter
7ebabc0 
- Fix async stream client: replace asyncio.to_thread(start) with await start()
- Make _IncomingHandler.process an async method
- Convert CallbackMessage.data to ChatbotMessage before dispatching
- Use fire-and-forget pattern for message processing
- Fix session_webhook regex to match oapi.dingtalk.com
- Fix log level for inbound messages (debug -> info)
- Fix platform_toolsets None AttributeError in tools_config.py
@Wonham Wonham mentioned this pull request Apr 17, 2026
Closed
1 task
@Wonham

Wonham commented Apr 17, 2026

Copy link
Copy Markdown

Verified all fixes in this PR work correctly on v0.9.0 + dingtalk-stream v0.24.3.

Just spent a few hours debugging the exact same issues from scratch and arrived at the same root causes. The three bugs (wrong parent class, CallbackMessage→ChatbotMessage conversion, and oapi.dingtalk.com regex) are confirmed — without all three fixes stacked together the DingTalk channel is completely broken.

Also independently confirmed: _is_user_authorized() only reads env vars (DINGTALK_ALLOWED_USERS / DINGTALK_ALLOW_ALL_USERS), not config.yaml's platforms.dingtalk.allowed_users. That's a separate usability issue worth tracking but not a blocker for this PR.

+1 for merging — there's at least one more user (issue #11463) blocked by these exact bugs.

teknium1 added a commit that referenced this pull request Apr 17, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
726bea3 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's #9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
@teknium1 teknium1 mentioned this pull request Apr 17, 2026
Merged
teknium1 added a commit that referenced this pull request Apr 17, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
c60b6dc 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's #9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
@teknium1

teknium1 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded. Almost all of this PR is now covered by other merged work:

I did cherry-pick your one-liner fix in hermes_cli/tools_config.py — config.get("platform_toolsets", {}) → config.get("platform_toolsets") or {}. That handles the edge case where the YAML key is present but explicitly null. Shipped via #11605 with a Co-authored-by: yyq4193 ... trailer on the commit. Also added a regression test.

One change I deliberately did NOT carry over: r'^https?://(api|oapi)\.dingtalk\.com/' — the https? would re-allow plain HTTP on the webhook URL allowlist, which is a security regression. The https only posture is intentional. Thanks for the contribution!

@teknium1 teknium1 closed this Apr 17, 2026
ulasbilgen pushed a commit to ulasbilgen/hermes-adhd-agent that referenced this pull request May 1, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
bcfa4f4 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's NousResearch#9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
aj-nt pushed a commit to aj-nt/hermes-agent that referenced this pull request May 1, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
fec3c80 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's NousResearch#9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
02356abc pushed a commit to 02356abc/hermes-agent that referenced this pull request May 14, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
9a80cf6 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's NousResearch#9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
bdad0e0 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's NousResearch#9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
Egavasyug pushed a commit to Egavasyug/hermes-agent that referenced this pull request Jun 10, 2026
@teknium1 @yyq4193
test(dingtalk): cover get_connected_platforms + null platform_toolsets
55ab5d2 
Follow-ups to the salvaged commits in this PR:

* gateway/config.py — strip trailing whitespace from youngDoo's diff
  (line 315 had ~140 trailing spaces).

* hermes_cli/tools_config.py — replace `config.get("platform_toolsets", {})`
  with `config.get("platform_toolsets") or {}`. Handles the case where the
  YAML key is present but explicitly null (parses as None, previously
  crashed with AttributeError on the next line's .get(platform)).
  Cherry-picked from yyq4193's NousResearch#9003 with attribution.

* tests/gateway/test_config.py — 4 new tests for TestGetConnectedPlatforms
  covering DingTalk via extras, via env vars, disabled, and missing creds.

* tests/hermes_cli/test_tools_config.py — regression test for the null
  platform_toolsets edge case.

* scripts/release.py — add kagura-agent, youngDoo, yyq4193 to AUTHOR_MAP.

Co-authored-by: yyq4193 <39405770+yyq4193@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yyq4193 @Wonham @teknium1