Skip to content

Commit 68035f6

Browse files
committed
fix(github): reject a fractional comment line instead of truncating it
toLineNumber ran Math.trunc, so line 3.9 posted the review comment on line 3 — a silent change to what the caller asked for, on a field where landing on the wrong line of the diff is invisible until someone reads the comment. A non-integer now fails with a message naming the field, matching how a missing head commit SHA fails on this path. Blank and unparseable input is still omitted: line is optional and nothing usable was supplied.
1 parent 1403919 commit 68035f6

2 files changed

Lines changed: 60 additions & 6 deletions

File tree

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,44 @@ describe('github_comment line coercion', () => {
286286
expect(calls()[1].body).toMatchObject({ line: 42 })
287287
})
288288

289+
it('keeps an integer line typed with surrounding whitespace', async () => {
290+
secureGitHubRequest.mockResolvedValueOnce(createdCommentResponse())
291+
292+
await commentTool.directExecution!({
293+
...FILE_COMMENT_PARAMS,
294+
commitId: OTHER_SHA,
295+
line: ' 42 ' as unknown as number,
296+
})
297+
298+
expect(calls()[0].body).toMatchObject({ line: 42 })
299+
})
300+
301+
it('rejects a fractional line rather than silently moving the comment', async () => {
302+
secureGitHubRequest.mockResolvedValueOnce(createdCommentResponse())
303+
304+
await expect(
305+
commentTool.directExecution!({
306+
...FILE_COMMENT_PARAMS,
307+
commitId: OTHER_SHA,
308+
line: 3.9,
309+
})
310+
).rejects.toThrow('GitHub line numbers are whole numbers, but line was 3.9')
311+
expect(secureGitHubRequest).not.toHaveBeenCalled()
312+
})
313+
314+
it('rejects a fractional line typed into the short input', async () => {
315+
secureGitHubRequest.mockResolvedValueOnce(createdCommentResponse())
316+
317+
await expect(
318+
commentTool.directExecution!({
319+
...FILE_COMMENT_PARAMS,
320+
commitId: OTHER_SHA,
321+
line: '3.9' as unknown as number,
322+
})
323+
).rejects.toThrow('GitHub line numbers are whole numbers, but line was 3.9')
324+
expect(secureGitHubRequest).not.toHaveBeenCalled()
325+
})
326+
289327
it('omits a blank or unparseable line rather than sending NaN', async () => {
290328
for (const line of ['', ' ', 'abc', undefined, null]) {
291329
secureGitHubRequest.mockReset()

apps/sim/tools/github/comment.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,14 +64,30 @@ function needsCommitLookup(params: CreateCommentParams): boolean {
6464

6565
/**
6666
* The block renders `line` as a short input, so a typed line number reaches the tool
67-
* as a string while GitHub types the field as an integer. Anything that is not a
68-
* finite number is omitted rather than sent as `NaN`.
67+
* as a string while GitHub types the field as an integer. Blank and unparseable input
68+
* is omitted rather than sent as `NaN` — `line` is optional, and nothing usable was
69+
* supplied.
70+
*
71+
* A fractional value is rejected instead of truncated. `3.9` is not the caller asking
72+
* for line 3, and quietly posting the review comment on a different line of the diff
73+
* than the one they named is the failure they would never think to look for. This
74+
* fails the way a missing head commit SHA does: loudly, naming what to set.
6975
*/
7076
function toLineNumber(value: unknown): number | undefined {
71-
if (typeof value === 'number') return Number.isFinite(value) ? Math.trunc(value) : undefined
72-
if (typeof value !== 'string' || !value.trim()) return undefined
73-
const parsed = Number(value.trim())
74-
return Number.isFinite(parsed) ? Math.trunc(parsed) : undefined
77+
let parsed: number
78+
if (typeof value === 'number') {
79+
parsed = value
80+
} else {
81+
if (typeof value !== 'string' || !value.trim()) return undefined
82+
parsed = Number(value.trim())
83+
}
84+
if (!Number.isFinite(parsed)) return undefined
85+
if (!Number.isInteger(parsed)) {
86+
throw new Error(
87+
`GitHub line numbers are whole numbers, but line was ${parsed}. Set line to the integer line number in the diff.`
88+
)
89+
}
90+
return parsed
7591
}
7692

7793
function fileCommentBody(

0 commit comments

Comments
 (0)