Skip to content

Add document-container crate#4191

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

Add document-container crate#4191
TrueDoctor wants to merge 16 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/container/src/lib.rs
Comment thread 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/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/container/src/lib.rs
Comment thread document/document-container/src/archive/zip.rs Outdated
Comment thread 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/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
Comment thread document/container/src/backends/opfs.rs
Comment thread document/document-container/src/backends/memory.rs Outdated
Comment thread 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.

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/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.

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 log NotFound as an error; this can create noisy/misleading error signals even though missing-file deletes are effectively idempotent.
  • In document/document-container/Cargo.toml, leaving tar default features enabled may pull in unnecessary xattr support, and in document/document-container/src/archive/xz.rs duplicated size-limit/usize conversion 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

Comment thread document/document-container/src/backends/opfs.rs Outdated
Comment thread document/document-container/Cargo.toml Outdated
Comment thread document/document-container/src/archive/xz.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.

4 issues found across 13 files

Confidence score: 3/5

  • There is a concrete runtime risk in document/document-container/src/backends/memory.rs: MemoryBackend uses lock().unwrap(), so poisoned mutexes can panic in library code instead of returning recoverable backend errors.
  • The list_filtered() behavior in document/document-container/src/backends/folder.rs can expose symlink entries that later fail in resolve(), which may cause confusing user-facing inconsistencies between listing and access.
  • The remaining findings in document/document-container/src/lib.rs are 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

Comment thread document/document-container/src/backends/folder.rs Outdated
Comment thread document/container/src/backends/memory.rs
Comment thread document/document-container/src/lib.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.

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 awaited append() operations can race on read-size/seek/write and overwrite each other’s data.
  • document/document-container/src/backends/opfs.rs also has state-consistency risk: failed background write/delete operations do not roll back on_disk, so exists_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_path accepting trailing/redundant separators) and error-classification loss in document/document-container/src/archive/zip.rs (zip_err flattening ZipError::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, stale on_disk tracking, path normalization consistency, and preserving structured I/O errors.

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

Re-trigger cubic

Comment thread document/container/src/backends/opfs.rs
Comment thread document/container/src/archive/zip.rs
Comment thread document/container/src/backends/opfs.rs
Comment thread document/container/src/lib.rs
@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Jun 3, 2026

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 @cubic-dev-ai review.

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