fix(kyberforge): resolve clean-context audit findings on primitive support
primitive-author: - description excludes read-only review (-> factory-audit) - validation Gotcha now matches the research: compile never reads prompts, install fails only on a bad Copilot hook payload and warns on prompt input names and dropped keys - instruction fold-in into AGENTS.md/CLAUDE.md stated as conditional on dedup and --force-instructions - hook checklist gains the wrapped-shape Must, drops hardlinks, notes why executable is stricter than the research, and states the separate Copilot-targeted package route instead of a blanket "don't" - prompt Must 5 keeps the research's Copilot-only-key exception; adds model-slug and 250-char Shoulds; descriptions name skills or agents - placeholder instruction covers both FILL IN and FILL_IN_ tokens factory-audit: hardlink FAIL scoped to instructions and prompts (find_hook_files skips symlinks only), with bats cases; prompt-flow description rubric names skills or agents. forge: version-bump, apm-routes and sources references updated for the primitive route. Refs #94 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:
@@ -46,7 +46,7 @@ Cite file and line for every finding.
|
||||
|
||||
**description**
|
||||
|
||||
- SUGGESTION: the description does not read as one user-facing action, or does not name the skills the prompt steers. On Claude the description is model-visible and apm drops `disable-model-invocation`, so naming the steered skills keeps the router pointed at the capability rather than the wrapper.
|
||||
- SUGGESTION: the description does not read as one user-facing action, or does not name the skills or agents the prompt steers. On Claude the description is model-visible and apm drops `disable-model-invocation`, so naming what it steers keeps the router pointed at the capability rather than the wrapper.
|
||||
|
||||
Then return to `SKILL.md` Step 4, opening the report with this coverage line:
|
||||
|
||||
|
||||
@@ -99,12 +99,15 @@ def read_text(path):
|
||||
return None
|
||||
|
||||
|
||||
def check_not_linked():
|
||||
# apm's find_files_by_glob rejects symlinks and hardlinks (link count > 1),
|
||||
# so a linked file is silently never deployed.
|
||||
def check_not_linked(hardlinks=True):
|
||||
# apm's find_files_by_glob (instructions, prompts) rejects symlinks and
|
||||
# hardlinks (link count > 1); find_hook_files skips symlinks only, so hooks
|
||||
# pass hardlinks=False. A rejected file is silently never deployed.
|
||||
if os.path.islink(target):
|
||||
fail(f"is a symlink — apm's discovery skips symlinks, so it is never deployed — {fname}")
|
||||
return
|
||||
if not hardlinks:
|
||||
return
|
||||
try:
|
||||
if os.stat(target).st_nlink > 1:
|
||||
fail(f"is a hardlink (link count > 1) — apm's discovery rejects hardlinks, so it is never deployed — {fname}")
|
||||
@@ -231,7 +234,7 @@ def check_script(kind_, rel, first, pkg_root, where):
|
||||
|
||||
|
||||
def audit_hook():
|
||||
check_not_linked()
|
||||
check_not_linked(hardlinks=False)
|
||||
stem = fname[:-len('.json')]
|
||||
hooks_dir = os.path.basename(parent_dir)
|
||||
if hooks_dir != 'hooks':
|
||||
|
||||
@@ -165,6 +165,14 @@ teardown() {
|
||||
assert_output --partial "is a symlink"
|
||||
}
|
||||
|
||||
@test "hook: a hardlinked hook file is not a FAIL (find_hook_files skips symlinks only)" {
|
||||
write_hook real.json '{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"true"}]}]}}'
|
||||
ln "$PKG/.apm/hooks/real.json" "$PKG/.apm/hooks/linked.json"
|
||||
run bash "$SCRIPT" "$PKG/.apm/hooks/linked.json"
|
||||
assert_success
|
||||
refute_output --partial "hardlink"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Instructions
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -227,6 +235,15 @@ applyTo: "**/*.py"' 'body'
|
||||
assert_output --partial "also exists at the package root"
|
||||
}
|
||||
|
||||
@test "instruction: a hardlinked instruction file is a FAIL" {
|
||||
write_instruction python 'description: x
|
||||
applyTo: "**/*.py"' 'body'
|
||||
ln "$PKG/.apm/instructions/python.instructions.md" "$TMPDIR/python.instructions.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/instructions/python.instructions.md"
|
||||
assert_failure
|
||||
assert_output --partial "is a hardlink"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Prompts
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -314,3 +331,11 @@ $(printf 'line\n%.0s' $(seq 1 80))
|
||||
assert_success
|
||||
refute_output --partial "SUGGESTION"
|
||||
}
|
||||
|
||||
@test "prompt: a hardlinked prompt file is a FAIL" {
|
||||
write_prompt review-pr 'description: Review a pull request with gitea-prs.' 'Review the PR with gitea-prs.'
|
||||
ln "$PKG/.apm/prompts/review-pr.prompt.md" "$TMPDIR/review-pr.prompt.md"
|
||||
run bash "$SCRIPT" "$PKG/.apm/prompts/review-pr.prompt.md"
|
||||
assert_failure
|
||||
assert_output --partial "is a hardlink"
|
||||
}
|
||||
|
||||
@@ -21,7 +21,7 @@ checkpoints to the user in real time.
|
||||
|
||||
## No clean-context recheck, and no automatic audit
|
||||
|
||||
Skill and agent routes close with a clean-context audit rerun; these two do not, and the omission
|
||||
Skill, agent and primitive routes close with a clean-context audit rerun; these two do not, and the omission
|
||||
is deliberate rather than an oversight. Neither artifact type has an audit skill counterpart to
|
||||
re-run, so detaching the route to earn a recheck it would never get buys nothing.
|
||||
|
||||
|
||||
@@ -28,7 +28,7 @@
|
||||
|
||||
- **URL:** https://agentskills.io/specification.md
|
||||
- **Research doc:** plugins/kyberforge/docs/research/docs/agentskillsio/sources.md
|
||||
- **Description:** Complete SKILL.md format specification — confirms `assets/`, `references/`, and `scripts/` are warranted only by the bulk/reusability of supporting content (large reference material, executable code, templates), not by a skill's category, and forge's per-route procedures are that kind of supporting content — so the spec permits the `references/` split here but does not require it. The warrant is a house decision: dispatch is mandatory at two or more mutually exclusive flows, which forge's four-row table is. `references/sources.md` likewise exists for this repo's own provenance-chain convention (see `CONTEXT.md`), not because the spec requires it.
|
||||
- **Description:** Complete SKILL.md format specification — confirms `assets/`, `references/`, and `scripts/` are warranted only by the bulk/reusability of supporting content (large reference material, executable code, templates), not by a skill's category, and forge's per-route procedures are that kind of supporting content — so the spec permits the `references/` split here but does not require it. The warrant is a house decision: dispatch is mandatory at two or more mutually exclusive flows, which forge's five-row table is. `references/sources.md` likewise exists for this repo's own provenance-chain convention (see `CONTEXT.md`), not because the spec requires it.
|
||||
- **Contributing files:** SKILL.md
|
||||
- **Status:** `extracted`
|
||||
|
||||
|
||||
@@ -7,8 +7,9 @@ source_keys:
|
||||
|
||||
Reached from `SKILL.md` Step 3 after a route has finished. A skill route always lands here:
|
||||
`skill-author` moves only a skill's own `metadata.version`, which is not the package manifest's
|
||||
number, so the package version is still behind when it reports done. `agent-author` bumps the
|
||||
resolved package's `apm.yml` itself at plugin/APM scope, and `apm-workflow`'s configure flow
|
||||
number, so the package version is still behind when it reports done. `agent-author` and
|
||||
`primitive-author` bump the resolved package's `apm.yml` themselves at plugin/APM scope, and
|
||||
`apm-workflow`'s configure flow
|
||||
carries the same policy — read those routes' output before acting here, because a second bump for
|
||||
one change is wrong.
|
||||
|
||||
|
||||
@@ -2,8 +2,8 @@
|
||||
name: primitive-author
|
||||
description: >
|
||||
Use when the user wants an apm hook, instruction or prompt file created, or
|
||||
audit findings or feedback applied to an existing one.
|
||||
Not skills -> skill-author. Not agents -> agent-author.
|
||||
audit findings or feedback applied to an existing one. Not read-only
|
||||
review -> factory-audit. Not skills -> skill-author. Not agents -> agent-author.
|
||||
allowed-tools: Bash Read Write Edit
|
||||
metadata:
|
||||
version: "0.1.0"
|
||||
@@ -15,8 +15,8 @@ metadata:
|
||||
|
||||
## Gotchas
|
||||
|
||||
- `apm compile --validate` is not a gate. apm turns every instruction and prompt problem into a warning and exits 0, and `apm install` never validates at all — `/factory-audit` is the only check that fails.
|
||||
- Never draft with the real suffix outside `.apm/<type>/`. apm's local discovery globs `**/*.instructions.md` across the whole tree, so a draft or template named that way anywhere in the repo compiles into `AGENTS.md`. The templates carry a trailing `.template` for this reason; drop it only on the final path.
|
||||
- `apm compile --validate` is not a gate: it reports instruction problems only as warnings, exits 0, and never reads prompts. `apm install` fails only on a hook payload Copilot would reject, and merely warns on bad prompt input names and dropped keys — `/factory-audit` is the only check that fails on the rest.
|
||||
- Never draft with the real suffix outside `.apm/<type>/`. apm's local discovery globs `**/*.instructions.md` across the whole tree, so a draft or template named that way anywhere in the repo is picked up as a real instruction. The templates carry a trailing `.template` for this reason; drop it only on the final path.
|
||||
- Never hand-write `.claude/settings.json`, even to test a hook. apm owns that file (ADR-0019), overwrites it outright when it is malformed, and `apm audit --ci` fails on anything it would not have written.
|
||||
|
||||
## Step 1 — Dispatch
|
||||
@@ -38,7 +38,7 @@ Run the reference's **Gate** section before writing anything. A failed gate stop
|
||||
|
||||
| Condition | Action |
|
||||
|---|---|
|
||||
| No file at the target path | Create: copy the reference's template from `assets/templates/`, drop `.template`, fill every `FILL IN`, and apply the reference's checklist |
|
||||
| No file at the target path | Create: copy the reference's template from `assets/templates/`, drop `.template`, fill every `FILL IN` and `FILL_IN_` placeholder, and apply the reference's checklist |
|
||||
| File exists, at least one signal | Improve: read the whole file, then apply each signal against the reference's checklist |
|
||||
| File exists, no signal | Stop and ask whether the user meant a new file or has feedback to apply |
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
---
|
||||
description: "FILL IN: one user-facing action naming the skills it steers"
|
||||
description: "FILL IN: one user-facing action naming the skills or agents it steers"
|
||||
input:
|
||||
- FILL_IN_name: "FILL IN: what the user supplies"
|
||||
---
|
||||
|
||||
@@ -50,33 +50,38 @@ target:
|
||||
`${CLAUDE_PLUGIN_ROOT}` also works but ties the source to one harness's name.
|
||||
- **Claude is the verified target.** apm 0.28.0 passes this nested shape to Copilot without
|
||||
reshaping it, and whether Copilot CLI runs nested entries or honours `matcher` is unverified. That
|
||||
gap is apm's to close — do not work around it with a second, Copilot-flat file.
|
||||
gap is apm's to close. Per-file target routing is deprecated, so a Copilot-native flat hook
|
||||
(`bash` / `powershell` / `timeoutSec`) can only live in a separate Copilot-targeted package — hand
|
||||
that to `apm-workflow` rather than adding a second file here.
|
||||
|
||||
## Checklist
|
||||
|
||||
Must:
|
||||
|
||||
1. The file sits directly in `.apm/hooks/`, is not a symlink or hardlink, and parses as a JSON
|
||||
object. apm skips invalid JSON silently.
|
||||
2. Every event value is a list of objects, and every nested `hooks` is a list of objects. Anything
|
||||
1. The file sits directly in `.apm/hooks/`, is not a symlink, and parses as a JSON object. apm
|
||||
skips invalid JSON silently.
|
||||
2. Use the wrapped shape `{"hooks": {Event: [...]}}`. If a naked settings slice is used instead,
|
||||
every top-level value must be a list, with no stray scalar keys anywhere.
|
||||
3. Every event value is a list of objects, and every nested `hooks` is a list of objects. Anything
|
||||
else fails the Copilot install outright.
|
||||
3. Event names are PascalCase (`PreToolUse`, `PostToolUse`, `UserPromptSubmit`, `SessionStart`,
|
||||
4. Event names are PascalCase (`PreToolUse`, `PostToolUse`, `UserPromptSubmit`, `SessionStart`,
|
||||
`Stop`, …). An all-lowercase name (`stop`) never warns and never fires; a camelCase name outside
|
||||
apm's rename map (`userPromptSubmit`) deploys verbatim to Claude and never fires.
|
||||
4. The script is referenced as `${PLUGIN_ROOT}/…` (package root) or `./…` (hook directory),
|
||||
5. The script is referenced as `${PLUGIN_ROOT}/…` (package root) or `./…` (hook directory),
|
||||
exists inside the package, and is executable. No absolute path, and no `$` or backtick in the
|
||||
path itself. A missing script is only a warning at install time.
|
||||
5. No `hooks-<target>` or `*-<target>-hooks` filename. That routing is deprecated; reach belongs to
|
||||
path itself. A missing script is only a warning at install time. Executable is stricter than the
|
||||
research's Should: a script invoked as the command's first token fails at runtime without it.
|
||||
6. No `hooks-<target>` or `*-<target>-hooks` filename. That routing is deprecated; reach belongs to
|
||||
`targets:` (see Gate).
|
||||
|
||||
Should:
|
||||
|
||||
6. Every handler sets `"type": "command"` and an explicit `timeout` in seconds — apm passes `type`
|
||||
7. Every handler sets `"type": "command"` and an explicit `timeout` in seconds — apm passes `type`
|
||||
through but never supplies it.
|
||||
7. Set `matcher` explicitly on tool events and on `SessionStart` (`startup`, `resume`, …). Omitted,
|
||||
8. Set `matcher` explicitly on tool events and on `SessionStart` (`startup`, `resume`, …). Omitted,
|
||||
Claude receives `"*"`.
|
||||
8. Do not author Copilot's flat `bash` / `powershell` / `timeoutSec` keys in a Claude-shaped file;
|
||||
9. Do not author Copilot's flat `bash` / `powershell` / `timeoutSec` keys in a Claude-shaped file;
|
||||
they render onto Claude as stray keys.
|
||||
9. Quote a script path that may contain spaces: `"${PLUGIN_ROOT}/scripts/my hook.sh"`.
|
||||
10. Keep helper files in the hook directory non-JSON. Copilot's loader rejects any bundled `.json`
|
||||
10. Quote a script path that may contain spaces: `"${PLUGIN_ROOT}/scripts/my hook.sh"`.
|
||||
11. Keep helper files in the hook directory non-JSON. Copilot's loader rejects any bundled `.json`
|
||||
without a `hooks` key.
|
||||
|
||||
@@ -17,8 +17,9 @@ glob. On Claude it deploys to `.claude/rules/<stem>.md` with `applyTo` renamed t
|
||||
- **A rule for this repo alone** → it belongs in the repo's AGENTS.md, which is the single
|
||||
always-on source. Stop and hand to `agentsmd-author`.
|
||||
- **No file pattern fits** → an instruction without `applyTo` is always-on in every session of
|
||||
every repo that installs this package, and `apm compile` folds it into the global sections of
|
||||
`AGENTS.md` and `CLAUDE.md`. Say exactly that to the user and continue only on an explicit yes.
|
||||
every repo that installs this package, and `apm compile` can fold it into the global sections of
|
||||
`AGENTS.md` and `CLAUDE.md` (skipped when `.github/instructions/` or `.claude/rules/` is already
|
||||
populated, unless `--force-instructions`). Say exactly that to the user and continue only on an explicit yes.
|
||||
Legitimate when a package deliberately ships guidance to its consumers; never a default.
|
||||
- **Procedure the agent follows step by step** → a skill. Stop and hand to `skill-author`.
|
||||
- **A rule scoped to a file pattern** → continue.
|
||||
@@ -32,7 +33,7 @@ Must:
|
||||
1. The path is `.apm/instructions/<stem>.instructions.md`, directly in that directory, not a
|
||||
symlink or hardlink.
|
||||
2. `description` is a non-empty string. apm only warns when it is missing.
|
||||
3. The body is non-empty. apm deploys an empty rule silently.
|
||||
3. The body is non-empty after trimming whitespace. apm deploys an empty rule silently.
|
||||
4. `applyTo` is a non-empty glob or comma-separated list — top-level commas only as separators,
|
||||
alternation inside `{}` (`"**/*.{ts,tsx}"`) — or absent after the Gate's explicit yes.
|
||||
5. The stem is unique across the package and its dependencies: a `.claude/rules/<stem>.md`
|
||||
|
||||
@@ -23,15 +23,16 @@ worse skill on every harness.
|
||||
afterwards, come back and write it against the new skill.
|
||||
- **A single-intent message the user would otherwise type repeatedly, steering existing skills or
|
||||
agents by name** → continue. Confirm each skill or agent it names exists and is not
|
||||
`disable-model-invocation: true`, which the model cannot invoke.
|
||||
`disable-model-invocation: true`: the prompt's body reaches the model, and the model cannot
|
||||
invoke a skill that sets it, so the steering would dead-end (the same check `factory-audit`'s
|
||||
prompt flow applies).
|
||||
|
||||
## Description contract
|
||||
|
||||
One plain, user-facing sentence stating the action and naming the skills it steers — "Review the
|
||||
One plain, user-facing sentence stating the action and naming the skills or agents it steers — "Review the
|
||||
current PR with `gitea-prs` and `factory-audit`, then summarise the findings." No "Use when"
|
||||
trigger clause and no `Not X -> Y` boundary: on Claude the description is model-visible, and a
|
||||
trigger clause invites the router to pick the wrapper over the skills it wraps. 250 characters at
|
||||
most.
|
||||
trigger clause invites the router to pick the wrapper over the skills it wraps.
|
||||
|
||||
## Checklist
|
||||
|
||||
@@ -50,10 +51,14 @@ Must:
|
||||
4. Every `${input:x}` in the body is declared in `input:`, and every declared name is used. Without
|
||||
`input:`, no `${input:…}` may appear — it would reach Claude unrewritten.
|
||||
5. Frontmatter keys stay within `description`, `allowed-tools`, `model`, `argument-hint` and
|
||||
`input`. Claude drops everything else with only a warning.
|
||||
`input`. Claude drops everything else with only a warning. The one exception: a Copilot-only key
|
||||
(`agent`, `tools`, …) that is intended, with its Claude drop accepted and said so.
|
||||
|
||||
Should:
|
||||
|
||||
6. Spell keys in kebab-case — `allowed-tools`, `argument-hint` — not the camelCase aliases.
|
||||
7. Omit `argument-hint` when `input:` is set; apm synthesises `<a> <b>` from the input names.
|
||||
8. Keep one intent per prompt, and write the body as second-person instructions.
|
||||
9. Keep `description` to 250 characters or fewer.
|
||||
10. Give `model` a slug the target accepts. Copilot ignores `allowed-tools` and `model`, so neither
|
||||
constrains a Copilot run.
|
||||
|
||||
Reference in New Issue
Block a user