Repository navigation
iOS: add in-app share diagnostics - #7006
Closed
austinywang wants to merge 45 commits into
Closed
austinywang wants to merge 45 commits into
austinywang wants to merge 45 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds auth failure tracking, mobile diagnostics data/reporting, in-app diagnostics sharing, and compiler-gated chat UI updates. ChangesDiagnostics sharing and auth telemetry
Chat UI compatibility adjustments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 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 |
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv # ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift
Contributor
Greptile SummaryAdds an in-app "Share Diagnostics" sheet for iOS that lets users copy or share a redacted diagnostics report from the workspace menu, and wires up the supporting infrastructure across auth, connection, and shell layers.
Confidence Score: 4/5The diagnostics infrastructure is carefully designed with proper scrubbing, account-boundary hygiene, and privacy guards; one auth-state cleanup gap noted in a prior review cycle remains open.
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant WorkspaceDetailView
participant WorkspaceDiagnosticsSheet
participant MobileShellComposite
participant AuthCoordinator
participant MobileDiagnosticsEventLog
participant OSLogStoreReader
participant MobileDiagnosticsReportBuilder
User->>WorkspaceDetailView: Tap "Share Diagnostics"
WorkspaceDetailView->>WorkspaceDiagnosticsSheet: present sheet
WorkspaceDiagnosticsSheet->>WorkspaceDiagnosticsSheet: .task → loadReport()
WorkspaceDiagnosticsSheet->>MobileShellComposite: diagnosticsEventLog?.snapshot()
MobileShellComposite->>MobileDiagnosticsEventLog: snapshot() [actor hop]
MobileDiagnosticsEventLog-->>WorkspaceDiagnosticsSheet: [MobileDiagnosticsEvent]
WorkspaceDiagnosticsSheet->>AuthCoordinator: isAuthenticated, lastAuthError
WorkspaceDiagnosticsSheet->>MobileShellComposite: connectionState, connectedHostName, lastConnectionError
WorkspaceDiagnosticsSheet->>OSLogStoreReader: recentEntries(since:limit:120) [Task.detached]
OSLogStoreReader-->>WorkspaceDiagnosticsSheet: [MobileDiagnosticsOSLogEntry] (message bodies omitted)
WorkspaceDiagnosticsSheet->>MobileDiagnosticsReportBuilder: buildReport() [Task.detached]
MobileDiagnosticsReportBuilder->>MobileDiagnosticsReportBuilder: scrub(rawReport)
MobileDiagnosticsReportBuilder-->>WorkspaceDiagnosticsSheet: String report
WorkspaceDiagnosticsSheet-->>User: Show selectable text + Copy/Share buttons
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant WorkspaceDetailView
participant WorkspaceDiagnosticsSheet
participant MobileShellComposite
participant AuthCoordinator
participant MobileDiagnosticsEventLog
participant OSLogStoreReader
participant MobileDiagnosticsReportBuilder
User->>WorkspaceDetailView: Tap "Share Diagnostics"
WorkspaceDetailView->>WorkspaceDiagnosticsSheet: present sheet
WorkspaceDiagnosticsSheet->>WorkspaceDiagnosticsSheet: .task → loadReport()
WorkspaceDiagnosticsSheet->>MobileShellComposite: diagnosticsEventLog?.snapshot()
MobileShellComposite->>MobileDiagnosticsEventLog: snapshot() [actor hop]
MobileDiagnosticsEventLog-->>WorkspaceDiagnosticsSheet: [MobileDiagnosticsEvent]
WorkspaceDiagnosticsSheet->>AuthCoordinator: isAuthenticated, lastAuthError
WorkspaceDiagnosticsSheet->>MobileShellComposite: connectionState, connectedHostName, lastConnectionError
WorkspaceDiagnosticsSheet->>OSLogStoreReader: recentEntries(since:limit:120) [Task.detached]
OSLogStoreReader-->>WorkspaceDiagnosticsSheet: [MobileDiagnosticsOSLogEntry] (message bodies omitted)
WorkspaceDiagnosticsSheet->>MobileDiagnosticsReportBuilder: buildReport() [Task.detached]
MobileDiagnosticsReportBuilder->>MobileDiagnosticsReportBuilder: scrub(rawReport)
MobileDiagnosticsReportBuilder-->>WorkspaceDiagnosticsSheet: String report
WorkspaceDiagnosticsSheet-->>User: Show selectable text + Copy/Share buttons
Reviews (25): Last reviewed commit: "Import mobile support in diagnostics ass..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
Addressed Greptile's diagnostics notes in db955e0: retained connection errors are now scrubbed before storage, and the OSLog reader now uses a fixed-size ring buffer instead of repeated removeFirst trimming. |
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift (1)
222-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord offline preflight failures too.
requireOnline()still runs before each newdo/catch, soAuthError.offlineexits these entrypoints without ever hittingrecordAuthError(_). That leaveslastAuthErrorstale/nil for one of the most common sign-in failures, so the diagnostics report can miss the actual auth failure that just happened.Also applies to: 248-268, 273-289, 302-323
🤖 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/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift` around lines 222 - 243, sendCode currently throws from requireOnline() before the do/catch block, so offline failures bypass recordAuthError and leave lastAuthError stale. Update sendCode (and the same pattern in the other auth entrypoints like the ones using runPhase and recordAuthError) to catch the requireOnline() failure and pass it through recordAuthError(_), ensuring AuthError.offline is recorded before rethrowing.
🤖 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
`@Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsAppInfo.swift`:
- Around line 45-46: The report-facing strings in MobileDiagnosticsAppInfo are
currently hardcoded in English, including buildStamp and the "unknown"
fallbacks. Update the MobileDiagnosticsAppInfo properties so they either return
structured values only or use localized strings via the app’s localization flow,
and keep any final user-facing formatting in the report builder. Make sure the
symbols buildStamp, bundleIdentifier, deviceModel, and osVersion no longer
expose plain English fallback text directly.
In
`@Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsReportBuilder.swift`:
- Around line 60-82: The diagnostics report builder is still emitting hardcoded
English section headers and fallback values, so the report body is not
localized. Update MobileDiagnosticsReportBuilder to route all user-facing
strings in the report through String(localized:) keys, including section titles
and sentinel values like “none” and status labels. Add the matching entries to
Resources/Localizable.xcstrings for every supported locale, and apply the same
localization approach to the related report-building content referenced in the
same builder.
In
`@Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsSecretScrubber.swift`:
- Around line 36-61: The secret redaction rules in
MobileDiagnosticsSecretScrubber.secretPatterns only handle key=value and %3D
formats, so JSON-style fields can still leak through. Update the scrubber to
match and redact quoted JSON key/value pairs for the same sensitive keys (for
example access_token, password, token, secret, api_key, client_secret, etc.)
while keeping the existing replacements consistent. Ensure the new pattern is
added alongside the current regex list so
MobileDiagnosticsReportBuilder.buildReport benefits from redaction in shared
report payloads.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDiagnosticsSheet.swift`:
- Around line 109-113: The connection diagnostics builder in
WorkspaceDiagnosticsSheet is mixing two error sources by falling back from
lastConnectionError to store.connectionError, which can bypass the retained
scrubbed value. Update the MobileDiagnosticsConnectionState construction to rely
only on the authoritative diagnostics field already stored for this feature, or
ensure the live error is scrubbed and recorded before it reaches this builder.
Keep the fix localized around the connection state report so the diagnostics
path stays single-sourced and consistent.
- Around line 82-89: Move the diagnostics assembly work out of the main actor in
WorkspaceDiagnosticsSheet.buildReport(). Keep only the actor-bound state capture
on `@MainActor`, then hand off the UTF-8 decoding of the exported diagnostics blob
and the MobileDiagnosticsReportBuilder report construction to a nonisolated
helper or Task.detached before setting reportText. Use buildReport() and
MobileDiagnosticsReportBuilder as the key locations to refactor, and avoid doing
String(decoding: ...) plus final report assembly on the main thread.
In
`@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift`:
- Around line 42-47: The shared diagnostics field lastAuthError is still falling
back to raw Swift type names for non-AuthError failures, which can leak
vendor/internal identifiers. Update AuthCoordinator’s auth-failure mapping logic
so the setter only stores curated, display-safe categories for unknown errors
instead of String(describing: type(of: error)), and apply the same replacement
to the other affected assignment path around the later auth error handling
block.
---
Outside diff comments:
In
`@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift`:
- Around line 222-243: sendCode currently throws from requireOnline() before the
do/catch block, so offline failures bypass recordAuthError and leave
lastAuthError stale. Update sendCode (and the same pattern in the other auth
entrypoints like the ones using runPhase and recordAuthError) to catch the
requireOnline() failure and pass it through recordAuthError(_), ensuring
AuthError.offline is recorded before rethrowing.
🪄 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
Run ID: 0471ee5c-0c06-445b-a943-89d03b06fb69
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (24)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+SessionPriming.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+Tokens.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthError.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsAppInfo.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsAuthState.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsConnectionState.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsEvent.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsEventLog.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsOSLogEntry.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsOSLogReading.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsOSLogStoreReader.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsReportBuilder.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsSecretScrubber.swiftPackages/iOS/CmuxMobileDiagnostics/Tests/CmuxMobileDiagnosticsTests/MobileDiagnosticsEventLogTests.swiftPackages/iOS/CmuxMobileDiagnostics/Tests/CmuxMobileDiagnosticsTests/MobileDiagnosticsReportBuilderTests.swiftPackages/iOS/CmuxMobileDiagnostics/Tests/CmuxMobileDiagnosticsTests/MobileDiagnosticsSecretScrubberTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDiagnosticsSheet.swiftios/cmux/AppCompositionRoot.swiftios/cmux/Resources/Localizable.xcstringsios/cmux/cmuxApp.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift
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)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift (2)
42-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
lastAuthErrordoc comment to match the new fallback.The comment still says unknown failures retain the thrown Swift error type, but
diagnosticDescription(for:)now always collapses them toAuthError.authFailure.Suggested diff
- /// Raw backend errors can contain opaque identifiers or token-adjacent - /// payloads, so diagnostics retain only ``AuthError`` case names or the - /// thrown Swift error type when no display-safe mapping exists. + /// Raw backend errors can contain opaque identifiers or token-adjacent + /// payloads, so diagnostics retain only ``AuthError`` case names or the + /// fallback category `AuthError.authFailure` when no display-safe mapping exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift` around lines 42 - 46, Update the `lastAuthError` documentation in `AuthCoordinator` so it matches `diagnosticDescription(for:)`’s current behavior: unknown failures should no longer be described as preserving the thrown Swift error type, but instead as collapsing to `AuthError.authFailure`. Keep the comment aligned with the actual fallback path used for diagnostics and reference the `lastAuthError` property and `diagnosticDescription(for:)` helper when editing.
631-636: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute external sign-in failures through
recordAuthError(_).completeExternalSignIn()is reached fromPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlow.swift, but itscatchbypasses the shared diagnostics path, so seeded sign-in failures never updatelastAuthError. ReuserecordAuthError(error)here instead of rethrowing the mapped error directly.🤖 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/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift` around lines 631 - 636, The external sign-in failure path is bypassing the shared diagnostics update, so seeded failures never set lastAuthError. Update completeExternalSignIn() in AuthCoordinator to route the caught error through recordAuthError(_:) before propagating it, instead of rethrowing the mapped error directly. Use the existing recordAuthError(_:) helper and the completeExternalSignIn()/HostBrowserSignInFlow flow to keep the diagnostics path consistent.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDiagnosticsSheet.swift`:
- Around line 107-110: The connection state serialization in
WorkspaceDiagnosticsSheet uses String(describing:) on store.connectionState,
which leaks enum case names into the report. Update the
MobileDiagnosticsConnectionState construction to map each connection state
through the existing localized mobile.connection.* strings before assigning
state, keeping user-facing export text fully localized and stable against enum
renames.
---
Outside diff comments:
In
`@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift`:
- Around line 42-46: Update the `lastAuthError` documentation in
`AuthCoordinator` so it matches `diagnosticDescription(for:)`’s current
behavior: unknown failures should no longer be described as preserving the
thrown Swift error type, but instead as collapsing to `AuthError.authFailure`.
Keep the comment aligned with the actual fallback path used for diagnostics and
reference the `lastAuthError` property and `diagnosticDescription(for:)` helper
when editing.
- Around line 631-636: The external sign-in failure path is bypassing the shared
diagnostics update, so seeded failures never set lastAuthError. Update
completeExternalSignIn() in AuthCoordinator to route the caught error through
recordAuthError(_:) before propagating it, instead of rethrowing the mapped
error directly. Use the existing recordAuthError(_:) helper and the
completeExternalSignIn()/HostBrowserSignInFlow flow to keep the diagnostics path
consistent.
🪄 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
Run ID: 03cfbd38-dd63-4e11-9a1d-7e980211dda1
📒 Files selected for processing (7)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsAppInfo.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsReportBuilder.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDiagnosticsSecretScrubber.swiftPackages/iOS/CmuxMobileDiagnostics/Tests/CmuxMobileDiagnosticsTests/MobileDiagnosticsSecretScrubberTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDiagnosticsSheet.swiftios/cmux/Resources/Localizable.xcstrings
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift (1)
42-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
lastAuthErrordocs to match the redaction contract.The comment still says unknown failures are stored as the thrown Swift error type, but
diagnosticDescription(for:)now collapses those toAuthError.authFailure. Leaving the old wording here advertises a leakier contract than the code actually guarantees. Based on the PR objective and downstream diagnostics contract, this field should describe only display-safe auth categories.🤖 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/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift` around lines 42 - 46, The documentation for lastAuthError in AuthCoordinator should be updated to reflect the current redaction behavior. Adjust the comment near diagnosticDescription(for:) so it no longer says unknown failures preserve the thrown Swift error type; instead, state that only display-safe AuthError categories are retained and uncategorized failures collapse to AuthError.authFailure. Keep the wording aligned with the existing diagnostic contract and the display-safe purpose of lastAuthError.
🤖 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.
Outside diff comments:
In
`@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift`:
- Around line 42-46: The documentation for lastAuthError in AuthCoordinator
should be updated to reflect the current redaction behavior. Adjust the comment
near diagnosticDescription(for:) so it no longer says unknown failures preserve
the thrown Swift error type; instead, state that only display-safe AuthError
categories are retained and uncategorized failures collapse to
AuthError.authFailure. Keep the wording aligned with the existing diagnostic
contract and the display-safe purpose of lastAuthError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30754d45-7718-4bac-b279-8cbfda2d90c0
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/Fakes.swift
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
…-share-diagnostics-copy-share-logs
…hare-logs Resolve conflicts: - MobileShellComposite.swift: keep diagnostics feature (import, secret scrubber, diagnosticsEventLog) + adopt main's isolated deinit. - CmuxAgentChatUI, CmuxMobileTerminal: reset to origin/main (my compat-only shims are unneeded on main's toolchain). - Localizable.xcstrings: union — main's values for all shared keys + 27 new mobile.diagnostics.* keys (502 total). - swift-file-length-budget.tsv: regenerated via swift_file_length_budget.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
signOut() wiped drafts, paired Macs, registry devices, and visible connection context, but retained the two new Share Diagnostics inputs: lastConnectionError (deliberately kept across a visible-error clear) and the injected diagnosticsEventLog. On a shared device a user could sign out after a connection failure, another could sign in, and Share Diagnostics would then surface the previous account's scrubbed connection error and prior auth/connection events. Clear both at the account boundary. Also blank connectedHostName before the .disconnected transition so the conn.state diagnostics event that connectionState.didSet records cannot carry the previous account's host across sign-out; this makes the event-log actor clear() order-independent for privacy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 31 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Tie WorkspaceDiagnosticsSheet report generation to the view lifecycle: bail out of buildReport() before the off-main OSLog read and report assembly when the driving `.task` is cancelled (sheet dismissed), so the detached work is not started for a report nothing will consume. Regenerate .github/swift-file-length-budget.tsv so MobileShellComposite's budget matches its actual 7827 lines (the sign-out diagnostics-clear fix grew it past the stale 7813 entry, failing workflow-guard-tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Share Diagnostics exported the process-wide structured `diagnosticLog` (a flight recorder created once in AppCompositionRoot) into the report, but that log is never reset at the account boundary — signOut() only clears the account-scoped `diagnosticsEventLog` and `lastConnectionError`. On a shared device the next user could open Share Diagnostics and surface the previous account's structured activity history (connection/pairing/ input/composer timing). Stop feeding the process-wide structured log into this account-scoped report: buildReport() no longer reads store.diagnosticLog, and assembleReport passes structuredEventLog: nil. The report keeps its account-scoped In-App Event Log (diagnosticsEventLog, cleared on sign-out) as the event source; the flight recorder keeps recording for its non-account-scoped debugging purpose. The report builder and its tests are unchanged (the "Structured Event Log" section renders "none"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The privileged agent-feedback path (submitPrivilegedAgentFeedback) is intentionally not #if DEBUG-gated so it ships to @manaflow.ai users on Release/TestFlight. This PR un-gated the process-wide structured diagnosticLog to all builds, which routed it into that release-capable export. Because the log is a process-lifetime flight recorder that is never reset at the account boundary, on a shared device a later signed-in team user could submit a Release feedback bundle carrying the previous account's structured activity (connection/pairing/input/composer timing). Export the structured log into the feedback bundle on DEBUG builds only; on Release the bundle carries only the freeform note, terminal text, and the string debug log. This restores the pre-PR Release behavior (diagnosticLog was nil on Release, so the export was already empty Data() there) and, with the structured log already omitted from the account-scoped Share Diagnostics report, leaves no release-reachable diagnosticLog.export() sink. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-share-diagnostics-copy-share-logs
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
The prior account-boundary fix cleared diagnosticsEventLog with a
fire-and-forget Task { await log.clear() }, which autoreview flagged as
racy: the clear could run after the next signed-in user's Share
Diagnostics snapshot() (leaking the previous account's events), and a
still-in-flight recordDiagnosticsEvent task could land after the clear and
reinsert a prior-account event.
Make the reset ordered with the (MainActor) sign-out by swapping in a
fresh MobileDiagnosticsEventLog instance instead of clearing in place. The
swap is synchronous, so it precedes anything the next user can do, and
because recordDiagnosticsEvent binds the current instance locally, any
in-flight record writes to the previous, now-discarded instance rather
than the new one. diagnosticsEventLog becomes private(set) var; the only
external holder (AppCompositionRoot) reads it once at init to inject it, so
the internal swap affects no other reader.
Bumps the MobileShellComposite length budget for the added rationale
comment (regenerated via scripts/swift_file_length_budget.py).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
…-share-diagnostics-copy-share-logs # Conflicts: # .github/swift-file-length-budget.tsv
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 participants
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.
Fixes #5515
Summary
Validation
Local dev builds and xcodebuild were intentionally not run per issue instructions.
Summary by cubic
Adds in‑app Share Diagnostics on iOS so users can copy or share a redacted report from the Workspace menu. Tightens privacy and improves support/triage for #5515.
New Features
CmuxMobileDiagnostics: secret scrubber, report builder, store‑scoped in‑app event log, and a current‑process OSLog reader that omits message bodies and preserves an “OSLog unavailable” status.DiagnosticLogfor all builds to stamp feedback; it remains private in Release and is exportable only on DEBUG.Bug Fixes
AuthCoordinator.lastAuthErroracross send/verify code, OAuth, password sign‑in, token read/refresh, and offline checks; clears on successful sign‑in.lastConnectionError, scopes the diagnostics event log to the shell store and swaps in a fresh instance on sign‑out to avoid races and cross‑account leakage, and cancels report work if the sheet is dismissed; gates structured diagnostic‑log export to DEBUG in privileged feedback.Written for commit 89e665d. Summary will update on new commits.
Summary by CodeRabbit