Sprint 3: Transport recovery, write journaling, buffer pool, FileProvider sync anchors, quirk governance, observability#9
Open
EffortlessSteven wants to merge 5 commits intomainfrom
Conversation
…ournaling, buffer pool, FileProvider incremental changes, quirk governance, observability timeline, libmtp compat harness ## New features - MTPDeviceActor.withTransaction<R>: queued exclusive protocol transaction lock - MTPError.sessionBusy: new error case for concurrent transaction attempts - TransportPhase enum (.bulkOut, .bulkIn, .responseWait) with classified timeouts - TransportError.stall: LIBUSB_ERROR_PIPE mapped to stall with auto-recovery in bulkWriteAll/bulkReadOnce - TransportError.timeoutInPhase(TransportPhase): actionable phase-specific timeout errors ## Write-path durability - TransferJournal: recordRemoteHandle(id:handle:) + addContentHash(id:hash:) (default no-ops) - TransferRecord: remoteHandle and contentHash fields - DeviceActor+Reconcile: reconcilePartials() detects and deletes partial objects on reconnect - DeviceActor+Transfer: captures remote handle from SendObjectInfo, reconciles on retry - AtomicHandleBox: thread-safe container for handle exchange between actor and write closure ## Buffer pool and 2-stage pipeline - BufferPool actor: preallocated PooledBuffer pool with acquire/release queue - PooledBuffer (@unchecked Sendable): @unchecked Sendable wrapper for actor boundary crossing - PipelinedUpload/PipelinedDownload: depth-2 read-ahead pipeline with PipelineMetrics ## FileProvider incremental changes - MTPFileProviderExtension: signalRootContainer() after createItem/modifyItem/deleteItem - FileProviderEnumerator: sync anchor encoding, enumerateChanges with index change log ## Quirk governance lifecycle - QuirkStatus enum: proposed | verified | promoted (backward-compatible) - DeviceQuirk: status, evidenceRequired, lastVerifiedDate, lastVerifiedBy fields - All existing quirk profiles promoted to 'promoted' status - scripts/validate-quirks.sh: validates status presence, warns on proposed, fails on promoted without evidence - scripts/generate-compat-matrix.sh: generates markdown compatibility matrix from quirks.json - swiftmtp quirks matrix subcommand ## Observability - TransactionLog actor (SwiftMTPObservability): ring-buffered per-transaction timeline (opcode, txid, bytes, duration, outcome) - MTPOpcodeLabel: human-readable labels for 21 common MTP opcodes - ActionableErrors protocol: user-friendly error descriptions for MTPError and TransportError - ActionableErrors.swift in SwiftMTPCore: maps all error cases to actionable user messages ## libmtp compat harness - scripts/compat-harness.py: Python harness comparing SwiftMTP vs mtp-tools output - compat/expectations/: per-device overlay files classifying diffs as bug/intentional/quirk_needed ## Tests (1920 tests, 0 failures, up from 1891) - TransactionTests: withTransaction serialization, queuing, error propagation - BufferPoolTests: acquire/release, blocking depth, PipelinedUpload/Download metrics - TransferJournalReconcileTests: remoteHandle persistence, partial object cleanup - ObservabilityTests: TransactionLog ring buffer, actionable descriptions, opcode labels - FileProviderEnumeratorTests: sync anchor encoding, enumerateChanges callbacks - PTPUSBHelpersCoverageTests: mapLibusb PIPE→stall, TransportPhase descriptions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughComprehensive enhancement introducing transaction serialization for exclusive MTP protocol access, write-path durability with journaling and partial-write recovery, sophisticated error classification with user-facing descriptions, event-driven cache invalidation, buffer pooling with pipelined transfers, FileProvider rename/move operations, quirk governance with verification metadata, structured transaction auditing, and a libmtp compatibility harness for cross-toolchain validation. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant D as MTPDeviceActor
participant Q as Transaction Queue
participant B as Body (Transactional)
C->>D: withTransaction(body1)
D->>D: transactionInProgress = true
D->>B: execute body1
Note over B: body1 executes
C->>D: withTransaction(body2) [concurrent]
D->>Q: queue continuation for body2
Note over Q: body2 waits in queue
B->>D: body1 completes
D->>D: transactionInProgress = false
D->>Q: resume next waiter
Q->>B: execute body2
Note over B: body2 executes
B->>D: body2 completes
D->>D: transactionInProgress = false
D->>Q: all queued completions resume
sequenceDiagram
participant App as Application
participant Dev as MTPDeviceActor
participant Xfr as write() Method
participant J as TransferJournal
participant Remote as Remote Device
App->>Xfr: write(parent, name, size, url)
Xfr->>Dev: withTransaction()
Xfr->>J: beginWrite(id)
Xfr->>Remote: SendObjectInfo
Remote-->>Xfr: remoteHandle
Xfr->>J: recordRemoteHandle(id, handle)
Xfr->>Remote: SendObject + data
alt Success
Xfr->>J: complete(id)
Xfr->>App: return Progress
else Failure (Partial)
Xfr->>J: updateProgress(id)
Xfr->>Dev: throw error
Note over Dev: On reconnect: reconcilePartials()
Dev->>Dev: load resumable records
Dev->>Remote: delete partial object
end
sequenceDiagram
participant UL as USB Link
participant EP as Event Pump
participant Coal as MTPEventCoalescer
participant Handler as handleMTPEvent()
UL->>EP: startEventPump()
EP->>EP: create coalescer (50ms window)
loop Read interrupt endpoint
EP->>EP: read event data
EP->>Coal: shouldForward()
alt Within window
Coal-->>EP: false (suppress)
else Window expired
Coal-->>EP: true (forward)
EP->>Handler: eventStream.yield(data)
end
EP->>EP: sleep briefly (coalesce burst)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 496d095331
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Pre-fill the first read buffer. | ||
| var readBuf = await pool.acquire() | ||
| var readCount = try source.read(into: readBuf.mutable(count: effectiveChunk)) |
There was a problem hiding this comment.
If FileSource.read fails during the initial prefill, the function exits before entering the loop and skips both pool.release(readBuf) and source.close(). In that failure mode (e.g., transient filesystem read error on the first chunk), the checked-out buffer is leaked from the pool and the file handle stays open, so repeated retries with the same BufferPool can exhaust available buffers and eventually hang waiting on acquire().
Useful? React with 👍 / 👎.
|
|
||
| // Receive the first chunk. | ||
| var recvBuf = await pool.acquire() | ||
| var recvCount = try await receiveChunk(recvBuf.mutable(count: effectiveChunk), effectiveChunk) |
There was a problem hiding this comment.
The first receiveChunk call can throw before the loop starts, but in that path the function does not close sink or return recvBuf to the pool. When an initial device read fails (timeout/disconnect translated to a throw), this leaks both a buffer and an open file handle, and repeated attempts can starve the pool and block subsequent downloads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Sprint 3 production-readiness work across SwiftMTP’s transport, transfer persistence, FileProvider integration, quirk governance, and observability—plus new tooling to generate/validate quirk metadata and compare SwiftMTP vs libmtp behavior.
Changes:
- Add quirk governance fields (
status, evidence/verification metadata) and validation + reporting tooling. - Improve transport/transfer robustness: stall recovery, transaction serialization, write journaling (remote handle/content hash), and partial-write reconciliation.
- Add new performance/observability primitives (transaction log + actionable errors) and expand test coverage for the new behavior.
Reviewed changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/validate-quirks.sh | Enforces new governance fields and evidence requirements for promoted quirks. |
| scripts/generate-compat-matrix.sh | Generates a Markdown compatibility matrix from quirks JSON. |
| scripts/compat-harness.py | New harness to compare SwiftMTP vs libmtp outputs and write structured evidence. |
| probes/mi-note2-probe.txt | Adds a captured probe log artifact for a Xiaomi device. |
| compat/expectations/README.md | Documents YAML expectation overlays for the compat harness. |
| compat/expectations/.gitkeep | Keeps expectations directory tracked in git. |
| SwiftMTPKit/Tests/TransportTests/PTPUSBHelpersCoverageTests.swift | Adds coverage for stall mapping and transport phase descriptions. |
| SwiftMTPKit/Tests/StoreTests/TransferJournalReconcileTests.swift | Adds tests for remote handle/content hash journaling and partial-write reconciliation. |
| SwiftMTPKit/Tests/PropertyTests/QuirkDatabaseCoverageTests.swift | Updates quirk status encoding/decoding expectations to new enum values. |
| SwiftMTPKit/Tests/PropertyTests/ObservabilityTests.swift | Adds tests for TransactionLog, opcode labels, and actionable error descriptions. |
| SwiftMTPKit/Tests/FileProviderTests/FileProviderEnumeratorTests.swift | Adds sync anchor round-trip + change enumeration tests. |
| SwiftMTPKit/Tests/CoreTests/TransactionTests.swift | Adds tests ensuring withTransaction serializes protocol operations. |
| SwiftMTPKit/Tests/CoreTests/BufferPoolTests.swift | Adds tests for BufferPool and pipelined upload/download metrics. |
| SwiftMTPKit/Sources/Tools/swiftmtp-cli/Support/CLIState.swift | Adds actionable CLI messages for new transport/session error cases. |
| SwiftMTPKit/Sources/Tools/swiftmtp-cli/Commands/WizardCommand.swift | Updates quirk status printing for new enum-backed status type. |
| SwiftMTPKit/Sources/Tools/swiftmtp-cli/Commands/SystemCommands.swift | Adds quirks matrix subcommand (Markdown/JSON). |
| SwiftMTPKit/Sources/Tools/learn-promote/main.swift | Updates output formatting for enum-backed quirk status. |
| SwiftMTPKit/Sources/Tools/docc-generator-tool/main.swift | Extends doc generation schema to include governance fields + tolerant status decoding. |
| SwiftMTPKit/Sources/SwiftMTPTransportLibUSB/PTPContainer+USB.swift | Maps LIBUSB pipe errors to a new .stall TransportError. |
| SwiftMTPKit/Sources/SwiftMTPTransportLibUSB/LibUSBTransport.swift | Adds stall recovery via libusb_clear_halt + retry in bulk transfers. |
| SwiftMTPKit/Sources/SwiftMTPStore/SwiftMTPStoreAdapter.swift | Adds adapter APIs to persist remote handle + content hash. |
| SwiftMTPKit/Sources/SwiftMTPStore/StoreActor.swift | Implements persistence for remote handle/content hash and exposes them in resumables. |
| SwiftMTPKit/Sources/SwiftMTPStore/Models/TransferEntity.swift | Adds remoteHandle and contentHash to transfer persistence model. |
| SwiftMTPKit/Sources/SwiftMTPQuirks/Resources/quirks.json | Updates packaged quirks DB with governance fields and promoted statuses. |
| SwiftMTPKit/Sources/SwiftMTPQuirks/Public/QuirkDatabase.swift | Introduces QuirkStatus enum and governance metadata fields on DeviceQuirk. |
| SwiftMTPKit/Sources/SwiftMTPObservability/TransactionLog.swift | Adds TransactionLog actor, opcode labels, and actionable error helpers. |
| SwiftMTPKit/Sources/SwiftMTPFileProvider/MTPFileProviderExtension.swift | Signals enumerators after mutations to improve Finder sync behavior. |
| SwiftMTPKit/Sources/SwiftMTPCore/Public/TransferJournal.swift | Extends journal protocol/record to track remote handle and content hash. |
| SwiftMTPKit/Sources/SwiftMTPCore/Public/Errors.swift | Adds .sessionBusy, .stall, TransportPhase, and .timeoutInPhase. |
| SwiftMTPKit/Sources/SwiftMTPCore/Public/ActionableErrors.swift | Adds actionable descriptions for core/transport errors. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/SendableAdapters.swift | Adds AtomicHandleBox for thread-safe handle capture across tasks. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/PipelinedTransfer.swift | Implements pipelined upload/download with metrics. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/BufferPool.swift | Adds actor-based reusable buffer pool for pipelining. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/Protocol/Proto+Transfer.swift | Captures SendObjectInfo handle for journaling before SendObject data phase. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor.swift | Adds withTransaction serialization + partial reconciliation on open. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor+Transfer.swift | Wraps write in transaction lock; persists remote handle; enhances retry/recovery. |
| SwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor+Reconcile.swift | Implements partial-write cleanup using journal + device state. |
| Specs/quirks.json | Updates source-of-truth quirks spec with governance/evidence/verification fields. |
| try? await journal.fail(id: transferId, error: error) | ||
| } | ||
| // Mark as failed in journal | ||
| if let journal = transferJournal, let transferId = journalTransferId { |
There was a problem hiding this comment.
On the final failure path, the journal is marked failed but the most recent remoteHandleCapture value is not persisted. This means a failed transfer can still leave a partial object on-device without enough journal evidence for reconcilePartialWrites to clean it up on next open. Persist the handle (if present) before/alongside journal.fail(...) so reconciliation has the latest value.
Suggested change
| if let journal = transferJournal, let transferId = journalTransferId { | |
| if let journal = transferJournal, let transferId = journalTransferId { | |
| // Persist the most recent remote handle (if any) so reconciliation can clean up partial writes | |
| if let handle = remoteHandleCapture { | |
| try? await journal.recordRemoteHandleCapture(id: transferId, handle: handle) | |
| } |
Comment on lines
+454
to
+457
| private func signalRootContainer() { | ||
| guard let manager = NSFileProviderManager(for: domain) else { return } | ||
| manager.signalEnumerator(for: .rootContainer) { _ in } | ||
| } |
There was a problem hiding this comment.
signalRootContainer() always signals .rootContainer, even when the changed item is in a specific storage/folder. The codebase already has ChangeSignaler.signalParents(...) to signal the precise parent identifier(s); using root container here may not refresh the correct enumerator and can also cause unnecessary re-enumeration work. Consider signaling the affected parent identifier (storage root or folder handle) instead of always .rootContainer.
Comment on lines
+38
to
+55
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | ||
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | ||
| nonisolated(unsafe) var onFinish: (() -> Void)? | ||
|
|
||
| func didUpdate(_ items: [NSFileProviderItem]) { | ||
| updatedItems.append(contentsOf: items) | ||
| } | ||
|
|
||
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | ||
| deletedIdentifiers.append(contentsOf: identifiers) | ||
| } | ||
|
|
||
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | ||
| onFinish?() | ||
| } | ||
|
|
||
| func finishEnumeratingWithError(_ error: Error) { | ||
| onFinish?() |
There was a problem hiding this comment.
MockChangeObserver stores updates in nonisolated(unsafe) arrays that are mutated from FileProvider callbacks without synchronization. If callbacks arrive on different threads (or tests run under TSAN), this can introduce data races and flaky tests. Consider protecting these arrays with an NSLock, making the observer @MainActor, or using an actor to collect updates.
Suggested change
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | |
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | |
| nonisolated(unsafe) var onFinish: (() -> Void)? | |
| func didUpdate(_ items: [NSFileProviderItem]) { | |
| updatedItems.append(contentsOf: items) | |
| } | |
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | |
| deletedIdentifiers.append(contentsOf: identifiers) | |
| } | |
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | |
| onFinish?() | |
| } | |
| func finishEnumeratingWithError(_ error: Error) { | |
| onFinish?() | |
| private let lock = NSLock() | |
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | |
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | |
| nonisolated(unsafe) var onFinish: (() -> Void)? | |
| func didUpdate(_ items: [NSFileProviderItem]) { | |
| lock.lock() | |
| updatedItems.append(contentsOf: items) | |
| lock.unlock() | |
| } | |
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | |
| lock.lock() | |
| deletedIdentifiers.append(contentsOf: identifiers) | |
| lock.unlock() | |
| } | |
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | |
| lock.lock() | |
| let handler = onFinish | |
| lock.unlock() | |
| handler?() | |
| } | |
| func finishEnumeratingWithError(_ error: Error) { | |
| lock.lock() | |
| let handler = onFinish | |
| lock.unlock() | |
| handler?() |
Comment on lines
+108
to
+116
| # Check that all entries have a governance status field | ||
| echo "🔍 Checking governance status field..." | ||
| missing_status=$(jq -r '.entries[] | select(.status == null) | .id' "$QUIRKS_FILE") | ||
| if [[ -n "$missing_status" ]]; then | ||
| echo "❌ Entries missing 'status' field:" | ||
| echo "$missing_status" | ||
| exit 1 | ||
| fi | ||
| echo "✅ All entries have a 'status' field" |
There was a problem hiding this comment.
validate-quirks.sh now enforces that .status is present, but it doesn't validate that the value is one of the supported governance statuses (e.g. proposed/verified/promoted). A typo or legacy value would pass validation but be silently coerced to .proposed by the Swift decoder, which can hide data issues; consider failing validation when .status is not in the allowed set.
Comment on lines
+125
to
+133
| # Fail if a promoted profile is missing evidenceRequired | ||
| echo "🔍 Checking promoted profiles have evidenceRequired..." | ||
| missing_evidence=$(jq -r '.entries[] | select(.status == "promoted") | select(.evidenceRequired == null or (.evidenceRequired | length) == 0) | .id' "$QUIRKS_FILE") | ||
| if [[ -n "$missing_evidence" ]]; then | ||
| echo "❌ Promoted profiles missing 'evidenceRequired':" | ||
| echo "$missing_evidence" | ||
| exit 1 | ||
| fi | ||
| echo "✅ All promoted profiles have evidenceRequired" |
There was a problem hiding this comment.
The promoted-profile check only verifies .evidenceRequired is non-null/non-empty, but not that it’s actually an array. If it were accidentally a string/object, jq’s length would still be >0 and validation would pass; consider asserting (.evidenceRequired | type) == "array" (and similarly for other array fields you rely on).
|
|
||
| // Capture the remote handle (params: [StorageID, ParentHandle, ObjectHandle]) for journaling. | ||
| // This is filled before SendObject so a partial can be tracked even if SendObject fails. | ||
| handleOut?.set(infoRes.params.count >= 3 ? infoRes.params[2] : (infoRes.params.last ?? 0)) |
There was a problem hiding this comment.
handleOut is set to 0 when the response has fewer than 3 params. That makes it indistinguishable from a real (but invalid) handle and could lead to journaling/deletion attempts against handle 0. Safer is to only set the box when the expected handle param is present, otherwise leave it unchanged/nil.
Suggested change
| handleOut?.set(infoRes.params.count >= 3 ? infoRes.params[2] : (infoRes.params.last ?? 0)) | |
| if infoRes.params.count >= 3 { | |
| handleOut?.set(infoRes.params[2]) | |
| } |
Comment on lines
+439
to
+445
| // Update last known remote handle so reconciliation can find the partial. | ||
| lastKnownRemoteHandle = remoteHandleCapture.value | ||
| if let handle = lastKnownRemoteHandle, let journal = transferJournal, | ||
| let transferId = journalTransferId | ||
| { | ||
| try? await journal.recordRemoteHandle(id: transferId, handle: handle) | ||
| } |
There was a problem hiding this comment.
remoteHandleCapture can change across retries (each SendObjectInfo may allocate a new handle), but the journal is only updated in the initial failure path and after a final success. If later retries fail, the journal may contain a stale handle (or none), which undermines reconcilePartialWrites and can leave orphan partial objects or target the wrong handle. Consider persisting the captured handle to the journal whenever it becomes available (e.g., a defer that records remoteHandleCapture.value before marking the transfer failed, and/or recording after each SendObjectInfo).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…orkflow - MTPEventCoalescer: 50ms burst coalescing with NSLock-guarded shouldForward() - LibUSBTransport: init-time eventStream/continuation (buffers 16 packets), guard against double-start, MTPEventCoalescer integration, 10ms burst sleep - MockTransport: init-time stream/continuation for protocol conformance - DeviceActor EventPump: final class with functional start/stop, cache invalidation on object/storage/deviceInfo events via handleMTPEvent - TransportProtocols: startEventPump() + eventStream with default no-ops - BDD scenarios: write-journal.feature, transport-recovery.feature, transaction-serialization.feature + 11 new CucumberTests step impls - PathSanitizer: safe path normalization utility with dotdot/null-byte guards - PathSanitizerTests: 8 coverage tests for all sanitization paths - MTPEventCoalescerTests: 4 tests (forward, burst coalesce, window expiry, boundary) - tsan-and-compat.yml: new CI workflow for TSAN + compat harness - PTPCodec: additional encode/decode coverage for large objects - Fix: BDDTests async let writeA/writeB type annotation (Void → infer Progress) Tests: 1962 passing, 0 failures (was 1920) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- MTPDevice protocol: add rename(_ handle: MTPObjectHandle, to newName: String) - MTPDeviceActor: implement rename via SetObjectPropValue(0x9804) on ObjectFileName(0xDC07) - VirtualMTPDevice: implement rename (updates objectTree in-memory) - XPC Protocol: RenameRequest, MoveObjectRequest (NSSecureCoding, Sendable) - MTPXPCService protocol: renameObject(_:withReply:), moveObject(_:withReply:) - MTPXPCServiceImpl: implement renameObject and moveObject (calls device.rename/move) - MTPFileProviderExtension.modifyItem: properly routes changedFields: .filename (no contents) → renameObject XPC call .parentItemIdentifier (no contents) → moveObject XPC call .contents → existing delete + re-upload path metadata-only → no-op (was previously no-op for all non-content changes) - All MTPDevice mock conformances updated (CompatFallbackDevice, MockSchedulerDevice, MockDevice) - XPCProtocolCoverageTests: 3 new round-trip tests for RenameRequest/MoveObjectRequest - XPCServiceCoverageTests: testRenameAndMoveObject covering success and device-not-found paths - FileProviderExtensionTests: FailingDeleteXPCService updated for protocol conformance - FileProviderIntegrationTests: MockXPCService updated with rename/move stubs - Fix: IndexCoverageTests/SnapshotterTests move() closing brace lost to swift-format Tests: 1966 passing, 0 failures (was 1962) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tsan-and-compat.yml:
- Around line 50-51: Change the loop that checks SPDX headers to use a raw read
to avoid backslash interpretation: update the while read f; do construct (the
loop that begins with "find SwiftMTPKit/Sources -name \"*.swift\" | while read
f; do") to use read -r so filenames with backslashes or escape sequences are
handled safely; keep the rest of the pipeline and the head/grep/exit behavior
the same.
In `@compat/expectations/README.md`:
- Around line 6-12: Update the fenced code block under the "## File naming"
section so it includes a language specifier (e.g., change the opening fence from
``` to ```text) for the block containing "<vid>_<pid>.yml e.g.
18d1_4ee1.yml ..."; locate the block in README.md under the "## File naming"
header and add the language token to the opening fence to satisfy markdownlint.
- Around line 135-144: The fenced code block in the "Adding a new device"
section is missing blank lines immediately before and after the ```bash block;
update the README.md so there is an empty line preceding the line with ```bash
and another empty line after the closing ``` to follow Markdown best practices
(locate the fenced block containing the command "./scripts/compat-harness.py
--vidpid <vid>:<pid>" and adjust the surrounding lines accordingly).
In `@probes/mi-note2-probe.txt`:
- Around line 2-6: Append deterministic capture context to the generated probe
output (the block starting with "Generated by: swift run swiftmtp probe" and
containing "Probe result: ...") by adding an ISO8601 UTC timestamp and concise
host/build metadata (e.g., OS name/version, kernel via uname -srm, machine/arch,
hostname, and swiftmtp version) so repeated runs are comparable; update the
probe writer invoked by the probe generator to emit a "Timestamp: <ISO8601Z>"
line and a "Host: <OS>/<kernel>/<arch> <hostname> <build-id?>" line immediately
after the header, ensuring stable formatting for automated diffing.
In `@scripts/compat-harness.py`:
- Around line 139-163: Add defensive validation for the --vidpid argument to
prevent malformed VID:PID values being passed to subprocess arguments: implement
a helper function named _vidpid_type that validates the string with a regex like
^[0-9a-fA-F]{4}:[0-9a-fA-F]{4}$, raises argparse.ArgumentTypeError on mismatch,
and returns the normalized (lowercased) value; then update the parser in
_build_parser() to use parser.add_argument(..., type=_vidpid_type,
metavar="XXXX:YYYY", ...) for the --vidpid option so only valid hex VID:PID
strings are accepted.
- Around line 829-832: Remove the unnecessary f-string prefixes on static string
literals added to the lines list (e.g., change f"*Generated by compat-harness.py
— SwiftMTP project*" to a normal string) and similarly for the other static
strings between the block around lines 908–948; locate the code that appends to
the variable lines and replace each f"..." with "..." where there are no
placeholders.
- Line 66: Update the type annotations to use Python 3.9+ built-in generics and
collections.abc: remove Dict, List, Tuple and Generator from the typing import
and instead use dict[..., ...], list[...], tuple[...] in annotations and import
Generator (if still needed) from collections.abc; replace Optional[T] usages
with T | None or plain union syntax, and keep Any imported from typing if used.
Update all occurrences in the file (functions/classes referenced by name in this
module) to use these modern forms so annotations conform to PEP 585.
In `@scripts/validate-quirks.sh`:
- Around line 108-123: The validation currently only treats .status == null as
missing; update the check around QUIRKS_FILE (variables missing_status and
proposed_count) to reject empty or unknown values by enforcing an allowed-status
set instead of only null — compute missing_status by selecting entries where
.status is null or not one of the allowed statuses (e.g.,
"proposed","accepted","deprecated","rejected") and fail if any exist, and keep
proposed_count as a separate warning that selects only .status == "proposed".
Locate the jq selection that builds missing_status and replace it with a
selection that treats (.status // "") and tests membership against the allowed
list (or negates membership) so empty string or unknown values are rejected.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor.swift`:
- Around line 96-112: The withTransaction method can deadlock on nested calls
from the same task; add a task-local reentrancy guard so a task that already
holds the transaction proceeds immediately instead of queuing. Concretely:
introduce a Task-local boolean (e.g. a `@TaskLocal` var like
transactionHeldForCurrentTask) and at the start of withTransaction check it — if
true, just execute and return body() directly; otherwise proceed with the
existing queueing logic but set the task-local flag to true for the duration of
the outer transaction (ensure the flag is cleared on exit in the defer). Keep
existing transactionInProgress and transactionWaiters behavior for cross-task
synchronization, but skip queuing when the task-local flag indicates reentrancy.
Ensure the task-local flag is set before calling body() and cleared in the same
defer that releases waiters.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor`+Transfer.swift:
- Around line 271-274: The code is silently swallowing errors from storages() by
using "try? await" and continuing with an empty rootStorages; change this to
preserve and propagate the failure: call storages() with "try await" (not try?)
so any error is thrown to the caller (or explicitly handle and rethrow with
context), then use the non-optional result to compute rootStorages and
preferredRootStorage; update surrounding function signature to allow throwing if
necessary and remove the fallback to an empty array so real transport/session
errors from storages() are not masked.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/MTPEventCoalescer.swift`:
- Around line 11-18: Add an explicit empty deinitializer to the
MTPEventCoalescer class to satisfy the SwiftLint required_deinit rule: update
the final class MTPEventCoalescer (which currently defines window, lastEmitTime,
lock, and init) by adding a public or internal deinit { } method (empty body) so
the linter recognizes an explicit deinitializer even though no cleanup is
needed.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/PathSanitizer.swift`:
- Around line 19-27: The sanitize(_ deviceName: String) -> String? method in
PathSanitizer currently trims whitespace, strips NULs and path separators but
does not remove other control characters (U+0001–U+001F) or newlines; update
sanitize to also remove all characters in CharacterSet.controlCharacters and
CharacterSet.newlines (or filter unicodeScalars where isASCII && value <= 0x1F)
after the existing replacements so that control chars/newlines are removed
safely while keeping the rest of the logic intact.
- Around line 35-38: The current truncation uses name.count (characters) which
can exceed the filesystem 255 UTF-8 byte limit; replace the simple prefix with
byte-aware truncation by iterating grapheme clusters and building a new String
while checking the UTF-8 byte length: start with an empty result, for each
Character in name (or name.characters), append it only if (result.utf8.count +
character.utf8.count) <= maxNameLength, then assign result back to name; update
the truncation block that references name and maxNameLength in
PathSanitizer.swift accordingly so filenames never exceed the UTF-8 byte limit.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/Protocol/Proto`+Transfer.swift:
- Around line 170-173: The code currently writes a synthetic 0 into the journal
by calling handleOut?.set(...) when infoRes.params is missing the expected
entries; change this to only call handleOut?.set when a real remote handle is
present (e.g. infoRes.params.count >= 3) or when params.last exists and is a
non-zero value, and otherwise skip calling set so no synthetic 0 is recorded;
apply the same conditional change to the other occurrence that uses
handleOut?.set with infoRes.params (the block around the second occurrence noted
in the review).
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/Protocol/PTPCodec.swift`:
- Line 178: The guard that silently returns nil when count exceeds
PTPReader.maxSafeCount should be changed to surface a protocol error instead:
replace the early return with a call to the existing validation routine
(validateCount(_:)) or a throwing path so the overflow is propagated as
MTPError.protocolError(0x2006) rather than nil; update the decoder/parse method
that currently does "guard count <= PTPReader.maxSafeCount else { return nil }"
to call and propagate validateCount(count) (or throw an explicit
MTPError.protocolError(0x2006)) so callers receive a deterministic protocol
error for oversize datasets.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/BufferPool.swift`:
- Around line 35-45: Validate constructor arguments at the top of
init(bufferSize:poolDepth:) to fail early on invalid values: add checks like
precondition(bufferSize > 0, "bufferSize must be > 0") and
precondition(poolDepth > 0, "poolDepth must be > 0") (or guard+fatalError if you
prefer) so poolDepth == 0 or negative and non-positive bufferSize are rejected
before using (0..<poolDepth) or allocate calls; keep the rest of init (assigning
self.bufferSize, self.poolDepth, self._allBuffers, self.available, and
self.waiters) unchanged and reference the existing init, acquire(),
PooledBuffer, _allBuffers and available symbols when making the change.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/PipelinedTransfer.swift`:
- Around line 91-93: The method returns PipelineMetrics without checking that
totalSent matches totalSize, allowing truncated transfers to be reported as
successful; update the function(s) that construct PipelineMetrics (the blocks
creating PipelineMetrics(bytesTransferred: totalSent, durationSeconds: elapsed,
retryCount: 0) around the totalSent/totalSize checks at the shown locations) to
validate transferred bytes: if totalSize > 0 verify totalSent == totalSize (or
totalSent >= totalSize if partials are acceptable) and on mismatch return/throw
a failure (e.g., throw an error or return a PipelineMetrics indicating
failure/retry) instead of reporting success — ensure both occurrences (the one
at lines ~91-93 and the one at ~162-164) use the same validation logic.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/SendableAdapters.swift`:
- Around line 42-58: Replace the direct NSLock usage in AtomicHandleBox with the
existing _Lock helper for consistency: remove the private let lock = NSLock()
and use the _Lock instance instead, then wrap reads and writes of _value inside
_Lock.with { ... } calls (update the set(_:) method and the value getter to call
_lock.with { _value = value } and return _lock.with { _value } respectively),
keeping the private storage name _value and the class name AtomicHandleBox
unchanged so callers remain unaffected.
In `@SwiftMTPKit/Sources/SwiftMTPCore/Public/MTPDevice.swift`:
- Around line 323-329: The new required protocol method rename(_ handle:
MTPObjectHandle, to newName: String) async throws is source-breaking for
external implementers of MTPDevice; add a default implementation in the
MTPDevice protocol extension (same place where devicePolicy and probeReceipt
defaults live) that provides a backward-compatible behavior (e.g., throw a
sensible unsupported/not-implemented error or no-op consistent with the existing
defaults) so existing conformances compile without change; implement the default
for the exact symbol rename(_:to:) with the async throws signature in the
MTPDevice extension.
In `@SwiftMTPKit/Sources/SwiftMTPFileProvider/MTPFileProviderExtension.swift`:
- Around line 379-383: The rename/move response is dropping file size because
both MTPFileProviderItem initializers set size: nil; update the two places that
create updatedItem (the MTPFileProviderItem constructors near the
completionHandler calls) to preserve the original size for non-directory items
by passing the captured size (e.g., capturedSize or the source item's size)
instead of nil (keep size nil only for directories), so the completionHandler
receives the correct size metadata after rename/move.
- Around line 351-359: The modifyItem implementation currently calls
completionHandler(item, [], false, nil) on guard failures and other error
branches, which incorrectly reports success/no-op; change those branches (in
modifyItem where getXPCService() / MTPFileProviderItem.parseItemIdentifier or
missing storageId/objectHandle fail, and where XPC/operation callbacks report
failure) to pass a non-nil NSError describing the failure to completionHandler
(e.g., create an NSError with a clear domain/code and localizedDescription) and
set the progress and returned value accordingly; update the guard failure and
the operation-failure branches that currently call completionHandler(item, [],
false, nil) to call completionHandler(item, [], false, error) and ensure
getXPCService(), MTPFileProviderItem.parseItemIdentifier(...), and any XPC
callback paths propagate that NSError to the caller.
In `@SwiftMTPKit/Sources/SwiftMTPObservability/TransactionLog.swift`:
- Around line 98-101: In dump(redacting:) the JSON encoding failure is currently
swallowed and returns "[]"; change the guard to catch the encoding error from
encoder.encode(snapshot) (and failure to construct the String from data), log
the error with the component's observability/logger (include a clear message
like "TransactionLog.dump encoding failed" and the error) and then return "[]"
so failures are visible; reference the dump(redacting:) method, the encoder
instance and snapshot being encoded when adding the error logging.
In `@SwiftMTPKit/Sources/SwiftMTPSync/MirrorEngine.swift`:
- Around line 130-133: The current logic in MirrorEngine.swift uses
components.compactMap { PathSanitizer.sanitize($0) } which silently drops
invalid components and collapses path structure; change this to preserve
detection of sanitization failures by mapping components with
PathSanitizer.sanitize and checking for any nil result, and if any component
failed sanitization then fail closed (return an error/throw/return nil) instead
of building a path; update the code paths that use
safeComponents/relativePath/root.appendingPathComponent to handle the failure
return value or thrown error accordingly so no invalid device path can map to an
unintended local path.
In `@SwiftMTPKit/Sources/SwiftMTPTransportLibUSB/LibUSBTransport.swift`:
- Around line 692-703: The read loop swallows errors from bulkReadOnce and
immediately retries, causing a hot-loop; modify the loop that calls
bulkReadOnce(evtEP, into: &buf, max: 1024, timeout: 1000) so that when the read
fails or returns <= 0 you await a short backoff (e.g., Task.sleep for a few
milliseconds) before continuing, rather than immediately continuing, while
keeping the existing short sleep after successful reads (the existing
Task.sleep(nanoseconds: 10_000_000)) for burst coalescing; locate the logic
around bulkReadOnce, the check `got > 0`, and the subsequent `continue` and
insert the sleep on the error/failure path to prevent tight spinning.
In `@SwiftMTPKit/Sources/SwiftMTPXPC/MTPXPCServiceImpl.swift`:
- Around line 291-304: The moveObject handler ignores
MoveObjectRequest.newStorageId, so storage-targeted moves are dropped; update
moveObject to inspect request.newStorageId and either pass it through to the
device move API or fail fast: if your MTPDevice.move API supports a storage
parameter (e.g., device.move(_:to:storage:) or similar), call that instead of
device.move(handle, to: parent) using MoveObjectRequest.newStorageId; otherwise,
detect a non-nil request.newStorageId and reply with WriteResponse(success:
false, errorMessage: "Storage migration unsupported") so callers are not misled
by a successful response. Ensure you reference MoveObjectRequest.newStorageId,
moveObject(_:,withReply:), and the device.move call when making the change.
In `@SwiftMTPKit/Sources/SwiftMTPXPC/Protocol.swift`:
- Around line 469-476: The XPC deserializers are converting Int64 → UInt32
without range checks (e.g., in the init?(coder:) shown where objectHandle =
UInt32(coder.decodeInt64(forKey: "objectHandle"))), which can silently wrap on
out-of-range values; update each deserializer that decodes an Int64 and assigns
to a UInt32 (search for patterns like UInt32(coder.decodeInt64(forKey: ...))) to
first read the Int64 into a temporary (e.g., let raw = coder.decodeInt64(forKey:
"...")), validate that raw is between Int64(UInt32.min) and Int64(UInt32.max),
and only then cast to UInt32, returning nil from the init? (failing
deserialization) if the value is out of range; apply this same check to all
occurrences (including the shown init?(coder:) and the other
initializers/decoders referenced) to ensure deserialization fails safely on
invalid handles.
In `@SwiftMTPKit/Sources/Tools/docc-generator-tool/main.swift`:
- Around line 26-37: The new governance fields evidenceRequired,
lastVerifiedDate, and lastVerifiedBy are parsed but not currently rendered by
generateDocCPage(); update generateDocCPage() to include a "Provenance" (or
"Governance") subsection that outputs these fields when non-nil, formatting
lastVerifiedDate/lastVerifiedBy as a date/byline and evidenceRequired as a list
or comma-separated string; if you prefer to defer UI work, add a clear TODO in
generateDocCPage() referencing these properties and the Status init(from:)
behavior so reviewers know unknown Status values default to .proposed.
In `@SwiftMTPKit/Tests/BDDTests/CucumberTests.swift`:
- Around line 271-284: The test helper assertUploadRecordsRemoteHandle currently
bypasses the real upload flow by directly calling
InMemoryJournal.recordRemoteHandle; replace that direct write with performing an
actual upload that exercises the production upload→journal integration (invoke
your uploader API that uploads to the MTP device so it will call
journal.recordRemoteHandle under the hood), then loadResumables and assert the
journal contains the remoteHandle as before; reference the existing symbols
assertUploadRecordsRemoteHandle, InMemoryJournal, recordRemoteHandle,
beginWrite, and loadResumables to locate where to swap the direct
recordRemoteHandle call for a real upload invocation and ensure tempURL/source
file is created and cleaned up for the upload.
- Around line 286-299: The test currently calls try await
device.delete(partialHandle...) which verifies delete behavior instead of
exercising the reconnect/reconcile flow; replace that direct deletion with a
simulated reconnect that triggers reconciliation (e.g., call await
device.reconnect() or perform await device.disconnect() then await
device.connect(), or invoke the explicit
reconcile/reconcileOnReconnect/reconcilePendingCleanup method if present) and
then call listRootObjects() and assert the partialHandle is absent; update the
code in assertPartialObjectCleanedUpOnReconnect to remove the device.delete(...)
call and instead drive the reconnect/reconcile path on the same device instance
before the XCTAssertFalse check.
- Around line 537-547: The three Cucumber step bodies are no-ops; replace the
comment-only bodies so they actually configure the mock transport, trigger the
transfer, and assert recovery: in the Given step call the same setup used by
testUSBStallRecoveredAutomatically (e.g. invoke VirtualMTPLink.pipeStall or the
test helper that installs a pipe stall), in the When step invoke the bulk
transfer path exercised in BDDRunner (e.g. call BDDRunner.getStorageIDs or the
helper that triggers the bulk transfer), and in the Then step assert the stall
was cleared and the transfer succeeded using XCTest assertions (e.g. XCTAssert/
XCTAssertNoThrow or checking the expected response), referencing
testUSBStallRecoveredAutomatically, VirtualMTPLink.pipeStall, and
BDDRunner.getStorageIDs to locate the relevant helpers.
- Around line 303-322: The test assertConcurrentWritesSerialized only checks
both writes complete but not that they were serialized; change it to verify
non-overlap or ordering by recording start and finish times for each write and
asserting they do not overlap (e.g., finishA <= startB or finishB <= startA).
Wrap each device.write call in a small async wrapper that captures Date()
immediately before and immediately after awaiting device.write (use async let to
start both wrappers), then await both and assert the timestamps show
serialization. If device.write cannot be instrumented, replace with a test
double/mocked device.write implementation that records start/finish events and
assert those events are non-overlapping. Reference:
assertConcurrentWritesSerialized and device.write.
- Around line 329-330: Add an explicit deinitializer to the InMemoryJournal
class to satisfy the SwiftLint required_deinit rule: in the class declaration
for InMemoryJournal (which implements TransferJournal and declares the NSLock
property `lock`), add a deinit { ... } body — at minimum an empty deinit to
silence the lint rule, or include any necessary cleanup such as releasing or
unlocking `lock` or clearing other resources owned by InMemoryJournal.
- Around line 132-142: The test currently allows the first call to
link.getStorageIDs() to succeed, which hides missing stall behavior; change the
test to require that the initial call fails with a TransportError.io (e.g., use
XCTAssertThrowsError or an explicit do/try/catch that calls XCTFail if the first
try succeeds) and only then perform the retry (link.getStorageIDs()) to verify
recovery; reference the existing symbols link.getStorageIDs() and TransportError
(and the surrounding test in CucumberTests.swift) so you locate and update the
initial invocation to assert a thrown .io error before the retry.
In `@SwiftMTPKit/Tests/BDDTests/Features/write-journal.feature`:
- Around line 10-13: The scenario contains a duplicate step "Given a connected
mock device" that is already provided by the Background; remove the redundant
"Given a connected mock device" line from the Scenario so the test relies on the
Background setup and avoids duplicate step execution (update the Scenario block
that currently contains "When I upload a file to the device" / "Then the
transfer journal contains the remote handle" to omit the repeated Given).
In `@SwiftMTPKit/Tests/CoreTests/DeviceServiceCoverageTests.swift`:
- Around line 389-390: The test double's rename(_ handle: MTPObjectHandle, to
newName: String) async throws {} currently silently succeeds; change it to throw
MTPError.notSupported with a descriptive message (e.g.,
MTPError.notSupported("rename called in DeviceServiceCoverageTests")) so callers
will fail fast and behavior matches other mocks like SnapshotterTests and
IndexCoverageTests; update the rename implementation in
DeviceServiceCoverageTests to throw that error.
In `@SwiftMTPKit/Tests/CoreTests/TransactionTests.swift`:
- Around line 82-105: The test currently uses Task.sleep to order t1 and t2
which is flaky; instead create an explicit async signal (e.g., a
CheckedContinuation/AsyncStream or an AsyncSemaphore) that t1 resumes from
inside its device.withTransaction body once it has entered the transaction, and
have the test await that signal before starting t2. Concretely: add a
continuation or semaphore variable (e.g., startedContinuation or startSignal) in
the test, call startSignal.resume()/startSignal.signal() inside the closure
passed to device.withTransaction in t1 immediately after acquiring the
transaction, replace the try await Task.sleep(...) with await
startSignal.wait()/withCheckedContinuation { ... } so t2 is only launched after
t1 has definitely entered the transaction, and keep recorder.record(1)/record(2)
and the final assertions unchanged.
In `@SwiftMTPKit/Tests/FileProviderTests/FileProviderEnumeratorTests.swift`:
- Around line 37-57: The MockChangeObserver test helper class is missing an
explicit deinit which triggers SwiftLint's required_deinit rule; add a deinit to
the MockChangeObserver class (class name: MockChangeObserver) — implement a
deinit { } (optionally clear references such as onFinish = nil or reset arrays
updatedItems/deletedIdentifiers) to satisfy the lint rule and keep lifecycle
cleanup explicit.
In `@SwiftMTPKit/Tests/IntegrationTests/FileProviderIntegrationTests.swift`:
- Around line 72-82: Add request-tracking arrays for the new operations (e.g.,
renameRequests: [RenameRequest] and moveRequests: [MoveObjectRequest]) alongside
the other mock request arrays near the existing properties (around where
writeRequests/deleteRequests/createFolderRequests are declared), and update the
methods renameObject(_:withReply:) and moveObject(_:withReply:) to append the
incoming request to their respective arrays before calling
reply(WriteResponse(success: true)); this mirrors the pattern used by
writeObject, deleteObject, and createFolder for test assertions.
In `@SwiftMTPKit/Tests/XPCTests/XPCServiceCoverageTests.swift`:
- Around line 256-264: The test's move success path (using impl.moveObject with
MoveObjectRequest and asserting moveResp.success) is a no-op because the seeded
object is already at the root; change the request to use a distinct target (set
newParentHandle to a different parentHandle or change newStorageId to a
different storage) and after the move, fetch the object's metadata (e.g., via
the same API used elsewhere in tests) to assert the object's
parentHandle/newStorageId changed; update the MoveObjectRequest parameters
(objectHandle, newParentHandle/newStorageId) and add an assertion that the
fetched object's location matches the intended target instead of only checking
moveResp.success.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
scripts/__pycache__/compat-harness.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (63)
.github/workflows/tsan-and-compat.ymlCHANGELOG.mdSpecs/quirks.jsonSwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor+Reconcile.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor+Transfer.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/DeviceActor.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/MTPEventCoalescer.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/PathSanitizer.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Protocol/PTPCodec.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Protocol/Proto+Transfer.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/BufferPool.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/PipelinedTransfer.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Transfer/SendableAdapters.swiftSwiftMTPKit/Sources/SwiftMTPCore/Internal/Transport/TransportProtocols.swiftSwiftMTPKit/Sources/SwiftMTPCore/Public/ActionableErrors.swiftSwiftMTPKit/Sources/SwiftMTPCore/Public/Errors.swiftSwiftMTPKit/Sources/SwiftMTPCore/Public/MTPDevice.swiftSwiftMTPKit/Sources/SwiftMTPCore/Public/TransferJournal.swiftSwiftMTPKit/Sources/SwiftMTPFileProvider/MTPFileProviderExtension.swiftSwiftMTPKit/Sources/SwiftMTPObservability/TransactionLog.swiftSwiftMTPKit/Sources/SwiftMTPQuirks/Public/QuirkDatabase.swiftSwiftMTPKit/Sources/SwiftMTPQuirks/Resources/quirks.jsonSwiftMTPKit/Sources/SwiftMTPStore/Models/TransferEntity.swiftSwiftMTPKit/Sources/SwiftMTPStore/StoreActor.swiftSwiftMTPKit/Sources/SwiftMTPStore/SwiftMTPStoreAdapter.swiftSwiftMTPKit/Sources/SwiftMTPSync/MirrorEngine.swiftSwiftMTPKit/Sources/SwiftMTPTestKit/VirtualMTPDevice.swiftSwiftMTPKit/Sources/SwiftMTPTransportLibUSB/LibUSBTransport.swiftSwiftMTPKit/Sources/SwiftMTPTransportLibUSB/MockTransport.swiftSwiftMTPKit/Sources/SwiftMTPTransportLibUSB/PTPContainer+USB.swiftSwiftMTPKit/Sources/SwiftMTPXPC/MTPXPCServiceImpl.swiftSwiftMTPKit/Sources/SwiftMTPXPC/Protocol.swiftSwiftMTPKit/Sources/Tools/docc-generator-tool/main.swiftSwiftMTPKit/Sources/Tools/learn-promote/main.swiftSwiftMTPKit/Sources/Tools/swiftmtp-cli/Commands/SystemCommands.swiftSwiftMTPKit/Sources/Tools/swiftmtp-cli/Commands/WizardCommand.swiftSwiftMTPKit/Sources/Tools/swiftmtp-cli/Support/CLIState.swiftSwiftMTPKit/Tests/BDDTests/CucumberTests.swiftSwiftMTPKit/Tests/BDDTests/Features/transaction-serialization.featureSwiftMTPKit/Tests/BDDTests/Features/transport-recovery.featureSwiftMTPKit/Tests/BDDTests/Features/write-journal.featureSwiftMTPKit/Tests/CoreTests/BufferPoolTests.swiftSwiftMTPKit/Tests/CoreTests/DeviceServiceCoverageTests.swiftSwiftMTPKit/Tests/CoreTests/MTPEventCoalescerTests.swiftSwiftMTPKit/Tests/CoreTests/PathSanitizerTests.swiftSwiftMTPKit/Tests/CoreTests/TransactionTests.swiftSwiftMTPKit/Tests/FileProviderTests/FileProviderEnumeratorTests.swiftSwiftMTPKit/Tests/FileProviderTests/FileProviderExtensionTests.swiftSwiftMTPKit/Tests/IndexTests/IndexCoverageTests.swiftSwiftMTPKit/Tests/IndexTests/SnapshotterTests.swiftSwiftMTPKit/Tests/IntegrationTests/FileProviderIntegrationTests.swiftSwiftMTPKit/Tests/PropertyTests/ObservabilityTests.swiftSwiftMTPKit/Tests/PropertyTests/QuirkDatabaseCoverageTests.swiftSwiftMTPKit/Tests/StoreTests/TransferJournalReconcileTests.swiftSwiftMTPKit/Tests/TransportTests/PTPUSBHelpersCoverageTests.swiftSwiftMTPKit/Tests/XPCTests/XPCProtocolCoverageTests.swiftSwiftMTPKit/Tests/XPCTests/XPCServiceCoverageTests.swiftcompat/expectations/.gitkeepcompat/expectations/README.mdprobes/mi-note2-probe.txtscripts/compat-harness.pyscripts/generate-compat-matrix.shscripts/validate-quirks.sh
Comment on lines
+50
to
+51
| find SwiftMTPKit/Sources -name "*.swift" | while read f; do | ||
| head -1 "$f" | grep -q "SPDX-License-Identifier" || (echo "Missing SPDX in $f" && exit 1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check if the workflow file exists and examine lines 50-51
if [ -f .github/workflows/tsan-and-compat.yml ]; then
echo "=== File exists ==="
wc -l .github/workflows/tsan-and-compat.yml
echo ""
echo "=== Lines 50-51 context (lines 48-53) ==="
sed -n '48,53p' .github/workflows/tsan-and-compat.yml
else
echo "File not found"
fiRepository: EffortlessMetrics/SwiftMTP-dev
Length of output: 434
🏁 Script executed:
# Run the verification command from the review comment
echo "=== Verification: checking for bare read loops ==="
rg -nP 'while\s+read(?!\s+-r)\b' .github/workflows -g '*.yml' -g '*.yaml' || echo "No bare read loops found"Repository: EffortlessMetrics/SwiftMTP-dev
Length of output: 235
🏁 Script executed:
# Check what linting tools or shellcheck might flag
echo "=== Checking for shellcheck SC2162 violation ==="
if command -v shellcheck &> /dev/null; then
shellcheck -f gcc .github/workflows/tsan-and-compat.yml 2>&1 | grep -E "(SC2162|read)" || echo "No SC2162 errors found by shellcheck"
else
echo "shellcheck not available in sandbox"
fiRepository: EffortlessMetrics/SwiftMTP-dev
Length of output: 159
Use read -r in the SPDX loop to safely handle filenames with special characters.
The while read loop should use -r to prevent backslash interpretation. While Swift filenames typically don't contain backslashes, this is a defensive best practice for robust shell scripts.
🔧 Proposed fix
- find SwiftMTPKit/Sources -name "*.swift" | while read f; do
+ find SwiftMTPKit/Sources -name "*.swift" | while IFS= read -r f; do
head -1 "$f" | grep -q "SPDX-License-Identifier" || (echo "Missing SPDX in $f" && exit 1)
done📝 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
| find SwiftMTPKit/Sources -name "*.swift" | while read f; do | |
| head -1 "$f" | grep -q "SPDX-License-Identifier" || (echo "Missing SPDX in $f" && exit 1) | |
| find SwiftMTPKit/Sources -name "*.swift" | while IFS= read -r f; do | |
| head -1 "$f" | grep -q "SPDX-License-Identifier" || (echo "Missing SPDX in $f" && exit 1) | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tsan-and-compat.yml around lines 50 - 51, Change the loop
that checks SPDX headers to use a raw read to avoid backslash interpretation:
update the while read f; do construct (the loop that begins with "find
SwiftMTPKit/Sources -name \"*.swift\" | while read f; do") to use read -r so
filenames with backslashes or escape sequences are handled safely; keep the rest
of the pipeline and the head/grep/exit behavior the same.
Comment on lines
+6
to
+12
| ## File naming | ||
|
|
||
| ``` | ||
| <vid>_<pid>.yml e.g. 18d1_4ee1.yml (Google Pixel 7) | ||
| 04e8_6860.yml (Samsung Galaxy S21) | ||
| 2717_ff10.yml (Xiaomi Mi Note 2) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block showing file naming examples should have a language specifier for consistency with markdownlint rules.
Proposed fix
## File naming
-```
+```text
<vid>_<pid>.yml e.g. 18d1_4ee1.yml (Google Pixel 7)
04e8_6860.yml (Samsung Galaxy S21)
2717_ff10.yml (Xiaomi Mi Note 2)
```📝 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
| ## File naming | |
| ``` | |
| <vid>_<pid>.yml e.g. 18d1_4ee1.yml (Google Pixel 7) | |
| 04e8_6860.yml (Samsung Galaxy S21) | |
| 2717_ff10.yml (Xiaomi Mi Note 2) | |
| ``` | |
| ## File naming | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compat/expectations/README.md` around lines 6 - 12, Update the fenced code
block under the "## File naming" section so it includes a language specifier
(e.g., change the opening fence from ``` to ```text) for the block containing
"<vid>_<pid>.yml e.g. 18d1_4ee1.yml ..."; locate the block in README.md
under the "## File naming" header and add the language token to the opening
fence to satisfy markdownlint.
Comment on lines
+135
to
+144
| ## Adding a new device | ||
|
|
||
| 1. Run the harness once without an overlay to collect raw diffs: | ||
| ```bash | ||
| ./scripts/compat-harness.py --vidpid <vid>:<pid> | ||
| ``` | ||
| 2. Inspect `evidence/<date>/<vid>_<pid>/<run-id>/diff.md`. | ||
| 3. Create `compat/expectations/<vid>_<pid>.yml` and classify each diff. | ||
| 4. Re-run the harness to verify the exit code is now `0`. | ||
| 5. Commit both the overlay file and the `diff.md` evidence as a PR. |
There was a problem hiding this comment.
Add blank lines around fenced code blocks.
Markdown best practices require blank lines before and after fenced code blocks for consistent rendering.
Proposed fix
1. Run the harness once without an overlay to collect raw diffs:
+
```bash
./scripts/compat-harness.py --vidpid <vid>:<pid>
```
+
2. Inspect `evidence/<date>/<vid>_<pid>/<run-id>/diff.md`.📝 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
| ## Adding a new device | |
| 1. Run the harness once without an overlay to collect raw diffs: | |
| ```bash | |
| ./scripts/compat-harness.py --vidpid <vid>:<pid> | |
| ``` | |
| 2. Inspect `evidence/<date>/<vid>_<pid>/<run-id>/diff.md`. | |
| 3. Create `compat/expectations/<vid>_<pid>.yml` and classify each diff. | |
| 4. Re-run the harness to verify the exit code is now `0`. | |
| 5. Commit both the overlay file and the `diff.md` evidence as a PR. | |
| ## Adding a new device | |
| 1. Run the harness once without an overlay to collect raw diffs: |
🧰 Tools
🪛 LanguageTool
[style] ~141-~141: ‘vid’ is informal. Consider replacing it.
Context: ... --vidpid : ``` 2. Inspect evidence/<date>/<vid>_<pid>/<run-id>/diff.md. 3. Create `co...
(VID)
[style] ~142-~142: ‘vid’ is informal. Consider replacing it.
Context: ...vid>//diff.md. 3. Create compat/expectations/.yml` and classify each diff. 4. ...
(VID)
🪛 markdownlint-cli2 (0.21.0)
[warning] 138-138: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 140-140: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compat/expectations/README.md` around lines 135 - 144, The fenced code block
in the "Adding a new device" section is missing blank lines immediately before
and after the ```bash block; update the README.md so there is an empty line
preceding the line with ```bash and another empty line after the closing ``` to
follow Markdown best practices (locate the fenced block containing the command
"./scripts/compat-harness.py --vidpid <vid>:<pid>" and adjust the surrounding
lines accordingly).
Comment on lines
+2
to
+6
| # Generated by: swift run swiftmtp probe | ||
| # VID: 0x2717 PID: 0xff40 | ||
| # Interface: class=0xff (vendor-specific) | ||
| # Endpoints: IN=0x81 OUT=0x01 EVT=0x82 | ||
| # Probe result: MTP session opened successfully (400ms stabilize) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding capture context for reproducibility.
Please consider appending a timestamp and host context (e.g., OS/build) so repeated probe logs can be compared deterministically over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@probes/mi-note2-probe.txt` around lines 2 - 6, Append deterministic capture
context to the generated probe output (the block starting with "Generated by:
swift run swiftmtp probe" and containing "Probe result: ...") by adding an
ISO8601 UTC timestamp and concise host/build metadata (e.g., OS name/version,
kernel via uname -srm, machine/arch, hostname, and swiftmtp version) so repeated
runs are comparable; update the probe writer invoked by the probe generator to
emit a "Timestamp: <ISO8601Z>" line and a "Host: <OS>/<kernel>/<arch> <hostname>
<build-id?>" line immediately after the header, ensuring stable formatting for
automated diffing.
Comment on lines
+139
to
+163
| def _run( | ||
| cmd: List[str], | ||
| *, | ||
| cwd: Optional[pathlib.Path] = None, | ||
| timeout: int = 120, | ||
| ) -> subprocess.CompletedProcess: | ||
| """Run *cmd* and return CompletedProcess. Never raises on non-zero exit.""" | ||
| _log().debug("exec: %s (cwd=%s)", " ".join(cmd), cwd or ".") | ||
| try: | ||
| return subprocess.run( | ||
| cmd, | ||
| cwd=str(cwd) if cwd else None, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| timeout=timeout, | ||
| ) | ||
| except FileNotFoundError: | ||
| result = subprocess.CompletedProcess(cmd, returncode=127, stdout="", stderr="") | ||
| result.stderr = f"command not found: {cmd[0]}" | ||
| return result | ||
| except subprocess.TimeoutExpired: | ||
| result = subprocess.CompletedProcess(cmd, returncode=124, stdout="", stderr="") | ||
| result.stderr = f"timed out after {timeout}s: {' '.join(cmd)}" | ||
| return result |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM with note on subprocess security.
The _run() helper properly handles timeouts and missing commands. The S603 static analysis warning about untrusted input is a general caution; here the commands are controlled by the script logic, and user-provided --vidpid values only affect command arguments (not the executable path).
For defense-in-depth, consider validating the --vidpid format at argument parsing time:
💡 Optional: Add vidpid validation
# In _build_parser(), add validation:
def _vidpid_type(value: str) -> str:
if not re.match(r'^[0-9a-fA-F]{4}:[0-9a-fA-F]{4}$', value):
raise argparse.ArgumentTypeError(
f"Invalid VID:PID format: {value!r} (expected XXXX:YYYY hex)"
)
return value.lower()
parser.add_argument(
"--vidpid",
metavar="XXXX:YYYY",
type=_vidpid_type,
...
)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 148-155: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
[error] 148-148: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/compat-harness.py` around lines 139 - 163, Add defensive validation
for the --vidpid argument to prevent malformed VID:PID values being passed to
subprocess arguments: implement a helper function named _vidpid_type that
validates the string with a regex like ^[0-9a-fA-F]{4}:[0-9a-fA-F]{4}$, raises
argparse.ArgumentTypeError on mismatch, and returns the normalized (lowercased)
value; then update the parser in _build_parser() to use parser.add_argument(...,
type=_vidpid_type, metavar="XXXX:YYYY", ...) for the --vidpid option so only
valid hex VID:PID strings are accepted.
Comment on lines
+389
to
+390
| func rename(_ handle: MTPObjectHandle, to newName: String) async throws {} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider throwing MTPError.notSupported for consistency with other test doubles.
Other mock implementations of rename in the codebase (e.g., SnapshotterTests.swift, IndexCoverageTests.swift) throw MTPError.notSupported("..."). This empty no-op implementation silently succeeds, which could mask unexpected calls during testing.
Proposed fix for consistency
- func rename(_ handle: MTPObjectHandle, to newName: String) async throws {}
+ func rename(_ handle: MTPObjectHandle, to newName: String) async throws {
+ throw MTPError.notSupported("rename is not needed in this test double")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SwiftMTPKit/Tests/CoreTests/DeviceServiceCoverageTests.swift` around lines
389 - 390, The test double's rename(_ handle: MTPObjectHandle, to newName:
String) async throws {} currently silently succeeds; change it to throw
MTPError.notSupported with a descriptive message (e.g.,
MTPError.notSupported("rename called in DeviceServiceCoverageTests")) so callers
will fail fast and behavior matches other mocks like SnapshotterTests and
IndexCoverageTests; update the rename implementation in
DeviceServiceCoverageTests to throw that error.
Comment on lines
+82
to
+105
| // Task 1: acquires the transaction, holds for 50 ms, then records "1". | ||
| let t1 = Task<Void, Error> { | ||
| try await device.withTransaction { | ||
| try await Task.sleep(nanoseconds: 50_000_000) | ||
| await recorder.record(1) | ||
| } | ||
| } | ||
|
|
||
| // Let t1 start and acquire the lock before t2 is launched. | ||
| try await Task.sleep(nanoseconds: 5_000_000) | ||
|
|
||
| // Task 2: should queue behind t1 and record "2" only after t1 finishes. | ||
| let t2 = Task<Void, Error> { | ||
| try await device.withTransaction { | ||
| await recorder.record(2) | ||
| } | ||
| } | ||
|
|
||
| try await t1.value | ||
| try await t2.value | ||
|
|
||
| let observed = await recorder.order | ||
| #expect(observed == [1, 2], "t1 must complete before t2 starts its body") | ||
| } |
There was a problem hiding this comment.
Sleep-based sequencing makes the serialization test flaky.
Using a fixed delay at Line 91 does not guarantee Task 1 has entered withTransaction before Task 2 starts. Under load, this can intermittently fail with non-deterministic ordering. Gate Task 2 using an explicit signal emitted from inside Task 1’s transaction body instead of timing assumptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SwiftMTPKit/Tests/CoreTests/TransactionTests.swift` around lines 82 - 105,
The test currently uses Task.sleep to order t1 and t2 which is flaky; instead
create an explicit async signal (e.g., a CheckedContinuation/AsyncStream or an
AsyncSemaphore) that t1 resumes from inside its device.withTransaction body once
it has entered the transaction, and have the test await that signal before
starting t2. Concretely: add a continuation or semaphore variable (e.g.,
startedContinuation or startSignal) in the test, call
startSignal.resume()/startSignal.signal() inside the closure passed to
device.withTransaction in t1 immediately after acquiring the transaction,
replace the try await Task.sleep(...) with await
startSignal.wait()/withCheckedContinuation { ... } so t2 is only launched after
t1 has definitely entered the transaction, and keep recorder.record(1)/record(2)
and the final assertions unchanged.
Comment on lines
+37
to
+57
| private class MockChangeObserver: NSObject, NSFileProviderChangeObserver { | ||
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | ||
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | ||
| nonisolated(unsafe) var onFinish: (() -> Void)? | ||
|
|
||
| func didUpdate(_ items: [NSFileProviderItem]) { | ||
| updatedItems.append(contentsOf: items) | ||
| } | ||
|
|
||
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | ||
| deletedIdentifiers.append(contentsOf: identifiers) | ||
| } | ||
|
|
||
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | ||
| onFinish?() | ||
| } | ||
|
|
||
| func finishEnumeratingWithError(_ error: Error) { | ||
| onFinish?() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding explicit deinit to satisfy SwiftLint rule.
SwiftLint flags required_deinit for classes. While this is a test helper where an explicit deinit may not be strictly necessary, adding one maintains consistency with project lint rules.
Proposed fix
private class MockChangeObserver: NSObject, NSFileProviderChangeObserver {
nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = []
nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = []
nonisolated(unsafe) var onFinish: (() -> Void)?
+
+ deinit {}
func didUpdate(_ items: [NSFileProviderItem]) {📝 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
| private class MockChangeObserver: NSObject, NSFileProviderChangeObserver { | |
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | |
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | |
| nonisolated(unsafe) var onFinish: (() -> Void)? | |
| func didUpdate(_ items: [NSFileProviderItem]) { | |
| updatedItems.append(contentsOf: items) | |
| } | |
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | |
| deletedIdentifiers.append(contentsOf: identifiers) | |
| } | |
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | |
| onFinish?() | |
| } | |
| func finishEnumeratingWithError(_ error: Error) { | |
| onFinish?() | |
| } | |
| } | |
| private class MockChangeObserver: NSObject, NSFileProviderChangeObserver { | |
| nonisolated(unsafe) var updatedItems: [NSFileProviderItem] = [] | |
| nonisolated(unsafe) var deletedIdentifiers: [NSFileProviderItemIdentifier] = [] | |
| nonisolated(unsafe) var onFinish: (() -> Void)? | |
| deinit {} | |
| func didUpdate(_ items: [NSFileProviderItem]) { | |
| updatedItems.append(contentsOf: items) | |
| } | |
| func didDeleteItems(withIdentifiers identifiers: [NSFileProviderItemIdentifier]) { | |
| deletedIdentifiers.append(contentsOf: identifiers) | |
| } | |
| func finishEnumeratingChanges(upTo anchor: NSFileProviderSyncAnchor, moreComing: Bool) { | |
| onFinish?() | |
| } | |
| func finishEnumeratingWithError(_ error: Error) { | |
| onFinish?() | |
| } | |
| } |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 37-37: Classes should have an explicit deinit method
(required_deinit)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SwiftMTPKit/Tests/FileProviderTests/FileProviderEnumeratorTests.swift` around
lines 37 - 57, The MockChangeObserver test helper class is missing an explicit
deinit which triggers SwiftLint's required_deinit rule; add a deinit to the
MockChangeObserver class (class name: MockChangeObserver) — implement a deinit {
} (optionally clear references such as onFinish = nil or reset arrays
updatedItems/deletedIdentifiers) to satisfy the lint rule and keep lifecycle
cleanup explicit.
Comment on lines
+72
to
+82
| func renameObject( | ||
| _ request: RenameRequest, withReply reply: @escaping (WriteResponse) -> Void | ||
| ) { | ||
| reply(WriteResponse(success: true)) | ||
| } | ||
|
|
||
| func moveObject( | ||
| _ request: MoveObjectRequest, withReply reply: @escaping (WriteResponse) -> Void | ||
| ) { | ||
| reply(WriteResponse(success: true)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add request tracking for renameObject and moveObject for consistency.
Other mock operations (writeObject, deleteObject, createFolder) track requests in arrays, enabling test assertions on call parameters. These new methods should follow the same pattern for future testability.
♻️ Proposed fix
Add tracking properties alongside existing ones (around line 21):
var createFolderRequests: [CreateFolderRequest] = []
+ var renameRequests: [RenameRequest] = []
+ var moveRequests: [MoveObjectRequest] = []
var writeResponse = WriteResponse(success: true)Then update the methods:
func renameObject(
_ request: RenameRequest, withReply reply: `@escaping` (WriteResponse) -> Void
) {
+ renameRequests.append(request)
reply(WriteResponse(success: true))
}
func moveObject(
_ request: MoveObjectRequest, withReply reply: `@escaping` (WriteResponse) -> Void
) {
+ moveRequests.append(request)
reply(WriteResponse(success: true))
}📝 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
| func renameObject( | |
| _ request: RenameRequest, withReply reply: @escaping (WriteResponse) -> Void | |
| ) { | |
| reply(WriteResponse(success: true)) | |
| } | |
| func moveObject( | |
| _ request: MoveObjectRequest, withReply reply: @escaping (WriteResponse) -> Void | |
| ) { | |
| reply(WriteResponse(success: true)) | |
| } | |
| func renameObject( | |
| _ request: RenameRequest, withReply reply: `@escaping` (WriteResponse) -> Void | |
| ) { | |
| renameRequests.append(request) | |
| reply(WriteResponse(success: true)) | |
| } | |
| func moveObject( | |
| _ request: MoveObjectRequest, withReply reply: `@escaping` (WriteResponse) -> Void | |
| ) { | |
| moveRequests.append(request) | |
| reply(WriteResponse(success: true)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SwiftMTPKit/Tests/IntegrationTests/FileProviderIntegrationTests.swift` around
lines 72 - 82, Add request-tracking arrays for the new operations (e.g.,
renameRequests: [RenameRequest] and moveRequests: [MoveObjectRequest]) alongside
the other mock request arrays near the existing properties (around where
writeRequests/deleteRequests/createFolderRequests are declared), and update the
methods renameObject(_:withReply:) and moveObject(_:withReply:) to append the
incoming request to their respective arrays before calling
reply(WriteResponse(success: true)); this mirrors the pattern used by
writeObject, deleteObject, and createFolder for test assertions.
Comment on lines
+256
to
+264
| // Move: success (move to root) | ||
| let moveResp = await withCheckedContinuation { (c: CheckedContinuation<WriteResponse, Never>) in | ||
| impl.moveObject( | ||
| MoveObjectRequest( | ||
| deviceId: config.stableId, objectHandle: config.objectHandle, | ||
| newParentHandle: nil, newStorageId: config.storageId) | ||
| ) { c.resume(returning: $0) } | ||
| } | ||
| XCTAssertTrue(moveResp.success) |
There was a problem hiding this comment.
Move success path is currently a no-op in this fixture.
On Line 256, the test moves the object to root/same storage, but the seeded object is already root-scoped in this setup. This can return success without validating actual move behavior. Use a distinct target parent (or storage) and assert post-move location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SwiftMTPKit/Tests/XPCTests/XPCServiceCoverageTests.swift` around lines 256 -
264, The test's move success path (using impl.moveObject with MoveObjectRequest
and asserting moveResp.success) is a no-op because the seeded object is already
at the root; change the request to use a distinct target (set newParentHandle to
a different parentHandle or change newStorageId to a different storage) and
after the move, fetch the object's metadata (e.g., via the same API used
elsewhere in tests) to assert the object's parentHandle/newStorageId changed;
update the MoveObjectRequest parameters (objectHandle,
newParentHandle/newStorageId) and add an assertion that the fetched object's
location matches the intended target instead of only checking moveResp.success.
| import tempfile | ||
| import textwrap | ||
| import uuid | ||
| from typing import Any, Dict, Generator, List, Optional, Tuple |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider modernizing type annotations for Python 3.9+.
The imports use deprecated typing module types. If targeting Python 3.9+, use built-in generics and collections.abc:
♻️ Suggested update
-from typing import Any, Dict, Generator, List, Optional, Tuple
+from collections.abc import Generator
+from typing import Any, OptionalThen replace Dict[K, V] → dict[K, V], List[T] → list[T], Tuple[...] → tuple[...] throughout. This aligns with PEP 585.
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 66-66: Import from collections.abc instead: Generator
Import from collections.abc
(UP035)
[warning] 66-66: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 66-66: typing.List is deprecated, use list instead
(UP035)
[warning] 66-66: typing.Tuple is deprecated, use tuple instead
(UP035)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/compat-harness.py` at line 66, Update the type annotations to use
Python 3.9+ built-in generics and collections.abc: remove Dict, List, Tuple and
Generator from the typing import and instead use dict[..., ...], list[...],
tuple[...] in annotations and import Generator (if still needed) from
collections.abc; replace Optional[T] usages with T | None or plain union syntax,
and keep Any imported from typing if used. Update all occurrences in the file
(functions/classes referenced by name in this module) to use these modern forms
so annotations conform to PEP 585.
…uirk regressions, disconnect mapping - Add post-write verification mode (MTPDeviceActor.verifyAfterWrite) - Add MTPError.verificationFailed(expected:actual:) with actionable message - Add journal reconcile on partial reconnect (DeviceActor+Reconcile) - Add FileProvider paged enumeration (500-item pages with cursor encoding) - Add XPC disconnect error classification (xpcError(from:) in FileProviderExtension) - Add RecordedTransport replay fixture harness (RecordedMTPLink, RecordedPacket) - Add QuirkRegressionTests (4 tests), VirtualDeviceRenameTests - Add VerificationModeTests (6 tests), RecordedTransportTests (3 tests) - Fix @mainactor isolation error in FileProviderExtensionTests (makeExtension) - Fix NSFileProviderPage implicit cast errors in FileProviderEnumeratorTests - Fix NSFileProviderPage.initialPageSortedByName type cast - 1987 tests, 0 failures Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sprint 3 production-readiness epics. 1920 tests, 0 failures, 0 lint warnings.