Conversation
VerificationAdversarial pass by a fresh run at head Arms, run by me$ # HEAD arm
$ cd apps/api && bun x vitest run test/modules/mail/mail-delivery.service.test.ts
Test Files 1 passed (1)
Tests 52 passed (52)
$ # BASE arm: production file at 4ad53d8a, test file at 1c1504a1
$ git checkout 4ad53d8a -- apps/api/src/modules/mail/mail-delivery.service.ts
$ bun x vitest run test/modules/mail/mail-delivery.service.test.ts --reporter=verbose
× parseMailQueue > keeps every distinct reason in the reading it returns
→ expected [ { kind: 'rejected', …(2) }, …(2) ] to have a length of 6 but got 3
× parseMailQueue > caps the reasons it reports
→ TypeError: (0 , __vite_ssr_import_1__.topDeferrals) is not a function
× checkMailDelivery > fails a relayed box on an auth refusal ranked below the display cap
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > fails a relayed box on a TLS failure at the smarthost ranked below the display cap
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > fails a relayed box on a connection failure at the smarthost ranked below the display cap
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > fails a relayed box on a refusal ranked far below the display cap
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > fails a relayed box on a refusal queued after three other one-off deferrals
→ expected 'warn' to be 'fail' // Object.is equality
Test Files 1 failed (1)
Tests 7 failed | 45 passed (52)
$ git checkout HEAD -- apps/api/src/modules/mail/mail-delivery.service.ts # cmp against saved head copy: identicalControls (pass on both arms, marked in the body's Test evidence table): The hole, and its fixtureAt Mutants at 1c1504a (kill set = the exact failing tests, 52 run each)
No mutant survives; no mutant is killed by everything. Other checks
Rules: mutate-the-rejected-alternatives=covered(fails a relayed box on a refusal ranked far below the display cap) | moved-transform-test-enters-above=covered(fails a relayed box on a refusal ranked far below the display cap) | dispatch-arm-boundary-coverage=covered(fails a relayed box on a connection failure at the smarthost ranked below the display cap) | ledger-row-needs-its-fixture=covered(fails a relayed box on a refusal queued after three other one-off deferrals) | base-arm-revert-committed=covered(git diff 4ad53d8..1c1504a on the production file is +28/-2) | run-every-ci-step-not-just-the-red-one=covered(tsc rc=0 plus fork CI run 35806569432 success) | no-control-cases-in-the-suite=covered(no test name carries control or patch narration; controls marked in the body table) | formatter-at-the-pinned-version=covered(prettier 3.8.1 = repo pin, drift confined to base lines) | prior-art-recheck-at-gate=unreachable(hunt re-checked at gate 00:5xZ against origin/main d03378b, both files byte-identical; not repeated within the hour) | control-returns-its-own-input=unreachable(no fixture asserts its own input; the controls assert a verdict string) | idempotence-test-asserts-only-agreement=unreachable(no test compares two invocations) | unreachable-row-same-bytes=unreachable(no ledger row is argued unobservable) | crossing-gated-fix-all-controls=unreachable(no search-based identity detector in the diff) | timeout-reintroduces-bug=unreachable(no timeout or retry in the diff) | shared-ref-cancellation=unreachable(synchronous pure functions) | static-row-vs-alias-stub=unreachable(no static rows; the real module is loaded) | run-the-artefact-the-fix-produces=unreachable(the artefact is the response object, asserted directly) | reads-as-generated=covered(test additions +140 vs production +28, in the file's it.each idiom; helper comment removed) |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
REQUEST CHANGES — the candidate is not ready for operator submission because the added test commentary contains generated-patch tells that upstream maintainers are explicitly likely to reject. rule:reads-as-generated
Blocking — generated-patch narration
apps/api/test/modules/mail/mail-delivery.service.test.ts:417-421
* The refusal that condemns the box is routinely the RAREST line in the queue — * one message has reached the relay and been refused while dozens of older ones * defer against ordinary receivers — so it sorts below the display cap. Both * halves of the verdict meet here: it is taken from the whole queue, while the * panel still renders at most MAX_DEFERRALS rows.
The new test-local commentary uses em dashes and narrates why this patch is correct rather than documenting a durable test fixture. The same generated-style narration appears in the added inline comments at lines 446-447 and 471-472. OSS candidate hygiene prohibits an em dash anywhere in added tests/comments and patch-history/explanatory commentary in the test source. This is a maintainer-readability failure independent of whether the behavior is correct; delete these comments (the test names and assertions already express the behavior) or restate only essential fixture semantics in the file's established terse idiom.
const crowdedQueue = (last: string) =>
What's good: I traced the base implementation: its parseMailQueue sliced before gradeDelivery inspected queue.deferrals, so a fourth-ranked fatal relay reason produced warn. The changed response grades the complete queue and caps only the returned display rows. The candidate supplies a complete facts sheet, discriminating base/head evidence, green fork CI (including API tests and typecheck), and no matching open upstream PR or issue in my independent searches. I did not run the local suite, per review environment policy.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
The functional fix looks correct, but this candidate is not ready under the required submission tell pass: trim the new explanatory scaffolding before upstream submission.
Finding
Low | Submission tell pass: new comments narrate the patch and explain the assertions instead of documenting a contract.
Exact changed locations:
apps/api/src/modules/mail/mail-delivery.service.ts:150-153:// Grade the WHOLE reading, then cap for display. The other order letfollowed by the historical fourth-reason failure narrative. This also describes an execution order the object literal does not use:topDeferralsruns beforegradeDelivery, safely becausesliceis non-mutating.- Same file,
:204-207:* EVERY distinct reason is returned, ordered but not truncated:followed by another argument for the fix;:267-270:* The deferral rows we show, most frequent first — the display cap, applied to aand* for the operator, but it is NOT the same story for the verdict;:288-289:* Takes the reading ... NOT the capped display list. These repeat the rationale at four neighboring sites; the new helper documentation also adds an em dash. apps/api/test/modules/mail/mail-delivery.service.test.ts:151-152:* The reading is what ... judges, so it holds every distinct reason./* Truncating here is what hid a fatal refusal behind three commoner ones.This narrates the patch's history.- Same test file,
:416-422: JSDoc on the test-localcrowdedQueuehelper, including* The refusal that condemns the box is routinely the RAREST line in the queue —and another em dash at:419. - Same test file,
:177:// Most frequent first, and the order the reading already had.;:446-447:// Still three rows on the panel, and the refusal is not one of them —;:471-472:// Every reason once, so nothing outranks anything ...;:494:// Ranked below the cap or not, a deferral is fatal only on the relay's own host.These restate the adjacent fixtures/assertions or argue their correctness. The last comment is also too broad: the auth arm does not require host matching. - Commit
e00daaefmessage containsThe cap is a display limit —andwrong — every relayed message dying —; commit65e34400containsBoth refusals ... — a SASL rejection and a/TLS failure naming the smarthost —. These fail the required commit-message tell check too.
The failure scenario here is submission/readability, not a runtime defect: a maintainer must read repeated change advocacy surrounding a small transformation move. The required tell pass explicitly rejects new em dashes, patch-history narration, assertion-restating comments, and JSDoc on test-local helpers. Existing upstream comments do contain similar punctuation and prose; I am flagging the new additions under the candidate submission check, not claiming upstream bans that punctuation. The relevant upstream quality bar is CONTRIBUTING's scoped-diff and no-test-spam guidance.
Suggested fix
Remove the new test-local JSDoc and assertion/fixture narration.
Keep one short contract statement: the parser returns all observed reasons;
the response exposes at most three. Remove repeated historical explanations.
Remove the added em dashes from code/comments and submitted commit messages.
Keep the behavioral fixtures and assertions.
Independent correctness and boundary read
I can confirm the bug from the base code without relying on the reported test transcript: with three distinct ordinary refusals followed by an equally frequent auth refusal, stable sorting leaves auth fourth; base slices it off, so the relay's fatal predicate sees only rejected reasons and returns warn. Head keeps it for grading and still returns the original three display rows. Auth, TLS-at-relay, and network-at-relay each follow that reachable path. There is no newly introduced runtime guard; the changed production boundary is the placement of slice(0, MAX_DEFERRALS).
Rebuilt boundary ledger from the diff:
| Changed expression / surface | Boundary and fixed behavior | Test pin / limitation |
|---|---|---|
Removed parser slice(0, MAX_DEFERRALS) |
0 reasons remains empty; 1/2/3 retained; 4 and beyond retained instead of capped | Existing empty/one-reason coverage; new six-reason parser assertion; four- and thirteen-reason end-to-end fixtures |
topDeferrals: slice(0, MAX_DEFERRALS) |
Length 0 returns empty; below 3 retains content; exactly 3 retains all; 4/6/13 returns first 3, without mutating source | Empty response baseline, last-shown-row case, six-reason helper test, crowded/far-below cases; no new dedicated two-element assertion |
Response cap plus gradeDelivery(queue, relay) |
Auth at index 2 still fails; index 3 or 12 now fails; equal counts preserve insertion order | Last-shown-row, each refusal variant, far-below, and four one-off tests |
| Longer list reaching fatal classification | auth/TLS/network at relay fails; wrong-host network, empty host with TLS, and direct auth remain warn | Three positive parametrized cases and three only warns cases |
| Non-mutating slice at response edge | Cap evaluates first but cannot shorten the grader's input | Each beyond-cap status === "fail" assertion rejects in-place truncation |
Added test index i % 12 |
i=0 through 23 creates twelve reasons twice, auth is thirteenth | Queued=25, fail status, length=3, every displayed count=2 |
Added test index [2]?.kind |
Exactly last allowed row contains auth | Last-shown-row assertion |
Added test predicate d.count === 2 |
All three displayed fillers have count 2 | Far-below test; no zero/negative count is generated by this fixture |
null, undefined, strings, negative lengths, and a configurable maximum are not admitted inputs to the new typed array helper; the production parser supplies an array after its existing null guard. Empty raw output remains an unreadable probe, not a helper input. Container/host command selection is unchanged, with the same parse/grade/response seam. Very large arrays retain all parsed reasons for grading and only three for display; I did not execute stress tests.
I read every added assertion by parameter variant. Each positive auth/TLS/network variant's status assertion distinguishes base from head. Queue totals and display-length assertions are preservation checks, not independent reproduction evidence. All three only warns variants and the last-shown-row case pass without the fix; they protect separate negative gates/boundaries, rather than proving this bug. The helper cap assertion fails against base because the export is absent, not because it independently reproduces the verdict error. The body accurately labels controls, although its prose says three where the table has four. Do not present those as four additional bug discriminators.
Maintainer context and what's good
- Reuses
queueOutput,box, the existing test module, and itsit.eachpattern. The new production helper is small and non-mutating; the response shape and grading rules stay unchanged. - Read the module's recent history: 6143f580 and 698893ec. The parser/grader separation already exists; this change restores that separation rather than introducing another grading abstraction.
- Reviewed recent merged outside-contributor PR context for #910, #909, and #898. Titles vary;
fix(mail): ...is consistent with the conventional scoped title in oblien#898. Their body formats are operator context, not findings against this fork description. No evidence from this read warrants requiring a new issue or changelog entry for this small bug fix. - Independently repeated upstream PR searches for
deferrals,MAX_DEFERRALS, andmail-delivery: zero matches for the first two; the latter returns nearby work including oblien#885 and oblien#423, not this cap relocation. This was a search check, not an exhaustive proof of no overlapping work. - All nine reported CI jobs pass at
1c1504a195449ff643162bb088d4bc52d718e03b. I read both changed files' diff and base module context; I did not run tests, clone the repository, or read the gating review. The posted A/B and mutation results are author/verification evidence, not executions I performed.
SECOND READ: NOT READY — Remove the new comment/helper-documentation and commit-message tells identified above before upstream submission.
…ree reasons parseMailQueue capped its reason list at MAX_DEFERRALS before returning, and checkMailDelivery graded that capped list. A relay refusal ranked below the three most common reasons never reached gradeDelivery, so a relayed box whose SASL credentials, TLS or egress to the smarthost were broken graded warn instead of fail. Return every distinct reason from the parse, grade that, and apply the cap in topDeferrals when building the response. The response still carries at most three rows.
1c1504a to
129e413
Compare
ReworkAnswers Redline's CHANGES_REQUESTED and the Second Read's NOT READY, both at What changed
Measured at $ # BASE arm (production file at 4ad53d8a)
× parseMailQueue > caps the reasons it reports
→ expected [ { kind: 'rejected', …(2) }, …(2) ] to have a length of 6 but got 3
× checkMailDelivery > grades an auth refusal ranked fourth
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > grades a TLS failure at the smarthost ranked fourth
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > grades a connection failure at the smarthost ranked fourth
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > grades an auth refusal ranked thirteenth
→ expected 'warn' to be 'fail' // Object.is equality
× checkMailDelivery > grades an auth refusal queued after three one-off deferrals
→ expected 'warn' to be 'fail' // Object.is equality
Tests 6 failed | 45 passed (51)
$ # HEAD arm
Tests 51 passed (51)Controls (pass on both arms): Mutants (kill counts): cap 4 in parse 2, cap 8 in parse 1 (
|
Verification at 129e413Second adversarial verification of this branch, a fresh run in a fresh worktree ( What the rework changed
Test file: the 13 separate cases at Head arm (fix in place)$ cd apps/api && bun x vitest run test/modules/mail/mail-delivery.service.test.ts --reporter=verbose
Test Files 1 passed (1)
Tests 51 passed (51)Base arm (production file reverted to 4ad53d8, tests kept)$ git checkout -q 4ad53d8a -- src/modules/mail/mail-delivery.service.ts
$ bun x vitest run test/modules/mail/mail-delivery.service.test.ts --reporter=verbose
× parseMailQueue > caps the reasons it reports 17ms
× checkMailDelivery > grades an auth refusal ranked fourth 6ms
× checkMailDelivery > grades a TLS failure at the smarthost ranked fourth 2ms
× checkMailDelivery > grades a connection failure at the smarthost ranked fourth 2ms
× checkMailDelivery > grades an auth refusal ranked thirteenth 1ms
× checkMailDelivery > grades an auth refusal queued after three one-off deferrals 1ms
Test Files 1 failed (1)
Tests 6 failed | 45 passed (51)
$ git checkout -q HEAD -- src/modules/mail/mail-delivery.service.ts && cmp src/modules/mail/mail-delivery.service.ts <saved head copy>
RESTORED-IDENTICALSix discriminating tests, four controls ( Mutants (rebuilt from the saved head copy, run against the 51 tests, source restored and
|
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Request changes: the production fix is correct, but four added table variants fail the candidate's regression-test gate. rule:none
Blocking: four added variants pass without the fix
apps/api/test/modules/mail/mail-delivery.service.test.ts:444-485 adds these variants:
"an auth refusal on the last shown row",
"a network failure at another host ranked fourth",
"a TLS failure ranked fourth with no relay host",
"an auth refusal ranked fourth on a direct box",The shared assertions at lines 484-485 are:
expect(health.status).toBe(status);
expect(health.deferrals.map((d) => d.reason)).toEqual(shown);Each variant produces exactly those results on the base implementation as well. The last-shown-row fixture has only three distinct reasons, so the old parser retains the auth refusal. In the other three fixtures, the old parser drops the fourth reason and returns warn; the fixed grader also returns warn because respectively the host differs, the relay host is empty, or the relay is absent. Both versions return the same first three reasons. The supplied before/after evidence explicitly confirms these four passes on both arms.
The OSS candidate gate requires added variants to discriminate between base and fix. The five failing integration variants already check exact response rows. Keep the added integration table focused on those regression variants; retain the other boundary measurements in the evidence sheet.
Remove the four base-passing rows from the new it.each table, retaining the five regression rows and their status and response-row assertions.
What's good
I traced the reproduction through the upstream base file: sorting and slicing before grading really does hide a fatal fourth reason. Moving the slice to the response preserves the display contract while grading the complete parsed reading; slice does not mutate that reading. The auth, TLS, network, tied-count and thirteenth-position regression fixtures exercise the real parser-to-grader route using the existing test harness.
Reviewed the complete two-file diff, surrounding parser/grader and test context, commit metadata, facts sheet, current-head verification, and upstream contribution policy. Repeated upstream PR searches for deferrals, mail-delivery and gradeDelivery and issue searches for deferrals/mail-delivery; no duplicate fix surfaced. All nine fork CI checks pass. I did not run local tests or test against a live Postfix/relay server.
Notes for the operator
Use the upstream PR template and preserve the actual before/after output and disclosure facts. The evidence states that the full monorepo suite was run by fork CI rather than locally; keep that distinction explicit when addressing the contribution guide's local-check instructions.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: Head 129e4131 fixes the bug correctly. The tells I flagged at 1c1504a1 are gone. The nine-row table pins every reachable boundary I rebuilt from the diff. No blocking issues. Two low, non-blocking notes follow.
Bug confirmed from base
I confirmed the bug by reading the base code. On base, parseMailQueue ran .slice(0, MAX_DEFERRALS) before it returned (the removed line in the mail-delivery.service.ts:246 hunk). checkMailDelivery then passed that capped list to gradeDelivery, whose queue.deferrals.some(...) (:281) never saw a fourth-ranked reason. Example: on a relayed box, a queue of greylist x3, 451 x2, 452 x2 and one 535 AUTH refusal graded warn on base. At head it grades fail. The fix removes the slice from the parse, grades the full list (:154) and applies the cap only to the response (:153, topDeferrals at :255). parseMailQueue has no other caller (git grep finds only :146). The MailDeliveryHealth.deferrals doc ("Capped.", :62) still holds.
Break-it pass (ledger rebuilt from the diff)
| # | Predicate / expression | Input | Fixed code does | Pinned by |
|---|---|---|---|---|
| 1 | topDeferrals slice(0, 3) |
empty list | [] |
"reports a healthy direct sender" (deferrals: []) |
| 2 | same | 1 row | 1 row | "reads the send hop from the state file..." |
| 3 | same | exactly 3 (limit), fatal row in 3rd place | 3 rows incl. AUTH, fail |
grades an auth refusal on the last shown row |
| 4 | same | 4 (one past), all ties at count 1 | first three by stable sort, AUTH hidden, fail |
grades an auth refusal queued after three one-off deferrals |
| 5 | same | 4, fatal ranked 4th by count | 3 rows, fail |
grades an auth refusal ranked fourth |
| 6 | same | 13 distinct (far past) | 3 rows, fail |
grades an auth refusal ranked thirteenth |
| 7 | parse no longer caps | 6 distinct | parse returns 6, topDeferrals returns first 3 |
"caps the reasons it reports" (test:159-160) |
| 8 | gradeDelivery tls branch on the uncapped list |
TLS at smarthost ranked 4th | fail |
grades a TLS failure at the smarthost ranked fourth |
| 9 | network branch, same | network at smarthost ranked 4th | fail |
grades a connection failure at the smarthost ranked fourth |
| 10 | host match, other path | network at another host ranked 4th | warn |
grades a network failure at another host ranked fourth |
| 11 | !!host with empty host |
host: "", TLS ranked 4th |
warn |
grades a TLS failure ranked fourth with no relay host |
| 12 | relay?.enabled other path |
relay undefined, AUTH ranked 4th |
warn |
grades an auth refusal ranked fourth on a direct box |
| 13 | unread / null parse | probe error, non-queue output | unchanged unknown path, never reaches topDeferrals |
existing "reports unknown..." tests |
I found no reachable row without a pin.
Tie handling (rows 3 and 4) relies on Array.prototype.sort being stable. That is guaranteed since ES2019, so the expected shown arrays are deterministic.
Per-row assertions. Rows 4, 5, 6, 8 and 9 fail on base on status (base grades warn). Rows 3, 10, 11 and 12 give the same status on both arms. That matches the four controls in the body table. The shown assertion gives the same result on base and head in every row, because base capped the list the same way. Its job is to catch a fix that drops the response cap, or one that moves a fatal-kind row into the top three. Rows 10 and 12 put a fatal-kind reason fourth in a non-fatal context, so they catch the second of those. No row asserts something that holds under every plausible implementation.
Size. The test hunk is +100 against +11 in prod. Each of the nine rows maps to a distinct ledger row above. Most of the extra lines come from prettier wrapping the data rows and from five new reason constants in the file's existing AUTH_REFUSAL / GREYLIST_REFUSAL style.
Tell pass
I found no tells in the diff, the commit message or the title. I grepped the added lines for em and en dashes, (control), "before the fix", "previously", "ensure", "gracefully", "robust", "leverage", "Note that", "Importantly", "for clarity", setTimeout and sleep: zero hits. The one new prod comment (:254) is a contract line in the file's JSDoc-on-every-export idiom. The test-local helpers crowdedQueue, crowdedRows and tryLater are short and undocumented. The table drives the real checkMailDelivery through the file's existing box() executor. grades %s matches the file's own refuses to read %s as a queue / classifies %s as %s shape. The commit body opens with the mechanism, not "This change".
Findings (non-blocking)
Low: apps/api/test/modules/mail/mail-delivery.service.test.ts:160, non-null assertion is new to these tests
expect(topDeferrals(parsed!.deferrals)).toEqual(parsed!.deferrals.slice(0, 3));This is the only !. in the 35 test files under apps/api/test/modules/mail/. The rest of this file reads the parse result with parsed?.. It is harmless, but a maintainer reading the hunk will see the change in idiom.
Suggested fix:
const deferrals = parsed?.deferrals ?? [];
expect(deferrals).toHaveLength(6);
expect(topDeferrals(deferrals)).toEqual(deferrals.slice(0, 3));Low / maintainer question: the verdict can now come from a row the panel does not show
mail-delivery.service.ts:153-154:
deferrals: topDeferrals(queue.deferrals),
status: gradeDelivery(queue, relay),Take row 5. The pill reads fail, but the three rows listed are greylist, 451 and 452. The relay-auth remedy hint only renders for a displayed row (health-tab.tsx:707 maps delivery.deferrals, and :766 picks d.hint.authRelay per row). The operator sees red with no row explaining why. This is still strictly better than base, which showed amber for a dead relay. The PR is scoped to the verdict and says the response still carries at most three rows, so I don't count this as a finding against the diff. Expect the maintainer to ask whether the row that drives the verdict should always be visible. If they do, one small follow-up would be for topDeferrals to keep the first fatal row in place of the last slot. That needs the relay, so it is a signature change and belongs in a separate round.
Maintainer's-eye notes
- Upstream history of the touched files.
gh api repos/oblien/openship/commits?path=...returns only two maintainer bulk commits for each file (6143f580"ship sdk, intial scale panel",698893ec"patch 0.6.2"). There is no per-module churn to align with, so the file's own idiom is the reference, and the diff follows it. - Scope. Two files, one fix. This matches CONTRIBUTING's "One change per PR" / "Scope the diff" (CONTRIBUTING.md:44-47). I saw no reformatting of unrelated lines in the diff.
- Issue first. Not needed for this change. CONTRIBUTING.md:24 says bug fixes are welcome as direct PRs.
- Changelog.
CHANGELOG.mdexists, but none of the last merged PRs I checked (oblien#936, oblien#930, oblien#925) touch it. Maintainers appear to curate it themselves, so no changelog line is expected. - Title.
fix(mail): ...matches upstream's conventional-commit titles (e.g. oblien#932fix(notifications): ..., oblien#60fix(mail): detect duplicate SPF records in DNS scan). - Test policy. CONTRIBUTING.md:50-57 asks for a test that fails without the change and warns against tests that "can't fail at all". Five of the nine rows fail on base, and the ledger above gives each of the four controls a distinct failure mode.
- Prior art, re-run. Upstream PR and issue searches for
MAX_DEFERRALSandgradeDeliveryare empty.deferralandmail deliveryreturn only unrelated items (oblien#477 amavis TLS, oblien#922 routing, oblien#392 amavis TLS issue, oblien#60 SPF). An open-PR search formail delivery OR deferrals OR postqueueis empty.
What I checked, and what I did not
I read the full PR diff, the commit message, mail-delivery.service.ts (lines 36-160 and 180-300 at head), the relevant test ranges, the dashboard consumer health-tab.tsx, CONTRIBUTING.md, and upstream history, merged PRs and prior-art searches. CI: all 9 fork checks pass on run 35813458516. I did not run the suite or the mutants; the Breaker's numbers stand as reported.
SECOND READ: READY
The last-shown-row, other-host, empty-relay-host and direct-box rows grade the same with the cap in the parse, so they pin nothing about this change. Read the parse result without a non-null assertion, like the rest of the file.
ReworkAddresses Redline's CHANGES_REQUESTED at Redline, blocking: four added variants pass without the fix. Removed the four rows from the Second Read, low: Measured at # BASE arm (src reverted to 4ad53d8a, test file at e610b22f)
× parseMailQueue > caps the reasons it reports
× checkMailDelivery > grades an auth refusal ranked fourth
× checkMailDelivery > grades a TLS failure at the smarthost ranked fourth
× checkMailDelivery > grades a connection failure at the smarthost ranked fourth
× checkMailDelivery > grades an auth refusal ranked thirteenth
× checkMailDelivery > grades an auth refusal queued after three one-off deferrals
Tests 6 failed | 41 passed (47)
# HEAD arm
Tests 47 passed (47)Every new or updated test fails on base; no controls remain. The mutants are still killed with the four rows gone (same builder, rebuilt from the
On Redline's operator note: Transcripts: |
VerificationAdversarial verification of rework round 3 (Redline CHANGES_REQUESTED at Superseded check, first. Numbers, re-measured independently in a fresh worktree at $ cd apps/api && bun x vitest run test/modules/mail/mail-delivery.service.test.ts --reporter=verbose
# HEAD
Test Files 1 passed (1)
Tests 47 passed (47)
$ git checkout 4ad53d8a -- src/modules/mail/mail-delivery.service.ts && bun x vitest run ...
# BASE
× parseMailQueue > caps the reasons it reports
× checkMailDelivery > grades an auth refusal ranked fourth
× checkMailDelivery > grades a TLS failure at the smarthost ranked fourth
× checkMailDelivery > grades a connection failure at the smarthost ranked fourth
× checkMailDelivery > grades an auth refusal ranked thirteenth
× checkMailDelivery > grades an auth refusal queued after three one-off deferrals
Test Files 1 failed (1)
Tests 6 failed | 41 passed (47)Matches the Hunter's claimed 6 failed / 41 passed and 47/47 exactly. All six mutants rebuilt from the head copy and re-run, kill sets identical to
Source file cmp'd byte-identical to the pushed head after every mutant run. Boundary probe re-run, output byte-identical to the Hunter's Ledger rebuilt from the diff (24 rows): every reachable row either has a fixture in the CI at PR body updated for this head: CI section reconciled from "pending" to the 9 green jobs above, Rules: no-control-cases-in-the-suite=covered(probe rows for R4/R11/R14b/R16/R18, base==head both |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the gating lane (gating review).
Verdict: APPROVE at head e610b22fc654d84bcdb36a511a29e71317a42481. No blocking findings; ready for the operator to submit.
This is a fresh verdict at the new head. My standing CHANGES_REQUESTED was at 129e4131; the only change since is apps/api/test/modules/mail/mail-delivery.service.test.ts (+3/-36), which removes the four table rows that passed on base and the parsed! assertion. The production file is byte-identical to 129e4131.
What I checked
Bug is real on the base. Fetched apps/api/src/modules/mail/mail-delivery.service.ts at oblien/openship@4ad53d8a and traced crowdedQueue(AUTH_REFUSAL) through it: the tally sorts to greylist(3), busy(2), full(2), auth(1); base parseMailQueue applies .slice(0, MAX_DEFERRALS) (line 244) before returning, so checkMailDelivery hands gradeDelivery three rejected rows (450/451/452 all match the rejected pattern) and .some() finds no auth/tls/network row: warn. With the fix the uncapped list reaches gradeDelivery and the 535 row classifies auth: fail. Same trace holds for the tls row (certificate verification failed naming the smarthost), the network row (Connection timed out naming the smarthost), the 13-distinct-reason fixture (12 fillers at count 2 sort ahead of the refusal at count 1), and the all-count-1 fixture (stable sort keeps insertion order, refusal at index 3).
Tests discriminate. All five grades %s rows and the updated caps the reasons it reports fail on base by the trace above and pass on the fix; the body's base-arm transcript (6 failed / 41 passed) matches. Each row asserts both the verdict and the exact three response rows, so the moved cap is pinned at the response edge too. No row survives on base any more.
Diff. checkMailDelivery grades queue (uncapped) and returns deferrals: topDeferrals(queue.deferrals); parseMailQueue drops the slice; topDeferrals is the same slice(0, MAX_DEFERRALS). gradeDelivery untouched. parseMailQueue's only non-test caller is checkMailDelivery (code search), so no API or dashboard consumer sees a longer array. Empty list, length equal to the cap, one past, and well past are each either pinned by a table row or identical on both arms per the Boundaries ledger; I agree with the ledger's reachability arguments.
Facts sheet. All required sections present with executed evidence: verbatim base/head transcripts, tsc rc=0, six mutants with kill sets, fork CI run 35961279325 green on all 9 jobs (gh pr checks 1), and the boundary probe output.
Prior art, re-run by me. gh search prs --repo oblien/openship for deferrals, MAX_DEFERRALS, topDeferrals, gradeDelivery, mail queue (open): all empty. gh search issues for deferrals: empty. Nothing upstream fixes this.
Policy. CONTRIBUTING quotes present and followed: one bug, two files, Conventional Commits, fix/ branch, Prettier-clean added lines with pre-existing drift left alone, a failing-on-base test per the "Prove it" line. AI use is permitted; no DCO or trailer required. Commit messages carry no attribution or model names.
Tells. No em dashes, control language, patch narration or filler in the added code, comments, tests, commit messages or title. The one-line doc on topDeferrals matches the file's comment idiom; the test names match the file's sentence-style names.
Non-blocking notes
parseMailQueue > caps the reasons it reports(test file line 149) now asserts the parse does not cap (toHaveLength(6)), and only its second assertion is about the cap (topDeferrals). The name reads backwards against the first assertion. Worth renaming before upstream submission if a further code round happens anyway; not worth a round on its own.- The ledger's "likely maintainer question" is a real one: a queue can now grade
failwhile none of the three displayed rows is the fatal reason, so the dashboard's per-row relay-auth hint will not render. Base showed amber for the same queue, so this is strictly better, and the body says so honestly. The operator should be ready to answer it.
Notes for the operator
- Squash the two commits before opening upstream: the second commit removes rows the first added, which reads as churn to a maintainer seeing the PR fresh. The body already says squash on merge is fine.
- CONTRIBUTING's "Green before you open" asks for
bun run testlocally; the fork CI ran the full suite green, but run it once on your machine as the line asks. - The PR template wants Summary / Motivation / Related issue / Changes / Verification / Checklist; the facts sheet supplies each, reshape to the template. Do not describe this as closing oblien#876.
askalf
added
ready-for-operator
|
Submitted upstream for review. |
3 participants
Summary
parseMailQueuecapped its deferral list atMAX_DEFERRALS(3) before returning it, andcheckMailDeliverygraded that capped list. A display cap was deciding the health verdict.failure naming it) is often the rarest line in the queue: one message has reached the relay
and been refused while older ones defer against ordinary receivers. Sorted by count and cut to
three, it never reached
gradeDelivery.warn("mail is moving late") instead offail.topDeferralswhen building the response. The response still carries at most three rows.e610b22f(production file +11/-2, the rest tests). Nonew dependencies, no reformatting of unrelated lines. The second commit is test-only (it drops
four table rows that passed without the fix); squash on merge is fine.
Both arms run at head
e610b22f. The test file holds 47 tests: 41 unchanged, 1 pre-existing testupdated (
caps the reasons it reports), 5 added as oneit.eachtable. All 6 new or updated testsfail on base; there are no controls.
Upstream
oblien/openshipmain4ad53d8a7ba53a1abb47a755639342d076f365e4. Currentorigin/mainis4fefe217(re-checked at
e610b22f); the source file is byte-identical between the two (blobdea13deaat both), and so is the test file (blob
f27baa46at both), so the base is current for thischange.
apps/api/src/modules/mail/mail-delivery.service.ts(checkMailDelivery,parseMailQueue, newtopDeferrals) and its test fileapps/api/test/modules/mail/mail-delivery.service.test.ts.Bug
Trigger. A server with an outbound relay enabled (
state.outboundRelay.enabled) whose relaycredentials, TLS or egress are broken, and whose Postfix queue also holds ordinary deferrals against
direct receivers (greylists, busy receivers, full mailboxes). Relayed mail fails with one distinct
reason, while direct mail to many receivers accumulates several commoner ones.
Wrong outcome.
parseMailQueuesorted the reason tally by count and applied.slice(0, MAX_DEFERRALS)inside the parse, so the reading it returned held at most three reasons.checkMailDeliverypassed that reading togradeDelivery, whosefatalcheck is a.some()overqueue.deferrals. With the fatal reason sorted to 4th place or lower it was not in the array,fatalwasfalse, and the function returned"warn".Blast radius. Every self-hosted openship box that relays outbound mail through a smarthost (SES,
Postmark, Mailgun, a provider SMTP) and has a broken relay configuration. The Health tab's Delivery
section shows amber "mail is moving late" while nothing leaves the box. The test file's own header
describes exactly this case as the one the module exists to catch:
Measured (probe output under Boundaries): with the fatal reason at sort index 0, 1 or 2 the base
verdict was
fail; at index 3 or beyond it waswarn.Repro
The bug is in a pure function chain fed by
postqueue -poutput, so the repro is the module's ownharness (
queueOutputbuilds a listing in Postfix's layout,box()stubs the executor) drivingcheckMailDelivery. Base arm = production file reverted to4ad53d8a, test file at heade610b22f:The main fixture (
crowdedQueue) is a queue of 8: three greylists, two busy receivers, two fullmailboxes, and one relay refusal, so the fatal reason ranks fourth.
Fix
Three edits in
apps/api/src/modules/mail/mail-delivery.service.ts:parseMailQueueno longer calls.slice(0, MAX_DEFERRALS). It returns every distinct reason,still sorted by count descending.
topDeferrals(deferrals)appliesMAX_DEFERRALS: the same slice, moved. Itcarries a one-line doc comment: "The deferral rows we report.
gradeDeliveryreads theuncapped list."
checkMailDeliverygradesqueue(whole) and returnsdeferrals: topDeferrals(queue.deferrals)(capped).
gradeDeliveryitself is untouched; it always judged whatever list it was handed.Why this is the minimal correct change. The cap is a display concern: the Health tab's
DeliverySectionrenders the rows in a small table. Moving the slice, rather than widening orremoving it, keeps the response shape identical. Every
checkMailDeliverycase added here assertsthe exact three rows the response carries.
Alternatives rejected, each built as a mutant of the production file at head
e610b22fand runagainst the touched test file (47 tests). The kill set is the exact list of tests that fail.
MAX_DEFERRALSto 4, cap still inside the parse. Still a cap deciding a verdict. Killedby 2:
caps the reasons it reports,grades an auth refusal ranked thirteenth.MAX_DEFERRALSto 8, cap still inside the parse. Killed by 1:grades an auth refusal ranked thirteenth(13 distinct reasons, the refusal 13th).parseMailQueue, cap kept there. Makes the parse depend onrelay state it does not receive. Killed by 6: the parse test and all five table rows.
health-tab.tsxand the APItype in
apps/dashboard/src/lib/api/mail.ts) would each need the cap. Killed by 5: every tablerow (each queue has more than three distinct reasons).
(
gradeDelivery({ ...queue, deferrals: topDeferrals(...) })). Killed by 5: every table row.topDeferralsthat setsdeferrals.length).Object-literal evaluation order then hands
gradeDeliverythe truncated list. Killed by the same5. This settles the aliasing row (R7) by execution.
gradeDeliverytake the raw output and re-parse. Not built: it duplicates the parser andchanges
gradeDelivery's signature, which the file's 8 directgradeDeliverytests would fail tocompile against.
Mutant transcript at
e610b22f:/agent-output/oss/openship/rv3-mutants.txt; builderverify-mutants.mjs, not committed. Historical, at129e4131(nine-row table):rework1-mutants.txt/rv2-mutants.txt, where the four rows since removed added kills only tothe fatal-first and drop-the-cap mutants, both still killed without them.
Test evidence
All tests live in the module's existing file, in its idiom: the file's
queueOutputfixturebuilder, its
box()executor stub, its named refusal constants, and oneit.eachtable with theexpected status and response rows carried as data. No new test file.
One pre-existing test is updated and five table rows are added. All six fail on base.
parseMailQueue > caps the reasons it reports(pre-existing, updated)expected … to have a length of 6 but got 3topDeferralsreturns the first 3 in ordercheckMailDelivery > grades an auth refusal ranked fourthexpected 'warn' to be 'fail'autharm of the fatal predicatecheckMailDelivery > grades a TLS failure at the smarthost ranked fourthexpected 'warn' to be 'fail'tlsarmcheckMailDelivery > grades a connection failure at the smarthost ranked fourthexpected 'warn' to be 'fail'networkarmcheckMailDelivery > grades an auth refusal ranked thirteenthexpected 'warn' to be 'fail'checkMailDelivery > grades an auth refusal queued after three one-off deferralsexpected 'warn' to be 'fail'Every table row asserts both
statusand the exact response rows (deferrals.map((d) => d.reason)).Transcripts at
e610b22f:/agent-output/oss/openship/rv3-{base,head}-arm.txt(base 6 failed / 41passed, head 47 passed, the same six names).
The four gate cases that passed on both arms (fatal reason on the last shown row; network failure at
another host; TLS failure with
relay.host === ""; auth refusal on a direct box) were table rows at129e4131and were removed ate610b22fbecause they do not discriminate the fix. Their behaviouris kept as measured rows under Boundaries (probe, plus the pre-existing
gradeDeliveryteststhat pin those gates).
Per CONTRIBUTING's "Prove the test can fail": the base arm above is that exercise. The code
under test was reverted to upstream
main, the six tests failed with the output quoted, and thecode was restored (checked byte-identical against a saved copy after the base arm and after the
mutants).
Tooling, at
e610b22f:prettier --checkon the two touched files reports style issues at head and at base alike: bothfiles carry pre-existing Prettier drift on upstream
main. Diffingprettier <file>against theworking copy shows every remaining hunk on a line this PR does not touch (the
mail-engineimport,two
it.eachclassification rows, thesystemctl showline, a pre-existingArray.fromin theparse test, and others). Per CONTRIBUTING, "Do not reformat unrelated lines or 'fix' pre-existing
Prettier/lint drift on lines you aren't otherwise changing", those hunks were not taken. Every
line this PR adds is Prettier-clean at the repo's pinned 3.8.1 under its
.prettierrc(
printWidth: 100).bun run test(the whole turbo monorepo suite) was not run locally in this container; theupstream's own CI workflows run it on the fork for every push. See Verification method.
Verification method
executed, in a Linux container: Bun 1.3.14 (the repo pins 1.3.10 in.bun-version; vitest/tscbehaviour is unaffected), Vitest 4.0.18, TypeScript via
bun run --cwd apps/api lint.bun install --frozen-lockfileagainst the repo's own lockfile.Executed independently TWICE at
e610b22f, first by the Hunter and then by the verification seatin a second fresh worktree: the touched test file on both arms (base 6 failed / 41 passed, head 47
passed, identical names both runs), the API workspace typecheck (
rc=0both times), sixproduction-file mutants rebuilt from the head copy and re-run (kill sets under Fix, identical
both runs), and the boundary probe quoted under Boundaries (output byte-identical across the
1c1504a1,129e4131ande610b22fruns).CI. The upstream's own workflows are enabled on the fork. At
e610b22frun35961279325 is green on all
9 jobs (
gh pr checks 1, re-checked by the verification seat): Documentation, Test, Test webmailserver, Tests (API 1/2), Tests (API 2/2), Tests (Database), Tests (Other packages), Tests (SDK and
CLI), Typecheck. At the previous head
129e4131(same production code, nine-row table), run35813458516 was also green
on all 9 jobs.
Not executed anywhere: end-to-end against a real Postfix/relay box. There is no mail stack in
this container or in CI. The queue fixtures are
postqueue -poutput in Postfix's layout, built bythe test file's pre-existing
queueOutputbuilder (unmodified). What this PR changes is which listreaches
gradeDelivery, which is pure and exercised above.Prior art
Re-run at
e610b22f, against currentorigin/main(4fefe217):git log --oneline 4ad53d8a..origin/main -- apps/api/src/modules/mail/ apps/api/test/modules/mail/git log --oneline -S"MAX_DEFERRALS" 4ad53d8a..origin/maingit rev-parse 4ad53d8a:<src> origin/main:<src>dea13deabothgh search prs --repo oblien/openship "deferrals" --state open[]gh search prs --repo oblien/openship "MAX_DEFERRALS" --state open[]gh search prs --repo oblien/openship "topDeferrals" --state open[]gh search prs --repo oblien/openship "gradeDelivery" --state open[]gh search prs --repo oblien/openship "mail-delivery" --state open[]gh search issues --repo oblien/openship "deferrals"[]gh search prs --repo oblien/openship "mail-delivery"(all states, hunt time)gh pr list --search "876 in:body" --state all(hunt time)No open PR fixes this. Issue oblien#876 ("Email service is waiting for emails to go out forever") is
the surface that led here but is not what this PR fixes: that queue stall was a Docker/Amavis
problem, addressed upstream by oblien#885 via oblien#891/oblien#892. This PR is a separate defect in the code that
renders the same panel. It should not be described as closing oblien#876.
The nearest precedent for the class is
73bd16c6, "fix(mail): read the DB probe verdict pastdocker's stderr warnings (oblien#783)": a presentation-layer transformation applied before a verdict was
computed, fixed by an outside contributor and merged.
Policy
Quoted verbatim from
CONTRIBUTING.mdatoblien/openship@main(4fefe217; file re-fetched ate610b22f, byte-identical to the copy these lines were taken from):- **One change per PR.** One bug, or one agreed feature. Don't bundle unrelated changes.→ One bug; two files.
- **Scope the diff.** Touch only the files your change needs. Do **not** reformat unrelated lines or "fix" pre-existing Prettier/lint drift on lines you aren't otherwise changing — runbun format, then review the diff and drop anything unrelated before you push.→ Prettier was run and its unrelated hunks dropped; see Test evidence.
- **Prove it.** Add a test that fails without your change and passes with it, and say so in the PR.→ Six such tests; both arms quoted verbatim.
- **No test spam.** A test earns its place by catching a regression that could actually happen. … one test that genuinely fails without your change is worth more than twenty that can't fail at all.→ Every new or updated test fails without the change; cases that passed on base were removed.
- **Green before you open.**bun run test, the relevant typecheck (bun run --cwdlint
), andbun formatall pass locally.→ Typecheck
rc=0locally.bun formataccounted for above.bun run testwas not runlocally; the fork's CI runs it (see Verification method). The operator should run it locally
before opening upstream, as this line asks.
### Using AI assistants):AI tools are fine to use — but **you** are the author and are accountable for every line you submit:→ no ban, no CLA, no DCO, no mandated commit trailer.- **Understand your whole diff.** If you can't explain a line in review, don't submit it.- **Verify, don't trust.** Actually run the change and confirm it does what the PR claims. Do not paste generated code — or a generated PR description — that you haven't checked against the real codebase.- **Commits**: [Conventional Commits](https://www.conventionalcommits.org/) -feat:,fix:,docs:,chore:``→
fix(mail): …and `test(mail): …`; branch `fix/mail-delivery-verdict-past-reason-cap`.- **Code style**: Prettier - runbun formatbefore committingAbsent at
main(contents API):AGENTS.md,CODE_OF_CONDUCT.md,AI_POLICY.md,.github/AI_POLICY.md,AI.md,AGENT_POLICY.md,.github/CONTRIBUTING.md. The PR template(
.github/pull_request_template.md) asks for Summary / Motivation / Related issue / Changes /Verification (real output) / Checklist; this sheet supplies each.
Disclosure facts for the operator
Plain facts, for you to word your own disclosure:
apps/apimail module and found this defect. It was not reported by a userand is not the bug described in issue [Bug]: Email service is waiting for emails to go out forever oblien/openship#876.
added cases while trying to break the first ones; AI reviewers then asked for explanatory comments
and em-dash prose to be removed, for the cases to be folded into one table, and for four table
rows that passed without the fix to be dropped. The AI made each of those changes.
bun run --cwd apps/api lint, Prettier 3.8.1,a boundary probe over the cap positions, and six mutants of the production file.
bun run test(full monorepo suite) orbun run buildin its own container;the upstream's CI workflows run on the fork at each head.
Boundaries
Every predicate, comparison and index expression the diff adds or changes. "Probe" rows were
measured by a throwaway harness (output below the table), run at
1c1504a1,129e4131ande610b22fwith identical output; it was not committed.topDeferrals:deferrals.slice(0, MAX_DEFERRALS)[][], a new arrayreports a healthy direct sender(empty queue →deferrals: [])topDeferrals(len=3) -> len 3); same on base, where the slice sat in the parsegrades an auth refusal ranked fourth,… queued after three one-off deferrals(exact rows asserted)caps the reasons it reports(6);grades an auth refusal ranked thirteenth(13)slicereturns a new array; source untouchedtopDeferralsmutant is killed by all five table rowsparseMailQueue: cap removedcaps the reasons it reports(6); probe (the rest)Mail queue is empty{queued:0, sampled:false, deferrals:[]}, unchangedreads an empty queue as a conclusion, not a failed probe(pre-existing)null; thenullguard is untouchedrefuses to read %s as a queue(pre-existing)gradeDelivery(queue, relay), argument now uncappedfail, same as baseauth at index 0/1/2: full=fail capped=fail); a table row at129e4131, removed because it passed on basefail; basewarngrades an auth refusal ranked fourthfail; basewarngrades an auth refusal ranked thirteenth(12)tlsarm of the fatal predicatefail; basewarngrades a TLS failure at the smarthost ranked fourthnetworkarmfail; basewarngrades a connection failure at the smarthost ranked fourthtls/networkarmwarnboth arms; the gate is unchanged codeonly warns when the failing host is not the smarthost(pre-existing); measured past the cap at129e4131(warnboth arms,rv2-{base,head}-arm.txt)autharmfail; basewarngrades an auth refusal ranked fourthrelay?.enabledgatewarnboth arms; the gate is unchanged codefails a relayed box on an auth refusal(pre-existing, itsundefinedrelay assertion); probe (no relay, auth present: warn)enabled: falsewarnignores a relay row that is switched off(pre-existing)!!hostguardrelay.host === "", TLS failurewarnboth arms; the empty host cannot matchrelay host empty string: warn); measured past the cap at129e4131queue.deferrals.length === 0early returnqueued: 0, deferrals non-emptyfail(relayed, auth); branch not takenfailgrades an auth refusal queued after three one-off deferrals(verdict and rows in queue order)deferrals: topDeferrals(queue.deferrals)in the responseMAX_DEFERRALSas a mutation targetgrades an auth refusal ranked thirteenthR4, R11, R14b, R16 and R18 describe behaviour that is identical on both arms, so no new test pins
them; each is pinned by a pre-existing test or measured by the probe.
Probe output, verbatim (at
e610b22f, identical to the1c1504a1and129e4131runs):capped=is the base behaviour andfull=the fixed behaviour; the boundary is exactly index 3,i.e.
MAX_DEFERRALS.Behaviour outside the stated bug: none found.
gradeDelivery,describePath, thenullguard,sampled,queuedand reason clamping are untouched; the 41 unchanged pre-existing tests pass onboth arms. One observable change beyond the verdict:
parseMailQueueis exported and now returns anuncapped list, but its only caller outside the tests is
checkMailDelivery(
git grep parseMailQueue), which caps before responding, so no API or dashboard consumer sees alonger array.
Likely maintainer question (not changed here). The verdict can now be
failwhile the fatalreason is not among the three rows shown, so the dashboard's relay-auth hint
(
health-tab.tsx, rendered per displayed row) does not appear. Base showed amber for the samequeue, so this is still strictly better. Keeping the fatal row visible would need the relay in
topDeferrals(a signature change); that is a separate change if the maintainer wants it.Suggested upstream PR title
fix(mail): grade outbound delivery on the whole queue, not the top three reasons