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
30 changes: 30 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,36 @@ 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

- **A cooldown marker AHEAD of now sticks the throttle ON, permanently and mutely** ([#326](https://github.com/Digital-Process-Tools/claude-remember/issues/326)) — `case "$X" in ''|*[!0-9]*) X=0 ;; esac` validates a marker's *syntax*. It does not bound its *range*. A digits-only value ahead of the current clock is accepted, makes `ELAPSED` negative, and a negative `ELAPSED` is `-lt` any cooldown — so the gate takes its `exit 0`. That exit sits **above** the line that rewrites the marker, so the self-heal the previous three releases rest on is unreachable on exactly the path that needs it. Four sites, four different outages: no session is ever saved again; `now.md` is never compressed again and grows unbounded; the git backup stops, which is [#258](https://github.com/Digital-Process-Tools/claude-remember/issues/258)'s original outage reached through a value the guard *accepts*; and the per-tool-call fork throttle refuses to fork the save that would have healed the marker, so the two throttles hold each other shut.

**This needs no corruption.** An NTP step backwards, a VM snapshot restore, a container clock jump, or a store on a share with a skewed clock all produce it. Not a timezone or DST change — epoch seconds do not move for those, which is why proceeding is the right call and costs at most one extra save rather than mis-throttling a laptop that suspended over a boundary.

**Three states, not two.** An out-of-range marker is not clamped in silence: the run **proceeds**, the marker is **reset where the reset is reachable** — at the point of rejection, not below an `exit` — and one line goes to `hook-errors.log` as well as the daily log, via a new `report_error` in `scripts/log.sh`. A silent clamp would trade a mute stuck throttle for a mute wrong value, which is the same defect one layer along. `/remember:doctor` reports "Recent errors" out of that file, so a clock that stepped backwards far enough to disable saving is now visible where a user already looks.

**`post-tool-hook.sh` is deliberately asymmetrical.** It declines the cooldown it cannot substantiate and does nothing else: no marker rewrite (the marker belongs to `save-session.sh`, and a second writer on a per-tool-call path is a race for no gain), no diagnostic (one per tool call for as long as the clock is behind), and no added subprocess spawn or file read ([#299](https://github.com/Digital-Process-Tools/claude-remember/issues/299)/[#330](https://github.com/Digital-Process-Tools/claude-remember/issues/330)).

**The source comment said the opposite, and that is why this was reopened.** The comment above the save gate in `scripts/save-session.sh` stated that both gates self-heal and that the cost is "one skipped cooldown per corruption event, not a throttle stuck off". True for the syntax case, false for this one. The CHANGELOG was corrected at 0.17.0; the comment was not, and PR [#328](https://github.com/Digital-Process-Tools/claude-remember/pull/328) closed the issue by *referencing* it without changing a line of its subject matter. Both comments are corrected here.

- **Every remaining case-guarded value in `$(( ))` now carries `10#`, and a check keeps it that way** ([#332](https://github.com/Digital-Process-Tools/claude-remember/issues/332)) — `08` and `09` are all digits, so they clear the digits-only guard and are then read as octal; bash abandons the rest of the enclosing command list, which in this repo has repeatedly been the throttle, the ERROR log or the backup. Four issues ([#321](https://github.com/Digital-Process-Tools/claude-remember/issues/321), [#325](https://github.com/Digital-Process-Tools/claude-remember/issues/325), [#329](https://github.com/Digital-Process-Tools/claude-remember/issues/329), [#331](https://github.com/Digital-Process-Tools/claude-remember/issues/331)) each fixed a correct subset by hand and each looked complete.

**Twelve sites remained, not the seven #332 lists.** The five it does not name — `scripts/doctor.sh:346`, `scripts/lib-lock.sh:412`, `:421`, `:430` and the second `run-consolidation.sh` read — were found by the check rather than by reading, which is the argument for having one.

`tests/test_arith_base_lint_332.py` sweeps `scripts/` and `hooks.d/` for the shape mechanically: a variable that passes through a digits-only `case` and later appears inside `$(( ))` with no `10#`. ShellCheck does not flag it ([koalaman/shellcheck#2679](https://github.com/koalaman/shellcheck/issues/2679), open since 2023), so nothing in CI caught it before. `[ "$x" -lt "$y" ]` parses base 10 without complaint, so a guarded value used only in `test` is not reported — `$(( ))` is the only sink, and that is what keeps the sweep finite. The check tests itself against a planted instance and a fixed one, so a green means "looked, found none" rather than "did not look".

**No shared helper.** A `_remember_epoch_or_zero` in `log.sh` would cover most sites and is worth doing on its own terms, but it does not prevent recurrence: the failure mode is a new call site written by someone who has read none of these five issues, and a helper they do not know about cannot help them. The check fails in the one place people already look.

- **A fifth marker-ahead-of-now site: the restore hook's fetch record** ([#326](https://github.com/Digital-Process-Tools/claude-remember/issues/326)) — `hooks.d/before_session_start/50-git-restore.sh` reads `started=` out of the fetch state file through the same digits-only `case`, and #326's four-site inventory does not name it. A `started` ahead of now makes `_age` negative, so `_fetch_health` answers **`in-flight`** — the one state that means "wait, something is already running" — about nothing at all. `_spawn_fetch` runs the same comparison and takes its early `return`, and the only writers of that file are inside the subshell that return skips, so no fetch is ever started again and the record can never heal. Identical geometry to the four named sites.

**The visible harm is worse than a wrong label.** The caller then prints `already up to date with origin/main` off remote-tracking refs that no fetch has refreshed — exactly what `_fetch_health`'s own header forbids ("Silence is not a fourth way of saying up to date"). `_fetch_health` now answers `abandoned`, which is the honest third state, and `_spawn_fetch` falls through to start a real fetch and rewrite the record, with one line saying why. `_fetch_health` is left pure: its stdout *is* the verdict and is read through `$( )`, so the diagnostic goes in `_spawn_fetch`.

Found by the review pass on this PR, not by the issue — which is the same lesson #332 is about: an inventory that names N sites is evidence about the reading, not about the code.

- **The new lint could not be trusted as a gate, and its own fixture was not the shape it claimed** ([#332](https://github.com/Digital-Process-Tools/claude-remember/issues/332)) — two defects in `tests/test_arith_base_lint_332.py`, both found reviewing this PR. `case "$1" in` captures the guarded name as the bare string `1`, and the matcher did not require the `$` sigil, so every `$(( x + 1 ))` in a file that guards a positional parameter would have been reported as a finding — `scripts/lib-lock.sh` guards two of them, so the next ordinary line of arithmetic there would have failed CI for no reason. A lint that cries wolf gets switched off, and then it is not a gate. Numeric names now require the sigil. Separately, `''` inside a single-quoted Python literal closes it and opens another, so the planted fixture had silently become `case "$LAST" in |*[!0-9]*)` — a self-test whose fixture is not the shape it plants still goes green, and a green here is supposed to mean "looked, found none". Both are pinned by tests.

## [0.18.0] — The rest of the readers

[#322](https://github.com/Digital-Process-Tools/claude-remember/issues/322)'s guard was written
Expand Down
15 changes: 14 additions & 1 deletion hooks.d/after_save/50-git-backup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,20 @@ if [ -f "$COOLDOWN_MARKER" ]; then
''|*[!0-9]*) LAST_MOD=0 ;;
esac
ELAPSED=$(( $(date +%s) - 10#$LAST_MOD ))
if [ "$ELAPSED" -lt "$BACKUP_COOLDOWN" ]; then
if [ "$ELAPSED" -lt 0 ]; then
# Range, not syntax (#326). A marker AHEAD of now is all digits, clears
# the case, and makes ELAPSED negative -- which reads as "inside the
# window" and takes the exit below. Every path that stamps the marker
# (_gb_stamp_cooldown, and both of its call sites) is beneath that exit,
# so this is #258's original outage reached through a value the guard
# ACCEPTS: the git history simply stops, and the only trace is the
# absence of commits.
#
# Proceed, reset here where the reset is reachable, and say so in both
# places a human looks.
report_error "git-backup" "WARNING: $COOLDOWN_MARKER is $(( 0 - ELAPSED ))s ahead of now — the clock moved back, or the marker is corrupt in a way a digits-only check cannot see. Resetting it and backing up; the cooldown resumes from now."
date +%s > "$COOLDOWN_MARKER" 2>/dev/null || true
elif [ "$ELAPSED" -lt "$BACKUP_COOLDOWN" ]; then
debug_enabled 0 && log "git-backup" "cooldown ${ELAPSED}s < ${BACKUP_COOLDOWN}s, skip"
exit 0
fi
Expand Down
33 changes: 31 additions & 2 deletions hooks.d/before_session_start/50-git-restore.sh
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,24 @@ _spawn_fetch() {
if [ -z "$_f" ] && [ "$_s" -gt 0 ]; then
_now=$(date +%s)
_age=$(( _now - 10#$_s ))
[ "$_age" -lt "$FETCH_TIMEOUT" ] && return 0
if [ "$_age" -lt 0 ]; then
# Range, not syntax -- #326's FIFTH site, which that issue's
# four-site inventory does not name. A `started` AHEAD of now is
# all digits, so it clears the guard above, and it makes _age
# negative -- which reads as "well inside the window" and takes
# the `return 0` below. The only writers of FETCH_STATE_FILE are
# inside the subshell that return skips, so nothing ever
# rewrites the record: no fetch is spawned again, ever, and
# _fetch_health goes on answering "in-flight" about a process
# that does not exist. Same geometry as the other four -- the
# self-heal sits below the early exit.
#
# Fall through and spawn. The spawn rewrites the record, which
# is the whole repair, and one line says why.
report_error "git-restore" "WARNING: $FETCH_STATE_FILE says a fetch started $(( 0 - _age ))s in the FUTURE — the clock moved back, or the record is corrupt in a way a digits-only check cannot see. No fetch is running; starting a real one and rewriting the record."
elif [ "$_age" -lt "$FETCH_TIMEOUT" ]; then
return 0
fi
fi
fi

Expand Down Expand Up @@ -312,7 +329,19 @@ _fetch_health() {
if [ -z "$_f" ]; then
_now=$(date +%s)
_age=$(( _now - 10#$_s ))
if [ "$_age" -lt "$FETCH_TIMEOUT" ]; then echo "in-flight"; else echo "abandoned"; fi
# `-ge 0` is the range half (#326, fifth site). A `started` ahead of now
# makes _age negative, and negative is `-lt` any timeout -- so the one
# answer that means "wait, something is already running" was being given
# about nothing at all, forever. Downstream that is not merely a wrong
# label: the caller then prints "already up to date" off refs no fetch
# ever refreshed, which is precisely the "silence is not a fourth way of
# saying up to date" this function's own header forbids.
#
# ABANDONED, not in-flight. We cannot substantiate a running fetch, and
# this function's contract is three honest states rather than a guess.
# Left pure -- the diagnostic belongs in _spawn_fetch, because this
# function's stdout IS the verdict and is read through $( ).
if [ "$_age" -ge 0 ] && [ "$_age" -lt "$FETCH_TIMEOUT" ]; then echo "in-flight"; else echo "abandoned"; fi
return
fi
if [ "$_rc" = "0" ]; then echo "ok"; else echo "failed:${_rc:-unknown}"; fi
Expand Down
5 changes: 3 additions & 2 deletions scripts/doctor.sh
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ _file_age_seconds() {
''|*[!0-9]*) return 1 ;;
esac
_now=$(date +%s)
echo $(( _now - _mtime ))
# 10# after the case, never instead of it (#332).
echo $(( _now - 10#$_mtime ))
return 0
}

Expand Down Expand Up @@ -343,7 +344,7 @@ if [ -d "$REMEMBER_DIR" ]; then
_MEMORY_FILE_COUNT=$((_MEMORY_FILE_COUNT + 1))
_mf_bytes=$(wc -c < "$_mf" 2>/dev/null | tr -d ' ')
case "$_mf_bytes" in ''|*[!0-9]*) _mf_bytes=0 ;; esac
_MEMORY_BYTES=$((_MEMORY_BYTES + _mf_bytes))
_MEMORY_BYTES=$((_MEMORY_BYTES + 10#$_mf_bytes))
done
done
fi
Expand Down
9 changes: 5 additions & 4 deletions scripts/lib-lock.sh
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ _lock_dir_age() {
''|*[!0-9]*) echo 0; return 0 ;;
esac
_now=$(date +%s)
echo $(( _now - _mtime ))
# 10# after the case, never instead of it (#332).
echo $(( _now - 10#$_mtime ))
}

# Adopt a lock directory that has no pid at all. A holder killed between
Expand Down Expand Up @@ -409,7 +410,7 @@ _lock_timing_us_to_ms() {
esac
# Truncation, never rounding: a hold must not come back longer than it was.
_f="${_f}000"
_LOCK_TIMING_NOW=$(( _s * 1000 + 10#${_f:0:3} ))
_LOCK_TIMING_NOW=$(( 10#$_s * 1000 + 10#${_f:0:3} ))
return 0
}

Expand All @@ -418,7 +419,7 @@ _lock_timing_ns_to_ms() {
case "$1" in
''|*[!0-9]*) _LOCK_TIMING_NOW=0; return 0 ;;
esac
_LOCK_TIMING_NOW=$(( $1 / 1000000 ))
_LOCK_TIMING_NOW=$(( 10#$1 / 1000000 ))
return 0
}

Expand All @@ -427,7 +428,7 @@ _lock_timing_s_to_ms() {
case "$1" in
''|*[!0-9]*) _LOCK_TIMING_NOW=0; return 0 ;;
esac
_LOCK_TIMING_NOW=$(( $1 * 1000 ))
_LOCK_TIMING_NOW=$(( 10#$1 * 1000 ))
return 0
}

Expand Down
22 changes: 21 additions & 1 deletion scripts/log.sh
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,24 @@ _dispatch_report_skip() {
return 0
}

# Report one thing that went wrong, in both places a human looks (#326).
#
# The generalisation of the two functions above, for callers that are not
# dispatch. `log()` alone is not enough and #252 is the demonstration: the daily
# narrative is not read, and `/remember:doctor` reports "Recent errors" out of
# hook-errors.log, which is the file a reporter is asked to paste.
#
# Written by PATH rather than by inherited stderr, for the reason
# _dispatch_report_failure gives: save-session.sh's stderr is the agent's own
# stream, and a hook must never gain the ability to write into the session.
report_error() {
log "$1" "$2"
[ -d "$REMEMBER_DIR/logs" ] || return 0
printf '%s\n' "$(_remember_date +%H:%M:%S) [$1] $2" \
>> "$REMEMBER_DIR/logs/hook-errors.log" 2>/dev/null || true
return 0
}

# Report one hook that was STOPPED because it never came back (#286).
#
# NOT _dispatch_report_failure, and the distinction is the three-state contract
Expand Down Expand Up @@ -1210,7 +1228,9 @@ rotate_logs() {
local prev=0
if [ -f "$state" ]; then read -r prev < "$state" 2>/dev/null || prev=0; fi
case "$prev" in ''|*[!0-9]*) prev=0 ;; esac
local streak=$((prev + 1))
# 10# after the case (#332) — an "08" here abandons the rest of this
# function, which is where the escalation ERROR is logged.
local streak=$((10#$prev + 1))
printf '%s\n%s\n%s\n' "$streak" "$(date '+%Y-%m-%d %H:%M:%S')" "$first_line" \
> "$state" 2>/dev/null || true

Expand Down
24 changes: 22 additions & 2 deletions scripts/post-tool-hook.sh
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,10 @@ if [ -f "$LAST_SAVE_FILE" ]; then
case "$LAST_LINE" in ''|*[!0-9]*) LAST_LINE=0 ;; esac
fi

DELTA=$((CURRENT_LINES - LAST_LINE))
# 10# after the case, never instead of it (#332) — the position is a decimal
# string from pipeline.shell, and a "08" in it would be read as octal and take
# the whole delta throttle down with the arithmetic.
DELTA=$((CURRENT_LINES - 10#$LAST_LINE))
SAVE_TRIGGERED=""

# --- Don't fork a save that save-session.sh will only discard on cooldown ---
Expand All @@ -332,7 +335,24 @@ if [ -f "$COOLDOWN_MARKER" ]; then
# needs it: SAVE_COOLDOWN is the right-hand operand of `[ ... -lt ... ]`,
# and `test` parses base 10 without evaluating -- measured, `[ 9 -lt 010 ]`
# is true. Marking it too would advertise a gap that is not there.
[ $(( $(_remember_date +%s) - 10#$LAST_TS )) -lt "$SAVE_COOLDOWN" ] && IN_COOLDOWN=true
# `-ge 0` is the range half of #326, and this site is deliberately NOT
# symmetrical with the three gates that heal the marker:
#
# * it does not rewrite it. tmp/last-save-ts belongs to save-session.sh,
# which stamps it on the very path this hook unblocks. A second writer
# on a per-tool-call path is a race for no gain.
# * it emits no diagnostic. save-session.sh emits exactly one when it
# heals; one here too would append to hook-errors.log on EVERY tool call
# for as long as the clock is behind.
# * it costs nothing (#299/#330): the value is already computed, so this
# adds no subprocess spawn and no file read to the hot path.
#
# What it must do is refuse to claim a cooldown it cannot substantiate. A
# negative ELAPSED used to read as "deep inside the window", so no save was
# forked, so save-session.sh never ran, so the marker it would have healed
# stayed ahead — the two throttles held each other shut.
_ELAPSED=$(( $(_remember_date +%s) - 10#$LAST_TS ))
[ "$_ELAPSED" -ge 0 ] && [ "$_ELAPSED" -lt "$SAVE_COOLDOWN" ] && IN_COOLDOWN=true
fi

# --- Fire save if delta exceeds threshold and no save already running ---
Expand Down
4 changes: 2 additions & 2 deletions scripts/run-consolidation.sh
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,12 @@ while IFS= read -r -d '' staging_path && IFS= read -r -d '' staging_consumed; do
rm -f "${staging_path}".tail-* 2>/dev/null
staging_tail=$(mktemp "${staging_path}.tail-XXXXXX")
if head -c "$staging_consumed" "$staging_path" > "$staging_done" 2>/dev/null &&
tail -c +$(( staging_consumed + 1 )) "$staging_path" > "$staging_tail" 2>/dev/null; then
tail -c +$(( 10#$staging_consumed + 1 )) "$staging_path" > "$staging_tail" 2>/dev/null; then
# Checked, not run bare under set -e: an unchecked failure here used
# to kill the whole script mid-loop, abandoning every other staging
# file in STAGING_PATHS_FILE rather than handling just this one.
if STAGING_MV_ERR=$(mv "$staging_tail" "$staging_path" 2>&1); then
log "consolidation" "kept $(( staging_now - staging_consumed ))b appended to $(basename "$staging_path") during consolidation"
log "consolidation" "kept $(( staging_now - 10#$staging_consumed ))b appended to $(basename "$staging_path") during consolidation"
else
# staging_path is untouched: same-directory mv is a real
# rename, which cannot fail partway (#246). staging_done
Expand Down
Loading