slack app: subscribe to status page

[thibaultleouay]

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
openstatus-dashboard	Error		Jul 1, 2026 7:56pm
openstatus-status-page	Error		Jul 1, 2026 7:56pm
openstatus-web	Error		Jul 1, 2026 7:56pm

[cubic-dev-ai]

22 issues found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/subscriptions/src/channels/slack.ts">

<violation number="1" location="packages/subscriptions/src/channels/slack.ts:251">
P2: Slack config validation is too permissive and accepts invalid value types. Tighten validation to require non-empty string `teamId`/`channelId`.</violation>
</file>

<file name="packages/subscriptions/src/channels/slack-store.ts">

<violation number="1" location="packages/subscriptions/src/channels/slack-store.ts:71">
P1: Delivery dedupe is non-atomic; concurrent workers can send the same update twice. The check-then-mark flow needs an atomic reservation/idempotency step.</violation>
</file>

<file name="apps/server/src/routes/slack/commands.ts">

<violation number="1" location="apps/server/src/routes/slack/commands.ts:102">
P1: List/remove flows are not scoped to Slack `team_id`, so subscription lookup/removal is keyed only by channel ID. This can cause cross-workspace data exposure or unintended unsubscribes.</violation>
</file>

<file name="packages/services/src/page-subscriber/slack.ts">

<violation number="1" location="packages/services/src/page-subscriber/slack.ts:72">
P2: Slack subscription matching is scoped only by channelId, ignoring teamId. Scope queries by both teamId and channelId to avoid cross-workspace subscription leakage/collisions.</violation>
</file>

<file name="apps/server/src/routes/slack/resolve-page.ts">

<violation number="1" location="apps/server/src/routes/slack/resolve-page.ts:61">
P2: Custom-domain resolution is case-sensitive even though DNS hostnames are case-insensitive. Mixed-case stored domains can fail to resolve from Slack input.</violation>
</file>

<sub>Note: This PR contains a large number of files. cubic only reviews up to 40 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Custom-domain resolution is case-sensitive even though DNS hostnames are case-insensitive. Mixed-case stored domains can fail to resolve from Slack input.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/server/src/routes/slack/resolve-page.ts, line 61:

<comment>Custom-domain resolution is case-sensitive even though DNS hostnames are case-insensitive. Mixed-case stored domains can fail to resolve from Slack input.</comment>

<file context>
@@ -0,0 +1,64 @@
+      customDomain: page.customDomain,
+    })
+    .from(page)
+    .where(or(eq(page.customDomain, host), eq(page.customDomain, bareHost)))
+    .get();
+  return row ?? null;
</file context>

[thibaultleouay]

@claude review this

[claude]

Claude finished @thibaultleouay's task in 3m 1s —— View job

---

PR Review: slack app: subscribe to status page

• Gathering context and reading changed files
• Reviewing core Slack subscription logic
• Reviewing DB schema changes
• Reviewing service layer
• Reviewing server routes
• Reviewing dashboard UI changes
• Writing final review

---

Overall this is a solid, well-structured feature. The delivery pipeline, DI-injectable Slack client, Redis dedup store, terminal-error auto-unsubscribe, and Slack signature verification are all well done. The unit test coverage in slack.ts, slack-blocks.ts, and the service layer is good. That said, there are a few real bugs that need fixing before merge.

---

Bugs to fix

[P1] Edit button appears for Slack subscribers but modal never opens
apps/dashboard/src/components/data-table/subscribers/data-table-row-actions.tsx:77

The Edit action is shown for all isVendorAdded rows (line 77), but editDefaults is undefined for Slack subscribers (line 64-74). The FormSheetSubscriber is only rendered when both isVendorAdded && editDefaults are truthy (line 134), so clicking Edit on a Slack subscriber silently does nothing.

Fix: exclude Slack from the Edit action, since its subscription is managed via the /openstatus slash command:

-    ...(isVendorAdded
+    ...(isVendorAdded && sub.channelType !== "slack"
       ? [{ id: "edit", label: "Edit", ... }]
       : []),

[Fix this →](https://claude.ai/code?q=In%20the%20file%20apps%2Fdashboard%2Fsrc%2Fcomponents%2Fdata-table%2Fsubscribers%2Fdata-table-row-actions.tsx%20on%20branch%20slack-app-status-page%2C%20the%20Edit%20button%20is%20shown%20for%20all%20isVendorAdded%20subscribers%20including%20Slack%2C%20but%20the%20FormSheetSubscriber%20modal%20is%20guarded%20by%20%60editDefaults%60%20which%20is%20undefined%20for%20Slack.%20Fix%20by%20changing%20line%2077%20from%20%60...(isVendorAdded%60%20to%20%60...(isVendorAdded%20%26%26%20sub.channelType%20!%3D%3D%20%22slack%22%60&repo=openstatusHQ/openstatus)

---

[P1] SQL migration: functional index expressions wrapped in backticks
packages/db/drizzle/0077_yellow_thor_girl.sql:26-27

Lines 26–27 of the migration generate:

ON `page_subscriber` (`LOWER("email")`,`page_id`) ...
ON `page_subscriber` (`LOWER("webhook_url")`,`page_id`) ...

In SQLite, backticks quote identifiers — so `LOWER("email")` is treated as a column name, not a function call. This will fail with "no such column: LOWER("email")" on migration. The expressions should be unquoted:

ON `page_subscriber` (LOWER("email"),`page_id`) ...
ON `page_subscriber` (LOWER("webhook_url"),`page_id`) ...

The Drizzle schema definition (using sql\LOWER(${table.email})``) is correct; it's the generated migration that is wrong. You may need to manually correct the generated migration file or regenerate it with a version of Drizzle that emits correct SQLite functional index syntax.

---

[P1] Thread replies and updates use subscription channelId instead of anchor channelId
packages/subscriptions/src/channels/slack.ts:160-179

When following up in a thread, the code always uses channelId from the subscription instead of anchor.channelId where the root message was actually posted. If the channel stored in the anchor ever differs from the current subscription channel (e.g., after a Slack channel migration), the thread_ts would silently fail to match:

// Uses channelId (subscription), should use anchor.channelId
const replyRes = await runSlack(sub.id, () =>
  client.postMessage({
    channel: channelId,  // anchor.channelId is safer
    thread_ts: anchor.ts,
    ...
  }),
);
// Same issue for the update call
await runSlack(sub.id, () =>
  client.update({
    channel: channelId,  // anchor.channelId
    ts: anchor.ts,
    ...
  }),
);

Fix: use anchor.channelId for threading calls.

---

Security concerns

[P2] Slack subscription queries not scoped by teamId
packages/services/src/page-subscriber/slack.ts:69-75 and apps/server/src/routes/slack/commands.ts:101-103

createSlackSubscriber, removeSlackSubscriber, and listSlackSubscribersForChannel all query only by channelId, ignoring teamId. While Slack channel IDs (C...) are effectively globally unique, this is still wrong by design — a subscriber row already stores teamId in channelConfig, and slash command payloads always carry team_id. Scoping by both prevents any theoretical cross-workspace leakage.

The RemoveSlackSubscriberInput and ListSlackSubscribersInput schemas should accept an optional teamId for scoping. The slash command handler already has team_id available (line 53 of commands.ts).

Fix this →

---

Minor issues

[P2] Non-atomic delivery dedupe
packages/subscriptions/src/channels/slack-store.ts:71-81

The isDelivered + markDelivered pattern is a read-then-mark sequence. Under concurrent workers handling the same statusReportUpdateId, two workers could both read isDelivered = false and both send the same Slack message. Consider using a Redis SET NX EX (set-if-not-exists with TTL) for an atomic reservation, or accepting the risk if workers are single-threaded in practice.

[P2] validateSlackConfig checks key existence only
packages/subscriptions/src/channels/slack.ts:248-254

"teamId" in config && "channelId" in config

This accepts { teamId: null, channelId: 42 }. Add .min(1) string checks matching the schema validators already in CreateSlackSubscriberInput.

---

Unrelated files in this PR (needs attention)

This PR includes the entire apps/private-location/ application (~6200 of the 6304 additions). These files contain several serious bugs flagged by cubic that are unrelated to the Slack subscription feature:

• Duplicate proto packages — apps/private-location/package.json uses "name": "@openstatus/proto", colliding with packages/proto/package.json. pnpm will reject this.
• Go bugs — In scheduler.go: log.Fatal on transient API errors (kills the process on any network hiccup), and time.NewTicker(0) on unknown periodicity (panics). In http_job.go: header assertion result is silently discarded.
• Duplicate proto definitions — apps/private-location/private_location/v1/ duplicates types from packages/proto/, causing potential compilation conflicts.

I'd recommend either splitting the private-location work into a separate PR or fixing the package name collision and Go bugs before merging.

---

What's well done

• Dependency injection in createSlackChannel — clean and testable; the test suite properly exercises the full delivery state machine
• Terminal error auto-unsubscribe — the TERMINAL_SLACK_ERRORS set with soft-unsubscribe on permanent failures is production-grade
• Slack signature verification — uses constant-time comparison (mismatch |= a[i] ^ b[i]) correctly
• App uninstall cleanup — tokens_revoked / app_uninstalled events properly mass-unsubscribe via json_extract(channelConfig, '$.teamId')
• DB schema — CHECK constraint on page_subscriber enforcing per-channel-type null conditions is a good safeguard
• Audit trail — all mutations properly emit audit rows with before/after snapshots, token stripped

[thibaultleouay]

@claude review this

[claude]

Claude finished @thibaultleouay's task in 1m 59s —— View job

---

PR Review: slack app: subscribe to status page

• Gather context (diff vs origin/main, prior review threads)
• Re-check previously flagged issues that are still open
• Review new/changed code since last review round
• Post final review

---

Re-reviewed against origin/main...HEAD (35 files, matches the PR description — apps/private-location is not part of this diff, so the earlier private-location findings from cubic/the prior @claude review are stale noise and can be ignored here). The P1 bugs from the first pass (Edit-button dead modal, SQL migration backtick bug, thread-reply using the wrong channelId) are all correctly fixed in 88d11d4, eab18e8, and 6247de8. Nice work.

Four issues from the first review are still open in the current HEAD (6247de8):

Still open

[P1] Slash-command list/remove not scoped by team_id
apps/server/src/routes/slack/commands.ts:101-140, packages/services/src/page-subscriber/slack.ts:230-253

listSlackSubscribersForChannel and removeSlackSubscriber still filter only on channelId (ListSlackSubscribersInput/RemoveSlackSubscriberInput in packages/services/src/page-subscriber/schemas.ts:51-64 have no teamId field). Slack's slash command payload already carries team_id (commands.ts:53) and channelConfig already stores it (slack.ts:61-65) — it's just not threaded through to the read/remove queries. Since Slack channel IDs are workspace-scoped and effectively unique this isn't currently exploitable, but it's a latent cross-workspace leak/unsubscribe risk and cheap to close: add teamId to both input schemas and filter with json_extract(channel_config, '$.teamId').

[P2] validateSlackConfig still accepts wrong value types
packages/subscriptions/src/channels/slack.ts:248-254

"teamId" in config && "channelId" in config

still passes for { teamId: null, channelId: 42 }. CreateSlackSubscriberInput already requires non-empty strings — this validator should match:

typeof (config as { teamId?: unknown }).teamId === "string" &&
(config as { teamId: string }).teamId.length > 0 &&
typeof (config as { channelId?: unknown }).channelId === "string" &&
(config as { channelId: string }).channelId.length > 0

No test exercises this path in slack.test.ts, so a regression here wouldn't be caught.

[P2] Non-atomic delivery dedupe
packages/subscriptions/src/channels/slack-store.ts:71-81

isDelivered + markDelivered is still a read-then-write pair with no atomicity. If dispatchPageUpdate/sendNotifications ever runs concurrently for the same (reportId, subscriberId, updateId) (retry + original both landing, or two dispatcher instances), both can pass the isDelivered check and post the same Slack message. An Upstash set(key, 1, { ex: TTL_SECONDS, nx: true }) returning whether the key was newly set would make this a single atomic reservation instead of two calls.

[P2] Custom-domain resolution is still case-sensitive
apps/server/src/routes/slack/resolve-page.ts:52-63

host is lowercased (resolve-page.ts:20), but the query does eq(page.customDomain, host) against a column that isn't normalized on write — customDomainSchema in packages/db/src/schema/pages/validation.ts:23-29 has no .toLowerCase() (unlike slugSchema right above it, which does). A status page saved with Status.Example.com won't resolve from /openstatus add status.example.com. Either lowercase in the query (sql\LOWER(${page.customDomain})` = host, matching the existing page_lower_custom_domain_idx) or normalize customDomainSchemato lowercase likeslugSchema`.

Confirmed fixed since last review

• Edit button now excludes Slack subscribers (data-table-row-actions.tsx:64-77)
• Migration LOWER("email")/LOWER("webhook_url") no longer backtick-quoted (0077_yellow_thor_girl.sql:26-27)
• Threaded replies/updates use anchor.channelId instead of the subscription's channelId (slack.ts:160-179)
• apps/private-location proto/Go issues from the earlier review — not present in this PR's diff at all (pre-existing on main), so no action needed here