From c2af52eeec289da937b739d4d78ae94146ca641f Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 18:54:25 +0200 Subject: [PATCH 1/3] fix: atomic swap so a crashed helper never truncates a committed index (#480) truncate_index() used to `: > "$tsv"` the REAL, committed 00-index.tsv before a single row of the new one existed. On macOS 27 the built-in bash can kill forked awk children with "Bus error: 10" partway through rebuild-tsv.sh -- not reproducible under Homebrew bash, and too rare in this plugin's own short test runs to ever show up there. The run already detected one shape of that crash and reported jit_rc 2, but kept writing into the already-truncated file, so "detected" did not mean "preserved": 7 indexes in the reporter's own project ended up empty, the largest going from 3,714 rows to 1,430. Every index is now built into a temp file in the same directory (truncate_index) and swapped in with one mv (commit_index), only when JIT_RC2_COUNT did not advance while that index's own loop ran -- a helper dying anywhere inside it leaves the previously committed index untouched rather than replaced by a partial one. A real subshell bug surfaced while wiring this up: truncate_index()'s own jit_rc(2) calls run inside the $( ) that captures its temp path, so they never reach the caller's JIT_RC -- every call site now raises jit_rc 2 explicitly on a truncate_index failure. test-rebuild-exit-codes.sh's section G is updated for the resulting, deliberate behaviour change: mv's rename() only needs write permission on the directory, not on the file it replaces, so a chmod'd-444 index file no longer blocks a rebuild -- the genuine "cannot write" case moved to the directory. New test-rebuild-atomic-swap-480.sh drives a real SIGBUS-killed awk child via a shimmed PATH and asserts the committed index is byte-identical before and after. Closes #480 Co-Authored-By: Claude Sonnet 5 --- .../jit-context/paths/00-manual/tooling.md | 4 +- changelog.d/480.fixed.md | 13 ++ scripts/rebuild-tsv.sh | 205 ++++++++++++++---- tests/test-rebuild-atomic-swap-480.sh | 194 +++++++++++++++++ tests/test-rebuild-exit-codes.sh | 35 ++- 5 files changed, 395 insertions(+), 56 deletions(-) create mode 100644 changelog.d/480.fixed.md create mode 100755 tests/test-rebuild-atomic-swap-480.sh diff --git a/.claude/jit-context/paths/00-manual/tooling.md b/.claude/jit-context/paths/00-manual/tooling.md index 0a41ec2c..505c4bf2 100644 --- a/.claude/jit-context/paths/00-manual/tooling.md +++ b/.claude/jit-context/paths/00-manual/tooling.md @@ -76,7 +76,9 @@ The half that was never a judgement call: **a newline in a name forged a whole r **A withheld name must not cost the reader the row.** `jit-dry-run.sh` keeps every refused row identifiable without printing the name: a `REFUSED`/`WARN` pattern row still prints its pattern verbatim on its own `untrusted>` line, and `check_entry_file()` — the one report whose whole class is unprintable, since a name carrying a separator is never a plain name either — closes with the position, worded exactly as `jit_row_id()` words it for the hooks. A fix that withheld the name and said nothing else would have replaced a disclosure with a dead end. -One line here prints a whole path rather than a name, and it needed a second fix: `truncate_index()`'s `FATAL`. Its `: > "$tsv" 2>/dev/null` applied the redirections **left to right**, so the failing one was set up while stderr was still the terminal — bash printed its own diagnostic carrying the absolute path, and the `2>/dev/null` that followed silenced nothing. A clone reaches that branch on purpose by shipping a *directory* named `00-index.tsv`. The redirection is now `: 2>/dev/null > "$tsv"`, and the reason bash would have given is replaced by the one case that is constructible. The `FATAL` then names `JIT_BASE` on its own line: the display string is `dimension/layer/leaf`, so the absolute path went with the withheld component, and the *ordinary* failure here — a read-only tree, a full disk — is the one where the reader needs a path they can act on. +One line here prints a whole path rather than a name, and it needed a second fix: `truncate_index()`'s `FATAL`. Its `: > "$tsv" 2>/dev/null` once applied the redirections **left to right**, so the failing one was set up while stderr was still the terminal — bash printed its own diagnostic carrying the absolute path, and the `2>/dev/null` that followed silenced nothing. A clone reaches that branch on purpose by shipping a *directory* named `00-index.tsv`. The `FATAL` names `JIT_BASE` on its own line: the display string is `dimension/layer/leaf`, so the absolute path went with the withheld component, and the *ordinary* failure here — a read-only tree, a full disk — is the one where the reader needs a path they can act on. + +**`truncate_index()` no longer truncates anything in place (#480).** The bash-leak this paragraph describes was about a REAL write to `$tsv` that could fail; since #480 that write never happens at all. Every index is built into a **temp file in the same directory** and `commit_index()` swaps it into place with one `mv`, and only when nothing crashed while that index was being built — a helper process dying partway through the loop (macOS 27's built-in bash killing forked `awk` children with `Bus error: 10`, not reproducible under Homebrew bash) used to leave the committed index truncated or half-written, because the truncation happened before the loop even started. Now a crash still exits the whole run non-zero, but the previously committed index for the layer that crashed is left on disk, untouched. The one consequence worth knowing if you touch this code: `mv`'s rename() only needs write permission on the **directory**, never on the file it replaces, so a `00-index.tsv` that is itself `chmod 444` no longer blocks a rebuild the way it used to — the genuine "cannot write" case is now the directory, not the file. **A check here has to agree with the hooks, because the hooks send people here.** The refusal notice they inject ends with `bash scripts/jit-dry-run.sh --base `, so a class the hooks refuse and this tool reports `ok` for makes that advice false — this repository's own defect class, in the tool written to report it. The byte checks (`#77`, `#78`) are read off the same `common.sh` functions the hooks use rather than a second copy in bash. Two traps came with them: `read -r` in the row loops truncates at a NUL, so that pass is one `awk` per index file rather than a bash loop; and that `awk` must be invoked `LC_ALL=C`, because the byte range it builds is a decode failure in a UTF-8 locale — unpinned, one-true-awk aborted the whole program on the first row of a tree carrying the byte the check exists to find. diff --git a/changelog.d/480.fixed.md b/changelog.d/480.fixed.md new file mode 100644 index 00000000..f0d81b63 --- /dev/null +++ b/changelog.d/480.fixed.md @@ -0,0 +1,13 @@ +- **A crashed helper process no longer truncates a committed index** (#480). On macOS 27, + the built-in `/bin/bash` 3.2.57 can kill forked `awk` children with `Bus error: 10` + partway through `rebuild-tsv.sh` -- not reproducible under Homebrew bash, and too rare in + this plugin's own short test runs to ever show up there. `rebuild-tsv.sh` already + detected one shape of that crash and reported it, but it had already truncated the REAL, + committed `00-index.tsv` before the rebuild loop even started, so the run kept writing + into the file a fresh clone and the hooks read as this tree's rules -- in the reporter's + project, 7 indexes ended up empty and the largest went from 3,714 rows to 1,430. + + Every index is now built into a temp file in the same directory and swapped into place + with a single `mv` only when that index's own build finished clean; a crashed helper + still exits the whole rebuild non-zero, but the previously committed index for the layer + it crashed in is left on disk, untouched, byte-identical to what it was before the run. diff --git a/scripts/rebuild-tsv.sh b/scripts/rebuild-tsv.sh index 1a89d742..27c33fbb 100755 --- a/scripts/rebuild-tsv.sh +++ b/scripts/rebuild-tsv.sh @@ -159,7 +159,10 @@ fi # # 0 the index was written and every row can be honoured # 1 the index was written, and at least one row will be REFUSED by the matcher -# 2 the index was not written, or not completely -- what is on disk is not this run +# 2 at least one index was not written. Since #480 that means ONLY "not written" -- +# truncate_index()/commit_index() build every index aside and swap it in only on a +# clean build, so a layer this run could not finish leaves its PREVIOUSLY committed +# index untouched rather than replaced by a partial one. # # The same 0/1/2 jit-dry-run.sh uses, on purpose: the two are read together and documented # in one table in paths/00-manual/tooling.md. @@ -191,8 +194,14 @@ fi # the same report block as the ambiguity tally. JIT_RC=0 # 2 outranks 1: an index that was not written is a worse claim than one that was. +# JIT_RC2_COUNT (#480): a plain max cannot tell "one FATAL happened, ever" apart from +# "a FATAL happened during THIS index's own build" -- commit_index() below needs the +# latter, snapshotted before and after one builder's loop, to decide whether a crashed +# helper process poisoned the index it is about to swap into place. +JIT_RC2_COUNT=0 jit_rc() { [ "$1" -gt "$JIT_RC" ] && JIT_RC="$1" + [ "$1" -eq 2 ] && JIT_RC2_COUNT=$((JIT_RC2_COUNT + 1)) return 0 } @@ -331,23 +340,30 @@ fi # silent one. echo "rebuild-tsv: writing JIT_BASE=$JIT_BASE (CLAUDE_PROJECT_DIR=${CLAUDE_PROJECT_DIR:-}, cwd=$(pwd))" >&2 -# Truncation failing left the previous index in place while every line after it reported -# the rule count read back OUT of that stale file -- a success, with a number, for an index -# nobody rebuilt. The other dimensions are independent, so the run continues and the code -# is raised once at the end. +# A failed build used to leave the previous index in place while every COUNT line after it +# was still read back OUT of that now-stale file -- a success, with a number, for an index +# nobody rebuilt. Since #480, COUNT is read from the TEMP build instead (see truncate_index/ +# commit_index just below), so that specific misreport is gone; the dimensions stay +# independent either way, and the run continues with the worst code raised once at the end. # DISP, not the path: the failing path runs through the layer directory, which is a name # the clone chose, and a clone can force this branch on purpose by shipping a DIRECTORY # called 00-index.tsv. Callers pass the already-withheld label plus the constant leaf, so # the reader still gets the two components that say which index this was (#113). +# Builds the next index ASIDE and reports where (#480): the caller's loop writes rows +# into the printed TEMP path, never into $tsv directly, and commit_index() below is the +# only thing that ever touches $tsv again. Before this, `: > "$tsv"` truncated the REAL, +# committed index before a single row of the new one existed -- so a helper process that +# died partway through the loop (a macOS 27 built-in-bash child taking "Bus error: 10", +# #480's own report) left whatever had been appended so far, which is to say an index +# shorter than the one on disk a moment earlier, silently committed as this run's answer. truncate_index() { - local tsv="$1" disp="${2:-$1}" why="" - # A SYMBOLIC LINK at this path is checked BEFORE the truncating redirect below, not - # after: `: > "$tsv"` truncates through a link exactly as readily as it truncates a - # real file, so by the time the redirect below could fail on anything, the outside - # target is already gone (#332). git clone recreates a committed symlink, so cloning a - # hostile tree is the whole attack -- the same one test-symlink-entry.sh already closed - # for an ENTRY file, one write site over. `[ -L ]` never follows, so this sees the link - # itself even when its target does not exist. + local tsv="$1" disp="${2:-$1}" tmp + # A SYMBOLIC LINK at this path is checked BEFORE anything is created, not after: `[ -L ]` + # never follows, so this sees the link itself even when its target does not exist. + # git clone recreates a committed symlink, so cloning a hostile tree is the whole attack + # -- the same one test-symlink-entry.sh already closed for an ENTRY file, one write site + # over. Checked again, against the same $tsv, by commit_index() right before the swap: + # this check only proves the path was not a symlink a moment ago. if [ -L "$tsv" ]; then echo "FATAL $disp: could not be written -- that path is a SYMBOLIC LINK, not a file" >&2 echo " -- refusing to truncate or write through a symlinked index path" >&2 @@ -356,27 +372,74 @@ truncate_index() { jit_rc 2 return 1 fi - # `2>/dev/null` BEFORE the redirection it is meant to silence, and this was a real leak. - # Redirections are applied left to right, so `: > "$tsv" 2>/dev/null` set up the failing - # one while stderr was still the terminal: bash printed its own diagnostic, carrying the - # ABSOLUTE path -- layer directory included -- and the 2>/dev/null that follows silenced - # nothing. Measured against `00-index.tsv` shipped as a directory. - if : 2> /dev/null > "$tsv"; then return 0; fi - # bash own reason is gone with that message, so the one case a clone can construct on - # purpose is classified here instead. Everything else stays unattributed rather than - # guessed at. - [ -d "$tsv" ] && why=" -- there is a DIRECTORY at that path, not a file" - echo "FATAL $disp: could not be written$why" >&2 - echo " -- that index was NOT rebuilt and is now stale." >&2 - # DISP is dimension/layer/leaf, so the absolute path is gone with the withheld component. - # JIT_BASE gets it back for the ordinary failure -- a read-only tree, a full disk -- which - # is the common one and the one where the reader needs a path they can act on. It is the - # same string the no-entry-tree FATAL above already prints, and it comes from - # CLAUDE_PROJECT_DIR rather than from the clone, so it is not the column this change is - # about. `ls` under it finds a withheld name in one step. - echo " -- under JIT_BASE=$JIT_BASE" >&2 - jit_rc 2 - return 1 + # The one case a clone can construct on purpose, classified before mktemp even runs -- + # a DIRECTORY at $tsv would otherwise make commit_index()'s own `mv` move the temp file + # INTO that directory instead of failing, which is worse than the old behaviour, not + # merely unfixed by it. + if [ -d "$tsv" ]; then + echo "FATAL $disp: could not be written -- there is a DIRECTORY at that path, not a file" >&2 + echo " -- that index was NOT rebuilt and is now stale." >&2 + echo " -- under JIT_BASE=$JIT_BASE" >&2 + jit_rc 2 + return 1 + fi + # Same directory as the real index, never $TMPDIR: a rename within one directory is the + # atomic swap a crash mid-build cannot half-apply, and crossing a filesystem boundary + # would turn commit_index()'s `mv` into a copy-then-unlink, un-doing exactly the + # atomicity this exists to buy. The leading dot keeps the scratch file out of every + # `*.md` glob the builders below walk; mktemp's own suffix keeps two concurrent rebuilds + # of the same tree from colliding on the same scratch name. + tmp="$(mktemp "$(dirname "$tsv")/.$(basename "$tsv").XXXXXX" 2> /dev/null)" + if [ -z "$tmp" ] || [ ! -e "$tmp" ]; then + echo "FATAL $disp: could not create a temp file to rebuild into" >&2 + echo " -- that index was NOT rebuilt and is now stale." >&2 + echo " -- under JIT_BASE=$JIT_BASE" >&2 + jit_rc 2 + return 1 + fi + printf '%s' "$tmp" + return 0 +} + +# Swaps a freshly-built TEMP index into place -- but only when nothing called jit_rc(2) +# for THIS index while it was being built (#480). The per-file loops below already detect +# a crashed helper (a non-zero awk exit, a verdict count that comes up short) and call +# jit_rc 2 when they do; comparing JIT_RC2_COUNT before and after one builder's own loop +# is what turns "somewhere, at some point in this whole run, something failed" into "THIS +# index's own build failed", without asking every builder to carry its own flag. +# +# $1 the temp path truncate_index() handed back, $2 the real destination ($tsv), $3 the +# display label, $4 the JIT_RC2_COUNT snapshot taken right after truncate_index() returned, +# before this index's own loop ran. +commit_index() { + local tmp="$1" tsv="$2" disp="$3" rc2_before="$4" + if [ "$JIT_RC2_COUNT" -gt "$rc2_before" ]; then + rm -f "$tmp" 2> /dev/null + echo "FATAL $disp: a helper process failed while this index was being rebuilt -- the partial rebuild was discarded and the COMMITTED index on disk was left untouched (#480)" >&2 + return 1 + fi + # Re-checked here, not just by truncate_index() above: nothing in this script can turn + # $tsv into a symlink mid-run on its own, but this is the narrowest possible window + # before the write. `mv`'s rename() would not write THROUGH the link the way the old + # truncating redirect did -- it replaces the directory entry itself, link included -- + # but that would silently turn a committed symlink into a plain file, which is still a + # change nobody asked for. Refused outright instead, the same call #332 already makes. + if [ -L "$tsv" ]; then + rm -f "$tmp" 2> /dev/null + echo "FATAL $disp: could not be written -- that path is now a SYMBOLIC LINK -- refusing to swap the rebuilt index into place" >&2 + echo " -- that index was NOT rebuilt and is now stale." >&2 + jit_rc 2 + return 1 + fi + if ! mv -f "$tmp" "$tsv" 2> /dev/null; then + rm -f "$tmp" 2> /dev/null + echo "FATAL $disp: the rebuilt index could not be moved into place" >&2 + echo " -- that index was NOT rebuilt and is now stale." >&2 + echo " -- under JIT_BASE=$JIT_BASE" >&2 + jit_rc 2 + return 1 + fi + return 0 } # A LAYER DIRECTORY that is itself a symbolic link is the other half of #332: @@ -454,7 +517,18 @@ build_tool_tsv() { T0=$(_ms) [ -d "$dir" ] || return - truncate_index "$tsv" "$label/${tsv##*/}" || return + local tsv_tmp rc2_before + # `|| { jit_rc 2; return; }`, not `|| return`: truncate_index() runs inside this + # command substitution's own SUBSHELL, so any jit_rc() call it makes internally never + # reaches JIT_RC/JIT_RC2_COUNT in this (the caller's) shell -- a subshell's variable + # writes die with it. Every failure return out of truncate_index() already means "the + # index was not written", so raising jit_rc 2 here, unconditionally, is not a guess + # about which failure it was. + tsv_tmp=$(truncate_index "$tsv" "$label/${tsv##*/}") || { + jit_rc 2 + return + } + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -525,10 +599,11 @@ build_tool_tsv() { # writing the row through -- see common.sh for why the row is not dropped. match=$(jit_expand_match "$match" tools "$label/$(jit_report_name "$filename")") || jit_rc 1 - printf '%s\t%s\t%s\t%s\t%s\t%s\t%s\n' "$tool" "$match" "$filename" "${mode:-remind}" "$require" "$forbid" "$requires" >> "$tsv" + printf '%s\t%s\t%s\t%s\t%s\t%s\t%s\n' "$tool" "$match" "$filename" "${mode:-remind}" "$require" "$forbid" "$requires" >> "$tsv_tmp" done - COUNT=$(wc -l < "$tsv" | tr -d ' ') + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" } @@ -789,7 +864,15 @@ build_vocab_tsv() { return fi - truncate_index "$tsv" "$label/${tsv##*/}" || return + local tsv_tmp rc2_before + # See build_tool_tsv()'s identical comment: truncate_index() runs in this command + # substitution's own subshell, so its internal jit_rc() calls never reach this shell's + # JIT_RC/JIT_RC2_COUNT -- raised explicitly here instead. + tsv_tmp=$(truncate_index "$tsv" "$label/${tsv##*/}") || { + jit_rc 2 + return + } + rc2_before=$JIT_RC2_COUNT # --- Deferred generic-word classification (#255) ----------------------------------- # Every non-blacklisted keyword this call writes a row for lands in ALL_KW (flat, one @@ -1137,10 +1220,11 @@ build_vocab_tsv() { " JIT_ALLGENERIC_N=$((JIT_ALLGENERIC_N + 1)) fi - printf '%s\n' "${_entry_rows[@]}" >> "$tsv" + printf '%s\n' "${_entry_rows[@]}" >> "$tsv_tmp" done - COUNT=$(wc -l < "$tsv" | tr -d ' ') + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT keywords" } @@ -1174,7 +1258,15 @@ build_vocab_path_tsv() { T0=$(_ms) [ -d "$dir" ] || return - truncate_index "$tsv" "$label/${tsv##*/}" || return + local tsv_tmp rc2_before + # See build_tool_tsv()'s identical comment: truncate_index() runs in this command + # substitution's own subshell, so its internal jit_rc() calls never reach this shell's + # JIT_RC/JIT_RC2_COUNT -- raised explicitly here instead. + tsv_tmp=$(truncate_index "$tsv" "$label/${tsv##*/}") || { + jit_rc 2 + return + } + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -1214,7 +1306,7 @@ build_vocab_path_tsv() { printf "%s%s/\t%s\n", prefix, m[i], file } } - ' "$md" >> "$tsv" + ' "$md" >> "$tsv_tmp" mod_rc=$? if [ "$mod_rc" -ne 0 ]; then echo "FATAL $label/${tsv##*/}: $(jit_report_name "$filename"): awk exited $mod_rc while reading its \"## Modules\" section -- rows for this file may be missing or partial, and the index is not this run" >&2 @@ -1222,7 +1314,8 @@ build_vocab_path_tsv() { fi done - COUNT=$(wc -l < "$tsv" | tr -d ' ') + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" # The LEAF, unlike every other builder's log line, because this is the only dimension # that writes two indexes out of one layer directory and the layer name alone would name # both. `$label/${tsv##*/}` is the same string truncate_index is handed above, so the @@ -1252,7 +1345,15 @@ build_path_tsv() { T0=$(_ms) [ -d "$dir" ] || return - truncate_index "$tsv" "$label/${tsv##*/}" || return + local tsv_tmp rc2_before + # See build_tool_tsv()'s identical comment: truncate_index() runs in this command + # substitution's own subshell, so its internal jit_rc() calls never reach this shell's + # JIT_RC/JIT_RC2_COUNT -- raised explicitly here instead. + tsv_tmp=$(truncate_index "$tsv" "$label/${tsv##*/}") || { + jit_rc 2 + return + } + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -1273,10 +1374,11 @@ build_path_tsv() { # indexed as a literal that can never match a path. match_line=$(jit_expand_match "$match_line" paths "$label/$(jit_report_name "$filename")") || jit_rc 1 - printf '%s\t%s\n' "$match_line" "$filename" >> "$tsv" + printf '%s\t%s\n' "$match_line" "$filename" >> "$tsv_tmp" done - COUNT=$(wc -l < "$tsv" | tr -d ' ') + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" } @@ -1750,8 +1852,15 @@ case "$JIT_RC" in echo " by the matcher. That rule is on disk and will never fire." >&2 ;; 2) - echo "rebuild-tsv: exit 2 -- an index could not be written. What is on disk is NOT what this" >&2 - echo " run built." >&2 + # #480: the FATAL lines above each build aside and swap in only on a clean build, so + # "what is on disk" is now the OLD committed index for anything this run could not + # finish -- stale, not corrupt, and still the index a hook would have read a moment + # ago. The one exception is the small set of guards that still exit before any + # builder runs at all (no entry tree found, a hostile CLAUDE_PROJECT_DIR) -- those + # print their own FATAL and `exit 2` directly, above, and never reach this line. + echo "rebuild-tsv: exit 2 -- at least one index above was NOT rebuilt. What is on disk for" >&2 + echo " it is the index this run STARTED with, left untouched rather than" >&2 + echo " replaced by a partial build." >&2 ;; esac exit "$JIT_RC" diff --git a/tests/test-rebuild-atomic-swap-480.sh b/tests/test-rebuild-atomic-swap-480.sh new file mode 100755 index 00000000..b5252cbd --- /dev/null +++ b/tests/test-rebuild-atomic-swap-480.sh @@ -0,0 +1,194 @@ +#!/bin/bash +# Tests for #480: a helper process that dies mid-rebuild must not truncate the index that +# was already committed. +# +# macOS 27's built-in /bin/bash 3.2.57 kills roughly twenty forked awk children with +# "Bus error: 10" (SIGBUS, exit 138) over the course of one real rebuild -- not +# reproducible under Homebrew bash, not reproducible running the same awk calls in +# isolation, and too rare in the plugin's own short test runs to ever show up there. The +# issue's own report: before this fix, rebuild-tsv.sh already detected one of those +# crashes (jit_rc 2) and kept going -- but `truncate_index()` had already truncated the +# REAL, committed 00-index.tsv before the loop even started, so "kept going" meant +# writing whatever came after the crash into the file a fresh clone (and the hooks) read +# as this tree's rules. 7 indexes in the reporter's own project ended up empty; the +# largest went from 3,714 rows to 1,430. +# +# This suite drives a real crash with a shimmed `awk` that dies on the ONE file named +# below, confirms rebuild-tsv exits 2, and -- the assertion #480 is actually about -- +# confirms the PREVIOUSLY COMMITTED index on disk is byte-identical to what it was before +# the crashed run, never shorter and never empty. A positive control runs the same +# fixture with no shim, so a harness that cannot drive a real crash reports that rather +# than a false pass. +# +# Usage: bash tests/test-rebuild-atomic-swap-480.sh + +set -uo pipefail + +REPO="$(cd "$(dirname "$0")/.." && pwd)" +REBUILD="$REPO/scripts/rebuild-tsv.sh" +PASS=0 +FAIL=0 + +ok() { + PASS=$((PASS + 1)) + echo " PASS: $1" +} +bad() { + FAIL=$((FAIL + 1)) + echo " FAIL: $1" + shift + [ $# -eq 0 ] || echo " $*" +} + +assert_rc() { + local desc="$1" want="$2" got="$3" + if [ "$got" = "$want" ]; then + ok "$desc" + else + bad "$desc" "wanted exit $want, got exit $got" + fi +} + +assert_contains() { + local desc="$1" out="$2" want="$3" + if grep -qF -- "$want" <<< "$out"; then + ok "$desc" + else + bad "$desc" "expected stderr to contain: $want" + fi +} + +# --- Positive control first (same shape test-awk-crash-397.sh uses): confirm this host +# can actually make a shimmed awk die with SIGBUS before trusting anything downstream of +# it. A shim that silently fails to crash would make every byte-identical assertion below +# pass for the wrong reason -- the loop below never actually crashed, it just never ran. +REAL_AWK="$(command -v awk)" +if [ -z "$REAL_AWK" ]; then + echo "SKIPPED: no 'awk' on PATH to wrap -- #480's crash-handling assertions are UNTESTED here." >&2 + exit 2 +fi +SHIM_DIR=$(mktemp -d 2> /dev/null || mktemp -d -t jit480) +if [ -z "$SHIM_DIR" ] || [ ! -d "$SHIM_DIR" ]; then + echo "SKIPPED: mktemp -d produced no directory, so no shim can be built here." >&2 + exit 2 +fi +trap 'rm -rf "$SHIM_DIR"' EXIT + +PROBE_OUT="$(PATH="$SHIM_DIR:$PATH" sh -c 'kill -BUS $$' 2>&1)" +PROBE_RC=$? +: "$PROBE_OUT" +if [ "$PROBE_RC" -ne 138 ] && [ "$PROBE_RC" -ne 135 ]; then + echo "SKIPPED: this host's shell cannot make a subprocess exit 138/135 via kill -BUS \$\$" >&2 + echo " (got exit $PROBE_RC instead) -- #480's crash-handling assertions are UNTESTED here." >&2 + exit 2 +fi + +# Crashes ONLY when its last argument is the sentinel file named below -- every other +# awk invocation during the run (frontmatter parsing on other files, the generic-word +# classifier canary, every other layer's own build) passes straight through to the real +# awk, so the rest of the tree still builds normally around the one crashed file. +cat > "$SHIM_DIR/awk" << SHIMEOF +#!/bin/sh +last="" +for a in "\$@"; do last="\$a"; done +case "\$last" in + */jit480-crash-me.md|jit480-crash-me.md) kill -BUS \$\$ ;; +esac +exec "$REAL_AWK" "\$@" +SHIMEOF +chmod +x "$SHIM_DIR/awk" + +# --- Fixture ------------------------------------------------------------------------ +ROOT="$(mktemp -d 2> /dev/null || mktemp -d -t jit480root)" +trap 'chmod -R u+rwX "$ROOT" "$SHIM_DIR" 2>/dev/null; rm -rf "$ROOT" "$SHIM_DIR"' EXIT +BASE="$ROOT/.claude/jit-context" +VOCAB_DIR="$BASE/vocabulary/00-manual" +mkdir -p "$BASE/tools/00-manual" "$BASE/paths/00-manual" "$VOCAB_DIR" + +write_vocab() { + # $1 filename under VOCAB_DIR, $2 keywords: value + cat > "$VOCAB_DIR/$1" << EOF2 +--- +title: $1 +description: fixture entry for #480 +keywords: $2 +--- + +Body for $1. +EOF2 +} + +write_vocab "keep-a-480.md" "alpha keyword" +write_vocab "jit480-crash-me.md" "bravo keyword" +write_vocab "keep-b-480.md" "charlie keyword" + +ERR="$ROOT/clean.err" +CLAUDE_PROJECT_DIR="$ROOT" bash "$REBUILD" > /dev/null 2> "$ERR" +CLEAN_RC=$? +assert_rc "a clean rebuild (no shim) exits 0" "0" "$CLEAN_RC" + +VOCAB_TSV="$VOCAB_DIR/00-index.tsv" +if [ ! -s "$VOCAB_TSV" ]; then + bad "the clean rebuild produced a non-empty vocabulary index" "found: $(wc -l < "$VOCAB_TSV" 2>/dev/null || echo missing) lines" +else + ok "the clean rebuild produced a non-empty vocabulary index" +fi + +COMMITTED_BEFORE="$(cat "$VOCAB_TSV" 2> /dev/null)" +COMMITTED_SHA_BEFORE="$(cksum "$VOCAB_TSV" 2> /dev/null)" + +# --- The crash run -------------------------------------------------------------------- +ERR2="$ROOT/crash.err" +PATH="$SHIM_DIR:$PATH" CLAUDE_PROJECT_DIR="$ROOT" bash "$REBUILD" > /dev/null 2> "$ERR2" +CRASH_RC=$? +assert_rc "a rebuild with a crashed helper exits 2, not 0" "2" "$CRASH_RC" + +ERR2_TXT="$(cat "$ERR2" 2> /dev/null)" +assert_contains "stderr names the crashed entry's keyword line as the reason" "$ERR2_TXT" "jit480-crash-me.md" +assert_contains "stderr says the partial rebuild was discarded (#480)" "$ERR2_TXT" "discarded" + +# --- The assertion #480 is about ------------------------------------------------------ +# A FILE comparison, never a captured bash variable: the whole point here is the byte +# count on disk, and a `$( )` capture would strip the very truncation this is checking +# for away before it could be seen (tests.md's own rule, paths/00-manual/tests.md). +if [ ! -e "$VOCAB_TSV" ]; then + bad "the committed vocabulary index still exists after the crashed run" "it is GONE" +else + ok "the committed vocabulary index still exists after the crashed run" +fi + +COMMITTED_SHA_AFTER="$(cksum "$VOCAB_TSV" 2> /dev/null)" +if [ "$COMMITTED_SHA_BEFORE" = "$COMMITTED_SHA_AFTER" ] && [ -n "$COMMITTED_SHA_BEFORE" ]; then + ok "the committed vocabulary index is byte-identical before and after the crashed run" +else + bad "the committed vocabulary index is byte-identical before and after the crashed run" \ + "before: $COMMITTED_SHA_BEFORE / after: $COMMITTED_SHA_AFTER" +fi + +AFTER_LINES=$(wc -l < "$VOCAB_TSV" 2> /dev/null | tr -d ' ') +BEFORE_LINES=$(printf '%s\n' "$COMMITTED_BEFORE" | grep -c '.') +if [ "$AFTER_LINES" -ge "$BEFORE_LINES" ]; then + ok "the index has not shrunk after the crashed run ($BEFORE_LINES -> $AFTER_LINES rows)" +else + bad "the index has not shrunk after the crashed run" "went from $BEFORE_LINES to $AFTER_LINES rows" +fi + +# No stray temp file left behind in the layer directory either -- commit_index()'s own +# failure path is responsible for cleaning up the scratch file it never got to swap in. +LEFTOVER=$(find "$VOCAB_DIR" -maxdepth 1 -name '.00-index.tsv.*' 2> /dev/null) +if [ -z "$LEFTOVER" ]; then + ok "no stray temp index file was left behind in the layer directory" +else + bad "no stray temp index file was left behind in the layer directory" "found: $LEFTOVER" +fi + +# --- Other layers, unaffected (per-index atomicity, not whole-run) -------------------- +# tools/ and paths/ have no entries at all in this fixture, so they are empty directories +# with no 00-index.tsv to even build -- real coverage of "a sibling layer with its OWN +# content still gets rebuilt even when the vocabulary layer's build was discarded" is +# left for a future pass; this suite's own assertion is the committed-index preservation, +# not cross-layer independence. + +echo "" +echo "PASS=$PASS FAIL=$FAIL" +[ "$FAIL" -eq 0 ] diff --git a/tests/test-rebuild-exit-codes.sh b/tests/test-rebuild-exit-codes.sh index c9ee5feb..d14dc418 100644 --- a/tests/test-rebuild-exit-codes.sh +++ b/tests/test-rebuild-exit-codes.sh @@ -239,24 +239,45 @@ echo "" echo "=== G. an index it cannot write is 2, not a count read back off the stale file ===" # Truncating the index failing left the previous one in place while the run reported the # rule count it read back OUT of that stale file -- success, with a number, for an index -# nobody rebuilt. +# nobody rebuilt. Since #480 the index is built ASIDE and swapped in with `mv`, which is a +# rename() -- governed by write permission on the DIRECTORY, never on the file it +# replaces. So the two probes below are deliberately different now: a FILE that cannot be +# written through no longer blocks the swap (#480's own intended behaviour change, not a +# regression -- the same reason `git` and most atomic-write tools can replace a read-only +# file as long as its directory is writable), and the genuine "cannot write" case moved to +# the directory. TSV="$BASE/tools/00-manual/00-index.tsv" +TSV_DIR="$(dirname "$TSV")" chmod 444 "$TSV" 2> /dev/null # The probe is an actual open-for-write, not `[ -w ]`. Root passes `-w` on a mode nobody # can write, and Git Bash answers it from an emulated attribute -- both of which decide # this section by a proxy for the capability rather than the capability. An append of zero -# bytes takes the same permission check as the truncation and changes nothing. +# bytes takes the same permission check an in-place truncation used to take. if printf %s "" >> "$TSV" 2> /dev/null; then echo " SKIP-NOTE: chmod did not remove write permission here (running as root, or a" - echo " filesystem without POSIX modes). Section G tested nothing." + echo " filesystem without POSIX modes). Section G's first probe tested nothing." else rebuild - assert_rc "an unwritable index exits 2" 2 "$RC" - # The phrase, not the path: bash prints its own "Permission denied" naming the file - # whatever this script does, so a bare name grep passed before the fix existed. - assert_contains "and says that index is now stale" "$(cat "$ERR")" "was NOT rebuilt" + assert_rc "a file that cannot itself be written through still rebuilds (#480: mv replaces it)" 0 "$RC" fi chmod 644 "$TSV" 2> /dev/null + +chmod 555 "$TSV_DIR" 2> /dev/null +# Same probe shape, now against the DIRECTORY: can a new entry be created in it at all -- +# the permission mktemp actually needs, and the one `[ -w ]` on the directory itself would +# answer the same way root or Git Bash already misreport for a file. +if : 2> /dev/null > "$TSV_DIR/.jit480-probe"; then + rm -f "$TSV_DIR/.jit480-probe" 2> /dev/null + echo " SKIP-NOTE: chmod did not remove write permission on the directory here (running as" + echo " root, or a filesystem without POSIX modes). Section G's second probe tested nothing." +else + rebuild + assert_rc "a directory that cannot be written into exits 2" 2 "$RC" + assert_contains "and says a temp file could not be created" "$(cat "$ERR")" "could not create a temp file" + assert_contains "and says that index is now stale" "$(cat "$ERR")" "was NOT rebuilt" +fi +chmod 755 "$TSV_DIR" 2> /dev/null + rebuild assert_rc "and once it is writable again, 0" 0 "$RC" From f20a105dd9872a31b930d34331969a9d33982706 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 19:09:16 +0200 Subject: [PATCH 2/3] fix: never log a crashed layer as committed, and clean up temp files on a killed process (#480) Self-review (Explore + oss:auditor, spawned against the first commit) both independently flagged the same regression: COUNT was read from the temp build before commit_index() ever decided whether to keep it, and the per-builder success line was logged unconditionally regardless of commit_index()'s verdict -- so a crashed, discarded build still printed a success-shaped "label: N rules" line to pipeline.log and stdout, directly below the FATAL saying it was discarded. Now logged conditionally on commit_index()'s own return value. Explore additionally reproduced a second gap: nothing cleaned up a temp index file if the whole rebuild-tsv.sh PROCESS (not merely a forked awk helper) is killed mid-build -- SIGTERM, an agent session cut off, an OOM kill -- between truncate_index() creating it and commit_index() ever running. A real SIGTERM 1s into an 800-entry fixture reproduced a leftover `.00-index.tsv.XXXXXX` that nothing on the next run cleans up either. An EXIT trap over a JIT_TEMP_FILES array now removes every temp file this run ever created, swapped in or not. Co-Authored-By: Claude Sonnet 5 --- scripts/rebuild-tsv.sh | 64 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/scripts/rebuild-tsv.sh b/scripts/rebuild-tsv.sh index 27c33fbb..5dde4083 100755 --- a/scripts/rebuild-tsv.sh +++ b/scripts/rebuild-tsv.sh @@ -205,6 +205,22 @@ jit_rc() { return 0 } +# Every temp index truncate_index() creates, so a killed PROCESS -- not merely a killed +# forked awk helper -- still leaves no debris (review finding, #480). A forked awk dying +# is already handled: the shell that is running this script survives it, the loop notices +# (jit_rc 2 / a short verdict count) and commit_index() cleans up its own temp file. But +# nothing removed a temp file if THIS SCRIPT ITSELF is killed (SIGTERM, SIGINT, an agent +# session cut off mid-rebuild, an OOM kill) between truncate_index() creating it and +# commit_index() ever running -- reproduced: a SIGTERM two seconds into a 1500-entry +# fixture left a real `.00-index.tsv.XXXXXX` behind in a committed layer directory, which +# nothing on the next run cleans up either, since mktemp only ever picks a fresh name. +# Appended right after each successful truncate_index() call (never inside it -- that +# runs in a subshell, the same reason its own jit_rc() calls need raising explicitly at +# the call site), so the trap below sees every temp file this run ever created, swapped +# in or not: `rm -f` on an already-renamed-away path is a silent no-op. +JIT_TEMP_FILES=() +trap 'rm -f "${JIT_TEMP_FILES[@]}" 2> /dev/null' EXIT + # --- What a report may say about a name that arrived with the clone (#113, #131) ---- # Every name printed by the reports below is a directory entry under # `.claude/jit-context/`, and that tree arrives with the repository. The policy for what @@ -528,6 +544,7 @@ build_tool_tsv() { jit_rc 2 return } + JIT_TEMP_FILES+=("$tsv_tmp") rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do @@ -603,8 +620,17 @@ build_tool_tsv() { done COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') - commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + # Logged conditionally on commit_index()'s own verdict (review finding, #480): COUNT is + # read from the TEMP build, before it is known whether that build gets swapped in at + # all, so logging it unconditionally would print a success-shaped "$label: N rules" + # line to pipeline.log and stdout for a build commit_index just discarded -- the exact + # "detected the crash but reported success anyway" confusion this whole fix exists to + # remove, one line further down. + if commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before"; then + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + else + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: build discarded, $COUNT row(s) NOT committed (see FATAL above)" + fi } TOOLS_BASE="$JIT_BASE/tools" @@ -872,6 +898,7 @@ build_vocab_tsv() { jit_rc 2 return } + JIT_TEMP_FILES+=("$tsv_tmp") rc2_before=$JIT_RC2_COUNT # --- Deferred generic-word classification (#255) ----------------------------------- @@ -1224,8 +1251,13 @@ build_vocab_tsv() { done COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') - commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT keywords" + # See build_tool_tsv()'s identical comment: logged conditionally on commit_index()'s own + # verdict, not unconditionally, so a discarded build never prints a success-shaped line. + if commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before"; then + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT keywords" + else + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: build discarded, $COUNT row(s) NOT committed (see FATAL above)" + fi } VOCAB_BASE="$JIT_BASE/vocabulary" @@ -1254,7 +1286,7 @@ build_vocab_path_tsv() { local dir="$1" local tsv="$2" local label="$3" - local T0 + local T0 COMMITTED T0=$(_ms) [ -d "$dir" ] || return @@ -1266,6 +1298,7 @@ build_vocab_path_tsv() { jit_rc 2 return } + JIT_TEMP_FILES+=("$tsv_tmp") rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do @@ -1315,13 +1348,20 @@ build_vocab_path_tsv() { done COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') - commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" + # See build_tool_tsv()'s identical comment: logged conditionally on commit_index()'s own + # verdict, not unconditionally, so a discarded build never prints a success-shaped line. + COMMITTED=0 + commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" && COMMITTED=1 # The LEAF, unlike every other builder's log line, because this is the only dimension # that writes two indexes out of one layer directory and the layer name alone would name # both. `$label/${tsv##*/}` is the same string truncate_index is handed above, so the # FATAL line and the success line for this index agree on what it is called -- and it is # a path that exists, which `vocabulary//paths` never was (#153). - _log "rebuild-tsv" $(($(_ms) - T0)) "$label/${tsv##*/}: $COUNT path mappings" + if [ "$COMMITTED" -eq 1 ]; then + _log "rebuild-tsv" $(($(_ms) - T0)) "$label/${tsv##*/}: $COUNT path mappings" + else + _log "rebuild-tsv" $(($(_ms) - T0)) "$label/${tsv##*/}: build discarded, $COUNT row(s) NOT committed (see FATAL above)" + fi } for dir in "$VOCAB_BASE"/*/; do @@ -1353,6 +1393,7 @@ build_path_tsv() { jit_rc 2 return } + JIT_TEMP_FILES+=("$tsv_tmp") rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do @@ -1378,8 +1419,13 @@ build_path_tsv() { done COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') - commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before" - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + # See build_tool_tsv()'s identical comment: logged conditionally on commit_index()'s own + # verdict, not unconditionally, so a discarded build never prints a success-shaped line. + if commit_index "$tsv_tmp" "$tsv" "$label/${tsv##*/}" "$rc2_before"; then + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + else + _log "rebuild-tsv" $(($(_ms) - T0)) "$label: build discarded, $COUNT row(s) NOT committed (see FATAL above)" + fi } PATHS_BASE="$JIT_BASE/paths" From 59c52639a971b257171f40e64e2ed7398caeb647 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 20:05:53 +0200 Subject: [PATCH 3/3] fix: declare test-rebuild-atomic-swap-480.sh's drivable helper (#480) CI red on PR #487 (6 legs, all the same root cause): test-assertion-helpers.sh requires every suite to carry at least one `# jit-drive:` declaration line, and the new test-rebuild-atomic-swap-480.sh had none -- "declares nothing". Declared assert_contains (its (desc, OUTPUT, needle) signature is the documented "capture" source, "contains" semantic); assert_rc stays undeclared since it compares exit codes rather than payload content, which is reported in the harness's own third-outcome list rather than failing. Verified locally: bash tests/test-assertion-helpers.sh now exits 0, 445 passed / 0 failed (previously failing on exactly this suite). Co-Authored-By: Claude Sonnet 5 --- tests/test-rebuild-atomic-swap-480.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test-rebuild-atomic-swap-480.sh b/tests/test-rebuild-atomic-swap-480.sh index b5252cbd..e3c56404 100755 --- a/tests/test-rebuild-atomic-swap-480.sh +++ b/tests/test-rebuild-atomic-swap-480.sh @@ -21,6 +21,8 @@ # than a false pass. # # Usage: bash tests/test-rebuild-atomic-swap-480.sh +# +# jit-drive: assert_contains contains capture set -uo pipefail @@ -129,7 +131,7 @@ assert_rc "a clean rebuild (no shim) exits 0" "0" "$CLEAN_RC" VOCAB_TSV="$VOCAB_DIR/00-index.tsv" if [ ! -s "$VOCAB_TSV" ]; then - bad "the clean rebuild produced a non-empty vocabulary index" "found: $(wc -l < "$VOCAB_TSV" 2>/dev/null || echo missing) lines" + bad "the clean rebuild produced a non-empty vocabulary index" "found: $(wc -l < "$VOCAB_TSV" 2> /dev/null || echo missing) lines" else ok "the clean rebuild produced a non-empty vocabulary index" fi