Comprehensive Sentry telemetry for iroh/transport failures (iOS + macOS) - #9305
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… policy DiagnosticLog gains a single settable event tap delivered on the drain task (after ring retention, so selected-path dedup is respected and the hot-path record() stays untouched). DiagnosticEventPresentation decodes events into stable case names and per-code fields for telemetry sinks. Pure TransportIncidentPolicy turns the failure stream into a bounded set of reportable incidents: per-signature cooldown with coalesced counts, hourly capture budget, sustained-streak outage escalation, and suppression of attributable noise (cancelled/superseded churn, offline-while-unreachable, idle timeout while backgrounded). pairFail now records the classified DiagnosticFailureKind in its b slot so pairing failures group by cause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TransportSentryReporter (CmuxSentryReporting) consumes the DiagnosticLog tap: every retained event becomes a scrubbed breadcrumb and a budget-limited structured log line, and failures that cross TransportIncidentPolicy's gates become Sentry events fingerprinted by code/failure/transport signature with the compact diagnostic ring export attached, so one issue carries the full connection timeline that previously had to be pulled off the device by hand. iOS gains the shared last-mile scrubber it was waiting on: beforeSend now scrubs (in addition to the consent gate), beforeBreadcrumb and beforeSendLog are installed, and enableLogs is on; swizzling and automatic network capture stay off. macOS enables logs, scrubs them, and taps the Mac host's hostDiagnosticLog with role macHost after SentrySDK.start. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds shared diagnostic presentation and incident policy types, privacy scrubbing, and a Sentry transport reporter. It connects these components to iOS, Mac, CLI, and Xcode targets with consent controls, event taps, structured logs, attachments, and tests. ChangesDiagnostic event processing
Telemetry and Sentry integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant DiagnosticLog
participant TransportSentryReporter
participant TransportIncidentPolicy
participant DiagnosticRing
participant Sentry
DiagnosticLog->>TransportSentryReporter: Ingest retained diagnostic event
TransportSentryReporter->>TransportIncidentPolicy: Decide incident outcome
TransportSentryReporter->>Sentry: Send breadcrumb and admitted log
TransportSentryReporter->>DiagnosticRing: Export ring for qualifying incident
DiagnosticRing-->>TransportSentryReporter: Return diagnostic data
TransportSentryReporter->>Sentry: Capture incident with metadata and attachment
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs/transport-sentry-diagnostics.md`:
- Around line 3-9: Qualify the Sentry coverage claim in the transport
diagnostics documentation to state that Sentry contains only policy-eligible
telemetry. Explicitly note that delivery may be disabled and that policy,
cooldown, or hourly-budget rules can suppress events; remove the implication
that every transport failure is guaranteed to appear.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 8234-8237: Update the exhausted-connect handling in
MobileShellComposite so one failed connect advances TransportIncidentPolicy’s
streakCount and signature cooldown only once. Remove the duplicate reportable
diagnostic event or route one event outside the streak/signature policy, while
preserving the intended failure kind and exhausted-connect diagnostics.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventPresentation.swift`:
- Around line 122-125: Add .routeUnavailable to the codesWithTransportA set and
update decodeA(_:code:) so this code follows the branch that decodes a using
DiagnosticTransportKind and returns the key "transport", preserving transport
data for route-unavailable incidents.
In `@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticLog.swift`:
- Around line 149-150: The setEventTap method can install an observer after
record queues an event but before the drain delivers it, causing the new
observer to receive events recorded before registration. Fix this by assigning
an ingress sequence ID to each event under the ingress lock in the record
method, storing the last admitted sequence ID when setEventTap installs the
observer via tap.set, and filtering delivered events to only those with sequence
IDs greater than the stored ID. Add a regression test that records an event
before calling setEventTap and verifies the new observer does not receive the
pre-registration event.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TransportIncidentPolicy.swift`:
- Around line 297-307: Make SignatureState.lastCaptureTNanos optional so a
budget-rejected signature can be tracked without starting a cooldown; initialize
new rejected states with nil, and update the cooldown logic to apply only when a
capture timestamp exists. Extend hourlyBudgetDropsAndReports or its related
tests with a non-zero signatureCooldown, a new signature dropped by the budget,
and a later capture after the budget window slides.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swift`:
- Around line 157-185: Update scrub(_:) to write scrubbed values using
SentryLog.Attribute(string:) rather than SentryAttribute(string:), matching the
attribute type stored by SentryLog. Extend stringValues collection and writeback
to include string-array attributes, ensuring arrays are passed through the
scrubber and restored without bypassing redaction.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportSentryReporter.swift`:
- Around line 181-191: Update captureIncident to retain each detached capture
task using the reporter’s existing synchronization mechanism and pending-task
storage, rather than leaving it fire-and-forget. Add or connect shutdown
handling so pending incident captures can be awaited or drained with a bounded
timeout before termination, while preserving the existing exportRing, attachment
creation, and delivery.capture behavior.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swift`:
- Around line 30-35: Update captureGroup(_:) so its existing bounds guard
rejects negative indices as well as indices at or beyond result.numberOfRanges
before calling result.range(at:). Preserve the current nil return behavior for
all invalid or unmatched ranges.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swift`:
- Around line 187-197: Update the recursive scrubber’s doc comment to remove
Data from the safe scalars that pass through untouched, and document that Data
values are replaced with redactedData. Keep the existing implementation behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32a9aabd-ce70-4c79-bf6d-863af90e9a5f
⛔ Files ignored due to path filters (4)
Packages/Shared/CmuxSentryTelemetry/Package.resolvedis excluded by!**/Package.resolvedPackages/iOS/CmuxMobileCrashReporting/Package.resolvedis excluded by!**/Package.resolvedcmux.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/cmuxPackage/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (29)
CLI/CLISocketSentryTelemetry.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventCode.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventPresentation.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticLog.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TransportIncidentPolicy.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/DiagnosticEventPresentationTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/DiagnosticLogTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/TransportIncidentPolicyTests.swiftPackages/Shared/CmuxSentryTelemetry/Package.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportSentryReporter.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportTelemetryLogBudget.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/ScrubberDenylists.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexPattern.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryReportingTests/SentryEventScrubberTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryReportingTests/TransportSentryReporterTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryScrubbingTests/ScrubberDenylistsTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryScrubbingTests/SentryScrubberTests.swiftPackages/iOS/CmuxMobileCrashReporting/Package.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashReporter.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/MobileCrashReporterTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftSources/AppDelegate.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MacSentryStartupPolicyTests.swiftdocs/transport-sentry-diagnostics.mdios/cmux/AppCompositionRoot.swift
Comment on lines
+181
to
+191
| private func captureIncident(_ incident: TransportIncidentPolicy.Incident) { | ||
| Task.detached(priority: .utility) { [self] in | ||
| let ring = await exportRing() | ||
| let attachment = ring.isEmpty ? nil : Attachment( | ||
| data: ring, | ||
| filename: "cmux-transport-diag.txt", | ||
| contentType: "text/plain" | ||
| ) | ||
| delivery.capture(makeEvent(incident), attachment) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Track the incident-capture task so a quit does not silently drop it.
captureIncident starts a fire-and-forget Task.detached to export the ring and call delivery.capture. Nothing stores, awaits, or cancels this task. If the app quits (Cmd+Q on macOS, or process termination on iOS) while the task is mid-flight, the ring export or the Sentry capture can be interrupted. This drops exactly the failure or outage diagnostic the feature exists to deliver, and it can happen more often than average during real connectivity incidents, since those incidents correlate with app restarts and force-quits.
Store each task (for example in a Set<Task<Void, Never>> guarded by the existing lock) so a shutdown path can wait for pending captures to finish, or drain them with a bounded timeout before the process exits.
♻️ Proposed direction
private struct MutableState: Sendable {
var policy: TransportIncidentPolicy
var logBudget: TransportTelemetryLogBudget
+ var pendingCaptures: [UUID: Task<Void, Never>] = [:]
}
private func captureIncident(_ incident: TransportIncidentPolicy.Incident) {
- Task.detached(priority: .utility) { [self] in
+ let taskId = UUID()
+ let task = Task.detached(priority: .utility) { [self] in
let ring = await exportRing()
let attachment = ring.isEmpty ? nil : Attachment(
data: ring,
filename: "cmux-transport-diag.txt",
contentType: "text/plain"
)
delivery.capture(makeEvent(incident), attachment)
+ state.withLock { $0.pendingCaptures.removeValue(forKey: taskId) }
}
+ state.withLock { $0.pendingCaptures[taskId] = task }
}
+
+ /// Awaits pending incident captures. Call before process/app termination
+ /// so an in-flight ring export + Sentry capture is not interrupted.
+ public func drainPendingCaptures() async {
+ let tasks = state.withLock { Array($0.pendingCaptures.values) }
+ for task in tasks {
+ await task.value
+ }
+ }Based on learnings, this repo's coding guidelines state: "Do not create fire-and-forget Task { ... } work with meaningful lifecycle unless it is stored, cancellable, or tied to a caller-owned operation."
🤖 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
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportSentryReporter.swift`
around lines 181 - 191, Update captureIncident to retain each detached capture
task using the reporter’s existing synchronization mechanism and pending-task
storage, rather than leaving it fire-and-forget. Add or connect shutdown
handling so pending incident captures can be awaited or drained with a bounded
timeout before termination, while preserving the existing exportRing, attachment
creation, and delivery.capture behavior.
Source: Path instructions
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🤖 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 `@docs/transport-sentry-diagnostics.md`:
- Around line 3-9: Qualify the Sentry coverage claim in the transport
diagnostics documentation to state that Sentry contains only policy-eligible
telemetry. Explicitly note that delivery may be disabled and that policy,
cooldown, or hourly-budget rules can suppress events; remove the implication
that every transport failure is guaranteed to appear.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 8234-8237: Update the exhausted-connect handling in
MobileShellComposite so one failed connect advances TransportIncidentPolicy’s
streakCount and signature cooldown only once. Remove the duplicate reportable
diagnostic event or route one event outside the streak/signature policy, while
preserving the intended failure kind and exhausted-connect diagnostics.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventPresentation.swift`:
- Around line 122-125: Add .routeUnavailable to the codesWithTransportA set and
update decodeA(_:code:) so this code follows the branch that decodes a using
DiagnosticTransportKind and returns the key "transport", preserving transport
data for route-unavailable incidents.
In `@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticLog.swift`:
- Around line 149-150: The setEventTap method can install an observer after
record queues an event but before the drain delivers it, causing the new
observer to receive events recorded before registration. Fix this by assigning
an ingress sequence ID to each event under the ingress lock in the record
method, storing the last admitted sequence ID when setEventTap installs the
observer via tap.set, and filtering delivered events to only those with sequence
IDs greater than the stored ID. Add a regression test that records an event
before calling setEventTap and verifies the new observer does not receive the
pre-registration event.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TransportIncidentPolicy.swift`:
- Around line 297-307: Make SignatureState.lastCaptureTNanos optional so a
budget-rejected signature can be tracked without starting a cooldown; initialize
new rejected states with nil, and update the cooldown logic to apply only when a
capture timestamp exists. Extend hourlyBudgetDropsAndReports or its related
tests with a non-zero signatureCooldown, a new signature dropped by the budget,
and a later capture after the budget window slides.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swift`:
- Around line 157-185: Update scrub(_:) to write scrubbed values using
SentryLog.Attribute(string:) rather than SentryAttribute(string:), matching the
attribute type stored by SentryLog. Extend stringValues collection and writeback
to include string-array attributes, ensuring arrays are passed through the
scrubber and restored without bypassing redaction.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportSentryReporter.swift`:
- Around line 181-191: Update captureIncident to retain each detached capture
task using the reporter’s existing synchronization mechanism and pending-task
storage, rather than leaving it fire-and-forget. Add or connect shutdown
handling so pending incident captures can be awaited or drained with a bounded
timeout before termination, while preserving the existing exportRing, attachment
creation, and delivery.capture behavior.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swift`:
- Around line 30-35: Update captureGroup(_:) so its existing bounds guard
rejects negative indices as well as indices at or beyond result.numberOfRanges
before calling result.range(at:). Preserve the current nil return behavior for
all invalid or unmatched ranges.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swift`:
- Around line 187-197: Update the recursive scrubber’s doc comment to remove
Data from the safe scalars that pass through untouched, and document that Data
values are replaced with redactedData. Keep the existing implementation behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32a9aabd-ce70-4c79-bf6d-863af90e9a5f
⛔ Files ignored due to path filters (4)
Packages/Shared/CmuxSentryTelemetry/Package.resolvedis excluded by!**/Package.resolvedPackages/iOS/CmuxMobileCrashReporting/Package.resolvedis excluded by!**/Package.resolvedcmux.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/cmuxPackage/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (29)
CLI/CLISocketSentryTelemetry.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventCode.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventPresentation.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticLog.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TransportIncidentPolicy.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/DiagnosticEventPresentationTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/DiagnosticLogTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/TransportIncidentPolicyTests.swiftPackages/Shared/CmuxSentryTelemetry/Package.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportSentryReporter.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/TransportTelemetryLogBudget.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/ScrubberDenylists.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexPattern.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryReportingTests/SentryEventScrubberTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryReportingTests/TransportSentryReporterTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryScrubbingTests/ScrubberDenylistsTests.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryScrubbingTests/SentryScrubberTests.swiftPackages/iOS/CmuxMobileCrashReporting/Package.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashReporter.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/MobileCrashReporterTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftSources/AppDelegate.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MacSentryStartupPolicyTests.swiftdocs/transport-sentry-diagnostics.mdios/cmux/AppCompositionRoot.swift
🛑 Comments failed to post (2)
Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swift (1)
30-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Complete the bounds check on
captureGroup.The guard checks only the upper bound. A negative
indexpasses it and reachesresult.range(at: index), which is defined only for0..<numberOfRangesand traps outside that range.captureGroupis public API ofCmuxSentryScrubbing, so an out-of-module caller can pass a negative index. A trap here runs inside a SentrybeforeSendhook and crashes the host app.Add the lower bound to the existing guard.
🛡️ Proposed fix
public func captureGroup(_ index: Int) -> String? { - guard index < result.numberOfRanges else { return nil } + guard index >= 0, index < result.numberOfRanges else { return nil } let range = result.range(at: index) guard range.location != NSNotFound, range.length >= 0 else { return nil } return source.substring(with: range) }📝 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.public func captureGroup(_ index: Int) -> String? { guard index >= 0, index < result.numberOfRanges else { return nil } let range = result.range(at: index) guard range.location != NSNotFound, range.length >= 0 else { return nil } return source.substring(with: range) }🤖 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 `@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryRegexMatch.swift` around lines 30 - 35, Update captureGroup(_:) so its existing bounds guard rejects negative indices as well as indices at or beyond result.numberOfRanges before calling result.range(at:). Preserve the current nil return behavior for all invalid or unmatched ranges.Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swift (1)
187-197: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the
Dataclaim in the doc comment.Line 190 lists
Dataamong the safe scalars that "pass through untouched". The implementation at lines 209-212 replaces everyDatavalue withredactedData. The two statements contradict each other inside the same doc comment.This matters because the doc describes a privacy decision. A maintainer who trusts line 190 would conclude that binary payloads are forwarded, and could reintroduce a pass-through case.
📝 Proposed doc fix
/// Strings are scrubbed; dictionaries and arrays are walked; safe scalars - /// (`NSNumber`/`Bool`/`Int`/`Double`, `Date`, `Data`, `NSNull`) pass through - /// untouched. Any other object (notably `URL` / `NSURL`, which carry a file + /// (`NSNumber`/`Bool`/`Int`/`Double`, `Date`, `NSNull`) pass through + /// untouched. `Data` is replaced wholesale with ``redactedData``, because + /// Sentry serializes it to a hex description that the string rules cannot + /// reach. Any other object (notably `URL` / `NSURL`, which carry a file /// path) is converted to its string form and scrubbed, because Sentry📝 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./// Recursively scrubs every string found inside a JSON-like value tree. /// /// Strings are scrubbed; dictionaries and arrays are walked; safe scalars /// (`NSNumber`/`Bool`/`Int`/`Double`, `Date`, `NSNull`) pass through /// untouched. `Data` is replaced wholesale with ``redactedData``, because /// Sentry serializes it to a hex description that the string rules cannot /// reach. Any other object (notably `URL` / `NSURL`, which carry a file /// path) is converted to its string form and scrubbed, because Sentry /// serializes unsupported Foundation objects to their description *after* /// `beforeSend` runs, which would otherwise leak the unscrubbed path. /// /// - Parameter value: A `String`, `[String: Any]`, `[Any]`, or scalar. /// - Returns: The value with all nested strings scrubbed.🤖 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 `@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryScrubbing/SentryScrubber.swift` around lines 187 - 197, Update the recursive scrubber’s doc comment to remove Data from the safe scalars that pass through untouched, and document that Data values are replaced with redactedData. Keep the existing implementation behavior unchanged.
…ys, single pairFail The event tap now gates on an ingress admission sequence: installing an observer while recorded events are still queued on the drain task no longer delivers those pre-installation events (regression test records a 500-event burst and installs the tap with no drain sync). A budget-dropped capture no longer stamps lastCaptureTNanos, so a brand-new failure signature arriving during budget exhaustion captures as soon as the window slides instead of serving a phantom cooldown. The structured-log scrubber now handles string-array attributes (previously bypassed) and writes back via SentryLog.Attribute. One exhausted connect now records a single pairFail carrying transport (a) and failure (b) instead of a pairFail+rpcFailed pair that double-counted the outage streak; pairFail and routeUnavailable decode their transport slot in presentation. The iOS workspace lockfile aligns sentry-cocoa to 9.24.0, matching the package-local pins (fixes the SwiftPM lockfile policy guard). Doc states coverage is policy-shaped, not per-event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Collaborator
Author
|
Addressed the CodeRabbit round in d0087f3:
Skipped: tracking the incident-capture |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/transport-sentry-diagnostics.md (1)
29-32: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the runtime role in the fingerprint.
The Sentry fingerprint includes
roleNameandincident.signature. State that the fingerprint contains the runtime role plus thecode/failureKind/transportKindsignature.Proposed wording
-fingerprinted by `code/failureKind/transportKind` signature +fingerprinted by runtime role plus the `code/failureKind/transportKind` signature🤖 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 `@docs/transport-sentry-diagnostics.md` around lines 29 - 32, Update the “Error events” documentation to state that Sentry fingerprints include the runtime role (roleName) together with the code/failureKind/transportKind incident.signature. Preserve the existing description of the compact diagnostic ring export.Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swift (1)
157-197: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScrub all typed log attributes and preserve scalar attributes.
scrub(_:)only collectsStringand[String]attribute values, so a sensitive key with integer, double, or bool attributes is not sent throughscrub(dictionary:). The key-aware dictionary path already redacts sensitive keys by name and keeps safe scalars unchanged, so pass every supported attribute value through it and add a regression case for sensitive numeric attributes.🤖 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 `@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swift` around lines 157 - 197, Update CmuxSentryReporting.scrub(_:) to include every supported log attribute value, not only String and [String], when building the dictionary passed to scrubber.scrub(dictionary:). Preserve scalar numeric and boolean values when the dictionary scrubber returns them, while retaining the existing String and [String] attribute reconstruction; add a regression case covering redaction of a sensitive key with a numeric attribute.
🤖 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 `@docs/transport-sentry-diagnostics.md`:
- Around line 7-8: Update the breadcrumb descriptions in
docs/transport-sentry-diagnostics.md, including the additional wording around
the referenced retained-event passage, to state that delivery includes only
transport events admitted after DiagnosticLog.setEventTap(_:) installation.
Replace claims that imply all retained events are delivered while preserving the
existing explanation of breadcrumb scope.
---
Outside diff comments:
In `@docs/transport-sentry-diagnostics.md`:
- Around line 29-32: Update the “Error events” documentation to state that
Sentry fingerprints include the runtime role (roleName) together with the
code/failureKind/transportKind incident.signature. Preserve the existing
description of the compact diagnostic ring export.
In
`@Packages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swift`:
- Around line 157-197: Update CmuxSentryReporting.scrub(_:) to include every
supported log attribute value, not only String and [String], when building the
dictionary passed to scrubber.scrub(dictionary:). Preserve scalar numeric and
boolean values when the dictionary scrubber returns them, while retaining the
existing String and [String] attribute reconstruction; add a regression case
covering redaction of a sensitive key with a numeric attribute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af68c160-c04c-442c-b716-5d77b8406693
⛔ Files ignored due to path filters (1)
ios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (10)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventCode.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticEventPresentation.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticLog.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TransportIncidentPolicy.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/DiagnosticLogTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/TransportIncidentPolicyTests.swiftPackages/Shared/CmuxSentryTelemetry/Sources/CmuxSentryReporting/SentryEventScrubber.swiftPackages/Shared/CmuxSentryTelemetry/Tests/CmuxSentryReportingTests/SentryEventScrubberTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftdocs/transport-sentry-diagnostics.md
Comment on lines
+7
to
+8
| budgets, and structured logs pass their own budget. Breadcrumbs are the widest | ||
| net (every retained transport event, attached to whatever ships next). The |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the event-tap delivery boundary.
DiagnosticLog.setEventTap(_:) does not replay events admitted before tap installation. The test eventTapNeverDeliversEventsQueuedBeforeInstallation covers this behavior. Replace “every retained transport event” and “each retained event” with wording that limits delivery to events admitted after tap installation.
Proposed wording
-`DiagnosticLog.setEventTap(_:)` delivers each retained event
+`DiagnosticLog.setEventTap(_:)` delivers events admitted after tap installationAlso applies to: 18-25
🤖 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 `@docs/transport-sentry-diagnostics.md` around lines 7 - 8, Update the
breadcrumb descriptions in docs/transport-sentry-diagnostics.md, including the
additional wording around the referenced retained-event passage, to state that
delivery includes only transport events admitted after
DiagnosticLog.setEventTap(_:) installation. Replace claims that imply all
retained events are delivered while preserving the existing explanation of
breadcrumb scope.
This was referenced Aug 3, 2026
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
1 participant
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.
Every iroh/transport failure a user can hit is now diagnosable from Sentry alone, on both the iOS client and the macOS host, without pulling logs off the device by hand.
The existing
DiagnosticLogring (fixed integer taxonomy, privacy-safe by construction) gains a single event tap delivered on its drain task. A new sharedTransportSentryReporter(Packages/Shared/CmuxSentryTelemetry) consumes it and emits three surfaces: a scrubbed breadcrumb per event (categorytransport, decoded names, so crashes and hangs carry the recent connection timeline), a budget-limited Sentry structured log line per event (enableLogs, 300/hour sliding budget so retry storms cannot flood quota), and, for failures that cross the pureTransportIncidentPolicygates, a Sentry event fingerprinted bycode/failureKind/transportKindsignature with the compactcmuxdiag v1ring export attached. The policy coalesces repeats behind a 10-minute per-signature cooldown, caps failure captures at 30/hour, escalates 5+ consecutive failures over 60s with no success into one error-severitytransport-outageissue, and suppresses what an operator can already attribute (cancelled/superseded churn, offline while reachability reports no network, idle timeout while backgrounded). Reachability, app phase, streak counts, and seconds-since-last-success ride on every capture.To unlock iOS, the pure scrubbing layer moved from the macOS-only CmuxFoundation to the shared package (
CmuxSentryScrubbing+CmuxSentryReportingglue; mac app and CLI re-linked, existing tests moved). iOS crash reporting now installsbeforeSendscrubbing (plus the existing consent gate),beforeBreadcrumb,beforeSendLog, andenableLogs; swizzling and automatic network capture stay off. macOS enables logs and tapsMobileHostIrohRuntime.hostDiagnosticLog(rolemacHost) afterSentrySDK.start.pairFailnow records the classifiedDiagnosticFailureKindin itsbslot so pairing failures group by cause. Design notes indocs/transport-sentry-diagnostics.md.Tests: 340 CMUXMobileCore (new tap/presentation/policy suites), 65 CmuxSentryTelemetry (reporter, budget, log scrub, moved scrubber suites), 16 CmuxMobileCrashReporting (new scrub/log-gate contract tests). Verified live on the tagged mac build:
category = transportbreadcrumbs in the SDK debug stream, envelopes written and accepted by ingest, log-batch envelopes at transport-activity timestamps, and the host ring exporting a realrelayPolicyRefreshFailed/policyUnavailablefailure.No user-facing strings added (telemetry only); localization audit not applicable.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds end-to-end Sentry telemetry for iroh/transport failures on iOS and macOS so we can diagnose issues from Sentry alone, without pulling device logs. Events are scrubbed, budgeted, grouped, and outage‑escalated to reduce noise and protect privacy.
New Features
DiagnosticLog.setEventTap(_:)to stream retained events to telemetry sinks.DiagnosticEventPresentationfor stable, human-readable event names and fields.TransportIncidentPolicyto throttle and group failures (10‑min per-signature cooldown, 30/hour cap) and escalate outages (5+ consecutive failures in 60s) while suppressing attributable noise (cancelled/superseded, offline, background idle).CmuxSentryTelemetrypackage:CmuxSentryScrubbingfor shared, testable value scrubbing.CmuxSentryReportingwithTransportSentryReporterthat emits:transport) per event.code/failureKind/transportKindand attaching the compact ring export.beforeSend,beforeBreadcrumb,beforeSendLog, andenableLogs; swizzling and automatic network capture remain off.SentrySDK.startand taps the host transport ring; carries reachability, app phase, streaks, and time-since-last-success on captures.pairFailrecordsDiagnosticFailureKindfor better grouping.docs/transport-sentry-diagnostics.md) and extensive tests acrossCMUXMobileCore,CmuxSentryTelemetry, and iOS crash reporting.Bug Fixes
SentryLog.Attribute.pairFailis recorded per exhausted connect (with transport inaandDiagnosticFailureKindinb), avoiding double-counted outage streaks; presentation decodes transport forpairFailandrouteUnavailable.sentry-cocoa9.24.0; introducedCmuxSentryTelemetryand linked where needed.Written for commit d0087f3. Summary will update on new commits.
Summary by CodeRabbit