Orchestration Planning Ux#605
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughReplaces summary-based plan quality gating with a deterministic, server-enforced planning state machine for the orchestrator lead role. Adds intake → three deliberation rounds → validation derivation → model selection → hash-gated approval flow backed by new service mutations, manifest normalization, and patch policies. Extends validation with structured ChangesDeterministic Planning State Machine and UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
3ff91f2 to
250aaf6
Compare
|
@codex review |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
250aaf6 to
bc5e8ed
Compare
|
@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: 250aaf67d0
ℹ️ 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 SECTION_HEADING_PATTERNS: Record<PlanSpecSectionId, RegExp> = { | ||
| goal: /goal|objective|summary/i, |
There was a problem hiding this comment.
Because this matcher is run against every heading, a heading like Out of scope / non-goals or Non-goals satisfies the required goal section before the out-of-scope check runs. In a plan that omits a real Goal heading but includes non-goals, assessPlanReadiness will not report goal as missing, so requestPlanApproval can approve a plan without the Goal section this gate requires. Please anchor or word-bound this pattern so negative-goal headings do not count as the plan goal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0732dff: the goal heading matcher now only accepts goal/objective/summary headings at the start of the heading, and the regression covers Out of scope / non-goals no longer satisfying the required Goal section.
| }; | ||
| } | ||
| const selectedOptionIds = result.answers?.[questionId] ?? []; | ||
| const freeText = (result.responseText ?? result.answer ?? "").trim(); |
There was a problem hiding this comment.
When the user picks an option without typing extra text, the ask-user bridge still returns answer as the first selected option value, while the actual selections are already in answers[questionId]. Falling back to result.answer here stores that option id as freeText, so normal planning rounds render/log answers like Safe path — safe even though the user did not enter free-form text. Please only populate freeText from responseText (or otherwise ignore answer when structured answers exist).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0732dff: askPlanningRound now stores freeText from responseText, while structured option selections stay in selectedOptionIds; added a regression for the ask-user bridge returning answer plus answers.
![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/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift (1)
1429-1450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset selection state when the pending request changes
Line 1429-Line 1450 only set defaults at init-time. In SwiftUI,
@Statepersists for the same view identity, so a later model-selection request can inherit stale selection and submit the wrong model.Proposed fix
struct WorkModelSelectionPendingCard: View { @@ `@State` private var selectedModel: WorkModelOption? `@State` private var pickerPresented = false + + private var requestResetKey: String { + [ + request.title, + request.workDescription ?? "", + request.filesHint.joined(separator: "\u{1F}"), + request.dependsOn.joined(separator: "\u{1F}") + ].joined(separator: "\u{1E}") + } @@ .sheet(isPresented: $pickerPresented) { @@ .environmentObject(syncService) } + .onChange(of: requestResetKey) { _, _ in + selectedModelId = "" + selectedProvider = "claude" + selectedReasoningEffort = "" + selectedCodexFastMode = false + selectedModel = nil + pickerPresented = false + } }🤖 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/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift` around lines 1429 - 1450, The init method initializes the `@State` variables (selectedModelId, selectedProvider, selectedReasoningEffort, selectedCodexFastMode, selectedModel) only at view creation time. Since `@State` persists across view identity, when the request parameter changes to a new WorkPendingModelSelectionModel, these state variables retain stale values from the previous selection. Add a .onChange modifier that watches the request parameter and resets all five `@State` variables back to their initial values (empty string for selectedModelId, "claude" for selectedProvider, empty string for selectedReasoningEffort, false for selectedCodexFastMode, and nil for selectedModel) whenever the request changes.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx (1)
453-459: ⚡ Quick winAdd an explicit accessible name for each roster “Open chat” button.
All roster buttons currently expose the same visible label (“Open chat”), which is ambiguous for assistive-tech navigation and encourages order-coupled test selectors. Add an
aria-labelwith agent context.Suggested change
<button type="button" data-testid="orchestration-roster-open-chat" + aria-label={`Open ${agent.role}${agent.tag ? ` · ${agent.tag}` : ""} chat`} onClick={() => onOpenSession?.(agent.sessionId)} className="inline-flex items-center gap-1 rounded-md border border-sky-300/30 bg-sky-300/10 px-1.5 py-0.5 text-[10px] font-medium text-sky-100 transition-colors hover:bg-sky-300/[0.18]" title={`Open ${agent.role}${agent.tag ? ` · ${agent.tag}` : ""} chat`} >🤖 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/orchestration/OrchestrationPanel.tsx` around lines 453 - 459, The roster "Open chat" button in the OrchestrationPanel component lacks an accessible name for assistive technologies, making all buttons appear identical to screen readers. Add an aria-label attribute to the button element (the one with data-testid="orchestration-roster-open-chat" and onClick handler for onOpenSession) that includes the agent context such as agent.role and agent.tag, similar to the existing title attribute pattern, to provide unique and descriptive accessible names for each button.
🤖 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/tuiClient/components/ApprovalPrompt.tsx`:
- Around line 53-56: The stringListValue function filters entries based on
trimmed content length but returns the original untrimmed strings. To fix this,
apply the trim() operation to the entries being returned from the filter, not
just in the condition check. Modify the filter logic in the Array.isArray branch
so that each string entry is trimmed before being included in the final array.
In `@apps/desktop/src/main/services/ai/tools/orchestrationTools.ts`:
- Around line 787-793: The await call to svc.setPlanApprovalState with
changes_requested state ignores its result and continues execution regardless of
success or failure. If this persistence operation fails, the function still
returns plan_rejected while the approval state remains inconsistent. Add error
handling after the await to check if setPlanApprovalState succeeded, and throw
an error or handle the failure appropriately before returning the plan_rejected
result to ensure the approval state and hash persist correctly.
- Around line 1287-1306: The askPlanningRound function prompts the user via
onAskUser before validating that the requested input.kind is unlocked or allowed
for the current planning stage. Add validation after checking
manifest.currentPhase is "planning" and before calling universalOpts.onAskUser
to confirm that input.kind is a valid and unlocked round for the manifest's
current stage. This validation should return an error response if the kind is
out-of-order or not yet unlocked, preventing users from answering questions that
the service would later reject.
In `@apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx`:
- Around line 52-55: Add a useEffect hook that monitors for changes to the
model-selection request (likely through a request ID or role/tag prop) and
resets the state variables modelId, setModelId, provider, setProvider,
reasoningEffort, setReasoningEffort, and fastMode, setFastMode back to their
initial values whenever a new request arrives. This prevents stale values from
the previous request from persisting when the component is reused with a
different model-selection request without unmounting.
In `@apps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsx`:
- Around line 64-83: The test "renders the agent briefing and starts with no
model pre-selected" is incomplete as it verifies the filesHint rendering but
does not verify that the dependsOn (Runs after) section is rendered, even though
the metadata fixture includes dependsOn data. Add two assertions after the
existing filesHint verification in the test to confirm that both the "Runs
after" text and the "backend" value from the dependsOn array are rendered on the
screen. This ensures the test fully validates all briefing information displayed
by the ChatModelSelectionPendingCard component.
---
Outside diff comments:
In `@apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift`:
- Around line 1429-1450: The init method initializes the `@State` variables
(selectedModelId, selectedProvider, selectedReasoningEffort,
selectedCodexFastMode, selectedModel) only at view creation time. Since `@State`
persists across view identity, when the request parameter changes to a new
WorkPendingModelSelectionModel, these state variables retain stale values from
the previous selection. Add a .onChange modifier that watches the request
parameter and resets all five `@State` variables back to their initial values
(empty string for selectedModelId, "claude" for selectedProvider, empty string
for selectedReasoningEffort, false for selectedCodexFastMode, and nil for
selectedModel) whenever the request changes.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx`:
- Around line 453-459: The roster "Open chat" button in the OrchestrationPanel
component lacks an accessible name for assistive technologies, making all
buttons appear identical to screen readers. Add an aria-label attribute to the
button element (the one with data-testid="orchestration-roster-open-chat" and
onClick handler for onOpenSession) that includes the agent context such as
agent.role and agent.tag, similar to the existing title attribute pattern, to
provide unique and descriptive accessible names for each button.
🪄 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: 0135f553-e5c4-4e3e-8717-49f4c87dfef7
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (31)
apps/ade-cli/src/tuiClient/__tests__/ApprovalPrompt.test.tsxapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.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/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/main/services/orchestration/runtimeProfile.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsxapps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/PanelChrome.tsxapps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsxapps/desktop/src/renderer/components/orchestration/PlanningTimeline.tsxapps/desktop/src/renderer/components/orchestration/TaskCard.tsxapps/desktop/src/renderer/components/orchestration/ValidationFindings.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/shared/types/orchestration.tsapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADETests/ADETests.swift
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
bc5e8ed to
0732dff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc5e8eda34
ℹ️ 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".
| for (const line of lines) { | ||
| const m = /^(#{1,6})\s+(.*)$/.exec(line); | ||
| if (m) { | ||
| if (current) out.push(current); |
There was a problem hiding this comment.
When a required plan section is organized with subsections, such as ## Agent plan followed immediately by ### Worker / ### Validator, this parser closes the parent section as soon as it sees the child heading. assessPlanReadiness then sees an empty or very thin parent body and blocks approval even though the nested subsection content is substantive. Aggregate lower-level headings into the current parent section, or otherwise account for child content before applying the body-length gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa5f100: parsePlanSections now keeps child subsection headings/content inside the parent section body, with a regression for an Agent plan made only of Worker/Validator subsections.
| const selectedOptionIds = result.answers?.[questionId] ?? []; | ||
| const freeText = (result.responseText ?? result.answer ?? "").trim(); |
There was a problem hiding this comment.
For structured planning questions answered by selecting an option, the ask-user bridge returns both answers[questionId] and answer populated from the first selected value. Falling back to result.answer here records that option as freeText, so an option-only response is stored as both selectedOptionIds and free text (and mixed option+freeform responses cannot be separated correctly). Split returned answer values against the offered option ids, and only store actual freeform text as freeText.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa5f100: option selections stay in selectedOptionIds and freeText only comes from explicit responseText for structured option questions; covered by the planning-round regression.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0732dfff94
ℹ️ 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".
| providerMetadata: { | ||
| orchestrationPlanApproval: true, | ||
| planContent: input.planSummary, | ||
| planQuality: quality, | ||
| planContentHash, | ||
| }, |
There was a problem hiding this comment.
When a user approves from iOS or the ADE CLI/TUI, there is no desktop sidebar plan narrative to review; those clients still derive the review body from the pending-input payload (for example, iOS pendingWorkPlanApprovalFromApproval looks for providerMetadata.planContent and otherwise falls back to the request text, and the TUI has no plan pane). This new payload sends only planContentHash, so those clients show a generic “Review the plan on the sidebar...” prompt while the accept action still approves the real plan, causing blind approvals outside the desktop sidebar flow. Keep the plan text (or a client-readable plan reference) in the approval metadata until those clients can render the live plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 52c268b: requestPlanApproval now includes the exact live plan.md content in providerMetadata.planContent alongside planContentHash, so iOS/TUI clients can review the same plan bytes without relying on the desktop sidebar. Added an orchestrationTools regression and reran shard 4/typecheck.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
0732dff to
fa5f100
Compare
|
@codex review |
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
fa5f100 to
17b1c76
Compare
|
Addressed the remaining CodeRabbit outside-diff/nitpick items in 17b1c76: iOS WorkModelSelectionPendingCard now resets selection state when the pending model-selection request changes, and roster Open chat buttons now have agent-specific aria-labels. Verified with focused OrchestrationPanel tests, desktop typecheck, and iOS build-for-testing.\n\n@codex review |
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
17b1c76 to
52c268b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c268b352
ℹ️ 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".
| ...(override.skipReason?.trim() ? { skipReason: override.skipReason.trim() } : {}), | ||
| }; | ||
| const ops: ManifestPatchOp[] = [ | ||
| { op: "replace", path: "/leadState/planning/overrides", value }, |
There was a problem hiding this comment.
This writes planning.overrides.skippedRounds directly from the tool input without verifying or appending a matching UserOverrideEntry; markPlanningReady later treats those skipped rounds as satisfied, so a lead can bypass the functional/UI/extras gates even when the user never waived them. The existing validation-gate bypass requires a human override plus a user-override entry in one transaction; the planning-round bypass needs the same server-side proof instead of relying on the prompt text to do a separate manifest patch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0f68e1. recordPlanningOverride now requires a literal skipReason for skipped rounds, appends matching UserOverrideEntry records atomically with affectedDefault=planning.round., and markPlanningReady only honors skipped rounds when those entries still exist. Added regressions for the normal skip path and an orphaned skipped-round state with deleted override entries. Verified with targeted orchestration tests, desktop typecheck, shard 4, and lint.
| message: "User dismissed the deliberation round without answering.", | ||
| }; | ||
| } | ||
| const selectedOptionIds = result.answers?.[questionId] ?? []; |
There was a problem hiding this comment.
For an options-based planning round with freeform enabled, the inline question UI submits both selected option values and typed freeform text in the same answers array; this line stores every value as selectedOptionIds, so custom text becomes a bogus option id and freeText stays empty. In that scenario the manifest loses the distinction between the user's selected option and their extra constraints, and the planning timeline/model context can render the typed text as if it were an option id; filter values against input.options and store unmatched values as free text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0f68e1. askPlanningRound now filters returned answer values against the offered option IDs and stores unmatched typed values as freeText, combined with responseText when present. Added a regression for mixed selected option + custom text. Verified with orchestrationTools.test.ts, desktop typecheck, shard 4, and lint.
| const validationWaived = manifest.userOverrides.some((o) => | ||
| /(?:skip|no|without)\s+validation/i.test(o.instruction), | ||
| ); |
There was a problem hiding this comment.
Because this regex scans literal user override text, instructions like “do not skip validation” or “no validation shortcuts” still match skip validation / no validation, which lets planning be marked ready with zero validation steps even though the user explicitly asked not to waive validation. Use an explicit override shape or stricter positive phrases so negated instructions cannot disable the validation gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0f68e1. Validation waivers now use a shared explicit helper that rejects negated phrasing like "do not skip validation" / "no validation shortcuts" while preserving direct waivers like "no validation for this run". The service and plan-quality gate both use it now, with regressions for the negated and positive cases. Verified with targeted orchestration tests, desktop typecheck, shard 4, and lint.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
52c268b to
f0f68e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/orchestrationPlanQuality.ts`:
- Around line 4-8: The documentation comment block references
`assessOrchestrationPlanQuality` as a retained legacy gate, but this function no
longer exists in the file. Remove the sentence in the comment that mentions this
non-existent function being retained for legacy callers, specifically the part
referencing the old regex-over-summary gate and `assessOrchestrationPlanQuality`
to keep the documentation accurate with the current codebase.
- Around line 136-151: The logic for finding matching sections uses
sections.find() which returns only the first heading that matches the pattern,
causing validation to fail if an earlier thin heading (like "Validation
findings") matches before a later, more substantial heading (like "Validation /
proof plan"). Instead of using find() to get a single match, retrieve all
sections that match the pattern and validate that at least one of them meets the
content requirements, or select the most appropriate match before checking the
body length against MIN_BODY_CHARS.
- Around line 51-53: The markdown parser in the for loop iterating through lines
does not account for code fences, so headings inside fenced code blocks are
incorrectly treated as real plan sections. Add a fence detection check before
the heading regex by introducing an inFence boolean variable that tracks whether
the parser is currently inside a markdown code fence (detected by lines matching
a fence pattern like ~~~ or ```). When a fence marker is detected, toggle the
inFence state, append the line to any openSections, and continue to the next
iteration. Only execute the heading regex pattern match when inFence is false,
ensuring that markdown headings inside code fences are skipped and treated as
content instead of section headers.
🪄 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: 3946a584-66dd-4b6f-91b4-829148fd9e27
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (32)
apps/ade-cli/src/tuiClient/__tests__/ApprovalPrompt.test.tsxapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.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.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/main/services/orchestration/runtimeProfile.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsxapps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/PanelChrome.tsxapps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsxapps/desktop/src/renderer/components/orchestration/PlanningTimeline.tsxapps/desktop/src/renderer/components/orchestration/TaskCard.tsxapps/desktop/src/renderer/components/orchestration/ValidationFindings.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/shared/types/orchestration.tsapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADETests/ADETests.swift
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/orchestration/TaskCard.tsx
🚧 Files skipped from review as they are similar to previous changes (27)
- apps/desktop/src/main/services/orchestration/patchPolicy.ts
- apps/ade-cli/src/tuiClient/tests/ApprovalPrompt.test.tsx
- apps/desktop/src/main/services/orchestration/runtimeProfile.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift
- apps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsx
- apps/desktop/src/renderer/components/orchestration/PanelChrome.tsx
- apps/ios/ADE/Views/Work/WorkModels.swift
- apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsx
- apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx
- apps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.ts
- apps/desktop/src/renderer/components/orchestration/PlanningTimeline.tsx
- apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md
- apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- apps/desktop/src/main/services/ai/tools/orchestrationValidationDerivation.ts
- apps/desktop/src/main/services/orchestration/manifestNormalization.ts
- apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx
- apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx
- apps/desktop/src/main/services/ai/tools/orchestrationTools.test.ts
- apps/desktop/src/shared/types/orchestration.ts
- apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift
- apps/desktop/src/main/services/ai/tools/orchestrationTools.ts
- apps/desktop/src/main/services/orchestration/orchestrationService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8591e316e
ℹ️ 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".
| return ( | ||
| Object.keys(manifest.modelRouting.byRoleTag ?? {}).length > 0 || | ||
| Object.keys(manifest.modelRouting.byTag ?? {}).length > 0 || | ||
| Boolean(manifest.modelRouting.default) |
There was a problem hiding this comment.
When a lead configures a valid route in modelRouting.byRole (which resolveOrchestrationModel already honors), the new readiness gate still reports missing model routing because this helper only checks byRoleTag, byTag, and default. That blocks markPlanningReady/approval even though spawning would resolve the model correctly; include non-empty manifest.modelRouting.byRole in this predicate.
Useful? React with 👍 / 👎.
| ...(args.planContentHash | ||
| ? [{ op: "replace" as const, path: "/planSpec/approval/approvedPlanContentHash", value: args.planContentHash }] | ||
| : []), |
There was a problem hiding this comment.
This records the hash captured before the pending approval was shown, but it never checks that the current plan.md still hashes to that value before advancing to developing. If the plan file changes while the approval is pending (the service watches and reloads external plan edits), the user can approve older bytes while the run proceeds with unreviewed plan content; compare runtime.planMd to args.planContentHash in the approved branch and require a fresh approval on mismatch.
Useful? React with 👍 / 👎.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
f8591e3 to
c24be82
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24be823c3
ℹ️ 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 approvalRes = await svc.setPlanApprovalState(ctx.runId, ctx.bundlePath, { | ||
| state: "approved", | ||
| sessionId: ctx.sessionId, | ||
| planContentHash, | ||
| }); |
There was a problem hiding this comment.
When plan.md is edited while the approval prompt is pending (for example by a concurrent planWrite/planAppend, which does not bump the manifest etag), this records approval with the hash computed before onAskUser returned and advances the current run to developing. That lets late plan edits bypass reapproval and leaves approvedPlanContentHash pointing at bytes that no longer match the plan the run will execute; re-read the bundle and compare the hash immediately before setting approved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b7dfb6b. requestPlanApproval now re-reads plan.md after an accept only to compare the current hash with the exact hash displayed in the pending approval; if it changed, ADE records changes_requested against the reviewed hash and returns plan_changed_after_review so the updated plan must be reviewed again. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
c24be82 to
b7dfb6b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7dfb6b96a
ℹ️ 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 nextStage: PlanningStage = | ||
| round.kind === "functional" | ||
| ? "round_ui" | ||
| : round.kind === "ui" | ||
| ? "round_extras" | ||
| : "rounds_complete"; | ||
| ops.push({ op: "replace", path: "/leadState/planning/stage", value: nextStage }); |
There was a problem hiding this comment.
When a user waives a later round before the current one is recorded (for example, after intake they say to skip the UI round while the stage is still round_functional), recordPlanningOverride stores the skip but does not advance because the current stage is not skipped yet; this transition then unconditionally moves the completed functional round to round_ui. The run is left stuck on a skipped round, so askPlanningRound('extras') and model selection remain locked unless the same override is recorded again. Advance through already-skipped stages here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b920536. recordPlanningRound now advances its next stage through already-skipped rounds using the same helper as recordPlanningOverride, so a prior UI/extras skip no longer leaves the lead stuck on round_ui after functional is recorded. Added a regression for pre-skipped UI+extras advancing to rounds_complete. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
| const NEGATED_VALIDATION_WAIVER_RE = | ||
| /\b(?:do\s+not|don't|dont|never)\s+(?:skip|waive|bypass)\s+(?:the\s+)?validation\b|\bno\s+validation\s+(?:shortcuts?|skips?|waivers?|bypasses?)\b|\bnot\s+without\s+(?:running\s+)?validation\b/i; | ||
| const POSITIVE_VALIDATION_WAIVER_RE = | ||
| /\b(?:skip|waive|bypass)\s+(?:the\s+)?validation\b|\bno\s+validation\b(?!\s+(?:shortcuts?|skips?|waivers?|bypasses?))|\bwithout\s+(?:running\s+)?validation\b|\bvalidation\s+(?:is\s+)?(?:waived|skipped|not\s+required|unnecessary|unneeded)\b/i; |
There was a problem hiding this comment.
For literal override text such as skip the UI round; no validation changes, this pattern matches no validation and causes recordPlanningOverride to record a validation waiver; markPlanningReady will then accept zero validation steps even though the user did not ask to bypass validation. Narrow this to explicit skip/waive/not-required wording, or exclude cases like no validation changes, so incidental wording cannot disable the validation gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b920536. Tightened validation-waiver detection so incidental text like "no validation changes" no longer waives validation; explicit forms such as skip/waive/bypass validation, no validation required/needed/for this run, without running validation, or validation not required still count. Added a regression proving "skip the UI round; no validation changes" skips UI only and still requires validation steps. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
b7dfb6b to
b920536
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b920536254
ℹ️ 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".
| Object.keys(manifest.modelRouting.byRoleTag ?? {}).length > 0 || | ||
| Object.keys(manifest.modelRouting.byTag ?? {}).length > 0 || | ||
| Boolean(manifest.modelRouting.default) |
There was a problem hiding this comment.
The readiness checks use this helper before requestPlanApproval. Because resolveOrchestrationModel already supports modelRouting.byRole, a run that deliberately sets only /modelRouting/byRole/worker or /validator can spawn with that route, but markPlanningReady/assessPlanReadiness still report missing model routing and block approval. Include byRole here so all supported routing forms satisfy the gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93b8b22. hasOrchestrationModelRouting now counts modelRouting.byRole in addition to byRoleTag/byTag/default, matching resolveOrchestrationModel support. Added a planning readiness regression where /modelRouting/byRole/worker alone satisfies the model-routing gate. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
| ...(selectedOptionIds.length ? { selectedOptionIds } : {}), | ||
| ...(freeText ? { freeText } : {}), | ||
| lockedSummary: input.lockedSummary, |
There was a problem hiding this comment.
lockedSummary is supplied in the tool input before onAskUser returns, then recorded unchanged after selectedOptionIds and freeText are known. When the user selects a different option or adds custom scope, the manifest still marks the round complete with a stale or contradictory locked outcome, so later planning/approval gates operate on misleading planning state. Build this summary from the actual response or record only the answered selection here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93b8b22. askPlanningRound now derives lockedSummary from the actual accepted response: selected option labels plus any response/custom free text, with the input lockedSummary only as a fallback. Added regressions for custom text and for a user selecting a different option than the supplied pre-prompt summary. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
b920536 to
93b8b22
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b8b22823
ℹ️ 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 response = await universalOpts.onAskUser({ | ||
| title: "Plan ready", | ||
| body: input.planSummary, | ||
| question: input.question ?? "Review the plan pane. Continue planning in chat if anything should change, or click Implement when ready.", | ||
| ...(input.note ? { body: input.note } : {}), | ||
| question: | ||
| input.question ?? | ||
| "Review the plan on the sidebar. Keep refining in chat if anything should change, or click Implement to start the build.", |
There was a problem hiding this comment.
When requestPlanApproval is used from the ADE CLI/TUI, there is no plan sidebar to review; the TUI ApprovalPrompt renders the request description/questions and does not read providerMetadata.planContent. Because this call now omits body unless an optional note is provided, those clients only see the sidebar-oriented question and can approve without seeing the plan bytes they are approving. Keep the live plan.md in the prompt body/description for non-sidebar clients, or teach the TUI approval prompt to render providerMetadata.planContent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83dfd7a. requestPlanApproval now always includes the live plan.md bytes in the approval prompt body, with any optional note prepended, while still carrying planContent/planContentHash in providerMetadata. Added a regression asserting the approval prompt body contains the plan for non-sidebar clients. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
| allowsNotApplicable && | ||
| body.length < 80 && | ||
| /\b(n\/?a|not applicable|none|no ui|no ux)\b/i.test(body); | ||
| return explicitNa || body.length >= MIN_BODY_CHARS; |
There was a problem hiding this comment.
When a run is created without a goalSummary, initialPlanMd seeds ## Goal with _pending — the lead will fill this in_; this readiness check treats that placeholder as substantive because it is longer than MIN_BODY_CHARS. A lead can fill the other required sections and pass approval while the Goal section still contains only the initial pending text, so the new gate does not actually enforce a real goal in that scenario.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83dfd7a. assessPlanReadiness now rejects the initial _pending — the lead will fill this in_ Goal placeholder even though it is longer than the minimum body threshold. Added a regression for that placeholder. Validated with targeted orchestration tests, desktop typecheck, desktop shard 4, diff check, and desktop lint.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
ade/orchestration-planning-ux-88fff44d
branch
from
93b8b22 to
83dfd7a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83dfd7a841
ℹ️ 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".
| // No recommended/suggested model — the user picks from scratch so they can | ||
| // match the model to what this agent actually does (see the briefing below). | ||
| const [modelId, setModelId] = useState<string>(""); | ||
| const [provider, setProvider] = useState<AgentChatProvider>("claude"); |
There was a problem hiding this comment.
When the model picker is used for an OpenCode-backed/local catalog model (for example ollama/*, lmstudio/*, OpenRouter/API-key families, or a runtime-catalog model not returned by getModelById), handleModelChange falls through and keeps the previous provider. This change now always initializes that provider to claude and no longer passes the suggested route, so the submitted selection can become { provider: "claude", modelId: "ollama/..." }; spawnAgent will then try to launch the wrong runtime for the user's chosen model. Resolve the provider from the picked descriptor the same way the main model switch path does, or keep the suggested provider as the fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9140949. The model picker now resolves the submitted provider from the shared model descriptor, with runtime/model-id prefix fallback for local and OpenCode-backed catalog models, while still starting with no preselected recommendation. Added a regression for ollama/* routing through opencode. Validated with the focused model-picker test, orchestration tests, TUI approval test, desktop/CLI typechecks, diff check, and desktop lint.
| title: "Plan ready", | ||
| body: input.planSummary, | ||
| question: input.question ?? "Review the plan pane. Continue planning in chat if anything should change, or click Implement when ready.", | ||
| body: [input.note?.trim(), bundle.planMd].filter(Boolean).join("\n\n"), |
There was a problem hiding this comment.
For TUI clients there is no plan sidebar, and plan_approval requests are handled as approval-mode cards that render only a single truncated primary line from this body. Since the body is now the entire plan.md while the prompt still tells the user to review the sidebar, approving from the TUI can happen without the user seeing the multi-section plan they are approving. Either keep a concise approval body for non-sidebar clients or update the TUI approval card to render the full providerMetadata.planContent preview.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9140949. The ADE CLI TUI approval card now renders providerMetadata.planContent as a multiline preview for plan_approval requests instead of showing only a truncated primary line. Added a TUI regression for the plan preview. Validated with focused desktop/TUI tests, desktop/CLI typechecks, diff check, and desktop lint.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx (2)
147-167: ⚡ Quick winConsider using "Files it touches" for clarity.
The label "Touches" on Line 150 is terse. The more explicit "Files it touches" would better communicate that these chips show the files this agent will modify or interact with.
✏️ Suggested label improvement
- Touches + Files it touches🤖 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/ChatModelSelectionPendingCard.tsx` around lines 147 - 167, The label "Touches" in the filesHint section is too terse and doesn't clearly communicate what the chips represent. Replace the text "Touches" with "Files it touches" in the span element that displays this label to make it more explicit and help users understand that these chips show the files the agent will modify or interact with.
169-183: 💤 Low valueConsider capping
dependsOnchips for consistency.The
filesHintchips are capped at 6 with an overflow indicator (Lines 161-165), butdependsOnrenders all dependency tags without limit. If an agent has many dependencies, this could cause layout overflow.♻️ Optional: Apply the same capping pattern
{dependsOn.length > 0 ? ( <div className="mb-2.5 flex flex-wrap items-center gap-1.5"> <span className="font-sans text-[length:calc(var(--chat-font-size)*9.5/14)] uppercase tracking-wide text-fg/40"> Runs after </span> - {dependsOn.map((dep) => ( + {dependsOn.slice(0, FILE_CHIP_CAP).map((dep) => ( <span key={dep} className="inline-flex items-center rounded-[var(--chat-radius-pill)] border border-white/[0.07] bg-white/[0.02] px-1.5 py-0.5 font-sans text-[length:calc(var(--chat-font-size)*10/14)] text-fg/62" > {dep} </span> ))} + {dependsOn.length > FILE_CHIP_CAP ? ( + <span className="font-sans text-[length:calc(var(--chat-font-size)*9.5/14)] text-fg/40"> + +{dependsOn.length - FILE_CHIP_CAP} + </span> + ) : null} </div> ) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx` around lines 169 - 183, The dependsOn chips render all dependencies without limit, whereas the filesHint chips are capped at 6 with an overflow indicator. Apply the same capping pattern to the dependsOn rendering block by limiting the mapped items to the first 6 dependencies and adding an overflow indicator when dependsOn.length exceeds 6. Extract the first 6 items using slice(0, 6) and conditionally render an additional chip showing the count of remaining dependencies (similar to the filesHint pattern) when there are more than 6 items.
🤖 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/orchestrationPlanQuality.ts`:
- Around line 50-59: The current fence tracking uses a simple boolean inFence
that toggles on any fence marker (backticks or tildes), which causes incorrect
behavior when fence types are mixed—a tilde fence inside backticks can
incorrectly re-enable heading parsing. Replace the inFence boolean with an
activeFence object that tracks the marker type and length. Update the fence
detection logic to capture the actual marker and length using a pattern like
/^\s{0,3}(`{3,}|~{3,})/, then only toggle the fence state when encountering the
same marker type with sufficient length. Update the heading parsing condition
from checking inFence to checking activeFence instead.
In `@apps/desktop/src/main/services/ai/tools/orchestrationTools.ts`:
- Around line 1454-1458: The fallbackAnswer logic in the orchestrationTools.ts
file incorrectly discards the plain string result.answer whenever input.options
are present. The condition should only exclude result.answer when answerValues
contains structured answers, not based on the presence of options. Modify the
fallbackAnswer assignment to remove the input.options?.length check from the
condition, keeping only the answerValues.length check, so that the plain text
answer is preserved and included in freeText even when options are provided for
option-based planning rounds.
- Around line 824-857: Before the final setPlanApprovalState call with state
"approved", add a manifest readiness check to re-validate that gates like model
routing and validation steps are still present and valid. Currently only the
plan.md content hash is compared, which misses changes to manifest structure
that could have occurred while the approval prompt was open. Re-run the same
validation/planning readiness checks that were performed earlier in the approval
flow immediately before invoking setPlanApprovalState with the approved state,
and return an error if readiness validation fails, so the approval transition
remains atomic and safe.
- Around line 766-794: The orchestrationTools function calls markPlanningReady
to persist approval state before invoking the onAskUser callback. If the
onAskUser call rejects or throws an error, the plan is left in a ready state
without a valid pending prompt. Wrap the universalOpts.onAskUser call in a
try-catch block to handle potential rejections and return an appropriate error
response that prevents leaving the manifest in an inconsistent state when the
user approval prompt fails.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx`:
- Around line 147-167: The label "Touches" in the filesHint section is too terse
and doesn't clearly communicate what the chips represent. Replace the text
"Touches" with "Files it touches" in the span element that displays this label
to make it more explicit and help users understand that these chips show the
files the agent will modify or interact with.
- Around line 169-183: The dependsOn chips render all dependencies without
limit, whereas the filesHint chips are capped at 6 with an overflow indicator.
Apply the same capping pattern to the dependsOn rendering block by limiting the
mapped items to the first 6 dependencies and adding an overflow indicator when
dependsOn.length exceeds 6. Extract the first 6 items using slice(0, 6) and
conditionally render an additional chip showing the count of remaining
dependencies (similar to the filesHint pattern) when there are more than 6
items.
🪄 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: e61e3c61-d940-4d51-86cf-85f87ea8a69b
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (32)
apps/ade-cli/src/tuiClient/__tests__/ApprovalPrompt.test.tsxapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.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.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/main/services/orchestration/runtimeProfile.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsxapps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/PanelChrome.tsxapps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsxapps/desktop/src/renderer/components/orchestration/PlanningTimeline.tsxapps/desktop/src/renderer/components/orchestration/TaskCard.tsxapps/desktop/src/renderer/components/orchestration/ValidationFindings.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/shared/types/orchestration.tsapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (25)
- apps/desktop/src/renderer/components/orchestration/TaskCard.tsx
- apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/ade-cli/src/tuiClient/tests/ApprovalPrompt.test.tsx
- apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts
- apps/desktop/src/main/services/ai/tools/orchestrationValidationDerivation.ts
- apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift
- apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md
- apps/desktop/src/main/services/orchestration/patchPolicy.ts
- apps/ios/ADE/Views/Work/WorkModels.swift
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- apps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsx
- apps/desktop/src/main/services/ai/tools/orchestrationTools.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx
- apps/desktop/src/main/services/orchestration/runtimeProfile.ts
- apps/desktop/src/renderer/components/orchestration/ValidationFindings.tsx
- apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx
- apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsx
- apps/desktop/src/shared/types/orchestration.ts
- apps/desktop/src/renderer/components/orchestration/PlanningTimeline.tsx
- apps/desktop/src/main/services/orchestration/orchestrationPlanningGates.test.ts
- apps/desktop/src/main/services/orchestration/orchestrationService.ts
- apps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsx
- apps/desktop/src/main/services/orchestration/manifestNormalization.ts
|
Fixed in 9140949. Also addressed the CodeRabbit review-body polish items: the model briefing label now says "Files it touches", and Runs after chips use the same cap/overflow treatment as file chips. Validated with focused desktop/TUI tests, desktop and CLI typechecks, diff check, and desktop lint. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9140949898
ℹ️ 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".
| runtime, | ||
| [ | ||
| { op: "replace", path: "/leadState/planning/intake", value: recorded }, | ||
| { op: "replace", path: "/leadState/planning/stage", value: "round_functional" }, |
There was a problem hiding this comment.
When the user gives a skip-round instruction before codebase intake, recordPlanningOverride stores it but cannot advance while the stage is still intake; this transition then unconditionally moves to round_functional without applying the existing skippedRounds. If functional was the waived round, the lead is left at round_functional where asking the UI round is rejected and model selection never unlocks unless the override is recorded a second time. Advance past already-recorded skips here as the round-recording path does.
Useful? React with 👍 / 👎.
| const stale = manifest.tasks.filter( | ||
| (task) => | ||
| (task.status === "claimed" || task.status === "in_progress") && | ||
| typeof task.claimLeaseUntil === "string" && | ||
| Number.isFinite(Date.parse(task.claimLeaseUntil)) && | ||
| Date.parse(task.claimLeaseUntil) <= nowMs, | ||
| ); |
There was a problem hiding this comment.
This recovery path only resets tasks whose claimLeaseUntil is a valid expired string. If a crashed worker leaves a task in claimed or in_progress with a missing, null, or malformed lease, this filter skips it and the task stays assigned forever, so the lead cannot recover the abandoned work through recoverStaleTasks. Treat tasks without a valid future lease as recoverable instead of requiring a parseable expired timestamp.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/orchestration/orchestrationService.ts
Line: 2147-2153
Comment:
**Recover invalid leases**
This recovery path only resets tasks whose `claimLeaseUntil` is a valid expired string. If a crashed worker leaves a task in `claimed` or `in_progress` with a missing, `null`, or malformed lease, this filter skips it and the task stays assigned forever, so the lead cannot recover the abandoned work through `recoverStaleTasks`. Treat tasks without a valid future lease as recoverable instead of requiring a parseable expired timestamp.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Release Notes
Greptile Summary
plan.mdreadiness checks, reviewed-content approval hashing, and richer model-selection briefing metadata.Confidence Score: 3/5
Merge safety is limited by task lifecycle gates that can leave abandoned work unrecovered or allow completion despite blocking validation findings.
The changes are well-scoped and include expanded tests, but the orchestration state-management paths contain likely runtime correctness issues in recovery and validation gating.
apps/desktop/src/main/services/orchestration/orchestrationService.ts; apps/desktop/src/main/services/orchestration/patchPolicy.ts
What T-Rex did
Comments Outside Diff (2)
General comment
filesHintrenders the labelTouches. The validation contract requested the desktop card visibly showFiles it touches(orFilesfor TUI). This may make the UI inconsistent with the intended prompt/card copy even though the metadata is otherwise consumed and truncated.apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx, the file row label text is hard-coded asTouches.Files it touchesif that is the intended UX contract, leaving TUI asFiles.apps/desktop/src/main/services/orchestration/patchPolicy.ts, line 332-334 (link)recordValidationRuncan now attach structured findings, but this gate only checks that the latest run hasstatus === "passed". A validator can record a passed run with ablockerorhighfinding, the Findings panel will still show the blocking issue, andreleaseTask(status: "done")will allow the task to complete. Make the gate reject latest runs with unresolved blocker/high findings, or rejectpassedvalidation runs that contain them.Artifacts
Repro: focused harness for passed validation run with blocker and high findings
Stack trace captured during the T-Rex run
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ship: iteration 13 address review feedba..." | Re-trigger Greptile