Merge pull request 'fix(gates): waive lockfile-exists for a package, which is not an install root' (#137) from fix/apm-audit-ci-package-lockfile into main
Reviewed-on: https://git.dev.rkdr.net/Defame1297/holocron/pulls/137
This commit was merged in pull request #137.
This commit is contained in:
@@ -97,8 +97,8 @@ repos:
|
||||
|
||||
- id: apm-audit-ci
|
||||
name: apm audit --ci
|
||||
description: Run apm's producer-side CI gate over the root manifest AND each of the six plugin packages. On the root manifest it runs ten checks -- lockfile-exists, ref-consistency, deployment-ledger-owners, deployed-files-present, no-orphaned-packages, skill-subset-consistency, config-consistency, content-integrity, includes-consent, drift -- so it is both a hidden-Unicode scan and a drift gate that replays the install and diffs it. In a plugin package it runs one, lockfile-exists. It does NOT enforce an org policy; see the comment below for why. Reference:plugins/kyberforge/.apm/skills/apm-workflow/references/audit.md
|
||||
entry: bash -c 'for d in . plugins/*/; do (cd "$d" && apm audit --ci) || { echo "apm audit --ci failed in $d" >&2; exit 1; }; done'
|
||||
description: Run apm's producer-side CI gate over the root manifest AND each plugin package, via scripts/apm-audit-ci.sh. On the root manifest it runs ten checks -- lockfile-exists, ref-consistency, deployment-ledger-owners, deployed-files-present, no-orphaned-packages, skill-subset-consistency, config-consistency, content-integrity, includes-consent, drift -- so it is both a hidden-Unicode scan and a drift gate that replays the install and diffs it. In a plugin package it runs one, lockfile-exists, which the script waives when that package declares dependencies, because a package is not an install root (ADR-0026). The waiver never applies to the root and never covers a second failing check. It does NOT enforce an org policy; see the comment below for why. Reference:plugins/kyberforge/.apm/skills/apm-workflow/references/audit.md
|
||||
entry: scripts/apm-audit-ci.sh
|
||||
language: system
|
||||
stages: [pre-push]
|
||||
pass_filenames: false
|
||||
@@ -121,9 +121,20 @@ repos:
|
||||
# does not (verified by adding a git dependency to
|
||||
# plugins/lint/apm.yml). Everything else above is root-only, because
|
||||
# only the root install has a lockfile, a deployment ledger and
|
||||
# deployed files to check. Running the six plugin packages is what
|
||||
# deployed files to check. Running every plugin package is what
|
||||
# makes lockfile-exists reachable for them at all -- the root-only
|
||||
# invocation audits the root manifest and nothing else.
|
||||
# THAT ARMING NOW HAPPENS: plugins/onedev declares a real dependency,
|
||||
# and there is no green state for it -- without a package lockfile
|
||||
# lockfile-exists fails, and with one it passes and arms the other
|
||||
# nine, where drift then demands the dependency's skills be deployed
|
||||
# INSIDE the package. A package is not an install root, so
|
||||
# scripts/apm-audit-ci.sh waives that single check for a package and
|
||||
# nothing else (ADR-0026). Dropping --ci for packages would have been
|
||||
# smaller and is wrong: verified on apm 0.28.0, plain `apm audit`
|
||||
# exits 0 on a dependency entry missing its git/path/registry field
|
||||
# while --ci exits 1 naming it, and malformed-dependency detection is
|
||||
# the whole reason packages are audited.
|
||||
# * HIDDEN CONTENT IS COVERED. content-integrity is that scan; it
|
||||
# reports `No critical hidden Unicode or hash drift detected`. An
|
||||
# earlier revision of this comment said the hook does NOT scan for
|
||||
|
||||
2266
apm.lock.yaml
2266
apm.lock.yaml
File diff suppressed because it is too large
Load Diff
60
docs/adr/0026-a-plugin-package-is-not-an-install-root.md
Normal file
60
docs/adr/0026-a-plugin-package-is-not-an-install-root.md
Normal file
@@ -0,0 +1,60 @@
|
||||
# A plugin package is not an install root — `apm-audit-ci` waives `lockfile-exists` for one
|
||||
|
||||
**Status:** Accepted (2026-09-20)
|
||||
|
||||
`plugins/onedev` is the first plugin package in this repo to declare a real dependency. It pins
|
||||
`code.onedev.io/onedev/tod#v4.3.4` so that a consumer installing `onedev` from the holocron
|
||||
marketplace picks up OneDev's eight TOD skills transitively — a `marketplace.packages` entry takes a
|
||||
local `source:` path, so a third-party repo cannot be listed for redistribution on its own, and the
|
||||
wrapper is the only mechanism that carries it.
|
||||
|
||||
That arms a check every previous plugin left vacuous, and leaves the package with no green state.
|
||||
|
||||
`apm audit --ci` in a plugin directory runs one check, `lockfile-exists`. While every plugin
|
||||
`apm.yml` declared `dependencies: {apm: [], mcp: []}` it reported `No dependencies declared --
|
||||
lockfile not required` and passed. `plugins/onedev` declares dependencies, so (verified against apm
|
||||
0.28.0):
|
||||
|
||||
- **without** a package `apm.lock.yaml` it fails — `apm.yml declares dependencies but apm.lock.yaml
|
||||
is absent`, reported as `1 of 1 check(s) failed`
|
||||
- **with** one it passes, and passing arms the other nine checks. `drift` then fails reporting eight
|
||||
unintegrated files at `.agents/skills/<name>/SKILL.md` — it wants the dependency's skills
|
||||
*deployed inside the package*. Generating the lockfile with `apm lock` also creates an
|
||||
`apm_modules/` tree in there.
|
||||
|
||||
The cause is that apm treats any directory holding both `apm.yml` and `apm.lock.yaml` as an **install
|
||||
root**. A plugin package is not one: it is content to be installed somewhere else. The second state
|
||||
is not a stricter version of the first, it is a category error — a package has no deployment target
|
||||
of its own, so there is nothing for a drift check to be right about.
|
||||
|
||||
The hook therefore waives `lockfile-exists`, and only that, for a non-root manifest.
|
||||
`scripts/apm-audit-ci.sh` replaces the inline `bash -c` loop that `.pre-commit-config.yaml` carried.
|
||||
The waiver fails closed on three axes: the root manifest is never waived whatever it reports; the
|
||||
failing check must be `lockfile-exists` and no other, asserted by matching `1 of 1 check(s) failed`,
|
||||
so any second failing check changes the count and fails the push normally; and output apm does not
|
||||
produce in the recognised shape is a failure.
|
||||
|
||||
**Dropping `--ci` for package directories was rejected.** It was the smaller change — plain
|
||||
`apm audit` in a plugin directory reports `No apm.lock.yaml found -- nothing to scan` and exits 0, so
|
||||
the loop would have gone green with a one-word edit. It is wrong. Verified on apm 0.28.0 against a
|
||||
scratch package whose dependency entry carried no `git`/`path`/`registry` field: `apm audit --ci`
|
||||
exits 1 naming the missing field, while plain `apm audit` exits 0 and says nothing. Malformed-
|
||||
dependency detection is the reason `docs/spec/gates.md` gives for auditing packages at all, and a
|
||||
package *with* dependencies is the only kind that can carry a malformed dependency entry — so the
|
||||
cheap fix would have discarded the check precisely where it earns its keep, in the one package that
|
||||
newly needs it.
|
||||
|
||||
Two alternatives were rejected for making the wrapper pointless or the repo fragile. Dropping the
|
||||
dependency from `plugins/onedev` turns the gate green immediately, but a consumer installing
|
||||
`onedev` from the marketplace then receives an empty package, which removes the only reason the
|
||||
wrapper exists. Committing a package lockfile and running `apm install` inside the package satisfies
|
||||
`drift` on a machine that has done so, but makes `deployed-files-present` a fresh-clone failure and
|
||||
commits this repo to maintaining a nested install root per package.
|
||||
|
||||
The weak point is stated rather than designed away: the waiver matches on apm's stdout, so an apm
|
||||
upgrade that rewords either line silently turns it off. That direction is safe — it fails the push
|
||||
rather than hiding a defect. Re-verify against the new output and update the two patterns rather
|
||||
than widening them.
|
||||
|
||||
This changes shared enforcement, which is why it is recorded here rather than left as a comment.
|
||||
`docs/spec/gates.md`'s `apm-audit-ci` section carries the operative detail.
|
||||
@@ -71,7 +71,7 @@ version-blind, so a stale key deploys fine (see [apm gates](#apm-gates)).
|
||||
|
||||
| Hook | Guards |
|
||||
|---|---|
|
||||
| `apm-audit-ci` | `apm audit --ci` once per manifest — root plus each of the six plugin packages |
|
||||
| `apm-audit-ci` | `scripts/apm-audit-ci.sh` — `apm audit --ci` once per manifest, root plus each of the seven plugin packages, waiving only a package's `lockfile-exists` (see [below](#apm-audit-ci)) |
|
||||
| `apm-pack-check-clean` | `apm pack --check-versions --check-clean --dry-run` — the compiled marketplace still matches what `apm.yml` + `.apm/` would generate, and per-package versions agree with the `per_package` strategy |
|
||||
|
||||
**Host validators** (needs the `claude` CLI on PATH)
|
||||
@@ -87,8 +87,8 @@ version-blind, so a stale key deploys fine (see [apm gates](#apm-gates)).
|
||||
| `check-skill-version-bump` | fails if a skill directory changed since the pushed commit's merge-base with `main` without its `metadata.version` rising above both the merge-base's and `main`'s tip's (see [below](#check-skill-version-bump)) |
|
||||
|
||||
Two of these shell out to `apm`: `apm-audit-ci` and `apm-pack-check-clean`. The second is a bare
|
||||
`apm …` entry and the first is a `bash -c` loop calling `apm` once per package, so without the CLI
|
||||
the push dies with an unhelpful "command not found". Install with `apm-install`, or
|
||||
`apm …` entry and the first is `scripts/apm-audit-ci.sh`, which calls `apm` once per manifest, so
|
||||
without the CLI on PATH the push dies on a "command not found" from inside the hook. Install with `apm-install`, or
|
||||
`curl -sSL https://aka.ms/apm-unix | sh`; verify with `apm --version`.
|
||||
|
||||
### `check-skill-version-bump`
|
||||
@@ -1059,11 +1059,12 @@ exclusion landed.
|
||||
|
||||
### `apm-audit-ci`
|
||||
|
||||
Runs `apm audit --ci` **once per manifest** — the root one and each of the six plugin packages —
|
||||
because the root-only invocation audits the marketplace manifest and **nothing else**, and
|
||||
`apm-pack-check-clean` does not parse plugin `dependencies:` blocks either. Verified: a malformed
|
||||
dependency entry passes `apm pack --check-versions --check-clean --dry-run` and fails
|
||||
`apm audit --ci` in that package's directory. Costs ~0.5s per package.
|
||||
`scripts/apm-audit-ci.sh` runs `apm audit --ci` **once per manifest** — the root one and each of the
|
||||
seven plugin packages — because the root-only invocation audits the marketplace manifest and
|
||||
**nothing else**, and `apm-pack-check-clean` does not parse plugin `dependencies:` blocks either.
|
||||
Verified: a malformed dependency entry passes
|
||||
`apm pack --check-versions --check-clean --dry-run` and fails `apm audit --ci` in that package's
|
||||
directory. Costs ~0.5s per package.
|
||||
|
||||
**What it actually runs is asymmetric**, and the two manifest classes are not comparable. Verified by
|
||||
running `apm audit --ci` (apm 0.28.0) at the repo root and in `plugins/lint/`, reading the check
|
||||
@@ -1074,10 +1075,40 @@ On the **root** manifest, **10 checks**: `lockfile-exists`, `ref-consistency`,
|
||||
`skill-subset-consistency`, `config-consistency`, `content-integrity`, `includes-consent`, `drift`.
|
||||
|
||||
On each **plugin** manifest, **1 check**: `lockfile-exists`. Conditional, and vacuous while every
|
||||
plugin `apm.yml` declares `dependencies: {apm: [], mcp: []}` — it reports `No dependencies declared
|
||||
-- lockfile not required` and arms itself the moment one does not (verified by adding a git
|
||||
dependency to `plugins/lint/apm.yml`). Everything else in the list above is root-only, because it is
|
||||
the root install that has a lockfile, a deployment ledger and deployed files to check.
|
||||
plugin `apm.yml` declared `dependencies: {apm: [], mcp: []}` — it reports `No dependencies declared
|
||||
-- lockfile not required`. An earlier revision of this section said it would arm the moment one did
|
||||
not. **It has armed.** `plugins/onedev` is the first plugin package to declare a real dependency — it
|
||||
pins `code.onedev.io/onedev/tod#v4.3.4` so the marketplace can redistribute OneDev's TOD skills — and
|
||||
the check now fires on it for real. Everything else in the list above is root-only, because it is the
|
||||
root install that has a lockfile, a deployment ledger and deployed files to check.
|
||||
|
||||
**A plugin package that declares dependencies has no green state, so the hook waives exactly one
|
||||
failure.** Verified against apm 0.28.0 in `plugins/onedev/`:
|
||||
|
||||
- **Without a package `apm.lock.yaml`**, `lockfile-exists` fails — `apm.yml declares dependencies but
|
||||
apm.lock.yaml is absent` — reported as `1 of 1 check(s) failed`.
|
||||
- **With one**, generated by `apm lock` in the package directory, `lockfile-exists` passes and
|
||||
thereby arms the other nine checks; `drift` then fails reporting **8 unintegrated files** at
|
||||
`.agents/skills/<name>/SKILL.md`, i.e. demanding the dependency's skills be *deployed inside the
|
||||
package*. `apm lock` also leaves an `apm_modules/` tree inside the package.
|
||||
|
||||
The cause is that apm treats any directory holding both `apm.yml` and `apm.lock.yaml` as an **install
|
||||
root**, and a plugin package is not one. `scripts/apm-audit-ci.sh` therefore waives `lockfile-exists`
|
||||
and nothing else, and only for a non-root manifest: it asserts the string `1 of 1 check(s) failed`,
|
||||
so any second failing check changes the count and the run fails normally, and output it does not
|
||||
recognise fails closed. The root manifest is never waived. Recorded as ADR-0026.
|
||||
|
||||
**Dropping `--ci` for package directories was considered and rejected.** It is the smaller change and
|
||||
it is wrong. Verified on apm 0.28.0 against a scratch package whose dependency entry carried no
|
||||
`git`/`path`/`registry` field: `apm audit --ci` exits 1 naming the field, while plain `apm audit`
|
||||
prints `No apm.lock.yaml found -- nothing to scan` and exits 0. Malformed-dependency detection is the
|
||||
reason this section gives for auditing packages at all, and a package *with* dependencies is the only
|
||||
kind that can carry a malformed dependency entry — so dropping `--ci` would discard the check
|
||||
precisely where it earns its keep.
|
||||
|
||||
**Known weak point: the waiver matches on apm's stdout.** An apm upgrade that rewords either line
|
||||
turns the waiver off. That fails the push rather than hiding a defect; re-verify against the new
|
||||
output and update the patterns rather than widening them.
|
||||
|
||||
**`manifest-parse` is not a named check** in apm 0.28.0's output, and an earlier revision of this
|
||||
section listed it as one. Parsing is still enforced — a dependency entry missing its
|
||||
@@ -1108,7 +1139,9 @@ or hash drift detected` — so the root invocation already covers it and nothing
|
||||
remains true is that the *standalone* mode is different: plain `apm audit` (`--ci` refuses to combine
|
||||
with `--file`/`--strip`/`--dry-run`/`PACKAGE`) run in a plugin directory reports
|
||||
`No apm.lock.yaml found -- nothing to scan` and exits 0, because only the root has a lockfile.
|
||||
Plugin manifests get `lockfile-exists` and nothing else; they are not Unicode-scanned.
|
||||
Plugin manifests get `lockfile-exists` and nothing else; they are not Unicode-scanned. That holds
|
||||
because no package carries an `apm.lock.yaml` — one would arm the other nine checks, `content-integrity`
|
||||
among them, which is the state ADR-0026 rules out rather than a second scan worth having.
|
||||
|
||||
### `check-executables-allow-sync`
|
||||
|
||||
|
||||
89
scripts/apm-audit-ci.sh
Executable file
89
scripts/apm-audit-ci.sh
Executable file
@@ -0,0 +1,89 @@
|
||||
#!/usr/bin/env bash
|
||||
# apm-audit-ci pre-push hook.
|
||||
#
|
||||
# Runs `apm audit --ci` once per manifest -- the root one and each plugin
|
||||
# package -- because the root-only invocation audits the marketplace manifest
|
||||
# and nothing else, and `apm pack --check-clean` does not parse plugin
|
||||
# `dependencies:` blocks either. Full rationale: docs/spec/gates.md,
|
||||
# "apm-audit-ci".
|
||||
#
|
||||
# WHY THIS IS A SCRIPT AND NOT THE ONE-LINE `for` LOOP IT REPLACED
|
||||
#
|
||||
# apm treats any directory holding both apm.yml and apm.lock.yaml as an INSTALL
|
||||
# ROOT. A plugin package is not one: it is content to be installed elsewhere.
|
||||
# While every plugin declared `dependencies: {apm: [], mcp: []}` the distinction
|
||||
# never surfaced, because the plugin-level `lockfile-exists` check reported
|
||||
# `No dependencies declared -- lockfile not required` and passed vacuously.
|
||||
#
|
||||
# plugins/onedev is the first package to declare a real dependency (it pins
|
||||
# OneDev's TOD skills so the marketplace can redistribute them), which arms that
|
||||
# check and leaves no green state:
|
||||
#
|
||||
# * no apm.lock.yaml in the package -> `lockfile-exists` fails with
|
||||
# "apm.yml declares dependencies but apm.lock.yaml is absent"
|
||||
# * an apm.lock.yaml in the package -> `lockfile-exists` passes and thereby
|
||||
# arms the other nine checks, and `drift` then fails demanding the
|
||||
# dependency's skills be DEPLOYED inside the package
|
||||
# (plugins/onedev/.agents/skills/...), which is meaningless for a package
|
||||
# and additionally litters it with an apm_modules/ tree
|
||||
#
|
||||
# So this hook waives exactly one failure: a plugin package whose ONLY failing
|
||||
# check is `lockfile-exists`. Verified against apm 0.28.0.
|
||||
#
|
||||
# WHAT IS DELIBERATELY NOT WAIVED
|
||||
#
|
||||
# Dropping `--ci` in package directories would have been the smaller change and
|
||||
# is WRONG. Verified on apm 0.28.0 against a scratch package whose dependency
|
||||
# entry carried no git/path/registry field: `apm audit --ci` exits 1 naming the
|
||||
# field, while plain `apm audit` prints "No apm.lock.yaml found -- nothing to
|
||||
# scan" and exits 0. Malformed-dependency detection is the reason gates.md gives
|
||||
# for auditing packages at all, and a package WITH dependencies is the only kind
|
||||
# that can carry a malformed dependency entry -- so the check would have been
|
||||
# discarded precisely where it earns its keep.
|
||||
#
|
||||
# The waiver is therefore narrow on three axes, and fails closed on each:
|
||||
# 1. the root manifest is never waived, whatever it reports
|
||||
# 2. the failing check must be `lockfile-exists` and no other -- the
|
||||
# "1 of 1 check(s) failed" assertion is what makes that true, since any
|
||||
# second failing check changes the count and the run fails normally
|
||||
# 3. output apm does not produce in the recognised shape is a failure
|
||||
#
|
||||
# Matching on apm's stdout is the weak point: an apm upgrade that rewords either
|
||||
# line silently turns the waiver off, which fails the push rather than hiding a
|
||||
# defect. If that happens, re-verify against the new output and update the two
|
||||
# patterns below rather than widening them.
|
||||
|
||||
set -uo pipefail
|
||||
|
||||
readonly WAIVED_CHECK='declares dependencies but apm.lock.yaml is absent'
|
||||
readonly SOLE_FAILURE='1 of 1 check(s) failed'
|
||||
|
||||
status=0
|
||||
|
||||
for manifest_dir in . plugins/*/; do
|
||||
output="$(cd "$manifest_dir" && apm audit --ci 2>&1)"
|
||||
exit_code=$?
|
||||
|
||||
if [ "$exit_code" -eq 0 ]; then
|
||||
continue
|
||||
fi
|
||||
|
||||
# Axis 1: the root is never waived.
|
||||
# Here-strings, not `printf ... | grep -q`: under `set -o pipefail` grep -q
|
||||
# exits on its first match, SIGPIPEs the writer, and the writer's death
|
||||
# becomes the pipeline's status -- a race tests/test-no-pipefail-early-exit-grep.sh
|
||||
# scans every tracked script for.
|
||||
if [ "$manifest_dir" != "." ] &&
|
||||
grep -qF "$WAIVED_CHECK" <<<"$output" &&
|
||||
grep -qF "$SOLE_FAILURE" <<<"$output"; then
|
||||
printf 'apm audit --ci: waived lockfile-exists in %s (package, not an install root)\n' \
|
||||
"$manifest_dir"
|
||||
continue
|
||||
fi
|
||||
|
||||
printf '%s\n' "$output" >&2
|
||||
printf 'apm audit --ci failed in %s\n' "$manifest_dir" >&2
|
||||
status=1
|
||||
done
|
||||
|
||||
exit "$status"
|
||||
@@ -737,9 +737,7 @@ EXPECTED = {
|
||||
'check-executables-allow-sync': (
|
||||
'bash scripts/check-executables-allow-sync.sh', ['pre-push']),
|
||||
'apm-audit-ci': (
|
||||
'bash -c \'for d in . plugins/*/; do (cd "$d" && apm audit --ci) || '
|
||||
'{ echo "apm audit --ci failed in $d" >&2; exit 1; }; done\'',
|
||||
['pre-push']),
|
||||
'scripts/apm-audit-ci.sh', ['pre-push']),
|
||||
'check-apm-agents-valid': (
|
||||
'bash scripts/check-apm-agents-valid.sh', ['pre-push']),
|
||||
'apm-pack-check-clean': (
|
||||
|
||||
Reference in New Issue
Block a user