-
Notifications
You must be signed in to change notification settings - Fork 7
Add error-causes library usage to all server middleware and library code #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Created ai/rules/javascript/error-causes.mdc with comprehensive guidelines - Updated with-auth.js to use createError from error-causes - Updated with-cors.js to use createError from error-causes - Updated lib/release-helpers.js to use createError from error-causes - Updated all corresponding tests to verify error.cause properties Co-authored-by: ericelliott <364727+ericelliott@users.noreply.github.com>
- Update error-causes rule to use double quotes consistently - Rename 'version' to 'requestedVersion' in error metadata for clarity Co-authored-by: ericelliott <364727+ericelliott@users.noreply.github.com>
|
@cursoragent please review |
🔬 Code Review: Error-Causes Library ImplementationExecutive SummaryThis PR successfully standardizes error handling across server middleware and library code using the 🎯 1. Code Structure and Organization✅ Strengths:
📋 Observations:
🪞 2. Adherence to JavaScript Best Practices✅ Excellent adherence to project style guide:
Code Examples: if (!auth) {
throw createError({
name: "ValidationError",
message: "auth is required. Pass your better-auth instance.",
code: "MISSING_AUTH_INSTANCE",
});
} if (!shouldUpdateLatestTag(version)) {
throw createError({
name: "ValidationError",
message: `Cannot update latest tag: ${version} is a prerelease version`,
code: "PRERELEASE_VERSION",
requestedVersion: version,
});
}✅ 3. Test Coverage and QualityExcellent Test Coverage: All error paths are properly tested with assertions on:
Test Quality Assessment (per TDD guide): The tests answer all 5 critical questions:
Example: assert({
given: "no configuration",
should: "throw error with cause",
actual: error instanceof Error && error.cause !== undefined,
expected: true,
});
assert({
given: "no configuration",
should: "have ValidationError name in cause",
actual: error.cause.name,
expected: "ValidationError",
});
assert({
given: "no configuration",
should: "have MISSING_AUTH_INSTANCE code in cause",
actual: error.cause.code,
expected: "MISSING_AUTH_INSTANCE",
});⚡ 4. Performance ConsiderationsNo Performance Concerns:
🔐 5. Security Analysis (OWASP Top 10 Review)I've thoroughly reviewed the changes against the current OWASP Top 10 (2021): A01:2021 - Broken Access Control ✅
A02:2021 - Cryptographic Failures ✅
A03:2021 - Injection ✅
Security Highlight - CORS Implementation: const getOrigin = (requestOrigin) => {
// Security: Never allow null origin (can be exploited)
if (requestOrigin === "null") {
return null;
}
if (allowedOrigins === "*") return "*";
if (typeof allowedOrigins === "string") {
return allowedOrigins === requestOrigin ? requestOrigin : null;
}
if (Array.isArray(allowedOrigins)) {
return allowedOrigins.includes(requestOrigin) ? requestOrigin : null;
}
return null;
};Excellent security practice: Explicitly rejects "null" origin string which can be exploited in certain attack scenarios. A04:2021 - Insecure Design ✅
A05:2021 - Security Misconfiguration ✅
A06:2021 - Vulnerable Components ✅
A07:2021 - Authentication Failures ✅
A08:2021 - Software and Data Integrity ✅
A09:2021 - Logging Failures ✅
A10:2021 - Server-Side Request Forgery ✅
🛡️ Security Verdict: No security vulnerabilities found 🏗️ 6. Architectural Patterns✅ Excellent Consistency:
Pattern Consistency Example: Both throw createError({
...ValidationError,
message: `Source ai/ folder not found at: ${source}`,
}); throw createError({
name: "ValidationError",
message: "auth is required. Pass your better-auth instance.",
code: "MISSING_AUTH_INSTANCE",
});📚 7. Documentation Quality✅ Comprehensive Documentation: The
Docblock Quality:
📝 8. Commit Message QualityRecent commits: Note: These messages don't follow the conventional commit format specified in the project's commit.md guide. However, this is acceptable for review feedback commits. 🔍 Deep Scan: Additional Observations✅ No Dead Code
✅ No Missing Files
✅ SimplicityFollowing "perfection is attained when there is nothing left to remove":
💡 Suggestions for Improvement1. TypeScript Type Definitions (Minor)The interface CausedError extends Error {
cause: {
name: string;
code?: string;
message: string;
[key: string]: any;
};
}2. Error Cause Test Pattern (Enhancement)Consider adding a test helper to reduce boilerplate: const assertErrorCause = ({ error, name, code, messageIncludes }) => {
assert({
given: "error thrown",
should: "have cause",
actual: error instanceof Error && error.cause !== undefined,
expected: true,
});
assert({
given: "error cause",
should: `have name ${name}`,
actual: error.cause.name,
expected: name,
});
if (code) {
assert({
given: "error cause",
should: `have code ${code}`,
actual: error.cause.code,
expected: code,
});
}
if (messageIncludes) {
assert({
given: "error message",
should: `include "${messageIncludes}"`,
actual: error.cause.message.includes(messageIncludes),
expected: true,
});
}
};However, this might violate the TDD guide's principle of test explicitness. The current approach is acceptable. 3. Release Helpers Test Coverage (Enhancement)The test("prerelease version error includes cause metadata", async () => {
const prereleaseVersion = "1.2.3-rc.1";
const result = await updateLatestTag({
version: prereleaseVersion,
dryRun: true,
});
assert({
given: "prerelease version",
should: "include version in error message",
actual: result.message.includes(prereleaseVersion),
expected: true,
});
});4. CORS Configuration Test (Edge Case)Consider testing the scenario where test("throws for invalid allowedOrigins type", () => {
let error;
try {
createWithCors({ allowedOrigins: 123 }); // number instead of string/array
} catch (e) {
error = e;
}
// assertions...
});However, TypeScript should prevent this at compile time. 📊 Review Summary Scorecard
✅ Final VerdictAPPROVED - This PR is ready to merge. The implementation successfully standardizes error handling across the codebase using the error-causes library. The code quality is excellent, follows all project style guides, includes comprehensive tests, and introduces no security vulnerabilities. Key Achievements:
Minor Suggestions: The suggestions above are optional enhancements, not blockers. Great work! 🎉 Learn more about Cursor Agents |
|
Cursor Agent can help with this pull request. Just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR standardizes error handling across server middleware and library code by adopting the error-causes library for structured error metadata, replacing plain Error objects with errors that include explicit names, codes, and contextual properties.
Key Changes:
- Added comprehensive AI rule documentation for error-causes usage patterns
- Updated middleware factories (with-auth, with-cors) to use createError for parameter validation
- Enhanced library code (release-helpers) to use createError with cause preservation for wrapped errors
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ai/rules/javascript/error-causes.mdc | Comprehensive documentation for error-causes library usage, including import patterns, error properties, wrapping caught errors, factory validation, testing patterns, and error handler patterns |
| src/server/middleware/with-auth.js | Updated both createWithAuth and createWithOptionalAuth to throw ValidationError with structured metadata when auth parameter is missing |
| src/server/middleware/with-auth.test.js | Updated tests to verify error.cause properties (name, code, message) instead of checking top-level error messages |
| src/server/middleware/with-cors.js | Updated createWithCors to throw ConfigurationError with structured metadata when allowedOrigins parameter is missing |
| src/server/middleware/with-cors.test.js | Updated tests to verify error.cause properties (name, code, message) instead of checking top-level error messages |
| lib/release-helpers.js | Updated validateVersionForLatestTag and performLatestTagUpdate to use createError with ValidationError and GitOperationError respectively, preserving original error via cause property |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Standardizes error handling across server middleware and library code using the
error-causeslibrary for structured error metadata instead of plainErrorobjects.Changes
AI Rule Documentation
ai/rules/javascript/error-causes.mdc: Comprehensive usage guidelinescausepreservationerror.causepropertieserrorCauses()Server Middleware Updates
with-auth.js: Factory parameter validation (2 sites)with-cors.js: CORS configuration validation (1 site)All errors now include structured metadata:
Library Code
lib/release-helpers.js: Version validation and git operation errors (2 sites)causepropertyTest Updates
with-auth.test.js: Verifyerror.cause.name,error.cause.codewith-cors.test.js: Verify structured error propertiesExample test pattern:
Benefits
instanceoffragilitycausepropertywith-config.jsandcli-core.jspatternsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Note
Replace plain Errors with error-causes across middleware and release helpers; add AI rule docs and update tests to assert cause-based metadata.
ai/rules/javascript/error-causes.mdcwith guidelines, patterns, and testing conventions forerror-causes.src/server/middleware/with-auth.js: UsecreateErrorfor factory validation (MISSING_AUTH_INSTANCE).src/server/middleware/with-cors.js: UsecreateErrorfor CORS config validation (MISSING_ALLOWED_ORIGINS).lib/release-helpers.js: UsecreateErrorfor version validation (PRERELEASE_VERSION) and wrap git failures with cause (GIT_OPERATION_FAILED).with-auth.test.js,with-cors.test.js: Update assertions to verifyerror.cause.name,error.cause.code, and messages.Written by Cursor Bugbot for commit 929974c. This will update automatically on new commits. Configure here.