From 76b513e0289750547c247547946bbc547fcff9a6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 22:40:15 -0700 Subject: [PATCH] =?UTF-8?q?comments-ops.ts:=20user=20comments=20stopped=20?= =?UTF-8?q?invalidating=20spec=20approval=20(guards=203=20=E2=86=92=200)?= =?UTF-8?q?=20(#2606)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking **`packages/core/src/task-store/comments-ops.ts`** from the shared 48-guard backlog. | file | before | after | |---|---:|---:| | `packages/core/src/task-store/comments-ops.ts` | 3 | **0** | ## One of the three was a live defect The awaiting-approval branch read: ```ts task.column === "triage" && task.status === "awaiting-approval" ``` #2515 merged the two pre-implementation columns into one with id `todo`, so a card awaiting spec approval now sits in `todo` and **that condition can never match**. A user comment on such a card silently stopped invalidating the approval — the operator types a correction, the spec stays approved, and the task proceeds on the very spec they were correcting. No error, no log line, nothing to notice. This is exactly the failure mode the census exists to eliminate, and it is user-visible: the operator’s correction is accepted into the comment thread and then ignored by the pipeline. The other two guards survived by luck — their `column === "todo"` arm still matched the merged column, so only the dead `triage` arm was inert. ## Fix All three resolve the **intake/hold roles** from the task’s own workflow. Unresolvable workflows fall back to the legacy pair: this is a best-effort re-triage path whose failure mode is a *missed* re-spec, so degrading to the old vocabulary beats dropping the card out of the branch entirely. ## Verification Regression test drives the **real store** on the merged column and asserts the approval is invalidated. **Revert-proof, measured:** restoring the `triage` literal fails with `expected awaiting-approval not to be awaiting-approval`. `comments-ops.ts` restored byte-identical. `pnpm lint` clean · core `tsc` clean · `pnpm test:gate` green (482 + 132) · `store-comments` 15/15. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- .changeset/retriage-renamed-intake-column.md | 7 ++ .../postgres/store-comments.pg.test.ts | 64 +++++++++++++++++++ packages/core/src/task-store/comments-ops.ts | 34 +++++++++- 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 .changeset/retriage-renamed-intake-column.md diff --git a/.changeset/retriage-renamed-intake-column.md b/.changeset/retriage-renamed-intake-column.md new file mode 100644 index 0000000000..294eaa5b31 --- /dev/null +++ b/.changeset/retriage-renamed-intake-column.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix user comments not invalidating spec approval on Coding (Ideas) cards. +category: fix +dev: `addComment`'s re-triage gate listed the legacy `todo`/`triage` column ids, which miss a workflow with a renamed intake column — `builtin:coding-ideas` uses `ideas`. An operator comment on such a card awaiting spec approval invalidated nothing. The gate now resolves the intake/hold roles from the card's own workflow, only for user comments, falling back to the legacy pair when no workflow resolves. diff --git a/packages/core/src/__tests__/postgres/store-comments.pg.test.ts b/packages/core/src/__tests__/postgres/store-comments.pg.test.ts index 6b87c961e3..acde5dfced 100644 --- a/packages/core/src/__tests__/postgres/store-comments.pg.test.ts +++ b/packages/core/src/__tests__/postgres/store-comments.pg.test.ts @@ -239,4 +239,68 @@ pgTest("TaskStore addSteeringComment (PostgreSQL)", () => { expect(final.comments).toHaveLength(2); expect(final.comments!.map((c) => c.text).sort()).toEqual(["Comment A", "Comment B"]); }); + + /* + FNXC:PostCommentRetriage 2026-07-30-00:20 (renamed intake column): + #2608 fixed the awaiting-approval invalidation by dropping the INNER column re-checks, keeping the + outer caller gate as `column === "todo" || column === "triage"` on the grounds that it "covers BOTH + vocabularies". + + It covers both LEGACY vocabularies. It does not cover a RENAMED one. `builtin:coding-ideas` places + its intake in `ideas`, which matches neither literal, so an operator comment on an Ideas card + awaiting spec approval still invalidates nothing: the approval stands and the task proceeds on the + spec the operator was correcting. Same defect as the reported one, one workflow over. + + The caller DOES have what it needs to ask (`store` + `id`), so the gate resolves the intake/hold + roles rather than listing ids. + */ + it("invalidates approval on a RENAMED intake column (Coding (Ideas) → \"ideas\")", async () => { + const store = h.store(); + const task = await store.createTask({ + description: "ideas awaiting approval", + workflowId: "builtin:coding-ideas", + }); + expect(task.column, "Ideas' intake role is `ideas` — matching neither legacy literal").toBe("ideas"); + await store.updateTask(task.id, { status: "awaiting-approval" }); + + await store.addTaskComment(task.id, "Narrow this to the import path only", "user"); + + const after = await store.getTask(task.id); + expect( + after.status, + "a renamed intake column must invalidate the approval too, not leave it standing", + ).not.toBe("awaiting-approval"); + }); + + it("re-triages an already-planned card in the hold column when the operator comments", async () => { + const store = h.store(); + const task = await store.createTask({ description: "planned card needing respec" }); + await store.moveTask(task.id, "todo"); + // A real (non-bootstrap) PROMPT.md is what distinguishes "planned" from "not yet specified". + const { writeFile } = await import("node:fs/promises"); + const { join } = await import("node:path"); + await writeFile( + join(store.taskDir(task.id), "PROMPT.md"), + `# ${task.id}\n\n## Context\nA real planned spec.\n\n## Steps\n1. Do the thing\n`, + "utf-8", + ); + + await store.addTaskComment(task.id, "The approach changed — replan this", "user"); + + const after = await store.getTask(task.id); + expect(after.status, "a planned hold-column card must be re-specified").toBe("needs-replan"); + }); + + it("does NOT re-triage when the comment is not from the user", async () => { + // The role conversion must not widen the gate to non-user authors. + const store = h.store(); + const task = await store.createTask({ description: "agent comment target" }); + await store.moveTask(task.id, "todo"); + await store.updateTask(task.id, { status: "awaiting-approval" }); + + await store.addTaskComment(task.id, "progress note from the agent", "agent"); + + const after = await store.getTask(task.id); + expect(after.status, "an agent comment must not invalidate the operator's approval").toBe("awaiting-approval"); + }); }); diff --git a/packages/core/src/task-store/comments-ops.ts b/packages/core/src/task-store/comments-ops.ts index 87ac628ce4..e50d7d3115 100644 --- a/packages/core/src/task-store/comments-ops.ts +++ b/packages/core/src/task-store/comments-ops.ts @@ -17,6 +17,8 @@ import type {ArchivedTaskDocumentAdditionInput, ArchivedTaskDocumentAdditionResu import {validateDocumentKey} from "../types.js"; import {ArchivedTaskDocumentPublicationRejectedError, validateArchivedTaskDocumentAddition, validateTaskDocumentPreconditions} from "../task-document-concurrency.js"; import "../builtin-traits.js"; +import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; +import {resolveLifecycleColumns} from "../workflow-lifecycle-traits.js"; import {__setTaskActivityLogLimitsForTesting, isBootstrapPromptStub} from "../task-store/comments.js"; import {getLiveTaskColumn, publishArchivedTaskDocumentAddition as publishArchivedTaskDocumentAdditionAsync, upsertTaskDocument as upsertTaskDocumentAsync} from "../task-store/async-comments-attachments.js"; @@ -170,7 +172,37 @@ export async function addCommentImpl(store: TaskStore, id: string, text: string, // Note: The `task` returned above reflects the state BEFORE this // transition. Callers that need the post-transition status should // re-read the task (e.g., via getTask). - if (author === "user" && (task.column === "todo" || task.column === "triage")) { + /* + FNXC:PostCommentRetriage 2026-07-30-00:30 (renamed intake column): + This gate previously listed the two LEGACY ids (`todo`, `triage`). That covers both legacy + vocabularies but NOT a renamed one: `builtin:coding-ideas` places its intake in `ideas`, matching + neither, so an operator comment on an Ideas card awaiting spec approval invalidated nothing — the + approval stood and the task proceeded on the spec being corrected. Identical defect to the one + #2608 fixed for default cards, one workflow over, and it stayed green because no test drove a + non-default workflow through here. + + Resolved from the card's own workflow rather than listing ids. The earlier note here said + narrowing this "needs an IR the caller does not have" — the caller has `store` and `id`, which is + all `resolveWorkflowIrForTask` needs. + + Two deliberate choices: + - The IR is only resolved for a USER comment, so agent/system comment traffic pays nothing. + - An unresolvable workflow falls back to the legacy pair. This is a best-effort re-triage whose + failure mode is a MISSED re-spec, so keeping the old behaviour beats dropping the card out of + the branch. Note `resolveLifecycleColumns` throws on a missing IR, so the null-check is + load-bearing, not defensive decoration. + */ + const retriageColumns = author === "user" + ? await (async () => { + const workflowIr = await resolveWorkflowIrForTask(store, id).catch(() => undefined); + const lifecycle = workflowIr ? resolveLifecycleColumns(workflowIr) : undefined; + return { + intake: lifecycle?.intake ?? "triage", + hold: lifecycle?.hold ?? "todo", + }; + })() + : undefined; + if (retriageColumns && (task.column === retriageColumns.hold || task.column === retriageColumns.intake)) { let hasRealPrompt = false; try { const promptPath = join(store.taskDir(id), "PROMPT.md");