fix(spec): correct email-delivery grounding — SendGrid, signal payload, result-not-offer (BH-1062)#104
Conversation
…l payload, result-not-offer (BH-1062)
Multi-agent architecture audit found the spec contracted against a system that
doesn't exist. Corrected against the real code:
- IMailService is bound to SendgridMailService (policy template locked), NOT
SesMailService. Spec now requires template parameterization + a routine_result
template; SES is framed as an explicit-rebind alternative, not 'reuse'.
- The workflow_suggestion signal carries only {title, routine_suggestion_id} —
proposed_delivery + recipients are dropped. PR-1 now explicitly extends the
signal payload; PR-2 keys on it.
- OFFER-time suggestions have no ownership/recipients (set at schedule). Re-scoped
from offer email to SCHEDULED-routine-RESULT email, which has recipients AND a
real off-means-off gate (turn-off deletes the cron → no result signal → no email).
§1.1 explains the offer→result pivot; §6 flags the SendGrid-vs-SES decision as
blocking PR-2; §11 reorders so payload + template land before the fan-out.
📝 WalkthroughWalkthroughChangesScheduled-routine email delivery
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 3f9df8ea91
ℹ️ 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".
| "proposed_delivery": suggestion.proposed_delivery.value, # net-new on the wire | ||
| "recipient_user_ids": suggestion.recipient_user_ids, # net-new; scheduled → non-empty |
There was a problem hiding this comment.
This serializes suggestion.proposed_delivery.value, but the parent BrightRoutines spec defines RoutineSuggestion.proposed_delivery as a DeliveryConfig, and the scheduled-agent payloads carry delivery as a config object such as {"webapp": true, "slack": true}, not a DeliveryHint enum. An implementation following this example will either fail while building the signal or drop schedule-time delivery edits; the spec needs to define the email-bearing DeliveryConfig shape (for example an email flag) and route on that shape instead of {EMAIL, ALL} enum values.
Useful? React with 👍 / 👎.
| `build_routine_suggestion_signal` (and the result-signal equivalent) must add | ||
| the two fields the email branch keys on. Counts-only still holds — these are |
There was a problem hiding this comment.
For the scheduled-result scope, this assigns the new metadata to build_routine_suggestion_signal in brightbot, but the result notification is emitted by the platform-core terminal bridge (brightroutines-execute-workflow-schedule.md lines 406-411 publish scheduled_workflow_success/error). If PR-1 only changes brightbot's offer signal, the scheduled-result notifications—the only path this spec now emails—still lack proposed_delivery and recipients, so the platform-core email branch has nothing to key on.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/brightroutines-email-delivery.md (1)
247-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not use an unconditional send
iffproperty.
ALLauthorizes the email path; it does not guarantee a sent message when there are no recipients, addresses cannot be resolved, or sends fail. Change this to “email delivery is attempted only whenproposed_delivery ∈ {EMAIL, ALL},” with the documented recipient and failure conditions.🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 247 - 249, Update Property 1, “Email only when opted in,” to state that email delivery is attempted only when proposed_delivery is EMAIL or ALL, rather than asserting that an email is always sent. Include the documented conditions that no recipients, unresolved addresses, or send failures can prevent successful delivery, while preserving the existing opt-in scope.
🤖 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 `@docs/specs/brightroutines-email-delivery.md`:
- Around line 119-127: Update the signal_publisher.py metadata schema to include
the evidence counts required by INV-4 and §4, or define the authoritative lookup
the email adapter must use to obtain them. Ensure PR-2 can produce a counts-only
body from this explicit contract without relying on unspecified data.
- Around line 131-141: Update the IMailService.sendMail contract to preserve
legacy {to, subject, body, from} requests for policy emails, defining how they
map to the new policy template request without changing rendered output. Specify
adapter handling for both legacy and template-based policy calls, and add a
compatibility test covering the old request shape.
- Around line 135-160: Add language identifiers to the three fenced
pseudo-contract blocks in the email delivery specification, including the
sendMail contract, deliverRoutineEmail contract, and resolveVerifiedEmails
contract. Use an appropriate language tag or text so each fence satisfies MD040
without changing the contract content.
- Around line 184-190: Update the §2.4 email-only scheduled routine result
scenario to define a stable per-result, per-recipient idempotency key, specify
that delivery records are persisted durably before or with sending, and require
retries to skip recipients whose key is already marked delivered while retrying
only undelivered recipients.
- Around line 46-48: Clarify the missing-email handling in the email delivery
specification around the UserNode.emailAddress guarantee: define whether an
unresolvable address represents legacy or invalid data, an external lookup miss,
or a missing member. Align the behavior described in sections 2.5 and 4.5 with
that definition, including whether the recipient is skipped.
---
Outside diff comments:
In `@docs/specs/brightroutines-email-delivery.md`:
- Around line 247-249: Update Property 1, “Email only when opted in,” to state
that email delivery is attempted only when proposed_delivery is EMAIL or ALL,
rather than asserting that an email is always sent. Include the documented
conditions that no recipients, unresolved addresses, or send failures can
prevent successful delivery, while preserving the existing opt-in scope.
🪄 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
Run ID: 4c3ecaa4-0904-4e3d-851c-e9096dd6996f
📒 Files selected for processing (1)
docs/specs/brightroutines-email-delivery.md
| routine ran — the loop is invisible to them. Email is the lowest-common- | ||
| denominator reach: every `UserNode` carries a non-null, unique `emailAddress` | ||
| in Neo4j (`typedefs.ts` — `emailAddress: EmailAddress! @unique`). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the spec and inspect the relevant sections.
git ls-files 'docs/specs/brightroutines-email-delivery.md' 'typedefs.ts' || true
echo '--- spec outline ---'
ast-grep outline docs/specs/brightroutines-email-delivery.md || true
echo '--- relevant spec excerpts ---'
nl -ba docs/specs/brightroutines-email-delivery.md | sed -n '1,220p'
echo '--- search for missing-email / no resolvable email / UserNode / emailAddress ---'
rg -n "missing-email|no resolvable email|UserNode|emailAddress|non-null|`@unique`|legacy|invalid" docs/specs/brightroutines-email-delivery.md typedefs.ts || trueRepository: brighthive/agentic-project-mgmt
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect just the sections referenced by the review comment if the file is large.
nl -ba docs/specs/brightroutines-email-delivery.md | sed -n '35,90p'Repository: brighthive/agentic-project-mgmt
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact references to sections 2.5 and 4 mentioned in the comment.
rg -n "^#{1,6} .*|2\.5|4\.|missing email|resolvable email|workspace member" docs/specs/brightroutines-email-delivery.md || trueRepository: brighthive/agentic-project-mgmt
Length of output: 1733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If typedefs.ts exists, inspect the email field declaration only.
if [ -f typedefs.ts ]; then
nl -ba typedefs.ts | sed -n '1,220p'
fiRepository: brighthive/agentic-project-mgmt
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=150 && NR<=215 {print NR ":" $0}' docs/specs/brightroutines-email-delivery.mdRepository: brighthive/agentic-project-mgmt
Length of output: 3875
Clarify the missing-email path. This spec says every UserNode.emailAddress is non-null, but §2.5 and §4.5 require skipping a recipient with no resolvable emailAddress. Define whether that case is legacy/invalid data, an external lookup miss, or a missing member, or the acceptance path remains inconsistent.
🧰 Tools
🪛 LanguageTool
[grammar] ~46-~46: Ensure spelling is correct
Context: ... Slack workspace never learns a routine ran — the loop is invisible to them. Email is the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 46 - 48, Clarify
the missing-email handling in the email delivery specification around the
UserNode.emailAddress guarantee: define whether an unresolvable address
represents legacy or invalid data, an external lookup miss, or a missing member.
Align the behavior described in sections 2.5 and 4.5 with that definition,
including whether the recipient is skipped.
| ```python | ||
| # signal_publisher.py — metadata gains: | ||
| { | ||
| "title": suggestion.title, | ||
| "routine_suggestion_id": suggestion.routine_suggestion_id, | ||
| "proposed_delivery": suggestion.proposed_delivery.value, # net-new on the wire | ||
| "recipient_user_ids": suggestion.recipient_user_ids, # net-new; scheduled → non-empty | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the evidence counts required by the email contract.
INV-4 and §4 require a counts-only body containing evidence counts, but this wire payload defines only title, ID, delivery intent, and recipient IDs. Add an explicit counts/data schema or define the authoritative lookup before PR-2; otherwise the adapter cannot produce the required body.
🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 119 - 127, Update
the signal_publisher.py metadata schema to include the evidence counts required
by INV-4 and §4, or define the authoritative lookup the email adapter must use
to obtain them. Ensure PR-2 can produce a counts-only body from this explicit
contract without relying on unspecified data.
| `IMailService.sendMail` today takes `{to, subject, body, from}` and the bound | ||
| `SendgridMailService` forces the policy template. Extend the port to select a | ||
| template so a routine email doesn't render as a policy email: | ||
|
|
||
| ``` | ||
| sendMail({ to, from, templateKey: "policy" | "routine_result", data: object }) -> Result | ||
| ``` | ||
| # Consumes the routine suggestion signal; when proposed_delivery includes EMAIL, | ||
| # renders and sends via the existing IMailService. | ||
|
|
||
| `policy` preserves the existing behavior byte-for-byte (back-compat). The | ||
| adapter maps `routine_result` → a new SendGrid dynamic template (or, if SES is | ||
| chosen instead, an explicit re-bind + SES provisioning — decide in §6). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define a genuinely backward-compatible policy request shape.
The new contract removes {subject, body} while claiming the policy path remains byte-for-byte compatible. Existing callers using the old shape will break unless the port/adapters retain it or the spec defines an explicit mapping and migration. Add that compatibility contract and test.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 131 - 141, Update
the IMailService.sendMail contract to preserve legacy {to, subject, body, from}
requests for policy emails, defining how they map to the new policy template
request without changing rendered output. Specify adapter handling for both
legacy and template-based policy calls, and add a compatibility test covering
the old request shape.
| ``` | ||
| sendMail({ to, from, templateKey: "policy" | "routine_result", data: object }) -> Result | ||
| ``` | ||
| # Consumes the routine suggestion signal; when proposed_delivery includes EMAIL, | ||
| # renders and sends via the existing IMailService. | ||
|
|
||
| `policy` preserves the existing behavior byte-for-byte (back-compat). The | ||
| adapter maps `routine_result` → a new SendGrid dynamic template (or, if SES is | ||
| chosen instead, an explicit re-bind + SES provisioning — decide in §6). | ||
|
|
||
| ### 2.4 Notification fan-out — email branch (`platform-core` — PR-2) | ||
|
|
||
| ``` | ||
| # Consumes the result signal; only when proposed_delivery ∈ {EMAIL, ALL}. | ||
| deliverRoutineEmail(input: { | ||
| workspaceId: ID, | ||
| routineSuggestionId: ID, | ||
| recipientUserIds: [ID], # already carried on the suggestion's ownership | ||
| title: string, # counts-only payload, matching Slack/webapp | ||
| }) -> { delivered: Boolean, skippedReason?: "no_verified_email" | "not_email_channel" } | ||
| recipientUserIds: [ID], # from the signal (2.2), set at schedule time | ||
| title: string, # counts-only, matching Slack/webapp | ||
| }) -> { delivered: Boolean, skippedReason?: "no_verified_email" | "not_email_channel" | "no_recipients" } | ||
| ``` | ||
|
|
||
| ### 2.3 Recipient resolution | ||
| ### 2.5 Recipient resolution | ||
|
|
||
| ``` | ||
| # user IDs (owner + recipientUserIds, workspace-membership-validated) → addresses | ||
| # member-validated user IDs → addresses | ||
| resolveVerifiedEmails(userIds: [ID], workspaceId: ID) -> [EmailAddress] | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the contract fences.
The fenced blocks at Lines 135, 145, and 157 omit language tags, triggering MD040. Use an appropriate language or text for pseudo-contracts.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 135 - 160, Add
language identifiers to the three fenced pseudo-contract blocks in the email
delivery specification, including the sendMail contract, deliverRoutineEmail
contract, and resolveVerifiedEmails contract. Use an appropriate language tag or
text so each fence satisfies MD040 without changing the contract content.
Source: Linters/SAST tools
| Scenario: Email-only scheduled routine result reaches its recipients | ||
| Given a SCHEDULED routine with proposed_delivery = EMAIL | ||
| And each recipient has an emailAddress and is a workspace member | ||
| When the routine runs and produces a result | ||
| Then exactly one email per recipient is sent | ||
| And it renders via the routine_result template, not the policy template | ||
| And the body contains the routine title and no row-level data |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '150,230p' docs/specs/brightroutines-email-delivery.md && printf '\n--- SEARCH ---\n' && rg -n "idempot|dedup|replay|delivery key|run/result|exactly one email|partial success|retry" docs/specs -SRepository: brighthive/agentic-project-mgmt
Length of output: 22112
🏁 Script executed:
sed -n '100,170p' docs/specs/brightroutines-email-delivery.md && printf '\n--- FULL-FILE SEARCH ---\n' && rg -n "idempot|dedup|replay|retry|duplicate|exactly one" docs/specs/brightroutines-email-delivery.md -SRepository: brighthive/agentic-project-mgmt
Length of output: 2978
Define a stable idempotency key for result-email fan-out. In docs/specs/brightroutines-email-delivery.md §2.4, recipient resolution is defined, but not how a retried result signal avoids resending emails already delivered on a partial failure. Add a durable delivery key/dedup rule and persistence behavior.
🤖 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 `@docs/specs/brightroutines-email-delivery.md` around lines 184 - 190, Update
the §2.4 email-only scheduled routine result scenario to define a stable
per-result, per-recipient idempotency key, specify that delivery records are
persisted durably before or with sending, and require retries to skip recipients
whose key is already marked delivered while retrying only undelivered
recipients.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f9df8e. Configure here.
| "title": suggestion.title, | ||
| "routine_suggestion_id": suggestion.routine_suggestion_id, | ||
| "proposed_delivery": suggestion.proposed_delivery.value, # net-new on the wire | ||
| "recipient_user_ids": suggestion.recipient_user_ids, # net-new; scheduled → non-empty |
There was a problem hiding this comment.
DeliveryConfig versus DeliveryHint mismatch
High Severity
The RoutineSuggestion.proposed_delivery field is defined as DeliveryConfig in brightroutines-intent-loop.md, but the changes in §2.1 and §2.2 treat it as a DeliveryHint enum. This type mismatch means the new EMAIL/ALL hints may not be correctly stored or serialized, as proposed_delivery.value is not valid for DeliveryConfig.
Reviewed by Cursor Bugbot for commit 3f9df8e. Configure here.
| that is a prerequisite, not part of this spec. | ||
| - **Deliverability** (only if SES is chosen in (b)): staging SES sandbox status | ||
| must be checked (`aws sesv2 get-account`) — out-of-sandbox or per-recipient | ||
| verified before rollout. Moot under (a) SendGrid. |
There was a problem hiding this comment.
Property one ignores result-only scope
Medium Severity
Property 1 states email is sent for "any suggestion" with EMAIL or ALL delivery hints. This conflicts with the revised scope, which explicitly limits email delivery to scheduled routine results only, not initial offers.
Reviewed by Cursor Bugbot for commit 3f9df8e. Configure here.
| "title": suggestion.title, | ||
| "routine_suggestion_id": suggestion.routine_suggestion_id, | ||
| "proposed_delivery": suggestion.proposed_delivery.value, # net-new on the wire | ||
| "recipient_user_ids": suggestion.recipient_user_ids, # net-new; scheduled → non-empty |
There was a problem hiding this comment.
recipient_user_ids missing on DTO
Medium Severity
The signal payload in §2.2 references suggestion.recipient_user_ids, but the RoutineSuggestion DTO (intent-loop §3) doesn't carry this field. Recipient IDs are only available after a routine is scheduled, stored in the ownership/Dynamo mirror, not directly on the RoutineSuggestion DTO.
Reviewed by Cursor Bugbot for commit 3f9df8e. Configure here.
| Given a routine suggestion with proposed_delivery = BOTH | ||
| When the suggestion is offered | ||
| Given a SCHEDULED routine with proposed_delivery = BOTH | ||
| When the routine runs |
There was a problem hiding this comment.
Run scenarios lack channel contracts
Medium Severity
§4 requires webapp inbox rows and Slack cards when a SCHEDULED routine runs (ALL and BOTH), but this spec only defines deliverRoutineEmail on the result signal. Intent-loop documents workflow_suggestion for offers, not result-run fan-out.
Reviewed by Cursor Bugbot for commit 3f9df8e. Configure here.
1 participant
Follow-up to #100. A multi-agent architecture audit found the merged email-delivery spec contracted against a system that doesn't exist. Corrects it against real code:
Supersedes the relevant sections #100 merged. Grounding verified against inversify.config.ts, sendgrid-mail-service.ts, signal_publisher.py, notifications.ts.
Note
Low Risk
Spec and acceptance-criteria edits only; no runtime or application code changes in this PR.
Overview
Documentation-only follow-up to the merged email-delivery spec: it replaces assumptions that don’t match production with code-grounded requirements.
Mail path: Drops “reuse SES
IMailService” as the default. The spec now statesSendgridMailServiceis bound with a policy-locked template, requirestemplateKey+routine_result, and treatsSesMailServiceas an explicit re-bind alternative.Wire format: Adds §2.2 so PR-1 must put
proposed_deliveryandrecipient_user_idson routine/result signals (today only title + id).Product scope: Re-scopes email to scheduled-routine results, not offers (§1.1): recipients and turn-off suppression exist only after schedule. INV-2, Gherkin, out-of-scope, tests, and e2e are updated accordingly.
Delivery API:
deliverRoutineEmailis defined on the result signal, addsno_recipients, andresolveVerifiedEmailsis tied tofilterRecipientsToWorkspaceMembers+ Neo4jemailAddress.Rollout: §11 orders brightbot (enum + payload) → platform-core (template + fan-out) → e2e, with a blocking SendGrid vs SES decision in §6.
Reviewed by Cursor Bugbot for commit 3f9df8e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation