fix(skills): validate required secret optional flags (#5738)

* fix(skills): validate required secret optional flags

* fix(skills): align review with required secret validation

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

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(

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):

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

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")

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,