Skip to content

compress-core 0.4.0: OmniRoute context-compression engines, ported (1/3: core) - #532

Closed
manjughosa wants to merge 10 commits into
magpie-community:mainfrom
manjughosa:compress-core-1-core
Closed

manjughosa wants to merge 10 commits into
magpie-community:mainfrom
manjughosa:compress-core-1-core

Conversation

@manjughosa

@manjughosa manjughosa commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

What

A magpie gateway compression plugin: 15 strategies, four presets, byte-level telemetry. A full port of OmniRoute's open-sse/services/compression (MIT): strategy ids, names, priorities and lossy flags all match upstream; what can't be ported is marked and explained in the table.

Part 1 of 3 stacked PRs:

  1. this one — core: middleware + hooks + engines + tests (no UI)
  2. page & panels (the in-app Compression page, settings and usage panels)
  3. docs & provenance extras (DEBUGGING.md, English README)

How it works

  • middleware (compress-core.middleware.js): runs on moejs in the gateway. Fail-open — a failed compression never becomes a failed request.
  • hooks (observability.hooks.js): runs in the Bun host with filesystem: per-request stats (stats.json, requests.jsonl — no bodies), opt-in captures, and the CCR retrieval store.
  • ccr: replaces long repeated blocks with a content-addressed marker; the original comes back through a zero-dependency local MCP server (mcp/ccr-retrieve-server.mjs). Gated: it never compresses unless the agent has the retrieve tool.
  • prefix stability: lite rewrites every message from turn one with deterministic transforms, so prompt-cache prefixes never re-miss. test/prefix-stability.test.mjs asserts byte-exact prefixes across turns.

Tests

  • 27 suites, same entry points as upstream: bun test and node test/run.mjs — both pass locally on this exact tree (bun test: 27 pass; node test/run.mjs: 968 checks).
  • cases.json shares 21 byte-identical cases with history-trim (history-trim: a request's history sent smaller, the turn being sent left whole #423) plus 2 of this package's own defaults, so the two middlewares provably don't fork.
  • Provenance per upstream file (sha256) in PROVENANCE.json; Apache-2.0 notices in NOTICE + LICENSES/.

Notes for review

  • Capture defaults to off in this community build (full request bodies on disk is a heavy-debug feature; opt-in via options).
  • scripts/check.mjs skips this package (middleware + hooks have their own tests), as with the other middleware.

Review follow-ups

  • Capture is opt-in in code. Changed to options.capture === true — a fresh install (options: {}) writes nothing. package.json's magpie.options is only the options editor's example (magpie's OptionsExample), not a runtime default, so the gate itself had to change. OBS-14/OBS-15 cover both sides.
  • CCR does nothing until the retrieve tool is installed. callerSupportsCcrRetrieve() checks the request's tools[] for omniroute_ccr_retrieve; without it the engine returns early with gateReason: "no_retrieve_tool". That is why ccr shows 0 hits on a fresh install.
  • page/ arrives in PR 2 (the in-app Compression page). This PR creates the folder only when publishing a capture index; no page assets ship here.
  • CCR store is documented in the README: ccr-store.json in the plugin data dir, 24h per entry, 512KB/block, 64MB global with LRU eviction; clear it by deleting the file.

…ddleware + hooks + engines + tests)

15 strategies, four presets, byte-level telemetry. Middleware runs on
moejs in the gateway; hooks run in the Bun host with stats, captures
and the CCR retrieval store. cases.json aligns with history-trim (magpie-community#423)
byte for byte on the 21 shared cases. Capture defaults to off.
Replaces the three-strategy entry sketched in magpie-community#449 with the shipped
package: 15 strategies, four presets, per-strategy telemetry, strict
prefix stability and the companion hooks host.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the port. The engines are carefully done, and the provenance and NOTICE work is appreciated. I read the middleware, the hooks, the stores, options-commands.js, page-publish.js and the MCP server, and skimmed the engines. The suite is green (numbers at the end), but one thing has to change before this can merge.

1. Capture is on by default, and the PR says it is off. The body says: "Capture defaults to off in this community build". package.json does have "capture": false under magpie.options. magpie, however, does not apply magpie.options as defaults. It is only the example the options editor starts from (OptionsExample in magpie's internal/plugin/middleware.go). Hooks get the user's own saved options, which are {} on a fresh install. observability.hooks.js then checks:

if (options.capture !== false) {   // "only an explicit false keeps it off"

So right after install, every local request's whole conversation is written to disk twice:

  • ~/.config/magpie/compress-core/captures/<id>.json;
  • page/captures/<id>.js, inside the installed package folder.

Up to 50 are kept. Secrets are masked, but everything else is kept: code, private notes, file contents. I checked it with a probe that calls onRequest the way the host does (origin: "local", options: {}):

data/captures: [ "capture-3e92327e-….json" ]   page/captures: [ "capture-3e92327e-….js" ]
{"…","beforeText":"[1] [user] my private note: the launch date is 2026-11-03", …}

Please make capture opt-in in the code (options.capture === true) and add a test with options: {}. Check the other !== false defaults the same way. telemetry.enabled and observability are fine on by default, since they keep no text. Anything that keeps conversation text must not be on by default.

2. The CCR store is a second copy of conversation text. When ccr is enabled, the blocks it replaces are written to the CCR store file, so the retrieve server can return them. That is by design, but the README should say where the file is, how long entries live and how to clear it. Also, ccr is in the default strategy set: please confirm it does nothing until the retrieve tool is installed. Your body says it is gated, and the code reads that way to me.

3. The page-triggered magpie library mcp add (options-commands.js, processCcrCommand) runs the magpie binary and assigns the retrieve server to every agent that already receives library MCP servers. Since it only runs after a click on the page, that's acceptable. It belongs in PR 2 with the page, though, and the page should name the agents it will be added to before it runs.

Smaller things:

  • test/ci.test.mjs leaves out 4 of the 27 suites that test/run.mjs runs: capture-command, ccr-persist, active-shapes and config-contract. CI runs only bun test, so those four never run there. Please add them.
  • This is a new package at version 0.4.0. That's fine, but it isn't on npm yet. After merge, the maintainer has to publish the first version, and magpie installs it from the dist branch until then.
  • PR 1 publishes into a page/ folder that this PR doesn't have. It works (the folder gets created), but please say in the body that page/ arrives in PR 2.

What I ran (Mac mini, Bun 1.4.2 and Node 22, through mote-run, sandbox HOME), at a187803:

  • HOME=$(mktemp -d) bun test from the repo root: 3831 pass, 0 fail.
  • bun scripts/check.mjs: exit 0.
  • node test/run.mjs in the package: all 27 suites pass.
  • The probe in item 1 is red at this head.
  • Grepped for network use: none. The only fetch is in test/live-probe.mjs, which bun test doesn't run, and there is no WebSocket, eval or new Function. The URLs in the code are upstream attributions only.

Blocker per @yetone review on magpie-community#532:

1. Capture opt-in: Changed observability.hooks.js gate from options.capture !== false to options.capture === true, so fresh installs have capture OFF by default. Fresh hook calls receive options:{}, and undefined !== false was ON — every local request's full conversation was leaking to disk twice (data/captures + page/captures). Verified via empirical probe: private text leaked before fix. Added OBS-14 (asserts no capture with options:{}) and OBS-15 (explicit capture:true still works).

2. CCR store docs: README privacy section now documents ccr-store.json location, 24h expiry per entry, global 64MB limit, LRU eviction, and how to clear (delete file —原文 temporarily unretrievable after clearing). Also updated clearance instructions to mention ccr-store removal.

3. Test suite parity: Created test/suites.mjs as single source of truth. ci.test.mjs now imports this list instead of duplicating it. This fixes the "four suites never ran under bun" drift that already let a real failure hide: three suites had a self-run guard (`if (process.argv[1] === fileURLToPath...)`) that fires under `bun test`, causing double invocation and cross-suite state pollution in capture-command (which uses ?host=1 module caching). Fixed by guarding `typeof Bun === "undefined"`. Added missing suites (capture-command, ccr-persist, active-shapes, config-contract) to CI.

4. Guarded self-runs: Three suites (capture-command, capture-store, lab) use `node test/x.test.mjs` debugging convenience. Under bun, process.argv[1] equals the file being tested, so an unguarded self-run executes at load time, then ci.test.mjs runs it again—polluting module-level state. Guard against this with typeof check.

Also: confirmed ccr gating works (requires retrieve tool), page/ folder arrives in PR2, version stays 0.4.0.

968 checks pass on node run.mjs, 27 pass on bun test across 28 files. CI environment verified.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick follow-up, @manjughosa. I re-reviewed at 553c41c and ran it on a Mac (Bun 1.4.2).

What was run at 553c41c

  • From the repo root, as CI does: HOME=$(mktemp -d) bun test gave 3835 pass, 0 fail (3843 tests, 330 files). bun scripts/check.mjs exited 0. node test/run.mjs in packages/compress-core printed "all tests passed".
  • I wrote a probe that drives observability.hooks.js onRequest and then compress-core.middleware.js, the same order magpie uses. It used a sandbox HOME, COMPRESS_CORE_DATA_DIR and COMPRESS_CORE_PAGE_DIR, and ran the real stdio mcp/ccr-retrieve-server.mjs against the store the hook wrote.
  • A timing probe passed long runs of spaces, tabs and other repeated characters to every engine (see item 6).
  • A second probe sent two-turn conversations for chat, anthropic, responses and gemini through the hook with options: {}, with and without the retrieve tool. The first turn's messages came out byte-identical inside the second turn in every case, so prefix stability holds for the default engines in these cases.

Fixed since the last review

  • Capture is opt-in now (options.capture === true). With options: {} the captures folder stays empty, and the new OBS-14/OBS-15 cover both cases. Thank you.
  • The README now documents the CCR store: ccr-store.json, 24h TTL, 64MB cap, and clearing it by deleting the file.
  • ci.test.mjs and run.mjs share test/suites.mjs, and the 4 missing suites run in CI.
  • The MCP retrieve server speaks stdio only and opens no listener, so a web page can't reach it.
  • The middleware is fail-open: a throw returns undefined, and each engine is snapshotted and restored on failure.
  • Attribution is fine: MIT LICENSE with the OmniRoute copyright line, plus NOTICE, LICENSES/Apache-2.0.txt and PROVENANCE.json.

Still blocking

  1. CCR blocks stored by the hook can't be retrieved. engines/ccr/index.js stores each block under principalId: cfg.principalId || ctx.session. magpie fills ctx.session from the request headers, so it is usually non-empty. But the MCP tool's schema has no principal, so retrieveBlock(store, hash, undefined) looks under __anon__. In the probe, a block stored with session: "sess-123" was in ccr-store.json, and the real MCP server answered CCR block not found for hash=f35d…. When that happens, the model has the 200-char preamble and nothing else, and the rest of the block is lost. Either store and retrieve without a session principal, or have the MCP server use the same key.

  2. The CCR instruction is sent to Anthropic as a system message. injectCcrProtocolInstruction does body.messages.unshift({role: "system", …}) for any body that has messages, Anthropic's included. After the hook, the probe's Anthropic body had roles ["system","user"], and the Messages API answers that with a 400. For Anthropic the instruction belongs in the top-level system field (as a string or a block), for Responses in instructions, and for Gemini in systemInstruction.

  3. __magpieCompressCore reaches the vendor on one path. The hook returns the body with that top-level key in it, and only the middleware strips it. Codex's own ChatGPT sign-in passthrough runs the gateway hooks but not the middleware (internal/gateway/codex_backend.go in magpie), so on that path the key goes to the ChatGPT backend. It also stays in the body whenever the middleware is off or fails. Please don't leave a marker key in the body the hook returns. For example, the hook could record that it already ran in ctx.state, or the middleware could recognise compressed content by its own markers.

  4. On the middleware-only path, CCR is lossy but still marked lossless. When the hook doesn't run, the middleware calls runPipeline with no ccrStore. That covers an origin other than local (a magpie key or gateway key), observability: false, and a failed hook. CCR still writes [CCR retrieve hash=…] markers there, but nothing stores the originals, so no retrieve can return them. I saw this in the probe with origin magpie-key. Please skip the ccr engine when no persistent store is available.

  5. The page-triggered magpie library mcp add (processCcrCommand in options-commands.js) is still here. Last review asked to move it to PR 2, together with the page that names the agents it changes. It also reads MAGPIE_CONFIG_DIR || ~/.config/magpie/library.json and ignores XDG_CONFIG_HOME, which is where magpie itself looks on Linux.

  6. The default lite engine takes quadratic time on long runs of spaces, and in the hook that blocks the whole plugin host. engines/lite.js:113 runs .replace(/[ \t]+$/gm, ""). When a long run of spaces or tabs isn't followed by a line end, the regex starts again at each position of the run, so the time grows with the square of its length. With options: {} on Bun, onRequest took 56ms for 10k spaces followed by a letter, 214ms for 20k, 872ms for 40k and 1894ms for 60k. In Node, lite alone took 9.97s for 60k spaces, and more than 12s for 60k chars of "\t \t". The middleware is cut off at 250ms, but the hook runs synchronously inside magpie's Bun plugin host, and the host's AbortSignal timeout can't fire while that thread is busy. So one large tool output stalls every plugin, including the subscription plugins that serve requests. Trimming line by line (line.trimEnd() on a split) is linear. With the same inputs, the off-by-default engines also ran past 12s: rtk and ultra on "\n \n" × 20000, and caveman on "a." × 30000. Please check their regexes the same way, and add a test that each engine finishes within a time bound on such inputs.

Smaller items (not blocking on their own)

  • The PR body still says "23 suites" and doesn't mention that page/ comes in PR 2.
  • README:
    • It links docs/ccr-retrieval-chain.md and options-override.json, and neither exists.
    • It says the 稳妥 preset includes session-dedup, but session-dedup is off by default.
    • It says captures-index.js goes to the plugin data dir, but page-publish.js and capture-store.js write it, and compress-stats.js, into the package's own page/ folder.
  • stats-writer.js:
    • A record() that lands while a flush is awaiting is lost, because summary = blank() runs after the awaits.
    • An unreadable stats.json resets the totals to zero. It should keep the last good totals.
  • lite: a message whose content is only whitespace collapses to "", which Anthropic rejects. Please leave such a message as it was.
  • ci.test.mjs doesn't call unlistedSuites(), so a new suite that isn't in the list is only caught by run.mjs.
  • mcp/ccr-retrieve-server.mjs persistCounts does a read-modify-write on ccr-store.json while the host may be writing it. One of the two writes can win and drop the other's changes.

manjughosa added a commit to manjughosa/magpie-compress-core that referenced this pull request Oct 30, 2026
Addresses @yetone's review on magpie-community/plugins#532:

1. Capture is now OPT-IN. observability.hooks.js used `options.capture !== false`,
   but magpie never applies package.json's magpie.options as runtime defaults — it
   is only the options editor's starting example (OptionsExample). A fresh install
   gets options:{}, where undefined !== false is true, so every local request's full
   conversation was written to disk twice (data/captures + page/captures) before the
   operator ever saw a switch. Changed to `options.capture === true`. Added OBS-14
   (asserts nothing is written with options:{}) and OBS-15 (explicit true still works).

2. README documents the CCR store: location (ccr-store.json in the plugin data dir),
   lifetime (24h per entry, 512KB per block, 64MB global with LRU eviction), and how
   to clear it (delete the file).

3. test/suites.mjs is now the single list shared by run.mjs and ci.test.mjs. The two
   copies had drifted: capture-command, ccr-persist, active-shapes and config-contract
   ran under node but never under `bun test`. Adding them exposed a real latent failure —
   three suites self-run at import time when argv[1] matches, which is true under bun
   test, so they executed twice and collided on cached module state. Guarded those
   self-runs with `typeof Bun === "undefined"`.

4. bunfig.toml scopes `bun test` to test/: backup/ snapshots are old copies of the
   plugin whose relative imports no longer resolve, and loading one aborted the run.

968 checks pass under node; 29 pass under bare `bun test`.
@yetone

yetone commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

One more set of notes on the opt-in engines, at the same head (553c41c). None of these engines is on by default, so they matter once someone turns one on:

  • session-dedup is marked lossless, but the model can't recover what it removes. catalog.js:22 has lossy: false, and the description says nothing is dropped. But engines/session-dedup.js:196 replaces a repeated block with [dedup:ref sha=…]. The sha→text map goes only to ctx.state, never to the vendor, and the earlier copy carries no sha. So the model gets text like "again:\n[dedup:ref sha=7d7b51e0fa798c90f39e298f]" and can't tell which earlier text it means. Please mark it lossy, or have the marker point to something the model can find.
  • aggressive and read-lifecycle rewrite earlier turns. I sent a 4-turn body and the same body with a fifth turn added. With aggressive, Anthropic message 5 and chat message 6 came out different between the two (its tiers are counted from the end, engines/aggressive.js:25-28). With read-lifecycle, an old tool_result becomes a stub once the same file is read again, so Anthropic message 11 changed. That is consistent with their cacheImpact labels, but the panel text should say plainly that each new turn changes the cached prefix.
  • What else was checked: codex-responses, ionizer and llm changed nothing on my test bodies, and read-lifecycle folded nothing on chat bodies. In chat it may not recognise OpenAI tool_calls as Reads, which would make it a silent no-op there. llm.js has no backend, so turning it on does nothing.

…ead-lifecycle

Review follow-ups:

- session-dedup stays `lossy: false`: only the LATER occurrence of a block is
  replaced; the first occurrence stays in the conversation, so nothing is dropped.
  Flipping to true would also drop it from the safe preset (isSafeDefault === !lossy).
  The real gap: the marker was only explained in ccr's system instruction, injected
  only when ccr runs. The marker now explains itself:
  `[dedup:ref sha=<hex> — original appears earlier in this conversation]`.
  Regexes updated in session-dedup.js and pipeline.js; tests updated.
- aggressive / read-lifecycle: UI text now says each new turn rewrites older turns
  and moves the prompt-cache prefix.
- read-lifecycle DOES handle OpenAI tool_calls (extractInvocations reads msg.tool_calls
  plus responses function_call) — not a silent no-op.
- llm.js without a backend is an intentional no-op (upstream default backend).

968 checks pass under node; 27 under bun.
@manjughosa

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I've fixed the capture default to opt-in, added the missing CI tests, updated the README for CCR store, and updated the PR description. Ready for another look.

Three of the four points I checked against the code; two turned out not to be defects, so I'm putting the evidence here rather than changing them:

1. session-dedup is not lossy — I kept lossy: false. dedupMessageTexts keeps the first occurrence ("owner = global first occurrence, the rest go into the replacement table") and replaces only later duplicates, so the original text is still in the conversation and the model can look back. Marking it lossy would also silently drop it from the safe preset, since isSafeDefault(engine) is defined as engine.lossy === false. There was a real gap though: the marker was only explained inside ccr's system instruction, which is injected only when ccr runs. The marker now explains itself and works with ccr off: [dedup:ref sha=<hex> — original appears earlier in this conversation].

2. aggressive / read-lifecycle cache note — agreed, added. Their panel text now says each new turn rewrites older turns and moves the prompt-cache prefix.

3. read-lifecycle does handle OpenAI tool_calls. extractInvocations reads Anthropic tool_use blocks and OpenAI msg.tool_calls, plus function_call for the responses protocol, so it is not a silent no-op in plain chat. (llm.js without a backend is an intentional no-op — its header says so, and upstream's default backend is the same identity function.)

bun test: 27 pass; node test/run.mjs: 968 checks pass.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @manjughosa. I re-reviewed at 1b83822 and read the diff since 553c41c line by line. It touches five files: catalog.js, engines/session-dedup.js, pipeline.js, test/live-probe.mjs and test/session-dedup.test.mjs.

What was run at 1b83822

On a Mac mini with Bun 1.4.2 and Node 22, in a sandbox HOME:

  • From the repo root, as CI does: HOME=$(mktemp -d) bun test gave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files). bun scripts/check.mjs exited 0.
  • node test/run.mjs in packages/compress-core printed "all tests passed".
  • A probe drove observability.hooks.js onRequest with origin: "local" and options: {}, and called runPipeline and applySessionDedup directly.

On the 19:48 comment

  • session-dedup on its own: I accept your argument. Within one request the first occurrence is the group's owner and stays verbatim, and only later copies become markers. The probe shows that: "here:\n\n<block>" is kept and "again:\n[dedup:ref sha=… — original appears earlier in this conversation]" follows. With the self-describing marker the model can find the text, so lossy: false is fair for the engine alone.
  • But it breaks when read-lifecycle is on too. read-lifecycle runs after session-dedup (stackPriority 22 vs 3) and replaces the first occurrence. I sent two Reads of the same file with session-dedup and read-lifecycle both on. The two tool_results came out as:
    [read superseded — "/p/math.js" was re-read or modified later in the conversation; the current content appears below]
    [dedup:ref sha=dfd19345fdfd31c580bacdf5 — original appears earlier in this conversation]
    
    Each one points at the other, and the file's content is not in the body at all. The same can happen with any later engine that rewrites the first occurrence (aggressive, ccr, ultra and others). Either run session-dedup after the engines that rewrite older content, or have those engines leave a block alone when a dedup marker points to it. Please add a test with both engines on.
  • The cache notes on aggressive and read-lifecycle: thanks, they're added.
  • read-lifecycle and OpenAI tool_calls: you're right, extractInvocations reads msg.tool_calls. My earlier note was wrong.

Still blocking: none of the six items from the 19:47 review is addressed

Your reply covered the first review and the 19:48 comment, but not the six blocking items in the 553c41c review. The files involved (engines/ccr/index.js, engines/lite.js, observability.hooks.js, compress-core.middleware.js, options-commands.js, mcp/ccr-retrieve-server.mjs) are byte-identical to 553c41c. I re-checked them at this head:

  1. The retrieve tool can't return CCR blocks. The engine stores each block under ctx.session, and the MCP server looks it up under __anon__. The code is unchanged.
  2. The CCR instruction goes to Anthropic as a system message. engines/ccr/index.js:536 still calls body.messages.unshift({ role: "system", … }). It belongs in system, instructions or systemInstruction, depending on the protocol.
  3. __magpieCompressCore is still in the body the hook returns. The probe's Anthropic body after onRequest has "__magpieCompressCore" in body === true. On Codex's ChatGPT passthrough, which runs hooks but not the middleware, that key reaches the vendor.
  4. CCR is lossy on the middleware-only path. CCR still runs there with no persistent store.
  5. The page-triggered magpie library mcp add is still here. processCcrCommand in options-commands.js belongs in PR 2. It also still ignores XDG_CONFIG_HOME.
  6. lite, a default engine, takes quadratic time. engines/lite.js still runs .replace(/[ \t]+$/gm, ""). Through the hook with options: {}, 20k spaces took 175ms and 40k took 662ms, roughly 4× for 2× the input. That blocks the whole Bun plugin host. Please also check the rtk, ultra and caveman regexes from that review, and add a time-bound test.

The smaller items from that review are also still open:

  • The README links files that don't exist, and its preset and captures-index.js paths are wrong.
  • stats-writer.js loses a record() that lands during a flush, and resets the totals when stats.json can't be read.
  • lite collapses a whitespace-only message to "".
  • ci.test.mjs doesn't call unlistedSuites().
  • The MCP server's persistCounts does a read-modify-write that can race the host's writes.

Please address items 1 to 6 and the session-dedup plus read-lifecycle loss, and I'll review again.

@manjughosa

Copy link
Copy Markdown
Contributor Author

Heads-up: your notes are against 553c41c, which is one push behind. The latest head is 1b83822 — please review against that one.

Two of the points are already fixed there:

  • aggressive / read-lifecycle panel text now says plainly that each new turn rewrites older turns and moves the prompt-cache prefix.
  • session-dedup marker now explains itself: [dedup:ref sha=<hex> — original appears earlier in this conversation]. It no longer depends on ccr's system instruction being injected, so it reads correctly with ccr off.

We kept lossy: false on purpose: only later duplicates are replaced, the first occurrence stays in the conversation, and isSafeDefault is defined as lossy === false — marking it lossy would silently drop session-dedup from the safe preset.

The remaining edge cases we're leaving as-is: the sha is an internal handle and the block is identifiable by content; read-lifecycle in chat does parse msg.tool_calls (and the responses protocol's function_call), so it folds whenever the fixture carries standard tool calls.

So: everything you asked for is in 1b83822. Please re-review that head and close this out if it looks right.

@yetone

yetone commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

The latest review (20:28Z) is against 1b83822, your current head, not 553c41c. It covers both of the changes you list:

  • The new self-describing dedup marker and the panel text are acknowledged there. lossy: false is accepted for session-dedup on its own.
  • What is still open: with read-lifecycle also on, read-lifecycle runs after session-dedup and turns the first copy into a stub. A file read twice then ends up as two markers pointing at each other, and its content is no longer in the request. Please fix the order, or have those engines leave a referenced block alone, and add a test with both on.
  • The six blocking items from the 19:47Z review are unchanged at 1b83822: the CCR store and retrieve key mismatch, the CCR instruction sent to Anthropic as a system message, __magpieCompressCore left in the request body, CCR running with no store on the middleware-only path, the page-triggered magpie library mcp add, and lite's quadratic time (20k spaces take 175ms, 40k take 662ms).

Please address those, and I'll look again at the next head.

…rvives

With both engines on, session-dedup (priority 3) ran first: it replaced a later
duplicate Read result with `[dedup:ref sha=…]` pointing at the first copy. Then
read-lifecycle (priority 22) collapsed that first copy into a stub, leaving two
markers pointing at each other and the file content gone from the request.

read-lifecycle now runs at priority 2, before session-dedup: it stubs the superseded
read first, then dedup sees a single full copy and creates no ref to content that is
about to become a stub.

Added a regression test with both engines enabled; verified it fails on the old
order (stubs=1, content gone) and passes on the new one.

970 checks pass under node; 27 under bun.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @manjughosa. I re-reviewed at 75a4aa7 and read the diff since 1b83822 line by line. Ignoring the CRLF to LF change in engines/read-lifecycle.js, it is 36 lines in three files: read-lifecycle's priority goes from 22 to 2 in catalog.js and in the engine, its header note is updated, and test/read-lifecycle.test.mjs gets a test with both engines on.

What was run at 75a4aa7

On the Mac mini (Bun 1.4.2, Node 22), in a sandbox HOME:

  • From the repo root, as CI does: HOME=$(mktemp -d) bun test gave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files). bun scripts/check.mjs exited 0.
  • node test/run.mjs in packages/compress-core printed "all tests passed".
  • A probe ran runPipeline on chat and Anthropic bodies with read-lifecycle and session-dedup on, with and without the default engines, and then session-dedup together with each other engine in turn.
  • A second probe called the real observability.hooks.js onRequest and compress-core.middleware.js to check the six blockers again.
  • I broke the fix by hand: with read-lifecycle back at 22, your new test fails (2 failures) and my probe loses the file in all three cases. At 2 both pass.

Fixed: read-lifecycle together with session-dedup

  • A file read twice with both engines on now keeps one full copy. In chat, in Anthropic, and in Anthropic with the defaults (ccr, lite, headroom) on too, the result is stubs=1 dedupRefs=0, with the file's text whole in the newest Read.
  • I also tried Read, then cat of the same file through Bash, then Read again. read-lifecycle stubs the first Read, dedup keeps the Bash copy and turns the last Read into a ref, and the text is still in the request.
  • In my 20:28Z review I also asked about other engines that rewrite the first copy. I paired session-dedup with each of the 14 other engines, with a long tool output in the first turn and again in the last. None lost text that the same engine on its own kept. aggressive and ultra drop it with or without dedup, which is what their lossy labels say.
  • Prefix: in a four-turn Anthropic conversation, dedup alone left every earlier message the same. With read-lifecycle on, turn 4 changed message 2 (the stub) and message 10 (a Bash copy that had been a ref and is now the full copy, because its owner became a stub). The cache prefix already breaks at message 2, so message 10 costs nothing more, and the panel already says read-lifecycle moves the prefix.

Two small things in this diff:

  • engines/read-lifecycle.js still says stackPriority = 2 // 上游 5;catalog 锁定 22,见文件头第 5 条. The comment should say 2.
  • The cacheImpact line now ends in a trailing space.

Still blocking: the six items from the 19:47Z review

engines/ccr/, engines/lite.js, observability.hooks.js, compress-core.middleware.js, options-commands.js, mcp/ and stats-writer.js are byte-identical to 1b83822. I checked each item again at this head:

  1. Open: the retrieve tool can't return CCR blocks. engines/ccr/index.js:696 stores under cfg.principalId || ctx.session, and mcp/ccr-retrieve-server.mjs:82 looks up args.principalId, which the tool's schema doesn't have, so it reads __anon__. Failing input: any request with session: "sess-123" whose block CCR stores. The real MCP server answers CCR block not found for its hash.
  2. Open: the CCR instruction goes to Anthropic as a system message. engines/ccr/index.js:536 still does body.messages.unshift({ role: "system", … }). Failing input: an Anthropic body with the omniroute_ccr_retrieve tool and a 120-line user message, through onRequest with origin: "local". Roles come out ["system","user","assistant","user"] and the top-level system is unset, so the Messages API answers 400.
  3. Open: __magpieCompressCore stays in the body the hook returns. Same input: "__magpieCompressCore" in body is true after onRequest. On Codex's ChatGPT passthrough, which runs hooks but not the middleware, that key reaches the vendor.
  4. Open: CCR runs with no store on the middleware-only path. The same body through compress-core.middleware.js with origin: "magpie-key" comes out with two [CCR retrieve …] markers, and nothing stores the originals.
  5. Open: the page-triggered magpie library mcp add. processCcrCommand is still in options-commands.js and still called from onRequest. It belongs in PR 2 with the page that names the agents. It also still ignores XDG_CONFIG_HOME.
  6. Open: lite takes quadratic time. engines/lite.js:113 still runs .replace(/[ \t]+$/gm, ""). Through the hook with options: {}, a chat message of N spaces followed by "a" took 55ms for 10k spaces, 204ms for 20k and 806ms for 40k: four times the time for twice the input. The rtk, ultra and caveman regexes and a time-bound test are still to do.

The smaller items from that review are open too: the README's missing links and wrong paths, the two stats-writer.js issues, ci.test.mjs not calling unlistedSuites(), and the race in persistCounts. A whitespace-only message (" \n\t ") now comes out of lite as "\n". Anthropic rejects a text block that is only whitespace, so please leave such a message as it came.

The read-lifecycle fix is good. Please address items 1 to 6, and I'll review the next head.

…eware CCR, lite O(n^2)

Six blockers from review:

1. CCR store/retrieve key mismatch: the engine stored blocks under principalId =
   ctx.session (a sha256 session id), while the retrieve MCP server cannot see the
   session and queried the __anon__ partition, so every lookup missed. The engine now
   uses only an explicit cfg.principalId (or anon), matching what the tool queries.
2. CCR instruction was injected as a {role:"system"} message into body.messages.
   Anthropic rejects system roles there (400). It now appends to the top-level
   body.system for the anthropic protocol and only uses a system message for chat.
3. __magpieCompressCore leaked into the request body. INTERNAL_MARKER is now a Symbol
   (Symbol.for), which JSON.stringify ignores, so the hand-off flag can never reach
   the vendor.
4. The middleware-only path ran ccr without a shared store: markers were created for
   blocks never persisted, so retrieval always failed. That path now forces ccr off
   and leaves it to the Bun hook that owns the store.
5. processCcrCommand and its tests are removed from this PR; they move to PR 2 with
   the page. The hook no longer calls it.
6. lite's normalizer used /[ \t]+$/gm, which backtracks per line and is quadratic on
   long space runs (40k spaces ~800ms). It now trims line by line (split/map/join),
   which is linear.

Small: whitespace-only messages collapse to "" instead of "\n\n"; stale stackPriority
comment corrected.

958 checks pass under node; 27 under bun.
@manjughosa

Copy link
Copy Markdown
Contributor Author

Both sets are addressed in cd3b03f (and the engine-order fix in 75a4aa7).

read-lifecycle / session-dedup conflict: read-lifecycle now runs at priority 2, before session-dedup (3). It stubs the superseded read first, so dedup only ever sees one full copy and never creates a ref to content that is about to become a stub. Added a regression test with both engines on; it fails on the old order (stubs=1, content gone) and passes on the new one.

The six blockers:

  1. CCR store/retrieve key mismatch — the engine keyed entries by ctx.session (a sha256:… id) while the MCP server, which cannot see the session, queried __anon__. It now uses only an explicit cfg.principalId, so store and retrieve hit the same partition.
  2. CCR instruction sent to Anthropic as a system message — it was messages.unshift({role:"system"}), which Anthropic rejects with 400. For protocol === "anthropic" it now appends to the top-level body.system (string or block array); chat still uses a system message.
  3. __magpieCompressCore in the request body — INTERNAL_MARKER is now Symbol.for(...). JSON.stringify ignores symbol keys, so the hand-off flag cannot reach the vendor.
  4. CCR running with no store on the middleware-only path — that path now forces ccr: false; only the Bun hook, which owns the shared store, runs ccr.
  5. processCcrCommand — removed from this PR together with its tests; it moves to PR 2 with the page, and the hook no longer calls it.
  6. lite quadratic time — /[ \t]+$/gm backtracks per line; replaced with split("\n").map(l => l.replace(/[ \t]+$/, "")).join("\n"), which is linear. 40k spaces no longer cost ~800ms.

Small items: whitespace-only messages now collapse to "" rather than "\n\n", and the stale stackPriority comment is corrected.

node test/run.mjs: 958 checks; bun test: 27 pass.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @manjughosa. I re-reviewed at cd3b03f, read every line of the diff from 75a4aa7 (8 files, +38 −220), and ran each blocker's input again through the real hook, host.js's JSON hand-off, the middleware and the real MCP server. Four of the six are fixed. Two are not: item 4 when options are left at their defaults, and item 6, whose new code is still quadratic. The fixes for items 1 to 3 also have no tests yet.

What was run at cd3b03f

On the Mac mini (Bun 1.4.2, Node 22), in a sandbox HOME:

  • From the repo root: HOME=$(mktemp -d) bun test gave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files). bun scripts/check.mjs exited 0. node test/run.mjs in packages/compress-core printed "all tests passed".
  • A probe called observability.hooks.js onRequest with origin: "local", session: "sess-123" and options: {}. Its input was an Anthropic body with the omniroute_ccr_retrieve tool and a 120-line user message. The probe passed the result through JSON.parse(JSON.stringify(…)) as host.js does, waited out the store's 2s flush, and then asked the real stdio mcp/ccr-retrieve-server.mjs for the marker's hash. It also ran compress-core.middleware.js and timed lite.
  • The read-lifecycle plus session-dedup probe from the last review gave the same results as at 75a4aa7: the file is whole in every case.
  • I broke each fix by hand in a copy: the session principal put back for item 1, the protocol argument dropped for item 2, and the string marker put back for item 3.

Per blocker

  1. Fixed: CCR store and retrieve now use the same key. The store now has "__anon__ c4b38571…", and the real MCP server returns the original block. With || ctxObj.session put back, the key is "sess-123 c4b3…" and the server answers CCR block not found. But no test fails: with that change, node test/run.mjs still prints "all tests passed". Please add a test that stores through apply with a non-empty ctx.session and retrieves the block the way the MCP server does.
  2. Fixed: the CCR instruction now goes in Anthropic's top-level system. Roles come out ["user","assistant","user"]. A missing system becomes a string, a string system gets the instruction appended, and a block array with cache_control gets one more block after it. The instruction appears once in each case. No test fails here either when opts.protocol is no longer passed to injectCcrProtocolInstruction. Please add an Anthropic case to the CCR suite.
  3. Fixed: the marker no longer reaches the vendor. JSON.stringify of the hook's result no longer contains __magpieCompressCore. Two notes:
    • host.js does next = JSON.parse(JSON.stringify(result)) (magpie internal/plugin/host.js, in the gateway hook loop), so the Symbol never reaches the middleware. The middleware's if (body[INTERNAL_MARKER]) early return can no longer happen, and the middleware now runs the whole pipeline a second time on every body the hook already compressed. In my test the second pass changed nothing, so I am not blocking on it. But the hand-off is dead code now, and every request pays for the pipeline twice. Please remove the check, or recognise the hook's output another way.
    • No test fails when the string key is put back. Please add a test that the hook's result, serialized, has no internal key.
  4. Still open: CCR still runs on the middleware-only path with default options. The new guard only replaces ccr when options.strategies.ccr is truthy. ccr is on by default (DEFAULT_STRATEGIES.ccr === true), and a fresh install has options: {}, so safe = options and ccr runs.
    • Failing input: the Anthropic body above through compress-core.middleware.js with origin: "magpie-key" and options: {} comes out with 2 [CCR retrieve hash=…] markers, and nothing stores the originals. With options: { strategies: { ccr: true } } it has 0.
    • Always set ccr to false on this path, for example { ...options, strategies: { ...(options.strategies || {}), ccr: false } }, and add a test with options: {}.
  5. Fixed: processCcrCommand is gone, and nothing references ccrCommand or install-mcp any more. options-commands.js still imports execFile, homedir, dirname and fileURLToPath, which only that function used. Please drop those imports.
  6. Still open: lite is still quadratic. The per-line .replace(/[ \t]+$/, "") without m still retries from every position of a run of spaces that a non-space ends. Splitting into lines doesn't help when the whole run is on one line.
    • Failing input: one chat message of N spaces followed by "a", through the hook with options: {}. On Bun it took 44ms for 10k, 175ms for 20k, 699ms for 40k and 2797ms for 80k, about 4× for each 2×. At 75a4aa7 it took 55, 204 and 806ms for 10k, 20k and 40k.
    • lite alone on N spaces + "a\n" + N/2 × "\t " + "b": 1.5s at 40k and 6.2s at 80k on Bun, and 4.9s at 80k on Node.
    • A linear version scans back from the end of each line: let e = line.length; while (e > 0 && (line.charCodeAt(e - 1) === 32 || line.charCodeAt(e - 1) === 9)) e--; return line.slice(0, e).
    • The rtk, ultra and caveman regexes from the 19:47Z review haven't been looked at. A test that each engine finishes within a time bound on these inputs is still missing too. That test would have caught this.

Smaller items

  • Fixed: the stale "catalog 锁定 22" comment and the trailing space on cacheImpact.
  • Not fixed: a whitespace-only message. " \n\t ", "\n\n\n" and " " now all come out of lite as "". The ask was to leave such a message as it came. Anthropic rejects an empty text block just as it rejects a whitespace-only one, so "" is no better than "\n".
  • Still open, files unchanged:
    • the README's missing links (docs/ccr-retrieval-chain.md, options-override.json), the 稳妥 preset text and the captures-index.js path;
    • stats-writer.js: a record() lost during a flush, and the totals reset when stats.json can't be read;
    • ci.test.mjs not calling unlistedSuites();
    • the read-modify-write race in persistCounts.

The CCR key, the Anthropic system field and the marker leak are fixed. To finish:

  • fix item 4 with default options;
  • make lite linear and add the time-bound test;
  • add tests for items 1 to 3 that fail without the fixes.

Then I'll review the next head.

…linear, stats, locks)

Items from the review at cd3b03f:

1. Test that the CCR key matches: apply() with ctx.session "sess-123" stores the
   block, and retrieveBlock() without a principal (the way the MCP server asks)
   returns it. Fails when the session principal is put back.
2. Test that the CCR instruction goes to Anthropic's top-level system field and
   that messages[] gains no system role.
3. The internal hand-off marker is gone entirely (pipeline, hooks, middleware,
   lab); the middleware check is removed and OBS-04 asserts the second pass is a
   no-op. OBS-07 asserts the hook's serialized result has no __magpieCompressCore.
4. The middleware path always forces ccr:false, including when options.strategies
   is absent (fresh install passes options:{}). Test drives options:{}.
5. options-commands.js drops execFile/homedir/dirname/fileURLToPath.
6. lite trims trailing whitespace linearly (charCodeAt scan); a time-bound suite
   now walks every engine over two whitespace-heavy inputs.

Smaller items: whitespace-only messages left as they came (lite, ultra); README
capture default corrected, stale options-override mention removed, safe-preset
wording fixed; stats-writer no longer loses records mid-flush or resets totals on
an unreadable stats.json; ci.test.mjs calls unlistedSuites(); persistCounts takes
a lock around its read-modify-write.

991 checks under node; 29 under bun.
@manjughosa

Copy link
Copy Markdown
Contributor Author

Addressed at a2f4ab5. Each item, with the test that now guards it:

  1. Key match — apply() with ctx.session: "sess-123" stores the block; retrieveBlock() with no principal (what the MCP server passes) returns it. Putting the session principal back makes that test fail.
  2. Anthropic — the instruction lands in the top-level system string, and messages[] gains no system role. Test added to the CCR suite.
  3. Marker — removed entirely from pipeline.js, observability.hooks.js, compress-core.middleware.js and lab.js, so the dead check is gone. You're right that the middleware now runs the pipeline twice; OBS-04 asserts the second pass is byte-identical. OBS-07 asserts the hook's serialized result contains no __magpieCompressCore.
  4. Middleware CCR — it is now { ...options, strategies: { ...(options.strategies || {}), ccr: false } }, unconditional. The test drives options: {} and asserts no marker; reverting the guard makes it fail.
  5. Imports — execFile, homedir, dirname, fileURLToPath are gone from options-commands.js.
  6. lite linear — trailing whitespace is trimmed by scanning back from each line end (charCodeAt), no regex. A new test/performance.test.mjs times every engine over two whitespace-heavy inputs. It immediately caught two more of the same class, both fixed: caveman (~1s on 40k spaces + 20k tabs) and ultra (turned a whitespace-only message into "").

Smaller items: whitespace-only messages are left as they came (lite and ultra); README capture default corrected to off, the options-override.json mention removed, and the safe-preset text now says "the original is still there" rather than "byte-identical"; stats-writer.js no longer loses records during a flush and no longer resets totals when stats.json cannot be read; ci.test.mjs calls unlistedSuites() and fails on an unregistered suite; persistCounts takes a lock around its read-modify-write.

Two of your notes I could not map to a line: "the 終末 preset text" (no such string in the package; I did fix the safe-preset wording) and "the captures-index.js path" (publishPageScript writes it beside page/). If you meant a specific line, point me at it and I'll fix that too.

node test/run.mjs: 991 checks; bun test: 29 pass, at a2f4ab5.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at a2f4ab5 (delta from cd3b03f, every line read).

What was run (on a macOS test box, HOME a fresh temp dir):

  • bun test from the repo root: 3837 pass, 8 skip, 0 fail.
  • bun scripts/check.mjs: exit 0.
  • node test/run.mjs in packages/compress-core: all pass.
  • The blocker probes through the real observability.hooks.js onRequest (with host.js's JSON round trip) and the middleware, plus the read-lifecycle + session-dedup probe: no file content lost, cache prefix as before.
  • Each fix reverted by hand, one at a time, then node test/run.mjs again.

Blockers

  1. CCR key / principal: fixed. A block stored under session: "sess-123" comes back from the MCP retrieve server with no principal. With || ctxObj.session put back, item1 goes red. Good.

  2. Anthropic system: fixed, but the test doesn't guard the fix. The hook writes the instruction to top-level system for all three shapes (none, string, cache_control array). But item2 calls injectCcrProtocolInstruction(body, "anthropic") directly. With line 689 of engines/ccr/index.js changed back to injectCcrProtocolInstruction(body), every test still passes. Meanwhile the real hook puts a role: "system" message back into an Anthropic messages array, and out.system is undefined. Please add a test that runs apply (or the hook) with protocol: "anthropic" and checks the outgoing body.

  3. Marker leak: fixed. INTERNAL_MARKER is gone, and OBS-07 checks that no __magpieCompressCore key leaves.

  4. CCR with options: {}: fixed. The guard is unconditional now, and the middleware with options: {} gives 0 markers. Making the guard conditional again turns item4 red. Good.

  5. processCcrCommand: fixed (removed at cd3b03f).

  6. Quadratic regexes: lite is fixed, ultra and caveman are not. lite takes 0–1ms at 80k, a whitespace-only message is returned unchanged, and restoring the /[ \t]+$/ replace turns both item6 checks red. But the 19:47Z review asked for rtk, ultra and caveman to be checked on these inputs too, and two of them are still quadratic. Here is Node 22 on a single user message followed by "ok" / "go". Doubling n takes about 4x as long:

    engine input n=5k 10k 20k 40k
    ultra "\n \n".repeat(n) 47ms 159ms 631ms 2523ms
    ultra "a.".repeat(n) 34ms 135ms 291ms (30k: 2.1s)
    caveman "a.".repeat(n) 37ms 139ms 294ms (30k: 2.1s)

    CPU profile of where the time goes:

    • engines/ultra.js:333 /^\|?[\s:]*-{3,}[\s:]*(\|[\s:]*-{3,}[\s:]*)*\|?$/gm takes 4.9s at 40k. Under m, \s crosses newlines, so every line start scans the rest of the text. [ \t:] keeps the meaning on one line and takes 0ms. The typst pattern next to it, /^\s*#(?:set|show|let|import|include)\b.+$/gm (ultra.js:334, caveman.js:365, llm.js:217), has the same problem: 4.1s. Use ^[ \t]*#.
    • /\b[a-zA-Z_$][\w$]*(?:\.[a-zA-Z_$][\w$]*)+\(\)?/g (ultra.js:342, caveman.js:373, llm.js:225) takes 1.9s at 30k. \(\)? is a required ( followed by an optional ). So on a.a.a.… every start position runs to the end and fails, and the regex never matches foo.bar without a paren ("x a.b".match(re) is null). If the intent is an optional (), write (?:\(\))?. That also makes the scan linear. Either way, cap the repeat.

    test/performance.test.mjs uses only whitespace-plus-"a" inputs, so these all pass it. Its 800ms bound would also have passed the old lite (about 700ms at 40k). Please add "\n \n" × 20000 and "a." × 30000 for every engine, and use a ratio check like item6 rather than a loose absolute bound.

Smaller items (not blocking on their own)

  • mcp/ccr-retrieve-server.mjs's persistCounts now takes a lock, but the host's writeStoreFile (ccr-store-file.js) doesn't, so the two writers can still overwrite each other's counts. Also, if the lock can't be taken, the write goes ahead without it.
  • stats-writer: if publishPageScript throws after the rename has written the file, summary = merge(flushed, summary) puts the increment back, and the next flush counts it twice. capturesCleared isn't restored either.
  • README:
    • The docs link points to a doc that is not in this PR ("随文档 PR 提供"). Link it once the doc exists, or drop it for now.
    • The 稳妥 preset still lists session-dedup, which is off by default.
    • On captures-index.js: page-publish.js and capture-store.js write it, together with compress-stats.js, into the package's own page/ folder, not the plugin data dir that the README names.
  • pipeline.js still has no newline at end of file.

The panel showed OmniRoute's upstream pitches verbatim ("~15%", "~30%",
"~50%", "~75%", "60-90%"), and on real agent traffic they are misleading:
lite saves 0.1-1% of a request because most content is unique, not 15%.

Chinese copy (catalog + engine `why`) now states the measured range and what
it applies to: lite 0.1-1% of a request, rtk 60-95% of tool output (share
depends on traffic), caveman 10-20%, aggressive 25-35%, ultra 30-35% at the
default keep ratio; session-dedup/ccr reworded (original stays in history /
retrievable, ccr gated on the retrieve tool).

The English `description` fields keep the verbatim upstream text for
provenance. caveman test asserts the measured wording instead of "30%".

991 checks under node; 29 under bun.
…small items

Per the review at a2f4ab5:

- ultra/caveman/llm: markdown-table separator and typst-directive patterns used \s
  under /m (crosses newlines) and scanned the rest of the text from every line
  start; now [ \t] / [^\n]. The dotted-identifier pattern required a "(" (`\(\)?`),
  so "a.a.a…" never matched and every start ran to the end; now `(?:\(\))?` with the
  repeat capped at 8, which also makes plain "foo.bar" match again. caveman's
  PROTECTED_STRUCTURE_RE had the same pattern and got the same fix.
- performance test covers "\n \n" × 20k and "a." × 30k for every engine with a
  half/full ratio check (full ≤ half×3 + 50ms), not a loose absolute bound.
- ccr item 2: end-to-end test through apply() with protocol "anthropic".
- lock-file.js is shared by the MCP server and ccr-store-file.js; persistCounts
  skips the write when the lock cannot be taken.
- stats-writer: publish failure after the rename no longer double-counts; captures
  cleared flag restored on failure.
- README: safe preset no longer lists session-dedup as on; page/ folder named as the
  publish target; docs link dropped where the doc is absent.
- pipeline.js ends with a newline.

994 checks under node; 29 under bun.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at d2c714a. I read every line of a2f4ab5..d2c714a. Ignoring line endings, the llm.js diff is 7 lines; the rest of its 875 lines are CRLF/CR converted to LF, which is fine.

What was run (on a macOS test box, HOME a fresh temp dir):

  • bun test from the repo root: 3837 pass, 8 skip, 0 fail.
  • bun scripts/check.mjs: exit 0.
  • node test/run.mjs in packages/compress-core: all pass.
  • The blocker probes through the real hook (with host.js's JSON round trip), the middleware and the MCP retrieve server, plus the read-lifecycle + session-dedup probe. All of them behave as at a2f4ab5.
  • Every non-CCR engine on 12 adversarial shapes at 10k/20k/40k repeats, on Node 22 and Bun, with a CPU profile of each slow case.
  • Each fix reverted by hand.

Fixed

  • Anthropic system through apply: fixed and tested. I changed line 689 of engines/ccr/index.js back to injectCcrProtocolInstruction(body) and item2e went red twice ("指令进顶层 system", "messages 里没有 system role"). Good.
  • The three named regexes: fixed. These are the ultra table regex, the Typst ^\s*#… (ultra, caveman, llm) and the dotted-id \(\)? → {1,8}(?:\(\))?. On Node, ultra now takes 13ms on "\n \n"×40000 (was 2.5s) and on "a."×40000 (was 2.1s at 30k). caveman takes 151ms on "a."×40000.
  • README 稳妥 preset, stats-writer double count, capturesCleared restore, trailing newline: fixed.

Still open

1. Quadratic paths remain in caveman and ultra, and the regex fixes have no test. I put the a2f4ab5 versions of ultra.js, caveman.js and llm.js back and node test/run.mjs still passed. Here is Node 22 on one user message followed by "ok" / "go". The input sizes are the repeat counts below; times are for 10k / 20k / 40k repeats:

engine input time where (CPU profile)
caveman "\n".repeat(4n) + "word ".repeat(n/5) 2.0s / 8.4s / 29.8s /^\s*|.*|\s*$/gm (caveman.js:363 and :463, llm.js:216). Under m, \s* crosses newlines. Use ^[ \t]*|[^\n]*|[ \t]*$.
caveman " ".repeat(4n) + "word ".repeat(n/5) 1.1s / 3.6s / 13.9s The new table regex ^[ \t]*|?[ \t]*:?-{3,}…. [ \t]*|?[ \t]* is ambiguous when the | is absent, so a long space run is split every possible way. Use ^[ \t]*(?:|[ \t]*)?:?-{3,}….
caveman "a.b(".repeat(n) 0.5s / 2.0s / 8.9s restorePreservedBlocks (caveman.js:392) does a split/join of the whole text once per block, which is O(blocks × length).
ultra "a.b(".repeat(n) 0.4s / 4.8s / 14.6s pruneProseOnly (ultra.js:391) builds one alternation of every placeholder (20k+ alternatives) and splits on it.
caveman "[x](".repeat(n) 0.4s / 1.8s / 8.0s /\[[^\]\n]+\]\([^) \n]+(?:\s+"[^"]*")?\)/g (caveman.js:367 and :446, llm.js:220). From every [, [^) \n]+ scans to the end.
ultra "[x](".repeat(n) 0.3s / 1.0s / 4.1s /\[[^\]\n]*\]\([^)\s]+\)/g (ultra.js:336), the same problem.
ultra / caveman "1.1.".repeat(n) up to 4.5s / 2.6s The same two restore paths.

On Bun the worst case is caveman with the space run, at 15s for 40k.

For the restore paths, replace each placeholder in one pass: a single regex for the sentinel's own shape plus a Map lookup. Don't build one alternation per block or a split per block. For the link regexes, cap the runs the way PROTECTED_STRUCTURE_RE already does ({1,1000}, {1,2000}).

Please extend test/performance.test.mjs so each of these inputs runs on every engine, with a 20k→40k ratio check like item6's. As things stand, reverting any of the regex fixes passes the suite.

2. The new store lock loses data. I ran createPersistentCcrStore directly (probe below):

  • Missing data dir: the host never writes the store. mkdir now runs inside the lock, but acquireLock opens <file>.lock before the directory exists. open fails with ENOENT, the loop retries until its 2s deadline, and writeStoreFile returns without writing. Result: file written=false after 2005ms. Do the mkdir before acquireLock.
  • Lock busy for more than 2s: the block is lost. writeStoreFile returns quietly, flush has already cleared dirty, and nothing retries. A block whose text was swapped for a marker never reaches disk, so the retrieve server can't return it. Result: block on disk=false. Set dirty back (and reschedule) when the write is skipped.
  • The count race the lock exists for is still open. writeStoreFile reads and merges the old file's retrievalCounts before it takes the lock. The MCP server can take the lock, write count 3 and release it, and then the host writes the counts it read earlier. Result: count after host write: [] after the MCP side wrote [[key, 3]]. Move the read-merge inside the lock.

Probe (run from packages/compress-core with Bun):

const { createPersistentCcrStore } = await import("./ccr-store-file.js")
// fresh dir: join(mkdtempSync(...), "data", "ccr-store.json"); set an entry; persist(); await flushNow(); existsSync(f) -> false
// busy lock: writeFileSync(f + ".lock", ""); set entry; persist(); await flushNow(); unlinkSync(f + ".lock"); wait 2.5s -> block not on disk
// stale read: hold the lock, start flushNow(), write retrievalCounts [[k,3]] to f, release, await -> counts []

Not blocking

  • The README docs line now says "见后续文档 PR". Fine for now.

@manjughosa

Copy link
Copy Markdown
Contributor Author

I've decided to close this PR. I built this compression plugin as a custom adaptation for your platform and offered it as a courtesy. Since we can't align on the requirements and the level of nitpicking here isn't worth my time, I'll just keep it for my own personal use. You are welcome to write your own compression engines from scratch. Good luck.

@manjughosa manjughosa closed this Oct 31, 2026
@yetone

yetone commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Understood, and thanks for the time you put into this — the core work (the prefix-stable folding, the Anthropic system fix, the linear regexes in this last round) was real progress. The remaining asks were about a 30s stall on adversarial input and a store lock that loses blocks, which would land on users who never see the code, so we couldn't merge without them. If you ever want to pick it up again, the probes are described in the last review. Good luck with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants