Files
holocron/scripts/check-executables-allow-sync.sh
Defame1297 ffcbed6c41 fix(tests): replace pipefail-racy echo | grep -q with here-strings
Why

Two suites failed intermittently — tests/test-vale-wrap.sh case 21 and
tests/test-check-release-needed.sh cases 4 and 15 — on correct output, and never
when run alone. The cause is the `echo "$OUT" | grep -q P` idiom under
`set -o pipefail`: grep -q exits as soon as it has an answer, bash's echo can
hand a multi-line value to the pipe one line at a time, and a write after the
reader is gone kills echo with SIGPIPE. pipefail then reports the writer's
death, so output that DID match reads as "no match". Every observed failure had
lines after its match; case 15's match is on line 1 of 6, the widest window in
that file.

Forced with a pause before the writer's last line, the pipe form failed 50 of 50
runs; a here-string, a match on the last line, and the same pipe without
pipefail each passed 50 of 50. Unforced the rate is about 1 per 670 suite runs,
which is why it read as a flaky gate rather than a bug.

The failures at review time are consistent with this, but were not proven to be
it: the suite was running while agents edited live config files in place, and a
brief change to .vale.ini or .pre-commit-hooks.yaml would produce the same two
failures. The race is real and fixed either way.

Implementation Notes

`grep -q P <<< "$VAR"` has no separate writer process, so there is nothing to
race. It is not a retry or a sleep. 121 sites converted across 9 files, three of
them scripts rather than tests: new-agent.sh, new-skill.sh and
check-executables-allow-sync.sh. None ships via .pre-commit-hooks.yaml, so no
external consumer pins them, and all three are single-pipeline checks whose
verdict cannot change.

Left alone deliberately: 14 sites whose writer is a command, not a shell
builtin — they either absorb the writer's status with `|| true` or are python3
and awk, which write once at exit — and one file with no pipefail. `printf '%s'`
sites differ from a here-string only by a trailing newline, which no -q verdict
on a non-empty pattern depends on.

tests/test-no-pipefail-early-exit-grep.sh is a static guard against new
occurrences, discovered automatically by run-tests.sh. It only scans files that
set pipefail, joins continuation lines, skips comments, and flags only
echo/printf writers. Its first case proves the scanner can fail before its
second trusts a clean verdict on the tree.

A guard covers exactly the spellings its regex models, so the miss surface was
measured rather than assumed. Four were found and closed: pipefail declared as
`set -o errexit -o pipefail` (where the old pattern required pipefail to follow
the FIRST -o, and a file-level miss skips every site in that file); a writer
separated from grep by an intermediate stage; a pipeline wrapped on a trailing
`|` rather than a backslash; and readers spelled egrep, fgrep, /bin/grep,
`command grep` or with an env-var prefix. Segment characters exclude a bare `&`
so `echo ok && other | grep -q x`, whose writer is `other`, does not false-fire.
Widening surfaced 5 live sites invisible to the original scanner, all in
tests/test-apm-current-hook.sh, all `echo "$out" | json_field ... | grep -q`;
they are safe today only because json_field is python3, which reads to EOF and
writes once. Fixtures go 4 to 12 vulnerable spellings plus near-miss negatives.

Two `grep ... | head -1` sites (test-vale-wrap.sh) are the same race with a
different early-exiting reader, and are fixed by absorbing the writer. The
scanner deliberately does not model `head`, `sed -n 1p` or a bare `read`: most
legitimate uses in this tree are already absorbed with `|| true` and the scanner
cannot see absorption from pipeline text, so a high false-positive rate would be
how this guard gets weakened. Heredoc bodies are scanned as code; none in the
tree trips it today.

Impact

The bug predates the factory-audit merge: every converted site in
check-release-needed and case 21 dates to 4d018af and aa8cc22 (2026-08-09).

Test suites go 19 to 20. `run-tests.sh --strict` passes 20/20 with 0 skipped,
four consecutive runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD
2026-09-16 09:14:01 +00:00

223 lines
8.7 KiB
Bash
Executable File

#!/usr/bin/env bash
set -euo pipefail
# Fails the push when root apm.yml's executables.allow key stops naming
# kyberforge's actual version. apm matches that key by exact dict lookup
# (apm_cli/security/executables.py) — a version bump that misses the key
# update deploys nothing, with no error anywhere. See ADR-0019, "The allow
# key is version-pinned, and that is a live failure mode", for the full
# argument; nothing else in the pre-push gate compares these two files.
#
# Run from repo root or pass REPO_ROOT as arg.
REPO_ROOT="${1:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}"
# A nonexistent REPO_ROOT fails loudly rather than falling through to the
# "no kyberforge plugin here" no-op below: that no-op is for a repo that
# legitimately does not ship the plugin, not for a typo'd or stale path, and
# exit 0 would read as "checked, in sync" when nothing was compared at all.
if [[ ! -d "$REPO_ROOT" ]]; then
echo "FAIL: executables-allow sync check: REPO_ROOT '$REPO_ROOT' is not a directory." >&2
echo " Fix: run this from the repo root, or pass a real repo path as the first argument." >&2
exit 1
fi
REPO_ROOT="$(cd "$REPO_ROOT" && pwd)"
PLUGIN_DIR="$REPO_ROOT/plugins/kyberforge"
PLUGIN_MANIFEST="$PLUGIN_DIR/apm.yml"
ROOT_MANIFEST="$REPO_ROOT/apm.yml"
# Only a repo with no kyberforge plugin at all is a legitimate no-op — there is
# no version to pin and no hook to deploy.
if [[ ! -d "$PLUGIN_DIR" ]]; then
exit 0
fi
if [[ ! -f "$PLUGIN_MANIFEST" ]]; then
echo "FAIL: plugins/kyberforge/ exists but has no apm.yml, so its version cannot be read." >&2
echo " Why: this gate compares that version against root apm.yml's executables.allow key;" >&2
echo " without the manifest it would exit 0 having compared nothing." >&2
echo " Fix: restore plugins/kyberforge/apm.yml, or remove plugins/kyberforge/ entirely." >&2
exit 1
fi
if [[ ! -f "$ROOT_MANIFEST" ]]; then
echo "FAIL: root apm.yml is missing, so kyberforge's executables allow key cannot be verified." >&2
echo " Why: apm reads executables.allow from the consuming package's manifest; with no root" >&2
echo " manifest nothing grants kyberforge's hooks/ and bin/ permission to deploy." >&2
echo " Fix: restore apm.yml at the repo root." >&2
exit 1
fi
# ---------------------------------------------------------------------------
# Manifest facts
# ---------------------------------------------------------------------------
# Both readers emit the same line protocol, so the rest of the script does not
# care which ran:
#
# v<TAB><kyberforge version> exactly once
# a<TAB>present|absent exactly once — is executables.allow a mapping?
# k<TAB><allow key> zero or more
#
# python3 + PyYAML is preferred where importable; the fallback below is not a
# YAML parser, it recognises only the two shapes these manifests use. PyYAML
# stays optional so a missing pip package can't block every push (ADR-0019).
read_facts_python() {
python3 - "$ROOT_MANIFEST" "$PLUGIN_MANIFEST" << 'PY'
import sys
import yaml
def load(path):
with open(path) as fh:
data = yaml.safe_load(fh)
return data if isinstance(data, dict) else {}
root, plugin = load(sys.argv[1]), load(sys.argv[2])
version = plugin.get("version")
# A 2-component version parses as a YAML float and would render back as e.g.
# "1.5" from 1.50 — a mismatch invented by the reader rather than found in the
# files. Refuse instead of guessing; semver keys here are always strings.
if version is not None and not isinstance(version, str):
sys.stderr.write(
"kyberforge's version: is not a string (%r) — quote it so the "
"executables.allow key can be compared verbatim.\n" % (version,)
)
raise SystemExit(2)
print("v\t%s" % ("" if version is None else version))
executables = root.get("executables")
allow = executables.get("allow") if isinstance(executables, dict) else None
if isinstance(allow, dict):
print("a\tpresent")
for key in allow:
print("k\t%s" % key)
else:
print("a\tabsent")
PY
}
# Strips one layer of matching quotes plus surrounding whitespace from a scalar.
unquote() {
local value="$1"
read -r value <<< "$value"
case "$value" in
\"*\") value="${value#\"}"; value="${value%\"}" ;;
\'*\') value="${value#\'}"; value="${value%\'}" ;;
esac
printf '%s\n' "$value"
}
read_facts_bash() {
local line version="" allow_state="absent" in_executables=0 in_allow=0 key
while IFS= read -r line || [[ -n "$line" ]]; do
[[ "$line" == version:* ]] || continue
version="$(unquote "${line#version:}")"
break
done < "$PLUGIN_MANIFEST"
while IFS= read -r line || [[ -n "$line" ]]; do
# Blank and full-line comments carry no structure at any depth.
case "$line" in
'' | '#'*) continue ;;
esac
# A top-level key (column 0) closes whatever block was open. Checked before
# anything else so `marketplace:` after `executables:` cannot leak keys in.
if [[ "$line" != [[:space:]]* ]]; then
if [[ "$line" == executables:* ]]; then
in_executables=1
else
in_executables=0
fi
in_allow=0
continue
fi
[[ $in_executables -eq 1 ]] || continue
# 2-space indent: a key directly under executables:. `allow:` opens the
# mapping this gate reads; any sibling key closes it.
if [[ "$line" =~ ^\ \ [^[:space:]#] ]]; then
if [[ "$line" =~ ^\ \ allow: ]]; then
in_allow=1
allow_state="present"
else
in_allow=0
fi
continue
fi
# 4-space indent while inside allow: — an allow key. Keys contain '#' by
# construction ("kyberforge#1.5.0"), so a trailing-comment strip would eat
# them; there is none, and inline comments are not used on these lines.
if [[ $in_allow -eq 1 && "$line" =~ ^\ \ \ \ ([^[:space:]#][^:]*): ]]; then
key="$(unquote "${BASH_REMATCH[1]}")"
printf 'k\t%s\n' "$key"
fi
done < "$ROOT_MANIFEST"
printf 'v\t%s\n' "$version"
printf 'a\t%s\n' "$allow_state"
}
if command -v python3 > /dev/null 2>&1 && python3 -c 'import yaml' > /dev/null 2>&1; then
READER="python3 + PyYAML"
if ! FACTS="$(read_facts_python)"; then
echo "FAIL: could not read apm.yml / plugins/kyberforge/apm.yml (see the parser error above)." >&2
echo " Why: this gate compares kyberforge's version against root apm.yml's executables.allow" >&2
echo " key; an unreadable manifest means the comparison did not happen." >&2
echo " Fix: make both manifests valid YAML, then re-run." >&2
exit 1
fi
else
READER="shape-scan fallback (PyYAML unavailable)"
FACTS="$(read_facts_bash)"
fi
VERSION="$(printf '%s\n' "$FACTS" | sed -n 's/^v\t//p')"
ALLOW_STATE="$(printf '%s\n' "$FACTS" | sed -n 's/^a\t//p')"
ALLOW_KEYS="$(printf '%s\n' "$FACTS" | sed -n 's/^k\t//p')"
if [[ -z "$VERSION" ]]; then
echo "FAIL: plugins/kyberforge/apm.yml declares no version:, so no allow key can be checked against it." >&2
echo " Why: apm's executables.allow lookup is keyed on '<package>#<version>' exactly; with no" >&2
echo " version there is nothing for root apm.yml's key to stay in sync with." >&2
echo " Fix: give plugins/kyberforge/apm.yml a top-level version: (read by $READER)." >&2
exit 1
fi
EXPECTED_KEY="kyberforge#$VERSION"
if [[ "$ALLOW_STATE" != "present" ]]; then
echo "FAIL: root apm.yml has no executables.allow mapping, but plugins/kyberforge is version $VERSION." >&2
echo " Why: without an allow entry apm refuses to deploy kyberforge's hooks/ and bin/, so the" >&2
echo " SessionStart hook that keeps this install level with the remote never runs and the" >&2
echo " deployed skills go stale silently (ADR-0019)." >&2
echo " Fix: add to root apm.yml:" >&2
echo " executables:" >&2
echo " allow:" >&2
echo " $EXPECTED_KEY:" >&2
echo " hooks: true" >&2
echo " bin: true" >&2
exit 1
fi
if grep -qxF "$EXPECTED_KEY" <<< "$ALLOW_KEYS"; then
exit 0
fi
STALE_KEYS="$(printf '%s\n' "$ALLOW_KEYS" | grep '^kyberforge#' || true)"
echo "FAIL: root apm.yml's executables.allow has no '$EXPECTED_KEY' key, but that is kyberforge's version." >&2
if [[ -n "$STALE_KEYS" ]]; then
echo " Found instead:" >&2
printf '%s\n' "$STALE_KEYS" | sed 's/^/ /' >&2
fi
echo " Why: apm matches this key by exact dict lookup — there is no wildcard and no version-less" >&2
echo " form — so a key naming any other version silently stops granting kyberforge's hooks/" >&2
echo " and bin/. The SessionStart hook then stops deploying and the apm install goes stale" >&2
echo " with no error anywhere (ADR-0019, 'The allow key is version-pinned')." >&2
echo " Fix: bump the key in root apm.yml to '$EXPECTED_KEY:' — the version bump in" >&2
echo " plugins/kyberforge/apm.yml is not complete without it." >&2
exit 1