diff --git a/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-provenance-skill.sh b/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-provenance-skill.sh index 25bb0b3..0f283d6 100755 --- a/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-provenance-skill.sh +++ b/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-provenance-skill.sh @@ -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 ) 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:** ' to the '## {slug}' entry in references/sources.md." + f"Add '- **Research doc:** ' to the '## {slug}' entry in references/sources.md, " + f"or '- **Research doc:** none' plus a '- **Basis:** ' 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:** ' 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:** ' line per path." ) + elif BASIS_REMOVED_RE.search(basis): + # A path the entry HISTORICALLY rested on, annotated + # '(removed in )' 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 )', 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): diff --git a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats index c7f4a02..3a51c53 100644 --- a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats +++ b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats @@ -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 )' 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 )' 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" <'" { + 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 "" +}