diff --git a/packages/core/src/__tests__/post-comment-retriage-decision.test.ts b/packages/core/src/__tests__/post-comment-retriage-decision.test.ts new file mode 100644 index 0000000000..aaabf26b4e --- /dev/null +++ b/packages/core/src/__tests__/post-comment-retriage-decision.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { resolvePostCommentRetriageDecision } from "../task-store/comments-ops.js"; + +/* +FNXC:PostCommentRetriage 2026-07-29-19:15: +Characterization of the decision `addCommentImpl` makes when a USER comments on a +card that is still in a pre-implementation column: either the pending spec approval +is invalidated, or an already-specified card is sent back for re-specification. +Both write `status: "needs-replan"`; they differ in the audit wording, and — the +case that matters — in WHETHER anything happens at all. + +Post-conversion: STATUS decides. The two cases marked U11 REGRESSION GUARD are the +ones the column literals got wrong for the default lineage — they fail if the +`column === "triage"` checks come back. +*/ +describe("resolvePostCommentRetriageDecision — status is the discriminator", () => { + it("invalidates a pending approval on the legacy planner column", () => { + expect(resolvePostCommentRetriageDecision({ column: "triage", status: "awaiting-approval", hasRealPrompt: false })) + .toEqual({ invalidateApproval: true, retriagePlanned: false }); + }); + + it("re-triages a specified card on the legacy planner column", () => { + expect(resolvePostCommentRetriageDecision({ column: "triage", status: null, hasRealPrompt: true })) + .toEqual({ invalidateApproval: false, retriagePlanned: true }); + }); + + it("re-triages a specified card in the hold column", () => { + expect(resolvePostCommentRetriageDecision({ column: "todo", status: null, hasRealPrompt: true })) + .toEqual({ invalidateApproval: false, retriagePlanned: true }); + }); + + it("does nothing for an unspecified card with no pending approval", () => { + expect(resolvePostCommentRetriageDecision({ column: "todo", status: null, hasRealPrompt: false })) + .toEqual({ invalidateApproval: false, retriagePlanned: false }); + }); + + /* + The three below are what the column literals got wrong. `builtin:coding` resolves + to BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR, whose merged Planning column + keeps the id `todo` and declares NO `triage` column — so a default card awaiting + spec approval matched neither `triage` arm. + */ + it("U11 REGRESSION GUARD: a merged-Planning card awaiting approval IS invalidated", () => { + // Pre-conversion this returned all-false: the approval silently stood. + expect(resolvePostCommentRetriageDecision({ column: "todo", status: "awaiting-approval", hasRealPrompt: false })) + .toEqual({ invalidateApproval: true, retriagePlanned: false }); + }); + + it("U11 REGRESSION GUARD: invalidation wins over re-triage when a spec exists", () => { + // Pre-conversion this re-triaged, mis-auditing an approval invalidation. + expect(resolvePostCommentRetriageDecision({ column: "todo", status: "awaiting-approval", hasRealPrompt: true })) + .toEqual({ invalidateApproval: true, retriagePlanned: false }); + }); + + it("a card on a workflow with a differently-named planning column behaves the same", () => { + // The point of the conversion: no column id appears in the decision at all. + expect(resolvePostCommentRetriageDecision({ column: "planning", status: "awaiting-approval", hasRealPrompt: false })) + .toEqual({ invalidateApproval: true, retriagePlanned: false }); + }); +}); diff --git a/packages/core/src/task-store/comments-ops.ts b/packages/core/src/task-store/comments-ops.ts index 3557b670e1..87ac628ce4 100644 --- a/packages/core/src/task-store/comments-ops.ts +++ b/packages/core/src/task-store/comments-ops.ts @@ -20,6 +20,40 @@ import "../builtin-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"; +/* +FNXC:PostCommentRetriage 2026-07-29-19:30 (U11 lifecycle-column conversion): +STATUS is the discriminator here, not the column id. + +Callers reach this only after establishing the card sits in a pre-implementation +column, so re-testing the column inside was always redundant — and after U11 it was +WRONG. `builtin:coding` resolves to BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR, +whose merged Planning column keeps the id `todo` and declares no `triage` column at +all. The old `column === "triage" && status === "awaiting-approval"` therefore never +matched a default card, and the consequences were graded: + + - with a real spec, the card fell through to the re-triage arm — same + `needs-replan` write, but audited as "requested re-specification" instead of + "invalidated spec approval"; + - with a bootstrap-stub spec, `hasRealPrompt` was false and NEITHER arm fired, so + an operator comment on a card awaiting spec approval invalidated nothing. The + approval silently stood. + +Dropping the column re-checks fixes both and removes 2 of this file's 3 `triage` +literals. The remaining one is the caller's gate (`column === "todo" || column === +"triage"`), which is deliberately left: it covers BOTH vocabularies, so it still +fires for default cards, and narrowing it to traits needs an IR the caller does not +have. +*/ +export function resolvePostCommentRetriageDecision(input: { + column: string; + status?: string | null; + hasRealPrompt: boolean; +}): { invalidateApproval: boolean; retriagePlanned: boolean } { + const invalidateApproval = input.status === "awaiting-approval"; + const retriagePlanned = input.hasRealPrompt && !invalidateApproval; + return { invalidateApproval, retriagePlanned }; +} + export async function addCommentImpl(store: TaskStore, id: string, text: string, author: string = "user", options?: { skipRefinement?: boolean; source?: "user" | "agent" | "github-review" | "github-review-comment"; externalId?: string; reviewState?: "APPROVED" | "CHANGES_REQUESTED" | "COMMENTED"; }, runContext?: RunMutationContext,): Promise { { const layer = store.asyncLayer!; @@ -152,13 +186,8 @@ export async function addCommentImpl(store: TaskStore, id: string, text: string, }); } - const shouldInvalidateAwaitingApproval = - task.column === "triage" && task.status === "awaiting-approval"; - const shouldRetriagePlannedTask = hasRealPrompt - && ( - task.column === "todo" - || (task.column === "triage" && task.status !== "awaiting-approval") - ); + const { invalidateApproval: shouldInvalidateAwaitingApproval, retriagePlanned: shouldRetriagePlannedTask } = + resolvePostCommentRetriageDecision({ column: task.column, status: task.status, hasRealPrompt }); if (shouldInvalidateAwaitingApproval || shouldRetriagePlannedTask) { const phase = shouldInvalidateAwaitingApproval