Repository navigation
fix(model): count GGUF nesting frames instead of total value depth - #10497
LexLuthorPrimero wants to merge 1 commit into
Conversation
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Low impact] The PR's reach is limited to deeply nested metadata during GGUF model inspection. The implementation appears safe to merge, with a non-blocking gap in regression coverage. Findings
Fix with agent prompt### Issue 1
tests/boundary/model/modelArtifact.test.ts:406-414
The new 32-array scalar test passes with the old guard too: its scalar reaches depth 32, while the old guard rejected only depths above 32. The existing 33-array scalar test also passes with the old guard because its scalar reaches depth 33.
Add a 33-frame case whose innermost array is empty, and assert the `metadata-nesting` capacity failure. That covers the changed behavior; reverting this fix currently leaves both nesting tests passing.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Summary
No security defect was identified. Review was source-based; tests were not run. Whether this PR is superseded could not be checked because GitHub access required authentication. Reviews (1) · Last reviewed commit: "fix(model): count GGUF nesting frames in..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Summary
Fixes #7312.
The GGUF metadata nesting cap (
MODEL_METADATA_NESTING= 32) was documented as "array frames" but enforced as depth of every recursive value, counting the depth of scalar leaves (and any non-array object) toward the cap. That meant an advertised-contract combination — 33 nested arrays ending with a scalar — tripped the cap, while a 33-nested but empty-final-array case slipped through. Test expectations were inconsistent with both contracts.Change and scope
src/model/GgufHeader.ts: the capacity throw fires only on nested array (type === 9) values atdepth >= MODEL_METADATA_NESTING— i.e., 32 ARRAY frames in, the 33rd depth-32 frame is rejected; scalar leaves at depth 32 parse normally; non-array terminals at any legal depth are not refused. The IEC Decoder capacity ("metadata-nesting") reference stays32 array frames; from the inside out, no nesting at which a scalar leaf is reachable is bounded.custom.nestedfixture (32 nested arrays with a UINT8 scalar leaf at maximum depth) is added, exercising the documented contract exactly: 32-array controls parse, deeper arrays and scalar-bearing ones do not.Evidence
npx vitest run src/model tests/boundary/model— 146 passed (incl. the new bounded-frame fixture).npm run typecheck,npm run format:check,npm run lint— passed.Known gaps / honest limitations
custom.nestedbuffers of ≤32 frames, withframe 0 → 0x00(UINT8) as the outer-optional grammar convention matching the existingnesting 33 framesfixture. Not an across-the-board GGUF conformance claim.Compatibility
Breaking changes: none for compliant artifacts — artifacts that previously parsed still parse; artifacts that previously hit the capacity failure at the scalar leaf of depth 32 (but were not actually exceeding 32 array frames) now parse.
Author readiness
Human owner: @LexLuthorPrimero