feat(backend): implement Skill management for Claude Code Skills #182
Conversation
Add comprehensive Skill management functionality to allow users to
upload, manage, and deploy Claude Code Skills ZIP packages.
**Database Layer:**
- Add `skill_binaries` table for storing ZIP packages
- Add Alembic migration (1a2b3c4d5e6f) with CASCADE delete
- Create `SkillBinary` model with binary data storage
**Schema Layer:**
- Add Skill CRD schemas (SkillSpec, SkillStatus, Skill, SkillList)
- Extend GhostSpec with `skills` field for skill references
- Update Bot schemas to support skills field
**Service Layer:**
- Implement `SkillValidator` for ZIP validation and SKILL.md parsing
- Create `SkillKindsService` with full CRUD operations
- Add skill validation in `BotKindsService` (create & update)
- Implement reference checking to prevent deleting used skills
**API Layer:**
- Add Skills API endpoints (upload, list, get, download, delete)
- Support filtering by name in list endpoint
- Stream ZIP downloads for Executor deployment
- Include database ID in Skill metadata labels
**Executor Integration:**
- Add `_download_and_deploy_skills` method to ClaudeCodeAgent
- Auto-download skills from Backend API on initialization
- Deploy skills to `~/.claude/skills/{skill_name}/` directory
- Clear skills directory before each deployment for consistency
- Implement Zip Slip protection and error handling
**Security:**
- SHA256 hash validation for integrity
- User isolation for all Skill operations
- ZIP path traversal prevention (Zip Slip protection)
- 10MB file size limit enforcement
**Key Features:**
- Users can upload Claude Code Skills as ZIP packages
- Ghosts can reference multiple Skills by name
- Executor automatically downloads and deploys Skills at runtime
- Referenced Skills cannot be deleted (referential integrity)
- Skills are validated for SKILL.md presence and YAML frontmatter
Note: Frontend UI implementation will be added in a follow-up commit.
|
Caution Review failedThe pull request is closed. WalkthroughAdds end-to-end Skill support: DB migration and ORM for skill binaries, ZIP validator and SkillKinds service, API endpoints and tests, frontend CRUD/UI/types and upload progress, bot/schema integration, executor download/deploy, docs, and docker env propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as "Skills API"
participant Validator as "SkillValidator"
participant Service as "SkillKindsService"
participant DB as "Database"
participant Binary as "SkillBinary"
User->>API: POST /kinds/skills/upload (ZIP)
API->>Validator: validate_zip(file_content)
Validator->>Validator: check ZIP, size, Zip‑Slip, parse SKILL.md, compute SHA‑256
Validator-->>API: metadata (description, version, tags, file_hash, file_size)
API->>Service: create_skill(name, namespace, file_content,...)
Service->>DB: insert Kind (type: Skill)
Service->>Binary: insert binary_data linked to kind_id
DB-->>Service: commit
Service-->>API: Skill CRD
API-->>User: 201 Created (Skill)
sequenceDiagram
participant BotCreator
participant API as "Bot API"
participant BotService as "BotKindsService"
participant SkillService as "SkillKindsService"
participant DB as "Database"
BotCreator->>API: POST /create bot (spec.skills = [...])
API->>BotService: create_with_user(spec)
BotService->>SkillService: _validate_skills(skill_names)
SkillService->>DB: query kinds where name IN skill_names AND owner = user
alt missing skills
SkillService-->>BotService: HTTP 400 (missing names)
BotService-->>API: 400 Bad Request
else all present
SkillService-->>BotService: OK
BotService->>DB: create Ghost kind with spec.skills populated
BotService-->>API: 201 Created (Bot with skills)
end
sequenceDiagram
participant Executor
participant Backend as "Backend API"
participant FS as "Filesystem (~/.claude/skills/)"
participant Extractor as "Zip Extractor"
Executor->>Backend: GET /kinds/skills?name=... (auth)
Backend-->>Executor: list of skills (names + ids)
loop each skill
Executor->>Backend: GET /kinds/skills/{id}/download (with task token)
Backend-->>Executor: stream ZIP
Executor->>FS: prepare ~/.claude/skills/{skill_name}/
Executor->>Extractor: extract ZIP (Zip‑Slip checks)
alt safe
Extractor-->>FS: write files
else unsafe
Extractor-->>Executor: log error and continue
end
end
Executor->>Executor: log summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (19)
executor/agents/claude_code/claude_code_agent.py (3)
1202-1205: Move imports to module level.Imports inside functions are generally discouraged per PEP 8. Consider moving
requests,zipfile,io, andshutilto the module-level imports for consistency and better performance (avoids repeated import overhead).import asyncio import os import json import importlib +import io import random +import requests +import shutil import string import subprocess import re import time +import zipfile from typing import Dict, Any, List, OptionalThen remove lines 1187 and 1202-1205.
1269-1271: Uselogger.exceptionfor proper stack trace logging.Per static analysis hints, use
logger.exceptioninstead oflogger.errorwhen logging exceptions to include the full stack trace.except Exception as e: - logger.error(f"Error in _download_and_deploy_skills: {str(e)}") + logger.exception("Error in _download_and_deploy_skills") # Don't raise - skills deployment failure shouldn't block task execution
1216-1219: Consider adding timeout configuration and retry logic.The HTTP requests use hardcoded timeouts (30s for list, 60s for download). For production resilience, consider making these configurable and adding basic retry logic for transient failures.
backend/app/models/skill_binary.py (2)
8-10: Reorder imports per PEP 8.Standard library imports (
datetime) should come before third-party imports (sqlalchemy). As per coding guidelines, use isort for import organization.-from sqlalchemy import Column, Integer, ForeignKey, LargeBinary, String, DateTime from datetime import datetime + +from sqlalchemy import Column, Integer, ForeignKey, LargeBinary, String, DateTime + from app.db.base import Base
17-22: Consider adding anupdated_atcolumn for consistency.The
Kindmodel includes bothcreated_atandupdated_atcolumns. For consistency across the codebase and to track binary updates, consider addingupdated_attoSkillBinary.file_hash = Column(String(64), nullable=False) # SHA256 hash created_at = Column(DateTime, default=datetime.now) + updated_at = Column(DateTime, default=datetime.now, onupdate=datetime.now)Note: This would require updating the Alembic migration as well.
backend/app/services/adapters/bot_kinds.py (2)
550-556: Avoid redundant parsing of Ghost JSON.
Ghost.model_validate(ghost.json)is called twice whenghostexists: once at line 539 and again at line 554. Reuse the already-parsedghost_crdvariable.if model and model.json: model_crd = Model.model_validate(model.json) agent_config = model_crd.spec.modelConfig # Extract skills from ghost skills = [] if ghost: - ghost_crd = Ghost.model_validate(ghost.json) - skills = ghost_crd.spec.skills or [] + # ghost_crd already parsed above when ghost exists + if ghost.json: + ghost_crd = Ghost.model_validate(ghost.json) + skills = ghost_crd.spec.skills or []Or better, restructure the ghost extraction block to parse once:
+ ghost_crd = None if ghost and ghost.json: ghost_crd = Ghost.model_validate(ghost.json) system_prompt = ghost_crd.spec.systemPrompt mcp_servers = ghost_crd.spec.mcpServers or {} ... # Extract skills from ghost skills = [] - if ghost: - ghost_crd = Ghost.model_validate(ghost.json) + if ghost_crd: skills = ghost_crd.spec.skills or []
587-593: Use SQLAlchemy boolean expression idiomatically.The Ruff hint (E712) flags
Kind.is_active == True. While this works with SQLAlchemy, the idiomatic form isKind.is_active.is_(True)or justKind.is_activefor truthy checks. However, since this pattern is used consistently throughout the file, this is a low-priority nitpick.existing_skills = db.query(Kind).filter( Kind.user_id == user_id, Kind.kind == "Skill", Kind.name.in_(skill_names), Kind.namespace == "default", - Kind.is_active == True + Kind.is_active.is_(True) ).all()backend/app/services/skill_service.py (5)
22-23: Remove unusedfile_nameparameter or document its intended use.The
file_nameparameter is accepted but never used. Either remove it or add a TODO if it's planned for future validation (e.g., extension checks).@staticmethod - def validate_zip(file_content: bytes, file_name: str) -> Dict[str, Any]: + def validate_zip(file_content: bytes) -> Dict[str, Any]:Note: This would require updating callers in
skill_kinds.pyaccordingly.
64-70: Zip Slip check may produce false positives.The check
'..' in file_info.filenamewill reject legitimate files likefile..txt. Consider using path normalization for a more precise check:import os for file_info in zip_file.filelist: # Normalize and check the path stays relative normalized = os.path.normpath(file_info.filename) if normalized.startswith('/') or normalized.startswith('..'): raise HTTPException( status_code=400, detail=f"Unsafe file path detected in ZIP: {file_info.filename}" )The current check is safe (conservative) but may reject valid archives.
98-109: Add exception chaining to preserve traceback context.Use
raise ... from errto chain exceptions, which helps with debugging by preserving the original traceback.except zipfile.BadZipFile: - raise HTTPException( + raise HTTPException( status_code=400, detail="Corrupted ZIP file" - ) + ) from None except Exception as e: if isinstance(e, HTTPException): raise raise HTTPException( status_code=400, - detail=f"Failed to process ZIP file: {str(e)}" - ) + detail=f"Failed to process ZIP file: {e!s}" + ) from e
156-167: Add exception chaining in YAML error handlers.Similar to the previous comment, chain exceptions for better debugging:
except yaml.YAMLError as e: raise HTTPException( status_code=400, - detail=f"Invalid YAML frontmatter in SKILL.md: {str(e)}" - ) + detail=f"Invalid YAML frontmatter in SKILL.md: {e!s}" + ) from e except Exception as e: if isinstance(e, HTTPException): raise raise HTTPException( status_code=400, - detail=f"Failed to parse SKILL.md frontmatter: {str(e)}" - ) + detail=f"Failed to parse SKILL.md frontmatter: {e!s}" + ) from e
76-78: Consider usingerrors='replace'instead of'ignore'.Using
errors='ignore'silently drops invalid UTF-8 bytes, which could corrupt metadata without warning. Using'replace'makes corruption visible:- skill_md_content = f.read().decode('utf-8', errors='ignore') + skill_md_content = f.read().decode('utf-8', errors='replace')backend/alembic/versions/1a2b3c4d5e6f_add_skill_binaries_table.py (2)
25-39: Consider using Alembic'sop.create_tablefor portability.Raw SQL reduces database portability. Alembic's operations provide cross-database compatibility:
def upgrade() -> None: op.create_table( 'skill_binaries', sa.Column('id', sa.Integer(), autoincrement=True, nullable=False), sa.Column('kind_id', sa.Integer(), nullable=False), sa.Column('binary_data', sa.LargeBinary(), nullable=False), sa.Column('file_size', sa.Integer(), nullable=False), sa.Column('file_hash', sa.String(64), nullable=False), sa.Column('created_at', sa.DateTime(), server_default=sa.func.now()), sa.PrimaryKeyConstraint('id'), sa.UniqueConstraint('kind_id'), sa.ForeignKeyConstraint(['kind_id'], ['kinds.id'], ondelete='CASCADE'), )Also note: the index
ix_skill_binaries_idon line 35 is redundant sinceidis already the primary key and thus auto-indexed.
32-32: Consider addingupdated_atcolumn for audit purposes.Since skills can be updated with new ZIP packages, tracking when the binary was last modified would be useful for debugging and audit:
updated_at DATETIME DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,backend/app/api/endpoints/kind/skills.py (1)
71-71: Use explicitOptional[str]type hint for clarity.While
Query(None, ...)implies optionality, an explicit type hint improves code readability and IDE support:+from typing import Optional + - name: str = Query(None, description="Filter by skill name"), + name: Optional[str] = Query(None, description="Filter by skill name"),backend/app/services/adapters/skill_kinds.py (4)
11-11: Remove unused import.The
and_import from SQLAlchemy is not used in this file.-from sqlalchemy import and_
158-163: Unusedtotalvariable - either remove or expose it.The
totalcount is computed but never used. Either remove it or include it in the response for pagination support:Option 1 - Remove if not needed:
- total = query.count() skills = query.offset(skip).limit(limit).all()Option 2 - Expose for pagination (requires schema update):
return SkillList( items=[self._kind_to_skill(skill) for skill in skills], total=total # Add to SkillList schema )
266-283: Ghost reference check loads all Ghosts into memory.This approach could be slow for users with many Ghosts. Consider optimizing with a database-level JSON query if your MySQL version supports it:
# For MySQL 5.7+ with JSON support: referenced_ghost = db.query(Kind).filter( Kind.user_id == user_id, Kind.kind == "Ghost", Kind.is_active == True, func.json_contains(Kind.json, f'"{skill_name}"', '$.spec.skills') ).first() if referenced_ghost: raise HTTPException( status_code=400, detail=f"Cannot delete Skill '{skill_name}' because it is referenced by Ghost: {referenced_ghost.name}" )The current approach is acceptable for small-to-medium datasets.
322-335: Add defensive access for JSON fields.If
kind.json["spec"]is missing or malformed, this will raise aKeyError. Consider defensive access:def _kind_to_skill(self, kind: Kind) -> Skill: """Convert Kind model to Skill CRD""" metadata = ObjectMeta( name=kind.name, namespace=kind.namespace, labels={"id": str(kind.id)} # Store database ID in labels ) + spec_data = kind.json.get("spec", {}) + status_data = kind.json.get("status", {}) return Skill( apiVersion=kind.json.get("apiVersion", "agent.wecode.io/v1"), kind="Skill", metadata=metadata, - spec=SkillSpec(**kind.json["spec"]), - status=SkillStatus(**kind.json.get("status", {})) + spec=SkillSpec(**spec_data) if spec_data else SkillSpec(description=""), + status=SkillStatus(**status_data) if status_data else None )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/alembic/versions/1a2b3c4d5e6f_add_skill_binaries_table.py(1 hunks)backend/app/api/endpoints/kind/__init__.py(1 hunks)backend/app/api/endpoints/kind/skills.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/skill_binary.py(1 hunks)backend/app/schemas/bot.py(4 hunks)backend/app/schemas/kind.py(2 hunks)backend/app/services/adapters/bot_kinds.py(5 hunks)backend/app/services/adapters/skill_kinds.py(1 hunks)backend/app/services/skill_service.py(1 hunks)executor/agents/claude_code/claude_code_agent.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant with Black formatter (line length: 88)
Use isort for import organization in Python
Python functions and variables must have descriptive names
Docstrings required for all public Python functions and classes
Extract magic numbers to named constants in Python
Python functions must not exceed 50 lines
Type hints required for all Python function signatures
Run pylint and flake8 for Python code quality checks
Files:
backend/app/api/endpoints/kind/__init__.pybackend/app/models/__init__.pybackend/app/services/adapters/bot_kinds.pybackend/app/schemas/bot.pybackend/app/services/adapters/skill_kinds.pybackend/app/schemas/kind.pybackend/alembic/versions/1a2b3c4d5e6f_add_skill_binaries_table.pyexecutor/agents/claude_code/claude_code_agent.pybackend/app/services/skill_service.pybackend/app/models/skill_binary.pybackend/app/api/endpoints/kind/skills.py
🧠 Learnings (1)
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Use Kubernetes-inspired CRD design (Ghost, Model, Shell, Bot, Team, Task)
Applied to files:
backend/app/schemas/kind.py
🧬 Code graph analysis (3)
backend/app/models/__init__.py (1)
backend/app/models/skill_binary.py (1)
SkillBinary(13-26)
backend/app/services/adapters/bot_kinds.py (2)
backend/app/schemas/kind.py (1)
Ghost(41-47)backend/app/models/kind.py (1)
Kind(14-30)
backend/app/api/endpoints/kind/skills.py (3)
backend/app/schemas/kind.py (2)
Skill(308-314)SkillList(317-321)backend/app/core/security.py (1)
get_current_user(27-50)backend/app/services/adapters/skill_kinds.py (7)
create_skill(22-110)list_skills(141-163)get_skill_by_name(126-139)get_skill_by_id(112-124)get_skill_binary(289-320)update_skill(165-238)delete_skill(240-287)
🪛 Ruff (0.14.5)
backend/app/services/adapters/bot_kinds.py
592-592: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
backend/app/services/adapters/skill_kinds.py
55-55: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
118-118: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
133-133: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
155-155: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
158-158: Local variable total is assigned to but never used
Remove assignment to unused variable total
(F841)
194-194: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
258-258: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
270-270: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
306-306: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
executor/agents/claude_code/claude_code_agent.py
1163-1163: Do not catch blind exception: Exception
(BLE001)
1260-1260: Do not catch blind exception: Exception
(BLE001)
1261-1261: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1269-1269: Do not catch blind exception: Exception
(BLE001)
1270-1270: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1270-1270: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/services/skill_service.py
23-23: Unused static method argument: file_name
(ARG004)
67-70: Abstract raise to an inner function
(TRY301)
81-84: Abstract raise to an inner function
(TRY301)
99-102: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
106-109: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
108-108: Use explicit conversion flag
Replace with conversion flag
(RUF010)
145-145: Prefer TypeError exception for invalid type
(TRY004)
145-145: Abstract raise to an inner function
(TRY301)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
149-152: Abstract raise to an inner function
(TRY301)
154-154: Consider moving this statement to an else block
(TRY300)
157-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
159-159: Use explicit conversion flag
Replace with conversion flag
(RUF010)
164-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
166-166: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/api/endpoints/kind/skills.py
24-24: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: 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)
28-28: 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)
72-72: 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)
73-73: 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)
104-104: 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)
105-105: 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)
121-121: 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)
122-122: 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)
160-160: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
161-161: 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)
162-162: 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)
194-194: 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)
195-195: 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)
🔇 Additional comments (13)
backend/app/models/__init__.py (1)
16-24: LGTM!The
SkillBinarymodel is correctly imported and exported in__all__, following the established pattern for other models likeSharedTeam.backend/app/api/endpoints/kind/__init__.py (1)
12-22: LGTM!The skills router is correctly imported and registered with the appropriate prefix. The ordering (batch → skills → kinds) properly avoids path conflicts.
backend/app/schemas/kind.py (2)
33-33: LGTM!The
skillsfield is correctly added toGhostSpecas an optional list of strings, aligning with the Kubernetes-inspired CRD design for linking Skill resources.
292-321: LGTM!The Skill CRD schemas (
SkillSpec,SkillStatus,Skill,SkillList) follow the established Kubernetes-style pattern used by other resources in the codebase (Ghost, Model, Shell, etc.). Docstrings and type hints are properly applied. Based on learnings, this aligns with the Kubernetes-inspired CRD design.backend/app/schemas/bot.py (1)
6-6: LGTM!The
skillsfield is consistently added toBotBase,BotUpdate, andBotDetailschemas. The field is correctly typed asOptional[List[str]]for backward compatibility.BotInDBappropriately excludes skills since they are stored in the Ghost resource, not the Bot table directly.Also applies to: 19-19, 33-33, 55-55
backend/app/models/skill_binary.py (1)
13-26: LGTM on core model design.The model correctly implements:
- CASCADE delete on
kind_idforeign key for referential integrity- Unique constraint on
kind_id(one binary per skill)- Appropriate column types (LargeBinary for ZIP, String(64) for SHA256)
- MySQL charset configuration
backend/app/services/adapters/bot_kinds.py (3)
47-57: LGTM on skills validation in create flow.Skills are properly validated before bot creation, and the ghost spec is correctly constructed with optional skills field. The early validation ensures referential integrity.
571-602: LGTM on_validate_skillsimplementation.The method efficiently validates all skills in a single database query using
INclause, properly computes missing skills, and raises an informative HTTP 400 error. Good defensive programming with the early return for empty list.
346-355: LGTM on skills update flow.Skills are properly validated before updating the Ghost spec. The pattern correctly handles empty skills list (clears skills) and validates non-empty lists.
backend/app/api/endpoints/kind/skills.py (3)
43-63: File extension validation is superficial but acceptable.The
.zipextension check on line 44 is easily bypassed by renaming files, but this is acceptable sinceSkillValidator.validate_zip()performs proper content-based validation. The extension check provides early feedback to users.
101-116: LGTM!The
get_skillendpoint correctly validates ownership via the service and returns appropriate 404 error when not found.
191-207: LGTM!The
delete_skillendpoint correctly delegates to the service, which handles reference checks and returns appropriate errors. The 204 status code is correct for successful deletion.backend/app/services/adapters/skill_kinds.py (1)
49-110: LGTM!The
create_skillmethod correctly validates duplicates, usesflush()to obtain the Kind ID before creating the binary record, and commits atomically. The transaction handling ensures consistency.
| return StreamingResponse( | ||
| io.BytesIO(binary_data), | ||
| media_type="application/zip", | ||
| headers={ | ||
| "Content-Disposition": f"attachment; filename={skill.metadata.name}.zip" | ||
| } | ||
| ) |
There was a problem hiding this comment.
Sanitize filename in Content-Disposition header.
If skill.metadata.name contains special characters (spaces, quotes, non-ASCII), the header may be malformed or exploitable. Use proper quoting:
return StreamingResponse(
io.BytesIO(binary_data),
media_type="application/zip",
headers={
- "Content-Disposition": f"attachment; filename={skill.metadata.name}.zip"
+ "Content-Disposition": f'attachment; filename="{skill.metadata.name}.zip"'
}
)For full RFC 5987 compliance with non-ASCII names, consider using urllib.parse.quote for the filename.
🤖 Prompt for AI Agents
In backend/app/api/endpoints/kind/skills.py around lines 148 to 154, the
Content-Disposition header uses raw skill.metadata.name which can break the
header or allow injection for names with spaces, quotes or non-ASCII; sanitize
and properly quote the filename and add an RFC 5987 compliant filename*
parameter: escape any double quotes/backslashes in the ASCII fallback and wrap
it in quotes for filename, and use urllib.parse.quote(skill.metadata.name,
safe='') encoded as UTF-8 for filename* (e.g.
"filename*=UTF-8''<percent-encoded-name>") so clients can correctly handle
non-ASCII names.
| # Update skill_kind JSON | ||
| skill_json = skill_kind.json | ||
| skill_json["spec"].update({ | ||
| "description": metadata["description"], | ||
| "version": metadata.get("version"), | ||
| "author": metadata.get("author"), | ||
| "tags": metadata.get("tags") | ||
| }) | ||
| skill_json["status"].update({ | ||
| "fileSize": metadata["file_size"], | ||
| "fileHash": metadata["file_hash"] | ||
| }) | ||
| skill_kind.json = skill_json |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Find the skill_kinds.py file and check its imports and context
cat -n backend/app/services/adapters/skill_kinds.py | head -50Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
1 # SPDX-FileCopyrightText: 2025 Weibo, Inc.
2 #
3 # SPDX-License-Identifier: Apache-2.0
4
5 """
6 Skill adapter service for managing Skills using kinds table
7 """
8 from typing import List, Optional, Dict, Any
9 from fastapi import HTTPException
10 from sqlalchemy.orm import Session
11 from sqlalchemy import and_
12
13 from app.models.kind import Kind
14 from app.models.skill_binary import SkillBinary
15 from app.schemas.kind import Skill, SkillList, SkillSpec, SkillStatus, ObjectMeta
16 from app.services.skill_service import SkillValidator
17
18
19 class SkillKindsService:
20 """Service for managing Skills in kinds table"""
21
22 def create_skill(
23 self,
24 db: Session,
25 *,
26 name: str,
27 namespace: str,
28 file_content: bytes,
29 file_name: str,
30 user_id: int
31 ) -> Skill:
32 """
33 Create a new Skill with ZIP package.
34
35 Args:
36 db: Database session
37 name: Skill name (unique per user)
38 namespace: Namespace (default: "default")
39 file_content: ZIP file binary content
40 file_name: Original file name
41 user_id: User ID
42
43 Returns:
44 Created Skill CRD
45
46 Raises:
47 HTTPException: If validation fails or name already exists
48 """
49 # Check duplicate skill name
50 existing = db.query(Kind).filter(
</shell_result>
Script executed:
#!/bin/bash
# Find the SkillKind model definition
fd -t f "skill_kind" --type py | head -20Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stderr:
error: 'py' isn't a valid value for '--type <filetype>'
[possible values: file, directory, symlink, executable, empty, socket, pipe]
For more information try '--help'
</shell_result>
Script executed:
#!/bin/bash
# Search for SkillKind or skill_kind model class definition
rg -n "class\s+SkillKind" --type=py -A 15Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
backend/app/services/adapters/skill_kinds.py:19:class SkillKindsService:
backend/app/services/adapters/skill_kinds.py-20- """Service for managing Skills in kinds table"""
backend/app/services/adapters/skill_kinds.py-21-
backend/app/services/adapters/skill_kinds.py-22- def create_skill(
backend/app/services/adapters/skill_kinds.py-23- self,
backend/app/services/adapters/skill_kinds.py-24- db: Session,
backend/app/services/adapters/skill_kinds.py-25- *,
backend/app/services/adapters/skill_kinds.py-26- name: str,
backend/app/services/adapters/skill_kinds.py-27- namespace: str,
backend/app/services/adapters/skill_kinds.py-28- file_content: bytes,
backend/app/services/adapters/skill_kinds.py-29- file_name: str,
backend/app/services/adapters/skill_kinds.py-30- user_id: int
backend/app/services/adapters/skill_kinds.py-31- ) -> Skill:
backend/app/services/adapters/skill_kinds.py-32- """
backend/app/services/adapters/skill_kinds.py-33- Create a new Skill with ZIP package.
backend/app/services/adapters/skill_kinds.py-34-
</shell_result>
Script executed:
#!/bin/bash
# Check SQLAlchemy version and how JSON columns are configured
rg -n "JSON\|json_column" --type=py backend/app/ -B 2 -A 2 | head -80Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
</shell_result>
Script executed:
#!/bin/bash
# Look for flag_modified usage in the codebase
rg -n "flag_modified" --type=pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
backend/app/services/shared_team.py:13:from sqlalchemy.orm.attributes import flag_modified
backend/app/services/shared_team.py:273: flag_modified(team, "json")
backend/app/services/adapters/bot_kinds.py:11:from sqlalchemy.orm.attributes import flag_modified
backend/app/services/adapters/bot_kinds.py:305: flag_modified(bot, "json") # Mark JSON field as modified
backend/app/services/adapters/bot_kinds.py:325: flag_modified(shell, "json") # Mark JSON field as modified
backend/app/services/adapters/bot_kinds.py:331: flag_modified(model, "json") # Mark JSON field as modified
backend/app/services/adapters/bot_kinds.py:337: flag_modified(ghost, "json") # Mark JSON field as modified
backend/app/services/adapters/bot_kinds.py:343: flag_modified(ghost, "json") # Mark JSON field as modified
backend/app/services/adapters/bot_kinds.py:354: flag_modified(ghost, "json")
backend/app/services/adapters/team_kinds.py:11:from sqlalchemy.orm.attributes import flag_modified
backend/app/services/adapters/team_kinds.py:430: flag_modified(team, "json")
backend/app/services/adapters/team_kinds.py:768: flag_modified(task, "json")
backend/app/services/adapters/task_kinds.py:13:from sqlalchemy.orm.attributes import flag_modified
backend/app/services/adapters/task_kinds.py:684: flag_modified(workspace, "json")
backend/app/services/adapters/task_kinds.py:694: flag_modified(task, "json")
backend/app/services/adapters/task_kinds.py:758: flag_modified(task, "json")
backend/app/services/adapters/executor_kinds.py:23:from sqlalchemy.orm.attributes import flag_modified
backend/app/services/adapters/executor_kinds.py:193: flag_modified(task, "json")
backend/app/services/adapters/executor_kinds.py:466: flag_modified(task, "json")
backend/app/services/adapters/executor_kinds.py:594: flag_modified(task, "json")
</shell_result>
Script executed:
#!/bin/bash
# View the exact lines in question with context
sed -n '195,225p' backend/app/services/adapters/skill_kinds.pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
).first()
if not skill_kind:
raise HTTPException(status_code=404, detail="Skill not found")
# Validate new ZIP package
metadata = SkillValidator.validate_zip(file_content, file_name)
# Update skill_kind JSON
skill_json = skill_kind.json
skill_json["spec"].update({
"description": metadata["description"],
"version": metadata.get("version"),
"author": metadata.get("author"),
"tags": metadata.get("tags")
})
skill_json["status"].update({
"fileSize": metadata["file_size"],
"fileHash": metadata["file_hash"]
})
skill_kind.json = skill_json
# Update or create SkillBinary
skill_binary = db.query(SkillBinary).filter(
SkillBinary.kind_id == skill_id
).first()
if skill_binary:
skill_binary.binary_data = file_content
skill_binary.file_size = metadata["file_size"]
skill_binary.file_hash = metadata["file_hash"]
</shell_result>
Script executed:
#!/bin/bash
# Find and examine the Kind model definition
find . -name "*.py" -path "*/models/*" | xargs grep -l "class Kind"Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
./backend/app/models/kind.py
</shell_result>
Script executed:
#!/bin/bash
# Check the Kind model JSON column configuration
rg -n "class Kind" -A 30 --type py backend/app/models/Repository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
backend/app/models/kind.py:14:class Kind(Base):
backend/app/models/kind.py-15- """Unified Kind model for all Kubernetes-style resources"""
backend/app/models/kind.py-16- __tablename__ = "kinds"
backend/app/models/kind.py-17-
backend/app/models/kind.py-18- id = Column(Integer, primary_key=True, index=True)
backend/app/models/kind.py-19- user_id = Column(Integer, nullable=False)
backend/app/models/kind.py-20- kind = Column(String(50), nullable=False, index=True)
backend/app/models/kind.py-21- name = Column(String(100), nullable=False)
backend/app/models/kind.py-22- namespace = Column(String(100), nullable=False, default="default")
backend/app/models/kind.py-23- json = Column(JSON, nullable=False)
backend/app/models/kind.py-24- is_active = Column(Boolean, default=True)
backend/app/models/kind.py-25- created_at = Column(DateTime, default=datetime.now)
backend/app/models/kind.py-26- updated_at = Column(DateTime, default=datetime.now, onupdate=datetime.now)
backend/app/models/kind.py-27-
backend/app/models/kind.py-28- __table_args__ = (
backend/app/models/kind.py-29- {"mysql_charset": "utf8mb4", "mysql_collate": "utf8mb4_unicode_ci"},
backend/app/models/kind.py-30- )
</shell_result>
Script executed:
#!/bin/bash
# Look for the update_skill method context - check if there's anything after line 215
sed -n '210,240p' backend/app/services/adapters/skill_kinds.pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
})
skill_json["status"].update({
"fileSize": metadata["file_size"],
"fileHash": metadata["file_hash"]
})
skill_kind.json = skill_json
# Update or create SkillBinary
skill_binary = db.query(SkillBinary).filter(
SkillBinary.kind_id == skill_id
).first()
if skill_binary:
skill_binary.binary_data = file_content
skill_binary.file_size = metadata["file_size"]
skill_binary.file_hash = metadata["file_hash"]
else:
skill_binary = SkillBinary(
kind_id=skill_id,
binary_data=file_content,
file_size=metadata["file_size"],
file_hash=metadata["file_hash"]
)
db.add(skill_binary)
db.commit()
db.refresh(skill_kind)
return self._kind_to_skill(skill_kind)
def delete_skill(self, db: Session, *, skill_id: int, user_id: int) -> None:
</shell_result>
Script executed:
#!/bin/bash
# Check if db.commit() is called after the JSON update
sed -n '203,250p' backend/app/services/adapters/skill_kinds.pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
# Update skill_kind JSON
skill_json = skill_kind.json
skill_json["spec"].update({
"description": metadata["description"],
"version": metadata.get("version"),
"author": metadata.get("author"),
"tags": metadata.get("tags")
})
skill_json["status"].update({
"fileSize": metadata["file_size"],
"fileHash": metadata["file_hash"]
})
skill_kind.json = skill_json
# Update or create SkillBinary
skill_binary = db.query(SkillBinary).filter(
SkillBinary.kind_id == skill_id
).first()
if skill_binary:
skill_binary.binary_data = file_content
skill_binary.file_size = metadata["file_size"]
skill_binary.file_hash = metadata["file_hash"]
else:
skill_binary = SkillBinary(
kind_id=skill_id,
binary_data=file_content,
file_size=metadata["file_size"],
file_hash=metadata["file_hash"]
)
db.add(skill_binary)
db.commit()
db.refresh(skill_kind)
return self._kind_to_skill(skill_kind)
def delete_skill(self, db: Session, *, skill_id: int, user_id: int) -> None:
"""
Delete Skill (soft delete).
Checks if the Skill is referenced by any Ghost before deletion.
Args:
db: Database session
skill_id: Skill ID
user_id: User ID
</shell_result>
Based on my verification of the codebase:
Missing flag_modified() call—inconsistent with codebase pattern and risks data persistence issues.
The codebase consistently uses flag_modified() after JSON mutations across all adapters:
bot_kinds.py(lines 305, 325, 331, 337, 343, 354)team_kinds.py(lines 430, 768)task_kinds.py(lines 684, 694, 758)executor_kinds.py(lines 193, 466, 594)shared_team.py(line 273)
However, skill_kinds.py at lines 203-215 mutates the JSON column without calling flag_modified(). While line 215 does reassign skill_kind.json = skill_json, this alone may not trigger SQLAlchemy's change tracking in all scenarios. Add the import and call after line 215:
from sqlalchemy.orm.attributes import flag_modified
# After line 215:
flag_modified(skill_kind, "json")🤖 Prompt for AI Agents
In backend/app/services/adapters/skill_kinds.py around lines 203 to 215 the code
mutates the JSON column (skill_kind.json) but does not call flag_modified(),
which the codebase uses to ensure SQLAlchemy detects JSON changes; import
flag_modified from sqlalchemy.orm.attributes and add a call
flag_modified(skill_kind, "json") immediately after the assignment on line 215
(and add the import near other imports in the file).
| with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: | ||
| # Security check: prevent Zip Slip | ||
| for file_info in zip_file.filelist: | ||
| if file_info.filename.startswith('/') or '..' in file_info.filename: | ||
| logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") | ||
| shutil.rmtree(skill_target_dir) | ||
| break | ||
| else: | ||
| zip_file.extractall(skill_target_dir) | ||
| logger.info(f"Deployed skill '{skill_name}' to {skill_target_dir}") | ||
| success_count += 1 |
There was a problem hiding this comment.
Strengthen Zip Slip protection with path normalization.
The current check for '..' in filename is a good start, but can be bypassed with certain encodings or path variations. Use os.path.realpath to ensure the resolved path stays within the target directory.
with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file:
# Security check: prevent Zip Slip
for file_info in zip_file.filelist:
- if file_info.filename.startswith('/') or '..' in file_info.filename:
+ # Normalize and verify path stays within target directory
+ target_path = os.path.realpath(
+ os.path.join(skill_target_dir, file_info.filename)
+ )
+ if not target_path.startswith(os.path.realpath(skill_target_dir) + os.sep):
logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}")
shutil.rmtree(skill_target_dir)
break
else:
zip_file.extractall(skill_target_dir)This ensures that even encoded or obfuscated path traversal attempts are caught.
📝 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.
| with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: | |
| # Security check: prevent Zip Slip | |
| for file_info in zip_file.filelist: | |
| if file_info.filename.startswith('/') or '..' in file_info.filename: | |
| logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") | |
| shutil.rmtree(skill_target_dir) | |
| break | |
| else: | |
| zip_file.extractall(skill_target_dir) | |
| logger.info(f"Deployed skill '{skill_name}' to {skill_target_dir}") | |
| success_count += 1 | |
| with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: | |
| # Security check: prevent Zip Slip | |
| for file_info in zip_file.filelist: | |
| # Normalize and verify path stays within target directory | |
| target_path = os.path.realpath( | |
| os.path.join(skill_target_dir, file_info.filename) | |
| ) | |
| if not target_path.startswith(os.path.realpath(skill_target_dir) + os.sep): | |
| logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") | |
| shutil.rmtree(skill_target_dir) | |
| break | |
| else: | |
| zip_file.extractall(skill_target_dir) | |
| logger.info(f"Deployed skill '{skill_name}' to {skill_target_dir}") | |
| success_count += 1 |
- Add unit tests for SkillValidator class (ZIP validation, YAML parsing, security checks) - Add integration tests for SkillKindsService (CRUD operations, user isolation, Ghost referential integrity) - Add API integration tests for Skills endpoints (upload, list, download, update, delete) - Fix bug in executor: extract skills from bot_config["spec"]["skills"] instead of bot_config["skills"] Coverage: - 15 unit tests for SkillValidator (file size, format validation, SKILL.md parsing, Zip Slip防护) - 15 integration tests for SkillKindsService (user isolation, pagination, Ghost reference checks) - 17 API tests for Skills endpoints (authentication, error handling, user isolation)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
executor/agents/claude_code/claude_code_agent.py (1)
1248-1258: Strengthen Zip Slip protection with path normalization.The current check for
'..'in filename can be bypassed with certain encodings or path variations (e.g.,foo/..%2f../bar). Useos.path.realpathto ensure the resolved path stays within the target directory.with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: # Security check: prevent Zip Slip for file_info in zip_file.filelist: - if file_info.filename.startswith('/') or '..' in file_info.filename: + # Normalize and verify path stays within target directory + target_path = os.path.realpath( + os.path.join(skill_target_dir, file_info.filename) + ) + if not target_path.startswith(os.path.realpath(skill_target_dir) + os.sep): logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") shutil.rmtree(skill_target_dir) break else: zip_file.extractall(skill_target_dir)
🧹 Nitpick comments (2)
executor/agents/claude_code/claude_code_agent.py (1)
1269-1271: Uselogger.exceptionto capture stack trace.When logging exceptions at the outer level, use
logger.exceptioninstead oflogger.errorto include the full stack trace, which aids debugging.except Exception as e: - logger.error(f"Error in _download_and_deploy_skills: {str(e)}") + logger.exception("Error in _download_and_deploy_skills") # Don't raise - skills deployment failure shouldn't block task executionbackend/tests/services/test_skill_service.py (1)
216-237: Consider documenting the non-deterministic behavior.The test correctly handles that ZIP file iteration order is not guaranteed, but the comment could be clearer. The assertion verifies that one of the SKILL.md files is used, which is the expected behavior per the implementation.
# Should use the first SKILL.md found - assert result["description"] in ["First skill", "Second skill"] - assert result["version"] in ["1.0.0", "2.0.0"] + # Note: ZIP iteration order is not guaranteed, so we verify one of the two is used + assert result["description"] in ["First skill", "Second skill"] + assert result["version"] in ["1.0.0", "2.0.0"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/api/test_skills_api.py(1 hunks)backend/tests/services/test_skill_kinds_service.py(1 hunks)backend/tests/services/test_skill_service.py(1 hunks)executor/agents/claude_code/claude_code_agent.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant with Black formatter (line length: 88)
Use isort for import organization in Python
Python functions and variables must have descriptive names
Docstrings required for all public Python functions and classes
Extract magic numbers to named constants in Python
Python functions must not exceed 50 lines
Type hints required for all Python function signatures
Run pylint and flake8 for Python code quality checks
Files:
backend/tests/services/test_skill_kinds_service.pybackend/tests/services/test_skill_service.pybackend/tests/api/test_skills_api.pyexecutor/agents/claude_code/claude_code_agent.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Follow AAA pattern in tests: Arrange, Act, Assert
Mock external services (Anthropic, OpenAI, Docker, APIs) in Python tests
Use descriptive test names explaining what is tested in Python tests
Test edge cases and error conditions in Python tests
Keep Python tests independent and isolated
Files:
backend/tests/services/test_skill_kinds_service.pybackend/tests/services/test_skill_service.pybackend/tests/api/test_skills_api.py
🧬 Code graph analysis (2)
backend/tests/services/test_skill_service.py (1)
backend/app/services/skill_service.py (2)
SkillValidator(17-167)validate_zip(23-109)
backend/tests/api/test_skills_api.py (3)
backend/app/models/kind.py (1)
Kind(14-30)backend/tests/services/test_skill_kinds_service.py (1)
create_test_zip(24-30)backend/tests/conftest.py (5)
test_client(137-155)test_token(121-125)test_db(22-47)test_user(67-81)test_admin_token(129-133)
🪛 Ruff (0.14.5)
executor/agents/claude_code/claude_code_agent.py
1163-1163: Do not catch blind exception: Exception
(BLE001)
1260-1260: Do not catch blind exception: Exception
(BLE001)
1261-1261: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1269-1269: Do not catch blind exception: Exception
(BLE001)
1270-1270: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1270-1270: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (19)
executor/agents/claude_code/claude_code_agent.py (4)
1216-1242: LGTM!HTTP requests have appropriate timeouts (30s for list, 60s for download) and proper Bearer token authentication headers.
1183-1191: LGTM!Skills directory is properly cleared before deployment to ensure a clean state, and the directory is created with
parents=Trueto handle nested path creation.
1195-1199: LGTM!Auth token validation with graceful degradation - missing token logs a warning but doesn't block task execution, which aligns with the design goal of non-blocking skill deployment.
441-443: LGTM!Skill deployment is appropriately integrated into the initialization flow, occurring after Claude Code settings are saved. This ensures skills are available before task execution begins.
backend/tests/services/test_skill_kinds_service.py (5)
19-30: LGTM!Test class is properly marked with
@pytest.mark.integrationand includes a reusable helper method for creating test ZIPs. The helper follows the pattern used in other test files.
32-61: LGTM!Test follows AAA pattern with comprehensive assertions covering all skill metadata fields including description, version, author, tags, state, file size, and SHA256 hash length validation.
148-172: LGTM!User isolation test correctly verifies that one user cannot access another user's skill. The test creates a skill as
test_userand confirmstest_admin_userreceivesNonewhen attempting to retrieve it.
366-416: LGTM!Critical test for referential integrity. Properly verifies that deleting a skill referenced by a Ghost returns HTTP 400 with an error message that includes both "referenced by Ghosts" and the Ghost name. This prevents orphaned references.
230-270: LGTM!Pagination test correctly creates multiple skills, fetches two pages with different offsets, and verifies no overlap using
isdisjoint(). This is a solid approach to testing pagination behavior.backend/tests/services/test_skill_service.py (4)
16-35: LGTM!Test class is properly marked with
@pytest.mark.unitand includes a flexible helper that accepts a dictionary of files, allowing tests to create various ZIP structures for different scenarios.
166-195: LGTM!Security tests properly verify Zip Slip and absolute path attacks are detected and rejected with HTTP 400. Tests use direct
zipfile.writestrto inject malicious paths, accurately simulating attack vectors.
239-257: LGTM!Excellent i18n coverage. Test verifies that Unicode content (Chinese characters) in description, author, and tags is correctly parsed and preserved through the validation process.
81-94: LGTM!Test correctly verifies the 10MB file size limit by creating an 11MB payload and asserting HTTP 413 response with appropriate error message.
backend/tests/api/test_skills_api.py (6)
18-29: LGTM!Test class properly marked with
@pytest.mark.apiand includes a helper method consistent with other test files in the PR.
56-67: LGTM!Test correctly verifies that the upload endpoint requires authentication by asserting HTTP 401 when no Bearer token is provided.
204-232: LGTM!Comprehensive download test verifies response headers (
content-type,Content-Disposition), filename in disposition header, and validates the downloaded ZIP by checking it containsSKILL.md.
363-392: LGTM!Comprehensive user isolation test verifies that one user cannot access or delete another user's skill. Both GET and DELETE operations correctly return HTTP 404 for cross-user access attempts.
307-352: LGTM!Test correctly verifies API-level enforcement of referential integrity. The approach of creating the Ghost directly in the database is appropriate for setting up test preconditions without requiring additional API endpoints.
277-305: LGTM!Test follows the create-delete-verify pattern correctly. After deletion (HTTP 204), it confirms the skill is no longer accessible (HTTP 404), validating the soft delete behavior.
- Add Skill type definitions to api.ts (Skill, SkillMetadata, SkillSpec, SkillStatus, SkillList) - Create Skills API client (upload, update, delete, download, list skills) - Implement SkillList component for Skills management page - Implement SkillUploadModal with drag-and-drop support and progress tracking - Add Skills tab to Settings page (desktop and mobile responsive) - Extend Bot API types to include skills field (CreateBotRequest, UpdateBotRequest) - Add Skills multi-select to BotEdit component with tag-based UI - Auto-load available skills on Bot edit page - Support adding/removing skills from Bots UI Features: - Skill cards with metadata display (version, author, tags, file size, status) - Upload with file validation (ZIP format, max 10MB, SKILL.md requirement) - Drag-and-drop file upload with progress indicator - Download existing skills - Delete with confirmation and Ghost reference warning - Skills multi-select with tag-based selection (add/remove skills easily) Integration: - Skills linked to Bots through skills array field - Skills deployed by Executor when Bot starts task
- Add Skill section to YAML specification (English and Chinese) - Document Skill CRD format with complete example - Explain ZIP package requirements and SKILL.md format - Document how to associate Skills with Ghosts - Add skills field to Ghost specification - Include detailed field descriptions and examples Content: - Skill metadata, spec, and status fields - YAML frontmatter format for SKILL.md - ZIP package structure requirements - Usage examples with Ghost configuration - Automatic deployment explanation
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
frontend/src/apis/skills.ts (2)
213-222: Error handling in deleteSkill could mask the original error.The catch block at lines 218-219 catches the JSON parse error but then throws a new error with the same text, which could lose the original error context if
ewas thrown for another reason.if (!response.ok) { const error = await response.text(); try { const json = JSON.parse(error); throw new Error(json.detail || 'Failed to delete skill'); - } catch (e) { - throw new Error(error || 'Failed to delete skill'); + } catch { + // JSON parse failed, use raw error text + throw new Error(error || 'Failed to delete skill'); } }The explicit comment clarifies the catch is only for JSON parsing.
103-141: Consider adding timeout handling for upload requests.The XHR-based upload has no timeout configured, meaning uploads could hang indefinitely on slow networks or unresponsive servers. While not critical, adding a timeout improves resilience.
xhr.open('POST', url); xhr.setRequestHeader('Authorization', `Bearer ${token}`); + xhr.timeout = 120000; // 2 minute timeout for uploads + xhr.addEventListener('timeout', () => { + reject(new Error('Upload timed out')); + }); xhr.send(formData);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/en/reference/yaml-specification.md(2 hunks)docs/zh/reference/yaml-specification.md(2 hunks)frontend/src/apis/bots.ts(2 hunks)frontend/src/apis/skills.ts(1 hunks)frontend/src/app/settings/page.tsx(7 hunks)frontend/src/features/settings/components/BotEdit.tsx(7 hunks)frontend/src/features/settings/components/skills/SkillList.tsx(1 hunks)frontend/src/features/settings/components/skills/SkillUploadModal.tsx(1 hunks)frontend/src/types/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: TypeScript strict mode must be enabled in Frontend
Use functional components with hooks in React
Use Prettier formatter in Frontend with single quotes and no semicolons
Use ESLint with Next.js configuration in Frontend
Useconstoverlet, never usevarin TypeScript
Component names must use PascalCase; file names must use kebab-case in Frontend
Use mobile-first responsive design approach in Frontend components
Frontend design: Low saturation + low contrast for reduced eye strain with generous whitespace
Frontend design: Minimal shadows, subtle component differentiation (<10% background variance)
Frontend design: Use mint blue (#14B8A6) as primary accent sparingly
Use Tailwind CSS variables for colors: --color-bg-base, --color-text-primary, --color-border, etc.
Frontend spacing: Use standard units - p-2 (8px), p-4 (16px), p-6 (24px), gap-3 (12px)
Frontend border-radius: rounded-2xl (16px) for large containers, rounded-lg (12px) for cards, rounded-md (6px) for buttons
Frontend typography: H1 text-xl font-semibold, H2 text-lg font-semibold, H3 text-base font-medium, Body text-sm
Use shadcn/ui Button component with variants: default, secondary, ghost, outline, link
Use shadcn/ui Form component with react-hook-form + zod validation
Files:
frontend/src/apis/bots.tsfrontend/src/features/settings/components/skills/SkillUploadModal.tsxfrontend/src/app/settings/page.tsxfrontend/src/types/api.tsfrontend/src/features/settings/components/skills/SkillList.tsxfrontend/src/features/settings/components/BotEdit.tsxfrontend/src/apis/skills.ts
frontend/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend: Only use
NEXT_PUBLIC_*for client-safe environment variables
Files:
frontend/src/apis/bots.tsfrontend/src/features/settings/components/skills/SkillUploadModal.tsxfrontend/src/app/settings/page.tsxfrontend/src/types/api.tsfrontend/src/features/settings/components/skills/SkillList.tsxfrontend/src/features/settings/components/BotEdit.tsxfrontend/src/apis/skills.ts
frontend/src/types/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define TypeScript types in
src/types/directory in Frontend
Files:
frontend/src/types/api.ts
🧠 Learnings (3)
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Use Kubernetes-inspired CRD design (Ghost, Model, Shell, Bot, Team, Task)
Applied to files:
docs/zh/reference/yaml-specification.mdfrontend/src/types/api.tsdocs/en/reference/yaml-specification.md
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Applies to frontend/**/*.{ts,tsx} : Use functional components with hooks in React
Applied to files:
frontend/src/features/settings/components/BotEdit.tsx
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Applies to frontend/**/*.{ts,tsx} : Use shadcn/ui Button component with variants: default, secondary, ghost, outline, link
Applied to files:
frontend/src/features/settings/components/BotEdit.tsx
🧬 Code graph analysis (4)
frontend/src/app/settings/page.tsx (2)
frontend/src/features/settings/components/skills/SkillList.tsx (1)
SkillList(34-285)frontend/src/types/api.ts (1)
SkillList(70-72)
frontend/src/types/api.ts (1)
frontend/src/features/settings/components/skills/SkillList.tsx (1)
SkillList(34-285)
frontend/src/features/settings/components/BotEdit.tsx (3)
frontend/src/apis/skills.ts (1)
fetchSkillsList(13-38)frontend/src/hooks/use-toast.ts (1)
toast(189-189)frontend/src/components/ui/select.tsx (5)
Select(147-147)SelectTrigger(150-150)SelectValue(149-149)SelectContent(151-151)SelectItem(153-153)
frontend/src/apis/skills.ts (3)
frontend/src/types/api.ts (2)
Skill(62-68)SkillList(70-72)frontend/src/apis/user.ts (1)
getToken(54-59)frontend/src/features/settings/components/skills/SkillList.tsx (1)
SkillList(34-285)
🔇 Additional comments (16)
docs/en/reference/yaml-specification.md (2)
10-10: TOC and Ghost spec updates look good.The Skill TOC entry is properly positioned, and the
spec.skillsfield description in the Ghost table is clear with a helpful example.Also applies to: 62-62
66-142: Skill section structure and content are well documented.The Skill section comprehensively covers the resource definition, ZIP package requirements, SKILL.md format, and integration with Ghost. The documentation aligns well with the PR's implementation details regarding ZIP validation, frontmatter extraction, and executor auto-deployment behavior.
docs/zh/reference/yaml-specification.md (3)
10-10: Chinese TOC and Ghost spec updates are properly formatted.The Skill TOC entry and
spec.skillsfield addition match the English version, with appropriate Chinese translations and clear examples.Also applies to: 61-62
66-87: Chinese Skill introduction and example YAML are accurate.The Chinese description clearly conveys the Skill concept and deployment mechanism, maintaining consistency with the English version while using appropriate Chinese terminology.
89-142: Chinese Skill section is fully and accurately translated.All field descriptions, ZIP requirements, and integration examples are properly translated to Chinese with consistent terminology. The documentation quality is high and matches the English version's scope.
frontend/src/apis/bots.ts (1)
15-15: LGTM!The optional
skills?: string[]field additions to both request interfaces correctly align with the backend schema changes and theBottype definition inapi.ts. The optional nature is appropriate since skills are not required for bot creation/update.Also applies to: 24-24
frontend/src/app/settings/page.tsx (1)
184-186: LGTM!The SkillList component integration into both desktop and mobile tab panels follows the established pattern of other settings tabs.
frontend/src/types/api.ts (2)
42-72: LGTM!The Skill-related types follow a clean CRD (Custom Resource Definition) pattern consistent with the Kubernetes-inspired design mentioned in the project learnings. The type structure with
apiVersion,kind,metadata,spec, andstatusprovides a well-organized and extensible schema.
36-36: LGTM!The optional
skillsfield addition to the Bot interface is correctly typed and the inline comment aids readability.frontend/src/features/settings/components/BotEdit.tsx (2)
241-258: LGTM!The skills fetching effect correctly handles loading state, errors via toast, and data transformation. The implementation mirrors the agents fetching pattern at lines 221-239, maintaining consistency.
413-413: LGTM!Conditionally including
skillsonly when there are selected items prevents sending an empty array, which correctly handles the optional nature of the field.frontend/src/features/settings/components/skills/SkillUploadModal.tsx (1)
136-254: LGTM!The Dialog implementation is well-structured with proper accessibility attributes (
htmlFor,idpairings), disabled states during upload, and consistent use of shadcn/ui components per coding guidelines.frontend/src/features/settings/components/skills/SkillList.tsx (2)
156-159: Key fallback is reasonable but consider consistency.Using
skill.metadata.labels?.id || skill.metadata.nameas key is acceptable since skill names should be unique within a namespace. However, ensure the backend always provides the ID label for consistency.
131-283: LGTM!The component renders a clean UI with proper empty state handling, consistent Card usage, and a well-implemented delete confirmation dialog that includes the referential integrity warning about Ghost dependencies.
frontend/src/apis/skills.ts (2)
227-251: LGTM!The download implementation correctly creates a blob URL, triggers the download via a temporary anchor element, and properly cleans up with
revokeObjectURLto prevent memory leaks.
256-261: LGTM!The
formatFileSizeutility correctly handles the expected range of file sizes (up to 10MB limit) with appropriate fallback for undefined values.
| |------|------|----------|-------------| | ||
| | `metadata.name` | string | Yes | Unique identifier for the Skill (used in Ghost `spec.skills` field) | | ||
| | `metadata.namespace` | string | Yes | Namespace, typically `default` | | ||
| | `spec.description` | string | Yes | Skill功能描述 (extracted from SKILL.md frontmatter) | |
There was a problem hiding this comment.
Fix mixed language in field description.
Line 95 contains a Chinese phrase "Skill功能描述" in English documentation. This should be entirely in English.
Apply this diff to correct the field description:
- | `spec.description` | string | Yes | Skill功能描述 (extracted from SKILL.md frontmatter) |
+ | `spec.description` | string | Yes | Skill description (extracted from SKILL.md frontmatter) |📝 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.
| | `spec.description` | string | Yes | Skill功能描述 (extracted from SKILL.md frontmatter) | | |
| | `spec.description` | string | Yes | Skill description (extracted from SKILL.md frontmatter) | |
🤖 Prompt for AI Agents
In docs/en/reference/yaml-specification.md around line 95, the field description
mixes Chinese ("Skill功能描述") into English docs; replace that phrase with an
English equivalent such as "Skill description (extracted from SKILL.md
frontmatter)" so the entire row reads: `| spec.description | string | Yes |
Skill description (extracted from SKILL.md frontmatter) |`.
frontend/src/app/settings/page.tsx
Outdated
| <Tab | ||
| className={({ selected }) => | ||
| `w-full flex items-center space-x-3 px-3 py-2 text-sm rounded-md transition-colors duration-200 focus:outline-none ${ | ||
| selected | ||
| ? 'bg-muted text-text-primary' | ||
| : 'text-text-muted hover:text-text-primary hover:bg-muted' | ||
| }` | ||
| } | ||
| > | ||
| <PackageIcon className="w-4 h-4" /> | ||
| <span>Skills</span> | ||
| </Tab> |
There was a problem hiding this comment.
Missing i18n translation for Skills tab label.
The Skills tab label is hardcoded as "Skills" while other tabs use t('settings.integrations'), t('settings.bot'), etc. for internationalization.
Apply this diff to use translation:
<Tab
className={({ selected }) =>
`w-full flex items-center space-x-3 px-3 py-2 text-sm rounded-md transition-colors duration-200 focus:outline-none ${
selected
? 'bg-muted text-text-primary'
: 'text-text-muted hover:text-text-primary hover:bg-muted'
}`
}
>
<PackageIcon className="w-4 h-4" />
- <span>Skills</span>
+ <span>{t('settings.skills')}</span>
</Tab>The same change should be applied to the mobile tab at line 237.
📝 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.
| <Tab | |
| className={({ selected }) => | |
| `w-full flex items-center space-x-3 px-3 py-2 text-sm rounded-md transition-colors duration-200 focus:outline-none ${ | |
| selected | |
| ? 'bg-muted text-text-primary' | |
| : 'text-text-muted hover:text-text-primary hover:bg-muted' | |
| }` | |
| } | |
| > | |
| <PackageIcon className="w-4 h-4" /> | |
| <span>Skills</span> | |
| </Tab> | |
| <Tab | |
| className={({ selected }) => | |
| `w-full flex items-center space-x-3 px-3 py-2 text-sm rounded-md transition-colors duration-200 focus:outline-none ${ | |
| selected | |
| ? 'bg-muted text-text-primary' | |
| : 'text-text-muted hover:text-text-primary hover:bg-muted' | |
| }` | |
| } | |
| > | |
| <PackageIcon className="w-4 h-4" /> | |
| <span>{t('settings.skills')}</span> | |
| </Tab> |
🤖 Prompt for AI Agents
In frontend/src/app/settings/page.tsx around lines 136 to 147, the Skills tab
label is hardcoded as "Skills"; replace the hardcoded string with the i18n
lookup t('settings.skills') (import/use the same t function used by other tabs)
so the tab label is localized, and apply the same replacement for the mobile tab
label at line ~237 to ensure both desktop and mobile use the translation key.
| {/* Skills Selection */} | ||
| <div className="flex flex-col"> | ||
| <div className="flex items-center mb-1"> | ||
| <label className="block text-base font-medium text-text-primary">Skills</label> | ||
| <span className="text-xs text-text-muted ml-2">(Optional)</span> | ||
| </div> | ||
| <div className="bg-base rounded-md p-2 min-h-[80px]"> | ||
| {loadingSkills ? ( | ||
| <div className="text-sm text-text-muted">Loading skills...</div> | ||
| ) : availableSkills.length === 0 ? ( | ||
| <div className="text-sm text-text-muted"> | ||
| No skills available. Upload skills in the Skills tab. | ||
| </div> | ||
| ) : ( | ||
| <div className="space-y-2"> | ||
| <Select | ||
| value="" | ||
| onValueChange={(value) => { | ||
| if (value && !selectedSkills.includes(value)) { | ||
| setSelectedSkills([...selectedSkills, value]); | ||
| } | ||
| }} | ||
| > | ||
| <SelectTrigger className="w-full"> | ||
| <SelectValue placeholder="Select a skill to add..." /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {availableSkills | ||
| .filter(skill => !selectedSkills.includes(skill)) | ||
| .map(skill => ( | ||
| <SelectItem key={skill} value={skill}> | ||
| {skill} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
|
|
||
| {selectedSkills.length > 0 && ( | ||
| <div className="flex flex-wrap gap-1.5"> | ||
| {selectedSkills.map(skill => ( | ||
| <div | ||
| key={skill} | ||
| className="inline-flex items-center gap-1 px-2 py-1 bg-muted rounded-md text-sm" | ||
| > | ||
| <span>{skill}</span> | ||
| <button | ||
| onClick={() => setSelectedSkills(selectedSkills.filter(s => s !== skill))} | ||
| className="text-text-muted hover:text-text-primary" | ||
| > | ||
| <XIcon className="w-3 h-3" /> | ||
| </button> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Missing i18n translations and accessibility attribute.
The Skills selection block has hardcoded English strings that should use the translation hook for consistency:
- Line 702:
"Skills" - Line 703:
"(Optional)" - Line 707:
"Loading skills..." - Lines 709-710:
"No skills available..." - Line 723:
"Select a skill to add..."
Additionally, the remove button at lines 744-749 should include an accessible label for screen readers.
Example fix for the remove button:
<button
onClick={() => setSelectedSkills(selectedSkills.filter(s => s !== skill))}
className="text-text-muted hover:text-text-primary"
+ aria-label={`Remove ${skill}`}
>
<XIcon className="w-3 h-3" />
</button>🤖 Prompt for AI Agents
In frontend/src/features/settings/components/BotEdit.tsx around lines 699 to
757, replace hard-coded English strings with translation hook calls and add an
accessible label to the remove button: use the i18n hook (e.g.
t('settings.skills.label'), t('settings.skills.optional'),
t('settings.skills.loading'), t('settings.skills.noneAvailable'),
t('settings.skills.selectPlaceholder')) for the displayed texts and the Select
placeholder, and add an aria-label to the remove button (e.g.
aria-label={t('settings.skills.remove', { skill })} or a localized template that
includes the skill name) so screen readers can announce the action.
| const handleConfirmDelete = async () => { | ||
| if (!skillToDelete) return; | ||
|
|
||
| try { | ||
| const skillId = parseInt(skillToDelete.metadata.labels?.id || '0'); | ||
| await deleteSkill(skillId); | ||
| toast({ | ||
| title: 'Success', | ||
| description: `Skill "${skillToDelete.metadata.name}" deleted successfully`, | ||
| }); | ||
| await loadSkills(); | ||
| } catch (error) { | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: 'Failed to delete skill', | ||
| description: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| } finally { | ||
| setDeleteConfirmVisible(false); | ||
| setSkillToDelete(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Same parseInt issue with skill ID parsing.
Lines 83 and 104 use the same parseInt(skill.metadata.labels?.id || '0') pattern that could silently fail with skillId = 0 if the ID is missing.
Consider extracting a helper function to validate and parse the skill ID consistently:
const getSkillId = (skill: Skill): number | null => {
const idStr = skill.metadata.labels?.id;
if (!idStr) return null;
const id = parseInt(idStr, 10);
return isNaN(id) ? null : id;
};Then validate before operations:
const skillId = getSkillId(skillToDelete);
if (skillId === null) {
toast({ variant: 'destructive', title: 'Invalid skill ID' });
return;
}🤖 Prompt for AI Agents
In frontend/src/features/settings/components/skills/SkillList.tsx around lines
79 to 100, the code uses parseInt(skill.metadata.labels?.id || '0') which can
silently produce 0 when the ID is missing or invalid; extract a helper (e.g.,
getSkillId) that reads the label, returns null if missing, parses with
parseInt(..., 10), and returns null for NaN, then replace the current parseInt
usage with this helper and validate the result before calling deleteSkill: if
null, show a destructive toast "Invalid skill ID" and return; apply the same
helper/validation wherever skill IDs are parsed in this component to avoid
silent fallback to 0.
| export default function SkillUploadModal({ open, onClose, skill }: SkillUploadModalProps) { | ||
| const [skillName, setSkillName] = useState(skill?.metadata.name || ''); | ||
| const [namespace] = useState('default'); | ||
| const [selectedFile, setSelectedFile] = useState<File | null>(null); | ||
| const [uploading, setUploading] = useState(false); | ||
| const [uploadProgress, setUploadProgress] = useState(0); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [dragActive, setDragActive] = useState(false); | ||
|
|
||
| const isEditMode = !!skill; |
There was a problem hiding this comment.
State is not reset when modal opens or when editing different skills.
When the modal opens in create mode after previously editing a skill, or when editing different skills consecutively, stale state (skillName, selectedFile, error, uploadProgress) may persist from the previous session.
Consider resetting state when open or skill changes:
+import { useState, useCallback, useEffect } from 'react';
...
export default function SkillUploadModal({ open, onClose, skill }: SkillUploadModalProps) {
const [skillName, setSkillName] = useState(skill?.metadata.name || '');
const [namespace] = useState('default');
const [selectedFile, setSelectedFile] = useState<File | null>(null);
const [uploading, setUploading] = useState(false);
const [uploadProgress, setUploadProgress] = useState(0);
const [error, setError] = useState<string | null>(null);
const [dragActive, setDragActive] = useState(false);
+ // Reset state when modal opens or skill changes
+ useEffect(() => {
+ if (open) {
+ setSkillName(skill?.metadata.name || '');
+ setSelectedFile(null);
+ setUploadProgress(0);
+ setError(null);
+ setDragActive(false);
+ }
+ }, [open, skill]);📝 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.
| export default function SkillUploadModal({ open, onClose, skill }: SkillUploadModalProps) { | |
| const [skillName, setSkillName] = useState(skill?.metadata.name || ''); | |
| const [namespace] = useState('default'); | |
| const [selectedFile, setSelectedFile] = useState<File | null>(null); | |
| const [uploading, setUploading] = useState(false); | |
| const [uploadProgress, setUploadProgress] = useState(0); | |
| const [error, setError] = useState<string | null>(null); | |
| const [dragActive, setDragActive] = useState(false); | |
| const isEditMode = !!skill; | |
| import { useState, useCallback, useEffect } from 'react' | |
| export default function SkillUploadModal({ open, onClose, skill }: SkillUploadModalProps) { | |
| const [skillName, setSkillName] = useState(skill?.metadata.name || '') | |
| const [namespace] = useState('default') | |
| const [selectedFile, setSelectedFile] = useState<File | null>(null) | |
| const [uploading, setUploading] = useState(false) | |
| const [uploadProgress, setUploadProgress] = useState(0) | |
| const [error, setError] = useState<string | null>(null) | |
| const [dragActive, setDragActive] = useState(false) | |
| // Reset state when modal opens or skill changes | |
| useEffect(() => { | |
| if (open) { | |
| setSkillName(skill?.metadata.name || '') | |
| setSelectedFile(null) | |
| setUploadProgress(0) | |
| setError(null) | |
| setDragActive(false) | |
| } | |
| }, [open, skill]) | |
| const isEditMode = !!skill |
🤖 Prompt for AI Agents
In frontend/src/features/settings/components/skills/SkillUploadModal.tsx around
lines 33 to 42, component state (skillName, selectedFile, error, uploadProgress,
uploading, dragActive) is not reset when the modal opens or when a different
skill is passed in, causing stale values to persist; add a useEffect that runs
when either open or skill changes and resets state: set skillName to
skill?.metadata.name || '', set selectedFile to null, set error to null, set
uploadProgress to 0, set uploading to false, and set dragActive to false (leave
namespace as-is), ensuring the modal shows fresh state for create/edit sessions.
| try { | ||
| if (isEditMode && skill) { | ||
| const skillId = parseInt(skill.metadata.labels?.id || '0'); | ||
| await updateSkill(skillId, selectedFile, setUploadProgress); | ||
| } else { | ||
| await uploadSkill(selectedFile, skillName.trim(), namespace, setUploadProgress); | ||
| } |
There was a problem hiding this comment.
Potential silent failure when skill ID is missing.
If skill.metadata.labels?.id is undefined or not a valid number, parseInt(...|| '0') will result in skillId = 0, which would likely cause an API error or unexpected behavior. Consider validating the ID before proceeding.
if (isEditMode && skill) {
- const skillId = parseInt(skill.metadata.labels?.id || '0');
+ const skillIdStr = skill.metadata.labels?.id;
+ if (!skillIdStr) {
+ setError('Skill ID is missing');
+ return;
+ }
+ const skillId = parseInt(skillIdStr, 10);
+ if (isNaN(skillId)) {
+ setError('Invalid skill ID');
+ return;
+ }
await updateSkill(skillId, selectedFile, setUploadProgress);🤖 Prompt for AI Agents
In frontend/src/features/settings/components/skills/SkillUploadModal.tsx around
lines 115 to 121, the code unconditionally parses skill.metadata.labels?.id
using parseInt(... || '0') which can silently produce 0 for missing/invalid IDs
and lead to incorrect API calls; instead validate that skill.metadata.labels?.id
exists and is a numeric string (e.g., use Number.isFinite(Number(idString)) or
/^\d+$/.test(idString)) before calling updateSkill, and if invalid, abort the
update branch by showing a user-facing error/toast and return early (or fall
back to upload flow only if appropriate) to avoid sending an incorrect skillId
to the API.
- Create managing-skills.md user guide in English and Chinese - Cover complete workflow: creating, uploading, managing, and using Skills - Document ZIP package requirements and SKILL.md format - Provide step-by-step instructions with screenshots descriptions - Include best practices and common troubleshooting - Add quick start example with code samples - Update main README.md files to include Skills guide link Content: - What is a Skill and when to use it - Creating Skill packages (directory structure, YAML frontmatter) - Uploading Skills via Web UI with validation requirements - Managing Skills (view, download, update, delete) - Using Skills in Bots (association, deployment, YAML config) - Best practices (naming, versioning, security, size optimization) - Common issues and solutions (upload failures, deletion blocks, deployment problems) - Quick start example: hello-skill walkthrough - Related resources and external links
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/en/guides/user/managing-skills.md (1)
83-83: Add language specifiers to fenced code blocks.Three code blocks lack language specifiers, which is a best practice for readability and syntax highlighting. While these are directory trees (plain text), explicitly marking them helps documentation tools process them correctly.
Apply this diff to add language specifiers:
### Directory Structure Example - ``` + ```text my-skill.zip ├── SKILL.md # Required: Documentation with frontmatter ├── main.py # Your skill code ├── config.json # Configuration file (optional) ├── utils/ │ ├── helper.py │ └── formatter.py └── README.md # Additional docs (optional) - ``` + ``` ### Creating the ZIP Package **Using command line:** - ```bash + ```bash cd my-skill-directory zip -r my-skill.zip . - ``` + ```And at line 237:
**Deployment path example**: - ``` + ```text ~/.claude/skills/ ├── python-debugger/ │ ├── SKILL.md │ ├── main.py │ └── utils/ ├── code-formatter/ │ ├── SKILL.md │ └── formatter.py └── api-tester/ ├── SKILL.md └── test_runner.py - ``` + ``` ---And at line 382:
Create a directory structure: - ``` + ```text hello-skill/ ├── SKILL.md └── hello.py - ``` + ```Also applies to: 237-237, 382-382
docs/zh/guides/user/managing-skills.md (1)
83-83: Add language specifiers to fenced code blocks.Same issue as the English version: three code blocks (directory trees) lack language specifiers. Add
textspecifier for consistency and documentation tool compatibility.Apply the same fixes as the English version at these line ranges. Example for line 83:
### 目录结构示例 - ``` + ```text my-skill.zip ├── SKILL.md # 必需: 带 frontmatter 的文档 ├── main.py # 您的 Skill 代码 ├── config.json # 配置文件(可选) ├── utils/ │ ├── helper.py │ └── formatter.py └── README.md # 额外文档(可选) - ``` + ```Apply similar changes at lines 237 and 382.
Also applies to: 237-237, 382-382
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/en/README.md(1 hunks)docs/en/guides/user/managing-skills.md(1 hunks)docs/zh/README.md(1 hunks)docs/zh/guides/user/managing-skills.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/zh/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Applies to AGENTS.md : Edit AGENTS.md directly with clear, concise instructions when updating
Applied to files:
docs/en/README.md
🪛 LanguageTool
docs/zh/guides/user/managing-skills.md
[uncategorized] ~330-~330: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：正确"地"缩
Context: ...ML 语法 - ✅ 确保 description 字段存在 - ✅ 检查正确的缩进 问题: "文件大小超过 10MB" - ✅ 删除不必要的文件 -...
(wb4)
🪛 markdownlint-cli2 (0.18.1)
docs/en/guides/user/managing-skills.md
11-11: Link fragments should be valid
(MD051, link-fragments)
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
382-382: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/zh/guides/user/managing-skills.md
11-11: Link fragments should be valid
(MD051, link-fragments)
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
382-382: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
docs/en/guides/user/managing-skills.md (2)
9-16: Verify table of contents links match section headers.The TOC links use lowercase with hyphens (e.g.,
#-what-is-a-skill), but section headers include emoji characters. Verify that the link fragments resolve correctly; markdown renderers typically strip emoji and convert titles to slugs, but manual validation is recommended to ensure TOC navigation works.
1-447: Content is comprehensive and well-aligned with feature implementation.The guide covers the full skill lifecycle (create, package, upload, manage, use, troubleshoot) with clear examples and best practices. Formatting and structure support good user experience. Once link fragments and code block language specifiers are addressed, this guide is ready for publication.
docs/en/README.md (1)
34-34: Entry placement and formatting are correct.The "Managing Skills" entry is properly positioned in the user guides, follows the established style, and references the new guide document with an appropriate description.
docs/zh/guides/user/managing-skills.md (2)
9-16: Verify table of contents links match section headers.Same consideration as the English version: TOC links with lowercase and hyphens (e.g.,
#-什么是-skill) need validation against actual section headers that include emoji characters. Confirm link fragments resolve correctly in markdown renderers.
1-447: Chinese translation is faithful and comprehensive.The guide effectively translates the English version with appropriate terminology and maintains all technical accuracy (API endpoints, file paths, feature references). Structure and formatting are consistent with the English guide. Once code block language specifiers and link fragments are addressed, this guide is publication-ready.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
executor/agents/claude_code/claude_code_agent.py (1)
1166-1274: Harden skill ZIP handling (Zip Slip), shorten method, and tidy logging/exception handling.This new method is the core of skills deployment; a few points need attention:
Zip Slip protection is still bypassable (critical).
The current checkfile_info.filename.startswith('/') or '..' in file_info.filenamecan miss crafted traversal paths (encodings, nested segments, etc.). This matches a previous review comment and should be upgraded to a realpath-based check.You can do:
- with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: - # Security check: prevent Zip Slip - for file_info in zip_file.filelist: - if file_info.filename.startswith('/') or '..' in file_info.filename: - logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") - shutil.rmtree(skill_target_dir) - break - else: - zip_file.extractall(skill_target_dir) - logger.info(f"Deployed skill '{skill_name}' to {skill_target_dir}") - success_count += 1 + with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: + # Security check: prevent Zip Slip using normalized paths + for file_info in zip_file.filelist: + target_path = os.path.realpath( + os.path.join(skill_target_dir, file_info.filename) + ) + base_dir = os.path.realpath(skill_target_dir) + if not target_path.startswith(base_dir + os.sep): + logger.error( + "Unsafe file path in skill ZIP: %s", + file_info.filename, + ) + shutil.rmtree(skill_target_dir) + break + else: + zip_file.extractall(skill_target_dir) + logger.info( + "Deployed skill '%s' to %s", + skill_name, + skill_target_dir, + ) + success_count += 1
Clean up logging and broad
Exceptionhandlers.
- Line 1201:
logger.info(f"Auth token available for skills download")doesn’t interpolate anything; drop thef:- logger.info(f"Auth token available for skills download") + logger.info("Auth token available for skills download")
- Lines 1262–1263 and 1271–1272: you’re catching
Exceptionand then interpolatingstr(e)into f-strings. Preferlogger.exception(captures stack) and avoid explicitstr(e):- except Exception as e: - logger.warning(f"Failed to download skill '{skill_name}': {str(e)}") + except Exception: + logger.exception( + "Failed to download skill '%s'", skill_name + ) continue @@ - except Exception as e: - logger.error(f"Error in _download_and_deploy_skills: {str(e)}") - # Don't raise - skills deployment failure shouldn't block task execution + except Exception: + logger.exception("Error in _download_and_deploy_skills") + # Don't raise - skills deployment failure shouldn't block task executionBroad
Exceptionis defensible here because you explicitly don’t want skills deployment to block tasks, but these logging tweaks make post‑mortems much easier and also satisfy the Ruff hints.
Optional: robustness and structure.
- The method currently does directory prep, API URL composition, querying, download, and extraction in a single ~100‑line function. For maintainability and to align with your “functions < 50 lines” guideline, consider splitting into helpers like
_prepare_skills_dir(),_fetch_skill_metadata(), and_download_and_extract_skill().- To be more robust to unusual skill names, you might use
params={"name": skill_name}instead of embedding the name directly inlist_url.Overall behavior is solid (sequential downloads, non‑blocking failures, clean deployment location); addressing the Zip Slip check should be treated as high priority, the rest as incremental refinements.
🧹 Nitpick comments (4)
executor_manager/executors/docker/executor.py (1)
262-267: Implementation is correct and follows established patterns.The method correctly propagates the
TASK_API_DOMAINenvironment variable to Docker containers, following the same pattern as_add_workspace_mountand_add_network_config. The conditional logic and debug logging are appropriate.Consider enhancing the docstring to provide more context:
- def _add_task_api_domain(self, cmd: List[str]) -> None: - """Add TASK_API_DOMAIN environment variable for executor to access backend API""" + def _add_task_api_domain(self, cmd: List[str]) -> None: + """ + Add TASK_API_DOMAIN environment variable to enable executor access to backend API. + + Used by agents (e.g., ClaudeCodeAgent) to download and manage skills at runtime. + """backend/app/services/adapters/executor_kinds.py (2)
339-349: Skills propagation into bot payload looks correct; watch log volume.The way you pull
ghost_crd.spec.skills or []and thread it through into thebotspayload is consistent with the rest of the CRD-based design and should be easy for executors to consume. The extralogger.infoper bot/ghost pair is helpful for debugging, but in high-traffic environments it might be worth downgrading todebuglater if logs get noisy.Also applies to: 387-395
630-631: Clarifyand/orprecedence in auto-delete condition.The long boolean in this
ifmixes severalandconditions and a list membership check. It’s currently correct, but adding parentheses around the entire metadata-labels part will make the intent clearer and silence the RuffRUF021hint.For example:
- if (task_crd.metadata and task_crd.metadata.labels and task_crd.metadata.labels.get("autoDeleteExecutor") == "true" and - task_crd.status and task_crd.status.status in ["COMPLETED", "FAILED"]): + if ( + task_crd.metadata + and task_crd.metadata.labels + and task_crd.metadata.labels.get("autoDeleteExecutor") == "true" + and task_crd.status + and task_crd.status.status in ["COMPLETED", "FAILED"] + ):executor/agents/claude_code/claude_code_agent.py (1)
1163-1164: Improve error logging when updating.git/info/exclude.Catching a broad
Exceptionhere makes sense to avoid breaking execution if git internals aren’t present, but usinglogger.exceptionwill give you a stack trace for debugging without changing behavior:- except Exception as e: - logger.warning(f"Failed to add '{pattern}' to .git/info/exclude: {e}") + except Exception: + logger.exception( + "Failed to add '%s' to .git/info/exclude", pattern + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
backend/app/services/adapters/executor_kinds.py(3 hunks)executor/agents/claude_code/claude_code_agent.py(2 hunks)executor_manager/executors/docker/executor.py(2 hunks)frontend/package.json(1 hunks)frontend/src/components/ui/progress.tsx(1 hunks)frontend/src/features/settings/components/skills/SkillUploadModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/features/settings/components/skills/SkillUploadModal.tsx
🧰 Additional context used
📓 Path-based instructions (3)
frontend/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend: Only use
NEXT_PUBLIC_*for client-safe environment variables
Files:
frontend/package.jsonfrontend/src/components/ui/progress.tsx
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant with Black formatter (line length: 88)
Use isort for import organization in Python
Python functions and variables must have descriptive names
Docstrings required for all public Python functions and classes
Extract magic numbers to named constants in Python
Python functions must not exceed 50 lines
Type hints required for all Python function signatures
Run pylint and flake8 for Python code quality checks
Files:
executor_manager/executors/docker/executor.pybackend/app/services/adapters/executor_kinds.pyexecutor/agents/claude_code/claude_code_agent.py
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: TypeScript strict mode must be enabled in Frontend
Use functional components with hooks in React
Use Prettier formatter in Frontend with single quotes and no semicolons
Use ESLint with Next.js configuration in Frontend
Useconstoverlet, never usevarin TypeScript
Component names must use PascalCase; file names must use kebab-case in Frontend
Use mobile-first responsive design approach in Frontend components
Frontend design: Low saturation + low contrast for reduced eye strain with generous whitespace
Frontend design: Minimal shadows, subtle component differentiation (<10% background variance)
Frontend design: Use mint blue (#14B8A6) as primary accent sparingly
Use Tailwind CSS variables for colors: --color-bg-base, --color-text-primary, --color-border, etc.
Frontend spacing: Use standard units - p-2 (8px), p-4 (16px), p-6 (24px), gap-3 (12px)
Frontend border-radius: rounded-2xl (16px) for large containers, rounded-lg (12px) for cards, rounded-md (6px) for buttons
Frontend typography: H1 text-xl font-semibold, H2 text-lg font-semibold, H3 text-base font-medium, Body text-sm
Use shadcn/ui Button component with variants: default, secondary, ghost, outline, link
Use shadcn/ui Form component with react-hook-form + zod validation
Files:
frontend/src/components/ui/progress.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Applies to frontend/**/*.{ts,tsx} : Use functional components with hooks in React
Applied to files:
frontend/src/components/ui/progress.tsx
📚 Learning: 2025-11-26T07:34:32.573Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:34:32.573Z
Learning: Use Kubernetes-inspired CRD design (Ghost, Model, Shell, Bot, Team, Task)
Applied to files:
backend/app/services/adapters/executor_kinds.py
🧬 Code graph analysis (2)
frontend/src/components/ui/progress.tsx (1)
frontend/src/lib/utils.ts (1)
cn(8-10)
backend/app/services/adapters/executor_kinds.py (2)
backend/app/schemas/kind.py (1)
Ghost(41-47)backend/app/core/security.py (1)
create_access_token(77-100)
🪛 Ruff (0.14.6)
backend/app/services/adapters/executor_kinds.py
398-398: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
415-415: Do not catch blind exception: Exception
(BLE001)
executor/agents/claude_code/claude_code_agent.py
1163-1163: Do not catch blind exception: Exception
(BLE001)
1201-1201: f-string without any placeholders
Remove extraneous f prefix
(F541)
1262-1262: Do not catch blind exception: Exception
(BLE001)
1263-1263: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1271-1271: Do not catch blind exception: Exception
(BLE001)
1272-1272: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1272-1272: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (5)
executor_manager/executors/docker/executor.py (1)
240-242: LGTM! Clean integration following established patterns.The integration of
_add_task_api_domainfollows the same helper method pattern used for workspace mounts, network config, and callback URLs. The comment clearly explains the purpose.frontend/src/components/ui/progress.tsx (1)
15-18: LGTM! Transform logic correctly implements progress animation.The transform calculation
translateX(-${100 - (value || 0)}%)properly animates the progress indicator from left to right, and the fallback to 0 safely handles undefined/null values.frontend/package.json (1)
25-25: Version 1.1.8 may not be available on the official npm registry.The npm package page shows the latest published version as 1.1.7 (as of November 27, 2025), while version 1.1.8 appears on jsDelivr but not as the latest on npmjs.com. This discrepancy suggests the version may not be published or may be unpublished. No security vulnerabilities were found for v1.1.8 itself, but the version availability should be verified before merging. Consider using the latest stable version (1.1.7) or confirming that 1.1.8 exists and is intentionally specified.
executor/agents/claude_code/claude_code_agent.py (1)
442-444: Hooking skills deployment into initialization is reasonable.Calling
_download_and_deploy_skills(bot_config)at the end ofinitialize()keeps skills setup co-located with other Claude Code configuration, and failures are already contained so they won’t derail task startup.backend/app/services/adapters/executor_kinds.py (1)
400-417: I'll help you verify and rewrite this review comment. Let me start by examining the file to validate the concerns raised.
<function_calls>
#!/bin/bashFirst, let's check the file size and get an overview
wc -l backend/app/services/adapters/executor_kinds.py
</function_calls>
<function_calls>
#!/bin/bashRead the file to see imports and the code section in question
head -30 backend/app/services/adapters/executor_kinds.py
</function_calls>
<function_calls>
#!/bin/bashGet lines 395-425 to see the full context of the target lines
sed -n '395,425p' backend/app/services/adapters/executor_kinds.py | cat -n
</function_calls>
<function_calls>
#!/bin/bashCheck line 446 context
sed -n '440,450p' backend/app/services/adapters/executor_kinds.py | cat -n
</function_calls>
<function_calls>
#!/bin/bashCheck if settings is imported at the top
grep -n "from app.core.config import settings" backend/app/services/adapters/executor_kinds.py | head -10
</function_calls>
<function_calls>
#!/bin/bashCheck all imports from app.core at the top of the file
head -50 backend/app/services/adapters/executor_kinds.py | grep "import"
</function_calls>Let me get a better view of the full import section:
<function_calls>
#!/bin/bashGet the complete import section
sed -n '1,80p' backend/app/services/adapters/executor_kinds.py | cat -n
</function_calls>Now let me check where
settingsis actually used in the file:
<function_calls>
#!/bin/bashSearch for usage of 'settings' throughout the file
rg -n "settings" backend/app/services/adapters/executor_kinds.py
</function_calls>Let me also verify the logger and exception handling patterns:
<function_calls>
#!/bin/bashCheck how logger is defined and used in the file
rg -n "logger" backend/app/services/adapters/executor_kinds.py | head -20
</function_calls>
| import * as React from 'react'; | ||
| import * as ProgressPrimitive from '@radix-ui/react-progress'; | ||
|
|
||
| import { cn } from '@/lib/utils'; | ||
|
|
||
| const Progress = React.forwardRef< | ||
| React.ElementRef<typeof ProgressPrimitive.Root>, | ||
| React.ComponentPropsWithoutRef<typeof ProgressPrimitive.Root> | ||
| >(({ className, value, ...props }, ref) => ( | ||
| <ProgressPrimitive.Root | ||
| ref={ref} | ||
| className={cn('relative h-4 w-full overflow-hidden rounded-full bg-secondary', className)} | ||
| {...props} | ||
| > | ||
| <ProgressPrimitive.Indicator | ||
| className="h-full w-full flex-1 bg-primary transition-all" | ||
| style={{ transform: `translateX(-${100 - (value || 0)}%)` }} | ||
| /> | ||
| </ProgressPrimitive.Root> | ||
| )); | ||
| Progress.displayName = ProgressPrimitive.Root.displayName; | ||
|
|
||
| export { Progress }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove semicolons to comply with Prettier configuration.
The code includes semicolons on lines 1, 2, 4, 21, and 23, which violates the coding guideline requiring Prettier formatting with no semicolons.
As per coding guidelines, Prettier should be configured with single quotes and no semicolons in the Frontend.
Apply this diff to remove semicolons:
-import * as React from 'react';
-import * as ProgressPrimitive from '@radix-ui/react-progress';
+import * as React from 'react'
+import * as ProgressPrimitive from '@radix-ui/react-progress'
-import { cn } from '@/lib/utils';
+import { cn } from '@/lib/utils'
const Progress = React.forwardRef<
React.ElementRef<typeof ProgressPrimitive.Root>,
React.ComponentPropsWithoutRef<typeof ProgressPrimitive.Root>
>(({ className, value, ...props }, ref) => (
<ProgressPrimitive.Root
ref={ref}
className={cn('relative h-4 w-full overflow-hidden rounded-full bg-secondary', className)}
{...props}
>
<ProgressPrimitive.Indicator
className="h-full w-full flex-1 bg-primary transition-all"
style={{ transform: `translateX(-${100 - (value || 0)}%)` }}
/>
</ProgressPrimitive.Root>
-));
-Progress.displayName = ProgressPrimitive.Root.displayName;
+))
+Progress.displayName = ProgressPrimitive.Root.displayName
-export { Progress };
+export { Progress }📝 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 * as React from 'react'; | |
| import * as ProgressPrimitive from '@radix-ui/react-progress'; | |
| import { cn } from '@/lib/utils'; | |
| const Progress = React.forwardRef< | |
| React.ElementRef<typeof ProgressPrimitive.Root>, | |
| React.ComponentPropsWithoutRef<typeof ProgressPrimitive.Root> | |
| >(({ className, value, ...props }, ref) => ( | |
| <ProgressPrimitive.Root | |
| ref={ref} | |
| className={cn('relative h-4 w-full overflow-hidden rounded-full bg-secondary', className)} | |
| {...props} | |
| > | |
| <ProgressPrimitive.Indicator | |
| className="h-full w-full flex-1 bg-primary transition-all" | |
| style={{ transform: `translateX(-${100 - (value || 0)}%)` }} | |
| /> | |
| </ProgressPrimitive.Root> | |
| )); | |
| Progress.displayName = ProgressPrimitive.Root.displayName; | |
| export { Progress }; | |
| import * as React from 'react' | |
| import * as ProgressPrimitive from '@radix-ui/react-progress' | |
| import { cn } from '@/lib/utils' | |
| const Progress = React.forwardRef< | |
| React.ElementRef<typeof ProgressPrimitive.Root>, | |
| React.ComponentPropsWithoutRef<typeof ProgressPrimitive.Root> | |
| >(({ className, value, ...props }, ref) => ( | |
| <ProgressPrimitive.Root | |
| ref={ref} | |
| className={cn('relative h-4 w-full overflow-hidden rounded-full bg-secondary', className)} | |
| {...props} | |
| > | |
| <ProgressPrimitive.Indicator | |
| className="h-full w-full flex-1 bg-primary transition-all" | |
| style={{ transform: `translateX(-${100 - (value || 0)}%)` }} | |
| /> | |
| </ProgressPrimitive.Root> | |
| )) | |
| Progress.displayName = ProgressPrimitive.Root.displayName | |
| export { Progress } |
🤖 Prompt for AI Agents
In frontend/src/components/ui/progress.tsx around lines 1 to 23, the file
contains semicolons that violate the project's Prettier rules (no semicolons);
remove the trailing semicolons at the ends of import lines, the cn import line,
the forwardRef closing line, and the export line so the file uses no semicolons,
then run Prettier (or save with the project's formatter) to ensure single quotes
and no-semicolon formatting is applied consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
executor/agents/claude_code/claude_code_agent.py (1)
1250-1260: Consider strengthening Zip Slip protection with path normalization.Similar to the backend validator, the current check
if file_info.filename.startswith('/') or '..' in file_info.filenamecan be bypassed with certain encodings or path variations.Use
os.path.realpathto ensure extracted files stay within the target directory:with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: # Security check: prevent Zip Slip attacks for file_info in zip_file.filelist: - if file_info.filename.startswith('/') or '..' in file_info.filename: + # Normalize and verify path stays within target directory + target_path = os.path.realpath( + os.path.join(skills_dir, file_info.filename) + ) + if not target_path.startswith(os.path.realpath(skills_dir) + os.sep): logger.error(f"Unsafe file path in skill ZIP: {file_info.filename}") break else: zip_file.extractall(skills_dir)This catches encoded or obfuscated path traversal attempts as suggested in the past review comments.
frontend/src/features/settings/components/skills/SkillUploadModal.tsx (2)
34-42: State is not reset when modal opens or skill changes.When switching between create/edit modes or editing different skills consecutively, stale state (skillName, selectedFile, error, uploadProgress, dragActive) may persist from previous sessions. This can confuse users and cause incorrect behavior.
Add a useEffect to reset state when modal opens or skill changes:
+import { useState, useCallback, useEffect } from 'react'; ... export default function SkillUploadModal({ open, onClose, skill }: SkillUploadModalProps) { const { t } = useTranslation('common'); const [skillName, setSkillName] = useState(skill?.metadata.name || ''); const [namespace] = useState('default'); const [selectedFile, setSelectedFile] = useState<File | null>(null); const [uploading, setUploading] = useState(false); const [uploadProgress, setUploadProgress] = useState(0); const [error, setError] = useState<string | null>(null); const [dragActive, setDragActive] = useState(false); + // Reset state when modal opens or skill changes + useEffect(() => { + if (open) { + setSkillName(skill?.metadata.name || ''); + setSelectedFile(null); + setUploadProgress(0); + setError(null); + setDragActive(false); + } + }, [open, skill]);
123-129: Validate skill ID before API call to prevent silent failures.If
skill.metadata.labels?.idis undefined or invalid,parseInt(...|| '0')producesskillId = 0, which would likely cause an API error or unexpected behavior. The error wouldn't be user-friendly.Validate the ID explicitly before calling updateSkill:
if (isEditMode && skill) { - const skillId = parseInt(skill.metadata.labels?.id || '0'); + const skillIdStr = skill.metadata.labels?.id; + if (!skillIdStr) { + setError('Skill ID is missing'); + return; + } + const skillId = parseInt(skillIdStr, 10); + if (isNaN(skillId)) { + setError('Invalid skill ID'); + return; + } await updateSkill(skillId, selectedFile, setUploadProgress);frontend/src/features/settings/components/skills/SkillList.tsx (1)
74-95: Validate skill IDs before operations to prevent silent failures.Lines 78 and 99 use
parseInt(skill.metadata.labels?.id || '0')which can silently produceskillId = 0when the ID is missing or invalid. This could lead to incorrect API calls or confusing errors.Extract a helper function to validate and parse skill IDs consistently:
const getSkillId = (skill: Skill): number | null => { const idStr = skill.metadata.labels?.id; if (!idStr) return null; const id = parseInt(idStr, 10); return isNaN(id) ? null : id; };Then use it in both handlers:
const handleConfirmDelete = async () => { if (!skillToDelete) return; try { - const skillId = parseInt(skillToDelete.metadata.labels?.id || '0'); + const skillId = getSkillId(skillToDelete); + if (skillId === null) { + toast({ variant: 'destructive', title: 'Invalid skill ID' }); + return; + } await deleteSkill(skillId); // ... } }; const handleDownloadSkill = async (skill: Skill) => { try { - const skillId = parseInt(skill.metadata.labels?.id || '0'); + const skillId = getSkillId(skill); + if (skillId === null) { + toast({ variant: 'destructive', title: 'Invalid skill ID' }); + return; + } await downloadSkill(skillId, skill.metadata.name); // ... } };Also applies to: 97-112
backend/app/api/endpoints/kind/skills.py (1)
161-167: Sanitize filename in Content-Disposition header (existing issue).This issue was flagged in a previous review but appears unaddressed. Special characters in
skill.metadata.name(spaces, quotes, non-ASCII) can cause malformed headers.backend/app/services/adapters/skill_kinds.py (1)
205-217: Addflag_modifiedfor JSON mutation consistency (existing issue).This was flagged in a previous review. The codebase consistently uses
flag_modified(entity, "json")after JSON mutations to ensure SQLAlchemy tracks the change. While the reassignment at line 217 may work, it's inconsistent with the established pattern.
🧹 Nitpick comments (10)
backend/app/services/skill_service.py (3)
64-70: Consider strengthening Zip Slip protection with path normalization.The current check
if file_info.filename.startswith('/') or '..' in file_info.filenameis a good baseline, but can be bypassed with certain path encodings or variations (e.g., URL-encoded paths, mixed separators).Consider using
os.path.realpathto normalize paths before extraction:# Security check: prevent Zip Slip attacks for file_info in zip_file.filelist: - if file_info.filename.startswith('/') or '..' in file_info.filename: + # Normalize path to catch encoded traversals + normalized = os.path.normpath(file_info.filename) + if normalized.startswith('/') or normalized.startswith('..') or os.path.isabs(normalized): raise HTTPException( status_code=400, detail=f"Unsafe file path detected in ZIP: {file_info.filename}" )Alternatively, after extraction, verify each extracted file's real path stays within the target directory as suggested in the past review comments.
119-130: Exception chaining could improve debugging.The catch-all exception handler re-raises
HTTPExceptionbut doesn't chain other exceptions. Addingfrom ewould preserve the original stack trace for debugging.Apply this change to improve exception context:
except zipfile.BadZipFile: raise HTTPException( status_code=400, detail="Corrupted ZIP file" - ) + ) from None except Exception as e: if isinstance(e, HTTPException): raise raise HTTPException( status_code=400, detail=f"Failed to process ZIP file: {str(e)}" - ) + ) from eThis aligns with Ruff's B904 suggestion while maintaining clean user-facing error messages.
177-188: Similar exception chaining improvement for YAML parsing.The YAML parsing error handler would also benefit from exception chaining for better debugging.
except yaml.YAMLError as e: raise HTTPException( status_code=400, detail=f"Invalid YAML frontmatter in SKILL.md: {str(e)}" - ) + ) from e except Exception as e: if isinstance(e, HTTPException): raise raise HTTPException( status_code=400, detail=f"Failed to parse SKILL.md frontmatter: {str(e)}" - ) + ) from eexecutor/agents/claude_code/claude_code_agent.py (1)
1269-1280: Uselogger.exceptionfor better error context.Line 1279 uses
logger.errorin an exception handler. Usinglogger.exceptionwould automatically include the stack trace, making debugging easier.except Exception as e: - logger.error(f"Error in _download_and_deploy_skills: {str(e)}") + logger.exception("Error in _download_and_deploy_skills") # Don't raise - skills deployment failure shouldn't block task executionThis aligns with Ruff's TRY400 suggestion and provides better debugging information while keeping the same behavior.
backend/tests/services/test_skill_service.py (1)
221-244: Non-deterministic assertion doesn't verify "first is used" behavior.The test name claims "the first one is used" but the assertion accepts either value. ZIP file iteration order is implementation-dependent, so this test doesn't reliably verify the stated behavior. Consider either:
- Using a deterministic structure where order is guaranteed
- Documenting that behavior is undefined when multiple SKILL.md exist
- # Should use the first SKILL.md found - assert result["description"] in ["First skill", "Second skill"] - assert result["version"] in ["1.0.0", "2.0.0"] + # Behavior with multiple SKILL.md is implementation-dependent + # Just verify one valid result is returned + assert result["description"] in ["First skill", "Second skill"] + assert result["version"] is not NoneOr rename the test to
test_validate_zip_multiple_skill_md_uses_anyto accurately reflect the behavior.frontend/src/features/settings/components/skills/SkillManagementModal.tsx (2)
48-62: Missingtin useCallback dependency array.The
loadSkillscallback usestfor translations in the error toast but it's not included in the dependency array. This could cause stale translation values if the language changes while the component is mounted.- }, [toast]); + }, [toast, t]);
85-107: Defensive check for missing skill ID would improve error handling.The
parseInt(skillToDelete.metadata.labels?.id || '0')fallback returns 0 when ID is missing, which would result in a confusing 404 error. Consider adding an explicit check:const handleConfirmDelete = async () => { if (!skillToDelete) return; + const skillIdStr = skillToDelete.metadata.labels?.id; + if (!skillIdStr) { + toast({ + variant: 'destructive', + title: t('skills.failed_delete'), + description: t('skills.invalid_skill_id'), + }); + return; + } + try { - const skillId = parseInt(skillToDelete.metadata.labels?.id || '0'); + const skillId = parseInt(skillIdStr, 10); await deleteSkill(skillId);Note: Also add radix parameter
10toparseIntfor explicit base-10 parsing.backend/app/services/adapters/skill_kinds.py (3)
8-11: Remove unused importand_.The
and_import from SQLAlchemy is declared but never used in this file.from typing import List, Optional, Dict, Any from fastapi import HTTPException from sqlalchemy.orm import Session -from sqlalchemy import and_
269-280: Consider database-level JSON query for Ghost reference check at scale.The current implementation loads all user Ghosts into memory to check skill references. For users with many Ghosts, this could be inefficient. A database-level JSON query (e.g., PostgreSQL's
@>operator or MySQL'sJSON_CONTAINS) would be more scalable.This is acceptable for current scale but worth revisiting if Ghost counts grow significantly.
159-165: Unusedtotalvariable - remove or use for pagination metadata.The
totalcount is computed but never used. Either remove it to satisfy linters, or include it in theSkillListresponse for pagination support.- total = query.count() skills = query.offset(skip).limit(limit).all() return SkillList( items=[self._kind_to_skill(skill) for skill in skills] )Or extend
SkillListschema to includetotalfor client-side pagination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/app/api/endpoints/kind/skills.py(1 hunks)backend/app/services/adapters/skill_kinds.py(1 hunks)backend/app/services/skill_service.py(1 hunks)backend/tests/api/test_skills_api.py(1 hunks)backend/tests/conftest.py(3 hunks)backend/tests/services/test_skill_kinds_service.py(1 hunks)backend/tests/services/test_skill_service.py(1 hunks)executor/agents/claude_code/claude_code_agent.py(2 hunks)frontend/src/apis/skills.ts(1 hunks)frontend/src/app/settings/page.tsx(0 hunks)frontend/src/features/settings/components/BotEdit.tsx(10 hunks)frontend/src/features/settings/components/skills/SkillList.tsx(1 hunks)frontend/src/features/settings/components/skills/SkillManagementModal.tsx(1 hunks)frontend/src/features/settings/components/skills/SkillUploadModal.tsx(1 hunks)frontend/src/i18n/locales/en/common.json(2 hunks)frontend/src/i18n/locales/zh-CN/common.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/app/settings/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/features/settings/components/BotEdit.tsx
- frontend/src/apis/skills.ts
🧰 Additional context used
📓 Path-based instructions (4)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: TypeScript strict mode must be enabled in Frontend
Use functional components with hooks in React
Use Prettier formatter in Frontend with single quotes and no semicolons
Use ESLint with Next.js configuration in Frontend
Useconstoverlet, never usevarin TypeScript
Component names must use PascalCase; file names must use kebab-case in Frontend
Use mobile-first responsive design approach in Frontend components
Frontend design: Low saturation + low contrast for reduced eye strain with generous whitespace
Frontend design: Minimal shadows, subtle component differentiation (<10% background variance)
Frontend design: Use mint blue (#14B8A6) as primary accent sparingly
Use Tailwind CSS variables for colors: --color-bg-base, --color-text-primary, --color-border, etc.
Frontend spacing: Use standard units - p-2 (8px), p-4 (16px), p-6 (24px), gap-3 (12px)
Frontend border-radius: rounded-2xl (16px) for large containers, rounded-lg (12px) for cards, rounded-md (6px) for buttons
Frontend typography: H1 text-xl font-semibold, H2 text-lg font-semibold, H3 text-base font-medium, Body text-sm
Use shadcn/ui Button component with variants: default, secondary, ghost, outline, link
Use shadcn/ui Form component with react-hook-form + zod validation
Files:
frontend/src/features/settings/components/skills/SkillList.tsxfrontend/src/features/settings/components/skills/SkillManagementModal.tsxfrontend/src/features/settings/components/skills/SkillUploadModal.tsx
frontend/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend: Only use
NEXT_PUBLIC_*for client-safe environment variables
Files:
frontend/src/features/settings/components/skills/SkillList.tsxfrontend/src/features/settings/components/skills/SkillManagementModal.tsxfrontend/src/features/settings/components/skills/SkillUploadModal.tsxfrontend/src/i18n/locales/en/common.jsonfrontend/src/i18n/locales/zh-CN/common.json
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be PEP 8 compliant with Black formatter (line length: 88)
Use isort for import organization in Python
Python functions and variables must have descriptive names
Docstrings required for all public Python functions and classes
Extract magic numbers to named constants in Python
Python functions must not exceed 50 lines
Type hints required for all Python function signatures
Run pylint and flake8 for Python code quality checks
Files:
executor/agents/claude_code/claude_code_agent.pybackend/tests/api/test_skills_api.pybackend/tests/conftest.pybackend/tests/services/test_skill_service.pybackend/app/services/skill_service.pybackend/app/api/endpoints/kind/skills.pybackend/tests/services/test_skill_kinds_service.pybackend/app/services/adapters/skill_kinds.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Follow AAA pattern in tests: Arrange, Act, Assert
Mock external services (Anthropic, OpenAI, Docker, APIs) in Python tests
Use descriptive test names explaining what is tested in Python tests
Test edge cases and error conditions in Python tests
Keep Python tests independent and isolated
Files:
backend/tests/api/test_skills_api.pybackend/tests/services/test_skill_service.pybackend/tests/services/test_skill_kinds_service.py
🧬 Code graph analysis (6)
frontend/src/features/settings/components/skills/SkillManagementModal.tsx (3)
frontend/src/apis/skills.ts (4)
fetchSkillsList(13-38)deleteSkill(203-222)downloadSkill(227-251)formatFileSize(256-261)frontend/src/components/common/UnifiedAddButton.tsx (1)
UnifiedAddButton(12-24)frontend/src/features/settings/components/skills/SkillUploadModal.tsx (1)
SkillUploadModal(34-277)
frontend/src/features/settings/components/skills/SkillUploadModal.tsx (8)
frontend/src/types/api.ts (1)
Skill(62-68)frontend/src/apis/skills.ts (2)
updateSkill(147-198)uploadSkill(87-142)frontend/src/components/ui/dialog.tsx (6)
Dialog(92-92)DialogContent(97-97)DialogHeader(98-98)DialogTitle(100-100)DialogDescription(101-101)DialogFooter(99-99)frontend/src/components/ui/label.tsx (1)
Label(21-21)frontend/src/components/ui/input.tsx (1)
Input(28-28)frontend/src/components/ui/progress.tsx (1)
Progress(23-23)frontend/src/components/ui/alert.tsx (2)
Alert(57-57)AlertDescription(57-57)frontend/src/components/ui/button.tsx (1)
Button(55-55)
backend/tests/api/test_skills_api.py (2)
backend/app/models/kind.py (1)
Kind(14-30)backend/tests/conftest.py (4)
test_token(128-132)test_db(29-54)test_user(74-88)test_admin_token(136-140)
backend/tests/conftest.py (2)
backend/app/models/kind.py (1)
Kind(14-30)backend/app/models/skill_binary.py (1)
SkillBinary(13-26)
backend/tests/services/test_skill_service.py (1)
backend/app/services/skill_service.py (2)
SkillValidator(17-188)validate_zip(23-130)
backend/app/api/endpoints/kind/skills.py (1)
backend/app/services/adapters/skill_kinds.py (7)
create_skill(22-112)list_skills(143-165)get_skill_by_name(128-141)get_skill_by_id(114-126)get_skill_binary(292-323)update_skill(167-241)delete_skill(243-290)
🪛 GitHub Actions: Tests
backend/tests/services/test_skill_service.py
[error] 95-95: Skill ZIP validation exceeded size limit; expected 413 but got 400 due to ZIP/SKILL.md structure check
backend/app/services/skill_service.py
[error] 94-102: SKILL.md not found in skill folder. Expected structure: skill-folder/SKILL.md
🪛 Ruff (0.14.6)
executor/agents/claude_code/claude_code_agent.py
1163-1163: Do not catch blind exception: Exception
(BLE001)
1201-1201: f-string without any placeholders
Remove extraneous f prefix
(F541)
1269-1269: Do not catch blind exception: Exception
(BLE001)
1270-1270: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1278-1278: Do not catch blind exception: Exception
(BLE001)
1279-1279: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1279-1279: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/tests/conftest.py
21-21: from app.models import * used; unable to detect undefined names
(F403)
backend/app/services/skill_service.py
67-70: Abstract raise to an inner function
(TRY301)
94-97: Abstract raise to an inner function
(TRY301)
102-105: Abstract raise to an inner function
(TRY301)
120-123: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
129-129: Use explicit conversion flag
Replace with conversion flag
(RUF010)
166-166: Prefer TypeError exception for invalid type
(TRY004)
166-166: Abstract raise to an inner function
(TRY301)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
170-173: Abstract raise to an inner function
(TRY301)
175-175: Consider moving this statement to an else block
(TRY300)
178-181: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
180-180: Use explicit conversion flag
Replace with conversion flag
(RUF010)
185-188: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/api/endpoints/kind/skills.py
24-24: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: 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)
28-28: 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)
85-85: 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)
86-86: 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)
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)
134-134: 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)
135-135: 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)
173-173: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
174-174: 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)
175-175: 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)
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)
backend/app/services/adapters/skill_kinds.py
55-55: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
120-120: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
135-135: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
157-157: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
160-160: Local variable total is assigned to but never used
Remove assignment to unused variable total
(F841)
196-196: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
261-261: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
273-273: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
309-309: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🔇 Additional comments (18)
backend/tests/conftest.py (3)
24-25: LGTM! Shared in-memory SQLite improves thread safety.The file-based in-memory URL with
cache=sharedenables multiple connections to share the same in-memory database, which is important for test reliability.
43-44: LGTM!expire_on_commit=Falseprevents lazy-loading issues.Setting
expire_on_commit=Falseis appropriate for tests because it prevents SQLAlchemy from expiring objects after commit, avoiding detached instance errors when accessing attributes outside the session context.
153-161: LGTM! Exception handling ensures rollback on errors.The test_client fixture correctly returns the same
test_dbsession across requests, and the exception handler ensures rollback on error. This prevents transaction leaks during test failures.frontend/src/i18n/locales/zh-CN/common.json (1)
24-25: LGTM! Comprehensive Chinese translations for Skill management.The translations cover all Skill management UI flows including upload, update, delete, download, validation messages, and error states. The structure mirrors the English localization file, ensuring consistency across languages.
Also applies to: 368-430
frontend/src/i18n/locales/en/common.json (1)
24-25: LGTM! Complete English translations for Skill management UI.All necessary translation keys are present for the Skill management feature, including modal texts, validation messages, error states, and UI labels. The structure is consistent and well-organized.
Also applies to: 367-429
backend/tests/services/test_skill_kinds_service.py (1)
19-473: Excellent comprehensive integration test suite!This test suite provides thorough coverage of SkillKindsService:
- ✅ CRUD operations (create, read, update, delete)
- ✅ Access control and multi-user isolation
- ✅ Pagination and filtering
- ✅ Duplicate handling
- ✅ Binary data retrieval
- ✅ Referential integrity (Ghost reference prevention)
The tests follow the AAA pattern, use descriptive names, and cover both happy paths and error conditions. The Ghost reference constraint test (lines 371-422) is particularly valuable for ensuring data integrity.
frontend/src/features/settings/components/skills/SkillUploadModal.tsx (1)
46-54: LGTM! File validation and submission logic are solid.The validation correctly enforces:
- ZIP file extension check
- 10MB size limit with user-friendly error messages
- Required field validation (file and name)
The submission flow properly differentiates between create (uploadSkill) and edit (updateSkill) modes, with progress tracking and error handling.
Also applies to: 108-136
frontend/src/features/settings/components/skills/SkillList.tsx (1)
122-270: LGTM! Well-structured component with good UX.The component provides:
- Clear loading and empty states
- Comprehensive skill information display (name, description, version, author, tags, file size, status)
- Intuitive action buttons (download, edit, delete)
- Proper modal and dialog management
- Success/error feedback via toasts
The UI follows the design guidelines with appropriate spacing, colors, and component usage.
executor/agents/claude_code/claude_code_agent.py (2)
1166-1202: LGTM! Graceful skill deployment with proper error handling.The method correctly:
- Extracts skills from bot_config (not spec)
- Clears existing skills directory before deployment
- Checks for auth token and exits gracefully if missing
- Logs appropriate messages for debugging
The decision to not block task execution on skill deployment failures is appropriate—skills are supplementary features.
1209-1271: LGTM! Robust per-skill download and deployment logic.The implementation correctly:
- Queries skills by name to get IDs
- Downloads skill ZIP packages with authentication
- Extracts ZIPs to the correct directory structure
- Includes Zip Slip protection
- Continues on per-skill failures without blocking the overall task
- Tracks and logs success/failure counts
The error handling ensures resilience while providing detailed logging for troubleshooting.
backend/tests/services/test_skill_service.py (2)
170-199: Good security test coverage for Zip Slip protection.The tests for path traversal (
../../../etc/passwd) and absolute paths (/etc/passwd) provide solid security validation. These are critical security tests that align with best practices.
51-56: I'll verify this review comment by examining the ZIP structure requirements in the SkillValidator. Let me start by investigating the validation logic and test implementation.
<function_calls>
#!/bin/bashFind and examine the validate_zip implementation
fd skill_service.py backend/app/services/ | head -5
</function_calls>
<function_calls>
#!/bin/bashExamine the validate_zip method in detail
rg -n "def validate_zip" backend/ -A 30
</function_calls>
<function_calls>
#!/bin/bashLook at the test file to understand the test structure and the create_test_zip helper
wc -l backend/tests/services/test_skill_service.py
</function_calls>Now let me examine the test file around lines 37-63 and 201-219:
<function_calls>
#!/bin/bashExamine the test_validate_zip_success test (lines 37-63)
sed -n '37,63p' backend/tests/services/test_skill_service.py
</function_calls>
<function_calls>
#!/bin/bashExamine the referenced test (lines 201-219)
sed -n '201,219p' backend/tests/services/test_skill_service.py
</function_calls>Now let me examine the create_test_zip helper method and understand how ZIP files are structured:
<function_calls>
#!/bin/bashFind the create_test_zip helper method
rg -n "def create_test_zip" backend/tests/services/test_skill_service.py -A 15
</function_calls>Let me also check the SkillValidator class implementation:
<function_calls>
#!/bin/bashFind SkillValidator class and its validate_zip method
rg -n "class SkillValidator" backend/ -A 5
</function_calls>
<function_calls>
#!/bin/bashGet the full validate_zip implementation with more context
rg -n "def validate_zip" backend/ -A 50
</function_calls>Let me check what files exist in the backend/app/services directory:
<function_calls>
#!/bin/bashList files in the services directory
fd . backend/app/services/ -type f
</function_calls>Now let me search more broadly for SkillValidator:
<function_calls>
#!/bin/bashSearch for SkillValidator across the backend
rg -n "SkillValidator" backend/ --type py -B 2 -A 10 | head -100
</function_calls>Let me look at the test file structure more carefully:
<function_calls>
#!/bin/bashDisplay the test file with line numbers to see the full context
cat -n backend/tests/services/test_skill_service.py | head -80
</function_calls><function_calls>
#!/bin/bashContinue from line 80 to around 220
sed -n '80,220p' backend/tests/services/test_skill_service.py | cat -n
</function_calls>Now let me search for where SkillValidator is imported or defined:
<function_calls>
#!/bin/bashSearch for SkillValidator import in the test file
head -30 backend/tests/services/test_skill_service.py
</function_calls><function_calls>
#!/bin/bashSearch for SkillValidator definition across entire backend
rg -n "class SkillValidator" backend/ -A 80 --type py
</function_calls>Let me check the project structure:
<function_calls>
#!/bin/bashFind all Python files in backend
fd . backend/ -name "*.py" -type f | grep -E "(skill|validator)" | head -20
</function_calls><function_calls>
#!/bin/bashList all files in backend/app
fd . backend/app/ -type f -name "*.py" 2>/dev/null
</function_calls>Let me try a different approach - search all Python files for validate_zip:
<function_calls>
#!/bin/bashSearch for validate_zip across all Python files
rg "validate_zip" --type py -n backend/
</function_calls><function_calls>
#!/bin/bashGet complete view of the test file
wc -l backend/tests/services/test_skill_service.py && echo "---" && cat backend/tests/services/test_skill_service.py
</function_calls><function_calls>
#!/bin/bashCheck if the validator file exists and search for it
fd validator --type f backend/
</function_calls><function_calls>
#!/bin/bashTry to find any file with Skill in name
fd Skill --type f backend/ 2>/dev/null
</function_calls>Let me check the repository root structure:
<function_calls>
#!/bin/bashList root directory
ls -la | head -20
</function_calls><function_calls>
#!/bin/bashList backend directory
ls -la backend/ 2>/dev/null || echo "backend not found at root"
</function_calls><function_calls>
#!/bin/bashList all directories at root
ls -1d / 2>/dev/null || ls -1d . 2>/dev/null | head -20
</function_calls><function_calls>
#!/bin/bashGet git information about the repo
git log --oneline -1
</function_calls><function_calls>
#!/bin/bashList all files to understand structure
git ls-files | head -50
</function_calls><function_calls>
#!/bin/bashFind the test file mentioned in the review
git ls-files | grep test_skill_service
</function_calls><function_calls>
#!/bin/bashGet full file listing
git ls-files | grep -E "(skill|validator)"
</function_calls>Let me examine the actual file path:
<function_calls>
#!/bin/bashSearch for test file
git ls-files "test_skill"
</function_calls><function_calls>
#!/bin/bashList all files to see structure
git ls-files | head -100
</function_calls>Based on my attempts to verify the review comment through shell commands, I'm encountering challenges accessing the specific files mentioned. Let me try a different approach to gather the information needed:
<function_calls>
#!/bin/bashFind Python test files
fd "test_" -e py | head -20
</function_calls><function_calls>
#!/bin/bashFind all Python files
find . -name "*.py" -type f 2>/dev/null | head -30
</function_calls>Let me try to locate the file differently:
<function_calls>
#!/bin/bashSearch for any reference to skill_service
find . -type f -name "skill" 2>/dev/null
</function_calls>Let me check if files actually exist:
<function_calls>
#!/bin/bashList current directory
pwd && ls -la
</function_calls>Let me check the git repository status more thoroughly:
<function_calls>
#!/bin/bashCheck git status
git status --short | head -20
</function_calls><function_calls>
#!/bin/bashCheck for any Python files at all
find . -name "*.py" 2>/dev/null | wc -l
</function_calls><function_calls>
#!/bin/bashList all files in repository
git ls-files | wc -l
</function_calls><function_calls>
#!/bin/bashTry to show files for the current branch
git ls-files --cached | head -50
</function_calls>Given the technical challenges with accessing the repository files through shell commands, let me assess what I can determine from the review comment itself and the code snippet provided:
Based on my investigation, I encountered technical limitations accessing the specific test file and validator implementation through shell commands in this sandbox environment. However, I can assess the review comment based on the context provided:
The review comment raises a valid consistency concern about ZIP structure - it identifies a potential mismatch between how the test creates ZIP files (root-level SKILL.md) versus documented requirements (folder structure with skill-folder/SKILL.md). This is a legitimate architectural consistency issue that should be verified against the actual SkillValidator implementation.
Without access to the validator code and referenced test lines (201-219), I cannot definitively confirm:
- Whether validate_zip actually requires folder structure or accepts root-level files
- How lines 201-219 structure their ZIP files
- What the PR documentation/API docstrings actually specify
Given these verification limitations, here is my rewritten review comment:
Verify ZIP structure consistency between test setup and validator requirements.
The test creates files at ZIP root level (
SKILL.md,script.py), but the code may require a skill folder structure (e.g.,my-skill/SKILL.md). Confirm that:
- The
validate_zipmethod accepts root-level SKILL.md files as the test assumes- This matches other tests in the file (referenced lines 201-219) and documented API expectations
- If folder structure is required, either update the test helper or adjust the validator
backend/tests/api/test_skills_api.py (3)
22-29: Well-designed helper aligns with expected ZIP structure.The
create_test_ziphelper correctly creates the folder structure ({folder_name}/SKILL.md) that matches the documented requirements. This is consistent with the API endpoint documentation.
372-402: User isolation test is comprehensive but relies on admin being a separate user.The test correctly validates that users cannot access or delete each other's skills by using different auth tokens. The assumption that
test_admin_tokenrepresents a different user is valid based on conftest fixtures. Consider adding a comment clarifying this design choice for future maintainers.
209-238: Thorough download test with ZIP integrity verification.The test validates the response headers, content-type, Content-Disposition, and verifies the ZIP is valid with expected contents. This is comprehensive coverage for the download functionality.
frontend/src/features/settings/components/skills/SkillManagementModal.tsx (1)
168-261: Well-structured skill card layout with proper accessibility attributes.The skill cards include title attributes for action buttons, proper semantic structure, and appropriate visual hierarchy. The status color coding (success/error) and Tag components for metadata provide good UX. As per coding guidelines, the component follows mobile-first responsive patterns.
backend/app/api/endpoints/kind/skills.py (2)
22-76: Upload endpoint has appropriate validation layering.The endpoint validates the file extension at the API layer, while delegating ZIP structure and content validation to
SkillValidatorin the service layer. This separation of concerns is appropriate. Thename.strip()at line 69 prevents leading/trailing whitespace issues.
79-111: List endpoint handles name filter correctly.The conditional logic for name-based filtering vs. full list is clear. When a name is provided, it returns a single-item list (or empty if not found), maintaining consistent response structure.
| # Import all models to ensure they are registered with Base | ||
| from app.models.kind import Kind | ||
| from app.models.subtask import Subtask | ||
| from app.models.shared_team import SharedTeam | ||
| from app.models.skill_binary import SkillBinary | ||
| # Import all models to ensure they are registered with same Base instance | ||
| from app.models import * |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
# Check what's exported from app.models/__init__.py
cat backend/app/models/__init__.pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
# SPDX-FileCopyrightText: 2025 Weibo, Inc.
#
# SPDX-License-Identifier: Apache-2.0
"""
Models package
"""
# Do NOT import Base here to avoid conflicts with app.db.base.Base
# All models should import Base directly from app.db.base
from app.models.user import User
from app.models.kind import (
Kind
)
from app.models.subtask import Subtask
from app.models.shared_team import SharedTeam
from app.models.skill_binary import SkillBinary
__all__ = [
"User",
"Kind",
"Subtask",
"SharedTeam",
"SkillBinary"
]
</shell_result>
Script executed:
# Check conftest.py for User model usage and overall content
cat backend/tests/conftest.pyRepository: wecode-ai/Wegent
<shell_result>
Repository: wecode-ai/Wegent
Exit code: 0
stdout:
# SPDX-FileCopyrightText: 2025 Weibo, Inc.
#
# SPDX-License-Identifier: Apache-2.0
import pytest
from typing import Generator
from sqlalchemy import create_engine
from sqlalchemy.orm import Session, sessionmaker
from fastapi.testclient import TestClient
from app.db.base import Base
from app.core.config import Settings
from app.core.security import get_password_hash, create_access_token
from app.models.user import User
# Import all models to ensure they are registered with Base
from app.models.kind import Kind
from app.models.subtask import Subtask
from app.models.shared_team import SharedTeam
from app.models.skill_binary import SkillBinary
# Import all models to ensure they are registered with same Base instance
from app.models import *
# Test database URL (SQLite in-memory with shared cache for thread safety)
TEST_DATABASE_URL = "sqlite:///file:testdb?mode=memory&cache=shared&uri=true"
@pytest.fixture(scope="function")
def test_db() -> Generator[Session, None, None]:
"""
Create a test database session using SQLite in-memory database.
Each test function gets a clean database instance.
"""
# Create test engine
engine = create_engine(
TEST_DATABASE_URL,
connect_args={"check_same_thread": False}
)
# Create all tables
Base.metadata.create_all(bind=engine)
# Create session factory with expire_on_commit=False to avoid lazy loading issues in tests
TestingSessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine, expire_on_commit=False)
# Create session
db = TestingSessionLocal()
try:
yield db
finally:
db.close()
# Drop all tables after test
Base.metadata.drop_all(bind=engine)
@pytest.fixture(scope="function")
def test_settings() -> Settings:
"""
Create test settings with overridden values.
"""
return Settings(
PROJECT_NAME="Test Project",
DATABASE_URL=TEST_DATABASE_URL,
SECRET_KEY="test-secret-key-for-testing-only",
ALGORITHM="HS256",
ACCESS_TOKEN_EXPIRE_MINUTES=30,
ENABLE_API_DOCS=False,
REDIS_URL="redis://localhost:6379/1"
)
@pytest.fixture(scope="function")
def test_user(test_db: Session) -> User:
"""
Create a test user in the database.
"""
user = User(
user_name="testuser",
password_hash=get_password_hash("testpassword123"),
email="test@example.com",
is_active=True,
git_info=None
)
test_db.add(user)
test_db.commit()
test_db.refresh(user)
return user
@pytest.fixture(scope="function")
def test_admin_user(test_db: Session) -> User:
"""
Create a test admin user in the database.
"""
admin = User(
user_name="admin",
password_hash=get_password_hash("adminpassword123"),
email="admin@example.com",
is_active=True,
git_info=None
)
test_db.add(admin)
test_db.commit()
test_db.refresh(admin)
return admin
@pytest.fixture(scope="function")
def test_inactive_user(test_db: Session) -> User:
"""
Create an inactive test user in the database.
"""
user = User(
user_name="inactiveuser",
password_hash=get_password_hash("inactive123"),
email="inactive@example.com",
is_active=False,
git_info=None
)
test_db.add(user)
test_db.commit()
test_db.refresh(user)
return user
@pytest.fixture(scope="function")
def test_token(test_user: User) -> str:
"""
Create a valid JWT token for the test user.
"""
return create_access_token(data={"sub": test_user.user_name})
@pytest.fixture(scope="function")
def test_admin_token(test_admin_user: User) -> str:
"""
Create a valid JWT token for the test admin user.
"""
return create_access_token(data={"sub": test_admin_user.user_name})
@pytest.fixture(scope="function")
def test_client(test_db: Session) -> TestClient:
"""
Create a test client with database dependency override.
"""
from app.main import create_app
from app.api.dependencies import get_db
app = create_app()
# Override database dependency to always return the same test_db session
def override_get_db():
try:
# Return the test_db session directly without yielding
# This ensures the same session is used across all requests
yield test_db
except Exception:
test_db.rollback()
raise
app.dependency_overrides[get_db] = override_get_db
return TestClient(app)
@pytest.fixture(scope="function")
def mock_redis(mocker):
"""
Mock Redis client for testing.
"""
mock_redis_client = mocker.MagicMock()
mock_redis_client.get.return_value = None
mock_redis_client.set.return_value = True
mock_redis_client.delete.return_value = True
mock_redis_client.exists.return_value = False
return mock_redis_client
</shell_result>
Remove the redundant wildcard import on line 21.
The wildcard import from app.models import * is completely redundant. All five models exported by app.models.__init__.py are already explicitly imported:
User(line 11)Kind,Subtask,SharedTeam,SkillBinary(lines 13–17)
The redundant wildcard violates PEP 8 and triggers the F403 linter warning. Delete line 21 entirely and keep only the explicit imports for clarity and compliance with Python best practices.
🧰 Tools
🪛 Ruff (0.14.6)
21-21: from app.models import * used; unable to detect undefined names
(F403)
🤖 Prompt for AI Agents
In backend/tests/conftest.py around lines 15 to 21, remove the redundant
wildcard import "from app.models import *" (line 21) and keep only the explicit
model imports already listed; delete that line so only the explicit imports
remain, which resolves the F403 linter warning and adheres to PEP 8.
Summary
Implemented comprehensive Skill management functionality for Wegent platform, enabling users to upload, manage, and deploy Claude Code Skills ZIP packages. This PR includes Backend, Executor integration, comprehensive test coverage, complete Frontend UI, and full documentation (YAML spec + user guides).
Core Changes
🗄️ Database Layer
skill_binariesfor storing Skill ZIP packages1a2b3c4d5e6f_add_skill_binaries_table.pySkillBinarywith binary data, file size, and SHA256 hashkinds.idfor automatic cleanup📋 Schema Layer
Skill,SkillSpec,SkillStatus,SkillListschemasskills: Optional[List[str]]fieldBotCreate,BotUpdate,BotDetailto include skills🔧 Service Layer
_validate_skills()method to check skill existencecreate_with_user()andupdate_with_user()_convert_to_bot_dict()🌐 API Layer
/api/v1/kinds/skills/upload- Upload Skill ZIP (max 10MB)/api/v1/kinds/skills- List Skills (supports?name=filter)/api/v1/kinds/skills/{skill_id}- Get Skill details/api/v1/kinds/skills/{skill_id}/download- Download ZIP (StreamingResponse)/api/v1/kinds/skills/{skill_id}- Update Skill ZIP/api/v1/kinds/skills/{skill_id}- Delete Skill (with reference check)🤖 Executor Integration
_download_and_deploy_skills()method~/.claude/skills/{skill_name}/bot_config["spec"]["skills"]🧪 Test Coverage (47 New Tests)
✅ Unit Tests (
backend/tests/services/test_skill_service.py):SkillValidatorclass✅ Integration Tests (
backend/tests/services/test_skill_kinds_service.py):SkillKindsServiceCRUD operations✅ API Tests (
backend/tests/api/test_skills_api.py):Test Summary:
🎨 Frontend UI
✅ Skills Management Page (
/settings?tab=skills):✅ Bot-Skill Integration:
Bot.skillsarray field✅ API Integration:
frontend/src/apis/skills.tswith full CRUD operationsfrontend/src/apis/bots.tstypes to includeskillsfieldBottype infrontend/src/types/api.tsfrontend/src/features/settings/services/bots.ts📚 Documentation
✅ YAML Specification (English & Chinese):
skillsfielddocs/en/reference/yaml-specification.md,docs/zh/reference/yaml-specification.md✅ User Guides (English & Chinese):
docs/en/guides/user/managing-skills.md,docs/zh/guides/user/managing-skills.md✅ README Updates (English & Chinese):
docs/en/README.md,docs/zh/README.md🧪 Test Plan
Backend API Testing
Ghost-Skill Integration
Executor Testing
~/.claude/skills/Frontend UI Testing
Security Testing
../or leading/)📝 Notes
Database Migration
ENVIRONMENT=developmentandDB_AUTO_MIGRATE=Truealembic upgrade headmanually before deploymentBreaking Changes
None. This is a purely additive feature.
🔗 Related Documentation
✅ Checklist
📊 Commits Summary
🎯 Implementation Highlights
Summary by CodeRabbit
New Features
Frontend
Backend
Validation & Security
Tests
Docs
✏️ Tip: You can customize this high-level summary in your review settings.