feat(bot): add bot_ci merge blocking check#2016
Conversation
- Add Mergeable: bot_ci check that monitors bot_ci status - Block merge when bot_ci is triggered but not passed (in_progress or failed) - Allow merge when bot_ci is not triggered - Add comprehensive tests for all scenarios Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughRegisters a new mergeable check handler in the Probot app and adds a module that creates/updates a public "Mergeable" check based on internal bot_ci check status; includes new webhook fixture payloads and a comprehensive test suite exercising these flows. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHubWebhook as GitHub Webhook
participant ProbotApp as Probot App
participant GitHubAPI as GitHub REST API
participant MergeableLogic as Mergeable Module
GitHubWebhook->>ProbotApp: Deliver check_run or pull_request event
activate ProbotApp
alt check_run event (bot_ci)
ProbotApp->>GitHubAPI: GET bot_ci check_runs for PR/head SHA
activate GitHubAPI
GitHubAPI-->>ProbotApp: bot_ci check_run(s)
deactivate GitHubAPI
ProbotApp->>MergeableLogic: Determine desired MERGEABLE_CHECK_NAME state
activate MergeableLogic
MergeableLogic-->>ProbotApp: desired status/conclusion/output
deactivate MergeableLogic
alt existing Mergeable check
ProbotApp->>GitHubAPI: PATCH update MERGEABLE check_run
else new Mergeable check needed
ProbotApp->>GitHubAPI: POST create MERGEABLE check_run
end
activate GitHubAPI
GitHubAPI-->>ProbotApp: confirmation
deactivate GitHubAPI
else pull_request event
ProbotApp->>GitHubAPI: GET checks for PR head SHA
activate GitHubAPI
GitHubAPI-->>ProbotApp: existing checks list
deactivate GitHubAPI
ProbotApp->>MergeableLogic: evaluate and create/update MERGEABLE as needed
end
deactivate ProbotApp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/github/src/index.ts (3)
111-135: Clarify behavior when multiplebot_cicheck runs exist for the same ref
getBotCiCheckRunalways returnsdata.check_runs[0]. If the CI can be re-run on the same commit, there may be multiplebot_cicheck runs for a ref, and this will implicitly pick whichever Octokit returns first.If the invariant is “at most one bot_ci check per ref”, it would be useful to document that assumption; otherwise, consider explicitly selecting the most recent (e.g., by
completed_at/started_at) for future-proofing.
235-264: Defensive handling ofcheck_run.pull_requestsThe handler early-returns when
pull_requests.length === 0, which is good to avoid unnecessary work. To harden against any webhook payloads wherepull_requestsmight be missing or null, you could normalize before accessing.length:- const pullRequests = checkRun.pull_requests; - if (pullRequests.length === 0) { + const pullRequests = checkRun.pull_requests ?? []; + if (pullRequests.length === 0) { context.log.info("No pull requests associated with this check run"); return; }Not strictly required given current fixtures, but it makes the handler more robust to odd payloads.
266-288: PR event handler wiring keeps Mergeable check in syncHooking
pull_request.opened/synchronize/reopenedintohandleBotCiCheckensures the Mergeable check is reconciled whenever the head SHA changes or a PR is (re)opened, even if no check_run event has fired yet. Combined with the check_run.* handler, this should keep the Mergeable check up to date across the full lifecycle.One design nuance to be aware of: immediately on PR open with no existing
bot_cirun, the Mergeable check is marked successful until abot_cicheck_run appears and flips it to in_progress. If you ever need zero “green window” before bot_ci starts, you may want to treat “no bot_ci yet” as in_progress instead.apps/github/test/index.test.ts (1)
232-470: bot_ci test suite provides strong coverage with clear expectationsThe new
"bot_ci check handler"suite exercises all key behaviors:
- created → in_progress Mergeable check
- completed success/failure → corresponding Mergeable status/conclusion/title
- PR opened with no bot_ci → “not triggered” success case
- Updating an existing Mergeable check vs creating a new one
- Ignoring non-bot_ci check_run events
The nock setups mirror the app’s GitHub API calls closely, and
probot.receiveuses the correct event names ("check_run"/"pull_request"withactionin payload). Overall, this is a solid, readable set of tests.You might later factor out the repeated access-token and
listForRefnock setup into small helpers to DRY up the suite, but it’s not necessary for this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/github/src/index.ts(3 hunks)apps/github/test/fixtures/check_run.completed.failure.json(1 hunks)apps/github/test/fixtures/check_run.completed.success.json(1 hunks)apps/github/test/fixtures/check_run.created.json(1 hunks)apps/github/test/fixtures/pull_request.opened.json(1 hunks)apps/github/test/index.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/github/src/index.tsapps/github/test/index.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/github/src/index.tsapps/github/test/index.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
🔇 Additional comments (6)
apps/github/test/fixtures/pull_request.opened.json (1)
1-24: PR opened fixture looks correct and minimalPayload includes the exact fields the bot_ci handler and tests rely on (head.sha, repo/owner, installation id, PR number/URL) without unnecessary noise. No changes needed.
apps/github/test/fixtures/check_run.completed.success.json (1)
1-27: Success check_run fixture matches handler expectationsThe fixture correctly models a completed bot_ci run with a PR association and matches what the tests/assertions expect. Good coverage baseline for the “success” path.
apps/github/src/index.ts (1)
137-214: Mapping bot_ci state to Mergeable check is clear and consistentThe
createOrUpdateMergeableCheckfunction cleanly encodes the merge policy:
- No bot_ci run → treat as successful/mergeable.
- Non-completed bot_ci → in_progress Mergeable check (blocks until completion).
- Successful bot_ci → completed/success.
- Any other conclusion → completed/failure, blocking merges.
Reusing
checks.listForRefto either update the existing Mergeable check or create a new one keeps the lifecycle simple and idempotent. This looks solid.apps/github/test/fixtures/check_run.completed.failure.json (1)
1-27: Failure check_run fixture correctly mirrors the success caseStructure and fields align with the success fixture, differing only in
conclusion: "failure". This is exactly what the failure-path tests need.apps/github/test/fixtures/check_run.created.json (1)
1-27: In-progress check_run fixture accurately represents the created stateThe combination of
action: "created",status: "in_progress",conclusion: null, and a PR association matches what the handler and tests expect for the “bot_ci created” scenario.apps/github/test/index.test.ts (1)
24-51: Fixture loading for new payloads is consistent and maintainableThe additional payload constants follow the existing pattern (read from
fixtures/*under__dirname) and keep test data centralized in JSON. This makes the bot_ci tests easy to reason about and extend.
- Add Mergeable: bot_ci check that monitors bot_ci status - Block merge when bot_ci is triggered but not passed (in_progress or failed) - Allow merge when bot_ci is not triggered - Add comprehensive tests for all scenarios - Revert changes to apps/github (moved to apps/bot) Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
devin-ai-integration
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/bot/src/index.ts (2)
13-17: Consider a type alias for readability.The context type
Parameters<Parameters<Probot["on"]>[1]>[0]is correct but verbose. Consider extracting a type alias if this pattern is reused.type ProbotContext = Parameters<Parameters<Probot["on"]>[1]>[0];
160-164: Consider logging the full error for debugging.Currently only the error message is logged. For debugging production issues, including the stack trace or full error object would be helpful.
} catch (error) { - context.log.error(`Failed to handle bot_ci check: ${error}`); + context.log.error({ err: error }, `Failed to handle bot_ci check`); }apps/bot/test/index.test.ts (2)
99-113: LGTM!Good use of typed
Probotinstead ofany. Consider updating the existing test suite at line 52 for consistency.
320-337: LGTM!Test correctly verifies that non-bot_ci checks are ignored.
Consider adding tests for edge cases in a follow-up:
check_runevent with emptypull_requestsarray- API failure scenarios (e.g., 500 response from GitHub)
check_run.rerequestedevent specifically
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/bot/src/index.ts(2 hunks)apps/bot/test/fixtures/check_run.completed.failure.json(1 hunks)apps/bot/test/fixtures/check_run.completed.success.json(1 hunks)apps/bot/test/fixtures/check_run.created.json(1 hunks)apps/bot/test/fixtures/pull_request.opened.json(1 hunks)apps/bot/test/index.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/bot/test/fixtures/check_run.completed.success.json
- apps/bot/test/fixtures/check_run.created.json
- apps/bot/test/fixtures/pull_request.opened.json
- apps/bot/test/fixtures/check_run.completed.failure.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/bot/src/index.tsapps/bot/test/index.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/bot/src/index.tsapps/bot/test/index.test.ts
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:32:24.348Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: apps/desktop-e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:24.348Z
Learning: Applies to apps/desktop-e2e/**/*.{test,spec}.{js,ts,tsx} : Refer to `scripts/setup-desktop-e2e.sh` for end-to-end test environment setup if setup is missing
Applied to files:
apps/bot/test/index.test.ts
🧬 Code graph analysis (2)
apps/bot/src/index.ts (1)
apps/github/src/index.ts (1)
app(101-150)
apps/bot/test/index.test.ts (1)
apps/web/scripts/gen-agents.js (1)
__dirname(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
🔇 Additional comments (11)
apps/bot/src/index.ts (5)
3-4: LGTM!Good practice to centralize check names as constants for maintainability and consistency.
6-11: LGTM!The interface is appropriately minimal, capturing only the fields needed for the bot_ci check logic.
39-116: LGTM!The logic correctly handles all four states (not triggered, in progress, success, failure) and properly creates or updates the check. The status/conclusion mapping aligns with the PR requirements.
118-126: LGTM!Clean orchestration function that maintains good separation of concerns.
168-190: LGTM with a note on race condition handling.The PR event handler correctly evaluates initial state. The potential race condition (PR opens before bot_ci is triggered) is properly mitigated by the
check_run.createdevent handler, which will update the Mergeable check when bot_ci starts.apps/bot/test/index.test.ts (6)
23-49: LGTM!Test fixtures are loaded consistently with the existing pattern and cover all necessary scenarios.
115-155: LGTM!Test correctly verifies that an in_progress check is created when bot_ci is created.
157-198: LGTM!Test correctly verifies that a success check is created when bot_ci passes.
200-241: LGTM!Test correctly verifies that a failure check is created when bot_ci fails.
243-274: LGTM!Test correctly verifies the "not triggered" scenario, which is important for allowing merges when bot_ci isn't required.
276-318: LGTM!Test correctly verifies that existing checks are updated via PATCH rather than creating duplicates.
- Extract bot_ci merge blocking logic to mergeable.ts - Keep index.ts clean with just app setup and handler registration Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/bot/src/mergeable.ts (1)
15-39: Clarify behavior when multiplebot_cicheck runs exist for the same ref
getBotCiCheckRuncurrently returnsdata.check_runs[0]fromchecks.listForRef. If GitHub ever returns multiplebot_ciruns for the same ref (e.g., re-runs), this implicitly relies on the API ordering such that index 0 is always the “right” one (typically the latest). It would be safer either to document this assumption or to explicitly select the desired run (e.g., by latest timestamp or by tightening the query) so future readers don’t have to reason about ordering guarantees.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/bot/src/index.ts(2 hunks)apps/bot/src/mergeable.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/bot/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/bot/src/mergeable.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/bot/src/mergeable.ts
🧬 Code graph analysis (1)
apps/bot/src/mergeable.ts (1)
apps/bot/src/index.ts (1)
app(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
🔇 Additional comments (2)
apps/bot/src/mergeable.ts (2)
41-118: Mergeable check mapping frombot_cistate to status/conclusion looks solidThe translation from
bot_cistate toMergeable: bot_cicheck is consistent with the PR contract:
botCiCheck === null→completed/success(“not triggered → merge allowed”).status !== "completed"→in_progresswith no conclusion (“waiting, merge blocked”).status === "completed" && conclusion === "success"→completed/success.- All other completed conclusions →
completed/failure(“blocked”).The create-vs-update logic via
checks.listForRefonMERGEABLE_CHECK_NAMEcorrectly prevents duplicate Mergeable checks while keeping the output title/summary in sync. No correctness issues spotted here.
130-184: Event wiring and guardrails for updating Mergeable checks are appropriateThe handlers:
- Listen to the right events (
check_run.created/completed/rerequestedandpull_request.opened/synchronize/reopened).- Filter on
checkRun.name === BOT_CI_CHECK_NAMEand require associated PRs for check_run events, avoiding accidental updates from unrelated checks.- Derive
owner,repo, andheadShaconsistently and delegate throughhandleBotCiCheck.- Wrap the core call in
try/catchwith logging, so failures won’t crash the app and will be debuggable.This is a clean, minimal integration point that aligns with the PR objectives.
1 participant
feat(bot): add bot_ci merge blocking check
Summary
Adds a "Mergeable: bot_ci" check to the
apps/botProbot app that monitors thebot_cicheck status and controls merge eligibility:The check listens to
check_run.created,check_run.completed,check_run.rerequestedevents for bot_ci, andpull_request.opened,pull_request.synchronize,pull_request.reopenedevents to evaluate the initial state.Updates since last revision
apps/githubtoapps/bot(the correct location, since bot_ci workflow triggers onapps/bot/**changes)index.tsby functionality: bot_ci merge blocking logic now lives inmergeable.tsReview & Testing Checklist for Human
"bot_ci"as the check name to monitor. Confirm this matches the actual check name in your CI workflows.checks: writepermission to create/update check runs.Recommended test plan:
apps/bot/**) - observe the "Mergeable: bot_ci" check behaviorNotes