feat(sync): MTP discovery and bounded Android browsing/transfer (tr-t3i) by jm2 · Pull Request #178 · jm2/tributary

jm2 · 2026-07-25T23:00:04Z

P3.2 / GitHub issue #8 follow-on. Add MTP-style discovery and
bounded browsing/transfer for typical Android devices without treating
host paths as portable device identity.

The new src/device/mtp/ module ships four pieces:

MtpDeviceId, MtpUsbDescriptor, and MtpDeviceVendor build the
portable device identity exclusively from a USB descriptor
(serial, vendor, product). A device whose serial changes across
sessions is a new device, never a relocated mount. The identity
string is prefixed and rejects empty serials, host-path
separators, and unknown vendors.
MtpTransport is the seam the binary wires a real backend
(libmtp / adb / raw USB) into. The in-memory transport
implementation is shipped in test_transport for unit tests.
MtpBrowser returns a bounded, in-order list of MtpObject
values. The browser carries a BrowseBudget (max entries + max
depth) and a list-children closure supplied by the transport;
it never opens a destination and never walks a host path.
MtpTransferPlanner turns the bounded browse into a transfer
plan. The plan emits an MTP-side stage list (open-session,
browse-storage, fetch-object), a list of staging writes (the
MTP handle and the staging relative path under a device-scoped
prefix), and the source-destination pairs the existing
TransferPlanner consumes. The planner refuses to admit objects
whose names contain path separators or that escape the device
prefix; it surfaces a HostPathLeaked error if a transport ever
smuggles a host path past the type system.

The portable identity rule is enforced at every boundary. A
device is addressed by MtpDeviceId (USB serial + vendor +
product). Its storage objects are addressed by MtpObjectHandle
(an integer the device allocated). The staging directory is
prefixed with the device id, and the destination is rebuilt
from the object parent chain; no component of any path in the
plan is host-path-shaped. The destination-side transfer still
flows through the existing TransferPlanner + MountedWriteAuthority
so atomic staged writes, conflict policies, capacity budgeting,
and rollback are all reused unchanged.

Validation: 1754 locked lib tests pass (36 new mtp + 1718 prior),
14 repository-metadata tests pass, strict Clippy in debug and
release is clean (-D warnings), Rust 1.95 in-development MSRV,
formatting and the doc-build gate stay green.

Depends on #175 (the mounted-filesystem transfer planner). The
refinery should merge #175 first; once that is on main, the only
diff between polecat/tr-t3i and main is the new
src/device/mtp/ submodule and the one-line module registration
in src/device/mod.rs.

BLOCKED for final live-device validation: the in-memory
transport stands in for a real libmtp / raw-USB backend. The
binary must wire a real MtpTransport implementation when one is
available; the MTP discovery, bounded browse, and bounded
transfer logic is the deliverable for this slice.

Summary by CodeRabbit

New Features
- Added MTP device discovery with portable device identity labeling, bounded object browsing, and a staged transfer planner.
- Introduced a mounted-filesystem transfer planner/executor with staged writes, conflict policies, atomic publishing when possible, progress callbacks, cancellation, and rollback.
- Added scoped mounted write authority with safe relative-path validation and robust staging/commit/rollback.
- Added never_cancelled() for cancellation observers.
Bug Fixes
- Safer failure behavior for lost sessions, invalid paths, capacity/budget limits, mount-state changes, and conflict/cancellation edge cases.

claude · 2026-07-25T23:00:19Z

Claude finished @jm2's task in 4m 28s —— View job

Reviewing PR #178

Gather context (read diff, changed files)
Understand request (code review requested)
Review src/device/mtp/ module (identity, browse, transport, planner)
Review src/device/transfer.rs and src/local/write_authority.rs (in progress, delegated)
Review smaller diffs (device/mod.rs, local/mod.rs, root_authority.rs, source_lifecycle.rs)
Post final review feedback

coderabbitai · 2026-07-25T23:00:20Z

📝 Walkthrough

Walkthrough

Adds retained mounted-filesystem write authority, staged transfer planning and execution with progress, cancellation, and rollback, plus MTP identity, transport, bounded browsing, and transfer-planning APIs.

Changes

Transfer and MTP capabilities

Layer / File(s)	Summary
Retained write authority `src/local/mod.rs`, `src/local/root_authority.rs`, `src/local/write_authority.rs`	Exports retained authority helpers and implements validated staged writes, conflict policies, atomic publication, directory operations, and rollback cleanup.
Generic transfer planning and execution `src/device/mod.rs`, `src/device/transfer.rs`, `src/source_lifecycle.rs`	Adds transfer requests, plans, stages, capacity checks, progress callbacks, cancellation handling, staged copying, commit tracking, and rollback execution.
MTP identity, transport, and browsing `src/device/mtp/mod.rs`, `src/device/mtp/identity.rs`, `src/device/mtp/transport.rs`, `src/device/mtp/browse.rs`	Defines descriptor-based device identities, transport sessions, storage/object types, bounded tree browsing, and in-memory transport tests.
MTP transfer planning `src/device/mtp/planner.rs`, `src/device/mtp/mod.rs`	Builds budgeted transfer plans from browsed MTP objects, validates object and destination paths, deduplicates handles, and records staging writes with device and storage identifiers.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MtpTransport
  participant MtpSession
  participant MtpBrowser
  participant MtpTransferPlanner
  participant TransferExecutor
  participant MountedWriteAuthority

  MtpTransport->>MtpSession: open_session(descriptor)
  MtpSession->>MtpBrowser: verify session
  MtpBrowser->>MtpTransferPlanner: bounded MTP objects
  MtpTransferPlanner->>TransferExecutor: validated transfer plan
  TransferExecutor->>MountedWriteAuthority: stage and commit writes
  MountedWriteAuthority-->>TransferExecutor: commit outcomes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: new MTP discovery plus bounded browsing and transfer support.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch polecat/tr-t3i

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

codacy-production · 2026-07-25T23:01:48Z

Not up to standards ⛔

🔴 Issues 3 high · 11 medium · 7 minor

Alerts:
⚠ 21 issues (≤ 0 issues of at least minor severity)

Results:
21 new issues

Category Results

Security 3 high

Complexity 11 medium
7 minor

View in Codacy

🟢 Metrics 442 complexity · 32 duplication

Metric Results

Complexity 442

Duplication 32

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

codacy-production

Pull Request Overview

This PR implements MTP-style discovery and a bounded transfer system for Android devices. However, it is currently not up to standards due to 21 new issues identified by automated analysis and several high-risk security flaws.

There are significant concerns regarding path traversal in the transfer planner and a TOCTOU vulnerability in the write authority layer. Furthermore, the transfer executor bypasses critical authority validation checks during the copy loop, which could lead to unsafe writes if the filesystem state changes during a transfer. While the core requirements for device identity and transfer planning are largely met, these security and architectural gaps must be addressed before merging.

About this PR

The system demonstrates a pattern of fragile path handling, including the use of characters (colons) illegal on Windows and manual string-based path joining. A more robust approach using the PathBuf API throughout the planning phase is recommended to ensure cross-platform compatibility.
The PR description states the changes are limited to an MTP submodule and a single line in mod.rs, yet the diff contains a substantial new transfer planner and executor subsystem. Please update the description to accurately reflect the scope of changes.

Test suggestions

Verify MtpDeviceId construction rejects empty serials and path separators.
Verify MtpBrowser respects max_entries and max_depth bounds during tree traversal.
Verify MtpTransferPlanner correctly reconstructs destination paths from the parent handle chain.
Verify MtpTransferPlanner enforces byte-count and file-count budgets.
Verify TransferExecutor performs atomic staged writes via rename and handles rollback correctly.
Verify conflict policies (Overwrite, Skip, Preserve) are correctly resolved during planning and execution.
Verify transfer cancellation stops in-flight copies and cleans up staged files.

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-25T23:02:02Z

+
+    fn unique(label: &str) -> PathBuf {
+        let path =
+            std::env::temp_dir().join(format!("tributary-transfer-{label}-{}", Uuid::new_v4()));


_{🔴 HIGH RISK}

Replace manual temporary directory construction with the tempfile crate for better security and reliability. Using std::env::temp_dir() can lead to predictable paths.

Suggested fix:

Suggested change

std::env::temp_dir().join(format!("tributary-transfer-{label}-{}", Uuid::new_v4()));

let path = tempfile::tempdir()?.into_path();

See Issue in Codacy

codacy-production · 2026-07-25T23:02:02Z

+fn staging_path_for(
+    object: &MtpObject,
+    request: &MtpTransferRequest,
+    name: &str,
+) -> Result<PathBuf, MtpPlanError> {
+    // The staging path is built from the device id, the storage id,
+    // and the object's parent chain. Host paths never appear here.
+    let mut components: Vec<String> = Vec::new();
+    let mut current = object.parent;
+    while let Some(handle) = current {
+        let parent_object = request
+            .objects
+            .iter()
+            .find(|candidate| candidate.handle == handle)
+            .ok_or_else(|| MtpPlanError::HostPathLeaked(format!("missing parent {handle}")))?;
+        components.push(parent_object.name.clone());
+        current = parent_object.parent;
+    }
+    components.reverse();
+    components.push(name.to_string());
+    let mut path = PathBuf::from(format!("device-{}", request.session.device_id()));
+    for component in components {
+        path.push(component);
+    }
+    Ok(path)
+}


_{🔴 HIGH RISK}

The path components retrieved from the MTP device handles are not sanitized, allowing for potential path traversal if a device returns '..' or '/' in a folder name. Sanitizing parent names using a relative-name helper is required before adding them to a PathBuf.

codacy-production · 2026-07-25T23:02:02Z

+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn execute_copy_file(


_{🟡 MEDIUM RISK}

The execute_copy_file function accepts 10 parameters, exceeding maintainability limits. Consider grouping progress-related parameters (bytes, total_bytes, stages) and the cancellation observer into a single 'TransferContext' struct.

See Issue in Codacy

codacy-production · 2026-07-25T23:02:02Z

+                    staged
+                        .staged_file()
+                        .write_all(&buffer[..read])
+                        .map_err(|error| TransferError::io("failed to write staged file", error))
+                        .map_err(io::Error::other)?;
+                    copied = copied.saturating_add(read as u64);


_{🟡 MEDIUM RISK}

The executor bypasses authority validation during the copy loop by writing directly to the file handle. Use staged.write_all(buffer) to ensure the mount is still valid for every chunk, preventing writes if the target is unmounted during transfer.

codacy-production · 2026-07-25T23:02:02Z

+#[cfg(unix)]
+fn create_exclusive_staged_file(path: &Path) -> io::Result<File> {
+    use rustix::fs::{Mode, OFlags};
+
+    let leaf = path.file_name().ok_or_else(|| {
+        io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "staged file path is missing a leaf",
+        )
+    })?;
+    let parent = path.parent().ok_or_else(|| {
+        io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "staged file path is missing a parent",
+        )
+    })?;
+    let parent_file = File::open(parent)?;
+    let descriptor = rustix::fs::openat(
+        &parent_file,
+        leaf,
+        OFlags::WRONLY
+            | OFlags::CREATE
+            | OFlags::EXCL
+            | OFlags::CLOEXEC
+            | OFlags::NOFOLLOW
+            | OFlags::NOCTTY,
+        Mode::from_bits_truncate(0o600),
+    )
+    .map_err(io::Error::from)?;
+    Ok(File::from(descriptor))
+}


_{🟡 MEDIUM RISK}

Opening the parent directory by path introduces a TOCTOU (Time-of-Check Time-of-Use) vulnerability. Use the directory handle already validated by the MountedRootAuthority to perform an openat call directly.

codacy-production · 2026-07-25T23:02:02Z

+    /// can never produce identical relative paths unless the device
+    /// serial is also identical.
+    #[allow(clippy::unused_self)]
+    pub fn plan(&self, request: &MtpTransferRequest) -> Result<MtpTransferPlan, MtpPlanError> {


_{⚪ LOW RISK}

The plan method has high cyclomatic complexity (16). Simplify this by extracting the sorting logic and the per-object staging loop into separate helper methods.

See Issue in Codacy
See Issue in Codacy

codacy-production · 2026-07-25T23:02:02Z

+        // First, sort objects by their depth-first walk so the staging
+        // directory mirrors the on-device tree.
+        let mut ordered = request.objects.clone();
+        ordered.sort_by(|left, right| {


_{⚪ LOW RISK}

Suggestion: Sorting by raw MTP handles here makes the transfer order deterministic but breaks the stable depth-first order provided by the Browser. Since MTP handles are opaque and not guaranteed to be sequential, this sort should be removed to maintain directory locality and expected traversal order.

P3.2 / GitHub issue #8 foundation. Add a generic mounted-filesystem transfer planner and executor that reuses the existing retained root/marker lease model. The new write authority builds on MountedRootAuthority: every byte is staged beneath the destination through a sibling temporary file opened with O_CREAT | O_EXCL | O_NOFOLLOW (and the Windows reparse-point attribute rejected), then committed by a single rename(2) / MoveFileExW replacement. The mount boundary, mount generation, parent chain, and root token are revalidated before and after every staging and commit, so a binder swap or remount between staging and commit produces a fail-closed error rather than a partial publish. The transfer module ships TransferPlanner (read-only planning, with per-item conflict resolution and capacity budgeting) and TransferExecutor (ordered stage execution with cooperative cancellation through CancellationObserver, per-stage progress reporting, and reverse-order rollback on failure). ConflictPolicy reuses the same Skip/Overwrite/Preserve/Fail variants as the underlying write authority; Preserve writes a disambiguated sibling only when the original name is already taken. Cancellation is cooperative between stages and inside each chunked copy. Added open_relative_directory, bind_root_directory, token(), and with_relative_file on MountedRootAuthority so the device module can open parents, validate the root, and pass cloned file handles into closures without exposing BoundFile. Added CancellationObserver::never_cancelled() so test paths and code that has no upstream source can construct a permanent observer without hand-wiring a watch channel. Files: - src/local/write_authority.rs: MountedWriteAuthority, ConflictPolicy, ConflictResolution, PreparedWriteTarget, CommitOutcome, MountedDirectory, plus nine focused regressions covering fresh write, Skip/Fail/Overwrite/Preserve policies, rollback, parent rejection, directory creation, multi-preserve disambiguation, and regular-file-only remove. - src/device/transfer.rs: TransferItem, Stage, TransferPlan, TransferRequest, TransferError, TransferPlanner, TransferExecutor, TransferProgress, TransferSummary, plus eleven focused regressions covering single-file plan, recursive directory plan, capacity budget, single-file copy, recursive copy, fail-reject, overwrite replace, skip-omit, empty plan, absolute-path reject, and per-stage + per-chunk progress reporting. - src/local/root_authority.rs: open_relative_directory, bind_root_directory, token(), with_relative_file. - src/source_lifecycle.rs: CancellationObserver::never_cancelled(). - src/local/mod.rs, src/device/mod.rs: wire new modules and clarify the device module's sync scope. Validation passes 1,718 locked debug tests (9 new write_authority + 11 new transfer + 1,698 prior), 14 repository-metadata tests, with strict Clippy in debug and release workspaces (-D warnings), Rust 1.92 all-target, formatting, and the doc-build gate green. MTP discovery, sync mapping, device-copy drop policy, attached-session recovery, and live acceptance remain separate follow-on records; this slice deliberately constructs no global state, no GTK dependency, and no retained sync schedule.

P3.2 / GitHub issue #8 follow-on. Add MTP-style discovery and bounded browsing/transfer for typical Android devices without treating host paths as portable device identity. The new src/device/mtp/ module ships four pieces: * MtpDeviceId, MtpUsbDescriptor, and MtpDeviceVendor build the portable device identity exclusively from a USB descriptor (serial, vendor, product). A device whose serial changes across sessions is a new device, never a relocated mount. The identity string is prefixed and rejects empty serials, host-path separators, and unknown vendors. * MtpTransport is the seam the binary wires a real backend (libmtp / adb / raw USB) into. The in-memory transport implementation is shipped in test_transport for unit tests. * MtpBrowser returns a bounded, in-order list of MtpObject values. The browser carries a BrowseBudget (max entries + max depth) and a list-children closure supplied by the transport; it never opens a destination and never walks a host path. * MtpTransferPlanner turns the bounded browse into a transfer plan. The plan emits an MTP-side stage list (open-session, browse-storage, fetch-object), a list of staging writes (the MTP handle and the staging relative path under a device-scoped prefix), and the source-destination pairs the existing TransferPlanner consumes. The planner refuses to admit objects whose names contain path separators or that escape the device prefix; it surfaces a HostPathLeaked error if a transport ever smuggles a host path past the type system. The portable identity rule is enforced at every boundary. A device is addressed by MtpDeviceId (USB serial + vendor + product). Its storage objects are addressed by MtpObjectHandle (an integer the device allocated). The staging directory is prefixed with the device id, and the destination is rebuilt from the object parent chain; no component of any path in the plan is host-path-shaped. The destination-side transfer still flows through the existing TransferPlanner + MountedWriteAuthority so atomic staged writes, conflict policies, capacity budgeting, and rollback are all reused unchanged. Files: - src/device/mod.rs: register the new mtp submodule. - src/device/mtp/mod.rs: public surface, MtpDeviceLabel, helpers. - src/device/mtp/identity.rs: USB descriptor, device id, vendor enumeration, identity tests. - src/device/mtp/transport.rs: MtpSession, MtpTransport trait, MtpStorageDescriptor, MtpObjectHandle, MtpObjectBytes, InMemoryMtpTransport and its tests. - src/device/mtp/browse.rs: MtpBrowser, BrowseBudget, MtpObject, MtpObjectKind, browse tests. - src/device/mtp/planner.rs: MtpTransferPlanner, MtpTransferRequest, MtpTransferPlan, MtpStagingWrite, MtpPlanError, planner tests. Validation: 1754 locked lib tests pass (36 new mtp + 1718 prior), 14 repository-metadata tests pass, strict Clippy in debug and release is clean (-D warnings), Rust 1.95 in-development MSRV, formatting and the doc-build gate stay green. BLOCKED for final live-device validation: the in-memory transport stands in for a real libmtp / raw-USB backend. The binary must wire a real MtpTransport implementation when one is available; the MTP discovery, bounded browse, and bounded transfer logic is the deliverable for this slice.

coderabbitai

Actionable comments posted: 13

🧹 Nitpick comments (17)

src/local/write_authority.rs (2)
73-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

lease_token is stored but never read.

Both PreparedWriteTarget::lease_token and MountedDirectory::lease_token are written at construction and never used — commit, rollback, and prepare_write_in_directory all go through the Arc<MountedRootAuthority> directly. Either validate the token against authority.token() before publish/delete (the intent documented on MountedRootAuthority::token), or drop the field.

Also applies to: 369-373
🤖 Prompt for 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.

In `@src/local/write_authority.rs` around lines 73 - 83, Remove the unused
lease_token fields from PreparedWriteTarget and MountedDirectory, and remove
their construction and propagation through prepare_write_in_directory and
related paths. Continue using Arc<MountedRootAuthority> directly in commit and
rollback without changing the existing authority behavior.
466-491: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Preserved-name search is O(n²) and unbounded to u32::MAX.

The full directory listing is scanned linearly for every candidate index, and the loop can iterate ~4 billion times in a pathological directory. A HashSet<String> lookup plus a modest attempt cap (then error out) keeps this bounded.
🤖 Prompt for 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.

In `@src/local/write_authority.rs` around lines 466 - 491, Update the
preserved-name search around the existing collection and candidate loop to store
names in a HashSet<String>, allowing constant-time collision checks instead of
rescanning existing for every candidate. Replace the unbounded 1..=u32::MAX
iteration with a modest fixed attempt limit, and return an appropriate error
when no available candidate is found within that limit; preserve the existing
relative-path construction and atomic staging-directory behavior.
src/device/transfer.rs (5)
895-895: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the leftover dead helper.

_unused_os_string is a debug artifact and the only consumer of the OsString import at Line 53; the PR description claims dead helpers were already removed.
♻️ Drop the helper and its import
-fn _unused_os_string(_value: OsString) {}
-
-use std::ffi::OsString;
🤖 Prompt for 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.

In `@src/device/transfer.rs` at line 895, Remove the dead `_unused_os_string`
helper from the transfer module and delete the now-unused `OsString` import. Do
not alter surrounding transfer logic.
323-431: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Planning re-validates both authorities per item and duplicates validate_request's empty-path checks.

Lines 334-340 repeat checks already performed by validate_request, and Lines 341-346 revalidate both mounts on every item — an O(items) syscall storm for large playlists. Hoisting validation out of the loop (or validating every N items) plus deleting the duplicated guard also addresses the flagged 103-line/complexity-18 warning on this method.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 323 - 431, The plan method redundantly
checks item paths and validates both authorities for every item. Keep
validate_request(request) as the single pre-loop validation, remove the
duplicated empty-path guard and per-item source/destination validate calls, and
leave the item-processing logic unchanged.
Source: Linters/SAST tools

566-594: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

ensure_ancestor_directory_stages takes a destination it never uses.

The parameter is discarded with let _ = destination; (Line 592), and ensure_parent_directories only forwards it. Drop the parameter from both to keep the planning helpers honest.

Also applies to: 610-628
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 566 - 594, Remove the unused destination
parameter from ensure_ancestor_directory_stages and delete the let _ =
destination discard. Update ensure_parent_directories and every call site to
stop forwarding the destination while preserving the existing directory staging
behavior.
660-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

destination_is_atomic does not answer the question it names.

It returns destination.validate().is_ok(), i.e. "authority still current", and the resulting Stage::atomic flag is then ignored by the executor (atomic: _). As written the field can read false for a perfectly atomic destination whose authority momentarily failed validation. Either drop the flag until cross-filesystem fallback exists, or compute it from the actual staging/destination filesystem identity.

Also applies to: 398-404
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 660 - 666, Remove the misleading
destination_is_atomic helper and the associated Stage::atomic field/assignments,
since atomicity is guaranteed for the supported same-filesystem staging path and
the executor ignores the flag. If the field must remain, derive it from actual
staging and destination filesystem identity rather than
destination.validate().is_ok(), and ensure the executor uses the resulting
value.
802-866: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Typed TransferErrors are flattened into io::Error::other and re-wrapped as TransferError::Io.

Every inner failure is converted to a string-carrying io::Error just to satisfy with_relative_file's io::Result closure, then re-wrapped at Line 862, so callers can no longer match ConflictRejected, CommitFailed, or AuthorityLost. Also note the Interrupted sentinel at Line 858 collides with a genuine EINTR. Consider capturing the typed error in an Option<TransferError> (or a local enum) outside the closure and returning a placeholder io::Error, then restoring the real variant.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 802 - 866, Preserve typed transfer
failures in the source-copy flow instead of converting them into io::Error
messages. Update the closure passed to with_relative_file to capture the
original TransferError in an outer Option or local error enum, return only a
placeholder io::Error as required by the closure signature, and restore the
captured variant after source_result handling. Replace the Interrupted sentinel
path so genuine io::ErrorKind::Interrupted values remain distinguishable from
TransferError::Cancelled, while preserving the existing rollback and error
context.
src/device/mtp/browse.rs (4)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead conditional in the depth increment.

Children are only pushed when budget.allows_recursion() is true, so u32::from(budget.allows_recursion()) is always 1 on the path where next_depth is used. depth.saturating_add(1) says the same thing without implying a second behaviour.
🤖 Prompt for 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.

In `@src/device/mtp/browse.rs` at line 108, Update the next_depth calculation in
the browse traversal to use depth.saturating_add(1) directly, removing the
redundant budget.allows_recursion() conditional conversion while preserving the
existing child-push behavior.
250-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test helper ignores its parent filter cost and derives kind from payload emptiness.

build_list_children decides MtpObjectKind::Folder purely from bytes.is_empty(), so a legitimately empty file in a future test silently becomes a folder and gets descended into. Reading kind from the stored InMemoryObject would make the helper faithful to the transport data.
🤖 Prompt for 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.

In `@src/device/mtp/browse.rs` around lines 250 - 304, Update build_list_children
to derive each object's MtpObjectKind from the corresponding stored
InMemoryObject metadata rather than bytes.is_empty(). Preserve the existing
fetch behavior and parent filtering, while ensuring legitimately empty files
remain regular files.
73-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Documented ordering does not match the traversal.

pending.pop() makes the walk LIFO, so siblings come back in reverse listing order and subtrees interleave with later siblings. "In the order the tree was walked" is technically true but reads as listing order; state that the order is unspecified, or use a VecDeque/reversed push to get stable order.
🤖 Prompt for 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.

In `@src/device/mtp/browse.rs` around lines 73 - 77, Update the browser traversal
documentation to state that the returned entry order is unspecified, since the
LIFO pending stack reverses sibling listing order and affects subtree ordering.
Keep the existing parent-first, depth-first traversal and planner parent-chain
behavior unchanged.
92-95: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

by_handle duplicates visited and forces a clone per child.

Dedup is by handle only, and every child is cloned into the map even though the map is never read. A BTreeSet<MtpObjectHandle> of emitted handles avoids holding a second full copy of the tree.

Also applies to: 113-115
🤖 Prompt for 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.

In `@src/device/mtp/browse.rs` around lines 92 - 95, Remove the unused by_handle
BTreeMap from the browse traversal and its child insertion logic. Use the
existing visited BTreeSet<MtpObjectHandle> as the sole deduplication and
emitted-handle tracking structure, avoiding MtpObject clones while preserving
traversal results.
src/device/mtp/identity.rs (1)
172-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant empty-serial check.

MtpDeviceLabel::from_descriptor (src/device/mtp/mod.rs Lines 77-81) already rejects empty/whitespace serials with the same error, and trimmed is otherwise unused. Dropping lines 173-178 keeps label admission as the single validation point, as the module docs claim.
🤖 Prompt for 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.

In `@src/device/mtp/identity.rs` around lines 172 - 185, Remove the redundant
trimmed serial validation and unused trimmed variable from
MtpDeviceIdentity::from_descriptor. Let MtpDeviceLabel::from_descriptor remain
the single validation point for empty or whitespace-only serials, preserving its
existing error behavior.
src/device/mtp/transport.rs (2)
246-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

open_sessions is written but never read.

Nothing queries this map, so it adds locking work and misleading state to the fake transport. Either drop it or use it to assert single-session semantics / detect use-after-close in tests.

Also applies to: 334-337
🤖 Prompt for 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.

In `@src/device/mtp/transport.rs` around lines 246 - 250, Remove the unused
open_sessions field from InMemoryState and update its initialization and all
write paths accordingly. Do not add replacement state unless the fake transport
uses it to enforce single-session semantics or detect use-after-close.
214-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Doc contradicts the visibility.

test_transport is declared pub, not pub(crate); the module is only gated by #[cfg(test)]. Fix the comment (or mark the module pub(crate)) so the stated contract matches.
🤖 Prompt for 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.

In `@src/device/mtp/transport.rs` around lines 214 - 218, Update the documentation
inside the test_transport module to accurately state its public visibility under
#[cfg(test)], or change the module declaration to pub(crate) if that is the
intended contract; ensure the declaration and comment agree.
src/device/mtp/planner.rs (3)
257-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Comment overstates the sort.

Sorting by (parent_handle, handle) groups siblings by numeric handle; it is not a depth-first walk and does not guarantee parents precede children (a child with a lower parent handle can sort ahead of its own parent). Reword, or sort by reconstructed depth if the staging layout depends on it.
🤖 Prompt for 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.

In `@src/device/mtp/planner.rs` around lines 257 - 266, Correct the misleading
depth-first comment above the `ordered.sort_by` call: either describe the actual
`(parent_handle, handle)` ordering and avoid claiming tree mirroring, or replace
the comparator with a reconstructed depth-first ordering that guarantees parents
precede children if staging depends on that behavior.
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silent clamp of max_chunk_bytes.

with_caps(.., 8) yields 1024, so callers cannot express a small chunk and get no signal. Either document the floor on the constructor or return an error for sub-minimum values.
🤖 Prompt for 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.

In `@src/device/mtp/planner.rs` around lines 93 - 99, Update the
Planner::with_caps constructor to avoid silently changing a caller-provided
max_chunk_bytes below 1024: either document the enforced minimum in the
constructor’s API documentation, or change the constructor contract to return an
error for sub-minimum values. Preserve the existing cap fields and behavior for
valid chunk sizes.
465-668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tests do not cover the ancestor-name path.

Every case uses a single-level object or a folder parent named "Music"; nothing exercises a parent named ".."/"a/b", a parent-chain cycle, or a non-UTF-8 name — the gaps behind the two path-construction findings above. Worth adding once those are fixed.
🤖 Prompt for 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.

In `@src/device/mtp/planner.rs` around lines 465 - 668, The planner tests lack
coverage for unsafe and unusual ancestor paths. Extend the tests around
MtpTransferPlanner::plan to include file objects whose parent chain contains
names like ".." or "a/b", a cyclic parent chain, and a non-UTF-8 name, asserting
the appropriate rejection or safe behavior for each case; use existing object
helpers and MtpPlanError variants where applicable.

🤖 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 `@src/device/mtp/browse.rs`:
- Around line 97-121: Enforce the max_entries limit within the child-processing
loop in browse, alongside the existing result insertion logic. Stop processing
children once result reaches budget.max_entries(), while preserving
deduplication and recursion behavior for entries admitted before the cap.

In `@src/device/mtp/identity.rs`:
- Around line 55-66: Update the constructor around the serial conversion and
validation to trim the serial once, reject the trimmed value when empty, and
store that normalized value in the returned descriptor. Preserve vendor and
product handling while ensuring MtpDeviceLabel::from_descriptor and
InMemoryMtpTransport::open_session observe the same serial.

In `@src/device/mtp/mod.rs`:
- Around line 50-53: Update the public planner re-export alongside
MtpTransferPlanner and related planner types to include MtpTransferPlan,
MtpStagingWrite, and MtpPlanError, allowing external callers to name the result
types returned by MtpTransferPlanner::plan and keeping rustdoc links valid.

In `@src/device/mtp/planner.rs`:
- Around line 307-314: Update the MtpStagingWrite construction in the planner to
remove the permanently empty bytes field, and remove the field from
MtpStagingWrite and any dependent executor logic. Use object_handle with the
session to fetch or write the object data where needed, preserving non-empty
file output.
- Around line 242-246: Update the staging-root validation in the planner to
reject both empty paths and paths for which is_dir() is false, returning
StagingRootMissing with the existing path. Preserve successful planning only
when request.staging_root refers to an existing directory.
- Around line 388-411: Replace the string-based path construction in the staging
path logic with PathBuf::push using each component’s OsStr, preserving non-UTF-8
names exactly. Build the destination root and staging components without
to_string_lossy(), join("/") or other separator assumptions, while retaining the
existing empty-staging validation and resulting path semantics.
- Around line 353-378: Update staging_path_for and the corresponding
destination_path_for component construction to validate every device-supplied
ancestor name through relative_name, not just the leaf name, while preserving
the existing escaped-path rejection behavior. Add a visited-handle set to the
ancestor walk and return the established MalformedResponse or HostPathLeaked
error when a parent handle repeats, preventing cyclic parent chains from looping
indefinitely.

In `@src/device/mtp/transport.rs`:
- Around line 103-112: Replace the unconditional `MtpSession::verify` label
check with transport-backed liveness verification, such as an
`MtpTransport::verify_session(&MtpSession)` hook that real backends implement
and callers use before `browse`, `list_storage`, `fetch_object`, and
`MtpTransferPlanner::plan`. Propagate backend failures as
`MtpTransportError::SessionLost`, and update the `verify` documentation to
describe the actual validation behavior.

In `@src/device/transfer.rs`:
- Around line 827-835: Update the copy loop to preserve authority revalidation
for every chunk: make the staged target mutable and replace the raw
staged_file().write_all call with PreparedWriteTarget::write_all via
staged.write_all. Keep the existing error mapping and progress accounting
unchanged.
- Around line 695-747: Update the stage execution loop around
execute_create_directory and execute_copy_file so every stage error, including
cancellation returned from execute_copy_file, invokes self.rollback(&mut
committed_files) before propagating the original failure. Preserve the existing
RollbackFailed mapping and ensure rollback errors are reported consistently
while retaining the original stage error when rollback succeeds.

In `@src/local/write_authority.rs`:
- Around line 519-549: Update prepare_write_relative_file and
create_exclusive_staged_file to retain and pass the authority-bound parent
directory handle instead of dropping it and reopening parent by path. Use that
descriptor as the openat directory, preserving leaf validation and exclusive
creation while ensuring the staged file remains within the validated directory.
- Around line 234-271: Update the commit publish path to honor
ConflictResolution:Fresh and ConflictResolution:Preserved without replacing an
existing destination, using an atomic no-replace operation and returning an
AlreadyExists error if the target appeared after preparation; apply this to both
the final destination and preserved sibling paths. Retain replacing rename only
for ConflictResolution::Overwrite, and ensure the commit logic—not just
prepare_write_relative_file’s exists checks—enforces Skip, Fail, and Preserve
semantics.

In `@src/source_lifecycle.rs`:
- Around line 461-466: Update SourceObserver::never_cancelled so the watch
channel’s sender remains alive for the observer’s lifetime, rather than being
dropped immediately. Store the sender alongside receiver in the returned Self,
or use an equivalent persistent ownership mechanism, while preserving
cancelled()’s never-completing behavior.

---

Nitpick comments:
In `@src/device/mtp/browse.rs`:
- Line 108: Update the next_depth calculation in the browse traversal to use
depth.saturating_add(1) directly, removing the redundant
budget.allows_recursion() conditional conversion while preserving the existing
child-push behavior.
- Around line 250-304: Update build_list_children to derive each object's
MtpObjectKind from the corresponding stored InMemoryObject metadata rather than
bytes.is_empty(). Preserve the existing fetch behavior and parent filtering,
while ensuring legitimately empty files remain regular files.
- Around line 73-77: Update the browser traversal documentation to state that
the returned entry order is unspecified, since the LIFO pending stack reverses
sibling listing order and affects subtree ordering. Keep the existing
parent-first, depth-first traversal and planner parent-chain behavior unchanged.
- Around line 92-95: Remove the unused by_handle BTreeMap from the browse
traversal and its child insertion logic. Use the existing visited
BTreeSet<MtpObjectHandle> as the sole deduplication and emitted-handle tracking
structure, avoiding MtpObject clones while preserving traversal results.

In `@src/device/mtp/identity.rs`:
- Around line 172-185: Remove the redundant trimmed serial validation and unused
trimmed variable from MtpDeviceIdentity::from_descriptor. Let
MtpDeviceLabel::from_descriptor remain the single validation point for empty or
whitespace-only serials, preserving its existing error behavior.

In `@src/device/mtp/planner.rs`:
- Around line 257-266: Correct the misleading depth-first comment above the
`ordered.sort_by` call: either describe the actual `(parent_handle, handle)`
ordering and avoid claiming tree mirroring, or replace the comparator with a
reconstructed depth-first ordering that guarantees parents precede children if
staging depends on that behavior.
- Around line 93-99: Update the Planner::with_caps constructor to avoid silently
changing a caller-provided max_chunk_bytes below 1024: either document the
enforced minimum in the constructor’s API documentation, or change the
constructor contract to return an error for sub-minimum values. Preserve the
existing cap fields and behavior for valid chunk sizes.
- Around line 465-668: The planner tests lack coverage for unsafe and unusual
ancestor paths. Extend the tests around MtpTransferPlanner::plan to include file
objects whose parent chain contains names like ".." or "a/b", a cyclic parent
chain, and a non-UTF-8 name, asserting the appropriate rejection or safe
behavior for each case; use existing object helpers and MtpPlanError variants
where applicable.

In `@src/device/mtp/transport.rs`:
- Around line 246-250: Remove the unused open_sessions field from InMemoryState
and update its initialization and all write paths accordingly. Do not add
replacement state unless the fake transport uses it to enforce single-session
semantics or detect use-after-close.
- Around line 214-218: Update the documentation inside the test_transport module
to accurately state its public visibility under #[cfg(test)], or change the
module declaration to pub(crate) if that is the intended contract; ensure the
declaration and comment agree.

In `@src/device/transfer.rs`:
- Line 895: Remove the dead `_unused_os_string` helper from the transfer module
and delete the now-unused `OsString` import. Do not alter surrounding transfer
logic.
- Around line 323-431: The plan method redundantly checks item paths and
validates both authorities for every item. Keep validate_request(request) as the
single pre-loop validation, remove the duplicated empty-path guard and per-item
source/destination validate calls, and leave the item-processing logic
unchanged.
- Around line 566-594: Remove the unused destination parameter from
ensure_ancestor_directory_stages and delete the let _ = destination discard.
Update ensure_parent_directories and every call site to stop forwarding the
destination while preserving the existing directory staging behavior.
- Around line 660-666: Remove the misleading destination_is_atomic helper and
the associated Stage::atomic field/assignments, since atomicity is guaranteed
for the supported same-filesystem staging path and the executor ignores the
flag. If the field must remain, derive it from actual staging and destination
filesystem identity rather than destination.validate().is_ok(), and ensure the
executor uses the resulting value.
- Around line 802-866: Preserve typed transfer failures in the source-copy flow
instead of converting them into io::Error messages. Update the closure passed to
with_relative_file to capture the original TransferError in an outer Option or
local error enum, return only a placeholder io::Error as required by the closure
signature, and restore the captured variant after source_result handling.
Replace the Interrupted sentinel path so genuine io::ErrorKind::Interrupted
values remain distinguishable from TransferError::Cancelled, while preserving
the existing rollback and error context.

In `@src/local/write_authority.rs`:
- Around line 73-83: Remove the unused lease_token fields from
PreparedWriteTarget and MountedDirectory, and remove their construction and
propagation through prepare_write_in_directory and related paths. Continue using
Arc<MountedRootAuthority> directly in commit and rollback without changing the
existing authority behavior.
- Around line 466-491: Update the preserved-name search around the existing
collection and candidate loop to store names in a HashSet<String>, allowing
constant-time collision checks instead of rescanning existing for every
candidate. Replace the unbounded 1..=u32::MAX iteration with a modest fixed
attempt limit, and return an appropriate error when no available candidate is
found within that limit; preserve the existing relative-path construction and
atomic staging-directory behavior.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7bd751d7-234d-49e8-b502-59c91668d378

📥 Commits

Reviewing files that changed from the base of the PR and between 68ed2b0 and ce09d0c.

📒 Files selected for processing (11)

src/device/mod.rs
src/device/mtp/browse.rs
src/device/mtp/identity.rs
src/device/mtp/mod.rs
src/device/mtp/planner.rs
src/device/mtp/transport.rs
src/device/transfer.rs
src/local/mod.rs
src/local/root_authority.rs
src/local/write_authority.rs
src/source_lifecycle.rs

coderabbitai · 2026-07-25T23:08:22Z

+        while let Some((handle, depth)) = pending.pop() {
+            if !visited.insert(handle) {
+                continue;
+            }
+            if result.len() as u64 >= budget.max_entries() {
+                break;
+            }
+            if depth > budget.max_depth() {
+                continue;
+            }
+            let children = list_children(session, handle)?;
+            let next_depth = depth.saturating_add(u32::from(budget.allows_recursion()));
+            for child in children {
+                let kind = child.kind;
+                let child_handle = child.handle;
+                let child_parent = child.parent;
+                if by_handle.insert(child_handle, child.clone()).is_none() {
+                    result.push(child);
+                }
+                if matches!(kind, MtpObjectKind::Folder) && budget.allows_recursion() {
+                    pending.push((child_handle, next_depth));
+                }
+                let _ = child_parent;
+            }
+        }


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

max_entries is not a hard bound.

The budget is only checked before listing a node's children, and the inner loop pushes every child unconditionally. With BrowseBudget::new(1, 4) and a root that lists 6 children, browse returns 6 entries. browse_respects_max_entries only passes because the budget equals the child count. Enforce the cap inside the child loop.

🐛 Proposed fix

for child in children { + if result.len() as u64 >= budget.max_entries() { + break; + } let kind = child.kind; let child_handle = child.handle; - let child_parent = child.parent; if by_handle.insert(child_handle, child.clone()).is_none() { result.push(child); } if matches!(kind, MtpObjectKind::Folder) && budget.allows_recursion() { pending.push((child_handle, next_depth)); } - let _ = child_parent; }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

while let Some((handle, depth)) = pending.pop() {

if !visited.insert(handle) {

continue;

}

if result.len() as u64 >= budget.max_entries() {

break;

}

if depth > budget.max_depth() {

continue;

}

let children = list_children(session, handle)?;

let next_depth = depth.saturating_add(u32::from(budget.allows_recursion()));

for child in children {

let kind = child.kind;

let child_handle = child.handle;

let child_parent = child.parent;

if by_handle.insert(child_handle, child.clone()).is_none() {

result.push(child);

}

if matches!(kind, MtpObjectKind::Folder) && budget.allows_recursion() {

pending.push((child_handle, next_depth));

}

let _ = child_parent;

}

}

while let Some((handle, depth)) = pending.pop() {

if !visited.insert(handle) {

continue;

}

if result.len() as u64 >= budget.max_entries() {

break;

}

if depth > budget.max_depth() {

continue;

}

let children = list_children(session, handle)?;

let next_depth = depth.saturating_add(u32::from(budget.allows_recursion()));

for child in children {

if result.len() as u64 >= budget.max_entries() {

break;

}

let kind = child.kind;

let child_handle = child.handle;

if by_handle.insert(child_handle, child.clone()).is_none() {

result.push(child);

}

if matches!(kind, MtpObjectKind::Folder) && budget.allows_recursion() {

pending.push((child_handle, next_depth));

}

}

}

🤖 Prompt for 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. In `@src/device/mtp/browse.rs` around lines 97 - 121, Enforce the max_entries limit within the child-processing loop in browse, alongside the existing result insertion logic. Stop processing children once result reaches budget.max_entries(), while preserving deduplication and recursion behavior for entries admitted before the cap.

coderabbitai · 2026-07-25T23:08:22Z

+        let serial = serial.into();
+        if serial.trim().is_empty() {
+            return Err(MtpTransportError::InvalidDescriptor(
+                "device serial is empty".to_string(),
+            ));
+        }
+        Ok(Self {
+            serial,
+            vendor,
+            product,
+        })
+    }


🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Store the trimmed serial so descriptors and labels agree.

new validates serial.trim() but stores the raw string. MtpDeviceLabel::from_descriptor trims, so " ABC123" and "ABC123" collapse to the same MtpDeviceId while remaining unequal MtpUsbDescriptors — and InMemoryMtpTransport::open_session matches on the raw serial, so a session lookup can fail for a descriptor that resolves to a valid id.

🔧 Proposed normalization

- let serial = serial.into(); - if serial.trim().is_empty() { + let serial: String = serial.into(); + let serial = serial.trim().to_string(); + if serial.is_empty() { return Err(MtpTransportError::InvalidDescriptor( "device serial is empty".to_string(), )); }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

let serial = serial.into();

if serial.trim().is_empty() {

return Err(MtpTransportError::InvalidDescriptor(

"device serial is empty".to_string(),

));

}

Ok(Self {

serial,

vendor,

product,

})

}

let serial: String = serial.into();

let serial = serial.trim().to_string();

if serial.is_empty() {

return Err(MtpTransportError::InvalidDescriptor(

"device serial is empty".to_string(),

));

}

Ok(Self {

serial,

vendor,

product,

})

}

🤖 Prompt for 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. In `@src/device/mtp/identity.rs` around lines 55 - 66, Update the constructor around the serial conversion and validation to trim the serial once, reject the trimmed value when empty, and store that normalized value in the returned descriptor. Preserve vendor and product handling while ensuring MtpDeviceLabel::from_descriptor and InMemoryMtpTransport::open_session observe the same serial.

coderabbitai · 2026-07-25T23:08:22Z

+#[allow(unused_imports)]
+pub use planner::{MtpTransferPlanner, MtpTransferRequest, MtpTransferStage, TransferBudget};
+#[allow(unused_imports)]
+pub use transport::{MtpSession, MtpTransport, MtpTransportError};


📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Re-export MtpTransferPlan, MtpStagingWrite, and MtpPlanError.

planner is a private module, so external callers can reach MtpTransferPlanner::plan but cannot name its Ok/Err types. The API is effectively unusable outside this module and rustdoc links will dangle.

♻️ Proposed re-export

#[allow(unused_imports)] -pub use planner::{MtpTransferPlanner, MtpTransferRequest, MtpTransferStage, TransferBudget}; +pub use planner::{ + MtpPlanError, MtpStagingWrite, MtpTransferPlan, MtpTransferPlanner, MtpTransferRequest, + MtpTransferStage, TransferBudget, +};

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

#[allow(unused_imports)]

pub use planner::{MtpTransferPlanner, MtpTransferRequest, MtpTransferStage, TransferBudget};

#[allow(unused_imports)]

pub use transport::{MtpSession, MtpTransport, MtpTransportError};

#[allow(unused_imports)]

pub use planner::{

MtpPlanError, MtpStagingWrite, MtpTransferPlan, MtpTransferPlanner, MtpTransferRequest,

MtpTransferStage, TransferBudget,

};

#[allow(unused_imports)]

pub use transport::{MtpSession, MtpTransport, MtpTransportError};

🤖 Prompt for 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. In `@src/device/mtp/mod.rs` around lines 50 - 53, Update the public planner re-export alongside MtpTransferPlanner and related planner types to include MtpTransferPlan, MtpStagingWrite, and MtpPlanError, allowing external callers to name the result types returned by MtpTransferPlanner::plan and keeping rustdoc links valid.

coderabbitai · 2026-07-25T23:08:22Z

+        if request.staging_root.as_os_str().is_empty() {
+            return Err(MtpPlanError::StagingRootMissing {
+                path: request.staging_root.clone(),
+            });
+        }


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

StagingRootMissing never checks that the root is a directory.

The variant's message says "is not a directory" but the guard only rejects an empty OsStr. A caller passing a nonexistent or file path reaches plan success and fails later during staging. Add the is_dir() check here (or reword the variant).

🔧 Proposed fix

- if request.staging_root.as_os_str().is_empty() { + if request.staging_root.as_os_str().is_empty() || !request.staging_root.is_dir() { return Err(MtpPlanError::StagingRootMissing { path: request.staging_root.clone(), }); }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

if request.staging_root.as_os_str().is_empty() {

return Err(MtpPlanError::StagingRootMissing {

path: request.staging_root.clone(),

});

}

if request.staging_root.as_os_str().is_empty() || !request.staging_root.is_dir() {

return Err(MtpPlanError::StagingRootMissing {

path: request.staging_root.clone(),

});

}

🤖 Prompt for 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. In `@src/device/mtp/planner.rs` around lines 242 - 246, Update the staging-root validation in the planner to reject both empty paths and paths for which is_dir() is false, returning StagingRootMissing with the existing path. Preserve successful planning only when request.staging_root refers to an existing directory.

coderabbitai · 2026-07-25T23:08:22Z

+            staging_writes.push(MtpStagingWrite {
+                device_id: request.session.device_id().clone(),
+                storage_id: request.storage.storage_id,
+                object_handle: object.handle,
+                bytes: Vec::new(),
+                staging_relative_path: staging_relative.clone(),
+                destination_relative_path: destination_relative.clone(),
+            });


🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

MtpStagingWrite::bytes is always empty.

The planner never fetches, so this field is a permanently empty Vec documented as "Bytes the planner will write". Any executor trusting it will write zero-length files. Drop the field (the executor has object_handle + the session) or make the planner responsible for populating it.

🤖 Prompt for 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. In `@src/device/mtp/planner.rs` around lines 307 - 314, Update the MtpStagingWrite construction in the planner to remove the permanently empty bytes field, and remove the field from MtpStagingWrite and any dependent executor logic. Use object_handle with the session to fetch or write the object data where needed, preserving non-empty file output.

coderabbitai · 2026-07-25T23:08:22Z

+        for (index, stage) in self.plan.stages().iter().enumerate() {
+            if cancellation.is_cancelled() {
+                self.rollback(&mut committed_files).map_err(|error| {
+                    TransferError::RollbackFailed {
+                        path: PathBuf::new(),
+                        context: error.to_string(),
+                    }
+                })?;
+                return Err(TransferError::Cancelled);
+            }
+            progress.on_stage_started(stage, index as u32, total_stages);
+            match stage {
+                Stage::CreateDirectory {
+                    destination_relative_path,
+                } => {
+                    self.execute_create_directory(destination_relative_path)?;
+                }
+                Stage::CopyFile {
+                    source_relative_path,
+                    destination_relative_path,
+                    bytes,
+                    atomic: _,
+                    conflict,
+                } => {
+                    let outcome = self.execute_copy_file(
+                        source_relative_path,
+                        destination_relative_path,
+                        *bytes,
+                        &mut bytes_so_far,
+                        total_bytes,
+                        index as u32,
+                        total_stages,
+                        progress,
+                        cancellation,
+                    )?;
+                    let _ = outcome;
+                    let _ = conflict;
+                    committed_files.push(destination_relative_path.clone());
+                }
+                Stage::RemoveFile { .. } => {
+                    // RemoveFile stages are inserted only by the rollback path
+                    // and never appear in a forward plan. Skip defensively.
+                }
+            }
+            committed_stages = committed_stages.saturating_add(1);
+            progress.on_stage_completed(
+                stage,
+                index as u32,
+                total_stages,
+                bytes_so_far,
+                total_bytes,
+            );
+        }


🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

Stage failures never roll back — only cancellation does.

The module docs (Lines 37-41) promise that "on cancellation or a failed stage, already-committed files are rolled back in reverse order", but execute_create_directory/execute_copy_file errors propagate with ? at Lines 710 and 719-729 without touching committed_files. A mid-plan I/O or commit failure leaves the destination half-published. The in-copy cancellation path (Line 816) has the same problem: it returns TransferError::Cancelled through ?, skipping the rollback branch entirely.

🐛 Route every stage error through rollback

- progress.on_stage_started(stage, index as u32, total_stages); - match stage { - Stage::CreateDirectory { - destination_relative_path, - } => { - self.execute_create_directory(destination_relative_path)?; - } - Stage::CopyFile { + progress.on_stage_started(stage, index as u32, total_stages); + let stage_result = match stage { + Stage::CreateDirectory { + destination_relative_path, + } => self.execute_create_directory(destination_relative_path), + Stage::CopyFile { source_relative_path, destination_relative_path, bytes, atomic: _, - conflict, + conflict: _, } => { - let outcome = self.execute_copy_file( + self.execute_copy_file( source_relative_path, destination_relative_path, *bytes, &mut bytes_so_far, total_bytes, index as u32, total_stages, progress, cancellation, - )?; - let _ = outcome; - let _ = conflict; - committed_files.push(destination_relative_path.clone()); + ) + .map(|_| committed_files.push(destination_relative_path.clone())) } Stage::RemoveFile { .. } => { // RemoveFile stages are inserted only by the rollback path // and never appear in a forward plan. Skip defensively. + Ok(()) } + }; + if let Err(error) = stage_result { + self.rollback(&mut committed_files).map_err(|rollback_error| { + TransferError::RollbackFailed { + path: PathBuf::new(), + context: rollback_error.to_string(), + } + })?; + return Err(error); }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

for (index, stage) in self.plan.stages().iter().enumerate() {

if cancellation.is_cancelled() {

self.rollback(&mut committed_files).map_err(|error| {

TransferError::RollbackFailed {

path: PathBuf::new(),

context: error.to_string(),

}

})?;

return Err(TransferError::Cancelled);

}

progress.on_stage_started(stage, index as u32, total_stages);

match stage {

Stage::CreateDirectory {

destination_relative_path,

} => {

self.execute_create_directory(destination_relative_path)?;

}

Stage::CopyFile {

source_relative_path,

destination_relative_path,

bytes,

atomic: _,

conflict,

} => {

let outcome = self.execute_copy_file(

source_relative_path,

destination_relative_path,

*bytes,

&mut bytes_so_far,

total_bytes,

index as u32,

total_stages,

progress,

cancellation,

)?;

let _ = outcome;

let _ = conflict;

committed_files.push(destination_relative_path.clone());

}

Stage::RemoveFile { .. } => {

// RemoveFile stages are inserted only by the rollback path

// and never appear in a forward plan. Skip defensively.

}

}

committed_stages = committed_stages.saturating_add(1);

progress.on_stage_completed(

stage,

index as u32,

total_stages,

bytes_so_far,

total_bytes,

);

}

for (index, stage) in self.plan.stages().iter().enumerate() {

if cancellation.is_cancelled() {

self.rollback(&mut committed_files).map_err(|error| {

TransferError::RollbackFailed {

path: PathBuf::new(),

context: error.to_string(),

}

})?;

return Err(TransferError::Cancelled);

}

progress.on_stage_started(stage, index as u32, total_stages);

let stage_result = match stage {

Stage::CreateDirectory {

destination_relative_path,

} => self.execute_create_directory(destination_relative_path),

Stage::CopyFile {

source_relative_path,

destination_relative_path,

bytes,

atomic: _,

conflict: _,

} => {

self.execute_copy_file(

source_relative_path,

destination_relative_path,

*bytes,

&mut bytes_so_far,

total_bytes,

index as u32,

total_stages,

progress,

cancellation,

)

.map(|_| committed_files.push(destination_relative_path.clone()))

}

Stage::RemoveFile { .. } => {

// RemoveFile stages are inserted only by the rollback path

// and never appear in a forward plan. Skip defensively.

Ok(())

}

};

if let Err(error) = stage_result {

self.rollback(&mut committed_files).map_err(|rollback_error| {

TransferError::RollbackFailed {

path: PathBuf::new(),

context: rollback_error.to_string(),

}

})?;

return Err(error);

}

committed_stages = committed_stages.saturating_add(1);

progress.on_stage_completed(

stage,

index as u32,

total_stages,

bytes_so_far,

total_bytes,

);

}

🤖 Prompt for 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. In `@src/device/transfer.rs` around lines 695 - 747, Update the stage execution loop around execute_create_directory and execute_copy_file so every stage error, including cancellation returned from execute_copy_file, invokes self.rollback(&mut committed_files) before propagating the original failure. Preserve the existing RollbackFailed mapping and ensure rollback errors are reported consistently while retaining the original stage error when rollback succeeds.

coderabbitai · 2026-07-25T23:08:23Z

+                    staged
+                        .staged_file()
+                        .write_all(&buffer[..read])
+                        .map_err(|error| TransferError::io("failed to write staged file", error))
+                        .map_err(io::Error::other)?;
+                    copied = copied.saturating_add(read as u64);
+                    *bytes_so_far = bytes_so_far.saturating_add(read as u64);
+                    progress.on_bytes_copied(stage_index, total_stages, *bytes_so_far, total_bytes);
+                }


🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Copy loop writes through &File, skipping the authority revalidation in PreparedWriteTarget::write_all.

staged.staged_file().write_all(..) uses the raw handle, so the boundary/mount-generation check that PreparedWriteTarget::write_all performs before and after each write never runs during the copy. That contradicts the module claim that both authorities are revalidated around every operation; a remount mid-copy is only noticed at commit. Make staged mutable and call staged.write_all(&buffer[..read]), or revalidate explicitly per chunk if per-chunk validate() is too costly.

🤖 Prompt for 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. In `@src/device/transfer.rs` around lines 827 - 835, Update the copy loop to preserve authority revalidation for every chunk: make the staged target mutable and replace the raw staged_file().write_all call with PreparedWriteTarget::write_all via staged.write_all. Keep the existing error mapping and progress accounting unchanged.

coderabbitai · 2026-07-25T23:08:23Z

+        let (resolution, final_relative, staged_dir) = match policy {
+            ConflictPolicy::Skip if final_path.exists() => {
+                return Err(io::Error::new(
+                    io::ErrorKind::AlreadyExists,
+                    "destination exists and policy is Skip",
+                ));
+            }
+            ConflictPolicy::Fail if final_path.exists() => {
+                return Err(io::Error::new(
+                    io::ErrorKind::AlreadyExists,
+                    "destination exists and policy is Fail",
+                ));
+            }
+            ConflictPolicy::Skip | ConflictPolicy::Fail => {
+                (ConflictResolution::Fresh, final_relative, parent_components)
+            }
+            ConflictPolicy::Overwrite => (
+                ConflictResolution::Overwrite,
+                final_relative,
+                parent_components,
+            ),
+            ConflictPolicy::Preserve => {
+                if final_path.exists() {
+                    let (preserved_rel, preserved_components) = preserved_sibling_path(
+                        self.mounted.root(),
+                        &parent_components,
+                        components.last().expect("non-empty"),
+                    )?;
+                    (
+                        ConflictResolution::Preserved,
+                        preserved_rel,
+                        preserved_components,
+                    )
+                } else {
+                    (ConflictResolution::Fresh, final_relative, parent_components)
+                }
+            }
+        };


🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Skip/Fail/Preserve resolutions are decided at prepare time but published with a replacing rename — TOCTOU can silently destroy a file.

final_path.exists() is checked in prepare_write_relative_file, while commit publishes via std::fs::rename, which unconditionally replaces an existing destination. If the destination (or the chosen preserved sibling name) appears between prepare and commit, Skip/Fail semantics are violated and the intervening file is overwritten with no error. Re-check under a non-replacing publish for the Fresh/Preserved resolutions (e.g. renameat2 with RENAME_NOREPLACE, link+unlink fallback) and only allow replacing rename for ConflictResolution::Overwrite.

Also applies to: 127-137

🤖 Prompt for 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. In `@src/local/write_authority.rs` around lines 234 - 271, Update the commit publish path to honor ConflictResolution:Fresh and ConflictResolution:Preserved without replacing an existing destination, using an atomic no-replace operation and returning an AlreadyExists error if the target appeared after preparation; apply this to both the final destination and preserved sibling paths. Retain replacing rename only for ConflictResolution::Overwrite, and ensure the commit logic—not just prepare_write_relative_file’s exists checks—enforces Skip, Fail, and Preserve semantics.

coderabbitai · 2026-07-25T23:08:23Z

+#[cfg(unix)]
+fn create_exclusive_staged_file(path: &Path) -> io::Result<File> {
+    use rustix::fs::{Mode, OFlags};
+
+    let leaf = path.file_name().ok_or_else(|| {
+        io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "staged file path is missing a leaf",
+        )
+    })?;
+    let parent = path.parent().ok_or_else(|| {
+        io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "staged file path is missing a parent",
+        )
+    })?;
+    let parent_file = File::open(parent)?;
+    let descriptor = rustix::fs::openat(
+        &parent_file,
+        leaf,
+        OFlags::WRONLY
+            | OFlags::CREATE
+            | OFlags::EXCL
+            | OFlags::CLOEXEC
+            | OFlags::NOFOLLOW
+            | OFlags::NOCTTY,
+        Mode::from_bits_truncate(0o600),
+    )
+    .map_err(io::Error::from)?;
+    Ok(File::from(descriptor))
+}


🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Staged file is created through a re-resolved parent path, bypassing the authority-bound directory handle.

prepare_write_relative_file opens the parent through the retained authority (Lines 228-232) but immediately drops that bound handle, then create_exclusive_staged_file re-opens the parent with File::open(parent) by absolute path. The openat/O_NOFOLLOW protection then only covers the leaf: the parent path is resolved a second time and can be swapped (symlink/bind) between the boundary check and the openat, so the staged file may land outside the validated mount. Thread the bound parent directory's descriptor into create_exclusive_staged_file instead of reopening by path.

Also applies to: 225-232

🤖 Prompt for 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. In `@src/local/write_authority.rs` around lines 519 - 549, Update prepare_write_relative_file and create_exclusive_staged_file to retain and pass the authority-bound parent directory handle instead of dropping it and reopening parent by path. Use that descriptor as the openat directory, preserving leaf validation and exclusive creation while ensuring the staged file remains within the validated directory.

coderabbitai · 2026-07-25T23:08:23Z

+    /// Construct a never-cancelled observer. Useful for tests and for code
+    /// paths that accept a cancellation handle but have no upstream source.
+    pub fn never_cancelled() -> Self {
+        let (_sender, receiver) = watch::channel(false);
+        Self { receiver }
+    }


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

tokio watch Receiver changed error when all senders dropped

💡 Result:

In the tokio::sync::watch module, a RecvError occurs when a Receiver attempts to detect changes after the channel has been closed [1][2]. A channel is considered closed when all Senders have been dropped [2][3]. However, the specific conditions under which this error is returned depend on whether you are using the changed or has_changed method [2][4]. The behavioral differences are as follows: 1. changed: This asynchronous method returns a RecvError if and only if the channel is closed AND the current value has already been marked as seen [1][4]. If there is an unseen value in the channel, it will return that value (marking it as seen) instead of returning an error, even if the channel is closed [1][2]. 2. has_changed: This synchronous method returns a RecvError if and only if the channel is closed [1][4]. Unlike changed, it does not require the current value to be seen to trigger the error [2]. If the channel is closed and you call changed, you will only receive an error once all pending notifications (the "unseen" values) have been consumed [2]. The RecvError itself indicates that the channel is closed and no further messages can be sent [1].

Citations:

1: https://docs.rs/tokio/latest/tokio/sync/watch/struct.Receiver.html

2: https://docs.rs/tokio/latest/tokio/sync/watch/

3: https://docs.rs/tokio/latest/tokio/sync/watch/struct.Sender.html

4: https://docs.rs/tokio/latest/tokio/sync/watch/index.html

🏁 Script executed:

sed -n '430,520p' src/source_lifecycle.rs

Repository: jm2/tributary

Length of output: 2482

🏁 Script executed:

rg -n "never_cancelled\(\)|CancellationObserver" src

Repository: jm2/tributary

Length of output: 3101

🌐 Web query:

tokio watch Receiver created from channel initial value seen changed await closed immediately

💡 Result:

In a tokio::sync::watch channel, the initial value (or the value present at the time a new Receiver is created via subscribe) is considered "seen" by default [1][2][3]. Because of this, calling Receiver::changed.await immediately after creating the receiver will not return; instead, it will asynchronously wait until a subsequent value is sent or the sender is dropped [1][2][3]. The changed method only returns immediately if there is an "unseen" value currently stored in the channel [1][2]. Since the initial value is marked as seen upon receiver creation, the receiver does not perceive it as a new, pending change [1][2]. If your goal is to process the initial value immediately upon starting, the idiomatic approach is to access that value explicitly before entering the await loop [1][3]. You can use Receiver::borrow_and_update to get the current value and mark it as seen, ensuring that your subsequent call to changed.await will correctly wait for the next update [1][3]. Example pattern: loop { // Process the current value (initial or updated) println!("Value: {:?}", *rx.borrow_and_update); // Wait for the next update, or break if the sender is dropped if rx.changed.await.is_err { break; } }

Citations:

1: https://docs.rs/tokio/latest/tokio/sync/watch/index.html

2: https://docs.rs/tokio/latest/src/tokio/sync/watch.rs.html

3: https://docs.rs/tokio/1.44.1/tokio/sync/watch/index.html

Keep the sender alive for never_cancelled()
Dropping the sender closes the watch channel, so cancelled().await returns immediately here. Store the sender (for example in a OnceLock) or otherwise keep it alive so the helper actually stays “never cancelled”.

🤖 Prompt for 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. In `@src/source_lifecycle.rs` around lines 461 - 466, Update SourceObserver::never_cancelled so the watch channel’s sender remains alive for the observer’s lifetime, rather than being dropped immediately. Store the sender alongside receiver in the returned Self, or use an equivalent persistent ownership mechanism, while preserving cancelled()’s never-completing behavior.

claude · 2026-07-25T23:10:17Z

Claude finished @jm2's task in 3m 12s —— View job

Reviewing PR #178

Gather context (read diff, changed files, prior review comments from CodeRabbit/Codacy)
Understand request (code review requested)
Review src/device/mtp/ module (identity, browse, transport, planner)
Review src/device/transfer.rs and src/local/write_authority.rs
Review smaller diffs (device/mod.rs, local/mod.rs, root_authority.rs, source_lifecycle.rs)
Post final review feedback

Summary

CodeRabbit and Codacy already surfaced a thorough set of findings on this PR. I independently read the current HEAD (f9557ce) against those findings rather than re-deriving a review from scratch, since the latest commit ("chore(mtp): drop dead helpers and unused imports") only touches src/device/mtp/planner.rs (removes two #[allow(dead_code)] stubs and an unused uuid/io import). None of the substantive correctness/security findings from the prior automated reviews have been addressed yet. I verified the highest-severity ones directly against the code and confirm they are still present:

Confirmed — Critical / High

Path traversal via unvalidated ancestor names — src/device/mtp/planner.rs:353-378 (staging_path_for). The leaf name goes through relative_name() (rejects /, \, ., ..), but the parent-chain walk pushes parent_object.name.clone() verbatim for every ancestor (line 368). A device that reports a folder named .. or containing a separator escapes both the staging root and, via destination_path_for, the destination root — exactly the case the module's doc comment (lines 3-15) claims is rejected. The same loop also has no cycle guard: a device reporting A.parent == B and B.parent == A spins forever, since current = parent_object.parent never checks against a visited set.
Stage failures never roll back — src/device/transfer.rs, TransferExecutor::run (lines 695-747). self.rollback(&mut committed_files) is only invoked in the cancellation.is_cancelled() branch at the top of the loop. execute_create_directory (line 710) and execute_copy_file (line 719) both propagate errors via ?, bypassing rollback entirely — including the in-copy cancellation path (line 816-818, surfaced as TransferError::Cancelled at line 858-860), which also skips rollback. This contradicts the module's own doc comment (lines 37-41) promising "on cancellation or a failed stage, already-committed files are rolled back in reverse order." A mid-plan I/O or commit failure leaves the destination half-published.
TOCTOU on conflict-resolution publish — src/local/write_authority.rs. commit() (lines 127-137) always publishes via publish_atomic → std::fs::rename (line 588), which unconditionally replaces an existing destination — even when prepare_write_relative_file resolved ConflictResolution::Fresh or Preserved because the destination was absent at prepare time (lines 234-271). If the destination (or chosen preserved sibling) is created between prepare and commit, Skip/Fail/Preserve semantics are silently violated with no error. Related: create_exclusive_staged_file (lines 520-548, Unix) reopens the parent directory via File::open(parent) instead of reusing the authority-bound directory handle already obtained in prepare_write_relative_file (lines 229/231 — bound then immediately dropped as _root_bound/_parent_bound), so the parent path is resolved a second time and can be swapped between the boundary check and the openat.
Copy loop bypasses per-chunk authority revalidation — src/device/transfer.rs:827-829. staged.staged_file().write_all(...) writes through the raw File handle, skipping the mount revalidation that PreparedWriteTarget::write_all (write_authority.rs:113-119) performs before/after each write. A remount mid-copy is only caught at commit, not per chunk, despite the module's claim that both authorities are revalidated around every operation.

Confirmed — Medium

BrowseBudget::max_entries is not a hard cap — src/device/mtp/browse.rs:97-121. The budget is checked once before listing a node's children (line 101), but every child returned by that single list_children call is pushed unconditionally (lines 109-115). A node with more children than the remaining budget overshoots the cap; the existing test (browse_respects_max_entries) only passes because it happens to set max_entries equal to the child count.
CancellationObserver::never_cancelled() doesn't behave as advertised — src/source_lifecycle.rs:463-466. The _sender half of the watch channel is dropped immediately, closing the channel. Per tokio's watch::Receiver::changed semantics, once the channel is closed and the current value has been marked seen, changed().await returns Err immediately — so cancelled().await (lines 472-479) resolves right away instead of never completing. is_cancelled() still correctly returns false (it just reads *receiver.borrow()), but any caller doing select! { _ = observer.cancelled() => ... } on a "never cancelled" observer will see a spurious immediate cancellation.
Serial not normalized consistently — src/device/mtp/identity.rs:55-60. MtpUsbDescriptor::new validates serial.trim().is_empty() but stores the untrimmed serial. Elsewhere (MtpDeviceLabel::from_descriptor) the trimmed value is used, so " ABC123" and "ABC123" collapse to the same MtpDeviceId while remaining unequal MtpUsbDescriptors — a session lookup keyed on the raw descriptor can miss a device whose id resolves fine.
Missing re-exports — src/device/mtp/mod.rs:50-51. planner is a private module; the re-export list omits MtpTransferPlan, MtpStagingWrite, and MtpPlanError, so callers outside the crate can call MtpTransferPlanner::plan but cannot name its Ok/Err types.
MtpStagingWrite::bytes is always empty — src/device/mtp/planner.rs:307-314. The planner never populates this field (no fetch happens during planning); it's documented as "Bytes the planner will write" but is a permanently empty Vec. Any executor code trusting it would write zero-length files. Either drop the field or make the planner responsible for populating it before returning the plan.

Minor / already-flagged, still present

src/device/transfer.rs:895 — fn _unused_os_string(_value: OsString) {} is still present (only planner.rs's dead helpers were removed by the latest commit, not this one), along with the OsString import it exists solely to justify.
src/local/write_authority.rs:73-83 — lease_token fields on PreparedWriteTarget/MountedDirectory are written but never read.
src/device/mtp/transport.rs:107-112 — MtpSession::verify() only checks label().is_empty(), which is unreachable given MtpDeviceLabel's construction guarantees, so it's effectively a no-op liveness check for every real caller (browse, list_storage, fetch_object, MtpTransferPlanner::plan). This is expected to be a stub until a real backend exists, per the PR's own "BLOCKED for live-device validation" note, but worth tracking as a known gap rather than an oversight.

Testing

The PR description's validation claims (1754 lib tests, strict clippy clean) are plausible given the code structure but I did not re-run the suite in this environment — I don't have permission to run cargo test/cargo clippy here. If you want that verified, please grant Bash access to cargo commands and I can re-check.

None of items 1-4 above have accompanying regression tests (the planner tests only cover single-level objects and a "Music"-named parent; nothing exercises a ".." or slash-containing ancestor, a parent-chain cycle, or a mid-transfer stage failure).

Recommendation

Given the PR is explicitly scoped to the MTP module and marked BLOCKED pending a real transport backend, items 5, 7, 8, and the minor nits are reasonably deferrable. Items 1-4, however, are concrete integrity/security defects in code paths this PR adds (path traversal and rollback correctness), not pre-existing code, and are worth fixing before merge rather than after — especially #1 (path traversal) and #2 (missing rollback), which directly contradict the safety guarantees the module docs advertise.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (5)

src/device/transfer.rs (5)
884-895: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

_unused_os_string is a dead placeholder that exists only to keep the OsString import alive.

Delete both the function and the use std::ffi::OsString; at Line 53; nothing else in the file references OsString. The PR notes say dead helpers and unused imports were removed, so this one looks like an oversight.
🧹 Proposed cleanup
-fn _unused_os_string(_value: OsString) {}
-
And at Line 53:
-use std::ffi::OsString;
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 884 - 895, Remove the dead
_unused_os_string function and delete the now-unused std::ffi::OsString import.
Leave WalkdirErrorExt and its into_io_error implementation unchanged.
566-628: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Drop the unused destination parameter.

ensure_ancestor_directory_stages never uses destination — Line 592 exists only to silence the warning — and ensure_parent_directories threads it through for the same reason. Removing it from both signatures (and the two call sites at Lines 391-397 and 494-500) simplifies the contract.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 566 - 628, Remove the unused destination
parameter from ensure_ancestor_directory_stages and ensure_parent_directories,
delete the placeholder assignment, and update both call sites to stop passing
destination. Preserve the existing directory staging and error behavior.
660-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

destination_is_atomic makes Stage::atomic unconditionally true.

The function collapses to validate().is_ok(), and planning already fails earlier when validation fails, so no stage can ever carry atomic: false. The flag documented at Lines 105-108 therefore conveys nothing to callers. Either derive it from a real destination/staging filesystem comparison or drop the field until a non-atomic path exists.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 660 - 666, Remove the redundant
atomicity flag and the destination_is_atomic helper, then update stage planning
and all consumers of Stage::atomic to stop carrying or relying on this value.
Preserve the existing validation behavior and staged-file rename flow; do not
replace it with another constant or validation-based boolean.
1246-1289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Coverage gap: no test exercises Preserve against an existing destination, cancellation, or rollback.

The suite covers Fail, Skip, and Overwrite conflicts plus happy-path execution, but the Preserve-with-existing-destination execution path (where the commit publishes a disambiguated name) and the rollback path are both untested — precisely where the committed_files bookkeeping issue at Lines 719-732 hides. A test that executes a Preserve transfer over an existing file and asserts the original bytes survive, plus one that cancels mid-plan and asserts the destination is clean, would catch both.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 1246 - 1289, The transfer tests need
coverage for Preserve conflict execution and cancellation rollback. Add a test
that runs a Preserve transfer when the destination already contains the original
filename, then assert the existing file’s bytes remain unchanged and the
transferred content is published under the disambiguated name; add another test
that cancels during execution and verifies the destination has no committed
transfer artifacts. Reuse the existing transfer setup and cancellation APIs used
by the nearby conflict and execution tests.
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stage::atomic doesn’t resolve as an intra-doc link. Link the field through [Stage::CopyFile] instead.
🤖 Prompt for 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.

In `@src/device/transfer.rs` around lines 29 - 33, Update the intra-doc link in
the module documentation to reference the atomic field through the
`Stage::CopyFile` variant, replacing the unresolved `Stage::atomic` link while
preserving the surrounding explanation.

🤖 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 `@src/device/transfer.rs`:
- Line 480: Update the source-size calculation in the CopyFile planning flow to
propagate metadata errors instead of converting them to zero with unwrap_or(0).
Ensure the surrounding function returns the metadata error and does not emit a
zero-byte CopyFile stage when entry.metadata() fails, preserving accurate
capacity budgeting and post-copy size validation.
- Around line 719-732: Update the run loop around execute_copy_file to record
outcome.relative_path in committed_files, ensuring rollback targets the actual
published path under ConflictPolicy::Preserve. Pass the planned conflict value
into execute_copy_file and use that parameter instead of re-reading
self.request.conflict_policy.

---

Nitpick comments:
In `@src/device/transfer.rs`:
- Around line 884-895: Remove the dead _unused_os_string function and delete the
now-unused std::ffi::OsString import. Leave WalkdirErrorExt and its
into_io_error implementation unchanged.
- Around line 566-628: Remove the unused destination parameter from
ensure_ancestor_directory_stages and ensure_parent_directories, delete the
placeholder assignment, and update both call sites to stop passing destination.
Preserve the existing directory staging and error behavior.
- Around line 660-666: Remove the redundant atomicity flag and the
destination_is_atomic helper, then update stage planning and all consumers of
Stage::atomic to stop carrying or relying on this value. Preserve the existing
validation behavior and staged-file rename flow; do not replace it with another
constant or validation-based boolean.
- Around line 1246-1289: The transfer tests need coverage for Preserve conflict
execution and cancellation rollback. Add a test that runs a Preserve transfer
when the destination already contains the original filename, then assert the
existing file’s bytes remain unchanged and the transferred content is published
under the disambiguated name; add another test that cancels during execution and
verifies the destination has no committed transfer artifacts. Reuse the existing
transfer setup and cancellation APIs used by the nearby conflict and execution
tests.
- Around line 29-33: Update the intra-doc link in the module documentation to
reference the atomic field through the `Stage::CopyFile` variant, replacing the
unresolved `Stage::atomic` link while preserving the surrounding explanation.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 35b96266-6694-4fd7-913b-1175c902cf58

📥 Commits

Reviewing files that changed from the base of the PR and between ce09d0c and f9557ce.

📒 Files selected for processing (11)

src/device/mod.rs
src/device/mtp/browse.rs
src/device/mtp/identity.rs
src/device/mtp/mod.rs
src/device/mtp/planner.rs
src/device/mtp/transport.rs
src/device/transfer.rs
src/local/mod.rs
src/local/root_authority.rs
src/local/write_authority.rs
src/source_lifecycle.rs

🚧 Files skipped from review as they are similar to previous changes (9)

src/local/mod.rs
src/source_lifecycle.rs
src/device/mtp/mod.rs
src/device/mtp/identity.rs
src/device/mtp/transport.rs
src/local/root_authority.rs
src/device/mtp/planner.rs
src/device/mtp/browse.rs
src/local/write_authority.rs

coderabbitai · 2026-07-25T23:15:01Z

+                Ok(relative) => relative.to_path_buf(),
+                Err(_) => continue,
+            };
+            let source_size = entry.metadata().map(|m| m.len()).unwrap_or(0);


🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Swallowed metadata error silently zeroes a file's size, defeating both the capacity budget and the post-copy size check.

unwrap_or(0) still emits a CopyFile stage with bytes: 0. That file contributes nothing to total_bytes, so the capacity_budget gate at Line 417 can be passed by a plan that overruns the destination, and the declared_bytes != 0 guard at Line 842 is skipped for it. Propagate the error instead.

🐛 Proposed fix

- let source_size = entry.metadata().map(|m| m.len()).unwrap_or(0); + let source_size = entry + .metadata() + .map_err(|error| { + TransferError::io( + "failed to read source entry metadata during planning", + error + .into_io_error() + .unwrap_or_else(|| io::Error::other("walkdir error without payload")), + ) + })? + .len();

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

let source_size = entry.metadata().map(|m| m.len()).unwrap_or(0);

let source_size = entry

.metadata()

.map_err(|error| {

TransferError::io(

"failed to read source entry metadata during planning",

error

.into_io_error()

.unwrap_or_else(|| io::Error::other("walkdir error without payload")),

)

})?

.len();

🤖 Prompt for 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. In `@src/device/transfer.rs` at line 480, Update the source-size calculation in the CopyFile planning flow to propagate metadata errors instead of converting them to zero with unwrap_or(0). Ensure the surrounding function returns the metadata error and does not emit a zero-byte CopyFile stage when entry.metadata() fails, preserving accurate capacity budgeting and post-copy size validation.

coderabbitai · 2026-07-25T23:15:01Z

+                    let outcome = self.execute_copy_file(
+                        source_relative_path,
+                        destination_relative_path,
+                        *bytes,
+                        &mut bytes_so_far,
+                        total_bytes,
+                        index as u32,
+                        total_stages,
+                        progress,
+                        cancellation,
+                    )?;
+                    let _ = outcome;
+                    let _ = conflict;
+                    committed_files.push(destination_relative_path.clone());


🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash # Inspect CommitOutcome and the preserve/disambiguation path in the write authority. ast-grep outline src/local/write_authority.rs --items all rg -nP -C6 '\b(CommitOutcome|Preserved|disambiguat)' src/local/write_authority.rs

Repository: jm2/tributary

Length of output: 5889

🏁 Script executed:

#!/bin/bash set -euo pipefail # Map the relevant parts of the transfer module first. ast-grep outline src/device/transfer.rs --items all # Read the precise ranges around the reported lines. sed -n '680,900p' src/device/transfer.rs | cat -n # Inspect the planner doc and execute_copy_file signature/usage. rg -n -C4 'never re-decides|execute_copy_file|prepare_write_relative_file|commit outcome|committed_files|rollback' src/device/transfer.rs

Repository: jm2/tributary

Length of output: 19065

🏁 Script executed:

#!/bin/bash set -euo pipefail # Inspect the transfer rollback code and any tests covering preserve/rollback behavior. rg -n -C6 'preserve|ConflictPolicy::Preserve|rollback|committed_files|remove_relative_file' src/device/transfer.rs src/local/write_authority.rs

Repository: jm2/tributary

Length of output: 38151

🏁 Script executed:

#!/bin/bash set -euo pipefail # Check whether CommitOutcome.relative_path is intended to be used by callers. rg -n -C3 'relative_path' src | sed -n '1,220p'

Repository: jm2/tributary

Length of output: 13814

🏁 Script executed:

#!/bin/bash set -euo pipefail # See whether run() has any error-path rollback beyond the cancellation branch, # and how callers handle a failed TransferExecutor::run. rg -n -C4 'rollback\(&mut committed_files\)|TransferExecutor::new\(\)\.run\(|\.run\(.*progress|TransferSummary|Cancelled|RollbackFailed' src

Repository: jm2/tributary

Length of output: 48997

Track the actual published path in rollback src/device/transfer.rs:719-732, 869-878

ConflictPolicy::Preserve can commit to a disambiguated name, but run still records destination_relative_path. On cancellation, rollback then removes the original destination file instead of the preserved copy. Push outcome.relative_path from CommitOutcome, and pass the planned conflict into execute_copy_file instead of re-reading self.request.conflict_policy.

🤖 Prompt for 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. In `@src/device/transfer.rs` around lines 719 - 732, Update the run loop around execute_copy_file to record outcome.relative_path in committed_files, ensuring rollback targets the actual published path under ConflictPolicy::Preserve. Pass the planned conflict value into execute_copy_file and use that parameter instead of re-reading self.request.conflict_policy.

codacy-production Bot reviewed Jul 25, 2026

View reviewed changes

jm2 and others added 3 commits July 25, 2026 19:07

chore(mtp): drop dead helpers and unused imports (tr-t3i)

f9557ce

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

jm2 force-pushed the polecat/tr-t3i branch from ce09d0c to f9557ce Compare July 25, 2026 23:09

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

jm2 mentioned this pull request Jul 25, 2026

feat(sync): playlist mapping, incremental state and opt-in auto-sync (tr-bau) #180

Open

	std::env::temp_dir().join(format!("tributary-transfer-{label}-{}", Uuid::new_v4()));
	let path = tempfile::tempdir()?.into_path();

Conversation

jm2 commented Jul 25, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewing PR #178

Uh oh!

coderabbitai Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

codacy-production Bot commented Jul 25, 2026

Not up to standards ⛔

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jm2 commented Jul 25, 2026 •

edited by coderabbitai Bot

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading

coderabbitai Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading