[EPIC-3-1] Vault Directory Structure and Markdown Utilities (Rust) - #21
Conversation
github-actions
Bot
added
service/sync-service
Summary of ChangesHello @deserat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational Rust-based utilities for the File Vault System (EPIC-3), enabling robust and compatible markdown-based knowledge storage. It introduces core modules for managing vault files, including templating, parsing, writing, hashing, and locking, ensuring data integrity and seamless integration with external markdown editors. The changes lay the groundwork for a reliable and extensible knowledge vault that can be interacted with programmatically and through third-party applications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of Rust utilities for managing a markdown-based vault, which is a crucial foundation for the File Vault System. The changes are well-structured into modules for templating, parsing, writing, hashing, and file locking. The code is generally of high quality, with thorough documentation and extensive unit tests.
My review focuses on improving robustness, error handling, and ensuring the code is safe for use in an asynchronous environment. Key suggestions include:
- Replacing
unwrap()andexpect()calls with properResultpropagation to prevent panics in library code. - Handling file system errors instead of ignoring them.
- Addressing the use of blocking operations within an async context.
- Enhancing validation logic for better data integrity.
Overall, this is an excellent contribution that lays a solid groundwork. The suggested changes will make the new vault utilities even more reliable and resilient.
| pub fn new(file_path: &Path) -> Self { | ||
| let lock_dir = file_path.parent().unwrap().join(".noosphere/locks"); | ||
| fs::create_dir_all(&lock_dir).ok(); | ||
|
|
||
| let lock_file = lock_dir.join(format!( | ||
| "{}.lock", | ||
| file_path.file_name().unwrap().to_string_lossy() | ||
| )); | ||
|
|
||
| Self { | ||
| lock_file, | ||
| _guard: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
The new function uses unwrap() and .ok(), which can either panic or silently ignore errors.
file_path.parent().unwrap()andfile_path.file_name().unwrap()will panic if the path is a root directory or ends in...fs::create_dir_all(&lock_dir).ok()will silently ignore errors if the lock directory cannot be created (e.g., due to permissions), leading to failures later.
For a robust library function, it's better to return a Result and propagate all errors to the caller.
pub fn new(file_path: &Path) -> Result<Self> {
let parent_dir = file_path.parent().ok_or_else(|| {
anyhow::anyhow!("File path '{}' has no parent directory", file_path.display())
})?;
let lock_dir = parent_dir.join(".noosphere/locks");
fs::create_dir_all(&lock_dir)?;
let file_name = file_path.file_name().ok_or_else(|| {
anyhow::anyhow!("File path '{}' has no file name", file_path.display())
})?;
let lock_file = lock_dir.join(format!(
"{}.lock",
file_name.to_string_lossy()
));
Ok(Self {
lock_file,
_guard: None,
})
}| pub fn acquire(&mut self, timeout: Duration) -> Result<()> { | ||
| let start = SystemTime::now(); | ||
|
|
||
| loop { | ||
| match OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .truncate(true) | ||
| .open(&self.lock_file) | ||
| { | ||
| Ok(file) => { | ||
| // Acquire exclusive lock using fs2 | ||
| file.lock_exclusive()?; | ||
| self._guard = Some(file); | ||
| return Ok(()); | ||
| } | ||
| Err(_e) => { | ||
| if start.elapsed()? > timeout { | ||
| anyhow::bail!("Lock timeout: failed to acquire lock within {:?}", timeout); | ||
| } | ||
| std::thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The acquire function is synchronous and contains blocking I/O operations (OpenOptions::open, file.lock_exclusive()) and a blocking sleep (std::thread::sleep). When used within the sync-service async daemon, calling this function directly from an async task will block the worker thread, which can severely impact concurrency and responsiveness.
It's highly recommended to wrap calls to this function in tokio::task::spawn_blocking to offload the blocking work to a dedicated thread pool for blocking tasks.
Example usage from an async context:
let mut lock = FileLock::new(path)?; // Assuming new() returns Result
tokio::task::spawn_blocking(move || {
lock.acquire(Duration::from_secs(5))
}).await??;| pub fn create_item_template( | ||
| title: &str, | ||
| category: &str, | ||
| subcategory: Option<&str>, | ||
| tags: Vec<String>, | ||
| confidence: Option<f64>, | ||
| ) -> String { | ||
| let now = Utc::now().to_rfc3339(); | ||
| let metadata = ItemMetadata { | ||
| id: Uuid::new_v4(), | ||
| title: title.to_string(), | ||
| category: category.to_string(), | ||
| subcategory: subcategory.map(|s| s.to_string()), | ||
| tags, | ||
| created: now.clone(), | ||
| modified: now, | ||
| state: "uncategorized".to_string(), | ||
| confidence, | ||
| }; | ||
|
|
||
| let yaml = serde_yaml::to_string(&metadata).expect("Failed to serialize metadata"); | ||
| format!( | ||
| "---\n{}---\n\n# {}\n\n[Content goes here...]\n", | ||
| yaml, title | ||
| ) | ||
| } |
There was a problem hiding this comment.
The function create_item_template uses .expect() when serializing metadata to YAML, which will cause a panic if serialization fails. For a reusable library function, it's safer to return a Result and let the caller handle the error. This would also make it consistent with write_markdown_file in writer.rs, which correctly propagates serialization errors.
pub fn create_item_template(
title: &str,
category: &str,
subcategory: Option<&str>,
tags: Vec<String>,
confidence: Option<f64>,
) -> anyhow::Result<String> {
let now = Utc::now().to_rfc3339();
let metadata = ItemMetadata {
id: Uuid::new_v4(),
title: title.to_string(),
category: category.to_string(),
subcategory: subcategory.map(|s| s.to_string()),
tags,
created: now.clone(),
modified: now,
state: "uncategorized".to_string(),
confidence,
};
let yaml = serde_yaml::to_string(&metadata)
.map_err(|e| anyhow::anyhow!("Failed to serialize metadata: {}", e))?;
Ok(format!(
"---\n{}---\n\n# {}\n\n[Content goes here...]\n",
yaml, title
))
}| ```rust | ||
| // 1. Update frontmatter | ||
| update_frontmatter("~/noosphere-vault/Ideas/project.md", |meta| { | ||
| meta.category = "Projects".to_string(); | ||
| meta.subcategory = Some("Work".to_string()); | ||
| })?; | ||
|
|
||
| // 2. Move file | ||
| fs::rename( | ||
| "~/noosphere-vault/Ideas/project.md", | ||
| "~/noosphere-vault/Projects/Work/project.md" | ||
| )?; | ||
| ``` |
There was a problem hiding this comment.
The example for moving files uses ~/noosphere-vault/... paths as string literals. The ~ character is typically expanded by shells, but not by most programming languages' standard libraries, including Rust's fs module. This could be misleading for developers. It would be clearer to use a variable representing the vault's base path and construct paths from it. For example:
// Assume vault_path is a Path object pointing to the vault root
let old_path = vault_path.join("Ideas/project.md");
let new_path = vault_path.join("Projects/Work/project.md");
// ... update frontmatter ...
fs::rename(&old_path, &new_path)?;| file.unlock()?; | ||
| } | ||
| self._guard = None; | ||
| fs::remove_file(&self.lock_file).ok(); |
There was a problem hiding this comment.
The release function ignores a potential error from fs::remove_file by using .ok(). If removing the lock file fails (e.g., due to permissions), the stale lock file will remain, potentially causing deadlocks for other processes later. It would be more robust to propagate this error.
| fs::remove_file(&self.lock_file).ok(); | |
| fs::remove_file(&self.lock_file)?; |
| pub fn validate_frontmatter(metadata: &ItemMetadata) -> bool { | ||
| // Validate required fields are present and non-empty | ||
| !metadata.title.is_empty() && !metadata.category.is_empty() && !metadata.created.is_empty() | ||
| } |
There was a problem hiding this comment.
The validate_frontmatter function provides basic checks for the presence of required fields. For more robust validation and to catch data integrity issues earlier, you could consider adding more specific checks, such as:
- Verifying that
createdis a valid RFC3339 timestamp usingchrono::DateTime::parse_from_rfc3339. - Ensuring
categoryis one of the predefined values (e.g., "Inbox", "People", etc.).
…ling and async documentation This commit addresses all 6 review feedback items from Gemini Code Assist: **Breaking Changes (improved error handling)**: 1. FileLock::new() now returns Result<Self> instead of Self - Propagates errors from path validation and directory creation - No more unwrap() panics on invalid paths - Better error messages for debugging 2. create_item_template() now returns Result<String> instead of String - Propagates YAML serialization errors instead of panicking - Consistent with write_markdown_file() error handling 3. FileLock::release() now propagates remove_file errors - Surfaces stale lock file issues instead of hiding them - Drop implementation logs errors to stderr **Non-Breaking Changes (documentation and validation)**: 4. FileLock::acquire() async documentation added - Warning about blocking I/O in async contexts - Example using tokio::spawn_blocking for async runtimes 5. validate_frontmatter() enhanced with robust validation - RFC3339 timestamp validation using chrono::DateTime::parse_from_rfc3339 - Category validation against allowed list: Inbox, People, Projects, Ideas, Admin - Added 2 new tests for invalid timestamps and categories 6. Documentation path examples updated (vault-structure.md) - Replaced tilde (~) paths with proper Path construction - Added note about shell expansion vs Rust standard library - Example now uses Path::new() and join() correctly **Test Updates**: - Updated 7 test functions to handle Result from FileLock::new() - Updated 2 test functions to handle Result from create_item_template() - Added 2 new validation tests (invalid timestamp, invalid category) - Total: 33 tests (32 passed, 1 ignored) **Quality Metrics**: - Coverage: 84.68% region coverage (above 80% target) - Clippy: Clean (no warnings) - All tests passing Addresses: PR #21 review comments Reviewer: Gemini Code Assist Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
22a5159 to
c0596ad
Compare
|
Updated PR to address all 6 review feedback items from Gemini Code Assist. Changes Made:
Quality Checks:
Latest commit: c0596ad |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that lays a solid foundation for the vault filesystem utilities. The code is well-structured into modules with clear responsibilities, following Rust best practices. The use of atomic writes, file locking with RAII, and comprehensive test coverage demonstrates great attention to detail and robustness. The documentation is also very thorough. I've found a few areas for improvement, mainly around making the file lock release idempotent and fixing a small validation logic bug. Overall, fantastic work!
| pub fn release(&mut self) -> Result<()> { | ||
| if let Some(file) = &self._guard { | ||
| file.unlock()?; | ||
| } | ||
| self._guard = None; | ||
| fs::remove_file(&self.lock_file)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The current implementation of release() is not idempotent. If it's called more than once (e.g., explicitly by a user, and then implicitly by Drop), the second call to fs::remove_file will fail because the file no longer exists, causing a spurious error. This can be made more robust by ensuring the unlock and file removal logic only runs if the lock is actually held. Using self._guard.take() is an idiomatic way to achieve this.
pub fn release(&mut self) -> Result<()> {
if let Some(file) = self._guard.take() {
file.unlock()?;
fs::remove_file(&self.lock_file)?;
}
Ok(())
}| ```rust | ||
| use noosphere_sync::vault::{parse_markdown_file, update_frontmatter}; | ||
|
|
||
| // Update category | ||
| update_frontmatter(path, |meta| { | ||
| meta.category = "Projects".to_string(); | ||
| })?; | ||
| ``` |
There was a problem hiding this comment.
The code example for programmatically editing files is a bit confusing as the path variable is not defined. To improve clarity for users, it would be beneficial to make this a complete, self-contained example, similar to the one for moving files.
| ```rust | |
| use noosphere_sync::vault::{parse_markdown_file, update_frontmatter}; | |
| // Update category | |
| update_frontmatter(path, |meta| { | |
| meta.category = "Projects".to_string(); | |
| })?; | |
| ``` | |
| use noosphere_sync::vault::writer::update_frontmatter; | |
| use std::path::Path; | |
| use chrono::Utc; | |
| let file_path = Path::new("/path/to/your/note.md"); | |
| // In a real application, ensure the file exists before updating. | |
| // Update the category and modified timestamp. | |
| update_frontmatter(file_path, |meta| { | |
| meta.category = "Projects".to_string(); | |
| meta.modified = Utc::now().to_rfc3339(); | |
| })?; |
This commit addresses all review feedback from Gemini Code Assist on PR #21. **Round 1 - Error Handling & Async Documentation (6 issues)**: Breaking Changes: 1. FileLock::new() now returns Result<Self> - Validates file paths have parent directory and file name - Propagates directory creation errors instead of panicking - Updated 5 test functions and all documentation examples 2. create_item_template() now returns Result<String> - Propagates YAML serialization errors instead of panicking - Consistent with write_markdown_file() error handling - Updated 2 test functions 3. FileLock::release() error propagation improved - Made idempotent using take() instead of borrow (Round 2 fix) - No spurious errors on double-release (explicit + Drop) Non-Breaking Changes: 4. FileLock::acquire() async documentation added - Warning about blocking I/O in async contexts - Example showing tokio::spawn_blocking usage for async runtimes 5. validate_frontmatter() enhanced validation - RFC3339 timestamp validation using chrono::DateTime::parse_from_rfc3339 - Category validation: must be Inbox, People, Projects, Ideas, or Admin - Added modified field empty check (Round 2 fix) - Added 3 new validation tests 6. Documentation path examples corrected (vault-structure.md) - Replaced tilde (~) paths with proper Path construction - Added note about shell vs Rust path expansion - Completed programmatic editing example (Round 2 fix) **Round 2 - Idempotency & Validation (3 issues)**: 1. FileLock::release() made idempotent [HIGH] - Changed from borrow (&self._guard) to take (self._guard.take()) - Prevents spurious errors when called multiple times - Unlock and file removal only happen if lock held 2. validate_frontmatter() missing modified check [HIGH] - Added metadata.modified.is_empty() validation - Documentation already stated modified is required - Added test_validate_frontmatter_missing_modified test 3. Documentation example completed [MEDIUM] - Added complete example with all imports and variable definitions - vault-structure.md programmatic editing section now self-contained **Test Updates**: - Updated 7 test functions for FileLock::new() Result - Updated 2 test functions for create_item_template() Result - Added 3 new validation tests (invalid timestamp, invalid category, missing modified) - Total: 34 tests (33 passed, 1 ignored) **Quality Metrics**: - Coverage: 84.66% region coverage (above 80% target) - Clippy: Clean (no warnings) - All tests passing Addresses: PR #21 review comments (rounds 1 & 2) Reviewer: Gemini Code Assist Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
c0596ad to
0e55b63
Compare
|
Updated PR to address all 3 review feedback items from round 2 (Gemini Code Assist). Changes Made: Round 2 Fixes:
Quality Checks:
Latest commit: 0e55b63 All review feedback from rounds 1 & 2 has been addressed. |
1 participant
Summary
Implements complete vault filesystem utilities in Rust (sync-service) for markdown-based knowledge storage. This provides the foundation for the File Vault System (EPIC-3), enabling local markdown files with YAML frontmatter compatible with Obsidian and VSCode.
Related Issue
Closes #12 (EPIC-3-1)
Changes
New Modules (sync-service/src/vault/):
template.rs- ItemMetadata struct + markdown template generationparser.rs- YAML frontmatter parsing using gray_matter cratewriter.rs- Atomic file operations using tempfile cratehash.rs- SHA-256 content hashing for change detectionlock.rs- File locking with RAII pattern using fs2 crateDependencies Added:
Documentation:
docs/vault-structure.md- Complete vault structure guidedocs/dev/plans/EPIC-3-1-vault-markdown-utilities.md- Implementation planTesting
Coverage by Module:
Architecture Notes
Critical Design Principle: All vault filesystem operations are in Rust only. The Python api-service NEVER touches
~/noosphere-vault/directly.Key Features:
Vault Structure:
Frontmatter Schema (ItemMetadata):
Verification Steps
Expected Results:
Checklist
🤖 Generated with Claude Code