test(daemon): stop the unit suite from reaching slack.com - #1083
Conversation
`Daemon.start()` builds a real `@slack/web-api` client for every configured Slack integration, so booting a daemon in a unit test dialled out. Measured over the whole suite: 82 real Slack apps constructed, concentrated in two files. That is a live flake source, not just latency. On CI the API answers `invalid_auth` quickly, but a slow answer is a failure — the one test that boots two distinct apps has twice the exposure and timed out at the 5s budget on this branch's own CI. Behind a proxy the request fails at the transport layer instead, so `p-retry` spends its 1s + 2s ladder and `start()` takes ~4s against ~40ms. `SlackConnection` already accepted a `factory`; the daemon simply never passed one. Expose it as `slackAppFactory` — injected by tests only, exactly like the existing `hostFactory` — and give the suites an inert app in `test/fakes/slack-app.ts`. Effect on the two dominant files: ~195s and 5 failures become 10.4s and 46 passing. Those 5 had been written off as environment flakes; they were this. Real Slack apps across the suite drop 82 to 30 — 3 of the remainder are `connection.test.ts` deliberately exercising default construction, and the rest arrive on async reconnect paths that lose the test frame, so they are not attributed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
![agentconnect-md-test[bot]](https://avatars.githubusercontent.com/in/4157507?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found one blocking issue: the new slackAppFactory seam does not cover every Daemon-owned Slack client. Direct startup and runtime reconcile use it, but background retries omit it and send-only/shared connections discard it. Those paths can still run auth.test() against the real Slack Web API from a unit test, so this PR does not yet establish the advertised no-network guarantee. Please propagate the factory through the retry path and make the send-only client construction injectable as well.
Verification completed on the trusted synthetic merge for head eaae8e7: the three focused files passed (82 tests), daemon typecheck passed, and ESLint passed on all changed files.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| hostFactory?: (agent: Agent, onUpdate: (sid: string, u: any) => void) => AcpHost | ||
| /** Builds the Slack app each connection drives. Injected by tests ONLY — unset, connections | ||
| * build the real client and reach slack.com, which is not something a unit suite should need. */ | ||
| slackAppFactory?: SlackAppFactory |
There was a problem hiding this comment.
[P2] Cover every Daemon-owned Slack connection lifecycle with this seam. retrySlackConnection() still calls new SlackConnection(...) without this.opts.slackAppFactory, so a failed injected startup later retries with the real Bolt client. The send-only/shared path also passes this factory, but SlackConnection ignores it when deps.sendOnly is true and hard-codes sendOnlyApp(), whose auth.test() reaches Slack. Tests exercising either path therefore remain network-dependent. Please thread the factory through retries and make send-only construction use an injectable factory too.
…actory Addresses review on #1083: the seam only covered direct startup and runtime reconcile. `retrySlackConnection()` rebuilt with the real Bolt client, and `SlackConnection` ignored an injected factory outright when `deps.sendOnly` was set, hard-coding `sendOnlyApp` whose `auth.test()` reaches Slack. - Pass the factory on the retry path too, so a failed injected startup does not come back with the real client. - Lift the constructor's default into `realSocketModeApp` and make `factory` a plain optional, so an injected factory wins in BOTH modes while production, which passes none, keeps today's send-only/socket-mode branch exactly. - Inject the fake in the 29 further suites that boot a daemon with a Slack integration. The earlier pass missed them because these construct on async continuations that lose the test frame, so a stack-trace attribution found only four files. Real Slack apps across the suite: 82 originally, 30 after the first pass, now 4 — 3 are `connection.test.ts` deliberately exercising default construction, which makes no call, and 1 is a reconcile-watch path still to be traced. Failures over the full suite drop from 15 to 3. `daemon-duty-replacement` and `daemon-duty-fence` were network-dependent too, which is another 7 I had been attributing to the sandbox. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
There was a problem hiding this comment.
The synchronized revision resolves the prior blocker. slackAppFactory is now propagated into background retries, and an injected factory takes precedence for send-only/shared connections, so every Daemon-owned Slack lifecycle can remain network-free in unit tests. I found no new blocking regression.
Verification on trusted head ba9ae6d: 241 focused Slack/daemon lifecycle/shared-mode tests passed; daemon typecheck, ESLint, and Prettier passed. The broader daemon run reached 3,586 passing tests; its remaining failures were caused by this review sandbox denying Git/process execution and were unrelated to the patch.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
2 participants
Follow-up to #1076.
Daemon.start()builds a real@slack/web-apiclient for every configured Slack integration, so booting a daemon in a unit test dialled out to slack.com. Instrumenting the default factory across a full run found 82 real Slack apps constructed.Why this matters — it is a flake source, not latency
On CI the API answers
invalid_authfast, so the time saved is small: roughly 5–20s across the job. That is not the reason to do this.distinctTokens: true) has twice the exposure and timed out at the 5s budget on this branch's own CI during perf(daemon): run the unit suite with a shared module registry #1076.p-retryspends its 1s + 2s ladder andstart()takes ~4s against ~40ms.Full-suite failures drop from 15 to 3. Twelve failures I had been attributing all along to "sandbox environment flakes" were this bug:
daemon-agent-mention-routing(4),daemon-platform-authorship(1),daemon-duty-replacement(6),daemon-duty-fence(1). Onlyacp-matrix,cp-agent-registryandcp/gh-shimare genuinely environmental.The change
SlackConnectionalready accepted afactory— the daemon never passed one. Exposed asslackAppFactoryonDaemonopts, injected by tests only, mirroring the existinghostFactoryseam ("injected hostFactory, never by the production CLI/config surface").Review on the first revision caught two gaps, both real:
retrySlackConnection()rebuilt with the real Bolt client, so a failed injected startup came back over the network.SlackConnectionignored an injected factory outright whendeps.sendOnlywas set, hard-codingsendOnlyAppwhoseauth.test()reaches Slack.Fixed by lifting the constructor's default into
realSocketModeAppand makingfactorya plain optional, so an injected factory wins in both modes while production — which passes none — keeps today's branch exactly.Attribution note
My first pass used stack traces and found only 4 test files. That was wrong: most of these construct on async continuations that lose the test frame. Grepping by content (
platform: 'slack'+new Daemon() found 29 more.Results
daemon-agent-mention-routing+daemon-platform-authorshipThe local wall-clock drop is mostly the proxy retry ladder, which CI does not have — expect the smaller number there. CPU is essentially unchanged (~1m52s user), which is the point: what disappeared was idle waiting, not work.
Of the 4 remaining, 3 are
connection.test.tsdeliberately exercising default construction (construction alone makes no call) and 1 is areconcile-watchpath I have not traced. So this is not a hard no-network guarantee — an earlier draft of this description claimed one, which was wrong.Test plan
tsc -p tsconfig.typecheck.json0 errors;eslint,prettier --checkcleanba9ae6d3🤖 Generated with Claude Code
https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu