From f47fc167ee9b857f40f07397f8d08ae65fc4f4eb Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 21:07:28 -0700 Subject: [PATCH] =?UTF-8?q?convert(core/task-store/comments-ops.ts):=20tri?= =?UTF-8?q?age=20guards=203=20=E2=86=92=201,=20and=20the=20dead=20approval?= =?UTF-8?q?-invalidation=20it=20hid=20(#2608)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Taking `packages/core/src/task-store/comments-ops.ts`** (announced for collision avoidance). Two commits: a behaviour-identical extraction, then the conversion. | File | triage column comparisons before | after | |---|---|---| | `packages/core/src/task-store/comments-ops.ts` | **3** | **1** | `pnpm test:gate` green. ## The bug the literal was hiding `builtin:coding` → `BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR`, whose merged Planning column keeps the id **`todo`** and declares **no `triage` column**. So `task.column === "triage" && task.status === "awaiting-approval"` never matched a default card. The damage was graded: - **with a real spec** — the card fell through to the re-triage arm. Same `needs-replan` write, but audited as *"requested re-specification of planned task"* instead of *"invalidated spec approval"*. - **with a bootstrap-stub spec** — `hasRealPrompt` was false and **neither arm fired**, so a user comment on a card awaiting spec approval invalidated **nothing**. The approval silently stood. That second case is the real regression; the wording is cosmetic. I checked both rather than assuming the first one was the whole story. ## The conversion The column was never the discriminator. Callers reach this only after establishing the card sits in a pre-implementation column, so re-testing it inside was redundant before U11 and wrong after. **Status carries the distinction** — the same conclusion `spec-staleness.test.ts` already reached for its sibling guard. **Red-green:** the 3 new cases fail with the literal reinstated (**3 failed / 4 passed**) and pass without it. Two assert the merged-Planning card is now invalidated; the third uses a `planning`-named column to show no column id remains in the decision at all. **The 1 remaining literal is deliberate:** the caller's gate `column === "todo" || column === "triage"` names *both* vocabularies, so it still fires for default cards, and narrowing it to traits needs an IR the caller doesn't have. Commit 1 is move-only — the extracted body is the inlined expression verbatim, `triage` literals included, so the moved logic diffs empty apart from field renames. Behaviour change is entirely in commit 2. --- ## Census correction — the 48 is 41, and "reach ZERO" is wrong as stated I re-measured before picking a file, and the shared number needs three corrections. Same-scope method: `packages/*/src`, `.ts`, tests excluded, **comments stripped**. | Measurement | Count | |---|---| | raw `=== "triage"` / `!== "triage"` | 54 | | …comments stripped | **48** ← matches your figure | | …of those, genuine **column** comparisons | **41** | | …non-column identifiers that must NOT be converted | **7** | The 7 are `role === "triage"` ×3 (`agent-prompts.ts`), `agentType === "triage"` ×2 (`usage-limit-detector.ts`), `sessionPurpose === "triage"` (`skill-resolver.ts`), `surface === "triage"` (`tool-availability.ts`). **The triage service keeps its name; only the column id was merged away.** Converting these would break the triage lane, so the bar cannot be literal zero — it's zero *column* comparisons, with those 7 documented as permanent. Two I nearly misclassified and hand-checked: `col === "triage"` (`cli/commands/task.ts`, indexes `COLUMN_LABELS`) and `from === "triage"` (`executor.ts`, a `moveTask` from-column) **are** columns despite their names. ## Of the 41, which are actually dead Splitting by whether a `todo` companion arm sits in the same condition: - **27 have one** → still fire for default cards. Real but lower priority. - **14 have none** → candidates for silently-dead. But on inspection that set shrinks further: - `register-task-workflow-routes.ts` ×5 compare against a *resolved* `approveIntakeColumn`/`refineIntakeColumn` variable **plus** a legacy `"triage"` fallback, so they still fire via the variable; - `spec-staleness.ts:40` is a **deliberate R11 compat retention** — `spec-staleness.test.ts` already carries a "U11 proof" block concluding the guard is carried by status, not column, and that other workflows still declare `triage`. Converting it would be wrong; - `self-healing.ts` ×7 is U4's file; - `comments-ops.ts` ×1 was genuinely dead — this PR. **So the actionable dead set is far smaller than 14, and `self-healing.ts` holds most of it.** I'd suggest whoever takes `self-healing.ts` starts from that 7 rather than its 11 total. ## Files I evaluated and did NOT convert - **`replan-target.ts`** — my first pick, then both its "sites" turned out to be **comment text**. Zero real sites; already trait-resolved via `workflowHasColumn`. - **`mission-feature-sync.ts:88`** — `(column === "triage" || column === "todo")` still fires via the `todo` arm. The genuine gap is a custom-named planning column, but `reconcileMissionFeatureState`'s store is narrowed to `Pick`, so trait resolution means plumbing through `scheduler.ts` — **U5's file**. Left to avoid the collision, per KTD-2's warning that most sites have no IR in scope. - **`tool-availability.ts` / `skill-resolver.ts` / `usage-limit-detector.ts`** — non-column identifiers, see above. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../post-comment-retriage-decision.test.ts | 60 +++++++++++++++++++ packages/core/src/task-store/comments-ops.ts | 43 ++++++++++--- 2 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 packages/core/src/__tests__/post-comment-retriage-decision.test.ts 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