diff --git a/.github/workflows/sync-check.yml b/.github/workflows/sync-check.yml index cdfe9a4..9005959 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 --dry-run + # placeholder, permission, and command-shape checks, reported together; + # skill names are gated on every path, including the --dry-run below + ./sync.py --validate # exits non-zero if any files would be written + ./sync.py --dry-run diff --git a/docs/adapters.md b/docs/adapters.md index 698a1cb..c713f0f 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. 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 | Adapter | Output | Read natively by | diff --git a/sync.py b/sync.py index aed1b06..66eb276 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", @@ -258,7 +261,12 @@ 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 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*$') @@ -282,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) == '!' @@ -315,8 +329,15 @@ 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. + + 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, 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) # ── Hash and change detection ───────────────────────────────────────────────── @@ -517,27 +538,95 @@ 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) + + +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]+)*$') -def validate_skill_names(skill_files): - """Check each SKILL.md's frontmatter against the Agent Skills spec.""" +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. + + 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: 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) + if find_unresolved(override): + 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 + + +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 @@ -554,7 +643,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 @@ -599,7 +688,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 @@ -650,7 +739,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 @@ -820,50 +909,93 @@ 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 + + # 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) + 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):") + 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 - errors = validate_placeholders(skill_files, project_config) - if errors: - print("Placeholder validation failed — undefined placeholders:\n") - for e in errors: - print(e) + name_errors += validate_skill_names(other_files) + if name_errors: + report_errors(NAME_FAILURE_HEADER, name_errors, leading_blank=False) failed = True 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) + # 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("Skill-name validation passed — frontmatter follows the Agent Skills spec.") + # 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.") - # 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')) + errors = validate_placeholders(skill_files, project_config) + if errors: + report_errors("Placeholder validation failed — undefined placeholders:", errors) + failed = True else: - perm_skill_files = all_skill_files - perm_errors = validate_permissions(perm_skill_files) + print("Placeholder validation passed — all placeholders are defined.") + + 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 35171fc..e726f34 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 @@ -319,6 +321,33 @@ 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 + 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": ""}) @@ -509,6 +538,21 @@ 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_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.""" + self.assertEqual(sync.find_unresolved("use {{1}} and {{0}} here"), []) + def test_ignores_lowercase(self): text = "{{lowercase}}" result = sync.find_unresolved(text) @@ -748,6 +792,55 @@ 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]), []) + + 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)) + + 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: Some_Other_Name\noutput_path_override: "{{PROMPT_PATH}}"', + "body") + 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 + 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) @@ -916,6 +1009,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): @@ -936,6 +1071,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) @@ -1207,6 +1349,228 @@ 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, 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. `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. + """ + # 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) + + 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) + 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", self.fake_root): + 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", 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: + 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_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", 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) + 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_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 _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, 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_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 + 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, "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)]): + with contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit) as ctx: + sync.main() + self.assertEqual(ctx.exception.code, 1) + 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 + 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.""" + 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", self.fake_root): + 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: