feat: periodic update check with TTL cache and auto-snooze#24
Merged
Conversation
Adds a UserPromptSubmit hook that checks for newer trimkit versions once per 24 hours. Fetches package.json from the main branch on GitHub, compares against the local version, and prints a one-line notice when an update is available. Auto-snoozes per-version for 7 days after notifying; silent on fetch failures. New files: - package.json (version source of truth) - bin/trimkit-update-check (hook script: TTL cache, fetch, compare, snooze) - bin/trimkit-update-snooze (writes snoozed-version + snoozed-until) - tests/bin/trimkit-update-check.bats - tests/bin/trimkit-update-snooze.bats Modified: - settings/hooks.json: add UserPromptSubmit hook entry - install.sh: write ~/.trimkit/install-dir so the check can find package.json - .github/workflows/test.yml: include tests/bin/ in BATS run - CHANGELOG.md: document the new feature State files live under ~/.trimkit/update-check/. All env vars are overrideable for testing (TRIMKIT_UPDATE_STATE_DIR, TRIMKIT_UPDATE_INSTALL_DIR, TRIMKIT_UPDATE_REMOTE_URL, TRIMKIT_UPDATE_TTL, TRIMKIT_UPDATE_SNOOZE_DAYS). Closes #8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
install.sh (1)
184-186: The warning vanishes into the void instead of joining the summary.Every other failure in this script gets parked in
warned+=(...)and displayed in the lovely "=== TrimKit install ===" summary at the end. This particular warning, however, is sent straight to stderr where the user is most likely to scroll past it and never see it. A trifle, but inconsistent.♻️ Possible tidy-up
-printf '%s\n' "$SCRIPT_DIR" > "$HOME/.trimkit/install-dir.tmp" \ - && mv "$HOME/.trimkit/install-dir.tmp" "$HOME/.trimkit/install-dir" \ - || echo "Warning: could not write ~/.trimkit/install-dir — update checks will be skipped" >&2 +if ! { printf '%s\n' "$SCRIPT_DIR" > "$HOME/.trimkit/install-dir.tmp" \ + && mv "$HOME/.trimkit/install-dir.tmp" "$HOME/.trimkit/install-dir"; }; then + warned+=("install-dir (could not write ~/.trimkit/install-dir — update checks will be skipped)") +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 184 - 186, The failure to write "$HOME/.trimkit/install-dir" is currently echoed directly to stderr instead of being recorded in the script's warnings summary; change the error path so that when the mv (or write) fails you append the same warning message into the warned+=(...) array used by the installer summary (use the existing warned variable), referencing the SCRIPT_DIR/ "$HOME/.trimkit/install-dir" operation and ensure the message text matches other entries so it will appear in the "=== TrimKit install ===" summary at the end rather than going straight to stderr.package.json (1)
1-5: A whole manifest, just to hold a version string. How thrilling.Mind, since this file is now the local source of truth that
trimkit-update-checkparses, you might eventually wantrepository,license, andhomepagefields here too — purely so future tooling has somewhere to look without me having to do all the thinking. Optional; ignore me, everyone usually does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 1 - 5, The package.json only contains name/version/description; add standard manifest fields so tooling can find metadata: include "repository" (URL and type), "license" (SPDX identifier), and "homepage" (project URL) entries alongside the existing "name", "version", and "description" so trimkit-update-check and other tools can read project metadata; update the package.json top-level object (the same file containing "name"/"version"/"description") to add these keys with appropriate values for this repo.bin/trimkit-update-check (2)
36-43: Style nit:TRIMKIT_UPDATE_INSTALL_DIRis read inside the function while the other env-var defaults are at the top.
STATE_DIR,REMOTE_URL, andTTLare resolved as global defaults at the top of the script, butTRIMKIT_UPDATE_INSTALL_DIRis consulted from insiderun_check. They're conceptually the same kind of override; collecting them in one place would make the configuration surface obvious at a glance. A purely cosmetic complaint, befitting my station.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/trimkit-update-check` around lines 36 - 43, Move the TRIMKIT_UPDATE_INSTALL_DIR resolution out of run_check and define it alongside the other global defaults (STATE_DIR, REMOTE_URL, TTL) at the top of the script so all overrides are collected in one place; create a single variable (e.g., INSTALL_DIR or reuse TRIMKIT_UPDATE_INSTALL_DIR after defaulting) there by checking ${TRIMKIT_UPDATE_INSTALL_DIR:-fallback} or reading ~/.trimkit/install-dir if present, then update run_check to use that pre-computed variable (referenced where install_dir is currently set) instead of reading the env/FS inside the function.
30-119: Up to fivepython3fork/exec cycles on every prompt. The hook tax adds up.On a warm-cache run (the common case), the script still spawns python3 at least three times: once to parse the local
package.json, once for the TTL check, and once for the version comparison. On a fresh-fetch run it's five. PerUserPromptSubmit. It works, and on a modern machine it's a few tens of ms — but a single Python interpreter invocation could do all of it, and since this hook fires on every prompt, it's not absurd to consolidate. Optional; the universe is meaningless either way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/trimkit-update-check` around lines 30 - 119, The run_check function spawns python3 multiple times (for reading package.json version, TTL check, computing now, parsing remote JSON, and version comparison); consolidate these into a single python3 invocation that (given argv: state_dir, ttl, pkg_json, remote_json_path/URL contents) reads package.json to extract local_version, reads/validates last-checked and computes ttl_expired and now, parses remote_json to extract latest_version if provided, performs the tuple version comparison and prints or returns structured results (e.g. lines: local_version, latest_version, now, is_newer) so the shell can read them into local variables; replace the separate calls that set local_version, ttl_expired/now, latest_version, and is_newer with a single call to that script and parse its stdout to set those variables (update use-sites of local_version, latest_version, ttl_expired, now, is_newer accordingly).bin/trimkit-update-snooze (1)
31-36: No sanity check onSNOOZE_DAYS. Negative numbers would yield a snooze-until in the past.
int(sys.argv[1])happily accepts-7or0, producing asnoozed-untilthat's already expired the instant it's written. Not exploitable, just mildly absurd. Amax(0, …)or a numeric/positive guard wouldn't go amiss, but feel free to leave it — life is suffering anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/trimkit-update-snooze` around lines 31 - 36, Validate and clamp SNOOZE_DAYS so negative or zero values cannot produce a past snooze-until: ensure the value passed into the python snippet is converted to an integer and bounded (e.g. days = max(0, int(...)) or check and exit on negative) before computing snooze_until; update the python snippet (or the shell wrapper that sets SNOOZE_DAYS) to use a non-negative guard and keep the produced timestamp computation in the existing block (refer to the snooze_until variable and the int(sys.argv[1]) conversion in the inline python).tests/bin/trimkit-update-snooze.bats (1)
57-62: Bash variables interpolated directly into Python source. Works, but tempts fate.
$FAKE_STATE_DIRis glued into a Python string literal via shell expansion. Withmktemp -dit's perfectly safe today, but a path containing a single quote or backslash would silently produce broken Python. Passing the path throughsys.argvis the conventional, less-fragile option.♻️ Suggested tidy-up (illustrative, applies to all three tests)
- python3 -c " -import re -ts = open('$FAKE_STATE_DIR/snoozed-until').read().strip() -assert re.match(r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$', ts), f'bad format: {ts}' -" + python3 -c ' +import re, sys +ts = open(sys.argv[1]).read().strip() +assert re.match(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$", ts), f"bad format: {ts}" +' "$FAKE_STATE_DIR/snoozed-until"Also applies to: 66-73, 79-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/trimkit-update-snooze.bats` around lines 57 - 62, Replace direct shell interpolation of $FAKE_STATE_DIR into the inline Python string with passing the file path as a command-line argument to Python and reading sys.argv[1]; specifically, update the python3 -c invocation that currently opens '$FAKE_STATE_DIR/snoozed-until' to instead import sys and re, open(sys.argv[1]) for the timestamp check, and pass "$FAKE_STATE_DIR/snoozed-until" after the -c script. Apply the same change to the other two occurrences referenced (the blocks covering lines 66-73 and 79-85) so no shell variable is embedded inside the Python literal.tests/bin/trimkit-update-check.bats (2)
218-228: First invocation runs bare-back withoutrun— output drifts off into the void.Line 222 invokes
bash "$SCRIPT"directly rather than viarun, so any stdout/stderr from the priming run is emitted into the test stream rather than captured. It happens to work today because the secondrunis whatassert_outputinspects, but if the first call ever starts grumbling (a strayprintfto stderr, apython3deprecation notice), you'll get mysteriously noisy CI logs with nothing to assert against. A trivial bit of hygiene goes a long way.🧹 Proposed tidy-up
- # First run: notice shown, snooze written - bash "$SCRIPT" + # First run: notice shown, snooze written (output discarded) + run bash "$SCRIPT" + assert_success🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/trimkit-update-check.bats` around lines 218 - 228, The initial priming invocation uses bash "$SCRIPT" directly which lets its stdout/stderr leak into the test harness; change that to use the bats helper run so output is captured and assertions remain reliable. Replace the first bare call bash "$SCRIPT" in the "second run is silent after auto-snooze is written" test with run bash "$SCRIPT" (keep the subsequent run bash "$SCRIPT" for the second invocation) so assert_success and assert_output '' inspect the captured output; references: MOCK_REMOTE_JSON, run, assert_output, and the script invocation bash "$SCRIPT".
28-37: Mockcurlrecords the call before honouringMOCK_CURL_EXIT— worth a passing thought.Counts a "call" even when the mock is configured to exit non-zero, which is presumably what you want (the real
curlwas invoked, after all). None of the failure-path tests assert on the call count, so this is purely a heads-up: if a future test ever asserts "no curl call on failure", it will be quietly wrong. Document the intent or move theprintfafter the exit gate, depending on which behaviour you actually want to lock in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/trimkit-update-check.bats` around lines 28 - 37, The mock curl currently writes to ${MOCK_CURL_CALLS} before checking ${MOCK_CURL_EXIT}, so failed-exit runs are still counted; update the mock curl here-doc (the script written into "$MOCK_BIN/curl") to check ${MOCK_CURL_EXIT:-0} and exit before calling printf, or move the printf '1\n' >> "${MOCK_CURL_CALLS}" to after the exit gate, ensuring the mock honors MOCK_CURL_EXIT by not recording calls when configured to exit non-zero (keep references to MOCK_CURL_CALLS, MOCK_CURL_EXIT, and MOCK_CURL_RESPONSE intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/trimkit-update-check`:
- Around line 136-137: The printf call that prints the suggested command uses
the raw install_dir variable which breaks when the path contains spaces; update
the printf format to shell-quote the path (use printf's %q for the install_dir
argument) or ensure the install_dir is wrapped and escaped (e.g., quoted) in the
printed command so that the generated "cd …; git pull; ./install.sh" works even
for paths with spaces; adjust the printf invocation that references
"$latest_version" "$local_version" "$install_dir" accordingly.
- Around line 107-119: The version parser currently in the inline Python (parse
function used to set is_newer) strips non-numeric suffixes to 0 so pre-release
tags like "-rc.1" are lost and release vs pre-release compare equal; update the
Python logic in that block to (1) parse the numeric major/minor/patch tuple as
before, (2) separately detect any pre-release suffix (e.g., anything after the
first non-numeric segment in the version string) and represent it as a boolean
or comparable token, and (3) when printing '1' vs '0' prefer the tuple
comparison first and if tuples are equal treat a version with a pre-release
suffix as less than a clean release (i.e., local "1.0.0-rc" < remote "1.0.0"
should yield is_newer=1), ensuring the variable is_newer logic and the
surrounding shell checks remain unchanged.
In `@bin/trimkit-update-snooze`:
- Around line 18-24: The current argument check [ $# -lt 1 ] allows an empty
string like trimkit-update-snooze "" to proceed and write an empty
snoozed-version; update the guard in the script to also reject empty or
all-whitespace version values by testing the actual parameter (e.g., check -z
after trimming or use [ -z "${1}" ] or a POSIX-safe test) and, if empty, print
the same usage/error messages and exit 1; modify the logic around the
version="$1" assignment to only set version when the check passes so
functions/variables like version and the snoozed-version write path never
receive an empty string.
In `@tests/bin/trimkit-update-check.bats`:
- Around line 5-48: The setup() function currently sets FAKE_HOME and redirects
TRIMKIT_UPDATE_STATE_DIR and TRIMKIT_UPDATE_INSTALL_DIR but doesn't change HOME,
so tests that unset TRIMKIT_* vars still read from the real user's ${HOME}; to
fix, export HOME="$FAKE_HOME" inside setup() (alongside FAKE_HOME,
FAKE_STATE_DIR, FAKE_INSTALL_DIR, etc.) so all fallback paths (used by
TRIMKIT_UPDATE_STATE_DIR and TRIMKIT_UPDATE_INSTALL_DIR) resolve into the test
sandbox.
---
Nitpick comments:
In `@bin/trimkit-update-check`:
- Around line 36-43: Move the TRIMKIT_UPDATE_INSTALL_DIR resolution out of
run_check and define it alongside the other global defaults (STATE_DIR,
REMOTE_URL, TTL) at the top of the script so all overrides are collected in one
place; create a single variable (e.g., INSTALL_DIR or reuse
TRIMKIT_UPDATE_INSTALL_DIR after defaulting) there by checking
${TRIMKIT_UPDATE_INSTALL_DIR:-fallback} or reading ~/.trimkit/install-dir if
present, then update run_check to use that pre-computed variable (referenced
where install_dir is currently set) instead of reading the env/FS inside the
function.
- Around line 30-119: The run_check function spawns python3 multiple times (for
reading package.json version, TTL check, computing now, parsing remote JSON, and
version comparison); consolidate these into a single python3 invocation that
(given argv: state_dir, ttl, pkg_json, remote_json_path/URL contents) reads
package.json to extract local_version, reads/validates last-checked and computes
ttl_expired and now, parses remote_json to extract latest_version if provided,
performs the tuple version comparison and prints or returns structured results
(e.g. lines: local_version, latest_version, now, is_newer) so the shell can read
them into local variables; replace the separate calls that set local_version,
ttl_expired/now, latest_version, and is_newer with a single call to that script
and parse its stdout to set those variables (update use-sites of local_version,
latest_version, ttl_expired, now, is_newer accordingly).
In `@bin/trimkit-update-snooze`:
- Around line 31-36: Validate and clamp SNOOZE_DAYS so negative or zero values
cannot produce a past snooze-until: ensure the value passed into the python
snippet is converted to an integer and bounded (e.g. days = max(0, int(...)) or
check and exit on negative) before computing snooze_until; update the python
snippet (or the shell wrapper that sets SNOOZE_DAYS) to use a non-negative guard
and keep the produced timestamp computation in the existing block (refer to the
snooze_until variable and the int(sys.argv[1]) conversion in the inline python).
In `@install.sh`:
- Around line 184-186: The failure to write "$HOME/.trimkit/install-dir" is
currently echoed directly to stderr instead of being recorded in the script's
warnings summary; change the error path so that when the mv (or write) fails you
append the same warning message into the warned+=(...) array used by the
installer summary (use the existing warned variable), referencing the
SCRIPT_DIR/ "$HOME/.trimkit/install-dir" operation and ensure the message text
matches other entries so it will appear in the "=== TrimKit install ===" summary
at the end rather than going straight to stderr.
In `@package.json`:
- Around line 1-5: The package.json only contains name/version/description; add
standard manifest fields so tooling can find metadata: include "repository" (URL
and type), "license" (SPDX identifier), and "homepage" (project URL) entries
alongside the existing "name", "version", and "description" so
trimkit-update-check and other tools can read project metadata; update the
package.json top-level object (the same file containing
"name"/"version"/"description") to add these keys with appropriate values for
this repo.
In `@tests/bin/trimkit-update-check.bats`:
- Around line 218-228: The initial priming invocation uses bash "$SCRIPT"
directly which lets its stdout/stderr leak into the test harness; change that to
use the bats helper run so output is captured and assertions remain reliable.
Replace the first bare call bash "$SCRIPT" in the "second run is silent after
auto-snooze is written" test with run bash "$SCRIPT" (keep the subsequent run
bash "$SCRIPT" for the second invocation) so assert_success and assert_output ''
inspect the captured output; references: MOCK_REMOTE_JSON, run, assert_output,
and the script invocation bash "$SCRIPT".
- Around line 28-37: The mock curl currently writes to ${MOCK_CURL_CALLS} before
checking ${MOCK_CURL_EXIT}, so failed-exit runs are still counted; update the
mock curl here-doc (the script written into "$MOCK_BIN/curl") to check
${MOCK_CURL_EXIT:-0} and exit before calling printf, or move the printf '1\n' >>
"${MOCK_CURL_CALLS}" to after the exit gate, ensuring the mock honors
MOCK_CURL_EXIT by not recording calls when configured to exit non-zero (keep
references to MOCK_CURL_CALLS, MOCK_CURL_EXIT, and MOCK_CURL_RESPONSE intact).
In `@tests/bin/trimkit-update-snooze.bats`:
- Around line 57-62: Replace direct shell interpolation of $FAKE_STATE_DIR into
the inline Python string with passing the file path as a command-line argument
to Python and reading sys.argv[1]; specifically, update the python3 -c
invocation that currently opens '$FAKE_STATE_DIR/snoozed-until' to instead
import sys and re, open(sys.argv[1]) for the timestamp check, and pass
"$FAKE_STATE_DIR/snoozed-until" after the -c script. Apply the same change to
the other two occurrences referenced (the blocks covering lines 66-73 and 79-85)
so no shell variable is embedded inside the Python literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f4b677f-957d-4e0a-af29-1c2edefbee3f
📒 Files selected for processing (9)
.github/workflows/test.ymlCHANGELOG.mdbin/trimkit-update-checkbin/trimkit-update-snoozeinstall.shpackage.jsonsettings/hooks.jsontests/bin/trimkit-update-check.batstests/bin/trimkit-update-snooze.bats
- bin/trimkit-update-check: use %q to shell-quote install_dir in notice so copy-paste works for paths with spaces - bin/trimkit-update-check: improve version parser with 4-tuple (major, minor, patch, is_release) so pre-release versions like 1.0.0-rc.1 correctly sort below their corresponding release - bin/trimkit-update-snooze: reject empty version argument - bin/trimkit-update-snooze: clamp SNOOZE_DAYS to min 1 day so a negative/zero value doesn't produce an already-expired snooze - install.sh: route install-dir write failure into warned+=() so it appears in the "=== TrimKit install ===" summary instead of going directly to stderr - package.json: add repository, homepage, license fields - tests: export HOME="$FAKE_HOME" in setup so HOME-based fallback paths resolve into the sandbox - tests: use run bash "$SCRIPT" for the priming call in the "second run" test to keep the harness clean - tests: move mock curl call counter after exit gate so only successful fetches count - tests: pass FAKE_STATE_DIR as sys.argv[1] in inline Python assertions to avoid shell interpolation into Python source
Owner
Author
|
Thanks for the thorough review, CodeRabbit! Addressed in fb04bf1:
Declined (with reasoning):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8
Summary
bin/trimkit-update-check— aUserPromptSubmithook that fetchespackage.jsonfrom the main branch on GitHub at most once per 24 hours, compares against the locally installed version, and prints a one-line notice when an update is availablebin/trimkit-update-snooze— writessnoozed-version+snoozed-untilto suppress re-notification for 7 days per version; called automatically bytrimkit-update-checkafter showing a noticepackage.jsonas the version source of truth (v0.5.0)install.shto write~/.trimkit/install-dirso the check can locate the localpackage.jsonwithout symlink resolutionUserPromptSubmithook entry intosettings/hooks.jsontests/bin/Behaviour
last-checkedto back off for the full TTL rather than retrying on every messageState files
All paths and durations are overrideable via env vars for testing.
Test plan
Summary by CodeRabbit
Release Notes
New Features
Tests