Skip to content

Add retry-safe live Slack contract tests - #406

Open
alex-clickhouse wants to merge 6 commits into
alex/slack-channel-notificationsfrom
alex/slack-channel-live-tests
Open

alex-clickhouse wants to merge 6 commits into
alex/slack-channel-notificationsfrom
alex/slack-channel-live-tests

Conversation

@alex-clickhouse

@alex-clickhouse alex-clickhouse commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Part 4 of the Slack channel stack. Adds tests that run against a real Slack
workspace, and the harness they need to be trustworthy. Everything here skips
unless the credentials are set, so a fork and the ordinary suite are
unaffected.

Stack

  1. Shared chat-channel foundations #404: shared chat-channel foundations
  2. Slack channel with coherent runtime and access guardrails #403: Slack channel, access guardrails, and runtime ownership
  3. Deliver notifications through Slack #405: Slack notification delivery
  4. Add retry-safe live Slack contract tests #406: real-Slack contract tests and diagnostics (this PR)

What only a real workspace can answer

The unit tests prove the channel is self-consistent against a fake. They
cannot tell you whether Slack accepts a Block Kit payload, whether an emoji
short name exists, whether users.info withholds an email rather than
failing, or whether an event survives the whole trip from a keystroke to an
InboundMessage. That is what these cover.

Two facts about Slack that shape the harness

Slack gives each event to exactly one of an app's open connections. Two
test runs at once therefore steal each other's events rather than both seeing
them. So CI serializes on the workspace, not on the branch: the concurrency
group is a fixed name, and an already-running job is allowed to finish rather
than being cancelled, because cancelling it would leave its events
unacknowledged.

Slack retries anything left unacknowledged, on a schedule: immediately,
then at about +60s and +5min. A run that ends with unacknowledged events
therefore poisons the next run, which sees them arrive as if they were its
own. Two consequences shape the layout:

  • The outbound tests post, edit, upload, and delete. Each of those produces an
    event. They run with a socket open that acknowledges and discards
    everything, so their own traffic never comes back later.
  • The inbound tests are the only thing that interprets events, and they run in
    a separate process so the two roles cannot overlap.

Making the tests mean something

A refusal test has to prove the event arrived. Waiting and finding the
router empty does not distinguish a guardrail that refused the message from an
event Slack never delivered, one it handed to another connection, or one the
harness dropped as stale. Each refusal test now waits for the envelope to
reach the channel first, then asserts nothing came out of it, and says which
of the two failed. Without that, deleting the access policy altogether left
these tests green.

Old events are dropped, in the harness only. A test only ever waits for a
message it just posted, so a retried envelope or one older than the delivery
timeout belongs to an earlier test or an earlier run. Production does the
opposite on purpose: handling a retry is how a message survives a restart.

Event age is measured on Slack's clock. The age of an event is the gap
between a timestamp Slack wrote and a reading of the local clock, which are
two different clocks. A runner drifted ahead of Slack by more than the cutoff
would call every fresh event stale and drop it, and on the refusal tests that
reads as a guardrail working. The offset is measured from a probe's own
timestamp instead of assumed.

Readiness is a fresh event, not a connected socket. While retries are
outstanding a newly opened socket is deaf to new events for 20 to 30 seconds.
Setup posts a probe and waits for that message's own timestamp to arrive
unretried, re-probing until it lands, then waits for its deletion event too.
That makes the round trip a fence: once it returns, it has left nothing of its
own unacknowledged.

Cleaning up after itself

The suite deletes the messages, reactions, and files it creates. Messages go
through chat.delete; a file is not a message, so uploads are tracked by id
and removed with files.delete. Items are recorded before the assertions that
could fail, so a failure part way through does not leave data behind. An
interrupted run can still leave some, which is why the docs call for a
throwaway workspace.

Diagnostics

The harness emits one-line SLACK_LIVE JSON records for connections, probes,
retries, event ages and types, and per-socket summaries. Secrets reach them
through a field allow-list, and CI scopes the tokens to the two steps that
need them so GitHub masks them as well.

docs/testing-slack.md covers the scratch-workspace setup, the required
scopes, and the two-process layout.

@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 8a17b88 to e5400e9 Compare August 24, 2026 07:15
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch 3 times, most recently from 41ea8ab to d05ba74 Compare August 24, 2026 07:43
@alex-clickhouse
alex-clickhouse marked this pull request as ready for review August 24, 2026 09:32
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch 2 times, most recently from 6f2cd50 to eaeddd5 Compare August 24, 2026 11:17
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch 3 times, most recently from cc477da to fb11c30 Compare August 24, 2026 11:53
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from fb11c30 to 54ccbdc Compare August 24, 2026 12:38
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 54ccbdc to 8efe050 Compare August 25, 2026 07:54
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 8efe050 to 3a9f203 Compare August 25, 2026 08:19
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 3a9f203 to 61ab950 Compare August 25, 2026 09:41
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 61ab950 to 3abe891 Compare August 25, 2026 09:44
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 3abe891 to b91d5ca Compare August 25, 2026 10:59
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from b91d5ca to 4b61e4c Compare August 25, 2026 13:48
alex-clickhouse and others added 6 commits September 16, 2026 15:20
Exercise the channel against a real workspace in a globally serialized workflow. Keep all outbound mutations acknowledged, fence phase transitions with fresh probes, discard stale test-only callbacks, and emit structured Socket Mode diagnostics without message content or credentials.
The four guardrail tests waited and asserted the router stayed empty. An
event Slack never delivered, one it gave to another connection, and one the
harness dropped as stale all look identical from there, so removing the
policy altogether left them green. Wait for the envelope to reach the
channel first, then assert nothing came out of it, and say which of the two
failed. The settle after arrival covers dispatch rather than delivery, so
it no longer sits at under a third of the delivery timeout.

The staleness cutoff subtracted a stamp Slack wrote from a reading of the
runner's clock. A runner ahead of Slack by more than the cutoff called every
fresh event stale and dropped it, which on those same refusal tests reads as
a guardrail doing its job. Measure the offset from a probe's own ts and
treat an uncalibrated harness as having nothing stale.

An upload is not a message, so chat.delete never removed the file the
send_file test creates and every run added another to the scratch channel.
Record file ids and remove them with files.delete. Track the streaming
placeholder before the edits and the bot's reply before its text is
checked, so a failure part way through leaves nothing behind.

The integration test's probe client returned a counter where Slack returns
epoch seconds, which left the cutoff it exercises uncalibrated. It also now
asserts the fresh probe arrived, not only that the stale one did not.
The reaction test cached a bare conversation target. Nothing in the channel
writes one for a shared channel: a top-level message is the root of its own
thread, so every entry carries a thread ts. Only a notification card sits at
conversation level, and a reaction there now has no session to join.

Cache the anchor against its own thread, which is the target the channel
would have stored for it.
Two live tests asserted less than they read.

**The reconnect test never ran the watchdog.** `build_channel` returns a bare
`SlackChannel`, and the watchdog task exists only where it is created, in
`SlackRuntime._enable_locked`, so nothing in that test was watching the
socket. The interval it lowered was `slack.WATCHDOG_INTERVAL`, while the loop
reads the name `slack_runtime` bound at import, so the write had no reader
either. What restored the connection is the SDK, which reconnects a session it
finds closed.

The last live run on this branch says so in its own diagnostics: one client
built, one `hello`, and that `hello` arrives 5.8 seconds after the break. A
watchdog repair builds a second client, because `rebuild_transport` closes the
old one and replaces the object.

The test now builds its channel through `SlackRuntime.reconcile`, lowers the
interval the loop actually reads, and turns the SDK's reconnect off, which
leaves the watchdog as the only thing that can recover the socket. What it
asserts is a rebuilt transport rather than a connected one, since that is the
half only the watchdog does. Delete `_watchdog` and the test fails.

`RecordingRouter` grows the three registry methods the runtime calls on it,
and the diagnostics wrapper moves into `instrument_channel` so a channel the
runtime built can be observed too — including the socket it rebuilds, because
the wrapper sits on the instance.

**The scope test asked a bot user for its email.** It read the bot's own id
from `auth_test` and asserted `profile.email` on it as the precondition for
the comparison. A bot user has no email, so the precondition cannot hold, and
the failure would have read "the main bot token needs users:read.email" —
naming a scope that is present.

It has never run. The repository holds four Slack secrets and
`NERVE_SLACK_TEST_BOT_TOKEN_NO_EMAIL` is not one of them, so the last live run
logged the test SKIPPED with an empty variable. The subject is the user
token's owner now, which is a person with an address the scope can withhold,
and the class carries `requires_inbound` to say it needs that credential.
`docs/testing-slack.md` says why the subject has to be a person, and that CI
skips the test.

Verification: 3739 passed, 21 skipped, no failures. The skips are the live
tests, which need a workspace this machine has no credentials for, so both
changed tests were checked by reading the code they drive: `reconcile` returns
"enabled" and starts the task, `rebuild_transport` assigns a fresh client
before it connects it (hence the test waits for both), `transport_connected`
turns a dead socket into False rather than an exception, and
`auto_reconnect_enabled` is the flag the SDK's own monitor reads before it
reconnects.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alex-clickhouse
alex-clickhouse force-pushed the alex/slack-channel-live-tests branch from 4889ef1 to fc823f0 Compare September 16, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant