fix(attention): audit the replied stamps the broad resolver wrote - #1577
lilyshen0722 wants to merge 2 commits into
Conversation
Before the narrow resolver landed, any post by a recipient in a pod closed every open mention they held there. Rows stamped `resolvedBy: 'replied'` by that path can assert a reply nobody wrote, and they are the population the per-kind inbox counts will be computed over. `auditRepliedMentionAttention` re-reads each stamped row's source with the same evidence `sweepResolvedMentionAttention` uses, and reopens only the rows the source contradicts. Dry run by default, `--apply` to write, and `--resolved-before=<iso>` to bound the scan to stamps written before the narrow resolver deployed. Three outcomes, not two. A Mongo-fallback source persists no reply or thread edges, so absence of evidence there is not evidence the recipient stayed silent — reopening those would re-raise attention a user has already dealt with. They are counted as `undecidable` and left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE: clear at acf4638a — one non-blocking gap, and it is the same shape as the three this stack has already produced.
I mutated rather than read. Four deciding lines in auditRepliedMentionAttention, each one red on its own:
| mutation | result |
|---|---|
drop resolvedAt: { $lt: before } |
1 red |
drop if (!apply) continue (dry run writes) |
1 red |
reopen Mongo-sourced rows instead of counting undecidable |
1 red |
reduce the update filter to { _id } (drop the re-stamp guard) |
1 red |
30 tests green across the service and PG model suites at this head, 21 in the service file.
The design reads right to me on the two things that matter most. Three outcomes rather than two is the correct call: a Mongo-fallback source carries no reply edges, so undecidable is an honest answer where reopened would be a fabricated one, and the audit reopening rows on absent evidence would be the same overreach in the other direction as the stamps it is auditing. And repeating the stamp in the updateOne filter closes the read-write window against an operator acknowledging a row mid-run.
The gap: the script's own argument parsing has no test
backend/scripts/audit-replied-attention-stamps.ts:29-33 carries the guard AND names the hazard in its comment: "An unparsable bound would silently widen the audit to every stamp ever written, which is the opposite of what the flag was passed to do." That is correct, and nothing pins it — no test references the script, so deleting the Number.isNaN throw leaves everything green.
The chain I verified rather than assumed: new Date('garbage') → Invalid Date → validDate returns null → before is falsy → the resolvedAt clause is spread away → the audit scans every replied stamp ever written. With --apply, that is an unbounded reopen from a typo'd flag.
There is precedent for pinning this in-repo (__tests__/unit/scripts/migrate-connector-gates.test.js), so it is a small ask: one test that imports the script with a bad --resolved-before and expects a throw, plus one that confirms a valid bound reaches the service.
Not blocking — the guard exists and is correct, and this is a maintenance script an operator runs deliberately. But it is the producer half of exactly the pattern you have now caught three times on this stack (#1579's manifest, #1582's reveal branch, #1582's collapsed PUT): the value's consumer is covered, the code that computes it is bare. I would rather name it on your PR than only on mine.
I did not run the audit against production; the dry run is yours to call or Sam's.
The flag's whole purpose is to NARROW the scan to the cutover window. An unparsable value fell through as `undefined`, the bound was spread away, and the audit scanned every replied stamp ever written — under `--apply`, an unbounded reopen from a typo. The comment named the hazard; nothing tested it, and deleting the throw left the suite green (lily-shen on #1577). Argv parsing moves into an exported `parseArgs` so the throw is reachable without a database. Deleting it now gives 3 red. One test written here was deleted rather than kept: no valid date contains an `=`, so `split('=')[1]` versus `.slice(1).join('=')` changes nothing a test can observe, and the case passed against both implementations. A test that cannot fail is a claim about nothing; the note in its place says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Corrective pass for the mention rows stamped
resolvedBy: 'replied'by the pre-#1573 resolver, requested in the pod before PR 3 renders them as per-kind inbox rows.What it does
auditRepliedMentionAttentioninattentionItemService.tssits besidesweepResolvedMentionAttentionand runs its test in reverse: for each row already stampedreplied, re-read the source and reopen it only if the source contradicts the stamp.Dry run is the default and prints
{ scanned, kept, contradicted, reopened, undecidable, unavailable }.Three outcomes, not two
recipientRepliedAfterMentiononly has reply and thread edges for a PostgreSQL source. For a Mongo-fallback source it returnsfalseunconditionally — so a two-way kept/reopen split would reopen every Mongo-sourced row, including the ones that were correctly resolved, re-raising attention a user has already dealt with. Those are counted asundecidableand left untouched. A test pins that they are never written and thathasReplyByUserAfteris not even called for them.--resolved-beforebounds the scan to the stamps the broad resolver could have written. An unparsable value throws rather than silently widening the audit to every stamp ever written.The reopen re-states
status: 'resolved', resolvedBy: 'replied'in the update filter, so a row acknowledged between the read and the write is not reopened underneath the operator.Tests
Five new cases in
backend/__tests__/unit/services/attentionItemService.test.js; suite is 21/21.Each was mutated against the diff:
if (false))recipientRepliedAfterMentionpolarity invertedif (!apply) continuedeletedresolvedAt: { $lt: before }dropped from the filtertsc --noEmitreports no errors in either changed file (the 51 it does report are pre-existing, incleanup-test-data.tsandtest-discord-*.ts). eslint could not run locally —@typescript-eslint/parseris not installed in this workspace — so the backend TS lint gate is unverified here and left to CI.Note for the reviewer:
attentionItemService.tsends with an explicitexport default {...}andmodule.exports = {...}enumeration. A new export is invisible to every CommonJS consumer until it is added to both; the first test run failed withis not a functionfor exactly that reason.🤖 Generated with Claude Code