feat(hooks): tune stop-hook cadence via env vars (MEMPAL_SAVE_INTERVAL / MEMPAL_STOP_HOOK_DISABLE)#985
Conversation
The stop hook currently blocks every 15 human messages with an "AUTO-SAVE checkpoint" prompt, hard-coded via the module-level ``SAVE_INTERVAL`` constant. Two new env vars let users tune this without reinstalling the plugin: - ``MEMPAL_SAVE_INTERVAL=N`` overrides the cadence. Invalid / empty values fall back to 15 so a typo can never drop the interval to 0 and block every single turn. Values <= 0 are clamped to 1. - ``MEMPAL_STOP_HOOK_DISABLE=1`` (or true/yes/on) turns the block off entirely — the hook still records state but never asks the model to save. Useful when MemPalace MCP tools are not registered in the session (e.g. the plugin is installed but not configured into the current project's MCP list), so the block would nag without any way for the model to satisfy it. Both read the env on every hook invocation, so users can change behavior mid-session without touching the plugin install. The public ``SAVE_INTERVAL`` constant is preserved (re-exports ``DEFAULT_SAVE_INTERVAL``) so any external code / tests that read it keep working. 22 new unit tests in tests/test_hooks_cli_tuning.py cover: - default when unset - numeric override + whitespace trimming - invalid strings fall back to default (no zero-interval hazard) - non-positive values clamped to 1 - truthy/falsy normalization for the disable flag - backwards-compatible constant export All tests pass.
There was a problem hiding this comment.
Pull request overview
Adds runtime-configurable tuning knobs for the MemPalace stop hook so users can adjust (or disable) the auto-save blocking cadence via environment variables without reinstalling.
Changes:
- Introduces
MEMPAL_SAVE_INTERVALparsing (with clamping/fallback) to override the default stop-hook blocking interval. - Introduces
MEMPAL_STOP_HOOK_DISABLEparsing to allow a pass-through “no blocking” mode. - Adds a new unit-test module covering env-var parsing helpers and backwards-compat constant export.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mempalace/hooks_cli.py |
Adds env-var parsing helpers and wires them into hook_stop decision logic and logging. |
tests/test_hooks_cli_tuning.py |
Adds unit tests for interval/disable env var parsing and compatibility constant behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Invalid / non-positive values silently fall back to the default so a | ||
| typo can never make the hook block on every single turn. |
There was a problem hiding this comment.
The _get_save_interval docstring says “Invalid / non-positive values … fall back to the default”, but the implementation clamps non-positive values to 1 via max(1, value). Please update the docstring (or the logic) so the documented behavior matches what actually happens (especially since an interval of 1 effectively blocks every turn).
| Invalid / non-positive values silently fall back to the default so a | |
| typo can never make the hook block on every single turn. | |
| Missing / non-integer values silently fall back to the default. | |
| Parsed values less than 1 are clamped to 1, which causes the hook to | |
| block on every turn. |
| # Fully disabled via env var — pass through. | ||
| if _stop_hook_disabled(): | ||
| _output({}) | ||
| return | ||
|
|
There was a problem hiding this comment.
MEMPAL_STOP_HOOK_DISABLE currently returns early before counting messages and before updating/maintaining the per-session *_last_save state (and also skips logging). This contradicts the PR description (“state still tracked, no blocking”) and can cause an immediate block when the flag is turned off mid-session because since_last will include all messages since the last real save. Consider still updating the last-save point (and/or logging) when disabled so the hook remains non-blocking without losing cadence state.
| """Stop hook: block every N messages for auto-save. | ||
|
|
||
| Respects two env vars: | ||
| - ``MEMPAL_STOP_HOOK_DISABLE=1`` — pass through without blocking. | ||
| - ``MEMPAL_SAVE_INTERVAL=N`` — override the 15-message default. | ||
| """ |
There was a problem hiding this comment.
The new env-var behaviors are only unit-tested at the helper level. Since hook_stop now depends on process environment at runtime, please add integration tests that exercise hook_stop with MEMPAL_SAVE_INTERVAL set (e.g., verify it blocks at the overridden interval) and with MEMPAL_STOP_HOOK_DISABLE set (verify pass-through behavior and whatever state-tracking semantics you intend). Also ensure existing hook_stop tests clear these env vars so they don’t become flaky when a developer has them set in their shell.
## MemPalace#1 docstring / implementation mismatch in _get_save_interval Docstring said "invalid / non-positive values fall back to the default" but the code clamps to 1 via max(1, value). Rewrote the docstring to document the actual behavior: missing / non-integer values fall back to default; parsed values < 1 clamp to 1 (effectively blocks every turn). Added a pointer to MEMPAL_STOP_HOOK_DISABLE for users who wanted "no block" rather than "block every turn". ## MemPalace#2 MEMPAL_STOP_HOOK_DISABLE must not drift cadence state Disabled path used to return before updating *_last_save, so toggling the flag off mid-session would make since_last cover every message accumulated while disabled and trigger an immediate retroactive block. Fixed by advancing the last-save watermark to the current exchange count while disabled — state stays synced with activity, no block is ever emitted. Re-enabling resumes the normal cadence from the current position, not from whenever the hook was last active. ## MemPalace#3 integration tests for hook_stop env-var behavior Unit tests only exercised _get_save_interval / _stop_hook_disabled helpers. Added five integration tests in test_hooks_cli_tuning.py that run hook_stop end-to-end: - custom interval triggers block at override - custom interval passes through below override - MEMPAL_STOP_HOOK_DISABLE=1 passes through even far above interval - disabled path still advances last-save watermark (state tracking) - stop_hook_active short-circuit must not touch state Also added an autouse fixture in test_hooks_cli.py that monkeypatches MEMPAL_SAVE_INTERVAL and MEMPAL_STOP_HOOK_DISABLE off, so a developer with those vars set in their shell can't break the interval assertions. 79 tests pass (46 in test_hooks_cli.py + 33 in test_hooks_cli_tuning.py).
|
Thanks for the review. All 3 comments addressed in ba07cb6.
Testing
Classification: #1 quality (docs), #2 critical (bug — disabled flag actually broke on toggle-off), #3 quality (test coverage). The state-tracking fix in #2 is the load-bearing one — the docstring and test improvements make the feature safe to recommend. |
|
Hi, thanks for the contribution. This PR has merge conflicts with Could you rebase onto If this change is no longer relevant, feel free to close the PR. (This message is part of a periodic backlog pass, sent to all open PRs that match this state.) |
Summary
The stop hook currently blocks every 15 human messages with the "AUTO-SAVE checkpoint" prompt, hard-coded via the module-level `SAVE_INTERVAL` constant in `mempalace/hooks_cli.py`. Two new env vars let users tune that without reinstalling or editing plugin files:
Both are read on every hook invocation, so users can change behavior mid-session without touching the install.
Why
Two concrete cases I hit on my workstation today:
Behavior preserved
Test plan
Added `tests/test_hooks_cli_tuning.py` — 22 unit tests, all passing:
```
$ pytest tests/test_hooks_cli_tuning.py -v
22 passed in 0.06s
```
Files
🤖 Generated with Claude Code