From b8dc400365e6ca2cd305fd6aeda5cb07468d9cc0 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Fri, 14 Aug 2026 14:21:04 +0000 Subject: [PATCH] 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 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: ". 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 Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT --- .../skills/agent-author/references/scripts.md | 1 + .../skills/agent-author/scripts/new-agent.sh | 38 ++++++++++++---- .../skills/agent-author/tests/new-agent.bats | 34 ++++++++++++++ scripts/check-vale-style-sync.sh | 23 ++++++++-- scripts/lib/batch-run.sh | 44 +++++++++++++++++-- 5 files changed, 124 insertions(+), 16 deletions(-) diff --git a/plugins/kyberforge/.apm/skills/agent-author/references/scripts.md b/plugins/kyberforge/.apm/skills/agent-author/references/scripts.md index 6a4966c..42c4493 100644 --- a/plugins/kyberforge/.apm/skills/agent-author/references/scripts.md +++ b/plugins/kyberforge/.apm/skills/agent-author/references/scripts.md @@ -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. - **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`). +- **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 diff --git a/plugins/kyberforge/.apm/skills/agent-author/scripts/new-agent.sh b/plugins/kyberforge/.apm/skills/agent-author/scripts/new-agent.sh index c771dd1..4742610 100755 --- a/plugins/kyberforge/.apm/skills/agent-author/scripts/new-agent.sh +++ b/plugins/kyberforge/.apm/skills/agent-author/scripts/new-agent.sh @@ -21,10 +21,12 @@ Arguments: without type: is a marketplace-only manifest and is skipped, the walk continues upward → creates /.apm/agents/.agent.md - (single vendor-neutral file — no tools, - isolation, maxTurns, effort, memory, or - permissionMode; apm compile has no per-target - field integrator, see ADR-0016) + (single vendor-neutral file; apm compile copies + its frontmatter verbatim to every target with no + per-target field integrator, so the permitted + field set is narrow — see the apm-agent-allowlist + section of agent-audit's + references/field-inventory.md and ADR-0016) → creates /sources.md (if absent) project scope : no type:-bearing apm.yml found; root is a project directory @@ -258,6 +260,19 @@ SOURCES 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 echo "All files already exist — nothing to do." >&2 else @@ -266,12 +281,17 @@ else echo "" >&2 echo "Next steps:" >&2 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 " 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 - echo " 1. Fill in $CC_FILE — replace all FILL IN: placeholders" >&2 - echo " 2. Fill in $CP_FILE — replace all FILL IN: placeholders" >&2 - echo " 3. Validate: check required fields (name, description, system prompt) in both files" >&2 + echo " 1. Fill in $CC_FILE — replace every FILL IN: placeholder. Optional fields are" >&2 + echo " scaffolded there as commented blocks; uncomment the ones that apply." >&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 diff --git a/plugins/kyberforge/.apm/skills/agent-author/tests/new-agent.bats b/plugins/kyberforge/.apm/skills/agent-author/tests/new-agent.bats index c254e3b..5904546 100644 --- a/plugins/kyberforge/.apm/skills/agent-author/tests/new-agent.bats +++ b/plugins/kyberforge/.apm/skills/agent-author/tests/new-agent.bats @@ -104,6 +104,40 @@ teardown() { 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" { printf 'name: my-package\ntype: skill\n' > "$ROOT/apm.yml" bash "$SCRIPT" my-agent "$ROOT" diff --git a/scripts/check-vale-style-sync.sh b/scripts/check-vale-style-sync.sh index f3b7804..a0c81e9 100755 --- a/scripts/check-vale-style-sync.sh +++ b/scripts/check-vale-style-sync.sh @@ -252,8 +252,16 @@ EOF_RE # carries a description with a token Kyberforge.VagueWording flags, so a config # whose glob matches but whose BasedOnStyles lost Kyberforge fails too: it would # 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() { - local cfg="$1" rel="$2" tmp out + local cfg="$1" rel="$2" tmp out rc=0 tmp="$(mktemp -d)" mkdir -p "$tmp/$(dirname "$rel")" { @@ -264,9 +272,16 @@ vale_flags_path() { echo "" echo "Body." } > "$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" - 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:-}" + return 1 } # 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 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 # `demo.md` (bare, no `.agent.md` suffix) is `hooks-only` rather than # `shared`: it exists only to exercise agent-audit's `[**/agents/*.md]` glob diff --git a/scripts/lib/batch-run.sh b/scripts/lib/batch-run.sh index a1612f0..b300c0a 100644 --- a/scripts/lib/batch-run.sh +++ b/scripts/lib/batch-run.sh @@ -32,22 +32,60 @@ batch_jobs_limit() { # directly) -- both patterns are preserved as-is by callers, not standardized # here, so existing error-handling behavior (including how each pattern # 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 [ ...] +# 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 ` 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() { local scratch_dir="$1" shift - local jobs_limit running key cmd + local jobs_limit running key cmd pids jobs_limit="$(batch_jobs_limit)" running=0 + pids=() while [[ $# -gt 0 ]]; do key="$1" cmd="$2" shift 2 (eval "$cmd") >"$scratch_dir/$key.log" 2>&1 & + pids+=("$!") running=$((running + 1)) if [[ $running -ge $jobs_limit ]]; then - wait + batch_wait_pids ${pids[@]+"${pids[@]}"} + pids=() running=0 fi done - wait + batch_wait_pids ${pids[@]+"${pids[@]}"} }