fix(kyberforge): harden Research doc and Basis parsing in the validator

Review of PR 139 found list-rejection and confinement holes that let the
exact malformed entries the grammar forbids pass check 7.

- Reject comma, space-separated and backticked path lists, so
  `a/sources.md (x), b/topic.md` no longer exits 0 unchecked.
- FAIL absolute paths and any path whose realpath leaves the repo, for
  both `Research doc:` and `Basis:`.
- Anchor `(removed in <sha>)` to the end of the value with a 7-40 hex
  sha. The sha is format-checked only, not resolved with git cat-file.
- Read `* ` bullets and `- **X**` bullets correctly under a `**Basis:**`
  header, and strip backticks from Basis paths.
- Stop the semicolon rule firing on annotation prose, and stop `none`
  matching `none/foo.md`.
- Update the stale field messages to the new grammar and report an empty
  field as empty, not missing.
- Skip a removed Basis silently when there is no repo root.

Adds 40 tests. Each guarded line was mutated in place and every mutant
is caught.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGHFJextYtVQseaHPDDhxB
This commit is contained in:
2026-09-21 19:40:28 +00:00
parent 2bde9a6a82
commit 2c4b6d2615
2 changed files with 359 additions and 19 deletions

View File

@@ -380,14 +380,18 @@ def parse_field_values(content, slug, label):
found" for the other two, and every caller read that as "nothing declared"
(#121, second comment; the same failure shape as #111 and #118). A header's
bullets stop at the first line that is neither blank nor a bullet, and a
'- **Other:**' bullet is the NEXT field, not a value of this one.
'- **Other:**' bullet is the NEXT field, not a value of this one ('* '
bullets count too, and a bold bullet with no colon is a value).
"""
block = _entry_block(content, slug)
if block is None:
return []
values = []
lines = block.splitlines()
label_re = re.compile(r'^(?:- )?\*\*' + re.escape(label) + r':\*\*[ \t]*(.*)$')
label_re = re.compile(r'^(?:[-*] )?\*\*' + re.escape(label) + r':\*\*[ \t]*(.*)$')
# A bullet that opens with a bold '**Other:**' label is the NEXT field. A
# bold bullet WITHOUT the colon ('- **docs/x.md**') is just a value.
next_field_re = re.compile(r'^[-*] \*\*[^*]*:\*\*')
i = 0
while i < len(lines):
m = label_re.match(lines[i])
@@ -398,15 +402,21 @@ def parse_field_values(content, slug, label):
if inline:
values.append(inline)
continue
found = False
while i < len(lines):
line = lines[i].strip()
if not line:
i += 1
continue
if not line.startswith('- ') or line.startswith('- **'):
if not (line.startswith('- ') or line.startswith('* ')) or next_field_re.match(line):
break
values.append(line[2:].strip())
found = True
i += 1
if not found:
# The field is DECLARED but carries nothing: report an empty value,
# not an absent field, so callers say 'empty' rather than 'missing'.
values.append('')
return values
def parse_research_docs(content, slug):
@@ -441,8 +451,10 @@ def parse_basis(content, slug):
RESEARCH_DOC_ANNOTATION_RE = re.compile(r'[§→(]')
def strip_research_doc_annotation(value):
"""Path part of a Research doc value, with any section annotation removed."""
return RESEARCH_DOC_ANNOTATION_RE.split(value, maxsplit=1)[0].strip()
"""Path part of a Research doc value, with any section annotation removed
and surrounding backticks unwrapped ('`a/b.md`' resolves as 'a/b.md')."""
head = RESEARCH_DOC_ANNOTATION_RE.split(value, maxsplit=1)[0].strip()
return head.strip('`').strip()
def research_doc_is_none(value):
"""True when a Research doc value declares that no research doc backs the slug.
@@ -452,7 +464,9 @@ def research_doc_is_none(value):
unresolvable path. Checked BEFORE the annotation strip, because '(none)'
is itself a parenthesis and would strip to the empty string.
"""
return re.match(r'\(?none\b', value.strip(), re.IGNORECASE) is not None
# 'none/foo.md' and 'none-of-these.md' are PATHS: after 'none' only the end,
# whitespace or an em/en dash may follow (or the parenthesised '(none)').
return re.match(r'(?:\(none\)|none(?=$|\s|[\u2014\u2013]))', value.strip(), re.IGNORECASE) is not None
# A Research doc or Basis value names ONE path. The three list spellings seen
# in the corpus — a brace expansion, a comma-separated list and a
@@ -463,15 +477,48 @@ def research_doc_is_none(value):
# said so. Detected on the raw value, with commas and semicolons INSIDE the
# annotation left alone: those are prose ('cross-cutting; no dedicated
# section'), and only a second path-shaped token after a ';' is a list.
SECOND_PATH_AFTER_SEMICOLON_RE = re.compile(r';\s*[\w.\-]+/[\w./\-]*\.[A-Za-z]+')
SECOND_PATH_AFTER_SEMICOLON_RE = re.compile(r'[;,]\s*[\w.\-]+/[\w./\-]*\.[A-Za-z]+')
BASIS_REMOVED_RE = re.compile(r'\(removed in [0-9a-fA-F]{7,40}\b[^)]*\)')
# Only the LAST character class matters for the removal annotation: it must end
# the value, so '(removed in <sha>) but still here' is not the annotation.
BASIS_REMOVED_RE = re.compile(r'\(removed in [0-9a-f]{7,40}\)\s*$')
PAREN_GROUP_RE = re.compile(r'\([^()]*\)')
def names_more_than_one_path(value):
path_part = strip_research_doc_annotation(value)
if '{' in path_part or '}' in path_part or ',' in path_part or ';' in path_part:
"""True when a Research doc / Basis value is a list rather than one path.
Three places to look, none of which is prose:
- the leading path token: whitespace inside it ('a.md b.md'), or any of
, ; { } or a stray backtick, is a list;
- the text after it, once balanced '(...)' annotations are removed (a
comma or semicolon INSIDE parentheses is prose): a bare , ; { } there
is a second path parked after the first ('a.md (x), b.md');
- after a section marker (§, →) prose may hold commas, so only a
second path-SHAPED token after ',' or ';' counts.
"""
head = strip_research_doc_annotation(value)
if re.search(r'[\s,;{}`]', head):
return True
return SECOND_PATH_AFTER_SEMICOLON_RE.search(value) is not None
rest = value[len(RESEARCH_DOC_ANNOTATION_RE.split(value, maxsplit=1)[0]):]
while True:
stripped = PAREN_GROUP_RE.sub('', rest)
if stripped == rest:
break
rest = stripped
if rest.lstrip().startswith(('§', '→')):
return SECOND_PATH_AFTER_SEMICOLON_RE.search(rest) is not None
return re.search(r'[,;{}]', rest) is not None
def path_escapes_repo(repo_root, rel_path):
"""True when rel_path is absolute or resolves (symlinks followed) outside
repo_root. Research doc and Basis are repo-relative, so anything else is
either a mistake or a way to make the checker read a file elsewhere."""
if os.path.isabs(rel_path):
return True
root = os.path.realpath(repo_root)
real = os.path.realpath(os.path.join(root, rel_path))
return not (real == root or real.startswith(root + os.sep))
def find_repo_root(start_dir):
"""Walk up from start_dir until we find a directory containing .git."""
@@ -904,14 +951,16 @@ for slug in unique_slugs:
f"Research doc field missing",
f"references/sources.md (## {slug})",
f"The '## {slug}' entry in sources.md has no '- **Research doc:**' line.",
f"Add '- **Research doc:** <path-or-(none)>' to the '## {slug}' entry in references/sources.md."
f"Add '- **Research doc:** <path to the plugin's research sources.md>' to the '## {slug}' entry in references/sources.md, "
f"or '- **Research doc:** none' plus a '- **Basis:** <repo path>' line if no registry backs it."
)
elif rd_value == "" or PLACEHOLDER_RE.search(rd_value):
emit_fail(
f"Research doc field is empty or placeholder",
f"references/sources.md (## {slug})",
f"The '## {slug}' entry has an unfilled Research doc value.",
f"Set '- **Research doc:**' to a real path relative to repo root, or '(none)' if not applicable."
f"Set '- **Research doc:**' to the plugin's research sources.md (a path relative to the repo root), or to 'none' "
f"with a '- **Basis:** <repo path>' line if no registry backs this entry."
)
elif research_doc_is_none(rd_value):
# An entry with no Research registry must still say what it WAS drawn
@@ -944,6 +993,14 @@ for slug in unique_slugs:
f"The Basis value '{basis}' is a brace expansion or a comma- or semicolon-separated list.",
f"Write one '- **Basis:** <repo path>' line per path."
)
elif BASIS_REMOVED_RE.search(basis):
# A path the entry HISTORICALLY rested on, annotated
# '(removed in <sha>)' at the end of the value, is a declaration
# that it is gone on purpose. The sha is not resolved
# (git cat-file was judged over-engineering, ADR-0028 Q7), and
# with no repo root there is nothing to check either way, so
# this skips silently in both cases.
continue
elif not repo_root:
emit_info(
f"Basis check skipped for '{slug}' — no repo root above the skill directory",
@@ -951,12 +1008,13 @@ for slug in unique_slugs:
f"'{basis}' is a path relative to the repo root, but no ancestor of the skill directory contains a .git entry, "
f"so it cannot be resolved. Run this script against a skill inside a checkout."
)
elif BASIS_REMOVED_RE.search(basis):
# A path the entry HISTORICALLY rested on, annotated
# '(removed in <sha>)', is a declaration that it is gone on
# purpose. The sha is not resolved: the annotation is the
# author saying "deleted, and here is where to look".
continue
elif path_escapes_repo(repo_root, basis_path):
emit_fail(
f"Basis path '{basis_path}' is outside the repository for '{slug}'",
f"references/sources.md (## {slug})",
f"'{basis_path}' is absolute or resolves outside the repo root. Basis names repo paths.",
f"Use a path relative to the repo root that stays inside it."
)
elif not os.path.exists(os.path.join(repo_root, basis_path)):
emit_fail(
f"Basis path '{basis_path}' does not exist",
@@ -993,6 +1051,13 @@ for slug in unique_slugs:
f"Check 7 did not run for this slug. "
f"Give the value a file path relative to the repo root, or record 'none' plus a '- **Basis:**' if no registry backs this entry."
)
elif path_escapes_repo(repo_root, rd_path):
emit_fail(
f"Research doc '{rd_path}' for '{slug}' is outside the repository",
f"references/sources.md (## {slug})",
f"'{rd_path}' is absolute or resolves outside the repo root. Research doc names a file in this repo.",
f"Point Research doc at the plugin's research sources.md, as a path relative to the repo root."
)
else:
rd_abs = os.path.join(repo_root, rd_path)
if not os.path.isfile(rd_abs):

View File

@@ -1948,3 +1948,278 @@ EOF
refute_output --partial "Research doc field missing"
assert_output ""
}
# --- #121 review round: list detection, repo confinement, parser edge cases --
# Helper: one-line Research doc / Basis fixtures over make_entry_skill.
rd_fixture() { make_entry_skill "$TMPDIR/fakerepo" "$1"; }
basis_fixture() { make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s' '- **Research doc:** none' "$1")"; }
@test "#121 FAIL: a brace-only Research doc (no comma) names more than one path" {
rd_fixture '- **Research doc:** docs/research/{sources}.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 FAIL: a bare 'a.md; b.md' Research doc names more than one path" {
rd_fixture '- **Research doc:** docs/research/sources.md; docs/other-basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 FAIL: an annotated first path followed by ', second-path' is a list" {
rd_fixture '- **Research doc:** docs/research/sources.md (x), docs/research/topic.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 FAIL: a space-separated pair of Research docs is a list" {
rd_fixture '- **Research doc:** docs/research/sources.md docs/other-basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 FAIL: a space-separated pair of backticked Research docs is a list" {
rd_fixture '- **Research doc:** `docs/research/sources.md` `docs/other-basis.md`'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 pass: a single backticked Research doc path is unwrapped before resolving" {
rd_fixture '- **Research doc:** `docs/research/sources.md`'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_success
assert_output ""
}
@test "#121 pass: a ';' inside an annotation that holds a path is prose (path part only is checked)" {
rd_fixture '- **Research doc:** docs/research/sources.md (digested; docs/other-basis.md)'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_success
assert_output ""
}
@test "#121 FAIL: a bare 'a.md; b.md' Basis names more than one path" {
basis_fixture '- **Basis:** docs/basis.md; docs/other-basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis value names more than one path"
}
@test "#121 FAIL: a brace Basis names more than one path" {
basis_fixture '- **Basis:** docs/{basis,other-basis}.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis value names more than one path"
}
@test "#121 FAIL: a brace-only Basis names more than one path" {
basis_fixture '- **Basis:** docs/{basis}.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis value names more than one path"
}
@test "#121 FAIL: a space-separated Basis pair names more than one path" {
basis_fixture '- **Basis:** docs/basis.md docs/other-basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis value names more than one path"
}
@test "#121 pass: a backticked Basis path is unwrapped before resolving" {
basis_fixture '- **Basis:** `docs/basis.md`'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_success
assert_output ""
}
@test "#121 FAIL: an absolute Research doc path is outside the repo" {
rd_fixture '- **Research doc:** /etc/passwd'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 FAIL: a '..' Research doc escape is outside the repo" {
rd_fixture '- **Research doc:** ../outside/sources.md'
mkdir -p "$TMPDIR/outside"
printf '# R\n\n## my-source\n' > "$TMPDIR/outside/sources.md"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 FAIL: an absolute Basis path is outside the repo" {
basis_fixture '- **Basis:** /etc/passwd'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 FAIL: a '..' Basis escape is outside the repo even though the file exists" {
basis_fixture '- **Basis:** ../outside.md'
printf 'x\n' > "$TMPDIR/outside.md"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 FAIL: '(removed in abc)' is too short a sha to skip the check" {
basis_fixture '- **Basis:** docs/deleted-adr.md (removed in abc)'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis path 'docs/deleted-adr.md' does not exist"
}
@test "#121 FAIL: '(removed in <sha>)' followed by more text is not the annotation" {
basis_fixture '- **Basis:** docs/deleted-adr.md (removed in 5b80f30) but really still here'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis path 'docs/deleted-adr.md' does not exist"
}
@test "#121 pass: '(removed in <sha>)' Basis with no repo root is skipped silently" {
local skill="$TMPDIR/my-skill"
make_skill_with_source_keys "$skill"
mkdir -p "$skill/references"
cat > "$skill/references/sources.md" <<EOF
# Sources
## my-source
- **URL:** https://example.com/my-source
- **Description:** A test source.
- **Contributing files:** SKILL.md
- **Research doc:** none
- **Basis:** docs/gone.md (removed in 5b80f30)
- **Status:** \`extracted\`
EOF
run bash "$SCRIPT" "$skill"
assert_success
refute_output --partial "Basis check skipped"
}
@test "#121 parity: a '- **X**' bullet under a Basis header is a value, not the next field" {
make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s\n%s' '- **Research doc:** none' '**Basis:**' '- **docs/gone.md**')"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "does not exist"
refute_output --partial "Basis missing"
}
@test "#121 parity: '* ' bullets under a Basis header are read" {
make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s\n%s\n%s' '- **Research doc:** none' '**Basis:**' '* docs/basis.md' '* docs/gone.md')"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis path 'docs/gone.md' does not exist"
}
@test "#121 'none/foo.md' is a path, not a 'none' declaration" {
rd_fixture '- **Research doc:** none/foo.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
refute_output --partial "Basis missing"
}
@test "#121 'none-of-these.md' is a path, not a 'none' declaration" {
rd_fixture '- **Research doc:** none-of-these.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
refute_output --partial "Basis missing"
}
@test "#121 pass: 'None' and 'NONE' are recognised case-insensitively" {
local v
for v in None NONE; do
make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s' "- **Research doc:** $v" '- **Basis:** docs/basis.md')"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_success
assert_output ""
done
}
@test "#121 FAIL: an empty Basis value is empty, not missing" {
basis_fixture '- **Basis:**'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis is empty or placeholder"
refute_output --partial "Basis missing"
}
@test "#121 FAIL: a 'FILL IN:' Basis is a placeholder" {
basis_fixture '- **Basis:** FILL IN: repo path'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis is empty or placeholder"
}
@test "#121 FAIL: an empty inline Research doc says empty, not missing" {
rd_fixture '- **Research doc:**'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc field is empty or placeholder"
refute_output --partial "Research doc field missing"
}
@test "#121 FAIL: a missing Research doc advises the new grammar, not '<path-or-(none)>'" {
rd_fixture ''
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc field missing"
refute_output --partial "path-or-(none)"
assert_output --partial "Basis"
}
@test "#121 parity: an inline Basis with no leading hyphen is read" {
basis_fixture '**Basis:** docs/gone.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis path 'docs/gone.md' does not exist"
refute_output --partial "Basis missing"
}
@test "#121 FAIL: 'a.md;b.md' with no space is a list, for Research doc" {
rd_fixture '- **Research doc:** docs/research/sources.md;docs/basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Research doc names more than one path"
}
@test "#121 FAIL: 'a.md;b.md' with no space is a list, for Basis" {
basis_fixture '- **Basis:** docs/basis.md;docs/other-basis.md'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis value names more than one path"
}
@test "#121 FAIL: an absolute Basis path is outside the repo even when it points inside the checkout" {
make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s' '- **Research doc:** none' "- **Basis:** $TMPDIR/fakerepo/docs/basis.md")"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 FAIL: an absolute Research doc path is outside the repo even when it points inside the checkout" {
rd_fixture "- **Research doc:** $TMPDIR/fakerepo/docs/research/sources.md"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "outside the repository"
}
@test "#121 parity: a '* **Basis:**' bullet spelling is read" {
make_entry_skill "$TMPDIR/fakerepo" "$(printf '%s\n%s' '- **Research doc:** none' '* **Basis:** docs/gone.md')"
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_failure
assert_output --partial "Basis path 'docs/gone.md' does not exist"
}
@test "#121 pass: a comma inside a section-marker annotation is prose, not a list" {
rd_fixture '- **Research doc:** docs/research/sources.md § "Foo, bar and baz"'
run bash "$SCRIPT" "$TMPDIR/fakerepo/my-skill"
assert_success
assert_output ""
}