feat: Slack-driven coding agent with Chat SDK#251
Merged
sweetmantech merged 21 commits intotestfrom Mar 6, 2026
Merged
Conversation
Fetch attachment download URLs from Resend, append them to email text so sandbox/tools can download files, and inject image parts into agent messages so the LLM can visually process images. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Construct a proper UserModelMessage instead of spreading a generic ModelMessage, fixing the discriminated union type incompatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove image part injection into agent messages. Attachment download URLs in the email text are sufficient — OpenClaw in the sandbox can fetch any file type directly. This avoids signed URL access issues and unsupported format errors (e.g. SVG) with LLM vision APIs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add Chat SDK (chat, @chat-adapter/slack, @chat-adapter/github, @chat-adapter/state-ioredis) - Create bot singleton with Slack + GitHub adapters and ioredis state - Add config for submodule base branches and channel/user allowlists - Add event handlers: onNewMention, onSubscribedMessage, onMergeAction - Add callback handler with secret verification and status dispatching - Add Zod validator for callback payloads - Add webhook route at /api/coding-agent/[platform] for Slack + GitHub - Add callback route at /api/coding-agent/callback for task notifications - Add trigger wrappers: triggerCodingAgent, triggerUpdatePR - 36 tests covering config, bot, handlers, validator, and callback handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Logs at each step to identify where the email response pipeline stops: memory validation, CC check, response generation, and send. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a coding-agent feature: a singleton Chat bot with Slack and GitHub adapters, Redis-backed thread state, new Next.js webhook routes and callback handler, Trigger.dev task triggers, Zod validation for callbacks, and event handlers for mentions, subscribed messages, and merge actions. Changes
Sequence DiagramsequenceDiagram
actor User as User (Slack)
participant Slack as Slack Adapter
participant Bot as CodingAgent Bot
participant State as Redis State
participant Trigger as Trigger.dev
participant GitHub as GitHub API
User->>Slack: Mention bot with prompt
Slack->>Bot: onNewMention event
Bot->>State: set status="running", save prompt, slackThreadId
Bot->>Slack: post "Starting..." message
Bot->>Trigger: triggerCodingAgent(payload)
Trigger->>Trigger: run coding-agent task (async)
Trigger->>Bot: POST /api/coding-agent/callback { status: "pr_created", prs, branch, snapshotId }
Bot->>State: update status="pr_created", save prs, branch, snapshotId
Bot->>Slack: post PR summary with links
User->>Slack: reply with feedback
Slack->>Bot: onSubscribedMessage event
Bot->>State: set status="updating"
Bot->>Trigger: triggerUpdatePR(feedback, snapshotId, branch, repo, callbackThreadId)
Trigger->>GitHub: apply changes / update PR
Trigger->>Bot: POST /api/coding-agent/callback { status: "updated", snapshotId }
Bot->>Slack: post update confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
- Replace `logger: console` with `new ConsoleLogger()` from chat SDK (Console lacks `child()` method) - Fix `message.author.id` → `message.author.userId` to match chat SDK's Author interface - Cast thread state in onMergeAction to CodingAgentThreadState for type safety - Update triggerUpdatePR payload: `repo: string` instead of `prs: CodingAgentPR[]` to match simplified task schema - Update onSubscribedMessage to pass `state.prs[0].repo` as repo string - Fix test repo name from `recoupable/recoup-api` to `recoupable/api` - Resolve merge conflicts with main (email inbound files) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
lib/emails/inbound/respondToInboundEmail.ts (1)
17-83: This handler has outgrown the repo’s function-size guideline.The added logging pushes this function into validation, orchestration, transport, persistence, and observability all in one block. Extracting a couple of small helpers would make the control flow and failure modes much easier to maintain.
As per coding guidelines, "Apply Single Responsibility Principle (SRP): one exported function per file; each file should do one thing well" and "Keep functions under 50 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/emails/inbound/respondToInboundEmail.ts` around lines 17 - 83, The respondToInboundEmail function is too large and mixes responsibilities; extract small helpers to follow SRP and keep the exported function under 50 lines. Create one helper (e.g., validateAndPrepareEmail) that wraps the validation steps using validateNewEmailMemory and validateCcReplyExpected and returns early responses or { chatRequestBody, emailText, roomId }, and another helper (e.g., buildAndSendReply) that calls generateEmailResponse, constructs the payload using getFromWithName and the original fields, calls sendEmailWithResend, and persists via saveChatCompletion; update respondToInboundEmail to call these two helpers and only handle high-level try/catch and top-level logging.lib/coding-agent/handlers/onMergeAction.ts (1)
51-52: Consider differentiating "merged" vs "partially_merged" status.The handler sets
status: "merged"regardless of whether all PRs succeeded or some failed. This could mislead downstream consumers about the actual merge outcome.♻️ Suggested approach
+ const allSucceeded = results.every(r => r.includes("merged") && !r.includes("failed")); + const newStatus = allSucceeded ? "merged" : "failed"; - await thread.setState({ status: "merged" }); + await thread.setState({ status: newStatus }); await thread.post(`Merge results:\n${results.map(r => `- ${r}`).join("\n")}`);Note: This requires adding validation that "failed" is acceptable here or introducing a new status like "partially_merged" in
CodingAgentThreadState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 51 - 52, The handler always sets thread.setState({ status: "merged" }) even when some PR merges failed; change it to inspect results (the results array) and set status to "merged" only if all entries indicate success, otherwise set status to a new "partially_merged" (or "failed" if none succeeded) value; update the CodingAgentThreadState union/type to include "partially_merged" if needed and ensure thread.post still reports the detailed results array (results.map) while thread.setState uses the computed status.lib/coding-agent/handleCodingAgentCallback.ts (2)
72-77: Duplicate thread instantiation inpr_createdcase.A
ThreadImplis created at line 72, thenhandlePRCreatedcreates another instance internally (line 30). Pass the existing thread to avoid redundant object creation.♻️ Proposed refactor
-async function handlePRCreated(threadId: string, body: CodingAgentCallbackBody) { - const thread = getThread(threadId); +async function handlePRCreated(thread: ThreadImpl<CodingAgentThreadState>, body: CodingAgentCallbackBody) { const prLinks = (body.prs ?? []) .map(pr => `- [${pr.repo}#${pr.number}](${pr.url}) → \`${pr.baseBranch}\``) .join("\n"); // ... rest unchanged } // In handleCodingAgentCallback: switch (validated.status) { case "pr_created": - await handlePRCreated(validated.threadId, validated); + await handlePRCreated(thread, validated); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handleCodingAgentCallback.ts` around lines 72 - 77, The code currently calls getThread(...) to create a ThreadImpl instance (const thread = getThread(validated.threadId)) but handlePRCreated(...) re-instantiates a ThreadImpl internally; update handlePRCreated to accept the existing thread instance instead of creating a new one: change the handlePRCreated signature to something like handlePRCreated(thread, validated) and remove the internal ThreadImpl construction inside handlePRCreated so the existing thread (from getThread) is reused when invoking handlePRCreated(validated.threadId, validated) — locate getThread and handlePRCreated symbols to make the signature and call-site change consistently.
55-63: Consider using timing-safe comparison for secret validation.String comparison with
!==is vulnerable to timing attacks. While the risk is low for internal callbacks, using a constant-time comparison is a security best practice.🔐 Optional security hardening
+import { timingSafeEqual } from "crypto"; + +function safeCompare(a: string, b: string): boolean { + if (a.length !== b.length) return false; + return timingSafeEqual(Buffer.from(a), Buffer.from(b)); +} + export async function handleCodingAgentCallback(request: Request): Promise<NextResponse> { const secret = request.headers.get("x-callback-secret"); const expectedSecret = process.env.CODING_AGENT_CALLBACK_SECRET; - if (!secret || secret !== expectedSecret) { + if (!secret || !expectedSecret || !safeCompare(secret, expectedSecret)) { return NextResponse.json( { status: "error", error: "Unauthorized" }, { status: 401, headers: getCorsHeaders() }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handleCodingAgentCallback.ts` around lines 55 - 63, Replace the direct string comparison of secret and expectedSecret in handleCodingAgentCallback.ts with a timing-safe comparison: ensure expectedSecret is defined first, convert both secret and expectedSecret to Buffers (or Uint8Array) of equal length (padding or early reject if lengths differ), and compare them using a constant-time routine such as crypto.timingSafeEqual; update the authorization branch that currently references secret and expectedSecret to use this timing-safe check and still return the same 401 NextResponse when the check fails.lib/coding-agent/bot.ts (1)
20-31: Environment variables accessed with non-null assertions lack runtime validation.Using
!assertions onprocess.env.*will cause cryptic runtime errors if any variable is missing. Consider failing fast with descriptive errors during initialization.♻️ Proposed validation helper
+function requireEnv(name: string): string { + const value = process.env[name]; + if (!value) { + throw new Error(`Missing required environment variable: ${name}`); + } + return value; +} + export function createCodingAgentBot() { // ...state setup... const slack = new SlackAdapter({ - botToken: process.env.SLACK_BOT_TOKEN!, - signingSecret: process.env.SLACK_SIGNING_SECRET!, + botToken: requireEnv("SLACK_BOT_TOKEN"), + signingSecret: requireEnv("SLACK_SIGNING_SECRET"), logger, }); const github = new GitHubAdapter({ - token: process.env.GITHUB_TOKEN!, - webhookSecret: process.env.GITHUB_WEBHOOK_SECRET!, - userName: process.env.GITHUB_BOT_USERNAME!, + token: requireEnv("GITHUB_TOKEN"), + webhookSecret: requireEnv("GITHUB_WEBHOOK_SECRET"), + userName: requireEnv("GITHUB_BOT_USERNAME"), logger, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/bot.ts` around lines 20 - 31, The code uses non-null assertions on process.env values when constructing SlackAdapter and GitHubAdapter (SlackAdapter, GitHubAdapter in bot.ts), which can produce cryptic runtime errors; replace these `!` usages with a small fail-fast validation/getEnv helper that checks required env vars (e.g., SLACK_BOT_TOKEN, SLACK_SIGNING_SECRET, GITHUB_TOKEN, GITHUB_WEBHOOK_SECRET, GITHUB_BOT_USERNAME) and throws a descriptive Error if any are missing before creating SlackAdapter or GitHubAdapter so initialization fails with clear messages.lib/coding-agent/handlers/onSubscribedMessage.ts (1)
13-36: Consider handling "merged" and "failed" states explicitly.The handler silently exits for
mergedandfailedstates. While this may be intentional (ignoring messages on completed threads), an explicit check with a user-facing message would improve UX.💡 Optional enhancement
if (state.status === "running" || state.status === "updating") { await thread.post("I'm still working on this. I'll let you know when I'm done."); return; } + if (state.status === "merged" || state.status === "failed") { + await thread.post("This task has already completed. Start a new mention to begin again."); + return; + } + if (state.status === "pr_created" && state.snapshotId && state.branch && state.prs?.length) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onSubscribedMessage.ts` around lines 13 - 36, The handler inside bot.onSubscribedMessage currently ignores state.status values "merged" and "failed"; add explicit branches in that handler to detect state.status === "merged" and state.status === "failed" and call thread.post with a short user-facing message (e.g., "This PR has been merged — no further changes needed." and "This operation has failed — please open a new thread or retry.") so users receive clear feedback, and return immediately after posting; update any existing early-return logic around state/status checks and preserve behavior for "running","updating", and "pr_created" flows (references: bot.onSubscribedMessage, state.status, thread.post, triggerUpdatePR).lib/coding-agent/config.ts (1)
20-39: DRY: Extract common parsing logic.Both
getAllowedChannelIdsandgetAllowedUserIdsshare identical parsing logic. Consider extracting a helper.♻️ Suggested refactor
+function parseCommaSeparatedEnv(envVar: string | undefined): string[] { + if (!envVar) return []; + return envVar + .split(",") + .map(id => id.trim()) + .filter(Boolean); +} + export function getAllowedChannelIds(): string[] { - const raw = process.env.CODING_AGENT_CHANNELS; - if (!raw) return []; - return raw - .split(",") - .map(id => id.trim()) - .filter(Boolean); + return parseCommaSeparatedEnv(process.env.CODING_AGENT_CHANNELS); } export function getAllowedUserIds(): string[] { - const raw = process.env.CODING_AGENT_USERS; - if (!raw) return []; - return raw - .split(",") - .map(id => id.trim()) - .filter(Boolean); + return parseCommaSeparatedEnv(process.env.CODING_AGENT_USERS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/config.ts` around lines 20 - 39, Both getAllowedChannelIds and getAllowedUserIds duplicate the same CSV parsing logic; extract a small helper (e.g., parseCsvEnv or parseEnvList) that accepts an env var name (string) and returns string[] by reading process.env[name], returning [] for falsy, splitting on ",", trimming entries and filtering out empty strings, then replace the bodies of getAllowedChannelIds and getAllowedUserIds to call this helper with "CODING_AGENT_CHANNELS" and "CODING_AGENT_USERS" respectively.lib/coding-agent/handlers/onNewMention.ts (2)
42-47: Unnecessary type assertion.The
as Partial<CodingAgentThreadState>cast suggests a type mismatch. Looking at theCodingAgentThreadStateinterface (from context snippet 2), the object being passed hasstatus,prompt,runId, andslackThreadId- all valid fields.If
setStateexpectsPartial<CodingAgentThreadState>, the assertion shouldn't be needed. If it expects the full type, this cast hides a potential issue where required fields are missing.Verify the
setStatemethod signature and remove the cast if it's already typed to accept partial state:- } as Partial<CodingAgentThreadState>); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onNewMention.ts` around lines 42 - 47, The object passed to thread.setState is being needlessly cast to Partial<CodingAgentThreadState>; inspect the setState signature on the thread object (thread.setState) and the CodingAgentThreadState interface, and remove the unnecessary "as Partial<CodingAgentThreadState>" cast from the call in onNewMention.ts if setState already accepts a partial state; if setState actually requires the full type, update its signature to accept Partial<CodingAgentThreadState> or supply the missing required fields (status, prompt, runId, slackThreadId) so the types align without a cast.
14-30: Silent failures on allowlist rejection provide no audit trail.When a request is rejected due to channel or user not being in the allowlist, the handler returns silently with no logging. This makes debugging and monitoring difficult.
🔍 Add logging for rejected requests
if (allowedChannels.length > 0) { const channelId = thread.id.split(":")[1]; if (!allowedChannels.includes(channelId)) { + console.info(`[coding-agent] Rejected: channel ${channelId} not in allowlist`); return; } } if (allowedUsers.length > 0) { const userId = message.author.userId; if (!allowedUsers.includes(userId)) { + console.info(`[coding-agent] Rejected: user ${userId} not in allowlist`); return; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onNewMention.ts` around lines 14 - 30, The handler bot.onNewMention currently returns silently when a channel or user is not in the allowlist; add logging before each early return so each rejection is auditable: call the project's logger (e.g., processLogger or an existing logger) in the onNewMention block when getAllowedChannelIds() blocks a request (log the channelId, thread.id and a clear message like "rejected mention: channel not allowed") and likewise when getAllowedUserIds() blocks a request (log message with userId, message.author.userId and thread.id). Place the logs immediately before each return in the bot.onNewMention function so rejections are recorded with the identifying symbols channelId, userId, thread.id and message.author.userId.app/api/coding-agent/[platform]/route.ts (2)
27-27: Clarify thewaitUntilwrapper pattern.The expression
{ waitUntil: p => after(() => p) }adapts Next.js'safter()API to the Chat SDK'swaitUntilinterface. This ensures background work completes even after the response is sent.This is a clever adaptation, but a brief inline comment would help future maintainers understand why this wrapper exists.
- return handler(request, { waitUntil: p => after(() => p) }); + // Adapt Next.js after() to Chat SDK's waitUntil interface for background processing + return handler(request, { waitUntil: p => after(() => p) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/coding-agent/`[platform]/route.ts at line 27, Add a brief inline comment above the return using handler(request, { waitUntil: p => after(() => p) }) explaining that this wrapper adapts Next.js's after() API to the Chat SDK's waitUntil interface so background tasks scheduled via waitUntil are preserved after the response is sent; reference the handler, waitUntil, and after symbols so maintainers know the intent and that after() is being used to bridge the two lifecycles.
6-15: Minor JSDoc formatting issue.The
@paramdocumentation has a redundant/malformed entry:
@param params.params(line 13) appears to be a typo@param params(line 14) is the correct entry📝 Fix JSDoc
/** * POST /api/coding-agent/[platform] * * Webhook endpoint for the coding agent bot. * Handles both Slack and GitHub webhooks via dynamic [platform] segment. * * `@param` request - The incoming webhook request - * `@param` params.params * `@param` params - Route params containing the platform name (slack or github) */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/coding-agent/`[platform]/route.ts around lines 6 - 15, Remove the malformed redundant JSDoc tag `@param params.params` in the route handler's header comment and keep the correct `@param params` entry; update the comment block above the exported webhook handler (the POST /api/coding-agent/[platform] JSDoc) so it only documents `@param request` and `@param params` with correct descriptions and no duplicated param lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/coding-agent/config.ts`:
- Around line 20-27: Return a "no-config" sentinel instead of an empty array so
the system fails closed: change getAllowedChannelIds to return null (or
undefined) when CODING_AGENT_CHANNELS is not set, and update callers (e.g., the
allowlist check in onNewMention.ts that reads allowedChannels) to treat null as
"deny all" (reject requests) while only allowing channels when an explicit
non-empty array is provided; ensure the new behavior is documented in the
function comment and tests are updated to expect null -> deny.
In `@lib/coding-agent/handleCodingAgentCallback.ts`:
- Around line 65-70: The current call to request.json() in
handleCodingAgentCallback can throw on malformed JSON and must be wrapped in a
try/catch to return a clean 400 response; update the function so the await
request.json() call is inside a try block and on catch create and return a
NextResponse (400) with a descriptive error message, then proceed to call
validateCodingAgentCallback(body) as before (preserve the existing NextResponse
handling when validateCodingAgentCallback returns a NextResponse).
In `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 43-48: When handling non-OK fetch responses in onMergeAction.ts
(the block using response, results and pr), wrap the call to response.json() in
a try/catch (or attempt json then fall back to response.text()) so JSON parse
failures don't throw; extract a safe error message (e.g., parsed.message ||
rawText || `status ${response.status}`) and push `${pr.repo}#${pr.number}
failed: <safe message>` to results instead of calling response.json() directly.
- Around line 28-41: Wrap the fetch call that merges PRs inside onMergeAction
(the loop over state.prs and the PUT to
`https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge` which
sends JSON { merge_method: "squash" } using token) in a try-catch so a network
error on one PR doesn't abort the whole loop; catch and log the error (including
which pr.number and pr.repo) via the existing logger or processLogger, and
continue to the next PR so remaining items in state.prs are still processed.
In `@lib/coding-agent/handlers/onNewMention.ts`:
- Around line 37-48: Wrap the async work started in the onNewMention flow in a
try/catch around the call to triggerCodingAgent and the subsequent
thread.setState; if triggerCodingAgent throws, catch the error, set the thread
state to a failure status (e.g., status: "failed") and include an error message
instead of runId, and send a follow-up message/reply to the user indicating the
operation failed (and optionally include the error.message). Ensure you only
reference handle.id after a successful triggerCodingAgent return and include the
caught error when calling process logging or thread replies so the user and logs
show the failure.
- Around line 18-23: Extract the thread ID parsing into a shared utility named
extractChannelIdFromThreadId(threadId: string): string | null that validates the
expected "adapterName:channelId:threadTimestamp" format and returns the
channelId or null on invalid input; replace the inline split usage in
onNewMention's check (where thread.id.split(":")[1] is used) and in
handleCodingAgentCallback to call this utility, and update the allowedChannels
check to skip/return early when the utility returns null, also ensure the
utility logs or throws a clear error only where appropriate so invalid thread
IDs are handled consistently across callers.
In `@lib/emails/inbound/respondToInboundEmail.ts`:
- Line 30: In respondToInboundEmail replace raw PII logs (e.g., the console.log
that prints emailId and to) with masked identifiers: log emailId and a
sanitized/masked version of addresses (e.g., show first char and domain or
replace middle with ****) instead of full sender/recipient strings; likewise,
when catching provider errors (where error is currently logged raw), log a safe
summary such as error.message, error.name, and a short sanitized code/ID but
never the full provider response or body. Apply the same masking/sanitization
pattern to the other console/log statements in this module that print addresses
or raw errors (the other logging calls around the inbound email processing).
- Around line 67-74: The persistence currently happens before checking whether
sendEmailWithResend returned an error NextResponse, causing failed deliveries to
be recorded; update the flow in respondToInboundEmail so that you call
sendEmailWithResend and immediately check if result instanceof NextResponse (and
return on error), and only after that successful-path check invoke
saveChatCompletion({ text, roomId }); use the existing symbols result,
sendEmailWithResend, NextResponse, saveChatCompletion, text, roomId and keep the
console log and early return for the error case as-is.
---
Nitpick comments:
In `@app/api/coding-agent/`[platform]/route.ts:
- Line 27: Add a brief inline comment above the return using handler(request, {
waitUntil: p => after(() => p) }) explaining that this wrapper adapts Next.js's
after() API to the Chat SDK's waitUntil interface so background tasks scheduled
via waitUntil are preserved after the response is sent; reference the handler,
waitUntil, and after symbols so maintainers know the intent and that after() is
being used to bridge the two lifecycles.
- Around line 6-15: Remove the malformed redundant JSDoc tag `@param
params.params` in the route handler's header comment and keep the correct
`@param params` entry; update the comment block above the exported webhook
handler (the POST /api/coding-agent/[platform] JSDoc) so it only documents
`@param request` and `@param params` with correct descriptions and no duplicated
param lines.
In `@lib/coding-agent/bot.ts`:
- Around line 20-31: The code uses non-null assertions on process.env values
when constructing SlackAdapter and GitHubAdapter (SlackAdapter, GitHubAdapter in
bot.ts), which can produce cryptic runtime errors; replace these `!` usages with
a small fail-fast validation/getEnv helper that checks required env vars (e.g.,
SLACK_BOT_TOKEN, SLACK_SIGNING_SECRET, GITHUB_TOKEN, GITHUB_WEBHOOK_SECRET,
GITHUB_BOT_USERNAME) and throws a descriptive Error if any are missing before
creating SlackAdapter or GitHubAdapter so initialization fails with clear
messages.
In `@lib/coding-agent/config.ts`:
- Around line 20-39: Both getAllowedChannelIds and getAllowedUserIds duplicate
the same CSV parsing logic; extract a small helper (e.g., parseCsvEnv or
parseEnvList) that accepts an env var name (string) and returns string[] by
reading process.env[name], returning [] for falsy, splitting on ",", trimming
entries and filtering out empty strings, then replace the bodies of
getAllowedChannelIds and getAllowedUserIds to call this helper with
"CODING_AGENT_CHANNELS" and "CODING_AGENT_USERS" respectively.
In `@lib/coding-agent/handleCodingAgentCallback.ts`:
- Around line 72-77: The code currently calls getThread(...) to create a
ThreadImpl instance (const thread = getThread(validated.threadId)) but
handlePRCreated(...) re-instantiates a ThreadImpl internally; update
handlePRCreated to accept the existing thread instance instead of creating a new
one: change the handlePRCreated signature to something like
handlePRCreated(thread, validated) and remove the internal ThreadImpl
construction inside handlePRCreated so the existing thread (from getThread) is
reused when invoking handlePRCreated(validated.threadId, validated) — locate
getThread and handlePRCreated symbols to make the signature and call-site change
consistently.
- Around line 55-63: Replace the direct string comparison of secret and
expectedSecret in handleCodingAgentCallback.ts with a timing-safe comparison:
ensure expectedSecret is defined first, convert both secret and expectedSecret
to Buffers (or Uint8Array) of equal length (padding or early reject if lengths
differ), and compare them using a constant-time routine such as
crypto.timingSafeEqual; update the authorization branch that currently
references secret and expectedSecret to use this timing-safe check and still
return the same 401 NextResponse when the check fails.
In `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 51-52: The handler always sets thread.setState({ status: "merged"
}) even when some PR merges failed; change it to inspect results (the results
array) and set status to "merged" only if all entries indicate success,
otherwise set status to a new "partially_merged" (or "failed" if none succeeded)
value; update the CodingAgentThreadState union/type to include
"partially_merged" if needed and ensure thread.post still reports the detailed
results array (results.map) while thread.setState uses the computed status.
In `@lib/coding-agent/handlers/onNewMention.ts`:
- Around line 42-47: The object passed to thread.setState is being needlessly
cast to Partial<CodingAgentThreadState>; inspect the setState signature on the
thread object (thread.setState) and the CodingAgentThreadState interface, and
remove the unnecessary "as Partial<CodingAgentThreadState>" cast from the call
in onNewMention.ts if setState already accepts a partial state; if setState
actually requires the full type, update its signature to accept
Partial<CodingAgentThreadState> or supply the missing required fields (status,
prompt, runId, slackThreadId) so the types align without a cast.
- Around line 14-30: The handler bot.onNewMention currently returns silently
when a channel or user is not in the allowlist; add logging before each early
return so each rejection is auditable: call the project's logger (e.g.,
processLogger or an existing logger) in the onNewMention block when
getAllowedChannelIds() blocks a request (log the channelId, thread.id and a
clear message like "rejected mention: channel not allowed") and likewise when
getAllowedUserIds() blocks a request (log message with userId,
message.author.userId and thread.id). Place the logs immediately before each
return in the bot.onNewMention function so rejections are recorded with the
identifying symbols channelId, userId, thread.id and message.author.userId.
In `@lib/coding-agent/handlers/onSubscribedMessage.ts`:
- Around line 13-36: The handler inside bot.onSubscribedMessage currently
ignores state.status values "merged" and "failed"; add explicit branches in that
handler to detect state.status === "merged" and state.status === "failed" and
call thread.post with a short user-facing message (e.g., "This PR has been
merged — no further changes needed." and "This operation has failed — please
open a new thread or retry.") so users receive clear feedback, and return
immediately after posting; update any existing early-return logic around
state/status checks and preserve behavior for "running","updating", and
"pr_created" flows (references: bot.onSubscribedMessage, state.status,
thread.post, triggerUpdatePR).
In `@lib/emails/inbound/respondToInboundEmail.ts`:
- Around line 17-83: The respondToInboundEmail function is too large and mixes
responsibilities; extract small helpers to follow SRP and keep the exported
function under 50 lines. Create one helper (e.g., validateAndPrepareEmail) that
wraps the validation steps using validateNewEmailMemory and
validateCcReplyExpected and returns early responses or { chatRequestBody,
emailText, roomId }, and another helper (e.g., buildAndSendReply) that calls
generateEmailResponse, constructs the payload using getFromWithName and the
original fields, calls sendEmailWithResend, and persists via saveChatCompletion;
update respondToInboundEmail to call these two helpers and only handle
high-level try/catch and top-level logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 845a3e5c-1322-4af3-8276-4ad1815da824
⛔ Files ignored due to path filters (7)
lib/coding-agent/__tests__/bot.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/coding-agent/__tests__/config.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/coding-agent/__tests__/handleCodingAgentCallback.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/coding-agent/__tests__/handlers.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/coding-agent/__tests__/validateCodingAgentCallback.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**package.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (14)
app/api/coding-agent/[platform]/route.tsapp/api/coding-agent/callback/route.tslib/coding-agent/bot.tslib/coding-agent/config.tslib/coding-agent/handleCodingAgentCallback.tslib/coding-agent/handlers/onMergeAction.tslib/coding-agent/handlers/onNewMention.tslib/coding-agent/handlers/onSubscribedMessage.tslib/coding-agent/handlers/registerHandlers.tslib/coding-agent/types.tslib/coding-agent/validateCodingAgentCallback.tslib/emails/inbound/respondToInboundEmail.tslib/trigger/triggerCodingAgent.tslib/trigger/triggerUpdatePR.ts
lib/coding-agent/config.ts
Outdated
Comment on lines
+20
to
+27
| export function getAllowedChannelIds(): string[] { | ||
| const raw = process.env.CODING_AGENT_CHANNELS; | ||
| if (!raw) return []; | ||
| return raw | ||
| .split(",") | ||
| .map(id => id.trim()) | ||
| .filter(Boolean); | ||
| } |
There was a problem hiding this comment.
Consider fail-closed behavior for allowlists.
When CODING_AGENT_CHANNELS is unset, returning an empty array causes the allowlist check in onNewMention.ts to be skipped entirely (line 18: if (allowedChannels.length > 0)). This means the bot accepts requests from all channels by default.
For a coding agent with significant capabilities, a fail-closed approach may be safer - require explicit configuration and reject requests if allowlists aren't configured.
🛡️ Optional: Fail-closed alternative
export function getAllowedChannelIds(): string[] {
const raw = process.env.CODING_AGENT_CHANNELS;
- if (!raw) return [];
+ if (!raw) {
+ console.warn("CODING_AGENT_CHANNELS not configured - no channels allowed");
+ return [];
+ }
return raw
.split(",")
.map(id => id.trim())
.filter(Boolean);
}Then in the handler, treat empty arrays as "deny all" rather than "allow all".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/config.ts` around lines 20 - 27, Return a "no-config"
sentinel instead of an empty array so the system fails closed: change
getAllowedChannelIds to return null (or undefined) when CODING_AGENT_CHANNELS is
not set, and update callers (e.g., the allowlist check in onNewMention.ts that
reads allowedChannels) to treat null as "deny all" (reject requests) while only
allowing channels when an explicit non-empty array is provided; ensure the new
behavior is documented in the function comment and tests are updated to expect
null -> deny.
Comment on lines
+28
to
+41
| for (const pr of state.prs) { | ||
| const [owner, repo] = pr.repo.split("/"); | ||
| const response = await fetch( | ||
| `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge`, | ||
| { | ||
| method: "PUT", | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| body: JSON.stringify({ merge_method: "squash" }), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Add network error handling for the fetch call.
The fetch call could throw on network failures. Without a try-catch, one failed request would abort the entire merge loop, leaving remaining PRs unprocessed.
🛡️ Proposed fix wrapping fetch in try-catch
for (const pr of state.prs) {
const [owner, repo] = pr.repo.split("/");
- const response = await fetch(
- `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge`,
- {
- method: "PUT",
- headers: {
- Authorization: `Bearer ${token}`,
- Accept: "application/vnd.github+json",
- "X-GitHub-Api-Version": "2022-11-28",
- },
- body: JSON.stringify({ merge_method: "squash" }),
- },
- );
-
- if (response.ok) {
- results.push(`${pr.repo}#${pr.number} merged`);
- } else {
- const error = await response.json();
- results.push(`${pr.repo}#${pr.number} failed: ${error.message}`);
+ try {
+ const response = await fetch(
+ `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge`,
+ {
+ method: "PUT",
+ headers: {
+ Authorization: `Bearer ${token}`,
+ Accept: "application/vnd.github+json",
+ "X-GitHub-Api-Version": "2022-11-28",
+ },
+ body: JSON.stringify({ merge_method: "squash" }),
+ },
+ );
+
+ if (response.ok) {
+ results.push(`${pr.repo}#${pr.number} merged`);
+ } else {
+ let errorMessage = `HTTP ${response.status}`;
+ try {
+ const error = await response.json();
+ errorMessage = error.message || errorMessage;
+ } catch {
+ // Response body wasn't valid JSON
+ }
+ results.push(`${pr.repo}#${pr.number} failed: ${errorMessage}`);
+ }
+ } catch (err) {
+ results.push(`${pr.repo}#${pr.number} failed: ${err instanceof Error ? err.message : "Network error"}`);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 28 - 41, Wrap the
fetch call that merges PRs inside onMergeAction (the loop over state.prs and the
PUT to `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge`
which sends JSON { merge_method: "squash" } using token) in a try-catch so a
network error on one PR doesn't abort the whole loop; catch and log the error
(including which pr.number and pr.repo) via the existing logger or
processLogger, and continue to the next PR so remaining items in state.prs are
still processed.
Comment on lines
+43
to
+48
| if (response.ok) { | ||
| results.push(`${pr.repo}#${pr.number} merged`); | ||
| } else { | ||
| const error = await response.json(); | ||
| results.push(`${pr.repo}#${pr.number} failed: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
Handle potential JSON parsing failure on error responses.
When response.ok is false, calling response.json() could throw if the response body isn't valid JSON (e.g., 502 gateway error). This would cause an unhandled exception.
🛡️ Proposed defensive fix
if (response.ok) {
results.push(`${pr.repo}#${pr.number} merged`);
} else {
- const error = await response.json();
- results.push(`${pr.repo}#${pr.number} failed: ${error.message}`);
+ let errorMessage = `HTTP ${response.status}`;
+ try {
+ const error = await response.json();
+ errorMessage = error.message || errorMessage;
+ } catch {
+ // Response body wasn't valid JSON
+ }
+ results.push(`${pr.repo}#${pr.number} failed: ${errorMessage}`);
}📝 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.
Suggested change
| if (response.ok) { | |
| results.push(`${pr.repo}#${pr.number} merged`); | |
| } else { | |
| const error = await response.json(); | |
| results.push(`${pr.repo}#${pr.number} failed: ${error.message}`); | |
| } | |
| if (response.ok) { | |
| results.push(`${pr.repo}#${pr.number} merged`); | |
| } else { | |
| let errorMessage = `HTTP ${response.status}`; | |
| try { | |
| const error = await response.json(); | |
| errorMessage = error.message || errorMessage; | |
| } catch { | |
| // Response body wasn't valid JSON | |
| } | |
| results.push(`${pr.repo}#${pr.number} failed: ${errorMessage}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 43 - 48, When
handling non-OK fetch responses in onMergeAction.ts (the block using response,
results and pr), wrap the call to response.json() in a try/catch (or attempt
json then fall back to response.text()) so JSON parse failures don't throw;
extract a safe error message (e.g., parsed.message || rawText || `status
${response.status}`) and push `${pr.repo}#${pr.number} failed: <safe message>`
to results instead of calling response.json() directly.
Comment on lines
+18
to
+23
| if (allowedChannels.length > 0) { | ||
| const channelId = thread.id.split(":")[1]; | ||
| if (!allowedChannels.includes(channelId)) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the Chat SDK documents thread ID format or provides a helper
rg -n "thread.id" --type=ts -C3 | head -50Repository: recoupable/api
Length of output: 2019
🏁 Script executed:
#!/bin/bash
# Read the full onNewMention.ts file to understand thread type and imports
cat -n lib/coding-agent/handlers/onNewMention.tsRepository: recoupable/api
Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Search for Thread type definition and Chat SDK imports
rg -n "Thread|thread:" --type=ts lib/coding-agent/ | head -30Repository: recoupable/api
Length of output: 2959
🏁 Script executed:
#!/bin/bash
# Look for any Chat SDK documentation, constants, or helper functions related to ID format
rg -n "split.*:" --type=ts lib/coding-agent/ -A2 -B2Repository: recoupable/api
Length of output: 1435
🏁 Script executed:
#!/bin/bash
# Search for chat module imports and see what's exported
rg -n "from.*['\"]chat['\"]" --type=ts -A5Repository: recoupable/api
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Check if there's a Chat SDK types file or helper utilities
fd -e ts -e d.ts | xargs grep -l "chat\|Chat" | head -10Repository: recoupable/api
Length of output: 431
🏁 Script executed:
#!/bin/bash
# Look at the test file to understand how channelId is expected to be extracted
cat -n lib/coding-agent/__tests__/handlers.test.ts | head -120Repository: recoupable/api
Length of output: 4435
Extract thread ID parsing into a reusable utility with error handling.
While the thread ID format (adapterName:channelId:threadTimestamp) is established throughout the codebase, the parsing logic lacks error handling and is repeated. If thread.id has an unexpected format, split(":")[1] returns undefined, which could silently pass invalid values to allowedChannels.includes().
Extract this into a shared utility function like extractChannelIdFromThreadId(threadId: string): string | null to consolidate the logic (used in both onNewMention.ts and handleCodingAgentCallback.ts) and add validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onNewMention.ts` around lines 18 - 23, Extract the
thread ID parsing into a shared utility named
extractChannelIdFromThreadId(threadId: string): string | null that validates the
expected "adapterName:channelId:threadTimestamp" format and returns the
channelId or null on invalid input; replace the inline split usage in
onNewMention's check (where thread.id.split(":")[1] is used) and in
handleCodingAgentCallback to call this utility, and update the allowedChannels
check to skip/return early when the utility returns null, also ensure the
utility logs or throws a clear error only where appropriate so invalid thread
IDs are handled consistently across callers.
Comment on lines
+37
to
+48
| const handle = await triggerCodingAgent({ | ||
| prompt, | ||
| callbackThreadId: thread.id, | ||
| }); | ||
|
|
||
| await thread.setState({ | ||
| status: "running", | ||
| prompt, | ||
| runId: handle.id, | ||
| slackThreadId: thread.id, | ||
| } as Partial<CodingAgentThreadState>); | ||
| }); |
There was a problem hiding this comment.
Missing error handling for async operations.
If triggerCodingAgent fails (network error, Trigger.dev outage), the user receives the "Starting work on..." message but never gets a follow-up. Consider wrapping in try/catch to notify the user of failures.
🛡️ Suggested error handling
await thread.subscribe();
await thread.post(`Starting work on: "${prompt}"\n\nI'll reply here when done.`);
- const handle = await triggerCodingAgent({
- prompt,
- callbackThreadId: thread.id,
- });
-
- await thread.setState({
- status: "running",
- prompt,
- runId: handle.id,
- slackThreadId: thread.id,
- } as Partial<CodingAgentThreadState>);
+ try {
+ const handle = await triggerCodingAgent({
+ prompt,
+ callbackThreadId: thread.id,
+ });
+
+ await thread.setState({
+ status: "running",
+ prompt,
+ runId: handle.id,
+ slackThreadId: thread.id,
+ });
+ } catch (error) {
+ console.error("[coding-agent] Failed to trigger task:", error);
+ await thread.post("❌ Failed to start the coding agent. Please try again.");
+ await thread.setState({ status: "failed", prompt });
+ }
});📝 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.
Suggested change
| const handle = await triggerCodingAgent({ | |
| prompt, | |
| callbackThreadId: thread.id, | |
| }); | |
| await thread.setState({ | |
| status: "running", | |
| prompt, | |
| runId: handle.id, | |
| slackThreadId: thread.id, | |
| } as Partial<CodingAgentThreadState>); | |
| }); | |
| try { | |
| const handle = await triggerCodingAgent({ | |
| prompt, | |
| callbackThreadId: thread.id, | |
| }); | |
| await thread.setState({ | |
| status: "running", | |
| prompt, | |
| runId: handle.id, | |
| slackThreadId: thread.id, | |
| }); | |
| } catch (error) { | |
| console.error("[coding-agent] Failed to trigger task:", error); | |
| await thread.post("❌ Failed to start the coding agent. Please try again."); | |
| await thread.setState({ status: "failed", prompt }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onNewMention.ts` around lines 37 - 48, Wrap the
async work started in the onNewMention flow in a try/catch around the call to
triggerCodingAgent and the subsequent thread.setState; if triggerCodingAgent
throws, catch the error, set the thread state to a failure status (e.g., status:
"failed") and include an error message instead of runId, and send a follow-up
message/reply to the user indicating the operation failed (and optionally
include the error.message). Ensure you only reference handle.id after a
successful triggerCodingAgent return and include the caught error when calling
process logging or thread replies so the user and logs show the failure.
| const from = getFromWithName(original.to, original.cc); | ||
| const cc = original.cc?.length ? original.cc : undefined; | ||
|
|
||
| console.log(`[respondToInboundEmail] Processing email ${emailId} from ${to}`); |
There was a problem hiding this comment.
Redact addresses and avoid logging raw provider errors.
These new debug logs persist sender/recipient email addresses, and the raw caught error can include provider response details. Please log only non-PII identifiers here, or mask the addresses before emitting them.
Also applies to: 66-66, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/emails/inbound/respondToInboundEmail.ts` at line 30, In
respondToInboundEmail replace raw PII logs (e.g., the console.log that prints
emailId and to) with masked identifiers: log emailId and a sanitized/masked
version of addresses (e.g., show first char and domain or replace middle with
****) instead of full sender/recipient strings; likewise, when catching provider
errors (where error is currently logged raw), log a safe summary such as
error.message, error.name, and a short sanitized code/ID but never the full
provider response or body. Apply the same masking/sanitization pattern to the
other console/log statements in this module that print addresses or raw errors
(the other logging calls around the inbound email processing).
Comment on lines
67
to
74
| const result = await sendEmailWithResend(payload); | ||
|
|
||
| // Save the assistant response message | ||
| await saveChatCompletion({ text, roomId }); | ||
|
|
||
| if (result instanceof NextResponse) { | ||
| console.log(`[respondToInboundEmail] Email ${emailId} - sendEmailWithResend returned error response`); | ||
| return result; |
There was a problem hiding this comment.
Persist the assistant reply only after delivery succeeds.
saveChatCompletion runs before you confirm that sendEmailWithResend did not return an error NextResponse, so failed deliveries are still recorded as sent. Move the persistence after the instanceof NextResponse guard.
🛠️ Suggested fix
console.log(`[respondToInboundEmail] Email ${emailId} - sending reply to ${to} from ${from}`);
const result = await sendEmailWithResend(payload);
-
- // Save the assistant response message
- await saveChatCompletion({ text, roomId });
if (result instanceof NextResponse) {
console.log(`[respondToInboundEmail] Email ${emailId} - sendEmailWithResend returned error response`);
return result;
}
+
+ // Save the assistant response message only after the reply is confirmed sent
+ await saveChatCompletion({ text, roomId });📝 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.
Suggested change
| const result = await sendEmailWithResend(payload); | |
| // Save the assistant response message | |
| await saveChatCompletion({ text, roomId }); | |
| if (result instanceof NextResponse) { | |
| console.log(`[respondToInboundEmail] Email ${emailId} - sendEmailWithResend returned error response`); | |
| return result; | |
| const result = await sendEmailWithResend(payload); | |
| if (result instanceof NextResponse) { | |
| console.log(`[respondToInboundEmail] Email ${emailId} - sendEmailWithResend returned error response`); | |
| return result; | |
| } | |
| // Save the assistant response message only after the reply is confirmed sent | |
| await saveChatCompletion({ text, roomId }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/emails/inbound/respondToInboundEmail.ts` around lines 67 - 74, The
persistence currently happens before checking whether sendEmailWithResend
returned an error NextResponse, causing failed deliveries to be recorded; update
the flow in respondToInboundEmail so that you call sendEmailWithResend and
immediately check if result instanceof NextResponse (and return on error), and
only after that successful-path check invoke saveChatCompletion({ text, roomId
}); use the existing symbols result, sendEmailWithResend, NextResponse,
saveChatCompletion, text, roomId and keep the console log and early return for
the error case as-is.
ioredis is configured with lazyConnect: true, so the state adapter's connect() hangs forever waiting for a "ready" event that never fires. Explicitly call redis.connect() when status is "wait" to unblock Chat SDK initialization and Slack webhook handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/coding-agent/bot.ts (1)
8-8: Avoid bootstrapping runtime infrastructure at import time.Line 8 and Lines 52 initialize logger/singleton state as soon as this module is imported. That adds side effects to a constructor module and makes tests pay for Redis/env setup even when they only need the factory.
As per coding guidelines,
lib/**/*.ts: "Avoid side effects" and "Apply Single Responsibility Principle (SRP): one exported function per file; each file should do one thing well".Also applies to: 48-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/bot.ts` at line 8, The file currently creates runtime state at import time by instantiating ConsoleLogger (const logger = new ConsoleLogger()) and other singletons; change this to lazy initialization by removing top-level instantiations and exposing a factory/getter that constructs ConsoleLogger (or the other singleton) on first use (e.g., getLogger() or createBotFactory()), or move initialization into the exported function/class constructor that uses it (e.g., inside the exported bot factory or Bot class), so importing the module has no side effects; update all internal references to use the getter/factory instead of the top-level logger variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/coding-agent/bot.ts`:
- Around line 16-18: The current startup code swallows Redis bootstrap failures
by logging and continuing; change it to fail fast: when redis.status === "wait"
call redis.connect() in a way that awaits the result (no silent .catch) and
propagate the error (either rethrow or call process.exit(1)) so the bot
factory/init fails during startup instead of returning a bot with an unusable
client; update the block around redis.status and redis.connect() in
lib/coding-agent/bot.ts to log the error with context and then throw the error
(or exit) so callers know the bootstrap failed.
- Around line 26-37: The SlackAdapter and GitHubAdapter are being passed
environment vars with non-null assertions (process.env.SLACK_BOT_TOKEN!, etc.)
which can be undefined at runtime; add explicit validation that throws a clear
error naming the missing key before constructing SlackAdapter/GitHubAdapter.
Implement a small helper (e.g., getEnvOrThrow) or inline checks that verify
botToken, signingSecret, token, webhookSecret, and userName are defined and
throw Error("Missing env: SLACK_BOT_TOKEN") (or the appropriate key) so
SlackAdapter and GitHubAdapter are only constructed with definite non-undefined
values.
---
Nitpick comments:
In `@lib/coding-agent/bot.ts`:
- Line 8: The file currently creates runtime state at import time by
instantiating ConsoleLogger (const logger = new ConsoleLogger()) and other
singletons; change this to lazy initialization by removing top-level
instantiations and exposing a factory/getter that constructs ConsoleLogger (or
the other singleton) on first use (e.g., getLogger() or createBotFactory()), or
move initialization into the exported function/class constructor that uses it
(e.g., inside the exported bot factory or Bot class), so importing the module
has no side effects; update all internal references to use the getter/factory
instead of the top-level logger variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcec2fb5-a2de-48c2-bdd9-3f4557ace624
📒 Files selected for processing (1)
lib/coding-agent/bot.ts
The Slack challenge request was timing out because bot initialization blocks on Redis connection. Now the route handles url_verification challenges immediately before loading the bot. Also adds debug logging and lazy imports to isolate initialization errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Verify Slack url_verification challenge responds immediately - Verify 404 for unknown platforms - Verify non-challenge requests delegate to bot webhook handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trace handler execution to diagnose why bot isn't replying: - Log when handler fires with thread/author info - Log channel/user allowlist rejections - Log each step: subscribe, post, trigger task - Catch and log errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Throws a clear error listing all missing env vars when the bot initializes. Required vars: - SLACK_BOT_TOKEN - SLACK_SIGNING_SECRET - GITHUB_TOKEN - GITHUB_WEBHOOK_SECRET - GITHUB_BOT_USERNAME - REDIS_URL - CODING_AGENT_CALLBACK_SECRET Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split GitHub adapter (GitHubAdapter, onSubscribedMessage, onMergeAction, triggerUpdatePR, SUBMODULE_CONFIG) into separate PR (MYC-4431). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All channels and users are now allowed to interact with the coding agent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…SRP) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep pr_created flow (posts PR links to Slack thread). Remove updated status (GitHub-specific, belongs in MYC-4431). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address CodeRabbit feedback: - handleCodingAgentCallback: catch malformed JSON, return 400 - bot.ts: throw on Redis connect failure instead of swallowing error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a user replies in a thread with pr_created status, triggers the update-pr task with their feedback. Tells user to wait if agent is still running. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 7, 2026
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
chatnpm package)/api/coding-agent/[platform]for Slack and GitHub events/api/coding-agent/callbackfor Trigger.dev task resultsonNewMention(start agent),onSubscribedMessage(feedback loop),onMergeAction(merge PRs)triggerCodingAgentandtriggerUpdatePRNew Environment Variables
SLACK_BOT_TOKENSLACK_SIGNING_SECRETGITHUB_WEBHOOK_SECRETGITHUB_BOT_USERNAMECODING_AGENT_CALLBACK_SECRETCODING_AGENT_CHANNELSCODING_AGENT_USERSTest plan
pnpm test)/api/coding-agent/slack🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements