Orchestration: idempotent delegation, transactional outbox, event-driven completion (ADE-124), light-plan lifecycle, evidence UI#878
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds durable orchestration receipts and outbox delivery, expands planning and recovery state, introduces lead-only waiting and read tools, records external evidence provenance, centralizes deeplink navigation, and displays evidence in the orchestration panel. ChangesOrchestration runtime and planning
Evidence presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/ai/tools/orchestrationTools.ts (1)
609-705: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the cancellation manifest patch with the delivery receipt.
messageAgentcommits the cancellation attempt/decision patches viamanifestPatchbeforefinalizeDelivery. IfcompleteReceiptlater fails, the receipt is released and a retry will reserve the same deterministicrequestId;cancelledAtis generated per execute call, so the re-applied patch appends a newA-cancel-...attempt and duplicate/decisions/-entry. Combine the cancellation patches withfinalizeDelivery's receipt transaction, or make the cancellation attempt/decision ids deterministic/idempotent.🤖 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/tools/orchestrationTools.ts` around lines 609 - 705, Make the cancellation flow in the intent === "cancellation" branch idempotent across receipt retries: derive cancelledAt and the cancellation decision/attempt identifiers from stable request data such as requestId, and update buildCancellationPatch to avoid appending duplicate decision or attempt entries when the patch is reapplied. Preserve the existing cancellation manifest updates and finalizeDelivery behavior while ensuring a retry cannot create duplicate records.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/orchestration/orchestrationService.ts (1)
2785-2789: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
addwhen writing absent top-level manifest fields.applyPatchescurrently writes/goalSourceand/finishingon non-existent paths, but objectaddis treated the same asreplace; useaddhere to match the service convention and future-proof against stricter RFC-6902 semantics.
apps/desktop/src/main/services/orchestration/orchestrationService.ts#L2580-L2582:/goalSourceinrecordPlanningIntake.apps/desktop/src/main/services/orchestration/orchestrationService.ts#L2744-L2745:/finishinginrecordFinishingChoice.apps/trunk/src/main/services/orchestration/orchestrationService.ts#L2785-L2789:/goalSourceinrecordGoalSource.🤖 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/orchestration/orchestrationService.ts` around lines 2785 - 2789, Use JSON Patch “add” instead of “replace” when writing absent top-level manifest fields: update /goalSource in recordGoalSource at apps/desktop/src/main/services/orchestration/orchestrationService.ts:2785-2789, /goalSource in recordPlanningIntake at :2580-2582, and /finishing in recordFinishingChoice at :2744-2745. Preserve the existing patch values and surrounding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md`:
- Around line 169-170: Correct the process-detection guidance near the `pgrep`
fallback: remove the imprecise `pgrep -f "codex exec" | grep -v $$` command, or
replace it with a precise check that identifies the intended process by captured
PID, argv, or session-file data and excludes the current invocation and
siblings. Keep the captured-PID/log/session-file path as the preferred approach.
- Around line 163-168: Update the detached codex execution guidance to capture
the background PID immediately via $! in a named variable, then use that exact
variable with wait for completion. Present the captured process exit status as
the authoritative completion signal, and remove or de-emphasize pgrep, log
markers, and session-file checks as deterministic evidence.
In `@apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md`:
- Line 60: Make Linear synchronization conditional on the run having a linked
Linear issue, while keeping PR finishing available for PR, goal.md, and
user-originated runs. In
apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md at lines 60, 101,
and 151, describe the finishing choice and finishing-phase worker steps as PR
work, adding the Linear update and linear_issue asset only when an issue is
linked. Apply the same conditional instruction in
apps/desktop/src/main/services/ai/tools/systemPrompt.ts at line 133.
- Line 46: Make plan-section requirements conditional on planning mode. In
apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md:46, state that the
lighter path is complete with Goal, Implementation order, Agent plan, and
Validation, while the full section list applies only to normal planning. In
apps/desktop/src/main/services/ai/tools/systemPrompt.ts:129, qualify the
all-sections requirement by planning mode so it matches assessPlanReadiness and
does not recreate the full ceremony for lightPlan.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 13734-13803: Update buildOrchestrationLeadReadServices so
listProofArtifacts uses the late-bound computerUseArtifactBrokerRef for both
availability checking and artifact listing, rather than the stale
computerUseArtifactBrokerService constructor parameter. Preserve the existing
limit handling and result shape, while allowing the method to attach after
setComputerUseArtifactBrokerService wires the broker.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ai/tools/orchestrationTools.ts`:
- Around line 609-705: Make the cancellation flow in the intent ===
"cancellation" branch idempotent across receipt retries: derive cancelledAt and
the cancellation decision/attempt identifiers from stable request data such as
requestId, and update buildCancellationPatch to avoid appending duplicate
decision or attempt entries when the patch is reapplied. Preserve the existing
cancellation manifest updates and finalizeDelivery behavior while ensuring a
retry cannot create duplicate records.
---
Nitpick comments:
In `@apps/desktop/src/main/services/orchestration/orchestrationService.ts`:
- Around line 2785-2789: Use JSON Patch “add” instead of “replace” when writing
absent top-level manifest fields: update /goalSource in recordGoalSource at
apps/desktop/src/main/services/orchestration/orchestrationService.ts:2785-2789,
/goalSource in recordPlanningIntake at :2580-2582, and /finishing in
recordFinishingChoice at :2744-2745. Preserve the existing patch values and
surrounding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f6056014-55e2-4dd9-8784-9f7013a7695a
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**
📒 Files selected for processing (23)
apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/orchestrationOutbox.tsapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.tsapps/desktop/src/main/services/ai/tools/orchestrationRuntime.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.test.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.tsapps/desktop/src/main/services/ai/tools/orchestrationValidationDerivation.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationDomain.test.tsapps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.test.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/renderer/components/orchestration/EvidenceSection.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.test.tsapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.tsapps/desktop/src/shared/types/orchestration.ts
|
@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: ba836f1d12
ℹ️ 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".
| status: nextAttempts >= claimed.maxAttempts ? "failed" : "pending", | ||
| error: errorMessage(err), | ||
| backoffMs: Math.min(30_000, 500 * 2 ** claimed.attempts), | ||
| }, |
There was a problem hiding this comment.
When a transient delivery failure occurs, this marks the entry pending with a future nextAttemptAt, but runDrain immediately excludes the attempted ID and no timer or future drain is scheduled. If this is a newly spawned worker's brief and the lead only calls awaitAgent, neither side generates another mutation, so spawnAgent reports success while the worker remains permanently idle unless the lead happens to invoke recoverStaleTasks or another draining action.
Useful? React with 👍 / 👎.
| // Idempotency and delivery state are authoritative service-owned records. | ||
| // Tool implementations write them through mutex-protected directPatch APIs. | ||
| "/receipts", | ||
| "/receipts/**", | ||
| "/outbox", | ||
| "/outbox/**", |
There was a problem hiding this comment.
The lead patch policy remains allow-by-default and protects the new receipts/outbox fields but not /finishing, so a lead can set finishing.mode directly through manifestPatch without displaying chooseFinishingMode to the user. In a run where it writes mode: "pr", the finishing flow can push the branch and open a PR despite the dedicated user-choice card never being accepted; make this field service-owned like the other gated lifecycle state.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
| if (reservation.status === "duplicate") { | ||
| return { | ||
| ok: true as const, | ||
| sessionId: reservation.receipt.result?.sessionId, | ||
| etag: reservation.receipt.result?.etag, | ||
| deduped: true, |
There was a problem hiding this comment.
A duplicate receipt can still have status: "pending" after a concurrent call or a process crash between reservation and completion, yet this branch returns ok: true with an undefined sessionId. The caller therefore believes delegation succeeded even though no agent row, session, or brief may exist, and the request stays suppressed until a later stale-receipt sweep; only completed receipts should replay success, while pending ones should wait or return an in-progress/retryable result.
Useful? React with 👍 / 👎.
| export function evidenceExternalUrl(asset: OrchestrationAsset): string | null { | ||
| const url = asset.externalRef?.url?.trim(); | ||
| return url ? url : null; |
There was a problem hiding this comment.
For proof_artifact, computer_use, and video assets carrying only externalRef.artifactId—the normal shape described here—this returns null, and both the aggregate row and agent chip render as noninteractive elements. No artifact callback is passed from OrchestrationPanel, so the claimed proof-drawer handling does not exist and users cannot inspect the evidence from this new UI; derive an artifact deeplink or wire the proof drawer for artifactId values.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d601eb25c1
ℹ️ 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".
| ops.push( | ||
| ...buildAgentCompletionOutboxOps(manifest, actor, task, edgeStatus), | ||
| ); |
There was a problem hiding this comment.
When a worker or validator releases a task as done or failed, this appends a completion outbox entry, but createReleaseTaskTool still wraps the call in withMutationSideEffects, which separately enqueues a lead_status notification. The subsequent drain delivers both entries via steer, so every terminal transition injects two prompts into the lead session and can trigger duplicate follow-up work. Suppress the generic release notification for terminal transitions or use only the new completion entry.
Useful? React with 👍 / 👎.
| patches.push( | ||
| task.attempts | ||
| ? { | ||
| op: "add", | ||
| path: `/tasks/{id:${task.id}}/attempts/-`, | ||
| value: attempt, | ||
| } |
There was a problem hiding this comment.
When the cancellation manifestPatch succeeds but finalizeDelivery later fails to complete the receipt—for example because persistence encounters a conflict—the receipt is released while this newly added cancellation attempt is already durable. Retrying the same logical request therefore appends another cancellation decision and another attempt with a new timestamp, defeating messageAgent idempotency and corrupting task-attempt history. Fold these cancellation patches into the same completeReceipt transaction as the receipt and outbox entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tools/orchestrationOutbox.ts`:
- Around line 36-59: Update scheduleDeferredRetry and the outbox retry flow so
pending entries whose backoff has expired or whose claimOutboxEntry attempt
fails have their stale retry metadata retired and can be retried immediately.
Ensure expired entries are not silently skipped by the retry scanner. In the
service/runtime teardown path, clear retryTimers and inFlight for the run before
disposal so no deferred drain can start afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ee535f0-3554-45be-b7e7-a9cadd0fbf0a
📒 Files selected for processing (14)
apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/services/ai/tools/orchestrationOutbox.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.test.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.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/orchestration/orchestrationService.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.test.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.test.tsapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/components/orchestration/orchestrationEvidence.test.ts
- apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- apps/desktop/src/renderer/components/orchestration/orchestrationEvidence.ts
- apps/desktop/src/main/services/orchestration/orchestrationService.test.ts
- apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md
- apps/desktop/src/main/services/ai/tools/orchestrationTools.ts
- apps/desktop/src/main/services/orchestration/orchestrationService.ts
…chema + SKILL refresh with plain-language protocol Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ancellation reach Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…only capability tools (ADE-124) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ith per-run PR choice, evidence UI, goalSource intake Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ness, stall detection on existing sweep Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eap wedged pending receipts, extract delivery epilogue Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ceipt completion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…strationDomain suite Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… lead read tools, light-plan lifecycle, evidence assets Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… corrections from review Read the late-bound computerUseArtifactBrokerRef (mutated post-construction by setComputerUseArtifactBrokerService to break a circular dep) instead of the raw constructor param when building lead read services, so listProofArtifacts attaches for lead sessions when the broker is wired via the setter. Add a regression test proving the tool reaches the broker when set after construction. SKILL/systemPrompt corrections from CodeRabbit review: - ade-cli-control-plane: capture background codex PID (CODEX_PID=$!) and wait on it as the authoritative completion signal; demote pgrep/log-markers/session-file to supporting evidence, keeping the session-file check as a wedge-detection liveness diagnostic. - ade-orchestrator: required plan sections are conditional on planning path (light path = Goal/Implementation order/Agent plan/Validation plan; full path = 9 sections), matching assessPlanReadiness. - ade-orchestrator: Linear updates + linear_issue asset registration in the finishing flow are conditional on the run being linked to a Linear issue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hing patch protection, pending-receipt replay guard, openable artifact evidence Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…notification, atomic cancellation, outbox retry hygiene Address four review findings on PR #878: - Light-plan model selection deadlock: isPlanningReadyForModelSelection now accepts the light_plan stage. The condensed path collapses the three rounds but readiness still requires model routing, so model selection must unlock at light_plan or the run can never reach ready. - Single completion notification: releaseTask reports whether it enqueued a structured completion entry; the release tool suppresses the generic lead_status ping for that same terminal transition (still draining the outbox so the completion is delivered), so the lead is notified once. - Atomic cancellation: the cancellation flag/decision/task-attempt patches are folded into the same completeReceipt transaction as the receipt and outbox entry, so a crash can no longer leave cancellation durable while the receipt is released (which let a retry re-append a second decision/attempt). - Outbox retry hygiene: an age cap settles a persistently un-deliverable entry failed instead of re-arming it forever; the coalesced deferred-retry timer and in-flight tracking are cleared on run teardown and service dispose so a disposed run cannot fire a stray drain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d601eb2 to
eb19dae
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb19dae3ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const work = runDrain(svc, chat, ctx).catch(() => undefined); | ||
| inFlight.set(ctx.runId, work); |
There was a problem hiding this comment.
If ADE exits after committing a worker's brief outbox row but before this drain runs, both the in-memory timer and inFlight state are lost. On restart, bundleRead/subscribe hydrate the manifest but never call drainOutbox, and retrying the same spawnAgent only replays its completed receipt without draining, so the worker can remain permanently pending without receiving its brief until an unrelated mutating tool happens to trigger a drain. Start a drain when a run with pending/delivering entries is loaded or when its chat backing becomes available.
Useful? React with 👍 / 👎.
| // success). Once it ages past the TTL we drop the wedged entry and reserve | ||
| // fresh so the deterministic requestId is not suppressed forever (this | ||
| // mirrors the recoverStaleTasks reap, but inline so a retry can proceed |
There was a problem hiding this comment.
When the process crashes after chat.createSession succeeds but before completeReceipt records the session ID, this generic TTL path drops the pending spawnAgent receipt after 15 minutes and permits the same deterministic request to create another session. The first session is persisted by the chat service but is absent from the manifest, and no request ID is stored on it for reconciliation, so retries leave duplicate/orphan orchestration chats. Stale spawn receipts need to recover or remove the matching existing session before they can be reserved again.
Useful? React with 👍 / 👎.
| const artifactId = asset.externalRef?.artifactId?.trim(); | ||
| if (artifactId) { | ||
| return buildDeeplink({ kind: "artifact", artifactId }, { form: "ade" }); |
There was a problem hiding this comment.
For proof, computer-use, and video assets that only provide artifactId, this returns an ade://artifact/... URL, but EvidenceSection passes it to openUrlInAdeBrowser; that helper falls back to the app's external-link IPC for non-http schemes, while services/shared/externalLinks.ts::normalizeExternalUrl rejects every scheme except HTTP(S) and mailto. The rejected promise is swallowed, so clicking these newly rendered evidence rows/chips does nothing. Dispatch the parsed ADE navigation target internally instead of treating it as an external browser URL.
Useful? React with 👍 / 👎.
… receipts, internal artifact navigation Round-3 review fixes for PR #878: P1 — Drain persisted outbox on run reload: the service now fires a coalesced activation drain when a run's runtime cold-hydrates (the single loadIntoRuntime chokepoint covers subscribe / bundleRead / hydrateRunForList / cold mutation). The chat service registers a stable chat-backed drainer (registerRunActivationDrainer), so a process restart delivers a persisted brief/ping without waiting for an unrelated mutating tool. Reuses the outbox inFlight guard for idempotency. P2 — Reconcile stale/pruned spawn+message receipts: reserveReceipt now checks whether the side effect already materialized (an agent linked by the new spawnRequestId for spawnAgent, or an outbox entry by requestId for messageAgent) before dropping a stale-pending or re-reserving a pruned receipt, completing the receipt with that result instead of duplicating the session/message. P2 — Route artifact deeplinks internally: evidence ade:// targets now dispatch through ADE's internal navigation (shared deeplinkToNavigationTarget) instead of the external-URL IPC that rejects non-http(s) schemes and swallowed the click; http(s)/file URLs keep the browser path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa34cf9ecb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const requestId = | ||
| input.requestId ?? | ||
| `msg:${sha256( | ||
| `${args.ctx.runId}|${args.ctx.sessionId}|${input.targetSessionId}|${input.kind}|${intent}|${input.taskId ?? ""}|${input.text}`, | ||
| )}`; |
There was a problem hiding this comment.
When requestId is omitted, this key depends only on the message content and participants, so a later intentional repetition—such as sending the same status ping after another stall—matches the completed receipt from the first send. messageAgent then returns deduped: true without enqueuing anything, silently dropping valid coordination messages for the rest of the run. Include an invocation/turn identifier in the default key or require callers to supply an idempotency key that distinguishes intentional sends.
Useful? React with 👍 / 👎.
| ...(followup.scheduledWorkId?.trim() | ||
| ? { scheduledWorkId: followup.scheduledWorkId.trim() } | ||
| : {}), | ||
| createdAt: nowIso(), | ||
| status: followup.status ?? "scheduled", |
There was a problem hiding this comment.
When the caller omits scheduledWorkId and status—both allowed by the tool schema—the service records status: "scheduled" even though no chat.createScheduledWork job is proven to exist. If the model skips or fails the preceding CLI action, the manifest falsely reports a durable follow-up and the promised check never fires; require the scheduler ID before accepting scheduled, or default such records to pending. This scheduling invariant should be enforced here rather than only in the tool prompt.
AGENTS.md reference: AGENTS.md:L63-L67
Useful? React with 👍 / 👎.
| // Idempotency and delivery state are authoritative service-owned records. | ||
| // Tool implementations write them through mutex-protected directPatch APIs. | ||
| "/receipts", | ||
| "/receipts/**", | ||
| "/outbox", | ||
| "/outbox/**", |
There was a problem hiding this comment.
Although this deny list protects receipts and outbox rows, a lead can still use manifestPatch to write the newly added service-owned /agents/{sessionId:*}/spawnRequestId and /stalled fields because lead access is deny-list based. A forged spawn key can make reserveReceipt reconcile a future spawn to the wrong existing session, while clearing stalled defeats notification deduplication; add these agent paths to the service-owned deny rules.
AGENTS.md reference: AGENTS.md:L63-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 13971-13983: The orchestrationChatHandle adapters for
createSession, sendMessage, and steer currently bypass type checking with as
never. Align OrchestrationAgentChatHandle’s corresponding argument types with
AgentChatCreateArgs, AgentChatSendArgs, and AgentChatSteerArgs, then call the
service methods directly without casts while leaving the other handle methods
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59be7cf3-c078-40c9-baa9-a4757de11a93
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**
📒 Files selected for processing (30)
apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/orchestrationOutbox.test.tsapps/desktop/src/main/services/ai/tools/orchestrationOutbox.tsapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.tsapps/desktop/src/main/services/ai/tools/orchestrationRuntime.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.test.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.tsapps/desktop/src/main/services/ai/tools/orchestrationValidationDerivation.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/deeplinks/protocolHandler.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationDomain.test.tsapps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.test.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/main/services/orchestration/runtimeProfile.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/orchestration/EvidenceSection.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.test.tsapps/desktop/src/renderer/components/orchestration/orchestrationEvidence.tsapps/desktop/src/renderer/lib/openExternal.tsapps/desktop/src/shared/deeplinks.tsapps/desktop/src/shared/types/orchestration.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/desktop/src/renderer/components/orchestration/orchestrationEvidence.test.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.ts
- apps/desktop/src/main/services/orchestration/patchPolicy.test.ts
- apps/desktop/src/main/services/ai/tools/orchestrationValidationDerivation.ts
- apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md
- apps/desktop/src/renderer/components/orchestration/EvidenceSection.tsx
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx
- apps/desktop/src/renderer/components/orchestration/orchestrationEvidence.ts
- apps/desktop/src/main/services/orchestration/orchestrationDomain.test.ts
- apps/desktop/src/shared/types/orchestration.ts
- apps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.ts
- apps/desktop/src/main/services/orchestration/manifestNormalization.ts
- apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md
- apps/desktop/src/main/services/ai/tools/orchestrationTools.ts
- apps/desktop/src/main/services/orchestration/orchestrationService.ts
…tatus accuracy, service-owned field protection, typed chat handle - messageAgent: salt the derived (content-fingerprint) requestId with an epoch counting prior committed sends, so an intentional repeat after a delivered send reserves fresh instead of dedup-dropping. Fresh send / transient retry / intentional-repeat paths each covered by tests. Explicit requestId keeps strict dedupe. - recordScheduledFollowup: status "scheduled" now requires a real scheduledWorkId; intent-only records (and unbacked "scheduled") are "pending". - patchPolicy: lead-deny service-owned agent-row fields (/agents/*/spawnRequestId, /stalled, /lastHeartbeatAt); worker SELF heartbeat via the role allow-list is unaffected. - orchestration chat handle: replace `as never` casts with the real AgentChat*Args types; fixed a latent optional-text unsoundness the cast hid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3186742b3d
ℹ️ 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".
| if (!context) return null; | ||
| const orchestrationService = getOrchestrationService?.() ?? null; | ||
| if (!orchestrationService) return null; | ||
| ensureOrchestrationDrainerRegistered(orchestrationService); |
There was a problem hiding this comment.
After an app restart, opening or listing a run can hydrate its manifest before any orchestration agent starts a turn; loadIntoRuntime then tries to fire the activation drain while no drainer is registered. Registration happens only here while constructing an orchestration runtime tool map, so an idle lead or worker with a persisted brief/completion message may never receive it until another turn starts, defeating crash-resilient delivery. Register the drainer when the orchestration service becomes available rather than waiting for tool-map construction.
Useful? React with 👍 / 👎.
| // Heartbeat auto-recovery: flag agents that have gone silent past the stall | ||
| // threshold and notify the lead once. Piggybacks this existing sweep — no | ||
| // new interval/timer. Stall flags are orthogonal to the lease/edge ops above | ||
| // (they touch /stalled + /outbox only), so evaluating against `manifest` is | ||
| // accurate. | ||
| const stall = buildStallDetectionOps(runtime, manifest, nowMs); |
There was a problem hiding this comment.
When a running worker simply goes silent, no heartbeat or other event occurs to execute this code, and this stall check is only called from the lead-invoked releaseStaleClaims/recoverStaleTasks path. Consequently the advertised automatic stalled flag and one-time lead notification never appear unless the lead already suspects a stall and manually invokes recovery. Run this check from an existing lifecycle scheduler or otherwise arrange a wake-up at the liveness threshold.
Useful? React with 👍 / 👎.
| inputSchema: z.object({ | ||
| target: z.record(z.string(), z.unknown()), | ||
| }), |
There was a problem hiding this comment.
For model inputs with an unsupported kind or missing kind-specific fields, this unconstrained record passes validation and is later cast to DeeplinkTarget. The builders then either return undefined for an unknown kind or produce malformed URLs containing values such as undefined, while mintDeeplink still reports success; those bad links can subsequently be recorded as evidence. Use a discriminated union matching every DeeplinkTarget variant so malformed targets are rejected at the tool boundary.
Useful? React with 👍 / 👎.
…timer, discriminated deeplink schema Round-5 codex review fixes on PR #878: 1. Outbox drainer registration ordering (P1, agentChatService.ts:14009): the chat-backed drainer was only registered lazily when an orchestration turn first built its tool map, so a run hydrated on boot (opened/listed) with no turn could never deliver a persisted brief/ping after a restart. Register it eagerly in main.ts as soon as the orchestration service exists, and make loadIntoRuntime queue drains when no drainer is registered yet, flushing them on registration (resident sweep + evicted-run queue) so the hydration-vs-registration order is irrelevant. Delivery stays exactly-once (coalesced per run). 2. Self-arming stall detection (P1, orchestrationService.ts:3519): stall flagging only ran inside the lead-invoked releaseStaleClaims path, so a worker that simply went silent was never flagged unless the lead already suspected it. Mirror the outbox deferred-retry timer: while a run has running, not-yet-flagged non-lead agents, arm ONE coalesced unref'd timer for the soonest stall horizon (liveness + threshold), re-armed on hydration, every manifest commit, and each heartbeat, and cleared on run teardown. Timer fires the existing buildStallDetectionOps sweep for that run only. Armed only while workers are running — not a poll loop. 3. Discriminated deeplink schema (P2, orchestrationTools.ts:2452): mintDeeplink accepted a loose record, so an unsupported kind or a target missing kind-specific fields passed validation and produced undefined/malformed URLs while still reporting success. Replace with a zod discriminated union over every DeeplinkTarget variant so malformed targets are rejected at the tool boundary. Tests: stall auto-flag with no manual sweep, no-timer-without-running-agents, timer-cleared-on-teardown; drainer resident + evicted-queue delivery; deeplink schema accept/reject matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74ebe48be0
ℹ️ 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".
| await directPatch( | ||
| runtime, | ||
| stall.ops, | ||
| `stall sweep${stall.flagged ? `: flag ${stall.flagged} stalled agent(s)` : ""}`, | ||
| ); |
There was a problem hiding this comment.
When a worker becomes silent without any other orchestration activity, this timer path persists the lead_status outbox entry but never invokes the registered activation drainer; activation draining only occurs during cold hydration, while tool-driven mutations call drainOutbox explicitly. The worker is therefore marked stalled, but the promised notification remains pending indefinitely until an unrelated tool call or restart, defeating automatic stall detection in exactly the idle scenario it handles.
Useful? React with 👍 / 👎.
| if (!planning.lightPlan) { | ||
| const skipped = effectiveSkippedRounds(manifest, planning); |
There was a problem hiding this comment.
When the lead omits or fails chooseFinishingMode, both the full and light paths can still reach ready and be approved because planningReadinessMissing never checks manifest.finishing. This leaves the mandatory user-facing finishing decision enforced only by prompt instructions and silently falls back to worktree, so a run can finish without ever asking whether the user wanted a PR; add the finishing decision to the approval gate.
AGENTS.md reference: AGENTS.md:L67-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional — skipping the code gate here (documented rationale).
The worktree-by-default behavior is a deliberate design decision from the plan phase, not an oversight:
- The finishing decision is asked, in-band, during planning (
chooseFinishingMode, §3 step 6a of the ade-orchestrator skill). It's enforced by prompt because it's a plain-language user question, not a structural precondition. - An unrecorded
manifest.finishingdeliberately falls back to the safe mode (worktree— leave the branch alone, no push/PR). The failure mode of an unanswered choice is therefore conservative, not destructive: the worst case is the user has to ask for a PR afterward, never an unwanted push. - Adding
manifest.finishingtoplanningReadinessMissingwould gate every run — including the condensed light path whose entire point is fewer mandatory questions — behind an extra hard block, contradicting the lighter-plan goal, and would invalidate the existing approval-gate test suite (587 approval tests assume finishing is not a readiness precondition).
I considered a courtesy non-blocking warn (full path only) but planningReadinessMissing is a pure hard-gate (missing[] → block); there's no non-blocking channel to surface a warning without either gating or plumbing a new advisory path through the approval flow and its tests — not worth the churn for a safe default.
I did add a clarifying sentence to the skill's finishing section making the intentional non-gating explicit ("approval is not blocked on this choice … you must actually ask chooseFinishingMode; do not lean on the default to skip it") so the default can't be used as an excuse to skip the question.
| const completeRes = await svc.completeReceipt( | ||
| ctx.runId, | ||
| ctx.bundlePath, | ||
| { requestId, result: { sessionId: created.id } }, | ||
| [patch, ...briefOps], |
There was a problem hiding this comment.
If the process exits after chat.createSession succeeds but before this completeReceipt transaction commits, the durable chat session exists but neither the agent row nor completed receipt does. After the pending receipt TTL expires, reconciliation only searches manifest agent rows, so retrying the same deterministic request creates a second session and leaves the first orphaned; persist a recoverable request identifier with the session or reconcile session metadata before reserving a replacement.
Useful? React with 👍 / 👎.
…an sessions on receipt reconcile Stall timer: after the self-arming stall sweep commits its lead_status outbox entry, fire the run's registered activation drainer so the notification is delivered now instead of sitting pending until an unrelated mutation/restart — the idle case the timer exists for. Spawn receipt: stamp the created sessionId onto the pending receipt right after chat.createSession and before the completion patch. A crash in that window leaves the session recoverable: reserveReceipt's stale-pending path now returns an adoptSessionId and spawnAgent adopts the existing session (append agent row + deliver brief + complete receipt) instead of spawning a twin and orphaning the first. recoverStaleTasks preserves stamped pending receipts so the recovery link survives its reap. Finishing gate (#2): documented-skip — worktree-by-default is intentional (safe fallback; light path stays ungated). Added a clarifying sentence to the ade-orchestrator skill; no approval gate change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08591c4419
ℹ️ 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".
| description: | ||
| "Record a durable follow-up scheduled for after the run (e.g. 're-check CI in 30m'). " + | ||
| "Do the actual scheduling with `ade actions run chat.createScheduledWork` from a shell, then record it here with the returned scheduledWorkId so the bundle owns the durable intent (manifest.scheduledFollowups).", |
There was a problem hiding this comment.
For every non-lead session, buildOrchestrationSandboxConfig enables blockByDefault, while the safe-command list contains neither ade nor the git push/gh commands needed by a finishing worker. Consequently the ade actions run chat.createScheduledWork command prescribed here is always rejected by checkWorkerSandbox, and the newly advertised PR finishing mode cannot push or open a PR either. Provide narrowly scoped service-backed tools or sandbox allowances before directing workers to perform these operations.
Useful? React with 👍 / 👎.
| if (!Array.isArray(manifest.scheduledFollowups)) { | ||
| ops.push({ op: "add", path: "/scheduledFollowups", value: [entry] }); | ||
| } else { | ||
| ops.push({ op: "add", path: "/scheduledFollowups/-", value: entry }); |
There was a problem hiding this comment.
When a follow-up is first recorded as pending and later becomes scheduled, fired, or cancelled, this path always appends a new entry rather than updating the existing lifecycle record. The public tool does not expose the service's optional id either, so callers cannot identify the original row; it remains permanently pending while unrelated duplicate rows accumulate. Accept a stable follow-up ID and replace/upsert the matching entry for lifecycle transitions.
Useful? React with 👍 / 👎.
…lowups upsert (ADE-131) (#885) FIX 1 — Finishing-command sandbox allowlist. buildOrchestrationSandboxConfig now accepts { allowFinishingCommands } and, when set, adds a narrow safe list (git push / gh pr / ade) so a finishing worker (manifest.finishing.mode "pr") can push, open the PR, and drive the ade CLI under blockByDefault. Scoped at the call site to orchestrator-worker sessions only (never validators); the finishing role isn't tagged separately at sandbox-build time. Note: real finishing workers run a native provider whose shell bypasses this TS sandbox (SKILL §4.5), so this only governs the ADE-SDK bash-tool path — fixed for consistency. FIX 2 — scheduledFollowups upsert. recordScheduledFollowup now upserts by id: re-recording an existing id merges onto the same lifecycle row (arming stamps scheduledWorkId + "scheduled" in place) instead of appending a duplicate, preserves createdAt, and never regresses a terminal (fired/cancelled) status. The tool exposes/echoes the follow-up id so callers can target the same row. References PR #878 bookkept comments 3635693954 / 3635693962. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
ade://deeplink routing and expanded evidence types (including proof artifacts).Greptile Summary
This PR extends orchestration with a condensed planning lifecycle and more durable event-driven delegation.
light_planstage, resolving the previously reported planning deadlock.Confidence Score: 5/5
The PR appears safe to merge because the light-plan model-selection deadlock is resolved and no additional blocking failure remains.
The condensed planning path now admits model selection at
light_plan, retains validation and routing requirements, and reaches approval without the circular readiness dependency.What T-Rex did
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Intake[Codebase intake] --> Choice{Planning path} Choice -->|Full| Rounds[Planning rounds] Choice -->|Light| Light[Condensed plan] Rounds --> Models[Model selection] Light --> Models Models --> Approval[Plan approval] Approval --> Delegation[Idempotent delegation] Delegation --> Outbox[Transactional outbox] Outbox --> Completion[Event-driven completion] Completion --> Validation[Validation and evidence] Validation --> Finish{Finishing mode} Finish -->|Worktree| Done[Validated worktree] Finish -->|PR| PR[Push and open/update PR]Reviews (8): Last reviewed commit: "fix(orchestration): drain stall notifica..." | Re-trigger Greptile