Harden legacy fragment decoding (zlib-wrapped deflate + arx dictionary drift) - #72
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying agent-render with
|
| Latest commit: |
9697483
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://73bd4770.agent-render.pages.dev |
| Branch Preview URL: | https://codex-codec-backcompat.agent-render.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a65f0e34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resolved = resolveEnvelope(parsed, header.fragmentLength); | ||
| if (!resolved.ok && resolved.code === "invalid-envelope" && (codec === "arx" || codec === "arx2" || codec === "arx3")) { | ||
| // An ARX payload that decoded but is not a valid envelope almost always means the active | ||
| // dictionary differs from the one it was encoded with (see tests/arx-dictionary-pin.test.ts). | ||
| return { ...resolved, message: `${resolved.message} It may have been encoded with a different ARX dictionary version.` }; |
There was a problem hiding this comment.
When a versioned legacy ARX fragment carries a dictionary version that differs from the active dictionary, this new logic only adds the hint after decode/validation has already failed. decodeArxFragmentPayload parses the numeric prefix but never checks it against the loaded dictionary version, so after a future dictionary bump an older fragment that still decompresses into a syntactically valid envelope can still be accepted and silently show garbled artifact content instead of being rejected as a version mismatch.
Useful? React with 👍 / 👎.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Reviewed the incremental commit Incremental changes verified
Notes
Remaining (non-blocking, carried forward)The prior suggestion at Files Reviewed (4 files)
Previous Review Summaries (2 snapshots, latest commit 7e589e5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7e589e5)Status: No New Issues Found | Recommendation: Merge The branch history was rewritten (force push) since the prior review, so this is a full re-review of all changed files. The new commit Incremental changes verified
Remaining (non-blocking, already commented inline)One prior suggestion remains open at Notes
Files Reviewed (3 files)
Previous review (commit 70a65f0)Status: No Issues Found | Recommendation: Merge This PR is a decode-only, wire-safe back-compat fix with two independent changes plus tests. The encoder is unchanged (still raw deflate), so no existing links are affected and no new attack surface is introduced — decoded output still flows through JSON.parse, the decoded-size limit, and envelope validation. Key checks:
Files Reviewed (3 files)
Reviewed by GLM-5.2 · Input: 182.8K · Output: 18.7K · Cached: 330.1K |
|
| Filename | Overview |
|---|---|
| src/lib/payload/fragment-arx.ts | Adds EXPECTED_ARX_DICTIONARY_VERSION / EXPECTED_ARX2_OVERLAY_VERSION constants and assertion helpers; restructures ensureArxDictionaryLoaded/ensureArx2DictionariesLoaded to (a) clear the cached promise on -1/fallback responses to enable retry, and (b) always run the version guard regardless of whether the dictionary was fetched or sync-injected. |
| src/lib/payload/fragment.ts | Adds inflateDeflatePayload with raw-then-zlib fallback for legacy zlib-wrapped deflate links; augments decodeFragmentAsync error handling to append a dictionary-version hint for arx codecs — but when the version guard throws, the original descriptive error is replaced with the generic hint text. |
| tests/arx-codec.test.ts | Updates the dictionary-version skew test to assert hard-fail (ok: false) when active dictionary is newer than build supports, replacing the previous expectation that decoding would succeed with any version. |
| tests/arx-dict-load-retry.test.ts | New test: verifies that a -1 resolve from loadArxDictionary clears the cached promise so subsequent decodes retry the fetch instead of reusing a poisoned no-op promise. |
| tests/arx-dict-version-guard.test.ts | New test: exercises the forward-incompatibility guard end-to-end — encode with v1, inject v2 dict, confirm hard-fail, restore v1, confirm recovery; also covers the arx2 overlay dictionary guard. |
| tests/arx-dictionary-pin.test.ts | New CI pin test: reads and re-serializes each dictionary file, hashes the canonical JSON, and asserts both version and content hash match the pinned values; includes a note that arx3 shares arx-dictionary.json. |
| tests/fragment.test.ts | Adds a round-trip test that manually constructs a zlib-wrapped deflate fragment (the legacy encoder format) and verifies it decodes cleanly through inflateDeflatePayload's fallback path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[decodeFragmentAsync] --> B{codec arx/arx2/arx3?}
B -- no --> C[decodePayload deflate path]
C --> D{inflateDeflatePayload}
D --> E[inflateSync raw deflate]
E -- success --> F[strFromU8 → JSON.parse]
E -- throws --> G[unzlibSync legacy zlib fallback]
G -- success --> F
G -- throws --> H[re-throw original raw-inflate error]
B -- yes --> I[decodeArxFragmentPayload]
I --> J[ensureArxDictionariesLoaded]
J --> K{isExternalDictionaryLoaded?}
K -- no --> L[loadArxDictionary resolves version or -1]
L -- version lt 0 --> M[clear cached promise fallback dict active]
L -- version ≥ 0 --> N[keep cached promise]
M --> O[assertVersion ≤ EXPECTED]
N --> O
K -- yes --> O
O -- version gt EXPECTED --> P[throw forward-incompat error]
O -- version OK --> Q[arxDecompress / arx2 / arx3]
P --> R[caught → ok:false code: invalid-json + hint]
Q -- success --> S[resolveEnvelope]
S -- invalid-envelope + arx --> T[append dict-version hint to message]
S -- ok --> U[return ok:true envelope]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[decodeFragmentAsync] --> B{codec arx/arx2/arx3?}
B -- no --> C[decodePayload deflate path]
C --> D{inflateDeflatePayload}
D --> E[inflateSync raw deflate]
E -- success --> F[strFromU8 → JSON.parse]
E -- throws --> G[unzlibSync legacy zlib fallback]
G -- success --> F
G -- throws --> H[re-throw original raw-inflate error]
B -- yes --> I[decodeArxFragmentPayload]
I --> J[ensureArxDictionariesLoaded]
J --> K{isExternalDictionaryLoaded?}
K -- no --> L[loadArxDictionary resolves version or -1]
L -- version lt 0 --> M[clear cached promise fallback dict active]
L -- version ≥ 0 --> N[keep cached promise]
M --> O[assertVersion ≤ EXPECTED]
N --> O
K -- yes --> O
O -- version gt EXPECTED --> P[throw forward-incompat error]
O -- version OK --> Q[arxDecompress / arx2 / arx3]
P --> R[caught → ok:false code: invalid-json + hint]
Q -- success --> S[resolveEnvelope]
S -- invalid-envelope + arx --> T[append dict-version hint to message]
S -- ok --> U[return ok:true envelope]
Reviews (3): Last reviewed commit: "Harden arx dictionary loading (adversari..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
An older encoder version emitted zlib-wrapped deflate, but the current decoder uses fflate's raw `inflateSync`, so those historical shared links fail with `invalid-json`. Verified against a real sha256-checked link (`#agent-render=v1.deflate.eNqN...`, a "cline/kanban analysis" envelope) that the current decoder rejects but is a complete, valid zlib stream. Decode now tries raw inflate first (so current output is never at risk of header mis-detection) and falls back to `unzlibSync` for the zlib-wrapped legacy form. Encoding is unchanged (still raw deflate); this is decode-only and wire-safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Real sha256-verified arx/arx2 links (dictVer=1) decode to garbage: brotli and the dictionary substitution run, but produce a non-envelope object. The `public/arx-dictionary.json` content drifted from what those fragments were encoded with while its `version` field stayed 1, so old links silently mis-decode — a back-compat hazard with no signal. - tests/arx-dictionary-pin.test.ts pins each dictionary's `version` to its canonical content hash. Changing slots now fails CI unless the version is bumped in the same commit, so content and version can never drift apart silently again. - decodeFragmentAsync now appends a dictionary-version hint to the error when an arx payload fails to decode or isn't a valid envelope, instead of a generic invalid-json message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reuse - inflateDeflatePayload: when the zlib-wrapped fallback also fails, re-throw the original raw-inflate error instead of a misleading zlib error, so genuinely corrupt deflate surfaces an accurate failure (greptile). - arx-dictionary-pin test: document that arx3 reuses arx-dictionary.json, so the pin also covers arx3 and a future arx3-specific dictionary needs its own entry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
70a65f0 to
7e589e5
Compare
Addresses two findings from an adversarial review of the stack: - Transient dictionary-load failures were cached for the page lifetime. loadArxDictionary() resolves -1 (it falls back to the built-in dictionary instead of rejecting), so the ensure helpers treated a failed external fetch as "loaded" and never retried. Now a -1 result clears the cached promise: the current call still proceeds on the built-in fallback, but a later call retries the external dictionary once the endpoint recovers. - Compact arx links carry no dictionary version (the tag implies the current dictionary), so decoding against a skewed dictionary (CDN/asset split or a future version bump) could silently produce a structurally-valid-but-wrong envelope. Decode now rejects any active dictionary NEWER than the build supports; the built-in (v0) and current (v1) dictionaries remain usable. A dictionary version bump is documented as a wire change that also needs new compact tags. Tests: load-retry (transient -1 then recovery), version-skew guard for base and overlay dictionaries, and the prior version-difference test updated to assert the new hard-fail behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Stacked on #71. Two independent back-compat fixes surfaced by decoding a corpus of real, sha256-verified agent-render links (not synthetic): of 12 verbatim legacy fragments, 5 failed to decode — 1 from a real bug fixed here, 3 from dictionary drift addressed here, and 1 from source-side truncation (not a code issue). Each fix is its own commit.
1. Decode legacy zlib-wrapped deflate fragments
An older encoder version emitted zlib-wrapped deflate (
78 da …), but the current decoder uses fflate's rawinflateSync, so those links fail withinvalid-json. One real verified link (a "cline/kanban analysis" envelope) is a complete, valid zlib stream that the current build rejects.Decode now tries raw inflate first (current output is never at risk of header mis-detection) and falls back to
unzlibSyncfor the zlib-wrapped legacy form. Encoding is unchanged (still raw deflate) — decode-only, wire-safe.2. Pin arx dictionary content to its version; flag mismatch on decode
Three real
arx/arx2links (dictVer=1) decode to garbage: brotli and the dictionary substitution run, butpublic/arx-dictionary.jsoncontent drifted from what those fragments were encoded with while itsversionstayed1. Old links silently mis-decode with no signal.tests/arx-dictionary-pin.test.tspins each dictionary'sversionto its canonical content hash. Changing slots now fails CI unless the version is bumped in the same commit, so content and version can't drift apart silently again.decodeFragmentAsyncnow appends a dictionary-version hint to the error when an arx payload fails to decode or isn't a valid envelope, instead of a generic message. (Already-encoded drifted links can't be recovered without the original dictionary bytes; this makes the failure legible and stops future drift.)Test plan
npm run checkgreen — 227 tests, typecheck, build, budgets.Note (not in this PR)
The same real corpus retired the trained-compression-dictionary idea: the 112 KB dict that gave +10.5% on the synthetic held-out set gives −0.74% on these real payloads — it was overfit to the generator. Combined with the drift hazard above, a trained compression dictionary is not worth shipping. Findings recorded in memory.
🤖 Generated with Claude Code