From c8a62e1cea7598e87f8adf88065b009df4982e3d Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 23 Feb 2026 02:34:23 -0500 Subject: [PATCH] Skills/Python: harden script edge cases and add regression tests (#24277) * Skill creator: skip self-including .skill output * Skill creator tests: cover output-dir-inside-skill case * Skill validator: parse frontmatter robustly across newlines * Skill validator tests: add CRLF and malformed frontmatter coverage * Model usage: require positive --days value * Model usage tests: cover --days validation and filtering * Nano banana: close input image handles after loading * Skill validator: keep type hints compatible with older python * Changelog: credit @vincentkoc for Python skills hardening --- CHANGELOG.md | 1 + skills/model-usage/scripts/model_usage.py | 12 ++++- .../model-usage/scripts/test_model_usage.py | 40 ++++++++++++++++ .../nano-banana-pro/scripts/generate_image.py | 7 +-- skills/skill-creator/scripts/package_skill.py | 4 ++ .../skill-creator/scripts/quick_validate.py | 24 +++++++--- .../scripts/test_package_skill.py | 14 ++++++ .../scripts/test_quick_validate.py | 46 +++++++++++++++++++ 8 files changed, 137 insertions(+), 11 deletions(-) create mode 100644 skills/model-usage/scripts/test_model_usage.py create mode 100644 skills/skill-creator/scripts/test_quick_validate.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 779d56805ea..6272e7e1235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Docs: https://docs.openclaw.ai - Skills/Python: add CI + pre-commit linting (`ruff`) and pytest discovery coverage for Python scripts/tests under `skills/`, including package test execution from repo root. Thanks @vincentkoc. - Sessions/Store: canonicalize inbound mixed-case session keys for metadata and route updates, and migrate legacy case-variant entries to a single lowercase key to prevent duplicate sessions and missing TUI/WebUI history. (#9561) Thanks @hillghost86. - Security/CI: add pre-commit security hook coverage for private-key detection and production dependency auditing, and enforce those checks in CI alongside baseline secret scanning. Thanks @vincentkoc. +- Skills/Python: harden skill script packaging and validation edge cases (self-including `.skill` outputs, CRLF frontmatter parsing, strict `--days` validation, and safer image file loading), with expanded Python regression coverage. Thanks @vincentkoc. ## 2026.2.23 diff --git a/skills/model-usage/scripts/model_usage.py b/skills/model-usage/scripts/model_usage.py index 0b71f96ea0f..ea28fa0d983 100644 --- a/skills/model-usage/scripts/model_usage.py +++ b/skills/model-usage/scripts/model_usage.py @@ -17,6 +17,16 @@ from datetime import date, datetime, timedelta from typing import Any, Dict, Iterable, List, Optional, Tuple +def positive_int(value: str) -> int: + try: + parsed = int(value) + except ValueError as exc: + raise argparse.ArgumentTypeError("must be an integer") from exc + if parsed < 1: + raise argparse.ArgumentTypeError("must be >= 1") + return parsed + + def eprint(msg: str) -> None: print(msg, file=sys.stderr) @@ -239,7 +249,7 @@ def main() -> int: parser.add_argument("--mode", choices=["current", "all"], default="current") parser.add_argument("--model", help="Explicit model name to report instead of auto-current.") parser.add_argument("--input", help="Path to codexbar cost JSON (or '-' for stdin).") - parser.add_argument("--days", type=int, help="Limit to last N days (based on daily rows).") + parser.add_argument("--days", type=positive_int, help="Limit to last N days (based on daily rows).") parser.add_argument("--format", choices=["text", "json"], default="text") parser.add_argument("--pretty", action="store_true", help="Pretty-print JSON output.") diff --git a/skills/model-usage/scripts/test_model_usage.py b/skills/model-usage/scripts/test_model_usage.py new file mode 100644 index 00000000000..4d5273401de --- /dev/null +++ b/skills/model-usage/scripts/test_model_usage.py @@ -0,0 +1,40 @@ +#!/usr/bin/env python3 +""" +Tests for model_usage helpers. +""" + +import argparse +from datetime import date, timedelta +from unittest import TestCase, main + +from model_usage import filter_by_days, positive_int + + +class TestModelUsage(TestCase): + def test_positive_int_accepts_valid_numbers(self): + self.assertEqual(positive_int("1"), 1) + self.assertEqual(positive_int("7"), 7) + + def test_positive_int_rejects_zero_and_negative(self): + with self.assertRaises(argparse.ArgumentTypeError): + positive_int("0") + with self.assertRaises(argparse.ArgumentTypeError): + positive_int("-3") + + def test_filter_by_days_keeps_recent_entries(self): + today = date.today() + entries = [ + {"date": (today - timedelta(days=5)).strftime("%Y-%m-%d"), "modelBreakdowns": []}, + {"date": (today - timedelta(days=1)).strftime("%Y-%m-%d"), "modelBreakdowns": []}, + {"date": today.strftime("%Y-%m-%d"), "modelBreakdowns": []}, + ] + + filtered = filter_by_days(entries, 2) + + self.assertEqual(len(filtered), 2) + self.assertEqual(filtered[0]["date"], (today - timedelta(days=1)).strftime("%Y-%m-%d")) + self.assertEqual(filtered[1]["date"], today.strftime("%Y-%m-%d")) + + +if __name__ == "__main__": + main() diff --git a/skills/nano-banana-pro/scripts/generate_image.py b/skills/nano-banana-pro/scripts/generate_image.py index 3365c20077f..8d60882c456 100755 --- a/skills/nano-banana-pro/scripts/generate_image.py +++ b/skills/nano-banana-pro/scripts/generate_image.py @@ -95,12 +95,13 @@ def main(): max_input_dim = 0 for img_path in args.input_images: try: - img = PILImage.open(img_path) - input_images.append(img) + with PILImage.open(img_path) as img: + copied = img.copy() + width, height = copied.size + input_images.append(copied) print(f"Loaded input image: {img_path}") # Track largest dimension for auto-resolution - width, height = img.size max_input_dim = max(max_input_dim, width, height) except Exception as e: print(f"Error loading input image '{img_path}': {e}", file=sys.stderr) diff --git a/skills/skill-creator/scripts/package_skill.py b/skills/skill-creator/scripts/package_skill.py index 926f4239ad2..aa4de89675a 100644 --- a/skills/skill-creator/scripts/package_skill.py +++ b/skills/skill-creator/scripts/package_skill.py @@ -93,6 +93,10 @@ def package_skill(skill_path, output_dir=None): if not _is_within(resolved_file, skill_path): print(f"[ERROR] File escapes skill root: {file_path}") return None + # If output lives under skill_path, avoid writing archive into itself. + if resolved_file == skill_filename.resolve(): + print(f"[WARN] Skipping output archive: {file_path}") + continue # Calculate the relative path within the zip. arcname = Path(skill_name) / file_path.relative_to(skill_path) diff --git a/skills/skill-creator/scripts/quick_validate.py b/skills/skill-creator/scripts/quick_validate.py index 0547b4041a5..dad42cc8362 100644 --- a/skills/skill-creator/scripts/quick_validate.py +++ b/skills/skill-creator/scripts/quick_validate.py @@ -6,12 +6,23 @@ Quick validation script for skills - minimal version import re import sys from pathlib import Path +from typing import Optional import yaml MAX_SKILL_NAME_LENGTH = 64 +def _extract_frontmatter(content: str) -> Optional[str]: + lines = content.splitlines() + if not lines or lines[0].strip() != "---": + return None + for i in range(1, len(lines)): + if lines[i].strip() == "---": + return "\n".join(lines[1:i]) + return None + + def validate_skill(skill_path): """Basic validation of a skill""" skill_path = Path(skill_path) @@ -20,16 +31,15 @@ def validate_skill(skill_path): if not skill_md.exists(): return False, "SKILL.md not found" - content = skill_md.read_text() - if not content.startswith("---"): - return False, "No YAML frontmatter found" + try: + content = skill_md.read_text(encoding="utf-8") + except OSError as e: + return False, f"Could not read SKILL.md: {e}" - match = re.match(r"^---\n(.*?)\n---", content, re.DOTALL) - if not match: + frontmatter_text = _extract_frontmatter(content) + if frontmatter_text is None: return False, "Invalid frontmatter format" - frontmatter_text = match.group(1) - try: frontmatter = yaml.safe_load(frontmatter_text) if not isinstance(frontmatter, dict): diff --git a/skills/skill-creator/scripts/test_package_skill.py b/skills/skill-creator/scripts/test_package_skill.py index 73a6d275b7c..fca51085622 100644 --- a/skills/skill-creator/scripts/test_package_skill.py +++ b/skills/skill-creator/scripts/test_package_skill.py @@ -135,6 +135,20 @@ class TestPackageSkillSecurity(TestCase): names = set(archive.namelist()) self.assertIn("nested-skill/lib/helpers/util.py", names) + def test_skips_output_archive_when_output_dir_is_skill_dir(self): + skill_dir = self.create_skill("self-output-skill") + + result = package_skill(str(skill_dir), str(skill_dir)) + + self.assertIsNotNone(result) + skill_file = skill_dir / "self-output-skill.skill" + self.assertTrue(skill_file.exists()) + with zipfile.ZipFile(skill_file, "r") as archive: + names = set(archive.namelist()) + self.assertIn("self-output-skill/SKILL.md", names) + self.assertIn("self-output-skill/script.py", names) + self.assertNotIn("self-output-skill/self-output-skill.skill", names) + if __name__ == "__main__": main() diff --git a/skills/skill-creator/scripts/test_quick_validate.py b/skills/skill-creator/scripts/test_quick_validate.py new file mode 100644 index 00000000000..717fbb8a8c2 --- /dev/null +++ b/skills/skill-creator/scripts/test_quick_validate.py @@ -0,0 +1,46 @@ +#!/usr/bin/env python3 +""" +Regression tests for quick skill validation. +""" + +import tempfile +from pathlib import Path +from unittest import TestCase, main + +from quick_validate import validate_skill + + +class TestQuickValidate(TestCase): + def setUp(self): + self.temp_dir = Path(tempfile.mkdtemp(prefix="test_quick_validate_")) + + def tearDown(self): + import shutil + + if self.temp_dir.exists(): + shutil.rmtree(self.temp_dir) + + def test_accepts_crlf_frontmatter(self): + skill_dir = self.temp_dir / "crlf-skill" + skill_dir.mkdir(parents=True, exist_ok=True) + content = "---\r\nname: crlf-skill\r\ndescription: ok\r\n---\r\n# Skill\r\n" + (skill_dir / "SKILL.md").write_text(content, encoding="utf-8") + + valid, message = validate_skill(skill_dir) + + self.assertTrue(valid, message) + + def test_rejects_missing_frontmatter_closing_fence(self): + skill_dir = self.temp_dir / "bad-skill" + skill_dir.mkdir(parents=True, exist_ok=True) + content = "---\nname: bad-skill\ndescription: missing end\n# no closing fence\n" + (skill_dir / "SKILL.md").write_text(content, encoding="utf-8") + + valid, message = validate_skill(skill_dir) + + self.assertFalse(valid) + self.assertEqual(message, "Invalid frontmatter format") + + +if __name__ == "__main__": + main()