diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index f68726b39..0f65fc157 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -239,6 +239,163 @@ describe('issue source operations', () => { ) }) + it('recovers issue 7704 oversized inline-image creation', async () => { + const imagePrefix = 'data:image/png;base64,' + const body = imagePrefix + 'x'.repeat(133596 - imagePrefix.length) + expect(body).toContain('data:image') + expect(body).toHaveLength(133596) + + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 422: body is too long (maximum is 65536 characters)')) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 926, + html_url: 'https://github.com/stablyai/orca/issues/926' + }) + }) + .mockResolvedValueOnce({ stdout: '' }) + + await expect( + createIssue('/repo-root', 'Image issue', body, undefined, undefined, { + labels: ['bug'], + assignees: ['octo'] + }) + ).resolves.toEqual({ + ok: true, + number: 926, + url: 'https://github.com/stablyai/orca/issues/926' + }) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + expect.arrayContaining([ + `body=${body}`, + 'title=Image issue', + 'labels[]=bug', + 'assignees[]=octo' + ]), + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + expect.arrayContaining(['body=', 'title=Image issue', 'labels[]=bug', 'assignees[]=octo']), + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 3, + ['api', '-X', 'PATCH', 'repos/stablyai/orca/issues/926', '--raw-field', `body=${body}`], + { cwd: '/repo-root' } + ) + }) + + it('recognizes the oversized-body response from structured gh stderr', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock + .mockRejectedValueOnce( + Object.assign(new Error('Command failed: gh'), { + stderr: 'gh: body is too long (maximum is 65536 characters) (HTTP 422)' + }) + ) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 929, + html_url: 'https://github.com/stablyai/orca/issues/929' + }) + }) + .mockResolvedValueOnce({ stdout: '' }) + + await expect(createIssue('/repo-root', 'Image issue', 'data:image')).resolves.toEqual({ + ok: true, + number: 929, + url: 'https://github.com/stablyai/orca/issues/929' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) + }) + + it('does not retry unrelated create failures', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock.mockRejectedValueOnce(new Error('HTTP 422: assignees is invalid')) + + await expect(createIssue('/repo-root', 'Invalid issue', 'Body')).resolves.toEqual({ + ok: false, + error: 'HTTP 422: assignees is invalid' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('stops when placeholder create fails during oversized-body recovery', async () => { + const body = `data:image/png;base64,${'x'.repeat(100)}` + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('body is too long (maximum is 65536 characters)')) + .mockRejectedValueOnce(new Error('HTTP 500: create failed')) + + await expect(createIssue('/repo-root', 'Placeholder failure', body)).resolves.toEqual({ + ok: false, + error: 'HTTP 500: create failed' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('preserves fields during oversized-body recovery', async () => { + const localGitOptions = { wslDistro: 'Ubuntu' } + const body = `data:image/png;base64,${'x'.repeat(100)}` + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('body is too long (maximum is 65536 characters)')) + .mockResolvedValueOnce({ stdout: JSON.stringify({ number: 927, url: 'issue-url' }) }) + .mockResolvedValueOnce({ stdout: '' }) + + await expect( + createIssue( + '/repo-root', + 'Fields issue', + body, + undefined, + null, + { + labels: ['bug'], + assignees: ['octo'] + }, + localGitOptions + ) + ).resolves.toEqual({ ok: true, number: 927, url: 'issue-url' }) + const firstCreateArgs = [...ghExecFileAsyncMock.mock.calls[0][0]] + const fallbackCreateArgs = [...ghExecFileAsyncMock.mock.calls[1][0]] + firstCreateArgs[firstCreateArgs.indexOf(`body=${body}`)] = 'body=' + expect(fallbackCreateArgs).toEqual(firstCreateArgs) + expect(ghExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( + true + ) + expect(resolveIssueSourceMock).toHaveBeenCalledWith( + '/repo-root', + undefined, + null, + localGitOptions + ) + }) + + it('reports partial success when oversized-body patch fails', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('body is too long (maximum is 65536 characters)')) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 928, + html_url: 'https://github.com/stablyai/orca/issues/928' + }) + }) + .mockRejectedValueOnce(new Error('HTTP 500: update failed')) + + await expect(createIssue('/repo-root', 'Partial issue', 'data:image')).resolves.toEqual({ + ok: true, + number: 928, + url: 'https://github.com/stablyai/orca/issues/928', + bodySaveWarning: + 'Issue https://github.com/stablyai/orca/issues/928 was created, but saving its body failed: HTTP 500: update failed' + }) + }) + it('updates issue body through the REST issue endpoint', async () => { getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index 8f80fc645..b0f3fa6cb 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -6,6 +6,7 @@ import type { ClassifiedError, GitHubAssignableUser, GitHubCreateIssueFields, + GitHubCreateIssueResult, GitHubCommentResult, GitHubIssueUpdate, IssueInfo, @@ -15,7 +16,7 @@ import type { import { mapIssueInfo } from './mappers' import type { LocalGitExecOptions, OwnerRepo } from './gh-utils' // prettier-ignore -import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext } from './gh-utils' +import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' // Why: distinguishes a successful-empty listing from a failed fetch. The // previous `catch { return [] }` conflated a 403 on a private upstream with an @@ -31,6 +32,11 @@ export type IssueListResult = { error?: ClassifiedError } +function githubIssueErrorMessage(error: unknown): string { + const { stderr, stdout } = extractExecError(error) + return stderr.trim() || stdout.trim() +} + /** * Get a single issue by number. * Uses gh api --cache so 304 Not Modified responses don't count against the rate limit. @@ -162,7 +168,7 @@ export async function createIssue( connectionId?: string | null, fields?: GitHubCreateIssueFields, localGitOptions: LocalGitExecOptions = {} -): Promise<{ ok: true; number: number; url: string } | { ok: false; error: string }> { +): Promise { const trimmedTitle = title.trim() if (!trimmedTitle) { return { ok: false, error: 'Title is required' } @@ -180,25 +186,71 @@ export async function createIssue( } await acquire() try { - const args = [ - 'api', - '-X', - 'POST', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues`, - '--raw-field', - `title=${trimmedTitle}`, - '--raw-field', - `body=${body}` - ] - for (const label of fields?.labels ?? []) { - args.push('--raw-field', `labels[]=${label}`) - } - for (const assignee of fields?.assignees ?? []) { - args.push('--raw-field', `assignees[]=${assignee}`) + const createArgs = (issueBody: string) => { + const args = [ + 'api', + '-X', + 'POST', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues`, + '--raw-field', + `title=${trimmedTitle}`, + '--raw-field', + `body=${issueBody}` + ] + for (const label of fields?.labels ?? []) { + args.push('--raw-field', `labels[]=${label}`) + } + for (const assignee of fields?.assignees ?? []) { + args.push('--raw-field', `assignees[]=${assignee}`) + } + return args + } + + const parseIssue = (stdout: string) => + JSON.parse(stdout) as { number?: number; html_url?: string; url?: string } + + let data: { number?: number; html_url?: string; url?: string } + try { + const { stdout } = await ghExecFileAsync(createArgs(body), ghOptions) + data = parseIssue(stdout) + } catch (err) { + const message = githubIssueErrorMessage(err) + if (!/body is too long \(maximum is \d+ characters\)/i.test(message)) { + return { ok: false, error: message } + } + + // Why: GitHub rejects oversized bodies on create but accepts the same body + // on update, so establish the issue before attaching its body. + const { stdout } = await ghExecFileAsync(createArgs(''), ghOptions) + data = parseIssue(stdout) + if (typeof data.number !== 'number') { + return { ok: false, error: 'Unexpected response from GitHub' } + } + + try { + await ghExecFileAsync( + [ + 'api', + '-X', + 'PATCH', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${data.number}`, + '--raw-field', + `body=${body}` + ], + ghOptions + ) + } catch (patchErr) { + const patchMessage = githubIssueErrorMessage(patchErr) + const identity = data.html_url ?? data.url ?? `#${data.number}` + return { + ok: true, + number: data.number, + url: String(data.html_url ?? data.url ?? ''), + bodySaveWarning: `Issue ${identity} was created, but saving its body failed: ${patchMessage}` + } + } } - const { stdout } = await ghExecFileAsync(args, ghOptions) - const data = JSON.parse(stdout) as { number?: number; html_url?: string; url?: string } if (typeof data.number !== 'number') { return { ok: false, error: 'Unexpected response from GitHub' } } @@ -208,8 +260,7 @@ export async function createIssue( url: String(data.html_url ?? data.url ?? '') } } catch (err) { - const message = err instanceof Error ? err.message : String(err) - return { ok: false, error: message } + return { ok: false, error: githubIssueErrorMessage(err) } } finally { release() } diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 773135597..7c9f989be 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -89,6 +89,7 @@ import type { GitStatusResult, GitUpstreamStatus, GitHubAssignableUser, + GitHubCreateIssueResult, GitHubPRFile, GitHubPRFileContents, GitHubPrStartPoint, @@ -1488,7 +1489,7 @@ export type PreloadApi = { body: string labels?: string[] assignees?: string[] - }) => Promise<{ ok: true; number: number; url: string } | { ok: false; error: string }> + }) => Promise countWorkItems: (args: { repoPath: string; repoId?: string; query?: string }) => Promise listWorkItems: (args: { repoPath: string diff --git a/src/preload/index.ts b/src/preload/index.ts index 4ad832fa8..059e2aa4e 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -24,6 +24,7 @@ import type { GitHubPRRefreshReason, GitHubAssignableUser, GitHubCommentResult, + GitHubCreateIssueResult, GitHubWorkItem, JiraProjectStatusOrder, GitPushTarget, @@ -1262,8 +1263,7 @@ const api = { body: string labels?: string[] assignees?: string[] - }): Promise<{ ok: true; number: number; url: string } | { ok: false; error: string }> => - ipcRenderer.invoke('gh:createIssue', args), + }): Promise => ipcRenderer.invoke('gh:createIssue', args), countWorkItems: (args: { repoPath: string diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 8d37800b8..c42be0d7b 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -6865,7 +6865,9 @@ export default function TaskPage(): React.JSX.Element { labels: newIssueLabels, assignees: newIssueAssignees.map((assignee) => assignee.login) }, - { timeoutMs: 30_000 } + // Why: oversized-body recovery can require two 30-second writes + // after GitHub rejects the initial create request. + { timeoutMs: 65_000 } ) : await window.api.gh.createIssue({ repoPath: newIssueTargetRepo.path, @@ -6883,28 +6885,42 @@ export default function TaskPage(): React.JSX.Element { ) return } - toast.success( - translate('auto.components.TaskPage.3f9604efc7', 'Opened issue #{{value0}}', { - value0: result.number - }), - { - action: result.url - ? { - label: translate('auto.components.TaskPage.9c57663908', 'View'), - onClick: () => window.open(result.url, '_blank') - } - : undefined - } + const createdIssueToast = translate( + 'auto.components.TaskPage.3f9604efc7', + 'Opened issue #{{value0}}', + { value0: result.number } ) + const createdIssueToastOptions = { + action: result.url + ? { + label: translate('auto.components.TaskPage.9c57663908', 'View'), + onClick: () => window.open(result.url, '_blank') + } + : undefined + } + if (result.bodySaveWarning) { + toast.warning(createdIssueToast, { + ...createdIssueToastOptions, + description: result.bodySaveWarning + }) + } else { + toast.success(createdIssueToast, createdIssueToastOptions) + } setNewIssueOpen(false) - setNewIssueTitle('') - setNewIssueBody('') - setNewIssueLabels([]) - setNewIssueAssignees([]) - // Why: a successful submit is the only path that discards the recovery - // draft. Closing `newIssueOpen` in the same commit keeps the write-through - // effect (gated on it) from re-persisting the emptied fields. - clearNewIssueDraft() + if (result.bodySaveWarning) { + // Why: retain the unsaved body for recovery, but clear the title so + // reopening the composer cannot repeat the create with one click. + setNewIssueTitle('') + setNewIssueDraft({ title: '' }) + } else { + setNewIssueTitle('') + setNewIssueBody('') + setNewIssueLabels([]) + setNewIssueAssignees([]) + // Why: only a complete success discards the recovery draft; a partial + // body save keeps the original text available without another create. + clearNewIssueDraft() + } // Why: bump the nonce so the list refetches and shows the new issue. setTaskRefreshNonce((current) => current + 1) @@ -6973,7 +6989,8 @@ export default function TaskPage(): React.JSX.Element { newIssueTitle, openGitHubDetailPage, setDialogWorkItem, - clearNewIssueDraft + clearNewIssueDraft, + setNewIssueDraft ]) const handleCreateNewLinearProject = useCallback(async (): Promise => { diff --git a/src/renderer/src/components/task-page-github-issue-creation.test.ts b/src/renderer/src/components/task-page-github-issue-creation.test.ts new file mode 100644 index 000000000..58335c3ce --- /dev/null +++ b/src/renderer/src/components/task-page-github-issue-creation.test.ts @@ -0,0 +1,33 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it } from 'vitest' + +const taskPageSource = readFileSync(new URL('./TaskPage.tsx', import.meta.url), 'utf8') + +function issueCreationSection(): string { + const start = taskPageSource.indexOf('const handleCreateNewIssue') + const end = taskPageSource.indexOf('const handleCreateNewLinearProject', start) + return taskPageSource.slice(start, end) +} + +describe('TaskPage GitHub issue creation', () => { + it('covers the complete remote oversized-body recovery timeout envelope', () => { + const section = issueCreationSection() + + expect(section).toContain("'github.createIssue'") + expect(section).toContain('{ timeoutMs: 65_000 }') + }) + + it('treats a body-save warning as created while preserving the recovery draft', () => { + const section = issueCreationSection() + const warningBranch = section.slice( + section.indexOf('if (result.bodySaveWarning)'), + section.indexOf('// Why: bump the nonce') + ) + + expect(warningBranch).toContain('toast.warning') + expect(warningBranch).toContain('description: result.bodySaveWarning') + expect(warningBranch).toContain("setNewIssueDraft({ title: '' })") + expect(warningBranch).toContain('} else {') + expect(warningBranch).toContain('clearNewIssueDraft()') + }) +}) diff --git a/src/shared/types.ts b/src/shared/types.ts index 6079fa4fe..24308e932 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1743,6 +1743,10 @@ export type GitHubCreateIssueFields = { assignees?: string[] } +export type GitHubCreateIssueResult = + | { ok: true; number: number; url: string; bodySaveWarning?: string } + | { ok: false; error: string } + export type GitHubIssueCloseReason = 'completed' | 'not_planned' | 'duplicate' export type GitHubIssueUpdate = {