Final launch hardening: SSH host-key verification, fork-PR import, team reviewers, mac packaging (+ pre-merge audit)#527
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds SSH host-key trust IPC/UI and verification, browser owned-tab targeting/reuse and new navigation args, GitHub publish owner/identity and team-reviewer support, packaging/runtime validation and signing tweaks, sync-host lifecycle adjustments, and iOS sync route scheme updates with tests. ChangesIntegrated runtime trust, browser, GitHub, packaging, iOS
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
arul28
changed the title
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 3, 2026 23:26
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…am reviewers, mac packaging, sync fixes
Branch work (9 subsystems):
- Remote runtime: real OpenSSH known_hosts verification + TOFU trust flow (service→IPC→preload→UI); per-call RPC timeout tears down + reconnects
- PR/GitHub: fork-PR import via refs/pull/<n>/head; team reviewers; org-aware repo create returning {owner,name,fullName}; publish-dialog owner field
- Built-in browser: agent background tab driving; hidden-surface screenshots; Touch ID WebAuthn defaults off
- macOS packaging: per-target native-deps bundle+sign, opencode runtime packaging, universal-merge fixes, DMG staple retry
- CLI/TUI/sync: background browser tabs, team-reviewer parsing, prepare-then-retire sync host switch, hardened TUI arg parser
- IPC: production IPC handler timeouts, SSH trust channels, remote-clone credential guard, sync/foreground decoupling
- iOS: wss:// routes, broadened LAN-private auth handling, db-version advertisement fix
- CTO identity reclassified as local-ignored state
Audit fixes (adversarially verified):
- CRITICAL: node-pty prebuild fs.cp filter dropped whole target dir → empty prebuilds/ (PTY broken in remote runtime)
- HIGH: local runtime pool now retries idempotent reads after per-call timeout teardown (symmetry with remote pool) + regression test
- known_hosts no longer accepts a port-22 key on a non-standard port (OpenSSH semantics)
- ade-cli sync parser handles teamReviewers / team-only reviewer requests
- sync host no longer leaked if phone disconnects between prepare and ack
- completeProjectConnection retires by current active host, not stale request projectId
- iOS: clear orphaned keychain token when a saved profile is re-keyed
- gitignore apps/desktop/release-logs/; re-indent PublishToGitHubDialog tabs
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- automate: add isLocalRuntimeMethodTimeout predicate unit test; note keychain re-key cleanup in iOS companion doc - finalize: update syncRemoteCommandService test assertion to match the new "at least one reviewer or team reviewer" message from the team-reviewers change Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/ade-cli/src/services/sync/syncHostService.ts (1)
1983-2006:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate completion on an actual successful prepare result.
resultbeing truthy here does not mean “prepare succeeded” — a{ ok: false, message }payload is still truthy. That means the new fallback can still callcompleteProjectConnection(...)for ordinary rejected switches, which is the opposite of what the comment says and can run the retire path for a switch the client never accepted. Please guard this withresult?.ok === true(or another explicit success marker) in both places.Suggested fix
- try { - await args.projectCatalogProvider.completeProjectConnection?.(payload ?? {}, result); - } catch (completionError) { - args.logger.warn("sync_host.project_switch_completion_failed", { - message: completionError instanceof Error ? completionError.message : String(completionError), - }); - } + if (result.ok) { + try { + await args.projectCatalogProvider.completeProjectConnection?.(payload ?? {}, result); + } catch (completionError) { + args.logger.warn("sync_host.project_switch_completion_failed", { + message: completionError instanceof Error ? completionError.message : String(completionError), + }); + } + } ... - if (result) { + if (result?.ok) { try { await args.projectCatalogProvider.completeProjectConnection?.(payload ?? {}, result); } catch (completionError) { args.logger.warn("sync_host.project_switch_completion_failed", { message: completionError instanceof Error ? completionError.message : String(completionError), }); } }🤖 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/syncHostService.ts` around lines 1983 - 2006, The current logic treats any truthy result as a successful prepare, but prepareProjectConnection can return a payload like {ok:false,...} which is truthy; update the guards so you only call completeProjectConnection and run the retire path when the prepare result indicates success (e.g. result?.ok === true). Concretely, change the two places referencing result (the inner try after sendAndWait and the fallback inside the outer catch) to explicitly check result?.ok === true before invoking args.projectCatalogProvider.completeProjectConnection?.(payload ?? {}, result), and keep the existing try/catch and logging around those calls.apps/ade-cli/src/cli.ts (2)
8197-8229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
browser open --backgroundis documented but ignored.This path never consumes
--background, soade browser open <url> --backgroundfalls through to the normal navigation behavior instead of creating a non-active tab. That is especially surprising now that the help text explicitly advertises the flag.Proposed fix
const activeTab = readFlag(args, [ "--active-tab", "--current-tab", "--same-tab", ]); const newTab = readFlag(args, ["--new-tab"]); + const background = readFlag(args, ["--background"]); const showPanel = readFlag(args, ["--panel", "--show-panel", "--reveal-panel"]); const noPanel = readFlag(args, ["--no-panel", "--hidden"]); @@ if (!url.trim()) throw new CliUsageError("browser open requires a URL."); const autoReuseOwnedTab = - !newTab && !activeTab && !tabId && Boolean(claimArgs.laneId || claimArgs.chatSessionId); + !background && + !newTab && + !activeTab && + !tabId && + Boolean(claimArgs.laneId || claimArgs.chatSessionId); const agentOwnedCall = Boolean(claimArgs.laneId || claimArgs.chatSessionId); @@ url, tabId, - newTab: newTab && !activeTab ? true : undefined, - activate: agentOwnedCall && !activeTab && !showPanel ? false : undefined, + newTab: (newTab || background) && !activeTab ? true : undefined, + activate: background + ? false + : agentOwnedCall && !activeTab && !showPanel + ? false + : undefined, reuseOwnedTab: autoReuseOwnedTab ? true : undefined, openPanel: showPanel || (!noPanel && !agentOwnedCall),🤖 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.ts` around lines 8197 - 8229, Add handling for the --background flag by calling readFlag(args, ["--background"]) and using that value when building the actionStep payload: treat background as requesting a new tab (i.e., set the newTab field when background is true) and ensure activate is set to false when background is true; preserve existing agent-owned logic by falling back to the current activate expression (agentOwnedCall && !activeTab && !showPanel ? false : undefined) when background is not set. Update references to newTab and activate in the actionStep call (alongside existing variables newTab, activeTab, showPanel, noPanel, claimArgs, genericArgs) so --background actually creates a non-active tab.
12774-12807:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFailed sync-host handoffs can leave the registry on the wrong active host.
After
switchSyncHost(..., { deactivatePreviousHost: false }), every laterok: falsereturn exits without rolling back the active host choice. That leaves the previous phone socket alive, but the registry now points at the half-initialized host andcompleteProjectConnectionnever runs to retire anything. The next handoff will start from inconsistent state.🤖 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.ts` around lines 12774 - 12807, Capture the existing active host before calling scopeRegistry.switchSyncHost (e.g., call a getActiveHost/getCurrentHost on scopeRegistry or save scope returned from that call) and wrap the post-switch logic (everything after switchSyncHost up through the connectInfo check) in try/catch/finally so that on any failure you explicitly revert the registry to the previous active host or clean up the new half-initialized scope; specifically, after calling scopeRegistry.switchSyncHost(record.projectId, { deactivatePreviousHost: false }) save the prior host identifier, and if you return early (no connectInfo or missing scope/syncService) call the appropriate scopeRegistry API to reactivate the prior host or call scope.dispose()/scope.runtime.syncService.shutdown() to remove the new host before returning so the registry and live sockets remain consistent (refer to scopeRegistry.switchSyncHost, scope, syncService, and completeProjectConnection when implementing the rollback).apps/ade-cli/src/services/projects/projectScope.ts (1)
143-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the previous sync host if activating the new one fails.
When
deactivatePreviousHostis left at its default, the old host is disabled beforeget(projectId)/configureSyncHost(scope, true)completes. If that later step throws, Line 160 clearssyncHostProjectIdand leaves the previously active cached host turned off, so one transient startup failure can strand sync until the user switches again.Suggested rollback shape
} catch (error) { if (this.syncHostProjectId === projectId) { - this.syncHostProjectId = deactivatePreviousHost ? null : previousHostId; + this.syncHostProjectId = previousHostId ?? null; + if (previousHostId && previousHostId !== projectId && deactivatePreviousHost) { + await this.configureCachedSyncHost(previousHostId, true); + } } throw error; }🤖 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/projects/projectScope.ts` around lines 143 - 163, The switchSyncHost flow currently sets this.syncHostProjectId and deactivates the previous host before get(projectId)/configureSyncHost succeed, so if activation fails the old host remains disabled; modify switchSyncHost so that you (1) record previousHostId and whether you deactivated it (deactivatePreviousHost) but do not overwrite this.syncHostProjectId until after configureSyncHost succeeds, (2) if configureSyncHost or get throws, restore state by re-enabling the previous host via configureCachedSyncHost(previousHostId, true) when you previously deactivated it and ensure this.syncHostProjectId is reset to previousHostId (or null) instead of leaving it cleared; reference switchSyncHost, configureCachedSyncHost, configureSyncHost, get, syncHostProjectId and the deactivatePreviousHost flag when making the change.apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx (1)
400-412:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBuild the fallback snapshot from the latest target list.
Lines 401-411 close over
targetsfrom the render that createdconnectTarget. InsaveAndConnect()on Lines 491-505,setTargets(...)runs andconnectTarget()is awaited before that closure refreshes, so thecurrent == nullpath can rebuildfallbackConnectionsfrom the pre-save list. That can temporarily resurrectreplacedTargetIdor miss the just-saved target until another snapshot arrives.Also applies to: 487-505
🤖 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/remoteTargets/RemoteTargetList.tsx` around lines 400 - 412, The closure used in setConnectionSnapshot closes over the render-scoped targets array, causing fallbackConnections to be built from stale data (resurrecting replacedTargetId or missing newly saved targets); change the code to use the latest targets when constructing fallbackConnections—for example maintain a latestTargetsRef (updated whenever targets state changes) and read latestTargetsRef.current inside the setConnectionSnapshot updater (and similarly in saveAndConnect before calling connectTarget) so fallbackConnections is always built from the up-to-date targets list rather than the closed-over variable; update references to targets inside setConnectionSnapshot, connectTarget, and saveAndConnect accordingly.apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts (1)
94-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the remote read-retry classifier boundary-safe.
With timeouts now flowing through the reconnect path, this
startsWith()check can replay non-read actions whose names merely begin withget/list/etc. (getaway,listenStop, ...). The local pool already guards this with a camelCase boundary check; the remote pool needs the same protection before retrying remote actions.Suggested fix
function shouldRetryRemoteRuntimeAction( request: RemoteRuntimeActionRequest, ): boolean { if (RETRYABLE_REMOTE_ACTIONS.has(`${request.domain}.${request.action}`)) { return true; } - return RETRYABLE_REMOTE_ACTION_PREFIXES.some((prefix) => - request.action.startsWith(prefix), - ); + return RETRYABLE_REMOTE_ACTION_PREFIXES.some( + (prefix) => + request.action === prefix || + (request.action.startsWith(prefix) && /^[A-Z]/.test(request.action.slice(prefix.length))), + ); }🤖 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/remoteRuntime/remoteConnectionPool.ts` around lines 94 - 103, The retry classifier in shouldRetryRemoteRuntimeAction is too permissive because RETRYABLE_REMOTE_ACTION_PREFIXES.some(prefix => request.action.startsWith(prefix)) will match names like "getaway"; update shouldRetryRemoteRuntimeAction to perform a camelCase boundary check similar to the local pool: for each prefix in RETRYABLE_REMOTE_ACTION_PREFIXES accept the action only if action === prefix or action starts with prefix and the next character after the prefix is an uppercase letter (A-Z), otherwise do not treat it as a match; keep the existing RETRYABLE_REMOTE_ACTIONS exact-match check unchanged.apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (1)
712-727:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry
projects.addtimeouts inensureProject().
RuntimeRpcClientnow turns any per-call timeout into a terminal client failure, but this catch block only retriesisLocalRuntimeConnectionDropped(). If the first call on a cold project times out inprojects.add, the pool still surfaces the timeout instead of reconnecting, so idempotent reads fail before they ever reach the new action-level retry path.Suggested fix
- if (!isLocalRuntimeConnectionDropped(projectError)) { + const dropped = isLocalRuntimeConnectionDropped(projectError); + const timedOut = isLocalRuntimeMethodTimeout(projectError); + if (!dropped && !timedOut) { throw projectError; } this.logger.warn("local_runtime.ensure_project_connection_dropped", { rootPath: normalizedRoot, socketPath: entry.socketPath, attempt, willRetry: attempt < 2, error: projectError.message, }); - this.resetActiveConnection(entry); + if (timedOut) this.resetConnectionAfterActionTimeout(entry); + else this.resetActiveConnection(entry); lastError = projectError; if (attempt < 2) continue; throw projectError;🤖 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/localRuntime/localRuntimeConnectionPool.ts` around lines 712 - 727, The catch in ensureProject() only retries when isLocalRuntimeConnectionDropped(projectError) is true, but RuntimeRpcClient turns per-call timeouts into terminal client failures so timeouts from projects.add should also trigger a reconnect/retry; update the catch in localRuntimeConnectionPool.ts (the block handling projectError, resetActiveConnection, logger.warn and lastError) to treat timeout-style RuntimeRpcClient failures as dropped connections by extending the condition—e.g., add a new predicate (isRuntimeRpcTimeout or similar) that checks for the RuntimeRpcClient timeout signature (error.code === 'ETIMEDOUT', error.name or message containing "timeout", or the specific client error shape your RuntimeRpcClient emits) and use if (!isLocalRuntimeConnectionDropped(projectError) && !isRuntimeRpcTimeout(projectError)) throw projectError; keep calling this.resetActiveConnection(entry), logging via this.logger.warn, and retrying exactly as before when the error is considered dropped.
🧹 Nitpick comments (3)
apps/ade-cli/src/tuiClient/__tests__/cli.test.tsx (1)
37-53: ⚡ Quick winAdd one happy-path test for
--socket/ desktop RPC mode.This suite covers the new failure cases and embedded mode, but not the socket-backed path that the parser also changed. A small assertion that
runAdeCodeCli(["--socket", "/tmp/ade.sock"])forwards the socket path would close that gap. Based on learnings, For ADE CLI changes, verify both headless mode and the desktop socket-backed ADE RPC 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/ade-cli/src/tuiClient/__tests__/cli.test.tsx` around lines 37 - 53, Add a happy-path test that calls runAdeCodeCli(["--socket", "/tmp/ade.sock"]) and asserts it resolves to 0, and verify the component that receives the socket (the RPC wiring/helper used in the CLI — e.g., the function that creates or configures the RPC client) was called with "/tmp/ade.sock"; keep the pattern consistent with the existing tests (use await expect(...).resolves.toBe(0) and a Jest spy/mock assertion similar to how renderMock is checked) so the parser and runtime path for socket-backed desktop RPC is covered alongside the existing embedded/headless test using runAdeCodeCli and parseArgs.apps/desktop/src/main/services/ipc/runtimeBridge.test.ts (1)
818-900: ⚡ Quick winAdd a non-GitHub clone regression case.
These tests only cover
github.comclone URLs, so they won't catch accidental PAT forwarding to arbitrary HTTPS origins. Add a case wherehasKnownSshHostKeyForTargetMockistruebut the clone URL points at a non-GitHub host, and assertgithubAuthHeaderis still omitted.🤖 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/ipc/runtimeBridge.test.ts` around lines 818 - 900, Add a test that verifies PATs are not forwarded for non-GitHub HTTPS origins: registerRuntimeBridge with a getGitHubTokenForRemoteClone mock (or provide a githubAuthHeader in the IPC call), set remoteRegistryGetMock to return target and hasKnownSshHostKeyForTargetMock to return true, then call ipcHandlers.get(IPC.remoteRuntimeCloneProject) with a clone URL on a non-GitHub host (e.g., "https://gitlab.com/org/repo.git") and assert the resolved object, that getGitHubTokenForRemoteClone was not called (or ignored), and that remoteCallMachineForTargetMock was called with the same params but without a githubAuthHeader property (i.e., projects.clone invoked without githubAuthHeader).apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts (1)
1157-1174: ⚡ Quick winAdd a team-reviewers-only success test for the updated contract.
Line 1161 updates the validation message, but this block still only proves the
reviewerspath. Add one test wherereviewersis empty/omitted andteamReviewersis provided to lock in the new OR behavior.Proposed test addition
it("prs.requestReviewers routes with valid reviewers", async () => { const result = await service.execute(makePayload("prs.requestReviewers", { prId: "pr-1", reviewers: ["alice", "bob"], })); expect(prService.requestReviewers).toHaveBeenCalledWith({ prId: "pr-1", reviewers: ["alice", "bob"], }); expect(result).toEqual({ ok: true }); }); + + it("prs.requestReviewers routes when only team reviewers are provided", async () => { + const result = await service.execute(makePayload("prs.requestReviewers", { + prId: "pr-1", + teamReviewers: ["mobile-team"], + })); + expect(prService.requestReviewers).toHaveBeenCalledWith({ + prId: "pr-1", + teamReviewers: ["mobile-team"], + }); + expect(result).toEqual({ ok: true }); + });As per coding guidelines: “Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes.”
🤖 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 1157 - 1174, Add a new spec in the same describe block that calls service.execute(makePayload("prs.requestReviewers", { prId: "pr-1", teamReviewers: ["team-a"] })) and assert that prService.requestReviewers was invoked with { prId: "pr-1", teamReviewers: ["team-a"] } and that the result equals { ok: true }; use the same naming style as the other tests (e.g., "prs.requestReviewers routes with teamReviewers only") and reuse makePayload/service.execute/prService.requestReviewers to lock in the new OR validation behavior.
🤖 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/scripts/notarize-static-runtime.mjs`:
- Around line 73-89: The isMachO function currently checks for Mach-O magic
numbers but omits the fat64/universal signatures; update the array in
isMachO(...) to also include the 'cafebabf' and 'bfbafeca' hex magic strings
(the 4-byte signatures for fat64) alongside the existing values so
fat64/universal Mach-O binaries are detected and processed; no other logic
changes are required—just add those two entries to the includes(...) list.
- Around line 130-131: The current sequence deletes archivePath then runs tar,
risking loss if tar fails; instead create a temporary target (e.g. archivePath +
".tmp" or use a random/unique temp name), run run("tar", ["-czf",
tempArchivePath, "-C", workDir, "."]) to write the new archive into that temp
file, verify success, and then atomically replace the original by renaming
tempArchivePath to archivePath (using fs.rename or equivalent); reference
archivePath, workDir, fs.rm and the run("tar", ["-czf", ...]) call to locate
where to change the logic.
In `@apps/ade-cli/src/cli.ts`:
- Around line 2193-2204: The parsing loop currently pushes "team:backend" into
teamReviewers when the token includes the "team:" prefix; change the logic so
that when value.toLowerCase().startsWith("team:") you strip the "team:" prefix
and push only the normalized reviewer name (preserving the original casing after
the colon) into teamReviewers, leaving other branches (the "/" path and regular
reviewers) unchanged; update the loop that fills reviewers and teamReviewers in
apps/ade-cli/src/cli.ts (the variables reviewers, teamReviewers and the value
processing inside the for..of over args) to use value.slice(value.indexOf(":") +
1).trim() (or equivalent) when handling the "team:" case.
In `@apps/desktop/package.json`:
- Around line 243-253: The extraResources runtime filter currently only includes
Darwin/Linux artifacts so Windows ADE runtimes are omitted; update the "filter"
array under the extraResources block (the object with "from":
"resources/runtime" and "to": "runtime") to also include the Windows artifact
names (e.g., add ade-win32-x64, ade-win32-x64.native.tar.gz and ade-win32-ia32,
ade-win32-ia32.native.tar.gz or whatever Windows artifact filenames your build
produces) so Windows builds (dist:win) will package the runtime payload
alongside the Darwin/Linux entries.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 863-865: The function reusableOwnedTabForInput currently returns
the first tab matching tabMatchesOwnerInput in creation order; change it to
prefer the most recently active/newest matching lane-owned tab by searching tabs
in reverse (or by using a last-active timestamp on BrowserTabState if available)
so actions (click/startSession/navigate with reuseOwnedTab) target the user's
most recently used tab for that lane; update reusableOwnedTabForInput to
pruneDestroyedTabs(), then locate the matching entry via reverse iteration (or
findLast) of tabs using tabMatchesOwnerInput to return the newest/active match,
keeping the same null fallback.
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 1145-1149: The POST is incorrectly sent to /orgs/{owner}/repos for
personal publishes; update the logic around owner and the apiRequest call so you
first determine whether the requested owner is the authenticated user and only
use /orgs/{owner}/repos for real org targets. Concretely, use the existing owner
variable, fetch the authenticated account (e.g. call apiRequest GET /user to
read the authenticated login) and compare it to owner; if they match use the
/user/repos path, otherwise use /orgs/${encodeURIComponent(owner)}/repos when
creating the repo in the apiRequest call.
In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts`:
- Around line 583-601: The code attaches a GitHub PAT (githubAuthHeader) based
only on runtime trust; instead, first verify the clone URL is a GitHub host or
an explicitly allowlisted GitHub Enterprise host before adding the header.
Update the block around githubAuthHeader creation to parse safeInput.url (or
input.url), check via a new or existing helper like
isGitHubHostOrAllowlisted(url) (implement host extraction and allowlist logic),
and only call createGitHubAuthHeader/getGitHubTokenForRemoteClone when the URL
check passes and the target is trusted (hasKnownSshHostKeyForTarget). Then pass
githubAuthHeader to remoteConnectionService.cloneProject only when both the host
check and target-trust check succeeded.
In `@apps/desktop/src/main/services/prs/prService.test.ts`:
- Around line 2621-2628: The test currently asserts the POST body via
expect(apiRequest).toHaveBeenCalledWith(...), but doesn't ensure only one
matching POST was sent; add an assertion that exactly one apiRequest call
targeted the requested_reviewers endpoint. After the existing expect,
compute/count apiRequest.mock.calls entries whose first-argument object has path
equal to `/repos/${REPO.owner}/${REPO.name}/pulls/90/requested_reviewers` (and
method "POST" if desired) and assert that count === 1 so the batching contract
is enforced; reference the mock function apiRequest and the REPO constant in
prs/prService.test.ts near the existing expectation.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 4129-4135: computeReviewStatus currently treats a PR as
"requested" only when requestedReviewers.length > 0 and ignores team requests;
update computeReviewStatus to accept and check requestedTeams (or
requested_teams) and return "requested" when either requestedReviewers.length >
0 or requestedTeams.length > 0. Then update call sites (refreshOne,
computeStatus) that call computeReviewStatus with { requestedReviewers,
reviewStatesByUser } to include the teams field (e.g., { requestedReviewers,
requestedTeams: requested_teams, reviewStatesByUser }) so PRs that request only
teams compute reviewStatus correctly.
- Around line 2494-2499: Remove the early-return gate in fetchPrHeadImportRef
that checks args.headRepoOwner/args.headRepoName and instead allow fork imports
to proceed; when formatting messages that previously used
headRepoOwner/headRepoName, substitute a safe "unknown" string when those values
are null and keep calling createLaneFromPrBranchBlock only for true errors (use
createLaneFromPrBranchBlock("fork_unavailable", ...) only on real fetch
failures). Also ensure team-requested reviewers are included when computing
reviewStatus: update the computeReviewStatus call sites (where reviewStatus is
derived, e.g., the computeReviewStatus(...) invocation and the code around it)
to merge pr.requested_teams / team_reviewers into the requestedReviewers input
(or pass an aggregated requestedReviewers array that includes team reviewers) so
team requests are reflected in reviewStatus.
In `@apps/desktop/src/main/services/remoteRuntime/sshTransport.test.ts`:
- Around line 390-457: Add a regression test that asserts
getSshHostKeyTrustForTarget(...) returns state "changed" when known_hosts
already contains a different key: create a temp SSH home with
createSshHomeWithKnownHosts containing a knownKey, simulate a scan returning a
different scannedKey via scannedHostKey, call
getSshHostKeyTrustForTarget(target, { env: {}, homeDir, sshConfigPath: null,
scanHostKey: async () => scannedHostKey({ homeDir, key: scannedKey }) }) and
expect the resolved object toHave property state: "changed" (and include
targetId/host/fingerprint if desired) to cover the mismatch branch.
In `@apps/desktop/src/main/services/remoteRuntime/sshTransport.ts`:
- Around line 500-523: The current early return using knownHostsHaveEntry(...)
incorrectly assumes any stored key means the host is still valid; remove the
short-circuit and instead always perform a host-key scan (use
scanSshHostKeyForTarget or the provided scanHostKey) and compare the scanned key
against known hosts via knownHostsTrustState to distinguish "trusted" vs
"changed" vs "needs_trust". Concretely, stop returning immediately from
getSshHostKeyTrustForTarget when knownHostsHaveEntry(...) is true; invoke the
scan, read scanEntries (readKnownHostEntries), compute trustState with
knownHostsTrustState(scanEntries, scanCandidates, scan.key), and then return the
state and remoteHostKeyIdentityFromScan(scan) as currently done for
non-short-circuited paths.
In
`@apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx`:
- Around line 142-145: The branch handling lower.startsWith("team:") currently
pushes the raw sliced value into teamReviewers, causing "team:org/platform" to
remain "org/platform" while plain "org/platform" is normalized elsewhere; change
the block so after value = value.slice("team:".length).trim() you apply the same
slash-delimited normalization used for other inputs (e.g., if
value.includes('/') then set value = value.split('/').pop() or call the shared
normalize function) before pushing into teamReviewers so both forms produce the
same slug; update the code around lower.startsWith("team:"), value, and
teamReviewers accordingly.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx`:
- Around line 825-829: The assertions in GitHubTab.test.tsx are querying the
entire document (screen.getAllByText(...)) which can match duplicates elsewhere;
instead locate the preflight dialog element (use await
screen.findByRole("dialog", { name: /create lane from pr branch/i })) and use
within(preflightDialog).getByText or within(preflightDialog).getAllByText(...)
for the subsequent checks (for "`#200` Unlinked PR", "origin/feature/open",
"Unlinked PR", "main") so the assertions are scoped to the dialog; apply the
same change to the similar assertions around the 910-913 region.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx`:
- Around line 460-466: The assertions about local-state cleanup are racing the
async bulk-delete handler; move the checks that verify
workMocks.currentWork.removeSessionFromList calls (and the negative check for
"shell-running") into the same waitFor that asserts agentChatDelete was called
so the test waits for the post-delete cleanup to finish; e.g., after awaiting
waitFor(() => expect(agentChatDelete).toHaveBeenCalledTimes(2)), include
expectations for workMocks.currentWork.removeSessionFromList having been called
with "chat-running-codex" and "chat-running-claude" and not with "shell-running"
(and keep the sessionDelete negative assertion inside that waitFor block or in a
follow-up waitFor that asserts cleanup completion).
In `@apps/desktop/src/shared/types/prs.ts`:
- Around line 1229-1233: RequestPrReviewersArgs currently allows { prId } with
no reviewers; change its shape to require at least one reviewer target by
replacing the single optional-fields type with a discriminated union such as: a
variant with prId + reviewers: string[] (non-empty), a variant with prId +
teamReviewers: string[] (non-empty), and a variant with both arrays present.
Update the RequestPrReviewersArgs type declaration (refer to the
RequestPrReviewersArgs symbol) to use that union so callers must provide at
least one reviewer array, and ensure any consumer code that constructs or
consumes this type (renderer/IPC handlers) validates arrays are non-empty before
sending/acting.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5868-5879: When re-keying a profile (in the block that computes
token = storedTokenForSavedProfile(profile), removes profiles[key], computes
updatedKey = profileStorageKey(updated), and sets profiles[updatedKey] =
updated), ensure the valid token is moved to the new key instead of preserving
any existing token at updatedKey; change the logic around
keychain.saveToken/clearToken so that if let token and updatedKey != key then
always call keychain.saveToken(token, hostKey: updatedKey) (overwriting any
stale token) and then call keychain.clearToken(hostKey: key) to remove the old
token.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 8197-8229: Add handling for the --background flag by calling
readFlag(args, ["--background"]) and using that value when building the
actionStep payload: treat background as requesting a new tab (i.e., set the
newTab field when background is true) and ensure activate is set to false when
background is true; preserve existing agent-owned logic by falling back to the
current activate expression (agentOwnedCall && !activeTab && !showPanel ? false
: undefined) when background is not set. Update references to newTab and
activate in the actionStep call (alongside existing variables newTab, activeTab,
showPanel, noPanel, claimArgs, genericArgs) so --background actually creates a
non-active tab.
- Around line 12774-12807: Capture the existing active host before calling
scopeRegistry.switchSyncHost (e.g., call a getActiveHost/getCurrentHost on
scopeRegistry or save scope returned from that call) and wrap the post-switch
logic (everything after switchSyncHost up through the connectInfo check) in
try/catch/finally so that on any failure you explicitly revert the registry to
the previous active host or clean up the new half-initialized scope;
specifically, after calling scopeRegistry.switchSyncHost(record.projectId, {
deactivatePreviousHost: false }) save the prior host identifier, and if you
return early (no connectInfo or missing scope/syncService) call the appropriate
scopeRegistry API to reactivate the prior host or call
scope.dispose()/scope.runtime.syncService.shutdown() to remove the new host
before returning so the registry and live sockets remain consistent (refer to
scopeRegistry.switchSyncHost, scope, syncService, and completeProjectConnection
when implementing the rollback).
In `@apps/ade-cli/src/services/projects/projectScope.ts`:
- Around line 143-163: The switchSyncHost flow currently sets
this.syncHostProjectId and deactivates the previous host before
get(projectId)/configureSyncHost succeed, so if activation fails the old host
remains disabled; modify switchSyncHost so that you (1) record previousHostId
and whether you deactivated it (deactivatePreviousHost) but do not overwrite
this.syncHostProjectId until after configureSyncHost succeeds, (2) if
configureSyncHost or get throws, restore state by re-enabling the previous host
via configureCachedSyncHost(previousHostId, true) when you previously
deactivated it and ensure this.syncHostProjectId is reset to previousHostId (or
null) instead of leaving it cleared; reference switchSyncHost,
configureCachedSyncHost, configureSyncHost, get, syncHostProjectId and the
deactivatePreviousHost flag when making the change.
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 1983-2006: The current logic treats any truthy result as a
successful prepare, but prepareProjectConnection can return a payload like
{ok:false,...} which is truthy; update the guards so you only call
completeProjectConnection and run the retire path when the prepare result
indicates success (e.g. result?.ok === true). Concretely, change the two places
referencing result (the inner try after sendAndWait and the fallback inside the
outer catch) to explicitly check result?.ok === true before invoking
args.projectCatalogProvider.completeProjectConnection?.(payload ?? {}, result),
and keep the existing try/catch and logging around those calls.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 712-727: The catch in ensureProject() only retries when
isLocalRuntimeConnectionDropped(projectError) is true, but RuntimeRpcClient
turns per-call timeouts into terminal client failures so timeouts from
projects.add should also trigger a reconnect/retry; update the catch in
localRuntimeConnectionPool.ts (the block handling projectError,
resetActiveConnection, logger.warn and lastError) to treat timeout-style
RuntimeRpcClient failures as dropped connections by extending the
condition—e.g., add a new predicate (isRuntimeRpcTimeout or similar) that checks
for the RuntimeRpcClient timeout signature (error.code === 'ETIMEDOUT',
error.name or message containing "timeout", or the specific client error shape
your RuntimeRpcClient emits) and use if
(!isLocalRuntimeConnectionDropped(projectError) &&
!isRuntimeRpcTimeout(projectError)) throw projectError; keep calling
this.resetActiveConnection(entry), logging via this.logger.warn, and retrying
exactly as before when the error is considered dropped.
In `@apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts`:
- Around line 94-103: The retry classifier in shouldRetryRemoteRuntimeAction is
too permissive because RETRYABLE_REMOTE_ACTION_PREFIXES.some(prefix =>
request.action.startsWith(prefix)) will match names like "getaway"; update
shouldRetryRemoteRuntimeAction to perform a camelCase boundary check similar to
the local pool: for each prefix in RETRYABLE_REMOTE_ACTION_PREFIXES accept the
action only if action === prefix or action starts with prefix and the next
character after the prefix is an uppercase letter (A-Z), otherwise do not treat
it as a match; keep the existing RETRYABLE_REMOTE_ACTIONS exact-match check
unchanged.
In `@apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx`:
- Around line 400-412: The closure used in setConnectionSnapshot closes over the
render-scoped targets array, causing fallbackConnections to be built from stale
data (resurrecting replacedTargetId or missing newly saved targets); change the
code to use the latest targets when constructing fallbackConnections—for example
maintain a latestTargetsRef (updated whenever targets state changes) and read
latestTargetsRef.current inside the setConnectionSnapshot updater (and similarly
in saveAndConnect before calling connectTarget) so fallbackConnections is always
built from the up-to-date targets list rather than the closed-over variable;
update references to targets inside setConnectionSnapshot, connectTarget, and
saveAndConnect accordingly.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/__tests__/cli.test.tsx`:
- Around line 37-53: Add a happy-path test that calls runAdeCodeCli(["--socket",
"/tmp/ade.sock"]) and asserts it resolves to 0, and verify the component that
receives the socket (the RPC wiring/helper used in the CLI — e.g., the function
that creates or configures the RPC client) was called with "/tmp/ade.sock"; keep
the pattern consistent with the existing tests (use await
expect(...).resolves.toBe(0) and a Jest spy/mock assertion similar to how
renderMock is checked) so the parser and runtime path for socket-backed desktop
RPC is covered alongside the existing embedded/headless test using runAdeCodeCli
and parseArgs.
In `@apps/desktop/src/main/services/ipc/runtimeBridge.test.ts`:
- Around line 818-900: Add a test that verifies PATs are not forwarded for
non-GitHub HTTPS origins: registerRuntimeBridge with a
getGitHubTokenForRemoteClone mock (or provide a githubAuthHeader in the IPC
call), set remoteRegistryGetMock to return target and
hasKnownSshHostKeyForTargetMock to return true, then call
ipcHandlers.get(IPC.remoteRuntimeCloneProject) with a clone URL on a non-GitHub
host (e.g., "https://gitlab.com/org/repo.git") and assert the resolved object,
that getGitHubTokenForRemoteClone was not called (or ignored), and that
remoteCallMachineForTargetMock was called with the same params but without a
githubAuthHeader property (i.e., projects.clone invoked without
githubAuthHeader).
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts`:
- Around line 1157-1174: Add a new spec in the same describe block that calls
service.execute(makePayload("prs.requestReviewers", { prId: "pr-1",
teamReviewers: ["team-a"] })) and assert that prService.requestReviewers was
invoked with { prId: "pr-1", teamReviewers: ["team-a"] } and that the result
equals { ok: true }; use the same naming style as the other tests (e.g.,
"prs.requestReviewers routes with teamReviewers only") and reuse
makePayload/service.execute/prService.requestReviewers to lock in the new OR
validation behavior.
🪄 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: 9c2252bf-ea38-4836-b663-a3a0ff433f1a
⛔ Files ignored due to path filters (9)
.ade/.gitignoreis excluded by!.ade/**.ade/cto/identity.yamlis excluded by!.ade/**docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-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/**
📒 Files selected for processing (87)
.gitignoreapps/ade-cli/scripts/build-static.mjsapps/ade-cli/scripts/notarize-static-runtime.mjsapps/ade-cli/scripts/package-native-deps.mjsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/projects/projectScope.test.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.test.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/cli.test.tsxapps/ade-cli/src/tuiClient/cli.tsxapps/desktop/build/entitlements.mac.inherit.plistapps/desktop/build/entitlements.mac.plistapps/desktop/package.jsonapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/notarize-mac-dmg.mjsapps/desktop/scripts/prepare-universal-mac-inputs.mjsapps/desktop/scripts/release-mac-local.mjsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.tsapps/desktop/src/main/services/cto/ctoState.test.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/projects/adeProjectService.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.test.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.tsapps/desktop/src/main/services/remoteRuntime/sshTransport.test.tsapps/desktop/src/main/services/remoteRuntime/sshTransport.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/projects/PublishToGitHubDialog.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailRightRail.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/renderer/components/usage/usage.test.tsxapps/desktop/src/renderer/lib/sessions.test.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/adeLayout.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (5)
- apps/desktop/build/entitlements.mac.plist
- apps/desktop/src/main/services/projects/adeProjectService.ts
- apps/desktop/src/shared/adeLayout.ts
- apps/desktop/build/entitlements.mac.inherit.plist
- apps/ade-cli/src/multiProjectRpcServer.ts
Comment on lines
243
to
+253
| "from": "resources/runtime", | ||
| "to": "runtime", | ||
| "filter": [ | ||
| "**/*" | ||
| "ade-darwin-arm64", | ||
| "ade-darwin-arm64.native.tar.gz", | ||
| "ade-darwin-x64", | ||
| "ade-darwin-x64.native.tar.gz", | ||
| "ade-linux-arm64", | ||
| "ade-linux-arm64.native.tar.gz", | ||
| "ade-linux-x64", | ||
| "ade-linux-x64.native.tar.gz" |
There was a problem hiding this comment.
Add the Windows runtime artifacts back to extraResources.
This filter now ships only Darwin/Linux runtimes, but the same package still builds Windows releases (dist:win) and expects a packaged runtime payload. As written, Windows builds will omit their ADE runtime resources entirely, which breaks local runtime provisioning in the packaged app.
🤖 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/package.json` around lines 243 - 253, The extraResources runtime
filter currently only includes Darwin/Linux artifacts so Windows ADE runtimes
are omitted; update the "filter" array under the extraResources block (the
object with "from": "resources/runtime" and "to": "runtime") to also include the
Windows artifact names (e.g., add ade-win32-x64, ade-win32-x64.native.tar.gz and
ade-win32-ia32, ade-win32-ia32.native.tar.gz or whatever Windows artifact
filenames your build produces) so Windows builds (dist:win) will package the
runtime payload alongside the Darwin/Linux entries.
Comment on lines
1229
to
1233
| export type RequestPrReviewersArgs = { | ||
| prId: string; | ||
| reviewers: string[]; | ||
| reviewers?: string[]; | ||
| teamReviewers?: string[]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep the reviewer-request contract non-empty at the type boundary.
With both fields optional, { prId } now type-checks even though it carries no reviewer target. The renderer currently guards against that, but this shared IPC type no longer encodes the invariant for other callers.
Suggested fix
+type NonEmptyStringArray = [string, ...string[]];
+
-export type RequestPrReviewersArgs = {
- prId: string;
- reviewers?: string[];
- teamReviewers?: string[];
-};
+export type RequestPrReviewersArgs =
+ | {
+ prId: string;
+ reviewers: NonEmptyStringArray;
+ teamReviewers?: string[];
+ }
+ | {
+ prId: string;
+ reviewers?: string[];
+ teamReviewers: NonEmptyStringArray;
+ };Based on learnings: Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes.
📝 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.
Suggested change
| export type RequestPrReviewersArgs = { | |
| prId: string; | |
| reviewers: string[]; | |
| reviewers?: string[]; | |
| teamReviewers?: string[]; | |
| }; | |
| type NonEmptyStringArray = [string, ...string[]]; | |
| export type RequestPrReviewersArgs = | |
| | { | |
| prId: string; | |
| reviewers: NonEmptyStringArray; | |
| teamReviewers?: string[]; | |
| } | |
| | { | |
| prId: string; | |
| reviewers?: string[]; | |
| teamReviewers: NonEmptyStringArray; | |
| }; |
🤖 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/shared/types/prs.ts` around lines 1229 - 1233,
RequestPrReviewersArgs currently allows { prId } with no reviewers; change its
shape to require at least one reviewer target by replacing the single
optional-fields type with a discriminated union such as: a variant with prId +
reviewers: string[] (non-empty), a variant with prId + teamReviewers: string[]
(non-empty), and a variant with both arrays present. Update the
RequestPrReviewersArgs type declaration (refer to the RequestPrReviewersArgs
symbol) to use that union so callers must provide at least one reviewer array,
and ensure any consumer code that constructs or consumes this type (renderer/IPC
handlers) validates arrays are non-empty before sending/acting.
Owner
Author
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade/final-launch-audit-a39de6a8
branch
from
1ce2231 to
605ffeb
Compare
Copilot stopped work on behalf of
arul28 due to an error
June 3, 2026 23:42
…view CI: - build-win: Windows app.asar.unpacked (821MB) exceeded the stale 720MB guard after the branch began bundling the OpenCode runtime (~150MB) into the unpacked payload. Raised validate-win-artifacts unpacked ceiling to 1000MB with a documented rationale (guard still catches runaway bloat). Review (CodeRabbit, PR #527): - githubService.createRepository: route self-owned publishes to /user/repos, only real orgs hit /orgs/{owner}/repos - prService.computeReviewStatus: account for team-only reviewer requests - sshTransport.getSshHostKeyTrust: always scan so a changed key reports "changed" instead of a stale "trusted" short-circuit (+ regression tests) - builtInBrowser reusableOwnedTabForInput: prefer active, else newest owned tab - cli.ts / PrDetailRightMetadataRail: normalize team:org/slug reviewer inputs - notarize-static-runtime: recognize fat64 Mach-O magic; repack archive atomically - iOS SyncService: move token to new key + clear old on profile re-key - test hardening: exact reviewer POST count, scoped dialog assertions, de-flaked terminal cleanup wait Skipped with reasoning: PAT-on-clone (already host-pinned via git extraheader), non-empty reviewer union (would break string[] callers), fork-import gate (intentional, locked by test), Windows runtime extraResources (no win32 runtime). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…h edge case
CI (build-win):
- The afterPack prune-then-ensure flow deliberately re-bundles the on-target
opencode-windows-x64 into app.asar.unpacked so opencode-ai can resolve its
sibling opencode.exe at runtime. validate-win-artifacts still forbade that
package as a "duplicate" (a pre-branch invariant). Flip that one assertion
from must-be-absent to must-be-present; all off-target/baseline/arm64 and
cross-platform opencode payloads remain forbidden. (Unpacked size ceiling
was already raised to 1000MB in iter 1 for the legitimate payload.)
Review (Greptile P1):
- githubService.createRepository: only take the /orgs/{owner}/repos route when
the authenticated login is POSITIVELY resolved and differs from owner. If
validateToken fails transiently (login unknown), fall back to /user/repos so a
personal publish is not mis-routed to the org-only endpoint and hard-fail.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 4, 2026 00:57
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/services/prs/prService.ts (1)
2498-2520:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t block fork imports on missing
headRepo*metadata.Line 2498 still returns
fork_unavailablebefore attempting the fetch, but the fetch below only needs+refs/pull/${args.githubPrNumber}/headfrom the base repo. If GitHub omitshead.repo.owner/name, fork imports that are otherwise fetchable still fail in both preflight and create flows.Suggested fix
}): Promise<CreateLaneFromPrBranchBlock | null> => { - if (!args.headRepoOwner || !args.headRepoName) { - return createLaneFromPrBranchBlock( - "fork_unavailable", - `PR #${args.githubPrNumber} is from a fork, but GitHub did not include a fetchable head repository.`, - ); - } - const headRepoLabel = `${args.headRepoOwner}/${args.headRepoName}`; + const headRepoLabel = `${args.headRepoOwner ?? "unknown"}/${args.headRepoName ?? "unknown"}`; const localRemoteRef = `refs/remotes/${prHeadImportRef(args.githubPrNumber, args.headBranch)}`; const fetch = await runGit([🤖 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/prs/prService.ts` around lines 2498 - 2520, The code currently returns "fork_unavailable" when args.headRepoOwner or args.headRepoName is missing, but the fetch using +refs/pull/${args.githubPrNumber}/head only needs the base repo and can succeed; remove that early return in the block that checks args.headRepoOwner/headRepoName, allow the fetch to proceed, and compute headRepoLabel defensively (e.g. use `${args.headRepoOwner}/${args.headRepoName}` only if both exist, otherwise use a fallback like `unknown-repo` or omit it) so the later error message in the fetch failure path (where createLaneFromPrBranchBlock is called) does not reference missing metadata; keep prHeadImportRef, runGit fetch call, and createLaneFromPrBranchBlock intact and only change the early-return and the construction of the headRepoLabel/failed-message to handle missing head repo metadata gracefully.
🧹 Nitpick comments (2)
apps/ade-cli/src/services/projects/projectScope.test.ts (1)
200-242: 💤 Low valueConsider verifying the runtime creation mock call count.
For consistency with other tests in this file (e.g., line 184), consider asserting that
createAdeRuntimeMockwas called exactly twice to confirm both project scopes were created.✨ Suggested addition
await scopeRegistry.deactivateInactiveSyncHosts(second.projectId); expect(firstSyncService.setHostDiscoveryEnabled).toHaveBeenCalledWith(false); expect(firstSyncService.setHostStartupEnabled).toHaveBeenCalledWith(false); + +expect(createAdeRuntimeMock).toHaveBeenCalledTimes(2); await scopeRegistry.disposeAll();🤖 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/projects/projectScope.test.ts` around lines 200 - 242, Add an assertion that the mocked runtime factory was invoked exactly twice to mirror other tests: after creating the scopeRegistry and switching hosts, assert createAdeRuntimeMock was called 2 times; reference the mock name createAdeRuntimeMock and the test flow around ProjectScopeRegistry.switchSyncHost / deactivateInactiveSyncHosts to locate where to insert the check.apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.ts (1)
348-391: ⚡ Quick winAdd assertion for
firstClient.callto match the pattern in similar tests.The similar test on lines 302-346 verifies both
firstClient.callandsecondClient.call(lines 344-345), but this test only assertssecondClient.call(line 390). For consistency and completeness, add an assertion confirming the first client attemptedprojects.listbefore the timeout occurred.✨ Suggested addition
expect(firstSsh.end).toHaveBeenCalledTimes(1); expect(bootstrapRemoteRuntimeMock).toHaveBeenCalledTimes(2); +expect(firstClient.call).toHaveBeenCalledWith("projects.list", {}); expect(secondClient.call).toHaveBeenCalledWith("projects.list", {});🤖 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/remoteRuntime/remoteConnectionPool.test.ts` around lines 348 - 391, Add an assertion that the mocked first RPC client attempted the idempotent read before timing out: in the "retries idempotent reads once when the RPC client times out" test, after awaiting pool.projectsForTarget(target) and before verifying secondClient.call, assert that firstClient.call was called with "projects.list" and an empty object (e.g., expect(firstClient.call).toHaveBeenCalledWith("projects.list", {})). This uses the existing firstClient mock created at the top of that test to mirror the pattern in the similar test that checks both firstClient.call and secondClient.call.
🤖 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/scripts/package-native-deps.mjs`:
- Around line 81-87: The regex in platformPackageTarget (used to detect
platform-specific packages) omits the win32 variant for the Anthropic Claude
SDK, so `@anthropic-ai/claude-agent-sdk-win32-`* isn't matched; update the regex
inside platformPackageTarget that currently references
/^`@anthropic-ai`\/claude-agent-sdk-(darwin|linux)-(arm64|x64)(?:-musl)?$/ to
include win32 as an allowed platform token (so it matches darwin|linux|win32 and
preserves the existing arch and optional -musl parts), then run tests to ensure
isPackageForOtherTarget() correctly skips Win32 packages.
---
Duplicate comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 2498-2520: The code currently returns "fork_unavailable" when
args.headRepoOwner or args.headRepoName is missing, but the fetch using
+refs/pull/${args.githubPrNumber}/head only needs the base repo and can succeed;
remove that early return in the block that checks
args.headRepoOwner/headRepoName, allow the fetch to proceed, and compute
headRepoLabel defensively (e.g. use `${args.headRepoOwner}/${args.headRepoName}`
only if both exist, otherwise use a fallback like `unknown-repo` or omit it) so
the later error message in the fetch failure path (where
createLaneFromPrBranchBlock is called) does not reference missing metadata; keep
prHeadImportRef, runGit fetch call, and createLaneFromPrBranchBlock intact and
only change the early-return and the construction of the
headRepoLabel/failed-message to handle missing head repo metadata gracefully.
---
Nitpick comments:
In `@apps/ade-cli/src/services/projects/projectScope.test.ts`:
- Around line 200-242: Add an assertion that the mocked runtime factory was
invoked exactly twice to mirror other tests: after creating the scopeRegistry
and switching hosts, assert createAdeRuntimeMock was called 2 times; reference
the mock name createAdeRuntimeMock and the test flow around
ProjectScopeRegistry.switchSyncHost / deactivateInactiveSyncHosts to locate
where to insert the check.
In `@apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.ts`:
- Around line 348-391: Add an assertion that the mocked first RPC client
attempted the idempotent read before timing out: in the "retries idempotent
reads once when the RPC client times out" test, after awaiting
pool.projectsForTarget(target) and before verifying secondClient.call, assert
that firstClient.call was called with "projects.list" and an empty object (e.g.,
expect(firstClient.call).toHaveBeenCalledWith("projects.list", {})). This uses
the existing firstClient mock created at the top of that test to mirror the
pattern in the similar test that checks both firstClient.call and
secondClient.call.
🪄 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: 36a807eb-a444-4a2c-a68c-7adf4aae411e
⛔ Files ignored due to path filters (9)
.ade/.gitignoreis excluded by!.ade/**.ade/cto/identity.yamlis excluded by!.ade/**docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-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/**
📒 Files selected for processing (88)
.gitignoreapps/ade-cli/scripts/build-static.mjsapps/ade-cli/scripts/notarize-static-runtime.mjsapps/ade-cli/scripts/package-native-deps.mjsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/projects/projectScope.test.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.test.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/cli.test.tsxapps/ade-cli/src/tuiClient/cli.tsxapps/desktop/build/entitlements.mac.inherit.plistapps/desktop/build/entitlements.mac.plistapps/desktop/package.jsonapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/notarize-mac-dmg.mjsapps/desktop/scripts/prepare-universal-mac-inputs.mjsapps/desktop/scripts/release-mac-local.mjsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.tsapps/desktop/src/main/services/cto/ctoState.test.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/projects/adeProjectService.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.test.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.tsapps/desktop/src/main/services/remoteRuntime/sshTransport.test.tsapps/desktop/src/main/services/remoteRuntime/sshTransport.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/projects/PublishToGitHubDialog.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailRightRail.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/renderer/components/usage/usage.test.tsxapps/desktop/src/renderer/lib/sessions.test.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/adeLayout.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (5)
- apps/desktop/build/entitlements.mac.inherit.plist
- apps/desktop/src/main/services/projects/adeProjectService.ts
- apps/ade-cli/src/multiProjectRpcServer.ts
- apps/desktop/src/shared/adeLayout.ts
- apps/desktop/build/entitlements.mac.plist
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- apps/desktop/src/main/services/cto/ctoStateService.ts
- apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx
- apps/desktop/resources/agent-skills/ade-browser/SKILL.md
- apps/desktop/src/shared/adeCliGuidance.ts
- apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts
- apps/ade-cli/src/tuiClient/tests/cli.test.tsx
- apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts
🚧 Files skipped from review as they are similar to previous changes (61)
- apps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsx
- apps/desktop/src/main/services/cto/ctoState.test.ts
- apps/desktop/package.json
- apps/ade-cli/src/services/sync/syncService.test.ts
- apps/ade-cli/src/tuiClient/cli.tsx
- apps/desktop/src/renderer/components/app/TopBar.tsx
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.ts
- apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/lib/sessions.ts
- apps/desktop/src/renderer/lib/sessions.test.ts
- apps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.test.ts
- apps/desktop/src/renderer/components/prs/detail/PrDetailPane.test.tsx
- apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx
- apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/main/services/projects/projectLifecycle.test.ts
- apps/ade-cli/src/services/sync/syncHostService.ts
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts
- apps/desktop/src/main/services/adeActions/registry.test.ts
- apps/desktop/src/renderer/components/usage/usage.test.tsx
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.ts
- apps/desktop/scripts/validate-mac-artifacts.mjs
- apps/desktop/src/main/services/remoteRuntime/remoteConnectionService.ts
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/main/services/ipc/runtimeBridge.ts
- apps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.ts
- apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts
- apps/ade-cli/scripts/build-static.mjs
- apps/desktop/scripts/release-mac-local.mjs
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/ade-cli/scripts/notarize-static-runtime.mjs
- apps/desktop/scripts/after-pack-runtime-fixes.cjs
- apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx
- apps/ade-cli/src/services/sync/syncService.ts
- apps/ade-cli/src/cli.test.ts
- apps/desktop/src/main/services/ipc/runtimeBridge.test.ts
- apps/desktop/scripts/prepare-universal-mac-inputs.mjs
- apps/ade-cli/src/services/projects/projectScope.ts
- apps/desktop/src/shared/types/builtInBrowser.ts
- apps/desktop/src/renderer/components/prs/shared/PrDetailRightRail.tsx
- apps/desktop/src/renderer/components/run/RunPage.tsx
- apps/desktop/src/shared/types/prs.ts
- apps/desktop/src/main/services/prs/prService.test.ts
- apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx
- apps/ade-cli/src/headlessLinearServices.ts
- apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx
- apps/ade-cli/src/multiProjectRpcServer.test.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/main/services/github/githubService.ts
- apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts
- apps/desktop/src/main/services/github/githubService.test.ts
- apps/desktop/src/renderer/components/projects/PublishToGitHubDialog.tsx
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/main/services/remoteRuntime/sshTransport.test.ts
- apps/desktop/scripts/notarize-mac-dmg.mjs
- apps/ios/ADETests/ADETests.swift
- apps/ios/ADE/Services/SyncService.swift
- apps/ade-cli/src/cli.ts
… matcher
package-native-deps platformPackageTarget() omitted win32 for the Anthropic
Claude SDK (codex/cursor/esbuild already include win32, opencode includes
windows), so @anthropic-ai/claude-agent-sdk-win32-{arm64,x64} returned null and
isPackageForOtherTarget() failed to skip them — leaking Win32 Claude binaries
into the darwin/linux remote-runtime native bundles. Add win32 to the matcher.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 4, 2026 01:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final launch hardening
Cross-subsystem hardening pass for launch. Major areas:
known_hostsverification (replacing trust-everything) + a TOFU "Trust & connect" flow wired service→IPC→preload→UI; per-call RPC timeout now tears down the connection and the pool reconnects/retries idempotent reads.refs/pull/<n>/head; GitHub team reviewers (team:slug,org/team); org-aware repo creation returning{owner,name,fullName}; publish dialog owner field.wss://route support, broadened LAN/private-route auth handling, db-version advertisement fix.Pre-merge audit (adversarially verified)
A multi-agent audit traced error paths/edge-cases across all 9 subsystems; every finding was independently re-verified before any edit. 8 fixes applied:
package-native-deps.mjsnode-pty prebuildfs.cpfilter dropped the entire target dir → emptyprebuilds/shipped → PTY broken in the remote runtime (reproduced).teamReviewers/ team-only reviewer requests.completeProjectConnectionretires by current active host, not a stale request projectId.apps/desktop/release-logs/; re-indentPublishToGitHubDialogtabs.Two findings were deliberately left alone with reasoning (production IPC timeouts are intentional; the cto/ layout relabel has an
ensureDirside-effect for a cosmetic gain).Validation
Typecheck ×3, lint, desktop 8-shard (~5k tests) + ade-cli (~1k), build ×3, doc validation — all green locally. (
remoteBootstrapshard failures locally are a gitignoredresources/runtime/artifact issue; CI checks out clean.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
A broad launch-hardening pass across 9 subsystems: real OpenSSH
known_hostsverification with a TOFU trust UI, fork-PR import viarefs/pull/N/head, GitHub team reviewers, org-aware repo publishing, background browser tab ownership, macOS packaging fixes, and sync/IPC reliability improvements.sshTransport.tsperforms full HMAC-SHA1 hashed-host lookup and key comparison;getSshHostKeyTrustForTargetalways scans the live key before reporting state, fixing the prior early-return that reported\"trusted\"without verifying the current key.fs.cpfilter that dropped the entireprebuilds/target directory is corrected; new per-target cross-platform filtering is introduced but contains a Windows normalization bug (see inline comment).Confidence Score: 4/5
Safe to merge for macOS/Linux targets; Windows native-package bundling is broken by the new cross-platform filter.
The new isPackageForOtherTarget helper normalizes the package platform to windows but compares it against the raw win32 target platform. For any win32-x64 build all esbuild and SDK win32 packages are misclassified as cross-platform and excluded. All other nine subsystems are well-tested and logically sound.
apps/ade-cli/scripts/package-native-deps.mjs — isPackageForOtherTarget needs the same win32-to-windows normalization applied to the target platform, not only the package platform.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as RemoteTargetList participant Pre as Preload/IPC participant Bridge as runtimeBridge participant Svc as RemoteConnectionService participant SSH as sshTransport UI->>Pre: getSshHostKeyTrust(id) Pre->>Bridge: IPC remoteRuntimeGetSshHostKeyTrust Bridge->>Svc: getSshHostKeyTrust(id) Svc->>SSH: getSshHostKeyTrustForTarget(target) SSH->>SSH: "scanSshHostKeyForTarget (hostVerifier=false, capture key)" SSH->>SSH: knownHostsTrustState(entries, candidates, liveKey) SSH-->>UI: state needs_trust or trusted or changed alt needs_trust UI->>Pre: trustSshHostKey(id, fingerprint) Pre->>Bridge: IPC remoteRuntimeTrustSshHostKey Bridge->>Svc: trustSshHostKey(id, fingerprint) Svc->>SSH: trustSshHostKeyForTarget(target, fp) SSH->>SSH: re-scan + compare fingerprint TOCTOU guard SSH->>SSH: appendFileSync known_hosts SSH-->>UI: trusted true UI->>Pre: "connect(id) skipHostKeyTrustCheck=true" else trusted UI->>Pre: connect(id) endComments Outside Diff (2)
apps/ios/ADE/Services/SyncService.swift, line 6678-6685 (link)latestRemoteDbVersionupdate dropped from send pathThe old code executed
latestRemoteDbVersion = max(latestRemoteDbVersion, currentDbVersion)immediately before constructingPendingOutboundChangeset. The refactor intomakeNextOutboundChangesetsilently removed this update. IflatestRemoteDbVersionfeeds peer metadata (e.g., thedbVersionfield incurrentPeerMetadata()), the remote will receive a stale version number after this change. Verify thatlatestRemoteDbVersionis updated on ACK receipt so the omission here is intentional, or restore the update in the call-site insendNextOutboundChangeset.Prompt To Fix With AI
apps/desktop/src/main/services/ipc/registerIpc.ts, line 8325-8350 (link)ownerblockThe
githubPublishCurrentProjecthandler has lines indented with a leading tab followed by spaces while the surrounding code uses only spaces. The same pattern appears inprService.tsin thePrLabel/PrTeam/PrUserimport block. Both should be normalised to the file's existing space-only style.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: iteration 3 — include win32 in cla..." | Re-trigger Greptile