feat(attachments): bridge inbound bytes into transcript AttachmentRefs (#4644)

[ilblackdragon]

Summary

Connects Track 6 (byte landing, #4668) to Track 2 (transcript AttachmentRef, #4655): the reusable unit the byte-bearing ingress layer calls to turn raw inbound attachments into the durable AttachmentRefs the Reborn transcript persists — with storage_key set to the landed ScopedPath. This is the "connect land_attachment → AttachmentRef.storage_key" step.

Scope: crates/ only, no src/ changes.

What changed

• land_inbound_attachments(filesystem, scope, project_alias, date, message_id, Vec<InboundAttachment>, max_bytes) -> Vec<AttachmentRef> — lands each attachment's bytes through the project-scoped filesystem authority and builds the AttachmentRef with storage_key = <landed ScopedPath> and size_bytes = <landed byte count>. Each item is bounded by max_bytes (over-limit fails the batch with TooLarge before any write); extracted_text is left None (extraction/transcription is a later pipeline stage). Read-only project mounts fail closed.
• InboundAttachment input carries id/mime_type/filename + raw bytes. The attachment kind and fallback extension are derived from mime_type through the ironclaw_common format registry (the authoritative source), so a caller cannot drift them out of sync with the MIME type. kind reuses ironclaw_common::AttachmentKind (re-exported by ironclaw_threads), so the whole pipeline speaks one kind vocabulary.
• attachment_scoped_path now folds the per-attachment index into the path ({message_id}-{index}-{filename}) so multiple attachments on one message land at distinct paths even when they share a filename. The single-file Track 6 primitive didn't need this; the batch bridge does. (Track 6's three path tests are updated accordingly.)
• AttachmentRef now lives in ironclaw_common (next to AttachmentKind/IncomingAttachment), with ironclaw_threads re-exporting it — the public path ironclaw_threads::AttachmentRef is unchanged. So the bridge's only new dep edge is ironclaw_attachments → ironclaw_common (a leaf crate), not an upward edge on the ironclaw_threads transcript-contract crate. ironclaw_attachments stays a clean leaf (filesystem + host_api + common).

Why here, not at inbound_turn.rs

I traced the product-workflow inbound point (inbound_turn.rs:376, where MessageContent::text(payload.text) is built): it has only bytes-free ProductAttachmentDescriptors and no filesystem handle (ironclaw_product_workflow doesn't depend on ironclaw_filesystem), so land_attachment cannot run there. The byte-bearing layer is the ingress; this is the reusable unit it calls. Wiring the ingress upload path to invoke this bridge — and thread a project-scoped ScopedFilesystem with the authenticated caller's scope — is the next slice (host-runtime plumbing).

Tests

6 bridge tests (16 total in the crate, all passing): batch landing sets storage_key/size/kind/mime/filename on each ref with extracted_text None and the bytes addressable at each storage_key via the same authority; same-filename attachments land at distinct paths; filename: None lands a synthesized name and keeps the ref's filename None; a later-item failure fails the batch and leaves earlier bytes landed (no ref); an over-max_bytes item fails the batch with TooLarge and lands nothing; read-only project mount fails closed.

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

Stacking

Based on fix/4644-track6-attachment-landing (#4668). Part of the #4644 epic stack (registry → transcript refs → byte landing → this bridge).

Next slices

• Ingress upload path: accept base64 uploads, build InboundAttachments, obtain a project-scoped ScopedFilesystem for the caller, call this bridge, and submit the turn with MessageContent::with_attachments(text, refs).
• Track 3 model-visibility: read bytes back from storage_key → ChatMessage.content_parts (images); run extraction/transcription to fill extracted_text; emit model-facing project_path; fix chat_workflow.rs's [non_text_content] drop.
• Per-attachment memory index note; sandbox e2e; WASM store_attachment_data convergence.

Summary by CodeRabbit

• New Features

  • Inbound attachments can be uploaded and persisted with tracked metadata (size, storage location, MIME-based classification); batch semantics handle partial successes and oversize rejection.

• Refactor

  • Unified attachment reference type exposed across crates for a consistent public API and simplified attachment handling.

[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): bridge inbound bytes into transcript AttachmentRefs (#4644)

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

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

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

Sev	Conf	Reviewer	Location	Finding
Medium	80%	tests	inbound.rs:73	No test for partial failure mid-batch (succeed-then-fail across the Vec)
Low	65%	tests	inbound.rs:200	Fallback-filename path (filename = None) is never exercised through the bridge

---

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

[abbyshekit]

[Medium · 80% confidence] No test for partial failure mid-batch (succeed-then-fail across the Vec)

tests reviewer

land_inbound_attachments lands attachments in a loop and early-returns on the first failure via land_attachment(...).await?. When attachment[0] lands successfully and attachment[N>0] fails, the half-built refs vector is dropped and the whole call returns Err — so the bytes for att-0 are physically landed in the filesystem but NO AttachmentRef is ever returned for them (dangling bytes / silent data loss from the caller's perspective). The doc comment asserts exactly this behavior ("On the first failure the whole batch returns the error; partial writes that may already have landed are left in place"), but no test exercises it. The only failure test, fails_closed_on_read_only_project_mount, uses a SINGLE attachment on a fully read-only mount, so every write fails on item 0 — it never reaches the succeed-then-fail boundary that the focus area is about. A per-item failure is constructible against InMemoryBackend (e.g. pre-seed a conflicting entry to force FilesystemError::IndexConflict on the second write, as in in_memory.rs conflicting kind on same name fails), so this is testable. The test should assert the return is Err AND document the dangling-bytes/no-ref outcome (or, if the intent is all-or-nothing with rollback, assert the landed-then-failed bytes were cleaned up).

Fix: Add tests::inbound::partial_failure_drops_already_landed_refs covering a multi-attachment batch where item 0 lands and item 1 fails (force via a pre-seeded conflicting entry / IndexConflict on item 1's path): assert the call returns Err and assert the observable state of item 0's already-landed bytes (present-but-unreferenced, documenting the data-loss boundary, or cleaned up if rollback is intended). Add a mirror case with the FAILING item first to confirm no ref is produced when nothing succeeds.

#[tokio::test]
async fn returns_err_and_drops_already_landed_refs_when_a_later_item_fails() {
    let backend = Arc::new(InMemoryBackend::new());
    // Seed a conflicting entry at att-1's resolved path so its write fails
    // (IndexConflict) while att-0 lands cleanly.
    let writer = project_mount(Arc::clone(&backend), MountPermissions::read_write());
    let err = land_inbound_attachments(&writer, &test_scope(), DEFAULT_PROJECT_MOUNT_ALIAS,
        "2026-06-09", "msg1", vec![
            inbound("att-0", AttachmentKind::Document, "text/plain", "a.txt", b"ok"),
            inbound("att-1", AttachmentKind::Document, "text/plain", "b.txt", b"boom"),
        ]).await.expect_err("a later landing failure fails the whole batch");
    assert!(matches!(err, AttachmentLandingError::Write(_)));
    // Document the data-loss boundary: att-0 bytes landed but no ref returned.
}

[ilblackdragon]

Addressed on this branch (commit 559a1c46f). later_item_failure_fails_the_batch_and_leaves_earlier_bytes_landed forces att-1's write to fail (by seeding a child under its computed path so the backend rejects writing a file where a directory exists), lands att-0 first, and asserts: the batch returns Err(Write(_)), and att-0's bytes remain addressable at its storage path — documenting exactly the dangling-bytes / no-ref boundary the rustdoc promises (all-or-nothing rollback is explicitly not the contract; a retry re-lands at the same deterministic paths). The pre-existing fails_closed_on_read_only_project_mount covers the fail-on-item-0 case.

[abbyshekit]

[Low · 65% confidence] Fallback-filename path (filename = None) is never exercised through the bridge

tests reviewer

InboundAttachment carries fallback_extension, used only when filename is None to synthesize the landed filename. The inbound() test helper always sets filename: Some(...), so every inbound.rs test lands a named file; the fallback_extension field of land_inbound_attachments is never exercised. The synthesis logic itself is unit-tested in landing.rs (scoped_path_synthesizes_filename_when_absent), but the bridge that wires InboundAttachment.fallback_extension -> AttachmentLanding.fallback_extension -> storage_key is not — a swapped/dropped field assignment in the InboundAttachment destructuring would compile and pass all current tests. Confirm the synthesized filename flows into AttachmentRef.storage_key and that AttachmentRef.filename stays None.

Fix: Add tests::inbound::lands_with_synthesized_filename_when_filename_absent: build an InboundAttachment with filename=None and fallback_extension="png" at a known index, then assert storage_key ends with msg1-{index}-attachment-{index+1}.png and that the returned AttachmentRef.filename is None.

#[tokio::test]
async fn lands_with_synthesized_filename_when_filename_absent() {
    let backend = Arc::new(InMemoryBackend::new());
    let writer = project_mount(backend, MountPermissions::read_write());
    let att = InboundAttachment { id: "att-0".into(), kind: AttachmentKind::Image,
        mime_type: "image/png".into(), filename: None,
        fallback_extension: "png".into(), bytes: vec![0x89,0x50] };
    let refs = land_inbound_attachments(&writer, &test_scope(),
        DEFAULT_PROJECT_MOUNT_ALIAS, "2026-06-09", "msg1", vec![att])
        .await.expect("lands");
    assert_eq!(refs[0].storage_key.as_deref(),
        Some("/workspace/attachments/2026-06-09/msg1-0-attachment-1.png"));
    assert!(refs[0].filename.is_none());
}

[ilblackdragon]

Addressed on this branch (commit 559a1c46f). lands_with_synthesized_filename_when_filename_absent builds an InboundAttachment with filename: None and asserts the returned storage_key ends with msg1-1-attachment.png (1-based index; png derived from image/png via the registry) and that AttachmentRef.filename stays None — exercising the InboundAttachment.fallback_extension → AttachmentLanding.fallback_extension → storage_key wiring the named-file tests never reach.

[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: cd67d04e-ce64-4e4b-b802-8314e2c37ed6

📥 Commits

Reviewing files that changed from the base of the PR and between 8b005f8 and 65ed8db.

📒 Files selected for processing (1)

• crates/ironclaw_common/src/attachment.rs

---

📝 Walkthrough

Walkthrough

Consolidates AttachmentRef into ironclaw_common and introduces ironclaw_attachments::inbound::land_inbound_attachments to persist inbound bytes into AttachmentRef entries with storage_key and size metadata. No CLAUDE.md/.claude/AGENTS.md invariants appear violated.

Changes

Attachment Type Consolidation and Inbound Landing

Layer / File(s)	Summary
AttachmentRef shared type in ironclaw_common crates/ironclaw_common/src/attachment.rs, crates/ironclaw_common/src/lib.rs	New public AttachmentRef struct (id, kind, mime_type, optional filename/size/storage_key/extracted_text) with serde defaults; crate-root re-exports expanded to include AttachmentRef and attachment_format helpers.
Inbound attachment bridge module crates/ironclaw_attachments/src/inbound.rs	Adds InboundAttachment and land_inbound_attachments that classify by MIME, call per-item land_attachment via ScopedFilesystem, and return Vec<AttachmentRef> with storage_key and size_bytes. Includes tests for path uniqueness, read-only mounts, synthesized filenames, batch failure semantics, and oversize rejection.
Crate scaffolding and public API surface crates/ironclaw_attachments/Cargo.toml, crates/ironclaw_attachments/src/lib.rs	Introduces ironclaw_attachments Cargo manifest with path dependencies and dev-deps; lib.rs declares inbound module and re-exports InboundAttachment and land_inbound_attachments.
AttachmentRef migration in ironclaw_threads crates/ironclaw_threads/src/contract.rs, crates/ironclaw_threads/src/lib.rs	Removes local AttachmentRef definition, imports ironclaw_common::AttachmentRef, updates test imports for AttachmentKind, and consolidates public re-exports to expose AttachmentKind and AttachmentRef from ironclaw_common.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• nearai/ironclaw#4668: Adds the underlying land_attachment/landing plumbing that inbound::land_inbound_attachments delegates to.
• nearai/ironclaw#4655: Threads transcript changes that rely on the consolidated AttachmentRef type.
• nearai/ironclaw#4654: Introduces the attachment_format registry and MIME helper APIs consumed via ironclaw_common exports.

Poem

📎 Bytes arrive, then find their ground,
Paths and sizes in refs are found,
Shared type moved where all can see—
Landing safe, batch-wise, and free. 🚀

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title follows Conventional Commits style with type and scope; accurately summarizes the bridge connecting inbound bytes to AttachmentRefs.
Description check	✅ Passed	Description provides substantial context: What changed, Why here, Tests section, and Stacking info. Summary, Change Type, Linked Issue, and Validation are present but incomplete per template.
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

This PR bridges inbound attachment bytes into durable transcript AttachmentRefs by introducing a reusable batch landing routine in a new leaf crate (ironclaw_attachments) and moving AttachmentRef into ironclaw_common (while keeping ironclaw_threads::AttachmentRef available via re-export).

Changes:

• Add ironclaw_attachments::land_inbound_attachments to land each attachment through ScopedFilesystem and build AttachmentRef with storage_key + size_bytes.
• Move AttachmentRef definition to ironclaw_common and re-export it from ironclaw_threads to preserve the primary public path.
• Update attachment landing path construction to include the per-attachment index to avoid same-filename collisions.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/ironclaw_threads/src/lib.rs	Switches AttachmentRef exposure to a re-export from ironclaw_common.
crates/ironclaw_threads/src/contract.rs	Removes the local AttachmentRef definition and consumes ironclaw_common::AttachmentRef in the transcript contract.
crates/ironclaw_common/src/lib.rs	Re-exports AttachmentRef and attachment-format registry helpers at the crate root.
crates/ironclaw_common/src/attachment.rs	Introduces AttachmentRef as the durable, byte-free transcript projection type.
crates/ironclaw_attachments/src/lib.rs	New crate root exporting landing + inbound bridge modules.
crates/ironclaw_attachments/src/landing.rs	Implements path sanitization, scoped-path construction, and land_attachment write-through-authority routine with tests.
crates/ironclaw_attachments/src/inbound.rs	Implements InboundAttachment + land_inbound_attachments bridge producing AttachmentRefs, with tests for collision avoidance and fail-closed mounts.
crates/ironclaw_attachments/Cargo.toml	Defines the new leaf crate and its dependencies.
Cargo.toml	Adds crates/ironclaw_attachments to the workspace members list.
Cargo.lock	Records the new crate in the lockfile.

---

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

[ilblackdragon]

Fixed the PR description. InboundAttachment carries id/mime_type/filename + raw bytes; the kind and fallback extension are derived from mime_type via the ironclaw_common registry (so they cannot drift from the MIME type), not supplied as input fields. Description now matches the actual shape.

[coderabbitai]

Actionable comments posted: 3

🤖 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/inbound.rs`:
- Around line 52-65: The function land_inbound_attachments currently only
enforces per-item max_bytes and must also enforce hard limits on batch
cardinality and aggregate bytes to prevent unbounded memory/storage use: before
iterating, check attachments.len() against a new constant (e.g. MAX_BATCH_ITEMS)
and compute the sum of each inbound size (use the field carrying byte length
inside InboundAttachment) and ensure total <= a new MAX_TOTAL_BYTES (or a
provided parameter), returning an AttachmentLandingError variant (add one if
needed) when either limit is exceeded; keep the existing per-item max_bytes
check inside the loop but short-circuit earlier on batch/aggregate violations to
avoid allocating refs or starting work unnecessarily.

In `@crates/ironclaw_common/src/attachment.rs`:
- Around line 97-99: The AttachmentRef struct's id field currently uses a raw
String; replace it with a validated newtype (e.g., AttachmentId) or justify
keeping String in a doc comment. Create an AttachmentId newtype (tuple struct or
opaque type) with parsing/validation (FromStr/TryFrom<&str>) and update
AttachmentRef to pub id: AttachmentId, adjusting any constructors, serializers
(Serialize/Deserialize) and usages to convert between String and AttachmentId;
alternatively, if the ID is truly opaque and unvalidated, add a clear doc
comment on AttachmentRef::id explaining that channel-provided opaque IDs require
no validation and thus String is intentionally used. Ensure all references to
AttachmentRef::id and any serde impls are updated accordingly.
- Around line 102-104: The field pub mime_type: String on the Attachment struct
is accepting arbitrary strings despite the doc stating it’s validated; introduce
a MimeType newtype (e.g., struct MimeType(String)) with a TryFrom<String>
(and/or FromStr) impl that performs the attachment format registry
lookup/validation and returns an error on mismatch, change the
Attachment.mime_type type to MimeType, and update all call sites/constructors to
use MimeType::try_from (or str::parse) so the validation is enforced at
construction time; also add unit tests for MimeType::try_from and adjust any
serialization/deserialization impls (serde) to map through the newtype.

🪄 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: 0e1655ea-f30d-40c9-8618-4c5ffad1c57d

📥 Commits

Reviewing files that changed from the base of the PR and between 3a21130 and af3acc6.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (9)

• Cargo.toml
• crates/ironclaw_attachments/Cargo.toml
• crates/ironclaw_attachments/src/inbound.rs
• crates/ironclaw_attachments/src/landing.rs
• crates/ironclaw_attachments/src/lib.rs
• crates/ironclaw_common/src/attachment.rs
• crates/ironclaw_common/src/lib.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/lib.rs

[coderabbitai]

♻️ Duplicate comments (1)

crates/ironclaw_attachments/src/inbound.rs (1)

52-104: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Batch cardinality and aggregate bytes unbounded.

attachments.len() and total bytes across the batch are uncapped. Per guidelines: "User-controlled inputs must not grow unbounded; apply hard size limits (entries + total bytes)." A caller can exhaust memory/storage in one request despite per-item max_bytes.

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f7102a2b-f172-453d-a7ee-69d36a948b99

📥 Commits

Reviewing files that changed from the base of the PR and between af3acc6 and 8b005f8.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (7)

• crates/ironclaw_attachments/Cargo.toml
• crates/ironclaw_attachments/src/inbound.rs
• crates/ironclaw_attachments/src/lib.rs
• crates/ironclaw_common/src/attachment.rs
• crates/ironclaw_common/src/lib.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/lib.rs

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.