refactor(kyberforge)!: merge skill-audit and agent-audit into factory-audit
Why The two audit skills carried 1,724 lines of byte-identical duplication: the ADR-0020 boundary resolver (1,061), vale-wrap.sh (526), the Vale style rules (44) and the Contributing-files parser (93). Nothing shared them — they were held in sync by a 413-line pre-push gate and its 797-line test suite. Sync-by-gate had already failed once: at484357athe two parser copies drifted into different spellings of the bullet loop while a docstring asserted they were identical. That drift was behaviour-neutral and was re-unified by hand at598a7c3, so the copies were identical at merge time — but nothing had caught it, and the next drift need not be neutral. Implementation Notes Self-containment binds BETWEEN skills, not within one. The agentskills.io spec forbids reaching across skill directories, which is why two separate skills needed embedded copies; two files inside ONE skill may source a third. That is the whole reason the merge removes duplication rather than relocating it. The union of both bodies measured 1,532 words against BODY_MAX_WORDS=900, and only 211 of those words were shared, so SKILL.md is a dispatch body. Step 0 resolves the flow from the target path before any validation, and its table mirrors validate.sh's detection exactly: a directory holding SKILL.md or a SKILL.md file (skill); a *.agent.md, or a .md directly under an agents/ directory (agent); anything else stops without running a validator. Steps 1-3 live in references/skill-flow.md and references/agent-flow.md, and gotchas that apply to one flow live in that flow's file, since it is loaded on every invocation anyway. If validate.sh reports on the other artifact type, the body restarts at Step 0. Named factory-audit rather than forge-audit because forge is a live skill, and a family prefix that matches a live sibling reads as ownership rather than membership. The description carries one arrow per boundary target, because ADR-0020 resolves only the first target after an arrow. It drops the quoted "audit this skill"-style phrases, which restated "audited" in a second register (ADR-0020's duplicate-register rule). 241 characters, Gotchas 16% of the body: no size SUGGESTIONs. The boundary resolver stays embedded in two files rather than imported: a cache-installed plugin cannot read outside its own directory, and the repo-root hook resolves via .pre-commit-hooks.yaml where entry[0] is the only token pre-commit rewrites, so no single file is reachable by both. tests/test-adr0020-contract.sh hashes both copies for byte-identity, and asserts validate.sh sources the resolver and that no third copy exists. The entry scripts classify the target from its resolved parent directory, so a bare agent filename typed inside agents/ works; resolve SCRIPT_DIR CDPATH-safely; and exit 2 when a lib-*.sh is missing, rather than dying with exit 1, the tier the flows relay as real findings. The provenance run functions stash their findings code in KYBERFORGE_PROV_RC and return 0, so validate-provenance.sh calls them UNTESTED. Testing a function's status (`f || RC=$?`) disables errexit for its entire body, and no subshell or `set -e` inside can re-arm it once the call sits in a condition context (measured, both spellings). Their error paths use `exit`, which is unaffected either way; this keeps errexit armed for anything added later. Case 0's readability guard reads the file instead of asking `[[ -r ]]`. `-r` is access(2), which answers yes for uid 0 even on a mode-000 file, and this repo's dev environment is root -- so the guard could never fire where it exists to fire. A read attempt is also the stricter question, catching EIO. This is the reasoning scripts/check-vale-style-sync.sh carried before this commit deleted it; the hazard did not go with it. All three entry scripts are CDPATH-safe, vale-wrap.sh included: both of its cd sites are cleared, the --config resolution and the directory-mirror walk, where an exported CDPATH would otherwise print a decoy path into the -print0 stream and build the mirror from the decoy's files. The two remaining bare cd calls take absolute paths, which CDPATH is never consulted for. Impact BREAKING: skill-audit and agent-audit no longer exist as invocable skills. kyberforge goes to 2.0.0 (catalog 0.4.7). Check logic is unchanged: differential runs of the old and new validators across every skill and agent produced byte-identical stdout, stderr and exit codes, and the reconstructed Python payloads differ only in comments and the references/field-inventory.md -> agent-field-inventory.md rename. One doctrine governs the tiers: exit 0 is audited and clean, exit 1 is audited with findings OR a target present but unreadable, exit 2 is that nothing was audited at all. Edge paths DID change, deliberately (full table in ADR-0025): - a missing target exits 2 (never ran), not 1, under its own "does not exist" message; detection is by path shape, so a shape-matching path that is simply absent used to reach the validator and come back as a FAIL against a file that never existed; - an unshaped target exits 2 under the generic "matches neither" message, and a directory with no SKILL.md under a third, distinct one -- three exit-2 messages, not one; - a dangling symlink or a symlink loop stays exit 1: it is present but broken, which is a finding about the artifact rather than a usage error; - a SKILL.md file path is audited as its skill directory instead of refused; - a .md agent outside an agents/ directory is refused rather than audited; - a missing script library, a missing python3, a missing PyYAML, and no argument at all each exit 2. validate-provenance.sh already exited 2 for the last two; validate.sh now matches it. .pre-commit-hooks.yaml is a published contract consumed by external repos. Both hook IDs and both files: regexes are unchanged; only entry: and description: moved. scripts/check-vale-style-sync.sh (413), scripts/sync-vale-styles.sh (21), tests/test-check-vale-style-sync.sh (797) and agent-audit/scripts/README.md (47) are deleted. The checker made 17 assertions: 6 compared the two Vale copies and are moot; 10 are rehomed into tests/test-vale-wrap.sh (case 0, cases 28-31, and the suite's Vale-absent skip); and the cross-manifest files: agreement check, which selected hooks by entry: and so could not survive both hooks sharing one, is ported as case 33 pairing hooks by id:. Cases 28, 30 and 33 carry mutation self-tests; narrowing the local skill prefilter to 6 of 38 SKILL.md files now fails the suite. Skills go 39 to 38. Pre-push goes 9 repo-authored hooks to 8. ADR: 0025 BREAKING-CHANGE: the skill-audit and agent-audit skills are removed. Both flows are served by factory-audit, which auto-detects whether it was handed a skill directory or an agent file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD
This commit is contained in:
@@ -1,5 +1,10 @@
|
||||
# Add INFO as a third finding level in skill-audit reports
|
||||
|
||||
**Skill renamed per ADR-0025 (2026-09-15):** `skill-audit` and `agent-audit` merged into
|
||||
`factory-audit`, which dispatches to a skill flow and an agent flow at Step 0. Read `skill-audit`
|
||||
below as `factory-audit`'s skill flow. The decision itself is unchanged — ADR-0025 carried every
|
||||
audit criterion, tier and finding level across as-is.
|
||||
|
||||
`skill-audit` shipped with two finding levels: FAIL (blocks shipping) and
|
||||
SUGGESTION (optional improvement). Provenance validation introduced observations
|
||||
that are worth surfacing but not actionable: a `references/*.md` file with no
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
# agent-audit takes a single file path and derives the counterpart by scope detection
|
||||
|
||||
**Skill renamed per ADR-0025 (2026-09-15):** `agent-audit` merged with `skill-audit` into
|
||||
`factory-audit`. Read `agent-audit` below as `factory-audit`'s agent flow. The single-file
|
||||
invocation contract this ADR sets survives the merge intact — `factory-audit` dispatches at Step 0
|
||||
on the target path, and an `*.agent.md` or a path under `.apm/agents/` takes the agent flow, so the
|
||||
caller still names one file and the script still derives the rest.
|
||||
|
||||
`agent-audit` validates agent definition file pairs (Claude Code `.md` + Copilot `.agent.md`). The skill accepts a path to either file and derives the counterpart using scope detection rather than requiring the caller to name both files or supply a root directory.
|
||||
|
||||
## Considered options
|
||||
|
||||
@@ -1,5 +1,10 @@
|
||||
# agent-audit reads field lists from a reference file, not hardcoded script arrays
|
||||
|
||||
**Skill renamed per ADR-0025 (2026-09-15):** `agent-audit` merged with `skill-audit` into
|
||||
`factory-audit`. Read `agent-audit` below as `factory-audit`'s agent flow; the reference file this
|
||||
ADR is about is now `factory-audit/references/agent-field-inventory.md`. The decision is unchanged —
|
||||
the field lists still live in a reference file read at runtime, not in script arrays.
|
||||
|
||||
`agent-audit`'s `validate.sh` checks for Claude Code-only fields in Copilot files and
|
||||
silently-ignored fields in plugin agents. Rather than hardcoding those field lists in the
|
||||
script, the script reads `references/field-inventory.md` at runtime. This keeps field list
|
||||
|
||||
@@ -15,6 +15,9 @@ ADR's own conclusion is unaffected by that move: the provenance file still belon
|
||||
auto-scans, and `.apm/agents/` is, if anything, further removed from plugin-root than the old
|
||||
flat `agents/` directory was, so the reasoning below still holds. References below to
|
||||
`<plugin-root>/agents/` describe the pre-APM layout in effect when this decision was made.
|
||||
**Skill renamed per ADR-0025 (2026-09-15):** `agent-audit` merged with `skill-audit` into
|
||||
`factory-audit`; read the `agent-audit` references below as `factory-audit`'s agent flow, whose
|
||||
`validate-provenance.sh` still resolves `<plugin-root>/sources.md` exactly as this ADR decided.
|
||||
**Scope boundary (per ADR-0016):** this path change is plugin scope only. Project scope
|
||||
(`.claude/agents/` + `.github/agents/`) and user scope (`~/.claude/agents/` +
|
||||
`~/.copilot/agents/`) are unaffected — they are not APM packages and keep the dual-file
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
|
||||
Three skills in the `core` plugin (`core`'s first active skills):
|
||||
|
||||
- **`agentsmd-author`** — creates/updates a target repo's `AGENTS.md`, including nested monorepo placement (nearest-file-wins). Closes out by invoking `agentsmd-audit` inline, mirroring the `skill-author`/`skill-audit` pattern. When it detects an existing provider-specific file (`CLAUDE.md`, etc.) with content that duplicates what AGENTS.md should own, it calls `provider-adapter-author` via skill composition.
|
||||
- **`agentsmd-author`** — creates/updates a target repo's `AGENTS.md`, including nested monorepo placement (nearest-file-wins). Closes out by invoking `agentsmd-audit` inline, mirroring the `skill-author`/`skill-audit` pattern (**skill renamed per ADR-0025, 2026-09-15:** `skill-audit` is now `factory-audit`'s skill flow; the author-then-audit pattern is unchanged). When it detects an existing provider-specific file (`CLAUDE.md`, etc.) with content that duplicates what AGENTS.md should own, it calls `provider-adapter-author` via skill composition.
|
||||
- **`agentsmd-audit`** — a single combined pass checking three mandatory baselines against `AGENTS.md` only: secrets/credentials (governance.md hard prohibition), structural completeness (common-sections checklist from the agents.md spec), and accuracy/drift (do referenced commands/paths resolve against the repo). Never inspects provider adapter files.
|
||||
- **`provider-adapter-author`** — detects and converts a provider-specific instruction file into a thin adapter that imports `AGENTS.md` (mirroring this repo's own two-tier `CLAUDE.md` pattern). Self-validates via its own bundled deterministic script (`scripts/validate-adapter.sh`) rather than a separate paired audit skill, since the check (import present, no duplicated headings, size threshold) is mechanical.
|
||||
|
||||
|
||||
@@ -109,7 +109,8 @@ every rule to `level: error` is what actually implements this decision.
|
||||
1.81 tokens per word, so a worst-case `SKILL.md` at the ceiling still lands under 5,000 tokens —
|
||||
`wc -w` is not BPE tokenization). Either one exceeded fails the hook. Both are
|
||||
inclusive: a file at exactly 500 lines or exactly 2,770 words passes, and only one past a ceiling
|
||||
fails. `skill-audit/scripts/validate.sh` enforces the same pair on the same inclusive terms, so
|
||||
fails. `skill-audit/scripts/validate.sh` (now `factory-audit/scripts/validate.sh`, see ADR-0025)
|
||||
enforces the same pair on the same inclusive terms, so
|
||||
the audit and the commit hook cannot disagree about whether a given `SKILL.md` is over size.
|
||||
- `styles/KyberforgeTrial/` and `.vale.trial.ini` were deliberately not created — noted here so a
|
||||
future reader doesn't wonder if a trial tier was forgotten.
|
||||
|
||||
@@ -5,6 +5,28 @@ out of the repo root was deliberately deferred there, not fixed. ADR-0013's othe
|
||||
(rule scope, `level: error` model, `SentenceOpenerThereIs`/`VagueQualifier` trial outcomes) is
|
||||
unaffected and remains in force.
|
||||
|
||||
**Amended by ADR-0025 (2026-09-15).** The reasoning below is not reversed; its *precondition* is
|
||||
gone. The two skill-scoped Vale copies this ADR mandates — `agent-audit/assets/vale/` (canonical)
|
||||
and `skill-audit/assets/vale/` (subset) — existed because the no-cross-skill-sharing rule made it
|
||||
impossible for one audit skill to read the other's config. ADR-0025 merges the two skills into
|
||||
`factory-audit`, so there is no boundary left to duplicate across: there is now **one** copy, at
|
||||
`plugins/kyberforge/.apm/skills/factory-audit/assets/vale/`, carrying both styles and the
|
||||
single-file `.vale.ini` — `[**/SKILL.md]`, `[**/agents/*.md]`, `[**/*.agent.md]` — that this ADR's
|
||||
"One hook per file-scope" section had split in two. `scripts/check-vale-style-sync.sh`, decided on
|
||||
below and wired at pre-push, is deleted with the copy it diffed. Nothing it asserted about the
|
||||
config was lost. Its six-row glob-coverage probe table is now `tests/test-vale-wrap.sh` cases 28-30,
|
||||
run against the merged config. Case 31 carries across the per-rule override allowlist, and case 0
|
||||
carries across the "config loads" guards. Its **cross-manifest `files:` drift check** is ported as
|
||||
case 33. The original keyed each hook's record on `entry:`, which stopped working once both vale
|
||||
hooks shared one entry, so the port pairs the hooks by `id:` instead. Of the script's 17 assertion
|
||||
sites, 6 compared the two copies and are moot, 10 are rehomed and 1 is ported. ADR-0025 gives the
|
||||
per-assertion mapping; read the "six" here as probe *rows*, not as a share of those 17.
|
||||
What does **not** change: the two exported hook IDs, `kyberforge-vale-audit-skill` and
|
||||
`kyberforge-vale-audit-agent`, keep their IDs and their `files:` regexes — external consumers pin
|
||||
them by name — and the argument-free `entry:` contract is untouched. Read the two-copy table, the
|
||||
sync-check paragraph, and the `tests/test-vale-wrap.sh` Consequences bullet below as the state this
|
||||
ADR established, not as current layout.
|
||||
|
||||
`skill-audit`/`agent-audit`'s Step 1 called
|
||||
`"$(git rev-parse --show-toplevel)/scripts/vale-wrap.sh" --config "$(git rev-parse --show-toplevel)/.vale.ini"`
|
||||
— which resolves to whichever repo the skill happens to be running in. Inside `ai-development`
|
||||
@@ -128,7 +150,9 @@ doesn't wonder if it was overlooked.
|
||||
`.pre-commit-config.yaml` stays byte-identical to the shipped manifest on those `entry:` lines
|
||||
so the local gate keeps exercising the same resolution path a consumer does.
|
||||
- `tests/test-vale-wrap.sh` now exercises skill-audit's copy specifically — its fixtures are all
|
||||
`SKILL.md`-shaped, and only skill-audit's `.vale.ini` has the matching glob section.
|
||||
`SKILL.md`-shaped, and only skill-audit's `.vale.ini` has the matching glob section. (State as of
|
||||
this ADR. Since ADR-0025 there is one `vale-wrap.sh` and one `.vale.ini` under `factory-audit/`,
|
||||
and that suite exercises all three glob sections of the merged config — see cases 28-30.)
|
||||
- The first `vX.Y.Z` tag is cut once this change and its tests pass, giving external
|
||||
`.pre-commit-hooks.yaml` consumers something to pin.
|
||||
- **Cutting the tag is not left to memory.** `scripts/check-release-needed.sh`, wired at
|
||||
|
||||
@@ -123,7 +123,8 @@ correction) sorted what they document into three buckets:
|
||||
- ADR-0016 (a narrower decision discovered while designing issue #89) turned out to gate how
|
||||
issue #90 had to re-author plugin-scope agents: `.apm/agents/*.agent.md` compiles verbatim to
|
||||
both Claude and Copilot, so those files carry only the fields in the `apm-agent-allowlist` section
|
||||
of `plugins/kyberforge/.apm/skills/agent-audit/references/field-inventory.md` (as amended
|
||||
of `plugins/kyberforge/.apm/skills/agent-audit/references/field-inventory.md` (now
|
||||
`factory-audit/references/agent-field-inventory.md`, see ADR-0025) (as amended
|
||||
2026-08-14: `name`/`description`/`model`/`source_keys`/`disallowedTools`) — existing dual-file
|
||||
`<name>.md`+`<name>.agent.md` pairs could not be raw-moved, only re-authored.
|
||||
- Two follow-up issues tracked the remaining work: #89 (`skill-author`/`agent-author` routing
|
||||
|
||||
@@ -143,7 +143,8 @@ below is narrowed accordingly.
|
||||
|
||||
Enforcement follows the decision: `agent-audit`'s plugin-scope validator reads its allowlist as
|
||||
data from the `apm-agent-allowlist` section of
|
||||
`plugins/kyberforge/.apm/skills/agent-audit/references/field-inventory.md`, and that line now reads
|
||||
`plugins/kyberforge/.apm/skills/agent-audit/references/field-inventory.md` (now
|
||||
`factory-audit/references/agent-field-inventory.md`, see ADR-0025), and that line now reads
|
||||
`name description model source_keys disallowedTools`. `disallowedTools` also stays in that file's
|
||||
`claude-code-only-fields` list, which is not a contradiction — that list governs whether a field
|
||||
may cross the CC/Copilot boundary in a real project/user-scope *pair*, a different question from
|
||||
|
||||
@@ -65,7 +65,9 @@ Three sub-decisions inside that:
|
||||
## Consequences
|
||||
|
||||
**Skills gain an unnamespaced name.** apm deploys plain project skills, so `git:git-commits` also
|
||||
answers to `git-commits` and `kyberforge:skill-audit` to `skill-audit`. This is not configurable —
|
||||
answers to `git-commits` and `kyberforge:skill-audit` to `skill-audit` (**skill renamed per ADR-0025,
|
||||
2026-09-15:** that skill is now `factory-audit`, so the live example is `kyberforge:factory-audit` to
|
||||
`factory-audit`; the rule is unchanged). This is not configurable —
|
||||
a project skill has no plugin to prefix. `AGENTS.md` and `CONTEXT.md` are updated to name the bare
|
||||
form, which is what apm deploys and the only form a repo consuming holocron through apm gets.
|
||||
|
||||
|
||||
@@ -8,6 +8,23 @@ gates that hold them.
|
||||
|
||||
**Status: accepted (2026-08-14).**
|
||||
|
||||
**Amended by ADR-0025 (2026-09-15).** The contract, the tiers and every verdict rule below stand
|
||||
unchanged. What moved is the **number and location of the scripts that carry them**. This ADR names
|
||||
three: `scripts/skill-size-check.sh`, `skill-audit/scripts/validate.sh` and
|
||||
`agent-audit/scripts/validate.sh` — "all three validators" (Decision), "all three scripts"
|
||||
(Enforcement table footnote), "`scripts/skill-size-check.sh` and its two mirrored copies"
|
||||
(the `_add()` amendment). ADR-0025 merged the two audit skills, so there are now **two**: the root
|
||||
`scripts/skill-size-check.sh`, which still embeds the 1,061-line block between `BEGIN`/`END ADR-0020
|
||||
SHARED BOUNDARY RESOLVER` markers, and one plugin copy — extracted out of the merged validator into
|
||||
`plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh` and sourced by
|
||||
`factory-audit`'s `validate.sh` and `validate-provenance.sh` rather than pasted into each. The
|
||||
Enforcement table's "constants mirrored in `skill-audit/scripts/validate.sh` and
|
||||
`agent-audit/scripts/validate.sh`" is one path now, `factory-audit/scripts/validate.sh`, which
|
||||
auto-detects the artifact type; the skills/agents columns are unaffected, since the merged validator
|
||||
applies the body tiers on the skill path only. The two copies must still stay byte-identical — a
|
||||
plugin script cannot source the root one, which is why a second copy exists at all. Read every
|
||||
"three" below as the count at the time of writing.
|
||||
|
||||
## Context
|
||||
|
||||
Every `file:line` citation in this ADR is against the base commit the decision was taken on,
|
||||
@@ -189,7 +206,8 @@ Agents take the same description gates — they are preloaded identically — an
|
||||
A skill body is loaded into the caller's context, competing with the live conversation; an agent body
|
||||
becomes the system prompt of a fresh context. The rationale for the 900-word FAIL does not transfer.
|
||||
|
||||
That exemption is expressed in `agent-audit/scripts/validate.sh`, which has no body constant, and in
|
||||
That exemption is expressed in `agent-audit/scripts/validate.sh` (now `factory-audit`'s
|
||||
auto-detecting `validate.sh` on its agent path, see ADR-0025), which has no body constant, and in
|
||||
the `files:` pattern of the `skill-size-check` pre-commit hook, which is `SKILL.md`-only. It is *not*
|
||||
expressed in `scripts/skill-size-check.sh` itself, which measures whatever path it is handed —
|
||||
running it directly over `plugins/*/.apm/agents/*.agent.md` exits 1 with 900-word body FAILs on
|
||||
@@ -201,7 +219,8 @@ file pattern, not by the script knowing the difference. Anyone widening that pat
|
||||
would silently enforce a gate this ADR declines to set.
|
||||
|
||||
A plugin-scope agent is a single file with no sibling `references/` directory, so it cannot disclose
|
||||
to itself — it can only delegate to skills. `agent-audit` therefore gains a **delegation check**: an
|
||||
to itself — it can only delegate to skills. `agent-audit` (now `factory-audit`'s agent flow, see
|
||||
ADR-0025) therefore gains a **delegation check**: an
|
||||
agent body that restates a procedure owned by a skill it can invoke is a FAIL, with the fix being
|
||||
"invoke `<skill>` instead". Length falls out of delegation rather than being gated directly.
|
||||
|
||||
@@ -232,18 +251,20 @@ type of input they take should be **one skill with a dispatch table**. This catc
|
||||
one-or-two-file agent pair, per ADR-0005 and ADR-0016) and their overlap is in the improve flow
|
||||
rather than the core job.
|
||||
|
||||
**DEFERRED — not implemented in the change that carries this ADR. Tracked as issue #101.** Both
|
||||
skills still exist separately, and this change made the split deeper rather than shallower: retrofit
|
||||
**DEFERRED when this ADR was written — not implemented in the change that carries it. Tracked as
|
||||
issue #101. IMPLEMENTED by ADR-0025 (2026-09-15), which merged the pair into `factory-audit` with a
|
||||
Step 0 dispatch and closed the deferral.** At the time of writing both
|
||||
skills still existed separately, and this change made the split deeper rather than shallower: retrofit
|
||||
to the dispatch pattern took `skill-audit` from 3 reference files to 7 and `agent-audit` from 4 to 8,
|
||||
and their two same-named `references/description-quality.md` files now differ on 100 of ~120 lines
|
||||
after normalising `skill`/`agent`, where before they were closer. It has kept deepening since: the
|
||||
#99 retrofit added `finding-criteria.md` to `skill-audit`, drawing it level with `agent-audit`. Both
|
||||
figures move with the next retrofit, so measure rather than quote —
|
||||
`ls plugins/kyberforge/.apm/skills/<name>/references/ | grep -c '\.md$'`. The merge stays the
|
||||
`ls plugins/kyberforge/.apm/skills/factory-audit/references/ | grep -c '\.md$'`. The merge stayed the
|
||||
decision; it reopens ADR-0008 (agent-audit's single-file invocation contract) and touches every call
|
||||
site in `skill-author`, `agent-author` and `forge`, which is why it is its own change and not a rider
|
||||
on this one. Recorded here rather than dropped, so the gap between the rule and the tree is deliberate
|
||||
and dated instead of discovered later.
|
||||
site in `skill-author`, `agent-author` and `forge`, which is why it was its own change and not a rider
|
||||
on this one. Recorded here rather than dropped, so the gap between the rule and the tree was deliberate
|
||||
and dated instead of discovered later — and ADR-0025 is where it was closed.
|
||||
|
||||
### Enforcement and rollout
|
||||
|
||||
@@ -266,7 +287,7 @@ which tier each rule is in, because the failure this ADR is most exposed to is a
|
||||
| description opener, composition notes in a description | skills, agents | prose pattern | `plugins/kyberforge/.apm/skills/*/assets/vale/styles/Kyberforge/` |
|
||||
| a Gotcha paraphrasing a body step | skills | **auditor judgment** | `references/body-discipline.md` |
|
||||
| dispatch at two or more mutually exclusive flows | skills | **auditor judgment** | `references/body-discipline.md` |
|
||||
| delegation: an agent body restating a skill's procedure | agents | **auditor judgment** | `agent-audit` |
|
||||
| delegation: an agent body restating a skill's procedure | agents | **auditor judgment** | `agent-audit` (now `factory-audit`'s agent flow, see ADR-0025) |
|
||||
| capability enumeration, restatement, trigger quality | skills, agents | **auditor judgment** | `references/description-quality.md` |
|
||||
|
||||
The rows in bold are stated as FAILs in the Decision above and are FAILs an *auditor* issues. None of
|
||||
@@ -399,16 +420,18 @@ over the same 39 files now reports 0 errors, 0 warnings and 0 suggestions, so
|
||||
independent of `skill-size-check`, so a new description can reintroduce it; `skill-size-check` does
|
||||
not cover the Vale half, and no `references/` file is linted by anything (`docs/spec/gates.md` has
|
||||
both causes, issue #117 tracks them). Re-derive rather than quote —*
|
||||
`bash plugins/kyberforge/.apm/skills/skill-audit/scripts/vale-wrap.sh plugins/*/.apm/skills/*/SKILL.md`.
|
||||
`bash plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh plugins/*/.apm/skills/*/SKILL.md`
|
||||
*(path re-pointed by ADR-0025; the `skill-audit` copy this ADR originally named no longer exists).*
|
||||
|
||||
**A ceiling does not produce an average.** If every author writes to the 400-character FAIL, the
|
||||
preload lands at 39 × 400 = 15,600 chars — a 33% cut off 23,427, not the ~50% intended. Writing to
|
||||
the 250-character SUGGESTION instead lands at 9,750, a 58% cut. The halving depends entirely on the
|
||||
250-character SUGGESTION tier being visible and respected. That tier works here in a way it does not
|
||||
elsewhere in this repo: `skill-audit` already reports `PASS (N suggestions)` as a first-class
|
||||
elsewhere in this repo: `skill-audit` (now `factory-audit`'s skill flow, see ADR-0025) already
|
||||
reports `PASS (N suggestions)` as a first-class
|
||||
outcome. This is explicitly **not** the failure ADR-0013 records — Vale warnings are invisible
|
||||
because vale's exit code keys on `error` alone, but these gates live in `validate.sh` and
|
||||
`skill-audit`, where a SUGGESTION reaches the report. Realistic landing is somewhere in that 33-58%
|
||||
because vale's exit code keys on `error` alone, but these gates live in `validate.sh` and the
|
||||
audit skill itself, where a SUGGESTION reaches the report. Realistic landing is somewhere in that 33-58%
|
||||
band, not a guaranteed 50%.
|
||||
|
||||
**A word gate cannot detect the defect it is standing in for.** `git-commits` carries twelve Gotchas
|
||||
|
||||
@@ -83,7 +83,8 @@ unnamed in `git`'s corrected description, though `65bac15`'s own commit message
|
||||
|
||||
**Nothing checks any of this.** `scripts/check-manifests.sh` does not contain the string
|
||||
`description`. The three ADR-0020 validators (`scripts/skill-size-check.sh` and skill-audit's and
|
||||
agent-audit's `validate.sh`) gate on SKILL.md and agent frontmatter; they do open `apm.yml`, but only
|
||||
agent-audit's `validate.sh` — two since ADR-0025 merged the audit pair into `factory-audit`, whose
|
||||
single auto-detecting `validate.sh` carries both) gate on SKILL.md and agent frontmatter; they do open `apm.yml`, but only
|
||||
to read `dependencies.apm` when resolving the boundary-target universe — none of them reads the
|
||||
`description:` key, and their hook globs match `SKILL.md` and `*.agent.md` only. `apm audit --ci`,
|
||||
`apm pack --check-clean` and `scripts/sync-plugin-content.sh --check --all` all compare compiled
|
||||
|
||||
@@ -0,0 +1,376 @@
|
||||
# `skill-audit` and `agent-audit` merge into one `factory-audit` with a Step 0 dispatch
|
||||
|
||||
**Status: accepted (2026-09-15).** Implements ADR-0020's "Merging siblings" rule, which named this
|
||||
exact pair, scoped itself to them, and then deferred the work as issue #101. The deferral is closed
|
||||
here. `skill-author` and `agent-author` stay separate — ADR-0020 excluded the author pair
|
||||
deliberately, and nothing in this change touches that exclusion.
|
||||
|
||||
## Context
|
||||
|
||||
Every figure below was measured against the worktree on 2026-09-15. Re-derive rather than quote; the
|
||||
commands are given where a number is load-bearing.
|
||||
|
||||
The two skills duplicate content because they cannot share a file.
|
||||
`plugins/kyberforge/.apm/skills/skill-author/references/deployment-modes.md`, sourced from the
|
||||
agentskills.io spec, states the constraint for APM package mode: file references inside
|
||||
`.apm/skills/<name>/` must not reach outside that skill's own directory, and the spec defines no
|
||||
cross-skill sharing mechanism. apm deploys skills flat into `.claude/skills/<name>/` with no plugin
|
||||
tier above them, so there is no directory a second skill could read from. ADR-0024 re-confirmed this
|
||||
after deleting the native install path, specifically to stop the constraint being re-litigated as a
|
||||
Claude-Code artifact. It is not one. It binds under the only install path that survives.
|
||||
|
||||
What that constraint costs, between these two skills:
|
||||
|
||||
| Duplicated artifact | Lines | Verification |
|
||||
|---|---|---|
|
||||
| ADR-0020 boundary resolver, embedded in both `validate.sh` copies | 1,061 | marker block `115..1175` (skill) and `189..1249` (agent); `tests/test-adr0020-contract.sh` assertion 1 hashes them |
|
||||
| `scripts/vale-wrap.sh` | 526 | `diff -q` clean |
|
||||
| `assets/vale/styles/Kyberforge/`, five rules | 44 | `diff -r` clean |
|
||||
| Contributing-files parser, embedded in both `validate-provenance.sh` copies | 93 | marker block `300..392` (skill) and `294..386` (agent); `diff -q` clean on the extracted blocks |
|
||||
| **Removable by merging** | **1,724** | |
|
||||
|
||||
On top of that, `scripts/check-vale-style-sync.sh` (413 lines) and
|
||||
`tests/test-check-vale-style-sync.sh` (797 lines) go with the merge. That is **not because the whole
|
||||
gate was a copy diff**; it was not, and saying so would overstate the case for deleting it. The
|
||||
script has **17 assertion sites**: 13 `err` calls and 4 hard-fail exits. Its closing
|
||||
`exit 1` only reports the `err` count, so it is not an assertion. Count them with
|
||||
`git show 61b0b9c^:scripts/check-vale-style-sync.sh`. An earlier revision of this ADR said 18. No
|
||||
reproducible counting rule gives 18, and it is corrected here.
|
||||
|
||||
| Class | Old line | What it asserted | Now |
|
||||
|---|---|---|---|
|
||||
| **Moot (6)** | 19 | `REPO_ROOT` is a directory | nothing to guard; no script |
|
||||
| | 42 | the `.apm/` paths are not stale | no copies to locate |
|
||||
| | 52, 56 | neither copy is missing | one copy |
|
||||
| | 60 | the two `vale-wrap.sh` copies are identical | one copy |
|
||||
| | 64 | the two `styles/Kyberforge/` copies are identical | one copy |
|
||||
| **Rehomed (10)** | 95, 113 | `.vale.ini` exists and is readable | case 0 |
|
||||
| | 122 | `StylesPath = styles` is set | case 0 |
|
||||
| | 127 | some section's `BasedOnStyles` names `Kyberforge` | case 28 (Part B proves it fails) |
|
||||
| | 174 | no Kyberforge rule is overridden below a bare `YES`/`error` | case 31 |
|
||||
| | 191 | `KyberforgeCopilot` ships and is loaded | case 30 |
|
||||
| | 308 | `vale` is installed | suite-level: exit 77, which `run-tests --strict` fails |
|
||||
| | 341 | every probe path matches some vale hook's `files:` regex | case 28 |
|
||||
| | 347 | every probe path raises a Kyberforge alert under the config | cases 28 and 29 |
|
||||
| | 397 | at least one probe row was checked | case 28's section floor |
|
||||
| **Ported (1)** | 343 | local and published `files:` regexes agree per probe | case 33 |
|
||||
|
||||
Six are moot. Two diffed the copies, and four guarded the script's own ability to locate them: a
|
||||
real `REPO_ROOT`, non-stale `.apm/` paths, and both copies present. With one copy and no script
|
||||
there is nothing left to diff or locate. The other ten read `.vale.ini`, the style directory and
|
||||
the hook manifests on their own terms, so they are **rehomed rather than retired**, into `tests/test-vale-wrap.sh`, which already owns the
|
||||
wrapper's behaviour against this config.
|
||||
|
||||
Two rehomed checks got stronger, because a text grep became a behavioural Vale probe:
|
||||
|
||||
- line 127: case 28 Part B drops `Kyberforge` from a copy and requires vale to report the style as
|
||||
not loaded;
|
||||
- line 191: case 30 requires the Copilot rule to fire on `.agent.md` and nowhere else, and Part B
|
||||
proves both an unload and a leak fail.
|
||||
|
||||
The rest moved at equal strength. Case 31 is the same grep as before. An earlier revision claimed
|
||||
"7 of 11 stronger"; that claim is withdrawn.
|
||||
|
||||
**The cross-manifest check is ported, not dropped.** It extracts each vale hook's `files:` regex
|
||||
from `.pre-commit-hooks.yaml` (the external-facing manifest) and from `.pre-commit-config.yaml`
|
||||
(this repo's own copy of the same hooks) *independently*. It then asserts that a probe path in scope
|
||||
of one is in scope of the other. That catches this repo narrowing its local hook without narrowing
|
||||
the published one, or the reverse.
|
||||
|
||||
The original selected each hook's record by matching `entry:` against the owning skill's
|
||||
`scripts/vale-wrap.sh` path. After the merge both vale hooks point at the same entry, so that
|
||||
selector can no longer tell them apart. Case 33 pairs the hooks by `id:` instead, from an explicit
|
||||
table: `kyberforge-vale-audit-skill` ↔ `vale-audit-prefilter-skill`, and
|
||||
`kyberforge-vale-audit-agent` ↔ `vale-audit-prefilter-agent`. It carries the original six probe rows
|
||||
unchanged. It also fails by name on a missing hook id, and on a class with no shared probe.
|
||||
|
||||
An earlier revision of this ADR shipped *without* that port and called the gap half-closed by case
|
||||
32. It was not. Narrowing `vale-audit-prefilter-skill` from `^plugins/[^/]+/...` to
|
||||
`^plugins/kyberforge/...` still matches tracked files of the right class. That clears case 32 while
|
||||
silently dropping every other plugin's skills from this repo's prefilter, and it was measured
|
||||
leaving the whole suite green. Case 33's Part B now makes exactly that mutation, the agent-hook
|
||||
equivalent and a renamed hook id, and requires each to fail.
|
||||
|
||||
Case 32 stays, for the separate zero-match question: each local hook must still select at least one
|
||||
tracked file, and only files of its own artifact class.
|
||||
|
||||
**Line count.** Merging removes the 1,724 duplicated lines above. Deleting the two sync-gate files
|
||||
(413 + 797 = 1,210) removes more, for **2,934 lines** in total, plus one pre-push hook,
|
||||
`check-vale-style-sync`, formerly at `.pre-commit-config.yaml:166`. Two smaller deletions are not in
|
||||
that figure:
|
||||
|
||||
- `scripts/sync-vale-styles.sh` (21 lines), the helper that regenerated skill-audit's copy from
|
||||
agent-audit's, now has nothing to sync.
|
||||
- `agent-audit/scripts/README.md` (47 lines) has no successor. Nothing referenced it, and the only
|
||||
README `references/skill-file-structure.md` mandates is `tests/README.md`, which survives.
|
||||
|
||||
The duplication is not symmetrical across the whole tree, and the asymmetry is what shapes the
|
||||
decision. Outside the shared resolver the two `validate.sh` copies total 1,293 lines (616 skill, 677
|
||||
agent) and have **91 distinct lines** in common. The two validators are not one script with a mode
|
||||
flag; they are two genuinely different scripts that happen to embed one identical block.
|
||||
|
||||
The bodies are the binding constraint on the merge. `skill-audit`'s body is 724 words and
|
||||
`agent-audit`'s is 808 — 1,532 together against `BODY_MAX_WORDS = 900`. Only 211 words are common
|
||||
to both (47 byte-identical body lines). A merged body that simply concatenated the two flows would
|
||||
fail the gate its own plugin enforces by a factor of 1.7, and there is no trimming route to 900:
|
||||
1,321 of the 1,532 words are flow-specific.
|
||||
|
||||
Both skills already carry `category: factory` in their metadata, and both carry five
|
||||
`source_keys` — ten in total, disjoint, because they audit against different specs.
|
||||
|
||||
## Decision
|
||||
|
||||
**The two skills become one, named `factory-audit`.**
|
||||
|
||||
**1. The name.** `factory` is what both already declare as their `category`, so the merged skill is
|
||||
named for the thing it audits rather than for the two input types it now dispatches between. Two
|
||||
alternatives were live and both are rejected below for naming reasons rather than substance:
|
||||
`audit` collides with the unrelated `agentsmd-audit`, and `forge-audit` makes a bare skill name a
|
||||
family prefix of a live sibling — `forge` exists at
|
||||
`plugins/kyberforge/.apm/skills/forge/`. No bare skill name may be a family prefix of another.
|
||||
|
||||
**2. `SKILL.md` becomes a dispatch body.** Steps 1-3 move out to `references/skill-flow.md` and
|
||||
`references/agent-flow.md`. The body carries the Gotchas that apply to both branches, the dispatch
|
||||
table, and Step 4 — Report, which is shared. This is ADR-0020's own rule ("Dispatch is mandatory at
|
||||
two or more mutually exclusive flows") applied to the file that defines it, and the word arithmetic
|
||||
above is why it is mandatory here rather than stylistic.
|
||||
|
||||
**Dispatch happens at Step 0, keyed on the target path, before Step 1 runs.** The table accepts
|
||||
exactly the shapes `scripts/validate.sh` detects:
|
||||
|
||||
- A directory containing `SKILL.md`, or a `SKILL.md` file (its parent directory is audited), takes
|
||||
the skill flow.
|
||||
- A `*.agent.md` file, or a `.md` file whose *immediate* parent directory is `agents/`, takes the
|
||||
agent flow.
|
||||
- Anything else stops, runs no validator, and names the two accepted shapes.
|
||||
|
||||
Putting the dispatch after any deterministic check would mean running the wrong validator first and
|
||||
reading its output as a finding. An earlier revision of the body carried a two-row table with no
|
||||
fallback row. It could not route a `SKILL.md` file path, a trigger its own description advertised.
|
||||
Its agent row ("a path under `.apm/agents/`… or an agent markdown file") was both wider than the
|
||||
script and circular.
|
||||
|
||||
**3. One entry point per script, auto-detecting, with the mode-specific half sourced.**
|
||||
|
||||
- `scripts/validate.sh` detects the target type itself, then sources `scripts/lib-boundary-resolver.sh`
|
||||
and one of `scripts/lib-checks-skill.sh` / `scripts/lib-checks-agent.sh`.
|
||||
- `scripts/validate-provenance.sh` does the same, sourcing `scripts/lib-contributing-files.sh` and
|
||||
one of `scripts/lib-provenance-skill.sh` / `scripts/lib-provenance-agent.sh`.
|
||||
|
||||
Two things justify this shape. First, **self-containment binds between skills, not within one.** The
|
||||
resolver had to be embedded verbatim in three copies because three skill directories cannot read
|
||||
each other's files; two files inside one skill directory have no such problem. Sourcing is available
|
||||
the moment the directory boundary between them disappears. Second, **a single auto-detecting entry
|
||||
point makes a Step 0 misdispatch detectable.** The script re-detects the flow from the target, so
|
||||
even after a misdispatch it runs the right checks and its finding tiers are correct. That alone does
|
||||
not make the misdispatch self-correcting, and an earlier revision of this ADR wrongly said it did.
|
||||
The flow file drives Steps 2-4, so a misdispatched audit would still apply the wrong Step 3 rubrics,
|
||||
print the wrong coverage line and recommend the wrong author skill. The body closes that gap with an
|
||||
explicit guard under the Step 0 table: if `validate.sh` reports on the other artifact type than the
|
||||
row taken, discard the run and restart at Step 0.
|
||||
|
||||
**4. Reference files are prefixed by flow, with one exception.** Every flow-specific file becomes
|
||||
`skill-*` or `agent-*` — `skill-description-quality.md`, `agent-description-quality.md`,
|
||||
`skill-finding-criteria.md`, `agent-finding-criteria.md`, and so on. The exception is `sources.md`,
|
||||
which stays singular and carries all ten `source_keys`, because the skill-side provenance check
|
||||
hard-codes `os.path.join(skill_dir, "references", "sources.md")` (pre-merge
|
||||
`skill-audit/scripts/validate-provenance.sh:180`, now `scripts/lib-provenance-skill.sh:215`). A per-flow sources file would mean
|
||||
changing the provenance contract to get a cosmetic gain.
|
||||
|
||||
**5. Both exported Vale hook IDs survive unchanged.** `.pre-commit-hooks.yaml` keeps
|
||||
`kyberforge-vale-audit-skill` and `kyberforge-vale-audit-agent`, keeps both `files:` regexes
|
||||
(`(^|/)SKILL\.md$` and `(^|/)agents/[^/]+\.md$|\.agent\.md$`), and re-points both `entry:` lines at
|
||||
the one surviving `vale-wrap.sh`. Nothing in the published hook-repo contract changes: an external
|
||||
consumer's `.pre-commit-config.yaml` keeps working byte-for-byte across the merge. Two IDs pointing
|
||||
at one script is not a redundancy — it is what keeps the two `files:` scopes addressable
|
||||
independently, which is exactly ADR-0014's "one hook per file-scope" finding.
|
||||
|
||||
**6. `tests/test-adr0020-contract.sh` changes in three ways, and the third is a conversion, not a
|
||||
deletion.** Assertion 1 drops from three resolver copies to two: the merged `factory-audit` holds
|
||||
one, and `scripts/skill-size-check.sh` keeps its embedded copy. A new assertion 1a gives the
|
||||
resolver the same protection 1b already gave the parser. It asserts that `validate.sh` sources
|
||||
`lib-boundary-resolver.sh` in both mode branches, and that the resolver's BEGIN marker and
|
||||
`def _authoring_root(` appear in exactly those two files and nowhere else. A byte-identity hash alone
|
||||
would miss a third pasted copy, or an entry point that quietly stopped sourcing the library. Sourcing the resolver from the
|
||||
plugin tree into `skill-size-check.sh` was considered and refuted — that script is a repo-root hook
|
||||
consumed through `.pre-commit-hooks.yaml`, where `entry[0]` is the only token pre-commit rewrites,
|
||||
so it cannot reach a file inside the plugin at a path any consumer has. Assertion 1b is **converted**:
|
||||
it stops pinning that two `validate-provenance.sh` copies of the Contributing-files parser are
|
||||
byte-identical, and starts pinning that `lib-contributing-files.sh` is a single sourced copy that has
|
||||
not been re-inlined into either mode library. The claim it protects is the same one — the parser has
|
||||
exactly one authority — stated against the new structure. The drift history behind it is smaller than
|
||||
an earlier revision of this ADR implied. `484357a` (2026-08-30) added the bullet-form parser to both
|
||||
copies with two different spellings of the loop: a temporary `rest` in skill-audit and an inline
|
||||
slice in agent-audit. The two were behaviourally identical. `598a7c3` (2026-09-01) unified the
|
||||
spellings and added the `SHARED CONTRIBUTING-FILES PARSER` markers that 1b hashed. From then until
|
||||
the merge's parent the two marker blocks were byte-identical (`md5 0857272d…` both). So the parser
|
||||
never *parsed* differently. What the gate never covered was the prose around the block, and a
|
||||
docstring there asserted identity the loop did not have. One sourced library removes the question.
|
||||
|
||||
**7. Two things this change does not do.** `skill-author` and `agent-author` are **not** merged
|
||||
here. That remains an open finding and it is unmeasured; ADR-0020 excluded the pair on the grounds
|
||||
that they emit genuinely different artifacts, and nothing measured in this session revisits that.
|
||||
And **no audit criterion changes.** Every check, tier, threshold, regex and branch is carried across
|
||||
as-is. The Python payloads reassembled from the new libraries differ from the pre-merge heredocs only
|
||||
in comments. The one exception is three lines naming `references/agent-field-inventory.md`, a
|
||||
byte-identical rename of `field-inventory.md`. Byte-level differential runs over every live skill
|
||||
directory and agent file matched stdout, stderr and exit code.
|
||||
|
||||
**The entry points are not behaviour-neutral, and an earlier revision of this ADR said they were.**
|
||||
Those differential runs used valid targets only, so they could not see that the new detection layer
|
||||
changed what happens to *invalid* ones. Every change below is deliberate:
|
||||
|
||||
| Input | Pre-merge | Now |
|
||||
|---|---|---|
|
||||
| a missing path, a directory with no `SKILL.md`, a non-agent `.md` (e.g. `README.md`) | exit 1, or a mode-specific exit-2 message | **exit 2** with one generic "matches neither" Error/Why/Fix. Exit 2 is the never-ran tier, so the flow files report the section as unverified and quote the reason. |
|
||||
| a `SKILL.md` file path | exit 1 or 2 (`…/SKILL.md/SKILL.md not found`, "not a directory") | **accepted**; its parent directory is audited |
|
||||
| an agent `.md` *not* under an `agents/` directory (e.g. `~/drafts/my-agent.md`) | audited | **refused, exit 2**. Detection never guesses. No tracked file in this repo is affected. |
|
||||
| a bare or `./`-relative agent filename, run from inside its `agents/` directory | audited | audited. The parent directory's name is read from the real path, not the typed string. |
|
||||
| a `lib-*.sh` missing or unreadable, or the script directory unresolvable | did not apply (single file) | **exit 2** with Error/Why/Fix, never a raw bash error at exit 1, which is the real-findings tier |
|
||||
| `CDPATH` exported | did not apply (no `cd`) | no effect. `SCRIPT_DIR` resolves with `CDPATH=''` and `cd -- … >/dev/null`. |
|
||||
| no argument | `Error: skill-dir is required.` / `agent-file is required.` | one combined message and usage block; exit code unchanged (1 from `validate.sh`, 2 from `validate-provenance.sh`) |
|
||||
|
||||
A single `validate.sh` copied or symlinked out of its `scripts/` directory still does not work,
|
||||
because its libraries are not beside it. It now fails at exit 2 and says so.
|
||||
|
||||
## Considered options
|
||||
|
||||
**Keep two skills and rely on the byte-identity contract test alone (rejected).** This is the status
|
||||
quo: `tests/test-adr0020-contract.sh` already hashes the resolver across copies, and
|
||||
`check-vale-style-sync.sh` already diffs the Vale halves at pre-push. Only 6 of its 17 assertion
|
||||
sites exist because there are two copies. The other 11 do other work, and are rehomed or ported above rather than being an
|
||||
argument for the status quo. On the duplication itself it polices drift rather than removing the thing that drifts,
|
||||
and it pays 2,934 lines plus a pre-push hook to do so. It also leaves
|
||||
the router carrying a mutually-excluding near-miss pair whose two descriptions each spend a boundary
|
||||
clause pointing at the other — a routing cost the merge removes for free. ADR-0020 already weighed
|
||||
this option for this pair and chose merging; nothing measured since changes the balance.
|
||||
|
||||
**One monolithic dispatching `validate.sh` (rejected).** Dropping one resolver copy from the
|
||||
concatenation of the two current files gives roughly 2,354 lines in a single script. It is the
|
||||
straightforward reading of "merge the scripts", and it is wrong on the evidence: the two validators
|
||||
share only 91 distinct lines outside the resolver, so a monolith would be two near-disjoint
|
||||
implementations behind one `if`, with every future edit to either half requiring a reader to hold
|
||||
both in context. Sourcing per-mode libraries gets the same single entry point and keeps the halves
|
||||
readable apart.
|
||||
|
||||
**Genuinely merging the three colliding reference files into two-section files (rejected).**
|
||||
`description-quality.md`, `finding-criteria.md` and `validation-scripts.md` exist under both skills
|
||||
today, and folding each into one file with a skill section and an agent section is the tidier-looking
|
||||
outcome. It defeats the dispatch. The entire point of moving Steps 1-3 into `references/` is that an
|
||||
invocation loads one flow's content and not the other's; a two-section reference file re-inflates
|
||||
per-invocation context to the full 1,532-word span the body ceiling forced out. ADR-0020 measured
|
||||
these same files at 100 of ~120 differing lines after normalising `skill`/`agent`, so the merged file
|
||||
would also be mostly disjoint text under one heading.
|
||||
|
||||
**Naming it `audit` (rejected).** Shortest available name and an accurate one. It collides with
|
||||
`agentsmd-audit`, which audits a repo's `AGENTS.md` and has nothing to do with the factory. A bare
|
||||
`audit` alongside it reads as the general case of a skill it is unrelated to, which is precisely the
|
||||
routing confusion a merge is supposed to reduce.
|
||||
|
||||
**Naming it `forge-audit` (rejected).** It matches the plugin and reads well. `forge` is a live skill
|
||||
in the same plugin, so `forge-audit` makes one bare skill name a prefix of another — a router asked
|
||||
to distinguish `forge` from `forge-audit` is being asked to disambiguate on a suffix, and a user
|
||||
typing `forge` gets an ambiguity that does not exist today.
|
||||
|
||||
**Collapsing the two exported Vale hook IDs into one (rejected).** With a single `vale-wrap.sh` and a
|
||||
single `.vale.ini`, one hook ID looks sufficient. It is a breaking change to a published hook-repo
|
||||
contract: any external repo pinning `kyberforge-vale-audit-agent` breaks on upgrade, for no gain.
|
||||
It also re-creates ADR-0014's measured failure in a new place — that ADR confirmed empirically that a
|
||||
single hook entry pointed at one config silently scanned 0 files of the other type. Two IDs cost two
|
||||
manifest stanzas and keep both file scopes explicit.
|
||||
|
||||
## Consequences
|
||||
|
||||
**The single-file `.vale.ini` comes back, and this does not reverse ADR-0014.** ADR-0014 split one
|
||||
root config into two skill-scoped copies because two skills each needed their own, and no
|
||||
plugin-level shared directory exists to hold one. Its reasoning is untouched; the merge removes the
|
||||
condition that reasoning operated on. One skill needs one config, so the union is written back into
|
||||
one file. **The union is behaviour-neutral and this was checked rather than assumed:** skill-audit's
|
||||
config has a single `[**/SKILL.md]` section, agent-audit's has `[**/agents/*.md]` and
|
||||
`[**/*.agent.md]`, and no file in the corpus matches more than one of the three. Where an overlap is
|
||||
constructible at all (`agents/SKILL.md`), both matching sections assign `BasedOnStyles = Kyberforge`,
|
||||
so even then no verdict moves. `KyberforgeCopilot` stays scoped to `[**/*.agent.md]` exactly as it is
|
||||
now, which is what keeps the merged config from widening Copilot-specific rules onto `SKILL.md`.
|
||||
|
||||
**`scripts/check-scope-walkup-sync.sh` survives, and confusing it with `check-vale-style-sync.sh` is
|
||||
the obvious mistake here.** The two look like the same kind of gate and are not. The walk-up checker
|
||||
covers **four** independent ports of the scope walk-up, and only two of them live in the audit pair:
|
||||
the other two are `agent-author/scripts/new-agent.sh` and `skill-author/scripts/new-skill.sh`, which
|
||||
this change does not touch. They are also Bash where the audit pair's are Python, so as its own header
|
||||
records, it can never become a text diff — it asserts behavioural agreement across a fixture matrix
|
||||
instead. Merging two of four ports leaves three ports and the same job.
|
||||
|
||||
**Roughly 71 files carry inbound references to the two skill names and must be re-pointed.**
|
||||
Derived as `git grep -l -E "skill-audit|agent-audit" | wc -l` — it includes ADRs, `LESSONS.md`,
|
||||
`docs/spec/gates.md`, both author skills' routing targets, `forge`'s dispatch, the test suite and the
|
||||
two manifests. Boundary clauses naming `skill-audit` or `agent-audit` are the sharp end: ADR-0020's
|
||||
resolvable-target check is a blocking ERROR on a dangling route, so a missed rename fails the push
|
||||
rather than degrading quietly. Historical references inside ADRs describing the pre-merge state stay
|
||||
as they are; the resolver reads boundary clauses in descriptions, not ADR prose.
|
||||
|
||||
**The dispatch body carries only the gotchas common to both flows, and ships with no SUGGESTION.** An
|
||||
earlier revision of this change shipped the Gotchas section at **229 of 548 body words, 42%**,
|
||||
against `GOTCHA_MAX_BODY_FRACTION = 0.25`. It accepted that as standing output, arguing that moving
|
||||
a gotcha to `references/` meant an extra file read on every invocation. That argument was wrong for
|
||||
the two gotchas that were over budget, because neither was shared:
|
||||
|
||||
- the `Agent flow, plugin/APM scope only` provider-safety bullet names its one branch in its own
|
||||
text;
|
||||
- the 112-word body-word-gate bullet was two separate pre-merge gotchas welded together, a skill half
|
||||
and an agent half.
|
||||
|
||||
A dispatch body is the dispatch table *plus the gates common to every branch* (CONTEXT.md; the
|
||||
skill-flow rubric `references/skill-body-discipline.md`). Keeping a single-branch gotcha in it
|
||||
contradicts that definition. Moving it into its flow file costs no read either, because the body
|
||||
already loads exactly one flow file on every invocation by construction.
|
||||
|
||||
So the skill half now sits under `## Gotchas` in `references/skill-flow.md`. The agent half and the
|
||||
provider-safety bullet sit under `## Gotchas` in `references/agent-flow.md`. The body keeps three
|
||||
gotchas: the no-narration rule, the `disable-model-invocation` exemption and the Vale `0 files`
|
||||
trap. Measured with `scripts/skill-size-check.sh` thresholds zeroed to force the figures out, the
|
||||
section is now **91 of 555 body words, 16%**.
|
||||
|
||||
**Every invocation now reads one extra `references/` file.** The dispatch body names the flow file
|
||||
and the agent loads it, where today Steps 1-3 arrive with the body. This is the cost the progressive-
|
||||
disclosure trade always carries, and it is paid against a saving: an invocation loads the dispatch
|
||||
body plus one flow instead of a body that would have to carry both. It is also the reason the
|
||||
two-section reference file was rejected above.
|
||||
|
||||
**The original audit's figures for this finding were wrong in three ways, and each is worth naming
|
||||
so the correction is not re-derived from scratch later.**
|
||||
|
||||
- It claimed roughly 3,300 duplicated lines and **two** pre-push hooks. The measured removal is
|
||||
**2,934 lines and one hook**. The second hook it counted was `check-scope-walkup-sync`, which
|
||||
survives for the reason above.
|
||||
- It claimed the two validators were one script hard-wired per mode. They are not. Outside the shared
|
||||
resolver they total 1,293 lines with 91 distinct lines in common. That error matters because it is
|
||||
what made the monolithic `validate.sh` look like the obvious implementation.
|
||||
- It named the **merged `description`** as the blocker on merging. It is not. Merging deletes
|
||||
description content rather than accumulating it: the `Not a skill directory -> skill-audit` clause
|
||||
loses its referent, and the `"is this ready to ship"` trigger was duplicated verbatim across both.
|
||||
The two descriptions it replaces measure **239** (skill-audit) and **250** (agent-audit) at
|
||||
`61b0b9c^`. The description this skill ships measures **241**, inside the 250 SUGGESTION target.
|
||||
It carries one arrow per boundary target (`Not applying skill fixes -> skill-author. Not applying
|
||||
agent fixes -> agent-author.`), because ADR-0020 resolves only the first target after an arrow, so
|
||||
a one-arrow form would leave `agent-author` checked by nothing. The real blocker was the body: 1,532
|
||||
words against `BODY_MAX_WORDS = 900`, with only 211 words shared. Diagnosing the description would
|
||||
have produced a merge with a concatenated body that failed its own plugin's gate.
|
||||
|
||||
**Correction to an earlier revision of this bullet.** It shipped the description at **319**
|
||||
characters and accepted the SUGGESTION. It said the excess paid for the second arrow and for
|
||||
"both flows' artifact-specific trigger phrases carried in full". Only the arrow was worth it.
|
||||
The trigger phrases stated one trigger twice in two registers: "a skill directory or agent
|
||||
definition audited", then quoted `audit this skill`, `review my SKILL.md`, `audit this agent` and
|
||||
`review my agent file`. ADR-0020 makes that a FAIL ("Stating the same trigger twice in two
|
||||
registers is a FAIL"), so it was not a cost of merging. Dropping the quoted duplicates, and keeping
|
||||
the one indirect trigger that omits the domain word (`is this ready to ship`), gives 241 with both
|
||||
arrows kept. The same revision's "240 characters" figure for a hypothetical single-arrow merge was
|
||||
never reproduced, and is withdrawn rather than re-derived.
|
||||
|
||||
**`factory-audit` shipped at `metadata.version: "1.0.0"`, not ADR-0022's `0.1.0` for a new skill.**
|
||||
It is a new directory, but not a new skill in the sense ADR-0022's starting version encodes: it
|
||||
carries every check, rubric and reference of two skills that were both already at `1.0.0`, and
|
||||
resetting to `0.1.0` would signal an immaturity that the merged content does not have. The fixes
|
||||
above to Step 0, the gotchas and the description are an improve pass, so under `skill-author`'s
|
||||
patch-bump rule it is now **`1.0.1`**. The plugin itself goes from `1.6.2` to **`2.0.0`**, because
|
||||
removing two invocable skills breaks anyone calling them by name.
|
||||
Reference in New Issue
Block a user