diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eec3ca..384c99f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,26 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- **`10#` reached `save-session.sh` and `post-tool-hook.sh` but never the two git hooks** ([#327](https://github.com/Digital-Process-Tools/claude-remember/issues/327)) — [#322](https://github.com/Digital-Process-Tools/claude-remember/issues/322)/[#329](https://github.com/Digital-Process-Tools/claude-remember/issues/329) closed four readers and left `hooks.d/after_save/50-git-backup.sh` and `hooks.d/before_session_start/50-git-restore.sh` on the bare digits-only guard, so `08`/`09` — all digits, therefore clean to that guard — still reached `$(( ))` as octal. + + **Six sites, not the one the issue names, and four were found by sweeping the two files.** Besides the backup cooldown marker and the restore hook's two fetch-timestamp reads, every consecutive-failure counter in both hooks (`git-backup-rejected`, `git-backup-commit-failed`, `git-backup-no-remote`, `git-restore-diverged`) reads itself back through the same guard into the same sink. Those four are the worse ones: bash abandons the rest of the branch on the failing increment, and the abandoned branch is the one holding `log "ERROR: push REJECTED …"`, `log "ERROR: commit FAILED …"` and `log "ERROR: … DIVERGED …"`. A corrupt counter did not mis-count — it silenced the loudest report in the file, which is [#257](https://github.com/Digital-Process-Tools/claude-remember/issues/257) reached through the counter instead of through `exit 0`. `_fetch_health` fails the same way pointing the other direction: the octal read falls through to the `rc` branch with `rc` unset, so a fetch that never came back is reported as one that FAILED with an unknown status — the wrong state out of the three, handing the user a remedy for a failure that did not happen. + + **The issue's severity claim does not survive measurement; the fix is right anyway.** #327 states the hook dies under `set -u` and the git backup is stopped permanently, and classes it `fails-to-preserve`. Measured on bash 3.2.57 in the shape the hook actually has — the read sits inside `if [ -f "$COOLDOWN_MARKER" ]` — the failing expansion abandons that body and resumes after `fi`: rc 0, the commit happens, and `_gb_stamp_cooldown` rewrites the marker, so the gate self-heals exactly as #322's two do. The issue's repro used a flattened command list, where the same input does exit 127. This is the mechanism #329's own docstring already records; `misreports` is the right class for all six sites. + + **Comparisons were checked and are not affected.** `[ "$x" -lt "$y" ]` parses base 10, so `[ 08 -lt 9 ]` is true with no diagnostic. `$(( ))` is the only sink, which is what bounds the sweep. + + The docstring half of #327 needs no work: [#329](https://github.com/Digital-Process-Tools/claude-remember/issues/329) already replaced the false claim in `tests/test_save_session_marker_arithmetic_322.py` with an accurate statement that the gap was still open here. That statement is now itself out of date and is updated in the same commit — a correct note about an open gap becomes a wrong one the moment the gap closes. + +- **#258's regression test wrote its marker where a compatibility shim happened to carry it** ([#324](https://github.com/Digital-Process-Tools/claude-remember/issues/324)) — reported by [@jmossie82](https://github.com/jmossie82). `test_a_corrupt_cooldown_marker_does_not_kill_the_hook` wrote `/.last-git-backup-ts` while the hook reads `$GB_STATE_DIR/last-git-backup-ts` ([#261](https://github.com/Digital-Process-Tools/claude-remember/issues/261)). + + **Filed as "the cooldown block is never entered and every corrupt value passes trivially"; measured, that part is not true.** The legacy carry-forward at the top of the hook copies the dotted root file into the state dir *before* the cooldown block, so every parametrized value did reach the evaluator — which is also why the reporter's glob found the dotted path gone rather than untouched. The defect is real but different: the test's transport was the compatibility shim, not the feature under test. That shim exists to be removed, and the day it goes, all four cases go vacuous with nothing turning red. The marker now goes through the same `hook_state` helper the sibling tests already use, resolved through `git rev-parse --git-common-dir` rather than spelled out. + + **What was genuinely unguarded is the octal pair, and the assertions.** `08`/`09` were absent from the parametrization, and the test asserted only rc and the commit — neither of which changes on bash 3.2.57, where the failing arithmetic abandons the `if` body and the hook proceeds. It now asserts on stderr, which is the only observable that separates broken from fixed on every bash: falling back to `0` makes a corrupt marker read as "very old", so corrupt and clean-but-ancient reach the same decision by design. + ## [0.17.0] — Markers that were trusted to be numbers Two of this repo's own state files are read straight into contexts that treat their contents as diff --git a/hooks.d/after_save/50-git-backup.sh b/hooks.d/after_save/50-git-backup.sh index e44d073..fb6be9e 100755 --- a/hooks.d/after_save/50-git-backup.sh +++ b/hooks.d/after_save/50-git-backup.sh @@ -166,10 +166,17 @@ if [ -f "$COOLDOWN_MARKER" ]; then # permanently, with dispatch's nameless `hook failed` as the only trace. # Falling back to 0 self-heals, because the run it allows is the run that # rewrites the marker. + # + # `case` AND `10#`, not one or the other (#327). "08"/"09" are all digits, + # so they clear the guard above and are then read as OCTAL -- the same + # abandonment, from a marker that looks clean. `10#` goes AFTER the case and + # never instead of it: `10#` on an empty string is itself an error on bash 5, + # and the case is also what rejects a space-padded value that arithmetic + # would have accepted. Same call save-session.sh already makes (#322). case "$LAST_MOD" in ''|*[!0-9]*) LAST_MOD=0 ;; esac - ELAPSED=$(( $(date +%s) - LAST_MOD )) + ELAPSED=$(( $(date +%s) - 10#$LAST_MOD )) if [ "$ELAPSED" -lt "$BACKUP_COOLDOWN" ]; then debug_enabled 0 && log "git-backup" "cooldown ${ELAPSED}s < ${BACKUP_COOLDOWN}s, skip" exit 0 @@ -388,11 +395,17 @@ esac return 0 fi + # 10# after the case (#327). Every counter in this file is read back + # from a file the hook wrote, so "08" is only reachable through + # corruption -- which is what the guard is for. Without it the increment + # is an octal error, bash abandons the rest of this branch, and the + # `log "ERROR: push REJECTED …"` below never runs: the loudest report in + # the file, silenced by the counter that exists to escalate it. _count=$(cat "$REJECT_STATE_FILE" 2>/dev/null || echo 0) case "$_count" in ''|*[!0-9]*) _count=0 ;; esac - _count=$((_count + 1)) + _count=$((10#$_count + 1)) echo "$_count" > "$REJECT_STATE_FILE" 2>/dev/null || true log "git-backup" "ERROR: push REJECTED by the remote — the backup has STOPPED for $SLUG and will not resume on its own (consecutive rejections: $_count). git rejected: ${_rejected%;}. The commit exists on this machine only. Nothing here will fetch, merge or rebase for you: run 'git -C \"$REPO_ROOT\" push' to see git's own advice and resolve it by hand — recent.md and archive.md are rewritten wholesale by consolidation, so a wrong automatic resolution would corrupt memory silently." @@ -585,11 +598,14 @@ esac else _gb_stamp_cooldown COMMIT_ERR=$(printf '%s' "$COMMIT_ERR" | tr '\n' ' ') + # 10# after the case (#327) — same shape as the reject counter above, + # and here the abandoned branch is the one reporting that this memory is + # in no git history at all. _cfail=$(cat "$COMMIT_FAIL_STATE_FILE" 2>/dev/null || echo 0) case "$_cfail" in ''|*[!0-9]*) _cfail=0 ;; esac - _cfail=$((_cfail + 1)) + _cfail=$((10#$_cfail + 1)) echo "$_cfail" > "$COMMIT_FAIL_STATE_FILE" 2>/dev/null || true log "git-backup" "ERROR: commit FAILED for $SLUG — this memory is recorded in NO git history at all, not locally and not on any remote, and the backup has STOPPED for this project (consecutive failures: $_cfail). git said: ${COMMIT_ERR:-}. Run 'git -C \"$REPO_ROOT\" commit -- \"$SLUG/\"' to see it yourself." @@ -620,11 +636,12 @@ esac # distinguished "deliberately local-only" from "the setup was never # finished". It still cannot be distinguished from inside this hook — # so it is asked ONCE, of the only person who knows. + # 10# after the case (#327), as above. _nr=$(cat "$NO_REMOTE_STATE_FILE" 2>/dev/null || echo 0) case "$_nr" in ''|*[!0-9]*) _nr=0 ;; esac - _nr=$((_nr + 1)) + _nr=$((10#$_nr + 1)) echo "$_nr" > "$NO_REMOTE_STATE_FILE" 2>/dev/null || true log "git-backup" "no remote configured for '$REMOTE_NAME' in $REPO_ROOT — the commit exists on this machine ONLY and nothing is backed up off it (consecutive saves in this state: $_nr). Add one: git -C \"$REPO_ROOT\" remote add origin " diff --git a/hooks.d/before_session_start/50-git-restore.sh b/hooks.d/before_session_start/50-git-restore.sh index 2434e46..fceff32 100755 --- a/hooks.d/before_session_start/50-git-restore.sh +++ b/hooks.d/before_session_start/50-git-restore.sh @@ -219,10 +219,14 @@ _spawn_fetch() { while IFS='=' read -r _k _v; do case "$_k" in started) _s="$_v" ;; finished) _f="$_v" ;; esac done < "$FETCH_STATE_FILE" + # 10# after the case, never instead of it (#327): "08" is all digits, + # clears the guard, and is then read as octal -- so the age comparison + # is abandoned and a fetch still inside its window gets a second one + # stacked on top of it. case "$_s" in ''|*[!0-9]*) _s=0 ;; esac if [ -z "$_f" ] && [ "$_s" -gt 0 ]; then _now=$(date +%s) - _age=$(( _now - _s )) + _age=$(( _now - 10#$_s )) [ "$_age" -lt "$FETCH_TIMEOUT" ] && return 0 fi fi @@ -298,10 +302,16 @@ _fetch_health() { while IFS='=' read -r _k _v; do case "$_k" in started) _s="$_v" ;; finished) _f="$_v" ;; rc) _rc="$_v" ;; esac done < "$FETCH_STATE_FILE" + # 10# after the case, never instead of it (#327). Without it "08" is octal, + # the arithmetic fails, bash abandons this whole `if` body, and control + # falls through to the `rc` test below with `_rc` empty -- so a fetch that + # never came back is reported as one that FAILED with an unknown status. + # Wrong state out of the three, and the remedy offered is for a failure + # that did not happen. case "$_s" in ''|*[!0-9]*) _s=0 ;; esac if [ -z "$_f" ]; then _now=$(date +%s) - _age=$(( _now - _s )) + _age=$(( _now - 10#$_s )) if [ "$_age" -lt "$FETCH_TIMEOUT" ]; then echo "in-flight"; else echo "abandoned"; fi return fi @@ -391,9 +401,13 @@ fi if [ "$AHEAD" -gt 0 ] && [ "$BEHIND" -gt 0 ]; then # ── DIVERGED: refuse, and say so ───────────────────────────────────────── + # 10# after the case (#327). Without it a corrupt counter makes the + # increment an octal error, this branch is abandoned, and the DIVERGED + # report below never runs -- a store that refused to restore, saying so + # nowhere. _count=$(cat "$DIVERGED_STATE_FILE" 2>/dev/null || echo 0) case "$_count" in ''|*[!0-9]*) _count=0 ;; esac - _count=$((_count + 1)) + _count=$((10#$_count + 1)) echo "$_count" > "$DIVERGED_STATE_FILE" 2>/dev/null || true log "git-restore" "ERROR: the memory store has DIVERGED — $AHEAD local commit(s) the remote does not have, $BEHIND remote commit(s) this machine does not have (consecutive session starts in this state: $_count). NOT restored, and nothing here will merge or rebase for you: recent.md and archive.md are rewritten wholesale by consolidation, so a wrong automatic resolution would corrupt memory silently. Resolve it by hand: git -C \"$REPO_ROOT\" log --oneline --left-right \"HEAD...$REMOTE_REF\"" diff --git a/tests/test_git_backup_silent_stops_257.py b/tests/test_git_backup_silent_stops_257.py index 074ca09..b68b69a 100644 --- a/tests/test_git_backup_silent_stops_257.py +++ b/tests/test_git_backup_silent_stops_257.py @@ -42,6 +42,7 @@ from .test_git_backup_hook import ( # noqa: E402 REPO_ROOT, _git, + hook_state, make_external_remember_repo, ) from .test_git_backup_push_rejected_253 import ( # noqa: E402 @@ -411,21 +412,54 @@ def test_a_store_with_no_remote_at_all_eventually_says_so_exactly_once(tmp_path) # ═════════════════════════════════════════════════════════════════════════════ -@pytest.mark.parametrize("corrupt", ["garbage-text", "12x34", "17\x00", " 1785512249 "]) +# Everything bash says when unvalidated content reaches an arithmetic +# evaluator, plus the `set -u` follow-on. Asserted on because rc and the commit +# do NOT separate broken from fixed here: falling back to 0 makes a corrupt +# marker read as "very old", so corrupt and clean-but-ancient produce the same +# decision, and on bash 3.2.57 a failing `$(( ))` inside the `if [ -f … ]` body +# abandons that body and resumes after `fi` — rc stays 0 and the commit still +# happens. The diagnostic is the only observable that changes. +ARITHMETIC_DIAGNOSTICS = ( + "syntax error", + "invalid arithmetic operator", + "value too great for base", + "unbound variable", +) + + +@pytest.mark.parametrize( + "corrupt", + ["garbage-text", "12x34", "17\x00", " 1785512249 ", "08", "09"], +) def test_a_corrupt_cooldown_marker_does_not_kill_the_hook(tmp_path, corrupt): - """#258 item 1, and its direction is wrong in the issue. + """#258 item 1, re-derived twice — the issue had the direction wrong, and + this test then had the path wrong (#324). `LAST_MOD` is unvalidated file content expanded inside `$(( ))` under - `set -u`. A non-numeric marker is not "the cooldown is skipped": bash - evaluates it as an *arithmetic expression*, hits an unbound variable or an - invalid base, and the hook DIES at that line — above the lock, above the - add, above the commit. The marker is only rewritten after a successful - commit, which is now unreachable, so one bad byte stops the backup - permanently and the only trace was dispatch's nameless `hook failed` — - nameless until #277 gave it the exit status and bash's own diagnostic. + `set -u`, so bash evaluates the marker's VALUE as an *arithmetic + expression*. What follows is version- and shape-dependent, which is why + nothing below rests on rc alone: measured on bash 3.2.57, the failing + expansion abandons the enclosing `if [ -f "$COOLDOWN_MARKER" ]` body and + resumes after `fi`, so the hook exits 0, commits, and re-stamps the marker + — one skipped cooldown, self-healing. Flattened out of that `if`, the same + input exits 127 on `$ELAPSED` being unbound. Both are real; only the + stderr separates either from a clean run. + + **Path (#324).** This wrote to `/.last-git-backup-ts` while the hook + reads `$GB_STATE_DIR/last-git-backup-ts` (#261). That was NOT vacuous — the + legacy carry-forward at the top of the hook copies the dotted root file into + the state dir before the cooldown block, so every value here did reach the + evaluator. But the transport was the compatibility shim, not the feature: + the day that shim is dropped, all of these go vacuous with nothing turning + red. `hook_state` puts the marker where the hook actually looks, resolved + through git rather than spelled out. + + `08`/`09` (#327) are the pair a digits-only `case` cannot catch: all digits, + so they clear the guard, and are then read as octal. """ home, remember, remote, slug_dir, project = _store(tmp_path) - (remember / ".last-git-backup-ts").write_text(corrupt, encoding="utf-8", errors="ignore") + marker = hook_state(remember, ".last-git-backup-ts", create_dir=True) + marker.write_text(corrupt, encoding="utf-8", errors="ignore") proc = _run_backup(slug_dir, project, home, _backup_config(tmp_path)) @@ -433,6 +467,13 @@ def test_a_corrupt_cooldown_marker_does_not_kill_the_hook(tmp_path, corrupt): f"the hook died on a corrupt cooldown marker (rc={proc.returncode}): " f"{proc.stderr.strip()}" ) + for phrase in ARITHMETIC_DIAGNOSTICS: + assert phrase not in proc.stderr, ( + f"the marker reached the arithmetic evaluator — bash said {phrase!r}. " + "The guard did not hold, and what the hook does next is decided by " + "the bash version rather than by the code: " + f"{proc.stderr.strip()}" + ) assert _wait_until(lambda: _slug_is_committed(remember)), ( "a corrupt cooldown marker stopped the backup entirely — and because " "the marker is rewritten only on a successful commit, it stays corrupt " @@ -440,6 +481,39 @@ def test_a_corrupt_cooldown_marker_does_not_kill_the_hook(tmp_path, corrupt): ) +def test_a_corrupt_failure_counter_does_not_silence_the_commit_failed_report(tmp_path): + """The same octal hole one file over, and here it eats the report itself. + + `git-backup-commit-failed` is a counter this hook writes and reads back to + decide when to escalate. It carries the same digits-only guard as the + cooldown marker and the same `$(( ))` sink, so `08` clears the guard, the + increment fails, and bash abandons the rest of that `else` branch — which is + where `log "ERROR: commit FAILED …"` lives. A store whose commits are + failing then reports nothing at all, which is #257 exactly, reached through + the counter instead of through `exit 0`. + + Found by sweeping the file rather than from either issue; #324 and #327 name + the cooldown marker only. + """ + home, remember, remote, slug_dir, project = _store(tmp_path) + _break_commits(remember) + hook_state(remember, ".git-backup-commit-failed", create_dir=True).write_text( + "08", encoding="utf-8") + + proc = _run_backup(slug_dir, project, home, _backup_config(tmp_path)) + + for phrase in ARITHMETIC_DIAGNOSTICS: + assert phrase not in proc.stderr, ( + f"the failure counter reached the arithmetic evaluator ({phrase!r}): " + f"{proc.stderr.strip()}" + ) + assert _wait_until(lambda: "commit FAILED" in _log_or_empty(slug_dir)), ( + "the commit failed and the hook said nothing — a corrupt failure " + "counter suppressed the one report that tells a user their memory is " + "in no git history at all\n--- log ---\n" + _log_or_empty(slug_dir) + ) + + def test_the_cooldown_marker_is_written_even_when_the_commit_fails(tmp_path): """#258 item 2. The two defects compound: a store whose commits fail never engages the throttle, so it does the full add/rm/commit on every single diff --git a/tests/test_git_restore_hook_253.py b/tests/test_git_restore_hook_253.py index 8586cec..ae0fd54 100644 --- a/tests/test_git_restore_hook_253.py +++ b/tests/test_git_restore_hook_253.py @@ -563,6 +563,41 @@ def test_an_abandoned_fetch_is_reported(self, tmp_path): "from one that returned nothing new\n--- log ---\n" + log ) + def test_a_leading_zero_start_time_is_read_as_decimal(self, tmp_path): + """#327, the restore half. `started=08` is all digits, so it clears the + digits-only guard in `_fetch_health` and is then read as OCTAL. + + The arithmetic fails, bash abandons the whole `if [ -z "$_f" ]` body, + and control falls through to the `rc` branch with `_rc` empty — so a + fetch that never came back is reported as one that FAILED with an + unknown status. The same three-state vocabulary, the wrong state, and + the remedy the user is handed ("run git fetch to see git's own error") + is for a failure that did not happen. + + `date +%s` never emits a leading zero, so this is reachable only through + a corrupt or truncated state file — which is that guard's whole premise. + """ + home, remember, remote, slug_dir, project = _store(tmp_path) + _fetch_now(remember) + (hook_state(remember, FETCH_STATE, create_dir=True)).write_text( + "started=08\n", encoding="utf-8" + ) + + result = _run(slug_dir, project, home, _config(tmp_path, enabled=True)) + + for phrase in ("syntax error", "invalid arithmetic operator", + "value too great for base", "unbound variable"): + assert phrase not in result.stderr, ( + "the state file reached the arithmetic evaluator — bash said " + f"{phrase!r}: {result.stderr.strip()}" + ) + log = _log_text(slug_dir) + assert "never completed" in log, ( + "a fetch with a leading-zero start time was not reported as " + "abandoned — the octal read fell through to the FAILED branch, so " + "the session is told a fetch failed when none did\n--- log ---\n" + log + ) + # ── The detached fetch ─────────────────────────────────────────────────────── diff --git a/tests/test_save_session_marker_arithmetic_322.py b/tests/test_save_session_marker_arithmetic_322.py index 9b98ecc..ce49c22 100644 --- a/tests/test_save_session_marker_arithmetic_322.py +++ b/tests/test_save_session_marker_arithmetic_322.py @@ -23,13 +23,15 @@ `"08"`/`"09"` are here because a digits-only guard is not enough on its own: they pass it and are then read as octal (`value too great for base`), failing -identically. That is why the guard carries `10#`. The same gap is still open in -#258's guard in `50-git-backup.sh` and in `50-git-restore.sh`'s fetch-state read -— both are `case`-guarded and neither carries `10#` — and is deliberately NOT -closed here: those two are not this issue's markers, the change is untestable -until `test_a_corrupt_cooldown_marker_does_not_kill_the_hook` stops writing to -`/.last-git-backup-ts` while the hook reads `/.git/remember/…`, and -an unpinned one-token edit is how a guard rots back out. +identically. That is why the guard carries `10#`. The gap this paragraph used to +report as still open in `50-git-backup.sh` and `50-git-restore.sh` was closed by +#324/#327, along with four more sinks in the same two files that nobody had +counted — the consecutive-failure counters, where the abandoned branch is the one +holding the ERROR log. It is pinned there rather than here, by +`tests/test_git_backup_silent_stops_257.py` and +`tests/test_git_restore_hook_253.py`; the ordering that paragraph called for was +the one taken, #324's path first and the one-token edit behind a test that can +fail. CONTROL: ` 1785512249 ` is in the list on purpose and must stay GREEN against the unfixed script. Arithmetic skips surrounding whitespace, so that marker works