Multi Linear Launch#446
Merged
Merged
Conversation
…king Pre-merge snapshot of the Linear-integration work: multi-issue launch modal (live-default, per-issue chat/CLI + model + reasoning + permission), session- scoped Linear issue links, agent dashboards, linear.issue_labeled trigger, ade-cli Linear commands, the bundled ade-linear skill, plus tonight's fixes (lane-branch uniquify, generic per-issue kickoff prompt, reasoning resolution, lanes dedupe, search-by-number). Committed to enable merging origin/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Widen the launch mock arg type to include contextAttachments so the per-issue-context assertions typecheck. Post-merge cleanup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract launchAgentChatCli into a shared service so the chat-domain runtime action and the IPC handler both route through it (fixes runtime-backed null in-process services), and route preload launchCli through the daemon when bound. Audit fixes (error/edge paths; happy path unchanged): - BatchLaunchModal: seed per-issue rows from the live Default row on reopen so the Default control can't silently disagree with what launches. - launchAgentChatCli: reject unknown/shell providers at the runtime boundary instead of falling through to a malformed OpenCode launch. - Batch rollback: make remote-branch delete non-fatal so a transient remote failure no longer leaves a half-deleted visible orphan lane. - getDeleteRisk: normalize branch_ref the same way delete() does. - LanesPage: prune completed_with_warnings progress once the lane is gone (was leaking + pinning deletingLaneIds); stop the clean-completion handler from nulling unrelated errors that merely mention "delete". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ixes automate: add agentChatCliLaunch.test.ts (provider-validation guard, input guards, worktree-path fallback, attachedLinearIssueIds filtering). finalize gate fixes (full 8-shard suite + typecheck + lint + build green): - agentChatCliLaunch.test.ts: type the mocks against real signatures instead of `as never` (was a desktop typecheck failure). - linearAuth.test.ts: drop the removed (invalid) `identifier` Linear filter from the expected IssueFilter. - AgentChatComposer.test.tsx / TopBar.test.tsx: follow the intentional issue-row `<button>`→`<div role="button">` change and the unified launch dock flow; branch derivation still asserted via linearIssue.branchName. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds Linear issue session linking across CLI and desktop, updates automation and PR integrations, introduces live status service, extends PTY/session env propagation, and delivers batch lane/agent launch UI with agent dashboards. Includes DB migration, IPC/preload wiring, tests, and docs. ChangesLinear issue linking and batch launch end-to-end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PR Review
Scope: 78 file(s), +8836 / −1272
Verdict: Needs changes
This PR adds multi-issue Linear batch launch (browser multi-select → config modal → bounded parallel lane/agent orchestration), session-scoped Linear issue links mirrored to lanes, CLI/TUI ade linear affordances, and Lanes-tab agent/highlight UX. The orchestration and rollback paths are thoughtful, but a few launch flows can mislead users or collide agents in one worktree.
🐛 Functionality
[High] Parallel batch launch can stack multiple agents in the same existing lane
File: apps/desktop/src/renderer/lib/linearBatchLaunch.ts:L254-L393
Issue: runBatchLaunch runs up to BATCH_LAUNCH_CONCURRENCY workers with no grouping or dedupe by existingLaneId. If several included issues target the same existing lane, multiple chat/CLI agents start concurrently in one worktree.
Repro: In the batch modal, include 3+ issues, set each row to Existing lane and pick the same lane, then Launch. Watch 3 agents attach to one lane at once.
Fix: Before starting the pool, detect duplicate existingLaneId values and either reject with a clear modal error, serialize launches per lane (Map<laneId, queue>), or warn and force lane-only / new-lane for duplicates.
[Medium] Launch button enabled but click does nothing when "Existing lane" has no selection
File: apps/desktop/src/renderer/components/app/BatchLaunchModal.tsx:L406-L431, L728-L730
Issue: launchCount counts included issues only. handleLaunch silently continues rows with laneTarget === "existing" and an empty existingLaneId, then returns when entries is empty—no toast or inline error.
Repro: Include one or more issues, switch Lane to Existing lane, leave the combobox empty, click Launch (button stays enabled).
Fix: Compute launchableCount with the same filters as handleLaunch, disable the primary button when it is 0, and show inline copy like “Select a lane for each existing-lane row.”
[Medium] Batch status shows "Ready" before headless kickoff finishes; kickoff errors are invisible
File: apps/desktop/src/renderer/lib/linearBatchLaunch.ts:L324-L341, apps/desktop/src/main/services/chat/agentChatService.ts:L24857-L24884, apps/desktop/src/renderer/components/app/BatchLaunchStatusToast.tsx:L7-L12
Issue: Chat batch items mark done / Ready as soon as agentChat.launch returns a session id. launchHeadless runs the first turn in the background and only logs agentChat.launchHeadless turn failed on failure—the toast never updates.
Repro: Launch a batch chat item that fails on first turn (e.g. missing/invalid model or runtime error). Toast shows Ready; lane exists but no agent activity and no row-level error.
Fix: Either await kickoff completion (or a session status signal) before marking done, or subscribe to session failure events and patch the batch row to failed with the error message.
🎨 UI/UX
[Low] Retry failed may relaunch with empty model config
File: apps/desktop/src/renderer/components/app/LinearQuickViewButton.tsx:L347-L358
Issue: handleRetryFailed falls back to modelId: "" when batchConfigByIssueRef has no entry. That can skip useful defaults and produce opaque runtime failures instead of the original per-issue settings.
Repro: Dismiss or reset state so the ref loses an issue id, then click Retry failed.
Fix: Persist the last BatchLaunchSubmit[] for the active run (or disable Retry when config is missing) instead of an empty fallback.
Notes
- Strong patterns worth keeping: concurrency cap (
BATCH_LAUNCH_CONCURRENCY), lane rollback after failed agent create, generic shared kickoff prompt with per-session Linear attachments, duplicate-issue guard viafindIssueConflicts, CLI provider validation inagentChatCliLaunch.ts, and Linear branch collision suffixing inlaneService. - No new
package.jsondependencies in this diff. - Could not exercise end-to-end agent kickoff or Linear API calls in this automation environment; findings above are from static path tracing and local diff review.
Sent by Cursor Automation: BUGBOT in Versic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
apps/ade-cli/src/adeRpcServer.test.ts-2903-2913 (1)
2903-2913:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the canonical Linear issue id in this detach contract test.
This block passes
ENG-431throughissueId, but the same fixture modelsidandidentifieras different fields. That makes the contract ambiguous and would still pass if the RPC layer accidentally forwarded identifiers instead of ids. Use the actual issue id here and reserveidentifierfor display data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/adeRpcServer.test.ts` around lines 2903 - 2913, The test currently passes the display identifier "ENG-431" to args.issueId which masks a bug where identifiers could be forwarded instead of canonical ids; update the callTool invocation so args.issueId uses the fixture's Linear issue id (the issue object's id field exposed by the test fixture) instead of the display identifier string, leaving chatSessionId unchanged and keeping the expectation on fixture.runtime.laneService.detachLinearIssueFromSession as-is.apps/desktop/src/renderer/components/app/BatchLaunchStatusToast.tsx-58-67 (1)
58-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpose the toast as a live region.
This status UI mounts and auto-dismisses asynchronously, but nothing marks it as a live region, so screen-reader users won't hear launch progress or completion updates.
As per coding guidelines, `apps/desktop/src/**`: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.💡 Suggested fix
- <div className="fixed bottom-4 right-4 z-[10001] w-[min(340px,calc(100vw-32px))]"> + <div + aria-live="polite" + aria-atomic="true" + className="fixed bottom-4 right-4 z-[10001] w-[min(340px,calc(100vw-32px))]" + >🤖 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/app/BatchLaunchStatusToast.tsx` around lines 58 - 67, BatchLaunchStatusToast mounts an auto-dismissing status UI but isn't exposed as a live region; update the top-level portal container rendered by BatchLaunchStatusToast (the div created by createPortal) to include ARIA live attributes (e.g., aria-live="polite" or "assertive", role="status", and aria-atomic="true") so screen readers announce progress and completion updates; ensure these attributes are set on the outermost div that contains the toast content (the element using LINEAR_BRAND styles) and keep any visual layout unchanged.apps/desktop/src/renderer/components/app/LinearIssueBrowser.tsx-782-793 (1)
782-793:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh cached issue snapshots when an issue reappears.
This effect only keeps the first version of each issue. If the same issue shows up again with a newer title or
updatedAt, off-page launches still use the stale snapshot from the first search.💡 Suggested fix
for (const issue of displayIssues) { - if (!map.has(issue.id)) { + const previous = map.get(issue.id); + if (!previous || issueListKey(previous) !== issueListKey(issue)) { map.set(issue.id, issue); changed = true; } }🤖 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/app/LinearIssueBrowser.tsx` around lines 782 - 793, The seenIssuesRef cache logic only adds new issue ids but never updates existing snapshots; update the effect that iterates displayIssues (using seenIssuesRef.current and setSeenVersion) to check existing entries by issue.id and replace the cached entry when the incoming issue has newer data (e.g., compare issue.updatedAt or title) so the map.set(...) runs for updated items as well and setSeenVersion is triggered when any replacement occurs; keep the same symbols (displayIssues, seenIssuesRef, issue.id, setSeenVersion) and only treat an item as unchanged when the incoming snapshot is identical by your chosen equality (timestamp/title).apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx-213-221 (1)
213-221:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the previous highlight timeout before scheduling a new one.
Each highlight event creates a fresh
setTimeout, but the prior one is never cleared. If a second launch lands within 6 seconds, the first timer will wipe the newer highlight early; it also leaves a pending callback after unmount.♻️ Proposed fix
+ const highlightClearTimerRef = React.useRef<number | null>(null); + React.useEffect(() => { const apply = (highlight: { sessionIds: string[] }) => { if (!highlight.sessionIds.length) return; setHighlightedSessionIds(new Set(highlight.sessionIds)); - window.setTimeout(() => setHighlightedSessionIds(new Set()), 6000); + if (highlightClearTimerRef.current != null) { + window.clearTimeout(highlightClearTimerRef.current); + } + highlightClearTimerRef.current = window.setTimeout(() => { + highlightClearTimerRef.current = null; + setHighlightedSessionIds(new Set()); + }, 6000); }; const pending = consumeLaunchedLanesHighlight(); if (pending) apply(pending); - return subscribeLaunchedLanesHighlight(apply); + const unsubscribe = subscribeLaunchedLanesHighlight(apply); + return () => { + unsubscribe(); + if (highlightClearTimerRef.current != null) { + window.clearTimeout(highlightClearTimerRef.current); + highlightClearTimerRef.current = null; + } + }; }, []);🤖 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/graph/WorkspaceGraphPage.tsx` around lines 213 - 221, The highlight timeout in the useEffect creates a new window.setTimeout each event but never clears previous timers or the pending timer on unmount; update the useEffect to track the timeout id (e.g. let highlightTimeoutId) and before calling window.setTimeout in apply(clear existing with clearTimeout(highlightTimeoutId)), store the new id, and ensure the cleanup returned by subscribeLaunchedLanesHighlight(apply) also clears any remaining timeout (clearTimeout(highlightTimeoutId)) so setHighlightedSessionIds isn't wiped by an earlier timer after a newer highlight or after unmount; keep references to apply, consumeLaunchedLanesHighlight and subscribeLaunchedLanesHighlight when making these changes.apps/desktop/src/renderer/components/app/LinearPaneModal.tsx-114-122 (1)
114-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the icon-only close button.
Line 114-122 uses an icon-only control with
titlebut noaria-label, which can be missed by assistive tech.Proposed fix
<button type="button" + aria-label="Close Linear" className="ade-shell-control inline-flex h-6 w-6 items-center justify-center rounded-md" data-variant="ghost" onClick={onClose} title="Close Linear" >As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
🤖 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/app/LinearPaneModal.tsx` around lines 114 - 122, The close button in LinearPaneModal is icon-only and only uses title, so add an explicit accessible name by adding an aria-label (e.g., aria-label="Close Linear") or aria-labelledby to the button element that uses the X icon; update the button in the LinearPaneModal component (the element rendering X size={12}) to include aria-label="Close Linear" to ensure screen readers can announce the control.apps/desktop/src/main/services/chat/agentChatService.test.ts-1645-1647 (1)
1645-1647:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd cleanup for per-test temp context directories.
contextRootis created for each test but never removed, which can accumulate temp files across long test runs. Add anafterEachin this describe block to remove it.💡 Suggested patch
describe("writeSessionLinearIssueContextFile", () => { function makeSessionLink(overrides: Record<string, unknown> = {}) { @@ let contextRoot: string; beforeEach(() => { contextRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-chat-linear-context-")); }); + afterEach(() => { + fs.rmSync(contextRoot, { recursive: true, force: true }); + });🤖 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 1645 - 1647, The tests create a per-test temp directory in the beforeEach using contextRoot = fs.mkdtempSync(...), but never clean it up; add an afterEach in the same describe block that removes contextRoot (e.g., call fs.rmSync(contextRoot, { recursive: true, force: true }) or fs.rmdirSync(contextRoot, { recursive: true }) depending on Node version) and defensively check that contextRoot is set before removing to avoid errors; place this afterEach next to the existing beforeEach so each test's temp directory is deleted after the test completes.apps/desktop/src/renderer/components/lanes/laneAgents.ts-133-178 (1)
133-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent stale
useLaneAgentsrefreshes from winning the race.A slower refresh for an old
laneIdsset can resolve after a newer one and overwritebyLanewith stale entries. That will briefly show the wrong agent roster after filter/nav changes.Proposed fix
export function useLaneAgents(laneIds: string[]): Map<string, LaneAgent[]> { const [byLane, setByLane] = useState<Map<string, LaneAgent[]>>(new Map()); const laneKey = useMemo(() => [...laneIds].sort().join(","), [laneIds]); const refreshTimerRef = useRef<number | null>(null); + const refreshRequestRef = useRef(0); const refresh = useCallback(async () => { + const requestId = ++refreshRequestRef.current; const ids = laneKey ? laneKey.split(",") : []; if (!ids.length) { setByLane(new Map()); return; } @@ return [laneId, buildLaneAgents(chat, cli)] as const; }), ); + if (refreshRequestRef.current !== requestId) return; setByLane(new Map(entries)); }, [laneKey]);🤖 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/lanes/laneAgents.ts` around lines 133 - 178, The refresh in useLaneAgents can finish out-of-order and overwrite byLane with stale data; fix by adding a monotonic request tracker (e.g., latestRequestRef) used inside refresh: increment latestRequestRef.current at the start of refresh, capture that id in a local variable, perform the async work (inside refresh), and only call setByLane (and clear for empty ids) if the captured id === latestRequestRef.current; keep using refreshTimerRef and existing event unsubs but ensure stale async responses are ignored this way so only the most recent refresh wins.apps/desktop/src/renderer/components/lanes/LanesPage.tsx-2404-2428 (1)
2404-2428:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the highlight timeout to the batch that created it.
The
setTimeout(() => setHighlightedSessionIds(new Set()), 6000)path clears all highlighted sessions. If a second batch launch arrives before the first timeout fires, the older timer will wipe the newer highlight early.Proposed fix
if (!highlight.sessionIds.length) return; setStackGraphHeaderOpen(true); - setHighlightedSessionIds(new Set(highlight.sessionIds)); - window.setTimeout(() => setHighlightedSessionIds(new Set()), 6000); + const batchSessionIds = highlight.sessionIds; + setHighlightedSessionIds((prev) => { + const next = new Set(prev); + for (const sessionId of batchSessionIds) next.add(sessionId); + return next; + }); + window.setTimeout(() => { + setHighlightedSessionIds((prev) => { + const next = new Set(prev); + for (const sessionId of batchSessionIds) next.delete(sessionId); + return next.size === prev.size ? prev : next; + }); + }, 6000);🤖 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/lanes/LanesPage.tsx` around lines 2404 - 2428, The timeout that clears highlights is currently unconditional and may wipe out newer highlights; modify apply so you capture the batch's session IDs into a local const (e.g. const batchSessionIds = new Set(highlight.sessionIds)), then replace the unconditional window.setTimeout(() => setHighlightedSessionIds(new Set()), 6000) with a timeout that only clears when the current highlighted set still matches this batch — e.g. in the timeout call setHighlightedSessionIds(prev => { if (prev.size === batchSessionIds.size && [...batchSessionIds].every(id => prev.has(id))) return new Set(); return prev; }); so older timers won’t clear newer highlights (refer to the apply function and setHighlightedSessionIds usage).apps/desktop/src/renderer/lib/launchedLanesHighlight.ts-131-137 (1)
131-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
subscribeCreatingIssuesdoes not “fire with the full current snapshot” as documented.The function currently only registers the listener and returns unsubscribe; it never invokes
listenerimmediately.Suggested fix
export function subscribeCreatingIssues( listener: (placeholders: CreatingIssuePlaceholder[]) => void, ): () => void { creatingListeners.add(listener); + pruneExpiredCreating(); + listener([...creating.values()]); return () => creatingListeners.delete(listener); }🤖 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/lib/launchedLanesHighlight.ts` around lines 131 - 137, The subscribeCreatingIssues function only registers the listener but doesn't invoke it with the current snapshot as its comment promises; update subscribeCreatingIssues (and use the existing CreatingIssuePlaceholder type) to immediately call listener with the current placeholders snapshot (a shallow copy of the current placeholders array/object) right after adding it to creatingListeners, then return the unsubscribe function as before.
🧹 Nitpick comments (3)
apps/desktop/src/main/main.ts (1)
2445-2456: Fix: CLI fallback key aligns with laneService’s session_id lookup
laneService.listLinearIssuesForSessiontakes the providedchatSessionIdstring and queriessession_linear_issuesbysession_id(it does not distinguish chat vs CLI ids).attachLinearIssueToSessionalso writes the terminal/PTysessionIdinto that samesession_idfield, so thechatSessionId ?? sessionIdfallback ingetSessionLinearEnvshould still populateADE_LINEAR_*for non-chat CLI sessions.Optional: rename the wrapper param/type from
chatSessionIdto a genericsessionIdto avoid the misleading naming.🤖 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/main.ts` around lines 2445 - 2456, getSessionLinearEnv currently accepts a param named chatSessionId which is misleading because laneService.listLinearIssuesForSession and attachLinearIssueToSession both use the same session_id field (including CLI/PTY sessionIds); rename the wrapper param and its type from chatSessionId to a generic sessionId (and update any callers) so the fallback logic (use chatSessionId ?? sessionId) is clearly expressed as a session identifier, keep the existing linkKey computation and the call to laneService.listLinearIssuesForSession({ chatSessionId: linkKey }) (or rename that call arg to match the new param name consistently), and ensure writeSessionLinearIssueContextFile and related uses still receive the same linkKey so ADE_LINEAR_* is populated for non-chat CLI sessions.apps/desktop/src/main/services/adeActions/registry.test.ts (1)
39-42: ⚡ Quick winCover
chat.launchHeadlessin this contract test too.This suite is pinning the newly exposed chat action surface, but
registry.tsalso addslaunchHeadlesstoADE_ACTION_ALLOWLIST. Leaving it out here makes that interface change easy to regress silently.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/adeActions/registry.test.ts` around lines 39 - 42, The test in registry.test.ts misses asserting that the newly-added chat action "launchHeadless" is exposed; update the test that currently checks isAllowedAdeAction("chat", "launchCli") and isCtoOnlyAdeAction("chat", "launchCli") to also assert isAllowedAdeAction("chat", "launchHeadless") === true and isCtoOnlyAdeAction("chat", "launchHeadless") === false so the ADE_ACTION_ALLOWLIST change is covered; locate the test block referencing isAllowedAdeAction and isCtoOnlyAdeAction and add the two corresponding expectations for "chat", "launchHeadless".apps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsx (1)
4-6: ⚡ Quick winAlign
openLinearIssueExternalUrlwith the IPC URL scheme allowlist (http/https/mailto)
window.ade.app.openExternalends up inipcMain.handle(IPC.appOpenExternal, ...), which trims, parses vianew URL(...), and only allowshttp:,https:, andmailto:before callingshell.openExternal, so the renderer’s raw string input isn’t a boundary bypass.Still, mirror the same scheme allowlist in
apps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsxfor defense-in-depth and to keep URL policy consistent at the call site.🤖 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/app/LinearIssueResolveModals.tsx` around lines 4 - 6, In openLinearIssueExternalUrl, enforce the same URL scheme allowlist (http, https, mailto) before calling window.ade.app.openExternal: trim the input, guard null/empty, attempt to construct new URL(trimmed) in a try/catch, and only call window.ade.app.openExternal(trimmed) if url.protocol is one of "http:", "https:" or "mailto:"; on parse failure or non-allowed protocol, return early. This mirrors the ipcMain IPC.appOpenExternal validation for defense-in-depth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 5317-5319: The code resolves session ids by calling
firstPositional(args) before readSessionId(args), which mistakenly treats
value-carrier flags as positionals; change the resolution order to prefer
explicit flags and parsed session ids first by using sessionId ??
readSessionId(args) ?? firstPositional(args) when constructing targetSession
(and make the identical change in the detach and list branches where
firstPositional is currently called before readSessionId); update the three
sites that build targetSession (the block using requireValue(..., "sessionId")
around targetSession and the corresponding detach/list occurrences) so
readSessionId(args) is evaluated before firstPositional(args).
- Around line 2101-2118: The code incorrectly prefers sessionId whenever
explicitValue is true (causing ENG-431 to be ignored); update the condition that
sets issueId so sessionId is only used when there are no positional args OR when
explicitValue is true and exactly one positional remains for the VALUE. Replace
the current check (sessionId && (positionals.length <= 1 || explicitValue)) with
sessionId && (positionals.length === 0 || (explicitValue && positionals.length
=== 1)) so functions/variables issueId, sessionId, positionals, explicitValue
and asString behave correctly.
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 6223-6249: The code currently substitutes
ACTIVE_SESSION_PLACEHOLDER with the general sessionId (which can be a terminal
session) causing terminal IDs to be used for chat-scoped actions; change both
substitutions to use the active chat identifier (e.g. activeChatId) instead of
sessionId/activeSessionIdRef.current: in the action branch where
actionArgs.chatSessionId === ACTIVE_SESSION_PLACEHOLDER set
actionArgs.chatSessionId = activeChatId (and guard for null the same way), and
in the actionList branch map ACTIVE_SESSION_PLACEHOLDER to activeChatId and
update the null-check/error message to reference the chat id if activeChatId is
missing so chat-scoped /linear commands use the chat session id, not terminal
session ids.
In `@apps/ade-cli/src/tuiClient/linearCommands.ts`:
- Around line 189-221: Add an exclusivity guard to both the attach and detach
branches: after reading laneId and sessionId (via optionString) check if both
are present and fail fast instead of falling through; locate the blocks that
call linkLinearIssues/attachLinearIssueToSession (attach path) and
unlinkLinearIssues/detachLinearIssueFromSession (detach path) and, when laneId
&& sessionId, return an error result (or throw) so the command rejects using
--lane and --session together rather than silently acting on the session (use
the existing action/dispatcher error convention used elsewhere).
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 5077-5089: buildAgentRuntimeEnv currently spreads process.env
which can leak stale ADE_LINEAR_* values; change it so after calling
writeSessionLinearIssueContext(managed.session.id) you remove any inherited
ADE_LINEAR_ISSUE_IDS and ADE_LINEAR_CONTEXT_FILE when linearContext is falsy,
and when linearContext exists set ADE_LINEAR_ISSUE_IDS from the returned
issueIds field (not identifiers) and ADE_LINEAR_CONTEXT_FILE from filePath;
update buildAgentRuntimeEnv to explicitly clear these keys from env before or
when constructing the env object and only populate them with the real values
returned by writeSessionLinearIssueContext.
In `@apps/desktop/src/main/services/cto/linearClient.ts`:
- Around line 621-636: The current logic strips team prefixes (e.g., "ENG-431")
down to a bare number and adds only { number: { eq: parsedNumber } } to
orClauses, causing cross-team hits; update the parsing so numberMatch also
detects a leading team prefix (e.g., capture a prefix before the dash), and when
a prefix exists add a conjunctive clause that restricts the issue's team to that
prefix (for example by adding a team filter alongside the number equality)
instead of only the number; adjust the regex that sets numberMatch and the
subsequent block that pushes to orClauses (referencing numberMatch,
parsedNumber, orClauses, and filter.or) to add the team constraint when a prefix
is present.
In `@apps/desktop/src/main/services/cto/linearLiveStatusService.ts`:
- Around line 94-96: The early return using movedInProgress.has(issue.id)
currently prevents assignee sync and posting the launch comment; change the
logic so deduping with movedInProgress only applies to actual state transitions:
first compute whether this event is a state transition (compare
previousState/currentState in the handler where you detect transitions), perform
assignee sync and post the launch comment unconditionally, and only after
confirming a state transition check movedInProgress.has(issue.id) and add
issue.id to movedInProgress when proceeding with the state-change-specific work;
alternatively, use a separate set (e.g., launchInProgress) if you need dedupe
for launches vs state transitions.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2791-2809: The loop currently deletes session_linear_issues and
then unconditionally deletes the lane mirror row in lane_linear_issue_links;
instead, after removing each session_linear_issues row (inside the same
transaction), query session_linear_issues for remaining rows with the same
project_id, lane_id (use laneId from resolveSessionLaneId or target link), and
issue_id (link.issue.id) and only run the delete on lane_linear_issue_links when
that count is zero; keep using the same db transaction (db.run("begin") / try)
and the same identifiers (projectId, laneId, link.id, link.issue.id) to ensure
you don't remove the lane mirror while other sessions still reference the issue.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 949-952: PR body generation misses session-only issue refs because
applyLinearPrLinkage (used by createFromLane and linkToLane) only uses
lane-level links; merge the session-linked refs there. Update
applyLinearPrLinkage (or the callers createFromLane/linkToLane) to gather and
dedupe both collectLinearPrIssueReferences(args.lane, args.closePrimaryOnMerge)
and collectLinearPrIssueReferencesForLaneSessions(args.lane.id) (using
dedupeLinearPrIssueReferences) and include those combined refs when building the
PR body so session-only issues with includeInPr/closeOnMerge get Refs/Fixes
entries and close on merge.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 3428-3435: The explicit color detection logic in ptyService.ts
currently checks only args.env and laneRuntimeEnv when computing explicitNoColor
and explicitForceColor, but sessionLinearEnv (from getSessionLinearEnv) is later
merged into baseLaunchEnv; update the explicitNoColor and explicitForceColor
checks to also inspect sessionLinearEnv so that keys returned by
getSessionLinearEnv (e.g., NO_COLOR or FORCE_COLOR) are respected; modify the
expressions that set explicitNoColor and explicitForceColor to include
hasEnvKey(sessionLinearEnv, "...") alongside the existing checks for args.env
and laneRuntimeEnv.
- Around line 3390-3412: The code attaches Linear issues via
laneService.attachLinearIssueToSession before spawning the PTY but does not roll
that back if spawn fails; update the PTY creation flow (where the PTY
spawn/launch happens for sessionId) to either defer calling
laneService.attachLinearIssueToSession until after the spawn succeeds or add a
rollback path that calls the corresponding detach API (e.g.,
laneService.detachLinearIssueFromSession or a remove/clear method) when spawn
fails; make sure to reference sessionId and args.linearIssues, wrap
attach/detach in try/catch, and apply the same fix at the other occurrence noted
around the 3570-3579 block so session_linear_issues are not left persisted for
failed launches.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 1256-1271: Add a nullable foreign key constraint on
session_linear_issues.lane_id so deleted lanes are cleaned up: update the
session_linear_issues table definition (table name session_linear_issues, column
lane_id) to include "foreign key(lane_id) references lanes(id) on delete set
null" (or equivalent nullable-FK behavior) alongside the existing foreign key on
project_id so lane deletions null out lane_id rather than leaving stale values.
In `@apps/desktop/src/renderer/components/app/BatchLaunchModal.tsx`:
- Around line 67-70: The PermissionPicker UI can show a different visible option
than the stored permissionMode when switching models; update the code to
normalize permissionMode whenever modelId (or the options returned by
launchPermissionOptions) changes by computing the available options (options =
launchPermissionOptions(modelId)), determining the active option (active =
options.find(o => o.value === permissionMode) ?? options[0]), and if
active.value !== permissionMode call the state updater (e.g.,
setPermissionMode(active.value)) before using permissionMode in handleLaunch or
submitting; apply the same normalization pattern around the PermissionPicker
usage and any other places that read permissionMode (handleLaunch, places
referenced by symbols PermissionPicker, handleLaunch, permissionMode,
setPermissionMode, launchPermissionOptions, active) so the submitted value
always matches the visible choice.
In `@apps/desktop/src/renderer/components/app/LinearIssueBrowser.tsx`:
- Around line 275-302: safeSaveSelection only writes ids, so safeLoadSelection
cannot reconstruct selections for items not on the current page and
resolvedSelectedIssues loses them; update persistence to store a minimal
snapshot per id (e.g., id plus title/summary/timestamp) when saving in
safeSaveSelection and update safeLoadSelection to parse and return both ids and
these minimal issue records to repopulate seenIssuesRef on mount (or
alternatively hydrate seenIssuesRef from storage before enabling launch
actions); ensure selectionStorageKey is used consistently and that
resolvedSelectedIssues and any launch-button enabling logic check the hydrated
seenIssuesRef so batch launches include restored off-page items.
In `@apps/desktop/src/renderer/lib/launchedLanesHighlight.ts`:
- Around line 87-97: Expired "creating" placeholders are only pruned in
consumeCreatingIssues(), so live subscribers may see stale items; update
emitCreating() to call pruneExpiredCreating() before building the snapshot (use
the existing pruneExpiredCreating function and the PENDING_TTL_MS constant and
creating map) so listeners always receive a TTL-enforced view, and apply the
same pattern to the pending path (add a pruneExpiredPending() helper and call it
from the corresponding emitPending()/pending snapshot emitter that uses
pendingListeners/pending collection).
---
Minor comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 2903-2913: The test currently passes the display identifier
"ENG-431" to args.issueId which masks a bug where identifiers could be forwarded
instead of canonical ids; update the callTool invocation so args.issueId uses
the fixture's Linear issue id (the issue object's id field exposed by the test
fixture) instead of the display identifier string, leaving chatSessionId
unchanged and keeping the expectation on
fixture.runtime.laneService.detachLinearIssueFromSession as-is.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 1645-1647: The tests create a per-test temp directory in the
beforeEach using contextRoot = fs.mkdtempSync(...), but never clean it up; add
an afterEach in the same describe block that removes contextRoot (e.g., call
fs.rmSync(contextRoot, { recursive: true, force: true }) or
fs.rmdirSync(contextRoot, { recursive: true }) depending on Node version) and
defensively check that contextRoot is set before removing to avoid errors; place
this afterEach next to the existing beforeEach so each test's temp directory is
deleted after the test completes.
In `@apps/desktop/src/renderer/components/app/BatchLaunchStatusToast.tsx`:
- Around line 58-67: BatchLaunchStatusToast mounts an auto-dismissing status UI
but isn't exposed as a live region; update the top-level portal container
rendered by BatchLaunchStatusToast (the div created by createPortal) to include
ARIA live attributes (e.g., aria-live="polite" or "assertive", role="status",
and aria-atomic="true") so screen readers announce progress and completion
updates; ensure these attributes are set on the outermost div that contains the
toast content (the element using LINEAR_BRAND styles) and keep any visual layout
unchanged.
In `@apps/desktop/src/renderer/components/app/LinearIssueBrowser.tsx`:
- Around line 782-793: The seenIssuesRef cache logic only adds new issue ids but
never updates existing snapshots; update the effect that iterates displayIssues
(using seenIssuesRef.current and setSeenVersion) to check existing entries by
issue.id and replace the cached entry when the incoming issue has newer data
(e.g., compare issue.updatedAt or title) so the map.set(...) runs for updated
items as well and setSeenVersion is triggered when any replacement occurs; keep
the same symbols (displayIssues, seenIssuesRef, issue.id, setSeenVersion) and
only treat an item as unchanged when the incoming snapshot is identical by your
chosen equality (timestamp/title).
In `@apps/desktop/src/renderer/components/app/LinearPaneModal.tsx`:
- Around line 114-122: The close button in LinearPaneModal is icon-only and only
uses title, so add an explicit accessible name by adding an aria-label (e.g.,
aria-label="Close Linear") or aria-labelledby to the button element that uses
the X icon; update the button in the LinearPaneModal component (the element
rendering X size={12}) to include aria-label="Close Linear" to ensure screen
readers can announce the control.
In `@apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx`:
- Around line 213-221: The highlight timeout in the useEffect creates a new
window.setTimeout each event but never clears previous timers or the pending
timer on unmount; update the useEffect to track the timeout id (e.g. let
highlightTimeoutId) and before calling window.setTimeout in apply(clear existing
with clearTimeout(highlightTimeoutId)), store the new id, and ensure the cleanup
returned by subscribeLaunchedLanesHighlight(apply) also clears any remaining
timeout (clearTimeout(highlightTimeoutId)) so setHighlightedSessionIds isn't
wiped by an earlier timer after a newer highlight or after unmount; keep
references to apply, consumeLaunchedLanesHighlight and
subscribeLaunchedLanesHighlight when making these changes.
In `@apps/desktop/src/renderer/components/lanes/laneAgents.ts`:
- Around line 133-178: The refresh in useLaneAgents can finish out-of-order and
overwrite byLane with stale data; fix by adding a monotonic request tracker
(e.g., latestRequestRef) used inside refresh: increment latestRequestRef.current
at the start of refresh, capture that id in a local variable, perform the async
work (inside refresh), and only call setByLane (and clear for empty ids) if the
captured id === latestRequestRef.current; keep using refreshTimerRef and
existing event unsubs but ensure stale async responses are ignored this way so
only the most recent refresh wins.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 2404-2428: The timeout that clears highlights is currently
unconditional and may wipe out newer highlights; modify apply so you capture the
batch's session IDs into a local const (e.g. const batchSessionIds = new
Set(highlight.sessionIds)), then replace the unconditional window.setTimeout(()
=> setHighlightedSessionIds(new Set()), 6000) with a timeout that only clears
when the current highlighted set still matches this batch — e.g. in the timeout
call setHighlightedSessionIds(prev => { if (prev.size === batchSessionIds.size
&& [...batchSessionIds].every(id => prev.has(id))) return new Set(); return
prev; }); so older timers won’t clear newer highlights (refer to the apply
function and setHighlightedSessionIds usage).
In `@apps/desktop/src/renderer/lib/launchedLanesHighlight.ts`:
- Around line 131-137: The subscribeCreatingIssues function only registers the
listener but doesn't invoke it with the current snapshot as its comment
promises; update subscribeCreatingIssues (and use the existing
CreatingIssuePlaceholder type) to immediately call listener with the current
placeholders snapshot (a shallow copy of the current placeholders array/object)
right after adding it to creatingListeners, then return the unsubscribe function
as before.
---
Nitpick comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 2445-2456: getSessionLinearEnv currently accepts a param named
chatSessionId which is misleading because laneService.listLinearIssuesForSession
and attachLinearIssueToSession both use the same session_id field (including
CLI/PTY sessionIds); rename the wrapper param and its type from chatSessionId to
a generic sessionId (and update any callers) so the fallback logic (use
chatSessionId ?? sessionId) is clearly expressed as a session identifier, keep
the existing linkKey computation and the call to
laneService.listLinearIssuesForSession({ chatSessionId: linkKey }) (or rename
that call arg to match the new param name consistently), and ensure
writeSessionLinearIssueContextFile and related uses still receive the same
linkKey so ADE_LINEAR_* is populated for non-chat CLI sessions.
In `@apps/desktop/src/main/services/adeActions/registry.test.ts`:
- Around line 39-42: The test in registry.test.ts misses asserting that the
newly-added chat action "launchHeadless" is exposed; update the test that
currently checks isAllowedAdeAction("chat", "launchCli") and
isCtoOnlyAdeAction("chat", "launchCli") to also assert
isAllowedAdeAction("chat", "launchHeadless") === true and
isCtoOnlyAdeAction("chat", "launchHeadless") === false so the
ADE_ACTION_ALLOWLIST change is covered; locate the test block referencing
isAllowedAdeAction and isCtoOnlyAdeAction and add the two corresponding
expectations for "chat", "launchHeadless".
In `@apps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsx`:
- Around line 4-6: In openLinearIssueExternalUrl, enforce the same URL scheme
allowlist (http, https, mailto) before calling window.ade.app.openExternal: trim
the input, guard null/empty, attempt to construct new URL(trimmed) in a
try/catch, and only call window.ade.app.openExternal(trimmed) if url.protocol is
one of "http:", "https:" or "mailto:"; on parse failure or non-allowed protocol,
return early. This mirrors the ipcMain IPC.appOpenExternal validation for
defense-in-depth.
🪄 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: d8a2908c-53a3-43af-b90c-72efe38f113d
⛔ Files ignored due to path filters (9)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/automations/triggers-and-actions.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/cto/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/workspace-graph/README.mdis excluded by!docs/**
📒 Files selected for processing (69)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/linearCommands.tsapps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/resources/agent-skills/ade-linear/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/automations/automationService.tsapps/desktop/src/main/services/automations/linearAutomationDispatch.tsapps/desktop/src/main/services/chat/agentChatCliLaunch.test.tsapps/desktop/src/main/services/chat/agentChatCliLaunch.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/cto/issueTracker.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearClient.tsapps/desktop/src/main/services/cto/linearIssueTracker.tsapps/desktop/src/main/services/cto/linearLiveStatusService.test.tsapps/desktop/src/main/services/cto/linearLiveStatusService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/BatchLaunchModal.tsxapps/desktop/src/renderer/components/app/BatchLaunchStatusToast.tsxapps/desktop/src/renderer/components/app/LinearIssueBrowser.tsxapps/desktop/src/renderer/components/app/LinearIssueResolveModals.tsxapps/desktop/src/renderer/components/app/LinearPaneModal.tsxapps/desktop/src/renderer/components/app/LinearQuickViewButton.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/automations/LinearTriggerFilters.tsxapps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsxapps/desktop/src/renderer/components/graph/graphTypes.tsapps/desktop/src/renderer/components/lanes/LaneAgentList.tsxapps/desktop/src/renderer/components/lanes/LaneStackPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/laneAgents.test.tsapps/desktop/src/renderer/components/lanes/laneAgents.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/laneNavigation.tsapps/desktop/src/renderer/lib/launchedLanesHighlight.tsapps/desktop/src/renderer/lib/linearBatchLaunch.test.tsapps/desktop/src/renderer/lib/linearBatchLaunch.tsapps/desktop/src/renderer/lib/linearIssueWorkNavigation.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/sessions.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/lib/linearIssueWorkNavigation.ts
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
Comment on lines
+2101
to
+2118
| if (!issueId) { | ||
| // With no explicit id: if the session injected one and exactly one positional | ||
| // remains for the value, treat that positional as the value. Otherwise the | ||
| // first positional is the issue id. | ||
| if (sessionId && (positionals.length <= 1 || explicitValue)) { | ||
| issueId = sessionId; | ||
| } else { | ||
| issueId = asString(positionals.shift() ?? null); | ||
| } | ||
| } | ||
| if (!issueId) { | ||
| throw new CliUsageError( | ||
| "Linear issue id is required. Pass --issue-id <id> or run inside a session with an attached issue (ADE sets $ADE_LINEAR_ISSUE_IDS).", | ||
| ); | ||
| } | ||
| if (!value) { | ||
| value = positionals.length ? positionals.join(" ").trim() : null; | ||
| } |
There was a problem hiding this comment.
Don't fall back to the session issue when a positional issue id is still present.
Line 2105 currently prefers $ADE_LINEAR_ISSUE_IDS whenever an explicit value flag was used, so ade linear comment ENG-431 --body "done" inside an attached session will write to the session's default issue instead of ENG-431. The same bug affects set-state, assign, and label.
Suggested fix
if (!issueId) {
// With no explicit id: if the session injected one and exactly one positional
// remains for the value, treat that positional as the value. Otherwise the
// first positional is the issue id.
- if (sessionId && (positionals.length <= 1 || explicitValue)) {
+ if (sessionId && positionals.length === 0) {
+ issueId = sessionId;
+ } else if (sessionId && positionals.length === 1 && !explicitValue) {
issueId = sessionId;
} else {
issueId = asString(positionals.shift() ?? null);
}
}🤖 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 2101 - 2118, The code incorrectly
prefers sessionId whenever explicitValue is true (causing ENG-431 to be
ignored); update the condition that sets issueId so sessionId is only used when
there are no positional args OR when explicitValue is true and exactly one
positional remains for the VALUE. Replace the current check (sessionId &&
(positionals.length <= 1 || explicitValue)) with sessionId &&
(positionals.length === 0 || (explicitValue && positionals.length === 1)) so
functions/variables issueId, sessionId, positionals, explicitValue and asString
behave correctly.
Comment on lines
+5317
to
+5319
| const targetSession = requireValue( | ||
| sessionId ?? firstPositional(args) ?? readSessionId(args), | ||
| "sessionId", |
There was a problem hiding this comment.
Resolve session ids from flags before scanning raw positionals.
Lines 5318, 5348, and 5372 call firstPositional(args) before readSessionId(args). firstPositional does not skip value-carrier flags, so commands like ade chat attach-linear-issue --source manual --chat-session s1 --issue-id ENG-431 can read manual as the session id and target the wrong session or fail.
Suggested fix
const targetSession = requireValue(
- sessionId ?? firstPositional(args) ?? readSessionId(args),
+ sessionId ?? readSessionId(args) ?? firstStandalonePositional(args),
"sessionId",
);Apply the same ordering in the detach and list branches as well.
Also applies to: 5347-5349, 5371-5373
🤖 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 5317 - 5319, The code resolves session
ids by calling firstPositional(args) before readSessionId(args), which
mistakenly treats value-carrier flags as positionals; change the resolution
order to prefer explicit flags and parsed session ids first by using sessionId
?? readSessionId(args) ?? firstPositional(args) when constructing targetSession
(and make the identical change in the detach and list branches where
firstPositional is currently called before readSessionId); update the three
sites that build targetSession (the block using requireValue(..., "sessionId")
around targetSession and the corresponding detach/list occurrences) so
readSessionId(args) is evaluated before firstPositional(args).
Comment on lines
+6223
to
+6249
| if (request.kind === "action") { | ||
| // Session-scoped attach/detach/list default to the active chat session: | ||
| // the ACTIVE_SESSION_PLACEHOLDER sentinel on chatSessionId is substituted | ||
| // with the live id. | ||
| const actionArgs = { ...request.args }; | ||
| if (actionArgs.chatSessionId === ACTIVE_SESSION_PLACEHOLDER) { | ||
| if (!sessionId) { | ||
| setRightPane({ kind: "details", title: request.title, body: "No active chat session. Pass --session <id>." }); | ||
| return; | ||
| } | ||
| actionArgs.chatSessionId = sessionId; | ||
| } | ||
| const result = await conn.action(request.domain, request.action, actionArgs); | ||
| setRightPane({ kind: "details", title: request.title, body: renderObject(result, 24) }); | ||
| return; | ||
| } | ||
| if (request.kind === "actionList") { | ||
| const argsList = request.argsList.map((entry) => | ||
| entry === ACTIVE_SESSION_PLACEHOLDER ? sessionId : entry, | ||
| ); | ||
| if (argsList.some((entry) => entry == null)) { | ||
| setRightPane({ kind: "details", title: request.title, body: "No active chat session. Pass --session <id>." }); | ||
| return; | ||
| } | ||
| const result = await conn.actionList(request.domain, request.action, argsList); | ||
| setRightPane({ kind: "details", title: request.title, body: renderObject(result, 24) }); | ||
| return; |
There was a problem hiding this comment.
Use the active chat id here, not the generic active session id.
Line 6228 and Line 6241 can substitute a terminal terminalId into chatSessionId, because activeSessionIdRef.current also tracks terminal sessions in this component. That breaks session-scoped /linear commands whenever the focused item is a terminal instead of an agent chat.
Suggested fix
- const sessionId = activeSessionIdRef.current;
+ const sessionId = activeSessionIdRef.current;
+ const activeChatSessionId = activeSession?.sessionId ?? null;
@@
if (request.kind === "action") {
@@
const actionArgs = { ...request.args };
if (actionArgs.chatSessionId === ACTIVE_SESSION_PLACEHOLDER) {
- if (!sessionId) {
+ if (!activeChatSessionId) {
setRightPane({ kind: "details", title: request.title, body: "No active chat session. Pass --session <id>." });
return;
}
- actionArgs.chatSessionId = sessionId;
+ actionArgs.chatSessionId = activeChatSessionId;
}
@@
if (request.kind === "actionList") {
const argsList = request.argsList.map((entry) =>
- entry === ACTIVE_SESSION_PLACEHOLDER ? sessionId : entry,
+ entry === ACTIVE_SESSION_PLACEHOLDER ? activeChatSessionId : entry,
);🤖 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/app.tsx` around lines 6223 - 6249, The code
currently substitutes ACTIVE_SESSION_PLACEHOLDER with the general sessionId
(which can be a terminal session) causing terminal IDs to be used for
chat-scoped actions; change both substitutions to use the active chat identifier
(e.g. activeChatId) instead of sessionId/activeSessionIdRef.current: in the
action branch where actionArgs.chatSessionId === ACTIVE_SESSION_PLACEHOLDER set
actionArgs.chatSessionId = activeChatId (and guard for null the same way), and
in the actionList branch map ACTIVE_SESSION_PLACEHOLDER to activeChatId and
update the null-check/error message to reference the chat id if activeChatId is
missing so chat-scoped /linear commands use the chat session id, not terminal
session ids.
Comment on lines
+189
to
+221
| const laneId = optionString(options, "laneId", "lane"); | ||
| const sessionId = optionString(options, "session", "sessionId", "chatSession", "chatSessionId"); | ||
| if (laneId && !sessionId) { | ||
| return action("Linear attach (lane)", "lane", "linkLinearIssues", { | ||
| laneId, | ||
| issues, | ||
| ...attachmentFlags(options), | ||
| }); | ||
| } | ||
| // attachLinearIssueToSession takes an issues array keyed by chatSessionId. A | ||
| // missing chatSessionId is filled with the active session by the dispatcher. | ||
| return action("Linear attach", "lane", "attachLinearIssueToSession", { | ||
| chatSessionId: sessionId ?? ACTIVE_SESSION_PLACEHOLDER, | ||
| issues, | ||
| ...attachmentFlags(options), | ||
| }); | ||
| } | ||
|
|
||
| if (group === "detach" || group === "detach-issue" || group === "detach-linear-issue") { | ||
| const laneId = optionString(options, "laneId", "lane"); | ||
| const sessionId = optionString(options, "session", "sessionId", "chatSession", "chatSessionId"); | ||
| // Omitting an issue id detaches all (non-primary for a lane; every issue for a session). | ||
| const issueId = optionString(options, "issueId", "issue", "linearIssueId") ?? modeArg ?? rest[0] ?? null; | ||
| if (laneId && !sessionId) { | ||
| return action("Linear detach (lane)", "lane", "unlinkLinearIssues", { | ||
| laneId, | ||
| issueId: issueId ?? undefined, | ||
| }); | ||
| } | ||
| return action("Linear detach", "lane", "detachLinearIssueFromSession", { | ||
| chatSessionId: sessionId ?? ACTIVE_SESSION_PLACEHOLDER, | ||
| issueId: issueId ?? undefined, | ||
| }); |
There was a problem hiding this comment.
Reject --lane and --session together.
If both flags are present, these branches silently fall through to the session path and ignore laneId, so /linear attach and /linear detach can mutate the wrong target instead of failing fast.
💡 Suggested guard
const laneId = optionString(options, "laneId", "lane");
const sessionId = optionString(options, "session", "sessionId", "chatSession", "chatSessionId");
+if (laneId && sessionId) {
+ return usage("Linear attach", "Usage: /linear attach --issue-id <id> [--session <id> | --lane <id>]");
+}Apply the same exclusivity check in the detach block as well.
🤖 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/linearCommands.ts` around lines 189 - 221, Add an
exclusivity guard to both the attach and detach branches: after reading laneId
and sessionId (via optionString) check if both are present and fail fast instead
of falling through; locate the blocks that call
linkLinearIssues/attachLinearIssueToSession (attach path) and
unlinkLinearIssues/detachLinearIssueFromSession (detach path) and, when laneId
&& sessionId, return an error result (or throw) so the command rejects using
--lane and --session together rather than silently acting on the session (use
the existing action/dispatcher error convention used elsewhere).
Comment on lines
+5077
to
+5089
| const buildAgentRuntimeEnv = (managed: ManagedChatSession): NodeJS.ProcessEnv => { | ||
| const env: NodeJS.ProcessEnv = { | ||
| ...(getAdeCliAgentEnv?.(process.env) ?? process.env), | ||
| ADE_CHAT_SESSION_ID: managed.session.id, | ||
| ADE_LANE_ID: managed.session.laneId, | ||
| ADE_PROJECT_ROOT: projectRoot, | ||
| ADE_WORKSPACE_ROOT: managed.laneWorktreePath, | ||
| }; | ||
| const linearContext = writeSessionLinearIssueContext(managed.session.id); | ||
| if (linearContext) { | ||
| env.ADE_LINEAR_ISSUE_IDS = linearContext.identifiers; | ||
| env.ADE_LINEAR_CONTEXT_FILE = linearContext.filePath; | ||
| } |
There was a problem hiding this comment.
Clear inherited Linear env vars and use real issue IDs for ADE_LINEAR_ISSUE_IDS.
This env starts from process.env, so sessions without attached issues can still inherit stale ADE_LINEAR_* values from the parent process. On the linked path, ADE_LINEAR_ISSUE_IDS is also populated with identifiers even though writeSessionLinearIssueContextFile already returns issueIds. That can leak the wrong Linear context into unrelated agent launches.
🛠️ Proposed fix
const env: NodeJS.ProcessEnv = {
...(getAdeCliAgentEnv?.(process.env) ?? process.env),
ADE_CHAT_SESSION_ID: managed.session.id,
ADE_LANE_ID: managed.session.laneId,
ADE_PROJECT_ROOT: projectRoot,
ADE_WORKSPACE_ROOT: managed.laneWorktreePath,
};
+ delete env.ADE_LINEAR_ISSUE_IDS;
+ delete env.ADE_LINEAR_CONTEXT_FILE;
const linearContext = writeSessionLinearIssueContext(managed.session.id);
if (linearContext) {
- env.ADE_LINEAR_ISSUE_IDS = linearContext.identifiers;
+ env.ADE_LINEAR_ISSUE_IDS = linearContext.issueIds;
env.ADE_LINEAR_CONTEXT_FILE = linearContext.filePath;
}As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
📝 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
| const buildAgentRuntimeEnv = (managed: ManagedChatSession): NodeJS.ProcessEnv => { | |
| const env: NodeJS.ProcessEnv = { | |
| ...(getAdeCliAgentEnv?.(process.env) ?? process.env), | |
| ADE_CHAT_SESSION_ID: managed.session.id, | |
| ADE_LANE_ID: managed.session.laneId, | |
| ADE_PROJECT_ROOT: projectRoot, | |
| ADE_WORKSPACE_ROOT: managed.laneWorktreePath, | |
| }; | |
| const linearContext = writeSessionLinearIssueContext(managed.session.id); | |
| if (linearContext) { | |
| env.ADE_LINEAR_ISSUE_IDS = linearContext.identifiers; | |
| env.ADE_LINEAR_CONTEXT_FILE = linearContext.filePath; | |
| } | |
| const buildAgentRuntimeEnv = (managed: ManagedChatSession): NodeJS.ProcessEnv => { | |
| const env: NodeJS.ProcessEnv = { | |
| ...(getAdeCliAgentEnv?.(process.env) ?? process.env), | |
| ADE_CHAT_SESSION_ID: managed.session.id, | |
| ADE_LANE_ID: managed.session.laneId, | |
| ADE_PROJECT_ROOT: projectRoot, | |
| ADE_WORKSPACE_ROOT: managed.laneWorktreePath, | |
| }; | |
| delete env.ADE_LINEAR_ISSUE_IDS; | |
| delete env.ADE_LINEAR_CONTEXT_FILE; | |
| const linearContext = writeSessionLinearIssueContext(managed.session.id); | |
| if (linearContext) { | |
| env.ADE_LINEAR_ISSUE_IDS = linearContext.issueIds; | |
| env.ADE_LINEAR_CONTEXT_FILE = linearContext.filePath; | |
| } |
🤖 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 5077 -
5089, buildAgentRuntimeEnv currently spreads process.env which can leak stale
ADE_LINEAR_* values; change it so after calling
writeSessionLinearIssueContext(managed.session.id) you remove any inherited
ADE_LINEAR_ISSUE_IDS and ADE_LINEAR_CONTEXT_FILE when linearContext is falsy,
and when linearContext exists set ADE_LINEAR_ISSUE_IDS from the returned
issueIds field (not identifiers) and ADE_LINEAR_CONTEXT_FILE from filePath;
update buildAgentRuntimeEnv to explicitly clear these keys from env before or
when constructing the env object and only populate them with the real values
returned by writeSessionLinearIssueContext.
Comment on lines
+3428
to
3435
| const sessionLinearEnv = getSessionLinearEnv?.({ sessionId, chatSessionId }) ?? {}; | ||
| const explicitNoColor = hasEnvKey(args.env ?? {}, "NO_COLOR") || hasEnvKey(laneRuntimeEnv, "NO_COLOR"); | ||
| const explicitForceColor = hasEnvKey(args.env ?? {}, "FORCE_COLOR") || hasEnvKey(laneRuntimeEnv, "FORCE_COLOR"); | ||
| const baseLaunchEnv = { | ||
| ...process.env, | ||
| ...laneRuntimeEnv, | ||
| ...sessionLinearEnv, | ||
| ...(args.env ?? {}) |
There was a problem hiding this comment.
Include sessionLinearEnv in the explicit color-flag checks.
baseLaunchEnv merges sessionLinearEnv, but explicitNoColor / explicitForceColor only inspect args.env and laneRuntimeEnv. If getSessionLinearEnv() returns NO_COLOR, withInteractiveTerminalColorEnv() will strip it and force color back on.
🤖 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/pty/ptyService.ts` around lines 3428 - 3435,
The explicit color detection logic in ptyService.ts currently checks only
args.env and laneRuntimeEnv when computing explicitNoColor and
explicitForceColor, but sessionLinearEnv (from getSessionLinearEnv) is later
merged into baseLaunchEnv; update the explicitNoColor and explicitForceColor
checks to also inspect sessionLinearEnv so that keys returned by
getSessionLinearEnv (e.g., NO_COLOR or FORCE_COLOR) are respected; modify the
expressions that set explicitNoColor and explicitForceColor to include
hasEnvKey(sessionLinearEnv, "...") alongside the existing checks for args.env
and laneRuntimeEnv.
Comment on lines
+1256
to
+1271
| create table if not exists session_linear_issues ( | ||
| id text primary key, | ||
| project_id text not null, | ||
| session_id text not null, | ||
| lane_id text, | ||
| issue_id text not null, | ||
| issue_json text not null, | ||
| role text not null, | ||
| source text not null, | ||
| include_in_pr integer not null default 1, | ||
| close_on_merge integer not null default 0, | ||
| evidence_json text, | ||
| created_at text not null, | ||
| updated_at text not null, | ||
| foreign key(project_id) references projects(id) on delete cascade | ||
| ) |
There was a problem hiding this comment.
Add the missing FK on lane_id.
lane_id is stored specifically so session-linked issues can fan out back to a lane, but this table does not constrain it to lanes(id). That means lane cleanup can leave stale session_linear_issues.lane_id values behind, and later conflict/PR-linking reads can still treat a deleted lane as attached. A nullable FK works fine for lane-less sessions, so this should match the lane-scoped tables here.
Suggested schema change
db.run(`
create table if not exists session_linear_issues (
id text primary key,
project_id text not null,
session_id text not null,
lane_id text,
issue_id text not null,
issue_json text not null,
role text not null,
source text not null,
include_in_pr integer not null default 1,
close_on_merge integer not null default 0,
evidence_json text,
created_at text not null,
updated_at text not null,
- foreign key(project_id) references projects(id) on delete cascade
+ foreign key(project_id) references projects(id) on delete cascade,
+ foreign key(lane_id) references lanes(id) on delete cascade
)
`);📝 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
| create table if not exists session_linear_issues ( | |
| id text primary key, | |
| project_id text not null, | |
| session_id text not null, | |
| lane_id text, | |
| issue_id text not null, | |
| issue_json text not null, | |
| role text not null, | |
| source text not null, | |
| include_in_pr integer not null default 1, | |
| close_on_merge integer not null default 0, | |
| evidence_json text, | |
| created_at text not null, | |
| updated_at text not null, | |
| foreign key(project_id) references projects(id) on delete cascade | |
| ) | |
| create table if not exists session_linear_issues ( | |
| id text primary key, | |
| project_id text not null, | |
| session_id text not null, | |
| lane_id text, | |
| issue_id text not null, | |
| issue_json text not null, | |
| role text not null, | |
| source text not null, | |
| include_in_pr integer not null default 1, | |
| close_on_merge integer not null default 0, | |
| evidence_json text, | |
| created_at text not null, | |
| updated_at text not null, | |
| foreign key(project_id) references projects(id) on delete cascade, | |
| foreign key(lane_id) references lanes(id) on delete cascade | |
| ) |
🤖 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/state/kvDb.ts` around lines 1256 - 1271, Add a
nullable foreign key constraint on session_linear_issues.lane_id so deleted
lanes are cleaned up: update the session_linear_issues table definition (table
name session_linear_issues, column lane_id) to include "foreign key(lane_id)
references lanes(id) on delete set null" (or equivalent nullable-FK behavior)
alongside the existing foreign key on project_id so lane deletions null out
lane_id rather than leaving stale values.
Comment on lines
+67
to
+70
| const options = launchPermissionOptions(modelId); | ||
| if (!options.length) return null; | ||
| const active = (value && options.find((o) => o.value === value)) ?? options[0]!; | ||
| const activeColors = safetyColors(active.safety); |
There was a problem hiding this comment.
Normalize permissionMode when the model changes.
PermissionPicker falls back to the first visible option when the stored mode is no longer valid for the selected model, but handleLaunch() still submits the stale permissionMode. Switching from a model that allows "edit" to a Codex model will therefore show “Default” in the UI while launching with the old hidden mode underneath.
🔧 Suggested fix
+function normalizePermissionMode(
+ modelId: string,
+ value: AgentChatPermissionMode | null,
+): AgentChatPermissionMode | null {
+ if (!value) return null;
+ return launchPermissionOptions(modelId).some((option) => option.value === value)
+ ? value
+ : null;
+}
+
...
<ModelPicker
value={defaultModel}
- onChange={(next) => { setDefaultModel(next); applyDefaultField("modelId", next); }}
+ onChange={(next) => {
+ const nextPermission = normalizePermissionMode(next, defaultPermission);
+ setDefaultModel(next);
+ setDefaultPermission(nextPermission);
+ applyDefaultField("modelId", next);
+ applyDefaultField("permissionMode", nextPermission);
+ }}
surfaceKey="batch-launch-default"
compact
fastModeActive={defaultFast}
onFastModeToggle={(next) => { setDefaultFast(next); applyDefaultField("codexFastMode", next); }}
fastModeSupported={batchLaunchSupportsFastMode(defaultModel)}
/>
...
<ModelPicker
value={state.modelId}
- onChange={(modelId) => patchIssue(issue.id, { modelId })}
+ onChange={(modelId) =>
+ patchIssue(issue.id, {
+ modelId,
+ permissionMode: normalizePermissionMode(modelId, state.permissionMode ?? null),
+ })}
surfaceKey={`batch-launch-${issue.id}`}
compact
fastModeActive={state.codexFastMode}
onFastModeToggle={(next) => patchIssue(issue.id, { codexFastMode: next })}
fastModeSupported={batchLaunchSupportsFastMode(state.modelId)}Also applies to: 426-427, 466-484, 559-577
🤖 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/app/BatchLaunchModal.tsx` around lines
67 - 70, The PermissionPicker UI can show a different visible option than the
stored permissionMode when switching models; update the code to normalize
permissionMode whenever modelId (or the options returned by
launchPermissionOptions) changes by computing the available options (options =
launchPermissionOptions(modelId)), determining the active option (active =
options.find(o => o.value === permissionMode) ?? options[0]), and if
active.value !== permissionMode call the state updater (e.g.,
setPermissionMode(active.value)) before using permissionMode in handleLaunch or
submitting; apply the same normalization pattern around the PermissionPicker
usage and any other places that read permissionMode (handleLaunch, places
referenced by symbols PermissionPicker, handleLaunch, permissionMode,
setPermissionMode, launchPermissionOptions, active) so the submitted value
always matches the visible choice.
Comment on lines
+275
to
+302
| // Multi-select survives the temporary remount while the launch modal is open by | ||
| // mirroring ids to localStorage. The quick-view host clears this key on real | ||
| // pane close, so a fresh open starts unchecked. | ||
| function safeLoadSelection(projectRoot: string | null | undefined): Set<string> { | ||
| const key = selectionStorageKey(projectRoot); | ||
| if (!key || typeof window === "undefined") return new Set(); | ||
| try { | ||
| const parsed = JSON.parse(window.localStorage.getItem(key) ?? "null"); | ||
| if (!Array.isArray(parsed)) return new Set(); | ||
| return new Set(parsed.filter((id): id is string => typeof id === "string").slice(0, SELECTION_STORAGE_MAX)); | ||
| } catch { | ||
| return new Set(); | ||
| } | ||
| } | ||
|
|
||
| function safeSaveSelection(projectRoot: string | null | undefined, ids: Set<string>): void { | ||
| const key = selectionStorageKey(projectRoot); | ||
| if (!key || typeof window === "undefined") return; | ||
| try { | ||
| if (ids.size === 0) { | ||
| clearLinearQuickViewSelection(projectRoot); | ||
| return; | ||
| } | ||
| window.localStorage.setItem(key, JSON.stringify([...ids].slice(0, SELECTION_STORAGE_MAX))); | ||
| } catch { | ||
| // Best effort only; losing this selection should never block browsing issues. | ||
| } | ||
| } |
There was a problem hiding this comment.
Persist enough data to rebuild off-page selections.
safeSaveSelection only stores ids. After a remount, any issue selected from an earlier search/filter but not present on the current page cannot be reconstructed, so resolvedSelectedIssues drops it and batch launch can run on fewer issues than the restored selection.
Persist a minimal issue snapshot with each id, or hydrate seenIssuesRef from storage on mount before the launch actions are enabled.
🤖 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/app/LinearIssueBrowser.tsx` around lines
275 - 302, safeSaveSelection only writes ids, so safeLoadSelection cannot
reconstruct selections for items not on the current page and
resolvedSelectedIssues loses them; update persistence to store a minimal
snapshot per id (e.g., id plus title/summary/timestamp) when saving in
safeSaveSelection and update safeLoadSelection to parse and return both ids and
these minimal issue records to repopulate seenIssuesRef on mount (or
alternatively hydrate seenIssuesRef from storage before enabling launch
actions); ensure selectionStorageKey is used consistently and that
resolvedSelectedIssues and any launch-button enabling logic check the hydrated
seenIssuesRef so batch launches include restored off-page items.
Comment on lines
+87
to
+97
| function pruneExpiredCreating(): void { | ||
| const now = Date.now(); | ||
| for (const [issueId, placeholder] of creating) { | ||
| if (now - placeholder.requestedAt > PENDING_TTL_MS) creating.delete(issueId); | ||
| } | ||
| } | ||
|
|
||
| function emitCreating(): void { | ||
| const snapshot = [...creating.values()]; | ||
| for (const listener of creatingListeners) listener(snapshot); | ||
| } |
There was a problem hiding this comment.
TTL expiry for creating placeholders is not enforced for live subscribers.
Expired placeholders are pruned only when consumeCreatingIssues() is called. If a launch fails and no consume path runs again, subscribers can retain stale spinner placeholders indefinitely.
Suggested direction
+let creatingExpiryTimer: ReturnType<typeof setTimeout> | null = null;
+
+function scheduleCreatingExpirySweep(): void {
+ if (creatingExpiryTimer) clearTimeout(creatingExpiryTimer);
+ if (!creating.size) return;
+ const now = Date.now();
+ const nextExpiryAt = Math.min(...[...creating.values()].map((p) => p.requestedAt + PENDING_TTL_MS));
+ const delay = Math.max(0, nextExpiryAt - now);
+ creatingExpiryTimer = setTimeout(() => {
+ pruneExpiredCreating();
+ emitCreating();
+ scheduleCreatingExpirySweep();
+ }, delay);
+}
+
export function rememberCreatingIssues(args: {
issues: Array<{ issueId: string; name: string }>;
}): void {
if (!args.issues.length) return;
const now = Date.now();
for (const { issueId, name } of args.issues) {
if (!issueId) continue;
creating.set(issueId, { issueId, name, requestedAt: now });
}
emitCreating();
+ scheduleCreatingExpirySweep();
}
export function clearCreatingIssue(issueId: string): void {
- if (creating.delete(issueId)) emitCreating();
+ if (creating.delete(issueId)) {
+ emitCreating();
+ scheduleCreatingExpirySweep();
+ }
}Also applies to: 126-129
🤖 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/lib/launchedLanesHighlight.ts` around lines 87 -
97, Expired "creating" placeholders are only pruned in consumeCreatingIssues(),
so live subscribers may see stale items; update emitCreating() to call
pruneExpiredCreating() before building the snapshot (use the existing
pruneExpiredCreating function and the PENDING_TTL_MS constant and creating map)
so listeners always receive a TTL-enforced view, and apply the same pattern to
the pending path (add a pruneExpiredPending() helper and call it from the
corresponding emitPending()/pending snapshot emitter that uses
pendingListeners/pending collection).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+83
to
+97
|
|
||
| /** On agent launch: In Progress + self-assign + branch-link comment. */ | ||
| const onAgentLaunched = async (input: { | ||
| issue: LaneLinearIssue; | ||
| branchName?: string | null; | ||
| laneName?: string | null; | ||
| }): Promise<void> => { | ||
| if (!enabled) return; | ||
| const tracker = args.getIssueTracker(); | ||
| if (!tracker) return; | ||
| const issue = input.issue; | ||
| if (movedInProgress.has(issue.id)) return; | ||
| movedInProgress.add(issue.id); | ||
|
|
||
| try { |
There was a problem hiding this comment.
viewerIdResolved permanently set on transient startup errors
viewerIdResolved = true is set unconditionally before the try block, so a transient getConnectionStatus() failure (network hiccup, Linear auth not yet ready) permanently sets viewerId = null for the entire session. Every subsequent onAgentLaunched call skips auto-assignment silently. Consider only setting viewerIdResolved = true when the call succeeds (or the user is confirmed not connected), and leaving it false on exception so the next launch retries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/cto/linearLiveStatusService.ts
Line: 83-97
Comment:
**`viewerIdResolved` permanently set on transient startup errors**
`viewerIdResolved = true` is set unconditionally before the `try` block, so a transient `getConnectionStatus()` failure (network hiccup, Linear auth not yet ready) permanently sets `viewerId = null` for the entire session. Every subsequent `onAgentLaunched` call skips auto-assignment silently. Consider only setting `viewerIdResolved = true` when the call succeeds (or the user is confirmed not connected), and leaving it `false` on exception so the next launch retries.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+37
to
+38
| // Resolved once per team; workflow states rarely change within a session. | ||
| const statesByTeam = new Map<string, WorkflowStateLite[]>(); |
There was a problem hiding this comment.
statesByTeam is populated once and never invalidated, so a long-running app session won't pick up workflow state changes (e.g. a team renames or adds a new "In Progress" state). A short TTL or a resolvedAt timestamp would let the cache refresh without requiring an app restart.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/cto/linearLiveStatusService.ts
Line: 37-38
Comment:
`statesByTeam` is populated once and never invalidated, so a long-running app session won't pick up workflow state changes (e.g. a team renames or adds a new "In Progress" state). A short TTL or a `resolvedAt` timestamp would let the cache refresh without requiring an app restart.
How can I resolve this? If you propose a fix, please make it concise.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!
Comment on lines
+1263
to
+1268
| title?: string; | ||
| /** Foreground opens/focuses the session; background leaves focus alone. */ | ||
| disposition?: "foreground" | "background"; | ||
| }; | ||
|
|
||
| export type AgentChatLaunchCliResult = { |
There was a problem hiding this comment.
attachedLinearIssueIds
The comment says "Identifiers of the issues attached" but the field is populated from issue.id (UUID), not issue.identifier (the human-readable VER-123 form). The field name …IssueIds implies UUIDs, which matches the implementation, but the comment contradicts it. Given that ADE_LINEAR_ISSUE_IDS is separately documented as carrying identifiers (link.issue.identifier), a caller reading this type might assume a consistent convention — clarifying the comment would prevent confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/shared/types/chat.ts
Line: 1263-1268
Comment:
**Misleading JSDoc on `attachedLinearIssueIds`**
The comment says "Identifiers of the issues attached" but the field is populated from `issue.id` (UUID), not `issue.identifier` (the human-readable `VER-123` form). The field name `…IssueIds` implies UUIDs, which matches the implementation, but the comment contradicts it. Given that `ADE_LINEAR_ISSUE_IDS` is separately documented as carrying identifiers (`link.issue.identifier`), a caller reading this type might assume a consistent convention — clarifying the comment would prevent confusion.
How can I resolve this? If you propose a fix, please make it concise.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!
Merging origin/main (#430 made AppContext daemon services nullable) surfaced typecheck-desktop failures: the lane's Linear session-link IPC handlers and the launchCli handler used ctx services without null guards. Route the session-link handlers through ensureLaneContext(), agentChat.launch through ensureAgentChatContext(), and add a ptyService guard to agentChat.launchCli — matching the file's established ensure*Context pattern. Desktop typecheck green; 234 lane tests pass. 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. |
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.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Documentation
Greptile Summary
This PR wires Linear issues directly into the ADE agent lifecycle: issues can be attached to chat/CLI sessions, batch-launched across multiple issues simultaneously, and their Linear state is updated automatically on agent launch and PR merge (gated off by default).
linearBatchLaunch.ts,BatchLaunchModal.tsx): bounded-parallel create-lane + headless agent kickoff per Linear issue, with per-issue model/prompt/branch config, conflict detection, and optimistic lane placeholders.session_linear_issuestable,laneServiceattach/detach/list): chat and CLI sessions can own their own issue links, fanning out to the lane's PR-open closeout path.linearLiveStatusService.ts): opt-in service that moves issues to In Progress on launch, posts PR links on open, and moves to Done on merge.Confidence Score: 4/5
Safe to merge with one fix: CLI agents launched via the batch path are silently invisible in the agent dashboard due to a self-referential chatSessionId triggering the child-terminal filter.
The core batch-launch and session-link logic is well-structured and well-tested. One concrete defect: launchAgentChatCli passes chatSessionId: sessionId (its own id) to ptyService.create, and buildLaneAgents filters out every terminal whose chatSessionId is non-null as a child of a chat session. CLI agents from the new batch-launch path therefore never appear in the lane agent dashboard. The fix is a single-line removal; the ADE_LINEAR_* env injection is unaffected.
agentChatCliLaunch.ts for the chatSessionId self-reference, and kvDb.ts for the missing lane-id cascade on session_linear_issues.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as BatchLaunchModal participant BL as linearBatchLaunch participant LS as laneService participant PS as ptyService participant CS as agentChatService participant Linear as linearLiveStatusService UI->>BL: runBatchLaunch(entries, deps) loop "per issue (concurrency=3)" BL->>LS: "createLane({ linearIssue })" LS-->>BL: "{ id: laneId }" alt "sessionType === cli" BL->>PS: "ptyService.create({ linearIssues })" PS->>LS: attachLinearIssueToSession PS->>PS: "getSessionLinearEnv ADE_LINEAR_* injected" PS-->>BL: "{ sessionId }" else "sessionType === chat" BL->>CS: "launchHeadless({ contextAttachments })" CS-->>BL: "{ id: sessionId }" end BL->>Linear: onAgentLaunched(issue) opt-in end BL-->>UI: BatchLaunchResult note over Linear: on PR merge opt-in Linear->>Linear: onIssueMerged move to DonePrompt To Fix All With AI
Reviews (2): Last reviewed commit: "ship: guard nullable daemon services aft..." | Re-trigger Greptile