Repository navigation
Groundwork: multiple assistant refactor, ignore non-human prompts - #13
Open
dannyjameswilliams wants to merge 17 commits into
Open
dannyjameswilliams wants to merge 17 commits into
dannyjameswilliams wants to merge 17 commits into
Conversation
The Stop hook fires on every turn, including ones nobody typed. A background task reporting back currently costs an extraction run and leaves a job log in the store that later surfaces as recall. The host already records who sent a turn. Every prompt-shaped transcript entry carries origin.kind, observed as "human" and "task-notification" across Claude Code 2.1.220 through 2.1.284, so core/transcript.py resolves it by prompt_id and the Stop hook skips anything not human. Two details the lookup depends on: tool results inherit the promptId of the prompt that spawned them and carry no origin, so it has to scan past them; and an unrecognised origin counts as human, so a host that changes this shape loses the skip rather than silently dropping real turns. Only storing is gated. Recall costs one round trip, its results are not harmful on a turn that needs them, and gating it would switch memory off entirely for autonomous runs where every turn is host-generated. Subagents need no handling: SubagentStop is not registered, so their turns never reach the store. get_client() passed no timeout, so a hung network stalled every prompt up to the platform cap. Recall now gets 8s, well inside the 30s the host enforces on UserPromptSubmit, and storing gets 25s since it runs async. ENGRAM_TIMEOUT overrides both, and falls back on a non-positive value because the SDK rejects those and get_client runs outside the hooks' try/except. ENGRAM_DEBUG prints one line per recall and store to stderr, visible under claude --debug. Nothing is written to disk. Adds the first CI workflow: the suite had two files and nothing ran them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything that differs between coding assistants was spread across core: transcript parsing keyed on Claude Code's payload shape, the client header hardcoded "claude", and the plugin root and data dir came only from the CLAUDE_-prefixed variables. core/assistants/ now holds that, one module per assistant providing turn_id, is_automated, last_user_text and STORE_FAILURE_EXIT. The hooks take an assistant rather than resolving one, and core/entry/ has a small module per assistant that names itself and passes it in. Three runtime approaches were tried first and discarded: sniffing PLUGIN_ROOT (a stray one silently switches assistants), discriminating on a payload key (needs a key unique across every assistant we will ever add), and an ENGRAM_ASSISTANT lookup (still a registry with a silent default). Passing the module removes the question — a mistake is an ImportError rather than a wrong answer. Each assistant now ships its own manifest and hooks file. There is no hooks/hooks.json, because Claude Code merges a manifest hooks path with that file when it exists, which would leak one assistant's entry point into the other's session. An assistant that ignored the manifest key would load no hooks rather than the wrong ones. Codex differs in three ways that matter. Its rollout files wrap a message in payload with role and input_text blocks and use a developer role for injected context, so it needs its own reader; the Claude Code parser returned nothing for every Codex turn. It reads exit 2 on Stop as "continue the turn, using stderr as the prompt", so a store failure must not use it. And it exposes no field recording who submitted a turn, so is_automated returns False and it loses the store skip, the same fail-open rule as an unrecognised Claude Code origin. client_origin no longer hardcodes the platform; the header carries the real assistant and reads that assistant's own manifest for the version. The plugin root and data dir accept either naming, specific name first: Codex sets PLUGIN_ROOT/DATA and aliases the CLAUDE_ ones to them, so preferring the generic name would let an unrelated PLUGIN_ROOT override the real path on Claude Code. ENGRAM_DEBUG now also reports the paths that return early, which were indistinguishable from the hook not running at all. Adds a store hook test covering the path that actually writes, which caught a variable collision that would have crashed every storable turn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The assistant modules were duck-typed: a module was an assistant if it happened to define the right names. Nothing checked that, so a new assistant missing STORE_FAILURE_EXIT or last_user_text failed at the first turn that needed it rather than at import, and the Codex module shipped without its own transcript reader for exactly that reason. core/assistants/base.py declares the contract as an abstract base. An incomplete subclass now raises at instantiation. Two methods are abstract; is_automated and STORE_FAILURE_EXIT carry the safe defaults, so an assistant has to opt in to the dangerous answers rather than remember to opt out — is_automated returning True by mistake drops real turns, and an exit code of 2 makes Codex treat a failed store as "keep working". Annotates the surfaces that contract touches: the hook run() signatures, the entry dispatch, the client and client origin helpers, and the transcript walk. client_origin imports Assistant under TYPE_CHECKING because the tests load it by path to avoid the SDK. Adds mypy over the annotated subset, listed in plugin/mypy.ini. A file joins that list once it checks clean, so the check never has to be weakened to keep CI green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Stop hook walked the transcript twice: once to resolve who submitted the turn, once for the user's message. Both only ever need the tail, and these files reach tens of megabytes — 66ms on the largest one on this machine. last_user_text now goes through the same walk the assistants use, and the file read is cached on the path plus its mtime and size, so a rewritten transcript is re-read rather than served stale. That matters for tests, which reuse a path; a hook process only lives for one turn. Halves the reads and removes the duplicated walk. The hook is asyncRewake, so this was never blocking a user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
core/transcript.py sat in the assistant-agnostic half of core while parsing
Claude Code's shape: {type: "user"}, message.content, text and tool_result
blocks. Its docstring stated that shape as what "the host" writes. That is the
assumption that made Codex return "" for every turn, and after the Codex
reader landed the module had one caller left while still reading as shared.
Each assistant now owns its own parsing, and the base class owns only the walk
that both happen to need: transcript() yields JSONL entries newest first and
is concrete rather than abstract, so an assistant whose transcript is not
JSONL can override it. The file read stays a module-level cached function
because an lru_cache on a method keeps the instance alive.
core/ no longer parses any assistant's transcript.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Storing had a 25s budget on the assumption it was the slow operation. It is not: memories.add returns a run id as soon as the server accepts the work — 0.39s measured — and the pipeline runs server-side, tracked through runs if anyone wants it. The hook never waits on that. The SDK applies its timeout per HTTP request, so every call the plugin makes is a single quick one and nothing needs a longer budget. One DEFAULT_TIMEOUT of 10s replaces the three values, which also drops a parameter from get_client: the caller supplies an assistant, not a budget. Ten seconds is a ceiling for a slow network rather than an expected cost, and stays well inside the 30s the host caps UserPromptSubmit at, leaving room for the venv build on a cold first prompt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
transcript() read payload["transcript_path"] directly, which stated as fact that every assistant uses that key. It happens to be true — Claude Code and Codex both document it among the fields every hook receives, unlike the turn id, which each spells differently — but the base class asserted it silently, and an assistant that disagreed would have had to override transcript() and copy the JSONL walk just to change a dictionary lookup. The lookup is now its own overridable method, so the key and the format are separate decisions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dannyjameswilliams
changed the title
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
core/ held both halves of the assistant split: the abstract base that core.hooks is written against, and the Claude Code and Codex classes implementing it. The second is vendor code in the generic half, and it grows with every assistant added. The contract stays in core/assistant.py, because core.hooks depends on it — moving it out would invert the dependency this change exists to establish. The implementations move to assistants/, and the composition root to entry/. core now imports nothing outward, so a new assistant never touches it. Grouped by role rather than by vendor, following sqlalchemy.dialects and django.db.backends: a module that outgrows one file becomes a package in place, with no top-level directory per vendor. A vendor directory could not have been consistent regardless, since Claude Code pins .claude-plugin/plugin.json to the plugin root. Pure move: no function body changed, and there is still one search.run and one store.run shared by every assistant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four things the directory move invalidated or revealed: core/__init__.py claimed assistants "differ only in their plugin manifest". This PR disproved that — they differ in transcript shape, turn id and the exit code a failed store should use. core/hooks/__init__.py documented `python -m core.hooks.search` as the way to run a hook. That has not worked since the hooks started taking an assistant; entry/ invokes them. client.py guarded its Assistant import with TYPE_CHECKING, which bought nothing: it already imports the SDK at module level, and core.assistant imports nothing from core, so there is no cycle to avoid. client_origin.py does still need the guard, but for the opposite reason to the one its comment gave — the tests load it by path, where a relative import has no package. Both hooks split their imports in two blocks with a noqa to satisfy an import sorter this repo does not run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
store.py's docstring described Claude Code's exit-2 wake as what the hook does. It is now one of two behaviours, chosen by the assistant, and the docstring said the opposite of what Codex gets. Both hook modules also kept a shebang and the executable bit from when they were invoked directly. run() takes an assistant and has no __main__ block, so running either file does nothing; entry/ is what the hooks files invoke. Also drops "host" where this code now says "assistant", and names Claude Code in the comment about the 30s UserPromptSubmit cap, which is its rule rather than a general one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tests listed the two assistants by hand, so a third could be added, registered and left unwired without failing anything. They now iterate assistants.__all__ and require, for each: an entry module, a manifest naming its own hooks file, and commands in that file invoking that entry module. Verified by adding a half-wired assistant and watching it fail. Adds the check the base class existed for and nobody had written — that an incomplete subclass raises at instantiation — and one that the manifests agree on a version, which nothing else keeps in step. search.py recomputed turn_id three times where store.py hoists it, skipped the debug line on the no-client path that store.py has, and carried trailing whitespace. The CI job was still called "unittest" after it started running mypy too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three comments described the plugin dir as holding "the core package", which was true until assistants/ and entry/ moved out of core. with-venv.sh puts the plugin root on PYTHONPATH for all three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_store_hook.py imported assistants and core at module level without putting the plugin root on sys.path, so running the file directly failed on the import despite its __main__ block. test_client_origin.py had no __main__ block at all, so running it defined the classes and exited 0 having tested nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fuzzing both hooks with malformed and wrong-typed stdin and transcripts found two reachable crashes. A transcript read while it is being written can split a UTF-8 sequence, and the whole-file read raised UnicodeDecodeError past the OSError guard. Reading with errors="replace" loses the damaged line to the json.loads guard and keeps the rest of the file. json.load accepts arrays, strings and numbers, so a payload that was valid JSON but not an object reached .get and failed with an AttributeError deep in a hook. read_input now rejects it with the clear failure its docstring already promised. Malformed JSON still raises, which is the documented contract — a hook should not run on garbage input — and a wrongly-typed field still crashes rather than being coerced, because searching for the string "42" is worse than failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t need Folds the entry/ package into the assistant modules: each is now runnable as `python -m assistants.<name> <hook>`, so adding an assistant is one file. The dispatch moves to core/hooks, where it belongs anyway — it is not assistant-specific. assistants/__init__.py must stay free of imports for that to work. A package that imports its own module makes `python -m` execute the module twice, which produces two distinct classes and a RuntimeWarning on stderr for every hook invocation. entry/ never had that problem because the runnable module was not part of its package's API. A test now runs each assistant as a subprocess and fails on the warning, so the constraint is enforced rather than hoped for. Reverts what this PR did not need: mypy and its CI step, the PLUGIN_ROOT/DATA fallbacks in with-venv.sh, ensure_deps.py and util.data_dir (Codex documents the CLAUDE_ names as aliases, so they already work), the assistant-aware client origin header and the get_client parameter that only fed it, a comment-only edit to requirements.txt, and a read_input guard for a payload shape no assistant sends. The client header still reports claude-plugin for Codex traffic. That is a one-line follow-up when Codex ships to real users, not something this PR has to carry. 29 files and 948 changed lines down to 20 and 754. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
client_origin_header and get_client take the assistant's name as a string, defaulting to "claude", so Codex traffic reports codex-plugin instead of claude-plugin. ClaudeCode.NAME is "claude" so its header is unchanged from main, and matches the default used by the schema fetch and migration CLI — one label per Claude session, not two. Tests now find each assistant's module from its class rather than deriving it from NAME by convention. Also trims diff the PR did not need: the manifest's author field reformatted, resolve_scope's call reflowed around a hoisted variable, the shebangs and exec bits on core/hooks, and a debug line on a path engram_warning already guards. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
store.py's opening sentence only needed "(asyncRewake)" removed, and its except-block comment was rewritten when it only needed deleting — the base class and module docstring already explain the exit code. with-venv.sh had two comment lines rewrapped for a one-phrase fix, and read_input gained an annotation on a function this PR does not otherwise touch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for improving plugin with various fixes.
Main change here is the refactor for multiple different assistant types. Each now has a class based on
Assistantwhich has methods such aslast_user_text,turn_id(prompt id),transcript, etc. Since these are all different per-assistant, this allows the entry points for search and store to take theassistantargument and re-use that shared logic. A lot of assistant-based logic used to live in separate files e.g.transcript.pybut now moved to assistant-level.Currently supports Claude Code and Codex.
Other changes in this PR:
ENGRAM_TIMEOUTvariable that can control timeoutdebugfunction that writes stderr when in debug modeis_automatedto find if a prompt is human written, and ignore storing memories on non-human memories