Fix duplicate resource errors from throwing 500 instead of user frien…#2262
Conversation
…dly error message
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 85f6790 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
🟠 1) packages/agents-core/src/utils/error.ts:293-308 Missing unit tests for new helper functions
Issue: The new isUniqueConstraintError() and throwIfUniqueConstraintError() functions have no dedicated unit tests. These functions detect unique constraint violations via three patterns: PostgreSQL code 23505, Doltgres MySQL errno 1062, and "already exists" message fallback.
Why: These functions are critical error-handling utilities used across 7 route handlers. The existing integration tests run against pglite (PostgreSQL), so only the '23505' path is tested. The Doltgres '1062' pattern and "already exists" fallback are never exercised in tests. If the detection logic regresses, Doltgres deployments would return 500 instead of 409 for duplicate resources.
Fix: Add unit tests in packages/agents-core/src/utils/__tests__/error.test.ts:
import { describe, it, expect } from 'vitest';
import { isUniqueConstraintError, throwIfUniqueConstraintError } from '../error';
import { HTTPException } from 'hono/http-exception';
describe('isUniqueConstraintError', () => {
it('should detect PostgreSQL unique violation code 23505', () => {
const error = { cause: { code: '23505' } };
expect(isUniqueConstraintError(error)).toBe(true);
});
it('should detect Doltgres MySQL errno 1062 in cause message', () => {
const error = { cause: { message: 'Error 1062: Duplicate entry' } };
expect(isUniqueConstraintError(error)).toBe(true);
});
it('should detect "already exists" in error message', () => {
const error = { message: 'Resource already exists' };
expect(isUniqueConstraintError(error)).toBe(true);
});
it('should return false for unrelated errors', () => {
expect(isUniqueConstraintError({ message: 'Connection timeout' })).toBe(false);
expect(isUniqueConstraintError(null)).toBe(false);
});
});
describe('throwIfUniqueConstraintError', () => {
it('should throw 409 HTTPException for unique constraint errors', () => {
const error = { cause: { code: '23505' } };
expect(() => throwIfUniqueConstraintError(error, 'Resource conflict'))
.toThrow(HTTPException);
});
it('should not throw for non-unique-constraint errors', () => {
const error = { message: 'Network error' };
expect(() => throwIfUniqueConstraintError(error, 'ignored')).not.toThrow();
});
});Refs:
🟡 Minor (2) 🟡
🟡 1) packages/agents-core/src/utils/error.ts:301 Broad "already exists" fallback pattern
Issue: The generic fallback err?.message?.includes('already exists') can match errors unrelated to database unique constraints (e.g., file system errors, application validation, downstream service errors).
Why: False positives could convert legitimate 500 errors into misleading 409 Conflict responses. However, this preserves existing behavior from the routes being refactored.
Fix: Consider making the fallback more specific in a follow-up, e.g., checking for cause structure or more specific message patterns like "unique constraint" or "duplicate key".
Refs: error.ts:301
🟡 2) packages/agents-work-apps/src/slack/routes/oauth.ts:334-338 Missed migration: oauth.ts still uses manual duplicate detection
Issue: The PR standardizes unique constraint detection with isUniqueConstraintError, but oauth.ts in the same package still uses manual string matching.
Why: Creates inconsistency within agents-work-apps where users.ts was updated but oauth.ts was not.
Fix: Update oauth.ts to use isUniqueConstraintError for consistency:
// Replace lines 334-338
if (isUniqueConstraintError(dbError)) {Refs: oauth.ts:334-338
💭 Consider (2) 💭
Inline Comments:
- 💭 Consider:
projects.ts:261Stale comment still mentions "PostgreSQL" only - 🟡 Minor:
projectFull.ts:196Error logged at ERROR level before unique constraint check
🧹 While You're Here (2) 🧹
🧹 1) packages/agents-work-apps/src/slack/routes/users.ts:261-265 Inconsistent API response shape on race condition
Issue: When isUniqueConstraintError catches a duplicate, the endpoint returns { success: true } but omits fields (linkId, slackUsername, etc.) that are returned on successful creation.
Why: Callers can't distinguish between new creation vs pre-existing mapping. Pre-existing issue, not introduced by this PR.
Fix: Consider fetching and returning the existing mapping for consistent response shape.
Refs: users.ts:261-265
🧹 2) agents-api/src/domains/manage/routes/subAgent*.ts Inconsistent error codes for duplicates
Issue: The new helper uses 409 conflict for duplicate resources, but several subAgent relation routes use 422 unprocessable_entity for the same semantic condition.
Why: Inconsistent error taxonomy for API consumers. Pre-existing pattern, not introduced by this PR.
Fix: Consider standardizing on 409 for all duplicate/conflict scenarios, or documenting the intentional distinction.
Refs: subAgentRelations.ts:202-206
💡 APPROVE WITH SUGGESTIONS
Summary: Good refactoring that centralizes duplicate resource error handling and improves user experience by returning proper 409 Conflict responses instead of 500 errors. The main actionable item is adding unit tests for the new helper functions to ensure Doltgres error detection doesn't regress. The changeset is well-written and the implementation is clean.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
AgentSession.ts:1799-1804 |
Manual duplicate detection not using new helper | Different error-handling context (silent session management) where throwing isn't appropriate |
| Integration test lenient assertion | Test accepts 409 or 500 | Pre-existing workaround; cleaning it up is out of scope for this PR |
| Positive feedback on naming | Helper naming follows patterns | Not an issue - informational only |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 0 | 1 |
pr-review-errors |
3 | 1 | 1 | 1 | 1 | 0 | 0 |
pr-review-consistency |
5 | 1 | 1 | 1 | 1 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 10 | 3 | 2 | 2 | 2 | 0 | 2 |
Note: pr-review-standards returned no findings, indicating the code quality and AGENTS.md compliance are good.
| @@ -258,12 +259,7 @@ app.openapi( | |||
| ); | |||
| } catch (error: any) { | |||
| // Handle duplicate project (PostgreSQL unique constraint violation) | |||
There was a problem hiding this comment.
💭 Consider: Stale comment
Issue: The comment still mentions "PostgreSQL unique constraint violation" but the new helper handles both PostgreSQL and Doltgres errors.
Why: Outdated comments can mislead future maintainers about the actual behavior.
Fix:
| // Handle duplicate project (PostgreSQL unique constraint violation) | |
| } catch (error: any) { | |
| throwIfUniqueConstraintError(error, 'Project with this ID already exists'); |
Refs:
| @@ -193,14 +194,8 @@ app.openapi( | |||
|
|
|||
| return c.json({ data: createdProject }, 201); | |||
| } catch (error: any) { | |||
There was a problem hiding this comment.
🟡 Minor: Error logging at wrong level for expected user errors
Issue: logger.error() is called before checking for unique constraint errors, meaning expected duplicate creation attempts (which return 409 to users) are logged at ERROR level.
Why: Logging expected user errors at ERROR level creates noise in monitoring, potentially triggering unnecessary alerts and making it harder to spot real issues.
Fix: Move the error log after the unique constraint check, or log at INFO level for duplicates:
| } catch (error: any) { | |
| } catch (error: any) { | |
| if (isUniqueConstraintError(error)) { | |
| logger.info({ projectId: projectData.id }, 'Duplicate project creation attempted'); | |
| throwIfUniqueConstraintError(error, `Project with ID '${projectData.id}' already exists`); | |
| } | |
| logger.error({ error }, 'Error creating project'); |
Note: This would require importing isUniqueConstraintError separately from throwIfUniqueConstraintError.
Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
This is a delta re-review. The previous review raised 4 issues — 3 have been addressed in the follow-up commits, 1 remains applicable.
✅ Findings Addressed Since Last Review
| Prior Finding | Status | Resolution |
|---|---|---|
🟠 Major: Missing unit tests for isUniqueConstraintError |
✅ Addressed | Comprehensive test suite added in error.test.ts (106 lines, 15 test cases covering PostgreSQL 23505, Doltgres 1062, "already exists" fallback, and edge cases) |
🟡 Minor: oauth.ts not migrated to use helper |
✅ Addressed | Now imports and uses isUniqueConstraintError consistently |
| 💭 Consider: Stale comment mentioning only PostgreSQL | ✅ Addressed | Comment removed in projects.ts |
🕐 Pending Recommendations (1)
- 🟡
projectFull.ts:196Error logging at ERROR level before unique constraint check (expected user errors logged as errors creates monitoring noise)
✅ APPROVE
Summary: Excellent follow-up work! The unit tests are thorough and well-structured, covering all three detection patterns (PostgreSQL 23505, Doltgres 1062, and the "already exists" fallback). The oauth.ts migration was completed as suggested. The one pending item (error logging level) is minor and doesn't block — it can be addressed in a follow-up if desired. Ship it! 🚀
Discarded (0)
No items discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| None dispatched | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: This was a delta re-review where all new changes addressed prior feedback. No new sub-reviewers were needed.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
This is a delta re-review (3rd review cycle). The changes since the last review are merge commits from main only (#2085 Slack workspace default agent clear/reset, #2159 scope config types refactor) — no modifications to this PR's duplicate error handling feature.
✅ PR Status
All prior feedback has been addressed except one minor item:
| Prior Finding | Current Status |
|---|---|
| 🟠 Major: Missing unit tests | ✅ Addressed (106-line test suite added) |
🟡 Minor: oauth.ts not using helper |
✅ Addressed |
| 💭 Consider: Stale PostgreSQL comment | ✅ Addressed |
| 🟡 Minor: Error logging level | ⏳ Still applicable (non-blocking) |
🕐 Pending Recommendations (1)
- 🟡
projectFull.ts:196Error logging at ERROR level before unique constraint check (expected user errors logged as errors creates monitoring noise)
✅ APPROVE
Summary: No new issues in this delta — just merge commits from main. The original feature is well-tested and ready to ship. The one pending logging-level suggestion is minor and can be addressed in a follow-up if desired. 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| None dispatched | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta contained only merge commits from main — no sub-reviewers needed.
amikofalvy
left a comment
amikofalvy
left a comment
There was a problem hiding this comment.
Ship this, but we agreed to work on some broader fixes for this class of problems later this week
2 participants
…dly error message