review: Ludwig's phase B review of Della's email plan (ludwig-0002, Vikunja 460)

docs/plans/2026-10-21_email-review-by-ludwig.md

@@ -0,0 +1,102 @@
+# Review of Della's email plan (ludwig-0002)
+
+- Reviewed: `origin/della/email-plan` 191dff32f, `docs/plans/2026-10-21_email-plan-della.md` (341 lines), read in full.
+- Mine: `origin/ludwig/email-plan` 4a27b73ac, `docs/plans/2026-10-21_email-plan-ludwig.md`.
+- Pins: Stack c3d88840c, `mail-processor` 95b0298, `email-processor` 7a64921, Q198 2ee68cd69. Della's lines are cited as `della:N`.
+- Phase B only. My plan is unchanged.
+
+## 1. Wrong or unsupported claims
+
+I checked 44 of Della's file:line citations against the pinned commits: every Stack, `mail-processor` and Q198 line she cites, plus the two `email-processor` PRD lines. Each one points at the construct she names. The plan's evidence is sound. Findings:
+
+- F1. Nit. `della:41` cites `poller.py:186` for the duplicate check. Line 186 initializes a counter. The check is at `poller.py:187-195`.
+- F2. Defect. `della:233` proposes the slot path `<dataRoot>/credentials/mail/<business>/<source>/oauth`. That breaks the Stack's convention and is unsafe:
+  - Credentials live under the config directory: `storePath` builds `<configDir>/secrets/agents/<business>/<agent>/<service>` (`packages/business/src/credentials.mjs:57`), and instance slots follow the same pattern (`packages/webui/src/sso.mjs:33-45`, `secrets/mosaic-stack/<slot>`).
+  - `scripts/reset.sh:47` runs `rm -rf` on the data root, so a reset would delete the mail credentials along with the data.
+  - Fix: put the slot under `<configDir>/secrets/`.
+- F3. Overreach in wording. `della:220` says mail access grants "follow the existing chief → `credential.mint` → authenticated human Approve chain".
+  - The PRD's grant methods today are machine key, Gitea and GitHub (`docs/prd/mosaic-stack.md:463-475`). `credential.mint` describes making a key or token. A mail read right mints no credential.
+  - Della does say a new grant method needs a Jason-approved PRD version (`della:222`), so the claim is hedged, not false. The cleaner form is a new action, for example `mail.access`. It is a hard wait under REQ-WAIT-3 (`:607`) until a PRD version rules otherwise, and it is approved through the same inbox flow.
+- F4. Gap against the brief. The brief asked to report any secret held in either repo or its history, by path and commit. Della's plan reports no scan result. My plan has one (F13: none found, pattern-based). Graft mine.
+
+No other claim is wrong. Della keeps the Microsoft protocol references (`della:104-110`) to "protocol references, not evidence of installed support". The caveats themselves are accurate: immutable IDs do not survive archive moves, and Entra grants are independent of Exchange RBAC scoping.
+
+## 2. What Della has that mine lacks (graft into the base)
+
+- G1. Single leased owner per source (`della:76-81`). One lease owns retries, so adapter and manager loops can't multiply them (`della:270`). Mine has one process but no lease.
+- G2. Delivery outbox with a stable key and a recipient-side unique receipt (`della:133-136`). This covers a crash after delivery and before acknowledgement. Mine dedups intake only.
+- G3. Quotas. Della caps each poll at 50 messages and 10 MiB, and holds new intake at 1,000 pending deliveries or 100 MiB stored per source, with persistent accounting, an alert and continued health checks (`della:138-142`). Mine has only a message-size cap and a per-run count cap.
+- G4. History import as a separately approved, previewed window (`della:143-146`). Mine only starts from now.
+- G5. Identity reconciliation for IMAP moves, epoch resets, expired delta cursors, archive moves and reimports. It also treats a reused or missing Message-ID as a correlation signal, never proof of identity, and an edit as a new version that doesn't rerun prior actions (`della:148-156`). Mine merges by Message-ID plus body hash, which is weaker. Take hers.
+- G6. Dedup tombstones kept beyond content expiry, and suppression so restoring a backup doesn't bring back expired content or old work (`della:155,195-196`).
+- G7. Explicit MIME bounds: depth 10, 100 parts, 256 KiB of normalized text (`della:159-161`).
+- G8. Separate rights for metadata, body, attachment, draft and routing. Each read is rechecked, and a revoke cancels in-flight reads (`della:212-216`). It adds isolation across businesses (`:218`) and independent deliveries for multiple recipients without disclosing the other recipients (`:124-125`).
+- G9. Protected credential-use callbacks (`della:92`). The adapter receives a callback, never the token. This matches the broker's existing `Credentials.use(instance, service, operation)` (`packages/bus/src/credentials.mjs:379`) and its leak check `assertClean` (`:364-371`). Mine had the mail service read the file. Della's form is better, because the token never leaves protected code.
+- G10. Bcc or ambiguous delivery goes to restricted operator review (`della:122-123`). Arbitrary `Delivered-To` headers are insufficient unless the enrolled delivery path proves their provenance (`della:121-122`). Mine said to verify the header per provider. Della's rule is stricter, and correct, because a sender can add a fake `Delivered-To` header.
+- G11. Send is blocked in three places: adapter interfaces, credentials and network policy (`della:246-249`).
+- G12. Recovery canary is a synthetic message through the pipeline, never live mail or test mail (`della:273-275`). Alerts go out once per incident, on severity change and on recovery (`della:257`). Health also reports lease owner, outbox age and disk pressure (`della:253-255`).
+- G13. Tools `mail_health` and `mail_ack` (Stack-local state only), with paginated list defaults. The CLI and Console use the same handlers (`della:203-208`).
+- G14. A test that the disabled add-on makes zero connections and that v1 starts without it (`della:289`). POP3 is an optional last row (`della:301`). Rows name their test files (`della:290-301`).
+
+## 3. What mine has that Della's lacks
+
+- L1. Secret scan of both repos and their history, result reported (my F13). See F4.
+- L2. The prior work targets the USC LLC tenant (`deploy/email-processor.prod.stack.yml:28-48`). The agent addresses should not land in an employer's tenant (my Q1). Della's Q1 asks which provider but doesn't raise this.
+- L3. Concrete test ports with counts: the 36 rule-engine cases (`tests/test_rule_engine.py`) and the 15 safety-gate cases (`tests/test_predeploy_safety.py`). Della says "selected synthetic tests" (`della:30`).
+- L4. Sender authentication. Wake and allowlist checks use `Authentication-Results` from the receiving server only, and ignore any sender-supplied copy of that header. Only allowlisted senders whose mail passes DMARC can wake an agent. Della has "sender cannot approve" (`della:238-239`) but no authentication gate on wake-up.
+- L5. Concrete injection controls:
+  - an untrusted notice modeled on `WEB_UNTRUSTED` (`packages/bus/src/broker.mjs:109,2553`)
+  - removal of hidden HTML text and zero-width/bidi characters
+  - a reduced tool set for a turn woken by mail: no push, no credentials, no `message.external`
+  - `source: mail` tagging on decisions raised from mail
+  - a red-team fixture corpus as acceptance tests
+
+  Della states the principle (`della:240-242`) but not these mechanisms.
+- L6. The routing model step sends metadata only, never body text, so a body cannot steer routing. Della allows "minimal, explicitly permitted content" (`della:171`).
+- L7. A rejected login is retried once and then stops, because repeated bad logins can lock the account. Della holds auth denial for correction (`della:263-264`), which is close. Mine adds the explicit single retry and the lockout reason.
+- L8. Two questions Della doesn't ask: the new npm dependencies (`imapflow`, `mailparser`), and whether the Stack may ever write back to source mail. Della forbids write-back in her design (`della:94`) without asking.
+- L9. Stack precedents to reuse: the Discord connector's authorize, journal, engine, deliver pipeline with replay dedup (`packages/discord/src/connector.mjs:1-30`, `journal.mjs:60`), and the closed credential service list that needs a `mail.<sourceId>` kind (`vocabulary.mjs:109,119`). Della cites the vocabulary but not the connector.
+
+## 4. Disagreements and which side I take
+
+- X1. Agents rerouting one message (`mail_route`, `della:204`). **Take mine: no agent reroute.** Rerouting gives another principal read access to that message, which widens access. REQ-GRANT-2 makes Approve the only thing that creates access (`:454-456`). An injected mail asking to be "forwarded to the chief" is the obvious attack. Reroutes should be a Console or CLI human action. An agent that needs another seat's help sends a bus message, and that seat reads only what is routed to it.
+- X2. Who binds an agent to a route. Mine used role authority plus a route table in the organization file. Della uses an approved grant per agent or user (`della:212-225`). **Take Della's for the agent-to-route binding**, with my F3 wording, since the brief requires the Stack's grant and approval model. **Keep mine for the route table itself**, which maps addresses to targets and changes through the organization write path.
+- X3. Pruning. Mine had `mail prune` with a receipt. Della requires `data.delete` approval or an approved retention authority (`della:197-198`). **Take Della's.** `data.delete` is a hard wait (`docs/prd/mosaic-stack.md:588`), and no role file may grant one (REQ-WAIT-2, `:602-606`). Automatic expiry therefore needs a retention policy Jason approves in a PRD version.
+- X4. Encrypting content at rest. Mine relied on host disk encryption. Della uses encrypted blobs and cursors with separate content keys (`della:183-187`). **Take Della's.** The data root is backed up, and deleting a key is the only way an expiry also reaches the backup copies (G6). It costs a key-management row.
+- X5. Retention lengths. Mine: 90 days for content, 365 for metadata. Della: 30 days for bodies and drafts, 7 days for fetched attachments, 365 days for metadata, and metadata only until Jason approves content retention (`della:191-194`). **Take Della's.** Less stored hostile and third-party content, and the stricter default is fail-closed.
+- X6. Plus-addressing or aliases first. Mine started with plus-addressing. Della starts with aliases and allows plus-addressing only after a provider test proves the recipient survives delivery (`della:114-116`). **Take Della's.** Both plans need the recipient proof, and her order doesn't rely on it at the start.
+- X7. Row order:
+  - Della puts grants (E02) before intake (E03). Intake, parse and rules don't need agent grants, so they can run in parallel. Grants must land before any agent tool (her E08, my M7). **Take a merge:** store, parse and rules in parallel with grants. Tools come after both.
+  - Della builds M365 first (E05). I built IMAP first (M4). **Neither is right by default.** The first source is the provider Jason names in Q1.
+- X8. Model input. **Take mine** (L6) for routing classification. **Take Della's** (`della:173-174`) for summaries and drafts: those run as ordinary governed agent work, not as a model client inside the mail service. The two fit together.
+- X9. Size caps. Mine: 25 MiB per message. Della: 10 MiB per poll and per attachment (`della:139,177`). **Take Della's** smaller pilot defaults.
+
+## 5. Merged questions for Jason
+
+Each one has a recommended answer. Source codes: L is my plan, D is Della's.
+
+- Q1. Which provider and domain host agent and user addresses? (L Q1, D Q1)
+  - One read-only ingress mailbox on a domain Jason owns personally, not the USC tenant.
+  - Aliases per agent and user. Plus-addressing only after the recipient-survives-delivery test.
+  - No catch-all and no MX change. A dedicated mailbox only where confidential separation needs one.
+- Q2. Which sources, in what order? (L Q2, D table)
+  - First the provider Q1 names, then the other of IMAP and Microsoft 365.
+  - POP3, Gmail API and JMAP only for a named need.
+- Q3. Webhooks, inbound listeners or paid mailbox infrastructure? (L Q2, D Q5) No. Outbound IMAP and Graph polling only. Each future webhook, DNS or MX change, or spend gets its own authorization.
+- Q4. A Stack-held source service credential, plus separate approved per-agent and per-user mail-access grants through a post-v1 PRD addendum? (D Q2) Yes. Use a new hard-wait action approved in the inbox (F3). No agent holds provider secrets or reads personal mail implicitly.
+- Q5. May rules route mail and wake a granted agent without per-message approval? (D Q3, L Q5) Yes, for reviewed routes, and only for allowlisted senders whose mail passes DMARC. Everything else goes to quarantine for a human. A model-needed decision goes through the Q207 module.
+- Q6. May the Stack ever change source mail (mark read, flag, move, delete)? (L Q3) No. Read-only, with Stack-side cursors and acknowledgements.
+- Q7. Retention, encryption and deletion authority? (L Q6, D Q4)
+  - 30 days for encrypted bodies and drafts, 7 days for fetched attachments, 365 days for minimal metadata and tombstones.
+  - Metadata only until Jason approves this.
+  - Pruning runs under a retention policy Jason approves in the PRD, because `data.delete` stays a hard wait.
+  - Jason names any business retention or legal-hold needs before live intake.
+- Q8. Sending? (L Q4, D Q6) Out of this add-on. R21 stays permanent for the specialist. Drafts are Stack-local and a human sends them. Any future sender is a separate design using per-recipient `message.external` approval.
+- Q9. Two exact-pinned npm dependencies, `imapflow` and `mailparser`, after license review? (L Q7) Yes. An in-house IMAP and MIME parser is the larger risk.
+- Q10. Which of Jason's own mailboxes, if any, may the Stack read? (L Q8) None at first. Add one later by name, read-only, with its own grant.
+- Q11. Attachment scanning? (L Q9, D D6) An allowlisted type, size-capped raw fetch into quarantine needs no scanner. Content extraction (OCR, archives, office parsing) waits for a local scanner row, as Della's D6 requires.
+- Q12. Is the USC deployment of the Python service still in production? (L Q10) Either way this add-on leaves it untouched. Recommend archiving `email-processor` (it is contained in `mail-processor`) and keeping `mail-processor` as the design reference.
+
+## 6. Base recommendation
+
+Della's plan is the stronger base for data integrity and the grant model (G1-G10, X2-X5). Mine is stronger on injection controls, sender authentication and concrete test ports (L3-L6). I recommend Della's plan as the base, with L1-L9, X1 and the F2 fix grafted in.