Add document-container crate#4191
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the document-container crate, which provides a virtual filesystem abstraction (Container and AsyncContainer traits) for the .gdd document format. It implements three storage backends (in-memory, loose-folder using memory-mapped files, and OPFS for browser WASM) along with streaming archive codecs for Zip and Xz formats. The review feedback highlights several critical improvement opportunities: addressing potential u64 to usize truncation bugs on 32-bit targets during Zip and Xz deserialization, ensuring proper stream aborts in the OPFS backend when synchronous JS exceptions are thrown, explicitly rejecting empty paths in validate_path, and bypassing memory-mapping for empty files in the folder backend to avoid platform-dependent issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
3 issues found
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported issues are low severity (3–4/10) and mostly affect logging noise, API consistency, and documentation accuracy rather than core functionality.
- Most impactful item is in
document/document-container/src/backends/folder.rs, wherelog::warn!appears too noisy on the expected huge-pages fallback path and could create unnecessary operational alert fatigue. document/document-container/src/backends/memory.rshas a behavior consistency gap (exists()not callingvalidate_path(path)), which can lead to inconsistent invalid-path handling versus peer methods andFolderBackend::exists().- Pay close attention to
document/document-container/src/backends/folder.rs,document/document-container/src/backends/memory.rs, anddocument/document-container/src/lib.rs- reduce fallback log noise, align path validation behavior, and correct thePath::componentsdoc claim.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
6 issues found across 13 files
Confidence score: 2/5
- There is a high-confidence, user-impacting bug in
document/document-container/src/lib.rs:MmappedBytes::as_ref()can turn mmap failures into&[], which risks silent document data loss instead of surfacing an error. document/document-container/src/lib.rsalso appears to regress path normalization ("."becoming"./"), which can break root listing behavior for backends likeMemoryBackendand indicates functional risk beyond edge-case cleanup.- State/format handling issues add additional uncertainty: optimistic
on_diskupdates indocument/document-container/src/backends/opfs.rscan report stale existence after worker failures, and archive boundary/filename handling indocument/document-container/src/archive/xz.rsanddocument/document-container/src/archive/zip.rsmay reject valid max-size archives or mutate non-UTF8 paths. - Pay close attention to
document/document-container/src/lib.rs,document/document-container/src/backends/opfs.rs,document/document-container/src/archive/xz.rs, anddocument/document-container/src/archive/zip.rs- they contain behavior that can affect correctness, listing semantics, and archive read reliability.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 2/5
- There is a high-risk safety concern in
document/document-container/src/backends/opfs.rs: a manualunsafe impl Send/Syncon a JS-handle-backed type can break thread-affinity assumptions and lead to unsound cross-thread access. document/document-container/src/lib.rshas a concrete behavior risk whereopen_external()skipscheck_readable(), allowing mmap read failures to surface later as empty bytes viaAsRef<[u8]>instead of returning an expected container error.- Given the high severity and strong confidence of both findings (memory/thread-safety plus user-visible I/O error handling), this is likely not safe to merge until addressed.
- Pay close attention to
document/document-container/src/backends/opfs.rs,document/document-container/src/lib.rs- thread-safety unsoundness and masked mmap read failures could cause hard-to-debug regressions.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 13 files
Confidence score: 3/5
- There is a meaningful regression/security risk in
document/document-container/src/backends/folder.rs:root.join(path)is symlink-unsafe, so symlinks under the root may allow access outside the container root. document/document-container/src/backends/opfs.rshas concrete behavior mismatches inremove()(can succeed on missing paths or remove empty directories), which can break expected file-only/not-found semantics for callers.- Additional backend consistency issues in
document/document-container/src/backends/opfs.rsanddocument/document-container/src/backends/memory.rs(list/list_dirserroring on file-prefix type mismatch vs empty list, and missingvalidate_prefix) increase cross-backend regression risk, so this is not a low-risk merge. - Pay close attention to
document/document-container/src/backends/folder.rs,document/document-container/src/backends/opfs.rs,document/document-container/src/backends/memory.rs- path safety and backend contract consistency need validation before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 2/5
- There is a high-confidence, user-impacting path safety risk in
document/document-container/src/backends/folder.rs:list/list_dirsmay follow a symlinkedprefixoutside the container root becauselist_filteredjoinsprefixdirectly instead of using the symlink-validating path resolver. document/document-container/src/backends/opfs.rsuseslock().unwrap()broadly, so a poisoned mutex can panic the backend during persistence flows rather than returning a recoverable error/false outcome.- Given one severe containment/security issue (7/10) plus a stability concern, merge risk is elevated until these are addressed.
- Pay close attention to
document/document-container/src/backends/folder.rsanddocument/document-container/src/backends/opfs.rs- root-escape via symlinked prefixes and panic-on-poisoned-lock behavior.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 13 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported issues are low severity (3–4/10) and mostly affect logging clarity, dependency footprint, and maintainability rather than core functionality.
- The highest-impact item is in
document/document-container/src/backends/opfs.rs, where background delete may logNotFoundas an error; this can create noisy/misleading error signals even though missing-file deletes are effectively idempotent. - In
document/document-container/Cargo.toml, leavingtardefault features enabled may pull in unnecessaryxattrsupport, and indocument/document-container/src/archive/xz.rsduplicated size-limit/usizeconversion logic raises mild policy-drift risk over time. - Pay close attention to
document/document-container/src/backends/opfs.rs,document/document-container/Cargo.toml,document/document-container/src/archive/xz.rs- adjust log semantics for idempotent deletes, trim optional dependency features, and keep deserializer limit logic consistent.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 13 files
Confidence score: 3/5
- There is a concrete runtime risk in
document/document-container/src/backends/memory.rs:MemoryBackenduseslock().unwrap(), so poisoned mutexes can panic in library code instead of returning recoverable backend errors. - The
list_filtered()behavior indocument/document-container/src/backends/folder.rscan expose symlink entries that later fail inresolve(), which may cause confusing user-facing inconsistencies between listing and access. - The remaining findings in
document/document-container/src/lib.rsare documentation mismatches (path normalization and leading-dot wording), which are low severity but worth fixing to avoid incorrect expectations. - Pay close attention to
document/document-container/src/backends/memory.rs,document/document-container/src/backends/folder.rs- panic handling and listing/access consistency are the main merge-risk areas.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 13 files
Confidence score: 2/5
- There is a high-confidence data integrity risk in
document/document-container/src/backends/opfs.rs: concurrent awaitedappend()operations can race on read-size/seek/write and overwrite each other’s data. document/document-container/src/backends/opfs.rsalso has state-consistency risk: failed background write/delete operations do not roll backon_disk, soexists_non_blocking()can return incorrect positives for the rest of the session.- Additional medium-risk behavior differences in
document/document-container/src/lib.rs(validate_pathaccepting trailing/redundant separators) and error-classification loss indocument/document-container/src/archive/zip.rs(zip_errflatteningZipError::Io) increase regression/debugging risk. - Pay close attention to
document/document-container/src/backends/opfs.rs,document/document-container/src/lib.rs,document/document-container/src/archive/zip.rs- concurrent append safety, staleon_disktracking, path normalization consistency, and preserving structured I/O errors.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
…es, shared entry-size cap
cbb6c63 to
e4b62a6
Compare
Preparatory work for the new document format.
Adds the storage layer as a standalone crate