Skip to content

Commit b321202

Browse files
committed
fix(github): address review findings on pr.ts and delete_comment.ts
- pr.ts: surface a real failure instead of silently returning success:true with an empty files array when the files sub-fetch fails; unify the sub-fetch Accept header with the rest of the file - delete_comment: success now tracks the actual deletion outcome instead of being hardcoded true
1 parent 1f0469d commit b321202

2 files changed

Lines changed: 63 additions & 5 deletions

File tree

apps/sim/tools/github/delete_comment.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ export const deleteCommentTool: ToolConfig<DeleteCommentParams, DeleteCommentRes
5353
: `Failed to delete comment #${commentId}`
5454

5555
return {
56-
success: true,
56+
success: deleted,
5757
output: {
5858
content,
5959
metadata: {
@@ -86,10 +86,11 @@ export const deleteCommentV2Tool: ToolConfig<DeleteCommentParams, any> = {
8686
request: deleteCommentTool.request,
8787

8888
transformResponse: async (response: Response, params) => {
89+
const deleted = response.status === 204
8990
return {
90-
success: true,
91+
success: deleted,
9192
output: {
92-
deleted: response.status === 204,
93+
deleted,
9394
comment_id: params?.comment_id || 0,
9495
},
9596
}

apps/sim/tools/github/pr.ts

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,34 @@ export const prTool: ToolConfig<PROperationParams, PullRequestResponse> = {
5252
`https://api.github.com/repos/${pr.base.repo.owner.login}/${pr.base.repo.name}/pulls/${pr.number}/files`,
5353
{
5454
headers: {
55-
Accept: 'application/vnd.github.v3+json',
55+
Accept: 'application/vnd.github+json',
5656
Authorization: `Bearer ${params?.apiKey}`,
5757
'X-GitHub-Api-Version': '2022-11-28',
5858
},
5959
}
6060
)
61+
62+
if (!filesResponse.ok) {
63+
const error = await filesResponse.json().catch(() => ({}))
64+
return {
65+
success: false,
66+
error: error.message || `Failed to fetch PR files (HTTP ${filesResponse.status})`,
67+
output: {
68+
content: '',
69+
metadata: {
70+
number: pr.number,
71+
title: pr.title,
72+
state: pr.state,
73+
html_url: pr.html_url,
74+
diff_url: pr.diff_url,
75+
created_at: pr.created_at,
76+
updated_at: pr.updated_at,
77+
files: [],
78+
},
79+
},
80+
}
81+
}
82+
6183
const filesJson = await filesResponse.json()
6284
const files = Array.isArray(filesJson) ? filesJson : []
6385

@@ -143,12 +165,47 @@ export const prV2Tool: ToolConfig<PROperationParams, any> = {
143165
`https://api.github.com/repos/${pr.base.repo.owner.login}/${pr.base.repo.name}/pulls/${pr.number}/files`,
144166
{
145167
headers: {
146-
Accept: 'application/vnd.github.v3+json',
168+
Accept: 'application/vnd.github+json',
147169
Authorization: `Bearer ${params?.apiKey}`,
148170
'X-GitHub-Api-Version': '2022-11-28',
149171
},
150172
}
151173
)
174+
175+
if (!filesResponse.ok) {
176+
const error = await filesResponse.json().catch(() => ({}))
177+
return {
178+
success: false,
179+
error: error.message || `Failed to fetch PR files (HTTP ${filesResponse.status})`,
180+
output: {
181+
id: pr.id,
182+
number: pr.number,
183+
title: pr.title,
184+
state: pr.state,
185+
html_url: pr.html_url,
186+
diff_url: pr.diff_url,
187+
body: pr.body ?? null,
188+
user: pr.user,
189+
head: pr.head,
190+
base: pr.base,
191+
merged: pr.merged,
192+
mergeable: pr.mergeable ?? null,
193+
merged_by: pr.merged_by ?? null,
194+
comments: pr.comments,
195+
review_comments: pr.review_comments,
196+
commits: pr.commits,
197+
additions: pr.additions,
198+
deletions: pr.deletions,
199+
changed_files: pr.changed_files,
200+
created_at: pr.created_at,
201+
updated_at: pr.updated_at,
202+
closed_at: pr.closed_at ?? null,
203+
merged_at: pr.merged_at ?? null,
204+
files: [],
205+
},
206+
}
207+
}
208+
152209
const filesJson = await filesResponse.json()
153210
const files = Array.isArray(filesJson) ? filesJson : []
154211

0 commit comments

Comments
 (0)