feat(shell): add custom Base Image support for Bot Shell #222
Conversation
- Add baseImage and baseShellRef fields to ShellSpec schema - Update public shells init data with default baseImage - Create Shell unified API endpoints (/shells/unified, CRUD operations) - Add image validation API to check compatibility with shell types - Modify task dispatch to pass baseImage to Executor Manager - Add executor binary extraction on Executor Manager startup - Support Base Image + Executor mount mode in Docker executor - Create frontend Shell API client and management UI components - Add Shells tab to Settings page with i18n support The feature enables users to create custom shells with their own base images while using the latest executor binary via Named Volume mount.
WalkthroughAdds a unified "shells" API with CRUD and image validation, executor-manager image validation tasks and binary extraction, shell schema/fixture updates, a new ImageValidator agent, backend callback forwarding, and frontend UI + i18n for shell management and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Frontend (ShellEdit/List)
participant API as Backend (/shells)
participant EM as Executor Manager
participant TaskProc as Task Processor / Agent
participant Docker as Docker Engine
participant Redis as Redis (validation status)
User->>UI: request image validation (image, shell_type)
UI->>API: POST /shells/validate-image
API->>EM: POST /executor-manager/images/validate
EM->>TaskProc: create/submit validation task (negative id, validation_id)
TaskProc->>Docker: start container (ImageValidatorAgent) using target/base image
Docker-->>TaskProc: per-check outputs (stdout/logs)
TaskProc->>EM: send callback(s) including validation results / progress
EM->>API: POST /shells/validation-status/{validation_id} (forwarded callback)
API->>Redis: store/update validation status
UI->>API: GET /shells/validation-status/{validation_id} (poll)
API->>Redis: return current validation status
UI->>User: display progress and final validation_result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
- Fix ShellList Tag variant error (use 'default' instead of 'outline') - Move image validation logic to Executor Manager API (/executor-manager/images/validate) - Backend shells API now proxies validation requests to Executor Manager - Support various deployment modes (Docker, K8s) where backend may not have direct Docker access
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
executor_manager/executors/docker/binary_extractor.py (1)
72-74: Uselogger.exception()to capture stack traces.When logging exceptions,
logger.exception()automatically includes the stack trace, which aids debugging.except Exception as e: - logger.error(f"Error during executor binary extraction: {e}") + logger.exception(f"Error during executor binary extraction: {e}") return FalseApply similar changes at lines 169 and 172.
executor_manager/routers/routers.py (3)
189-194: Add proper type hints for list fields.The
checksanderrorsfields should use typed lists for better IDE support and validation.+from typing import List + class ValidateImageResponse(BaseModel): """Response for image validation""" valid: bool - checks: list - errors: list + checks: List[ImageCheckResult] + errors: List[str]
217-224: Confusing response:valid=Truewith items inerrorslist.Returning
valid=Truewhile populating theerrorslist sends mixed signals. Consider using a different field likeinfoornotesfor informational messages.if shell_type == "Dify": return ValidateImageResponse( valid=True, checks=[], - errors=["Dify is an external_api type and doesn't require image validation"], + errors=[], )Or add an
infofield to the response model for non-error messages.
342-343: Silent exception swallowing hides version comparison failures.The
passstatement silently ignores version parsing errors, which could hide misconfigurations or unexpected version formats.except Exception: - pass # Skip version comparison on error + logger.warning(f"Failed to parse version for {check['name']}: {version}") + # Treat as pass since dependency existsbackend/app/schemas/kind.py (1)
115-116: Consider adding validation for the baseImage field.The new optional fields are well-structured and backward compatible. However, consider adding Pydantic validation for
baseImageto ensure it follows Docker image naming conventions (e.g.,registry/image:tag).Example validation:
from pydantic import field_validator import re class ShellSpec(BaseModel): """Shell specification""" runtime: str supportModel: Optional[List[str]] = None baseImage: Optional[str] = None baseShellRef: Optional[str] = None @field_validator('baseImage') @classmethod def validate_base_image(cls, v: Optional[str]) -> Optional[str]: if v is None: return v # Basic Docker image pattern validation pattern = r'^[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*(:[\w][\w.-]*)?(@sha256:[a-f0-9]{64})?$' if not re.match(pattern, v, re.IGNORECASE): raise ValueError('Invalid Docker image format') return vfrontend/src/features/settings/components/ShellEdit.tsx (1)
29-45: Consider using react-hook-form with zod validation for form management.The component uses manual useState for form fields and custom validation logic. As per coding guidelines, React forms should use
react-hook-formandzodfor validation. This would provide better validation patterns, cleaner error handling, and improved form state management.Current implementation works but could be refactored for consistency with the codebase conventions.
frontend/src/features/settings/components/ShellList.tsx (1)
64-81: Consider adding loading state during delete operation.The delete handler doesn't disable the confirm button or show a loading indicator, which could allow double-clicks or confusion during slow network conditions. Consider adding a
deletingstate similar to thesavingstate in ShellEdit.+ const [deleting, setDeleting] = useState(false) const handleDelete = async () => { if (!deleteConfirmShell) return + setDeleting(true) try { await shellApis.deleteShell(deleteConfirmShell.name) toast({ title: t('shells.delete_success'), }) setDeleteConfirmShell(null) fetchShells() } catch (error) { toast({ variant: 'destructive', title: t('shells.errors.delete_failed'), description: (error as Error).message, }) + } finally { + setDeleting(false) } }frontend/src/apis/shells.ts (1)
115-128: Consider efficiency of filter helpers for large shell lists.
getPublicShellsandgetLocalEngineShellsfetch all shells and filter client-side. This is acceptable for small lists but could be inefficient if the shell count grows. Consider adding server-side filtering endpoints if this becomes a performance concern.backend/app/api/endpoints/adapter/shells.py (3)
143-148: Consider catching a more specific exception for shell parsing.The broad
Exceptioncatch is used when parsing shells, which could mask unexpected errors. Consider catchingValidationErrorfrom Pydantic specifically, sinceShellCRD.model_validateis the likely failure point.+from pydantic import BaseModel, ValidationError for shell in public_shells: try: result.append(_public_shell_to_unified(shell)) - except Exception as e: + except ValidationError as e: logger.warning(f"Failed to parse public shell {shell.name}: {e}")
433-439: Dify validation returnsvalid=Truewith an error message, which is semantically confusing.When
shellType == "Dify", the response hasvalid=Truebut includes an error message. Consider using an emptyerrorslist or achecksentry instead to convey this is informational rather than an error.if shell_type == "Dify": return ImageValidationResponse( valid=True, - checks=[], - errors=["Dify is an external_api type and doesn't require image validation"], + checks=[ImageCheckResult( + name="Shell Type", + status="pass", + message="Dify is an external_api type and doesn't require image validation" + )], + errors=[], )
485-504: Uselogger.exceptioninstead oflogger.errorfor exception logging.When logging exceptions in catch blocks,
logger.exceptionautomatically includes the traceback, which aids debugging. This aligns with static analysis hint TRY400.except httpx.TimeoutException: - logger.error(f"Timeout calling executor manager for image validation: {image}") + logger.exception(f"Timeout calling executor manager for image validation: {image}") return ImageValidationResponse( valid=False, checks=[], errors=["Validation request timed out. The image may be large or slow to pull."], ) except httpx.RequestError as e: - logger.error(f"Error calling executor manager: {e}") + logger.exception(f"Error calling executor manager: {e}") return ImageValidationResponse( valid=False, checks=[], - errors=[f"Failed to connect to executor manager: {str(e)}"], + errors=[f"Failed to connect to executor manager: {e!s}"], ) except Exception as e: - logger.error(f"Image validation error: {e}") + logger.exception(f"Image validation error: {e}") return ImageValidationResponse( valid=False, checks=[], - errors=[f"Validation error: {str(e)}"], + errors=[f"Validation error: {e!s}"], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/app/api/api.py(2 hunks)backend/app/api/endpoints/adapter/shells.py(1 hunks)backend/app/schemas/kind.py(1 hunks)backend/app/services/adapters/executor_kinds.py(4 hunks)backend/init_data/02-public-shells.yaml(2 hunks)executor_manager/executors/docker/binary_extractor.py(1 hunks)executor_manager/executors/docker/executor.py(3 hunks)executor_manager/main.py(1 hunks)executor_manager/routers/routers.py(1 hunks)frontend/src/apis/shells.ts(1 hunks)frontend/src/app/settings/page.tsx(7 hunks)frontend/src/features/settings/components/ShellEdit.tsx(1 hunks)frontend/src/features/settings/components/ShellList.tsx(1 hunks)frontend/src/i18n/locales/en/common.json(2 hunks)frontend/src/i18n/locales/zh-CN/common.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments MUST be written in English, including inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions
Files:
frontend/src/app/settings/page.tsxbackend/app/schemas/kind.pyfrontend/src/features/settings/components/ShellList.tsxbackend/app/api/api.pyfrontend/src/features/settings/components/ShellEdit.tsxbackend/app/services/adapters/executor_kinds.pyfrontend/src/apis/shells.tsexecutor_manager/main.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/executors/docker/binary_extractor.pybackend/app/api/endpoints/adapter/shells.pyexecutor_manager/routers/routers.py
**/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript/React code must use strict mode, functional components with hooks, Prettier formatter, ESLint (Next.js config), single quotes, and no semicolons
Files:
frontend/src/app/settings/page.tsxfrontend/src/features/settings/components/ShellList.tsxfrontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
**/frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
constoverlet, nevervarin TypeScript/JavaScript
Files:
frontend/src/app/settings/page.tsxfrontend/src/features/settings/components/ShellList.tsxfrontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
**/frontend/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/frontend/**/*.{tsx,jsx}: Use Tailwind CSS utility classes with the color system CSS variables for consistent styling across the calm UI philosophy
Frontend mobile-first responsive design approach should be used with Tailwind CSS breakpoints (sm, md, lg, etc.)
Files:
frontend/src/app/settings/page.tsxfrontend/src/features/settings/components/ShellList.tsxfrontend/src/features/settings/components/ShellEdit.tsx
**/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
React hooks from react-hook-form and zod validation must be used for form implementation in TypeScript components
Files:
frontend/src/app/settings/page.tsxfrontend/src/features/settings/components/ShellList.tsxfrontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant, formatted with Black (line length: 88), and use isort for imports
Python code must use type hints
Python functions should have descriptive names, include docstrings for public functions/classes, extract magic numbers to constants, and keep functions to a maximum of 50 lines preferred
Files:
backend/app/schemas/kind.pybackend/app/api/api.pybackend/app/services/adapters/executor_kinds.pyexecutor_manager/main.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/executors/docker/binary_extractor.pybackend/app/api/endpoints/adapter/shells.pyexecutor_manager/routers/routers.py
🧬 Code graph analysis (7)
frontend/src/app/settings/page.tsx (1)
backend/app/schemas/kind.py (1)
ShellList(135-140)
frontend/src/features/settings/components/ShellEdit.tsx (1)
frontend/src/apis/shells.ts (3)
UnifiedShell(10-19)ImageCheckResult(43-48)shellApis(57-129)
backend/app/services/adapters/executor_kinds.py (3)
backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/schemas/kind.py (1)
Shell(125-132)frontend/src/types/api.ts (1)
Shell(75-84)
frontend/src/apis/shells.ts (2)
backend/app/api/endpoints/adapter/shells.py (6)
UnifiedShell(25-35)ShellCreateRequest(38-44)ShellUpdateRequest(47-51)ImageValidationRequest(54-58)ImageCheckResult(61-67)ImageValidationResponse(70-75)frontend/src/apis/client.ts (2)
apiClient(105-105)request(22-80)
executor_manager/main.py (1)
executor_manager/executors/docker/binary_extractor.py (1)
extract_executor_binary(33-74)
executor_manager/executors/docker/executor.py (1)
executor_manager/executors/docker/utils.py (1)
find_available_port(65-92)
executor_manager/executors/docker/binary_extractor.py (1)
shared/logger.py (1)
setup_logger(37-107)
🪛 Ruff (0.14.6)
backend/app/services/adapters/executor_kinds.py
420-420: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
executor_manager/main.py
44-44: Do not catch blind exception: Exception
(BLE001)
executor_manager/executors/docker/binary_extractor.py
1-1: Shebang is present but file is not executable
(EXE001)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
89-89: subprocess call: check for execution of untrusted input
(S603)
90-95: Starting a process with a partial executable path
(S607)
116-116: Do not catch blind exception: Exception
(BLE001)
133-133: subprocess call: check for execution of untrusted input
(S603)
134-134: Starting a process with a partial executable path
(S607)
149-149: subprocess call: check for execution of untrusted input
(S603)
150-155: Starting a process with a partial executable path
(S607)
166-166: Consider moving this statement to an else block
(TRY300)
169-169: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
171-171: Do not catch blind exception: Exception
(BLE001)
172-172: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/api/endpoints/adapter/shells.py
112-112: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
113-113: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
146-146: Do not catch blind exception: Exception
(BLE001)
164-164: Do not catch blind exception: Exception
(BLE001)
176-176: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
177-177: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
223-223: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
224-224: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
323-323: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
324-324: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
378-378: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
379-379: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
412-412: Unused function argument: current_user
(ARG001)
412-412: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
486-486: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
493-493: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
497-497: Use explicit conversion flag
Replace with conversion flag
(RUF010)
499-499: Do not catch blind exception: Exception
(BLE001)
500-500: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
504-504: Use explicit conversion flag
Replace with conversion flag
(RUF010)
executor_manager/routers/routers.py
276-276: subprocess call: check for execution of untrusted input
(S603)
277-277: Starting a process with a partial executable path
(S607)
293-293: subprocess call: check for execution of untrusted input
(S603)
294-302: Starting a process with a partial executable path
(S607)
342-343: try-except-pass detected, consider logging the exception
(S110)
342-342: Do not catch blind exception: Exception
(BLE001)
366-366: Do not catch blind exception: Exception
(BLE001)
375-375: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
379-379: Do not catch blind exception: Exception
(BLE001)
380-380: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
382-382: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (24)
frontend/src/i18n/locales/zh-CN/common.json (1)
542-585: LGTM! Comprehensive i18n translations for Shell management.The new
shellstranslation object provides complete coverage for the Shell management UI, including:
- CRUD operations (create, edit, delete)
- Validation messages
- Error handling messages
- Form field labels and hints
The structure is consistent with the existing
modelstranslations pattern.executor_manager/executors/docker/binary_extractor.py (1)
176-188: LGTM!The
get_volume_mount_config()function provides a clean API for consumers to get the volume mount configuration with appropriate readonly flag for security.executor_manager/main.py (1)
36-45: Good graceful degradation pattern.The extraction failure is logged as a warning rather than blocking startup, which is appropriate. However, consider adding more context to help operators understand the impact.
One minor enhancement - the warning could be more actionable:
- logger.warning(f"Executor binary extraction error: {e}, custom base images may not work") + logger.warning(f"Executor binary extraction error: {e}. Tasks using custom base images will fail. Check EXECUTOR_IMAGE env var.")executor_manager/routers/routers.py (1)
389-418: LGTM!The
cancel_taskendpoint properly handles theHTTPExceptionre-raise pattern and provides appropriate HTTP status codes for different failure scenarios.backend/app/api/api.py (2)
12-12: LGTM!The shells module import follows the existing pattern with other adapter modules.
25-25: LGTM!The shells router is properly registered with consistent prefix and tag naming conventions.
frontend/src/i18n/locales/en/common.json (1)
541-584: LGTM! Well-structured shell management translations.The new
shellsnamespace follows the established patterns frommodelsand other sections. The translations are comprehensive, covering CRUD operations, validation messages, and error states appropriately.frontend/src/app/settings/page.tsx (2)
120-131: LGTM! Clean integration of the Shells tab.The shells tab is consistently added to both desktop and mobile layouts with proper index mapping. The styling follows the existing tab patterns exactly.
178-180: Desktop and mobile panels properly added.The ShellList component is rendered in both desktop (line 179) and mobile (line 271) Tab.Panels, maintaining consistency with other settings tabs.
backend/app/services/adapters/executor_kinds.py (2)
467-469: LGTM! Correct base image override logic.The user-defined shell's
baseImagecorrectly takes precedence over the public shell's base image when present.
611-623: LGTM! Base image correctly propagated to executor.The
base_imagefield is appropriately added to the bot configuration, enabling the Docker executor to use custom base images when specified.backend/init_data/02-public-shells.yaml (1)
17-17: Verification confirms the review is accurate — no issues found.The Docker image returns HTTP 401 (requiring authentication), which is expected for private GitHub Container Registry access and indicates the image reference is valid. The YAML syntax is correct, baseImage is consistently applied only to local_engine shells (ClaudeCode and Agno at lines 17 and 31), and correctly omitted from the external_api Dify shell. The architectural distinction in the original review comment is sound.
frontend/src/features/settings/components/ShellEdit.tsx (4)
64-113: LGTM!The image validation handler correctly validates prerequisites, looks up the runtime from the selected base shell, and handles both success and error cases appropriately.
115-185: LGTM!The save handler correctly validates required fields, enforces name format rules matching the backend regex, and differentiates between create and update modes appropriately.
187-199: LGTM!The keyboard handler is correctly implemented with proper cleanup to prevent memory leaks.
201-380: LGTM!The JSX structure follows Tailwind CSS conventions with CSS variable-based color system and includes responsive breakpoint classes for mobile-first design.
frontend/src/features/settings/components/ShellList.tsx (3)
94-98: LGTM!The helper function provides a clean mapping from shell type constants to display labels.
130-209: LGTM!The shell list rendering correctly uses composite keys for uniqueness, distinguishes public shells visually, and appropriately restricts edit/delete actions to user-owned shells. The responsive Tailwind classes follow mobile-first design.
44-62: Great! Now let me examine the implementations of these custom hooks:
<function_calls>
cat frontend/src/hooks/use-toast.ts
</function_calls><function_calls>
cat frontend/src/hooks/useTranslation.ts
</function_calls>Now let me check the exact implementation details and see how the review comment's concern applies. Let me also search for similar patterns in the codebase:
<function_calls>
grep -r "useCallback.toast|useCallback.useTranslation" frontend/src --include=".ts" --include=".tsx" -A 3 | head -30
</function_calls>Let me create a comprehensive analysis by checking if this is a real issue:
<function_calls>
cat > /tmp/analyze_pattern.py << 'EOF'Analyze the useCallback pattern with toast and t dependencies
print("=== Analysis of useCallback Pattern ===\n")
print("Pattern in ShellList.tsx (lines 44-62):")
print("""
const fetchShells = useCallback(async () => {
// Uses: toast and t
setLoading(true)
try {
const response = await shellApis.getUnifiedShells()
setShells(response.data || [])
} catch (error) {
console.error('Failed to fetch shells:', error)
toast({...}) // Uses toast
} finally {
setLoading(false)
}
}, [toast, t]) // DependenciesuseEffect(() => {
fetchShells()
}, [fetchShells]) // Depends on fetchShells
""")print("\n=== Key Questions ===\n")
print("1. Do toast and t return stable references?")
print(" - If both return stable refs → fetchShells callback is stable → useEffect runs once ✓")
print(" - If they change → fetchShells is recreated → useEffect runs again → INFINITE LOOP ✗\n")print("2. What do the hook implementations show?")
print(" - Need to check if hooks use useMemo/useCallback internally\n")print("3. Is the concern valid?")
print(" - The concern is theoretically valid IF toast/t change on every render")
print(" - But custom hooks often ensure stability")
EOF
python /tmp/analyze_pattern.py
</function_calls>Now let me verify the actual hook implementations more carefully:
<function_calls>
wc -l frontend/src/hooks/use-toast.ts frontend/src/hooks/useTranslation.ts
</function_calls><function_calls>
head -100 frontend/src/hooks/use-toast.ts
</function_calls><function_calls>
head -50 frontend/src/hooks/useTranslation.ts
</function_calls>Let me also verify the actual line numbers and context in ShellList.tsx:
<function_calls>
sed -n '40,70p' frontend/src/features/settings/components/ShellList.tsx
</function_calls>Now let me check if there's documentation or examples about this pattern in React/hooks best practices:
<function_calls>
React useCallback with dependencies in useEffect dependency array best practices
</function_calls><function_calls>
React hooks useCallback stable references infinite loop prevention
</function_calls>executor_manager/executors/docker/executor.py (3)
197-205: LGTM!The helper method safely extracts
base_imagewith proper type guards and defensive checks. The comment clarifies that only the first bot's configuration is used.
270-297: LGTM!The Init Container pattern is correctly implemented. When a custom
base_imageis provided, the executor binary is mounted from the named volume and the entrypoint is overridden. The read-only mount and final image selection logic are appropriate.
214-236: LGTM!The updated docstring clearly explains the Init Container pattern behavior and documents all parameters including the new
base_imageparameter.frontend/src/apis/shells.ts (2)
10-19: LGTM!The
UnifiedShellinterface correctly mirrors the backendUnifiedShellPydantic model with matching field names and types, including proper nullability handling with| null.
73-82: LGTM!The
getUnifiedShellmethod correctly handles the optionalshellTypequery parameter with proper URL encoding for the path parameter.
| import os | ||
|
|
||
| import httpx |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move imports to module level.
The os and httpx imports are inside the function. Per PEP 8 and coding guidelines, imports should be at the module level.
import logging
+import os
import re
from typing import List, Optional
from fastapi import APIRouter, Depends, HTTPException, Query, status
+import httpx
from pydantic import BaseModel
from sqlalchemy.orm import SessionThen remove the inline imports at lines 426-428.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/shells.py around lines 426 to 428, the
imports for os and httpx are done inside a function; move those import
statements to the top of the module (module-level imports) and then remove the
inline import lines at 426-428 so the function uses the module-level os and
httpx names instead.
| # If user shell not found, try public shells | ||
| shell_base_image = None | ||
| if not shell: | ||
| from app.models.public_shell import PublicShell | ||
|
|
||
| public_shell = ( | ||
| db.query(PublicShell) | ||
| .filter( | ||
| PublicShell.name == bot_crd.spec.shellRef.name, | ||
| PublicShell.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
| if public_shell and public_shell.json: | ||
| shell_crd_temp = Shell.model_validate(public_shell.json) | ||
| shell_base_image = shell_crd_temp.spec.baseImage | ||
| # Create a mock shell object for compatibility | ||
| class MockShell: | ||
| def __init__(self, json_data): | ||
| self.json = json_data | ||
| shell = MockShell(public_shell.json) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor inline class and fix boolean comparison.
Several issues in this block:
- Static analysis (Ruff E712): Line 420 uses
== Trueinstead of direct boolean check. - Inline class definition:
MockShelldefined inside the method is a code smell. It should be extracted to module level or use a simpler approach. - Import inside function: While acceptable for avoiding circular imports, consider if this can be moved to top-level.
+# At module level (after line 26)
+class MockShell:
+ """Mock shell object for compatibility when using public shells."""
+ def __init__(self, json_data):
+ self.json = json_data
+
# In the method (lines 411-431):
if not shell:
from app.models.public_shell import PublicShell
public_shell = (
db.query(PublicShell)
.filter(
PublicShell.name == bot_crd.spec.shellRef.name,
- PublicShell.is_active == True,
+ PublicShell.is_active.is_(True),
)
.first()
)
if public_shell and public_shell.json:
shell_crd_temp = Shell.model_validate(public_shell.json)
shell_base_image = shell_crd_temp.spec.baseImage
- # Create a mock shell object for compatibility
- class MockShell:
- def __init__(self, json_data):
- self.json = json_data
shell = MockShell(public_shell.json)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
420-420: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
| executor_image: Default executor image | ||
| base_image: Optional custom base image | ||
| """ | ||
| from executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUME |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find binary_extractor.py to verify file location
fd -t f "binary_extractor.py"Repository: wecode-ai/Wegent
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check current imports and structure in executor.py
head -50 executor_manager/executors/docker/executor.pyRepository: wecode-ai/Wegent
Length of output: 1296
🏁 Script executed:
#!/bin/bash
# Find all occurrences of EXECUTOR_BINARY_VOLUME in executor.py
rg "EXECUTOR_BINARY_VOLUME" executor_manager/executors/docker/executor.py -n -B2 -A2Repository: wecode-ai/Wegent
Length of output: 727
🏁 Script executed:
#!/bin/bash
# Check the import statement at line 237 in context
sed -n '230,245p' executor_manager/executors/docker/executor.pyRepository: wecode-ai/Wegent
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Check __init__.py files to understand module structure
find executor_manager/executors/docker -name "__init__.py" -exec cat {} +Repository: wecode-ai/Wegent
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Check what binary_extractor.py exports
cat executor_manager/executors/docker/binary_extractor.pyRepository: wecode-ai/Wegent
Length of output: 5967
🏁 Script executed:
#!/bin/bash
# Verify if there are any circular dependency risks
rg "from.*executor import\|import.*executor" executor_manager/executors/docker/binary_extractor.pyRepository: wecode-ai/Wegent
Length of output: 42
Move import to module level and use correct full module path.
The import of EXECUTOR_BINARY_VOLUME at line 237 uses an incorrect relative path format (executors.docker.binary_extractor instead of the full module path) and is placed inside the method. Per PEP 8, imports must be at module level. This is a simple constant with no circular dependency risk (binary_extractor.py does not import from executor.py).
from executor_manager.executors.docker.constants import (
CONTAINER_OWNER,
DEFAULT_DOCKER_HOST,
DEFAULT_API_ENDPOINT,
DEFAULT_TIMEZONE,
DEFAULT_LOCALE,
DOCKER_SOCKET_PATH,
WORKSPACE_MOUNT_PATH,
DEFAULT_PROGRESS_RUNNING,
DEFAULT_PROGRESS_COMPLETE,
DEFAULT_TASK_ID,
)
+from executor_manager.executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUMEThen remove the inline import at line 237.
🤖 Prompt for AI Agents
In executor_manager/executors/docker/executor.py around line 237, move the
inline import out of the method and add a module-level import using the full
package path: from executor_manager.executors.docker.binary_extractor import
EXECUTOR_BINARY_VOLUME; then remove the inline import at line 237 so the method
references EXECUTOR_BINARY_VOLUME directly. Ensure the new module-level import
is placed with the other imports at top of the file and verify there are no
circular imports.
executor_manager/routers/routers.py
Outdated
| pull_result = subprocess.run( | ||
| ["docker", "pull", image], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=300, # 5 minutes timeout for pull | ||
| ) | ||
| if pull_result.returncode != 0: | ||
| logger.error(f"Failed to pull image {image}: {pull_result.stderr}") | ||
| return ValidateImageResponse( | ||
| valid=False, | ||
| checks=[], | ||
| errors=[f"Failed to pull image: {pull_result.stderr}"], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and understand the broader context
find . -name "routers.py" -path "*/executor_manager/routers/*" | head -5Repository: wecode-ai/Wegent
Length of output: 97
🏁 Script executed:
# Search for the validate_image function and surrounding code
rg -n "validate_image|def.*image" executor_manager/routers/ -A 5 -B 5 --type pyRepository: wecode-ai/Wegent
Length of output: 918
🏁 Script executed:
# Look for any existing image validation patterns in the codebase
rg -n "image.*valid|IMAGE|image.*format" executor_manager/ --type py -C 3Repository: wecode-ai/Wegent
Length of output: 5022
🏁 Script executed:
# Search for all docker command executions to understand the full scope
rg -n "docker.*pull|docker.*run" executor_manager/ --type py -C 2Repository: wecode-ai/Wegent
Length of output: 5933
🏁 Script executed:
# Read the full validate_image function
sed -n '197,350p' executor_manager/routers/routers.pyRepository: wecode-ai/Wegent
Length of output: 5701
🏁 Script executed:
# Check the ValidateImageRequest class definition
sed -n '170,196p' executor_manager/routers/routers.pyRepository: wecode-ai/Wegent
Length of output: 652
🏁 Script executed:
# Search for any image validation or sanitization utilities
rg -n "def.*validate.*image|image.*pattern|IMAGE.*PATTERN" executor_manager/ --type pyRepository: wecode-ai/Wegent
Length of output: 297
🌐 Web query:
Docker image name format specification official
💡 Result:
Official (de facto) grammar: image references follow the docker/distribution "reference" format:
reference := name [ ":" tag ] [ "@" digest ]
name := [domain '/'] path-component ['/' path-component]*
domain := domain-component ['.' domain-component]* [':' port-number]
path-component := alpha-numeric [separator alpha-numeric]*
tag := /[\w][\w.-]{0,127}/
digest := algorithm ":" hex
(See docker/distribution reference grammar.) [1]
The OCI Image spec recommends using the distribution reference form for image names and defines an allowed character set for annotated reference names (alphanum plus -._:@/+ and "--") and recommends fully-qualified (registry + path) names. [2]
Sources:
[1] docker/distribution reference grammar (reference package).
[2] OCI Image Format specification — annotations / ref.name and recommendations.
🌐 Web query:
Docker image naming conventions registry repository tag
💡 Result:
Short summary — Docker/OCI image reference format, rules, and examples
-
Full image reference format:
[registryhost[:port]/]repository-path[:tag][@digest]. [1][2] -
Components:
- registry host (optional; defaults to docker.io). [1]
- repository name / namespace (path of one or more slash-separated components). [1][2]
- tag (human-friendly version; optional, defaults to latest). [1][2]
- digest (content-addressable identifier, e.g. sha256:..., used with @). [1][2]
-
Key rules (from the OCI/Distribution spec):
- Repository name () regex: [a-z0-9]+([.-][a-z0-9]+)*(/[a-z0-9]+([.-][a-z0-9]+)). Max length often constrained by clients/registries (practical limits ~255 total). [2]
- Tag regex: [A-Za-z0-9_][A-Za-z0-9._-]{0,127} (max 128 chars). [2]
- Digest format: algorithm:encoded (commonly sha256:<64-hex>). If both tag and digest present, digest is authoritative. [2]
-
Practical notes:
- If the first path component contains a . or : (or is localhost) it’s treated as a registry hostname; otherwise it’s a Docker Hub path and may default to library namespace. [1][2]
- Use tags for human versions (v1.2.3, latest, rc1); use digests for immutable, reproducible pulls. [1][2]
-
Examples:
- alpine -> docker.io/library/alpine:latest. [1]
- myregistry.example.com:5000/team/app:1.0.0
- ghcr.io/org/repo/my-app:pr-311
- docker pull ubuntu@sha256:abcdef... (pulls by digest). [1][2]
Sources: Docker docs (Build/Tag image + tagging rules). [1] OCI Distribution Specification (name/tag/digest grammar and limits). [2]
Security concern is valid, but suggested regex is incomplete and doesn't match Docker's full image reference specification.
The image parameter from user input is indeed passed directly to docker pull and docker run commands without validation. While the subprocess list form prevents shell injection, validating the image name format against Docker's specification is reasonable defensive practice.
However, the suggested regex is incomplete:
Issues with the proposed pattern:
- Missing support for registry hostname with port (e.g.,
myregistry.example.com:5000) - Missing support for digest syntax (e.g.,
image@sha256:abc123...) - Incomplete domain/registry handling
- Won't validate images like
ghcr.io/org/repo:tag(uppercase letters in domain)
Docker's actual format (from official Distribution spec):
[registryhost[:port]/]repository-path[:tag][@digest]- Repository name:
[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)* - Tag:
[A-Za-z0-9_][A-Za-z0-9._-]{0,127} - Digest:
algorithm:encoded(e.g.,sha256:...)
A more complete validation would need to handle registry hostname (with optional port), path components, optional tag, and optional digest. Consider using a library like python-docker or implementing a more comprehensive regex that matches the actual OCI Distribution specification.
🧰 Tools
🪛 Ruff (0.14.6)
276-276: subprocess call: check for execution of untrusted input
(S603)
277-277: Starting a process with a partial executable path
(S607)
- Create ImageValidatorAgent for running validation inside container - Modify executor_manager to dispatch validation tasks via task processor - Update backend shells API for async validation response - Update frontend to handle async validation status - Add i18n translations for validation status messages This approach uses the actual executor run flow instead of direct docker subprocess calls, making it extensible for both Docker and K8s modes. Validation runs inside the target container and reports results via callback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
executor_manager/routers/routers.py (1)
175-180: Image parameter validation still missing.The
imagefield accepts any string without format validation. As extensively discussed in the previous review, this parameter is eventually passed to Docker commands and should be validated against the Docker/OCI image reference specification.While the subprocess list form prevents shell injection, validating the image format is a reasonable defensive practice and helps catch user errors early.
Consider adding a validator or creating a follow-up issue to implement proper image name validation as discussed in the previous review.
backend/app/api/endpoints/adapter/shells.py (4)
281-288: Docker image regex validation remains too restrictive (duplicate concern).As previously flagged, the current regex
^[a-z0-9.-]+(/[a-z0-9._-]+)+:[a-z0-9._-]+$rejects valid Docker image formats including library images (ubuntu:22.04), images without explicit tags, localhost registries with ports, and digest references. Consider using a proper Docker/OCI reference parser or significantly relaxing the validation pattern.
360-368: Docker image regex validation remains too restrictive (duplicate concern).Same overly restrictive regex as in create_shell. This validation should be relaxed or replaced with proper Docker/OCI reference parsing.
380-411: Missing validation for shell deletion when referenced by active bots (duplicate concern).As previously identified, the delete endpoint performs soft delete without checking if any active bots reference this shell via
shellRef.name. When bots query for shells withis_active == True, they will encounter NotFoundException if their referenced shell is deleted. Consider adding validation to prevent deletion or warn users when shells are in use by bots.
435-437: Move imports to module level (duplicate concern).As previously noted, the
osandhttpximports should be at module level per PEP 8 guidelines. Move these to the top of the file with other imports.
🧹 Nitpick comments (6)
executor/callback/callback_handler.py (1)
113-150: Result propagation looks good; consider reusingsend_status_callbackfor consistent loggingThe new
resultparameter is correctly typed, documented, and forwarded tocallback_client.send_callback, which aligns with the callback client's signature and enables structured completion results.One behavioral change, though: by calling
callback_client.send_callbackdirectly, this path now bypassessend_status_callback’s status-specific logging and error-wrapping, while thestartedandfailedhelpers still go throughsend_status_callback. If you want consistent observability across all task states, consider extendingsend_status_callbackto accept an optionalresult: Optional[Dict[str, Any]] = Noneand havesend_task_completed_callbackcall it, forwardingresult. That keeps all callbacks on the same logging/error-handling path.Also, in this function’s docstring the
task_idargument is documented asstrwhile the annotation isint; updating those types in the docstring would avoid confusion.executor_manager/routers/routers.py (1)
275-283: Use logging.exception for better error diagnostics.When logging exceptions in an exception handler, use
logger.exception()instead oflogger.error()to automatically include the traceback, which aids debugging.except Exception as e: - logger.error(f"Failed to submit validation task for {image}: {e}") + logger.exception(f"Failed to submit validation task for {image}: {e}") return { "status": "error", "message": f"Failed to submit validation task: {str(e)}", "valid": False, "checks": [], "errors": [str(e)], }backend/app/api/endpoints/adapter/shells.py (1)
494-517: Consider usinglogging.exceptionfor better error context.When logging exceptions in except blocks (lines 495, 503, 511), use
logging.exception()instead oflogging.error()to automatically include the traceback, which aids debugging.executor/agents/factory.py (1)
25-36: LGTM: Factory registration correctly extended for validator agent.The _agents dictionary now includes the imagevalidator type, and the docstring appropriately documents the new validator classification.
Consider annotating
_agentswithtyping.ClassVarper static analysis suggestion:+from typing import Dict, Any, Optional, ClassVar ... - _agents = { + _agents: ClassVar[Dict[str, type]] = { "claudecode": ClaudeCodeAgent, "agno": AgnoAgent, "dify": DifyAgent, "imagevalidator": ImageValidatorAgent, }executor/agents/image_validator/image_validator_agent.py (2)
26-72: LGTM: Agent class structure and validation checks are well-defined.The ImageValidatorAgent correctly extends Agent, defines appropriate AGENT_TYPE, and provides comprehensive validation checks for ClaudeCode and Agno shell types. The check configurations are clear and maintainable.
Consider annotating VALIDATION_CHECKS with
typing.ClassVarper static analysis suggestion:+from typing import Dict, Any, Tuple, Optional, List, ClassVar ... - VALIDATION_CHECKS = { + VALIDATION_CHECKS: ClassVar[Dict[str, List[Dict[str, Any]]]] = {
136-208: LGTM: Check execution logic is robust with comprehensive error handling.The method properly executes validation commands, parses versions, performs semantic version comparisons, and handles various failure modes. The subprocess security warnings are false positives since commands come from predefined class constants, not user input.
Consider using
logging.exception()instead oflogging.error()at lines 196 and 203 for better debugging context:except subprocess.TimeoutExpired: - logger.error(f"Check '{name}' timed out") + logger.exception(f"Check '{name}' timed out") return { "name": name, "status": "fail", "message": "Check timed out", } except Exception as e: - logger.error(f"Check '{name}' error: {e}") + logger.exception(f"Check '{name}' error: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/app/api/endpoints/adapter/shells.py(1 hunks)executor/agents/factory.py(2 hunks)executor/agents/image_validator/__init__.py(1 hunks)executor/agents/image_validator/image_validator_agent.py(1 hunks)executor/callback/callback_handler.py(3 hunks)executor_manager/routers/routers.py(1 hunks)frontend/src/apis/shells.ts(1 hunks)frontend/src/features/settings/components/ShellEdit.tsx(1 hunks)frontend/src/i18n/locales/en/common.json(2 hunks)frontend/src/i18n/locales/zh-CN/common.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments MUST be written in English, including inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions
Files:
executor/agents/image_validator/__init__.pyfrontend/src/features/settings/components/ShellEdit.tsxexecutor/callback/callback_handler.pyfrontend/src/apis/shells.tsexecutor/agents/image_validator/image_validator_agent.pyexecutor/agents/factory.pyexecutor_manager/routers/routers.pybackend/app/api/endpoints/adapter/shells.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant, formatted with Black (line length: 88), and use isort for imports
Python code must use type hints
Python functions should have descriptive names, include docstrings for public functions/classes, extract magic numbers to constants, and keep functions to a maximum of 50 lines preferred
Files:
executor/agents/image_validator/__init__.pyexecutor/callback/callback_handler.pyexecutor/agents/image_validator/image_validator_agent.pyexecutor/agents/factory.pyexecutor_manager/routers/routers.pybackend/app/api/endpoints/adapter/shells.py
**/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript/React code must use strict mode, functional components with hooks, Prettier formatter, ESLint (Next.js config), single quotes, and no semicolons
Files:
frontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
**/frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
constoverlet, nevervarin TypeScript/JavaScript
Files:
frontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
**/frontend/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/frontend/**/*.{tsx,jsx}: Use Tailwind CSS utility classes with the color system CSS variables for consistent styling across the calm UI philosophy
Frontend mobile-first responsive design approach should be used with Tailwind CSS breakpoints (sm, md, lg, etc.)
Files:
frontend/src/features/settings/components/ShellEdit.tsx
**/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
React hooks from react-hook-form and zod validation must be used for form implementation in TypeScript components
Files:
frontend/src/features/settings/components/ShellEdit.tsxfrontend/src/apis/shells.ts
🧬 Code graph analysis (6)
executor/agents/image_validator/__init__.py (1)
executor/agents/image_validator/image_validator_agent.py (1)
ImageValidatorAgent(26-212)
frontend/src/features/settings/components/ShellEdit.tsx (8)
backend/app/api/endpoints/adapter/shells.py (2)
UnifiedShell(25-35)ImageCheckResult(62-68)frontend/src/apis/shells.ts (3)
UnifiedShell(10-19)ImageCheckResult(44-49)shellApis(62-134)executor_manager/routers/routers.py (1)
ImageCheckResult(182-187)frontend/src/hooks/use-toast.ts (1)
toast(189-189)frontend/src/components/ui/button.tsx (1)
Button(55-55)frontend/src/components/ui/label.tsx (1)
Label(21-21)frontend/src/components/ui/input.tsx (1)
Input(28-28)frontend/src/components/ui/select.tsx (5)
Select(147-147)SelectTrigger(150-150)SelectValue(149-149)SelectContent(151-151)SelectItem(153-153)
executor/callback/callback_handler.py (1)
executor/callback/callback_client.py (1)
send_callback(89-150)
executor/agents/image_validator/image_validator_agent.py (2)
shared/logger.py (1)
setup_logger(37-107)executor_manager/tests/executors/test_docker_executor.py (1)
executor(31-35)
executor/agents/factory.py (1)
executor/agents/image_validator/image_validator_agent.py (1)
ImageValidatorAgent(26-212)
executor_manager/routers/routers.py (3)
backend/app/api/endpoints/adapter/shells.py (2)
ImageCheckResult(62-68)validate_image(415-517)frontend/src/apis/shells.ts (1)
ImageCheckResult(44-49)frontend/src/apis/client.ts (2)
post(86-91)request(22-80)
🪛 Ruff (0.14.6)
executor/agents/image_validator/image_validator_agent.py
1-1: Shebang is present but file is not executable
(EXE001)
37-72: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
144-144: subprocess call: check for execution of untrusted input
(S603)
145-145: Starting a process with a partial executable path
(S607)
178-178: Do not catch blind exception: Exception
(BLE001)
196-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
202-202: Do not catch blind exception: Exception
(BLE001)
203-203: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
executor/agents/factory.py
31-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
executor_manager/routers/routers.py
269-273: Consider moving this statement to an else block
(TRY300)
271-271: f-string without any placeholders
Remove extraneous f prefix
(F541)
275-275: Do not catch blind exception: Exception
(BLE001)
276-276: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
279-279: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/api/endpoints/adapter/shells.py
117-117: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
118-118: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
151-151: Do not catch blind exception: Exception
(BLE001)
169-169: Do not catch blind exception: Exception
(BLE001)
181-181: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
182-182: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
228-228: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
229-229: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
328-328: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
329-329: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
383-383: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
384-384: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
417-417: Unused function argument: current_user
(ARG001)
417-417: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
495-495: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
503-503: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
506-506: Use explicit conversion flag
Replace with conversion flag
(RUF010)
508-508: Use explicit conversion flag
Replace with conversion flag
(RUF010)
510-510: Do not catch blind exception: Exception
(BLE001)
511-511: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
514-514: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (22)
executor_manager/routers/routers.py (3)
182-188: LGTM!The model structure is correct and consistent with the backend and frontend definitions.
220-228: LGTM!The early return for Dify shell type is appropriate since it's an external_api type that doesn't require image validation.
230-238: LGTM!The shell_type validation is appropriate and provides a clear error message for unsupported types.
backend/app/api/endpoints/adapter/shells.py (5)
5-21: LGTM: Imports are properly organized.The imports are correctly placed at module level, follow PEP 8 conventions, and are appropriately grouped.
24-81: LGTM: Pydantic models are well-defined.The models provide clear structure for shell management and validation, with appropriate field types and documentation.
83-112: LGTM: Helper functions correctly convert shells to unified format.The conversion logic appropriately handles both public and user shells with proper error handling via try-except in the calling code.
115-172: LGTM: Unified shell listing correctly aggregates public and user shells.The function properly queries both shell sources, converts them to unified format, and gracefully handles parsing errors with logging. The bare exception catches here are appropriate for non-critical parsing failures.
175-222: LGTM: Shell retrieval logic correctly implements fallback behavior.The function appropriately prioritizes user shells over public shells when type is not specified, and provides clear error messages for not-found cases.
executor/agents/image_validator/__init__.py (1)
1-7: LGTM: Clean package initialization.The module correctly exports ImageValidatorAgent with proper
__all__definition for explicit public API.executor/agents/factory.py (1)
16-16: LGTM: ImageValidatorAgent import added correctly.The new import properly integrates the ImageValidatorAgent into the factory pattern.
frontend/src/i18n/locales/zh-CN/common.json (1)
316-316: LGTM: Chinese translations for Shell management are complete.The translations provide comprehensive localization for all Shell management UI elements including titles, descriptions, form fields, validation messages, and error states.
Also applies to: 542-589
frontend/src/i18n/locales/en/common.json (1)
315-315: LGTM: English translations for Shell management are comprehensive.The translations cover all necessary UI strings for Shell management features with clear, professional language.
Also applies to: 541-588
frontend/src/features/settings/components/ShellEdit.tsx (5)
5-27: LGTM: Component setup and props definition are clean.The imports are well-organized, the component uses 'use client' directive appropriately, and the props interface is properly typed.
29-64: LGTM: State management and data fetching are properly implemented.The component correctly initializes state from props, fetches available base shells on mount, and handles loading/error states appropriately.
66-149: LGTM: Image validation handler correctly implements async validation flow.The function appropriately validates inputs, handles various response statuses (skipped, submitted, error), and provides clear user feedback via toasts. The async nature of validation is properly communicated.
151-221: LGTM: Save handler implements comprehensive validation and error handling.The function properly validates input fields, matches backend validation requirements, and provides clear user feedback for both success and error cases. The conditional logic for create vs. update modes is correct.
223-425: LGTM: Component lifecycle and rendering are well-implemented.The escape-to-close behavior is properly implemented with cleanup, the form structure is accessible with proper labels, and the validation status UI provides clear visual feedback with appropriate color coding and icons. Tailwind classes are used consistently.
executor/agents/image_validator/image_validator_agent.py (3)
74-98: LGTM: Initialization logic correctly validates required parameters.The agent properly extracts validation parameters and verifies shell_type is present and supported before proceeding with validation.
100-134: LGTM: Execute method correctly orchestrates validation checks.The method properly iterates through configured checks, aggregates results, and reports completion with structured validation data via the callback mechanism.
210-212: LGTM: Cancel method appropriately indicates cancellation is not applicable.The implementation correctly signals that validation tasks cannot be meaningfully cancelled.
frontend/src/apis/shells.ts (2)
5-59: LGTM: Type definitions accurately represent the Shell API surface.The interfaces correctly mirror the backend models, providing strong typing for shell management and image validation operations. The type definitions are comprehensive and well-documented.
62-133: LGTM: API methods correctly implement the Shell management surface.All methods properly handle URL encoding, query parameters, and request/response typing. The helper methods (getPublicShells, getLocalEngineShells) efficiently filter the unified list for specific use cases.
| class ValidateImageResponse(BaseModel): | ||
| """Response for image validation""" | ||
| status: str # 'submitted' for async validation | ||
| message: str | ||
| validation_task_id: Optional[int] = None | ||
|
|
There was a problem hiding this comment.
Response model doesn't match actual response structure.
The ValidateImageResponse model only declares status, message, and validation_task_id, but the actual responses at lines 222-228, 232-238, and 277-283 return additional fields: valid, checks, and errors. This breaks type safety and can lead to confusion.
Update the model to include all returned fields:
class ValidateImageResponse(BaseModel):
"""Response for image validation"""
status: str # 'submitted' for async validation
message: str
validation_task_id: Optional[int] = None
+ valid: Optional[bool] = None
+ checks: Optional[list] = None
+ errors: Optional[list] = None🤖 Prompt for AI Agents
In executor_manager/routers/routers.py around lines 190-195, the
ValidateImageResponse Pydantic model is missing the fields actually returned by
the endpoints (valid, checks, errors); update the model to add a boolean valid,
a checks field (List[Dict[str, Any]] or a dedicated Check model if checks have
structure), and an optional errors field (Optional[List[str]] or
Optional[List[Dict[str, Any]]] depending on shape), keep validation_task_id
optional, and import the required typing types (List, Dict, Any, Optional) so
the response model matches the actual responses at lines 222-228, 232-238, and
277-283.
|
|
||
| # Build validation task data | ||
| # Use a unique negative task_id to distinguish validation tasks from regular tasks | ||
| import time |
There was a problem hiding this comment.
Remove duplicate import.
The time module is already imported at line 14. This import statement is redundant.
- import time
validation_task_id = -int(time.time() * 1000) % 1000000 # Negative ID for validation tasks📝 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.
| import time | |
| validation_task_id = -int(time.time() * 1000) % 1000000 # Negative ID for validation tasks |
🤖 Prompt for AI Agents
In executor_manager/routers/routers.py around line 242, there is a duplicate
"import time" statement (the time module is already imported at line 14); remove
the redundant import at line 242 so the module is imported only once at the top
of the file to eliminate the duplicate and keep imports clean.
…ling
- Add UUID-based validation tracking with Redis storage (5min TTL)
- Add validation status query endpoint GET /api/shells/validation-status/{id}
- Add validation status update endpoint for internal callback forwarding
- Enhance Executor Manager to pass validation_id and forward callbacks to Backend
- Enhance Docker executor to report container start/pull failures
- Add stage progress reporting in ImageValidatorAgent (submitted/pulling/starting/running_checks/completed)
- Implement frontend polling mechanism with 2s interval, 120s timeout
- Disable save button until validation passes (forced validation for new/changed baseImage)
- Add progress bar UI showing real-time validation stages
- Add i18n messages for validation stages in both en/zh-CN
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
executor_manager/executors/docker/executor.py (1)
243-327: Fix inline import forEXECUTOR_BINARY_VOLUME(wrong module path and non‑PEP 8 placement).The inline import
from executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUMEinside
_prepare_docker_commandhas two issues:
- The module path likely omits the top-level package (
executor_manager), which will raiseModuleNotFoundErrorat runtime whenbase_imageis used.- Per PEP 8 and project style, imports should be at module level, not inside functions, especially for a simple constant with no circular dependency risk.
Move the import to the top of the file with the correct package path and remove the inline import:
from executor_manager.executors.docker.constants import ( CONTAINER_OWNER, @@ DEFAULT_TASK_ID, ) +from executor_manager.executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUME @@ - """ - from executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUME - + """ @@ - # If using custom base_image, mount executor binary from Named Volume - if base_image: - cmd.extend([ - "-v", f"{EXECUTOR_BINARY_VOLUME}:/app:ro", # Mount executor binary as read-only - "--entrypoint", "/app/executor" # Override entrypoint - ]) + # If using custom base_image, mount executor binary from Named Volume + if base_image: + cmd.extend( + [ + "-v", + f"{EXECUTOR_BINARY_VOLUME}:/app:ro", # Mount executor binary as read-only + "--entrypoint", + "/app/executor", # Override entrypoint + ] + )This will avoid import errors when launching containers with custom base images and aligns with prior review feedback.
executor_manager/routers/routers.py (2)
227-248: Clarify/alignValidateImageResponseusage with actual responses.This model currently only declares
status,message, andvalidation_task_id, while the endpoint below returns additional fields (valid,checks,errors) and does not declareresponse_model=ValidateImageResponse. Either (a) extend this model to include all returned fields and use it as theresponse_modelfor/executor-manager/images/validate, or (b) drop the unused model to avoid confusion about the canonical response shape.
274-338: Fix validation task ID generation and remove duplicatetimeimport.
validation_task_id = -int(time.time() * 1000) % 1000000never yields a negative ID in Python due to modulo semantics, so the comment “Negative ID for validation tasks” is currently incorrect and any consumer expecting negative IDs will be broken. If you want a bounded negative ID, compute it asvalidation_task_id = -(int(time.time() * 1000) % 1_000_000). Also,timeis already imported at the top of the file, so the innerimport timehere is redundant and should be removed.- import time - validation_task_id = -int(time.time() * 1000) % 1000000 # Negative ID for validation tasks + validation_task_id = -(int(time.time() * 1000) % 1_000_000) # Negative ID for validation tasksbackend/app/api/endpoints/adapter/shells.py (2)
258-322: Docker image regex is too restrictive and will reject valid image references.The regex
^[a-z0-9.-]+(/[a-z0-9._-]+)+:[a-z0-9._-]+$(used both on create and update) disallows common valid forms likeubuntu:22.04(no/), images without explicit tags, registries with ports (localhost:5000/app:tag), and digest references (image@sha256:...). This will cause user frustration and diverges from Docker/OCI’s actual reference grammar.- # Validate baseImage format (basic URL validation) - if not request.baseImage or not re.match( - r"^[a-z0-9.-]+(/[a-z0-9._-]+)+:[a-z0-9._-]+$", request.baseImage, re.IGNORECASE - ): - raise HTTPException( - status_code=400, - detail="Invalid base image format. Expected format: registry/image:tag", - ) + # Validate baseImage format (relaxed Docker/OCI reference check) + if not request.baseImage: + raise HTTPException(status_code=400, detail="Base image is required") + # TODO: consider using a dedicated Docker/OCI image reference parser or a more + # complete regex that allows optional registry, port, tag, and digest. + # For now, keep validation minimal to avoid rejecting valid images.Also applies to: 391-403
413-445: Prevent deleting shells that are still referenced by active bots.
delete_shellsimply soft-deletes the user shell without checking whether any active bots reference it viashellRef.name, which will cause those bots to start failing lookups once the shell is deactivated. Before deactivating, consider checking for referencing bots (e.g., via the existing Kind/bot services) and either blocking deletion with a clear 400/409 error or returning a warning/flag so the UI can surface the impact.
🧹 Nitpick comments (10)
executor_manager/executors/docker/executor.py (1)
176-224: Base-image support flow looks correct; double-check task shape and bot selection.The new
base_imageextraction and plumbing into_prepare_docker_commandpreserves existing behavior whenbase_imageisNoneand cleanly activates the custom-base-image mode when present. The validation-specific reporting on container start success/failure is also wired in appropriately.Two minor points to verify/tweak:
_get_base_image_from_taskonly inspects the first entry intask["bot"]. If multi-bot tasks are possible, confirm this is the intended behavior or consider selecting the relevant bot more explicitly.- The condition
if bots and isinstance(bots, list) and len(bots) > 0:is redundant;if isinstance(bots, list):is sufficient since indexingbots[0]already implies non-empty.If the single‑bot assumption holds, the implementation is fine as-is.
Also applies to: 226-234
executor/agents/image_validator/image_validator_agent.py (5)
1-8: Shebang not needed for non-executable agent moduleRuff flags the shebang because the file is not executable. Since this module is imported (not run as a script), you can safely drop the shebang to silence EXE001 and simplify the header.
-#!/usr/bin/env python - -# SPDX-FileCopyrightText: 2025 Weibo, Inc. +# SPDX-FileCopyrightText: 2025 Weibo, Inc.
15-21: AnnotateVALIDATION_CHECKSas aClassVarto satisfy Ruff and clarify intentRuff’s RUF012 is valid here:
VALIDATION_CHECKSis a mutable class-level constant, not an instance attribute. Adding an explicitClassVarannotation both documents intent and fixes the lint warning.-from typing import Dict, Any, Tuple, Optional, List +from typing import Any, ClassVar, Dict, List @@ - # Shell type to validation checks mapping - VALIDATION_CHECKS = { + # Shell type to validation checks mapping + VALIDATION_CHECKS: ClassVar[Dict[str, List[Dict[str, Any]]]] = {Also applies to: 36-72
74-100: Initialization logic is sound; consider tightening validation and avoiding duplication
initializecorrectly fails fast on missing/unknownshell_type, which is good. Two optional tweaks:
Agent.__init__already storestask_data, soself.task_data = task_datais likely redundant.- If
imageandvalidation_idare required upstream, you might also validate them here and log a clear error when missing.
101-166: Minor progress-calculation tweak for better UX when few checks existThe execute flow and use of
TaskStatus/callbacks look correct. One small UX improvement: withcurrent_progress = 70 + int((index / total_checks) * 25), the first (and only) check fortotal_checks == 1never advances progress beyond 70 before jumping to 100.Consider using
index + 1so each check visibly moves progress:- for index, check in enumerate(checks): + for index, check in enumerate(checks): @@ - current_progress = 70 + int((index / total_checks) * 25) + current_progress = 70 + int(((index + 1) / total_checks) * 25)
241-243: Clarifycancel_runsemantics vs. docstringThe docstring says “Cancel is not applicable for validation tasks” but the method returns
True, which usually means “cancellation supported/acknowledged” per the baseAgentcontract. Consider either returningFalseor adjusting the docstring to make the intent explicit so callers don’t misinterpret this.executor_manager/routers/routers.py (1)
88-92: Validation callback forwarding logic is sound; consider small robustness tweaks.Forwarding only when
result.validation_idis present and mapping executor statuses to validation statuses before POSTing to the backend is a good pattern, and local try/except in_forward_validation_callbackensures failures there do not break the main callback flow. You might still want to guard againstrequest.statusbeingNone(e.g., default to"completed"or"running_checks") and move thehttpximport to module level for consistency with the rest of the file and PEP 8.Also applies to: 117-162
backend/app/api/endpoints/adapter/shells.py (3)
31-146: Data models look consistent with executor and frontend; consider adding type hints on helpers.The Pydantic models (
UnifiedShell,ShellCreateRequest,ShellUpdateRequest,ImageValidation*,ValidationStatus*) line up with the described API and the referenced frontend interfaces, and the CRD conversion helpers correctly preserve labels/runtime/supportModel. To match project guidelines, you may want to add explicit type hints on_public_shell_to_unified/_user_shell_to_unifiedsignatures (e.g.,-> UnifiedShell) and on other new functions in this module.
620-671: Redis status helper is coherent; consider tightening types and logging failures with stack traces.
_update_validation_statuscorrectly preserves existing fields, applies partial updates, and resets TTL, which matches how the polling endpoint reads from Redis. Since callers always pass lists of dicts representing checks, you might tighten thecheckstype hint fromOptional[List[dict]]toOptional[List[ImageCheckResult]](and convert to dicts internally) and uselogger.exceptioninstead oflogger.errorin the catch-all to ease debugging on unexpected Redis errors.
673-703: Validation-status GET handler matches stored shape; minor polish only.The endpoint cleanly maps the cached dict back into
ValidationStatusResponseand returns 404 when no entry is found, which is exactly what the frontend polling flow needs. As a small improvement, you could normalize missingchecksfrom Redis to[]instead ofNoneto reduce null handling on the client, but the current behavior is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/api/endpoints/adapter/shells.py(1 hunks)executor/agents/image_validator/image_validator_agent.py(1 hunks)executor_manager/executors/docker/executor.py(6 hunks)executor_manager/routers/routers.py(3 hunks)frontend/src/apis/shells.ts(1 hunks)frontend/src/features/settings/components/ShellEdit.tsx(1 hunks)frontend/src/i18n/locales/en/common.json(2 hunks)frontend/src/i18n/locales/zh-CN/common.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/features/settings/components/ShellEdit.tsx
- frontend/src/i18n/locales/zh-CN/common.json
- frontend/src/apis/shells.ts
- frontend/src/i18n/locales/en/common.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments MUST be written in English, including inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions
Files:
executor_manager/routers/routers.pyexecutor/agents/image_validator/image_validator_agent.pyexecutor_manager/executors/docker/executor.pybackend/app/api/endpoints/adapter/shells.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant, formatted with Black (line length: 88), and use isort for imports
Python code must use type hints
Python functions should have descriptive names, include docstrings for public functions/classes, extract magic numbers to constants, and keep functions to a maximum of 50 lines preferred
Files:
executor_manager/routers/routers.pyexecutor/agents/image_validator/image_validator_agent.pyexecutor_manager/executors/docker/executor.pybackend/app/api/endpoints/adapter/shells.py
🧬 Code graph analysis (3)
executor_manager/routers/routers.py (2)
backend/app/api/endpoints/adapter/shells.py (2)
ImageCheckResult(69-75)validate_image(448-617)frontend/src/apis/shells.ts (1)
ImageCheckResult(44-49)
executor_manager/executors/docker/executor.py (1)
executor_manager/executors/docker/utils.py (1)
find_available_port(65-92)
backend/app/api/endpoints/adapter/shells.py (4)
backend/app/models/kind.py (1)
Kind(25-42)backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/schemas/kind.py (1)
Shell(125-132)backend/app/core/security.py (1)
get_current_user(28-50)
🪛 Ruff (0.14.6)
executor_manager/routers/routers.py
160-160: Do not catch blind exception: Exception
(BLE001)
161-161: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
324-328: Consider moving this statement to an else block
(TRY300)
326-326: f-string without any placeholders
Remove extraneous f prefix
(F541)
330-330: Do not catch blind exception: Exception
(BLE001)
331-331: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
334-334: Use explicit conversion flag
Replace with conversion flag
(RUF010)
executor/agents/image_validator/image_validator_agent.py
1-1: Shebang is present but file is not executable
(EXE001)
37-72: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
175-175: subprocess call: check for execution of untrusted input
(S603)
176-176: Starting a process with a partial executable path
(S607)
209-209: Do not catch blind exception: Exception
(BLE001)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
233-233: Do not catch blind exception: Exception
(BLE001)
234-234: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
executor_manager/executors/docker/executor.py
623-623: Do not catch blind exception: Exception
(BLE001)
624-624: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/api/endpoints/adapter/shells.py
150-150: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
151-151: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
184-184: Do not catch blind exception: Exception
(BLE001)
202-202: Do not catch blind exception: Exception
(BLE001)
214-214: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
215-215: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
261-261: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
262-262: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
361-361: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
362-362: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
416-416: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
417-417: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
450-450: Unused function argument: current_user
(ARG001)
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
509-509: Do not catch blind exception: Exception
(BLE001)
510-510: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
568-568: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
585-585: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
592-592: Use explicit conversion flag
Replace with conversion flag
(RUF010)
596-596: Use explicit conversion flag
Replace with conversion flag
(RUF010)
599-599: Use explicit conversion flag
Replace with conversion flag
(RUF010)
601-601: Do not catch blind exception: Exception
(BLE001)
602-602: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
609-609: Use explicit conversion flag
Replace with conversion flag
(RUF010)
613-613: Use explicit conversion flag
Replace with conversion flag
(RUF010)
667-667: Consider moving this statement to an else block
(TRY300)
668-668: Do not catch blind exception: Exception
(BLE001)
669-669: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
676-676: Unused function argument: current_user
(ARG001)
676-676: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
688-691: Abstract raise to an inner function
(TRY301)
705-705: Do not catch blind exception: Exception
(BLE001)
706-706: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
707-707: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
707-707: Use explicit conversion flag
Replace with conversion flag
(RUF010)
734-734: Abstract raise to an inner function
(TRY301)
736-736: Consider moving this statement to an else block
(TRY300)
739-739: Do not catch blind exception: Exception
(BLE001)
740-740: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
741-741: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
741-741: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (2)
executor_manager/executors/docker/executor.py (1)
19-19: Ensurehttpxis declared as a runtime dependency.This module now imports
httpx; please verify thathttpxis included in your dependency management (e.g.,requirements.txt,pyproject.toml) in all deployment environments where this executor runs, otherwise_report_validation_stagewill fail at import time.backend/app/api/endpoints/adapter/shells.py (1)
148-205: Unified listing/get semantics are reasonable and error handling is appropriate.Preferring user shells over public shells when
shell_typeis omitted, and returning specific 404 messages when only user shells are requested, matches the intended UX and keeps behavior predictable. The try/except around CRD parsing when listing shells is a good guard against bad data without taking down the whole endpoint.
| @router.post("/validation-status/{validation_id}") | ||
| async def update_validation_status( | ||
| validation_id: str, | ||
| request: ValidationStatusUpdateRequest, | ||
| ): | ||
| """ | ||
| Update the status of a validation task (internal API for Executor Manager callback). | ||
| This endpoint is called by Executor Manager to update validation progress. | ||
| Note: This is an internal API and should not be exposed publicly. | ||
| """ | ||
| try: | ||
| success = await _update_validation_status( | ||
| validation_id=validation_id, | ||
| status=request.status, | ||
| stage=request.stage, | ||
| progress=request.progress, | ||
| valid=request.valid, | ||
| checks=[c.model_dump() for c in request.checks] if request.checks else None, | ||
| errors=request.errors, | ||
| error_message=request.errorMessage, | ||
| ) | ||
|
|
||
| if not success: | ||
| raise HTTPException(status_code=500, detail="Failed to update validation status") | ||
|
|
||
| return {"status": "success", "message": "Validation status updated"} | ||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Error updating validation status: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Error updating validation status: {str(e)}") |
There was a problem hiding this comment.
Internal status update endpoint lacks authentication/authorization safeguards.
update_validation_status is intentionally marked as internal, but it is currently completely unauthenticated: any caller who can reach the service and guess a validation_id can mutate validation state in Redis. Consider adding at least a shared-secret header, IP allowlist, or a lightweight auth mechanism so only the executor-manager can call this endpoint, rather than relying solely on deployment topology and comments.
🧰 Tools
🪛 Ruff (0.14.6)
734-734: Abstract raise to an inner function
(TRY301)
736-736: Consider moving this statement to an else block
(TRY300)
739-739: Do not catch blind exception: Exception
(BLE001)
740-740: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
741-741: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
741-741: Use explicit conversion flag
Replace with conversion flag
(RUF010)
| def _report_validation_stage( | ||
| self, | ||
| task: Dict[str, Any], | ||
| stage: str, | ||
| status: str, | ||
| progress: int, | ||
| message: str, | ||
| error_message: Optional[str] = None, | ||
| valid: Optional[bool] = None, | ||
| ) -> None: | ||
| """ | ||
| Report validation stage progress to Backend via HTTP call. | ||
| Args: | ||
| task: Task data containing validation_params | ||
| stage: Current validation stage (pulling_image, starting_container, etc.) | ||
| status: Status (running, failed, completed) | ||
| progress: Progress percentage (0-100) | ||
| message: Human-readable message | ||
| error_message: Optional error message | ||
| valid: Optional validation result (True/False/None) | ||
| """ | ||
| validation_params = task.get("validation_params", {}) | ||
| validation_id = validation_params.get("validation_id") | ||
|
|
||
| if not validation_id: | ||
| logger.debug("No validation_id in task, skipping stage report") | ||
| return | ||
|
|
||
| task_api_domain = os.getenv("TASK_API_DOMAIN", "http://localhost:8000") | ||
| update_url = f"{task_api_domain}/api/shells/validation-status/{validation_id}" | ||
|
|
||
| update_payload = { | ||
| "status": "completed" if status == "failed" else stage, | ||
| "stage": message, | ||
| "progress": progress, | ||
| "valid": valid, | ||
| "errorMessage": error_message, | ||
| } | ||
|
|
||
| try: | ||
| with httpx.Client(timeout=10.0) as client: | ||
| response = client.post(update_url, json=update_payload) | ||
| if response.status_code == 200: | ||
| logger.info(f"Reported validation stage: {validation_id} -> {stage} ({progress}%)") | ||
| else: | ||
| logger.warning(f"Failed to report validation stage: {response.status_code} {response.text}") | ||
| except Exception as e: | ||
| logger.error(f"Error reporting validation stage: {e}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find callers of _report_validation_stage to understand the actual flow
rg "_report_validation_stage" --type py -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 1429
🏁 Script executed:
# Search for any validation-status API references or backend schema docs
rg "validation-status" --type py -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 2017
🏁 Script executed:
# Check exception logging patterns in the codebase
rg "logger\.exception" --type py | head -20Repository: wecode-ai/Wegent
Length of output: 2192
🏁 Script executed:
# Look for exception handlers to see what pattern is used in similar code
rg "except Exception" --type py -A 1 | head -30Repository: wecode-ai/Wegent
Length of output: 1430
🏁 Script executed:
# Check the full context of the _report_validation_stage method and surrounding code
sed -n '550,650p' executor_manager/executors/docker/executor.pyRepository: wecode-ai/Wegent
Length of output: 2803
🏁 Script executed:
# Find the backend endpoint handler for validation status update
rg "async def update_validation_status" -A 30 backend/app/api/endpoints/adapter/shells.pyRepository: wecode-ai/Wegent
Length of output: 1270
🏁 Script executed:
# Check for ValidationStatusResponse or related schemas
rg "ValidationStatusResponse|class.*Status.*Response" --type py -A 10 backend/app/api/Repository: wecode-ai/Wegent
Length of output: 2806
🏁 Script executed:
# Find complete calls to _report_validation_stage with all parameters
rg "_report_validation_stage" --type py -B 2 -A 8Repository: wecode-ai/Wegent
Length of output: 2878
🏁 Script executed:
# Check for any models or schemas related to validation status
fd -e py -x rg -l "validation.*status" {} \; | head -10Repository: wecode-ai/Wegent
Length of output: 191
Fix payload field mapping and update exception logging.
The implementation of _report_validation_stage has a critical bug that breaks the API contract with the backend. The backend endpoint expects separate status and stage fields, where:
statusshould be values like "running", "failed", "completed"stageshould be the stage identifier like "pulling_image", "starting_container"
Current code incorrectly maps:
update_payload = {
"status": "completed" if status == "failed" else stage, # Wrong: sends stage value and inverts "failed" to "completed"
"stage": message, # Wrong: puts human message in stage field
...
}This breaks the backend state machine. Use correct field mapping:
update_payload = {
"status": status, # "running", "failed", "completed"
"stage": stage, # "pulling_image", "starting_container"
"progress": progress,
"valid": valid,
"errorMessage": error_message,
}Also update exception logging to use logger.exception() to capture stack traces and align with codebase standards:
except Exception:
logger.exception("Error reporting validation stage")🧰 Tools
🪛 Ruff (0.14.6)
623-623: Do not catch blind exception: Exception
(BLE001)
624-624: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In executor_manager/executors/docker/executor.py around lines 576 to 624, the
payload fields are mis-mapped (sending stage in the status field and message in
the stage field, and inverting "failed" to "completed") and the exception
handling uses logger.error which loses the stack trace; fix by setting
update_payload["status"] = status and update_payload["stage"] = stage (keep
progress, valid, errorMessage as-is) so the backend gets the expected status and
stage values, and replace the except block to call logger.exception("Error
reporting validation stage") (remove the exception variable) to capture the
stack trace.
| def _run_check(self, check: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Run a single validation check""" | ||
| name = check["name"] | ||
| command = check["command"] | ||
| version_regex = check["version_regex"] | ||
| min_version = check.get("min_version") | ||
|
|
||
| try: | ||
| result = subprocess.run( | ||
| ["sh", "-c", command], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
|
|
||
| output = result.stdout.strip() | ||
| if result.returncode != 0 or "not found" in output.lower(): | ||
| logger.warning(f"Check '{name}' failed: command returned error or not found") | ||
| return { | ||
| "name": name, | ||
| "status": "fail", | ||
| "message": "Command failed or not found", | ||
| } | ||
|
|
||
| # Extract version | ||
| version_match = re.search(version_regex, output) | ||
| if version_match: | ||
| version = version_match.group(1) | ||
|
|
||
| # Check minimum version if specified | ||
| if min_version: | ||
| try: | ||
| from packaging import version as pkg_version | ||
|
|
||
| if pkg_version.parse(version) < pkg_version.parse(min_version): | ||
| logger.warning(f"Check '{name}': version {version} < required {min_version}") | ||
| return { | ||
| "name": name, | ||
| "version": version, | ||
| "status": "fail", | ||
| "message": f"Version {version} < required {min_version}", | ||
| } | ||
| except Exception as e: | ||
| logger.warning(f"Version comparison error for '{name}': {e}") | ||
|
|
||
| logger.info(f"Check '{name}' passed: version={version}") | ||
| return { | ||
| "name": name, | ||
| "version": version, | ||
| "status": "pass", | ||
| } | ||
| else: | ||
| logger.info(f"Check '{name}' passed but version not parsed") | ||
| return { | ||
| "name": name, | ||
| "status": "pass", | ||
| "message": "Detected but version not parsed", | ||
| } | ||
|
|
||
| except subprocess.TimeoutExpired: | ||
| logger.error(f"Check '{name}' timed out") | ||
| return { | ||
| "name": name, | ||
| "status": "fail", | ||
| "message": "Check timed out", | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Check '{name}' error: {e}") | ||
| return { | ||
| "name": name, | ||
| "status": "fail", | ||
| "message": str(e), | ||
| } | ||
|
|
There was a problem hiding this comment.
Version parsing should consider stderr; also align logging with Ruff suggestions
Right now you only parse result.stdout. Many CLIs (including some Python versions) print --version output to stderr, which will cause version_regex to miss and skip min_version enforcement, potentially marking an incompatible image as valid. You also have Ruff hints about logging in exception branches.
I’d recommend:
- Combine stdout and stderr for version parsing and “not found” detection.
- Use
logging.exceptionin exception handlers to satisfy TRY400 and get tracebacks.
This keeps the subprocess pattern (S603/S607) acceptable as long as command remains internal and not user-controlled.
def _run_check(self, check: Dict[str, Any]) -> Dict[str, Any]:
@@
try:
result = subprocess.run(
["sh", "-c", command],
capture_output=True,
text=True,
timeout=30,
)
- output = result.stdout.strip()
+ stdout = result.stdout or ""
+ stderr = result.stderr or ""
+ output = f"{stdout}\n{stderr}".strip()
if result.returncode != 0 or "not found" in output.lower():
@@
- except subprocess.TimeoutExpired:
- logger.error(f"Check '{name}' timed out")
+ except subprocess.TimeoutExpired:
+ logger.exception("Check '%s' timed out", name)
return {
"name": name,
"status": "fail",
"message": "Check timed out",
}
- except Exception as e:
- logger.error(f"Check '{name}' error: {e}")
+ except Exception as e:
+ logger.exception("Check '%s' error", name)
return {
"name": name,
"status": "fail",
"message": str(e),
}If you want to fully address BLE001 as well, you could narrow the final except Exception to specific exception types or add an explicit # noqa: BLE001 with a brief justification.
🧰 Tools
🪛 Ruff (0.14.6)
175-175: subprocess call: check for execution of untrusted input
(S603)
176-176: Starting a process with a partial executable path
(S607)
209-209: Do not catch blind exception: Exception
(BLE001)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
233-233: Do not catch blind exception: Exception
(BLE001)
234-234: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In executor/agents/image_validator/image_validator_agent.py around lines
167-240, the code only inspects result.stdout and uses
logger.error/logger.warning in exception branches; update the function to (1)
combine result.stdout and result.stderr (e.g., output = (result.stdout + "\n" +
result.stderr).strip()) and use that combined output for the version_regex
search and the "not found" detection, and (2) replace logger.error(...) in the
except blocks with logging.exception(...) (or logger.exception(...)) so
tracebacks are logged; optionally tighten the final bare except to specific
exceptions or add a brief "# noqa: BLE001" justification if you must keep a
broad catch.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executor_manager/requirements.txt(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
executor_manager/requirements.txt
[error] 501-501: During 'pip install -r requirements.txt' under --require-hashes mode, all requirements must be pinned with '=='. The following entry is not: httpx>=0.24.0 (line 501).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
executor_manager/requirements.txt
Outdated
| httpx>=0.24.0 | ||
| # via executor-manager (for validation status reporting) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -20 executor_manager/requirements.txtRepository: wecode-ai/Wegent
Length of output: 1069
🏁 Script executed:
sed -n '495,506p' executor_manager/requirements.txtRepository: wecode-ai/Wegent
Length of output: 598
🏁 Script executed:
# Check a few example entries to confirm the == and hash pattern
sed -n '10,30p' executor_manager/requirements.txtRepository: wecode-ai/Wegent
Length of output: 978
🏁 Script executed:
# Look for httpx in other config files
find . -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" | head -5Repository: wecode-ai/Wegent
Length of output: 140
🏁 Script executed:
cat executor_manager/pyproject.tomlRepository: wecode-ai/Wegent
Length of output: 555
🏁 Script executed:
uv export --format requirements-txt --output-file executor_manager/requirements.txtRepository: wecode-ai/Wegent
Length of output: 102
Fix version pinning and add missing hashes for httpx.
The httpx dependency violates the --require-hashes mode enforced by this file. Like all other entries in this auto-generated file, it must be pinned with == (not >=) and include SHA256 hashes. The entry is also missing the backslash continuation character.
This causes an immediate pipeline failure: pip install under --require-hashes mode rejects the entry.
Additionally, httpx is not declared in executor_manager/pyproject.toml. If this is an intentional new dependency, add it to pyproject.toml first, then regenerate requirements.txt using uv export. If httpx was added manually in error, remove it from requirements.txt.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 501-501: During 'pip install -r requirements.txt' under --require-hashes mode, all requirements must be pinned with '=='. The following entry is not: httpx>=0.24.0 (line 501).
🤖 Prompt for AI Agents
In executor_manager/requirements.txt around lines 501-502, the httpx entry is
non-compliant with --require-hashes: it uses >= instead of ==, lacks the
line-continuation backslash and SHA256 hashes, and httpx is not declared in
executor_manager/pyproject.toml. Either remove the manual httpx line if it was
added by mistake, or add httpx to executor_manager/pyproject.toml and then
regenerate requirements.txt with uv export so the entry is pinned (==version),
includes the trailing backslash continuation and the required SHA256 hashes for
all wheels/sdists; do not hand-edit hashes—recreate the file from pyproject to
ensure consistency.
…orting Add httpx>=0.24.0 to requirements.txt to fix CI test failure. The httpx library is required by docker executor to report validation stage progress to the backend API.
qdaxb
force-pushed
the
wegent/feature-custom-shell-base-image
branch
from
ab292c4 to
073d867
Compare
Fix ESLint errors for unused variables: - Rename validationId to _validationId (stored for future use) - Remove pollingCount state (internal count only needed in closure)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
executor/services/agent_service.py (1)
60-61: Normalize_agent_sessionskeys to avoid losing agent reuse and cancellation
create_agentlooks up an existing agent viaself.get_agent(f"{task_id}"), but stores sessions asself._agent_sessions[task_id] = AgentSession(...). Elsewhere (delete_session_async,cancel_task,send_cancel_callback_async) access_agent_sessionsusingtask_iddirectly, sometimes typed asstr, sometimes asint. Iftask_idintask_datais an integer (typical for JSON), this string/int mismatch will prevent:
- Reusing existing agents for the same
task_id(a new agent is created each time).- Session deletion/cancellation from finding the stored session.
- Proper cleanup of long-lived sessions.
Consider normalizing keys to strings everywhere (or ints everywhere) and using a helper to enforce it:
- def get_agent(self, agent_session_id: str) -> Optional[Agent]: - session = self._agent_sessions.get(agent_session_id) + def get_agent(self, agent_session_id: str) -> Optional[Agent]: + session = self._agent_sessions.get(str(agent_session_id)) return session.agent if session else None @@ - if (existing_agent := self.get_agent(f"{task_id}")): + session_id = str(task_id) + if (existing_agent := self.get_agent(session_id)): @@ - self._agent_sessions[task_id] = AgentSession(agent=agent, created_at=time.time()) + self._agent_sessions[session_id] = AgentSession(agent=agent, created_at=time.time())And similarly wrap lookups in
delete_session_async,cancel_task, andsend_cancel_callback_asyncwithstr(task_id). This will make reuse, cancellation, and validation flows more reliable.Also applies to: 94-95
docs/en/reference/yaml-specification.md (1)
95-95: Inconsistent language: Chinese text in English documentation.The description contains Chinese text "Skill功能描述" which should be translated to English for consistency in this English documentation file.
-| `spec.description` | string | Yes | Skill功能描述 (extracted from SKILL.md frontmatter) | +| `spec.description` | string | Yes | Skill description (extracted from SKILL.md frontmatter) |executor/callback/callback_handler.py (2)
42-50: Docstring type annotations don't match function signatures.The docstrings document
task_idasstrbut the function signature declares it asint. This inconsistency appears in multiple functions (send_status_callback,send_task_started_callback,send_task_completed_callback,send_task_failed_callback).Args: - task_id (str): Task ID + task_id (int): Task ID + subtask_id (int): Subtask ID task_title (str): Task title
107-119: Add error handling to callback functions to matchsend_status_callbackpattern.The
send_task_started_callback,send_task_completed_callback, andsend_task_failed_callbackfunctions directly callcallback_client.send_callback()without try/except wrappers. This is inconsistent withsend_status_callback, which catches exceptions and returns an error dict.Line 119 in
task_processor.pyassumesresultis a dict (calling.get()on it), but if an exception occurs insend_task_started_callback, it will propagate and crash with anAttributeError. Similarly, exceptions insend_task_completed_callback(line 144) andsend_task_failed_callback(line 159) will propagate uncaught since these calls are outside the try/except block that wrapsagent_service.execute_task().All three functions should wrap
callback_client.send_callback()in try/except blocks and return error dicts likesend_status_callbackdoes, or document that callers must add exception handling.
♻️ Duplicate comments (9)
backend/app/services/adapters/executor_kinds.py (1)
411-433: Static analysis: Use.is_(True)for SQLAlchemy boolean comparison.Line 420 uses
== Truewhich Ruff flags (E712). Additionally, the inlineMockShellclass definition is a code smell that was flagged in a previous review.executor_manager/executors/docker/binary_extractor.py (1)
200-241: Command injection risk via shell heredoc embedding.The
image_digestvalue is embedded directly into the shell script string at line 232. While in most cases this comes fromdocker inspectoutput (which should be safe), if the digest retrieval fails, line 184 falls back to usingexecutor_imagedirectly, which originates from an environment variable that could potentially contain shell metacharacters.Consider using
shlex.quoteor writing the version file separately to avoid any risk:+import shlex + def _extract_binary_to_volume(executor_image: str) -> bool: # ... existing code ... + escaped_digest = shlex.quote(image_digest) extract_cmd = f""" # ... other commands ... - echo '{image_digest}' > {VERSION_FILE_PATH} + echo {escaped_digest} > {VERSION_FILE_PATH} # ... rest of script ... """executor_manager/routers/routers.py (2)
317-317: Remove duplicate import.The
timemodule is already imported at line 14. This import statement inside the function is redundant.- import time validation_task_id = -(int(time.time() * 1000) % 1000000) # Negative ID for validation tasks
264-269: Response model doesn't match actual response structure.The
ValidateImageResponsemodel only declaresstatus,message, andvalidation_task_id, but the actual responses at lines 297-303, 307-313, and 353-358 return additional fields:valid,checks, anderrors.class ValidateImageResponse(BaseModel): """Response for image validation""" status: str # 'submitted' for async validation message: str validation_task_id: Optional[int] = None + valid: Optional[bool] = None + checks: Optional[list] = None + errors: Optional[list] = Nonebackend/app/api/endpoints/adapter/shells.py (3)
5-13: Moveosandhttpximports to module level.
validate_imagestill importsosandhttpxinside the function body. This is against our usual style (and PEP 8) and makes the module slightly harder to analyze/test.You already use
httpx.AsyncClient, so you can safely move these imports to the top:-import logging -import re -import uuid -from datetime import datetime -from typing import List, Optional +import logging +import os +import re +import uuid +from datetime import datetime +from typing import List, Optional @@ -from fastapi import APIRouter, Depends, HTTPException, Query, status -from pydantic import BaseModel -from sqlalchemy.orm import Session +from fastapi import APIRouter, Depends, HTTPException, Query, status +import httpx +from pydantic import BaseModel +from sqlalchemy.orm import Session @@ - """ - Validate if a base image is compatible with a specific shell type. - ... - """ - import os - - import httpx + """ + Validate if a base image is compatible with a specific shell type. + ... + """Also applies to: 466-489
432-463: Prevent deleting shells that are still referenced by active bots.
delete_shellperforms a soft delete (is_active = False) without checking whether any active bots reference this shell (viashellRef.name). That makes it easy to create broken bots that will fail to load or run later when their shell disappears.Given existing validation around bot → shell references (
BotKindService._validate_referencesinkind_impl.py), consider adding a guard here before soft-deleting, e.g.:
- Query for active bots referencing
shell_namein the same namespace.- If any exist, either:
- Reject the delete with a 400/409 and a clear message, or
- Return a warning / list of dependent bots so the user can clean them up first.
This keeps referential integrity between shells and bots and avoids hard-to-debug runtime failures.
734-769: Secure the internalupdate_validation_statusendpoint.
POST /validation-status/{validation_id}is explicitly intended as an internal callback for Executor Manager, but it currently has:
- No
Depends(security.get_current_user)or other auth,- No shared secret, token, or IP allowlist check.
Anyone who can reach this service and guess a
validation_idcould tamper with validation results in Redis. At minimum, add one of:
- A shared secret header (e.g.,
X-Executor-Token) validated against an env var.- A dedicated service account / JWT that Executor Manager uses, validated via
security.- Network-level restriction plus a lightweight assertion (e.g., fixed origin header).
Example (shared-secret approach):
@router.post("/validation-status/{validation_id}") async def update_validation_status( - validation_id: str, - request: ValidationStatusUpdateRequest, + validation_id: str, + request: ValidationStatusUpdateRequest, + executor_token: str = Header(..., alias="X-Executor-Token"), ): @@ - try: + expected = os.getenv("EXECUTOR_MANAGER_TOKEN") + if not expected or executor_token != expected: + raise HTTPException(status_code=401, detail="Unauthorized executor callback") + + try: success = await _update_validation_status(Adjust the mechanism to match your existing infra, but some explicit auth check should be present.
executor_manager/executors/docker/executor.py (2)
367-387: Fix binary_extractor imports (module path + move to module level).This still has the two issues previously flagged:
- The import path
from executors.docker.binary_extractor import ...is inconsistent with the rest of the codebase, where modules live underexecutor_manager.executors.docker. This is likely to raiseModuleNotFoundErrorat runtime.- The imports for
extract_executor_binaryandEXECUTOR_BINARY_VOLUMElive inside methods instead of at module level, which goes against our usual import style and makes them harder to track/test.Suggested refactor:
-from executor_manager.executors.docker.constants import ( +from executor_manager.executors.docker.constants import ( CONTAINER_OWNER, DEFAULT_DOCKER_HOST, DEFAULT_API_ENDPOINT, DEFAULT_TIMEZONE, DEFAULT_LOCALE, DOCKER_SOCKET_PATH, WORKSPACE_MOUNT_PATH, DEFAULT_PROGRESS_RUNNING, DEFAULT_PROGRESS_COMPLETE, DEFAULT_TASK_ID, ) +from executor_manager.executors.docker.binary_extractor import ( + EXECUTOR_BINARY_VOLUME, + extract_executor_binary, +)Then remove the inline imports:
- from executors.docker.binary_extractor import extract_executor_binary @@ - from executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUMEThis keeps imports consistent and avoids surprises at runtime.
Also applies to: 428-468
738-786: Correct validation status payload mapping and improve exception logging.
_report_validation_stageis still sending mismatched fields to/api/shells/validation-status/{validation_id}:update_payload = { "status": "completed" if status == "failed" else stage, "stage": message, ... }This:
- Overwrites
"failed"into"completed", and- Puts the human-readable message into the
stagefield instead of the stage identifier.Given the backend’s
ValidationStatusUpdateRequestand helper_update_validation_status, the payload should preserve both:
status: high-level status ("running","failed","completed").stage: machine-readable stage identifier ("pulling_image","starting_container", etc.).And keep the message in
errorMessageor separate UI-level status if needed. A minimal fix:- update_payload = { - "status": "completed" if status == "failed" else stage, - "stage": message, + update_payload = { + "status": status, + "stage": stage, "progress": progress, "valid": valid, "errorMessage": error_message, } @@ - except Exception as e: - logger.error(f"Error reporting validation stage: {e}") + except Exception: + logger.exception("Error reporting validation stage")This aligns the executor’s payload with the backend schema and preserves stack traces on errors.
Please confirm in the latest backend docs for `/api/shells/validation-status/{validation_id}` what the intended values and meanings are for the `status` and `stage` fields, so we can be sure this mapping matches the contract.
🧹 Nitpick comments (14)
frontend/src/features/settings/components/TeamEdit.tsx (2)
91-105: Shell-aware bot filtering is wired correctly; consider user-visible error when shell load failsLoading unified shells on mount and passing
shellsintogetFilteredBotsForMode(bots, mode, shells)matches the updated helper signature and allows correct handling of custom shell runtime types. The data-flow (shellApis.getUnifiedShells→setShells(response.data || [])→filteredBotsuseMemo) looks sound.Right now, a failure to load shells only emits
console.error. If shell metadata becomes important for allowed/blocked combinations, you may want a lightweight user-visible warning (e.g., toast) so users know why filtering falls back to a less accurate state.Also applies to: 108-111
293-298:selectedShellTypeis always null—simplify or clarify its intended use
selectedShellTypeis memoized to always returnnull, and the surrounding comments indicate shell_type restrictions have been removed. It is only passed down toLeaderModeEditor:const selectedShellType = useMemo(() => { // No shell_type restriction - always return null return null; }, []);If
nullsimply means “no restriction” forLeaderModeEditor, you can:
- Either drop the
useMemoand passnulldirectly, or- Remove the prop altogether if the editor no longer needs shell restrictions.
This will make the intent clearer and avoid suggesting there’s dynamic shell selection when there isn’t.
Also applies to: 300-304, 315-316, 625-625
backend/app/services/adapters/task_kinds.py (1)
551-552: Bot detail now surfacesshell_typefrom Shell CRD as expectedInitializing
shell_typeand then, when available, setting it fromshell_crd.spec.shellTypebefore placing it intobot_dict["shell_type"]keeps task-detail bot objects aligned with the new Shell spec and frontend Bot expectations. If the Shell CRD is missing or inactive,shell_typecorrectly falls back to an empty string, mirroring previous “no runtime” behavior.If you anticipate partially migrated Shell CRDs, you might optionally add a defensive fallback (e.g., using
bot_crd.spec.shellRef.namewhenshellTypeis empty), but the current logic is consistent with the new schema.Also applies to: 590-605, 607-612
executor/tasks/task_processor.py (1)
104-118: Consider simplifying the validation_params extraction.The current code handles
validation_paramsbeing missing (returns{}) but Line 106's conditionalif validation_paramsis redundant since.get()already returns{}when the key is missing, which is truthy when empty but the subsequent.get("validation_id")would returnNoneanyway.However, there's a subtle issue: if
validation_paramsis explicitly set toNoneintask_data, the code would fail on Line 106 withAttributeErrorwhen calling.get()onNone.- validation_params = task_data.get("validation_params", {}) - validation_id = validation_params.get("validation_id") if validation_params else None + validation_params = task_data.get("validation_params") or {} + validation_id = validation_params.get("validation_id")backend/app/services/adapters/team_kinds.py (1)
864-873: Consider extracting the shell type to agent type mapping.The mapping logic is duplicated conceptually and could benefit from being a utility function, especially if this mapping is needed elsewhere.
+SHELL_TYPE_TO_AGENT_TYPE = { + 'Agno': 'agno', + 'ClaudeCode': 'claude', + 'Dify': 'dify', +} + +def get_agent_type_from_shell_type(shell_type: str) -> Optional[str]: + """Map shellType to agent type identifier.""" + if shell_type in SHELL_TYPE_TO_AGENT_TYPE: + return SHELL_TYPE_TO_AGENT_TYPE[shell_type] + return shell_type.lower() if shell_type else Nonefrontend/src/features/settings/components/BotEdit.tsx (2)
756-764: Good UX for distinguishing custom shells.The dropdown correctly displays a visual badge
[自定义]for user-defined shells (shell.type === 'user'), helping users distinguish between public and custom shells.Consider providing a translation key fallback for better i18n support:
- <span className="ml-1 text-xs text-text-muted"> - [{t('bot.custom_shell', '自定义')}] - </span> + <span className="ml-1 text-xs text-text-muted"> + [{t('bot.custom_shell')}] + </span>Ensure the translation key
bot.custom_shellexists in both locale files.
1210-1372: Consider extracting inline styles to a separate CSS file.The extensive inline
<style>block withdangerouslySetInnerHTMLis a code smell. While it works, this approach:
- Bypasses React's style encapsulation
- Makes styles harder to maintain
- Could potentially be a vector for XSS if content were dynamic
Consider moving these responsive styles to a CSS module or Tailwind configuration.
backend/app/api/endpoints/adapter/models.py (1)
355-408: Consider consolidating compatible models logic.The
get_compatible_modelsendpoint duplicates shell-type filtering logic that likely exists elsewhere in the model aggregation service. The hardcoded mapping ("Agno" → "openai","ClaudeCode" → "claude") should ideally be centralized.Also, the static analysis hint about
Depends()in argument defaults (B008) is a false positive for FastAPI's dependency injection pattern - this is the standard approach.backend/app/services/adapters/bot_kinds.py (1)
838-846: Prefix unused variables with underscore.The
shellandmodelvariables from unpacking are not used indelete_with_user. Prefix them to indicate intentional non-use:- ghost, shell, model = self._get_bot_components(db, bot, user_id) + ghost, _shell, _model = self._get_bot_components(db, bot, user_id) # Delete bot and ghost only # Shell is not deleted because it's a reference to user's custom shell or public shell db.delete(bot) if ghost: db.delete(ghost) - # Note: shell is not deleted - it's a shared resourceexecutor_manager/executors/docker/binary_extractor.py (1)
264-269: Uselogger.exceptionto preserve stack traces.When catching exceptions in error paths, using
logger.exceptioninstead oflogger.errorwill automatically include the stack trace, which is valuable for debugging extraction failures.except subprocess.TimeoutExpired: - logger.error("Binary extraction timed out") + logger.exception("Binary extraction timed out") return False except Exception as e: - logger.error(f"Error extracting binary: {e}") + logger.exception(f"Error extracting binary: {e}") return FalseThe same pattern should be applied at lines 79-80 and 104-106.
executor_manager/routers/routers.py (2)
181-182: Uselogger.exceptionto preserve stack trace.When catching exceptions, using
logger.exceptioninstead oflogger.errorwill automatically include the stack trace for better debugging.except Exception as e: - logger.error(f"Error forwarding validation callback: {e}") + logger.exception(f"Error forwarding validation callback: {e}")
345-349: Remove extraneous f-string prefix.The string at line 347 has no placeholders, so the
fprefix is unnecessary.return { "status": "submitted", - "message": f"Validation task submitted. Results will be returned via callback.", + "message": "Validation task submitted. Results will be returned via callback.", "validation_task_id": validation_task_id, }frontend/src/apis/shells.ts (1)
82-162: Avoid relying onthisinsideshellApismethods to keep them safe when destructured.
getPublicShells/getLocalEngineShellscallthis.getUnifiedShells(). This works when invoked asshellApis.getPublicShells(), but will break if the method is ever extracted (const { getPublicShells } = shellApis). To make these helpers more robust you can:
- Call the function via the object:
const response = await shellApis.getUnifiedShells(), or- Hoist
getUnifiedShellsas a standalone function and reference it directly.Optionally, you could also parametrize
apiClientwith generics (e.g.apiClient.get<UnifiedShellListResponse>(...)) to enforce response typing at the call site.backend/app/services/adapters/shell_utils.py (1)
41-75: Tighten filters (is_activestyle) and fix batch debug-count calculation.Two small things here:
- Boolean comparisons (E712):
In all queries usingKind.is_active == True/PublicShell.is_active == True, prefer the idiomatic form:- Kind.is_active == True + Kind.is_active - PublicShell.is_active == True + PublicShell.is_activeThis removes Ruff E712 warnings and is more in line with typical SQLAlchemy style.
- Batch fetch logging math:
Inget_shells_by_names_batch,found_keys/missing_keysare computed before public shells are fetched, but later used to infer user/public counts:logger.debug( f"Batch fetched {len(shell_map)} shells for {len(shell_keys)} keys " f"(user: {len(shell_map) - len(missing_keys) + len(found_keys)}, " f"public: {len(shell_map) - len(found_keys)})" )If no public shells are found,
len(shell_map)is the user count whilemissing_keysstill reflects the pre-public-query set, so the “user” count can go negative. A simple fix is to track counts explicitly:- if user_shell_filter is not None: - user_shells = ( + user_shells: List[Kind] = [] + if user_shell_filter is not None: + user_shells = ( db.query(Kind) ... ) ... - if missing_keys: + public_shells: List[PublicShell] = [] + if missing_keys: ... - if public_shell_filter is not None: - public_shells = ( + if public_shell_filter is not None: + public_shells = ( db.query(PublicShell) ... ) ... - logger.debug( - f"Batch fetched {len(shell_map)} shells for {len(shell_keys)} keys " - f"(user: {len(shell_map) - len(missing_keys) + len(found_keys)}, " - f"public: {len(shell_map) - len(found_keys)})" - ) + logger.debug( + f"Batch fetched {len(shell_map)} shells for {len(shell_keys)} keys " + f"(user: {len(user_shells)}, public: {len(public_shells)})" + )Also applies to: 171-191, 231-314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
backend/app/api/endpoints/adapter/models.py(6 hunks)backend/app/api/endpoints/adapter/shells.py(1 hunks)backend/app/schemas/bot.py(2 hunks)backend/app/schemas/kind.py(2 hunks)backend/app/services/adapters/bot_kinds.py(12 hunks)backend/app/services/adapters/executor_kinds.py(5 hunks)backend/app/services/adapters/public_model.py(2 hunks)backend/app/services/adapters/public_shell.py(2 hunks)backend/app/services/adapters/shell_utils.py(2 hunks)backend/app/services/adapters/task_kinds.py(2 hunks)backend/app/services/adapters/team_kinds.py(10 hunks)backend/app/services/model_aggregation_service.py(6 hunks)backend/init_data/02-public-shells.yaml(3 hunks)docs/en/reference/yaml-specification.md(3 hunks)docs/zh/reference/yaml-specification.md(3 hunks)executor/agents/base.py(3 hunks)executor/callback/callback_client.py(3 hunks)executor/callback/callback_handler.py(12 hunks)executor/services/agent_service.py(1 hunks)executor/tasks/task_processor.py(3 hunks)executor_manager/executors/docker/binary_extractor.py(1 hunks)executor_manager/executors/docker/executor.py(8 hunks)executor_manager/routers/routers.py(4 hunks)frontend/src/apis/bots.ts(2 hunks)frontend/src/apis/mocks/bot.ts(3 hunks)frontend/src/apis/models.ts(3 hunks)frontend/src/apis/shells.ts(1 hunks)frontend/src/features/settings/components/BotEdit.tsx(9 hunks)frontend/src/features/settings/components/BotList.tsx(1 hunks)frontend/src/features/settings/components/ShellEdit.tsx(1 hunks)frontend/src/features/settings/components/ShellList.tsx(1 hunks)frontend/src/features/settings/components/TeamEdit.tsx(6 hunks)frontend/src/features/settings/components/team-modes/BotTransfer.tsx(3 hunks)frontend/src/features/settings/components/team-modes/LeaderModeEditor.tsx(5 hunks)frontend/src/features/settings/components/team-modes/SoloModeEditor.tsx(2 hunks)frontend/src/features/settings/components/team-modes/index.ts(2 hunks)frontend/src/features/settings/services/bots.ts(1 hunks)frontend/src/i18n/locales/en/common.json(3 hunks)frontend/src/i18n/locales/zh-CN/common.json(3 hunks)frontend/src/types/api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/features/settings/components/ShellEdit.tsx
- frontend/src/features/settings/components/ShellList.tsx
- frontend/src/i18n/locales/zh-CN/common.json
- backend/init_data/02-public-shells.yaml
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments MUST be written in English, including inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions
Files:
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsxfrontend/src/features/settings/services/bots.tsfrontend/src/apis/bots.tsfrontend/src/features/settings/components/BotList.tsxbackend/app/services/adapters/public_shell.pyfrontend/src/features/settings/components/team-modes/BotTransfer.tsxfrontend/src/features/settings/components/team-modes/LeaderModeEditor.tsxfrontend/src/features/settings/components/team-modes/index.tsbackend/app/services/adapters/public_model.pyfrontend/src/types/api.tsexecutor/agents/base.pyfrontend/src/features/settings/components/TeamEdit.tsxbackend/app/api/endpoints/adapter/models.pyfrontend/src/apis/mocks/bot.tsbackend/app/services/adapters/task_kinds.pyexecutor/tasks/task_processor.pyexecutor/callback/callback_handler.pybackend/app/schemas/kind.pybackend/app/services/adapters/team_kinds.pyfrontend/src/features/settings/components/BotEdit.tsxexecutor/services/agent_service.pyfrontend/src/apis/shells.tsbackend/app/schemas/bot.pyexecutor/callback/callback_client.pybackend/app/services/adapters/bot_kinds.pyfrontend/src/apis/models.tsexecutor_manager/executors/docker/binary_extractor.pybackend/app/services/model_aggregation_service.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/routers/routers.pybackend/app/services/adapters/shell_utils.pybackend/app/api/endpoints/adapter/shells.pybackend/app/services/adapters/executor_kinds.py
**/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript/React code must use strict mode, functional components with hooks, Prettier formatter, ESLint (Next.js config), single quotes, and no semicolons
Files:
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsxfrontend/src/features/settings/services/bots.tsfrontend/src/apis/bots.tsfrontend/src/features/settings/components/BotList.tsxfrontend/src/features/settings/components/team-modes/BotTransfer.tsxfrontend/src/features/settings/components/team-modes/LeaderModeEditor.tsxfrontend/src/features/settings/components/team-modes/index.tsfrontend/src/types/api.tsfrontend/src/features/settings/components/TeamEdit.tsxfrontend/src/apis/mocks/bot.tsfrontend/src/features/settings/components/BotEdit.tsxfrontend/src/apis/shells.tsfrontend/src/apis/models.ts
**/frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
constoverlet, nevervarin TypeScript/JavaScript
Files:
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsxfrontend/src/features/settings/services/bots.tsfrontend/src/apis/bots.tsfrontend/src/features/settings/components/BotList.tsxfrontend/src/features/settings/components/team-modes/BotTransfer.tsxfrontend/src/features/settings/components/team-modes/LeaderModeEditor.tsxfrontend/src/features/settings/components/team-modes/index.tsfrontend/src/types/api.tsfrontend/src/features/settings/components/TeamEdit.tsxfrontend/src/apis/mocks/bot.tsfrontend/src/features/settings/components/BotEdit.tsxfrontend/src/apis/shells.tsfrontend/src/apis/models.ts
**/frontend/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/frontend/**/*.{tsx,jsx}: Use Tailwind CSS utility classes with the color system CSS variables for consistent styling across the calm UI philosophy
Frontend mobile-first responsive design approach should be used with Tailwind CSS breakpoints (sm, md, lg, etc.)
Files:
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsxfrontend/src/features/settings/components/BotList.tsxfrontend/src/features/settings/components/team-modes/BotTransfer.tsxfrontend/src/features/settings/components/team-modes/LeaderModeEditor.tsxfrontend/src/features/settings/components/TeamEdit.tsxfrontend/src/features/settings/components/BotEdit.tsx
**/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
React hooks from react-hook-form and zod validation must be used for form implementation in TypeScript components
Files:
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsxfrontend/src/features/settings/services/bots.tsfrontend/src/apis/bots.tsfrontend/src/features/settings/components/BotList.tsxfrontend/src/features/settings/components/team-modes/BotTransfer.tsxfrontend/src/features/settings/components/team-modes/LeaderModeEditor.tsxfrontend/src/features/settings/components/team-modes/index.tsfrontend/src/types/api.tsfrontend/src/features/settings/components/TeamEdit.tsxfrontend/src/apis/mocks/bot.tsfrontend/src/features/settings/components/BotEdit.tsxfrontend/src/apis/shells.tsfrontend/src/apis/models.ts
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant, formatted with Black (line length: 88), and use isort for imports
Python code must use type hints
Python functions should have descriptive names, include docstrings for public functions/classes, extract magic numbers to constants, and keep functions to a maximum of 50 lines preferred
Files:
backend/app/services/adapters/public_shell.pybackend/app/services/adapters/public_model.pyexecutor/agents/base.pybackend/app/api/endpoints/adapter/models.pybackend/app/services/adapters/task_kinds.pyexecutor/tasks/task_processor.pyexecutor/callback/callback_handler.pybackend/app/schemas/kind.pybackend/app/services/adapters/team_kinds.pyexecutor/services/agent_service.pybackend/app/schemas/bot.pyexecutor/callback/callback_client.pybackend/app/services/adapters/bot_kinds.pyexecutor_manager/executors/docker/binary_extractor.pybackend/app/services/model_aggregation_service.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/routers/routers.pybackend/app/services/adapters/shell_utils.pybackend/app/api/endpoints/adapter/shells.pybackend/app/services/adapters/executor_kinds.py
🧬 Code graph analysis (19)
frontend/src/features/settings/components/team-modes/SoloModeEditor.tsx (1)
frontend/src/components/ui/tooltip.tsx (2)
TooltipTrigger(34-34)TooltipContent(34-34)
backend/app/services/adapters/public_shell.py (2)
backend/app/schemas/kind.py (1)
Shell(129-136)frontend/src/types/api.ts (1)
Shell(76-85)
frontend/src/features/settings/components/team-modes/index.ts (5)
backend/app/api/endpoints/adapter/shells.py (1)
UnifiedShell(32-44)frontend/src/apis/shells.ts (1)
UnifiedShell(10-19)frontend/src/features/settings/components/BotEdit.tsx (1)
AgentType(35-35)frontend/src/types/api.ts (1)
Bot(29-41)frontend/src/features/settings/components/team-modes/types.ts (1)
TeamMode(7-7)
backend/app/services/adapters/public_model.py (1)
backend/app/models/public_shell.py (1)
PublicShell(19-38)
frontend/src/features/settings/components/TeamEdit.tsx (3)
frontend/src/apis/shells.ts (2)
UnifiedShell(10-19)shellApis(82-163)frontend/src/features/settings/components/team-modes/index.ts (1)
getFilteredBotsForMode(68-93)frontend/src/types/api.ts (1)
Bot(29-41)
backend/app/api/endpoints/adapter/models.py (4)
backend/app/services/base.py (1)
get(30-34)backend/app/models/user.py (1)
User(14-33)backend/app/core/security.py (1)
get_current_user(28-50)backend/app/api/dependencies.py (1)
get_db(12-21)
executor/tasks/task_processor.py (1)
executor/callback/callback_handler.py (2)
send_task_started_callback(83-119)send_task_failed_callback(165-203)
executor/callback/callback_handler.py (1)
executor/callback/callback_client.py (1)
send_callback(89-154)
backend/app/services/adapters/team_kinds.py (2)
backend/app/schemas/kind.py (1)
Shell(129-136)frontend/src/types/api.ts (1)
Shell(76-85)
executor/services/agent_service.py (2)
executor/tests/agents/test_base.py (1)
agent(34-36)executor/agents/factory.py (1)
AgentFactory(21-89)
backend/app/services/adapters/bot_kinds.py (6)
backend/app/services/adapters/shell_utils.py (4)
get_shell_by_name(25-74)get_shell_info_by_name(77-166)get_shell_type(169-190)get_shells_by_names_batch(231-316)backend/app/services/adapters/team_kinds.py (1)
create_with_user(65-158)backend/app/schemas/bot.py (1)
BotCreate(13-22)backend/app/schemas/kind.py (3)
Bot(176-183)Ghost(47-54)Shell(129-136)frontend/src/types/api.ts (2)
Bot(29-41)Shell(76-85)backend/app/models/kind.py (1)
Kind(25-42)
frontend/src/apis/models.ts (1)
frontend/src/apis/client.ts (1)
apiClient(105-105)
executor_manager/executors/docker/binary_extractor.py (1)
shared/logger.py (1)
setup_logger(37-107)
backend/app/services/model_aggregation_service.py (1)
backend/app/models/public_shell.py (1)
PublicShell(19-38)
executor_manager/executors/docker/executor.py (1)
executor_manager/executors/docker/binary_extractor.py (1)
extract_executor_binary(40-81)
executor_manager/routers/routers.py (1)
backend/app/api/endpoints/adapter/shells.py (2)
ImageCheckResult(71-77)validate_image(467-637)
backend/app/services/adapters/shell_utils.py (3)
backend/app/models/kind.py (1)
Kind(25-42)backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/schemas/kind.py (1)
Shell(129-136)
backend/app/api/endpoints/adapter/shells.py (4)
backend/app/models/kind.py (1)
Kind(25-42)backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/schemas/kind.py (1)
Shell(129-136)backend/app/core/security.py (1)
get_current_user(28-50)
backend/app/services/adapters/executor_kinds.py (3)
backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/schemas/kind.py (1)
Shell(129-136)frontend/src/types/api.ts (1)
Shell(76-85)
🪛 Ruff (0.14.6)
backend/app/api/endpoints/adapter/models.py
358-358: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
359-359: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/services/adapters/bot_kinds.py
352-352: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
560-560: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
839-839: Unpacked variable shell is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
839-839: Unpacked variable model is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
executor_manager/executors/docker/binary_extractor.py
1-1: Shebang is present but file is not executable
(EXE001)
79-79: Do not catch blind exception: Exception
(BLE001)
80-80: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
95-95: subprocess call: check for execution of untrusted input
(S603)
96-96: Starting a process with a partial executable path
(S607)
103-103: Consider moving this statement to an else block
(TRY300)
104-104: Do not catch blind exception: Exception
(BLE001)
130-130: subprocess call: check for execution of untrusted input
(S603)
131-136: Starting a process with a partial executable path
(S607)
159-159: Do not catch blind exception: Exception
(BLE001)
187-187: subprocess call: check for execution of untrusted input
(S603)
188-188: Starting a process with a partial executable path
(S607)
243-243: subprocess call: check for execution of untrusted input
(S603)
244-249: Starting a process with a partial executable path
(S607)
262-262: Consider moving this statement to an else block
(TRY300)
265-265: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
267-267: Do not catch blind exception: Exception
(BLE001)
268-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
executor_manager/executors/docker/executor.py
320-321: try-except-pass detected, consider logging the exception
(S110)
320-320: Do not catch blind exception: Exception
(BLE001)
324-324: Abstract raise to an inner function
(TRY301)
324-324: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Do not catch blind exception: Exception
(BLE001)
385-385: Do not catch blind exception: Exception
(BLE001)
785-785: Do not catch blind exception: Exception
(BLE001)
786-786: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
executor_manager/routers/routers.py
181-181: Do not catch blind exception: Exception
(BLE001)
182-182: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
345-349: Consider moving this statement to an else block
(TRY300)
347-347: f-string without any placeholders
Remove extraneous f prefix
(F541)
351-351: Do not catch blind exception: Exception
(BLE001)
352-352: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
355-355: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/services/adapters/shell_utils.py
49-49: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
64-64: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
111-111: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
141-141: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
302-302: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
backend/app/api/endpoints/adapter/shells.py
152-152: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
153-153: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
186-186: Do not catch blind exception: Exception
(BLE001)
204-204: Do not catch blind exception: Exception
(BLE001)
216-216: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
217-217: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
263-263: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
264-264: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
374-374: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
375-375: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
435-435: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
436-436: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
469-469: Unused function argument: current_user
(ARG001)
469-469: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
528-528: Do not catch blind exception: Exception
(BLE001)
529-529: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
588-588: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
605-605: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
612-612: Use explicit conversion flag
Replace with conversion flag
(RUF010)
616-616: Use explicit conversion flag
Replace with conversion flag
(RUF010)
619-619: Use explicit conversion flag
Replace with conversion flag
(RUF010)
621-621: Do not catch blind exception: Exception
(BLE001)
622-622: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
629-629: Use explicit conversion flag
Replace with conversion flag
(RUF010)
633-633: Use explicit conversion flag
Replace with conversion flag
(RUF010)
687-687: Consider moving this statement to an else block
(TRY300)
688-688: Do not catch blind exception: Exception
(BLE001)
689-689: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
698-698: Unused function argument: current_user
(ARG001)
698-698: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
710-713: Abstract raise to an inner function
(TRY301)
727-727: Do not catch blind exception: Exception
(BLE001)
728-728: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
729-731: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
730-730: Use explicit conversion flag
Replace with conversion flag
(RUF010)
758-760: Abstract raise to an inner function
(TRY301)
762-762: Consider moving this statement to an else block
(TRY300)
765-765: Do not catch blind exception: Exception
(BLE001)
766-766: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
767-769: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
768-768: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/services/adapters/executor_kinds.py
420-420: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
| try: | ||
| shell_info = get_shell_info_by_name(db, obj_in.shell_name, user_id) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
Preserve exception context with raise ... from.
When re-raising as HTTPException, preserve the original exception context for better debugging:
try:
shell_info = get_shell_info_by_name(db, obj_in.shell_name, user_id)
except ValueError as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from e🧰 Tools
🪛 Ruff (0.14.6)
352-352: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/services/adapters/bot_kinds.py around lines 349 to 352, the
ValueError caught when calling get_shell_info_by_name is re-raised as an
HTTPException but loses the original exception context; modify the exception
re-raise to chain the original exception by using "raise
HTTPException(status_code=400, detail=str(e)) from e" so the original traceback
is preserved for debugging and logging.
| try: | ||
| shell_info = get_shell_info_by_name(db, new_shell_name, user_id) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
Same exception chaining issue in update flow.
Apply the same fix here:
try:
shell_info = get_shell_info_by_name(db, new_shell_name, user_id)
except ValueError as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from e📝 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.
| try: | |
| shell_info = get_shell_info_by_name(db, new_shell_name, user_id) | |
| except ValueError as e: | |
| raise HTTPException(status_code=400, detail=str(e)) | |
| try: | |
| shell_info = get_shell_info_by_name(db, new_shell_name, user_id) | |
| except ValueError as e: | |
| raise HTTPException(status_code=400, detail=str(e)) from e |
🧰 Tools
🪛 Ruff (0.14.6)
560-560: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/services/adapters/bot_kinds.py around lines 557 to 560, the
except block catches ValueError and raises an HTTPException but loses the
original exception context; update the raise to chain the original exception
(use "raise HTTPException(status_code=400, detail=str(e)) from e") so the
original traceback/context is preserved when converting ValueError to
HTTPException.
| // Find the selected shell to get its shellType for model filtering | ||
| const selectedShell = shells.find(s => s.name === agentName); | ||
| // Use shell's shellType for model filtering, fallback to agentName for public shells | ||
| const shellType = selectedShell?.shellType || agentName; | ||
|
|
||
| // Use the new unified models API which includes type information | ||
| const response = await modelApis.getUnifiedModels(agentName); | ||
| const response = await modelApis.getUnifiedModels(shellType); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "shellType" frontend/src/apis/shells.ts -A 3 -B 3Repository: wecode-ai/Wegent
Length of output: 1067
🏁 Script executed:
rg -n "interface UnifiedShell\|type UnifiedShell" frontend/ -A 10Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "baseShellRef" frontend/src/apis/shells.ts -A 5 -B 5Repository: wecode-ai/Wegent
Length of output: 824
🏁 Script executed:
rg -n "getUnifiedModels" frontend/src/apis/ -A 10Repository: wecode-ai/Wegent
Length of output: 1593
🏁 Script executed:
rg -n "async.*getUnifiedShell\|getAllShells" frontend/src/apis/shells.ts -A 15Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "shells\s*=" frontend/src/features/settings/components/BotEdit.tsx -B 5 -A 5Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "useQuery\|useEffect" frontend/src/features/settings/components/BotEdit.tsx -A 5 | head -50Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "const.*shells\|getAllShells\|useShells" frontend/src/features/settings/components/BotEdit.tsx -B 3 -A 3Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
cat frontend/src/features/settings/components/BotEdit.tsx | head -100Repository: wecode-ai/Wegent
Length of output: 3782
🏁 Script executed:
rg -n "setShells" frontend/src/features/settings/components/BotEdit.tsx -B 5 -A 5Repository: wecode-ai/Wegent
Length of output: 1050
🏁 Script executed:
sed -n '320,345p' frontend/src/features/settings/components/BotEdit.tsxRepository: wecode-ai/Wegent
Length of output: 1328
🏁 Script executed:
sed -n '250,280p' frontend/src/features/settings/components/BotEdit.tsxRepository: wecode-ai/Wegent
Length of output: 1254
🏁 Script executed:
rg -n "shellApis\." frontend/src/features/settings/components/BotEdit.tsx -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 317
🏁 Script executed:
sed -n '100,270p' frontend/src/features/settings/components/BotEdit.tsx | grep -A 30 "fetchShells\|useEffect"Repository: wecode-ai/Wegent
Length of output: 548
🏁 Script executed:
rg -n "getUnifiedShells" frontend/src/apis/shells.ts -A 10Repository: wecode-ai/Wegent
Length of output: 988
🏁 Script executed:
rg -n "getAllShells\|getUnifiedShells" frontend/ -A 15 | head -80Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "baseShellRef" frontend/src/ -B 3 -A 3Repository: wecode-ai/Wegent
Length of output: 6116
🏁 Script executed:
rg -n "shellType.*baseShellRef\|baseShellRef.*shellType" frontend/src/Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
fd -e ts -e tsx frontend/src/apis/shells.ts | xargs cat | grep -A 50 "UnifiedShellListResponse"Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "createShell\|updateShell" frontend/src/apis/shells.ts -A 15Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "shellType" frontend/src/features/settings/components/ShellEdit.tsxRepository: wecode-ai/Wegent
Length of output: 204
🏁 Script executed:
rg -n "test.*shell\|Shell.*test" frontend/ -l | head -5Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
sed -n '265,275p' frontend/src/features/settings/components/BotEdit.tsxRepository: wecode-ai/Wegent
Length of output: 529
🏁 Script executed:
rg -n "allowedAgents\.includes\|shell\.shellType as AgentType" frontend/src/features/settings/components/BotEdit.tsx -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 42
Ensure user-defined shells have shellType populated from baseShellRef at data fetch time.
The model fetching logic correctly extracts shellType from the selected shell to filter compatible models, with a fallback to agentName. However, the shell filtering logic at line 269 directly uses shell.shellType in the allowedAgents filter without null checks. If user-defined shells don't have shellType populated by the backend when fetched, they may be silently filtered out. Verify that the backend properly derives and returns shellType from baseShellRef for all user-defined shells when calling shellApis.getUnifiedShells().
🤖 Prompt for AI Agents
In frontend/src/features/settings/components/BotEdit.tsx around lines 329–335
the code assumes every shell has shellType populated but backend may not set it
for user-defined shells; update the backend endpoint used by
shellApis.getUnifiedShells() so that when shells are assembled/returned you
populate shell.shellType from shell.baseShellRef (or the referenced base shell's
type) whenever shell.shellType is null/undefined; ensure the mapping happens at
fetch time, update the response type accordingly, and add a small
unit/integration test to confirm user-defined shells always include shellType.
…compatibility Add automatic data migration to convert legacy shell data from old format (using 'runtime' field) to new format (using 'shellType' field). Background: - PR #222 renamed Shell spec field from 'runtime' to 'shellType' - Existing databases with old data cannot display shells correctly - Agent startup fails because shell data cannot be properly parsed Solution: - Add migrate_legacy_shell_data() function in yaml_init.py - Migration runs automatically on application startup - Migrates both public_shells and user shells in kinds table - Migration is idempotent - only updates records using old format - Uses flag_modified() to ensure JSON field changes are detected The migration: 1. Finds all shells with 'runtime' field but no 'shellType' 2. Copies runtime value to shellType 3. Removes old runtime field for clean data 4. Commits changes with proper error handling
Summary
baseImageandbaseShellReffields to ShellSpec schema for custom shell configuration/api/shells/unified, CRUD operations, image validation)Key Changes
Backend
ShellSpecschema withbaseImageandbaseShellReffields/api/shells/unifiedendpoint for unified shell listing (public + user-defined)/api/shells/validate-imageendpoint for Base Image compatibility validation (async)base_imagein bot configuration for Executor ManagerExecutor
ImageValidatorAgentfor running validation checks inside the target containerreport_progressto send results via callbackcallback_handler.pyto supportresultfield in completed callbacksExecutor Manager
binary_extractor.pyfor extracting executor binary from official image to Named Volume-v wegent-executor-binary:/app:ro)/executor-manager/images/validateto submit validation tasks via task processor (async)Frontend
shells.ts) with full CRUD and validation supportArchitecture: Async Image Validation
The image validation now uses the actual executor run flow:
/api/shells/validate-imageagent_name: "ImageValidator"and targetbase_imageImageValidatorAgentruns validation checks inside the containerresultfieldThis approach:
Test plan
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.