fix(kyberforge): fix scope walk-up and manifest-parsing bugs from PR #93 review
A fresh /code-review of the APM-native authoring retarget (PR #93) found several correctness bugs beyond the ones already fixed on this branch: - new-agent.sh silently walked a marker-less subdirectory under $HOME up to user scope, contradicting its own usage text ("user scope is checked directly, no walk-up") and risking scaffolding into shared global ~/.claude or ~/.copilot directories instead of the intended local path. - The hand-copied apm.yml type: manifest detector in new-agent.sh and new-skill.sh accepted mismatched quotes (e.g. `type: "skill'`) that validate.sh's regex correctly rejects, and silently dropped a final apm.yml line lacking a trailing newline — causing the scaffolder and validator to disagree on scope for identical input. - Plugin-scope agent frontmatter could still contain the apm-agent.md template's HTML comments at ship time with no audit signal, yet apm compile copies frontmatter verbatim and <!-- --> breaks YAML parsing on both downstream harnesses. - ADR-0016 asserted agent-audit already implements a SUGGESTION heuristic for tool-restriction-needing plugin-scope agents; it doesn't. - agent-audit/README.md still described the old plugin-pair model this PR replaced with a single-file allowlist model. - validate.sh's project/user-scope CC-only/Copilot-only field checks and counterpart-missing check lost their only test coverage when the old plugin-pair fixture was deleted. Also replaces an echo-into-sed two-value parse (4 forks per call) with a single space-separated echo + read in both scaffolders. Regression tests added for every fix above, including one for a bug this pass introduced and the test suite caught: an initial two-line echo + `read` attempt silently dropped the second value, since `read` consumes only one line regardless of embedded newlines. Full suite: 158 bats tests, 39 shell-script tests, 12/12 summary categories, 0 failures. Refs: #89, #93
This commit is contained in:
@@ -84,20 +84,23 @@ 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.
|
||||
# types (instructions/skill/hybrid/prompts) — mirrors validate.sh's
|
||||
# APM_TYPE_RE: an optional quote around the value must be closed by the
|
||||
# *same* quote character (a mismatched or unterminated quote is rejected,
|
||||
# not silently stripped), and the value must be followed by whitespace or
|
||||
# end-of-line so `prompts-only` doesn't false-match on the `prompts` prefix.
|
||||
# `|| [[ -n "$line" ]]` in the read condition also processes a final line
|
||||
# that lacks a trailing newline, which `read` alone would otherwise skip.
|
||||
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
|
||||
local apm_yml="$1" line
|
||||
while IFS= read -r line || [[ -n "$line" ]]; do
|
||||
if [[ "$line" =~ ^type:[[:space:]]*(instructions|skill|hybrid|prompts)([[:space:]]|$) ]]; then
|
||||
return 0
|
||||
fi
|
||||
if [[ "$line" =~ ^type:[[:space:]]*([\"\'])(instructions|skill|hybrid|prompts)([\"\'])([[:space:]]|$) ]] \
|
||||
&& [[ "${BASH_REMATCH[1]}" == "${BASH_REMATCH[3]}" ]]; then
|
||||
return 0
|
||||
fi
|
||||
done < "$apm_yml"
|
||||
return 1
|
||||
}
|
||||
@@ -110,44 +113,53 @@ is_apm_package_manifest() {
|
||||
# (plugin/APM scope) — stop and return it.
|
||||
# - an apm.yml with no `type:` field is a marketplace-only manifest — skip
|
||||
# it, keep walking up.
|
||||
# - 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).
|
||||
# - user scope is checked directly at $HOME, no walk-up (see usage text
|
||||
# above): ROOT itself being $HOME resolves to user scope, 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).
|
||||
# Walking *up into* $HOME from a nested directory with no apm.yml/.git
|
||||
# of its own does NOT promote to user scope — it resolves to project
|
||||
# scope instead, same as any other unmatched boundary, so a stray
|
||||
# directory under $HOME can't be silently redirected into the shared
|
||||
# global ~/.claude or ~/.copilot agent directories.
|
||||
# - 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.
|
||||
# - filesystem root reached with neither found — project scope, same as
|
||||
# any other unmatched boundary.
|
||||
find_package_root() {
|
||||
local current="$1"
|
||||
local root="$1" current="$1"
|
||||
while true; do
|
||||
if [[ -f "$current/apm.yml" ]] && is_apm_package_manifest "$current/apm.yml"; then
|
||||
echo "plugin"
|
||||
echo "$current"
|
||||
echo "plugin $current"
|
||||
return
|
||||
fi
|
||||
if [[ "$current" == "$HOME" ]]; then
|
||||
echo "user"
|
||||
echo "$current"
|
||||
if [[ "$current" == "$root" ]]; then
|
||||
echo "user $current"
|
||||
return
|
||||
fi
|
||||
echo "project $current"
|
||||
return
|
||||
fi
|
||||
if [[ -e "$current/.git" ]]; then
|
||||
echo "project"
|
||||
echo "$current"
|
||||
echo "project $current"
|
||||
return
|
||||
fi
|
||||
local parent
|
||||
parent="$(dirname "$current")"
|
||||
if [[ "$parent" == "$current" ]]; then
|
||||
echo "boundary-reached"
|
||||
echo "$current"
|
||||
echo "project $current"
|
||||
return
|
||||
fi
|
||||
current="$parent"
|
||||
done
|
||||
}
|
||||
|
||||
# `read` consumes a single line, so kind and path are emitted on one
|
||||
# space-separated line rather than two `echo`s — kind first (never contains
|
||||
# spaces), path last (absorbs any spaces in the path safely).
|
||||
WALK_RESULT="$(find_package_root "$ROOT")"
|
||||
WALK_KIND="$(echo "$WALK_RESULT" | sed -n '1p')"
|
||||
WALK_ROOT="$(echo "$WALK_RESULT" | sed -n '2p')"
|
||||
read -r WALK_KIND WALK_ROOT <<< "$WALK_RESULT"
|
||||
|
||||
PACKAGE_ROOT=""
|
||||
case "$WALK_KIND" in
|
||||
@@ -161,11 +173,6 @@ case "$WALK_KIND" in
|
||||
project)
|
||||
SCOPE="project"
|
||||
;;
|
||||
boundary-reached)
|
||||
# Default fallback, same as the pre-walk-up script: no plugin/APM
|
||||
# marker, no $HOME boundary, and no .git means project scope.
|
||||
SCOPE="project"
|
||||
;;
|
||||
esac
|
||||
|
||||
# Determine file destinations
|
||||
|
||||
@@ -130,6 +130,29 @@ teardown() {
|
||||
assert [ -f "$ROOT/.apm/agents/my-agent.agent.md" ]
|
||||
}
|
||||
|
||||
@test "plugin/APM scope: matched-quote type value ('skill') is recognized" {
|
||||
printf 'name: my-package\ntype: "skill"\n' > "$ROOT/apm.yml"
|
||||
run bash "$SCRIPT" my-agent "$ROOT"
|
||||
assert_success
|
||||
assert [ -f "$ROOT/.apm/agents/my-agent.agent.md" ]
|
||||
}
|
||||
|
||||
@test "plugin/APM scope: mismatched-quote type value is rejected, falls through to project scope" {
|
||||
mkdir -p "$ROOT/.git"
|
||||
printf "name: my-package\ntype: \"skill'\n" > "$ROOT/apm.yml"
|
||||
run bash "$SCRIPT" my-agent "$ROOT"
|
||||
assert_success
|
||||
assert [ ! -f "$ROOT/.apm/agents/my-agent.agent.md" ]
|
||||
assert [ -f "$ROOT/.claude/agents/my-agent.md" ]
|
||||
}
|
||||
|
||||
@test "plugin/APM scope: type: line is recognized even without a trailing newline on the file" {
|
||||
printf 'name: my-package\ntype: skill' > "$ROOT/apm.yml"
|
||||
run bash "$SCRIPT" my-agent "$ROOT"
|
||||
assert_success
|
||||
assert [ -f "$ROOT/.apm/agents/my-agent.agent.md" ]
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Old plugin.json marker is no longer recognized (full switch, no dual-mode)
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -222,6 +245,18 @@ teardown() {
|
||||
rm -rf "$FAKE_HOME"
|
||||
}
|
||||
|
||||
@test "user scope is checked directly at \$HOME, no walk-up: a marker-less subdir under \$HOME resolves to project scope, not user scope" {
|
||||
FAKE_HOME="$(mktemp -d)"
|
||||
mkdir -p "$FAKE_HOME/scratch/testdir"
|
||||
run env HOME="$FAKE_HOME" bash "$SCRIPT" my-agent "$FAKE_HOME/scratch/testdir"
|
||||
assert_success
|
||||
assert [ -f "$FAKE_HOME/scratch/testdir/.claude/agents/my-agent.md" ]
|
||||
assert [ -f "$FAKE_HOME/scratch/testdir/.github/agents/my-agent.agent.md" ]
|
||||
refute [ -f "$FAKE_HOME/.claude/agents/my-agent.md" ]
|
||||
refute [ -f "$FAKE_HOME/.copilot/agents/my-agent.agent.md" ]
|
||||
rm -rf "$FAKE_HOME"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Name validation
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user