Skip to content

Commit ae542d9

Browse files
committed
fix(github): select the comment endpoint by comment type, not by path
The endpoint was chosen by the presence of path, while the body was chosen by commentType, so a pr_comment naming a file posted a review body to POST /pulls/{n}/comments. GitHub documents body, commit_id and path as required there, so that request can only ever 422 — it has been broken since before this PR. The endpoint now follows the comment type: only a file comment carrying a path uses /comments, everything else stays on /reviews. The test that codified the broken routing is corrected, and the full type/path matrix is pinned.
1 parent 68035f6 commit ae542d9

2 files changed

Lines changed: 67 additions & 11 deletions

File tree

apps/sim/tools/github/comment.test.ts

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,12 @@ describe('github_comment routing', () => {
109109
])
110110
})
111111

112-
it('never looks the pull request up for a general PR comment carrying a path', async () => {
112+
/**
113+
* `POST /pulls/{n}/comments` documents `body`, `commit_id` and `path` as required,
114+
* so a review body sent there is a guaranteed 422. The endpoint therefore follows
115+
* the comment type, not the presence of a path.
116+
*/
117+
it('keeps a general PR comment carrying a path on the reviews endpoint', async () => {
113118
secureGitHubRequest.mockResolvedValueOnce(createdCommentResponse())
114119

115120
await commentTool.directExecution!({
@@ -124,7 +129,29 @@ describe('github_comment routing', () => {
124129

125130
expect(calls()).toEqual([
126131
{
127-
url: 'https://api.github.com/repos/octo/demo/pulls/7/comments',
132+
url: 'https://api.github.com/repos/octo/demo/pulls/7/reviews',
133+
method: 'POST',
134+
body: { body: 'Nice', event: 'COMMENT' },
135+
signal: undefined,
136+
},
137+
])
138+
})
139+
140+
it('keeps a comment with no type carrying a path on the reviews endpoint', async () => {
141+
secureGitHubRequest.mockResolvedValueOnce(createdCommentResponse())
142+
143+
await commentTool.directExecution!({
144+
owner: 'octo',
145+
repo: 'demo',
146+
pullNumber: 7,
147+
body: 'Nice',
148+
path: 'src/main.ts',
149+
apiKey: 'ghp_test',
150+
})
151+
152+
expect(calls()).toEqual([
153+
{
154+
url: 'https://api.github.com/repos/octo/demo/pulls/7/reviews',
128155
method: 'POST',
129156
body: { body: 'Nice', event: 'COMMENT' },
130157
signal: undefined,
@@ -214,6 +241,24 @@ describe('github_comment routing', () => {
214241
expect(commentTool.params.position).toBeUndefined()
215242
})
216243

244+
it('routes every comment type / path combination to the endpoint that accepts it', () => {
245+
const url = commentTool.request.url as (params: CreateCommentParams) => string
246+
const base = { owner: 'octo', repo: 'demo', pullNumber: 7, body: 'Nice', apiKey: 'ghp_test' }
247+
const reviews = 'https://api.github.com/repos/octo/demo/pulls/7/reviews'
248+
const comments = 'https://api.github.com/repos/octo/demo/pulls/7/comments'
249+
250+
expect(url({ ...base, commitId: OTHER_SHA })).toBe(reviews)
251+
expect(url({ ...base, commitId: OTHER_SHA, path: 'src/main.ts' })).toBe(reviews)
252+
expect(url({ ...base, commitId: OTHER_SHA, commentType: 'pr_comment' })).toBe(reviews)
253+
expect(
254+
url({ ...base, commitId: OTHER_SHA, commentType: 'pr_comment', path: 'src/main.ts' })
255+
).toBe(reviews)
256+
expect(url({ ...base, commitId: OTHER_SHA, commentType: 'file_comment' })).toBe(reviews)
257+
expect(
258+
url({ ...base, commitId: OTHER_SHA, commentType: 'file_comment', path: 'src/main.ts' })
259+
).toBe(comments)
260+
})
261+
217262
it('keeps the declarative request in step with the executed routing', () => {
218263
const url = commentTool.request.url as (params: CreateCommentParams) => string
219264
const method = commentTool.request.method as (params: CreateCommentParams) => string

apps/sim/tools/github/comment.ts

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,18 +48,26 @@ function pullRequestUrl(params: CreateCommentParams): string {
4848
return `${GITHUB_API_BASE}/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}`
4949
}
5050

51+
/**
52+
* Whether the request is headed for `POST /pulls/{n}/comments`. GitHub documents
53+
* `body`, `commit_id` and `path` as required there, so only a file comment that
54+
* actually carries a path can use it. `path` is optional on the block, so a file
55+
* comment left without one falls back to `/pulls/{n}/reviews`, where GitHub creates
56+
* a pending review and neither field is required.
57+
*/
58+
function isFileCommentRequest(params: CreateCommentParams): boolean {
59+
return params.commentType === 'file_comment' && Boolean(params.path)
60+
}
61+
5162
/**
5263
* GitHub requires `commit_id` on a pull request review comment. When the caller did
5364
* not supply one, the pull request is fetched first so its head SHA can be used —
5465
* mirroring how Jira resolves a missing `cloudId` from `domain`.
5566
*
56-
* The lookup is gated on `path` because only a request headed for the `/comments`
57-
* endpoint needs a commit SHA. `path` is optional on the block, so a file comment
58-
* left without one still falls through to `/pulls/{n}/reviews`, where GitHub creates
59-
* a pending review and `commit_id` is optional.
67+
* The lookup is gated on the endpoint, because only `/comments` needs a commit SHA.
6068
*/
6169
function needsCommitLookup(params: CreateCommentParams): boolean {
62-
return params.commentType === 'file_comment' && Boolean(params.path) && !params.commitId
70+
return isFileCommentRequest(params) && !params.commitId
6371
}
6472

6573
/**
@@ -104,12 +112,15 @@ function fileCommentBody(
104112
}
105113

106114
/**
107-
* The endpoint the comment itself is posted to. `path` selects the review-comment
108-
* endpoint; everything else lands on the reviews endpoint, where GitHub creates a
109-
* pending review whose `commit_id` is optional.
115+
* The endpoint the comment itself is posted to. The comment TYPE selects it, not the
116+
* mere presence of `path`: a general PR comment sends `{body, event}`, which the
117+
* review-comment endpoint rejects with a 422 for the missing `commit_id` and `path`,
118+
* so a `pr_comment` that happens to name a file has to stay on `/reviews`.
110119
*/
111120
function commentEndpointUrl(params: CreateCommentParams): string {
112-
return params.path ? `${pullRequestUrl(params)}/comments` : `${pullRequestUrl(params)}/reviews`
121+
return isFileCommentRequest(params)
122+
? `${pullRequestUrl(params)}/comments`
123+
: `${pullRequestUrl(params)}/reviews`
113124
}
114125

115126
function commentRequestBody(

0 commit comments

Comments
 (0)