Conversation
Reject absolute and root-escaping shard paths, fail closed on missing referenced shards, and report duplicate tensor ownership with typed diagnostics. Keep unreferenced shards in reserved metadata and sort directory/tensor records so repeated inspection is byte-identical. RM-1360 Co-authored-by: Raul Cardenas Montoya <montoyaraul34@gmail.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_004e7115-23a7-4a4f-891a-dea86abcb991) |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully hardens Safetensors shard-index resolution with comprehensive security improvements and deterministic manifest generation. The changes effectively address path traversal vulnerabilities through lexical and canonical validation, enforce tensor ownership integrity, and ensure deterministic behavior through sorted operations.
Security Enhancements Verified:
- Path traversal protection correctly rejects absolute paths and parent-directory escapes via lexical analysis before file access
- Symlink escape prevention through canonicalization on platforms that support it, with documented lexical fallback
- Early failure on missing referenced shards with typed error diagnostics
- Duplicate tensor ownership detection prevents conflicting shard claims
- Deterministic directory listing and tensor record ordering eliminates non-deterministic manifest generation
Test Coverage:
The PR includes comprehensive test coverage for all security-critical paths including absolute path rejection, parent directory escape, symlink escape (platform-specific), duplicate ownership detection, missing shard handling, and deterministic ordering. The implementation is production-ready.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Codex Review SummaryThis 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. |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| /// A tensor name is claimed by more than one Safetensors shard. | ||
| DuplicateTensorOwnership { | ||
| /// Tensor name with conflicting owners. | ||
| name: String, | ||
| /// Checkpoint-relative shard paths that declare the tensor, sorted. | ||
| shards: Vec<String>, | ||
| /// Path of the checkpoint or index being inspected. | ||
| path: String, | ||
| }, | ||
| /// An index-referenced Safetensors shard is missing on disk. | ||
| MissingShard { | ||
| /// Shard path as declared in the index (checkpoint-relative). | ||
| shard: String, | ||
| /// Path of the index or checkpoint that referenced the shard. | ||
| path: String, | ||
| }, |
There was a problem hiding this comment.
Suggestion: Adding variants to this public enum breaks downstream users that exhaustively match ParserError, contradicting the documented matching contract. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/error.rs
**Line:** 55:70
**Comment:**
*Api Mismatch: Adding variants to this public enum breaks downstream users that exhaustively match `ParserError`, contradicting the documented matching contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
CodeAnt Nitpicks1 code suggestion1. After this metadata check succeeds, a deleted shard makes
|
2 participants
User description
Fixes RM-1360.
Harden the feature-gated Safetensors shard-index path so a manifest is only returned when shard references stay inside the checkpoint root, every indexed tensor maps to exactly one existing shard, and repeated inspection is byte-identical.
Changes
weight_mapshard paths relative to the checkpoint root; reject absolute paths and..traversal that escapes the root; allow in-rootnested/../filenormalization.ParserError::MissingShardbefore a manifest is returned.ParserError::DuplicateTensorOwnership(tensor name + sorted shard list).index:unreferenced_shards), not a hard error.Tests
..normalizationLocal evidence
All passed.
GitHub CI
All required checks on
cb569b6passed:https://github.com/rmems/engram-parser/actions/runs/34931643984
Linear Issue: RM-1360
Summary by cubic
Fixes RM-1360 by hardening the
safetensorsshard-index path so a manifest is only returned when shard references stay inside the checkpoint root, every indexed tensor maps to exactly one existing shard, and repeated inspection is byte-identical. Previously, missing referenced shards and duplicate tensor ownership could surface as generic load errors or invalid manifests.Behavior
weight_mapshard paths relative to the checkpoint root; in-rootnested/../filepaths normalize, while absolute paths and parent-dir escapes are rejected.ParserError::MissingShardwhen an indexed shard is absent andParserError::DuplicateTensorOwnershipwhen multiple shards declare the same tensor.index:unreferenced_shardsmetadata instead of failing.Validation
..escapes, in-root normalization, unix symlink escape, missing shards, duplicate ownership, shuffled creation order, and byte-stable fixture output.Written for commit cb569b6. Summary will update on new commits.
CodeAnt-AI Description
Harden Safetensors shard validation and produce deterministic manifests
What Changed
nested/../model.safetensorswhen they remain inside the checkpointImpact
✅ Safer checkpoint shard loading✅ Clearer missing-shard and duplicate-tensor errors✅ Stable manifest output across repeated inspections💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.