diff --git a/dokimi/changelog.d/20261106_120000_napetrov_skill-script-checks.md b/dokimi/changelog.d/20261106_120000_napetrov_skill-script-checks.md new file mode 100644 index 000000000..318053961 --- /dev/null +++ b/dokimi/changelog.d/20261106_120000_napetrov_skill-script-checks.md @@ -0,0 +1,12 @@ + + +### Added + +- **`catalog check` checks the scripts a skill bundles** — each `.sh`, `.bash` and `.py` file, and each + file starting with `#!`, under the skill directory is read, never run or imported. A CRLF line ending + in a shell or shebang script is an `error` (`script-crlf`); a Python file that `ast.parse` rejects, or a + shell script that `bash -n` rejects, is an `error` (`script-syntax`). When `ruff` or `shellcheck` is on + PATH, one `note` (`script-lint`) gives the count and the top rules of each. Missing tools add nothing. diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index e06db52a0..a34bd2357 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -240,6 +240,21 @@ directory name and description but different `SKILL.md` text are a `warn` on bot (`mirror-drift`), naming the other copies. Skills with `disable-model-invocation: true` are exempt from the description checks. +The scan also reads the scripts a skill bundles: each `.sh`, `.bash` and `.py` file, and each file that +starts with `#!`, under the skill directory (a file over 1 MB, and a link out of the directory, is not read). +A file with a `#!` line and no `.sh`, `.bash` or `.py` suffix is read for CRLF only. +Nothing is run or imported. A CRLF line ending in a shell script or a `#!` file is an `error` (`script-crlf`): bash and a +shebang interpreter fail on the carriage return on Linux. A Python file that the running Python's +`ast.parse` rejects is an `error` (`script-syntax`). So is a shell script that `bash -n` rejects: the text +goes to bash on stdin with no environment, and only when bash is on PATH; a script whose shebang names +a shell other than bash, sh, dash or ksh is not checked that way, and a `.sh` with no shebang is taken as bash. The shebang's `env` options and `NAME=value` words are looked through, and `-O extglob` is given to `bash -n` when the shebang asks for it or the script names `extglob` (`bash -n` runs no `shopt -s extglob`); a shebang argument that changes parsing and is not one of these leaves the script unchecked. When `ruff` or `shellcheck` is on +PATH, the bundle is linted with `ruff check --select S,E9,F63,F7,F82` and `shellcheck -f json`, and one +`note` (`script-lint`) gives the count and the top three rules of each tool. Both run with their config +ignored (`--isolated`, `--norc`), so a skill's own config files do not change the result. A missing tool +adds nothing, and so does a tool that cannot start. The vendored and scaffold trees are left out +(`.git`, `node_modules`, `site-packages`, `.venv`, `templates` and the rest the loader skips). A skill +at the root of the checkout has no script bundle: its bundle would be the whole checkout, tests included. + Frontmatter that is not valid YAML (a bare colon in `description:`, say) stops the skill from loading. The scan reports that as a `frontmatter-parse` error carrying the YAML message, and it exits 1 with or without `--strict`, as a load diff --git a/dokimi/dokimi/external/script_checks.py b/dokimi/dokimi/external/script_checks.py new file mode 100644 index 000000000..356620306 --- /dev/null +++ b/dokimi/dokimi/external/script_checks.py @@ -0,0 +1,281 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Checks of the scripts a skill bundles. Nothing here runs or imports a script. + +A bundled ``.sh``/``.bash``/``.py`` file, or any file that starts with ``#!``, is read as bytes: +CRLF line endings break it (``script-crlf``), and it is parsed for syntax (``script-syntax``): +``ast.parse`` for Python, and ``bash -n`` for shell with the text on stdin, only when bash is on +PATH. ruff and shellcheck lint the bundle when they are on PATH; the counts go in one ``script-lint`` +note. Each file is read only inside the skill directory, and only up to :data:`MAX_BYTES`. +""" + +from __future__ import annotations + +import ast +import json +import os +import shutil +import warnings +from collections import Counter +from pathlib import Path +from typing import Callable, Optional + +from dokimi import process +from dokimi.skills.loader import TEMPLATE_DIR_NAMES, VENDORED_DIR_NAMES + +ScriptFinding = tuple[str, str, str] # (level, code, message), as ``validate.Finding`` takes them +Which = Callable[[str], Optional[str]] + +MAX_BYTES = 1_000_000 # a larger file is not read +SYNTAX_TIMEOUT_SEC = 10.0 +LINT_TIMEOUT_SEC = 60.0 +RUFF_SELECT = "S,E9,F63,F7,F82" +SHELL_SUFFIXES = (".sh", ".bash") +SCRIPT_SUFFIXES = (*SHELL_SUFFIXES, ".py") +BASH_FAMILY = frozenset({"bash", "sh", "dash", "ksh"}) # the shebang names bash -n can judge +ENV_OPERAND_OPTIONS = frozenset({"-u", "--unset", "-C", "--chdir"}) # env options with an operand +PARSE_SHOPTS = frozenset({"extglob"}) # bash -O options that change how a script parses; bash -n is given them +PARSE_NEUTRAL_FLAGS = frozenset("BEPTeuxh") # shebang flags, as letters, that change no parse +PARSE_NEUTRAL_OPTIONS = frozenset({"errexit", "nounset", "pipefail", "xtrace"}) # shebang -o names likewise +SYNTAX_CODES = frozenset({"E999", "invalid-syntax"}) # a syntax error is script-syntax's, not the lint's +SKIP_DIRS = VENDORED_DIR_NAMES | TEMPLATE_DIR_NAMES # the loader's vendored and scaffold trees +TOP_RULES = 3 + + +def script_findings( + skill_dir: Path, + *, + which: Optional[Which] = None, + runner: Optional[process.Runner] = None, + checkout: Optional[Path] = None, +) -> list[ScriptFinding]: + """The findings on the scripts under ``skill_dir``; ``which`` and ``runner`` default to PATH and :func:`process.run`. + + A skill at the root of ``checkout`` bundles the whole checkout, tests and fixtures included, so it has no script bundle. + """ + which = which or shutil.which + runner = runner or process.run + base = skill_dir.resolve() + if checkout is not None and base == checkout.resolve(): + return [] + bash = which("bash") + findings: list[ScriptFinding] = [] + python_files: list[Path] = [] + shell_files: list[Path] = [] + for path in _bundled_files(skill_dir, base): + data = _script_bytes(path) + if data is None: + continue + if path.suffix.lower() == ".py": + python_files.append(path) + elif path.suffix.lower() in SHELL_SUFFIXES: + shell_files.append(path) + findings += _file_findings(path, path.relative_to(skill_dir).as_posix(), data, bash, runner) + tools = _lint(python_files, shell_files, which, runner) + if any(tools.values()): + findings.append(("note", "script-lint", _lint_message(tools))) + return findings + + +def _file_findings(path: Path, rel: str, data: bytes, bash: Optional[str], runner: process.Runner) -> list[ScriptFinding]: + """The CRLF and syntax findings on one script: a .py is parsed, a shell script is judged by ``bash -n``.""" + suffix = path.suffix.lower() + crlf = b"\r\n" in data + found: list[ScriptFinding] = [] + if crlf and (suffix in SHELL_SUFFIXES or data.startswith(b"#!")): + found.append(("error", "script-crlf", f"{rel} has CRLF line endings: bash and a shebang " + "interpreter fail on the carriage return")) + if suffix == ".py": + problem = _python_problem(data, path) + if problem: + found.append(("error", "script-syntax", f"{rel} does not parse as Python: {problem}")) + elif suffix in SHELL_SUFFIXES and bash is not None and not crlf and _runs_under_bash(data): + # a CRLF script fails on every line, so its crlf finding is the one to fix; fish, zsh etc. are not bash's + options = _shell_options(data) # None: the shebang asks for an option bash -n cannot be given + problem = None if options is None else _shell_problem(bash, options, data, runner) + if problem: + found.append(("error", "script-syntax", f"{rel} does not parse as shell: {problem}")) + return found + + +def _bundled_files(skill_dir: Path, base: Path) -> list[Path]: + """Every regular file under ``skill_dir`` that resolves inside ``base``, in path order; no link is followed out.""" + found: list[Path] = [] + for root, dirnames, filenames in os.walk(skill_dir): + dirnames[:] = sorted(d for d in dirnames if d not in SKIP_DIRS) + for name in sorted(filenames): + path = Path(root) / name + try: + real = path.resolve() + if real.is_relative_to(base) and real.is_file(): + found.append(path) + except (OSError, RuntimeError, ValueError): + continue # a name the OS refuses, or a link loop: not a script we can read + return found + + +def _script_bytes(path: Path) -> Optional[bytes]: + """The bytes of ``path`` if it is a script (by suffix, or a ``#!`` first line) of at most MAX_BYTES, else None.""" + try: + if path.stat().st_size > MAX_BYTES: + return None + with path.open("rb") as handle: + head = handle.read(2) + if path.suffix.lower() not in SCRIPT_SUFFIXES and head != b"#!": + return None + handle.seek(0) + return handle.read() + except OSError: + return None + + +def _python_problem(data: bytes, path: Path) -> Optional[str]: + """The syntax error ``ast.parse`` gives for ``data``, or None. The file is parsed, never compiled or run.""" + try: + with warnings.catch_warnings(): # an invalid escape is a SyntaxWarning, not a reason to print + warnings.simplefilter("ignore") + ast.parse(data, filename=str(path)) + except SyntaxError as exc: + return f"{exc.msg} (line {exc.lineno})" if exc.lineno else str(exc.msg) + except (ValueError, RecursionError) as exc: # a NUL byte; nesting past the parser's limit + return " ".join(str(exc).split())[:200] + return None + + +def _runs_under_bash(data: bytes) -> bool: + """True when the script has no shebang, or one naming a POSIX-family shell (``env`` is looked through).""" + words = _shebang_words(data) + if words is None: + return True + return bool(words) and os.path.basename(words[0]) in BASH_FAMILY + + +def _shebang_words(data: bytes) -> Optional[list[str]]: + """The words of the ``#!`` line, from the interpreter on; None when there is no shebang.""" + if not data.startswith(b"#!"): + return None + words = data.split(b"\n", 1)[0][2:].decode("utf-8", "replace").split() + if words and os.path.basename(words[0]) == "env": + words = _env_command(words[1:]) + return words + + +def _env_command(args: list[str]) -> list[str]: + """What ``env`` runs: its options (``-u`` and ``-C`` take an operand; ``-S`` splits what follows) and the + ``NAME=value`` words before the command are skipped.""" + i = 0 + while i < len(args): + if args[i] in ENV_OPERAND_OPTIONS: + i += 2 + elif args[i].startswith("-") or "=" in args[i]: + i += 1 + else: + break + return args[i:] + + +def _spawn(runner: process.Runner, argv: list[str], **kwargs) -> Optional[process.Completed]: + """What ``runner`` gives for ``argv``, or None when the program cannot start (a missing interpreter, say).""" + try: + return runner(argv, **kwargs) + except OSError: + return None + + +def _shell_options(data: bytes) -> Optional[list[str]]: + """The bash options ``bash -n`` is given for the shebang's arguments: ``-O`` for a shopt that changes parsing + (``extglob``), and nothing for a flag or ``-o`` option that changes no parse. None when the shebang has an + argument this check cannot reproduce, so the script is not judged.""" + words = _shebang_words(data) + args = words[1:] if words else [] + options: list[str] = [] + i = 0 + while i < len(args): + word, operand = args[i], args[i + 1] if i + 1 < len(args) else "" + if word == "-O" and operand in PARSE_SHOPTS: + options += ["-O", operand] + i += 2 + elif word == "-o" and operand in PARSE_NEUTRAL_OPTIONS: + i += 2 + elif word.startswith("-") and len(word) > 1 and set(word[1:]) <= PARSE_NEUTRAL_FLAGS: + i += 1 + else: + return None + if b"extglob" in data and "extglob" not in options: + # A script that turns extglob on with ``shopt -s extglob`` has its patterns parsed only after that + # runs, and bash -n runs nothing: so the option is given when the text names it. + options += ["-O", "extglob"] + return options + + +def _shell_problem(bash: str, options: list[str], data: bytes, runner: process.Runner) -> Optional[str]: + """The first line ``bash -n`` reports for the script on stdin, or None. Nothing is run; no environment is passed.""" + done = _spawn(runner, [bash, *options, "-n"], timeout=SYNTAX_TIMEOUT_SEC, env=process.Env.only(), input=data) + if done is None or done.timed_out or done.returncode == 0: + return None + first = next((line.strip() for line in done.stderr.splitlines() if line.strip()), "") + return first[:200] or f"bash -n exited {done.returncode}" + + +def _lint(python_files: list[Path], shell_files: list[Path], which: Which, + runner: process.Runner) -> dict[str, Counter]: + """Rule counts per tool that is on PATH and ran to a verdict; a tool that is missing or fails is left out.""" + tools: dict[str, Counter] = {} + ruff = which("ruff") if python_files else None + if ruff is not None: + counts = _ruff_counts(ruff, python_files, runner) + if counts is not None: + tools["ruff"] = counts + shellcheck = which("shellcheck") if shell_files else None + if shellcheck is not None: + counts = _shellcheck_counts(shellcheck, shell_files, runner) + if counts is not None: + tools["shellcheck"] = counts + return tools + + +def _ruff_counts(ruff: str, files: list[Path], runner: process.Runner) -> Optional[Counter]: + # --isolated: no config from the skill's own directory; --no-cache: nothing written into it + done = _spawn(runner, [ruff, "check", "--select", RUFF_SELECT, "--output-format", "json", "--isolated", + "--no-cache", *map(str, files)], timeout=LINT_TIMEOUT_SEC, env=process.Env.only()) + items = _json_list(done) + if items is None: + return None + codes = [str(item.get("code") or "syntax") for item in items if isinstance(item, dict)] + return Counter(code for code in codes if code not in SYNTAX_CODES) + + +def _shellcheck_counts(shellcheck: str, files: list[Path], runner: process.Runner) -> Optional[Counter]: + done = _spawn(runner, [shellcheck, "-f", "json", "--norc", *map(str, files)], + timeout=LINT_TIMEOUT_SEC, env=process.Env.only()) + data = _json_list(done, wrapped_key="comments") + if data is None: + return None + return Counter(f"SC{item['code']}" for item in data + if isinstance(item, dict) and item.get("code") is not None) + + +def _json_list(done: Optional[process.Completed], *, wrapped_key: str = "") -> Optional[list]: + """The JSON list a linter printed (or the list under ``wrapped_key``); None if it did not run to a verdict. + + Both linters exit 1 when they found something and 0 when they did not; 2 or more is a failure. + """ + if done is None or done.timed_out or done.returncode not in (0, 1): + return None + try: + data = json.loads(done.stdout or "[]") + except json.JSONDecodeError: + return None + if wrapped_key and isinstance(data, dict): # a linter version that wraps its list in an object + data = data.get(wrapped_key) + return data if isinstance(data, list) else None + + +def _lint_message(tools: dict[str, Counter]) -> str: + parts = [] + for name, counts in tools.items(): + if counts: + top = ", ".join(f"{rule} x{n}" for rule, n in counts.most_common(TOP_RULES)) + parts.append(f"{name} {sum(counts.values())} ({top})") + return "bundled-script lint: " + "; ".join(parts) diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 3c1490f95..9e95a3c83 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -84,6 +84,7 @@ from .content_checks import content_findings from .skill_markdown import link_pieces, outside_fences from .listing import in_field +from .script_checks import script_findings NAME_PATTERN = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*") NAME_MAX = 64 @@ -673,6 +674,8 @@ def review_skills( Finding("warn", code, message) for code, message in content_findings(skill, at_root=at_root and not standalone) ] + review.findings += [Finding(*item) for item in script_findings( + skill.path.parent, checkout=None if standalone else entry.source_root)] if not hidden: review.findings += _description_findings(skill) review.content_sha256 = _skill_digest(skill) diff --git a/dokimi/tests/test_catalog_validate_scripts.py b/dokimi/tests/test_catalog_validate_scripts.py new file mode 100644 index 000000000..67d6807eb --- /dev/null +++ b/dokimi/tests/test_catalog_validate_scripts.py @@ -0,0 +1,384 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Script checks of ``catalog check``: CRLF, syntax and the optional linters on the scripts a skill bundles. + +The rules are in ``dokimi/external/script_checks.py``; these tests read them through ``review_skills`` +and, for the linter and ``bash -n`` calls, through a :class:`process.FakeRunner` (nothing is spawned). +""" + +from __future__ import annotations + +import json +import os +import shutil +from pathlib import Path + +import pytest + +from dokimi import process +from dokimi.external import review_skill_dir, script_checks +from dokimi.external.script_checks import script_findings +from tests.test_catalog_validate import _codes, _reviews_by_name, _skill + +LINTERS = ("ruff", "shellcheck") +BASH = shutil.which("bash") +needs_bash = pytest.mark.skipif(BASH is None, reason="bash is not on PATH") + + +@pytest.fixture +def no_linters(monkeypatch): + """ruff and shellcheck taken off PATH, so a box with them installed gives the same result as one without.""" + real = shutil.which + monkeypatch.setattr( + shutil, "which", lambda name, *a, **k: None if name in LINTERS else real(name, *a, **k) + ) + + +def _bundle(root: Path, files: dict[str, bytes]) -> Path: + """A skill ``bundled`` under ``root/skills`` with ``files`` written beside its SKILL.md.""" + _skill(root / "skills", "bundled", name="bundled", description="Use for bundled work.") + skill = root / "skills" / "bundled" + for rel, data in files.items(): + (skill / rel).parent.mkdir(parents=True, exist_ok=True) + (skill / rel).write_bytes(data) + return skill + + +def _tools(*names: str): + """A ``which`` that finds only ``names``, each at a fake absolute path.""" + return lambda name: f"/usr/bin/{name}" if name in names else None + + +def _linter_runner(ruff=None, shellcheck=None) -> process.FakeRunner: + """A runner that answers ruff and shellcheck with the given JSON (exit 1: they found something).""" + def handler(call): + if call.argv[0].endswith("ruff") and ruff is not None: + return process.reply(json.dumps(ruff), returncode=1) + if call.argv[0].endswith("shellcheck") and shellcheck is not None: + return process.reply(json.dumps(shellcheck), returncode=1) + return None # bash -n and anything else: exit 0, no output + return process.FakeRunner(handler) + + +def test_a_shell_script_with_crlf_line_endings_is_an_error(tmp_path, no_linters): + _bundle(tmp_path, {"run.sh": b"#!/bin/bash\r\necho hi\r\n"}) + + review = _reviews_by_name(tmp_path)["bundled"] + assert review.state == "error" + assert _codes(review) == ["script-crlf"] + + +def test_an_extensionless_shebang_script_with_crlf_is_an_error(tmp_path, no_linters): + _bundle(tmp_path, {"tool": b"#!/bin/sh\r\necho hi\r\n"}) + + assert _codes(_reviews_by_name(tmp_path)["bundled"]) == ["script-crlf"] + + +def test_a_python_file_that_does_not_parse_is_an_error_with_its_line(tmp_path, no_linters): + _bundle(tmp_path, {"tool.py": b"import os\n\ndef f(:\n pass\n"}) + + review = _reviews_by_name(tmp_path)["bundled"] + assert _codes(review) == ["script-syntax"] + assert review.findings[0].message.startswith("tool.py does not parse as Python: ") + assert review.findings[0].message.endswith("(line 3)") + assert review.findings[0].level == "error" + assert review.state == "error" + + +@needs_bash +def test_a_shell_script_bash_rejects_is_an_error(tmp_path, no_linters): + _bundle(tmp_path, {"run.sh": b"#!/bin/bash\nif true; then\n echo x\n"}) # no fi + + review = _reviews_by_name(tmp_path)["bundled"] + assert _codes(review) == ["script-syntax"] + assert review.findings[0].message.startswith("run.sh does not parse as shell: ") + + +def test_a_bash_suffix_file_with_crlf_line_endings_is_an_error(tmp_path, no_linters): + _bundle(tmp_path, {"lib.bash": b"echo hi\r\n"}) + + assert _codes(_reviews_by_name(tmp_path)["bundled"]) == ["script-crlf"] + + +@needs_bash +def test_a_bash_suffix_file_bash_rejects_is_an_error(tmp_path, no_linters): + _bundle(tmp_path, {"lib.bash": b"if true; then\n echo x\n"}) # no fi + + review = _reviews_by_name(tmp_path)["bundled"] + assert _codes(review) == ["script-syntax"] + assert review.findings[0].message.startswith("lib.bash does not parse as shell: ") + + +def test_bash_n_gets_the_script_on_stdin_with_no_environment(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"if true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: line 2: syntax error\n", returncode=2)) + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert findings == [("error", "script-syntax", + "run.sh does not parse as shell: bash: line 2: syntax error")] + [call] = runner.calls + assert call.argv == ("/usr/bin/bash", "-n") # the script is never a file argument + assert call.input == b"if true; then\n" + assert call.env == {} # no BASH_ENV, no PATH: nothing the script could source + + +def test_a_crlf_shell_script_is_reported_once_and_not_passed_to_bash(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"echo a\r\nif true; then\r\n"}) + runner = process.FakeRunner() + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert [code for _, code, _ in findings] == ["script-crlf"] + assert runner.calls == [] + + +def test_ruff_and_shellcheck_counts_go_in_one_script_lint_note(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"x = 1\n", "run.sh": b"#!/bin/sh\necho $x\n"}) + runner = _linter_runner( + ruff=[{"code": "S602"}, {"code": "S602"}, {"code": "S607"}], + shellcheck=[{"code": 2086}], + ) + + findings = script_findings(skill, which=_tools("ruff", "shellcheck", "bash"), runner=runner) + + assert findings == [("note", "script-lint", + "bundled-script lint: ruff 3 (S602 x2, S607 x1); shellcheck 1 (SC2086 x1)")] + ruff_call = next(c for c in runner.calls if c.argv[0].endswith("ruff")) + assert "--isolated" in ruff_call.argv and "--no-cache" in ruff_call.argv + assert ruff_call.argv[ruff_call.argv.index("--select") + 1] == "S,E9,F63,F7,F82" + assert all(c.env == {} for c in runner.calls if c.argv[0].endswith(("ruff", "shellcheck"))) + + +def test_shellcheck_json_wrapped_in_an_object_is_read_too(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/bin/sh\necho $x\n"}) + runner = process.FakeRunner(lambda call: process.reply(json.dumps({"comments": [{"code": 2086}]}), returncode=1)) + + findings = script_findings(skill, which=_tools("shellcheck"), runner=runner) + + assert findings == [("note", "script-lint", "bundled-script lint: shellcheck 1 (SC2086 x1)")] + + +def test_no_linter_on_path_adds_no_finding_and_spawns_nothing(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"x = 1\n", "run.sh": b"echo hi\n"}) + runner = process.FakeRunner() + + assert script_findings(skill, which=_tools(), runner=runner) == [] + assert runner.calls == [] + + +def test_a_linter_that_fails_to_run_adds_no_lint_note(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"x = 1\n"}) + runner = process.FakeRunner(lambda call: process.reply("", stderr="error: unknown flag", returncode=2)) + + assert script_findings(skill, which=_tools("ruff"), runner=runner) == [] + + +def test_a_linter_that_prints_no_json_adds_no_lint_note(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"x = 1\n"}) + runner = process.FakeRunner(lambda call: process.reply("not json", returncode=1)) + + assert script_findings(skill, which=_tools("ruff"), runner=runner) == [] + + +def test_python_with_crlf_and_no_shebang_is_fine(tmp_path, no_linters): + _bundle(tmp_path, {"tool.py": b"x = 1\r\n"}) + + assert _reviews_by_name(tmp_path)["bundled"].findings == [] + + +def test_an_invalid_escape_is_a_warning_not_a_syntax_error(tmp_path, no_linters): + _bundle(tmp_path, {"tool.py": b"import re\nre.compile('\\d+')\n"}) + + assert _reviews_by_name(tmp_path)["bundled"].findings == [] + + +def test_a_file_over_the_size_cap_is_not_read(tmp_path, monkeypatch): + monkeypatch.setattr(script_checks, "MAX_BYTES", 8) + skill = _bundle(tmp_path, {"big.py": b"def f(:\n pass\n"}) + + assert script_findings(skill, which=_tools(), runner=process.FakeRunner()) == [] + + +def test_a_script_reached_through_a_link_out_of_the_skill_is_not_read(tmp_path, no_linters): + outside = tmp_path / "outside.sh" + outside.write_bytes(b"echo a\r\n") + skill = _bundle(tmp_path, {}) + try: + os.symlink(outside, skill / "linked.sh") + except (OSError, NotImplementedError): + pytest.skip("this platform cannot make symlinks") + + assert script_findings(skill, which=_tools(), runner=process.FakeRunner()) == [] + + +@needs_bash +def test_a_clean_bundle_has_no_script_findings(tmp_path, no_linters): + _bundle(tmp_path, {"run.sh": b"#!/bin/sh\necho hi\n", "tool.py": b"print('hi')\n"}) + + review = _reviews_by_name(tmp_path)["bundled"] + assert review.state == "ok" + assert review.findings == [] + + +def test_a_script_with_another_shebang_is_not_checked_as_bash(tmp_path): + # a valid fish script: bash -n would reject it, so no bash is spawned for it + skill = _bundle(tmp_path, {"run.sh": b"#!/usr/bin/env fish\nif test -f x.txt\n echo y\nend\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: syntax error\n", returncode=2)) + + assert script_findings(skill, which=_tools("bash"), runner=runner) == [] + assert runner.calls == [] + + +def test_a_script_with_an_env_bash_shebang_is_checked_as_bash(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/usr/bin/env bash\nif true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: line 2: syntax error\n", returncode=2)) + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert [code for _, code, _ in findings] == ["script-syntax"] + + +def test_a_skill_at_the_checkout_root_has_no_script_bundle(tmp_path, no_linters): + _skill(tmp_path, ".", name="root", description="Use for root work.") + (tmp_path / "tests" / "fixtures").mkdir(parents=True) + (tmp_path / "tests" / "fixtures" / "broken.py").write_bytes(b"def f(:\n") + + review = _reviews_by_name(tmp_path)["root"] + assert [code for code in _codes(review) if code.startswith("script-")] == [] + + +def test_vendored_and_scaffold_trees_are_not_the_skills_scripts(tmp_path, no_linters): + _bundle(tmp_path, {"site-packages/bad.py": b"def f(:\n", "templates/bad.py": b"def f(:\n"}) + + assert _reviews_by_name(tmp_path)["bundled"].state == "ok" + + +def test_a_tool_that_cannot_start_adds_no_finding(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"x = 1\n", "run.sh": b"echo hi\n"}) + + def cannot_start(call): + raise FileNotFoundError(2, "No such file or directory", call.argv[0]) + + findings = script_findings(skill, which=_tools("bash", "ruff", "shellcheck"), + runner=process.FakeRunner(cannot_start)) + + assert findings == [] + + +def test_a_script_in_a_dot_directory_is_checked(tmp_path, no_linters): + _bundle(tmp_path, {".scripts/run.sh": b"#!/bin/sh\r\necho hi\r\n"}) + + assert _codes(_reviews_by_name(tmp_path)["bundled"]) == ["script-crlf"] + + +def test_a_python_syntax_error_is_not_counted_again_by_ruff(tmp_path): + skill = _bundle(tmp_path, {"tool.py": b"def f(:\n"}) + runner = _linter_runner(ruff=[{"code": "invalid-syntax"}]) + + findings = script_findings(skill, which=_tools("ruff"), runner=runner) + + assert [code for _, code, _ in findings] == ["script-syntax"] + + +def test_review_skill_dir_checks_the_scripts_of_a_skill_reviewed_alone(tmp_path, no_linters): + # the per-skill API takes the skill directory as a synthetic scan root, not as a checkout: its bundle is checked + _skill(tmp_path, "solo", name="solo", description="Use for solo work.") + (tmp_path / "solo" / "scripts").mkdir() + (tmp_path / "solo" / "scripts" / "bad.py").write_bytes(b"def f(:\n") + + review = review_skill_dir(tmp_path / "solo") + assert [f.code for f in review.findings if f.code.startswith("script-")] == ["script-syntax"] + + +@pytest.mark.parametrize("shebang", [ + b"#!/usr/bin/env -S -u BASH_ENV bash", + b"#!/usr/bin/env -S FOO=bar bash", + b"#!/usr/bin/env FOO=bar bash", +]) +def test_env_options_and_assignments_do_not_hide_the_shell(tmp_path, shebang): + skill = _bundle(tmp_path, {"run.sh": shebang + b"\nif true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: line 3: syntax error\n", returncode=2)) + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert [code for _, code, _ in findings] == ["script-syntax"] + assert len(runner.calls) == 1 + + +def test_a_shebang_shopt_that_changes_parsing_is_given_to_bash_n(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/usr/bin/env -S bash -O extglob\ncase x in @(x|y)) : ;; esac\n"}) + runner = process.FakeRunner() + + assert script_findings(skill, which=_tools("bash"), runner=runner) == [] + [call] = runner.calls + assert call.argv == ("/usr/bin/bash", "-O", "extglob", "-n") + + +@needs_bash +def test_extglob_from_the_shebang_is_valid_under_real_bash(tmp_path, no_linters): + _bundle(tmp_path, {"run.sh": b"#!/usr/bin/env -S bash -O extglob\ncase x in @(x|y)) : ;; esac\n"}) + + assert _reviews_by_name(tmp_path)["bundled"].findings == [] + + +def test_a_shopt_extglob_inside_the_script_is_given_to_bash_n(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/bin/bash\nshopt -s extglob\ncase x in +(a|b)) : ;; esac\n"}) + runner = process.FakeRunner() + + assert script_findings(skill, which=_tools("bash"), runner=runner) == [] + [call] = runner.calls + assert call.argv == ("/usr/bin/bash", "-O", "extglob", "-n") + + +@needs_bash +def test_shopt_extglob_inside_the_script_is_valid_under_real_bash(tmp_path, no_linters): + _bundle(tmp_path, {"run.sh": b"#!/bin/bash\nshopt -s extglob\ncase x in +(a|b)) : ;; esac\n"}) + + assert _reviews_by_name(tmp_path)["bundled"].findings == [] + + +@pytest.mark.parametrize("shebang", [b"#!/bin/bash -eu", b"#!/bin/bash -o pipefail"]) +def test_a_shebang_option_that_changes_no_parse_keeps_the_check(tmp_path, shebang): + skill = _bundle(tmp_path, {"run.sh": shebang + b"\nif true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: syntax error\n", returncode=2)) + + assert [code for _, code, _ in script_findings(skill, which=_tools("bash"), runner=runner)] == ["script-syntax"] + assert runner.calls[0].argv == ("/usr/bin/bash", "-n") + + +def test_a_shebang_option_bash_n_cannot_reproduce_leaves_the_script_unjudged(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/bin/bash -O globstar\nif true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: syntax error\n", returncode=2)) + + assert script_findings(skill, which=_tools("bash"), runner=runner) == [] + assert runner.calls == [] + + +@pytest.mark.parametrize("interpreter", ["sh", "dash", "ksh"]) +def test_a_sh_shebang_is_checked_by_bash_n(tmp_path, interpreter): + skill = _bundle(tmp_path, {"run.sh": f"#!/bin/{interpreter}\nif true; then\n".encode()}) + runner = process.FakeRunner(lambda call: process.reply(stderr=f"{interpreter}: line 2: syntax error\n", returncode=2)) + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert [code for _, code, _ in findings] == ["script-syntax"] + assert runner.calls[0].argv == ("/usr/bin/bash", "-n") + + +def test_a_bash_n_timeout_gives_no_finding(tmp_path): + skill = _bundle(tmp_path, {"run.sh": b"#!/bin/bash\nif true; then\n"}) + runner = process.FakeRunner(lambda call: process.reply(timed_out=True)) + + assert script_findings(skill, which=_tools("bash"), runner=runner) == [] + assert len(runner.calls) == 1 + + +def test_the_suffix_is_matched_in_any_case(tmp_path): + skill = _bundle(tmp_path, {"RUN.SH": b"if true; then\n", "Tool.PY": b"def f(:\n"}) + runner = process.FakeRunner(lambda call: process.reply(stderr="bash: syntax error\n", returncode=2)) + + findings = script_findings(skill, which=_tools("bash"), runner=runner) + + assert sorted(message.split(" does not")[0] for _, _, message in findings) == ["RUN.SH", "Tool.PY"]