Defame1297/holocron
0
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases 3 Wiki Activity

docs: close the PR #135 documentation review findings

Browse Source 
Group 3 of the validated PR #135 review fixes. Every figure and commit
citation below was re-verified at HEAD before being written.

ADR and architecture:
- #7 ADR-0025 cited 61b0b9c, which no published branch reaches. Repointed
  to 620f20b (identical parent tree, reachable from the PR branch), with a
  note that neither is reachable from origin/main. The parser-drift
  paragraph now credits 598a7c3 (the reachable PR #129 squash) and keeps
  484357a only as a pre-squash parenthetical.
- #8 architecture.md dropped the pointer at the LESSONS.md entry this
  branch deleted.
- #9 architecture.md's ADR entry points now name ADR-0015 (the one
  compiler) and ADR-0024, and list ADR-0024 as superseding ADR-0017.
- #10 ADR-0024 section 4 rewritten: the standing patch-bump rule is
  apm-workflow's configure.md, not ADR-0006's, and this change does not
  trigger it. ADR-0015:93 carries a correction for the misattribution.
- N5 ADR-0021 gained a Correction note for the deleted
  scripts/check-manifests.sh (e647f14).

gates.md:
- #11a the four ADR-0020 constants live in lib-checks-skill.sh:313-316 and
  lib-checks-agent.sh:164-165, not in validate.sh.
- #11b the pretty-format-json exclude is two alternations expanding to
  three tracked files, including .claude/apm-hooks.json.
- #11c the ADR-0020 contract suite runs 28 -> 27 -> 29 (620f20b,
  4de5b6b, ef27c97), 29 at HEAD; the unverifiable 25 is dropped.
- #11d the boundary resolver is one copy since ef27c97.
- #12 apm-audit-ci documents the 10 root checks and the 1 plugin check
  apm 0.28.0 actually runs, that content-integrity IS the hidden-Unicode
  scan, that manifest-parse is not a named check, and that the hook needs
  a completed apm install. The offline claim is qualified accordingly.
- N9 gates.md:142-146 verified to still match the hook description.

AGENTS.md:
- #12 the no-network session rule is qualified to a populated
  apm_modules/.

Audit note:
- A1 hook counts corrected to 27/9 -> 26/8 -> 27/9 -> 26/8 (26 and 8 at
  HEAD) and the dangling pointer dropped.
- A2 skill-size-check.sh is 509 lines with the resolver sourced, not 1,522
  embedded; citations repointed to skill-size-check.sh:323-335 and
  lib-checks-skill.sh:235-283 (fail() at :265 and :280), and that library
  is 627 lines.
- A3 consumers receive 15 test files across 5 skills; 16 tracked test
  paths repo-wide.
- A4 the "do not run apm update on this branch" instruction is marked
  superseded, with the branch-aware guidance in its place.
- Finding 31's "true orphans" claim corrected for HOTL and Sycophancy,
  both still used in core/ai-constitution.md.

Same class, found during group 2:
- skill-author's deployment-modes.md no longer points at .mcp.json
  configs (deleted in c96ca9c); metadata.version 1.0.2 -> 1.0.3.
- git-orchestrate's context contract clarifies that
  user_config_overrides is caller-supplied session state, not a config
  read. The field name is unchanged.
- B3 root apm.yml's executables.allow comment: grants are version-blind
  in apm 0.28.0, so the #2.0.0 suffix is cosmetic to apm and a bump does
  not break the hook; the suffix stays because
  check-executables-allow-sync.sh requires it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NwD8Egs5r4ndqeFLmhusX2
This commit is contained in:
Defame1297
2026-09-19 21:30:29 +00:00
parent 3920dfab20
commit 82b7bbcf5c
12 changed files with 131 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
docs/spec/architecture.md
View File

@@ -75,7 +75,7 @@ Both `CLAUDE.md` files are thin adapters: they import from their respective `AGE
This repo also has a `CLAUDE.md` at its root — the Claude Code entry point for working in this repo. It imports `AGENTS.md` and nothing else; there is no `@CONTEXT.md` import. It is not import-only either: below the import sits a fenced `<!-- rtk-instructions v2 -->` … `<!-- /rtk-instructions -->` block carrying the RTK command-prefix convention, which is tool-specific content with no `AGENTS.md` source. This is distinct from `providers/claude-code/CLAUDE.md`, which is the global config deployed to `~/.claude/`.
`CONTEXT.md` is therefore **not** always-loaded. `AGENTS.md` instructs agents to read it at session start, which is a behavioural instruction, not an `@import` guarantee — `LESSONS.md`'s 2026-05-17 entry proposed adding the import and it was never applied. Treat that entry as open work rather than a record of a landed change.
`CONTEXT.md` is therefore **not** always-loaded. `AGENTS.md` instructs agents to read it at session start, which is a behavioural instruction, not an `@import` guarantee.
## Reference conventions
@@ -87,4 +87,4 @@ The stated convention is that files referencing other files declare those refere
## Architectural decisions
Key hard-to-reverse decisions are recorded as ADRs in `docs/adr/`. There is no index file — the directory holds numbered ADRs whose filenames state their decision, so `ls docs/adr/` is the index. Read a superseding ADR before the one it supersedes: ADR-0015 (apm as the authoring source of truth) supersedes ADR-0001 and moots ADR-0006, ADR-0017 corrects ADR-0015's host-discovery gap, and ADR-0019 supersedes one claim in ADR-0018 (that `.claude/settings.json`'s committed content is exactly `{"hooks": {}}`) while keeping the rule behind it. Entry points for the structure described on this page: ADR-0002 (two-tier CLAUDE.md), ADR-0003 (AGENTS.md as the provider-agnostic entry point), ADR-0015 and ADR-0017 (the two compilers behind the plugin roots).
Key hard-to-reverse decisions are recorded as ADRs in `docs/adr/`. There is no index file — the directory holds numbered ADRs whose filenames state their decision, so `ls docs/adr/` is the index. Read a superseding ADR before the one it supersedes: ADR-0015 (apm as the authoring source of truth) supersedes ADR-0001 and moots ADR-0006, ADR-0024 supersedes ADR-0017 (which had corrected ADR-0015's host-discovery gap with a compiled flat content mirror, now deleted), and ADR-0019 supersedes one claim in ADR-0018 (that `.claude/settings.json`'s committed content is exactly `{"hooks": {}}`) while keeping the rule behind it. Entry points for the structure described on this page: ADR-0002 (two-tier CLAUDE.md), ADR-0003 (AGENTS.md as the provider-agnostic entry point), ADR-0015 (`apm pack`, the one compiler behind the plugin roots) and ADR-0024 (apm as the only install path).

88
docs/spec/gates.md
View File

@@ -403,10 +403,13 @@ findings.
### Duplicated constants
`factory-audit`'s `validate.sh` holds a second copy of the four ADR-0020 constants
(`DESC_SUGGEST_CHARS` / `DESC_MAX_CHARS` / `BODY_SUGGEST_WORDS` / `BODY_MAX_WORDS`) — the two
description constants apply to both artifact types it handles, the two body constants only to
skills. They are copied rather than imported because a skill's files may not reach outside that skill's own
`factory-audit` holds a second copy of the four ADR-0020 constants
(`DESC_SUGGEST_CHARS` / `DESC_MAX_CHARS` / `BODY_SUGGEST_WORDS` / `BODY_MAX_WORDS`). They are not in
its `validate.sh`, which carries none of them: they live in the mode libraries it sources —
`scripts/lib-checks-skill.sh:313-316` carries all four, and `scripts/lib-checks-agent.sh:164-165`
carries the two description constants only. That split is the contract stated directly: the two
description constants apply to both artifact types `factory-audit` handles, the two body constants
only to skills. They are copied rather than imported because a skill's files may not reach outside that skill's own
directory (the self-contained constraint in `docs/spec/architecture.md`, "Plugin model"), and
`scripts/skill-size-check.sh` does not ship with the plugin. `tests/test-skill-size-check.sh` asserts the copies agree,
so drift fails CI rather than silently letting an audit bless a skill the commit hook then rejects.
@@ -428,9 +431,14 @@ and duplicate the `skill)` arm's, and the file-wide count is still 2 and the ass
with the agent path running no resolver or some other one. It is now a **per-arm structural check** —
each arm of `validate.sh`'s `case "$MODE" in` block must carry exactly one `source` line inside its
own body, and the file must carry exactly those two — with a mutation self-test that performs that
exact count-preserving edit on a copy and requires the check to fail on it. The suite went 25 → 28
cases. It stood at 27 before the 2026-09-16 change and 29 after it, which replaced the two-copy
hash and its line-count floor with the six one-copy assertions above.
exact count-preserving edit on a copy and requires the check to fail on it. The suite's case count
runs **28 → 27 → 29**, and is **29** at HEAD: 28 at `620f20b` (the ADR-0025 merge), 27 after
`4de5b6b` retired the `.pre-commit-hooks.yaml` export, and 29 after `ef27c97` replaced the two-copy
hash and its line-count floor with the six one-copy assertions above. There are two 2026-09-16
changes here, not one, which is what an earlier revision of this section conflated. Each figure is
`bash tests/test-adr0020-contract.sh` run in a worktree at that commit, reading its `Results:` line.
An earlier revision also opened the chain at 25; that predates the branch squash, no reachable
commit reproduces it, and it is dropped as unverifiable rather than carried.
### `python3` and PyYAML are hard requirements
@@ -1030,14 +1038,29 @@ because the root-only invocation audits the marketplace manifest and **nothing e
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.
It verifies **exactly two things** per manifest and claims no more:
**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
names straight off its own compliance table:
- **manifest-parse** — each `apm.yml` parses as a valid APM manifest. Unconditional; verified to fire
on a dependency entry missing its `git`/`path`/`registry` field (`Cannot parse apm.yml`).
- **lockfile-exists** — any package declaring dependencies has a consistent `apm.lock.yaml`.
Conditional, and vacuous while every plugin `apm.yml` declares `dependencies: {apm: [], mcp: []}`;
it arms itself the moment one does not (verified by adding a git dependency to
`plugins/lint/apm.yml`).
On the **root** manifest, **10 checks**: `lockfile-exists`, `ref-consistency`,
`deployment-ledger-owners`, `deployed-files-present`, `no-orphaned-packages`,
`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.
**`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
`git`/`path`/`registry` field fails with `Cannot parse apm.yml` — but it fails the invocation before
the table is built rather than appearing as a row in it.
**The hook needs a completed `apm install`.** `deployed-files-present` checks the install output on
disk, so on a fresh clone it fails with `303 deployed file(s) missing` and takes the push with it.
That is not a defect in the gate; it is the gate correctly reporting that nothing has been installed
yet. Run `apm install` before the first push from a new checkout.
It does **not** enforce an org policy. apm discovers one from the git remote and only understands
github.com and Azure DevOps, so against this repo's self-hosted Gitea remote it prints:
@@ -1052,9 +1075,13 @@ not make the check meaningful, it makes it permanently red — `apm audit --ci`
`No org policy found at unknown (policy.fetch_failure_default=block)` on every push, forever. A gate
that can never go green is not a gate. Revisit only if this repo gains a policy source apm can reach.
It also does not scan for hidden Unicode: that scan is plain `apm audit`, a different mode (`--ci`
refuses to combine with `--file`/`--strip`/`--dry-run`/`PACKAGE`), and plain `apm audit` here reports
`No apm.lock.yaml found -- nothing to scan` and exits 0. Adding it would buy a second vacuous check.
**It does scan for hidden Unicode.** An earlier revision of this section said the opposite. The
`content-integrity` check in the root table *is* that scan — it reports `No critical hidden Unicode
or hash drift detected` — so the root invocation already covers it and nothing needs adding. What
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.
### `check-executables-allow-sync`
@@ -1092,9 +1119,12 @@ does not deploy and the replay does not compare; shared enforcement belongs in
### Why it is excluded from `pretty-format-json`
It is the **second and last alternation** in that hook's `exclude:` pattern, and the only one there
for a reason other than "generated manifest". Mind which number you are quoting: **two alternations,
expanding to two real files** — `.claude-plugin/marketplace.json`, plus this one.
It is in the **second and last alternation** in that hook's `exclude:` pattern, and that alternation
is the only one there for a reason other than "generated manifest". Mind which number you are
quoting: the pattern is `^(\.claude-plugin/marketplace\.json|\.claude/(settings|apm-hooks)\.json)$`
— **two top-level alternations, expanding to three real tracked files**:
`.claude-plugin/marketplace.json`, this one, and its committed `.claude/apm-hooks.json` sidecar,
which is apm output under the same byte-for-byte replay and is excluded for the same reason.
`pretty-format-json --autofix` sorts object keys unless `--no-sort-keys` is passed, while apm's hook
integrator emits insertion order (`matcher` before `hooks`, `type` before `command`). Leaving the
@@ -1108,11 +1138,18 @@ fix.
## Pushing without a network
No pre-push hook needs the network. Every entry in root `apm.yml`'s `marketplace.packages[]`
resolves from a local `./plugins/<name>` path, so `apm-pack-check-clean` never calls `git ls-remote`.
No pre-push hook needs the network **once `apm install` has populated `apm_modules/`**. Every entry
in root `apm.yml`'s `marketplace.packages[]` resolves from a local `./plugins/<name>` path, so
`apm-pack-check-clean` never calls `git ls-remote`.
`apm-audit-ci` calls `apm` too but was always local: its org-policy discovery resolves nothing on
this remote before any network call.
`apm-audit-ci` calls `apm` too, and its org-policy discovery resolves nothing on this remote before
any network call. But it is local only against a populated install: `drift` and `config-consistency`
replay the install to diff scratch against the working tree, and that replay is cache-only —
`[>] Replaying install (cache-only)` — which is exactly why it costs no network here. On a **fresh
clone** there is no cache to replay from, so the replay clones from the holocron remote and those two
checks fail offline, with `deployed-files-present` already failing for the same reason (see
`apm-audit-ci` above). The offline guarantee is a property of a populated `apm_modules/`, not of the
hook set: run `apm install` once on a new checkout and it holds from then on.
---
@@ -1131,7 +1168,8 @@ this remote before any network call.
styles
- `docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md` — the audit-pair merge that
collapsed the two Vale copies to one, removed the `check-vale-style-sync` hook, and took the shared
boundary resolver from three copies to two. It amends ADR-0014 and ADR-0020 on those points
boundary resolver from three copies to two (one since `ef27c97`). It amends ADR-0014 and ADR-0020
on those points
- `docs/spec/architecture.md` — directory structure, install pipeline, what is generated and what is
hand-authored
- `.pre-commit-config.yaml` — the hooks themselves, with inline rationale comments