From 460cb573ab82b63e84c23a4a9ff566798f59e77e Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Sat, 8 Aug 2026 16:52:56 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(#326,#332):=20bound=20the=20marker's=20?= =?UTF-8?q?range,=20not=20just=20its=20syntax=20=E2=80=94=20and=20let=20a?= =?UTF-8?q?=20check=20do=20the=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #326 — the four cooldown guards validate SYNTAX and not RANGE. A digits-only marker ahead of now is accepted, makes ELAPSED negative, and a negative ELAPSED is -lt any cooldown, so the gate takes its `exit 0` — which sits ABOVE the line that rewrites the marker. The self-heal the last three releases rest on is unreachable on exactly the path that needs it: no save, no compression, no git backup, permanently and mutely, until the wall clock catches up. Three states, not two. Out of range now PROCEEDS, resets the marker at the point of rejection where the reset is reachable, and says so once in hook-errors.log via a new report_error() in log.sh. A silent clamp would trade a mute stuck throttle for a mute wrong value. post-tool-hook.sh is deliberately asymmetrical: it declines the cooldown it cannot substantiate and does nothing else — no rewrite (the marker is save-session.sh's), no diagnostic (one per tool call), no added spawn or read (#299/#330). The source comment at save-session.sh:155 asserted the opposite and is corrected here. That comment, not the CHANGELOG, is why the issue was reopened: PR #328 closed it by referencing it. #332 — twelve remaining sites read a case-guarded value into $(( )) with no 10#, not the seven the issue lists. The five extra were found by tests/test_arith_base_lint_332.py, which sweeps scripts/ and hooks.d/ for the shape mechanically and is the part that prevents a sixth issue. ShellCheck does not flag it (koalaman/shellcheck#2679). No shared helper: a helper a new author does not know about cannot stop them writing the sixteenth site. tests/test_ndc_day_boundary.py was suppressing NDC by writing a marker 34,000 years in the future — #326's defect used as an off switch. Switched to _suppress_ndc, the brake the product documents (#159). Targeted RED verified before the fix at all four sites; full suite 1545 passed, 43 skipped. Windows unverified — CI is the authority. Co-Authored-By: Max --- CHANGELOG.md | 22 ++ hooks.d/after_save/50-git-backup.sh | 14 +- scripts/doctor.sh | 5 +- scripts/lib-lock.sh | 9 +- scripts/log.sh | 22 +- scripts/post-tool-hook.sh | 24 +- scripts/run-consolidation.sh | 4 +- scripts/save-session.sh | 66 ++++- scripts/session-start-hook.sh | 5 +- tests/test_arith_base_lint_332.py | 208 +++++++++++++++ tests/test_marker_range_guard_326.py | 372 +++++++++++++++++++++++++++ tests/test_ndc_day_boundary.py | 22 +- tests/test_save_session_gates.py | 8 +- 13 files changed, 749 insertions(+), 32 deletions(-) create mode 100644 tests/test_arith_base_lint_332.py create mode 100644 tests/test_marker_range_guard_326.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 26d2d1a..bb1b4f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,28 @@ 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.** `scripts/save-session.sh:155` 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. + ## [0.18.0] — The rest of the readers [#322](https://github.com/Digital-Process-Tools/claude-remember/issues/322)'s guard was written diff --git a/hooks.d/after_save/50-git-backup.sh b/hooks.d/after_save/50-git-backup.sh index fb6be9e..99d889a 100755 --- a/hooks.d/after_save/50-git-backup.sh +++ b/hooks.d/after_save/50-git-backup.sh @@ -177,7 +177,19 @@ 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, :596/:599) 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 diff --git a/scripts/doctor.sh b/scripts/doctor.sh index 33e4813..0898e2f 100644 --- a/scripts/doctor.sh +++ b/scripts/doctor.sh @@ -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 } @@ -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 diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index 2946f68..22136cb 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -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 @@ -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 } @@ -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 } @@ -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 } diff --git a/scripts/log.sh b/scripts/log.sh index 6ccce11..a18e4da 100644 --- a/scripts/log.sh +++ b/scripts/log.sh @@ -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 @@ -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 diff --git a/scripts/post-tool-hook.sh b/scripts/post-tool-hook.sh index 8420ae3..47962fe 100755 --- a/scripts/post-tool-hook.sh +++ b/scripts/post-tool-hook.sh @@ -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 --- @@ -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 --- diff --git a/scripts/run-consolidation.sh b/scripts/run-consolidation.sh index e2a2278..1c47f8d 100755 --- a/scripts/run-consolidation.sh +++ b/scripts/run-consolidation.sh @@ -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 diff --git a/scripts/save-session.sh b/scripts/save-session.sh index 9fb4c7e..1d0cd76 100755 --- a/scripts/save-session.sh +++ b/scripts/save-session.sh @@ -152,9 +152,16 @@ if [ -f "$COOLDOWN_MARKER" ] && [ "$DRY_RUN" != true ] && [ "$FORCE" != true ]; # file does not) -- $ELAPSED is inside the abandoned body, so no unbound # variable is ever referenced there either. # - # Scope it honestly: `date +%s > "$COOLDOWN_MARKER"` sits below the `fi`, so - # it runs regardless and the marker SELF-HEALS. One corruption costs one - # skipped cooldown, not a throttle stuck off. The guard is worth having + # Scope it honestly, and the honest scope is narrower than this comment used + # to claim (#326). For a marker bash cannot PARSE, `date +%s > + # "$COOLDOWN_MARKER"` sits below the `fi`, so it runs regardless and the + # marker SELF-HEALS: one corruption, one skipped cooldown. That was written + # as though it covered every corrupt marker. It does not. A marker that + # parses and is merely OUT OF RANGE takes the `exit 0` below instead, which + # is ABOVE that line — handled explicitly now, in the range arm. The + # confident wrong scoping outlived the CHANGELOG correction at v0.17.0 by a + # release, which is the whole reason #326 had to be reopened. The guard is + # worth having # because unvalidated file content in an arithmetic evaluator is a sink in # its own right (BashPitfalls #7), and because an unexplained diagnostic in # hook-errors.log is visible to users since #277. @@ -171,7 +178,27 @@ if [ -f "$COOLDOWN_MARKER" ] && [ "$DRY_RUN" != true ] && [ "$FORCE" != true ]; esac ELAPSED=$(( $(date +%s) - 10#$LAST_MOD )) SAVE_COOLDOWN=$(config ".cooldowns.save_seconds" 120) - if [ "$ELAPSED" -lt "$SAVE_COOLDOWN" ]; then + if [ "$ELAPSED" -lt 0 ]; then + # The case above validates SYNTAX, not RANGE (#326). A marker AHEAD of + # now is all digits, so it is accepted, and it makes ELAPSED negative -- + # which reads as "deep inside the window" and takes the exit below. That + # exit sits above `date +%s > "$COOLDOWN_MARKER"`, so the self-heal the + # paragraph above promises is unreachable on exactly this path and the + # throttle stays on until the wall clock passes the marker. Permanent, + # and until now completely mute. + # + # How it happens without corruption: an NTP step backwards, a VM + # snapshot restore, a container clock jump, a store on a share with a + # skewed clock. NOT a timezone or DST change -- epoch seconds do not + # move for those, which is why proceeding here costs one extra save + # rather than mis-throttling a laptop that suspended over a boundary. + # + # PROCEED, reset, and SAY SO -- three states, not two. Clamping in + # silence would trade a mute stuck throttle for a mute wrong value, + # which is the same defect one layer along. + report_error "cooldown" "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 saving; the cooldown resumes from now." + date +%s > "$COOLDOWN_MARKER" 2>/dev/null || true + elif [ "$ELAPSED" -lt "$SAVE_COOLDOWN" ]; then debug_enabled 1 && log "cooldown" "${ELAPSED}s < ${SAVE_COOLDOWN}s, skip" exit 0 fi @@ -339,7 +366,9 @@ record_summary_failure() { fi _key="${SESSION_ID}:${POSITION}" if [ "$_prev_key" = "$_key" ]; then - _count=$(( _prev_count + 1 )) + # 10# after the case (#332): a truncated marker read back as "08" + # would abandon this branch, and the branch is what escalates. + _count=$(( 10#$_prev_count + 1 )) else _count=1 fi @@ -591,17 +620,32 @@ if [ "$(config '.features.ndc_compression' true)" != "true" ]; then log "ndc" "disabled by features.ndc_compression" fi if [ "$RUN_NDC" = true ] && [ -f "$NDC_MARKER" ]; then - # Same read and same guard as the save cooldown above. Here the abandoned - # body is the whole `if`, so `RUN_NDC=false` never runs and this save - # compresses despite the cooldown -- and `date +%s > "$NDC_MARKER"` below - # then rewrites the marker on exactly that path, so this gate self-heals - # too. One skipped compression cooldown per corruption event. + # Same read and same guard as the save cooldown above. For an UNPARSEABLE + # marker the abandoned body is the whole `if`, so `RUN_NDC=false` never runs + # and this save compresses despite the cooldown -- and `date +%s > + # "$NDC_MARKER"` below then rewrites the marker on exactly that path, so + # that case self-heals. One skipped compression cooldown per event. + # + # An OUT-OF-RANGE marker is the opposite and does not self-heal (#326): it + # parses, so `RUN_NDC=false` DOES run, and the rewrite below is inside the + # branch that decision skips. Handled in the range arm. NDC_MOD=$(cat "$NDC_MARKER" 2>/dev/null || echo 0) case "$NDC_MOD" in ''|*[!0-9]*) NDC_MOD=0 ;; esac NDC_COOLDOWN=$(config ".cooldowns.ndc_seconds" 3600) - [ $(( $(date +%s) - 10#$NDC_MOD )) -lt "$NDC_COOLDOWN" ] && RUN_NDC=false + NDC_ELAPSED=$(( $(date +%s) - 10#$NDC_MOD )) + if [ "$NDC_ELAPSED" -lt 0 ]; then + # Range, not syntax (#326) — see the save gate above. Here the marker is + # rewritten at :622 only inside `if [ "$RUN_NDC" = true ]`, so a marker + # ahead of now sets RUN_NDC=false and thereby skips the only line that + # would have healed it: now.md is never compressed again and grows + # without bound, which is the one file every later read walks. + report_error "ndc" "WARNING: $NDC_MARKER is $(( 0 - NDC_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 compressing; the cooldown resumes from now." + date +%s > "$NDC_MARKER" 2>/dev/null || true + elif [ "$NDC_ELAPSED" -lt "$NDC_COOLDOWN" ]; then + RUN_NDC=false + fi fi # The day the content belongs to, not the day this run happens to fall on. diff --git a/scripts/session-start-hook.sh b/scripts/session-start-hook.sh index 8e2057e..e1df5e0 100755 --- a/scripts/session-start-hook.sh +++ b/scripts/session-start-hook.sh @@ -872,7 +872,10 @@ if [ -f "$REMEMBER_HANDOFF" ] && [ -s "$REMEMBER_HANDOFF" ]; then echo "=== LAST HANDOFF ===" if [ -n "$PREV_FP" ] && [ "$HANDOFF_FP" = "$PREV_FP" ]; then - DELIVERIES=$((DELIVERIES + 1)) + # 10# after the case (#332). The record is explicitly hand-editable, + # which is the premise of the guard above and the one source that can + # deliver "08". + DELIVERIES=$((10#$DELIVERIES + 1)) echo "[already delivered ${DELIVERIES} times since ${FIRST_DELIVERED:-an earlier session} — no new handoff has been written since, so this is pending replacement, not news. You may already have acted on it. Running /remember replaces it.]" else DELIVERIES=1 diff --git a/tests/test_arith_base_lint_332.py b/tests/test_arith_base_lint_332.py new file mode 100644 index 0000000..ed7dfe1 --- /dev/null +++ b/tests/test_arith_base_lint_332.py @@ -0,0 +1,208 @@ +"""The digits-only guard feeds `$(( ))` without `10#`, and hand-fixing recurs (#332). + +Four issues — #321, #325, #329, #331 — each added `10#` to a correct subset of +call sites and each looked complete. None was. #327 was filed with an inventory +("`10#` appears in save-session.sh and lib-lock.sh and nowhere else") that was +already stale on the day it was written, because #329 had added two more. + +So the defect this file pins is not any one site. It is that the sweep was being +done by hand. + +THE SHAPE +========= + case "$X" in ''|*[!0-9]*) X=0 ;; esac # rejects non-digits + ... + n=$(( now - X )) # reads "08" as octal, and dies + +`08` and `09` are all digits, so they clear the guard; `$(( ))` then reads a +leading zero as octal and refuses with "value too great for base". Bash abandons +the whole enclosing command list at that point, so what is skipped is not the +arithmetic — it is every remaining statement in the branch, which in this repo +has repeatedly been the throttle, the ERROR log, or the backup. + +WHY THIS IS ENUMERABLE RATHER THAN OPEN-ENDED +============================================= +`$(( ))` is the only sink. `[ "$x" -lt "$y" ]` parses base 10 without +complaint — measured, `[ 08 -lt 9 ]` is true and silent — so a guarded value +used only in `test` is NOT a finding and is not reported as one. That is what +keeps this check finite and false-positive-free enough to be a gate. + +WHY A LINT AND NOT A SHARED HELPER +================================== +A `_remember_epoch_or_zero` in log.sh would cover most sites, and #332 argues +for it as its own change rather than bundled here. It also would not prevent +recurrence: the failure mode is a NEW call site written next month by someone +who has read none of these five issues, and a helper they do not know about +cannot help them. ShellCheck does not flag this shape either +(koalaman/shellcheck#2679, open since 2023), so CI catches nothing today. + +A check in the test suite fails in the one place people already look. + +THIS FILE IS ALSO A TEST OF ITSELF +================================== +`test_the_detector_finds_a_planted_instance` and its clean twin exist because a +linter that silently matches nothing is the exact defect class this repo keeps +shipping — an absence produced by the tool, read as an absence in the world. A +green here has to mean "looked, found none", never "did not look". +""" + +from __future__ import annotations + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +SHELL_DIRS = ("scripts", "hooks.d") + +# The guard's signature: any `case` arm rejecting a non-digit. +_DIGIT_ARM = re.compile(r"\*\[!0-9\]\*") +# `case "$VAR" in`, `case $VAR in`, `case "${VAR}" in`. +_CASE_HEAD = re.compile(r"\bcase\s+\"?\$\{?(\w+)\}?\"?\s+in\b") + +# How far above a digit-rejecting arm the `case` head may sit. The single-line +# form puts them on the same line; the block form in this repo spans two. +_CASE_LOOKBACK = 4 + + +def _guarded_names(text: str) -> set: + """Every variable this file passes through a digits-only `case`.""" + lines = text.splitlines() + names = set() + for i, line in enumerate(lines): + if not _DIGIT_ARM.search(line): + continue + for j in range(i, max(-1, i - _CASE_LOOKBACK) - 1, -1): + head = _CASE_HEAD.search(lines[j]) + if head: + names.add(head.group(1)) + break + return names + + +def _arith_spans(text: str): + """Yield (offset, source) for every `$(( ... ))`, nesting included. + + Hand-matched rather than regexed: every real site in this repo contains a + nested `$(date +%s)`, and a non-greedy match stops at that inner `))` — + which would silently exclude exactly the lines that matter. + """ + i = 0 + while True: + start = text.find("$((", i) + if start < 0: + return + depth = 0 + j = start + 1 + while j < len(text): + if text[j] == "(": + depth += 1 + elif text[j] == ")": + depth -= 1 + if depth == 0: + break + j += 1 + yield start, text[start:j + 1] + i = start + 3 + + +def _unbased_uses(text: str, names): + """Guarded names read inside `$(( ))` without a `10#` radix prefix.""" + findings = [] + for offset, span in _arith_spans(text): + for name in sorted(names): + pattern = re.compile(r"(?:\$\{?)?\b" + re.escape(name) + r"\b") + for m in pattern.finditer(span): + head = m.start() + while head > 0 and span[head - 1] in "${": + head -= 1 + if span[:head].endswith("10#"): + continue + line = text.count("\n", 0, offset + m.start()) + 1 + findings.append((line, name, span.strip())) + return findings + + +def _shell_files(): + for directory in SHELL_DIRS: + yield from sorted((REPO_ROOT / directory).rglob("*.sh")) + + +def test_no_case_guarded_value_reaches_arithmetic_without_a_radix(): + """The sweep. One finding fails the build and names file, line and variable. + + If you are reading this because it went red on a line you wrote: the fix is + `10#$VAR` inside the `$(( ))`, AFTER the `case`, 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 otherwise accept. + """ + offenders = [] + for path in _shell_files(): + text = path.read_text(encoding="utf-8", errors="replace") + names = _guarded_names(text) + if not names: + continue + for line, name, span in _unbased_uses(text, names): + rel = path.relative_to(REPO_ROOT) + offenders.append(f" {rel}:{line} ${name} in {span}") + + assert not offenders, ( + "a value guarded by a digits-only `case` reaches `$(( ))` with no " + "`10#`, so 08/09 clear the guard and are then read as octal — bash " + "abandons the rest of the enclosing command list, which is where the " + "throttle, the ERROR log or the backup lives " + "(#321/#325/#327/#329/#331/#332):\n" + "\n".join(offenders) + ) + + +PLANTED_GUARD = ( + 'LAST=$(cat "$f")\n' + 'case "$LAST" in ''|*[!0-9]*) LAST=0 ;; esac\n' +) + + +def test_the_detector_finds_a_planted_instance(): + """A linter that matches nothing must not be able to read as a clean repo.""" + planted = PLANTED_GUARD + "ELAPSED=$(( $(date +%s) - LAST ))\n" + names = _guarded_names(planted) + assert names == {"LAST"}, names + found = _unbased_uses(planted, names) + assert [f[1] for f in found] == ["LAST"], found + + +def test_the_detector_finds_it_through_the_dollar_form_too(): + planted = PLANTED_GUARD + "ELAPSED=$(( $(date +%s) - $LAST ))\n" + assert [f[1] for f in _unbased_uses(planted, _guarded_names(planted))] == ["LAST"] + + +def test_the_detector_accepts_the_fixed_form(): + fixed = PLANTED_GUARD + "ELAPSED=$(( $(date +%s) - 10#$LAST ))\n" + assert _unbased_uses(fixed, _guarded_names(fixed)) == [] + + +def test_the_detector_does_not_flag_a_test_builtin_comparison(): + """`[ 08 -lt 9 ]` is true and silent, so `test` is not a sink. Flagging it + would advertise a gap that is not there and train people to ignore this.""" + only_test = ( + 'case "$COOLDOWN" in ''|*[!0-9]*) COOLDOWN=120 ;; esac\n' + '[ "$ELAPSED" -lt "$COOLDOWN" ] && echo throttled\n' + ) + assert _unbased_uses(only_test, _guarded_names(only_test)) == [] + + +def test_the_sweep_actually_reads_the_shell_files(): + """`_guarded_names` returning nothing everywhere would make the sweep vacuous + and green. Pin that the corpus is non-empty and that the guard is found in + it, so a rename of scripts/ or hooks.d/ fails here instead of going quiet.""" + files = list(_shell_files()) + assert len(files) > 10, f"only {len(files)} shell files found — corpus is wrong" + guarded = { + p.relative_to(REPO_ROOT).as_posix(): _guarded_names( + p.read_text(encoding="utf-8", errors="replace")) + for p in files + } + with_guards = {k: v for k, v in guarded.items() if v} + assert len(with_guards) >= 5, ( + "the digits-only guard was found in almost no file — the detector has " + f"stopped recognising it: {with_guards}" + ) diff --git a/tests/test_marker_range_guard_326.py b/tests/test_marker_range_guard_326.py new file mode 100644 index 0000000..9659087 --- /dev/null +++ b/tests/test_marker_range_guard_326.py @@ -0,0 +1,372 @@ +"""A digits-only cooldown marker AHEAD of now sticks the throttle ON (#326). + +#322/#324/#327 taught the four cooldown gates to reject a marker that is not a +number. None of them bounds the number. `case "$X" in ''|*[!0-9]*) X=0 ;; esac` +accepts `99999999999999999999` and `9223372036854775807` as readily as it +accepts a real epoch, and both produce a NEGATIVE `ELAPSED`: + + $ M=9223372036854775807; echo $(( $(date +%s) - 10#$M )) + -9223372035068589843 + +`[ "$ELAPSED" -lt "$SAVE_COOLDOWN" ]` is then true and the gate takes its +`exit 0` — which sits ABOVE the line that rewrites the marker. So the self-heal +every one of those four fixes rests on is unreachable on exactly the path that +needs it, and the throttle stays engaged until the wall clock catches up. + +That is a different class from #322. A syntax-error marker self-heals and leaves +one line in the log; this one is PERMANENT and MUTE. Class `fails-to-preserve`: +the data is still on the machine, what stops is the preservation. + +How a marker gets ahead of now without anybody corrupting anything: an NTP step +backwards, a VM snapshot restore, a container clock jump, a store on a share +with a skewed clock. Not a timezone or DST change — epoch seconds do not move +for those, which is why proceeding here costs at most one extra save rather than +mis-throttling a laptop that suspended over a TZ boundary. + +WHAT IS PINNED, AND WHY IT IS BEHAVIOUR RATHER THAN A DIAGNOSTIC +================================================================ +The issue is explicit that a diagnostic alone is the wrong pin here: falling +back to 0 makes a corrupt marker read as "very old", so corrupt and +clean-but-ancient reach the same decision. A future-dated marker is different — +it reaches the OPPOSITE decision from every other input — so the observable is +the one that matters: does the second run still do the work. + +Each gate is pinned twice, and both halves have to hold: + + * the work happens (a save extracts, NDC compresses, the backup commits), and + * the marker is left holding a sane value, so the run after this one is + throttled normally instead of skipping again. + +The diagnostic is pinned SEPARATELY, in its own tests, so a reworded message +cannot take the behaviour assertions down with it — and so that a silent clamp +cannot pass. A silent clamp trades a mute stuck throttle for a mute wrong value, +which is the same defect one layer along. + +CONTROLS +======== +`test_a_marker_written_now_still_throttles` and its NDC/backup siblings are +green against the UNFIXED scripts on purpose. Without them a "fix" that simply +retires the cooldown would satisfy everything above. +""" + +from __future__ import annotations + +import sys +import time +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.skipif( + sys.platform == "win32", + reason="bash subprocess + POSIX layout — not portable to Windows runners (#79)", +) + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "tests")) + +from subprocess_helpers import subprocess_failure_detail +from test_save_session_gates import _make_env, _run +from test_save_session_marker_arithmetic_322 import _set_cooldowns + +# Three ways to land ahead of `now`, kept as three because they fail for two +# different reasons and a fix could plausibly catch one and miss the others. +# +# 3600 a plain clock step backwards. Still a valid epoch. +# 9223372036854775807 INT64_MAX. Digits only, so the case guard passes it. +# 99999999999999999999 wider than int64; `10#` wraps it. Measured negative +# on both bash 3.2.57 and 5.2.37 (the issue's own +# transcript) — asserted below rather than assumed, +# because a wrap that landed positive would make this +# parameter vacuous. +OVERFLOW_MARKERS = ["9223372036854775807", "99999999999999999999"] + + +def _future(seconds: int) -> str: + return str(int(time.time()) + seconds) + + +def _elapsed_is_negative(marker: str) -> bool: + """Does this marker actually reach the gate as a negative ELAPSED? + + A parameter that cannot fail is not a test. Computed the way bash computes + it, in Python's unbounded ints wrapped to int64, so an overflow value that + happened to land positive is skipped loudly instead of passing silently. + """ + value = int(marker) + wrapped = ((value + 2 ** 63) % 2 ** 64) - 2 ** 63 + return int(time.time()) - wrapped < 0 + + +@pytest.mark.parametrize("marker_value", OVERFLOW_MARKERS) +def test_the_overflow_parameters_really_do_go_negative(marker_value): + assert _elapsed_is_negative(marker_value), ( + f"{marker_value!r} does not produce a negative ELAPSED on this " + "platform, so every case parametrized on it below is vacuous" + ) + + +def _hook_errors(project: Path) -> str: + log = project / ".remember" / "logs" / "hook-errors.log" + return log.read_text(encoding="utf-8", errors="replace") if log.is_file() else "" + + +# ═════════════════════════════════════════════════════════════════════════════ +# save-session.sh :169 — the save cooldown +# ═════════════════════════════════════════════════════════════════════════════ + +@pytest.mark.parametrize("marker_value", [None, *OVERFLOW_MARKERS]) +def test_a_future_dated_save_marker_does_not_stick_the_throttle(tmp_path, marker_value): + """The headline. Nothing is ever saved again while the marker is ahead.""" + stamp = _future(3600) if marker_value is None else marker_value + env, project, plugin, calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + _set_cooldowns(env, plugin, save_seconds=120, ndc_seconds=999999) + (project / ".remember" / "tmp" / "last-save-ts").write_text(stamp) + + proc = _run(plugin, env, session_id) + + assert proc.returncode == 0, subprocess_failure_detail( + proc, project / ".remember") + ran = calls.read_text() if calls.is_file() else "" + assert "extract" in ran, ( + f"a cooldown marker holding {stamp!r} — ahead of now, and all digits so " + "the guard accepts it — made ELAPSED negative, which reads as 'inside " + "the window' and takes the exit 0. That exit sits above the line that " + "rewrites the marker, so no save will ever run again until the wall " + f"clock passes the marker. calls: {ran!r}" + ) + + +def test_a_future_dated_save_marker_is_reset_to_a_usable_value(tmp_path): + """Self-heal, the other half. Proceeding once is not enough if the marker + is left ahead: the NEXT run reads the same value and skips again.""" + env, project, plugin, _calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + _set_cooldowns(env, plugin, save_seconds=120, ndc_seconds=999999) + marker = project / ".remember" / "tmp" / "last-save-ts" + marker.write_text(_future(3600)) + + _run(plugin, env, session_id) + + after = marker.read_text(encoding="utf-8").strip() + assert after.isdigit(), f"marker is not a number after the run: {after!r}" + assert int(after) <= int(time.time()) + 2, ( + f"the marker is still ahead of now ({after}) — the run got through but " + "left the trap armed for the next one" + ) + + +def test_a_future_dated_save_marker_says_so(tmp_path): + """A silent clamp trades a mute stuck throttle for a mute wrong value. + + hook-errors.log, not just the daily log: /remember:doctor reports 'Recent + errors' out of it (#252/#260/#266), and a clock that stepped backwards far + enough to disable saving is exactly what a user pastes that file to find. + """ + env, project, plugin, _calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + _set_cooldowns(env, plugin, save_seconds=120, ndc_seconds=999999) + (project / ".remember" / "tmp" / "last-save-ts").write_text(_future(3600)) + + _run(plugin, env, session_id) + + errors = _hook_errors(project) + assert "ahead of now" in errors, ( + "the marker was ahead of now and nothing said so. The value was " + "rejected and replaced silently, which is the same shape as the bug: a " + f"decision nobody can see.\n--- hook-errors.log ---\n{errors.strip()}" + ) + + +def test_a_marker_written_now_still_throttles(tmp_path): + """CONTROL — green before the fix too. The range check must not become a + way of turning the cooldown off for everybody.""" + env, project, plugin, calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + _set_cooldowns(env, plugin, save_seconds=3600, ndc_seconds=999999) + (project / ".remember" / "tmp" / "last-save-ts").write_text(str(int(time.time()))) + + proc = _run(plugin, env, session_id) + + assert proc.returncode == 0, subprocess_failure_detail( + proc, project / ".remember") + ran = calls.read_text() if calls.is_file() else "" + assert "extract" not in ran, ( + f"a marker written seconds ago no longer engages the cooldown: {ran!r}" + ) + assert "ahead of now" not in _hook_errors(project), ( + "a perfectly good marker was reported as being in the future" + ) + + +# ═════════════════════════════════════════════════════════════════════════════ +# save-session.sh :593 — the NDC compression gate +# ═════════════════════════════════════════════════════════════════════════════ + +def test_a_future_dated_ndc_marker_does_not_stop_compression(tmp_path): + """Same shape, different consequence: now.md is never compressed again and + grows without bound, which is the one file every later read walks.""" + env, project, plugin, _calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + # Without a haiku body the run exits at Step 6 and never reaches Step 8 — + # the gate under test. #322's NDC case documents the same trap. + env["STUB_HAIKU_TEXT"] = "## 12:00 | main\n\n- an entry\n" + _set_cooldowns(env, plugin, save_seconds=0, ndc_seconds=3600) + marker = project / ".remember" / "tmp" / "last-ndc.ts" + marker.write_text(_future(7200)) + + proc = _run(plugin, env, session_id) + + assert proc.returncode == 0, subprocess_failure_detail( + proc, project / ".remember") + after = marker.read_text(encoding="utf-8").strip() + assert after.isdigit() and int(after) <= int(time.time()) + 2, ( + f"the NDC marker is still ahead of now ({after!r}) — the gate took the " + "RUN_NDC=false branch, and the marker is only rewritten inside the " + "branch it did not take, so compression never runs again" + ) + + +def test_a_future_dated_ndc_marker_says_so(tmp_path): + env, project, plugin, _calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + env["STUB_HAIKU_TEXT"] = "## 12:00 | main\n\n- an entry\n" + _set_cooldowns(env, plugin, save_seconds=0, ndc_seconds=3600) + (project / ".remember" / "tmp" / "last-ndc.ts").write_text(_future(7200)) + + _run(plugin, env, session_id) + + errors = _hook_errors(project) + assert "ahead of now" in errors, ( + "the NDC marker was ahead of now and nothing said so\n" + f"--- hook-errors.log ---\n{errors.strip()}" + ) + + +def test_a_recent_ndc_marker_still_suppresses_compression(tmp_path): + """CONTROL — green before the fix.""" + env, project, plugin, _calls, session_id = _make_env( + tmp_path, exchanges=40, humans=5) + env["STUB_HAIKU_TEXT"] = "## 12:00 | main\n\n- an entry\n" + _set_cooldowns(env, plugin, save_seconds=0, ndc_seconds=3600) + marker = project / ".remember" / "tmp" / "last-ndc.ts" + written = str(int(time.time())) + marker.write_text(written) + + _run(plugin, env, session_id) + + assert marker.read_text(encoding="utf-8").strip() == written, ( + "a marker written seconds ago no longer suppresses NDC — the gate has " + "been disabled rather than bounded" + ) + +# ═════════════════════════════════════════════════════════════════════════════ +# scripts/post-tool-hook.sh :322 — the fork throttle that reads the same marker +# ═════════════════════════════════════════════════════════════════════════════ +# +# This one is deliberately NOT symmetrical with the other three, and the +# asymmetry is the design: +# +# * it does not rewrite the marker. `tmp/last-save-ts` belongs to +# save-session.sh, which stamps it on the 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 +# the marker; emitting one here as well would put a line in +# hook-errors.log on EVERY tool call for as long as the clock is behind. +# * it therefore costs zero extra subprocess spawns and zero extra file +# reads (#299/#330) — the range test reuses the value already computed. +# +# What it must do is decline to claim a cooldown it cannot substantiate, so the +# fork happens and save-session.sh gets the chance to heal. + +from test_post_tool_cooldown import _reap, _run_post_tool + + +def test_the_fork_throttle_does_not_stick_on_a_future_marker(tmp_path): + proc, remember = _run_post_tool(tmp_path, cooldown_ts=int(time.time()) + 3600) + _reap(remember) + + assert proc.returncode == 0 + assert (remember / "tmp" / "save-session.pid").exists(), ( + "the hook read a marker an hour ahead of now, computed a negative " + "ELAPSED, and took that for 'inside the cooldown'. No save is forked, " + "so save-session.sh never runs, so the marker it would have healed " + "stays ahead — the two throttles hold each other shut" + ) + + +def test_the_fork_throttle_still_suppresses_on_a_fresh_marker(tmp_path): + """CONTROL — green before the fix (this is #125's own test, restated).""" + proc, remember = _run_post_tool(tmp_path, cooldown_ts=int(time.time())) + _reap(remember) + + assert proc.returncode == 0 + assert not (remember / "tmp" / "save-session.pid").exists(), ( + "the range check disabled the fork throttle instead of bounding it" + ) + + +# ═════════════════════════════════════════════════════════════════════════════ +# hooks.d/after_save/50-git-backup.sh :157 — #258's outage, through an +# accepted value +# ═════════════════════════════════════════════════════════════════════════════ + +from tests.test_git_backup_hook import hook_state +from tests.test_git_backup_silent_stops_257 import ( + _backup_config, + _log_or_empty, + _run_backup, + _slug_is_committed, + _store, + _wait_until, +) + + +def test_a_future_dated_backup_marker_does_not_stop_the_backup(tmp_path): + """#258's original outage, reached through a value the guard ACCEPTS. + + `exit 0` at the cooldown sits above every path that stamps the marker + (`_gb_stamp_cooldown`, :596/:599), so once it is ahead of now the store is + never committed again — the git history simply stops, and the only trace is + the absence of commits. + """ + home, remember, _remote, slug_dir, project = _store(tmp_path) + marker = hook_state(remember, ".last-git-backup-ts", create_dir=True) + marker.write_text(str(int(time.time()) + 3600), encoding="utf-8") + + proc = _run_backup(slug_dir, project, home, _backup_config(tmp_path)) + + assert proc.returncode == 0, proc.stderr.strip() + assert _wait_until(lambda: _slug_is_committed(remember)), ( + "a cooldown marker an hour ahead of now stopped the git backup, and " + "the marker is only rewritten below the exit it took — so it stays " + "ahead and the store is never backed up again" + ) + + +def test_a_future_dated_backup_marker_says_so(tmp_path): + home, remember, _remote, slug_dir, project = _store(tmp_path) + hook_state(remember, ".last-git-backup-ts", create_dir=True).write_text( + str(int(time.time()) + 3600), encoding="utf-8") + + _run_backup(slug_dir, project, home, _backup_config(tmp_path)) + + assert _wait_until(lambda: "ahead of now" in _log_or_empty(slug_dir)), ( + "the backup cooldown marker was ahead of now and nothing said so\n" + f"--- log ---{_log_or_empty(slug_dir)}" + ) + + +def test_a_fresh_backup_marker_still_suppresses_the_backup(tmp_path): + """CONTROL — green before the fix.""" + home, remember, _remote, slug_dir, project = _store(tmp_path) + hook_state(remember, ".last-git-backup-ts", create_dir=True).write_text( + str(int(time.time())), encoding="utf-8") + + _run_backup(slug_dir, project, home, _backup_config(tmp_path)) + + assert not _slug_is_committed(remember), ( + "a marker written seconds ago no longer suppresses the backup — the " + "cooldown has been disabled rather than bounded" + ) diff --git a/tests/test_ndc_day_boundary.py b/tests/test_ndc_day_boundary.py index 743c74f..432ec82 100644 --- a/tests/test_ndc_day_boundary.py +++ b/tests/test_ndc_day_boundary.py @@ -38,9 +38,19 @@ REPO_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO_ROOT / "tests")) -from test_save_session_gates import _make_env, _run # noqa: E402 - - +from test_save_session_gates import _make_env, _run, _suppress_ndc # noqa: E402 + + +# These tests need NDC OFF so that now.md survives to be asserted on. They used +# to get that by writing `2**40` into tmp/last-ndc.ts — a marker ~34,000 years +# in the future, whose negative ELAPSED read as "inside the cooldown". +# +# That is #326's defect, used as an off switch. It stopped working the moment +# the range guard landed, which is the correct outcome twice over: the gate no +# longer accepts a marker ahead of now, and #159 already established that a +# hand-written marker was never the supported brake. `_suppress_ndc` writes a +# CURRENT timestamp against the 999999s cooldown `_make_env` configures, which +# suppresses NDC through the mechanism the product documents. def _day_stamp(project: Path) -> str: f = project / ".remember" / "tmp" / "now-day" return f.read_text(encoding="utf-8").strip() if f.is_file() else "" @@ -55,7 +65,7 @@ def test_now_md_is_stamped_with_the_day_it_started(tmp_path): """Nothing else in the file records the day.""" env, project, plugin, calls, sid = _make_env(tmp_path, exchanges=6, humans=5) env["STUB_HAIKU_TEXT"] = "## 23:50 | main\n\n- late evening work\n" - (project / ".remember" / "tmp" / "last-ndc.ts").write_text(str(2**40)) # suppress NDC + _suppress_ndc(project) _run(plugin, env, sid) from datetime import datetime @@ -71,7 +81,7 @@ def test_now_md_is_stamped_with_the_day_it_started(tmp_path): def test_the_stamp_marks_the_file_not_each_entry(tmp_path): """Three entries, one day — the stamp is set when the file starts.""" env, project, plugin, calls, sid = _make_env(tmp_path, exchanges=6, humans=5) - (project / ".remember" / "tmp" / "last-ndc.ts").write_text(str(2**40)) + _suppress_ndc(project) for i in range(3): env["STUB_HAIKU_TEXT"] = f"## 1{i}:00 | main\n\n- entry {i}\n" _run(plugin, env, sid) @@ -140,7 +150,7 @@ def test_the_day_is_not_written_into_now_md(tmp_path): know about it, and no reader has to strip it back out. """ env, project, plugin, calls, sid = _make_env(tmp_path, exchanges=6, humans=5) - (project / ".remember" / "tmp" / "last-ndc.ts").write_text(str(2**40)) + _suppress_ndc(project) env["STUB_HAIKU_TEXT"] = "## 12:00 | main\n\n- work\n" _run(plugin, env, sid) diff --git a/tests/test_save_session_gates.py b/tests/test_save_session_gates.py index 96ba470..082c241 100644 --- a/tests/test_save_session_gates.py +++ b/tests/test_save_session_gates.py @@ -277,8 +277,12 @@ def _saved_position(project: Path): def _suppress_ndc(project: Path): """Stop the background NDC run from draining now.md mid-assertion. - features.ndc_compression is documented but read nowhere, so the only - working brake is the cooldown marker (ndc_seconds is 999999 here). + A CURRENT timestamp against the 999999s ndc_seconds _make_env configures. + Not a future-dated one: that used to work, by way of #326's negative + ELAPSED, and the range guard now rejects it. + + (features.ndc_compression was documented and read nowhere when this was + written; #159 wired it up, so it is a second working brake today.) """ import time (project / ".remember" / "tmp" / "last-ndc.ts").write_text(str(int(time.time()))) From a00fa309a3c07587478ab7c950ee24438f80d35a Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:38:42 +0200 Subject: [PATCH 2/2] review(#336): a fifth site the issue never named, and a lint that would have cried wolf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the /code-review pass on PR #336, fixed in the same run. FIFTH SITE (#326). hooks.d/before_session_start/50-git-restore.sh reads `started=` through the same digits-only case. A value ahead of now makes _age negative, so _fetch_health answers "in-flight" — the one state meaning "wait, something is already running" — about nothing, and _spawn_fetch takes its early return so the only writers of the record never run. Same geometry as the four sites #326 enumerates, and not on its list. The harm is worse than a wrong label: the caller then prints "already up to date with origin/main" off refs no fetch refreshed, which is exactly what _fetch_health's own header forbids. Now answers "abandoned" and spawns a real fetch that rewrites the record. _fetch_health stays pure — its stdout is the verdict, read through $( ) — so the diagnostic goes in _spawn_fetch. LINT COULD NOT BE TRUSTED AS A GATE (#332). `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 guarding a positional parameter was a finding. lib-lock.sh guards two, so the next ordinary arithmetic there would have failed CI for no reason. Numeric names now require the sigil. ITS OWN FIXTURE WAS NOT THE SHAPE IT CLAIMED. `''` inside a single-quoted Python literal closes it and opens another, so the planted guard had silently become `case "$LAST" in |*[!0-9]*)`. A self-test whose fixture is not what it plants still goes green, and green here is supposed to mean "looked, found none". Pinned by a test. STALE LINE-NUMBER CITATIONS. Six comments this PR added cited line numbers; three were already wrong at commit time. All replaced with construct names. A PR that exists because a comment outlived the truth should not add more. Targeted RED verified before each fix — including catching that the first draft of the restore tests passed vacuously, because a doubled backslash put a literal \\n in the state file and the digits-only guard rejected it for the wrong reason. Full suite 1551 passed, 43 skipped. Windows unverified. Co-Authored-By: Max --- CHANGELOG.md | 10 ++- hooks.d/after_save/50-git-backup.sh | 7 +- .../before_session_start/50-git-restore.sh | 33 +++++++- scripts/save-session.sh | 2 +- tests/test_arith_base_lint_332.py | 76 +++++++++++++++++-- tests/test_git_restore_hook_253.py | 68 +++++++++++++++++ tests/test_marker_range_guard_326.py | 20 +++-- 7 files changed, 196 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb1b4f9..58692b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **`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.** `scripts/save-session.sh:155` 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. + **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. @@ -27,6 +27,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **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 diff --git a/hooks.d/after_save/50-git-backup.sh b/hooks.d/after_save/50-git-backup.sh index 99d889a..7db40af 100755 --- a/hooks.d/after_save/50-git-backup.sh +++ b/hooks.d/after_save/50-git-backup.sh @@ -181,9 +181,10 @@ if [ -f "$COOLDOWN_MARKER" ]; 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, :596/:599) 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. + # (_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. diff --git a/hooks.d/before_session_start/50-git-restore.sh b/hooks.d/before_session_start/50-git-restore.sh index fceff32..79396eb 100755 --- a/hooks.d/before_session_start/50-git-restore.sh +++ b/hooks.d/before_session_start/50-git-restore.sh @@ -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 @@ -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 diff --git a/scripts/save-session.sh b/scripts/save-session.sh index 1d0cd76..a3229b2 100755 --- a/scripts/save-session.sh +++ b/scripts/save-session.sh @@ -637,7 +637,7 @@ if [ "$RUN_NDC" = true ] && [ -f "$NDC_MARKER" ]; then NDC_ELAPSED=$(( $(date +%s) - 10#$NDC_MOD )) if [ "$NDC_ELAPSED" -lt 0 ]; then # Range, not syntax (#326) — see the save gate above. Here the marker is - # rewritten at :622 only inside `if [ "$RUN_NDC" = true ]`, so a marker + # rewritten only inside `if [ "$RUN_NDC" = true ]`, so a marker # ahead of now sets RUN_NDC=false and thereby skips the only line that # would have healed it: now.md is never compressed again and grows # without bound, which is the one file every later read walks. diff --git a/tests/test_arith_base_lint_332.py b/tests/test_arith_base_lint_332.py index ed7dfe1..18983c6 100644 --- a/tests/test_arith_base_lint_332.py +++ b/tests/test_arith_base_lint_332.py @@ -111,7 +111,17 @@ def _unbased_uses(text: str, names): findings = [] for offset, span in _arith_spans(text): for name in sorted(names): - pattern = re.compile(r"(?:\$\{?)?\b" + re.escape(name) + r"\b") + if name.isdigit(): + # A positional parameter: `case "$1" in` captures the name "1". + # Inside `$(( ))` a bare `1` is the integer one, not a reference + # to $1, so the sigil is REQUIRED here. Without it every + # `$(( x + 1 ))` in a file that guards a positional becomes a + # finding — and lib-lock.sh guards two of them today, so this + # gate would have started failing the build on the next + # ordinary line of arithmetic anyone added there. + pattern = re.compile(r"\$\{?" + re.escape(name) + r"\}?") + else: + pattern = re.compile(r"(?:\$\{?)?\b" + re.escape(name) + r"\b") for m in pattern.finditer(span): head = m.start() while head > 0 and span[head - 1] in "${": @@ -155,9 +165,13 @@ def test_no_case_guarded_value_reaches_arithmetic_without_a_radix(): ) +# Double-quoted, with the inner quotes escaped, and NOT single-quoted: `''` +# inside a single-quoted Python literal closes it and opens another, so the +# empty-string case arm vanishes into implicit concatenation with no error. +# test_the_planted_guard_is_actually_the_shape_it_claims pins that. PLANTED_GUARD = ( - 'LAST=$(cat "$f")\n' - 'case "$LAST" in ''|*[!0-9]*) LAST=0 ;; esac\n' + "LAST=$(cat \"$f\")\n" + "case \"$LAST\" in ''|*[!0-9]*) LAST=0 ;; esac\n" ) @@ -184,8 +198,8 @@ def test_the_detector_does_not_flag_a_test_builtin_comparison(): """`[ 08 -lt 9 ]` is true and silent, so `test` is not a sink. Flagging it would advertise a gap that is not there and train people to ignore this.""" only_test = ( - 'case "$COOLDOWN" in ''|*[!0-9]*) COOLDOWN=120 ;; esac\n' - '[ "$ELAPSED" -lt "$COOLDOWN" ] && echo throttled\n' + "case \"$COOLDOWN\" in ''|*[!0-9]*) COOLDOWN=120 ;; esac\n" + "[ \"$ELAPSED\" -lt \"$COOLDOWN\" ] && echo throttled\n" ) assert _unbased_uses(only_test, _guarded_names(only_test)) == [] @@ -206,3 +220,55 @@ def test_the_sweep_actually_reads_the_shell_files(): "the digits-only guard was found in almost no file — the detector has " f"stopped recognising it: {with_guards}" ) + +# ── Two ways this detector can lie, both found reviewing #336 ──────────────── + +def test_the_planted_guard_is_actually_the_shape_it_claims(): + """The fixtures above must contain a real empty-string case arm. + + `''` inside a SINGLE-quoted Python string is not two apostrophes — it closes + the literal and opens another, and the two characters vanish into implicit + concatenation with no error. The planted guard silently became + `case "$LAST" in |*[!0-9]*)`, which still matched only because the detector + keys off the `*[!0-9]*` arm. + + A self-test whose fixture is not the shape it claims still goes green, and a + green here is supposed to mean "looked, found none". + """ + assert "in ''|" in PLANTED_GUARD, ( + f"the empty-string arm was eaten by Python string concatenation: " + f"{PLANTED_GUARD!r}" + ) + + +POSITIONAL_GUARD = "case \"$1\" in ''|*[!0-9]*) return 1 ;; esac\n" + + +def test_a_positional_parameter_guard_does_not_flag_the_integer_literal(): + """`case "$1" in` guards a positional parameter, so the captured name is the + bare string "1" — and a bare `1` inside `$(( ))` is the integer one, not a + reference to `$1`. + + Matching the name without its sigil turns every `$(( x + 1 ))` in the file + into a finding. scripts/lib-lock.sh already carries two of these guards + (_lock_timing_ns_to_ms, _lock_timing_s_to_ms), so the next ordinary + arithmetic added to that file would have failed the build for no reason. + + A lint gate that cries wolf gets switched off, and then it is not a gate. + """ + src = POSITIONAL_GUARD + "MS=$(( x + 1 ))\n" + assert _guarded_names(src) == {"1"} + assert _unbased_uses(src, _guarded_names(src)) == [], ( + "the integer literal 1 was mistaken for a reference to $1" + ) + + +def test_a_positional_parameter_is_still_flagged_in_its_dollar_form(): + """The other side: narrowing to the sigil must not make $1 invisible.""" + src = POSITIONAL_GUARD + "MS=$(( $1 * 1000 ))\n" + assert [f[1] for f in _unbased_uses(src, _guarded_names(src))] == ["1"] + + +def test_a_positional_parameter_accepts_the_fixed_form(): + src = POSITIONAL_GUARD + "MS=$(( 10#$1 * 1000 ))\n" + assert _unbased_uses(src, _guarded_names(src)) == [] diff --git a/tests/test_git_restore_hook_253.py b/tests/test_git_restore_hook_253.py index ae0fd54..5e04fa8 100644 --- a/tests/test_git_restore_hook_253.py +++ b/tests/test_git_restore_hook_253.py @@ -598,6 +598,74 @@ def test_a_leading_zero_start_time_is_read_as_decimal(self, tmp_path): "the session is told a fetch failed when none did\n--- log ---\n" + log ) + def test_a_future_dated_start_time_does_not_stick_the_fetch_in_flight(self, tmp_path): + """#326's fifth site, found reviewing PR #336 rather than by the issue. + + #326 enumerates four cooldown gates and stops. The fetch state file has + the identical shape and was not on the list: `started=` is read through + the same digits-only `case`, which bounds its SYNTAX and not its RANGE. + + A `started` ahead of now makes `_age` negative, `-lt "$FETCH_TIMEOUT"` + is then true, and `_fetch_health` answers `in-flight` — the one state + that means "wait, something is already running". Nothing is running. + + And it compounds the way the four named sites do: `_spawn_fetch` runs + the same comparison and takes its `return 0`, so the replacement fetch + is never started — and the ONLY writers of the state file are inside + the subshell that early return skips. The self-heal is unreachable from + the path that needs it, exactly as at the other four. + """ + home, remember, remote, slug_dir, project = _store(tmp_path) + _fetch_now(remember) + (hook_state(remember, FETCH_STATE, create_dir=True)).write_text( + f"started={int(time.time()) + 3600}\n", encoding="utf-8" + ) + + _run(slug_dir, project, home, _config(tmp_path, enabled=True)) + + log = _log_text(slug_dir) + assert "never completed" in log, ( + "a fetch whose start time is an hour in the future was reported as " + "IN-FLIGHT. Nothing is in flight; the clock moved. The session is " + "told to wait for a process that does not exist, and will be told " + "the same thing every session until the wall clock catches up." + "\n--- log ---\n" + log + ) + + def test_a_future_dated_start_time_is_replaced_by_a_real_fetch(self, tmp_path): + """The structural half: the state file has to become writable again. + + Reporting the right state is not enough. `_spawn_fetch` must decline to + treat a future `started` as a live claim on the job, or nothing ever + rewrites the file and the wrong state is permanent. + """ + home, remember, remote, slug_dir, project = _store(tmp_path) + _fetch_now(remember) + state = hook_state(remember, FETCH_STATE, create_dir=True) + future = int(time.time()) + 3600 + state.write_text(f"started={future}\n", encoding="utf-8") + + _run(slug_dir, project, home, _config(tmp_path, enabled=True)) + + deadline = time.monotonic() + 60 + started = future + while time.monotonic() < deadline: + body = dict( + line.split("=", 1) + for line in state.read_text(encoding="utf-8").splitlines() + if "=" in line + ) + started = int(body.get("started", future)) + if started != future: + break + time.sleep(0.2) + + assert started <= int(time.time()) + 2, ( + f"the fetch state file still holds a future start time ({started}) " + "— _spawn_fetch took its early return, so no fetch was spawned, so " + "nothing rewrote the file. The store never fetches again." + ) + # ── The detached fetch ─────────────────────────────────────────────────────── diff --git a/tests/test_marker_range_guard_326.py b/tests/test_marker_range_guard_326.py index 9659087..b048cb6 100644 --- a/tests/test_marker_range_guard_326.py +++ b/tests/test_marker_range_guard_326.py @@ -112,8 +112,13 @@ def _hook_errors(project: Path) -> str: # ═════════════════════════════════════════════════════════════════════════════ -# save-session.sh :169 — the save cooldown +# save-session.sh — the save cooldown gate # ═════════════════════════════════════════════════════════════════════════════ +# +# Named rather than numbered, here and below. A line number in a comment is +# stale the next time anyone edits above it — and three of the four in the first +# draft of this file were already wrong by the time it was committed. This PR +# exists because a comment outlived the truth; it should not add more of them. @pytest.mark.parametrize("marker_value", [None, *OVERFLOW_MARKERS]) def test_a_future_dated_save_marker_does_not_stick_the_throttle(tmp_path, marker_value): @@ -201,7 +206,7 @@ def test_a_marker_written_now_still_throttles(tmp_path): # ═════════════════════════════════════════════════════════════════════════════ -# save-session.sh :593 — the NDC compression gate +# save-session.sh — the NDC compression gate # ═════════════════════════════════════════════════════════════════════════════ def test_a_future_dated_ndc_marker_does_not_stop_compression(tmp_path): @@ -262,7 +267,7 @@ def test_a_recent_ndc_marker_still_suppresses_compression(tmp_path): ) # ═════════════════════════════════════════════════════════════════════════════ -# scripts/post-tool-hook.sh :322 — the fork throttle that reads the same marker +# scripts/post-tool-hook.sh — the fork throttle that reads the same marker # ═════════════════════════════════════════════════════════════════════════════ # # This one is deliberately NOT symmetrical with the other three, and the @@ -308,8 +313,7 @@ def test_the_fork_throttle_still_suppresses_on_a_fresh_marker(tmp_path): # ═════════════════════════════════════════════════════════════════════════════ -# hooks.d/after_save/50-git-backup.sh :157 — #258's outage, through an -# accepted value +# hooks.d/after_save/50-git-backup.sh — #258's outage, through an accepted value # ═════════════════════════════════════════════════════════════════════════════ from tests.test_git_backup_hook import hook_state @@ -327,9 +331,9 @@ def test_a_future_dated_backup_marker_does_not_stop_the_backup(tmp_path): """#258's original outage, reached through a value the guard ACCEPTS. `exit 0` at the cooldown sits above every path that stamps the marker - (`_gb_stamp_cooldown`, :596/:599), so once it is ahead of now the store is - never committed again — the git history simply stops, and the only trace is - the absence of commits. + (`_gb_stamp_cooldown` and both its call sites), so once it is ahead of now + the store is never committed again — the git history simply stops, and the + only trace is the absence of commits. """ home, remember, _remote, slug_dir, project = _store(tmp_path) marker = hook_state(remember, ".last-git-backup-ts", create_dir=True)