From 6f6b70781d0142360c0bd17bcc4986ccabfcb1ea Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Tue, 11 Aug 2026 21:49:38 +0000 Subject: [PATCH] fix(kyberforge): fix scope walk-up and manifest-parsing bugs from PR #93 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...rimitive-drops-provider-specific-fields.md | 12 ++- .../kyberforge/skills/agent-audit/README.md | 21 +++- .../skills/agent-audit/scripts/validate.sh | 11 +++ .../skills/agent-audit/tests/validate.bats | 95 +++++++++++++++++++ .../skills/agent-author/scripts/new-agent.sh | 73 +++++++------- .../skills/agent-author/tests/new-agent.bats | 35 +++++++ .../skills/skill-author/scripts/new-skill.sh | 48 +++++----- .../skills/skill-author/tests/new-skill.bats | 22 +++++ 8 files changed, 254 insertions(+), 63 deletions(-) diff --git a/docs/adr/0016-apm-agent-primitive-drops-provider-specific-fields.md b/docs/adr/0016-apm-agent-primitive-drops-provider-specific-fields.md index c8daf82..bf53a5a 100644 --- a/docs/adr/0016-apm-agent-primitive-drops-provider-specific-fields.md +++ b/docs/adr/0016-apm-agent-primitive-drops-provider-specific-fields.md @@ -40,11 +40,13 @@ Absent `tools:` means inherit-all-tools on both harnesses — the one value that on either target, unlike a present, harness-specific value that is guaranteed wrong on at least one of them. -`agent-audit`, at plugin scope, flags — as a **SUGGESTION**, not a FAIL, since this is an -upstream schema limitation rather than an authoring mistake — any agent whose description or -body implies a need for tool restriction or a Claude-only behavior the frontmatter can no -longer express. This gives visibility into the gap without pretending the schema can do -something it can't. +`agent-audit`, at plugin scope, is intended to flag — as a **SUGGESTION**, not a FAIL, since +this is an upstream schema limitation rather than an authoring mistake — any agent whose +description or body implies a need for tool restriction or a Claude-only behavior the +frontmatter can no longer express. This would give visibility into the gap without pretending +the schema can do something it can't. **Not yet implemented**: `check_apm_agent_file()` in +`validate.sh` currently validates only the field allowlist, `name`, `description`, and +body-emptiness/length — it has no heuristic for this case. Tracked as follow-up work. ### Scope boundary diff --git a/plugins/kyberforge/skills/agent-audit/README.md b/plugins/kyberforge/skills/agent-audit/README.md index 2053c91..f74140d 100644 --- a/plugins/kyberforge/skills/agent-audit/README.md +++ b/plugins/kyberforge/skills/agent-audit/README.md @@ -1,10 +1,27 @@ # agent-audit -Audits a Claude Code and Copilot agent definition file pair for correctness and quality. +Audits an agent definition for correctness and quality — a single vendor-neutral file at +plugin/APM scope, or a Claude Code and Copilot file pair at project/user scope. ## What it does -Accepts either file in a CC `.md` / Copilot `.agent.md` pair, derives the counterpart automatically, and validates both. Runs structural checks via `validate.sh` (required fields, kebab-case name, no placeholders, no CC-only fields in the Copilot file, silently-ignored fields at plugin scope), provenance chain validation via `validate-provenance.sh` (checks `source_keys` against `sources.md` at the plugin root), then qualitative checks on description phrasing and system prompt quality. Step 1 also runs a Vale-based prose sub-check via `vale-wrap.sh` against both files of the pair, using the `Kyberforge` style (both files) and `KyberforgeCopilot` style (Copilot file only) — every alert is a `FAIL`, cited by rule ID — falling back to Step 2 judgment when the `vale` binary is unavailable or reports `0 files` scanned. Produces a compact findings report in the same format as `skill-audit`. +At **plugin/APM scope**, accepts the single `.apm/agents/.agent.md` file — there is no +counterpart. Structural checks via `validate.sh` hard-`FAIL` any frontmatter field outside the +vendor-neutral allowlist (`name`, `description`, `model` — see ADR-0016), since `apm compile` +copies frontmatter verbatim to both harnesses and an unsafe field can't be silently dropped for +just one of them. + +At **project/user scope**, accepts either file in a CC `.md` / Copilot `.agent.md` pair, derives +the counterpart automatically, and validates both. Runs structural checks via `validate.sh` +(required fields, kebab-case name, no placeholders, no CC-only fields in the Copilot file, no +Copilot-only fields in the CC file), provenance chain validation via `validate-provenance.sh` +(checks `source_keys` against `sources.md` at the plugin root — plugin/APM scope only), then +qualitative checks on description phrasing and system prompt quality. Step 1 also runs a +Vale-based prose sub-check via `vale-wrap.sh` against both files of the pair, using the +`Kyberforge` style (both files) and `KyberforgeCopilot` style (Copilot file only) — every alert +is a `FAIL`, cited by rule ID — falling back to Step 2 judgment when the `vale` binary is +unavailable or reports `0 files` scanned. Produces a compact findings report in the same format +as `skill-audit`. ## Usage diff --git a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh index 9ec48f1..b48f15f 100755 --- a/plugins/kyberforge/skills/agent-audit/scripts/validate.sh +++ b/plugins/kyberforge/skills/agent-audit/scripts/validate.sh @@ -185,6 +185,17 @@ def check_apm_agent_file(fpath, allowlist, stem): fail(f"no valid YAML frontmatter (---...---) — {local_fname}") return + # The apm-agent.md template embeds its authoring guidance as HTML + # comments inside the frontmatter block (so they render invisible in a + # Markdown preview but stay visible in the raw file). get_frontmatter_keys + # silently ignores any line that isn't a `key:` match, so a comment left + # behind at ship time would otherwise pass unnoticed — yet apm compile + # copies this frontmatter verbatim to both harnesses, and `` is + # not valid YAML, so yaml.safe_load breaks on both downstream (ADR-0016). + if re.search(r'', fm): + fail(f"frontmatter still contains template HTML comments () " + f"— delete them before shipping — {local_fname}") + # Allowlist: only name/description/model may appear — no tools, no # Claude-only or Copilot-only fields. apm compile verbatim-copies # frontmatter to every target, so anything else is unsafe on at least diff --git a/plugins/kyberforge/skills/agent-audit/tests/validate.bats b/plugins/kyberforge/skills/agent-audit/tests/validate.bats index 7ed3568..68ad0a8 100644 --- a/plugins/kyberforge/skills/agent-audit/tests/validate.bats +++ b/plugins/kyberforge/skills/agent-audit/tests/validate.bats @@ -92,6 +92,78 @@ EOF refute_output --partial "FAIL" } +# --------------------------------------------------------------------------- +# Failing cases — project/user scope: CC/Copilot pair checks +# --------------------------------------------------------------------------- + +@test "fails when a CC-only field ('maxTurns') is present in a project-scope Copilot file" { + local root="$TMPDIR/project" + mkdir -p "$root/.git" "$root/.claude/agents" "$root/.github/agents" + cat > "$root/.claude/agents/my-agent.md" < "$root/.github/agents/my-agent.agent.md" < "$root/.claude/agents/my-agent.md" < "$root/.github/agents/my-agent.agent.md" < "$root/.claude/agents/my-agent.md" < "$root/apm.yml" < "$root/.apm/agents/my-agent.agent.md" < +--- + +You are a test agent. When invoked, do the thing. +EOF + run bash "$SCRIPT" "$root/.apm/agents/my-agent.agent.md" + assert_failure + assert_output --partial "template HTML comments" +} + @test "fails when body contains unfilled FILL IN: placeholder in a plugin/APM-scope agent file" { local root="$TMPDIR/pkg" mkdir -p "$root/.apm/agents" diff --git a/plugins/kyberforge/skills/agent-author/scripts/new-agent.sh b/plugins/kyberforge/skills/agent-author/scripts/new-agent.sh index a68302f..262479d 100755 --- a/plugins/kyberforge/skills/agent-author/scripts/new-agent.sh +++ b/plugins/kyberforge/skills/agent-author/scripts/new-agent.sh @@ -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 diff --git a/plugins/kyberforge/skills/agent-author/tests/new-agent.bats b/plugins/kyberforge/skills/agent-author/tests/new-agent.bats index 6546174..23d585d 100644 --- a/plugins/kyberforge/skills/agent-author/tests/new-agent.bats +++ b/plugins/kyberforge/skills/agent-author/tests/new-agent.bats @@ -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 # --------------------------------------------------------------------------- diff --git a/plugins/kyberforge/skills/skill-author/scripts/new-skill.sh b/plugins/kyberforge/skills/skill-author/scripts/new-skill.sh index 851c2a6..e4efd57 100755 --- a/plugins/kyberforge/skills/skill-author/scripts/new-skill.sh +++ b/plugins/kyberforge/skills/skill-author/scripts/new-skill.sh @@ -82,21 +82,24 @@ if [[ ! -d "$TARGET_INPUT" ]]; then 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. +# 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. # 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 + 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 } @@ -105,7 +108,7 @@ is_apm_package_manifest() { # Walk up from looking for a type-bearing apm.yml (package mode) or a # .git boundary / filesystem root (standalone mode). An apm.yml with no # top-level 'type:' field is a marketplace-only manifest — skip it and keep -# walking up. Prints two lines: the resolved root, then the mode. +# walking up. Prints one space-separated line: mode, then the resolved root. # --------------------------------------------------------------------------- find_package_root() { local current @@ -113,8 +116,7 @@ find_package_root() { while true; do if [[ -f "$current/apm.yml" ]]; then if is_apm_package_manifest "$current/apm.yml"; then - echo "$current" - echo "package" + echo "package $current" return 0 fi # apm.yml exists but has no type: field — marketplace-only manifest. @@ -123,15 +125,13 @@ find_package_root() { # .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" + echo "no-package $current" return 0 fi local parent parent="$(dirname "$current")" if [[ "$parent" == "$current" ]]; then - echo "$current" - echo "no-package" + echo "no-package $current" return 0 fi current="$parent" @@ -139,10 +139,12 @@ find_package_root() { } # `mapfile`/`readarray` are bash 4.0+ builtins with no fallback on macOS's -# stock /bin/bash 3.2 — read the two output lines individually instead. +# stock /bin/bash 3.2 — read the single space-separated output line with a +# plain `read` instead (bash 3.2-safe). `read` consumes only one line, so +# mode and path must be on the same line: MODE first (never contains +# whitespace), PKG_ROOT last (safely absorbs a path containing spaces). WALK_OUTPUT="$(find_package_root "$TARGET_INPUT")" -PKG_ROOT="$(echo "$WALK_OUTPUT" | sed -n '1p')" -MODE="$(echo "$WALK_OUTPUT" | sed -n '2p')" +read -r MODE PKG_ROOT <<< "$WALK_OUTPUT" if [[ "$MODE" == "package" ]]; then TARGET="$PKG_ROOT/.apm/skills/$SKILL_NAME" diff --git a/plugins/kyberforge/skills/skill-author/tests/new-skill.bats b/plugins/kyberforge/skills/skill-author/tests/new-skill.bats index 67cf7e1..1bd2631 100644 --- a/plugins/kyberforge/skills/skill-author/tests/new-skill.bats +++ b/plugins/kyberforge/skills/skill-author/tests/new-skill.bats @@ -184,3 +184,25 @@ EOF assert [ -d "$DEST/repo/sub/my-tool" ] assert [ ! -d "$DEST/repo/my-tool" ] } + +@test "package mode: matched-quote type value ('skill') is recognized" { + mkdir -p "$DEST/pkg" + printf 'name: my-pkg\ntype: "skill"\n' > "$DEST/pkg/apm.yml" + run bash "$SCRIPT" my-tool "$DEST/pkg" + assert_success + assert_output --partial "Mode: package" +} + +@test "mismatched-quote type value is rejected, falls through to standalone mode" { + printf "name: my-pkg\ntype: \"skill'\n" > "$DEST/apm.yml" + run bash "$SCRIPT" my-tool "$DEST" + assert_success + assert_output --partial "Mode: standalone" +} + +@test "type: line is recognized even without a trailing newline on apm.yml" { + printf 'name: my-pkg\ntype: skill' > "$DEST/apm.yml" + run bash "$SCRIPT" my-tool "$DEST" + assert_success + assert_output --partial "Mode: package" +}