Make the handoff say why it's blocked, and imports say what they are - #910
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on August 7. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
📝 WalkthroughWalkthroughChangesThe PR adds cross-machine destination lane fast-forwarding, structured handoff blockers and controls, richer external-session transcript discovery and rendering, iOS import support, and a shared permission picker. Cross-machine handoff
External session transcripts
Shared permission controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts (1)
105-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisplaced doc block. The markup-density comment (Lines 105-110) explains the import-path test at Line 130, but now sits on the classifier-consistency test. Move it so each block documents the test below it.
♻️ Suggested move
- /** - * The markup-density gate exists to keep junk out of row *previews*. It must - * not run on `cleanExternalSessionUserText`, which also builds the imported - * chat transcript — rejecting there silently deletes real messages from - * someone's history. - */ /** * Regression: the count/preview reader and the messages reader each derivedand re-add it directly above
it("keeps markup-heavy and very short user turns intact for the import path", ...).🤖 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/externalSessions/discoveryUtils.test.ts` around lines 105 - 118, Move the markup-density JSDoc block currently above the classifier-consistency test to directly above the import-path test named “keeps markup-heavy and very short user turns intact for the import path.” Keep the classifier-consistency regression comment immediately above “agrees across count, preview, and messages about what a user turn is.”apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts (1)
1925-1950: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case for the new parser.
Only the happy path is covered. A missing
laneId/expectedHeadshould surface the parser's specific error rather than reaching the service.💚 Suggested additional assertions
+ await expect(service.execute(makePayload("chat.fastForwardCrossMachineHandoffLane", { + expectedHead: capsule.source.headSha, + }))).rejects.toThrow("chat.fastForwardCrossMachineHandoffLane requires laneId."); + await expect(service.execute(makePayload("chat.fastForwardCrossMachineHandoffLane", { + laneId: "lane-2", + }))).rejects.toThrow("chat.fastForwardCrossMachineHandoffLane requires expectedHead.");🤖 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/services/sync/syncRemoteCommandService.test.ts` around lines 1925 - 1950, Extend the tests around the cross-machine handoff command parser to cover payloads missing laneId or expectedHead. Assert that each invalid payload returns the parser’s specific validation error and that execute does not reach the service handler, while preserving the existing happy-path assertions.apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
1604-1613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
merge --ff-onlyfailures.
installCrossMachineDestinationLaneGitFixtureexposesbranchRefandmergeExitCode, but the destination fast-forward tests never set them. SincemergeExitCodecontrols themerge --ff-onlyfailure path, add a case withmergeExitCode: 1or remove the unused fixture options.🤖 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/chat/agentChatService.test.ts` around lines 1604 - 1613, Add coverage for the merge --ff-only failure path by adding a destination fast-forward test that configures installCrossMachineDestinationLaneGitFixture with a valid branchRef and mergeExitCode: 1, then asserts the expected failure behavior. If that scenario is not needed, remove the unused branchRef and mergeExitCode fixture options instead.
🤖 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/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsx`:
- Around line 791-825: Remove the redundant standalone blocker UI near the
existing turn and publish controls, including the amber “Current turn is not
ready” block and standalone “Publish branch” button. Keep `continueBlockers` as
the single source for `turn-active`, `awaiting-input`, and `needs-push` actions,
rendered through `BlockedReasons` with its existing busy/disabled behavior.
In
`@apps/desktop/src/renderer/components/terminals/importSessions/affordances.test.ts`:
- Around line 288-319: Update sessionAnchors so it suppresses latest whenever
its text equals started, including titled single-message sessions. Add a test in
the sessionAnchors suite covering a titled session with one message and assert
latest is null while started remains the message text; preserve existing
behavior for distinct anchors and message-less sessions.
In
`@apps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsx`:
- Line 647: Add keyboard access to the bounded transcript container in
ImportSessionBrowser by giving the max-height overflow wrapper a non-negative
tabindex and an accessible name. Preserve its existing scrolling behavior while
ensuring keyboard-only users can focus and scroll the region.
In
`@apps/desktop/src/renderer/components/terminals/importSessions/sessionPresentation.ts`:
- Around line 48-63: Update sessionAnchors so latest is also excluded when its
text matches the started value, reusing the existing started and heading
comparisons. Preserve the current null handling and return latest only when it
differs from both started and heading, matching the externalSessionAnchors
behavior.
In `@apps/desktop/src/shared/crossMachineHandoff.ts`:
- Around line 144-152: Update requirePositiveInteger to validate numeric values
with Number.isSafeInteger and require them to be at least 1, rejecting unsafe
behindBy distances at the decoder boundary while preserving the existing error
and return behavior.
In `@apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift`:
- Around line 313-339: Scope chat-import configuration to session.provider
instead of screen-wide importProvider/importModelId state. Update the model
picker’s available provider/models or normalize the selected chat fields when
importSession handles a chat, ensuring model, permissionMode, reasoningEffort,
and fastMode all match the session provider while preserving existing behavior
for non-chat imports.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts`:
- Around line 1925-1950: Extend the tests around the cross-machine handoff
command parser to cover payloads missing laneId or expectedHead. Assert that
each invalid payload returns the parser’s specific validation error and that
execute does not reach the service handler, while preserving the existing
happy-path assertions.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 1604-1613: Add coverage for the merge --ff-only failure path by
adding a destination fast-forward test that configures
installCrossMachineDestinationLaneGitFixture with a valid branchRef and
mergeExitCode: 1, then asserts the expected failure behavior. If that scenario
is not needed, remove the unused branchRef and mergeExitCode fixture options
instead.
In `@apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts`:
- Around line 105-118: Move the markup-density JSDoc block currently above the
classifier-consistency test to directly above the import-path test named “keeps
markup-heavy and very short user turns intact for the import path.” Keep the
classifier-consistency regression comment immediately above “agrees across
count, preview, and messages about what a user turn is.”
🪄 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 Plus
Run ID: bf00616c-9089-4200-bdf6-d839c513b6c6
⛔ Files ignored due to path filters (7)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/cross-machine-session-handoff.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/external-session-import.mdis excluded by!docs/**
📒 Files selected for processing (38)
apps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/externalSessionBrowser.test.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/externalSessionBrowser.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/externalSessions/discoverClaude.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoverCursor.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.test.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsxapps/desktop/src/renderer/components/chat/crossMachineHandoffPresentation.tsxapps/desktop/src/renderer/components/shared/BlockedAction.tsxapps/desktop/src/renderer/components/shared/PermissionModePicker.tsxapps/desktop/src/renderer/components/shared/SessionLaunchModelControls.tsxapps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsxapps/desktop/src/renderer/components/terminals/importSessions/affordances.test.tsapps/desktop/src/renderer/components/terminals/importSessions/contract.tsapps/desktop/src/renderer/components/terminals/importSessions/sessionPresentation.tsapps/desktop/src/shared/crossMachineHandoff.tsapps/desktop/src/shared/types/chat.test.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/externalSessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkExternalSessionAffordances.swiftapps/ios/ADE/Views/Work/WorkImportSessionScreen.swiftapps/ios/ADETests/ADETests.swift
| describe("sessionAnchors", () => { | ||
| it("returns the opening ask and the latest message", () => { | ||
| const anchors = sessionAnchors(session({ | ||
| title: "Mobile chat truncation", | ||
| preview: "text is cut mid-word", | ||
| messages: [ | ||
| { role: "user", text: "text is cut mid-word", at: 1 }, | ||
| { role: "assistant", text: "Found it \u2014 byte-offset split.", at: 2 }, | ||
| { role: "user", text: "now shrink the logo", at: 3 }, | ||
| ], | ||
| })); | ||
| expect(anchors.started).toBe("text is cut mid-word"); | ||
| expect(anchors.latest?.text).toBe("now shrink the logo"); | ||
| }); | ||
|
|
||
| it("suppresses an anchor that would repeat the heading", () => { | ||
| // Untitled single-message thread: heading === preview === that message, so | ||
| // printing it again below reads as a rendering bug. | ||
| const anchors = sessionAnchors(session({ | ||
| title: null, | ||
| preview: "only one thing was ever said", | ||
| messages: [{ role: "user", text: "only one thing was ever said", at: 1 }], | ||
| })); | ||
| expect(anchors.started).toBeNull(); | ||
| expect(anchors.latest).toBeNull(); | ||
| }); | ||
|
|
||
| it("has no latest anchor on an older host that sends no messages", () => { | ||
| const anchors = sessionAnchors(session({ title: "Something", preview: "a preview" })); | ||
| expect(anchors.started).toBe("a preview"); | ||
| expect(anchors.latest).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppress latest when it duplicates started.
A titled single-message session currently renders the opening prompt twice: once as started and again as latest. Add this case and make sessionAnchors also reject a latest value equal to started, matching the CLI helper.
Proposed fix
- latest: latest && asHeadingText(latest.text) !== heading ? latest : null,
+ latest: latest
+ && asHeadingText(latest.text) !== heading
+ && asHeadingText(latest.text) !== started
+ ? latest
+ : null,🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 289-297: Enforce overriding default config
Context: session({
title: "Mobile chat truncation",
preview: "text is cut mid-word",
messages: [
{ role: "user", text: "text is cut mid-word", at: 1 },
{ role: "assistant", text: "Found it \u2014 byte-offset split.", at: 2 },
{ role: "user", text: "now shrink the logo", at: 3 },
],
})
Note: [CWE-1188] Insecure Default Initialization of Resource (default session cookie name not overridden).
(default-session-config-typescript)
[warning] 305-309: Enforce overriding default config
Context: session({
title: null,
preview: "only one thing was ever said",
messages: [{ role: "user", text: "only one thing was ever said", at: 1 }],
})
Note: [CWE-1188] Insecure Default Initialization of Resource (default session cookie name not overridden).
(default-session-config-typescript)
[warning] 315-315: Enforce overriding default config
Context: session({ title: "Something", preview: "a preview" })
Note: [CWE-1188] Insecure Default Initialization of Resource (default session cookie name not overridden).
(default-session-config-typescript)
🤖 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/renderer/components/terminals/importSessions/affordances.test.ts`
around lines 288 - 319, Update sessionAnchors so it suppresses latest whenever
its text equals started, including titled single-message sessions. Add a test in
the sessionAnchors suite covering a titled session with one message and assert
latest is null while started remains the message text; preserve existing
behavior for distinct anchors and message-less sessions.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21ccd9f625
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model: configuresChat ? importModelId : nil, | ||
| permissionMode: action.target == "chat" ? wire.permissionMode : nil, | ||
| reasoningEffort: configuresChat && !normalizedReasoning.isEmpty ? normalizedReasoning : nil, | ||
| fastMode: configuresChat && supportsFastMode ? importFastMode : nil |
There was a problem hiding this comment.
For every chat import, these newly sent model, permission, reasoning, and fast-mode values are discarded by externalSessionsService.ts: its target === "chat" branch calls importExternalChatSession with only provider/session/lane/cwd/fork/title, and the importer creates the chat with DEFAULT_CLAUDE_MODEL or DEFAULT_CODEX_MODEL. Consequently, the new iOS controls never affect the resulting chat—including a selected read-only access mode—so these fields need to be carried through ExternalChatImporter into chat creation.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
| ...(sourceCheck.needsPush | ||
| ? [{ | ||
| id: "needs-push", | ||
| title: `${sourceCheck.branch ?? "This branch"} hasn't been published`, | ||
| detail: "The other machine fetches your work from origin, so the branch has to exist there.", | ||
| fix: { label: "Publish branch", onFix: () => void publishBranch(), busy: Boolean(busyLabel) }, | ||
| }] |
There was a problem hiding this comment.
When the source has diverged, sync.ahead is positive, so needsPush is also true and this block now shows “hasn't been published” with a Publish branch fix beside the divergence blocker. The upstream branch already exists, and a normal push is non-fast-forward and cannot resolve the divergence; the previous UI deliberately hid this action whenever hasSourceBlock was true. Only offer this blocker for a genuinely unpublished/non-diverged branch, or distinguish publishing from reconciliation.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e529410f4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| role="listbox" | ||
| aria-label={ariaLabel} | ||
| data-permission-mode-picker-dropdown | ||
| className="fixed z-[100] overflow-hidden rounded-xl border border-white/[0.08] bg-[#13111A]/95 shadow-[0_18px_48px_rgba(0,0,0,0.55)] backdrop-blur-md" |
There was a problem hiding this comment.
When this shared picker is opened from CrossMachineHandoffModal, the portal is rendered at z-[100] while the modal overlay is at z-[190]. The option list therefore appears behind the modal/backdrop and cannot be clicked, making the newly added destination permission control unusable during cross-machine handoff. Use a layer above the modal or let the caller provide the portal layer.
Useful? React with 👍 / 👎.
Handing a chat to another machine dead-ended on a disabled Continue button with no stated reason: a branch that was pushed but behind origin pushed a blocker into a list that only ever fed `disabled`, under a green "main is pushed" row that read only the push direction. Even past that, the destination required its own lane to sit at the identical commit, which two machines' `main` never do. - Add a BlockedAction primitive. The button takes reasons, not a boolean, so a surface cannot disable its primary action without surfacing why. - Report behind/diverged on the branch row; offer Update branch for behind, and keep diverged a hard block rather than choosing a reconcile strategy. - Report laneFastForward from destination preflight and offer a repair that is re-validated there and only ever runs `git merge --ff-only`. - Render the destination's model, reasoning, fast mode and permission pills. The capsule always carried these and the destination always applied them; only the UI was missing, so handoffs shipped whatever the local drawer held. - Gate fork on cross-machine support. Droid's fork tab was live and threw. Imports rendered a raw <task-notification> blob under an "ADE - 9m ago" heading. Wrapper rejection is no longer a closed allow-list, previews may only come from prefix records so a tail receipt can't pose as the opening prompt, clipping stops at a word boundary, and rows carry started/latest anchors over a scrollable thread. Extract PermissionModePicker from AgentChatComposer into shared/ (removing a three-way duplicated trigger class), and mirror it all on iOS with lossy message decoding so one malformed element cannot drop a whole row. Co-Authored-By: Claude <noreply@anthropic.com>
- Delete the standalone turn-state panel and Publish button. `BlockedReasons` already renders both from `continueBlockers`, so the same blocker and the same fix appeared twice with different disabled behavior on each copy. - Suppress the `latest` anchor when it equals `started`, not just the heading. A titled single-message thread cleared the heading check and still printed the sentence twice; the TUI sibling already guarded this. - Make the bounded transcript focusable so keyboard-only users can scroll it. - Validate `behindBy` with isSafeInteger; isInteger admits 1e30. - Scope iOS chat-import model/reasoning/fast mode to the session's own provider. They came from screen-level composer preferences while the request sent `provider: session.provider`, so importing a Claude session while the saved model was Codex shipped a mismatch the host cannot honor. Co-Authored-By: Claude <noreply@anthropic.com>
…ntrols - Only surface the Publish blocker when a plain push can resolve it. A diverged branch is also ahead, so it reported needsPush and offered Publish right beside the divergence blocker — but its upstream exists and the push is rejected as non-fast-forward. The pre-refactor UI hid it via `!hasSourceBlock`; the blocker list lost that guard. - Remove the model, access mode, reasoning effort, and fast mode controls from the iOS import screen. The host discards all four for chat targets: externalSessionsService calls importExternalChatSession with only provider/session/lane/cwd/fork/title, and AgentChatImportExternalSessionArgs has no such fields. The controls changed nothing about the resulting chat, which is the exact bug class this branch exists to fix. Carrying them through chat creation is a separate change with its own provider-matching rules; the pre-existing model/permissionMode args on ExternalSessionImportArgs remain honoured for CLI targets. Co-Authored-By: Claude <noreply@anthropic.com>
e529410 to
2c172fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
29466-29590: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider verifying the lane worktree's checked-out branch before the ff-only merge.
The function re-validates fetch/reachability/dirty-state/ancestry from scratch (good — it doesn't trust the preflight snapshot), but never confirms that
refreshedLane.worktreePath's current branch is actuallybranchRefbefore runninggit merge --ff-only expectedHead. If that invariant is ever violated (e.g. a user manually checked out a different ref in the lane worktree), this would silently fast-forward whatever branch is currently checked out instead of failing clearly.💡 Proposed defensive check
const status = await runGit(["status", "--porcelain=v1"], { cwd: refreshedLane.worktreePath, timeoutMs: 15_000, }); if (status.exitCode !== 0) { ... } if (status.stdout.trim()) { ... } + + const currentBranch = await runGit(["symbolic-ref", "--short", "HEAD"], { + cwd: refreshedLane.worktreePath, + timeoutMs: 8_000, + }); + if (currentBranch.exitCode !== 0 || currentBranch.stdout.trim() !== branchRef) { + throw new Error( + `Cross-machine handoff lane '${refreshedLane.name}' is not currently on branch '${branchRef}'.`, + ); + }🤖 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/chat/agentChatService.ts` around lines 29466 - 29590, Update fastForwardCrossMachineHandoffLane to verify the refreshed lane worktree’s checked-out branch matches the validated branchRef before the merge. Use the refreshed worktree and an appropriate Git branch query, fail clearly when it differs or cannot be determined, and perform this check before git merge --ff-only while preserving the existing validation flow.apps/desktop/src/main/services/externalSessions/externalSessionsService.test.ts (1)
146-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text regex assertion is brittle.
Counting
messages: session.messagesoccurrences in the file passes for dead code, breaks on a rename or reformat, and silently under-asserts if a third summary path appears. SincefindExternalSummaryis reachable throughimportExternalSession, a behavioral assertion on the returned summary (or extracting the mapper and testing it directly) would pin the same contract without coupling to source text.🤖 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/externalSessions/externalSessionsService.test.ts` around lines 146 - 156, The test for optional preview fields in externalSessionsService.test.ts is coupled to source-text occurrence counts instead of runtime behavior. Replace the fs.readFileSync and regex assertions with a behavioral test that exercises importExternalSession and findExternalSummary, then verifies the returned summaries preserve both messages and preview fields across the supported summary paths.
🤖 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/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 29470-29590: Update fastForwardCrossMachineHandoffLane to require
and validate the established in-flight handoff/session context before performing
any fetch or merge, ensuring the requested laneId matches the previously
prepared handoff target. Reject generic or unrelated lane IDs, and preserve the
existing merge --ff-only validation only after this target check passes.
In `@apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift`:
- Around line 898-901: Update latestAnchorMessage to normalize the latest
message text and compare it against normalized rowHeading plus the displayed
“started” anchor text. Return nil when either comparison matches, preserving the
existing latest-message behavior for non-colliding text.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 29466-29590: Update fastForwardCrossMachineHandoffLane to verify
the refreshed lane worktree’s checked-out branch matches the validated branchRef
before the merge. Use the refreshed worktree and an appropriate Git branch
query, fail clearly when it differs or cannot be determined, and perform this
check before git merge --ff-only while preserving the existing validation flow.
In
`@apps/desktop/src/main/services/externalSessions/externalSessionsService.test.ts`:
- Around line 146-156: The test for optional preview fields in
externalSessionsService.test.ts is coupled to source-text occurrence counts
instead of runtime behavior. Replace the fs.readFileSync and regex assertions
with a behavioral test that exercises importExternalSession and
findExternalSummary, then verifies the returned summaries preserve both messages
and preview fields across the supported summary paths.
🪄 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 Plus
Run ID: 4b9bc045-ebf4-4d6b-803a-b1bee2d8f685
⛔ Files ignored due to path filters (2)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**
📒 Files selected for processing (38)
apps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/externalSessionBrowser.test.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/externalSessionBrowser.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/externalSessions/discoverClaude.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoverCursor.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.test.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsxapps/desktop/src/renderer/components/chat/crossMachineHandoffPresentation.tsxapps/desktop/src/renderer/components/shared/BlockedAction.tsxapps/desktop/src/renderer/components/shared/PermissionModePicker.tsxapps/desktop/src/renderer/components/shared/SessionLaunchModelControls.tsxapps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsxapps/desktop/src/renderer/components/terminals/importSessions/affordances.test.tsapps/desktop/src/renderer/components/terminals/importSessions/contract.tsapps/desktop/src/renderer/components/terminals/importSessions/sessionPresentation.tsapps/desktop/src/shared/crossMachineHandoff.tsapps/desktop/src/shared/types/chat.test.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/externalSessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkExternalSessionAffordances.swiftapps/ios/ADE/Views/Work/WorkImportSessionScreen.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (30)
- apps/desktop/src/shared/types/sync.ts
- apps/desktop/src/renderer/components/terminals/importSessions/contract.ts
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/desktop/src/main/services/externalSessions/discoverCursor.ts
- apps/desktop/src/shared/crossMachineHandoff.ts
- apps/desktop/src/renderer/components/shared/BlockedAction.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/ios/ADE/Services/SyncService.swift
- apps/desktop/src/shared/types/chat.ts
- apps/ade-cli/src/tuiClient/externalSessionBrowser.ts
- apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts
- apps/desktop/src/renderer/components/chat/crossMachineHandoffPresentation.tsx
- apps/desktop/src/main/services/externalSessions/discoverCodex.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/shared/types/chat.test.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/renderer/components/shared/SessionLaunchModelControls.tsx
- apps/ade-cli/src/tuiClient/components/RightPane.tsx
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
- apps/desktop/src/main/services/externalSessions/discoverClaude.ts
- apps/desktop/src/main/services/externalSessions/discoverProviders.test.ts
- apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts
- apps/desktop/src/renderer/components/terminals/importSessions/sessionPresentation.ts
- apps/desktop/src/renderer/components/shared/PermissionModePicker.tsx
- apps/desktop/src/shared/types/externalSessions.ts
- apps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsx
- apps/ios/ADE/Models/RemoteModels.swift
- apps/ios/ADE/Views/Work/WorkExternalSessionAffordances.swift
- apps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.test.tsx
- apps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsx
| const fastForwardCrossMachineHandoffLane = async ( | ||
| args: { laneId: string; expectedHead: string }, | ||
| ): Promise<{ ok: true; head: string }> => { | ||
| const laneId = typeof args.laneId === "string" ? args.laneId.trim() : ""; | ||
| if (!laneId) { | ||
| throw new Error("Cross-machine handoff lane fast-forward requires a destination lane."); | ||
| } | ||
| const expectedHead = typeof args.expectedHead === "string" ? args.expectedHead.trim() : ""; | ||
| if (!/^[0-9a-f]{40,64}$/i.test(expectedHead)) { | ||
| throw new Error("Cross-machine handoff lane fast-forward received an invalid source commit."); | ||
| } | ||
|
|
||
| const lane = await laneService.getSummary(laneId, { includeStatus: true }); | ||
| if (!lane || lane.archivedAt) { | ||
| throw new Error(`Cross-machine handoff lane '${laneId}' is unavailable or archived.`); | ||
| } | ||
| const branchRef = await requireGitBranchForHandoff(lane.branchRef, lane.worktreePath); | ||
| const fetch = await runGit( | ||
| ["fetch", "origin", `refs/heads/${branchRef}:refs/remotes/origin/${branchRef}`], | ||
| { | ||
| cwd: lane.worktreePath, | ||
| timeoutMs: 60_000, | ||
| env: await destinationGitEnv(), | ||
| }, | ||
| ); | ||
| if (fetch.exitCode !== 0) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${lane.name}' could not fetch '${branchRef}': ${fetch.stderr.trim() || "unknown Git error"}`, | ||
| ); | ||
| } | ||
| const fetchedHead = await requireGitOutputForHandoff( | ||
| ["rev-parse", `refs/remotes/origin/${branchRef}`], | ||
| lane.worktreePath, | ||
| `Cross-machine handoff lane '${lane.name}' could not resolve origin/${branchRef}.`, | ||
| ); | ||
| if (fetchedHead !== expectedHead) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${lane.name}' cannot fast-forward because origin/${branchRef} no longer points at the expected source commit.`, | ||
| ); | ||
| } | ||
| const reachable = await runGit(["cat-file", "-e", `${expectedHead}^{commit}`], { | ||
| cwd: lane.worktreePath, | ||
| timeoutMs: 8_000, | ||
| }); | ||
| if (reachable.exitCode !== 0) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${lane.name}' cannot reach the expected source commit after fetching origin.`, | ||
| ); | ||
| } | ||
|
|
||
| const refreshedLane = await laneService.getSummary(laneId, { includeStatus: true }); | ||
| if (!refreshedLane || refreshedLane.archivedAt) { | ||
| throw new Error(`Cross-machine handoff lane '${laneId}' became unavailable before it could be updated.`); | ||
| } | ||
| if (refreshedLane.status.rebaseInProgress) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' cannot fast-forward while a rebase is in progress.`, | ||
| ); | ||
| } | ||
| const status = await runGit(["status", "--porcelain=v1"], { | ||
| cwd: refreshedLane.worktreePath, | ||
| timeoutMs: 15_000, | ||
| }); | ||
| if (status.exitCode !== 0) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' could not be checked for local changes: ${status.stderr.trim() || "unknown Git error"}`, | ||
| ); | ||
| } | ||
| if (status.stdout.trim()) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' has uncommitted changes and cannot be fast-forwarded.`, | ||
| ); | ||
| } | ||
|
|
||
| const laneHead = await requireGitOutputForHandoff( | ||
| ["rev-parse", "HEAD"], | ||
| refreshedLane.worktreePath, | ||
| `Cross-machine handoff lane '${refreshedLane.name}' does not have a readable commit.`, | ||
| ); | ||
| if (laneHead === expectedHead) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' is already at the expected source commit.`, | ||
| ); | ||
| } | ||
| const ancestor = await runGit(["merge-base", "--is-ancestor", laneHead, expectedHead], { | ||
| cwd: refreshedLane.worktreePath, | ||
| timeoutMs: 15_000, | ||
| }); | ||
| if (ancestor.exitCode === 1) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' has diverged; its current commit is not an ancestor of the expected source commit.`, | ||
| ); | ||
| } | ||
| if (ancestor.exitCode !== 0) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' ancestry could not be verified: ${ancestor.stderr.trim() || "unknown Git error"}`, | ||
| ); | ||
| } | ||
|
|
||
| const merge = await runGit(["merge", "--ff-only", expectedHead], { | ||
| cwd: refreshedLane.worktreePath, | ||
| timeoutMs: 60_000, | ||
| }); | ||
| if (merge.exitCode !== 0) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' could not be fast-forwarded: ${merge.stderr.trim() || "git merge --ff-only failed"}`, | ||
| ); | ||
| } | ||
| const head = await requireGitOutputForHandoff( | ||
| ["rev-parse", "HEAD"], | ||
| refreshedLane.worktreePath, | ||
| `Cross-machine handoff lane '${refreshedLane.name}' could not verify the updated commit.`, | ||
| ); | ||
| if (head !== expectedHead) { | ||
| throw new Error( | ||
| `Cross-machine handoff lane '${refreshedLane.name}' did not reach the expected source commit.`, | ||
| ); | ||
| } | ||
| return { ok: true, head }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how fastForwardCrossMachineHandoffLane is registered/allowlisted
fd -HI 'registry.ts' apps/desktop/src/main/services/adeActions
fd -HI 'syncRemoteCommandService.ts' apps/ade-cli/src/services/sync
rg -n -C5 -i 'fastForward|laneFastForward|fast_forward' apps/desktop/src/main/services/adeActions/registry.ts apps/ade-cli/src/services/sync/syncRemoteCommandService.ts 2>/dev/nullRepository: arul28/ADE
Length of output: 4846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remote command service outline relevant sections =="
wc -l apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
sed -n '1,180p' apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
echo
sed -n '4100,4145p' apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
echo
echo "== inspect parser and registration flow =="
rg -n -C6 'register\(|viewerAllowed|remote command|remote-command|Remote.*Command|executeRemote|allowlist|allowed.*command|command.*allowed' apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
echo
echo "== inspect desktop registry type/mapping =="
rg -n -C6 'fastForwardCrossMachineHandoffLane|type .*Register|register|allowlist|viewerAllowed|queueable' apps/desktop/src/main/services/adeActions/registry.tsRepository: arul28/ADE
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== types for sync remote command policy/action/descriptor =="
rg -n -C4 'export (interface|type) (SyncRemoteCommand|SyncCommand|AgentChatRemoteCommand)|SyncRemoteCommandPolicy|SyncRemoteCommandAction|SyncRemoteCommandDescriptor|channelName|scope :' apps/shared apps/ade-cli/src apps/desktop/src/main/services/adeActions 2>/dev/null | head -n 220
echo
echo "== broad remote action definitions around chat actions type =="
sed -n '260,380p' apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
echo
echo "== fast forward implementation excerpt =="
sed -n '29000,29600p' apps/desktop/src/main/services/chat/agentChatService.tsRepository: arul28/ADE
Length of output: 19447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared sync remote command types =="
rg -n -C6 'SyncRemoteCommandAction|SyncRemoteCommandDescriptor|SyncRemoteCommandPolicy|SyncCommandPayload|SyncCommandAckPayload' apps/desktop/src/shared apps/ade-cli/src/services/sync/syncPeerService.ts apps/ade-cli/src/services/sync/syncHostService.ts | head -n 220
echo
echo "== syncPeerService execute/deny path =="
sed -n '640,715p' apps/ade-cli/src/services/sync/syncPeerService.ts
echo
echo "== syncHostService command routing enforcement =="
rg -n -C8 'executeRemoteCommand|remoteCommandDescriptors|isRemoteCommandAllowed|viewerAllowed|commandRouting|allowed.*remote|disallow|deny|unavailable' apps/ade-cli/src/services/sync/syncHostService.ts | head -n 320
echo
echo "== registry action type around chat actions =="
sed -n '480,570p' apps/desktop/src/main/services/adeActions/registry.tsRepository: arul28/ADE
Length of output: 37579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== registry function registering actions =="
sed -n '450,500p' apps/desktop/src/main/services/adeActions/registry.ts
echo
echo "== registry type definitions =="
rg -n -C8 'export type AdeAction|export interface AdeAction|adeActionsRegistry|export const actions|remoteCommand|commandRouting|actions.*chat|chat.*actions' apps/desktop/src/main/services/adeActions/registry.ts apps/desktop/src/shared apps/ade-cli/src/services/sync/syncPeerService.ts | sed -n '1,260p'
echo
echo "== remote command service supported actions/descriptors implementation =="
rg -n -C8 'getSupportedActions|getDescriptors|supportedActions|descriptor|scope|policy|registerChatRemoteCommands|registerAllRemoteCommands|remoteCommandService' apps/ade-cli/src/services/sync/syncRemoteCommandService.ts apps/desktop/src/main/services/adeActions/registry.ts
echo
echo "== typescript definitions exact =="
sed -n '1,80p' apps/desktop/src/shared/types/sync* 2>/dev/null || true
rg -n -C8 'type SyncRemoteCommandAction|interface SyncRemoteCommandAction|type SyncRemoteCommandDescriptor|interface SyncRemoteCommandDescriptor|SyncRemoteCommandPolicy' apps/desktop/src/shared apps/ade-cli/src 2>/dev/null | sed -n '1,240p'Repository: arul28/ADE
Length of output: 50366
Scope fastForwardCrossMachineHandoffLane to the established handoff handoff target.
The command is exposed to paired controllers with a generic { laneId, expectedHead } payload, so a caller can currently try laneId values outside the previously prepared/validated handoff target. Make the fast-forward path require an in-flight handoff/session context or otherwise scope the operation to the expected handoff target before the merge --ff-only.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/chat/agentChatService.ts` around lines 29470 -
29590, Update fastForwardCrossMachineHandoffLane to require and validate the
established in-flight handoff/session context before performing any fetch or
merge, ensuring the requested laneId matches the previously prepared handoff
target. Reject generic or unrelated lane IDs, and preserve the existing merge
--ff-only validation only after this target check passes.
There was a problem hiding this comment.
Skipping this one deliberately, with reasoning.
fastForwardCrossMachineHandoffLane grants no capability a paired controller does not already have. The sync surface already exposes git.discardFile, git.stageAll, lanes.archive, and lanes.resolveBranchDrift as viewerAllowed: true — all strictly more destructive than this method, which can only merge --ff-only a lane that is clean, not rebasing, and a strict ancestor, onto a commit that origin/<branch> already points at. No commits are lost and no arbitrary commit can be checked out. A paired controller is a trusted controller for the machine, which is the documented threat model.
Binding the call to an in-flight handoff is not available cheaply either: the fast-forward runs during preflight, before any acceptance, and the destination's durable handoff record is only created at accept time. Adding that binding means introducing new durable preflight state — a design change rather than a quick win, and one that would need its own idempotency story.
Leaving as-is and noting it as a follow-up if we later want preflight to be stateful.
There was a problem hiding this comment.
The Comment image shows the stylized “ADE” logo in large white block lettering with purple highlights and dark shadows, surrounded by layered wavy purple, pink, and blue contours on a black background.
@arul28, agreed. Given the trusted paired-controller threat model and the existing viewerAllowed mutation surface, this guarded merge --ff-only does not introduce a meaningful new capability. The clean-worktree, no-rebase, expected remote-head, reachability, and ancestor checks keep the operation non-destructive.
Requiring durable preflight handoff state would be a larger design change, so leaving this as a future consideration is appropriate. Withdrawing this finding.
🐇💭
✏️ Learnings added
Learnt from: arul28
Repo: arul28/ADE PR: 910
File: apps/desktop/src/main/services/chat/agentChatService.ts:29470-29590
Timestamp: 2026-07-27T16:39:36.856Z
Learning: In ADE cross-machine handoff flows, paired controllers are trusted controllers for the target machine. The sync remote-command surface intentionally permits viewer-allowed mutations including `git.discardFile`, `git.stageAll`, `lanes.archive`, and `lanes.resolveBranchDrift`. Therefore, `apps/desktop/src/main/services/chat/agentChatService.ts` `fastForwardCrossMachineHandoffLane` does not need to be bound to durable in-flight handoff state when it remains constrained to a clean, non-rebasing lane and a verified `git merge --ff-only` to the expected origin branch head. Preflight currently happens before accept-time durable handoff state exists; making it stateful requires a separate idempotent design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
… anchor - The permission list is portalled to document.body at z-[100], so opened from the handoff modal (overlay z-[190]) it rendered behind the backdrop and could not be clicked — the destination permission control was unusable. Let the caller supply the layer; the composer keeps its existing stacking. - Normalize both sides of the iOS latest-anchor comparison and check it against the started anchor too, matching the desktop and TUI rule. Skipped, with reasoning on the thread: binding fastForwardCrossMachineHandoffLane to an in-flight handoff. It grants no capability a paired controller lacks — git.discardFile, lanes.archive and lanes.resolveBranchDrift are already viewerAllowed and strictly more destructive — and the fast-forward runs during preflight, before any durable handoff record exists. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ea90e4e45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {destinationFastModeSupported && onFastModeChange ? ( | ||
| <button | ||
| type="button" | ||
| aria-pressed={Boolean(fastMode)} | ||
| onClick={() => onFastModeChange(!fastMode)} |
There was a problem hiding this comment.
When a fast-capable Claude model is selected, modelSupportsFastMode makes this new toggle visible, but crossMachineHandoffTarget in AgentChatPane.tsx still serializes fastMode only for Codex and OpenCode. Consequently, changing this toggle for Claude never reaches prepareCrossMachineHandoff; the service instead inherits the source session's fast setting, potentially starting the destination at the opposite tier from the UI selection. Pass the setting for every model for which this control is rendered.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| messages: recentExternalSessionMessagesFromRecords( | ||
| recentCodexRecords(record.sourcePath, lookupId != null), | ||
| ), |
There was a problem hiding this comment.
Normal Codex rollouts persist the same conversational turn as both a canonical event_msg and a mirrored response_item; the existing first-prompt and message-count paths explicitly prefer the canonical representation for this reason. Passing the raw suffix to the generic sampler classifies both representations as messages, so the new last-eight preview can duplicate user and assistant turns and evict genuine earlier exchanges, also producing incorrect latest anchors and search results. Filter or deduplicate the provider records in discovery before returning the shared summary rather than compensating in each client.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
| reasoningEffort: String? = nil, | ||
| fastMode: Bool? = nil |
There was a problem hiding this comment.
These new parameters are never supplied by the iOS import flow: WorkImportSessionScreen.importSession remains the only caller and passes only provider/session/lane/target/mode, with no reasoning or fast-mode state or controls anywhere in the screen. Thus every iOS ADE-chat import still sends both values as nil and uses host defaults, despite this change documenting that users can select and persist them. Add the chat-only controls and pass their selections through this interface.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
Same class as the rest of this branch — a surface that appears live but never reaches the thing it claims to configure. - Serialize destination fast mode for every model whose toggle the modal renders. It was spread only for Codex and OpenCode, so a fast-capable Claude model showed a working switch that never reached the capsule and the destination silently inherited the source's tier. - Drop Codex mirror rows before sampling messages. Codex writes each turn as both a canonical event_msg and a mirrored response_item, so previews showed every turn twice and evicted genuinely older exchanges from the capped window, skewing the latest anchor and search along with it. - Remove reasoningEffort and fastMode from the iOS importExternalSession signature. Nothing supplies them now that the inert controls are gone, and a parameter no caller can reach is the same lie in a different place. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c40d8b1202
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const summarized = summarizeNativeControls(providerGroup, nativeControls).permissionMode; | ||
| const current = options.some((option) => option.value === summarized) | ||
| ? summarized! | ||
| : options[0]!.value; |
There was a problem hiding this comment.
When a source chat has a Codex native-control combination that summarizeNativeControls cannot map to a preset—for example, approvalPolicy: "never" with sandbox: "workspace-write"—summarized is undefined, so this displays options[0] as “Default permissions.” The fallback is never written into nativeControls; AgentChatPane still serializes the original raw controls, and the destination runs them. A user can therefore send a handoff that appears to use the default/on-request policy while it actually skips approvals. Represent the state as Custom or normalize the underlying controls before allowing the handoff.
Useful? React with 👍 / 👎.
| const fetch = await runGit( | ||
| ["fetch", "origin", `refs/heads/${branchRef}:refs/remotes/origin/${branchRef}`], |
There was a problem hiding this comment.
When origin/<branch> on the destination still records a commit newer than or unrelated to the source commit after the remote branch was force-pushed or rewound, this unforced destination refspec is rejected as a non-fast-forward update. git fetch -h documents --force as “force overwrite of local reference”; without it, preflight adds a fetch blocker before checking whether the clean lane HEAD is actually an ancestor of the expected commit, so an otherwise safe fast-forward cannot be offered. Use a forced refspec (+refs/heads/...) or --force after the existing ls-remote equality check.
Useful? React with 👍 / 👎.
…king ref - A Codex native combination the presets can't express (approval "never" with sandbox "workspace-write", say) fell back to the first option and rendered as "Default permissions". The raw controls are what actually travel in the capsule, so the pill claimed the destination would ask for approval while it would skip it. Show Custom instead, unselectable, since choosing a real preset is what rewrites the underlying controls. - Fetch the destination's remote-tracking ref with a forced refspec. It is only a local mirror of origin, and when the destination still records a rewound or newer commit the unforced update is rejected as non-fast-forward, blocking a fast-forward that is otherwise safe. The existing origin/HEAD equality check is what gates the merge. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da2d17b682
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const applyHandoffNativeControls = useCallback((next: NativeControlState) => { | ||
| setHandoffClaudePermissionMode(next.claudePermissionMode); |
There was a problem hiding this comment.
When the source Claude chat is in plan mode, handoffNativeControlState carries interactionMode: "plan", but this callback drops the updated next.interactionMode. Selecting Default, Edit, or Full auto in the new destination picker therefore leaves summarizeNativeControls pinned to plan mode, so the picker does not change and the capsule still receives permissionMode: "plan". Store interaction mode in handoff-specific state alongside the other native controls rather than retaining the source chat's mode.
Useful? React with 👍 / 👎.
| message.role === "user" ? "text-violet-300/85" : "text-emerald-300/70", | ||
| )} | ||
| > | ||
| {message.role === "user" ? "you" : "ADE"} |
There was a problem hiding this comment.
For every sampled Claude or Codex transcript, assistant turns are labeled ADE even though they were authored by that external provider. This makes the newly added conversation preview misattribute history on desktop (and the iOS row repeats the same label); use the already available summary.provider display name instead.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
| `messages`. Chat imports also choose how the resulting ADE chat starts: model, | ||
| reasoning effort, fast mode (only where the model supports it), and permission | ||
| mode, seeded from and saved back to `WorkComposerPreferences` so the phone's | ||
| composer and its imports agree. Those fields are sent only for `target: "chat"`; |
There was a problem hiding this comment.
This newly documented iOS behavior does not exist in the reviewed implementation: WorkImportSessionScreen.importSession still sends only provider/session/lane/target/mode, and SyncService.importExternalSession accepts only optional model and permission mode—there are no model, reasoning, fast-mode, or permission controls in the screen and no reasoning/fast payload fields. The documentation currently tells users and maintainers that these settings are selectable and persisted when imports always use the existing defaults.
Useful? React with 👍 / 👎.
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)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
29144-29188: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
cursorModeIdinclusion condition checks the wrong variable — session fallback is silently dropped.
targetCursorModeIdis computed with an args-override-or-session-fallback pattern identical to every other field here (targetClaudePermissionMode,targetCodexApprovalPolicy, etc.), so it's meant to carry the session's current Cursor mode into the capsule when the caller doesn't explicitly override it. But the capsule spread condition testsargs.cursorModeId !== undefinedinstead oftargetCursorModeId !== undefined:...(args.cursorModeId !== undefined ? { cursorModeId: targetCursorModeId } : {}),Since callers of
prepareCrossMachineHandofftypically don't pass an explicitcursorModeIdoverride, this means the capsule omitscursorModeIdin the common case even whenmanaged.session.cursorModeIdholds real state (e.g."plan"or"full-auto"), silently dropping it and leaving the destination chat on its default Cursor mode instead of inheriting the source's mode.🐛 Proposed fix
- ...(args.cursorModeId !== undefined ? { cursorModeId: targetCursorModeId } : {}), + ...(targetCursorModeId !== undefined ? { cursorModeId: targetCursorModeId } : {}),🤖 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/chat/agentChatService.ts` around lines 29144 - 29188, Update the target object in prepareCrossMachineHandoff so the cursorModeId spread condition checks targetCursorModeId rather than args.cursorModeId, preserving the computed explicit-override or managed.session.cursorModeId fallback when present.
🤖 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 `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 29144-29188: Update the target object in
prepareCrossMachineHandoff so the cursorModeId spread condition checks
targetCursorModeId rather than args.cursorModeId, preserving the computed
explicit-override or managed.session.cursorModeId fallback when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44803371-e02d-4253-be07-a30283a70cc2
📒 Files selected for processing (11)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.test.tsxapps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsxapps/desktop/src/renderer/components/chat/crossMachineHandoffPresentation.tsxapps/desktop/src/renderer/components/shared/PermissionModePicker.tsxapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkImportSessionScreen.swift
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts
- apps/desktop/src/main/services/externalSessions/discoverCodex.ts
- apps/desktop/src/renderer/components/chat/crossMachineHandoffPresentation.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/renderer/components/shared/PermissionModePicker.tsx
- apps/desktop/src/main/services/externalSessions/discoveryUtils.ts
- apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift
- apps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.test.tsx
- apps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsx
1 participant
Summary by CodeRabbit
chat.fastForwardCrossMachineHandoffLane, enabling safe fast-forwarding of eligible destination lanes.