[xizhuomengcontin]

Orca-Code-Review — push 11

Severity	Count	Δ vs previous push
P0	0	0
P1	0	-1
P2	0	0
P3	0	0

✅ no blocking findings

A browser agent puts a screenshot in every request it makes. Neither half of orca handled that, and
each half was individually defensible, which is why it survived: the redactor is right that a long
random-looking run is what a leaked key looks like, and the matcher is right to compare what it was
given. Together they made an agent with eyes impossible to record faithfully or replay.

Measured throughout on a real browser-use run against Wikipedia — gpt-5.6-luna, a real Chromium,
four model calls, six screenshots — recorded and then replayed. Every number below is from that run.

1. A data: URI is not a secret

The entropy sweep looks for long random-looking runs, which is what a base64 payload is by
construction. #scan already skips a body the proxy itself base64-encoded, for exactly this reason
— its comment records that the sweep "shredded a 14 KB Connect/protobuf response into 226
placeholders, destroying it outright while protecting nothing". An image the agent encoded
arrives at the same problem through the other door, and that door was open.

that one run	before	after
<secret:high_entropy:…> placeholders	47,314	6
records in redactions.json	47,314, every one a piece of a PNG	6
screenshot bytes	shredded	intact
trace size	3.39 MB	3.22 MB

Three costs, and nothing on the other side of the ledger:

• the recorded bytes stop being the bytes that were sent, so a request carrying an image can never
  match on replay — by construction, not by bad luck
• redactions.json is the one file that answers what was removed from this trace, and it fills
  with tens of thousands of entries that were never secrets
• a credential visible in a screenshot is pixels. Entropy could not see it before and cannot
  see it now; nothing was being protected

Excluded from the entropy sweep only, alongside the spans already there. The named rules still run
over the whole value, so an sk-… sitting inside a data URI is still caught by shape — pinned by a
test, because the whole risk of an exclusion is that it becomes a hiding place.

2. Compare an image on its presence, not on its pixels

Keeping the screenshot intact is necessary and not sufficient. Replay does not intercept the world
— that is the design — so replaying a browser agent drives the real browser, which paints the
page a second time. Two paintings of one page are never the same bytes.

With the redaction fix already in place, turn 1 still scored:

distance 545,134   —   with the pixels set aside: 171

99.97% of it was one PNG. No threshold can absorb that: the screenshot is 94% of the request
body, so a rung-2 budget generous enough to admit a repaint would admit a wholly different
conversation.

So the image payload joins the salted placeholder digest as something folded away below rung 1.
Rung 1 still compares it, so an exact match still means exact. And the divergence names the fold,
because the ladder's own rule is that replay never silently approximates:

minor: request 0 differs by 315 chars with an identical message count, 315 of them in the
       trailing message, not counting 1 image, whose pixels are not compared below rung 1

The fold is sound because the pixels are not what identify the request. browser-use sends its
element tree, the page URL and the agent's own memory in the same message, as text, and that text
stays fully compared. Same measurement, the turns where two runs genuinely took different paths:

turn 1     545,134  →     171      ← the repaint
turn 2                 14,191
turn 3                 19,409      ← genuinely divergent paths
turn 4                 57,519

Blinding the payload does not make everything match. It stops one unreproducible field from
drowning out everything that does.

Spec §4 gains the paragraph, written in the shape the redaction one already has.

3. A check that crosses the seam

None of the twelve integration checks sent an image, which is precisely why this survived: the bug
lived between two components that were each correct alone.

vision-repaint records a generated noise PNG, asserts the trace holds it byte for byte and that
no entropy placeholder appears, then replays the agent with a different PNG — the only thing a
browser can do — and asserts the match survives and says that it did. Remove either fix and it goes
red, for the right reason each time:

• without §1 → the recorded image is not the image that was sent
• without §2 → the replay halts

The PNG is generated rather than committed: it has to be incompressible noise to look like a secret
to the sweep, and a checked-in file of random bytes is a thing reviewers rightly distrust. CI
already installs openai, so it runs there under --require-all with no workflow change.

What this does and does not fix

On that recording: reused 0/4 → 1/4, and the halt moves from turn 1 to turn 2.

What stops turn 2 is not orca. I captured the requests a replay actually sends (an orca attach --replay serving the recording, with an orca record in front of it) and diffed them line by line
against the recording:

recorded:  [203]<nav aria-label=Site />   [18]<input search>   Current tab: 4E1C
replayed:  [165]<nav aria-label=Site />   [15]<input search>   Current tab: 2175

browser-use renumbers every element on each page load. The recorded answer says type into element
18, and browser-use itself reports what happens next:

Element index 18 not available - page may have changed. Try refreshing browser state.

orca served the recorded answer faithfully and the world had moved. Replaying an agent that acts on
a live world has that ceiling, and reporting it is the correct behaviour — orca does not patch your
agent, so the agent really drives the real browser.

Verification

• 15 new unit tests. Mutation-tested in three directions: fold removed → 4 red; fold widened to
  media_type → 1 red; fold widened to the accompanying text → 2 red, including the safety test
  that a different page must still halt the replay
• full vitest run matches the clean baseline line for line — zero regressions (the 24 Windows
  failures on this machine are ERR_UNSUPPORTED_ESM_URL_SCHEME, already fixed by 7f928ab in fix(test): eleven Windows failures, one of them a real bug #69)
• conformance 0 failures · fidelity --check 0 regressions, still exact 3 — rung 1 is not
  weakened · check-neutrality 0 failures
• integration matrix 12 checks, 0 failed
• the real Wikipedia run re-recorded and re-replayed against the built CLI, before and after

🤖 Generated with Claude Code

[xizhuomengcontin]

@orcacode-review

[orcacode-review]

🐳 OrcaCode Review

Found 4 issues in this PR: 🟠 4 P1.

Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.

---

packages/proxy/src/matching.ts (line 527): 🟠 P1 Deferred match does not advance cursor, allowing duplicate consumption of a recorded request

When a request matches a deferred (out‑of‑order) recorded entry, #matchDeferred returns the match without advancing the matcher’s cursor. The recorded request at that index remains unconsumed, so a later incoming request could match the same recorded entry again, causing the same recorded response to be replayed multiple times. This breaks the invariant that each recorded request is used at most once and can lead to incorrect answers being served.

---

test/integrations/run.mjs (line 48): 🟠 P1 Missing import of exec leads to ReferenceError crash in integration runner

In test/integrations/run.mjs, the orca helper calls await exec(process.execPath, [cli, ...argv], ...) but exec is never imported (only execFile and spawn are imported). This results in a ReferenceError: exec is not defined at runtime, aborting the integration test runner and preventing any replay checks from executing. The failure path is uncaught and crashes the entire test harness.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 506 calls · 14.2M tokens · 56% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Guard against non‑array messages/content to avoid TypeError crashes

If a request's messages field is missing or not an array (e.g., malformed or unexpected shape), foldImages and countImages cast it to Record<string, unknown>[] and then call .map/iteration, causing a TypeError and crashing the matcher. This unhandled exception aborts the replay process, leading to loss of trace handling and potential data loss.

[xizhuomengcontin]

This does not reproduce. foldImages and countImages never see a non-array, because they run after normalizeRequest, which builds both with .map():

comparableRequest -> foldImages(mapStrings(redactedForm(req, redactor), foldPlaceholders))
redactedForm      -> normalizeRequest(req)

normalizeRequest does req.messages.map(...) and m.content.map(normalizeContent) with no guard at all, so a non-array throws there first — never reaching this line. Driving the built module with each malformed shape:

messages is a string  -> normalizeRequest threw  [req.messages.map is not a function]
messages is an object -> normalizeRequest threw  [req.messages.map is not a function]
messages missing      -> normalizeRequest threw  [Cannot read properties of undefined (reading 'map')]
content is a string   -> normalizeRequest threw  [m.content.map is not a function]
well-formed           -> normalizeRequest ok / comparableRequest ok

So foldImages is only ever reached with arrays that normalizeRequest constructed, and its ?? [] is belt-and-braces rather than the only guard.

Two supporting points:

• CanonicalRequest.messages is typed CanonicalMessage[] (packages/plugin-api/src/canonical.ts:31) and is built by the dialect parsers, so a malformed body is handled upstream of here.
• The ((form.messages ?? []) as Record<string, unknown>[]).map(...) pattern already exists in this same file on main, at line 305. This PR follows it rather than introducing it.

normalizeRequest having no guard whatsoever is a real robustness gap, but it pre-dates this PR and is not what this comment points at.

[orcacode-review]

🟠 P1 Duplicate foldImages function definition breaks TypeScript compilation

The file packages/proxy/src/matching.ts now contains two definitions of the function foldImages. TypeScript does not allow duplicate function declarations in the same scope, causing a compilation error and breaking the build. This prevents the package from typechecking and running tests.

[xizhuomengcontin]

foldImages is defined once, so there is nothing for TypeScript to reject. grep -n foldImages packages/proxy/src/matching.ts:

171:function foldImages(form: Record<string, unknown>): Record<string, unknown> {
232:  return foldImages(mapStrings(redactedForm(req, redactor), foldPlaceholders));
457:  this.#comparable = normalized.map((n) => foldImages(mapStrings(n, foldPlaceholders)));

Line 171 is the declaration. 232 and 457 are call sites — one in comparableRequest, one in the RequestMatcher constructor.

Compilation is clean:

$ npx tsc -b --pretty false ; echo $?
0

with zero duplicate or redeclare diagnostics. CI's check (node 20) and check (node 22) both run npx tsc --build, and both passed on this head (464e4d4) — a second declaration of the same function in the same scope could not get past either of them.

[xizhuomengcontin]

All four findings, verified

Each of the four was checked against the code and run where it was runnable. None of them reproduces on this head (464e4d4). Findings 1 and 2 are answered in their own threads; 3 and 4 came through unattached, so they are answered here.

#	finding	verdict
1	matching.ts:176 — non-array messages/content crashes the matcher	does not reproduce
2	matching.ts:225 — duplicate foldImages breaks compilation	does not reproduce
3	matching.ts:527 — deferred match lets a recording be consumed twice	does not reproduce
4	run.mjs:48 — missing import of exec	does not reproduce

3. matching.ts:527 — deferred match does not advance the cursor

Consumption of a deferred entry is not tracked by the cursor — a deferred index sits behind it — but by removal from the queue, which #matchDeferred does before returning, at matching.ts:531:

this.#deferred.splice(slot, 1);

Driving the real RequestMatcher with a recording of AAA, BBB, CCC, asking for CCC first so the first two are deferred, then asking for AAA twice:

recorded: 0=AAA 1=BBB 2=CCC
request CCC
request AAA          matched=true  index=0  rung=1
request AAA (again)  matched=false          rung=4   <- not matched a second time

The second AAA falls to rung 4, which is the halt. The at-most-once invariant holds.

This code is also outside the diff — git diff origin/main...464e4d4 -- packages/proxy/src/matching.ts contains no change to any cursor, deferred or consumption line.

4. test/integrations/run.mjs:48 — missing import of exec

exec is defined at the top of the file, from execFile:

14:import { execFile, spawn } from 'node:child_process';
21:const exec = promisify(execFile);

Both lines pre-date this PR. node --check test/integrations/run.mjs passes, a single check runs clean (1 checked, 0 failed, 0 skipped), and CI's framework integrations job — which runs this exact file under --require-all — passed on this head. A ReferenceError at line 190 would have failed every check in the suite, not just one.

---

What the change itself measures

Differential against main at ac3a619: baseline 2036 passed / 24 failed, this PR 2051 passed / 24 failed, with the failure set identical line for line and stable across two runs. The 24 are this machine's Windows-environment failures — POSIX file modes, SIGTERM, symlinks — present on main too.

tsc, fmt:check, conformance (57 events, 0 failures, including the spec change), fidelity (0 regressions), plugin neutrality (0), and integrations 12 checked, 0 failed, including the new vision-repaint.

Both halves of the fix are mutation-checked, and they fail for the different reasons the description claims:

mutation	result
remove the data: URI exclusion	FAILED — the recorded image is not the image that was sent
make foldImages the identity	FAILED — replay exited -1 (the replay halts)
both restored	passes

The real risk in excluding data: URIs from the entropy sweep is that the exclusion becomes a hiding place, so I widened DATA_URI to match everything and checked whether the new tests would notice: still redacts a high-entropy token outside the image goes red, while still redacts a key inside the data URI itself stays green — because the named rules are unaffected by the entropy exclusion, which is exactly the separation the change claims.

---

Happy to be shown wrong on any of the four — each answer above names the file, line and command, so they should be cheap to check. If the reasoning behind one of them was about a different line than the one it landed on, that would be useful to know too.

[xizhuomengcontin]

@orcacode-review please take another look.

All four findings from the last pass are answered above with the file, line and command for each — two in their own threads, two in the comment before this one. In short: foldImages has one declaration at line 171 (232 and 457 are call sites), exec is defined at run.mjs:21 from the execFile import on line 14, #matchDeferred removes the matched index at matching.ts:531, and normalizeRequest throws on a non-array before foldImages is ever reached.

Two of those contradict a green CI on this same head — check (node 20) and check (node 22) run npx tsc --build, and framework integrations runs test/integrations/run.mjs — so if a duplicate declaration or a ReferenceError were really there, neither job could have passed.

If the re-review still reports them, could it say which line it resolved each symbol at? That would tell us whether the finding is about code I am misreading, or about a view of the file that stops at the diff hunk.

[orcacode-review]

🐳 OrcaCode Review

This pull request was not reviewed.

A maintainer of this repository can comment @orcacode review to run it again. If it keeps failing, the usual causes are the account running out of credit or the app losing access to this repository — both are visible in the OrcaRouter settings. If neither applies, contact OrcaRouter support.

No comments were posted for this commit.

Details

Code	ORCA-FAILED
Error	Error
Attempt	3 of 3

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.}

[xizhuomengcontin]

@orcacode-review

[orcacode-review]

🐳 OrcaCode Review

This pull request was not reviewed.

A maintainer of this repository can comment @orcacode review to run it again. If it keeps failing, the usual causes are the account running out of credit or the app losing access to this repository — both are visible in the OrcaRouter settings. If neither applies, contact OrcaRouter support.

No comments were posted for this commit.

Details

Code	ORCA-RELAY
Stage	review
Error	Error
HTTP	500
Attempt	3 of 3

the model provider could not be reached

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.}

[ZhenghuaBao]

@orcacode-review

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 486 calls · 41.5M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Scope the data-URI exemption to image media types — it currently spares any base64 payload from the entropy sweep

The exemption is justified (in this comment, in the spec, and in the tests) by a screenshot, but the pattern is data:<any type>/<any subtype>;base64,<payload> — the media type is not constrained, and it is added to spansOf, which is the set of regions #scanEntropy refuses to look at.

Base64 is exactly the representation in which the named rules cannot see a credential (sk-, ghp_, AKIA…, -----BEGIN… all lose their shape when encoded), so the entropy sweep was the only thing protecting a data: URI payload. Uploads in that form are ordinary API usage: OpenAI's input_file takes file_data as data:application/pdf;base64,… / data:text/plain;base64,…, and attaching a file an agent just read is the same shape.

Measured with a faithful copy of #scanEntropy (TOKEN, looksRandom, 4.0 bits/char — the module's own constants) over a 900-byte random payload base64'd into a data URI:

application/octet-stream   before: 21 <secret:high_entropy:…> placeholders   after: 0, payload intact
text/plain                 before: 21                                       after: 0, payload intact
image/png                  before: 21                                       after: 0 (this is the intended case)

So for any non-image data URI the trace now holds the payload verbatim, redactions.json records nothing, and the value is on disk — where before this change the sweep destroyed it. That contradicts the promise SECURITY.md makes in writing ("Everything that is written goes through the redactor: … and high-entropy tokens become <secret:kind:hash8>", "a near-perfect credential harvester if we get it wrong"), and it departs from the convention the two existing exemptions in this file set out: PROTOCOL_ID_VALUE is deliberately constrained to a value that "must already look like a protocol id … anything else under that key is left to the ordinary sweep", and PROTOCOL_SIGNATURE_VALUE likewise. This exemption has no such constraint.

Fix: restrict the pattern to images, e.g. /data:image\/[a-zA-Z0-9.+-]+;base64,[A-Za-z0-9+/=\\]+/g (and note image/svg+xml is text, so if the intent is raster bytes only, list the raster types rather than accepting every image/*). Anything else must fall through to #scanEntropy as it did before.

[xizhuomengcontin]

Confirmed, and fixed in edcbc24. This one reproduces.

Measured against main with a 900-byte random payload in a data URI, counting <secret:high_entropy:…> spans and checking whether the payload survives verbatim:

                          main        this branch before     after
image/png                 19 spans    0, intact              0, intact   (the intended case)
application/pdf           19 spans    0, written verbatim    18 spans
text/plain                19 spans    0, written verbatim    18 spans
application/octet-stream  19 spans    0, written verbatim    18 spans

And the reason it matters, measured rather than asserted — a named rule finds sk-… by its shape, and base64 destroys the shape:

plaintext sk-… caught by a named rule : yes
same key base64'd, still caught       : no
decodes back to the key               : yes

So for an encoded payload the sweep really was the only thing left, exactly as you said.

The fix lists raster types rather than image/*, because image/svg+xml is text: a key inside it is one the named rules would have caught had the agent not encoded it. That also matches the convention PROTOCOL_ID_VALUE and PROTOCOL_SIGNATURE_VALUE already set in this file — an exemption fires only on the shape it was justified for.

Six new cases pin it (application/pdf, text/plain, application/octet-stream, application/json, image/svg+xml still swept; image/png|jpeg|webp|gif still spared; plus one asserting the base64-encoded key is invisible to the named rules). Mutation-checked: restoring the unconstrained pattern — valid code, tsc clean — turns all six red.

End to end on the merged branch: full suite 2074 passed / 10 failed against a main baseline of 2049 / 10, failure set identical line for line and stable across two runs; conformance 60 events 0 failures; fidelity 0 regressions; plugin neutrality 0; integrations 14 checked, 0 failed including vision-repaint.

Thank you for this one — it is the case the change's own justification did not cover.

[xizhuomengcontin]

@orcacode-review please re-review.

The media-type finding was right and is fixed in edcbc24: the data: URI exemption is now scoped to raster image types (png|jpeg|jpg|gif|webp|avif|bmp), with image/svg+xml and every non-image type falling back to the entropy sweep as before. Six cases pin it, mutation-checked against the unconstrained pattern.

main has also been merged in, so this branch now carries the mastra and llama-index checks alongside vision-repaint.

CI is green on this head, and locally: full suite 2074 passed / 10 failed against a main baseline of 2049 / 10 with an identical failure set, conformance 60 events 0 failures, fidelity 0 regressions, integrations 14 checked 0 failed.

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 491 calls · 43.2M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Do not grant the entropy exemption on a declared media type — verify the bytes are raster

The exemption is a hole that fails open, and the hole is the size of whatever the caller writes before the payload, not the size of a raster image.

RASTER_DATA_URI matches on the literal text data:image/png;base64, (and six siblings) inside a value that came out of the agent's request body. Nothing checks that the bytes that follow are an image: that string is agent-controlled (a prompt-injected or merely sloppy agent can emit any type it likes in front of any bytes), and the span registered in spansOf (line 200) makes #scanEntropy skip every token inside it.

Concrete, measured with this repository's own regex + TOKEN + looksRandom + entropy logic (redactString(JSON.stringify({url: 'data:<type>;base64,' + <base64 credential>}))):

data:image/png;base64,…   → 0 placeholders  (written verbatim, no record)
data:image/jpeg;base64,…  → 0 placeholders
data:application/pdf;base64,… → 1 placeholder
data:text/plain;base64,…  → 1 placeholder

The identical payload is spared or swept purely by the four-byte type label in front of it. And the sweep is the only detector left for an encoded payload — this commit's own test (packages/core/test/data-uri-redaction.test.ts, "a base64-encoded key is invisible to the named rules, so the sweep has to see it") asserts exactly that. So for a credential base64'd behind data:image/png;base64, the bytes land in events.jsonl/blobs verbatim and redactions.json — the one file a reader consults to learn what was removed — records nothing. That is what SECURITY.md promises cannot happen ("everything that is written goes through the redactor … and high-entropy tokens become <secret:kind:hash8>"). Same class as the hole the parent commit d189e77 was written to close; the narrowing to raster types narrows it by label only. It also contradicts the convention this file states one paragraph earlier: "an exemption is a hole; it should be exactly the shape of what goes through it."

Fix: make the span conditional on the payload being the format the label claims. In spansOf, take the match, strip data:<type>;base64,, base64-decode the first bytes and require the signature for the declared type before pushing the span — PNG 89 50 4E 47 0D 0A 1A 0A, JPEG FF D8 FF, GIF GIF87a/GIF89a, BMP BM, WebP RIFF….WEBP, AVIF ftyp at offset 4. Anything that does not decode to the declared raster format keeps no span, so the entropy sweep covers it as it does for any other data URI. (Cost is one decode of a handful of bytes per match.)

[xizhuomengcontin]

Right again, and fixed in 5a32622.

Reproduced exactly as you described — same encoded credential, only the type string differing:

data:image/png;base64,…         0 placeholders, written verbatim
data:image/jpeg;base64,…        0 placeholders, written verbatim
data:image/webp;base64,…        0 placeholders, written verbatim
data:application/pdf;base64,…   1 placeholder, swept
data:text/plain;base64,…        1 placeholder, swept

Four bytes of type string decided whether it reached the trace intact. Narrowing to raster types narrowed the hole by label only, which is what you said and what the file's own paragraph about exemptions asks for — my previous commit did not meet it.

The exemption is now granted on the payload: the first bytes are base64-decoded and checked against the signature for the declared format (PNG 89 50 4E 47 0D 0A 1A 0A, JPEG FF D8 FF, GIF GIF87a/GIF89a, BMP BM, WebP RIFF…WEBP, AVIF ftyp at offset 4). Anything that will not decode, or decodes to something else, keeps no span and falls to the sweep like any other data URI. At most 24 base64 characters are decoded per match.

After:

data:image/png;base64,<credential>    1 placeholder, swept
data:image/jpeg;base64,<credential>   1 placeholder, swept
data:image/webp;base64,<credential>   1 placeholder, swept

It also caught three of my own tests from the previous commit: they dressed the same PNG in a jpeg, webp and gif label — which is to say they pinned the label-only behaviour rather than the intended one. They now build real bytes per format, and six new cases assert the converse: a non-image payload is swept however it is labelled. Mutation-checked — making the signature check return true unconditionally turns exactly those six red, and nothing else.

The real-image path is unaffected: vision-repaint and browser-use both still pass, which is a real PNG from a real Chromium through the whole record-and-replay cycle.

Full suite, five runs: 10, 10, 11, 11, 15 failures against a main baseline of 10 — two of them identical to it line for line, and every extra in the compare/fork-provenance family this machine has flaked in all day, on main as well as here. conformance, fidelity and plugin neutrality clean.

[xizhuomengcontin]

@orcacode-review please re-review.

5a32622 grants the data: URI exemption on the payload rather than on the declared type: the first bytes are base64-decoded and checked against the signature for the format the label claims, and anything that does not match keeps no span and falls to the entropy sweep. Six cases pin it, and three of my own earlier tests turned out to be pinning the label-only behaviour — they now build real bytes per format.

CI is green on this head. Locally: vision-repaint and browser-use still pass (real PNG, real Chromium, full record-and-replay), conformance 60 events 0 failures, fidelity 0 regressions, plugin neutrality 0, and the full suite matches main's failure set line for line on the runs where this machine's known compare/fork-provenance flake stays quiet.

[orcacode-review]

🐳 OrcaCode Review

Found 3 issues in this PR: 🟠 3 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 551 calls · 49.1M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Cover the Anthropic image spelling too, or the exemption still misses half the providers

The exemption is keyed to one provider's serialization of an image: data:image/<type>;base64,<payload>. That is how the OpenAI chat-completions and Responses dialects put an image on the wire (packages/providers/src/translate/openai.ts:293, packages/providers/src/translate/openai-responses.ts:261). The Anthropic dialect — the untouched other side, packages/providers/src/translate/anthropic.ts:256 — puts the same raster bytes on the wire as {"type":"image","source":{"type":"base64","media_type":"image/png","data":"<base64>"}}, with no data: prefix anywhere. spansOf therefore grants it no span, and #scanEntropy sweeps it exactly as before.

Failure, concretely: orca record on anything talking /v1/messages with a screenshot (Claude Code with a pasted image, the Anthropic SDK with computer use — the anthropic-sdk check in CI sends text only, so nothing covers this) still rewrites the screenshot into <secret:high_entropy:…> placeholders. I verified the mechanism against the real rules: a bare base64 raster payload under a "data" key yields swept 20+ char mixed-alphabet runs (the test's own 1x1 PNG gives 1/1 swept; 3 KB of noise gives 55/56), so the recorded bytes are not the bytes that were sent and redactions.json fills with thousands of records that were never secrets — the two costs the commit message says this change removes. The comment at line 119 claims to follow the convention of PROTOCOL_ID_VALUE, but that pattern enumerates every dialect's spelling (id|tool_use_id|tool_call_id) precisely because one spelling is not the shape.

Fix: add the Anthropic spelling to the exemption, signature-checked the same way, e.g. a second pattern /"data"\s*:\s*"([A-Za-z0-9+/=\\]+)"/g accepted only when isDeclaredRaster validates it and the enclosing part says "type":"base64" with an image/... media_type (match the preceding "media_type":"(image\/[a-z+.-]+)" rather than the type string alone, or the hole F1's own comment warns about reopens). Cheaper alternative: exempt any quoted data value that base64-decodes to a raster signature, since the signature check, not the prefix, is what the commit argues earns the exemption.

[xizhuomengcontin]

All three reproduced, and all three are fixed in 3c8c221 — by one change, because they share a cause: the rule was keyed on how an image is written rather than on whether the bytes are one.

Measured before the fix:

prefix attack      iVBORw0KGgo + <credential>        exempt, credential verbatim in the trace
                   AAAAIGZ0eXA + <credential>        exempt, credential verbatim
                   <real PNG> + <credential>         exempt, credential verbatim
span overrun       <image> \n <token>                token verbatim, rulesFired={}
                   <image> " " <token>               swept, rulesFired={high_entropy:1}
spelling           OpenAI data: URI                  intact
                   Anthropic source.data             shredded

The exemption is now granted on the payload: a base64 run is decoded and has to be one whole raster image — PNG's chunk walk ending at IEND with nothing after it, JPEG's FF D9, GIF's 0x3B, a declared size that matches for BMP, WebP and AVIF. A structure that has to close is not something an appended secret survives; the syntax around it stops mattering, which covers Anthropic's spelling and whatever the next dialect does; and the run is delimited by the base64 alphabet alone, so it cannot walk past the payload.

After:

prefix + credential        swept
real image + credential    swept
escaped newline + token    swept, rulesFired={high_entropy:1}
Anthropic source.data      intact
a real screenshot          intact

Three notes on where I drew the line:

• A secret hidden inside otherwise valid pixel data is still not detectable, and I do not think any content rule could be. The claim is only that a payload which is not a picture does not get a picture's exemption.
• Cost: 34 ms → 55 ms on a 2.67 MB body holding a 2 MB PNG, measured against the previous commit built in a separate worktree. The head is still checked before decoding, so a long run that is not an image costs four bytes rather than a full decode.
• Your suggestion of refusing a payload containing a long printable-ASCII run I did not take: PNG tEXt chunks legitimately carry text, so it would re-shred real screenshots carrying metadata. Requiring the structure to close gets the same cases without that risk.

The test file is rewritten around the new contract rather than patched again — 26 cases, including every shape above. vision-repaint and browser-use both still pass, which is a real PNG from a real Chromium through the whole record-and-replay cycle. Full suite: three runs at 10, 11, 11 against a main baseline of 10, one identical line for line and the extra in the fork-provenance family this machine flakes on with main as well. conformance, fidelity and plugin neutrality clean.

[orcacode-review]

🟠 P1 The exempt span runs past the payload across JSON escapes: a token on the next line is swallowed into the data URI and never swept

The payload character class [A-Za-z0-9+/=\\]+ (line 125) admits \ and every letter, so a JSON-escaped newline/tab/CR does not end the run: \n is \ + n, both in the class. The exempt span therefore continues across the line break and absorbs whatever base64-alphabet text follows it, and #scanEntropy skips any TOKEN that starts inside the span (line 354). The result is that a value on the next line of the same string field is never swept — written verbatim on the record path, with hits empty, rulesFired() empty, no redacted attr on the event and nothing in redactions.json.

Reproduced against the module (copies of the files at this commit, Node type-stripping):

head = '{"text":"data:image/png;base64,' + <real PNG base64>
head + '\\n'  + 'Zx9Qw3Lm7Rt2Yv8Bn4Kd6Hs1Jf5Gp0Ac' + '"}'   -> LEAK   rulesFired={}
head + '\\nAAAA\\n' + 'Zx9Qw3Lm7Rt2Yv8Bn4Kd6Hs1Jf5Gp0Ac' + '"}' -> LEAK   rulesFired={}
same value, double-encoded as the writer stores it (trace line / spilled blob: "{\"text\":\"...\\n...\"}") -> LEAK
head + ' '  + token  -> swept,  rulesFired={"high_entropy":1}
head + '\\n ' + token -> swept (the space ends the run)
head + ':'  + token  -> swept
head + '\n' (raw) + token -> swept

The pre-change file (git show 0d78203:packages/core/src/redaction.ts) sweeps every one of these, so this is a regression introduced by the exemption, and it is not the same trigger as the _/- straddle: here the token is entirely inside the span because the span itself runs on, and the payload-delimiter fix proposed for that one (grant the span only when the next character is not a token character) does not help, because the character after the run here is a quote.

Realistic shapes that hit it: any recorded string field (message text, tool result, an agent's own log or file dump) that contains an inline image data URI at the end of a line and a random-looking token on the next line — e.g. a .env or config dump pasted under a screenshot — and equally any agent that wants to smuggle a credential: data:image/png;base64,<12 valid signature characters><newline><secret>.

Fix: stop the run at anything that is not actually payload. Accept a backslash only when it begins an escape that stands for payload text — / or \ — e.g. (?:[A-Za-z0-9+/=]|\\[/\\])+ — instead of [A-Za-z0-9+/=\\]+. The trailing \" the previous commit cared about then falls outside the span, which is harmless because \ and " are not TOKEN characters, and isDeclaredRaster's head is unchanged (it strips backslashes before decoding).

[orcacode-review]

🟠 P1 Only the first 2–12 payload bytes are checked, so any caller can buy the exemption with a prefix and land a credential on disk verbatim

isDeclaredRaster decodes slice(0, 24) — at most 18 bytes — and asks only whether the head is the format's magic. Nothing checks that the rest of the payload is that format, so the exemption covers an arbitrary blob as soon as it starts with the magic bytes, which are themselves just base64 text the caller writes.

Worked case, verified by replaying #scan/#scanEntropy at HEAD: the trace text

data:image/png;base64,iVBORw0KGgo + <base64 of a credential>

is matched by RASTER_DATA_URI, the head decodes to the 8-byte PNG signature, and the whole span is pushed into spansOf — so #scanEntropy skips every token in it. The named rules cannot see the credential either, because base64 destroyed its shape (the file's own comment and the new test a base64-encoded key is invisible to the named rules, so the sweep has to see it say exactly this). Result: the credential is written to events.jsonl / blobs verbatim and redactions.json records nothing, whereas the byte-identical payload behind data:application/pdf;base64, is swept. orca scrub uses the same Redactor, so the second pass cannot recover it either.

This is the outcome the doc comment above RASTER_SIGNATURES says the check exists to prevent — "the same encoded credential is swept behind application/pdf and written verbatim behind image/png, decided by four bytes of type string" — and the same one the enum test still sweeps a data: URI of type %s pins for the non-raster types. Prefixing 8 characters moves that test's payload from "swept" to "verbatim"; the only test that exercises the exemption's payload check uses a real PNG, so the hole is invisible to the suite. The weaker signatures need even less: bmp is 2 bytes (Qk…), avif is ftyp at offset 4, so data:image/avif;base64,AAAAIGZ0eXA… is spared after 12 characters.

Fix: validate the payload against the format it claims, end to end, and let anything that fails fall back to the sweep (which the code already does for a head that does not decode). Concretely, walk the payload rather than peeking: png — signature, then IHDR as the first chunk, then chunk-walk by declared length to IEND with nothing after it; jpeg/jpg — SOI, then EOI (FF D9) at the end; gif — header plus 0x3B trailer; bmp — the file-size field at offset 2 equals the decoded length; webp — RIFF size equals decoded length − 8; avif — the ftyp box size is consistent. An appended or spliced credential then breaks the structure (a chunk length, a declared size, or a missing end marker) and the span is not granted. If the full walk is too much here, the cheaper equivalent is to decode the whole payload and refuse the exemption when it contains a ≥20-character run of printable ASCII: base64 of a credential decodes to text, and pixel data does not.

[xizhuomengcontin]

@orcacode-review please re-review.

3c8c221 addresses all three findings with one change, because they shared a cause: the rule was keyed on how an image is written rather than on whether the bytes are one. A base64 run is now decoded and has to be one whole raster image — PNG chunk-walked to IEND with nothing after it, JPEG's FF D9, GIF's 0x3B, a matching declared size for BMP, WebP and AVIF — so a prefix or an appended secret breaks the structure, Anthropic's source.data spelling is covered without naming it, and the run is delimited by the base64 alphabet alone so it cannot walk past the payload.

I did not take the printable-ASCII heuristic: PNG tEXt chunks legitimately carry text, so it would re-shred real screenshots that carry metadata.

CI is green on this head. Locally: 26 cases in the rewritten test file, vision-repaint and browser-use passing (real PNG, real Chromium, full record-and-replay), conformance 60 events 0 failures, fidelity 0 regressions, plugin neutrality 0, and the full suite matching main's failure set line for line on the run where this machine's known fork-provenance flake stayed quiet.

[orcacode-review]

🐳 OrcaCode Review

Found 3 issues in this PR: 🟠 3 P1.

Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.

---

packages/core/src/redaction.ts (line 389): 🟠 P1 Skip only tokens that lie wholly inside an exempt span, not those that merely start in one

The new raster spans (line 282) are the first spans that can be followed immediately by a character that ends the span but continues a TOKEN match — - and _ are not in BASE64_RUN's class [A-Za-z0-9+/=] but they are in TOKEN's [A-Za-z0-9_-]{20,}. The containment test at line 389 only looks at the token's start, so a token that starts inside the image payload and runs past the span's end is skipped in full and the text after the image is never swept.

Measured with the real Redactor, a real 16x16 random-pixel PNG whose base64 carries no = padding, and an unpatterned high-entropy token placed after the payload:

before gap="" imageIntact: false secretLeaked: false rules: {"high_entropy":4}
before gap="" imageIntact: false secretLeaked: false rules: {"high_entropy":4}
before gap="-" imageIntact: false secretLeaked: false rules: {"high_entropy":4}
after gap="" imageIntact: false secretLeaked: false rules: {"high_entropy":4}
after gap="" imageIntact: true secretLeaked: true rules: {}
after gap="-" imageIntact: true secretLeaked: true rules: {}

The only difference between the leaking and the non-leaking runs is that one character, so this is the span test and not the entropy heuristic: with the token glued straight onto the payload the run merges, decodes to something that is not a whole image, and everything is swept (4 placeholders); with a single _ or - in between, the image keeps its exemption and the credential is written verbatim with an empty ledger — nothing in redactions.json, so nothing tells the reader a secret was left behind. Any of the three separators that are not token characters (space, :, /) still redacts, which is why the added tests ("sweeps a token that follows the image after …", with gaps \n, \nAAAA\n, , :) all pass: none of them uses a gap that continues the token. This is a regression against main, where no exemption existed and all three layouts were swept (the run above shows it).

Concrete fix: make the test whole-token containment rather than a start test, so the token is skipped only when it lies entirely inside an exempt span:

  if (spans.some(([a, b]) => start >= a && start + token.length <= b)) continue;

With that, <payload>_<secret> falls through to the entropy sweep (as it does today for <payload><secret>), which replaces the merged run and keeps the fail-closed direction consistent. A looser test — skipping on any overlap — would be wrong in the other direction: it would exempt text that lies outside the image, which is how this defect arose.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 430 calls · 39.8M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Reject a base64 run with interior = padding before granting the image exemption

Buffer.from(payload, 'base64') silently ignores everything from the first = (node: Buffer.from('QUJD==QUJD','base64') → 'ABC', 3 bytes). isWholeRasterImage therefore validates only a prefix of the run, while spans.push([m.index, m.index + run.length]) marks the whole run exempt from the entropy sweep. Since BASE64_RUN admits = and base64 characters anywhere, any base64-shaped text appended to a padded image payload is exempted too and reaches the trace verbatim.

Reproduced against this tree (packages/core/src/redaction.ts as committed), with the repo's own 1×1 PNG constant (96 chars, ends == — 70 bytes, 70 % 3 = 1, so every such image is padded):

const PNG = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==';
const NOISE = Buffer.from('<30-char secret>'.repeat(6)).toString('base64');   // 232 base64 chars
redactString(JSON.stringify({ url: 'data:image/png;base64,' + PNG + NOISE }))
// → value.includes(NOISE) === true, value.includes('<secret:high_entropy') === false, rulesFired() === {}

The same input at the base commit (0d78203, no exemption) is swept. So this change introduces a path where a base64-encoded credential is written to events.jsonl/blobs in the clear with redactions.json recording nothing — precisely the case this commit claims to have closed ("sweeps a real image with a credential appended to it", "A structure that has to close … is not something an appended secret survives"). The new test 'sweeps a real image with a credential appended to it' only passes because it builds the input as PNG.replace(/=+$/,'') + NOISE — i.e. it removes the padding that makes the appended secret invisible; with the real (padded) payload that test fails. The truncation also defeats the closing walk for the same reason, so IEND/FF D9-at-the-end is satisfied by the image alone while the span covers whatever follows.

Fix (verified: appended credential swept again; the whole-image cases in data-uri-redaction.test.ts, padded and unpadded, for png/jpeg/gif/bmp/webp still exempt):

const payload = run.replace(/\\(.)/g, '$1');
// `Buffer.from(…, 'base64')` ignores everything from the first `=`, so a run that continues
// past interior padding decodes to a *prefix* of itself — the closing walk would pass on an
// image that is not the whole run, and the span below would cover the rest of the run too.
// Padding is only ever legitimate at the very end.
if (payload.replace(/=+$/, '').includes('=')) continue;
let bytes: Buffer;

(equivalently if (!/^[A-Za-z0-9+/]*={0,2}$/.test(payload)) continue;).

[orcacode-review]

🟠 P1 A caller-written wrapper still buys the image exemption: validate image data, not its framing

isWholeRasterImage never validates content — every arm only checks bytes the caller writes around the content, so a credential base64-wrapped in framing comes back exempt and is written to the trace verbatim. This is exactly the failure the commit says it closed ("iVBORw0KGgo followed by a base64 credential passed a head-only check, and the credential landed in the trace verbatim with nothing in redactions.json"), just needing a longer wrapper.

What each arm actually verifies:

• JPEG (150-152): first three bytes and last two. b64(FF D8 FF + <credential> + FF D9) passes — 5 bytes of decoration.
• GIF (155-157): the six-byte header and one trailer byte. b64('GIF89a' + <credential> + ';') passes — 7 bytes.
• PNG (136-147): chunk lengths only. CRCs are never checked and the IDAT stream is never inflated, so sig + IHDR + IDAT{<credential>} + IEND with a self-consistent length field passes, with no pixel data at all.
• BMP (160-162), WebP (165-167), AVIF (170-178): a size field the same caller computes, plus (AVIF) boxes tiling the buffer. BM + <len> + <credential>, RIFF + <len> + WEBP + <credential>, and an ftyp-plus-payload box tiling all pass.

Measured with the real Redactor (payload built in $TMPDIR, unpatterned high-entropy token, same input run against the pre-change file and HEAD):
before (0d78203) forged png kept: false rules: {"high_entropy":1}
before (0d78203) forged gif kept: false rules: {"high_entropy":1}
after (HEAD) forged png kept: true rules: {}
after (HEAD) forged gif kept: true rules: {}
i.e. the whole run is now exempted, rulesFired() is empty so neither redactions.json nor the manifest's redaction.rules_fired records anything, and the credential is one Buffer.from(payload,'base64') away (verified: the decoded payload contains the token verbatim).

Concrete fix — make the exemption depend on bytes that cannot be framing alone:

1. PNG: verify each chunk's CRC32 and zlib.inflateSync the concatenated IDAT, requiring the result to be exactly height * (1 + width * bytesPerPixel) as declared by IHDR. A wrapper carrying a credential then has to be a genuinely decodable PNG.
2. JPEG: walk the marker chain (FF xx segments, skipping entropy-coded scan data) to the EOI, requiring a SOF header and dimensions/component count consistent with the scan length — not just FF D9 at the end.
3. GIF: walk past the logical screen descriptor, global colour table and image descriptor through the LZW sub-blocks to the trailer, requiring the block chain to consume the payload exactly.
4. BMP/WebP/AVIF: require the declared geometry (BMP DIB header, WebP VP8/VP8L chunk, AVIF ispe/mdat sizes) to be consistent with the payload length rather than trusting a size field the caller set.
5. For any format that cannot be validated without a real decoder, do not grant the exemption — the sweep running is the safe direction for a redaction rule, and the current no-exemption behaviour (main) is strictly better than granting one on a trailer.

[xizhuomengcontin]

All three reproduced, and 2eb8b03 fixes them. The first is the one that mattered — the other two were ways of getting past it.

Framing is not content. Every arm verified only the bytes around the payload:

PNG signature + IHDR + IDAT{credential} + IEND   exempt, credential verbatim
FF D8 FF + credential + FF D9                    exempt, credential verbatim
GIF89a + credential + 0x3B                       exempt, credential verbatim
BM + <length> + credential                       exempt, credential verbatim
RIFF + <length> + WEBP + credential              exempt, credential verbatim

I was ready to argue that full validation was too expensive for a write path, and measured it instead of asserting it. On a 1 MB PNG: chunk walk 0.2 ms, inflate 1.2 ms — against the 55 ms the base64 decode already costs. The objection did not survive the measurement, so the check does the work you asked for: a PNG's IDAT has to inflate to exactly the size its IHDR declares, and a JPEG's marker chain has to reach EOI with a frame header in it.

GIF, BMP, WebP and AVIF lose the exemption rather than gain a validator. They were listed on no evidence — the case this exists for is a browser screenshot, which is PNG or JPEG — and each validated by a size field the caller computes. A format with no validator gets the sweep.

Padding. Confirmed: <image>=<credential> decoded to a valid image because Buffer.from stops at the first =, and the span covered the credential. The run now ends at its padding.

Whole-token containment. Confirmed with an unpadded PNG, exactly as you measured:

before  gap="_"  image kept: true   secret leaked: true   rules: {}
        gap="-"  image kept: true   secret leaked: true   rules: {}
after   gap="_"  secret leaked: false                     rules: {high_entropy: 16}
        gap="-"  secret leaked: false                     rules: {high_entropy: 15}

The count is not one because replacing the straddling token breaks the image and what is left is swept in pieces — the fail-closed direction you named, and what the test now asserts.

Two things worth flagging about my own work here:

• The test file's fixtures were runs of a repeated byte, so after the narrowing three of them had stopped testing anything — they passed because the payload could never have tripped the sweep. Rewritten with real high-entropy payloads.
• My first forgery fixture carried a zero-dimension IHDR, so it was being rejected on dimensions rather than on content. The mutation check caught that: short-circuiting the inflate left it green. Fixed, and it now turns red for the right reason.

Unchanged: a secret hidden inside real pixel data is not detectable here, and I do not think any content rule could be. The claim is only that the payload has to be a picture.

vision-repaint and browser-use both still pass — a real PNG from a real Chromium through the whole record-and-replay cycle — and a real image is kept byte for byte at every base64 padding length. Full suite 2078 passed / 10 failed against a main baseline of 10, identical line for line across two runs; conformance, fidelity, neutrality clean; integrations 14 checked, 0 failed.

[xizhuomengcontin]

@orcacode-review please re-review.

2eb8b03 makes the exemption depend on content: a PNG's IDAT has to inflate to the size its IHDR declares, a JPEG's marker chain has to reach EOI with a frame header, and GIF/BMP/WebP/AVIF lose the exemption rather than gain a validator. The padding hole and the whole-token containment are fixed with it. I measured the cost rather than assuming it — 0.2 ms for the chunk walk and 1.2 ms for the inflate on a 1 MB PNG, against 55 ms for the base64 decode that was already happening.

Locally: 21 cases in the rewritten test file, mutation-checked; vision-repaint and browser-use passing; full suite 2078 passed / 10 failed against a main baseline of 10 with an identical failure set; conformance, fidelity, neutrality clean; integrations 14 checked 0 failed.

[orcacode-review]

🐳 OrcaCode Review

Found 4 issues in this PR: 🟠 4 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 668 calls · 66.7M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Enforce the JPEG frame/scan requirement at the EOI transition, not only inside the SOS branch

isWholeJpeg tracks sawFrame (line 193) but only consults it at line 214, inside the marker === 0xda branch — it is checked after the walk has already advanced past the SOS. A marker chain that never contains an SOS therefore reaches the terminal transition at line 203 and is accepted, so the exemption this commit exists to narrow is still bought with a handful of caller-written bytes.

Reproduced against the code at HEAD (Node 22, redaction.ts loaded directly):

bytes = FF D8 | FF FE 00 34 | <48-byte high-entropy credential> | FF D9   (65 bytes)
value = {"model":"stub-1",...,"url":"data:image/jpeg;base64,<base64 of those bytes>"}

redactString(value).value still contains the run byte for byte, redactor.rulesFired() is {}, so nothing lands in redactions.json and the manifest's redaction.rules_fired says nothing was removed — while the credential is recovered by base64-decoding the value that was written. The control (the same credential with no JPEG wrapping) is swept, high_entropy: 1, so it is the exemption and not the content that saves it. The only image-ish bytes present are the SOI/EOI pair and a length field the caller writes: exactly the case the commit message tabulates as closed ("FF D8 FF + credential + FF D9 — exempt, credential verbatim", "five bytes of decoration for JPEG"). The test that is supposed to pin it ('sweeps a JPEG that is SOI, a credential and EOI') does not, because its random payload makes the segment length field inconsistent rather than because no frame header is present.

Fix (fix-first): make the requirement part of the terminal transition instead of a branch that a payload can skip. Track the scan as well and check both where the walk accepts EOI:

let sawFrame = false;
let sawScan = false;
...
if (marker === 0xd9) return at === b.length && sawFrame && sawScan; // EOI, and nothing after it
...
at += len;
if (marker === 0xda) {
  sawScan = true;                                   // the frame check moves to the EOI return
  while (at + 1 < b.length) { ... }
}

and reject the buffer when the loop falls out without EOI (it already returns false). Requiring a frame and a scan is necessary but not sufficient — the credential can still sit in a segment's data rather than in the entropy-coded scan, which is the second finding.

[xizhuomengcontin]

All four reproduced, and 166f7d8 fixes them. Controls on each: the same credential unwrapped is swept high_entropy: 1, and for the interlace one, flipping IHDR byte 12 back to 0 is swept — so it was the exemption saving them, not the content.

The JPEG arm is gone rather than repaired. Your own note is why: COM/APPn carry arbitrary bytes, DQT/DHT are arbitrary within a shape, and the entropy-coded scan cannot be told from a credential without a full Huffman decode. Requiring a frame and a scan at the EOI transition would have closed the case you reproduced and left the second finding's version of it open, so "a marker chain that closes" was never going to be more than framing. GIF, BMP and WebP went earlier for the same reason and this is the same reason. The cost is real and stated in the header: JPEG screenshots get shredded again.

[orcacode-review]

🟠 P1 Do not exempt a container's uncompressed metadata chunks (tEXt/COM/APPn) from the sweep

The chunk walk accepts any chunk type and rasterSpans exempts the whole base64 run, so every byte of a non-IDAT chunk is exempt too — an uncompressed metadata field is not pixels, and a reader recovers it by base64-decoding what was written.

Reproduced against the code at HEAD:

bytes = 89 50 4E 47 0D 0A 1A 0A            (signature)
      + IHDR(1x1, depth 8, colour 0)       (13-byte IHDR, so only the size formula has to hold)
      + tEXt("Comment\0" + 300 random bytes)
      + IDAT(deflate of two zero bytes)    (inflates to 2 = height * (rowBytes + 1), so the check passes)
      + IEND

redactString('{"u":"data:image/png;base64,<that>"}') keeps the run byte for byte, rulesFired() is {}, and the credential is recoverable from the value that was written. The identical bytes with the IEND chunk removed are swept (high_entropy: 6), so it is the exemption that saves them, not the content. The same holds for JPEG with a fully valid chain: FF D8 | FF FE 00 34 <credential> | SOF0 | SOS | entropy-coded scan | FF D9 is accepted and the credential inside the COM segment is written verbatim.

So "the payload has to be a picture" is satisfied by a one-pixel image plus a comment field, and the header's claim that the exempt bytes "cannot hide a credential a reader could recover" is false for every ancillary/metadata chunk. Note this is not the same defect as the missing frame check: requiring a frame and a scan leaves it in place, because a valid image can carry the credential in COM/APPn (JPEG) or tEXt/iTXt/zTXt/unknown (PNG) alongside real pixels.

Fix (fix-first): exempt only the container's structural and compressed bytes, and let the entropy sweep see the rest. Concretely, have rasterSpans return spans that cover the runner's
signature/IHDR/IDAT bytes (and their CRCs) rather than the whole run, mapping chunk byte offsets back to base64 offsets, and for JPEG cover the SOI/SOF/SOS/DHT/DQT header bytes plus the entropy-coded scan but not COM/APPn data. If that offset mapping is more than you want here, the minimal safe version is to refuse the exemption whenever the container carries a segment whose data is not structural: for PNG reject any chunk whose type is not IHDR/PLTE/IDAT/IEND, for JPEG reject any COM/APPn segment with a non-empty payload. Either way those bytes go back through the sweep instead of being exempted.

[xizhuomengcontin]

Reproduced exactly as written — a 1x1 IHDR, a tEXt holding the credential, an IDAT that really inflates to 2 bytes — exempt, rulesFired() === {}, credential recoverable from the trace. This was the finding that decided the shape of the fix.

I did not take the offset-mapping option. Mapping chunk byte offsets back to base64 offsets can only exempt on 3-byte/4-character boundaries, and a token straddling one of those boundaries then gets replaced and breaks the image anyway — so it buys precision at the cost of a new class of partial-shredding bug.

What it does instead is refuse the exemption to any chunk whose bytes it cannot account for. The admission rule is not "this chunk is harmless" but "exempting this chunk cannot hide anything the sweep was protecting": IHDR, IDAT, PLTE and IEND, plus gAMA/sRGB/pHYs/tIME/sBIT/bKGD each held to its exact spec length — every one of those is shorter than MIN_ENTROPY_LENGTH (20), so none can carry a run #scanEntropy would ever have flagged. cHRM is fixed-length too and is not on the list, because 32 bytes is long enough to matter. Everything variable-length — tEXt, iTXt, zTXt, iCCP, unknown types — costs the whole run its exemption.

PLTE is admitted only on colour type 3, where the palette is the image's colours and is exempt on the same footing as the pixel stream; on truecolour it is a suggested palette, which is up to 768 bytes a caller picks and nothing reads.

The fail-closed direction was checked too, because refusing too much would make the PR pointless: all five PNGs in this repo, produced by real encoders, are IHDR + IDAT* + IEND with no ancillary chunks at all, and all five stay byte for byte. vision-repaint still passes.

[orcacode-review]

🟠 P1 Do not exempt an interlaced PNG on "inflates to something" — one IHDR byte skips the whole content check

isWholePng grants the entropy-sweep exemption without any size validation when IHDR's interlace byte is non-zero: if (interlace !== 0) return raw.length > 0; accepts any IDAT that inflates to at least one byte. The interlace byte is written by whoever wrote the payload, so this is a framing-only exemption — exactly what this commit says it removed ("A credential wrapped in framing is neither").

Verified against this checkout (node 22, the module run from source with --experimental-strip-types):

• sig + IHDR(16x16, depth 8, colour 2, interlace 1) + IDAT(deflate(<30 credential bytes>)) + IEND — a "PNG" with no pixels at all — is returned byte-for-byte by Redactor#redactString with rulesFired() === {} (the sweep never runs over the span), and the credential is recovered from the trace by inflating the IDAT.
• The identical forgery with IHDR byte 12 flipped to 0 is swept ({ high_entropy: 1 }).
• Under the base of this diff (0d78203) the same string is redacted, so the behaviour is introduced by this change.

Fix: Adam7 sizes are computable, so compute one instead of skipping the check. For the seven passes with (x0,y0,dx,dy) = (0,0,8,8), (4,0,8,8), (0,4,4,8), (2,0,4,4), (0,2,2,4), (1,0,2,2), (0,1,1,2), let pass_width = ceil((width - x0)/dx), pass_height = ceil((height - y0)/dy), and require raw.length === Σ pass_height * (1 + ceil(pass_width * channels * depth / 8)). If that is too much to add here, do not accept on a successful inflate alone: an image this check cannot size gets the sweep, like every other format with no validator.

[xizhuomengcontin]

Confirmed, including your control: with interlace 1 the forgery is returned verbatim and rulesFired() is {}; with byte 12 flipped to 0 the same bytes are swept. So it was the interlace byte and nothing else.

Computed rather than skipped, as you suggested — the seven passes with pass_width = ceil((width - x0) / dx), pass_height = ceil((height - y0) / dy), summed as pass_height * (1 + ceil(pass_width * channels * depth / 8)), with a pass contributing nothing when either dimension is zero.

I did not want to trust my own arithmetic here, since a wrong sum silently puts real interlaced screenshots back through the shredder. Two independent checks: the closed form agrees with a pixel-by-pixel simulation that actually enumerates which columns and rows land in each pass, across 11,200 width/height/depth/channel combinations, 0 disagreements; and 8x8 truecolour depth 8 comes to 207 against 200 non-interlaced, which is the value the spec's own layout gives. The tests carry the simulation, not the closed form, so the two cannot drift into agreeing.

Interlaced images at 8x8, 64x48 and 17x23 are exempt; one byte off in either direction is swept.

[orcacode-review]

🟠 P1 Verify the IDAT stream consumed the whole chunk — trailing bytes inside IDAT carry a credential out unredacted

inflateSync stops at the end of the zlib stream, so nothing after it inside an IDAT chunk is ever examined, and the chunk length is the caller's (the CRC is deliberately not checked). <valid zlib stream for the declared pixels> || <arbitrary bytes> therefore passes the chunk walk and the raw.length === height * (rowBytes + 1) check, and the whole run is skipped by the entropy sweep.

Reproduced against this checkout: a 40-character base64-alphabet secret (the shape the sweep exists to catch, and the shape this file's own tests use) placed inside the IDAT of a normal non-interlaced IHDR(16x16, 8, 2) PNG, with the chunk length covering it, is written to the trace verbatim — redactString returns the run untouched, rulesFired() === {}, and the secret string is present in the output. The same body through 0d78203's redactor yields <secret:high_entropy:…> and no secret, so a credential that this change's base removed is now kept. This is exactly the case the doc-comment above rasterSpans claims is not purchasable ("bytes that cannot hide a credential a reader could recover"); the caller ships a real screenshot and hides the credential inside the framing around the pixel stream.

Fix (one line, verified on node 22): have the stream prove it consumed the whole chunk before the exemption is granted.

let raw: Buffer;
try {
  const bytes = Buffer.concat(idat);
  const inflated = inflateSync(bytes, { info: true });
  // bytesWritten is the input actually consumed by the stream; anything past it is
  // unexamined framing, which is what a caller hides a credential in.
  if (inflated.engine.bytesWritten !== bytes.length) return false;
  raw = inflated.buffer;
} catch {
  return false;
}

(zlib.inflateSync(buf, { info: true }) returns { buffer, engine } and engine.bytesWritten is the consumed input length — measured 16 for a 16-byte stream followed by 12 bytes of junk.) The JPEG arm has the same shape — APP/COM segment payloads and the entropy-coded scan are walked over without being examined — so if a JPEG exemption is kept, its guarantee should be stated as "a marker chain that closes", not as content.

[xizhuomengcontin]

Reproduced, and your fix is the one that shipped. inflateSync(buf, { info: true }) reports engine.bytesWritten as the consumed input — measured 11 for an 11-byte stream with 24 bytes of junk appended — so the check is result.engine.bytesWritten !== stream.length → false. One wrinkle worth recording: @types/node models only the plain-Buffer overload, so the { buffer, engine } shape is spelled out at the call site rather than inferred.

Taking the mutation test as the real evidence: removing that single line turns sweeps a real image carrying a credential inside IDAT past the end of the zlib stream red on its own.

You are right that the header overclaimed, and it has been rewritten. It now says the payload has to be a picture and explicitly does not claim a picture cannot be abused: a stored-mode deflate block whose "pixels" are a credential inflates to the declared size like any other, and no content rule catches that. The line this draws is around bytes that are not pixels; the pixels themselves are conceded.

[xizhuomengcontin]

@orcacode-review please re-review.

All four P1s from the last round reproduced against 2eb8b03 before anything was changed, each with a control showing it was the exemption and not the content that saved the credential. 166f7d8 fixes them, and replies are on each thread.

The shape of the fix, since it is not four separate patches: the exemption now refuses any chunk whose bytes it cannot account for. The admission rule is not that a chunk is harmless but that exempting it cannot hide anything the sweep was protecting — structure, the pixel stream, and six ancillary chunks each held to its exact spec length, all of them shorter than MIN_ENTROPY_LENGTH. IHDR is read before the walk rather than during it, so no field is used before it is checked. The pixel stream must inflate, consume every byte of itself, and match the size IHDR declares, Adam7 included. JPEG loses its exemption outright, for the reason your first finding ends on.

Evidence:

• 45 cases in the test file; 12 mutations of the individual guards, all caught, in both directions — over-widening (cHRM admitted) and over-narrowing (pHYs refused) each turn a specific test red.
• The Adam7 sum agrees with a pixel-by-pixel simulation over 11,200 size/depth/channel combinations, and with the spec's 207 for 8x8 truecolour. The tests carry the simulation, not the closed form.
• All five PNGs in this repo, from real encoders, stay byte for byte.
• vision-repaint passes; 14 integration checks, 0 failed; conformance, fidelity and neutrality clean.
• 2103 unit tests pass, 10 fail — the same 10 as main, matched title by title against six recorded baseline runs. None are in packages/core.

Known and stated in the header rather than fixed: a credential written into the pixels themselves is not detectable, and JPEG screenshots are shredded again.

[orcacode-review]

🐳 OrcaCode Review

Found 2 issues in this PR: 🟠 2 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 689 calls · 90.1M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 inflateSync on IDAT has no maxOutputLength: a crafted PNG inflates to gigabytes and the process is OOM-killed

isWholePng decompresses the caller-supplied IDAT with no output bound, and only checks the size afterwards (return raw.length === pngRawLength(...) on redaction.ts:286). The expected size is already known before the inflate — it is derived from fields the function has read out of IHDR several lines earlier — so nothing forces the redactor to inflate a stream that declares gigabytes.

Demonstrated through the real code path (this file copied, node --experimental-strip-types): a PNG whose IHDR says 1x1 truecolour and whose IDAT is a 2.99 MB level-6 deflate stream of zeros (base64 run 3.98 MB, comfortably below the 8 MB regexp-stack limit reported separately) makes new Redactor({}).redactString(...) inflate towards ~3 GB; the process is killed by the OOM killer (Killed, exit 137). No exception is raised, nothing is caught, no message is printed: the recording (or orca scrub) dies mid-write and leaves its trace unsealed, which the repo's own comment says verifyIntegrity then reports as tampered. A smaller variant (10.8 MB IDAT, 240:1) was killed the same way; a 0.83 MB IDAT already costs 191 MB of RSS, so the cost per captured payload is unbounded as well as fatal at the top end. The redactor is the one place in the write path that runs over bytes the agent's tools, MCP servers and (for orca scrub) a third-party trace file supply — a compressed "image" that inflates to gigabytes is a cheap way to take the recorder down.

Concrete fix: bound the inflate by the size IHDR already declares, which makes an oversized stream fail closed through the existing catch:
const expected = pngRawLength(width, height, channels, depth, interlace === 1);
if (expected > MAX_RAW_PIXELS) return false; // e.g. 1 << 30, a policy cap
let raw: Buffer;
try {
const result = inflateSync(stream, { info: true, maxOutputLength: expected }) as unknown as {
buffer: Buffer; engine: { bytesWritten: number };
};
if (result.engine.bytesWritten !== stream.length) return false;
raw = result.buffer;
} catch { return false; } // ERR_BUFFER_TOO_LARGE = a bomb -> swept
return raw.length === expected;
A stream that inflates to more than expected now throws ERR_BUFFER_TOO_LARGE, which the existing catch turns into "not an image", so the sweep runs as it did before the exemption existed.

[xizhuomengcontin]

Confirmed, and it is worse than the report in one respect: the heap cap does not bound it at all, because zlib allocates outside the JS heap. A 400 KB IDAT declaring 1×1, run under --max-old-space-size=256, did not die — it finished, with RSS up 821 MB, in 450 ms. So the ceiling is physical memory, not a V8 limit, and there is no OOM error to catch on the way.

8404ff5 takes your fix as written: expected is computed before the inflate and passed as maxOutputLength, so an oversized stream fails through the existing catch and is simply not an image. Same payload after: RSS +3 MB, 104 ms.

I added the policy cap you suggested as well, at 1 << 28 (256 MiB — an 8K screenshot at four channels is about 127 MiB, so roughly double). It is not redundant: maxOutputLength: expected caps the inflate at whatever IHDR declares, and IHDR's dimensions are up to 2^32 each, so a bomb whose declared size is also enormous gets exactly as much room as it asked for. That case is the only one the cap decides — every cheaper forgery is refused on the size mismatch instead — so the test for it carries a stream that really does inflate to the 286 MiB its header claims, and removing the cap turns that test red on its own.

[orcacode-review]

🟠 P1 rasterSpans' BASE64_RUN blows the regexp stack (RangeError) on an ~8 MB contiguous base64 run, where the pre-change code was fine

BASE64_RUN = /(?:[A-Za-z0-9+/]|\\[/\\])+={0,2}/g is a backtracking + over the whole run, and V8's regexp stack (default --regexp-stack-limit, 1 MB) overflows once a single contiguous run reaches ~8 MB of [A-Za-z0-9+/]. matchAll then throws RangeError: Maximum call stack size exceeded out of rasterSpans -> spansOf -> #scanEntropy -> redactString. Nothing catches it: the try/catch in the loop only wraps Buffer.from, and it is reached after the iteration.

Reproduced end to end (copy of this file, node --experimental-strip-types, identical call depth):
new code: new Redactor({}).redactString(JSON.stringify({url: 'data:image/png;base64,' + <8 MB base64>})) -> RangeError: Maximum call stack size exceeded at RegExpStringIterator.next / rasterSpans (redaction.ts:304).
pre-change code (git cat-file blob e4b1b0e = the version at 0d78203): the same input, and 31 MB of it, completes without throwing. It could not throw, because the entropy sweep's TOKEN = /[A-Za-z0-9_-]{20,}/g cannot span +//, so real base64 arrives as thousands of short tokens. The new prefilter is the first thing that runs a + over the whole run, so this is a regression introduced by the change, and the failing input is exactly what the change is for: a large base64 image (>= ~8 MB of base64 = a ~6 MB screenshot, or any >= 8 MB base64 payload).

What a throw costs: TraceWriter.#write calls redactString(JSON.stringify(event))/the payload JSON, so append rejects; in orca record that surfaces at await writes.drain() (record.ts:412/476) or await drainMcpFrames(...) (record.ts:460) — both outside the try that funnels into abandon — so the command aborts, the writer is never closed, and the run is left unsealed (the file's own comment says an unsealed trace makes verifyIntegrity report the run as tampered rather than unfinished). In orca scrub the same throw aborts the scrub. Either way the whole recording is lost to one over-long string, with an error that names nothing.

Concrete fix: stop running a backtracking quantifier over the payload — locate runs by hand, which is linear and stack-free:
// instead of value.matchAll(BASE64_RUN)
for (let i = value.indexOf('iVBORw'); i !== -1; i = value.indexOf('iVBORw', i + 1)) {
if (i > 0 && isB64Char(value[i - 1])) continue; // the run really starts here
let end = i;
while (end < value.length) {
const c = value[end]!;
if (isB64Char(c)) { end += 1; continue; }
if (c === '\' && (value[end + 1] === '/' || value[end + 1] === '\')) { end += 2; continue; }
break;
}
// then the existing MIN_RASTER_CHARS / decode / isWholeRasterImage checks, span [i, end)
}
(with isB64Char = (c) => /[A-Za-z0-9+/]/.test(c), and end extended over at most two trailing = as today). A cheaper stop-gap — one that fails closed, at the cost of shredding the very largest images — is to continue (skip the run, so the sweep runs) when the run exceeds a length the decoder could ever accept, but the manual scan is what removes the failure mode.

[xizhuomengcontin]

Confirmed, including that it is a regression — though my first attempt at the repro said the opposite, which is worth recording.

I built the payload as a run of [A-Za-z0-9] with no + or /. On that input the base version throws too, from #scanEntropy's own value.matchAll(TOKEN), because without + or / the whole run is a single [A-Za-z0-9_-]{20,} match. That made the regression look pre-existing. Your reasoning is the thing that is load-bearing here: real base64 contains + and /, so TOKEN sees thousands of short matches and never backtracks across the payload. Re-run with real base64:

run	base (0d78203)	before 8404ff5
4.0 MB	ok 509 ms	ok 557 ms
6.7 MB	ok 795 ms	ok 854 ms
8.0 MB	ok 952 ms	RangeError
16.0 MB	ok 1957 ms	RangeError

Your 8 MB figure lands exactly. The test uses real base64 for this reason and says so, because the [A-Za-z0-9]-only version is a test that passes for the wrong reason on both sides.

Fixed with the manual scan. One deviation from your sketch: rather than indexOf('iVBORw') and walking, it scans left to right once and takes each maximal run as it comes, then checks whether that run starts with the signature. indexOf from hit + 1 would be quadratic on a payload of repeated iVBORw — every hit inside an already-scanned run walks to the run's end again — whereas advancing to the run's end is correct for free, since a signature inside a run does not start one. 16 MB now costs 2026 ms against the base version's 2026 ms.

[xizhuomengcontin]

@orcacode-review please re-review.

Both P1s reproduced against 166f7d8 before anything changed, and 8404ff5 fixes them. Replies are on each thread; the short version:

The bomb is worse than reported — the heap cap does not bound it, because zlib allocates outside the JS heap. A 400 KB IDAT declaring 1×1, under --max-old-space-size=256, did not die: it finished with RSS up 821 MB. So the ceiling was physical memory and there was no error to catch. Now expected is computed before the inflate and passed as maxOutputLength, plus a 256 MiB policy cap on what IHDR may declare — that second one is not redundant, since IHDR's dimensions are the caller's up to 2^32 and a bomb declaring its own huge size would otherwise get exactly the room it asked for. Same payload after: RSS +3 MB, 104 ms.

The regexp stack is fixed with a single left-to-right scan. I took a different route than the sketch: scanning runs as they come rather than indexOf + walk, because indexOf from hit + 1 is quadratic on a payload of repeated iVBORw. 16 MB of base64 now costs 2026 ms against the pre-PR redactor's 2026 ms.

One correction to my own work that is worth stating plainly: my first repro of the second finding said it was not a regression, because I built the payload from [A-Za-z0-9] only — which is one enormous TOKEN and overflows on both sides of the change. Your reasoning about + and / is what made it visible. With real base64 the pre-change code walks 16 MB fine and this branch threw at 8 MB, exactly as you measured. The test uses real base64 and says why.

Evidence: 49 cases; 15 mutations of the individual guards, all caught, including one that only fails when the bomb's header declares the size it inflates to. 2103 unit tests pass, 10 fail — the same 10 as main. 14 integration checks 0 failed; conformance, fidelity, neutrality clean; vision-repaint passes.

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 707 calls · 85.8M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Stop spreading rasterSpans into spans.push — it blows the stack on a value holding many images, aborting the write path

spread passes one argument per span, and rasterSpans returns one span for every whole PNG in the string. V8's argument-count limit is ~125k (measured here: push(...a) with 125k elements is fine, 150k throws RangeError: Maximum call stack size exceeded). The smallest whole PNG this file accepts (1×1, 8-bit greyscale, IDAT = 2 bytes) base64-encodes to 92 characters, so ~130k of them — about 12 MB of caller-controlled payload — overflow the call stack.

Reproduced on the working tree at HEAD with the module's own code:

const png = /* 1x1 greyscale PNG, base64 */;            // 92 chars
const value = JSON.stringify({ a: Array.from({length: n}, () => png) });
new Redactor({}).redactString(value);
// n = 120_000 -> ok (27 s);  n = 130_000 -> RangeError: Maximum call stack size exceeded
//   at spansOf (packages/core/src/redaction.ts:463)

Nothing on the write path catches it: writer.#write hands the whole serialized event — and, for a spilled payload, the whole body — to redactString (packages/core/src/writer.ts:234 and :247); redactedForm does the same for every string of a live replay request (packages/proxy/src/matching.ts:219, reached from RequestMatcher.match via comparableRequest, matching.ts:582); orca scrub does it per blob (packages/cli/src/commands/scrub.ts:102). A throw out of redactString mid-record is exactly the failure this commit sets out to remove, in its own words: the recording aborts and the trace is left unsealed, which verifyIntegrity then reports as tampered rather than unfinished. The input is reachable: readBody (packages/proxy/src/server.ts:451) puts no size limit on the base-URL route, and any agent, page, tool result or MCP server that can put a body in front of orca can produce a 12 MB text field full of small data-URI PNGs.

It is not the regexp this commit replaced — that scanner is genuinely linear now — but the same crash class survives at the call site, so "runs are found by hand, in one left-to-right pass — linear, stack-free" does not hold for a payload with many images. The added test only covers the opposite shape (one enormous run, data-uri-redaction.test.ts:628), which is why nothing catches it.

Fix: iterate instead of spreading, e.g.

for (const span of rasterSpans(value)) spans.push(span);

(or return spans.concat(rasterSpans(value))), and ideally cap the number of spans accepted, since an image-laden 12 MB field otherwise also costs tens of seconds of scanning.

[xizhuomengcontin]

Confirmed, both halves, and your numbers land exactly: 130,000 of the 92-character 1×1 greyscale threw RangeError from spansOf; 120,000 finished in 27.7 s.

bcb8da1 iterates instead of spreading. I also took the second suggestion, but not as a cap — the quadratic containment test is the thing that costs the 27.7 s, and a cap would trade one arbitrary failure for another. spansOf now returns spans sorted by start, and the sweep walks them with a single cursor carrying the furthest end seen so far: reach is max(b) over every span with a <= start, so start + len <= reach is exactly spans.some(([a, b]) => start >= a && start + len <= b), in one pass.

After: 130,000 images 0.9 s instead of throwing, 120,000 0.8 s instead of 27.7 s, 200,000 (18 MB) 1.4 s.

Worth recording that the quadratic cost is also new, which I did not assume. The placeholder spans that existed before this PR cannot produce it — <secret:…> contains : and <, which break TOKEN, so many spans never coincide with many tokens. Measured at 40k elements: placeholders 40 ms before / 42 ms after, images 142 ms before / 2683 ms after.

Touching the sweep's predicate is the real risk here, so it is checked twice rather than argued: the predicate against some(...) over 276,433 randomised judgements with overlapping, nested and zero-length spans, and the whole redactString against 8404ff5 over 3,000 randomised bodies mixing images, credentials, placeholders and protocol signatures. No disagreement in either. Both mutations — putting back the spread, and putting back some(...) — turn the new test red on their own.

And you are right about why it survived: the test I added last round covered one enormous run and not many small ones, which is the same crash class in the shape I had just stopped looking at.

[xizhuomengcontin]

@orcacode-review please re-review.

Reproduced against 8404ff5 first — 130,000 of the smallest accepted PNG threw RangeError from spansOf, 120,000 finished in 27.7 s — and bcb8da1 fixes both halves. Reply is on the thread.

The spread is now an iteration. For the 27.7 s I did not take the cap: the cost is the per-token spans.some(...), which is quadratic once a value holds many images, and a cap trades one arbitrary failure for another. spansOf returns spans sorted by start and the sweep walks them with one cursor carrying max(b) over spans starting at or before the token — the same predicate, one pass. 130,000 images now take 0.9 s, 200,000 take 1.4 s.

I checked the premise rather than assuming it: the quadratic cost is introduced by this PR. The placeholder spans that predate it cannot produce it, because <secret:…> breaks TOKEN and so many spans never coincide with many tokens — at 40k elements, placeholders 40 ms before / 42 ms after, images 142 ms before / 2683 ms after.

Changing the sweep's containment test is the risky part of this, so it is verified by differential rather than by argument: the predicate against some(...) over 276,433 randomised judgements including overlapping, nested and zero-length spans, and the whole redactString against 8404ff5 over 3,000 randomised bodies mixing images, credentials, placeholders and protocol signatures. Zero disagreements in either.

17 mutations of the guards, all caught. 50 cases. 2103 unit tests pass, 10 fail — the same 10 as main. 14 integration checks 0 failed; conformance, fidelity, neutrality clean.

On why this survived the last round: the test I added there covered one enormous run and not many small ones. Same crash class, in the shape I had just stopped looking at.

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 602 calls · 66.2M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Bound PLTE: up to 768 caller-chosen bytes ride out inside the exempt run

The exemption is granted to the whole base64 run, but the walk's "can the exemption account for these bytes?" bookkeeping is per chunk (length caps, 167-174) while a sweep TOKEN is judged against the whole run. PLTE is the one admitted chunk that is both variable-length and caller-chosen: for colour === 3 it accepts up to 1 << depth entries = 768 uncompressed bytes, and the length is only bounded by the bit depth, never by how many entries the pixels actually reference.

Reproduced end to end through redactString (verified here by running the file under node --experimental-strip-types; the credential is a 32-char base64 string that the sweep redacts on its own — rulesFired() === {high_entropy: 1} — when it is anywhere else in the value):

• palette = 4 filler bytes + Buffer.from(CRED, 'base64') (24 bytes) + 2 filler bytes = 30 bytes, so the credential lands on a 3-byte base64 group boundary 45 = 15×3 inside the file, and the encoded payload text then literally contains CRED (image.includes(CRED) === true);
• PNG = signature + IHDR(1×1, depth 8, colour 3, no interlace) + that PLTE + IDAT(deflate(2 bytes)) + IEND — a valid indexed PNG whose single pixel references palette entry 0, i.e. every byte of the credential sits in entries 1-9, which no pixel refers to and which a decoder renders nothing of;
• new Redactor({}).redactString(eventLine) where eventLine is the writer's exact shape (an event JSON whose payload is a JSON string holding {messages:[{content:[{image_url:{url:'data:image/png;base64,…'}}]}]}) returns the credential verbatim, hits.length === 0, records() empty, and the line still parses. So the credential reaches events.jsonl/the blob store and redactions.json records nothing.

That is the class of hole the change says it closed: the file's own admission criterion is "not that the chunk is harmless, but that exempting it cannot hide anything the sweep was protecting" and it rejects cHRM at 32 fixed bytes as "long enough to matter" — yet admits 768 caller-chosen bytes here. The same mismatch applies to chunk CRCs, which the walk never validates (the test file says so): every admitted chunk carries 4 more caller-chosen bytes, for instance pHYs data 9 + CRC 4 = 13 contiguous bytes, which is only just under the 20-character sweep threshold. Note also that IDAT is exempt as "the pixels", and a credential spelled into a stored-mode deflate block is admittedly not detectable — but the palette is not analogous: its bytes are uncompressed text space in the trace, and they need not be referenced by any pixel.

Fix (P1, so concretely): move PLTE out of the admitted set, exactly as JPEG/GIF/BMP/WebP/cHRM were moved out — reject on type === 'PLTE' like any unaccounted chunk. Holding it to a length that cannot carry a 20-character run means len <= 14, which no real palette satisfies, so the honest alternatives are: (a) drop the exemption and let indexed PNGs be swept, keeping the file's "a format whose bytes cannot be validated does not get an exemption" doctrine; or (b) keep it and write the limitation into spec §5 as a stated one, with REDACTION_POLICY_VERSION bumped so a reader can tell a palette-carrying trace from one recorded under the old policy. While here, either validate the chunk CRCs or exclude the 4 CRC bytes of each admitted chunk from the exempt run, so the per-chunk length argument actually bounds what the run contains.

[xizhuomengcontin]

You are right, and the contradiction is three lines apart in the same file: the criterion says "not that the chunk is harmless, but that exempting it cannot hide anything the sweep was protecting", it refuses cHRM at 32 fixed bytes as long enough to matter, and then admits up to 768 caller-chosen ones. Your PoC reproduces as written — credential verbatim, rulesFired() empty.

1814e91 takes option (a), and then some of (b) anyway.

PLTE is out, and with it colour type 3, since a valid indexed PNG cannot exist without a palette — so this is the same move JPEG, GIF, BMP, WebP and cHRM already got rather than a special case. My "the palette is the pixels" argument does not survive your rebuttal: the entries need not be referenced by any pixel, and unlike IDAT nothing forces them to reconstruct to a declared size. Uncompressed byte space is the distinction that matters and I had it backwards.

The CRCs are checked. You are right that it is a counting argument and not an integrity one — a caller who can set a length can set a CRC — but four unchecked bytes per chunk were four more the caller could fill, and pHYs at 9 + 4 = 13 contiguous chosen bytes against a threshold of 20 was closer than I want any of this to be. It now contributes 9. Cost: 0.25 ms for a 381 KB PNG, against the 55 ms its base64 decode already costs.

REDACTION_POLICY_VERSION → 4. This should have been in the first commit, not this one: the constant's own rule is that it bumps when what reaches the trace changes, and a body recorded under v3 and v4 differs. The quickstart trace keeps policy_version: 3 — it was recorded under 3, and distinguishing that is what the field is for.

Spec §5 now states the exemption instead of leaving it implicit: granted on decoded content, every covered byte accounted for, unbounded caller-chosen chunks (tEXt/iTXt/zTXt/iCCP/PLTE, COM/APPn, formats with no validator) not exempted, a secret written into the pixels not detectable, and policy_version MUST bump when the extent changes.

18 mutations, all caught, including removing either CRC check and putting colour type 3 back. All five real PNGs in the repo still pass with CRC validation on.

[xizhuomengcontin]

@orcacode-review please re-review.

Reproduced as written and fixed in 1814e91. Reply is on the thread. The finding was right about the shape of the mistake too — the contradiction sits three lines apart in the same file, between the stated admission criterion and the branch that admits 768 caller-chosen bytes.

• PLTE is out, and colour type 3 with it. A valid indexed PNG cannot exist without a palette, so this is the same move JPEG, GIF, BMP, WebP and cHRM already got, not a special case. My "the palette is the pixels" argument had it backwards: palette entries need not be referenced by any pixel, and nothing forces them to reconstruct to a declared size the way IDAT does.
• CRCs are checked. A counting argument rather than an integrity one — a caller who can set a length can set a CRC — but four unchecked bytes per chunk were four more they could fill, and pHYs at 9 + 4 = 13 against a threshold of 20 was too close. Now 9. Cost 0.25 ms on a 381 KB PNG.
• REDACTION_POLICY_VERSION → 4, which belonged in the first commit: the constant's own rule is that it bumps when what reaches the trace changes. The quickstart trace stays at 3, because it was recorded under 3.
• Spec §5 now states the exemption and its limitation rather than leaving it to the implementation, including that policy_version MUST bump when the extent changes.

18 mutations of the guards, all caught — including removing either CRC check and putting colour type 3 back. 52 cases. All five real PNGs in the repo still exempt with CRC validation on. 2103 unit tests pass, 10 fail — the same 10 as main. 14 integration checks 0 failed; conformance, fidelity, neutrality clean.

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 856 calls · 106.9M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Do not require Node 22-only zlib.crc32 on a CLI declared to support Node 20

This import was added by the change (line 2, import { crc32, inflateSync } from 'node:zlib'), and crc32 is used at line 262 inside hasValidCrc. Node documents zlib.crc32() as added in v22.2.0; this repository states in writing that the CLI must run on Node 20 — packages/cli/package.json declares "node": ">=20.0.0", packages/cli/src/commands/doctor.ts sets MIN_NODE_MAJOR = 20, and CONTRIBUTING.md §"node versions" says: "the check job runs on node 20 as well as 22, so a 22-only API passes locally and fails there". The neighbouring code already follows that rule: decodeBody looks zstdDecompressSync up dynamically because "zstd was added to Node's built-in zlib API after the oldest runtime Orca supports", and packages/proxy/test/tls-intercept.test.ts:22 records that on Node 20 a named zlib import "was simply undefined and the test below died on zstdCompressSync is not a function — a red build on every pull request".

Consequence on a runtime without crc32 (Node 20.x): the first value containing a base64 PNG reaches hasValidCrc and throws TypeError: crc32 is not a function. Nothing catches it — only the Buffer.from and the inflateSync call are wrapped in try/catch, isWholeRasterImage/isWholePng are not — so it propagates out of redactString, which on the write path (packages/core/src/writer.ts) aborts the recording of any agent that sends a screenshot, and on the read/replay path aborts the match. The same import in packages/core/test/data-uri-redaction.test.ts:5 fails the whole new test file on the Node 20 CI leg.

I could not run Node 20 in this sandbox to confirm the API's absence (only Node 22.23.2 is installed), hence low confidence — the check is one command on a 20.x host: node -p "typeof require('node:zlib').crc32".

Fix: look the function up the way decodeBody does for zstd and fall back to a local table-driven CRC-32 so the module loads and the exemption keeps working on Node 20:

import * as zlib from 'node:zlib';
const zlibCrc32 = (zlib as typeof zlib & { crc32?: (data: Buffer) => number }).crc32;
function crc32(data: Buffer): number {
  if (zlibCrc32) return zlibCrc32(data);
  let c = 0xffffffff;
  for (const byte of data) { c = (c >>> 8) ^ TABLE[(c ^ byte) & 0xff]!; }
  return (c ^ 0xffffffff) >>> 0;
}

(and use the same helper in the test file, so the 20 leg builds the CRCs it asserts on).

[xizhuomengcontin]

Right, and I had the evidence in front of me last round and misread it: I checked engines on the root package (^20.19.0 || >=22.12.0, which is new enough) and did not weigh packages/cli's >=20.0.0 or doctor.ts's MIN_NODE_MAJOR = 20 against it. The floor the CLI promises its users is 20.0, not 20.19.

I could not run a 20.x host either. What settles it without one is the repository's own floor plus the precedent you point at — intercept.ts:188 looks zstdDecompressSync up dynamically with a comment saying zstd landed after the oldest runtime Orca supports, and tls-intercept.test.ts:22 records that the named import was undefined and killed the build. Your point about the throw being uncaught is exact: isWholePng is not inside the try, so it leaves redactString.

Worth adding: CI would not have caught this. actions/setup-node with '20' resolves to a 20.x new enough to have crc32, so the node 20 leg stays green while the shipped CLI breaks for anyone on an earlier 20.

eb5d410 takes the fix, with one deviation: the CRC-32 is computed here unconditionally rather than looked up with a fallback. A fallback branch that only some runtimes take is a branch the tests never run, and I would rather have one implementation exercised on every call than two where the important one is never reached on CI. Verified against zlib.crc32 over 20,000 random buffers and the standard "123456789" vector (0xCBF43926); the test file still builds its CRCs with zlib.crc32, so every "a real PNG stays exempt" case passes only while the two agree. Cost 0.75 ms on a 381 KB PNG against 0.10 ms, beside the 55 ms its base64 decode already costs.

I also audited the rest of the diff for the same mistake rather than fixing only the line you named: Buffer.concat/from, matchAll, charCodeAt, subarray, readUInt32BE, startsWith, sort, and zlib's info/maxOutputLength options all predate Node 20 by several majors. crc32 was the only new API in it.

[xizhuomengcontin]

@orcacode-review please re-review.

Right, and I had the evidence last round and misread it — I checked the root engines (^20.19.0) and did not weigh packages/cli's >=20.0.0 or doctor.ts's MIN_NODE_MAJOR = 20 against it. eb5d410 fixes it.

Two things worth surfacing beyond the finding:

CI would not have caught this. actions/setup-node with '20' resolves to a 20.x new enough to have crc32, so the node 20 leg stays green while the shipped CLI breaks for anyone on an earlier 20. The matrix leg exists for this class of bug and would have missed this instance of it.

I computed the CRC here rather than looking it up with a fallback. A fallback branch that only some runtimes take is a branch the tests never run; I would rather have one implementation exercised on every call. It is verified against zlib.crc32 over 20,000 random buffers and the standard "123456789" vector, and the test file still builds its CRCs with zlib.crc32 — so every "a real PNG stays exempt" case passes only while the two agree.

I audited the rest of the diff for the same mistake rather than fixing only the flagged line: Buffer.concat/from, matchAll, charCodeAt, subarray, readUInt32BE, startsWith, sort, and zlib's info/maxOutputLength options all predate Node 20 by several majors. crc32 was the only new API in the diff.

18 mutations, all caught. 52 cases. 2103 unit tests pass, 10 fail — the same 10 as main. 14 integration checks 0 failed; conformance, fidelity, neutrality clean.

[orcacode-review]

🐳 OrcaCode Review

✅ No findings — nothing to flag in this PR. Great work!

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 814 calls · 106.4M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter