diff --git a/src/core/github-sync.test.ts b/src/core/github-sync.test.ts index a8eab98..ab1554a 100644 --- a/src/core/github-sync.test.ts +++ b/src/core/github-sync.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import nock from "nock"; import { GitHubSyncService, getGitHubToken, @@ -532,18 +533,22 @@ describe("GitHubSyncService", () => { }); const store = createStore([task]); + // Single-task sync uses getIssue (not listIssues) since task already has issueNumber // Issue is already CLOSED on GitHub (was closed on another machine) - // The cache will report state: "closed" - githubMock.listIssues("test-owner", "test-repo", [ + githubMock.getIssue( + "test-owner", + "test-repo", + 42, createIssueFixture({ number: 42, title: task.name, state: "closed", body: ``, }), - ]); + ); // Update should keep the issue closed (not reopen it) + // The key assertion: update is called but doesn't include state: "open" githubMock.updateIssue( "test-owner", "test-repo", @@ -563,6 +568,115 @@ describe("GitHubSyncService", () => { // but the issue stays closed on GitHub (we don't reopen it) expect(getGitHubMetadata(result)?.state).toBe("open"); }); + + it("does not reopen closed issue when syncing incomplete task without cache", async () => { + // Critical test for the fix: when syncTask is called directly (not through syncAll), + // there's no issue cache, so getIssueChangeResult must fetch the current state. + // If the remote issue is closed, we must not reopen it by sending state: "open". + const task = createTask({ + id: "incomplete-task", + name: "Incomplete Task", + completed: false, // Task is NOT completed locally + metadata: { + github: { + issueNumber: 99, + issueUrl: "https://github.com/test-owner/test-repo/issues/99", + repo: "test-owner/test-repo", + state: "open", // Stale local metadata + }, + }, + }); + const store = createStore([task]); + + // The GitHub issue is already CLOSED (closed externally or by another machine) + githubMock.getIssue( + "test-owner", + "test-repo", + 99, + createIssueFixture({ + number: 99, + title: task.name, + state: "closed", // CLOSED on remote + body: ``, + }), + ); + + // The update should NOT include state: "open" (would reopen the issue) + // Since the local content differs from remote, an update is needed + // but the state field should be omitted to preserve the closed state + githubMock.updateIssue( + "test-owner", + "test-repo", + 99, + createIssueFixture({ + number: 99, + title: task.name, + state: "closed", // Should stay closed + }), + ); + + const result = await service.syncTask(task, store); + + expect(result).not.toBeNull(); + // Expected state is "open" (task not completed), but issue should stay closed on GitHub + expect(getGitHubMetadata(result)?.state).toBe("open"); + }); + + it("verifies request body does not contain state:open when issue is closed", async () => { + // This test uses nock body matching to PROVE we don't send state: "open" + const task = createTask({ + id: "body-check-task", + name: "Body Check Task", + completed: false, + metadata: { + github: { + issueNumber: 88, + issueUrl: "https://github.com/test-owner/test-repo/issues/88", + repo: "test-owner/test-repo", + state: "open", + }, + }, + }); + const store = createStore([task]); + + // Setup getIssue to return closed issue + githubMock.getIssue( + "test-owner", + "test-repo", + 88, + createIssueFixture({ + number: 88, + title: task.name, + state: "closed", + body: ``, + }), + ); + + // Use nock directly with body matching to verify state is NOT "open" + let capturedBody: Record | null = null; + nock("https://api.github.com") + .patch(`/repos/test-owner/test-repo/issues/88`, (body) => { + capturedBody = body as Record; + // Accept any body - we'll verify after + return true; + }) + .reply(200, { + number: 88, + title: task.name, + state: "closed", + html_url: "https://github.com/test-owner/test-repo/issues/88", + }); + + await service.syncTask(task, store); + + // THE CRITICAL ASSERTION: state should NOT be "open" + expect(capturedBody).not.toBeNull(); + expect(capturedBody!.state).not.toBe("open"); + // state should either be undefined (not sent) or "closed" + expect( + capturedBody!.state === undefined || capturedBody!.state === "closed", + ).toBe(true); + }); }); }); diff --git a/src/core/github/sync.ts b/src/core/github/sync.ts index dce18a6..d98ddd7 100644 --- a/src/core/github/sync.ts +++ b/src/core/github/sync.ts @@ -306,8 +306,10 @@ export class GitHubSyncService { } // Get cached data for change detection and current state + // IMPORTANT: When state is unknown (no cache), use undefined to preserve remote state + // This prevents reopening closed issues when syncing a single task without cache const cached = issueCache?.get(parent.id); - const currentState = cached?.state ?? "open"; + let currentState: "open" | "closed" | undefined = cached?.state; // Check if remote is newer than local (staleness detection) // If so, pull remote state to local instead of pushing @@ -369,21 +371,27 @@ export class GitHubSyncService { const expectedBody = this.renderBody(parent, descendants); const expectedLabels = this.buildLabels(parent, shouldClose); - const hasChanges = cached - ? this.hasIssueChangedFromCache( - cached, - parent.name, - expectedBody, - expectedLabels, - shouldClose, - ) - : await this.hasIssueChanged( - issueNumber, - parent.name, - expectedBody, - expectedLabels, - shouldClose, - ); + let hasChanges: boolean; + if (cached) { + hasChanges = this.hasIssueChangedFromCache( + cached, + parent.name, + expectedBody, + expectedLabels, + shouldClose, + ); + } else { + // No cache - need to fetch from API to get current state + const changeResult = await this.getIssueChangeResult( + issueNumber, + parent.name, + expectedBody, + expectedLabels, + shouldClose, + ); + hasChanges = changeResult.hasChanges; + currentState = changeResult.currentState; + } if (!hasChanges) { onProgress?.({ @@ -400,6 +408,10 @@ export class GitHubSyncService { true, ); } + } else if (!cached) { + // skipUnchanged is false and no cache - still need to fetch current state + // to avoid accidentally reopening closed issues + currentState = await this.fetchIssueState(issueNumber); } onProgress?.({ @@ -453,16 +465,20 @@ export class GitHubSyncService { } /** - * Check if an issue has changed compared to what we would push. - * Returns true if the issue needs updating. + * Check if an issue has changed and get its current state. + * Returns both change detection result and current state for safe updates. + * When we can't fetch the issue, currentState is undefined to preserve remote state. */ - private async hasIssueChanged( + private async getIssueChangeResult( issueNumber: number, expectedTitle: string, expectedBody: string, expectedLabels: string[], shouldClose: boolean, - ): Promise { + ): Promise<{ + hasChanges: boolean; + currentState: "open" | "closed" | undefined; + }> { try { const { data: issue } = await this.octokit.issues.get({ owner: this.owner, @@ -474,7 +490,7 @@ export class GitHubSyncService { .map((l) => (typeof l === "string" ? l : l.name || "")) .filter((l) => l.startsWith(this.labelPrefix)); - return this.issueNeedsUpdate( + const hasChanges = this.issueNeedsUpdate( { title: issue.title, body: issue.body || "", @@ -486,9 +502,34 @@ export class GitHubSyncService { expectedLabels, shouldClose, ); + + return { + hasChanges, + currentState: issue.state as "open" | "closed", + }; } catch { // If we can't fetch the issue, assume it needs updating - return true; + // but use undefined state to preserve whatever the remote state is + return { hasChanges: true, currentState: undefined }; + } + } + + /** + * Fetch only the state of an issue (for when we need to avoid reopening). + * Returns undefined if the issue can't be fetched. + */ + private async fetchIssueState( + issueNumber: number, + ): Promise<"open" | "closed" | undefined> { + try { + const { data: issue } = await this.octokit.issues.get({ + owner: this.owner, + repo: this.repo, + issue_number: issueNumber, + }); + return issue.state as "open" | "closed"; + } catch { + return undefined; } } @@ -570,6 +611,7 @@ export class GitHubSyncService { * Update an existing GitHub issue. * * @param currentState - The current state of the issue on GitHub. + * undefined means we don't know the current state. * Used to prevent reopening closed issues. */ private async updateIssue( @@ -577,7 +619,7 @@ export class GitHubSyncService { descendants: HierarchicalTask[], issueNumber: number, shouldClose: boolean, - currentState: "open" | "closed", + currentState: "open" | "closed" | undefined, ): Promise { const body = this.renderBody(parent, descendants); @@ -585,15 +627,16 @@ export class GitHubSyncService { // - If shouldClose is true, always close (even if already closed) // - If shouldClose is false and currently open, keep open // - If shouldClose is false and currently closed, DON'T reopen - // (this prevents reopening issues that were closed elsewhere) + // - If shouldClose is false and state is unknown (undefined), don't set state + // (this preserves whatever the remote state is, preventing accidental reopening) let state: "open" | "closed" | undefined; if (shouldClose) { state = "closed"; } else if (currentState === "open") { state = "open"; } - // If currentState is "closed" and shouldClose is false, don't set state - // (keeps the issue closed, doesn't reopen it) + // If currentState is "closed" or undefined and shouldClose is false, don't set state + // (keeps the issue in its current state, doesn't reopen it) await this.octokit.issues.update({ owner: this.owner, diff --git a/src/core/shortcut-sync.test.ts b/src/core/shortcut-sync.test.ts new file mode 100644 index 0000000..0c933c8 --- /dev/null +++ b/src/core/shortcut-sync.test.ts @@ -0,0 +1,235 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { ShortcutSyncService } from "./shortcut/sync.js"; +import type { TaskStore, ShortcutMetadata } from "../types.js"; +import type { SyncResult } from "./sync/registry.js"; +import type { ShortcutMock } from "../test-utils/shortcut-mock.js"; +import { + setupShortcutMock, + cleanupShortcutMock, + createStoryFixture, + createWorkflowFixture, + createTeamFixture, +} from "../test-utils/shortcut-mock.js"; +import { createTask, createStore } from "../test-utils/github-mock.js"; + +/** + * Cast SyncResult metadata to ShortcutMetadata for test assertions. + */ +function getShortcutMetadata( + result: SyncResult | null | undefined, +): ShortcutMetadata | undefined { + return result?.metadata as ShortcutMetadata | undefined; +} + +// Mock git remote detection and git operations +vi.mock("./git-utils.js", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + isCommitOnRemote: vi.fn(() => false), // Default: commits not on remote + }; +}); + +describe("ShortcutSyncService", () => { + let service: ShortcutSyncService; + let shortcutMock: ShortcutMock; + + const defaultWorkflow = createWorkflowFixture({ id: 500000000 }); + const defaultTeam = createTeamFixture(); + + beforeEach(() => { + shortcutMock = setupShortcutMock(); + + service = new ShortcutSyncService({ + token: "test-token", + workspace: "test-workspace", + team: "test-team", + }); + }); + + afterEach(() => { + cleanupShortcutMock(); + vi.restoreAllMocks(); + }); + + describe("syncTask - preventing backwards state movement", () => { + it("does not move done story backwards when syncing incomplete task without cache", async () => { + // Critical test for the fix: when syncTask is called directly (not through syncAll), + // there's no story cache, so getStoryChangeResult must fetch the current workflow state. + // If the remote story is in "done" state, we must not move it backwards. + const task = createTask({ + id: "incomplete-task", + name: "Incomplete Task", + completed: false, // Task is NOT completed locally + metadata: { + shortcut: { + storyId: 99, + storyUrl: "https://app.shortcut.com/test-workspace/story/99", + workspace: "test-workspace", + state: "unstarted", // Stale local metadata + }, + }, + }); + const store = createStore([task]); + + // Setup: team lookup and workflow + shortcutMock.listGroups([defaultTeam]); + shortcutMock.getGroup(defaultTeam.id, defaultTeam); + shortcutMock.getWorkflow(defaultWorkflow.id, defaultWorkflow); + + // The Shortcut story is already in "done" state (completed externally) + shortcutMock.getStory( + 99, + createStoryFixture({ + id: 99, + name: task.name, + description: ``, + completed: true, // DONE on remote + workflow_state_id: 500000003, // Done state ID + labels: [{ name: "dex" }], + }), + ); + + // The update should NOT include a workflow_state_id that moves backwards + // (the story should stay in done state) + shortcutMock.updateStory( + 99, + createStoryFixture({ + id: 99, + name: task.name, + completed: true, // Should stay done + workflow_state_id: 500000003, // Should stay in done state + labels: [{ name: "dex" }], + }), + ); + + const result = await service.syncTask(task, store); + + expect(result).not.toBeNull(); + // Expected state is "unstarted" (task not started), but story should stay in done on Shortcut + expect(getShortcutMetadata(result)?.state).toBe("unstarted"); + }); + + it("moves story to done when task is completed with verified commit", async () => { + // Import and mock isCommitOnRemote + const gitUtils = await import("./git-utils.js"); + vi.mocked(gitUtils.isCommitOnRemote).mockReturnValue(true); + + const task = createTask({ + id: "completed-task", + name: "Completed Task", + completed: true, + metadata: { + shortcut: { + storyId: 100, + storyUrl: "https://app.shortcut.com/test-workspace/story/100", + workspace: "test-workspace", + state: "started", + }, + commit: { + sha: "abc123", + message: "Complete task", + }, + }, + }); + const store = createStore([task]); + + // Setup: team lookup and workflow + shortcutMock.listGroups([defaultTeam]); + shortcutMock.getGroup(defaultTeam.id, defaultTeam); + shortcutMock.getWorkflow(defaultWorkflow.id, defaultWorkflow); + + // Story is currently in started state + shortcutMock.getStory( + 100, + createStoryFixture({ + id: 100, + name: task.name, + description: ``, + completed: false, + workflow_state_id: 500000002, // Started state + labels: [{ name: "dex" }], + }), + ); + + // Should update to done state + shortcutMock.updateStory( + 100, + createStoryFixture({ + id: 100, + name: task.name, + completed: true, + workflow_state_id: 500000003, // Done state + labels: [{ name: "dex" }], + }), + ); + + const result = await service.syncTask(task, store); + + expect(result).not.toBeNull(); + expect(getShortcutMetadata(result)?.state).toBe("done"); + }); + + it("keeps story in started state when task completed without verified commit", async () => { + // Reset mock to ensure commit is NOT on remote + const gitUtils = await import("./git-utils.js"); + vi.mocked(gitUtils.isCommitOnRemote).mockReturnValue(false); + + // Task is completed but commit is not on remote - keep story in started state + const task = createTask({ + id: "unverified-task", + name: "Unverified Task", + completed: true, + started_at: new Date().toISOString(), + metadata: { + shortcut: { + storyId: 101, + storyUrl: "https://app.shortcut.com/test-workspace/story/101", + workspace: "test-workspace", + state: "started", + }, + commit: { + sha: "unpushed123", + message: "Complete task", + }, + }, + }); + const store = createStore([task]); + + // Setup: team lookup and workflow + shortcutMock.listGroups([defaultTeam]); + shortcutMock.getGroup(defaultTeam.id, defaultTeam); + shortcutMock.getWorkflow(defaultWorkflow.id, defaultWorkflow); + + // Story is in started state + shortcutMock.getStory( + 101, + createStoryFixture({ + id: 101, + name: task.name, + description: ``, + completed: false, + workflow_state_id: 500000002, // Started state + labels: [{ name: "dex" }], + }), + ); + + // Should update but stay in started state (commit not verified) + shortcutMock.updateStory( + 101, + createStoryFixture({ + id: 101, + name: task.name, + completed: false, + workflow_state_id: 500000002, // Still started + labels: [{ name: "dex" }], + }), + ); + + const result = await service.syncTask(task, store); + + expect(result).not.toBeNull(); + expect(getShortcutMetadata(result)?.state).toBe("started"); + }); + }); +}); diff --git a/src/core/shortcut/sync.ts b/src/core/shortcut/sync.ts index 6b7f64b..8c1c6da 100644 --- a/src/core/shortcut/sync.ts +++ b/src/core/shortcut/sync.ts @@ -29,6 +29,8 @@ export interface CachedStory { description: string; completed: boolean; labels: string[]; + /** Workflow state ID - used to prevent moving done stories backwards */ + workflow_state_id: number; } export interface ShortcutSyncServiceOptions { @@ -407,30 +409,46 @@ export class ShortcutSyncService { ); } + // Get cached data for change detection and current workflow state + // IMPORTANT: When state is unknown (no cache), use undefined to preserve remote state + // This prevents moving done stories backwards when syncing a single task without cache + const cached = storyCache?.get(parent.id); + let currentWorkflowStateId: number | undefined = + cached?.workflow_state_id; + // Check if we can skip this update by comparing with Shortcut let parentSkipped = false; if (skipUnchanged) { const expectedDescription = renderStoryDescription(parent); // Use cached data for change detection when available - const cached = storyCache?.get(parent.id); - const hasChanges = cached - ? this.hasStoryChangedFromCache( - cached, - parent.name, - expectedDescription, - shouldComplete, - ) - : await this.hasStoryChanged( - storyId, - parent.name, - expectedDescription, - shouldComplete, - ); + let hasChanges: boolean; + if (cached) { + hasChanges = this.hasStoryChangedFromCache( + cached, + parent.name, + expectedDescription, + shouldComplete, + ); + } else { + // No cache - need to fetch from API to get current state + const changeResult = await this.getStoryChangeResult( + storyId, + parent.name, + expectedDescription, + shouldComplete, + ); + hasChanges = changeResult.hasChanges; + currentWorkflowStateId = changeResult.currentWorkflowStateId; + } if (!hasChanges) { parentSkipped = true; } + } else if (!cached) { + // skipUnchanged is false and no cache - still need to fetch current state + // to avoid accidentally moving done stories backwards + currentWorkflowStateId = await this.fetchStoryWorkflowStateId(storyId); } if (parentSkipped) { @@ -448,7 +466,13 @@ export class ShortcutSyncService { phase: "updating", }); - await this.updateStory(parent, storyId, workflowId, store); + await this.updateStory( + parent, + storyId, + workflowId, + store, + currentWorkflowStateId, + ); } // Sync subtasks as Shortcut Sub-tasks @@ -527,11 +551,9 @@ export class ShortcutSyncService { for (const subtask of subtasks) { // Check for existing story: first metadata, then cache, then API fallback let subtaskStoryId = getShortcutStoryId(subtask); - if (!subtaskStoryId && storyCache) { - const cached = storyCache.get(subtask.id); - if (cached) { - subtaskStoryId = cached.id; - } + const cached = storyCache?.get(subtask.id); + if (!subtaskStoryId && cached) { + subtaskStoryId = cached.id; } if (!subtaskStoryId) { // Fallback search by task ID in description @@ -543,8 +565,22 @@ export class ShortcutSyncService { const shouldComplete = this.shouldMarkCompleted(subtask, store); if (subtaskStoryId) { + // Get current workflow state ID to prevent moving done stories backwards + let currentWorkflowStateId: number | undefined = + cached?.workflow_state_id; + if (currentWorkflowStateId === undefined) { + // No cache - fetch current state to avoid moving done stories backwards + currentWorkflowStateId = + await this.fetchStoryWorkflowStateId(subtaskStoryId); + } // Update existing subtask - await this.updateStory(subtask, subtaskStoryId, workflowId, store); + await this.updateStory( + subtask, + subtaskStoryId, + workflowId, + store, + currentWorkflowStateId, + ); } else { // Create new subtask with same team as parent const description = renderStoryDescription(subtask); @@ -654,18 +690,44 @@ export class ShortcutSyncService { } /** - * Check if a story has changed compared to what we would push. - * Returns true if the story needs updating. + * Check if a story has changed using cached data. + */ + private hasStoryChangedFromCache( + cached: CachedStory, + expectedName: string, + expectedDescription: string, + shouldBeCompleted: boolean, + ): boolean { + return this.storyNeedsUpdate( + { + name: cached.name, + description: cached.description, + completed: cached.completed, + labels: cached.labels, + }, + expectedName, + expectedDescription, + shouldBeCompleted, + ); + } + + /** + * Check if a story has changed and get its current workflow state. + * Returns both change detection result and current state for safe updates. + * When we can't fetch the story, currentWorkflowStateId is undefined to preserve remote state. */ - private async hasStoryChanged( + private async getStoryChangeResult( storyId: number, expectedName: string, expectedDescription: string, shouldBeCompleted: boolean, - ): Promise { + ): Promise<{ + hasChanges: boolean; + currentWorkflowStateId: number | undefined; + }> { try { const story = await this.api.getStory(storyId); - return this.storyNeedsUpdate( + const hasChanges = this.storyNeedsUpdate( { name: story.name, description: story.description, @@ -676,32 +738,31 @@ export class ShortcutSyncService { expectedDescription, shouldBeCompleted, ); + + return { + hasChanges, + currentWorkflowStateId: story.workflow_state_id, + }; } catch { // If we can't fetch the story, assume it needs updating - return true; + // but use undefined state to preserve whatever the remote state is + return { hasChanges: true, currentWorkflowStateId: undefined }; } } /** - * Check if a story has changed using cached data. + * Fetch only the workflow state ID of a story (for when we need to avoid moving backwards). + * Returns undefined if the story can't be fetched. */ - private hasStoryChangedFromCache( - cached: CachedStory, - expectedName: string, - expectedDescription: string, - shouldBeCompleted: boolean, - ): boolean { - return this.storyNeedsUpdate( - { - name: cached.name, - description: cached.description, - completed: cached.completed, - labels: cached.labels, - }, - expectedName, - expectedDescription, - shouldBeCompleted, - ); + private async fetchStoryWorkflowStateId( + storyId: number, + ): Promise { + try { + const story = await this.api.getStory(storyId); + return story.workflow_state_id; + } catch { + return undefined; + } } /** @@ -766,25 +827,53 @@ export class ShortcutSyncService { /** * Update an existing Shortcut story. + * + * @param currentWorkflowStateId - The current workflow state ID of the story. + * undefined means we don't know the current state. + * Used to prevent moving done stories backwards. */ private async updateStory( task: Task, storyId: number, workflowId: number, store?: TaskStore, + currentWorkflowStateId?: number, ): Promise { const description = renderStoryDescription(task); const shouldComplete = this.shouldMarkCompleted(task, store); - const stateId = await this.getWorkflowStateId( + const targetStateId = await this.getWorkflowStateId( task, workflowId, shouldComplete, ); + // Determine if we should update the workflow state + // - If shouldComplete is true (moving to done), always set the state + // - If currentWorkflowStateId is unknown (undefined), only update if moving to done + // (this prevents accidentally moving done stories backwards) + // - If currentWorkflowStateId is known, only update if not moving backwards + let workflowStateId: number | undefined = targetStateId; + if (!shouldComplete && currentWorkflowStateId !== undefined) { + const currentType = await this.getWorkflowStateType( + workflowId, + currentWorkflowStateId, + ); + if (currentType === "done") { + // Don't move a done story backwards + workflowStateId = undefined; + } + } else if (!shouldComplete && currentWorkflowStateId === undefined) { + // Unknown current state and not moving to done - skip workflow state update + // to preserve remote state (prevents moving done stories backwards) + workflowStateId = undefined; + } + await this.api.updateStory(storyId, { name: task.name, description, - workflow_state_id: stateId, + ...(workflowStateId !== undefined && { + workflow_state_id: workflowStateId, + }), labels: [{ name: this.label }], }); } @@ -867,6 +956,7 @@ export class ShortcutSyncService { description, completed: story.completed, labels: story.labels.map((l) => l.name), + workflow_state_id: story.workflow_state_id, }); } }