feat(rfp): 24h pending-RFP SLA breach email to leadership#864
Conversation
A triggered RFP can sit awaiting approval indefinitely with no nudge. Add an hourly worker scan that emails the RFP reviewers (leadership) once when an RFP has been pending approval for more than 24 hours. - worker/src/jobs/rfp-pending-sla.ts: scans every active office for opportunity, non-bid-board-owned deals with rfp_approval_status in (pending_outbox, pending) whose rfp_approval_requested_at is older than 24h (excludes on-hold + test-data). Emails resolveRfpReviewerEmails() — the SAME leadership set as the RFP-decline email — with a branded, Outlook-safe template (View Deal + Pending RFP queue, officeId-scoped links so cross-office reviewers don't 404). - Exactly-once + retry-safe: a claim row in public.rfp_pending_sla_email_receipts (keyed tenant_schema, deal_id, rfp_approval_requested_at) is INSERTed ON CONFLICT DO NOTHING before the send; a conflict skips (already alerted this cycle), a send failure DELETEs the claim so the next hourly scan retries. A re-triggered RFP gets a new requested_at -> a fresh cycle -> a new alert. Migration 0174 adds the ledger. - Gated behind RFP_PENDING_SLA_ENABLED (default OFF) so it merges inert; leadership opts in deliberately (flipping it on alerts on all currently-breaching RFPs once). - Hourly cron in worker/src/index.ts. Tests (runtime, CI-gated): buildRfpPendingSlaEmail (links/escaping/SLA copy), isRfpPendingSlaEnabled, findPendingRfpSlaBreaches on real SQL (PGlite — filters awaiting/>24h/opportunity/non-BBO/active/non-test/non-hold, oldest first), and the scan orchestration (inert-when-off, no-op-without-recipients, claim-then-send-once, skip-on-conflict, delete-on-send-failure). Added the shared/lib/rfpReviewerEmails vitest alias the worker was missing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
|
@codex review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new worker job that scans active tenants hourly for Pending RFP deals breaching a 24-hour SLA, sends leadership a one-time email per breach cycle using an advisory-lock single-flight and a new database receipt ledger table, wires the job into the worker cron scheduler, and adds tests plus a vitest alias. ChangesPending RFP SLA Scan Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Cron as Worker Cron
participant Scan as runRfpPendingSlaScan
participant DB as Tenant DB
participant Lock as Advisory Lock
participant Receipts as Receipt Ledger
participant Email as Email Provider
Cron->>Scan: hourly trigger (America/Chicago)
Scan->>Lock: acquireScanAdvisoryLock
Lock-->>Scan: lock acquired or null
alt lock acquired
loop each active office
Scan->>DB: findPendingRfpSlaBreaches(schema, slaHours)
DB-->>Scan: breach list
loop each breach
Scan->>Receipts: check existing receipt
Receipts-->>Scan: exists / not found
alt no receipt
Scan->>DB: isStillBreaching(deal)
DB-->>Scan: still breaching / not
alt still breaching
Scan->>Email: send(buildRfpPendingSlaEmail, idempotencyKey)
Email-->>Scan: success / failure
alt success
Scan->>Receipts: record receipt
end
end
end
end
end
Scan->>Lock: release lock
else lock not acquired
Scan-->>Cron: skip tick
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: f364e18b78
ℹ️ 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".
… (Codex #864) - P2 crash-suppression: switch from claim-first to record-AFTER-a-durable-send. The receipt is now written only once the email is accepted, so a crash between the decision and the send leaves no receipt and the next hourly scan retries (was: a committed claim could permanently suppress the alert). - P2 legacy stage aliases: resolve every stage id that canonicalizes to Opportunity (incl. `dd`) via toCanonicalDealStageSlug, matching the trigger route + pending-rfp-service, instead of requiring slug='opportunity'. - P2 override-resolved rows: exclude rfp_override_decision='denial_reconfirmed' and rfp_override_state='approving' (the NOT_RECONFIRMED_DENIAL / NOT_OVERRIDE_APPROVING guards the queue applies), so a row hidden from /deals/pending-rfp never triggers a leadership alert. - P2 stale send: re-check the deal is STILL an awaiting, unresolved breach for the same rfp_approval_requested_at cycle immediately before sending, so an approve/decline landing between the batch scan and the send doesn't fire a false alert. Tests updated: real-SQL fixture gains a `dd`-stage breach + override-resolved rows (asserted excluded); orchestration covers receipt-exists skip, re-check skip, and no-receipt-on-send-failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All four addressed:
Tests updated (real-SQL @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f12d21c4f3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f12d21c4f3
ℹ️ 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".
…d (Codex #864 r2) - P2 stage re-check: isStillBreaching now also verifies the deal's stage STILL canonicalizes to Opportunity. A deal moved out of Opportunity via POST /stage keeps its rfp_approval_status, so the status check alone would let a moved deal slip through and get a stale alert. - P2 concurrency: add a single-flight Postgres advisory lock (pg_try_advisory_lock over a dedicated pooled client) around the scan, so a still-running run or a second worker instance skips its tick instead of both submitting the same alert. - P2 idempotent retries: the email payload is now STABLE per cycle (">24h" + fixed "Pending since" instant, no drifting live hours). The Resend idempotencyKey is keyed on rfp_approval_requested_at, and Resend rejects a same-key/different-payload retry — so a crash-then-retry now re-sends cleanly instead of failing until the key TTL expires and then duplicating. - P2 test-gate stability: bump this PGlite beforeAll to a 30s hook timeout so it survives the concurrent full test:runtime gate (several PGlite suites cold-start together under maxWorkers 4). Tests: added single-flight (lock-held skip) + stage-moved-out skip cases; email assertions now check the stable ">24h" / "More than 24 hours" payload. Worker typecheck + full runtime suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Round 2 addressed (all 5):
Added single-flight + stage-moved-out test cases; worker typecheck + full runtime suite green. @codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@migrations/0174_rfp_pending_sla_email_receipts.sql`:
- Around line 5-10: The WHY block in the RFP pending SLA receipt migration
describes the old claim-first flow, but the shipped logic in rfp-pending-sla.ts
records the receipt only after a successful durable send. Update the header
comment to match the actual exactly-once behavior by referencing the worker scan
path and its receipt-recording step, and remove the claim/DELETE wording since
that protocol is not implemented.
In `@worker/src/jobs/rfp-pending-sla.ts`:
- Around line 84-93: The stage-id lookup in findPendingRfpSlaBreaches is being
repeated even though it queries shared public.pipeline_stage_config data that
does not vary per office. Refactor so opportunityStageIds is resolved once per
scan in the caller that loops over active offices, then pass the resulting ids
into findPendingRfpSlaBreaches as an argument and use them directly there; keep
the existing schema validation and empty-array fast path intact.
- Around line 214-220: The unlock cleanup currently always calls
client.release() even if client.query("SELECT pg_advisory_unlock($1)",
[LOCK_KEY]) throws, which can return a locked session to the pool. Update the
async unlock closure in the advisory-lock cleanup path to catch the query error
and pass that error into client.release(error) so the connection is destroyed
instead of reused; keep the successful path using a normal release.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15718de8-4679-4685-9bd4-6de3725eb2b7
📒 Files selected for processing (5)
migrations/0174_rfp_pending_sla_email_receipts.sqlworker/src/index.tsworker/src/jobs/rfp-pending-sla.tsworker/tests/jobs/rfp-pending-sla.runtime.test.tsworker/vitest.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build-gate
🧰 Additional context used
🪛 OpenGrep (1.23.0)
worker/tests/jobs/rfp-pending-sla.runtime.test.ts
[ERROR] 72-118: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🪛 SQLFluff (4.2.2)
migrations/0174_rfp_pending_sla_email_receipts.sql
[error] 32-33: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
🔇 Additional comments (7)
worker/src/index.ts (1)
23-23: LGTM!Also applies to: 272-284
worker/tests/jobs/rfp-pending-sla.runtime.test.ts (1)
1-264: LGTM!worker/vitest.config.ts (1)
13-15: LGTM!migrations/0174_rfp_pending_sla_email_receipts.sql (1)
19-33: LGTM!worker/src/jobs/rfp-pending-sla.ts (3)
94-121: LGTM!
223-341: LGTM!
343-471: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d52aa203a1
ℹ️ 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".
| const subject = input.dealNumber | ||
| ? `RFP pending approval >${input.slaHours}h: ${input.dealNumber} (${input.dealName})` | ||
| : `RFP pending approval >${input.slaHours}h: ${input.dealName}`; |
There was a problem hiding this comment.
When a send succeeds but the worker crashes or the receipt insert fails before the receipt is recorded, the next scan reuses the same rfp-pending-sla-...-${breach.requestedAt} idempotency key for this cycle. If the deal name or project/deal number changed in the meantime, these fields make the subject/body differ from the first Resend request, so the retry can hit the same-key/different-payload rejection and then duplicate once the key expires. Snapshot these display fields durably for the cycle or keep the idempotent payload to immutable values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d52aa203a1
ℹ️ 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".
| const subject = input.dealNumber | ||
| ? `RFP pending approval >${input.slaHours}h: ${input.dealNumber} (${input.dealName})` | ||
| : `RFP pending approval >${input.slaHours}h: ${input.dealName}`; |
There was a problem hiding this comment.
In the crash-after-provider-success/before-receipt-insert path, the next hourly run reuses the same idempotencyKey but rebuilds the subject/body from the current dealNumber and dealName. Fresh evidence in this patch is that the fixed >24h subject still interpolates live deal fields here; the normal deal edit path allows name and projectNumber to change while the same RFP cycle is still pending, so a retry can submit a different payload under the same key and be rejected or delayed instead of recording the receipt. Render from immutable cycle data or persist the attempted payload before sending.
Useful? React with 👍 / 👎.
The SLA scan's single-flight advisory lock released its pooled client in a finally that always ran, so an unlock-query error returned a possibly-still- locked session to the pool. Release with the error (release(err)) on unlock failure so pg destroys the connection instead of reusing it — Postgres frees a session-level advisory lock when its backend disconnects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ffice opportunityStageIds() reads office-invariant public.pipeline_stage_config but was called inside findPendingRfpSlaBreaches, which runs per office. Hoist the lookup to runRfpPendingSlaScan (once, before the office loop) and pass the ids into the finder via a new optional 4th param. Direct callers that omit it still resolve internally, so existing behavior + tests are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…potent processBreach rendered the SLA email from the fresh-scan deal name/number and recorded the receipt only after send. A crash after a successful send but before the insert let the next scan re-read a renamed/renumbered deal and rebuild a different subject/body under the SAME Resend idempotencyKey (rejected or duplicated after TTL). Evolve the record-after-send into a retry-safe claim: insert a claim row (carrying deal_name/deal_number as first seen) BEFORE sending, render the email from that STORED snapshot, and stamp a nullable sent_at only after a durable send. The claim is never deleted; a failed send leaves sent_at NULL so the next scan retries with the same snapshot -> a stable payload. The single-flight advisory lock keeps the claim insert + sent_at check race-free. Migration 0174: add deal_name; make sent_at nullable (no default) so the row is a claim at insert and a receipt at completion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The header still described the removed protocol ("INSERT a claim ON CONFLICT DO
NOTHING before sending, and a send failure DELETEs the claim"). Rewrite it to
the actual protocol: a single-flight advisory lock serializes runs; a claim row
(with a display snapshot) is inserted before send and is NEVER deleted; sent_at
(nullable) marks completion, so a failed send leaves sent_at NULL and the next
scan retries with the SAME snapshot for a stable Resend payload.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be274f21a7
ℹ️ 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".
| const result = await sendEmail(recipients, email.subject, email.html, { | ||
| text: email.text, | ||
| idempotencyKey: `rfp-pending-sla-${tenantSchema}-${breach.dealId}-${breach.requestedAt}`, | ||
| }); |
There was a problem hiding this comment.
When the provider accepts the email but the worker dies or the sent_at update fails, the next scan retries this same cycle with the same idempotency key but rebuilds to from the current RFP_REJECTION_EMAIL_RECIPIENTS. If that env list is edited or reordered between attempts, the retry submits a different provider payload under the same key, which can be rejected until the key expires or eventually duplicate to a different audience; store the recipient snapshot in the claim and reuse it for retries.
Useful? React with 👍 / 👎.
…nt retries The SLA breach email re-resolved RFP_REJECTION_EMAIL_RECIPIENTS on every attempt, so an env edit/reorder between a failed send and its retry changed the Resend payload's `to` while the idempotencyKey stayed the same — the provider then rejects it as a key/payload mismatch (delayed or dropped alert). Snapshot the recipient list into the claim row at claim time (the recipient_emails column already existed) alongside deal_name/deal_number, read it back, and send to the STORED set. The success UPDATE no longer re-writes recipient_emails, so the row records what was actually sent. Adds a retry-stability test covering a recipient-list change between attempts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up addressed (pushed as 2ad9904): freeze recipients for idempotent retries — the recipient set is now snapshotted into the SLA claim ( @codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What
An hourly worker scan that emails the RFP reviewers (leadership) once when an RFP has been awaiting approval for more than 24 hours. There was no existing "pending RFP" email — this is net-new (the only prior daily email is the platform-usage summary).
How
worker/src/jobs/rfp-pending-sla.ts— scans every active office foropportunity, non-bid-board-owned deals withrfp_approval_status IN (pending_outbox, pending)whoserfp_approval_requested_at(the indexed pending clock) is older than 24h. Excludes on-hold + test-data deals so a paused/demo deal never alerts. EmailsresolveRfpReviewerEmails()— the same leadership set as the RFP-decline email — with a branded, Outlook-safe template (View Deal + Pending RFP queue,officeId-scoped links so cross-office reviewers don't 404).public.rfp_pending_sla_email_receipts(keyedtenant_schema, deal_id, rfp_approval_requested_at) isINSERT … ON CONFLICT DO NOTHINGbefore the send: a conflict → skip (already alerted this cycle), a send failure →DELETEthe claim so the next hourly scan retries. A re-triggered RFP gets a newrequested_at→ a fresh cycle → a new alert. Migration 0174 adds the ledger.RFP_PENDING_SLA_ENABLED(default OFF) so it merges inert; leadership opts in deliberately.worker/src/index.ts.Flipping
RFP_PENDING_SLA_ENABLED=truewill, on the first hourly run, alert on every RFP currently past 24h pending (the ledger starts empty). That's a one-time backlog blast — expected. Recipients come from the existingRFP_REJECTION_EMAIL_RECIPIENTS; if unset the scan logs and no-ops.Deploy
Two-service, like the RFP-decline email (0148): the server applies migration 0174, the worker ships the job. Both redeploy.
Tests (runtime, CI-gated)
buildRfpPendingSlaEmail— links, officeId scoping, SLA copy, HTML escaping.isRfpPendingSlaEnabled— flag parsing (default off).findPendingRfpSlaBreacheson real SQL (PGlite) — filters awaiting / >24h / opportunity / non-BBO / active / non-test / non-hold, oldest first, project_number-first.@trock-crm/shared/lib/rfpReviewerEmailsvitest alias the worker config was missing. Worker typecheck + full runtime suite green.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests