fix: scan model links in oci layers - #1567
Conversation
Performance BenchmarksCompared
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0c6bef9b8
ℹ️ 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".
|
The implementation and focused QA are clean: The only remaining blocker is a one-line
Local resolved merge tree: @codex please sync current |
|
Summary
Testing
|
|
Integrated current The critical audit also found quadratic link-chain resolution. Resolutions are now path-compressed through a per-layer cache; a 256-link regression verifies resolving every alias performs exactly 256 target-resolution steps rather than O(n^2) work. Focused QA: all 143 OCI scanner tests pass, with scoped Ruff, format, mypy, and |
|
@codex Please sync the latest Exact local integrated head: Critical review of the current OCI link resolver found no remaining same-layer false-positive/false-negative defect: target resolution is confined to admitted metadata-valid members, duplicate normalized paths stay ambiguous, absolute symlinks use OCI root semantics, payload copies are charged to per-file/per-layer/manifest budgets, and link-chain resolution is path-compressed. The documented cross-layer limitation still fails closed. Focused validation after merging latest main: |
|
Summary
Testing
|
|
Random critical review completed and current False-positive / false-negative audit found no remaining same-layer defect: link resolution is restricted to admitted metadata-valid targets, duplicate normalized targets remain ambiguous, absolute symlinks use OCI root semantics, payload copies are charged to per-file/layer/manifest budgets, equivalent aliases are deduplicated by payload plus routing suffix, and unresolved cross-layer targets still fail closed. Focused QA:
All review threads were already resolved. Full pytest remains delegated to CI as requested. |
4e5de68 to
2a03e88
Compare
|
Critical follow-up found and fixed two remaining link-resolution defects. First, the path-compression cache was keyed by normalized link name, so duplicate link headers with the same path but different targets could make the second reuse the first payload. It is now keyed by the actual |
|
Critical follow-up review found an algorithmic denial-of-service path in component-directory symlink traversal: N directory links combined with N distinct model aliases could require O(N²) resolver work (for example, 200 + 200 entries produced 40,200 path-resolution calls before the fix). Fixed in Associated QA: all 149 OCI scanner tests pass; scoped Ruff, format, mypy, and diff checks are clean. The final published tree is |
Summary
False-positive / false-negative audit
max_file_sizeagainst target payloads rather than zero-size link headerspreflight_member_limit/max_oci_layer_entriesmodels -> payloadsplusweights.onnx -> models/data.binValidation
149 passedgit diff --check origin/main...HEAD69079ef35c8a66d8fa38af71bfd447db1bb24466Known limitation
Cross-layer symlink targets require an overlay-aware index with OCI whiteout semantics. Until that broader support exists, unresolved cross-layer links fail closed as incomplete coverage.
All inline review threads are resolved. Full pytest remains delegated to CI. Final reviewed head:
5eb923c3ac440dd35b8e6a3673aefe76317298c9, based on mainc55de216637b0ca341cd6659d915d1cc2c4f5e2c.