feat(catalog): check the scripts a skill bundles by napetrov · Pull Request #455 · intel/dokimi

napetrov · 2026-11-07T07:58:33Z

Refs #413.

Summary

catalog check now checks the scripts a skill bundles, without running or importing them:

script-crlf (error): CRLF line endings on a shell script or on a file with a #! line.
script-syntax (error): a .py that ast.parse rejects; a shell script that bash -n rejects (only when bash is on PATH, and the shebang is absent or names bash, sh, dash or ksh).
script-lint (note): ruff and shellcheck counts, only when those tools are on PATH; one note.

A skill at the checkout root has no script bundle. A missing tool, or one that cannot start, adds nothing.

Evidence

23 tests in tests/test_catalog_validate_scripts.py. The defect tests fail on the commits before their fixes (4 of 5 first-pass tests, 2 of 2 second-pass tests); the env-bash guard passes by design.
make ci-local on 12a042d: 16813 passed, 1 failed: test_process.py::test_sighup_kills_the_live_groups_too, a load flake that passes alone.
Self-review: 3 passes on the final commit, 0 blocking. Local marker written for 12a042d.
Not run: shellcheck (not installed on the box), so its path is covered only by a fake runner.

Merge Danger

b1, catalog eval fails on error codes. A skill with a bundled script error (CRLF, or a syntax error) now makes catalog eval report status fail and exit 1. Probe: a CRLF run.sh gives pass on base and fail on head. Routing also leaves such a skill out of the field as a spec error. This is intended for error codes, but it changes eval results for any catalog with a broken bundled script. Please check it against the eval gate before merge.
Declined gaps:
- an extensionless file with a #! Python or shell line is read for CRLF but not parsed;
- a no-shebang .sh is judged as bash, so a zsh-only script gets an error;
- bash -n is weaker for sh, dash and ksh shebangs;
- nested skill directories are scanned by both the outer and the inner skill;
- vendor/ is not in the loader's skip set, so it is still scanned;
- shellcheck-side notes are untested here.
Follow-up: tighten the module docstring on #! files.

🤖 Generated with Claude Code

catalog check reads the scripts a skill bundles, token-free and without running them: - script-crlf (error): CRLF line endings on a shell script or on a file with a #! line. - script-syntax (error): a .py that ast.parse rejects; a shell script that bash -n rejects, only when bash is on PATH and the shebang (if any) names bash, sh, dash or ksh. - script-lint (note): ruff and shellcheck counts, only when they are on PATH; one note. The loader's vendored and template trees are not scanned, a skill at the checkout root has no script bundle, and a tool that is missing or cannot start adds nothing. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

Copilot

🟡 Changes recommended

Bundled-script checks have unresolved coverage gaps, valid Bash shebangs can trigger false syntax errors, and null-code Ruff parse errors are double-counted as lint findings.

4 open issues

What changed in this PR

5 new findings: 4 must-fix and 1 lower-priority documentation correction.

This PR extends catalog validation to check scripts bundled with skills without executing or importing them, supporting broader corpus scanning.

Changes:

Adds CRLF and syntax checks, plus optional aggregated ruff and shellcheck results.
Integrates script findings into skill reviews, with checkout-root and excluded-directory handling.
Adds regression tests, user documentation, and a changelog entry.

File	Description
`dokimi/tests/test_catalog_validate_scripts.py`	Tests script checks, tool failures, and bundle boundaries.
`dokimi/dokimi/external/validate.py`	Includes script findings in skill reviews.
`dokimi/dokimi/external/script_checks.py`	Implements script discovery, syntax checks, and optional lint summaries.
`dokimi/docs/external-content.md`	Documents script checks and exclusions.
`dokimi/changelog.d/20261106_120000_napetrov_skill-script-checks.md`	Announces bundled-script validation.

_{Review effort: Balanced}

napetrov · 2026-11-07T22:51:26Z

+    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)]


Not reproduced: ruff 0.15.12 and 0.17.2 both report code invalid-syntax for def f(:, with no null code. The syntax filter already catches it. If you have a ruff version where the code is null, send it and I will add it.

codecov-commenter · 2026-11-07T08:12:50Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 95.59748% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.10%. Comparing base (e4e2f5f) to head (12a042d).

Files with missing lines	Patch %	Lines
dokimi/dokimi/external/script_checks.py	95.54%	6 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #455    +/-   ##
========================================
  Coverage   95.09%   95.10%            
========================================
  Files         372      373     +1     
  Lines       49245    49404   +159     
  Branches     8467     8496    +29     
========================================
+ Hits        46828    46984   +156     
- Misses       1532     1534     +2     
- Partials      885      886     +1

Files with missing lines	Coverage Δ
dokimi/dokimi/external/validate.py	`96.22% <100.00%> (+0.02%)`	⬆️
dokimi/dokimi/external/script_checks.py	`95.54% <95.54%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

antonwolfy

Review at 12a042d. Three bugs and one test gap. The CRLF and syntax logic, the containment of links, the loader skip set and the no-environment runs are sound, but the checkout-root guard, the shebang handling and the bash -n input each give a wrong verdict on a plain input.

What I ran, on a Linux review host at 12a042d:

make ci-local: 32 passed, 0 failed, 12 skipped (the skips need docker, a matrix or an org-level tool). 16,752 tests passed in the main run.
Reproductions for the code findings, with the LLM transport mocked.
51 hand-made mutants of script_checks.py against the new tests: 31 killed, 20 survived.

Findings not anchored inline:

The shebang mapping and the bash -n timeout are not pinned by any test. Mutation check: removing sh or dash from BASH_FAMILY, removing done.timed_out in _shell_problem, and dropping .lower() on the suffix (so RUN.SH and Tool.PY are skipped) all pass the full file of 23 tests. The Python script-syntax error level is not asserted either: downgrading it to a warning passes. These lines decide whether catalog check reports an error. Add one test per line: a #!/bin/sh script with an unterminated if, a bash reply with timed_out=True that gives no finding, an uppercase .SH and .PY, and review.state for the Python case.

Bot points I checked that no longer apply at this head (1)

Copilot's point on null-code ruff diagnostics does not apply: ruff 0.17.2 reports a parse error as "code": "invalid-syntax", which SYNTAX_CODES already filters, so def f(: adds no lint count.

review_skill_dir takes the skill directory as a synthetic scan root, so the checkout-root guard in script_findings skipped every script of a skill reviewed alone. review_skills takes root_is_checkout, which the per-skill entry point sets to False. Thread: PRRT_kwDOVBg7g860zx1a Thread: PRRT_kwDOVBg7g8601_r5 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

_runs_under_bash dropped only the words that start with "-", so the operand of -u or -C and a NAME=value word stayed in front of the interpreter, and "#!/usr/bin/env -S -u BASH_ENV bash" hid a bash script from bash -n. The command is now found the way env reads it. Thread: PRRT_kwDOVBg7g860zx1l Thread: PRRT_kwDOVBg7g8601_r7 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

bash -n ran with no options, so "#!/usr/bin/env -S bash -O extglob" with "case x in @(x|y)) ..." was reported as a syntax error. The shebang's -O extglob is now given to bash -n. A shebang argument that changes parsing and that this check cannot reproduce leaves the script unjudged, instead of judging it by the wrong rules. Thread: PRRT_kwDOVBg7g860zx1t Thread: PRRT_kwDOVBg7g8601_r9 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

…ase and the Python error level Four mutants of script_checks.py each passed the suite: dropping sh from the bash family, dropping the bash -n timeout, not lowering the suffix, and reporting a Python syntax error as a warning. Each now fails a test. Review: PRR_kwDOVBg7g88AAAABWm8I3A Comment: IC_kwDOVBg7g88AAAABj5wqqw Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

The docs now say which shebang forms are looked through (env options and NAME=value words), that -O extglob reaches bash -n, and that another parsing argument leaves the script unchecked. Review: PRR_kwDOVBg7g88AAAABWlvvuQ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

The shebang "-o pipefail" branch of _shell_options had no test. It is now pinned, next to the -eu flag case. Comment: IC_kwDOVBg7g88AAAABj5wqqw Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

napetrov · 2026-11-07T22:51:31Z

Three fixed (see threads); zx14 pushed back there. Docs in a5dd96d.

Fixed in 11ca09a: sh, timeout, suffix and Python level pinned by tests.

Fixed in 1dbe1fe: -o branch covered; 6 OS-fallback lines left untested.

napetrov requested a review from a team as a code owner November 7, 2026 07:58

Copilot AI balanced review requested due to automatic review settings November 7, 2026 07:58

Copilot started reviewing on behalf of napetrov November 7, 2026 07:58

View session

Copilot AI reviewed Nov 7, 2026

View reviewed changes

antonwolfy reviewed Nov 7, 2026

View reviewed changes

Comment thread dokimi/dokimi/external/script_checks.py Resolved

Comment thread dokimi/dokimi/external/script_checks.py Resolved

Comment thread dokimi/dokimi/external/script_checks.py Resolved

napetrov added 6 commits November 7, 2026 14:30

Address review: cover the -o option that changes no parse

1dbe1fe

The shebang "-o pipefail" branch of _shell_options had no test. It is now pinned, next to the -eu flag case. Comment: IC_kwDOVBg7g88AAAABj5wqqw Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

Navigation Menu

Repository navigation

feat(catalog): check the scripts a skill bundles - #455

feat(catalog): check the scripts a skill bundles#455
napetrov wants to merge 7 commits into
intel:mainfrom
napetrov:feat/skill-script-checks

napetrov commented Nov 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

napetrov Nov 7, 2026

Uh oh!

codecov-commenter commented Nov 7, 2026

Uh oh!

antonwolfy left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

napetrov commented Nov 7, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

napetrov commented Nov 7, 2026

Summary

Evidence

Merge Danger

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

napetrov Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 7, 2026

Codecov Report

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

napetrov commented Nov 7, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants