fix(kyberforge): fix apm-scope validation gaps in agent/skill authoring scripts
Post-implementation review of PR #93 (issue #89's apm.yml-native retargeting of skill-author/skill-audit/agent-author/agent-audit) found four confirmed defects across the four scripts' apm.yml `type:` walk-up logic: - field-inventory.md's apm-agent-allowlist was missing `source_keys`, contradicting agent-author/SKILL.md's own instruction (Step 5 checklist) to allow it at plugin/APM scope — a correctly-authored file with source_keys failed validate.sh. - validate.sh's APM_TYPE_RE and validate-provenance.sh's TYPE_RE disagreed: the former tolerated a quoted `type: "skill"` value, the latter didn't, despite agent-audit/SKILL.md explicitly documenting that validate-provenance.sh walks up "the same way validate.sh does". Both also used `\b` word-boundary matching, which false-matches a malformed value like `type: prompts-only` on the `prompts` prefix. Unified both regexes to be quote-tolerant and require an exact value. - All four scripts' `.git` project-boundary check used isdir()/[[ -d ]], which misses git worktrees where `.git` is a regular file (`gitdir: ...`) rather than a directory. Switched to exists()/[[ -e ]]. - new-agent.sh and new-skill.sh had the same quote-intolerance as above via inline `grep -qE` calls (new-skill.sh's also had the `\b` false-match bug); replaced both with a shared-shape `is_apm_package_manifest` bash helper matching the Python regex's semantics. Four other findings from the same review turned out not to be bugs: a bare `plugin.json` no longer signaling plugin scope is documented, intentional behavior (agent-audit/SKILL.md:30, agent-author/SKILL.md:87), deferred to issue #90's real plugin.json-to-apm.yml conversion — not something this fix should reverse. Verified via direct reproduction of each defect plus the full test suite: 147/147 bats tests, 39/39 shell-script tests, 12/12 summary categories. Refs: #89
This commit is contained in:
@@ -25,4 +25,4 @@ target disable-model-invocation user-invocable mcp-servers metadata
|
||||
|
||||
## apm-agent-allowlist
|
||||
|
||||
name description model
|
||||
name description model source_keys
|
||||
|
||||
@@ -54,7 +54,12 @@ if not (fname.endswith('.agent.md') or fname.endswith('.md')):
|
||||
print(f"Error: unrecognized extension '{fname}' — expected .md or .agent.md", file=sys.stderr)
|
||||
sys.exit(2)
|
||||
|
||||
TYPE_RE = re.compile(r'^type:\s*(instructions|skill|hybrid|prompts)\b')
|
||||
# Matches a top-level `type:` line whose value is exactly one of the four
|
||||
# package content types — identical to validate.sh's APM_TYPE_RE. Group 1's
|
||||
# optional quote must be closed by \1 (or nothing), and the value must be
|
||||
# followed by whitespace/end-of-line so a malformed value like `prompts-only`
|
||||
# doesn't false-match on the `prompts` prefix.
|
||||
TYPE_RE = re.compile(r"^type:\s*(['\"]?)(instructions|skill|hybrid|prompts)\1(?:\s|$)")
|
||||
|
||||
# --- Find package root: walk up for the nearest ancestor apm.yml that
|
||||
# declares a top-level type: field. An apm.yml with no type: field is a
|
||||
@@ -69,7 +74,9 @@ def find_plugin_root(start_dir):
|
||||
with open(apm_yml) as f:
|
||||
if any(TYPE_RE.match(line) for line in f):
|
||||
return current
|
||||
if os.path.isdir(os.path.join(current, '.git')):
|
||||
# .git is a directory in a normal checkout but a file (`gitdir: ...`)
|
||||
# in a git worktree — exists() covers both.
|
||||
if os.path.exists(os.path.join(current, '.git')):
|
||||
return None
|
||||
parent = os.path.dirname(current)
|
||||
if parent == current:
|
||||
|
||||
@@ -133,9 +133,13 @@ def is_copilot_cloud_ide(fpath):
|
||||
|
||||
# --- Detect scope ---
|
||||
# APM_TYPE_RE matches a top-level (column-0) `type:` line in apm.yml whose value is
|
||||
# one of the four package content types. `[\'"]?` tolerates a quoted value; the
|
||||
# pattern doesn't anchor the line end, so trailing whitespace/comments don't matter.
|
||||
APM_TYPE_RE = re.compile(r"^type:\s*['\"]?(instructions|skill|hybrid|prompts)\b")
|
||||
# exactly one of the four package content types. Group 1 captures an optional
|
||||
# opening quote; \1 requires the same character (or nothing) to close it, so
|
||||
# "skill" and '"skill"' both match but a mismatched quote doesn't. The value
|
||||
# must then be followed by whitespace or end-of-line — not just a non-word
|
||||
# character — so a malformed value like `prompts-only` is correctly rejected
|
||||
# instead of false-matching on the `prompts` prefix.
|
||||
APM_TYPE_RE = re.compile(r"^type:\s*(['\"]?)(instructions|skill|hybrid|prompts)\1(?:\s|$)")
|
||||
|
||||
def find_apm_package_root(apm_yml_path):
|
||||
"""Return True if apm_yml_path has a top-level type: line (i.e. is a package
|
||||
@@ -158,7 +162,9 @@ def detect_scope(start_dir):
|
||||
# can't shadow user scope by being its own .git repo.
|
||||
if current == home:
|
||||
return 'user', home
|
||||
if os.path.isdir(os.path.join(current, '.git')):
|
||||
# .git is a directory in a normal checkout but a file (`gitdir: ...`)
|
||||
# in a git worktree — exists() covers both.
|
||||
if os.path.exists(os.path.join(current, '.git')):
|
||||
return 'project', current
|
||||
parent = os.path.dirname(current)
|
||||
if parent == current:
|
||||
|
||||
@@ -83,6 +83,25 @@ if [[ ! -d "$ROOT" ]]; then
|
||||
fi
|
||||
ROOT="$(cd "$ROOT" && pwd)"
|
||||
|
||||
# True if apm_yml's top-level `type:` line names one of the four APM package
|
||||
# types (instructions/skill/hybrid/prompts) — tolerating an optional matching
|
||||
# quote around the value and requiring the value end there, so a malformed
|
||||
# value like `prompts-only` doesn't false-match on the `prompts` prefix.
|
||||
is_apm_package_manifest() {
|
||||
local apm_yml="$1" line value
|
||||
while IFS= read -r line; do
|
||||
[[ "$line" =~ ^type:[[:space:]]*(.*)$ ]] || continue
|
||||
value="${BASH_REMATCH[1]}"
|
||||
value="${value%%[[:space:]]*}"
|
||||
value="${value#\"}"; value="${value%\"}"
|
||||
value="${value#\'}"; value="${value%\'}"
|
||||
case "$value" in
|
||||
instructions|skill|hybrid|prompts) return 0 ;;
|
||||
esac
|
||||
done < "$apm_yml"
|
||||
return 1
|
||||
}
|
||||
|
||||
# --- Walk-up package-root detection ---
|
||||
#
|
||||
# Mirrors agent-audit's validate.sh scope walk-up, with apm.yml + type: swapped
|
||||
@@ -94,12 +113,13 @@ ROOT="$(cd "$ROOT" && pwd)"
|
||||
# - reaching $HOME marks the user-scope boundary — stop, even if $HOME is
|
||||
# itself a .git-tracked dotfiles directory (checked before the .git test
|
||||
# below, so a dotfiles repo at $HOME can't shadow user scope).
|
||||
# - a .git directory marks the project-scope boundary — stop.
|
||||
# - a .git file or directory marks the project-scope boundary (a worktree's
|
||||
# .git is a file, not a directory) — stop.
|
||||
# - filesystem root reached with neither found — boundary-reached.
|
||||
find_package_root() {
|
||||
local current="$1"
|
||||
while true; do
|
||||
if [[ -f "$current/apm.yml" ]] && grep -qE '^type:[[:space:]]*(instructions|skill|hybrid|prompts)([[:space:]]|$)' "$current/apm.yml"; then
|
||||
if [[ -f "$current/apm.yml" ]] && is_apm_package_manifest "$current/apm.yml"; then
|
||||
echo "plugin"
|
||||
echo "$current"
|
||||
return
|
||||
@@ -109,7 +129,7 @@ find_package_root() {
|
||||
echo "$current"
|
||||
return
|
||||
fi
|
||||
if [[ -d "$current/.git" ]]; then
|
||||
if [[ -e "$current/.git" ]]; then
|
||||
echo "project"
|
||||
echo "$current"
|
||||
return
|
||||
|
||||
@@ -81,6 +81,26 @@ if [[ ! -d "$TARGET_INPUT" ]]; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# True if apm_yml's top-level `type:` line names one of the four APM package
|
||||
# types (instructions/skill/hybrid/prompts) — tolerating an optional matching
|
||||
# quote around the value and requiring the value end there, so a malformed
|
||||
# value like `prompts-only` doesn't false-match on the `prompts` prefix.
|
||||
# Identical to agent-author's new-agent.sh copy of this helper.
|
||||
is_apm_package_manifest() {
|
||||
local apm_yml="$1" line value
|
||||
while IFS= read -r line; do
|
||||
[[ "$line" =~ ^type:[[:space:]]*(.*)$ ]] || continue
|
||||
value="${BASH_REMATCH[1]}"
|
||||
value="${value%%[[:space:]]*}"
|
||||
value="${value#\"}"; value="${value%\"}"
|
||||
value="${value#\'}"; value="${value%\'}"
|
||||
case "$value" in
|
||||
instructions|skill|hybrid|prompts) return 0 ;;
|
||||
esac
|
||||
done < "$apm_yml"
|
||||
return 1
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Walk up from <path> looking for a type-bearing apm.yml (package mode) or a
|
||||
# .git boundary / filesystem root (standalone mode). An apm.yml with no
|
||||
@@ -92,7 +112,7 @@ find_package_root() {
|
||||
current="$(cd "$1" && pwd)"
|
||||
while true; do
|
||||
if [[ -f "$current/apm.yml" ]]; then
|
||||
if grep -qE '^type:[[:space:]]*(instructions|skill|hybrid|prompts)\b' "$current/apm.yml"; then
|
||||
if is_apm_package_manifest "$current/apm.yml"; then
|
||||
echo "$current"
|
||||
echo "package"
|
||||
return 0
|
||||
@@ -100,7 +120,9 @@ find_package_root() {
|
||||
# apm.yml exists but has no type: field — marketplace-only manifest.
|
||||
# Not a package match; keep walking up.
|
||||
fi
|
||||
if [[ -d "$current/.git" ]]; then
|
||||
# .git is a directory in a normal checkout but a file (`gitdir: ...`) in
|
||||
# a git worktree — -e covers both.
|
||||
if [[ -e "$current/.git" ]]; then
|
||||
echo "$current"
|
||||
echo "no-package"
|
||||
return 0
|
||||
|
||||
Reference in New Issue
Block a user