feat(attachments): MountView-based attachment landing crate (#4644)

[ilblackdragon]

Summary

Track 6 (byte-storage foundation) of #4644, originally stacked on Track 2 (#4655, now merged). We reordered Track 6 ahead of the rest of Track 3 because model-visibility of attachments depends on bytes living in storage — ProductAttachmentDescriptor is bytes-free by design, so an image/doc can only reach the model by being read back from the project filesystem. This PR builds that storage foundation.

New leaf crate ironclaw_attachments owns the single, channel-agnostic routine that writes inbound attachment bytes into the agent-accessible project filesystem and returns the ScopedPath to record as an attachment's storage key.

Scope: crates/ only, no src/ changes.

Why through the MountView authority

The write goes through the project-scoped ScopedFilesystem (deps: ironclaw_filesystem + ironclaw_host_api only), not a self-computed host path. Writer and reader share one MountView, so:

• the landed attachment is reachable at the same virtual path the agent's file_read/list_dir tools resolve through (this and later turns);
• the write requires a MountPermissions write grant — a read-only mount fails closed with PermissionDenied, enforced inside ScopedFilesystem before any backend dispatch.

This structurally prevents the v2 "two roots" bug (attachments under one host root, the agent's /project/ mount under another) from recurring — the writer and the reader literally cannot diverge.

API (exported surface)

• attachment_scoped_path(project_alias, date, &AttachmentLanding) -> Result<ScopedPath> — build {alias}/attachments/{date}/{message_id}-{index}-{filename} with every user-influenced segment sanitized. The 1-based index prefix makes two attachments in one message land at distinct paths even when they share a filename; uniqueness is carried by (message_id, index), never by the (cosmetic) filename segment. ScopedPath::new additionally rejects .. path segments and raw host paths. Date is passed in (no chrono dep, fully deterministic).
• land_attachment(&ScopedFilesystem<F>, &ResourceScope, project_alias, date, &AttachmentLanding, bytes, max_bytes) -> Result<ScopedPath> — reject over-max_bytes input with AttachmentLandingError::TooLarge before any write, then resolve the path + write the bytes through the authority; return the ScopedPath storage key.
• DEFAULT_MAX_ATTACHMENT_BYTES (25 MiB) — a default size cap callers may pass to land_attachment or tighten to their provider's limit.
• AttachmentLanding, AttachmentLandingError, ATTACHMENTS_DIR.

sanitize_attachment_segment is an internal helper (collapse an attacker-controlled string to one safe segment: alphanumerics + . - _; everything else → _; trim leading/trailing dots, neutralizing .. and hidden-file segments). It is intentionally not exported — callers land through land_attachment, which sanitizes for them.

Tests

9 tests: sanitization (separators / interior-vs-edge dots / empty / non-ASCII); UUID message_id is sanitization-stable so distinct messages never alias; path construction + synthesized filename; same-named attachments on one message land at distinct paths; traversal contained under the mount (no .. segment escapes); write→read round-trip through a shared InMemoryBackend; same-filename land-then-land round-trip proving no clobber (both byte sets survive); oversize rejection asserting TooLarge and that nothing was written; and fail-closed PermissionDenied on a read-only mount.

cargo clippy -p ironclaw_attachments --all-features --tests   # zero warnings
cargo test  -p ironclaw_attachments --all-features            # 9/9 pass

Deferred (later Track 6 + Track 3 slices)

• Wire land_attachment into the Reborn inbound path and set AttachmentRef.storage_key (Track 2's field) to the returned ScopedPath; that caller enforces max_bytes on a streaming reader before buffering the full Vec<u8>.
• Per-attachment memory index note through the Reborn memory surface (memory_search discoverability).
• Model-facing project_path so file_read resolves it; sandbox (SANDBOX_ENABLED=true) integration test; convergence with WASM store_attachment_data.
• The remaining Track 3 pieces (extraction/transcription/augment → content_parts, chat_workflow.rs [non_text_content] fix) now have a place to read bytes from.

Stacking

Rebased onto main after #4655 merged (only this crate's own commits replayed). Brand-new crate, code-independent of Tracks 1–2.

Summary by CodeRabbit

Release Notes

• New Features
  • Added attachment persistence system that stores inbound attachments in the project filesystem for ongoing access across interactions.
  • Enforces secure filename handling and path containment checks to prevent unsafe file operations.
  • Default size limit of 25MB per attachment with configurable thresholds.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[abbyshekit]

Code review — feat(attachments): MountView-based attachment landing crate (#4644)

Multi-agent review (security · bugs · performance · tests · conventions) at d1f9b109. Diff-only mode.

Review of a draft/WIP PR (review requested) — early feedback; some items may be addressed in later commits of the stack.

4 findings — 2 High, 1 Medium, 1 Nit. Posted as a comment (advisory). Confidence ≥ 50, deduplicated across reviewers.

Sev	Conf	Reviewer	Location	Finding
High	80%	bugs	landing.rs:98	Two attachments in one message with the same filename collide and silently overwrite (index never enters the path)
High	80%	tests	landing.rs:365 (in body)	No test for attachment path collision / silent overwrite (last-write-wins)
Medium	72%	performance	landing.rs:121	Attachment bytes landed with no size cap (full in-memory materialization + unbounded disk growth)
Nit	62%	conventions	Cargo.toml:1	New crate enforces neither workspace lints nor the inline unreachable_pub warn its dependencies use

Findings without an inline anchor (cited lines fall outside the diff hunks)

[High · 80%] No test for attachment path collision / silent overwrite (last-write-wins)

crates/ironclaw_attachments/src/landing.rs:365-383 — tests reviewer

land_attachment writes via ScopedFilesystem::write_bytes, which calls put(.., CasExpectation::Any) (verified in crates/ironclaw_filesystem/src/scoped.rs:365-374 and :101-111) — an unconditional last-write-wins overwrite, no CAS, no dedup. attachment_scoped_path derives the path from {project_alias}/attachments/{date}/{message_id}-{filename}; the index field only disambiguates the SYNTHESIZED fallback name (fallback_filename, used when filename is None), NOT a caller-provided filename. So two attachments in the same message with the same provided filename and date resolve to the identical ScopedPath, and the second land_attachment call silently overwrites the first attachment's bytes — data loss. The PR's tests cover the happy-path build, the synthesized-name path, one traversal string, a read-write round trip, and the read-only fail-closed path, but none assert what happens on a duplicate path. The focus for this PR explicitly calls out 'path COLLISION/overwrite when two attachments share message_id/filename' and 'negative tests for ... collision'. This is happy-path-only coverage of a write routine whose collision semantics are load-bearing.

Fix: Add tests::landing::two_attachments_with_same_message_id_and_filename_collide covering the collision case: land_attachment twice over one InMemoryBackend with identical message_id/date/filename but different bytes, then read back via get and assert which write wins (documents the last-write-wins overwrite), OR — if collision is meant to be prevented — assert the second landing produces a distinct ScopedPath (e.g. index-suffixed) so no overwrite occurs.

---

_{Generated by near-ai-code-review (5 parallel reviewer agents + intent analysis). Diff-only; confidence ≥ 50; ≤ 15 inline comments.}

[abbyshekit]

[High · 80% confidence] Two attachments in one message with the same filename collide and silently overwrite (index never enters the path)

bugs (also: performance, tests) reviewer

attachment_scoped_path builds the storage key as {project_alias}/attachments/{date}/{message_id}-{filename}. The index field — documented at line 41-43 as "Zero-based index of this attachment within its message. Disambiguates same-named files" — is ONLY consulted in the None branch (line 100, via fallback_filename). When a filename is present (line 99), index is dropped entirely, so the path contains no per-attachment uniqueness.

Many channels deliver multiple attachments in a single message that share a filename (e.g. two image.png, or a client that names every photo image.jpg). Both land at the identical ScopedPath. land_attachment then calls filesystem.write_bytes(...), which is put(.., CasExpectation::Any) (crates/ironclaw_filesystem/src/scoped.rs:364-373) — an unconditional overwrite. The second attachment silently clobbers the first. The first attachment's ScopedPath was already returned to the caller and is recorded as that attachment's storage key, so it now resolves to the WRONG bytes when the agent reads it back. Since ProductAttachmentDescriptor is bytes-free and the model only sees attachments via read-back from the project FS, the model receives attachment B's bytes for attachment A — a lost-write / wrong-content invariant break, not just a cosmetic clash.

Sanitization widens the collision surface: distinct raw filenames map to one segment (a/b.png and a_b.png both → a_b.png; résumé.txt and r sum .txt both → r_sum_.txt), so even differently-named attachments in the same message+date collide and overwrite. The unconditional-index fix below resolves every same-message variant because each attachment in a message carries a distinct index.

Fix: Always include index in the path segment so each attachment in a message is unique regardless of (possibly colliding) filenames. E.g. build the leaf as {message_id}-{index}-{filename} in both branches, or fold index into fallback_filename and a parallel prefix for the Some branch. Add negative tests: two AttachmentLanding with the same message_id+filename but different index must produce distinct ScopedPaths; and a land-then-land round-trip must not clobber the first attachment's bytes.

    let leaf = match landing.filename {
        Some(name) => format!("{}-{}", landing.index, sanitize_attachment_segment(name)),
        None => fallback_filename(landing.index, landing.fallback_extension),
    };
    let full = format!(
        "{}/{ATTACHMENTS_DIR}/{date}/{message_id}-{leaf}",
        project_alias.trim_end_matches('/')
    );

[ilblackdragon]

Already addressed on this branch (commit 648f39659, predating the review): attachment_scoped_path now renders index into both branches — the leaf is {message_id}-{index}-{filename} (1-based) unconditionally, so two attachments in one message never share a path even with identical (or identically-sanitized) filenames. Tests same_named_attachments_on_one_message_never_collide (path level) and a new same_named_attachments_land_without_clobbering (land-then-land round-trip asserting both byte sets survive distinctly) cover exactly the lost-write case you described.

[abbyshekit]

[Medium · 72% confidence] Attachment bytes landed with no size cap (full in-memory materialization + unbounded disk growth)

performance (also: tests, security) reviewer

land_attachment takes bytes: Vec<u8> (line 127) — the entire attachment already materialized in memory — and passes it straight to write_bytes (line 133) with no size check anywhere in this crate or on the write path. ScopedFilesystem::write_bytes -> put -> backend put impose no byte bound (verified in scoped.rs / local.rs / in_memory.rs). The sibling read API ships read_bytes_bounded(max_bytes) precisely to avoid materializing oversized content, but there is no write-side equivalent here, so the landing routine is the unbounded-write counterpart. A single large attachment is fully buffered as one Vec<u8> (peak RSS ~= file size, plus a copy at Entry::bytes), and many attachments grow attachments/{date}/ without limit, so this leaf routine is the choke point for both memory and disk exhaustion as soon as a channel adapter feeds it real inbound bytes (per-message hot path).

Fix: Add an explicit max_bytes bound to land_attachment/AttachmentLanding and reject (return an AttachmentLandingError variant) when bytes.len() exceeds it before calling write_bytes, mirroring read_bytes_bounded. Document that callers must enforce the cap before buffering the full Vec<u8> (ideally pass a streaming/bounded reader rather than an owned Vec so oversized inputs are rejected without full materialization). Add an oversize negative test.

[ilblackdragon]

Addressed in 5249ebc66. land_attachment now takes a required max_bytes and rejects over-limit input with a new AttachmentLandingError::TooLarge { size, max } before building the path or calling write_bytes — the write-side counterpart to read_bytes_bounded. Added DEFAULT_MAX_ATTACHMENT_BYTES (25 MiB) as a default callers may use or tighten. New test land_rejects_oversized_attachment_before_writing asserts TooLarge and that nothing was written. On full materialization: the bytes already arrive as an owned Vec<u8> at this boundary, so the doc now explicitly directs callers to enforce the same bound on a streaming reader before buffering; the streaming-reader signature is a follow-up for the channel-adapter track that feeds real inbound bytes.

[abbyshekit]

[Nit · 62% confidence] New crate enforces neither workspace lints nor the inline unreachable_pub warn its dependencies use

conventions reviewer

ironclaw_attachments declares no lint policy at all. The repo has two coexisting conventions for the unreachable_pub lint, and this new crate follows neither. (1) The root Cargo.toml defines [workspace.lints.rust] with unreachable_pub = "warn" and dead_code = "warn", opted into per-crate via [lints] workspace = true; its own comment says this is "scoped to the hook/reborn crates" (e.g. crates/ironclaw_reborn/Cargo.toml:90-91, crates/ironclaw_reborn_composition/Cargo.toml:200-201, crates/ironclaw_hooks/Cargo.toml:50-51). This crate is part of "IronClaw Reborn" (per its own lib.rs doc) yet does not opt in. (2) Both of its direct dependencies instead declare the lint inline at crate root: crates/ironclaw_filesystem/src/lib.rs:16 and crates/ironclaw_host_api/src/lib.rs:32 both carry #![warn(unreachable_pub)] (as does crates/ironclaw_common/src/lib.rs:2). The new crate's lib.rs has no such attribute either. Net: every crate this one directly touches enforces unreachable_pub, but ironclaw_attachments enforces nothing. Low impact in practice — the crate's surface is entirely intentional pub use re-exports, so the lint likely would not fire today — but it is a consistency gap with a clear in-repo precedent, and adopting it guards the surface as the crate grows.

Fix: Add [lints]\nworkspace = true to crates/ironclaw_attachments/Cargo.toml (preferred — matches the reborn-crate convention and also picks up dead_code), or add #![warn(unreachable_pub)] at the top of crates/ironclaw_attachments/src/lib.rs to match its two direct dependencies (ironclaw_filesystem, ironclaw_host_api).

# in crates/ironclaw_attachments/Cargo.toml, after [dependencies]/[dev-dependencies]:
[lints]
workspace = true

[ilblackdragon]

Addressed (commit ad08a17a4): crates/ironclaw_attachments/Cargo.toml now carries [lints]\nworkspace = true, matching the reborn-crate convention (and picking up dead_code too).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a94ca937-b638-4f5a-91da-206e47959c1d

📥 Commits

Reviewing files that changed from the base of the PR and between 5249ebc and a5bd836.

📒 Files selected for processing (1)

• crates/ironclaw_attachments/src/landing.rs

---

📝 Walkthrough

Walkthrough

Repo invariants: none violated (CLAUDE.md/AGENTS.md/.claude/rules). New crate ironclaw_attachments persists inbound attachment bytes to a project-scoped filesystem with sanitized path construction, pre-write size limits, permission-aware writes, and extensive tests for edge cases and containment.

Changes

Attachment Landing Module

Layer / File(s)	Summary
Workspace and crate manifest setup Cargo.toml, crates/ironclaw_attachments/Cargo.toml	Workspace adds member crate; manifest declares Rust 2024 edition, dependencies on ironclaw_filesystem and ironclaw_host_api, thiserror, and tokio dev-dependency.
Core type contract and error model crates/ironclaw_attachments/src/landing.rs (lines 1–58)	ATTACHMENTS_DIR and DEFAULT_MAX_ATTACHMENT_BYTES (25 MiB); AttachmentLandingError variants for invalid path, oversized input, and write failures; AttachmentLanding<'a> metadata struct.
Safe path construction and sanitization crates/ironclaw_attachments/src/landing.rs (lines 60–123)	sanitize_attachment_segment restricts characters and trims dots; attachment_filename uses sanitized original or synthesizes attachment.{ext}; attachment_scoped_path formats {project_alias}/attachments/{date}/{message_id}-{1-based-index}-{filename} and maps ScopedPath::new errors to InvalidPath.
Async attachment landing and filesystem write crates/ironclaw_attachments/src/landing.rs (lines 125–164)	land_attachment rejects oversized inputs before writes, computes scoped path, writes bytes via ScopedFilesystem::write_bytes under the given ResourceScope, and returns the resulting ScopedPath.
Public module API and comprehensive tests crates/ironclaw_attachments/src/lib.rs, crates/ironclaw_attachments/src/landing.rs (lines 166–477)	lib.rs re-exports landing API. Tests validate sanitization, dot trimming, filename fallback, collision avoidance, directory-traversal safety, round-trip write/read across scopes, fail-closed read-only behavior, and pre-write rejection preventing writes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Universal attachments across all channels: wire the legacy attachment pipeline into v1 + extensible format registry + polished web UX #4644 — overlap: implements attachment landing semantics, scoped-path construction, sanitization, size checks, and ScopedFilesystem/ResourceScope behavior described in the issue.

Poem

Clean bytes fall into scoped domains,
Paths sanitized, no rogue chains,
Size checked first, writes made right,
Read-only mounts close up tight,
Tests sing collisions into the night. 📎

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	Description is comprehensive, covering summary, rationale, API surface, tests, and deferred work. However, required validation checklist items (cargo fmt, clippy, cargo build, manual testing) are not explicitly marked, and Security Impact / Reborn Trust-Boundary / Database Impact / Blast Radius sections are incomplete or missing detail.	Complete the Validation section checkboxes (at least mark cargo clippy and cargo test results), and fill in Security Impact, Reborn Trust-Boundary checklist (relevant for storage/permissions), and Blast Radius sections to meet template requirements.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title follows Conventional Commits style (type(scope): summary), is directly related to the PR's main change (new attachment landing crate), and clearly describes the core work.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Introduces a new leaf crate (ironclaw_attachments) that provides a single, channel-agnostic way to “land” inbound attachment bytes into the agent-accessible project filesystem via ScopedFilesystem, returning a ScopedPath storage key to persist in attachment refs.

Changes:

• Added ironclaw_attachments crate with path construction, sanitization, and landing/write routine.
• Implemented unit + async tests covering sanitization, path containment, write/read round-trip, permission-denied fail-closed behavior, and size limiting.
• Registered the new crate in the workspace (Cargo.toml) and lockfile (Cargo.lock).

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/ironclaw_attachments/src/lib.rs	New crate root exposing the attachment landing API surface.
crates/ironclaw_attachments/src/landing.rs	Core implementation: segment sanitization, scoped path building, bounded write via ScopedFilesystem, plus tests.
crates/ironclaw_attachments/Cargo.toml	New crate manifest and dependencies.
Cargo.toml	Adds crates/ironclaw_attachments to workspace members.
Cargo.lock	Adds the new crate entry and dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ilblackdragon]

The PR description was stale — sanitize_attachment_segment is intentionally internal, not public API. Callers land through land_attachment, which sanitizes for them; exposing the raw sanitizer would invite callers to pre-sanitize and double-transform. Updated the description to list only the exported surface and to mark this helper as internal, rather than making it pub.

[ilblackdragon]

Right — and the fix is to keep it internal rather than re-export it. land_attachment/attachment_scoped_path are the public entry points and they sanitize internally, so there is no reason for a downstream caller to reach the raw segment sanitizer. Corrected the PR description to match the actual (intentional) exported surface.

[ilblackdragon]

Reconciled the description: land_attachment(..., bytes, max_bytes) is the intended signature (the max_bytes bound was added in this PR to reject oversized uploads before any write), and DEFAULT_MAX_ATTACHMENT_BYTES (25 MiB) is exported as the default callers may pass or tighten. I kept max_bytes explicit rather than adding a defaulting wrapper so every call site states its bound — a silent default is exactly what let the unbounded-write gap exist. Description now shows the full signature.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_attachments/src/landing.rs`:
- Around line 64-81: sanitize_attachment_segment currently produces a lossy,
non-unique string which can collide (e.g., "a/b" -> "a_b"), so change the key to
include a short deterministic digest of the original raw input; update
sanitize_attachment_segment (and any callers like attachment_scoped_path) to
produce a readable sanitized base (keep current char-filter + trim logic,
falling back to "attachment" when empty) and append a fixed-length hex digest
(e.g., first 8-12 chars of SHA-256) computed from the raw inputs that affect
identity (message_id and original filename) separated by a delimiter like "--"
or "_" so the stored key remains human-friendly but collision-resistant and
stable across runs; ensure the digest is computed from the raw un-sanitized
strings and that the final returned string is valid for existing storage
constraints.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 977ddf5f-baf0-42b9-8e38-4e51280e8cad

📥 Commits

Reviewing files that changed from the base of the PR and between c9edbfe and 5249ebc.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (4)

• Cargo.toml
• crates/ironclaw_attachments/Cargo.toml
• crates/ironclaw_attachments/src/landing.rs
• crates/ironclaw_attachments/src/lib.rs