Skip to content

Commit 83b16ae

Browse files
committed
fix(github): gate the commit lookup on path, coerce line, name the failing field
Three corrections to the file-comment fix, from a validation sweep. needsCommitLookup did not check `path`, and ran before the `path` branch in request.url. A file comment with an empty File Path — reachable, since path is not required on the block — went GET /pulls/{n} then POST /comments with path undefined, a 422. On staging it posted to /pulls/{n}/reviews, which GitHub documents as creating a pending review and where commit_id is optional. The lookup is now gated on path, so only a request headed for /comments triggers it. The block has no tools.config.params, so `line` reached the tool as the string the short-input produced while GitHub types it as an integer — file comments would still have 422'd, one API call later. Coerced in request.body, which runs at execution; anything non-finite is omitted rather than sent as NaN. readGitHubErrorMessage returned only the top-level message, so a 422 read "Validation Failed" with no indication of which field was rejected. The errors[] detail is now appended. Responses without errors[] are unchanged.
1 parent a4d9a73 commit 83b16ae

2 files changed

Lines changed: 46 additions & 3 deletions

File tree

apps/sim/tools/github/comment.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,34 @@ function pullRequestUrl(params: CreateCommentParams): string {
2222
* GitHub requires `commit_id` on a pull request review comment. When the caller did
2323
* not supply one, the pull request is fetched first so its head SHA can be used —
2424
* mirroring how Jira resolves a missing `cloudId` from `domain`.
25+
*
26+
* The lookup is gated on `path` because only a request headed for the `/comments`
27+
* endpoint needs a commit SHA. `path` is optional on the block, so a file comment
28+
* left without one still falls through to `/pulls/{n}/reviews`, where GitHub creates
29+
* a pending review and `commit_id` is optional.
2530
*/
2631
function needsCommitLookup(params: CreateCommentParams): boolean {
27-
return params.commentType === 'file_comment' && !params.commitId
32+
return params.commentType === 'file_comment' && Boolean(params.path) && !params.commitId
33+
}
34+
35+
/**
36+
* The block renders `line` as a short input, so a typed line number reaches the tool
37+
* as a string while GitHub types the field as an integer. Anything that is not a
38+
* finite number is omitted rather than sent as `NaN`.
39+
*/
40+
function toLineNumber(value: unknown): number | undefined {
41+
if (typeof value === 'number') return Number.isFinite(value) ? Math.trunc(value) : undefined
42+
if (typeof value !== 'string' || !value.trim()) return undefined
43+
const parsed = Number(value.trim())
44+
return Number.isFinite(parsed) ? Math.trunc(parsed) : undefined
2845
}
2946

3047
function fileCommentBody(params: CreateCommentParams, commitId: string): Record<string, any> {
3148
return {
3249
body: params.body,
3350
commit_id: commitId,
3451
path: params.path,
35-
line: params.line,
52+
line: toLineNumber(params.line),
3653
side: params.side || 'RIGHT',
3754
}
3855
}

apps/sim/tools/github/response-parsers.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,38 @@ export function requiredRecord(
137137
return value
138138
}
139139

140+
/**
141+
* Renders one entry of GitHub's `errors[]` array. Entries are either a bare string or
142+
* an object carrying some combination of `field`, `code`, and `message`.
143+
*/
144+
function readGitHubErrorEntry(entry: unknown): string | undefined {
145+
if (typeof entry === 'string') return entry.trim() || undefined
146+
if (!isRecordLike(entry)) return undefined
147+
const field = typeof entry.field === 'string' ? entry.field.trim() : ''
148+
const message = typeof entry.message === 'string' ? entry.message.trim() : ''
149+
const code = typeof entry.code === 'string' ? entry.code.trim() : ''
150+
const detail = message || code
151+
if (!detail) return field || undefined
152+
return field ? `${field}: ${detail}` : detail
153+
}
154+
155+
/**
156+
* A GitHub 422 names the offending field only in `errors[]` — the top-level `message`
157+
* is the useless `"Validation Failed"`. The field-level detail is appended so the user
158+
* can tell which input was rejected. Responses without an `errors[]` array, and
159+
* responses without a top-level `message` at all, are unchanged.
160+
*/
140161
export async function readGitHubErrorMessage(response: Response): Promise<string | undefined> {
141162
try {
142163
const value: unknown = await response.json()
143164
if (!isRecordLike(value)) return undefined
144165
const message = value.message
145-
return typeof message === 'string' && message.trim() ? message : undefined
166+
if (typeof message !== 'string' || !message.trim()) return undefined
167+
if (!Array.isArray(value.errors)) return message
168+
const details = value.errors
169+
.map(readGitHubErrorEntry)
170+
.filter((detail): detail is string => Boolean(detail))
171+
return details.length ? `${message}: ${details.join('; ')}` : message
146172
} catch {
147173
return undefined
148174
}

0 commit comments

Comments
 (0)