github/deer-flow
1
0
Fork 0
mirror of https://github.com/bytedance/deer-flow.git synced 2026-10-26 21:26:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(skills): keep a required secret when an optional duplicate precedes it (#7569)

Browse Source 
* 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 <willem.jiang@gmail.com>
This commit is contained in:
Starchaser1024 2026-10-26 21:25:46 +08:00 committed by GitHub
parent b84b0667d7
commit 8d3af33f8e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 123 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
backend/packages/harness/deerflow/skills/export.py
View File

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

15
backend/packages/harness/deerflow/skills/parser.py
View File

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

33
backend/tests/test_skill_export.py
View File

@ -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

61
backend/tests/test_skill_request_scoped_secrets.py
View File

@ -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