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

[sebastientaggart]

What changed

validate_skill_names() previously ran only inside the --validate branch of main(). The write path and --dry-run never called it, so a skill whose frontmatter name no longer matched its directory (or that violated the Agent Skills spec, or was missing a description) would sync silently — sync_skill writes output under fm['name'], and CI's ./sync.py --dry-run passed. Mismatched output could ship to every consumer repo until someone manually ran --validate.

The check is now a hard gate in main(), placed right after the skill list resolves and before the --validate branch, adapter resolution, or any write. Every path — write, --dry-run, --validate — fails with exit 1 and a spec-compliance message before producing output.

• sync.py — added the shared gate; removed the now-duplicated name-validation block from the --validate branch, which keeps only its success line.
• .github/workflows/sync-check.yml — added ./sync.py --validate ahead of the existing --dry-run, so CI explicitly covers the placeholder, permission, and command-shape checks too rather than relying on the implicit gate for names alone.
• docs/adapters.md — documents that spec compliance is enforced on every sync path.
• tests/test_sync.py — three new TestMainCLI cases plus a _fake_checkout helper that builds a throwaway CodeCannon checkout (symlinking the real adapters/, config.schema.yaml, and permissions.yaml) around a single synthetic skill.

Behaviour note: the gate is a hard failure, not a per-skill skip. Sync stops entirely rather than silently omitting the offending skill, which is the intended reading of "run it as part of the normal sync path".

Test plan

• make test — 176 tests, all passing.
• make check — all four validations pass.
• The two mismatch tests were verified to be discriminating: with validate_skill_names stubbed to return [] (simulating pre-fix behaviour), both fail; the compliant-skill test passes either way, as the no-regression guard.
• The --dry-run test asserts on the failure message and the absence of "would write" rather than on the exit code alone, since --dry-run already exits 1 on pending writes and would otherwise pass without the fix.

Closes #221

[sebastientaggart]

Low severity — --validate loses its aggregate reporting.

The --validate block below deliberately accumulates failures (failed = True for placeholder, permission, and command-shape checks) so one run surfaces everything wrong. Hoisting the name check above it with a hard sys.exit(1) breaks that for --validate specifically: a single name error now short-circuits before validate_placeholders, validate_permissions, and validate_command_shapes ever run.

Scenario: a contributor renames a skill directory but forgets the frontmatter name, and the same skill also references an undefined {{PLACEHOLDER}}. make check reports only the name error; they fix it, re-run, and only then discover the placeholder error — two round trips where the pre-existing behavior reported both at once.

The write/--dry-run hard stop is correct and should stay; only the --validate path needs to fall through:

Suggested change

	sys.exit(1)
	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)
	sys.exit(1)

(then in the --validate block, report name_errors and set failed = True instead of the unconditional "passed" print at line 838, which currently prints "passed" only because a failure could never reach it).

[sebastientaggart]

Code Cannon review

Scope: sync.py name-validation hoist, .github/workflows/sync-check.yml, docs/adapters.md, tests/test_sync.py. Verified by reading main(), validate_skill_names, and sync_skill, and by running ./sync.py --validate and the test suite.

The core change is sound. The gate sits after skill-group and --skill resolution and before every write, --dry-run, and --validate path, and the new tests genuinely cover the write, dry-run, and happy paths.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:833 — The hard sys.exit(1) short-circuits the deliberate accumulate-then-exit design of --validate: a name error now suppresses the placeholder, permission, and command-shape results. A skill with both a frontmatter/directory name mismatch and an undefined {{PLACEHOLDER}} would need two make check round trips instead of one. Suggested fix: gate on if name_errors and not args.validate: so the write and dry-run stop stays hard while --validate falls through and sets failed = True. Related: the "passed" print at sync.py:838 is currently unconditional — it reads correctly only because failure can never reach it.

[NOTE] .github/workflows/sync-check.yml:13 — The new comment describes the --validate call on line 12 but sits below it, directly above --dry-run, so it reads as annotating the wrong command.

Verdict: APPROVE

[sebastientaggart]

Medium — output_path_override entries are now hard-blocked by a gate they are documented to be exempt from.

sync_skill's own docstring (sync.py:446-448) says a skill declaring output_path_override "is not a skill in the spec sense — it renders as a bare file with no frontmatter": neither name nor description reaches the output, and the output path comes from the override, not from <name>/SKILL.md. But validate_skill_names has no exemption for those entries, and this PR promotes it from a --validate report into a gate that aborts the whole run.

Scenario: add a prompt-only entry alongside review-agent — say skills/github-agile/pr-summary-prompt/SKILL.md with output_path_override: .github/prompts/summary.md and name: pr-summary (or no description, since neither is emitted). ./sync.py now exits 1 before writing anything, so every skill stops syncing over a field that is never used for that entry's output. review-agent only passes today by coincidence — it happens to carry a matching name and a description.

Suggest skipping entries with output_path_override in validate_skill_names (or asserting only the fields that actually render for them).

[sebastientaggart]

Low — the gate is scoped to the enabled group, so CI still cannot catch a bad name in a non-enabled skill group.

validate_skill_names(skill_files) runs over the post---skill, post-skill_group list. A few lines below, the permission check deliberately widens for exactly this reason:

When sync.py runs from inside the CodeCannon repo itself ... validate permissions across every skill group so a gap in a non-enabled group can't ship unnoticed.

The name check gets no equivalent widening. Scenario: a second group is added under skills/ (say skills/foo/) containing a skill whose frontmatter name doesn't match its directory. CI runs ./sync.py --validate + --dry-run from the CodeCannon repo with skill_group: github-agile, so skill_files never includes it — the violation ships green, and only breaks later in a consumer repo that enables that group. docs/adapters.md in this PR states enforcement happens "on every sync path", which reads stronger than what is implemented.

Mirroring the perm_skill_files widening (skills_dir.glob('*/*/SKILL.md') when CODECANNON_DIR == project_root) for the name check under --validate would close this.

[sebastientaggart]

Low — the failure header and error-printing loop are now duplicated verbatim two blocks apart.

The same four lines (header string + for e in name_errors: print(e)) appear at sync.py:834-837 and again here. They will drift: a future edit to the wording, the doc link, or the formatting in one copy leaves the other stale, and the two are reachable on mutually exclusive paths so no test would catch the divergence — test_name_directory_mismatch_blocks_dry_run asserts on the first copy, test_validate_reports_all_checks_despite_name_error on the second.

Lifting it into a small local (e.g. def _report_name_errors(): ...) or restructuring so one block handles both paths keeps them in sync.

Also cosmetic: the --validate name block prints with no leading \n while the permission and command-shape blocks below still use "\nPermission validation...", so the name section runs flush against the placeholder section while the later ones are spaced.

[sebastientaggart]

Code Cannon review — round 2 (677c9ef)

Scope vs dev: sync.py, tests/test_sync.py, .github/workflows/sync-check.yml, docs/adapters.md. Full suite passes and ./sync.py --validate exits 0. The --validate deferral from round 1 is confirmed working — all four check classes report together.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:832 — output_path_override entries are now hard-blocked by a gate they are documented to be exempt from. sync_skill's docstring states such an entry "is not a skill in the spec sense — it renders as a bare file with no frontmatter", so neither name nor description reaches its output, yet validate_skill_names has no exemption and now aborts the entire run. Adding a prompt-only entry whose directory name differs from its name, or that omits description, would stop every skill from syncing. Confirmed: review-agent is the sole such entry today and passes only by coincidence.

[WARNING] sync.py:833 — The gate runs over the skill_group-filtered list, while the permission check below deliberately widens to all groups so "a gap in a non-enabled group can't ship unnoticed". A name violation in a future second group would ship green through CI and surface only in a consumer repo, despite the new docs/adapters.md line claiming enforcement on every sync path.

[WARNING] sync.py:845 — The failure header and error loop are duplicated verbatim across the two mutually exclusive paths, so wording drift between them is invisible to tests. The --validate name block also lacks the leading newline the permission and command-shape blocks use, making its spacing inconsistent.

Verdict: APPROVE

[sebastientaggart]

Low — The exemption keys off the raw frontmatter value, but sync_skill decides the same question after placeholder substitution (output_path_override = apply_placeholders(...) then if output_path_override:). The two disagree when the override is a placeholder that resolves to empty.

Concrete: review-agent/SKILL.md declares output_path_override: "{{REVIEW_AGENT_PROMPT}}". A consumer that explicitly sets REVIEW_AGENT_PROMPT: "" in .codecannon.yaml (to disable the prompt file) makes the substituted value falsy, so sync_skill falls through to the skill-directory branch and writes <output_dir>/<frontmatter name>/SKILL.md with built frontmatter — for an entry the gate deliberately skipped. With the exact frontmatter the new test uses (name: Some_Other_Name), that writes .claude/skills/Some_Other_Name/SKILL.md: precisely the spec-violating skill directory this gate exists to prevent. Today it is harmless only because review-agent's name happens to match its directory.

Cheapest fix: exempt only entries whose override is still truthy after substitution, or validate name/description whenever sync_skill would take the frontmatter branch.

[sebastientaggart]

Low — This equality compares an unresolved path against a resolved one: CODECANNON_DIR = Path(__file__).parent (sync.py:27) is only abspath-normalized, while project_root = Path.cwd() comes from os.getcwd(), which returns the fully symlink-resolved path. Any symlink in the checkout path makes the comparison false.

Concrete: a checkout reachable as /Users/me/dev -> /Volumes/Code/dev (or CI that checks out under a symlinked workspace). Running ./sync.py --validate from the repo root takes the else branch, so audit_skill_files silently narrows to the enabled group only — no error, exit 0 — and the guarantee this PR adds to docs/adapters.md ("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") quietly does not hold. The same silent narrowing applies to the permission and command-shape checks that now share audit_skill_files.

The new tests already work around exactly this — _fake_checkout has to call .resolve() with a comment explaining that os.chdir() yields a resolved cwd. The fix is one word at line 27: Path(__file__).resolve().parent.

(Pre-existing, but this PR moves the branch and now leans a documented CI guarantee on it.)

[sebastientaggart]

Code Cannon review — round 3 (a0fbc99)

Scope: git diff dev...HEAD across three commits. The round-2 fixes are confirmed — the output_path_override exemption, the widened audit scope, and the report_errors extraction all behave as intended. 180 tests pass and ./sync.py --validate is green in-repo, so the new CI step won't fail on existing content.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:551 — The output_path_override exemption keys off the raw frontmatter value, while sync_skill makes the same branch decision after placeholder substitution. A consumer setting REVIEW_AGENT_PROMPT: "" makes the substituted override falsy, so sync_skill takes the frontmatter branch and writes <output_dir>/<frontmatter name>/SKILL.md for an entry the gate skipped — emitting exactly the spec-violating directory the gate exists to prevent. Harmless today only because review-agent's name happens to match its directory.

[WARNING] sync.py:27 — CODECANNON_DIR = Path(__file__).parent is unresolved, but project_root = Path.cwd() is symlink-resolved. Any symlink in the checkout path makes CODECANNON_DIR == project_root false, silently narrowing audit_skill_files to the enabled group with no error and exit 0 — voiding the group-wide guarantee this PR documents in docs/adapters.md, and now also affecting the permission and command-shape checks that share the variable. The new _fake_checkout test helper already works around this with an explicit .resolve(), which is the tell. Fix: Path(__file__).resolve().parent.

Verdict: APPROVE

[sebastientaggart]

Low — an unresolved placeholder override silently keeps the exemption, and still writes a junk path.

The exemption tests truthiness of the substituted override. apply_placeholders leaves an undefined token intact, so an override like {{FOO}} where FOO is in neither .codecannon.yaml nor config.schema.yaml stays "{{FOO}}" — truthy — and the entry is exempted. sync_skill then takes the same branch and writes to project_root / "{{FOO}}", literally creating a {{FOO}} directory in the consumer repo. That is exactly the class of "output under a directory that shouldn't exist" this gate was added to prevent, and the new resolution logic is the natural place to catch it: treat an override still containing {{...}} after substitution as a failure (or at minimum surface it the way sync_skill's unresolved-placeholder warning does). Today it's shielded by the schema default for REVIEW_AGENT_PROMPT, so the risk is only for a future prompt-only entry whose placeholder has no default.

[sebastientaggart]

Low — error messages are ambiguous now that the audit spans every group.

dir_name is skill_path.parent.name, so every error reads <dir>/SKILL.md: .... That was unambiguous when the check only ever saw one group's skills. With the new in-repo widening to skills/*/*/SKILL.md, two groups can each contain a start/SKILL.md, and a violation in skills/othergroup/start/ prints start/SKILL.md: name 'x' must match its directory name — pointing the maintainer at the wrong file, in a group they may not even have enabled. Include the group (e.g. skill_path.relative_to(skills_dir) or f"{skill_path.parent.parent.name}/{dir_name}"). validate_permissions has the same shape and would benefit from the same fix.

[sebastientaggart]

Low — the doc states the exemption unconditionally, but the code deliberately makes it conditional.

"Entries declaring output_path_override are exempt" is the rule a reader will act on, yet validate_skill_names exempts only entries whose substituted override is non-empty — the behavior test_override_resolving_to_empty_is_not_exempt pins. A consumer who sets REVIEW_AGENT_PROMPT: to an empty value reads this paragraph, expects review-agent to stay exempt, and instead gets a hard sync failure if its frontmatter isn't spec-compliant. Add the qualifier, e.g. "…are exempt, as long as the override resolves to a non-empty path; one whose placeholder resolves to empty renders as an ordinary skill and is held to the spec."

[sebastientaggart]

Code Cannon review — round 4 (c2340bd)

Scope: dev...HEAD across four files. Round-3 fixes confirmed: CODECANNON_DIR = Path(__file__).resolve().parent genuinely fixes the in-repo comparison against the always-resolved Path.cwd(), and the substituted-override exemption matches the branch sync_skill takes. Suite passes and ./sync.py --validate is green in-repo. No correctness issue of medium or higher severity.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:561 — An unresolved placeholder override keeps the exemption. apply_placeholders leaves an undefined {{FOO}} intact, so the override stays truthy, the entry is exempted, and sync_skill takes the same branch and writes to a literal project_root/{{FOO}} directory — the "output under a directory that shouldn't exist" the gate exists to prevent. Masked today by the REVIEW_AGENT_PROMPT schema default; it would bite a future prompt-only entry whose placeholder has no default.

[WARNING] sync.py:564 — Error messages use only skill_path.parent.name, which was unambiguous when the check saw a single group. The new in-repo widening to skills/*/*/SKILL.md means two groups can both contain start/SKILL.md, so a violation prints start/SKILL.md: ... and points at the wrong file. validate_permissions has the same shape and inherits the same ambiguity from sharing audit_skill_files.

[WARNING] docs/adapters.md:5 — States the output_path_override exemption unconditionally, contradicting the implemented and tested rule that an override resolving to empty is not exempt. A consumer setting REVIEW_AGENT_PROMPT: empty would be surprised by a hard sync failure.

Verdict: APPROVE

[sebastientaggart]

The in-repo audit widens validate_skill_names to every skill group, but resolves each group's output_path_override against the enabled group's project_config. validate_placeholders was deliberately scoped to skill_files (enabled group only) for exactly this reason — a non-enabled group's placeholders are not guaranteed to be defined in this project's config.

Scenario: add skills/other-group/foo/SKILL.md with output_path_override: "{{OTHER_PATH}}", where OTHER_PATH has no config.schema.yaml default and is not in .codecannon.yaml. Running plain ./sync.py inside the CodeCannon repo now exits 1 with output_path_override has undefined placeholder(s): OTHER_PATH — blocking all syncs over a group nobody enabled and whose override is never rendered. Name/description checks are fine to widen; the placeholder-resolution half is not.

One fix: only resolve/report the override placeholder for files in the enabled group, and fall back to the raw-value exemption for out-of-group files.

[sebastientaggart]

The new undefined-placeholder branch is only as complete as find_unresolved, whose regex is r"\{\{([A-Z_]+)\}\}" — no digits. So a placeholder name containing a digit escapes detection entirely.

Scenario: a skill declares output_path_override: "{{PROMPT_PATH2}}" and PROMPT_PATH2 is undefined. apply_placeholders leaves the token intact, find_unresolved returns [], the truthy raw string falls through to if override: continue, and the entry is treated as exempt. sync_skill then takes the same branch and writes to project_root / "{{PROMPT_PATH2}}" — a literal {{PROMPT_PATH2}} file in the consumer repo, which is exactly the failure this branch was added to prevent.

The regex predates this PR, but this is the first place its completeness is load-bearing for a correctness gate. Widening it to [A-Z0-9_]+ closes the hole.

[sebastientaggart]

Code Cannon review — round 5 (087c4d2)

Scope: dev...HEAD. Verified with the full suite (185 tests), ./sync.py --validate (clean), and ./sync.py --dry-run (no drift). The exemption branch was traced against sync_skill across all four cases — raw override, substituted override, override resolving to empty, undefined placeholder — and they agree. CODECANNON_DIR resolution is correct and no other use of the constant depends on the unresolved form. The deferred --validate exit and the immediate write-path exit are both wired correctly, with no writes possible before the gate. skill_label's parent.parent assumption holds at every call site.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:577 — The new undefined-placeholder detection leans on find_unresolved, whose regex \{\{([A-Z_]+)\}\} excludes digits. An override like {{PROMPT_PATH2}} with that key undefined yields no unresolved names, falls through to the exemption, and sync_skill writes to a literal {{PROMPT_PATH2}} path — precisely the failure this branch exists to prevent. The regex predates this PR but is newly load-bearing for a correctness gate; [A-Z0-9_]+ closes it.

[WARNING] sync.py:897 — The in-repo audit widens validate_skill_names to every skill group but resolves each group's output_path_override against the enabled group's project_config. validate_placeholders is deliberately scoped to skill_files for exactly this reason. A second group with an override placeholder undefined in the current config would hard-fail every sync path over a group nobody enabled. Not reachable today (only github-agile exists), but it is a latent trap introduced by the widened scope.

Both are hardening against future states rather than live defects.

Verdict: APPROVE

[sebastientaggart]

find_unresolved now accepts digits ([A-Z0-9_]+), but _IF_OPEN (sync.py:264) still matches [A-Z_]+. A key like PROMPT_PATH2 is therefore usable as a plain {{PROMPT_PATH2}} placeholder but not in a conditional: {{#if PROMPT_PATH2}} is not recognized as a directive, so apply_conditionals leaves the literal {{#if PROMPT_PATH2}} / {{/if}} lines in the body, and find_unresolved does not match them either (the #if prefix breaks the pattern) — so --validate stays green and the directive lines ship verbatim into the generated SKILL.md. If digit-bearing keys are now considered legal, widen _IF_OPEN to the same character class.

[sebastientaggart]

This PR moves validate_permissions and validate_command_shapes onto the new group-qualified skill_label(), but validate_placeholders (sync.py:616) was left on skill_path.name, which is always the literal string SKILL.md. Concretely: run ./sync.py --validate with an undefined placeholder and every error line reads SKILL.md: {{FOO}} not defined in config — with 11 skills in the group there is no way to tell which file to fix. Switching that f-string to skill_label(skill_path) makes it consistent with the rest of the report.

[sebastientaggart]

Non-enabled groups are validated with project_config=None, so in validate_skill_names the if override and project_config is not None branch is skipped and any entry with an output_path_override is exempted unconditionally — including one holding an undefined placeholder. Scenario: a second skill group ships output_path_override: "{{OTHER_PATH}}" with no schema default; in-repo CI (--validate + --dry-run) passes green, and the failure only appears when a consumer sets skill_group to that group, at which point sync_skill writes the file into a literal {{OTHER_PATH}} directory. This is the one violation class the widened audit does not cover, so the docs/adapters.md line "a violation in a group nobody has turned on still fails CI" overstates it. A cheap fix is to check the raw override for find_unresolved hits in the non-enabled pass too (undefined-in-any-config is a bug regardless of which group is enabled), or to soften the doc sentence.

[sebastientaggart]

Code Cannon review — round 6 (50991a5)

Scope: dev...HEAD. Verified with ./sync.py --validate (exit 0), ./sync.py --dry-run (no drift), and the full suite (187 tests, OK). The exemption in validate_skill_names matches the branch sync_skill takes, the enabled/non-enabled split does not misclassify paths, --validate defers its exit so all four reports print, and the Path(__file__).resolve() change genuinely fixes the in-repo comparison. No high-severity bugs.

Sensitive-area gate: not triggered — no authentication, payments, secrets handling, production configuration, or destructive operations in this diff.

Findings

[WARNING] sync.py:616 — The group-qualification sweep covered validate_skill_names, validate_permissions, and validate_command_shapes but missed validate_placeholders, which still labels errors skill_path.name — the literal string SKILL.md for every skill. An undefined-placeholder report therefore names no file at all.

[WARNING] sync.py:264 — find_unresolved widened to [A-Z0-9_]+ but _IF_OPEN still matches [A-Z_]+. A digit-bearing key works as a plain placeholder, yet {{#if FOO2}} is neither treated as a directive nor flagged as unresolved, so the literal directive lines ship into generated output with --validate green.

[WARNING] docs/adapters.md — Non-enabled groups are now validated with project_config=None (correct: their placeholders aren't resolvable against the enabled group's config), so an output_path_override holding an undefined placeholder is exempted rather than reported there. That makes the docs claim that any violation in a non-enabled group fails CI an overstatement — name and description violations are caught, override-placeholder ones are not.

Verdict: APPROVE

[sebastientaggart]

Low — Undefined output_path_override placeholders are reported under the wrong header.

validate_skill_names appends output_path_override has undefined placeholder(s): ... into the same name_errors list that is printed under NAME_FAILURE_HEADER ("frontmatter not spec-compliant (see agentskills.io)"). But that error is a config problem, not a spec problem — the fix is to define the key under config: in .codecannon.yaml, not to edit frontmatter.

Scenario: a consumer enables a skill group containing an entry with output_path_override: "{{PROMPT_PATH}}" and no PROMPT_PATH in their config (and no schema default). ./sync.py exits 1 with a header pointing them at agentskills.io and telling them their frontmatter violates the Agent Skills spec, while the actual remedy is a missing config key. That is the same class of error validate_placeholders already reports under "Placeholder validation failed — undefined placeholders", so the two now disagree on how to describe it.

Consider collecting override-placeholder errors into a separate list reported under the placeholder header (or its own), rather than folding them into the name gate.

[sebastientaggart]

Low — The write path is blocked by violations in groups it will never write.

When run in-repo, audit_skill_files spans every group under skills/, and name_errors from non-enabled groups reach this hard exit on the write path (plain ./sync.py, --force, --dry-run), not just under --validate. The justification in the comment above — "a frontmatter-name/directory mismatch makes sync_skill write output under a directory the spec says shouldn't exist" — only holds for the enabled group. A non-enabled group is never passed to sync_skill, so nothing bad can be written on its account.

Scenario: a Code Cannon developer starts skills/experimental/ with a WIP skill whose frontmatter name does not yet match its directory. ./sync.py --force — which the pre-commit hook requires before every commit — now exits 1 for the enabled github-agile group, blocking all commits until the unrelated WIP group is fixed, even though not one byte of it would have been written. Note the asymmetry: the permission and command-shape audits, which are also group-wide, only fail under --validate.

Since CI already runs ./sync.py --validate (which keeps the widened scope), scoping the write-path exit to enabled_group errors and leaving cross-group enforcement to --validate would preserve the CI guarantee without the local friction.

[sebastientaggart]

LOW — Widening find_unresolved to [A-Z0-9_]+ also makes it match all-digit tokens such as {{1}} or {{0}}, which the previous [A-Z_]+ charset could not match.

validate_placeholders feeds this result straight into missing = [p for p in find_unresolved(...) if p not in project_config], so any skill body that documents a numeric double-brace template — a regex backreference example like {{1}}, a Handlebars/Vue snippet, a positional-substitution illustration in a fenced block — is now reported as an undefined placeholder and hard-fails ./sync.py --validate, which CI runs. Nothing in the tree trips it today, so this is latent rather than broken, but it is a false-positive class the old charset was immune to.

The digit support that motivated the change is only needed for keys like PROMPT_PATH2, which always start with a letter.

Suggested change

	return re.findall(r'\{\{([A-Z0-9_]+)\}\}', text)
	return re.findall(r"\{\{([A-Z][A-Z0-9_]*)\}\}", text)

[sebastientaggart]

LOW — Two enforcement holes in the non-enabled-group audit that the success messages hide.

1. validate_skill_names(other_files) is called without project_config, so the exemption is decided on the raw override. A non-enabled group skill with name: Bad_Name and output_path_override: "{{X}}" is silently exempted from both the name and description checks. Whoever later enables that group with X set to "" gets the spec violation at their own sync — the very escape this PR set out to close, except it now passes CodeCannon's own CI green first.

2. validate_output_paths is only ever run on enabled_files, never on other_files, yet line 959 prints "Output-path validation passed — all override placeholders are defined." unconditionally. In-repo --validate therefore claims a pass over a set it did not check. An undefined override placeholder in a non-enabled group surfaces only when someone enables it.

Both follow from the deliberate config-scoping decision (config resolves against the enabled group only), so the fix is probably not to widen the checks but to stop claiming the wider guarantee — e.g. qualify the two pass messages with the scope actually audited when other_files is non-empty.

[sebastientaggart]

The new comment claims _IF_OPEN and find_unresolved staying in step is what keeps an unmatched directive from shipping literally, but find_unresolved can never report a directive line: its pattern is \{\{([A-Z][A-Z0-9_]*)\}\}, and {{#if FOO}} / {{/if}} contain #, / and a space, so they never match regardless of charset. There is no safety net here.

That matters because this hunk narrows the charset — [A-Z_]+ accepted a leading underscore, [A-Z][A-Z0-9_]* does not. Concrete failure: a skill containing {{#if _INTERNAL}}...{{/if}} (or any key _IF_OPEN misses). apply_conditionals finds the first {{/if}}, scans backwards for an open, finds none, and breaks out of the whole while changed loop — so every conditional block in that file is left unprocessed. The literal {{#if}}/{{/if}} lines ship into generated output, and the body of blocks that should have been stripped (e.g. {{#if !BRANCH_DEV}}) leaks into the synced skill. Exit code 0, no warning.

No current skill uses such a key, so nothing is broken today. But if the charsets are meant to be load-bearing, apply_conditionals should report a {{/if}} with no matching open as an error instead of silently breaking — otherwise the comment overstates the guarantee.

[sebastientaggart]

The mirror case is still silent. This PR adds a warning for a {{/if}} with no matching open, but the opposite malformation — a {{#if KEY}} with no {{/if}} at all — hits the close_idx is None: break at line 285 and returns the text unchanged with no warning and no error.

Verified:

>>> sync.apply_conditionals("{{#if FOO}}\nbody", {"FOO": "yes"})
"{{#if FOO}}\nbody"
>>> sync.find_unresolved("{{#if FOO}}")
[]

So a skill that loses its closing directive in an edit ships the literal {{#if FOO}} line straight into every generated SKILL.md, and (as the new comment on _IF_OPEN notes) find_unresolved cannot see it either, so --validate reports nothing. Given the PR's stated goal of not failing silently at exit 0, this branch deserves the same warning as line 297.

[sebastientaggart]

Two problems with this warning as written.

1. It cannot be traced to a skill. apply_conditionals has no access to skill_path, and it is called once per skill per adapter (plus twice more per skill during --validate, once for the body and once for the description). On a repo-wide sync that is one anonymous line among dozens of ✓ lines, with no way to tell which of the ~10 skills is malformed. Passing a label into apply_conditionals (or returning the condition and having sync_skill print it) would make it actionable.

2. It does not affect the exit code, so CI stays green. The scenario: someone drops a closing directive, ./sync.py --force regenerates every adapter output with the literal {{#if …}} lines in it, and both files get committed. In CI, --validate never sees it (find_unresolved cannot match a {{#if X}} line, per the new comment at line 264) and --dry-run compares the generated output against the equally-broken committed output, so it reports no drift. Exit 0 on both, broken directives shipped to consumer repos.

That is the same class of hole the rest of this PR closes for skill names. Collecting the condition and failing --validate on it would close it.

[sebastientaggart]

The docstring says this checks the override "resolves to a usable path", but it only checks for leftover placeholders. Two shapes still get through and both escape project_root:

• Absolute path. sync_skill does out_path = project_root / output_path_override, and Python discards the left operand when the right is absolute: Path("/proj") / "/etc/x" → /etc/x. A consumer who writes REVIEW_AGENT_PROMPT: /Users/me/.claude/prompt.md in .codecannon.yaml gets a file written outside their repo, silently, with a ✓ line.
• Parent traversal. Path("/proj") / "../../etc/x" → /proj/../../etc/x, which mkdir(parents=True) + write_text will happily create outside the checkout.

Since this validator now runs on the write path and hard-fails the sync, it is the natural place to reject both — e.g. flag Path(resolved).is_absolute() and any resolved path that is not under project_root. That would need project_root threaded in, but it turns a silent out-of-repo write into a named error.