From 4dc25dc2ec03749d603a86b65dc07cbe4fc753d1 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Thu, 3 Sep 2026 17:36:46 -0400 Subject: [PATCH 01/10] Enforce Agent Skills name validation on every sync path, not only --validate --- .github/workflows/sync-check.yml | 3 ++ docs/adapters.md | 2 + sync.py | 24 ++++++----- tests/test_sync.py | 71 ++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/.github/workflows/sync-check.yml b/.github/workflows/sync-check.yml index cdfe9a4..fc67c61 100644 --- a/.github/workflows/sync-check.yml +++ b/.github/workflows/sync-check.yml @@ -9,5 +9,8 @@ jobs: - uses: actions/checkout@v5 - name: Check for sync drift run: | + ./sync.py --validate + # placeholder, permission, and command-shape checks; skill names are + # gated on every path, including the --dry-run below ./sync.py --dry-run # exits non-zero if any files would be written diff --git a/docs/adapters.md b/docs/adapters.md index 698a1cb..b939b1b 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -2,6 +2,8 @@ Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `/SKILL.md` folder with YAML frontmatter (`name`, `description`). Adapters no longer translate between per-tool formats — they just decide which standard skills directories to render into. +Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. + ## Supported adapters | Adapter | Output | Read natively by | diff --git a/sync.py b/sync.py index aed1b06..d7b92a1 100755 --- a/sync.py +++ b/sync.py @@ -820,9 +820,23 @@ def main(): else: skill_files = all_skill_files + # Spec-compliance gate, enforced on every path (write, --dry-run, --validate). + # A frontmatter-name/directory mismatch makes sync_skill write output under a + # directory the spec says shouldn't exist, so this must fail before any write + # rather than only under --validate. + name_errors = validate_skill_names(skill_files) + if name_errors: + print("Skill-name validation failed — frontmatter not spec-compliant " + "(see agentskills.io):\n") + for e in name_errors: + print(e) + sys.exit(1) + # --validate: pre-flight placeholder check + permissions check, no writes if args.validate: failed = False + print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") + errors = validate_placeholders(skill_files, project_config) if errors: print("Placeholder validation failed — undefined placeholders:\n") @@ -832,16 +846,6 @@ def main(): else: print("Placeholder validation passed — all placeholders are defined.") - name_errors = validate_skill_names(skill_files) - if name_errors: - print("\nSkill-name validation failed — frontmatter not spec-compliant " - "(see agentskills.io):\n") - for e in name_errors: - print(e) - failed = True - else: - print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") - # When sync.py runs from inside the CodeCannon repo itself (rather than # as a consumer submodule), validate permissions across every skill group # so a gap in a non-enabled group can't ship unnoticed. diff --git a/tests/test_sync.py b/tests/test_sync.py index 35171fc..7ab9d8c 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -1,6 +1,8 @@ """Tests for CodeCannon sync.py — the sync engine.""" +import contextlib import hashlib +import io import json import os import re @@ -1207,6 +1209,75 @@ def test_validate_passes_when_optional_placeholder_omitted(self): with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): sync.main() # should not raise SystemExit(1) + def _fake_checkout(self, tmpdir, skill_dir_name, frontmatter): + """Build a throwaway CodeCannon checkout containing a single skill. + + Symlinks the pieces main() reads from CODECANNON_DIR (adapter defs, + schema, permissions) back to the real repo, so only the skill under + test is synthetic. Returns the config path to pass as --config. + """ + fake_root = Path(tmpdir) / "codecannon" + fake_root.mkdir() + for entry in ("adapters", "config.schema.yaml", "permissions.yaml"): + (fake_root / entry).symlink_to(REPO_ROOT / entry) + + skill_dir = fake_root / "skills" / "testgroup" / skill_dir_name + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(f"---\n{frontmatter}\n---\n\nbody\n") + + project_root = Path(tmpdir) / "project" + project_root.mkdir() + cfg = project_root / ".codecannon.yaml" + cfg.write_text("skill_group: testgroup\nadapters:\n - claude\nconfig:\n FOO: bar\n") + os.chdir(project_root) + return cfg + + def test_name_directory_mismatch_blocks_write(self): + """A frontmatter-name/directory mismatch must abort the write path + before any output is produced — not just under --validate. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "real-dir", 'name: other-name\ndescription: "d"') + with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + written = list((Path(tmpdir) / "project").rglob("SKILL.md")) + self.assertEqual(written, [], "no output should be written when the gate fails") + + def test_name_directory_mismatch_blocks_dry_run(self): + """CI runs --dry-run, so the gate must fail there too. #221. + + --dry-run exits 1 on pending writes regardless, so assert on the + reason: the run must stop at the name gate and never reach the + sync loop that reports what it would write. + """ + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "real-dir", 'name: other-name\ndescription: "d"') + buf = io.StringIO() + with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sys.argv", ["sync.py", "--config", str(cfg), "--dry-run"]): + with contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + out = buf.getvalue() + self.assertIn("Skill-name validation failed", out) + self.assertNotIn("would write", out) + + def test_spec_compliant_skill_syncs(self): + """The gate must not block a compliant skill from being written.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"') + with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + sync.main() + out = Path(tmpdir) / "project" / ".claude" / "skills" / "good-skill" / "SKILL.md" + self.assertTrue(out.exists(), "compliant skill should be written") + def test_nonexistent_skill_group_exits_1(self): """skill_group naming a directory that doesn't exist should fail loudly.""" with tempfile.TemporaryDirectory() as tmpdir: From 677c9efd45431c9541e74c085c01223eb65a1b42 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Thu, 3 Sep 2026 17:44:17 -0400 Subject: [PATCH 02/10] Defer the skill-name exit under --validate so all checks still report --- .github/workflows/sync-check.yml | 6 +++--- sync.py | 16 ++++++++++++++-- tests/test_sync.py | 21 +++++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/.github/workflows/sync-check.yml b/.github/workflows/sync-check.yml index fc67c61..9005959 100644 --- a/.github/workflows/sync-check.yml +++ b/.github/workflows/sync-check.yml @@ -9,8 +9,8 @@ jobs: - uses: actions/checkout@v5 - name: Check for sync drift run: | + # placeholder, permission, and command-shape checks, reported together; + # skill names are gated on every path, including the --dry-run below ./sync.py --validate - # placeholder, permission, and command-shape checks; skill names are - # gated on every path, including the --dry-run below - ./sync.py --dry-run # exits non-zero if any files would be written + ./sync.py --dry-run diff --git a/sync.py b/sync.py index d7b92a1..6a3e90d 100755 --- a/sync.py +++ b/sync.py @@ -824,8 +824,13 @@ def main(): # A frontmatter-name/directory mismatch makes sync_skill write output under a # directory the spec says shouldn't exist, so this must fail before any write # rather than only under --validate. + # + # Under --validate the exit is deferred rather than immediate: that mode is a + # report, and short-circuiting here would hide the placeholder, permission, + # and command-shape results behind a name error, costing a round trip per + # class of problem. The block below accumulates instead. name_errors = validate_skill_names(skill_files) - if name_errors: + if name_errors and not args.validate: print("Skill-name validation failed — frontmatter not spec-compliant " "(see agentskills.io):\n") for e in name_errors: @@ -835,7 +840,14 @@ def main(): # --validate: pre-flight placeholder check + permissions check, no writes if args.validate: failed = False - print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") + if name_errors: + print("Skill-name validation failed — frontmatter not spec-compliant " + "(see agentskills.io):\n") + for e in name_errors: + print(e) + failed = True + else: + print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") errors = validate_placeholders(skill_files, project_config) if errors: diff --git a/tests/test_sync.py b/tests/test_sync.py index 7ab9d8c..a22a1e7 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -1267,6 +1267,27 @@ def test_name_directory_mismatch_blocks_dry_run(self): self.assertIn("Skill-name validation failed", out) self.assertNotIn("would write", out) + def test_validate_reports_all_checks_despite_name_error(self): + """--validate is a report, not a fail-fast gate: a name error must not + short-circuit the placeholder/permission/command-shape results, or each + class of problem costs its own round trip. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "real-dir", 'name: other-name\ndescription: "d"') + buf = io.StringIO() + with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): + with contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + out = buf.getvalue() + self.assertIn("Skill-name validation failed", out) + self.assertNotIn("Skill-name validation passed", out) + self.assertIn("Placeholder validation", out) + self.assertIn("Permission validation", out) + self.assertIn("Command-shape validation", out) + def test_spec_compliant_skill_syncs(self): """The gate must not block a compliant skill from being written.""" with tempfile.TemporaryDirectory() as tmpdir: From a0fbc99f142bcb30afe909744dfb38a7fd589c03 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Thu, 3 Sep 2026 18:51:16 -0400 Subject: [PATCH 03/10] Exempt prompt-only entries from the name gate, widen its scope, and dedupe error reporting --- docs/adapters.md | 2 +- sync.py | 76 ++++++++++++++++++++++++++---------------- tests/test_sync.py | 83 ++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 118 insertions(+), 43 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index b939b1b..d340674 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -2,7 +2,7 @@ Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `/SKILL.md` folder with YAML frontmatter (`name`, `description`). Adapters no longer translate between per-tool formats — they just decide which standard skills directories to render into. -Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. +Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a violation in a group nobody has turned on still fails CI. ## Supported adapters diff --git a/sync.py b/sync.py index 6a3e90d..59fa365 100755 --- a/sync.py +++ b/sync.py @@ -517,16 +517,38 @@ def sync_skill(skill_path, adapter, project_config, project_root, args): return False +def report_errors(header, errors, leading_blank=True): + """Print a validation failure header and its error lines. + + Shared by every validation block so the wording and spacing can't drift + between the write-path gate and the --validate report. The first block in + a run passes leading_blank=False; the rest are separated by a blank line. + """ + if leading_blank: + print() + print(header + '\n') + for e in errors: + print(e) + + # Agent Skills spec (agentskills.io): lowercase alphanumerics and hyphens, no # leading/trailing/consecutive hyphens, max 64 chars, must match the directory. _SKILL_NAME_RE = re.compile(r'^[a-z0-9]+(-[a-z0-9]+)*$') def validate_skill_names(skill_files): - """Check each SKILL.md's frontmatter against the Agent Skills spec.""" + """Check each SKILL.md's frontmatter against the Agent Skills spec. + + Entries declaring `output_path_override` are skipped: they render as bare + files with no frontmatter (see `sync_skill`), so they are not skills in the + spec sense and neither `name` nor `description` reaches their output. + Holding them to the spec would block the sync over fields nobody reads. + """ errors = [] for skill_path in skill_files: fm, _ = parse_frontmatter(skill_path.read_text()) + if fm.get('output_path_override'): + continue dir_name = skill_path.parent.name name = fm.get('name', '') if not name: @@ -820,6 +842,17 @@ def main(): else: skill_files = all_skill_files + # Skills held to the spec. Scope mirrors the permission check: when sync.py + # runs from inside the CodeCannon repo itself (rather than as a consumer + # submodule), every group is checked so a violation in a non-enabled group + # can't ship unnoticed; a consumer repo only has the enabled group. The + # --skill filter deliberately does not narrow this — syncing one skill must + # not weaken enforcement over the rest. + if CODECANNON_DIR == project_root: + audit_skill_files = sorted(skills_dir.glob('*/*/SKILL.md')) + else: + audit_skill_files = all_skill_files + # Spec-compliance gate, enforced on every path (write, --dry-run, --validate). # A frontmatter-name/directory mismatch makes sync_skill write output under a # directory the spec says shouldn't exist, so this must fail before any write @@ -829,57 +862,42 @@ def main(): # report, and short-circuiting here would hide the placeholder, permission, # and command-shape results behind a name error, costing a round trip per # class of problem. The block below accumulates instead. - name_errors = validate_skill_names(skill_files) + name_errors = validate_skill_names(audit_skill_files) + NAME_FAILURE_HEADER = ("Skill-name validation failed — frontmatter not " + "spec-compliant (see agentskills.io):") if name_errors and not args.validate: - print("Skill-name validation failed — frontmatter not spec-compliant " - "(see agentskills.io):\n") - for e in name_errors: - print(e) + report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) sys.exit(1) # --validate: pre-flight placeholder check + permissions check, no writes if args.validate: failed = False if name_errors: - print("Skill-name validation failed — frontmatter not spec-compliant " - "(see agentskills.io):\n") - for e in name_errors: - print(e) + report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) failed = True else: print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") errors = validate_placeholders(skill_files, project_config) if errors: - print("Placeholder validation failed — undefined placeholders:\n") - for e in errors: - print(e) + report_errors("Placeholder validation failed — undefined placeholders:", errors) failed = True else: print("Placeholder validation passed — all placeholders are defined.") - # When sync.py runs from inside the CodeCannon repo itself (rather than - # as a consumer submodule), validate permissions across every skill group - # so a gap in a non-enabled group can't ship unnoticed. - if CODECANNON_DIR == project_root: - perm_skill_files = sorted(skills_dir.glob('*/*/SKILL.md')) - else: - perm_skill_files = all_skill_files - perm_errors = validate_permissions(perm_skill_files) + perm_errors = validate_permissions(audit_skill_files) if perm_errors: - print("\nPermission validation failed — commands not in permissions.yaml:\n") - for e in perm_errors: - print(e) + report_errors("Permission validation failed — commands not in permissions.yaml:", + perm_errors) failed = True else: print("Permission validation passed — all command prefixes are listed.") - shape_errors = validate_command_shapes(perm_skill_files) + shape_errors = validate_command_shapes(audit_skill_files) if shape_errors: - print("\nCommand-shape validation failed — un-allowlistable shell shapes " - "(these prompt on every run and can't be 'always allowed'):\n") - for e in shape_errors: - print(e) + report_errors("Command-shape validation failed — un-allowlistable shell shapes " + "(these prompt on every run and can't be 'always allowed'):", + shape_errors) failed = True else: print("Command-shape validation passed — all commands are allowlist-friendly.") diff --git a/tests/test_sync.py b/tests/test_sync.py index a22a1e7..2de566e 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -750,6 +750,16 @@ def test_missing_description_reported(self): errors = sync.validate_skill_names([path]) self.assertTrue(any("missing required 'description'" in e for e in errors)) + def test_output_path_override_entry_is_exempt(self): + """A prompt-only entry renders as a bare file with no frontmatter, so + it isn't a skill in the spec sense and must not be held to the spec — + otherwise it blocks the whole sync over fields nobody reads. #221.""" + path = _make_skill( + self.tmpdir, "prompt-dir", + 'name: Some_Other_Name\noutput_path_override: ".claude/prompt.md"', + "body") + self.assertEqual(sync.validate_skill_names([path]), []) + # ═══════════════════════════════════════════════════════════════════════════════ # SYNC SKILL (integration-level) @@ -1209,24 +1219,37 @@ def test_validate_passes_when_optional_placeholder_omitted(self): with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): sync.main() # should not raise SystemExit(1) - def _fake_checkout(self, tmpdir, skill_dir_name, frontmatter): + def _fake_checkout(self, tmpdir, skill_dir_name, frontmatter, extra_groups=None, + in_repo=False): """Build a throwaway CodeCannon checkout containing a single skill. Symlinks the pieces main() reads from CODECANNON_DIR (adapter defs, schema, permissions) back to the real repo, so only the skill under - test is synthetic. Returns the config path to pass as --config. + test is synthetic. `extra_groups` adds {group: {dir: frontmatter}} + alongside the enabled `testgroup`. `in_repo=True` makes the checkout + its own project root, the shape that widens auditing to every group. + Returns the config path to pass as --config. """ - fake_root = Path(tmpdir) / "codecannon" + # Resolved: os.chdir() below yields a resolved cwd, and main() compares + # CODECANNON_DIR against it to decide whether it is running in-repo. + fake_root = (Path(tmpdir) / "codecannon").resolve() fake_root.mkdir() + self.fake_root = fake_root for entry in ("adapters", "config.schema.yaml", "permissions.yaml"): (fake_root / entry).symlink_to(REPO_ROOT / entry) - skill_dir = fake_root / "skills" / "testgroup" / skill_dir_name - skill_dir.mkdir(parents=True) - (skill_dir / "SKILL.md").write_text(f"---\n{frontmatter}\n---\n\nbody\n") - - project_root = Path(tmpdir) / "project" - project_root.mkdir() + groups = {"testgroup": {skill_dir_name: frontmatter}} + for group, skills in (extra_groups or {}).items(): + groups.setdefault(group, {}).update(skills) + for group, skills in groups.items(): + for dir_name, fm in skills.items(): + skill_dir = fake_root / "skills" / group / dir_name + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(f"---\n{fm}\n---\n\nbody\n") + + project_root = fake_root if in_repo else Path(tmpdir) / "project" + if not in_repo: + project_root.mkdir() cfg = project_root / ".codecannon.yaml" cfg.write_text("skill_group: testgroup\nadapters:\n - claude\nconfig:\n FOO: bar\n") os.chdir(project_root) @@ -1238,7 +1261,7 @@ def test_name_directory_mismatch_blocks_write(self): with tempfile.TemporaryDirectory() as tmpdir: cfg = self._fake_checkout( tmpdir, "real-dir", 'name: other-name\ndescription: "d"') - with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sync.CODECANNON_DIR", self.fake_root): with patch("sys.argv", ["sync.py", "--config", str(cfg)]): with self.assertRaises(SystemExit) as ctx: sync.main() @@ -1257,7 +1280,7 @@ def test_name_directory_mismatch_blocks_dry_run(self): cfg = self._fake_checkout( tmpdir, "real-dir", 'name: other-name\ndescription: "d"') buf = io.StringIO() - with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sync.CODECANNON_DIR", self.fake_root): with patch("sys.argv", ["sync.py", "--config", str(cfg), "--dry-run"]): with contextlib.redirect_stdout(buf): with self.assertRaises(SystemExit) as ctx: @@ -1275,7 +1298,7 @@ class of problem costs its own round trip. #221.""" cfg = self._fake_checkout( tmpdir, "real-dir", 'name: other-name\ndescription: "d"') buf = io.StringIO() - with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sync.CODECANNON_DIR", self.fake_root): with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): with contextlib.redirect_stdout(buf): with self.assertRaises(SystemExit) as ctx: @@ -1288,12 +1311,46 @@ class of problem costs its own round trip. #221.""" self.assertIn("Permission validation", out) self.assertIn("Command-shape validation", out) + def test_name_violation_in_non_enabled_group_blocks_in_repo(self): + """Inside the CodeCannon repo, auditing widens to every group — same as + the permission check — so a violation in a group nobody has enabled + can't ship green through CI. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', + extra_groups={"othergroup": {"real-dir": 'name: other-name\ndescription: "d"'}}, + in_repo=True) + buf = io.StringIO() + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + with contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + self.assertIn("other-name", buf.getvalue()) + + def test_prompt_only_entry_does_not_block_sync(self): + """An output_path_override entry is exempt from the spec, so a + name/directory mismatch on one must not abort the run. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', + extra_groups={"testgroup": { + "prompt-dir": 'name: Some_Other_Name\n' + 'output_path_override: ".claude/prompt.md"'}}) + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + sync.main() + project = Path(tmpdir) / "project" + self.assertTrue((project / ".claude" / "skills" / "good-skill" / "SKILL.md").exists()) + self.assertTrue((project / ".claude" / "prompt.md").exists()) + def test_spec_compliant_skill_syncs(self): """The gate must not block a compliant skill from being written.""" with tempfile.TemporaryDirectory() as tmpdir: cfg = self._fake_checkout( tmpdir, "good-skill", 'name: good-skill\ndescription: "d"') - with patch("sync.CODECANNON_DIR", Path(tmpdir) / "codecannon"): + with patch("sync.CODECANNON_DIR", self.fake_root): with patch("sys.argv", ["sync.py", "--config", str(cfg)]): sync.main() out = Path(tmpdir) / "project" / ".claude" / "skills" / "good-skill" / "SKILL.md" From c2340bdab24d3041e054fefff273e4824e541c6b Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Fri, 4 Sep 2026 09:43:17 -0400 Subject: [PATCH 04/10] Resolve CODECANNON_DIR and decide the prompt-only exemption on the substituted override --- sync.py | 20 ++++++++++++++++---- tests/test_sync.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/sync.py b/sync.py index 59fa365..fa86300 100755 --- a/sync.py +++ b/sync.py @@ -24,7 +24,10 @@ import subprocess from pathlib import Path -CODECANNON_DIR = Path(__file__).parent +# Resolved so it can be compared against Path.cwd(), which is always symlink- +# resolved. An unresolved value here makes the in-repo check below silently +# false whenever the checkout path contains a symlink. +CODECANNON_DIR = Path(__file__).resolve().parent MARKER = "generated by CodeCannon/sync.py" LEGACY_MARKERS = [ "generated by CodeCannon/sync.sh", @@ -536,18 +539,27 @@ def report_errors(header, errors, leading_blank=True): _SKILL_NAME_RE = re.compile(r'^[a-z0-9]+(-[a-z0-9]+)*$') -def validate_skill_names(skill_files): +def validate_skill_names(skill_files, project_config=None): """Check each SKILL.md's frontmatter against the Agent Skills spec. Entries declaring `output_path_override` are skipped: they render as bare files with no frontmatter (see `sync_skill`), so they are not skills in the spec sense and neither `name` nor `description` reaches their output. Holding them to the spec would block the sync over fields nobody reads. + + The exemption is decided on the *substituted* override, matching the branch + `sync_skill` takes. An override that resolves to empty (e.g. a consumer sets + its placeholder to "") sends `sync_skill` down the frontmatter branch, so the + entry does render as a skill and must be held to the spec after all. Pass + project_config to get that resolution; without it the raw value is used. """ errors = [] for skill_path in skill_files: fm, _ = parse_frontmatter(skill_path.read_text()) - if fm.get('output_path_override'): + override = fm.get('output_path_override', '') + if override and project_config is not None: + override = apply_placeholders(override, project_config) + if override: continue dir_name = skill_path.parent.name name = fm.get('name', '') @@ -862,7 +874,7 @@ def main(): # report, and short-circuiting here would hide the placeholder, permission, # and command-shape results behind a name error, costing a round trip per # class of problem. The block below accumulates instead. - name_errors = validate_skill_names(audit_skill_files) + name_errors = validate_skill_names(audit_skill_files, project_config) NAME_FAILURE_HEADER = ("Skill-name validation failed — frontmatter not " "spec-compliant (see agentskills.io):") if name_errors and not args.validate: diff --git a/tests/test_sync.py b/tests/test_sync.py index 2de566e..74d5e9f 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -760,6 +760,26 @@ def test_output_path_override_entry_is_exempt(self): "body") self.assertEqual(sync.validate_skill_names([path]), []) + def test_override_substituted_before_exemption(self): + """A placeholder override that resolves to a real path stays exempt.""" + path = _make_skill( + self.tmpdir, "prompt-dir", + 'name: Some_Other_Name\noutput_path_override: "{{PROMPT_PATH}}"', + "body") + self.assertEqual( + sync.validate_skill_names([path], {"PROMPT_PATH": ".claude/prompt.md"}), []) + + def test_override_resolving_to_empty_is_not_exempt(self): + """sync_skill branches on the *substituted* override, so one whose + placeholder resolves to "" renders as an ordinary skill — and must be + held to the spec, or it emits the very directory the gate prevents.""" + path = _make_skill( + self.tmpdir, "prompt-dir", + 'name: Some_Other_Name\noutput_path_override: "{{PROMPT_PATH}}"', + "body") + errors = sync.validate_skill_names([path], {"PROMPT_PATH": ""}) + self.assertTrue(any("violates the spec" in e for e in errors)) + # ═══════════════════════════════════════════════════════════════════════════════ # SYNC SKILL (integration-level) @@ -1311,6 +1331,21 @@ class of problem costs its own round trip. #221.""" self.assertIn("Permission validation", out) self.assertIn("Command-shape validation", out) + def test_codecannon_dir_is_resolved_when_loaded_via_symlink(self): + """CODECANNON_DIR is compared against Path.cwd(), which is always + symlink-resolved. Loaded through a symlinked path it must still equal + the real directory, or any symlink in the checkout silently narrows the + audit scope with no error and exit 0. #221.""" + import importlib.util + with tempfile.TemporaryDirectory() as tmpdir: + link = Path(tmpdir) / "linked-checkout" + link.symlink_to(REPO_ROOT) + spec = importlib.util.spec_from_file_location( + "sync_via_symlink", link / "sync.py") + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + self.assertEqual(mod.CODECANNON_DIR, REPO_ROOT.resolve()) + def test_name_violation_in_non_enabled_group_blocks_in_repo(self): """Inside the CodeCannon repo, auditing widens to every group — same as the permission check — so a violation in a group nobody has enabled From 087c4d2af5ccbf0661a999439a3ccf0388d8acc8 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Fri, 4 Sep 2026 09:51:26 -0400 Subject: [PATCH 05/10] Report unresolved override placeholders and qualify validator errors by group --- docs/adapters.md | 2 +- sync.py | 32 ++++++++++++++++++++++++++------ tests/test_sync.py | 24 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index d340674..cd5dc0c 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -2,7 +2,7 @@ Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `/SKILL.md` folder with YAML frontmatter (`name`, `description`). Adapters no longer translate between per-tool formats — they just decide which standard skills directories to render into. -Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a violation in a group nobody has turned on still fails CI. +Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense — but only when the override resolves to a real path. If its placeholder is set to an empty value the entry renders as an ordinary skill and is held to the spec like any other; if the placeholder is undefined entirely, that is reported as its own error rather than exempted. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a violation in a group nobody has turned on still fails CI. ## Supported adapters diff --git a/sync.py b/sync.py index fa86300..1715903 100755 --- a/sync.py +++ b/sync.py @@ -534,6 +534,16 @@ def report_errors(header, errors, leading_blank=True): print(e) +def skill_label(skill_path): + """Group-qualified label for a skill file, e.g. `github-agile/start/SKILL.md`. + + The validators audit every group when run inside the CodeCannon repo, and + two groups can hold the same skill name, so a bare directory name would + point at the wrong file. + """ + return f"{skill_path.parent.parent.name}/{skill_path.parent.name}/{skill_path.name}" + + # Agent Skills spec (agentskills.io): lowercase alphanumerics and hyphens, no # leading/trailing/consecutive hyphens, max 64 chars, must match the directory. _SKILL_NAME_RE = re.compile(r'^[a-z0-9]+(-[a-z0-9]+)*$') @@ -552,26 +562,36 @@ def validate_skill_names(skill_files, project_config=None): its placeholder to "") sends `sync_skill` down the frontmatter branch, so the entry does render as a skill and must be held to the spec after all. Pass project_config to get that resolution; without it the raw value is used. + + An override left holding an undefined placeholder is neither: `sync_skill` + would write to a literal `{{FOO}}` directory. That is reported outright + rather than exempted or spec-checked, since no frontmatter fix repairs it. """ errors = [] for skill_path in skill_files: fm, _ = parse_frontmatter(skill_path.read_text()) + label = skill_label(skill_path) override = fm.get('output_path_override', '') if override and project_config is not None: override = apply_placeholders(override, project_config) + unresolved = sorted(set(find_unresolved(override))) + if unresolved: + errors.append(f" {label}: output_path_override has undefined " + f"placeholder(s): {', '.join(unresolved)}") + continue if override: continue dir_name = skill_path.parent.name name = fm.get('name', '') if not name: - errors.append(f" {dir_name}/SKILL.md: missing required 'name' field") + errors.append(f" {label}: missing required 'name' field") elif not _SKILL_NAME_RE.match(name) or len(name) > 64: - errors.append(f" {dir_name}/SKILL.md: name '{name}' violates the spec " + errors.append(f" {label}: name '{name}' violates the spec " "(lowercase alphanumerics and single hyphens, max 64 chars)") elif name != dir_name: - errors.append(f" {dir_name}/SKILL.md: name '{name}' must match its directory name") + errors.append(f" {label}: name '{name}' must match its directory name") if not fm.get('description'): - errors.append(f" {dir_name}/SKILL.md: missing required 'description' field") + errors.append(f" {label}: missing required 'description' field") return errors @@ -633,7 +653,7 @@ def validate_permissions(skill_files): # For simple commands (git, make), check the prefix if token not in allowed and token not in seen: seen.add(token) - errors.append(f" {skill_path.parent.name}/{skill_path.name}: command '{token}' not in permissions.yaml") + errors.append(f" {skill_label(skill_path)}: command '{token}' not in permissions.yaml") return errors @@ -684,7 +704,7 @@ def validate_command_shapes(skill_files): if bad in line: lineno = fence_line + 1 + i errors.append( - f" {skill_path.parent.name}/{skill_path.name}:{lineno}: " + f" {skill_label(skill_path)}:{lineno}: " f"'{bad}' — {msg}\n {line}") break diff --git a/tests/test_sync.py b/tests/test_sync.py index 74d5e9f..8062c67 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -780,6 +780,30 @@ def test_override_resolving_to_empty_is_not_exempt(self): errors = sync.validate_skill_names([path], {"PROMPT_PATH": ""}) self.assertTrue(any("violates the spec" in e for e in errors)) + def test_override_with_undefined_placeholder_is_reported(self): + """An override left holding {{FOO}} would have sync_skill write to a + literal '{{FOO}}' directory, which no frontmatter fix repairs — so it + is reported outright rather than exempted or spec-checked. #221.""" + path = _make_skill( + self.tmpdir, "prompt-dir", + 'name: prompt-dir\ndescription: "d"\n' + 'output_path_override: "{{PROMPT_PATH}}"', + "body") + errors = sync.validate_skill_names([path], {"OTHER": "x"}) + self.assertEqual(len(errors), 1) + self.assertIn("output_path_override has undefined placeholder(s): PROMPT_PATH", + errors[0]) + + def test_errors_are_group_qualified(self): + """Auditing spans every group in-repo, and two groups can hold the same + skill name, so a bare directory name would point at the wrong file.""" + path = _make_skill(self.tmpdir, "dir-name", + 'name: other-name\ndescription: "d"', "body") + errors = sync.validate_skill_names([path]) + self.assertTrue(errors) + expected = f"{path.parent.parent.name}/dir-name/SKILL.md" + self.assertTrue(all(expected in e for e in errors), errors) + # ═══════════════════════════════════════════════════════════════════════════════ # SYNC SKILL (integration-level) From 50991a584feac9b17e8e7dae9e697e48c9b1dc27 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Sat, 5 Sep 2026 09:25:27 -0400 Subject: [PATCH 06/10] Match digit-bearing placeholders and scope override resolution to the enabled group --- sync.py | 20 +++++++++++++++++--- tests/test_sync.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/sync.py b/sync.py index 1715903..51ab21e 100755 --- a/sync.py +++ b/sync.py @@ -318,8 +318,13 @@ def apply_placeholders(text, values): def find_unresolved(text): - """Return list of placeholder names that were not substituted.""" - return re.findall(r'\{\{([A-Z_]+)\}\}', text) + """Return list of placeholder names that were not substituted. + + Digits are matched as well as letters: the name gate treats an unresolved + override as an error, so a key this misses (e.g. `{{PROMPT_PATH2}}`) would + slip through as a literal output directory. + """ + return re.findall(r'\{\{([A-Z0-9_]+)\}\}', text) # ── Hash and change detection ───────────────────────────────────────────────── @@ -894,7 +899,16 @@ def main(): # report, and short-circuiting here would hide the placeholder, permission, # and command-shape results behind a name error, costing a round trip per # class of problem. The block below accumulates instead. - name_errors = validate_skill_names(audit_skill_files, project_config) + # Placeholders resolve against the enabled group's config, so only that + # group's overrides can be substituted. Other groups are checked with the + # raw value — the same reason validate_placeholders stays scoped to + # skill_files — otherwise a group nobody enabled would hard-fail the sync + # over a placeholder the current config was never meant to define. + enabled_group = set(all_skill_files) + name_errors = validate_skill_names( + [f for f in audit_skill_files if f in enabled_group], project_config) + name_errors += validate_skill_names( + [f for f in audit_skill_files if f not in enabled_group]) NAME_FAILURE_HEADER = ("Skill-name validation failed — frontmatter not " "spec-compliant (see agentskills.io):") if name_errors and not args.validate: diff --git a/tests/test_sync.py b/tests/test_sync.py index 8062c67..8fa7090 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -794,6 +794,19 @@ def test_override_with_undefined_placeholder_is_reported(self): self.assertIn("output_path_override has undefined placeholder(s): PROMPT_PATH", errors[0]) + def test_override_placeholder_with_digits_is_reported(self): + """find_unresolved is load-bearing for this gate, so its regex must + match digit-bearing keys — otherwise {{PROMPT_PATH2}} slips through the + exemption and sync_skill writes to it as a literal directory. #221.""" + path = _make_skill( + self.tmpdir, "prompt-dir", + 'name: prompt-dir\ndescription: "d"\n' + 'output_path_override: "{{PROMPT_PATH2}}"', + "body") + errors = sync.validate_skill_names([path], {"OTHER": "x"}) + self.assertEqual(len(errors), 1) + self.assertIn("PROMPT_PATH2", errors[0]) + def test_errors_are_group_qualified(self): """Auditing spans every group in-repo, and two groups can hold the same skill name, so a bare directory name would point at the wrong file.""" @@ -1388,6 +1401,23 @@ def test_name_violation_in_non_enabled_group_blocks_in_repo(self): self.assertEqual(ctx.exception.code, 1) self.assertIn("other-name", buf.getvalue()) + def test_other_group_override_placeholder_does_not_block_sync(self): + """The widened audit spans groups, but config is per-enabled-group. A + non-enabled group's override placeholder is not resolvable against the + current config and must not hard-fail the sync. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', + extra_groups={"othergroup": {"prompt-dir": ( + 'name: prompt-dir\ndescription: "d"\n' + 'output_path_override: "{{OTHER_GROUP_PATH}}"')}}, + in_repo=True) + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + sync.main() + out = self.fake_root / ".claude" / "skills" / "good-skill" / "SKILL.md" + self.assertTrue(out.exists(), "enabled group should still sync") + def test_prompt_only_entry_does_not_block_sync(self): """An output_path_override entry is exempt from the spec, so a name/directory mismatch on one must not abort the run. #221.""" From 7236d907bb22048a5fcae98ea7f5bd8334a2e0d1 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Sat, 5 Sep 2026 11:33:43 -0400 Subject: [PATCH 07/10] Align the conditional-directive charset and qualify placeholder errors by skill --- docs/adapters.md | 2 +- sync.py | 7 +++++-- tests/test_sync.py | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index cd5dc0c..007b6a5 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -2,7 +2,7 @@ Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `/SKILL.md` folder with YAML frontmatter (`name`, `description`). Adapters no longer translate between per-tool formats — they just decide which standard skills directories to render into. -Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense — but only when the override resolves to a real path. If its placeholder is set to an empty value the entry renders as an ordinary skill and is held to the spec like any other; if the placeholder is undefined entirely, that is reported as its own error rather than exempted. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a violation in a group nobody has turned on still fails CI. +Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense — but only when the override resolves to a real path. If its placeholder is set to an empty value the entry renders as an ordinary skill and is held to the spec like any other; if the placeholder is undefined entirely, that is reported as its own error rather than exempted. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a name or description violation in a group nobody has turned on still fails CI. Override placeholders are the exception: they resolve against the enabled group's config, so a non-enabled group's `output_path_override` is checked only for its raw value and any undefined placeholder in it surfaces when that group is enabled. ## Supported adapters diff --git a/sync.py b/sync.py index 51ab21e..c7d7e3b 100755 --- a/sync.py +++ b/sync.py @@ -261,7 +261,10 @@ def parse_frontmatter(text): # The directive lines are always removed from the output. # Nesting is supported (inner blocks are evaluated innermost-first). -_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z_]+)\}\}\s*$') +# Key charset must stay in step with find_unresolved: a key one matches and the +# other doesn't is neither expanded as a directive nor reported as unresolved, +# so the literal line ships into generated output. +_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z0-9_]+)\}\}\s*$') _IF_CLOSE = re.compile(r'^\s*\{\{/if\}\}\s*$') @@ -613,7 +616,7 @@ def validate_placeholders(skill_files, project_config): text_to_check += '\n' + apply_conditionals(fm['description'], project_config) missing = [p for p in find_unresolved(text_to_check) if p not in project_config] for p in missing: - errors.append(f" {skill_path.name}: {{{{{p}}}}} not defined in config") + errors.append(f" {skill_label(skill_path)}: {{{{{p}}}}} not defined in config") return errors diff --git a/tests/test_sync.py b/tests/test_sync.py index 8fa7090..b5f4c2e 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -321,6 +321,15 @@ def test_truthy_keeps_block(self): self.assertNotIn("{{#if", result) self.assertNotIn("{{/if}}", result) + def test_digit_bearing_key_is_a_directive(self): + """_IF_OPEN's charset must stay in step with find_unresolved: a key one + matches and the other doesn't is neither expanded nor reported, so the + literal directive line ships into generated output. #221.""" + text = "before\n{{#if FOO2}}\nkept\n{{/if}}\nafter" + result = sync.apply_conditionals(text, {"FOO2": "yes"}) + self.assertIn("kept", result) + self.assertNotIn("{{#if", result) + def test_falsy_removes_block(self): text = "before\n{{#if FOO}}\nremoved\n{{/if}}\nafter" result = sync.apply_conditionals(text, {"FOO": ""}) @@ -1005,6 +1014,13 @@ def test_reports_undefined_placeholder(self): self.assertEqual(len(errors), 1) self.assertIn("MISSING", errors[0]) + def test_error_names_the_skill_not_just_SKILL_md(self): + """Every SKILL.md shares a filename, so the label must carry the group + and skill directory or the report points at nothing. #221.""" + path = _make_skill(self.tmpdir, "bad", 'name: bad\ndescription: "test"', "Use {{MISSING}}") + errors = sync.validate_placeholders([path], {}) + self.assertIn(f"{path.parent.parent.name}/bad/SKILL.md", errors[0]) + def test_placeholder_in_stripped_conditional_not_reported(self): body = "{{#if ACTIVE}}\n{{OPTIONAL}}\n{{/if}}\nPlain text." path = _make_skill(self.tmpdir, "cond", 'name: cond\ndescription: "test"', body) From 55d485892083b12fb9558ec311206dfee82b839c Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Sat, 5 Sep 2026 11:41:18 -0400 Subject: [PATCH 08/10] Split output-path validation from the name check and scope the write gate to the enabled group --- docs/adapters.md | 2 +- sync.py | 75 +++++++++++++++++++--------- tests/test_sync.py | 119 +++++++++++++++++++++++++++++++++------------ 3 files changed, 143 insertions(+), 53 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index 007b6a5..c713f0f 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -2,7 +2,7 @@ Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `/SKILL.md` folder with YAML frontmatter (`name`, `description`). Adapters no longer translate between per-tool formats — they just decide which standard skills directories to render into. -Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense — but only when the override resolves to a real path. If its placeholder is set to an empty value the entry renders as an ordinary skill and is held to the spec like any other; if the placeholder is undefined entirely, that is reported as its own error rather than exempted. Run from inside the Code Cannon repo itself, the check covers every skill group rather than just the enabled one, so a name or description violation in a group nobody has turned on still fails CI. Override placeholders are the exception: they resolve against the enabled group's config, so a non-enabled group's `output_path_override` is checked only for its raw value and any undefined placeholder in it surfaces when that group is enabled. +Spec compliance is enforced on every sync path, not just under `--validate`: if a skill's frontmatter `name` is missing, violates the spec, or doesn't match its directory name — or its `description` is missing — `sync.py` stops with a non-zero exit before writing anything. This keeps a rename on one side of the pair from silently shipping mismatched output to consumer repos. Entries declaring `output_path_override` are exempt, since they render as bare files with no frontmatter and so aren't skills in the spec sense — but only when the override resolves to a real path. If its placeholder is set to an empty value the entry renders as an ordinary skill and is held to the spec like any other; if the placeholder is undefined entirely, that is reported as its own error rather than exempted. The write path enforces this over the group being synced — those are the skills that would produce bad output. Run from inside the Code Cannon repo itself, `--validate` widens the audit to every skill group, so a name or description violation in a group nobody has turned on still fails CI, which runs `--validate`. A work-in-progress skill in an unrelated group therefore doesn't block a local sync. Override placeholders resolve against the enabled group's config, so a non-enabled group's `output_path_override` is checked only for its raw value; an undefined placeholder in one surfaces when that group is enabled. ## Supported adapters diff --git a/sync.py b/sync.py index c7d7e3b..b3ed5da 100755 --- a/sync.py +++ b/sync.py @@ -571,9 +571,8 @@ def validate_skill_names(skill_files, project_config=None): entry does render as a skill and must be held to the spec after all. Pass project_config to get that resolution; without it the raw value is used. - An override left holding an undefined placeholder is neither: `sync_skill` - would write to a literal `{{FOO}}` directory. That is reported outright - rather than exempted or spec-checked, since no frontmatter fix repairs it. + An override left holding an undefined placeholder is neither: no frontmatter + fix repairs it, so it is skipped here and reported by validate_output_paths. """ errors = [] for skill_path in skill_files: @@ -582,10 +581,7 @@ def validate_skill_names(skill_files, project_config=None): override = fm.get('output_path_override', '') if override and project_config is not None: override = apply_placeholders(override, project_config) - unresolved = sorted(set(find_unresolved(override))) - if unresolved: - errors.append(f" {label}: output_path_override has undefined " - f"placeholder(s): {', '.join(unresolved)}") + if find_unresolved(override): continue if override: continue @@ -603,6 +599,27 @@ def validate_skill_names(skill_files, project_config=None): return errors +def validate_output_paths(skill_files, project_config): + """Check that every `output_path_override` resolves to a usable path. + + An override still holding an undefined placeholder would have `sync_skill` + write to a literal `{{FOO}}` directory. Reported separately from the name + check because the fix is a missing `config:` key in .codecannon.yaml, not + a frontmatter change. + """ + errors = [] + for skill_path in skill_files: + fm, _ = parse_frontmatter(skill_path.read_text()) + override = fm.get('output_path_override', '') + if not override: + continue + unresolved = sorted(set(find_unresolved(apply_placeholders(override, project_config)))) + if unresolved: + errors.append(f" {skill_label(skill_path)}: output_path_override has " + f"undefined placeholder(s): {', '.join(unresolved)}") + return errors + + def validate_placeholders(skill_files, project_config): """Scan all skills for {{PLACEHOLDER}} tokens; error if any are undefined.""" errors = [] @@ -893,40 +910,54 @@ def main(): else: audit_skill_files = all_skill_files - # Spec-compliance gate, enforced on every path (write, --dry-run, --validate). - # A frontmatter-name/directory mismatch makes sync_skill write output under a - # directory the spec says shouldn't exist, so this must fail before any write - # rather than only under --validate. - # - # Under --validate the exit is deferred rather than immediate: that mode is a - # report, and short-circuiting here would hide the placeholder, permission, - # and command-shape results behind a name error, costing a round trip per - # class of problem. The block below accumulates instead. # Placeholders resolve against the enabled group's config, so only that # group's overrides can be substituted. Other groups are checked with the # raw value — the same reason validate_placeholders stays scoped to # skill_files — otherwise a group nobody enabled would hard-fail the sync # over a placeholder the current config was never meant to define. enabled_group = set(all_skill_files) - name_errors = validate_skill_names( - [f for f in audit_skill_files if f in enabled_group], project_config) - name_errors += validate_skill_names( - [f for f in audit_skill_files if f not in enabled_group]) + enabled_files = [f for f in audit_skill_files if f in enabled_group] + other_files = [f for f in audit_skill_files if f not in enabled_group] + + # Spec-compliance gate. A frontmatter-name/directory mismatch makes + # sync_skill write output under a directory the spec says shouldn't exist, + # so the enabled group must fail before any write rather than only under + # --validate — that is the hole this gate closes. + # + # Other groups are never handed to sync_skill, so they cannot produce bad + # output and are not a reason to block a write. They are still audited, but + # only under --validate, matching the permission and command-shape checks. + # CI runs --validate, so the cross-group guarantee holds without a WIP skill + # in an unrelated group blocking a local ./sync.py. + name_errors = validate_skill_names(enabled_files, project_config) + path_errors = validate_output_paths(enabled_files, project_config) NAME_FAILURE_HEADER = ("Skill-name validation failed — frontmatter not " "spec-compliant (see agentskills.io):") - if name_errors and not args.validate: - report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) + PATH_FAILURE_HEADER = ("Output-path validation failed — output_path_override " + "placeholders not defined in config:") + if not args.validate and (name_errors or path_errors): + if name_errors: + report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) + if path_errors: + report_errors(PATH_FAILURE_HEADER, path_errors, leading_blank=bool(name_errors)) sys.exit(1) # --validate: pre-flight placeholder check + permissions check, no writes if args.validate: failed = False + name_errors += validate_skill_names(other_files) if name_errors: report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) failed = True else: print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") + if path_errors: + report_errors(PATH_FAILURE_HEADER, path_errors) + failed = True + else: + print("Output-path validation passed — all override placeholders are defined.") + errors = validate_placeholders(skill_files, project_config) if errors: report_errors("Placeholder validation failed — undefined placeholders:", errors) diff --git a/tests/test_sync.py b/tests/test_sync.py index b5f4c2e..78b7df0 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -789,32 +789,14 @@ def test_override_resolving_to_empty_is_not_exempt(self): errors = sync.validate_skill_names([path], {"PROMPT_PATH": ""}) self.assertTrue(any("violates the spec" in e for e in errors)) - def test_override_with_undefined_placeholder_is_reported(self): - """An override left holding {{FOO}} would have sync_skill write to a - literal '{{FOO}}' directory, which no frontmatter fix repairs — so it - is reported outright rather than exempted or spec-checked. #221.""" + def test_unresolved_override_is_left_to_validate_output_paths(self): + """An override still holding {{FOO}} is not a frontmatter problem, so + the name check stays silent and validate_output_paths owns it. #221.""" path = _make_skill( self.tmpdir, "prompt-dir", - 'name: prompt-dir\ndescription: "d"\n' - 'output_path_override: "{{PROMPT_PATH}}"', - "body") - errors = sync.validate_skill_names([path], {"OTHER": "x"}) - self.assertEqual(len(errors), 1) - self.assertIn("output_path_override has undefined placeholder(s): PROMPT_PATH", - errors[0]) - - def test_override_placeholder_with_digits_is_reported(self): - """find_unresolved is load-bearing for this gate, so its regex must - match digit-bearing keys — otherwise {{PROMPT_PATH2}} slips through the - exemption and sync_skill writes to it as a literal directory. #221.""" - path = _make_skill( - self.tmpdir, "prompt-dir", - 'name: prompt-dir\ndescription: "d"\n' - 'output_path_override: "{{PROMPT_PATH2}}"', + 'name: Some_Other_Name\noutput_path_override: "{{PROMPT_PATH}}"', "body") - errors = sync.validate_skill_names([path], {"OTHER": "x"}) - self.assertEqual(len(errors), 1) - self.assertIn("PROMPT_PATH2", errors[0]) + self.assertEqual(sync.validate_skill_names([path], {"OTHER": "x"}), []) def test_errors_are_group_qualified(self): """Auditing spans every group in-repo, and two groups can hold the same @@ -994,6 +976,48 @@ def test_regenerates_when_source_changes(self): # ═══════════════════════════════════════════════════════════════════════════════ +class TestValidateOutputPaths(unittest.TestCase): + + def setUp(self): + self.tmpdir = tempfile.mkdtemp() + + def tearDown(self): + import shutil + shutil.rmtree(self.tmpdir, ignore_errors=True) + + def test_resolved_override_passes(self): + path = _make_skill(self.tmpdir, "prompt-dir", + 'name: prompt-dir\noutput_path_override: "{{P}}"', "body") + self.assertEqual(sync.validate_output_paths([path], {"P": ".claude/x.md"}), []) + + def test_skill_without_override_is_ignored(self): + path = _make_skill(self.tmpdir, "plain", + 'name: plain\ndescription: "d"', "body") + self.assertEqual(sync.validate_output_paths([path], {}), []) + + def test_undefined_override_placeholder_is_reported(self): + """An override left holding {{FOO}} would have sync_skill write to a + literal '{{FOO}}' directory. The fix is a missing config key, not a + frontmatter change, so it gets its own check and header. #221.""" + path = _make_skill(self.tmpdir, "prompt-dir", + 'name: prompt-dir\noutput_path_override: "{{PROMPT_PATH}}"', + "body") + errors = sync.validate_output_paths([path], {"OTHER": "x"}) + self.assertEqual(len(errors), 1) + self.assertIn("output_path_override has undefined placeholder(s): PROMPT_PATH", + errors[0]) + + def test_digit_bearing_override_placeholder_is_reported(self): + """find_unresolved is load-bearing here, so its charset must match + digit-bearing keys or {{PROMPT_PATH2}} ships as a literal path. #221.""" + path = _make_skill(self.tmpdir, "prompt-dir", + 'name: prompt-dir\noutput_path_override: "{{PROMPT_PATH2}}"', + "body") + errors = sync.validate_output_paths([path], {"OTHER": "x"}) + self.assertEqual(len(errors), 1) + self.assertIn("PROMPT_PATH2", errors[0]) + + class TestValidatePlaceholders(unittest.TestCase): def setUp(self): @@ -1399,15 +1423,47 @@ def test_codecannon_dir_is_resolved_when_loaded_via_symlink(self): spec.loader.exec_module(mod) self.assertEqual(mod.CODECANNON_DIR, REPO_ROOT.resolve()) - def test_name_violation_in_non_enabled_group_blocks_in_repo(self): - """Inside the CodeCannon repo, auditing widens to every group — same as + def _checkout_with_bad_other_group(self, tmpdir): + return self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', + extra_groups={"othergroup": {"real-dir": 'name: other-name\ndescription: "d"'}}, + in_repo=True) + + def test_name_violation_in_non_enabled_group_fails_validate(self): + """Inside the CodeCannon repo, --validate audits every group — same as the permission check — so a violation in a group nobody has enabled - can't ship green through CI. #221.""" + can't ship green through CI, which runs --validate. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._checkout_with_bad_other_group(tmpdir) + buf = io.StringIO() + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): + with contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + self.assertIn("other-name", buf.getvalue()) + + def test_name_violation_in_non_enabled_group_does_not_block_write(self): + """Non-enabled groups are never handed to sync_skill, so they can't + produce bad output and must not block a local sync — otherwise a WIP + skill in an unrelated group breaks ./sync.py --force. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._checkout_with_bad_other_group(tmpdir) + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg)]): + sync.main() + out = self.fake_root / ".claude" / "skills" / "good-skill" / "SKILL.md" + self.assertTrue(out.exists(), "enabled group should still sync") + + def test_unresolved_override_in_enabled_group_blocks_write(self): + """The enabled group's overrides do reach sync_skill, so an undefined + placeholder there must stop the write under its own header. #221.""" with tempfile.TemporaryDirectory() as tmpdir: cfg = self._fake_checkout( - tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', - extra_groups={"othergroup": {"real-dir": 'name: other-name\ndescription: "d"'}}, - in_repo=True) + tmpdir, "prompt-dir", + 'name: prompt-dir\ndescription: "d"\n' + 'output_path_override: "{{NOT_DEFINED}}"') buf = io.StringIO() with patch("sync.CODECANNON_DIR", self.fake_root): with patch("sys.argv", ["sync.py", "--config", str(cfg)]): @@ -1415,7 +1471,10 @@ def test_name_violation_in_non_enabled_group_blocks_in_repo(self): with self.assertRaises(SystemExit) as ctx: sync.main() self.assertEqual(ctx.exception.code, 1) - self.assertIn("other-name", buf.getvalue()) + out = buf.getvalue() + self.assertIn("Output-path validation failed", out) + self.assertIn("NOT_DEFINED", out) + self.assertNotIn("Skill-name validation failed", out) def test_other_group_override_placeholder_does_not_block_sync(self): """The widened audit spans groups, but config is per-enabled-group. A From 6b4d117db4b697419ed34be534f6959846f48983 Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Sat, 5 Sep 2026 11:47:00 -0400 Subject: [PATCH 09/10] Require a leading letter in placeholder keys and scope the validate pass messages --- sync.py | 24 +++++++++++++++++------- tests/test_sync.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/sync.py b/sync.py index b3ed5da..d7a2db5 100755 --- a/sync.py +++ b/sync.py @@ -264,7 +264,7 @@ def parse_frontmatter(text): # Key charset must stay in step with find_unresolved: a key one matches and the # other doesn't is neither expanded as a directive nor reported as unresolved, # so the literal line ships into generated output. -_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z0-9_]+)\}\}\s*$') +_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z][A-Z0-9_]*)\}\}\s*$') _IF_CLOSE = re.compile(r'^\s*\{\{/if\}\}\s*$') @@ -323,11 +323,13 @@ def apply_placeholders(text, values): def find_unresolved(text): """Return list of placeholder names that were not substituted. - Digits are matched as well as letters: the name gate treats an unresolved - override as an error, so a key this misses (e.g. `{{PROMPT_PATH2}}`) would - slip through as a literal output directory. + Keys may contain digits but must start with a letter: the output-path check + treats an unresolved override as an error, so a key this misses (e.g. + `{{PROMPT_PATH2}}`) would slip through as a literal output directory. The + leading-letter requirement keeps all-digit tokens (`{{1}}` in a documented + regex backreference or Handlebars snippet) from reading as placeholders. """ - return re.findall(r'\{\{([A-Z0-9_]+)\}\}', text) + return re.findall(r'\{\{([A-Z][A-Z0-9_]*)\}\}', text) # ── Hash and change detection ───────────────────────────────────────────────── @@ -950,13 +952,21 @@ def main(): report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) failed = True else: - print("Skill-name validation passed — frontmatter follows the Agent Skills spec.") + # Other groups are checked without placeholder resolution, so an + # entry there whose override is a placeholder stays exempt on the + # raw value. Say so rather than implying full coverage. + caveat = (" (other groups checked without placeholder resolution)" + if other_files else "") + print("Skill-name validation passed — frontmatter follows the " + f"Agent Skills spec{caveat}.") if path_errors: report_errors(PATH_FAILURE_HEADER, path_errors) failed = True else: - print("Output-path validation passed — all override placeholders are defined.") + # Only the enabled group's overrides are resolvable against this config. + print(f"Output-path validation passed — all override placeholders in " + f"{skill_group} are defined.") errors = validate_placeholders(skill_files, project_config) if errors: diff --git a/tests/test_sync.py b/tests/test_sync.py index 78b7df0..777302a 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -520,6 +520,16 @@ def test_no_unresolved(self): result = sync.find_unresolved(text) self.assertEqual(result, []) + def test_finds_key_with_digits(self): + """Keys like PROMPT_PATH2 must be seen, or an unresolved override + placeholder slips through as a literal output directory. #221.""" + self.assertEqual(sync.find_unresolved("{{PROMPT_PATH2}}"), ["PROMPT_PATH2"]) + + def test_ignores_all_digit_token(self): + """A documented regex backreference or Handlebars snippet like {{1}} is + not a placeholder and must not fail --validate as a missing key. #221.""" + self.assertEqual(sync.find_unresolved("use {{1}} and {{0}} here"), []) + def test_ignores_lowercase(self): text = "{{lowercase}}" result = sync.find_unresolved(text) @@ -1444,6 +1454,24 @@ def test_name_violation_in_non_enabled_group_fails_validate(self): self.assertEqual(ctx.exception.code, 1) self.assertIn("other-name", buf.getvalue()) + def test_validate_pass_messages_state_their_scope(self): + """Other groups are checked without placeholder resolution and their + overrides aren't checked at all, so the pass lines must not imply full + coverage. #221.""" + with tempfile.TemporaryDirectory() as tmpdir: + cfg = self._fake_checkout( + tmpdir, "good-skill", 'name: good-skill\ndescription: "d"', + extra_groups={"othergroup": {"fine": 'name: fine\ndescription: "d"'}}, + in_repo=True) + buf = io.StringIO() + with patch("sync.CODECANNON_DIR", self.fake_root): + with patch("sys.argv", ["sync.py", "--config", str(cfg), "--validate"]): + with contextlib.redirect_stdout(buf): + sync.main() + out = buf.getvalue() + self.assertIn("other groups checked without placeholder resolution", out) + self.assertIn("all override placeholders in testgroup are defined", out) + def test_name_violation_in_non_enabled_group_does_not_block_write(self): """Non-enabled groups are never handed to sync_skill, so they can't produce bad output and must not block a local sync — otherwise a WIP From 1311d6a0f00d2538c10fcbcdfd9c9868917a34ad Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Wed, 9 Sep 2026 19:23:39 -0400 Subject: [PATCH 10/10] Restore leading-underscore placeholder keys and warn on unmatched conditional close --- sync.py | 26 +++++++++++++++++--------- tests/test_sync.py | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/sync.py b/sync.py index d7a2db5..66eb276 100755 --- a/sync.py +++ b/sync.py @@ -261,10 +261,12 @@ def parse_frontmatter(text): # The directive lines are always removed from the output. # Nesting is supported (inner blocks are evaluated innermost-first). -# Key charset must stay in step with find_unresolved: a key one matches and the -# other doesn't is neither expanded as a directive nor reported as unresolved, -# so the literal line ships into generated output. -_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z][A-Z0-9_]*)\}\}\s*$') +# Key charset matches find_unresolved's, but that is for consistency only — it +# is not a safety net: find_unresolved's pattern cannot match a `{{#if X}}` line +# (the `#` and the space), so a key this misses is not reported as unresolved +# either. It silently fails to open a block, which strands the matching +# `{{/if}}` and stops conditional processing for the whole file. +_IF_OPEN = re.compile(r'^\s*\{\{#if\s+(!?)([A-Z_][A-Z0-9_]*)\}\}\s*$') _IF_CLOSE = re.compile(r'^\s*\{\{/if\}\}\s*$') @@ -288,7 +290,13 @@ def apply_conditionals(text, values): open_idx = i break if open_idx is None: - break # malformed — stop processing + # Malformed: this {{/if}} has no matching open. Stopping here leaves + # every remaining conditional in the file unprocessed, so literal + # directive lines and blocks that should have been stripped ship + # into the output — say so rather than failing silently at exit 0. + print(" Warning: {{/if}} with no matching {{#if}} — conditional " + "blocks in this skill were left unprocessed") + break m = _IF_OPEN.match(lines[open_idx]) negated = m.group(1) == '!' @@ -323,13 +331,13 @@ def apply_placeholders(text, values): def find_unresolved(text): """Return list of placeholder names that were not substituted. - Keys may contain digits but must start with a letter: the output-path check + Keys may contain digits but may not start with one: the output-path check treats an unresolved override as an error, so a key this misses (e.g. - `{{PROMPT_PATH2}}`) would slip through as a literal output directory. The - leading-letter requirement keeps all-digit tokens (`{{1}}` in a documented + `{{PROMPT_PATH2}}`) would slip through as a literal output directory, while + excluding a leading digit keeps all-digit tokens (`{{1}}` in a documented regex backreference or Handlebars snippet) from reading as placeholders. """ - return re.findall(r'\{\{([A-Z][A-Z0-9_]*)\}\}', text) + return re.findall(r'\{\{([A-Z_][A-Z0-9_]*)\}\}', text) # ── Hash and change detection ───────────────────────────────────────────────── diff --git a/tests/test_sync.py b/tests/test_sync.py index 777302a..e726f34 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -321,6 +321,24 @@ def test_truthy_keeps_block(self): self.assertNotIn("{{#if", result) self.assertNotIn("{{/if}}", result) + def test_leading_underscore_key_is_a_directive(self): + """A key _IF_OPEN doesn't match fails to open a block, stranding its + {{/if}} and stopping conditional processing for the whole file. #221.""" + text = "before\n{{#if _INTERNAL}}\nkept\n{{/if}}\nafter" + result = sync.apply_conditionals(text, {"_INTERNAL": "yes"}) + self.assertIn("kept", result) + self.assertNotIn("{{#if", result) + self.assertNotIn("{{/if}}", result) + + def test_unmatched_close_warns(self): + """Stopping on a stray {{/if}} leaves every later conditional in the + file unprocessed — that must not happen silently at exit 0. #221.""" + text = "{{/if}}\n{{#if FOO}}\nbody\n{{/if}}" + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + sync.apply_conditionals(text, {"FOO": "yes"}) + self.assertIn("no matching {{#if}}", buf.getvalue()) + def test_digit_bearing_key_is_a_directive(self): """_IF_OPEN's charset must stay in step with find_unresolved: a key one matches and the other doesn't is neither expanded nor reported, so the @@ -525,6 +543,11 @@ def test_finds_key_with_digits(self): placeholder slips through as a literal output directory. #221.""" self.assertEqual(sync.find_unresolved("{{PROMPT_PATH2}}"), ["PROMPT_PATH2"]) + def test_finds_key_with_leading_underscore(self): + """The original charset accepted a leading underscore; the digit + widening must not have narrowed it. #221.""" + self.assertEqual(sync.find_unresolved("{{_INTERNAL}}"), ["_INTERNAL"]) + def test_ignores_all_digit_token(self): """A documented regex backreference or Handlebars snippet like {{1}} is not a placeholder and must not fail --validate as a missing key. #221."""