docs(spec): inbound ingestion + Gmail-push behavioral spec (HT-34) - #34
Merged
Conversation
Adds specs/mail/inbound-ingestion.md (provider-agnostic ingest pipeline) and specs/mail/gmail-push.md (Gmail push transport) — the behavioral contract HT-35..HT-44 build against. Pins: raw-message provider boundary (parse once, by our code); idempotency on (mailboxId, providerMessageId) rather than the sender-controlled RFC Message-ID; transactional cursor advancement; at-least-once ingest with a dead-letter ledger; own-message loop suppression; and the dogfood expired-cursor policy (pause + manual rebaseline, not auto-resync). Flags one deliberate divergence per charter §2: does NOT suppress generic third-party Auto-Submitted/bulk mail by default (auto-submitted.json shows the reference system ingesting it) — left as an explicit open question, not a silent change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds specifications for a provider-agnostic inbound email ingestion pipeline and a Gmail Pub/Sub push transport, covering raw-message processing, authentication, reconciliation, idempotency, cursor handling, lifecycle renewal, suppression, and acceptance scenarios. ChangesInbound mail processing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PubSub
participant GmailWebhook
participant GmailAPI
participant IngestPipeline
participant MailStore
PubSub->>GmailWebhook: Send authenticated notification
GmailWebhook->>GmailAPI: Reconcile history from stored cursor
GmailAPI-->>GmailWebhook: Return changed messages
GmailWebhook->>GmailAPI: Fetch raw RFC822 messages
GmailAPI-->>GmailWebhook: Return raw message bytes
GmailWebhook->>IngestPipeline: Submit message bytes and metadata
IngestPipeline->>MailStore: Store or suppress message
MailStore-->>IngestPipeline: Confirm outcome
IngestPipeline-->>GmailWebhook: Confirm batch completion
GmailWebhook->>MailStore: Persist advanced cursor
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/mail/gmail-push.md`:
- Around line 44-47: Update the OIDC JWT verification requirements in the Gmail
push authentication documentation to require the email_verified claim to be
present and true, in addition to the existing signature, issuer, audience,
email, and expiration checks.
- Around line 48-50: Update the Gmail push authentication specification to
require an explicit allowlist check on the push envelope’s subscription resource
name. The handler must compare that value with the configured expected
subscription or allowlist in addition to validating the JWT and service account,
and reject notifications from any other subscription.
- Around line 66-76: Update the Gmail push reconciliation flow to resolve and
validate the mailbox using the notification’s emailAddress before recording the
cursor or invoking users.history.list. Reject mismatched or unresolved
notifications, and only continue with the resolved mailboxId for history
reconciliation and ingestion.
- Around line 109-119: Extend the daily mailbox renewal flow around
SchedulerProvider.registerCron and Gmail watch() so it also reconciles each
active mailbox’s cursor via a bounded history.list sync. Ensure missed or
delayed notifications are recovered without introducing an unbounded polling
loop, while preserving the existing daily watch re-arming behavior.
In `@specs/mail/inbound-ingestion.md`:
- Line 136: Update the human-support-staff reference in the inbound-ingestion
documentation sentence to use “an Agent” instead of “an agent,” preserving the
surrounding wording and meaning.
- Around line 122-128: Revise the “Loop suppression” rule so matching our
sending identity in From or Return-Path alone never drops a message. Require a
verifiable correlation, such as an exact outbound Message-ID, provider
sent-message identity, or signed reply token; use sender identity only as
supporting evidence, while preserving the rate-cap backstop.
- Around line 57-78: Update the inbound-ingestion flow to atomically claim or
lease the delivery-ledger key `(mailboxId, providerMessageId)` before
processing, preventing concurrent deliveries from both proceeding. Make
`createConversation` and `appendThread` idempotent for that key, or wrap the
ledger claim, store operation, and `stored` outcome update in a
transaction/outbox so retries return the existing outcome without duplicating
writes.
- Around line 33-36: Align the shared InboundEmailProvider contract and all
adapters so each delivery exposes raw RFC822 bytes (or a blob reference)
together with provider metadata, without engine-facing message parsing. Update
specs/mail/inbound-ingestion.md lines 33-36 to document this contract and its
adapter requirements; update specs/mail/gmail-push.md lines 71-76 so Gmail
follows the same raw-byte hand-off and places attachment extraction and blob
ownership in the agreed layer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a2db3d7-8f2d-4885-b454-f04c920725c2
📒 Files selected for processing (2)
specs/mail/gmail-push.mdspecs/mail/inbound-ingestion.md
…sh (HT-34) All 8 findings incorporated. gmail-push: require email_verified=true on the Pub/Sub OIDC JWT; bind to the push envelope's exact subscription field; resolve emailAddress->mailbox (reject on mismatch) before reconciling; add a bounded daily history.list reconciliation sweep so dropped/delayed pushes cannot leave a mailbox stale. inbound-ingestion: attachment extraction is the pipeline's job (post-parse), not the transport's; make the ledger claim atomic (unique-key get-or-insert) and commit the store write + 'stored' outcome in one transaction (closes the concurrent-delivery and partial-failure double-create windows); loop suppression now requires a verifiable correlation (own Message-ID / valid own token) — sender identity alone never drops mail (invariant #1); 'Agent' capitalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
specs/mail/gmail-push.md (2)
140-150: 🚀 Performance & Scalability | 🔵 TrivialConsider per-mailbox mutual exclusion between push-triggered reconciliation and the daily sweep.
Push-triggered reconciliation (§3) and this daily sweep both read/advance the same stored cursor for a mailbox and can run concurrently (e.g., a push arrives mid-sweep). Per §4 each run independently gates its own cursor advance on confirmed storage, so this isn't a drop risk, but concurrent overlapping runs mean redundant
history.list/messages.getcalls and duplicate (deduped, but still wasted) ingest work against the same mailbox. A brief note on a per-mailbox lock/lease for reconciliation runs would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/mail/gmail-push.md` around lines 140 - 150, Add a brief note to the daily sweep design describing per-mailbox mutual exclusion, using a lock or lease shared by push-triggered reconciliation and the sweep. State that overlapping runs for the same mailbox must be serialized or skipped, while preserving concurrent reconciliation across different mailboxes and the existing cursor/storage guarantees.
66-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpecify what triggers §3 reconciliation after the fast ack.
The handler acks Pub/Sub quickly and "lets the reconciliation step (§3) do the fetching," but the hand-off mechanism from "durable marker recorded" to "history.list actually runs" is unspecified (queue consumer,
waitUntil-style deferred execution, separate worker poll, etc.). Since push is meant to be the near-real-time path — the §6 daily sweep is explicitly the 24h-bounded fallback, not the primary path — a silently-dropped hand-off degrades push to "eventually caught by the sweep" without anyone noticing. Worth naming the mechanism explicitly so an implementer doesn't accidentally do the fetch inline (defeating "no heavy work inline") or drop it entirely in a serverless runtime that doesn't guarantee post-response execution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/mail/gmail-push.md` around lines 66 - 73, Specify the concrete hand-off that triggers §3 reconciliation after the durable notification marker is recorded and the fast Pub/Sub acknowledgment is returned, such as an explicit queue consumer or separate worker poll. Ensure the mechanism is reliable in the target runtime, preserves near-real-time processing, and does not perform Gmail or persistence fetches inline or rely on unguaranteed post-response execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specs/mail/gmail-push.md`:
- Around line 140-150: Add a brief note to the daily sweep design describing
per-mailbox mutual exclusion, using a lock or lease shared by push-triggered
reconciliation and the sweep. State that overlapping runs for the same mailbox
must be serialized or skipped, while preserving concurrent reconciliation across
different mailboxes and the existing cursor/storage guarantees.
- Around line 66-73: Specify the concrete hand-off that triggers §3
reconciliation after the durable notification marker is recorded and the fast
Pub/Sub acknowledgment is returned, such as an explicit queue consumer or
separate worker poll. Ensure the mechanism is reliable in the target runtime,
preserves near-real-time processing, and does not perform Gmail or persistence
fetches inline or rely on unguaranteed post-response execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5d9b177-8a49-4d9d-864a-c84c71878cc7
📒 Files selected for processing (2)
specs/mail/gmail-push.mdspecs/mail/inbound-ingestion.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/mail/inbound-ingestion.md
gmail-push §2: the webhook enqueues the reconcile job onto QueueProvider (a durable hand-off, not an unguaranteed serverless post-response continuation) so a dropped hand-off can't silently degrade push to the daily sweep. gmail-push §6: serialize reconciliation per mailbox via a lease (the inbound analogue of the outbound delivery lease, sending.md §3a) so push-triggered and swept reconciliation don't do redundant work; different mailboxes stay concurrent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 14, 2026
Merged
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
1 participant
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.
Filed HT-34 [A] — the behavioral spec that pins every downstream decision in the HT-33 Gmail-OAuth inbound intake epic before any engine code touches the front door.
What's here
Two specs under
specs/mail/:inbound-ingestion.md— the provider-agnostic pipeline (raw →parseInboundEmail→decideThreading→ store). This is the orchestrationthreading.md/sending.mdkeep deferring to as "the mail-ingestion pipeline, not yet built." The future forwarding transport reuses it unchanged.gmail-push.md— the Gmail push transport (Pub/Sub receipt + security, history reconciliation, cursor,watch()renewal) — the workspace-native mode.Decisions pinned
InboundEmailProviderseam (→ HT-35).(mailboxId, providerMessageId)— not the optional, sender-controlled RFCMessage-ID.One deliberate divergence — flagged for your call
The spec does not suppress generic third-party
Auto-Submitted/bulk mail by default, becausefixtures/mail/observed/auto-submitted.jsonshows the reference system ingesting it — suppressing would diverge from an observed fixture, which charter §2 says needs explicit written justification. It's left as an explicit open question (inbound-ingestion.md§5), not smuggled in. (Codex had recommended dropping such mail; I held to the fixture instead — that's the judgment I'd most like a second opinion on.)Follow-ups (kept out of this PR to stay surgical)
threading.md§5 andSTATUS.mdstill read "the mail-ingestion pipeline (not yet built)" / "a future auto-responder spec" — both are now homed here. Happy to update those cross-refs in a small follow-up.Reviewed independently by Codex against the codebase before the epic was filed. Nothing else touched.
🤖 Generated with Claude Code
Summary by CodeRabbit