Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a provider-error Discord notifier service, updates the worker to batch and dispatch notifications after DB commit, and expands tests to cover notify/no-notify Discord webhook scenarios and restore test globals/env. Changes
Sequence DiagramsequenceDiagram
participant Worker as Worker (batchProcessLogs)
participant DB as Database
participant Filter as Eligibility Checker
participant Service as ProviderErrorService
participant Discord as Discord Webhook
Worker->>DB: fetch batch of logs
DB-->>Worker: log rows
loop per log
Worker->>Filter: shouldNotifyProviderError(log)
alt eligible
Filter-->>Worker: true
Worker->>Worker: accumulate ProviderErrorNotificationLog
else not eligible
Filter-->>Worker: false
end
end
Worker->>DB: commit processed flags
DB-->>Worker: commit complete
loop per accumulated log
Worker->>Service: notifyProviderError(log)
Service->>Service: build embed (type, description, redactions, truncation)
Service->>Discord: POST embed JSON (webhook URL, 5s timeout)
alt 2xx
Discord-->>Service: success
else non-2xx / timeout / fetch error
Discord-->>Service: error + body
Service->>Service: catch and logger.error(logId, err)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: acd2480d33
ℹ️ 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 statusCode = logRow.error_details?.statusCode; | ||
| if (statusCode !== undefined) { | ||
| return statusCode >= 500; | ||
| } |
There was a problem hiding this comment.
shouldNotifyProviderError currently returns based on statusCode whenever error_details exists, so any upstream_error/gateway_error logged with a 4xx status (for example 429 upstream rate limits or 401/403 gateway auth failures classified in apps/gateway/src/chat/tools/get-finish-reason-from-error.ts) is silently skipped. This causes the new Discord alerting path to miss real provider/gateway failures even though they already have a non-client unified finish reason.
Useful? React with 👍 / 👎.
| const url = process.env.PROVIDER_ERROR_DISCORD_URL?.trim(); | ||
| return url ?? null; |
There was a problem hiding this comment.
getProviderErrorDiscordUrl trims the env var but returns an empty string instead of null, so notifyProviderError proceeds and calls fetch("") when PROVIDER_ERROR_DISCORD_URL is set but blank. In that configuration every notifiable log generates avoidable error noise and no notifications are delivered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds worker-side Discord notifications for provider error logs, controlled via PROVIDER_ERROR_DISCORD_URL, so upstream/gateway failures can be surfaced operationally while excluding client errors.
Changes:
- Add provider error notification logic to
batchProcessLogs()and ashouldNotifyProviderError()filter. - Introduce
provider-error-discordservice to format and send Discord webhook embeds. - Add Vitest coverage for upstream/gateway reporting, client-error exclusion, and missing finish-reason scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/worker/src/worker.ts | Collects notifiable provider-error logs during batch processing and dispatches Discord notifications post-transaction. |
| apps/worker/src/services/provider-error-discord.ts | New service to build Discord embeds and POST them to the configured webhook URL. |
| apps/worker/src/log-processing.spec.ts | Adds unit tests validating Discord reporting behavior and exclusion rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| const statusCode = logRow.error_details?.statusCode; | ||
| if (statusCode !== undefined) { |
There was a problem hiding this comment.
shouldNotifyProviderError returns early based on statusCode and only notifies for >= 500. In the gateway, upstream fetch/connection failures are logged with errorDetails.statusCode = 0 but unified_finish_reason = upstream_error (see apps/gateway/src/chat/chat.ts:6728+). With the current logic those upstream errors will never be reported to Discord. Consider treating statusCode === 0 (or generally non-HTTP / <100) as notifiable when unified_finish_reason indicates upstream/gateway error, instead of short-circuiting on the status code check.
| if (statusCode !== undefined) { | |
| if (statusCode !== undefined) { | |
| // Treat non-HTTP / sentinel status codes (e.g. 0) as upstream/gateway signals | |
| // and fall back to unified_finish_reason-based logic. | |
| if (statusCode < 100) { | |
| return ( | |
| logRow.unified_finish_reason === null || | |
| NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason) | |
| ); | |
| } | |
| // For real HTTP status codes, only notify on server errors. |
| await Promise.all( | ||
| providerErrorLogs.map(async (providerErrorLog) => { | ||
| await notifyProviderError(providerErrorLog); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Promise.all will fire one webhook request per notifiable log (up to BATCH_SIZE) concurrently. This can easily trigger Discord rate limits and can also create a burst of outbound requests from the worker. Consider adding a small concurrency limit / batching strategy (e.g., process sequentially or with a capped pool) to smooth traffic and reduce the chance of webhook failures.
| await Promise.all( | |
| providerErrorLogs.map(async (providerErrorLog) => { | |
| await notifyProviderError(providerErrorLog); | |
| }), | |
| ); | |
| for (const providerErrorLog of providerErrorLogs) { | |
| await notifyProviderError(providerErrorLog); | |
| } |
| const response = await fetch(providerErrorDiscordUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify(buildDiscordEmbed(log)), | ||
| }); |
There was a problem hiding this comment.
The Discord payload includes provider/gateway response text and model/provider strings, which may contain @everyone / @here / role mentions (untrusted input). Without allowed_mentions set, the webhook can generate unwanted pings in Discord. Consider including allowed_mentions: { parse: [] } (or equivalent) in the webhook JSON body to disable mention parsing.
| const response = await fetch(providerErrorDiscordUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify(buildDiscordEmbed(log)), | ||
| }); |
There was a problem hiding this comment.
fetch to the Discord webhook has no timeout/abort handling. If Discord (or the network) stalls, batchProcessLogs() will await these requests (via Promise.all) and the worker can get stuck processing the batch. Consider adding an AbortController with a reasonable timeout and ensuring the request is aborted/handled cleanly.
![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 the current code and only fix it if needed.
Inline comments:
In `@apps/worker/src/services/provider-error-discord.ts`:
- Around line 141-147: The fetch call in notifyProviderError (using
providerErrorDiscordUrl and buildDiscordEmbed) needs a hard timeout to avoid
hanging the Promise.all; add an AbortController, pass its signal to fetch, start
a setTimeout (e.g. 3–5s) that calls controller.abort(), and clear the timeout
after fetch completes; also catch abort/fetch errors inside notifyProviderError
so an aborted webhook or network error is logged and does not propagate to block
the caller.
In `@apps/worker/src/worker.ts`:
- Around line 880-886: The code is awaiting outbound webhook calls
(providerErrorLogs.map -> notifyProviderError) while the credit-processing lock
is still held, which can extend lock duration; to fix, stop awaiting these calls
inside the locked section by moving the notifyProviderError invocation(s) to
after the lock is released or dispatching them as non-blocking (fire-and-forget)
tasks; specifically locate the providerErrorLogs handling and either (a)
relocate the Promise.all(providerErrorLogs.map(... notifyProviderError ...)) so
it executes after the lock release point, or (b) call notifyProviderError
without await (or schedule via setImmediate/queue) so the lock-holding function
returns immediately. Ensure notifyProviderError remains called for each
providerErrorLog but no outbound-awaits occur while the lock is held.
- Around line 143-151: The current logic lets a present statusCode short-circuit
notifications which suppresses explicit provider reasons; change the decision
order so you first check logRow.unified_finish_reason (and return true if it's
null or in NOTIFIABLE_PROVIDER_ERROR_REASONS) and only if that check doesn't
trigger consult statusCode (logRow.error_details?.statusCode) returning
statusCode >= 500; update the logic around unified_finish_reason,
NOTIFIABLE_PROVIDER_ERROR_REASONS, and statusCode in the function in worker.ts
so provider error reasons always cause notification regardless of a non-5xx
status.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a7b225bc-35d2-446d-ae63-45560bdff410
📒 Files selected for processing (3)
apps/worker/src/log-processing.spec.tsapps/worker/src/services/provider-error-discord.tsapps/worker/src/worker.ts
| const statusCode = logRow.error_details?.statusCode; | ||
| if (statusCode !== undefined) { | ||
| return statusCode >= 500; | ||
| } | ||
|
|
||
| return ( | ||
| logRow.unified_finish_reason === null || | ||
| NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason) | ||
| ); |
There was a problem hiding this comment.
Don’t let HTTP status override explicit provider error reasons.
On Lines 143-146, a present statusCode short-circuits the decision. That suppresses notifications for explicit gateway_error / upstream_error whenever status is <500, which breaks the intended behavior.
🔧 Proposed fix
function shouldNotifyProviderError(logRow: ProcessedLogRow): boolean {
if (logRow.hasError !== true) {
return false;
}
if (logRow.unified_finish_reason === "client_error") {
return false;
}
- const statusCode = logRow.error_details?.statusCode;
- if (statusCode !== undefined) {
- return statusCode >= 500;
- }
-
- return (
- logRow.unified_finish_reason === null ||
- NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason)
- );
+ if (logRow.unified_finish_reason !== null) {
+ return NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason);
+ }
+
+ const statusCode = logRow.error_details?.statusCode;
+ return statusCode !== undefined && statusCode >= 500;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/worker/src/worker.ts` around lines 143 - 151, The current logic lets a
present statusCode short-circuit notifications which suppresses explicit
provider reasons; change the decision order so you first check
logRow.unified_finish_reason (and return true if it's null or in
NOTIFIABLE_PROVIDER_ERROR_REASONS) and only if that check doesn't trigger
consult statusCode (logRow.error_details?.statusCode) returning statusCode >=
500; update the logic around unified_finish_reason,
NOTIFIABLE_PROVIDER_ERROR_REASONS, and statusCode in the function in worker.ts
so provider error reasons always cause notification regardless of a non-5xx
status.
steebchen
force-pushed
the
provider-discord-pr
branch
from
acd2480 to
ce90aa2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce90aa2f54
ℹ️ 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".
| if (providerErrorLogs.length > 0) { | ||
| await Promise.all( | ||
| providerErrorLogs.map(async (providerErrorLog) => { | ||
| await notifyProviderError(providerErrorLog); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
batchProcessLogs keeps the CREDIT_PROCESSING lock until every notifyProviderError call completes. Since these webhook POSTs are network-bound and have no timeout, a slow or unreachable Discord endpoint can hold the lock for an extended period, so later worker ticks fail to acquire the lock and skip log processing/credit deductions for everyone. Send notifications after releasing the lock (or enforce a strict timeout/background queue) so billing throughput is not blocked by webhook latency.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/worker/src/worker.ts (1)
215-223:⚠️ Potential issue | 🟠 MajorHTTP status check overrides explicit provider error reasons.
When
statusCodeis present, the check on Lines 216-218 short-circuits the decision. This suppresses notifications for explicitgateway_error/upstream_errorwhenever status is <500 (e.g., 429 rate-limit errors).Consider reordering so explicit finish reasons take precedence:
🔧 Proposed fix
function shouldNotifyProviderError(logRow: ProcessedLogRow): boolean { if (logRow.hasError !== true) { return false; } if (logRow.unified_finish_reason === "client_error") { return false; } - const statusCode = logRow.error_details?.statusCode; - if (statusCode !== undefined) { - return statusCode >= 500; - } - - return ( - logRow.unified_finish_reason === null || - NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason) - ); + if (logRow.unified_finish_reason !== null) { + return NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason); + } + + const statusCode = logRow.error_details?.statusCode; + return statusCode !== undefined && statusCode >= 500; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/worker.ts` around lines 215 - 223, The logic currently short-circuits on statusCode and suppresses explicit provider finish reasons; change the order so checks on logRow.unified_finish_reason happen first: if logRow.unified_finish_reason is non-null return true when NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason) (i.e., treat explicit provider errors as notifiable regardless of HTTP status), otherwise fall back to checking logRow.error_details?.statusCode and return statusCode >= 500; if neither applies return false. Use the existing symbols logRow.unified_finish_reason, NOTIFIABLE_PROVIDER_ERROR_REASONS and logRow.error_details?.statusCode to locate and update the code path.
🧹 Nitpick comments (1)
apps/worker/src/worker.ts (1)
995-1001: Simplify redundant async wrapper.The inner
asyncfunction and nestedawaitare unnecessary sincenotifyProviderErroralready returns aPromise. This can be simplified:♻️ Suggested simplification
if (providerErrorLogs.length > 0) { await Promise.all( - providerErrorLogs.map(async (providerErrorLog) => { - await notifyProviderError(providerErrorLog); - }), + providerErrorLogs.map((providerErrorLog) => + notifyProviderError(providerErrorLog), + ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/worker.ts` around lines 995 - 1001, The map callback wrapping notifyProviderError in an unnecessary async function causes redundant await nesting; replace the providerErrorLogs.map(async (providerErrorLog) => { await notifyProviderError(providerErrorLog); }) with a direct call so Promise.all receives the notifyProviderError promises (e.g., use providerErrorLogs.map(providerErrorLog => notifyProviderError(providerErrorLog)) or simply providerErrorLogs.map(notifyProviderError)) to eliminate the extra async/await while keeping the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/worker/src/worker.ts`:
- Around line 215-223: The logic currently short-circuits on statusCode and
suppresses explicit provider finish reasons; change the order so checks on
logRow.unified_finish_reason happen first: if logRow.unified_finish_reason is
non-null return true when
NOTIFIABLE_PROVIDER_ERROR_REASONS.has(logRow.unified_finish_reason) (i.e., treat
explicit provider errors as notifiable regardless of HTTP status), otherwise
fall back to checking logRow.error_details?.statusCode and return statusCode >=
500; if neither applies return false. Use the existing symbols
logRow.unified_finish_reason, NOTIFIABLE_PROVIDER_ERROR_REASONS and
logRow.error_details?.statusCode to locate and update the code path.
---
Nitpick comments:
In `@apps/worker/src/worker.ts`:
- Around line 995-1001: The map callback wrapping notifyProviderError in an
unnecessary async function causes redundant await nesting; replace the
providerErrorLogs.map(async (providerErrorLog) => { await
notifyProviderError(providerErrorLog); }) with a direct call so Promise.all
receives the notifyProviderError promises (e.g., use
providerErrorLogs.map(providerErrorLog => notifyProviderError(providerErrorLog))
or simply providerErrorLogs.map(notifyProviderError)) to eliminate the extra
async/await while keeping the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed795612-0c9b-49f7-bf1c-9feba583b940
📒 Files selected for processing (3)
apps/worker/src/log-processing.spec.tsapps/worker/src/services/provider-error-discord.tsapps/worker/src/worker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/worker/src/services/provider-error-discord.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/worker/src/services/provider-error-discord.ts`:
- Around line 57-69: formatErrorDescription currently forwards raw responseText
and other parts of ErrorDetails to Discord; redact sensitive stable identifiers
(requestId, projectId, organizationId, traceId, logId) and any PII before
sending. Modify formatErrorDescription to sanitize errorDetails.responseText
(and any other text fields) by removing/masking those identifier keys with a
deterministic redaction (e.g., replace values with "<redacted>" or hash) using a
regex or JSON-parsing-safe sanitizer, ensure errorDetails.cause and status lines
are preserved but scrubbed, and apply the same sanitizer to the code paths that
build the payload around lines referenced (where requestId/projectId/etc. are
added) so none of those raw IDs are forwarded to the Discord webhook; use
functions like sanitizeErrorText or redactSensitiveIds and call them from
formatErrorDescription and the code that constructs the webhook payload.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a24a10b-ef1c-4e35-a81c-e9d8ec1b9ac3
📒 Files selected for processing (3)
apps/worker/src/log-processing.spec.tsapps/worker/src/services/provider-error-discord.tsapps/worker/src/worker.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ad624c672
ℹ️ 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 statusCode = logRow.error_details?.statusCode; | ||
| return statusCode !== undefined && statusCode >= 500; |
There was a problem hiding this comment.
When unified_finish_reason is missing, shouldNotifyProviderError currently only notifies for statusCode >= 500. That skips provider/gateway failures like 429/404 (upstream_error) and 401/403 (gateway_error), even though the gateway’s canonical classifier (apps/gateway/src/chat/tools/get-finish-reason-from-error.ts) treats those statuses as non-client errors. The fallback path was added specifically for missing finish reasons, so these cases will silently miss Discord alerts.
Useful? React with 👍 / 👎.
steebchen
force-pushed
the
provider-discord-pr
branch
from
1ad624c to
5a7da10
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/worker/src/worker.ts (1)
1013-1019: Optional: usePromise.allSettledto future-proof the fan-out.Today
notifyProviderErrorcatches all errors internally, soPromise.allwon't reject in practice. However, if a future refactor ever lets an error escape (e.g., a synchronous throw inbuildDiscordEmbedbefore thetry, or a change to rethrow on non-2xx),Promise.allwould abandon the remaining notifications.Promise.allSettledmakes the guarantee structural rather than reliant on the callee's internal error handling.♻️ Suggested change
- if (shouldSendProviderErrorNotifications && providerErrorLogs.length > 0) { - await Promise.all(providerErrorLogs.map(notifyProviderError)); - } + if (shouldSendProviderErrorNotifications && providerErrorLogs.length > 0) { + await Promise.allSettled(providerErrorLogs.map(notifyProviderError)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/worker.ts` around lines 1013 - 1019, The fan-out using Promise.all for provider error notifications is brittle if a future change lets notifyProviderError throw; replace the Promise.all call with Promise.allSettled to ensure all notifyProviderError calls run to completion regardless of individual failures. Locate the block that checks shouldSendProviderErrorNotifications and providerErrorLogs and change the await Promise.all(providerErrorLogs.map(notifyProviderError)) to await Promise.allSettled(providerErrorLogs.map(notifyProviderError)); keep the surrounding condition and the untouched low-balance call to checkLowBalanceAlerts(deductedOrgIds).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/worker/src/worker.ts`:
- Around line 1013-1019: The fan-out using Promise.all for provider error
notifications is brittle if a future change lets notifyProviderError throw;
replace the Promise.all call with Promise.allSettled to ensure all
notifyProviderError calls run to completion regardless of individual failures.
Locate the block that checks shouldSendProviderErrorNotifications and
providerErrorLogs and change the await
Promise.all(providerErrorLogs.map(notifyProviderError)) to await
Promise.allSettled(providerErrorLogs.map(notifyProviderError)); keep the
surrounding condition and the untouched low-balance call to
checkLowBalanceAlerts(deductedOrgIds).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa5e7557-92a7-4b95-a1c9-6780bdb778ba
📒 Files selected for processing (3)
apps/worker/src/log-processing.spec.tsapps/worker/src/services/provider-error-discord.tsapps/worker/src/worker.ts
✅ Files skipped from review due to trivial changes (1)
- apps/worker/src/log-processing.spec.ts
steebchen
force-pushed
the
provider-discord-pr
branch
from
5a7da10 to
334b5d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 334b5d1728
ℹ️ 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".
| if (deductedOrgIds.length > 0) { | ||
| void checkLowBalanceAlerts(deductedOrgIds); |
There was a problem hiding this comment.
After this change, checkLowBalanceAlerts runs outside the try block, so it executes even when the credit-deduction transaction throws and rolls back. Because deductedOrgIds is populated before the transaction completes, a late failure (for example in referral or API-key updates) can still trigger low-balance emails for orgs whose balance was never actually reduced. This creates false customer alerts and should be gated on successful transaction completion (similar to shouldSendProviderErrorNotifications).
Useful? React with 👍 / 👎.
steebchen
force-pushed
the
provider-discord-pr
branch
from
334b5d1 to
0879878
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0879878d58
ℹ️ 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 statusCode = logRow.error_details?.statusCode; | ||
| return statusCode !== undefined && statusCode >= 500; |
There was a problem hiding this comment.
When unified_finish_reason is missing, this fallback only notifies for statusCode >= 500, so provider/gateway failures like 429, 404, 401, and 403 are silently skipped. That conflicts with the gateway’s own error classifier (apps/gateway/src/chat/tools/get-finish-reason-from-error.ts), which treats those statuses as non-client errors, so missing/legacy rows with null finish reason will under-report real provider incidents.
Useful? React with 👍 / 👎.
| if (deductedOrgIds.length > 0) { | ||
| void checkLowBalanceAlerts(deductedOrgIds); |
There was a problem hiding this comment.
deductedOrgIds is populated inside the transaction before all writes complete, but checkLowBalanceAlerts now runs unconditionally after the try/catch block. If any later statement in the transaction fails and rolls back, this can still send low-balance emails for orgs whose credits were never actually deducted, producing false customer alerts.
Useful? React with 👍 / 👎.
steebchen
force-pushed
the
provider-discord-pr
branch
from
0879878 to
613c6d7
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This adds worker-side Discord notifications for provider error logs via PROVIDER_ERROR_DISCORD_URL. The worker now reports upstream and gateway failures, while continuing to exclude client errors. It also handles provider errors that are missing a unified finish reason by falling back to HTTP status classification and a generic Discord title. Tests cover upstream errors, gateway errors, client-error exclusion, and missing-finish-reason reporting.
Summary by CodeRabbit
New Features
Tests