#29 post to Slack when merge bot fails#30
Conversation
|
Deleted global.mdc since it's auto-generated from cursor-commands (if you add a line like |
WalkthroughThis PR refactors the main merge bot execution flow to introduce centralized error handling with try/catch, adds helper functions for orchestrating the automerge and branch maintenance phases, and enhances test infrastructure to capture outputs and simulate errors. Changes
Sequence DiagramsequenceDiagram
participant GA as GitHub Actions
participant MB as Merge Bot
participant AM as AutoMerger
participant BM as BranchMaintainer
participant GH as GitHub API
participant Slack as Slack API
GA->>MB: Execute merge-bot.js
rect rgba(100, 150, 200, 0.5)
MB->>AM: automerge({ config, shell, gh, git })
AM->>GH: Query branches & PRs
GH-->>AM: Branch data
AM->>GH: Create/update PRs
GH-->>AM: Result
AM-->>MB: Return AutoMerger instance
end
rect rgba(150, 150, 100, 0.5)
MB->>BM: maintainBranches({ config, shell, automerger })
BM->>GH: Manage branches with conflict data
GH-->>BM: Confirmation
BM-->>MB: Complete
end
rect rgba(100, 200, 150, 0.5)
MB->>GH: setFinalStatus(automerger)
GH-->>MB: Status set
MB->>GA: Set outputs
end
rect rgba(200, 100, 100, 0.5)
alt Error occurs
MB->>Slack: Report error with actionUrl
Slack-->>MB: Message sent
MB->>GA: Set error outputs & failed status
end
end
MB-->>GA: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/merge-bot.js (1)
36-37: Nit:let→constforstatusMessage.
statusMessageis never reassigned.Proposed fix
- let statusMessage = + const statusMessage = `Merge bot error: ${error.message} <${actionUrl}|Action Run>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/merge-bot.js` around lines 36 - 37, The declaration for statusMessage in merge-bot.js is using let but the variable is never reassigned; change the declaration of statusMessage to const to reflect immutability (update the line that currently reads let statusMessage = `Merge bot error: ${error.message} <${actionUrl}|Action Run>` to use const instead).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/merge-bot.js`:
- Around line 15-42: The destructuring of github.context and the actionUrl
computation must be moved inside the existing try block in function run() so any
exceptions there are caught by the catch that sets Slack outputs; relocate the
lines that read github.context and build actionUrl into the try before other
initialization (Shell, GitHubClient, Git, etc.). Also add a top-level safety net
by changing the invocation of run() to run().catch(err => {
core.setFailed(err.message); core.setOutput('status','error');
core.setOutput('status-message', `Merge bot error: ${err.message}`); }); so any
rejection that escapes run() still sets failure outputs (use the same
core.setFailed and core.setOutput calls used in the existing catch). Ensure
references to actionUrl inside the catch use the local variable moved into the
try or fall back safely to a plain message if not available.
---
Nitpick comments:
In `@src/merge-bot.js`:
- Around line 36-37: The declaration for statusMessage in merge-bot.js is using
let but the variable is never reassigned; change the declaration of
statusMessage to const to reflect immutability (update the line that currently
reads let statusMessage = `Merge bot error: ${error.message}
<${actionUrl}|Action Run>` to use const instead).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
action.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
.cursor/rules/global.mdcsrc/merge-bot.jstest/merge-bot-test.jstest/test-helpers.js
💤 Files with no reviewable changes (1)
- .cursor/rules/global.mdc
| async function run() { | ||
| const config = readConfig() | ||
| const shell = new Shell(core) | ||
| const gh = new GitHubClient({ core, github }) | ||
| const git = new Git(shell) | ||
| const { serverUrl, runId, repo } = github.context | ||
| const actionUrl = | ||
| `${serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${runId}` | ||
|
|
||
| await git.configureIdentity( | ||
| 'Spider Merge Bot', 'merge-bot@spiderstrategies.com') | ||
| try { | ||
| const config = readConfig() | ||
| const shell = new Shell(core) | ||
| const gh = new GitHubClient({ core, github }) | ||
| const git = new Git(shell) | ||
|
|
||
| // Phase 1: Merge forward | ||
| await git.configureIdentity('Spider Merge Bot', | ||
| 'merge-bot@spiderstrategies.com') | ||
|
|
||
| const automerger = await automerge({ config, shell, gh, git }) | ||
| await maintainBranches({ config, shell, automerger }) | ||
| setFinalStatus(automerger) | ||
| } catch (error) { | ||
| // Issue #29 - Ensure Slack gets notified on crashes. | ||
| // Without this, setFinalStatus is never called and | ||
| // the Slack step gets empty outputs. | ||
| let statusMessage = | ||
| `Merge bot error: ${error.message} <${actionUrl}|Action Run>` | ||
| core.setFailed(error.message) | ||
| core.setOutput('status', 'error') | ||
| core.setOutput('status-message', statusMessage) | ||
| } | ||
| } |
There was a problem hiding this comment.
actionUrl resolution sits outside the try block — an early throw skips Slack notification.
Lines 16-18 destructure github.context before entering the try. If that destructuring ever fails (e.g., unexpected context shape in a non-standard runner), the error propagates out of run() uncaught, and no outputs are set for Slack — exactly the scenario this PR aims to prevent.
Additionally, run() at line 128 is invoked without a trailing .catch(), so any rejection that escapes the try becomes an unhandled promise rejection.
Consider moving the actionUrl computation inside the try, and adding a top-level .catch() as a safety net:
Proposed fix
async function run() {
- const { serverUrl, runId, repo } = github.context
- const actionUrl =
- `${serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${runId}`
-
+ let actionUrl = ''
try {
+ const { serverUrl, runId, repo } = github.context
+ actionUrl =
+ `${serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${runId}`
+
const config = readConfig()
...
} catch (error) {
- let statusMessage =
+ const statusMessage =
`Merge bot error: ${error.message} <${actionUrl}|Action Run>`
core.setFailed(error.message)
core.setOutput('status', 'error')
core.setOutput('status-message', statusMessage)
}
}
-run()
+run().catch(err => {
+ core.setFailed(`Unhandled: ${err.message}`)
+})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/merge-bot.js` around lines 15 - 42, The destructuring of github.context
and the actionUrl computation must be moved inside the existing try block in
function run() so any exceptions there are caught by the catch that sets Slack
outputs; relocate the lines that read github.context and build actionUrl into
the try before other initialization (Shell, GitHubClient, Git, etc.). Also add a
top-level safety net by changing the invocation of run() to run().catch(err => {
core.setFailed(err.message); core.setOutput('status','error');
core.setOutput('status-message', `Merge bot error: ${err.message}`); }); so any
rejection that escapes run() still sets failure outputs (use the same
core.setFailed and core.setOutput calls used in the existing catch). Ensure
references to actionUrl inside the catch use the local variable moved into the
try or fall back safely to a plain message if not available.
There was a problem hiding this comment.
Good catch on the let → const — fixed that.
For the other two suggestions:
Moving actionUrl inside the try: github.context is already destructured at module scope on line 10 (const { pull_request, repository } = github.context.payload). If github.context were broken, the module would crash at line 10 before run() is ever called. The proposed let actionUrl = '' fallback would produce a broken Slack link (<|Action Run>) on that impossible path — not genuinely useful.
Adding run().catch(): The only way run() rejects past the try/catch is if the catch block itself throws. The catch block calls core.setFailed() and core.setOutput() — simple @actions/core functions that don't throw. A safety net for the safety net adds complexity for a scenario that isn't realistic.
There was a problem hiding this comment.
@mattsgarlata, you're absolutely right on both points. The module-scope destructuring at line 10 means github.context would fail to load before run() is ever invoked, and the catch block's operations are indeed simple setters that won't throw. Your reasoning is sound — no need to add defensive code for scenarios that can't realistically occur. Thanks for the clarification!
🧠 Learnings used
Learnt from: mattsgarlata
Repo: SpiderStrategies/merge-bot PR: 28
File: src/merge-bot.js:6-7
Timestamp: 2026-02-23T23:20:57.098Z
Learning: In src/merge-bot.js, ensure that extractTargetFromMergeForward is only invoked after the startsWith(MB_BRANCH_FORWARD_PREFIX) guard and operates on merge-forward-pr-{prNumber}-{targetBranch} formatted names. Since the system controls this data, defensive checks for malformed branch names are unnecessary (YAGNI). Do not add such checks; rely on the guard and the defined format. If adding tests, verify the guard condition and the format expectation.
|
I think the let -> const thing wasn't changed, but I also don't care (that's a trivial change without any functional difference). |
2 participants
No description provided.