Skip to content

Add document-container crate#4191

Open
TrueDoctor wants to merge 5 commits into
masterfrom
document-container
Open

Add document-container crate#4191
TrueDoctor wants to merge 5 commits into
masterfrom
document-container

Conversation

@TrueDoctor
Copy link
Copy Markdown
Member

Preparatory work for the new document format.
Adds the storage layer as a standalone crate

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread document/document-container/src/archive/xz.rs Outdated
Comment thread document/document-container/src/archive/zip.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs Outdated
Comment thread document/document-container/src/lib.rs
Comment thread document/document-container/src/backends/folder.rs
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, where log::warn! appears too noisy on the expected huge-pages fallback path and could create unnecessary operational alert fatigue.
  • document/document-container/src/backends/memory.rs has a behavior consistency gap (exists() not calling validate_path(path)), which can lead to inconsistent invalid-path handling versus peer methods and FolderBackend::exists().
  • Pay close attention to document/document-container/src/backends/folder.rs, document/document-container/src/backends/memory.rs, and document/document-container/src/lib.rs - reduce fallback log noise, align path validation behavior, and correct the Path::components doc claim.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread document/document-container/src/backends/folder.rs Outdated
Comment thread document/document-container/src/backends/memory.rs
Comment thread document/document-container/src/lib.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs also appears to regress path normalization ("." becoming "./"), which can break root listing behavior for backends like MemoryBackend and indicates functional risk beyond edge-case cleanup.
  • State/format handling issues add additional uncertainty: optimistic on_disk updates in document/document-container/src/backends/opfs.rs can report stale existence after worker failures, and archive boundary/filename handling in document/document-container/src/archive/xz.rs and document/document-container/src/archive/zip.rs may 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, and document/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

Comment thread document/document-container/src/lib.rs
Comment thread document/document-container/src/archive/zip.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs
Comment thread document/document-container/src/archive/xz.rs Outdated
Comment thread document/document-container/src/archive/xz.rs Outdated
Comment thread document/document-container/src/lib.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 manual unsafe impl Send/Sync on a JS-handle-backed type can break thread-affinity assumptions and lead to unsound cross-thread access.
  • document/document-container/src/lib.rs has a concrete behavior risk where open_external() skips check_readable(), allowing mmap read failures to surface later as empty bytes via AsRef<[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

Comment thread document/document-container/src/lib.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs has concrete behavior mismatches in remove() (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.rs and document/document-container/src/backends/memory.rs (list/list_dirs erroring on file-prefix type mismatch vs empty list, and missing validate_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

Comment thread document/document-container/src/backends/folder.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs
Comment thread document/document-container/src/backends/memory.rs Outdated
Comment thread document/document-container/src/backends/opfs.rs
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_dirs may follow a symlinked prefix outside the container root because list_filtered joins prefix directly instead of using the symlink-validating path resolver.
  • document/document-container/src/backends/opfs.rs uses lock().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.rs and document/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) };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant