From 5a530b31ee0b19b70fcf5cc6b386654d6ea42021 Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Tue, 20 Oct 2026 10:00:03 +0900 Subject: [PATCH] fix(skills): answer invalid custom-skill history names with 400 (#7123) * fix(skills): answer invalid custom-skill history names with 400 GET /api/skills/custom/{skill_name}/history let the ValueError from validate_skill_name fall through to its catch-all, so a malformed name returned 500 and logged a traceback on every request, while delete and rollback answer 400. Validate the name up front instead of adding a handler-wide except ValueError: a corrupt history line raises JSONDecodeError, also a ValueError, and must stay a server error. Both outcomes are pinned. * fix(skills): keep corrupt history a 500 on rollback, validate delete names early Sibling sweep of the history fix. Rollback's handler-wide except ValueError also caught json.JSONDecodeError, so a corrupt history record answered 400 with the parser message and no log; it is now a logged 500 like history. Delete only failed an invalid name inside the drained mutation tail, after taking the owner's mutation lock, and logged it as a tail error although _drain_skill_mutation keeps caller 4xx unlogged. It now rejects the name before any storage work. Each guard is pinned by a mutation-checked test. * docs(changelog): reference #7123 for the skill history status fix * fix(skills): treat non-UTF-8 skill history as a server fault on rollback UnicodeDecodeError is a ValueError too, so a non-UTF-8 history file still reached rollback's ValueError -> 400 mapping while GET .../history answered the same file with 500. Catch it alongside JSONDecodeError and pin both corruption shapes on both routes. Also drop the whitespace-only separator lines the changelog merge added between other entries. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 9 +++ CHANGELOG_zh.md | 8 ++ backend/app/gateway/routers/skills.py | 19 +++++ backend/docs/API.md | 3 + .../harness/deerflow/skills/AGENTS.md | 2 +- backend/tests/test_skills_custom_router.py | 81 +++++++++++++++++++ 6 files changed, 121 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d48b1bb345..735aa2660a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1265,6 +1265,14 @@ This release closes that milestone with **621 merged pull requests**. ### Fixed +- **skills:** `GET /api/skills/custom/{skill_name}/history` now answers an + invalid skill name (not hyphen-case, or longer than 64 characters) with `400`, + as delete and rollback already do, instead of `500` with a logged traceback on + every request. Only the name check maps to `400`: a corrupt history file + (malformed JSON or non-UTF-8) is still a `500`, and `POST .../rollback`, which + answered one with `400`, now returns `500` too. Deleting a skill with an + invalid name no longer logs a mutation-tail error. ([#7123]) + - **channels:** An IM message delivered twice no longer runs twice, and two Gateway instances no longer split a new conversation across two threads. Discord direct messages had no inbound dedupe key (no guild id), and a reply @@ -11902,3 +11910,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#7040]: https://github.com/bytedance/deer-flow/pull/7040 [#7099]: https://github.com/bytedance/deer-flow/pull/7099 [#7104]: https://github.com/bytedance/deer-flow/pull/7104 +[#7123]: https://github.com/bytedance/deer-flow/pull/7123 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 010a603447..0de21fd433 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1015,6 +1015,13 @@ ### 修复 +- **技能：** `GET /api/skills/custom/{skill_name}/history` 收到非法技能名（不是 + hyphen-case，或超过 64 个字符）时，现在与删除、回滚一样返回 `400`，不再返回 `500` + 并在每次请求时记录一次带堆栈的错误日志。只有名称校验映射为 `400`：损坏的历史文件 + （JSON 格式错误或非 UTF-8）仍返回 `500`，此前对其返回 `400` 的 + `POST .../rollback` 现在也返回 `500`。用非法名称删除技能时不再记录变更尾部 + （mutation tail）错误日志。 ([#7123]) + - **渠道：** 同一条 IM 消息被投递两次不再被执行两次，两个 Gateway 实例也不再把一个新 会话拆到两个线程上。Discord 私信此前没有入站去重键（没有 guild id），而 Discord 线程 内的回复在不跟踪该线程的实例上会得到另一个键；由于每个实例的会话都会收到每条消息， @@ -9455,3 +9462,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#7040]: https://github.com/bytedance/deer-flow/pull/7040 [#7099]: https://github.com/bytedance/deer-flow/pull/7099 [#7104]: https://github.com/bytedance/deer-flow/pull/7104 +[#7123]: https://github.com/bytedance/deer-flow/pull/7123 diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index a0cfaf1b55..3822f7036f 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -1,4 +1,5 @@ import asyncio +import json import logging import tempfile import threading @@ -817,6 +818,9 @@ async def delete_custom_skill(skill_name: str, request: Request, config: AppConf await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) try: skill_name = skill_name.replace("\r\n", "").replace("\n", "") + # Reject a malformed name (400 below) before the drained tail: failing + # there first takes the owner's mutation lock and logs a caller error. + SkillStorage.validate_skill_name(skill_name) # A cold user-scoped storage resolves the project root and builds # absolute paths on construction: blocking filesystem IO. storage = await asyncio.to_thread(_get_user_skill_storage, config) @@ -863,6 +867,15 @@ async def get_custom_skill_history(skill_name: str, request: Request, config: Ap await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) try: skill_name = skill_name.replace("\r\n", "").replace("\n", "") + # A malformed name is a client error (400, as delete and rollback + # answer). Validate it here rather than with a handler-wide + # ``except ValueError``: a corrupt history file raises + # ``json.JSONDecodeError`` or ``UnicodeDecodeError``, both ValueErrors, + # and must stay a 500. + try: + SkillStorage.validate_skill_name(skill_name) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) def _read_history() -> list[dict] | None: # Worker thread: storage construction, the existence probes, and the @@ -972,6 +985,12 @@ async def rollback_custom_skill(skill_name: str, body: SkillRollbackRequest, req raise HTTPException(status_code=400, detail="history_index is out of range") except FileNotFoundError as e: raise HTTPException(status_code=404, detail=str(e)) + except (json.JSONDecodeError, UnicodeDecodeError) as e: + # A corrupt (malformed JSON or non-UTF-8) history file is a server + # fault, not a bad request; both are ValueErrors, so they must be + # caught before the 400 mapping below. + logger.error("Failed to roll back custom skill %s: %s", skill_name, e, exc_info=True) + raise HTTPException(status_code=500, detail=f"Failed to roll back custom skill: {str(e)}") except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) except Exception as e: diff --git a/backend/docs/API.md b/backend/docs/API.md index 07b85de32f..5b832ccfe4 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -1189,6 +1189,9 @@ GET /api/skills/custom/{skill_name}/history } ``` +An invalid skill name returns `400`; a skill with neither a `SKILL.md` nor +recorded history returns `404`. + #### Rollback Custom Skill Restore a custom skill's `SKILL.md` from a history entry. The restored content is diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 4f3fca7032..631658fa01 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -4,7 +4,7 @@ - **Management YAML diagnostics**: `parse_skill_file(..., diagnostics=...)` optionally collects `SkillLoadDiagnostic` values separately from valid `Skill` entries. Only `UserScopedSkillStorage.load_custom_skill_diagnostics()` supplies that sink, after validating root ancestors and each resolved file against the caller's custom root. Exclude legacy/shared locations and external symlinks; do not weaken runtime discovery's separate compatibility policy. `GET /api/skills/diagnostics/custom` is admin-only and returns stable codes plus package-relative identifiers and 1-based file positions. Never return `_format_yaml_error`, exceptions or source excerpts. The management UI posts the existing admin-only reload before refetching diagnostics and normal skills; active Agent snapshots are unchanged. - **Diagnostic scope**: only `yaml.YAMLError` produces a management diagnostic. Missing frontmatter, non-mapping or invalid metadata, non-UTF-8 files and read errors (including permission denied) remain excluded; the UI and README state those limits. An empty diagnostic list is not a package-validity check. `tests/test_skill_load_diagnostics.py` pins these exclusions and permission-error redaction without changing runtime parsing. -- **History JSONL**: split records on LF, not Unicode text separators; preserve U+0085/U+2028/U+2029 in saved fields. Keep malformed JSON rejected. Tests: `tests/test_skill_history_jsonl.py`. +- **History JSONL**: split records on LF, not Unicode text separators; preserve U+0085/U+2028/U+2029 in saved fields. Keep malformed JSON rejected. Tests: `tests/test_skill_history_jsonl.py`. The history and rollback routes keep it a 500 and map only an invalid name to 400; `JSONDecodeError` and `UnicodeDecodeError` are `ValueError`s, so never let a handler-wide `except ValueError` catch them (`tests/test_skills_custom_router.py`). - **SkillScan usage counters**: the text assignment sweep recognizes unquoted nonnegative integers under the exact model-usage names `tokens`, `total_tokens`, `input_tokens`, `output_tokens`, `prompt_tokens`, `completion_tokens`, `cached_tokens`, and `reasoning_tokens`. Benchmark `tokens` also accepts a signed numeric delta or an object containing exactly four nonnegative finite numeric statistics (`mean`, `stddev`, `min`, `max`); JSON parsing is capped at 512 characters. Recover the complete key before applying the exception. Quoted unsigned counts, credential prefixes, arbitrary containers, and cloud-token detection remain active. Regressions live in `tests/test_skillscan_secret_assignment_formats.py`. diff --git a/backend/tests/test_skills_custom_router.py b/backend/tests/test_skills_custom_router.py index d98a56d1e2..764c244b90 100644 --- a/backend/tests/test_skills_custom_router.py +++ b/backend/tests/test_skills_custom_router.py @@ -778,6 +778,87 @@ def test_custom_skill_delete_preserves_history_and_allows_restore(monkeypatch, t assert refresh_calls == [("refresh", "default"), ("refresh", "default")] +def _make_demo_custom_skill_app(monkeypatch, tmp_path: Path) -> FastAPI: + from deerflow.config.paths import Paths + + custom_dir = _user_custom_dir(tmp_path, "default") / "demo-skill" + custom_dir.mkdir(parents=True, exist_ok=True) + (custom_dir / "SKILL.md").write_text(_skill_content("demo-skill"), encoding="utf-8") + + config = SimpleNamespace( + skills=SimpleNamespace(get_skills_path=lambda: tmp_path / "skills", container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None), + ) + monkeypatch.setattr("deerflow.config.get_app_config", lambda: config) + monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + monkeypatch.setattr("app.gateway.routers.skills.get_effective_user_id", lambda: "default") + return _make_test_app(config) + + +@pytest.mark.parametrize( + ("skill_name", "detail"), + [ + ("Bad_Name", "hyphen-case"), + ("a" * 65, "64 characters or fewer"), + ], +) +def test_custom_skill_history_rejects_invalid_name(monkeypatch, tmp_path, caplog, skill_name, detail): + """An invalid name is a 400 like delete and rollback, not a logged 500.""" + import logging + + app = _make_demo_custom_skill_app(monkeypatch, tmp_path) + + with TestClient(app) as client, caplog.at_level(logging.ERROR, logger=skills_router.__name__): + response = client.get(f"/api/skills/custom/{skill_name}/history") + + assert response.status_code == 400 + assert detail in response.json()["detail"] + assert "Failed to read history" not in caplog.text + + +def test_custom_skill_delete_rejects_invalid_name_before_mutation_tail(monkeypatch, tmp_path, caplog): + """A malformed name is a 400 the drained delete tail never sees, so nothing is logged.""" + import logging + + app = _make_demo_custom_skill_app(monkeypatch, tmp_path) + + with TestClient(app) as client, caplog.at_level(logging.ERROR, logger=skills_router.__name__): + response = client.delete("/api/skills/custom/Bad_Name") + + assert response.status_code == 400 + assert "hyphen-case" in response.json()["detail"] + assert caplog.records == [] + + +@pytest.mark.parametrize( + "history_bytes", + [ + pytest.param(b"{not json\n", id="malformed-json"), + pytest.param(b'{"action": "human_edit", "prev_content": "\xff"}\n', id="non-utf8"), + ], +) +@pytest.mark.parametrize( + ("method", "path", "body", "detail"), + [ + ("GET", "/api/skills/custom/demo-skill/history", None, "Failed to read history:"), + ("POST", "/api/skills/custom/demo-skill/rollback", {"history_index": -1}, "Failed to roll back custom skill:"), + ], +) +def test_custom_skill_corrupt_history_record_stays_server_error(monkeypatch, tmp_path, method, path, body, detail, history_bytes): + """Only the name maps to 400: a corrupt history file (JSONDecodeError and UnicodeDecodeError are ValueErrors too) is a server fault.""" + app = _make_demo_custom_skill_app(monkeypatch, tmp_path) + history_file = _user_custom_dir(tmp_path, "default") / ".history" / "demo-skill.jsonl" + history_file.parent.mkdir(parents=True, exist_ok=True) + history_file.write_bytes(history_bytes) + + with TestClient(app) as client: + response = client.request(method, path, json=body) + + assert response.status_code == 500 + assert response.json()["detail"].startswith(detail) + + def test_custom_skill_delete_continues_when_history_write_is_readonly(monkeypatch, tmp_path): skills_root = tmp_path / "skills" from deerflow.config.paths import Paths