mirror of
https://github.com/addyosmani/agent-skills.git
synced 2026-10-26 23:15:04 +00:00
fix(hooks): expand simplify-ignore files at Stop instead of restoring backups
The Stop branch restores every cached file from its backup, but that
backup is written on PreToolUse Read and refreshed only by the
PostToolUse Edit|Write branch. A change made to a filtered file through
any other route between Read and Stop — a Bash command, a formatter, an
external editor — is overwritten at session end. Reproduced in #295:
Bash wrote const editable = 2 and read it back, Stop left const
editable = 1 on disk, with no crash and no failed tool call.
The hook protects the content of an annotated block, not the file: the
header documents the backup as holding the model's changes, and Edit and
Write already preserve them. So Stop now runs the same placeholder
expansion the Edit|Write branch runs, against the file as it is on disk.
Whatever wrote the file wrote its filtered form with the placeholders
intact, so expanding it restores the protected blocks and keeps the
change, whichever route it came from.
The expansion body moves into an expand_file function that both branches
call, so the fuzzy-match and deleted-block warnings apply at Stop too.
When no cached BLOCK_<hash> is left, the file was rewritten wholesale and
the hook cannot place the block; it falls back to the backup as before,
now with a warning instead of in silence. That residual is documented in
SIMPLIFY-IGNORE.md. With no outside write, expansion reproduces the
backup exactly, so Read→Stop and Read→Edit→Stop are byte-identical.
Test 11 drives the real hook through Read, an Edit event, a write with no
event after it, and Stop; it fails at be4e44a on the two assertions for
the lost write. Suite: 28 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
mavericksea-aiandClaude Opus 5 committed
1 parent
be4e44a9fb
commit
b1ac22c7d2
3 files changed
+147
-65
No files matched your search
@@ -54,7 +54,7 @@ One script, three hook events:
|
||||
|---|---|
|
||||
| `PreToolUse Read` | Backs up file, replaces blocks with `BLOCK_<hash>` placeholders in-place |
|
||||
| `PostToolUse Edit\|Write` | Expands placeholders back to real code, saves model's changes, re-filters |
|
||||
| `Stop` | Restores all files from backup when session ends |
|
||||
| `Stop` | Expands the placeholders left in each file on disk, so changes made outside `Edit`/`Write` survive; falls back to the backup only if no placeholder is left |
|
||||
|
||||
Each block is content-hashed (8 hex chars via `shasum`/`sha1sum`) so the round-trip is unambiguous even if the model duplicates or reorders placeholders. Cache is project-scoped to prevent cross-session interference.
|
||||
|
||||
@@ -83,6 +83,7 @@ Backups are stored in `.claude/.simplify-ignore-cache/` within your project dire
|
||||
- **Single-line blocks hide the entire line.** If `simplify-ignore-start` and `simplify-ignore-end` appear on the same line as other code, the whole line is hidden from the model, not just the annotated portion. Use dedicated lines for annotations.
|
||||
- **Comment suffix detection covers `*/` and `-->` only.** Template engines with non-standard comment closers (ERB `%>`, Blade `--}}`) may produce unbalanced placeholders. Use `#` or `//` style comments instead.
|
||||
- **Fallback expansion is progressive, not exact.** If the model alters a placeholder's formatting (e.g. changes the reason text), the hook tries progressively simpler matches: full placeholder → prefix+hash+suffix → hash-only. The hash-only fallback may leave cosmetic debris (e.g. stray `:` or reason text). A warning is printed to stderr when this happens.
|
||||
- **A wholesale rewrite falls back to the backup.** If something replaces a filtered file's entire contents and removes every `BLOCK_<hash>` placeholder, the hook can no longer tell where the protected block belongs. At `Stop` it restores the backup and prints a warning, so that rewrite is lost. Changes that leave the placeholders in place — a `Bash` edit, a formatter, an external editor — are kept.
|
||||
- **File renaming leaves placeholders.** If the model renames or moves a file via a shell command, the new file will retain `BLOCK_<hash>` placeholders. The original code is saved as `<old-filename>.recovered` when the session stops. You must manually restore the recovered code into the new file.
|
||||
|
||||
## Requirements
|
||||
|
||||
@@ -247,6 +247,50 @@ else
|
||||
assert_eq "missing-jq guard surfaced" "1" "$(printf '%s' "$warning_out" | grep -c 'error: missing jq')"
|
||||
fi
|
||||
|
||||
# ── Test 11: Stop keeps a change made outside Edit|Write ─────────────────
|
||||
printf '\nTest 11: Stop keeps a Bash write made after the last Edit\n'
|
||||
|
||||
if ! command -v jq >/dev/null 2>&1; then
|
||||
printf ' SKIP: full lifecycle needs jq (the hook exits at its jq guard)\n'
|
||||
else
|
||||
PROJ="$TMPDIR/proj"
|
||||
rm -rf "$PROJ"
|
||||
mkdir -p "$PROJ/.claude"
|
||||
TARGET="$PROJ/app.js"
|
||||
cat > "$TARGET" <<'EOF'
|
||||
const editable = 1;
|
||||
/* simplify-ignore-start: perf-critical */
|
||||
const protectedValue = 42;
|
||||
/* simplify-ignore-end */
|
||||
EOF
|
||||
|
||||
hook_event() { printf '%s' "$1" | CLAUDE_PROJECT_DIR="$PROJ" bash hooks/simplify-ignore.sh; }
|
||||
read_event=$(printf '{"tool_name":"Read","tool_input":{"file_path":"%s"}}' "$TARGET")
|
||||
edit_event=$(printf '{"tool_name":"Edit","tool_input":{"file_path":"%s"}}' "$TARGET")
|
||||
|
||||
# PreToolUse Read → block hidden behind a placeholder, backup taken
|
||||
hook_event "$read_event"
|
||||
assert_eq "protected block hidden after Read" "1" "$(grep -c 'BLOCK_' "$TARGET")"
|
||||
assert_eq "protected content off disk after Read" "0" "$(grep -c 'protectedValue' "$TARGET")"
|
||||
|
||||
# Model edits the filtered file → PostToolUse Edit refreshes the backup
|
||||
printf 'const added = 3;\n' >> "$TARGET"
|
||||
hook_event "$edit_event"
|
||||
|
||||
# A later write through Bash — no Edit or Write event follows it
|
||||
sed 's/const editable = 1;/const editable = 2;/' "$TARGET" > "$TMPDIR/bash-write.js"
|
||||
cat "$TMPDIR/bash-write.js" > "$TARGET"
|
||||
|
||||
# Stop
|
||||
hook_event '{}'
|
||||
|
||||
assert_eq "Bash write survives Stop" "1" "$(grep -c 'const editable = 2;' "$TARGET")"
|
||||
assert_eq "stale backup value not restored" "0" "$(grep -c 'const editable = 1;' "$TARGET")"
|
||||
assert_eq "edit made through Edit survives Stop" "1" "$(grep -c 'const added = 3;' "$TARGET")"
|
||||
assert_eq "protected block back on disk" "1" "$(grep -c 'const protectedValue = 42;' "$TARGET")"
|
||||
assert_eq "no placeholder left after Stop" "0" "$(grep -c 'BLOCK_' "$TARGET")"
|
||||
fi
|
||||
|
||||
# ── Summary ──────────────────────────────────────────────────────────────
|
||||
printf '\n══════════════════════════════════════════\n'
|
||||
printf 'Results: %d passed, %d failed\n' "$PASS" "$FAIL"
|
||||
|
||||
+101
-64
@@ -4,10 +4,13 @@
|
||||
# PreToolUse Read → backs up file, replaces blocks with BLOCK_<hash> in-place
|
||||
# PostToolUse Edit → expands placeholders, re-filters so file stays hidden
|
||||
# PostToolUse Write → expands placeholders, re-filters so file stays hidden
|
||||
# Stop → restores real file content from backup
|
||||
# Stop → expands the placeholders still in the file on disk, so a
|
||||
# change made by any route survives; the backup is used
|
||||
# only when no placeholder is left to expand
|
||||
#
|
||||
# The file on disk ALWAYS has placeholders while the session is active.
|
||||
# The real content (with model's changes applied) lives in the backup.
|
||||
# The real content is that file with its placeholders expanded; the backup
|
||||
# holds the last state the hook itself saw.
|
||||
#
|
||||
# Dependencies: jq, shasum or sha1sum (auto-detected)
|
||||
|
||||
@@ -141,7 +144,87 @@ ${line}"
|
||||
[ $count -gt 0 ] && return 0 || return 1
|
||||
}
|
||||
|
||||
# ── Stop: restore all files from backup ───────────────────────────────────────
|
||||
# ── expand_file: replace BLOCK_<hash> placeholders with the real block code ───
|
||||
# Reads $1 (source), writes the expanded version to $2 (dest), using the blocks
|
||||
# cached under $3 (file id). Warns on placeholders that were altered or deleted.
|
||||
expand_file() {
|
||||
local src="$1" dest="$2" fid="$3"
|
||||
local line bf h bp bs br placeholder block_content esc_placeholder esc_fuzzy bh first_line
|
||||
rm -f "$dest"
|
||||
while IFS= read -r line || [ -n "$line" ]; do
|
||||
case "$line" in *BLOCK_*)
|
||||
# Expand all placeholders on this line (supports multiple per line)
|
||||
for bf in "$CACHE/${fid}".block.*; do
|
||||
[ -f "$bf" ] || continue
|
||||
h="${bf##*.}"
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
# Reconstruct the exact placeholder pattern
|
||||
bp=""; bs=""; br=""
|
||||
[ -f "$CACHE/${fid}.prefix.${h}" ] && bp=$(cat "$CACHE/${fid}.prefix.${h}")
|
||||
[ -f "$CACHE/${fid}.suffix.${h}" ] && bs=$(cat "$CACHE/${fid}.suffix.${h}")
|
||||
[ -f "$CACHE/${fid}.reason.${h}" ] && br=$(cat "$CACHE/${fid}.reason.${h}")
|
||||
if [ -n "$br" ]; then
|
||||
placeholder="${bp}BLOCK_${h}: ${br}${bs}"
|
||||
else
|
||||
placeholder="${bp}BLOCK_${h}${bs}"
|
||||
fi
|
||||
block_content=$(cat "$bf"; printf x); block_content="${block_content%x}"
|
||||
# Escape glob metacharacters (* ? [ \) in the pattern
|
||||
esc_placeholder=$(escape_glob "$placeholder")
|
||||
# Bash native substitution (// = global replace): replace placeholder, keep surrounding code
|
||||
line="${line//$esc_placeholder/$block_content}"
|
||||
# Fallback: if model altered the reason text, try without reason
|
||||
# (only trigger if BLOCK_hash is still present AND wasn't in the original block content)
|
||||
case "$block_content" in *"BLOCK_${h}"*) ;; *)
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
printf 'Warning: placeholder BLOCK_%s was modified by model, using fuzzy match\n' "$h" >&2
|
||||
esc_fuzzy=$(escape_glob "${bp}BLOCK_${h}${bs}")
|
||||
line="${line//$esc_fuzzy/$block_content}"
|
||||
# Last resort: match just the hash token
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
line="${line//BLOCK_${h}/$block_content}"
|
||||
;; esac
|
||||
;; esac
|
||||
;; esac
|
||||
;; esac
|
||||
done
|
||||
;; esac
|
||||
printf '%s\n' "$line" >> "$dest"
|
||||
done < "$src"
|
||||
# Preserve trailing newline status
|
||||
if [ -s "$dest" ] && [ -s "$src" ] && [ -n "$(tail -c 1 "$src")" ]; then
|
||||
perl -pe 'chomp if eof' "$dest" > "${dest}.nnl" && \
|
||||
cat "${dest}.nnl" > "$dest" && rm -f "${dest}.nnl"
|
||||
fi
|
||||
# Warn if model deleted a protected block entirely
|
||||
for bf in "$CACHE/${fid}".block.*; do
|
||||
[ -f "$bf" ] || continue
|
||||
bh="${bf##*.}"
|
||||
# After expansion, blocks appear as original code (simplify-ignore-start).
|
||||
# If neither the expanded code nor the placeholder is in EXPANDED, it was deleted.
|
||||
if ! grep -qF "BLOCK_${bh}" "$dest" 2>/dev/null; then
|
||||
# Get first line of block to check if it was expanded back
|
||||
first_line=$(head -1 "$bf")
|
||||
if ! grep -qF "$first_line" "$dest" 2>/dev/null; then
|
||||
printf 'Warning: protected block BLOCK_%s was deleted by model\n' "$bh" >&2
|
||||
fi
|
||||
fi
|
||||
done
|
||||
return 0
|
||||
}
|
||||
|
||||
# ── has_placeholders: is a cached BLOCK_<hash> for $2 still present in file $1? ─
|
||||
has_placeholders() {
|
||||
local f="$1" fid="$2" bf h
|
||||
for bf in "$CACHE/${fid}".block.*; do
|
||||
[ -f "$bf" ] || continue
|
||||
h="${bf##*.}"
|
||||
grep -qF "BLOCK_${h}" "$f" 2>/dev/null && return 0
|
||||
done
|
||||
return 1
|
||||
}
|
||||
|
||||
# ── Stop: expand each cached file in place, backup only as fallback ──────────
|
||||
if [ -z "$TOOL_NAME" ]; then
|
||||
[ -d "$CACHE" ] || exit 0
|
||||
for bak in "$CACHE"/*.bak; do
|
||||
@@ -151,7 +234,20 @@ if [ -z "$TOOL_NAME" ]; then
|
||||
[ -f "$pathfile" ] || { rm -f "$bak"; continue; }
|
||||
orig=$(cat "$pathfile")
|
||||
if [ -f "$orig" ]; then
|
||||
cat "$bak" > "$orig"
|
||||
# A change can reach the file through a route that fires no Edit or Write
|
||||
# event (a Bash command, a formatter, an external editor), and the backup
|
||||
# is stale for all of them. Expand what is on disk rather than overwrite it.
|
||||
if has_placeholders "$orig" "$fid"; then
|
||||
EXPANDED="$CACHE/${fid}.$$.expanded"
|
||||
expand_file "$orig" "$EXPANDED" "$fid"
|
||||
cat "$EXPANDED" > "$orig"
|
||||
rm -f "$EXPANDED"
|
||||
else
|
||||
# Nothing left to expand — the file was rewritten wholesale, so the
|
||||
# backup is the only remaining copy of the protected blocks.
|
||||
printf 'Warning: no BLOCK_ placeholder left in %s, restored from backup\n' "$orig" >&2
|
||||
cat "$bak" > "$orig"
|
||||
fi
|
||||
rm -f "$bak" "$pathfile" "$CACHE/${fid}".block.* "$CACHE/${fid}".reason.* "$CACHE/${fid}".prefix.* "$CACHE/${fid}".suffix.*
|
||||
rmdir "$CACHE/${fid}.lock" 2>/dev/null
|
||||
else
|
||||
@@ -223,66 +319,7 @@ if [ "$TOOL_NAME" = "Edit" ] || [ "$TOOL_NAME" = "Write" ]; then
|
||||
|
||||
# Expand placeholders, preserving any inline code the model added around them
|
||||
EXPANDED="$CACHE/${ID}.$$.expanded"
|
||||
rm -f "$EXPANDED"
|
||||
while IFS= read -r line || [ -n "$line" ]; do
|
||||
case "$line" in *BLOCK_*)
|
||||
# Expand all placeholders on this line (supports multiple per line)
|
||||
for bf in "$CACHE/${ID}".block.*; do
|
||||
[ -f "$bf" ] || continue
|
||||
h="${bf##*.}"
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
# Reconstruct the exact placeholder pattern
|
||||
bp=""; bs=""; br=""
|
||||
[ -f "$CACHE/${ID}.prefix.${h}" ] && bp=$(cat "$CACHE/${ID}.prefix.${h}")
|
||||
[ -f "$CACHE/${ID}.suffix.${h}" ] && bs=$(cat "$CACHE/${ID}.suffix.${h}")
|
||||
[ -f "$CACHE/${ID}.reason.${h}" ] && br=$(cat "$CACHE/${ID}.reason.${h}")
|
||||
if [ -n "$br" ]; then
|
||||
placeholder="${bp}BLOCK_${h}: ${br}${bs}"
|
||||
else
|
||||
placeholder="${bp}BLOCK_${h}${bs}"
|
||||
fi
|
||||
block_content=$(cat "$bf"; printf x); block_content="${block_content%x}"
|
||||
# Escape glob metacharacters (* ? [ \) in the pattern
|
||||
esc_placeholder=$(escape_glob "$placeholder")
|
||||
# Bash native substitution (// = global replace): replace placeholder, keep surrounding code
|
||||
line="${line//$esc_placeholder/$block_content}"
|
||||
# Fallback: if model altered the reason text, try without reason
|
||||
# (only trigger if BLOCK_hash is still present AND wasn't in the original block content)
|
||||
case "$block_content" in *"BLOCK_${h}"*) ;; *)
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
printf 'Warning: placeholder BLOCK_%s was modified by model, using fuzzy match\n' "$h" >&2
|
||||
esc_fuzzy=$(escape_glob "${bp}BLOCK_${h}${bs}")
|
||||
line="${line//$esc_fuzzy/$block_content}"
|
||||
# Last resort: match just the hash token
|
||||
case "$line" in *"BLOCK_${h}"*)
|
||||
line="${line//BLOCK_${h}/$block_content}"
|
||||
;; esac
|
||||
;; esac
|
||||
;; esac
|
||||
;; esac
|
||||
done
|
||||
;; esac
|
||||
printf '%s\n' "$line" >> "$EXPANDED"
|
||||
done < "$FILE_PATH"
|
||||
# Preserve trailing newline status
|
||||
if [ -s "$EXPANDED" ] && [ -s "$FILE_PATH" ] && [ -n "$(tail -c 1 "$FILE_PATH")" ]; then
|
||||
perl -pe 'chomp if eof' "$EXPANDED" > "${EXPANDED}.nnl" && \
|
||||
cat "${EXPANDED}.nnl" > "$EXPANDED" && rm -f "${EXPANDED}.nnl"
|
||||
fi
|
||||
# Warn if model deleted a protected block entirely
|
||||
for bf in "$CACHE/${ID}".block.*; do
|
||||
[ -f "$bf" ] || continue
|
||||
bh="${bf##*.}"
|
||||
# After expansion, blocks appear as original code (simplify-ignore-start).
|
||||
# If neither the expanded code nor the placeholder is in EXPANDED, it was deleted.
|
||||
if ! grep -qF "BLOCK_${bh}" "$EXPANDED" 2>/dev/null; then
|
||||
# Get first line of block to check if it was expanded back
|
||||
first_line=$(head -1 "$bf")
|
||||
if ! grep -qF "$first_line" "$EXPANDED" 2>/dev/null; then
|
||||
printf 'Warning: protected block BLOCK_%s was deleted by model\n' "$bh" >&2
|
||||
fi
|
||||
fi
|
||||
done
|
||||
expand_file "$FILE_PATH" "$EXPANDED" "$ID"
|
||||
# Preserve inode and permissions
|
||||
cat "$EXPANDED" > "$FILE_PATH"
|
||||
rm -f "$EXPANDED"
|
||||
|
||||
Reference in new issue
Block a user