From 8d3af33f8e57c23df1df523038a060f0bb4c23ea Mon Sep 17 00:00:00 2001 From: Starchaser1024 <73461444+Starchaser1024@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:25:46 +0800 Subject: [PATCH] fix(skills): keep a required secret when an optional duplicate precedes it (#7569) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): keep a required secret when an optional duplicate precedes it parse_required_secrets dropped every declaration of a name after the first, so an earlier 'optional: true' entry made a later required declaration unreachable and the skill ran without the secret. Duplicates now keep the first position but a required declaration upgrades the entry, and the export manifest applies the same rule. * test(skills): cover duplicate secret order and whitespace Port the three parameterized ordering and whitespace cases from PR #7570 while retaining #7569's full-YAML, optional-only, and export regressions. * refactor(skills): keep the export optional read fail-closed export.py read `optional` with `bool(secret.get("optional", False))`, so a truthy string would have become optional=True — the opposite of parse_required_secrets, which treats a non-boolean value as required (#5738). validate_skill_frontmatter_text already refuses such a package before the dedup loop (installation applies the same rule), so that coercion is unreachable today; read the flag as `is True` anyway so the two surfaces cannot drift if the gate ever moves. Pin the parity from both sides: the secrets suite asserts that the same document is rejected by validation and read as required by the parser, and the export suite asserts a manifest with skill_export_invalid_frontmatter and no requirements instead of a coerced optional=True. --------- Co-authored-by: Willem Jiang --- .../harness/deerflow/skills/export.py | 22 +++++-- .../harness/deerflow/skills/parser.py | 15 ++++- backend/tests/test_skill_export.py | 33 ++++++++++ .../test_skill_request_scoped_secrets.py | 61 +++++++++++++++++++ 4 files changed, 123 insertions(+), 8 deletions(-) diff --git a/backend/packages/harness/deerflow/skills/export.py b/backend/packages/harness/deerflow/skills/export.py index 6237f24bbd..07dc6e528b 100644 --- a/backend/packages/harness/deerflow/skills/export.py +++ b/backend/packages/harness/deerflow/skills/export.py @@ -306,17 +306,29 @@ def _manifest(name, entries, blockers, warnings, snapshot, budget): secrets = metadata.get("required-secrets") if secrets is not None: requirements["required_secrets"] = [] - seen = set() + positions = {} for secret in secrets: if isinstance(secret, str): secret = {"name": secret, "optional": False} if isinstance(secret, dict): raw_name = secret.get("name") secret_name = raw_name.strip() if isinstance(raw_name, str) else "" - if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", secret_name) and secret_name not in seen: - seen.add(secret_name) - requirements["required_secrets"].append({"name": secret_name, "optional": bool(secret.get("optional", False))}) - elif not secret_name or not re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", secret_name): + if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", secret_name): + # Only a literal boolean ``true`` is optional: the same + # fail-closed read as parse_required_secrets (#5738). + # validate_skill_frontmatter_text rejects the package + # above, so a malformed value never reaches this line. + optional = secret.get("optional") is True + position = positions.get(secret_name) + if position is not None: + # First occurrence keeps its slot; a required + # duplicate still wins, mirroring parse_required_secrets. + if not optional: + requirements["required_secrets"][position]["optional"] = False + continue + positions[secret_name] = len(requirements["required_secrets"]) + requirements["required_secrets"].append({"name": secret_name, "optional": optional}) + else: warnings.append(_issue("skill_export_invalid_declaration", "A malformed required-secrets declaration was omitted; inspect SKILL.md.", "SKILL.md")) else: warnings.append(_issue("skill_export_invalid_declaration", "A malformed required-secrets declaration was omitted; inspect SKILL.md.", "SKILL.md")) diff --git a/backend/packages/harness/deerflow/skills/parser.py b/backend/packages/harness/deerflow/skills/parser.py index e5ae43e56e..2205d39667 100644 --- a/backend/packages/harness/deerflow/skills/parser.py +++ b/backend/packages/harness/deerflow/skills/parser.py @@ -145,6 +145,12 @@ def parse_required_secrets(raw: object, skill_file: Path) -> tuple[SecretRequire environment-variable name are dropped with a warning, so one malformed declaration does not invalidate the whole skill. Raises ValueError only when the field is present but is not a list. + + Duplicate names collapse into the first entry of that name, but a name is + required when *any* declaration of it is required: without that merge an + earlier ``optional: true`` entry hides a later required one and the skill + silently runs without the secret. This is the same fail-closed direction as + the malformed-``optional`` handling below (issue #5738). """ if raw is None: return () @@ -152,7 +158,7 @@ def parse_required_secrets(raw: object, skill_file: Path) -> tuple[SecretRequire raise ValueError(f"required-secrets in {skill_file} must be a list") secrets: list[SecretRequirement] = [] - seen: set[str] = set() + positions: dict[str, int] = {} for item in raw: if isinstance(item, str): name, optional = item.strip(), False @@ -176,9 +182,12 @@ def parse_required_secrets(raw: object, skill_file: Path) -> tuple[SecretRequire if not _ENV_VAR_NAME_RE.match(name): logger.warning("Ignoring required-secrets entry with invalid env var name in %s: %r", skill_file, name) continue - if name in seen: + position = positions.get(name) + if position is not None: + if not optional and secrets[position].optional: + secrets[position] = SecretRequirement(name=name, optional=False) continue - seen.add(name) + positions[name] = len(secrets) secrets.append(SecretRequirement(name=name, optional=optional)) return tuple(secrets) diff --git a/backend/tests/test_skill_export.py b/backend/tests/test_skill_export.py index c3ae934ccf..b4475f7257 100644 --- a/backend/tests/test_skill_export.py +++ b/backend/tests/test_skill_export.py @@ -447,6 +447,39 @@ def test_required_secrets_normalization_and_invalid_warning(package): assert any(warning["code"] == "skill_export_invalid_declaration" for warning in manifest["warnings"]) +@requires_safe_capture +def test_duplicate_required_secrets_keep_first_position_and_required_wins(package): + storage, root = package + (root / "SKILL.md").write_text( + "---\nname: sample\ndescription: Example\nrequired-secrets:\n - name: ERP_TOKEN\n optional: true\n - ERP_TOKEN\n - name: OTHER_TOKEN\n optional: true\n---\n", + encoding="utf-8", + ) + manifest = export.export_manifest(storage, "sample") + # The manifest must agree with the runtime parser: an earlier optional + # declaration cannot hide a later required one (issue #7568). + assert manifest["requirements"]["required_secrets"] == [ + {"name": "ERP_TOKEN", "optional": False}, + {"name": "OTHER_TOKEN", "optional": True}, + ] + + +@requires_safe_capture +def test_non_boolean_optional_blocks_export_instead_of_coercing(package): + storage, root = package + (root / "SKILL.md").write_text( + '---\nname: sample\ndescription: Example\nrequired-secrets:\n - name: ERP_TOKEN\n optional: "false"\n---\n', + encoding="utf-8", + ) + manifest = export.export_manifest(storage, "sample") + # validate_skill_frontmatter_text gates the dedup loop, so the optional read + # never sees a string: the package is refused rather than reported with the + # truthy-string coercion (optional=True) that would disagree with + # parse_required_secrets' fail-closed read. + assert manifest["can_export"] is False + assert manifest["requirements"]["required_secrets"] is None + assert any(blocker["code"] == "skill_export_invalid_frontmatter" for blocker in manifest["blockers"]) + + @requires_safe_capture def test_mtime_does_not_change_revision(package): storage, root = package diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py index bed0488d67..6d676cc05b 100644 --- a/backend/tests/test_skill_request_scoped_secrets.py +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -768,6 +768,49 @@ class TestRequiredSecretsParsing: assert by_name["EXPLICIT_REQUIRED"].optional is False assert by_name["REQUIRED_ONE"].optional is False + def test_duplicate_declarations_keep_first_position_and_required_wins(self, tmp_path): + from deerflow.skills.parser import parse_skill_file + from deerflow.skills.types import SkillCategory + + # The required duplicate comes *after* an optional one: the secret must + # still be required, otherwise a caller that omits it never gets a + # missing-secret diagnostic (issue #7568). + skill_file = self._write_skill( + tmp_path, + "name: erp-report\ndescription: d\nrequired-secrets:\n - name: ERP_TOKEN\n optional: true\n - ERP_TOKEN\n - name: OTHER_TOKEN\n optional: true", + ) + skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) + assert skill.required_secrets == ( + SecretRequirement(name="ERP_TOKEN", optional=False), + SecretRequirement(name="OTHER_TOKEN", optional=True), + ) + + @pytest.mark.parametrize( + "raw", + [ + [{"name": "ERP_TOKEN", "optional": True}, "ERP_TOKEN"], + ["ERP_TOKEN", {"name": "ERP_TOKEN", "optional": True}], + [{"name": "ERP_TOKEN", "optional": True}, {"name": " ERP_TOKEN ", "optional": False}], + ], + ) + def test_duplicate_declaration_keeps_required(self, raw): + from deerflow.skills.parser import parse_required_secrets + + requirements = parse_required_secrets([*raw, "OTHER_TOKEN"], Path("SKILL.md")) + assert requirements == ( + SecretRequirement(name="ERP_TOKEN", optional=False), + SecretRequirement(name="OTHER_TOKEN", optional=False), + ) + + def test_optional_duplicates_stay_optional(self): + from deerflow.skills.parser import parse_required_secrets + + requirements = parse_required_secrets( + [{"name": "ERP_TOKEN", "optional": True}, {"name": "ERP_TOKEN", "optional": True}], + Path("SKILL.md"), + ) + assert requirements == (SecretRequirement(name="ERP_TOKEN", optional=True),) + @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 @@ -780,6 +823,24 @@ class TestRequiredSecretsParsing: 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 + @pytest.mark.parametrize("value", ['"false"', '"true"', "1", "null"]) + def test_malformed_optional_is_rejected_by_validation_too(self, value): + from deerflow.skills.frontmatter import split_skill_markdown + from deerflow.skills.parser import parse_required_secrets + from deerflow.skills.validation import validate_skill_frontmatter_text + + # Export gates on this validation rule before it reads `optional`, which + # is why a malformed declaration is refused there instead of being + # coerced into optional=True; installation applies the same rule. + content = f"---\nname: erp-report\ndescription: d\nrequired-secrets:\n - name: ERP_TOKEN\n optional: {value}\n---\n" + valid, message, _ = validate_skill_frontmatter_text(content) + assert valid is False + assert "optional must be a boolean" in message + + parts, error = split_skill_markdown(content) + assert error is None + assert parse_required_secrets(parts.metadata["required-secrets"], Path("SKILL.md")) == (SecretRequirement(name="ERP_TOKEN", optional=False),) + 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