feat(task-board): let report-generated tasks be deleted, and make it stick - #5720
Conversation
…stick TASK_BOARD_ITEM_DELETE refused any task with createdBy === "system", so the 36 Commerce Discovery findings on a board could never be cleared. The card detail rendered a delete button anyway and swallowed the rejection: the mutation had no onError, the board imports no toast, and the dialog closed before the result arrived — a refused delete read as a successful one. Drop the isReportsTask guard from delete. The edit guard in update.ts stays: the import legitimately owns title/description/priority. Lifting the guard alone would be a half-fix. The import dedups findings by external_key against OPEN items only, so a deleted row matches nothing and the next scan re-creates the card. Deleting all 36 and re-running a scan would bring all 36 back. Migration 163 adds task_board_dismissed_findings; delete tombstones the finding's key inside the existing transaction (outside it, a tombstone could survive a rollback and suppress a finding whose card still exists). The import skips those keys and reports `dismissed: N`, since a silent skip reads as "imported everything" when it didn't. TASK_BOARD_DISMISSED_LIST / _RESTORE undo it. Migration 164 closes a billing bypass the guard removal opens. task_quota_claims cascaded from task_board_items — unreachable while reports tasks (the only ones the quota charges) couldn't be deleted. Once they can be: delegate to the Super Agent to charge a period slot, delete the card, slot refunded, repeat for unlimited subsidized runs. Dropping the FK makes claims an append-only ledger; a refund already has one writer and one meaning (state = 'released'). Web: add onError to the delete mutation, and removeMany so a partly-failed bulk delete reports one summary instead of one toast per card. Also correct the Task Manager agent prompt, which called deletion merely irreversible without mentioning it now dismisses the finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tombstone table Replaces the task_board_dismissed_findings table and the task_quota_claims FK drop with one nullable column, task_board_items.dismissed_at. The tombstone's whole payload — (org, external_key, who, when) — already lived on the row it shadowed. Keeping the row instead of deleting it means: - migration 164 is unnecessary. The refund-by-delete hole only existed because task_quota_claims cascades from a hard-deleted card; a dismissed card never fires the cascade, so the charge survives without changing billing-ledger semantics or shipping a down() that deletes ledger rows. - restoring brings back the SAME card, with its comments, activity, linked threads, PRs and quota claim. Before, a restore re-created a fresh card on the next import and lost all of it. - the import needs one query, not two: it already selects by external_key. Dismissal keys on isReportsTask (created_by = 'system'), not on external_key — the quota gate charges that same class, and an imported item may omit its key, so keying on the key would leave keyless system cards hard-deletable and therefore refundable. The quota invariant test covers exactly that case. Also: getUserId(ctx) ?? "system" instead of a non-null assertion, and the two new i18n keys follow the neighboring taskBoard.taskBoard.* prefix.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Deleting a reports card dismisses its finding. Without this the delete | ||
| // would silently undo itself: a deleted row matches no open item, so the | ||
| // next run would create the card again. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // The card is dismissed, not dropped — its comments, activity and quota | ||
| // claim stay with it, so restoring brings back the same card. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| * A key the org has DISMISSED (deleted its card — the row stays with | ||
| * `dismissed_at` set, migration 163) is skipped entirely and counted in the | ||
| * response's `dismissed`. That's what makes deleting a reports-pushed card | ||
| * stick: without it, the next run re-creates the card, since a card that's gone | ||
| * matches no open item. | ||
| * |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Existing cards for the batch's external keys. An OPEN one gets | ||
| // refreshed instead of duplicated; done ones don't match (a regression is | ||
| // a new card); a DISMISSED one suppresses the finding entirely — that's | ||
| // what makes deleting a reports card stick, since a delete the next scan | ||
| // undoes isn't a delete. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Dismissal wins over an open card with the same key: a finding can | ||
| // be closed, regress into a fresh card, then be dismissed. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Suppression must be visible: a silent skip reads as "imported | ||
| // everything" when it didn't. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Pure predicate — the single source of truth for "this card is a report's | ||
| // finding", shared with the quota gate that charges the same class of task. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Dismissed findings are off the board — see `delete`. `getById` still | ||
| // resolves them, so a run reaction or a restore can reach the row. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Reports-pushed tasks are deletable like any other — storage dismisses | ||
| // theirs instead of dropping the row, so the next import skips the finding | ||
| // rather than re-creating the card. TASK_BOARD_DISMISSED_RESTORE undoes it. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Deleting a card must not be a backdoor refund. `task_quota_claims` cascades | ||
| // from `task_board_items`, so a hard delete would turn "delete the card" into | ||
| // a refund: delegate, delete, and the period slot frees up — repeat for | ||
| // unlimited subsidized runs. A reports card is dismissed, not dropped, so the | ||
| // cascade never fires and the charge outlives the card. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // The period the charge landed in, read rather than guessed — it depends on | ||
| // the org's billing status. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| * content immutability (UPDATE), the delete-and-dismiss path (DELETE writes a | ||
| * dismissal tombstone so the finding doesn't return on the next import), and | ||
| * the pre-write paywall on the delegation flip — everything that fires BEFORE | ||
| * any thread/dispatch machinery, so the ctx stub stays small. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Deleting a reports task used to be refused outright. It's allowed now — | ||
| // the board's delete button offered it and silently failed. What replaces the | ||
| // refusal is the dismissal tombstone: the finding stays off the board instead | ||
| // of the card coming back on the next import. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // A rejected delete used to land nowhere: the dialog had already closed, so | ||
| // a refused deletion read as a successful one. Surface it and refetch, so | ||
| // the card the server kept comes back into the list. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Bulk delete from the selection bar. Calls the tool directly rather than | ||
| // fanning out through `remove` so a partly-failed batch reports one summary | ||
| // instead of one toast per card. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| * A DISMISSED key (`dismissed_at` set) is skipped and counted in `dismissed`. | ||
| * |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // Open cards get refreshed, done ones don't match, dismissed ones | ||
| // suppress the finding. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
| // `task_quota_claims` cascades from `task_board_items`, so a hard delete | ||
| // would be a backdoor refund: delegate, delete, repeat. |
There was a problem hiding this comment.
A comment that takes a paragraph to justify a workaround is a signal the code is wrong, not the comment — fix the code, don't explain it away (CLAUDE.md).
PR: #5720 feat(task-board): let report-generated tasks be deleted, and make it stick Bump type: minor - decocms (apps/api/package.json): 4.178.0 -> 4.179.0 - @decocms/native (apps/native/package.json): 4.178.0 -> 4.179.0 - @decocms/shared (packages/shared/package.json): 0.23.0 -> 0.24.0 Deploy-Scope: both
TaskBoardStorage.delete() returned void unconditionally, so a
nonexistent or cross-org id fell through as a silent no-op — the
tool still returned {success: true} and broadcast a removal event
for a card that was never touched. This is the exact bug class
#5720 just fixed for the missing-guard case (delete swallowed a
rejection and read as success), reintroduced for the not-found case.
delete() now returns whether the id existed in this org; the tool
throws "Task board item not found" and skips the broadcast when
it didn't.
Regression test: reports-task-guards.integration.test.ts asserts
TASK_BOARD_ITEM_DELETE rejects an unknown id instead of resolving.
Verified: bun run fmt, apps/api tsc --noEmit, oxlint on the 3
touched files. The integration test needs real Postgres (unavailable
here) — CI runs it.
2 participants
What is this contribution about?
TASK_BOARD_ITEM_DELETErefused any task withcreatedBy === "system", so report-generated findings (e.g. 36 Commerce Discovery cards on the deco-studio board) could never be cleared — while the card detail rendered a delete button anyway and swallowed the rejection, because the mutation had noonErrorand the dialog closed before the result arrived. This drops the guard (the edit guard inupdate.tsstays — the import legitimately owns title/description/priority) and makes the delete stick.A reports card is dismissed, not dropped: one nullable column,
task_board_items.dismissed_at. The row stays, leaves the board, and the import skips itsexternal_key— which it has to, because the import dedups by key against open items only, so a deleted row matches nothing and the next scan re-creates the card.Keeping the row is what makes the rest fall out for free:
task_quota_claimscascades fromtask_board_items, so hard-deleting a charged card would refund its period slot — delegate, delete, repeat for unlimited subsidized runs. A dismissed card never fires the cascade, so no FK drop and no change to billing-ledger semantics.external_key.Dismissal keys on
isReportsTask(created_by = "system"), not on "has anexternal_key" — the quota gate charges that same class of task and an imported item may omit its key, so keying on the key would leave keyless system cards hard-deletable and therefore refundable.Also adds
removeManyso a partly-failed bulk delete reports one summary instead of one toast per card, and corrects the Task Manager agent prompt, which called deletion merely irreversible without mentioning it now dismisses the finding.How did you verify your code works?
410 tests pass, 0 fail against real Postgres 16 (
bun testoverapps/api/src/{tools/task-board,billing,storage}andtask-board-import.integration.test.ts).bun run checkandbun run lint(0 errors) are clean.I inverted the test that encoded the old behavior —
reports-task-guards.integration.test.tsasserted delete throws for a reports task — and added coverage for dismiss-on-delete, import skip +dismissed: N, restore-refreshes-the-same-card, org-scoping of dismissals, and empty-array-restores-nothing.The quota invariant test (
quota-refund.integration.test.ts) earns its keep: it caught an earlier version of this change that keyed dismissal onexternal_key, which left keyless system cards hard-deletable and the refund hole open. It charges acreated_by = "system"card with no key, deletes it, and asserts the claim count is unchanged and the claim stillheld.Migration 163 was confirmed to roll back and re-apply cleanly via a scripted
migrateDown/migrateToLatestround-trip, checking the column's absence and presence on each side.Screenshots/Demonstration
None — the UI change is behavioral (an error toast that previously never appeared). Reproduce per "How to Test" below.
How to Test
bun run --cwd=apps/api migrate(addstask_board_items.dismissed_at).POST /api/:org/internal/task-board/importwithVAULT_SERVICE_TOKENbearer, body{"items":[{"title":"finding","externalKey":"diag:example.com:lcp"}],"source":{"url":"example.com","run_id":"r1"}}./$org/board, open the card, click the trash icon — expected: the card leaves the board (before this PR it silently stayed).run_id— expected:{"created":0,"updated":0,"delegated":0,"dismissed":1}and no new card.TASK_BOARD_DISMISSED_RESTOREwith{"externalKeys":["diag:example.com:lcp"]}— expected: the same card is back on the board immediately, comments and history intact; the next import reportsupdated: 1, notcreated: 1.Migration Notes
One Kysely migration, reversible (
bun run --cwd=apps/api migrate):task_board_items.dismissed_at.down()drops it, which un-dismisses every card back onto the board — the recoverable direction (a finding reappears; nothing is lost).list()filtersdismissed_at is null;getByIddeliberately does not, so a run reaction or a restore can still reach the row by id.Behavioral note for the reports engine: the import response gains an optional
dismissedfield, and previously-imported findings whose cards get dismissed will stop being re-pushed. Dismissal is currently permanent — a genuinely regressing finding won't re-surface until restored. Making it expire is now adismissed_atage comparison in the import filter, no schema change needed; picking the window (30d? 90d?) is a product call, so it's deliberately not in this PR.Review Checklist
dismissedfield is additive, and the quota ledger's semantics are unchanged (a dismissed card never triggers the claim cascade).