From 02eb1b42b9d94828965e8f7d0ed06a72251d474e Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Mon, 20 Jul 2026 19:19:10 +0800 Subject: [PATCH 01/10] feat: align skill repository behavior with agents --- backend/apps/skill_app.py | 8 + backend/consts/model.py | 6 + backend/database/db_models.py | 2 + backend/database/skill_db.py | 15 ++ backend/database/skill_repository_db.py | 28 ++- backend/services/skill_repository_service.py | 176 +++++++++++++++-- backend/services/skill_service.py | 62 +++++- ...ll_permission_and_repository_snapshots.sql | 44 +++++ .../agentConfig/SkillBuildModal.tsx | 183 ++++++++++++++++-- .../agentConfig/SkillDraftPanel.tsx | 53 +++++ .../skill-space/components/MineSkillsView.tsx | 143 +++++++++----- .../skill-space/components/RepositoryView.tsx | 89 ++++----- .../components/ReviewSkillList.tsx | 180 ++++++++--------- .../components/SkillRepositoryControls.tsx | 43 ++-- .../components/SkillRepositoryDetailModal.tsx | 13 +- .../components/skillRepositoryShared.ts | 6 +- frontend/app/[locale]/skill-space/page.tsx | 66 +++++-- frontend/public/locales/en/common.json | 6 +- frontend/public/locales/zh/common.json | 6 +- frontend/services/agentConfigService.ts | 20 ++ frontend/services/skillService.ts | 17 ++ frontend/types/skill.ts | 2 + frontend/types/skillRepository.ts | 6 +- test/backend/app/test_skill_app.py | 4 + test/backend/database/test_skill_db.py | 10 + .../database/test_skill_repository_db.py | 1 + .../services/test_skill_repository_service.py | 128 +++++++++++- test/backend/services/test_skill_service.py | 56 ++++++ 28 files changed, 1105 insertions(+), 268 deletions(-) create mode 100644 deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql diff --git a/backend/apps/skill_app.py b/backend/apps/skill_app.py index 7cc5b9f829..f3573e509e 100644 --- a/backend/apps/skill_app.py +++ b/backend/apps/skill_app.py @@ -46,6 +46,8 @@ def _build_skill_update_data(request: SkillUpdateRequest) -> Dict[str, Any]: "content", "tags", "source", + "group_ids", + "ingroup_permission", "config_schemas", "config_values", ): @@ -165,6 +167,8 @@ async def create_skill( "tool_ids": tool_ids, "tags": request.tags, "source": request.source, + "group_ids": request.group_ids, + "ingroup_permission": request.ingroup_permission, "config_schemas": request.config_schemas, "config_values": request.config_values, "files": request.files if request.files else [], @@ -342,6 +346,8 @@ async def update_skill_from_file( return JSONResponse(content=skill) except UnauthorizedError as e: raise HTTPException(status_code=401, detail=str(e)) + except ForbiddenError as e: + raise HTTPException(status_code=403, detail=str(e)) except SkillException as e: if _NOT_FOUND_TEXT in str(e).lower(): raise HTTPException(status_code=404, detail=str(e)) @@ -643,6 +649,8 @@ async def update_skill( return JSONResponse(content=skill) except UnauthorizedError as e: raise HTTPException(status_code=401, detail=str(e)) + except ForbiddenError as e: + raise HTTPException(status_code=403, detail=str(e)) except SkillException as e: if _NOT_FOUND_TEXT in str(e).lower(): raise HTTPException(status_code=404, detail=str(e)) diff --git a/backend/consts/model.py b/backend/consts/model.py index 0ec8ee4079..461f95b1d3 100644 --- a/backend/consts/model.py +++ b/backend/consts/model.py @@ -1275,6 +1275,8 @@ class SkillCreateRequest(BaseModel): tool_names: Optional[List[str]] = [] tags: Optional[List[str]] = [] source: Optional[str] = "custom" + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[Dict[str, str]]] = Field( @@ -1300,6 +1302,8 @@ class SkillUpdateRequest(BaseModel): tool_names: Optional[List[str]] = None tags: Optional[List[str]] = None source: Optional[str] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[SkillFileData]] = Field( @@ -1318,6 +1322,8 @@ class SkillResponse(BaseModel): tool_ids: List[int] tags: List[str] source: str + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None created_by: Optional[str] = None diff --git a/backend/database/db_models.py b/backend/database/db_models.py index b50e801fb6..92470c22f4 100644 --- a/backend/database/db_models.py +++ b/backend/database/db_models.py @@ -1058,6 +1058,8 @@ class SkillInfo(TableBase): JSON, doc="Runtime parameter values from config/config.yaml") source = Column(String(30), nullable=False, default="official", doc="Skill source: official, custom, etc.") + group_ids = Column(String, doc="Skill group IDs list") + ingroup_permission = Column(String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") class SkillToolRelation(TableBase): diff --git a/backend/database/skill_db.py b/backend/database/skill_db.py index 8f51849bbe..14bfc73653 100644 --- a/backend/database/skill_db.py +++ b/backend/database/skill_db.py @@ -10,6 +10,7 @@ from database.client import get_db_session, filter_property, as_dict from database.db_models import SkillInfo, SkillToolRelation, SkillInstance, ToolInfo from utils.skill_params_utils import strip_params_comments_for_db +from utils.str_utils import convert_list_to_string, convert_string_to_list logger = logging.getLogger(__name__) @@ -203,10 +204,16 @@ def _build_skill_update_values( "content": "skill_content", "tags": "skill_tags", "source": "source", + "ingroup_permission": "ingroup_permission", } for input_field, model_field in field_mapping.items(): if input_field in skill_data: row_values[model_field] = skill_data[input_field] + if "group_ids" in skill_data: + group_ids = skill_data["group_ids"] + row_values["group_ids"] = ( + convert_list_to_string(group_ids) if isinstance(group_ids, list) else group_ids + ) for field in ("config_schemas", "config_values"): if field in skill_data: @@ -242,6 +249,8 @@ def _to_dict(skill: SkillInfo) -> Dict[str, Any]: "config_schemas": skill.config_schemas, "config_values": skill.config_values, "source": skill.source, + "group_ids": convert_string_to_list(skill.group_ids), + "ingroup_permission": skill.ingroup_permission, "created_by": skill.created_by, "create_time": skill.create_time.isoformat() if skill.create_time else None, "updated_by": skill.updated_by, @@ -368,6 +377,12 @@ def create_skill(skill_data: Dict[str, Any], tenant_id: str) -> Dict[str, Any]: config_values=_params_value_for_db( skill_data.get("config_values")), source=skill_data.get("source", "custom"), + group_ids=( + convert_list_to_string(skill_data.get("group_ids")) + if isinstance(skill_data.get("group_ids"), list) + else skill_data.get("group_ids") + ), + ingroup_permission=skill_data.get("ingroup_permission"), created_by=skill_data.get("created_by"), create_time=datetime.now(), updated_by=skill_data.get("updated_by"), diff --git a/backend/database/skill_repository_db.py b/backend/database/skill_repository_db.py index e999974762..823d84b4ef 100644 --- a/backend/database/skill_repository_db.py +++ b/backend/database/skill_repository_db.py @@ -66,6 +66,7 @@ def get_skill_repository_by_skill_id( skill_id: int, *, publisher_tenant_id: Optional[str] = None, + statuses: Optional[Collection[str]] = None, ) -> Optional[dict]: """Fetch an active repository listing by source skill_id.""" with get_db_session() as session: @@ -77,7 +78,9 @@ def get_skill_repository_by_skill_id( query = query.filter( SkillRepository.publisher_tenant_id == publisher_tenant_id, ) - record = query.first() + if statuses is not None: + query = query.filter(SkillRepository.status.in_(list(statuses))) + record = query.order_by(SkillRepository.update_time.desc()).first() return as_dict(record) if record else None @@ -265,6 +268,29 @@ def update_skill_repository_status_by_id( return int(result.rowcount or 0) +def reset_skill_repository_status( + *, + repository_id: int, + skill_id: int, + status: str, + publisher_tenant_id: str, +) -> int: + """Set other active listings with the same skill and status to not_shared.""" + with get_db_session() as session: + result = session.execute( + update(SkillRepository) + .where( + SkillRepository.skill_id == skill_id, + SkillRepository.status == status, + SkillRepository.skill_repository_id != repository_id, + SkillRepository.publisher_tenant_id == publisher_tenant_id, + SkillRepository.delete_flag != "Y", + ) + .values(status=STATUS_NOT_SHARED) + ) + return int(result.rowcount or 0) + + def increment_skill_repository_downloads( *, repository_id: int, diff --git a/backend/services/skill_repository_service.py b/backend/services/skill_repository_service.py index 28f181c756..edb729e919 100644 --- a/backend/services/skill_repository_service.py +++ b/backend/services/skill_repository_service.py @@ -15,7 +15,12 @@ VALID_OWNERSHIP_FILTERS, VALID_REPOSITORY_STATUSES, ) -from consts.const import CAN_EDIT_ALL_USER_ROLES, PERMISSION_EDIT, PERMISSION_READ +from consts.const import ( + CAN_EDIT_ALL_USER_ROLES, + PERMISSION_EDIT, + PERMISSION_PRIVATE, + PERMISSION_READ, +) from consts.exceptions import ForbiddenError, SkillDuplicateError, SkillException from database.skill_repository_db import ( get_skill_repository_by_id_and_publisher, @@ -24,12 +29,15 @@ insert_skill_repository_record, list_skill_repository_by_skill_ids, list_skill_repository_summaries, + reset_skill_repository_status, update_skill_repository_by_id, update_skill_repository_status_by_id, ) from database.skill_db import get_skill_by_name +from database.group_db import query_group_ids_by_user from database.user_tenant_db import get_user_tenant_by_user_id from services.skill_service import SkillService +from utils.str_utils import convert_string_to_list logger = logging.getLogger("skill_repository_service") _REPOSITORY_LISTING_NOT_FOUND = "Repository listing not found" @@ -83,6 +91,18 @@ ) +def _to_group_id_set(group_ids: Any) -> set[int]: + if isinstance(group_ids, str): + return set(convert_string_to_list(group_ids)) + if isinstance(group_ids, list): + return { + int(group_id) + for group_id in group_ids + if str(group_id).strip().isdigit() + } + return set() + + def _serialize_created_at(create_time: Any) -> Optional[str]: """Serialize DB create_time to an ISO string for API consumers.""" if create_time is None: @@ -119,12 +139,16 @@ def _to_detail_item( ) -> Dict[str, Any]: """Map a DB record to a skill marketplace detail payload.""" snapshot = _as_dict(record.get("skill_info_json")) + creator_id = str(snapshot.get("created_by") or "").strip() + creator = get_user_tenant_by_user_id(creator_id) if creator_id else None + author = str((creator or {}).get("user_email") or "").strip() or None detail = { "skill_repository_id": record.get("skill_repository_id"), "skill_id": record.get("skill_id"), "name": record.get("name"), "description": record.get("description"), "source": record.get("source"), + "author": author, "submitted_by": record.get("submitted_by"), "icon": record.get("icon"), "status": record.get("status"), @@ -164,12 +188,14 @@ def _to_repository_info_item(record: Dict[str, Any]) -> Dict[str, Any]: def _matches_ownership(skill: Dict[str, Any], user_id: str, ownership_filter: str) -> bool: """Return whether a skill belongs to the requested ownership bucket.""" - created_by = skill.get("created_by") - if ownership_filter in (OWNERSHIP_ALL, OWNERSHIP_CREATED): - return created_by == user_id + if ownership_filter == OWNERSHIP_ALL: + return True + is_creator = str(skill.get("created_by")) == str(user_id) + if ownership_filter == OWNERSHIP_CREATED: + return is_creator if ownership_filter == OWNERSHIP_OTHERS: - return False - return created_by == user_id + return not is_creator + return True def _matches_search(skill: Dict[str, Any], search: Optional[str]) -> bool: @@ -190,11 +216,16 @@ def _matches_search(skill: Dict[str, Any], search: Optional[str]) -> bool: def _count_skills_by_ownership(skills: List[Dict[str, Any]], user_id: str) -> Dict[str, int]: """Count editable skills in each ownership bucket.""" - created = sum(1 for skill in skills if skill.get("created_by") == user_id) + created = sum( + 1 + for skill in skills + if str(skill.get("created_by")) == str(user_id) + ) + others = len(skills) - created return { - OWNERSHIP_ALL: created, + OWNERSHIP_ALL: len(skills), OWNERSHIP_CREATED: created, - OWNERSHIP_OTHERS: 0, + OWNERSHIP_OTHERS: others, } @@ -238,7 +269,43 @@ def _resolve_mine_skill_permission( """Resolve list-item permission for skill repository mine view.""" if user_role in CAN_EDIT_ALL_USER_ROLES: return PERMISSION_EDIT - return PERMISSION_EDIT if skill.get("created_by") == user_id else PERMISSION_READ + if str(skill.get("created_by")) == str(user_id): + return PERMISSION_EDIT + ingroup_permission = skill.get("ingroup_permission") + return ingroup_permission if ingroup_permission is not None else PERMISSION_READ + + +def _can_publish_skill( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, +) -> bool: + """Return whether the user may submit the skill to the repository.""" + if user_role == "ADMIN": + return True + return ( + user_role == "DEV" + and str(skill.get("created_by")) == str(user_id) + ) + + +def _can_view_mine_skill( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, + user_group_ids: set[int], +) -> bool: + """Return whether a skill should appear in the current user's mine list.""" + if user_role in CAN_EDIT_ALL_USER_ROLES: + return True + if str(skill.get("created_by")) == str(user_id): + return True + if skill.get("ingroup_permission") == PERMISSION_PRIVATE: + return False + skill_group_ids = _to_group_id_set(skill.get("group_ids")) + return bool(user_group_ids.intersection(skill_group_ids)) def _resolve_submitter_email(user_id: str) -> Optional[str]: @@ -255,9 +322,11 @@ def _validate_create_listing_permission( ) -> None: """Only ADMIN, or DEV who created the skill, may share to marketplace.""" user_role = _get_user_role(user_id) - if user_role == "ADMIN": - return - if user_role == "DEV" and skill_info.get("created_by") == user_id: + if _can_publish_skill( + skill=skill_info, + user_id=user_id, + user_role=user_role, + ): return raise ForbiddenError( f"User role {user_role} not authorized to create repository listing" @@ -322,6 +391,8 @@ def _build_skill_info_json(skill_info: Dict[str, Any]) -> Dict[str, Any]: "config_schemas": skill_info.get("config_schemas"), "config_values": skill_info.get("config_values"), "source": skill_info.get("source"), + "group_ids": skill_info.get("group_ids") or [], + "ingroup_permission": skill_info.get("ingroup_permission"), "tool_ids": skill_info.get("tool_ids") or [], "created_by": skill_info.get("created_by"), } @@ -386,6 +457,48 @@ def _build_repository_data_from_skill( return repository_data +def _find_resubmittable_repository_record( + skill_id: int, + tenant_id: str, +) -> Optional[Dict[str, Any]]: + """Find an existing review draft that can be refreshed by a new submission.""" + pending = get_skill_repository_by_skill_id( + skill_id, + publisher_tenant_id=tenant_id, + statuses=[STATUS_PENDING_REVIEW], + ) + if pending: + return pending + return get_skill_repository_by_skill_id( + skill_id, + publisher_tenant_id=tenant_id, + statuses=[STATUS_REJECTED], + ) + + +def _reset_repository_peer_statuses( + *, + skill_repository_id: int, + skill_id: int, + status: str, + publisher_tenant_id: str, +) -> None: + """Reset peer listings with the same status; also clear rejected when submitting.""" + reset_skill_repository_status( + repository_id=skill_repository_id, + skill_id=skill_id, + status=status, + publisher_tenant_id=publisher_tenant_id, + ) + if status == STATUS_PENDING_REVIEW: + reset_skill_repository_status( + repository_id=skill_repository_id, + skill_id=skill_id, + status=STATUS_REJECTED, + publisher_tenant_id=publisher_tenant_id, + ) + + def _validate_create_payload(repository_data: Dict[str, Any]) -> None: """Validate required fields before inserting a repository listing.""" required_fields = ( @@ -426,9 +539,9 @@ def create_skill_repository_listing_impl( ) _validate_create_payload(repository_data) - existing = get_skill_repository_by_skill_id( + existing = _find_resubmittable_repository_record( skill_id, - publisher_tenant_id=tenant_id, + tenant_id, ) if not existing: repository_id = insert_skill_repository_record( @@ -454,6 +567,13 @@ def create_skill_repository_listing_impl( raise ValueError("Failed to update repository listing") is_updated = True + _reset_repository_peer_statuses( + skill_repository_id=repository_id, + skill_id=skill_id, + status=STATUS_PENDING_REVIEW, + publisher_tenant_id=tenant_id, + ) + record = get_skill_repository_by_id_and_publisher( repository_id, tenant_id, @@ -591,6 +711,13 @@ def update_skill_repository_status_impl( if rows_affected == 0: raise ValueError(_REPOSITORY_LISTING_NOT_FOUND) + _reset_repository_peer_statuses( + skill_repository_id=skill_repository_id, + skill_id=record["skill_id"], + status=status, + publisher_tenant_id=tenant_id, + ) + updated = get_skill_repository_by_id_and_publisher( skill_repository_id, tenant_id, @@ -755,6 +882,8 @@ def _to_mine_skill_item( "description": skill.get("description"), "source": skill.get("source"), "tags": skill.get("tags") or [], + "group_ids": skill.get("group_ids") or [], + "ingroup_permission": skill.get("ingroup_permission"), "created_by": skill.get("created_by"), "created_at": skill.get("create_time"), "updated_at": skill.get("update_time"), @@ -763,6 +892,11 @@ def _to_mine_skill_item( user_id=user_id, user_role=user_role, ), + "can_publish": _can_publish_skill( + skill=skill, + user_id=user_id, + user_role=user_role, + ), "repository_info": repository_info, } @@ -789,7 +923,17 @@ def list_my_editable_skills_impl( safe_page_size = max(int(page_size or 10), 1) user_role = _get_user_role(user_id) - skills = SkillService(tenant_id=tenant_id).list_skills(tenant_id=tenant_id) + user_group_ids = set(query_group_ids_by_user(user_id) or []) + skills = [ + skill + for skill in SkillService(tenant_id=tenant_id).list_skills(tenant_id=tenant_id) + if _can_view_mine_skill( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) + ] counts = _count_skills_by_ownership(skills, user_id) filtered_skills = [ diff --git a/backend/services/skill_service.py b/backend/services/skill_service.py index 4622208294..b8a684d505 100644 --- a/backend/services/skill_service.py +++ b/backend/services/skill_service.py @@ -21,18 +21,66 @@ from nexent.skills.skill_loader import SkillLoader from nexent.core.utils.observer import MessageObserver from nexent.core.agents.agent_model import ModelConfig -from consts.const import CONTAINER_SKILLS_PATH, OFFICIAL_SKILLS_ZIP_PATH, ROOT_DIR +from consts.const import ( + CAN_EDIT_ALL_USER_ROLES, + CONTAINER_SKILLS_PATH, + OFFICIAL_SKILLS_ZIP_PATH, + PERMISSION_EDIT, + PERMISSION_PRIVATE, + ROOT_DIR, +) from consts.exceptions import ForbiddenError, SkillException from database import skill_db +from database.group_db import query_group_ids_by_user +from database.user_tenant_db import get_user_tenant_by_user_id from agents.skill_creation_agent import create_skill_from_request from utils.prompt_template_utils import get_skill_creation_simple_prompt_template from utils.content_classifier_utils import ContentClassifier +from utils.str_utils import convert_list_to_string logger = logging.getLogger(__name__) _skill_manager: Optional[SkillManager] = None +def _apply_default_skill_permission_fields( + skill_data: Dict[str, Any], + user_id: Optional[str], +) -> None: + """Default user-created skills to the creator's groups with edit permission.""" + if not user_id: + return + if skill_data.get("group_ids") is None: + skill_data["group_ids"] = convert_list_to_string(query_group_ids_by_user(user_id)) + if not skill_data.get("ingroup_permission"): + skill_data["ingroup_permission"] = PERMISSION_EDIT + + +def _get_user_role(user_id: Optional[str]) -> str: + if not user_id: + return "USER" + user_tenant = get_user_tenant_by_user_id(user_id) + if not user_tenant: + return "USER" + return str(user_tenant.get("user_role") or "USER") + + +def _can_edit_skill(skill: Dict[str, Any], user_id: Optional[str]) -> bool: + if not user_id: + return False + if _get_user_role(user_id) in CAN_EDIT_ALL_USER_ROLES: + return True + if str(skill.get("created_by")) == str(user_id): + return True + if skill.get("ingroup_permission") in (None, PERMISSION_PRIVATE): + return False + if skill.get("ingroup_permission") != PERMISSION_EDIT: + return False + skill_group_ids = {int(group_id) for group_id in skill.get("group_ids") or []} + user_group_ids = set(query_group_ids_by_user(user_id) or []) + return bool(skill_group_ids.intersection(user_group_ids)) + + def _normalize_zip_entry_path(name: str) -> str: """Normalize a ZIP member path for comparison (slashes, strip ./).""" norm = name.replace("\\", "/").strip() @@ -1033,6 +1081,7 @@ def create_skill( if user_id: skill_data["created_by"] = user_id skill_data["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_data, user_id) try: # Create database record first @@ -1160,6 +1209,7 @@ def _create_skill_from_md( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) @@ -1293,6 +1343,7 @@ def _create_skill_from_zip( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) @@ -1449,6 +1500,8 @@ def update_skill_from_file( existing = skill_db.get_skill_by_name(skill_name, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_name}") + if user_id is not None and not _can_edit_skill(existing, user_id): + raise ForbiddenError("Not authorized to update this skill") content_bytes: bytes if isinstance(file_content, str): @@ -1619,6 +1672,8 @@ def update_skill( existing = skill_db.get_skill_by_name(skill_name, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_name}") + if user_id is not None and not _can_edit_skill(existing, user_id): + raise ForbiddenError("Not authorized to update this skill") result = skill_db.update_skill( skill_name, skill_data, effective_tenant_id, updated_by=user_id or None @@ -1671,7 +1726,7 @@ def update_skill( ) return self._enrich_configs_from_yaml(result) - except SkillException: + except (ForbiddenError, SkillException): raise except Exception as e: logger.error(f"Error updating skill {skill_name}: {e}") @@ -1692,7 +1747,7 @@ def update_skill_by_id( existing = skill_db.get_skill_by_id(skill_id, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_id}") - if not user_id or existing.get("created_by") != user_id: + if not _can_edit_skill(existing, user_id): raise ForbiddenError("Not authorized to update this skill") local_dir = self._resolve_local_skills_dir_for_overlay() @@ -2240,6 +2295,7 @@ def create_skill_from_zip_bytes( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) diff --git a/deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql b/deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql new file mode 100644 index 0000000000..1abd9020d5 --- /dev/null +++ b/deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql @@ -0,0 +1,44 @@ +-- Migration: Add skill group permissions and allow separate repository snapshots by status +-- Date: 2026-07-13 +-- Description: Align skill ownership and repository status behavior with agent repository semantics. + +SET search_path TO nexent; + +ALTER TABLE IF EXISTS nexent.ag_skill_info_t + ADD COLUMN IF NOT EXISTS group_ids VARCHAR, + ADD COLUMN IF NOT EXISTS ingroup_permission VARCHAR(30); + +COMMENT ON COLUMN nexent.ag_skill_info_t.group_ids IS 'Skill group IDs list'; +COMMENT ON COLUMN nexent.ag_skill_info_t.ingroup_permission IS 'In-group permission: EDIT, READ_ONLY, PRIVATE'; + +WITH tenant_groups AS ( + SELECT + tenant_id, + string_agg(group_id::text, ',' ORDER BY group_id) AS group_ids + FROM nexent.tenant_group_info_t + WHERE delete_flag = 'N' + GROUP BY tenant_id +) +UPDATE nexent.ag_skill_info_t skill +SET group_ids = tenant_groups.group_ids +FROM tenant_groups +WHERE skill.tenant_id = tenant_groups.tenant_id + AND skill.delete_flag = 'N' + AND skill.tenant_id IS NOT NULL + AND (skill.group_ids IS NULL OR skill.group_ids = ''); + +UPDATE nexent.ag_skill_info_t +SET ingroup_permission = 'EDIT' +WHERE delete_flag = 'N' + AND tenant_id IS NOT NULL + AND (ingroup_permission IS NULL OR ingroup_permission = ''); + +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_active; +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_shared_active; +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_pending_active; + +CREATE INDEX IF NOT EXISTS idx_skill_repository_skill_status_delete + ON nexent.ag_skill_repository_t (publisher_tenant_id, skill_id, status, delete_flag); + +COMMENT ON COLUMN nexent.ag_skill_repository_t.skill_id IS + 'Source skill ID from ag_skill_info_t; multiple active snapshots may exist across statuses'; diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx index 1abc1d1fa1..1c10b9e400 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, useEffect, useRef, type ChangeEvent } from "react"; +import { useState, useEffect, useMemo, useRef, type ChangeEvent } from "react"; import { useTranslation } from "react-i18next"; import { Modal, @@ -44,10 +44,18 @@ import { type SkillListItem, type SkillData, } from "@/services/skillService"; -import { fetchSkillById } from "@/services/agentConfigService"; import type { MyEditableSkillItem } from "@/types/skillRepository"; +import { + fetchSkillById, + fetchSkillFileContent, + fetchSkillFiles, + type SkillFileNode, +} from "@/services/agentConfigService"; +import { normalizeSkillFiles } from "@/lib/skillFileUtils"; import { MarkdownRenderer } from "@/components/common/markdownRenderer"; import log from "@/lib/logger"; +import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; +import { useGroupDetails, useGroupList } from "@/hooks/group/useGroupList"; import SkillDraftPanel from "./SkillDraftPanel"; const { TextArea } = Input; @@ -110,6 +118,39 @@ function mergeGeneratedSkillTabs( return { updatedTabs, finalTabs }; } +function flattenSkillFiles( + nodes: SkillFileNode[], + skillName: string +): string[] { + const paths: string[] = []; + const walk = (items: SkillFileNode[], parentPath = "") => { + items.forEach((item) => { + const isRootSkillDirectory = + !parentPath && item.type === "directory" && item.name === skillName; + const path = isRootSkillDirectory + ? "" + : parentPath + ? `${parentPath}/${item.name}` + : item.name; + if (item.type === "file") { + paths.push(path); + } else if (item.children?.length) { + walk(item.children, path); + } + }); + }; + walk(nodes); + return paths; +} + +function sortSkillTabs(tabs: SkillFileContent[]): SkillFileContent[] { + return [...tabs].sort((a, b) => { + if (a.path === "SKILL.md") return -1; + if (b.path === "SKILL.md") return 1; + return a.path.localeCompare(b.path); + }); +} + export default function SkillBuildModal({ isOpen, onCancel, @@ -118,10 +159,33 @@ export default function SkillBuildModal({ onBeforeEditSave, }: SkillBuildModalProps) { const { t } = useTranslation("common"); + const { user, getAccessibleGroupIds } = useAuthorizationContext(); const [form] = Form.useForm(); const isEditMode = Boolean(editingSkill); + const { data: groupData } = useGroupList(user?.tenantId ?? null); + const accessibleGroupIds = useMemo( + () => getAccessibleGroupIds(), + [getAccessibleGroupIds] + ); + const { groups: filteredGroups } = useGroupDetails( + groupData?.groups ?? [], + accessibleGroupIds + ); + const groupSelectOptions = useMemo( + () => + filteredGroups.map((group) => ({ + label: group.group_name, + value: group.group_id, + })), + [filteredGroups] + ); const [activeTab, setActiveTab] = useState("interactive"); const [isSubmitting, setIsSubmitting] = useState(false); + const [isLoadingEditFiles, setIsLoadingEditFiles] = useState(false); + const [loadedEditSkillId, setLoadedEditSkillId] = useState( + null + ); + const [editFilesError, setEditFilesError] = useState(null); const [allSkills, setAllSkills] = useState([]); const [uploadFile, setUploadFile] = useState(null); const [uploadExtractedSkillName, setUploadExtractedSkillName] = @@ -228,6 +292,14 @@ export default function SkillBuildModal({ }; }, [isOpen]); + useEffect(() => { + if (!isOpen || isEditMode) return; + form.setFieldsValue({ + group_ids: accessibleGroupIds, + ingroup_permission: "READ_ONLY", + }); + }, [accessibleGroupIds, form, isEditMode, isOpen]); + useEffect(() => { if (!isOpen) { // Abort any ongoing streaming request @@ -254,6 +326,9 @@ export default function SkillBuildModal({ setSummaryContent(""); currentAssistantIdRef.current = ""; setAccumulatedDraft(null); + setLoadedEditSkillId(null); + setEditFilesError(null); + setIsLoadingEditFiles(false); } }, [isOpen]); @@ -307,24 +382,72 @@ export default function SkillBuildModal({ description: skill.description || "", source: skill.source || "custom", tags: Array.isArray(skill.tags) ? skill.tags : [], + group_ids: Array.isArray(skill.group_ids) ? skill.group_ids : [], + ingroup_permission: skill.ingroup_permission || "READ_ONLY", }); - setSkillTabs([{ path: "SKILL.md", content: skill.content || "" }]); - setActiveSkillTab("SKILL.md"); }; setActiveTab("interactive"); - applySkillInfo({ - name: skillName, - description: editingSkill.description || "", - source: editingSkill.source || "custom", - tags: editingSkill.tags || [], - }); + setEditFilesError(null); + setLoadedEditSkillId(null); + setIsLoadingEditFiles(true); - void fetchSkillById(editingSkill.skill_id).then((result) => { - if (result.success && result.data) { - applySkillInfo(result.data); + const loadEditFiles = async () => { + try { + const result = await fetchSkillById(editingSkill.skill_id); + const skillInfo = + result.success && result.data + ? result.data + : { + name: skillName, + description: editingSkill.description || "", + source: editingSkill.source || "custom", + tags: editingSkill.tags || [], + group_ids: editingSkill.group_ids || [], + ingroup_permission: + editingSkill.ingroup_permission || "READ_ONLY", + }; + const resolvedSkillName = skillInfo.name?.trim() || skillName; + const fileTree = await fetchSkillFiles(resolvedSkillName); + const filePaths = flattenSkillFiles( + normalizeSkillFiles(fileTree), + resolvedSkillName + ); + if (filePaths.length === 0) { + throw new Error("Skill file tree is empty"); + } + const tabs = await Promise.all( + filePaths.map(async (path) => { + const content = await fetchSkillFileContent( + resolvedSkillName, + path + ); + if (content === null) { + throw new Error(`Failed to load skill file: ${path}`); + } + return { path, content }; + }) + ); + if (!cancelled) { + const sortedTabs = sortSkillTabs(tabs); + applySkillInfo(skillInfo); + setSkillTabs(sortedTabs); + setActiveSkillTab(sortedTabs[0]?.path || "SKILL.md"); + setLoadedEditSkillId(editingSkill.skill_id); + } + } catch (error) { + log.error("Failed to load skill files for editing:", error); + if (!cancelled) { + setEditFilesError(t("skillManagement.message.loadFilesFailed")); + } + } finally { + if (!cancelled) { + setIsLoadingEditFiles(false); + } } - }); + }; + + void loadEditFiles(); return () => { cancelled = true; @@ -354,6 +477,12 @@ export default function SkillBuildModal({ const handleManualSubmit = async () => { try { + if (isEditMode && (isLoadingEditFiles || editFilesError)) { + message.error( + editFilesError || t("skillManagement.message.loadFilesFailed") + ); + return; + } const values = await form.validateFields(); if (isEditMode && editingSkill && onBeforeEditSave) { const shouldContinue = await onBeforeEditSave(editingSkill); @@ -600,7 +729,9 @@ export default function SkillBuildModal({ taskIdRef.current = taskId; }, onThinkingUpdate: (step, desc) => { - setThinkingDescription(desc || t("skillManagement.generatingSkill")); + setThinkingDescription( + desc || t("skillManagement.generatingSkill") + ); }, onThinkingVisible: (visible) => { setIsThinkingVisible(visible); @@ -775,6 +906,8 @@ export default function SkillBuildModal({ const modalBodyFrame = "min(92vh, 760px)"; const editingSkillName = editingSkill?.name?.trim() || interactiveSkillName.trim(); + const isEditContentReady = + !isEditMode || loadedEditSkillId === editingSkill?.skill_id; const renderUploadTab = () => { const existingSkill = allSkills.find( @@ -1071,6 +1204,7 @@ export default function SkillBuildModal({ textareaRefs={textareaRefs} shouldAutoScrollRef={shouldAutoScrollRef} onTextareaScroll={handleTextareaScroll} + groupSelectOptions={groupSelectOptions} /> ); @@ -1111,7 +1245,9 @@ export default function SkillBuildModal({ title={
- {isEditMode ? t("skillManagement.edit.title") : t("skillManagement.title")} + {isEditMode + ? t("skillManagement.edit.title") + : t("skillManagement.title")}
{isEditMode @@ -1143,6 +1279,9 @@ export default function SkillBuildModal({ type="primary" loading={isSubmitting} onClick={handleManualSubmit} + disabled={ + isEditMode && (isLoadingEditFiles || Boolean(editFilesError)) + } > {getConfirmButtonText()} @@ -1173,7 +1312,17 @@ export default function SkillBuildModal({ items={visibleTabItems} className="skill-build-tabs shrink-0" /> - {isEditMode || activeTab === "interactive" ? ( + {isEditMode && !isEditContentReady ? ( +
+ + {editFilesError ? ( +

{editFilesError}

+ ) : ( +
+ )} + +
+ ) : isEditMode || activeTab === "interactive" ? (
{renderChatPanel()} {renderDraftPanel()} diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx index e6790b8968..f1f85a53b4 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx @@ -13,6 +13,7 @@ import { Button, Col, Form, Input, Modal, Row, Select, Tooltip } from "antd"; import { FileText, Folder, Maximize2, Pencil, Plus, X } from "lucide-react"; import { useTranslation } from "react-i18next"; +import { Can } from "@/components/permission/Can"; import type { SkillFileContent, SkillFormData } from "@/types/skill"; const { TextArea } = Input; @@ -31,6 +32,7 @@ interface SkillDraftPanelProps { textareaRefs?: MutableRefObject>; shouldAutoScrollRef?: MutableRefObject>; onTextareaScroll?: (tabPath: string) => void; + groupSelectOptions?: Array<{ label: string; value: number }>; className?: string; } @@ -46,6 +48,7 @@ export default function SkillDraftPanel({ textareaRefs, shouldAutoScrollRef, onTextareaScroll, + groupSelectOptions = [], className, }: SkillDraftPanelProps) { const { t } = useTranslation("common"); @@ -224,6 +227,56 @@ export default function SkillDraftPanel({ + + + + + + + + + + void; searchQuery: string; onSearchChange: (value: string) => void; isLoading: boolean; @@ -80,6 +92,7 @@ export function MineSkillsView({ onRetry: () => void; onCreateSkill: () => void; onEditSkill: (skill: MyEditableSkillItem) => void; + onViewSkill: (skill: MyEditableSkillItem) => void; onDeleteSkill: (skill: MyEditableSkillItem) => Promise; onApplyListing: ( skill: MyEditableSkillItem, @@ -95,6 +108,11 @@ export function MineSkillsView({ useState(null); const [reviewModalInfo, setReviewModalInfo] = useState(null); + const ownershipLabelKey: Record = { + all: "agentRepository.mine.filter.all", + created: "agentRepository.mine.filter.created", + others: "agentRepository.mine.filter.others", + }; const openReviewModal = (skill: MyEditableSkillItem) => { const repositoryInfo = pickReviewDisplayRepositoryInfo( @@ -183,10 +201,16 @@ export function MineSkillsView({
- {}}> - {t("skillRepository.filter.all")} - {counts.all} - + {MINE_OWNERSHIP_FILTERS.map((filter) => ( + onOwnershipChange(filter)} + > + {t(ownershipLabelKey[filter])} + {counts[filter]} + + ))}

@@ -198,36 +222,38 @@ export function MineSkillsView({ isError={isError} isFetching={isFetching} onRetry={onRetry} - isEmpty={false} + isEmpty={skills.length === 0} emptyDescription={t("skillRepository.mine.empty")} > -

- {skills.map((skill) => - isNewSkillPaddingItem(skill) ? ( -
- -
- ) : ( - onEditSkill(skill)} - onDelete={() => handleDeleteSkill(skill)} - onApplyListing={() => handleEnableSkill(skill)} - onViewReview={() => openReviewModal(skill)} - /> - ) - )} -
+ <> +
+ {skills.map((skill) => + isNewSkillPaddingItem(skill) ? ( +
+ +
+ ) : ( + onEditSkill(skill)} + onView={() => onViewSkill(skill)} + onDelete={() => handleDeleteSkill(skill)} + onApplyListing={() => handleEnableSkill(skill)} + onViewReview={() => openReviewModal(skill)} + /> + ) + )} +
+ + - - = { function MineSkillCard({ skill, onEdit, + onView, onDelete, onApplyListing, onViewReview, }: { skill: MyEditableSkillItem; onEdit: () => void; + onView: () => void; onDelete: () => void; onApplyListing: () => void; onViewReview: () => void; @@ -267,22 +295,35 @@ function MineSkillCard({ const latestRepository = pickReviewDisplayRepositoryInfo( skill.repository_info ?? [] ); + const repositoryInfo = skill.repository_info ?? []; + const hasRepositoryInfo = latestRepository != null; const repositoryStatus = latestRepository?.status ?? "not_shared"; - const hasRepositoryInfo = (skill.repository_info ?? []).length > 0; - const canApplyListing = - !latestRepository || latestRepository.status === "rejected"; - const isEnabled = latestRepository?.status === "shared"; - const canEdit = skill.permission !== "READ_ONLY"; + const hasSharedRepository = repositoryInfo.some( + (info) => info.status === "shared" + ); + const canEdit = + skill.permission !== "READ_ONLY" && skill.permission !== "PRIVATE"; + const canPublish = skill.can_publish === true; const updatedAt = formatRepositoryDate(skill.updated_at ?? skill.update_time); const sourceLabel = getSkillSourceLabel(skill.source, t); const tags = skill.tags?.filter((tag) => tag.trim()) ?? []; const isPendingReview = repositoryStatus === "pending_review"; + const canApplyListing = canPublish && !isPendingReview; + const applyButtonLabel = isPendingReview + ? getSkillRepositoryStatusLabel(t, repositoryStatus) + : hasSharedRepository + ? t("skillRepository.mine.button.reapply") + : t("skillRepository.mine.button.apply"); const menuItems: MenuProps["items"] = [ - ...(isPendingReview + ...(canPublish && hasRepositoryInfo ? [ { key: "review", - label: t("skillRepository.mine.viewReviewProgress"), + label: t( + isPendingReview + ? "skillRepository.mine.viewReviewProgress" + : "skillRepository.mine.viewRepositoryStatus" + ), icon: , onClick: onViewReview, }, @@ -309,7 +350,7 @@ function MineSkillCard({

{skill.name || t("skillRepository.common.untitled")}

- {hasRepositoryInfo ? ( + {hasSharedRepository ? ( Hub @@ -389,24 +430,22 @@ function MineSkillCard({ type="default" className="min-w-0 flex-1" icon={} - disabled + onClick={onView} > {t("skillRepository.common.view")} )} - + {canPublish ? ( + + ) : null}
diff --git a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx index 9551a46e3b..a040d9fa12 100644 --- a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx +++ b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx @@ -71,51 +71,52 @@ export function RepositoryView({ isEmpty={listings.length === 0} emptyDescription={t("skillRepository.repository.empty")} > -
- {listings.map((listing) => ( - onDetailClick(listing)} - showAdminMenu={showAdminMenu} - isTakingDown={ - takingDownRepositoryId === listing.skill_repository_id - } - onTakeDown={() => onTakeDown(listing)} - action={ -
- - -
- } - /> - ))} -
+ <> +
+ {listings.map((listing) => ( + onDetailClick(listing)} + showAdminMenu={showAdminMenu} + isTakingDown={ + takingDownRepositoryId === listing.skill_repository_id + } + onTakeDown={() => onTakeDown(listing)} + action={ +
+ + +
+ } + /> + ))} +
+ + - -
); } diff --git a/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx b/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx index 58b7f2d791..dca725697a 100644 --- a/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx +++ b/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx @@ -73,103 +73,105 @@ export function ReviewSkillList({ isEmpty={listings.length === 0} emptyDescription={t("skillRepository.review.empty")} > -
-
-
- {t("skillRepository.review.skill")} - {t("skillRepository.review.submitter")} - {t("skillRepository.review.status")} - {t("skillRepository.review.actions")} -
+ <> +
+
+
+ {t("skillRepository.review.skill")} + {t("skillRepository.review.submitter")} + {t("skillRepository.review.status")} + {t("skillRepository.review.actions")} +
-
    - {listings.map((listing) => { - const isUpdating = - updatingRepositoryId === listing.skill_repository_id; - const isPendingReview = listing.status === "pending_review"; +
      + {listings.map((listing) => { + const isUpdating = + updatingRepositoryId === listing.skill_repository_id; + const isPendingReview = listing.status === "pending_review"; - return ( -
    • -
      -
      - -
      -
      -

      - {listing.name || t("skillRepository.common.untitled")} -

      - {listing.description ? ( -

      - {listing.description} -

      - ) : null} + return ( +
    • +
      +
      + +
      +
      +

      + {listing.name || + t("skillRepository.common.untitled")} +

      + {listing.description ? ( +

      + {listing.description} +

      + ) : null} +
      -
-
- {getSubmitterDisplay(listing.submitted_by, t)} -
+
+ {getSubmitterDisplay(listing.submitted_by, t)} +
-
- - {t(REVIEW_STATUS_LABEL_KEYS[listing.status])} - -
+
+ + {t(REVIEW_STATUS_LABEL_KEYS[listing.status])} + +
-
- - {isPendingReview ? ( - <> - - - - ) : null} -
- - ); - })} - +
+ + {isPendingReview ? ( + <> + + + + ) : null} +
+ + ); + })} + +
-
+ + - - ); } diff --git a/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx b/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx index 43d812c4a7..fec41b927d 100644 --- a/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx +++ b/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx @@ -67,26 +67,45 @@ export function PaginationBar({ onPageChange: (page: number) => void; }) { const { t } = useTranslation("common"); - const totalPages = Math.max(1, Math.ceil(total / pageSize)); - if (total <= pageSize) return null; + const totalPages = total > 0 ? Math.ceil(total / pageSize) : 0; + if (totalPages <= 1) return null; return ( -
+
+ {Array.from({ length: totalPages }, (_, index) => index + 1).map( + (pageNumber) => ( + + ) + )}
); } diff --git a/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx b/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx index cefc72fb5f..45b08eacd8 100644 --- a/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx +++ b/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx @@ -28,8 +28,7 @@ export function SkillRepositoryDetailModal({ onRetry: () => void; }) { const { t } = useTranslation("common"); - const author = - detail?.submitted_by?.trim() || t("skillRepository.detail.unknownAuthor"); + const author = detail?.author?.trim(); const updatedAt = formatRepositoryDate(detail?.updated_at) || formatRepositoryDate(detail?.created_at) || @@ -72,10 +71,12 @@ export function SkillRepositoryDetailModal({
-

- - {author} -

+ {author ? ( +

+ + {author} +

+ ) : null} diff --git a/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts b/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts index eb0a0f9bfc..124267c1b6 100644 --- a/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts +++ b/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts @@ -72,11 +72,13 @@ export function pickReviewDisplayRepositoryInfo( const rejected = pickLatestRepositoryInfo( items.filter((item) => item.status === "rejected") ); - if (rejected) return rejected; - return pickLatestRepositoryInfo( + const shared = pickLatestRepositoryInfo( items.filter((item) => item.status === "shared") ); + if (shared) return shared; + + return rejected; } export function isCancelableRepositoryStatus( diff --git a/frontend/app/[locale]/skill-space/page.tsx b/frontend/app/[locale]/skill-space/page.tsx index ffaa3adb56..e2ebd2dada 100644 --- a/frontend/app/[locale]/skill-space/page.tsx +++ b/frontend/app/[locale]/skill-space/page.tsx @@ -22,11 +22,13 @@ import { ApiError } from "@/services/api"; import { deleteSkillByName } from "@/services/skillService"; import { cn } from "@/lib/utils"; import type { + MineOwnershipFilter, MyEditableSkillItem, MySkillRepositoryInfoItem, SkillRepositoryListingItem, SkillRepositoryListingStatus, } from "@/types/skillRepository"; +import type { Skill } from "@/types/agentConfig"; import { CountBadge } from "./components/SkillRepositoryControls"; import { SkillRepositoryDetailModal } from "./components/SkillRepositoryDetailModal"; import { MineSkillsView } from "./components/MineSkillsView"; @@ -37,6 +39,7 @@ import { STATUS_LABEL_KEYS, } from "./components/skillRepositoryShared"; import SkillBuildModal from "../agents/components/agentConfig/SkillBuildModal"; +import SkillDetailModal from "../agents/components/agentConfig/SkillDetailModal"; enum SkillRepositoryTab { REPOSITORY = "repository", @@ -47,7 +50,6 @@ enum SkillRepositoryTab { const REPOSITORY_PAGE_SIZE = 6; const MINE_PAGE_SIZE = 6; const REVIEW_PAGE_SIZE = 10; - const skillRepositoryTheme = { token: { colorPrimary: "#2563eb", colorInfo: "#3b82f6", borderRadius: 12 }, }; @@ -73,6 +75,8 @@ export default function SkillRepositoryPage() { const [repositoryPage, setRepositoryPage] = useState(1); const [repositorySearch, setRepositorySearch] = useState(""); const [minePage, setMinePage] = useState(1); + const [mineOwnership, setMineOwnership] = + useState("all"); const [mineSearch, setMineSearch] = useState(""); const [reviewPage, setReviewPage] = useState(1); const [detailRepositoryId, setDetailRepositoryId] = useState( @@ -82,6 +86,9 @@ export default function SkillRepositoryPage() { const [editingSkill, setEditingSkill] = useState( null ); + const [viewingSkill, setViewingSkill] = useState( + null + ); const [copyListing, setCopyListing] = useState(null); const [copyTargetName, setCopyTargetName] = useState(""); @@ -103,17 +110,20 @@ export default function SkillRepositoryPage() { const mineParams = useMemo( () => ({ - ownership: "all" as const, + ownership: mineOwnership, page: minePage, page_size: MINE_PAGE_SIZE, ...(mineSearch.trim() ? { search: mineSearch.trim() } : {}), - ...(!mineSearch.trim() ? { new_skill_padding: true } : {}), + ...(mineOwnership === "all" && !mineSearch.trim() + ? { new_skill_padding: true } + : {}), }), - [minePage, mineSearch] + [mineOwnership, minePage, mineSearch] ); const reviewParams = useMemo( () => ({ + status: "pending_review" as const, page: reviewPage, page_size: REVIEW_PAGE_SIZE, sort_by_update_time: true, @@ -232,7 +242,9 @@ export default function SkillRepositoryPage() { const handleInstall = (listing: SkillRepositoryListingItem) => { const baseName = listing.name?.trim() || "Skill"; setCopyListing(listing); - setCopyTargetName(t("skillRepository.copy.defaultName", { name: baseName })); + setCopyTargetName( + t("skillRepository.copy.defaultName", { name: baseName }) + ); setCopyNameError(null); }; @@ -271,7 +283,9 @@ export default function SkillRepositoryPage() { return; } message.error( - error instanceof Error ? error.message : t("skillRepository.copy.failed") + error instanceof Error + ? error.message + : t("skillRepository.copy.failed") ); } }; @@ -316,28 +330,23 @@ export default function SkillRepositoryPage() { ); }; - const getActiveRepositoryInfo = (skill?: MyEditableSkillItem | null) => + const getPendingRepositoryInfo = (skill?: MyEditableSkillItem | null) => (skill?.repository_info ?? []).filter( - (info) => info.status === "shared" || info.status === "pending_review" + (info) => info.status === "pending_review" ); const confirmEditListedSkill = async ( skill: MyEditableSkillItem ): Promise => { - const activeInfo = getActiveRepositoryInfo(skill); - if (activeInfo.length === 0) { + const pendingInfo = getPendingRepositoryInfo(skill); + if (pendingInfo.length === 0) { return true; } - const hasShared = activeInfo.some((info) => info.status === "shared"); const confirmed = await new Promise((resolve) => { modal.confirm({ - title: hasShared - ? t("skillRepository.edit.confirmTakeDownTitle") - : t("skillRepository.edit.confirmWithdrawTitle"), - content: hasShared - ? t("skillRepository.edit.confirmTakeDownContent") - : t("skillRepository.edit.confirmWithdrawContent"), + title: t("skillRepository.edit.confirmWithdrawTitle"), + content: t("skillRepository.edit.confirmWithdrawContent"), okText: t("skillRepository.edit.continueSave"), cancelText: t("common.cancel"), onOk: () => resolve(true), @@ -350,7 +359,7 @@ export default function SkillRepositoryPage() { try { await Promise.all( - activeInfo.map((info) => + pendingInfo.map((info) => updateStatusMutation.mutateAsync({ skillRepositoryId: info.skill_repository_id, status: "not_shared", @@ -497,6 +506,11 @@ export default function SkillRepositoryPage() { { + setMineOwnership(ownership); + setMinePage(1); + }} searchQuery={mineSearch} onSearchChange={(value) => { setMineSearch(value); @@ -518,6 +532,7 @@ export default function SkillRepositoryPage() { setEditingSkill(skill); setSkillBuildOpen(true); }} + onViewSkill={(skill) => setViewingSkill(skill)} onDeleteSkill={async (skill) => { const name = skill.name?.trim(); if (!name) { @@ -645,6 +660,21 @@ export default function SkillRepositoryPage() { onSuccess={handleSkillBuildSuccess} onBeforeEditSave={confirmEditListedSkill} /> + setViewingSkill(null)} + /> ); } diff --git a/frontend/public/locales/en/common.json b/frontend/public/locales/en/common.json index 3d1e160710..780ad89a3b 100644 --- a/frontend/public/locales/en/common.json +++ b/frontend/public/locales/en/common.json @@ -1348,6 +1348,7 @@ "skillManagement.message.createSuccess": "Skill created successfully", "skillManagement.message.updateSuccess": "Skill updated successfully", "skillManagement.message.submitFailed": "Failed to submit skill", + "skillManagement.message.loadFilesFailed": "Failed to load Skill files. Saving is disabled to prevent data loss.", "skillManagement.message.pleaseSelectFile": "Please select a file to upload", "skillManagement.message.chatError": "Failed to generate skill, please try again", "skillManagement.message.nameExists": "Skill name already exists. Please change the name", @@ -2189,10 +2190,11 @@ "skillRepository.mine.applySuccess": "Listing request submitted", "skillRepository.mine.applyError": "Failed to submit review", "skillRepository.mine.button.apply": "List", - "skillRepository.mine.button.listed": "Listed", + "skillRepository.mine.button.reapply": "List again", "skillRepository.mine.deleteTitle": "Delete Skill", "skillRepository.mine.deleteContent": "Delete {{name}}?", "skillRepository.mine.viewReviewProgress": "View review progress", + "skillRepository.mine.viewRepositoryStatus": "View repository status", "skillRepository.mine.takeDownSuccess": "Taken down", "skillRepository.mine.withdrawSuccess": "Listing request withdrawn", "skillRepository.copy.defaultName": "{{name}} Copy", @@ -2210,9 +2212,7 @@ "skillRepository.delete.emptyName": "Skill name is empty and cannot be deleted", "skillRepository.delete.failed": "Delete failed", "skillRepository.delete.success": "Deleted successfully", - "skillRepository.edit.confirmTakeDownTitle": "Saving will take this Skill down", "skillRepository.edit.confirmWithdrawTitle": "Saving will withdraw review", - "skillRepository.edit.confirmTakeDownContent": "This Skill is listed. Saving changes will take it down, and you will need to submit it for review again.", "skillRepository.edit.confirmWithdrawContent": "This Skill is under review. Saving changes will withdraw the review, and you will need to submit it again.", "skillRepository.edit.continueSave": "Continue saving", "skillRepository.review.status.notShared": "Not listed", diff --git a/frontend/public/locales/zh/common.json b/frontend/public/locales/zh/common.json index 168c75a767..3bfebc2464 100644 --- a/frontend/public/locales/zh/common.json +++ b/frontend/public/locales/zh/common.json @@ -1319,6 +1319,7 @@ "skillManagement.message.createSuccess": "技能创建成功", "skillManagement.message.updateSuccess": "技能更新成功", "skillManagement.message.submitFailed": "提交技能失败", + "skillManagement.message.loadFilesFailed": "加载 Skill 文件失败，无法保存以避免丢失文件", "skillManagement.message.pleaseSelectFile": "请选择要上传的文件", "skillManagement.message.chatError": "生成技能失败，请重试", "skillManagement.message.nameExists": "技能名称已存在，请修改名称", @@ -2315,10 +2316,11 @@ "skillRepository.mine.applySuccess": "已提交上架申请", "skillRepository.mine.applyError": "提交审批失败", "skillRepository.mine.button.apply": "上架", - "skillRepository.mine.button.listed": "已上架", + "skillRepository.mine.button.reapply": "重新上架", "skillRepository.mine.deleteTitle": "删除 Skill", "skillRepository.mine.deleteContent": "确认删除 {{name}}？", "skillRepository.mine.viewReviewProgress": "查看审批进度", + "skillRepository.mine.viewRepositoryStatus": "查看仓库状态", "skillRepository.mine.takeDownSuccess": "已下架", "skillRepository.mine.withdrawSuccess": "已撤回申请", "skillRepository.copy.defaultName": "{{name}} 副本", @@ -2336,9 +2338,7 @@ "skillRepository.delete.emptyName": "Skill 名称为空，无法删除", "skillRepository.delete.failed": "删除失败", "skillRepository.delete.success": "删除成功", - "skillRepository.edit.confirmTakeDownTitle": "保存后将自动下架", "skillRepository.edit.confirmWithdrawTitle": "保存后将撤回审核", - "skillRepository.edit.confirmTakeDownContent": "该 Skill 已上架，保存修改后将自动下架，需要重新提交审核。", "skillRepository.edit.confirmWithdrawContent": "该 Skill 正在审核中，保存修改后将撤回审核，需要重新提交。", "skillRepository.edit.continueSave": "继续保存", "skillRepository.review.status.notShared": "未上架", diff --git a/frontend/services/agentConfigService.ts b/frontend/services/agentConfigService.ts index 16ab250853..516ae700a9 100644 --- a/frontend/services/agentConfigService.ts +++ b/frontend/services/agentConfigService.ts @@ -1244,6 +1244,8 @@ export const createSkill = async (skillData: { source?: string; tags?: string[]; content?: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }) => { try { @@ -1257,6 +1259,12 @@ export const createSkill = async (skillData: { if (skillData.files && skillData.files.length > 0) { requestBody.files = skillData.files; } + if (skillData.group_ids !== undefined) { + requestBody.group_ids = skillData.group_ids; + } + if (skillData.ingroup_permission !== undefined) { + requestBody.ingroup_permission = skillData.ingroup_permission; + } const response = await fetch(API_ENDPOINTS.skills.create, { method: "POST", @@ -1304,6 +1312,8 @@ export const updateSkill = async ( tags?: string[]; content?: string; config_values?: Record; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }, tenantId?: string | null @@ -1319,6 +1329,10 @@ export const updateSkill = async ( requestBody.content = skillData.content; if (skillData.config_values !== undefined) requestBody.config_values = skillData.config_values; + if (skillData.group_ids !== undefined) + requestBody.group_ids = skillData.group_ids; + if (skillData.ingroup_permission !== undefined) + requestBody.ingroup_permission = skillData.ingroup_permission; if (skillData.files !== undefined) requestBody.files = skillData.files; const url = tenantId @@ -1365,6 +1379,8 @@ export const updateSkillById = async ( tags?: string[]; content?: string; config_values?: Record; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }, tenantId?: string | null @@ -1381,6 +1397,10 @@ export const updateSkillById = async ( requestBody.content = skillData.content; if (skillData.config_values !== undefined) requestBody.config_values = skillData.config_values; + if (skillData.group_ids !== undefined) + requestBody.group_ids = skillData.group_ids; + if (skillData.ingroup_permission !== undefined) + requestBody.ingroup_permission = skillData.ingroup_permission; if (skillData.files !== undefined) requestBody.files = skillData.files; const url = tenantId diff --git a/frontend/services/skillService.ts b/frontend/services/skillService.ts index 3e94307a9f..864880236c 100644 --- a/frontend/services/skillService.ts +++ b/frontend/services/skillService.ts @@ -29,6 +29,8 @@ export interface SkillData { source: string; tags: string[]; content: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: SkillFileContent[]; } @@ -41,6 +43,8 @@ export interface SkillListItem { description?: string; tags: string[]; content?: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; config_values: Record | null; config_schemas: unknown[] | null; source: string; @@ -196,6 +200,10 @@ export async function fetchSkillsList( const toolIds = Array.isArray(rawToolIds) ? rawToolIds.map((id) => Number(id)).filter((n) => !Number.isNaN(n)) : []; + const rawGroupIds = s.group_ids; + const groupIds = Array.isArray(rawGroupIds) + ? rawGroupIds.map((id) => Number(id)).filter((n) => !Number.isNaN(n)) + : []; return { skill_id: Number.isNaN(skillId) ? 0 : skillId, name: String(s.name ?? ""), @@ -207,6 +215,11 @@ export async function fetchSkillsList( config_values, source: String(s.source ?? "custom"), tool_ids: toolIds, + group_ids: groupIds, + ingroup_permission: + s.ingroup_permission !== undefined + ? (s.ingroup_permission as "EDIT" | "READ_ONLY" | "PRIVATE" | null) + : undefined, created_by: s.created_by !== undefined ? (s.created_by as string | null) @@ -247,6 +260,8 @@ export const submitSkillForm = async ( source: values.source, tags: values.tags, content: values.content, + group_ids: values.group_ids, + ingroup_permission: values.ingroup_permission, files: values.files, }); } else { @@ -256,6 +271,8 @@ export const submitSkillForm = async ( source: values.source, tags: values.tags, content: values.content, + group_ids: values.group_ids, + ingroup_permission: values.ingroup_permission, files: values.files, }); } diff --git a/frontend/types/skill.ts b/frontend/types/skill.ts index 54a4e2d401..73dc9345a8 100644 --- a/frontend/types/skill.ts +++ b/frontend/types/skill.ts @@ -41,6 +41,8 @@ export interface SkillFormData { source: string; tags: string[]; content: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; } /** diff --git a/frontend/types/skillRepository.ts b/frontend/types/skillRepository.ts index 6440edf609..605593ebf3 100644 --- a/frontend/types/skillRepository.ts +++ b/frontend/types/skillRepository.ts @@ -18,6 +18,7 @@ export interface SkillRepositoryListingItem { tags?: string[]; downloads?: number; category_id?: number | null; + author?: string | null; submitted_by?: string | null; } @@ -66,13 +67,16 @@ export interface MyEditableSkillItem { description?: string | null; source?: string | null; tags?: string[]; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; created_by?: string | null; updated_by?: string | null; create_time?: string | null; update_time?: string | null; updated_at?: string | null; downloads?: number; - permission?: "EDIT" | "READ_ONLY"; + permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; + can_publish?: boolean; repository_info: MySkillRepositoryInfoItem[]; } diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 1699f1b24c..9d4f2cdecd 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -142,6 +142,8 @@ class MockSkillCreateRequest(BaseModel): config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[Dict[str, str]]] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None class MockSkillFileData(BaseModel): path: str @@ -158,6 +160,8 @@ class MockSkillUpdateRequest(BaseModel): config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[MockSkillFileData]] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None class MockSkillResponse(BaseModel): skill_id: Optional[int] = None diff --git a/test/backend/database/test_skill_db.py b/test/backend/database/test_skill_db.py index 8ff2806e95..1345af397e 100644 --- a/test/backend/database/test_skill_db.py +++ b/test/backend/database/test_skill_db.py @@ -1,6 +1,7 @@ """Unit tests for backend.database.skill_db module.""" import sys import os +import types sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../backend")) sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../sdk")) @@ -45,11 +46,18 @@ utils_skill_params_mock = MagicMock() utils_skill_params_mock.strip_params_comments_for_db = lambda x: x +utils_str_utils_mock = types.ModuleType('utils.str_utils') +utils_str_utils_mock.convert_list_to_string = lambda items: "" if items is None else ",".join(str(item) for item in items) +utils_str_utils_mock.convert_string_to_list = ( + lambda items: [] if not items else [int(item.strip()) for item in items.split(",") if item.strip().isdigit()] +) sys.modules['utils'] = MagicMock() sys.modules['utils.auth_utils'] = MagicMock() sys.modules['utils.skill_params_utils'] = utils_skill_params_mock +sys.modules['utils.str_utils'] = utils_str_utils_mock sys.modules['backend.utils'] = MagicMock() sys.modules['backend.utils.skill_params_utils'] = utils_skill_params_mock +sys.modules['backend.utils.str_utils'] = utils_str_utils_mock from backend.database.skill_db import ( _params_value_for_db, @@ -115,6 +123,8 @@ def __init__(self, **kwargs): self.config_schemas = kwargs.get('config_schemas', {}) self.config_values = kwargs.get('config_values', {}) self.source = kwargs.get('source', 'custom') + self.group_ids = kwargs.get('group_ids', '') + self.ingroup_permission = kwargs.get('ingroup_permission') self.created_by = kwargs.get('created_by', 'creator1') self.create_time = kwargs.get('create_time', datetime.now()) self.updated_by = kwargs.get('updated_by', 'updater1') diff --git a/test/backend/database/test_skill_repository_db.py b/test/backend/database/test_skill_repository_db.py index d0129563b0..1e26b8ec5e 100644 --- a/test/backend/database/test_skill_repository_db.py +++ b/test/backend/database/test_skill_repository_db.py @@ -108,6 +108,7 @@ def test_get_repository_by_skill_id_with_optional_tenant(monkeypatch, mock_sessi session, query = mock_session record = MagicMock(payload={"skill_id": 8}) query.filter.return_value = query + query.order_by.return_value = query query.first.return_value = record _patch_session(monkeypatch, session) diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index 5fabc70639..a923fd94ab 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -16,6 +16,19 @@ if str(_BACKEND_ROOT) not in sys.path: sys.path.insert(0, str(_BACKEND_ROOT)) +_MOCKED_MODULE_NAMES = [ + "database.skill_repository_db", + "database.group_db", + "database.skill_db", + "database.user_tenant_db", + "services.skill_service", + "utils.str_utils", +] +_ORIGINAL_MODULES = { + name: sys.modules.get(name) + for name in _MOCKED_MODULE_NAMES +} + _consts_package = sys.modules.get("consts") if _consts_package is not None and not hasattr(_consts_package, "__path__"): _consts_package.__path__ = [] @@ -50,6 +63,8 @@ consts_const_module.PERMISSION_EDIT = "edit" if not hasattr(consts_const_module, "PERMISSION_READ"): consts_const_module.PERMISSION_READ = "read" + if not hasattr(consts_const_module, "PERMISSION_PRIVATE"): + consts_const_module.PERMISSION_PRIVATE = "private" _skill_repo_db_mock = MagicMock() _skill_repo_db_mock.get_skill_repository_by_id_and_publisher = MagicMock() @@ -58,10 +73,25 @@ _skill_repo_db_mock.insert_skill_repository_record = MagicMock(return_value=1) _skill_repo_db_mock.list_skill_repository_by_skill_ids = MagicMock(return_value=[]) _skill_repo_db_mock.list_skill_repository_summaries = MagicMock() +_skill_repo_db_mock.reset_skill_repository_status = MagicMock(return_value=0) _skill_repo_db_mock.update_skill_repository_by_id = MagicMock(return_value=1) _skill_repo_db_mock.update_skill_repository_status_by_id = MagicMock(return_value=1) sys.modules["database.skill_repository_db"] = _skill_repo_db_mock +_group_db_mock = MagicMock() +_group_db_mock.query_group_ids_by_user = MagicMock(return_value=[]) +sys.modules["database.group_db"] = _group_db_mock + +_utils_str_utils_mock = types.ModuleType("utils.str_utils") +_utils_str_utils_mock.convert_string_to_list = MagicMock( + side_effect=lambda value: [ + int(item) + for item in str(value or "").split(",") + if item.strip().isdigit() + ] +) +sys.modules["utils.str_utils"] = _utils_str_utils_mock + _skill_db_mock = MagicMock() _skill_db_mock.get_skill_by_name = MagicMock(return_value=None) sys.modules["database.skill_db"] = _skill_db_mock @@ -136,6 +166,14 @@ def __init__(self, duplicate_names): from backend.services import skill_repository_service as srs +def teardown_module(): + for name, original in _ORIGINAL_MODULES.items(): + if original is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = original + + def setup_function(): _skill_repo_db_mock.reset_mock() _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.side_effect = None @@ -145,8 +183,11 @@ def setup_function(): _skill_repo_db_mock.increment_skill_repository_downloads.return_value = 1 _skill_repo_db_mock.insert_skill_repository_record.return_value = 1 _skill_repo_db_mock.list_skill_repository_by_skill_ids.return_value = [] + _skill_repo_db_mock.reset_skill_repository_status.return_value = 0 _skill_repo_db_mock.update_skill_repository_by_id.return_value = 1 _skill_repo_db_mock.update_skill_repository_status_by_id.return_value = 1 + _group_db_mock.reset_mock() + _group_db_mock.query_group_ids_by_user.return_value = [] _skill_db_mock.reset_mock() _skill_db_mock.get_skill_by_name.return_value = None _user_tenant_db_mock.reset_mock() @@ -169,7 +210,11 @@ def _repository_record(status="not_shared", publisher_user_id="user-1"): "submitted_by": "dev@example.com", "tags": ["tag"], "downloads": 0, - "skill_info_json": {"content": "content", "tags": ["tag"]}, + "skill_info_json": { + "content": "content", + "tags": ["tag"], + "created_by": "user-1", + }, "create_time": None, "update_time": None, } @@ -211,6 +256,28 @@ def test_create_skill_repository_listing_updates_existing_record(): _skill_repo_db_mock.update_skill_repository_by_id.assert_called_once() +def test_create_skill_repository_listing_does_not_overwrite_shared_record(): + _skill_repo_db_mock.get_skill_repository_by_skill_id.side_effect = [None, None] + _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = ( + _repository_record(status="pending_review") + ) + + result = srs.create_skill_repository_listing_impl( + skill_id=10, + tenant_id="tenant-1", + user_id="user-1", + ) + + assert result["is_updated"] is False + _skill_repo_db_mock.insert_skill_repository_record.assert_called_once() + _skill_repo_db_mock.update_skill_repository_by_id.assert_not_called() + requested_statuses = [ + call.kwargs["statuses"] + for call in _skill_repo_db_mock.get_skill_repository_by_skill_id.call_args_list + ] + assert requested_statuses == [["pending_review"], ["rejected"]] + + def test_create_skill_repository_listing_rejects_non_owner_dev(): class SkillServiceNonOwner(_SkillServiceMock): def get_skill_by_id(self, skill_id, tenant_id=None): @@ -248,6 +315,12 @@ def test_update_status_admin_approves_pending_review(): assert result["status"] == "shared" _skill_repo_db_mock.update_skill_repository_status_by_id.assert_called_once() + _skill_repo_db_mock.reset_skill_repository_status.assert_called_once_with( + repository_id=1, + skill_id=10, + status="shared", + publisher_tenant_id="tenant-1", + ) def test_update_status_dev_cannot_approve_review(): @@ -356,6 +429,54 @@ def list_skills(self, tenant_id=None): assert [item["name"] for item in result["items"]] == ["Excel Report"] +def test_mine_ownership_uses_creator_not_edit_permission(): + class ListSkillService(_SkillServiceMock): + def list_skills(self, tenant_id=None): + return [ + { + "skill_id": 1, + "name": "Created Skill", + "created_by": 100, + "group_ids": [1], + "ingroup_permission": "EDIT", + }, + { + "skill_id": 2, + "name": "Editable Skill", + "created_by": 200, + "group_ids": [1], + "ingroup_permission": "EDIT", + }, + ] + + with ( + patch.object(srs, "SkillService", ListSkillService), + patch.object( + srs, + "get_user_tenant_by_user_id", + return_value={"user_role": "DEV"}, + ), + patch.object(srs, "query_group_ids_by_user", return_value=[1]), + ): + created_result = srs.list_my_editable_skills_impl( + tenant_id="tenant-1", + user_id="100", + ownership="created", + ) + others_result = srs.list_my_editable_skills_impl( + tenant_id="tenant-1", + user_id="100", + ownership="others", + ) + + assert created_result["counts"] == {"all": 2, "created": 1, "others": 1} + assert [item["name"] for item in created_result["items"]] == ["Created Skill"] + assert [item["name"] for item in others_result["items"]] == ["Editable Skill"] + assert others_result["items"][0]["permission"] == "EDIT" + assert created_result["items"][0]["can_publish"] is True + assert others_result["items"][0]["can_publish"] is False + + def test_list_repository_listings_validates_status(): with pytest.raises(ValueError): srs.list_skill_repository_listings_impl( @@ -528,6 +649,11 @@ def test_repository_list_and_detail_success(): ) detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") assert detail["skill_repository_id"] == 1 + assert detail["author"] == "dev@example.com" + + _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = None + detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") + assert detail["author"] is None def test_mapping_and_filter_helpers_cover_edge_branches(): diff --git a/test/backend/services/test_skill_service.py b/test/backend/services/test_skill_service.py index ceb8edd6ca..db81d9271f 100644 --- a/test/backend/services/test_skill_service.py +++ b/test/backend/services/test_skill_service.py @@ -167,6 +167,9 @@ def get_cached_message(self): consts_const_mock.CONTAINER_SKILLS_PATH = TEST_LOCAL_SKILLS_DIR consts_const_mock.OFFICIAL_SKILLS_ZIP_PATH = "/tmp/official-skills.zip" consts_const_mock.ROOT_DIR = "/tmp" +consts_const_mock.CAN_EDIT_ALL_USER_ROLES = {"ADMIN"} +consts_const_mock.PERMISSION_EDIT = "EDIT" +consts_const_mock.PERMISSION_PRIVATE = "PRIVATE" consts_exceptions_mock = types.ModuleType('consts.exceptions') class SkillException(Exception): @@ -210,6 +213,10 @@ async def open(self, path, mode='r', encoding=None): utils_prompt_template_utils_mock = types.ModuleType('utils.prompt_template_utils') utils_prompt_template_utils_mock.get_skill_creation_simple_prompt_template = MagicMock(return_value={"system_prompt": "", "user_prompt": ""}) utils_content_classifier_utils_mock = types.ModuleType('utils.content_classifier_utils') +utils_str_utils_mock = types.ModuleType('utils.str_utils') +utils_str_utils_mock.convert_list_to_string = MagicMock( + side_effect=lambda items: "" if items is None else ",".join(str(item) for item in items) +) class MockContentClassifier: def classify(self, content): @@ -220,6 +227,7 @@ def classify(self, content): sys.modules['utils.skill_params_utils'] = utils_skill_params_utils_mock sys.modules['utils.prompt_template_utils'] = utils_prompt_template_utils_mock sys.modules['utils.content_classifier_utils'] = utils_content_classifier_utils_mock +sys.modules['utils.str_utils'] = utils_str_utils_mock # Set up database mocks database_mock = types.ModuleType('database') @@ -230,6 +238,12 @@ def classify(self, content): database_db_models_mock = types.ModuleType('database.db_models') database_db_models_mock.SkillInfo = MagicMock() +database_group_db_mock = types.ModuleType('database.group_db') +database_group_db_mock.query_group_ids_by_user = MagicMock(return_value=[]) +database_user_tenant_db_mock = types.ModuleType('database.user_tenant_db') +database_user_tenant_db_mock.get_user_tenant_by_user_id = MagicMock( + return_value={"user_role": "DEV"} +) # Create mock skill_db module with functions database_skill_db_mock = types.ModuleType('database.skill_db') @@ -319,6 +333,8 @@ def mock_update_skill_by_id(skill_id, skill_data, tenant_id=None, updated_by=Non sys.modules['database.client'] = database_client_mock sys.modules['database.skill_db'] = database_skill_db_mock sys.modules['database.db_models'] = database_db_models_mock +sys.modules['database.group_db'] = database_group_db_mock +sys.modules['database.user_tenant_db'] = database_user_tenant_db_mock setattr(database_mock, 'skill_db', database_skill_db_mock) # Mock nexent.core.agents.run_agent for create_skill_from_request @@ -809,6 +825,26 @@ def test_update_skill_success(self, mocker): assert result["description"] == "updated" + def test_update_skill_rejects_user_without_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=False, + ) + + service = SkillService(tenant_id="test-tenant") + + with pytest.raises(skill_service.ForbiddenError): + service.update_skill( + "existing", + {"description": "updated"}, + tenant_id="test-tenant", + user_id="viewer", + ) + def test_update_skill_with_params(self, mocker): mocker.patch( 'backend.services.skill_service.skill_db.get_skill_by_name', @@ -1879,6 +1915,26 @@ def test_update_from_md_explicit_type(self, mocker): assert result["description"] == "updated" + def test_update_from_file_rejects_user_without_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=False, + ) + + service = SkillService(tenant_id="test-tenant") + + with pytest.raises(skill_service.ForbiddenError): + service.update_skill_from_file( + "existing", + b"---\nname: existing\n---", + tenant_id="test-tenant", + user_id="viewer", + ) + def test_update_from_zip(self, mocker): import zipfile zip_buffer = io.BytesIO() From cc607067b6bc52c96f9c262869aab6477b10a277 Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Tue, 21 Jul 2026 16:50:08 +0800 Subject: [PATCH 02/10] feat: align skill visibility and repository permissions --- backend/apps/skill_app.py | 7 +- backend/apps/skill_repository_app.py | 3 +- backend/database/skill_repository_db.py | 2 + backend/services/skill_repository_service.py | 106 ++++++----------- backend/services/skill_service.py | 108 ++++++++++++++++-- .../agentConfig/SkillDetailModal.tsx | 53 ++++++++- .../agentConfig/SkillManagement.tsx | 25 ++-- .../skill-space/components/MineSkillsView.tsx | 30 +++-- .../skill-space/components/RepositoryView.tsx | 2 +- frontend/hooks/agent/useSkillList.ts | 11 +- frontend/public/locales/en/common.json | 1 + frontend/public/locales/zh/common.json | 1 + frontend/services/agentConfigService.ts | 5 + frontend/types/agentConfig.ts | 3 + frontend/types/skillRepository.ts | 1 + test/backend/app/test_skill_app.py | 15 ++- test/backend/app/test_skill_repository_app.py | 1 + .../services/test_skill_repository_service.py | 61 ++++++++-- test/backend/services/test_skill_service.py | 91 +++++++++++++++ 19 files changed, 386 insertions(+), 140 deletions(-) diff --git a/backend/apps/skill_app.py b/backend/apps/skill_app.py index f3573e509e..3ef683cbdd 100644 --- a/backend/apps/skill_app.py +++ b/backend/apps/skill_app.py @@ -68,11 +68,14 @@ async def list_skills( ) -> JSONResponse: """List all available skills for the current tenant (or a specific tenant for super admin).""" try: - _, current_tenant_id = get_current_user_id(authorization) + user_id, current_tenant_id = get_current_user_id(authorization) # Super admin can query a specific tenant's skills; otherwise use current user's tenant effective_tenant_id = tenant_id if tenant_id else current_tenant_id service = SkillService(tenant_id=effective_tenant_id) - skills = service.list_skills(tenant_id=effective_tenant_id) + skills = service.list_visible_skills( + tenant_id=effective_tenant_id, + user_id=user_id, + ) return JSONResponse(content={"skills": skills}) except SkillException as e: raise HTTPException(status_code=500, detail=str(e)) diff --git a/backend/apps/skill_repository_app.py b/backend/apps/skill_repository_app.py index 108642f6ad..704630c8ee 100644 --- a/backend/apps/skill_repository_app.py +++ b/backend/apps/skill_repository_app.py @@ -46,9 +46,10 @@ async def list_skill_repository_listings_api( ): """List all skill marketplace repository listings with optional filters.""" try: - _, tenant_id = get_current_user_id(authorization) + user_id, tenant_id = get_current_user_id(authorization) result = list_skill_repository_listings_impl( tenant_id, + user_id=user_id, status=status, skill_id=skill_id, category_id=category_id, diff --git a/backend/database/skill_repository_db.py b/backend/database/skill_repository_db.py index 823d84b4ef..fff9b28aed 100644 --- a/backend/database/skill_repository_db.py +++ b/backend/database/skill_repository_db.py @@ -136,6 +136,7 @@ def list_skill_repository_summaries( query = session.query( SkillRepository.skill_repository_id, SkillRepository.skill_id, + SkillRepository.publisher_user_id, SkillRepository.submitted_by, SkillRepository.name, SkillRepository.description, @@ -174,6 +175,7 @@ def list_skill_repository_summaries( { "skill_repository_id": row.skill_repository_id, "skill_id": row.skill_id, + "publisher_user_id": row.publisher_user_id, "submitted_by": row.submitted_by, "name": row.name, "description": row.description, diff --git a/backend/services/skill_repository_service.py b/backend/services/skill_repository_service.py index edb729e919..ef2becefde 100644 --- a/backend/services/skill_repository_service.py +++ b/backend/services/skill_repository_service.py @@ -15,12 +15,7 @@ VALID_OWNERSHIP_FILTERS, VALID_REPOSITORY_STATUSES, ) -from consts.const import ( - CAN_EDIT_ALL_USER_ROLES, - PERMISSION_EDIT, - PERMISSION_PRIVATE, - PERMISSION_READ, -) +from consts.const import PERMISSION_PRIVATE, PERMISSION_READ from consts.exceptions import ForbiddenError, SkillDuplicateError, SkillException from database.skill_repository_db import ( get_skill_repository_by_id_and_publisher, @@ -34,10 +29,8 @@ update_skill_repository_status_by_id, ) from database.skill_db import get_skill_by_name -from database.group_db import query_group_ids_by_user from database.user_tenant_db import get_user_tenant_by_user_id from services.skill_service import SkillService -from utils.str_utils import convert_string_to_list logger = logging.getLogger("skill_repository_service") _REPOSITORY_LISTING_NOT_FOUND = "Repository listing not found" @@ -91,18 +84,6 @@ ) -def _to_group_id_set(group_ids: Any) -> set[int]: - if isinstance(group_ids, str): - return set(convert_string_to_list(group_ids)) - if isinstance(group_ids, list): - return { - int(group_id) - for group_id in group_ids - if str(group_id).strip().isdigit() - } - return set() - - def _serialize_created_at(create_time: Any) -> Optional[str]: """Serialize DB create_time to an ISO string for API consumers.""" if create_time is None: @@ -112,9 +93,13 @@ def _serialize_created_at(create_time: Any) -> Optional[str]: return str(create_time) -def _to_summary_item(record: Dict[str, Any]) -> Dict[str, Any]: +def _to_summary_item( + record: Dict[str, Any], + *, + can_take_down: Optional[bool] = None, +) -> Dict[str, Any]: """Map a DB record to a lightweight skill marketplace summary item.""" - return { + item = { "id": record.get("skill_repository_id"), "skill_repository_id": record.get("skill_repository_id"), "skill_id": record.get("skill_id"), @@ -130,6 +115,9 @@ def _to_summary_item(record: Dict[str, Any]) -> Dict[str, Any]: "created_at": record.get("created_at") or _serialize_created_at(record.get("create_time")), "updated_at": record.get("updated_at") or _serialize_created_at(record.get("update_time")), } + if can_take_down is not None: + item["can_take_down"] = can_take_down + return item def _to_detail_item( @@ -260,21 +248,6 @@ def _get_user_role(user_id: str) -> str: return str(user_tenant.get("user_role") or "USER") -def _resolve_mine_skill_permission( - *, - skill: Dict[str, Any], - user_id: str, - user_role: str, -) -> str: - """Resolve list-item permission for skill repository mine view.""" - if user_role in CAN_EDIT_ALL_USER_ROLES: - return PERMISSION_EDIT - if str(skill.get("created_by")) == str(user_id): - return PERMISSION_EDIT - ingroup_permission = skill.get("ingroup_permission") - return ingroup_permission if ingroup_permission is not None else PERMISSION_READ - - def _can_publish_skill( *, skill: Dict[str, Any], @@ -290,24 +263,6 @@ def _can_publish_skill( ) -def _can_view_mine_skill( - *, - skill: Dict[str, Any], - user_id: str, - user_role: str, - user_group_ids: set[int], -) -> bool: - """Return whether a skill should appear in the current user's mine list.""" - if user_role in CAN_EDIT_ALL_USER_ROLES: - return True - if str(skill.get("created_by")) == str(user_id): - return True - if skill.get("ingroup_permission") == PERMISSION_PRIVATE: - return False - skill_group_ids = _to_group_id_set(skill.get("group_ids")) - return bool(user_group_ids.intersection(skill_group_ids)) - - def _resolve_submitter_email(user_id: str) -> Optional[str]: """Resolve submitter email from user_tenant_t for pending_review listings.""" user_tenant = get_user_tenant_by_user_id(user_id) or {} @@ -804,6 +759,7 @@ def install_skill_from_repository_impl( source="repository", user_id=user_id, tenant_id=tenant_id, + ingroup_permission=PERMISSION_READ, ) except SkillException as exc: message = str(exc) @@ -887,11 +843,7 @@ def _to_mine_skill_item( "created_by": skill.get("created_by"), "created_at": skill.get("create_time"), "updated_at": skill.get("update_time"), - "permission": _resolve_mine_skill_permission( - skill=skill, - user_id=user_id, - user_role=user_role, - ), + "permission": skill.get("permission"), "can_publish": _can_publish_skill( skill=skill, user_id=user_id, @@ -923,17 +875,10 @@ def list_my_editable_skills_impl( safe_page_size = max(int(page_size or 10), 1) user_role = _get_user_role(user_id) - user_group_ids = set(query_group_ids_by_user(user_id) or []) - skills = [ - skill - for skill in SkillService(tenant_id=tenant_id).list_skills(tenant_id=tenant_id) - if _can_view_mine_skill( - skill=skill, - user_id=user_id, - user_role=user_role, - user_group_ids=user_group_ids, - ) - ] + skills = SkillService(tenant_id=tenant_id).list_visible_skills( + tenant_id=tenant_id, + user_id=user_id, + ) counts = _count_skills_by_ownership(skills, user_id) filtered_skills = [ @@ -982,6 +927,7 @@ def list_my_editable_skills_impl( def list_skill_repository_listings_impl( tenant_id: str, *, + user_id: str, status: Optional[str] = None, skill_id: Optional[int] = None, category_id: Optional[int] = None, @@ -1007,8 +953,24 @@ def list_skill_repository_listings_impl( search=search, sort_by_update_time=sort_by_update_time, ) + user_role = _get_user_role(user_id) return { - "items": [_to_summary_item(record) for record in result.get("items", [])], + "items": [ + _to_summary_item( + record, + can_take_down=( + record.get("status") == STATUS_SHARED + and ( + user_role in ("ADMIN", "SU") + or ( + user_role == "DEV" + and str(record.get("publisher_user_id")) == str(user_id) + ) + ) + ), + ) + for record in result.get("items", []) + ], "pagination": result.get("pagination"), } diff --git a/backend/services/skill_service.py b/backend/services/skill_service.py index b8a684d505..bf21561ecb 100644 --- a/backend/services/skill_service.py +++ b/backend/services/skill_service.py @@ -27,6 +27,7 @@ OFFICIAL_SKILLS_ZIP_PATH, PERMISSION_EDIT, PERMISSION_PRIVATE, + PERMISSION_READ, ROOT_DIR, ) from consts.exceptions import ForbiddenError, SkillException @@ -43,6 +44,62 @@ _skill_manager: Optional[SkillManager] = None +def _to_group_id_set(group_ids: Any) -> set[int]: + if isinstance(group_ids, str): + return { + int(group_id.strip()) + for group_id in group_ids.split(",") + if group_id.strip().isdigit() + } + if isinstance(group_ids, list): + return { + int(group_id) + for group_id in group_ids + if str(group_id).strip().isdigit() + } + return set() + + +def can_view_skill( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, + user_group_ids: set[int], +) -> bool: + """Return whether a skill is available to the current user.""" + if user_role in CAN_EDIT_ALL_USER_ROLES: + return True + if str(skill.get("created_by")) == str(user_id): + return True + if skill.get("ingroup_permission") == PERMISSION_PRIVATE: + return False + return bool( + user_group_ids.intersection(_to_group_id_set(skill.get("group_ids"))) + ) + + +def resolve_skill_permission( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, + user_group_ids: set[int], +) -> str: + """Resolve whether the current user can edit or only use a visible skill.""" + if user_role in CAN_EDIT_ALL_USER_ROLES: + return PERMISSION_EDIT + if str(skill.get("created_by")) == str(user_id): + return PERMISSION_EDIT + if skill.get("ingroup_permission") != PERMISSION_EDIT: + return PERMISSION_READ + return ( + PERMISSION_EDIT + if user_group_ids.intersection(_to_group_id_set(skill.get("group_ids"))) + else PERMISSION_READ + ) + + def _apply_default_skill_permission_fields( skill_data: Dict[str, Any], user_id: Optional[str], @@ -68,17 +125,14 @@ def _get_user_role(user_id: Optional[str]) -> str: def _can_edit_skill(skill: Dict[str, Any], user_id: Optional[str]) -> bool: if not user_id: return False - if _get_user_role(user_id) in CAN_EDIT_ALL_USER_ROLES: - return True - if str(skill.get("created_by")) == str(user_id): - return True - if skill.get("ingroup_permission") in (None, PERMISSION_PRIVATE): - return False - if skill.get("ingroup_permission") != PERMISSION_EDIT: - return False - skill_group_ids = {int(group_id) for group_id in skill.get("group_ids") or []} + user_role = _get_user_role(user_id) user_group_ids = set(query_group_ids_by_user(user_id) or []) - return bool(skill_group_ids.intersection(user_group_ids)) + return resolve_skill_permission( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) == PERMISSION_EDIT def _normalize_zip_entry_path(name: str) -> str: @@ -994,6 +1048,34 @@ def list_skills(self, tenant_id: Optional[str] = None) -> List[Dict[str, Any]]: logger.error(f"Error listing skills: {e}") raise SkillException(f"Failed to list skills: {str(e)}") from e + def list_visible_skills( + self, + *, + tenant_id: Optional[str] = None, + user_id: str, + ) -> List[Dict[str, Any]]: + """List skills visible to a user and attach the resolved permission.""" + user_role = _get_user_role(user_id) + user_group_ids = set(query_group_ids_by_user(user_id) or []) + visible_skills = [ + skill + for skill in self.list_skills(tenant_id=tenant_id) + if can_view_skill( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) + ] + for skill in visible_skills: + skill["permission"] = resolve_skill_permission( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) + return visible_skills + def get_skill(self, skill_name: str, tenant_id: Optional[str] = None) -> Optional[Dict[str, Any]]: """Get a specific skill within a tenant. @@ -2177,7 +2259,8 @@ def create_skill_from_zip_bytes( source: str = "导入", user_id: Optional[str] = None, tenant_id: Optional[str] = None, - skip_duplicate_check: bool = False + skip_duplicate_check: bool = False, + ingroup_permission: Optional[str] = None, ) -> Dict[str, Any]: """Create a skill from ZIP bytes, optionally skipping the duplicate name check. @@ -2192,6 +2275,7 @@ def create_skill_from_zip_bytes( user_id: Creator user ID tenant_id: Tenant ID skip_duplicate_check: If True, skip the "skill already exists" check + ingroup_permission: Optional group permission override for the new skill Returns: Created skill dict @@ -2295,6 +2379,8 @@ def create_skill_from_zip_bytes( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + if ingroup_permission is not None: + skill_dict["ingroup_permission"] = ingroup_permission _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx index c995ee8988..4dd4d05c0f 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { Alert, Form, Modal, Spin } from "antd"; import { useTranslation } from "react-i18next"; @@ -11,12 +11,15 @@ import type { SkillFormData, } from "@/types/skill"; import { + fetchSkillById, fetchSkillFileContent, fetchSkillFiles, SkillFilesAccessDeniedError, } from "@/services/agentConfigService"; import { normalizeSkillFiles } from "@/lib/skillFileUtils"; import log from "@/lib/logger"; +import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; +import { useGroupDetails, useGroupList } from "@/hooks/group/useGroupList"; import SkillDraftPanel from "./SkillDraftPanel"; interface SkillDetailModalProps { @@ -33,7 +36,25 @@ export default function SkillDetailModal({ onClose, }: SkillDetailModalProps) { const { t } = useTranslation("common"); + const { user, getAccessibleGroupIds } = useAuthorizationContext(); const [form] = Form.useForm(); + const { data: groupData } = useGroupList(user?.tenantId ?? null); + const accessibleGroupIds = useMemo( + () => getAccessibleGroupIds(), + [getAccessibleGroupIds] + ); + const { groups: filteredGroups } = useGroupDetails( + groupData?.groups ?? [], + accessibleGroupIds + ); + const groupSelectOptions = useMemo( + () => + filteredGroups.map((group) => ({ + label: group.group_name, + value: group.group_id, + })), + [filteredGroups] + ); const [skillTabs, setSkillTabs] = useState([ { path: "SKILL.md", content: "" }, ]); @@ -51,6 +72,8 @@ export default function SkillDetailModal({ source: formatSource(skill.source, t), tags: Array.isArray(skill.tags) ? skill.tags : [], content: skill.content || "", + group_ids: skill.group_ids || [], + ingroup_permission: skill.ingroup_permission || undefined, }); setSkillTabs([{ path: "SKILL.md", content: skill.content || "" }]); setActiveSkillTab("SKILL.md"); @@ -59,10 +82,29 @@ export default function SkillDetailModal({ const loadFiles = async () => { setLoading(true); try { - const files = await fetchSkillFiles(skill.name); + const detailResult = await fetchSkillById( + skill.skill_id, + skill.tenant_id + ); + const detail = + detailResult.success && detailResult.data ? detailResult.data : skill; + const skillName = detail.name?.trim() || skill.name; + if (!cancelled) { + form.setFieldsValue({ + name: skillName, + description: detail.description || "", + source: formatSource(detail.source, t), + tags: Array.isArray(detail.tags) ? detail.tags : [], + content: detail.content || "", + group_ids: Array.isArray(detail.group_ids) ? detail.group_ids : [], + ingroup_permission: detail.ingroup_permission || undefined, + }); + } + + const files = await fetchSkillFiles(skillName); const flatFiles = flattenSkillFiles( normalizeSkillFiles(files), - skill.name + skillName ); if (flatFiles.length === 0) { return; @@ -71,7 +113,7 @@ export default function SkillDetailModal({ const tabs = await Promise.all( flatFiles.map(async (path) => { try { - const content = await fetchSkillFileContent(skill.name, path); + const content = await fetchSkillFileContent(skillName, path); return { path, content: content || "" }; } catch (error) { log.error("Failed to load skill file content:", error); @@ -104,7 +146,7 @@ export default function SkillDetailModal({ return () => { cancelled = true; }; - }, [open, skill?.skill_id, form, t]); + }, [open, skill, form, t]); const handleClose = () => { setSkillTabs([{ path: "SKILL.md", content: "" }]); @@ -145,6 +187,7 @@ export default function SkillDetailModal({ setSkillTabs={setSkillTabs} activeSkillTab={activeSkillTab} setActiveSkillTab={setActiveSkillTab} + groupSelectOptions={groupSelectOptions} readOnly /> diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx index a506b92243..6274da629e 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx @@ -3,7 +3,7 @@ import { useState, useEffect } from "react"; import { useTranslation } from "react-i18next"; import { SkillGroup, Skill, SkillParam } from "@/types/agentConfig"; -import { Tabs, message, Tooltip, Badge } from "antd"; +import { Badge, Button, message, Tabs, Tooltip } from "antd"; import { useAgentConfigStore } from "@/stores/agentConfigStore"; import { useSkillList } from "@/hooks/agent/useSkillList"; import { Info, Trash2, Settings } from "lucide-react"; @@ -277,13 +277,22 @@ export default function SkillManagement({ }`} onClick={isReadOnly ? undefined : (e) => handleInfoClick(skill, e)} /> - handleDeleteClick(skill, e)} - /> + + )} - {canPublish ? ( - - ) : null} + + + + + diff --git a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx index a040d9fa12..12320f3055 100644 --- a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx +++ b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx @@ -78,7 +78,7 @@ export function RepositoryView({ key={listing.skill_repository_id} listing={listing} onDetailClick={() => onDetailClick(listing)} - showAdminMenu={showAdminMenu} + showAdminMenu={showAdminMenu || listing.can_take_down === true} isTakingDown={ takingDownRepositoryId === listing.skill_repository_id } diff --git a/frontend/hooks/agent/useSkillList.ts b/frontend/hooks/agent/useSkillList.ts index b0b101a98a..5f69e7259a 100644 --- a/frontend/hooks/agent/useSkillList.ts +++ b/frontend/hooks/agent/useSkillList.ts @@ -2,7 +2,6 @@ import { useQuery, useQueryClient } from "@tanstack/react-query"; import { fetchSkills } from "@/services/agentConfigService"; import { useMemo } from "react"; import { Skill, SkillGroup } from "@/types/agentConfig"; -import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; import { useTranslation } from "react-i18next"; const OFFICIAL_SKILL_SOURCES = new Set(["official", "官方"]); @@ -17,7 +16,6 @@ export function useSkillList(options?: { staleTime?: number; }) { const queryClient = useQueryClient(); - const { user } = useAuthorizationContext(); const { t } = useTranslation("common"); const query = useQuery({ @@ -34,14 +32,7 @@ export function useSkillList(options?: { }); const skills = query.data ?? []; - const currentUserId = user?.id ?? null; - - const availableSkills = useMemo(() => { - return skills.filter((skill: Skill) => { - if (isOfficialSkill(skill)) return true; - return Boolean(currentUserId && skill.created_by === currentUserId); - }); - }, [skills, currentUserId]); + const availableSkills = skills; const groupedSkills = useMemo(() => { const groups: SkillGroup[] = []; diff --git a/frontend/public/locales/en/common.json b/frontend/public/locales/en/common.json index 780ad89a3b..1825e93c28 100644 --- a/frontend/public/locales/en/common.json +++ b/frontend/public/locales/en/common.json @@ -1365,6 +1365,7 @@ "skillManagement.delete.confirmTitle": "Confirm Delete Skill", "skillManagement.delete.confirmContent": "Are you sure you want to delete skill 「{{skillName}}」? This action cannot be undone.", "skillManagement.delete.success": "Skill deleted successfully", + "skillManagement.noEditPermission": "No permission to edit this skill", "skillManagement.delete.failed": "Failed to delete skill", "mcpConfig.modal.title": "MCP Server Configuration", "mcpConfig.modal.close": "Close", diff --git a/frontend/public/locales/zh/common.json b/frontend/public/locales/zh/common.json index 3bfebc2464..7095a80de7 100644 --- a/frontend/public/locales/zh/common.json +++ b/frontend/public/locales/zh/common.json @@ -1336,6 +1336,7 @@ "skillManagement.delete.confirmTitle": "确认删除技能", "skillManagement.delete.confirmContent": "确定要删除技能「{{skillName}}」吗？删除后无法恢复。", "skillManagement.delete.success": "技能删除成功", + "skillManagement.noEditPermission": "无技能编辑权限", "skillManagement.delete.failed": "技能删除失败", "mcpConfig.modal.title": "MCP服务器配置", "mcpConfig.modal.close": "关闭", diff --git a/frontend/services/agentConfigService.ts b/frontend/services/agentConfigService.ts index 516ae700a9..29f11441ed 100644 --- a/frontend/services/agentConfigService.ts +++ b/frontend/services/agentConfigService.ts @@ -1093,6 +1093,11 @@ export const fetchSkills = async (tenantId?: string | null) => { config_schemas: skill.config_schemas ?? null, config_values: skill.config_values ?? null, tool_ids: Array.isArray(skill.tool_ids) ? skill.tool_ids.map(Number) : [], + group_ids: Array.isArray(skill.group_ids) + ? skill.group_ids.map(Number) + : [], + ingroup_permission: skill.ingroup_permission ?? null, + permission: skill.permission ?? "READ_ONLY", created_by: skill.created_by ?? null, updated_by: skill.updated_by ?? null, update_time: skill.update_time, diff --git a/frontend/types/agentConfig.ts b/frontend/types/agentConfig.ts index aa9bdab6bd..c2feafa723 100644 --- a/frontend/types/agentConfig.ts +++ b/frontend/types/agentConfig.ts @@ -214,6 +214,9 @@ export interface Skill { config_schemas?: SkillParam[] | null; config_values?: Record | null; tool_ids?: number[]; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; + permission?: "EDIT" | "READ_ONLY"; created_by?: string | null; updated_by?: string | null; update_time?: string; diff --git a/frontend/types/skillRepository.ts b/frontend/types/skillRepository.ts index 605593ebf3..b8703685c5 100644 --- a/frontend/types/skillRepository.ts +++ b/frontend/types/skillRepository.ts @@ -20,6 +20,7 @@ export interface SkillRepositoryListingItem { category_id?: number | null; author?: string | null; submitted_by?: string | null; + can_take_down?: boolean; } export interface SkillRepositoryListingPagination { diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 9d4f2cdecd..02606c5200 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -271,7 +271,7 @@ def test_list_skills_success(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [ + mock_service.list_visible_skills.return_value = [ {"skill_id": 1, "name": "skill1", "description": "Desc1"}, {"skill_id": 2, "name": "skill2", "description": "Desc2"} ] @@ -294,7 +294,7 @@ def test_list_skills_empty(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [] + mock_service.list_visible_skills.return_value = [] app = FastAPI() app.include_router(skill_app.router) @@ -314,7 +314,7 @@ def test_list_skills_error(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.side_effect = SkillException("Database error") + mock_service.list_visible_skills.side_effect = SkillException("Database error") app = FastAPI() app.include_router(skill_app.router) @@ -331,7 +331,7 @@ def test_list_skills_super_admin_with_tenant_id(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [ + mock_service.list_visible_skills.return_value = [ {"skill_id": 10, "name": "admin_skill", "description": "Admin desc"} ] @@ -349,7 +349,10 @@ def test_list_skills_super_admin_with_tenant_id(self, mocker): assert "skills" in data assert len(data["skills"]) == 1 # Verify the service was called with the target tenant_id, not super_tenant - mock_service.list_skills.assert_called_once_with(tenant_id="target_tenant") + mock_service.list_visible_skills.assert_called_once_with( + tenant_id="target_tenant", + user_id="super_user", + ) # ===== Create Skill Endpoint Tests ===== @@ -1099,7 +1102,7 @@ def test_unexpected_error_in_list_skills(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.side_effect = Exception("Unexpected error") + mock_service.list_visible_skills.side_effect = Exception("Unexpected error") app = FastAPI() app.include_router(skill_app.router) diff --git a/test/backend/app/test_skill_repository_app.py b/test/backend/app/test_skill_repository_app.py index 8171bb05e7..60f75a4522 100644 --- a/test/backend/app/test_skill_repository_app.py +++ b/test/backend/app/test_skill_repository_app.py @@ -95,6 +95,7 @@ def test_list_skill_repository_listings_api_passes_filters(mocker, mock_auth_hea mock_get_user_id.assert_called_once_with(mock_auth_header["Authorization"]) mock_list.assert_called_once_with( "tenant-1", + user_id="user-1", status="pending_review", skill_id=3, category_id=2, diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index a923fd94ab..d7074e9b09 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -132,6 +132,29 @@ def create_skill_from_zip_bytes(self, **kwargs): def list_skills(self, tenant_id=None): return [] + def list_visible_skills(self, *, tenant_id=None, user_id): + user_tenant = _user_tenant_db_mock.get_user_tenant_by_user_id(user_id) or {} + user_role = str(user_tenant.get("user_role") or "USER") + user_group_ids = set(_group_db_mock.query_group_ids_by_user(user_id) or []) + skills = [ + skill + for skill in self.list_skills(tenant_id=tenant_id) + if user_role in {"ADMIN", "SUPER_ADMIN"} + or str(skill.get("created_by")) == str(user_id) + or ( + skill.get("ingroup_permission") != "PRIVATE" + and bool(user_group_ids.intersection(skill.get("group_ids") or [])) + ) + ] + for skill in skills: + skill["permission"] = ( + "EDIT" + if user_role in {"ADMIN", "SUPER_ADMIN"} + or str(skill.get("created_by")) == str(user_id) + else skill.get("ingroup_permission") or "READ_ONLY" + ) + return skills + _skill_service_module_mock = MagicMock() _skill_service_module_mock.SkillService = _SkillServiceMock @@ -352,21 +375,30 @@ def test_update_status_dev_cannot_update_other_users_listing(): def test_install_skill_from_repository_success_increments_downloads(): + create_kwargs = {} + + class CapturingSkillService(_SkillServiceMock): + def create_skill_from_zip_bytes(self, **kwargs): + create_kwargs.update(kwargs) + return super().create_skill_from_zip_bytes(**kwargs) + encoded_zip = base64.b64encode(b"zip").decode("ascii") _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = { **_repository_record(status="shared"), "skill_zip_base64": encoded_zip, } - result = srs.install_skill_from_repository_impl( - skill_repository_id=1, - tenant_id="tenant-1", - user_id="user-1", - target_name="Skill A Copy", - ) + with patch.object(srs, "SkillService", CapturingSkillService): + result = srs.install_skill_from_repository_impl( + skill_repository_id=1, + tenant_id="tenant-1", + user_id="user-1", + target_name="Skill A Copy", + ) assert result["name"] == "Skill A Copy" assert result["source"] == "repository" + assert create_kwargs["ingroup_permission"] == "READ_ONLY" _skill_repo_db_mock.increment_skill_repository_downloads.assert_called_once_with( repository_id=1, user_id="user-1", @@ -456,7 +488,7 @@ def list_skills(self, tenant_id=None): "get_user_tenant_by_user_id", return_value={"user_role": "DEV"}, ), - patch.object(srs, "query_group_ids_by_user", return_value=[1]), + patch.object(_group_db_mock, "query_group_ids_by_user", return_value=[1]), ): created_result = srs.list_my_editable_skills_impl( tenant_id="tenant-1", @@ -481,6 +513,7 @@ def test_list_repository_listings_validates_status(): with pytest.raises(ValueError): srs.list_skill_repository_listings_impl( "tenant-1", + user_id="user-1", status="bad_status", ) @@ -640,9 +673,18 @@ def test_repository_list_and_detail_success(): } result = srs.list_skill_repository_listings_impl( "tenant-1", + user_id="user-1", status="shared", ) assert result["items"][0]["status"] == "shared" + assert result["items"][0]["can_take_down"] is True + + result = srs.list_skill_repository_listings_impl( + "tenant-1", + user_id="user-2", + status="shared", + ) + assert result["items"][0]["can_take_down"] is False _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = ( _repository_record(status="shared") @@ -676,11 +718,6 @@ def test_mapping_and_filter_helpers_cover_edge_branches(): assert srs._paginate_mine_skills_with_optional_padding([], 1, 10, False) == ([], 0) _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = None assert srs._get_user_role("missing-user") == "USER" - assert srs._resolve_mine_skill_permission( - skill={"created_by": "someone-else"}, - user_id="admin-1", - user_role="ADMIN", - ) == srs.PERMISSION_EDIT assert srs._normalize_listing_tags(None) == [] with pytest.raises(ValueError, match="icon is required"): srs._validate_card_fields({"icon": 123}) diff --git a/test/backend/services/test_skill_service.py b/test/backend/services/test_skill_service.py index db81d9271f..b31e609c34 100644 --- a/test/backend/services/test_skill_service.py +++ b/test/backend/services/test_skill_service.py @@ -170,6 +170,7 @@ def get_cached_message(self): consts_const_mock.CAN_EDIT_ALL_USER_ROLES = {"ADMIN"} consts_const_mock.PERMISSION_EDIT = "EDIT" consts_const_mock.PERMISSION_PRIVATE = "PRIVATE" +consts_const_mock.PERMISSION_READ = "READ_ONLY" consts_exceptions_mock = types.ModuleType('consts.exceptions') class SkillException(Exception): @@ -550,6 +551,96 @@ def test_list_skills_error(self, mocker): with pytest.raises(Exception): service.list_skills() + def test_list_skills_filters_by_creator_group_and_private_permission(self, mocker): + mocker.patch( + 'backend.services.skill_service.skill_db.list_skills', + return_value=[ + { + "skill_id": 1, + "name": "own-private", + "created_by": "user-1", + "group_ids": [], + "ingroup_permission": "PRIVATE", + }, + { + "skill_id": 2, + "name": "group-read-only", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "READ_ONLY", + }, + { + "skill_id": 3, + "name": "group-edit", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "EDIT", + }, + { + "skill_id": 4, + "name": "group-private", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "PRIVATE", + }, + { + "skill_id": 5, + "name": "different-group", + "created_by": "user-2", + "group_ids": [20], + "ingroup_permission": "EDIT", + }, + ], + ) + mocker.patch( + 'backend.services.skill_service.query_group_ids_by_user', + return_value=[10], + ) + mocker.patch( + 'backend.services.skill_service.get_user_tenant_by_user_id', + return_value={"user_role": "DEV"}, + ) + + result = create_test_service().list_visible_skills(user_id="user-1") + + assert [skill["name"] for skill in result] == [ + "own-private", + "group-read-only", + "group-edit", + ] + assert [skill["permission"] for skill in result] == [ + "EDIT", + "READ_ONLY", + "EDIT", + ] + + def test_list_skills_admin_can_view_all_tenant_skills(self, mocker): + mocker.patch( + 'backend.services.skill_service.skill_db.list_skills', + return_value=[ + { + "skill_id": 1, + "name": "private-skill", + "created_by": "user-2", + "group_ids": [], + "ingroup_permission": "PRIVATE", + } + ], + ) + mocker.patch( + 'backend.services.skill_service.query_group_ids_by_user', + return_value=[], + ) + mocker.patch( + 'backend.services.skill_service.get_user_tenant_by_user_id', + return_value={"user_role": "ADMIN"}, + ) + + result = create_test_service().list_visible_skills(user_id="admin-1") + + assert len(result) == 1 + assert result[0]["permission"] == "EDIT" + class TestSkillServiceGetSkill: """Test SkillService.get_skill method.""" From bb82afd4368b8ff8e4d13e6d29d9911a734a673c Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Tue, 21 Jul 2026 19:06:41 +0800 Subject: [PATCH 03/10] fix: align skill sorting and agent config editing --- backend/database/skill_db.py | 3 ++ .../agents/components/AgentConfigComp.tsx | 17 ++++++- .../agentConfig/SkillManagement.tsx | 19 +++++--- frontend/app/[locale]/skill-space/page.tsx | 48 ------------------- test/backend/database/test_skill_db.py | 8 +++- 5 files changed, 38 insertions(+), 57 deletions(-) diff --git a/backend/database/skill_db.py b/backend/database/skill_db.py index 14bfc73653..492474f726 100644 --- a/backend/database/skill_db.py +++ b/backend/database/skill_db.py @@ -268,6 +268,9 @@ def list_skills(tenant_id: str) -> List[Dict[str, Any]]: skills = session.query(SkillInfo).filter( SkillInfo.tenant_id == tenant_id, SkillInfo.delete_flag != 'Y' + ).order_by( + SkillInfo.create_time.desc(), + SkillInfo.skill_id.desc(), ).all() results = [] for s in skills: diff --git a/frontend/app/[locale]/agents/components/AgentConfigComp.tsx b/frontend/app/[locale]/agents/components/AgentConfigComp.tsx index 4868bbfaf2..7b2111692b 100644 --- a/frontend/app/[locale]/agents/components/AgentConfigComp.tsx +++ b/frontend/app/[locale]/agents/components/AgentConfigComp.tsx @@ -17,6 +17,8 @@ import { useSkillList } from "@/hooks/agent/useSkillList"; import { useExternalAgents } from "@/hooks/agent/useExternalAgents"; import McpConfigModal from "./agentConfig/McpConfigModal"; import A2AAgentDiscoveryModal from "./a2a/A2AAgentDiscoveryModal"; +import type { Skill } from "@/types/agentConfig"; +import type { MyEditableSkillItem } from "@/types/skillRepository"; import { Wrench, RefreshCw, Lightbulb, Plug, BlocksIcon, Globe } from "lucide-react"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; @@ -36,6 +38,9 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { const [isMcpModalOpen, setIsMcpModalOpen] = useState(false); const [isSkillModalOpen, setIsSkillModalOpen] = useState(false); + const [editingSkill, setEditingSkill] = useState( + null + ); const [isRefreshing, setIsRefreshing] = useState(false); const [isRefreshingSkill, setIsRefreshingSkill] = useState(false); const [showA2ADiscovery, setShowA2ADiscovery] = useState(false); @@ -83,6 +88,11 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { invalidateSkills(); }, [invalidateSkills]); + const handleEditSkill = useCallback((skill: Skill) => { + setEditingSkill({ ...skill, repository_info: [] }); + setIsSkillModalOpen(true); + }, []); + return ( <> {/* Import handled by Ant Design Upload (no hidden input required) */} @@ -254,6 +264,7 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { isCreatingMode={isCreatingMode} currentAgentId={currentAgentId ?? undefined} isReadOnly={isReadOnly} + onEditSkill={handleEditSkill} /> @@ -279,7 +290,11 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { setIsSkillModalOpen(false)} + editingSkill={editingSkill} + onCancel={() => { + setIsSkillModalOpen(false); + setEditingSkill(null); + }} onSuccess={handleSkillBuildSuccess} /> diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx index 6274da629e..8b70cb3eb3 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx @@ -6,7 +6,7 @@ import { SkillGroup, Skill, SkillParam } from "@/types/agentConfig"; import { Badge, Button, message, Tabs, Tooltip } from "antd"; import { useAgentConfigStore } from "@/stores/agentConfigStore"; import { useSkillList } from "@/hooks/agent/useSkillList"; -import { Info, Trash2, Settings } from "lucide-react"; +import { Eye, Pencil, Trash2, Settings } from "lucide-react"; import { useConfirmModal } from "@/hooks/useConfirmModal"; import { deleteSkill, fetchSkillInstances } from "@/services/agentConfigService"; import log from "@/lib/logger"; @@ -18,6 +18,7 @@ interface SkillManagementProps { isCreatingMode?: boolean; currentAgentId?: number | undefined; isReadOnly?: boolean; + onEditSkill?: (skill: Skill) => void; } export default function SkillManagement({ @@ -25,6 +26,7 @@ export default function SkillManagement({ isCreatingMode, currentAgentId, isReadOnly: isReadOnlyProp, + onEditSkill, }: SkillManagementProps) { const { t } = useTranslation("common"); const { confirm } = useConfirmModal(); @@ -136,6 +138,10 @@ export default function SkillManagement({ const handleInfoClick = (skill: Skill, e: React.MouseEvent) => { e.stopPropagation(); + if (!isReadOnly && skill.permission === "EDIT" && onEditSkill) { + onEditSkill(skill); + return; + } setSelectedSkill(skill); setIsDetailModalOpen(true); }; @@ -244,6 +250,9 @@ export default function SkillManagement({ > {group.skills.map((skill) => { const isSelected = originalSelectedSkillIdsSet.has(skill.skill_id); + const canEditSkill = + !isReadOnly && skill.permission === "EDIT" && onEditSkill != null; + const SkillActionIcon = canEditSkill ? Pencil : Eye; const hasConfigurableParams = Array.isArray(skill.config_schemas) && skill.config_schemas.length > 0; @@ -270,12 +279,10 @@ export default function SkillManagement({ onClick={isReadOnly ? undefined : (e) => handleConfigClick(skill, e)} /> )} - handleInfoClick(skill, e)} + className="cursor-pointer text-gray-400 hover:text-gray-600 transition-colors" + onClick={(e) => handleInfoClick(skill, e)} /> - (skill?.repository_info ?? []).filter( - (info) => info.status === "pending_review" - ); - - const confirmEditListedSkill = async ( - skill: MyEditableSkillItem - ): Promise => { - const pendingInfo = getPendingRepositoryInfo(skill); - if (pendingInfo.length === 0) { - return true; - } - - const confirmed = await new Promise((resolve) => { - modal.confirm({ - title: t("skillRepository.edit.confirmWithdrawTitle"), - content: t("skillRepository.edit.confirmWithdrawContent"), - okText: t("skillRepository.edit.continueSave"), - cancelText: t("common.cancel"), - onOk: () => resolve(true), - onCancel: () => resolve(false), - }); - }); - if (!confirmed) { - return false; - } - - try { - await Promise.all( - pendingInfo.map((info) => - updateStatusMutation.mutateAsync({ - skillRepositoryId: info.skill_repository_id, - status: "not_shared", - }) - ) - ); - return true; - } catch (error) { - message.error( - error instanceof Error - ? error.message - : t("skillRepository.common.statusUpdateFailed") - ); - return false; - } - }; - const handleSkillBuildSuccess = async () => { await refetchMine().catch(() => {}); setEditingSkill(null); @@ -658,7 +611,6 @@ export default function SkillRepositoryPage() { setEditingSkill(null); }} onSuccess={handleSkillBuildSuccess} - onBeforeEditSave={confirmEditListedSkill} /> Date: Tue, 21 Jul 2026 19:44:18 +0800 Subject: [PATCH 04/10] test: isolate skill service module mocks --- backend/apps/skill_app.py | 3 +-- test/backend/app/test_skill_app.py | 15 +++++++++++---- .../services/test_skill_repository_service.py | 6 +++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/backend/apps/skill_app.py b/backend/apps/skill_app.py index 3ef683cbdd..2ef7b794eb 100644 --- a/backend/apps/skill_app.py +++ b/backend/apps/skill_app.py @@ -17,6 +17,7 @@ stream_skill_creation, update_skill_list, get_official_skills_with_status, + install_skills_from_zip_for_tenant, ) from consts.model import SkillInstanceInfoRequest, SkillCreateRequest, SkillCreateInteractiveRequest, SkillUpdateRequest, SkillResponse from utils.auth_utils import get_current_user_id, get_current_user_info @@ -127,8 +128,6 @@ async def install_skills( """ try: user_id, current_tenant_id = get_current_user_id(authorization) - from services.skill_service import install_skills_from_zip_for_tenant - effective_tenant_id = tenant_id if tenant_id else current_tenant_id installed_names = install_skills_from_zip_for_tenant( skill_names=request.skill_names, diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 02606c5200..420f14f9a7 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -202,6 +202,13 @@ def __init__(self): services_skill_service_mock.update_skill_list = MagicMock() services_skill_service_mock.get_official_skills_with_status = MagicMock(return_value=[]) services_skill_service_mock.install_skills_from_zip_for_tenant = MagicMock(return_value=[]) + + +def setup_function(): + """Restore module-level service stubs after tests that isolate imports.""" + sys.modules['services'] = services_mock + sys.modules['services.skill_service'] = services_skill_service_mock + sys.modules['services.asset_owner_visibility'] = services_asset_owner_visibility_mock services_asset_owner_visibility_mock.can_view_skill = MagicMock(return_value=True) # Mock utils @@ -2278,7 +2285,7 @@ def test_install_skills_success(self, mocker): """Test successful skill installation.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = ["skill1", "skill2"] app = FastAPI() @@ -2305,7 +2312,7 @@ def test_install_skills_empty_list(self, mocker): """Test installing empty skill list.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = [] app = FastAPI() @@ -2344,7 +2351,7 @@ def test_install_skills_super_admin_with_tenant_id(self, mocker): """Test super admin installing skills for a specific tenant via tenant_id query param.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("super_user", "super_tenant") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = ["skill1"] app = FastAPI() @@ -2371,7 +2378,7 @@ def test_install_skills_error(self, mocker): """Test installing skills with error.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.side_effect = Exception("Installation failed") app = FastAPI() diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index d7074e9b09..a9a48f6bc6 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -60,11 +60,11 @@ if not hasattr(consts_const_module, "CAN_EDIT_ALL_USER_ROLES"): consts_const_module.CAN_EDIT_ALL_USER_ROLES = {"ADMIN"} if not hasattr(consts_const_module, "PERMISSION_EDIT"): - consts_const_module.PERMISSION_EDIT = "edit" + consts_const_module.PERMISSION_EDIT = "EDIT" if not hasattr(consts_const_module, "PERMISSION_READ"): - consts_const_module.PERMISSION_READ = "read" + consts_const_module.PERMISSION_READ = "READ_ONLY" if not hasattr(consts_const_module, "PERMISSION_PRIVATE"): - consts_const_module.PERMISSION_PRIVATE = "private" + consts_const_module.PERMISSION_PRIVATE = "PRIVATE" _skill_repo_db_mock = MagicMock() _skill_repo_db_mock.get_skill_repository_by_id_and_publisher = MagicMock() From 698fe7ffc1b2cc71b5ca6c78984d3131a68e765c Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Wed, 22 Jul 2026 14:27:33 +0800 Subject: [PATCH 05/10] chore: move skill migration to v2.4.0 --- ....4.0_0722_add_skill_permission_and_repository_snapshots.sql} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename deploy/sql/migrations/{v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql => v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql} (98%) diff --git a/deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql b/deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql similarity index 98% rename from deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql rename to deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql index 1abd9020d5..e158a31030 100644 --- a/deploy/sql/migrations/v2.3.0_0713_add_skill_permission_and_repository_snapshots.sql +++ b/deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql @@ -1,5 +1,5 @@ -- Migration: Add skill group permissions and allow separate repository snapshots by status --- Date: 2026-07-13 +-- Date: 2026-07-22 -- Description: Align skill ownership and repository status behavior with agent repository semantics. SET search_path TO nexent; From 117557480f829b49f8fb3466fd747944fa05c625 Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Wed, 22 Jul 2026 15:32:03 +0800 Subject: [PATCH 06/10] fix: hide group settings in read-only skill detail --- .../agentConfig/SkillDraftPanel.tsx | 100 +++++++++--------- 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx index f1f85a53b4..3aed88a153 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx @@ -227,55 +227,57 @@ export default function SkillDraftPanel({ - - - - - - - - - + {!readOnly ? ( + + + + + + + + + + ) : null} Date: Wed, 22 Jul 2026 17:17:34 +0800 Subject: [PATCH 07/10] fix: refine skill detail editor and quality checks --- backend/apps/skill_app.py | 10 +- backend/database/db_models.py | 7 +- backend/services/skill_service.py | 7 +- .../agentConfig/SkillDetailModal.tsx | 71 +++++--------- .../agentConfig/SkillDraftPanel.tsx | 9 +- .../skill-space/components/MineSkillsView.tsx | 94 +++++++++++++------ 6 files changed, 110 insertions(+), 88 deletions(-) diff --git a/backend/apps/skill_app.py b/backend/apps/skill_app.py index 2ef7b794eb..a7b6710e29 100644 --- a/backend/apps/skill_app.py +++ b/backend/apps/skill_app.py @@ -315,7 +315,10 @@ async def get_skill_file_content( raise HTTPException(status_code=500, detail="Internal server error") -@router.put("/{skill_name}/upload") +@router.put( + "/{skill_name}/upload", + responses={403: {"description": "Not authorized to update this skill"}}, +) async def update_skill_from_file( skill_name: str, file: UploadFile = File(..., description="SKILL.md file or ZIP archive"), @@ -610,7 +613,10 @@ async def get_skill(skill_name: str, authorization: Optional[str] = Header(None) raise HTTPException(status_code=500, detail="Internal server error") -@router.put("/{skill_name}") +@router.put( + "/{skill_name}", + responses={403: {"description": "Not authorized to update this skill"}}, +) async def update_skill( skill_name: str, request: SkillUpdateRequest, diff --git a/backend/database/db_models.py b/backend/database/db_models.py index 39ff0f7cc8..ac43006e3f 100644 --- a/backend/database/db_models.py +++ b/backend/database/db_models.py @@ -16,6 +16,7 @@ _PUBLISHER_TENANT_ID_DOC = "Publisher tenant ID" _PUBLISHER_USER_ID_DOC = "Publisher user ID" _MCP_NAME_DOC = "MCP name" +_INGROUP_PERMISSION_DOC = "In-group permission: EDIT, READ_ONLY, PRIVATE" # Base class for tables without audit fields @@ -611,7 +612,7 @@ class AgentInfo(TableBase): group_ids = Column(String, doc="Agent group IDs list") is_new = Column(Boolean, default=False, doc="Whether this agent is marked as new for the user") current_version_no = Column(Integer, nullable=True, doc="Current published version number. NULL means no version published yet") - ingroup_permission = Column(String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") + ingroup_permission = Column(String(30), doc=_INGROUP_PERMISSION_DOC) requested_output_tokens = Column( Integer, doc=( @@ -707,7 +708,7 @@ class KnowledgeRecord(TableBase): tenant_id = Column(String(100), doc="Tenant ID") group_ids = Column(String, doc="Knowledge base group IDs list") ingroup_permission = Column( - String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") + String(30), doc=_INGROUP_PERMISSION_DOC) summary_frequency = Column(String(10), nullable=True, doc="Auto-summary frequency: '3h', '5h', '1d', '1w', or NULL (disabled)") last_summary_time = Column(TIMESTAMP(timezone=False), nullable=True, @@ -1201,7 +1202,7 @@ class SkillInfo(TableBase): source = Column(String(30), nullable=False, default="official", doc="Skill source: official, custom, etc.") group_ids = Column(String, doc="Skill group IDs list") - ingroup_permission = Column(String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") + ingroup_permission = Column(String(30), doc=_INGROUP_PERMISSION_DOC) class SkillToolRelation(TableBase): diff --git a/backend/services/skill_service.py b/backend/services/skill_service.py index bf21561ecb..6d14436add 100644 --- a/backend/services/skill_service.py +++ b/backend/services/skill_service.py @@ -40,6 +40,7 @@ from utils.str_utils import convert_list_to_string logger = logging.getLogger(__name__) +_SKILL_UPDATE_FORBIDDEN_MESSAGE = "Not authorized to update this skill" _skill_manager: Optional[SkillManager] = None @@ -1583,7 +1584,7 @@ def update_skill_from_file( if not existing: raise SkillException(f"Skill not found: {skill_name}") if user_id is not None and not _can_edit_skill(existing, user_id): - raise ForbiddenError("Not authorized to update this skill") + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) content_bytes: bytes if isinstance(file_content, str): @@ -1755,7 +1756,7 @@ def update_skill( if not existing: raise SkillException(f"Skill not found: {skill_name}") if user_id is not None and not _can_edit_skill(existing, user_id): - raise ForbiddenError("Not authorized to update this skill") + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) result = skill_db.update_skill( skill_name, skill_data, effective_tenant_id, updated_by=user_id or None @@ -1830,7 +1831,7 @@ def update_skill_by_id( if not existing: raise SkillException(f"Skill not found: {skill_id}") if not _can_edit_skill(existing, user_id): - raise ForbiddenError("Not authorized to update this skill") + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) local_dir = self._resolve_local_skills_dir_for_overlay() if local_dir and "name" in skill_data: diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx index 4dd4d05c0f..39517b52ae 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useMemo, useState } from "react"; +import { useEffect, useState } from "react"; import { Alert, Form, Modal, Spin } from "antd"; import { useTranslation } from "react-i18next"; @@ -18,8 +18,6 @@ import { } from "@/services/agentConfigService"; import { normalizeSkillFiles } from "@/lib/skillFileUtils"; import log from "@/lib/logger"; -import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; -import { useGroupDetails, useGroupList } from "@/hooks/group/useGroupList"; import SkillDraftPanel from "./SkillDraftPanel"; interface SkillDetailModalProps { @@ -30,31 +28,29 @@ interface SkillDetailModalProps { const OFFICIAL_SOURCES = new Set(["official", "\u5b98\u65b9"]); +async function loadSkillFileTabs(skillName: string): Promise { + const files = await fetchSkillFiles(skillName); + const paths = flattenSkillFiles(normalizeSkillFiles(files), skillName); + return Promise.all( + paths.map(async (path) => { + try { + const content = await fetchSkillFileContent(skillName, path); + return { path, content: content || "" }; + } catch (error) { + log.error("Failed to load skill file content:", error); + return { path, content: "" }; + } + }) + ); +} + export default function SkillDetailModal({ skill, open, onClose, }: SkillDetailModalProps) { const { t } = useTranslation("common"); - const { user, getAccessibleGroupIds } = useAuthorizationContext(); const [form] = Form.useForm(); - const { data: groupData } = useGroupList(user?.tenantId ?? null); - const accessibleGroupIds = useMemo( - () => getAccessibleGroupIds(), - [getAccessibleGroupIds] - ); - const { groups: filteredGroups } = useGroupDetails( - groupData?.groups ?? [], - accessibleGroupIds - ); - const groupSelectOptions = useMemo( - () => - filteredGroups.map((group) => ({ - label: group.group_name, - value: group.group_id, - })), - [filteredGroups] - ); const [skillTabs, setSkillTabs] = useState([ { path: "SKILL.md", content: "" }, ]); @@ -72,8 +68,6 @@ export default function SkillDetailModal({ source: formatSource(skill.source, t), tags: Array.isArray(skill.tags) ? skill.tags : [], content: skill.content || "", - group_ids: skill.group_ids || [], - ingroup_permission: skill.ingroup_permission || undefined, }); setSkillTabs([{ path: "SKILL.md", content: skill.content || "" }]); setActiveSkillTab("SKILL.md"); @@ -96,35 +90,15 @@ export default function SkillDetailModal({ source: formatSource(detail.source, t), tags: Array.isArray(detail.tags) ? detail.tags : [], content: detail.content || "", - group_ids: Array.isArray(detail.group_ids) ? detail.group_ids : [], - ingroup_permission: detail.ingroup_permission || undefined, }); } - const files = await fetchSkillFiles(skillName); - const flatFiles = flattenSkillFiles( - normalizeSkillFiles(files), - skillName - ); - if (flatFiles.length === 0) { - return; - } + const tabs = await loadSkillFileTabs(skillName); - const tabs = await Promise.all( - flatFiles.map(async (path) => { - try { - const content = await fetchSkillFileContent(skillName, path); - return { path, content: content || "" }; - } catch (error) { - log.error("Failed to load skill file content:", error); - return { path, content: "" }; - } - }) - ); - - if (!cancelled) { - setSkillTabs(sortSkillTabs(tabs)); - setActiveSkillTab(sortSkillTabs(tabs)[0]?.path || "SKILL.md"); + if (!cancelled && tabs.length > 0) { + const sortedTabs = sortSkillTabs(tabs); + setSkillTabs(sortedTabs); + setActiveSkillTab(sortedTabs[0]?.path || "SKILL.md"); } } catch (error) { if (cancelled) return; @@ -187,7 +161,6 @@ export default function SkillDetailModal({ setSkillTabs={setSkillTabs} activeSkillTab={activeSkillTab} setActiveSkillTab={setActiveSkillTab} - groupSelectOptions={groupSelectOptions} readOnly /> diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx index 3aed88a153..74d060c272 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx @@ -325,7 +325,9 @@ export default function SkillDraftPanel({ mode="tags" maxCount={MAX_SKILL_TAGS} suffixIcon={null} - placeholder={t("skillManagement.form.tagsPlaceholder")} + placeholder={ + readOnly ? "-" : t("skillManagement.form.tagsPlaceholder") + } open={false} onInputKeyDown={handleTagInputKeyDown} onChange={() => { @@ -491,7 +493,8 @@ export default function SkillDraftPanel({ setExpandedEditorContent(""); }} centered - width={640} + width="min(960px, calc(100vw - 48px))" + className="expanded-file-editor" styles={{ body: { padding: 0 } }} footer={ readOnly @@ -535,7 +538,7 @@ export default function SkillDraftPanel({ if (readOnly) return; setExpandedEditorContent(e.target.value); }} - autoSize={{ minRows: 10, maxRows: 28 }} + autoSize={{ minRows: 16, maxRows: 32 }} className="rounded-none border-0 font-mono text-sm shadow-none focus:border-0 focus:shadow-none" style={{ resize: "none" }} /> diff --git a/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx b/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx index 076a8e61ca..8c22860e28 100644 --- a/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx +++ b/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx @@ -276,6 +276,58 @@ const MINE_SKILL_STATUS_CLASS: Record = { "bg-emerald-50 text-emerald-700 dark:bg-emerald-500/10 dark:text-emerald-300", }; +function getApplyButtonLabel( + isPendingReview: boolean, + hasSharedRepository: boolean, + repositoryStatus: SkillRepositoryListingStatus, + t: (key: string) => string +) { + if (isPendingReview) { + return getSkillRepositoryStatusLabel(t, repositoryStatus); + } + return hasSharedRepository + ? t("skillRepository.mine.button.reapply") + : t("skillRepository.mine.button.apply"); +} + +function getMineSkillMenuItems({ + canPublish, + hasRepositoryInfo, + isPendingReview, + t, + onViewReview, + onDelete, +}: { + canPublish: boolean; + hasRepositoryInfo: boolean; + isPendingReview: boolean; + t: (key: string) => string; + onViewReview: () => void; + onDelete: () => void; +}): MenuProps["items"] { + const items: MenuProps["items"] = []; + if (canPublish && hasRepositoryInfo) { + items.push({ + key: "review", + label: t( + isPendingReview + ? "skillRepository.mine.viewReviewProgress" + : "skillRepository.mine.viewRepositoryStatus" + ), + icon: , + onClick: onViewReview, + }); + } + items.push({ + key: "delete", + label: t("common.delete"), + icon: , + danger: true, + onClick: onDelete, + }); + return items; +} + function MineSkillCard({ skill, onEdit, @@ -309,34 +361,20 @@ function MineSkillCard({ const tags = skill.tags?.filter((tag) => tag.trim()) ?? []; const isPendingReview = repositoryStatus === "pending_review"; const canApplyListing = canPublish && !isPendingReview; - const applyButtonLabel = isPendingReview - ? getSkillRepositoryStatusLabel(t, repositoryStatus) - : hasSharedRepository - ? t("skillRepository.mine.button.reapply") - : t("skillRepository.mine.button.apply"); - const menuItems: MenuProps["items"] = [ - ...(canPublish && hasRepositoryInfo - ? [ - { - key: "review", - label: t( - isPendingReview - ? "skillRepository.mine.viewReviewProgress" - : "skillRepository.mine.viewRepositoryStatus" - ), - icon: , - onClick: onViewReview, - }, - ] - : []), - { - key: "delete", - label: t("common.delete"), - icon: , - danger: true, - onClick: onDelete, - }, - ]; + const applyButtonLabel = getApplyButtonLabel( + isPendingReview, + hasSharedRepository, + repositoryStatus, + t + ); + const menuItems = getMineSkillMenuItems({ + canPublish, + hasRepositoryInfo, + isPendingReview, + t, + onViewReview, + onDelete, + }); return (
From 578d5dbb16eb4f9d179f796d1b6977aec26ed3ce Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Thu, 23 Jul 2026 09:22:55 +0800 Subject: [PATCH 08/10] add test case --- test/backend/app/test_skill_app.py | 23 ++++ .../database/test_skill_repository_db.py | 21 +++- .../services/test_skill_repository_service.py | 18 +++ test/backend/services/test_skill_service.py | 113 ++++++++++++++++++ 4 files changed, 174 insertions(+), 1 deletion(-) diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 420f14f9a7..2740b4461f 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -919,6 +919,29 @@ def test_update_skill_not_found(self, mocker): assert response.status_code == 404 + def test_update_skill_from_file_forbidden(self, mocker): + from backend.apps.skill_app import ForbiddenError + + with patch('backend.apps.skill_app.SkillService') as mock_service_class: + with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: + mock_auth.return_value = ("user123", "tenant123") + mock_service = MagicMock() + mock_service_class.return_value = mock_service + mock_service.update_skill_from_file.side_effect = ForbiddenError( + "Not authorized" + ) + + app = FastAPI() + app.include_router(skill_app.router) + client = TestClient(app) + response = client.put( + "/skills/test/upload", + files={"file": ("test.md", b"# Skill", "text/markdown")}, + headers={"Authorization": "Bearer token123"}, + ) + + assert response.status_code == 403 + # ===== Update Skill Instance Endpoint Tests ===== class TestUpdateSkillInstanceEndpoint: diff --git a/test/backend/database/test_skill_repository_db.py b/test/backend/database/test_skill_repository_db.py index 1e26b8ec5e..c5eac4fef5 100644 --- a/test/backend/database/test_skill_repository_db.py +++ b/test/backend/database/test_skill_repository_db.py @@ -115,8 +115,27 @@ def test_get_repository_by_skill_id_with_optional_tenant(monkeypatch, mock_sessi assert repo_db.get_skill_repository_by_skill_id( 8, publisher_tenant_id="tenant-1", + statuses={"pending_review", "rejected"}, ) == {"skill_id": 8} - assert query.filter.call_count == 2 + assert query.filter.call_count == 3 + query.order_by.assert_called_once_with(repo_db.SkillRepository.update_time.desc()) + + +def test_reset_repository_status_resets_matching_peer_records(monkeypatch, mock_session): + session, _ = mock_session + session.execute.return_value.rowcount = 2 + _patch_session(monkeypatch, session) + statement = _patch_update(monkeypatch) + + affected = repo_db.reset_skill_repository_status( + repository_id=5, + skill_id=8, + status="pending_review", + publisher_tenant_id="tenant-1", + ) + + assert affected == 2 + assert statement.values.call_args.kwargs == {"status": "not_shared"} def test_list_repository_summaries_with_filters(monkeypatch, mock_session): diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index a9a48f6bc6..b7223e7050 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -698,6 +698,24 @@ def test_repository_list_and_detail_success(): assert detail["author"] is None +def test_repository_list_does_not_grant_take_down_to_regular_user(): + _skill_repo_db_mock.list_skill_repository_summaries.return_value = { + "items": [_repository_record(status="shared")], + "pagination": {"total": 1}, + } + _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = { + "user_role": "USER" + } + + result = srs.list_skill_repository_listings_impl( + "tenant-1", + user_id="user-1", + status="shared", + ) + + assert result["items"][0]["can_take_down"] is False + + def test_mapping_and_filter_helpers_cover_edge_branches(): created_at = datetime(2026, 1, 1, 12, 0, 0) assert srs._serialize_created_at(created_at) == created_at.isoformat() diff --git a/test/backend/services/test_skill_service.py b/test/backend/services/test_skill_service.py index b31e609c34..0a88845fea 100644 --- a/test/backend/services/test_skill_service.py +++ b/test/backend/services/test_skill_service.py @@ -382,6 +382,119 @@ def create_test_service(tenant_id="test-tenant"): # ===== Helper Functions Tests ===== +class TestSkillGroupPermissions: + def test_group_permission_helpers_handle_edit_read_only_and_private(self): + group_skill = { + "created_by": "creator", + "group_ids": "10,invalid,20", + "ingroup_permission": "EDIT", + } + + assert skill_service._to_group_id_set(group_skill["group_ids"]) == {10, 20} + assert skill_service.can_view_skill( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) is True + assert skill_service.resolve_skill_permission( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) == "EDIT" + + group_skill["ingroup_permission"] = "READ_ONLY" + assert skill_service.resolve_skill_permission( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) == "READ_ONLY" + + group_skill["ingroup_permission"] = "PRIVATE" + assert skill_service.can_view_skill( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) is False + + def test_group_permission_helpers_preserve_creator_and_admin_access(self): + private_skill = { + "created_by": "creator", + "group_ids": [], + "ingroup_permission": "PRIVATE", + } + + assert skill_service.can_view_skill( + skill=private_skill, + user_id="creator", + user_role="DEV", + user_group_ids=set(), + ) is True + assert skill_service.resolve_skill_permission( + skill=private_skill, + user_id="admin", + user_role="ADMIN", + user_group_ids=set(), + ) == "EDIT" + + def test_group_permission_helpers_reject_unmatched_groups_and_normalize_lists(self): + skill = { + "created_by": "creator", + "group_ids": [10, "20", "invalid"], + "ingroup_permission": "EDIT", + } + + assert skill_service._to_group_id_set(skill["group_ids"]) == {10, 20} + assert skill_service._to_group_id_set(None) == set() + assert skill_service.can_view_skill( + skill=skill, + user_id="outsider", + user_role="DEV", + user_group_ids={30}, + ) is False + assert skill_service.resolve_skill_permission( + skill=skill, + user_id="outsider", + user_role="DEV", + user_group_ids={30}, + ) == "READ_ONLY" + + def test_default_group_permission_does_not_override_explicit_values(self, mocker): + skill_data = { + "group_ids": [99], + "ingroup_permission": "READ_ONLY", + } + query_groups = mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10], + ) + + skill_service._apply_default_skill_permission_fields(skill_data, "user-1") + + assert skill_data == { + "group_ids": [99], + "ingroup_permission": "READ_ONLY", + } + query_groups.assert_not_called() + + def test_default_group_permission_uses_creator_groups(self, mocker): + skill_data = {} + mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10, 20], + ) + + skill_service._apply_default_skill_permission_fields(skill_data, "user-1") + + assert skill_data == { + "group_ids": "10,20", + "ingroup_permission": "EDIT", + } + + class TestNormalizeZipEntryPath: """Test _normalize_zip_entry_path function.""" From 60b6a4dfb6950c649c9fe224648ba5594450f54b Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Thu, 23 Jul 2026 10:16:23 +0800 Subject: [PATCH 09/10] fix test case --- test/sdk/core/agents/test_nexent_agent.py | 66 ++++++++++++++--------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/test/sdk/core/agents/test_nexent_agent.py b/test/sdk/core/agents/test_nexent_agent.py index ab54d347f5..f0784a731d 100644 --- a/test/sdk/core/agents/test_nexent_agent.py +++ b/test/sdk/core/agents/test_nexent_agent.py @@ -4065,19 +4065,23 @@ def test_create_builtin_tool_run_skill_script(self, nexent_agent_instance): }, ) - # Mock the skill script tool module - mock_run_skill_script = MagicMock() - mock_get_tool = MagicMock(return_value=mock_run_skill_script) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.run_skill_script_tool": MagicMock( - get_run_skill_script_tool=mock_get_tool, - run_skill_script=mock_run_skill_script + RunSkillScriptTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_run_skill_script - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + observer=nexent_agent_instance.observer, + ) def test_create_builtin_tool_read_skill_md(self, nexent_agent_instance): """Test create_builtin_tool with ReadSkillMdTool.""" @@ -4096,18 +4100,22 @@ def test_create_builtin_tool_read_skill_md(self, nexent_agent_instance): }, ) - mock_read_skill_md = MagicMock() - mock_get_tool = MagicMock(return_value=mock_read_skill_md) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.read_skill_md_tool": MagicMock( - get_read_skill_md_tool=mock_get_tool, - read_skill_md=mock_read_skill_md + ReadSkillMdTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_read_skill_md - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_write_skill_file(self, nexent_agent_instance): """Test create_builtin_tool with WriteSkillFileTool.""" @@ -4126,18 +4134,22 @@ def test_create_builtin_tool_write_skill_file(self, nexent_agent_instance): }, ) - mock_write_skill_file = MagicMock() - mock_get_tool = MagicMock(return_value=mock_write_skill_file) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.write_skill_file_tool": MagicMock( - get_write_skill_file_tool=mock_get_tool, - write_skill_file=mock_write_skill_file + WriteSkillFileTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_write_skill_file - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_read_skill_config(self, nexent_agent_instance): """Test create_builtin_tool with ReadSkillConfigTool.""" @@ -4156,18 +4168,22 @@ def test_create_builtin_tool_read_skill_config(self, nexent_agent_instance): }, ) - mock_read_skill_config = MagicMock() - mock_get_tool = MagicMock(return_value=mock_read_skill_config) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.read_skill_config_tool": MagicMock( - get_read_skill_config_tool=mock_get_tool, - read_skill_config=mock_read_skill_config + ReadSkillConfigTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_read_skill_config - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_unknown_tool(self, nexent_agent_instance): """Test create_builtin_tool raises ValueError for unknown tool.""" From 8dffe3d5ec5dba7a389deae1958d527da0376e1d Mon Sep 17 00:00:00 2001 From: Summer-Si Date: Thu, 23 Jul 2026 11:07:11 +0800 Subject: [PATCH 10/10] add test case --- test/backend/app/test_skill_app.py | 8 +++ test/backend/database/test_skill_db.py | 14 +++++ .../database/test_skill_repository_db.py | 11 ++++ .../services/test_skill_repository_service.py | 8 +++ test/backend/services/test_skill_service.py | 59 ++++++++++++++++++- 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 2dd39a4c25..e5d66ac6ec 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -2861,6 +2861,8 @@ def test_build_skill_update_data_with_all_fields(self, mocker): request.config_schemas = {"key": "value"} request.config_values = {"param": "val"} request.files = [mock_file_data] + request.group_ids = [10, 20] + request.ingroup_permission = "READ_ONLY" result = _build_skill_update_data(request) @@ -2872,6 +2874,8 @@ def test_build_skill_update_data_with_all_fields(self, mocker): assert result["config_schemas"] == {"key": "value"} assert result["config_values"] == {"param": "val"} assert result["files"] == [{"path": "test.py", "content": "code"}] + assert result["group_ids"] == [10, 20] + assert result["ingroup_permission"] == "READ_ONLY" def test_build_skill_update_data_with_partial_fields(self, mocker): """Test _build_skill_update_data with only some fields provided.""" @@ -2886,6 +2890,8 @@ def test_build_skill_update_data_with_partial_fields(self, mocker): request.config_schemas = None request.config_values = None request.files = None + request.group_ids = None + request.ingroup_permission = None result = _build_skill_update_data(request) @@ -2906,6 +2912,8 @@ def test_build_skill_update_data_with_empty_files(self, mocker): request.config_schemas = None request.config_values = None request.files = [] # Empty list is not None, so it gets included + request.group_ids = None + request.ingroup_permission = None result = _build_skill_update_data(request) diff --git a/test/backend/database/test_skill_db.py b/test/backend/database/test_skill_db.py index 82a814ce42..a951317843 100644 --- a/test/backend/database/test_skill_db.py +++ b/test/backend/database/test_skill_db.py @@ -2288,6 +2288,20 @@ def test_config_fields(self): assert "config_schemas" in result assert "config_values" in result + def test_group_permission_fields(self): + result = _build_skill_update_values( + {"group_ids": [10, 20], "ingroup_permission": "READ_ONLY"}, + "admin", + ) + + assert result["group_ids"] == "10,20" + assert result["ingroup_permission"] == "READ_ONLY" + + def test_group_ids_string_is_preserved(self): + result = _build_skill_update_values({"group_ids": "10,20"}, "admin") + + assert result["group_ids"] == "10,20" + def test_empty_skill_data(self): result = _build_skill_update_values({}, "user1") assert "update_time" in result diff --git a/test/backend/database/test_skill_repository_db.py b/test/backend/database/test_skill_repository_db.py index c5eac4fef5..419ca3d573 100644 --- a/test/backend/database/test_skill_repository_db.py +++ b/test/backend/database/test_skill_repository_db.py @@ -121,6 +121,17 @@ def test_get_repository_by_skill_id_with_optional_tenant(monkeypatch, mock_sessi query.order_by.assert_called_once_with(repo_db.SkillRepository.update_time.desc()) +def test_get_repository_by_skill_id_without_status_filter(monkeypatch, mock_session): + session, query = mock_session + query.filter.return_value = query + query.order_by.return_value = query + query.first.return_value = None + _patch_session(monkeypatch, session) + + assert repo_db.get_skill_repository_by_skill_id(8) is None + assert query.filter.call_count == 1 + + def test_reset_repository_status_resets_matching_peer_records(monkeypatch, mock_session): session, _ = mock_session session.execute.return_value.rowcount = 2 diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index b7223e7050..95a3d7c783 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -697,6 +697,14 @@ def test_repository_list_and_detail_success(): detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") assert detail["author"] is None + record_without_creator = _repository_record(status="shared") + record_without_creator["skill_info_json"]["created_by"] = None + _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = record_without_creator + _user_tenant_db_mock.get_user_tenant_by_user_id.reset_mock() + detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") + assert detail["author"] is None + _user_tenant_db_mock.get_user_tenant_by_user_id.assert_not_called() + def test_repository_list_does_not_grant_take_down_to_regular_user(): _skill_repo_db_mock.list_skill_repository_summaries.return_value = { diff --git a/test/backend/services/test_skill_service.py b/test/backend/services/test_skill_service.py index e1837edffb..a322ede0be 100644 --- a/test/backend/services/test_skill_service.py +++ b/test/backend/services/test_skill_service.py @@ -515,6 +515,35 @@ def test_default_group_permission_uses_creator_groups(self, mocker): "ingroup_permission": "EDIT", } + def test_default_group_permission_skips_anonymous_user(self, mocker): + query_groups = mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + ) + + skill_data = {} + skill_service._apply_default_skill_permission_fields(skill_data, None) + + assert skill_data == {} + query_groups.assert_not_called() + + def test_can_edit_skill_requires_user_and_allows_group_editor(self, mocker): + skill = { + "created_by": "owner", + "group_ids": [10], + "ingroup_permission": "EDIT", + } + mocker.patch( + "backend.services.skill_service.get_user_tenant_by_user_id", + return_value={"user_role": "DEV"}, + ) + mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10], + ) + + assert skill_service._can_edit_skill(skill, None) is False + assert skill_service._can_edit_skill(skill, "group-editor") is True + class TestNormalizeZipEntryPath: """Test _normalize_zip_entry_path function.""" @@ -2199,6 +2228,35 @@ def test_update_from_file_rejects_user_without_edit_permission(self, mocker): user_id="viewer", ) + def test_update_from_file_allows_user_with_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + can_edit = mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=True, + ) + mocker.patch( + "backend.services.skill_service.skill_db.update_skill", + return_value={"skill_id": 1, "name": "existing"}, + ) + mocker.patch( + "backend.services.skill_service.skill_db.get_tool_ids_by_names", + return_value=[], + ) + service = SkillService(tenant_id="test-tenant") + service.skill_manager = MagicMock() + service._enrich_configs_from_yaml = lambda result: result + service.update_skill_from_file( + "existing", + b"---\nname: existing\n---\n# Content", + file_type="md", + user_id="group-editor", + ) + + can_edit.assert_called_once() + def test_update_from_zip(self, mocker): import zipfile zip_buffer = io.BytesIO() @@ -6068,4 +6126,3 @@ def test_non_dict_value(self): """Test with non-dict/cm value.""" result = skill_service._tooltip_for_commented_map_key("not a map", [], 0, "key") assert result is None -