Repository navigation
compress-core 0.4.0: OmniRoute context-compression engines, ported (1/3: core) - #532
manjughosa wants to merge 10 commits into
Conversation
…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
left a comment
There was a problem hiding this comment.
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.mjsleaves out 4 of the 27 suites thattest/run.mjsruns:capture-command,ccr-persist,active-shapesandconfig-contract. CI runs onlybun 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 thedistbranch 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 thatpage/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 testfrom the repo root: 3831 pass, 0 fail.bun scripts/check.mjs: exit 0.node test/run.mjsin the package: all 27 suites pass.- The probe in item 1 is red at this head.
- Grepped for network use: none. The only
fetchis intest/live-probe.mjs, whichbun testdoesn't run, and there is no WebSocket,evalornew 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
left a comment
There was a problem hiding this comment.
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 testgave 3835 pass, 0 fail (3843 tests, 330 files).bun scripts/check.mjsexited 0.node test/run.mjsinpackages/compress-coreprinted "all tests passed". - I wrote a probe that drives
observability.hooks.jsonRequestand thencompress-core.middleware.js, the same order magpie uses. It used a sandbox HOME,COMPRESS_CORE_DATA_DIRandCOMPRESS_CORE_PAGE_DIR, and ran the real stdiomcp/ccr-retrieve-server.mjsagainst 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). Withoptions: {}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.mjsandrun.mjssharetest/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
-
CCR blocks stored by the hook can't be retrieved.
engines/ccr/index.jsstores each block underprincipalId: cfg.principalId || ctx.session. magpie fillsctx.sessionfrom the request headers, so it is usually non-empty. But the MCP tool's schema has no principal, soretrieveBlock(store, hash, undefined)looks under__anon__. In the probe, a block stored withsession: "sess-123"was in ccr-store.json, and the real MCP server answeredCCR 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. -
The CCR instruction is sent to Anthropic as a
systemmessage.injectCcrProtocolInstructiondoesbody.messages.unshift({role: "system", …})for any body that hasmessages, 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-levelsystemfield (as a string or a block), for Responses ininstructions, and for Gemini insystemInstruction. -
__magpieCompressCorereaches 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.goin 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 inctx.state, or the middleware could recognise compressed content by its own markers. -
On the middleware-only path, CCR is lossy but still marked lossless. When the hook doesn't run, the middleware calls
runPipelinewith noccrStore. That covers an origin other thanlocal(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 originmagpie-key. Please skip the ccr engine when no persistent store is available. -
The page-triggered
magpie library mcp add(processCcrCommandinoptions-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 readsMAGPIE_CONFIG_DIR || ~/.config/magpie/library.jsonand ignoresXDG_CONFIG_HOME, which is where magpie itself looks on Linux. -
The default
liteengine takes quadratic time on long runs of spaces, and in the hook that blocks the whole plugin host.engines/lite.js:113runs.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. Withoptions: {}on Bun,onRequesttook 56ms for 10k spaces followed by a letter, 214ms for 20k, 872ms for 40k and 1894ms for 60k. In Node,litealone 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'sAbortSignaltimeout 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:rtkandultraon"\n \n"× 20000, andcavemanon"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.mdandoptions-override.json, and neither exists. - It says the 稳妥 preset includes session-dedup, but session-dedup is off by default.
- It says
captures-index.jsgoes to the plugin data dir, butpage-publish.jsandcapture-store.jswrite it, andcompress-stats.js, into the package's ownpage/folder.
- It links
stats-writer.js:- A
record()that lands while a flush is awaiting is lost, becausesummary = blank()runs after the awaits. - An unreadable stats.json resets the totals to zero. It should keep the last good totals.
- A
lite: a message whose content is only whitespace collapses to"", which Anthropic rejects. Please leave such a message as it was.ci.test.mjsdoesn't callunlistedSuites(), so a new suite that isn't in the list is only caught byrun.mjs.mcp/ccr-retrieve-server.mjspersistCountsdoes 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.
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`.
|
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:
|
…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.
|
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. 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
|
yetone
left a comment
There was a problem hiding this comment.
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 testgave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files).bun scripts/check.mjsexited 0. node test/run.mjsinpackages/compress-coreprinted "all tests passed".- A probe drove
observability.hooks.jsonRequestwithorigin: "local"andoptions: {}, and calledrunPipelineandapplySessionDedupdirectly.
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, solossy: falseis 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-dedupandread-lifecycleboth on. The two tool_results came out as: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.[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] - The cache notes on aggressive and read-lifecycle: thanks, they're added.
- read-lifecycle and OpenAI
tool_calls: you're right,extractInvocationsreadsmsg.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:
- 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. - The CCR instruction goes to Anthropic as a
systemmessage.engines/ccr/index.js:536still callsbody.messages.unshift({ role: "system", … }). It belongs insystem,instructionsorsystemInstruction, depending on the protocol. __magpieCompressCoreis still in the body the hook returns. The probe's Anthropic body afteronRequesthas"__magpieCompressCore" in body === true. On Codex's ChatGPT passthrough, which runs hooks but not the middleware, that key reaches the vendor.- CCR is lossy on the middleware-only path. CCR still runs there with no persistent store.
- The page-triggered
magpie library mcp addis still here.processCcrCommandinoptions-commands.jsbelongs in PR 2. It also still ignoresXDG_CONFIG_HOME. lite, a default engine, takes quadratic time.engines/lite.jsstill runs.replace(/[ \t]+$/gm, ""). Through the hook withoptions: {}, 20k spaces took 175ms and 40k took 662ms, roughly 4× for 2× the input. That blocks the whole Bun plugin host. Please also check thertk,ultraandcavemanregexes 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.jspaths are wrong. stats-writer.jsloses arecord()that lands during a flush, and resets the totals when stats.json can't be read.litecollapses a whitespace-only message to"".ci.test.mjsdoesn't callunlistedSuites().- The MCP server's
persistCountsdoes 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.
|
Heads-up: your notes are against Two of the points are already fixed there:
We kept The remaining edge cases we're leaving as-is: the sha is an internal handle and the block is identifiable by content; So: everything you asked for is in |
|
The latest review (20:28Z) is against
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
left a comment
There was a problem hiding this comment.
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 testgave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files).bun scripts/check.mjsexited 0. node test/run.mjsinpackages/compress-coreprinted "all tests passed".- A probe ran
runPipelineon 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.jsonRequestandcompress-core.middleware.jsto 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
catof 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.jsstill saysstackPriority = 2 // 上游 5;catalog 锁定 22,见文件头第 5 条. The comment should say 2.- The
cacheImpactline 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:
- Open: the retrieve tool can't return CCR blocks.
engines/ccr/index.js:696stores undercfg.principalId || ctx.session, andmcp/ccr-retrieve-server.mjs:82looks upargs.principalId, which the tool's schema doesn't have, so it reads__anon__. Failing input: any request withsession: "sess-123"whose block CCR stores. The real MCP server answersCCR block not foundfor its hash. - Open: the CCR instruction goes to Anthropic as a
systemmessage.engines/ccr/index.js:536still doesbody.messages.unshift({ role: "system", … }). Failing input: an Anthropic body with theomniroute_ccr_retrievetool and a 120-line user message, throughonRequestwithorigin: "local". Roles come out["system","user","assistant","user"]and the top-levelsystemis unset, so the Messages API answers 400. - Open:
__magpieCompressCorestays in the body the hook returns. Same input:"__magpieCompressCore" in bodyistrueafteronRequest. On Codex's ChatGPT passthrough, which runs hooks but not the middleware, that key reaches the vendor. - Open: CCR runs with no store on the middleware-only path. The same body through
compress-core.middleware.jswithorigin: "magpie-key"comes out with two[CCR retrieve …]markers, and nothing stores the originals. - Open: the page-triggered
magpie library mcp add.processCcrCommandis still inoptions-commands.jsand still called fromonRequest. It belongs in PR 2 with the page that names the agents. It also still ignoresXDG_CONFIG_HOME. - Open:
litetakes quadratic time.engines/lite.js:113still runs.replace(/[ \t]+$/gm, ""). Through the hook withoptions: {}, 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. Thertk,ultraandcavemanregexes 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.
|
Both sets are addressed in read-lifecycle / session-dedup conflict: The six blockers:
Small items: whitespace-only messages now collapse to
|
yetone
left a comment
There was a problem hiding this comment.
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 testgave 3835 pass, 8 skip, 0 fail (3843 tests, 330 files).bun scripts/check.mjsexited 0.node test/run.mjsinpackages/compress-coreprinted "all tests passed". - A probe called
observability.hooks.jsonRequestwithorigin: "local",session: "sess-123"andoptions: {}. Its input was an Anthropic body with theomniroute_ccr_retrievetool and a 120-line user message. The probe passed the result throughJSON.parse(JSON.stringify(…))as host.js does, waited out the store's 2s flush, and then asked the real stdiomcp/ccr-retrieve-server.mjsfor the marker's hash. It also rancompress-core.middleware.jsand timedlite. - 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
protocolargument dropped for item 2, and the string marker put back for item 3.
Per blocker
- 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.sessionput back, the key is"sess-123 c4b3…"and the server answersCCR block not found. But no test fails: with that change,node test/run.mjsstill prints "all tests passed". Please add a test that stores throughapplywith a non-emptyctx.sessionand retrieves the block the way the MCP server does. - Fixed: the CCR instruction now goes in Anthropic's top-level
system. Roles come out["user","assistant","user"]. A missingsystembecomes a string, a stringsystemgets the instruction appended, and a block array withcache_controlgets one more block after it. The instruction appears once in each case. No test fails here either whenopts.protocolis no longer passed toinjectCcrProtocolInstruction. Please add an Anthropic case to the CCR suite. - Fixed: the marker no longer reaches the vendor.
JSON.stringifyof the hook's result no longer contains__magpieCompressCore. Two notes:- host.js does
next = JSON.parse(JSON.stringify(result))(magpieinternal/plugin/host.js, in the gateway hook loop), so the Symbol never reaches the middleware. The middleware'sif (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.
- host.js does
- Still open: CCR still runs on the middleware-only path with default options. The new guard only replaces
ccrwhenoptions.strategies.ccris truthy. ccr is on by default (DEFAULT_STRATEGIES.ccr === true), and a fresh install hasoptions: {}, sosafe = optionsand ccr runs.- Failing input: the Anthropic body above through
compress-core.middleware.jswithorigin: "magpie-key"andoptions: {}comes out with 2[CCR retrieve hash=…]markers, and nothing stores the originals. Withoptions: { 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 withoptions: {}.
- Failing input: the Anthropic body above through
- Fixed:
processCcrCommandis gone, and nothing referencesccrCommandorinstall-mcpany more.options-commands.jsstill importsexecFile,homedir,dirnameandfileURLToPath, which only that function used. Please drop those imports. - Still open:
liteis still quadratic. The per-line.replace(/[ \t]+$/, "")withoutmstill 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 withoptions: {}. 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. litealone 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,ultraandcavemanregexes 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.
- Failing input: one chat message of N spaces followed by
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 ofliteas"". 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 thecaptures-index.jspath; stats-writer.js: arecord()lost during a flush, and the totals reset when stats.json can't be read;ci.test.mjsnot callingunlistedSuites();- the read-modify-write race in
persistCounts.
- the README's missing links (
The CCR key, the Anthropic system field and the marker leak are fixed. To finish:
- fix item 4 with default options;
- make
litelinear 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.
|
Addressed at
Smaller items: whitespace-only messages are left as they came (lite and ultra); README capture default corrected to off, the 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" (
|
yetone
left a comment
There was a problem hiding this comment.
Re-review at a2f4ab5 (delta from cd3b03f, every line read).
What was run (on a macOS test box, HOME a fresh temp dir):
bun testfrom the repo root: 3837 pass, 8 skip, 0 fail.bun scripts/check.mjs: exit 0.node test/run.mjsin packages/compress-core: all pass.- The blocker probes through the real
observability.hooks.jsonRequest (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.mjsagain.
Blockers
-
CCR key / principal: fixed. A block stored under
session: "sess-123"comes back from the MCP retrieve server with no principal. With|| ctxObj.sessionput back,item1goes red. Good. -
Anthropic system: fixed, but the test doesn't guard the fix. The hook writes the instruction to top-level
systemfor all three shapes (none, string, cache_control array). Butitem2callsinjectCcrProtocolInstruction(body, "anthropic")directly. With line 689 of engines/ccr/index.js changed back toinjectCcrProtocolInstruction(body), every test still passes. Meanwhile the real hook puts arole: "system"message back into an Anthropicmessagesarray, andout.systemis undefined. Please add a test that runsapply(or the hook) withprotocol: "anthropic"and checks the outgoing body. -
Marker leak: fixed. INTERNAL_MARKER is gone, and OBS-07 checks that no
__magpieCompressCorekey leaves. -
CCR with
options: {}: fixed. The guard is unconditional now, and the middleware withoptions: {}gives 0 markers. Making the guard conditional again turnsitem4red. Good. -
processCcrCommand: fixed (removed at cd3b03f).
-
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:]*)*\|?$/gmtakes 4.9s at 40k. Underm,\scrosses 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 ona.a.a.…every start position runs to the end and fails, and the regex never matchesfoo.barwithout 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.mjsuses 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" × 20000and"a." × 30000for 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'spersistCountsnow takes a lock, but the host'swriteStoreFile(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
publishPageScriptthrows after the rename has written the file,summary = merge(flushed, summary)puts the increment back, and the next flush counts it twice.capturesClearedisn'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.jsandcapture-store.jswrite it, together withcompress-stats.js, into the package's ownpage/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.
This reverts commit 4fc4df7.
…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
left a comment
There was a problem hiding this comment.
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 testfrom the repo root: 3837 pass, 8 skip, 0 fail.bun scripts/check.mjs: exit 0.node test/run.mjsin 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 toinjectCcrProtocolInstruction(body)anditem2ewent 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,
capturesClearedrestore, 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.
mkdirnow runs inside the lock, butacquireLockopens<file>.lockbefore the directory exists.openfails with ENOENT, the loop retries until its 2s deadline, andwriteStoreFilereturns without writing. Result:file written=false after 2005ms. Do themkdirbeforeacquireLock. - Lock busy for more than 2s: the block is lost.
writeStoreFilereturns quietly,flushhas already cleareddirty, 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. Setdirtyback (and reschedule) when the write is skipped. - The count race the lock exists for is still open.
writeStoreFilereads and merges the old file'sretrievalCountsbefore 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.
|
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. |
|
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. |
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:
How it works
compress-core.middleware.js): runs on moejs in the gateway. Fail-open — a failed compression never becomes a failed request.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.mcp/ccr-retrieve-server.mjs). Gated: it never compresses unless the agent has the retrieve tool.test/prefix-stability.test.mjsasserts byte-exact prefixes across turns.Tests
bun testandnode test/run.mjs— both pass locally on this exact tree (bun test: 27 pass;node test/run.mjs: 968 checks).cases.jsonshares 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.json; Apache-2.0 notices inNOTICE+LICENSES/.Notes for review
scripts/check.mjsskips this package (middleware + hooks have their own tests), as with the other middleware.Review follow-ups
options.capture === true— a fresh install (options: {}) writes nothing.package.json'smagpie.optionsis only the options editor's example (magpie'sOptionsExample), not a runtime default, so the gate itself had to change.OBS-14/OBS-15cover both sides.callerSupportsCcrRetrieve()checks the request'stools[]foromniroute_ccr_retrieve; without it the engine returns early withgateReason: "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.jsonin the plugin data dir, 24h per entry, 512KB/block, 64MB global with LRU eviction; clear it by deleting the file.