fix: test isolation and proper error handling for constraint violations #600

amikofalvy · 2025-10-13T20:39:43Z

Summary

This PR addresses critical test isolation issues and improves error handling in the agents-manage-api:

Test Isolation Fix ✅

Fixed vitest.config.ts: Changed isolate: false to isolate: true
Problem: All test files were sharing the same in-memory database worker context
Impact: This caused spurious SQLITE_CONSTRAINT_PRIMARYKEY errors across unrelated tests
Solution: Each test file now gets its own isolated database instance, eliminating cross-test contamination

Error Handling Improvements ✅

Fixed src/routes/projects.ts: Corrected SQLite error detection logic
Problem: Code was checking error.code instead of error.cause.code for SQLite constraint violations
Impact: Duplicate project creation was returning 500 Internal Server Error instead of proper 409 Conflict
Solution: Now properly detects primary key violations and returns appropriate HTTP status codes

Test Quality Improvements ✅

Fixed src/__tests__/routes/crud/projects.test.ts: Removed workaround that masked the error handling bug
Reduced test output noise: Tool discovery failures in test environment now log at debug level instead of error
Modified packages/agents-core/src/data-access/tools.ts: Added conditional logging based on ENVIRONMENT variable

Test Results

Test Files  22 passed | 2 skipped (24)
      Tests  371 passed | 51 skipped (422)

✅ All tests passing
✅ Eliminated spurious primary key constraint errors
✅ Dramatically reduced ERROR log output during test runs
✅ Tests now strictly validate correct HTTP status codes

Technical Details

SQLite Error Structure:

// WRONG ❌
if (error.code === 'SQLITE_CONSTRAINT_PRIMARYKEY')

// CORRECT ✅
if (error.cause.code === 'SQLITE_CONSTRAINT_PRIMARYKEY')

Test Isolation:

// BEFORE: Shared database across all test files ❌
isolate: false

// AFTER: Each test file gets own database ✅
isolate: true

Files Changed

agents-manage-api/vitest.config.ts - Test isolation fix
agents-manage-api/src/routes/projects.ts - Error handling correction
agents-manage-api/src/__tests__/routes/crud/projects.test.ts - Removed workaround
packages/agents-core/src/data-access/tools.ts - Conditional logging for tests

Future Work

A follow-up PR will address remaining instances where the API returns 500 for predictable errors (foreign key violations, validation errors) that should use proper HTTP status codes (400, 404, 409).

🤖 Generated with Claude Code

This commit addresses test isolation issues and improves error handling: **Test Isolation Fix:** - Fixed vitest.config.ts: Changed `isolate: false` to `isolate: true` - Previously, all test files shared the same in-memory database worker context - This caused spurious SQLITE_CONSTRAINT_PRIMARYKEY errors across tests - Now each test file gets its own isolated database instance **Error Handling Improvements:** - Fixed projects.ts: Corrected SQLite error detection - Changed from checking `error.code` to `error.cause.code` - Now properly returns 409 Conflict for duplicate project IDs instead of 500 - SQLite constraint violations are in `error.cause.code`, not `error.code` **Test Quality:** - Removed workaround in projects.test.ts that accepted both 409 and 500 - Tests now strictly expect correct HTTP status codes - Reduced test output noise by using debug level for expected tool discovery failures in test environment **Test Results:** - All 371 tests passing (51 skipped) - Eliminated spurious primary key constraint errors - Dramatically reduced ERROR log output during test runs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

changeset-bot · 2025-10-13T20:39:48Z

⚠️ No Changeset found

Latest commit: e8a07f1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2025-10-13T20:39:51Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
agents-manage-api	Ready	Preview	Comment	Oct 13, 2025 8:42pm
agents-manage-ui	Ready	Preview	Comment	Oct 13, 2025 8:42pm
agents-run-api	Ready	Preview	Comment	Oct 13, 2025 8:42pm

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Skipped			Oct 13, 2025 8:42pm

claude · 2025-10-13T20:40:00Z

Claude finished @amikofalvy's task —— View job

Code Review Complete ✅

I've conducted a comprehensive review of the test isolation and error handling fixes. Here's my detailed analysis:

Summary

This is a high-quality, well-targeted fix that addresses two critical issues: test isolation problems and incorrect SQLite error handling. The changes are minimal, surgical, and thoroughly tested.

✅ Strengths

1. Excellent Problem Identification & Root Cause Analysis

Test Isolation Issue: Correctly identified that isolate: false was causing cross-test contamination through shared in-memory database contexts
Error Handling Bug: Properly diagnosed that SQLite errors have their codes in error.cause.code, not error.code
Both issues had clear symptoms and the fixes directly address root causes

2. Surgical, Minimal Changes

Only 4 files changed with just +16/-22 lines
Each change serves a specific purpose with no unnecessary modifications
Changes are focused and don't introduce unrelated improvements

3. Consistent Error Handling Pattern

The fix aligns with existing codebase patterns. I found other files already using the correct error?.cause?.code approach:

artifactComponents.ts:150 ✅ error?.cause?.code === 'SQLITE_CONSTRAINT_PRIMARYKEY'
projectFull.ts ✅ error?.cause?.code === 'SQLITE_CONSTRAINT_PRIMARYKEY'
apiKeys.ts ✅ error?.cause?.code === 'SQLITE_CONSTRAINT_FOREIGNKEY'

4. Proper Test Quality Improvements

Removed workaround in projects.test.ts that accepted both 409 and 500 status codes
Tests now validate correct behavior - expecting exactly 409 for duplicate IDs
Excellent test coverage with 371 passing tests validates the fix works

🤔 Areas for Consideration

1. Inconsistency Across Projects

Issue: agents-run-api/vitest.config.ts still has isolate: false while agents-manage-api now uses isolate: true

Risk: The same test isolation issues could occur in agents-run-api

Recommendation: Consider updating agents-run-api/vitest.config.ts to also use isolate: true for consistency and to prevent similar issues

2. Incomplete Error Handling Migration

I found one location that might still have the old pattern:

agents-run-api/src/handlers/executionHandler.ts:71 uses error?.code === 'SQLITE_CONSTRAINT_PRIMARYKEY'

Recommendation: Review and potentially update this to use error?.cause?.code for consistency

3. Error Structure Documentation

Observation: The SQLite error structure (error.cause.code vs error.code) isn't well documented

Suggestion: Consider adding a code comment explaining why .cause.code is used, as this pattern might not be obvious to future developers

🏗️ Architectural Considerations

Test Isolation Strategy

The move to isolate: true is architecturally sound:

✅ Eliminates race conditions between test files
✅ Ensures deterministic test behavior
✅ Prevents data contamination across test suites
⚠️ Performance trade-off: Tests may run slightly slower due to worker isolation

Error Handling Consistency

The codebase now has a consistent approach to SQLite constraint handling, which is excellent for:

Maintainability: Developers know the pattern to follow
Reliability: Proper error classification ensures correct HTTP status codes
API Contract: Clients get predictable 409 responses for conflicts

🧪 Testing Excellence

Test Strategy

Integration tests properly validate the full request/response cycle
Error scenarios are well covered (duplicate creation, not found, etc.)
Tenant isolation is thoroughly tested
Pagination and CRUD operations have comprehensive coverage

Test Quality Improvements

The removal of the 409/500 status code workaround is particularly good:

// BEFORE (problematic workaround):
expect([409, 500]).toContain(res2.status);

// AFTER (correct validation):
expect(res2.status).toBe(409);

🔒 Security & Data Integrity

Proper Constraint Validation

✅ Primary key violations properly return 409 Conflict
✅ Tenant isolation is maintained and tested
✅ Foreign key constraints are handled consistently across the codebase

Database Safety

✅ Foreign keys enabled in test setup (PRAGMA foreign_keys = ON)
✅ Transaction boundaries are properly managed
✅ No SQL injection risks (using proper ORM patterns)

💡 Recommendations

Consistency Fix: Update agents-run-api/vitest.config.ts to use isolate: true
Complete Migration: Review executionHandler.ts for the old error pattern
Documentation: Add comments explaining the SQLite error structure
Future Work: The PR mentions addressing remaining 500 errors - this should be prioritized

🎯 Conclusion

This is an exemplary bug fix that demonstrates:

Clear problem analysis with proper root cause identification
Minimal, focused changes that don't over-engineer the solution
Thorough testing that validates the fix works correctly
Good architectural thinking around test isolation

The changes are ready for production and will significantly improve test reliability and API error handling consistency.

Recommendation: ✅ APPROVE - This PR addresses critical issues with surgical precision and excellent test coverage.

vercel bot deployed to Preview – agents-manage-api October 13, 2025 20:40 View deployment

vercel bot deployed to Preview – agents-run-api October 13, 2025 20:40 View deployment

vercel bot deployed to Preview – agents-manage-ui October 13, 2025 20:42 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: test isolation and proper error handling for constraint violations #600

fix: test isolation and proper error handling for constraint violations #600

Uh oh!

amikofalvy commented Oct 13, 2025

Uh oh!

changeset-bot bot commented Oct 13, 2025

Uh oh!

vercel bot commented Oct 13, 2025 •

edited

Loading

Uh oh!

claude bot commented Oct 13, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fix: test isolation and proper error handling for constraint violations #600

Are you sure you want to change the base?

fix: test isolation and proper error handling for constraint violations #600

Uh oh!

Conversation

amikofalvy commented Oct 13, 2025

Summary

Test Isolation Fix ✅

Error Handling Improvements ✅

Test Quality Improvements ✅

Test Results

Technical Details

Files Changed

Future Work

Uh oh!

changeset-bot bot commented Oct 13, 2025

⚠️ No Changeset found

Uh oh!

vercel bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

Summary

✅ Strengths

1. Excellent Problem Identification & Root Cause Analysis

2. Surgical, Minimal Changes

3. Consistent Error Handling Pattern

4. Proper Test Quality Improvements

🤔 Areas for Consideration

1. Inconsistency Across Projects

2. Incomplete Error Handling Migration

3. Error Structure Documentation

🏗️ Architectural Considerations

Test Isolation Strategy

Error Handling Consistency

🧪 Testing Excellence

Test Strategy

Test Quality Improvements

🔒 Security & Data Integrity

Proper Constraint Validation

Database Safety

💡 Recommendations

🎯 Conclusion

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel bot commented Oct 13, 2025 •

edited

Loading

claude bot commented Oct 13, 2025 •

edited

Loading