fix(#322): save-session.sh's two cooldown markers reach $(( )) unvalidated, and a digits-only guard still lets octal through

CHANGELOG.md

@@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Two cooldown markers were read straight into `$(( ))`, where bash evaluates file content as an arithmetic expression** ([#322](https://github.com/Digital-Process-Tools/claude-remember/issues/322)) — `tmp/last-save-ts` and `tmp/last-ndc.ts`. [#258](https://github.com/Digital-Process-Tools/claude-remember/issues/258) fixed this exact read in `50-git-backup.sh`; these two were missed. A marker holding `1786158837;` is a syntax error rather than a bad number, and bash's response is to abandon the **entire if/then body** and resume after `fi` — so `[ "$ELAPSED" -lt … ]` is never reached, `RUN_NDC=false` never runs, and the save skips its cooldown with one stderr line as the only trace.
+
+  **Smaller than it first looks, and the issue says so.** `date +%s > "$COOLDOWN_MARKER"` sits below the `fi` and `date +%s > "$NDC_MARKER"` runs on precisely the path a corrupt marker allows, so both gates **self-heal**: the cost is one skipped cooldown per corruption event, not a throttle stuck off. Nothing dies either — `set -e` plays no part, and `$ELAPSED` lives inside the abandoned body, so the `set -u` in `50-git-backup.sh` never sees an unbound variable there. The issue was filed claiming both, and both are wrong; the corrected mechanism is measured on bash 3.2.57 and 5.2.37.
+
+  **`case` and `10#`, not one or the other.** `08` and `09` are all digits, so they clear a digits-only guard and are then read as octal (`value too great for base`) — the identical abandonment from a marker that looks clean. Only reachable through a corrupt marker, which is the premise. `10#` goes after the guard, never instead of it, since `10#` on an empty string is itself an error on bash 5. The guard also rejects a space-padded value that arithmetic would have accepted; deliberate, and the same call #258 already made.
+
+  What this buys is narrow and worth stating plainly: an unexplained diagnostic stops appearing in `hook-errors.log` (visible to users since [#277](https://github.com/Digital-Process-Tools/claude-remember/issues/277)), and unvalidated file content stops reaching an arithmetic evaluator — the sink BashPitfalls #7 names, and the reason `case` was the answer in #258. ShellCheck does not flag this class at all, even with `-o all`; the standing request is koalaman/shellcheck#2679, open and unassigned since 2023.
+
+  Pinned by `tests/test_save_session_marker_arithmetic_322.py`, which asserts on the diagnostic rather than on whether a save happened — falling back to `0` makes a corrupt marker read as "very old", so corrupt and clean-but-ancient reach the same decision and only the stderr separates broken from fixed.
+
 - **The marker scan's list of field names was assumed exhaustive, and the assumption failed silently** ([#320](https://github.com/Digital-Process-Tools/claude-remember/issues/320)) — [#318](https://github.com/Digital-Process-Tools/claude-remember/issues/318) narrowed `_failure_haystack` to the fields the CLI itself authors (`error` / `result` / `message`, the `errors` list, stderr) and kept the raw-stdout fallback behind `if not authored`. That guard needs **every** recognised field to be empty. So a terminal record reporting an auth failure in some field this does not read, while a field it does read holds something benign, is scanned, found clean, and reported as "not an isolation problem" — the un-isolated retry never runs, capture fails permanently, and nothing says why. [#316](https://github.com/Digital-Process-Tools/claude-remember/issues/316)'s outage exactly, reached through the field set instead of through the spelling list.
 
   **Not a present defect, and it is fixed anyway.** Every input that reproduces it names a field the measured CLI does not emit. The point is that no test asserted the set was exhaustive and no comment recorded it as an assumption, so the day it stopped being true it would have stopped being true in silence — which is the one property this repo keeps paying for.

scripts/save-session.sh

@@ -140,7 +140,36 @@ fi
 [ "$FORCE" = true ] && log "force" "bypassing cooldown + min msgs"
 if [ -f "$COOLDOWN_MARKER" ] && [ "$DRY_RUN" != true ] && [ "$FORCE" != true ]; then
     LAST_MOD=$(cat "$COOLDOWN_MARKER" 2>/dev/null || echo 0)
-    ELAPSED=$(( $(date +%s) - LAST_MOD ))
+    # Unvalidated file content inside $(( )) is evaluated as an ARITHMETIC
+    # EXPRESSION, so one stray byte is a syntax error, not a bad number. Same
+    # read, same guard, as 50-git-backup.sh's cooldown marker (#258).
+    #
+    # What it costs, measured on bash 3.2.57 and 5.2.37: bash abandons the
+    # ENTIRE if/then body and resumes after `fi`. So the `[ "$ELAPSED" -lt ... ]`
+    # below is never reached, `exit 0` never runs, and this save skips its
+    # cooldown -- one stderr line the only trace. Nothing dies: `set -e` is not
+    # involved, and neither is `set -u` (which 50-git-backup.sh sets and this
+    # 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
+    # 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.
+    #
+    # 10# is for "08"/"09": all-digits, so they pass the case, and are then read
+    # as octal ("value too great for base"). Reachable only via a corrupt
+    # marker -- which is this guard's entire premise. It goes after the case,
+    # never instead of it: 10# on an empty string is itself an error on bash 5.
+    #
+    # The case also rejects a marker padded with spaces, which arithmetic would
+    # have accepted. Deliberate, and the same call #258's guard makes.
+    case "$LAST_MOD" in
+        ''|*[!0-9]*) LAST_MOD=0 ;;
+    esac
+    ELAPSED=$(( $(date +%s) - 10#$LAST_MOD ))
     SAVE_COOLDOWN=$(config ".cooldowns.save_seconds" 120)
     if [ "$ELAPSED" -lt "$SAVE_COOLDOWN" ]; then
         debug_enabled 1 && log "cooldown" "${ELAPSED}s < ${SAVE_COOLDOWN}s, skip"
@@ -562,9 +591,17 @@ 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.
     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) - NDC_MOD )) -lt "$NDC_COOLDOWN" ] && RUN_NDC=false
+    [ $(( $(date +%s) - 10#$NDC_MOD )) -lt "$NDC_COOLDOWN" ] && RUN_NDC=false
 fi
 
 # The day the content belongs to, not the day this run happens to fall on.

tests/test_save_session_marker_arithmetic_322.py

@@ -0,0 +1,203 @@
+"""save-session.sh's two cooldown markers are unvalidated data in `$(( ))` (#322).
+
+#258 fixed this shape in `50-git-backup.sh`. `save-session.sh` reads two markers
+the same way and neither was guarded: `tmp/last-save-ts` at the save cooldown and
+`tmp/last-ndc.ts` at the NDC gate.
+
+Bash evaluates a bare variable's VALUE inside `$(( ))` as an arithmetic
+expression, so a stray byte is a syntax error. Bash then abandons the rest of the
+current command list and resumes at the NEXT LINE. Measured with `-e`, without
+it, and with `-u`: identical, so errexit plays no part in this. What separates
+the two files is the absence of `set -u` here —
+
+* `save-session.sh` (no `-u`) — `ELAPSED=$(( ... ))` is abandoned, `ELAPSED`
+  stays unset, and the `[ "$ELAPSED" -lt ... ]` on the next line complains and is
+  false. This run skips its cooldown.
+* `50-git-backup.sh` (`-u`) — the next line's `$ELAPSED` is an unbound variable
+  and the shell exits, which is the permanent stop #258 documented.
+
+Scope it honestly: both gates rewrite their marker on exactly the path a corrupt
+one allows, so each SELF-HEALS. The cost is one skipped cooldown per corruption
+event, not a throttle stuck off. These tests therefore pin the diagnostic rather
+than a missed save — falling back to 0 makes a corrupt marker read as "very old",
+so corrupt and clean-but-ancient produce the same decision, and the stderr is the
+only thing that separates broken from fixed.
+
+`"08"`/`"09"` are here because a digits-only guard is not enough on its own: they
+pass it and are then read as octal (`value too great for base`), failing
+identically. That is why the guard carries `10#` — and why the same `10#` is
+added to #258's existing guard in `50-git-backup.sh`, where that shape is not a
+skipped cooldown but the unbound-variable exit above.
+
+CONTROL: `  1785512249  ` is in the list on purpose and must stay GREEN against
+the unfixed script. Arithmetic skips surrounding whitespace, so that marker works
+today -- and the guard now rejects it and falls back to 0. That is a real
+behaviour change, deliberate, and the same call #258's guard already makes; the
+parameter documents it rather than hiding it.
+"""
+
+from __future__ import annotations
+
+import json
+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  # noqa: E402
+from test_save_session_gates import _make_env, _run  # noqa: E402
+
+# Shapes that genuinely reach the evaluator as something other than a plain
+# decimal integer. "08"/"09" are the octal pair a digits-only guard lets through.
+TRIPS_THE_BUG = ["12x34", "1786158837;", "08", "09"]
+
+# Controls — see the module docstring. These must be green against the UNFIXED
+# script too, so they assert the fix is not doing something it was not asked to.
+# `  1785512249  ` additionally documents a real behaviour change: arithmetic
+# accepts padded digits today, and after the guard it reads as 0. Deliberate, and
+# the same call #258's guard already makes.
+# `garbage-text` is deliberately NOT here: it parses as `garbage - text`, two
+# unset identifiers that are both 0 without `set -u`, so it produces no
+# diagnostic and the fix does not change its behaviour at all. A parameter that
+# cannot fail either way is not a control, it is decoration.
+CONTROLS = ["  1785512249  "]
+
+CORRUPT_MARKERS = TRIPS_THE_BUG + CONTROLS
+
+# bash's own wording for the two ways this shows up, plus the knock-on `test`
+# error the save gate emits once ELAPSED has been left unset.
+ARITHMETIC_DIAGNOSTICS = (
+    "syntax error",
+    "invalid arithmetic operator",
+    "value too great for base",
+    # NOT "integer expression expected": the whole if/then body is abandoned, so
+    # the `[ "$ELAPSED" -lt ... ]` that would print it is never reached. Listing
+    # a phrase the code cannot emit reads as coverage and is not.
+    #
+    # "invalid integer constant" is what bash 5 says for `10#` on an empty
+    # string, i.e. the failure mode if a later change keeps 10# but drops the
+    # case guard in front of it.
+    "invalid integer constant",
+)
+
+
+def _set_cooldowns(env: dict, plugin: Path, *, save_seconds: int,
+                   ndc_seconds: int) -> None:
+    """Rewrite the config the script reads. _make_env pins save_seconds to 0,
+    which retires the very gate these tests need to enter.
+
+    Both copies, deliberately: _make_env writes the same document to
+    $REMEMBER_CONFIG and to <plugin>/config.json, and updating only the first
+    leaves save_seconds at 0 with no visible error — the run just sails through
+    the gate and the test reads as a product bug. Caught exactly that way.
+    """
+    cfg_path = Path(env["REMEMBER_CONFIG"])
+    cfg = json.loads(cfg_path.read_text())
+    cfg["cooldowns"] = {"save_seconds": save_seconds, "ndc_seconds": ndc_seconds}
+    for target in (cfg_path, plugin / "config.json"):
+        target.write_text(json.dumps(cfg))
+
+
+def _assert_no_arithmetic_error(proc, project: Path, marker_name: str) -> None:
+    log = project / ".remember" / "logs" / "hook-errors.log"
+    haystack = proc.stderr + (log.read_text(encoding="utf-8", errors="replace")
+                              if log.is_file() else "")
+    for phrase in ARITHMETIC_DIAGNOSTICS:
+        assert phrase not in haystack, (
+            f"a corrupt {marker_name} reached $(( )) unvalidated: bash reported "
+            f"{phrase!r}. The command list is abandoned at that point, so the "
+            f"cooldown it guards is silently skipped.\n--- stderr/log ---\n"
+            f"{haystack.strip()}"
+        )
+
+
+@pytest.mark.parametrize("corrupt", CORRUPT_MARKERS)
+def test_a_corrupt_save_cooldown_marker_does_not_break_the_gate(tmp_path, corrupt):
+    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(
+        corrupt, encoding="utf-8", errors="ignore")
+
+    proc = _run(plugin, env, session_id)
+
+    assert proc.returncode == 0, subprocess_failure_detail(
+        proc, project / ".remember")
+    _assert_no_arithmetic_error(proc, project, "tmp/last-save-ts")
+
+
+@pytest.mark.parametrize("corrupt", CORRUPT_MARKERS)
+def test_a_corrupt_ndc_marker_does_not_break_the_gate(tmp_path, corrupt):
+    env, project, plugin, _calls, session_id = _make_env(
+        tmp_path, exchanges=40, humans=5)
+    # Without this the stub's call-haiku returns SKIP, the run exits at Step 6,
+    # and Step 8 — the gate under test — is never reached. The test then passes
+    # against the unfixed script and proves nothing. Verified by reverting the
+    # fix and watching it stay green until this line was added.
+    env["STUB_HAIKU_TEXT"] = "## 12:00 | main\n\n- an entry\n"
+    _set_cooldowns(env, plugin, save_seconds=0, ndc_seconds=999999)
+    (project / ".remember" / "tmp" / "last-ndc.ts").write_text(
+        corrupt, encoding="utf-8", errors="ignore")
+
+    proc = _run(plugin, env, session_id)
+
+    assert proc.returncode == 0, subprocess_failure_detail(
+        proc, project / ".remember")
+    _assert_no_arithmetic_error(proc, project, "tmp/last-ndc.ts")
+
+
+def test_a_valid_recent_marker_still_throttles(tmp_path):
+    """The guard must not pass by turning the cooldown off for everyone.
+
+    A marker written now, against a 3600s window, has to skip the run — so the
+    fallback-to-0 path is reached only by markers that genuinely do not parse.
+    """
+    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, (
+        "a marker written seconds ago did not engage the save cooldown — the "
+        f"guard has disabled the throttle rather than protected it. calls: {ran!r}"
+    )
+
+
+def test_a_leading_zero_marker_is_read_as_decimal(tmp_path):
+    """`10#`, specifically. "08" passes a digits-only guard and is then octal.
+
+    Pinned separately from the parametrized cases because it is the one shape
+    that survives the fix #258 shipped, and a future simplification back to a
+    bare `case` would leave every other case above green.
+    """
+    env, project, plugin, calls, session_id = _make_env(
+        tmp_path, exchanges=40, humans=5)
+    _set_cooldowns(env, plugin, save_seconds=3600, ndc_seconds=999999)
+    # Interpreted as octal this is an error; as decimal it is 8 seconds past the
+    # epoch, i.e. far outside the window, so the run proceeds.
+    (project / ".remember" / "tmp" / "last-save-ts").write_text("08")
+
+    proc = _run(plugin, env, session_id)
+
+    assert proc.returncode == 0, subprocess_failure_detail(
+        proc, project / ".remember")
+    _assert_no_arithmetic_error(proc, project, "tmp/last-save-ts")
+    ran = calls.read_text() if calls.is_file() else ""
+    assert "extract" in ran, (
+        "a marker of \"08\" should read as 8 seconds past the epoch — ancient, so "
+        f"the cooldown is long expired and the save proceeds. calls: {ran!r}"
+    )