More robust handling of checking guardrailLlm client type #32
Conversation
steven10a
requested review from
Copilot and
gabor-openai
and removed request for
Copilot
steven10a
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the moderation guardrail error handling to use the executionFailed flag instead of throwing exceptions or returning custom error messages. This change standardizes error handling by delegating error-propagation decisions to the runGuardrails function via the raiseGuardrailErrors flag.
- Removed
instanceoftype checking for the context client, relying on try-catch for validation - Changed all error paths to return
executionFailed: truewithoriginalExceptioninstead of throwing - Updated error handling to preserve the actual error message rather than using generic fallback messages
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/checks/moderation.ts | Refactored error handling to use executionFailed flag and originalException instead of throwing errors, removed instanceof check for client validation |
| src/tests/unit/checks/moderation-secret-keys.test.ts | Updated tests to verify executionFailed flag and originalException are set correctly, added test for API key error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function ensureError(error: unknown): Error { | ||
| return error instanceof Error ? error : new Error(String(error)); | ||
| } |
There was a problem hiding this comment.
The ensureError helper function is duplicating error normalization logic that exists in user-defined-llm.ts (line 139). Consider extracting this to a shared utility module to maintain consistency and reduce code duplication across guardrail implementations.
There was a problem hiding this comment.
[nit] I don't think we need to address this
Fixes reported [BUG]
instanceofcheck confirming theguardrailLlmis an OpenAI client is fragile and can fail under different resolution paths or uses. This check was removed as it is not needed, we test for the moderation endpoint directly, falling back to creating a new client withenv varsif the endpoint doesn't exit.runGuardrailsmethod rather than using the Guardrails client.Note: The current fallback is not a very robust way to handle the base client not being an OpenAI client. A PR should follow that allows the user to set an OpenAI api key for guardrails that require them.