fix(kyberforge): resolve PR #144 review and audit round 3
- factory-audit: hook events judged per deployed target after apm's rename (Claude/Copilot event sets FAIL, others SUGGESTION); Claude plugin layouts accepted as hook sources; interpreter options and sh -c strings checked; bats 378 -> 386 - primitive-author: Must 4/5 match the audit; reference hand-back points at the right steps; Step 4.2 --target all fallback - skill-author: new-skill.sh repair only on the template marker line, so complete skills stay a no-op; provenance and calibration text - forge: restore "already named" qualifier; drop false HITL claim - apm-workflow: token example uses an env var - docs/hooks.md: the apm-hooks.json sidecar is committed, not ignored 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:
@@ -2,6 +2,8 @@
|
||||
source_keys:
|
||||
- apm-cli-installed-source
|
||||
- apm-docs-llms-full
|
||||
- claude-code-hooks-reference
|
||||
- github-copilot-hooks-configuration
|
||||
---
|
||||
|
||||
# Hook Flow
|
||||
@@ -11,7 +13,7 @@ Steps 1 to 3 for an apm hook — the target Step 0 matched as a `.json` file dir
|
||||
|
||||
## Gotchas
|
||||
|
||||
- apm checks almost nothing here. Invalid JSON is skipped without a word, an all-lowercase event deploys verbatim and never fires on every target but Kiro (whose map alone renames `stop`), and a missing script only warns — so `apm install` exiting 0 says nothing about whether the hook works. Never cite a clean install as evidence against a finding.
|
||||
- apm checks almost nothing here. Invalid JSON is skipped without a word, an event name its rename map does not cover deploys verbatim with no warning (a lowercase `stop` or a typo such as `PreToolUSe` never fires on Claude or Copilot), and a missing script only warns — so `apm install` exiting 0 says nothing about whether the hook works. Never cite a clean install as evidence against a finding.
|
||||
- Copilot receiving a Claude-shaped file is not a finding. apm renders one source for every target and documents that it owns the per-target shape; whether Copilot CLI honours a nested entry or `matcher` is unverified upstream, not a defect in the file.
|
||||
- Quoting is not the fix for a script path with a space. apm rewrites a whole-token-quoted `"${PLUGIN_ROOT}/scripts/x.sh"`, but still stops reading the path at the space; the only fix is a path without one.
|
||||
|
||||
@@ -23,13 +25,15 @@ Resolve the path against this skill's own directory. Run exactly:
|
||||
bash scripts/validate.sh <hook-file>
|
||||
```
|
||||
|
||||
Its findings become the `### Structure` dimension, FAILs and SUGGESTIONs both, at the tier the script assigned: JSON validity, the wrapped-or-naked shape, event lists and nested handler lists (the checks whose failure makes the Copilot install fail), a file contributing no entries (no events, only empty event lists, or an entry with no handler), event names that never fire, unfilled `FILL IN` or `FILL_IN_` template placeholders, a file under apm's deployed output rather than package source, referenced scripts that are missing, outside the package, not executable when run directly, or referenced by an absolute, bare relative, `../`, split-quoted or space-containing path apm will not bundle correctly, deprecated filename routing, and `${CLAUDE_PLUGIN_ROOT}` where `${PLUGIN_ROOT}` would do. Script references are read with apm 0.28.0's own patterns: `${PLUGIN_ROOT}/…` only when the path follows the token directly, up to the first whitespace or quote, and `./…` anywhere in the command. A `./` or `../` match is held to the script rules — a FAIL when missing — only in command position (the first token, or the first argument after `bash`, `sh`, `zsh`, `python`, `python3`, `node`, `pwsh`, `ruby` or `perl`), when it ends in a script extension, or when it names a package entry that is not a file; any other match (`npx prettier --check ./src`, `printf '.\n'`) is at most a SUGGESTION, because apm only warns and it runs against the consumer's working directory as meant. Absolute and bare relative script paths are checked in the same command positions. An all-lowercase event FAILs only when no target the package deploys to renames it, and is a SUGGESTION when only some do. A camelCase event outside Claude's rename map FAILs in a Claude-shaped file, and in a flat Copilot-shaped file too whenever the nearest `apm.yml` above the file targets Claude — no `target:`/`targets:` means every target. It exits **0** with no FAIL, **1** on real findings, **2** when it never ran — report that as `### Structure` unverified, quoting the stderr reason. An INFO line is observational: report it under `### Structure` and count it in `· P info`.
|
||||
Its findings become the `### Structure` dimension, FAILs and SUGGESTIONs both, at the tier the script assigned: JSON validity, the wrapped-or-naked shape, event lists and nested handler lists (the checks whose failure makes the Copilot install fail), a file contributing no entries (no events, only empty event lists, or an entry with no handler), event names that never fire, unfilled `FILL IN` or `FILL_IN_` template placeholders, a symlinked file or one under apm's deployed output rather than package source, referenced scripts that are missing, outside the package, not executable when run directly, or referenced by an absolute, bare relative, `../`, split-quoted, space-containing, or `$`/backtick-containing path apm will not bundle correctly, deprecated filename routing, and `${CLAUDE_PLUGIN_ROOT}` where `${PLUGIN_ROOT}` would do. Script references are read with apm 0.28.0's own patterns: `${PLUGIN_ROOT}/…` only when the path follows the token directly, up to the first whitespace or quote, and `./…` anywhere in the command. A `./` or `../` match is held to the script rules — a FAIL when missing — only in command position (the first token, or the first operand after `bash`, `sh`, `zsh`, `python`, `python3`, `node`, `pwsh`, `ruby` or `perl`, past its options such as `-e` or `-u`; after a `sh`-family `-c`, the first token of the command string; inline code such as `python3 -c` has none), when it ends in a script extension, or when it names a package entry that is not a file; any other match (`npx prettier --check ./src`, `printf '.\n'`) is at most a SUGGESTION, because apm only warns and it runs against the consumer's working directory as meant. Absolute and bare relative script paths are checked in the same command positions. Each event is judged per target the package root's `apm.yml` deploys to — no `target:`/`targets:`, `all`, or no `apm.yml` means every hook target — after apm's rename map for that target: it FAILs when a target with a published event list (Claude, Copilot) does not fire the renamed name, whatever its casing. It exits **0** with no FAIL, **1** on real findings, **2** when it never ran — report that as `### Structure` unverified, quoting the stderr reason. An INFO line is observational: report it under `### Structure` and count it in `· P info`.
|
||||
|
||||
There is no provenance and no Vale step: a hook carries no `source_keys` and no prose.
|
||||
|
||||
Three tiers deliberately differ from `primitive-author`'s checklist or the research's. Do not re-tier them by judgment:
|
||||
Five tiers deliberately differ from `primitive-author`'s checklist or the research's. Do not re-tier them by judgment:
|
||||
|
||||
- A hook file directly under a package-root `hooks/` — beside the package's `apm.yml` — passes. apm discovers both `.apm/hooks/` and `hooks/`, and this audit may target a third-party package; `primitive-author` authors only in `.apm/hooks/`. Any other `hooks/` directory (`.github/hooks/`, `.cursor/hooks/`, …) is apm's deployed output and FAILs.
|
||||
- A hook file directly under a package-root `hooks/` — beside the package's `apm.yml`, or beside a `plugin.json` at any location apm's `find_plugin_json` reads (root, `.github/plugin/`, `.claude-plugin/`, `.cursor-plugin/`) — passes. apm discovers both `.apm/hooks/` and `hooks/`, installs a Claude plugin with no `apm.yml`, and this audit may target a third-party package; `primitive-author` authors only in `.apm/hooks/`. Any other `hooks/` directory (`.github/hooks/`, `.cursor/hooks/`, …) is apm's deployed output and FAILs.
|
||||
- An event that every listed target fires, but that reaches a target with no published event list (Cursor, Kiro, Gemini, Codex, Antigravity, Windsurf) in a non-PascalCase form after apm's rename, is a SUGGESTION, not the FAIL Must 4 implies: the script cannot tell a harness's native spelling (Cursor's `stop`, Windsurf's `pre_run_command`) from a typo. A Cursor-only `stop` therefore exits 0.
|
||||
- Copilot counts every name apm's own Copilot map emits as fired (`userPromptSubmit`, although Copilot documents `userPromptSubmitted`): the author cannot route around apm's rename, so that is not a finding in the file.
|
||||
- Deprecated filename routing is a SUGGESTION, matching the author's Should: the research allows it when deprecated routing is intended.
|
||||
- A non-executable script run as the command's first token is a FAIL, stricter than the research's Should, because it fails every time it fires.
|
||||
|
||||
@@ -44,13 +48,13 @@ Cite file and line for every finding.
|
||||
**purpose** — apm's own rule is to reach for a skill, instruction or prompt first; a hook is for "this must always happen at this event".
|
||||
|
||||
- FAIL: the script carries procedure the agent should follow — instructions printed to the model, a multi-step workflow — rather than a runtime callback. That is a skill.
|
||||
- SUGGESTION: the behaviour is harness-specific (a Claude-only event, a Claude-only matcher value) in a package whose `targets:` includes other harnesses, and nothing records that the other targets receiving it was accepted. The apm-native fix is a separate package with its own `targets:`, not a routing filename.
|
||||
- SUGGESTION: the behaviour is harness-specific (a Claude-only event reaching a harness the script has no event list for, a Claude-only matcher value) in a package whose `targets:` includes other harnesses, and nothing records that the other targets receiving it was accepted. The apm-native fix is a separate package with its own `targets:`, not a routing filename.
|
||||
|
||||
**handlers** — the research checklist's Should and audit-only items, which apm never checks:
|
||||
|
||||
- SUGGESTION: a handler without `"type": "command"` or an explicit numeric `timeout` in seconds.
|
||||
- SUGGESTION: a tool event (`PreToolUse`, `PostToolUse`) or `SessionStart` with no `matcher` — Claude receives `"*"`. A `matcher` on an event Claude ignores it for (`Stop`, `UserPromptSubmit`) is inert, not wrong.
|
||||
- SUGGESTION: a PascalCase event name that is not a real Claude Code event (a misspelling deploys verbatim and never fires; the script cannot tell a typo from an event it does not know).
|
||||
- SUGGESTION: a PascalCase event, in a package reaching only harnesses with no published event list, that is not one of that harness's events — the script FAILs a misspelling only where it has the list (Claude, Copilot).
|
||||
- SUGGESTION: `bash`/`powershell`/`timeoutSec` keys in a Claude-shaped file — they render, but leave stray keys in `settings.json`.
|
||||
- SUGGESTION: a helper `.json` file in the hook directory without a `hooks` key — Copilot's loader scans the bundled scripts directory and rejects it. Keep helper configuration non-JSON.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user