fix(plugin): a compaction clears every session ledger, not the two on the list (#4101)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m4s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m4s
CI & Build / Build & push image (push) Skipped
`scribe_session_context.sh` cleared `.rules.ids` and `.opened.ids` by name and left `.ids`, `.sync.ids` and `.derive.ids` standing, under a comment asserting that was a decision. Reading the note arms says it was not: their exclusions go straight into `semantic_search_notes`, so a surfaced note leaves the result set rather than being rendered as a reference the way #3750 gave a repeated rule, and unlike the rules ledger they never age. Hard, permanent, never cleared — a note surfaced in a session's first minute is unreachable for the rest of it, which is milestone 386's own defect alive on the arms that fire most often. The list was the bug, so the fix is not a longer list. `scribe_clear_session_ ledgers` matches the naming convention instead — a per-session ledger is `<sid>[.<kind>].ids` — which covers all five and covers the sixth on the day it is written. `<sid>.unreached` is deliberately outside it: that records an outage, not held context, and #2932 needs it to survive. tests/test_session_ledger_clear.py runs the hook rather than grepping it for `rm -f`, since grepping for the names is the pattern being removed. It pins both directions — `compact`/`clear` take all five, `startup`/`resume` take none — plus the convention the glob rests on, checked against the hooks themselves so a ledger named outside it fails loudly instead of silently never clearing. Also drops a stale comment pointing at a rules-etag marker that milestone 394 retired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -0,0 +1,161 @@
|
||||
"""A compaction clears every session ledger, by convention not by list (#4101).
|
||||
|
||||
WHY THIS EXISTS
|
||||
|
||||
Milestone 386 established the claim: a ledger describes what a session HOLDS,
|
||||
so the events that destroy context must destroy it too, or the records it names
|
||||
become permanently unreachable mid-session. `scribe_session_context.sh`
|
||||
implemented that — for the rules ledger, by name.
|
||||
|
||||
Five ledgers live in that directory and two were on the list. The note arms'
|
||||
three (`.ids`, `.sync.ids`, `.derive.ids`) were left, under a comment asserting
|
||||
this was "a decision rather than an oversight". It was not a decision, and the
|
||||
arms left out are the ones where it costs most:
|
||||
|
||||
- their exclusions are HARD — `exclude_ids` is passed into
|
||||
`semantic_search_notes` itself, so a surfaced note leaves the result set
|
||||
entirely, with no weaker rendering to fall back to the way #3750 gave a
|
||||
repeated rule one;
|
||||
- and they never AGE — #3751's TTL was added to the rules ledger only.
|
||||
|
||||
Hard, permanent and never cleared: a note surfaced in a session's first minute
|
||||
is unreachable for the rest of it, through any number of compactions.
|
||||
|
||||
WHAT THIS PINS
|
||||
|
||||
The list was the bug, so the fix cannot be a longer list and neither can the
|
||||
test. Both sides are asserted:
|
||||
|
||||
1. BEHAVIOUR — the hook is run on a real `compact` event with all five
|
||||
ledgers on disk, and all five are gone afterwards. Run rather than
|
||||
grepped, because grepping for the names is the hand-maintained pattern
|
||||
this step removes.
|
||||
2. THE CONVENTION THE BEHAVIOUR RESTS ON — every per-session ledger any hook
|
||||
builds is named `<sid>[.<kind>].ids`. That is what makes a sixth ledger
|
||||
covered on the day it is written, and it is the assumption that would
|
||||
rot silently, because a ledger named outside it simply never clears and
|
||||
nothing says so.
|
||||
|
||||
And the negative: `<sid>.unreached` is not a ledger of held context but a
|
||||
record that the instance could not be reached, and a glob that swept it away
|
||||
would make a hook forget an outage it is meant to report (#2932).
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
HOOKS = Path(__file__).resolve().parents[1] / "plugin" / "hooks"
|
||||
SESSION_START = HOOKS / "scribe_session_context.sh"
|
||||
|
||||
# Every ledger the hooks write today. Named here so a failure READS as "this
|
||||
# one survived", but never used to build the hook's own delete list — the
|
||||
# convention test below is what keeps this roster honest as it grows.
|
||||
LEDGERS = (".ids", ".rules.ids", ".opened.ids", ".sync.ids", ".derive.ids")
|
||||
|
||||
|
||||
def _run_session_start(source: str, tmp: Path) -> Path:
|
||||
"""Run the SessionStart hook for real, with a populated ledger directory."""
|
||||
for tool in ("bash", "jq"):
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"hook runtime tool {tool!r} not installed")
|
||||
|
||||
state = tmp / "scribe-priorart"
|
||||
state.mkdir(parents=True, exist_ok=True)
|
||||
for suffix in LEDGERS:
|
||||
(state / f"s1{suffix}").write_text("42\t1789600000\n")
|
||||
# Not a ledger: an outage marker that must outlive the clear.
|
||||
(state / "s1.unreached").write_text("1\n")
|
||||
|
||||
env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)}
|
||||
out = subprocess.run(
|
||||
["bash", str(SESSION_START)],
|
||||
input=json.dumps({"session_id": "s1", "source": source}),
|
||||
capture_output=True, text=True, env=env, timeout=60,
|
||||
)
|
||||
assert out.returncode == 0, out.stderr
|
||||
return state
|
||||
|
||||
|
||||
@pytest.mark.parametrize("source", ["compact", "clear"])
|
||||
def test_a_context_destroying_source_clears_every_ledger(source, tmp_path):
|
||||
"""The step, stated as behaviour: all five, not the two that were listed."""
|
||||
state = _run_session_start(source, tmp_path)
|
||||
survived = [s for s in LEDGERS if (state / f"s1{s}").exists()]
|
||||
assert not survived, (
|
||||
f"{survived} survived a {source!r} that destroyed what they describe"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("source", ["startup", "resume"])
|
||||
def test_a_source_that_kept_the_context_keeps_the_ledgers(source, tmp_path):
|
||||
"""The mirror error, and the more expensive one.
|
||||
|
||||
`resume` genuinely restores the context, so the ledger still describes what
|
||||
the session holds; clearing there would re-surface every record after a
|
||||
restore that lost nothing. A blanket glob makes over-clearing cheap to
|
||||
write, which is exactly why this direction needs a test of its own.
|
||||
"""
|
||||
state = _run_session_start(source, tmp_path)
|
||||
for suffix in LEDGERS:
|
||||
assert (state / f"s1{suffix}").exists(), (
|
||||
f"s1{suffix} was cleared on {source!r}, which lost no context"
|
||||
)
|
||||
|
||||
|
||||
def test_the_outage_marker_is_not_swept_with_them(tmp_path):
|
||||
"""`.unreached` records that the instance was down, not what was surfaced.
|
||||
|
||||
Different lifetime, different question. #2932's whole point is that "we
|
||||
checked and found nothing" and "we never managed to check" must stay
|
||||
distinguishable, and a clear that took this file out would quietly answer
|
||||
the second with the first.
|
||||
"""
|
||||
state = _run_session_start("compact", tmp_path)
|
||||
assert (state / "s1.unreached").exists()
|
||||
|
||||
|
||||
def test_every_ledger_any_hook_builds_follows_the_naming_convention():
|
||||
"""The assumption the glob rests on, asserted where it can actually fail.
|
||||
|
||||
A ledger named outside `<sid>[.<kind>].ids` does not break loudly — it just
|
||||
never clears, on the arm whose author had no reason to know a convention
|
||||
existed. So the convention is checked against the hooks themselves rather
|
||||
than trusted: every path any hook composes from the session-id stem has to
|
||||
end in `.ids`, or be named below as a deliberate non-ledger.
|
||||
"""
|
||||
# Files built from the safe session id that are NOT per-session ledgers.
|
||||
NOT_LEDGERS = {".unreached"}
|
||||
|
||||
offenders = []
|
||||
for script in sorted(HOOKS.glob("*.sh")):
|
||||
for suffix in re.findall(r'\$\{safe_sid\}([A-Za-z0-9_.]*)',
|
||||
script.read_text()):
|
||||
if suffix in NOT_LEDGERS or suffix.endswith(".ids"):
|
||||
continue
|
||||
offenders.append(f"{script.name}: ${{safe_sid}}{suffix}")
|
||||
|
||||
assert not offenders, (
|
||||
"these session files clear on no compaction, because the clear matches "
|
||||
f"on the `.ids` convention and they do not follow it: {offenders}"
|
||||
)
|
||||
|
||||
|
||||
def test_the_clear_is_derived_and_not_a_list_of_names():
|
||||
"""The regression that would look like a fix.
|
||||
|
||||
Appending an `rm -f` per ledger passes every behavioural test above while
|
||||
rebuilding the trap for the sixth one. The property worth keeping is that
|
||||
the hook names no ledger at all.
|
||||
"""
|
||||
sh = SESSION_START.read_text()
|
||||
block = sh.split('case "$source" in')[1].split("esac")[0]
|
||||
assert "scribe_clear_session_ledgers" in block
|
||||
for suffix in LEDGERS:
|
||||
assert suffix not in block, (
|
||||
f"the clear names {suffix} again — a list, not a convention"
|
||||
)
|
||||
Reference in New Issue
Block a user