fix: resolve #825 — slack-primitive: persist askQuestion exchanges to workflow run record#834
Conversation
…n exchanges to workflow run record Fixes AgentWorkforce#825 Signed-off-by: Dinh Truong (SlncTrZ) <46520299+SlncTrZ@users.noreply.github.com>
…n exchanges to workflow run record Fixes AgentWorkforce#825 Signed-off-by: Dinh Truong (SlncTrZ) <46520299+SlncTrZ@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds persistence for Slack ask-question exchanges to workflow run records. It introduces two TypeScript interfaces defining the run record and exchange schemas, and implements a handler that waits for Slack replies, constructs exchange objects with question/reply/metadata, and persists them to storage. ChangesSlack Ask-Question Exchange Persistence
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const exchange = { | ||
| question: step.input.question, | ||
| timestamp: new Date(), | ||
| reply: result.reply, | ||
| timeout: result.timeout === true | ||
| }; |
There was a problem hiding this comment.
🔴 Exchange object fields don't match the AskQuestionExchange interface from the same PR
The exchange object constructed in handleSlackAskQuestion uses field names and types that are completely misaligned with the AskQuestionExchange interface defined in packages/runner/src/types/workflow-run-record.ts:10-20:
question→ should bequestionTexttimestamp(Date) → should bequestionTimestamp(string)reply→ should bereplyTexttimeout(boolean) → should betimeoutOutcome('timeout' | 'cancelled' | null)- Missing required field
channel
Since updateRunAskQuestionExchanges doesn't exist yet (the persistence module is unimplemented), there is no compile-time type constraint on this object — the mismatch will silently persist until the persistence layer is wired up, at which point either this code will break or the wrong data will be stored.
Prompt for agents
The exchange object built at slack-ask-question-handler.ts:11-16 uses field names (question, timestamp, reply, timeout) that don't match the AskQuestionExchange interface defined at workflow-run-record.ts:10-20 (questionText, questionTimestamp, replyText, timeoutOutcome, channel). The handler should import AskQuestionExchange and construct an object conforming to it. Specifically: (1) rename 'question' to 'questionText', (2) rename 'timestamp' to 'questionTimestamp' and convert it to a string, (3) rename 'reply' to 'replyText', (4) replace the boolean 'timeout' with 'timeoutOutcome' using the correct union type ('timeout' | 'cancelled' | null), (5) add the required 'channel' field (likely from step.input or step.config). The import of AskQuestionExchange should come from '../types/workflow-run-record'.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/runner/src/execution/slack-ask-question-handler.ts`:
- Around line 9-20: Remove the no-op try/catch wrapper around the await calls:
delete the try { ... } catch (error) { throw error; } block and leave the two
awaits as-is so errors from step.waitForReply(timeoutMs) and
updateRunAskQuestionExchanges(runId, exchange) propagate naturally; keep
construction of the exchange object (question, timestamp, reply, timeout) and
preserve existing variable names (step, timeoutMs, exchange, runId).
- Around line 11-17: The persisted exchange object currently created in
slack-ask-question-handler (variable exchange) uses
question/timestamp/reply/timeout which doesn't match the AskQuestionExchange
schema; update construction to populate the schema fields expected by
updateRunAskQuestionExchanges: set channel from the Slack context (e.g.,
step.input.channel or handler context), questionTimestamp to the original
timestamp (new Date() or step.input.timestamp if available), questionText from
step.input.question, replyText from result.reply, replyTimestamp from
result.replyTimestamp or now when reply arrived, and timeoutOutcome from
result.timeout (map boolean to the enum/value expected); ensure the object you
pass to updateRunAskQuestionExchanges uses these property names (channel,
questionTimestamp, questionText, replyText, replyTimestamp, timeoutOutcome) so
typing and persisted forensic fields are complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce907ad5-ffa5-4f1a-b510-b36412368bb7
📒 Files selected for processing (2)
packages/runner/src/execution/slack-ask-question-handler.tspackages/runner/src/types/workflow-run-record.ts
| try { | ||
| const result = await step.waitForReply(timeoutMs); | ||
| const exchange = { | ||
| question: step.input.question, | ||
| timestamp: new Date(), | ||
| reply: result.reply, | ||
| timeout: result.timeout === true | ||
| }; | ||
| await updateRunAskQuestionExchanges(runId, exchange); | ||
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify useless catch/rethrow patterns in TS files
ast-grep --lang ts --pattern $'try { $$$BODY } catch ($ERR) { throw $ERR; }'Repository: AgentWorkforce/relay
Length of output: 1143
Remove the no-op catch/rethrow wrapper.
The try-catch block at lines 9–20 catches and immediately rethrows the error unchanged, which violates the no-useless-catch rule and adds unnecessary noise. Remove the entire try-catch and let the error propagate naturally.
🧰 Tools
🪛 ESLint
[error] 9-20: Unnecessary try/catch wrapper.
(no-useless-catch)
🤖 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 `@packages/runner/src/execution/slack-ask-question-handler.ts` around lines 9 -
20, Remove the no-op try/catch wrapper around the await calls: delete the try {
... } catch (error) { throw error; } block and leave the two awaits as-is so
errors from step.waitForReply(timeoutMs) and
updateRunAskQuestionExchanges(runId, exchange) propagate naturally; keep
construction of the exchange object (question, timestamp, reply, timeout) and
preserve existing variable names (step, timeoutMs, exchange, runId).
| const exchange = { | ||
| question: step.input.question, | ||
| timestamp: new Date(), | ||
| reply: result.reply, | ||
| timeout: result.timeout === true | ||
| }; | ||
| await updateRunAskQuestionExchanges(runId, exchange); |
There was a problem hiding this comment.
Persisted exchange payload does not match the new run-record schema.
On Line 11–16, the object uses question/timestamp/reply/timeout, but the new AskQuestionExchange schema expects fields like channel, questionTimestamp, questionText, replyText, replyTimestamp, and timeoutOutcome. This will either fail typing upstream or persist incomplete data that misses #825’s required forensic fields.
Suggested mapping update
+import type { AskQuestionExchange } from '../types/workflow-run-record';
- const exchange = {
- question: step.input.question,
- timestamp: new Date(),
- reply: result.reply,
- timeout: result.timeout === true
- };
+ const exchange: AskQuestionExchange = {
+ channel: step.input.channel,
+ questionTimestamp: step.input.questionTs,
+ questionText: step.input.question,
+ replierUserId: result.replierUserId,
+ replyText: result.reply,
+ replyTimestamp: result.replyTs,
+ matchedChoices: result.matchedChoices,
+ matchedRegexGroups: result.matchedRegexGroups,
+ timeoutOutcome: result.timeout ? 'timeout' : null,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exchange = { | |
| question: step.input.question, | |
| timestamp: new Date(), | |
| reply: result.reply, | |
| timeout: result.timeout === true | |
| }; | |
| await updateRunAskQuestionExchanges(runId, exchange); | |
| import type { AskQuestionExchange } from '../types/workflow-run-record'; | |
| const exchange: AskQuestionExchange = { | |
| channel: step.input.channel, | |
| questionTimestamp: step.input.questionTs, | |
| questionText: step.input.question, | |
| replierUserId: result.replierUserId, | |
| replyText: result.reply, | |
| replyTimestamp: result.replyTs, | |
| matchedChoices: result.matchedChoices, | |
| matchedRegexGroups: result.matchedRegexGroups, | |
| timeoutOutcome: result.timeout ? 'timeout' : null, | |
| }; | |
| await updateRunAskQuestionExchanges(runId, exchange); |
🤖 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 `@packages/runner/src/execution/slack-ask-question-handler.ts` around lines 11
- 17, The persisted exchange object currently created in
slack-ask-question-handler (variable exchange) uses
question/timestamp/reply/timeout which doesn't match the AskQuestionExchange
schema; update construction to populate the schema fields expected by
updateRunAskQuestionExchanges: set channel from the Slack context (e.g.,
step.input.channel or handler context), questionTimestamp to the original
timestamp (new Date() or step.input.timestamp if available), questionText from
step.input.question, replyText from result.reply, replyTimestamp from
result.replyTimestamp or now when reply arrived, and timeoutOutcome from
result.timeout (map boolean to the enum/value expected); ensure the object you
pass to updateRunAskQuestionExchanges uses these property names (channel,
questionTimestamp, questionText, replyText, replyTimestamp, timeoutOutcome) so
typing and persisted forensic fields are complete.
1 participant
Summary
fix: resolve #825 — slack-primitive: persist askQuestion exchanges to workflow run record
Problem
Severity:
High| File:packages/runner/src/types/workflow-run-record.tsAdd an optional
askQuestionExchangesarray field to the workflow run record. Each element corresponds to one askQuestion step execution, storing channel, question timestamp, question text, replier user ID, reply text, reply timestamp, matched choices/regex groups, and timeout outcome. This enables post-mortem inspection without relying on Slack's message retention.Solution
Changes
packages/runner/src/types/workflow-run-record.ts(new)packages/runner/src/execution/slack-ask-question-handler.ts(new)Testing