fix(core): TAKING comments-ops.ts — re-triage on renamed planner lanes (3 triage guards → 0) (#2612)
**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) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -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<string> {
|
||||
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<void> {
|
||||
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<string | null | undefined> {
|
||||
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<string[]> {
|
||||
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");
|
||||
});
|
||||
});
|
||||
@@ -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 });
|
||||
|
||||
|
||||
Reference in New Issue
Block a user