fix(skillscan): read Python secret assignments from the AST (#5648)

* fix(skillscan): read Python secret assignments from the AST

The `secret-env-assignment` rule swept every text file with a
`name[:=]value` regex, which misreads Python syntax in two ways:

- `def __init__(self, token: Optional[str] = None):` — the captured
  "value" is a type annotation, not embedded secret material.
- `api_key = os.getenv("MINIMAX_API_KEY")` — reading a secret from the
  environment is this rule's own documented remediation, yet it was
  reported as a hardcoded credential.

Both are HIGH severity, so they map to a review `error` and fail the
Skill Review gate. Two bundled public skills therefore failed CI on an
unchanged checkout:

- skills/public/github-deep-research/scripts/github_api.py:56
- skills/public/music-generation/scripts/generate.py:27

Python sources now go through the AST instead of the line-oriented
sweep, keeping only real literal values. The text sweep is unchanged for
config, shell, YAML, and Markdown. Annotated assignments are still
reported, and now at the literal rather than at the annotation.

Tests: `secret-env-assignment` previously had no coverage anywhere in
backend/tests. Added six tests, including a regression test that scans
every bundled public skill script. Verified red on main and green here.

* fix(skillscan): keep text coverage for unparseable Python

Reviewer feedback on #5648: when `ast.parse` failed, the rule returned no
findings at all. One syntax error -- or a NUL byte, which `ast.parse` rejects
with the same exception -- therefore silenced the HIGH-severity
`secret-env-assignment` rule for the whole file, where `main` still swept the
raw text and reported it. For a review-gate rule that is a trivial evasion.

The line-oriented sweep moves into `_scan_secret_assignments_by_text`, which
the non-Python path now calls and which `_scan_python_secret_assignments` falls
back to when the file will not parse. Parseable files keep the precise AST
semantics this change introduces; unparseable ones keep main-level coverage
instead of losing the rule entirely.

Tests: both fallback paths added (syntax error, NUL byte); both fail before this
commit and pass after.

backend/packages/harness/deerflow/skills/skillscan/orchestrator.py

@@ -114,6 +114,10 @@ _HIDDEN_SENSITIVE_FILES = {
     "config",
 }
 _PLACEHOLDER_VALUES = {"", "x", "xx", "xxx", "xxxx", "changeme", "change-me", "example", "placeholder", "test", "dummy", "your-key", "<your-key>"}
+# `name[:=]value` sweep for line-oriented text (config, shell, YAML, Markdown). Python is
+# analyzed from its AST instead, because a regex cannot tell an annotation from a value.
+_SECRET_ASSIGNMENT_RE = re.compile(r"(?im)\b(token|password|passwd|api[_-]?key|secret|credential)s?\b\s*[:=]\s*[\"']?([^\"'\s#]+)")
+_SECRET_ASSIGNMENT_NAME_RE = re.compile(r"(?i)^(?:token|password|passwd|api[_-]?key|secret|credential)s?$")
 _SENSITIVE_PATH_RE = re.compile(r"(~/.ssh|/etc/passwd|/etc/shadow|/var/run/docker\.sock|docker\.sock|169\.254\.169\.254)")
 _EXTERNAL_HTTP_RE = re.compile(r"http://([A-Za-z0-9.-]+)(?::\d+)?(?:/|\b)")
 _URL_RE = re.compile(r"https?://[^\s)'\"<>]+")
@@ -323,13 +327,67 @@ def _scan_secrets(rel_path: str, text: str) -> list[SecurityFinding]:
             findings.append(_finding_from_match("secret-cloud-token", rel_path, text, match))
             break
 
-    assignment_re = re.compile(r"(?im)\b(token|password|passwd|api[_-]?key|secret|credential)s?\b\s*[:=]\s*[\"']?([^\"'\s#]+)")
-    for match in assignment_re.finditer(text):
+    if _is_python_path(rel_path, text):
+        findings.extend(_scan_python_secret_assignments(rel_path, text))
+        return findings
+
+    findings.extend(_scan_secret_assignments_by_text(rel_path, text))
+    return findings
+
+
+def _scan_secret_assignments_by_text(rel_path: str, text: str) -> list[SecurityFinding]:
+    """``name[:=]value`` sweep for line-oriented text, and for Python that will not parse."""
+    for match in _SECRET_ASSIGNMENT_RE.finditer(text):
         value = match.group(2).strip()
         if not _looks_like_placeholder(value):
-            findings.append(_finding_from_match("secret-env-assignment", rel_path, text, match))
-            break
-    return findings
+            return [_finding_from_match("secret-env-assignment", rel_path, text, match)]
+    return []
+
+
+def _python_secret_assignment_target(node: ast.expr) -> str | None:
+    """Final identifier bound by a simple assignment target, else None."""
+    if isinstance(node, ast.Name):
+        return node.id
+    if isinstance(node, ast.Attribute):
+        return node.attr
+    if isinstance(node, ast.Subscript) and isinstance(node.slice, ast.Constant) and isinstance(node.slice.value, str):
+        return node.slice.value
+    return None
+
+
+def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFinding]:
+    """Report embedded Python secrets from real literal assignments, not from raw text.
+
+    A line-oriented sweep cannot tell an annotation (``token: Optional[str]``), a
+    statement colon (``if not api_key:``), or this rule's own remediation
+    (``api_key = os.getenv("X")``) from a literal, and it points at an annotated
+    assignment's annotation rather than at its value.
+
+    A file Python cannot parse falls back to that sweep: the AST is only an
+    improvement, and returning nothing would let one syntax error (or a NUL byte)
+    silence a HIGH-severity rule for the whole file.
+    """
+    try:
+        tree = ast.parse(text)
+    except SyntaxError:
+        return _scan_secret_assignments_by_text(rel_path, text)
+
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Assign):
+            targets, value = list(node.targets), node.value
+        elif isinstance(node, ast.AnnAssign):
+            # A bare annotation binds no value at all, so `value` stays None.
+            targets, value = [node.target], node.value
+        else:
+            continue
+        if not isinstance(value, ast.Constant) or not isinstance(value.value, (str, bytes, int)):
+            continue
+        literal = value.value.decode("utf-8", "replace") if isinstance(value.value, bytes) else str(value.value)
+        if _looks_like_placeholder(literal):
+            continue
+        if any(_SECRET_ASSIGNMENT_NAME_RE.match(_python_secret_assignment_target(target) or "") for target in targets):
+            return [_finding_for_node("secret-env-assignment", rel_path, value, literal)]
+    return []
 
 
 def _scan_declaration(rel_path: str, text: str) -> list[SecurityFinding]:

backend/tests/test_skillscan_native.py

@@ -1427,3 +1427,104 @@ def test_python_reverse_shell_via_create_connection_blocks(tmp_path: Path) -> No
 
     assert _finding_by_rule(result["findings"], "python-reverse-shell")["severity"] == "CRITICAL"
     assert result["blocked"] is True
+
+
+def _scan_python_sample(tmp_path: Path, source: str) -> list[dict]:
+    """Scan a minimal skill package whose only code file is one Python script."""
+    skill_dir = tmp_path / "demo-skill"
+    _write_skill(skill_dir)
+    scripts_dir = skill_dir / "scripts"
+    scripts_dir.mkdir()
+    (scripts_dir / "sample.py").write_text(source, encoding="utf-8")
+    return scan_skill_dir(skill_dir)["findings"]
+
+
+def _secret_assignments(findings: list[dict]) -> list[dict]:
+    return [finding for finding in findings if finding["rule_id"] == "secret-env-assignment"]
+
+
+def test_secret_assignment_ignores_python_bare_annotation(tmp_path: Path) -> None:
+    """`token: Optional[str]` names a parameter's type; it embeds no secret value."""
+    source = "from typing import Optional\n\n\nclass Client:\n    def __init__(self, token: Optional[str] = None):\n        self.token = token\n"
+
+    assert _secret_assignments(_scan_python_sample(tmp_path, source)) == []
+
+
+def test_secret_assignment_ignores_python_environment_lookup(tmp_path: Path) -> None:
+    """Reading the secret from the environment is the documented remediation, not a finding."""
+    source = 'import os\n\n\ndef load():\n    api_key = os.getenv("MINIMAX_API_KEY")\n    return api_key\n'
+
+    assert _secret_assignments(_scan_python_sample(tmp_path, source)) == []
+
+
+def test_secret_assignment_still_flags_python_string_literal(tmp_path: Path) -> None:
+    """A hardcoded literal stays reported, and the value never reaches the finding."""
+    source = 'import os\n\n\ndef load():\n    api_key = "9f8e7d6c5b4a3210ff"\n    return api_key\n'
+
+    finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")
+
+    assert finding["line"] == 5
+    assert finding["evidence"] == "[redacted]"
+    assert "9f8e7d6c5b4a3210ff" not in repr(finding)
+
+
+def test_secret_assignment_still_flags_annotated_python_literal(tmp_path: Path) -> None:
+    """An annotated assignment still embeds its literal, so it must stay reported."""
+    source = 'import os\n\n\ndef load():\n    password: str = "hunter2-literal"\n    return password\n'
+
+    assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 5
+
+
+def test_secret_assignment_still_flags_non_python_text(tmp_path: Path) -> None:
+    """Non-Python text keeps the line-oriented sweep for config and shell files."""
+    skill_dir = tmp_path / "demo-skill"
+    _write_skill(skill_dir)
+    scripts_dir = skill_dir / "scripts"
+    scripts_dir.mkdir()
+    (scripts_dir / "deploy.sh").write_text("#!/bin/sh\nPASSWORD=hunter2-literal\n", encoding="utf-8")
+
+    finding = _finding_by_rule(scan_skill_dir(skill_dir)["findings"], "secret-env-assignment")
+
+    assert finding["file"] == "scripts/deploy.sh"
+    assert finding["line"] == 2
+
+
+def test_secret_assignment_survives_syntax_error_in_python(tmp_path: Path) -> None:
+    """A syntax error must not silence this rule for the whole file.
+
+    ``ast.parse`` rejects the file, so the rule has to fall back to the text sweep.
+    Otherwise appending one syntax error disables a HIGH-severity rule for an entire
+    file that ``main`` still scanned.
+    """
+    source = 'def broken(:\n    api_key = "9f8e7d6c5b4a3210ff"\n'
+
+    finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")
+
+    assert finding["line"] == 2
+    assert finding["evidence"] == "[redacted]"
+
+
+def test_secret_assignment_survives_nul_byte_in_python(tmp_path: Path) -> None:
+    """``ast.parse`` also rejects NUL bytes, so that path needs the same fallback."""
+    source = 'import os\napi_key = "9f8e7d6c5b4a3210ff"\x00\n'
+
+    finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")
+
+    assert finding["file"] == "scripts/sample.py"
+
+
+def test_bundled_public_skill_scripts_report_no_secret_assignment() -> None:
+    """Bundled skill scripts must not fail the review gate on an unchanged checkout (#4996).
+
+    Scoped to ``.py`` files on purpose: ``SKILL.md`` inside ``evals/fixtures`` is deliberately
+    hostile review material that the reviewer withholds from SkillScan, and declaration
+    scanning of real ``SKILL.md`` prose is governed by a separate rule.
+    """
+    skills_public_dir = Path(__file__).resolve().parents[2] / "skills" / "public"
+    offenders: dict[str, list[tuple[str | None, int | None]]] = {}
+    for skill_dir in sorted(path for path in skills_public_dir.iterdir() if path.is_dir()):
+        hits = [finding for finding in _secret_assignments(scan_skill_dir(skill_dir)["findings"]) if (finding["file"] or "").endswith(".py")]
+        if hits:
+            offenders[skill_dir.name] = [(finding["file"], finding["line"]) for finding in hits]
+
+    assert offenders == {}