fix(gateway): guard chained .get() against None intermediate values - #34695
annguyenNous wants to merge 1 commit into
Conversation
.get("key", {}) only applies the default when the key is ABSENT.
When the key exists with value None (null in JSON), .get() returns
None and the subsequent .get() raises AttributeError.
Fix: replace .get("key", {}).get(...) with (.get("key") or {}).get(...)
which handles both missing keys AND None values.
8 instances across 6 files:
- gateway/run.py: tool_call function name check
- acp_adapter/server.py: tool name/description extraction
- gateway/platforms/qqbot/onboard.py: API response task_id
- gateway/platforms/yuanbao.py: message content parsing (x2)
- gateway/platforms/slack.py: block text extraction (x2)
- tui_gateway/server.py: error message extraction
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real defensive-parsing issue. The premise still holds on current main, but this branch needs targeted salvage before it can provide the stated coverage.
Problems
- The Slack edits target
gateway/platforms/slack.py, but the live handlers moved toplugins/platforms/slack/adapter.pyin5600105478ffde29d7566b45421b100eaa29c4ef; the unsafe chains remain atplugins/platforms/slack/adapter.py:3514and:3636. - The Yuanbao methods remain unsafe at
gateway/platforms/yuanbao.py:2156and:2175on current main. - ACP has a sibling unsafe access over the same tool list at
acp_adapter/server.py:1800, in addition to the two display accesses changed by this PR. - The six-file diff contains no regression test for present-but-null intermediates.
Suggested changes
- Port the Slack and Yuanbao guards to their current locations, guard the ACP sibling, and add focused null-intermediate regression tests.
Automated hermes-sweeper review.
| for block in message.get("blocks", []): | ||
| if block.get("type") == "section": | ||
| original_text = block.get("text", {}).get("text", "") | ||
| original_text = (block.get("text") or {}).get("text", "") |
There was a problem hiding this comment.
This handler has moved on current main: please port this guard to plugins/platforms/slack/adapter.py:3514 (and the matching approval handler at :3636), otherwise the active Slack code remains unguarded.
| for t in tools: | ||
| name = t.get("function", {}).get("name", "?") | ||
| desc = t.get("function", {}).get("description", "") | ||
| name = (t.get("function") or {}).get("name", "?") |
There was a problem hiding this comment.
Please also guard the sibling tool.get("function", {}).get("name") used to build valid_tool_names; on current main it is in the same _cmd_tools flow at acp_adapter/server.py:1800.
|
Merged via #70196 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your None-intermediate guards were cherry-picked. Thanks for the contribution! |
Summary
.get("key", {})only applies the default{}when the key is absent from the dict. When the key exists with valueNone(null in JSON/API responses),.get()returnsNoneand the subsequent.get()raisesAttributeError.This is the same pattern documented in Pitfall 28 —
.get(key, default)vs.get(key) or default.Fix
Replace
.get("key", {}).get(...)with(.get("key") or {}).get(...)which handles both missing keys AND None values.Affected locations (8 instances, 6 files)
gateway/run.pyacp_adapter/server.pygateway/platforms/qqbot/onboard.pygateway/platforms/yuanbao.pygateway/platforms/slack.pytui_gateway/server.pyImpact
All instances involve parsing external API responses or tool call structures where null/None values are possible. The crash manifests as
AttributeError: 'NoneType' object has no attribute 'get'.