slackapi / slackapi/python-slack-sdk

chat_postMessage silently forwards thread_id to the API, so a threaded reply posts to the channel

Đang mở
#1,923 2 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

auto-triage-skip enhancement
Ngôn ngữ chính
Python
Star
4k
Fork
857
Merge trung bình
22 giờ 21 phút
Pull request đã merge (30 ngày)
16

Mô tả

Summary

Every WebClient method takes **kwargs and forwards unrecognised keys into the request body
untouched. A one-character mistake in a parameter name therefore produces a request that is
missing the parameter you meant and carrying one nobody reads, with no exception and no warning.

For thread_ts nothing errors at all. A threaded reply becomes a top-level channel message and
every downstream check reports success.

Reproduction

Against slack_sdk 3.43.0, with the transport stubbed so the outgoing body is visible:

client.chat_postMessage(
    channel="C1",
    text="hi",
    thread_id="1750000000.0001",   # the parameter is thread_ts
    unfurl_link=False,             # the parameter is unfurl_links
)

Body actually sent:

{"thread_id": "1750000000.0001", "unfurl_link": false, "channel": "C1", "text": "hi"}

warnings raised: 0. thread_ts is absent from the body entirely, because it was None and
_remove_none_values stripped it. So the request is well formed, it just is not the request the
caller wrote.

Mechanism

In slack_sdk/web/client.py, chat_postMessage declares its known parameters and then does:

kwargs.update({ "channel": channel, "text": text, ..., "thread_ts": thread_ts, ... })
_parse_web_class_objects(kwargs)
kwargs = _remove_none_values(kwargs)
return self.api_call("chat.postMessage", json=kwargs)

thread_id arrived through **kwargs, survives _remove_none_values because it is not None,
and goes out with everything else. Nothing in the path compares the caller's keys against the
declared parameter list.

Why I think this deserves a fix

The declared parameters are already there, in the signature, which is what makes this cheap. The
method knows the full set of names it accepts. A caller who misses by one character is currently
told nothing at all, and the failure surfaces somewhere else, later, as a message in the wrong
place.

Three options, cheapest first:

  1. Warn on unknown keys. warnings.warn naming the key and the method. Non-breaking, and it
    makes the mistake visible in a test run.
  2. Warn harder on near misses. If an unknown key is within an edit distance of one or two of a
    declared parameter, say which one you probably meant. thread_id to thread_ts,
    unfurl_link to unfurl_links, file_content to content.
  3. Do nothing, but say so in the docstring, since the current behaviour is reasonable as an
    escape hatch for API parameters the SDK has not caught up with. That is a real design reason
    for **kwargs and I do not think it should be removed.

I would expect (1) or (2). Removing **kwargs would break the escape hatch and I am not
suggesting it.

A related one, same mechanism, different symptom

files_upload_v2(file_content=...) sends file_content through **kwargs and then raises

SlackRequestError: Any of file, content, and file_uploads must be specified.

which names three parameters the caller does not believe they omitted. Same root cause, and the
error message is actively misleading rather than merely absent.

How I found this, and what I am not claiming

I maintain a test harness that measures whether a coding model can drive a given SDK by running
the code it writes and asserting on the HTTP the SDK emits. slack_sdk 3.43.0 came out at the
top of everything I have measured: 16 tasks, 89 checks, three models at three attempts each, and
both frontier models passed all 89 checks on all 48 rollouts. So this is not a report that
models struggle with slack_sdk. They do not.

No model hit this bug. I found it by construction while checking whether my own scoring
could be gamed. That makes it latent, not measured, and I would rather say so than let it read
as a field report.

One limit on the repro above: my transport is stubbed, so what I can show is the body your SDK
sends. Whether Slack ignores thread_id and answers ok: true is your knowledge, not mine.

Happy to open a separate issue about files_upload's deprecation warning, which describes a
timeout risk ("may cause some issues like timeouts for relatively large files",
internal_utils.py:443) for an endpoint that has been sunset. Say the word rather than me
filing two at once.

toolshed is a small studio run by its owner, who directs the work, and AI does a lot of the
engineering.

Cal / toolshed / toolshedlabs@gmail.com

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu trong slack_sdk/web/client.py, đặc biệt là chat_postMessage và files_upload_v2, rồi theo dõi cách các tham số được khai báo và **kwargs đi đến _remove_none_values và api_call. Xem lại các tùy chọn cảnh báo của issue và các bài kiểm thử hiện có trước khi chọn cách xử lý. Được xem là hoàn tất khi cách xử lý đã chọn được bao phủ cho các khóa không xác định và các trường hợp thread_id và file_content được báo cáo không còn thất bại một cách âm thầm hoặc gây hiểu lầm.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
api, backend-api-design
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
48/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.