diff --git a/packages/dashboard/src/__tests__/github-tracking-comments.test.ts b/packages/dashboard/src/__tests__/github-tracking-comments.test.ts index 5ad585bf4d..c6b80787c9 100644 --- a/packages/dashboard/src/__tests__/github-tracking-comments.test.ts +++ b/packages/dashboard/src/__tests__/github-tracking-comments.test.ts @@ -415,6 +415,68 @@ describe("GitHubTrackingCommentService", () => { expect(mockCommentOnIssue).toHaveBeenCalledTimes(1); }); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-00:40 (PR #2715 review — greptile): + A TRACKED TASK ON A RENAMED BOARD MUST STILL GET ITS COMMENT. + + The service resolved the wip/complete columns but tested `event.to` against the literal ids FIRST, + so on a renamed board it returned before reaching any resolved code and the comment was silently + skipped — no error, no log, just a tracked issue that stops being updated. + + MEASURED: restoring that literal early return leaves all 101 existing cases green. Nothing here + drove a workflow at all — `MockStore` has no selection methods, so every case resolved to the + legacy fallback and the conversion was untestable by construction. + */ + it("posts a comment when a tracked task moves to a RENAMED wip column", async () => { + const RENAMED_IR = { + version: "v2", + id: "wf-renamed", + name: "Renamed", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }], + edges: [], + }; + const widened = store as unknown as Record; + widened.getTaskWorkflowSelection = vi.fn(() => ({ workflowId: "wf-renamed" })); + widened.getTaskWorkflowSelectionAsync = vi.fn(async () => ({ workflowId: "wf-renamed" })); + widened.getWorkflowDefinition = vi.fn(async () => ({ id: "wf-renamed", ir: RENAMED_IR })); + + service.start(); + store.emit("task:moved", { task: createTask(), from: "backlog", to: "building" }); + await flushAsync(); + + expect(mockCommentOnIssue).toHaveBeenCalledTimes(1); + }); + + it("still ignores a column the RENAMED board does not use for tracking", async () => { + /* The negative half: resolving must not make every move post a comment. */ + const widened = store as unknown as Record; + widened.getTaskWorkflowSelection = vi.fn(() => ({ workflowId: "wf-renamed" })); + widened.getTaskWorkflowSelectionAsync = vi.fn(async () => ({ workflowId: "wf-renamed" })); + widened.getWorkflowDefinition = vi.fn(async () => ({ + id: "wf-renamed", + ir: { + version: "v2", id: "wf-renamed", name: "Renamed", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }], edges: [], + }, + })); + + service.start(); + store.emit("task:moved", { task: createTask(), from: "building", to: "backlog" }); + await flushAsync(); + + expect(mockCommentOnIssue).not.toHaveBeenCalled(); + }); + it("ignores non-target columns", async () => { service.start(); diff --git a/packages/dashboard/src/__tests__/gitlab-parity-inventory-documentation.test.ts b/packages/dashboard/src/__tests__/gitlab-parity-inventory-documentation.test.ts index da521b0d01..2152c8b3ca 100644 --- a/packages/dashboard/src/__tests__/gitlab-parity-inventory-documentation.test.ts +++ b/packages/dashboard/src/__tests__/gitlab-parity-inventory-documentation.test.ts @@ -68,3 +68,52 @@ describe("gitlab parity inventory documentation contract", () => { expect(readDoc("docs/signals-connectors.md")).toContain("[GitLab Parity Inventory](./gitlab-parity-inventory.md)"); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-09:45 (fleet phase — CODE parity, not just documentation parity): +The contract above checks that the parity INVENTORY DOC mentions each surface. It cannot notice that the +two tracking services, which implement one of those surfaces twice, have drifted in how they decide which +moves warrant a comment. + +That drift really happened in this program: `github-tracking-comments.ts` was converted to resolved +lifecycle roles while `gitlab-tracking-comments.ts` kept comparing `event.to` to `"in-progress"` and +`"done"`. The GitHub half worked on a renamed board and the GitLab half silently posted nothing — the +FN-6115 -> FN-6118 -> FN-6123 shape (one behaviour in two modules, one converted) arriving through the +provider-parity door instead of the desktop/mobile one. + +Comments are stripped before searching, so the FNXC notes at those sites — which necessarily quote the +literals they explain — do not satisfy or trip this check. +*/ +describe("github and gitlab tracking services resolve lanes the same way", () => { + const SERVICES = [ + "packages/dashboard/src/github-tracking-comments.ts", + "packages/dashboard/src/gitlab-tracking-comments.ts", + ]; + + it("both resolve the moved task's lifecycle columns", async () => { + const { stripComments } = await import("../../../../scripts/lib/lifecycle-column-census.mjs") as { + stripComments: (source: string) => string; + }; + for (const file of SERVICES) { + const code = stripComments(readDoc(file)); + expect(code, `${file} must resolve lifecycle columns rather than name lanes`) + .toContain("resolveTaskLifecycleColumns"); + } + }); + + it("neither compares a move target to a legacy lane id", async () => { + const { stripComments } = await import("../../../../scripts/lib/lifecycle-column-census.mjs") as { + stripComments: (source: string) => string; + }; + /* + `event.to` specifically: these services' move handlers are the drifted surface. A broader scan would + trip on unrelated status strings and on the formatters' own `transition` discriminant, which is a + caller-chosen mode and deliberately still a literal in BOTH files. + */ + const OFFENDING = /event\.to\s*[!=]==\s*"(in-progress|done|todo|triage|in-review|archived)"/g; + for (const file of SERVICES) { + const code = stripComments(readDoc(file)); + expect(code.match(OFFENDING) ?? [], `${file} still compares event.to to a legacy lane id`).toEqual([]); + } + }); +}); diff --git a/packages/dashboard/src/__tests__/gitlab-tracking-comments.test.ts b/packages/dashboard/src/__tests__/gitlab-tracking-comments.test.ts index 75e255a5c1..271fc96a59 100644 --- a/packages/dashboard/src/__tests__/gitlab-tracking-comments.test.ts +++ b/packages/dashboard/src/__tests__/gitlab-tracking-comments.test.ts @@ -22,6 +22,64 @@ describe("GitLabTrackingCommentService", () => { expect(fetchImpl.mock.calls[1][0]).toBe("https://gitlab.example.com/api/v4/projects/g%2Fp/issues/5/notes"); expect(s.logEntry).toHaveBeenCalledWith("FN-1", "Posted GitLab tracking comment", "g/p!5 (done)"); }); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-09:35 (fleet phase — the GitLab half, and the PAIR): + `handleTaskMoved` decided which moves warrant a comment by comparing `event.to` to the literals + `in-progress` and `done`. On a renamed board neither matched, so GitLab tracking silently posted NO + comments — the linked issue or MR just stopped being updated, with no error and no log line. + + The store fake gains a workflow reader ONLY for these cases; every case above omits it and keeps + asserting the legacy-id fallback, which is exactly why none of them could have caught this. (See + docs/solutions/test-failures/optional-flags-seam-hides-unconverted-column-guards.md.) + + REVERT CHECK, measured: restoring the literal lane test makes the renamed case fail with 0 fetch calls. + */ + 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: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + }; + + function renamedStore() { + const s = store(); + return Object.assign(s, { + getTaskWorkflowSelection: () => ({ workflowId: "custom:renamed", stepIds: [] }), + getWorkflowDefinition: async () => ({ ir: RENAMED_IR }), + }); + } + + it("posts a tracking comment when a card reaches a RENAMED complete lane", async () => { + const fetchImpl = vi.fn().mockResolvedValue(jsonResponse({ id: 1 })); vi.stubGlobal("fetch", fetchImpl); + const s = renamedStore(); new GitLabTrackingCommentService(s as any).start(); + s.emit("task:moved", { task: task("merge_request"), from: "backlog", to: "shipped" }); + await vi.waitFor(() => expect(fetchImpl).toHaveBeenCalledTimes(1)); + // The comment SHAPE is still the fixed "done" mode — a role decides the lane, not the wording. + expect(s.logEntry).toHaveBeenCalledWith("FN-1", "Posted GitLab tracking comment", "g/p!5 (shipped)"); + }); + + it("posts a tracking comment when a card reaches a RENAMED wip lane", async () => { + const fetchImpl = vi.fn().mockResolvedValue(jsonResponse({ id: 1 })); vi.stubGlobal("fetch", fetchImpl); + const s = renamedStore(); new GitLabTrackingCommentService(s as any).start(); + s.emit("task:moved", { task: task("group_issue"), from: "backlog", to: "building" }); + await vi.waitFor(() => expect(fetchImpl).toHaveBeenCalledTimes(1)); + }); + + it("stays silent for a move into a lane that plays no notable role on that renamed board", async () => { + // Non-vacuous: without this, a service commenting on EVERY move satisfies both cases above. + const fetchImpl = vi.fn(); vi.stubGlobal("fetch", fetchImpl); + const s = renamedStore(); new GitLabTrackingCommentService(s as any).start(); + s.emit("task:moved", { task: task(), from: "building", to: "backlog" }); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + it("skips missing auth without calling GitLab", async () => { const s = store(); s.getSettings.mockResolvedValueOnce({ gitlabAuthToken: "" }); const fetchImpl = vi.fn(); vi.stubGlobal("fetch", fetchImpl); new GitLabTrackingCommentService(s as any).start(); s.emit("task:moved", { task: task(), from: "todo", to: "done" }); diff --git a/packages/dashboard/src/github-tracking-comments.ts b/packages/dashboard/src/github-tracking-comments.ts index aee8efa304..55faf8af5e 100644 --- a/packages/dashboard/src/github-tracking-comments.ts +++ b/packages/dashboard/src/github-tracking-comments.ts @@ -1,4 +1,4 @@ -import { createLogger } from "@fusion/core"; +import { createLogger, resolveTaskLifecycleColumns } from "@fusion/core"; const severityAuditLog = createLogger("dashboard-github-tracking-comments"); import type { GlobalSettings, MergeDetails, ProjectSettings, Task, TaskStore } from "@fusion/core"; @@ -229,11 +229,34 @@ export class GitHubTrackingCommentService { return; } - if (event.to !== "in-progress" && event.to !== "done") { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-23:55 (fleet: github-tracking-comments.ts): + Resolved ONCE here — after the tracking-enabled gate — so a move on an UNTRACKED task pays nothing. + + FNXC:WorkflowResolvedColumns 2026-07-31-00:40 (PR #2715 review — greptile): + THE TRACKING GATE NOW RUNS FIRST, AND THE COLUMN TEST IS RESOLVED. + + An earlier version kept a literal `to !== "in-progress" && to !== "done"` early return ABOVE the + tracking gate, on the reasoning that converting it would make every task move in the project + resolve a workflow. That reasoning was sound about cost and wrong about correctness: on a renamed + board the literal never matched, so the function returned before reaching any of the resolved + code below and the tracking comment was silently skipped. A conversion that cannot be reached is + not a conversion. + + Reordering satisfies both. The tracking-enabled check is a plain property read on the event's own + task, so it costs nothing and still short-circuits every UNTRACKED move — which is the population + the cost argument was actually about. Only tracked tasks resolve a workflow, and those are the + ones that need the answer. + */ + if (event.task.githubTracking?.enabled !== true) { return; } - if (event.task.githubTracking?.enabled !== true) { + const movedLifecycle = await resolveTaskLifecycleColumns(this.store, event.task.id); + const wipColumn = movedLifecycle?.wip ?? "in-progress"; + const completeColumn = movedLifecycle?.complete ?? "done"; + + if (event.to !== wipColumn && event.to !== completeColumn) { return; } @@ -252,7 +275,7 @@ export class GitHubTrackingCommentService { return; } - if (event.to === "in-progress") { + if (event.to === wipColumn) { if (this.inProgressCommentClaims.has(event.task.id)) { return; } @@ -268,7 +291,7 @@ export class GitHubTrackingCommentService { const authoritativeTask = await this.store.getTask(event.task.id).catch(() => null); const taskForComment = authoritativeTask ?? event.task; if ( - event.to === "in-progress" + event.to === wipColumn && ( taskForComment.githubTracking?.inProgressCommentedAt || taskForComment.log?.some((entry) => ( @@ -279,9 +302,17 @@ export class GitHubTrackingCommentService { ) { return; } - const body = event.to === "done" - ? formatTrackingComment(taskForComment, event.to, { owner, repo }) - : formatTrackingComment(taskForComment, event.to); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-00:40 (PR #2715 review — greptile): + `formatTrackingComment`'s second parameter is a TRANSITION KIND, not a column id — it chooses + which comment to build. Passing `event.to` only type-checked because the literal early return had + narrowed it to the two legacy ids, so the id and the kind coincided on the default board. They do + not coincide on a renamed one, which is the conflation this whole conversion is about. The role is + now passed explicitly. + */ + const body = event.to === completeColumn + ? formatTrackingComment(taskForComment, "done", { owner, repo }) + : formatTrackingComment(taskForComment, "in-progress"); let commentPosted = false; try { @@ -289,7 +320,7 @@ export class GitHubTrackingCommentService { const globalSettings = (await this.store.getGlobalSettingsStore?.()?.getSettings?.() ?? {}) as Pick; const resolution = resolveGithubTrackingAuth({ projectSettings, globalSettings }); if (!resolution.ok) { - if (event.to === "in-progress") { + if (event.to === wipColumn) { this.inProgressCommentClaims.delete(event.task.id); } await this.safeLogDeletedTaskEntry(event.task.id, "Skipped GitHub tracking comment", resolution.message); @@ -301,7 +332,7 @@ export class GitHubTrackingCommentService { : new GitHubClient({ forceMode: "gh-cli" }); await client.commentOnIssue(owner, repo, number, body); commentPosted = true; - if (event.to === "in-progress") { + if (event.to === wipColumn) { try { await this.store.updateTask(event.task.id, { githubTracking: { inProgressCommentedAt: new Date().toISOString() }, @@ -324,7 +355,7 @@ export class GitHubTrackingCommentService { `${owner}/${repo}#${number} (${event.to})`, ); } catch (err) { - if (event.to === "in-progress" && !commentPosted) { + if (event.to === wipColumn && !commentPosted) { this.inProgressCommentClaims.delete(event.task.id); } const message = err instanceof Error ? err.message : String(err); diff --git a/packages/dashboard/src/gitlab-tracking-comments.ts b/packages/dashboard/src/gitlab-tracking-comments.ts index 2e1ec946c6..718273dc6a 100644 --- a/packages/dashboard/src/gitlab-tracking-comments.ts +++ b/packages/dashboard/src/gitlab-tracking-comments.ts @@ -1,4 +1,5 @@ import type { Task, TaskStore } from "@fusion/core"; +import { resolveTaskLifecycleColumns } from "@fusion/core"; import { resolveGitLabClient, resolveGitLabTargetFromItem, safeLogGitLabEntry } from "./gitlab-lifecycle.js"; import { getCliPackageVersion } from "./cli-package-version.js"; import { formatReleaseVersionLines } from "./fusion-release-version.js"; @@ -29,6 +30,13 @@ export function formatGitLabTrackingComment( targetUrl?: string, options?: { repository?: string; currentVersion?: string | (() => string) }, ): string { + /* + FNXC:WorkflowResolvedColumns 2026-07-31-09:10 (fleet phase — FLAGGED AND LEFT COUNTED, same as its GitHub twin): + A pure formatter. `transition` is this function's OWN `"in-progress" | "done"` parameter — a discriminant + the caller chose, not a column id read off a task — and there is no store or task id in scope to resolve + from. `github-tracking-comments.ts:165` is the same site with the same decision, so both halves of the + pair now leave exactly one literal, in the same place, for the same reason. + */ if (transition === "in-progress") { const prefix = `Fusion task: ${task.id}\n\n`; const stem = "🚧 In progress — work has started on “"; @@ -73,18 +81,55 @@ export class GitLabTrackingCommentService { } private async handleTaskMoved(event: TaskMovedEvent): Promise { - if (event.from === event.to || (event.to !== "in-progress" && event.to !== "done")) return; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-09:10 (fleet phase — the GitLab half of the pair): + IDENTICAL shape and ordering to `github-tracking-comments.ts`'s `handleTaskMoved`, on purpose. That + file's note explains the reordering: the lane test needs a resolved workflow, and resolving on EVERY + move to discover that most moves are not notable is the cost worth avoiding — so the cheap tracked-item + read (a plain property read on the event's own task) runs FIRST and short-circuits every untracked + move. Only tracked tasks resolve a workflow, and those are the ones that need the answer. + + This file is why the pair matters. GitHub tracking was being converted while GitLab tracking kept the + literals, which is the FN-6115 -> FN-6118 -> FN-6123 shape: one behaviour living in two modules with + only one converted. The two now read the same, so a reviewer can diff them. + */ + if (event.from === event.to) return; const item = event.task.gitlabTracking?.item; if (!item) return; + + const movedLifecycle = await resolveTaskLifecycleColumns(this.store, event.task.id); + const wipColumn = movedLifecycle?.wip ?? "in-progress"; + const completeColumn = movedLifecycle?.complete ?? "done"; + + if (event.to !== wipColumn && event.to !== completeColumn) return; const target = resolveGitLabTargetFromItem(item); if (!target) { await safeLogGitLabEntry(this.store, event.task.id, "Failed to post GitLab tracking comment", "Linked GitLab metadata is incomplete"); return; } + /* + FNXC:WorkflowResolvedColumns 2026-07-31-09:20 (fleet phase — a narrowing the old literal was doing for free): + `transition` is derived EXPLICITLY rather than passed as `event.to`. The removed early return + (`event.to !== "in-progress" && event.to !== "done"`) was not only a guard — it also NARROWED + `event.to` to the formatter's `"in-progress" | "done"` parameter type. Comparing against resolved + lane variables cannot narrow a string, so tsc rejected the call, which is the compiler catching the + real consequence: on a renamed board `event.to` was never one of those two words anyway, so passing + it through was always the wrong value for a parameter that means "which comment shape". + + Naming the discriminant separates the two questions the literal was conflating — WHICH LANE did the + card enter (a role question, resolved) and WHICH COMMENT does that warrant (a formatter mode, fixed). + + A BOOLEAN, not a `"done" | "in-progress"` string. My first version named it as a string and then asked + `transition === "done"` further down, which the census correctly counted as a NEW column guard (its + receiver is a local, but the classifier cannot know that, and it is the same shape as the + `mode === "done"` pair in useTaskDiffStats). A boolean answers the same question without introducing a + literal comparison at all — better than adding a site and then exempting it with a marker. + */ + const isCompleteTransition = event.to === completeColumn; const body = formatGitLabTrackingComment( event.task, - event.to, - event.to === "done" ? target.url : undefined, + isCompleteTransition ? "done" : "in-progress", + isCompleteTransition ? target.url : undefined, { repository: item.projectPath }, ); try { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 3be22b9a50..de21e11545 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-comments.ts": 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, @@ -31,7 +30,7 @@ "packages/core/src/task-store/task-store-helpers.ts": 4, "packages/dashboard/app/components/TaskReviewTab.tsx": 4, "packages/dashboard/app/components/taskSorting.ts": 4, - "packages/dashboard/src/gitlab-tracking-comments.ts": 4, + "packages/dashboard/src/gitlab-tracking-comments.ts": 1, "packages/dashboard/src/routes/register-git-github.ts": 4, "packages/engine/src/agent-heartbeat.ts": 4, "packages/engine/src/replan-target.ts": 4, @@ -116,6 +115,7 @@ "packages/dashboard/app/utils/stalePausedReviewCopy.ts": 1, "packages/dashboard/app/utils/taskStuck.ts": 1, "packages/dashboard/src/github-issue-comment.ts": 1, + "packages/dashboard/src/github-tracking-comments.ts": 1, "packages/dashboard/src/gitlab-issue-comment.ts": 1, "packages/dashboard/src/gitlab-source-issue-reconciler.ts": 1, "packages/dashboard/src/knowledge-index-refresh.ts": 1,