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
This commit is contained in:
Vincent Koc
2026-02-23 02:34:23 -05:00
committed by GitHub
parent 36400df086
commit c8a62e1cea
8 changed files with 137 additions and 11 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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