Skip to content

Commit fc32880

Browse files
dcramerclaude
andcommitted
fix(sync): Prevent reopening closed issues/stories during single-task sync
When syncTask is called directly (via `dex sync <task-id>` or `dex export`), there's no cached remote state. Previously this caused: - GitHub: currentState defaulted to "open", reopening closed issues - Shortcut: workflow_state_id was always set, moving done stories backwards Changes: - GitHub: Track currentState as undefined when unknown, fetch via getIssueChangeResult() or fetchIssueState() when no cache - GitHub: updateIssue() omits state field when unknown, preserving remote - Shortcut: Add workflow_state_id to CachedStory for state tracking - Shortcut: Track currentWorkflowStateId, fetch when no cache - Shortcut: updateStory() omits workflow_state_id when moving backwards - Remove dead hasIssueChanged() and hasStoryChanged() methods - Add tests with nock body verification to prove fix works Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 1724b8b commit fc32880

File tree

4 files changed

+558
-76
lines changed

4 files changed

+558
-76
lines changed

src/core/github-sync.test.ts

Lines changed: 117 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
2+
import nock from "nock";
23
import {
34
GitHubSyncService,
45
getGitHubToken,
@@ -532,18 +533,22 @@ describe("GitHubSyncService", () => {
532533
});
533534
const store = createStore([task]);
534535

536+
// Single-task sync uses getIssue (not listIssues) since task already has issueNumber
535537
// Issue is already CLOSED on GitHub (was closed on another machine)
536-
// The cache will report state: "closed"
537-
githubMock.listIssues("test-owner", "test-repo", [
538+
githubMock.getIssue(
539+
"test-owner",
540+
"test-repo",
541+
42,
538542
createIssueFixture({
539543
number: 42,
540544
title: task.name,
541545
state: "closed",
542546
body: `<!-- dex:task:id:test-task -->`,
543547
}),
544-
]);
548+
);
545549

546550
// Update should keep the issue closed (not reopen it)
551+
// The key assertion: update is called but doesn't include state: "open"
547552
githubMock.updateIssue(
548553
"test-owner",
549554
"test-repo",
@@ -563,6 +568,115 @@ describe("GitHubSyncService", () => {
563568
// but the issue stays closed on GitHub (we don't reopen it)
564569
expect(getGitHubMetadata(result)?.state).toBe("open");
565570
});
571+
572+
it("does not reopen closed issue when syncing incomplete task without cache", async () => {
573+
// Critical test for the fix: when syncTask is called directly (not through syncAll),
574+
// there's no issue cache, so getIssueChangeResult must fetch the current state.
575+
// If the remote issue is closed, we must not reopen it by sending state: "open".
576+
const task = createTask({
577+
id: "incomplete-task",
578+
name: "Incomplete Task",
579+
completed: false, // Task is NOT completed locally
580+
metadata: {
581+
github: {
582+
issueNumber: 99,
583+
issueUrl: "https://github.com/test-owner/test-repo/issues/99",
584+
repo: "test-owner/test-repo",
585+
state: "open", // Stale local metadata
586+
},
587+
},
588+
});
589+
const store = createStore([task]);
590+
591+
// The GitHub issue is already CLOSED (closed externally or by another machine)
592+
githubMock.getIssue(
593+
"test-owner",
594+
"test-repo",
595+
99,
596+
createIssueFixture({
597+
number: 99,
598+
title: task.name,
599+
state: "closed", // CLOSED on remote
600+
body: `<!-- dex:task:id:incomplete-task -->`,
601+
}),
602+
);
603+
604+
// The update should NOT include state: "open" (would reopen the issue)
605+
// Since the local content differs from remote, an update is needed
606+
// but the state field should be omitted to preserve the closed state
607+
githubMock.updateIssue(
608+
"test-owner",
609+
"test-repo",
610+
99,
611+
createIssueFixture({
612+
number: 99,
613+
title: task.name,
614+
state: "closed", // Should stay closed
615+
}),
616+
);
617+
618+
const result = await service.syncTask(task, store);
619+
620+
expect(result).not.toBeNull();
621+
// Expected state is "open" (task not completed), but issue should stay closed on GitHub
622+
expect(getGitHubMetadata(result)?.state).toBe("open");
623+
});
624+
625+
it("verifies request body does not contain state:open when issue is closed", async () => {
626+
// This test uses nock body matching to PROVE we don't send state: "open"
627+
const task = createTask({
628+
id: "body-check-task",
629+
name: "Body Check Task",
630+
completed: false,
631+
metadata: {
632+
github: {
633+
issueNumber: 88,
634+
issueUrl: "https://github.com/test-owner/test-repo/issues/88",
635+
repo: "test-owner/test-repo",
636+
state: "open",
637+
},
638+
},
639+
});
640+
const store = createStore([task]);
641+
642+
// Setup getIssue to return closed issue
643+
githubMock.getIssue(
644+
"test-owner",
645+
"test-repo",
646+
88,
647+
createIssueFixture({
648+
number: 88,
649+
title: task.name,
650+
state: "closed",
651+
body: `<!-- dex:task:id:body-check-task -->`,
652+
}),
653+
);
654+
655+
// Use nock directly with body matching to verify state is NOT "open"
656+
let capturedBody: Record<string, unknown> | null = null;
657+
nock("https://api.github.com")
658+
.patch(`/repos/test-owner/test-repo/issues/88`, (body) => {
659+
capturedBody = body as Record<string, unknown>;
660+
// Accept any body - we'll verify after
661+
return true;
662+
})
663+
.reply(200, {
664+
number: 88,
665+
title: task.name,
666+
state: "closed",
667+
html_url: "https://github.com/test-owner/test-repo/issues/88",
668+
});
669+
670+
await service.syncTask(task, store);
671+
672+
// THE CRITICAL ASSERTION: state should NOT be "open"
673+
expect(capturedBody).not.toBeNull();
674+
expect(capturedBody!.state).not.toBe("open");
675+
// state should either be undefined (not sent) or "closed"
676+
expect(
677+
capturedBody!.state === undefined || capturedBody!.state === "closed",
678+
).toBe(true);
679+
});
566680
});
567681
});
568682

src/core/github/sync.ts

Lines changed: 69 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,10 @@ export class GitHubSyncService {
306306
}
307307

308308
// Get cached data for change detection and current state
309+
// IMPORTANT: When state is unknown (no cache), use undefined to preserve remote state
310+
// This prevents reopening closed issues when syncing a single task without cache
309311
const cached = issueCache?.get(parent.id);
310-
const currentState = cached?.state ?? "open";
312+
let currentState: "open" | "closed" | undefined = cached?.state;
311313

312314
// Check if remote is newer than local (staleness detection)
313315
// If so, pull remote state to local instead of pushing
@@ -369,21 +371,27 @@ export class GitHubSyncService {
369371
const expectedBody = this.renderBody(parent, descendants);
370372
const expectedLabels = this.buildLabels(parent, shouldClose);
371373

372-
const hasChanges = cached
373-
? this.hasIssueChangedFromCache(
374-
cached,
375-
parent.name,
376-
expectedBody,
377-
expectedLabels,
378-
shouldClose,
379-
)
380-
: await this.hasIssueChanged(
381-
issueNumber,
382-
parent.name,
383-
expectedBody,
384-
expectedLabels,
385-
shouldClose,
386-
);
374+
let hasChanges: boolean;
375+
if (cached) {
376+
hasChanges = this.hasIssueChangedFromCache(
377+
cached,
378+
parent.name,
379+
expectedBody,
380+
expectedLabels,
381+
shouldClose,
382+
);
383+
} else {
384+
// No cache - need to fetch from API to get current state
385+
const changeResult = await this.getIssueChangeResult(
386+
issueNumber,
387+
parent.name,
388+
expectedBody,
389+
expectedLabels,
390+
shouldClose,
391+
);
392+
hasChanges = changeResult.hasChanges;
393+
currentState = changeResult.currentState;
394+
}
387395

388396
if (!hasChanges) {
389397
onProgress?.({
@@ -400,6 +408,10 @@ export class GitHubSyncService {
400408
true,
401409
);
402410
}
411+
} else if (!cached) {
412+
// skipUnchanged is false and no cache - still need to fetch current state
413+
// to avoid accidentally reopening closed issues
414+
currentState = await this.fetchIssueState(issueNumber);
403415
}
404416

405417
onProgress?.({
@@ -453,16 +465,20 @@ export class GitHubSyncService {
453465
}
454466

455467
/**
456-
* Check if an issue has changed compared to what we would push.
457-
* Returns true if the issue needs updating.
468+
* Check if an issue has changed and get its current state.
469+
* Returns both change detection result and current state for safe updates.
470+
* When we can't fetch the issue, currentState is undefined to preserve remote state.
458471
*/
459-
private async hasIssueChanged(
472+
private async getIssueChangeResult(
460473
issueNumber: number,
461474
expectedTitle: string,
462475
expectedBody: string,
463476
expectedLabels: string[],
464477
shouldClose: boolean,
465-
): Promise<boolean> {
478+
): Promise<{
479+
hasChanges: boolean;
480+
currentState: "open" | "closed" | undefined;
481+
}> {
466482
try {
467483
const { data: issue } = await this.octokit.issues.get({
468484
owner: this.owner,
@@ -474,7 +490,7 @@ export class GitHubSyncService {
474490
.map((l) => (typeof l === "string" ? l : l.name || ""))
475491
.filter((l) => l.startsWith(this.labelPrefix));
476492

477-
return this.issueNeedsUpdate(
493+
const hasChanges = this.issueNeedsUpdate(
478494
{
479495
title: issue.title,
480496
body: issue.body || "",
@@ -486,9 +502,34 @@ export class GitHubSyncService {
486502
expectedLabels,
487503
shouldClose,
488504
);
505+
506+
return {
507+
hasChanges,
508+
currentState: issue.state as "open" | "closed",
509+
};
489510
} catch {
490511
// If we can't fetch the issue, assume it needs updating
491-
return true;
512+
// but use undefined state to preserve whatever the remote state is
513+
return { hasChanges: true, currentState: undefined };
514+
}
515+
}
516+
517+
/**
518+
* Fetch only the state of an issue (for when we need to avoid reopening).
519+
* Returns undefined if the issue can't be fetched.
520+
*/
521+
private async fetchIssueState(
522+
issueNumber: number,
523+
): Promise<"open" | "closed" | undefined> {
524+
try {
525+
const { data: issue } = await this.octokit.issues.get({
526+
owner: this.owner,
527+
repo: this.repo,
528+
issue_number: issueNumber,
529+
});
530+
return issue.state as "open" | "closed";
531+
} catch {
532+
return undefined;
492533
}
493534
}
494535

@@ -570,30 +611,32 @@ export class GitHubSyncService {
570611
* Update an existing GitHub issue.
571612
*
572613
* @param currentState - The current state of the issue on GitHub.
614+
* undefined means we don't know the current state.
573615
* Used to prevent reopening closed issues.
574616
*/
575617
private async updateIssue(
576618
parent: Task,
577619
descendants: HierarchicalTask[],
578620
issueNumber: number,
579621
shouldClose: boolean,
580-
currentState: "open" | "closed",
622+
currentState: "open" | "closed" | undefined,
581623
): Promise<void> {
582624
const body = this.renderBody(parent, descendants);
583625

584626
// Determine the state to set:
585627
// - If shouldClose is true, always close (even if already closed)
586628
// - If shouldClose is false and currently open, keep open
587629
// - If shouldClose is false and currently closed, DON'T reopen
588-
// (this prevents reopening issues that were closed elsewhere)
630+
// - If shouldClose is false and state is unknown (undefined), don't set state
631+
// (this preserves whatever the remote state is, preventing accidental reopening)
589632
let state: "open" | "closed" | undefined;
590633
if (shouldClose) {
591634
state = "closed";
592635
} else if (currentState === "open") {
593636
state = "open";
594637
}
595-
// If currentState is "closed" and shouldClose is false, don't set state
596-
// (keeps the issue closed, doesn't reopen it)
638+
// If currentState is "closed" or undefined and shouldClose is false, don't set state
639+
// (keeps the issue in its current state, doesn't reopen it)
597640

598641
await this.octokit.issues.update({
599642
owner: this.owner,

0 commit comments

Comments
 (0)