test(slack): add peer-agent smoke target - #51637
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Well-structured addition of a repeatable smoke test for Slack peer-agent routing invariants. Key positives:
- Uses synthetic in-process Slack events (no live messages or real tokens needed)
- Thorough preflight contract check covering config, connectivity, and bot identity
- Tests all critical routing paths: human mention routing, peer-bot ignore without mention, explicit peer-bot mention routing, passive ack/status/error suppression
- Good documentation of failure buckets in the companion docs update
- Clean mock setup with
_ensure_slack_mock()for environments without slack_bolt installed
No security concerns. The test file is self-contained and well-documented.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding a focused synthetic routing target and documenting an operator command.
Problems
tests/gateway/test_slack_peer_agent_smoke.py:91creates the passing configuration, mocked client, and bot identity before_assert_peer_agent_preflight()runs. The preflight cannot therefore produce the config, connectivity, or bot-identity failures thatwebsite/docs/user-guide/messaging/slack.md:409presents as post-deploy checks.tests/gateway/test_slack_peer_agent_smoke.py:163suppliesbot_idfor peer events. Current main already recognizes that shape atplugins/platforms/slack/adapter.py:2600; this does not cover the ordinary bot-user event shape discussed in linked PR #51627.
Suggested changes
- Either describe this as a deterministic adapter regression test, or add a path that inspects effective runtime configuration and identity state.
- Add the no-
bot_id/no-subtypepeer bot-user event shape if this target is intended to cover the peer-routing incident class.
Automated hermes-sweeper review.
| text="status: work finished", | ||
| user=PEER_USER_ID, | ||
| bot_id="B_PEER", | ||
| ts=REPLY_TS, |
There was a problem hiding this comment.
This only exercises the already-recognized bot_id event shape. Please add the ordinary peer bot-user event shape (a user ID without bot_id or subtype) if this smoke target is meant to cover the routing incident described by #51627.
| allowed_channels: "" | ||
| ``` | ||
|
|
||
| After gateway config changes, deploys, or restarts, run this synthetic smoke target: |
There was a problem hiding this comment.
The target constructs its own passing config, mocked client, and bot identity, so it cannot validate a deployed gateway after a restart or distinguish those runtime failure buckets. Please either narrow this wording to an adapter regression test or add a runtime-aware check.
Summary
pytestcommand and failure buckets in the Slack messaging guideTesting
Closes #51630.