diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 26e70c1..d987ca8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,7 +12,7 @@ permissions: jobs: lint: - name: REUSE + config validation + name: REUSE + config + skill-description validation runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -29,3 +29,13 @@ jobs: - name: Validate JSON / JSONC / TOML / YAML run: python3 .github/validate-configs.py + + # Standard §5.6: the description cap is enforced by CI, not only by the + # opt-in .githooks pre-commit hook (which needs `git config + # core.hooksPath .githooks` per clone and so cannot be relied on). + # `find` rather than a glob list, so grok-skills/, android-skills/, and + # any future nesting are covered without a list to maintain. + - name: SKILL.md description cap (<= 1000 rendered chars) + run: | + find . -name SKILL.md -not -path './.git/*' -print0 \ + | xargs -0 python3 .githooks/check-description-length.py diff --git a/AGENTS.md b/AGENTS.md index 61fd49b..780dd9d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,8 +14,12 @@ rules that bite. (A maintainer-local `CLAUDE.md` overlay adds host-specific note MUST NOT exceed 1000 chars. Folded `description: >` blocks render by joining lines with spaces (blank lines → newlines, plus a trailing newline); that rendered length is what counts, not the raw line count. Re-check after any edit. - The `.githooks/pre-commit` hook enforces this on staged skills — activate once - per clone with `git config core.hooksPath .githooks`. + Normative as Standard §5.6, and gated in CI (`SKILL.md description cap` step, + every `SKILL.md` in the tree) and by `construct skill ship`, which refuses to + stage or push an over-cap skill (exit 5, `CONFLICT`). The `.githooks/pre-commit` + hook is the fast local signal only — it is opt-in per clone + (`git config core.hooksPath .githooks`), so trim *before* packing rather than + relying on it. - **Rebuild BOTH bundles after any skill-dir edit**, in the same commit: `.zip` (`zip -qr`, keeps dir entries) and `.skill` (`zip -qrD`, drops them). A bundle that lags its `SKILL.md`/`references/` ships broken diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7046991..05998a9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -180,8 +180,16 @@ consolidated zip intentionally differs from any on-disk tree). print(len('\n'.join(out))+1) PY ``` - The [pre-commit hook](#pre-commit-hook) enforces this automatically on every - staged skill — no need to remember the snippet, but do activate the hook. + The cap is **normative** (Standard §5.6) and enforced in three places, so the + snippet above is only for a quick manual count: + - **CI** — the `SKILL.md description cap` step runs the checker over every + `SKILL.md` in the tree on each PR and push to `main`. This is the gate. + - **`construct skill ship`** — refuses to stage, commit, or push a skill whose + description is over the cap, before any bundle is shipped (exit 5, + `CONFLICT`, with an `oversized_skills` list naming each offender). + - **The [pre-commit hook](#pre-commit-hook)** — the fast local signal. It is + opt-in per clone, so it is explicitly *not* the gate; §5.6 requires the two + above precisely because a hook can be skipped. - **`microsoft-rust-guidelines` is intentionally `user-invocable: false`.** It is the mandatory auto-load Rust base — `spacecraft-standard-constitution` mandates loading it before any Rust and `spacecraft-rust-guidelines` defers to it as "load first," so @@ -206,9 +214,11 @@ consolidated zip intentionally differs from any on-disk tree). ## Pre-commit hook -A tracked hook enforces the description cap so a stale or over-long count can -never reach a commit. Git does **not** honour tracked hooks automatically, so -activate it **once per clone**: +A tracked hook catches an over-long description before it reaches a commit — the +fastest of the three enforcement points, though not the authoritative one (CI and +`construct skill ship` are; see the cap rule under [Editing rules](#editing-rules)). +Git does **not** honour tracked hooks automatically, so activate it **once per +clone**: ```sh git config core.hooksPath .githooks @@ -221,10 +231,12 @@ rendered description exceeds **1000** characters. It validates the *staged blob* not the working tree, so a fixup you forgot to re-stage is still caught. The only dependency is `python3`. -Run the same checker by hand over the whole catalogue any time: +Run the same checker by hand over the whole catalogue any time — this is the +command CI runs, so it covers `android-skills/` and any future nesting too: ```sh -python3 .githooks/check-description-length.py */SKILL.md grok-skills/*/SKILL.md +find . -name SKILL.md -not -path './.git/*' -print0 \ + | xargs -0 python3 .githooks/check-description-length.py ``` It exits non-zero and lists each offender (with how many chars over) when any diff --git a/construct-cli/src/commands/ship.rs b/construct-cli/src/commands/ship.rs index 9480be1..2b61f41 100644 --- a/construct-cli/src/commands/ship.rs +++ b/construct-cli/src/commands/ship.rs @@ -22,12 +22,25 @@ use crate::commands::sync; use crate::context::Context; use crate::install::plan::NON_SKILL_DIRS; use crate::output::error::{AppError, ErrorCode}; +use crate::sources::skillmd; use crate::output::{CommandOutput, HumanRender}; /// Default catalogue clone to ship from. const DEFAULT_REPO: &str = "/spacecraft-software/construct"; -/// The remote a ship is allowed to push to (substring check). +/// The remote a ship is allowed to push to (substring check). Standard §6.4: +/// publication targets are limited to namespaces Spacecraft Software controls. const EXPECTED_REMOTE: &str = "Spacecraft-Software/Construct"; +/// Maximum rendered length of a skill's frontmatter `description` (Standard +/// §5.6). +/// +/// The consuming skill loader rejects anything over **1024** characters at +/// install time — after the bundles are built and pushed — so the cap sits at +/// 1000 for a 24-character margin covering encoding and trailing-newline edge +/// cases. Raising it past the loader's limit would ship bundles that cannot be +/// installed. `.githooks/check-description-length.py` enforces the same number +/// in CI and in the pre-commit hook; changing one without the other lets a +/// bundle pass one gate and fail the next. +const DESCRIPTION_CAP: usize = 1000; /// Assistant co-authorship trailer (CONTRIBUTING §4). const COAUTHOR: &str = "Co-Authored-By: Claude Opus 4.8 (1M context) "; @@ -94,6 +107,42 @@ pub(crate) fn run(ctx: &Context, args: &ShipArgs) -> Result = shipped + .iter() + .filter_map(|skill| { + let len = skillmd::description_len(&repo.join(skill).join("SKILL.md"))?; + (len > DESCRIPTION_CAP).then(|| (skill.clone(), len)) + }) + .collect(); + if let Some((first, _)) = oversized.first() { + let detail = oversized + .iter() + .map(|(skill, len)| format!("{skill} ({len} chars, {} over)", len - DESCRIPTION_CAP)) + .collect::>() + .join(", "); + return Err(AppError::new( + ctx, + ErrorCode::Conflict, + 5, + format!("SKILL.md description exceeds the {DESCRIPTION_CAP}-character cap: {detail}"), + format!("$EDITOR {first}/SKILL.md # trim the `description` frontmatter field"), + ) + .with_extension( + "oversized_skills", + json!(oversized + .iter() + .map(|(skill, len)| json!({ + "skill": skill, + "chars": len, + "over_by": len - DESCRIPTION_CAP, + })) + .collect::>()), + )); + } + // Build the explicit stage list: shipped skills' files + their bundles + // catalogue-level root files (README.md, flake.lock). Never `git add -A`. let mut stage: Vec = Vec::new(); diff --git a/construct-cli/src/manifest.rs b/construct-cli/src/manifest.rs index 8240767..4b40167 100644 --- a/construct-cli/src/manifest.rs +++ b/construct-cli/src/manifest.rs @@ -223,7 +223,7 @@ pub(crate) fn commands() -> Vec { ("3", "NOT_FOUND — repo path does not exist"), ( "5", - "CONFLICT — skill source changed without rebuilt .zip/.skill bundles", + "CONFLICT — skill source changed without rebuilt .zip/.skill bundles, or a SKILL.md description exceeds the 1000-character cap (Standard §5.6)", ), ("127", "DEPENDENCY_MISSING — git or nix not on PATH"), ]), diff --git a/construct-cli/src/sources/skillmd.rs b/construct-cli/src/sources/skillmd.rs index d6dff47..dc22cb2 100644 --- a/construct-cli/src/sources/skillmd.rs +++ b/construct-cli/src/sources/skillmd.rs @@ -29,6 +29,24 @@ pub(crate) fn frontmatter(skill_md: &Path) -> (Option, Option) { } } +/// The rendered length, in characters, of a `SKILL.md` frontmatter +/// `description` — the exact string the skill loader measures (Standard §5.6). +/// +/// Deliberately does not reuse [`frontmatter`], which trims for display: a +/// folded `description: >` scalar carries the trailing newline the loader +/// counts, so trimming under-reports by one character and would let a +/// description sitting exactly on the cap slip through. `.githooks/ +/// check-description-length.py` counts the same way, and the two must agree. +/// +/// Returns `None` when the file is unreadable, has no frontmatter, has no +/// `description`, or does not parse — none of which this cap can adjudicate. +pub(crate) fn description_len(skill_md: &Path) -> Option { + let content = std::fs::read_to_string(skill_md).ok()?; + let (fm, _) = split(&content)?; + let front = serde_yaml::from_str::(fm).ok()?; + front.description.map(|d| d.chars().count()) +} + /// The markdown body of a `SKILL.md` (everything after the frontmatter), or the /// whole file when there is no frontmatter. pub(crate) fn body(skill_md: &Path) -> String { @@ -39,13 +57,67 @@ pub(crate) fn body(skill_md: &Path) -> String { } } +#[cfg(test)] +mod tests { + use std::io::Write as _; + + use super::description_len; + + /// `description_len` on a `SKILL.md` written to a temp file. + fn len_of(content: &str) -> Option { + let mut f = tempfile::NamedTempFile::new().expect("temp file"); + f.write_all(content.as_bytes()).expect("write"); + description_len(f.path()) + } + + #[test] + fn folded_scalar_keeps_its_trailing_newline_as_the_last_key() { + // The closing `---` fence ends the block. The loader still counts the + // newline that terminates the folded content, so this is 4 chars. + assert_eq!(len_of("---\nname: d\ndescription: >\n abc\n---\nb\n"), Some(4)); + } + + #[test] + fn folded_scalar_keeps_its_trailing_newline_before_another_key() { + // A dedented key ends the block instead. Same count either way — the + // two shapes must not disagree, or the cap would depend on key order. + assert_eq!( + len_of("---\ndescription: >\n abc\nname: d\n---\nb\n"), + Some(4) + ); + } + + #[test] + fn folded_scalar_joins_wrapped_lines_with_single_spaces() { + // "a b c\n" — raw line lengths are not the measurement. + assert_eq!( + len_of("---\nname: d\ndescription: >\n a\n b\n c\n---\nb\n"), + Some(6) + ); + } + + #[test] + fn plain_single_line_scalar_has_no_trailing_newline() { + assert_eq!(len_of("---\nname: d\ndescription: abc\n---\nb\n"), Some(3)); + } + + #[test] + fn absent_description_is_not_measurable() { + assert_eq!(len_of("---\nname: d\n---\nb\n"), None); + assert_eq!(len_of("no frontmatter here\n"), None); + } +} + /// Split `---\n\n---\n` into `(frontmatter, body)`. fn split(content: &str) -> Option<(&str, &str)> { let rest = content .strip_prefix("---\n") .or_else(|| content.strip_prefix("---\r\n"))?; let idx = rest.find("\n---")?; - let fm = &rest[..idx]; + // Inclusive of the newline before the closing fence: without it a block + // scalar that is the *last* frontmatter key loses its trailing newline, and + // `description_len` would under-count by one against the loader. + let fm = &rest[..=idx]; // Body begins after the closing fence line. let after = &rest[idx + 1..]; // at the closing "---" let body = after.split_once('\n').map_or("", |(_, b)| b); diff --git a/construct-cli/tests/ship.rs b/construct-cli/tests/ship.rs index 36caebd..8b4b936 100644 --- a/construct-cli/tests/ship.rs +++ b/construct-cli/tests/ship.rs @@ -119,6 +119,84 @@ fn ship_refuses_bundle_drift() { assert!(err["error"]["hint"].as_str().unwrap().contains("zip")); } +/// A `SKILL.md` whose folded `description` renders to `len` characters. +/// +/// The folded scalar joins its wrapped lines with single spaces and keeps one +/// trailing newline, which the loader counts — so the body is `len - 1` filler +/// characters on a single indented line. +fn skill_md_with_description(len: usize) -> String { + let filler = "d".repeat(len - 1); + format!("---\nname: demo\ndescription: >\n {filler}\n---\nbody\n") +} + +#[test] +fn ship_refuses_oversized_description() { + let repo = fixture(REMOTE); + let p = repo.path(); + write(p, "demo/SKILL.md", &skill_md_with_description(900)); + write(p, "demo.zip", "z1"); + write(p, "demo.skill", "s1"); + run_git(p, &["add", "demo/SKILL.md", "demo.zip", "demo.skill"]); + run_git(p, &["commit", "-qm", "init"]); + // Push the description past the 1000-char cap; bundles rebuilt, so the only + // thing standing between this and a push is the §5.6 gate. + write(p, "demo/SKILL.md", &skill_md_with_description(1001)); + write(p, "demo.zip", "z2"); + write(p, "demo.skill", "s2"); + + let assertion = bin() + .args([ + "skill", + "ship", + "--repo", + p.to_str().unwrap(), + "--no-sync", + "--dry-run", + "--json", + ]) + .assert() + .code(5); + let err: Value = + serde_json::from_slice(&assertion.get_output().stderr).expect("structured error"); + assert_eq!(err["error"]["code"], "CONFLICT"); + assert_eq!(err["error"]["oversized_skills"][0]["skill"], "demo"); + assert_eq!(err["error"]["oversized_skills"][0]["chars"], 1001); + assert_eq!(err["error"]["oversized_skills"][0]["over_by"], 1); +} + +#[test] +fn ship_allows_description_exactly_at_cap() { + let repo = fixture(REMOTE); + let p = repo.path(); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv1\n"); + write(p, "demo.zip", "z1"); + write(p, "demo.skill", "s1"); + run_git(p, &["add", "demo/SKILL.md", "demo.zip", "demo.skill"]); + run_git(p, &["commit", "-qm", "init"]); + // Exactly 1000 rendered characters is compliant — the cap is inclusive. + write(p, "demo/SKILL.md", &skill_md_with_description(1000)); + write(p, "demo.zip", "z2"); + write(p, "demo.skill", "s2"); + + let out = bin() + .args([ + "skill", + "ship", + "--repo", + p.to_str().unwrap(), + "--no-sync", + "--dry-run", + "--json", + ]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let v: Value = serde_json::from_slice(&out).expect("valid JSON"); + assert_eq!(v["data"]["status"], "planned"); +} + #[test] fn ship_rejects_wrong_remote() { let repo = fixture("https://example.com/foo/bar.git");