Skip to content

Commit e1051c6

Browse files
rainxchzedclaude
andauthored
Compaction review fixes + payload-capture diagnostic (#9)
* Compaction review fixes + payload-capture diagnostic Three-persona review of the shipped compaction re-injection feature. Headline: the feature is unverified speculative plumbing — its primary channel (SessionStart source=compact additionalContext) is documented as dropped on current Claude Code (#15174), and the PostCompact-stdout fallback is an unconfirmed assumption. The good news the security review confirmed: the task #51 prompt-injection fence is fully preserved on both emit paths. Applied the cheap safety + correctness batch (not the larger hooklib refactor): - hook_compact now asserts its own identity: main(default_event="PostCompact"). If the host omits hook_event_name, a real PostCompact no longer falls through to the SessionStart JSON format on the very path this hook exists to serve. - Emit path is now non-fatal: a BrokenPipeError while writing the block is swallowed (return 0) instead of escaping to SystemExit as a traceback + nonzero exit, honoring the never-break-the-session contract. - Compaction no longer does heavy work in the hook's critical path: build_block takes fresh=, and on a compaction (fresh=False) we query the already-built shared index instead of running a full project reindex + 500-row pool re-mirror + embed backfill on every /compact. Fixed the misleading "Cheap: a local store read" comment. - Double-injection guard + breadcrumb: we register BOTH SessionStart(compact) and PostCompact (a host hedge); on a host that honors both, the block would be injected twice. A state.json breadcrumb (last_compact_reinject {key,event,ts}) written on emit lets the sibling event no-op within a 45s window — and doubles as on-device observability for which path actually fired. Uses the existing atomic+locked update_state. - Installer matches komi hooks by COMMAND SHAPE (regex on `-m komi.adapters.claude_code.<module>`), not a loose substring. The old substring test could self-heal-overwrite or uninstall-remove an unrelated user hook that merely mentioned the module path; PostCompact widened that surface. - Bounded the hook stdin read at 4MB. New: payload-capture diagnostic to finally verify on-device. - hook_capture.py records the raw stdin Claude Code sends (entry event, parsed keys, hook_event_name/source/trigger/session_id) to ~/.claude/komi/_hook_capture.jsonl, then delegates to normal recall so the session is unaffected. - `komi-learn capture on|off|show`: `on` re-points SessionStart+PostCompact at the recorder, `show` prints what fired, `off` restores normal hooks. This is how we'll answer "does re-injection actually work on this host." Tests: test_compaction_fixes.py (16) — explicit-event routing, emit broken-pipe swallowed, fresh=False on compaction / True on session start, _merged_store skip, dedup guard + per-session breadcrumb + end-to-end second-sibling no-op, bounded stdin, command-shape matching (lookalike user hook survives install+uninstall), capture on/off. Updated test_compact_reinjection.py mocks for the new build_block signature. Full suite 282 passed, 1 skip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * update: verify the AGENT's behavior was upgraded, not just the CLI A user asked exactly the right question: `update` upgrades the CLI, but does the coding agent's behavior (recall/distill/compaction) actually change too? Answer: yes — the agent behavior IS the komi package. The hooks invoke `python -m komi.adapters.claude_code.hook_recall` with a pinned interpreter, so upgrading that interpreter's site-packages means the next hook firing runs the new code. CLI and agent are one shipment, not two. The ONE caveat: the upgrade has to land in the same Python the hooks are pinned to. `pip install -U` upgrades the env it runs in; if the CLI lives in a different venv than the hooks, the CLI updates and the agent silently stays old. So make "the behavior is updated" a verified fact, not an assumption: - setup.hook_interpreters() reads the actual Python each installed komi hook runs under (parsing the interpreter token out of the command line, quoted or not). - updater.installed_version_via_subprocess(python=) can now query ANY interpreter's komi-learn version, not just the CLI's. - After a successful upgrade, cmd_update asks each hook interpreter what version it imports: * match -> "agent behavior updated — hooks run X (verified)" * differ -> loudly warns the agent is still on old code + prints the exact `"<that python>" -m pip install --upgrade komi-learn` to fix it * no hooks -> tells the user to run `komi-learn install` to enable the agent Proven on the real machine: reports the hook interpreter is current with proof. README: `update` now reads "upgrade komi-learn + the agent's hooks". Tests: 6 added in test_compaction_fixes.py — interpreter extraction (quoted/plain), hook_interpreters with/without install, verify match / stale-other-python (the critical divergence case, asserts the fix line) / no-hooks. Made the cmd_update CLI tests hermetic by neutering _verify_agent_updated (it shells the real hook Python; it has its own dedicated tests). Full suite 288 passed, 1 skip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address ultrareview findings (7 nits, all real consistency gaps) /ultrareview on this PR returned 7 findings, all severity "nit" but each tracing to an invariant the PR itself introduced. Fixed all: - _emit (hook_recall) is now event-aware + pipe-safe. The three early-exit no-op paths (dedup, recall-failure, empty-block) previously emitted JSON even on PostCompact — where stdout is appended verbatim to the model context, so a diagnostic {"_note": ...} blob would pollute it — and two of them sat OUTSIDE the broken-pipe try/except. _emit now suppresses output on PostCompact and swallows write errors, so every caller inherits both protections. (merged_bug_005) - hook_capture bounds its stdin read with the same _MAX_STDIN_BYTES cap as hook_recall. set_capture re-points the real events at hook_capture, so the cap must not be lost on those routes. (bug_001) - install no longer silently disables active capture: if an existing hook is a hook_capture command, _install_hooks leaves it and reports "capture left ON (run komi-learn capture off to restore)" instead of overwriting it as a benign "refreshed". (bug_013) - set_capture no longer re-installs removed hooks: it refuses up front when komi isn't installed (matching the missing-settings.json guard) and skips — never appends — per-event. So `capture on/off` after `uninstall` can't silently resurrect the hooks. (bug_016) - _verify_agent_updated splits the stale message on same_as_cli. A stale result on the SAME interpreter the CLI just upgraded (the couldn't-confirm fallback) now says "the upgrade did not land in this interpreter — retry/elevate/check pin" instead of the self-contradictory "DIFFERENT Python than the one just upgraded" with a fix command identical to the one that ran. (bug_015) - paths.read_state(): a lock-briefly-read-no-write helper. _compaction_already_served was calling update_state(identity) — its docstring said "read-only" but update_state always re-serializes + os.replace's state.json, taking an exclusive flock and rewriting byte-identical content on every compaction event. Now it only reads. (bug_003) - Fixed a tautological test: test_hook_interpreters_extracts_pinned_python had `assert X or interps[0]` (always true); now asserts the extracted interpreter equals sys.executable, so the invariant _verify_agent_updated relies on is actually guarded. (bug_008) Tests: +11 (emit suppression/json/broken-pipe, dedup-postcompact-empty, capture stdin bound, install-keeps-capture, set_capture-refuses-uninstalled, verify-same-python-message, read_state-no-write) and corrected the empty-block / recall-failure tests to assert the new per-event behavior. Full suite 299 passed, 1 skip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9ad470d commit e1051c6

11 files changed

Lines changed: 1027 additions & 43 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ pip install -e .
3434

3535
```bash
3636
komi-learn doctor # check the install and what to fix
37-
komi-learn update # upgrade to the latest version (--check to only look)
37+
komi-learn update # upgrade komi-learn + the agent's hooks (--check to only look)
3838
komi-learn status # config + how much it has learned
3939
komi-learn config # change any setting (menu, or `config set <key> <val>`)
4040
komi-learn sync # pull the latest community learnings
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
"""Diagnostic hook — capture the RAW payload Claude Code sends, then behave normally.
2+
3+
Purpose: the compaction re-injection feature is built on assumptions about which
4+
events fire on a ``/compact`` and what field names they carry (``hook_event_name``,
5+
``source`` vs ``trigger``, whether ``additionalContext`` / plain stdout actually
6+
reach the model). We have never observed a real payload. This hook records the exact
7+
stdin we receive — keyed by which entry point invoked it — to a JSONL file, then
8+
delegates to the normal recall path so the session is unaffected.
9+
10+
Enable it with ``komi-learn capture on`` (which re-points the SessionStart +
11+
PostCompact hooks here), run ``/compact`` in a real Claude Code session, then
12+
``komi-learn capture show`` to inspect what actually fired. ``komi-learn capture
13+
off`` restores the normal hooks.
14+
15+
Two entry points so we can tell SessionStart from PostCompact even if the payload
16+
omits the event name:
17+
``python -m komi.adapters.claude_code.hook_capture`` (SessionStart)
18+
``python -m komi.adapters.claude_code.hook_capture --compact`` (PostCompact)
19+
"""
20+
21+
from __future__ import annotations
22+
23+
import json
24+
import sys
25+
import time
26+
27+
from . import paths
28+
29+
30+
def capture_path():
31+
return paths.personal_root() / "_hook_capture.jsonl"
32+
33+
34+
def _capture(entry_event: str, raw: str) -> None:
35+
"""Append one capture record. Best-effort; never raises into the hook."""
36+
try:
37+
parsed = None
38+
try:
39+
parsed = json.loads(raw) if raw.strip() else {}
40+
except Exception:
41+
parsed = None
42+
rec = {
43+
"ts": time.time(),
44+
"entry_event": entry_event, # which entry point ran (authoritative)
45+
"raw_len": len(raw),
46+
"raw": raw[:8192], # cap; payloads are tiny
47+
"parsed_keys": sorted(parsed.keys()) if isinstance(parsed, dict) else None,
48+
"hook_event_name": (parsed or {}).get("hook_event_name") if isinstance(parsed, dict) else None,
49+
"source": (parsed or {}).get("source") if isinstance(parsed, dict) else None,
50+
"trigger": (parsed or {}).get("trigger") if isinstance(parsed, dict) else None,
51+
"session_id": (parsed or {}).get("session_id") if isinstance(parsed, dict) else None,
52+
}
53+
p = capture_path()
54+
p.parent.mkdir(parents=True, exist_ok=True)
55+
with open(p, "a", encoding="utf-8") as f:
56+
f.write(json.dumps(rec) + "\n")
57+
except Exception:
58+
pass
59+
60+
61+
def main(default_event: str = "") -> int:
62+
# Read stdin ONCE, capture it, then hand the same payload to the real recall
63+
# path so behavior is unchanged. We re-feed stdin by monkeypatching the reader.
64+
# Mirror hook_recall's bound (set_capture re-points the real events here, so the
65+
# cap must not be lost on these routes).
66+
from . import hook_recall
67+
raw = ""
68+
try:
69+
raw = sys.stdin.read(hook_recall._MAX_STDIN_BYTES + 1)
70+
if len(raw) > hook_recall._MAX_STDIN_BYTES:
71+
raw = "" # oversized/garbage → safe no-op
72+
except Exception:
73+
raw = ""
74+
_capture(default_event or "SessionStart", raw)
75+
76+
# Delegate to the normal recall, feeding it the bytes we already consumed.
77+
try:
78+
hook_recall._read_stdin_json = lambda: _safe_json(raw) # type: ignore
79+
return hook_recall.main(default_event=default_event)
80+
except Exception:
81+
# Even if delegation fails, never break the session.
82+
sys.stdout.write("{}")
83+
return 0
84+
85+
86+
def _safe_json(raw: str) -> dict:
87+
try:
88+
return json.loads(raw) if raw.strip() else {}
89+
except Exception:
90+
return {}
91+
92+
93+
if __name__ == "__main__":
94+
ev = "PostCompact" if (len(sys.argv) >= 2 and sys.argv[1] == "--compact") else "SessionStart"
95+
raise SystemExit(main(default_event=ev))

komi/adapters/claude_code/hook_compact.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,8 @@
1414
from .hook_recall import main
1515

1616
if __name__ == "__main__":
17-
raise SystemExit(main())
17+
# This entry point IS the PostCompact hook — assert that identity rather than
18+
# re-deriving it from stdin. If the host omits ``hook_event_name`` on the
19+
# PostCompact payload, main() would otherwise default to SessionStart and emit
20+
# the wrong format on the very path this hook exists to serve.
21+
raise SystemExit(main(default_event="PostCompact"))

komi/adapters/claude_code/hook_recall.py

Lines changed: 132 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,10 @@
3434
from . import paths
3535

3636

37-
def main() -> int:
37+
def main(default_event: str = "") -> int:
3838
payload = _read_stdin_json()
3939
cwd = payload.get("cwd", "") or ""
40-
event, source = _classify_event(payload)
40+
event, source = _classify_event(payload, default_event)
4141
is_compaction = (event == "PostCompact") or (event == "SessionStart" and source == "compact")
4242

4343
# Background maintenance (pool sync ~12h, curator ~7d) belongs to a genuine
@@ -51,26 +51,51 @@ def main() -> int:
5151
except Exception:
5252
pass
5353

54+
# Double-injection guard: we register BOTH SessionStart(compact) and PostCompact
55+
# for one compaction (a host-reliability hedge — see module docstring). On a host
56+
# that honors both channels the block would otherwise be injected twice. If a
57+
# sibling event already served THIS compaction moments ago, no-op.
58+
if is_compaction and _compaction_already_served(payload, event):
59+
_emit({}, note="komi recall: compaction already re-injected by a sibling event",
60+
event=event)
61+
return 0
62+
5463
try:
5564
# Recompute the block FRESH every time — at compaction this picks up anything
56-
# learned earlier this session. Cheap: a local store read.
57-
block = build_block(cwd, payload)
65+
# learned earlier this session. On a genuine session start we rebuild the
66+
# index from Markdown (fresh=True); on a compaction we query the index that
67+
# was already built at session start (fresh=False) — rebuilding mid-session
68+
# would be a synchronous reindex + pool re-mirror in the hook's critical path.
69+
block = build_block(cwd, payload, fresh=not is_compaction)
5870
except Exception as e:
5971
# Never break the session because recall failed — emit nothing.
60-
_emit({}, note=f"komi recall skipped: {e}")
72+
_emit({}, note=f"komi recall skipped: {e}", event=event)
6173
return 0
6274

6375
if not block:
64-
_emit({})
76+
_emit({}, event=event)
6577
return 0
66-
67-
_emit_block(block, event, is_compaction)
78+
# _emit_block is internally format-correct per event; the no-op/diagnostic emits
79+
# above go through _emit, which is now event-aware + pipe-safe (suppresses the
80+
# JSON diagnostic on PostCompact, where stdout is appended verbatim to context).
81+
try:
82+
_emit_block(block, event, is_compaction)
83+
if is_compaction:
84+
_record_compaction_served(payload, event)
85+
except Exception:
86+
pass # a broken pipe / write error must never wedge the session
6887
return 0
6988

7089

71-
def build_block(cwd: str, payload: dict) -> str:
72-
"""Build the recall context block from the merged store. Reusable across events."""
73-
store = _merged_store(cwd)
90+
def build_block(cwd: str, payload: dict, *, fresh: bool = True) -> str:
91+
"""Build the recall context block from the merged store. Reusable across events.
92+
93+
``fresh`` rebuilds this store's index slice + re-mirrors the pool from disk (the
94+
right thing at a genuine session start). When False (compaction), we skip that
95+
rebuild and query the existing shared index — it was already populated at session
96+
start, and a mid-session reindex is needless synchronous work in the hook path.
97+
"""
98+
store = _merged_store(cwd, fresh=fresh)
7499
return recall(
75100
store,
76101
cwd=cwd,
@@ -80,16 +105,73 @@ def build_block(cwd: str, payload: dict) -> str:
80105
)
81106

82107

83-
def _classify_event(payload: dict) -> tuple[str, str]:
108+
def _classify_event(payload: dict, default_event: str = "") -> tuple[str, str]:
84109
"""Return (event, source). ``event`` is the hook event name (SessionStart /
85110
PostCompact / …); ``source`` is the SessionStart trigger (startup/resume/clear/
86-
compact) or the compaction trigger (manual/auto), empty if absent. Defaults to
111+
compact) or the compaction trigger (manual/auto), empty if absent.
112+
113+
``default_event`` is supplied by the invoking entry point (e.g. hook_compact
114+
passes "PostCompact") and WINS over a missing/absent ``hook_event_name`` — the
115+
entry point knows its own identity, so we never misroute a real PostCompact to
116+
the SessionStart format just because the host omitted the field. Falls back to
87117
SessionStart so a bare/legacy payload behaves exactly as before."""
88-
event = payload.get("hook_event_name") or "SessionStart"
118+
event = payload.get("hook_event_name") or default_event or "SessionStart"
89119
source = payload.get("source") or payload.get("trigger") or ""
90120
return event, source
91121

92122

123+
# How close two events must be (seconds) to count as serving the SAME compaction.
124+
# SessionStart(compact) and PostCompact fire within moments of one /compact; a later
125+
# genuine compaction is many seconds away. Generous enough to dedup siblings, tight
126+
# enough not to swallow a real subsequent compaction.
127+
_COMPACTION_DEDUP_WINDOW = 45.0
128+
129+
130+
def _compaction_key(payload: dict) -> str:
131+
"""Identify a compaction event for dedup. Prefer the host's session id (both
132+
sibling events share it); fall back to a constant so dedup still works per-window
133+
when no id is present."""
134+
return str(payload.get("session_id") or payload.get("sessionId") or "_nosid")
135+
136+
137+
def _compaction_already_served(payload: dict, event: str) -> bool:
138+
"""True if a sibling event already re-injected for THIS compaction (same session
139+
id, within the dedup window) — so we don't inject the block twice. Read-only."""
140+
import time
141+
key = _compaction_key(payload)
142+
try:
143+
state = paths.read_state() # read-only: no lock-and-rewrite
144+
except Exception:
145+
return False
146+
last = state.get("last_compact_reinject") or {}
147+
if last.get("key") != key:
148+
return False
149+
if last.get("event") == event:
150+
return False # the SAME event re-firing (e.g. retry) — let it re-inject
151+
try:
152+
return (time.time() - float(last.get("ts", 0))) < _COMPACTION_DEDUP_WINDOW
153+
except Exception:
154+
return False
155+
156+
157+
def _record_compaction_served(payload: dict, event: str) -> None:
158+
"""Breadcrumb: record that THIS event re-injected for THIS compaction. Doubles as
159+
the dedup signal a sibling event reads, and as on-device observability (which path
160+
actually fired in production)."""
161+
import time
162+
key = _compaction_key(payload)
163+
now = time.time()
164+
165+
def _mut(s: dict):
166+
s["last_compact_reinject"] = {"key": key, "event": event, "ts": now}
167+
return None
168+
169+
try:
170+
paths.update_state(_mut)
171+
except Exception:
172+
pass
173+
174+
93175
def _emit_block(block: str, event: str, is_compaction: bool) -> None:
94176
"""Emit the recall block in the form the given event supports.
95177
@@ -124,16 +206,25 @@ def _emit_block(block: str, event: str, is_compaction: bool) -> None:
124206
"additionalContext": ctx}})
125207

126208

127-
def _merged_store(cwd: str) -> Store:
209+
def _merged_store(cwd: str, *, fresh: bool = True) -> Store:
128210
"""Personal store is the base; if in a project, its learnings share the same
129211
index so a single recall query sees both. We open the personal store (which
130212
owns index.db) and ensure the project store + synced global pool are mirrored
131-
into the shared index so one recall query sees personal + project + global."""
213+
into the shared index so one recall query sees personal + project + global.
214+
215+
When ``fresh`` is False (a compaction re-inject), we SKIP the project reindex
216+
and the pool re-mirror: the shared index was already built at session start, and
217+
those operations are a full DELETE+re-INSERT (project Markdown) plus up to a
218+
500-row pool mirror — too heavy to run synchronously in the hook's critical path
219+
on every mid-session compaction. We just query what's already indexed.
220+
"""
132221
personal = Store(paths.personal_root(), index_path=paths.index_path())
222+
if not fresh:
223+
return personal
133224
proot = paths.project_root(cwd)
134225
if proot is not None:
135226
proj = Store(proot, index_path=paths.index_path())
136-
# cheap: make sure project rows are present in the shared index
227+
# make sure project rows are present in the shared index (session start only)
137228
proj.reindex()
138229
_mirror_pool_into_index(personal)
139230
return personal
@@ -226,19 +317,39 @@ def _recent_files(payload: dict) -> list[str]:
226317
return []
227318

228319

320+
_MAX_STDIN_BYTES = 4 * 1024 * 1024 # hook payloads are tiny; cap to avoid a runaway read
321+
322+
229323
def _read_stdin_json() -> dict:
230324
try:
231-
data = sys.stdin.read()
325+
data = sys.stdin.read(_MAX_STDIN_BYTES + 1)
326+
if len(data) > _MAX_STDIN_BYTES:
327+
return {} # oversized/garbage payload → safe no-op
232328
return json.loads(data) if data.strip() else {}
233329
except Exception:
234330
return {}
235331

236332

237-
def _emit(obj: dict, *, note: str = "") -> None:
333+
def _emit(obj: dict, *, note: str = "", event: str = "") -> None:
334+
"""Emit a no-op / diagnostic result. Event-aware and pipe-safe:
335+
336+
- For PostCompact, stdout is appended VERBATIM to the model's context (its
337+
documented add-to-context path), so a diagnostic JSON blob like
338+
``{"_note": ...}`` would pollute the context. Emit nothing on PostCompact.
339+
- For SessionStart, additionalContext comes from a structured JSON object, so a
340+
bare ``{}`` (optionally with a ``_note``) is the correct no-op.
341+
- A closed stdout (BrokenPipeError, host hung up early) must never wedge the
342+
session — swallow any write error.
343+
"""
344+
if event == "PostCompact":
345+
return
238346
if note:
239347
obj = {**obj, "_note": note}
240-
sys.stdout.write(json.dumps(obj))
241-
sys.stdout.flush()
348+
try:
349+
sys.stdout.write(json.dumps(obj))
350+
sys.stdout.flush()
351+
except Exception:
352+
pass
242353

243354

244355
if __name__ == "__main__":

komi/adapters/claude_code/paths.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,41 @@ def update_state(mutator):
124124
fh.close()
125125

126126

127+
def read_state() -> dict:
128+
"""Read state.json under the shared lock WITHOUT writing it back.
129+
130+
``update_state`` always performs a full read-modify-write (it re-serializes and
131+
atomically replaces the file even for an identity mutator), which is wasteful and
132+
serializes concurrent sessions for a pure read. Callers that only need to inspect
133+
state (e.g. the compaction dedup check) use this instead. The lock is still held
134+
briefly so a reader never observes a torn mid-``os.replace`` write. Best-effort:
135+
returns {} on any error.
136+
"""
137+
import json
138+
139+
sp = state_path()
140+
if not sp.exists():
141+
return {}
142+
lock = sp.with_suffix(".lock")
143+
fh = None
144+
try:
145+
fh = open(lock, "a+")
146+
_lock_file(fh)
147+
try:
148+
data = json.loads(sp.read_text(encoding="utf-8")) or {}
149+
return data if isinstance(data, dict) else {}
150+
except (json.JSONDecodeError, OSError):
151+
return {}
152+
except Exception:
153+
return {}
154+
finally:
155+
if fh is not None:
156+
try:
157+
_unlock_file(fh)
158+
finally:
159+
fh.close()
160+
161+
127162
def _lock_file(fh) -> None:
128163
"""Acquire an exclusive advisory lock (blocking). No-op if locking is unavailable."""
129164
try:
@@ -153,4 +188,5 @@ def _unlock_file(fh) -> None:
153188
__all__ = [
154189
"claude_home", "personal_root", "project_root", "index_path",
155190
"queue_dir", "outbox_dir", "inbox_dir", "keys_dir", "state_path", "update_state",
191+
"read_state",
156192
]

0 commit comments

Comments
 (0)