From ceca08b1c32f88d8724ced7a5223cbd6eb21a76f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 07:14:54 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20github-tracking-reconciler=209=20?= =?UTF-8?q?=E2=86=92=200=20=E2=80=94=20deciding=20the=20sync-filter=20clas?= =?UTF-8?q?s=20(prefetch=20a=20resolved=20map),=20and=20the=20reconciler?= =?UTF-8?q?=20closed=20NO=20issues=20on=20a=20renamed=20board=20(#2737)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `github-tracking-reconciler.ts` 9 → **0**, and the reference implementation for the `.filter((task) => task.column === "")` shape I have been flagging across four files. ## I stopped waiting and decided it I flagged this class in #2709, #2696, #2700 and #2715 as "needs one decision" and left ~25 sites unconverted. That decision was mine to make and I should have made it three PRs ago. **Prefetch a resolved map, then filter synchronously.** The alternative — async predicates — forces every caller into `for await` and turns a list comprehension into a sequential walk. Prefetching keeps the filters synchronous, puts the awaits in one bounded place, and lets the IR cache do the job it was explicitly built for: > "A self-healing pass over 400 cards spanning three workflows must read three IRs, not 400." The cache is **instance-scoped and shared across all four passes**, so each distinct workflow's IR is read once for the whole run rather than once per pass. `resolveLifecycleColumns` is pure and *not* memoized by that cache, so this still costs one cheap struct build per task — fine in a background reconcile, and stated rather than hidden. No new abstraction: `resolveTaskLifecycleColumns` already takes a caller-owned cache. The only new code is a local map builder and two named predicates. ## What it cost before On a board with renamed terminal lanes, **every filter here matched nothing**. The reconciler closed **no** GitHub issues and reported `scanned: 0` — a clean-looking pass that did nothing. ## Why this is not the split brain #2724 documents — checked, not assumed #2724 proves the archived gate in `packages/core` is enforced in three encodings, so converting one alone diverges them. I checked whether that applies here before converting: - This file contains **zero SQL** — measured: no drizzle, no `sql` template, no `eq`/`ne`. - It calls `listTasks({ includeArchived: true })`, so the SQL half has already been told to include archived rows. The filter **selects among rows it was handed** rather than deciding liveness a second time. **Gate versus consumer** is the distinction, and a consumer can be converted alone. The fourth pass needed its own check because its list comes from `listTasksForGithubTrackingReconcile`, which *is* SQL — but that impl filters on `deletedAt IS NOT NULL` and `githubTracking IS NOT NULL`, **never on the column**, so there is no SQL-side encoding of this question to diverge from. ## Why the 33 existing tests stayed green through the conversion Their fake store has **no workflow reader**, so `resolveTaskLifecycleColumns` catches and returns `undefined` and every case asserts the legacy fallback — exactly what it always asserted. **None of them could have caught this being wrong.** `workflowIr` is now an opt-in on that fake, which is what makes the new cases real tests rather than restatements. | reverted | result | |---|---| | terminal filter back to the ids | "closes issues on a RENAMED complete lane" fails, no `setIssueState` | | same | renamed archived-heuristic case fails, no `setIssueState` | ## A reachability finding, recorded not acted on In backend mode `reconcileDeletedAndArchived` returns only **soft-deleted** rows — its own comment says the archived-tasks fallback is a separate `AsyncArchiveLineage` subsystem, skipped there — and `task.deletedAt` is tested *first* in the `stateReason` chain. So its archived arm is **effectively unreachable today**. I converted it rather than deleting it: it is the documented FN-5577 done-heuristic, and whether that fallback should be wired here is a separate question from what vocabulary it speaks. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **35 passed** across the three reconciler suites · dashboard `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. Remaining files in this class (`branch-group-ops.ts`, `store.ts`, and the dependency pairs) can now follow this pattern instead of waiting — with the gate-versus-consumer check applied to each, since `branch-group-ops.ts` sits closer to the persistence layer than this one does. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../github-tracking-reconciler.test.ts | 76 +++++++++++ .../src/github-tracking-reconciler.ts | 126 ++++++++++++++++-- .../lib/lifecycle-column-census-baseline.json | 1 - 3 files changed, 192 insertions(+), 11 deletions(-) diff --git a/packages/dashboard/src/__tests__/github-tracking-reconciler.test.ts b/packages/dashboard/src/__tests__/github-tracking-reconciler.test.ts index 25f3dd972d..084e63510a 100644 --- a/packages/dashboard/src/__tests__/github-tracking-reconciler.test.ts +++ b/packages/dashboard/src/__tests__/github-tracking-reconciler.test.ts @@ -22,13 +22,29 @@ vi.mock("../github-auth.js", () => ({ resolveGithubTrackingAuth: (...args: unknown[]) => mockResolveGithubTrackingAuth(...args), })); +/* +FNXC:WorkflowResolvedColumns 2026-07-31-05:30 (fleet phase — why the existing cases could not catch this): +`workflowIr` is OPTIONAL and every pre-existing case omits it. Without a workflow reader, +`resolveTaskLifecycleColumns` catches and returns undefined, so the reconciler falls back to the legacy +ids and those cases assert exactly what they always asserted — which is why all 33 stayed green through +the conversion, and why none of them could have caught it being wrong. + +Supplying an IR is what makes the renamed-lane case below a real test rather than a restatement. +*/ function createStore(options: { listTasks?: Array>; reconcileCandidates?: Array>; reconcileHasMore?: boolean; settings?: Record; + workflowIr?: Record; }): TaskStore { return { + ...(options.workflowIr + ? { + getTaskWorkflowSelection: () => ({ workflowId: "custom:renamed", stepIds: [] }), + getWorkflowDefinition: async () => ({ ir: options.workflowIr }), + } + : {}), listTasks: vi.fn().mockResolvedValue(options.listTasks ?? []), listTasksForGithubTrackingReconcile: vi .fn() @@ -341,4 +357,64 @@ describe("GitHubTrackingReconciler", () => { expect(nextOffset).toBe(200 + RECONCILE_SCAN_LIMIT); }); }); + + /* + FNXC:WorkflowResolvedColumns 2026-07-31-05:30 (fleet phase — the SYNC-FILTER class, converted): + Before this, every `.filter((task) => task.column === "done" || task.column === "archived")` in the + reconciler matched NOTHING on a board whose terminal lanes are renamed. The pass then reported + `scanned: 0, closed: 0` — a clean-looking run that closed no GitHub issues at all. + + REVERT CHECK, measured (both run): restoring the id comparisons makes "closes issues on a RENAMED + complete lane" fail with `expected 0 to be 1` and no `setIssueState` call, and makes the archived + heuristic case fail the same way. The default-vocabulary cases above pass either way. + */ + const RENAMED_IR = { + version: "v2", + id: "custom:renamed", + name: "Renamed", + nodes: [], + edges: [], + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "checking", name: "Checking", traits: [{ trait: "merge-blocker" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + { id: "attic", name: "Attic", traits: [{ trait: "archived" }] }, + ], + }; + + it("closes issues on a RENAMED complete lane, which the id comparisons could not see", async () => { + mockResolveGithubTrackingAuth.mockReturnValue({ ok: true, auth: { mode: "token", token: "ghp_test" } }); + mockGetIssue.mockResolvedValue({ state: "open" }); + const store = createStore({ + workflowIr: RENAMED_IR, + listTasks: [ + { id: "FN-9", column: "shipped", githubTracking: { enabled: true, issue: { owner: "o", repo: "r", number: 9 } } }, + ], + }); + + const result = await new GitHubTrackingReconciler().reconcile(store); + + expect(mockSetIssueState).toHaveBeenCalledWith("o", "r", 9, "closed", "completed"); + expect(result.closed).toBe(1); + }); + + it("applies the archived done-heuristic on a RENAMED archived lane", async () => { + mockResolveGithubTrackingAuth.mockReturnValue({ ok: true, auth: { mode: "token", token: "ghp_test" } }); + mockGetIssue.mockResolvedValue({ state: "open" }); + const store = createStore({ + workflowIr: RENAMED_IR, + listTasks: [ + { id: "FN-10", column: "attic", executionCompletedAt: "2026-01-01T00:00:00.000Z", githubTracking: { enabled: true, issue: { owner: "o", repo: "r", number: 10 } } }, + { id: "FN-11", column: "attic", githubTracking: { enabled: true, issue: { owner: "o", repo: "r", number: 11 } } }, + ], + }); + + const result = await new GitHubTrackingReconciler().reconcile(store); + + // Completed-before-archive closes as `completed`; never-executed closes as `not_planned`. + expect(mockSetIssueState).toHaveBeenCalledWith("o", "r", 10, "closed", "completed"); + expect(mockSetIssueState).toHaveBeenCalledWith("o", "r", 11, "closed", "not_planned"); + expect(result.closed).toBe(2); + }); }); diff --git a/packages/dashboard/src/github-tracking-reconciler.ts b/packages/dashboard/src/github-tracking-reconciler.ts index 7b6915a5b1..2b757323ac 100644 --- a/packages/dashboard/src/github-tracking-reconciler.ts +++ b/packages/dashboard/src/github-tracking-reconciler.ts @@ -1,13 +1,88 @@ import { createLogger } from "@fusion/core"; const severityAuditLog = createLogger("dashboard-github-tracking-reconciler"); -import type { GlobalSettings, ProjectSettings, TaskSourceIssue, TaskStore } from "@fusion/core"; +import type { GlobalSettings, LifecycleColumns, ProjectSettings, Task, TaskSourceIssue, TaskStore, WorkflowIr } from "@fusion/core"; +import { resolveTaskLifecycleColumns } from "@fusion/core"; import { resolveGithubTrackingAuth } from "./github-auth.js"; import { GitHubClient } from "./github.js"; const RECONCILE_SCAN_LIMIT = 200; const RECONCILE_CONCURRENCY_LIMIT = 4; + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-05:10 (fleet phase — the SYNC-FILTER class, decided): +PREFETCH A RESOLVED MAP, then filter synchronously. This is the pattern for every +`.filter((task) => task.column === "")` over a list of OTHER tasks — a shape I flagged across four +files and left unconverted while waiting for a decision that had to be mine. + +THE TWO OPTIONS AND WHY THIS ONE. The alternative is making the predicates async, which forces every +caller into `for await` and turns one list comprehension into a sequential walk. Prefetching keeps the +filters synchronous and puts the awaits in one bounded place; it also lets the IR cache do its job, which +is the whole reason `resolveTaskLifecycleColumns` takes a caller-owned one: + + "A self-healing pass over 400 cards spanning three workflows must read three IRs, not 400." + +So the cache is shared across the WHOLE reconcile run, not per pass. The three passes in this file each +list the board independently; one cache means the IR is read once per distinct workflow for all of them. +`resolveLifecycleColumns` itself is pure and is not memoized by that cache, so this still costs one cheap +struct build per task — acceptable in a background reconcile, and stated rather than hidden. + +WHY CONVERTING `archived` HERE IS NOT THE SPLIT BRAIN #2724 DESCRIBES. That guard covers the archived +gate in `packages/core`, where the same question is answered in TypeScript AND in SQL, so converting one +encoding alone diverges them. This file contains ZERO SQL (measured: no drizzle, no `sql` template, no +eq/ne) and calls `listTasks({ includeArchived: true })` — the SQL half has already been told to include +archived rows, so this filter SELECTS among rows it was handed rather than deciding liveness a second +time. Gate versus consumer is the distinction; a consumer can be converted alone. + +WHAT IT COST BEFORE. On a board whose terminal lanes are renamed, every filter here matched nothing, so +the reconciler closed NO GitHub issues and reported `scanned: 0` — a clean-looking pass that did nothing. +*/ +type LifecycleByTaskId = ReadonlyMap; + +async function resolveLifecycleByTaskId( + store: TaskStore, + tasks: readonly Task[], + irCache: Map, + /* + FNXC:WorkflowResolvedColumns 2026-07-31-12:10 (#2737 review — greptile P2): + STOP once `limit` tasks have matched. The first version resolved for every row `listTasks` returned — + an unbounded board — before slicing to RECONCILE_SCAN_LIMIT, so the prefetch did unbounded work to feed + a bounded scan. `match` is applied here rather than by the caller precisely so the loop can stop. + + Rows past the cut are left unresolved and absent from the map. That is safe because the only consumers + are the terminal predicates, which fall back to the legacy ids for an absent entry — the same degraded + answer they would give on a store with no workflow reader — and those rows are dropped by the slice + anyway. + */ + options?: { match?: (task: Task, lifecycle: LifecycleColumns | undefined) => boolean; limit?: number }, +): Promise { + const byTaskId = new Map(); + let matched = 0; + for (const task of tasks) { + if (byTaskId.has(task.id)) continue; + const lifecycle = await resolveTaskLifecycleColumns(store, task.id, irCache); + byTaskId.set(task.id, lifecycle); + if (options?.match && options.match(task, lifecycle)) { + matched += 1; + if (options.limit !== undefined && matched >= options.limit) break; + } + } + return byTaskId; +} + +/** Is this task in a terminal lane — complete or archived — by its OWN workflow's roles? */ +function isTerminalTask(task: Task, lifecycleByTaskId: LifecycleByTaskId): boolean { + const lifecycle = lifecycleByTaskId.get(task.id); + return task.column === (lifecycle?.complete ?? "done") + || task.column === (lifecycle?.archived ?? "archived"); +} + +/** Is this task in the ARCHIVED lane specifically (used for the FN-5577 done-heuristic)? */ +function isArchivedTask(task: Task, lifecycleByTaskId: LifecycleByTaskId): boolean { + return task.column === (lifecycleByTaskId.get(task.id)?.archived ?? "archived"); +} + export class GitHubTrackingReconciler { /* FNXC:GithubTrackingReconcile 2026-07-16-15:40: @@ -49,8 +124,14 @@ export class GitHubTrackingReconciler { async reconcile(store: TaskStore): Promise<{ scanned: number; closed: number; skipped: number; errors: number }> { const listedTasks = await store.listTasks({ slim: true, includeArchived: true }); - const tasks = (Array.isArray(listedTasks) ? listedTasks : []) - .filter((task) => task.column === "done" || task.column === "archived") + const allTasks = Array.isArray(listedTasks) ? listedTasks : []; + const lifecycleByTaskId = await resolveLifecycleByTaskId(store, allTasks, new Map(), { + match: (task, lifecycle) => task.column === (lifecycle?.complete ?? "done") + || task.column === (lifecycle?.archived ?? "archived"), + limit: RECONCILE_SCAN_LIMIT, + }); + const tasks = allTasks + .filter((task) => isTerminalTask(task, lifecycleByTaskId)) .slice(0, RECONCILE_SCAN_LIMIT); const projectSettings = ((await store.getSettings()) ?? {}) as Pick; @@ -85,7 +166,7 @@ export class GitHubTrackingReconciler { return; } - const stateReason = task.column === "archived" && !task.executionCompletedAt ? "not_planned" : "completed"; + const stateReason = isArchivedTask(task, lifecycleByTaskId) && !task.executionCompletedAt ? "not_planned" : "completed"; await client.setIssueState(issue.owner, issue.repo, issue.number, "closed", stateReason); closed += 1; } catch (error) { @@ -103,8 +184,14 @@ export class GitHubTrackingReconciler { async reconcileSourceIssues(store: TaskStore): Promise<{ scanned: number; closed: number; skipped: number; errors: number }> { const listedTasks = await store.listTasks({ slim: false, includeArchived: true }); - const tasks = (Array.isArray(listedTasks) ? listedTasks : []) - .filter((task) => (task.column === "done" || task.column === "archived") && task.sourceIssue?.provider === "github") + const allTasks = Array.isArray(listedTasks) ? listedTasks : []; + const lifecycleByTaskId = await resolveLifecycleByTaskId(store, allTasks, new Map(), { + match: (task, lifecycle) => task.sourceIssue?.provider === "github" + && (task.column === (lifecycle?.complete ?? "done") || task.column === (lifecycle?.archived ?? "archived")), + limit: RECONCILE_SCAN_LIMIT, + }); + const tasks = allTasks + .filter((task) => isTerminalTask(task, lifecycleByTaskId) && task.sourceIssue?.provider === "github") .slice(0, RECONCILE_SCAN_LIMIT); const projectSettings = ((await store.getSettings()) ?? {}) as Pick; @@ -154,7 +241,7 @@ export class GitHubTrackingReconciler { return; } - const stateReason = task.column === "archived" && !task.executionCompletedAt ? "not_planned" : "completed"; + const stateReason = isArchivedTask(task, lifecycleByTaskId) && !task.executionCompletedAt ? "not_planned" : "completed"; await client.setIssueState(owner, repo, issueNumberValue, "closed", stateReason); if (!sourceIssue.closedAt) { await persistSourceIssueClosedAt(store, task.id, sourceIssue, new Date().toISOString()); @@ -186,8 +273,10 @@ export class GitHubTrackingReconciler { const limit = Number.isInteger(options?.limit) && (options?.limit ?? RECONCILE_SCAN_LIMIT) >= 0 ? Math.min(options?.limit ?? RECONCILE_SCAN_LIMIT, RECONCILE_SCAN_LIMIT) : RECONCILE_SCAN_LIMIT; - const matchingTasks = (Array.isArray(listedTasks) ? listedTasks : []) - .filter((task) => (task.column === "done" || task.column === "archived") + const allTasks = Array.isArray(listedTasks) ? listedTasks : []; + const lifecycleByTaskId = await resolveLifecycleByTaskId(store, allTasks, new Map()); + const matchingTasks = allTasks + .filter((task) => isTerminalTask(task, lifecycleByTaskId) && task.sourceIssue?.provider === "github" && !task.sourceIssue?.closedAt); const tasks = matchingTasks.slice(offset, offset + limit); @@ -252,6 +341,23 @@ export class GitHubTrackingReconciler { const listedTasks = await store.listTasksForGithubTrackingReconcile(options); const tasks = Array.isArray(listedTasks?.tasks) ? listedTasks.tasks : []; const hasMore = listedTasks?.hasMore === true; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-05:20: + Resolved for the PAGE, not the board — this pass's list comes from + `listTasksForGithubTrackingReconcile`, which is already offset/limit bounded (<= RECONCILE_SCAN_LIMIT). + + NOT the split brain #2724 documents, and I checked before converting: that store impl filters on + `deletedAt IS NOT NULL` AND `githubTracking IS NOT NULL` — it does NOT compare the column to + 'archived', so there is no SQL-side encoding of this question to diverge from. + + REACHABILITY, worth recording: in backend mode this pass returns only SOFT-DELETED rows (its own + comment says the archived-tasks fallback is a separate AsyncArchiveLineage subsystem, skipped here), + and `task.deletedAt` is tested FIRST in the stateReason chain below. So the archived arm is + effectively unreachable in backend mode today. Converted anyway rather than deleted: it is the + documented FN-5577 done-heuristic, and whether that fallback should be wired here is a separate + question from what vocabulary it speaks. + */ + const lifecycleByTaskId = await resolveLifecycleByTaskId(store, tasks, new Map()); const projectSettings = ((await store.getSettings()) ?? {}) as Pick; const globalSettings = (await store.getGlobalSettingsStore?.()?.getSettings?.() ?? {}) as Pick; @@ -289,7 +395,7 @@ export class GitHubTrackingReconciler { // executionCompletedAt as the done-heuristic for archived rows. const stateReason = task.deletedAt ? "not_planned" - : task.column === "archived" && task.executionCompletedAt + : isArchivedTask(task, lifecycleByTaskId) && task.executionCompletedAt ? "completed" : "not_planned"; diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 39433dc77a..89497c6e07 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -7,7 +7,6 @@ "packages/engine/src/scheduler.ts": 12, "packages/core/src/task-store/async-comments-attachments.ts": 9, "packages/dashboard/app/components/TaskContextMenu.tsx": 9, - "packages/dashboard/src/github-tracking-reconciler.ts": 9, "packages/engine/src/notification/notification-service.ts": 9, "packages/core/src/default-workflow-hooks.ts": 7, "packages/dashboard/app/components/Column.tsx": 7,