diff --git a/CHANGELOG.md b/CHANGELOG.md index 58692b9..f95ea63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **A compaction no longer re-injects the whole memory recap** ([#339](https://github.com/Digital-Process-Tools/claude-remember/issues/339)) — `SessionStart` fires at every auto-compaction with `source=compact`, and the hook read `session_id` out of that payload ([#206](https://github.com/Digital-Process-Tools/claude-remember/issues/206)/[#270](https://github.com/Digital-Process-Tools/claude-remember/issues/270)) while discarding `source`. Every memory file was therefore `cat`'d again, into a context that had just been replaced by a summary of the conversation those same bytes were already in. The reporter measured `compact` firing about as often as `startup` over 40 days. + + **A compaction is not a new session.** The store has not changed since this session started and the recap is not news, so at `source=compact` the bodies are not repeated — with one exception. + + **Identity still is.** `identity.md` works by *presence*: a path to it does not make the agent behave as that persona, and no other line of the hook's output even names the file. Everything else is recall-on-demand and stays addressable — the unconditional `=== REMEMBER ===` hint names the store's files on every fire, and the `=== MEMORY ===` block now names the withheld ones again with their sizes. That is [#124](https://github.com/Digital-Process-Tools/claude-remember/issues/124)'s vocabulary for "kept but not injected": a recap that shrinks in silence is indistinguishable from a store that emptied. + + **The default runs one way only.** A payload with no `source`, an empty value, a spelling from a future release, or no stdin at all is left unrecognised and gets today's output unchanged. An absence read as `compact` would silently stop injecting memory for anyone whose payload shape differs from the one this heuristic was written against — the failure this plugin exists to prevent, not to cause. + + **Nothing else narrows.** `startup`, `resume`, `clear` and `fork` are untouched, and so are `=== HANDOFF ===`, `=== LAST HANDOFF ===`, the history hint, the consolidation trigger and the `hooks.d/` dispatches, at every source. `fork` in particular is left at the full recap deliberately: what a fork inherits from its parent's context was not established, and an unverified belief is not grounds for withholding memory. Neither the recovery block nor the capture-gap check branches on `source` — #206 settled that by changing the shape of the evidence store, precisely because a source filter answers the wrong half of that question. + ### Fixed - **A cooldown marker AHEAD of now sticks the throttle ON, permanently and mutely** ([#326](https://github.com/Digital-Process-Tools/claude-remember/issues/326)) — `case "$X" in ''|*[!0-9]*) X=0 ;; esac` validates a marker's *syntax*. It does not bound its *range*. A digits-only value ahead of the current clock is accepted, makes `ELAPSED` negative, and a negative `ELAPSED` is `-lt` any cooldown — so the gate takes its `exit 0`. That exit sits **above** the line that rewrites the marker, so the self-heal the previous three releases rest on is unreachable on exactly the path that needs it. Four sites, four different outages: no session is ever saved again; `now.md` is never compressed again and grows unbounded; the git backup stops, which is [#258](https://github.com/Digital-Process-Tools/claude-remember/issues/258)'s original outage reached through a value the guard *accepts*; and the per-tool-call fork throttle refuses to fork the save that would have healed the marker, so the two throttles hold each other shut. diff --git a/README.md b/README.md index ad1e8b9..a1a05ed 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,8 @@ On session start, the `SessionStart` hook automatically injects into Claude's co No manual prompting, no "read this file" instructions. The agent begins every session with its memory already loaded. It just remembers. +**Except after a compaction.** `SessionStart` fires again with `source=compact`, and a compaction is not a new session: the store has not changed and the same bytes were already delivered, once, to the context the compaction has just replaced ([#339](https://github.com/Digital-Process-Tools/claude-remember/issues/339)). There the hook still injects `identity.md` — a path to it does not make the agent behave as that persona — and names the rest with their sizes instead of injecting them, so they stay greppable. `startup`, `resume`, `clear` and `fork` are unchanged, and so is any payload whose `source` this hook does not recognise. + ### How memory files are written Writers of `now.md` take `save.lock`. **Readers do not, by design** — the `SessionStart` hook that injects memory into a new session sources only what it needs (`resolve-paths.sh`, `detect-tools.sh`, `bootstrap-dirs.sh`, `log.sh`, `lib-env-cache.sh`) and never `lib-lock.sh`, so it *cannot* lock even if it wanted to. That is deliberate: it runs before your first prompt, and `save.lock` is held for the whole of a save including its `claude -p` call ([#227](https://github.com/Digital-Process-Tools/claude-remember/issues/227), [#230](https://github.com/Digital-Process-Tools/claude-remember/issues/230), [#204](https://github.com/Digital-Process-Tools/claude-remember/issues/204)). A hook that blocks your prompt behind a model call is a worse outcome than anything it would be protecting you from. @@ -214,7 +216,7 @@ The plugin registers three Claude Code hooks: | Hook | Script | Purpose | | ------------------ | ----------------------- | --------------------------------------------------------- | -| `SessionStart` | `session-start-hook.sh` | Loads memory files into context, recovers missed sessions | +| `SessionStart` | `session-start-hook.sh` | Loads memory files into context (identity only at `source=compact`), recovers missed sessions | | `UserPromptSubmit` | `user-prompt-hook.sh` | Injects current timestamp so the agent knows the time | | `PostToolUse` | `post-tool-hook.sh` | Auto-saves session when tool call delta exceeds threshold | diff --git a/scripts/session-start-hook.sh b/scripts/session-start-hook.sh index e1df5e0..7d91c2b 100755 --- a/scripts/session-start-hook.sh +++ b/scripts/session-start-hook.sh @@ -6,7 +6,8 @@ # DESCRIPTION # Runs at the beginning of every Claude Code session. Performs three jobs: # 1. Injects memory files (identity, core memories, today, now, recent, -# archive) into the session context via stdout. +# archive) into the session context via stdout. At source=compact only +# identity is injected and the rest are named — see #339, below. # 2. Recovers the most recent missed session by launching save-session.sh # with --force in the background. # 3. Triggers background maintenance: consolidation of past-day staging @@ -108,9 +109,11 @@ _remember_env_cache_publish # # #206 named the enabler and shipped the other half of the fix: this hook never # read its stdin, so it had neither `source` nor `session_id` and could not -# exclude itself. Only `session_id` is needed. Excluding our own transcript by -# id is correct at EVERY source, so there is no source list to enumerate and -# nothing that was being reported stops being reported. +# exclude itself. Only `session_id` is needed for THAT job: excluding our own +# transcript by id is correct at EVERY source, so there is no source list to +# enumerate and nothing that was being reported stops being reported. `source` +# is read too, since #339, but for a different job entirely — how much of the +# memory recap to print — and nothing on this path consults it. # # Reading stdin is only safe if it cannot wait forever, so this takes both # guards post-tool-hook.sh documents: a tty stdin (hand invocation from a @@ -131,12 +134,17 @@ fi # same reason: the key must be followed by nothing but whitespace and a colon # before the value's opening quote, so a `session_id` appearing inside some # other field is not mistaken for the field. It is a heuristic and is treated -# as one — the result is validated as a path component below before anything -# is done with it. -_stdin_session_id() { - local raw="$1" rest prefix value - case "$raw" in *'"session_id"'*) ;; *) return 1 ;; esac - rest=${raw#*\"session_id\"} +# as one — every result is validated below before anything is done with it. +# +# Taken over the key rather than hard-coded, because #339 needs a second field +# — `source` — out of the same payload, and one heuristic is easier to reason +# about than two copies of it. post-tool-hook.sh keeps its own single-key +# copy: it reads one field, and sourcing a shared library from a hook that has +# to survive a broken install is a worse trade than ten duplicated lines. +_stdin_json_string() { + local key="$1" raw="$2" rest prefix value + case "$raw" in *"\"$key\""*) ;; *) return 1 ;; esac + rest=${raw#*\"$key\"} prefix=${rest%%\"*} case "$prefix" in *[!:[:space:]]*) return 1 ;; esac value=${rest#*\"} @@ -145,6 +153,10 @@ _stdin_session_id() { printf '%s' "$value" } +_stdin_session_id() { + _stdin_json_string session_id "$1" +} + CURRENT_SESSION_ID=$(_stdin_session_id "$HOOK_STDIN" 2>/dev/null) || CURRENT_SESSION_ID="" # stdin is not more trustworthy than a basename. This is compared against # names taken off the transcript directory, and `..` would match nothing @@ -154,6 +166,26 @@ case "$CURRENT_SESSION_ID" in ''|.|..|*[!A-Za-z0-9._-]*) CURRENT_SESSION_ID="" ;; esac +# ── Which KIND of SessionStart is this? (#339) ──────────────────────────── +# `source` is one of startup | resume | clear | compact | fork. It is read for +# exactly one decision — how much of the memory recap to print — and nothing +# else in this script branches on it. In particular the recovery block and the +# capture-gap check below deliberately do NOT: #206 settled that question by +# changing the shape of the evidence store, precisely because a source filter +# answers the wrong half of it. +# +# Read strictly, and in one direction only. A payload with no `source`, an +# empty value, a spelling from a future release, or no stdin at all leaves +# this empty and takes the unchanged path. An absence must never be read as +# `compact`: that would silently stop injecting memory for anyone whose +# payload shape differs from the one this heuristic was written against — +# the failure this plugin exists to prevent, not to cause. +SESSION_START_SOURCE=$(_stdin_json_string source "$HOOK_STDIN" 2>/dev/null) || SESSION_START_SOURCE="" +case "$SESSION_START_SOURCE" in + startup|resume|clear|compact|fork) ;; + *) SESSION_START_SOURCE="" ;; +esac + # ── Publish the consumed payload to hooks.d/ ────────────────────────────── # This hook now reads stdin, so a listener that wanted the payload would find # EOF where one used to be. It travels by the route #266 settled on: a file for @@ -898,8 +930,13 @@ cat "$PLUGIN_ROOT/prompts/session-history-hint.txt" 2>/dev/null echo "" # ── Inject memory into context ──────────────────────────────────────────── +# One list, read three times below — the membership test, the injection loop +# and the named-only loop. Kept in a single place so a seventh memory file +# cannot be added to one of them and forgotten by the others. +MEMORY_FILES=("$IDENTITY_FILE" "$CORE_MEMORIES" "$REMEMBER_TODAY_FILE" "$REMEMBER_NOW" "$REMEMBER_RECENT" "$REMEMBER_ARCHIVE") + HAS_MEMORY="" -for MFILE in "$IDENTITY_FILE" "$CORE_MEMORIES" "$REMEMBER_TODAY_FILE" "$REMEMBER_NOW" "$REMEMBER_RECENT" "$REMEMBER_ARCHIVE"; do +for MFILE in "${MEMORY_FILES[@]}"; do if [ -f "$MFILE" ]; then HAS_MEMORY="true" fi @@ -915,14 +952,46 @@ fi if [ -n "$HAS_MEMORY" ]; then echo "=== MEMORY ===" - for MFILE in "$IDENTITY_FILE" "$CORE_MEMORIES" "$REMEMBER_TODAY_FILE" "$REMEMBER_NOW" "$REMEMBER_RECENT" "$REMEMBER_ARCHIVE"; do + # At source=compact these bodies were already delivered — in this same + # session, to the context the compaction has just replaced with a summary + # of it. SessionStart fires again there, but the store has not changed and + # nothing about the recap is news. + # + # Identity is the exception and is still printed in full, because it works + # by PRESENCE: a path to identity.md does not make the agent behave as + # that persona, and no other line of this hook's output even names the + # file. Everything else is recall-on-demand and stays addressable — the + # === REMEMBER === hint above names the store's files on every single + # fire, and the block below names these ones again with their sizes. + # + # Named rather than dropped, which is the #124 vocabulary for "kept but + # not injected": a file nobody names is a file nobody greps, and a recap + # that shrinks in silence is indistinguishable from a store that emptied. + for MFILE in "${MEMORY_FILES[@]}"; do if [ -f "$MFILE" ] && [ -s "$MFILE" ]; then + if [ "$SESSION_START_SOURCE" = "compact" ] && [ "$MFILE" != "$IDENTITY_FILE" ]; then + continue + fi BASENAME=$(basename "$MFILE") echo "--- $BASENAME ---" cat "$MFILE" echo "" fi done + if [ "$SESSION_START_SOURCE" = "compact" ]; then + # Built before the header is printed, so the header is never printed + # over an empty list — a store can hold identity.md and nothing else. + DEFERRED_MEMORY=$(for MFILE in "${MEMORY_FILES[@]}"; do + [ "$MFILE" != "$IDENTITY_FILE" ] || continue + [ -f "$MFILE" ] && [ -s "$MFILE" ] || continue + printf '%s (%s bytes)\n' "$MFILE" "$(wc -c < "$MFILE" | tr -d ' ')" + done) + if [ -n "$DEFERRED_MEMORY" ]; then + echo "--- not re-injected at compact (delivered at session start); read or grep on request ---" + printf '%s\n' "$DEFERRED_MEMORY" + echo "" + fi + fi # ── Rotated archives: named, not injected (#124) ────────────────────── # An oversized archive.md is rotated to archive-YYYY-MM-DD.md and a fresh # one started (#123). The bytes are kept, but nothing in the read path diff --git a/tests/test_session_start_compact_recap_339.py b/tests/test_session_start_compact_recap_339.py new file mode 100644 index 0000000..e9483d5 --- /dev/null +++ b/tests/test_session_start_compact_recap_339.py @@ -0,0 +1,207 @@ +"""At `source=compact` the recap must not be re-injected wholesale (#339). + +`SessionStart` fires again at every auto-compaction. The hook read `session_id` +out of the payload (#206/#270) and discarded `source`, so a compaction received +the same full recap a cold start does — every memory file cat'd into a context +that was just replaced by a summary of a conversation which already contained +them. + +What a compaction is NOT is a fresh session, and that asymmetry is the whole +design here: + +* **Identity is presence-required.** A pointer to `identity.md` does not make + the agent behave as that persona, and nothing else in the session-start + output even names the file. It stays injected in full. +* **Everything else is recall-on-demand**, and already addressable: the + unconditional `=== REMEMBER ===` hint names `now.md`, `today-*.md`, + `recent.md`, `archive.md` and `core-memories.md` on every fire. Naming them + again with their sizes costs a line each and loses nothing that cannot be + read back. + +The direction of the default is the other half. A missing, empty or +unrecognised `source` must produce today's output exactly — an absence read as +`compact` would silently stop injecting memory for anyone whose payload shape +differs from the one this heuristic was written against, which is the failure +this plugin exists to prevent rather than to cause. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.skipif( + sys.platform == "win32", + reason="bash hook subprocess + POSIX semantics — not portable to Windows runners", +) + +REPO_ROOT = Path(__file__).resolve().parent.parent +SESSION_START = REPO_ROOT / "scripts" / "session-start-hook.sh" + +sys.path.insert(0, str(REPO_ROOT)) +from pipeline.slug import session_dir_slug as _slug + +SESSION = "dddddddd-0000-4000-8000-000000000339" + +# One sentinel per memory file. Asserting on the BODY, not on the section +# header: the header survives either way, so a test that looked for it would +# pass against a hook that still cat'd everything. +# Today's staging file is named for the date, so it is built the way the hook +# builds it: system-local, because this repo ships no config.json and an unset +# REMEMBER_TZ falls back to local rather than UTC (#99). Naming it matters — +# it is the sixth entry of MEMORY_FILES, and a fixture that omitted it would +# leave the largest of the deferred files untested. +TODAY_FILE = "today-" + time.strftime("%Y-%m-%d") + ".md" + +BODIES = { + "identity.md": "IDENTITY-BODY-339", + "core-memories.md": "CORE-BODY-339", + TODAY_FILE: "TODAY-BODY-339", + "now.md": "NOW-BODY-339", + "recent.md": "RECENT-BODY-339", + "archive.md": "ARCHIVE-BODY-339", +} +DEFERRABLE = [n for n in BODIES if n != "identity.md"] + + +def _store(tmp_path): + home = tmp_path / "home" + project = tmp_path / "project" + remember = project / ".remember" + (remember / "tmp").mkdir(parents=True) + (home / ".claude" / "projects" / _slug(str(project))).mkdir(parents=True) + for name, body in BODIES.items(): + (remember / name).write_text(body + "\n", encoding="utf-8") + (remember / "remember.md").write_text("HANDOFF-BODY-339\n", encoding="utf-8") + return home, project, remember + + +def _env(home, project, remember): + return { + **os.environ, + "HOME": str(home), + "CLAUDE_PROJECT_DIR": str(project), + "CLAUDE_PLUGIN_ROOT": str(REPO_ROOT), + "REMEMBER_DIR": str(remember), + "_LIB_MEMORY_DIR_LOADED": "1", + } + + +def _payload(source=None, extra=None): + """A SessionStart payload. `source=None` omits the field entirely.""" + p = { + "session_id": SESSION, + "transcript_path": "/does/not/matter/" + SESSION + ".jsonl", + "hook_event_name": "SessionStart", + "cwd": "/does/not/matter", + } + if source is not None: + p["source"] = source + if extra: + p.update(extra) + return json.dumps(p) + + +def _run(tmp_path, payload): + home, project, remember = _store(tmp_path) + kwargs = { + "env": _env(home, project, remember), + "capture_output": True, + "text": True, + "timeout": 60, + } + if payload is None: + kwargs["stdin"] = subprocess.DEVNULL + else: + kwargs["input"] = payload + result = subprocess.run(["bash", str(SESSION_START)], check=False, **kwargs) + assert result.returncode == 0, result.stderr + return result.stdout, remember + + +def _assert_full_recap(out): + assert "=== MEMORY ===" in out + for name, body in BODIES.items(): + assert body in out, name + " body missing from a full recap" + + +# ── compact: identity injected, the rest named ──────────────────────────── + +def test_compact_still_injects_identity(tmp_path): + out, _ = _run(tmp_path, _payload("compact")) + assert "=== MEMORY ===" in out + assert BODIES["identity.md"] in out + + +def test_compact_does_not_reinject_the_recap_bodies(tmp_path): + out, _ = _run(tmp_path, _payload("compact")) + for name in DEFERRABLE: + assert BODIES[name] not in out, name + " was re-injected at source=compact" + + +def test_compact_names_every_file_it_withheld_by_full_path(tmp_path): + """Withholding silently would be the same defect in the other direction: + the agent cannot grep a file it was never told is there. + + The assertion is on the RESOLVED PATH, not the basename — a full recap + already prints `--- recent.md ---` as its body header, so a basename check + would pass against a hook that changed nothing at all. + """ + out, remember = _run(tmp_path, _payload("compact")) + for name in DEFERRABLE: + assert str(remember / name) in out, name + " was withheld without a path" + + +def test_compact_states_the_size_of_what_it_withheld(tmp_path): + """Named-not-injected is the #124 vocabulary, and #124 prints sizes: the + agent has to be able to tell a 300-byte now.md from a 300kB archive before + deciding to read it.""" + out, remember = _run(tmp_path, _payload("compact")) + size = (remember / "recent.md").stat().st_size + assert str(size) + " bytes" in out + + +def test_compact_keeps_the_other_sections(tmp_path): + """Only the recap bodies change. The handoff and the history hint are not + in scope and must not be narrowed by the same commit.""" + out, _ = _run(tmp_path, _payload("compact")) + assert "=== LAST HANDOFF ===" in out + assert "HANDOFF-BODY-339" in out + assert "=== REMEMBER ===" in out + + +# ── every other source is unchanged ─────────────────────────────────────── + +@pytest.mark.parametrize("source", ["startup", "resume", "clear", "fork"]) +def test_known_sources_other_than_compact_get_the_full_recap(tmp_path, source): + _assert_full_recap(_run(tmp_path, _payload(source))[0]) + + +def test_absent_source_field_gets_the_full_recap(tmp_path): + """An absence is not a compaction. A payload shape this heuristic does not + recognise must lose nothing.""" + _assert_full_recap(_run(tmp_path, _payload(None))[0]) + + +def test_no_stdin_at_all_gets_the_full_recap(tmp_path): + _assert_full_recap(_run(tmp_path, None)[0]) + + +@pytest.mark.parametrize("source", ["", "Compact", "COMPACT", "compaction", + "compact ", "startup,compact"]) +def test_unrecognised_source_values_get_the_full_recap(tmp_path, source): + _assert_full_recap(_run(tmp_path, _payload(source))[0]) + + +def test_a_similarly_named_key_is_not_read_as_source(tmp_path): + """The extractor is deliberately narrow: the key must be `"source"`, not a + field that merely ends in it.""" + out, _ = _run(tmp_path, _payload(None, {"hook_source": "compact", + "data_source": "compact"})) + _assert_full_recap(out)