Skip to content

feat: split hooks.auto_save and hooks.auto_mine for independent control of diary vs mine#1110

Open
sha2fiddy wants to merge 2 commits intoMemPalace:developfrom
sha2fiddy:feature/hooks-auto-mine-split
Open

feat: split hooks.auto_save and hooks.auto_mine for independent control of diary vs mine#1110
sha2fiddy wants to merge 2 commits intoMemPalace:developfrom
sha2fiddy:feature/hooks-auto-mine-split

Conversation

@sha2fiddy
Copy link
Copy Markdown
Contributor

@sha2fiddy sha2fiddy commented Apr 22, 2026 •
edited
Loading

Problem

The Stop and PreCompact hooks currently do three things with no way to enable them independently:

  1. _save_diary_direct is cheap, a few hundred ms. Writes the distilled memory entry.
  2. _ingest_transcript is medium cost. Syncs the current session's .jsonl.
  3. _maybe_auto_ingest / _mine_sync is expensive. Walks the whole ~/.claude/projects/<cwd>/ tree and can peg a CPU core for hours on large archives.

So users are stuck picking between "no hook-driven memory" and "burn a core every 15 exchanges." Some people want the fast diary write but not the recursive mine, especially on machines with thousands of accumulated transcripts. #1068 and #1069 have context.

Solution

Adds a new config flag hooks.auto_mine (default true) that gates the two mine paths in both hooks. The diary save stays unconditional because it's cheap and it's the thing users actually want.

With auto_mine: false, hook-driven diary writes keep working but the recursive mine is skipped. "Combined mode" is just leaving both silent_save and auto_mine on.

Flags

Flag Default Gates
hooks.silent_save true Silent Python diary write vs legacy {"decision": "block"} AI-driven save
hooks.auto_mine (new) true Single-file ingest + recursive mine after the diary save

Override precedence follows the existing pattern in config.py: env var (MEMPALACE_HOOKS_AUTO_MINE, legacy alias MEMPAL_HOOKS_AUTO_MINE) > ~/.mempalace/config.json > default True.

Usage

Via the existing MCP tool:

mempalace_hook_settings(auto_mine=False)

Via config file:

{
  "hooks": {
    "auto_mine": false
  }
}

Via env var:

export MEMPALACE_HOOKS_AUTO_MINE=false

Falsey values: 0, false, no, off (case-insensitive). Anything else is truthy. Empty-string env var falls through to the config file, matching the pattern used by MEMPALACE_PALACE_PATH and MEMPALACE_ENTITY_LANGUAGES.

Scope note

This PR doesn't touch hooks.silent_save or any other existing flag. It's independent of #711 (hooks.auto_save, the outer hook enable/disable). Either can land first; the other rebases cleanly. I kept them as separate PRs.

Test plan

  • tests/test_config.py: 5 new tests plus 7 parametrize cases covering default, file override, env override, and the legacy env prefix.
  • tests/test_hooks_cli.py: 3 new tests for the stop-hook silent branch (auto_mine on and off) and precompact with auto_mine off.
  • Full non-benchmark suite 1080/1080 locally on Python 3.14.
  • ruff check . passes.
  • ruff format --check passes on touched files under both current ruff and CI-pinned ruff>=0.4.0,<0.5.
  • CI: 6/6 green (lint, test-linux 3.9/3.11/3.13, test-macos, test-windows).

Backwards compatibility

hook_auto_mine defaults to True. Users who don't set the flag see no behavior change.

Files touched

  • mempalace/config.py: new hook_auto_mine property
  • mempalace/hooks_cli.py: gate mine calls in hook_stop (both branches) and hook_precompact
  • mempalace/mcp_server.py: auto_mine parameter on mempalace_hook_settings
  • tests/test_config.py: config coverage
  • tests/test_hooks_cli.py: hook coverage (helper _capture_hook_output gains an auto_mine=True default)
  • website/reference/mcp-tools.md: doc update
  • CHANGELOG.md: [Unreleased] entry

Related

@sha2fiddy sha2fiddy marked this pull request as ready for review April 22, 2026 16:27
jphein
jphein reviewed Apr 22, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jphein jphein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled and verified on Python 3.12:

  • 1080/1080 non-benchmark tests pass (matches your claim).
  • Gate placement checks out: _save_diary_direct stays unconditional; _ingest_transcript + _maybe_auto_ingest both guarded in silent AND legacy-block branches; precompact early-returns cleanly when auto_mine=false. Exception fallback defaults to True, so a broken config never accidentally disables mining — same defensive posture as silent_save.
  • Env-var override (MEMPALACE_HOOKS_AUTO_MINE + legacy alias) works as documented; falsey parsing matches the MEMPALACE_PALACE_PATH precedent.

Design aligns with what I proposed in #1083 — thanks for picking this up. _mine_sync's recursive walk has been our production's biggest CPU headache on a 165K-drawer palace; first-class opt-out is much cleaner than the wrapper workarounds I was queueing in a fork-ahead PR. I'll drop that row from our tracker in favor of yours.

Nit, not a blocker: hook_precompact re-instantiates MempalaceConfig() on its own rather than reusing the instance loaded elsewhere in the module. Fires once per session so probably not worth a re-roll — just flagging.

@igorls igorls added enhancement New feature or request area/hooks Claude Code hook scripts (Stop, PreCompact, SessionStart) labels Apr 24, 2026
@fubak fubak mentioned this pull request Apr 26, 2026
Open
@sha2fiddy sha2fiddy force-pushed the feature/hooks-auto-mine-split branch 3 times, most recently from 2805524 to dd86086 Compare April 29, 2026 21:26
@sha2fiddy
feat(hooks): add hooks.auto_mine toggle to decouple diary save from b…
4967008 
…ackground mine

Adds a `hooks.auto_mine` config flag so users can keep the fast diary
write in stop/precompact hooks but skip the recursive
`~/.claude/projects/<cwd>/` mine that can peg a CPU core for hours on
large transcript archives.

- config.py: new `hook_auto_mine` property (default True; file config
  under `hooks.auto_mine`; `MEMPALACE_HOOKS_AUTO_MINE` env var override
  with legacy `MEMPAL_HOOKS_AUTO_MINE` alias). Falsey values: 0, false,
  no, off.
- hooks_cli.py:
  - `hook_stop` silent branch: gates `_ingest_transcript` and
    `_maybe_auto_ingest` on `auto_mine`. `_save_diary_direct` stays
    unconditional — the diary write is the memory-preservation core
    and must not depend on the mine flag.
  - `hook_stop` legacy-block branch: same gating applied to
    `_ingest_transcript` + `_maybe_auto_ingest`.
  - `hook_precompact`: gates the whole ingest+mine block on
    `auto_mine`; compaction still proceeds when disabled.
- mcp_server.py / `mempalace_hook_settings`: accepts `auto_mine`
  alongside `silent_save` and `desktop_toast`, and returns it in the
  settings payload.
- Tests: 13 new cases covering defaults, file config, env var
  override (including legacy `MEMPAL_` prefix), parametrized falsey
  values, and the stop/precompact hook gating behavior.
- Docs: MCP tool reference and CHANGELOG entry.

Default remains `True` — existing users see zero behavior change until
they opt out.
@sha2fiddy sha2fiddy force-pushed the feature/hooks-auto-mine-split branch from dd86086 to 4967008 Compare April 29, 2026 23:01
@Qodo-Free-For-OSS
Copy link
Copy Markdown

Qodo-Free-For-OSS commented Apr 30, 2026

Hi, MempalaceConfig.hook_auto_mine only applies the env override when the env var is truthy, so MEMPALACE_HOOKS_AUTO_MINE="" falls back to config/default instead of overriding. Whitespace values (e.g. " ") are treated as truthy and end up enabling mining, which is inconsistent with the intent of using env vars to disable background mining.

Severity: remediation recommended | Category: correctness

How to fix: Handle empty env values

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

hook_auto_mine treats MEMPALACE_HOOKS_AUTO_MINE="" as “not set” and treats whitespace-only as truthy (enables mining). This makes env-based disabling unreliable in environments that inject empty values.

Issue Context

The code currently checks if env_val: and then interprets any non-listed value as truthy.

Fix Focus Areas

  • mempalace/config.py[298-316]

Suggested change

  • Change the guard to distinguish between unset vs set-to-empty, e.g. if env_val is not None:.
  • Decide semantics for empty/whitespace explicitly (common options: treat as False, or treat as “unset” consistently after strip()).
  • Add unit tests for "" and " " env values to lock the behavior.

Found by Qodo code review

@sha2fiddy
Copy link
Copy Markdown
Contributor Author

sha2fiddy commented Apr 30, 2026

Fixed in ec33d18. Changed the guard from if env_val: to if env_val is not None: and only treat the env var as authoritative when the stripped value is non-empty — so "" and whitespace-only fall back to config (matching the unset case), and 0/false/no/off correctly disable mining. Also separated the primary/legacy env var lookup so an empty primary doesn't silently fall through to the legacy var.

Added 5 parametrized tests in tests/test_config.py covering "", " ", "\t", "\n", plus the empty-primary-doesn't-leak-to-legacy case. 4 of them fail against the prior code; all pass after the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner

@bensig bensig Awaiting requested review from bensig bensig is a code owner

@igorls igorls Awaiting requested review from igorls igorls is a code owner

1 more reviewer

@jphein jphein jphein left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/hooks Claude Code hook scripts (Stop, PreCompact, SessionStart) enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sha2fiddy @Qodo-Free-For-OSS @jphein @igorls