diff --git a/.claude/jit-context/paths/00-manual/tooling.md b/.claude/jit-context/paths/00-manual/tooling.md index 0a41ec2..505c4bf 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 0000000..f0d81b6 --- /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 1a89d74..5dde408 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,11 +194,33 @@ 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 } +# 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 @@ -331,23 +356,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 +388,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 +533,19 @@ 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 + } + JIT_TEMP_FILES+=("$tsv_tmp") + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -525,11 +616,21 @@ 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 ' ') - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + # 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" @@ -789,7 +890,16 @@ 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 + } + JIT_TEMP_FILES+=("$tsv_tmp") + 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,11 +1247,17 @@ 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 ' ') - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT keywords" + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + # 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" @@ -1170,11 +1286,20 @@ build_vocab_path_tsv() { local dir="$1" local tsv="$2" local label="$3" - local T0 + local T0 COMMITTED 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 + } + JIT_TEMP_FILES+=("$tsv_tmp") + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -1214,7 +1339,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,13 +1347,21 @@ build_vocab_path_tsv() { fi done - COUNT=$(wc -l < "$tsv" | tr -d ' ') + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + # 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 @@ -1252,7 +1385,16 @@ 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 + } + JIT_TEMP_FILES+=("$tsv_tmp") + rc2_before=$JIT_RC2_COUNT for md in "$dir"/*.md; do [ -f "$md" ] || continue @@ -1273,11 +1415,17 @@ 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 ' ') - _log "rebuild-tsv" $(($(_ms) - T0)) "$label: $COUNT rules" + COUNT=$(wc -l < "$tsv_tmp" | tr -d ' ') + # 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" @@ -1750,8 +1898,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 0000000..e3c5640 --- /dev/null +++ b/tests/test-rebuild-atomic-swap-480.sh @@ -0,0 +1,196 @@ +#!/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 +# +# jit-drive: assert_contains contains capture + +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 c9ee5fe..d14dc41 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"