pipe SQL errors into the existing onError method using a new SqlError class#768
Conversation
🦋 Changeset detectedLatest commit: 3d7670c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Claude Code ReviewSummary: Clean implementation that properly routes SQL errors through the existing error handling mechanism. The approach is solid and aligns well with the codebase patterns. Issues Found: None Analysis:
Testing: No dedicated tests for SqlError, but the class is simple enough and implicitly tested through existing sql method tests in oauth2-mcp-client.test.ts. Consider adding explicit error case tests if SQL error handling becomes critical in production. The PR is ready to merge. |
27b965b to
377299b
Compare
377299b to
42ec18c
Compare
Allow users to override onSqlError(query, error) to customize logging behavior for SQL errors instead of using console.error directly. Fixes #767
42ec18c to
7b72e17
Compare
commit: |
Document the new onSqlError hook that allows users to customize SQL error logging behavior. This hook provides: - Ability to override the default console.error logging - Access to both the failed query and error details - Use cases for integrating with monitoring services Related PR: cloudflare/agents#768 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| // Execute the SQL query with the provided values | ||
| return [...this.ctx.storage.sql.exec(query, ...values)] as T[]; | ||
| } catch (e) { | ||
| console.error(`failed to execute sql query: ${query}`, e); |
There was a problem hiding this comment.
instead why don't we use our logger directly here?
There was a problem hiding this comment.
agree with piping, done that
Address review feedback by piping SQL errors through onError with a SqlError class that preserves query context, rather than adding a separate onSqlError hook. - Add SqlError class with query and cause properties - Update sql() catch block to throw onError(new SqlError(query, e)) - Remove onSqlError method Users can now handle SQL errors in onError by checking instanceof SqlError.
Add documentation for the new SqlError class that is thrown when SQL queries fail in Agents. This helps developers understand how to handle SQL-specific errors through the onError method. Related to cloudflare/agents#768 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
threepointone
left a comment
There was a problem hiding this comment.
approving pending CI
Instead of adding a separate onSqlError hook, I implemented a cleaner approach (suggested by @threepointone) that pipes SQL errors into the existing onError method using a new SqlError class
Fixes #767