Skip to content

Fix Slack integration event context reads #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khaliqgant merged 2 commits into from
Jun 7, 2026
Merged

Fix Slack integration event context reads #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00128f5
fix: harden slack integration event context
khaliqgant Jun 7, 2026
8b7b26b
fix: address slack event review feedback
khaliqgant Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 78 additions & 36 deletions src/main/__tests__/integration-event-bridge.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ function makeHarness(
}
readFileFailuresBeforeSuccess?: number
failReadFile?: boolean
readFileError?: Error
sendDelayMs?: number
onSendStart?: (activeSends: number) => void
waitForDeliveryNeverSettles?: boolean
Expand Down Expand Up @@ -205,6 +206,7 @@ function makeHarness(
if (options.readFileFailuresBeforeSuccess && readFileAttempts <= options.readFileFailuresBeforeSuccess) {
throw new Error('remote file not ready')
}
if (options.readFileError) throw options.readFileError
if (options.failReadFile) throw new Error('remote file not ready')
return options.readFileResponse?.(workspaceId, path) ?? {
path,
Expand Down Expand Up @@ -606,25 +608,16 @@ test('integration events watch selected relayfile mount paths', async () => {

assert.deepEqual(harness.subscribeCalls[0].globs, [
'/slack/channels/C123ABC/**',
'/slack/channels/C123ABC__proj-cloud/**',
'/slack/channels/D*/**',
'/slack/dms/*/**',
'/slack/users/*/messages/**'
'/slack/channels/C123ABC__proj-cloud/**'
])
assert.deepEqual(harness.subscribeCalls[0].options?.pathScope, [
'/slack/channels/C123ABC/**',
'/slack/channels/C123ABC__proj-cloud/**',
'/slack/channels/D*/**',
'/slack/dms/*/**',
'/slack/users/*/messages/**'
'/slack/channels/C123ABC__proj-cloud/**'
])
assert.equal(harness.subscribeCalls[0].options?.from, 'legacy')
assert.deepEqual(integrationSubscriptionSummaries([slackIntegration])[0].watches, [
'.integrations/slack/channels/C123ABC/**',
'.integrations/slack/channels/C123ABC__proj-cloud/**',
'.integrations/slack/channels/D*/**',
'.integrations/slack/dms/*/**',
'.integrations/slack/users/*/messages/**'
'.integrations/slack/channels/C123ABC__proj-cloud/**'
])

const selectedPath = '/slack/channels/C123ABC__proj-cloud/messages/1780668000_000000/meta.json'
Expand Down Expand Up @@ -669,8 +662,7 @@ test('integration events watch selected relayfile mount paths', async () => {
assert.deepEqual(harness.sent, [])

await harness.emit(changeEvent('/slack/channels/D123ABC/messages/1780668180_000000/meta.json', 'slack'))
await waitForSent(harness, 1)
assert.deepEqual(harness.sent.map((message) => message.input.to), ['alice'])
assert.deepEqual(harness.sent, [])
})

test('slack raw-id and slug alias paths with distinct revisions inject once per logical message', async () => {
Expand Down Expand Up @@ -800,7 +792,7 @@ test('slack raw-id and slug alias duplicates suppress when one context read is s
assert.match(harness.sent[0].input.text, /Message:\nreadable Slack message/u)
})

test('slack edits after a blind alias claim still inject once the content changes', async () => {
test('slack raw-id event resolves context through mounted slug alias', async () => {
let messageText = 'original Slack message'
const harness = makeHarness(['alice'], {
readFileResponse: (_workspaceId, path) => {
Expand All @@ -827,8 +819,8 @@ test('slack edits after a blind alias claim still inject once the content change
])
})

// Raw-id copy first: every targeted read fails and the expanded event only
// carries the sparse relayfile pointer, so the injection is blind.
// Raw-id copy first: the raw targeted read fails, then the bridge retries the
// selected mounted slug alias so the injection has usable context.
await harness.emit({
...changeEvent(
'/slack/channels/C123ABC/messages/1780668000_000000/meta.json',
Expand All @@ -846,10 +838,21 @@ test('slack edits after a blind alias claim still inject once the content change
} as ChangeEvent)
await waitForSent(harness, 1, 2_500)
assert.equal(harness.sent.length, 1)
assert.match(harness.sent[0].input.text, /Message: unavailable; targeted context read did not return content\./u)
assert.match(harness.sent[0].input.text, /Message:\noriginal Slack message/u)
assert.match(harness.sent[0].input.text, /Path: \.integrations\/slack\/channels\/C123ABC__proj-cloud\/messages\/1780668000_000000\/meta\.json/u)
assert.deepEqual(harness.readFileCalls.slice(0, 2), [
{
workspaceId: 'workspace-id',
path: '/slack/channels/C123ABC/messages/1780668000_000000/meta.json'
},
{
workspaceId: 'workspace-id',
path: '/slack/channels/C123ABC__proj-cloud/messages/1780668000_000000/meta.json'
}
])

// The slug alias copy of the same record carries content: suppressed as a
// duplicate, but the claim learns the content hash.
// The slug alias copy of the same record is now a duplicate of the
// content-bearing raw delivery.
await harness.emit(changeEvent(
'/slack/channels/C123ABC__proj-cloud/messages/1780668000_000000/meta.json',
'slack',
Expand Down Expand Up @@ -1052,7 +1055,7 @@ test('historical download subscriptions can receive older remote events', async
assert.deepEqual(harness.sent.map((message) => message.input.to), ['alice'])
})

test('slack direct message event scope can be disabled', async () => {
test('slack direct message event scope is opt-in', async () => {
const harness = makeHarness()
const slackIntegration = integration({
provider: 'slack',
Expand Down Expand Up @@ -1173,11 +1176,17 @@ test('slack context falls back to expanded event data when targeted remote previ
assert.match(harness.sent[0].input.text, /Slack message event/u)
assert.match(harness.sent[0].input.text, /Author: Khaliq/u)
assert.match(harness.sent[0].input.text, /Message:\nexpanded Slack context/u)
assert.equal(harness.readFileCalls.length, 4)
assert.deepEqual(harness.readFileCalls[0], {
workspaceId: 'workspace-id',
path: messagePath
})
assert.equal(harness.readFileCalls.length, 8)
assert.deepEqual(harness.readFileCalls.slice(0, 2), [
{
workspaceId: 'workspace-id',
path: messagePath
},
{
workspaceId: 'workspace-id',
path: '/slack/channels/C123ABC/messages/1780668000_000000/meta.json'
}
])
assert.equal((harness.sent[0].input.data?.contextPreview as { kind?: string } | undefined)?.kind, 'text')
assert.equal((harness.sent[0].input.data?.contextPreview as { content?: string } | undefined)?.content, undefined)
})
Expand All @@ -1193,7 +1202,7 @@ test('slack context retries targeted remote preview before falling back to spars
integrationId: 'slack-1',
mountPaths: ['/slack/channels/C123ABC__proj-cloud'],
downloadHistoricalData: false,
scope: { notifyAgents: ['alice'] }
scope: { listenDms: true, notifyAgents: ['alice'] }
})
])
})
Expand All @@ -1217,6 +1226,45 @@ test('slack context retries targeted remote preview before falling back to spars
assert.doesNotMatch(harness.sent[0].input.text, /"deleted": false/u)
})

test('slack context stops targeted remote preview retries on auth failures', async () => {
const error = new Error('http 403 forbidden') as Error & { status: number }
error.status = 403
const harness = makeHarness(['alice'], { readFileError: error })
const messagePath = '/slack/channels/C123ABC__proj-cloud/messages/1780668000_000000/meta.json'

await withMockedNow('2026-06-05T14:00:00.000Z', async () => {
await harness.bridge.reconcile('project-1', [
integration({
provider: 'slack',
integrationId: 'slack-1',
mountPaths: ['/slack/channels/C123ABC__proj-cloud'],
downloadHistoricalData: false,
scope: { notifyAgents: ['alice'] }
})
])
})

await harness.emit({
...changeEvent(messagePath, 'slack'),
expand: async () => ({
level: 'full',
path: messagePath,
data: {
text: 'expanded Slack context'
}
})
} as ChangeEvent)
await waitForSent(harness, 1, 2_500)

assert.deepEqual(harness.readFileCalls, [
{
workspaceId: 'workspace-id',
path: messagePath
}
])
assert.match(harness.sent[0].input.text, /Message:\nexpanded Slack context/u)
})

test('slack context does not inject sparse relayfile pointer fallback as message content', async () => {
const harness = makeHarness(['alice'], { failReadFile: true })
const messagePath = '/slack/channels/D123ABC/messages/1780668000_000000/meta.json'
Expand All @@ -1228,7 +1276,7 @@ test('slack context does not inject sparse relayfile pointer fallback as message
integrationId: 'slack-1',
mountPaths: ['/slack/channels/C123ABC__proj-cloud'],
downloadHistoricalData: false,
scope: { notifyAgents: ['alice'] }
scope: { listenDms: true, notifyAgents: ['alice'] }
})
])
})
Expand Down Expand Up @@ -1600,16 +1648,10 @@ test('integration events preserve discovery mount paths', async () => {
await harness.bridge.reconcile('project-1', [slackIntegration])

assert.deepEqual(harness.subscribeCalls[0].globs, [
'/discovery/slack/**',
'/slack/channels/D*/**',
'/slack/dms/*/**',
'/slack/users/*/messages/**'
'/discovery/slack/**'
])
assert.deepEqual(integrationSubscriptionSummaries([slackIntegration])[0].watches, [
'.integrations/discovery/slack/**',
'.integrations/slack/channels/D*/**',
'.integrations/slack/dms/*/**',
'.integrations/slack/users/*/messages/**'
'.integrations/discovery/slack/**'
])

await harness.emit(changeEvent('/discovery/slack/actions/create-message/.schema.json', 'slack'))
Expand Down
68 changes: 58 additions & 10 deletions src/main/integration-event-bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,18 @@ function dedupeStrings(values: string[]): string[] {
return Array.from(new Set(values.map((value) => value.trim()).filter(Boolean))).sort()
}

function dedupeStringsInOrder(values: string[]): string[] {
const seen = new Set<string>()
const deduped: string[] = []
for (const value of values) {
const trimmed = value.trim()
if (!trimmed || seen.has(trimmed)) continue
seen.add(trimmed)
deduped.push(trimmed)
}
return deduped
}

function sameStringList(left: string[], right: string[]): boolean {
return left.length === right.length && left.every((value, index) => value === right[index])
}
Expand All @@ -400,7 +412,7 @@ function scopeBooleanDefault(scope: Record<string, unknown>, keys: string[], def

function slackListenDms(integration: ConnectedIntegration): boolean {
if (!isSlackProvider(integration.provider)) return false
return scopeBooleanDefault(integration.scope, ['listenDms', 'listenDirectMessages', 'directMessages'], true)
return scopeBooleanDefault(integration.scope, ['listenDms', 'listenDirectMessages', 'directMessages'], false)
}
Comment on lines 413 to 416
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align DM defaults across UI and bridge to keep DM listening truly opt-in.

Line 415 now defaults unset DM scope to false, but the provided UI snippets still default listenDms to true when unset. That mismatch can silently keep DM subscriptions enabled and weakens the PR’s intended privacy posture.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/integration-event-bridge.ts` around lines 413 - 416, The bridge
function slackListenDms uses scopeBooleanDefault(..., false) to make DM
listening opt-in, but the UI snippets still default listenDms to true; update
the UI/snippet/default payloads so the
listenDms/listenDirectMessages/directMessages scope defaults to false (matching
slackListenDms and scopeBooleanDefault) — ensure the UI uses the same default
value or the same helper logic when constructing the integration scope so DM
listening is opt-in across both codepaths.

Comment on lines 412 to 416
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Slack DM listening default change is a breaking behavioral change

The slackListenDms function at src/main/integration-event-bridge.ts:403 changes its default from true to false. Any existing integration configuration that omits an explicit listenDms/listenDirectMessages/directMessages scope key will silently stop receiving DM events. This is clearly intentional (test renamed from 'can be disabled' to 'is opt-in'), but downstream consumers or existing project configurations that relied on the implicit default will be affected without any migration path or warning log.

(Refers to lines 403-416)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


function pathSegments(path: string): string[] {
Expand Down Expand Up @@ -1708,6 +1720,32 @@ function slackEventContextPath(path: string): boolean {
return /^\/slack\/(?:channels|dms|users)\/[^/]+\/(?:messages|threads)\/.+\.json$/u.test(path)
}

function slackContextReadCandidatePaths(path: string, specs: SubscriptionSpec[]): string[] {
const normalizedPath = path.startsWith('/') ? path : `/${path}`
if (!slackEventContextPath(normalizedPath)) return [normalizedPath]

const match = normalizedPath.match(/^\/slack\/channels\/([^/]+)(\/(?:messages|threads)\/.+\.json)$/u)
if (!match?.[1] || !match[2]) return [normalizedPath]

const currentChannel = match[1]
const channelId = canonicalSlackChannelSegment(currentChannel)
const tail = match[2]
const candidates = [normalizedPath]

for (const spec of specs) {
for (const mountPath of spec.mountPaths) {
const mountMatch = mountPath.match(/^\/slack\/channels\/([^/]+)(?:\/|$)/u)
const mountedChannel = mountMatch?.[1]
if (!mountedChannel || canonicalSlackChannelSegment(mountedChannel) !== channelId) {
continue
}
candidates.push(`/slack/channels/${mountedChannel}${tail}`)
}
}

return dedupeStringsInOrder(candidates)
}

function slackScopeLabel(path: string): string | undefined {
const segments = pathSegments(path)
const channelIndex = segments.indexOf('channels')
Expand All @@ -1730,8 +1768,9 @@ function formatSlackIntegrationEventMessage(
const relayfilePath = eventSummaryValue(resource.path)
if (provider !== 'slack' || !relayfilePath || !slackEventContextPath(relayfilePath)) return null

const projectPath = projectIntegrationPathForRelayfilePath(relayfilePath)
const scopeLabel = slackScopeLabel(relayfilePath)
const contextPath = contextPreview?.path || relayfilePath
const projectPath = projectIntegrationPathForRelayfilePath(contextPath)
const scopeLabel = slackScopeLabel(contextPath)
const messageText = slackPreviewText(contextPreview)
const author = slackPreviewAuthor(contextPreview)
const lines = [
Expand Down Expand Up @@ -2271,7 +2310,11 @@ export class IntegrationEventBridge {
)
}

private async readEventContextPreview(projectId: string, event: ChangeEvent): Promise<EventContextPreview | undefined> {
private async readEventContextPreview(
projectId: string,
event: ChangeEvent,
matchedSpecs: SubscriptionSpec[]
): Promise<EventContextPreview | undefined> {
if (event.type === 'file.deleted' || event.type === 'relayfile.changed.summary') return undefined
const path = eventSummaryValue(event.resource.path)
if (!path) return undefined
Expand All @@ -2281,14 +2324,19 @@ export class IntegrationEventBridge {
const readDelays = slackEventContextPath(path) ? [0, ...EVENT_CONTEXT_READ_RETRY_DELAYS_MS] : [0]
const handle = await this.getWorkspaceHandle()
const client = handle.client()
const candidatePaths = slackEventContextPath(path)
? slackContextReadCandidatePaths(path, matchedSpecs)
: [path]
if (typeof client.readFile === 'function') {
for (const [index, delayMs] of readDelays.entries()) {
if (delayMs > 0) await delay(delayMs)
try {
return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path))
} catch (error) {
readFileError = error
if (index === readDelays.length - 1) break
for (const candidatePath of candidatePaths) {
try {
return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, candidatePath))
} catch (error) {
readFileError = error
if (isUnauthorizedError(error)) throw error
}
}
}
Comment on lines 2331 to 2341
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a candidate path fails with a permanent error like 403 Forbidden or 401 Unauthorized (which can happen if the raw channel ID path is outside the allowed pathScope), retrying it in subsequent delay iterations is wasteful and increases latency. We should filter out candidate paths that encounter unauthorized/forbidden errors so they aren't retried in the next rounds.

        let activeCandidates = [...candidatePaths]
        for (const [index, delayMs] of readDelays.entries()) {
          if (activeCandidates.length === 0) break
          if (delayMs > 0) await delay(delayMs)
          const nextCandidates: string[] = []
          for (const candidatePath of activeCandidates) {
            try {
              return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, candidatePath))
            } catch (error) {
              readFileError = error
              if (!isUnauthorizedError(error)) {
                nextCandidates.push(candidatePath)
              }
            }
          }
          activeCandidates = nextCandidates
        }

}
Expand Down Expand Up @@ -2401,7 +2449,7 @@ export class IntegrationEventBridge {
}

const eventMetadata = integrationEventMetadata(event)
const contextPreview = await this.readEventContextPreview(projectId, event)
const contextPreview = await this.readEventContextPreview(projectId, event, matchedSpecs)
const usesConcreteAgentTargets = uniqueRecipients.every((recipient) => !recipient.startsWith('#'))
const canTrackInjectedDelivery = usesConcreteAgentTargets && typeof bridge.sendMessageAndWaitForInjected === 'function'
const shouldTrackDedupe = canTrackInjectedDelivery
Expand Down
18 changes: 18 additions & 0 deletions src/main/integration-mounts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,24 @@ describe('IntegrationMountManager', () => {
})
})

it('mounts Slack thread context roots in mirror mode', async () => {
const manager = new IntegrationMountManager()

await manager.ensureMounted([
{
provider: 'slack',
mountPaths: ['/slack/channels/C123/threads']
}
])

expect(mock.mountInputs[0]).toMatchObject({
localDir: '/tmp/pear-home/.agentworkforce/pear/relayfile/workspaces/account-workspace-id/slack/channels/C123/threads',
remotePath: '/slack/channels/C123/threads',
localLayout: 'exact',
syncMode: 'mirror'
})
})
Comment on lines +279 to +295
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add a test case for mounting both messages and threads for the same channel.

The coding guideline requires regression tests when touching integration notifications, not just happy-path cases. Based on integrations.test.ts line 490-507, when downloadHistoricalData is false, a single channel spec expands to both /slack/channels/C123/messages and /slack/channels/C123/threads. However, there's no test verifying that mounting both together works correctly as separate mounts without conflicts.

Suggested test case 
+  it('mounts both messages and threads for the same channel as separate mounts', async () => {
+    const manager = new IntegrationMountManager()
+
+    await manager.ensureMounted([
+      {
+        provider: 'slack',
+        mountPaths: ['/slack/channels/C123/messages', '/slack/channels/C123/threads']
+      }
+    ])
+
+    expect(mock.mountInputs).toHaveLength(2)
+    expect(mock.mountInputs[0]).toMatchObject({
+      localDir: '/tmp/pear-home/.agentworkforce/pear/relayfile/workspaces/account-workspace-id/slack/channels/C123/messages',
+      remotePath: '/slack/channels/C123/messages',
+      localLayout: 'exact',
+      syncMode: 'write-only'
+    })
+    expect(mock.mountInputs[1]).toMatchObject({
+      localDir: '/tmp/pear-home/.agentworkforce/pear/relayfile/workspaces/account-workspace-id/slack/channels/C123/threads',
+      remotePath: '/slack/channels/C123/threads',
+      localLayout: 'exact',
+      syncMode: 'mirror'
+    })
+  })
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/integration-mounts.test.ts` around lines 279 - 295, Add a regression
test that ensures a single Slack channel spec expands into both messages and
threads mounts when downloadHistoricalData is false: instantiate
IntegrationMountManager, call ensureMounted with a spec like { provider:
'slack', mountPaths: ['/slack/channels/C123'], downloadHistoricalData: false }
and assert that mock.mountInputs contains two entries matching remotePath
'/slack/channels/C123/messages' and '/slack/channels/C123/threads' with distinct
localDir values and both having localLayout 'exact' and syncMode 'mirror' (use
IntegrationMountManager, ensureMounted, and mock.mountInputs to locate and
verify the mounts).

Source: Coding guidelines


it('rejects Slack command roots with traversal segments', async () => {
const manager = new IntegrationMountManager()

Expand Down
13 changes: 13 additions & 0 deletions src/main/integrations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,9 @@ describe('IntegrationsManager', () => {
})).toEqual([
'/discovery/slack',
'/slack/channels/C123/messages',
'/slack/channels/C123/threads',
'/slack/dms/D123/messages',
'/slack/dms/D123/threads',
'/slack/users/U123/messages'
])
})
Expand Down Expand Up @@ -534,6 +536,7 @@ describe('IntegrationsManager', () => {

expect(message).toContain('create writeback files under .integrations/slack/channels/C123/messages')
expect(message).toContain('Writeback command roots are mounted at .integrations/slack/channels/C123/messages')
expect(message).toContain('live thread context roots are mounted at .integrations/slack/channels/C123/threads')
expect(message).not.toContain('create writeback files under .integrations/slack/channels/C123, not under discovery')
})

Expand Down Expand Up @@ -577,6 +580,16 @@ describe('IntegrationsManager', () => {
await vi.waitFor(() => {
expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled()
})
expect(mock.integrationMountManager.ensureMounted).toHaveBeenLastCalledWith([
{
provider: 'slack',
mountPaths: [
'/discovery/slack',
'/slack/channels/C123/messages',
'/slack/channels/C123/threads'
]
}
])

finishMountReconcile()
})
Expand Down
Loading
Loading