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

Residual low-severity items from PR #95's fourth review round #97

New Issue
Closed
opened 2026-08-14 07:00:24 +00:00 by Claude · 1 comment
Claude commented 2026-08-14 07:00:24 +00:00
Collaborator
Copy Link

Six residuals from PR #95's final integration review. All Low or Informational; none gated that PR. Grouped here so they are not lost.

1. One new guard has no test (the round's only piece of decoration)

scripts/check-vale-style-sync.sh:74-79 — the unreadable-.vale.ini guard. Replacing if [[ ! -r "$ini" ]] with if false leaves the suite green.

Not fixed in PR #95 deliberately: the suite runs as root in the dev environment, and root's [[ -r ]] returns true on a chmod 000 file, so a mode-based test would pass or fail depending on invoking uid. A flaky test is worse than an untested three-line guard.

Needs either a non-uid-dependent test (e.g. a directory where a file is expected, or an unreadable path via a bind mount in CI) or an explicit in-file note that it is untested by design.

2. Stale # shellcheck source= directive silently disarms an exemption path

tests/run-tests.sh:61 declares source=lib/batch-run.sh, which resolves to neither $REPO_ROOT/lib/batch-run.sh nor tests/lib/batch-run.sh — both absent. The correct spelling is at tests/run-bats.sh:54 (../scripts/lib/batch-run.sh).

Consequence: the sourced_files() exemption in test-vale-wrap.sh's bash-3.2 scan cannot see batch-run.sh's array seeding when scanning run-tests.sh. No live miss today — both of run-tests.sh's expansions are already guarded and batch-run.sh is scanned directly — but the exemption path is untested against a real resolution failure, and shellcheck's SC1091 is below the repo's --severity=warning threshold so nothing flags it.

3. Stray content inside the new hooks/ directory is unchecked

sync-plugin-content.sh's checked_paths covers MIRROR_DIRS + hooks/hooks.json + the legacy hooks.json, but not the hooks/ directory itself. Measured:

  • a stray plugins/kyberforge/hooks/extra.json → --check exit 0, real sync leaves it
  • an empty plugins/core/hooks/ → --check exit 0, real sync leaves it

Check and sync agree in both cases, so the invariant holds and this is not a correctness break — but a stray file in a directory the mirror now owns is invisible to the gate. Contrast with a stray at the plugin root, which is out of scope by design (README.md, docs/, bin/, sources.md, .mcp.json are hand-authored).

4. prompts documented as mirrored but absent from MIRROR_DIRS

scripts/sync-plugin-content.sh:4 says it mirrors .apm/{agents,skills,prompts,commands,instructions,extensions,hooks}; :115 is MIRROR_DIRS=(agents skills commands instructions extensions).

A plugin adding .apm/prompts/ would silently never be mirrored, and --check would report clean. No plugin has one today. Either add it or correct the header.

5. Test suite is ~2× slower

Wall time 114s. The cost is real coverage (16 Vale override fixtures, each running vale), and for an always_run pre-push gate on a generator whose output ships to users that is a defensible trade — but worth knowing where it went:

script before after
test-check-vale-style-sync.sh 14s 51s
test-sync-plugin-content.sh 11s 37s
test-check-scope-walkup-sync.sh 2s 4s
test-vale-wrap.sh 14s 14s

test-check-vale-style-sync.sh is now the single wall-clock bottleneck.

6. AGENTS.md says 12 pre-push hooks; the command it recommends reports 14

The repo defines 12, but check-hooks-apply and check-useless-excludes (meta repo, no stages: restriction) also run at pre-push. A reader following the instruction sees a mismatch on the first try. Either say "12 repo-defined (14 including pre-commit's meta hooks)" or drop the count.

Suggested order: 6 and 2 are one-line fixes; 4 is a decision (add vs. correct); 1 and 3 need a little thought; 5 is informational unless the pre-push time becomes annoying.

Six residuals from PR #95's final integration review. All Low or Informational; none gated that PR. Grouped here so they are not lost. ## 1. One new guard has no test (the round's only piece of decoration) `scripts/check-vale-style-sync.sh:74-79` — the unreadable-`.vale.ini` guard. Replacing `if [[ ! -r "$ini" ]]` with `if false` leaves the suite green. Not fixed in PR #95 deliberately: the suite runs as root in the dev environment, and root's `[[ -r ]]` returns true on a `chmod 000` file, so a mode-based test would pass or fail depending on invoking uid. A flaky test is worse than an untested three-line guard. Needs either a non-uid-dependent test (e.g. a directory where a file is expected, or an unreadable path via a bind mount in CI) or an explicit in-file note that it is untested by design. ## 2. Stale `# shellcheck source=` directive silently disarms an exemption path `tests/run-tests.sh:61` declares `source=lib/batch-run.sh`, which resolves to neither `$REPO_ROOT/lib/batch-run.sh` nor `tests/lib/batch-run.sh` — both absent. The correct spelling is at `tests/run-bats.sh:54` (`../scripts/lib/batch-run.sh`). Consequence: the `sourced_files()` exemption in `test-vale-wrap.sh`'s bash-3.2 scan cannot see `batch-run.sh`'s array seeding when scanning `run-tests.sh`. No live miss today — both of `run-tests.sh`'s expansions are already guarded and `batch-run.sh` is scanned directly — but the exemption path is untested against a real resolution failure, and `shellcheck`'s SC1091 is below the repo's `--severity=warning` threshold so nothing flags it. ## 3. Stray content inside the new `hooks/` directory is unchecked `sync-plugin-content.sh`'s `checked_paths` covers `MIRROR_DIRS` + `hooks/hooks.json` + the legacy `hooks.json`, but not the `hooks/` directory itself. Measured: - a stray `plugins/kyberforge/hooks/extra.json` → `--check` exit 0, real sync leaves it - an empty `plugins/core/hooks/` → `--check` exit 0, real sync leaves it Check and sync **agree** in both cases, so the invariant holds and this is not a correctness break — but a stray file in a directory the mirror now owns is invisible to the gate. Contrast with a stray at the plugin root, which is out of scope by design (`README.md`, `docs/`, `bin/`, `sources.md`, `.mcp.json` are hand-authored). ## 4. `prompts` documented as mirrored but absent from `MIRROR_DIRS` `scripts/sync-plugin-content.sh:4` says it mirrors `.apm/{agents,skills,prompts,commands,instructions,extensions,hooks}`; `:115` is `MIRROR_DIRS=(agents skills commands instructions extensions)`. A plugin adding `.apm/prompts/` would silently never be mirrored, and `--check` would report clean. No plugin has one today. Either add it or correct the header. ## 5. Test suite is ~2× slower Wall time 114s. The cost is real coverage (16 Vale override fixtures, each running vale), and for an `always_run` pre-push gate on a generator whose output ships to users that is a defensible trade — but worth knowing where it went: | script | before | after | |---|---|---| | `test-check-vale-style-sync.sh` | 14s | 51s | | `test-sync-plugin-content.sh` | 11s | 37s | | `test-check-scope-walkup-sync.sh` | 2s | 4s | | `test-vale-wrap.sh` | 14s | 14s | `test-check-vale-style-sync.sh` is now the single wall-clock bottleneck. ## 6. `AGENTS.md` says 12 pre-push hooks; the command it recommends reports 14 The repo defines 12, but `check-hooks-apply` and `check-useless-excludes` (meta repo, no `stages:` restriction) also run at pre-push. A reader following the instruction sees a mismatch on the first try. Either say "12 repo-defined (14 including pre-commit's meta hooks)" or drop the count. --- Suggested order: 6 and 2 are one-line fixes; 4 is a decision (add vs. correct); 1 and 3 need a little thought; 5 is informational unless the pre-push time becomes annoying.
Claude added the Kind/Enhancement
Priority
Low
4
Reviewed
Confirmed
1
 labels 2026-08-14 07:00:38 +00:00
Claude commented 2026-08-14 09:28:56 +00:00
Author
Collaborator
Copy Link

All six items addressed on feat/90-execute-apm-conversion, folded into PR #95 and disclosed there. Three of the six did not survive verification as written — details below, because two of them would send the next reader down a wrong path.

item commit outcome
1 — untested -r guard c442f7e Fixed — but the guard was dead, not merely untested
2 — stale source= directive 73393b9 Fixed — the suggested fix was also wrong; a third instance found
3 — stray content in hooks/ 5a61b41 Fixed
4 — prompts missing from MIRROR_DIRS 5a61b41 Refuted — not a defect
5 — suite ~2× slower c442f7e Fixed locally, net regression overall
6 — hook count 12 vs 14 0f0ac58 Fixed

Item 4 — refuted, no MIRROR_DIRS change made

A plugin adding .apm/prompts/ would silently never be mirrored

It would be mirrored. MIRROR_DIRS lists destination directory names at the plugin root; the header comment lists source directories under .apm/. The two lists are legitimately different, and prompts is the one entry where they diverge: apm folds .apm/prompts/ into commands/ alongside .apm/commands/, renaming *.prompt.md → *.md. ADR-0017:49 already documents this mapping.

Verified empirically rather than read off the ADR — a fixture carrying .apm/prompts/greet.prompt.md produces commands/greet.md and no prompts/ directory at the plugin root.

Adding the prompts entry this item asks for would have named an output directory apm never emits and no plugin host scans. Instead: a comment at MIRROR_DIRS explaining why the lists differ, and a characterization test pinning the mapping — so an apm upgrade that gave prompts a destination of its own fails loudly, which is the only condition under which the entry would actually be needed.

Item 2 — the recommended spelling is also broken

The correct spelling is at tests/run-bats.sh:54 (../scripts/lib/batch-run.sh)

It is not. Directives resolve against the source-path, which under pre-commit is the repo root, so ../scripts/... escapes the repo:

tests/run-bats.sh:
  SC1091 (info): Not following: ../scripts/lib/batch-run.sh: does not exist

Both spellings were broken. run-bats.sh only looked correct because test-vale-wrap.sh's sourced_files() tries the script's own directory as a second candidate — a heuristic shellcheck itself does not share. The spelling that satisfies both is repo-root-relative (scripts/lib/batch-run.sh), matching scripts/install.sh:5.

A third instance the issue does not mention, scripts/check-manifests.sh:49, was broken the same way. All now resolve, asserted by a new case that holds every directive in the scanned corpus to the resolution rule — so the next stale one fails at test time rather than lying dormant. Confirmed it kills the original mutation: restoring the old spelling gives FAIL … tests/run-tests.sh (0/1).

Item 1 — the guard was dead code

The issue frames this as untestable-because-root. The sharper statement is that it was unreachable because of root:

[[ -r ]] is access(2) — it reports whether the permission bits would allow a read. For uid 0 that is yes even on a mode-000 file (verified: [[ -r ]] true and cat succeeds). This hook runs at pre-push and the environment is root, so the guard could never fire in the one place it exists to fire. There was no uid-independent test for it because there was nothing to test.

Readability is now decided by actually reading (cat), which is uid-independent and strictly stronger — it catches EISDIR and EIO, which access(2) reports on neither. cat, not a < "$ini" redirect: opening a directory for reading succeeds, only the read fails. The missing branch moved to -e, so a directory sitting where the file belongs is reported as unreadable rather than as deleted.

The new case asserts the message, not the exit code. With the guard removed the script still exits 1 — the greps hit the unreadable path and report a missing StylesPath for a file that has one. An exit-code-only test would have been green with the guard deleted, i.e. exactly the failure mode this repo keeps finding.

Ruled out by experiment, not assumption: directory / broken symlink / symlink loop all fail -f and get caught by the preceding branch; /proc/self/mem is Linux-only and made grep hang; privilege-drop is Linux-only and would drag the whole fixture tree's permissions into scope.

Item 5 — fixed locally, but the suite got slower overall

test-check-vale-style-sync.sh: 51s → 32s, by masking vale in the 21 of 28 script runs that only assert .vale.ini text (the PATH_NO_VALE mechanism case 12 already builds). The helper falls back to an unmasked run rather than skipping, so a machine where masking is unavailable loses speed, never coverage.

But the suite went 114s → 129s. The saving was more than eaten by the tests added for items 1 and 3: test-sync-plugin-content.sh went 37s → 64s, since each new hooks/-stray and prompts case runs a real apm pack. The bottleneck moved from test-check-vale-style-sync.sh to test-sync-plugin-content.sh rather than disappearing. Recording this plainly because "3.5× faster" is true of the file and false of the suite.

The masking turned out to matter more for coverage than for time. With vale on PATH, cases 8 and 9 could not detect deletion of the assertions they were written to catch: a dropped StylesPath also breaks the glob probe, so the script exited 1 for the wrong reason and both cases went green. Verified against the pre-change files — the same mutation was caught by one incidental assertion before, and by three after.

Remaining unclaimed win: cases 11 and 11b need no vale either (~2 more runs, ~7%).

Item 3 — fixed, and it immediately found real strays

hooks/ is now wiped and rebuilt like every MIRROR_DIRS destination, and the directory itself (not just hooks/hooks.json) is in checked_paths, so the recursive manifest sees one-sided entries. Both measured cases in the issue now report drift and are cleaned by a real sync.

On first run the new gate flagged three empty plugins/{git,gitea,core}/hooks/ directories. Untracked local cruft rather than anything committed — git does not track empty directories, so a fresh clone never had them — but the gate working on its first outing.

One defect introduced while fixing this and caught before commit: the new rm -rf "$target_dir/$HOOKS_DIR_REL" tripped SC2115. set -u aborts on an unset variable but not an empty one, so an empty $target_dir would make it rm -rf /hooks. Guarded with ${target_dir:?}.

Item 6

AGENTS.md now says 12 repo-defined hooks and explains that pre-commit's own meta hooks (check-hooks-apply, check-useless-excludes) declare no stages: and so also run at pre-push, which is why the recommended command reports 14.

Verification: full suite 16/16, 0 failures; all 14 pre-push hooks pass including apm marketplace check, apm audit --ci, apm pack --check-clean and both claude-CLI validators; shellcheck --severity=warning clean on all nine changed shell files. Every new guard was mutation-tested to fail under the defect it exists to catch, not merely to pass.

All six items addressed on `feat/90-execute-apm-conversion`, folded into PR #95 and disclosed there. **Three of the six did not survive verification as written** — details below, because two of them would send the next reader down a wrong path. | item | commit | outcome | |---|---|---| | 1 — untested `-r` guard | `c442f7e` | Fixed — but the guard was **dead**, not merely untested | | 2 — stale `source=` directive | `73393b9` | Fixed — the **suggested fix was also wrong**; a third instance found | | 3 — stray content in `hooks/` | `5a61b41` | Fixed | | 4 — `prompts` missing from `MIRROR_DIRS` | `5a61b41` | **Refuted** — not a defect | | 5 — suite ~2× slower | `c442f7e` | Fixed locally, **net regression overall** | | 6 — hook count 12 vs 14 | `0f0ac58` | Fixed | --- ## Item 4 — refuted, no `MIRROR_DIRS` change made > A plugin adding `.apm/prompts/` would silently never be mirrored It would be mirrored. `MIRROR_DIRS` lists **destination** directory names at the plugin root; the header comment lists **source** directories under `.apm/`. The two lists are legitimately different, and `prompts` is the one entry where they diverge: apm folds `.apm/prompts/` into `commands/` alongside `.apm/commands/`, renaming `*.prompt.md` → `*.md`. ADR-0017:49 already documents this mapping. Verified empirically rather than read off the ADR — a fixture carrying `.apm/prompts/greet.prompt.md` produces `commands/greet.md` and no `prompts/` directory at the plugin root. Adding the `prompts` entry this item asks for would have named an output directory apm never emits and no plugin host scans. Instead: a comment at `MIRROR_DIRS` explaining why the lists differ, and a characterization test pinning the mapping — so an apm upgrade that gave `prompts` a destination of its own fails loudly, which is the only condition under which the entry would actually be needed. ## Item 2 — the recommended spelling is also broken > The correct spelling is at `tests/run-bats.sh:54` (`../scripts/lib/batch-run.sh`) It is not. Directives resolve against the source-path, which under pre-commit is the repo root, so `../scripts/...` escapes the repo: ``` tests/run-bats.sh: SC1091 (info): Not following: ../scripts/lib/batch-run.sh: does not exist ``` Both spellings were broken. `run-bats.sh` only *looked* correct because `test-vale-wrap.sh`'s `sourced_files()` tries the script's own directory as a second candidate — a heuristic shellcheck itself does not share. The spelling that satisfies both is repo-root-relative (`scripts/lib/batch-run.sh`), matching `scripts/install.sh:5`. A third instance the issue does not mention, `scripts/check-manifests.sh:49`, was broken the same way. All now resolve, asserted by a new case that holds every directive in the scanned corpus to the resolution rule — so the next stale one fails at test time rather than lying dormant. Confirmed it kills the original mutation: restoring the old spelling gives `FAIL … tests/run-tests.sh (0/1)`. ## Item 1 — the guard was dead code The issue frames this as untestable-because-root. The sharper statement is that it was **unreachable** because of root: `[[ -r ]]` is `access(2)` — it reports whether the permission bits *would* allow a read. For uid 0 that is yes even on a mode-000 file (verified: `[[ -r ]]` true and `cat` succeeds). This hook runs at pre-push and the environment is root, so the guard could never fire in the one place it exists to fire. There was no uid-independent test for it because there was nothing to test. Readability is now decided by actually reading (`cat`), which is uid-independent and strictly stronger — it catches `EISDIR` and `EIO`, which `access(2)` reports on neither. `cat`, not a `< "$ini"` redirect: opening a directory for reading *succeeds*, only the read fails. The missing branch moved to `-e`, so a directory sitting where the file belongs is reported as unreadable rather than as deleted. The new case asserts the **message**, not the exit code. With the guard removed the script still exits 1 — the greps hit the unreadable path and report a missing `StylesPath` for a file that has one. An exit-code-only test would have been green with the guard deleted, i.e. exactly the failure mode this repo keeps finding. Ruled out by experiment, not assumption: directory / broken symlink / symlink loop all fail `-f` and get caught by the preceding branch; `/proc/self/mem` is Linux-only and made `grep` hang; privilege-drop is Linux-only and would drag the whole fixture tree's permissions into scope. ## Item 5 — fixed locally, but the suite got slower overall `test-check-vale-style-sync.sh`: **51s → 32s**, by masking vale in the 21 of 28 script runs that only assert `.vale.ini` text (the `PATH_NO_VALE` mechanism case 12 already builds). The helper falls back to an unmasked run rather than skipping, so a machine where masking is unavailable loses speed, never coverage. **But the suite went 114s → 129s.** The saving was more than eaten by the tests added for items 1 and 3: `test-sync-plugin-content.sh` went **37s → 64s**, since each new `hooks/`-stray and `prompts` case runs a real `apm pack`. The bottleneck moved from `test-check-vale-style-sync.sh` to `test-sync-plugin-content.sh` rather than disappearing. Recording this plainly because "3.5× faster" is true of the file and false of the suite. The masking turned out to matter more for **coverage** than for time. With vale on `PATH`, cases 8 and 9 could not detect deletion of the assertions they were written to catch: a dropped `StylesPath` also breaks the glob probe, so the script exited 1 for the wrong reason and both cases went green. Verified against the pre-change files — the same mutation was caught by one incidental assertion before, and by three after. Remaining unclaimed win: cases 11 and 11b need no vale either (~2 more runs, ~7%). ## Item 3 — fixed, and it immediately found real strays `hooks/` is now wiped and rebuilt like every `MIRROR_DIRS` destination, and the directory itself (not just `hooks/hooks.json`) is in `checked_paths`, so the recursive manifest sees one-sided entries. Both measured cases in the issue now report drift and are cleaned by a real sync. On first run the new gate flagged three empty `plugins/{git,gitea,core}/hooks/` directories. Untracked local cruft rather than anything committed — git does not track empty directories, so a fresh clone never had them — but the gate working on its first outing. One defect introduced while fixing this and caught before commit: the new `rm -rf "$target_dir/$HOOKS_DIR_REL"` tripped `SC2115`. `set -u` aborts on an *unset* variable but not an *empty* one, so an empty `$target_dir` would make it `rm -rf /hooks`. Guarded with `${target_dir:?}`. ## Item 6 AGENTS.md now says 12 repo-defined hooks and explains that `pre-commit`'s own `meta` hooks (`check-hooks-apply`, `check-useless-excludes`) declare no `stages:` and so also run at pre-push, which is why the recommended command reports 14. --- **Verification:** full suite 16/16, 0 failures; all 14 pre-push hooks pass including `apm marketplace check`, `apm audit --ci`, `apm pack --check-clean` and both `claude`-CLI validators; `shellcheck --severity=warning` clean on all nine changed shell files. Every new guard was mutation-tested to **fail** under the defect it exists to catch, not merely to pass.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label Kind/Enhancement
Priority
Low
4
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Claude renovate-bot Defame1297
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Defame1297/holocron#97
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?