Skip to content

Conversation

@qdaxb
Copy link
Contributor

@qdaxb qdaxb commented Nov 29, 2025

Summary

  • Add baseImage and baseShellRef fields to ShellSpec schema for custom shell configuration
  • Create Shell unified API endpoints (/api/shells/unified, CRUD operations, image validation)
  • Implement Init Container pattern: extract executor binary to Named Volume on Executor Manager startup
  • Support custom Base Image mode in Docker executor with executor binary mount
  • Add frontend Shell management UI with validation support and i18n
  • Refactor: Use executor flow for async image validation (extensible for Docker/K8s)

Key Changes

Backend

  • Extended ShellSpec schema with baseImage and baseShellRef fields
  • Created /api/shells/unified endpoint for unified shell listing (public + user-defined)
  • Added /api/shells/validate-image endpoint for Base Image compatibility validation (async)
  • Modified task dispatch to include base_image in bot configuration for Executor Manager

Executor

  • Created ImageValidatorAgent for running validation checks inside the target container
  • Agent registered in factory.py and uses report_progress to send results via callback
  • Extended callback_handler.py to support result field in completed callbacks

Executor Manager

  • Added binary_extractor.py for extracting executor binary from official image to Named Volume
  • Executor Manager extracts binary on startup and records version for incremental updates
  • Docker executor supports custom Base Image mode with executor binary mount (-v wegent-executor-binary:/app:ro)
  • Refactored /executor-manager/images/validate to submit validation tasks via task processor (async)

Frontend

  • Created Shell API client (shells.ts) with full CRUD and validation support
  • Added ShellList and ShellEdit components for shell management
  • Updated validation UI to handle async validation status (submitted, in_progress, success, error)
  • Integrated Shells tab into Settings page with i18n support (en/zh-CN)

Architecture: Async Image Validation

The image validation now uses the actual executor run flow:

  1. Frontend calls /api/shells/validate-image
  2. Backend proxies to Executor Manager
  3. Executor Manager creates a validation task with agent_name: "ImageValidator" and target base_image
  4. Docker executor starts container with custom base_image and mounts executor binary
  5. ImageValidatorAgent runs validation checks inside the container
  6. Results are reported back via callback mechanism with result field

This approach:

  • Works for both Docker and Kubernetes deployment modes
  • Validates inside the actual target image
  • Uses existing task/callback infrastructure
  • Returns async results (extensible for real-time status updates)

Test plan

  • Verify public shells display correctly in Settings > Shells tab
  • Create a custom shell with a valid base image
  • Test image validation for ClaudeCode and Agno shell types
  • Verify validation task submission and async response
  • Execute a task with custom shell and verify executor binary mount
  • Test shell update and delete operations

- 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.
@coderabbitai
Copy link

coderabbitai bot commented Nov 29, 2025

Walkthrough

Adds a new shell management feature spanning backend API endpoints, executor integration, and frontend UI. Introduces unified shell representations combining public and user-defined shells, image validation against shell dependencies, Docker binary extraction to a named volume, and custom base image support in task execution.

Changes

Cohort / File(s) Summary
Backend API Router Registration
backend/app/api/api.py
Registers the new shells router at /shells endpoint
Shell Management Endpoints & Models
backend/app/api/endpoints/adapter/shells.py
New FastAPI router with full CRUD endpoints for unified shells (public and user-defined), shell request/response models, conversion helpers, and image validation orchestration; integrates with user authentication and Kind object persistence
Schema Extensions
backend/app/schemas/kind.py
Adds baseImage and baseShellRef optional fields to ShellSpec for custom base image and public shell reference support
Service Logic Updates
backend/app/services/adapters/executor_kinds.py
Extends shell resolution to include public shell fallback, extracts baseImage from shell CRD, and propagates base_image to executor response and bot prompt building
Public Shell Data
backend/init_data/02-public-shells.yaml
Adds baseImage field to ClaudeCode and Agno public shell definitions
Executor Binary Management
executor_manager/executors/docker/binary_extractor.py
New module implementing extraction of executor binary from official image into a Docker named volume with version tracking and mount configuration
Executor Docker Integration
executor_manager/executors/docker/executor.py
Extends task execution to extract and apply custom base_image, mounting executor binary volume and configuring container entrypoint when base_image is provided
Executor Startup
executor_manager/main.py
Adds startup logic in lifespan to extract executor binary into named volume with error handling
Image Validation Service
executor_manager/routers/routers.py
New endpoint /executor-manager/images/validate with image validation models and logic; runs per-check dependency validation (node, python, sqlite, etc.) in containers with version comparison and structured result aggregation
Frontend API Client
frontend/src/apis/shells.ts
New API module defining shell types and ShellApis service with methods for listing, fetching, creating, updating, deleting shells and validating images
Settings Page
frontend/src/app/settings/page.tsx
Adds new "shells" tab to settings page with ShellList component, URL-driven tab routing, and responsive layout
Shell Edit Component
frontend/src/features/settings/components/ShellEdit.tsx
New React form component for creating/editing user shells with name, display name, base shell reference selection, base image input, and integrated image validation with detailed check results
Shell List Component
frontend/src/features/settings/components/ShellList.tsx
New React component displaying unified shells with filtering, CRUD actions (edit/delete for non-public shells), confirmation dialogs, loading/empty states, and shell metadata display
Internationalization
frontend/src/i18n/locales/en/common.json
frontend/src/i18n/locales/zh-CN/common.json
Adds English and Chinese translations for shell management UI strings, labels, actions, validations, and error messages

Sequence Diagram

sequenceDiagram
    participant User as User
    participant Frontend as Frontend UI
    participant Backend as Backend API<br/>/shells
    participant Executor as Executor Manager<br/>/images/validate
    participant Docker as Docker
    
    User->>Frontend: Click "Validate Image"
    Frontend->>Frontend: Select shell type & enter image
    Frontend->>Backend: POST /shells/validate-image<br/>(image, shellType)
    
    Backend->>Executor: POST /executor-manager/images/validate<br/>(image, shell_type)
    
    Executor->>Docker: Pull image
    Docker-->>Executor: Image pulled
    
    Executor->>Docker: Run container with dependency checks<br/>(node, python, sqlite, etc.)
    Docker-->>Executor: Check output (versions)
    
    Executor->>Executor: Parse versions & compare<br/>against minimums
    Executor->>Executor: Aggregate results
    
    Executor-->>Backend: ValidateImageResponse<br/>(valid, checks[], errors[])
    Backend-->>Frontend: ImageValidationResponse
    Frontend->>Frontend: Display per-check results<br/>(name, version, status)
    Frontend-->>User: Show validation results
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Image validation logic (executor_manager/routers/routers.py): Regex parsing, version comparison using packaging library, multi-check orchestration with timeouts and error handling
  • Binary extraction module (executor_manager/executors/docker/binary_extractor.py): Docker API interaction, volume management, version tracking, extraction workflow
  • Custom base image propagation (backend/app/services/adapters/executor_kinds.py, executor_manager/executors/docker/executor.py): Multi-point data flow from schema through services to executor with fallback logic
  • Frontend form validation (frontend/src/features/settings/components/ShellEdit.tsx): Image validation integration, async state management, error presentation
  • Heterogeneous file changes: Schema extensions, service logic, API endpoints, Docker integration, frontend components, translations—requires separate reasoning per area

Possibly related PRs

Suggested reviewers

  • feifei325

🐇 Shells are set and image checks aligned,
From public pools to custom designs,
Binary volumes packed with care,
Executors dance through digital air!
A feature complete, both swift and fair!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding support for custom base image configuration in bot shells.
Docstring Coverage ✅ Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wegent/feature-custom-shell-base-image

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- 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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (11)
executor_manager/executors/docker/binary_extractor.py (1)

72-74: Use logger.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 False

Apply similar changes at lines 169 and 172.

executor_manager/routers/routers.py (3)

189-194: Add proper type hints for list fields.

The checks and errors fields 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=True with items in errors list.

Returning valid=True while populating the errors list sends mixed signals. Consider using a different field like info or notes for 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 info field to the response model for non-error messages.


342-343: Silent exception swallowing hides version comparison failures.

The pass statement 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 exists
backend/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 baseImage to 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 v
frontend/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-form and zod for 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 deleting state similar to the saving state 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.

getPublicShells and getLocalEngineShells fetch 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 Exception catch is used when parsing shells, which could mask unexpected errors. Consider catching ValidationError from Pydantic specifically, since ShellCRD.model_validate is 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 returns valid=True with an error message, which is semantically confusing.

When shellType == "Dify", the response has valid=True but includes an error message. Consider using an empty errors list or a checks entry 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: Use logger.exception instead of logger.error for exception logging.

When logging exceptions in catch blocks, logger.exception automatically 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

📥 Commits

Reviewing files that changed from the base of the PR and between 502a024 and f6d85b0.

📒 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.tsx
  • backend/app/schemas/kind.py
  • frontend/src/features/settings/components/ShellList.tsx
  • backend/app/api/api.py
  • frontend/src/features/settings/components/ShellEdit.tsx
  • backend/app/services/adapters/executor_kinds.py
  • frontend/src/apis/shells.ts
  • executor_manager/main.py
  • executor_manager/executors/docker/executor.py
  • executor_manager/executors/docker/binary_extractor.py
  • backend/app/api/endpoints/adapter/shells.py
  • executor_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.tsx
  • frontend/src/features/settings/components/ShellList.tsx
  • frontend/src/features/settings/components/ShellEdit.tsx
  • frontend/src/apis/shells.ts
**/frontend/**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use const over let, never var in TypeScript/JavaScript

Files:

  • frontend/src/app/settings/page.tsx
  • frontend/src/features/settings/components/ShellList.tsx
  • frontend/src/features/settings/components/ShellEdit.tsx
  • frontend/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.tsx
  • frontend/src/features/settings/components/ShellList.tsx
  • 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/app/settings/page.tsx
  • frontend/src/features/settings/components/ShellList.tsx
  • frontend/src/features/settings/components/ShellEdit.tsx
  • frontend/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.py
  • backend/app/api/api.py
  • backend/app/services/adapters/executor_kinds.py
  • executor_manager/main.py
  • executor_manager/executors/docker/executor.py
  • executor_manager/executors/docker/binary_extractor.py
  • backend/app/api/endpoints/adapter/shells.py
  • executor_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 shells translation 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 models translations 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_task endpoint properly handles the HTTPException re-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 shells namespace follows the established patterns from models and 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 baseImage correctly takes precedence over the public shell's base image when present.


611-623: LGTM! Base image correctly propagated to executor.

The base_image field 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]) // Dependencies

useEffect(() => {
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_image with 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_image is 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_image parameter.

frontend/src/apis/shells.ts (2)

10-19: LGTM!

The UnifiedShell interface correctly mirrors the backend UnifiedShell Pydantic model with matching field names and types, including proper nullability handling with | null.


73-82: LGTM!

The getUnifiedShell method correctly handles the optional shellType query parameter with proper URL encoding for the path parameter.

Comment on lines +276 to +283
# 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",
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the actual code in the file
cd /tmp && git clone --depth 1 https://github.com/coderabbit-ai/coderabbit.git repo 2>/dev/null || cd repo

# Find and read the relevant file
if [ -f "backend/app/api/endpoints/adapter/shells.py" ]; then
  cat -n backend/app/api/endpoints/adapter/shells.py | sed -n '270,290p'
fi

Repository: wecode-ai/Wegent

Length of output: 114


🏁 Script executed:

# Search for tests related to baseImage validation
fd -type f -name "*.py" | xargs grep -l "baseImage" | head -10

Repository: wecode-ai/Wegent

Length of output: 230


🏁 Script executed:

# Search for frontend validation mentioned in the code
fd -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" \) | xargs grep -l "baseImage" 2>/dev/null | head -10

Repository: wecode-ai/Wegent

Length of output: 230


🏁 Script executed:

# List Python files to find the target
find . -name "shells.py" -path "*/adapter/*"

Repository: wecode-ai/Wegent

Length of output: 105


🏁 Script executed:

# Read the file with correct syntax
if [ -f "backend/app/api/endpoints/adapter/shells.py" ]; then
  wc -l backend/app/api/endpoints/adapter/shells.py
  echo "---"
  sed -n '270,290p' backend/app/api/endpoints/adapter/shells.py
fi

Repository: wecode-ai/Wegent

Length of output: 826


🌐 Web query:

Docker image reference format specification RFC OCI

💡 Result:

Short answer — canonical image-reference grammar and rules:

  • Docker/distribution reference grammar (common canonical form): reference := name [ ":" tag ] [ "@" digest ], where name = [domain '/'] path, tag = [\w][\w.-]{0,127}, digest = algo:hex (e.g., sha256:...), with rules for defaulting/normalizing hostnames and library namespace. (pkg.go.dev)

  • OCI / image-layout usage: OCI image layout/index annotations define a reference name (org.opencontainers.image.ref.name) and point to using fully-qualified image references (registry host + path + optional :tag or @digest); OCI image spec defines descriptors/digests and image/index formats. (godocs.io)

If you want the exact source text or links (Docker distribution reference package and the Open Containers image-spec/distribution spec), I can fetch and paste the precise grammar and examples.

Citations:


🏁 Script executed:

# Search for tests related to baseImage validation
find . -name "*.py" -path "*/test*" -o -name "test_*.py" | xargs grep -l "baseImage" 2>/dev/null | head -5

Repository: wecode-ai/Wegent

Length of output: 42


🏁 Script executed:

# Search for frontend validation code
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) 2>/dev/null | xargs grep -l "baseImage" 2>/dev/null | head -5

Repository: wecode-ai/Wegent

Length of output: 205


🏁 Script executed:

# Check if there are any comments about image format validation in the file
sed -n '200,320p' backend/app/api/endpoints/adapter/shells.py | grep -A5 -B5 "baseImage\|image.*format\|Expected format"

Repository: wecode-ai/Wegent

Length of output: 1073


🏁 Script executed:

python3 << 'EOF'
import re

regex_pattern = r"^[a-z0-9.-]+(/[a-z0-9._-]+)+:[a-z0-9._-]+$"
test_cases = [
    ("ubuntu:22.04", "Simple image without registry"),
    ("ghcr.io/org/image", "Registry without explicit tag"),
    ("ghcr.io/org/image:latest", "Valid with registry and tag"),
    ("localhost:5000/image:tag", "Localhost registry"),
    ("registry.example.com/image:tag", "Domain registry"),
    ("image@sha256:abc123", "Digest reference"),
    ("ubuntu@sha256:abc123", "Simple image with digest"),
    ("ghcr.io/org/image@sha256:abc123", "Registry with digest"),
    ("myimage:1.0", "Simple with tag"),
]

regex = re.compile(regex_pattern, re.IGNORECASE)

print("Testing regex pattern:")
print(f"Pattern: {regex_pattern}\n")

for image, description in test_cases:
    match = regex.match(image)
    result = "✓ MATCHES" if match else "✗ REJECTS"
    print(f"{result}: {image:40} ({description})")
EOF

Repository: wecode-ai/Wegent

Length of output: 823


🏁 Script executed:

# Check frontend validation
cat frontend/src/apis/shells.ts | head -100

Repository: wecode-ai/Wegent

Length of output: 2669


🏁 Script executed:

# Search for baseImage validation in frontend
grep -n "baseImage" frontend/src/features/settings/components/ShellEdit.tsx -A3 -B3

Repository: wecode-ai/Wegent

Length of output: 2559


Docker image regex validation is indeed too restrictive and will reject valid image formats.

The test results confirm the review comment. The regex ^[a-z0-9.-]+(/[a-z0-9._-]+)+:[a-z0-9._-]+$ rejects several valid Docker image formats:

  • ubuntu:22.04 — standard library image (rejected: requires /)
  • ghcr.io/org/image — valid without explicit tag (rejected: requires :)
  • localhost:5000/image:tag — localhost registry (rejected: :5000 contains digits not in character class)
  • image@sha256:abc123 — digest reference (rejected: pattern doesn't support @)

Per Docker/OCI image reference specifications, tags and digests are optional and the format should allow:

  • Library images without registry: ubuntu:22.04 or ubuntu (defaults to :latest)
  • Registry references without explicit tags: ghcr.io/org/image
  • Digest references: image@sha256:...
  • Localhost registries: localhost:5000/image:tag

The frontend (ShellEdit.tsx) performs no format validation—only an empty check—relying entirely on backend validation. Either broaden the regex or add more detailed format documentation explaining the imposed restrictions.

🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/shells.py around lines 276-283, the current
strict regex rejects many valid Docker/OCI image references (library images,
registry with port, optional tags, and digests); replace this brittle regex with
a proper Docker/OCI reference validator: either use a known parser from a
maintained library (for example the docker SDK/reference parsing utilities or an
OCI image reference parser) to validate request.baseImage, or implement a
relaxed validation that accepts optional registry (with optional :port),
optional namespace, image name, and optional tag or digest (@:sha256...). Update
the error message to reflect supported forms if you keep any validation and
ensure the frontend no longer relies on this restrictive pattern.

Comment on lines +375 to +406
@router.delete("/{shell_name}")
def delete_shell(
shell_name: str,
db: Session = Depends(get_db),
current_user: User = Depends(security.get_current_user),
):
"""
Delete a user-defined shell.
Only user-defined shells can be deleted. Public shells cannot be deleted.
"""
# Get user shell
shell = (
db.query(Kind)
.filter(
Kind.user_id == current_user.id,
Kind.kind == "Shell",
Kind.name == shell_name,
Kind.namespace == "default",
Kind.is_active == True, # noqa: E712
)
.first()
)
if not shell:
raise HTTPException(status_code=404, detail="User shell not found")

# Soft delete
shell.is_active = False
db.add(shell)
db.commit()

return {"message": "Shell deleted successfully"}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's explore the repository structure to understand the codebase organization
find . -type f -name "*.py" | grep -E "(model|task|bot)" | head -20

Repository: wecode-ai/Wegent

Length of output: 818


🏁 Script executed:

#!/bin/bash
# Examine the Kind model to understand shell relationships
cat -n backend/app/models/kind.py | head -100

Repository: wecode-ai/Wegent

Length of output: 1454


🏁 Script executed:

#!/bin/bash
# Search for shell_name references in tasks and bots endpoints
rg "shell_name" --type py -C 3 backend/app/api/endpoints/adapter/tasks.py backend/app/api/endpoints/adapter/bots.py

Repository: wecode-ai/Wegent

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Check task schema to see how shells might be referenced
cat -n backend/app/schemas/task.py | head -80

Repository: wecode-ai/Wegent

Length of output: 2881


🏁 Script executed:

#!/bin/bash
# Check bot schema for shell references
cat -n backend/app/schemas/bot.py | head -100

Repository: wecode-ai/Wegent

Length of output: 2266


🏁 Script executed:

#!/bin/bash
# Search for how shells are actually used in the codebase
rg "shell" --type py -i -C 2 backend/app/api/endpoints/adapter/bots.py backend/app/api/endpoints/adapter/tasks.py | head -80

Repository: wecode-ai/Wegent

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Examine the shells.py file to understand the full context
wc -l backend/app/api/endpoints/adapter/shells.py

Repository: wecode-ai/Wegent

Length of output: 107


🏁 Script executed:

#!/bin/bash
# Look at create and other shell endpoints to understand usage patterns
sed -n '1,150p' backend/app/api/endpoints/adapter/shells.py | cat -n

Repository: wecode-ai/Wegent

Length of output: 5346


🏁 Script executed:

#!/bin/bash
# Look at the delete endpoint and surrounding code
sed -n '370,420p' backend/app/api/endpoints/adapter/shells.py | cat -n

Repository: wecode-ai/Wegent

Length of output: 1863


🏁 Script executed:

#!/bin/bash
# Search for references to shells in Kind queries or other places
rg "baseShellRef|shell_name|Kind.*shell" --type py -C 3 backend/app/

Repository: wecode-ai/Wegent

Length of output: 23640


🏁 Script executed:

#!/bin/bash
# Let's check if there's any validation in bot creation for shell references
rg -A 15 "def create_bot|def _create_bot" --type py backend/app/services/adapters/bot_kinds.py | head -40

Repository: wecode-ai/Wegent

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Verify the validation logic in kind_impl.py for bot creation
sed -n '50,120p' backend/app/services/kind_impl.py | cat -n

Repository: wecode-ai/Wegent

Length of output: 3253


🏁 Script executed:

#!/bin/bash
# Get the full _validate_references method in BotKindService
sed -n '64,110p' backend/app/services/kind_impl.py | cat -n

Repository: wecode-ai/Wegent

Length of output: 1951


🏁 Script executed:

#!/bin/bash
# Get a larger section to see the full BotKindService validation
sed -n '58,150p' backend/app/services/kind_impl.py | cat -n

Repository: wecode-ai/Wegent

Length of output: 3989


Based on my verification through the codebase, I can now provide the rewritten review comment:

Add validation to prevent deletion of shells referenced by active bots.

Shells are directly referenced by bots through shellRef.name (confirmed in BotKindService._validate_references). When bots are created or executed, they require the referenced shell to have is_active == True. The current delete endpoint soft-deletes without checking if any active bots reference the shell, which will cause those bots to fail during load or execution with a NotFoundException. Add a check to either prevent deletion or warn the user when attempting to delete a shell in use.

For example, in backend/app/services/kind_impl.py at lines 82-93, bots query for shells with is_active == True. If a shell is soft-deleted while bots reference it, those bots become non-functional.

🧰 Tools
🪛 Ruff (0.14.6)

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)

Comment on lines +426 to +428
import os

import httpx
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 Session

Then 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.

Comment on lines +411 to +431
# 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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Refactor inline class and fix boolean comparison.

Several issues in this block:

  1. Static analysis (Ruff E712): Line 420 uses == True instead of direct boolean check.
  2. Inline class definition: MockShell defined inside the method is a code smell. It should be extracted to module level or use a simpler approach.
  3. 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)

Comment on lines +143 to +147
extract_cmd = f"""
cp -r /app/* /target/ 2>/dev/null || cp /app/executor /target/executor;
echo '{executor_image}' > {VERSION_FILE_PATH};
chmod +x /target/executor 2>/dev/null || true
"""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential command injection via shell command construction.

The executor_image value is embedded directly into a shell command string. While it comes from an environment variable (which is typically trusted), if the image name contains shell metacharacters (e.g., ; rm -rf /), it could execute unintended commands.

Consider escaping the image name or using a safer approach:

-        extract_cmd = f"""
-            cp -r /app/* /target/ 2>/dev/null || cp /app/executor /target/executor;
-            echo '{executor_image}' > {VERSION_FILE_PATH};
-            chmod +x /target/executor 2>/dev/null || true
-        """
+        import shlex
+        escaped_image = shlex.quote(executor_image)
+        extract_cmd = f"""
+            cp -r /app/* /target/ 2>/dev/null || cp /app/executor /target/executor;
+            echo {escaped_image} > {VERSION_FILE_PATH};
+            chmod +x /target/executor 2>/dev/null || true
+        """
🤖 Prompt for AI Agents
In executor_manager/executors/docker/binary_extractor.py around lines 143-147,
the shell heredoc builds a command that embeds executor_image directly, which
risks command injection if the image string contains shell metacharacters;
instead, avoid interpolating untrusted values into shell code—either 1) write
the executor_image to VERSION_FILE_PATH from Python (open the file and write the
value) before or after running the cp/chmod commands, or 2) if you must use a
shell command, safely escape the value with shlex.quote when constructing the
shell string or pass it as an environment variable/argument and use a safe shell
write (e.g., printf with a single %s format) so no characters in executor_image
are interpreted by the shell.

executor_image: Default executor image
base_image: Optional custom base image
"""
from executors.docker.binary_extractor import EXECUTOR_BINARY_VOLUME
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.py

Repository: 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 -A2

Repository: 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.py

Repository: 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.py

Repository: 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.py

Repository: 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_VOLUME

Then 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.

Comment on lines 276 to 288
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}"],
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the file and understand the broader context
find . -name "routers.py" -path "*/executor_manager/routers/*" | head -5

Repository: 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 py

Repository: 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 3

Repository: 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 2

Repository: wecode-ai/Wegent

Length of output: 5933


🏁 Script executed:

# Read the full validate_image function
sed -n '197,350p' executor_manager/routers/routers.py

Repository: wecode-ai/Wegent

Length of output: 5701


🏁 Script executed:

# Check the ValidateImageRequest class definition
sed -n '170,196p' executor_manager/routers/routers.py

Repository: 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 py

Repository: 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants