diff --git a/backend/packages/harness/deerflow/skills/parser.py b/backend/packages/harness/deerflow/skills/parser.py index 5fa8b80e4..7c2aa5804 100644 --- a/backend/packages/harness/deerflow/skills/parser.py +++ b/backend/packages/harness/deerflow/skills/parser.py @@ -156,7 +156,17 @@ def parse_required_secrets(raw: object, skill_file: Path) -> tuple[SecretRequire name, optional = item.strip(), False elif isinstance(item, dict): name = str(item.get("name") or "").strip() - optional = bool(item.get("optional", False)) + raw_optional = item.get("optional", False) + if isinstance(raw_optional, bool): + optional = raw_optional + else: + logger.warning( + "Treating non-boolean optional value of type %s for required-secrets entry %r as required in %s", + type(raw_optional).__name__, + name, + skill_file, + ) + optional = False else: logger.warning("Ignoring malformed required-secrets entry in %s: %r", skill_file, item) continue diff --git a/backend/packages/harness/deerflow/skills/review/analyzer.py b/backend/packages/harness/deerflow/skills/review/analyzer.py index ae000816d..bcfe3e7dc 100644 --- a/backend/packages/harness/deerflow/skills/review/analyzer.py +++ b/backend/packages/harness/deerflow/skills/review/analyzer.py @@ -245,8 +245,9 @@ def _analyze_skill_md(content: str, *, profile: ProfileName, findings: list[dict ) ) + required_secrets = metadata.get("required-secrets") try: - parse_required_secrets(metadata.get("required-secrets"), Path("SKILL.md")) + parse_required_secrets(required_secrets, Path("SKILL.md")) except ValueError as exc: findings.append( make_finding( @@ -258,6 +259,17 @@ def _analyze_skill_md(content: str, *, profile: ProfileName, findings: list[dict ) ) + if isinstance(required_secrets, list) and any(isinstance(item, dict) and not isinstance(item.get("optional", False), bool) for item in required_secrets): + findings.append( + make_finding( + "structure.invalid-required-secrets-optional", + severity="error", + path="SKILL.md", + message="required-secrets[].optional must be a boolean.", + remediation="Use true or false for each required-secrets entry's optional field.", + ) + ) + if "secrets-autonomous" in metadata and not isinstance(metadata.get("secrets-autonomous"), bool): findings.append( make_finding( diff --git a/backend/packages/harness/deerflow/skills/validation.py b/backend/packages/harness/deerflow/skills/validation.py index 43361ff02..6ec241fbf 100644 --- a/backend/packages/harness/deerflow/skills/validation.py +++ b/backend/packages/harness/deerflow/skills/validation.py @@ -87,6 +87,12 @@ def validate_skill_frontmatter_text(content: str) -> tuple[bool, str, str | None required_secrets = frontmatter.get("required-secrets") if required_secrets is not None and not isinstance(required_secrets, list): return False, f"required-secrets in {SKILL_MD_FILE} must be a list", None + if required_secrets is not None: + for item in required_secrets: + if isinstance(item, dict) and not isinstance(item.get("optional", False), bool): + if item.get("name") is None: + return False, "required-secrets entry without a name has an optional field that must be a boolean", None + return False, f"required-secrets entry {item.get('name')!r} optional must be a boolean", None secrets_autonomous = frontmatter.get("secrets-autonomous") if secrets_autonomous is not None and not isinstance(secrets_autonomous, bool): diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py index 609266be6..d56e653c2 100644 --- a/backend/tests/test_skill_request_scoped_secrets.py +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -597,13 +597,26 @@ class TestRequiredSecretsParsing: skill_file = self._write_skill( tmp_path, - "name: erp-report\ndescription: d\nrequired-secrets:\n - name: ERP_TOKEN\n optional: true\n - name: REQUIRED_ONE", + "name: erp-report\ndescription: d\nrequired-secrets:\n - name: ERP_TOKEN\n optional: true\n - name: EXPLICIT_REQUIRED\n optional: false\n - name: REQUIRED_ONE", ) skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) by_name = {s.name: s for s in skill.required_secrets} assert by_name["ERP_TOKEN"].optional is True + assert by_name["EXPLICIT_REQUIRED"].optional is False assert by_name["REQUIRED_ONE"].optional is False + @pytest.mark.parametrize("value", ["false", "true", "no", 1, [], {}, None]) + def test_malformed_optional_fails_closed(self, value, caplog): + from deerflow.skills.parser import parse_required_secrets + + requirements = parse_required_secrets( + [{"name": "ERP_TOKEN", "optional": value}], + Path("SKILL.md"), + ) + assert requirements == (SecretRequirement(name="ERP_TOKEN", optional=False),) + assert f"non-boolean optional value of type {type(value).__name__}" in caplog.text + assert "required-secrets entry 'ERP_TOKEN' as required" in caplog.text + def test_invalid_env_name_entry_is_dropped(self, tmp_path): from deerflow.skills.parser import parse_skill_file from deerflow.skills.types import SkillCategory diff --git a/backend/tests/test_skill_review_core.py b/backend/tests/test_skill_review_core.py index 221b4305c..a9da84a19 100644 --- a/backend/tests/test_skill_review_core.py +++ b/backend/tests/test_skill_review_core.py @@ -81,6 +81,20 @@ def test_review_core_reports_non_string_frontmatter_key_as_unknown_field(tmp_pat assert "unexpected-field" in finding["message"] +def test_review_core_reports_non_boolean_required_secret_optional(tmp_path): + _write( + tmp_path / "SKILL.md", + '---\nname: demo-skill\ndescription: Demo skill. Invoke when testing review.\nrequired-secrets:\n - name: ERP_TOKEN\n optional: "true"\n---\n\n# Demo\n\nFollow the steps and stop.\n', + ) + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + finding = next(f for f in facts["findings"] if f["rule_id"] == "structure.invalid-required-secrets-optional") + assert finding["severity"] == "error" + assert finding["message"] == "required-secrets[].optional must be a boolean." + assert finding["remediation"] == "Use true or false for each required-secrets entry's optional field." + + def test_resource_graph_reports_unreferenced_resource(tmp_path): _write(tmp_path / "SKILL.md", _valid_skill()) _write(tmp_path / "references" / "unused.md", "# Unused\n") diff --git a/backend/tests/test_skills_validation.py b/backend/tests/test_skills_validation.py index de7f000c3..070adbcaf 100644 --- a/backend/tests/test_skills_validation.py +++ b/backend/tests/test_skills_validation.py @@ -6,6 +6,8 @@ test_skills_router.py and this module) into a single dedicated module. from pathlib import Path +import pytest + from deerflow.skills.validation import ALLOWED_FRONTMATTER_PROPERTIES, _validate_skill_frontmatter @@ -47,6 +49,38 @@ class TestValidateSkillFrontmatter: assert msg == "Skill is valid!" assert name == "my-skill" + @pytest.mark.parametrize("optional", ["", " optional: false\n", " optional: true\n"]) + def test_required_secrets_optional_boolean_values(self, tmp_path, optional): + skill_dir = _write_skill( + tmp_path, + f"---\nname: my-skill\ndescription: A skill\nrequired-secrets:\n - name: ERP_TOKEN\n{optional}---\n\nBody\n", + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is True + assert msg == "Skill is valid!" + assert name == "my-skill" + + @pytest.mark.parametrize("optional", ['"false"', '"true"', '"no"', "1", "[]", "{}", "null"]) + def test_required_secrets_optional_rejects_non_booleans(self, tmp_path, optional): + skill_dir = _write_skill( + tmp_path, + f"---\nname: my-skill\ndescription: A skill\nrequired-secrets:\n - name: ERP_TOKEN\n optional: {optional}\n---\n\nBody\n", + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is False + assert msg == "required-secrets entry 'ERP_TOKEN' optional must be a boolean" + assert name is None + + def test_required_secrets_optional_error_identifies_unnamed_entry(self, tmp_path): + skill_dir = _write_skill( + tmp_path, + '---\nname: my-skill\ndescription: A skill\nrequired-secrets:\n - optional: "true"\n---\n\nBody\n', + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is False + assert msg == "required-secrets entry without a name has an optional field that must be a boolean" + assert name is None + def test_allows_argument_hint(self, tmp_path): skill_dir = _write_skill( tmp_path,