fix(factory-audit): flag unbraced plugin-root tokens and close hook check gaps
Why: PR #144 review round 4 reproduced hooks referencing $PLUGIN_ROOT or ${PLUGIN_ROOT} without a path separator passing the audit, although apm only rewrites ${TOKEN}/ and the deployed hook points nowhere. - FAIL unbraced or unseparated plugin-root tokens - check the exec bit for scripts run via an interpreter -c string - skip env NAME=value prefixes when locating bare relative paths - correct input: and empty-frontmatter messages, depth-walk applyTo braces - INFO on unrecognised targets; failing-case tests for untested checks - document tiers, blind spots and crash exit 2; drop rtk from portable flow - restore the after-a-hand-edit trigger; pin upstream apm source URL Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkT7RSDwDbmrM9T34b6sTi
This commit is contained in:
@@ -58,7 +58,7 @@ suite must mean a differently named script.
|
||||
Each entry point decides for itself what it was handed. ADR-0025 states the
|
||||
skill and agent rule: a directory containing `SKILL.md` takes the skill flow; an
|
||||
`.agent.md` file, or a file under a directory named `agents/`, takes the agent
|
||||
flow. `scripts/validate.sh` adds three primitive shapes: a `*.instructions.md`
|
||||
flow. `scripts/validate.sh` adds the hook, instruction and prompt shapes: a `*.instructions.md`
|
||||
or `*.prompt.md` file takes the instruction or prompt flow wherever it sits, and
|
||||
a `.json` file directly under a `hooks/` directory takes the hook flow. Any
|
||||
shape outside the five is rejected rather than guessed at. Classification could
|
||||
@@ -66,17 +66,17 @@ not be wrong before the merge — each script was hard-wired to one artifact typ
|
||||
— so it is asserted from every side rather than in one place:
|
||||
|
||||
- the skill-side suites pin the skill-directory classification and the
|
||||
neither-shape rejection,
|
||||
no-shape rejection,
|
||||
- the agent-side suites pin the two agent rules *separately* — `.agent.md` in a
|
||||
directory that is not `agents/`, and a plain `.md` under `.apm/agents/` — so
|
||||
that a detector implementing only one of them cannot pass both. Plus a control
|
||||
asserting an agent file never picks up a skill-only gate.
|
||||
- `validate-primitive.bats` pins the hook, instruction and prompt shapes,
|
||||
including the precedence that makes a `*.instructions.md` or `*.prompt.md`
|
||||
under `agents/` take the primitive flow rather than the agent flow, a hook
|
||||
under `agents/` take the instruction or prompt flow rather than the agent flow, a hook
|
||||
file under a package-root `hooks/`, and a `.json` outside `hooks/` matching
|
||||
no shape. It also pins the primitive exit tiers: every negative case asserts
|
||||
exit 1 (`assert_failure 1`), and a missing python3 or PyYAML asserts exit 2.
|
||||
no shape. It also pins their exit tiers: every negative case asserts
|
||||
exit 1 (`assert_failure 1`), and a missing python3 or PyYAML, or a crash inside the checks, asserts exit 2.
|
||||
|
||||
Both skill-side suites additionally pin the `SKILL.md` **file** path, not just
|
||||
the directory: a pre-commit `files:` hook matches files, so every hook-driven
|
||||
|
||||
@@ -535,6 +535,150 @@ teardown() {
|
||||
refute_output --partial "FAIL"
|
||||
}
|
||||
|
||||
@test "hook: an unbraced plugin-root token is a FAIL — apm rewrites only \${TOKEN}/<path>" {
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"\"$PLUGIN_ROOT/scripts/missing.sh\"","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "unbraced \$PLUGIN_ROOT"
|
||||
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"bash $CLAUDE_PLUGIN_ROOT/scripts/x.sh","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "unbraced \$CLAUDE_PLUGIN_ROOT"
|
||||
}
|
||||
|
||||
@test "hook: a braced plugin-root token not followed directly by a path separator is a FAIL" {
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"cd ${PLUGIN_ROOT} && node index.js","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "\${PLUGIN_ROOT} is not followed directly by / or \\"
|
||||
|
||||
# The split-quote form keeps its own, more specific FAIL and is not double-reported.
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"\"${PLUGIN_ROOT}\"/.apm/hooks/scripts/check.sh","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "splits the quote"
|
||||
refute_output --partial "is not followed directly by"
|
||||
}
|
||||
|
||||
@test "hook: a script run as the first token of a -c command string must be executable" {
|
||||
chmod -x "$PKG/.apm/hooks/scripts/check.sh"
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"bash -c \"${PLUGIN_ROOT}/.apm/hooks/scripts/check.sh\"","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "is run directly but is not executable"
|
||||
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"sh -xc ./scripts/check.sh","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "is run directly but is not executable"
|
||||
|
||||
# Through an interpreter inside the command string, the exec bit is not needed.
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"bash -c \"bash ${PLUGIN_ROOT}/.apm/hooks/scripts/check.sh\"","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_success
|
||||
refute_output --partial "FAIL"
|
||||
}
|
||||
|
||||
@test "hook: env options and NAME=value assignments are skipped to reach the interpreter's script" {
|
||||
local cmd
|
||||
for cmd in 'env FOO=1 bash scripts/check.sh' 'env -i FOO=1 bash scripts/check.sh' 'env -u HOME bash scripts/check.sh' 'FOO=1 bash scripts/check.sh'; do
|
||||
write_hook hooks.json "{\"hooks\":{\"Stop\":[{\"hooks\":[{\"type\":\"command\",\"command\":\"$cmd\",\"timeout\":5}]}]}}"
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "script 'scripts/check.sh' is a bare relative path"
|
||||
done
|
||||
}
|
||||
|
||||
@test "hook: targets: naming no recognised hook target is an INFO, not a silent skip" {
|
||||
printf 'name: test-package\nversion: 0.1.0\ntargets: [claude-code]\n' > "$PKG/apm.yml"
|
||||
write_hook hooks.json '{"hooks":{"stop":[{"hooks":[{"type":"command","command":"true","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_success
|
||||
assert_output --partial "INFO targets: in apm.yml names no hook target apm 0.28.0 recognises (claude-code)"
|
||||
}
|
||||
|
||||
@test "hook: malformed shapes are FAILs — top level, hooks value, entry and nested hooks" {
|
||||
write_hook hooks.json '[]'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "top level is not a JSON object"
|
||||
|
||||
write_hook hooks.json '{"hooks":[]}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "'hooks' is not an object"
|
||||
|
||||
write_hook hooks.json '{"hooks":{"Stop":["true"]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "event 'Stop' entry 0 is not an object"
|
||||
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":"true"}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "nested 'hooks' is not a list of objects"
|
||||
}
|
||||
|
||||
@test "hook: an empty event name is a FAIL" {
|
||||
write_hook hooks.json '{"hooks":{"":[{"hooks":[{"type":"command","command":"true","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "empty event name"
|
||||
}
|
||||
|
||||
@test "hook: a script path containing \$ or a backtick is a FAIL" {
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"${PLUGIN_ROOT}/scripts/$X.sh","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "contains '\$' or a backtick"
|
||||
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"${PLUGIN_ROOT}/scripts/`x`.sh","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "contains '\$' or a backtick"
|
||||
}
|
||||
|
||||
@test "hook: a lowercase event reaching an unlisted harness is judged after apm's rename" {
|
||||
printf 'name: test-package\nversion: 0.1.0\ntargets: [gemini]\n' > "$PKG/apm.yml"
|
||||
# Gemini renames preToolUse to BeforeTool: PascalCase after the rename, no finding.
|
||||
write_hook hooks.json '{"hooks":{"preToolUse":[{"matcher":"x","hooks":[{"type":"command","command":"true","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_success
|
||||
refute_output --partial "preToolUse"
|
||||
|
||||
# sessionStart is not in Gemini's map, so it reaches Gemini verbatim.
|
||||
write_hook hooks.json '{"hooks":{"sessionStart":[{"matcher":"x","hooks":[{"type":"command","command":"true","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_success
|
||||
assert_output --partial "SUGGESTION event 'sessionStart' reaches gemini verbatim"
|
||||
}
|
||||
|
||||
@test "hook: no apm.yml at the inferred .apm/ package root is an INFO" {
|
||||
rm "$PKG/apm.yml"
|
||||
write_hook hooks.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"true","timeout":5}]}]}}'
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_success
|
||||
assert_output --partial "INFO no apm.yml at the inferred package root"
|
||||
}
|
||||
|
||||
@test "hook, instruction, prompt: a file that is not valid UTF-8 is a FAIL" {
|
||||
printf '{"hooks":{"Stop":[]}}\xff\n' > "$PKG/.apm/hooks/hooks.json"
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
assert_failure 1
|
||||
assert_output --partial "not valid UTF-8"
|
||||
|
||||
printf -- '---\ndescription: x\n---\n\nbody \xff\n' > "$PKG/.apm/instructions/x.instructions.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "not valid UTF-8"
|
||||
|
||||
printf -- '---\ndescription: x\n---\n\nbody \xff\n' > "$PKG/.apm/prompts/x.prompt.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/x.prompt.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "not valid UTF-8"
|
||||
}
|
||||
|
||||
@test "hook: an unedited primitive-author hook template is an unfilled-placeholder FAIL" {
|
||||
cp "$REPO_ROOT/plugins/kyberforge/.apm/skills/primitive-author/assets/templates/hook.json.template" "$PKG/.apm/hooks/hooks.json"
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/hooks.json"
|
||||
@@ -656,6 +800,45 @@ applyTo: "**/*.py"' 'body'
|
||||
assert_output --partial "unfilled template placeholder 'FILL IN'"
|
||||
}
|
||||
|
||||
@test "instruction: no frontmatter block is a FAIL" {
|
||||
printf 'Just a body.\n' > "$PKG/.apm/instructions/x.instructions.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "has no YAML frontmatter block"
|
||||
}
|
||||
|
||||
@test "instruction: an empty frontmatter block is read as empty, not as a missing block" {
|
||||
printf -- '---\n---\n\nbody\n' > "$PKG/.apm/instructions/x.instructions.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
refute_output --partial "has no YAML frontmatter block"
|
||||
assert_output --partial "'description' is missing or empty"
|
||||
}
|
||||
|
||||
@test "instruction: frontmatter that is not a mapping is a FAIL" {
|
||||
write_instruction x '- a
|
||||
- b' 'body'
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "frontmatter is not a YAML mapping"
|
||||
}
|
||||
|
||||
@test "instruction: an applyTo that is neither a string nor a list is a FAIL" {
|
||||
write_instruction x 'description: x
|
||||
applyTo: 5' 'body'
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "applyTo is neither a string nor a list"
|
||||
}
|
||||
|
||||
@test "instruction: an applyTo glob closing a brace before opening it is unbalanced" {
|
||||
write_instruction x 'description: x
|
||||
applyTo: "src/}{.py"' 'body'
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/x.instructions.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "has unbalanced braces or brackets"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Prompts
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -684,6 +867,7 @@ input:
|
||||
description: The PR' 'Review ${input:pr_number}.'
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "is one map with several keys — apm reads every key as an argument name"
|
||||
assert_output --partial "yields arguments [name, description]"
|
||||
}
|
||||
|
||||
@@ -809,6 +993,38 @@ $(printf 'line\n%.0s' $(seq 1 80))
|
||||
assert_output --partial "unfilled template placeholder 'FILL IN'"
|
||||
}
|
||||
|
||||
@test "prompt: body references with input: declared but every name invalid do not claim input: is absent" {
|
||||
write_prompt review-pr 'description: Review a PR.
|
||||
input:
|
||||
- 1pr: "The PR"' 'Review ${input:pr_number}.'
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_failure 1
|
||||
refute_output --partial "no input: is declared"
|
||||
assert_output --partial "input: does not declare 'pr_number'"
|
||||
}
|
||||
|
||||
@test "prompt: a name that is not a safe path segment is a FAIL" {
|
||||
printf -- '---\ndescription: x\n---\n\nbody\n' > "$PKG/.apm/prompts/..prompt.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/..prompt.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "is not a safe path segment"
|
||||
}
|
||||
|
||||
@test "prompt: an input entry that is not a string name, and an input of no known shape, are FAILs" {
|
||||
write_prompt review-pr 'description: Review a PR.
|
||||
input:
|
||||
- 5: "x"' 'Review.'
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "input entry 5 is not a string name"
|
||||
|
||||
write_prompt review-pr 'description: Review a PR.
|
||||
input: 5' 'Review.'
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_failure 1
|
||||
assert_output --partial "input is neither a name, a list nor a map"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Never ran (exit 2)
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -836,3 +1052,14 @@ $(printf 'line\n%.0s' $(seq 1 80))
|
||||
assert_equal "$status" 2
|
||||
assert_output --partial "PyYAML is required"
|
||||
}
|
||||
|
||||
@test "exit 2: a crash inside the checks is the never-ran tier, not a verdict" {
|
||||
write_prompt review-pr 'description: Review a PR.' 'Review the PR.'
|
||||
local crashyaml="$TMPDIR/crashyaml"
|
||||
mkdir -p "$crashyaml"
|
||||
# Importable, so the preflight passes; safe_load raises what no check expects.
|
||||
printf 'class YAMLError(Exception):\n pass\ndef safe_load(_):\n raise RuntimeError("boom")\n' > "$crashyaml/yaml.py"
|
||||
PYTHONPATH="$crashyaml" run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_equal "$status" 2
|
||||
assert_output --partial "the prompt checks crashed (RuntimeError: boom)"
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user