diff --git a/eval/harbor/README.md b/eval/harbor/README.md index 2f77b7344..4dd0da2e6 100644 --- a/eval/harbor/README.md +++ b/eval/harbor/README.md @@ -228,6 +228,10 @@ aggregate accuracy; each trial dir has the agent's stream-json log under --ak version=1.2.1 # pin the clawcodex-cli PyPI version --ak source=git+https://github.com/agentforce314/clawcodex@main # install from git instead of PyPI (unreleased code) + --ak source=dist/clawcodex_cli-1.4.0-py3-none-any.whl + # a local wheel: uploaded into each container and + # installed there, so a working tree can be + # benchmarked without pushing (uv build --wheel) --ak subscription=true # Claude Pro/Max OAuth instead of ANTHROPIC_API_KEY # Pass the key explicitly instead of exporting it @@ -239,6 +243,32 @@ aggregate accuracy; each trial dir has the agent's stream-json log under --model anthropic/claude-opus-4-5 # needs ANTHROPIC_API_KEY ``` +## Measuring prefix-cache efficiency + +`prefix_cache_probe.py` answers "how many tokens is each request re-sending?", +which is the number that actually moves cost on DeepSeek. Aggregate hit rate +hides the failure mode: a harness can sit at 90% while re-billing the same +multi-thousand-token block every single turn. + +```bash +# 1. Record — wraps any clawcodex invocation, capturing every wire payload +python eval/harbor/prefix_cache_probe.py record --out /tmp/pl -- \ + --print --dangerously-skip-permissions \ + --model deepseek-v4-flash --provider deepseek -- "your task" + +# 2. Analyse — diff consecutive requests, attribute the misses +python eval/harbor/prefix_cache_probe.py analyse --out /tmp/pl +``` + +`analyse` prints, per consecutive pair, the longest common message prefix and +the bytes that had to be recomputed, next to the provider's own +`cached_tokens`. A healthy session diverges only at the append point. Anything +re-sent every turn (the DeepSeek REQUEST-scope tail) shows up immediately. + +Reference points, terminal-bench 2.1 on deepseek-v4-flash: Reasonix 98.24% hit +/ ~1,295 miss tokens per request; clawcodex ~1,600-3,400 after the tail split +(~6,764 before it). + ## Notes - The model name uses Harbor's `provider/model` form; the adapter splits it diff --git a/eval/harbor/clawcodex_agent.py b/eval/harbor/clawcodex_agent.py index 4640e9143..fe5f112c4 100644 --- a/eval/harbor/clawcodex_agent.py +++ b/eval/harbor/clawcodex_agent.py @@ -140,6 +140,12 @@ * ``source`` — full pip-installable spec overriding the PyPI package, e.g. ``git+https://github.com/agentforce314/clawcodex@main`` to eval unreleased code. Mutually exclusive with ``version``. + + A path to a local ``.whl``/``.tar.gz`` on the host also works and is the + fast loop for harness changes that are not pushed anywhere: the file is + uploaded into each container and installed from there, so a working-tree + build can be benchmarked without a commit. Build one with + ``uv build --wheel`` and pass ``--ak source=dist/clawcodex_cli-…-py3-none-any.whl``. * ``subscription`` — ``true`` to authenticate the Anthropic provider with a Claude Pro/Max subscription instead of an API key. Reads the host's ``~/.clawcodex/anthropic-oauth.json`` (created by ``clawcodex login``; @@ -390,6 +396,20 @@ def __init__( self._subscription = parse_bool_env_value(subscription, name="subscription") self._source = source + # A ``source`` that resolves to a real file on the host is a + # working-tree build to upload rather than a spec for uv to resolve + # over the network. Resolved once, here, so ``install`` fails fast on + # a typo'd path instead of once per container. + self._local_artifact: Path | None = None + if source and not source.startswith(("git+", "http://", "https://")): + candidate = Path(source).expanduser() + if candidate.is_file(): + self._local_artifact = candidate.resolve() + elif candidate.suffix in (".whl", ".gz") or "/" in source: + raise ValueError( + f"Agent kwarg 'source' looks like a local path but does not " + f"exist: {candidate}" + ) # ``advisor`` is ``:`` — the reviewer model the # worker consults through the advisor tool. Same rationale as # ``fusion`` for living here rather than in CLI_FLAGS: it is config, @@ -509,7 +529,13 @@ async def install(self, environment: BaseEnvironment) -> None: env={"DEBIAN_FRONTEND": "noninteractive"}, ) - if self._source: + if self._local_artifact is not None: + # Working-tree build: ship the artifact into the container and + # install from there. uv treats a bare path as a local install. + remote = f"/tmp/{self._local_artifact.name}" + await environment.upload_file(self._local_artifact, remote) + install_spec = remote + elif self._source: install_spec = self._source elif self._version: install_spec = f"clawcodex-cli=={self._version}" diff --git a/eval/harbor/prefix_cache_probe.py b/eval/harbor/prefix_cache_probe.py new file mode 100644 index 000000000..841008bd2 --- /dev/null +++ b/eval/harbor/prefix_cache_probe.py @@ -0,0 +1,237 @@ +"""Measure DeepSeek prefix-cache efficiency of a real clawcodex session. + +Prefix caches bill from the first changed byte onward, so the metric that +matters is not "hit rate" in the abstract but **how many tokens each request +re-sends**. A harness can look healthy at 90% and still be re-billing a +multi-thousand-token block every single turn — that is exactly the bug this +script was written to find (a ~3.4K-token static block sitting in the +relocated tail; see ``build_memory_prompt_parts``). + +Two modes: + + # 1. Record: run any clawcodex command, capturing every wire payload. + python eval/harbor/prefix_cache_probe.py record --out /tmp/pl -- \ + --print --dangerously-skip-permissions \ + --model deepseek-v4-flash --provider deepseek -- "your task" + + # 2. Analyse: diff consecutive payloads and attribute the misses. + python eval/harbor/prefix_cache_probe.py analyse --out /tmp/pl + +``analyse`` reports, per consecutive pair, the longest common message prefix +and the exact bytes that had to be recomputed, alongside the provider's own +``cached_tokens`` so the model of the cache can be checked against reality. +A healthy session diverges only at the append point: everything before the +newest assistant/tool messages is shared, and the relocated tail is small. + +Reference points, terminal-bench 2.1, deepseek-v4-flash: + Reasonix 98.24% hit, ~1,295 miss tokens/request + clawcodex 90.23% hit, ~6,764 miss tokens/request (before the split fixes) +""" +from __future__ import annotations + +import argparse +import glob +import json +import os +import sys +import threading + + +# --------------------------------------------------------------------------- # +# record +# --------------------------------------------------------------------------- # + +def _install_recorder(out_dir: str) -> None: + """Patch the OpenAI SDK so every chat payload lands on disk. + + Hooking the SDK rather than clawcodex's provider means the capture is the + literal wire content — no risk of measuring a pre-serialisation shape that + differs from what DeepSeek's cache actually keys on. + """ + os.makedirs(out_dir, exist_ok=True) + from openai.resources.chat import completions as _c + + orig = _c.Completions.create + counter = [0] + lock = threading.Lock() + + def patched(self, *args, **kwargs): + with lock: + counter[0] += 1 + idx = counter[0] + try: + with open(os.path.join(out_dir, f"req-{idx:04d}.json"), "w") as fh: + json.dump( + { + "idx": idx, + "model": kwargs.get("model"), + "messages": kwargs.get("messages"), + "tools": kwargs.get("tools"), + }, + fh, + ) + except Exception as exc: # never break the session being measured + print(f"[probe] dump failed: {exc}", file=sys.stderr) + + result = orig(self, *args, **kwargs) + if kwargs.get("stream"): + return _UsageCapturingStream(result, out_dir, idx) + _write_usage(out_dir, idx, getattr(result, "usage", None)) + return result + + _c.Completions.create = patched + + +def _write_usage(out_dir: str, idx: int, usage) -> None: + if usage is None: + return + try: + with open(os.path.join(out_dir, f"usage-{idx:04d}.json"), "w") as fh: + json.dump(usage.model_dump(), fh) + except Exception: + pass + + +class _UsageCapturingStream: + """Transparent proxy that persists the terminal usage chunk.""" + + def __init__(self, inner, out_dir, idx): + self._inner, self._out, self._idx = inner, out_dir, idx + + def __iter__(self): + for chunk in self._inner: + _write_usage(self._out, self._idx, getattr(chunk, "usage", None)) + yield chunk + + def __getattr__(self, name): + return getattr(self._inner, name) + + def close(self): + return self._inner.close() + + def __enter__(self): + self._inner.__enter__() + return self + + def __exit__(self, *exc): + return self._inner.__exit__(*exc) + + +def _cmd_record(out_dir: str, argv: list[str]) -> int: + _install_recorder(out_dir) + sys.argv = ["clawcodex"] + argv + from src.cli import main + + return main() or 0 + + +# --------------------------------------------------------------------------- # +# analyse +# --------------------------------------------------------------------------- # + +def _norm(msg) -> str: + return json.dumps(msg, sort_keys=True, ensure_ascii=False) + + +def _describe(msg, limit=100) -> str: + content = msg.get("content") + if isinstance(content, list): + content = " ".join( + str(b.get("text") or b.get("type")) for b in content if isinstance(b, dict) + ) + text = (content or "").replace("\n", "\\n") + if msg.get("tool_calls"): + names = ",".join( + str(tc.get("function", {}).get("name")) for tc in msg["tool_calls"] + ) + text = f"[tool_calls: {names}] {text}" + return f"{msg.get('role'):9s} {len(_norm(msg)):7d}ch {text[:limit]}" + + +def _cmd_analyse(out_dir: str) -> int: + requests = [] + for path in sorted(glob.glob(os.path.join(out_dir, "req-*.json"))): + record = json.load(open(path)) + usage_path = os.path.join(out_dir, f"usage-{record['idx']:04d}.json") + record["usage"] = ( + json.load(open(usage_path)) if os.path.exists(usage_path) else None + ) + requests.append(record) + + if not requests: + print(f"no payloads in {out_dir}") + return 1 + + print(f"{len(requests)} requests in {out_dir}") + tools_sig = _norm(requests[0].get("tools")) + print(f"tools payload: {len(tools_sig)} chars, " + f"{len(requests[0].get('tools') or [])} tools") + for r in requests[1:]: + if _norm(r.get("tools")) != tools_sig: + print(f"!! TOOLS PAYLOAD CHANGED at request {r['idx']} " + f"— this busts the whole prefix") + + total_prompt = total_cached = 0 + for r in requests: + usage = r.get("usage") or {} + details = usage.get("prompt_tokens_details") or {} + cached = details.get("cached_tokens", usage.get("prompt_cache_hit_tokens", 0)) + total_prompt += usage.get("prompt_tokens", 0) + total_cached += cached or 0 + + for a, b in zip(requests, requests[1:]): + na = [_norm(m) for m in a["messages"]] + nb = [_norm(m) for m in b["messages"]] + i = 0 + while i < min(len(na), len(nb)) and na[i] == nb[i]: + i += 1 + recompute = sum(len(x) for x in nb[i:]) + print( + f"\nreq {a['idx']}->{b['idx']}: {len(na)}->{len(nb)} msgs | " + f"common prefix {i} msgs | recompute {recompute} ch " + f"(~{recompute // 4} tok)" + ) + if len(na) - i: + print(" -- invalidated from the OLD request --") + for m in a["messages"][i:i + 2]: + print(" ", _describe(m)) + print(" -- recomputed --") + for m in b["messages"][i:i + 5]: + print(" ", _describe(m)) + + if total_prompt: + miss = total_prompt - total_cached + print( + f"\nWIRE TOTALS: prompt={total_prompt:,} cached={total_cached:,} " + f"miss={miss:,} hit={total_cached / total_prompt:.2%} " + f"| avg miss/request={miss // len(requests):,}" + ) + return 0 + + +def main() -> int: + # Split on the first bare ``--`` by hand rather than leaning on argparse's + # REMAINDER: REMAINDER swallows every later flag, so ``--out`` placed after + # the mode would silently land in the child argv and the probe would write + # to its default directory instead. + argv = sys.argv[1:] + passthrough: list[str] = [] + if "--" in argv: + idx = argv.index("--") + argv, passthrough = argv[:idx], argv[idx + 1:] + + parser = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument("mode", choices=("record", "analyse")) + parser.add_argument("--out", default="/tmp/clawcodex-payloads") + args = parser.parse_args(argv) + + if args.mode == "record": + return _cmd_record(args.out, passthrough) + return _cmd_analyse(args.out) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/context_system/__init__.py b/src/context_system/__init__.py index d19215fcb..184cf1045 100644 --- a/src/context_system/__init__.py +++ b/src/context_system/__init__.py @@ -1,6 +1,6 @@ from __future__ import annotations -from .builder import build_context_prompt +from .builder import build_context_prompt, build_context_prompt_parts from .prompt_assembly import ( append_system_context, clear_context_caches, @@ -31,6 +31,7 @@ __all__ = [ # Legacy (backward compat) "build_context_prompt", + "build_context_prompt_parts", # Prompt assembly (WS-5) "append_system_context", "clear_context_caches", diff --git a/src/context_system/builder.py b/src/context_system/builder.py index 9e2661718..1eaa55d6d 100644 --- a/src/context_system/builder.py +++ b/src/context_system/builder.py @@ -13,36 +13,63 @@ from pathlib import Path -def build_context_prompt( +def build_context_prompt_parts( workspace_root: str | Path, *, cwd: str | Path | None = None, -) -> str: - """ - Build a context prompt string (legacy API). - - Uses the new WS-5 context system under the hood. - For new code, prefer fetch_system_prompt_parts() directly. +) -> tuple[str, str]: + """Split the context prompt into ``(snapshot, instructions)``. + + * ``snapshot`` — ``## Runtime Context`` + ``## Git Context``. A live view of + the workspace; ``git status`` moves the moment the agent edits a file, so + for a coding agent this changes essentially every turn. + * ``instructions`` — ``## Project Instructions``, i.e. the CLAWCODEX.md + bodies. Read once and fixed for the life of the session. + + Callers that place these separately can keep ``instructions`` in the cached + prefix and relocate only ``snapshot``. That matters because the relocated + tail sits *after* the conversation and is therefore re-sent — and re-billed + as a prefix-cache miss — on every request, so a large CLAWCODEX.md was + costing its full token count per turn. Same reasoning as + :func:`src.memdir.build_memory_prompt_parts`. + + Either half may be empty. ``build_context_prompt`` joins them back in this + order, so its output is unchanged. """ root = Path(workspace_root).expanduser().resolve() current = Path(cwd).expanduser().resolve() if cwd is not None else root - sections: list[str] = [] + snapshot_sections: list[str] = [] # Workspace info section - sections.append(_build_workspace_section(root, current)) + snapshot_sections.append(_build_workspace_section(root, current)) # Git context (sync wrapper around async collect) git_section = _build_git_section(str(root)) if git_section: - sections.append(git_section) + snapshot_sections.append(git_section) # CLAWCODEX.md context (sync wrapper around async get_memory_files) claude_section = _build_clawcodex_md_section(str(current), root) - if claude_section: - sections.append(claude_section) - return "\n\n".join(section for section in sections if section.strip()) + snapshot = "\n\n".join(s for s in snapshot_sections if s.strip()) + instructions = claude_section if claude_section and claude_section.strip() else "" + return snapshot, instructions + + +def build_context_prompt( + workspace_root: str | Path, + *, + cwd: str | Path | None = None, +) -> str: + """ + Build a context prompt string (legacy API). + + Uses the new WS-5 context system under the hood. + For new code, prefer fetch_system_prompt_parts() directly. + """ + snapshot, instructions = build_context_prompt_parts(workspace_root, cwd=cwd) + return "\n\n".join(part for part in (snapshot, instructions) if part) def _run_async(coro): diff --git a/src/context_system/prompt_assembly.py b/src/context_system/prompt_assembly.py index 8d37874db..9526d7478 100644 --- a/src/context_system/prompt_assembly.py +++ b/src/context_system/prompt_assembly.py @@ -520,10 +520,8 @@ def build_full_system_prompt( if env_section: sections.append(env_section) - # 25. Auto-memory section (MEMORY.md + behavioral instructions) - memory_section = _build_memory_section() - if memory_section: - sections.append(memory_section) + # 25. Auto-memory doctrine (SESSION) + 26. MEMORY.md index (REQUEST) + sections.extend(_build_memory_sections()) # 26. Bounded persistent-memory snapshot (hermes-agent port) memory_store_section = _build_memory_store_section() @@ -694,9 +692,7 @@ def build_full_system_prompt_blocks( env_section = _build_env_section(cwd, use_cache) if env_section: sections.append(env_section) - memory_section = _build_memory_section() - if memory_section: - sections.append(memory_section) + sections.extend(_build_memory_sections()) memory_store_section = _build_memory_store_section() if memory_store_section: sections.append(memory_store_section) @@ -1260,29 +1256,74 @@ def _build_env_section(cwd: str | None, use_cache: bool) -> SystemPromptSection return SystemPromptSection(id="environment", content=content, cache_scope=CacheScope.REQUEST, order=20) -def _build_memory_section() -> SystemPromptSection | None: - """Build the auto-memory system-prompt section. +def _build_memory_sections() -> list[SystemPromptSection]: + """Build the auto-memory system-prompt section(s), split by volatility. Mirrors TS ``constants/prompts.ts:495`` (``systemPromptSection('memory', - () => loadMemoryPrompt())``). Returned with ``REQUEST`` scope so any - upstream caching layer rebuilds the section per render — ``MEMORY.md`` - can change mid-session as the model writes to it, and none of the - existing cache scopes invalidate on file mtime. The work is small - (one read of a ≤25KB file), so correctness over cache hit-rate. + () => loadMemoryPrompt())``), but emits up to two sections instead of one: + + * ``memory`` — the typed-memory doctrine, at ``SESSION`` scope. It + interpolates only the memory directory path, which is fixed for the life + of the process, so these bytes are identical on every render. + * ``memory_index`` — the ``## MEMORY.md`` body, at ``REQUEST`` scope. This + genuinely can change mid-session as the model writes to it, and no + existing cache scope invalidates on file mtime. + + Why the split, rather than one REQUEST-scope section as before: for + DeepSeek, ``query`` relocates REQUEST-scope sections into a trailing + message placed *after* the conversation history, to keep the cached prefix + byte-stable. Anything in that tail is therefore re-sent — and re-billed as + a cache miss — on every single request. The combined section is ~3.4K + tokens of which ~95% is doctrine, so the old scoping paid ~3.4K miss + tokens per turn to keep a ~150-token index fresh. Measured on + terminal-bench 2.1 that was the largest single contributor to the gap + between clawcodex's 90.2% cache hit rate and Reasonix's 98.2%. + + Effect on other providers, precisely: + + * ``build_full_system_prompt`` (the flattened-string path) sorts purely by + section ``order``, and 25-then-26 preserves the original adjacency, so + its output is byte-for-byte unchanged. + * ``build_full_system_prompt_blocks`` (Anthropic) emits GLOBAL → boundary + → SESSION → REQUEST, so the doctrine block does move earlier — out of + the volatile REQUEST group and into the cache_control-marked SESSION + group. That is a deliberate improvement, not a regression: ~13KB stops + riding the least-cacheable group. It is a prompt-order change, though, + so it is not a no-op there. """ try: - from src.memdir import load_memory_prompt + from src.memdir import load_memory_prompt_parts except Exception: - return None - content = load_memory_prompt() - if not content: - return None - return SystemPromptSection( - id="memory", - content=content, - cache_scope=CacheScope.REQUEST, - order=25, - ) + return [] + try: + guidance, index = load_memory_prompt_parts() + except Exception: + return [] + sections: list[SystemPromptSection] = [] + if guidance: + sections.append( + SystemPromptSection( + id="memory", + # The doctrine's last line is blank, so ``guidance`` ends in a + # newline. Sections are joined with "\n\n" whereas the old + # single section joined these two halves with "\n"; rstripping + # here makes the flattened prompt byte-identical to before for + # every provider that does not relocate REQUEST scope. + content=guidance.rstrip("\n"), + cache_scope=CacheScope.SESSION, + order=25, + ) + ) + if index: + sections.append( + SystemPromptSection( + id="memory_index", + content=index, + cache_scope=CacheScope.REQUEST, + order=26, + ) + ) + return sections #: Behavioral guidance injected alongside the bounded-memory snapshot @@ -1411,6 +1452,11 @@ def _build_mcp_instructions_section( return SystemPromptSection( id="mcp_instructions", content=content, + # Deliberately REQUEST, unlike the other static-ish sections re-scoped + # for prefix caching: MCP servers can connect mid-session (on-demand + # start), so this block genuinely can change between turns. Chapter C2 + # / PR #650 pinned that split on purpose — see + # tests/test_mcp_instructions_live_wiring.py. cache_scope=CacheScope.REQUEST, order=31, ) @@ -1493,7 +1539,10 @@ def _build_output_style_section( def _build_non_interactive_section(use_cache: bool) -> SystemPromptSection | None: - return SystemPromptSection(id="non_interactive", content=_NON_INTERACTIVE_PROMPT, cache_scope=CacheScope.REQUEST, order=80) + # SESSION, not REQUEST: the content is a module constant, so it cannot + # differ between turns. REQUEST-scope sections are relocated behind the + # conversation for DeepSeek and re-billed as a cache miss every request. + return SystemPromptSection(id="non_interactive", content=_NON_INTERACTIVE_PROMPT, cache_scope=CacheScope.SESSION, order=80) def _build_tool_restrictions_section( @@ -1505,4 +1554,7 @@ def _build_tool_restrictions_section( for r in restrictions: parts.append(f"- {r}") content = "\n".join(parts) - return SystemPromptSection(id="tool_restrictions", content=content, cache_scope=CacheScope.REQUEST, order=90) + # SESSION: the restriction list is fixed when the session's tool set is + # resolved, so this never changes turn to turn (see the non_interactive + # note above for why REQUEST is expensive). + return SystemPromptSection(id="tool_restrictions", content=content, cache_scope=CacheScope.SESSION, order=90) diff --git a/src/memdir/__init__.py b/src/memdir/__init__.py index 48b2218d4..e4ba91a08 100644 --- a/src/memdir/__init__.py +++ b/src/memdir/__init__.py @@ -20,8 +20,10 @@ MAX_ENTRYPOINT_LINES, build_memory_lines, build_memory_prompt, + build_memory_prompt_parts, ensure_memory_dir_exists, load_memory_prompt, + load_memory_prompt_parts, truncate_entrypoint_content, ) from .memory_age import ( @@ -105,8 +107,10 @@ "MAX_ENTRYPOINT_LINES", "build_memory_lines", "build_memory_prompt", + "build_memory_prompt_parts", "ensure_memory_dir_exists", "load_memory_prompt", + "load_memory_prompt_parts", "truncate_entrypoint_content", # scan / recall "FRONTMATTER_MAX_LINES", diff --git a/src/memdir/memdir.py b/src/memdir/memdir.py index 1541cc8ac..3514cf87c 100644 --- a/src/memdir/memdir.py +++ b/src/memdir/memdir.py @@ -290,18 +290,33 @@ def build_memory_lines( return lines -def build_memory_prompt( +def build_memory_prompt_parts( *, display_name: str, memory_dir: str, extra_guidelines: Iterable[str] | None = None, -) -> str: - """Assemble the memory section, including ``MEMORY.md`` body. - - Reads ``MEMORY.md`` synchronously, runs through - :func:`truncate_entrypoint_content`, and appends after the prose - lines. Used by both auto-memory (this module's - :func:`load_memory_prompt`) and the eventual agent-memory variant. +) -> tuple[str, str]: + """Split the memory section into its stable and volatile halves. + + Returns ``(guidance, index)``: + + * ``guidance`` — the typed-memory behavioral doctrine. Interpolates only + ``display_name`` and ``memory_dir``, both fixed for the life of a + session, so these bytes never change between turns. + * ``index`` — the ``## MEMORY.md`` heading plus the entrypoint body, which + the model rewrites mid-session via the Memory tool. + + The split exists for prefix caching. DeepSeek (and any other automatic + prefix cache) re-bills every token from the first changed byte onward, and + ``query`` relocates REQUEST-scope prompt sections to a trailing message + *after* the conversation — so anything left in that tail is recomputed on + every single request. Tagging the whole memory section REQUEST-scope put + ~3.4K tokens of unchanging doctrine in that tail and re-billed it every + turn; measured against terminal-bench 2.1 that was the single largest + source of cache misses. Only ``index`` genuinely belongs there. + + ``build_memory_prompt`` remains the concatenation of the two, so callers + that want the whole section are byte-for-byte unaffected. """ entrypoint_path = Path(memory_dir) / ENTRYPOINT_NAME entrypoint_content = "" @@ -310,7 +325,7 @@ def build_memory_prompt( except (FileNotFoundError, OSError): entrypoint_content = "" - lines = build_memory_lines( + guidance_lines = build_memory_lines( display_name=display_name, memory_dir=memory_dir, extra_guidelines=extra_guidelines, @@ -318,37 +333,55 @@ def build_memory_prompt( if entrypoint_content.strip(): truncation = truncate_entrypoint_content(entrypoint_content) - lines.extend([f"## {ENTRYPOINT_NAME}", "", truncation.content]) + index_lines = [f"## {ENTRYPOINT_NAME}", "", truncation.content] else: - lines.extend( - [ - f"## {ENTRYPOINT_NAME}", - "", - f"Your {ENTRYPOINT_NAME} is currently empty. When you save new memories, they will appear here.", - ] - ) + index_lines = [ + f"## {ENTRYPOINT_NAME}", + "", + f"Your {ENTRYPOINT_NAME} is currently empty. When you save new memories, they will appear here.", + ] - return "\n".join(lines) + return "\n".join(guidance_lines), "\n".join(index_lines) -def load_memory_prompt() -> str | None: - """Top-level dispatch for the auto-memory system prompt section. +def build_memory_prompt( + *, + display_name: str, + memory_dir: str, + extra_guidelines: Iterable[str] | None = None, +) -> str: + """Assemble the memory section, including ``MEMORY.md`` body. - Returns ``None`` when auto-memory is disabled. Otherwise creates - the memory directory(ies) if they do not exist and returns the - assembled prompt section. + Reads ``MEMORY.md`` synchronously, runs through + :func:`truncate_entrypoint_content`, and appends after the prose + lines. Used by both auto-memory (this module's + :func:`load_memory_prompt`) and the eventual agent-memory variant. - Dispatch order matches TS ``loadMemoryPrompt``: + Equivalent to joining :func:`build_memory_prompt_parts` with a newline; + that function is the one to reach for when the caller needs to place the + stable and volatile halves at different points in the request. + """ + guidance, index = build_memory_prompt_parts( + display_name=display_name, + memory_dir=memory_dir, + extra_guidelines=extra_guidelines, + ) + return f"{guidance}\n{index}" - 1. Auto-memory disabled → ``None``. - 2. Team memory enabled → combined private + team prompt. - 3. Else → single-directory auto-memory prompt. - KAIROS daily-log mode is still deferred (Slice D in the refactor - plan). +def load_memory_prompt_parts() -> tuple[str | None, str | None]: + """``load_memory_prompt`` split into ``(guidance, index)``. + + Same dispatch and same directory side effects; the only difference is that + the caller receives the session-stable doctrine separately from the + mutable ``MEMORY.md`` body so each can be placed in the request where it + belongs (see :func:`build_memory_prompt_parts` for why that matters). + + The team-memory prompt carries no entrypoint body at all — it is doctrine + end to end — so that branch returns ``index=None``. """ if not is_auto_memory_enabled(): - return None + return None, None # Lazy import to avoid a circular import at module load time # (team_mem_prompts imports from this module). @@ -361,11 +394,33 @@ def load_memory_prompt() -> str | None: # parents=True creates auto_dir as a side effect. team_dir = get_team_mem_path() ensure_memory_dir_exists(team_dir) - return build_combined_memory_prompt() + return build_combined_memory_prompt(), None auto_dir = get_auto_mem_path() ensure_memory_dir_exists(auto_dir) - return build_memory_prompt( + return build_memory_prompt_parts( display_name=_AUTO_MEM_DISPLAY_NAME, memory_dir=auto_dir, ) + + +def load_memory_prompt() -> str | None: + """Top-level dispatch for the auto-memory system prompt section. + + Returns ``None`` when auto-memory is disabled. Otherwise creates + the memory directory(ies) if they do not exist and returns the + assembled prompt section. + + Dispatch order matches TS ``loadMemoryPrompt``: + + 1. Auto-memory disabled → ``None``. + 2. Team memory enabled → combined private + team prompt. + 3. Else → single-directory auto-memory prompt. + + KAIROS daily-log mode is still deferred (Slice D in the refactor + plan). + """ + guidance, index = load_memory_prompt_parts() + if guidance is None: + return None + return guidance if index is None else f"{guidance}\n{index}" diff --git a/src/query/agent_loop_compat.py b/src/query/agent_loop_compat.py index b9b229df1..267bdb64a 100644 --- a/src/query/agent_loop_compat.py +++ b/src/query/agent_loop_compat.py @@ -205,8 +205,8 @@ def build_effective_system_prompt( *auto-memory* (``MEMORY.md`` via ``load_memory_prompt``), **not** CLAWCODEX.md. On the engine path CLAWCODEX.md is injected into the *messages* via ``prepend_user_context``; the cutover does not do that, so we keep - ``build_context_prompt`` (which emits ``## Project Instructions``) to - preserve CLAWCODEX.md — option (b) in + ``build_context_prompt_parts`` (whose second half emits + ``## Project Instructions``) to preserve CLAWCODEX.md — option (b) in ``my-docs/get-parity-by-folder/live-base-system-prompt-gap-analysis.md``. This overlaps the base ``# Environment`` section on CWD/date (a benign, documented duplication). @@ -232,7 +232,7 @@ def build_effective_system_prompt( """ # Local imports — context_system is a heavier dep; only the cutover # callers need it, no need to drag it into agent_loop_compat's import time. - from ..context_system import build_context_prompt + from ..context_system import build_context_prompt_parts from ..context_system.prompt_assembly import build_full_system_prompt_blocks from ..context_system.system_prompt_cache import CacheScope from ..coordinator.mode import is_coordinator_mode @@ -288,27 +288,37 @@ def build_effective_system_prompt( skills=skills, ) - # Preserve the existing workspace + git + CLAWCODEX.md context verbatim as a - # trailing uncached block (CLAWCODEX.md is NOT in the base blocks above). + # Preserve the workspace + git + CLAWCODEX.md context (CLAWCODEX.md is NOT + # in the base blocks above), but as TWO trailing blocks split by volatility. # - # Tag it REQUEST-scope. This block is a *live workspace snapshot*: it embeds - # ``git status`` (and file counts / top-level entries) that mutate the moment - # the agent edits a file — which, for a coding agent, is essentially every - # turn. For DeepSeek, ``query._split_system_prompt_blocks`` relocates - # REQUEST-scope sections out of the byte-stable ``system + tools + history`` - # prefix into the trailing tail; without the tag this snapshot would sit in - # the prefix and a single mid-session file edit would bust DeepSeek's - # automatic prefix cache for the entire prefix. The tag honours the block's - # already-intended "uncached" status while keeping the prefix stable. It is a - # strict no-op for every other provider: relocation only fires for DeepSeek, - # and the Anthropic path strips ``_cache_scope`` before the wire. + # The snapshot half is REQUEST-scope. It embeds ``git status`` (and file + # counts / top-level entries) that mutate the moment the agent edits a file + # — for a coding agent, essentially every turn. For DeepSeek, + # ``query._split_system_prompt_blocks`` relocates REQUEST-scope sections out + # of the byte-stable ``system + tools + history`` prefix into a trailing + # tail; without the tag this snapshot would sit in the prefix and a single + # mid-session file edit would bust the cache for the whole prefix. + # + # The instructions half (``## Project Instructions``, i.e. CLAWCODEX.md) is + # SESSION-scope. It is read once and fixed for the session, and the tail is + # not free: it sits after the conversation, so it is re-sent and re-billed + # as a cache miss on EVERY request. A large CLAWCODEX.md was paying its full + # token count per turn for nothing. Keeping it in the cached prefix is the + # same trade Reasonix makes (internal/boot/boot.go: project instructions + # "fold into the system prompt exactly here, once ... so memory costs + # nothing per turn"). + # + # Snapshot is appended first so the flattened prompt keeps the order + # ``build_context_prompt`` produced; providers that do not relocate + # REQUEST scope therefore see unchanged bytes. try: - context_prompt = build_context_prompt( + context_snapshot, context_instructions = build_context_prompt_parts( tool_context.workspace_root, cwd=tool_context.cwd, ) except Exception: - context_prompt = "" + context_snapshot, context_instructions = "", "" + context_prompt = context_snapshot if coordinator: # workerToolsContext — TS merges this into the per-session userContext @@ -333,17 +343,31 @@ def build_effective_system_prompt( scratchpad_dir=scratchpad_dir, ).get("workerToolsContext", "") if worker_ctx: + # Session-stable (MCP server names + scratchpad dir), so it rides + # with the instructions half rather than the live snapshot. entry = f"# workerToolsContext\n{worker_ctx}" - context_prompt = ( - f"{context_prompt}\n\n{entry}" if context_prompt.strip() else entry + context_instructions = ( + f"{context_instructions}\n\n{entry}" + if context_instructions.strip() + else entry ) + # Snapshot first, instructions second: that is the order + # ``build_context_prompt`` produced, and providers which do not relocate + # REQUEST scope flatten blocks in list order — so their bytes are + # unchanged. Only DeepSeek pulls the REQUEST block out to the tail. if context_prompt.strip(): blocks = blocks + [{ "type": "text", "text": context_prompt, "_cache_scope": CacheScope.REQUEST.value, }] + if context_instructions.strip(): + blocks = blocks + [{ + "type": "text", + "text": context_instructions, + "_cache_scope": CacheScope.SESSION.value, + }] return blocks diff --git a/tests/coordinator/test_wiring.py b/tests/coordinator/test_wiring.py index e3db71eda..c74a55ad5 100644 --- a/tests/coordinator/test_wiring.py +++ b/tests/coordinator/test_wiring.py @@ -97,8 +97,13 @@ def test_prompt_branch_replaces_base_blocks( marked = [i for i, b in enumerate(blocks) if "cache_control" in b] assert marked == [0] assert blocks[0]["cache_control"]["type"] == "ephemeral" - # Trailing context block survives, REQUEST-scoped (DeepSeek splitter). - assert blocks[-1]["_cache_scope"] == "request" + # Trailing context survives. It is emitted as up to two blocks — the live + # workspace/git snapshot (REQUEST, relocated to the tail by the DeepSeek + # splitter) and the session-stable CLAWCODEX.md / workerToolsContext + # (SESSION, kept in the cached prefix) — so assert on the scopes present + # rather than on which one happens to be last. + trailing_scopes = {b.get("_cache_scope") for b in blocks[1:]} + assert "request" in trailing_scopes def test_prompt_branch_style_appends_and_carries_marker( @@ -175,7 +180,7 @@ def test_prompt_branch_off_matches_reference_composition( monkeypatch.delenv("CLAUDE_CODE_COORDINATOR_MODE", raising=False) from src.command_system import get_skill_tool_commands - from src.context_system import build_context_prompt + from src.context_system import build_context_prompt_parts from src.context_system.prompt_assembly import build_full_system_prompt_blocks from src.context_system.system_prompt_cache import CacheScope from src.query.agent_loop_compat import build_effective_system_prompt @@ -195,13 +200,26 @@ def test_prompt_branch_off_matches_reference_composition( mcp_servers=None, skills=skills, ) - ctx = build_context_prompt(tc.workspace_root, cwd=tc.cwd) - if ctx.strip(): + # Two trailing blocks, split by volatility: the live workspace/git snapshot + # stays REQUEST-scoped (relocated to the tail for DeepSeek), while + # CLAWCODEX.md is SESSION-scoped so it is cached in the prefix instead of + # being re-sent every request. Snapshot first — that order keeps the + # flattened prompt byte-identical for providers that do not relocate. + ctx_snapshot, ctx_instructions = build_context_prompt_parts( + tc.workspace_root, cwd=tc.cwd + ) + if ctx_snapshot.strip(): expected = expected + [{ "type": "text", - "text": ctx, + "text": ctx_snapshot, "_cache_scope": CacheScope.REQUEST.value, }] + if ctx_instructions.strip(): + expected = expected + [{ + "type": "text", + "text": ctx_instructions, + "_cache_scope": CacheScope.SESSION.value, + }] actual = build_effective_system_prompt("STYLE-REF", tc) diff --git a/tests/test_deepseek_prefix_cache.py b/tests/test_deepseek_prefix_cache.py index 1761c55be..757eaec36 100644 --- a/tests/test_deepseek_prefix_cache.py +++ b/tests/test_deepseek_prefix_cache.py @@ -350,19 +350,163 @@ def test_non_deepseek_prefix_changes_when_request_scope_block_changes(): assert sys1 != sys2 -def test_memory_section_is_request_scoped(): +def test_memory_index_is_request_scoped_and_doctrine_is_not(): """Regression guard tying the relocation guarantee to the real section - taxonomy: the auto-memory section embeds the mutable MEMORY.md body, so it - MUST stay REQUEST-scoped (relocated to the tail for DeepSeek). If it's ever - retagged SESSION/GLOBAL it would sit in the cached prefix and a memory - write would bust the whole history cache. Skips when no memory section is - produced in the test environment.""" - from src.context_system.prompt_assembly import _build_memory_section + taxonomy. + + Two halves, two scopes, and the split is the whole point: + + * ``memory_index`` embeds the mutable ``MEMORY.md`` body, so it MUST stay + REQUEST-scoped (relocated to the tail for DeepSeek). If it were ever + retagged SESSION/GLOBAL it would sit in the cached prefix and a memory + write would bust the whole history cache. + * ``memory`` is the typed-memory doctrine, which interpolates nothing that + changes within a session. It MUST NOT be REQUEST-scoped: the tail is + re-sent and re-billed as a cache miss on every single request, and this + block is ~3.4K tokens. Tagging it REQUEST cost ~3.4K miss tokens per turn + on terminal-bench 2.1 — the largest single contributor to the 90.2% vs + 98.2% cache-hit gap against Reasonix. + + Skips whichever half the test environment does not produce. + """ + from src.context_system.prompt_assembly import _build_memory_sections from src.context_system.system_prompt_cache import CacheScope - section = _build_memory_section() - if section is not None: - assert section.cache_scope is CacheScope.REQUEST + sections = {s.id: s for s in _build_memory_sections()} + + index = sections.get("memory_index") + if index is not None: + assert index.cache_scope is CacheScope.REQUEST + assert "MEMORY.md" in index.content + + doctrine = sections.get("memory") + if doctrine is not None: + assert doctrine.cache_scope is not CacheScope.REQUEST + # The mutable body must not have leaked back into the cached half. + assert "## MEMORY.md" not in doctrine.content + + +def test_relocated_tail_stays_within_a_token_budget(tmp_path, monkeypatch): + """Budget guard on the whole REQUEST-scope group, not just the memory half. + + Everything tagged REQUEST is relocated behind the conversation for + DeepSeek, which means it is re-sent on every request and re-billed at the + cache-miss rate — the prefix cache can never cover it. So the tail's size + is a direct, permanent per-turn tax, and the only things that earn a place + there are values that actually change between turns. + + The number below is deliberately generous (~500 tokens against a real tail + of roughly 200). It is a tripwire for a whole *section* being misfiled, not + a style rule: the auto-memory doctrine alone was 13.5KB in this group and + cost ~3.4K miss tokens on every single request. If this fails, do not raise + the budget — split the offending section and leave only its volatile part + behind, the way ``build_memory_prompt_parts`` and + ``build_context_prompt_parts`` do. + + MEMORY.md is pointed at an empty temp dir so a developer's real (and + legitimately large) memory index cannot make this flaky. + """ + monkeypatch.setenv("CLAUDE_COWORK_MEMORY_PATH_OVERRIDE", str(tmp_path)) + + blocks = build_full_system_prompt_blocks(cwd=str(tmp_path), non_interactive=True) + request_blocks = [b for b in blocks if b.get("_cache_scope") == "request"] + total = sum(len(b.get("text", "")) for b in request_blocks) + + detail = "\n".join( + f" {len(b.get('text', '')):6d} ch | " + f"{(b.get('text', '').strip().splitlines() or [''])[0][:70]}" + for b in request_blocks + ) + assert total < 2000, ( + f"REQUEST-scope group is {total} chars (~{total // 4} tokens); every one " + f"of those is re-sent and re-billed as a cache miss on every DeepSeek " + f"request. Blocks:\n{detail}" + ) + + +def test_relocation_conserves_content_exactly(tmp_path, monkeypatch): + """Relocation must move bytes, never drop or duplicate them. + + This is the capability guarantee behind the cache work: the model has to + see exactly the same content, just partitioned so the stable part can be + cached. Concatenating the DeepSeek ``system + tail`` must therefore yield + the same character count as the single flattened system prompt every other + provider receives — anything else means a section went missing (the model + is now blind to it) or got emitted twice (wasted tokens and a confusing + double instruction). + """ + monkeypatch.setenv("CLAUDE_COWORK_MEMORY_PATH_OVERRIDE", str(tmp_path)) + (tmp_path / "MEMORY.md").write_text("- [Thing](thing.md) — hook\n") + + blocks = build_full_system_prompt_blocks(cwd=str(tmp_path), non_interactive=True) + flat, empty_tail = _split_system_prompt_blocks(blocks, relocate_request_scope=False) + system, tail = _split_system_prompt_blocks(blocks, relocate_request_scope=True) + + assert empty_tail == "", "non-relocating providers must get no tail" + # Joining introduces separator whitespace, so compare on non-whitespace + # characters: that is invariant under how the two halves are glued. + def _dense(s): + return "".join(s.split()) + + assert _dense(system) + _dense(tail) == _dense(flat), ( + "DeepSeek's system+tail must carry exactly the flattened prompt's " + "content — no section dropped, none duplicated" + ) + assert tail, "something volatile should still be relocated" + + +def test_prefix_survives_a_mid_session_memory_write(tmp_path, monkeypatch): + """The split must not cost the freshness guarantee it was carved out of. + + Splitting doctrine into the cached prefix is only safe if the half that + actually changes stays behind. The model rewrites ``MEMORY.md`` mid-session + via the Memory tool; when it does, the DeepSeek system prefix must be + untouched (only the relocated tail moves), exactly as before the split. + Getting this wrong would trade a per-turn tax for something far worse — a + full prefix bust every time the agent saved a memory. + """ + monkeypatch.setenv("CLAUDE_COWORK_MEMORY_PATH_OVERRIDE", str(tmp_path)) + entrypoint = tmp_path / "MEMORY.md" + + entrypoint.write_text("- [First](first.md) — hook\n") + sys1, tail1 = _split_system_prompt_blocks( + build_full_system_prompt_blocks(cwd=str(tmp_path), non_interactive=True), + relocate_request_scope=True, + ) + entrypoint.write_text("- [First](first.md) — hook\n- [Second](second.md) — hook\n") + sys2, tail2 = _split_system_prompt_blocks( + build_full_system_prompt_blocks(cwd=str(tmp_path), non_interactive=True), + relocate_request_scope=True, + ) + + assert sys1 == sys2, "a MEMORY.md write must not perturb the DeepSeek prefix" + assert tail1 != tail2, "the new memory must actually reach the model" + assert "Second" in tail2 and "Second" not in sys2 + + +def test_memory_sections_rejoin_to_the_legacy_single_section(): + """The split must be a pure relocation, not a prompt edit. + + Providers that do not relocate REQUEST scope concatenate every section, so + doctrine + index have to rejoin into exactly the bytes the old single + ``memory`` section produced. ``build_full_system_prompt`` is covered + end-to-end elsewhere; this pins the memdir-level contract directly. + """ + import tempfile + from pathlib import Path + + from src.memdir import build_memory_prompt, build_memory_prompt_parts + + with tempfile.TemporaryDirectory() as tmp: + (Path(tmp) / "MEMORY.md").write_text("- [Thing](thing.md) — hook\n") + whole = build_memory_prompt(display_name="auto memory", memory_dir=tmp) + guidance, index = build_memory_prompt_parts( + display_name="auto memory", memory_dir=tmp + ) + assert f"{guidance}\n{index}" == whole + # And the halves are actually split on the intended seam. + assert "## MEMORY.md" not in guidance + assert index.startswith("## MEMORY.md") # --------------------------------------------------------------------------- # @@ -385,16 +529,24 @@ def test_memory_section_is_request_scoped(): def _effective_blocks(context_text, tmp_path): - """build_effective_system_prompt with build_context_prompt stubbed to a fixed - workspace snapshot, so the test pins the trailing-block scope deterministically.""" + """build_effective_system_prompt with the context builder stubbed to a fixed + workspace snapshot, so the test pins the trailing-block scope deterministically. + + ``context_text`` is the whole legacy blob; it is split on the + ``## Project Instructions`` seam to feed ``build_context_prompt_parts``, + which is what the builder now calls. + """ from unittest import mock from src.query.agent_loop_compat import build_effective_system_prompt from src.tool_system.context import ToolContext + head, sep, tail_txt = context_text.partition("## Project Instructions") + parts = (head.strip("\n"), (sep + tail_txt).strip("\n") if sep else "") + ctx = ToolContext(workspace_root=tmp_path) with mock.patch( - "src.context_system.build_context_prompt", return_value=context_text + "src.context_system.build_context_prompt_parts", return_value=parts ): return build_effective_system_prompt( "", ctx, provider=DeepSeekProvider(api_key=_KEY) @@ -407,15 +559,33 @@ def test_effective_prompt_tags_context_block_request_scope(tmp_path): assert ctx_block.get("_cache_scope") == "request" -def test_deepseek_relocates_git_context_out_of_prefix(tmp_path): - """The volatile workspace snapshot rides the tail; CLAWCODEX.md is preserved - there (not dropped) and is NOT left in the prefix.""" +def test_effective_prompt_keeps_project_instructions_out_of_request_scope(tmp_path): + """CLAWCODEX.md is read once and fixed for the session, so it must NOT ride + the relocated tail — everything there is re-sent and re-billed as a cache + miss on every request. Only the live workspace/git snapshot belongs there.""" + blocks = _effective_blocks(_CTX_GIT_T1, tmp_path) + instr = next(b for b in blocks if "CLAUDE_MD_SENTINEL" in b.get("text", "")) + assert instr.get("_cache_scope") != "request" + # ...and it must not have been merged back into the volatile block. + snapshot = next(b for b in blocks if "## Git Context" in b.get("text", "")) + assert "CLAUDE_MD_SENTINEL" not in snapshot.get("text", "") + + +def test_deepseek_relocates_git_context_but_caches_project_instructions(tmp_path): + """The volatile workspace snapshot rides the tail; CLAWCODEX.md stays in the + cached prefix. + + The prefix placement is the point: CLAWCODEX.md can be many KB and never + changes mid-session, so paying for it in the tail on every turn was pure + waste. It must still be present somewhere — dropping it would lose the + user's project instructions entirely. + """ blocks = _effective_blocks(_CTX_GIT_T1, tmp_path) system, tail = _split_system_prompt_blocks(blocks, relocate_request_scope=True) assert "## Git Context" not in system and "Status:" not in system assert "## Git Context" in tail - assert "CLAUDE_MD_SENTINEL" in tail - assert "CLAUDE_MD_SENTINEL" not in system + assert "CLAUDE_MD_SENTINEL" in system + assert "CLAUDE_MD_SENTINEL" not in tail def test_deepseek_prefix_stable_across_mid_session_git_change(tmp_path): @@ -475,8 +645,10 @@ def chat_stream_response(self, messages, tools=None, on_text_chunk=None, ctx = ToolContext(workspace_root=tmp_path) registry = build_default_registry(provider=provider) + head, sep, rest = _CTX_GIT_T1.partition("## Project Instructions") with mock.patch( - "src.context_system.build_context_prompt", return_value=_CTX_GIT_T1 + "src.context_system.build_context_prompt_parts", + return_value=(head.strip("\n"), (sep + rest).strip("\n")), ): system_prompt = build_effective_system_prompt("", ctx, provider=provider) @@ -492,10 +664,11 @@ def chat_stream_response(self, messages, tools=None, on_text_chunk=None, assert captured, "the DeepSeek provider was never called" wire = captured[0] - # System message (index 0) is free of the volatile snapshot + CLAWCODEX.md. + # System message (index 0) is free of the volatile snapshot, but DOES carry + # the session-stable CLAWCODEX.md so it is paid for once and then cached. assert wire[0]["role"] == "system" assert "## Git Context" not in wire[0]["content"] - assert "CLAUDE_MD_SENTINEL" not in wire[0]["content"] + assert "CLAUDE_MD_SENTINEL" in wire[0]["content"] # The relocated snapshot rides the LAST message (after the history) as a # user . last = wire[-1] @@ -505,7 +678,7 @@ def chat_stream_response(self, messages, tools=None, on_text_chunk=None, assert last["role"] == "user" assert "" in last_text assert "## Git Context" in last_text - assert "CLAUDE_MD_SENTINEL" in last_text + assert "CLAUDE_MD_SENTINEL" not in last_text # --------------------------------------------------------------------------- #