mirror of
https://github.com/Owl-Listener/designer-skills.git
synced 2026-09-19 03:07:41 +08:00
refactor: parse frontmatter with YAML in the linter, not by hand (#46)
The linter was the only reader of these files that was not a YAML parser. It split each line on the first ":", so `argument-hint: [what to pass]` reached it as the string "[what to pass]" and passed its bracket check, while every runtime saw a list. That is how #27 shipped four skills Copilot CLI silently rejected, and #45 could only work around it from outside. lint-frontmatter.py now parses with PyYAML, so it sees what the runtimes see: - _frontmatter_or_report says precisely why a block is unusable — absent, invalid YAML, or not a mapping — rather than collapsing all three - _string_field reports a value YAML coerced into a list, bool, number, null, or mapping, names the fix, and returns "" so downstream checks stay meaningful instead of raising a confusing second error - the argument-hint rule drops its optional-quote tolerance, since YAML strips quotes before the value arrives Verified by injecting each fault. The linter now catches all seven YAML-type faults, including the three it passed before: unquoted argument-hint (#27 exact), description: yes, and an unquoted colon in a value. All pre-existing house rules still fire. check-runtimes.py loses its parser-divergence check, which compared against a reproduction of the hand parser that no longer exists and so tested a fiction. What remains there is whether each runtime can load what we ship, including the Gemini extension manifests.
This commit is contained in:
+2
-2
@@ -87,9 +87,9 @@ bash scripts/build-gemini.sh
|
||||
python3 scripts/check-marketplace.py
|
||||
```
|
||||
|
||||
`lint-frontmatter.py` reports frontmatter errors with file and line references.
|
||||
`lint-frontmatter.py` reports frontmatter errors with file and line references. It parses with a real YAML parser, so it sees your frontmatter the way a runtime does: an unquoted `argument-hint: [like this]` is a *list* in YAML, not a string, which is what silently broke four skills on Copilot CLI in #27. **Quote any value containing brackets, a colon, or a bare `yes`/`no`.** Needs PyYAML (`pip install pyyaml`).
|
||||
|
||||
`check-runtimes.py` parses the same frontmatter with a real YAML parser and fails when a file would mean something different to a runtime than it does to the linter — an unquoted `argument-hint: [like this]` is a *list* in YAML, which is what silently broke four skills on Copilot CLI in #27. Quote any value containing brackets, a colon, or a bare `yes`/`no`. Needs PyYAML (`pip install pyyaml`).
|
||||
`check-runtimes.py` asks the separate question of whether each runtime can actually load what we ship — the frontmatter contract plus each plugin's Gemini extension manifest and context file.
|
||||
|
||||
`check-marketplace.py` confirms every plugin in `marketplace.json` still resolves — local sources to a real directory, remote ones to a clonable URL. Add `--network` to also check the remote repos are reachable.
|
||||
|
||||
|
||||
+18
-42
@@ -14,24 +14,27 @@ Copilot CLI >= 1.0.65 validates the field as a string, rejected the skill, and
|
||||
the command silently vanished from its menu. Four skills shipped that way until
|
||||
an outside contributor debugged it (#28).
|
||||
|
||||
scripts/lint-frontmatter.py cannot catch this. It parses frontmatter by hand,
|
||||
splitting each line on the first ":", so the broken form reaches it as the
|
||||
string "[product or feature to research]" and passes its bracket check. Every
|
||||
real runtime sees a list. That gap is the bug.
|
||||
scripts/lint-frontmatter.py once parsed frontmatter by hand, splitting each
|
||||
line on the first ":", so the broken form reached it as the string
|
||||
"[product or feature to research]" and passed its bracket check while every
|
||||
real runtime saw a list. That gap is why this script was written, and it is
|
||||
now closed at the source: the linter parses with PyYAML too, so the two can no
|
||||
longer disagree about what a file says.
|
||||
|
||||
So this script asks a different question from the linter's "does this follow
|
||||
our house rules?" — it asks "does this file mean the same thing to a strict
|
||||
parser as it does to us?" It checks:
|
||||
What remains here is the question the linter does not ask — not "is this file
|
||||
correct?" but "will each runtime actually load it?" It checks:
|
||||
|
||||
- the frontmatter block exists and parses as a YAML mapping
|
||||
- every field value is a plain string, not a value YAML silently coerced
|
||||
into a list, bool, number, null, or nested mapping
|
||||
- the repo's own hand-rolled parse agrees with a real YAML parse; where they
|
||||
disagree, the linter is validating something the runtime will never see
|
||||
- required fields are present for each file kind
|
||||
- every local plugin ships a valid Gemini extension manifest and a non-empty
|
||||
context file
|
||||
|
||||
The first three overlap with the linter by design. They are cheap, and they
|
||||
state the runtime contract in one place rather than leaving it implicit in
|
||||
another script's house rules.
|
||||
|
||||
Run it:
|
||||
|
||||
python3 scripts/check-runtimes.py
|
||||
@@ -100,16 +103,6 @@ def _frontmatter_block(path: Path) -> str | None:
|
||||
return m.group(1) if m else None
|
||||
|
||||
|
||||
def _hand_parse(block: str) -> dict[str, str]:
|
||||
"""Reproduce scripts/lint-frontmatter.py's parser, to compare against YAML."""
|
||||
fields: dict[str, str] = {}
|
||||
for raw_line in block.splitlines():
|
||||
if ":" in raw_line:
|
||||
key, _, value = raw_line.partition(":")
|
||||
fields[key.strip()] = value.strip()
|
||||
return fields
|
||||
|
||||
|
||||
def _line_of_key(path: Path, key: str) -> int | None:
|
||||
for i, line in enumerate(path.read_text(encoding="utf-8").splitlines(), start=1):
|
||||
if re.match(rf"^{re.escape(key)}\s*:", line):
|
||||
@@ -136,30 +129,14 @@ def check_file(path: Path, kind: str, explain: bool) -> None:
|
||||
f"{type(parsed).__name__}", 1)
|
||||
return
|
||||
|
||||
hand = _hand_parse(block)
|
||||
|
||||
for key, value in parsed.items():
|
||||
line = _line_of_key(path, str(key))
|
||||
|
||||
# 1. The #27 class: YAML coerced the value into something else.
|
||||
# The #27 class: YAML coerced the value into something else.
|
||||
if not isinstance(value, str):
|
||||
reason = _COERCION.get(type(value), f"a {type(value).__name__}")
|
||||
_report(path, f"`{key}` is not a string — YAML reads it as {reason}", line)
|
||||
continue
|
||||
_report(path, f"`{key}` is not a string — YAML reads it as {reason}",
|
||||
_line_of_key(path, str(key)))
|
||||
|
||||
# 2. The linter and a real parser disagree about what this value is.
|
||||
# When they do, the linter is checking something no runtime sees.
|
||||
hand_value = hand.get(str(key), "")
|
||||
if hand_value.strip('"').strip("'") != value:
|
||||
_report(
|
||||
path,
|
||||
f"`{key}` parses differently for the linter and for a runtime — "
|
||||
f"lint-frontmatter.py sees {hand_value!r} but a YAML parser sees "
|
||||
f"{value!r}; quote the value so both agree",
|
||||
line,
|
||||
)
|
||||
|
||||
# 3. Required fields, checked against the real parse rather than the hand one.
|
||||
# Required fields.
|
||||
for field in REQUIRED[kind]:
|
||||
if field not in parsed:
|
||||
_report(path, f"required field `{field}` is missing", 1)
|
||||
@@ -233,9 +210,8 @@ def main() -> None:
|
||||
sys.exit(1)
|
||||
|
||||
print(
|
||||
f"OK — {len(skills)} skills and {len(commands)} commands parse "
|
||||
"identically for a strict YAML runtime and for this repo's linter; "
|
||||
"all Gemini extensions are loadable.",
|
||||
f"OK — {len(skills)} skills and {len(commands)} commands load cleanly "
|
||||
"under a strict YAML runtime; all Gemini extensions are loadable.",
|
||||
flush=True,
|
||||
)
|
||||
|
||||
|
||||
+91
-19
@@ -1,6 +1,13 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Lint SKILL.md and command frontmatter per CONTRIBUTING.md rules.
|
||||
|
||||
Frontmatter is parsed with PyYAML, exactly as every runtime that reads these
|
||||
files does. This used to be a hand-rolled split on the first ":", which could
|
||||
not see YAML types: `argument-hint: [what to pass]` is a *list*, and reaching
|
||||
this linter as the string "[what to pass]" is how issue #27 shipped four
|
||||
skills that Copilot CLI silently rejected. A linter that parses differently
|
||||
from its consumers cannot see that class of bug, so it no longer does.
|
||||
|
||||
Checks applied to every */skills/*/SKILL.md:
|
||||
- frontmatter block present (opening and closing ---)
|
||||
- `name` field present and non-empty
|
||||
@@ -24,6 +31,17 @@ import re
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
try:
|
||||
import yaml
|
||||
except ImportError: # pragma: no cover - environment problem, not a repo problem
|
||||
print(
|
||||
"ERROR PyYAML is required (pip install pyyaml). Frontmatter must be "
|
||||
"parsed the way the runtimes parse it; a hand-rolled parser is what "
|
||||
"let issue #27 through.",
|
||||
flush=True,
|
||||
)
|
||||
sys.exit(2)
|
||||
|
||||
ROOT = Path(__file__).resolve().parent.parent
|
||||
_errors: list[tuple[str, int | None, str]] = []
|
||||
IN_CI = "GITHUB_ACTIONS" in os.environ
|
||||
@@ -40,18 +58,74 @@ def _report(path: Path, msg: str, line: int | None = None) -> None:
|
||||
print(f"ERROR {loc}: {msg}", flush=True)
|
||||
|
||||
|
||||
def _parse_frontmatter(path: Path) -> dict[str, str] | None:
|
||||
"""Parse the leading YAML frontmatter block; return field dict or None."""
|
||||
# What a non-string frontmatter value means, in terms an author can act on.
|
||||
_COERCION = {
|
||||
list: "a list — YAML reads bare [brackets] as a sequence; wrap the value "
|
||||
"in double quotes (this is issue #27)",
|
||||
bool: "a boolean — YAML reads bare yes/no/true/false as bools; quote it",
|
||||
int: "a number — quote it to keep it a string",
|
||||
float: "a number — quote it to keep it a string",
|
||||
type(None): "null — the value is empty, ~, or a bare null; give it a value",
|
||||
dict: "a nested mapping — an unquoted ': ' inside the value splits it; "
|
||||
"quote the whole value",
|
||||
}
|
||||
|
||||
|
||||
def _parse_frontmatter(path: Path) -> dict | None:
|
||||
"""Parse the leading frontmatter block with a real YAML parser.
|
||||
|
||||
Returns the field mapping, or None when there is no block, the YAML is
|
||||
invalid, or it is not a mapping. Callers that need the reason reported
|
||||
should use _frontmatter_or_report instead.
|
||||
"""
|
||||
text = path.read_text(encoding="utf-8")
|
||||
m = re.match(r"^---\n(.*?)\n---", text, re.DOTALL)
|
||||
if not m:
|
||||
return None
|
||||
fields: dict[str, str] = {}
|
||||
for raw_line in m.group(1).splitlines():
|
||||
if ":" in raw_line:
|
||||
key, _, value = raw_line.partition(":")
|
||||
fields[key.strip()] = value.strip()
|
||||
return fields
|
||||
try:
|
||||
data = yaml.safe_load(m.group(1))
|
||||
except yaml.YAMLError:
|
||||
return None
|
||||
return data if isinstance(data, dict) else None
|
||||
|
||||
|
||||
def _frontmatter_or_report(path: Path) -> dict | None:
|
||||
"""Parse frontmatter, reporting precisely why it is unusable."""
|
||||
text = path.read_text(encoding="utf-8")
|
||||
m = re.match(r"^---\n(.*?)\n---", text, re.DOTALL)
|
||||
if not m:
|
||||
_report(path, "no frontmatter block found (file must start with ---)", line=1)
|
||||
return None
|
||||
try:
|
||||
data = yaml.safe_load(m.group(1))
|
||||
except yaml.YAMLError as exc:
|
||||
detail = str(exc).replace("\n", " ")
|
||||
_report(path, f"frontmatter is not valid YAML, so no runtime can load "
|
||||
f"this file — {detail}", line=1)
|
||||
return None
|
||||
if not isinstance(data, dict):
|
||||
_report(path, f"frontmatter must be a mapping of fields, got "
|
||||
f"{type(data).__name__}", line=1)
|
||||
return None
|
||||
return data
|
||||
|
||||
|
||||
def _string_field(path: Path, fm: dict, key: str) -> str:
|
||||
"""Return a field's value, reporting it when YAML gave us a non-string.
|
||||
|
||||
Every field in this frontmatter is a string by contract. Returning "" for
|
||||
a coerced value keeps the downstream checks meaningful instead of raising
|
||||
a confusing second error about the wrong type.
|
||||
"""
|
||||
if key not in fm:
|
||||
return ""
|
||||
value = fm[key]
|
||||
if isinstance(value, str):
|
||||
return value
|
||||
reason = _COERCION.get(type(value), f"a {type(value).__name__}")
|
||||
_report(path, f"`{key}` must be a string but YAML reads it as {reason}",
|
||||
line=_line_of_key(path, key))
|
||||
return ""
|
||||
|
||||
|
||||
def _body_after_frontmatter(path: Path) -> str:
|
||||
@@ -73,13 +147,12 @@ def lint_skills() -> None:
|
||||
for skill_md in sorted(ROOT.glob("*/skills/*/SKILL.md")):
|
||||
skill_dir = skill_md.parent.name
|
||||
|
||||
fm = _parse_frontmatter(skill_md)
|
||||
fm = _frontmatter_or_report(skill_md)
|
||||
if fm is None:
|
||||
_report(skill_md, "no frontmatter block found (file must start with ---)", line=1)
|
||||
continue
|
||||
|
||||
name = fm.get("name", "")
|
||||
desc = fm.get("description", "")
|
||||
name = _string_field(skill_md, fm, "name")
|
||||
desc = _string_field(skill_md, fm, "description")
|
||||
|
||||
if not name:
|
||||
_report(skill_md, "required field `name` is missing or empty",
|
||||
@@ -111,13 +184,12 @@ def lint_skills() -> None:
|
||||
|
||||
def lint_commands() -> None:
|
||||
for cmd_md in sorted(ROOT.glob("*/commands/*.md")):
|
||||
fm = _parse_frontmatter(cmd_md)
|
||||
fm = _frontmatter_or_report(cmd_md)
|
||||
if fm is None:
|
||||
_report(cmd_md, "no frontmatter block found (file must start with ---)", line=1)
|
||||
continue
|
||||
|
||||
desc = fm.get("description", "")
|
||||
arg_hint = fm.get("argument-hint", "")
|
||||
desc = _string_field(cmd_md, fm, "description")
|
||||
arg_hint = _string_field(cmd_md, fm, "argument-hint")
|
||||
|
||||
if not desc:
|
||||
_report(cmd_md, "required field `description` is missing or empty",
|
||||
@@ -126,7 +198,7 @@ def lint_commands() -> None:
|
||||
_report(cmd_md, "required field `argument-hint` is missing or empty",
|
||||
line=_line_of_key(cmd_md, "argument-hint") or 3)
|
||||
|
||||
if arg_hint and not re.match(r'"?\[.+\]"?$', arg_hint):
|
||||
if arg_hint and not re.fullmatch(r"\[.+\]", arg_hint):
|
||||
_report(cmd_md,
|
||||
f'`argument-hint` must be a bracketed placeholder '
|
||||
f'like "[what to pass]" — got: {arg_hint!r}',
|
||||
@@ -152,8 +224,8 @@ def lint_descriptions() -> None:
|
||||
if fm is None:
|
||||
continue
|
||||
desc = fm.get("description", "")
|
||||
if not desc:
|
||||
continue
|
||||
if not isinstance(desc, str) or not desc:
|
||||
continue # missing, empty, or coerced — already reported by lint_skills
|
||||
line = _line_of_key(skill_md, "description")
|
||||
|
||||
if len(desc) > MAX_DESC:
|
||||
|
||||
Reference in New Issue
Block a user