Reconcile Claude scheduled-work ownership and cancellation#811
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughDurable scheduled work now has persisted lifecycle metadata, Claude provider-session ownership, expiration and quarantine handling, shared snapshot reconciliation, cancellation APIs, CLI and remote commands, and desktop/iOS management controls. Tests cover scheduler migration, provider hooks, authorization, compatibility gating, cancellation, and UI behavior. ChangesScheduler lifecycle and provider integration
Cross-platform APIs and commands
Snapshot reconciliation and management UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 6447-6450: Update the scheduledWorkOperation condition to
recognize the same schedule aliases handled by the dispatch block:
“scheduled-work”, “schedules”, and “schedule”. Preserve the existing list/cancel
checks and ensure these aliases identify the operation before the sessionId
resolver processes positional arguments.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 1372-1383: Update the listScheduledWork adapter to pass an empty
object fallback to readObjectActionArg when args is undefined, while preserving
the existing sessionId and includeTerminal handling.
In `@apps/desktop/src/main/services/chat/chatScheduledWorkScheduler.ts`:
- Around line 298-306: Update the inactive-session cancellation branches in both
processDue and reconcileSchedule to invoke pruneTerminalHistory() immediately
after markTerminal, including when quarantineInactiveProviderSchedule does not
mark the schedule terminal. Preserve the existing timer cleanup, persistence,
transition emission, and return flow.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 5485-5506: Update the callProjectRuntimeActionOr invocations in
listScheduledWork and cancelScheduledWork to pass args directly instead of
wrapping them as { args }, while preserving the existing IPC fallback calls and
cache-clearing behavior.
In `@apps/desktop/src/renderer/components/settings/AiFeaturesSection.tsx`:
- Around line 430-436: The scheduled-work Cancel button in AiFeaturesSection
must be disabled when item.cancellable is false; update the button rendering
while preserving handleCancelScheduledWork for cancellable items. In
apps/desktop/src/renderer/components/settings/AiFeaturesSection.test.tsx lines
164-173, add cancellable: true to the mocked scheduled work item so the test can
click the enabled button.
In `@apps/desktop/src/shared/types/sync.ts`:
- Line 1030: Add "chat.listScheduledWork" to the SyncRemoteCommandAction union
alongside the existing chat scheduling actions, preserving the union’s type
contract for the remote action exposed by callProjectRuntimeActionOr.
🪄 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: 4737df37-534b-40f9-8519-cec5c1698d72
⛔ Files ignored due to path filters (5)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (40)
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/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/__tests__/chatInfo.test.tsapps/ade-cli/src/tuiClient/chatInfo.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatScheduledWorkScheduler.test.tsapps/desktop/src/main/services/chat/chatScheduledWorkScheduler.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/usage/usageStatsStore.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/settings/AiFeaturesSection.test.tsxapps/desktop/src/renderer/components/settings/AiFeaturesSection.tsxapps/desktop/src/shared/chatScheduledWork.test.tsapps/desktop/src/shared/chatScheduledWork.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
arul28
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 317fd82ac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
33480-33483: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce chat session scoping for schedule cancellation.
cancelScheduledWorkis exposed via IPC but does not verify that the target chat session exists or is valid. This allows remote clients to invoke cancellation without proper chat scoping (as noted in the PR objectives).Add the same
sessionServicecheck here that is already used insetScheduledWorkPausedandlistScheduledWorkto ensure proper authorization and state validation. As per path instructions, we must check for IPC security.🔒️ Proposed fix to add session scoping
await scheduledWorkReady; + const row = sessionService.get(normalizedSessionId); + if (!row || !isChatToolType(row.toolType)) { + throw new Error(`Chat session '${normalizedSessionId}' was not found.`); + } if (!scheduledWorkScheduler) {🤖 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 33480 - 33483, Update cancelScheduledWork to perform the same sessionService chat-session validation used by setScheduledWorkPaused and listScheduledWork before cancelling scheduled work; preserve the existing scheduler availability check and ensure invalid or unauthorized sessions are rejected through the established IPC security path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 33480-33483: Update cancelScheduledWork to perform the same
sessionService chat-session validation used by setScheduledWorkPaused and
listScheduledWork before cancelling scheduled work; preserve the existing
scheduler availability check and ensure invalid or unauthorized sessions are
rejected through the established IPC security path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9afd008-b44f-4f4b-a692-2c9e0e1773d7
📒 Files selected for processing (15)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatScheduledWorkScheduler.tsapps/desktop/src/renderer/components/settings/AiFeaturesSection.test.tsxapps/desktop/src/renderer/components/settings/AiFeaturesSection.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/shared/chatScheduledWork.test.tsapps/desktop/src/shared/chatScheduledWork.tsapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/shared/chatScheduledWork.test.ts
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/ade-cli/src/cli.ts
- apps/desktop/src/renderer/components/settings/AiFeaturesSection.tsx
- apps/desktop/src/shared/chatScheduledWork.ts
- apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/desktop/src/main/services/chat/chatScheduledWorkScheduler.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5bf448edb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
CodeRabbit out-of-diff session-validation suggestion was not applied: authorization is enforced at the ADE RPC/sync boundaries, while the service intentionally allows an operator to tombstone scheduled-work rows whose chat record is already gone. Requiring a live session row would recreate the exact uncancellable-orphan failure this PR fixes; the orphan cleanup regression remains green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef22b86486
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 670438443f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d63897b953
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
33443-33511: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
requestClaudeScheduledWorkCancellationcan throw and blockdeleteSession/archiveSession.
messageSession({ kind: "wake", ... })can throw (busy turn, full steer queue, pending-input guard), and whenconfirmedOwnerSchedules.length > 0this rethrows unconditionally (line ~33465). Separately, the 30s confirmation poll throws"Claude did not confirm CronDelete within 30 seconds…"if Claude never runs the tool call in time (line ~33511). Both call sites —deleteSession(line 34965-34971) andarchiveSession(line ~35050-35056) — call this with{ awaitConfirmation: true }and notry/catch, so either failure mode aborts the entire delete/archive operation, leaving the chat un-deleted.A user should always be able to delete/archive a chat regardless of whether the underlying agent is reachable. Consider catching the error at both call sites, logging a warning, and proceeding with deletion/archival (the orphaned schedule can still be reconciled later via the tombstone path already used for deleted-session rows).
🛡️ Proposed fix sketch
await scheduledWorkReady; if (scheduledWorkScheduler) { const providerSchedules = scheduledWorkScheduler.list(trimmedSessionId).filter((schedule) => schedule.provider === "claude" && schedule.status !== "done" && schedule.status !== "cancelled"); - await requestClaudeScheduledWorkCancellation(providerSchedules, { awaitConfirmation: true }); + try { + await requestClaudeScheduledWorkCancellation(providerSchedules, { awaitConfirmation: true }); + } catch (error) { + logger.warn("agent_chat.scheduled_work_cancel_before_delete_failed", { + sessionId: trimmedSessionId, + error: error instanceof Error ? error.message : String(error), + }); + } }(apply the equivalent change in
archiveSession)🤖 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 33443 - 33511, Wrap the `{ awaitConfirmation: true }` calls to `requestClaudeScheduledWorkCancellation` in both `deleteSession` and `archiveSession` with error handling so failures from the wake request or confirmation timeout are caught, logged as warnings with relevant error details, and do not abort deletion or archival. Preserve the existing successful cancellation behavior and allow the tombstone reconciliation path to handle any orphaned schedules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 33443-33511: Wrap the `{ awaitConfirmation: true }` calls to
`requestClaudeScheduledWorkCancellation` in both `deleteSession` and
`archiveSession` with error handling so failures from the wake request or
confirmation timeout are caught, logged as warnings with relevant error details,
and do not abort deletion or archival. Preserve the existing successful
cancellation behavior and allow the tombstone reconciliation path to handle any
orphaned schedules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01b20178-7fa1-445a-8f79-96afcc5d5708
📒 Files selected for processing (10)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/services/personalChats/personalChatScope.test.tsapps/ade-cli/src/services/personalChats/personalChatScope.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/shared/types/personalChats.tsapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/ade-cli/src/adeRpcServer.ts
|
Addressed CodeRabbit review 4693968581 in fb84223: deleteSession and archiveSession now catch/log provider cancellation wake or confirmation failures, then always honor the user lifecycle action and locally terminalize scheduled work. Added fake-timer regressions proving both operations complete after the 30-second provider timeout. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Verification
Open in ADE
Summary by CodeRabbit
ade chat scheduled-workCLI commands to list durable scheduled jobs and cancel a job.ade chathelp text to include scheduled-work list/cancel commands.Greptile Summary
This PR adds durable scheduled-work management across Claude-backed ADE chats. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Comments Outside Diff (2)
General comment
src/main/services/chat/agentChatService.test.ts > explicit provider-thread continuity recovery > decompresses a gzipped transcript when building the recovery capsule. The test creates only a.gztranscript containingCompressed original task, runsrecoverContinuity({ mode: "recover_from_history" }), and expects the returnedcapsulePreviewto include that original task. Instead the capsule preview omits it, so recovery from compressed history can lose the original task context.agentChatServicecontinuity recovery path appears not to read/decompress the.gztranscript when building the recovery capsule, or otherwise drops the decompressed user message before constructingcapsulePreview.capsulePreview; keep the existing test passing as regression coverage.General comment
adeRpcServer.test.tsspawn_agent tests. In each failure,runtime.ptyService.createis called without the expected concretecommandandargsfields for Claude/Codex launches, leaving onlystartupCommandplus env/metadata. This contradicts the PR verification claim that ADE CLI tests pass and weakens the runtime PTY launch contract these tests cover: spawn_agent callers/tests expect the launch executable and permission/model flags to be passed structurally, not only embedded in a preview string.apps/ade-cli/src/adeRpcServer.ts, the spawn_agent path conditionally spreads{ command: providerExecutable, args: commandArgs }only whenresolveExecutableOnPath(provider)returns a path. When resolution returns null in the test/runtime environment, the PTY create payload omits bothcommandandargs, even thoughstartupCommandstill contains the intended provider and flags.runtime.ptyService.createfor spawn_agent, falling back to the provider name when path resolution fails, or otherwise update the PTY service contract/tests intentionally if startupCommand-only launch is now supported. Preserve structuredargsfor Claude/Codex permission/model flags so PTY launch and management plumbing can inspect them reliably.Reviews (7): Last reviewed commit: "fix: preserve chat lifecycle recovery" | Re-trigger Greptile