Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<store>/.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
Expand Down
25 changes: 21 additions & 4 deletions hooks.d/after_save/50-git-backup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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."
Expand Down Expand Up @@ -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:-<no output>}. Run 'git -C \"$REPO_ROOT\" commit -- \"$SLUG/\"' to see it yourself."
Expand Down Expand Up @@ -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 <url>"
Expand Down
20 changes: 17 additions & 3 deletions hooks.d/before_session_start/50-git-restore.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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\""
Expand Down
94 changes: 84 additions & 10 deletions tests/test_git_backup_silent_stops_257.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -411,35 +412,108 @@ 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 `<store>/.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))

assert proc.returncode == 0, (
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 "
"and the store never backs up again"
)


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
Expand Down
Loading