Repository navigation
fix(admin-digest): fail closed when the super-admin recipient list cannot be read - #685
Conversation
…nnot be read
The digest's recipients are the code/env allowlist plus every super_admins row.
The lookup read the table with a bare `const { data } = await ...`: a PostgREST
error resolved to "no rows", a thrown query hit an empty catch, and the digest
went out to the allowlist alone and answered 200 ok. An admin who exists only in
the table silently stopped receiving it (characterized in #677; its contract
case C6).
- lib/feedback/notify.ts: new readSuperAdminRecipients returns the allowlist ∪
the table, or an error for a failed, thrown or truncated read (readAll with a
1,000-row ceiling, ordered by the primary key). An empty table is NOT an error:
the configured allowlist is then the whole list.
- The admin digest uses it and answers 502 {ok:false,
adminDigest.recipientsUnavailable} before sending anything.
- allSuperAdminEmails keeps its best-effort fallback for its only other caller,
notifySuperAdmins (alert emails still reach the allowlist rather than nobody),
but now logs the failed read instead of swallowing it.
- New key adminDigest.recipientsUnavailable in en-US and the six full catalogues.
- tests/a-paged-read-needs-a-total-order: super_admins' key registered
(0008: `email text PRIMARY KEY`).
Regressions: tests/admin-digest-recipients-fail-closed.test.ts (6) and
tests/super-admin-recipients-read.test.ts (10). Against the previous source 10
of them fail (the three fail-closed route cases and seven reader/log cases); the
three unchanged-behaviour cases pass on both. No live email, configuration or
migration. Duplicate delivery (#677 C1-C5) is untouched and still unresolved.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
… for the log only tests/the-database-does-not-talk-to-the-browser flagged `error: error.message` in readSuperAdminRecipients' result: a raw database message under an `error` key is one refactor away from a response body. The result now carries a classification (`read_failed` | `truncated`) and the raw cause under `cause`, which only the two server-side log lines read. Behaviour is unchanged: the digest still answers 502 with the translated message and sends nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
NewWorldVenture
left a comment
There was a problem hiding this comment.
Please prepare the combined #677/#685 test acceptance before integration; #677 exact e90fa5a is the independently reviewed characterization candidate.
Beyond changing the two recipient-read failure expectations from fallback200 to502 with zero sends, the new strict reader changes two harness contracts: (1) #677 mocks only allSuperAdminEmails, while the route now calls readSuperAdminRecipients; adapt the empty-recipient override to the strict {emails: [], failure: null} shape so that case still exercises the intended branch. (2) Paged recipient lookup reads the one-row page plus an empty page per call, so the concurrent test's barrier.arrived === 2 assumption becomes4. Synchronize the first page of each invocation without pinning the total number of paged reads. Preserve meaningful replay/concurrency send assertions and the dated original evidence.
Promote only the reproduced C6 fail-closed contract after exact combined-source checks. Keep C1-C5 replay/receipt/idempotency gaps explicit; no live email or provider-delivery claim. Do not weaken the desired contract merely to fit the current sender.
The seven new catalogue-key additions also need comparison with NWV's unpublished I18N-001 catalogue work before merge. Preserve both variants; no catalogue overwrite is authorized by this review. Source/CI review continues separately.
…il-closed fix for combined acceptance Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
…ent cases, adapt the harness, promote C6 only Owner review on #685: prepare the combined test acceptance before integration, with #677 at exactly e90fa5a (merged in the previous commit). tests/admin-digest-replay-contract.test.ts on the combined source: - the two super_admins read cases (error, throw) move from "200 ok, built-in admin only" to 502 adminDigest.recipientsUnavailable with zero provider attempts; the pre-#685 observation stays in the dated evidence; - the empty-recipient override also answers readSuperAdminRecipients (the reader the route now uses) in its success shape {emails: [], failure: null}, so the empty-recipient branch is still the one exercised; - recipient reads are paged since #685 (one-row page + empty page per call), so the concurrency barrier holds only the FIRST page of each invocation instead of pinning the number of paged reads; replay/concurrency send counts (2 per admin) are unchanged; - C6 is promoted (it failed on 231e814 and passes with #685). C1-C5 stay in the evidence folder as unmet contract cases. Evidence folder: the dated 231e814 outputs are unchanged; combined-source outputs are added (characterization 20/20; contract 5 failed / 2 passed before C6 was moved), C6 is replaced by a pointer in desired-contract.test.ts, and the README gets a dated addendum. Duplicate delivery is not fixed; no live email. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
|
The combined #677/#685 acceptance you asked for is pushed. Head is Commits
Harness changes (
Contract status
Local runs
i18n: the seven Generated by Claude Code |
NewWorldVenture
left a comment
There was a problem hiding this comment.
Catalogue scope review is clear for the published source. Parsed all seven full JSON catalogues against b090aeb/current main and published #674. Every existing 16,607 key/value pair survives unchanged in each locale, with no removals, value changes or duplicate keys. This candidate adds only adminDigest.recipientsUnavailable in de-DE, en-US, es-ES, fr-FR, it-IT, nl-NL and pt-PT.
#674's three additions (coach.onlyYourOwnHealth, medicationsModule.noMedicinesPrescribedToYou, medicationsModule.yourOwnMedicinesOnly) are disjoint. Their computed union preserves both candidates' values. Sharing catalogue filenames is therefore not a demonstrated semantic conflict and is no longer a broad integration hold for this fix.
NWV's unpublished catalogue delta was not available for comparison. Preserve that WIP and this additive key during its later port; no unpublished content was overwritten or claimed reviewed. The current #677 adaptation review, exact-head CI and combined-main tests remain the acceptance gates.
|
Independent source and synthetic regression review of exact head I reproduced the prior route behavior using the exact current-main The source now classifies read failures without exposing their raw cause in the response and refuses to send a partial digest. The existing best-effort alert caller retains its separate logged fallback. The #677 harness uses the strict reader shape and synchronizes each invocation's first page without changing the replay send assertions. C1–C5 remain unresolved; this does not establish idempotent delivery or provider acceptance. Hosted CI36764530456 completed all four jobs successfully. Its E2E result is 1,815 passed plus one flaky retry recovery, not 1,816 clean first-pass successes: An initial local configuration-loader setup error and two inherited Windows single-quoted |
…very engine Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
Small source fix. It touches no migration, configuration or schedule, sends no live email, and does not edit
finalaudit.md. Duplicate delivery (#677 C1–C5) is not addressed here and stays unresolved until the durable scheduling design lands. No in-memory dedupe was added.The defect
The admin digest's recipients are the code/env allowlist plus every
super_adminsrow.allSuperAdminEmailsread the table with a bareconst { data } = await …:catch.Either way the digest went out to the allowlist alone and answered 200 ok. An admin who exists only in the table silently stopped receiving it. This was characterized in #677, and its contract case C6 is the same requirement.
The fix
lib/feedback/notify.ts: newreadSuperAdminRecipients. It returns the allowlist plus the table (lowercased, de-duplicated), or a classified failure:read_failedfor an error or a throw;truncatedwhen there are more than 1,000 rows (readAll, ordered by the primary key).An empty table is not an error: the configured allowlist is then the whole, legitimate list. The raw cause is kept apart, for the server log only.
app/api/cron/admin-digest/route.tsuses the new reader. On any failure it answers502 { ok: false, error: adminDigest.recipientsUnavailable }before sending anything. A quiet day still answers before reading recipients at all.allSuperAdminEmails(callers checked by repository grep: the digest andnotifySuperAdmins, nothing else) keeps its best-effort fallback fornotifySuperAdminsalert emails, so they still reach the allowlist rather than nobody. It now logs the failed read instead of swallowing it.New i18n key
adminDigest.recipientsUnavailablein en-US and the six full catalogues. The regional overlays inherit it.tests/a-paged-read-needs-a-total-order:super_admins' key is registered there (0008:email text PRIMARY KEY), as that guard instructs.Regressions, and proof they fail on the old code
tests/admin-digest-recipients-fail-closed.test.ts(6 tests). It drives the real route over the in-memory database, withfetchstubbed as the email provider. It covers:tests/super-admin-recipients-read.test.ts(10 tests) covers:allSuperAdminEmails/notifySuperAdmins: still best-effort, and now logged.notify.tsandroute.ts, 10 of the 16 fail: the three fail-closed route cases, and the seven reader and log cases. The 6 behaviour-preserving cases pass on both old and new code.Verification (local, on
6418c2a9; merges cleanly with current main6511f6c8)tsc --noEmitis clean.npm run lint: 0 errors, and the 15 warnings are pre-existing. ESLint on the new test files is clean.tests/the-database-does-not-talk-to-the-browserflagged a rawerror.messagein the first version's result. The result was changed to a classification plus a log-onlycause; the guard was not exempted.Ordering note with #677
#677 pins today's behaviour as characterization, including "a
super_adminsread error is swallowed: 200 ok" (2 cases), and keeps C6 as a failing contract case. Whichever PR lands second needs those two cases re-pinned to 502 with nothing sent, and C6 moved intotests/. This PR does not edit #677's files.Limits
CRON_SECRETis recorded as unset, so the route currently answers 401.API-96040DFB5635 and JOB-EF2453D9F633 stay open: duplicate delivery and the durable scheduler remain.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KP9rt5rVQ9jDMpZp2xBy3K
Generated by Claude Code