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

[jmossie82]

Closes #322

scripts/save-session.sh reads two cooldown markers straight into $(( )), where bash evaluates file content as an arithmetic expression. #258 fixed this exact read in 50-git-backup.sh; these two were missed:

• tmp/last-save-ts at the save gate (:142)
• tmp/last-ndc.ts at the NDC gate (:565)

A correction to my own issue, up front

I filed #322 claiming the script dies and that compression then runs on every save. Both are wrong, and the issue body and title now carry the correction. Measured on /bin/bash 3.2.57 and 5.2.37:

enter then-block
t.sh: line 7: 08: value too great for base (error token is "08")
AFTER_FI                     <-- the statements between are all skipped

Bash abandons the entire if/then body and resumes after fi. So:

• Nothing dies. set -e plays no part — the behaviour is identical with -e, without it, and with -u.
• One diagnostic, not two. [ "$ELAPSED" -lt … ] is never reached, so integer expression expected is never emitted.
• Both gates self-heal. date +%s > "$COOLDOWN_MARKER" is below the fi (:158) and date +%s > "$NDC_MARKER" (:585) runs on precisely the path a corrupt marker allows.

So the real cost is one skipped cooldown per corruption event, plus one stderr line — then it repairs itself. That is a good deal smaller than the issue claimed, and I would rather say so here than have you find it.

This also corrects something I wrote about your file. I said the 08 case in 50-git-backup.sh was #258's death mode. It is not: $ELAPSED sits inside the abandoned body, so set -u never sees an unbound variable there either. Against the real hook with a corrupt marker: rc=0, backup proceeds, commit lands. The consequence is the same in both files.

Why this is still worth fixing

Narrow, and I will not overstate it:

1. Unvalidated file content should not reach an arithmetic evaluator. BashPitfalls #7 covers this read and recommends exactly the case $foo in ("" | *[!0123456789]*) form you already chose for Cooldown and lock robustness in 50-git-backup.sh: a corrupt marker bypasses the throttle, and the marker is only written on success #258 — this change just applies it to the two places that were missed.
2. It removes an unexplained diagnostic from hook-errors.log, which users can see since dispatch discards a dying hook's stderr and reports it as a nameless hook failed, so a fatal hook error reaches nobody #277.
3. ShellCheck does not detect this class at all — verified at 0.11.0 including -o all, zero findings. The standing request is Warn about the use of unvalidated input in arithmetic contexts. koalaman/shellcheck#2679, open and unassigned since 2023-02-01. So a guard in the source is the only thing that catches it.

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.

case "$LAST_MOD" in ''|*[!0-9]*) LAST_MOD=0 ;; esac
ELAPSED=$(( $(date +%s) - 10#$LAST_MOD ))

10# goes after the guard and never instead of it: 10# on an empty string is itself an error on bash 5 (silently 0 on 3.2).

If the objection is that 10# is unreachable because no writer emits a leading zero — that is true, and it is equally true of ;, which is the byte that started this. Neither shape is writer-emitted; both arrive only through a corrupt marker, which is the premise #258 already accepted. I would rather the guard be uniform than draw a line between two kinds of corruption on the grounds that only one of them has been seen.

A marker that worked and now reads as 0

Disclosing a real behaviour change: " 1785512249 " is not corrupt today. Arithmetic skips surrounding whitespace, so it currently computes correctly and the throttle works. After this change the case rejects it and it falls back to 0.

Deliberate, and the same call your #258 guard already makes for that file. It is kept as a test parameter so the change is documented rather than discovered.

Tests

tests/test_save_session_marker_arithmetic_322.py, red before / green after.

RED — pristine scripts, this test file added:

9 failed, 3 passed

GREEN — with the patch: 12 passed. Full suite: 1517 passed, 43 skipped, coverage 94.27% against the 80% floor.

Two deliberate choices in that file:

• It asserts on the diagnostic, not 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 — the stderr is the only thing that separates broken from fixed.
• garbage-text is deliberately not a parameter. It parses as garbage - text, two unset identifiers that are both 0 without set -u, so it emits nothing and the fix does not change it. A parameter that cannot fail either way is decoration, not coverage. " 1785512249 " is kept, as the control documenting the change above.

"integer expression expected" is likewise absent from the asserted phrases, because the abandoned body means it can never be produced. "invalid integer constant" is there instead — that is what bash 5 says if a later change keeps 10# but drops the case in front of it.

Found in passing — filed as #324

Not in this PR; filed separately as #324 so it stands on its own evidence.

test_a_corrupt_cooldown_marker_does_not_kill_the_hook writes <store>/.last-git-backup-ts, but the hook resolves the marker through _gb_common_dir and reads <store>/.git/remember/last-git-backup-ts. I confirmed where the file actually lands by running a backup and globbing for it. The test has been feeding the script a decoy, so every corrupt value passes trivially and #258's regression is currently unguarded. The cleanup helper at the top of that file globs both names, which is the trace the path moved.

Pointing it at the real path is not quite enough on its own: with 08 the hook prints the octal error but still returns rc=0 and commits on bash 3.2.57, so an outcome-only assertion stays green on the macOS leg. Asserting on proc.stderr makes it fail on any version.

Patch for that one is ready whenever you want it — see #324.

Platform

Everything above measured on /bin/bash 3.2.57 (macOS, the shebang's interpreter) and cross-checked on 5.2.37. pytestmark = skipif(sys.platform == "win32", …#79) matches the neighbouring shell tests.

[Copilot]

Pull request overview

Hardens scripts/save-session.sh against corrupt cooldown marker contents being evaluated as bash arithmetic expressions, preventing avoidable diagnostics and ensuring cooldown logic behaves predictably. This aligns the save-session hook’s marker handling with the robustness work previously done around marker corruption.

Changes:

• Add case-based numeric validation and 10# decimal coercion when reading tmp/last-save-ts and tmp/last-ndc.ts.
• Add regression tests that assert the absence of bash arithmetic diagnostics for corrupt marker values.
• Document the fix and its impact in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
scripts/save-session.sh	Validates marker values before arithmetic evaluation and forces decimal parsing (10#) to prevent octal/parse errors.
tests/test_save_session_marker_arithmetic_322.py	Adds regression tests covering corrupt marker shapes and ensures no arithmetic diagnostics leak to stderr/log.
CHANGELOG.md	Documents the fix, impact, and rationale for the marker validation change.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[fdaviddpt]

Merged, and shipping in v0.17.0.

Thank you — genuinely. You filed the issue, you traced the mechanism, and then you sent the patch, which is not the usual order of events around here. A few things I want to name because they were better than they had to be:

• case and 10#, with the reasoning for why it is both and in that order. The 08/09 octal case is the one a digits-only guard quietly lets through, and most patches for this class stop at the digits check.
• You corrected your own issue in the patch. The filed text claimed set -e and set -u consequences; your comment says plainly that both are wrong and gives the measured behaviour instead. That is rarer than the fix.
• The test asserts the diagnostic rather than whether a save happened, and the comment explains why — falling back to 0 makes a corrupt marker read as "very old", so corrupt and clean-but-ancient reach the same decision. That reasoning is better than most of what is already in this suite.
• The note about why garbage-text is not a valid control stayed in, and it should have.

Your #324 is next — you are right that the regression test writes to a path the hook does not read, so that guard is currently asserting nothing.

Two follow-ups came out of auditing your change, both pre-existing and neither yours: #326 (the guards validate syntax but not range, so a future-dated marker sticks the throttle on) and #327 (10# landed here only; 50-git-backup.sh still takes the octal path). Both are credited to your patch for surfacing the class.

Unrelated, but you clearly enjoy this kind of thing: we also maintain claude-supertool, token-efficient batched ops for Claude Code sessions. Same house style — a checker that cannot answer has to say so rather than return a zero. You would probably find things in it. Issues and PRs very welcome.