Files
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

304 lines
11 KiB
Bash
Executable File

#!/usr/bin/env bash
set -euo pipefail
SKILL_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
SKILL_ROOT="$(cd "$SKILL_DIR/.." && pwd)"
TEMPLATES_DIR="$SKILL_ROOT/assets/templates"
usage() {
cat <<EOF
Usage: new-agent.sh <agent-name> <root>
Scaffold agent definition file(s) for Claude Code, GitHub Copilot CLI, and/or
vendor-neutral APM packages.
Arguments:
agent-name Kebab-case agent identifier (e.g. code-reviewer, deploy-assistant).
root Starting directory — scope is resolved by walking up from here:
plugin/APM scope : nearest ancestor (at/above root) whose apm.yml
has a top-level type: field (instructions,
skill, hybrid, or prompts) — an apm.yml
without type: is a marketplace-only manifest
and is skipped, the walk continues upward
→ creates <package-root>/.apm/agents/<name>.agent.md
(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 factory-audit's
references/agent-field-inventory.md and ADR-0016)
→ creates <package-root>/sources.md (if absent)
project scope : no type:-bearing apm.yml found; root is a
project directory
→ creates <root>/.claude/agents/<name>.md
→ creates <root>/.github/agents/<name>.agent.md
user scope : root is exactly ~ (home directory; checked
directly, no walk-up)
→ creates ~/.claude/agents/<name>.md
→ creates ~/.copilot/agents/<name>.agent.md
Each file is created only if it does not already exist (no-op per file).
Exit codes:
0 Files created or already existed (no-op)
1 Invalid arguments, missing root, or templates not found
EOF
}
if [[ "${1:-}" == "--help" || "${1:-}" == "-h" ]]; then
usage
exit 0
fi
if [[ $# -lt 2 ]]; then
echo "Error: agent-name and root are required." >&2
echo "" >&2
usage >&2
exit 1
fi
AGENT_NAME="$1"
ROOT="$2"
# Validate agent name format
if ! grep -qE '^[a-z0-9]+(-[a-z0-9]+)*$' <<< "$AGENT_NAME"; then
echo "Error: agent-name must use lowercase letters, numbers, and hyphens only." >&2
echo " No leading, trailing, or consecutive hyphens." >&2
echo " Received: '$AGENT_NAME'" >&2
exit 1
fi
# Validate templates directory
if [[ ! -d "$TEMPLATES_DIR" ]]; then
echo "Error: templates directory not found at '$TEMPLATES_DIR'." >&2
echo " Run this script from its original location inside the agent-author skill." >&2
exit 1
fi
# Expand tilde
ROOT="${ROOT/#\~/$HOME}"
# Validate root exists
if [[ ! -d "$ROOT" ]]; then
echo "Error: root directory '$ROOT' does not exist." >&2
exit 1
fi
ROOT="$(cd "$ROOT" && pwd)"
# True if apm_yml's top-level `type:` line names one of the four APM package
# types (instructions/skill/hybrid/prompts) — mirrors validate.sh's
# APM_TYPE_RE: an optional quote around the value must be closed by the
# *same* quote character (a mismatched or unterminated quote is rejected,
# not silently stripped), and the value must be followed by whitespace or
# end-of-line so `prompts-only` doesn't false-match on the `prompts` prefix.
# `|| [[ -n "$line" ]]` in the read condition also processes a final line
# that lacks a trailing newline, which `read` alone would otherwise skip.
is_apm_package_manifest() {
local apm_yml="$1" line
while IFS= read -r line || [[ -n "$line" ]]; do
if [[ "$line" =~ ^type:[[:space:]]*(instructions|skill|hybrid|prompts)([[:space:]]|$) ]]; then
return 0
fi
if [[ "$line" =~ ^type:[[:space:]]*([\"\'])(instructions|skill|hybrid|prompts)([\"\'])([[:space:]]|$) ]] \
&& [[ "${BASH_REMATCH[1]}" == "${BASH_REMATCH[3]}" ]]; then
return 0
fi
done < "$apm_yml"
return 1
}
# --- Walk-up package-root detection ---
#
# Mirrors factory-audit's validate.sh scope walk-up, with apm.yml + type: swapped
# in for the old plugin.json marker. Starting at ROOT, walk upward:
# - an apm.yml with a top-level `type:` field marks an APM package root
# (plugin/APM scope) — stop and return it.
# - an apm.yml with no `type:` field is a marketplace-only manifest — skip
# it, keep walking up.
# - user scope is checked directly at $HOME, no walk-up (see usage text
# above): ROOT itself being $HOME resolves to user scope, even if $HOME
# is itself a .git-tracked dotfiles directory (checked before the .git
# test below, so a dotfiles repo at $HOME can't shadow user scope).
# Walking *up into* $HOME from a nested directory with no apm.yml/.git
# of its own does NOT promote to user scope — it resolves to project
# scope instead, same as any other unmatched boundary, so a stray
# directory under $HOME can't be silently redirected into the shared
# global ~/.claude or ~/.copilot agent directories.
# - a .git file or directory marks the project-scope boundary (a worktree's
# .git is a file, not a directory) — stop.
# - filesystem root reached with neither found — project scope, same as
# any other unmatched boundary.
find_package_root() {
local root="$1" current="$1"
while true; do
if [[ -f "$current/apm.yml" ]] && is_apm_package_manifest "$current/apm.yml"; then
echo "plugin $current"
return
fi
if [[ "$current" == "$HOME" ]]; then
if [[ "$current" == "$root" ]]; then
echo "user $current"
return
fi
echo "project $current"
return
fi
if [[ -e "$current/.git" ]]; then
echo "project $current"
return
fi
local parent
parent="$(dirname "$current")"
if [[ "$parent" == "$current" ]]; then
echo "project $current"
return
fi
current="$parent"
done
}
# kind and path are emitted on one space-separated line rather than two
# `echo`s — kind first (never contains spaces), path last (absorbs any spaces
# in the path safely). `mapfile`/`readarray` would need bash 4.0+, which
# macOS's stock /bin/bash 3.2 is not; a here-string `read` splits the single
# line without it. Same form as skill-author's new-skill.sh, deliberately.
WALK_RESULT="$(find_package_root "$ROOT")"
read -r WALK_KIND WALK_ROOT <<< "$WALK_RESULT"
PACKAGE_ROOT=""
case "$WALK_KIND" in
plugin)
SCOPE="plugin"
PACKAGE_ROOT="$WALK_ROOT"
;;
user)
SCOPE="user"
;;
project)
SCOPE="project"
;;
esac
# Determine file destinations
case "$SCOPE" in
plugin)
APM_DIR="$PACKAGE_ROOT/.apm/agents"
SOURCES_DIR="$PACKAGE_ROOT"
;;
project)
CC_DIR="$ROOT/.claude/agents"
CP_DIR="$ROOT/.github/agents"
SOURCES_DIR=""
;;
user)
CC_DIR="$HOME/.claude/agents"
CP_DIR="$HOME/.copilot/agents"
SOURCES_DIR=""
;;
esac
created_any=false
if [[ "$SCOPE" == "plugin" ]]; then
APM_FILE="$APM_DIR/$AGENT_NAME.agent.md"
mkdir -p "$APM_DIR"
if [[ -f "$APM_FILE" ]]; then
echo "Skipping '$APM_FILE' — already exists." >&2
else
sed "s/AGENT_NAME/$AGENT_NAME/g" "$TEMPLATES_DIR/apm-agent.md" > "$APM_FILE"
echo "Created: $APM_FILE" >&2
created_any=true
fi
else
CC_FILE="$CC_DIR/$AGENT_NAME.md"
CP_FILE="$CP_DIR/$AGENT_NAME.agent.md"
mkdir -p "$CC_DIR"
mkdir -p "$CP_DIR"
# Copy Claude Code template (no-op if exists)
if [[ -f "$CC_FILE" ]]; then
echo "Skipping '$CC_FILE' — already exists." >&2
else
sed "s/AGENT_NAME/$AGENT_NAME/g" "$TEMPLATES_DIR/claude-code.md" > "$CC_FILE"
echo "Created: $CC_FILE" >&2
created_any=true
fi
# Copy Copilot template (no-op if exists)
if [[ -f "$CP_FILE" ]]; then
echo "Skipping '$CP_FILE' — already exists." >&2
else
sed "s/AGENT_NAME/$AGENT_NAME/g" "$TEMPLATES_DIR/copilot.agent.md.template" > "$CP_FILE"
echo "Created: $CP_FILE" >&2
created_any=true
fi
fi
# Create sources.md at plugin/APM package root (no-op if exists)
if [[ -n "$SOURCES_DIR" ]]; then
SOURCES_FILE="$SOURCES_DIR/sources.md"
if [[ -f "$SOURCES_FILE" ]]; then
echo "Skipping '$SOURCES_FILE' — already exists." >&2
else
cat > "$SOURCES_FILE" <<'SOURCES'
# Sources
<!-- List research sources that informed agents in this package.
Follow the format below. Only include entries with `extracted` status.
Delete this file if no research sources informed these agents. -->
<!-- ## source-slug
- **URL:** <url>
- **Research doc:** <relative-path-to-upstream-research-sources-file>
- **Description:** <what this source covers>
- **Contributing files:** .apm/agents/<name>.agent.md
- **Status:** `extracted` -->
SOURCES
echo "Created: $SOURCES_FILE" >&2
created_any=true
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 factory-audit's validate.sh —
# which reads the allowlist from agent-field-inventory.md as data — for what is permitted.
AUDIT_SCRIPTS="$(cd "$SKILL_ROOT/../factory-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="factory-audit's scripts/validate.sh"
fi
if [[ "$created_any" == false ]]; then
echo "All files already exist — nothing to do." >&2
else
echo "" >&2
echo "Scope: $SCOPE" >&2
echo "" >&2
echo "Next steps:" >&2
if [[ "$SCOPE" == "plugin" ]]; then
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 " Description: 250 chars target / 400 ceiling (ADR-0020). The body has no" >&2
echo " word gate — delegate to a skill instead of restating what it does." >&2
echo " 2. Populate $SOURCES_DIR/sources.md with research sources, or delete it" >&2
echo " 3. Validate: $VALIDATE_HINT $APM_FILE" >&2
echo " It checks the frontmatter against the apm-agent-allowlist section of" >&2
echo " factory-audit's references/agent-field-inventory.md, the authoritative field list." >&2
else
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 " Description: 250 chars target / 400 ceiling (ADR-0020). The body has no" >&2
echo " word gate — delegate to a skill instead of restating what it does." >&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