Fix Claude runtime resolution and lane delete warnings#290
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@cursor review |
📝 WalkthroughWalkthroughThis PR extends system notice rendering with explicit severity fields, implements bundled Claude Code executable detection, enhances rate-limit event severity computation, introduces non-fatal lane deletion warnings, and integrates Claude plugin settings-based discovery and filtering. ChangesSystem Notice Severity, Executable Resolution, and Rate-Limit Handling
Lane Deletion with Non-Fatal Warnings
Claude Plugin Discovery with Settings Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@cursor review |
Owner
Author
|
@cursor review |
Owner
Author
|
@cursor review |
Owner
Author
|
@cursor review |
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b991737. Configure here.
| laneDeleteWarningMessagesRef.current.delete(laneId); | ||
| const remainingWarnings = formatLaneDeleteWarningMessages(laneDeleteWarningMessagesRef.current); | ||
| setLaneActionError((current) => remainingWarnings ?? (current && /\bdelet(?:e|ed|ing)\b/i.test(current) ? null : current)); | ||
| } |
There was a problem hiding this comment.
Stale error banner persists after clean lane deletion
Medium Severity
The completed branch clears laneActionError only when the current error matches /\bdelet(?:e|ed|ing)\b/i. A prior non-delete laneActionError (e.g. from an archive or adopt action) will persist indefinitely after a subsequent clean lane deletion, since the regex won't match. The old code unconditionally called setLaneActionError(null), which always cleared stale banners. The new conditional preserves unrelated errors that the user has no way to clear except by manually dismissing the chip.
Reviewed by Cursor Bugbot for commit b991737. Configure here.
| }); | ||
| return event; | ||
| } | ||
| } |
There was a problem hiding this comment.
Error decorator resolves executable without session environment
Low Severity
decorateAgentCliError calls resolveClaudeCodeExecutable() with no arguments (defaulting to process.env), while the session's SDK was started with resolveClaudeCodeExecutable({ env: claudeEnv }). If claudeEnv differs from process.env (e.g., a custom CLAUDE_CODE_EXECUTABLE_PATH injected by buildAgentRuntimeEnv), the suppression check may reference a different path than what the SDK actually attempted to spawn, potentially suppressing or failing to suppress the agentCli decoration incorrectly.
Reviewed by Cursor Bugbot for commit b991737. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/desktop/src/main/services/lanes/laneService.ts (1)
3548-3585: ⚡ Quick winConsider adding inline comments to clarify fatal vs non-fatal behavior.
The implementation is correct, but the distinction between fatal and non-fatal step failures could be clearer for future maintainers. Consider adding a brief comment explaining:
- Fatal failures (default) stop execution and throw
- Non-fatal failures log warnings and allow execution to continue
- The status is set to "warning" instead of "failed" for non-fatal failures
📝 Example inline documentation
const runStep = async ( name: LaneDeleteStepName, work: () => Promise<{ detail?: string } | void>, options?: { fatal?: boolean }, ): Promise<void> => { const step = findStep(name); if (!step) return; + // Fatal failures (default) stop execution; non-fatal failures log warnings and continue. const fatal = options?.fatal !== false; step.status = "running";🤖 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/lanes/laneService.ts` around lines 3548 - 3585, Add short inline comments inside the runStep function (around the try/catch and where fatal is computed) explaining the fatal vs non-fatal behavior: note that fatal defaults to true (options?.fatal !== false), fatal failures set step.status = "failed", set errorMessage, broadcast and rethrow to stop deletion; non-fatal failures set step.status = "warning", push to nonFatalFailures, log a warning via logger.warn("lane.delete.non_fatal_step_failed", ...), broadcastDeleteEvent(progress), and return to allow subsequent steps to continue. Place these comments near the declarations of fatal, the catch block, and where status is assigned so future maintainers can quickly see the intent.apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts (1)
67-71: ⚡ Quick winStrengthen executable-path assertion to reject empty strings.
expect.any(String)still passes"", which can hide resolver regressions.Diff suggestion
- pathToClaudeCodeExecutable: expect.any(String), + pathToClaudeCodeExecutable: expect.stringMatching(/.+/),🤖 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/ai/claudeRuntimeProbe.test.ts` around lines 67 - 71, The test currently uses expect.any(String) for pathToClaudeCodeExecutable which allows empty strings; update the assertion in the mockState.query expectation for pathToClaudeCodeExecutable to assert a non-empty string (for example use expect.stringMatching(/\S/) or expect.not.toEqual('')) so empty values fail and regressions are caught. Target the expectation inside claudeRuntimeProbe.test.ts where mockState.query is asserted.apps/desktop/src/main/services/chat/agentChatService.ts (1)
7011-7022: ⚡ Quick winConsider caching
resolveClaudeCodeExecutable()result if "missing" CLI errors occur frequently.The function performs filesystem and PATH scanning operations without caching. Similar utilities in the codebase (
git.ts,openCodeBinaryManager.ts) cache results to avoid repeated resolution work. If this error suppression path triggers multiple times in a session, caching the resolved path would improve efficiency.🤖 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 7011 - 7022, The suppression path calls resolveClaudeCodeExecutable() repeatedly; cache its result and reuse it to avoid repeated filesystem/PATH scans. Add a small module-level cache (e.g., cachedResolvedClaudeExecutable + cachedResolvedAt or a Map keyed by managed.session.id) and read from it in the if-block before calling resolveClaudeCodeExecutable(); only call resolveClaudeCodeExecutable() when there is no cached entry or the cache is stale, then store the returned object (path and source). Use the cached value in the isExecutablePath(resolved.path) check and when logging via logger.warn, and ensure the cache can be invalidated when environment/installation changes if necessary.apps/desktop/src/main/services/chat/claudeOutputStyles.ts (1)
226-230: 💤 Low valueVerify enablement predicate matches Claude SDK specification.
The function treats any value except
false,null,undefined, or{enabled: false}as enabled. This is permissive and would accept numbers, strings, empty objects, etc. as "enabled."While this may be intentional to support both boolean (
"plugin-id": true) and object ("plugin-id": {enabled: true, config: {...}}) formats, please verify this matches Claude SDK's actual plugin enablement semantics.Claude SDK plugin settings enablement format specification🤖 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/claudeOutputStyles.ts` around lines 226 - 230, The predicate is too permissive: update isEnabledPluginSetting to only return true for explicit boolean true or for objects where maybeRecord(value)?.enabled is true (or enabled is missing only if the Claude SDK allows implicit enablement—confirm spec); reject numbers, strings, empty objects, and other types. Modify the logic in isEnabledPluginSetting (and use maybeRecord) so it first checks typeof value === "boolean" && value === true, then checks for a record where enabled === true (or allowed missing enabled per SDK), otherwise return false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/ai/claudeCodeExecutable.ts`:
- Around line 27-30: isExecutablePath currently checks executability using
process.platform which breaks cross-platform resolution in
resolveBundledClaudeCodeExecutable(platform); update isExecutablePath to accept
a platform parameter (e.g., isExecutablePath(candidatePath: string, platform?:
string)) and use that platform value (fallback to process.platform if undefined)
when deciding Windows vs POSIX executable checks, then update calls from
resolveBundledClaudeCodeExecutable and any other callers (including the block
referenced around lines 80-92) to pass the platform argument so .exe files on
Windows bundles are recognized even when running on macOS/Linux.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 2536-2547: The inferredSeverity logic should first use
event.status (when present) to classify rate-limit notices before falling back
to message regexes; modify the inferredSeverity calculation in
AgentChatMessageList (and the similar block at the other occurrence) so that
when event.noticeKind === "rate_limit" and event.status exists you map known
statuses to severity (e.g., an allowed/ok status => "info", a warning status =>
"warning", failure/blocked => "error") and return that value prior to running
the regex tests on event.message; keep the existing fallbacks (event.severity,
noticeKind checks for error/warning, and final default "info") intact and
reference the same symbols (inferredSeverity, event.status, event.noticeKind,
event.message) so the change is localized and consistent across both locations.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Line 331: laneDeleteWarningMessagesRef (a useRef Map) retains entries across
project switches and unmounts causing stale messages to leak; clear it when the
surrounding project/context changes and on component unmount. Add logic that
calls laneDeleteWarningMessagesRef.current.clear() inside an effect that depends
on the project/context identifier (and/or in the component's cleanup) so the Map
is reset on project switch and when the component unmounts; reference the
existing laneDeleteWarningMessagesRef symbol to place the clear() calls.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts`:
- Around line 67-71: The test currently uses expect.any(String) for
pathToClaudeCodeExecutable which allows empty strings; update the assertion in
the mockState.query expectation for pathToClaudeCodeExecutable to assert a
non-empty string (for example use expect.stringMatching(/\S/) or
expect.not.toEqual('')) so empty values fail and regressions are caught. Target
the expectation inside claudeRuntimeProbe.test.ts where mockState.query is
asserted.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 7011-7022: The suppression path calls
resolveClaudeCodeExecutable() repeatedly; cache its result and reuse it to avoid
repeated filesystem/PATH scans. Add a small module-level cache (e.g.,
cachedResolvedClaudeExecutable + cachedResolvedAt or a Map keyed by
managed.session.id) and read from it in the if-block before calling
resolveClaudeCodeExecutable(); only call resolveClaudeCodeExecutable() when
there is no cached entry or the cache is stale, then store the returned object
(path and source). Use the cached value in the isExecutablePath(resolved.path)
check and when logging via logger.warn, and ensure the cache can be invalidated
when environment/installation changes if necessary.
In `@apps/desktop/src/main/services/chat/claudeOutputStyles.ts`:
- Around line 226-230: The predicate is too permissive: update
isEnabledPluginSetting to only return true for explicit boolean true or for
objects where maybeRecord(value)?.enabled is true (or enabled is missing only if
the Claude SDK allows implicit enablement—confirm spec); reject numbers,
strings, empty objects, and other types. Modify the logic in
isEnabledPluginSetting (and use maybeRecord) so it first checks typeof value ===
"boolean" && value === true, then checks for a record where enabled === true (or
allowed missing enabled per SDK), otherwise return false.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 3548-3585: Add short inline comments inside the runStep function
(around the try/catch and where fatal is computed) explaining the fatal vs
non-fatal behavior: note that fatal defaults to true (options?.fatal !== false),
fatal failures set step.status = "failed", set errorMessage, broadcast and
rethrow to stop deletion; non-fatal failures set step.status = "warning", push
to nonFatalFailures, log a warning via
logger.warn("lane.delete.non_fatal_step_failed", ...),
broadcastDeleteEvent(progress), and return to allow subsequent steps to
continue. Place these comments near the declarations of fatal, the catch block,
and where status is assigned so future maintainers can quickly see the intent.
🪄 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: 18bbec48-f9fa-4856-bcdc-b2d8648ec0bf
⛔ Files ignored due to path filters (2)
docs/features/chat/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**
📒 Files selected for processing (20)
apps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/format.tsapps/desktop/src/main/packagedRuntimeSmoke.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/claudeCodeExecutable.test.tsapps/desktop/src/main/services/ai/claudeCodeExecutable.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.test.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/claudeOutputStyles.test.tsapps/desktop/src/main/services/chat/claudeOutputStyles.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/lanes.ts
Comment on lines
+27
to
+30
| export function isExecutablePath(candidatePath: string): boolean { | ||
| try { | ||
| const stat = fs.statSync(candidatePath); | ||
| return stat.isFile() && (process.platform === "win32" || (stat.mode & 0o111) !== 0); |
There was a problem hiding this comment.
Use the target platform when checking executability.
resolveBundledClaudeCodeExecutable() accepts platform, but isExecutablePath() still keys off process.platform. That makes cross-platform resolution false-negative for Windows bundles when this code runs on macOS/Linux, so a valid packaged .exe can be skipped and the resolver falls back to auth/PATH instead.
Suggested fix
-export function isExecutablePath(candidatePath: string): boolean {
+export function isExecutablePath(
+ candidatePath: string,
+ platform: NodeJS.Platform = process.platform,
+): boolean {
try {
const stat = fs.statSync(candidatePath);
- return stat.isFile() && (process.platform === "win32" || (stat.mode & 0o111) !== 0);
+ return stat.isFile() && (platform === "win32" || (stat.mode & 0o111) !== 0);
} catch {
return false;
}
}
@@
for (const candidate of candidates) {
- if (isExecutablePath(candidate)) return candidate;
+ if (isExecutablePath(candidate, platform)) return candidate;
}
@@
- return isExecutablePath(binaryPath) ? binaryPath : null;
+ return isExecutablePath(binaryPath, platform) ? binaryPath : null;Also applies to: 80-92
🤖 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/ai/claudeCodeExecutable.ts` around lines 27 -
30, isExecutablePath currently checks executability using process.platform which
breaks cross-platform resolution in
resolveBundledClaudeCodeExecutable(platform); update isExecutablePath to accept
a platform parameter (e.g., isExecutablePath(candidatePath: string, platform?:
string)) and use that platform value (fallback to process.platform if undefined)
when deciding Windows vs POSIX executable checks, then update calls from
resolveBundledClaudeCodeExecutable and any other callers (including the block
referenced around lines 80-92) to pass the platform argument so .exe files on
Windows bundles are recognized even when running on macOS/Linux.
Comment on lines
+2536
to
+2547
| const inferredSeverity = event.severity | ||
| ?? (event.noticeKind === "rate_limit" && /^Claude rate limit allowed warning/i.test(event.message) ? "warning" as const : undefined) | ||
| ?? (event.noticeKind === "rate_limit" && /^Claude rate limit allowed/i.test(event.message) ? "info" as const : undefined) | ||
| ?? ( | ||
| event.noticeKind === "rate_limit" | ||
| || event.noticeKind === "error" | ||
| || event.noticeKind === "thread_error" | ||
| || event.noticeKind === "provider_health" | ||
| ? "error" as const | ||
| : undefined | ||
| ) | ||
| ?? (event.noticeKind === "warning" ? "warning" as const : "info" as const); |
There was a problem hiding this comment.
Use event.status to classify rate-limit severity before message regexes.
Line 2536 currently relies on severity or message text patterns. If upstream provides status (but no severity), a non-blocking rate-limit notice can still render as an error when message text changes.
Suggested fix
+ const rateLimitStatus = event.noticeKind === "rate_limit"
+ ? String(event.status ?? "").trim().toLowerCase()
+ : "";
const inferredSeverity = event.severity
+ ?? (
+ event.noticeKind === "rate_limit" && /^allowed(?:_|$)/.test(rateLimitStatus)
+ ? (rateLimitStatus.includes("warning") ? "warning" as const : "info" as const)
+ : undefined
+ )
?? (event.noticeKind === "rate_limit" && /^Claude rate limit allowed warning/i.test(event.message) ? "warning" as const : undefined)
?? (event.noticeKind === "rate_limit" && /^Claude rate limit allowed/i.test(event.message) ? "info" as const : undefined)
?? (
event.noticeKind === "rate_limit"
|| event.noticeKind === "error"
|| event.noticeKind === "thread_error"
|| event.noticeKind === "provider_health"
? "error" as const
: undefined
)
?? (event.noticeKind === "warning" ? "warning" as const : "info" as const);As per coding guidelines **/*.{ts,tsx}: Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes in TypeScript.
Also applies to: 2570-2589
🤖 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/chat/AgentChatMessageList.tsx` around
lines 2536 - 2547, The inferredSeverity logic should first use event.status
(when present) to classify rate-limit notices before falling back to message
regexes; modify the inferredSeverity calculation in AgentChatMessageList (and
the similar block at the other occurrence) so that when event.noticeKind ===
"rate_limit" and event.status exists you map known statuses to severity (e.g.,
an allowed/ok status => "info", a warning status => "warning", failure/blocked
=> "error") and return that value prior to running the regex tests on
event.message; keep the existing fallbacks (event.severity, noticeKind checks
for error/warning, and final default "info") intact and reference the same
symbols (inferredSeverity, event.status, event.noticeKind, event.message) so the
change is localized and consistent across both locations.
| const [laneActionError, setLaneActionError] = useState<string | null>(null); | ||
| const [laneActionKind, setLaneActionKind] = useState<"delete" | "archive" | "adopt" | null>(null); | ||
| const [deleteProgressByLaneId, setDeleteProgressByLaneId] = useState<Record<string, LaneDeleteProgress>>({}); | ||
| const laneDeleteWarningMessagesRef = useRef<Map<string, string>>(new Map()); |
There was a problem hiding this comment.
Clear stale delete-warning state on project/context reset (Line 331).
laneDeleteWarningMessagesRef survives project switches and unmount cleanup paths, so old warning text can leak into later delete warning aggregation.
💡 Proposed fix
useEffect(() => {
completedLaneDeleteRefreshesRef.current.clear();
pendingLaneDeleteRefreshIdsRef.current.clear();
+ laneDeleteWarningMessagesRef.current.clear();
+ setLaneActionError((current) =>
+ current && /\bdelet(?:e|ed|ing)\b/i.test(current) ? null : current
+ );
if (laneDeleteRefreshTimerRef.current != null) {
window.clearTimeout(laneDeleteRefreshTimerRef.current);
laneDeleteRefreshTimerRef.current = null;
}
setDeleteProgressByLaneId({});
}, [project?.rootPath]);
useEffect(() => {
const pendingLaneDeleteRefreshIds = pendingLaneDeleteRefreshIdsRef.current;
return () => {
+ laneDeleteWarningMessagesRef.current.clear();
for (const timer of chipTimersRef.current.values()) window.clearTimeout(timer);
chipTimersRef.current.clear();
if (laneDeleteRefreshTimerRef.current != null) {
window.clearTimeout(laneDeleteRefreshTimerRef.current);
laneDeleteRefreshTimerRef.current = null;
}
pendingLaneDeleteRefreshIds.clear();
};
}, []);📝 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 laneDeleteWarningMessagesRef = useRef<Map<string, string>>(new Map()); | |
| useEffect(() => { | |
| completedLaneDeleteRefreshesRef.current.clear(); | |
| pendingLaneDeleteRefreshIdsRef.current.clear(); | |
| laneDeleteWarningMessagesRef.current.clear(); | |
| setLaneActionError((current) => | |
| current && /\bdelet(?:e|ed|ing)\b/i.test(current) ? null : current | |
| ); | |
| if (laneDeleteRefreshTimerRef.current != null) { | |
| window.clearTimeout(laneDeleteRefreshTimerRef.current); | |
| laneDeleteRefreshTimerRef.current = null; | |
| } | |
| setDeleteProgressByLaneId({}); | |
| }, [project?.rootPath]); | |
| useEffect(() => { | |
| const pendingLaneDeleteRefreshIds = pendingLaneDeleteRefreshIdsRef.current; | |
| return () => { | |
| laneDeleteWarningMessagesRef.current.clear(); | |
| for (const timer of chipTimersRef.current.values()) window.clearTimeout(timer); | |
| chipTimersRef.current.clear(); | |
| if (laneDeleteRefreshTimerRef.current != null) { | |
| window.clearTimeout(laneDeleteRefreshTimerRef.current); | |
| laneDeleteRefreshTimerRef.current = null; | |
| } | |
| pendingLaneDeleteRefreshIds.clear(); | |
| }; | |
| }, []); |
🤖 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` at line 331,
laneDeleteWarningMessagesRef (a useRef Map) retains entries across project
switches and unmounts causing stale messages to leak; clear it when the
surrounding project/context changes and on component unmount. Add logic that
calls laneDeleteWarningMessagesRef.current.clear() inside an effect that depends
on the project/context identifier (and/or in the component's cleanup) so the Map
is reset on project switch and when the component unmounts; reference the
existing laneDeleteWarningMessagesRef symbol to place the clear() calls.
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
Validation
Note
Medium Risk
Medium risk: changes how the Claude runtime executable is resolved/passed into the SDK and alters lane deletion completion semantics, which could affect startup reliability and user-facing delete flows across platforms.
Overview
Improves Claude runtime reliability by centralizing executable resolution in
claudeCodeExecutable.ts(env override → packaged bundled binary → auth/PATH fallbacks) and threadingpathToClaudeCodeExecutablethrough Claude probes, smoke tests, and chat session startup, with clearer diagnostics when spawn errors look like false “native binary not found” failures.Refines chat/system notice handling by adding
severity/statustosystem_notice, emitting structured Claude/Codex rate-limit severities, and rendering non-blocking “allowed” rate-limit telemetry as non-error notices in both the CLI TUI and desktop UI.Makes lane deletion more resilient by treating local/remote branch deletion as non-fatal steps, introducing
warningstep status pluscompleted_with_warnings, and updating the renderer progress/error surfaces to show actionable warnings while still completing local lane cleanup.Tightens Claude plugin discovery to avoid loading cache/marketplace copies unless a plugin is installed and enabled via Claude settings/registry.
Reviewed by Cursor Bugbot for commit b991737. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR centralises Claude executable resolution into a single
resolveClaudeCodeExecutable()helper (env override → ASAR-unpacked bundled binary → auth/PATH fallbacks) and threadspathToClaudeCodeExecutablethrough probe, startup, and session call sites. It also addsseverity/statusfields tosystem_noticeevents so allowed rate-limit telemetry renders as non-error notices, refines plugin discovery to skip uninstalled marketplace cache copies, and makes branch-cleanup steps non-fatal so lane deletion can finish ascompleted_with_warnings.claudeCodeExecutable.ts,claudeRuntimeProbe.ts,agentChatService.ts,packagedRuntimeSmoke.ts): newresolveBundledClaudeCodeExecutableprobes ASAR-unpacked paths first; false-negative "native binary not found" SDK errors are suppressed when the resolved binary already exists on disk.claudeOutputStyles.ts):cache/marketplacessubdirectories are skipped by the normal walk; only plugins listed ininstalled_plugins.jsonwith matchingenabledPluginssettings are loaded from managed dirs.laneService.ts,LanesPage.tsx,ManageLaneDialog.tsx):git_branch_deleteandgit_remote_branch_deleterun with{ fatal: false }, upgrade failed branch-cleanup towarningstep status, and bubble the aggregate up tocompleted_with_warningswith amber UI treatment.Confidence Score: 5/5
Safe to merge; the runtime resolution and lane-deletion changes are well-tested and follow a clear, incremental strategy.
All critical paths — bundled binary resolution with ASAR guard, session startup with pathToClaudeCodeExecutable, probe health check, and the non-fatal branch-cleanup flow — have accompanying unit tests covering the new behaviour. The two observations raised are edge cases in error-message suppression and stale banner state that don't affect core functionality.
No files require special attention beyond the two inline comments.
Important Files Changed
resolveBundledClaudeCodeExecutablewith ASAR-unpacked path probing and aresolvePackageFromRuntimeRootsfallback guarded by an ASAR path check;isExecutablePathexported for use in probe/suppression logic.pathToClaudeCodeExecutableinto session startup; adds "missing" error suppression using a second unconstrainedresolveClaudeCodeExecutable()call; rate-limit events now carryseverity/status; stale-session check gains the "no conversation found" message variant.cache/marketplacesdirs at depth 0; addsdiscoverEnabledInstalledPluginPathsto load marketplace plugins frominstalled_plugins.jsononly when enabled in settings.runStepgains an optionalfatalflag; branch-cleanup steps run with{ fatal: false };completed_with_warningsoverall status surfaced when non-fatal failures accumulate.isLaneDeleteProgressActiveand lane refresh path now handlecompleted_with_warnings; warning messages tracked in a ref; clean-delete path uses a regex heuristic to clear stale delete-related error banners.completed_with_warningsamber tone toDeleteProgressStripandwarningstep rendering with inline error message display inProgressStepRow.inferredSeveritylogic for rate-limit notice styling; non-error rate-limit notices with detail render as compact inline chip without a collapsible card.pathToClaudeCodeExecutableto the probe query; adds a false-negative suppression path that enriches the error message with the resolved path when SDK reports "native binary not found" despite a resolvable executable."warning"toLaneDeleteStepStatusand"completed_with_warnings"toLaneDeleteOverallStatus.severityandstatusfields to thesystem_noticeevent union member.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[resolveClaudeCodeExecutable] --> B{CLAUDE_CODE_EXECUTABLE_PATH set?} B -- yes --> C[source: env] B -- no --> D{ASAR-unpacked binary exists?} D -- yes --> E[source: bundled] D -- no --> F{Auth-detected path?} F -- yes --> G[source: auth] F -- no --> H{Binary on PATH?} H -- yes --> I[source: path / common-dir] H -- no --> J[source: fallback-command] C & E & G & I --> K[pathToClaudeCodeExecutable passed to SDK] J --> K K --> L[Claude SDK session / probe] L --> M{SDK reports native binary not found?} M -- yes --> N{source != fallback-command AND isExecutablePath?} N -- yes --> O[Suppress false-negative, log warning] N -- no --> P[Surface error to user] M -- no --> Q[Normal event flow]Comments Outside Diff (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx, line 2046-2050 (link)The backward-compat fallback for old events without
severityderives the visual tone by regex-testingevent.message. If the message wording changes inagentChatService.ts— even just capitalisation or a prefix change — old events replayed from the database will silently regress to "error" styling. Sinceseverityis now always emitted by the service, consider treating events without an explicitseverityon arate_limitnotice as"error"unconditionally, removing the fragile regex path entirely.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "Stabilize lane delete warning state" | Re-trigger Greptile