Repository navigation
feat(reborn): add filesystem substrate - #2996
Conversation
github-actions
Bot
added
scope: db/postgres
github-actions
Bot
added
size: XL
![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 the ironclaw_filesystem crate, which provides a scoped filesystem service supporting local, PostgreSQL, and libSQL backends. It implements a composite root for managing multiple virtual mounts and a scoped view for enforcing granular permissions. Feedback focuses on critical performance and architectural improvements: replacing synchronous I/O with asynchronous operations in the local backend to avoid blocking the Tokio executor, optimizing database backends to use targeted SQL queries instead of in-memory filtering, and refining error handling by replacing string-based 'not found' checks with a dedicated error variant.
| impl RootFilesystem for LocalFilesystem { | ||
| async fn read_file(&self, path: &VirtualPath) -> Result<Vec<u8>, FilesystemError> { | ||
| let resolved = self.resolve_existing(path, FilesystemOperation::ReadFile)?; | ||
| std::fs::read(resolved).map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::ReadFile, | ||
| reason: io_reason(error), | ||
| }) | ||
| } | ||
|
|
||
| async fn write_file(&self, path: &VirtualPath, bytes: &[u8]) -> Result<(), FilesystemError> { | ||
| let resolved = self.resolve_for_write(path, FilesystemOperation::WriteFile)?; | ||
| std::fs::write(resolved, bytes).map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::WriteFile, | ||
| reason: io_reason(error), | ||
| }) | ||
| } | ||
|
|
||
| async fn append_file(&self, path: &VirtualPath, bytes: &[u8]) -> Result<(), FilesystemError> { | ||
| let resolved = self.resolve_for_write(path, FilesystemOperation::AppendFile)?; | ||
| let mut file = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(resolved) | ||
| .map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::AppendFile, | ||
| reason: io_reason(error), | ||
| })?; | ||
| file.write_all(bytes) | ||
| .map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::AppendFile, | ||
| reason: io_reason(error), | ||
| }) | ||
| } | ||
|
|
||
| async fn list_dir(&self, path: &VirtualPath) -> Result<Vec<DirEntry>, FilesystemError> { | ||
| let resolved = self.resolve_existing(path, FilesystemOperation::ListDir)?; | ||
| let mut entries = Vec::new(); | ||
| for entry in std::fs::read_dir(resolved).map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::ListDir, | ||
| reason: io_reason(error), | ||
| })? { | ||
| let entry = entry.map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::ListDir, | ||
| reason: io_reason(error), | ||
| })?; | ||
| let name = entry.file_name().to_string_lossy().to_string(); | ||
| let entry_path = | ||
| VirtualPath::new(format!("{}/{}", path.as_str().trim_end_matches('/'), name))?; | ||
| let metadata = entry.metadata().map_err(|error| FilesystemError::Backend { | ||
| path: entry_path.clone(), | ||
| operation: FilesystemOperation::Stat, | ||
| reason: io_reason(error), | ||
| })?; | ||
| entries.push(DirEntry { | ||
| name, | ||
| path: entry_path, | ||
| file_type: file_type_from_metadata(&metadata), | ||
| }); | ||
| } | ||
| entries.sort_by(|left, right| left.name.cmp(&right.name)); | ||
| Ok(entries) | ||
| } | ||
|
|
||
| async fn stat(&self, path: &VirtualPath) -> Result<FileStat, FilesystemError> { | ||
| let resolved = self.resolve_existing(path, FilesystemOperation::Stat)?; | ||
| let metadata = std::fs::metadata(resolved).map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::Stat, | ||
| reason: io_reason(error), | ||
| })?; | ||
| Ok(FileStat { | ||
| path: path.clone(), | ||
| file_type: file_type_from_metadata(&metadata), | ||
| len: metadata.len(), | ||
| }) | ||
| } | ||
|
|
||
| async fn delete(&self, path: &VirtualPath) -> Result<(), FilesystemError> { | ||
| let resolved = self.resolve_existing(path, FilesystemOperation::Delete)?; | ||
| let metadata = std::fs::metadata(&resolved).map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::Delete, | ||
| reason: io_reason(error), | ||
| })?; | ||
| if metadata.is_dir() { | ||
| std::fs::remove_dir_all(resolved) | ||
| } else { | ||
| std::fs::remove_file(resolved) | ||
| } | ||
| .map_err(|error| FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::Delete, | ||
| reason: io_reason(error), | ||
| }) | ||
| } | ||
|
|
||
| async fn create_dir_all(&self, path: &VirtualPath) -> Result<(), FilesystemError> { | ||
| self.resolve_for_create_dir_all(path).map(|_| ()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The LocalFilesystem implementation uses synchronous std::fs operations (read, write, canonicalize, etc.) within an async_trait. In a high-concurrency service environment, these blocking calls can lead to thread starvation in the Tokio runtime. Consider using tokio::fs for all filesystem operations to ensure the executor remains responsive.
There was a problem hiding this comment.
Fixed in f4a83cd. LocalFilesystem async RootFilesystem methods now use tokio::fs / tokio::io for canonicalize/read/write/append/list/stat/delete/create-dir paths, so the async backend no longer performs synchronous file I/O in the executor.
| let rows = self.all_paths().await?; | ||
| if rows | ||
| .iter() | ||
| .any(|(child_path, _, _)| virtual_prefix_matches(path.as_str(), child_path.as_str())) | ||
| { | ||
| return Ok(FileStat { | ||
| path: path.clone(), | ||
| file_type: FileType::Directory, | ||
| len: 0, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The stat implementation for PostgresRootFilesystem fetches all paths from the database using all_paths() when an exact entry is not found, just to check if the path is a directory prefix. This is extremely inefficient for large filesystems. Use a targeted SQL query with LIKE and LIMIT 1 instead to prevent performance bottlenecks.
| let rows = self.all_paths().await?; | |
| if rows | |
| .iter() | |
| .any(|(child_path, _, _)| virtual_prefix_matches(path.as_str(), child_path.as_str())) | |
| { | |
| return Ok(FileStat { | |
| path: path.clone(), | |
| file_type: FileType::Directory, | |
| len: 0, | |
| }); | |
| } | |
| let client = self.client().await?; | |
| let prefix = format!("{}/%", path.as_str()); | |
| let row = client | |
| .query_opt( | |
| "SELECT 1 FROM root_filesystem_entries WHERE path LIKE $1 LIMIT 1", | |
| &[&prefix], | |
| ) | |
| .await | |
| .map_err(|error| db_error(path.clone(), FilesystemOperation::Stat, error))?; | |
| if row.is_some() { | |
| return Ok(FileStat { | |
| path: path.clone(), | |
| file_type: FileType::Directory, | |
| len: 0, | |
| }); | |
| } |
References
- Use targeted database queries to fetch specific records instead of loading all records and filtering in the application, especially for public endpoints, to prevent performance bottlenecks and DoS vulnerabilities.
There was a problem hiding this comment.
Fixed in f4a83cd. PostgresRootFilesystem::stat now uses a targeted SELECT 1 ... WHERE path LIKE $1 ESCAPE '!' LIMIT 1 child-existence query instead of loading every path.
| reason: "not a directory".to_string(), | ||
| }); | ||
| } | ||
| let rows = self.all_paths().await?; |
There was a problem hiding this comment.
The list_dir implementation for LibSqlRootFilesystem (and PostgresRootFilesystem) fetches all paths from the database to filter them in memory. This will not scale as the number of files grows. Use a targeted SQL query with a prefix match (LIKE 'path/%') to fetch only the relevant entries and avoid performance bottlenecks.
References
- Use targeted database queries to fetch specific records instead of loading all records and filtering in the application, especially for public endpoints, to prevent performance bottlenecks and DoS vulnerabilities.
There was a problem hiding this comment.
Fixed in f4a83cd. PostgreSQL and libSQL list_dir now use targeted escaped-prefix LIKE queries for child rows instead of fetching all paths and filtering in memory.
| pub enum FilesystemError { | ||
| #[error(transparent)] | ||
| Contract(#[from] HostApiError), | ||
| #[error("permission denied for {operation} on scoped path {path:?}")] | ||
| PermissionDenied { | ||
| path: ScopedPath, | ||
| operation: FilesystemOperation, | ||
| }, | ||
| #[error("no backend mount found for virtual path {path:?}")] | ||
| MountNotFound { path: VirtualPath }, | ||
| #[error("virtual path escaped backend mount {path:?}")] | ||
| PathOutsideMount { path: VirtualPath }, | ||
| #[error("symlink escapes backend mount at virtual path {path:?}")] | ||
| SymlinkEscape { path: VirtualPath }, | ||
| #[error("backend mount conflict at virtual path {path:?}")] | ||
| MountConflict { path: VirtualPath }, | ||
| #[error("filesystem backend error during {operation} at {path:?}: {reason}")] | ||
| Backend { | ||
| path: VirtualPath, | ||
| operation: FilesystemOperation, | ||
| reason: String, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The FilesystemError enum should include a dedicated NotFound variant. Currently, the code relies on string matching (e.g., reason == "not found" at line 1232) to detect missing files, which is fragile and inconsistent across backends. Creating specific error variants for different failure modes provides semantically correct and clear error messages and avoids the use of generic string matching patterns.
References
- Create specific error variants for different failure modes (e.g.,
DownloadFailedwith a URL string vs.ManifestReadwith a file path) to provide semantically correct and clear error messages. - When classifying errors by matching substrings, avoid overly generic patterns like "request failed" that can cause false positives. Prefer more specific markers like HTTP status codes or explicit error messages.
There was a problem hiding this comment.
Fixed in f4a83cd. Added FilesystemError::NotFound and removed the stringly reason == "not found" classification path; DB not-found helpers and local ErrorKind::NotFound now use the typed variant.
| if matches!( | ||
| self.exact_entry(path).await?, | ||
| Some((_, FileType::Directory)) | ||
| ) { | ||
| return Err(FilesystemError::Backend { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::AppendFile, | ||
| reason: "cannot append to a directory".to_string(), | ||
| }); | ||
| } | ||
| let mut contents = match self.read_file(path).await { | ||
| Ok(contents) => contents, | ||
| Err(FilesystemError::Backend { reason, .. }) if reason == "not found" => Vec::new(), | ||
| Err(error) => return Err(error), | ||
| }; | ||
| contents.extend_from_slice(bytes); | ||
| self.write_file(path, &contents).await | ||
| } |
There was a problem hiding this comment.
The LibSqlRootFilesystem::append_file implementation reads the entire file into memory, appends the new bytes, and writes it back. This is inefficient and can lead to high memory usage for large files. Moving this logic to the database layer using the SQL concatenation operator (||) will improve performance and simplify the application code.
async fn append_file(&self, path: &VirtualPath, bytes: &[u8]) -> Result<(), FilesystemError> {
if matches!(
self.exact_entry(path).await?,
Some((_, FileType::Directory))
) {
return Err(FilesystemError::Backend {
path: path.clone(),
operation: FilesystemOperation::AppendFile,
reason: "cannot append to a directory".to_string(),
});
}
let conn = self.connect().await?;
conn.execute(
r#"
INSERT INTO root_filesystem_entries (path, contents, is_dir, updated_at)
VALUES (?1, ?2, 0, strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))
ON CONFLICT (path) DO UPDATE SET
contents = contents || excluded.contents,
is_dir = 0,
updated_at = excluded.updated_at
"#,
libsql::params![path.as_str(), libsql::Value::Blob(bytes.to_vec())],
)
.await
.map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::AppendFile, error))?;
Ok(())
}References
- When application logic becomes complex or inefficient, consider moving it to the database layer (e.g., a dedicated SQL query) to improve performance and simplify application code.
There was a problem hiding this comment.
Fixed in f4a83cd. LibSqlRootFilesystem::append_file now performs append in SQL with INSERT ... ON CONFLICT DO UPDATE; the concat result is cast back to BLOB to avoid libSQL returning an invalid value type when reading appended contents.
Filesystem substrate mental model + clarificationsMy understanding is that this PR implements the filesystem substrate primitives, not the full deployment composition yet. The core model I'm reading is: flowchart TD
Tool["Runtime / tool caller"] -->|ScopedPath\n/workspace/README.md| Scoped["ScopedFilesystem"]
Scoped --> MountView["MountView\naliases + permissions"]
MountView -->|resolves to VirtualPath\n/projects/.../README.md| Root["RootFilesystem"]
Host["Trusted host service"] -->|VirtualPath\n/engine/...| Root
Root --> Composite["CompositeRootFilesystem\nlongest-prefix router"]
Composite -->|/projects/*| Local["Local directory backend"]
Composite -->|/engine/* or /memory/*| DB["Postgres / libSQL backend"]
Composite -. follow-up .-> Object["Object store backend"]
Composite -. follow-up .-> Memory["In-memory test backend"]
Catalog["FilesystemCatalog\ntrusted diagnostics only"] -. describes .-> Composite
So routing is automatic from the caller's perspective, but split into two stages: That separation looks right to me:
A few clarifications / follow-up notes:
Overall, this looks aligned with the mental model: the PR gives us the scoped view, trusted root interface, composite router, and concrete local/DB backends. The remaining work is mostly deployment-policy and composition wiring rather than the core filesystem abstraction. |
There was a problem hiding this comment.
Pull request overview
Adds the Reborn filesystem substrate as a new internal crate, including root/scoped filesystem abstractions, backend routing/catalog metadata, local + DB-backed implementations (feature-gated), and initial schema migrations.
Changes:
- Introduces
crates/ironclaw_filesystemwithRootFilesystem,ScopedFilesystem,CompositeRootFilesystem, and mount/catalog metadata types. - Adds local filesystem backend with containment/symlink-escape checks plus libSQL/Postgres DB-backed backends behind feature flags.
- Adds DB schema migrations (
V26/V27) and contract tests for scoped/composite/catalog/root filesystem behavior.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/V26__root_filesystem_entries.sql | Creates root_filesystem_entries table and index for DB-backed root filesystem storage. |
| migrations/V27__root_filesystem_entries_directories.sql | Adds explicit directory support (is_dir) and default contents for DB-backed entries. |
| crates/ironclaw_filesystem/src/lib.rs | Implements filesystem traits/types, local backend, composite routing, and DB backends (feature-gated). |
| crates/ironclaw_filesystem/tests/filesystem_contract.rs | Contract tests for scoped permissions, mount resolution, local backend behavior, and symlink escape denial. |
| crates/ironclaw_filesystem/tests/catalog_contract.rs | Contract tests for catalog placement metadata and composite routing behavior. |
| crates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs | Contract tests for libSQL DB backend behavior (and postgres trait conformance). |
| crates/ironclaw_filesystem/Cargo.toml | Adds new crate manifest with postgres/libsql feature flags and deps. |
| crates/ironclaw_filesystem/CLAUDE.md | Adds crate-local guardrails for boundaries and invariants. |
| Cargo.toml | Wires ironclaw_filesystem into the workspace members. |
| Cargo.lock | Adds lockfile entries for the new crate and its dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_root_filesystem_entries_path | ||
| ON root_filesystem_entries(path); |
There was a problem hiding this comment.
path is the PRIMARY KEY, which already creates a unique btree index in Postgres. The additional idx_root_filesystem_entries_path index is redundant for equality lookups and also won’t help the LIKE '/prefix/%' queries used by the DB backends unless it uses a pattern opclass. Consider removing this index, or replacing it with an index tailored for prefix LIKE (e.g., btree (path text_pattern_ops)), depending on the intended query patterns.
| ON root_filesystem_entries(path); | |
| ON root_filesystem_entries(path text_pattern_ops); |
There was a problem hiding this comment.
Fixed in 7f6e735. The PostgreSQL migration index is now path text_pattern_ops, so prefix LIKE ... ESCAPE '!' child scans can use the index; equality remains covered by the primary key.
| ADD COLUMN IF NOT EXISTS is_dir BOOLEAN NOT NULL DEFAULT FALSE; | ||
|
|
||
| ALTER TABLE root_filesystem_entries | ||
| ALTER COLUMN contents SET DEFAULT '\\x'; |
There was a problem hiding this comment.
The BYTEA default is set to '\\x', which is easy to misread and depends on bytea input escaping rules. Prefer an unambiguous empty-bytea default such as ''::bytea (or E'\\x'::bytea / '\x'::bytea) to make the migration’s intent clear and avoid subtle escaping differences across tooling.
| ALTER COLUMN contents SET DEFAULT '\\x'; | |
| ALTER COLUMN contents SET DEFAULT ''::bytea; |
There was a problem hiding this comment.
Fixed in 7f6e735. V27 now uses ALTER COLUMN contents SET DEFAULT ''::bytea for an explicit empty-bytea default.
| // backend root. If its canonical parent leaves the backend root, an | ||
| // existing symlink in the parent chain caused the escape. | ||
| ensure_contained(path, mount, &canonical_parent, true)?; | ||
| Ok(joined) |
There was a problem hiding this comment.
resolve_for_write returns joined (a non-canonical path) for new targets after checking the canonical parent is contained. Subsequent writes/opens on this path can still follow symlinks if an attacker swaps a path component between the check and the write (TOCTOU), enabling mount escape despite the containment checks. Consider using fd-relative operations (e.g., openat/cap-std) and O_NOFOLLOW/component-by-component traversal to make containment robust against races.
| Ok(joined) | |
| let file_name = joined | |
| .file_name() | |
| .ok_or_else(|| FilesystemError::PathOutsideMount { path: path.clone() })?; | |
| // Return a path rooted at the canonicalized, containment-checked parent | |
| // rather than the original joined path so later writes do not re-resolve | |
| // unchecked ancestor components. | |
| Ok(canonical_parent.join(file_name)) |
There was a problem hiding this comment.
Fixed in 7f6e735. New write targets are now re-rooted on the canonical containment-checked parent before returning the final leaf path. I also documented the remaining local-backend TOCTOU limitation at crate level because fully closing it needs fd-relative/openat-style traversal.
| let len = row.get::<i64>(0).unwrap_or(0).max(0) as u64; | ||
| let is_dir = row.get::<i64>(1).unwrap_or(0) != 0; | ||
| ( |
There was a problem hiding this comment.
row.get::<i64>(0).unwrap_or(0) and the similar is_dir read silently convert any decoding/type error into 0, which can mask DB/schema issues and lead to incorrect behavior (e.g., treating a directory as a file). It’s safer to propagate the libsql error via map_err(...) (as done elsewhere in this file) rather than defaulting.
There was a problem hiding this comment.
Fixed in 7f6e735. LibSqlRootFilesystem::exact_entry now maps libSQL decode errors into FilesystemError::Backend instead of using unwrap_or(0).
| let len = row.get::<i64>(1).unwrap_or(0).max(0) as u64; | ||
| let is_dir = row.get::<i64>(2).unwrap_or(0) != 0; | ||
| paths.push(( |
There was a problem hiding this comment.
These unwrap_or(0) calls on row.get(...) in child_entries silently swallow decoding/type errors and can misclassify entries or lengths. Prefer to propagate the error (or at least map it into a FilesystemError::Backend) so failures are explicit and diagnosable.
There was a problem hiding this comment.
Fixed in 7f6e735. LibSqlRootFilesystem::child_entries now propagates path/length/is_dir decode errors instead of defaulting failed reads to zero.
| fn empty_local_backend(virtual_root: &str) -> LocalFilesystem { | ||
| let dir = tempdir().unwrap(); | ||
| let path = dir.path().to_path_buf(); | ||
| std::mem::forget(dir); | ||
|
|
||
| let mut backend = LocalFilesystem::new(); | ||
| backend | ||
| .mount_local( | ||
| VirtualPath::new(virtual_root).unwrap(), | ||
| HostPath::from_path_buf(path), | ||
| ) | ||
| .unwrap(); | ||
| backend | ||
| } |
There was a problem hiding this comment.
std::mem::forget(dir) intentionally leaks the TempDir to keep the directory alive for the test, but it also leaks resources and can accumulate when running many tests. Prefer TempDir::into_path() (and clean up explicitly) or return a wrapper that holds the TempDir alongside the backend so it stays alive without leaking.
There was a problem hiding this comment.
Fixed in 7f6e735. The catalog test helper now returns the TempDir alongside the backend so the directory stays alive without std::mem::forget or deprecated into_path().
| CREATE INDEX IF NOT EXISTS idx_root_filesystem_entries_path | ||
| ON root_filesystem_entries(path); |
There was a problem hiding this comment.
This schema creates idx_root_filesystem_entries_path, but path is already a PRIMARY KEY (and therefore already indexed). If the goal is to accelerate prefix LIKE queries (used by child_entries/delete), consider using a pattern opclass index; otherwise, remove the redundant index to save write amplification and storage.
| CREATE INDEX IF NOT EXISTS idx_root_filesystem_entries_path | |
| ON root_filesystem_entries(path); |
There was a problem hiding this comment.
Fixed in 7f6e735. The embedded PostgreSQL schema now uses the migration files as its source via include_str!, and V26 defines the text_pattern_ops prefix index.
Code ReviewOverviewAdds a new internal crate Strengths
Issues / ConcernsCorrectness
Local backend TOCTOU windowIn Test hygiene
Minor
RiskLow for merge — the crate is unwired and feature-gated as PR description states. The risks above mostly land when something starts depending on it. The 2 GiB truncation, schema duplication, and tenant column are worth resolving before the first production caller, not necessarily before merging substrate. Suggested follow-ups
|
f4a83cd to
7f6e735
Compare
|
Addressed the current review pass in Implemented from inline/Copilot/Gemini/Ilblackdragon feedback:
Clarifications / intentionally deferred:
Verification run with target artifacts off the NVME volume: CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_filesystem
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_filesystem --features libsql
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_filesystem --features postgres
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo clippy -p ironclaw_filesystem --all-targets --features libsql,postgres -- -D warnings
cargo fmt --check
git diff --checkNote: I attempted the full |
Code ReviewOverviewAdds a new
Cleanly scoped — no Strengths
IssuesCorrectness / consistency
Style / convention
Architecture / future-proofing
Risk Assessment
RecommendationApprove with the delete-semantics divergence (#1) and double pool acquisition (#4) addressed before any production wiring lands. Everything else is style/cleanup that can ride a follow-up PR. The substrate is well-scoped, well-tested, and the security tradeoffs are documented honestly. |
|
Addressed the latest Ilblackdragon review in Implemented:
Left intentionally as follow-up / explicit limitation:
Verification passed: CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_host_api -p ironclaw_filesystem
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_filesystem --features libsql
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo test -p ironclaw_filesystem --features postgres
CARGO_TARGET_DIR=/tmp/ironclaw-fs-target cargo clippy -p ironclaw_host_api -p ironclaw_filesystem --all-targets --features libsql,postgres -- -D warnings
cargo fmt --check
git diff --check |
* feat(reborn): add filesystem substrate * fix(reborn): address filesystem review findings * fix(reborn): address filesystem substrate review * fix(reborn): align filesystem substrate semantics
coderabbitai (round 7) flagged that leaf containment checks aren't atomic with the mutation they gate. The gap is real but matches the already-deferred residual documented on resolve_for_write's re-rooting step (full fix needs openat/O_NOFOLLOW/cap-std, tracked via PR #2996 review) — extend that documentation to resolve_for_create_dir_all and delete instead of re-litigating the same heavy-lift follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ity primitives (#6695) * feat(sandbox): leaf-scoped mount containment + per-user sandbox identity primitives Ships two related, unwired-by-design slices of the persistent per-user sandbox program: - ironclaw_filesystem: leaf-scoped mount containment. resolve_joined now returns a per-request containment_root that, for a leaf_scoped mount, is host_root/<first-tail-segment> instead of the shared host_root — closing a same-mount cross-leaf symlink escape a plain mount_local containment check would miss. mount_local_per_leaf is the constructor; a bare-root request against such a mount is rejected outright (no safe containment root for "every caller's leaf"). - ironclaw_host_runtime: identity + attribution primitives for the persistent per-user sandbox container model — RebornSandboxUserKey ({tenant,user}-only container/workspace key), the labels-as-identity registry (Docker label helpers, SandboxActivityRegistry, BackgroundJobRegistry), and ConnectionAttributionResolver (source-IP to {tenant,user} resolution for the shared egress proxy, design decision D9). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): attribution real-docker test reuses connect_docker() fallback docker_gate::docker_available() shells out to the docker CLI, which resolves the daemon through whatever context is active (Colima, Docker Desktop, a remote host). The test then connected directly via Docker::connect_with_local_defaults(), which only honors DOCKER_HOST or the hardcoded /var/run/docker.sock, so the gate could pass while the connection still failed on any machine using a non-default socket. Reuse sandbox_process::connect_docker() instead of reimplementing resolution: it already tries connect_with_local_defaults() then falls back through unix_socket_candidates(), the same path production containers connect through. A connect_docker() failure now prints a SKIP line rather than panicking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR review — leaf-creation bug, attribution hygiene, docker CI gate Leaf-scoped mounts rejected a brand-new leaf's first write/create_dir_all (ensure_existing_ancestor_contained had no bootstrap case for the shared host_root when a caller's leaf doesn't exist yet); accept that one ancestor now and add regression coverage for write-path creation, write-path cross-leaf symlink escape, and create_dir_all bootstrap. Attribution cache: sweep expired entries on miss so a long-running resolver doesn't grow the cache unboundedly, and log the missing/malformed-label fail-closed branch like its sibling branches. Add coverage for malformed/ missing container network IPs. Docker CI gate: the attribution real-Docker test's connect_docker() failure branch always skipped, even under IRONCLAW_REQUIRE_DOCKER_TESTS=1 — panic in that mode instead, matching docker_gate's existing fail-closed pattern. Pull busybox:1.36 before create_container so the test doesn't depend on a pre-warmed local image cache (this is what broke it in CI). registry.rs/attribution.rs: crate::-rooted imports per repo convention; trim sandbox_process.rs's module header to stable ownership, not PR-state. Add BackgroundJobRegistry, malformed-candidate-parsing, and concurrent SandboxActivityRegistry coverage. docs/reborn/contracts/host-runtime.md: one forward-pointing sentence noting RebornSandboxUserKey's future coarser identity model doesn't yet supersede the scope-derived identity this contract already documents. architecture ratchet: baseline the two new test/dead-code seams this introduces (attribution.rs dead-code method x4, test-support method x1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): O(1) drop_dead pruning + coverage for empty-alive-pids and concurrent attribution resolve Addresses review 4784267719 on PR #6695: - BackgroundJobRegistry::drop_dead now builds a HashSet once instead of a per-job linear scan of alive_pids (O(J) not O(J x A)). - Add background_job_registry_drop_dead_with_empty_alive_pids_removes_all_jobs. - Add concurrent_resolve_calls_complete_with_consistent_attribution covering simultaneous ConnectionAttributionResolver::resolve calls. - Doc-comment the two known-but-deferred tradeoffs (attribution thundering herd, unbounded BackgroundJobRegistry growth) — both need the not-yet-wired caller (W6 / exec_transport+reaper) to know the right shape, so building a mechanism now would be guessing; left as explicit follow-ups instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): force real cache-miss overlap in attribution concurrency test FakeLookup::containers_on_network returned immediately with no yield point, so the 20 spawned resolve() tasks could run to sequential completion without ever actually overlapping in the miss/query/insert window the test claims to exercise. Add an optional Barrier that all callers wait on inside containers_on_network, forcing genuine concurrent cache misses before any insert proceeds. Addresses CodeRabbit review 4788508880. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address new PR #6695 review round (CI-hang test, silent-ok docs, IPv6 attribution, typed mount resolution, key codec dedup - attribution.rs: bound the concurrent-resolve barrier test with a timeout so a regression can't hang CI; add silent-ok rationale comments on the Docker-boundary .ok() parses; fix container_addresses_on_network to also read bollard's global_ipv6_address field (an IPv6-only peer was previously never matchable); add empty-listing and IPv6 coverage. - registry.rs: module header now names all three responsibilities (label codec, activity registry, background-job registry); add a concurrent record/jobs_for/drop_dead test for BackgroundJobRegistry to match the existing SandboxActivityRegistry coverage. - user_key.rs: clarify that RebornSandboxScopeKey remains authoritative for the currently-wired transport; RebornSandboxUserKey is reserved for the future persistent per-user transport. - docker_gate.rs: header now describes the daemon-only gate accurately instead of overclaiming a required ironclaw-worker image. - local.rs: resolve_joined now returns a typed ResolvedMountPath (joined, containment_root, bootstrap_root) instead of a positional tuple plus a separately re-derived bootstrap_root at two of three call sites. - New key_codec module: shared length-prefixed encoding + SHA-256 digest for RebornSandboxScopeKey and RebornSandboxUserKey, replacing two independently-maintained copies of the same framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(sandbox): split attribution.rs test module into its own file My prior fixes pushed attribution.rs from 951 to 1029 lines, crossing the hard 1000-line threshold. Moved #[cfg(test)] mod tests (FakeLookup harness, 17 unit tests, the gated real-Docker integration test) into a sibling attribution_tests.rs via #[path], matching the file's existing docker_gate #[path] pattern. Pure move, no behavior change: production attribution.rs is now 367 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address round-6 review findings on #6695 - crate::-qualify key_codec imports in user_key.rs/scope_key.rs (matches the PR's own crate:: convention already used for registry/attribution) - add missing silent-ok rationale for the dropped created_at parse error in UserContainerCandidate::from_summary - gate the two cross-leaf symlink-escape tests in local.rs with #[cfg(unix)] (std::os::unix::fs::symlink does not compile on the windows release target) - document mount_local_per_leaf's bare-root-denial, first-use-bootstrap, and per-leaf symlink-containment contract in filesystem.md - add missing-tenant and malformed-tenant-label fail-closed tests to attribution_tests.rs (existing coverage only exercised the user field) - correct the ConnectionAttributionResolver doc comments: invalidate() collapses staleness "toward" zero, not "to" zero — a concurrent in-flight resolve() can still re-insert a stale entry after invalidate() removes it. Not fixed (no caller exists yet to fix a race against), but the doc must not overclaim a guarantee it does not have. * fix(filesystem): reject dangling final symlink in resolve_for_write Addresses coderabbitai review 4790451629 on PR #6695: try_exists follows symlinks and reports false for a dangling one, so a pre-planted dangling symlink at the write target fell through to the brand-new-file bootstrap path. write_file/append_file open with O_CREAT, so the OS would create the file wherever the symlink points, escaping leaf containment. resolve_for_write now checks existence via symlink_metadata (lstat) and fails closed with SymlinkEscape when a symlink entry can't be canonicalized, instead of silently falling through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(filesystem): document TOCTOU residual on create_dir_all/delete coderabbitai (round 7) flagged that leaf containment checks aren't atomic with the mutation they gate. The gap is real but matches the already-deferred residual documented on resolve_for_write's re-rooting step (full fix needs openat/O_NOFOLLOW/cap-std, tracked via PR #2996 review) — extend that documentation to resolve_for_create_dir_all and delete instead of re-litigating the same heavy-lift follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ity primitives (nearai#6695) * feat(sandbox): leaf-scoped mount containment + per-user sandbox identity primitives Ships two related, unwired-by-design slices of the persistent per-user sandbox program: - ironclaw_filesystem: leaf-scoped mount containment. resolve_joined now returns a per-request containment_root that, for a leaf_scoped mount, is host_root/<first-tail-segment> instead of the shared host_root — closing a same-mount cross-leaf symlink escape a plain mount_local containment check would miss. mount_local_per_leaf is the constructor; a bare-root request against such a mount is rejected outright (no safe containment root for "every caller's leaf"). - ironclaw_host_runtime: identity + attribution primitives for the persistent per-user sandbox container model — RebornSandboxUserKey ({tenant,user}-only container/workspace key), the labels-as-identity registry (Docker label helpers, SandboxActivityRegistry, BackgroundJobRegistry), and ConnectionAttributionResolver (source-IP to {tenant,user} resolution for the shared egress proxy, design decision D9). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): attribution real-docker test reuses connect_docker() fallback docker_gate::docker_available() shells out to the docker CLI, which resolves the daemon through whatever context is active (Colima, Docker Desktop, a remote host). The test then connected directly via Docker::connect_with_local_defaults(), which only honors DOCKER_HOST or the hardcoded /var/run/docker.sock, so the gate could pass while the connection still failed on any machine using a non-default socket. Reuse sandbox_process::connect_docker() instead of reimplementing resolution: it already tries connect_with_local_defaults() then falls back through unix_socket_candidates(), the same path production containers connect through. A connect_docker() failure now prints a SKIP line rather than panicking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR review — leaf-creation bug, attribution hygiene, docker CI gate Leaf-scoped mounts rejected a brand-new leaf's first write/create_dir_all (ensure_existing_ancestor_contained had no bootstrap case for the shared host_root when a caller's leaf doesn't exist yet); accept that one ancestor now and add regression coverage for write-path creation, write-path cross-leaf symlink escape, and create_dir_all bootstrap. Attribution cache: sweep expired entries on miss so a long-running resolver doesn't grow the cache unboundedly, and log the missing/malformed-label fail-closed branch like its sibling branches. Add coverage for malformed/ missing container network IPs. Docker CI gate: the attribution real-Docker test's connect_docker() failure branch always skipped, even under IRONCLAW_REQUIRE_DOCKER_TESTS=1 — panic in that mode instead, matching docker_gate's existing fail-closed pattern. Pull busybox:1.36 before create_container so the test doesn't depend on a pre-warmed local image cache (this is what broke it in CI). registry.rs/attribution.rs: crate::-rooted imports per repo convention; trim sandbox_process.rs's module header to stable ownership, not PR-state. Add BackgroundJobRegistry, malformed-candidate-parsing, and concurrent SandboxActivityRegistry coverage. docs/reborn/contracts/host-runtime.md: one forward-pointing sentence noting RebornSandboxUserKey's future coarser identity model doesn't yet supersede the scope-derived identity this contract already documents. architecture ratchet: baseline the two new test/dead-code seams this introduces (attribution.rs dead-code method x4, test-support method x1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): O(1) drop_dead pruning + coverage for empty-alive-pids and concurrent attribution resolve Addresses review 4784267719 on PR nearai#6695: - BackgroundJobRegistry::drop_dead now builds a HashSet once instead of a per-job linear scan of alive_pids (O(J) not O(J x A)). - Add background_job_registry_drop_dead_with_empty_alive_pids_removes_all_jobs. - Add concurrent_resolve_calls_complete_with_consistent_attribution covering simultaneous ConnectionAttributionResolver::resolve calls. - Doc-comment the two known-but-deferred tradeoffs (attribution thundering herd, unbounded BackgroundJobRegistry growth) — both need the not-yet-wired caller (W6 / exec_transport+reaper) to know the right shape, so building a mechanism now would be guessing; left as explicit follow-ups instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): force real cache-miss overlap in attribution concurrency test FakeLookup::containers_on_network returned immediately with no yield point, so the 20 spawned resolve() tasks could run to sequential completion without ever actually overlapping in the miss/query/insert window the test claims to exercise. Add an optional Barrier that all callers wait on inside containers_on_network, forcing genuine concurrent cache misses before any insert proceeds. Addresses CodeRabbit review 4788508880. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address new PR nearai#6695 review round (CI-hang test, silent-ok docs, IPv6 attribution, typed mount resolution, key codec dedup - attribution.rs: bound the concurrent-resolve barrier test with a timeout so a regression can't hang CI; add silent-ok rationale comments on the Docker-boundary .ok() parses; fix container_addresses_on_network to also read bollard's global_ipv6_address field (an IPv6-only peer was previously never matchable); add empty-listing and IPv6 coverage. - registry.rs: module header now names all three responsibilities (label codec, activity registry, background-job registry); add a concurrent record/jobs_for/drop_dead test for BackgroundJobRegistry to match the existing SandboxActivityRegistry coverage. - user_key.rs: clarify that RebornSandboxScopeKey remains authoritative for the currently-wired transport; RebornSandboxUserKey is reserved for the future persistent per-user transport. - docker_gate.rs: header now describes the daemon-only gate accurately instead of overclaiming a required ironclaw-worker image. - local.rs: resolve_joined now returns a typed ResolvedMountPath (joined, containment_root, bootstrap_root) instead of a positional tuple plus a separately re-derived bootstrap_root at two of three call sites. - New key_codec module: shared length-prefixed encoding + SHA-256 digest for RebornSandboxScopeKey and RebornSandboxUserKey, replacing two independently-maintained copies of the same framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(sandbox): split attribution.rs test module into its own file My prior fixes pushed attribution.rs from 951 to 1029 lines, crossing the hard 1000-line threshold. Moved #[cfg(test)] mod tests (FakeLookup harness, 17 unit tests, the gated real-Docker integration test) into a sibling attribution_tests.rs via #[path], matching the file's existing docker_gate #[path] pattern. Pure move, no behavior change: production attribution.rs is now 367 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address round-6 review findings on nearai#6695 - crate::-qualify key_codec imports in user_key.rs/scope_key.rs (matches the PR's own crate:: convention already used for registry/attribution) - add missing silent-ok rationale for the dropped created_at parse error in UserContainerCandidate::from_summary - gate the two cross-leaf symlink-escape tests in local.rs with #[cfg(unix)] (std::os::unix::fs::symlink does not compile on the windows release target) - document mount_local_per_leaf's bare-root-denial, first-use-bootstrap, and per-leaf symlink-containment contract in filesystem.md - add missing-tenant and malformed-tenant-label fail-closed tests to attribution_tests.rs (existing coverage only exercised the user field) - correct the ConnectionAttributionResolver doc comments: invalidate() collapses staleness "toward" zero, not "to" zero — a concurrent in-flight resolve() can still re-insert a stale entry after invalidate() removes it. Not fixed (no caller exists yet to fix a race against), but the doc must not overclaim a guarantee it does not have. * fix(filesystem): reject dangling final symlink in resolve_for_write Addresses coderabbitai review 4790451629 on PR nearai#6695: try_exists follows symlinks and reports false for a dangling one, so a pre-planted dangling symlink at the write target fell through to the brand-new-file bootstrap path. write_file/append_file open with O_CREAT, so the OS would create the file wherever the symlink points, escaping leaf containment. resolve_for_write now checks existence via symlink_metadata (lstat) and fails closed with SymlinkEscape when a symlink entry can't be canonicalized, instead of silently falling through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(filesystem): document TOCTOU residual on create_dir_all/delete coderabbitai (round 7) flagged that leaf containment checks aren't atomic with the mutation they gate. The gap is real but matches the already-deferred residual documented on resolve_for_write's re-rooting step (full fix needs openat/O_NOFOLLOW/cap-std, tracked via PR nearai#2996 review) — extend that documentation to resolve_for_create_dir_all and delete instead of re-litigating the same heavy-lift follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Closes the filesystem half of PR2 from #2987. The event substrate half is already in review as #2993; this PR keeps the filesystem substrate separate so reviewers can evaluate it independently.
Adds
crates/ironclaw_filesystemwith:RootFilesystem,ScopedFilesystem, andCompositeRootFilesystemFilesystemCatalog,MountDescriptor,PathPlacement, and backend placement metadataread_file,write_file,append_file,list_dir,stat,delete,create_dir_allcrates/ironclaw_filesystem/CLAUDE.mdMigrations are included as
V26/V27becausereborn-integrationalready hasV25__wasm_fuel_limit_bump.sql:migrations/V26__root_filesystem_entries.sqlmigrations/V27__root_filesystem_entries_directories.sqlScope boundaries
This PR intentionally does not include:
ironclaw_eventschanges — covered by feat(reborn): durable event/audit substrate #2993ironclaw_extensions— deferred from PR1 and should land as a separate follow-up after filesystem substrate review/memorygrammar or memory repository adapters — those remain owned byironclaw_memoryExposure checklist
Verification
TDD note: copied the filesystem contract tests first against a RED stub and confirmed
cargo test -p ironclaw_filesystemfailed due to missing contract types before porting the implementation.Passed:
Earlier boundary grep returned no forbidden normal Reborn dependencies:
Refs #2987.