Cli Mobile Launcher#255
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds provider-backed tracked CLI session start/resume end-to-end: shared cliLaunch/shell contracts and types, desktop sync/PTY handler and persistence, ADE CLI planner and start_cli_session RPC, iOS SyncService API/UI and terminal emulator, plus tests, docs, and config updates. ChangesCLI Session Support Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
@copilot review but do not make fixes |
Here's the review. Code Review: CLI Mobile LauncherSummaryThis PR adds a Positives
Issues / ConcernsModerate
Minor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts (1)
1687-1699: ⚡ Quick winStrengthen the shell injection assertion to verify
startupCommandis entirely absent.
expect.not.objectContaining({ startupCommand: "rm -rf nope" })only guards against that specific string — the assertion passes even ifstartupCommandis set to any other value. For a security intent (no caller-controlled startup command passes through), asserting the key is completely absent is more precise:♻️ Proposed fix
- expect(ptyService.create).toHaveBeenCalledWith( - expect.not.objectContaining({ - startupCommand: "rm -rf nope", - }), - ); - expect(ptyService.create).toHaveBeenCalledWith( - expect.objectContaining({ - laneId: "lane-1", - title: "Shell", - toolType: "shell", - }), - ); + const shellCall = ptyService.create.mock.calls.at(-1)?.[0]; + expect(shellCall).not.toHaveProperty("startupCommand"); + expect(shellCall).toMatchObject({ + laneId: "lane-1", + title: "Shell", + toolType: "shell", + });🤖 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 `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts` around lines 1687 - 1699, The test currently only blocks the specific string "rm -rf nope" from being passed as startupCommand; replace the weak check in the ptyService.create assertion with a check that the startupCommand property is entirely absent by using expect.not.objectContaining({ startupCommand: expect.anything() }) so any presence of the key will fail; keep the other assertions (expect.objectContaining with laneId/title/toolType and expect(ptyService.writeBySessionId).not.toHaveBeenCalled()) unchanged.apps/ios/ADE/Views/Work/WorkNewChatScreen.swift (1)
286-354: ⚡ Quick winUse
deferto guaranteebusyreset on all submit paths.
busy = falseis currently repeated across branches/catch. This is easy to miss when adding new returns.Proposed refactor
busy = true errorMessage = nil + defer { busy = false } let wire = workRuntimeWireFields(provider: provider, mode: runtimeMode) let normalizedReasoning = reasoningEffort.trimmingCharacters(in: .whitespacesAndNewlines) do { if sessionMode == .cli { let result = try await syncService.startCliSession( laneId: selectedLaneId, @@ ) - busy = false return true } @@ let summary = try await syncService.createChatSession( @@ ) await onStarted(summary, opener) - busy = false return true } catch { ADEHaptics.error() errorMessage = error.localizedDescription - busy = false return false }🤖 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 `@apps/ios/ADE/Views/Work/WorkNewChatScreen.swift` around lines 286 - 354, The submit flow sets busy = true but resets it in multiple places; wrap the operation with a defer to guarantee busy is cleared on every exit: after setting busy = true at the start of the handler put a defer { busy = false } and then remove the duplicate busy = false lines inside the sessionMode == .cli branch and after await onStarted, leaving only the defer to reset busy; reference the surrounding symbols like sessionMode, syncService.startCliSession, syncService.createChatSession, onCliStarted and onStarted when locating the code to change.
🤖 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 `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 4341-4369: If resumeSessionId is provided, validate that
runtime.sessionService.get(resumeSessionId) returns a session and that its
provider/tool type matches the requested provider before building startupCommand
or calling ptyService.create; if the session is missing or the provider
mismatches, reject the request (throw or return an error) instead of falling
back to a generic buildTrackedCliResumeCommand. Update the code around
resumeSessionId/resumeTargetId (the resumeSession variable, the startupCommand
construction using buildTrackedCliResumeCommand, and the subsequent
ptyService.create call/launchFields) to perform this guard early and fail fast
when validation fails.
In `@apps/ade-cli/src/cli.ts`:
- Around line 2262-2267: The provider fallback currently uses
firstPositional(args) which can pick up values that belong to flags (e.g.,
"--permission-mode edit"); change the fallback to pick the first true positional
token only (skip any token that starts with '-' and skip tokens that are
flag-arguments). Implement or call a helper that scans args and returns the
first element that does not start with '-' and is not immediately following a
known flag (or more simply, filter args to exclude tokens that start with '-'
and exclude tokens that are flagged as flag-values) and use that instead of
firstPositional(args) in the requireValue(providerArg ?? readValue(...) ?? ... )
expression; reference providerArg, readValue, firstPositional, requireValue and
args when making the replacement.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 372-373: The transcript storage cap constant MAX_TRANSCRIPT_BYTES
(64 * 1024 * 1024) should remain as the server-side write cap, but do not reuse
it as the default tail/read size for readTerminal calls; introduce a separate
smaller default read cap (e.g., DEFAULT_READ_TAIL_BYTES) and use that in
readTerminal instead of MAX_TRANSCRIPT_BYTES to avoid huge WS payloads and
memory spikes. Update any function signature or call sites that currently pass
MAX_TRANSCRIPT_BYTES as the default tail (look for readTerminal usage) to use
the new DEFAULT_READ_TAIL_BYTES, keep TRANSCRIPT_LIMIT_NOTICE unchanged, and
ensure enforcement of MAX_TRANSCRIPT_BYTES still applies when appending/writing
transcripts.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 1804-1806: The code calls
args.ptyService.writeBySessionId(result.sessionId, ...) after ptyService.create
has already succeeded, so a synchronous throw from writeBySessionId can orphan
the created PTY; wrap the writeBySessionId call in a try/catch and on error
perform deterministic cleanup of the newly created session (use
args.ptyService.killSession(result.sessionId) or the appropriate cleanup API
such as closeSession/terminateSession) then rethrow or return a handled error;
reference parsed.initialInput, provider, result.sessionId and
args.ptyService.writeBySessionId when making the change.
In `@apps/desktop/src/shared/cliLaunch.ts`:
- Around line 73-138: buildTrackedCliLaunchCommand must validate provider vs
AgentChatPermissionMode combinations up front instead of silently remapping; add
a validation block at the top of buildTrackedCliLaunchCommand that checks
args.permissionMode against an explicit allowed-set per provider (for example:
Claude allowedModes = {...}, Codex allowedModes = {...}, Cursor allowedModes =
{...}, Droid allowedModes = {...}, Opencode allowedModes = {...}) and throw a
clear Error (or return a rejected result) when an unsupported combination (e.g.,
permissionMode === "config-toml" with a provider that doesn't support it) is
requested; reference the function buildTrackedCliLaunchCommand and the
permissionMode-to-flag helpers (permissionModeToClaudeFlag,
permissionModeToCodexFlags, permissionModeToCursorFlags,
permissionModeToDroidExecFlags, buildOpenCodeCommandParts) so callers are
prevented from receiving silently remapped modes.
In `@apps/desktop/src/shared/shell.ts`:
- Around line 67-127: parseCommandLine (and the Windows counterpart
parseWindowsCommandLine) currently drops empty quoted arguments because tokens
are only emitted when current.length > 0; change the logic to preserve empty
quoted tokens by tracking whether the current token was quoted (e.g., a boolean
like wasQuoted set when entering a quote) and: 1) when closing a quote, if
current is empty push "" (or mark wasQuoted so the token will be emitted later),
2) when encountering whitespace push the token even if current is empty if
wasQuoted is true, and reset wasQuoted after pushing; also apply the same
pattern in parseWindowsCommandLine so "" and '' are preserved as distinct empty
arguments instead of being dropped.
In `@apps/ios/ADE/Views/Lanes/LaneTypes.swift`:
- Around line 139-140: The confirmation copy for destructive actions currently
uses soft "Restore" wording which is misleading; update the enum string returns
in LaneTypes.swift for .restoreStaged and .restoreAllStaged to explicit
destructive language such as "Discard staged file?" and "Discard all staged
files?" (and likewise change the other cases that currently return "Restore" for
bulk restore actions to explicit "Discard…" phrasing); ensure you update any
corresponding localization keys/usages and tests that rely on these message
strings so the UI and automated checks reflect the new destructive wording.
In `@apps/ios/ADE/Views/Work/WorkNewChatScreen.swift`:
- Around line 128-133: When sessionMode changes back to .chat the handler in
WorkNewChatScreen currently only normalizes unsupported providers, but it
doesn't validate modelId compatibility which can leave an invalid provider/model
pair (e.g., provider set to "codex" with a Claude model) and later break chat
creation; update the .onChange(of: sessionMode) block so that when newMode ==
.chat you also revalidate the current modelId against the chosen provider (or
clear/reset modelId)—adjust the logic near the provider/model assignment (the
provider and modelId variables modified in the onChange handler) and ensure the
same validation/clearing is applied before createNewChat is invoked (the code
around the create/new chat flow referenced at lines 334-338) so only compatible
provider/model pairs are submitted.
In `@apps/ios/ADE/Views/Work/WorkRootScreen`+Actions.swift:
- Around line 346-350: The code stores the resumed session as `resumed` (from
`result.session`) into `optimisticSessions[resumed.id]` but then calls
`openSession(session)`, so navigation can open the stale original session;
change the call to `openSession(resumed)` (or otherwise use `result.session`)
after assigning `optimisticSessions[resumed.id] = resumed` so the UI opens the
resumed session id returned by `startCliSession`; ensure `resumed` is non-nil
before calling `openSession`.
---
Nitpick comments:
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts`:
- Around line 1687-1699: The test currently only blocks the specific string "rm
-rf nope" from being passed as startupCommand; replace the weak check in the
ptyService.create assertion with a check that the startupCommand property is
entirely absent by using expect.not.objectContaining({ startupCommand:
expect.anything() }) so any presence of the key will fail; keep the other
assertions (expect.objectContaining with laneId/title/toolType and
expect(ptyService.writeBySessionId).not.toHaveBeenCalled()) unchanged.
In `@apps/ios/ADE/Views/Work/WorkNewChatScreen.swift`:
- Around line 286-354: The submit flow sets busy = true but resets it in
multiple places; wrap the operation with a defer to guarantee busy is cleared on
every exit: after setting busy = true at the start of the handler put a defer {
busy = false } and then remove the duplicate busy = false lines inside the
sessionMode == .cli branch and after await onStarted, leaving only the defer to
reset busy; reference the surrounding symbols like sessionMode,
syncService.startCliSession, syncService.createChatSession, onCliStarted and
onStarted when locating the code to change.
🪄 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: CHILL
Plan: Pro
Run ID: 2ca60289-1b69-4526-835c-e651c6b5b560
⛔ Files ignored due to path filters (8)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (30)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/tsconfig.jsonapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/lib/shell.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/shell.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneTypes.swiftapps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
👮 Files not reviewed due to content moderation or server errors (8)
- apps/ios/ADE/Services/SyncService.swift
- apps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swift
- apps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swift
- apps/ade-cli/tsconfig.json
- apps/ios/ADE/Views/Work/WorkRootComponents.swift
- apps/desktop/src/main/services/sync/syncHostService.ts
- apps/desktop/src/main/services/sync/syncHostService.test.ts
- apps/ios/ADE/Views/Work/WorkRootScreen.swift
| if (parsed.initialInput && provider !== "shell") { | ||
| args.ptyService.writeBySessionId(result.sessionId, `${parsed.initialInput}\r`); | ||
| } |
There was a problem hiding this comment.
Unhandled writeBySessionId failure can orphan the created session.
ptyService.create has already succeeded by the time writeBySessionId is called. If writeBySessionId throws synchronously, the handler rejects and the caller sees an error — but the PTY session already exists on the desktop. A simple guard prevents the leak:
🛡️ Proposed fix
- if (parsed.initialInput && provider !== "shell") {
- args.ptyService.writeBySessionId(result.sessionId, `${parsed.initialInput}\r`);
- }
+ if (parsed.initialInput && provider !== "shell") {
+ try {
+ args.ptyService.writeBySessionId(result.sessionId, `${parsed.initialInput}\r`);
+ } catch {
+ // best-effort; session is still returned to the caller
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (parsed.initialInput && provider !== "shell") { | |
| args.ptyService.writeBySessionId(result.sessionId, `${parsed.initialInput}\r`); | |
| } | |
| if (parsed.initialInput && provider !== "shell") { | |
| try { | |
| args.ptyService.writeBySessionId(result.sessionId, `${parsed.initialInput}\r`); | |
| } catch { | |
| // best-effort; session is still returned to the caller | |
| } | |
| } |
🤖 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 `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines
1804 - 1806, The code calls args.ptyService.writeBySessionId(result.sessionId,
...) after ptyService.create has already succeeded, so a synchronous throw from
writeBySessionId can orphan the created PTY; wrap the writeBySessionId call in a
try/catch and on error perform deterministic cleanup of the newly created
session (use args.ptyService.killSession(result.sessionId) or the appropriate
cleanup API such as closeSession/terminateSession) then rethrow or return a
handled error; reference parsed.initialInput, provider, result.sessionId and
args.ptyService.writeBySessionId when making the change.
Addresses review comments: 3206094825, 3206094865, 3206094975, 4403060699, 3206116137, 3206116148, 3206116150, 3206116152, 3206116157, 3206116159, 3206116162, 3206116164, 3206116167.
|
@copilot review but do not make fixes |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a185b2. Configure here.
| return value ?? null; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
New CLI flags missing from VALUE_CARRIER_FLAGS set
Medium Severity
The new value-carrying flags --resume-session, --resume-session-id, --resume-target, --resume-target-id, and --initial-input are consumed via readValue in buildCliSessionStartPlan but are not added to VALUE_CARRIER_FLAGS. Since firstStandalonePositional uses that set to skip flag values, it will misidentify the value of these flags as the provider positional argument. For example, ade shell start-cli --resume-session session-1 codex --lane lane-1 would treat session-1 as the provider instead of codex, causing a "provider must be one of…" error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1a185b2. Configure here.
| resumeCommand: nil, | ||
| resumeMetadata: nil, | ||
| chatIdleSinceAt: nil | ||
| )) |
There was a problem hiding this comment.
Fallback optimistic session sets toolType to nil
Low Severity
When result.session is nil, the fallback TerminalSessionSummary sets toolType: nil instead of deriving it from the selected provider. The server maps providers to tool types (e.g., "cursor" → "cursor-cli", "codex" → "codex"), so the optimistic session will have the wrong tool type until the real session syncs. This causes cliProviderForTerminalSession to return "shell" for any provider, and session icon/classification helpers to misidentify the session.
Reviewed by Cursor Bugbot for commit 1a185b2. Configure here.
| return { startupCommand }; | ||
| } | ||
| return buildTrackedCliLaunchCommand({ provider, permissionMode, sessionId: preassignedSessionId }); | ||
| })(); |
There was a problem hiding this comment.
Caller-controlled resumeTargetId not sanitized before shell command
Medium Severity
In the start_cli_session handler, resumeTargetId from caller-controlled RPC input is passed directly into buildTrackedCliResumeCommand without applying stripInjectionChars first. This value is interpolated into a shell startup command. While commandArrayToLine provides quoting, the project rule requires stripInjectionChars() on each value individually before assembling the string, as a defense-in-depth measure against injection via control characters.
Triggered by learned rule: Apply stripInjectionChars to all caller-controlled values before prompt/shell interpolation
Reviewed by Cursor Bugbot for commit 1a185b2. Configure here.
Here's a review of the current state (both commits). Code Review: CLI Mobile Launcher (updated)What commit 2 addressed from the first review
Remaining / new issuesModerate
Minor
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/ade-cli/src/adeRpcServer.test.ts (1)
2019-2024: 💤 Low valueExact-match
startupCommandassertion is brittle.Hard-coding the full resume command (
"codex --no-alt-screen --sandbox workspace-write --ask-for-approval untrusted resume thread-77") couples this test to the exact flag ordering and defaults of the shared resume builder. A future, behavior-preserving tweak (flag reordering, alias change, additional default) will break this test for no functional reason. Consider matching on the meaningful substrings (provider,resume,thread-77) viaexpect.stringContaining/expect.stringMatchinginstead.♻️ Suggested loosening of the assertion
- expect(fixture.runtime.ptyService.create).toHaveBeenCalledWith( - expect.objectContaining({ - sessionId: "session-existing", - startupCommand: "codex --no-alt-screen --sandbox workspace-write --ask-for-approval untrusted resume thread-77", - }), - ); + expect(fixture.runtime.ptyService.create).toHaveBeenCalledWith( + expect.objectContaining({ + sessionId: "session-existing", + startupCommand: expect.stringMatching(/^codex\b.*\bresume\s+thread-77\b/), + }), + );🤖 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 `@apps/ade-cli/src/adeRpcServer.test.ts` around lines 2019 - 2024, The test currently asserts an exact startupCommand string in the call to fixture.runtime.ptyService.create which is brittle; update the assertion to allow flexible flag ordering by matching only meaningful substrings of the command (e.g., the provider/executable name like "codex", the action "resume", and the thread id "thread-77") using expect.stringContaining or expect.stringMatching inside the expect.objectContaining for startupCommand while keeping sessionId: "session-existing" unchanged.
🤖 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 `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 280-283: Update the mock for ptyService.writeBySessionId to mirror
the real method signature: replace vi.fn(() => true) with a mock that accepts
(sessionId: string, data: string) and returns true so calls and typings match
the real ptyService.writeBySessionId(sessionId: string, data: string): boolean;
keep enrichSessions as-is (vi.fn((sessions: unknown[]) => sessions)) and ensure
the mock uses vi.fn with the explicit parameter names for clarity.
In `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 2143-2146: The parser parseCliSessionPermissionMode currently
coerces unknown/invalid inputs to "default"; instead, validate using
asTrimmedString and isTrackedCliPermissionMode and throw an invalidParams error
for invalid values to fail fast. Modify parseCliSessionPermissionMode to call
asTrimmedString(value), check isTrackedCliPermissionMode(mode) and if true
return mode (typed as AgentChatPermissionMode), otherwise throw an invalidParams
(or similarly structured parameter validation) error describing the invalid
permissionMode input so callers cannot silently proceed with "default".
In `@apps/ade-cli/src/cli.ts`:
- Around line 1265-1270: takeArgsAfterTerminator currently returns an empty
array when "--" is present with no trailing tokens, which callers like
buildShellPlan and buildCliSessionStartPlan treat as explicit input; change
takeArgsAfterTerminator to return null when there are no tokens after the
terminator (i.e., if rest.length === 0) so callers can detect "no explicit
input" correctly, and leave args.splice(index) behavior intact; also verify
buildShellPlan and buildCliSessionStartPlan use a null check (not truthiness) to
decide whether to override --command / --message.
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 2019-2024: The test currently asserts an exact startupCommand
string in the call to fixture.runtime.ptyService.create which is brittle; update
the assertion to allow flexible flag ordering by matching only meaningful
substrings of the command (e.g., the provider/executable name like "codex", the
action "resume", and the thread id "thread-77") using expect.stringContaining or
expect.stringMatching inside the expect.objectContaining for startupCommand
while keeping sessionId: "session-existing" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b78168d-0304-4bc7-a024-0adf5609d91a
📒 Files selected for processing (16)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/lib/shell.test.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/shell.tsapps/ios/ADE/Views/Lanes/LaneTypes.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/ios/ADE/Views/Lanes/LaneTypes.swift
- apps/ios/ADE/Views/Work/WorkRootScreen+Actions.swift
- apps/desktop/src/main/services/pty/ptyService.ts
- apps/ade-cli/src/cli.test.ts
- apps/desktop/src/main/services/sync/syncRemoteCommandService.ts
- apps/ios/ADE/Views/Work/WorkNewChatScreen.swift
- apps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swift
- apps/desktop/src/shared/cliLaunch.ts
Addresses review comments: 3206267285, 3206267286, 3206267288, 3206284545, 3206290738, 3206290740, 3206290745, 4403344136.
|
@copilot review but do not make fixes |
Addresses review comment: 3206348939.
|
@copilot review but do not make fixes |
…er-8f96925f # Conflicts: # apps/ade-cli/src/cli.ts
|
@copilot review but do not make fixes |
Addresses review comments: 3206463962, 3206464059.
|
@copilot review but do not make fixes |
| saveProfile(nil) | ||
| saveRemoteCommandDescriptors([]) |
There was a problem hiding this comment.
resetTerminalSubscriptionState(clearHistory: false) calls subscribedTerminalSessionIds.removeAll() on disconnect, then restoreTerminalSubscriptions() reads from the same now-empty set on reconnect and returns early (guard !sessionIds.isEmpty else { return }). The result is that after every disconnect/reconnect cycle, all open WorkTerminalSessionView instances stop receiving terminal_data because the set remains empty — the host never receives a fresh terminal_subscribe for already-open views, and .task doesn't re-fire on reconnect. To fix, save the session IDs before clearing, or don't clear them on disconnect (just remove them one-by-one as restoreTerminalSubscriptions iterates).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Services/SyncService.swift
Line: 2343-2344
Comment:
**Terminal subscriptions permanently silenced after reconnect**
`resetTerminalSubscriptionState(clearHistory: false)` calls `subscribedTerminalSessionIds.removeAll()` on disconnect, then `restoreTerminalSubscriptions()` reads from the same now-empty set on reconnect and returns early (`guard !sessionIds.isEmpty else { return }`). The result is that after every disconnect/reconnect cycle, all open `WorkTerminalSessionView` instances stop receiving `terminal_data` because the set remains empty — the host never receives a fresh `terminal_subscribe` for already-open views, and `.task` doesn't re-fire on reconnect. To fix, save the session IDs before clearing, or don't clear them on disconnect (just remove them one-by-one as `restoreTerminalSubscriptions` iterates).
How can I resolve this? If you propose a fix, please make it concise.
| .task { | ||
| try? await syncService.subscribeTerminal(sessionId: session.id) | ||
| } | ||
| .onDisappear { | ||
| Task { | ||
| try? await syncService.unsubscribeTerminal(sessionId: session.id) |
There was a problem hiding this comment.
The bare Task { try? await syncService.unsubscribeTerminal(...) } in onDisappear is not tied to the view lifecycle and cannot be cancelled. If the user navigates away and immediately back (common on iPhone with swipe-back gestures), the .task from the re-appeared view calls subscribeTerminal (which inserts the ID into subscribedTerminalSessionIds), but then the detached onDisappear task executes, calling unsubscribeTerminal which removes the ID and sends an unsubscribe message. All subsequent terminal_data and terminal_exit messages are then filtered out, leaving the visible terminal silently frozen. The fix is to track a cancellation signal (e.g., a Bool state or a Task handle stored in a class-based holder) and skip the unsubscribe if the view has reappeared.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swift
Line: 173-178
Comment:
**Non-cancellable unsubscribe task races with re-subscription on quick navigation**
The bare `Task { try? await syncService.unsubscribeTerminal(...) }` in `onDisappear` is not tied to the view lifecycle and cannot be cancelled. If the user navigates away and immediately back (common on iPhone with swipe-back gestures), the `.task` from the re-appeared view calls `subscribeTerminal` (which inserts the ID into `subscribedTerminalSessionIds`), but then the detached `onDisappear` task executes, calling `unsubscribeTerminal` which removes the ID and sends an unsubscribe message. All subsequent `terminal_data` and `terminal_exit` messages are then filtered out, leaving the visible terminal silently frozen. The fix is to track a cancellation signal (e.g., a `Bool` state or a `Task` handle stored in a class-based holder) and skip the unsubscribe if the view has reappeared.
How can I resolve this? If you propose a fix, please make it concise.
|
@copilot review but do not make fixes |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/ade-cli/src/cli.test.ts (1)
1301-1322: ⚡ Quick winConsider adding a companion test for
--permission-modedefault/required behavior onshell start --provider.The existing
shell start-clitests always supply--permission-mode, butshell start --providerhere omits it. It's worth a small test to pin whether the field defaults tonull, a specific value, or is required — especially given the PR notes call out permission-mode correctness as medium risk.🤖 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 `@apps/ade-cli/src/cli.test.ts` around lines 1301 - 1322, Add a companion unit test to apps/ade-cli/src/cli.test.ts that builds a CLI plan via buildCliPlan(["shell","start","--provider","claude","--lane","lane-1","--resume-session","session-1"]) and asserts the generated execute plan (plan.kind === "execute") contains the first step params.name "start_cli_session" and that params.arguments includes a permissionMode value that matches the expected default (e.g., permissionMode: null or undefined depending on current implementation); use the same pattern as the existing test (expect(plan.steps[0]?.params).toMatchObject({...})) to explicitly verify permission-mode default/required behavior for shell start with --provider.
🤖 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 `@apps/ade-cli/README.md`:
- Line 71: The example command uses the wrong CLI entrypoint: replace the
tracked-session example "ade shell start --provider claude --lane lane-id
--permission-mode default" with the tracked CLI session command "ade shell
start-cli --provider claude --lane lane-id --permission-mode default" so the
example uses start-cli (not start) and avoids the invalid flag combination shown
in the README; update the line where the example command appears accordingly.
In `@apps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swift`:
- Around line 202-228: The full-screen clear branch (when first == 2 || first ==
3) only wipes lines/cursor but must also reset text attributes to match the live
emulator; update the branch in the function containing ensureCursor() so it
either invokes WorkTerminalScreen.reset() or explicitly sets the same defaults
for foreground and bold that WorkTerminalScreen.reset() uses (in addition to
clearing lines/row/column) so previews and the live terminal share identical
reset semantics.
In `@apps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swift`:
- Around line 46-55: updateViewport currently calls screen.reset() and
screen.write(rawText) on every viewport change which re-parses the entire
transcript on the main thread; change it so heavy work only runs when the
underlying transcript changes and viewport updates only re-render the visible
region: 1) if revision == lastRevision and rawText == lastRawText, skip
screen.reset()/screen.write() and only compute the visible attributed string for
the requested cols/rows (don’t reparse whole rawText); 2) when revision changed,
perform screen.write(rawText) and any ANSI parsing on a background queue, update
lastRawText/lastRevision there, then dispatch view.render(...) to the main queue
with the already-parsed visible output; 3) remove unconditional
screen.reset()/screen.write() from updateViewport and instead add a fast path
that reuses the existing screen buffer and extracts only the viewport slice for
ADETerminalTextView.render, calling onViewportChange(viewport) after UI update.
- Around line 20-22: The closure assigned to
ADETerminalTextView.onViewportChange strongly captures the view and causes a
retain cycle; update both assignments in makeUIView and updateUIView to capture
the view weakly (e.g. [weak view]) and early-return if the view is nil, then
call context.coordinator.updateViewport(viewport, rawText: rawText, revision:
revision, view: view) with the unwrapped view; this breaks the view →
onViewportChange → closure → view cycle while preserving calls to
Coordinator.updateViewport.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 1301-1322: Add a companion unit test to
apps/ade-cli/src/cli.test.ts that builds a CLI plan via
buildCliPlan(["shell","start","--provider","claude","--lane","lane-1","--resume-session","session-1"])
and asserts the generated execute plan (plan.kind === "execute") contains the
first step params.name "start_cli_session" and that params.arguments includes a
permissionMode value that matches the expected default (e.g., permissionMode:
null or undefined depending on current implementation); use the same pattern as
the existing test (expect(plan.steps[0]?.params).toMatchObject({...})) to
explicitly verify permission-mode default/required behavior for shell start with
--provider.
🪄 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: CHILL
Plan: Pro
Run ID: b6ff2f05-e795-41fc-af0f-d2603d482bca
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (15)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swiftapps/ios/ADETests/ADETests.swift
✅ Files skipped from review due to trivial changes (2)
- apps/ios/ADE/Models/RemoteModels.swift
- apps/desktop/src/main/services/pty/ptyService.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/ios/ADE/Views/Work/WorkModelCatalog.swift
- apps/ios/ADETests/ADETests.swift
- apps/ade-cli/src/cli.ts
- apps/ade-cli/src/adeRpcServer.ts
- apps/ios/ADE/Services/SyncService.swift
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/ios/ADE/Views/Work/WorkNewChatScreen.swift
|
@copilot review but do not make fixes |
|
@copilot review but do not make fixes |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Note
Medium Risk
Adds new terminal-launch and resume pathways across the CLI RPC server, desktop sync command layer, and iOS UI; mistakes could start the wrong provider/session or mis-handle terminal I/O and resuming. Also increases transcript/stream sizes, which may impact memory/perf on large outputs.
Overview
Enables starting and resuming tracked Work CLI terminal sessions (Claude/Codex/Cursor/Droid/OpenCode/Shell) from both
ade-cliand the iOS companion app, using shared provider/permission launch helpers and stricter allowlisting.Adds a new CLI RPC tool
start_cli_sessionand a new mobile sync remote commandwork.startCliSession, including resume support (byresumeSessionId/metadata), Claude pre-assigned--session-id, bounded initial input writing with cleanup on failure, and persisted replay results for reconnect dedupe.Refactors the desktop renderer-only launch + shell quoting utilities into
apps/desktop/src/shared/(cliLaunch.ts,shell.ts), tightens permission-mode validation (e.g.,config-tomlonly for Codex), improves terminal read defaults/limits (64MB transcript cap, smaller default read tail), and updates iOS Work to support creating/resuming CLI sessions plus a new terminal emulator view with subscribe/unsubscribe tracking.Reviewed by Cursor Bugbot for commit 1a185b2. Configure here.
Greptile Summary
This PR wires up tracked CLI session launches and resumes across three layers: a new
start_cli_sessionJSON-RPC tool inade-cli, a newwork.startCliSessionsync remote command in the desktop service, and a full CLI session creation/resume UI flow in the iOS companion app. It also extracts the launch-profile helpers (cliLaunch.ts,shell.ts) into a shared package, expands the terminal transcript cap to 64 MB, and adds a newWorkTerminalEmulatorViewwith cell-based ANSI rendering.start_cli_session(RPC) andwork.startCliSession(sync) share the samebuildTrackedCliLaunchCommand/buildTrackedCliResumeCommandhelpers, covering Claude pre-assigned session IDs, boundedinitialInputwrites with cleanup on failure, and provider/permission-mode validation.WorkNewChatScreengains a CLI mode picker;WorkRootScreen+Actionsauto-resumes stopped sessions that have a resume target;WorkTerminalEmulatorViewis a newUIViewRepresentablecell-based terminal emulator with 256-color, true-color, and scroll-region (\\e[L]/\\e[M]/\\e[P]/\\e[r]) support.WorkTerminalSubscriptionLifecyclefixes the subscribe/unsubscribe race on quick navigation.Confidence Score: 4/5
Safe to merge for the primary use cases (Claude Code, Codex, Cursor); the scroll-region implicit-newline gap in the new terminal emulator can cause visual mis-positioning for programs that mix DECSTBM regions with natural newline scrolling.
The launch/resume plumbing, permission-mode validation, dispose-on-failure cleanup, and subscribe/unsubscribe lifecycle are all handled correctly. The one functional gap is
newline()not scrolling within an active DECSTBM region — for the specific CLIs launched here (alt-screen Claude Code, explicit-\e[M] Codex) this rarely surfaces, but any program that writes to the bottom of a scroll region via natural newlines will see the cursor drift below the region boundary.WorkTerminalEmulatorView.swiftandWorkNavigationAndTranscriptHelpers.swift— thenewline()methods in bothWorkTerminalScreenandWorkTerminalTextReplayneed scroll-region awareness.Important Files Changed
start_cli_sessionRPC tool with provider/permission validation, pre-assigned Claude session IDs, bounded initialInput writes, and proper dispose-in-try/catch cleanup on write failure.work.startCliSessionsync command; contains one unreachable null guard insideresolveLaunch()that can never execute becauserequireResumeSessionForProvideralready throws when the session is missing.--no-alt-screeninjection.newline()does not handle implicit scrolling when cursor sits atscrollBottom.WorkTerminalTextReplay; samenewline()scroll-region gap present as inWorkTerminalScreen.restoreTerminalSubscriptionson reconnect, andstartCliSessionremote call; data/exit events filtered to subscribed sessions only.WorkTerminalEmulatorView; fixes subscribe/unsubscribe race withWorkTerminalSubscriptionLifecyclegeneration counter.!isChatSession,status != "running", andterminalSessionHasResumeTarget; uses optimistic session map and reload on completion.workCliPermissionModehelper;workCliPermissionModecorrectly maps all provider/mode combinations including returningnilfor shell.read_terminal_sessionmax-bytes reduced to 220 KB to avoid large response payloads when nomaxBytesis specified.Sequence Diagram
sequenceDiagram participant iOS as iOS App participant Sync as SyncService participant Host as SyncRemoteCommandService participant PTY as PtyService participant CLI as CLI Process iOS->>Sync: startCliSession(laneId, provider, permissionMode, resumeSessionId?) Sync->>Host: work.startCliSession (sync command) Host->>Host: parseCliProvider / parseCliPermissionMode Host->>Host: validateLaunchProfilePermissionMode alt resumeSessionId provided Host->>Host: requireResumeSessionForProvider Host->>Host: resolveTrackedCliResumeCommand → startupCommand else new session Host->>Host: buildTrackedCliLaunchCommand (pre-assigns Claude --session-id) end Host->>PTY: ptyService.create(sessionId, toolType, startupCommand, cols, rows) PTY-->>CLI: spawn process opt "initialInput provided and provider != shell" Host->>PTY: writeBySessionId(initialInput + CR) alt write failed Host->>PTY: dispose(ptyId, sessionId) Host-->>Sync: Error: could not write initialInput end end Host-->>Sync: "{ sessionId, ptyId, session }" Sync-->>iOS: StartCliSessionResult iOS->>Sync: subscribeTerminal(sessionId) Sync->>Host: terminal_subscribe Host-->>Sync: TerminalSnapshot (transcript) Sync-->>iOS: terminal_data events (live stream)Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "ship: handle terminal line editing" | Re-trigger Greptile