feat(queue): retention, delete en clear in /queue/messages #67
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
janpeter/scrum4me-workers!67
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/queue-purge-parity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Maakt
/queue/messageshet enige queue-dashboard, zodat de Messages-pagina van Ops-dashboard eruit kan (volgende PR) zonder functieverlies.Waarom dit moet
De berichtenqueue
agent_messageverhuist naar de scrum4me-DB. Ops-dashboard schrijft erop via zijn eigen Prisma-client op zijn eigen database — na de verhuizing zou die stil naar een verlaten tabel blijven schrijven: je stuurt een taak, hij verdwijnt, geen foutmelding. Prisma kan geen twee databases in één client, dus meeverhuizen kan niet. Die feature gaat er dus uit, en deze repo moet eerst alles kunnen.Geen van beide dashboards was een superset. Ops kon
delete,clear,previewPurgeenpurgeOlderThan; deze repo konreplyenmax2/jpadresseren (Ops kent alleenmac/scrum4me-server×claude/codex). Onderweg bleken er nog twee gaten, die er ook in zitten.Wat er in zit
Geporteerd uit Ops (semantiek 1-op-1, harde delete, archief ongemoeid):
previewPurgeOlderThan,purgeOlderThan,delete,clear.Twee pariteitsgaten gedicht (
2f8ef68):pending, terwijl Ops hem toont bijpendingófclaimed— de actie ondersteunde het al, de UI liep achter. Zonder fix verlies je "annuleer een geclaimd verzoek".Eén bewuste structurele afwijking: de bron dupliceert de retention-CTE in preview én purge. Dat is een drift-val — wijzigt iemand er één, dan telt de dry-run iets anders dan de purge verwijdert, precies wanneer je hem nodig hebt. Hier is het één gedeelde
CLOSED_THREAD_TARGETS_CTE; divergeren kan niet meer.Wat de reviews opleverden
Vier dingen die de code beter maakten dan een 1-op-1-port:
De delete-knop faalde op de meeste rijen waar hij stond. Een verzoek mét replies is niet te verwijderen —
reply_link_matches_typeblokkeert deSET NULLdie de FK wil doen. En omdatdone --replyde normale flow is, hébben de meeste gesloten threads een reply. Ops gooit daar vandaag een rauwe Postgres-fout; hier vertaaltdeleteAgentMessage23514op die constraint naar "Dit verzoek heeft antwoorden; ruim de hele thread op met de purge."clearbevestigde op proza terwijlpurgeop cijfers bevestigde — precies verkeerd om, want clear sloopt óókpending/claimed, dus lopend agent-werk. Nu toont de dialoogrows,open verzoekenenongelezen antwoordenapart. Gesplitst omdat het twee verschillende verliezen zijn: openstaande verzoeken zijn agent-werk dat sneuvelt, ongelezen antwoorden staan nog ongeackt in iemands inbox. Eén getal dat ze optelt zegt geen van beide.De open-telling zat er 4× naast. Replies dragen default
status='pending', dusCOUNT(*) FILTER (WHERE status IN ('pending','claimed'))telde ze mee: de dialoog zei "4 openstaande verzoeken" waar er één liep. Erger nog: de test asserteerdeopen: 4mét een comment die de replies als verwacht opsomde — de guard codeerde de bug. Veertien rode mutaties vonden het daarom niet; een mutatietest bewijst dat de code doet wat de test zegt, niet dat de test iets waars zegt.De bevestigingsdialoog draait nu op Base UI's
alert-dialog, die ongebruikt inpackage.jsonzat: focus-trap, Escape,aria-labelledby/aria-describedby. Backdrop-click krijg je bewust niet — Base UI sluit pointer-dismissal structureel uit op de alert-variant (Omit<DialogRoot.Props, 'modal' | 'disablePointerDismissal' | …>), want dat is het WAI-ARIA-patroon: een keuze die rijen definitief verwijdert hoor je te beantwoorden, niet per ongeluk weg te klikken.Bewijs
De mocks bewijzen de structuur van de queries. Wat een mock niet kan checken: of
WITH RECURSIVE+ een data-modifying CTE (DELETE … RETURNING) in één statement geldig is én de juiste rijen raakt. Daarom staat de echte run nu in de repo alsdescribe.skipIf(!process.env.OPS_TEST_DATABASE_URL)(patroon uit__tests__/mcp-tester/schema-snapshot.test.ts), tegen een fixture die een metdiffgeverifieerde letterlijke kopie van Ops' migratie is — inclusief FK en alle vier de CHECKs.Dat committen betaalde zich meteen terug: met het volledige schema blijkt
type='result'+in_reply_to IS NULLillegaal. Een eerdere validatie draaide op een DDL die uit het Prisma-model was afgeleid — en Prisma-modellen dragen geen CHECKs — en testte dus een "losse reply-wortel" die niet kan bestaan. Die val staat nu als eerste test in het bestand.Uitkomsten:
deletedRows === targetRows(de dry-run liegt niet), geneste replies gaan mee als thread, open verzoeken blijven,donezónderfinished_atblijft, de<-grens klopt (3-dagen-oude wortel valt bij 1d wél en bij 7d niet), tweede purge is een no-op,clearoverleeft de self-FK.De real-DB-test heeft een guard die de database-naam op wegwerpbaarheid controleert vóór hij iets aanraakt — URL-parsing, geen string-compare, dus
?sslmode=requireomzeilt hem niet.Verificatie
npx vitest run→ 711 passed | 10 skipped (nulmeting op main: 665 | 2). MétOPS_TEST_DATABASE_URL→ 8 extra passed.npm run typecheck→ 0 errorsnpm run lint→ 0 errors (1 pre-existing warning incomponents/jobs/job-card.tsx, niet aangeraakt)Volgorde
Deze PR moet gemerged zijn vóór de volgende (Ops-dashboard opruimen) — anders verdwijnt de retention. Daarna pas de data-cutover.
🤖 Generated with Claude Code
Volgt de mock-conventie van `__tests__/lib/worker-insights/ops-db.test.ts` (`vi.mock('pg')`), dus geen echte DB. Kern is de structurele garantie: `CLOSED_THREAD_TARGETS_CTE` moet letterlijk in zowel de preview- als de purge-SQL zitten. Wie daar een eigen kopie inlijnt en de twee laat divergeren, breekt de test — dat is precies de drift-val die de bron nog open heeft staan. Verder: preview bevat geen DELETE, purge wél; de cutoff bindt als parameter en staat niet in de SQL-tekst; delete/clear raken alleen `agent_message`; `parseRetentionDays` laat 1 en 7 door en gooit op 0/3/30/'abc'/undefined. Elke case van de gooi-tabel heeft een expliciet label. Kaal `%j` noemde de case `[1]` namelijk "gooit op 1", wat leest als de geldige waarde 1 — en `%p` bestaat niet eens in vitest (jest-ism) en bleef letterlijk in de naam staan, waardoor `-t` er niet op kon filteren. Elke assertie is rood bewezen door de implementatie te muteren: gesplitste CTE, DELETE in de preview, geinterpoleerde cutoff, WHERE-loze delete, clear op de verkeerde tabel, uren i.p.v. dagen, en een CTE zonder recursie of status-filter — 12 mutaties, allemaal rood. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Twee gaten die de Ops-pagina onverwijderbaar maakten: zonder deze fixes is "geen functieverlies" aantoonbaar onwaar. 1. De cancel-knop verscheen alleen bij `pending`, terwijl Ops hem ook bij `claimed` toont — en `cancelAgentMessage` het allang ondersteunt (`status IN ('pending','claimed')`). De UI liep dus achter op zijn eigen backend, en juist een verzoek dat al bij een agent ligt wil je kunnen intrekken. Nu via `isCancellable`, dat de SQL-voorwaarde spiegelt. 2. De cancel-SQL miste Ops' type-guard. Replies krijgen bij insert de default `status='pending'`, dus workers kon een antwoord op `cancelled` zetten — een status die voor een reply niets betekent en die de s4m-queue CLI nooit produceert. Ops blokkeert dat; workers niet. Beide zijn rood bewezen door de implementatie te muteren, niet de assertie. De knopconditie krijgt een render-test: een test op alleen het predicaat zou groen blijven als de knop `isCancellable` negeert, en dat is precies de regressie die hier hoort te vallen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>De comment beweerde dat replies na een parent-delete blijven bestaan zonder parent-verwijzing. Dat is onwaar, en het beschreef een toestand die per constructie onmogelijk is. De FK `in_reply_to` staat inderdaad op ON DELETE SET NULL, maar `reply_link_matches_type` (CHECK: type IN ('result','data','reviewed') <=> in_reply_to IS NOT NULL) verbiedt een reply zonder parent. Die SET NULL is dus illegaal en Postgres weigert de héle delete — er verdwijnt niets. Nagemeten tegen het volledige Ops-schema (FK + alle vier CHECKs): delete verzoek MET replies -> ERROR reply_link_matches_type, 0 rijen weg CONTEXT: UPDATE ONLY agent_message SET in_reply_to = NULL WHERE ... delete verzoek ZONDER replies -> DELETE 1 delete reply zelf -> DELETE 1 purge hele thread -> 3 rijen weg clear met threads erin -> 3 rijen weg De laatste twee bevestigen meteen dat purge en clear niet op deze constraint stuiten: die nemen wortel en replies in één statement mee, waardoor de SET NULL alleen al-verwijderde rijen raakt en een no-op is. Pariteit met de bron is intact en dat blijft de kern: Ops zet geen `relationMode`, dus Prisma stuurt dezelfde kale DELETE naar dezelfde constraint en krijgt dezelfde violation. Alleen de verklaring klopte niet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>De vorige comment verwees naar een niet-gecommitte, niet-herhaalbare handeling ("gezaghebbend is de review-run"). Niets hield dat waar; over een maand staat er stellig proza dat niemand kan narekenen. Nu draait het als test. `describe.skipIf(!process.env.OPS_TEST_DATABASE_URL)`, conform `__tests__/mcp-tester/schema-snapshot.test.ts`: zonder env-var skippen we en blijft verify groen; mét env-var draait de echte selectie-semantiek. Het schema komt uit `fixtures/agent_message.sql`, een geverifieerd letterlijke kopie van Ops' migratie — niet uit het Prisma-model afgeleid. Dat was de val: Prisma-modellen dragen geen CHECKs, dus mijn eerdere fixture miste er vier, waaronder juist `reply_link_matches_type`. Meteen zichtbaar in wat nu wél kan: de test bewijst dat een reply zonder parent niet eens te insert'en is — een rij die in de oude fixture gewoon geldig leek, en die ik daar als scenario had staan. Guard: staat OPS_TEST_DATABASE_URL gelijk aan OPS_DATABASE_URL, dan gooit het bestand. Deze test DROPt agent_message; daar stil-en-groen op skippen zou de verkeerde kant op falen. Preamble van `ops-db-retention.test.ts` teruggebracht tot wat er hoort: mocks bewijzen structuur, niet uitkomst, en de gedeelde CTE is de garantie. Het review-narratief is weg. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>`countAgentMessages` filterde `open` alleen op status, niet op type. Replies dragen bij insert de default `status='pending'` — precies het feit waarvoor de type-guard in `cancelAgentMessage` bestaat. Op onze eigen fixture nagemeten: total=9, "open"=4, werkelijk lopende verzoeken=1. De bevestiging waarschuwde dus voor 4× zoveel verlies als er was, en de doc-comment zei "verzoeken die nog lopen" terwijl de SQL dat niet zei. Elke andere open-verzoek-check hier paart status mét type (cancelAgentMessage, isCancellable, CLOSED_THREAD_TARGETS_CTE, DetailPanel.canReply). Deze was de enige die het niet deed, en de nieuwste. Uitgerekend het getal waarvoor de dry-run bestond: een cijfer dat vertrouwd wordt en er 4× naast zit is erger dan proza, want proza wordt niet geloofd. Gesplitst in plaats van alleen gefilterd. Lopend werk en ongelezen post zijn verschillende verliezen: `openRequests` is agent-werk dat sneuvelt, `unreadReplies` is wat er nog in iemands inbox staat (niet geackt met `s4m-queue done`). Eén getal dat ze optelt zegt geen van beide, en `rows` minus `openRequests` zou de ongelezen antwoorden stilzwijgend wegmoffelen onder "verder alleen afgesloten rijen". Nu: rows=142 waarvan open verzoeken=7 waarvan ongelezen antwoorden=12 De waarschuwing noemt alleen `lopend agent-werk` bij openstaande verzoeken. De echte fout zat in de test. `ops-db-realdb.test.ts` asserteerde `{ total: 9, open: 4 }` en somde in de comment de drie replies op als verwacht: de test pinde de bug vast in plaats van hem te vangen. Daarom vonden de veertien rode mutaties dit niet — een mutatietest bewijst dat de code doet wat de test zegt, niet dat de test iets waars zegt. Nu `openRequests: 1, unreadReplies: 3`, met de reden erbij. Plus de wegwerp-guard: de vergelijking met OPS_DATABASE_URL is vacuüm in een gewone `OPS_TEST_DATABASE_URL=… npx vitest run` (setup.ts laadt geen .env) en was een rauwe string-compare, dus `?sslmode=require` liep eromheen. Nu eerst een positieve bevestiging op de databasenaam via URL-parsing; de oude check blijft als tweede linie. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>De volledige suite viel één keer om vlak na de mutatie-run; vier runs erna waren groen. Niet als ruis weggezet maar uitgezocht, en het was mijn eigen test. `bindt de cutoff als parameter` deed: const before = Date.now() const { cutoff } = await countClosed…(1) expect(before - cutoff.getTime()).toBeGreaterThanOrEqual(86_400_000) `retentionCutoff` leest de klok zélf, ná `before`. Dus `before - cutoff = DAG - (inner - before)`, en dat is alleen >= DAG als beide Date.now()-calls in dezelfde milliseconde vallen. Eén tik ertussen en de assertie valt. Deterministisch aangetoond met een gestubde klok: delta wordt 86399999. De test slaagde dus op geluk, en onder de load van de mutatie-run was dat geluk op. Nu een exacte bracket: de functie leest de klok ergens tussen `before` en `after`, dus de cutoff ligt tussen `before - dag` en `after - dag`. Geen wall-clock-marge meer (die 10s was ook maar een gok). Vangt nog steeds waarvoor hij bestaat — geverifieerd tegen de mutaties "uren i.p.v. dagen" en "cutoff geïnterpoleerd i.p.v. gebonden", allebei rood — en 20/20 stabiel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Verdict: REQUEST_CHANGES
geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.
Findings
lib/queue/ops-db.ts:475—clearAgentMessageQueue()gebruiktDELETE FROM agent_messageop een tabel met een self-referential FKON DELETE SET NULLplus de CHECKreply_link_matches_type. De patch erkent elders dat een parent-delete met replies daardoor faalt, omdat de FK-actie replies tijdelijk parentloos maakt. Een volledige queue met threads bevat precies zulke parent/reply-relaties; daardoor kan “Leeg queue” falen zodra er replies bestaan, terwijl de UI dit als gegarandeerde destructive clear aanbiedt. Gebruik hier een aanpak die de self-FK/CHECK niet triggert, bijvoorbeeld eerst tellen en daarnaTRUNCATE agent_messagein een transactie, of een bewezen child-first strategie die ook voor geneste replies werkt.Opmerkingen
De server-actions behouden de admin-guard en de UI gebruikt een expliciete confirmation flow met dry-run cijfers. De retentie-selectie en cancel-typeguard zijn goed afgedekt met unit-tests; de blokkade zit specifiek in de clear-implementatie rond databaseconstraints.