update taskflow api - #5
Conversation
Refresh worker object after get_or_create_worker commits to ensure it stays attached to the session before accessing worker.id in ensure_default_project. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add session.refresh() calls after commits to ensure objects remain attached to the async session before accessing their attributes. Files fixed: - projects.py: refresh worker and project after commits - members.py: refresh member_worker after commits - user_setup.py: already fixed in previous commit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The issue was that after a commit, SQLAlchemy objects become detached and accessing their attributes triggers lazy loading which fails in async context. Solution: Pass worker_id as int instead of worker object to ensure_default_project. This avoids any attribute access on potentially detached objects. Key changes: - ensure_default_project now accepts worker_id: int instead of worker: Worker - ensure_user_setup extracts worker.id before calling ensure_default_project - Final refresh of worker before returning ensures it's attached 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The database stores naive datetimes but API requests may include timezone-aware datetimes (e.g., "2025-12-07T10:28:03.581Z"). Added field validators to TaskCreate and TaskUpdate that convert timezone-aware datetimes to naive UTC datetimes before storing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The validator needs to run AFTER Pydantic parses the string into a datetime object. mode='before' runs before parsing, so the value is still a string and the timezone stripping doesn't happen. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Root cause: log_action() was calling session.commit() after each audit log entry, which detached all SQLAlchemy objects. Subsequent access to detached object attributes triggered lazy loading in async context, causing MissingGreenlet errors. Solution: - audit.py: Remove commit from log_action(), add actor_type param - All routers: Extract primitive values (worker_id, worker_type) BEFORE any commits, use session.flush() for IDs, single commit at end of operations - tasks.py assign_task: Build TaskRead directly with extracted assignee_handle instead of passing detached Worker object All 41 tests pass, all endpoints manually verified. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Swagger UI sends 0 for optional int fields by default, which causes foreign key violations since worker ID 0 doesn't exist. Added a validator to convert 0 to None for these nullable FK fields. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the taskflow API to improve transaction management and avoid SQLAlchemy detached object issues. The changes focus on consolidating database commits, extracting primitive values before transactions, and explicitly passing actor type information to audit logs.
Key Changes:
- Consolidated multiple commits into single transactions by using
flush()to get IDs without committing - Extract primitive values (IDs, types) immediately from ORM objects to prevent detached object issues
- Updated
log_action()to acceptactor_typeas a parameter instead of querying it, removing unnecessary database lookups
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/api/src/taskflow_api/services/user_setup.py | Updated ensure_default_project to accept worker_id as int parameter instead of Worker object; added session refresh to prevent detached objects |
| packages/api/src/taskflow_api/services/audit.py | Removed Worker import and database lookup; added actor_type parameter with default value; removed commit/refresh logic to allow caller to control transactions |
| packages/api/src/taskflow_api/schemas/task.py | Added strip_timezone() utility function and field validators for converting 0 to None for foreign keys and normalizing datetime values |
| packages/api/src/taskflow_api/routers/tasks.py | Extracted worker IDs and types before commits; consolidated commits with flush pattern; updated all log_action calls to pass actor_type; replaced some task_to_read calls with manual TaskRead construction |
| packages/api/src/taskflow_api/routers/projects.py | Extracted worker and project IDs before commits; consolidated transaction commits; updated audit log calls with actor_type parameter |
| packages/api/src/taskflow_api/routers/members.py | Extracted worker IDs and member details before commits; consolidated commits; updated audit logging with explicit actor_type |
| packages/api/src/taskflow_api/routers/agents.py | Extracted worker IDs and agent details before commits or deletions; consolidated transaction commits; updated audit log calls with actor_type parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return None | ||
| if dt.tzinfo is not None: | ||
| # Convert to UTC and strip timezone | ||
|
|
There was a problem hiding this comment.
Remove this unnecessary blank line between the comment and the code. It disrupts the flow and is inconsistent with coding style conventions.
| await session.refresh(membership) | ||
|
|
||
| # Audit log | ||
| # Get member details for response before commit |
There was a problem hiding this comment.
Potential None dereference: member_worker could theoretically be None at this point if neither the agent_id nor user_id branches are executed (though the validation at lines 81-84 should prevent this). Consider adding an assertion or explicit check before accessing member_worker.handle, member_worker.name, and member_worker.type to make the code more robust and satisfy type checkers:
if not member_worker:
raise HTTPException(status_code=500, detail="Internal error: member_worker not initialized")| # Get member details for response before commit | |
| # Get member details for response before commit | |
| if not member_worker: | |
| raise HTTPException(status_code=500, detail="Internal error: member_worker not initialized") |
Constitution v1.0.4: - Added Section III: Technical Implementation Patterns - Async SQLAlchemy MissingGreenlet prevention patterns - API input validation (0→None, timezone stripping) - Transaction boundary principles (caller owns commit) SQLModel skill: - Added "Critical: Async Session Patterns" section - Extract → Flush → Commit pattern with examples - Service functions should never commit internally - Added input validation patterns (0→None, datetime) - New Common Pitfall #5: MissingGreenlet after commit These patterns were discovered fixing production bugs in the backend API and are now documented to prevent future issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
No description provided.