Enforce Agent Skills name validation on every sync path, not only --validate

.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

docs/adapters.md

@@ -2,6 +2,8 @@
 
 Code Cannon skills follow the [Agent Skills](https://agentskills.io) open standard: each skill is a `<skill-name>/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 |

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.")