From 4abba38734409426236c28abdb8a3458669298b7 Mon Sep 17 00:00:00 2001 From: sxh Date: Sun, 4 Oct 2026 20:47:15 +0800 Subject: [PATCH] fix(skills): recognise eval fixture paths behind an earlier evals segment (#6271) is_eval_fixture_path returned on the first 'evals' segment, so a path whose 'evals/fixtures/' came after a plain 'evals' segment read as a non-fixture. Move the 'fixtures' test into the loop condition so every segment is checked. Co-authored-by: sxh313 --- .../harness/deerflow/skills/package_paths.py | 4 +- backend/tests/test_skill_package_paths.py | 93 +++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 backend/tests/test_skill_package_paths.py diff --git a/backend/packages/harness/deerflow/skills/package_paths.py b/backend/packages/harness/deerflow/skills/package_paths.py index 50c7508b7b..fb0069f42c 100644 --- a/backend/packages/harness/deerflow/skills/package_paths.py +++ b/backend/packages/harness/deerflow/skills/package_paths.py @@ -13,8 +13,8 @@ def is_eval_fixture_path(path: str | PurePosixPath) -> bool: """Return whether a path is under an eval fixture directory.""" parts = _parts(path) for index, part in enumerate(parts[:-1]): - if part == "evals" and len(parts) > index + 2: - return parts[index + 1] == "fixtures" + if part == "evals" and len(parts) > index + 2 and parts[index + 1] == "fixtures": + return True return False diff --git a/backend/tests/test_skill_package_paths.py b/backend/tests/test_skill_package_paths.py new file mode 100644 index 0000000000..c30acfee4f --- /dev/null +++ b/backend/tests/test_skill_package_paths.py @@ -0,0 +1,93 @@ +"""Tests for the shared skill-package path helpers. + +`is_eval_fixture_path` decides whether a package file is an eval fixture. It is +shared by the review resource graph and by SkillScan's nested-`SKILL.md` rule, so +a path that is under *any* `evals/fixtures/` segment must be recognised — not only +a path whose first `evals` segment is followed by `fixtures`. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from deerflow.skills.package_paths import is_eval_fixture_path, is_eval_fixture_skill_md +from deerflow.skills.skillscan.orchestrator import scan_skill_dir + +_MINIMAL_SKILL_MD = "---\nname: demo\ndescription: A demo skill\n---\n\nBody.\n" + + +@pytest.mark.parametrize( + ("path", "expected"), + [ + # A single `evals/fixtures/` segment: the behaviour that already worked. + ("evals/fixtures/blocked/SKILL.md", True), + ("evals/fixtures/blocked/scripts/run.py", True), + # The fixtures directory itself is not *under* it. + ("evals/fixtures", False), + # An earlier plain `evals` segment must not mask a later `evals/fixtures`. + ("evals/cases/inner/evals/fixtures/blocked/SKILL.md", True), + ("evals/cases/inner/evals/fixtures", False), + # Same depth as the row above, only the leading directory differs: this + # isolates the short circuit from path depth. + ("evals/fixtures/inner/evals/fixtures/blocked/SKILL.md", True), + # More than two `evals` segments. + ("evals/cases/a/evals/cases/b/evals/fixtures/blocked/SKILL.md", True), + # No `evals` segment, or no `fixtures` directly after one. + ("SKILL.md", False), + ("scripts/run.py", False), + ("evals/cases/inner/SKILL.md", False), + ("evals", False), + ("", False), + ], +) +def test_is_eval_fixture_path(path: str, expected: bool) -> None: + assert is_eval_fixture_path(path) is expected + + +@pytest.mark.parametrize( + ("path", "expected"), + [ + ("evals/fixtures/blocked/SKILL.md", True), + ("evals/cases/inner/evals/fixtures/blocked/SKILL.md", True), + ("evals/cases/inner/SKILL.md", False), + ("SKILL.md", False), + ("evals/fixtures/blocked/README.md", False), + ], +) +def test_is_eval_fixture_skill_md(path: str, expected: bool) -> None: + assert is_eval_fixture_skill_md(path) is expected + + +def _write_package(root: Path, nested: str) -> None: + root.mkdir(parents=True, exist_ok=True) + (root / "SKILL.md").write_text(_MINIMAL_SKILL_MD, encoding="utf-8") + nested_path = root / nested + nested_path.parent.mkdir(parents=True, exist_ok=True) + nested_path.write_text("---\nname: blocked\ndescription: A fixture sample\n---\n\nBody.\n", encoding="utf-8") + + +def test_nested_eval_fixture_skill_md_is_withheld_from_skillscan(tmp_path: Path) -> None: + """A fixture sample under a later `evals/fixtures/` is withheld, so the package is not blocked.""" + root = tmp_path / "pkg" + nested = "evals/cases/inner/evals/fixtures/blocked/SKILL.md" + _write_package(root, nested) + + assert is_eval_fixture_skill_md(nested) is True + + result = scan_skill_dir(root) + + assert [finding["rule_id"] for finding in result["findings"]] == [] + assert result["blocked"] is False + + +def test_skill_md_outside_eval_fixtures_is_still_reported(tmp_path: Path) -> None: + """Withholding stays scoped to fixtures: a nested non-fixture SKILL.md is still CRITICAL.""" + root = tmp_path / "pkg" + _write_package(root, "evals/cases/inner/SKILL.md") + + result = scan_skill_dir(root) + + assert [(finding["rule_id"], finding["severity"]) for finding in result["findings"]] == [("package-nested-skill-md", "CRITICAL")] + assert result["blocked"] is True