From f8c053c3fa87367a5a0d66507b3eadb49465fc80 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 00:27:43 -0700 Subject: [PATCH] =?UTF-8?q?fix(core):=20TAKING=20comments-ops.ts=20?= =?UTF-8?q?=E2=80=94=20re-triage=20on=20renamed=20planner=20lanes=20(3=20t?= =?UTF-8?q?riage=20guards=20=E2=86=92=200)=20(#2612)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Claiming `packages/core/src/task-store/comments-ops.ts`** from the shared backlog so nobody collides. ## Guard count | scope | before | after | |---|---|---| | `comments-ops.ts` | **3** | **0** | | repo-wide `column === / !== "triage"` in `packages/*/src` (excl. tests) | **26** | **23** | ## Why this file, and why it matters more than its size `addComment`'s post-comment **re-triage** decides, from the card's column, whether a user comment should invalidate an approved spec or send already-planned work back for re-specification. It asked with three legacy literals: ```ts task.column === "todo" || task.column === "triage" task.column === "triage" && status === "awaiting-approval" hasRealPrompt && (todo || (triage && status !== "awaiting-approval")) ``` On a renamed board none match, so a user comment on planned work does **nothing**: no approval invalidation, no re-specification, no error. **The operator types a correction and the agent never sees it.** This is the surface a human actually touches, which makes it the worst place in the program for a silent guard. Now resolved per task via `resolveLifecycleColumns`, fail-soft to the legacy pair — this phase is documented best-effort (*"failures are logged but never fail the comment add"*), so an unresolvable workflow must behave exactly as before rather than skip re-triage. ## Red-green, not green-only The suite was written **first** and failed **3 of 6** against the literals — precisely the three renamed cases — while the two negatives and the default-vocabulary floor passed throughout. Both negatives earn their place: re-triaging a **WIP** card would discard an in-flight session, and the **author gate** (agent comments must not re-triage) has to survive the conversion. ## Fixture guards its own preconditions `PROMPT.md` is written where the guard reads it rather than relying on task creation's side effects. `hasRealPrompt` gates two of the three branches, so a bootstrap stub would make those cases pass for the wrong reason — the trap that has produced two vacuous tests in this program already. ## Verification - new suite 6/6; `store-comments` 14/14 - full core PG: **1050 passed / 3 failed** — the same three that reproduce with this change stashed (`central-archive-secrets`, `workflow-settings-project-identity`) - core `tsc --noEmit` clean; `pnpm test:gate` green (482 + 132 + 10) 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit - **Bug Fixes** - Improved comment-driven re-triage for workflows with renamed planning columns. - Comments on planned or awaiting-approval tasks now correctly move eligible tasks to “Needs re-plan.” - Prevented re-triage for tasks actively in progress. - Preserved existing re-triage behavior for standard workflow columns. - Non-user comments no longer incorrectly trigger re-triage. --------- Co-authored-by: Claude Opus 5 (1M context) --- ...comments-retriage-renamed-lanes.pg.test.ts | 185 ++++++++++++++++++ packages/core/src/task-store/comments-ops.ts | 38 +++- 2 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/__tests__/postgres/comments-retriage-renamed-lanes.pg.test.ts diff --git a/packages/core/src/__tests__/postgres/comments-retriage-renamed-lanes.pg.test.ts b/packages/core/src/__tests__/postgres/comments-retriage-renamed-lanes.pg.test.ts new file mode 100644 index 0000000000..e37ed38dfb --- /dev/null +++ b/packages/core/src/__tests__/postgres/comments-retriage-renamed-lanes.pg.test.ts @@ -0,0 +1,185 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-09:10 (Phase B vocabulary conversion — comments-ops): + +`addComment`'s post-comment RE-TRIAGE decides, from the card's column, whether a user +comment should invalidate an approved spec or send already-planned work back for +re-specification. It asked that question with three legacy literals: + + task.column === "todo" || task.column === "triage" -> in a planner lane + task.column === "triage" && status === "awaiting-approval" -> intake, awaiting approval + hasRealPrompt && (todo || (triage && status !== awaiting-approval)) -> planned, re-triage + +On a renamed board none of them match, so a user comment on a planned card does nothing: +no invalidation, no re-specification, no error. The operator types a correction and the +agent never sees it — silent by construction, which is why no test caught it. + +RED-GREEN: every renamed case below fails against the pre-conversion literals (verified by +reverting), and the default-vocabulary cases are the regression floor. + +Real store, real persisted workflow, assertions on the persisted row — the transition this +path performs is a status write, so it is observable. +*/ +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { mkdirSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; + +pgDescribe("addComment re-triage under a renamed planner vocabulary (PostgreSQL)", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_comments_retriage_renamed", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /** A workflow whose intake is `drafting` and whose hold is `queued` — neither is a + * legacy id, so a literal-keyed guard matches nothing. */ + async function seedRenamedWorkflow(): Promise { + const created = await h.store().createWorkflowDefinition({ + name: "Renamed Planner", + kind: "workflow", + ir: { + version: "v2", + id: "custom:renamed-planner", + nodes: [ + { id: "start", kind: "start", column: "drafting" }, + { id: "end", kind: "end", column: "shipped" }, + ], + edges: [{ from: "start", to: "end" }], + columns: [ + { id: "drafting", label: "Drafting", traits: [{ trait: "intake" }] }, + { id: "queued", label: "Queued", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }, + ], + }, + } as never); + return (created as { id: string }).id; + } + + /** A card with a REAL (non-bootstrap) spec resting in `column`. */ + async function seedPlannedTask( + id: string, + column: string, + opts: { workflowId?: string; status?: string } = {}, + ): Promise { + const store = h.store(); + await store.createTaskWithReservedId( + { description: `retriage ${id}`, column } as never, + { taskId: id, applyDefaultWorkflowSteps: false } as never, + ); + if (opts.workflowId) await store.writeTaskWorkflowSelection(id, opts.workflowId, []); + /* A real (non-bootstrap) spec written where the guard reads it. Written directly + rather than through a store API so the fixture does not depend on task creation's + prompt side effects — `hasRealPrompt` is a precondition of two of the three branches + and a bootstrap stub would make those cases pass or fail for the wrong reason. */ + mkdirSync(store.taskDir(id), { recursive: true }); + writeFileSync( + join(store.taskDir(id), "PROMPT.md"), + "## Goal\nA genuine specification with real content.\n\n## Steps\n### Step 1: do it\n", + "utf-8", + ); + if (opts.status) { + await h.adminSql()`UPDATE project.tasks SET status = ${opts.status} WHERE id = ${id}`; + } + await h.adminSql()`UPDATE project.tasks SET "column" = ${column} WHERE id = ${id}`; + store.taskCache.delete(id); + const seeded = await store.getTask(id); + // Prove the fixture: a bootstrap-stub prompt or the wrong column would make every + // assertion below pass or fail for reasons unrelated to the vocabulary. + expect(seeded.column).toBe(column); + if (opts.status) expect(seeded.status).toBe(opts.status); + } + + async function persistedStatus(id: string): Promise { + const store = h.store(); + store.taskCache.delete(id); + return (await store.getTask(id)).status; + } + + /* + FNXC:PostCommentRetriage 2026-07-31-10:20 (PR #2612 review — coderabbit): + `needs-replan` is written by BOTH branches — approval invalidation and ordinary + planned-task re-triage — so asserting the status alone cannot tell them apart. A + card taking the WRONG branch persists the same status and the test still passes. + + The audited phrase is the only durable thing that distinguishes them, so the + awaiting-approval case asserts that instead of merely alongside. + */ + async function persistedLogPhrases(id: string): Promise { + const store = h.store(); + store.taskCache.delete(id); + const task = await store.getTask(id); + return (task.log ?? []).map((entry) => `${entry.action ?? ""} ${entry.details ?? ""}`.trim()); + } + + it("re-triages a planned card resting in the RENAMED hold column", async () => { + const wf = await seedRenamedWorkflow(); + await seedPlannedTask("FN-CR-1", "queued", { workflowId: wf }); + + await h.store().addComment("FN-CR-1", "please also handle the empty case", "user"); + + expect(await persistedStatus("FN-CR-1")).toBe("needs-replan"); + }); + + it("re-triages a planned card resting in the RENAMED intake column", async () => { + const wf = await seedRenamedWorkflow(); + await seedPlannedTask("FN-CR-2", "drafting", { workflowId: wf }); + + await h.store().addComment("FN-CR-2", "scope changed", "user"); + + expect(await persistedStatus("FN-CR-2")).toBe("needs-replan"); + }); + + it("invalidates spec approval for a RENAMED intake card that is awaiting-approval", async () => { + /* The second literal: `column === "triage" && status === "awaiting-approval"`. This is + the branch that stops an approved-but-now-stale spec from executing. */ + const wf = await seedRenamedWorkflow(); + await seedPlannedTask("FN-CR-3", "drafting", { workflowId: wf, status: "awaiting-approval" }); + + await h.store().addComment("FN-CR-3", "actually, do it differently", "user"); + + expect(await persistedStatus("FN-CR-3")).toBe("needs-replan"); + /* The discriminator. Without it this test passes when the card falls through to + the ordinary re-triage arm, which writes the same status under a different + audit phrase — the exact confusion this branch exists to prevent. */ + expect(await persistedLogPhrases("FN-CR-3")).toContainEqual( + expect.stringContaining("invalidated spec approval"), + ); + }); + + it("does NOT re-triage a card in the renamed WIP column", async () => { + /* The negative half: re-triaging executing work would discard an in-flight session. + `building` is neither intake nor hold. */ + const wf = await seedRenamedWorkflow(); + await seedPlannedTask("FN-CR-4", "building", { workflowId: wf }); + + await h.store().addComment("FN-CR-4", "a note mid-implementation", "user"); + + expect(await persistedStatus("FN-CR-4")).not.toBe("needs-replan"); + }); + + it("does NOT re-triage on a non-user comment", async () => { + /* The author gate is orthogonal to the vocabulary and must survive the conversion. */ + const wf = await seedRenamedWorkflow(); + await seedPlannedTask("FN-CR-5", "queued", { workflowId: wf }); + + await h.store().addComment("FN-CR-5", "agent progress note", "agent"); + + expect(await persistedStatus("FN-CR-5")).not.toBe("needs-replan"); + }); + + it("still re-triages a default-vocabulary card in `todo` (regression floor)", async () => { + await seedPlannedTask("FN-CR-6", "todo"); + + await h.store().addComment("FN-CR-6", "please adjust", "user"); + + expect(await persistedStatus("FN-CR-6")).toBe("needs-replan"); + }); +}); diff --git a/packages/core/src/task-store/comments-ops.ts b/packages/core/src/task-store/comments-ops.ts index e50d7d3115..c289840da5 100644 --- a/packages/core/src/task-store/comments-ops.ts +++ b/packages/core/src/task-store/comments-ops.ts @@ -194,7 +194,32 @@ export async function addCommentImpl(store: TaskStore, id: string, text: string, */ const retriageColumns = author === "user" ? await (async () => { - const workflowIr = await resolveWorkflowIrForTask(store, id).catch(() => undefined); + /* + FNXC:PostCommentRetriage 2026-07-31-08:05 (PR #2612 review — greptile): + SAY SO WHEN THE FALLBACK FIRES. The legacy pair is the right BEHAVIOUR on a + resolution failure — this is best-effort re-triage whose failure mode is a + MISSED re-spec — but swallowing the cause left the one question an operator + asks unanswerable: "why did my correction not re-trigger planning on a + renamed board?" The card simply does not re-triage, and the lanes it was + compared against are invisible. + + DEBUG, not warn, and deliberately: an unresolvable workflow is also the + ordinary shape for a task with no selection yet, so a warn would fire on + every such comment add and train people to ignore it. The four sibling + best-effort logs in this file warn because they describe an action that was + supposed to happen and did not; this describes a fallback that is frequently + correct. + */ + const workflowIr = await resolveWorkflowIrForTask(store, id).catch((err: unknown) => { + storeLog.debug("Post-comment re-triage planner-lane resolution failed", { + ...commentContextBase, + phase: "addComment:planner-lane-resolution", + fallbackHold: "todo", + fallbackIntake: "triage", + error: err instanceof Error ? err.message : String(err), + }); + return undefined; + }); const lifecycle = workflowIr ? resolveLifecycleColumns(workflowIr) : undefined; return { intake: lifecycle?.intake ?? "triage", @@ -218,6 +243,17 @@ export async function addCommentImpl(store: TaskStore, id: string, text: string, }); } + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-14:40 (rebase onto main's extraction): + Main extracted the two inner column checks into `resolvePostCommentRetriageDecision`, + which takes `column` and DOES NOT USE IT — the column decision now lives entirely in the + outer planner-lane gate above, which this branch converts. So the inner half of this + conversion is dropped as obsolete rather than re-applied on top. + + Worth flagging for that extraction's author: the unused `column` parameter reads as though + a column decision is still being made there. It is not, and a future reader adding one + back would silently double-gate. + */ const { invalidateApproval: shouldInvalidateAwaitingApproval, retriagePlanned: shouldRetriagePlannedTask } = resolvePostCommentRetriageDecision({ column: task.column, status: task.status, hasRealPrompt });