fix: reap only our own jobs, and say why a vale probe missed
batch_run ended with a bare wait, which blocks on every background job the calling shell has, not the ones it started. Harmless for all three current callers, but a future caller that backgrounds anything of its own would have batch_run block on it or consume its status. It now records each $! and reaps exactly those PIDs. The `wait "$pid" || true` there is load-bearing: unlike a bare wait, wait <pid> returns the job's status, so without it a single failing job would abort the set -e caller at the call site -- before run-tests.sh or sync-plugin-content.sh could read their .status files and print a summary. Status semantics stay in those files, exactly as before. check-vale-style-sync.sh's glob probe discarded vale's exit code and output and decided purely on a grep, so a failed exec, an OOM-killed vale or a full TMPDIR was indistinguishable from a real glob defect -- both printed "its glob sections do not cover a path" with no evidence. A flake seen once in this probe could not be diagnosed afterwards for that reason. The probe now attaches vale's rc and output: a genuine glob defect reads "vale exited 0 ... in 0 files", a killed vale reads "vale exited 137; output: <empty>". That flake was investigated and not reproduced -- 1680 probes across three contention setups including an offline namespace, all clean -- so nothing is changed speculatively. The misattribution is worth recording: it was reported against tests/test-vale-wrap.sh, which never invokes this script; the assertion belongs to check-vale-style-sync.sh and reaches a log through a different suite. Also drops the last stale field roster from agent-author's scaffolder. Its next-steps hint enumerated "(name, description, model, body only)" -- omitting disallowedTools, and never accurate anyway, since the template marks only description and the body FILL IN. Its --help carried the inverted form, already missing six forbidden fields. Both now state the shape rule and point at field-inventory.md, and a bats case enforces all-or-nothing: name every allowlisted field or name none, since a partial roster is the shape that goes stale silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -15,6 +15,7 @@ All scripts in this skill must follow these rules:
|
|||||||
- **Idempotent** — "create if not exists" per file. The scaffold script skips any file that already exists; agents may safely re-run it.
|
- **Idempotent** — "create if not exists" per file. The scaffold script skips any file that already exists; agents may safely re-run it.
|
||||||
- **Meaningful exit codes** — `0` success, `1` invalid arguments or precondition failure. Document in `--help`.
|
- **Meaningful exit codes** — `0` success, `1` invalid arguments or precondition failure. Document in `--help`.
|
||||||
- **Self-contained** — no external package installs at runtime. The script uses only bash builtins and POSIX tools (`sed`, `mkdir`, `cat`).
|
- **Self-contained** — no external package installs at runtime. The script uses only bash builtins and POSIX tools (`sed`, `mkdir`, `cat`).
|
||||||
|
- **No restated field rosters** — no script output, in `--help` or in next-steps guidance, enumerates permitted, forbidden, or required frontmatter fields. Point at the `apm-agent-allowlist` section of `agent-audit`'s `references/field-inventory.md`, which `agent-audit`'s `validate.sh` reads from there as data. A roster copied into script output goes stale one step further out than the list itself: the next-steps hint `(name, description, model, body only)` kept printing after ADR-0016's 2026-08-14 amendment added `disallowedTools` to the permitted set. `tests/new-agent.bats` enforces this for the plugin/APM branch — naming some allowlisted fields but not all is a failure.
|
||||||
|
|
||||||
## Template variables
|
## Template variables
|
||||||
|
|
||||||
|
|||||||
@@ -21,10 +21,12 @@ Arguments:
|
|||||||
without type: is a marketplace-only manifest
|
without type: is a marketplace-only manifest
|
||||||
and is skipped, the walk continues upward
|
and is skipped, the walk continues upward
|
||||||
→ creates <package-root>/.apm/agents/<name>.agent.md
|
→ creates <package-root>/.apm/agents/<name>.agent.md
|
||||||
(single vendor-neutral file — no tools,
|
(single vendor-neutral file; apm compile copies
|
||||||
isolation, maxTurns, effort, memory, or
|
its frontmatter verbatim to every target with no
|
||||||
permissionMode; apm compile has no per-target
|
per-target field integrator, so the permitted
|
||||||
field integrator, see ADR-0016)
|
field set is narrow — see the apm-agent-allowlist
|
||||||
|
section of agent-audit's
|
||||||
|
references/field-inventory.md and ADR-0016)
|
||||||
→ creates <package-root>/sources.md (if absent)
|
→ creates <package-root>/sources.md (if absent)
|
||||||
project scope : no type:-bearing apm.yml found; root is a
|
project scope : no type:-bearing apm.yml found; root is a
|
||||||
project directory
|
project directory
|
||||||
@@ -258,6 +260,19 @@ SOURCES
|
|||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
|
|
||||||
|
# Next-steps guidance names no frontmatter fields, by rule (see references/scripts.md).
|
||||||
|
# A roster restated in terminal output goes stale one step further out than the list
|
||||||
|
# itself: the old "(name, description, model, body only)" hint outlived ADR-0016's
|
||||||
|
# 2026-08-14 amendment, which added disallowedTools to the permitted set. Point at the
|
||||||
|
# scaffolded file's own comments for what to fill, and at agent-audit's validate.sh —
|
||||||
|
# which reads the allowlist from field-inventory.md as data — for what is permitted.
|
||||||
|
AUDIT_SCRIPTS="$(cd "$SKILL_ROOT/../agent-audit/scripts" 2>/dev/null && pwd || true)"
|
||||||
|
if [[ -n "$AUDIT_SCRIPTS" && -f "$AUDIT_SCRIPTS/validate.sh" ]]; then
|
||||||
|
VALIDATE_HINT="$AUDIT_SCRIPTS/validate.sh"
|
||||||
|
else
|
||||||
|
VALIDATE_HINT="agent-audit's scripts/validate.sh"
|
||||||
|
fi
|
||||||
|
|
||||||
if [[ "$created_any" == false ]]; then
|
if [[ "$created_any" == false ]]; then
|
||||||
echo "All files already exist — nothing to do." >&2
|
echo "All files already exist — nothing to do." >&2
|
||||||
else
|
else
|
||||||
@@ -266,12 +281,17 @@ else
|
|||||||
echo "" >&2
|
echo "" >&2
|
||||||
echo "Next steps:" >&2
|
echo "Next steps:" >&2
|
||||||
if [[ "$SCOPE" == "plugin" ]]; then
|
if [[ "$SCOPE" == "plugin" ]]; then
|
||||||
echo " 1. Fill in $APM_FILE — replace all FILL IN: placeholders (name, description, model, body only)" >&2
|
echo " 1. Fill in $APM_FILE — replace every FILL IN: placeholder. Optional fields are" >&2
|
||||||
|
echo " scaffolded there as commented blocks; uncomment the ones that apply." >&2
|
||||||
echo " 2. Populate $SOURCES_DIR/sources.md with research sources, or delete it" >&2
|
echo " 2. Populate $SOURCES_DIR/sources.md with research sources, or delete it" >&2
|
||||||
echo " 3. Validate: check required fields (name, description, system prompt) in the file" >&2
|
echo " 3. Validate: $VALIDATE_HINT $APM_FILE" >&2
|
||||||
|
echo " It checks the frontmatter against the apm-agent-allowlist section of" >&2
|
||||||
|
echo " agent-audit's references/field-inventory.md, the authoritative field list." >&2
|
||||||
else
|
else
|
||||||
echo " 1. Fill in $CC_FILE — replace all FILL IN: placeholders" >&2
|
echo " 1. Fill in $CC_FILE — replace every FILL IN: placeholder. Optional fields are" >&2
|
||||||
echo " 2. Fill in $CP_FILE — replace all FILL IN: placeholders" >&2
|
echo " scaffolded there as commented blocks; uncomment the ones that apply." >&2
|
||||||
echo " 3. Validate: check required fields (name, description, system prompt) in both files" >&2
|
echo " 2. Fill in $CP_FILE — same, and heed its closing comment: the Claude Code-only" >&2
|
||||||
|
echo " fields it names must not cross over from the file above." >&2
|
||||||
|
echo " 3. Validate: run $VALIDATE_HINT on each file" >&2
|
||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
|
|||||||
@@ -104,6 +104,40 @@ teardown() {
|
|||||||
done
|
done
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Same drift class as the assertion above, one step further out: the next-steps text
|
||||||
|
# used to enumerate "(name, description, model, body only)" and went on printing that
|
||||||
|
# after ADR-0016's amendment added disallowedTools. Rather than assert on wording, this
|
||||||
|
# asserts the roster is all-or-nothing: if the guidance names any allowlisted field it
|
||||||
|
# must name every one of them, so a partial restatement -- the only shape that can go
|
||||||
|
# stale silently -- fails. Naming none, the current design, passes.
|
||||||
|
@test "plugin/APM scope: next-steps guidance does not partially restate the allowlist" {
|
||||||
|
inventory="$BATS_TEST_DIRNAME/../../agent-audit/references/field-inventory.md"
|
||||||
|
[ -f "$inventory" ] || fail "field-inventory.md not found at $inventory"
|
||||||
|
allowlist="$(awk '
|
||||||
|
/^## apm-agent-allowlist$/ { insection = 1; next }
|
||||||
|
insection && /^##/ { exit }
|
||||||
|
insection && NF && $0 !~ /^#/ && $0 !~ /^---/ { print; exit }
|
||||||
|
' "$inventory")"
|
||||||
|
[ -n "$allowlist" ] || fail "apm-agent-allowlist section is empty in $inventory"
|
||||||
|
|
||||||
|
printf 'name: my-package\ntype: skill\n' > "$ROOT/apm.yml"
|
||||||
|
steps="$(bash "$SCRIPT" my-agent "$ROOT" 2>&1 | sed -n '/^Next steps:/,$p')"
|
||||||
|
[ -n "$steps" ] || fail "scaffolder printed no next-steps block"
|
||||||
|
|
||||||
|
named=""
|
||||||
|
missing=""
|
||||||
|
for field in $allowlist; do
|
||||||
|
if grep -qw -- "$field" <<< "$steps"; then
|
||||||
|
named="$named $field"
|
||||||
|
else
|
||||||
|
missing="$missing $field"
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
if [ -n "$named" ] && [ -n "$missing" ]; then
|
||||||
|
fail "next-steps names allowlisted field(s)$named but omits$missing -- a partial roster. Point at field-inventory.md instead of restating it."
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
@test "plugin/APM scope: sources.md contributing-files template mentions the single-file path" {
|
@test "plugin/APM scope: sources.md contributing-files template mentions the single-file path" {
|
||||||
printf 'name: my-package\ntype: skill\n' > "$ROOT/apm.yml"
|
printf 'name: my-package\ntype: skill\n' > "$ROOT/apm.yml"
|
||||||
bash "$SCRIPT" my-agent "$ROOT"
|
bash "$SCRIPT" my-agent "$ROOT"
|
||||||
|
|||||||
@@ -252,8 +252,16 @@ EOF_RE
|
|||||||
# carries a description with a token Kyberforge.VagueWording flags, so a config
|
# carries a description with a token Kyberforge.VagueWording flags, so a config
|
||||||
# whose glob matches but whose BasedOnStyles lost Kyberforge fails too: it would
|
# whose glob matches but whose BasedOnStyles lost Kyberforge fails too: it would
|
||||||
# lint the file and report nothing.
|
# lint the file and report nothing.
|
||||||
|
# On a miss, the caller reports a glob defect -- but a miss is also what a failed
|
||||||
|
# exec, an OOM-killed vale, or a full TMPDIR looks like, and discarding vale's rc
|
||||||
|
# and output made those indistinguishable and evidence-free. A flake seen once in
|
||||||
|
# this probe could not be diagnosed afterwards for exactly that reason. The rc and
|
||||||
|
# output are now stashed for the caller to attach to its message; VALE_PROBE_DIAG
|
||||||
|
# is set on every call, so a stale value from an earlier probe can never be
|
||||||
|
# reported against a later one.
|
||||||
|
VALE_PROBE_DIAG=""
|
||||||
vale_flags_path() {
|
vale_flags_path() {
|
||||||
local cfg="$1" rel="$2" tmp out
|
local cfg="$1" rel="$2" tmp out rc=0
|
||||||
tmp="$(mktemp -d)"
|
tmp="$(mktemp -d)"
|
||||||
mkdir -p "$tmp/$(dirname "$rel")"
|
mkdir -p "$tmp/$(dirname "$rel")"
|
||||||
{
|
{
|
||||||
@@ -264,9 +272,16 @@ vale_flags_path() {
|
|||||||
echo ""
|
echo ""
|
||||||
echo "Body."
|
echo "Body."
|
||||||
} > "$tmp/$rel"
|
} > "$tmp/$rel"
|
||||||
out="$(cd "$tmp" && vale --config "$cfg" "$rel" 2>&1)" || true
|
out="$(cd "$tmp" && vale --config "$cfg" "$rel" 2>&1)" || rc=$?
|
||||||
rm -rf "$tmp"
|
rm -rf "$tmp"
|
||||||
printf '%s\n' "$out" | grep -qF "Kyberforge.VagueWording"
|
if printf '%s\n' "$out" | grep -qF "Kyberforge.VagueWording"; then
|
||||||
|
VALE_PROBE_DIAG=""
|
||||||
|
return 0
|
||||||
|
fi
|
||||||
|
# vale exits nonzero merely for *having* alerts, so rc alone proves nothing --
|
||||||
|
# it is evidence only alongside the absent alert.
|
||||||
|
VALE_PROBE_DIAG="vale exited $rc; output: ${out:-<empty>}"
|
||||||
|
return 1
|
||||||
}
|
}
|
||||||
|
|
||||||
# Missing vale is a HARD FAILURE, not a warning. Six of this script's assertions
|
# Missing vale is a HARD FAILURE, not a warning. Six of this script's assertions
|
||||||
@@ -328,7 +343,7 @@ while IFS='|' read -r skill rel scope; do
|
|||||||
fi
|
fi
|
||||||
|
|
||||||
if [[ "$VALE_AVAILABLE" == true ]] && ! vale_flags_path "$ini" "$rel"; then
|
if [[ "$VALE_AVAILABLE" == true ]] && ! vale_flags_path "$ini" "$rel"; then
|
||||||
err "$skill/assets/vale/.vale.ini raises no Kyberforge alert on $rel — its glob sections do not cover a path its own pre-commit hook is scoped to, so the hook passes that shape without linting it"
|
err "$skill/assets/vale/.vale.ini raises no Kyberforge alert on $rel — its glob sections do not cover a path its own pre-commit hook is scoped to, so the hook passes that shape without linting it [$VALE_PROBE_DIAG]"
|
||||||
fi
|
fi
|
||||||
# `demo.md` (bare, no `.agent.md` suffix) is `hooks-only` rather than
|
# `demo.md` (bare, no `.agent.md` suffix) is `hooks-only` rather than
|
||||||
# `shared`: it exists only to exercise agent-audit's `[**/agents/*.md]` glob
|
# `shared`: it exists only to exercise agent-audit's `[**/agents/*.md]` glob
|
||||||
|
|||||||
@@ -32,22 +32,60 @@ batch_jobs_limit() {
|
|||||||
# directly) -- both patterns are preserved as-is by callers, not standardized
|
# directly) -- both patterns are preserved as-is by callers, not standardized
|
||||||
# here, so existing error-handling behavior (including how each pattern
|
# here, so existing error-handling behavior (including how each pattern
|
||||||
# interacts with `set -e` in the caller) is unchanged by this extraction.
|
# interacts with `set -e` in the caller) is unchanged by this extraction.
|
||||||
|
#
|
||||||
|
# batch_run waits ONLY on the PIDs it started, never with a bare `wait`. A bare
|
||||||
|
# `wait` blocks on every background job of the calling shell, so a caller that
|
||||||
|
# backgrounds anything of its own would (a) have batch_run block until that
|
||||||
|
# unrelated job finished and (b) have that job reaped here, with its exit status
|
||||||
|
# consumed by the wrong `wait` -- leaving the caller's later `wait $pid` to fail
|
||||||
|
# with "not a child of this shell". None of the three current callers backgrounds
|
||||||
|
# anything else, so this was latent rather than live, but it was an undocumented
|
||||||
|
# constraint on every future caller. Recording each `$!` and waiting on it by PID
|
||||||
|
# removes the constraint instead of documenting it.
|
||||||
|
|
||||||
|
# batch_wait_pids [<pid> ...]
|
||||||
|
# Reaps exactly the given PIDs and always returns 0.
|
||||||
|
#
|
||||||
|
# The `|| true` is load-bearing, not defensive noise: unlike a bare `wait`
|
||||||
|
# (which is unconditionally 0), `wait <pid>` returns that job's exit status, so
|
||||||
|
# without it a single failing job would make batch_run return nonzero and abort
|
||||||
|
# its `set -e` caller at the call site -- before the caller could read the
|
||||||
|
# .status files and print its own summary. Status semantics stay entirely in
|
||||||
|
# the .status files, exactly as before.
|
||||||
|
#
|
||||||
|
# `${@+"$@"}` rather than a bare `"$@"`, for the same reason every `${arr[@]}`
|
||||||
|
# in this repo carries the `${arr[@]+...}` guard. Bash 4.4 is what relaxed
|
||||||
|
# `set -u` for an all-empty `@`/`*` expansion (CHANGES, 4.4 "New Features in
|
||||||
|
# Bash" 3a); 3.2 predates that relaxation, and no bash on a modern machine can
|
||||||
|
# reproduce the abort, so the guarded spelling is asserted rather than tested.
|
||||||
|
# Zero args is a normal path here, not an edge case: the trailing call receives
|
||||||
|
# an empty list whenever the job count divides evenly into the concurrency cap.
|
||||||
|
batch_wait_pids() {
|
||||||
|
local pid
|
||||||
|
for pid in ${@+"$@"}; do
|
||||||
|
wait "$pid" || true
|
||||||
|
done
|
||||||
|
}
|
||||||
|
|
||||||
batch_run() {
|
batch_run() {
|
||||||
local scratch_dir="$1"
|
local scratch_dir="$1"
|
||||||
shift
|
shift
|
||||||
local jobs_limit running key cmd
|
local jobs_limit running key cmd pids
|
||||||
jobs_limit="$(batch_jobs_limit)"
|
jobs_limit="$(batch_jobs_limit)"
|
||||||
running=0
|
running=0
|
||||||
|
pids=()
|
||||||
|
|
||||||
while [[ $# -gt 0 ]]; do
|
while [[ $# -gt 0 ]]; do
|
||||||
key="$1" cmd="$2"
|
key="$1" cmd="$2"
|
||||||
shift 2
|
shift 2
|
||||||
(eval "$cmd") >"$scratch_dir/$key.log" 2>&1 &
|
(eval "$cmd") >"$scratch_dir/$key.log" 2>&1 &
|
||||||
|
pids+=("$!")
|
||||||
running=$((running + 1))
|
running=$((running + 1))
|
||||||
if [[ $running -ge $jobs_limit ]]; then
|
if [[ $running -ge $jobs_limit ]]; then
|
||||||
wait
|
batch_wait_pids ${pids[@]+"${pids[@]}"}
|
||||||
|
pids=()
|
||||||
running=0
|
running=0
|
||||||
fi
|
fi
|
||||||
done
|
done
|
||||||
wait
|
batch_wait_pids ${pids[@]+"${pids[@]}"}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user