-
Notifications
You must be signed in to change notification settings - Fork 0
fix(ai-engineer): fail-closed memory rewrite helper for shared store #2546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # memory-rewrite.sh — fail-closed whole-file rewrite for the shared durable | ||
| # memory store (monorepo#2293). | ||
| # | ||
| # The footgun this closes: agents "surgically" trim a section with | ||
| # s=$(grep -n '^## …' "$f" | cut -d: -f1) | ||
| # { sed -n "1,$((s-1))p" "$f"; echo replacement; } > /tmp/new && mv /tmp/new "$f" | ||
| # When grep matches nothing (a sibling restructured the file), s is empty, | ||
| # sed gets `1,-1p`, errors and emits nothing, the `{…}` block still produces | ||
| # a near-empty stream, and `mv` permanently clobbers an unversioned file. | ||
| # Measured 2026-07-20: two independent ticks destroyed learnings.md and | ||
| # portfolio-status.md the same day; neither was recoverable. | ||
| # | ||
| # This helper backs up first, refuses empty/non-positive keep-through bounds, | ||
| # refuses empty or drastically-shrunk rebuilds (unless --allow-shrink), refuses | ||
| # a rebuild that drops a markdown heading the original had, and only then | ||
| # swaps. Prefer a non-clobbering append when you can; use this only when a | ||
| # whole-file rewrite is genuinely required. | ||
| # | ||
| # Usage: | ||
| # memory-rewrite.sh --file <path> --from <new-content-path> [options] | ||
| # memory-rewrite.sh --file <path> --stdin [options] | ||
| # memory-rewrite.sh --file <path> --keep-through <N> --suffix <path> [options] | ||
| # | ||
| # Options: | ||
| # --allow-shrink permit a rebuild smaller than --max-shrink-pct of the original | ||
| # --max-shrink-pct N refuse when new size < (100-N)% of old (default: 50) | ||
| # --backup-dir <dir> directory for the pre-swap backup (default: same dir as --file) | ||
| # | ||
| # Exit codes: | ||
| # 0 rewrite applied; stdout carries `backup=<path>` | ||
| # 1 content refused (empty, shrink, lost heading) — target untouched | ||
| # 2 usage / bound error — target untouched | ||
| set -Eeuo pipefail | ||
|
|
||
| DEFAULT_MAX_SHRINK_PCT=50 | ||
|
|
||
| file="" | ||
| from="" | ||
| stdin=0 | ||
| keep_through="" | ||
| suffix="" | ||
| allow_shrink=0 | ||
| max_shrink_pct="$DEFAULT_MAX_SHRINK_PCT" | ||
| backup_dir="" | ||
|
|
||
| usage() { | ||
| sed -n '/^# Usage:/,/^# 2 /p' "$0" | sed 's/^# \{0,1\}//' | ||
| } | ||
|
|
||
| fail_usage() { | ||
| echo "memory-rewrite: $*" >&2 | ||
| usage >&2 | ||
| exit 2 | ||
| } | ||
|
|
||
| fail_content() { | ||
| echo "memory-rewrite: $*" >&2 | ||
| exit 1 | ||
| } | ||
|
|
||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --file) file="${2-}"; shift 2 || fail_usage "--file needs a path" ;; | ||
| --from) from="${2-}"; shift 2 || fail_usage "--from needs a path" ;; | ||
| --stdin) stdin=1; shift ;; | ||
| --keep-through) keep_through="${2-}"; shift 2 || fail_usage "--keep-through needs a line number" ;; | ||
| --suffix) suffix="${2-}"; shift 2 || fail_usage "--suffix needs a path" ;; | ||
| --allow-shrink) allow_shrink=1; shift ;; | ||
| --max-shrink-pct) max_shrink_pct="${2-}"; shift 2 || fail_usage "--max-shrink-pct needs an integer" ;; | ||
| --backup-dir) backup_dir="${2-}"; shift 2 || fail_usage "--backup-dir needs a path" ;; | ||
| -h|--help) usage; exit 0 ;; | ||
| *) fail_usage "unknown argument '$1'" ;; | ||
| esac | ||
| done | ||
|
|
||
| [[ -n "$file" ]] || fail_usage "--file is required" | ||
| [[ -f "$file" ]] || fail_usage "target file not found: $file" | ||
|
|
||
| # Exactly one content source. | ||
| source_count=0 | ||
| [[ -n "$from" ]] && source_count=$((source_count + 1)) | ||
| [[ "$stdin" -eq 1 ]] && source_count=$((source_count + 1)) | ||
| [[ -n "$keep_through" || -n "$suffix" ]] && source_count=$((source_count + 1)) | ||
| [[ "$source_count" -eq 1 ]] || fail_usage "provide exactly one of --from, --stdin, or --keep-through/--suffix" | ||
|
|
||
| if ! [[ "$max_shrink_pct" =~ ^[0-9]+$ ]]; then | ||
| fail_usage "--max-shrink-pct must be a non-negative integer (got '$max_shrink_pct')" | ||
| fi | ||
| printf -v max_shrink_pct '%d' "$((10#$max_shrink_pct))" | ||
| if [[ "$max_shrink_pct" -lt 0 || "$max_shrink_pct" -gt 99 ]]; then | ||
| fail_usage "--max-shrink-pct must be 0..99 (got '$max_shrink_pct')" | ||
| fi | ||
|
|
||
| workdir="$(mktemp -d "${TMPDIR:-/tmp}/memory-rewrite.XXXXXX")" | ||
| cleanup() { rm -rf "$workdir"; } | ||
| trap cleanup EXIT | ||
|
|
||
| new_path="$workdir/new" | ||
|
|
||
| if [[ -n "$from" ]]; then | ||
| [[ -f "$from" ]] || fail_usage "--from file not found: $from" | ||
| cp "$from" "$new_path" | ||
| elif [[ "$stdin" -eq 1 ]]; then | ||
| cat > "$new_path" | ||
| else | ||
| # Assemble: lines 1..N of the target + suffix. Bound must be a positive integer — | ||
| # this is the empty-grep failure mode made explicit. | ||
| if [[ -z "$keep_through" ]]; then | ||
| fail_usage "--keep-through is empty or missing (refusing the sed 1,-1p footgun)" | ||
| fi | ||
| # Reject a leading minus before base-10 normalisation — bash `$((10#-3))` is a | ||
| # syntax error and would exit 1 under `set -e` instead of the usage exit 2. | ||
| if [[ "$keep_through" == -* ]]; then | ||
| fail_usage "--keep-through must be >= 1 (got '$keep_through'); refusing empty/non-positive bound" | ||
| fi | ||
| if ! [[ "$keep_through" =~ ^[0-9]+$ ]]; then | ||
| fail_usage "--keep-through must be an integer (got '$keep_through')" | ||
| fi | ||
| # Normalise leading zeros via 10#; reject non-positive. | ||
| printf -v keep_through '%d' "$((10#$keep_through))" | ||
| if [[ "$keep_through" -lt 1 ]]; then | ||
| fail_usage "--keep-through must be >= 1 (got '$keep_through'); refusing empty/non-positive bound" | ||
| fi | ||
| [[ -n "$suffix" ]] || fail_usage "--keep-through requires --suffix" | ||
| [[ -f "$suffix" ]] || fail_usage "--suffix file not found: $suffix" | ||
| target_lines="$(wc -l < "$file" | tr -d '[:space:]')" | ||
| if [[ "$keep_through" -gt "$target_lines" ]]; then | ||
| fail_usage "--keep-through $keep_through exceeds file line count $target_lines" | ||
| fi | ||
| { | ||
| sed -n "1,${keep_through}p" "$file" | ||
| cat "$suffix" | ||
| } > "$new_path" | ||
| fi | ||
|
|
||
| old_bytes="$(wc -c < "$file" | tr -d '[:space:]')" | ||
| new_bytes="$(wc -c < "$new_path" | tr -d '[:space:]')" | ||
|
|
||
| if [[ "$new_bytes" -eq 0 ]]; then | ||
| fail_content "refused empty rebuild of $file (new content is 0 bytes); target untouched" | ||
| fi | ||
|
|
||
| if [[ "$allow_shrink" -eq 0 ]]; then | ||
| # Keep at least (100 - max_shrink_pct)% of the original size. | ||
| min_bytes=$(( old_bytes * (100 - max_shrink_pct) / 100 )) | ||
| # Always require at least 1 byte when old was non-empty (covered above). | ||
| if [[ "$old_bytes" -gt 0 && "$new_bytes" -lt "$min_bytes" ]]; then | ||
| fail_content "refused drastic shrink of $file: new=${new_bytes}B old=${old_bytes}B (below $((100 - max_shrink_pct))% keep); re-run with --allow-shrink if intentional" | ||
| fi | ||
| fi | ||
|
|
||
| # Structure guard: if the original had a markdown heading, the rebuild must too. | ||
| if grep -qE '^#{1,6}[[:space:]]' "$file"; then | ||
| if ! grep -qE '^#{1,6}[[:space:]]' "$new_path"; then | ||
| fail_content "refused rebuild of $file: original had a markdown heading and the new content does not; target untouched" | ||
| fi | ||
| fi | ||
|
|
||
| # Frontmatter guard: if the original opened with YAML frontmatter, keep it. | ||
| if head -n 1 "$file" | grep -qx -- '---'; then | ||
| if ! head -n 1 "$new_path" | grep -qx -- '---'; then | ||
| fail_content "refused rebuild of $file: original had YAML frontmatter and the new content does not; target untouched" | ||
| fi | ||
| fi | ||
|
|
||
| # Backup first, then atomic-ish replace via temp + mv in the same directory. | ||
| if [[ -z "$backup_dir" ]]; then | ||
| backup_dir="$(dirname "$file")" | ||
| fi | ||
| mkdir -p "$backup_dir" | ||
| stamp="$(date -u +%Y%m%dT%H%M%SZ)" | ||
| base="$(basename "$file")" | ||
| backup_path="$backup_dir/${base}.bak.${stamp}.$$" | ||
| cp "$file" "$backup_path" | ||
|
|
||
| swap="$file.memory-rewrite.$$" | ||
| cp "$new_path" "$swap" | ||
| mv "$swap" "$file" | ||
|
|
||
| printf 'memory-rewrite: ok file=%s backup=%s new_bytes=%s\n' "$file" "$backup_path" "$new_bytes" | ||
| exit 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # Self-test for memory-rewrite.sh — RED-proves the two real clobber modes from | ||
| # monorepo#2293: an empty/non-positive keep-through bound (the sed `1,-1p` | ||
| # footgun), and a rebuild that is empty or drastically smaller than the | ||
| # original. Also asserts backups are written and reported, and that an | ||
| # intentional shrink needs --allow-shrink. | ||
| # | ||
| # Fixtures are throwaway files in a temp dir — no real memory touched. | ||
| set -Eeuo pipefail | ||
|
|
||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| tool="$script_dir/memory-rewrite.sh" | ||
|
|
||
| tmp="$(mktemp -d)" | ||
| trap 'rm -rf "$tmp"' EXIT | ||
|
|
||
| failures=0 | ||
| pass() { printf 'ok — %s\n' "$1"; } | ||
| fail() { printf 'FAIL — %s\n' "$1"; failures=$(( failures + 1 )); } | ||
|
|
||
| check() { | ||
| local desc="$1" expected="$2" actual="$3" | ||
| if [[ "$expected" == "$actual" ]]; then pass "$desc"; else | ||
| fail "$desc (expected '$expected', got '$actual')" | ||
| fi | ||
| } | ||
|
|
||
| run() { local rc=0; "$tool" "$@" >/dev/null 2>&1 || rc=$?; echo "$rc"; } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Happy path: full rewrite from --from keeps content and reports a backup. | ||
| # --------------------------------------------------------------------------- | ||
| target="$tmp/happy.md" | ||
| printf '%s\n' '---' 'version: 1' '---' '# Notes' '' 'old body line' > "$target" | ||
| new="$tmp/happy-new.md" | ||
| printf '%s\n' '---' 'version: 1' '---' '# Notes' '' 'new body line' 'another' > "$new" | ||
| rc=0 | ||
| out="$("$tool" --file "$target" --from "$new" 2>&1)" || rc=$? | ||
| check "happy rewrite exit code" "0" "$rc" | ||
| if grep -q 'new body line' "$target" && grep -q '# Notes' "$target"; then | ||
| pass "happy rewrite installs new content" | ||
| else | ||
| fail "happy rewrite installs new content" | ||
| fi | ||
| if grep -q 'backup=' <<<"$out"; then | ||
| pass "happy rewrite reports backup= path" | ||
| bak="$(sed -n 's/.*backup=//p' <<<"$out" | awk '{print $1}' | head -1)" | ||
| if [[ -f "$bak" ]] && grep -q 'old body line' "$bak"; then | ||
| pass "backup file preserves prior content" | ||
| else | ||
| fail "backup file preserves prior content (bak='$bak')" | ||
| fi | ||
| else | ||
| fail "happy rewrite reports backup= path (got: $out)" | ||
| fi | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # RED case 1: empty / non-positive --keep-through bound must refuse BEFORE | ||
| # touching the target (the empty-grep → sed 1,-1p clobber). | ||
| # --------------------------------------------------------------------------- | ||
| target="$tmp/bound.md" | ||
| printf '%s\n' '# Heading' 'line2' 'line3' 'line4' 'line5' > "$target" | ||
| before="$(cat "$target")" | ||
| suffix="$tmp/suffix.md" | ||
| printf '%s\n' 'replacement section' > "$suffix" | ||
|
|
||
| rc=0 | ||
| out="$("$tool" --file "$target" --keep-through '' --suffix "$suffix" 2>&1)" || rc=$? | ||
| check "empty keep-through exits non-zero" "2" "$rc" | ||
| check "empty keep-through leaves target untouched" "$before" "$(cat "$target")" | ||
| if grep -qi 'keep-through' <<<"$out"; then | ||
| pass "empty keep-through names the bound in the error" | ||
| else | ||
| fail "empty keep-through names the bound in the error (got: $out)" | ||
| fi | ||
|
|
||
| rc=0 | ||
| out="$("$tool" --file "$target" --keep-through 0 --suffix "$suffix" 2>&1)" || rc=$? | ||
| check "zero keep-through exits non-zero" "2" "$rc" | ||
| check "zero keep-through leaves target untouched" "$before" "$(cat "$target")" | ||
|
|
||
| rc=0 | ||
| out="$("$tool" --file "$target" --keep-through -3 --suffix "$suffix" 2>&1)" || rc=$? | ||
| check "negative keep-through exits non-zero" "2" "$rc" | ||
| check "negative keep-through leaves target untouched" "$before" "$(cat "$target")" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # RED case 2: empty / near-empty rebuild must refuse (the sed-error → empty | ||
| # `{…}` block still producing output, then mv clobber). | ||
| # --------------------------------------------------------------------------- | ||
| target="$tmp/empty-rebuild.md" | ||
| { | ||
| printf '%s\n' '---' 'version: 1' '---' '# Portfolio status' | ||
| local_i=0 | ||
| for (( local_i = 0; local_i < 40; local_i++ )); do | ||
| printf 'tick note line %s with enough bytes to matter for shrink detection\n' "$local_i" | ||
| done | ||
| } > "$target" | ||
| before="$(cat "$target")" | ||
| empty_new="$tmp/empty-new.md" | ||
| : > "$empty_new" | ||
|
|
||
| rc=0 | ||
| out="$("$tool" --file "$target" --from "$empty_new" 2>&1)" || rc=$? | ||
| check "empty rebuild exits non-zero" "1" "$rc" | ||
| check "empty rebuild leaves target untouched" "$before" "$(cat "$target")" | ||
| if grep -qiE 'empty|refuse|rejected' <<<"$out"; then | ||
| pass "empty rebuild error mentions refusal" | ||
| else | ||
| fail "empty rebuild error mentions refusal (got: $out)" | ||
| fi | ||
|
|
||
| tiny_new="$tmp/tiny-new.md" | ||
| # Keep frontmatter + heading so only the shrink guard fires (not structure). | ||
| printf '%s\n' '---' 'version: 1' '---' '# Portfolio status' 'x' > "$tiny_new" | ||
| rc=0 | ||
| out="$("$tool" --file "$target" --from "$tiny_new" 2>&1)" || rc=$? | ||
| check "drastic shrink exits non-zero by default" "1" "$rc" | ||
| check "drastic shrink leaves target untouched" "$before" "$(cat "$target")" | ||
| if grep -qiE 'shrink|smaller|allow-shrink' <<<"$out"; then | ||
| pass "drastic shrink error mentions shrink / allow-shrink" | ||
| else | ||
| fail "drastic shrink error mentions shrink / allow-shrink (got: $out)" | ||
| fi | ||
|
|
||
| # Opt-in shrink is allowed when explicitly requested (and still backs up). | ||
| rc=0 | ||
| out="$("$tool" --file "$target" --from "$tiny_new" --allow-shrink 2>&1)" || rc=$? | ||
| check "allow-shrink permits intentional shrink" "0" "$rc" | ||
| if grep -q '^# Portfolio status$' "$target" && grep -q 'backup=' <<<"$out"; then | ||
| pass "allow-shrink installs tiny content with backup" | ||
| else | ||
| fail "allow-shrink installs tiny content with backup (got out: $out / file: $(cat "$target"))" | ||
| fi | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # keep-through assemble path succeeds with a positive bound. | ||
| # --------------------------------------------------------------------------- | ||
| target="$tmp/assemble.md" | ||
| printf '%s\n' '# Heading' 'keep-me-1' 'keep-me-2' 'drop-me' 'drop-me-2' > "$target" | ||
| suffix="$tmp/suffix2.md" | ||
| printf '%s\n' '## Replacement' 'fresh content' > "$suffix" | ||
| rc=0 | ||
| out="$("$tool" --file "$target" --keep-through 3 --suffix "$suffix" 2>&1)" || rc=$? | ||
| check "assemble with positive keep-through exits 0" "0" "$rc" | ||
| if grep -q 'keep-me-2' "$target" && grep -q 'fresh content' "$target" && ! grep -q 'drop-me' "$target"; then | ||
| pass "assemble keeps prefix and appends suffix" | ||
| else | ||
| fail "assemble keeps prefix and appends suffix (got: $(cat "$target"))" | ||
| fi | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Structure guard: original had a markdown heading; new content without one | ||
| # is refused. | ||
| # --------------------------------------------------------------------------- | ||
| target="$tmp/heading.md" | ||
| printf '%s\n' '# Real heading' 'body body body body body body body body' > "$target" | ||
| before="$(cat "$target")" | ||
| nohead="$tmp/nohead.md" | ||
| printf '%s\n' 'just prose with no heading at all, padded enough not to trip shrink alone.......' > "$nohead" | ||
| rc=0 | ||
| out="$("$tool" --file "$target" --from "$nohead" 2>&1)" || rc=$? | ||
| check "lost heading exits non-zero" "1" "$rc" | ||
| check "lost heading leaves target untouched" "$before" "$(cat "$target")" | ||
|
Comment on lines
+153
to
+165
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Cover the YAML-frontmatter refusal path. The structure tests only cover lost headings. Add a fixture whose original starts with Based on learnings, executable 🤖 Prompt for AI AgentsSource: Learnings |
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Usage errors. | ||
| # --------------------------------------------------------------------------- | ||
| check "missing --file exits 2" "2" "$(run --from "$new")" | ||
| check "missing content source exits 2" "2" "$(run --file "$tmp/happy.md")" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| if [[ "$failures" -eq 0 ]]; then | ||
| printf '\nAll memory-rewrite tests passed.\n' | ||
| exit 0 | ||
| fi | ||
| printf '\n%d memory-rewrite test(s) FAILED.\n' "$failures" | ||
| exit 1 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject stale rewrites before replacing shared memory.
A sibling can change
$fileafter candidate assembly/validation but beforemv. This code backs up that newer content and then silently overwrites it with the stale candidate, defeating the documented multi-writer fail-closed behavior. Hold a shared rewrite lock from the initial read through replacement and verify the target fingerprint immediately before swapping.🤖 Prompt for AI Agents
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the target’s permissions during replacement.
cp "$new_path" "$swap"creates the swap file with the candidate’s mode. Replacing a0600memory file with a typical0644candidate can expose its contents to other local users.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents