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.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="document/document-container/src/backends/opfs.rs">
<violation number="1" location="document/document-container/src/backends/opfs.rs:68">
P2: Mutex lock poisoning currently causes panics via `lock().unwrap()` across the OPFS backend; persistence operations should degrade gracefully (error/false) instead of crashing.
(Based on your team's feedback about avoiding unwrap()-driven panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="document/document-container/src/backends/folder.rs">
<violation number="1" location="document/document-container/src/backends/folder.rs:56">
P1: `list`/`list_dirs` can follow a symlinked prefix outside the container root because `list_filtered` joins `prefix` directly instead of resolving through the symlink-checking path validator.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| fn list_filtered(&self, prefix: &str, want_files: bool) -> Result<Vec<String>> { | ||
| validate_prefix(prefix)?; | ||
| let base = if prefix.is_empty() || prefix == "." { self.root.clone() } else { self.root.join(prefix) }; |
There was a problem hiding this comment.
P1: list/list_dirs can follow a symlinked prefix outside the container root because list_filtered joins prefix directly instead of resolving through the symlink-checking path validator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/document-container/src/backends/folder.rs, line 56:
<comment>`list`/`list_dirs` can follow a symlinked prefix outside the container root because `list_filtered` joins `prefix` directly instead of resolving through the symlink-checking path validator.</comment>
<file context>
@@ -0,0 +1,179 @@
+
+ fn list_filtered(&self, prefix: &str, want_files: bool) -> Result<Vec<String>> {
+ validate_prefix(prefix)?;
+ let base = if prefix.is_empty() || prefix == "." { self.root.clone() } else { self.root.join(prefix) };
+
+ // A missing prefix has no entries; a prefix that names a file is a misuse.
</file context>
| } | ||
|
|
||
| fn directory(&self) -> FileSystemDirectoryHandle { | ||
| self.inner.lock().unwrap().directory.clone() |
There was a problem hiding this comment.
P2: Mutex lock poisoning currently causes panics via lock().unwrap() across the OPFS backend; persistence operations should degrade gracefully (error/false) instead of crashing.
(Based on your team's feedback about avoiding unwrap()-driven panics in application code.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/document-container/src/backends/opfs.rs, line 68:
<comment>Mutex lock poisoning currently causes panics via `lock().unwrap()` across the OPFS backend; persistence operations should degrade gracefully (error/false) instead of crashing.
(Based on your team's feedback about avoiding unwrap()-driven panics in application code.) </comment>
<file context>
@@ -0,0 +1,456 @@
+ }
+
+ fn directory(&self) -> FileSystemDirectoryHandle {
+ self.inner.lock().unwrap().directory.clone()
+ }
+
</file context>
Preparatory work for the new document format.
Adds the storage layer as a standalone crate