doctor(Email): guard the drain's failure path, cut the dead backfill arm, retire the prose about a section that moved - #1790
Merged
Conversation
… its owning sections EMAIL-2 named src/Sections/Humans.Email/Services/EmailRenderer.cs, deleted in #1651. The defect is still live, but the templates moved out with the renderer, so the fix now lives in three other sections: GoogleIntegration (GoogleIntegrationEmails.cs:38,46,54), Issues (IssuesEmails.cs:31) and Shifts (ShiftsEmails.cs:36,76). Debt belongs where the next reader of that section meets it (memory/process/debt-ledger-additions.md), so the row is closed here and re-filed there. Also files the doctor run's own out-of-scope finds against Email: the duplicated test-address rule, the hardcoded dashboard throttle figure, and three unpinned halves of the section's invariants.
EmailOutboxProcessor mirrored the campaign grant unwrapped inside the per-message catch. A throw from another section's bookkeeping had nowhere left to land: it escaped the loop and ProcessQueuedAsync itself, so the message never got its error log, the pending meter was never set, and the rest of the batch stayed stamped PickedUpAt until the five-minute stale window released it. The success path already wraps the identical call in TryUpdateGrantEmailStatusAsync with a comment saying why. The failure path now uses the same helper, and the test reproduces the abort against the old code. Behaviour changes on the error path only: a grant-mirror failure is now logged and swallowed there, as it already was after a successful delivery. Reviewed by doctor-reviewer-critical: APPROVE-with-correction (two health.md line anchors, applied). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
The old target described a renderer pipeline that #1651 deleted and carried a Needs-Peter question the same PR answered. The new one adds the daily tally (nobodies-collective#1195) and the pause's move to /Settings#email (#1634), gives every invariant an enforcement cite, and names the shape the code does not yet have: a name for each of the two things the send log is asked to be — the retry queue and the per-human record. Run file carries the assessment, the findings and the 3f debt verdict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
GetSentOrFailedSinceAsync loaded Failed rows that its only caller filtered straight back out, so the failures-are-never-backfilled rule from nobodies-collective#1195 was written twice — once as a query arm that fetched them, once as the filter that threw them away. It is now stated once, in a query whose name says it: GetSentSinceAsync. The service-side status filter goes with it. SentAt is set only by MarkSentAsync and pickup requires SentAt null, so no non-Sent row can carry a SentAt and the SentAt dereference downstream stays safe. The service test that fed a Failed row now feeds a Sent one with RetryCount 2 — the tempting case, a message that failed twice before it went out — and still asserts FailedCount 0; the repository test carries the exclusion itself. Reviewed by doctor-reviewer-critical: APPROVE-with-correction (the test was vacuous as first written, applied). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
…r has Three claims went stale under #1651 and the G5 split: the architecture test's header listed "the renderer" among the section-internal pieces (EmailRenderer.cs was deleted), its sealed-repository note cited a rule docs/architecture/roslyn-analysis.md records as retired along with the Humans.Infrastructure assembly it swept, and OutboxEmailServiceTests described two of its own members as borrowed from Humans.Application.Tests, a project that no longer exists. The Application-layer / Infrastructure-layer descriptors go with them: both projects dissolved in the split and every type they qualify is section-internal now, so the labels record the old shape rather than this one. The same retired-rule comment sits in three other sections' test projects, which this run does not touch — filed as CENTRAL-65. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
Eight sites told a reader about a move rather than about the code in front of them: the invariant doc's account of the TriggerImmediate field, the DbContext peel, the FK and nav removals, and the two jobs' route from Base into Jobs/; the settings tab and its view component on the toggle's old home; and the job's rebuttal of a claim nobody makes here any more. Where the history carried a live reason — why the jobs are public, why the immediate processor sits under Contracts/, why the assembly is not load-bearing — the reason stays and only the itinerary goes. Also drops "the renderer" from the doc's list of section-internal pieces: #1651 deleted it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
- The feature doc still put the pause/resume toggle on the outbox dashboard; #1634 moved it to /Settings#email and left a link behind. - authorization.md listed only EmailController, so the one endpoint in the section open to any authenticated human — EmailPreviewController's POST /Email/PreviewMarkdown — appeared nowhere in the table a reader checks for exactly that. - data-access.md recorded EmailPreviewService as Scoped where Section.cs registers it singleton, and filed the section-internal IEmailBodyComposer among OutboxEmailService's cross-section calls. - Two of the guide's freshness triggers watched a view and a controller that no longer carry the Profile Emails surface; they now point at ProfileEmails*. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
It checked only that the daily tally stayed empty, which a message that was never picked up would also satisfy. The invariant is narrower than that: a test-domain row is marked Sent deliberately, without a transport call, because sending to it bounces and costs sender reputation. Both halves are now asserted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
Findings, the 3f verdict, what was worked, what was left and why, the retro and the three questions for Peter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Reviewed commit 55ddbbe — no issues found. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
Owner
Author
API-equivalent $, list rates; run under subscription quota. Measured Phase 1 to PR creation; PR create/backfill and Phase 8 excluded. Peak main-thread context: 289,991 tokens (assess). Generated by Claude Code |
PR Surface ReportCompared Summary: 28 changed file(s) | EF migrations: 0 added file(s), max 0/1 per context Reforge Surface Score
Section DeltasNo section score changes. Section Size & Complexity Deltas
Rule DeltasNo rule score changes. Corpus Size & Complexity
At head: largest class Published Write Surface14 of 48 sections publish write capability, 23 interfaces (0). Interface SurfaceNo new interfaces or interface methods. Diff Size
New Files
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ddbbeb65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…view memory/process/debt-ledger-additions.md sets one test for the field: review: light only when the fix is rule-prescribed and the verifier is mechanical, otherwise panel. Two of the rows this run filed fail it on their own evidence. EMAIL-3 says in its own what that neither class is the obvious owner, which is why the run ledgered it rather than picking one. EMAIL-4 is the same shape: the tile shows a true figure under the wrong label, and relabelling it or computing the real one are both defensible. Marked light, /debt-sweep would have skipped the second-opinion panel and let one implementer's pick land unreviewed. EMAIL-5 through EMAIL-7 stay light. Each names the invariant to pin and the file that lacks it, so the remedy is prescribed and the verifier is a test that passes or does not. Review-round: 1 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
One conflict, in src/Sections/Humans.Email/Docs/authorization.md: both sides added the EmailPreviewController row. Took main's, which additionally names the two actions and the 5/min per-human rate limit on SendMarkdownToSelf — verified against EmailPreviewController.cs:24,25,46 and Section.cs:62-71. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
1194 closed as completed, 1657 retargeted at Events and Tickets with its label moved, the outbox-table split left as is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 085bbc02fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The skill is explicit that a ledger entry is prose with no line numbers. Seven rows this run filed carried file:line coordinates; paths and symbols stay, the numeric anchors go. Review-round: 2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
…dy fixed Textual conflict was only Email/Docs/debt.yml — both sides closed EMAIL-2, this side then added EMAIL-3..7. Kept ours; next_id only ever increases. The larger conflict was semantic. Main's nightly sweep (#1789) independently fixed the subject double-encoding this run had re-filed, so ISSUES-9, SHIFTS-5 and GOOGLE-3 are deleted unworked: verified on the merged tree that every subject argument at those sites now passes raw while the body arguments stay encoded. The finding was real when filed; the row would have been stale on arrival. next_id preserved in all three ledgers. CENTRAL-65 checked against the same sweep and left standing — all three architecture test files still reference the rule it names. Run file records the deletions and Peter's rulings on the three Needs-Peter items (close / retarget / leave it). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The daily section-doctor run on Email. One real defect fixed in the outbox drain, one dead query arm cut, and the section's comments and docs brought back in line with the code after three recent PRs moved things out from under them.
The full assessment, every finding with its disposition, and the retro are in
docs/health/runs/2026-09-22-Email.md. The regenerated target shape issrc/Sections/Humans.Email/Docs/health.md.Why
Email had just absorbed the template move out of the renderer (#1651), the daily send tally (nobodies-collective#1195) and the pause moving to
/Settings#email(#1634). Most of what this run found is the wake of those: prose describing a section that no longer exists.The defect is not.
EmailOutboxProcessormirrored the campaign-grant status unwrapped inside the per-messagecatch. A throw from another section's bookkeeping had nowhere left to land — it escaped the loop andProcessQueuedAsyncitself, so the message never got its error log, the pending meter was never set, and the rest of the batch stayed stampedPickedUpAtuntil the five-minute stale window released it. The success path has always wrapped the identical call inTryUpdateGrantEmailStatusAsync, with a comment explaining exactly why.Behaviour changes on the error path only, and only in the direction the success path already had: a grant-mirror failure there is now logged and swallowed instead of aborting the drain. Nothing else in this PR changes what the section does.
Existing surface checked
No new durable surface. The grant-mirror fix reuses the private helper that was already there; the query keeps its one call site under a narrower name (
GetSentOrFailedSinceAsync→GetSentSinceAsync); the strengthened tests extend cases that already existed. Considered and rejected: a shared helper for the duplicated test-address rule — neitherEmailOutboxProcessornorEmailOutboxServiceis its obvious owner and the third option invents a type, so it is ledgered as EMAIL-3 rather than decided here.UI changes / screenshots
None. No view, resx or route changed, so there is nothing to render. This was an unattended cloud run: the app was never booted.
Checklist
mainonpeterdrier/Humans(the QA fork).origin/main.EF migrations— no schema change; a run never makes one.NuGet packages— none touched.New project rule— the one durable lesson this run hit (phantom Razor errors from a concurrent build) is alreadymemory/process/no-concurrent-roslyn.md; no new atom.dotnet test Humans.slnx -v quietgreen on the merged head (49 test projects, 0 failures);Humans.Integration.Testsself-skips under cloud by design.Nav coverage— no new page.Dates/times via NodaTime— no new date handling; existingIClockusage untouched.What else changed
Contracts/— the reason stays and only the itinerary goes.EmailPreviewController(the one endpoint open to any authenticated human),data-access.md's service lifetime and a section-internal interface filed as cross-section, and two stale freshness triggers in the guide.Sentand the transport is never called.Reviewer notes
Both non-mechanical changes went through
doctor-reviewer-critical, and both came back APPROVE-with-correction on the same failure mode — a test that passes without pinning anything. Both corrections are applied and named in the run file. The reviewer verified that dropping the service-side status filter loses no guarantee:SentAtis written only byMarkSentAsyncand pickup requires it null, so no non-Sentrow can carry one.Needs Peter — answered
All three are settled; nothing here is waiting.
EventsEmailsin Events andTicketsEmailsin Tickets as the sites carrying the English literals;section:emailreplaced bysection:events+section:tickets.EmailOutboxMessageas both retry queue and GDPR record). → leave it. One table with two jobs is the right answer at this scale; no issue filed.Later
First base merge (
79fc8db48). One conflict, insrc/Sections/Humans.Email/Docs/authorization.md: both sides had added theEmailPreviewControllerrow. Took main's, which additionally names the two actions and the per-human rate limit onSendMarkdownToSelf— verified againstEmailPreviewControllerand the policy registration inSection.cs. Everything else auto-merged; main'sThrottleDelayAsyncseam and this PR's guarded failure-path mirror both survive inEmailOutboxProcessor.Two review rounds, both Codex, both real.
review:routing.EMAIL-3was filedreview: lightwhen its own text says neither class is the obvious owner. Changed topanel, and the same audit applied to the other four rows —EMAIL-4also becamepanel;EMAIL-5throughEMAIL-7staylight, each naming the exact invariant to pin and the file that lacks it.file:lineanchors. Stripped, keeping paths and symbols — the run file keeps its anchors, which is where the evidence bar wants them. The same pattern in rows earlier runs filed is pre-existing and left alone.Second base merge (
153216e4c), and the more interesting one. The textual conflict was small — onlysrc/Sections/Humans.Email/Docs/debt.yml, where both sides had closed EMAIL-2 and this branch had since added EMAIL-3–7. Kept ours;next_idonly ever increases.The semantic conflict was larger. Main's nightly sweep (Daily tech-debt sweep — 2026-09-22 #1789) had independently fixed the subject double-encoding that this run re-filed as GOOGLE-3, ISSUES-9 and SHIFTS-5. Verified on the merged tree rather than on the claim: at every site those rows named, the subject argument now passes raw and only the body arguments are encoded. All three rows are deleted, unworked — a ledger row for a bug that is already fixed is worse than no row.
next_idis preserved in each ledger (Google 4, Issues 10, Shifts 6), so no id is recycled. The findings were real when filed; they would have been stale the moment they shipped.CENTRAL-65 was checked against the same sweep and left standing —
IRepositoryImplementationsAreSealedRuleis still referenced in all three architecture test files it names.Neither base merge is a review round: rounds spent stay at 2 of 5.
UPCOMING
Next four sections by the selector's priority, so you can redirect it before tomorrow's run: Gdpr, CityPlanning, Expenses, Store.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SCrRPogs7ZYUv9VwxewFZ3