fleet: github-tracking-comments.ts 9 → 3 — and the sync-filter class is now in a FOURTH file (~25 sites on one decision) (#2715)

Claiming the **github-tracking pair**. This converts the comments half;
the reconciler half is the flagged class, with the evidence below.

## Census before/after

| | before | after |
|---|---:|---:|
| `github-tracking-comments.ts` | **9** | **3** |

Baseline re-recorded; `--strict` exits 0.

## Converted: 6

The `event.to === "in-progress"` / `=== "done"` sites in
`handleTaskMoved`, to the **wip** and **complete** roles. One
resolution, placed **immediately after the tracking-enabled gate** — so
a move on an **untracked** task pays nothing, which is most moves in
most projects.

## Deliberately not converted: 2 — the ordering is the reason

```ts
if (event.to !== "in-progress" && event.to !== "done") return;   // line 232
```

This runs **before** the tracked-task gate. Converting it moves the
resolution ahead of that gate and makes **every task move in the
project** resolve a workflow just to decide the task has no GitHub
issue. That's a real cost on the hottest event in the system, to convert
a guard whose only job is a cheap filter. Recorded at the site.

## The remaining 1

**Line 165** — `transition === "done"` inside `formatTrackingComment`, a
**pure formatter** with no store and no task. Same shape as
`project-engine.ts:2555`. Threading a resolution into a formatter to
pick a string is the wrong trade.

## `github-tracking-reconciler.ts` (9) — not claimed here, and here is
why

All nine are:

```ts
.filter((task) => task.column === "done" || task.column === "archived")
```

Synchronous filters over task **lists**, where per-task resolution is N
awaits inside a sync predicate.

**This is the fourth file with that exact shape** — after `store.ts`
(#2709), and the dependency pairs in `TaskDetailModal` (#2696) and
`register-task-workflow-routes` (#2700). By my count **roughly 25 sites
across four files now wait on one decision**:

1. **Prefetch lifecycle columns alongside the task list** and pass a
resolved map into these predicates — keeps them synchronous, one
resolution per *distinct workflow* rather than per task. This is the one
I'd argue for.
2. Make the predicates async and accept per-task resolution.

It's a design change, and four workers guessing separately is exactly
how two halves of one rule drift apart. One decision covers all of them.

## Verification

`pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) ·
`github-tracking-comments` + `github-issue-comment` **81/81** · `pnpm
lint` clean · dashboard `tsc` clean · `--strict` exits 0.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 06:39:12 -07:00
committed by GitHub
parent b9f3fb0ff2
commit e0010f241e
6 changed files with 261 additions and 16 deletions

View File

@@ -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<string, unknown>;
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<string, unknown>;
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();

View File

@@ -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([]);
}
});
});

View File

@@ -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" });

View File

@@ -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<GlobalSettings, never>;
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);

View File

@@ -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<void> {
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 {

View File

@@ -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,