Add GitHub PR merge writeback client#148
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds GithubClient.mergePullRequest that writes a merge draft JSON to a PR’s relayfile path, extends receipt shapes and polling to recognize merged receipts and SHA, includes unit tests for write-and-wait behavior, and updates example/e2e mocks to stub mergePullRequest. ChangesGitHub mergePullRequest operation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 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)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime/src/clients/github.test.ts (1)
85-108: ⚡ Quick winAdd a regression test for empty merge metadata preservation.
This test validates non-empty metadata, but it doesn’t lock in the behavior from the fix commit (preserving empty strings). Please add a case where
commitTitle: ''and/orcommitMessage: ''and assert those keys are still present inmerge.json.Proposed test addition
+test('github.mergePullRequest preserves empty commit metadata fields', async () => { + const root = await tempMount(); + try { + const client = createGithubClient({ relayfileMountRoot: root, writebackTimeoutMs: 0 }); + await client.mergePullRequest({ + owner: 'o', + repo: 'r', + number: 42, + commitTitle: '', + commitMessage: '' + }); + + const mergePath = path.join(root, 'github/repos/o/r/pulls/42/merge.json'); + assert.deepEqual(JSON.parse(await readFile(mergePath, 'utf8')), { + method: 'squash', + commitTitle: '', + commitMessage: '' + }); + } finally { + await rm(root, { recursive: true, force: true }); + } +});🤖 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/runtime/src/clients/github.test.ts` around lines 85 - 108, The existing test github.mergePullRequest writes non-empty merge metadata but misses a regression case for preserving empty strings; update or add a test (e.g., in the same test block or a new test alongside test('github.mergePullRequest writes a merge draft under pulls/<n>/merge.json', ...)) that calls createGithubClient(...).mergePullRequest with commitTitle: '' and commitMessage: '' (keeping other args identical) and assert that the resulting merge.json (path built like in the test: root/github/repos/o/r/pulls/42/merge.json) contains the keys commitTitle and commitMessage with empty-string values (not omitted or undefined); ensure cleanup logic (rm(root,...)) remains as in the original test.
🤖 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.
Nitpick comments:
In `@packages/runtime/src/clients/github.test.ts`:
- Around line 85-108: The existing test github.mergePullRequest writes non-empty
merge metadata but misses a regression case for preserving empty strings; update
or add a test (e.g., in the same test block or a new test alongside
test('github.mergePullRequest writes a merge draft under pulls/<n>/merge.json',
...)) that calls createGithubClient(...).mergePullRequest with commitTitle: ''
and commitMessage: '' (keeping other args identical) and assert that the
resulting merge.json (path built like in the test:
root/github/repos/o/r/pulls/42/merge.json) contains the keys commitTitle and
commitMessage with empty-string values (not omitted or undefined); ensure
cleanup logic (rm(root,...)) remains as in the original test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f6cea0d-5c5f-4a63-891c-58996681b8a9
📒 Files selected for processing (2)
packages/runtime/src/clients/github.test.tspackages/runtime/src/clients/github.ts
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/clients/request.ts (1)
239-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReceipt detection doesn't handle
mergedas a string.
WritebackReceiptdeclaresmerged?: boolean | string, andmergePullRequesthandlesreceipt.merged === 'true'. However,waitForReceiptonly checkstypeof parsed.merged === 'boolean'. If the writeback returns{ merged: 'true' }(string) without other receipt fields, detection will fail and timeout.Proposed fix
if ( isRecord(parsed) && (typeof parsed.created === 'string' || typeof parsed.path === 'string' || typeof parsed.id === 'string' || typeof parsed.externalId === 'string' || - typeof parsed.merged === 'boolean') + typeof parsed.merged === 'boolean' || + typeof parsed.merged === 'string') ) {🤖 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/runtime/src/clients/request.ts` around lines 239 - 248, waitForReceipt's detection logic only accepts parsed.merged when it's a boolean, but WritebackReceipt declares merged?: boolean | string and mergePullRequest treats 'true' strings as valid; update the conditional in waitForReceipt that checks parsed to also accept typeof parsed.merged === 'string' (in addition to 'boolean') so receipts like { merged: 'true' } are recognized and returned as a WritebackReceipt; reference symbols: parsed, waitForReceipt, WritebackReceipt, merged, mergePullRequest.
🤖 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/runtime/src/clients/github.test.ts`:
- Around line 125-129: The test uses void writeFile(...) inside the setTimeout
which fire-and-forgets the write and can hide rejections; change the timeout
handler to capture the writeFile promise (e.g., assign to a local writePromise)
and then await it together with resultPromise (for example via Promise.all or
awaiting both) so any writeFile rejection surfaces; reference writeFile,
mergePath, resultPromise and the setTimeout callback when making this change.
---
Outside diff comments:
In `@packages/runtime/src/clients/request.ts`:
- Around line 239-248: waitForReceipt's detection logic only accepts
parsed.merged when it's a boolean, but WritebackReceipt declares merged?:
boolean | string and mergePullRequest treats 'true' strings as valid; update the
conditional in waitForReceipt that checks parsed to also accept typeof
parsed.merged === 'string' (in addition to 'boolean') so receipts like { merged:
'true' } are recognized and returned as a WritebackReceipt; reference symbols:
parsed, waitForReceipt, WritebackReceipt, merged, mergePullRequest.
🪄 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: b5ce56e0-ebe3-45f3-b0b8-d68b1a45e4a9
📒 Files selected for processing (5)
examples/notion-essay-pr/notion-essay-pr.smoke.test.tspackages/deploy/test/e2e/notion-essay-pr.smoke.test.tspackages/runtime/src/clients/github.test.tspackages/runtime/src/clients/github.tspackages/runtime/src/clients/request.ts
2 participants
Summary
ctx.github.mergePullRequest(...)to the VFS-backed runtime GitHub client./github/repos/{owner}/{repo}/pulls/{number}/merge.jsonwithmethod,commitTitle, andcommitMessagesupport.Part of #147.
Companion PRs
Validation
corepack pnpm --filter @agentworkforce/persona-kit build && corepack pnpm --filter @agentworkforce/runtime testgit diff --check HEAD~1..HEAD