fix(slack): recheck channel privacy before posting sensitive content (#2735)#2861
Merged
Conversation
…Private
Two assertions in the root test dir (separate from server/tests/) also
check the sendChannelMessage call shape. Updating them to expect the
new `{ requirePrivate: true }` third arg — same fix I already applied
to the server-scoped sibling in the previous commit.
631 root unit tests green; 1855 server unit tests green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All Should-Fix items from code / security / testing reviewers:
- `verifyChannelStillPrivate` return type widened from `boolean` to
`'private' | 'public' | 'unknown'`. Confirmed drift (`'public'`)
and verify failures (`'unknown'`) were both collapsed to `false`,
but they deserve different caller behavior: leak prevention vs
observability preservation. (code + security Should-Fix)
- `sendChannelMessage({ requirePrivate })` now accepts `true` OR
`'strict-public-only'`. The latter drops only on confirmed
`'public'`, letting `'unknown'` proceed so transient Slack API
failures don't silence notifications that a dropped message hurts
more than a small leak risk does. The system-error notifier uses
this mode — production errors can't go silent just because Slack
is flaky. (code + security Should-Fix)
- Error notifier emits `event: 'error_channel_drift_silenced'` at
`error` level on confirmed drift so log aggregation can alert.
Closes the observability gap where a public-flipped error
channel would silently drop system alerts. (security Should-Fix)
- `verifyChannelStillPrivate` invalidates the channel-info cache
when it observes `is_private !== true`. Admin re-privatize is
picked up on the very next send instead of waiting out the
remaining 30-min TTL. (code Should-Fix)
- `sendChannelMessage` return type grew a `SendSkipReason` union
(`'not_private' | 'privacy_unknown'`) so future skip reasons
don't widen the public API breakingly. (code Nice-to-Have)
- New `__resetChannelCacheForTests()` export + call in `beforeEach`
of `slack-channel-privacy.test.ts` — the module-level cache was
a landmine for any future test that reuses a channel ID across
cases. Dead `vi.mock(...)` block that was a no-op deleted.
(testing Must-Fix)
- Test coverage expanded: `verifyChannelStillPrivate` returning
each of the three states, cache invalidation after drift,
`requirePrivate: true` dropping on `'unknown'`, and
`'strict-public-only'` proceeding on `'unknown'` while still
dropping on `'public'`. Both error-notifier tests updated to
assert the new `'strict-public-only'` shape. (10 scenarios in
the new file, up from 5.)
Drive-by: rebase onto main picked up SDK bump to 5.12 which
restored `asset_type: 'html'` as a required discriminator. Kept
main's resolution (my earlier speculative removal was correct for
5.9.1 but wrong for 5.12).
1920 server unit tests + 631 root unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
force-pushed
the
bokelley/channel-privacy-recheck
branch
from
85f7dba to
573998e
Compare
4 tasks
bokelley
added a commit
that referenced
this pull request
Apr 23, 2026
* feat(slack): daily channel-privacy audit backstop (#2849) #2735 added a send-time recheck so a channel that drifted private → public stops receiving sensitive posts on the first send after the drift. Channels that sit idle between writes could linger in a drifted state until someone tries to post, so the fix only handled the hot path. Adds a daily audit that proactively checks each of the six admin-settings channels (billing, escalation, admin, prospect, error, editorial) against Slack and emits a structured `channel_privacy_drift_audit` log on drift or unverifiable states. When drift is found, posts a summary to `admin_slack_channel` — unless that channel itself drifted, in which case the summary is suppressed so we don't leak drift details into the now-public channel. Non-destructive: the audit does NOT auto-null settings. The send-time gate (#2735) is the enforcement path; this job is pure observability. Log aggregation alerting should key on `event: 'channel_privacy_drift_audit'`. Registered with the existing jobScheduler at 24h interval / 10min initial delay. Reuses `verifyChannelStillPrivate` + the `sendChannelMessage({ requirePrivate: 'strict-public-only' })` mode from #2861. 9 unit tests cover the orchestration logic: unconfigured channels skipped, admin-channel self-drift suppresses the summary (the core #2849 acceptance), throws collapse to 'unknown', non-destructive behavior pinned against future refactors. Typecheck clean, 1923 server unit tests pass, 631 root unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(slack): expert-review follow-ups on #2924 All Should-Fix items from the code / security / testing reviewers: - Suppress the admin summary when admin_slack_channel is 'unknown', not just 'public'. Security reviewer correctly flagged the 'strict-public-only' mode lets 'unknown' through — narrow window where the admin channel could actually be public while Slack info returns unverifiable. The #2849 spec explicitly prioritizes "not using the drifted channel as the notification surface" and structured-log aggregation is the documented backstop. Now the summary is attempted only when admin is confirmed 'private' AND sent with the strict `requirePrivate: true` gate (belt-and-braces against a drift that raced between the audit loop and the send). (security Should-Fix) - Sanitize the `err` payload in the verify-threw log — use `err instanceof Error ? .message : String(err)` so pg / library errors don't spill `.query` / `.parameters` through pino's default err serializer. Mirrors the pattern from #2830. (code Should-Fix) - Tighten `shouldLogResult` type in job-definitions.ts by importing the exported `ChannelPrivacyAuditResult` instead of the inline structural shape. (code Should-Fix) - Spy on the logger in tests and assert the structured `channel_privacy_drift_audit` record fires even when the summary is suppressed — it's the only remaining alert signal in those cases and wasn't being verified. (testing Should-Fix) - Pin the admin-'unknown' + billing-drift branch with an explicit test: confirms the summary is suppressed, both bucket assignments are correct, and the structured log captures the drift. (testing Should-Fix) - New test for multi-drift fan-out (billing + editorial both drifted → single summary mentioning both) to guard against a future refactor that sends per-channel posts. (testing Nice-to-Have) - New test for admin + billing both drifted → summary suppressed, structured log still carries BOTH settings. Distinct from the admin-only case because it proves suppression doesn't accidentally drop billing from the drifted array. (testing Nice-to-Have) - Renamed the non-destructive test for clarity: "does not write to the drifted setting — audit is pure observability". (testing Nice-to-Have) 12 scenarios total (up from 9), 1932 server + 631 root unit tests pass, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2735.
The six admin-settings notification routes (`billing-channel`, `escalation-channel`, `admin-channel`, `prospect-channel`, `error-channel`, `editorial-channel`) validate `is_private === true` at write time, but Slack lets a channel owner toggle the channel public afterward — and the server wouldn't notice. Committee lead names, billing events, escalation summaries, editorial reviewer alerts, prospect data, and system errors could all leak into a formerly-private channel that's now workspace-visible.
What changed
`server/src/slack/client.ts`:
Six sensitive notification flows now opt in:
New tests (`server/tests/unit/slack-channel-privacy.test.ts`):
Drive-by
`training-agent/task-handlers.ts:2788` had a pre-existing main typecheck break from #2795 (asset_type discriminator dropped from the SDK asset union). The bad property is removed so the precommit hook passes. Blocker fix, not a scope expansion.
Out of scope / filed as follow-up
Test plan
🤖 Generated with Claude Code