diff --git a/apps/client/src/main/github/githubIssueSync.test.ts b/apps/client/src/main/github/githubIssueSync.test.ts index 58b35790..ef485942 100644 --- a/apps/client/src/main/github/githubIssueSync.test.ts +++ b/apps/client/src/main/github/githubIssueSync.test.ts @@ -194,15 +194,14 @@ describe('reconcileGitHubIssues', () => { const jira = { ...card({ id: 'j' }), source: 'jira', externalSource: 'jira' } as Task; const adhoc = { ...card({ id: 'a' }), source: 'adhoc', externalSource: null } as Task; const result = reconcileGitHubIssues([jira, adhoc], [], opts); - expect(result).toMatchObject({ upserts: [], removals: [], restoreIds: [] }); + expect(result).toMatchObject({ upserts: [], removals: [] }); }); - it('brings an archived card back when its issue returns to the query', () => { + it('leaves an archived card alone even when its issue returns to the query', () => { const gone = card({ archivedAt: 5, archivedReason: 'left-query' }); const result = reconcileGitHubIssues([gone], [issue(1)], opts); - expect(result.restoreIds).toEqual(['gh-acme-web-1']); - // The same ROW comes back — the id is the card's, not a new one. - expect(result.upserts[0].id).toBe('gh-acme-web-1'); + expect(result.upserts).toEqual([]); + expect(result.removals).toEqual([]); }); it('archives a card GitHub was asked about and still has, but the query dropped', () => { diff --git a/apps/client/src/main/github/githubIssueSync.ts b/apps/client/src/main/github/githubIssueSync.ts index bd1f8dca..b356a001 100644 --- a/apps/client/src/main/github/githubIssueSync.ts +++ b/apps/client/src/main/github/githubIssueSync.ts @@ -121,8 +121,6 @@ export interface GitHubIssueSyncResult { upserts: Task[]; /** Cards to take off the board — every one confirmed by a question GitHub answered. */ removals: ForgeRemoval[]; - /** Ids of archived cards whose issue is back in the query — put them back on the board. */ - restoreIds: string[]; /** Removals `guardRemovals` would not let through. Nothing was done to these. */ refused: ForgeRemoval[]; /** What the human should be told about this sync, or null when there is nothing to say. */ @@ -404,8 +402,11 @@ export function parseIssueKey(key: string): IssueRef | null { * * 1. **blocked** — untouched. A blocked card is never removed, so no answer could change * the outcome and there is no question worth asking. - * 2. **archived** — already off the board. Back in the query ⇒ it returns (`restoreIds`); - * still absent ⇒ nothing to say. + * 2. **archived** — already off the board, and sync leaves it there whether or not the + * query returns its issue again. A removed card stays off the board until an explicit + * restore; nothing here brings it back. (If that restore is of an issue the search + * genuinely no longer matches, the next sync re-archives it through the ordinary + * `left-query` path below — correct given the query, and not this function's problem.) * 3. **the search was truncated** — everything is kept, whatever else is true of it. * 4. **the re-read did not run, or this issue's own call failed** — kept. A card must not * be archived on a question that errored. @@ -428,18 +429,19 @@ export function reconcileGitHubIssues( ): GitHubIssueSyncResult { const existingByKey = githubTasksByKey(existing); const seen = new Set(); - const restoreIds: string[] = []; - const upserts = issues.map((issue, i) => { + const upserts: Task[] = []; + for (let i = 0; i < issues.length; i++) { + const issue = issues[i]; const { owner, repo } = repoRefFrom(issue.repository_url); const key = issueKeyFor(owner, repo, issue.number); seen.add(key); const prior = existingByKey.get(key); - // Archived, and the query returns it again: the issue matches, so the card comes back to - // the board — the same row, with the timeline, files and links it left with. - if (prior?.archivedAt != null) restoreIds.push(prior.id); - return issueToTask(issue, prior, opts, i); - }); + // Archived cards stay off the board even when the query returns their issue again — the + // sync must never resurrect a removed card; only an explicit restore does. + if (prior?.archivedAt != null) continue; + upserts.push(issueToTask(issue, prior, opts, i)); + } const rechecked = opts.rechecked ?? null; const recheckedKeys = asSet(opts.recheckedKeys); @@ -534,7 +536,6 @@ export function reconcileGitHubIssues( return { upserts, removals: guarded.removals, - restoreIds, refused: guarded.refused, warning: notes.length ? notes.join(' ') : null, }; diff --git a/apps/client/src/main/ipc.ts b/apps/client/src/main/ipc.ts index 2e7996e8..72d61e65 100644 --- a/apps/client/src/main/ipc.ts +++ b/apps/client/src/main/ipc.ts @@ -3268,30 +3268,23 @@ export function registerIpcHandlers(mainWindow: BrowserWindow): Engine { // Recorded before the reconcile rather than after: the applies below cannot throw a // network error, so this is the last point at which "the query we just ran" is true. store.saveJiraLastQuery(jql); - const { upserts, removals, restoreIds, refused, warning } = reconcileJiraTasks( - personalForSync, - issues, - { - baseUrl: jira.baseUrl, - overrides: jira.statusCategoryOverrides, - learned: jira.learnedStatusColumns, - epicFieldId: epicField, - epicNames, - sprintFieldId: sprintField, - identity, - rechecked, - recheckedKeys, - queryChecked: confirmed?.checked ?? null, - queryMatches: confirmed?.matching ?? null, - truncated, - queryChanged, - now, - retentionMs: Math.max(0, jira.doneRetentionDays) * 24 * 60 * 60 * 1000, - }, - ); - // Restore first: a ticket that has come back into the query lands on its own card again, - // rather than beside the archived one it used to be. - for (const id of restoreIds) store.unarchiveTask(id); + const { upserts, removals, refused, warning } = reconcileJiraTasks(personalForSync, issues, { + baseUrl: jira.baseUrl, + overrides: jira.statusCategoryOverrides, + learned: jira.learnedStatusColumns, + epicFieldId: epicField, + epicNames, + sprintFieldId: sprintField, + identity, + rechecked, + recheckedKeys, + queryChecked: confirmed?.checked ?? null, + queryMatches: confirmed?.matching ?? null, + truncated, + queryChanged, + now, + retentionMs: Math.max(0, jira.doneRetentionDays) * 24 * 60 * 60 * 1000, + }); for (const t of upserts) store.upsertJiraTask(t); // ARCHIVED, not deleted. A card leaving the board is not the human deleting it — the row // keeps its timeline, its files and its links, and "Removed cards" can put it back. Each @@ -3467,23 +3460,18 @@ export function registerIpcHandlers(mainWindow: BrowserWindow): Engine { // Recorded before the reconcile rather than after: nothing below can throw a network // error, so this is the last point at which "the query we just ran" is true. store.saveGitHubLastQuery(query); - const { upserts, removals, restoreIds, refused, warning } = reconcileGitHubIssues( - personalForSync, - items, - { - overrides: github.labelColumnOverrides, - learned: github.learnedLabelColumns, - identity, - comments, - rechecked, - recheckedKeys, - truncated, - queryChanged, - now, - retentionMs: Math.max(0, github.doneRetentionDays) * 24 * 60 * 60 * 1000, - }, - ); - for (const id of restoreIds) store.unarchiveTask(id); + const { upserts, removals, refused, warning } = reconcileGitHubIssues(personalForSync, items, { + overrides: github.labelColumnOverrides, + learned: github.learnedLabelColumns, + identity, + comments, + rechecked, + recheckedKeys, + truncated, + queryChanged, + now, + retentionMs: Math.max(0, github.doneRetentionDays) * 24 * 60 * 60 * 1000, + }); for (const t of upserts) store.upsertJiraTask(t); // ARCHIVED, not deleted — see the same loop in `syncJira`. The row keeps its timeline, // its files and its links, and "Removed cards" can put it back. diff --git a/apps/client/src/main/jira/jiraSync.integration.test.ts b/apps/client/src/main/jira/jiraSync.integration.test.ts index 493ebf14..619a1946 100644 --- a/apps/client/src/main/jira/jiraSync.integration.test.ts +++ b/apps/client/src/main/jira/jiraSync.integration.test.ts @@ -301,7 +301,6 @@ describe('a 300-card board survives a sync', () => { expect(run.upserts.map((t) => t.id)).toEqual(board.map((t) => t.id)); expect(run.upserts.every((t) => t.externalUrl?.startsWith('https://jira.company.com/browse/'))); - expect(run.restoreIds).toEqual([]); }); it('costs nothing extra: a whole answer asks no by-key questions at all', async () => { @@ -536,7 +535,7 @@ describe('a truncated re-read takes nothing off the board', () => { // --------------------------------------------------------------------------- -describe('a sprint switch narrows the board, and the way back restores it', () => { +describe('a sprint switch narrows the board, and archived cards stay off it', () => { const board = range(1, BOARD_SIZE).map((n) => cardFor(n)); /** The new sprint carries 60 of the 300 over; the other 240 are last sprint's. */ const carriedOver = range(1, 60).map((n) => issueFor(n)); @@ -579,7 +578,7 @@ describe('a sprint switch narrows the board, and the way back restores it', () = expect(run.warning).toContain('more than 25% of the board in one sync'); }); - it('the way back: the same rows return, same ids, nothing re-created', async () => { + it('the sprint rolling back does not resurrect the 240 that left it', async () => { // The board a sync later, with the 240 archived — which is what `getPersonalTasksForSync` // hands the reconciler, and the reason it is the ONE read that includes archived cards. const afterSwitch = [ @@ -596,15 +595,12 @@ describe('a sprint switch narrows the board, and the way back restores it', () = reconcile: { queryChanged: true }, }); - expect(run.restoreIds).toEqual(range(61, BOARD_SIZE).map((n) => `card-${key(n)}`)); expect(run.removals).toEqual([]); expect(run.refused).toEqual([]); - // The whole point of archiving rather than deleting: the ticket lands back on the row it - // left on. A `jira-*` id in here would mean a brand-new card beside the archived one, and - // everything the old row carried stranded on a card nobody can see. - expect(run.upserts).toHaveLength(BOARD_SIZE); - expect(run.upserts.map((t) => t.id)).toEqual(afterSwitch.map((t) => t.id)); - expect(run.upserts.some((t) => t.id.startsWith('jira-'))).toBe(false); + // The 240 archived rows stay off the board — no upsert brings any of them back, whatever + // their issue does in JIRA. Only the 60 that were never archived are upserted. + expect(run.upserts).toHaveLength(60); + expect(run.upserts.map((t) => t.id)).toEqual(range(1, 60).map((n) => `card-${key(n)}`)); }); it('an archived card the query still does not return is left alone, not asked about again', async () => { @@ -624,7 +620,6 @@ describe('a sprint switch narrows the board, and the way back restores it', () = expect(run.retained).toEqual([]); expect(searcher.asked).toEqual([]); expect(run.removals).toEqual([]); - expect(run.restoreIds).toEqual([]); expect(run.warning).toBeNull(); }); }); diff --git a/apps/client/src/main/jira/jiraSync.test.ts b/apps/client/src/main/jira/jiraSync.test.ts index 90f94768..fe44e1c7 100644 --- a/apps/client/src/main/jira/jiraSync.test.ts +++ b/apps/client/src/main/jira/jiraSync.test.ts @@ -766,8 +766,8 @@ describe('reconcileJiraTasks — no card leaves without JIRA answering about it' }); }); -describe('reconcileJiraTasks — a card comes back', () => { - it('restores an archived card whose issue is in the query again, on the same row', () => { +describe('reconcileJiraTasks — an archived card stays archived', () => { + it('leaves an archived card alone even though its issue is in the query again', () => { const archived = jiraTask({ archivedAt: 1_700, projectTagId: 'p-billing', @@ -775,22 +775,28 @@ describe('reconcileJiraTasks — a card comes back', () => { lastReadCommentAt: 555, }); const result = reconcileJiraTasks([archived], [issue('1', 'PROJ-1', 'indeterminate')], opts); - expect(result.restoreIds).toEqual(['jira-1']); - expect(result.upserts).toHaveLength(1); - // The same row, with everything JIRA has never heard of still on it. - expect(result.upserts[0]).toMatchObject({ - id: 'jira-1', - projectTagId: 'p-billing', - agentProjectId: 'agent-1', - lastReadCommentAt: 555, - status: 'in-progress', - }); + expect(result.upserts).toEqual([]); + expect(result.removals).toEqual([]); }); it('says nothing about an archived card the query still does not return', () => { const archived = jiraTask({ archivedAt: 1_700 }); const result = reconcileJiraTasks([archived], [], { ...opts, ...asked('PROJ-1') }); - expect(result).toMatchObject({ upserts: [], removals: [], restoreIds: [], refused: [] }); + expect(result).toMatchObject({ upserts: [], removals: [], refused: [] }); + }); + + it('regression: a card archived left-query does not come back when the query finds it again', () => { + // The reported bug: sync one) removes a card that left the query, sync two) sees the + // ticket in a fresh page of the SAME query and must not resurrect the card for it. + const archived = jiraTask({ + id: 'jira-42', + externalKey: 'PROJ-42', + archivedAt: 2_000, + archivedReason: 'left-query', + }); + const result = reconcileJiraTasks([archived], [issue('42', 'PROJ-42', 'indeterminate')], opts); + expect(result.upserts).toEqual([]); + expect(result.removals).toEqual([]); }); }); diff --git a/apps/client/src/main/jira/jiraSync.ts b/apps/client/src/main/jira/jiraSync.ts index da04242a..0e5aff21 100644 --- a/apps/client/src/main/jira/jiraSync.ts +++ b/apps/client/src/main/jira/jiraSync.ts @@ -173,8 +173,6 @@ export interface JiraSyncResult { * leaving the board is not the human deleting it. See `store.archiveTask`. */ removals: JiraRemoval[]; - /** Ids of archived cards whose issue is back in the query — put them back on the board. */ - restoreIds: string[]; /** Removals `guardRemovals` would not let through. Nothing was done to these. */ refused: JiraRemoval[]; /** What the human should be told about this sync, or null when there is nothing to say. */ @@ -436,8 +434,11 @@ export function removalCandidateKeys( * about it any more (since `isBlockedishStatus`, a Blocked ticket is exactly what puts * a card here), but because the answer could not change the outcome: a blocked card is * never removed, so there is no question worth asking. - * 2. **archived** — already off the board. Back in the query ⇒ it returns (`restoreIds`); - * still absent ⇒ nothing to say. + * 2. **archived** — already off the board, and sync leaves it there whether or not the + * query returns its issue again. A removed card stays off the board until an explicit + * restore; nothing here brings it back. (If that restore is of a ticket the JQL + * genuinely no longer matches, the next sync re-archives it through the ordinary + * `left-query` path below — correct given the query, and not this function's problem.) * 3. **the search was truncated** — everything is kept, whatever else is true of it. * 4. **never asked about** (`queryChecked` has no such key) — kept. * 5. **asked, and JIRA says it still matches** (`queryMatches`) — kept, and counted: this @@ -459,16 +460,17 @@ export function reconcileJiraTasks( ): JiraSyncResult { const existingByKey = jiraTasksByKey(existing); const seen = new Set(); - const restoreIds: string[] = []; - const upserts = issues.map((issue, i) => { + const upserts: Task[] = []; + for (let i = 0; i < issues.length; i++) { + const issue = issues[i]; seen.add(issue.key); const prior = existingByKey.get(issue.key); - // Archived, and the query returns it again: the ticket matches, so the card comes back - // to the board — the same row, with the timeline, files and links it left with. - if (prior?.archivedAt != null) restoreIds.push(prior.id); - return issueToTask(issue, prior, opts, i); - }); + // Archived cards stay off the board even when the query returns their issue again — the + // sync must never resurrect a removed card; only an explicit restore does. + if (prior?.archivedAt != null) continue; + upserts.push(issueToTask(issue, prior, opts, i)); + } const rechecked = opts.rechecked ? new Map(opts.rechecked.map((i) => [i.key, i])) : null; const recheckedKeys = asSet(opts.recheckedKeys); @@ -567,7 +569,6 @@ export function reconcileJiraTasks( return { upserts, removals: guarded.removals, - restoreIds, refused: guarded.refused, warning: notes.length ? notes.join(' ') : null, };