feat(ci,cli): gate the SKILL.md description cap in CI and at pack time - #22
Conversation
Standard §5.6 requires the 1000-rendered-character description cap to be machine-enforced both in CI and in the command that produces the bundle. It was neither: the checker only ran from .githooks/pre-commit, which needs `git config core.hooksPath .githooks` per clone, and `construct skill ship` packed and pushed bundles without consulting it. An over-cap description therefore surfaced at install time, after the packing work was done. CI gains a step running .githooks/check-description-length.py over every SKILL.md found in the tree — `find` rather than a glob list, so grok-skills/ and android-skills/ are covered without a list to maintain. `construct skill ship` refuses to stage, commit, or push an over-cap skill (exit 5, CONFLICT, with an `oversized_skills` array naming each offender and its overage). The check runs right after the bundle-drift refusal, before anything is staged. skillmd::description_len measures what the loader sees and deliberately does not reuse frontmatter(), which trims for display. That exposed an off-by-one in split(): it excluded the newline before the closing fence, so a folded `description: >` that is the *last* frontmatter key lost its trailing newline and under-counted by one. split() now includes it, and unit tests pin that both block shapes count identically — otherwise the cap would depend on key order. Verified against a real 998-character description: the Python checker and the Rust gate agree exactly, and padding it to 1001 trips both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9106f5c3e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let oversized: Vec<(String, usize)> = shipped | ||
| .iter() | ||
| .filter_map(|skill| { | ||
| let len = skillmd::description_len(&repo.join(skill).join("SKILL.md"))?; | ||
| (len > DESCRIPTION_CAP).then(|| (skill.clone(), len)) |
There was a problem hiding this comment.
Validate descriptions in pending commits before pushing
When the worktree is clean but ahead > 0, shipped is empty because it is derived only from git status, so this loop validates nothing and the later pending-commit path pushes origin main. Thus a user who committed an oversized SKILL.md with the optional hook disabled can run construct skill ship and push it without the promised CONFLICT; inspect the skills changed in the unpushed commit range, or validate all catalogue skills before taking that push path.
AGENTS.md reference: AGENTS.md:L17-L21
Useful? React with 👍 / 👎.
| find . -name SKILL.md -not -path './.git/*' -print0 \ | ||
| | xargs -0 python3 .githooks/check-description-length.py |
There was a problem hiding this comment.
Parse rendered YAML in the authoritative CI gate
For folded descriptions containing trailing spaces, this new CI gate can pass an over-cap value because .githooks/check-description-length.py applies strip() to every block line (line 51), although YAML preserves those spaces. For example, 999 visible characters followed by 10 spaces renders to 1010 characters including the newline, while the invoked checker reports exactly 1000 and succeeds; use the already-installed YAML parser here/checker so every SKILL.md is measured from the decoded scalar.
AGENTS.md reference: AGENTS.md:L11-L18
Useful? React with 👍 / 👎.
…mption (#25) The two repos disagreed about workflow. /spacecraft-software/standard/CLAUDE.md states "Branch + PR, never push to main" for **both** the Standard and Construct repos, while Construct's own maintainer-local CLAUDE.md claimed this repo was "pre-authorised for auto-push on skill-directory changes" and CONTRIBUTING.md step 5 was a bare `git push … main`. Construct's actual recent history (#11, #21, #22) is squash-merged PRs, so the auto-push note was the outlier. Resolved in favour of branch + PR: - AGENTS.md gains a "Branch + PR — never push to main" hard rule (it had no push rule at all, only a pointer to CONTRIBUTING.md). - CONTRIBUTING.md step 5 replaced: feature branch → PR → squash-merge → delete branch, with the gh invocation. Noted as a two-repo rule binding human and assistant-driven changes alike. - Both state that an agent stops at opening the PR; merging is the maintainer's call and an agent never merges its own PR. - The maintainer-local CLAUDE.md (gitignored) is updated to match. Also flagged, not fixed: `construct skill ship` hard-codes `git push origin main` (construct-cli/src/commands/ship.rs:235), so the shipped tool can still bypass the rule. All three docs now say not to use it to publish until it is reworked — branch + `gh pr create`, or stop after the signed commit. That is a CLI behaviour change with schema, help-text, and test impact, so it is left as a separate decision. No skill directories touched, so no bundle rebuild. Description cap and reuse lint pass. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
Standard §5.6 requires the 1000-rendered-character
descriptioncap to be machine-enforced both in CI and in the command that produces the bundle. It was neither:.githooks/check-description-length.pyonly ran from the pre-commit hook, which needsgit config core.hooksPath .githooksper clone — opt-in, therefore skippable..github/workflows/ci.ymlnever invoked it.construct skill shipbuilt, committed, and pushed.zip/.skillbundles without consulting it at all.So an over-cap description surfaced at install time — after the packing work was done and the bundle was already public.
What
CI step. Runs the existing checker over every
SKILL.mdin the tree.findrather than a glob list, sogrok-skills/andandroid-skills/are covered with no list to maintain — same auto-discovery principle asvalidate-configs.py. No new dependency; the checker is stdlib-only.construct skill shippre-pack gate. Refuses to stage, commit, or push an over-cap skill (exit5,CONFLICT) with a structuredoversized_skillsarray naming each offender and its overage:{"error":{"code":"CONFLICT","exit_code":5, "message":"SKILL.md description exceeds the 1000-character cap: demo (1001 chars, 1 over)", "hint":"$EDITOR demo/SKILL.md # trim the `description` frontmatter field", "oversized_skills":[{"chars":1001,"over_by":1,"skill":"demo"}]}}The check sits directly after the bundle-drift refusal, before anything is staged. Exit code
5is reused rather than a new code invented —CONFLICTis canonically bound to5in the CLI Standard's exit map; the manifest's description for5now names both causes.An off-by-one this surfaced
skillmd::description_lenmeasures what the loader sees and deliberately does not reusefrontmatter(), which trims for display — a foldeddescription: >scalar carries a trailing newline the loader counts, so trimming under-reports by one and would let a description sitting exactly on the cap through.Measuring untrimmed exposed a latent bug in
split(): it excluded the newline before the closing---fence, so adescriptionthat is the last frontmatter key lost its trailing newline and under-counted by one, while the same description followed by another key counted correctly.split()now includes that newline, and unit tests pin both shapes to the same count — otherwise the cap would silently depend on frontmatter key order.Verification
cargo test→ all pass, including 5 newskillmdunit tests and 2 new ship tests (ship_refuses_oversized_description,ship_allows_description_exactly_at_cap— the cap is inclusive)cargo clippy --all-targets→ cleanspacecraft-accessibility-support's 998-char description into a fixture repo — Python checker says 998, ship plans normally; padded to 1001 — Python says 1001, ship exits 5 with1001 chars, 1 over. The two gates agree exactly at the boundary.reuse lint→ 606/606 ·validate-configs.py→ 302 files, 0 failed · description cap → clean across all 63 skillsDocs updated:
CONTRIBUTING.mdandAGENTS.mdnow describe all three enforcement points and rank them, and the by-hand command matches what CI runs.🤖 Generated with Claude Code