feat: scriptify native base and file-manager skills#123
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/gemini review |
Summary of ChangesHello @yazelin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取請求旨在將系統中原生的 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| fallback_requested = ( | ||
| script_error == "fallback_required" | ||
| or output_error == "fallback_required" | ||
| or ( | ||
| isinstance(output_json, dict) | ||
| and output_json.get("allow_fallback") is True | ||
| ) |
There was a problem hiding this comment.
fallback_requested 的判斷條件較為複雜，包含多個 or 邏輯。為了提高可讀性，可以考慮將這些條件拆分成獨立的變數，或者使用輔助函數來判斷，使邏輯更清晰。
is_script_error_fallback = script_error == "fallback_required"
is_output_error_fallback = output_error == "fallback_required"
is_allow_fallback_true = isinstance(output_json, dict) and output_json.get("allow_fallback") is True
fallback_requested = is_script_error_fallback or is_output_error_fallback or is_allow_fallback_true
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) |
There was a problem hiding this comment.
在處理 JSON 解析異常時，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊（{exc}）。這可能會洩露系統的內部結構或潛在的敏感資訊。更好的做法是記錄完整的異常堆疊追蹤，並向使用者返回一個更通用、更友好的錯誤訊息。
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| raise ValueError("input 必須是 JSON 物件") | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| return 1 | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| raise ValueError("input 必須是 JSON 物件") | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| return 1 | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except Exception as exc: | ||
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) |
There was a problem hiding this comment.
與 create_share_link.py 中的建議相同，建議避免在錯誤訊息中直接暴露內部異常的詳細資訊。請考慮提供一個更通用、更友好的錯誤訊息。
| print(json.dumps({"success": False, "error": f"invalid_input: {exc}"}, ensure_ascii=False)) | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
已修正 Gemini review comments：\n- scripts 的 JSON parse 錯誤改為通用訊息，不再暴露 exception 細節\n- 拆成具名布林條件以提升可讀性\n- 已跑 ........ [100%] |
|
已推送修正 commit
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on converting the native base and file-manager skills to a script-first approach, aligning them with existing external scripts and reducing the default MCP dependency. The changes include modifying the allowed-tools in SKILL.md to use mcp__ching-tech-os__run_skill_script as the primary entry point, implementing fallback mechanisms using script_mcp_fallback in the metadata, and adding new scripts for core functionalities with input validation and explicit error messages. The changes also include a new test case to verify the script-first configuration and fallback mappings, and another test case to ensure that parameter validation errors in scripts do not trigger a fallback to the MCP tool. Documentation has been updated to reflect the new architecture, including environment variables, project structure, and the role of run_skill_script. Review comments highlight the importance of these changes for enforcing the script-first routing policy, defining fallback boundaries, and providing clear error messages. Some suggestions were made to refactor the logic for determining fallback_requested for better readability, ensure scripts consistently set result["error"] directly, and explicitly state the reason for fallback.
| async def test_native_base_file_manager_script_first(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: | ||
| """native base/file-manager 應為 script-first 並具備 fallback mapping。""" | ||
| external = tmp_path / "external" | ||
| external.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| import ching_tech_os.skills.seed_external as seed_external | ||
| monkeypatch.setattr(seed_external, "ensure_seed_skills", lambda _p: None) | ||
|
|
||
| mgr = SkillManager(skills_dir=SKILLS_DIR, external_skills_dir=external) | ||
| await mgr.load_skills() | ||
|
|
||
| base = await mgr.get_skill("base") | ||
| file_manager = await mgr.get_skill("file-manager") | ||
| assert base is not None | ||
| assert file_manager is not None | ||
| assert base.allowed_tools == ["mcp__ching-tech-os__run_skill_script"] | ||
| assert file_manager.allowed_tools == ["mcp__ching-tech-os__run_skill_script"] | ||
| assert await mgr.has_scripts("base") is True | ||
| assert await mgr.has_scripts("file-manager") is True | ||
|
|
||
| base_map = await mgr.get_script_fallback_map("base") | ||
| fm_map = await mgr.get_script_fallback_map("file-manager") | ||
| assert base_map["get_message_attachments"] == "get_message_attachments" | ||
| assert base_map["summarize_chat"] == "summarize_chat" | ||
| assert base_map["create_share_link"] == "create_share_link" | ||
| assert fm_map["search_nas_files"] == "search_nas_files" | ||
| assert fm_map["get_nas_file_info"] == "get_nas_file_info" | ||
| assert fm_map["prepare_file_message"] == "prepare_file_message" | ||
| assert fm_map["convert_pdf_to_images"] == "convert_pdf_to_images" |
There was a problem hiding this comment.
This new test test_native_base_file_manager_script_first is a critical addition. It directly verifies that the base and file-manager skills are configured as script-first and have the correct fallback mappings, as specified in openspec/changes/scriptify-native-base-file-manager-skills/specs/skill-management/spec.md (lines 16-19) and openspec/changes/scriptify-native-base-file-manager-skills/specs/mcp-tools/spec.md (lines 20-28). This ensures the core functionality of the pull request is working as expected.
| print(json.dumps({ | ||
| "success": False, | ||
| "error": "fallback_required", | ||
| "normalized_input": payload, | ||
| }, ensure_ascii=False)) |
There was a problem hiding this comment.
The script explicitly returns "error": "fallback_required" and "normalized_input": payload. This is a correct implementation of the defined fallback boundary, where the script signals that the MCP tool should be called with the normalized input, as described in openspec/changes/scriptify-native-base-file-manager-skills/design.md (Decision 3).
| print(json.dumps({ | ||
| "success": False, | ||
| "error": "fallback_required", | ||
| "normalized_input": payload, | ||
| }, ensure_ascii=False)) |
There was a problem hiding this comment.
The script explicitly returns "error": "fallback_required" and "normalized_input": payload. This is a correct implementation of the defined fallback boundary, where the script signals that the MCP tool should be called with the normalized input, as described in openspec/changes/scriptify-native-base-file-manager-skills/design.md (Decision 3).
| description: 基礎工具（對話附件、分享連結） | ||
| allowed-tools: get_message_attachments summarize_chat create_share_link Read | ||
| description: 基礎工具（對話附件、分享連結，script-first） | ||
| allowed-tools: mcp__ching-tech-os__run_skill_script |
There was a problem hiding this comment.
The allowed-tools field is now correctly set to mcp__ching-tech-os__run_skill_script. This is a critical change for enforcing the script-first routing policy for the base skill, as outlined in the pull request description and openspec/changes/scriptify-native-base-file-manager-skills/specs/mcp-tools/spec.md (lines 21-23).
| > 目前 native `base`、`file-manager` 也已採 script-first，並以 `script_mcp_fallback` 對應回舊 MCP tool。 | ||
| > fallback 僅在 script 明確回傳 `fallback_required`（或 `allow_fallback: true`）時觸發；參數驗證錯誤不會 fallback。 | ||
| > 仍高度依賴外部 MCP 的 skill（如 `project`、`inventory`、`printer`、`ai-assistant`）建議維持 MCP 模式並獨立規劃模組化遷移。 |
There was a problem hiding this comment.
This new section in the documentation clearly explains the script-first approach for base and file-manager and the conditions for fallback. This is crucial for operators and developers to understand the new routing and fallback logic, directly addressing openspec/changes/scriptify-native-base-file-manager-skills/design.md (Decision 4).
| │ │ ├── base/ # 內建：基礎工具（script-first） | ||
| │ │ ├── file-manager/ # 內建：檔案管理（script-first） |
There was a problem hiding this comment.
Updating the project structure to include base/ and file-manager/ under skills/ with the script-first annotation provides clear documentation of the new architecture. This is a good update for maintainability and understanding the project layout.
|
|
||
| **AI Skills** | ||
| - `run_skill_script` - 執行 Skill 腳本（如 media-downloader、media-transcription） | ||
| - `run_skill_script` - 執行 Skill 腳本（含 native base/file-manager、media-downloader、media-transcription） |
There was a problem hiding this comment.
Updating the description of run_skill_script to explicitly mention native base/file-manager clarifies its expanded role. This ensures that users and developers understand that these native skills are now also handled via scripts, aligning with the overall goal of the pull request.
| script_mcp_fallback: | ||
| get_message_attachments: get_message_attachments | ||
| summarize_chat: summarize_chat | ||
| create_share_link: create_share_link |
There was a problem hiding this comment.
The addition of script_mcp_fallback under metadata.ctos is a good implementation of the fallback mechanism. This clearly defines which scripts should fall back to their corresponding MCP tools, as specified in the pull request description and openspec/changes/scriptify-native-base-file-manager-skills/design.md (Decision 3).
| skills/base/ ← 內建：基礎工具（script-first） | ||
| skills/file-manager/ ← 內建：檔案管理（script-first） |
There was a problem hiding this comment.
Adding skills/base/ and skills/file-manager/ to the module index with the script-first annotation is a good documentation update. It helps developers quickly locate these modules and understand their new implementation strategy.
| @@ -0,0 +1,2 @@ | |||
| schema: spec-driven | |||
| created: 2026-02-24 | |||
There was a problem hiding this comment.
The created date is set to 2026-02-24. While this is a valid date, it's in the future. It's generally best practice to use the actual creation date of the document or the current date to maintain accuracy.
There was a problem hiding this comment.
Code Review
此拉取請求成功地將 base 和 file-manager 技能轉換為 script-first 模式，並為其核心功能引入了新的 Python 腳本。這項變更符合將 allowed-tools 統一為 mcp__ching-tech-os__run_skill_script 的目標，並明確定義了 fallback 邊界，即僅在腳本明確要求時才執行 fallback。相關的測試和文件也已更新，以反映這些變更。總體而言，這是一個結構良好且經過深思熟慮的改動，有助於降低預設 MCP 依賴並提升部署彈性。
然而，在新增的 Python 腳本中，存在一些錯誤處理和一致性方面的改進空間，這將有助於提高代碼的健壯性和可預測性。
| payload = json.loads(raw) | ||
| if not isinstance(payload, dict): | ||
| raise ValueError("input 必須是 JSON 物件") | ||
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
There was a problem hiding this comment.
這裡存在兩個問題：
- 捕獲所有
Exception過於寬泛，應縮小為json.JSONDecodeError。 - 當
payload不是字典時，拋出ValueError會導致腳本以非 JSON 格式的錯誤訊息退出，這與腳本始終輸出 JSON 的預期不符。所有錯誤都應以 JSON 格式輸出。
| payload = json.loads(raw) | |
| if not isinstance(payload, dict): | |
| raise ValueError("input 必須是 JSON 物件") | |
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| try: | |
| payload = json.loads(raw) | |
| if not isinstance(payload, dict): | |
| print(json.dumps({"success": False, "error": "invalid_input: input 必須是 JSON 物件"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
| payload = json.loads(raw) | ||
| if not isinstance(payload, dict): | ||
| raise ValueError("input 必須是 JSON 物件") | ||
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) |
There was a problem hiding this comment.
這裡存在兩個問題：
- 捕獲所有
Exception過於寬泛，應縮小為json.JSONDecodeError。 - 當
payload不是字典時，拋出ValueError會導致腳本以非 JSON 格式的錯誤訊息退出，這與腳本始終輸出 JSON 的預期不符。所有錯誤都應以 JSON 格式輸出。
try:
payload = json.loads(raw)
if not isinstance(payload, dict):
print(json.dumps({"success": False, "error": "invalid_input: input 必須是 JSON 物件"}, ensure_ascii=False))
return 1
except json.JSONDecodeError:
print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False))
return 1| output_json = _parse_json_object(result.get("output", "")) | ||
| script_error = str(result.get("error") or "") | ||
| output_error = ( | ||
| output_json.get("error") | ||
| if isinstance(output_json, dict) and isinstance(output_json.get("error"), str) | ||
| else "" | ||
| ) | ||
| is_script_error_fallback = script_error == "fallback_required" | ||
| is_output_error_fallback = output_error == "fallback_required" | ||
| is_allow_fallback_true = ( | ||
| isinstance(output_json, dict) | ||
| and output_json.get("allow_fallback") is True | ||
| ) | ||
| fallback_requested = ( | ||
| is_script_error_fallback | ||
| or is_output_error_fallback | ||
| or is_allow_fallback_true | ||
| ) |
There was a problem hiding this comment.
這段邏輯引入了多個布林變數來判斷是否需要 fallback。雖然功能正確，但可以考慮將這些條件合併或簡化，以提高可讀性和簡潔性。例如，可以使用一個輔助函數來判斷 fallback_requested，或者將條件直接寫在 if 語句中，避免過多的中間變數。
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
捕獲所有 Exception 過於寬泛，可能會掩蓋其他潛在的程式錯誤。建議將其縮小為更具體的異常類型，例如 json.JSONDecodeError，以確保只處理預期的 JSON 解析錯誤。
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
捕獲所有 Exception 過於寬泛，可能會掩蓋其他潛在的程式錯誤。建議將其縮小為更具體的異常類型，例如 json.JSONDecodeError，以確保只處理預期的 JSON 解析錯誤。
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
捕獲所有 Exception 過於寬泛，可能會掩蓋其他潛在的程式錯誤。建議將其縮小為更具體的異常類型，例如 json.JSONDecodeError，以確保只處理預期的 JSON 解析錯誤。
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
捕獲所有 Exception 過於寬泛，可能會掩蓋其他潛在的程式錯誤。建議將其縮小為更具體的異常類型，例如 json.JSONDecodeError，以確保只處理預期的 JSON 解析錯誤。
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
| except Exception: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
捕獲所有 Exception 過於寬泛，可能會掩蓋其他潛在的程式錯誤。建議將其縮小為更具體的異常類型，例如 json.JSONDecodeError，以確保只處理預期的 JSON 解析錯誤。
| except Exception: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 | |
| except json.JSONDecodeError: | |
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | |
| return 1 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
已針對最新 Gemini review comments 完成第二輪修正（commit
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the base and file-manager native skills to a script-first architecture, successfully unifying AI skill entry points and strengthening the fallback mechanism for improved modularity and maintainability. However, a significant security flaw was identified in the fallback logic of the run_skill_script tool: the authenticated user identity can be spoofed by providing a ctos_user_id in the tool's input, which could lead to privilege escalation. This vulnerability must be addressed before merging. Additionally, consider improving code reusability by refactoring duplicate code found across multiple new scripts.
| raw = sys.stdin.read().strip() | ||
| try: | ||
| payload = json.loads(raw) if raw else {} | ||
| except json.JSONDecodeError: | ||
| print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False)) | ||
| return 1 | ||
|
|
||
| if not isinstance(payload, dict): | ||
| print(json.dumps({"success": False, "error": "invalid_input: input 必須是 JSON 物件"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
這段從 stdin 讀取並解析 JSON 輸入的邏輯，在這次新增的 7 個 skill script 中都有重複。為了提升程式碼的可維護性並避免未來修改時需要同步更新多個檔案，建議將這段共用邏輯提取到一個輔助函式或模組中。
例如，可以考慮在 ching_tech_os/skills/ 目錄下新增一個 script_utils.py 模組來處理通用的腳本任務，例如輸入解析和輸出格式化。
範例 script_utils.py:
import json
import sys
from typing import Any, Callable, Dict
def script_main(handler: Callable[[Dict[str, Any]], tuple[int, Dict[str, Any]]]) -> int:
"""
包裝 skill script 的主函式，統一處理 stdin JSON 輸入解析。
"""
raw = sys.stdin.read().strip()
payload = {}
if raw:
try:
payload = json.loads(raw)
except json.JSONDecodeError:
print(json.dumps({"success": False, "error": "invalid_input: 無效的 JSON 輸入"}, ensure_ascii=False))
return 1
if not isinstance(payload, dict):
print(json.dumps({"success": False, "error": "invalid_input: input 必須是 JSON 物件"}, ensure_ascii=False))
return 1
exit_code, output_data = handler(payload)
print(json.dumps(output_data, ensure_ascii=False))
return exit_code簡化後的 create_share_link.py:
#!/usr/bin/env python3
"""標準化分享連結參數，交由 MCP fallback 執行。"""
import sys
# from ching_tech_os.skills.script_utils import script_main # 假設的路徑
def handle_request(payload: dict) -> tuple[int, dict]:
"""處理請求並回傳 exit_code 和輸出字典。"""
if "resource_type" not in payload or "resource_id" not in payload:
return 1, {"success": False, "error": "缺少 resource_type/resource_id"}
payload.setdefault("expires_in", "24h")
output = {
"success": False,
"error": "fallback_required",
"normalized_input": payload,
}
return 1, output
# if __name__ == "__main__":
# raise SystemExit(script_main(handle_request))這樣做可以讓每個 script 更專注於自身的核心驗證與參數標準化邏輯，也讓未來的維護更加輕鬆。
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
已修正 Gemini 指出的 fallback 身分冒用風險（commit
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully transitions native base and file-manager skills to a script-first approach, enhancing modularity and aligning with existing script-first patterns. The changes include adding new scripts for input normalization and fallback handling, updating documentation, and adding comprehensive tests to ensure correctness and prevent regressions. The introduction of _is_fallback_requested function and the security measure to pop ctos_user_id from fallback_args are good improvements. The detailed OpenSpec documents provide excellent context and justification for these changes.
| # 安全性：禁止使用 input 內注入的 ctos_user_id，僅允許 framework 注入值 | ||
| fallback_args.pop("ctos_user_id", None) |
There was a problem hiding this comment.
The comment 安全性：禁止使用 input 內注入的 ctos_user_id，僅允許 framework 注入值 is a good explanation of the security measure. However, the fallback_args.pop("ctos_user_id", None) line should ideally be placed before the if ctos_user_id is not None: block to ensure that any ctos_user_id from the input is always removed, regardless of whether a valid ctos_user_id is provided by the framework. This makes the intent clearer and prevents potential confusion if the framework's ctos_user_id is None but the input still contains one.
| # 安全性：禁止使用 input 內注入的 ctos_user_id，僅允許 framework 注入值 | |
| fallback_args.pop("ctos_user_id", None) | |
| fallback_args.pop("ctos_user_id", None) | |
| if ctos_user_id is not None: | |
| fallback_args["ctos_user_id"] = ctos_user_id |
| if "resource_type" not in payload or "resource_id" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 resource_type/resource_id"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
The error message for missing resource_type or resource_id is generic. It would be more helpful to specify which of the two is missing, or both if both are missing, to provide clearer feedback to the user.
if "resource_type" not in payload and "resource_id" not in payload:
print(json.dumps({"success": False, "error": "缺少 resource_type 和 resource_id"}, ensure_ascii=False))
return 1
if "resource_type" not in payload:
print(json.dumps({"success": False, "error": "缺少 resource_type"}, ensure_ascii=False))
return 1
if "resource_id" not in payload:
print(json.dumps({"success": False, "error": "缺少 resource_id"}, ensure_ascii=False))
return 1| if "pdf_path" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 pdf_path"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
The error message for missing pdf_path is clear. However, consider adding validation for pages and output_format if they are expected to be in specific formats, even though they have default values. This would make the script more robust against malformed inputs.
if "pdf_path" not in payload:
print(json.dumps({"success": False, "error": "缺少 pdf_path"}, ensure_ascii=False))
return 1
# Optional: Add validation for 'pages' and 'output_format' if specific formats are expected
# For example, if 'pages' must be a string like '1-3' or 'all'
# if 'pages' in payload and not isinstance(payload['pages'], str):
# print(json.dumps({"success": False, "error": "invalid_input: pages 必須是字串"}, ensure_ascii=False))
# return 1| if "file_path" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 file_path"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
The error message for missing file_path is good. For consistency and better user experience, consider adding validation for the format of file_path if there are specific requirements (e.g., must start with shared://).
if "file_path" not in payload:
print(json.dumps({"success": False, "error": "缺少 file_path"}, ensure_ascii=False))
return 1
# Optional: Add validation for the format of 'file_path'
# if not payload["file_path"].startswith("shared://"):
# print(json.dumps({"success": False, "error": "invalid_input: file_path 必須以 shared:// 開頭"}, ensure_ascii=False))
# return 1| if "file_path" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 file_path"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
Similar to get_nas_file_info.py, consider adding validation for the format of file_path to ensure it adheres to expected patterns (e.g., shared://). This would prevent later errors if an invalid path is passed.
if "file_path" not in payload:
print(json.dumps({"success": False, "error": "缺少 file_path"}, ensure_ascii=False))
return 1
# Optional: Add validation for the format of 'file_path'
# if not payload["file_path"].startswith("shared://"):
# print(json.dumps({"success": False, "error": "invalid_input: file_path 必須以 shared:// 開頭"}, ensure_ascii=False))
# return 1| if "keywords" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 keywords"}, ensure_ascii=False)) | ||
| return 1 |
There was a problem hiding this comment.
The error message for missing keywords is clear. If file_types is an optional parameter, it might be beneficial to validate its format if provided (e.g., comma-separated strings).
if "keywords" not in payload:
print(json.dumps({"success": False, "error": "缺少 keywords"}, ensure_ascii=False))
return 1
# Optional: Add validation for 'file_types' if it needs to be a specific format
# if 'file_types' in payload and not isinstance(payload['file_types'], str):
# print(json.dumps({"success": False, "error": "invalid_input: file_types 必須是字串"}, ensure_ascii=False))
# return 1Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
補上 Gemini 提到的重複邏輯整併（commit
/gemini review |
There was a problem hiding this comment.
Code Review
這次的變更相當出色，成功地將 base 和 file-manager 兩個原生 skill 重構為 script-first 模式，這與專案的架構方向一致。整個實作非常穩固，包含了清晰的 fallback 機制，並且修正了一個潛在的安全性問題。
主要亮點：
- 架構一致性：將核心 skill 轉換為 script-first，統一了 skill 的執行模式。
- 安全性提升：在
run_skill_script中，透過過濾並重新注入ctos_user_id，有效地防止了使用者透過input參數進行身分偽冒的風險。 - 錯誤處理優化：更精確地定義了 fallback 的觸發條件，區分了腳本執行失敗與業務邏輯錯誤（如參數驗證失敗），讓錯誤回報更清晰。
- 完整的測試：新增的單元測試與整合測試充分涵蓋了新功能、安全性修復以及 fallback 邏輯，確保了程式碼的品質。
- 詳盡的文件：相關的 README、設計文件和 OpenSpec 都進行了同步更新，這對於專案的維護非常有幫助。
整體而言，這是一次高品質的重構，不僅改善了架構，也提升了系統的安全性與可維護性。
| output_error = ( | ||
| output_json.get("error") | ||
| if isinstance(output_json, dict) and isinstance(output_json.get("error"), str) | ||
| else "" | ||
| ) |
There was a problem hiding this comment.
這裡的邏輯是正確的，但可以寫得更簡潔一些，以提升可讀性。可以將「獲取值」和「檢查類型」兩個步驟分開，讓程式碼意圖更清晰。
建議修改如下：
error_from_json = (output_json or {}).get("error")
output_error = error_from_json if isinstance(error_from_json, str) else ""這樣做的好處是：
- 使用
(output_json or {})避免了對output_json是否為None的單獨檢查。 - 將獲取
error值的邏輯與其類型檢查分開，使每一步都更簡單。
error_from_json = (output_json or {}).get("error")
output_error = error_from_json if isinstance(error_from_json, str) else ""Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
已依最新 Gemini 建議補上可讀性微調（commit
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a script-first approach for the base and file-manager skills, enhancing modularity and reducing MCP dependencies. The changes include new scripts for these skills, modifications to skill routing logic, and updates to documentation. The code has been reviewed for correctness, security, and maintainability, with suggestions provided to improve error handling and code clarity.
backend/tests/test_skill_routing.py
Outdated
| input='{"resource_type":"knowledge","resource_id":"kb-001","ctos_user_id":999}', | ||
| ctos_user_id=123, |
There was a problem hiding this comment.
high: The ctos_user_id is being passed in the input and as a separate argument. This could lead to confusion or unintended behavior. It's better to either pass it only in the input or only as a separate argument, but not both. Passing it as a separate argument is more secure, as it prevents the script from manipulating the user ID.
script="create_share_link",
input='{"resource_type":"knowledge","resource_id":"kb-001"}',
ctos_user_id=123,
backend/tests/test_skill_routing.py
Outdated
| } | ||
|
|
||
| async def fake_execute_tool(_tool_name: str, _arguments: dict) -> str: | ||
| raise AssertionError("invalid_input 不應觸發 fallback") |
There was a problem hiding this comment.
high: Raising an AssertionError directly might not be the best way to handle this in a production environment. Consider using a more specific exception or a custom exception for better error handling and clarity.
async def fake_execute_tool(_tool_name: str, _arguments: dict) -> str:
raise ValueError("invalid_input 不應觸發 fallback")| base_error = result.get("error") or output_error or "script_failed" | ||
| result["error"] = f"{base_error}; fallback 失敗: {fallback_output}" |
There was a problem hiding this comment.
medium: The error message formatting could be improved for better readability and debugging. Consider adding more context to the base_error variable.
| base_error = result.get("error") or output_error or "script_failed" | |
| result["error"] = f"{base_error}; fallback 失敗: {fallback_output}" | |
| base_error = result.get("error") or output_error or "script_failed" # Get the base error message | |
| result["error"] = f"Script {skill}/{script} failed: {base_error}; fallback 失敗: {fallback_output}" # Include script name in error message |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
已再修正 Gemini 最新一輪評論（commit
/gemini review |
There was a problem hiding this comment.
Code Review
這是一個品質很高的 Pull Request，清楚地達成了將 base 和 file-manager skill script 化的目標。整體的程式碼結構、更嚴謹的 fallback 邏輯、以及安全性提升（防止 ctos_user_id 偽冒）都做得很好。測試和文件也相當完整。
我只提出了一項關於程式碼重構的建議，旨在將新加入的 skill scripts 中的重複邏輯抽取到 script_utils.py 中，以提高程式碼的一致性和可維護性。這項修改可以讓未來的 script 開發更為簡潔。
除此之外，這次的變更相當完善，做得很好！
| """Skill scripts 共用工具。""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import sys | ||
|
|
||
|
|
||
| def parse_stdin_json_object() -> tuple[dict | None, str | None]: | ||
| """解析 stdin JSON 物件，回傳 (payload, error_message)。""" | ||
| raw = sys.stdin.read().strip() | ||
| if not raw: | ||
| return {}, None | ||
|
|
||
| try: | ||
| payload = json.loads(raw) | ||
| except json.JSONDecodeError: | ||
| return None, "invalid_input: 無效的 JSON 輸入" | ||
|
|
||
| if not isinstance(payload, dict): | ||
| return None, "invalid_input: input 必須是 JSON 物件" | ||
|
|
||
| return payload, None |
There was a problem hiding this comment.
這是一個很棒的共用工具模組！為了讓各個 skill script 更簡潔並減少重複的樣板程式碼 (boilerplate code)，建議可以在這裡新增幾個輔助函式來封裝 JSON 輸出和程式結束的邏輯。
例如，可以新增 script_error 和 request_fallback 函式。這樣一來，每個獨立的 script 就可以專注在自己的參數驗證和正規化邏輯，而不用處理 json.dumps 和 return 1 的細節。我會在另一個留言中展示如何使用這些新函式。
"""Skill scripts 共用工具。"""
from __future__ import annotations
import json
import sys
def def parse_stdin_json_object() -> tuple[dict | None, str | None]:
"""解析 stdin JSON 物件，回傳 (payload, error_message)。"""
raw = sys.stdin.read().strip()
if not raw:
return {}, None
try:
payload = json.loads(raw)
except json.JSONDecodeError:
return None, "invalid_input: 無效的 JSON 輸入"
if not isinstance(payload, dict):
return None, "invalid_input: input 必須是 JSON 物件"
return payload, None
def script_exit(payload: dict, exit_code: int = 0) -> None:
"""將 JSON payload 印到 stdout 並結束程式。"""
print(json.dumps(payload, ensure_ascii=False))
sys.exit(exit_code)
def script_error(message: str) -> None:
"""印出標準的錯誤 JSON 並以 exit code 1 結束。"""
script_exit({"success": False, "error": message}, 1)
def request_fallback(normalized_input: dict) -> None:
"""印出標準的 fallback 請求 JSON 並以 exit code 1 結束。"""
payload = {
"success": False,
"error": "fallback_required",
"normalized_input": normalized_input,
}
script_exit(payload, 1)| #!/usr/bin/env python3 | ||
| """標準化分享連結參數，交由 MCP fallback 執行。""" | ||
|
|
||
| import json | ||
|
|
||
| from ching_tech_os.skills.script_utils import parse_stdin_json_object | ||
|
|
||
|
|
||
| def main() -> int: | ||
| payload, error = parse_stdin_json_object() | ||
| if error: | ||
| print(json.dumps({"success": False, "error": error}, ensure_ascii=False)) | ||
| return 1 | ||
| payload = payload or {} | ||
|
|
||
| if "resource_type" not in payload or "resource_id" not in payload: | ||
| print(json.dumps({"success": False, "error": "缺少 resource_type/resource_id"}, ensure_ascii=False)) | ||
| return 1 | ||
|
|
||
| payload.setdefault("expires_in", "24h") | ||
| print(json.dumps({ | ||
| "success": False, | ||
| "error": "fallback_required", | ||
| "normalized_input": payload, | ||
| }, ensure_ascii=False)) | ||
| return 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
承接前一個關於 script_utils.py 的建議，如果採用了新的輔助函式，這個 script 可以被大幅簡化，變得更清晰易讀。
修改後的版本會像這樣：
#!/usr/bin/env python3
"""標準化分享連結參數，交由 MCP fallback 執行。"""
from ching_tech_os.skills.script_utils import (
parse_stdin_json_object,
script_error,
request_fallback,
)
def main() -> None:
payload, error = parse_stdin_json_object()
if error:
script_error(error)
payload = payload or {}
if "resource_type" not in payload or "resource_id" not in payload:
script_error("缺少 resource_type/resource_id")
payload.setdefault("expires_in", "24h")
request_fallback(payload)
if __name__ == "__main__":
main()這個模式可以套用到這次 PR 新增的所有 skill scripts (base/* 和 file-manager/*)，讓程式碼更一致且易於維護。
#!/usr/bin/env python3
"""標準化分享連結參數，交由 MCP fallback 執行。"""
from ching_tech_os.skills.script_utils import (
parse_stdin_json_object,
script_error,
request_fallback,
)
def main() -> None:
payload, error = parse_stdin_json_object()
if error:
script_error(error)
payload = payload or {}
if "resource_type" not in payload or "resource_id" not in payload:
script_error("缺少 resource_type/resource_id")
payload.setdefault("expires_in", "24h")
request_fallback(payload)
if __name__ == "__main__":
main()
1 participant
摘要
base、file-managerskill 改為 script-first，allowed-tools統一改為mcp__ching-tech-os__run_skill_scriptfallback_required導向既有 MCP toolfallback_required或allow_fallback: true）才 fallback；參數錯誤不 fallback測試
cd backend && uv run pytest tests/test_skill_routing.py tests/test_skills_manager.py -qcd backend && uv run pytest -q（本地環境既有 2 項 DB 密碼失敗：tests/test_single_tenant.py）npm run buildOpenSpec
scriptify-native-base-file-manager-skills