fix(skill-author): fail loudly when scaffold repair cannot substitute
Why: repair_placeholders ran inside a command substitution, so a failing sed left an emptied file behind and the script still exited 0 reporting success. - write via tmp file and abort on sed or mv failure - re-check the target before moving staging into place - stage in a dot-prefixed mktemp dir so a killed run leaves no fake skill - source apm claims in deployment-modes.md; tag untyped code blocks Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkT7RSDwDbmrM9T34b6sTi
This commit is contained in:
@@ -42,7 +42,8 @@ Output:
|
||||
Exit codes:
|
||||
0 Scaffold created, destination already complete (no-op), or a partial
|
||||
scaffold from an earlier failed run repaired
|
||||
1 Invalid arguments, missing path, or templates not found
|
||||
1 Invalid arguments, missing path, templates not found, name
|
||||
substitution failed, or the destination appeared mid-build
|
||||
EOF
|
||||
}
|
||||
|
||||
@@ -162,41 +163,60 @@ SUBST_MARKERS=("name: SKILL_NAME" "bats <destination-dir>/SKILL_NAME/tests/")
|
||||
# Replace SKILL_NAME in one file. `sed -i` is not portable — GNU takes an
|
||||
# optional attached suffix, BSD/macOS requires a separate suffix argument and
|
||||
# reads the expression as one — so write to a temp file and move it over.
|
||||
# The move runs only if sed succeeded: a failed sed leaves an empty or partial
|
||||
# temp file, and moving that over the original would destroy it.
|
||||
substitute_file() {
|
||||
local f="$1"
|
||||
sed "s/SKILL_NAME/$SKILL_NAME/g" "$f" > "$f.tmp"
|
||||
mv "$f.tmp" "$f"
|
||||
if sed "s/SKILL_NAME/$SKILL_NAME/g" "$f" > "$f.tmp" && mv "$f.tmp" "$f"; then
|
||||
return 0
|
||||
fi
|
||||
rm -f "$f.tmp"
|
||||
echo "Error: could not substitute the skill name in '$f'." >&2
|
||||
return 1
|
||||
}
|
||||
|
||||
# Substitute every placeholder file under dir $1 (a fresh template copy).
|
||||
substitute_name() {
|
||||
local dir="$1" rel
|
||||
for rel in "${SUBST_FILES[@]}"; do
|
||||
[[ -f "$dir/$rel" ]] && substitute_file "$dir/$rel"
|
||||
if [[ -f "$dir/$rel" ]]; then
|
||||
substitute_file "$dir/$rel" || return 1
|
||||
fi
|
||||
done
|
||||
return 0
|
||||
}
|
||||
|
||||
# Substitute only the placeholder files under dir $1 that still carry their
|
||||
# template marker line; print how many were repaired.
|
||||
# template marker line. Sets REPAIRED to how many were repaired; returns 1 on
|
||||
# the first failure. Called directly, never inside $(...): a command
|
||||
# substitution would swallow the failure and let the caller report success.
|
||||
REPAIRED=0
|
||||
repair_placeholders() {
|
||||
local dir="$1" i f n=0
|
||||
local dir="$1" i f
|
||||
REPAIRED=0
|
||||
for i in "${!SUBST_FILES[@]}"; do
|
||||
f="$dir/${SUBST_FILES[$i]}"
|
||||
if [[ -f "$f" ]] && grep -qxF "${SUBST_MARKERS[$i]}" "$f"; then
|
||||
substitute_file "$f"
|
||||
n=$((n + 1))
|
||||
substitute_file "$f" || return 1
|
||||
REPAIRED=$((REPAIRED + 1))
|
||||
fi
|
||||
done
|
||||
echo "$n"
|
||||
return 0
|
||||
}
|
||||
|
||||
if [[ -d "$TARGET" ]]; then
|
||||
# A scaffold left half-built by an earlier failed run still carries a
|
||||
# template marker line; finish it instead of reporting a silent no-op.
|
||||
# Anything else — including a complete skill — is left untouched.
|
||||
if [[ "$(repair_placeholders "$TARGET")" -gt 0 ]]; then
|
||||
echo "Repaired partial scaffold at '$TARGET' — substituted SKILL_NAME." >&2
|
||||
if ! repair_placeholders "$TARGET"; then
|
||||
echo "Error: repair of '$TARGET' failed; no file was left half-written." >&2
|
||||
exit 1
|
||||
fi
|
||||
if [[ "$REPAIRED" -gt 0 ]]; then
|
||||
# The marker line proves only that the name was never substituted, not
|
||||
# that the earlier copy finished — a file may still be missing.
|
||||
echo "Repaired partial scaffold at '$TARGET' — only the name placeholder (SKILL_NAME) was substituted." >&2
|
||||
echo "The earlier run may also have left files missing: run /factory-audit on it, or delete it and re-run this script." >&2
|
||||
exit 0
|
||||
fi
|
||||
echo "Scaffold already exists at '$TARGET' — nothing to do." >&2
|
||||
@@ -207,10 +227,23 @@ mkdir -p "$(dirname "$TARGET")"
|
||||
|
||||
# Build in a sibling staging directory and rename it into place only once
|
||||
# complete, so a failure mid-build never leaves a half-built $TARGET behind.
|
||||
STAGING="$TARGET.partial.$$"
|
||||
# The dot prefix matters: a SIGKILL skips the trap, and a leftover must not
|
||||
# look like a skill to anything scanning .apm/skills/.
|
||||
STAGING="$(mktemp -d "$(dirname "$TARGET")/.new-skill.XXXXXX")"
|
||||
trap 'rm -rf "$STAGING"' EXIT
|
||||
cp -r "$TEMPLATES_DIR" "$STAGING"
|
||||
# mktemp creates the directory 0700; give the skill the umask default instead.
|
||||
chmod "$(umask -S)" "$STAGING"
|
||||
cp -R "$TEMPLATES_DIR/." "$STAGING"
|
||||
substitute_name "$STAGING"
|
||||
|
||||
# $TARGET may have appeared since the check above (a concurrent run). `mv`
|
||||
# onto an existing directory nests the source inside it instead of failing,
|
||||
# and GNU `mv -T` is not portable, so re-check immediately before the rename.
|
||||
# This narrows the window to the gap between two syscalls; it does not close it.
|
||||
if [[ -e "$TARGET" ]]; then
|
||||
echo "Error: '$TARGET' appeared while the scaffold was being built; left it untouched." >&2
|
||||
exit 1
|
||||
fi
|
||||
mv "$STAGING" "$TARGET"
|
||||
trap - EXIT
|
||||
|
||||
|
||||
Reference in New Issue
Block a user