fix: improve moderation error logs - #1932
Conversation
WalkthroughReplaced ad-hoc console error logging with structured Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/openai-content-filter.ts (1)
285-312: Consider guarding against circular cause references.The depth limit of 5 prevents runaway iteration, but a malformed error with
error.cause === error(or an indirect cycle within 5 hops) would still loop. This is an edge case, but adding aSetto track visited errors would make it fully defensive.♻️ Optional defensive fix
function getErrorCode(error: unknown): string | undefined { if (!(error instanceof Error)) { return undefined; } const directCode = typeof (error as ErrorWithCode).code === "string" ? (error as ErrorWithCode).code : undefined; if (directCode) { return directCode; } + const seen = new Set<unknown>([error]); let current = (error as ErrorWithCode).cause; for (let depth = 0; depth < 5; depth++) { - if (!(current instanceof Error)) { + if (!(current instanceof Error) || seen.has(current)) { return undefined; } + seen.add(current); if (typeof (current as ErrorWithCode).code === "string") { return (current as ErrorWithCode).code; } current = (current as ErrorWithCode).cause; } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/openai-content-filter.ts` around lines 285 - 312, The getErrorCode function can loop on circular error.cause chains; update its traversal in getErrorCode to track visited error objects (e.g., a Set<Error>) and check this set before descending into (error as ErrorWithCode).cause so you break/return undefined if you encounter an already-seen Error, while keeping the existing shallow depth limit; reference the getErrorCode function, the ErrorWithCode type and the use of .cause when adding the visited guard.
🤖 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/gateway/src/chat/tools/openai-content-filter.ts`:
- Around line 285-312: The getErrorCode function can loop on circular
error.cause chains; update its traversal in getErrorCode to track visited error
objects (e.g., a Set<Error>) and check this set before descending into (error as
ErrorWithCode).cause so you break/return undefined if you encounter an
already-seen Error, while keeping the existing shallow depth limit; reference
the getErrorCode function, the ErrorWithCode type and the use of .cause when
adding the visited guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7d62ab5-921d-44ca-8784-cddacd702e64
📒 Files selected for processing (2)
apps/gateway/src/chat/tools/openai-content-filter.spec.tsapps/gateway/src/chat/tools/openai-content-filter.ts
There was a problem hiding this comment.
Pull request overview
Improves observability for the gateway’s OpenAI moderation/content-filter helper by moving error logging to the shared structured logger and enriching logs with nested cause/code details, with unit tests covering key failure modes.
Changes:
- Replaced raw
console.errormoderation error logging with@llmgateway/logger. - Added extraction of nested error cause and error code for moderation request failures.
- Added unit tests for fetch-failure logging and missing-credential logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/gateway/src/chat/tools/openai-content-filter.ts | Switches moderation error logging to structured logger and adds error detail extraction (cause/code). |
| apps/gateway/src/chat/tools/openai-content-filter.spec.ts | Adds focused tests asserting structured logging for fetch failures and missing credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function buildModerationErrorDetails( | ||
| error: unknown, | ||
| ): Record<string, string | undefined> { | ||
| if (!(error instanceof Error)) { | ||
| return {}; | ||
| } | ||
|
|
||
| return { | ||
| error: error.message, | ||
| errorName: error.name, | ||
| errorCause: extractErrorCause(error), | ||
| errorCode: getErrorCode(error), | ||
| }; |
There was a problem hiding this comment.
buildModerationErrorDetails drops all error info when the caught value is not an Error, which means logModerationError can emit a moderation error log without any error field (regression vs the previous String(error) fallback). Consider adding a fallback field (e.g., error: String(error) / errorType) for non-Error throwables so logs remain actionable.
| function getErrorCode(error: unknown): string | undefined { | ||
| if (!(error instanceof Error)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const directCode = | ||
| typeof (error as ErrorWithCode).code === "string" | ||
| ? (error as ErrorWithCode).code | ||
| : undefined; | ||
| if (directCode) { | ||
| return directCode; | ||
| } | ||
|
|
||
| let current = (error as ErrorWithCode).cause; | ||
| for (let depth = 0; depth < 5; depth++) { | ||
| if (!(current instanceof Error)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (typeof (current as ErrorWithCode).code === "string") { | ||
| return (current as ErrorWithCode).code; | ||
| } | ||
|
|
||
| current = (current as ErrorWithCode).cause; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
getErrorCode (and the ErrorWithCode shape) duplicates the same logic already present in normalize-streaming-error.ts. Consider extracting a shared helper (e.g., extract-error-code.ts) so the code/code-depth behavior stays consistent across the gateway and avoids future drift.
![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: b6ff373d72
ℹ️ 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".
| apiKeyId: context.apiKeyId, | ||
| ...payload, | ||
| }); | ||
| ...buildModerationErrorDetails(error), |
There was a problem hiding this comment.
logModerationError always spreads buildModerationErrorDetails(error) into the payload, even when callers pass no throwable (e.g., the non-OK and invalid-response branches in runOpenAIContentFilterRequest). In those paths this now logs error: "undefined" and errorName: "UndefinedThrownValue", which misclassifies normal HTTP/parse moderation failures as missing exceptions and can skew alerting or log-based analytics. Only attach derived error fields when an actual thrown value is provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/openai-content-filter.spec.ts (1)
682-683: Assert logger invocation before indexingmock.calls[0].This will produce clearer failures and avoid relying on non-null assertion if the call unexpectedly doesn’t happen.
Suggested tweak
- const [eventName, payload, loggedError] = loggerErrorSpy.mock.calls[0]!; + expect(loggerErrorSpy).toHaveBeenCalledTimes(1); + const [eventName, payload, loggedError] = loggerErrorSpy.mock.calls[0]!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/openai-content-filter.spec.ts` around lines 682 - 683, The test accesses loggerErrorSpy.mock.calls[0] with a non-null assertion which can crash the test and produce unclear failures; before destructuring the call, add an assertion that loggerErrorSpy was called (e.g., expect(loggerErrorSpy).toHaveBeenCalled() or toHaveBeenCalledTimes(1)) so the test fails clearly if no call occurred, then destructure the first call into [eventName, payload, loggedError] and assert eventName === "gateway_content_filter_error".
🤖 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/gateway/src/chat/tools/openai-content-filter.spec.ts`:
- Around line 682-683: The test accesses loggerErrorSpy.mock.calls[0] with a
non-null assertion which can crash the test and produce unclear failures; before
destructuring the call, add an assertion that loggerErrorSpy was called (e.g.,
expect(loggerErrorSpy).toHaveBeenCalled() or toHaveBeenCalledTimes(1)) so the
test fails clearly if no call occurred, then destructure the first call into
[eventName, payload, loggedError] and assert eventName ===
"gateway_content_filter_error".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d90eee0c-2e50-4de7-95ec-efde1f872ece
📒 Files selected for processing (2)
apps/gateway/src/chat/tools/openai-content-filter.spec.tsapps/gateway/src/chat/tools/openai-content-filter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/chat/tools/openai-content-filter.ts
Summary
fetch failedlogs show the real root causeVerification
Notes
pnpm formatstill fails in this workspace because of unrelated pre-existing lint errors outside this change, including inapps/worker/src/worker.tsSummary by CodeRabbit
Tests
Bug Fixes