Add bidirectional Slack status watcher#246
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughFactory now watches Slack dispatch threads: when dispatch completes, it creates a thread, seeds existing replies, subscribes to new messages (with polling fallback), dedupes parsed replies, and responds in-thread when humans request status. Configuration adds ChangesSlack Dispatch Thread Watching
Sequence DiagramsequenceDiagram
participant DispatchFlow as Dispatch Flow
participant Factory
participant SlackWriteback
participant SlackMount as SlackMount (Cloud)
participant SlackWatcher as Slack Watcher
participant HumanUser as Human in Thread
DispatchFlow->>Factory: dispatch() produces result
Factory->>Factory: ensureSlackDispatchThread(issue)
Factory->>SlackWriteback: postThread(payload)
SlackWriteback->>SlackMount: confirmPath(fileWrite)
SlackMount-->>SlackWriteback: readback with ts/thread_ts
SlackWriteback->>SlackWriteback: parseSlackTsFromContent()
SlackWriteback-->>Factory: threadId
Factory->>SlackWatcher: watch(issue, threadId)
SlackWatcher->>SlackMount: subscribe/poll for replies
HumanUser->>SlackMount: post status request in thread
SlackMount-->>SlackWatcher: new reply event
SlackWatcher->>SlackWatcher: parseSlackReply, dedup by hash
SlackWatcher->>SlackWatcher: isOwnSlackBotReply? no
SlackWatcher->>Factory: respondToSlackStatus(issue)
Factory->>SlackWriteback: reply(issueState, prUrl)
SlackWriteback->>SlackMount: confirmPath(fileWrite)
SlackMount-->>HumanUser: status response in thread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to watch in-flight factory Slack threads and reply to human status requests with live state, roster, and PR information. The reviewer feedback focuses on improving robustness and error handling: wrapping the event handler in a try-catch block to prevent unhandled rejections from breaking the polling loop, wrapping the Slack dispatch thread setup in a try-catch block to prevent auxiliary Slack failures from crashing the core dispatch operation, and updating the event identity resolver to support numeric IDs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const handle = async (event: ChangeEvent): Promise<void> => { | ||
| if (stopped || !event.resource.path.startsWith(replyPrefix)) { | ||
| return | ||
| } | ||
|
|
||
| const eventKey = eventIdentity(event) | ||
| if (!eventKey) { | ||
| if (!missingIdentityLogged) { | ||
| missingIdentityLogged = true | ||
| this.#logger.warn?.('[factory] Slack reply event missing stable identity; falling back to path/content dedupe') | ||
| } | ||
| } else if (preExistingEvents.has(eventKey)) { | ||
| return | ||
| } | ||
|
|
||
| if (preExistingPaths.has(event.resource.path)) { | ||
| return | ||
| } | ||
|
|
||
| const reply = await this.#readSlackReply(event.resource.path) | ||
| if (!reply || reply.threadTs !== threadId || reply.channelDir !== channelDir) { | ||
| return | ||
| } | ||
|
|
||
| const replyKey = `${eventKey ?? event.resource.path}:${stableHash(JSON.stringify(reply.raw))}` | ||
| if (seenReplies.has(replyKey)) { | ||
| this.#logger.debug?.('[factory] suppressed duplicate Slack reply payload', { issue: record.issue.key, path: event.resource.path }) | ||
| return | ||
| } | ||
| seenReplies.add(replyKey) | ||
|
|
||
| if (reply.isBot) { | ||
| return | ||
| } | ||
|
|
||
| await this.#respondToSlackStatus(record, threadId) | ||
| } |
There was a problem hiding this comment.
If an error occurs during the processing of a Slack reply (e.g., due to transient network issues when calling the Fleet API or Slack API), the unhandled rejection will propagate and abort the for...of loop in the polling mechanism. Since the cursor is advanced before processing the events, any remaining events in the current page will be silently skipped and never processed. Wrapping the body of the handle function in a try...catch block ensures that an error in one event does not disrupt the processing of other events and prevents unhandled promise rejections in the subscription callback.
const handle = async (event: ChangeEvent): Promise<void> => {
try {
if (stopped || !event.resource.path.startsWith(replyPrefix)) {
return
}
const eventKey = eventIdentity(event)
if (!eventKey) {
if (!missingIdentityLogged) {
missingIdentityLogged = true
this.#logger.warn?.('[factory] Slack reply event missing stable identity; falling back to path/content dedupe')
}
} else if (preExistingEvents.has(eventKey)) {
return
}
if (preExistingPaths.has(event.resource.path)) {
return
}
const reply = await this.#readSlackReply(event.resource.path)
if (!reply || reply.threadTs !== threadId || reply.channelDir !== channelDir) {
return
}
const replyKey = (eventKey ?? event.resource.path) + ":" + stableHash(JSON.stringify(reply.raw))
if (seenReplies.has(replyKey)) {
this.#logger.debug?.('[factory] suppressed duplicate Slack reply payload', { issue: record.issue.key, path: event.resource.path })
return
}
seenReplies.add(replyKey)
if (reply.isBot) {
return
}
await this.#respondToSlackStatus(record, threadId)
} catch (error) {
this.#logger.error?.('[factory] failed to handle Slack reply event', error)
}
}| async #ensureSlackDispatchThread(record: InFlightIssue, result: DispatchResult): Promise<void> { | ||
| if (!this.#slack || !this.#config.slack || result.dryRun) { | ||
| return | ||
| } | ||
|
|
||
| const key = issueKey(record.issue) | ||
| if (this.#slackThreadIds.has(key) || this.#slackWatcherStarts.has(key)) { | ||
| await this.#slackWatcherStarts.get(key) | ||
| return | ||
| } | ||
|
|
||
| const start = this.#postAndWatchSlackDispatchThread(record, result) | ||
| this.#slackWatcherStarts.set(key, start) | ||
| try { | ||
| await start | ||
| } finally { | ||
| this.#slackWatcherStarts.delete(key) | ||
| } | ||
| } |
There was a problem hiding this comment.
If posting to Slack or watching the Slack thread fails (e.g., due to rate limits or Slack API downtime), the error will propagate up and cause the entire dispatch operation to fail. Since the agents have already been spawned and the Linear state has been updated, failing the dispatch at this stage is undesirable. Slack status watching is an auxiliary feature and should be treated as best-effort. Wrapping the await start call in a try...catch block inside #ensureSlackDispatchThread prevents Slack-related failures from crashing the core dispatch loop.
async #ensureSlackDispatchThread(record: InFlightIssue, result: DispatchResult): Promise<void> {
if (!this.#slack || !this.#config.slack || result.dryRun) {
return
}
const key = issueKey(record.issue)
if (this.#slackThreadIds.has(key) || this.#slackWatcherStarts.has(key)) {
try {
await this.#slackWatcherStarts.get(key)
} catch {
// Ignored, logged by the initiator
}
return
}
const start = this.#postAndWatchSlackDispatchThread(record, result)
this.#slackWatcherStarts.set(key, start)
try {
await start
} catch (error) {
this.#logger.warn?.("[factory] failed to establish Slack dispatch thread for " + record.issue.key, error)
} finally {
this.#slackWatcherStarts.delete(key)
}
}| const eventIdentity = (event: ChangeEvent): string | undefined => { | ||
| const record = event as unknown as Record<string, unknown> | ||
| const id = stringValue(record.id) ?? stringValue(record.event_id) ?? stringValue(record.seq) | ||
| return id ? `event:${id}` : undefined | ||
| } |
There was a problem hiding this comment.
The stringValue helper strictly checks typeof value === 'string'. If the event identity (such as seq or id) is a number, eventIdentity will return undefined. This causes the system to fall back to path/content deduplication and log unnecessary warnings. Updating eventIdentity to handle both string and number IDs ensures reliable event deduplication.
| const eventIdentity = (event: ChangeEvent): string | undefined => { | |
| const record = event as unknown as Record<string, unknown> | |
| const id = stringValue(record.id) ?? stringValue(record.event_id) ?? stringValue(record.seq) | |
| return id ? `event:${id}` : undefined | |
| } | |
| const eventIdentity = (event: ChangeEvent): string | undefined => { | |
| const record = event as unknown as Record<string, unknown> | |
| const rawId = record.id ?? record.event_id ?? record.seq | |
| const id = typeof rawId === 'string' || typeof rawId === 'number' ? String(rawId) : undefined | |
| return id ? "event:" + id : undefined | |
| } |
|
Implemented fixes in the PR scope. Changes:
Addressed comments
Advisory NotesNone. Validation run locally:
I did not print |
|
pr-reviewer could not complete review for #246 in AgentWorkforce/pear. |
|
Reviewed and fixed the current checkout defined by Changes made:
Addressed comments
Advisory Notes
VerificationPassed locally:
Not printing |
kjgbot
force-pushed
the
factory-sdk/sb-bidirectional-slack
branch
from
f111140 to
dc5564f
Compare
|
Fixed one validated PR issue: Slack reply event identity now recognizes Relayfile’s camelCase Changed: Addressed comments
Advisory NotesNone. Local validation completed:
I did not print |
|
Reviewed PR #246 against Addressed comments
Validation run:
Note: the first Playwright attempt failed because Chromium was not installed locally; after installing Chromium, redraw passed and fidelity passed when rerun in CI’s sequential style. I did not run the macOS |
kjgbot
force-pushed
the
factory-sdk/sb-bidirectional-slack
branch
from
b1c5bed to
fd52281
Compare
1 participant
Summary
Verification
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.json