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 to4d018afandaa8cc22(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
190 lines
7.0 KiB
Bash
Executable File
190 lines
7.0 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
|
|
SKILL_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
|
TEMPLATES_DIR="$SKILL_DIR/../assets/templates"
|
|
|
|
usage() {
|
|
cat <<EOF
|
|
Usage: new-skill.sh <skill-name> <path>
|
|
|
|
Create a new skill scaffold by copying annotated templates to the resolved
|
|
destination. <path> is any existing path inside or at the target — a
|
|
package or a standalone location. It does not have to be a package root
|
|
itself.
|
|
|
|
The script walks up from <path> to pick one of two modes:
|
|
|
|
Package mode:
|
|
If an apm.yml with a top-level 'type:' field (instructions, skill,
|
|
hybrid, or prompts) is found at or above <path>, the skill is
|
|
scaffolded into <package-root>/.apm/skills/<skill-name>/ — not under
|
|
<path> itself. An apm.yml with no 'type:' field is a marketplace-only
|
|
manifest, not a package; it is skipped and the walk continues upward.
|
|
|
|
Standalone mode:
|
|
If the walk reaches a '.git' directory or the filesystem root without
|
|
finding a type-bearing apm.yml, the skill is scaffolded directly into
|
|
<path>/<skill-name>/, exactly as <path> was given.
|
|
|
|
Arguments:
|
|
skill-name Kebab-case skill identifier (e.g. my-tool, data-analyzer).
|
|
Must match the directory name exactly.
|
|
path Any existing path inside/at the target. Used to locate the
|
|
package (package mode) or as the literal parent directory
|
|
(standalone mode). Must already exist.
|
|
Examples: ~/.agents/skills/ packages/my-pkg/some/subdir/
|
|
|
|
Output:
|
|
Package mode: <package-root>/.apm/skills/<skill-name>/
|
|
Standalone mode: <path>/<skill-name>/
|
|
|
|
Exit codes:
|
|
0 Scaffold created successfully, or destination already exists (no-op)
|
|
1 Invalid arguments, missing path, or templates not found
|
|
EOF
|
|
}
|
|
|
|
if [[ "${1:-}" == "--help" || "${1:-}" == "-h" ]]; then
|
|
usage
|
|
exit 0
|
|
fi
|
|
|
|
if [[ $# -lt 2 ]]; then
|
|
echo "Error: skill-name and path are required." >&2
|
|
echo "" >&2
|
|
usage >&2
|
|
exit 1
|
|
fi
|
|
|
|
SKILL_NAME="$1"
|
|
TARGET_INPUT="$2"
|
|
|
|
# Validate skill name format
|
|
if ! grep -qE '^[a-z0-9]+(-[a-z0-9]+)*$' <<< "$SKILL_NAME"; then
|
|
echo "Error: skill-name must use lowercase letters, numbers, and hyphens only." >&2
|
|
echo " No leading, trailing, or consecutive hyphens." >&2
|
|
echo " Received: '$SKILL_NAME'" >&2
|
|
exit 1
|
|
fi
|
|
|
|
# Validate templates directory exists
|
|
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 skill-author skill." >&2
|
|
exit 1
|
|
fi
|
|
|
|
# Validate path exists
|
|
if [[ ! -d "$TARGET_INPUT" ]]; then
|
|
echo "Error: path '$TARGET_INPUT' does not exist." >&2
|
|
exit 1
|
|
fi
|
|
|
|
# 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.
|
|
# Identical to agent-author's new-agent.sh copy of this helper.
|
|
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 from <path> looking for a type-bearing apm.yml (package mode) or a
|
|
# .git boundary / filesystem root (standalone mode). An apm.yml with no
|
|
# top-level 'type:' field is a marketplace-only manifest — skip it and keep
|
|
# walking up. Prints one space-separated line: mode, then the resolved root.
|
|
# ---------------------------------------------------------------------------
|
|
find_package_root() {
|
|
local current
|
|
current="$(cd "$1" && pwd)"
|
|
while true; do
|
|
if [[ -f "$current/apm.yml" ]]; then
|
|
if is_apm_package_manifest "$current/apm.yml"; then
|
|
echo "package $current"
|
|
return 0
|
|
fi
|
|
# apm.yml exists but has no type: field — marketplace-only manifest.
|
|
# Not a package match; keep walking up.
|
|
fi
|
|
# .git is a directory in a normal checkout but a file (`gitdir: ...`) in
|
|
# a git worktree — -e covers both.
|
|
if [[ -e "$current/.git" ]]; then
|
|
echo "no-package $current"
|
|
return 0
|
|
fi
|
|
local parent
|
|
parent="$(dirname "$current")"
|
|
if [[ "$parent" == "$current" ]]; then
|
|
echo "no-package $current"
|
|
return 0
|
|
fi
|
|
current="$parent"
|
|
done
|
|
}
|
|
|
|
# `mapfile`/`readarray` are bash 4.0+ builtins with no fallback on macOS's
|
|
# stock /bin/bash 3.2 — read the single space-separated output line with a
|
|
# plain `read` instead (bash 3.2-safe). `read` consumes only one line, so
|
|
# mode and path must be on the same line: MODE first (never contains
|
|
# whitespace), PKG_ROOT last (safely absorbs a path containing spaces).
|
|
WALK_OUTPUT="$(find_package_root "$TARGET_INPUT")"
|
|
read -r MODE PKG_ROOT <<< "$WALK_OUTPUT"
|
|
|
|
if [[ "$MODE" == "package" ]]; then
|
|
TARGET="$PKG_ROOT/.apm/skills/$SKILL_NAME"
|
|
else
|
|
TARGET="$TARGET_INPUT/$SKILL_NAME"
|
|
fi
|
|
|
|
# Destination already exists — treat as a no-op so retries are safe
|
|
if [[ -d "$TARGET" ]]; then
|
|
echo "Scaffold already exists at '$TARGET' — nothing to do." >&2
|
|
exit 0
|
|
fi
|
|
|
|
mkdir -p "$(dirname "$TARGET")"
|
|
|
|
# Copy templates to destination
|
|
cp -r "$TEMPLATES_DIR" "$TARGET"
|
|
|
|
# Set skill name in templates
|
|
sed -i "s/SKILL_NAME/$SKILL_NAME/g" "$TARGET/SKILL.md"
|
|
sed -i "s/SKILL_NAME/$SKILL_NAME/g" "$TARGET/tests/README.md"
|
|
|
|
if [[ "$MODE" == "package" ]]; then
|
|
echo "Mode: package — type-bearing apm.yml found at '$PKG_ROOT'" >&2
|
|
echo "Scaffold created: $TARGET" >&2
|
|
echo "" >&2
|
|
echo "Note: if '$PKG_ROOT/apm.yml' has an explicit 'includes:' list (not 'auto')," >&2
|
|
echo " add '.apm/skills/$SKILL_NAME/' to it." >&2
|
|
else
|
|
echo "Mode: standalone — no type-bearing apm.yml found above '$TARGET_INPUT'" >&2
|
|
echo "Scaffold created: $TARGET" >&2
|
|
fi
|
|
echo "" >&2
|
|
echo "Next steps:" >&2
|
|
echo " 1. Fill in $TARGET/SKILL.md — replace all FILL IN: placeholders." >&2
|
|
echo " Description: 250 chars target / 400 ceiling. Body: 600 / 900, body only." >&2
|
|
echo " 2. Add scripts to scripts/ if needed (or delete the directory)" >&2
|
|
echo " 3. Add docs to references/ if needed (or delete the directory)" >&2
|
|
echo " 4. Add resources to assets/ if needed (or delete the directory)" >&2
|
|
echo " 5. Add tests to tests/ if the skill has scripts (or delete the directory)" >&2
|
|
echo " 6. Populate references/sources.md with research sources, or delete it" >&2
|
|
echo " 7. Validate: run /factory-audit on $TARGET" >&2
|