queue pr fixes#241
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR introduces a comprehensive "Path to Merge" orchestration system that automates iterative PR convergence through CI/review checks and merge actions. It expands pipeline configuration settings to support conflict strategies, auto-agent behavior, force-finalization, and early merging; persists orchestrator state across restarts; wires convergence controls via IPC/preload/remote commands; enhances queue landing with base retargeting and post-merge cleanup; and updates CLI, UI, and mobile client support accordingly. ChangesPath-to-Merge Convergence & Pipeline Settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/prs/prService.ts (1)
3098-3103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fail
retargetBase()after the PATCH already succeeded.
refreshOne(prId)is post-mutation bookkeeping, but here it decides the method outcome. A transient refresh failure will make callers treat the retarget as failed; inadvanceChildLanesAfterLand()that pushes the child lane intofailedLaneIdsand blocks branch/lane cleanup even though GitHub already changed the PR base.💡 Suggested fix
await githubService.apiRequest({ method: "PATCH", path: `/repos/${repo.owner}/${repo.name}/pulls/${Number(row.github_pr_number)}`, body: { base: baseBranch } }); - await refreshOne(prId); + db.run( + `update pull_requests + set base_branch = ?, updated_at = ? + where id = ? and project_id = ?`, + [baseBranch, nowIso(), prId, projectId], + ); + markHotRefresh([prId]); + await refreshOne(prId).catch((error) => { + logger.warn("prs.retarget_refresh_failed", { + prId, + baseBranch, + error: getErrorMessage(error), + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 3098 - 3103, The PATCH to GitHub (githubService.apiRequest) in retargetBase() already succeeds but a subsequent refreshOne(prId) failure causes the whole operation to be treated as failed; change retargetBase() so the API PATCH result determines success and wrap the refreshOne(prId) call in a try/catch (or otherwise handle errors) so any refresh failure is logged/recorded but does not propagate as an error/throw or change the return value—ensure githubService.apiRequest and retargetBase() remain authoritative for success, while advanceChildLanesAfterLand() and callers see the operation as successful even if refreshOne() failed.apps/desktop/src/main/services/prs/queueLandingService.ts (1)
313-347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive the legacy resolver mirrors from the resolved
pipeline.The new queue contract is
pipeline, but this method still persistsautoResolve/resolverModel/permissionModefrom deprecated args only. Downstream,maybeResolveConflict()reads those mirrors, so a caller that sends onlypipeline.conflictStrategy = "auto"andpipeline.autoAgentSettings.*will still pause on merge conflicts instead of invoking the resolver.Suggested fix
const basePipeline = mergePipelineSettings(DEFAULT_QUEUE_CONFIG.pipeline, prior.pipeline); const pipeline = mergePipelineSettings(basePipeline, argsPipeline); + const autoResolve = pipeline.conflictStrategy === "auto"; return { ...DEFAULT_QUEUE_CONFIG, ...prior, method: args.method ?? prior.method ?? "squash", archiveLane: args.archiveLane ?? prior.archiveLane ?? false, - autoResolve: args.autoResolve ?? prior.autoResolve ?? false, + autoResolve, ciGating: args.ciGating ?? prior.ciGating ?? Boolean(group?.ci_gating), pipeline, - resolverProvider: args.resolverProvider ?? prior.resolverProvider ?? null, - resolverModel: args.resolverModel ?? prior.resolverModel ?? null, - reasoningEffort: args.reasoningEffort ?? prior.reasoningEffort ?? null, - permissionMode: args.permissionMode ?? prior.permissionMode ?? "guarded_edit", - confidenceThreshold: args.confidenceThreshold ?? prior.confidenceThreshold ?? null, + resolverProvider: pipeline.autoAgentSettings.provider, + resolverModel: pipeline.autoAgentSettings.model, + reasoningEffort: pipeline.autoAgentSettings.reasoningEffort, + permissionMode: pipeline.autoAgentSettings.permissionMode ?? "guarded_edit", + confidenceThreshold: pipeline.autoAgentSettings.confidenceThreshold, originSurface: args.originSurface ?? prior.originSurface ?? "manual", originMissionId: args.originMissionId ?? prior.originMissionId ?? null, originRunId: args.originRunId ?? prior.originRunId ?? null,As per coding guidelines, "apps/desktop//{ipc,preload,types,shared}//*.{ts,tsx}: Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/queueLandingService.ts` around lines 313 - 347, The function currently prefers deprecated args over the resolved pipeline and returns legacy mirror fields (autoResolve, resolverProvider, resolverModel, reasoningEffort, permissionMode, confidenceThreshold) only from args/prior; update the return construction so that after computing pipeline (via pipelineFromLegacyQueueConfig, mergePipelineSettings, basePipeline) you also derive and populate those legacy mirror fields from the resolved pipeline when they are not explicitly provided in args or prior—e.g., map pipeline.conflictStrategy / pipeline.autoAgentSettings / pipeline.permissionMode / pipeline.confidenceThreshold into autoResolve, resolverProvider, resolverModel, reasoningEffort, permissionMode, confidenceThreshold respectively so maybeResolveConflict() sees the effective behavior; keep existing precedence (args > prior > derived-from-pipeline > defaults) and ensure references to pipeline, argsPipeline, mergePipelineSettings, pipelineFromLegacyQueueConfig and maybeResolveConflict are used to locate where to add the derivation.apps/ade-cli/src/cli.ts (1)
1951-1957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSupport the documented
prs pipeline <pr> saveorder.The new help/docs advertise
ade prs pipeline <pr> save ..., but this branch still reads the first positional afterpipelineasmodeand the second asprId. Forade prs pipeline pr-1 save ..., that resolves tomode="pr-1"andprId="save".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 1951 - 1957, The branch treats the first positional after "pipeline" as mode and the second as prId, breaking the documented "prs pipeline <pr> save" order; swap the parsing so the first positional is the PR id and the second positional is the mode. Concretely, when sub === "pipeline" compute id from the first positional (falling back to prId) via requireValue, then compute mode from the next positional (falling back to "get"), and pass those into the existing action steps and settings collection (functions: firstPositional, prId, requireValue, collectGenericObjectArgs, readPipelineSettingsPatch, actionArgsListStep). Ensure the "get", "delete", and "save" branches use the newly-ordered id and mode variables.
🧹 Nitpick comments (4)
apps/ios/ADE/Services/SyncService.swift (1)
3988-3999: ⚡ Quick winConstrain
scopeto known values to prevent silent fallback to host default.
scopeis currently a free-formString?, but the backend only honors"checks","comments", or"both". Invalid values are ignored server-side, which can make the caller think the requested scope was applied when it wasn’t.♻️ Proposed change
+enum PathToMergeScope: String { + case checks + case comments + case both +} + `@discardableResult` func startPathToMerge( prId: String, modelId: String? = nil, reasoning: String? = nil, - scope: String? = nil, + scope: PathToMergeScope? = nil, additionalInstructions: String? = nil ) async throws -> StartPathToMergeResult { var payload: [String: Any] = ["prId": prId] if let modelId, !modelId.isEmpty { payload["modelId"] = modelId } if let reasoning, !reasoning.isEmpty { payload["reasoning"] = reasoning } - if let scope, !scope.isEmpty { payload["scope"] = scope } + if let scope { payload["scope"] = scope.rawValue } if let additionalInstructions, !additionalInstructions.isEmpty { payload["additionalInstructions"] = additionalInstructions }As per coding guidelines,
apps/ios/**/*.swift: iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 3988 - 3999, The startPathToMerge function accepts a free-form scope String which can silently be ignored by the server; change it to a constrained enum (e.g., Scope { case checks, comments, both }) or validate the incoming scope parameter against the allowed values before adding to payload: in startPathToMerge, replace or validate the scope parameter and only set payload["scope"] when scope matches "checks", "comments", or "both", otherwise throw or return an error so callers can correct their input; update any callers of startPathToMerge to pass the enum or validated string accordingly to avoid silent fallback.apps/desktop/src/main/services/prs/prMergeQueue.test.ts (1)
339-347: ⚡ Quick winAdd a pipeline-only queue auto-resolve test.
This case still exercises the deprecated
autoResolve/resolverModelargs, so it won't catch regressions in the new authoritativepipelinepath. A focused variant that passes onlypipeline.conflictStrategy = "auto"pluspipeline.autoAgentSettings.modelwould have failed against the current service logic.As per coding guidelines, "apps/desktop/**/*.test.{ts,tsx}: Run test with
npm --prefix apps/desktop run testfor desktop app validation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prMergeQueue.test.ts` around lines 339 - 347, Add a new test case in prs/prMergeQueue.test.ts that calls service.startQueue using the pipeline-based auto-resolve configuration instead of the deprecated args: pass pipeline: { conflictStrategy: "auto", autoAgentSettings: { model: "<model-id>" } } and omit autoResolve and resolverModel to ensure the service honors the pipeline path; name the test to indicate "pipeline-only queue auto-resolve" and run it via npm --prefix apps/desktop run test as per desktop test guidelines.apps/desktop/src/main/services/prs/issueInventoryService.ts (1)
502-508: 💤 Low valueConsider validating
permissionModelike other enum fields.The
auto_agent_provideris validated againstAUTO_AGENT_PROVIDER_VALUES, butpermissionModeat line 506 is cast directly without validation. If invalid data exists in the database, it could propagate to the UI.That said, since the database is the trusted source and this value originates from controlled writes via
savePipelineSettings, this is a low-risk concern. If you prefer consistency with the other enum validations, a validation set could be added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/issueInventoryService.ts` around lines 502 - 508, Validate the permissionMode before casting like provider: ensure row.auto_agent_permission_mode is checked against an allowed set (e.g., a PERMISSION_MODE_VALUES constant or the AutoConflictAgentSettings permissionMode enum) and only assign/cast to AutoConflictAgentSettings["permissionMode"] if it matches; otherwise fall back to a safe default or undefined. Update the autoAgentSettings construction (reference: autoAgentSettings, AutoConflictAgentSettings, row.auto_agent_permission_mode) to perform this check so invalid DB values don't propagate to the UI, mirroring the existing validation for AUTO_AGENT_PROVIDER_VALUES and keeping savePipelineSettings behavior unchanged.apps/desktop/src/main/services/prs/pathToMergeOrchestrator.test.ts (1)
291-348: ⚡ Quick winAssert the scheduling side effect explicitly.
These cases only prove
resumeFromPersistedState()does not throw. A no-op implementation — or one that still arms timers afterdispose()— would still pass. Add one positive assertion for the live PR scheduling path and one negative assertion thatdispose()suppresses any later scheduling.Also applies to: 387-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/pathToMergeOrchestrator.test.ts` around lines 291 - 348, Add explicit assertions that resumeFromPersistedState actually schedules the live PR and that dispose prevents later scheduling: after calling orchestrator.resumeFromPersistedState(), assert that the scheduling side-effect for the live PR (e.g. the warming timer or the deps mock used to schedule timers for "pr-live") has been invoked exactly once and that no scheduling was invoked for "pr-merged", "pr-stopped", "pr-disabled", or "pr-poller-stopped"; then call orchestrator.dispose() and attempt an operation that would schedule (or simulate timer firing) and assert that no further scheduling occurs (no additional timer callbacks, no new invocations of the scheduling mock, and interrupt is not called). Ensure you reference orchestrator.resumeFromPersistedState, orchestrator.dispose, orchestrator.stopPathToMerge, and the "pr-live"/other PR ids or the runtimeByPrId entries when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 2384-2403: The path-to-merge orchestrator created by
createPathToMergeOrchestrator (variable pathToMergeOrchestrator) is resumed but
never disposed, so add teardown logic to dispose it in disposeContextResources:
store pathToMergeOrchestrator on the same context/resource holder used by other
services, and in disposeContextResources call pathToMergeOrchestrator.dispose()
(guarded for existence and wrapped in try/catch to log any errors) to stop
timers/background work when the project/context is closed.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 7592-7599: The IPC handler is passing whitespace-only strings
through to orchestrator.startPathToMerge (e.g., args.modelId, args.reasoning,
args.additionalInstructions, args.reason), which should be normalized to
null/undefined so downstream defaults aren’t overridden; update the callsite(s)
that invoke orchestrator.startPathToMerge to trim each optional string and treat
empty or whitespace-only results as null (or undefined where appropriate for
scope), e.g., normalize args.modelId, args.reasoning,
args.additionalInstructions, and args.reason via String.prototype.trim() checks
before dispatching; apply the same normalization to the other occurrence noted
(lines ~7613–7615) so both calls use the same sanitized values.
- Around line 6977-6980: The IPC handler for IPC.prsRetargetBase currently
dereferences arg.prId and arg.baseBranch without validation; add explicit
validation at the IPC boundary in the ipcMain.handle callback to ensure arg is
an object, prId is a non-empty string (valid id format if available) and
baseBranch is a non-empty string (trimmed) before calling
getCtx().prService.retargetBase; if validation fails, throw or return a
controlled error (e.g., new Error('Invalid IPC payload: prId/baseBranch
required')) so malformed or empty branch names never reach
prService.retargetBase.
In `@apps/desktop/src/main/services/prs/pathToMergeOrchestrator.ts`:
- Around line 688-697: The pre-refresh fast-path that handles ctx.pr.state ===
"merged" marks the runtime and returns without calling
prService.runPostMergeCleanup(), so reuse the same merged-observation helper
used in the later merged-observation branch: after
issueInventoryService.saveConvergenceRuntime(...) and before
clearTimer(prId)/return, invoke the same helper logic that calls
prService.runPostMergeCleanup(prId, ctx, ...) (or directly call
prService.runPostMergeCleanup with the same arguments the merged-observation
branch uses) so lane deletion, lane advancement and cache cleanup run on
restart/resume; keep clearTimer(prId) and the existing runtime save call intact.
- Around line 299-327: stopPathToMerge currently only clears timers and
interrupts the persisted session, but long-running iterations (runIterationInner
-> applyConflictStrategy/runMergeLadder/launchPrIssueResolutionChat) can
continue; add a per-PR generation/abort token stored via
issueInventoryService.saveConvergenceRuntime and checked by runIterationInner.
On stopPathToMerge (function stopPathToMerge) bump or set a new abort token (and
persist it with saveConvergenceRuntime) and ensure
inProcessState/clearTimer/agentChatService.interrupt behavior is unchanged; then
in runIterationInner capture the token at start and after each awaited external
call (e.g., after applyConflictStrategy, runMergeLadder,
launchPrIssueResolutionChat and any other awaits) compare the persisted token
via issueInventoryService.getConvergenceRuntime(prId).abortToken (or similar)
and abort the iteration early (skip scheduling, pushing, or saving runtime) if
the token changed; ensure any runtime save
(issueInventoryService.saveConvergenceRuntime) or scheduling only happens when
tokens still match.
- Around line 494-498: The branch treating autoSettings.provider as a hard error
prevents using the UI "Inherit default" (serialized as null); change the logic
so that when autoSettings.provider is null you resolve it to the project's
default resolver before proceeding to runExternalResolver. Concretely: in the
block using pipelineSettings.autoAgentSettings and provider, check for provider
== null (or undefined) and substitute the project-level default resolver (e.g.,
from pipelineSettings.projectDefaultResolver / pipelineSettings.defaultResolver
or by calling the existing function that returns the project default) so that
runExternalResolver always receives a concrete provider string instead of
returning a failure.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 3048-3055: After a successful merge the local PR row remains
"open" until runPostMergeCleanup() calls refreshOne(), which can cause racey
helpers like getActiveRowForCurrentLaneBranch() to act on an already-merged PR;
immediately mark and persist the PR as merged locally (update the in-memory
row/state and write it to the local store) using the same identifiers (row.id
and mergeCommitSha) before invoking runPostMergeCleanup({ prId: row.id,
mergeCommitSha, archiveLane: Boolean(args.archiveLane), operationId:
op.operationId }), so that any concurrent reads see the merged state even if
refreshOne() later fails or is delayed.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 3373-3381: The migration that adds conflict_strategy currently
creates the column with default 'pause', which masks existing rows where
on_rebase_needed = 'auto_rebase' and causes
issueInventoryService.readPipelineSettings() to lose legacy behavior; fix the
migration in the block using db.run on table pr_pipeline_settings by (1) adding
conflict_strategy as a nullable column without a default, (2) backfilling rows
with conflict_strategy = 'auto_rebase' where on_rebase_needed = 'auto_rebase',
(3) updating any remaining NULL conflict_strategy rows to 'pause' if you want
the default for new/other rows, and (4) if you require the column to be NOT
NULL, alter it afterward to set NOT NULL and a default 'pause'; ensure all SQL
statements reference pr_pipeline_settings and the on_rebase_needed =
'auto_rebase' predicate so legacy auto-rebase rows are preserved.
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Line 534: Locate the remote command registrations for prs.pathToMerge.start
and prs.pathToMerge.stop in syncRemoteCommandService (the block that currently
has { viewerAllowed: true, queueable: true }) and add requiresApproval: true to
each policy object; ensure the registration entries for prs.pathToMerge.start
and prs.pathToMerge.stop include { viewerAllowed: true, queueable: true,
requiresApproval: true } so these sensitive operations require on-desktop
approval before executing the pathToMergeOrchestrator flow.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 2247-2251: Normalize the optional string inputs by piping them
through the module's asTrimmedString(...) helper before using or forwarding
them: replace the current raw checks for modelId, reasoning,
additionalInstructions (and the other similar occurrence for reason) with calls
to asTrimmedString(payload?.modelId), asTrimmedString(payload?.reasoning),
asTrimmedString(payload?.additionalInstructions) (and
asTrimmedString(payload?.reason)) and treat empty/whitespace-only results as
null; update the assignments in syncRemoteCommandService.ts where modelId,
reasoning, additionalInstructions and reason are derived so downstream callers
receive trimmed strings or null.
- Around line 2264-2270: The handler registered under
register("prs.pathToMerge.stop", { viewerAllowed: true, queueable: true }, ...)
should not be queueable; update the registration options to remove or set
queueable to false so stop requests are processed immediately. Locate the
register call for "prs.pathToMerge.stop" and change its options object, ensuring
the rest of the logic (parseIssueInventoryPrArgs(...) and
args.pathToMergeOrchestrator.stopPathToMerge({ prId, reason })) remains
unchanged.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Line 4607: pipelineSettingsGet currently returns the shared
DEFAULT_PIPELINE_SETTINGS reference which allows callers to mutate the
module-level constant; change pipelineSettingsGet to return a copy instead
(e.g., shallow clone via { ...DEFAULT_PIPELINE_SETTINGS } or, if nested objects
exist, a deep clone via structuredClone(DEFAULT_PIPELINE_SETTINGS)) so callers
receive an isolated object — mirror the approach used by convergenceStateGet to
avoid shared-state mutation.
In `@apps/desktop/src/renderer/components/prs/tabs/QueueAutomateMergingModal.tsx`:
- Around line 172-194: The modal currently reads the live members array
(members) during the run which can change due to parent polling; freeze the
member list when opening by capturing a snapshot (e.g., frozenMembersRef or
frozenMembers state) inside the open transition in the useEffect that uses
wasOpenRef, then use that frozen snapshot everywhere the run reads members
(replace direct uses of members in the setStatuses initialization, the
run/orchestration loop, and any other logic referenced around
setSequence/cancelledRef) and remove members from the effect dependency so
reopening is the only time the snapshot is taken; ensure cancelledRef/current
behavior and setSequence initialization remain unchanged but operate against the
frozen list.
- Around line 219-230: The loop currently only returns for statuses in
TERMINAL_SUCCESS or TERMINAL_FAILED, causing infinite polling when Path to Merge
reaches non-auto-progress states like "paused" or "converged"; add a check after
the success/failure checks that if status is present and is not an auto-progress
state (e.g., statuses that require human action such as "paused" or "converged")
then call updateStatus(prId, { kind: "running", runtimeStatus: status,
pauseReason, round }) and return { outcome: "failed", runtime, error:
runtime?.errorMessage?.trim() || `Path to Merge ${status}` } (or a suitable
actionable message) to treat those states as terminal instead of continuing to
await POLL_INTERVAL_MS; use the existing symbols status, TERMINAL_SUCCESS,
TERMINAL_FAILED, updateStatus, runtime, pauseReason, round, and POLL_INTERVAL_MS
to locate and implement the change.
In `@apps/desktop/src/renderer/components/prs/tabs/QueueTab.tsx`:
- Around line 110-117: The describeConvergenceWaitState helper currently falls
through to "running" when conv.status === "converged"; update the function
(describeConvergenceWaitState) to explicitly handle conv.status === "converged"
and return "converged" (place this check after existing wait/pause checks but
before the final default return) so the queue badge shows the converged state
instead of "running".
- Around line 461-495: The polling can overwrite newer state because fetchAll()
calls overlap; replace the setInterval approach in the effect (the const
fetchAll, interval, cancelled logic) with a self-scheduling loop that prevents
concurrent runs: implement a single-run loop using an async function (e.g.,
runLoop) that awaits fetchAll() then waits 5000ms via setTimeout before
re-invoking itself, or add an inFlight boolean guard inside fetchAll to return
early if a previous invocation is still running; ensure you still respect the
cancelled flag, clear the pending timeout in the cleanup, and keep setting state
only from the latest completed run (use the cancelled flag or a sequence id) so
setConvergenceByPrId and window.ade.prs.convergenceStateGet calls cannot overlap
and race.
In `@apps/ios/ADE/Resources/DatabaseBootstrap.sql`:
- Around line 711-714: The unconditional "delete from queue_landing_state;"
removes all in-flight queue progress; instead remove that statement and
implement a safe, idempotent migration: either skip deleting entirely or perform
a conditional purge (e.g., DELETE FROM queue_landing_state WHERE updated_at < ?
OR state IN ('invalid','stale')) or add a migration flag column and only remove
rows tied to the old schema; update the bootstrap script to preserve existing
rows and, if needed, add an explicit, well-documented selective cleanup step
that targets only truly orphaned or stale records in queue_landing_state.
In `@apps/ios/ADE/Views/PRs/PrDetailScreen.swift`:
- Around line 1048-1075: stopPathToMerge currently only updates local UI when
result.runtime exists which leaves issueInventory.runtime.autoConvergeEnabled
true if the server returns nil; change stopPathToMerge (and symmetrically
startPathToMerge) so when result.runtime is nil you optimistically patch the
local runtime: if issueInventory != nil create a new ConvergenceRuntimeState by
copying the existing issueInventory.runtime (or creating a minimal default
runtime) with autoConvergeEnabled set to false (or true for start) and call
applyConvergenceRuntime(next); if issueInventory is nil call await
reload(refreshRemote: false) on the MainActor as a fallback so the mode strip is
consistent; use the existing symbols stopPathToMerge, startPathToMerge,
applyConvergenceRuntime, issueInventory, and ConvergenceRuntimeState to locate
and implement the change.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 1951-1957: The branch treats the first positional after "pipeline"
as mode and the second as prId, breaking the documented "prs pipeline <pr> save"
order; swap the parsing so the first positional is the PR id and the second
positional is the mode. Concretely, when sub === "pipeline" compute id from the
first positional (falling back to prId) via requireValue, then compute mode from
the next positional (falling back to "get"), and pass those into the existing
action steps and settings collection (functions: firstPositional, prId,
requireValue, collectGenericObjectArgs, readPipelineSettingsPatch,
actionArgsListStep). Ensure the "get", "delete", and "save" branches use the
newly-ordered id and mode variables.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 3098-3103: The PATCH to GitHub (githubService.apiRequest) in
retargetBase() already succeeds but a subsequent refreshOne(prId) failure causes
the whole operation to be treated as failed; change retargetBase() so the API
PATCH result determines success and wrap the refreshOne(prId) call in a
try/catch (or otherwise handle errors) so any refresh failure is logged/recorded
but does not propagate as an error/throw or change the return value—ensure
githubService.apiRequest and retargetBase() remain authoritative for success,
while advanceChildLanesAfterLand() and callers see the operation as successful
even if refreshOne() failed.
In `@apps/desktop/src/main/services/prs/queueLandingService.ts`:
- Around line 313-347: The function currently prefers deprecated args over the
resolved pipeline and returns legacy mirror fields (autoResolve,
resolverProvider, resolverModel, reasoningEffort, permissionMode,
confidenceThreshold) only from args/prior; update the return construction so
that after computing pipeline (via pipelineFromLegacyQueueConfig,
mergePipelineSettings, basePipeline) you also derive and populate those legacy
mirror fields from the resolved pipeline when they are not explicitly provided
in args or prior—e.g., map pipeline.conflictStrategy /
pipeline.autoAgentSettings / pipeline.permissionMode /
pipeline.confidenceThreshold into autoResolve, resolverProvider, resolverModel,
reasoningEffort, permissionMode, confidenceThreshold respectively so
maybeResolveConflict() sees the effective behavior; keep existing precedence
(args > prior > derived-from-pipeline > defaults) and ensure references to
pipeline, argsPipeline, mergePipelineSettings, pipelineFromLegacyQueueConfig and
maybeResolveConflict are used to locate where to add the derivation.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/issueInventoryService.ts`:
- Around line 502-508: Validate the permissionMode before casting like provider:
ensure row.auto_agent_permission_mode is checked against an allowed set (e.g., a
PERMISSION_MODE_VALUES constant or the AutoConflictAgentSettings permissionMode
enum) and only assign/cast to AutoConflictAgentSettings["permissionMode"] if it
matches; otherwise fall back to a safe default or undefined. Update the
autoAgentSettings construction (reference: autoAgentSettings,
AutoConflictAgentSettings, row.auto_agent_permission_mode) to perform this check
so invalid DB values don't propagate to the UI, mirroring the existing
validation for AUTO_AGENT_PROVIDER_VALUES and keeping savePipelineSettings
behavior unchanged.
In `@apps/desktop/src/main/services/prs/pathToMergeOrchestrator.test.ts`:
- Around line 291-348: Add explicit assertions that resumeFromPersistedState
actually schedules the live PR and that dispose prevents later scheduling: after
calling orchestrator.resumeFromPersistedState(), assert that the scheduling
side-effect for the live PR (e.g. the warming timer or the deps mock used to
schedule timers for "pr-live") has been invoked exactly once and that no
scheduling was invoked for "pr-merged", "pr-stopped", "pr-disabled", or
"pr-poller-stopped"; then call orchestrator.dispose() and attempt an operation
that would schedule (or simulate timer firing) and assert that no further
scheduling occurs (no additional timer callbacks, no new invocations of the
scheduling mock, and interrupt is not called). Ensure you reference
orchestrator.resumeFromPersistedState, orchestrator.dispose,
orchestrator.stopPathToMerge, and the "pr-live"/other PR ids or the
runtimeByPrId entries when adding these assertions.
In `@apps/desktop/src/main/services/prs/prMergeQueue.test.ts`:
- Around line 339-347: Add a new test case in prs/prMergeQueue.test.ts that
calls service.startQueue using the pipeline-based auto-resolve configuration
instead of the deprecated args: pass pipeline: { conflictStrategy: "auto",
autoAgentSettings: { model: "<model-id>" } } and omit autoResolve and
resolverModel to ensure the service honors the pipeline path; name the test to
indicate "pipeline-only queue auto-resolve" and run it via npm --prefix
apps/desktop run test as per desktop test guidelines.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 3988-3999: The startPathToMerge function accepts a free-form scope
String which can silently be ignored by the server; change it to a constrained
enum (e.g., Scope { case checks, comments, both }) or validate the incoming
scope parameter against the allowed values before adding to payload: in
startPathToMerge, replace or validate the scope parameter and only set
payload["scope"] when scope matches "checks", "comments", or "both", otherwise
throw or return an error so callers can correct their input; update any callers
of startPathToMerge to pass the enum or validated string accordingly to avoid
silent fallback.
🪄 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: efd7e4e6-14ed-4b32-a60e-6e07b6bede2c
⛔ Files ignored due to path filters (5)
.claude/scheduled_tasks.lockis excluded by!**/*.lockapps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/pull-requests/path-to-merge.mdis excluded by!docs/**
📒 Files selected for processing (35)
apps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/prs/issueInventoryService.tsapps/desktop/src/main/services/prs/pathToMergeOrchestrator.test.tsapps/desktop/src/main/services/prs/pathToMergeOrchestrator.tsapps/desktop/src/main/services/prs/prIssueResolution.test.tsapps/desktop/src/main/services/prs/prMergeQueue.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/prs/queueLandingService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/sync/syncService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrPipelineSettings.tsxapps/desktop/src/renderer/components/prs/shared/prFormatters.tsapps/desktop/src/renderer/components/prs/tabs/QueueAutomateMergingModal.tsxapps/desktop/src/renderer/components/prs/tabs/QueueTab.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/PRs/PrDetailOverviewTab.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swift
Comment on lines
+1935
to
1947
| // Path to Merge orchestrator reads conflictStrategy / forceFinalizeMode / | ||
| // earlyMergeOnGreen / autoMerge / maxRounds / mergeMethod from saved | ||
| // PipelineSettings, not from the launch args. Persist any user-supplied | ||
| // overrides before the resolver step so the loop picks them up. | ||
| const pipelinePatch = readPipelineSettingsPatch(args); | ||
| const steps: InvocationStep[] = []; | ||
| if (maxRounds != null || autoMerge || noAutoMerge || mergeMethod) { | ||
| if (Object.keys(pipelinePatch).length > 0) { | ||
| steps.push(actionArgsListStep("pipelineSettings", "issue_inventory", "savePipelineSettings", [ | ||
| id, | ||
| { | ||
| ...(maxRounds != null ? { maxRounds } : {}), | ||
| ...(autoMerge || noAutoMerge ? { autoMerge: autoMerge && !noAutoMerge } : {}), | ||
| ...(mergeMethod ? { mergeMethod } : {}), | ||
| }, | ||
| pipelinePatch, | ||
| ])); | ||
| } | ||
| steps.push(actionCallStep("result", mode === "preview" ? "pr_preview_issue_resolution_prompt" : "pr_start_issue_resolution", collectGenericObjectArgs(args, input))); |
There was a problem hiding this comment.
Don't persist pipeline overrides during preview.
This now runs issue_inventory.savePipelineSettings for ade prs path-to-merge preview ... too, so a prompt preview mutates the PR's saved pipeline configuration. That makes a read-only preview stateful and can silently affect later real runs.
Suggested fix
- if (Object.keys(pipelinePatch).length > 0) {
+ if (mode !== "preview" && Object.keys(pipelinePatch).length > 0) {
steps.push(actionArgsListStep("pipelineSettings", "issue_inventory", "savePipelineSettings", [
id,
pipelinePatch,
]));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Path to Merge orchestrator reads conflictStrategy / forceFinalizeMode / | |
| // earlyMergeOnGreen / autoMerge / maxRounds / mergeMethod from saved | |
| // PipelineSettings, not from the launch args. Persist any user-supplied | |
| // overrides before the resolver step so the loop picks them up. | |
| const pipelinePatch = readPipelineSettingsPatch(args); | |
| const steps: InvocationStep[] = []; | |
| if (maxRounds != null || autoMerge || noAutoMerge || mergeMethod) { | |
| if (Object.keys(pipelinePatch).length > 0) { | |
| steps.push(actionArgsListStep("pipelineSettings", "issue_inventory", "savePipelineSettings", [ | |
| id, | |
| { | |
| ...(maxRounds != null ? { maxRounds } : {}), | |
| ...(autoMerge || noAutoMerge ? { autoMerge: autoMerge && !noAutoMerge } : {}), | |
| ...(mergeMethod ? { mergeMethod } : {}), | |
| }, | |
| pipelinePatch, | |
| ])); | |
| } | |
| steps.push(actionCallStep("result", mode === "preview" ? "pr_preview_issue_resolution_prompt" : "pr_start_issue_resolution", collectGenericObjectArgs(args, input))); | |
| const pipelinePatch = readPipelineSettingsPatch(args); | |
| const steps: InvocationStep[] = []; | |
| if (mode !== "preview" && Object.keys(pipelinePatch).length > 0) { | |
| steps.push(actionArgsListStep("pipelineSettings", "issue_inventory", "savePipelineSettings", [ | |
| id, | |
| pipelinePatch, | |
| ])); | |
| } | |
| steps.push(actionCallStep("result", mode === "preview" ? "pr_preview_issue_resolution_prompt" : "pr_start_issue_resolution", collectGenericObjectArgs(args, input))); |
Comment on lines
+2384
to
+2403
| const pathToMergeOrchestrator = createPathToMergeOrchestrator({ | ||
| logger, | ||
| prService, | ||
| laneService, | ||
| agentChatService, | ||
| sessionService, | ||
| issueInventoryService, | ||
| conflictService, | ||
| defaultModelId: null, | ||
| defaultReasoningEffort: null, | ||
| }); | ||
| setImmediate(() => { | ||
| try { | ||
| pathToMergeOrchestrator.resumeFromPersistedState(); | ||
| } catch (err) { | ||
| logger.warn("path_to_merge.resume_failed", { | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dispose the path-to-merge orchestrator during context teardown.
The orchestrator is now resumed from persisted state, but it is not disposed in disposeContextResources. That can leave timers/background convergence activity running against torn-down services after project switch/close.
Proposed fix
--- a/apps/desktop/src/main/main.ts
+++ b/apps/desktop/src/main/main.ts
@@
try {
+ ctx.pathToMergeOrchestrator?.dispose?.();
+ } catch {
+ // ignore
+ }
+ try {
ctx.prPollingService.dispose();
} catch {
// ignore
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/main.ts` around lines 2384 - 2403, The path-to-merge
orchestrator created by createPathToMergeOrchestrator (variable
pathToMergeOrchestrator) is resumed but never disposed, so add teardown logic to
dispose it in disposeContextResources: store pathToMergeOrchestrator on the same
context/resource holder used by other services, and in disposeContextResources
call pathToMergeOrchestrator.dispose() (guarded for existence and wrapped in
try/catch to log any errors) to stop timers/background work when the
project/context is closed.
Comment on lines
+6977
to
+6980
| ipcMain.handle(IPC.prsRetargetBase, async (_event, arg: { prId: string; baseBranch: string }): Promise<void> => { | ||
| const ctx = getCtx(); | ||
| return await ctx.prService.retargetBase(arg.prId, arg.baseBranch); | ||
| }); |
There was a problem hiding this comment.
Validate the retarget payload at the IPC boundary.
This new handler dereferences arg.prId / arg.baseBranch directly and forwards raw values into prService.retargetBase(). A malformed invoke becomes a generic main-process TypeError, and an empty branch name falls through to the GitHub mutation path.
Suggested fix
ipcMain.handle(IPC.prsRetargetBase, async (_event, arg: { prId: string; baseBranch: string }): Promise<void> => {
const ctx = getCtx();
- return await ctx.prService.retargetBase(arg.prId, arg.baseBranch);
+ const prId = typeof arg?.prId === "string" ? arg.prId.trim() : "";
+ const baseBranch = typeof arg?.baseBranch === "string" ? arg.baseBranch.trim() : "";
+ if (!prId) throw new Error("prId is required");
+ if (!baseBranch) throw new Error("baseBranch is required");
+ await ctx.prService.retargetBase(prId, baseBranch);
});As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 6977 - 6980,
The IPC handler for IPC.prsRetargetBase currently dereferences arg.prId and
arg.baseBranch without validation; add explicit validation at the IPC boundary
in the ipcMain.handle callback to ensure arg is an object, prId is a non-empty
string (valid id format if available) and baseBranch is a non-empty string
(trimmed) before calling getCtx().prService.retargetBase; if validation fails,
throw or return a controlled error (e.g., new Error('Invalid IPC payload:
prId/baseBranch required')) so malformed or empty branch names never reach
prService.retargetBase.
Comment on lines
+7592
to
+7599
| return await orchestrator.startPathToMerge({ | ||
| prId, | ||
| modelId: typeof args?.modelId === "string" ? args.modelId : null, | ||
| reasoning: typeof args?.reasoning === "string" ? args.reasoning : null, | ||
| scope: args?.scope === "checks" || args?.scope === "comments" || args?.scope === "both" | ||
| ? args.scope | ||
| : undefined, | ||
| additionalInstructions: typeof args?.additionalInstructions === "string" ? args.additionalInstructions : null, |
There was a problem hiding this comment.
Normalize optional Path-to-Merge strings before dispatching.
modelId, reasoning, additionalInstructions, and reason currently preserve whitespace-only values. That makes empty renderer inputs look like real overrides and can bypass downstream defaults or persist empty instructions.
Suggested fix
return await orchestrator.startPathToMerge({
prId,
- modelId: typeof args?.modelId === "string" ? args.modelId : null,
- reasoning: typeof args?.reasoning === "string" ? args.reasoning : null,
+ modelId: typeof args?.modelId === "string" && args.modelId.trim().length > 0 ? args.modelId.trim() : null,
+ reasoning: typeof args?.reasoning === "string" && args.reasoning.trim().length > 0 ? args.reasoning.trim() : null,
scope: args?.scope === "checks" || args?.scope === "comments" || args?.scope === "both"
? args.scope
: undefined,
- additionalInstructions: typeof args?.additionalInstructions === "string" ? args.additionalInstructions : null,
+ additionalInstructions:
+ typeof args?.additionalInstructions === "string" && args.additionalInstructions.trim().length > 0
+ ? args.additionalInstructions.trim()
+ : null,
});
@@
return await orchestrator.stopPathToMerge({
prId,
- reason: typeof args?.reason === "string" ? args.reason : null,
+ reason: typeof args?.reason === "string" && args.reason.trim().length > 0 ? args.reason.trim() : null,
});As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
Also applies to: 7613-7615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 7592 - 7599,
The IPC handler is passing whitespace-only strings through to
orchestrator.startPathToMerge (e.g., args.modelId, args.reasoning,
args.additionalInstructions, args.reason), which should be normalized to
null/undefined so downstream defaults aren’t overridden; update the callsite(s)
that invoke orchestrator.startPathToMerge to trim each optional string and treat
empty or whitespace-only results as null (or undefined where appropriate for
scope), e.g., normalize args.modelId, args.reasoning,
args.additionalInstructions, and args.reason via String.prototype.trim() checks
before dispatching; apply the same normalization to the other occurrence noted
(lines ~7613–7615) so both calls use the same sanitized values.
Comment on lines
+299
to
+327
| async function stopPathToMerge(args: { prId: string; reason?: string | null }): Promise<StopPathToMergeResult> { | ||
| const prId = args.prId.trim(); | ||
| if (!prId) throw new Error("prId is required"); | ||
|
|
||
| clearTimer(prId); | ||
| inProcessState.delete(prId); | ||
|
|
||
| const current = issueInventoryService.getConvergenceRuntime(prId); | ||
| const activeSessionId = current.activeSessionId; | ||
| if (activeSessionId) { | ||
| try { | ||
| await agentChatService.interrupt({ sessionId: activeSessionId }); | ||
| } catch (err) { | ||
| logger.warn("ptm.interrupt_failed", { prId, sessionId: activeSessionId, error: getErrorMessage(err) }); | ||
| } | ||
| } | ||
|
|
||
| const runtime = issueInventoryService.saveConvergenceRuntime(prId, { | ||
| autoConvergeEnabled: false, | ||
| status: "stopped", | ||
| pollerStatus: "stopped", | ||
| activeSessionId: null, | ||
| pauseReason: args.reason?.trim() || null, | ||
| errorMessage: null, | ||
| lastStoppedAt: nowIso(), | ||
| }); | ||
| issueInventoryService.savePathToMergeArgs(prId, null); | ||
|
|
||
| return { prId, stopped: true, runtime }; |
There was a problem hiding this comment.
Guard stopPathToMerge against in-flight iterations.
stopPathToMerge() only clears timers and interrupts the session id that is already persisted. If an iteration is currently inside applyConflictStrategy(), runMergeLadder(), or launchPrIssueResolutionChat(), it keeps going because runIterationInner() only checks autoConvergeEnabled once before the long async chain. A stop that races with those awaits can still push, reschedule, or merge after the user asked it to stop. Add a per-PR abort/generation token and re-check it after each awaited external step before saving runtime or scheduling the next wake-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/prs/pathToMergeOrchestrator.ts` around lines
299 - 327, stopPathToMerge currently only clears timers and interrupts the
persisted session, but long-running iterations (runIterationInner ->
applyConflictStrategy/runMergeLadder/launchPrIssueResolutionChat) can continue;
add a per-PR generation/abort token stored via
issueInventoryService.saveConvergenceRuntime and checked by runIterationInner.
On stopPathToMerge (function stopPathToMerge) bump or set a new abort token (and
persist it with saveConvergenceRuntime) and ensure
inProcessState/clearTimer/agentChatService.interrupt behavior is unchanged; then
in runIterationInner capture the token at start and after each awaited external
call (e.g., after applyConflictStrategy, runMergeLadder,
launchPrIssueResolutionChat and any other awaits) compare the persisted token
via issueInventoryService.getConvergenceRuntime(prId).abortToken (or similar)
and abort the iteration early (skip scheduling, pushing, or saving runtime) if
the token changed; ensure any runtime save
(issueInventoryService.saveConvergenceRuntime) or scheduling only happens when
tokens still match.
Comment on lines
+219
to
+230
| const status = runtime?.status ?? null; | ||
| const round = runtime?.currentRound ?? 0; | ||
| const pauseReason = runtime?.pauseReason ?? null; | ||
| updateStatus(prId, { kind: "running", runtimeStatus: status, pauseReason, round }); | ||
| if (status && TERMINAL_SUCCESS.has(status)) { | ||
| return { outcome: "merged", runtime }; | ||
| } | ||
| if (status && TERMINAL_FAILED.has(status)) { | ||
| const message = runtime?.errorMessage?.trim() || `Path to Merge ${status}`; | ||
| return { outcome: "failed", runtime, error: message }; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); |
There was a problem hiding this comment.
Treat non-auto-progress states as terminal for the queue flow.
This loop only exits on merged, failed, cancelled, or stopped. If Path to Merge settles in paused or converged, the modal keeps polling forever and never advances or halts with an actionable error.
💡 Suggested fix
const status = runtime?.status ?? null;
const round = runtime?.currentRound ?? 0;
const pauseReason = runtime?.pauseReason ?? null;
updateStatus(prId, { kind: "running", runtimeStatus: status, pauseReason, round });
if (status && TERMINAL_SUCCESS.has(status)) {
return { outcome: "merged", runtime };
}
+ if (status === "paused" || status === "converged") {
+ return {
+ outcome: "failed",
+ runtime,
+ error: runtime?.pauseReason?.trim()
+ || `Path to Merge is waiting for manual action (${status})`,
+ };
+ }
if (status && TERMINAL_FAILED.has(status)) {
const message = runtime?.errorMessage?.trim() || `Path to Merge ${status}`;
return { outcome: "failed", runtime, error: message };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/tabs/QueueAutomateMergingModal.tsx`
around lines 219 - 230, The loop currently only returns for statuses in
TERMINAL_SUCCESS or TERMINAL_FAILED, causing infinite polling when Path to Merge
reaches non-auto-progress states like "paused" or "converged"; add a check after
the success/failure checks that if status is present and is not an auto-progress
state (e.g., statuses that require human action such as "paused" or "converged")
then call updateStatus(prId, { kind: "running", runtimeStatus: status,
pauseReason, round }) and return { outcome: "failed", runtime, error:
runtime?.errorMessage?.trim() || `Path to Merge ${status}` } (or a suitable
actionable message) to treat those states as terminal instead of continuing to
await POLL_INTERVAL_MS; use the existing symbols status, TERMINAL_SUCCESS,
TERMINAL_FAILED, updateStatus, runtime, pauseReason, round, and POLL_INTERVAL_MS
to locate and implement the change.
Comment on lines
+110
to
+117
| function describeConvergenceWaitState(conv: ConvergenceRuntimeState): string { | ||
| if (conv.pollerStatus === "waiting_for_checks") return "waiting for CI"; | ||
| if (conv.pollerStatus === "waiting_for_comments") return "waiting for review"; | ||
| if (conv.pauseReason) return `paused: ${conv.pauseReason}`; | ||
| if (conv.status === "polling") return "polling"; | ||
| if (conv.status === "paused") return "paused"; | ||
| return "running"; | ||
| } |
There was a problem hiding this comment.
Handle converged state explicitly in the badge text.
When conv.status === "converged" and there is no wait/pause reason, this helper falls through to "running", so the queue badge shows an active run after the automation has already converged.
Suggested fix
function describeConvergenceWaitState(conv: ConvergenceRuntimeState): string {
if (conv.pollerStatus === "waiting_for_checks") return "waiting for CI";
if (conv.pollerStatus === "waiting_for_comments") return "waiting for review";
if (conv.pauseReason) return `paused: ${conv.pauseReason}`;
+ if (conv.status === "launching") return "starting";
if (conv.status === "polling") return "polling";
if (conv.status === "paused") return "paused";
+ if (conv.status === "converged") return "converged";
return "running";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function describeConvergenceWaitState(conv: ConvergenceRuntimeState): string { | |
| if (conv.pollerStatus === "waiting_for_checks") return "waiting for CI"; | |
| if (conv.pollerStatus === "waiting_for_comments") return "waiting for review"; | |
| if (conv.pauseReason) return `paused: ${conv.pauseReason}`; | |
| if (conv.status === "polling") return "polling"; | |
| if (conv.status === "paused") return "paused"; | |
| return "running"; | |
| } | |
| function describeConvergenceWaitState(conv: ConvergenceRuntimeState): string { | |
| if (conv.pollerStatus === "waiting_for_checks") return "waiting for CI"; | |
| if (conv.pollerStatus === "waiting_for_comments") return "waiting for review"; | |
| if (conv.pauseReason) return `paused: ${conv.pauseReason}`; | |
| if (conv.status === "launching") return "starting"; | |
| if (conv.status === "polling") return "polling"; | |
| if (conv.status === "paused") return "paused"; | |
| if (conv.status === "converged") return "converged"; | |
| return "running"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/tabs/QueueTab.tsx` around lines 110
- 117, The describeConvergenceWaitState helper currently falls through to
"running" when conv.status === "converged"; update the function
(describeConvergenceWaitState) to explicitly handle conv.status === "converged"
and return "converged" (place this check after existing wait/pause checks but
before the final default return) so the queue badge shows the converged state
instead of "running".
Comment on lines
+461
to
+495
| React.useEffect(() => { | ||
| // memberPrIdsKey is a sorted-joined string and is the only identity-stable | ||
| // signal for the member set; depending on `selectedGroup?.members` would | ||
| // re-arm this effect on every queue-state poll because the parent rebuilds | ||
| // the array reference each time. The effect already reads the current | ||
| // members through the closure, so re-running on identity churn buys | ||
| // nothing. | ||
| const memberIds = memberPrIdsKey ? memberPrIdsKey.split("|") : []; | ||
| if (memberIds.length === 0) { | ||
| setConvergenceByPrId({}); | ||
| return; | ||
| } | ||
| let cancelled = false; | ||
| const fetchAll = async () => { | ||
| const next: Record<string, ConvergenceRuntimeState | null> = {}; | ||
| await Promise.all( | ||
| memberIds.map(async (prId) => { | ||
| try { | ||
| next[prId] = await window.ade.prs.convergenceStateGet(prId); | ||
| } catch { | ||
| next[prId] = null; | ||
| } | ||
| }), | ||
| ); | ||
| if (!cancelled) setConvergenceByPrId(next); | ||
| }; | ||
| void fetchAll(); | ||
| const interval = setInterval(() => { | ||
| if (!cancelled) void fetchAll(); | ||
| }, 5000); | ||
| return () => { | ||
| cancelled = true; | ||
| clearInterval(interval); | ||
| }; | ||
| }, [memberPrIdsKey]); |
There was a problem hiding this comment.
Prevent overlapping convergence polls from racing each other.
This setInterval keeps firing even if the previous fetchAll() is still in flight. A slow IPC round can then finish after a newer one and overwrite fresher convergence state, while also stacking extra convergenceStateGet calls.
Suggested fix
React.useEffect(() => {
const memberIds = memberPrIdsKey ? memberPrIdsKey.split("|") : [];
if (memberIds.length === 0) {
setConvergenceByPrId({});
return;
}
let cancelled = false;
+ let inFlight = false;
+ let requestSeq = 0;
+
const fetchAll = async () => {
+ if (inFlight) return;
+ inFlight = true;
+ const seq = ++requestSeq;
const next: Record<string, ConvergenceRuntimeState | null> = {};
- await Promise.all(
- memberIds.map(async (prId) => {
- try {
- next[prId] = await window.ade.prs.convergenceStateGet(prId);
- } catch {
- next[prId] = null;
- }
- }),
- );
- if (!cancelled) setConvergenceByPrId(next);
+ try {
+ await Promise.all(
+ memberIds.map(async (prId) => {
+ try {
+ next[prId] = await window.ade.prs.convergenceStateGet(prId);
+ } catch {
+ next[prId] = null;
+ }
+ }),
+ );
+ if (!cancelled && seq === requestSeq) setConvergenceByPrId(next);
+ } finally {
+ inFlight = false;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| React.useEffect(() => { | |
| // memberPrIdsKey is a sorted-joined string and is the only identity-stable | |
| // signal for the member set; depending on `selectedGroup?.members` would | |
| // re-arm this effect on every queue-state poll because the parent rebuilds | |
| // the array reference each time. The effect already reads the current | |
| // members through the closure, so re-running on identity churn buys | |
| // nothing. | |
| const memberIds = memberPrIdsKey ? memberPrIdsKey.split("|") : []; | |
| if (memberIds.length === 0) { | |
| setConvergenceByPrId({}); | |
| return; | |
| } | |
| let cancelled = false; | |
| const fetchAll = async () => { | |
| const next: Record<string, ConvergenceRuntimeState | null> = {}; | |
| await Promise.all( | |
| memberIds.map(async (prId) => { | |
| try { | |
| next[prId] = await window.ade.prs.convergenceStateGet(prId); | |
| } catch { | |
| next[prId] = null; | |
| } | |
| }), | |
| ); | |
| if (!cancelled) setConvergenceByPrId(next); | |
| }; | |
| void fetchAll(); | |
| const interval = setInterval(() => { | |
| if (!cancelled) void fetchAll(); | |
| }, 5000); | |
| return () => { | |
| cancelled = true; | |
| clearInterval(interval); | |
| }; | |
| }, [memberPrIdsKey]); | |
| React.useEffect(() => { | |
| // memberPrIdsKey is a sorted-joined string and is the only identity-stable | |
| // signal for the member set; depending on `selectedGroup?.members` would | |
| // re-arm this effect on every queue-state poll because the parent rebuilds | |
| // the array reference each time. The effect already reads the current | |
| // members through the closure, so re-running on identity churn buys | |
| // nothing. | |
| const memberIds = memberPrIdsKey ? memberPrIdsKey.split("|") : []; | |
| if (memberIds.length === 0) { | |
| setConvergenceByPrId({}); | |
| return; | |
| } | |
| let cancelled = false; | |
| let inFlight = false; | |
| let requestSeq = 0; | |
| const fetchAll = async () => { | |
| if (inFlight) return; | |
| inFlight = true; | |
| const seq = ++requestSeq; | |
| const next: Record<string, ConvergenceRuntimeState | null> = {}; | |
| try { | |
| await Promise.all( | |
| memberIds.map(async (prId) => { | |
| try { | |
| next[prId] = await window.ade.prs.convergenceStateGet(prId); | |
| } catch { | |
| next[prId] = null; | |
| } | |
| }), | |
| ); | |
| if (!cancelled && seq === requestSeq) setConvergenceByPrId(next); | |
| } finally { | |
| inFlight = false; | |
| } | |
| }; | |
| void fetchAll(); | |
| const interval = setInterval(() => { | |
| if (!cancelled) void fetchAll(); | |
| }, 5000); | |
| return () => { | |
| cancelled = true; | |
| clearInterval(interval); | |
| }; | |
| }, [memberPrIdsKey]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/tabs/QueueTab.tsx` around lines 461
- 495, The polling can overwrite newer state because fetchAll() calls overlap;
replace the setInterval approach in the effect (the const fetchAll, interval,
cancelled logic) with a self-scheduling loop that prevents concurrent runs:
implement a single-run loop using an async function (e.g., runLoop) that awaits
fetchAll() then waits 5000ms via setTimeout before re-invoking itself, or add an
inFlight boolean guard inside fetchAll to return early if a previous invocation
is still running; ensure you still respect the cancelled flag, clear the pending
timeout in the cleanup, and keep setting state only from the latest completed
run (use the cancelled flag or a sequence id) so setConvergenceByPrId and
window.ade.prs.convergenceStateGet calls cannot overlap and race.
Comment on lines
+711
to
+714
| delete from queue_landing_state; | ||
|
|
||
| insert into kv (key, value) values (?, ?) on conflict(key) do update set value = excluded.value; | ||
|
|
There was a problem hiding this comment.
Avoid unconditional queue-state purge in bootstrap migration.
At Line 711, delete from queue_landing_state; wipes all in-flight queue automation state. In a migration-style bootstrap script, this causes existing users to lose resumable queue progress on upgrade/restart.
💡 Suggested fix
-delete from queue_landing_state;
-
insert into kv (key, value) values (?, ?) on conflict(key) do update set value = excluded.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Resources/DatabaseBootstrap.sql` around lines 711 - 714, The
unconditional "delete from queue_landing_state;" removes all in-flight queue
progress; instead remove that statement and implement a safe, idempotent
migration: either skip deleting entirely or perform a conditional purge (e.g.,
DELETE FROM queue_landing_state WHERE updated_at < ? OR state IN
('invalid','stale')) or add a migration flag column and only remove rows tied to
the old schema; update the bootstrap script to preserve existing rows and, if
needed, add an explicit, well-documented selective cleanup step that targets
only truly orphaned or stale records in queue_landing_state.
Comment on lines
+1048
to
+1075
| private func stopPathToMerge() { | ||
| guard !isPathToMergeBusy else { return } | ||
| Task { @MainActor in | ||
| isPathToMergeBusy = true | ||
| defer { isPathToMergeBusy = false } | ||
| do { | ||
| let result = try await syncService.stopPathToMerge(prId: prId, reason: "Stopped from iOS.") | ||
| if let runtime = result.runtime { | ||
| applyConvergenceRuntime(runtime) | ||
| } | ||
| actionMessage = "Path to Merge stopped." | ||
| } catch { | ||
| errorMessage = error.localizedDescription | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Replace the runtime row inside `issueInventory` (if any) with `next`. | ||
| /// Used by the start/stop handlers so the mode toggle flips immediately. | ||
| private func applyConvergenceRuntime(_ next: ConvergenceRuntimeState) { | ||
| guard let current = issueInventory else { return } | ||
| issueInventory = IssueInventorySnapshot( | ||
| prId: current.prId, | ||
| items: current.items, | ||
| convergence: current.convergence, | ||
| runtime: next | ||
| ) | ||
| } |
There was a problem hiding this comment.
Mode strip can remain stuck on "Auto-Converge" after a successful stop when result.runtime is nil.
stopPathToMerge() only calls applyConvergenceRuntime when the server returns a runtime object. When result.runtime is nil (explicitly handled by if let), the cached issueInventory.runtime.autoConvergeEnabled is never set to false, so isAutoConverge stays true and the mode strip still renders "Auto-Converge" as the selected pill despite the confirmed stop. The user sees "Path to Merge stopped." but the toggle doesn't visually flip.
The sibling stopAiResolver (lines 1001–1024) avoids this by manually patching the local aiResolution state even when the server returns no data.
A symmetric fix — patch the existing runtime optimistically when no server runtime is returned, and fall back to a lightweight reload if there is no existing runtime to patch:
🛠️ Proposed fix
let result = try await syncService.stopPathToMerge(prId: prId, reason: "Stopped from iOS.")
if let runtime = result.runtime {
applyConvergenceRuntime(runtime)
+ } else if var existingRuntime = issueInventory?.runtime {
+ existingRuntime.autoConvergeEnabled = false
+ applyConvergenceRuntime(existingRuntime)
}
actionMessage = "Path to Merge stopped."The same applyConvergenceRuntime guard-return when issueInventory == nil also means startPathToMerge won't flip the mode strip if the inventory hasn't been fetched yet (narrow race while live). Both cases can be addressed with the same pattern, or alternatively by calling await reload(refreshRemote: false) as a fallback at the end of each operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/PRs/PrDetailScreen.swift` around lines 1048 - 1075,
stopPathToMerge currently only updates local UI when result.runtime exists which
leaves issueInventory.runtime.autoConvergeEnabled true if the server returns
nil; change stopPathToMerge (and symmetrically startPathToMerge) so when
result.runtime is nil you optimistically patch the local runtime: if
issueInventory != nil create a new ConvergenceRuntimeState by copying the
existing issueInventory.runtime (or creating a minimal default runtime) with
autoConvergeEnabled set to false (or true for start) and call
applyConvergenceRuntime(next); if issueInventory is nil call await
reload(refreshRemote: false) on the MainActor as a fallback so the mode strip is
consistent; use the existing symbols stopPathToMerge, startPathToMerge,
applyConvergenceRuntime, issueInventory, and ConvergenceRuntimeState to locate
and implement the change.
arul28
added a commit
that referenced
this pull request
May 4, 2026
Admin rollback of accidental Path to Merge landing. Reverts PR #241 so the PtM work can be reopened as a reviewed v2 PR.
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR introduces the Path-to-Merge orchestrator — a native TypeScript port of the
/shipLanestate machine — plus Graphite-style stacked-base queue PR creation, base-retargeting after each queue land, and a new suite of pipeline-settings flags (--conflict-strategy,--force-finalize,--early-merge-on-green) exposed through both the CLI and iOS sync commands.prs.pathToMerge.startandprs.pathToMerge.stopare registered withviewerAllowed: trueinsyncRemoteCommandService.ts, allowing read-only iOS clients to arm or disarm the automated merge loop; all other write-path PR commands omit this flag.earlyMergeOnGreen: trueandautoMerge: falseand CI is green,parkConvergedkeeps schedulingwaitingOnReviewwakes without ever settingautoConvergeEnabled: false, so the loop never sleeps permanently.savePathToMergeArgs: CallingsaveConvergenceRuntimeState(prId, {})then a separateUPDATE … SET ptm_args_jsonleaves a window where a concurrent convergence write could be silently overwritten.Confidence Score: 3/5
Not safe to merge until the viewer-allowed privilege escalation on pathToMerge start/stop is resolved.
One P1 security finding (viewer-allowed write commands) caps the score at 4; two P2 logic issues (infinite reschedule, fragile two-step upsert) pull it below the ceiling to 3.
apps/desktop/src/main/services/sync/syncRemoteCommandService.ts (viewerAllowed P1), apps/desktop/src/main/services/prs/pathToMergeOrchestrator.ts (infinite reschedule, conflict bypass), apps/desktop/src/main/services/prs/issueInventoryService.ts (two-step upsert race)
Security Review
prs.pathToMerge.start/prs.pathToMerge.stop: Both sync commands are registered withviewerAllowed: trueinsyncRemoteCommandService.ts. These commands launch or halt an automated merge loop that dispatches AI fix agents, performs Git operations, and ultimately merges code. Viewer-level iOS/remote callers can trigger these write operations, violating the established pattern where all other state-mutating PR commands require owner access.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Start([startPathToMerge]) --> SaveState[Save runtime: launching/scheduled] SaveState --> SaveArgs[Persist run args to DB] SaveArgs --> SetImmediate[setImmediate → runIteration] SetImmediate --> CheckEnabled{autoConvergeEnabled?} CheckEnabled -- no --> Exit([clearTimer & exit]) CheckEnabled -- yes --> Refresh[Refresh PR from GitHub] Refresh --> CheckMerged{PR merged?} CheckMerged -- yes --> RunCleanup[runPostMergeCleanup] --> MarkMerged([status=merged]) CheckMerged -- no --> CheckBehind{behindBaseBy > 0?} CheckBehind -- yes --> ApplyConflict[applyConflictStrategy: base_advance] ApplyConflict -- paused/failed --> PauseLoop([pauseLoop]) ApplyConflict -- ok --> Step2 CheckBehind -- no --> Step2 Step2{earlyMergeOnGreen AND CI passing AND review clean?} Step2 -- yes autoMerge=false --> ParkConverged[parkConverged ⚠️ autoConvergeEnabled stays true → schedules waitingOnReview FOREVER] Step2 -- yes autoMerge=true --> MergeLadder1[runMergeLadder] Step2 -- no --> TerminalGate MergeLadder1 -- merged --> MarkMerged MergeLadder1 -- conflict --> ApplyConflict2[applyConflictStrategy: merge_time] --> Reschedule([schedule justPushed]) MergeLadder1 -- blocked --> TerminalGate MergeLadder1 -- failed --> PauseLoop TerminalGate{CI + review both terminal?} TerminalGate -- no --> Warming([schedule warming 720s]) TerminalGate -- yes --> HardCap{completedRounds >= maxRounds?} HardCap -- no --> DispatchAgent[launchPrIssueResolutionChat] HardCap -- yes forceFinalizeUsed --> PauseLoop HardCap -- yes not used --> ForceFinalize{decideForceFinalize} ForceFinalize -- skip --> PauseLoop ForceFinalize -- run, CI green --> MergeLadder2[runMergeLadder force-finalize] ForceFinalize -- run, CI not green --> DispatchAgent MergeLadder2 -- merged --> MarkMerged MergeLadder2 -- blocked/failed --> FailLoop([failLoop]) DispatchAgent --> Reschedule style ParkConverged fill:#ffddaa,stroke:#cc8800 style PauseLoop fill:#ffcccc,stroke:#cc0000 style FailLoop fill:#ffcccc,stroke:#cc0000 style MarkMerged fill:#ccffcc,stroke:#008800Comments Outside Diff (4)
apps/desktop/src/main/services/sync/syncRemoteCommandService.ts, line 2965-2994 (link)Both
prs.pathToMerge.startandprs.pathToMerge.stopare registered withviewerAllowed: true, but these commands launch or halt an automated merge loop that directly lands code to the repository. A sync viewer (e.g. an iOS client with read-only access) can start the convergence engine on any PR, dispatching fix agents, performing rebases/merges, and ultimately merging to the base branch — all write operations that should require at minimum the same privilege as the owner who started the ADE session. Every other state-mutating command in this file (prs.startIssueResolution,prs.stopIssueResolution,prs.land, etc.) is registered withoutviewerAllowed, establishing the precedent. SettingviewerAllowed: truehere breaks that boundary.Prompt To Fix With AI
apps/desktop/src/main/services/prs/pathToMergeOrchestrator.ts, line 1843-1851 (link)earlyMergeOnGreenis true butautoMergeis falseWhen
earlyMergeOnGreen: trueandautoMerge: falseand CI is passing,parkConvergedis called: it writesstatus: "converged"but leavesautoConvergeEnabled: true, then callsschedule(prId, "waitingOnReview"). On the next wake (1 800 s later) the same conditions still hold, soparkConvergedfires again and schedules another wake — the loop never stops polling. If many PRs are in this state the timer backlog grows unboundedly. Contrast withpauseLoop, which explicitly setsautoConvergeEnabled: falseto break the cycle.Prompt To Fix With AI
apps/desktop/src/main/services/prs/pathToMergeOrchestrator.ts, line 1623-1643 (link)--adminrung; conflict strategy is bypassedThe REST rung catches conflicts via
/conflict|409/i.test(restErr)and returns early (kind: "conflict"), which triggersapplyConflictStrategy. However, the--adminrung only checksadminRes.exitCode === 0; ifgh pr merge --adminfails because of a merge conflict, the error text is logged as a warning and execution falls through to the--autorung (or toblocked). The caller'sapplyConflictStrategypath is never invoked, so the user's configuredconflictStrategy(rebase / merge / auto) is not applied.Prompt To Fix With AI
apps/desktop/src/main/services/prs/issueInventoryService.ts, line 543-552 (link)savePathToMergeArgsmay clobber concurrent convergence statesavePathToMergeArgscallssaveConvergenceRuntimeState(prId, {})to ensure the row exists, then issues a separateUPDATE … SET ptm_args_json = ?. IfsaveConvergenceRuntimeStateperforms a fullINSERT … ON CONFLICT DO UPDATE, the first statement re-writes all convergence columns, then theUPDATEpatchesptm_args_json. Between the two SQL statements any concurrent write (e.g., from an in-flightsaveConvergenceRuntimecall on a different async turn) could be silently overwritten. A singleINSERT … ON CONFLICT DO UPDATE SET ptm_args_json = ?that touches only the one column would eliminate this window entirely.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Persist Path to Merge pipeline controls" | Re-trigger Greptile