Add cmux-remote iOS companion#4719
Open
moltar-aleatoric wants to merge 5 commits into
Open
Conversation
|
@24601 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> WalkthroughAdds a full iOS remote client and CmuxKit transport/client/event stack, Live Activities/widgets, background draining, notification/action resolution, AFK/snippet features, CLI/FeedCoordinator changes to require item_id and support DiffApprovalRequest, CI/project files, docs, and extensive tests. ChangesiOS Remote client + Feed diff-approval and reply path
Estimated code review effort: 🎯 5 (Critical) | ⏱️ ~180 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Author
|
Current external gates for this fork PR:
Local receipts are in the PR body; no local tests were run per repo policy. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cmux-remote-ios.yml:
- Around line 41-43: The Checkout step currently uses
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd and retains persisted
credentials; update that step (named "Checkout") to set persist-credentials:
false so the job does not keep auth tokens (add the key persist-credentials:
false under the Checkout step’s uses line).
- Around line 1-37: The workflow currently lacks a permissions block so
GITHUB_TOKEN inherits broad defaults; add an explicit permissions: block (either
top-level or under the build job) to restrict GITHUB_TOKEN to the
least-privilege scopes your tasks need (e.g., limit to contents: read and only
other specific scopes required) and update the build job to use those minimal
permissions; target the top-level permissions key or the build job entry (job
name "build") when making the change.
In `@iOS/cmux-remote/docs/architecture.md`:
- Around line 5-46: The fenced diagram block in architecture.md is missing a
language tag causing markdownlint MD040; update the opening triple-backtick that
precedes the ASCII diagram to include a language tag (e.g., ```text) so the
block becomes a fenced code block with a language, and leave the closing
triple-backticks unchanged; locate the diagram block by searching for the ASCII
art that starts with the box containing "SwiftUI views" and modify that opening
fence accordingly.
In `@iOS/cmux-remote/project.yml`:
- Around line 112-113: Remove the hardcoded NSFaceIDUsageDescription and
NSLocalNetworkUsageDescription entries from project.yml and move those
user-facing strings into the per-locale InfoPlist.strings files; add the two
keys ("NSFaceIDUsageDescription" and "NSLocalNetworkUsageDescription") with
English values to App/Resources/en.lproj/InfoPlist.strings and the corresponding
Japanese translations to App/Resources/ja.lproj/InfoPlist.strings so every
supported locale contains the Info.plist usage descriptions.
In `@iOS/cmux-remote/README.md`:
- Line 50: The table row for "Browser control" contains unescaped pipe
characters inside the cell (`browser open|goto|click|fill|press|find|...`) which
breaks table parsing; fix it by escaping each pipe as \| or by replacing the
inline code with an HTML code tag (e.g., <code>browser open|goto|...</code>) so
the pipes are not treated as column separators, and update the "Browser control"
row accordingly.
- Around line 145-147: The README claim about the .xcodeproj being uncommitted
conflicts with the presence of iOS/cmux-remote/cmux-remote.xcodeproj in this PR;
either remove the committed Xcode project files or update the README to reflect
the actual policy. Pick one: (A) if you want .xcodeproj uncommitted — remove
iOS/cmux-remote/cmux-remote.xcodeproj (and its folder contents) from the
repo/PR, add it to .gitignore, and ensure project.yml remains the source of
truth for regeneration; or (B) if you want to keep the committed .xcodeproj —
edit README.md to change the line referencing `cmux-remote.xcodeproj` and
`project.yml` to state that the .xcodeproj is committed (and note why), so the
README matches the repository state.
- Around line 107-132: The fenced code block in README.md (the project tree
block under iOS/cmux-remote/) is missing a language tag which triggers
markdownlint MD040; update the opening fence to include a language identifier
(e.g., "text") so the block reads ```text instead of ``` to satisfy the linter
and preserve formatting for the project tree display.
In `@iOS/cmux-remote/Sources/CmuxKit/Commands/AgentDecisionResolver.swift`:
- Around line 138-145: In the .freeform branch of AgentDecisionResolver (where
method, itemID and params are set), treat choiceLabel values that are empty or
only whitespace as if they were nil before building the "selections" array; i.e.
trim choiceLabel using .trimmingCharacters(in: .whitespacesAndNewlines) and if
the result is empty, use choiceID instead when constructing params["selections"]
(the same location that calls Self.requiredItemID(itemID, decisionID:
decisionID, kind: kind) and sets request_id/item_id).
In `@iOS/cmux-remote/Sources/CmuxKit/Intents/ResolveDecisionIntent.swift`:
- Around line 109-117: In the .failed case inside ResolveDecisionIntent.swift
(the branch building the dialog variable), stop interpolating the raw upstream
`message` into the user-facing `dialog`; instead use a generic localized string
(e.g., localized: "intent.resolve_decision.failed", defaultValue: "Couldn't
deliver.") with no `%@` formatting, and send the raw `message` to internal
logs/telemetry (use the project logger/telemetry helper or OSLog) so only
generic text is shown to users while full details are recorded internally.
In `@iOS/cmux-remote/Sources/CmuxKit/Models/AFKPolicy.swift`:
- Around line 143-166: Replace hardcoded English labels on the AFKPolicy.Rule
instances with localized strings using String(localized: "key.name",
defaultValue: "English text"); specifically update the label values for the
rules with labels "Read-only file inspection (strict)", "git status / log / diff
(read-only)", and "Block any rm -rf" (these are the label arguments passed into
AFKPolicy.Rule) to use localized keys (choose unique keys like
"afkpolicy.rule.readOnlyStrict", "afkpolicy.rule.gitReadOnly",
"afkpolicy.rule.blockRmRf") and keep the original English text as defaultValue.
In `@iOS/cmux-remote/Sources/CmuxKit/Models/AgentDecision.swift`:
- Around line 157-173: The switch on hookName can miss prefixed values (e.g.,
"agent.hook.PermissionRequest"); normalize hookName before the switch by
deriving it from payload["hook_event_name"] ?? event.name and then stripping any
known prefix or taking the last dot-separated component (e.g., if hookName
contains ".", set hookName = hookName.components(separatedBy: ".").last!).
Update the variable used in the switch (hookName) so cases like
"PermissionRequest" and "tool_use_request" match prefixed inputs; keep existing
symbols: payload, hookName, event.name, and the switch on hookName in
AgentDecision initialization.
In `@iOS/cmux-remote/Sources/CmuxKit/Models/CmuxError.swift`:
- Around line 23-66: The switch arms in errorDescription (cases transport,
command, unauthenticated, decoding, unsupportedCapability) currently interpolate
raw upstream messages/stderr; replace those interpolations with sanitized,
user-facing strings (e.g., generic messages like "Connection failed", "Command
failed", "Not authenticated", "Response could not be decoded", "Unsupported
server capability") and avoid including the raw message/trimmed stderr or
decoder payload in the returned description; if needed, record the original raw
details to internal logs or diagnostics (not returned by errorDescription) so
you keep the existing cases and localization keys but remove direct exposure of
upstream content.
In `@iOS/cmux-remote/Sources/CmuxKit/Models/WidgetState.swift`:
- Around line 36-40: The init(appGroup:) currently falls back silently to
FileManager.default.temporaryDirectory which breaks shared widget state; change
it to fail loudly by resolving the container URL via
FileManager.default.containerURL(forSecurityApplicationGroupIdentifier:) and if
that returns nil throw an error from the initializer (make init(appGroup:) a
throwing initializer) or make it failable and return nil; update any callers to
handle the thrown error or nil; reference the initializer init(appGroup:), the
appGroup parameter,
FileManager.default.containerURL(forSecurityApplicationGroupIdentifier:) and the
fileURL property when implementing this change so the code no longer writes to
temporaryDirectory silently.
In `@iOS/cmux-remote/Sources/CmuxKit/Reactor/AgentWatchdog.swift`:
- Around line 68-72: The loop in loop() uses try? await Task.sleep(for:
config.pollInterval), which hides CancellationError allowing scan() to run once
after stop(); change the sleep handling to explicitly catch CancellationError
and break/return from loop instead of swallowing it so scan() is not called
post-cancellation (referencing loop(), scan(), and config.pollInterval).
- Around line 77-80: The threshold calculation currently uses only
config.stuckThreshold.components.seconds which drops sub-second precision;
update the threshold computation in AgentWatchdog.swift (where threshold is set
and used with lastActivity and now.timeIntervalSince) to include the
components.nanoseconds (or other subsecond component) so you build a full
TimeInterval/Double like seconds + Double(nanoseconds)/1_000_000_000 (or use a
provided totalSeconds/TimeInterval helper if available) to preserve millisecond+
precision when comparing now.timeIntervalSince(value.timestamp).
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift`:
- Around line 296-300: The stderr trailing buffer is not flushed before
finishing the async stream, so final stderr fragments (stderrTail) are dropped;
in the same places where you flush stdoutTail (e.g., the code using stdoutTail
and continuation.yield) add the analogous logic to check if stderrTail.isEmpty
is false, convert it to String using String(data: stderrTail, encoding: .utf8)
and call continuation.yield(...) (or the appropriate stderr-yielding
continuation) before calling continuation.finish(); update both locations
referenced (the block near stdoutTail and the later finish at lines around
342–343) in CitadelSSHTransport so stderrTail is emitted when present.
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift`:
- Around line 246-256: The fallback constructs a CmuxWorkspace using
WindowID(resolvedWindowID) which can be empty; instead use the already-computed
lookupWindowID (or the existing windowID) so the returned CmuxWorkspace never
contains an empty WindowID—update the CmuxWorkspace initializer to pass
lookupWindowID (or windowID) for the windowID field and ensure types align with
the existing lookupWindowID logic and the listWorkspaces(windowID:) usage.
- Around line 385-390: The runRaw(_ args:) method currently logs the full
command string (via ShellEscape.command(parts)) which may include sensitive
payloads; change the log to avoid recording argument values — e.g. log only the
command verb/name and a safe representation such as the argument count or a
redacted shape (use parts.first or cmuxBinaryPath for the verb and args.count or
args.map { _ in "<redacted>" } for the shape) and remove the full command from
metadata; update the log.debug call in runRaw to emit only the safe fields and
keep the actual command passed to transport.runOneShot unchanged.
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CmuxEventDecoder.swift`:
- Around line 134-145: The parseTimestamp(_:) function creates two
ISO8601DateFormatter instances on every call; move these into static cached
formatters (e.g., static let fractionalFormatter and static let
wholeSecondFormatter) inside CmuxEventDecoder (or as fileprivate globals) and
reuse them in parseTimestamp to avoid per-frame allocations; ensure you set the
same formatOptions on the cached ISO8601DateFormatter instances and then call
fractionalFormatter.date(from: s) / wholeSecondFormatter.date(from: s) instead
of allocating new formatters each call.
In `@iOS/cmux-remote/Sources/CmuxKit/Util/TerminalInputCoding.swift`:
- Around line 30-39: The current multiline detection uses split(separator:)
counts (beforeLineCount and lineCount) which drop empty segments and misclassify
strings like "\n"; change to newline-presence checks instead: record
beforeHasNewline from the original input (check for "\r\n" or "\r" or "\n"),
normalize newlines as already done, then compute afterHasNewline by checking
s.contains("\n"), and set isMultiLine to afterHasNewline || beforeHasNewline;
update references to beforeLineCount/lineCount/isMultiLine in
TerminalInputCoding.swift accordingly.
In `@iOS/cmux-remote/Sources/CmuxRemote/Background/BGScheduler.swift`:
- Line 19: The code force-casts the incoming BGTask to BGAppRefreshTask causing
a crash if the subtype is unexpected; replace the force-cast with a guarded cast
(use `as? BGAppRefreshTask`) and if the cast fails call
`task.setTaskCompleted(success: false)` (or otherwise finish the task) and
return, otherwise call `handleAppRefresh` with the safely unwrapped
`BGAppRefreshTask`; ensure any existing expiration/cleanup logic still runs for
the valid task.
In `@iOS/cmux-remote/Sources/CmuxRemote/CmuxRemoteApp.swift`:
- Around line 103-105: The catch block that returns .failed(message:
error.localizedDescription) leaks internal error details to users; replace the
user-facing message with a generic localized string (e.g., a NSLocalizedString
key like "IntentFailedGenericMessage") and move the raw error details into a log
call so only the logger records error (keep the .failed return with the generic
text). Locate the catch in CmuxRemoteApp.swift where .failed(...) is returned
and update the .failed message and add a logging statement (use the existing app
logger or OSLog/Logger) to record error.localizedDescription/stack for
diagnostics.
In `@iOS/cmux-remote/Sources/CmuxRemote/iPad/GestureRecognizers.swift`:
- Around line 34-38: The overlay currently calls allowsHitTesting(false) on
MultiFingerGestureBridge which prevents the underlying PassThroughView from
receiving multi-touch events (its hitTest(_:with:) logic depends on
event?.allTouches count >= 2), so remove the .allowsHitTesting(false) (or change
it to allowsHitTesting(true)) on the MultiFingerGestureBridge overlay so the
PassThroughView and its UISwipeGestureRecognizer handlers can receive the
required multi-touch events; locate the call where MultiFingerGestureBridge is
overlaid and update/remove the allowsHitTesting(false) modifier.
- Around line 21-29: The drag handler currently uses a plain DragGesture
(triggering previousSurface()/nextSurface()) which permits single-finger swipes
and conflicts with the intended two-finger behavior and the
MultiFingerGestureBridge; fix by requiring a two-finger gesture: remove the
single-finger DragGesture usage for surface switching and either (A) switch to
the existing MultiFingerGestureBridge path for horizontal two-finger swipes
(call previousSurface()/nextSurface() from the bridge) or (B) replace the
SwiftUI DragGesture with a UIViewRepresentable/gesture recognizer that sets
numberOfTouchesRequired = 2 and forwards two-finger horizontal swipes to
previousSurface()/nextSurface(); also stop suppressing touch delivery to the
bridge by removing .allowsHitTesting(false) so PassThroughView.hitTest (which
already filters touches.count >= 2) can deliver multi-touch events to the UIKit
recognizer.
In `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift`:
- Around line 104-109: canvasViewDrawingDidChange currently spawns a new Task
for every stroke and lets detached VisionHandwriting.recognize tasks race, and
VNRecognizeTextRequest.recognitionLanguages is hardcoded; fix by keeping a
mutable Task handle (e.g., a property like recognitionTask) and canceling it
before creating the new Task in canvasViewDrawingDidChange so prior in-flight
recognition is cancelled, call Self.recognize from that managed Task, and update
VisionHandwriting.recognize (or where VNRecognizeTextRequest is created) to use
a locale-derived language identifier (e.g., Locale.current.identifier or
Locale.current.languageCode with a safe fallback) instead of the fixed ["en-US"]
so OCR adapts to the user locale and avoids stale onRecognized callbacks.
- Line 160: In PencilOverlayView.swift change the hardcoded English-only setting
on the VNRecognizeTextRequest (the request variable) so it uses the user's
preferred languages instead of ["en-US"]; populate request.recognitionLanguages
from Locale.preferredLanguages (or map/limit that array as needed to valid
BCP‑47 codes) or leave it nil to let Vision pick automatically; update any
text-recognition request creation code in PencilOverlayView to use this dynamic
recognitionLanguages source.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Hosts/HostAddView.swift`:
- Around line 181-183: In HostAddView's catch block where you currently assign
self.error = error.localizedDescription, replace the raw upstream message with a
sanitized, user-facing message (e.g., "Unable to add host. Please try again.")
and record the original error for diagnostics (e.g., send to logger/telemetry)
instead of exposing it in self.error; ensure the UI-binding (self.error) shows
only the generic, non-sensitive message and, if helpful, map known error types
to specific safe messages within the same catch in HostAddView so internal
details are never rendered to users.
In
`@iOS/cmux-remote/Sources/CmuxRemote/UI/NotificationsList/NotificationsListView.swift`:
- Around line 27-31: The Task blocks that call connection.client(for:) and then
use try? with client.openNotification(notification.id) and
client.jumpToNotification(notification.id) should not swallow errors and must
only call dismiss() on success; replace the try? usage with proper error
handling (do/catch or await Result) around client.openNotification(...) and
client.jumpToNotification(...), surface or log the error (e.g., show an alert or
return early) when the call fails, and only invoke dismiss() after the open/jump
completes successfully—look for the Task closures containing
connection.client(for: "open") and connection.client(for: "jump") and adjust
their control flow accordingly.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/RootView.swift`:
- Line 200: Replace the single formatted key used in accessibilityValue with a
plural-aware localization call that uses ICU-style .one/.other variants for the
"notifications.toolbar.count" key; specifically, change the L10n.format(...)
invocation inside the .accessibilityValue modifier to the repo’s plural API
(e.g., L10n.plural or the equivalent helper) so the localization looks up
"notifications.toolbar.count.one" vs "notifications.toolbar.count.other" based
on the Int count and passes the count as the numeric argument.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/BrowserSurfaceView.swift`:
- Line 92: Replace direct assignment of error.localizedDescription to lastError
in the browser actions (methods goBack, goForward, reload, captureScreenshot and
the catch at the shown line) with a generic, user-facing message (e.g., "Unable
to perform action" or action-specific friendly text) assigned to lastError and
send the raw error details to a logger or debug-only diagnostic channel instead;
locate the catch blocks referencing lastError and error.localizedDescription and
change them to set the safe message while preserving the original error for
internal logging/telemetry.
- Around line 90-91: The UI leaves stale lastError text after previously failing
commands because successful code paths never clear it; update the success paths
in BrowserSurfaceView (where you call client.browserGoto(url, surfaceID:),
client browser navigation calls, and after await refreshURL()) to set lastError
= nil on the main actor after the awaited call succeeds (or immediately before
calling refreshURL if that also updates UI), so any prior error message is
cleared when an action succeeds. Ensure you clear lastError in every successful
branch corresponding to browserGoto and the other navigation/refresh/stop
methods mentioned in the diff.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SnippetPickerView.swift`:
- Around line 62-66: In SnippetPickerView.send(_:) stop suppressing send errors
and re-order operations: first attempt client.sendText(...) with await inside a
do/catch so errors are handled, only after a successful send call
store.markUsed(id:) and then dismiss(); in the catch present or propagate an
error (e.g., show an alert) and do not mark the snippet used or dismiss. In
CmuxSnippetStore.shared replace the force-unwrap of
FileManager.default.urls(for: .applicationSupportDirectory, in:
.userDomainMask).first! with a safe unwrap and fallback (e.g., create/use
FileManager.default.temporaryDirectory or throw/return an error) so the app
cannot crash if the URL list is empty.
- Line 75: The code force-unwraps FileManager.default.urls(...).first in the
CmuxSnippetStore.shared initialization (and in SnippetPickerView.swift), which
can crash if the array is empty; replace the .first! with a safe guarded binding
(guard let appSupport = FileManager.default.urls(for:
.applicationSupportDirectory, in: .userDomainMask).first else { ... }) and
provide a safe fallback (e.g. FileManager.default.temporaryDirectory or
documents directory) or gracefully handle the error by returning/throwing from
the CmuxSnippetStore initializer; update any uses of the snippet storage URL
(the variable assigned from .first!) to use the guarded appSupport value so no
force-unwrap remains.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SurfaceDetailView.swift`:
- Around line 34-37: The pencil send currently swallows errors because it uses
try? on client.sendText, so modify the PencilOverlayView callback in
SurfaceDetailView (the Task that awaits connection.client(for: "send") and calls
client.sendText) to handle failures explicitly: replace try? with a do/catch
around try await client.sendText(recognized, surfaceID: surface.id, workspaceID:
workspace?.id), log the caught error (using your app logger/OSLog) and present
user-facing feedback (e.g., a toast/alert or update a UI error state) so
failures are visible to the user; ensure you still await connection.client(for:
"send") and only attempt send when client is non-nil.
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Workspaces/WorkspaceSidebarView.swift`:
- Around line 35-37: The refresh handler currently wraps
connection.handleEnterForeground() inside a detached Task, which prevents the
.refreshable lifecycle/cancellation from awaiting it; replace the Task wrapper
in the .refreshable closure so you directly call await
connection.handleEnterForeground() (i.e., make the closure use await
connection.handleEnterForeground() rather than Task { await ... }) in
WorkspaceSidebarView.swift to ensure the refresh UI waits and cancellation is
propagated.
- Around line 11-16: The Section order currently uses
Array(connection.snapshot.windows.values) which preserves insertion order and
can lead to unstable reordering; instead sort the windows collection before
iterating (e.g., when building the ForEach feeding Section) by a stable key such
as window.isKey (descending), then window.title ?? "" (ascending), then
window.id.raw (ascending). Update the ForEach that iterates
connection.snapshot.windows.values (and the Section creation using window.title)
to map and sort the windows accordingly so sections are rendered in the
deterministic order.
In `@iOS/cmux-remote/Sources/CmuxRemoteWidgets/WorkspaceStatusWidget.swift`:
- Around line 20-22: Replace hardcoded English strings used in the widget
configuration with localized variants: change the .configurationDisplayName(...)
and .description(...) calls in WorkspaceStatusWidget (and the similar strings at
the other occurrence around the 124-126 region) to use String(localized:
"widget.workspace.displayName", defaultValue: "cmux Workspace") and
String(localized: "widget.workspace.description", defaultValue: "Latest unread
workspace and pending count.") (or similar keys), ensuring you add corresponding
localization entries to the string catalog for those keys.
- Around line 66-70: Replace the single-key pluralization calls to
WidgetL10n.format for pending counts with explicit plural keys by switching to
the `.one` and `.other` variants and selecting the correct key based on the
numeric count (e.g., use the singular key when entry.unread == 1, otherwise the
plural key); update both the WidgetL10n.format call around the pending-count at
the WidgetL10n.format("widget.pending_count", ...) site and the similar call at
lines 88–92 to pass the appropriate localized key and value for
Int64(entry.unread) so the UI uses correct singular/plural strings.
In `@Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swift`:
- Around line 274-276: Replace the hardcoded "DiffApprovalRequest" literal with
the enum raw value to avoid divergence: where you check event.hookEventName ==
.diffApprovalRequest and currently return "DiffApprovalRequest", return
event.hookEventName.rawValue (falling back to event.toolName ?? "unknown" as
before) so the returned toolName always reflects the enum's raw value; update
the expression around event.hookEventName, .diffApprovalRequest, and
event.toolName accordingly.
In `@Sources/AppDelegate.swift`:
- Around line 14309-14318: The switch case handling
"feed.exit_plan.bypassPermissions" in handleFeedNotificationResponse is
unreachable because configureUserNotifications registers CMUXFeedExitPlan
actions only for "feed.exit_plan.ultraplan", "feed.exit_plan.manual", and
"feed.exit_plan.autoAccept", and
FeedNotificationActionSecurity.privilegedActionIdentifiers omits
bypassPermissions; either remove the unused case from
handleFeedNotificationResponse or add a corresponding UNNotificationAction for
"feed.exit_plan.bypassPermissions" in configureUserNotifications and include
that identifier in FeedNotificationActionSecurity.privilegedActionIdentifiers
(and ensure authentication settings match the intended behavior).
In `@Sources/TerminalController.swift`:
- Around line 10842-10872: The current logic only forbids providing both
"selections" and "question_selections" together but still allows combinations
with "selection_ids" and silently falls back when a provided
"question_selections" fails to resolve; change the guard to enforce mutual
exclusivity across the three keys ("selections", "question_selections",
"selection_ids") by checking how many of these keys are present in params and
returning an invalid_params error if more than one is provided, then only
attempt resolution for the single declared source: if "selections" is present
use it directly, else if "question_selections" is present call
Self.v2FeedQuestionSelections(...) and
FeedCoordinator.shared.questionSelectionLabels(requestId:itemId:questionSelections:)
and if resolution fails return an error (do not fall back), else if
"selection_ids" is present call
FeedCoordinator.shared.questionSelectionLabels(requestId:itemId:selectionIds:)
and if that resolution fails return an error.
🪄 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: 9f084a08-04c4-4d60-a5f7-b251bc2c2f2d
📒 Files selected for processing (108)
.github/workflows/cmux-remote-ios.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/cmux.swiftPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamEvent.swiftPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swiftResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate+EqualizeSplitsShortcut.swiftSources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate.swiftSources/CmuxLifecycleEventPublishing.swiftSources/Feed/FeedCoordinator.swiftSources/KeyboardShortcutContext.swiftSources/TerminalController.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedCoordinatorTests.swiftiOS/cmux-remote/App/Configuration/CmuxKit-Info.plistiOS/cmux-remote/App/Configuration/CmuxRemote.Release.entitlementsiOS/cmux-remote/App/Configuration/CmuxRemote.entitlementsiOS/cmux-remote/App/Configuration/CmuxRemoteWidgets-Info.plistiOS/cmux-remote/App/Configuration/CmuxRemoteWidgets.entitlementsiOS/cmux-remote/App/Configuration/Info.plistiOS/cmux-remote/App/Resources/Localizable.xcstringsiOS/cmux-remote/App/Resources/PrivacyInfo.xcprivacyiOS/cmux-remote/App/Resources/en.lproj/InfoPlist.stringsiOS/cmux-remote/App/Resources/ja.lproj/InfoPlist.stringsiOS/cmux-remote/README.mdiOS/cmux-remote/Sources/CmuxKit/Commands/AgentDecisionResolver.swiftiOS/cmux-remote/Sources/CmuxKit/Commands/BrowserCommands.swiftiOS/cmux-remote/Sources/CmuxKit/Intents/CmuxIntentResolverRegistry.swiftiOS/cmux-remote/Sources/CmuxKit/Intents/ResolveDecisionIntent.swiftiOS/cmux-remote/Sources/CmuxKit/Models/AFKPolicy.swiftiOS/cmux-remote/Sources/CmuxKit/Models/AgentDecision.swiftiOS/cmux-remote/Sources/CmuxKit/Models/AgentDecisionActivityAttributes.swiftiOS/cmux-remote/Sources/CmuxKit/Models/CMUXActivityAttributes.swiftiOS/cmux-remote/Sources/CmuxKit/Models/CmuxError.swiftiOS/cmux-remote/Sources/CmuxKit/Models/CmuxEvent.swiftiOS/cmux-remote/Sources/CmuxKit/Models/Identifiers.swiftiOS/cmux-remote/Sources/CmuxKit/Models/Snippet.swiftiOS/cmux-remote/Sources/CmuxKit/Models/WidgetState.swiftiOS/cmux-remote/Sources/CmuxKit/Models/Workspace.swiftiOS/cmux-remote/Sources/CmuxKit/Package.support.mdiOS/cmux-remote/Sources/CmuxKit/Reactor/AgentWatchdog.swiftiOS/cmux-remote/Sources/CmuxKit/Reactor/EventReactor.swiftiOS/cmux-remote/Sources/CmuxKit/Reactor/ResumeJournal.swiftiOS/cmux-remote/Sources/CmuxKit/Reactor/ServerState.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CmuxEventDecoder.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CmuxSSHTransport.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/HostConfig.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/ShellEscape.swiftiOS/cmux-remote/Sources/CmuxKit/Util/Logging.swiftiOS/cmux-remote/Sources/CmuxKit/Util/TerminalInputCoding.swiftiOS/cmux-remote/Sources/CmuxRemote/AFK/AFKPolicyStore.swiftiOS/cmux-remote/Sources/CmuxRemote/Auth/CmuxCredentialStore.swiftiOS/cmux-remote/Sources/CmuxRemote/Auth/HostStore.swiftiOS/cmux-remote/Sources/CmuxRemote/Background/BGScheduler.swiftiOS/cmux-remote/Sources/CmuxRemote/Background/BackgroundEventDrain.swiftiOS/cmux-remote/Sources/CmuxRemote/CmuxRemoteApp.swiftiOS/cmux-remote/Sources/CmuxRemote/CmuxRemoteAppDelegate.swiftiOS/cmux-remote/Sources/CmuxRemote/Connection/ConnectionManager.swiftiOS/cmux-remote/Sources/CmuxRemote/Intents/CmuxAppShortcuts.swiftiOS/cmux-remote/Sources/CmuxRemote/LiveActivity/CMUXLiveActivityController.swiftiOS/cmux-remote/Sources/CmuxRemote/Localization/L10n.swiftiOS/cmux-remote/Sources/CmuxRemote/Notifications/AgentDecisionNotifier.swiftiOS/cmux-remote/Sources/CmuxRemote/Notifications/NotificationCategories.swiftiOS/cmux-remote/Sources/CmuxRemote/Notifications/NotificationCenterBridge.swiftiOS/cmux-remote/Sources/CmuxRemote/Terminal/TerminalAccessoryBar.swiftiOS/cmux-remote/Sources/CmuxRemote/Terminal/TerminalInputAssist.swiftiOS/cmux-remote/Sources/CmuxRemote/Terminal/TerminalSurfaceView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/CommandPalette/CommandPaletteView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Hosts/HostAddView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Hosts/HostListView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/NotificationsList/NotificationsListView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Panes/PaneTreeView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/RootView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Settings/AFKSettingsView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/BrowserSurfaceView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SnippetPickerView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SurfaceDetailView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Workspaces/WorkspaceSidebarView.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/GestureRecognizers.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/KeyboardShortcutCatalog.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/AgentDecisionActivityWidget.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/CmuxLiveActivityWidget.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/CmuxWidgetBundle.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/WidgetL10n.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/WidgetState.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/WorkspaceStatusWidget.swiftiOS/cmux-remote/Tests/CmuxKitTests/AgentDecisionTests.swiftiOS/cmux-remote/Tests/CmuxKitTests/CMUXClientCommandTests.swiftiOS/cmux-remote/Tests/CmuxKitTests/CmuxEventDecoderTests.swiftiOS/cmux-remote/Tests/CmuxKitTests/ShellEscapeTests.swiftiOS/cmux-remote/Tests/CmuxRemoteTests/AFKPolicyEvaluatorTests.swiftiOS/cmux-remote/Tests/CmuxRemoteTests/NotificationPayloadTests.swiftiOS/cmux-remote/cmux-remote.xcodeproj/project.pbxprojiOS/cmux-remote/cmux-remote.xcodeproj/project.xcworkspace/contents.xcworkspacedataiOS/cmux-remote/cmux-remote.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvediOS/cmux-remote/cmux-remote.xcodeproj/xcshareddata/xcschemes/CmuxKit.xcschemeiOS/cmux-remote/cmux-remote.xcodeproj/xcshareddata/xcschemes/CmuxRemote.xcschemeiOS/cmux-remote/docs/architecture.mdiOS/cmux-remote/docs/known-limitations.mdiOS/cmux-remote/docs/review-fixes.mdiOS/cmux-remote/docs/verification.mdiOS/cmux-remote/project.ymliOS/cmux-remote/scripts/generate.sh
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift (1)
99-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove internal vendor name from user-facing error.
The error message exposes "Citadel" which is an internal library/vendor name. Per coding guidelines, user-facing errors must not expose upstream vendor names.
🔧 Suggested fix
case .rsaPrivateKey: throw CmuxError.transport( - "RSA keys are accepted by Citadel but cmux-remote ships ed25519/P-256 only", + "RSA keys are not supported — use ed25519 or P-256", underlying: nil )As per coding guidelines: user-facing errors must not expose upstream vendor names or internal provider names.
🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift` around lines 99 - 103, The user-facing error in CitadelSSHTransport.swift for the .rsaPrivateKey case currently exposes the internal vendor name; update the CmuxError.transport call in the .rsaPrivateKey branch to remove "Citadel" and replace it with a generic, user-facing message (e.g., mention accepted key types or that RSA is unsupported) while keeping the same error type and signature (CmuxError.transport(..., underlying: nil)) so callers and tests continue to work.iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift (1)
449-451: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider guarding against empty
windowIDindecodeWorkspacefor consistency.The static decoder can still produce
WindowID("")when all fallbacks (entry["window_id"],entry["window_ref"],fallbackWindowID?.raw) are nil or empty. WhilenewWorkspacenow guards against this,listWorkspacescallers may receive workspace objects with invalid empty IDs if the server response is malformed.🛡️ Defensive fix to return nil on empty windowID
static func decodeWorkspace(_ entry: [String: Any], fallbackWindowID: WindowID? = nil) -> CmuxWorkspace? { guard let id = (entry["id"] as? String) ?? (entry["ref"] as? String) else { return nil } - let windowID = WindowID((entry["window_id"] as? String) ?? (entry["window_ref"] as? String) ?? fallbackWindowID?.raw ?? "") + let windowIDRaw = (entry["window_id"] as? String) ?? (entry["window_ref"] as? String) ?? fallbackWindowID?.raw + guard let windowIDRaw, !windowIDRaw.isEmpty else { return nil } + let windowID = WindowID(windowIDRaw)🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift` around lines 449 - 451, In decodeWorkspace, guard against constructing and returning a CmuxWorkspace with an empty WindowID: after computing id and windowID (WindowID(...)), check that windowID.raw is non-empty and if empty return nil so malformed server responses don't produce invalid workspaces; update the static method decodeWorkspace to return nil when windowID.raw is empty (referencing decodeWorkspace, WindowID, and CmuxWorkspace).
🤖 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 `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift`:
- Around line 87-90: The computed property footerTextColor can remain red when
sendError was set even after recognized becomes empty; update footerTextColor
logic to prefer the neutral hint state by first checking recognized.isEmpty and
returning .secondary, then only if not empty check sendError to return .red,
otherwise return .primary (i.e. reorder the condition checks in the
footerTextColor property so recognized.isEmpty takes precedence over sendError).
In `@iOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SurfaceDetailView.swift`:
- Around line 35-49: pencilError state is being mutated off the main actor
inside the onRecognized async closure from SurfaceDetailView (invoked by
PencilOverlayView's Send Task); ensure those state mutations run on the main
actor by either marking the onRecognized closure `@MainActor` or wrapping the two
pencilError assignments in await MainActor.run { pencilError = ... } before
throwing (the assignments around the guard failure and inside the catch in
SurfaceDetailView where connection.client(for:), client.sendText(...), and throw
error are used).
---
Outside diff comments:
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift`:
- Around line 99-103: The user-facing error in CitadelSSHTransport.swift for the
.rsaPrivateKey case currently exposes the internal vendor name; update the
CmuxError.transport call in the .rsaPrivateKey branch to remove "Citadel" and
replace it with a generic, user-facing message (e.g., mention accepted key types
or that RSA is unsupported) while keeping the same error type and signature
(CmuxError.transport(..., underlying: nil)) so callers and tests continue to
work.
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift`:
- Around line 449-451: In decodeWorkspace, guard against constructing and
returning a CmuxWorkspace with an empty WindowID: after computing id and
windowID (WindowID(...)), check that windowID.raw is non-empty and if empty
return nil so malformed server responses don't produce invalid workspaces;
update the static method decodeWorkspace to return nil when windowID.raw is
empty (referencing decodeWorkspace, WindowID, and CmuxWorkspace).
🪄 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: cf17fbad-3afb-498e-9f20-507ff08348bc
📒 Files selected for processing (32)
.github/workflows/cmux-remote-ios.ymlPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swiftSources/AppDelegate.swiftSources/TerminalController.swiftiOS/cmux-remote/App/Resources/Localizable.xcstringsiOS/cmux-remote/README.mdiOS/cmux-remote/Sources/CmuxKit/Commands/AgentDecisionResolver.swiftiOS/cmux-remote/Sources/CmuxKit/Intents/ResolveDecisionIntent.swiftiOS/cmux-remote/Sources/CmuxKit/Models/AFKPolicy.swiftiOS/cmux-remote/Sources/CmuxKit/Models/AgentDecision.swiftiOS/cmux-remote/Sources/CmuxKit/Models/CmuxError.swiftiOS/cmux-remote/Sources/CmuxKit/Models/WidgetState.swiftiOS/cmux-remote/Sources/CmuxKit/Reactor/AgentWatchdog.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CmuxEventDecoder.swiftiOS/cmux-remote/Sources/CmuxKit/Util/TerminalInputCoding.swiftiOS/cmux-remote/Sources/CmuxRemote/Background/BGScheduler.swiftiOS/cmux-remote/Sources/CmuxRemote/CmuxRemoteApp.swiftiOS/cmux-remote/Sources/CmuxRemote/Connection/ConnectionManager.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Hosts/HostAddView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/NotificationsList/NotificationsListView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/RootView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/BrowserSurfaceView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SnippetPickerView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SurfaceDetailView.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Workspaces/WorkspaceSidebarView.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/GestureRecognizers.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swiftiOS/cmux-remote/Sources/CmuxRemoteWidgets/WorkspaceStatusWidget.swiftiOS/cmux-remote/Tests/CmuxKitTests/CMUXClientCommandTests.swiftiOS/cmux-remote/docs/architecture.md
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 (3)
iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift (1)
246-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the post-create refresh mask a successful workspace creation.
workspace.createhas already returned a workspace id here. IflistWorkspaces(windowID:)throws,newWorkspaceexits before the fallback below and callers can treat the create as failed and retry into duplicate workspaces.🔧 Suggested fix
- if let workspace = try await listWorkspaces(windowID: lookupWindowID).first(where: { $0.id == WorkspaceID(id) }) { - return workspace - } + do { + if let workspace = try await listWorkspaces(windowID: lookupWindowID) + .first(where: { $0.id == WorkspaceID(id) }) { + return workspace + } + } catch { + log.warning("workspace lookup after create failed; using fallback", metadata: [ + "workspace_id": "\(id)" + ]) + } guard let resolvedWindowID else { return nil }🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift` around lines 246 - 256, The post-create lookup using listWorkspaces(windowID:) can throw and mask a successful create; in the newWorkspace flow wrap the listWorkspaces(windowID: lookupWindowID).first(where: { $0.id == WorkspaceID(id) }) call in a do-catch (or use try? and handle nil) so any thrown error is ignored and execution falls through to the existing fallback that constructs CmuxWorkspace from the create response (using responseWindowID/resolvedWindowID); ensure you still return the found workspace when lookup succeeds but do not propagate lookup errors from listWorkspaces.iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift (1)
61-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the host-key probe auth delegate for each connection attempt.
HostKeyProbeAuthenticationremembers whether it already offered.none, but the probe initializer stores one delegate for the lifetime of the transport. After the first connect attempt, laterensureConnected()calls reuse a delegate withofferedNone == true, so TOFU probe retries fail before auth even starts.🔧 Suggested fix
- private let auth: SSHAuthenticationMethod + private let authFactory: () throws -> SSHAuthenticationMethod @@ - self.auth = try Self.makeAuthMethod(username: username, credential: credential) + self.authFactory = { try Self.makeAuthMethod(username: username, credential: credential) } @@ - self.auth = SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(username: username)) + self.authFactory = { SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(username: username)) } @@ - authenticationMethod: auth, + authenticationMethod: try authFactory(),Also applies to: 448-478
🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift` around lines 61 - 76, The transport currently creates a single HostKeyProbeAuthentication instance in the initializer (self.auth = SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(...))) which is reused across connection attempts and retains offeredNone state; instead, instantiate a fresh HostKeyProbeAuthentication for every connect attempt (i.e. move creation of SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(...)) out of the init and into the connection path such as ensureConnected()/connect()), replacing the stored persistent auth or assigning the new auth before each authentication attempt so each connection uses a new HostKeyProbeAuthentication delegate.iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift (1)
64-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable Send while the previous send is still in flight.
The button stays tappable until
onRecognizedreturns, so a quick double-tap can dispatch the same command multiple times to the remote surface.🔧 Suggested fix
`@State` private var isSending = falseButton { let payload = recognized.trimmingCharacters(in: .whitespacesAndNewlines) - guard !payload.isEmpty else { return } + guard !payload.isEmpty, !isSending else { return } + isSending = true Task { + defer { isSending = false } do { try await onRecognized(payload) isPresented = false } catch { sendError = L10n.string( "pencil.send_failed", defaultValue: "Could not send handwriting." ) } } } label: { Label(L10n.string("common.send", defaultValue: "Send"), systemImage: "paperplane.fill") } .buttonStyle(.borderedProminent) - .disabled(recognized.isEmpty) + .disabled(isSending || recognized.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty)🤖 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 `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift` around lines 64 - 80, Button allows duplicate sends because it remains enabled while onRecognized is awaiting; add a `@State` private var isSending = false and update the Task in the Button action to set isSending = true before calling try await onRecognized(payload) and ensure isSending = false in a defer/finally block (also set isPresented/sendError as currently done on success/failure), then include isSending in the .disabled(...) condition (e.g., .disabled(recognized.isEmpty || isSending)) and optionally guard early in the action to return if isSending is already true to avoid race conditions.
♻️ Duplicate comments (1)
iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift (1)
304-308:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve buffered
stderrwhen the stream ends withCommandFailed.Both failure catch blocks throw
CmuxError.command(stderr: ""), so a non-zero exit drops the stderr lines/fragments already accumulated in this task and makes stream failures opaque to callers.Also applies to: 349-353
🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift` around lines 304 - 308, The catch blocks that convert SSHClient.CommandFailed into CmuxError.command currently drop previously captured stderr by passing an empty string; update both catch sites in CitadelSSHTransport (the CommandFailed handlers around the async stream continuation) to pass the accumulated stderr buffer (the variable holding collected stderr fragments from the task/stream, e.g., stderrBuffer or accumulatedStderr) into CmuxError.command so any buffered lines/fragments are preserved in the thrown error; ensure you coalesce the buffer into a single String (including any trailing fragment) before constructing the CmuxError.command.
🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift`:
- Around line 61-76: The transport currently creates a single
HostKeyProbeAuthentication instance in the initializer (self.auth =
SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(...))) which is reused
across connection attempts and retains offeredNone state; instead, instantiate a
fresh HostKeyProbeAuthentication for every connect attempt (i.e. move creation
of SSHAuthenticationMethod.custom(HostKeyProbeAuthentication(...)) out of the
init and into the connection path such as ensureConnected()/connect()),
replacing the stored persistent auth or assigning the new auth before each
authentication attempt so each connection uses a new HostKeyProbeAuthentication
delegate.
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swift`:
- Around line 246-256: The post-create lookup using listWorkspaces(windowID:)
can throw and mask a successful create; in the newWorkspace flow wrap the
listWorkspaces(windowID: lookupWindowID).first(where: { $0.id == WorkspaceID(id)
}) call in a do-catch (or use try? and handle nil) so any thrown error is
ignored and execution falls through to the existing fallback that constructs
CmuxWorkspace from the create response (using
responseWindowID/resolvedWindowID); ensure you still return the found workspace
when lookup succeeds but do not propagate lookup errors from listWorkspaces.
In `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift`:
- Around line 64-80: Button allows duplicate sends because it remains enabled
while onRecognized is awaiting; add a `@State` private var isSending = false and
update the Task in the Button action to set isSending = true before calling try
await onRecognized(payload) and ensure isSending = false in a defer/finally
block (also set isPresented/sendError as currently done on success/failure),
then include isSending in the .disabled(...) condition (e.g.,
.disabled(recognized.isEmpty || isSending)) and optionally guard early in the
action to return if isSending is already true to avoid race conditions.
---
Duplicate comments:
In `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift`:
- Around line 304-308: The catch blocks that convert SSHClient.CommandFailed
into CmuxError.command currently drop previously captured stderr by passing an
empty string; update both catch sites in CitadelSSHTransport (the CommandFailed
handlers around the async stream continuation) to pass the accumulated stderr
buffer (the variable holding collected stderr fragments from the task/stream,
e.g., stderrBuffer or accumulatedStderr) into CmuxError.command so any buffered
lines/fragments are preserved in the thrown error; ensure you coalesce the
buffer into a single String (including any trailing fragment) before
constructing the CmuxError.command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f0a43d6-a25d-4aca-8d10-e4c0d44a65e1
📒 Files selected for processing (4)
iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swiftiOS/cmux-remote/Sources/CmuxRemote/UI/Surfaces/SurfaceDetailView.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift
Author
|
Addressed the latest CodeRabbit comment-only findings in
Validation on head
No unresolved GitHub review threads are currently returned by the reviewThreads API. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swift`:
- Around line 264-265: The code currently accumulates unbounded stderr into
stderrBuffer (used to populate CmuxError.command) in the streaming methods
(e.g., runLineStream and its stderr handling), which can grow indefinitely for
long-lived sessions; change stderrBuffer to retain only a bounded tail (for
example, cap to the last N bytes or last M lines) whenever appending stderr data
so you keep recent context without unbounded growth, update the logic in the
stderr append sites (the variable stderrBuffer and any calls that read it for
CmuxError.command) to trim to that bound after each append, and apply the same
bounded-tail change to the other similar blocks referenced (around the other
stderr handling ranges: the blocks covering 287-309, 327-340, 353-357) so all
streaming/error paths use the same bounded-buffer strategy.
In `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift`:
- Line 22: The send Task started in PencilOverlayView when tapping the Send
button is not cancelled/blocked by isSending, so Cancel/Clear or dismiss can
still trigger sending; update PencilOverlayView to either (A) disable the other
header actions (Cancel, Clear, dismiss handlers) while isSending == true, or (B)
store the Task returned by Task { … } (the send Task started in the Send button
action), keep it as a `@State` or `@StateObject` task handle, and call cancel() from
the Clear, Cancel, and onDismiss handlers to stop the in-flight send; reference
the isSending state, the Send button action that launches the Task, and the
Clear/Cancel/dismiss handlers to implement the chosen fix and ensure isSending
is reset only after successful completion or cancellation.
🪄 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: 64815252-edac-4d74-aaa3-4b8373a77f92
📒 Files selected for processing (3)
iOS/cmux-remote/Sources/CmuxKit/Transport/CMUXClient.swiftiOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift
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)
iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift (1)
175-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard may pass for empty drawing with potentially problematic bounds.
When
drawing.boundsis.null(empty drawing),insetBy(dx: -24, dy: -24)produces a rect with infinite origin but positive dimensions (48×48). The guard passes, anddrawing.image(from:scale:)is called with infinite coordinates. This also causes unnecessary recognition when the user taps Clear.🛡️ Proposed fix: check for empty drawing first
`@MainActor` static func recognize(drawing: PKDrawing, on canvas: PKCanvasView) async -> String { + guard !drawing.strokes.isEmpty else { return "" } let bounds = drawing.bounds.insetBy(dx: -24, dy: -24) - guard bounds.width > 0, bounds.height > 0 else { return "" } let scale = canvas.window?.windowScene?.screen.scale ?? max(canvas.traitCollection.displayScale, 1) let image = drawing.image(from: bounds, scale: scale) return await VisionHandwriting.recognize(image: image) ?? "" }🤖 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 `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift` around lines 175 - 179, The code uses drawing.bounds.insetBy(...) which can produce infinite origins when drawing.bounds is .null; add a pre-check to return early when the drawing bounds are empty/null to avoid creating a rect with infinite coordinates and unnecessary recognition. Specifically, in the method in PencilOverlayView where you call drawing.bounds.insetBy and drawing.image(from:scale:), add a guard like guard !drawing.bounds.isNull && !drawing.bounds.isEmpty else { return "" } before computing bounds and scale, then proceed to compute scale and call drawing.image(from: scale:) and VisionHandwriting.recognize(image:).
🤖 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 `@iOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift`:
- Around line 175-179: The code uses drawing.bounds.insetBy(...) which can
produce infinite origins when drawing.bounds is .null; add a pre-check to return
early when the drawing bounds are empty/null to avoid creating a rect with
infinite coordinates and unnecessary recognition. Specifically, in the method in
PencilOverlayView where you call drawing.bounds.insetBy and
drawing.image(from:scale:), add a guard like guard !drawing.bounds.isNull &&
!drawing.bounds.isEmpty else { return "" } before computing bounds and scale,
then proceed to compute scale and call drawing.image(from: scale:) and
VisionHandwriting.recognize(image:).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b230dc62-08f5-4f5b-b8dd-03cfaa4ac55f
📒 Files selected for processing (2)
iOS/cmux-remote/Sources/CmuxKit/Transport/CitadelSSHTransport.swiftiOS/cmux-remote/Sources/CmuxRemote/iPad/PencilOverlayView.swift
Author
|
Current status after follow-up hardening on
Remaining external blockers:
Local worktree is clean except untracked |
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
2 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.
Summary
Adds
cmux-remote, a standalone iOS/iPadOS companion app for remote cmux use, plus the desktop/socket/feed support needed for item-bound remote agent decisions.Key pieces:
iOS/cmux-remoteXcode project with app, widget/Live Activity, AppIntents, tests, docs, privacy manifest, entitlements, and localized resourcesDiffApprovalRequest, capability advertisement, item_id-required replies, and generic notification copy.github/workflows/cmux-remote-ios.ymlCI gate for the standalone iOS projectVerification
Local build/static gates run on this branch:
git diff --cached --checkbefore commitpython3 -m json.tool Resources/Localizable.xcstrings >/dev/nullpython3 -m json.tool iOS/cmux-remote/App/Resources/Localizable.xcstrings >/dev/nullplutil -lint iOS/cmux-remote/App/Configuration/Info.plist iOS/cmux-remote/App/Configuration/CmuxRemoteWidgets-Info.plist iOS/cmux-remote/App/Resources/PrivacyInfo.xcprivacyBGProcessing,NSSupportsLiveActivitiesFrequentUpdates, and unused Live Activity APNs token plumbing underiOS/cmux-remotexcodebuild -quiet -project iOS/cmux-remote/cmux-remote.xcodeproj -scheme CmuxRemote -configuration Debug -destination 'generic/platform=iOS' -derivedDataPath /tmp/cmux-remote-hardening-ios-25 -skipPackagePluginValidation build CODE_SIGNING_ALLOWED=NOxcodebuild -quiet -project iOS/cmux-remote/cmux-remote.xcodeproj -scheme CmuxRemote -configuration Debug -destination 'generic/platform=iOS' -derivedDataPath /tmp/cmux-remote-hardening-ios-tests-21 -skipPackagePluginValidation build-for-testing CODE_SIGNING_ALLOWED=NOCMUX_SKIP_ZIG_BUILD=1 xcodebuild -quiet -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-cmux-remote-hardening-unit-skip-zig-7 build-for-testing CODE_SIGNING_ALLOWED=NOCMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag cmux-remote-hardeningNo local tests were executed, per repo policy.
Review Notes
Four independent adversarial reviewer passes were run during hardening. Valid findings were fixed, including:
Final targeted Swift/iOS confirmation returned no current findings and verdict GO.
Release Gates
Still required outside this PR diff:
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a standalone iOS/iPadOS companion app,
cmux-remote, and hardens the desktop feed/socket/CLI for secure, item‑bound remote decisions and Lock Screen actions. Also bounds iOS stream state during sends to prevent stale or unsafe commands.New Features
DiffApprovalRequestacross hooks, workstream mapping, CLI persistence, and localized diff notifications.item_idon v2 feed replies; CLI now sends it; capabilities advertisefeed.reply.item_id_requiredandfeed.question_selections.item_idenforcement and action auth.iOS/cmux-remoteapp with widgets/Live Activities, App Intents, background refresh/drain, and stream-send paths now bound to current state for reliability.Migration
feed.permission.reply,feed.exit_plan.reply, andfeed.question.replyto includeitem_idor they will returninvalid_params.Written for commit 60cb4bc. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests
Documentation
Chores