diff --git a/.changeset/fn-6007-fix-duplicate-github-issues.md b/.changeset/fn-6007-fix-duplicate-github-issues.md new file mode 100644 index 0000000000..f19e22d754 --- /dev/null +++ b/.changeset/fn-6007-fix-duplicate-github-issues.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix duplicate GitHub tracking issues and harden GitHub issue import deduping. diff --git a/packages/cli/src/__tests__/extension.test.ts b/packages/cli/src/__tests__/extension.test.ts index 36261b1058..c9f5be7f9f 100644 --- a/packages/cli/src/__tests__/extension.test.ts +++ b/packages/cli/src/__tests__/extension.test.ts @@ -2234,6 +2234,74 @@ describe.skipIf(!SHOULD_RUN_LEGACY_EXTENSION_INTEGRATION)("fn pi extension (lega }); }); + it("fn_task_import_github skips issues already imported via sourceIssue even when description was edited", async () => { + const store = new TaskStore(tmpDir); + await store.init(); + await store.createTask({ + title: "Existing imported issue", + description: "Edited description without source URL", + sourceIssue: { + provider: "github", + repository: "acme/demo", + externalIssueId: "1", + issueNumber: 1, + url: "https://github.com/acme/demo/issues/1", + }, + }); + store.close(); + + const tool = api.tools.get("fn_task_import_github")!; + vi.mocked(runGhJsonAsync).mockResolvedValueOnce([ + { + number: 1, + title: "Issue one", + body: "First issue body", + html_url: "https://github.com/acme/demo/issues/1", + }, + ] as never); + + const result = await tool.execute("gh-2b", { ownerRepo: "acme/demo" }, undefined, undefined, makeCtx(tmpDir)); + + expect(result.content[0].text).toContain("Imported 0 tasks from acme/demo"); + expect(result.details.createdTasks).toHaveLength(0); + }); + + it("fn_task_import_github_issue skips issues already imported via sourceIssue even when description was edited", async () => { + const store = new TaskStore(tmpDir); + await store.init(); + const existing = await store.createTask({ + title: "Existing imported issue", + description: "Edited description without source URL", + sourceIssue: { + provider: "github", + repository: "acme/demo", + externalIssueId: "1", + issueNumber: 1, + url: "https://github.com/acme/demo/issues/1", + }, + }); + store.close(); + + const tool = api.tools.get("fn_task_import_github_issue")!; + vi.mocked(runGhJsonAsync).mockResolvedValueOnce({ + number: 1, + title: "Issue one", + body: "First issue body", + html_url: "https://github.com/acme/demo/issues/1", + } as never); + + const result = await tool.execute( + "gh-2c", + { owner: "acme", repo: "demo", issueNumber: 1 }, + undefined, + undefined, + makeCtx(tmpDir), + ); + + expect(result.details).toMatchObject({ skipped: true, existingTaskId: existing.id }); + expect(result.content[0].text).toContain(existing.id); + }); + it("fn_task_browse_github_issues lists issues via gh api", async () => { const tool = api.tools.get("fn_task_browse_github_issues")!; vi.mocked(runGhJsonAsync).mockResolvedValueOnce([ diff --git a/packages/cli/src/extension.ts b/packages/cli/src/extension.ts index ebc77b7eeb..9e9ab2874d 100644 --- a/packages/cli/src/extension.ts +++ b/packages/cli/src/extension.ts @@ -442,6 +442,20 @@ function buildGitHubIssueSource(owner: string, repo: string, issue: { number: nu }; } +function isIssueAlreadyImported( + task: Pick, + owner: string, + repo: string, + issueNumber: number, + sourceUrl: string, +): boolean { + const sourceIssue = task.sourceIssue; + return task.description.includes(sourceUrl) + || (sourceIssue?.provider === "github" + && sourceIssue.repository === `${owner}/${repo}` + && sourceIssue.issueNumber === issueNumber); +} + async function fetchGitHubIssueViaGh( owner: string, repo: string, @@ -1274,12 +1288,12 @@ export default function kbExtension(pi: ExtensionAPI) { } const store = await getStore(ctx.cwd); - const existingTasks = await store.listTasks({ slim: true }); + const existingTasks = await store.listTasks({ slim: false }); const createdTasks: Array<{ id: string; title: string }> = []; for (const issue of issues) { const sourceUrl = issue.html_url; - const alreadyImported = existingTasks.some((task) => task.description.includes(sourceUrl)); + const alreadyImported = existingTasks.some((task) => isIssueAlreadyImported(task, owner, repo, issue.number, sourceUrl)); if (alreadyImported) { continue; } @@ -1304,7 +1318,7 @@ export default function kbExtension(pi: ExtensionAPI) { await store.logEntry(task.id, "Imported from GitHub", sourceUrl); createdTasks.push({ id: task.id, title: task.title || issue.title }); - existingTasks.push({ ...task, description }); + existingTasks.push(task); } const summary = `✓ Imported ${createdTasks.length} tasks from ${owner}/${repo}`; @@ -1358,11 +1372,11 @@ export default function kbExtension(pi: ExtensionAPI) { // Check if already imported const store = await getStore(ctx.cwd); - const existingTasks = await store.listTasks({ slim: true }); + const existingTasks = await store.listTasks({ slim: false }); const sourceUrl = issue.html_url; for (const task of existingTasks) { - if (task.description.includes(sourceUrl)) { + if (isIssueAlreadyImported(task, owner, repo, issueNumber, sourceUrl)) { return { content: [ { diff --git a/packages/dashboard/src/__tests__/github-tracking-hook.test.ts b/packages/dashboard/src/__tests__/github-tracking-hook.test.ts index 4a92134273..fb2eee5759 100644 --- a/packages/dashboard/src/__tests__/github-tracking-hook.test.ts +++ b/packages/dashboard/src/__tests__/github-tracking-hook.test.ts @@ -470,6 +470,74 @@ describe("registerGithubTrackingHook", () => { expect(mockCreateIssue).toHaveBeenCalledTimes(2); }); + it("creates exactly one tracking issue when duplicating a tracked task", async () => { + registerGithubTrackingHook(); + + await store.updateSettings({ + githubTrackingEnabledByDefault: true, + githubTrackingDefaultRepo: "owner/repo", + githubAuthMode: "token", + githubAuthToken: "tok", + }); + + const sourceTask = await store.createTask({ + title: "Tracked source task", + description: "source task for duplication", + githubTracking: { enabled: true }, + }); + + await vi.waitFor(() => { + expect(mockCreateIssue).toHaveBeenCalledTimes(1); + }); + mockCreateIssue.mockClear(); + + const duplicatedTask = await store.duplicateTask(sourceTask.id); + + expect(duplicatedTask.id).not.toBe(sourceTask.id); + expect(mockCreateIssue).toHaveBeenCalledTimes(1); + expect(mockCreateIssue).toHaveBeenCalledWith( + expect.objectContaining({ + owner: "owner", + repo: "repo", + title: expect.stringContaining(duplicatedTask.id), + }), + ); + }); + + it("creates exactly one tracking issue when refining a tracked task", async () => { + registerGithubTrackingHook(); + + await store.updateSettings({ + githubTrackingDefaultRepo: "owner/repo", + githubAuthMode: "token", + githubAuthToken: "tok", + }); + + const sourceTask = await store.createTask({ + title: "Tracked refinement source", + description: "source task for refinement", + column: "done", + githubTracking: { enabled: true }, + }); + + await vi.waitFor(() => { + expect(mockCreateIssue).toHaveBeenCalledTimes(1); + }); + mockCreateIssue.mockClear(); + + const refinedTask = await store.refineTask(sourceTask.id, "Follow-up work needed"); + + expect(refinedTask.id).not.toBe(sourceTask.id); + expect(mockCreateIssue).toHaveBeenCalledTimes(1); + expect(mockCreateIssue).toHaveBeenCalledWith( + expect.objectContaining({ + owner: "owner", + repo: "repo", + title: expect.stringContaining(refinedTask.id), + }), + ); + }); + it("creates issue during createTask await when summarization is disabled", async () => { registerGithubTrackingHook(); diff --git a/packages/dashboard/src/__tests__/routes-github.test.ts b/packages/dashboard/src/__tests__/routes-github.test.ts index 6454c98168..9ad6ad5189 100644 --- a/packages/dashboard/src/__tests__/routes-github.test.ts +++ b/packages/dashboard/src/__tests__/routes-github.test.ts @@ -701,6 +701,33 @@ describe("POST /github/issues/import", () => { expect(store.createTask).not.toHaveBeenCalled(); }); + it("returns 409 when sourceIssue matches even if description URL was edited away", async () => { + (store.listTasks as ReturnType).mockResolvedValueOnce([ + { + id: "FN-002", + description: "Edited description without source URL", + column: "triage", + sourceIssue: { + provider: "github", + repository: "owner/repo", + externalIssueId: "1", + issueNumber: 1, + url: "https://github.com/owner/repo/issues/1", + }, + }, + ]); + + getIssueSpy.mockResolvedValueOnce(mockGitHubIssue); + + const res = await REQUEST(buildApp(), "POST", "/api/github/issues/import", JSON.stringify({ owner: "owner", repo: "repo", issueNumber: 1 }), { + "Content-Type": "application/json", + }); + + expect(res.status).toBe(409); + expect(res.body.details?.existingTaskId).toBe("FN-002"); + expect(store.createTask).not.toHaveBeenCalled(); + }); + it("truncates long titles to 200 chars", async () => { const longTitleIssue = { ...mockGitHubIssue, @@ -888,6 +915,52 @@ describe("POST /github/issues/batch-import", () => { expect(res2.body.results[0].taskId).toBe(createdTaskId); }); + it("skips batch issues whose sourceIssue already matches even if description URL was edited away", async () => { + (store.listTasks as ReturnType).mockResolvedValue([ + { + id: "FN-200", + description: "Edited description without source URL", + column: "triage", + sourceIssue: { + provider: "github", + repository: "owner/repo", + externalIssueId: "1", + issueNumber: 1, + url: "https://github.com/owner/repo/issues/1", + }, + }, + ]); + + const throttledSpy = vi.spyOn(GitHubClient.prototype, "fetchThrottled") + .mockResolvedValueOnce({ + success: true, + data: mockGitHubIssue(1, "Already Imported Issue"), + } as Awaited>) + .mockResolvedValueOnce({ + success: true, + data: mockGitHubIssue(2, "Fresh Issue"), + } as Awaited>); + + const res = await REQUEST( + buildApp(), + "POST", + "/api/github/issues/batch-import", + JSON.stringify({ owner: "owner", repo: "repo", issueNumbers: [1, 2], delayMs: 1 }), + { "Content-Type": "application/json" } + ); + + expect(res.status).toBe(200); + expect(res.body.results).toEqual([ + { issueNumber: 1, success: true, skipped: true, taskId: "FN-200" }, + { issueNumber: 2, success: true, taskId: expect.any(String) }, + ]); + expect(store.createTask).toHaveBeenCalledTimes(1); + expect(store.createTask).toHaveBeenCalledWith(expect.objectContaining({ + sourceIssue: expect.objectContaining({ issueNumber: 2 }), + })); + expect(throttledSpy).toHaveBeenCalledTimes(2); + }); + it("returns 400 for empty issueNumbers array", async () => { const res = await REQUEST( buildApp(), diff --git a/packages/dashboard/src/__tests__/routes-tasks-ops.test.ts b/packages/dashboard/src/__tests__/routes-tasks-ops.test.ts index 4cc88b5f46..525d873ae6 100644 --- a/packages/dashboard/src/__tests__/routes-tasks-ops.test.ts +++ b/packages/dashboard/src/__tests__/routes-tasks-ops.test.ts @@ -836,20 +836,7 @@ describe("POST /tasks/:id/duplicate", () => { return app; } - it("duplicates a task, returns 201, and attempts tracking issue creation", async () => { - const createIssueSpy = vi.spyOn(GitHubClient.prototype, "createIssue").mockResolvedValue({ - owner: "task", - repo: "repo", - number: 91, - htmlUrl: "https://github.com/task/repo/issues/91", - createdAt: "2026-01-01T00:00:00.000Z", - }); - (store.getSettings as ReturnType).mockResolvedValue({ - githubTrackingDefaultRepo: "task/repo", - githubAuthMode: "token", - githubAuthToken: "tok", - }); - + it("duplicates a task and returns 201", async () => { const newTask = { ...FAKE_TASK_DETAIL, id: "FN-002", @@ -865,35 +852,6 @@ describe("POST /tasks/:id/duplicate", () => { expect(res.status).toBe(201); expect(res.body.id).toBe("FN-002"); expect(store.duplicateTask).toHaveBeenCalledWith("KB-001"); - expect(createIssueSpy).toHaveBeenCalledWith(expect.objectContaining({ owner: "task", repo: "repo" })); - expect(store.linkGithubIssue).toHaveBeenCalledWith("FN-002", expect.objectContaining({ owner: "task", repo: "repo", number: 91 })); - createIssueSpy.mockRestore(); - }); - - it("duplicate remains successful when tracking issue creation fails", async () => { - const createIssueSpy = vi.spyOn(GitHubClient.prototype, "createIssue").mockRejectedValue(new Error("boom")); - (store.getSettings as ReturnType).mockResolvedValue({ - githubTrackingDefaultRepo: "task/repo", - githubAuthMode: "token", - githubAuthToken: "tok", - }); - - const newTask = { - ...FAKE_TASK_DETAIL, - id: "FN-002", - column: "triage", - githubTracking: { enabled: true, repoOverride: "task/repo" }, - }; - (store.duplicateTask as ReturnType).mockResolvedValue(newTask); - - const res = await REQUEST(buildApp(), "POST", "/api/tasks/KB-001/duplicate", JSON.stringify({}), { - "Content-Type": "application/json", - }); - - expect(res.status).toBe(201); - expect(res.body.id).toBe("FN-002"); - expect(createIssueSpy).toHaveBeenCalledTimes(1); - createIssueSpy.mockRestore(); }); it("returns 404 when source task not found", async () => { @@ -938,20 +896,7 @@ describe("POST /tasks/:id/refine", () => { return app; } - it("creates refinement task from done task, returns 201, and attempts tracking issue creation", async () => { - const createIssueSpy = vi.spyOn(GitHubClient.prototype, "createIssue").mockResolvedValue({ - owner: "task", - repo: "repo", - number: 92, - htmlUrl: "https://github.com/task/repo/issues/92", - createdAt: "2026-01-01T00:00:00.000Z", - }); - (store.getSettings as ReturnType).mockResolvedValue({ - githubTrackingDefaultRepo: "task/repo", - githubAuthMode: "token", - githubAuthToken: "tok", - }); - + it("creates refinement task from done task and returns 201", async () => { const refinedTask = { ...FAKE_TASK_DETAIL, id: "FN-002", @@ -970,37 +915,6 @@ describe("POST /tasks/:id/refine", () => { expect(res.body.id).toBe("FN-002"); expect(store.refineTask).toHaveBeenCalledWith("KB-001", "Need improvements"); expect(store.logEntry).toHaveBeenCalledWith("KB-001", "Refinement requested", "Need improvements"); - expect(createIssueSpy).toHaveBeenCalledWith(expect.objectContaining({ owner: "task", repo: "repo" })); - expect(store.linkGithubIssue).toHaveBeenCalledWith("FN-002", expect.objectContaining({ owner: "task", repo: "repo", number: 92 })); - createIssueSpy.mockRestore(); - }); - - it("refine remains successful when tracking issue creation fails", async () => { - const createIssueSpy = vi.spyOn(GitHubClient.prototype, "createIssue").mockRejectedValue(new Error("boom")); - (store.getSettings as ReturnType).mockResolvedValue({ - githubTrackingDefaultRepo: "task/repo", - githubAuthMode: "token", - githubAuthToken: "tok", - }); - - const refinedTask = { - ...FAKE_TASK_DETAIL, - id: "FN-002", - column: "triage", - title: "Refinement: KB-001", - githubTracking: { enabled: true, repoOverride: "task/repo" }, - }; - (store.refineTask as ReturnType).mockResolvedValue(refinedTask); - (store.logEntry as ReturnType).mockResolvedValue(FAKE_TASK_DETAIL); - - const res = await REQUEST(buildApp(), "POST", "/api/tasks/KB-001/refine", JSON.stringify({ feedback: "Need improvements" }), { - "Content-Type": "application/json", - }); - - expect(res.status).toBe(201); - expect(res.body.id).toBe("FN-002"); - expect(createIssueSpy).toHaveBeenCalledTimes(1); - createIssueSpy.mockRestore(); }); it("creates refinement task from in-review task and returns 201", async () => { diff --git a/packages/dashboard/src/routes/register-git-github.ts b/packages/dashboard/src/routes/register-git-github.ts index d542a6081b..b638ea3a1a 100644 --- a/packages/dashboard/src/routes/register-git-github.ts +++ b/packages/dashboard/src/routes/register-git-github.ts @@ -2115,6 +2115,20 @@ function buildGitHubIssueSource(owner: string, repo: string, issue: { number: nu }; } +function isIssueAlreadyImported( + task: Pick, + owner: string, + repo: string, + issueNumber: number, + sourceUrl: string, +): boolean { + const sourceIssue = task.sourceIssue; + return task.description.includes(sourceUrl) + || (sourceIssue?.provider === "github" + && sourceIssue.repository === `${owner}/${repo}` + && sourceIssue.issueNumber === issueNumber); +} + export function getDefaultGitHubRepo(store: TaskStore): { owner: string; repo: string } | null { const envRepo = process.env.GITHUB_REPOSITORY; if (envRepo) { @@ -3868,10 +3882,10 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { } // Check if already imported - const existingTasks = await scopedStore.listTasks({ slim: true, includeArchived: false }); + const existingTasks = await scopedStore.listTasks({ slim: false, includeArchived: false }); const sourceUrl = issue.html_url; for (const existingTask of existingTasks) { - if (existingTask.description.includes(sourceUrl)) { + if (isIssueAlreadyImported(existingTask, owner, repo, issueNumber, sourceUrl)) { throw new ApiError(409, `Issue #${issueNumber} already imported as ${existingTask.id}`, { existingTaskId: existingTask.id, }); @@ -3953,7 +3967,7 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { const { store: scopedStore } = await getProjectContext(req); // Get existing tasks to check for duplicates - const existingTasks = await scopedStore.listTasks({ slim: true, includeArchived: false }); + const existingTasks = await scopedStore.listTasks({ slim: false, includeArchived: false }); // Process issues sequentially with throttling const results: Array<{ @@ -4001,7 +4015,7 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { // Check if already imported const sourceUrl = issue.html_url; - const existingTask = existingTasks.find((t) => t.description.includes(sourceUrl)); + const existingTask = existingTasks.find((t) => isIssueAlreadyImported(t, owner, repo, issueNumber, sourceUrl)); if (existingTask) { results.push({ issueNumber, @@ -4041,7 +4055,7 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { }); // Add to existingTasks to avoid duplicate imports within the same batch - existingTasks.push({ ...task, description }); + existingTasks.push(task); } catch (err: unknown) { if (err instanceof ApiError) { throw err; diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index 4f1a1aadf1..1f8324bebc 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -1736,15 +1736,6 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork try { const { store: scopedStore } = await getProjectContext(req); const newTask = await scopedStore.duplicateTask(req.params.id); - // Fire github tracking explicitly so duplicates created through the - // route (which may not pass through TaskStore.createTask's hook - // invocation in mocked test setups) still produce a tracking issue - // when the source task had tracking enabled. Best-effort. - try { - await createTrackingIssueForTask(scopedStore, newTask, { githubToken: options?.githubToken }); - } catch { - // never block duplicate response - } res.status(201).json(newTask); } catch (err: unknown) { if (err instanceof ApiError) { @@ -1772,13 +1763,6 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork const refinedTask = await scopedStore.refineTask(req.params.id, trimmedFeedback); await scopedStore.logEntry(req.params.id, "Refinement requested", trimmedFeedback); - // Fire github tracking explicitly so refinements get a tracking issue - // when the source task had tracking enabled. Best-effort. - try { - await createTrackingIssueForTask(scopedStore, refinedTask, { githubToken: options?.githubToken }); - } catch { - // never block refine response - } res.status(201).json(refinedTask); } catch (err: unknown) { if (err instanceof ApiError) {